Clarify prop-to-layer flow and move render stack ownership into layers - #49
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 9 minutes and 21 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughIntroduces a versioned ChangesRenderStack Schema, Adapters, Viewer Wiring, and Docs
Sequence Diagram(s)sequenceDiagram
participant Caller as MDV / Demo App
participant SpatialCanvasViewerInner
participant renderStackToLayerInputs
participant resolveRenderStackHostLayers
participant useSpatialCanvasRendererFromLayerInputs
participant useLayerData
Caller->>SpatialCanvasViewerInner: renderStack + hostLayerResolver + onFeatureHover/onFeatureClick
SpatialCanvasViewerInner->>renderStackToLayerInputs: renderStack entries (spatial)
renderStackToLayerInputs-->>SpatialCanvasViewerInner: { layers, layerOrder }
SpatialCanvasViewerInner->>resolveRenderStackHostLayers: renderStack entries (host) + resolver
resolveRenderStackHostLayers-->>SpatialCanvasViewerInner: hostDeckLayers[]
SpatialCanvasViewerInner->>useSpatialCanvasRendererFromLayerInputs: layerInputs + hostDeckLayers + renderOrder
useSpatialCanvasRendererFromLayerInputs->>useLayerData: spatialData + layerInputs
useLayerData-->>useSpatialCanvasRendererFromLayerInputs: getFeaturePickEvent()
useSpatialCanvasRendererFromLayerInputs-->>SpatialCanvasViewerInner: renderer (deckLayers, layerOrder, getFeaturePickEvent)
SpatialCanvasViewerInner->>Caller: onFeatureHover(SpatialFeaturePickEvent)
SpatialCanvasViewerInner->>Caller: onFeatureClick(SpatialFeaturePickEvent)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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 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: 3
🧹 Nitpick comments (4)
packages/layers/tests/renderStack.spec.ts (1)
89-105: ⚡ Quick winAdd a regression test for duplicate
entry.idrejection.Current tests assert ordering but not uniqueness enforcement. Add one case that expects
renderStackSchema.safeParseto fail when two entries share the sameid.🤖 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/tests/renderStack.spec.ts` around lines 89 - 105, The test suite for renderStackSchema needs to add a regression test case that verifies duplicate entry IDs are rejected by the schema validation. Add a new test that creates a stack object with duplicate id values in the entries array and calls renderStackSchema.safeParse() on it, then assert that the parsing fails (the result should indicate validation errors, not success). This ensures the schema enforces uniqueness constraints on entry IDs alongside the existing tests that verify ordering behavior of getRenderStackEntryIds and getRenderStackHostLayerIds.packages/vis/demo/src/buildHeadlessLayers.ts (1)
49-49: ⚡ Quick winUse the shared RenderStack schema version constant
Line 49 hardcodes
schemaVersion: 1; this can drift from the canonical version and silently desync demo output from the RenderStack contract.Suggested diff
-import type { RenderStack, RenderStackSpatialElementType } from '../../src/index'; +import { + RENDER_STACK_SCHEMA_VERSION, + type RenderStack, + type RenderStackSpatialElementType, +} from '../../src/index'; ... - return { schemaVersion: 1, entries }; + return { schemaVersion: RENDER_STACK_SCHEMA_VERSION, entries };🤖 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/buildHeadlessLayers.ts` at line 49, The return statement in the buildHeadlessLayers function hardcodes schemaVersion as the literal value 1, which can drift from the canonical RenderStack schema version constant. Replace the hardcoded numeric value 1 with the shared RenderStack schema version constant, and ensure that constant is imported at the top of the file if it is not already.packages/vis/tests/spatialCanvasViewer.spec.ts (1)
69-78: ⚡ Quick winAdd a regression test for host resolvers that return
Layer[].Current coverage only exercises single-layer host resolution. A case with one host entry resolving to multiple layers (and a spatial entry after it) would catch ordering regressions in adapter/sort integration.
Also applies to: 96-108
🤖 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/tests/spatialCanvasViewer.spec.ts` around lines 69 - 78, Add a new test case after the existing 'resolves host descriptors into deck layers with stack ids' test that specifically exercises host resolvers returning multiple layers. Create a test where a single host entry in the renderStackSchema resolves to multiple layers via the host resolver function, and include a spatial entry after it in the stack configuration. Verify that the resolved layers maintain proper ordering, with the multiple layers from the host resolver appearing before the spatial layer. This will provide regression test coverage for the adapter/sort integration when handling multi-layer host resolution scenarios.packages/vis/src/SpatialCanvas/renderStackAdapters.ts (1)
44-50: ⚡ Quick winReplace
asassertions with typed narrowing in the adapter boundary.At Line 44-50, these casts are avoidable and weaken the type contract for this conversion path.
Refactor sketch
case 'image': { - return { ...base, type: 'image' } as ImageLayerConfig; + const config: ImageLayerConfig = { ...base, type: 'image' }; + return config; } case 'shapes': { - return { ...base, type: 'shapes' } as ShapesLayerConfig; + const config: ShapesLayerConfig = { ...base, type: 'shapes' }; + return config; } case 'points': { - return { ...base, type: 'points' } as PointsLayerConfig; + const config: PointsLayerConfig = { ...base, type: 'points' }; + return config; } case 'labels': { - return { ...base, type: 'labels' } as LabelsLayerConfig; + const config: LabelsLayerConfig = { ...base, type: 'labels' }; + return config; }As per coding guidelines, “Avoid type assertions (
as); usesatisfies, discriminated unions, and small helpers that return precise types.”🤖 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/src/SpatialCanvas/renderStackAdapters.ts` around lines 44 - 50, The switch statement in the adapter is using `as` type assertions to cast return values for ImageLayerConfig, ShapesLayerConfig, PointsLayerConfig, and LabelsLayerConfig, which weakens the type contract. Replace these assertions by either creating separate typed helper functions for each case that naturally return the correct layer config type, or restructure the switch cases to leverage a discriminated union pattern where the type property itself provides the necessary type narrowing to eliminate the need for 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.
Inline comments:
In `@packages/layers/src/renderStack.ts`:
- Around line 56-59: The renderStackSchema does not validate that all entries in
the entries array have unique id values, which can cause issues with stable
ordering and host/layer mapping. Add a refinement to the renderStackSchema
object after defining the base schema that validates the entries array contains
no duplicate entry.id values by checking that the number of unique IDs equals
the total number of entries, using appropriate Zod refinement methods like
superRefine() to provide a clear error message when duplicate IDs are detected.
- Around line 15-31: The renderStackEntryBaseSchema and
renderStackSpatialEntrySchema (along with other object schemas in the file at
lines 35, 56 and the ranges 33-39, 41-48, 56-59) currently use non-strict
z.object() calls which silently accept and drop unknown keys, weakening contract
validation. Convert all these z.object() definitions to use strict parsing by
chaining .strict() after the object() call and its property definitions. This
ensures that any unknown properties in persisted RenderStack configurations are
rejected during validation rather than silently ignored, aligning with the
strict schema guideline.
In `@packages/vis/src/SpatialCanvas/renderStackAdapters.ts`:
- Around line 82-85: When a resolver returns multiple Layer instances
(Array.isArray(resolved) is true and compact.length > 1), those layers are
pushed without cloning and assigning the entry.id, which causes them to retain
unknown runtime ids. This breaks stack ordering because unknown ids are sorted
after ordered ids in the sorting logic at lines 102-117. Fix this by cloning
each layer in the compact array with the entry.id (similar to what's done in the
compact.length === 1 case), so all resolved layers maintain their declared
render-stack position regardless of whether the resolver returns a single layer
or multiple layers.
---
Nitpick comments:
In `@packages/layers/tests/renderStack.spec.ts`:
- Around line 89-105: The test suite for renderStackSchema needs to add a
regression test case that verifies duplicate entry IDs are rejected by the
schema validation. Add a new test that creates a stack object with duplicate id
values in the entries array and calls renderStackSchema.safeParse() on it, then
assert that the parsing fails (the result should indicate validation errors, not
success). This ensures the schema enforces uniqueness constraints on entry IDs
alongside the existing tests that verify ordering behavior of
getRenderStackEntryIds and getRenderStackHostLayerIds.
In `@packages/vis/demo/src/buildHeadlessLayers.ts`:
- Line 49: The return statement in the buildHeadlessLayers function hardcodes
schemaVersion as the literal value 1, which can drift from the canonical
RenderStack schema version constant. Replace the hardcoded numeric value 1 with
the shared RenderStack schema version constant, and ensure that constant is
imported at the top of the file if it is not already.
In `@packages/vis/src/SpatialCanvas/renderStackAdapters.ts`:
- Around line 44-50: The switch statement in the adapter is using `as` type
assertions to cast return values for ImageLayerConfig, ShapesLayerConfig,
PointsLayerConfig, and LabelsLayerConfig, which weakens the type contract.
Replace these assertions by either creating separate typed helper functions for
each case that naturally return the correct layer config type, or restructure
the switch cases to leverage a discriminated union pattern where the type
property itself provides the necessary type narrowing to eliminate the need for
assertions.
In `@packages/vis/tests/spatialCanvasViewer.spec.ts`:
- Around line 69-78: Add a new test case after the existing 'resolves host
descriptors into deck layers with stack ids' test that specifically exercises
host resolvers returning multiple layers. Create a test where a single host
entry in the renderStackSchema resolves to multiple layers via the host resolver
function, and include a spatial entry after it in the stack configuration.
Verify that the resolved layers maintain proper ordering, with the multiple
layers from the host resolver appearing before the spatial layer. This will
provide regression test coverage for the adapter/sort integration when handling
multi-layer host resolution scenarios.
🪄 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: a679e8fc-a862-4468-9920-478f3f2a0fa3
📒 Files selected for processing (24)
CONTEXT.mddocs/adr/0001-render-stack-owned-by-layers.mddocs/docs/intro.mdxdocs/docs/layers/overview.mdxdocs/docs/vis/headless-viewer.mdxdocs/docs/vis/layer-prop-flow.mdxdocs/docs/vis/mdv-integration.mdxdocs/docs/vis/mdv-release-checklist.mdxdocs/docs/vis/overview.mdxpackages/layers/src/index.tspackages/layers/src/renderStack.tspackages/layers/tests/renderStack.spec.tspackages/vis/demo/src/CodecFixtureDemo.tsxpackages/vis/demo/src/HeadlessBlobsDemo.tsxpackages/vis/demo/src/buildHeadlessLayers.tspackages/vis/demo/tsconfig.jsonpackages/vis/src/SpatialCanvas/SpatialCanvasViewer.tsxpackages/vis/src/SpatialCanvas/VivSpatialViewer.tsxpackages/vis/src/SpatialCanvas/index.tsxpackages/vis/src/SpatialCanvas/public.tspackages/vis/src/SpatialCanvas/renderStackAdapters.tspackages/vis/src/SpatialCanvas/useLayerData.tspackages/vis/src/index.tspackages/vis/tests/spatialCanvasViewer.spec.ts
validate unique layer id. avoid potential issue with stack-order breaking by always cloning layers (hopefully not too garbage-intensive).
Merged
This was referenced Jul 14, 2026
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
@spatialdata/layersand adapts@spatialdata/visto consume it.Testing
Summary by CodeRabbit
New Features
RenderStack-based rendering API to replace separate layer configuration, providing a unified model for spatial and overlay layers with explicit ordering control.onFeatureHover,onFeatureClick) for improved runtime event handling.Documentation