Skip to content

fix(annotator): a save stores the work without moving the camera - #485

Merged
JArmandoAnaya merged 1 commit into
mainfrom
fix/viewport-survives-save
Aug 10, 2026
Merged

fix(annotator): a save stores the work without moving the camera#485
JArmandoAnaya merged 1 commit into
mainfrom
fix/viewport-survives-save

Conversation

@JArmandoAnaya

Copy link
Copy Markdown
Contributor

Closes #482.

Zoom into a detail, place a box, press Save and stay — and the stage jumped back to
the fitted view. The work was stored; where the person was looking was not. On a 4K frame
that means finding the detail again by hand after every save.

The mechanism, not the symptom

AnnotatorCanvas holds the viewport in its own state and resets it from the initial-fit
layout effect:

const fit = useCallback(() => {  fitToViewport(asset, ) }, [asset, applyViewport]);
useLayoutEffect(fit, [fit]);

asset is snapshot.document.asset, so fit's identity moves whenever the document
object
is replaced — not when the frame changes. A save replaces it:
useSaveAnnotations invalidates the asset's annotations query (deliberately — the reload
is what turns client-minted ids into the kernel's), the refetch's payload really is
different, so loaded is a new array, AnnotationPage's useMemo builds a new store,
and documentFromWire mints a fresh AssetDescriptor. New object, new fit, effect
fires, camera resets.

It is a near relative of the recorded precedent, not that precedent. The
ui-capabilities skill's rule — a query key naming a value the page can change is an
unmount trigger
— was the prime suspect and is not what this is: nothing unmounts.
Workspace's key is asset.id alone, which is correct and unchanged, and the store's own
useMemo is keyed on values rather than on a query. What moved was an object identity a
useCallback dependency list treats as a change, so the reset survived a component that
never remounted — which is why it also could not be found by looking for the unmount.

It also explains why this only ever happened on a save. TanStack Query shares its results
structurally, so a background refetch returning identical JSON returns the same array,
the memo holds, and no camera moves. Only a write, which by definition changes the
payload, trips it.

The fix, at the layer that owns the state

A fit is a function of the frame — an id and a size — so it depends on those three numbers
and not on the object carrying them. AssetDescriptor is exactly {id, width, height},
so this is lossless.

const { id: assetId, width: assetWidth, height: assetHeight } = asset;
const fit = useCallback(() => {
  
  applyViewport(fitToViewport({ id: assetId, width: assetWidth, height: assetHeight }, ));
}, [assetId, assetWidth, assetHeight, applyViewport]);

The effect now fires when the picture changes and at no other time, which is what it
always meant. This is not a throttle on an effect that was otherwise right: a document
rebuild is not a reason to move a camera, and an equality check on the object could never
have told the two apart.

The primitives are fit's own dependencies rather than the effect's because
react-hooks/exhaustive-deps is an error in this package and reports an unnecessary
dependency as loudly as a missing one — so the honest spelling is a callback whose
identity already tracks the right thing.

The invalidation stays. A 200 means the write is durable and the UI may refresh what
it shows; refreshing data and resetting the camera are different things. mod+0, the
imperative fit() on viewRef, the wheel, the pinch and the pan are all untouched.

Test

frontend/app/e2e/annotate.spec.tssaving leaves the viewport exactly where it was.
Chromium, not jsdom, and structurally so: getBoundingClientRect answers all zeros there,
so there is no fit to disturb, no wheel notch to apply and no pan to measure — a component
test would pass with the bug fully present.

It zooms off the fitted view over a point that is not the pane's centre (which moves zoom
and pan), pans again with a secondary drag, records the readout and the <svg>'s
on-screen box — _frame.ts's idiom, since that element is laid out at native size inside
the translate(pan) scale(zoom) wrapper, so its rect folds all of it into one measurement
— then draws a box, saves, waits for the rebuilt document rather than for the button, and
asserts both are unchanged. The readout is asserted beside the box because a zoom that
survived while the pan did not would otherwise read as a pass.

Red before the fix:

Error: expect(received).toBe(expected) // Object.is equality
  Expected: "448%"
  Received: "100%"
  at e2e/annotate.spec.ts:2880:64

(100% is the fit for this fixture's 640×480 asset in the suite's pane — fitToViewport
never enlarges.)

Mutation verification

Reverting the fix must turn that test red again, so it was reverted and it did. The
mutation puts asset back into fit's dependency list — the minimal, exact restoration of
the defect — with the anchor asserted unique before the patch and present after, and the
revert applied as git apply -R on the recorded diff rather than by checking out a path.
The work was committed first, so a revert could not take the implementation with it.

