Design notes: Resource Resolver in core (ADR 0004/0005) + implementation handoff - #84
Merged
Merged
Conversation
…ing it
CONTEXT.md already separates the Resource Resolver ("store-agnostic ... stable
loaded resources for renderers") from the Renderer Adapter ("avoid: state
store"), and says a Spatial Entry is resolved by the former before the latter
runs. The code drifted: the resolver doesn't exist as a module, and its
responsibilities are split between a 1,873-line React hook in vis and a 940-line
engine in layers — both above the deck.gl seam.
The decisive evidence is not architectural taste. tgpu-htj2k renders SpatialData
imagery through three.js/WebGPU today, depends on @spatialdata/core + zarrextra,
and excludes layers/vis/avivatorish by name. It reached into core for a
resolution layer, found only readZarr plus element discovery, and hand-rolled
Select / Resolve / Tileset / TileCache / loadScheduler / LOD — ~10 files with
tests. A second Resource Resolver already exists, because the first was locked
behind deck.gl.
ADR 0004 — Resource Resolver Owned By Core:
- the Resolver lives in core; per-kind resolvers behind one interface
- Resolution<T> and SpatialEntryError are core types; failure is per-resource
- the deck Renderer Adapter stays in layers (identity-stable memoisation is a
deck requirement and belongs on the renderer side)
- RenderStack moves to core, forced by dependency direction
- no runtime dependency (Effect, neverthrow, TanStack) enters core's interface
- Group Entry / blend compositing explicitly out of scope; add no framebuffer
hook in anticipation
Amends ADR 0001's package-placement claim only. Its substance stands: host
overlays as descriptors, no parallel layerOrder state, MobX outside the contract.
ADR 0005 — Memory Accounting Before Management:
Both ingest paths have the same two-tier shape and neither is managed. Parquet
holds encoded bytes AND the decoded Arrow table for the same file, unbounded,
forever. Zarr holds neither: fizarrita has no cache, only a cache-shaped hole we
never fill, so every tile re-fetches and re-decodes. The only eviction machinery
in the repo is PointsDataEngine.evict(), keyed on element unload.
Adopt in rungs: the byteLength scalar, bound the two unbounded caches, fill the
empty chunk seam. Defer the encoded tier and the Resource Ceiling until
measurement justifies them — following tgpu-htj2k, which built and tested
selectWithinBudget and then never called it.
CONTEXT.md gains: Resolution, Spatial Entry Error, Entry Notice, Encoded/Decoded
Tier, Resource Ceiling. Resource Resolver and Renderer Adapter sharpened.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ngine plan
Sequencing for ADR 0004/0005, ready to pick up.
Step 0 (types only) and Step 1 (the resolver interface + four thin
behaviour-identical adapters) are shared and land first. Step 1 is the fork
point: after it, three tracks touch different files and can run in parallel.
Track A — points state model. RequestSlot replaces four hand-rolled
dedup/supersede implementations whose guards compare values (a memory cap, a
signature string) instead of request identity. Four live races are named with
their triggers; none is reachable by the existing 845-line engine spec,
because PointsEntry is private and nothing can hand it two deferred runs.
Carries the Effect spike, scoped to the matching scan only, behind the slot
interface, with a kill criterion agreed up front.
Track B — shapes. The loader seam ADR 0003 already specifies, then a flat
transferable batch. Needs nothing from Track A: viewport loading lives behind
loadInBounds() inside deck's TileLayer, not in the resolver. Starts
immediately.
Track C — memory, ADR 0005 rungs 1-3. Leak fixes, not architecture.
Definition of done is written as invariants that fail loudly on drift: no react
in core, no 'use no memo' left in vis, the resolver exercised by a test that
constructs no GL context.
The LayerDataEngine plan is superseded. Its diagnosis stands and is still worth
reading — the god-hook, the unreachable-headless problem — but its target home
was one package too high, and its open question 2 ("one engine or per-type
sub-engines?") is now answered: per-kind.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 36 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 (5)
📝 WalkthroughWalkthroughDocumentation updates establish Resource Resolver ownership in ChangesResource Resolver architecture
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 6
🤖 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 `@CONTEXT.md`:
- Around line 63-65: The Resolution contract must define the fallback when stale
is unavailable, so qualify the “never blanks” guarantee to cover only retained
stale values and specify the behavior for eviction or non-retryable failure
without stale data. Update CONTEXT.md at lines 63-65 and
docs/adr/0005-memory-accounting-before-management.md at lines 178-181 so the
documented eviction policy and Resolution semantics agree; both sites require
documentation changes only.
In `@docs/adr/0001-render-stack-owned-by-layers.md`:
- Around line 3-10: Update the retained ADR paragraph to state that the
canonical RenderStack lives in `@spatialdata/core`, matching the ADR 0004
amendment, while preserving the remaining rendering and overlay contract
statements.
In `@docs/adr/0004-resource-resolver-owned-by-core.md`:
- Around line 72-75: Clarify the ADR’s ownership boundary for viewport-driven
loading: state that the deck.gl adapter only supplies viewport state to the core
Resource Resolver, while core initiates and reconciles requests, including
supersession, cancellation, caching, and eviction. Update the corresponding
out-of-scope section as well so it does not imply that TileLayer owns request
reconciliation.
- Around line 95-100: Clarify the RenderStack export migration contract in the
ADR: explicitly state whether the existing `@spatialdata/layers` and
`@spatialdata/vis` re-exports remain as compatibility shims or are removed after
`@spatialdata/core` becomes canonical. Include the status of RenderStack and its
persistence schemas, and align the migration wording with the chosen behavior.
In `@docs/plans/resource-resolver-handoff.md`:
- Around line 14-27: Add the text language identifier to the Markdown fence
surrounding the architecture diagram, changing the opening fence to ```text
while preserving the diagram content and closing fence.
- Around line 47-64: Update Step 0 in the resource resolver handoff plan to
reflect that it contains runtime behavior as well as shared types: either rename
it to shared contracts or move the implementations of toSpatialEntryError() and
fromResult() into a later behavior step, while keeping the Resolution and error
type definitions in the initial shared step.
🪄 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: 5667df37-0f56-48fa-b7a3-dea61ed0db03
📒 Files selected for processing (6)
CONTEXT.mddocs/adr/0001-render-stack-owned-by-layers.mddocs/adr/0004-resource-resolver-owned-by-core.mddocs/adr/0005-memory-accounting-before-management.mddocs/plans/layer-data-engine-decomposition.mddocs/plans/resource-resolver-handoff.md
Comment on lines
+72
to
+75
| 1. **The Resource Resolver lives in `@spatialdata/core`.** Framework-free: no | ||
| React, no deck.gl, no Viv. It owns the cache, request supersession, | ||
| cancellation, streaming partials, eviction, entry resolution | ||
| (element + transform to the active coordinate system), and world bounds. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Clarify who owns viewport-driven request reconciliation.
The decision assigns supersession, cancellation, caching, and eviction to the renderer-agnostic core resolver, but the out-of-scope section leaves viewport-driven loading inside deck's TileLayer. If that layer still initiates or owns request reconciliation, the resolver is not the shared lifecycle owner described here. State that the adapter only supplies viewport state to core, or narrow the ownership claim.
Also applies to: 154-159
🤖 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 `@docs/adr/0004-resource-resolver-owned-by-core.md` around lines 72 - 75,
Clarify the ADR’s ownership boundary for viewport-driven loading: state that the
deck.gl adapter only supplies viewport state to the core Resource Resolver,
while core initiates and reconciles requests, including supersession,
cancellation, caching, and eviction. Update the corresponding out-of-scope
section as well so it does not imply that TileLayer owns request reconciliation.
… shims
The sharpest review comment and the sharpest human one turned out to be the same
one, from opposite directions: ADR 0004 assigned supersession, cancellation,
caching and eviction to a renderer-agnostic core resolver, then left
viewport-driven loading inside deck's TileLayer. Those contradict.
Resolved by saying out loud that tile scheduling is renderer-owned, by design:
element-level lifecycle (preload, catalog, row codes, geometry, tooltip,
fill colour) -> Resource Resolver, in core. Identical for every renderer;
duplicating it is what tgpu-htj2k was forced to do.
tile-level lifecycle (which tiles, what LOD, when to abort on pan)
-> Renderer Adapter. deck's TileLayer and tgpu-htj2k's Select +
loadScheduler implement different, correct policies for different viewports
and budgets. A renderer-neutral tile scheduler would serve neither.
the loader facet (capabilities, loadInBounds) and the byte-level cache
-> core, shared. Both tile schedulers call them.
Adds an explicit Non-goals section, because "deck-agnostic resolver" was reading
as "deck-agnostic render path", which it is not:
- A renderer-neutral render abstraction is NOT a goal. The adapter seam exists so
the deck adapter can lean all the way into deck-geoarrow and viv, not so we can
reimplement them. (ADR 0003 already said this.)
- Batch representation is decided per encoding, not by policy. Delegate where an
existing deck layer can consume the encoding; hand-roll flat typed arrays where
it cannot — both are expected outcomes, and the strategy registry is already the
dispatch. Wild-type shapes are WKB, not GeoArrow, so a decode is unavoidable;
points are x/y columns, not encoded geometry, so GeoArrowScatterplotLayer buys
nothing. The only requirement on a batch is that it be transferable across the
worker seam and not allocate one JS object per vertex.
- This ADR does not decide viv-vs-own-renderer for images. That is a Renderer
Adapter question. zarrextra's VivCompatiblePixelSource already serves both viv
and tgpu-htj2k, so the shared image seam already exists below the resolver. A
renderer-agnostic resolver buys the 3D option without spending it.
Also from review:
- Resolution.stale is a retention, not a guarantee. CONTEXT.md promised a failed
refine "never blanks a working view" while ADR 0005 permitted dropping stale on
eviction and non-retryable failure. Both now say the same thing: the no-blank
behaviour holds while stale is retained; once released the resource is not
renderable and the UI shows the error. Callers must handle the no-stale case.
- RenderStack migration contract stated: core becomes canonical; layers and vis
retain re-exports as compatibility shims. No consumer import moves. Removing the
shims is a separate deprecation, coordinated with MDV.
- Handoff Step 0 renamed "shared contracts" — it does carry toSpatialEntryError()
and fromResult(), which are inseparable from the types.
- ADR 0001's body marked historical inline, so a reader who skims past the
amendment banner is not misdirected. The body is not rewritten: an amended ADR
is a historical record.
- Fence language on the architecture diagram.
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.
Design notes only — no source changes. Two ADRs, a
CONTEXT.mdupdate, and an implementation handoff, ready to pick up.Why
CONTEXT.mdalready separates the Resource Resolver ("store-agnostic … stable loaded resources for renderers") from the Renderer Adapter ("avoid: state store"), and says a Spatial Entry is resolved by the former before the latter runs. The code drifted from that: the Resolver doesn't exist as a module, and its responsibilities are split between a 1,873-line React hook invisand a 940-line engine inlayers— both above the deck.gl seam.The decisive evidence isn't architectural taste.
tgpu-htj2krenders SpatialData imagery through three.js/WebGPU today, depends on@spatialdata/core+zarrextra, and excludeslayers/vis/avivatorishby name (its ADR-0010: "No deck.gl / React enters the render path"). It reached intocorefor a resolution layer, found onlyreadZarrplus element discovery, and hand-rolled the whole thing —Select,Resolve,Tileset,TileCache,loadScheduler, Nyquist LOD. Ten files, with tests.A second Resource Resolver already exists, because the first one was locked behind deck.gl. The duplication is being paid for now, in another repo.
What's decided
ADR 0004 — Resource Resolver Owned By Core
core. Per-kind resolvers behind one interface, not one engine.Resolution<T>andSpatialEntryErrorare core types. Failure is per-resource, not per-entry.layers— identity-stable memoisation is a deck requirement.RenderStackmoves tocore(dependency direction forces it).core's interface —coreis alsotgpu-htj2k's dependency root.Amends ADR 0001's package-placement claim only. Its substance stands: host overlays as descriptors, no parallel
layerOrderstate, MobX outside the contract.ADR 0005 — Memory Accounting Before Management
Both ingest paths have the same two-tier shape, and neither is managed:
ChunkCacheseam — emptyparquetTableBytes— unbounded, never evictedparquetTableCache— unbounded, never evictedParquet holds both tiers for the same file, forever. Zarr holds neither — fizarrita has no cache, only a cache-shaped hole we never fill, so every tile re-fetches and re-decodes. The only eviction machinery in the repo is
PointsDataEngine.evict(), keyed on element unload.Decided as a ladder with an explicit stop line: adopt the
byteLengthscalar, bound the two unbounded caches, fill the empty chunk seam — then stop. The encoded tier and the Resource Ceiling wait for measurement, followingtgpu-htj2k, which built and unit-testedselectWithinBudgetand then never called it.What's sequenced
Resource Resolver — implementation handoff
Step 0 (types) and Step 1 (interface + four thin behaviour-identical adapters) are shared and land first. Step 1 is the fork point — after it, three tracks touch different files:
RequestSlotreplaces four hand-rolled supersede implementations whose guards compare values (a memory cap, a signature string) instead of request identity. Four live races are named with their triggers; none is reachable by the existing 845-line engine spec. Carries the Effect spike, scoped to the matching scan only, with a kill criterion agreed up front.loadInBounds()inside deck'sTileLayer, not in the resolver. Can start immediately.Definition of done is written as invariants that fail loudly on drift: no
reactincore, no'use no memo'left invis, the resolver exercised by a test that constructs no GL context.Also here
CONTEXT.mdgains Resolution, Spatial Entry Error, Entry Notice, Encoded/Decoded Tier, Resource Ceiling;Resource ResolverandRenderer Adaptersharpened.docs/plans/layer-data-engine-decomposition.mdis superseded — its diagnosis stands, its target home was one package too high, and its open question 2 is now answered (per-kind).probeDecompressedSizedoesn't recogniseimagecodecs_jpeg2kor HTJ2K, so for JP2K-backed images it hands the compressed byte length toinferChunkShapeas the decompressed size. That's our actual imagery, and it's owed upstream.Review notes
The interface behind ADR 0004 was designed four ways in parallel under four different constraints (minimise the interface; maximise extensibility; optimise for the caller; error-as-value spine). All four independently produced per-kind resolvers with per-resource resolutions — that convergence is the evidence for decisions 2 and 3, not my preference.
🤖 Generated with Claude Code
Summary by CodeRabbit