Consume Resource Resolvers in useLayerData (ADR 0004 Step 1) - #86
Merged
Conversation
Make useLayerData *consume* the resolvers #85 landed unconsumed, per docs/plans/step1-consumption-tactics.md. The 17-member public surface is unchanged; useLayerData.spec.tsx stays green throughout. Net -724/+394 lines. - Inc 1 (shapes): ShapesResolver drives geometry/tooltip/fill-colour-row loads; vis-side projection memos handle the tooltip->geometry patch (coupling #1) and keep prebuilt/fill-colour lazy. Fill-colour entry is withheld until rows load so the feature-state runtime rebuilds and fill colours actually appear. - Inc 2 (images + labels): ImagesResolver/LabelsResolver consumed via getLoadedData; LabelsLoaderData retyped to LabelsChannelDefaults (tooltip is now a separate resource). Physical-size world-bounds compute kept in the hook (coupling #2). - Inc 3 (store): one SpatialEntryStore + one reconcile() commit-effect replace the per-kind driving effects (hook now has two useEffects total). Points is wrapped in a non-owning proxy so the stable PointsDataEngine the panels subscribe to survives a store rebuild on dataset swap; points row-codes/matching stay on the render-phase engine calls in getLayers (Track A). Verified: vis typecheck + build clean, full suite 549 passing, Biome gate clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The store is designed to own its resolvers (subscribe + dispose), but points is owned by the stable PointsDataEngine the panels subscribe to — so the store borrows it through createNonOwningResolver (no-op dispose) to avoid clearing the engine's cache on a dataset-swap rebuild. Expand the rationale where it goes against the store's ownership grain: the two-owner problem, why the no-op is correct not just safe, why it doesn't reintroduce "points is special" in the store, alternatives rejected, and the exit condition. Add the ownership model at the construction site and note the StrictMode useMemo-subscribe caveat. Add a lifecycle test: a spatialData swap rebuilds the raster resolvers and the store, and the points cache/render-resource identity must survive it — the test that fails if the proxy ever regresses to a real dispose. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- resource-resolver-handoff.md: flip Status off "ready for implementation", add a Progress section and mark Step 0/1 landed (contracts, four resolvers, useLayerData consumption via SpatialEntryStore.reconcile); note the non-owning points proxy and the deferred render-phase points calls (Track A). - spatial-canvas-status.mdx: replace the stale "minimal ScatterplotLayer" points description with the PointsDataEngine reality; retitle useLayerData; add a Resource Resolver entry to "Recently landed"; refresh the feature/table roadmap item (tooltip/pick routing done, ping-pong remains). - Add a changeset for the vis-side resolver consumption. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Intermediate decomposition pass, no behaviour change. Moves two cohesive, kind-local chunks out of the 1.6kloc hook: - resolvers/nonOwningResolver.ts — `createNonOwningResolver` + its lifecycle rationale (the store-ownership exception for points). - shapesProjection.ts — the shapes feature-state / fill-colour projection helpers and their cache-entry types (`ShapePrebuiltEntry`, `ShapeFillColorEntry`, `getStableShapeFeatureStateRuntime`, signature/serialise helpers). This is the `project()` half of ADR 0004 §4; a vis-local waypoint before Step 3 relocates it into @spatialdata/layers. useLayerData.ts drops from 1623 to 1446 lines and imports both. The shapes read path now lives in a small dedicated module, so Track B / Step 3 touch it rather than the hook. Behaviour-preserving: control-char signature separators kept byte-identical; full suite (550 tests) green, vis typecheck + build clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesResolver consumption and rendering integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant useLayerData
participant SpatialEntryStore
participant ResourceResolvers
participant PointsDataEngine
useLayerData->>SpatialEntryStore: reconcile visible layer contexts
SpatialEntryStore->>ResourceResolvers: plan and load layer resources
ResourceResolvers->>PointsDataEngine: access points through non-owning resolver
SpatialEntryStore-->>useLayerData: notify cache updates
useLayerData->>ResourceResolvers: read render and tooltip metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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
🤖 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/vis/src/SpatialCanvas/shapesProjection.ts`:
- Around line 71-100: Preserve explicit strokeColorByFeatureId values in
mergeShapeFeatureStateForRender when fillColorByColumn is active instead of
unconditionally replacing them with the fill map. If outlines should mirror fill
colors only when no stroke override exists, apply that fallback and add a brief
comment; update getShapeFeatureStateSignature to stop hashing
strokeColorByFeatureId in that mirroring case.
In `@packages/vis/src/SpatialCanvas/useLayerData.ts`:
- Around line 533-590: The reconciliation effect does not rerun when element
resolution or eviction changes elementMap. Update reloadElement to trigger
store.reconcile with the current visible layer contexts, or introduce an
equivalent elementMap-driven trigger, ensuring shapes, images, and labels are
replanned after reloads and previously missing elements become available.
🪄 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: ddecf903-fef6-4969-b1d6-ef27c413f624
📒 Files selected for processing (9)
.changeset/consume-resource-resolvers.md.vscode/settings.jsondocs/docs/vis/spatial-canvas-status.mdxdocs/plans/resource-resolver-handoff.mddocs/plans/step1-consumption-tactics.mdpackages/vis/src/SpatialCanvas/resolvers/nonOwningResolver.tspackages/vis/src/SpatialCanvas/shapesProjection.tspackages/vis/src/SpatialCanvas/useLayerData.tspackages/vis/tests/useLayerData.spec.tsx
…hange Two code-review findings, verified against current code: - shapesProjection: mergeShapeFeatureStateForRender no longer clobbers an explicit per-feature strokeColorByFeatureId when a fill-by-column encoding is active. It now mirrors the fill map only when the caller has NOT set an explicit stroke override (the schema allows both together, e.g. via SpatialLayerProps). Signature unchanged and deliberately so: it already hashes the explicit stroke, which is what drives the render; dropping that term would stale the runtime when the stroke changes, and in the mirroring case the term is already empty. - useLayerData: the reconcile effect now depends on elementMapValue, so it replans when element resolution changes without layers/store changing — e.g. a coordinate system switch that makes a previously unavailable element resolvable. The map is memoised on availableElements, so no per-render churn. Skipped: wiring reconcile into reloadElement (the finding's other suggestion) — reloadElement has zero runtime callers (dead surface, per the Step 3 punchlist), so it would fix nothing observable. Verified: vis typecheck + build clean, Biome gate clean, full suite 550 passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Merged
This was referenced Aug 5, 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.
Completes Step 1 of the Resource Resolver work (ADR 0004):
useLayerDatanowconsumes the per-kind resolvers that #85 landed unconsumed, replacing the ~400-line
Promise.allkind-switch with a singleSpatialEntryStore.reconcile()loop. This isthe "fork point" the handoff calls out — after it lands, Track A (points) and Track B
(shapes) touch mostly different files.
Executed per
docs/plans/step1-consumption-tactics.md. The 17-member public surfaceis unchanged and guarded by
useLayerData.spec.tsxthroughout.What changed
Increment 1 — shapes.
ShapesResolverdrives geometry/tooltip/fill-colour-rowloads. Vis-side projection memos handle the two couplings the plan flags: the
tooltip→geometry
rowIndexByFeatureIndexpatch (identity-stable so deck doesn't teardown per frame), and keeping the physical-size world-bounds compute in the hook.
Increment 2 — images + labels.
ImagesResolver/LabelsResolverconsumed viagetLoadedData.LabelsLoaderDataretyped to the resolver'sLabelsChannelDefaults(tooltip is now a separate resolver resource).
ImageLoaderData(public API)unchanged.
Increment 3 — fold points + the store. One
SpatialEntryStore+ onereconcile()commit-effect replace the per-kind driving effects (the hook now has two
useEffectstotal). Points stays owned by the stable
PointsDataEnginethe panels subscribe to;the store borrows it through a non-owning proxy (
createNonOwningResolver, no-opdispose) so a dataset-swap rebuild of the store doesn't clear the engine's cache.Docs + decomposition. Expanded the lifecycle rationale for the proxy; refreshed
the resolver handoff and the public SpatialCanvas status page (both had stale status
claims); added a changeset. Finally, an intermediate extraction moved the proxy and
the shapes projection helpers into
resolvers/nonOwningResolver.tsandshapesProjection.ts, dropping the hook from 1873 → 1446 lines.Reviewer notes
ownership model — worth the closest look. Rationale (two-owner problem, why the
no-op is correct not just safe, alternatives rejected, exit condition) is documented
in
resolvers/nonOwningResolver.ts, and guarded by a test that fails if it everregresses to a real
dispose(points cache must survive aspatialDataswap).pointsEngine.ensureMatchingFeaturesLoaded/
ensureRowFeatureCodescalls ingetLayersstay put (they migrate intoplan()under Track A), so the points reconcile context carries only the memory cap.
must not be created until rows load, or the feature-state runtime never rebuilds and
fill colours never appear. Fixed; noted in the plan doc.
identity, and the surface — but not shape fill-by-column, image/labels rendering, or
tooltips. Those were verified by reading. Worth adding before Track A/B build on this.
c6234e3(disable the tsgo VS Code extension) is an incidental workspace-configcommit picked up on this branch, unrelated to the resolver work.
Verification
tscclean, Biome CI gate (packages/*/src) clean, full suitepnpm -r --filter='!docs' test= 550 passing,
pnpm --filter @spatialdata/vis buildsucceeds. (Bothcoreandlayersneed a local build first — vis typechecks against theirdist.)🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Documentation
Tests
Chores