From f6ea49ff901ec8c0ca1e44d64956d1781517ab02 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Mon, 29 Jun 2026 23:41:13 +0100 Subject: [PATCH 1/9] SpatialCanvas picking perf + Rules-of-React cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tooltip/picking performance: - Default `aggregateHoverTooltips` to false. Aggregation ran extra `pickMultipleObjects` GPU passes per pointer move on top of deck's own hover/highlight pick — very costly over large pickable geometry. Single-pick hover reuses the existing pick; aggregation is now opt-in. - Disable shape picking + autoHighlight during pan/zoom via a debounced interaction gate (`useViewInteractionGate` -> `pickingEnabled` on the shapes layer), so deck doesn't re-render shape geometry into the picking buffer mid-gesture. - Throttle hover tooltip resolution to one run per animation frame, skip redundant same-pixel picks, suppress picking while a pointer button is held, and batch the per-missing-layer supplemental pick into a single pass. Rules-of-React (eslint-plugin-react-hooks): clear the 19-finding backlog and make the `react-lint` CI job required. Replace ref-during-render and setState-in-effect patterns with derived state across `@spatialdata/react` useSpatialData and the vis Transforms/Table/Shapes/ImageView/SpatialCanvas components; document the one intentional external-store ref read in useLayerData's isBlocking memo. Adds tests for the batched supplemental pick and the drag-suppression helper. Co-Authored-By: Claude Opus 4.8 --- ...lcanvas-picking-perf-and-rules-of-react.md | 29 ++++ .github/workflows/test.yml | 11 +- packages/layers/src/shapesLayer.ts | 14 +- packages/react/src/hooks/useSpatialData.ts | 52 ++++-- packages/vis/src/ImageView/index.tsx | 27 ++- packages/vis/src/Shapes/index.tsx | 57 +++--- .../src/SpatialCanvas/SpatialCanvasViewer.tsx | 164 +++++++++++------- .../src/SpatialCanvas/featureTooltipHover.ts | 21 ++- packages/vis/src/SpatialCanvas/index.tsx | 107 ++++++++---- .../SpatialCanvas/renderers/shapesRenderer.ts | 8 + .../vis/src/SpatialCanvas/useLayerData.ts | 47 ++++- .../SpatialCanvas/useThrottledHoverTooltip.ts | 91 ++++++++++ .../SpatialCanvas/useViewInteractionGate.ts | 61 +++++++ packages/vis/src/Table/index.tsx | 20 ++- packages/vis/src/Transforms/index.tsx | 48 +++-- .../vis/tests/featureTooltipHover.spec.ts | 44 ++++- .../vis/tests/throttledHoverTooltip.spec.ts | 18 ++ 17 files changed, 597 insertions(+), 222 deletions(-) create mode 100644 .changeset/spatialcanvas-picking-perf-and-rules-of-react.md create mode 100644 packages/vis/src/SpatialCanvas/useThrottledHoverTooltip.ts create mode 100644 packages/vis/src/SpatialCanvas/useViewInteractionGate.ts create mode 100644 packages/vis/tests/throttledHoverTooltip.spec.ts 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..3bf22b90 --- /dev/null +++ b/.changeset/spatialcanvas-picking-perf-and-rules-of-react.md @@ -0,0 +1,29 @@ +--- +"@spatialdata/vis": minor +"@spatialdata/layers": patch +"@spatialdata/react": patch +--- + +SpatialCanvas hover/picking performance and Rules-of-React cleanup. + +Picking/tooltip performance: + +- `aggregateHoverTooltips` now defaults to `false`. Aggregation issued extra + `pickMultipleObjects` GPU passes on every pointer move (on top of the pick + deck.gl already does for hover/highlight), which is very expensive over large + pickable geometry. Single-pick hover uses the existing pick; enable + aggregation explicitly when stacked-layer tooltips are needed. +- 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 throttled to one run per animation frame, skips + redundant same-pixel work, is suppressed while a pointer button is held + (drag), and collapses the per-missing-layer supplemental pick storm 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/.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/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) => (