docs: SpatialData tables integration roadmap#522
Open
xinaesthete wants to merge 2 commits into
Open
Conversation
Research/scoping design docs (exploratory, not decisions) for making SpatialData `tables` first-class in MDV. Grounded with file:line anchors from a codebase deep-dive across MDV, SpatialData.ts, anndata.js, and tgpu-htj2k. Centered on the core deliverable (doc 00): spatial elements (shapes/labels) annotated by a table should be coloured/filtered/highlighted by table columns, reusing the points path's shared DataStore row-index space. Covers the enabling/adjacent themes: a JS zarr-read DataLoader with h5 write routing, flexible convert-spatial table grouping, project-scoped shared spatial contexts, views/DataSources/ChartManager rework, and async/GPU/WASM filtering. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Deploy Preview for mdv-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (4)
📝 WalkthroughWalkthroughAdds seven new design documents under ChangesSpatial Tables Design Documentation
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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
🧹 Nitpick comments (1)
docs/design/spatial-tables/02-convert-spatial-tables.md (1)
103-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPick one actual default grouping policy.
The recommendation currently says default to merge-by-region or one-per-table, which are materially different behaviors. Please choose one as the default and relegate the other to an explicit fallback/compatibility mode so downstream docs and implementation don’t diverge.
🤖 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 `@docs/design/spatial-tables/02-convert-spatial-tables.md` around lines 103 - 105, The default grouping policy is ambiguous in the spatial tables design doc, since “merge-by-region” and “one-per-table” are both presented as defaults. Update the recommendation to choose one actual default in the relevant section of the document, and move the other option into an explicit compatibility or fallback mode so the intended behavior is unambiguous for downstream implementation and docs.
🤖 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 `@docs/design/spatial-tables/03-shared-spatial-contexts.md`:
- Around line 24-29: The fenced example in the shared spatial contexts doc is
missing a language tag, which triggers markdownlint MD040. Update the code fence
around the QueryClientProvider/ChartManagerProvider snippet to include an
appropriate language identifier so the example renders consistently and passes
the docs lint checks.
- Around line 65-74: The shared spatial store cache proposal can permanently
poison future loads if it stores raw rejected `readZarr` promises. Update the
cache logic in `spatial_store_cache.ts` and the wrapper around
`SpatialDataProvider`/`createMdvPortal` so failed loads are evicted on
rejection, or split the inflight promise tracking from the fulfilled
`SpatialData` entry. Keep the deduplication behavior for successful loads keyed
by resolved store URL (and `selection` if applicable), but ensure transient
errors can be retried instead of reusing a rejected promise.
In `@docs/design/spatial-tables/04-views-datasources-chartmanager.md`:
- Around line 89-120: The view schema change in the ChartManager/ViewManager
flow introduces a new discriminator and moves layout state from per-datasource
to region-level, so add a backward-compatible migration/defaulting path for
existing saved views. Update the relevant branches in `_init(view)`,
`getState()`, and `ViewManager.hasUnsavedChanges` to recognize both legacy and
new shapes, normalize older `views.json` entries on load, and ensure the
save/compare logic does not mark unchanged migrated views as dirty forever.
In `@docs/design/spatial-tables/05-dimension-async-filtering.md`:
- Around line 74-76: The sibling repo reference in the design doc uses a
machine-local absolute path, which is not portable. Replace the
`/Users/ptodd/code/codecs/tgpu-htj2k` wording in the affected markdown with a
repo-relative reference or a neutral placeholder, keeping the rest of the
guidance about WebGPU/TypeGPU/TGSL and import-by-path intact.
- Around line 8-13: Clarify the description of the synchronous filtering path in
the spatial-tables design doc: the current wording overstates that all
filter-derived work is already off-thread, but chart-scope predicate/ownership
handling still runs on the main thread. Update the affected section to
distinguish the remaining synchronous layer from the off-thread worker/SAB-based
derivations, and adjust the phrasing around the filter-evaluation hotspot so it
accurately reflects what is still main-thread work. Refer to the “Filter
evaluation” section and the “everything derived from a filter” sentence when
revising the wording.
- Around line 45-70: Clarify the documentation so `useFilteredIndices()` is not
described as the shared worker-computed compact index list when that role is
actually implemented by `useSimplerFilteredIndices`. Update this section to
separate the two responsibilities: `useSimplerFilteredIndices` for the cached
async worker result, and `useFilteredIndices` for the higher-level hook that
also applies the synchronous chart-scope predicate. Keep the wording aligned
with the symbols `useFilteredIndices`, `useSimplerFilteredIndices`, and
`useChartScopeFilterPredicate` so the ownership of each filtering path is
unambiguous.
In `@docs/design/spatial-tables/README.md`:
- Around line 84-108: The ASCII diagram fence in the spatial-tables README is
missing a language tag, which triggers markdownlint in this repo. Update the
fenced block around the diagram to use the text language label so the docs stay
lint-clean; the change is isolated to the README diagram block.
---
Nitpick comments:
In `@docs/design/spatial-tables/02-convert-spatial-tables.md`:
- Around line 103-105: The default grouping policy is ambiguous in the spatial
tables design doc, since “merge-by-region” and “one-per-table” are both
presented as defaults. Update the recommendation to choose one actual default in
the relevant section of the document, and move the other option into an explicit
compatibility or fallback mode so the intended behavior is unambiguous for
downstream implementation and docs.
🪄 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: d13147ec-4291-487e-ab80-15b1fa80baa2
📒 Files selected for processing (7)
docs/design/spatial-tables/00-table-element-association.mddocs/design/spatial-tables/01-js-dataloader.mddocs/design/spatial-tables/02-convert-spatial-tables.mddocs/design/spatial-tables/03-shared-spatial-contexts.mddocs/design/spatial-tables/04-views-datasources-chartmanager.mddocs/design/spatial-tables/05-dimension-async-filtering.mddocs/design/spatial-tables/README.md
Comment on lines
+65
to
+74
| - New module `src/react/spatialdata/spatial_store_cache.ts`: a `Map<string, Promise<SpatialData>>` | ||
| keyed by resolved store URL (+ `selection` if ever used). One instance per project — a module | ||
| singleton mirroring `queryClient`, or hung off `chartManager` / `ProjectContext`. | ||
| - Add a thin `SpatialStoreProvider` inside `createMdvPortal` | ||
| ([react_utils.tsx:108](../../../src/react/react_utils.tsx)), alongside `ProjectProvider`, | ||
| exposing the cache. | ||
| - Replace the two direct `<SpatialDataProvider source>` usages with a wrapper that resolves the | ||
| store from the shared cache (or wrap upstream `SpatialDataProvider` so its `readZarr` is deduped | ||
| by URL). Result: N charts + dialogs on one path → **1 `readZarr`, 1 `SpatialData` instance, | ||
| shared `parquetTableCache` etc.** (layers (a) and (b) shared) with minimal surface area. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't cache rejected readZarr promises indefinitely.
A failed load would poison this cache if you store the raw promise here; subsequent reads for the same URL would keep failing until eviction or process restart. Please evict on rejection, or separate the inflight promise from the fulfilled SpatialData entry, so transient store errors remain recoverable.
🤖 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 `@docs/design/spatial-tables/03-shared-spatial-contexts.md` around lines 65 -
74, The shared spatial store cache proposal can permanently poison future loads
if it stores raw rejected `readZarr` promises. Update the cache logic in
`spatial_store_cache.ts` and the wrapper around
`SpatialDataProvider`/`createMdvPortal` so failed loads are evicted on
rejection, or split the inflight promise tracking from the fulfilled
`SpatialData` entry. Keep the deduplication behavior for successful loads keyed
by resolved store URL (and `selection` if applicable), but ensure transient
errors can be retried instead of reusing a rejected promise.
- tgpu-htj2k: replace machine-local absolute path with the public repo URL (https://github.com/xinaesthete/tgpu-htj2k, may be renamed) - 05: don't overstate async coverage — chart-scope predicate and cross-chart ownership still run on the main thread; clarify that the shared worker-computed compact list is useSimplerFilteredIndices and useFilteredIndices layers the sync chart-scope predicate on top - 04: note the region-scoped layout is an additive/opt-in View schema change (kind discriminator), not a migration; getState()/hasUnsavedChanges must dispatch on the discriminator (per @xinaesthete) - 03: caution against caching rejected readZarr promises indefinitely; evict on rejection or separate inflight from fulfilled - label fenced code blocks (MD040): tsx / text Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
Addressed the review in d156c85 (docs-only):
Still exploratory research, not decisions. |
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.
What this is
An exploratory design-doc set (research/scoping, not decisions — decisions still belong in
docs/adr/) for making SpatialDatatablesfirst-class in MDV. Added underdocs/design/spatial-tables/. No code changes.Every claim is anchored to
file:line, from a codebase deep-dive across MDV plus the sibling librariesSpatialData.ts,anndata.js, andcodecs/tgpu-htj2k.The core deliverable (doc 00)
A spatial element (shapes / labels) annotated by a table should get a first-class association to that table's rows — so geometry is coloured, filtered, highlighted, and tooltipped by table columns, and filtering flows both ways with every other chart on that table.
Key findings:
DataStorerow-index space:filterArray+getColorFunction+dataHighlighted). Shapes/labels do not — the association is a bare stub (table_association.tstype with no resolver),fillColorByColumnUI is commented out, shapes render static and ignore the filter.SpatialData.tsalready ships the join layer (tableAssociations.ts:loadFeatureRowIndexByFeatureIndex → Int32Array, per-featurefeatureStatecolouring, pick events carryingrowIndex) — but it joins oninstance_keyagainst the untouched zarr store.convert-spatialpath destroys that identity:instance_keyis discarded (_instance_keyatconversion.py:373/:639) and the singleanndata.concatrow-stacks tables, scrambling order. So the core feature and the read-path / converter rework are one project, not separate ones.The enabling / adjacent themes
00-table-element-association.md01-js-dataloader.mdDataLoader(spatialdata.js/anndata.js) coexisting with h5 writes via abackinghint02-convert-spatial-tables.mdinstance_key/region_key03-shared-spatial-contexts.mdqueryClientprecedent)04-views-datasources-chartmanager.md05-dimension-async-filtering.mduseFilteredIndicesREADME.mdFor reviewers
instance_key/region_key. Doc 00 lays out both.@spatialdata/*@0.2.5packages (absent in the worktree): whether@spatialdata/visexposes per-layerfeatureStateinjection (Option A in doc 00).🤖 Generated with Claude Code
Summary by CodeRabbit