Spatialdata chart with associated layers dialog - #514
Merged
Conversation
…k management. Lots of boilerplate here that I hope we can reduce. - Introduced `renderStackGeneration` to track changes in render stack entries. - Added `bumpRenderStackGeneration` method to increment the generation counter. - Refactored `SpatialCanvasFromRenderStack` to utilize the new render stack management. - Implemented caching and synchronization for render stack layer inputs to optimize performance. - Updated layer dialog components to use new hooks for render stack entry management. - Improved handling of host layer resolution and cloning for better rendering efficiency.
…untime functions with a new channel bridge. Update spatial data components to utilize the new structure, improving maintainability and performance. Doeesn't properly work with add/remove channels, showing histogram...
Introduce immediate updates for histogram and tone controls, ensuring state ownership is clear. Replace deprecated channel bridge with a new context-based approach, improving performance and maintainability. Update documentation to reflect changes in spatial image channel controls.
✅ Deploy Preview for mdv-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SpatialData MDV integration across chart registration, render-stack helpers, runtime bridges, layer panels, channel controls, tests, and docs. ChangesSpatialData.js MDV integration
Sequence Diagram(s)sequenceDiagram
participant SpatialDataChartRoot
participant SpatialDataMainChart
participant SpatialDataViewer
participant useRenderStackAdapter
participant SpatialCanvasFromRenderStack
SpatialDataChartRoot->>SpatialDataMainChart: render inside VivProvider
SpatialDataMainChart->>SpatialDataViewer: compose SpatialDataProvider and SpatialAnnotationProvider
SpatialDataViewer->>useRenderStackAdapter: stack, generation, hostLayerResolver
useRenderStackAdapter-->>SpatialDataViewer: layers, layerOrder, deckLayers
SpatialDataViewer->>SpatialCanvasFromRenderStack: render layer inputs
SpatialCanvasFromRenderStack->>SpatialDataViewer: onSpatialViewStateChange / onFeatureHover
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/react/components/ColorChannelComponents.tsx (1)
398-451: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDouble-click reset in spatial mode doesn't update the slider's displayed value.
The sliders read their value from the zustand store (
contrast[index]/brightness[index]). On drag (onChange) the spatial path updates both zustand andpatchToneAtIndex, but the double-click reset (onClick,e.detail === 2) only callspatchToneAtIndex(..., DEFAULT_BRIGHTNESS_CONTRAST)and returns without writing back to zustand. As a result the underlying tone resets but the slider thumb stays at its previous position. Mirror theonChangebehavior by also syncing the store on reset.🐛 Proposed fix
onClick={(e) => { if (e.detail === 2) { if (spatial) { + channelsStore.setState({ + contrast: withChannelValue(contrast, index, DEFAULT_BRIGHTNESS_CONTRAST), + }); spatial.patchToneAtIndex(index, "contrast", DEFAULT_BRIGHTNESS_CONTRAST); return; } channelsStore.setState({ contrast: withChannelValue(contrast, index, DEFAULT_BRIGHTNESS_CONTRAST), }); } }}Apply the equivalent change to the Brightness
onClickreset.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/components/ColorChannelComponents.tsx` around lines 398 - 451, The double-click reset in the `ColorChannelComponents` slider handlers is only updating the spatial tone via `patchToneAtIndex`, so the zustand-backed `contrast` and `brightness` values stay stale and the thumb does not move. Update the `onClick` reset branches for both `contrast` and `brightness` to mirror the `onChange` path by also calling `channelsStore.setState` with `withChannelValue(...)` before/alongside `spatial.patchToneAtIndex(...)`. Keep the reset behavior consistent for both spatial and non-spatial modes using the existing `DEFAULT_BRIGHTNESS_CONTRAST`, `withChannelValue`, and `channelsStore` symbols.
🧹 Nitpick comments (8)
src/react/components/SpatialDataMDVReact.tsx (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve the commented-out
removeSpatialDataRootViv(config).The inline note says it's only needed for dev-saved charts and "we shouldn't keep this". Since
getConfigalready callsremoveSpatialDataRootVivon save (Line 195), leaving this commented line invites confusion about whether load-time stripping is still required. Remove it (or convert to a one-off migration with a clear comment) before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/components/SpatialDataMDVReact.tsx` at line 42, The commented-out removeSpatialDataRootViv(config) in SpatialDataMDVReact is stale and confusing because getConfig already handles stripping on save. Remove this load-time call from the component, or replace it with a clearly documented one-time migration only if it is still genuinely needed, and keep the logic centered around getConfig so there is no duplicate behavior.src/react/spatialdata/spatial_feature_tooltip.ts (1)
27-33: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHand-rolled HTML escaping flagged by static analysis (CWE-79).
The current
escapeHtmlescapes& < > "in the correct order and is applied to every dynamic label/value, so the present markup (staticstyle, dynamic text only in text-node context) is adequately neutralized. However, manual escaping is easy to outgrow—if any future field is interpolated into an attribute or unescaped context this will silently become an XSS sink. Prefer a vetted encoder/sanitizer (e.g. DOMPurify) for the assembled HTML, especially since the output is injected viainnerHTMLdownstream.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/spatialdata/spatial_feature_tooltip.ts` around lines 27 - 33, The `escapeHtml` helper in `spatial_feature_tooltip` is a hand-rolled XSS guard that may not stay safe as the tooltip HTML evolves. Replace the manual escaping approach with a vetted HTML sanitizer/encoder for the assembled markup used by the tooltip rendering path, and ensure the `innerHTML`-bound output is sanitized centrally rather than relying on `escapeHtml` for each dynamic field.Source: Linters/SAST tools
src/react/spatialdata/image_layer_runtime.ts (1)
113-115: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
statsCacheRefgrows unbounded for the panel lifetime.Entries are keyed by
selectionStatsKey(channelId + z/c/t) and are never evicted, so repeatedly adding/removing channels or scrubbing z/t accumulates stats (includingFloat32Arrayrasters) for the lifetime of the panel. Consider an LRU/size cap if selection churn is expected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/spatialdata/image_layer_runtime.ts` around lines 113 - 115, The stats cache in image_layer_runtime.ts is never evicted, so ChannelStats entries keyed by selectionStatsKey can accumulate for the entire panel lifetime. Update the cache management around statsCacheRef and the code that reads/writes selectionStatsKey so it enforces a bounded policy, such as an LRU or fixed-size cap, and evicts old channel/z/c/t entries when selection churn occurs.src/react/spatialdata/render_stack_adapter.ts (1)
81-84: 🚀 Performance & Scalability | 🔵 TrivialUse
renderStackOrderhere
syncRenderStackLayerInputsonly needs the order, so callingrenderStackToLayerInputs(stack)just to readlayerOrderdoes extra work on the hot render path. IfrenderStackOrder(stack)is available from@spatialdata/vis, switch to that helper and keeprenderStackToLayerInputsfor the full inputs case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/spatialdata/render_stack_adapter.ts` around lines 81 - 84, In syncRenderStackLayerInputs, avoid calling renderStackToLayerInputs(stack) when only the order is needed; use renderStackOrder(stack) from `@spatialdata/vis` to compute nextOrder on the hot path, and keep renderStackToLayerInputs only for cases that need the full layer inputs. Update the comparison against cache.layerOrder to use the order returned by renderStackOrder so the function stays focused on order syncing without extra work.src/react/components/SpatialLayerDialogReactWrapper.tsx (1)
22-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
ensureChunkWorker()out of the render body.Calling a side-effecting initializer directly in render breaks render purity and runs on every render (and twice under StrictMode in React 19). Since it's a one-time init it's harmless today, but prefer an effect to keep the component pure.
♻️ Proposed change
-import { useRegion } from "../hooks"; +import { useRegion } from "../hooks"; +import { useEffect } from "react"; @@ const SpatialLayerDialogReact = observer(function SpatialLayerDialogReact() { const rawRegion = useRegion(); const spatialDataUrl = getSpatialDataUrl(rawRegion); - ensureChunkWorker(); + useEffect(() => { + ensureChunkWorker(); + }, []);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/components/SpatialLayerDialogReactWrapper.tsx` around lines 22 - 31, Move the side-effecting ensureChunkWorker() call out of the SpatialLayerDialogReact render body so the component stays pure. Initialize the worker in an effect inside SpatialLayerDialogReact (or another one-time lifecycle hook) and keep the render path limited to deriving rawRegion, spatialDataUrl, and returning the SpatialDataProvider tree. Make sure the init still runs once on mount and does not execute on every re-render or twice under StrictMode.src/react/components/SpatialDataMDVReactComponent.tsx (1)
150-158: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRegistry effect lists deps it doesn't read.
renderer.isBlockingandrenderer.isLoadingare in the dependency array but unused in the effect body, so the image-layer registry is recreated on every load-state transition. If that re-registration is intentional (to refresh load state), prefer keying on the values actually consumed; otherwise drop these two deps to avoid redundant churn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/components/SpatialDataMDVReactComponent.tsx` around lines 150 - 158, The registry effect in SpatialDataMDVReactComponent is depending on renderer.isBlocking and renderer.isLoading even though the effect body does not read them, which causes unnecessary re-registration on load-state changes. Update the dependency list for that effect to include only the values actually used inside it, or if the intent is to refresh on load-state transitions, make that dependency explicit in the effect logic rather than relying on unused deps.src/react/components/spatialLayers/ImageLayerPanel.tsx (1)
67-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
channelConfigKeyrelies onJSON.stringifykey ordering for change detection.Using
JSON.stringifyas the dedupe key inonChannelsChangeis order-sensitive; semantically equal configs with different key insertion order will be treated as changed (extrapatchLayer), and there's no stable canonicalization. Acceptable for now, but a structural compare or stable key would be more robust.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/components/spatialLayers/ImageLayerPanel.tsx` around lines 67 - 69, `channelConfigKey` in ImageLayerPanel currently uses `JSON.stringify`, so `onChannelsChange` can treat semantically identical `LayerChannelConfig` values as different when key insertion order varies. Update the dedupe logic to use a stable canonical key or a structural equality check in `channelConfigKey`/`onChannelsChange`, so repeated channel updates only call `patchLayer` when the actual channel config changes.src/react/components/spatialLayers/ShapesLayerPanel.tsx (1)
21-24: 📐 Maintainability & Code Quality | 🔵 TrivialTracked WIP:
chartColorByfill-by-column noted as "not working".The doc comment flags this as broken with broader
colorBytype concerns. Leaving as-is for the initial merge is fine, but worth a tracking issue so it isn't lost.Want me to open an issue to track the
fillColorByColumn/chartColorBywiring?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/components/spatialLayers/ShapesLayerPanel.tsx` around lines 21 - 24, The `chartColorBy` / `fillColorByColumn` wiring is still marked broken and should not be forgotten before merge. Leave the current `ShapesLayerPanel` behavior unchanged for now, but add a tracking task/issue (or equivalent TODO note in the related `chartColorBy` / `fillColorByColumn` path) so the type mismatch and background-color wiring work is explicitly followed up.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONTEXT.md`:
- Line 56: Update the “Layer Channel Config” glossary entry in CONTEXT.md so the
example no longer lists tone brightness/contrast as part of
renderStack.entries[].props.channels; instead, make it clear that tone-related
fields belong in entry.props.vivLayerProps and keep channels limited to
ChannelConfig data like colors, contrast limits, visibility, selections, and
extension fields that actually persist there.
In `@docs/spatialdata-vis-integration.md`:
- Around line 196-203: The commit stage table has a column-count mismatch: the
header in the “Commit stage / Status” section defines two columns, but the “5+”
row includes an extra cell, causing the “Done” status to be dropped. Update the
table so the row structure matches the header by removing the extra cell or
adding the missing header column, and verify the “5+” entry renders its status
correctly in the doc.
In `@package.json`:
- Around line 98-102: The dependency entry for `@spatialdata/vis` is using a caret
range while patchedDependencies is tied to the exact 0.2.3 package, so the patch
can stop applying if the version floats. Update the package.json dependency for
`@spatialdata/vis` to a fixed 0.2.3 version, or adjust the patchedDependencies key
to match the intended range, and keep the change aligned with the surrounding
`@spatialdata/`* entries.
In `@src/react/components/spatialLayers/PointsLayerPanel.tsx`:
- Around line 42-46: Guard the RGBA input handling in PointsLayerPanel’s
onChange callback so `Number(event.target.value)` cannot write invalid values
into `config.color`. Update the local `next` array before calling `updateLayer({
color: next })` by coercing non-finite input to 0 and clamping each channel to
the 0–255 range, keeping the existing `color`/`index` flow intact.
In `@src/react/components/spatialLayers/ShapesLayerPanel.tsx`:
- Around line 47-51: The ColorFields handler in ShapesLayerPanel has the same
invalid RGBA input bug as PointsLayerPanel: Number(event.target.value) can
produce NaN for empty input and it does not clamp values to the valid 0–255
range. Update the onChange logic in ColorFields so it sanitizes the parsed value
with a finite check and clamps it before calling onChange, keeping fill/stroke
colors valid through the existing ShapesLayerPanel and ColorFields flow.
In `@src/react/spatialdata/spatialdata_config.ts`:
- Around line 6-8: The helper removeSpatialDataRootViv() is mutating the passed
config object by deleting viv in place, which can strip the live chart config
unexpectedly. Update removeSpatialDataRootViv<T>() to return a shallow copy of
config without the viv property instead of modifying the original object, so
callers using it for persistence don’t affect runtime state.
In `@src/react/spatialdata/table_association.ts`:
- Around line 1-4: Update the TableAssociation type so the resolved variant
cannot exist without a table name; in TableAssociation, change the "resolved"
branch to require tableName instead of making it optional. If there is a need to
represent a resolved-but-unnamed state, add a separate status rather than
reusing "resolved", and then adjust any consumers of TableAssociation to match
the new discriminated union shape.
---
Outside diff comments:
In `@src/react/components/ColorChannelComponents.tsx`:
- Around line 398-451: The double-click reset in the `ColorChannelComponents`
slider handlers is only updating the spatial tone via `patchToneAtIndex`, so the
zustand-backed `contrast` and `brightness` values stay stale and the thumb does
not move. Update the `onClick` reset branches for both `contrast` and
`brightness` to mirror the `onChange` path by also calling
`channelsStore.setState` with `withChannelValue(...)` before/alongside
`spatial.patchToneAtIndex(...)`. Keep the reset behavior consistent for both
spatial and non-spatial modes using the existing `DEFAULT_BRIGHTNESS_CONTRAST`,
`withChannelValue`, and `channelsStore` symbols.
---
Nitpick comments:
In `@src/react/components/SpatialDataMDVReact.tsx`:
- Line 42: The commented-out removeSpatialDataRootViv(config) in
SpatialDataMDVReact is stale and confusing because getConfig already handles
stripping on save. Remove this load-time call from the component, or replace it
with a clearly documented one-time migration only if it is still genuinely
needed, and keep the logic centered around getConfig so there is no duplicate
behavior.
In `@src/react/components/SpatialDataMDVReactComponent.tsx`:
- Around line 150-158: The registry effect in SpatialDataMDVReactComponent is
depending on renderer.isBlocking and renderer.isLoading even though the effect
body does not read them, which causes unnecessary re-registration on load-state
changes. Update the dependency list for that effect to include only the values
actually used inside it, or if the intent is to refresh on load-state
transitions, make that dependency explicit in the effect logic rather than
relying on unused deps.
In `@src/react/components/SpatialLayerDialogReactWrapper.tsx`:
- Around line 22-31: Move the side-effecting ensureChunkWorker() call out of the
SpatialLayerDialogReact render body so the component stays pure. Initialize the
worker in an effect inside SpatialLayerDialogReact (or another one-time
lifecycle hook) and keep the render path limited to deriving rawRegion,
spatialDataUrl, and returning the SpatialDataProvider tree. Make sure the init
still runs once on mount and does not execute on every re-render or twice under
StrictMode.
In `@src/react/components/spatialLayers/ImageLayerPanel.tsx`:
- Around line 67-69: `channelConfigKey` in ImageLayerPanel currently uses
`JSON.stringify`, so `onChannelsChange` can treat semantically identical
`LayerChannelConfig` values as different when key insertion order varies. Update
the dedupe logic to use a stable canonical key or a structural equality check in
`channelConfigKey`/`onChannelsChange`, so repeated channel updates only call
`patchLayer` when the actual channel config changes.
In `@src/react/components/spatialLayers/ShapesLayerPanel.tsx`:
- Around line 21-24: The `chartColorBy` / `fillColorByColumn` wiring is still
marked broken and should not be forgotten before merge. Leave the current
`ShapesLayerPanel` behavior unchanged for now, but add a tracking task/issue (or
equivalent TODO note in the related `chartColorBy` / `fillColorByColumn` path)
so the type mismatch and background-color wiring work is explicitly followed up.
In `@src/react/spatialdata/image_layer_runtime.ts`:
- Around line 113-115: The stats cache in image_layer_runtime.ts is never
evicted, so ChannelStats entries keyed by selectionStatsKey can accumulate for
the entire panel lifetime. Update the cache management around statsCacheRef and
the code that reads/writes selectionStatsKey so it enforces a bounded policy,
such as an LRU or fixed-size cap, and evicts old channel/z/c/t entries when
selection churn occurs.
In `@src/react/spatialdata/render_stack_adapter.ts`:
- Around line 81-84: In syncRenderStackLayerInputs, avoid calling
renderStackToLayerInputs(stack) when only the order is needed; use
renderStackOrder(stack) from `@spatialdata/vis` to compute nextOrder on the hot
path, and keep renderStackToLayerInputs only for cases that need the full layer
inputs. Update the comparison against cache.layerOrder to use the order returned
by renderStackOrder so the function stays focused on order syncing without extra
work.
In `@src/react/spatialdata/spatial_feature_tooltip.ts`:
- Around line 27-33: The `escapeHtml` helper in `spatial_feature_tooltip` is a
hand-rolled XSS guard that may not stay safe as the tooltip HTML evolves.
Replace the manual escaping approach with a vetted HTML sanitizer/encoder for
the assembled markup used by the tooltip rendering path, and ensure the
`innerHTML`-bound output is sanitized centrally rather than relying on
`escapeHtml` for each dynamic field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: add416e1-efa8-4164-adfc-5bebe5856714
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (35)
AGENTS.mdCONTEXT.mddocs/spatialdata-vis-integration.mdpackage.jsonpatches/@spatialdata__vis@0.2.3.patchsrc/charts/registerChartModules.tssrc/react/components/ColorChannelComponents.tsxsrc/react/components/SpatialDataMDVReact.tsxsrc/react/components/SpatialDataMDVReactComponent.tsxsrc/react/components/SpatialLayerDialogComponent.tsxsrc/react/components/SpatialLayerDialogReactWrapper.tsxsrc/react/components/histogram/useBrushX.tssrc/react/components/spatialLayers/DeckOverlayLayerPanel.tsxsrc/react/components/spatialLayers/ImageLayerPanel.tsxsrc/react/components/spatialLayers/LabelsLayerPanel.tsxsrc/react/components/spatialLayers/PointsLayerPanel.tsxsrc/react/components/spatialLayers/ShapesLayerPanel.tsxsrc/react/spatialdata/ensureChunkWorker.tssrc/react/spatialdata/host_overlay_ids.tssrc/react/spatialdata/image_layer_registry.tssrc/react/spatialdata/image_layer_runtime.tssrc/react/spatialdata/render_stack_adapter.tssrc/react/spatialdata/render_stack_control.tssrc/react/spatialdata/render_stack_defaults.tssrc/react/spatialdata/render_stack_display.tssrc/react/spatialdata/render_stack_observe.tssrc/react/spatialdata/smoke_import.tssrc/react/spatialdata/spatial_feature_tooltip.tssrc/react/spatialdata/spatialdata_config.tssrc/react/spatialdata/table_association.tssrc/react/spatialdata/view_state_bridge.tssrc/tests/react/spatialdata/renderStackHelpers.test.tssrc/tests/react/spatialdata/spatialDataConfig.test.tstsconfig.jsonvite.config.mts
Demote the per-panel avivatorish channelsStore to stats + flags only; the spatial channel UI now reads colors/contrast/visibility/selections from the canonical render-stack config and tone from vivLayerProps, via the panel context. Removes the two-store tone sync so the double-click reset moves the slider thumb (it previously wrote only vivLayerProps, leaving the store stale). - image_layer_runtime: stop mirroring config/tone into channelsStore; keep domains/raster/loader + viewer flags. Drop the vivLayerProps param. - ImageLayerPanel: expose brightness/contrast from vivLayerProps; patchToneAtIndex reads base tone from vivLayerProps not the store; channelConfigKey uses the order-stable serializeChannelConfig instead of JSON.stringify. - ColorChannelComponents: spatial branch reads tone/color/visibility/ids from the panel context; tone onChange writes vivLayerProps only. Legacy Avivator branch unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Gated behind localStorage.MDV_SPATIAL_PERF; zero-cost when off. Adds measureSpatial() around the render-stack adapter (observe/revision/sync) and React <Profiler> boundaries for the viewer vs canvas subtree, surfaced on window.__spatialPerf. Measured finding: on in-place image-prop edits the React/adapter cost is ~0.5ms/commit (viewer ~= canvas; revision JSON.stringify ~0.02ms). The frame cost is downstream in deck.gl/viv layer updates, with intermittent large GC pauses on channel-config (contrast) edits. Kept on-branch for follow-up perf work on the library side. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… stats
The stats fetch/cache/cancel loop was lifted into the library as
@spatialdata/avivatorish#useChannelSelectionStats. Replace MDV's hand-rolled
engine in useImageLayerRuntime with a call to the hook plus a one-effect
projection of its output (statsByIndex -> channelsStore.{domains,raster},
loadingByChannelId -> viewerStore.isChannelLoading, plus channelOptions/
pixelValues/loader). Deletes the local load loop, stats cache refs, toRasterSlice,
and the duplicated selectionStatsKey/pickDefaultSelectionForAdd helpers (now
imported from the package). ChannelHistogram and the legacy non-spatial Avivator
path are unchanged.
Verified: tsgo, spatialdata vitest, pnpm build; in-app on project 188
(multi-channel/multi-image histograms render real stats, add-channel fetches via
the hook, no console errors).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ization biome's useExhaustiveDependencies flagged the hand-written `tone` useMemo (it wants vivLayerProps in the deps, which would reintroduce the stale-thumb bug since vivLayerProps is patched in place). Drop the manual memo and let the React Compiler memoize: - toneFromArrays(brightnessRaw, contrastRaw, count): compute tone from the inner arrays (which are replaced on each in-place patch) rather than the stable vivLayerProps object, so the compiler keys the memo correctly — and the result can't go stale even if it bailed. - panelContext is now a plain object (compiler-memoized), removing the hand-maintained dependency list and its missing-`hookState` warning. - image_layer_runtime effect deps: use the stable hookState array refs directly instead of JSON string proxies, so the dependency list is honest for biome (this file is .ts, outside the compiler's .tsx scope). biome + react-compiler eslint + tsgo + spatialdata tests + build all clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ChannelSelectionStats Updated integrity hashes for the local file: overrides of @spatialdata/avivatorish and @spatialdata/vis after re-packing the library with the stateful channel-stats hook adopted in d6b6955. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The library was published to npm, so drop the local `.local-pack` file: tarball overrides (the workspace-link iteration workflow) and pin the five @spatialdata/* deps to ^0.2.4 (the version carrying the public-API exports + useChannelSelectionStats). All packages now resolve from the registry; zarrextra stays ^0.2.2. Verified: tsgo, spatialdata vitest, pnpm build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The library publishes to the `next` channel (`next` -> 0.2.4; `latest` is still 0.2.2). Point the five @spatialdata/* deps at the `next` tag so installs follow that channel. zarrextra stays ^0.2.2 — its `next` tag (0.2.0) is older than `latest`, so a `next` specifier there would downgrade it. Resolves to 0.2.4 (unchanged from the prior ^0.2.4 pin); tsgo clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/react/spatialdata/perf.ts (1)
60-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a typed global declaration to drop the repeated
window as unknown as {...}casts.A small
declare globalblock for__spatialPerf/__resetSpatialPerfwould let you assign directly without the double casts on Lines 60, 66, and 68. As per coding guidelines: "Avoidascasts where possible."♻️ Suggested typed global
declare global { interface Window { __spatialPerf?: unknown; __resetSpatialPerf?: () => void; } }Then
window.__spatialPerf = table;andwindow.__resetSpatialPerf = () => { ... }without casts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/spatialdata/perf.ts` around lines 60 - 69, Add a typed global Window augmentation for __spatialPerf and __resetSpatialPerf in perf.ts so the code can assign to these properties directly instead of using repeated window as unknown as casts. Update the existing logic around the ENABLED/window check and the stats.clear() reset path to use window.__spatialPerf and window.__resetSpatialPerf with the new declaration, keeping the current behavior unchanged while removing unnecessary assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/react/spatialdata/perf.ts`:
- Around line 60-69: Add a typed global Window augmentation for __spatialPerf
and __resetSpatialPerf in perf.ts so the code can assign to these properties
directly instead of using repeated window as unknown as casts. Update the
existing logic around the ENABLED/window check and the stats.clear() reset path
to use window.__spatialPerf and window.__resetSpatialPerf with the new
declaration, keeping the current behavior unchanged while removing unnecessary
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a6f4dc1d-cd85-4885-b3ca-cb0997b55658
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
docs/spatialdata-vis-integration.mdpackage.jsonsrc/react/components/ColorChannelComponents.tsxsrc/react/components/SpatialDataMDVReactComponent.tsxsrc/react/components/spatialLayers/ImageLayerPanel.tsxsrc/react/spatialdata/image_layer_registry.tssrc/react/spatialdata/image_layer_runtime.tssrc/react/spatialdata/perf.tssrc/react/spatialdata/render_stack_adapter.ts
✅ Files skipped from review due to trivial changes (1)
- docs/spatialdata-vis-integration.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/react/components/spatialLayers/ImageLayerPanel.tsx
- src/react/spatialdata/image_layer_registry.ts
- src/react/components/SpatialDataMDVReactComponent.tsx
- src/react/spatialdata/render_stack_adapter.ts
- src/react/components/ColorChannelComponents.tsx
leaving `ensureChunkWorker` mechanism now with a log in case we end up having similar again in future.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/react/spatialdata/ensureChunkWorker.ts`:
- Around line 11-16: The ensureChunkWorker() helper is incorrectly marking the
chunk worker as initialized without actually initializing it. Update
ensureChunkWorker in src/react/spatialdata/ensureChunkWorker.ts so
chunkWorkerEnabled is only set after a real enableWorkerChunkDecode(options)
call succeeds, or remove the chunkWorkerEnabled guard entirely if this helper is
meant to rely on external defaults; also keep the early return behavior aligned
with the actual initialization path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6ac2ef3-8465-4b8d-ba65-549c501da1b8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.jsonsrc/react/components/SpatialLayerDialogComponent.tsxsrc/react/spatialdata/ensureChunkWorker.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/react/components/SpatialLayerDialogComponent.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/react/spatialdata/render_stack_defaults.ts`:
- Around line 27-28: The render-stack ID generator in renderStack defaults
currently uses a short Math.random suffix, which is not safe for a uniqueness
contract. Update the logic in the ID creation path to generate IDs with
collision detection against the current stack state, or switch to a stronger
UUID-style suffix, so the returned value from the render-stack default helper
cannot collide for the same elementType/elementKey pair.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a291dfcc-c41f-44ce-a97e-4dd5ac801bd6
📒 Files selected for processing (3)
src/react/components/SpatialLayerDialogComponent.tsxsrc/react/spatialdata/render_stack_control.tssrc/react/spatialdata/render_stack_defaults.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/react/spatialdata/render_stack_control.ts
- src/react/components/SpatialLayerDialogComponent.tsx
…prop pane for selected element
- table_association: require `tableName` in the `resolved` branch (no resolved-without-name state). - spatialdata_config: remove `removeSpatialDataRootViv` entirely — it was dead code for any properly-created chart (scatterDefaults injects no `viv`, nothing reads/writes `config.viv`); only mid-dev saved charts ever had a root `viv`, which the chart ignores anyway. Refocus its test on the surviving `toSerializableSpatialDataViewState`. - render_stack_defaults: replace the 4-char `Math.random` entry-id suffix with the project's `getRandomString()` so duplicate entries of the same element (intended, for different roles/blend-modes) get collision-safe ids. - PointsLayerPanel: guard/clamp RGBA input to [0,255] (NaN -> 0). - spatial_feature_tooltip: sanitize assembled tooltip markup with DOMPurify; point the tooltip `dangerouslySetInnerHTML` biome-ignore at that sanitization. - ensureChunkWorker: call from an effect, not during render. - docs/CONTEXT: tone lives in `vivLayerProps`, not `channels`; fix the initial-PR-scope table column count. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The `layers` / `deckLayers` memos in the render-stack adapter carry trigger-only deps (`spatialRevision`, `hostFingerprint`, `generation`) that force a recompute but aren't read in the body. Reference them inside the callback (the project's bare-reference pattern) so useExhaustiveDependencies sees them used and the intent is documented — instead of suppressing the rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Lean on react-compiler in SpatialLayerDialogComponent: `availableFields` becomes a plain expression and the drag/insert/remove handlers plain arrow functions (removing the useCallback wrappers + dep arrays). `insertOptions` is left an explicit useMemo (complex multi-statement derivation reads clearer that way), and the panels' `options` memos are left alone — they back the disabled colour/tooltip UI. tsgo + biome + react-compiler eslint + build all clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Host overlays (scatter/gates/selection) are excluded from the layer-ordering UI and should always render on top. insertSpatialRenderStackEntry appended new spatial entries to the end of the stack — after the host entries — so a layer added from the dialog rendered on top of the overlays. Splice it before the first host entry instead (matching insertDefaultImageLayer), keeping the serialised order otherwise intact. Host insertion still appends (re-added overlays go on top). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Commit 0f30906 dropped useMemo/useCallback in the layer dialog on the assumption the React Compiler would cover them. It won't: the compiler does not optimize observer()-wrapped components — it doesn't recognize mobx-react-lite's `observer` HOC, so a `const X = observer(() => {…})` component is invisible to it (the component is an argument to a call, not a recognized definition). Every component in SpatialLayerDialogComponent is observer-wrapped, so none of them are memoized by the compiler, in dev or build. Verified empirically against the running dev server: a plain sibling panel (ShapesLayerPanel) emits compiler memo-cache output (~116 markers) while this all-observer file emits none. The compiler is NOT build-only — it runs in dev too (plain components like ViewGalleryCard are compiled there). Effect of the regression: touchRenderStackEntry subscribes to contrastLimits/brightness/contrast, so LayerDetails re-renders on every contrast-brush tick, and 0f30906 made each tick re-run getAvailableFields (Object.keys(columnIndex).sort()) and reallocate the drag/insert/remove handlers with nothing memoizing them — the state-management lag. Restore the explicit useMemo/useCallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…er-blind) ImageLayerPanelReady is observer()-wrapped, so the React Compiler does not optimize it (it doesn't recognize mobx-react-lite's observer HOC). The `panelContext` object was written as a plain literal "the compiler memoizes it", but it's the SpatialImagePanelContext Provider value — a fresh identity every render re-rendered every channel-row consumer of useSpatialImagePanelContext() on each render of the panel (e.g. every contrast-brush tick, and on unrelated observable changes like opacity). Wrap `panelContext` and the `tone` it feeds in explicit useMemo. Destructure the stable hookState store refs into locals so they are honest deps and the hooks linter is satisfied (member access on the freshly-allocated hookState object would make it demand the whole object as a dep). Also corrects the toneFromArrays doc comment, which made the same wrong build-time claim. Audit of the other observer-wrapped spatial components: SpatialDataMDVReactComponent is already thoroughly hand-memoized; SpatialLayerDialogReactWrapper is trivial. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ary one
When a SpatialData chart had no saved viewState it rendered unframed — you had to
pan/zoom to find the image. Confirmed in-app: the committed viewState was the
degenerate {target:[0,0], zoom:0}.
Cause: the library's built-in autoFit fires the moment `isBlocking` clears and, if
the visible layers' world bounds aren't populated at that instant, commits
{target:[0,0], zoom:0}. That is non-null, so `spatialViewState === null` is no
longer true and it never re-fits — the bad view sticks.
Fix: pass `autoFit: false` to the renderer and drive the fit from MDV. An effect
fits to `getWorldBoundsForVisibleLayers()` via `viewStateFromBounds`, but only once
`hasLayersDrawn` is true (bounds guaranteed available) and only commits a non-null
result. While bounds aren't ready it leaves viewState null, so it retries across the
re-renders that happen as image data loads instead of locking in a degenerate view.
Verified in-app: the fitted viewState is now the image centre/extent (e.g.
target ~[29004, 8549], zoom -7.8) instead of [0,0]/0.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
This adds a new SpatialData.js based viewer, which allows for layers of different spatial elements to be overlaid.
We hope that the foundation of state-management is reasonable, but there is certainly room for improvement.
The separate "Layers dialog" may be moved inside the more general settings dialog, but having it separate for now was a bit less risky. There are some settings that it would be useful to have mirrored there (control over our normal scatterplot etc) but this isn't currently the way it works, so you often need to switch between (which makes #518 very irritating).
A number of significant limitations in the current implementation in terms of coverage of spatialdata features:
In this version, we do not yet usefully associate tables, so things like colouring and tooltips on shapes (or labels, not to mention points) are not yet enabled. We intend to follow-up with changes to more properly make use of
zarrforDataLoaderso that there's a really proper association with datasources and the spatial elements.Getting this chart PR merged means that we can start a pass on that. Actually we could anyway, but it'll be more tangibly relevant.
Performance of
shapesis much less good than it should be, this should be addressed upstream in a future revision.pointsrendering is still rudimentary / fairly useless; we're working on this upstream and believe that there is potential for elegant/efficient approaches but this hasn't landed yet. Taylor-CCB-Group/SpatialData.js#51 needs cleaning up and I have several ideas for improvements.labelsdon't have any exposed UI for setting colours – the interface for this upstream is more complicated than it should be due to viv legacy, and I don't want to serialise things in the current form that will then require an adapter to a different schema. Taylor-CCB-Group/SpatialData.js#74We should be able to share SpatialData contexts between charts (and the data more generally) and make better use of cache.
Summary by CodeRabbit