diff --git a/.changeset/spatialcanvas-picking-perf-and-rules-of-react.md b/.changeset/spatialcanvas-picking-perf-and-rules-of-react.md new file mode 100644 index 00000000..535c3001 --- /dev/null +++ b/.changeset/spatialcanvas-picking-perf-and-rules-of-react.md @@ -0,0 +1,31 @@ +--- +"@spatialdata/vis": minor +"@spatialdata/layers": patch +"@spatialdata/react": patch +--- + +SpatialCanvas hover/picking performance and Rules-of-React cleanup. + +Picking/tooltip performance: + +- New `hoverTooltipMode` prop (`'off' | 'simple' | 'aggregate'`, default + `'simple'`) on `SpatialCanvas` and `SpatialCanvasViewer`, with a matching + selector in the `SpatialCanvas` UI. `'simple'` resolves the tooltip from the + single top-most pick deck.gl already does for hover/highlight; `'aggregate'` + adds `pickMultipleObjects` GPU passes to include every layer under the cursor + (more expensive); `'off'` makes shape layers non-pickable entirely (no + autoHighlight, no picking-buffer render) — the cheapest mode. Replaces the + earlier boolean `aggregateHoverTooltips`. +- Shape layers are made non-pickable (and `autoHighlight` disabled) while the + camera is being panned/zoomed, so deck.gl does not re-render the shape + geometry into the picking buffer during gestures. New `pickingEnabled` option + on the shapes layer (`@spatialdata/layers`) drives this. +- Hover tooltip resolution is suppressed while a pointer button is held (drag), + and the per-missing-layer supplemental aggregation pick is collapsed into a + single batched pick. + +Rules-of-React fixes (eslint-plugin-react-hooks, `pnpm lint:react` now clean and +the `react-lint` CI job is required): removed ref reads/writes during render and +replaced setState-in-effect patterns with derived state in `@spatialdata/react` +`useSpatialData` and the vis `Transforms`, `Table`, `Shapes`, `ImageView`, and +`SpatialCanvas` components. diff --git a/.claude/launch.json b/.claude/launch.json index 8a2e2dbe..cf42fafc 100644 --- a/.claude/launch.json +++ b/.claude/launch.json @@ -5,6 +5,7 @@ "name": "vis-demo", "runtimeExecutable": "pnpm", "runtimeArgs": ["--filter", "@spatialdata/vis", "dev:demo"], + "autoPort": true, "port": 5173 }, { diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index d9b109da..0f6358a4 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -17,9 +17,9 @@ jobs: runs-on: ubuntu-latest permissions: contents: read - # Surfaces React Hooks / React Compiler (Rules-of-React) findings for the - # React-shipping packages via eslint-plugin-react-hooks. Informational for - # now: there is an existing backlog of findings, so this must not gate merges. + # Enforces React Hooks / React Compiler (Rules-of-React) compliance for the + # React-shipping packages via eslint-plugin-react-hooks. The backlog has been + # cleared, so this is a required gate: any new finding fails the check. steps: - uses: actions/checkout@v4 with: @@ -39,11 +39,6 @@ jobs: run: pnpm install --frozen-lockfile - name: Lint React packages (react-hooks / react-compiler rules) - # continue-on-error (at the step, not the job) keeps this check green - # while the backlog exists, so it does not read as a failing/required - # check; findings still show in this step's log. Remove this line once - # `pnpm lint:react` is clean to turn it into a required gate. - continue-on-error: true run: pnpm lint:react test: diff --git a/packages/layers/src/shapesLayer.ts b/packages/layers/src/shapesLayer.ts index 08cd5c48..dfb4791d 100644 --- a/packages/layers/src/shapesLayer.ts +++ b/packages/layers/src/shapesLayer.ts @@ -237,6 +237,12 @@ export interface CreateShapesDeckLayerOptions { spatialCoordinateSystem?: string | null; onShapeHover?: (event: ShapesLayerPickEvent) => void; onShapeClick?: (event: ShapesLayerPickEvent) => void; + /** + * When false, the layer is rendered non-pickable with autoHighlight disabled. + * Used to suppress deck.gl's per-pointer-move picking-buffer render over large + * shape geometry while the camera is being panned/zoomed. Defaults to true. + */ + pickingEnabled?: boolean; } function multiplyAlpha( @@ -603,8 +609,8 @@ function createPolygonDeckLayer( stroked: true, opacity: options.opacity ?? 1, modelMatrix: options.modelMatrix, - pickable: true, - autoHighlight: true, + pickable: options.pickingEnabled ?? true, + autoHighlight: options.pickingEnabled ?? true, highlightColor: [255, 255, 0, 128], onHover: createPickHandler( options.id, @@ -648,8 +654,8 @@ function createCircleDeckLayer( }, opacity: options.opacity ?? 1, modelMatrix: options.modelMatrix, - pickable: true, - autoHighlight: true, + pickable: options.pickingEnabled ?? true, + autoHighlight: options.pickingEnabled ?? true, highlightColor: [255, 255, 0, 128], onHover: createPickHandler( options.id, diff --git a/packages/react/src/hooks/useSpatialData.ts b/packages/react/src/hooks/useSpatialData.ts index 484350bf..aae94ad9 100644 --- a/packages/react/src/hooks/useSpatialData.ts +++ b/packages/react/src/hooks/useSpatialData.ts @@ -1,36 +1,54 @@ -import { useEffect, useState } from 'react'; import type { SpatialData } from '@spatialdata/core'; +import { useEffect, useState } from 'react'; import { useSpatialDataContext } from '../provider/SpatialDataProvider'; +type ResolvedSpatialData = { + /** The promise this result was produced from, used to detect stale results. */ + promise: Promise | null; + spatialData: SpatialData | null; + error: Error | null; +}; + export function useSpatialData() { const { spatialDataPromise } = useSpatialDataContext(); - const [spatialData, setSpatialData] = useState(null); - const [error, setError] = useState(null); - const [loading, setLoading] = useState(true); + // Track which promise each settled result came from so loading/reset can be + // derived during render rather than synchronised with a setState-in-effect. + const [resolved, setResolved] = useState({ + promise: null, + spatialData: null, + error: null, + }); useEffect(() => { + if (!spatialDataPromise) return; let cancelled = false; - setLoading(true); - setError(null); - setSpatialData(null); - if (!spatialDataPromise) { - setLoading(false); - return; - } spatialDataPromise .then((s) => { - if (!cancelled) setSpatialData(s); + if (!cancelled) setResolved({ promise: spatialDataPromise, spatialData: s, error: null }); }) .catch((e: unknown) => { - if (!cancelled) setError(e instanceof Error ? e : new Error(String(e))); - }) - .finally(() => { - if (!cancelled) setLoading(false); + if (!cancelled) { + setResolved({ + promise: spatialDataPromise, + spatialData: null, + error: e instanceof Error ? e : new Error(String(e)), + }); + } }); return () => { cancelled = true; }; }, [spatialDataPromise]); - return { spatialData, loading, error } as const; + // When the current promise hasn't settled into `resolved` yet, we're loading + // (or idle, if there is no promise). Deriving this avoids resetting state in + // an effect every time `spatialDataPromise` changes. + const settled = resolved.promise === spatialDataPromise; + const loading = Boolean(spatialDataPromise) && !settled; + + return { + spatialData: settled ? resolved.spatialData : null, + loading, + error: settled ? resolved.error : null, + } as const; } diff --git a/packages/vis/package.json b/packages/vis/package.json index 4ae88ed5..49493ed7 100644 --- a/packages/vis/package.json +++ b/packages/vis/package.json @@ -21,7 +21,7 @@ "build": "vite build && tsc --noEmit", "dev": "node scripts/dev.mjs", "dev:stop": "node ../../scripts/dev-stop.mjs", - "dev:demo": "vite --config vite.config.demo.ts --host 127.0.0.1 --port 5173 --strictPort", + "dev:demo": "vite --config vite.config.demo.ts", "watch": "vite build --watch", "test": "vitest run", "test:watch": "vitest", diff --git a/packages/vis/scripts/dev.mjs b/packages/vis/scripts/dev.mjs index 45a3090e..871a5a43 100644 --- a/packages/vis/scripts/dev.mjs +++ b/packages/vis/scripts/dev.mjs @@ -33,18 +33,18 @@ async function fixtureServerIsOurs(port) { } /** - * Check the ports we need before spawning anything, so a leftover dev server - * (often from another checkout) produces a clear message instead of a cryptic - * strict-port crash. Returns whether we still need to start the fixture server. + * Check the ports we need before spawning anything: a busy demo port just gets a + * heads-up (Vite falls back to a free one), while a foreign process on the fixed + * fixture port is fatal. Returns whether we still need to start the fixture server. */ async function preflight() { if (await isPortInUse(DEMO_PORT)) { const holder = describePortHolder(DEMO_PORT); - console.error( - `\n[dev] Demo port ${DEMO_PORT} is already in use${holder ? `:\n ${holder}` : '.'}` + console.log( + `[dev] Demo port ${DEMO_PORT} is already in use${ + holder ? ` (${holder})` : '' + }; Vite will fall back to the next free port.` ); - console.error('[dev] Run `pnpm dev:stop` to clear SpatialData dev processes, then retry.\n'); - process.exit(1); } if (await isPortInUse(FIXTURE_SERVER_PORT)) { @@ -126,13 +126,6 @@ if (startFixtures) { } console.log(`Starting ${startedParts.join(', ')}...`); start('watch', ['vite', 'build', '--watch']); -start('demo', [ - 'vite', - '--config', - 'vite.config.demo.ts', - '--host', - '127.0.0.1', - '--port', - String(DEMO_PORT), - '--strictPort', -]); +// Host/port (and the free-port fallback) are owned by vite.config.demo.ts so +// the port selection stays in one place; honour PORT if the caller pins it. +start('demo', ['vite', '--config', 'vite.config.demo.ts']); diff --git a/packages/vis/src/ImageView/index.tsx b/packages/vis/src/ImageView/index.tsx index 4010e16d..ddbfc202 100644 --- a/packages/vis/src/ImageView/index.tsx +++ b/packages/vis/src/ImageView/index.tsx @@ -148,31 +148,24 @@ export default function ImageView() { const [selectedImage, setSelectedImage] = useState(''); const [ref, { width, height }] = useMeasure(); - useEffect(() => { - if (!spatialData?.images) return; - if (selectedImage === '' || !spatialData.images[selectedImage]) { - setSelectedImage(Object.keys(spatialData.images)[0]); - } - }, [spatialData?.images, selectedImage]); + const imageKeys = useMemo(() => Object.keys(spatialData?.images ?? {}), [spatialData?.images]); + // Default to the first available image, derived during render. + const effectiveImage = + selectedImage && imageKeys.includes(selectedImage) ? selectedImage : (imageKeys[0] ?? ''); const vivStores = useMemo(() => { return createVivStores(); }, []); const image = useMemo(() => { - return spatialData?.images?.[selectedImage]; - }, [selectedImage, spatialData?.images]); - const [imageUrl, setImageUrl] = useState(); - useEffect(() => { - if (image) { - setImageUrl(image.url ?? ''); - } else { - setImageUrl(''); - } - }, [image]); + return spatialData?.images?.[effectiveImage]; + }, [effectiveImage, spatialData?.images]); + // The url is synchronously available on the image, so derive it rather than + // syncing through state in an effect. + const imageUrl = useMemo(() => image?.url ?? '', [image]); return (
{spatialData?.images && ( - setSelectedImage(e.target.value)}> {Object.keys(spatialData.images).map((key) => (