Add shape fill-by-column controls and outline defaults - #39
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 48 minutes and 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. 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 (3)
📝 WalkthroughWalkthroughAdds per-column fill-color encoding, positional feature-row alignment, a shape feature-state runtime, hover-tooltip aggregation via DeckGL refs, a ShapeFillColorPanel UI, renderer/schema updates for stroke-width, async caching for derived fill-color state, tests, and documentation updates. ChangesMulti-element shape visualization with fill colors and tooltip aggregation
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 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 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/layers/src/spatialLayerProps.ts (1)
29-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd cross-field validation for
defaultStrokeWidthMinPixels <= defaultStrokeWidthMaxPixelsinpackages/layers/src/spatialLayerProps.ts— the schema allowsmin > max, andpackages/layers/src/shapesLayer.tsforwardslineWidthMinPixels/lineWidthMaxPixelsto deck.gl without additional checks. Enforce the relationship at the Zod schema boundary withsuperRefine.Suggested fix
export const spatialShapesSublayerSchema = sublayerBase.extend({ kind: z.literal('shapes'), elementKey: z.string(), tooltipFields: z.array(z.string()).optional(), defaultFillColor: rgbaColorSchema.optional(), defaultStrokeColor: rgbaColorSchema.optional(), defaultStrokeWidth: z.number().min(0).optional(), defaultStrokeWidthUnits: z.enum(['common', 'pixels']).optional(), defaultStrokeWidthMinPixels: z.number().min(0).optional(), defaultStrokeWidthMaxPixels: z.number().min(0).optional(), featureState: z .object({ fillColorByFeatureId: z.record(z.string(), rgbaColorSchema).optional(), strokeColorByFeatureId: z.record(z.string(), rgbaColorSchema).optional(), hiddenFeatureIds: z.array(z.string()).optional(), fadedFeatureIds: z.array(z.string()).optional(), filteredOpacityMultiplier: z.number().min(0).max(1).optional(), }) .optional(), -}); +}).superRefine((value, ctx) => { + if ( + value.defaultStrokeWidthMinPixels !== undefined && + value.defaultStrokeWidthMaxPixels !== undefined && + value.defaultStrokeWidthMinPixels > value.defaultStrokeWidthMaxPixels + ) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'defaultStrokeWidthMinPixels must be <= defaultStrokeWidthMaxPixels', + path: ['defaultStrokeWidthMinPixels'], + }); + } +});🤖 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/spatialLayerProps.ts` around lines 29 - 48, The spatialShapesSublayerSchema currently allows defaultStrokeWidthMinPixels > defaultStrokeWidthMaxPixels; update spatialShapesSublayerSchema to call .superRefine(...) and, inside the refine callback, if both defaultStrokeWidthMinPixels and defaultStrokeWidthMaxPixels are defined and min > max, add a ZodIssue (pathing to ['defaultStrokeWidthMinPixels'] and/or ['defaultStrokeWidthMaxPixels']) with a helpful message enforcing "defaultStrokeWidthMinPixels must be <= defaultStrokeWidthMaxPixels" so invalid configs are rejected at schema validation time.
🤖 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 `@AGENTS.md`:
- Around line 19-22: The example path in the guidance that shows using
Volta-managed pnpm is machine-specific ("/Users/ptodd/.volta/bin/pnpm"); update
the sentence that follows "Use the Node.js and pnpm versions pinned in
`package.json` under `volta`" to use a portable example such as
"~/.volta/bin/pnpm" or the dynamic form "$(volta which pnpm)" instead of the
hardcoded user path so the recommendation applies across environments.
In `@packages/vis/src/SpatialCanvas/featureTooltipHover.ts`:
- Around line 28-34: The deck property in ResolveHoverFeatureTooltipOptions and
the corresponding parameter on collectPicks should be narrowed to an interface
exposing only the pickMultipleObjects method instead of the full Deck type;
change ResolveHoverFeatureTooltipOptions.deck to type { pickMultipleObjects:
(opts: {x: number; y: number; radius?: number; layerIds?: any[]; depth?:
number;}) => any } | null (or a suitably named PickMultipleInterface) and update
collectPicks's deck parameter to accept that same interface so tests/mocks can
implement pickMultipleObjects without casting to Deck; keep nullable semantics
and preserve pickRadius/pickDepth usage.
In `@packages/vis/src/SpatialCanvas/ShapeFillColorPanel.tsx`:
- Around line 39-58: The "Fill colour" text is not programmatically associated
with the <select>, hurting accessibility; update the JSX in ShapeFillColorPanel
so the select is labeled: either wrap the select in a <label> or add an id to
the <select> and a corresponding htmlFor on a <label> that contains the visible
text (keep existing styles like helperTextStyle/selectStyle and the same
value/onChange behavior using selected, availableFields and onChange). Ensure
the label text matches the displayed "Fill colour" and that the new id is unique
within this component.
In `@packages/vis/src/SpatialCanvas/useLayerData.ts`:
- Around line 334-336: The cache key generation for the color maps uses string
interpolation `${fillColors}` and `${strokeColors}`, which yields "[object
Object]" and causes signature collisions when mapping contents change; update
the key construction in useLayerData (the array that currently includes
`fillColors ? \`\\x02${Object.keys(fillColors).length}:${fillColors}\` : ''` and
the similar `strokeColors` entry) to serialize the actual mapping contents (for
example by using a stable serializer like JSON.stringify on a sorted list of
entries or joining Object.entries after sorting) so the key reflects both count
and content rather than the object’s default string form.
---
Outside diff comments:
In `@packages/layers/src/spatialLayerProps.ts`:
- Around line 29-48: The spatialShapesSublayerSchema currently allows
defaultStrokeWidthMinPixels > defaultStrokeWidthMaxPixels; update
spatialShapesSublayerSchema to call .superRefine(...) and, inside the refine
callback, if both defaultStrokeWidthMinPixels and defaultStrokeWidthMaxPixels
are defined and min > max, add a ZodIssue (pathing to
['defaultStrokeWidthMinPixels'] and/or ['defaultStrokeWidthMaxPixels']) with a
helpful message enforcing "defaultStrokeWidthMinPixels must be <=
defaultStrokeWidthMaxPixels" so invalid configs are rejected at schema
validation time.
🪄 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: 126e3219-7af8-43dd-a16b-14d2befa7256
📒 Files selected for processing (26)
AGENTS.mddocs/docs/vis/layer-prop-flow.mdxpackages/core/src/tableAssociations.tspackages/core/src/tooltip.tspackages/core/tests/shapesRenderData.spec.tspackages/core/tests/tooltipDisplay.spec.tspackages/layers/src/index.tspackages/layers/src/shapesLayer.tspackages/layers/src/spatialLayerProps.tspackages/layers/tests/shapesLayer.spec.tspackages/layers/tests/spatialLayerProps.spec.tspackages/vis/src/SpatialCanvas/ShapeFillColorPanel.tsxpackages/vis/src/SpatialCanvas/SpatialCanvasViewer.tsxpackages/vis/src/SpatialCanvas/SpatialFeatureTooltip.tsxpackages/vis/src/SpatialCanvas/SpatialViewer.tsxpackages/vis/src/SpatialCanvas/VivSpatialViewer.tsxpackages/vis/src/SpatialCanvas/featureTooltipHover.tspackages/vis/src/SpatialCanvas/index.tsxpackages/vis/src/SpatialCanvas/public.tspackages/vis/src/SpatialCanvas/renderers/shapesRenderer.tspackages/vis/src/SpatialCanvas/shapeColorEncoding.tspackages/vis/src/SpatialCanvas/types.tspackages/vis/src/SpatialCanvas/useLayerData.tspackages/vis/src/index.tspackages/vis/tests/featureTooltipHover.spec.tspackages/vis/tests/shapeColorEncoding.spec.ts
Contributor
|
Actionable comments posted: 0 |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vis/src/SpatialCanvas/featureTooltipHover.ts (1)
37-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix hover tooltip aggregation:
pickLayerIdsmust use deck.gl concrete renderedlayer.idstrings
ResolveHoverFeatureTooltipOptions.pickLayerIdsis documented as “Candidate logical deck layer ids”, butcollectPicksforwards them directly todeck.pickMultipleObjects({ layerIds }). In deck.gl,layerIdsmatches only the exact renderedlayer.idvalues (for composite layers, the concrete sublayer ids), so any “logical”/suffixed ids that are normalized only after picking won’t match and aggregated hover tooltips silently miss those sections. Either plumb the exact rendered deck ids intolayerIdsbefore the pick call, or remove/replace thelayerIdsfilter until a correct logical→rendered mapping exists (applies to 57-78 too).🤖 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/featureTooltipHover.ts` around lines 37 - 42, The pickLayerIds option in ResolveHoverFeatureTooltipOptions is passing logical layer ids straight into collectPicks / deck.pickMultipleObjects, but deck.gl expects concrete rendered layer.id strings (including sublayer suffixes); update the code that calls collectPicks (or resolveHoverFeatureTooltip) to map the provided pickLayerIds from logical ids to the actual rendered deck layer ids before forwarding them to deck.pickMultipleObjects (or else remove the layerIds filtering temporarily). Locate uses of ResolveHoverFeatureTooltipOptions.pickLayerIds, the collectPicks helper, and the deck.pickMultipleObjects call and ensure you either translate logical → rendered ids (by querying deck.getLayers() / inspecting layer.id on rendered layers) or stop passing layerIds through until a proper mapping function exists.
🤖 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.
Outside diff comments:
In `@packages/vis/src/SpatialCanvas/featureTooltipHover.ts`:
- Around line 37-42: The pickLayerIds option in
ResolveHoverFeatureTooltipOptions is passing logical layer ids straight into
collectPicks / deck.pickMultipleObjects, but deck.gl expects concrete rendered
layer.id strings (including sublayer suffixes); update the code that calls
collectPicks (or resolveHoverFeatureTooltip) to map the provided pickLayerIds
from logical ids to the actual rendered deck layer ids before forwarding them to
deck.pickMultipleObjects (or else remove the layerIds filtering temporarily).
Locate uses of ResolveHoverFeatureTooltipOptions.pickLayerIds, the collectPicks
helper, and the deck.pickMultipleObjects call and ensure you either translate
logical → rendered ids (by querying deck.getLayers() / inspecting layer.id on
rendered layers) or stop passing layerIds through until a proper mapping
function exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9b86ba8-ae91-4be5-b150-8eef8d38e846
📒 Files selected for processing (5)
packages/vis/src/SpatialCanvas/SpatialCanvasViewer.tsxpackages/vis/src/SpatialCanvas/SpatialViewer.tsxpackages/vis/src/SpatialCanvas/featureTooltipHover.tspackages/vis/src/SpatialCanvas/index.tsxpackages/vis/tests/featureTooltipHover.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/vis/src/SpatialCanvas/SpatialViewer.tsx
- packages/vis/src/SpatialCanvas/index.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/vis/src/SpatialCanvas/shapeColorEncoding.ts (1)
97-101: 💤 Low valueOpen design questions left inline — worth tracking, not blocking.
You've flagged three valid concerns in-comment: feature-id↔row association should be a single consistent mechanism, this is a per-feature hot path, and the resolution arguably belongs in a shared layer helper rather than
vis/SpatialCanvas. The current implementation is correct, but these comments will rot if left untracked. Consider extractingresolveShapeFillColorRowIndexinto@spatialdata/layers(shared by labels/shapes) once the association contract is settled, and replacing the musing with a tracked reference.Want me to open an issue capturing the row-association consolidation and hot-path concern?
🤖 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/shapeColorEncoding.ts` around lines 97 - 101, The inline TODOs about feature-id↔row association, hot-path performance, and placement in vis/SpatialCanvas should be converted into tracked work and the shared logic extracted: move resolveShapeFillColorRowIndex out of vis/SpatialCanvas into a shared helper in `@spatialdata/layers` (so labels and shapes reuse a single association contract), replace the informal comments in shapeColorEncoding.ts with a single TODO that references the new issue/PR, and ensure resolveShapeFillColorRowIndex’s API is optimized for the hot path (minimal allocations and clear input types) before importing it back into SpatialCanvas.packages/vis/src/SpatialCanvas/featureTooltipHover.ts (1)
77-90: 💤 Low valueAccessing internal deck.gl API — consider adding a maintenance note.
Lines 80-87 reach into
deck.layerManager.getLayers()which is not part of deck.gl's public API surface. The defensiveReflect.getapproach avoids crashes if the structure changes, but a future deck.gl update could silently break layer resolution.Consider adding a brief comment documenting why this internal access is needed and which deck.gl version it was verified against, so future maintainers know to re-verify after upgrades.
🤖 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/featureTooltipHover.ts` around lines 77 - 90, collectCurrentDeckLayerIds currently reaches into deck.gl internals via Reflect.get on deck -> layerManager -> getLayers to resolve nested layers; add a concise maintenance comment above this logic explaining that internal API access is intentional, which deck.gl major/minor version this was validated against, the reason (e.g., to access flattened layers not exposed publicly), and a TODO to re-verify when upgrading deck.gl; reference the function name collectCurrentDeckLayerIds and the local symbols layerManager and getLayers so future maintainers can easily find and reassess this hack when updating dependencies.
🤖 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 `@packages/vis/src/SpatialCanvas/featureTooltipHover.ts`:
- Around line 77-90: collectCurrentDeckLayerIds currently reaches into deck.gl
internals via Reflect.get on deck -> layerManager -> getLayers to resolve nested
layers; add a concise maintenance comment above this logic explaining that
internal API access is intentional, which deck.gl major/minor version this was
validated against, the reason (e.g., to access flattened layers not exposed
publicly), and a TODO to re-verify when upgrading deck.gl; reference the
function name collectCurrentDeckLayerIds and the local symbols layerManager and
getLayers so future maintainers can easily find and reassess this hack when
updating dependencies.
In `@packages/vis/src/SpatialCanvas/shapeColorEncoding.ts`:
- Around line 97-101: The inline TODOs about feature-id↔row association,
hot-path performance, and placement in vis/SpatialCanvas should be converted
into tracked work and the shared logic extracted: move
resolveShapeFillColorRowIndex out of vis/SpatialCanvas into a shared helper in
`@spatialdata/layers` (so labels and shapes reuse a single association contract),
replace the informal comments in shapeColorEncoding.ts with a single TODO that
references the new issue/PR, and ensure resolveShapeFillColorRowIndex’s API is
optimized for the hot path (minimal allocations and clear input types) before
importing it back into SpatialCanvas.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a22a18a4-6e53-4477-9732-5df0291dc5d4
📒 Files selected for processing (4)
packages/vis/src/SpatialCanvas/featureTooltipHover.tspackages/vis/src/SpatialCanvas/shapeColorEncoding.tspackages/vis/tests/featureTooltipHover.spec.tspackages/vis/tests/shapeColorEncoding.spec.ts
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
SpatialCanvasthat maps table-backed columns to per-feature fill colours.Testing
@spatialdata/vis,@spatialdata/layers, and@spatialdata/coretest suites locally.Summary by CodeRabbit
New Features
Improvements
Documentation
Tests