Shapes: non-blocking off-thread loading + vertex-pulling FlatPolygonLayer; live picking + shared tooltip - #88
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughShapes now load through transferable flat buffers, tessellate in a worker when available, render polygons with a texture-driven ChangesShapes rendering and loading
Hover and tooltip interaction
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ShapesResolver
participant VShapesSource
participant GeometryWorker
participant FlatPolygonLayer
ShapesResolver->>VShapesSource: request shapes render data
VShapesSource->>GeometryWorker: decode and tessellate geometry
GeometryWorker-->>VShapesSource: return transferable flat buffers
VShapesSource-->>ShapesResolver: provide polygonBinary and bounds
ShapesResolver->>FlatPolygonLayer: render binary polygon topology
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…ayer Shapes no longer gate first paint. The geometry worker decodes WKB -> flat buffers and tessellates the render topology, transferring both back zero-copy; ShapesResolver.blockingResources = [] with a main-thread tessellation fallback. This removes the full-element main-thread WKB decode behind the loading overlay and lets Visium HD square_002um (~2.7M polygons) load without running out of memory. Polygon shapes render through a new hand-rolled FlatPolygonLayer (@spatialdata/layers) instead of SolidPolygonLayer + a PathLayer outline: - Vertex pulling: an attribute-less draw where the vertex shader reconstructs each vertex's position + boundary edge-distance from two shared geometry textures via gl_VertexID, and imputes an fwidth anti-aliased outline in the fragment shader (no separate outline layer). Per-frame cost ~ the fill; geometry memory ~ the stock indexed fill; arbitrary polygons at ~2.7M scale. - Feature state via a per-feature colour texture (the reusable "table column -> buffer" primitive): colour-by-column / hide / fade re-upload only a small texture, never the geometry buffers. Picking is computed in-shader from the feature index. - Outline is a lightened derivation of the fill, width-capped to a fraction of each shape's on-screen size and faded out for sub-pixel shapes (clear zoomed in, non-dominating / no moire zoomed out). Also decouples the one-shot auto-fit from the shapes-blocking transition, and fixes the fill-colour "one column behind" bug and the hover/pan buffer-thrash from unstable deck updateTrigger arrays. New public surface: core exports tessellateFlatPolygons / TessellatedPolygons and carries the tessellated topology on ShapesRenderData; layers exports FlatPolygonLayer. Includes docs updates and a changeset. Known follow-up: the main-thread GPU texture upload (~seconds on the largest elements) is not yet off the main thread; a WGSL/WebGPU variant (storage buffers instead of texture-packing) is the intended fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two shapes layers with no explicit featureState both resolve to the same EMPTY_SHAPE_FEATURE_STATE_RUNTIME singleton, so a runtime-keyed colour cache held one slot the layers evicted each other in — a per-frame rebuild of both (million-element) buffers on every getLayers() (pan/hover). Pan start surfaced it as a createShapesLayer block dominated by getFeatureColors. Key the cache by layer id, reusing the entry only while (runtime, featureIds, defaultColor) keep identity — the exact inputs the buffer depends on. Two layers get two slots; the buffer stays stable across bare re-renders and rebuilds precisely on a real feature-state/element/default-colour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The `pickingEnabled = … && !interacting` guard disabled shape picking + autoHighlight during camera moves to avoid deck's full-geometry picking-buffer draw on the old SolidPolygonLayer + PathLayer path. The FlatPolygonLayer picking pass is a single vertex-pulled draw, so the guard is no longer needed; picking now stays live through pan/zoom (verified smooth on Visium HD square_002um with two shapes layers). Pulled at both call sites — SpatialCanvasInner (full-UI) and SpatialCanvasViewer (headless). The interaction gate is still driven via onInteractionStateChange, so restoring the guard is a one-line change per site. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Now that shape picking stays live through pan/zoom (single vertex-pulled pick pass), the more informative aggregate tooltip — every feature under the cursor across layers — is affordable as the default. Changed at both entry points (SpatialCanvasInner, SpatialCanvasViewer) and their docstrings; the UI selector still switches modes live and callers can still pass an explicit mode. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The pan/zoom picking gate is gone (picking stays live on the FlatPolygonLayer path), so nothing consumes the interaction state anymore. Delete useViewInteractionGate and its wiring at both call sites: - SpatialCanvasViewer: drop the hook and the mergedDeckProps merge; the caller's deckProps (incl. their own onInteractionStateChange) now passes straight through. - SpatialCanvasInner/ViewerSection: drop the hook, the onInteractionStateChange prop, and the deckProps memo that only carried it. Removes the duplicated gate logic across the two components — the source of the earlier "second gate got missed" hazard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both SpatialCanvas surfaces (headless SpatialCanvasViewer and full-UI SpatialCanvasInner) carried their own copy of the hover-tooltip logic — the hoverTooltip state, the pick→tooltip resolution, and the document.body portal. That copy is now a single hook; each component keeps only its own thin handleHover (the drag guard, plus the viewer's extra onFeatureHover/onShapeHover pick-event callbacks). The hook takes already-derived `enabled`/`aggregate` booleans (not HoverTooltipMode) and expresses the render-tooltip union structurally, so it imports no types from SpatialCanvasViewer — no import cycle. A single DEFAULT_HOVER_TOOLTIP_MODE constant now backs both entry points' default so they can't drift. New useHoverFeatureTooltip.spec covers the hook's wiring (resolve → portal, the enabled / renderTooltip===false / clear / unpicked gates); the pick→tooltip core stays covered by featureTooltipHover.spec. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Update the two unreleased canvas changesets so the combined release notes match what ships (they otherwise described intermediate behaviour that was reversed within the same cycle): - spatialcanvas-picking: default hover tooltip mode is now 'aggregate' (was 'simple'); picking stays live through pan/zoom (the interaction gate was removed — the FlatPolygonLayer pick pass is cheap); note the shared useHoverFeatureTooltip hook. - shapes-nonblocking: add the per-feature colour-buffer thrash fix (two shapes layers sharing the default runtime rebuilt each other's colour buffer every frame; the cache is now keyed per layer). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Biome's organize-exports flags the export order: `shapesPolygonTessellate.js` must follow `shapesLoader.js` (L < P). Barrel reorder only, no behaviour change; keeps the `biome-lint` gate green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3be6202 to
594a327
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/engine/ShapesResolver.ts (1)
139-164: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTrack attempted resource requests to prevent retry loops on failure.
Please track the attempted request keys independently of success (for example, by adding
attemptedTooltipSignatureandattemptedFillColorColumnto theShapesEntrystate) so that repeated reconcile calls don't create a tight retry loop.Based on learnings, in Resource Resolver implementations, review
plan()behavior to ensure failed optional resource requests (such as tooltip or fillColor-related tasks) do not get re-added on everyplan()call just because success-only state updates (e.g.,tooltipSignature/fillColorColumnupdates) haven’t occurred. Instead, track attempted request keys independently of success.🤖 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/src/engine/ShapesResolver.ts` around lines 139 - 164, Update the ShapesEntry state and the resource planning logic around tooltipSignature and fillColorColumn so attempted request keys are recorded independently of successful resource completion. In plan(), suppress re-adding a tooltip or fillColor task when its requested signature or column matches the corresponding attempted state, while preserving retries for new request keys; ensure attempted state is updated when tasks are scheduled, not only after success.Source: Learnings
🧹 Nitpick comments (3)
packages/vis/tests/useHoverFeatureTooltip.spec.tsx (1)
20-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the scattered double assertions with typed test fixtures.
Use
createRef<DeckGLRef>()for the ref and define a narrow picking fixture contract matching the fields this flow consumes. If Deck’s externally-createdPickingInforequires one assertion, isolate it in a documented fixture factory rather than repeatingas unknown as.As per coding guidelines, avoid type assertions when a narrower API contract or local helper can express the runtime shape.
Also applies to: 93-95
🤖 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/useHoverFeatureTooltip.spec.tsx` around lines 20 - 30, Replace the double-cast deckRef fixture with a typed createRef<DeckGLRef>() ref. Define a narrow local picking fixture contract for the fields consumed by the hover flow and use it for PICK; if conversion to PickingInfo still requires an assertion, isolate that single assertion in a documented fixture factory instead of casting the fixture inline.Source: Coding guidelines
packages/core/src/shapesPolygonTessellate.ts (1)
26-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the earcut interop cast.
packages/core/src/earcut.d.tsalready declares a default export, soearcutModulecan be used directly here; theas unknown as { default?: ... }fallback is unnecessary.🤖 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/src/shapesPolygonTessellate.ts` around lines 26 - 28, Remove the interop fallback and cast from the earcut initialization in shapesPolygonTessellate, using the imported earcutModule directly. Preserve the existing earcut symbol and surrounding tessellation behavior.Source: Coding guidelines
packages/layers/src/FlatPolygonLayer.ts (1)
68-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftModel the new flat-polygon contracts directly instead of bypassing TypeScript.
packages/layers/src/FlatPolygonLayer.ts#L68-L82: type the deck.gl subclass, constructor, props, and shader result.packages/layers/src/shapesLayer.ts#L1002-L1034: remove constructor/result double assertions once the layer is typed.packages/layers/tests/shapesLayer.spec.ts#L506-L510: use a typed layer-result guard and typed props.packages/vis/tests/shapesProjection.spec.ts#L20-L23: construct a valid fixture withsatisfiesor a fixture builder.As per coding guidelines, avoid type assertions when narrower API contracts, local guards, or
satisfiescan express the runtime behavior.🤖 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/FlatPolygonLayer.ts` around lines 68 - 82, Type FlatPolygonLayer with the appropriate deck.gl Layer generics, explicit constructor props, and shader result instead of widening through any; retain the existing layer behavior. In packages/layers/src/shapesLayer.ts:1002-1034, remove constructor and result double assertions now made unnecessary by the typed layer. In packages/layers/tests/shapesLayer.spec.ts:506-510, use a typed layer-result guard and typed props. In packages/vis/tests/shapesProjection.spec.ts:20-23, build a valid fixture with satisfies or an existing fixture builder rather than 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/core/src/shapes.ts`:
- Around line 37-46: Refactor ShapesRenderData into a discriminated union so the
flat-polygons variant requires geometryKind: 'polygon' and a defined
polygonBinary payload. Preserve the existing fields and support other geometry
kinds through separate union variants without allowing polygonBinary to be
undefined for flat-polygons.
In `@packages/core/src/shapesGeometryDecode.ts`:
- Around line 83-95: The exteriorRing function misidentifies MultiPolygon
nesting and returns the polygon’s rings array instead of its first ring. Update
the nested-array branch to descend through first to first[0], using a local type
guard to validate the ring shape rather than asserting the coordinate tree;
preserve Polygon handling and add a round-trip MultiPolygon test covering valid
decoded coordinates.
In `@packages/layers/src/FlatPolygonLayer.ts`:
- Around line 100-119: Update updateState to invalidate geometry textures when
any dependency changes: ringPositions, triangleData, counts, featureScale, or
featureCount. Also trigger _updateFeatureTexture() when either featureColors or
featureCount changes, preserving the existing texture-update methods and prop
comparisons.
- Around line 122-129: Update FlatPolygonLayer.finalizeState to destroy the
custom Model along with the existing textures, using the model state symbol
created by the layer and preserving the superclass finalization call.
In `@packages/layers/src/flatPolygonLayerShaders.ts`:
- Around line 123-125: Align the shader constants STROKE_LIGHTEN and
STROKE_ALPHA_LIFT with the corresponding 0.45 lightening and 55 / 255 alpha-lift
values used by deriveStrokeColor, preserving consistent outline derivation
across binary and object rendering paths.
In `@packages/layers/src/shapesLayer.ts`:
- Around line 963-1013: The binary polygon path in createBinaryPolygonDeckLayers
must preserve or explicitly reject the public stroke settings currently
discarded by FlatPolygonLayer. Update FlatPolygonLayer and its construction to
honor defaultStrokeColor, defaultStrokeWidth, stroke units, and minimum width
alongside the existing maximum width; if any cannot be supported, validate and
reject those configurations instead of silently ignoring them.
- Around line 470-505: Replace the module-level strong Map used by
getFeatureColors with lifecycle-safe caching: use WeakMap nesting keyed by the
featureIds, runtime, and defaultColor object identities, or otherwise evict
entries when a layer is removed. Preserve cache reuse for matching identities
while ensuring discarded layer IDs cannot retain Uint8Array buffers, feature
IDs, or runtimes indefinitely.
- Around line 440-451: Update getTessellation and tessellationCache so cached
tessellation results are keyed by both the positions and startIndices buffers.
Preserve memoization for identical buffer pairs while ensuring different
startIndices with the same positions produce and retain separate
TessellatedPolygons results.
In `@packages/vis/src/SpatialCanvas/SpatialCanvasViewer.tsx`:
- Around line 270-275: Replace autoFitLoadingObservedRef-based readiness in the
auto-fit effect with the authoritative pending/settled state returned by
useLayerData, scoped to the current spatial-data and layer inputs. Ensure the
fallback initializes the view when bounds are absent and the current inputs are
settled, including when loading was never observed, while keeping the view
pending during active loading and resetting correctly for new input cycles.
In `@packages/vis/src/SpatialCanvas/useHoverFeatureTooltip.tsx`:
- Around line 83-152: Update useHoverFeatureTooltip so hoverTooltip is cleared
whenever tooltip resolution is disabled or its resolution inputs are
reconfigured, including enabled, renderTooltip, aggregate, getFeatureTooltip,
hoverPickLayerIds, and deckRef. Add the invalidation near the hook’s
state/effect logic while preserving normal hover resolution, and add a
regression test covering an off→on transition without another hover event.
In `@packages/vis/tests/shapesProjection.spec.ts`:
- Around line 38-61: Update both test cache Map declarations in the shape
projection tests to include the required fillColorEntry field in their cached
value type, matching the cache entry contract expected by
getStableShapeFeatureStateRuntime. Apply the same type replacement to both tests
while preserving their existing signature and runtime fields.
---
Outside diff comments:
In `@packages/core/src/engine/ShapesResolver.ts`:
- Around line 139-164: Update the ShapesEntry state and the resource planning
logic around tooltipSignature and fillColorColumn so attempted request keys are
recorded independently of successful resource completion. In plan(), suppress
re-adding a tooltip or fillColor task when its requested signature or column
matches the corresponding attempted state, while preserving retries for new
request keys; ensure attempted state is updated when tasks are scheduled, not
only after success.
---
Nitpick comments:
In `@packages/core/src/shapesPolygonTessellate.ts`:
- Around line 26-28: Remove the interop fallback and cast from the earcut
initialization in shapesPolygonTessellate, using the imported earcutModule
directly. Preserve the existing earcut symbol and surrounding tessellation
behavior.
In `@packages/layers/src/FlatPolygonLayer.ts`:
- Around line 68-82: Type FlatPolygonLayer with the appropriate deck.gl Layer
generics, explicit constructor props, and shader result instead of widening
through any; retain the existing layer behavior. In
packages/layers/src/shapesLayer.ts:1002-1034, remove constructor and result
double assertions now made unnecessary by the typed layer. In
packages/layers/tests/shapesLayer.spec.ts:506-510, use a typed layer-result
guard and typed props. In packages/vis/tests/shapesProjection.spec.ts:20-23,
build a valid fixture with satisfies or an existing fixture builder rather than
assertions.
In `@packages/vis/tests/useHoverFeatureTooltip.spec.tsx`:
- Around line 20-30: Replace the double-cast deckRef fixture with a typed
createRef<DeckGLRef>() ref. Define a narrow local picking fixture contract for
the fields consumed by the hover flow and use it for PICK; if conversion to
PickingInfo still requires an assertion, isolate that single assertion in a
documented fixture factory instead of casting the fixture inline.
🪄 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: dc41fbe5-a71c-4a59-a0fb-5e750e9c8b6f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.changeset/shapes-nonblocking-flat-polygon-layer.md.changeset/spatialcanvas-picking-perf-and-rules-of-react.mddocs/docs/vis/layer-prop-flow.mdxdocs/plans/shapes-nonblocking-tiled-loading.mdpackages/core/package.jsonpackages/core/src/earcut.d.tspackages/core/src/engine/ShapesResolver.tspackages/core/src/index.tspackages/core/src/models/VShapesSource.tspackages/core/src/models/index.tspackages/core/src/shapes.tspackages/core/src/shapesGeometryDecode.tspackages/core/src/shapesLoader.tspackages/core/src/shapesPolygonTessellate.tspackages/core/src/spatialViewFit.tspackages/core/src/workers/index.tspackages/core/src/workers/points-worker.tspackages/core/src/workers/pointsWorkerClient.tspackages/core/src/workers/pointsWorkerProtocol.tspackages/core/tests/shapesGeometryDecode.spec.tspackages/core/tests/shapesLoader.spec.tspackages/core/tests/shapesPolygonTessellate.spec.tspackages/core/tests/shapesResolver.spec.tspackages/core/tests/vshapes.spec.tspackages/layers/package.jsonpackages/layers/src/FlatPolygonLayer.tspackages/layers/src/flatPolygonLayerShaders.tspackages/layers/src/index.tspackages/layers/src/shapesLayer.tspackages/layers/tests/shapesLayer.spec.tspackages/vis/src/SpatialCanvas/SpatialCanvasViewer.tsxpackages/vis/src/SpatialCanvas/index.tsxpackages/vis/src/SpatialCanvas/renderers/shapesRenderer.tspackages/vis/src/SpatialCanvas/shapesProjection.tspackages/vis/src/SpatialCanvas/useHoverFeatureTooltip.tsxpackages/vis/src/SpatialCanvas/useLayerData.tspackages/vis/src/SpatialCanvas/useViewInteractionGate.tspackages/vis/tests/shapesProjection.spec.tspackages/vis/tests/useHoverFeatureTooltip.spec.tsx
💤 Files with no reviewable changes (2)
- packages/vis/src/SpatialCanvas/useViewInteractionGate.ts
- packages/core/src/models/index.ts
| export interface ShapesRenderData { | ||
| kind: ShapesGeometryRepresentationKind; | ||
| /** Geometry semantics within the representation (e.g. Xenium cell_circles). */ | ||
| geometryKind: ShapesGeometryKind; | ||
| elementKey: string; | ||
| featureIds: string[]; | ||
| /** Legacy nested polygons (main-thread decode / compat). Prefer `polygonBinary`. */ | ||
| polygons?: ShapePolygon[]; | ||
| /** Transferable flat polygon geometry. Set by the off-thread decode path. */ | ||
| polygonBinary?: FlatPolygonGeometry; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make flat-polygons require its binary payload.
The interface permits { kind: 'flat-polygons', polygonBinary: undefined } and non-polygon geometry kinds, although consumers rely on polygonBinary to select this path. Model ShapesRenderData as a discriminated union requiring geometryKind: 'polygon' and polygonBinary for this variant.
As per coding guidelines, “Prefer types that match runtime behavior” and use discriminated unions instead of permitting impossible states.
🤖 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/src/shapes.ts` around lines 37 - 46, Refactor ShapesRenderData
into a discriminated union so the flat-polygons variant requires geometryKind:
'polygon' and a defined polygonBinary payload. Preserve the existing fields and
support other geometry kinds through separate union variants without allowing
polygonBinary to be undefined for flat-polygons.
Source: Coding guidelines
| #define STROKE_LIGHTEN 0.55 | ||
| #define STROKE_ALPHA_LIFT 0.35 | ||
| #define STROKE_MAX_FRACTION 0.28 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep binary and object-path outline derivation consistent.
The shader uses 0.55 lightening and 0.35 alpha lift, while deriveStrokeColor uses 0.45 and 55 / 255. This contradicts the claimed rendering parity.
Proposed fix
-#define STROKE_LIGHTEN 0.55
-#define STROKE_ALPHA_LIFT 0.35
+#define STROKE_LIGHTEN 0.45
+#define STROKE_ALPHA_LIFT 0.21568627📝 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.
| #define STROKE_LIGHTEN 0.55 | |
| #define STROKE_ALPHA_LIFT 0.35 | |
| #define STROKE_MAX_FRACTION 0.28 | |
| `#define` STROKE_LIGHTEN 0.45 | |
| `#define` STROKE_ALPHA_LIFT 0.21568627 | |
| `#define` STROKE_MAX_FRACTION 0.28 |
🤖 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/flatPolygonLayerShaders.ts` around lines 123 - 125, Align
the shader constants STROKE_LIGHTEN and STROKE_ALPHA_LIFT with the corresponding
0.45 lightening and 55 / 255 alpha-lift values used by deriveStrokeColor,
preserving consistent outline derivation across binary and object rendering
paths.
| function createBinaryPolygonDeckLayers( | ||
| binary: ShapesBinaryPolygonData, | ||
| sublayer: SpatialShapesRuntimeSublayer, | ||
| options: CreateShapesDeckLayerOptions | ||
| ): Layer[] { | ||
| const featureState = normalizeShapeFeatureState(sublayer.featureState); | ||
| const defaultFillColor = sublayer.defaultFillColor ?? DEFAULT_SHAPE_FILL_COLOR; | ||
|
|
||
| const { positions, startIndices, featureIds } = binary; | ||
|
|
||
| // Prefer the topology tessellated off the main thread by the geometry worker; fall | ||
| // back to lazy main-thread tessellation only when the worker path didn't provide it. | ||
| const tess = binary.tessellation ?? getTessellation(positions, startIndices); | ||
| if (tess.triangleCount === 0) { | ||
| return []; | ||
| } | ||
|
|
||
| const fillColorAt = (index: number): [number, number, number, number] => { | ||
| const featureId = featureIds[index]; | ||
| if (!featureId || featureState.hiddenFeatureIds.has(featureId)) { | ||
| return TRANSPARENT_RGBA; | ||
| } | ||
| return resolveFeatureColor( | ||
| featureId, | ||
| featureState.fillColorByFeatureId, | ||
| EMPTY_SHAPE_FEATURE_STATE_RUNTIME.fillColorByFeatureId, | ||
| defaultFillColor, | ||
| featureState | ||
| ); | ||
| }; | ||
|
|
||
| const featureColors = getFeatureColors( | ||
| options.id, | ||
| featureState, | ||
| featureIds, | ||
| defaultFillColor, | ||
| fillColorAt | ||
| ); | ||
|
|
||
| const layer = new FlatPolygonLayer({ | ||
| id: options.id, | ||
| ringPositions: tess.ringPositions, | ||
| ringVertexCount: tess.ringVertexCount, | ||
| triangleData: tess.triangleData, | ||
| triangleCount: tess.triangleCount, | ||
| featureScale: tess.featureScale, | ||
| featureColors, | ||
| featureCount: featureIds.length, | ||
| strokeWidthPixels: | ||
| sublayer.defaultStrokeWidthMaxPixels ?? DEFAULT_SHAPE_STROKE_WIDTH_MAX_PIXELS, | ||
| opacity: options.opacity ?? 1, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve the public stroke configuration on the binary path.
This path ignores defaultStrokeColor, defaultStrokeWidth, units, and minimum width, and uses only defaultStrokeWidthMaxPixels. The same layer configuration therefore renders differently after switching to flat geometry. Either implement these controls in FlatPolygonLayer or explicitly reject unsupported settings rather than silently discarding them.
🤖 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/shapesLayer.ts` around lines 963 - 1013, The binary
polygon path in createBinaryPolygonDeckLayers must preserve or explicitly reject
the public stroke settings currently discarded by FlatPolygonLayer. Update
FlatPolygonLayer and its construction to honor defaultStrokeColor,
defaultStrokeWidth, stroke units, and minimum width alongside the existing
maximum width; if any cannot be supported, validate and reject those
configurations instead of silently ignoring them.
| // Latches once a visible layer has been observed loading. It lets the auto-fit | ||
| // distinguish "no world bounds *yet*" (data still arriving — stay armed) from "no | ||
| // world bounds *ever*" (everything settled without any — default the view). See | ||
| // the effect below. | ||
| const autoFitLoadingObservedRef = useRef(false); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use an input-scoped readiness state for the auto-fit fallback.
If bounds are absent and isLoading is already false before this effect observes a loading render, Line 304 never initializes the view. Conversely, this ref remains true across subsequent spatial-data/layer cycles and can default a new cycle prematurely.
Expose an authoritative pending/settled state from useLayerData, scoped to the current inputs, rather than inferring readiness from a persistent “loading was once seen” boolean.
Also applies to: 291-316
🤖 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/SpatialCanvasViewer.tsx` around lines 270 -
275, Replace autoFitLoadingObservedRef-based readiness in the auto-fit effect
with the authoritative pending/settled state returned by useLayerData, scoped to
the current spatial-data and layer inputs. Ensure the fallback initializes the
view when bounds are absent and the current inputs are settled, including when
loading was never observed, while keeping the view pending during active loading
and resetting correctly for new input cycles.
…ltip state Correctness: - shapesGeometryDecode: `exteriorRing` returned the first sub-polygon's RINGS array for a MultiPolygon (both Polygon and MultiPolygon match the nested-array branch), so the fill applied Number() to coordinate arrays and emitted NaN. Disambiguate on one more level of nesting and descend for MultiPolygon. Adds a round-trip MultiPolygon test (verified red without the fix). - FlatPolygonLayer.finalizeState: destroy the Model, not just the textures — layer removal / dataset switches leaked its GPU resources. - FlatPolygonLayer.updateState: invalidate on every prop the texture builders read (ringVertexCount, triangleCount, featureScale, featureCount), since the counts size the textures; a count change with a reused buffer left stale dimensions. - getTessellation: key on BOTH positions and startIndices — topology depends on the offsets too, so a reused coordinate buffer could serve the wrong triangles and picking indices. - useHoverFeatureTooltip: disabling only hid the portal while retaining the resolved tooltip, so switching the mode back on resurrected a stale tooltip at its old position without a fresh hover. Drop the stored result on deactivation. Adds an off→on regression test (verified red without the fix). Memory: - getFeatureColors: replace the module-level strong Map keyed by layer id (which pinned a million-element Uint8Array per discarded id forever) with nested WeakMaps keyed on the runtime, feature-id array, and default colour — the three identities that actually determine the buffer's contents. Leak-free, and two layers with matching keys correctly share one buffer. Tests/docs: - shapesProjection.spec: cache type now includes `fillColorEntry` (the package tsconfig only includes `src`, so this latent type error was never checked). - flatPolygonLayerShaders: the outline comment claimed parity with the object path's deriveStrokeColor; the constants deliberately differ (tuned against the anti-aliased, width-capped result). Corrected the comment rather than the values. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @coderabbitai — worked through all 11. Verified each against the current code as instructed; 8 fixed, 3 declined/deferred with reasons below. All in Two of these were real bugs with real failure modes, and I confirmed both new tests go red without the fix: Fixed1. MultiPolygon → NaN ( 2. 3. 4. 5. 6. Stale tooltip on off→on ( 7. 8. Stroke constants — fixed the comment, not the values. You're right they diverge (shader Declined / deferred9. 10. Binary path drops 11. Auto-fit readiness ref. Real design smell, but the fix you describe means exposing authoritative per-input pending/settled state from Nitpick — earcut interop cast: keeping it. The Nitpick — test fixtures: took the Outside-diff — Verification: core 258, layers 135, vis 99 tests pass; |
|
Tip For best results, initiate chat on the files or code changes.
The eight fixes—including the two red-before/green-after regressions—address the reported issues well. The nested The three deferred items are reasonable to keep out of this PR given their API/design scope. I also agree with retaining the earcut runtime interop fallback and with the scoped 🐇 ✅ ✏️ Learnings added
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/layers/src/shapesLayer.ts (1)
473-520: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unnecessary key casts.
normalizeShapeFeatureStatealready returns a stable runtime for the same input object (or the shared empty singleton), so the cache key stays stable;featureIdsanddefaultColorcan be used directly asWeakMapkeys here.♻️ Proposed simplification
- const idsKey = featureIds as unknown as object; - const colorKey = defaultColor as unknown as object; + const idsKey: object = featureIds; + const colorKey: object = defaultColor;🤖 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/shapesLayer.ts` around lines 473 - 520, Update getFeatureColors to use featureIds and defaultColor directly as WeakMap keys, removing the idsKey and colorKey casts and using the original parameters for cache lookups and writes. Preserve the existing cache structure and behavior.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 `@packages/layers/src/shapesLayer.ts`:
- Around line 473-520: Update getFeatureColors to use featureIds and
defaultColor directly as WeakMap keys, removing the idsKey and colorKey casts
and using the original parameters for cache lookups and writes. Preserve the
existing cache structure and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 265e83fb-bd88-4240-b4f7-635e10829186
📒 Files selected for processing (8)
packages/core/src/shapesGeometryDecode.tspackages/core/tests/shapesGeometryDecode.spec.tspackages/layers/src/FlatPolygonLayer.tspackages/layers/src/flatPolygonLayerShaders.tspackages/layers/src/shapesLayer.tspackages/vis/src/SpatialCanvas/useHoverFeatureTooltip.tsxpackages/vis/tests/shapesProjection.spec.tspackages/vis/tests/useHoverFeatureTooltip.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/vis/tests/shapesProjection.spec.ts
- packages/vis/tests/useHoverFeatureTooltip.spec.tsx
- packages/vis/src/SpatialCanvas/useHoverFeatureTooltip.tsx
- packages/layers/src/flatPolygonLayerShaders.ts
- packages/core/src/shapesGeometryDecode.ts
- packages/layers/src/FlatPolygonLayer.ts
The off→on stale-tooltip fix used setState inside useEffect, which react-hooks/set-state-in-effect rejects (eslint-plugin-react-hooks v7 recommended-latest) — it failed the react-lint CI job. Use React's "adjusting state when a prop changes" pattern instead: compare the previous activation against the current one during render and reset there. Same behaviour, and it re-renders immediately with the corrected state rather than painting the stale tooltip for a frame first. pnpm lint:react and pnpm lint:biome both exit 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Makes SpatialData shapes non-blocking and performant at Visium HD scale (~2.7M polygons), replaces the deck
SolidPolygonLayer+PathLayerrender with a hand-rolled vertex-pulling layer, and cleans up the two SpatialCanvas surfaces.Highlights
Non-blocking, off-thread shapes (
@spatialdata/core,@spatialdata/layers)ShapesResolver.blockingResourcesis now[](a main-thread fallback covers the no-worker path), so shapes no longer gate first paint.square_002um(~2.7M polygons) loads without OOM.Vertex-pulling
FlatPolygonLayer(@spatialdata/layers)gl_VertexID, and imputes an anti-aliased outline withfwidthin the fragment shader (no separate outline layer). Per-frame cost ≈ the fill; geometry memory ≈ the stock indexed fill. Works on arbitrary polygons (cell segmentation, not just grids).Canvas picking + tooltips (
@spatialdata/vis)SolidPolygonLayer+PathLayerpick draw; the FlatPolygonLayer pick pass is a single cheap draw). Verified smooth onsquare_002umwith two shapes layers.'aggregate'.SpatialCanvasViewerheadless + full-UISpatialCanvasInner) are deduped: the interaction-gate machinery is deleted, and the shared hover-tooltip logic (pick → tooltip → portal) is a singleuseHoverFeatureTooltiphook. Public API of the headless viewer is unchanged.Bug fixes
updateTriggerarrays.FlatPolygonLayercolour cache is now keyed per layer.Tests
@spatialdata/core,@spatialdata/layers(135),@spatialdata/vis(98, incl. newuseHoverFeatureTooltiphook +shapesPolygonTessellatetopology/boundary tests).tscclean;biome ciclean on all touchedsrc.Known follow-ups (documented in the changeset)
CI note
biome-lintdepends on #87 (a pre-existing, unrelatedbiome-ignoreerror inSpatialEntryStore.tsonmain). Once #87 merges, mergingmaininto this branch makes the gate green.🤖 Generated with Claude Code
Summary by CodeRabbit