fix(annotator): a save stores the work without moving the camera - #485
Merged
Conversation
`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
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
AnnotatorCanvasholds the viewport in its own state and resets it from the initial-fitlayout effect:
assetissnapshot.document.asset, sofit's identity moves whenever the documentobject is replaced — not when the frame changes. A save replaces it:
useSaveAnnotationsinvalidates the asset's annotations query (deliberately — the reloadis what turns client-minted ids into the kernel's), the refetch's payload really is
different, so
loadedis a new array,AnnotationPage'suseMemobuilds a new store,and
documentFromWiremints a freshAssetDescriptor. New object, newfit, effectfires, camera resets.
It is a near relative of the recorded precedent, not that precedent. The
ui-capabilitiesskill's rule — a query key naming a value the page can change is anunmount trigger — was the prime suspect and is not what this is: nothing unmounts.
Workspace's key isasset.idalone, which is correct and unchanged, and the store's ownuseMemois keyed on values rather than on a query. What moved was an object identity auseCallbackdependency list treats as a change, so the reset survived a component thatnever 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.
AssetDescriptoris exactly{id, width, height},so this is lossless.
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 becausereact-hooks/exhaustive-depsis anerrorin this package and reports an unnecessarydependency 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, theimperative
fit()onviewRef, the wheel, the pinch and the pan are all untouched.Test
frontend/app/e2e/annotate.spec.ts→saving leaves the viewport exactly where it was.Chromium, not jsdom, and structurally so:
getBoundingClientRectanswers 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>'son-screen box —
_frame.ts's idiom, since that element is laid out at native size insidethe
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:
(100% is the fit for this fixture's 640×480 asset in the suite's pane —
fitToViewportnever enlarges.)
Mutation verification
Reverting the fix must turn that test red again, so it was reverted and it did. The
mutation puts
assetback intofit's dependency list — the minimal, exact restoration ofthe defect — with the anchor asserted unique before the patch and present after, and the
revert applied as
git apply -Ron 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.
assetback infit's dependency listsaving leaves the viewport exactly where it was(chromium)Expected: "448%"/Received: "100%", the original failure verbatimLocal 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:pytest tests/architecture0pytest tests/cli0pytest tests/examples0pytest tests/fixtures0pytest tests/formats0pytest tests/inference0pytest tests/jobs0pytest tests/kernel0pytest tests/mcp0pytest tests/packaging0pytest tests/scripts5pytest tests/server0pytest tests/test_versioning.py0ruff check .0ruff format --check .0mypy src/visionset/kernel0lint-imports0check.sh frontend generated0check.sh browser0— 238 e2e passed (was 237), 1 cycle passedThe annotator's three boundary gates are inside the frontend stage and all pass: ESLint's
no-restricted-imports/no-restricted-globalsoversrc/core/, andtsconfig.core.json's no-DOM compile. This change is insrc/adapters/react/, which iswhere the DOM is allowed to live.
tests/scriptsexits 5 by design — nothing pytest-shaped lives there; it isnode --testand runs under
check.sh generated.Found, not fixed
line, and deliberate: the kernel mints its own annotation ids, the page refetches to
learn them, and a rebuilt
AnnotatorStorestarts with an empty command log. That is thispage'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 ratherthan 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.