Mutation Guard Result
asset back in fit's dependency list saving leaves the viewport exactly where it was (chromium) red — Expected: "448%" / Received: "100%", the original failure verbatim

Local gate

Staged against this box's ~10-minute command ceiling, pytest split by directory derived
from ls tests/ at run time. Every stage's exit code:

Stage Exit
pytest tests/architecture 0
pytest tests/cli 0
pytest tests/examples 0
pytest tests/fixtures 0
pytest tests/formats 0
pytest tests/inference 0
pytest tests/jobs 0
pytest tests/kernel 0
pytest tests/mcp 0
pytest tests/packaging 0
pytest tests/scripts 5
pytest tests/server 0
pytest tests/test_versioning.py 0
ruff check . 0
ruff format --check . 0
mypy src/visionset/kernel 0
lint-imports 0
check.sh frontend generated 0
check.sh browser 0 — 238 e2e passed (was 237), 1 cycle passed

The annotator's three boundary gates are inside the frontend stage and all pass: ESLint's
no-restricted-imports / no-restricted-globals over src/core/, and
tsconfig.core.json's no-DOM compile. This change is in src/adapters/react/, which is
where the DOM is allowed to live.

tests/scripts exits 5 by design — nothing pytest-shaped lives there; it is node --test
and runs under check.sh generated.

Found, not fixed

  • The store's selection and undo history do not survive a save. Same rebuild, different
    line, and deliberate: the kernel mints its own annotation ids, the page refetches to
    learn them, and a rebuilt AnnotatorStore starts with an empty command log. That is this
    page's documented behaviour ("saving is a diff, and then a reload") rather than a defect,
    and AddClassDialog's step ordering exists to keep it from ever costing work rather
    than history. The full inventory of what else the rebuild reaches — nothing, as it turns
    out — is a comment on Saving resets the editor viewport (zoom/pan) #482.

`AnnotatorCanvas`'s initial fit ran again whenever the *document object* was
replaced, because `fit` closed over `snapshot.document.asset` and
`useLayoutEffect(fit, [fit])` keys on that callback's identity.
`documentFromWire` mints a fresh `AssetDescriptor` on every rebuild, and a save
rebuilds: the write is followed by a refetch so the kernel's own annotation ids
replace the client-minted ones, which is a materially different payload and so a
new array, a new store, a new document. Zoom into a detail, store the work, and
the stage jumped back to the fitted view.

A fit is a function of the frame — an id and a size — so it depends on those
three numbers now and not on the object carrying them. The effect fires when the
picture changes and at no other time, which is what it always meant. The
primitives are `fit`'s own dependencies rather than the effect's, because
`react-hooks/exhaustive-deps` is an error here and reports an unnecessary
dependency as loudly as a missing one.

Nothing else moves. The invalidation stays — a 200 means the write is durable and
the UI may refresh what it shows — and refreshing data is not the same act as
resetting a camera. An ordinary background refetch never tripped this: TanStack
Query shares structurally, so identical JSON returns the same array and the
memo above holds.
@JArmandoAnaya
JArmandoAnaya merged commit 9135411 into main Aug 10, 2026
13 checks passed
@JArmandoAnaya
JArmandoAnaya deleted the fix/viewport-survives-save branch August 10, 2026 04:27
JArmandoAnaya added a commit that referenced this pull request Aug 10, 2026
…th nothing unmounting (cf. #482) (#489)

The skill records that a query key naming a mutable value is an unmount
trigger. #482 was dispatched against that rule and the query key turned out to
be innocent: `AnnotatorCanvas`'s initial-fit layout effect re-fired because
`fit` depended on the `AssetDescriptor` object, which `documentFromWire` mints
afresh on every rebuild — so a save's refetch reset zoom and pan in a component
that never remounted. The hunt for an unmount that never happened is the cost
of the two mechanisms not being written down together.

They are adjacent now, with the tell that separates them: sibling state in the
same component. An unmount takes all of it and flashes a loading state on the
way; a re-fire disturbs only what that one hook writes and leaves everything
beside it untouched.

Also records the two habits that follow — depend on the values a hook is really
a function of rather than the object carrying them, and put the primitives in
the callback's dependency list rather than the effect's, since
`react-hooks/exhaustive-deps` is an error in `frontend/annotator` and refuses a
widened list.

The skill's `description` gains the state-lifetime clause, so the next agent
debugging a silently reset piece of view state finds this file by searching for
what they are actually looking at. cf. #485.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Saving resets the editor viewport (zoom/pan)

1 participant