diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 869e023..07ba513 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -233,6 +233,15 @@ one means. Region drawing, shape annotations and snapshot export are *optional* a host that omits one turns that feature off, affordances included, rather than presenting a control that does nothing. +Both canvases declare deck.gl's camera controller **on the view** they hand to +``, never through its `controller` prop. Deck copies that prop onto +the first view only when the prop is truthy (`Deck._getViews`, "Backward compatibility: +support controller prop"), and it writes it onto the view instance in place — so a +memoized view keeps whatever controller an earlier render gave it, and `controller={false}` +never takes it back. That silently disabled `lock_view`: every other branch is a truthy +options object, so the lock was the only setting affected. Keep the controller inside the +`useMemo` that builds the view, with the lock in its dependency list. + `frontend/src/components/StudioCanvasHost.tsx` is the app's implementation and the only place the store and the canvas meet: it reads the store, `useEditGate()` and `hooks/useDisplayPersistence.ts` (the optimistic store write + the 500 ms debounced @@ -640,9 +649,10 @@ Tests: `src/lib/urlViewState.test.ts` (vitest, `npm run test -w spatial-data-stu covers the encoder. That one vitest run covers both workspaces — `frontend/vite.config.ts` includes `../packages/viewer/src/**/*.test.ts`, since the canvas library has no runner of its own. Also `e2e/serverless-share.spec.ts` covers the wiring by sharing a link -between two browser contexts. The e2e drives the camera through the zoom buttons — -`onZoom` writes the viewport directly, bypassing deck's controller, which synthetic drag -and wheel events never reach. +between two browser contexts. That spec drives the camera through the zoom buttons, +which is the simpler lever: `onZoom` writes the viewport directly and does not depend on +deck's controller being live. A synthetic wheel *does* reach the controller, though — +`e2e/lock-view.spec.ts` turns on that fact to prove the camera is frozen. ## Documentation site @@ -823,6 +833,10 @@ collection as well as embeddable per page. Pull requests build but do not publis loaded, since half the tour's targets only exist in one of the two. The webServer entries reuse whatever already listens on 5173/8000, so make sure those are this app's servers and not another project's. + `e2e/lock-view.spec.ts` covers `lock_view` through the embed protocol: it locks a + canvas that is already live (the sequence a host's inspector produces, and the only + one that catches a stale deck controller — loaded pre-locked, there is no stale + controller to survive) and asserts the wheel stops producing `display-changed`. ## Test datasets diff --git a/frontend/e2e/lock-view.spec.ts b/frontend/e2e/lock-view.spec.ts new file mode 100644 index 0000000..71b61c1 --- /dev/null +++ b/frontend/e2e/lock-view.spec.ts @@ -0,0 +1,115 @@ +// `lock_view` freezes the camera against pointer input, including when it is turned on +// while the canvas is already live. +// +// That last part is the whole point. deck.gl copies its `controller` prop onto the first +// view only when the prop is truthy (`Deck._getViews`, "Backward compatibility: support +// controller prop") and writes it onto the view instance in place, so a memoized view +// kept the controller a previous render had given it and `controller={false}` never took +// it back. The canvases declare the controller on the view itself now. +// +// A unit test cannot see this: the bug was in *where* the controller was passed, not in +// what the code computed. Nor can a pre-locked share link — loaded locked, the controller +// is false from the first render and there is no stale `true` to survive, so such a test +// passes against the broken code too. It has to be toggled at runtime on a live canvas, +// which is what the Cirro dashboard's inspector does over the embed protocol. +// +// Embed mode renders no in-canvas controls, so the camera is observed through the +// protocol instead: the viewer emits `display-changed` (debounced 500ms) whenever a pan +// or zoom moves the active display's viewport. Applying a display never echoes one back +// (the protocol's echo guard), so any event after an apply came from the camera. +import path from 'node:path'; +import { expect, test, type Page } from '@playwright/test'; + +// The smallest demo checkpoint carrying an image layer; this spec is about the camera, +// and the 32MB Xenium one spends minutes loading cells nothing here reads. +const CHECKPOINT = `/@fs${path.resolve(process.cwd(), '..', 'docs-site/viewer-data/visium-mouse-brain.sdata.zarr.zip')}`; +const OPEN = `/?checkpoint=${encodeURIComponent(CHECKPOINT)}&embed=1`; + +// A cold load parses the archive and fits the camera before `ready`; the suite's default +// per-assertion timeout is not enough for that on a cold Vite cache. +const READY_MS = 120_000; +// Comfortably past the viewer's 500ms `display-changed` debounce. +const SETTLE_MS = 3_000; + +test.setTimeout(300_000); + +interface DisplayPayload { + kind: string; + encoding: Record; + viewport: unknown; +} + +/** Collect every viewer -> parent message. In embed mode at the top level + * `window.parent === window`, so the bridge's posts land back on this page. */ +async function collectEmbedMessages(page: Page): Promise { + await page.addInitScript(() => { + (window as unknown as { __sds: unknown[] }).__sds = []; + window.addEventListener('message', (e: MessageEvent) => { + const msg = e.data as { source?: string } | null; + if (msg && msg.source === 'sds-embed') (window as unknown as { __sds: unknown[] }).__sds.push(msg); + }); + }); +} + +const messagesOfType = (page: Page, type: string) => page.evaluate( + (t) => (window as unknown as { __sds: { type: string }[] }).__sds.filter((m) => m.type === t), + type, +); + +const clearMessages = (page: Page) => page.evaluate(() => { + (window as unknown as { __sds: unknown[] }).__sds = []; +}); + +async function applyDisplay(page: Page, display: DisplayPayload): Promise { + await page.evaluate((d) => { + window.postMessage( + { source: 'cirro-dashboard', version: 1, type: 'apply-display', display: d }, + '*', + ); + }, display); + await page.waitForTimeout(1_000); +} + +/** Scroll-zoom over the middle of the canvas — pointer input, which only deck's + * controller can act on. */ +async function wheelOverCanvas(page: Page): Promise { + const box = (await page.locator('canvas').first().boundingBox())!; + await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); + await page.mouse.wheel(0, -400); + await page.waitForTimeout(SETTLE_MS); +} + +test('locking a live canvas stops the camera, and unlocking gives it back', async ({ page }) => { + await collectEmbedMessages(page); + await page.goto(OPEN); + await expect.poll(async () => (await messagesOfType(page, 'ready')).length, { timeout: READY_MS }) + .toBeGreaterThan(0); + + // Control: the wheel reaches deck's controller at all, and a camera move is observable. + await clearMessages(page); + await wheelOverCanvas(page); + const moved = await messagesOfType(page, 'display-changed') as { display: DisplayPayload }[]; + expect(moved.length, 'the wheel should move the camera while unlocked').toBeGreaterThan(0); + + // Lock the display that is on screen, carrying its current viewport so the lock is the + // only thing that changes. This is the sequence that broke: the canvas has been live + // and interactive, so its view already carries a controller. + const live = moved[moved.length - 1].display; + await applyDisplay(page, { ...live, encoding: { ...live.encoding, lock_view: true } }); + + await clearMessages(page); + await wheelOverCanvas(page); + expect( + await messagesOfType(page, 'display-changed'), + 'the wheel must not move the camera while the view is locked', + ).toHaveLength(0); + + // Control: the canvas is still live, and the lock releases. + await applyDisplay(page, { ...live, encoding: { ...live.encoding, lock_view: false } }); + await clearMessages(page); + await wheelOverCanvas(page); + expect( + await messagesOfType(page, 'display-changed'), + 'unlocking should give the camera back', + ).not.toHaveLength(0); +}); diff --git a/frontend/e2e/serverless-share.spec.ts b/frontend/e2e/serverless-share.spec.ts index 426fb97..4bf88f9 100644 --- a/frontend/e2e/serverless-share.spec.ts +++ b/frontend/e2e/serverless-share.spec.ts @@ -4,10 +4,10 @@ // wiring the unit tests can't see — that edits reach the URL, and that a fresh page // load rebuilds the same view from it. // -// The deck.gl canvas is not drivable by automation (synthetic drag and wheel events -// never reach deck's controller), so everything here goes through real DOM controls. -// The zoom buttons are the lever for the camera: `onZoom` sets React view state and -// writes the viewport directly, bypassing the controller entirely. +// Everything here goes through real DOM controls. The zoom buttons are the simpler lever +// for the camera: `onZoom` sets React view state and writes the viewport directly, so it +// does not depend on deck's controller being live. (A synthetic wheel does reach the +// controller — see `lock-view.spec.ts`, which relies on that.) import path from 'node:path'; import { expect, test } from '@playwright/test'; diff --git a/frontend/package.json b/frontend/package.json index 7394c10..10d553f 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,7 +1,7 @@ { "name": "spatial-data-studio-frontend", "private": true, - "version": "0.1.10", + "version": "1.0.1", "type": "module", "scripts": { "dev": "vite", diff --git a/package.json b/package.json index 69fad8c..9418d61 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "spatial-data-studio", "private": true, - "version": "0.1.10", + "version": "1.0.1", "workspaces": [ "packages/viewer", "frontend", diff --git a/packages/viewer/package.json b/packages/viewer/package.json index 20fbaf4..59887c2 100644 --- a/packages/viewer/package.json +++ b/packages/viewer/package.json @@ -1,6 +1,6 @@ { "name": "@cirrobio/spatial-viewer", - "version": "0.1.10", + "version": "1.0.1", "description": "Host-agnostic WebGL spatial/embedding canvases and the .zarr.zip checkpoint reader from Spatial Data Studio.", "license": "SEE LICENSE IN LICENSE.md", "sideEffects": false, diff --git a/packages/viewer/src/canvas/EmbeddingCanvas.tsx b/packages/viewer/src/canvas/EmbeddingCanvas.tsx index bb15714..9f6afae 100644 --- a/packages/viewer/src/canvas/EmbeddingCanvas.tsx +++ b/packages/viewer/src/canvas/EmbeddingCanvas.tsx @@ -129,10 +129,6 @@ export default function EmbeddingCanvas({ const legendVisible = display.encoding.legend_visible !== false; const legendTitle = display.encoding.legend_title || colorByLabel(colorByPath); - const views = useMemo( - () => (is_3d ? [new OrbitView({ id: 'main' })] : [new OrthographicView({ id: 'main', flipY: false })]), - [is_3d], - ); const layers = useMemo(() => { if (!positions || !colors) return [] as Layer[]; @@ -304,6 +300,26 @@ export default function EmbeddingCanvas({ // Bound once so the 3D overlay's handle markers below read a narrowed shape. const selectionShape = selection.shape; + // Declared on the view, NOT as DeckGL's `controller` prop: deck copies that prop onto + // a view only when it is truthy (`Deck._getViews`, "Backward compatibility: support + // controller prop"), mutating the view instance in place, so a memoized view keeps the + // controller an earlier render gave it and `controller={false}` never takes it back. + // Lock view did nothing as a result; the other branches are truthy objects and worked. + const controller = useMemo( + () => ((display.encoding.lock_view ?? EMBEDDING_ENCODING_DEFAULTS.lock_view) ? false + // A shape gesture owns the pointer, so the camera must not also claim it — + // dragRotate too, since the orbit view spends left-drag on rotation. + : selection.interacting ? { dragPan: false, dragRotate: false, doubleClickZoom: false } + : lassoMode ? { doubleClickZoom: false } : true), + [display.encoding.lock_view, selection.interacting, lassoMode], + ); + const views = useMemo( + () => (is_3d + ? [new OrbitView({ id: 'main', controller })] + : [new OrthographicView({ id: 'main', flipY: false, controller })]), + [is_3d, controller], + ); + if (!viewState) { return (
@@ -341,11 +357,6 @@ export default function EmbeddingCanvas({ onDrag={selection.onDrag} onDragEnd={selection.onDragEnd} layers={[...layers, ...drawLayers]} - controller={(display.encoding.lock_view ?? EMBEDDING_ENCODING_DEFAULTS.lock_view) ? false - // A shape gesture owns the pointer, so the camera must not also claim it — - // dragRotate too, since the orbit view spends left-drag on rotation. - : selection.interacting ? { dragPan: false, dragRotate: false, doubleClickZoom: false } - : lassoMode ? { doubleClickZoom: false } : true} getCursor={lassoMode ? () => selection.cursor : ({ isDragging }) => (isDragging ? 'grabbing' : 'grab')} /> diff --git a/packages/viewer/src/canvas/SpatialCanvas.tsx b/packages/viewer/src/canvas/SpatialCanvas.tsx index 45be2c0..4d832f9 100644 --- a/packages/viewer/src/canvas/SpatialCanvas.tsx +++ b/packages/viewer/src/canvas/SpatialCanvas.tsx @@ -183,10 +183,6 @@ export default function SpatialCanvas({ const invertX = display.encoding.invert_x ?? SPATIAL_ENCODING_DEFAULTS.invert_x; const invertY = display.encoding.invert_y ?? SPATIAL_ENCODING_DEFAULTS.invert_y; const bg = display.encoding.background ?? SPATIAL_ENCODING_DEFAULTS.background; - const views = useMemo( - () => [new FlipOrthographicView({ id: 'main', flipX: invertX, flipY: invertY })], - [invertX, invertY], - ); // Polygon draw state lives on the host so the active tab's left panel owns the // commit / apply / clear actions; the canvas is purely the drawing surface. Absent // (a host that offers no region drawing) the lasso stays disarmed. @@ -742,6 +738,25 @@ export default function SpatialCanvas({ const layerNames = fields?.layers ?? []; const colorByName = colorByLabel(colorByPath); + // The controller is declared on the view, NOT as DeckGL's `controller` prop. Deck only + // copies that prop onto a view when the prop is truthy (`Deck._getViews`, "Backward + // compatibility: support controller prop"), and it writes it onto the view instance in + // place — so a memoized view keeps whatever controller an earlier render gave it, and + // `controller={false}` never takes it back. Lock view therefore did nothing at all + // unless the view happened to be re-created; the other branches below are truthy + // objects, so only the lock was affected. `viewState` is controlled, so re-minting the + // view when this changes leaves the camera where it is. + const controller = useMemo( + () => (lockView ? false + : shapeInteracting || selection.interacting ? { dragPan: false, doubleClickZoom: false } + : drawMode ? { doubleClickZoom: false } : true), + [lockView, shapeInteracting, selection.interacting, drawMode], + ); + const views = useMemo( + () => [new FlipOrthographicView({ id: 'main', flipX: invertX, flipY: invertY, controller })], + [invertX, invertY, controller], + ); + if (!viewState) { return (
@@ -762,9 +777,6 @@ export default function SpatialCanvas({ persistDisplay({ ...currentSpec(), viewport: { target: [t[0], t[1]], zoom: v.zoom as number } }); }} layers={[...layers, ...drawLayers, ...shapeLayers]} - controller={lockView ? false - : shapeInteracting || selection.interacting ? { dragPan: false, doubleClickZoom: false } - : drawMode ? { doubleClickZoom: false } : true} onClick={handleClick} onHover={shapesMode ? handleHover : lassoMode ? selection.onHover : undefined} onDragStart={(info) => { handleShapeDragStart(info); selection.onDragStart(info); }}