Document the feature-table association split and core/layers responsibilities - #40
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR extracts feature→table row resolution into a core resolver (createFeatureTableAlignment), refactors shape color encoding to use pre-resolved indices and palettes, updates SpatialCanvas to consume shared helpers and re-export SpatialViewer, and expands docs, demos, and fixture/server tooling. ChangesFeature-Table Alignment Refactor
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (1)
docs/src/pages/index.tsx (1)
26-26: ⚡ Quick winSimplify by removing unnecessary template literal.
The template literal
${siteConfig.title}adds no value since you're only interpolating a single variable. Direct property access is cleaner.♻️ Simplify to direct property access
- title={`${siteConfig.title}`} + title={siteConfig.title}🤖 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/src/pages/index.tsx` at line 26, Replace the unnecessary template literal in the JSX prop so title uses direct property access: change title={`${siteConfig.title}`} to title={siteConfig.title} (locate the JSX where the title prop is set and update the expression).
🤖 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 `@packages/core/tests/tableAssociations.spec.ts`:
- Around line 69-72: Replace the unsafe type assertion at the SpatialData
constructor call by ensuring the fixture object rootStore is declared with a
structural check using the TypeScript "satisfies ConsolidatedStore" operator
instead of "as ConsolidatedStore"; locate the test fixture that constructs new
SpatialData('https://example.com/mock.zarr', rootStore as ConsolidatedStore,
[...]) and change the declaration of rootStore (not the call site) so it uses
"satisfies ConsolidatedStore" to validate its shape while keeping its inferred
type for downstream code.
In `@packages/layers/src/shapeColorEncoding.ts`:
- Around line 126-127: The code currently indexes into categoricalPalette
without checking for emptiness, causing NaN/undefined colors; update the logic
around the mapping that sets colors[featureId] to first validate
categoricalPalette and fall back to the default palette variable (or a
predefined default) when categoricalPalette.length === 0, then compute
paletteColor = palette[index % palette.length] and call rgba(paletteColor,
alpha) so colors[featureId] always receives a valid color; modify the block that
assigns paletteColor and uses rgba to reference the safe palette instead of
categoricalPalette.
---
Nitpick comments:
In `@docs/src/pages/index.tsx`:
- Line 26: Replace the unnecessary template literal in the JSX prop so title
uses direct property access: change title={`${siteConfig.title}`} to
title={siteConfig.title} (locate the JSX where the title prop is set and update
the expression).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37655351-0716-4fed-bf40-3b195b1881ed
📒 Files selected for processing (19)
docs/docs/core/overview.mdxdocs/docs/layers/overview.mdxdocs/docs/vis/feature-table-associations.mdxdocs/docs/vis/layer-prop-flow.mdxdocs/docs/vis/mdv-release-checklist.mdxdocs/docs/vis/spatial-canvas-status.mdxdocs/docusaurus.config.tsdocs/src/pages/index.tsxpackage.jsonpackages/core/src/tableAssociations.tspackages/core/tests/tableAssociations.spec.tspackages/layers/src/index.tspackages/layers/src/shapeColorEncoding.tspackages/layers/tests/shapeColorEncoding.spec.tspackages/vis/src/SpatialCanvas/public.tspackages/vis/src/SpatialCanvas/types.tspackages/vis/src/SpatialCanvas/useLayerData.tspackages/vis/src/index.tspackages/vis/tests/index.spec.tsx
Comment on lines
+69
to
+72
| return new SpatialData('https://example.com/mock.zarr', rootStore as ConsolidatedStore, [ | ||
| 'shapes', | ||
| 'tables', | ||
| ]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current cast usage and inspect SpatialData constructor declaration.
fd 'tableAssociations.spec.ts$' packages/core/tests --exec rg -n -C2 'rootStore as ConsolidatedStore|new SpatialData'
fd 'index.ts$' packages/core/src/store --exec rg -n -C3 'class SpatialData|constructor\s*\('Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 605
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the fixture definition and imports in the spec
spec_file="packages/core/tests/tableAssociations.spec.ts"
echo "---- $spec_file (context around rootStore) ----"
rg -n "const rootStore|ConsolidatedStore|new SpatialData" "$spec_file" -n
echo "---- $spec_file (lines 50-95) ----"
nl -ba "$spec_file" | sed -n '50,95p'
# Locate the ConsolidatedStore type and see its required shape
echo "---- ConsolidatedStore definition (search) ----"
fd 'ConsolidatedStore' packages/core/src --exec rg -n "export (type|interface) ConsolidatedStore|class ConsolidatedStore|type ConsolidatedStore|interface ConsolidatedStore"
# Also search within store directory for ConsolidatedStore usage / type exports
fd 'index.ts$' packages/core/src/store --exec rg -n "ConsolidatedStore" -S
# Try to open the file that actually declares ConsolidatedStore (best-effort)
# (Use rg to capture the file path from matches)
match_path="$(rg -n "export (type|interface) ConsolidatedStore|type ConsolidatedStore|interface ConsolidatedStore" packages/core/src/store -S --files-with-matches | head -n 1 || true)"
echo "---- candidate file: $match_path ----"
if [ -n "$match_path" ] && [ -f "$match_path" ]; then
nl -ba "$match_path" | sed -n '1,200p'
fiRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 476
🏁 Script executed:
#!/bin/bash
set -euo pipefail
spec_file="packages/core/tests/tableAssociations.spec.ts"
echo "---- $spec_file (context around rootStore and cast) ----"
rg -n "const rootStore|ConsolidatedStore|as ConsolidatedStore|new SpatialData" "$spec_file"
echo "---- $spec_file (lines 40-90) ----"
nl -ba "$spec_file" | sed -n '40,90p'
echo "---- Searching for ConsolidatedStore declaration ----"
# Find the actual declaration location(s)
rg -n "export (type|interface) ConsolidatedStore|type ConsolidatedStore|interface ConsolidatedStore" packages/core/src/store -S --files-with-matches | head -n 20
declare_file="$(rg -n "export (type|interface) ConsolidatedStore|type ConsolidatedStore|interface ConsolidatedStore" packages/core/src/store -S --files-with-matches | head -n 1 || true)"
echo "---- candidate ConsolidatedStore file: $declare_file ----"
if [ -n "$declare_file" ] && [ -f "$declare_file" ]; then
nl -ba "$declare_file" | sed -n '1,220p'
fi
echo "---- Checking SpatialData import of ConsolidatedStore (store/index etc) ----"
rg -n "ConsolidatedStore" packages/core/src -S | head -n 50Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
spec="packages/core/tests/tableAssociations.spec.ts"
echo "---- $spec (line count) ----"
wc -l "$spec"
echo "---- $spec (lines 1-40) ----"
awk 'NR>=1 && NR<=40 {print NR ":" $0}' "$spec"
echo "---- $spec (lines 55-85) ----"
awk 'NR>=55 && NR<=85 {print NR ":" $0}' "$spec"
echo "---- Occurrences of ConsolidatedStore casts ----"
rg -n "as\s+ConsolidatedStore" "$spec"Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 2532
Replace the rootStore as ConsolidatedStore cast with satisfies ConsolidatedStore in the fixture.
SpatialData’s constructor takes rootStore: ConsolidatedStore, and the test fixture currently uses as at the call site, which can mask fixture structural drift.
Suggested diff
- const rootStore = {
+ const rootStore = {
tree: {
shapes: {
cells: {
[ATTRS_KEY]: {
'encoding-type': 'ngff:shapes',
},
},
cell_circles: {
[ATTRS_KEY]: {
'encoding-type': 'ngff:shapes',
},
},
nuclei: {
[ATTRS_KEY]: {
'encoding-type': 'ngff:shapes',
},
},
},
tables: {
cells_table: {
[ATTRS_KEY]: {
instance_key: 'cell_id',
region: 'cells',
region_key: 'region',
'spatialdata-encoding-type': 'ngff:regions_table',
},
},
path_table: {
...
},
xenium_table: {
[ATTRS_KEY]: {
instance_key: 'cell_id',
region: ['cells', 'cell_circles'],
region_key: 'region',
'spatialdata-encoding-type': 'ngff:regions_table',
},
},
},
},
zarritaStore: {},
- };
+ } satisfies ConsolidatedStore;
- return new SpatialData('https://example.com/mock.zarr', rootStore as ConsolidatedStore, [
+ return new SpatialData('https://example.com/mock.zarr', rootStore, [
'shapes',
'tables',
]);🤖 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 `@packages/core/tests/tableAssociations.spec.ts` around lines 69 - 72, Replace
the unsafe type assertion at the SpatialData constructor call by ensuring the
fixture object rootStore is declared with a structural check using the
TypeScript "satisfies ConsolidatedStore" operator instead of "as
ConsolidatedStore"; locate the test fixture that constructs new
SpatialData('https://example.com/mock.zarr', rootStore as ConsolidatedStore,
[...]) and change the declaration of rootStore (not the call site) so it uses
"satisfies ConsolidatedStore" to validate its shape while keeping its inferred
type for downstream code.
Comment on lines
+126
to
127
| const paletteColor = categoricalPalette[index % categoricalPalette.length]; | ||
| colors[featureId] = rgba(paletteColor, alpha); |
Contributor
There was a problem hiding this comment.
Empty categoricalPalette causes invalid color output.
If a caller passes an empty categoricalPalette, index % 0 yields NaN, and categoricalPalette[NaN] returns undefined. This propagates invalid values into the color map.
Consider validating the palette or falling back to the default when empty.
Proposed guard
export function buildShapeFillColorByFeatureId({
featureIds,
rowIndexByFeatureIndex,
column,
mode,
alpha,
categoricalPalette = DEFAULT_SHAPE_CATEGORICAL_PALETTE,
numericRamp = DEFAULT_SHAPE_NUMERIC_RAMP,
}: BuildShapeFillColorByFeatureIdOptions): Record<string, ShapeRgbaColor> {
if (!column) return {};
+ const palette = categoricalPalette.length > 0 ? categoricalPalette : DEFAULT_SHAPE_CATEGORICAL_PALETTE;Then use palette instead of categoricalPalette at line 126.
🤖 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 `@packages/layers/src/shapeColorEncoding.ts` around lines 126 - 127, The code
currently indexes into categoricalPalette without checking for emptiness,
causing NaN/undefined colors; update the logic around the mapping that sets
colors[featureId] to first validate categoricalPalette and fall back to the
default palette variable (or a predefined default) when
categoricalPalette.length === 0, then compute paletteColor = palette[index %
palette.length] and call rgba(paletteColor, alpha) so colors[featureId] always
receives a valid color; modify the block that assigns paletteColor and uses rgba
to reference the safe palette instead of categoricalPalette.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/vis/demo/src/HeadlessBlobsDemo.tsx (1)
25-27: ⚡ Quick winRemove the debug table lookup/log from render path.
This work is unused and logs on every render, which adds noise and unnecessary overhead.
🧹 Proposed cleanup
- const tables = spatialData?.getAssociatedTables("shapes", "blobs_multipolygons"); - console.log(tables);🤖 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 `@packages/vis/demo/src/HeadlessBlobsDemo.tsx` around lines 25 - 27, Remove the debug lookup and console.log from the render path: delete the call that assigns tables from spatialData?.getAssociatedTables("shapes", "blobs_multipolygons") and the subsequent console.log(tables) in the HeadlessBlobsDemo component so you no longer perform an unused lookup or log on every render; if the lookup is needed for future debugging, move it behind a conditional or into a callback/handler (or a useEffect with appropriate deps) and replace console.log with proper logging only where required.
🤖 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 @.github/workflows/test.yml:
- Around line 75-76: Replace the hardcoded port literal "38473" used in the curl
readiness probes with the shared fixture-port variable so the CI uses the
canonical port value; update the two curl invocations (the lines containing
"curl -sSf http://localhost:38473/" and "curl -sSf
http://localhost:38473/v0.5.0/blobs.zarr/zmetadata") and the other occurrences
noted (around the same block) to reference the existing fixture-port variable
(use the correct expansion for the workflow context, e.g. ${{ env.FIXTURE_PORT
}} or $FIXTURE_PORT depending on whether the line runs in a step shell) so all
probes derive from the single source of truth instead of the literal 38473.
In `@docs/docs/vis/spatial-canvas-status.mdx`:
- Line 19: The 3D limitation sentence is missing a verb and reads awkwardly;
update the bullet for SpatialCanvas/SpatialCanvasViewer to read clearly (e.g.,
replace "3D is not supported for MDV v1 integration, but a high priority
following that." with "3D is not supported for MDV v1 integration, but it is a
high priority for future work.") and ensure the surrounding references to Viv
DetailView, ViewState3D, and the MDV v1 integration remain intact and accurate.
In `@scripts/fixture-server-port.mjs`:
- Around line 8-10: The exported FIXTURE_SERVER_PORT currently uses Number(...)
directly which can produce NaN, 0, negatives or non-integers; change the
initialization to parse the chosen env var as an integer (e.g. parseInt on
process.env.SPATIALDATA_FIXTURE_PORT ?? process.env.PORT ??
DEFAULT_FIXTURE_SERVER_PORT), then validate with Number.isInteger and a port
range (1–65535) and if invalid throw a clear Error (e.g. "Invalid
FIXTURE_SERVER_PORT: ...") so the process fails fast; keep
DEFAULT_FIXTURE_SERVER_PORT as the fallback value and ensure the final exported
FIXTURE_SERVER_PORT is a validated integer.
---
Nitpick comments:
In `@packages/vis/demo/src/HeadlessBlobsDemo.tsx`:
- Around line 25-27: Remove the debug lookup and console.log from the render
path: delete the call that assigns tables from
spatialData?.getAssociatedTables("shapes", "blobs_multipolygons") and the
subsequent console.log(tables) in the HeadlessBlobsDemo component so you no
longer perform an unused lookup or log on every render; if the lookup is needed
for future debugging, move it behind a conditional or into a callback/handler
(or a useEffect with appropriate deps) and replace console.log with proper
logging only where required.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf77ec53-c8ac-4eeb-885e-747d08f51139
📒 Files selected for processing (22)
.github/workflows/test.yml.vscode/settings.jsonREADME.mddocs/docs/core/overview.mdxdocs/docs/intro.mdxdocs/docs/layers/overview.mdxdocs/docs/vis/headless-viewer.mdxdocs/docs/vis/mdv-integration.mdxdocs/docs/vis/mdv-release-checklist.mdxdocs/docs/vis/overview.mdxdocs/docs/vis/spatial-canvas-status.mdxpackages/core/README.mdpackages/vis/demo/src/App.tsxpackages/vis/demo/src/HeadlessBlobsDemo.tsxpackages/vis/demo/src/buildHeadlessLayers.tspackages/vis/demo/src/fixtureUrls.tspackages/vis/scripts/dev.mjspackages/vis/vite.config.demo.tsscripts/fixture-server-defaults.mjsscripts/fixture-server-port.mjsscripts/test-server.jstests/integration/fixtures.test.ts
✅ Files skipped from review due to trivial changes (12)
- .vscode/settings.json
- docs/docs/vis/overview.mdx
- packages/core/README.md
- packages/vis/demo/src/fixtureUrls.ts
- packages/vis/demo/src/buildHeadlessLayers.ts
- scripts/fixture-server-defaults.mjs
- docs/docs/layers/overview.mdx
- docs/docs/vis/headless-viewer.mdx
- README.md
- docs/docs/intro.mdx
- docs/docs/vis/mdv-integration.mdx
- docs/docs/vis/mdv-release-checklist.mdx
Comment on lines
+75
to
+76
| if curl -sSf http://localhost:38473/ >/dev/null && \ | ||
| curl -sSf http://localhost:38473/v0.5.0/blobs.zarr/zmetadata >/dev/null; then |
Contributor
There was a problem hiding this comment.
Avoid hardcoding fixture port in CI probe.
The workflow duplicates 38473 instead of deriving from one variable, so it can drift from the shared fixture-port config and break readiness checks.
Suggested fix
- name: Run integration tests with local server
shell: bash
run: |
+ FIXTURE_PORT="${SPATIALDATA_FIXTURE_PORT:-38473}"
+
# Verify fixtures exist before starting server
if [ ! -d "test-fixtures/v0.5.0/blobs.zarr" ]; then
echo "Error: Fixtures not found at test-fixtures/v0.5.0/blobs.zarr"
echo "Listing test-fixtures directory:"
ls -la test-fixtures/ || echo "test-fixtures directory does not exist"
exit 1
fi
# Start test server in background
- pnpm test:server &
+ SPATIALDATA_FIXTURE_PORT="$FIXTURE_PORT" pnpm test:server &
SERVER_PID=$!
@@
- if curl -sSf http://localhost:38473/ >/dev/null && \
- curl -sSf http://localhost:38473/v0.5.0/blobs.zarr/zmetadata >/dev/null; then
+ if curl -sSf "http://localhost:${FIXTURE_PORT}/" >/dev/null && \
+ curl -sSf "http://localhost:${FIXTURE_PORT}/v0.5.0/blobs.zarr/zmetadata" >/dev/null; then
echo "Test server is up and serving fixtures"
break
fi
- echo "Waiting for test server on http://localhost:38473/ ..."
+ echo "Waiting for test server on http://localhost:${FIXTURE_PORT}/ ..."
sleep 1
done
@@
- # Run integration tests (will hit http://localhost:38473/…)
+ # Run integration tests (will hit http://localhost:${FIXTURE_PORT}/…)
pnpm test:integrationAlso applies to: 80-80, 87-87
🤖 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 @.github/workflows/test.yml around lines 75 - 76, Replace the hardcoded port
literal "38473" used in the curl readiness probes with the shared fixture-port
variable so the CI uses the canonical port value; update the two curl
invocations (the lines containing "curl -sSf http://localhost:38473/" and "curl
-sSf http://localhost:38473/v0.5.0/blobs.zarr/zmetadata") and the other
occurrences noted (around the same block) to reference the existing fixture-port
variable (use the correct expansion for the workflow context, e.g. ${{
env.FIXTURE_PORT }} or $FIXTURE_PORT depending on whether the line runs in a
step shell) so all probes derive from the single source of truth instead of the
literal 38473.
|
|
||
| ## Known limitations | ||
|
|
||
| - **No 3D rendering:** `SpatialCanvas`, `SpatialCanvasViewer`, and the underlying Viv/deck stack are **2D Cartesian only**. The viewer uses Viv `DetailView` (orthographic pan/zoom), not `OrbitView` or volume rendering. You can select an image **`z`** slice (or `c` / `t`) through `LayerConfig.channels`, but that is OME axis indexing — not 3D scene navigation. `ViewState` includes a `ViewState3D` type stub, but pitch, bearing, and orbit state are not wired through to rendering. **3D is not supported for MDV v1 integration, but a high priority following that.** |
Contributor
There was a problem hiding this comment.
Fix the broken sentence in the 3D limitation bullet.
The clause reads awkwardly and drops a verb (“but a high priority…”), which hurts docs clarity.
✏️ Proposed wording fix
-- **No 3D rendering:** `SpatialCanvas`, `SpatialCanvasViewer`, and the underlying Viv/deck stack are **2D Cartesian only**. The viewer uses Viv `DetailView` (orthographic pan/zoom), not `OrbitView` or volume rendering. You can select an image **`z`** slice (or `c` / `t`) through `LayerConfig.channels`, but that is OME axis indexing — not 3D scene navigation. `ViewState` includes a `ViewState3D` type stub, but pitch, bearing, and orbit state are not wired through to rendering. **3D is not supported for MDV v1 integration, but a high priority following that.**
+- **No 3D rendering:** `SpatialCanvas`, `SpatialCanvasViewer`, and the underlying Viv/deck stack are **2D Cartesian only**. The viewer uses Viv `DetailView` (orthographic pan/zoom), not `OrbitView` or volume rendering. You can select an image **`z`** slice (or `c` / `t`) through `LayerConfig.channels`, but that is OME axis indexing — not 3D scene navigation. `ViewState` includes a `ViewState3D` type stub, but pitch, bearing, and orbit state are not wired through to rendering. **3D is not supported for MDV v1 integration, but it is a high priority afterward.**📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **No 3D rendering:** `SpatialCanvas`, `SpatialCanvasViewer`, and the underlying Viv/deck stack are **2D Cartesian only**. The viewer uses Viv `DetailView` (orthographic pan/zoom), not `OrbitView` or volume rendering. You can select an image **`z`** slice (or `c` / `t`) through `LayerConfig.channels`, but that is OME axis indexing — not 3D scene navigation. `ViewState` includes a `ViewState3D` type stub, but pitch, bearing, and orbit state are not wired through to rendering. **3D is not supported for MDV v1 integration, but a high priority following that.** | |
| - **No 3D rendering:** `SpatialCanvas`, `SpatialCanvasViewer`, and the underlying Viv/deck stack are **2D Cartesian only**. The viewer uses Viv `DetailView` (orthographic pan/zoom), not `OrbitView` or volume rendering. You can select an image **`z`** slice (or `c` / `t`) through `LayerConfig.channels`, but that is OME axis indexing — not 3D scene navigation. `ViewState` includes a `ViewState3D` type stub, but pitch, bearing, and orbit state are not wired through to rendering. **3D is not supported for MDV v1 integration, but it is a high priority afterward.** |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...supported for MDV v1 integration, but a high priority following that.** - **Channel UI is bas...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🤖 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/docs/vis/spatial-canvas-status.mdx` at line 19, The 3D limitation
sentence is missing a verb and reads awkwardly; update the bullet for
SpatialCanvas/SpatialCanvasViewer to read clearly (e.g., replace "3D is not
supported for MDV v1 integration, but a high priority following that." with "3D
is not supported for MDV v1 integration, but it is a high priority for future
work.") and ensure the surrounding references to Viv DetailView, ViewState3D,
and the MDV v1 integration remain intact and accurate.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.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.
Summary
spatialdataas the source of truth for feature/table association semantics in the docs.@spatialdata/coreas the owner ofFeatureTableAlignmentand the row-alignment helpers, with@spatialdata/layersconsuming resolved alignment for colour encoding.Testing
pnpm --filter docs buildpassed.@spatialdata/core,@spatialdata/layers, and@spatialdata/vistests/builds passed, plus targeted Biome checks on the touched implementation files.Summary by CodeRabbit
New Features
Documentation
Updates