Points: Morton-tiled viewport loading, on by default (D5) - #155
Open
xinaesthete wants to merge 11 commits into
Open
Points: Morton-tiled viewport loading, on by default (D5)#155xinaesthete wants to merge 11 commits into
xinaesthete wants to merge 11 commits into
Conversation
`PointsResolver` gains a `tiling` resource — a one-key `RequestSlot` holding the
element's tileable Morton metadata, or `null` when it cannot drive viewport tiles
(no artifact, no row-group range reads, no bounds, or a failed probe). `plan()`
asks `planPointsLoads` for the probe and preload decisions together, so a tileable
element no longer schedules a full-table read it would immediately throw away.
The row-codes and matching tasks are deferred on the same question. Gating them on
`isTiled` alone is not enough: planning them while the probe is still in flight
does the wasted read AND settles the codes, so the next pass reads "already
loaded" and the waste becomes invisible — the exact shape of bug the deferral
exists to prevent.
A failed probe reads two ways on purpose: `failed` and retryable in the snapshot,
but `null` ("cannot tile") to the planner, so the layer falls through to the
ordinary preload rather than stranding — and `isTilingSettled` counts it as
answered, so a persistent failure does not re-probe every reconcile.
Opt-in and inert by default: `PointsResolveConfig.pointsTiling` defaults to
`'off'`, collapsing planning to exactly today's behaviour. Nothing renders through
the tiled path yet.
`planPointsLoads` moves to core (core cannot import from layers, and a second copy
is how the two drift); layers re-exports it, so no consumer import moves.
Plan: docs/plans/points-morton-tiled-viewport-loading.md, harvested from
backup/points-wip-20260702.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…obes Diagnosing holes in the tiled points render ruled out the store: on a real 12.1M -point Xenium artifact the stored morton codes reproduce exactly from the sentinel bbox under the reader's own formula (215k rows sampled, 100% match), row-group statistics are monotonic and non-overlapping, byte ranges are contiguous, and the reader's row-group selection for a viewport rectangle is exact — 92 of 92, nothing missed, nothing wasted. What is wrong is how much work it does to get there. `zcoverRectangle` recursed to the full 16 bits per axis, resolving the rectangle to individual quantised cells when its only job is picking row groups: 38,014 intervals for one viewport-sized query, each driving two bisects. Capped at depth 10 that is 521 intervals selecting the SAME 92 row groups — verified over a viewport tile, the whole slide and a zoomed-in box. Stopping early only ever widens a cell, and the extra rows are filtered against the exact bounds after the read, so the cover stays complete; the tests pin completeness rather than the interval count alone. The extent cache held the settled value, so it deduped nothing while a read was in flight — and this index is built under exactly that load, every viewport tile bisecting over the same row groups at once. Each tile started its own full row-group fetch for an entry the others were already fetching: 8 concurrent callers cost 18 range reads, now 4. A failed probe is evicted instead of cached so a transient error cannot strand the bisect for the life of the source. The real fix for that probe is to read the column statistics already sitting in the footer we parse, rather than range-reading and decoding the row group twice. The vendored parquet-wasm build exposes no statistics accessor, so it needs a wasm rebuild or a minimal Thrift footer read; the constraint is now documented at the call site. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… holes)
The tiled points render had holes. The store was not at fault: on the real 12.1M
-point artifact the stored morton codes reproduce exactly from the sentinel bbox
under the reader's own formula, every row group decodes, and all 12,165,021 rows are
readable. The reader was losing them.
`readParquetRowGroupColumnExtent` took a row group's last value with
`readParquetRowGroup(..., { offset: rowCount - 1, limit: 1 })`. The vendored
parquet-wasm ignores `offset` on a row-group read and hands back the FIRST row again,
so every row group reported max === min — that it spanned a single code. Measured
against the parquet statistics: row group 1 reported (437752573, 437752573) for a
true span of (437752573, 724881652).
Nothing errored, because an understated max is still a valid answer to a monotone
predicate. The bisect asks "first row group whose max >= target", so it landed one
group late and never read the group CONTAINING the interval start. One viewport query
lost 11 of the 92 matching row groups — 187,990 points, 6% — in Z-order-shaped bands,
which is exactly what the holes were. Verified end to end: the same query now scans
every matching row group and returns 3,128,988 points, matching an independent
full-table scan (the residual 2 against pyarrow are float32/float64 boundary
comparisons, not losses).
The bound now comes from the sort order the format already guarantees: a row group's
values lie at or below the next group's first value. Conservative where equal codes
span a boundary, needs only the read that works, and halves the reads since each
first value is cached and shared with its neighbour. The last row group keeps an open
bound, which the bisect already reads as "may contain the target".
The regression test asserts an exact count for a bounded query — under-selection is
silent by construction and only a total can see it. Confirmed it fails against the
old behaviour (224 of 260 points).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A layer with `pointsTiling: 'auto'` on a Morton artifact now draws through `mortonTiledStrategy` — deck's TileLayer reading row groups for the viewport — instead of a memory-capped resident preload. Verified on a real 12.1M-point Xenium element: tiles load and pan, the layer frames itself, and the memory cap stops applying to it. The snapshot is what makes it drawable. A tiled entry plans no preload, so a preload resolution left sitting at `idle` reads as blocking forever — and auto-fit rides the isBlocking true->false transition, so the layer would never frame either. Instead the entry reports only the resources it HAS: no preload (absent, and `isBlocking` already skips a resource that is not there), plus world bounds derived from the artifact's own extent, which is the only thing that lets auto-fit run before a single tile has loaded. `blockingResources` grows to cover the probe, since until it answers we do not know which path the entry is on. The adapter's tiled resource is memoised on (element, metadata). That is not a nicety: the resource's loader is what TileLayer keys `getTileData` on, so a fresh identity per project() refetches every visible tile and turns a pan into a full reload. Tiling is per LAYER but the probe's answer is cached per ELEMENT, and reading the probe alone is wrong — found in the browser, not by a test: switching tiling off left the layer still drawing tiles while plan() went back to preloading, so it did both. Every consumer now combines the two (`isTiledFor` in core, `usesTiledPath` in vis). Known and deliberate, tracked in the plan doc: a tiled layer draws flat-coloured and ignores the feature filter (the tile scan does not return per-point codes yet — step 3); switching to tiling does not evict the preload already performed, so the panel's truncation notice still reports that resident memory over a tiled render; and the tiling control's status line reads the engine without subscribing, so it can lag the probe by a render. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three follow-ons from watching a tiled layer in the app, plus the sizing choice that made a real acquisition boundary look like a rendering fault. Releasing the resident window. A layer switched to tiling mid-session has usually already preloaded, and nothing gave those rows back: plan() stops ASKING for a preload, which is not the same as evicting one, so up to the full memory cap stayed held behind a render that never reads it. The probe's settle now drops the preload, its row-aligned codes and its feature-index scan — all three are defined against that window, so keeping them would leave state describing a batch that no longer exists. The catalog stays: it describes the element's features, not the window. Eviction runs once, on the settle, so a second layer reading the same element un-tiled re-requests it on its next plan pass rather than ping-ponging. The truncation notice. It read "4,000,000 of 12,165,021 points in memory — capped; raise the cap for more" directly above "the memory cap does not apply" — each true, the pair nonsense. A tiled layer draws from the viewport, so a resident count says nothing about what is on screen; the panel now says nothing instead. The tiling status line. It read engine.isTiled() directly, outside the engine subscription, so it kept insisting an element had no Morton index while tiling it — the probe settles asynchronously and nothing re-rendered the line. `tiled` now comes through usePointsFeatureState, which carries the subscription. Point sizing is one behaviour instead of two. Tiles sized in fixed pixels while the preloaded path used world units, so pointSize meant different things depending on a checkbox, and a zoomed-out tiled layer drew every one of millions of points as a fixed screen dot: density saturated to a flat mass and every tile seam and density edge hardened into an artefact. Both paths now size in world units with the model-matrix scale folded in, so points shrink as you zoom out and overdraw self-limits. Verified in the app: the capped notice is gone from a tiled layer and the status line is correct on first paint. The sizing change is pinned by unit tests on the layer props rather than pixels. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Viewport tiles now carry a feature code per point, so a tiled layer colours, takes per-feature overrides and answers Feature Highlight exactly like the preloaded path. Until now it drew flat, and the same element looked like two different datasets depending on a checkbox. The codes were already being read and discarded: the tile scan consults the feature column in order to filter on it, then returned bare coordinates. So the change is mostly about not throwing them away — `scanMortonTableInBounds` takes an optional Int32PointBuffer and appends to it in lockstep with the geometry, and the worker's tile-scan handler builds one and returns it. The worker boundary needed nothing: the protocol already declared an optional featureCodes on the columnar result and already transferred its buffer. One real gate had to move. `loadMortonPointsInBounds` projected the code column only when a filter was active, which is exactly backwards for colour: the no-filter "all features" view is the common case and was the one arriving without codes. It now projects whenever the artifact HAS a code column, and both the worker and main-thread returns carry the result. Short codes are dropped rather than padded, on both paths. A partial array would leave the tail reading code 0 — a VALID feature — and mis-colour it with conviction, which is worse than the flat fallback. Verified on the real 12.1M-point element: a tiled layer draws per-feature colours matching the preloaded path. Tests pin one code per point, every code a real catalog entry, and codes still returned (all equal to the selection) under a filter. The feature FILTER still does not narrow tiles — that is step 4 — so a tiled layer draws every feature in the viewport regardless of the selection. The panel now says so rather than claiming flat colour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A tiled layer drew every feature in the viewport whatever was selected. The selection now reaches getTileData — and its updateTriggers, so changing it refetches instead of serving the previous selection's cached tiles — and is applied INSIDE the row-group scan, so a tile arrives holding only what was asked for. On the real 12.1M-point element one gene takes a viewport tile from 3,128,988 points to 87,594. The composite's own filter machinery is untouched and needs to be: it is gated on the preloaded-columnar kind, so a tiled layer passes straight through rather than filtering twice. renderCap stays unset — a resident-window notion, and a tile is already bounded by its viewport. The catalog is now planned for a tiled entry. On the preloaded path it arrives free as a preview off the geometry decode; a tiled entry never decodes a resident batch, so nothing built one — and a selection persists as feature NAMES, which cannot become the codes the scan filters on without it. A saved config with a selection would draw every feature until someone happened to open the panel. A failed catalog is deliberately not re-planned: it reports undefined exactly as one that never ran, so a gate on the value alone re-emits the task on every reconcile forever. The feature-row panel needed a tiled case of its own. Every other signal it reads describes a resident batch a tiled layer does not have, so its rows fell through to "beyond the resident window; select it to fetch its points" — greyed, and wrong twice over: the points are available, and no feature-index scan is involved. I had this wrong in the plan and briefly in the code: the filter does NOT narrow I/O. Measured on the same query, unfiltered vs one gene — 92 row groups and 158.1MB either way, 3,128,988 points vs 87,594. Row groups are chosen SPATIALLY on a Morton artifact and a gene's points are spread across all of them, so no feature filter can skip one. The win is 36x fewer points leaving the worker, not less reading. Narrowing the fetch by feature needs a feature-primary index — the open index-selection question in ADR 0002/0003, and what the `*_feature_then_morton` permutations in the test store exist to explore. The plan now records the measurement rather than the expectation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…5 step 5) The sentinel rows are a claim a Parquet artifact makes about itself, and nothing else in the file forces them to be true. Believing a wrong one does not fail: the box is both the TileLayer extent and the domain mortonIntervalsForBounds normalises against, so a wrong one clips the tile grid — whole regions are never requested, no pending tile, nothing to wait for — and mis-maps every viewport to row groups. Points are never misplaced, because the reader re-filters to the query bounds, so the only symptom is that part of the map is missing. That is the third silent under-selection on this path after the zcover depth and the row-group bisect. The probe now recomputes morton_code_2d from x/y for a sample of real rows and refuses to tile unless a majority agree. That tests the invariant that matters — is this box the quantisation domain? — not a convention, so an artifact with a deliberately padded domain still tiles. Samples come from the middle of the file: a truncated box can agree with the true one near the origin by coincidence, never in the interior. A rejected element reports supportsRowGroupRangeReads: false with no bounds, the same pair the oversized-sentinel case already produced, so it degrades to the capped preload through the existing resolver gate — and warns, since a silent downgrade is what got us here. Found by way of the index-permutations store, whose *_feature elements recorded a box a quarter the size of their own domain: 0/320 sampled rows reproduced their codes from it, 320/320 from the true extent. Those files have been regenerated in place; the writer never had the bug, they were just older than it. Cost is one extra row-group read per element, cached with the metadata — 3 range reads / 0.37 MB / 414 ms becomes 4 / 2.16 MB / 523 ms on a 12.1M-point element, about one step of the bisect a single viewport query already runs eight of. The plan doc also records what the tile grid actually does, measured rather than assumed: minZoom/maxZoom -1 with tileSize 512 pins every tile at 1024 local units at every zoom, so the whole element is one fixed 11x4 grid that never subdivides. Zooming in gets no more detail and zooming out reads all 44 tiles unbudgeted. The pointsTiling default cannot flip until that is fixed, which is not the probe cost the open question assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Having a morton_code_2d column does not make a file Morton-ordered. A feature-primary artifact — sorted (feature, morton) — carries the identical column with identical, correct values, a correct sentinel box, and every field the tiling probe looks for. Only the order is wrong, and nothing in the file said so. The row-group bisect binary-searches that index assuming it ascends, so on such a file it landed arbitrarily and a tile came back holding whichever feature blocks happened to live in the row groups it picked: some tiles showed one or two genes, most showed none. That is what `transcripts_feature_then_morton` has been doing. The probe now reads the per-row-group [min, max] for the Morton column out of the parquet footer and requires it to be non-decreasing. On the permutations store that element descends at 185 of its 244 row-group boundaries; both morton-primary elements descend at none — INCLUDING transcripts_morton_then_feature, so a secondary feature key stays supported and the verdict is on the file rather than on the element's name. The store's index-manifest does say `experimental` for the feature-primary condition, but a reader cannot rely on a manifest; now the file answers for itself. The check is free: datasetMetadata.parts already carries the footer bytes when the probe runs, and the statistics are complete on all 245 row groups. Failing it also short-circuits the sentinel sampling read, since the outcome can no longer change, so a rejected element now costs less than it did before either guard. The element still loads through the capped preload, and the feature-code row-group index it exists to exercise lives on that path and is untouched. decodeUnsignedIntStat is new because morton_code_2d is uint32, which parquet stores as INT32 with a UINT_32 annotation: Morton codes use the top bit for real, so decodeIntStat reads the far corner of a slide as negative. Those statistics also retire a standing TODO. loadParquetRowGroupColumnExtent says it should be reading row-group statistics and instead range-reads and decodes each group twice for a few boundary values; they are right here, complete, and free. Noted against step 6, where a subdividing grid will issue many more bisects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…he data (D5 step 6)
Two things, and the first is what makes the second affordable.
**Row-group selection stops reading the file.** selectMortonRowGroups picks row
groups from the per-row-group [min, max] the tiling probe already parsed out of the
parquet footer. The bisect it replaces range-read the row group's BYTES — every
column, ~2MB on a real transcripts artifact — to recover two boundary values,
log2(rowGroups) steps per Morton interval, a few hundred intervals per query. That
is how a viewport query could pull most of a 439MB file to answer a question the
footer had already answered. On one 1024um tile of the 12.1M-point element, both
returning the same 643,961 points:
bisect 97 range reads / 175.12 MB / 2911 ms
footer index 32 range reads / 57.83 MB / 1035 ms
The remaining 32 reads are the row-group data itself. The new path is also stricter
— the bisect tested only `max` and assumed the groups tile the code space without
gaps, this intersects both ends — and the bisect stays as the fallback when
statistics will not parse, so it is an optimisation, not a new requirement.
**The grid comes from the artifact.** It was one fixed level, so every tile was 1024
local units at every zoom: zooming in read a 1024-unit tile to look at 50 units of
it, and 1024 came from deck's defaults rather than from the data. mortonTileGrid
derives both ends from the point density — finest stays at least one row group's
footprint, since below that four tiles fetch what one used to for the same bytes;
coarsest holds at most 400k rows, so one request is a fraction of the layer. And
zoomOffset = log2(modelMatrixScale) couples deck's z, chosen from a WORLD-space
zoom, to tile spans expressed in LOCAL units — the model matrix is exactly that
difference, and without it a tile lands at whatever size the transform implied.
For the Xenium element that is two levels, 1024um and 512um. The narrowness is the
finding, not a shortfall: the row-group size is the floor, and 50k-row groups put it
at ~402um. The old fixed 1024 was accidentally near-optimal for this file and would
not be for one an order of magnitude smaller or denser.
**The tile cache is budgeted in rows.** maxCacheSize now comes from a row budget
rather than deck's `5 x the selected tile count`, which on a coarse viewport of this
element could retain ~220 tiles / ~71M rows against a 4M resident cap; it is 16
tiles / ~5.2M rows, stated. That answers the plan's open question 3 at the level
ADR 0005 asks for — the second pool is accounted, not managed. Nothing evicts by
bytes, and a tile's real footprint is whatever its points weigh. maxRequests stays
at 6, now as a decision rather than an inheritance.
Verified in the app: at viewport zoom -7.8, 44 tiles at z -1; at -2.8, 6 tiles at
z 0, with the cache trimmed from 44 to exactly 16.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pointsTiling defaults to 'auto' (DEFAULT_POINTS_TILING), read everywhere through the new pointsTilingEnabled() rather than compared to 'auto' — the default has to mean the same thing to the resolver deciding what to load, the hook deciding what to render, and the panel drawing the checkbox, and three literal comparisons is how those drift apart. The argument for flipping is not that the probe is cheap, which is how the plan's open question framed it. It is that on a Morton artifact the capped preload is NOT a neutral alternative: it keeps the first `cap` rows in FILE order, and file order there is a prefix of the Z-curve, so the 4M-row default shows a spatially skewed chunk of the slide rather than a sample of it. Tiles read what is in view. What it costs, measured rather than assumed: at the default zoomed-out framing of the 12.1M-point element the tiled path loads all 44 tiles — 12,165,029 points / ~158 MB, the whole artifact — against a 4M-row prefix for the preload. First paint on a fully zoomed-out view is ~3x the rows, in exchange for a correct picture that streams in 44 pieces instead of blocking on one decode. Zooming OUT is the one direction viewport tiling does not help: there is no coarser representation to read. That wants a multi-resolution points pyramid, not a finer index, and it is now the loudest thing in the punch-list. `pointsTiling: 'off'` restores the old behaviour per layer, and the panel keeps the toggle precisely so that comparison can be made. An element that cannot be tiled pays one probe (4 range reads / ~2.16 MB, cached; on a non-Morton element it is footer metadata the preload reads anyway) and is otherwise untouched. The three guards decline loudly and fall through. The panel hides the memory-cap control on a tiled layer — it governs nothing there and sat directly above a line saying the cap does not apply. Twelve resolver tests changed, which is the honest cost of the flip: a fresh entry now plans ['tiling'] and defers the preload until the probe answers. The tests that were about preload/rowCodes mechanics pin pointsTiling: 'off' and say why; the default gets its own tests rather than being asserted incidentally forty times. Docs: D5 closed in the punch-list, per-element path table in points-preload-feature-filter-status, and the plan marked complete with the no-LOD gap and the byte-level cache accounting recorded as deliberately open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds Morton viewport tiling for point artifacts. The change validates Morton metadata, selects row groups from footer statistics, adds resolver fallback and lifecycle handling, renders viewport tiles, propagates feature data and filters, derives tile grids, and integrates tiled state and controls into visualization. ChangesMorton tiling core and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🟠 High · up to This PR enables tiled point loading by default, but the current implementation can silently drop points or select incorrect row groups for malformed or insufficient metadata, and can reuse stale tiling data when an element changes. These correctness risks, along with a reported type-check failure, should be fixed before merge. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 10
🧹 Nitpick comments (7)
docs/plans/points-morton-tiled-viewport-loading.md (1)
296-300: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language identifier to this fenced code block.
Use
textfor this console-style output. This resolves markdownlint MD040.🤖 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/plans/points-morton-tiled-viewport-loading.md` around lines 296 - 300, Update the fenced code block containing the extent and tile-loading output to declare the text language identifier, preserving the console output unchanged.Source: Linters/SAST tools
packages/core/src/models/VTableSource.ts (2)
1024-1036: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing
evictIfCurrenthelper.This class already has
evictIfCurrentat Line 649, which performs the same identity-checked delete forparquetDatasetMetadataCacheandparquetPartPathsCache. Lines 1024-1036 and Lines 1109-1119 reimplement it inline, so the eviction rule now lives in three places.
evictIfCurrentis typedMap<string, Promise<V>>, which matches both new caches.♻️ Proposed refactor for the extent cache site
pending .then((extent) => { - if (extent === null && this.rowGroupColumnExtentCache.get(cacheKey) === pending) { - this.rowGroupColumnExtentCache.delete(cacheKey); + if (extent === null) { + this.evictIfCurrent(this.rowGroupColumnExtentCache, cacheKey, pending); } }) .catch(() => { - if (this.rowGroupColumnExtentCache.get(cacheKey) === pending) { - this.rowGroupColumnExtentCache.delete(cacheKey); - } + this.evictIfCurrent(this.rowGroupColumnExtentCache, cacheKey, pending); });Apply the same change at Lines 1109-1119 with
rowGroupColumnFirstValueCache.🤖 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/models/VTableSource.ts` around lines 1024 - 1036, Reuse the existing evictIfCurrent helper for the rowGroupColumnExtentCache cleanup in the pending promise handlers, replacing the inline identity checks and deletes while preserving eviction only when the cached promise is the same. Apply the same refactor to the rowGroupColumnFirstValueCache cleanup at the corresponding site.
1040-1066: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the extent contract explicit
loadParquetRowGroupColumnExtentis public, but its documentation promises the current row group's first and last values. The implementation returns the first value and the next row group's first value, ornullfor the final group. The only production caller usesmaxas a conservative upper boundary. Rename the field or provide a separate internal boundary helper, and document the sorted-column precondition.🤖 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/models/VTableSource.ts` around lines 1040 - 1066, Update the public loadParquetRowGroupColumnExtent contract and implementation so it returns the current row group’s documented first and last values, rather than using the next group’s first value or an open final bound. Preserve the conservative upper-bound behavior needed by the production caller by adding a separate private boundary helper or renaming the internal result field, and document that the column must be sorted for these extents to be meaningful.packages/core/src/workers/pointsWorkerScan.ts (1)
468-472: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist narrowed locals so the two assertions are not needed.
collectCodesis a derived boolean, so TypeScript cannot use it to narrowinput.codesorfeatureCodeValues. That is why Lines 526 and 531 needas ArrayLike<number>andas Int32PointBuffer. Capturing both non-null values in locals before the loop expresses the same fact through narrowing, and it removes the pre-existing assertion on Line 500 too.The coding guidelines state: "Avoid type assertions (
as); usesatisfies,as const, discriminated unions, and small helpers that return precise types."♻️ Proposed refactor
- const collectCodes = input.codes !== undefined && featureCodeValues !== null; + // One narrowed pair, so neither the filter predicate nor the code push needs an + // assertion: both are non-null exactly when this local is set. + const codeReader = featureCodeValues !== null ? featureCodeValues : null; + const codesOut = codeReader !== null ? input.codes : undefined;Then use
codeReaderin the filter predicate and:- if (collectCodes) { - const code = (featureCodeValues as ArrayLike<number>)[rowIndex]; + if (codesOut && codeReader) { + const code = codeReader[rowIndex]; // A non-finite code is a real row with an unknown feature: keep the point and // record -1, the same "no feature" sentinel the shader's `featureCode >= 0` // guard already understands. Dropping the point instead would put a hole in // the geometry to express a gap in the colour. - (input.codes as Int32PointBuffer).push(Number.isFinite(code) ? code : -1); + codesOut.push(Number.isFinite(code) ? code : -1); }Also applies to: 525-532
🤖 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/workers/pointsWorkerScan.ts` around lines 468 - 472, In the points scan flow around collectCodes, hoist narrowed locals for the non-null input.codes and featureCodeValues values before the loop, and use those locals in the filter predicate and code collection logic. Remove the related type assertions, including the existing assertion near the initial code setup, while preserving the current behavior when either value is unavailable.Source: Coding guidelines
packages/core/src/engine/PointsResolver.ts (1)
184-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType
boundsTransformasMatrix4instead ofunknown.
tiledBoundsonly ever writes thetransform: Matrix4parameter into this field, andtransformAxisAlignedBoundsrequires aMatrix4.unknownis wider than the runtime value and hides that the memo key is a transform. The identity comparison works either way, so this is a type-precision change only.♻️ Proposed refactor
boundsSource?: PointsTilingMetadata; - boundsTransform?: unknown; + boundsTransform?: Matrix4;As per coding guidelines: "Prefer types that match runtime behavior" and "add explicit annotations at API boundaries or when inference is too wide".
🤖 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 184 - 188, Change the boundsTransform property in the tiled-entry metadata type to Matrix4, matching the transform parameter written by tiledBounds and consumed by transformAxisAlignedBounds; leave the existing memoization and identity comparison behavior unchanged.Source: Coding guidelines
packages/core/tests/pointsResolver.spec.ts (1)
812-835: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the duplicated
tiling()andstore()helpers.Four describe blocks each define a near-identical
tiling()metadata factory, and three of them redefinestore()— which already exists at Line 368. The only real variation is theboundsvalue in the step-2 block. A single module-level factory with an override parameter keeps the four blocks honest about what they actually vary.♻️ Sketch
+const tilingMetadata = (over: Partial<PointsTilingMetadata> = {}): PointsTilingMetadata => ({ + kind: 'morton-points', + parquetPath: 'points/transcripts/points.parquet', + axisNames: ['x', 'y'], + featureCodeColumnName: 'feature_name_codes', + mortonCodeColumnName: MORTON_CODE_2D_COLUMN, + totalRows: 12_000_000, + totalRowGroups: 96, + maxRowsPerGroup: 131_072, + supportsRowGroupRangeReads: true, + bounds: { minX: 0, minY: 0, maxX: 100, maxY: 100 }, + ...over, +});Then the step-2 block calls
tilingMetadata({ bounds: { minX: 10, minY: 20, maxX: 110, maxY: 220 } }), and each block reuses the sharedstore().Also applies to: 1041-1064, 1191-1204, 1352-1379
🤖 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/pointsResolver.spec.ts` around lines 812 - 835, Hoist the duplicated tiling metadata factory and store helper to module scope, reusing the existing store() definition near the earlier tests. Update each affected describe block to call the shared tiling() factory, passing the step-2 bounds through its override parameter while preserving the default metadata elsewhere.packages/layers/tests/pointsResourceIdentity.spec.ts (1)
348-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse typed fixtures instead of unchecked assertions.
Line 361 makes the metadata fixture
never, so TypeScript cannot validate its contract. TypeoverasPartial<PointsTilingMetadata>and return aPointsTilingMetadatavalue withsatisfies. If Line 374 must mock thePointsElementclass, keep the assertion at that boundary and add a short comment that explains why it is necessary.As per coding guidelines: “Avoid type assertions (
as); usesatisfies,as const, discriminated unions, and small helpers that return precise types.”🤖 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 348 - 374, Update the tilingMetadata fixture to type over as Partial<PointsTilingMetadata> and return an object validated with satisfies PointsTilingMetadata instead of asserting never, preserving the existing defaults and override behavior. Keep the tiledElement cast only at the PointsElement mock boundary if required, and add a brief comment explaining why that boundary assertion is necessary.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 @.changeset/points-tiling-metadata-probe.md:
- Around line 17-20: Update .changeset/points-tiling-metadata-probe.md lines
17-20 to describe the 'off' default and no tiled rendering as the behavior of
this step, while noting that a later step in the same release enables 'auto' and
tiled rendering. Update .changeset/points-morton-tiled-render.md lines 33-37 by
removing the known-gap statements for tiled feature colours and filtering,
preload eviction when switching to tiling, and resident-memory truncation
notices on tiled layers.
In `@packages/core/src/engine/PointsResolver.ts`:
- Around line 1490-1517: Update ensureTilingMetadata to return a resolved
promise when slot.isReady is true before calling slot.request, preventing
repeated settled probes from emitting loading/ready or releasing resident
batches. Do not guard on slot.isFailed so failed probes remain retryable.
In `@packages/core/src/models/VPointsSource.ts`:
- Around line 2389-2400: Update the Morton tiling guard around mortonExtents and
mortonRowGroupExtentsAreSorted so empty extents, null dataset metadata, and
all-null row-group statistics are treated as unverified rather than sorted.
Require positive evidence of usable extents and verified ordering before
enabling row-group range reads; otherwise keep tiling disabled. Split the
warning path so unverified statistics use an accurate message, while genuinely
unsorted extents retain the existing warning.
In `@packages/core/src/pointsTileGrid.ts`:
- Around line 119-123: Update the cache budget calculation near maxCacheSize to
use this module’s documented default resident-points memory cap when
input.cacheRowBudget is omitted, rather than defaulting to 0 and
FALLBACK_CACHE_TILES. Add coverage for the omitted-cacheRowBudget path,
verifying it produces the row-budgeted cache size based on the established
default-budget symbol.
In `@packages/core/src/pointsTiling.ts`:
- Around line 214-227: Update mortonRowGroupExtentsAreSorted to reject any
extent whose min or max is non-finite, or whose min exceeds max, before
evaluating cross-group ordering. Preserve the existing ordering validation for
valid extents, and add coverage for a single inverted extent so tiled selection
falls back instead of omitting points.
In `@packages/core/tests/mortonPointsTiling.spec.ts`:
- Around line 400-405: Add an explicit non-empty assertion for the filtered
result before deriving or iterating over pointCount in the filtered-codes test.
Keep the existing featureCodes length and per-item wanted-value checks, ensuring
the test fails when the filtered tile query returns zero points.
In `@packages/layers/src/engine/PointsDataEngine.ts`:
- Around line 170-173: Update getTiledResource to track the PointsElement
identity associated with each key and compare it before reusing resolver tiling
metadata. When the element differs from the previously recorded element for that
key, invalidate or re-probe the metadata before calling
adapter.getTiledResource, while preserving the existing null handling and
same-element reuse path.
In `@packages/layers/tests/pointsRenderStrategies.spec.ts`:
- Around line 183-190: Strengthen the unfiltered-view test by asserting that
calls contains exactly one entry before checking calls[0]?.featureCodes. Update
the test case containing getTileData and retain the existing undefined
featureCodes assertion.
In `@packages/layers/tests/pointsScatterSizing.spec.ts`:
- Around line 60-83: Add color and opacity values to the style props passed to
renderColumnarScatterLayer in both the direct options object and the shared
common object, preserving the existing sizing assertions and tile comparison
behavior.
In `@packages/vis/src/SpatialCanvas/useLayerData.ts`:
- Around line 545-562: Update the global progress aggregation near the
tileDebugStoresRef usage to include only currently visible layers whose elements
still satisfy usesTiledPath(...), rather than every retained store. Remove or
replace stores when their layer or element becomes inactive, including when
pointsTiling is switched to off during an in-flight request, and add a
regression test covering that transition so stale loadingTileIds cannot keep
isLoading true.
---
Nitpick comments:
In `@docs/plans/points-morton-tiled-viewport-loading.md`:
- Around line 296-300: Update the fenced code block containing the extent and
tile-loading output to declare the text language identifier, preserving the
console output unchanged.
In `@packages/core/src/engine/PointsResolver.ts`:
- Around line 184-188: Change the boundsTransform property in the tiled-entry
metadata type to Matrix4, matching the transform parameter written by
tiledBounds and consumed by transformAxisAlignedBounds; leave the existing
memoization and identity comparison behavior unchanged.
In `@packages/core/src/models/VTableSource.ts`:
- Around line 1024-1036: Reuse the existing evictIfCurrent helper for the
rowGroupColumnExtentCache cleanup in the pending promise handlers, replacing the
inline identity checks and deletes while preserving eviction only when the
cached promise is the same. Apply the same refactor to the
rowGroupColumnFirstValueCache cleanup at the corresponding site.
- Around line 1040-1066: Update the public loadParquetRowGroupColumnExtent
contract and implementation so it returns the current row group’s documented
first and last values, rather than using the next group’s first value or an open
final bound. Preserve the conservative upper-bound behavior needed by the
production caller by adding a separate private boundary helper or renaming the
internal result field, and document that the column must be sorted for these
extents to be meaningful.
In `@packages/core/src/workers/pointsWorkerScan.ts`:
- Around line 468-472: In the points scan flow around collectCodes, hoist
narrowed locals for the non-null input.codes and featureCodeValues values before
the loop, and use those locals in the filter predicate and code collection
logic. Remove the related type assertions, including the existing assertion near
the initial code setup, while preserving the current behavior when either value
is unavailable.
In `@packages/core/tests/pointsResolver.spec.ts`:
- Around line 812-835: Hoist the duplicated tiling metadata factory and store
helper to module scope, reusing the existing store() definition near the earlier
tests. Update each affected describe block to call the shared tiling() factory,
passing the step-2 bounds through its override parameter while preserving the
default metadata elsewhere.
In `@packages/layers/tests/pointsResourceIdentity.spec.ts`:
- Around line 348-374: Update the tilingMetadata fixture to type over as
Partial<PointsTilingMetadata> and return an object validated with satisfies
PointsTilingMetadata instead of asserting never, preserving the existing
defaults and override behavior. Keep the tiledElement cast only at the
PointsElement mock boundary if required, and add a brief comment explaining why
that boundary assertion is necessary.
🪄 Autofix
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 Plus
Run ID: 0c9ce0d1-9e3f-4557-9bfd-bc708ae466a5
📒 Files selected for processing (51)
.changeset/morton-bisect-cost.md.changeset/morton-rowgroup-extent-max.md.changeset/points-morton-sentinel-guard.md.changeset/points-morton-sort-guard.md.changeset/points-morton-tile-grid.md.changeset/points-morton-tiled-render.md.changeset/points-morton-tiling-default.md.changeset/points-tiled-coherence.md.changeset/points-tiled-feature-colours.md.changeset/points-tiled-feature-filter.md.changeset/points-tiling-metadata-probe.mddocs/plans/points-morton-tiled-viewport-loading.mddocs/plans/points-preload-feature-filter-status.mddocs/plans/points-redesign-punchlist.mdpackages/core/src/engine/PointsResolver.tspackages/core/src/index.tspackages/core/src/models/VPointsSource.tspackages/core/src/models/VTableSource.tspackages/core/src/parquetFooterStats.tspackages/core/src/pointsLoadPlan.tspackages/core/src/pointsLoader.tspackages/core/src/pointsTileGrid.tspackages/core/src/pointsTiling.tspackages/core/src/spatialViewFit.tspackages/core/src/workers/points-worker.tspackages/core/src/workers/pointsWorkerScan.tspackages/core/tests/mortonPointsTiling.spec.tspackages/core/tests/pointsFeatureTallySentinels.spec.tspackages/core/tests/pointsResolver.spec.tspackages/core/tests/pointsRowCodesCapAlignment.spec.tspackages/core/tests/pointsTileGrid.spec.tspackages/core/tests/pointsTiling.spec.tspackages/core/tests/vtableRangeProbeCache.spec.tspackages/layers/src/adapters/PointsRendererAdapter.tspackages/layers/src/engine/PointsDataEngine.tspackages/layers/src/mortonTiledStrategy.tspackages/layers/src/pointsLoadPlan.tspackages/layers/src/pointsLoader.tspackages/layers/src/pointsScatterLayer.tspackages/layers/tests/pointsRenderStrategies.spec.tspackages/layers/tests/pointsResourceIdentity.spec.tspackages/layers/tests/pointsScatterSizing.spec.tspackages/vis/src/SpatialCanvas/PointsFeatureFilterPanel.tsxpackages/vis/src/SpatialCanvas/PointsFeatureState.tsxpackages/vis/src/SpatialCanvas/PointsLayerPanel.tsxpackages/vis/src/SpatialCanvas/featureRowState.tspackages/vis/src/SpatialCanvas/pointsTileProgress.tspackages/vis/src/SpatialCanvas/types.tspackages/vis/src/SpatialCanvas/useLayerData.tspackages/vis/tests/pointsFeatureRowState.spec.tspackages/vis/tests/pointsTileProgress.spec.ts
Comment on lines
+17
to
+20
| This is **opt-in and inert by default**: the new `PointsResolveConfig.pointsTiling` | ||
| defaults to `'off'`, which collapses planning to exactly today's behaviour. Nothing | ||
| renders through the tiled path yet — that is the next step of | ||
| `docs/plans/points-morton-tiled-viewport-loading.md`. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Step-scoped changesets state facts that later steps of this PR invalidate. Both files describe the state of one intermediate step, but changesets are concatenated into one published release entry, so a reader gets claims that contradict the merged behaviour.
.changeset/points-tiling-metadata-probe.md#L17-L20: restate the'off'default and "nothing renders through the tiled path" as the behaviour at this step, and note that a later step in the same release enables'auto'and tiled rendering..changeset/points-morton-tiled-render.md#L33-L37: remove the three "known gaps" that this PR closes — tiled feature colours and filtering, preload eviction on switching to tiling, and the resident-memory truncation notice on a tiled layer.
📍 Affects 2 files
.changeset/points-tiling-metadata-probe.md#L17-L20(this comment).changeset/points-morton-tiled-render.md#L33-L37
🤖 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 @.changeset/points-tiling-metadata-probe.md around lines 17 - 20, Update
.changeset/points-tiling-metadata-probe.md lines 17-20 to describe the 'off'
default and no tiled rendering as the behavior of this step, while noting that a
later step in the same release enables 'auto' and tiled rendering. Update
.changeset/points-morton-tiled-render.md lines 33-37 by removing the known-gap
statements for tiled feature colours and filtering, preload eviction when
switching to tiling, and resident-memory truncation notices on tiled layers.
Comment on lines
+1490
to
+1517
| ensureTilingMetadata(target: PointsLoadTarget): Promise<void> { | ||
| const { key, layerId, element } = target; | ||
| const slot = this.ensureEntry(key).tiling; | ||
| const before = slot.pending; | ||
| const loading = slot.request('probe', async () => { | ||
| const metadata = await element.getPointsTilingMetadata(); | ||
| if (!metadata?.supportsRowGroupRangeReads || !metadata.bounds) { | ||
| return null; | ||
| } | ||
| return metadata; | ||
| }); | ||
|
|
||
| // Same contract as the preload's: 'loading' only when a NEW request starts (not | ||
| // on a dedup). The only terminal state the probe owns is a *tileable* answer — | ||
| // that entry is now drawable. Every other outcome hands off to the preload, which | ||
| // reports its own; claiming 'ready' here would clear the spinner while the real | ||
| // geometry load had not started. | ||
| if (loading !== before) { | ||
| this.callbacks.onStatus?.(layerId, 'loading'); | ||
| void loading.then(() => { | ||
| if (this.isTiled(key)) { | ||
| this.releaseResidentBatch(key); | ||
| this.callbacks.onStatus?.(layerId, 'ready'); | ||
| } | ||
| }); | ||
| } | ||
| return loading; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm RequestSlot.pending semantics and locate repeat ensureTilingMetadata callers.
set -euo pipefail
# The slot's pending/isReady/isFailed getters and the already-satisfied return in request().
ast-grep outline packages/core/src/engine/RequestSlot.ts --items all
rg -nP -C4 'get (pending|isReady|isFailed|readyKey)\b' packages/core/src/engine/RequestSlot.ts
# Every call site that can invoke the probe again after it settled.
rg -nP -C6 '\bensureTilingMetadata\s*\(' --type=tsRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 1813
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- RequestSlot implementation ---'
sed -n '100,245p' packages/core/src/engine/RequestSlot.ts
printf '%s\n' '--- ensureTilingMetadata declarations and calls ---'
rg -n -P -C8 'ensureTilingMetadata' --glob '*.ts' --glob '*.tsx' .
printf '%s\n' '--- status and preload interactions ---'
rg -n -P -C6 'releaseResidentBatch|isTiled|ensureLoaded|tiling\.request|onStatus' packages/core/src/engine/PointsResolver.tsRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 46904
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repeat-probe and shared-preload tests ---'
sed -n '980,1025p' packages/core/tests/pointsResolver.spec.ts
sed -n '1234,1260p' packages/core/tests/pointsResolver.spec.ts
printf '%s\n' '--- ensureLoaded status guard ---'
sed -n '935,1060p' packages/core/src/engine/PointsResolver.ts
printf '%s\n' '--- tiling release implementation and entry lifecycle ---'
sed -n '1518,1565p' packages/core/src/engine/PointsResolver.ts
rg -n -P -C5 'ensureEntry\(|entries\.|new Map|releaseResidentBatch' packages/core/src/engine/PointsResolver.ts | head -160Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 19312
Guard settled tiling probes before requesting them again. A ready RequestSlot returns a new resolved promise while pending is undefined, so repeated calls re-emit loading and ready and can reset a resident preload through releaseResidentBatch. Add if (slot.isReady) return Promise.resolve(); before slot.request(...). Do not include slot.isFailed; failed probes must remain retryable.
🤖 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 1490 - 1517, Update
ensureTilingMetadata to return a resolved promise when slot.isReady is true
before calling slot.request, preventing repeated settled probes from emitting
loading/ready or releasing resident batches. Do not guard on slot.isFailed so
failed probes remain retryable.
Comment on lines
+2389
to
+2400
| const mortonExtents = datasetMetadata | ||
| ? rowGroupMortonExtents(datasetMetadata.parts, datasetMetadata.totalNumRowGroups) | ||
| : []; | ||
| const rowGroupsAreSorted = mortonRowGroupExtentsAreSorted(mortonExtents); | ||
| if (!rowGroupsAreSorted) { | ||
| console.warn( | ||
| `Morton tiling disabled for ${elementPath}: its morton_code_2d column is not ` + | ||
| 'sorted across row groups, so it is indexed by something else (a ' + | ||
| 'feature-primary artifact carries the same column unsorted). The row-group ' + | ||
| 'bisect needs a sorted index; falling back to the capped preload.' | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
"No statistics" is read as "sorted", so the guard can pass on zero evidence.
mortonRowGroupExtentsAreSorted skips null extents and returns true for an empty list (packages/core/src/pointsTiling.ts Lines 214-227). Two inputs therefore produce a sorted verdict without any evidence of order:
datasetMetadataisnull, orrowGroupMortonExtentsreturns[]because a footer failed to parse or the row-group count disagreed. ThenmortonExtentsis[].- The Morton column is present in the footer but carries no statistics. Then every entry is
nulland the array still has the expected length.
In both cases rowGroupsAreSorted becomes true, so a feature-primary artifact passes the guard and supportsRowGroupRangeReads can be set. selectRowGroupsForIntervals then falls back to bisectRowGroupsRight, which is exactly the search this changeset added the guard to prevent. The all-null case is worse in one respect: the extents array is stored on the metadata, and selectMortonRowGroups includes every row group whose extent is null, so every tile scans the whole file.
The sort check is a correctness gate, not an optimisation, so it needs a third state. Treat "no usable statistics" as "cannot verify" and keep tiling off, or verify order by another means before enabling it.
🛡️ Proposed fix: require positive evidence of order
const mortonExtents = datasetMetadata
? rowGroupMortonExtents(datasetMetadata.parts, datasetMetadata.totalNumRowGroups)
: [];
- const rowGroupsAreSorted = mortonRowGroupExtentsAreSorted(mortonExtents);
+ // An empty list, or a list of all-null entries, is "unverified" — not "sorted".
+ // `mortonRowGroupExtentsAreSorted` skips nulls, so it cannot make that
+ // distinction on its own.
+ const hasMortonStatistics = mortonExtents.some((extent) => extent !== null);
+ const rowGroupsAreSorted =
+ hasMortonStatistics && mortonRowGroupExtentsAreSorted(mortonExtents);
if (!rowGroupsAreSorted) {The existing warning text then needs a branch for the unverified case, since "is not sorted across row groups" is not what happened.
🤖 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/models/VPointsSource.ts` around lines 2389 - 2400, Update
the Morton tiling guard around mortonExtents and mortonRowGroupExtentsAreSorted
so empty extents, null dataset metadata, and all-null row-group statistics are
treated as unverified rather than sorted. Require positive evidence of usable
extents and verified ordering before enabling row-group range reads; otherwise
keep tiling disabled. Split the warning path so unverified statistics use an
accurate message, while genuinely unsorted extents retain the existing warning.
Comment on lines
+119
to
+123
| const budget = input.cacheRowBudget ?? 0; | ||
| const maxCacheSize = | ||
| budget > 0 && estimatedRowsPerTile > 0 | ||
| ? Math.min(512, Math.max(16, Math.round(budget / estimatedRowsPerTile))) | ||
| : FALLBACK_CACHE_TILES; |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Apply the documented default cache row budget.
When cacheRowBudget is omitted, this code uses 0 and returns FALLBACK_CACHE_TILES. The public input contract says the resident points memory cap is the default. Direct callers then get a tile-count cache instead of a row-budgeted cache.
Set the default budget in this module, and add a test for an omitted cacheRowBudget.
Proposed fix
- const budget = input.cacheRowBudget ?? 0;
+ const budget = input.cacheRowBudget ?? DEFAULT_POINTS_MEMORY_CAP;🤖 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/pointsTileGrid.ts` around lines 119 - 123, Update the cache
budget calculation near maxCacheSize to use this module’s documented default
resident-points memory cap when input.cacheRowBudget is omitted, rather than
defaulting to 0 and FALLBACK_CACHE_TILES. Add coverage for the
omitted-cacheRowBudget path, verifying it produces the row-budgeted cache size
based on the established default-budget symbol.
Comment on lines
+214
to
+227
| export function mortonRowGroupExtentsAreSorted(extents: readonly MortonRowGroupExtent[]): boolean { | ||
| let previousMax: number | null = null; | ||
| for (const extent of extents) { | ||
| if (!extent) { | ||
| continue; | ||
| } | ||
| const [min, max] = extent; | ||
| if (previousMax !== null && min < previousMax) { | ||
| return false; | ||
| } | ||
| previousMax = previousMax === null ? max : Math.max(previousMax, max); | ||
| } | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject invalid row-group ranges before enabling tiled selection.
An extent with min > max passes this check when it is the first or only known extent. selectMortonRowGroups then treats that invalid range as non-intersecting and can omit the row group. The tiled path silently drops points instead of falling back to the bisect path.
Reject non-finite and inverted extents before checking cross-group ordering. Add coverage for a single inverted extent.
Proposed fix
}
const [min, max] = extent;
+ if (!Number.isFinite(min) || !Number.isFinite(max) || min > max) {
+ return false;
+ }
if (previousMax !== null && min < previousMax) {
return false;
}📝 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
| export function mortonRowGroupExtentsAreSorted(extents: readonly MortonRowGroupExtent[]): boolean { | |
| let previousMax: number | null = null; | |
| for (const extent of extents) { | |
| if (!extent) { | |
| continue; | |
| } | |
| const [min, max] = extent; | |
| if (previousMax !== null && min < previousMax) { | |
| return false; | |
| } | |
| previousMax = previousMax === null ? max : Math.max(previousMax, max); | |
| } | |
| return true; | |
| } | |
| export function mortonRowGroupExtentsAreSorted(extents: readonly MortonRowGroupExtent[]): boolean { | |
| let previousMax: number | null = null; | |
| for (const extent of extents) { | |
| if (!extent) { | |
| continue; | |
| } | |
| const [min, max] = extent; | |
| if (!Number.isFinite(min) || !Number.isFinite(max) || min > max) { | |
| return false; | |
| } | |
| if (previousMax !== null && min < previousMax) { | |
| return false; | |
| } | |
| previousMax = previousMax === null ? max : Math.max(previousMax, max); | |
| } | |
| return true; | |
| } |
🤖 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/pointsTiling.ts` around lines 214 - 227, Update
mortonRowGroupExtentsAreSorted to reject any extent whose min or max is
non-finite, or whose min exceeds max, before evaluating cross-group ordering.
Preserve the existing ordering validation for valid extents, and add coverage
for a single inverted extent so tiled selection falls back instead of omitting
points.
Comment on lines
+400
to
+405
| const pointCount = result.shape[1] ?? 0; | ||
|
|
||
| expect(result.featureCodes?.length).toBe(pointCount); | ||
| for (let i = 0; i < pointCount; i += 1) { | ||
| expect(result.featureCodes?.[i]).toBe(wanted); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The filtered-codes test passes when the filter returns nothing.
pointCount is read from the result, and both assertions are driven by it. If a regression made the filtered tile query return zero points, result.featureCodes?.length would be 0, pointCount would be 0, and the loop body would never run. The test would pass.
A filter that returns nothing is a documented failure mode for this scan path, so add the non-empty assertion.
💚 Proposed fix
const pointCount = result.shape[1] ?? 0;
+ // Without this the two assertions below hold trivially for an empty result,
+ // which is exactly the fail-closed regression worth catching.
+ expect(pointCount).toBeGreaterThan(0);
expect(result.featureCodes?.length).toBe(pointCount);📝 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
| const pointCount = result.shape[1] ?? 0; | |
| expect(result.featureCodes?.length).toBe(pointCount); | |
| for (let i = 0; i < pointCount; i += 1) { | |
| expect(result.featureCodes?.[i]).toBe(wanted); | |
| } | |
| const pointCount = result.shape[1] ?? 0; | |
| // Without this the two assertions below hold trivially for an empty result, | |
| // which is exactly the fail-closed regression worth catching. | |
| expect(pointCount).toBeGreaterThan(0); | |
| expect(result.featureCodes?.length).toBe(pointCount); | |
| for (let i = 0; i < pointCount; i += 1) { | |
| expect(result.featureCodes?.[i]).toBe(wanted); | |
| } |
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/mortonPointsTiling.spec.ts` around lines 400 - 405, Add
an explicit non-empty assertion for the filtered result before deriving or
iterating over pointCount in the filtered-codes test. Keep the existing
featureCodes length and per-item wanted-value checks, ensuring the test fails
when the filtered tile query returns zero points.
Comment on lines
+170
to
+173
| getTiledResource(element: PointsElement, key: string): PointsRenderResource | null { | ||
| const metadata = this.resolver.getTilingMetadata(key); | ||
| if (!metadata) return null; | ||
| return this.adapter.getTiledResource(element, key, metadata); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/core/src/engine/PointsResolver.ts --items all --match 'PointsResolver|TilingProbeKey'
rg -n -C 8 'TilingProbeKey|ensureTilingMetadata|entry\.tiling|PointsElement|target\.element' \
packages/core/src/engine/PointsResolver.ts \
packages/vis/src/SpatialCanvas/useLayerData.tsRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 27580
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- PointsResolver entry, planning, and tiling paths ---'
sed -n '130,205p' packages/core/src/engine/PointsResolver.ts
sed -n '225,340p' packages/core/src/engine/PointsResolver.ts
sed -n '335,445p' packages/core/src/engine/PointsResolver.ts
sed -n '1480,1545p' packages/core/src/engine/PointsResolver.ts
printf '%s\n' '--- RequestSlot implementation and resolver lifecycle callers ---'
rg -n -C 12 'class RequestSlot|interface RequestSlot|new PointsResolver|\.plan\(|resolvePointsTarget|getTiledResource\(' packages/core packages/layers packages/visRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Tiling getters and identity handling in PointsResolver ---'
rg -n -C 10 'getTilingMetadata|isTilingSettled|isTiledFor|ensureEntry\(|element' \
packages/core/src/engine/PointsResolver.ts
printf '%s\n' '--- ResolveContext lifecycle and element replacement handling ---'
rg -n -C 10 'ResolveContext|elementKey|ctx\.element|resolver\.evict|\.evict\(' \
packages/vis/src/SpatialCanvas packages/core/src/engine \
-g '*.ts' -g '*.tsx' | head -n 500
printf '%s\n' '--- RequestSlot request/reset behavior ---'
sed -n '90,260p' packages/core/src/engine/RequestSlot.tsRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 50387
Invalidate tiling metadata when the PointsElement changes.
PointsResolver keys the tiling slot by key and does not track PointsElement identity. A same-key replacement can reuse the previous element’s bounds and artifact metadata. Compare element identity and re-probe on mismatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/layers/src/engine/PointsDataEngine.ts` around lines 170 - 173,
Update getTiledResource to track the PointsElement identity associated with each
key and compare it before reusing resolver tiling metadata. When the element
differs from the previously recorded element for that key, invalidate or
re-probe the metadata before calling adapter.getTiledResource, while preserving
the existing null handling and same-element reuse path.
Source: Learnings
Comment on lines
+183
to
+190
| it('passes no codes at all for the unfiltered view', async () => { | ||
| // `undefined` means "no filter" to the scan; `[]` would mean "match nothing". | ||
| const { layer, calls } = tiledLayer({}); | ||
|
|
||
| await tileLayerOf(layer).props.getTileData(tileProps); | ||
|
|
||
| expect(calls[0]?.featureCodes).toBeUndefined(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This assertion passes when loadInBounds is never called.
calls[0]?.featureCodes is undefined both when the scan receives no filter and when no scan ran at all. A regression that stops getTileData from reaching loadInBounds would keep this test green. The first test guards against that with toHaveLength(1); add the same guard here.
💚 Proposed fix
await tileLayerOf(layer).props.getTileData(tileProps);
+ expect(calls).toHaveLength(1);
expect(calls[0]?.featureCodes).toBeUndefined();📝 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
| it('passes no codes at all for the unfiltered view', async () => { | |
| // `undefined` means "no filter" to the scan; `[]` would mean "match nothing". | |
| const { layer, calls } = tiledLayer({}); | |
| await tileLayerOf(layer).props.getTileData(tileProps); | |
| expect(calls[0]?.featureCodes).toBeUndefined(); | |
| }); | |
| it('passes no codes at all for the unfiltered view', async () => { | |
| // `undefined` means "no filter" to the scan; `[]` would mean "match nothing". | |
| const { layer, calls } = tiledLayer({}); | |
| await tileLayerOf(layer).props.getTileData(tileProps); | |
| expect(calls).toHaveLength(1); | |
| expect(calls[0]?.featureCodes).toBeUndefined(); | |
| }); |
🤖 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/pointsRenderStrategies.spec.ts` around lines 183 - 190,
Strengthen the unfiltered-view test by asserting that calls contains exactly one
entry before checking calls[0]?.featureCodes. Update the test case containing
getTileData and retain the existing undefined featureCodes assertion.
Comment on lines
+60
to
+83
| const batch = { | ||
| format: 'columnar-ndarray' as const, | ||
| shape: [2, 3], | ||
| data: [new Float32Array([0, 1, 2]), new Float32Array([0, 1, 2])], | ||
| pointCount: 3, | ||
| }; | ||
|
|
||
| it('sizes in world units and folds in the model-matrix scale', () => { | ||
| const layer = renderColumnarScatterLayer('scatter', batch, { | ||
| pointSize: 2, | ||
| modelMatrix: new Matrix4().scale([4, 4, 1]), | ||
| }); | ||
|
|
||
| expect(layer.props.radiusUnits).toBe('common'); | ||
| expect(layer.props.radiusScale).toBeCloseTo(8, 9); | ||
| }); | ||
|
|
||
| it('does not change because the batch came from a tile', () => { | ||
| const common = { pointSize: 2, modelMatrix: new Matrix4().scale([4, 4, 1]) }; | ||
| const plain = renderColumnarScatterLayer('plain', batch, common); | ||
| const tiled = renderColumnarScatterLayer('tiled', batch, { | ||
| ...common, | ||
| tileBounds: [0, 0, 10, 10], | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare the test literals against the declared prop and batch types.
set -euo pipefail
rg -nP -A22 'interface PointsScatterStyleProps' packages/layers/src/pointsScatterLayer.ts
rg -nP -B2 -A20 '(interface|type)\s+ColumnarNdarrayPointsBatch\b' --type=ts
# Type-check the layers package if a script exists.
rg -nP '"(typecheck|check-types|tsc)"' packages/layers/package.json || trueRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 1231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test file ---'
cat -n packages/layers/tests/pointsScatterSizing.spec.ts | sed -n '1,130p'
printf '%s\n' '--- scatter helper and prop usage ---'
rg -n -A45 -B10 'renderColumnarScatterLayer|PointsScatterStyleProps' packages/layers/src packages/layers/tests --type=ts --type=tsx
printf '%s\n' '--- batch declarations and related types ---'
rg -n -A30 -B10 'ColumnarNdarrayPointsBatch|columnar-ndarray' packages --type=ts --type=tsx
printf '%s\n' '--- package scripts and TypeScript configuration ---'
cat packages/layers/package.json
find . -maxdepth 3 -name 'tsconfig*.json' -printRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 4423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- helper implementation and prop type ---'
rg -n -A55 -B12 'renderColumnarScatterLayer|PointsScatterStyleProps' packages/layers/src packages/layers/tests -g '*.ts' -g '*.tsx'
printf '%s\n' '--- batch declarations ---'
rg -n -A35 -B12 'ColumnarNdarrayPointsBatch' packages -g '*.ts' -g '*.tsx' || true
rg -n -A25 -B8 "format: 'columnar-ndarray'" packages -g '*.ts' -g '*.tsx'
printf '%s\n' '--- package scripts ---'
cat packages/layers/package.jsonRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
source = Path("packages/layers/src/pointsScatterLayer.ts").read_text()
test = Path("packages/layers/tests/pointsScatterSizing.spec.ts").read_text()
loader = Path("packages/layers/src/pointsLoader.ts").read_text()
props = dict(re.findall(r'^\s*(\w+)(\?)?:\s*([^;]+);', source[source.index("export interface PointsScatterStyleProps"):source.index("}\n\n// One shared extension")], re.M))
required = {name for name, (optional, _type) in props.items() if not optional}
print("required PointsScatterStyleProps members:", sorted(required))
calls = re.findall(r'renderColumnarScatterLayer\([^,]+,\s*batch,\s*\{(.*?)\}\)', test, re.S)
for i, body in enumerate(calls, 1):
supplied = set(re.findall(r'^\s*(\w+)\s*:', body, re.M))
print(f"batch call {i}: missing required members =", sorted(required - supplied))
common = re.search(r'const common = \{(.*?)\};', test, re.S).group(1)
common_supplied = set(re.findall(r'^\s*(\w+)\s*:', common, re.M))
print("common call object members:", sorted(common_supplied))
print("common object missing required members:", sorted(required - common_supplied))
batch = test[test.index("const batch = {"):test.index(" };\n\n it", test.index("const batch = {"))]
batch_members = set(re.findall(r'^\s*(\w+)\s*:', batch, re.M))
decl = loader[loader.index("export interface ColumnarNdarrayPointsBatch"):loader.index("}\n\n/** Placeholder")]
declared = set(re.findall(r'^\s*(\w+)\??:', decl, re.M))
print("batch literal members:", sorted(batch_members))
print("batch declaration members:", sorted(declared))
print("batch has all required declaration members:", {"format", "data", "shape"} <= batch_members)
PY
printf '%s\n' '--- package scripts ---'
node -e 'const p=require("./packages/layers/package.json"); console.log(JSON.stringify(p.scripts ?? {}, null, 2))'Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 320
Add color and opacity to the test style props.
PointsScatterStyleProps requires both fields, but the direct props object and common omit them. The batch literal satisfies ColumnarNdarrayPointsBatch; only the style props cause the type error.
🤖 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/pointsScatterSizing.spec.ts` around lines 60 - 83, Add
color and opacity values to the style props passed to renderColumnarScatterLayer
in both the direct options object and the shared common object, preserving the
existing sizing assertions and tile comparison behavior.
Comment on lines
+545
to
+562
| // Per-layer tile-status stores for the Morton path (D5). One per points layer, | ||
| // created on first render of a tiled layer and kept for its lifetime — the store is | ||
| // a `PointsLayer` prop, so a fresh one per render would churn the debug overlay. | ||
| // Its `update` no-ops when the state signature is unchanged, so the notify below | ||
| // fires on real tile transitions only, not on every deck frame. | ||
| const tileDebugStoresRef = useRef(new Map<string, TileDebugStore>()); | ||
| const getTileDebugStore = useCallback( | ||
| (layerId: string): TileDebugStore => { | ||
| const stores = tileDebugStoresRef.current; | ||
| let store = stores.get(layerId); | ||
| if (!store) { | ||
| store = createTileDebugStore(notifyLoadedDataChanged); | ||
| stores.set(layerId, store); | ||
| } | ||
| return store; | ||
| }, | ||
| [notifyLoadedDataChanged] | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exclude inactive tile-debug stores from global progress.
tileDebugStoresRef is append-only, but Lines 1864-1866 aggregate every retained store. If a layer is hidden, removed, or switched to pointsTiling: 'off' during a tile request, its old loadingTileIds can keep global isLoading true.
Aggregate only visible layers that still satisfy usesTiledPath(...). Remove or replace stores when their layer or element becomes inactive. Add a regression test for disabling tiling during an in-flight tile load.
Also applies to: 1858-1867
🤖 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/useLayerData.ts` around lines 545 - 562,
Update the global progress aggregation near the tileDebugStoresRef usage to
include only currently visible layers whose elements still satisfy
usesTiledPath(...), rather than every retained store. Remove or replace stores
when their layer or element becomes inactive, including when pointsTiling is
switched to off during an in-flight request, and add a regression test covering
that transition so stale loadingTileIds cannot keep isLoading true.
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.
Lights up the
morton-tiledpoints encoding end to end and turns it on by default, closing D5 in the points punch-list. Plan and full rationale:docs/plans/points-morton-tiled-viewport-loading.md.A Morton-sorted points element is now probed, framed from its own extent, and drawn from viewport tiles that colour by feature, honour the feature filter inside the row-group scan, and subdivide with zoom — instead of a capped resident preload of the first N rows in file order.
Why it is on by default
The preload it replaces is not a neutral alternative on a Morton artifact. It keeps the first
caprows in file order, and file order there is a prefix of the Z-curve — so the 4M-row default shows a spatially skewed chunk of the slide rather than a sample of it. Tiles read what is actually in view.pointsTilingdefaults to'auto', resolved throughpointsTilingEnabled(...)rather than compared to'auto'soundefinedmeans the same thing to the resolver, the render hook and the panel.What it costs — measured, not assumed
At the default zoomed-out framing of a 12.1M-point Xenium element:
First paint on a fully zoomed-out view is ~3x the rows, in exchange for a correct picture that streams in 44 pieces rather than blocking on one decode. Zooming out is the one direction viewport tiling does not help: there is no coarser representation to read, every tile is full resolution. That wants a multi-resolution points pyramid (the Python writer already has
points multiscale), not a finer index, and it is recorded as the loudest remaining item in the punch-list.pointsTiling: 'off'restores the previous behaviour per layer, and the panel keeps the toggle so the comparison can be made.This is the part worth arguing about before merge.
Reading fewer bytes
Row-group selection moved off the file and onto parquet footer statistics. The bisect it replaces range-read a row group's bytes — every column, ~2MB — to recover two boundary values,
log2(rowGroups)steps per Morton interval, a few hundred intervals per query. One 1024 µm viewport tile, both returning the same 643,961 points:The remaining 32 reads are the row-group data. It is also stricter than the bisect, which tested only
maxand assumed row groups tile the code space without gaps. The bisect stays as the fallback when statistics will not parse.The grid comes from the artifact
It was one fixed level (
minZoom/maxZoom: -1), so every tile was 1024 local units at every zoom and the number came from deck's defaults rather than the data. Both ends now derive from the point density:zoomOffset = log2(modelMatrixScale)couples deck'sz, chosen from a world-space zoom, to tile spans expressed in local unitsFor the Xenium element that is two levels, 1024 µm and 512 µm. The narrowness is the finding, not a shortfall: 50k-row groups put the floor at ~402 µm, so the old fixed 1024 was accidentally near-optimal for this file and would not be for one an order of magnitude smaller or denser.
The tile cache is now budgeted in rows — 16 tiles / ~5.2M worst case — rather than deck's
5 x the selected tile count, which on a coarse viewport of this element could retain ~220 tiles / ~71M rows against a 4M resident cap. That answers open question 3 at the level ADR 0005 asks for: accounted, not managed. Nothing evicts by bytes.Three guards, because under-selection is silent
This path failed by quietly returning fewer points four times during the work — the
zcoverdepth, the row-group bisect, and two claims the artifact makes about itself that nothing checked. Each now fails loudly and falls through to the preload:offseton row-group reads, so every group reportedmax === minand the bisect skipped the group containing the interval start — 11 of 92 row groups, 188k points, rendered as Z-order-shaped holes. Fixed by derivingmaxfrom the next group's first value.TileLayerextent and the domain viewports are normalised against, so a wrong one clips the tile grid — whole regions never requested — and mis-maps queries to row groups. The probe now recomputesmorton_code_2dfrom x/y for a sample of real rows: a sound element matched 320/320, one with a stale box 0/320.morton_code_2dcolumn that is not sorted. A feature-primary artifact carries the identical column with identical values, unsorted; the bisect landed arbitrarily and a tile came back holding whichever feature blocks were in the row groups it picked. Read from footer statistics, free:transcripts_feature_then_mortondescends at 185 of 244 boundaries, both morton-primary elements at none.Guard 2 was found because the
index-permutationsstore's*_featureelements carry a stale sentinel box. Those files have been regenerated in place — the writer never had the bug, they were simply older than it.Also in here
getTileDataand itsupdateTriggers, and is applied inside the row-group scan: one gene takes a viewport tile from 3,128,988 points to 87,594. It does not reduce I/O — row groups are chosen spatially and a gene is spread across all of them — and the plan's original expectation that it would is corrected in place.blockingResourcesbecame state-derived, so a tiled entry does not sit on "Loading layer data…" with auto-fit never firing.Verification
Headless specs throughout (core 456, layers 245, vis 166); each guard was checked non-vacuously by reverting the fix and confirming its test fails. Verified in the app against a real 12.1M-point Xenium store: framing, tile subdivision on zoom, per-feature colour, the feature filter, and each guard's fallback path with its console warning.
Deliberately left open
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation