Resource Resolver foundation: contracts + four resolvers (unconsumed) - #85
Merged
Merged
Conversation
ADR 0004 §6 claimed `createImageLoader` closes over the React `VivLoaderRegistry` context, making the image loader "the one genuine ports-and-adapters dependency". It closes over nothing: it already takes `fetchMultiscales` as an injected parameter, and the React context is merely the DI container at the call site. There is no closure to break and therefore no port to invent. The claim was assumed, not checked against the code, before it was committed. Three things follow, and together they keep images out of core for now: - `avivatorish` is a deliberate de-vendoring holding pen for code that also lives upstream in Viv and in MDV, and its own README calls the serialized image-state model "still evolving". A port designed against it today would freeze a guess about an unsettled model into the interface `tgpu-htj2k` depends on. - The second consumer does not need it. zarrextra's `VivCompatiblePixelSource` already serves both Viv and `tgpu-htj2k`, so the shared images seam already exists *below* the resolver — as this ADR's own Non-goals section says. Images is the one kind of the four where the duplication argument, the entire reason for this ADR, does not apply. - So an image port in core pays a real cost to solve a problem that is not there. Resolver *placement* is therefore per-kind and driven by dependency, not by dogma. core defines the ResourceResolver interface. Points and Shapes resolvers live in core; Images and Labels implement the same interface but live in vis, next to the Viv/avivatorish dependencies they already have. The store holds only ResourceResolver and must not know which package an implementation came from. Also: - §7 (no runtime dependency in core's public signatures) is marked as a current position resting on tgpu-htj2k's stance — revisable, not a law. Read it as "not now", not "never". - The handoff plan's phase table is corrected: project()/render() belong to the Renderer Adapter in `layers`, per ADR 0004 §4, not to the resolver. This decides where PointsDataEngine's lazy memos land. Docs only; no source changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The shared contracts for the Resource Resolver (ADR 0004 §3). Types plus
two pure functions, in a new packages/core/src/engine/. Zero call sites —
this commit changes no behaviour.
Resolution<T> = idle | loading{partial?, stale?, progress?} | ready{value,
notices?} | failed{error, stale?}. Per-resource, not per-entry: a shapes
entry with a broken tooltip must still draw its geometry.
Two design points worth stating, because both are easy to undo later:
- The identity rule. Resolutions are constructed at mutation time and
returned by reference. Constructing one during project()/render() is a
fresh identity per frame, which is a deck layer teardown per frame — the
pan flash this design exists to avoid. Only idle() is a frozen singleton.
- No valueOf() collapsing lastGood ?? partial. The points path draws
lastGood as the base layer AND partial as an overlay sub-layer, at the
same time. A merged accessor would destroy that distinction.
toSpatialEntryError() classifies from the SEAM, not from the throw. Core's
leaf loaders keep throwing bare Error(string), and anything off the points
worker has already lost its type — that failure channel is
`{ ok: false; error: string }`. So the context carries a `fallback` kind:
the seam knows what it was doing even when the throw doesn't. Three tiers:
instanceof (the only lossless one — core has exactly two typed error
classes), one quarantined message-recogniser for worker-unavailable
(TODO(Track A): delete once the worker protocol has a typed error), then
the fallback. Message-sniffing beyond that is banned and documented as such.
SpatialEntryErrorFallbackKind is deliberately narrower than
SpatialEntryErrorKind: coordinate-system-not-found and
points-preload-too-large carry payload only their typed class can supply,
so naming either as a fallback is a promise the classifier cannot keep. The
type forbids it rather than degrading at runtime.
isCancellation() is the gate before the classifier. Superseding a load is
normal operation, not a domain failure, so there is no `cancelled` case in
the union — and without this check every memory-cap drag would paint an
error where today there is none.
This also gives PointsPreloadTooLargeError a home. It has been exported and
never constructed; it is now one of the two lossless inputs.
30 new tests. Build, full suite (427) and biome all green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ADR 0004 §5, amending ADR 0001. Forced by dependency direction: the Resource Resolver takes a Render Stack as input, and it lives in core. This also lands the schemas where they already wanted to be — both are zod persistence schemas that `vis` re-exported verbatim because MDV consumes them as a *data contract*, and core already owns schemas/ and depends on zod. Mechanical: both files imported nothing but zod. The migration contract from ADR 0004 §5 is honoured — core becomes canonical, and `layers` (and through it `vis`) retains its re-exports as compatibility shims. No consumer import path moves. Removing the shims is a separate, deliberate deprecation coordinated with MDV, and is not part of this work. tests/renderStackShim.spec.ts guards that promise, and guards it by *identity*, not by name: an `export … from` chain still typechecks if someone later re-declares a schema locally, and that drift would only surface downstream in MDV. Thirteen values, asserted `layers[x] === core[x]`. The two existing specs move to core/tests/ with the schemas. Build, full suite (441) and biome green. Both dependency-boundary DoD boxes still hold: core imports no react/deck.gl/viv, layers imports no react. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…aries The net, landed BEFORE any behaviour moves. Nothing here changes source; all three files are written against the current code and must stay green through the resolver refactor. useLayerData.spec.tsx — the first test that actually renders the hook. Nothing did, until now. Two specs import from the module (one takes a type, one takes two module-scope helpers) but the 1,873-line hook itself was never invoked by any test in the repo. Its whole public surface — seventeen members that reach MDV through a `...layerData` spread — was unguarded, and the refactor dissolves six of its seven kind-switch ladders and re-points every member at a resolver snapshot. Asserts the contract, not the internals: the exact 17 keys, idle→ready for shapes and points, world bounds, and render-resource identity across repeated getLayers() calls. pointsResourceIdentity.spec.ts — all three points resources, not one. PointsDataEngine exposes three render resources and memoises all three lazily on read, because today their only caller is render. Deck rebuilds a layer's batch when data identity changes, so a resource rebuilt per call is a teardown per frame — the pan flash. The existing 845-line spec pins exactly one of the three (the resident one). Nothing guarded getMatchingResource or getMatchingPartialResource, so a flash on the selected genes, or a teardown-per-frame on the streaming partial overlay, would have been invisible to every test here. C5 moves these memos into the Renderer Adapter's project(); this is the contract that move must not break. Writing it surfaced a real ordering subtlety: ensureMatchingFeaturesLoaded assigns entry.matchingLoading AFTER constructing the scan's async IIFE, so a loader firing onProgress before its first await has that chunk silently dropped by the currency guard. Unreachable in production (real loaders do I/O first), but the stubs now model the real thing rather than a state the engine never reaches. dependencies.spec.ts — two DoD boxes that said "Still true". A box that says "still true" is a box that rots, and these are the constraints the whole design rests on: core is tgpu-htj2k's dependency root, and its framework-freedom is what makes the resolver testable with no GL context. Now enforced: core imports no react/deck.gl/viv/avivatorish and no sibling package; layers imports no react. Every assertion in it is `toEqual([])`, so a broken scanner would pass vacuously and read as proof — worse than no test. Four canary cases point the detector at imports we know exist (deck.gl in layers, react in vis, core in layers, and specifically via `export … from`) and require it to find them. Adds @testing-library/react to vis devDeps; jsdom was already configured. 486 tests (was 427). Build and biome green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The resolver interface, extracted from the shape PointsDataEngine already has, plus the points implementation and the reconcile loop. All unconsumed: useLayerData still runs the old path, and PointsDataEngine is untouched. The interface pins the phase separation, which is the load-bearing part: plan(ctx) resolver pure, sync commit only no — RETURNS task descriptors load(task) resolver async commit only yes — the only place project() adapter pure, sync end of reconcile render() adapter pure, sync during render — handed NO engine handle project() and render() are deliberately NOT on this interface. They belong to the Renderer Adapter in `layers`, per ADR 0004 §4: identity-stable memoisation is a *deck* requirement, so it belongs on the renderer side. The handoff plan's phase table implied otherwise and has been corrected. PointsResolver is PointsDataEngine's cache/lifecycle half, lifted. The three render-resource memos did NOT come with it — they go to layers in C5. What it exposes instead is their INPUTS by identity: getData / getMatchedBatch / getPartialBatch. Batches here are always replaced, never mutated in place, so identity is an exact invalidation key. That matters more than it looks: pointsRenderResourceSignature keys on row COUNT, not identity, which is exactly why the old engine had to manually null entry.resource on every swap. Keying the adapter's memo on identity removes that bookkeeping entirely. plan() is where the two render-phase `void engine.ensureX(...)` calls go. Both conditions — "does this selection need a scan", "does this filter need row codes" — were always pure functions of config plus entry state. They were being asked in the wrong phase, from inside getLayers(). Here they cannot start work even by accident, and a test asserts plan() touches no element method. Consequently there is no queueMicrotask in the scan path. The old engine's `queueMicrotask(() => this.notify())` existed solely to defend against a synchronous notify during render; nothing kicks a load from render any more, so the defence is unreachable by construction rather than by discipline. ResolveTask.id carries everything the request depends on — the memory cap is IN the preload id, so a cap change supersedes rather than dedups. Step 1 does not act on this: resolvers keep today's in-flight-promise dedup byte for byte. It is the seam Track A's RequestSlot consumes, shaped now so that landing it changes no public type. (Every one of R1/R2/R3/R5 is a keying bug, so the key is where the fix has to go.) SpatialEntryStore holds only ResourceResolver and cannot tell which package an implementation came from — which is what keeps "images is special" from becoming representable once ImagesResolver lands in vis. pointsResolver.spec.ts (20 tests) is the DoD box "the resolver is exercised by a test that constructs no deck layer and no GL context". It imports nothing from layers or vis. The deeper behavioural net stays pointsDataEngine.spec.ts, which must pass UNCHANGED through C5. Preserved verbatim, deliberately, because this commit is a re-housing and not a rewrite: R5 (ensureRowFeatureCodes takes no memory cap, so it loads at the 4M default against a possibly-8M resident batch and misaligns the filter mask). It is documented in place and left for Track A. 506 tests. Build and biome green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The split lands. PointsDataEngine (940 lines) becomes a 228-line facade over PointsResolver in core (the cache and lifecycle, framework-free) and PointsRendererAdapter in layers (the three render-resource memos, 135 lines). ACCEPTANCE: packages/layers/tests/pointsDataEngine.spec.ts passes UNCHANGED, byte for byte — 35 tests, not a character edited. That was the whole point of keeping the facade, and it is the only credible proof that a 940-line class was cut in half without changing what it does. The memos move because ADR 0004 §4 says identity-stable memoisation is a *deck* requirement — deck tears a layer down when its data identity changes — so it belongs on the renderer side. Nothing was deleted: the memo was RESCHEDULED, from "lazily, whenever a getter is first called this frame" to "eagerly, once, at the end of reconcile". The interesting part is the invalidation key. pointsRenderResourceSignature keys on the batch's row COUNT, not its identity — two different batches with the same row count produce the same signature. That is exactly why the old engine had to reach in and manually null entry.resource on every swap and every in-memory shed: the signature alone would happily serve a stale resource. Once the memo lives outside the entry that manual invalidation isn't available, so the adapter keys on the batch OBJECT IDENTITY as well. That is exact, not approximate: PointsResolver always replaces a batch, never mutates one in place, so identity changes precisely when the data changes and never otherwise. The bookkeeping disappears — and with it the class of bug where someone adds a new mutation path and forgets to invalidate. The facade survives because it has to. `pointsEngine` is member 8 of useLayerData's seventeen, and PointsFeatureState.tsx calls ten of these methods directly. Keeping it is what lets the split land without touching the panels, the 845-line spec, or MDV. It goes away in Step 3, once the panels read from project() and nothing needs a live engine handle. pointsResourceIdentity.spec.ts (added in C3, before any of this moved) passes unchanged too — all three resources still identity-stable across repeated reads, including the matched and partial ones that no test covered before. 506 tests. Build, biome and the dependency boundaries all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three resources — geometry, tooltip, fillColor — behind the same interface PointsResolver implements. Unconsumed: useLayerData still runs the old path. All three loaders already lived in core (element.loadRenderData, loadShapesTooltipMetadata, loadAssociatedTableFeatureRows). Only the orchestration was stranded in a React hook. This also absorbs loadShapesData from vis's shapesRenderer — a try/catch that re-threw with a nicer message — and replaces the re-throw with a SpatialEntryError, which is what the classifier exists for. The seam nominates `decode-failed` for geometry and `load-failed` for the table reads, because the seam knows what it was doing and the bare `throw new Error(string)` does not. Failure is PER-RESOURCE, and this is the resolver where that stops being an abstract claim: a shapes entry whose tooltip column is missing must still draw its geometry. An entry-wide Result would blank a layer whose geometry was perfectly fine. There is a test for exactly that. blockingResources = ['geometry']. Tooltip and fill colour have never blocked a first paint — which is why isBlocking already treats shapes differently from points today. That asymmetry was a kind-switch; here it is data. The fill COLOUR is deliberately not here. load() fetches the table rows; buildShapeFillColorByFeatureId stays in layers. That is not a dependency dodge, it is the phase separation doing its job: fetching rows is I/O, and mapping a column through a palette is a pure projection *for a renderer* — it belongs in project(). Same for buildShapesPrebuiltData. So the fillColor resource is the ROWS, not the colours. ResolveContext gains `transform`. World bounds are meaningless in element space, and ADR 0004 §1 gives the resolver entry resolution and bounds — so the element→coordinate-system matrix is the resolver's input, not something a renderer hands down. Bounds are memoised on the render data's identity. Preserved deliberately: the tooltip ping-pong. Tooltip metadata is cached per element but requested per layer config, so two layers over one element with different tooltipFields invalidate each other forever. (shapePrebuiltData and shapeFillColorData were keyed by layer id to avoid exactly this; the tooltip cache was missed.) Step 1 is a re-housing — documented in place, left to Track B, which owns it on the punchlist. Labels have the identical shape. **Track B unblocks here**, not at the end of Step 1: the shapes loader seam and the batch representation can start against this resolver now. 521 tests. Build and biome green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The images and labels resolvers implement core's ResourceResolver — the same interface PointsResolver and ShapesResolver implement — but they live in @spatialdata/vis, next to the Viv and avivatorish dependencies they need. Unconsumed: useLayerData still runs the old path. This is the payoff of the ADR 0004 §6 amendment. There is no RasterSourcePort in core, because there was never a closure to break: createImageLoader already takes fetchMultiscales as an injected parameter, and the React VivLoaderRegistry context is merely the DI container at the call site. The resolver takes the fetcher as a constructor arg; useLayerData will feed it useVivLoaderRegistry()'s value in C8 — exactly the DI that exists today, hoisted out of the load body. The ~180-line channel-defaults ladder moves too, but only in PHASE, not package: it goes from inline in useLayerData's kind-switch to buildImageChannelDefaults / buildLabelsChannelDefaults in vis's existing imageLoaderChannelDefaults.ts, lifted verbatim. The nested try/catch is preserved deliberately — computing contrast stats reads pixels, which can fail on a store that served its metadata fine, and a channel-defaults failure must degrade to a fallback rather than fail the image. That is exactly the healthy-data-with-a-caveat case EntryNotice exists for. World bounds are computed right here with getImageSize. That single line is what killed the port: core owns bounds (ADR 0004 §1) but may not import Viv, so a core-resident images resolver would have needed extents handed across a port. A vis-resident one just asks Viv, and core never learns rasters have a width. The test that matters most asserts the store CANNOT TELL these live in a different package: it registers them through the same SpatialEntryStore registry, typed only as ResourceResolver, and drives them through the same plan/load/snapshot loop. If the store could distinguish a vis-resident resolver from a core-resident one, "images is special" would become representable — and that is how one interface quietly becomes two. avivatorish is untouched. No image-state-model decision is forced; that question stays open and separately decidable, which is where it belongs. 542 tests. Build, biome and the core dependency boundaries all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 4 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 (10)
📝 WalkthroughWalkthroughThis PR introduces a shared resource-resolver architecture across ChangesResource resolver architecture
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
![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: 16
🧹 Nitpick comments (12)
packages/core/src/engine/errors.ts (1)
163-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid type assertions by using the
inoperator.As per coding guidelines, avoid type assertions (
as ...) in TypeScript when a local type guard can express the same fact. You can safely narrow the object using theinoperator instead of asserting its type.♻️ Proposed refactor
export function isCancellation(cause: unknown): boolean { if (cause instanceof DOMException) return cause.name === 'AbortError'; return ( typeof cause === 'object' && cause !== null && - (cause as { name?: unknown }).name === 'AbortError' + 'name' in cause && + cause.name === 'AbortError' ); }🤖 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/errors.ts` around lines 163 - 170, Update isCancellation to remove the type assertion when reading cause.name. After confirming cause is a non-null object, use the in operator to verify the name property exists, then compare that property to 'AbortError' while preserving the existing DOMException handling.Source: Coding guidelines
packages/core/src/engine/resolution.ts (1)
60-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unnecessary type assertions for the
IDLEsingleton.As per coding guidelines, avoid type assertions (
as) when a discriminated union or narrower API contract can express the same fact. The frozen object{ status: 'idle' }natively matches theidlebranch of theResolution<T>union. Dropping the explicit annotations allows TypeScript to resolve the assignment safely without casts.♻️ Proposed refactor
-const IDLE: Resolution<never> = Object.freeze({ status: 'idle' as const }); +const IDLE = Object.freeze({ status: 'idle' as const }); /** * Constructors and guards for {`@link` Resolution}. * * Namespaced deliberately: bare `ready` / `failed` / `loading` are far too * generic for `@spatialdata/core`'s top-level export surface. */ export const Resolution = { /** One frozen singleton, so `idle` is identity-stable across renders. */ - idle: <T>(): Resolution<T> => IDLE as Resolution<T>, + idle: <T>(): Resolution<T> => IDLE,🤖 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/resolution.ts` around lines 60 - 71, Update the IDLE singleton and Resolution.idle constructor to remove the unnecessary `as const` and `as Resolution<T>` assertions, relying on the discriminated union’s inferred idle type while preserving the frozen, identity-stable singleton behavior.Source: Coding guidelines
packages/core/src/renderStack.ts (1)
96-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
flatMapto simplify filtering and mapping.You can replace the
.filter()(with its type guard) and.map()chain with a single.flatMap()call. This avoids iterating the array twice and reduces boilerplate.♻️ Proposed refactor
-export function getRenderStackHostLayerIds(renderStack: RenderStack): string[] { - return renderStack.entries - .filter((entry): entry is RenderStackHostEntry => entry.kind === 'host') - .map((entry) => entry.source.hostLayerId); -} +export function getRenderStackHostLayerIds(renderStack: RenderStack): string[] { + return renderStack.entries.flatMap((entry) => + entry.kind === 'host' ? [entry.source.hostLayerId] : [] + ); +}🤖 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/renderStack.ts` around lines 96 - 100, Update getRenderStackHostLayerIds to use a single flatMap over renderStack.entries, returning each host entry’s source.hostLayerId and excluding non-host entries, while preserving the existing string[] result.packages/core/src/spatialLayerProps.ts (1)
98-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
flatMapto parse and filter valid sublayers.The imperative
for...ofloop is perfectly valid and readable, but replacing it with.flatMap()provides a more concise, functional alternative that extracts and filters valid sublayers in one step.♻️ Proposed refactor
-function migrateV0ToV1(raw: z.infer<typeof spatialLayerPropsV0Schema>): SpatialLayerProps { - const sublayersIn = raw.sublayers ?? []; - const sublayers: SpatialSublayer[] = []; - for (const item of sublayersIn) { - const parsed = spatialSublayerSchema.safeParse(item); - if (parsed.success) { - sublayers.push(parsed.data); - } - } - return spatialLayerPropsSchema.parse({ - schemaVersion: SPATIAL_LAYER_PROPS_SCHEMA_VERSION, - viewMode: raw.viewMode ?? '2d', - globalTimeIndex: raw.globalTimeIndex, - sublayers, - }); -} +function migrateV0ToV1(raw: z.infer<typeof spatialLayerPropsV0Schema>): SpatialLayerProps { + const sublayersIn = raw.sublayers ?? []; + const sublayers = sublayersIn.flatMap((item) => { + const parsed = spatialSublayerSchema.safeParse(item); + return parsed.success ? [parsed.data] : []; + }); + + return spatialLayerPropsSchema.parse({ + schemaVersion: SPATIAL_LAYER_PROPS_SCHEMA_VERSION, + viewMode: raw.viewMode ?? '2d', + globalTimeIndex: raw.globalTimeIndex, + sublayers, + }); +}🤖 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/spatialLayerProps.ts` around lines 98 - 113, Refactor migrateV0ToV1 to derive sublayers with flatMap: safely parse each item using spatialSublayerSchema.safeParse, return the parsed data for successful results and no item for failures, preserving the existing filtering behavior and resulting SpatialSublayer[].packages/vis/tests/useLayerData.spec.tsx (1)
50-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a short comment explaining the
as unknown as Xmock casts.Both
pointsElement/shapesElementmocks bypass the realPointsElement/ShapesElementinterfaces via double assertions with no explanation. Per coding guidelines, when an assertion is unavoidable at an external boundary it should be kept local with a short comment on why the compiler can't prove it (here, only a subset of the interface is mocked).♻️ Example
} as unknown as PointsElement; + // Only the members `useLayerData` actually reads are mocked; a plain `as + // PointsElement` would fail because the rest of the interface is missing. return { key, type: 'points', element, transform: new Matrix4() };Also applies to: 63-84
🤖 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/useLayerData.spec.tsx` around lines 50 - 61, Add brief comments immediately before the double assertions in pointsElement and shapesElement explaining that the test mocks only the subset of the PointsElement/ShapesElement interfaces needed by the test, so the compiler cannot validate the partial mock directly. Keep the casts local and unchanged.Source: Coding guidelines
packages/core/src/engine/SpatialEntryStore.ts (1)
20-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the registry type with its guarded runtime behavior.
Record<SpatialEntryKind, ...>guarantees every resolver exists, making all missing-resolver branches unreachable. Declare this asPartial<Record<...>>if partial registries are supported; otherwise remove the guards.As per coding guidelines, types should match runtime behavior and branches that cannot run under the declared types should be avoided.
Also applies to: 68-70, 94-96, 118-125, 137-138
🤖 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/SpatialEntryStore.ts` around lines 20 - 25, Update ResolverRegistry to use Partial<Record<SpatialEntryKind, ResourceResolver<any, any>>> so its type permits missing resolvers and matches the guarded runtime behavior throughout the related lookup branches. Preserve the existing missing-resolver checks and handling in the affected resolver access paths.Source: Coding guidelines
packages/core/src/engine/ShapesResolver.ts (1)
179-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow shape tasks instead of asserting their resource and payload types.
The
as/as neverassertions hide mismatched payloads and force heterogeneous assignments past the compiler. Use a discriminated shapes-task union or local type guards before switching and assigning.As per coding guidelines, avoid type assertions when a local type guard, discriminated union, or narrower API contract can express the same fact.
Also applies to: 196-203, 228-228
🤖 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 179 - 184, Replace the resource and payload type assertions in the shape-task handling flow around the stale-value assignment and the referenced locations with a discriminated shapes-task union or local type guards. Narrow each task by its resource before reading or assigning entry fields, so geometry, tooltip, and fillColor payloads are type-checked without `as` or `as never`, while preserving the existing loading/stale-value behavior.Source: Coding guidelines
packages/core/tests/shapesResolver.spec.ts (1)
34-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the test fixtures and resolution checks type-safe.
Use
satisfies Pick<ShapesElement, ...>for the mock and narrow resolutions bystatus, as already done at Line 168. If the full element assertion is unavoidable, keep one assertion at the fixture boundary and document why.As per coding guidelines, prefer precise inferred types and narrowing over
as unknown asoras never.Also applies to: 119-121, 151-152, 193-196, 253-256
🤖 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/shapesResolver.spec.ts` around lines 34 - 40, Replace the `as unknown as ShapesElement` assertion in the `element` fixture with a type-safe `satisfies Pick<ShapesElement, ...>` shape, retaining only the fields required by the tests and preserving precise inference. Apply the same approach to the marked fixtures and resolution checks, narrowing resolved values by their `status` before accessing status-specific fields, consistent with the existing pattern near line 168; if any full-element assertion remains necessary, keep it only at the fixture boundary and document its necessity.Source: Coding guidelines
packages/core/src/engine/PointsResolver.ts (1)
200-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoidable assertions bypass the new resolver contracts.
packages/core/src/engine/PointsResolver.ts#L200-L203: parse the unknown task payload with a resource-specific guard.packages/core/src/engine/PointsResolver.ts#L403-L411: use a slice-capability type guard.packages/core/src/engine/PointsResolver.ts#L809-L812: narrow the indexed value before adding it.packages/core/tests/pointsResolver.spec.ts#L40-L48: type the mock fixture withsatisfiesor document one unavoidable boundary assertion.packages/core/tests/pointsResolver.spec.ts#L168-L185: narrow resolutions withoutas never.packages/core/tests/pointsResolver.spec.ts#L213-L230: narrow resolutions withoutas never.packages/core/tests/pointsWorkerScan.spec.ts#L12-L19: annotate the throwing function directly.As per coding guidelines, “Avoid type assertions (
as ...) when guards, precise contracts, orsatisfiescan express the same fact.”🤖 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/PointsResolver.ts` around lines 200 - 203, Replace avoidable type assertions with guards and precise contracts across the listed sites: in packages/core/src/engine/PointsResolver.ts lines 200-203, parse task.payload with a resource-specific guard; lines 403-411, use a slice-capability type guard; and lines 809-812, narrow the indexed value before adding it. In packages/core/tests/pointsResolver.spec.ts lines 40-48, type the mock fixture with satisfies or document the unavoidable boundary assertion; lines 168-185 and 213-230, narrow resolutions without as never. In packages/core/tests/pointsWorkerScan.spec.ts lines 12-19, annotate the throwing function directly.Source: Coding guidelines
packages/layers/tests/pointsResourceIdentity.spec.ts (1)
71-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLocalize the
PointsElementmock boundary. Keep one helper for thePointsElementcast; its protected/private members make that assertion unavoidable, but theresults[call++]ando.onProgressassertions can be replaced with checked access and a guard.🤖 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/pointsResourceIdentity.spec.ts` around lines 71 - 74, Localize the unsafe cast in the PointsElement mock setup by keeping it within a single dedicated helper. In the surrounding test assertions, replace results[call++] with checked access and guard o.onProgress before invoking or asserting on it, while preserving the existing test behavior.Source: Coding guidelines
packages/vis/src/SpatialCanvas/resolvers/RasterResolvers.ts (1)
93-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the raster loader before calling
getImageSize.loaderisunknownhere, sosource as neveronly hides the type gap. Add a small guard for the Viv pixel-source shape (or thread a proper loader type through); if the cast must stay, keep it local and explain why the compiler can’t express this boundary.🤖 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/resolvers/RasterResolvers.ts` around lines 93 - 98, Update rasterBounds to validate that source matches the supported Viv pixel-source shape before passing it to getImageSize, rather than using source as never without validation. Keep the guard local to rasterBounds and preserve the existing null return for absent or invalid sources; only retain a cast if necessary after narrowing, with a brief explanation of the type boundary.Source: Coding guidelines
packages/vis/tests/rasterResolvers.spec.ts (1)
32-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the assertion-heavy fixtures in
packages/vis/tests/rasterResolvers.spec.ts:32-48, 107-109, 225-246. The double casts andas neverregistry entries hide model/resolver contract drift; use typed helpers and directresolution.statuschecks instead.🤖 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/rasterResolvers.spec.ts` around lines 32 - 48, Update the raster resolver test fixtures built by imageElement and labelsElement to use typed helpers that satisfy ImageElement and LabelsElement without double casts. Replace registry entries using as never with correctly typed values, and simplify the affected assertions to check resolution.status directly while preserving the existing expected statuses.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/engine/PointsResolver.ts`:
- Line 117: Update the snapshot caching logic around the resolver’s version
checks and snapshot storage to key entries by elementKey, ctx.entryId, a stable
signature of ctx.config.featureCodes, and version. Include idle snapshots in
this keyed cache, and ensure entries absent from entries still reuse the same
identity for identical inputs while changes to entry ID or feature codes produce
a fresh snapshot.
- Around line 591-599: Update the matching-batch reuse branch in PointsResolver
so it does not reuse entry.matching when its result exceeds the new memoryCap.
Add the same upper-bound validation used by isLoadedWithCap alongside
batchAdequateForCap, allowing smaller-cap scans to replace oversized results
while preserving reuse for batches within the cap.
- Around line 170-174: The row-code planning in PointsResolver must wait for
resident point preloading to complete before scheduling row-code loading. Update
the logic around needsRowCodes and the generated preload/rowCodes tasks so fresh
filtered entries plan preload first and do not run rowCodes concurrently;
preserve the existing load-plan ordering contract and prevent independently
loaded codes from overwriting loadPoints() results.
- Around line 190-217: Update PointsResolver.load() and its ensureLoaded,
ensureFeatureCatalog, ensureRowFeatureCodes, and ensureMatchingFeaturesLoaded
calls to accept and propagate the supplied AbortSignal through all loader
operations. In PointsResolver.evict() and dispose(), abort any active preload,
catalog, row-code, or matching requests before clearing their cache/state
entries, preserving normal cleanup behavior after cancellation.
In `@packages/core/src/engine/ShapesResolver.ts`:
- Around line 182-184: Update the cancellation path in the geometry-loading
logic around Resolution.loading and plan() to capture the exact pre-load
resolution and restore it when loading is cancelled, including for an initially
unloaded slot. Ensure cancellation never leaves entry[slot] stuck in loading,
and add a regression test covering cancellation during the initial geometry
load.
- Around line 235-283: Update ShapesResolver.snapshot and bounds so memoization
is entry-local and context-aware: cache snapshots using the relevant
ctx.entryId/element context and ctx.transform rather than only the resolver-wide
version, ensuring shared elements retain distinct layer context and transform
changes recompute bounds. Replace the resolver-wide revision dependency with a
per-entry revision that changes only when that entry mutates, and preserve
identity stability when the entry and context are unchanged.
- Around line 323-325: Update the ShapesResolver cache evict method to increment
the store version after deleting the entry, ensuring external-store subscribers
are notified and receive a fresh snapshot immediately.
- Around line 127-148: Update the resource planning logic in ShapesResolver.plan
for tooltip, fillColor, and the additional optional resources so failed requests
are not automatically re-added on every plan call. Track each attempted request
key separately from success-only fields such as tooltipSignature and
fillColorColumn, suppress unchanged failed requests, and allow retries only
through an explicit retry mechanism.
In `@packages/core/src/engine/SpatialEntryStore.ts`:
- Around line 31-32: Update the in-flight tracking used by reconcile to store
each task’s promise alongside its AbortController. Use a stable per-entry
supersession slot or generation key rather than the request ID, so identical
overlapping calls await the existing promise while changed requests abort the
prior controller before starting new work. Ensure stale shape tooltip/fill-color
results cannot overwrite newer results.
In `@packages/vis/src/SpatialCanvas/imageLoaderChannelDefaults.ts`:
- Around line 125-126: Strengthen the hasVivMetadata type guard by validating
that labels and shape have the runtime array values required by
buildLabelsChannelDefaults, rather than only checking property presence. Reject
null or otherwise invalid metadata so callers fall back to blind defaults
without invoking array helpers on malformed fields.
- Around line 171-190: Update the guessRgb call in the image metadata
initialization flow to pass the complete Pixels metadata object, including
SamplesPerPixel, Type, SizeC, Interleaved, and Channels, rather than rebuilding
it with only channel names. Preserve the existing isInterleaved-based
contrastLimits and colors assignments once guessRgb receives the required
metadata.
In `@packages/vis/src/SpatialCanvas/resolvers/RasterResolvers.ts`:
- Around line 186-210: The cancellation path in the async run within the raster
resolver leaves entry.loader stuck in loading. Preserve the prior resolution
before assigning Resolution.loading, then restore that saved resolution before
returning from the isCancellation(cause) branch; keep normal success and failure
handling unchanged.
- Around line 244-260: Update the image-loading flow around
buildImageChannelDefaults and snapshot so its onNotice callback records
channel-default failures in the entry’s notices. Persist those collected notices
in EntryResources instead of always returning notices: [], while preserving the
documented fallback behavior and existing snapshot version checks.
- Around line 155-164: Update boundsFor and both snapshot cache-key paths to
include the current Matrix4 transform alongside resolver version or loader
identity. Ensure changed transforms invalidate reused bounds and snapshots,
while unchanged transforms continue using the existing cached values.
- Around line 319-321: Guard the optional task.payload before accessing
tooltipFields in the tooltip branch of RasterResolvers. Ensure invalid or
missing tooltip payloads are handled within the existing resolution error path
so reconcile() records Resolution.failed rather than throwing before the try
block; preserve the current signature generation for valid payloads.
In `@packages/vis/tests/useLayerData.spec.tsx`:
- Around line 212-219: Update the resource reads in the test around getLayers()
so every .props cast includes undefined and accesses resource through optional
chaining, matching the existing safe pattern. Add a brief comment explaining why
the props cast is required at this boundary, covering all affected assertions
and preserving their current expectations.
---
Nitpick comments:
In `@packages/core/src/engine/errors.ts`:
- Around line 163-170: Update isCancellation to remove the type assertion when
reading cause.name. After confirming cause is a non-null object, use the in
operator to verify the name property exists, then compare that property to
'AbortError' while preserving the existing DOMException handling.
In `@packages/core/src/engine/PointsResolver.ts`:
- Around line 200-203: Replace avoidable type assertions with guards and precise
contracts across the listed sites: in packages/core/src/engine/PointsResolver.ts
lines 200-203, parse task.payload with a resource-specific guard; lines 403-411,
use a slice-capability type guard; and lines 809-812, narrow the indexed value
before adding it. In packages/core/tests/pointsResolver.spec.ts lines 40-48,
type the mock fixture with satisfies or document the unavoidable boundary
assertion; lines 168-185 and 213-230, narrow resolutions without as never. In
packages/core/tests/pointsWorkerScan.spec.ts lines 12-19, annotate the throwing
function directly.
In `@packages/core/src/engine/resolution.ts`:
- Around line 60-71: Update the IDLE singleton and Resolution.idle constructor
to remove the unnecessary `as const` and `as Resolution<T>` assertions, relying
on the discriminated union’s inferred idle type while preserving the frozen,
identity-stable singleton behavior.
In `@packages/core/src/engine/ShapesResolver.ts`:
- Around line 179-184: Replace the resource and payload type assertions in the
shape-task handling flow around the stale-value assignment and the referenced
locations with a discriminated shapes-task union or local type guards. Narrow
each task by its resource before reading or assigning entry fields, so geometry,
tooltip, and fillColor payloads are type-checked without `as` or `as never`,
while preserving the existing loading/stale-value behavior.
In `@packages/core/src/engine/SpatialEntryStore.ts`:
- Around line 20-25: Update ResolverRegistry to use
Partial<Record<SpatialEntryKind, ResourceResolver<any, any>>> so its type
permits missing resolvers and matches the guarded runtime behavior throughout
the related lookup branches. Preserve the existing missing-resolver checks and
handling in the affected resolver access paths.
In `@packages/core/src/renderStack.ts`:
- Around line 96-100: Update getRenderStackHostLayerIds to use a single flatMap
over renderStack.entries, returning each host entry’s source.hostLayerId and
excluding non-host entries, while preserving the existing string[] result.
In `@packages/core/src/spatialLayerProps.ts`:
- Around line 98-113: Refactor migrateV0ToV1 to derive sublayers with flatMap:
safely parse each item using spatialSublayerSchema.safeParse, return the parsed
data for successful results and no item for failures, preserving the existing
filtering behavior and resulting SpatialSublayer[].
In `@packages/core/tests/shapesResolver.spec.ts`:
- Around line 34-40: Replace the `as unknown as ShapesElement` assertion in the
`element` fixture with a type-safe `satisfies Pick<ShapesElement, ...>` shape,
retaining only the fields required by the tests and preserving precise
inference. Apply the same approach to the marked fixtures and resolution checks,
narrowing resolved values by their `status` before accessing status-specific
fields, consistent with the existing pattern near line 168; if any full-element
assertion remains necessary, keep it only at the fixture boundary and document
its necessity.
In `@packages/layers/tests/pointsResourceIdentity.spec.ts`:
- Around line 71-74: Localize the unsafe cast in the PointsElement mock setup by
keeping it within a single dedicated helper. In the surrounding test assertions,
replace results[call++] with checked access and guard o.onProgress before
invoking or asserting on it, while preserving the existing test behavior.
In `@packages/vis/src/SpatialCanvas/resolvers/RasterResolvers.ts`:
- Around line 93-98: Update rasterBounds to validate that source matches the
supported Viv pixel-source shape before passing it to getImageSize, rather than
using source as never without validation. Keep the guard local to rasterBounds
and preserve the existing null return for absent or invalid sources; only retain
a cast if necessary after narrowing, with a brief explanation of the type
boundary.
In `@packages/vis/tests/rasterResolvers.spec.ts`:
- Around line 32-48: Update the raster resolver test fixtures built by
imageElement and labelsElement to use typed helpers that satisfy ImageElement
and LabelsElement without double casts. Replace registry entries using as never
with correctly typed values, and simplify the affected assertions to check
resolution.status directly while preserving the existing expected statuses.
In `@packages/vis/tests/useLayerData.spec.tsx`:
- Around line 50-61: Add brief comments immediately before the double assertions
in pointsElement and shapesElement explaining that the test mocks only the
subset of the PointsElement/ShapesElement interfaces needed by the test, so the
compiler cannot validate the partial mock directly. Keep the casts local and
unchanged.
🪄 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: 4564582d-79c1-4e79-9186-003f1cc0db6c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (60)
docs/adr/0004-resource-resolver-owned-by-core.mddocs/plans/resource-resolver-handoff.mdpackages/core/src/engine/PointsResolver.tspackages/core/src/engine/ShapesResolver.tspackages/core/src/engine/SpatialEntryStore.tspackages/core/src/engine/errors.tspackages/core/src/engine/index.tspackages/core/src/engine/resolution.tspackages/core/src/engine/resolver.tspackages/core/src/index.tspackages/core/src/renderStack.tspackages/core/src/spatialLayerProps.tspackages/core/tests/badFiles.spec.tspackages/core/tests/dependencies.spec.tspackages/core/tests/mortonPointsTiling.spec.tspackages/core/tests/parquetFooterStats.spec.tspackages/core/tests/pointsFeatures.spec.tspackages/core/tests/pointsLoader.spec.tspackages/core/tests/pointsPreloadGuard.spec.tspackages/core/tests/pointsPreloadReadStrategy.spec.tspackages/core/tests/pointsResolver.spec.tspackages/core/tests/pointsWorker.spec.tspackages/core/tests/pointsWorkerScan.spec.tspackages/core/tests/renderStack.spec.tspackages/core/tests/resolution.spec.tspackages/core/tests/schemas.spec.tspackages/core/tests/shapesRenderData.spec.tspackages/core/tests/shapesResolver.spec.tspackages/core/tests/spatialEntryError.spec.tspackages/core/tests/spatialLayerProps.spec.tspackages/core/tests/spatialViewFit.spec.tspackages/core/tests/tableAssociations.spec.tspackages/core/tests/transformations.spec.tspackages/core/tests/vshapes.spec.tspackages/core/tests/vtableMultipart.spec.tspackages/layers/src/SpatialLayer.tspackages/layers/src/adapters/PointsRendererAdapter.tspackages/layers/src/engine/PointsDataEngine.tspackages/layers/src/index.tspackages/layers/src/shapesLayer.tspackages/layers/tests/labelsLayer.spec.tspackages/layers/tests/pointsDataEngine.spec.tspackages/layers/tests/pointsFeatureColor.spec.tspackages/layers/tests/pointsLayerFilter.spec.tspackages/layers/tests/pointsLoadPlan.spec.tspackages/layers/tests/pointsRenderAttributes.spec.tspackages/layers/tests/pointsRenderStrategies.spec.tspackages/layers/tests/pointsResourceIdentity.spec.tspackages/layers/tests/renderStackShim.spec.tspackages/layers/tests/shapesLayer.spec.tspackages/vis/package.jsonpackages/vis/src/SpatialCanvas/imageLoaderChannelDefaults.tspackages/vis/src/SpatialCanvas/resolvers/RasterResolvers.tspackages/vis/tests/demoUrl.spec.tspackages/vis/tests/pointsFeatureRowState.spec.tspackages/vis/tests/rasterResolvers.spec.tspackages/vis/tests/spatialCanvasViewer.spec.tspackages/vis/tests/useLayerData.spec.tsxpackages/vis/tests/vivImagePassthrough.spec.tspackages/vis/tests/vivSpatialViewer.spec.ts
Comment on lines
+170
to
+174
| // Was `void engine.ensureRowFeatureCodes(...)` at useLayerData.ts:1425. | ||
| const needsRowCodes = selectionActive || config.colorByFeature === true; | ||
| if (needsRowCodes && !this.hasRowFeatureCodes(key)) { | ||
| tasks.push({ id: `${key}#rowCodes`, resource: 'rowCodes' }); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wait for the resident preload before planning row codes.
A fresh filtered entry currently schedules preload and rowCodes concurrently. If the independent row-code load finishes last, it can overwrite the codes returned with loadPoints()—including with a differently capped array—breaking row alignment. The existing load-plan contract in packages/layers/tests/pointsLoadPlan.spec.ts Lines 130-166 also requires preloaded points first.
Proposed fix
- if (needsRowCodes && !this.hasRowFeatureCodes(key)) {
+ if (
+ needsRowCodes &&
+ this.isLoadedWithCap(key, cap) &&
+ !this.hasRowFeatureCodes(key)
+ ) {
tasks.push({ id: `${key}`#rowCodes``, resource: 'rowCodes' });
}📝 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
| // Was `void engine.ensureRowFeatureCodes(...)` at useLayerData.ts:1425. | |
| const needsRowCodes = selectionActive || config.colorByFeature === true; | |
| if (needsRowCodes && !this.hasRowFeatureCodes(key)) { | |
| tasks.push({ id: `${key}#rowCodes`, resource: 'rowCodes' }); | |
| } | |
| // Was `void engine.ensureRowFeatureCodes(...)` at useLayerData.ts:1425. | |
| const needsRowCodes = selectionActive || config.colorByFeature === true; | |
| if ( | |
| needsRowCodes && | |
| this.isLoadedWithCap(key, cap) && | |
| !this.hasRowFeatureCodes(key) | |
| ) { | |
| tasks.push({ id: `${key}`#rowCodes``, resource: 'rowCodes' }); | |
| } |
🤖 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/PointsResolver.ts` around lines 170 - 174, The
row-code planning in PointsResolver must wait for resident point preloading to
complete before scheduling row-code loading. Update the logic around
needsRowCodes and the generated preload/rowCodes tasks so fresh filtered entries
plan preload first and do not run rowCodes concurrently; preserve the existing
load-plan ordering contract and prevent independently loaded codes from
overwriting loadPoints() results.
Contributor
Author
There was a problem hiding this comment.
Deferred — and the proposed gate is premature here. This is race R5 from the implementation plan (docs/plans/resource-resolver-handoff.md, Track A), preserved verbatim: ensureRowFeatureCodes takes no memory cap and can overwrite the preload's row codes with a differently-capped array. Step 1 is a faithful re-housing, not a behaviour change.
Gating rowCodes on isLoadedWithCap only partially fixes R5 and interacts with the preload's own code-setting (the preload already sets entry.rowCodes at the user's cap when it completes). The correct fix — a single cap threaded identically to both loads — lands with Track A's RequestSlot rework, which owns cap-consistent supersession.
(Aside: the pointsLoadPlan.spec.ts reference is a different module — the tiled loader's plan — not this resolver's plan().)
Contributor
There was a problem hiding this comment.
@xinaesthete Thanks for the detailed correction — that's a fair point on both counts. I mis-cited pointsLoadPlan.spec.ts (tiled loader plan, unrelated to this resolver's plan()), and I agree gating on isLoadedWithCap here would be a partial fix that could conflict with the preload's own rowCodes write, rather than the proper cap-consistent supersession that Track A's RequestSlot rework is meant to own. Given this is a faithful re-housing of existing behavior (bug included) rather than a behavior change, deferring makes sense.
✏️ Learnings added
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/SpatialData.js PR: 85
File: packages/core/src/engine/PointsResolver.ts:170-174
Timestamp: 2026-07-15T10:46:42.545Z
Learning: In `packages/core/src/engine/PointsResolver.ts`, the concurrent scheduling of `preload` and `rowCodes` tasks in `plan()` is a known, intentionally-preserved race (R5) documented in `docs/plans/resource-resolver-handoff.md` Track A: `ensureRowFeatureCodes` takes no memory cap and can overwrite the preload's row codes with a differently-capped array. Partial fixes like gating `rowCodes` on `isLoadedWithCap` are insufficient because the preload path already sets `entry.rowCodes` at the user's cap on completion. The correct fix requires a single cap threaded identically to both loads, landing with Track A's `RequestSlot` rework which owns cap-consistent supersession. Do not re-flag this as a new issue in this resolver until that rework lands.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+127
to
+148
| const fields = ctx.config.tooltipFields; | ||
| if (fields && fields.length > 0) { | ||
| const signature = tooltipSignature(fields); | ||
| if (!entry || entry.tooltipSignature !== signature) { | ||
| // The signature is IN the id: changing the tooltip columns supersedes. | ||
| tasks.push({ | ||
| id: `${key}#tooltip:${signature}`, | ||
| resource: 'tooltip', | ||
| payload: { tooltipFields: fields }, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| const column = ctx.config.fillColorByColumn?.columnName; | ||
| if (column) { | ||
| if (!entry || entry.fillColorColumn !== column) { | ||
| tasks.push({ | ||
| id: `${key}#fillColor:${column}`, | ||
| resource: 'fillColor', | ||
| payload: { column }, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent failed optional resources from being replanned indefinitely.
tooltipSignature and fillColorColumn are updated only on success. After a failure, every subsequent plan() returns the same task again, potentially creating a tight retry loop. Track the attempted request key independently of its successful value, with retry driven explicitly.
Also applies to: 195-212, 216-229
🤖 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 127 - 148, Update
the resource planning logic in ShapesResolver.plan for tooltip, fillColor, and
the additional optional resources so failed requests are not automatically
re-added on every plan call. Track each attempted request key separately from
success-only fields such as tooltipSignature and fillColorColumn, suppress
unchanged failed requests, and allow retries only through an explicit retry
mechanism.
Contributor
Author
There was a problem hiding this comment.
Partially deferred. The retry mechanism (retryable + an explicit retry()) is a Track A deliverable per the plan, so the general fix lands there.
The nuance you spotted is worth recording, though: the tight loop — re-planning a failed tooltip/fillColor on every reconcile — is marginally worse than the old effect-driven behaviour. It's latent in Step 1 (the resolvers are unconsumed; nothing drives reconcile in a loop yet), but I've flagged it as a candidate to pull forward if it actually spins once the flip wires reconcile to renders.
Contributor
There was a problem hiding this comment.
@xinaesthete Thanks for the context — that's a reasonable scoping call. Deferring the general retry/backoff mechanism to Track A (with retryable + explicit retry()) makes sense, and I agree the risk is currently latent since nothing drives reconcile() in a loop yet in Step 1. Good call flagging it as a pull-forward candidate once the render-wiring lands; that's exactly the kind of thing that could go from "latent" to "spinning" quickly once reconcile is called on every render.
I'll leave this as-is for now given the deferral.
✏️ Learnings added
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/SpatialData.js PR: 85
File: packages/core/src/engine/ShapesResolver.ts:130-151
Timestamp: 2026-07-15T10:46:46.939Z
Learning: In `packages/core/src/engine/ShapesResolver.ts` (and similarly for other Resource Resolvers), `plan()` currently re-adds tasks for failed optional resources (e.g. tooltip, fillColor) on every call because `tooltipSignature`/`fillColorColumn` are only updated on success. Per xinaesthete, the general fix (tracking attempted request keys independently of success, with explicit retry via a `retryable` flag and `retry()` method) is a Track A deliverable in the Resource Resolver plan, deferred intentionally in this Step 1 PR since resolvers are not yet wired to a `reconcile` loop driven by renders. This is a known/flagged candidate to prioritize if `reconcile` starts being called repeatedly (e.g., once render-driven wiring lands) and the retry loop becomes an actual tight loop rather than a latent risk.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+212
to
+219
| const resources = [0, 1, 2].map( | ||
| () => (result.current.getLayers()[0]?.props as { resource?: unknown } | undefined)?.resource | ||
| ); | ||
|
|
||
| expect(resources[0]).toBeDefined(); | ||
| expect(resources[1]).toBe(resources[0]); | ||
| expect(resources[2]).toBe(resources[0]); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Inconsistent optional chaining on .props cast — lines 234/238 can throw where 213-214 don't.
Lines 213-214 cast props to { resource?: unknown } | undefined and then use ?.resource. Lines 234 and 238 cast to { resource?: unknown } (no | undefined) and access .resource directly, even though getLayers()[0]?.props can still be undefined at runtime. If that assumption ever breaks, this throws an unhelpful TypeError instead of a clear assertion failure. Also, per coding guidelines, none of these .props assertions include a comment on why the cast is needed at this boundary.
🐛 Proposed fix
- const before = (result.current.getLayers()[0]?.props as { resource?: unknown }).resource;
+ const before = (result.current.getLayers()[0]?.props as { resource?: unknown } | undefined)
+ ?.resource;
...
- const after = (result.current.getLayers()[0]?.props as { resource?: unknown }).resource;
+ const after = (result.current.getLayers()[0]?.props as { resource?: unknown } | undefined)
+ ?.resource;Also applies to: 234-234, 238-238
🤖 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/useLayerData.spec.tsx` around lines 212 - 219, Update the
resource reads in the test around getLayers() so every .props cast includes
undefined and accesses resource through optional chaining, matching the existing
safe pattern. Add a brief comment explaining why the props cast is required at
this boundary, covering all affected assertions and preserving their current
expectations.
Source: Coding guidelines
Contributor
Author
There was a problem hiding this comment.
Minor — acknowledged. This identity-assertion block in the useLayerData spec gets rewritten when the hook is flipped onto the resolver loop (C8), since getLayers()'s internals change then. I'll make the .props casts consistently optional-chained (with a boundary comment) as part of that work. Not blocking here.
Contributor
There was a problem hiding this comment.
@xinaesthete Sounds good — deferring this to the C8 resolver-loop rework makes sense given this block gets rewritten anyway. Thanks for confirming the plan to make the .props casts consistently optional-chained with a boundary comment at that point.
🐰💭
Fixes the review findings that touch the contract the flip (C8) builds on, before it builds on them. The deferred items (worker cancellation, R3/R5, store supersession, pre-existing guessRgb) are left for Track A / their own issues, as planned. Snapshot memoization was keyed on the resolver version alone, but an EntryResources embeds per-entry data (entryId) and per-context data (the transform, via bounds; the selection, via notices). Two layers over one element got each other's snapshot; a coordinate-system change left bounds stale. New SnapshotCache keys by (entryId, version, transform identity, config signature) and self-cleans via evictByElement. Applied in all three resolvers (points, shapes, raster) — it recurred in each. Transform identity is the right key, not value: availableElements is memoised on [spatialData, coordinateSystem], so a given entry sees a stable transform across pan/zoom and a fresh one exactly when the coordinate system changes — which is when bounds must recompute. The inner bounds memo now keys on the transform too, or it would defeat the outer fix. Cancellation left a slot stuck in `loading` forever: the loader was set to loading before the load, and the isCancellation early-return didn't restore it, so plan() (which only reschedules an idle slot) never retried. Both the shapes and raster load paths now capture the prior resolution and restore it on cancel. Regression tests for initial-load cancellation added. evict() now notifies, so external-store consumers drop the stale snapshot immediately instead of at the next unrelated mutation. Points evict notifies too; the frozen pointsDataEngine.spec.ts still passes unchanged (its one notify test unsubscribes before evicting). Channel-default failures are surfaced as a channel-defaults-fallback EntryNotice instead of vanishing. The verbatim lift had replaced the old console.warn with an onNotice callback that was never wired — strictly less observable than before. ImagesResolver now collects the notice and puts it in the snapshot; the image still draws with fallback channels, which is exactly the healthy-data-with-a-caveat case EntryNotice exists for. Also: guard the optional tooltip payload before deref (a malformed task is a no-op, not a reconcile-rejecting throw); isCancellation uses the `in` operator instead of a type assertion. New coverage: SnapshotCache unit spec (shared-element separation, per-dim invalidation, evict-by-element); shared-element and transform-invalidation tests in the points, shapes and raster suites; cancellation-restore regression tests. 559 tests (was 542). Build, biome, dependency boundaries green; frozen spec untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
xinaesthete
added a commit
that referenced
this pull request
Jul 16, 2026
* Consume Resource Resolvers in useLayerData (Step 1, all 3 increments) 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> * disable tsgo extension in this workspace, pending upgrade to TS7 * Document the non-owning points-resolver lifecycle + guard it with a test 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> * docs: reflect Resource Resolver Step 1 as landed + add changeset - 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> * Extract non-owning proxy + shapes projection out of useLayerData 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> * Review fixes: preserve explicit shape stroke; replan on element-map change 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> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.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.
Resource Resolver — the foundation (Step 0 + Step 1 infrastructure)
Implements Step 0 and the resolver infrastructure of Step 1 from
docs/plans/resource-resolver-handoff.md, per ADR 0004.All four resolvers exist, satisfy one interface, and are tested — but nothing consumes them yet.
useLayerDatastill runs its old path unchanged, so this PR is almost entirely additive. The only behavioural surfaces touched are the mechanical RenderStack move and thePointsDataEnginesplit, both proven identical by specs that pass byte-for-byte. The flip ofuseLayerDataonto the resolver loop is deliberately deferred to a focused follow-up (see below), so the risky, browser-verified change stays isolated from this foundation.Why this work exists
Not architectural taste.
tgpu-htj2k(separate repo) renders SpatialData imagery via three.js/WebGPU today, depends on@spatialdata/core, and excludes deck.gl/React by name. It reached intocorefor a resolution layer, found onlyreadZarr+ element discovery, and hand-rolled the whole thing —Select,Tileset,TileCache,loadScheduler, Nyquist LOD, ~10 files with tests. A second Resource Resolver already exists because the first was locked behind deck.gl. This moves the resolver tocoreso both consumers share it.What's in this PR
createImageLoaderalready takes an injectedfetchMultiscales— no React closure, so no port to invent. Placement is per-kind, driven by dependency.Resolution<T>,SpatialEntryError,EntryNotice,toSpatialEntryError,isCancellation,fromResultincore/src/engine.layers/visretain re-exports as compat shims (ADR 0004 §5). Guarded by an identity assertion so MDV's imports can't silently drift.useLayerData; identity stability for all three points resources (only one was covered before); dependency-boundary tests that turn two "still true" DoD boxes into CI.PointsDataEngine's cache/lifecycle half, framework-free incore.layers;PointsDataEnginebecomes a 228-line facade.pointsDataEngine.spec.tspasses unchanged, byte for byte — the acceptance criterion for the split.coreinterface, resident invisnext to their Viv deps. No port.Design decisions worth a reviewer's eye
Error(string), and anything off the points worker has already lost its type ({ ok: false; error: string }). SotoSpatialEntryError's context carries afallbackkind. Message-sniffing is quarantined to one recogniser with aTODO(Track A).pointsRenderResourceSignaturekeys on row count, which is why the old engine manually nulledentry.resourceon every swap. Out of the entry, the adapter keys on the batch object's identity — exact, because batches are always replaced, never mutated. The bookkeeping disappears.SpatialEntryStoreholds onlyResourceResolver. If it could distinguish avis-resident resolver from acore-resident one, "images is special" would become representable — that's how one interface quietly becomes two. A test asserts it can't.plan()/load()on the resolver;project()/render()on the Renderer Adapter — corrects the handoff's phase table against ADR 0004 §4. This is what makes today'svoid engine.ensureX(...)insidegetLayers()a type error once the flip lands.Deferred, by design
useLayerDataflip — the ~700-line rewrite that deletes six of seven kind-switch ladders and re-points all 17 members at resolver snapshots. This is the browser-verified change (pan-during-scan flash, filter-toggle-without-mouse-move, cap drag), kept out of this PR on purpose. The guard tests here are its net.queueMicrotaskdefence, the'use no memo'hatches. Follows the flip.Verification
Build (TS7 native typecheck), full suite (542 tests, up from 427), Biome, and the new dependency-boundary tests all green.
coreimports no react/deck.gl/viv/avivatorish;layersimports no react — now enforced, not asserted in prose.🤖 Generated with Claude Code
Summary by CodeRabbit