Fix lock_view: deck's camera controller must live on the view, not the DeckGL prop - #4
Merged
Merged
Conversation
deck.gl copies its `controller` prop onto the first view only when that prop is truthy
(`Deck._getViews`, "Backward compatibility: support controller prop"), and it writes it
onto the view instance in place. Both canvases memoize their view on something unrelated
to the camera — `[invertX, invertY]` and `[is_3d]` — so the instance outlives the render
that set `controller: true`, and switching to `controller={false}` never took it back.
Lock view was the only setting this reached: the other branches are truthy options
objects, which the shim happily copies. Measured against a Xenium checkpoint in a Cirro
dashboard tile, a wheel-zoom plus drag with the lock ON moved the canvas as much as with
it off (mean |pixel diff| 53.9 vs 49.0 against an idle noise floor of 0.011); with the
controller declared on the view it drops to 0.4, which is the hover highlight rather than
the camera.
`viewState` is controlled in both canvases, so re-minting the view when the controller
changes leaves the camera where the user put it.
Co-Authored-By: Claude Opus 5 <[email protected]>
The regression only appears when the lock is turned on while the canvas is already live, which is what a host's inspector does — loaded pre-locked, `controller` is false from the first render and there is no stale `true` to survive, so a share-link test passes against the broken code. Embed mode is the one place this repo can drive that sequence: `apply-display` toggles `lock_view` at runtime, and `display-changed` reports camera moves, so the spec asserts the wheel stops producing them. Verified both ways: against the previous canvases the spec fails with a `display-changed` carrying `lock_view: true` and a moved viewport; against these it passes. Two controls keep it from passing vacuously — the wheel must move an unlocked camera, and unlocking must give it back. Other: - `DEVELOPMENT.md` and `serverless-share.spec.ts` claimed synthetic wheel events never reach deck's controller. They do; the zoom buttons are simply the easier lever there. Co-Authored-By: Claude Opus 5 <[email protected]>
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.
lock_viewdid nothing when it was turned on while a canvas was already live — reported against a Cirro dashboard tile, where the inspector's "Lock view (no zoom or pan)" left the camera fully pannable and zoomable.deck.gl only copies its
controllerprop onto a view when that prop is truthy, and it mutates the view instance in place (@deck.gl/core/dist/lib/deck.js):Both canvases memoize their view on something unrelated to the camera —
[invertX, invertY]inSpatialCanvas,[is_3d]inEmbeddingCanvas— so the instance outlives the render that wrotecontroller: true, and switching tocontroller={false}never took it back. The lock was the only setting this reached: every other branch ({ dragPan: false, … }) is a truthy options object, which the shim copies happily.Changes
<DeckGL views={…}>and drop thecontrollerprop. The view memos moved below the state they now depend on.viewStateis controlled in both, so re-minting the view leaves the camera where the user put it.e2e/lock-view.spec.tsguards it through the embed protocol —apply-displaytoggleslock_viewon a live canvas anddisplay-changedreports camera moves.1.0.1in all three manifests, per the release rule inCLAUDE.md. Note they read0.1.10whilev1.0.0sits on the same commit, so this also closes that drift.Verification
Measured in a Cirro dashboard tile against a Xenium checkpoint — mean absolute pixel difference across the canvas for a wheel-zoom plus drag, against an idle no-gesture noise floor:
1.0.0On
1.0.0a locked gesture moves the camera as much as an unlocked one. Here it drops to the noise floor; the 1.1% residual is the hover highlight, not the camera.e2e/lock-view.spec.tswas checked in both directions. Against the previous canvases it fails with adisplay-changedcarryinglock_view: trueand a moved viewport; against these it passes. Two controls keep it from passing vacuously: the wheel must move an unlocked camera, and unlocking must give it back. A pre-locked share link is not sufficient — loaded locked,controlleris false from the first render, there is no staletrueto survive, and such a test passes against the broken code.Also:
viewer typecheckclean,npm run test -w spatial-data-studio-frontend51/51, fullnpm run buildclean.eslintis not installed in my checkout, so lint did not run.Other:
DEVELOPMENT.mdandserverless-share.spec.tsboth claimed synthetic wheel events never reach deck's controller. They do — the zoom buttons are simply the easier lever there — and that claim is what would stop the next person writing this test.serverless-share.spec.ts(both tests) fails on a clean checkout because it points atdocs-site/viewer-data/fluorescence-section.sdata.zarr.zip, which is neither on disk nor tracked by git. Not touched here.This needs a
v1.0.1tag frommainafter merge for the built asset to reach anything; the Cirro-portal side is then a one-line bump of the@cirrobio/spatial-viewertarball URL.🤖 Generated with Claude Code