Points: make feature selection work on large elements, with a name-based selection API - #89
Conversation
The one tested dedup/supersede/settle primitive Track A hangs the four points slots off. Record-identity supersession (a superseded load can write nothing — not its result, error, or a late progress tick); keyed dedup so everything a request depends on lives in K; owns the AbortController; failure is a Resolution.failed state via toSpatialEntryError, not a console.error; retry() re-runs the last loader; streaming partials via emit(). notifyOnLoading lets a slot stay quiet on loading-start when its stale keeps drawing. Unconsumed; unit-tested standalone (15 cases). Mirrors how Step 0 landed contracts alone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PointsEntry's preload (data/memoryCap/loadAbort/status/loading) and rowCodes
(rowCodes/rowCodesLoaded/rowCodesLoading) field-groups become two RequestSlots,
both keyed on the memory cap.
- R1 (cap drag 4M→8M→4M double-decode) falls out of record-identity
supersession: a superseded reload can no longer wipe the live one's markers.
- R5 (filter-mask misalignment) is fixed by keying rowCodes on the cap and
threading it into ensureRowFeatureCodes → loadRowFeatureCodes({ memoryCap }),
so codes and geometry read the same window.
The PointsResolver public surface is unchanged; the 855-line pointsDataEngine
regression net stays green, minus two assertions flipped to expect the now
cap-aligned loadRowFeatureCodes call. Adds fail-before/pass-after R1 and R5
tests through the resolver.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PointsEntry's matching/matchingLoading fields become one RequestSlot keyed on
`${signature}#${cap}`. The coverage-based reuse fast path (a superset scan serves
a subset selection) and the cap-adequacy checks stay in the resolver; the slot
owns the scan lifecycle, streaming partial, and supersession.
- R2 (reselect a covered feature mid-scan → two same-signature scans corrupting
each other): record-identity supersession means a superseded scan can write
nothing, so it can't clobber the live one.
- R3 (raise the cap during a scan → served by the smaller scan): the cap is in
the slot key, so it supersedes rather than dedups.
RequestSlot.emit gains a `silent` option so the scan keeps its partial buffer
fresh on every producer tick (its identity drives the overlay resource) while
throttling host notifies to the old 5000-matched-row granularity — preserving
the pan-flash identity guard.
Catalog slotification is deferred to A4: its two-phase preview/full and its
failure semantics change together with the retryable-catalog work. Adds
fail-before/pass-after R2 and R3 tests through the resolver; the engine and
resource-identity regression nets stay green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ack A, step A4) Completes the four-slot state model: the two-phase feature catalog becomes a RequestSlot keyed 'preview' | 'full'. The resident-subset preview (from the geometry decode) settles 'preview'; the authoritative listFeaturesWithCounts scan requests 'full', retaining the preview as stale so it keeps showing while the full list loads. The failure semantics change (ADR 0004 §3): a rejected full-catalog scan is now a retryable `failed` resolution, not a permanent null-settle that could never recover. PointsResolver.retry(key) (+ a PointsDataEngine passthrough) re-runs any failed slot's loader — the stuck-catalog fix the punchlist called out. preload, rowCodes and matching already surfaced structured failures via their slots; catalog now joins them. The two pointsDataEngine/resolver cases that pinned the old "settle null, stay settled" behaviour are flipped to assert the retryable failure + recovery. Adds a retry test through the resolver. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rack A, step A5 / D8) The matching-scan and row-codes slots now hand their AbortSignal to the element loaders, which thread it to the VPointsSource scan generator. The generator checks it between row-group chunks (checkAbort), so a superseded or evicted scan stops decoding — bounding wasted work to at most one in-flight chunk instead of running the whole scan into a dropped result. PointsResolver.evict/dispose now reset the slots, aborting in-flight loads. Scope note (the `// we should be passing abort to worker` TODO): cancellation is enforced between chunks on the main thread, NOT inside the worker. Each worker call decodes a single row group — one uninterruptible WASM decode — and chunks are awaited serially, so there is no pending-request queue to drain and a worker-side cancel message would buy nothing. Documented at the call site; revisit only if a single worker request ever spans many row groups. Adds resolver tests proving supersede and evict abort the in-flight scan's signal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… A, step A7) Dissolves the Step 1 boundary: the points reconcile context now carries the full config (featureCodes, colorByFeature, memory cap) instead of only the cap, so PointsResolver.plan() emits the row-codes and feature-index-scan tasks and the store loads them from the commit phase. The two render-phase kicks in getLayers — `void pointsEngine.ensureMatchingFeaturesLoaded(...)` and `void ensureRowFeatureCodes(...)` — are removed; getLayers is now pure reads. The reconcile effect gains `loadedDataRevision` as a dependency so an async settle (the preload landing, which flips supportsFeatureScan) replans and the scan gets emitted. It is touched as a bare reference in the body — a re-trigger, not a read — rather than suppressing exhaustive-deps. The plan/load dedup keeps the extra runs cheap and convergent. vis surface + lifecycle guard (useLayerData.spec.tsx) and all suites stay green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…p A8) Ran the spike empirically: added effect@3.22 as a core devDependency, implemented the matching-scan supersession twice (plain RequestSlot and an Effect Fiber + Effect.async slot), and drove both through the same two-concurrent-scan race, then removed both per "delete the loser". Verdict against the agreed kill criterion (drop Effect unless it wins all three): - Supersession correctness under two concurrent scans — tie (both drop the superseded result). - Interruption reaches the worker — tie (both abort the scan's AbortSignal; Effect needs a runtime tick, plain aborts synchronously). - Fewer lines to set up a race — plain wins (2 synchronous calls vs an Effect.async + Fiber + runFork slot and an awaited tick before every assertion, plus a multi-second import on a zero-runtime-dep package). Ties two, loses one → plain RequestSlot wins; Effect not adopted. Outcome recorded in the handoff doc. The RequestSlot seam stays shaped so revisiting later is a swap, not a rewrite. No dependency or spike code lands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(Track A, step A6 / D10)
The partial overlay minted a NEW render resource every scan chunk (memoised on the
growing batch's identity), so PointsLayer saw a new loader identity, reset its state,
and deck tore the __partial sublayer down and rebuilt it per chunk — the flash.
Now the partial resource is held STABLE for the lifetime of one scan
(PointsRendererAdapter keys it on the scan key `${signature}#${cap}`, exposed by
PointsResolver.getPartialScanKey). Its backing batch is swapped through a mutable
holder whose loader.loadAll reads the current buffer, and a revision counter bumps on
each growth (getMatchingPartialRevision). PointsLayer takes a resourceRevision prop
and re-reads loadAll on a revision change WITHOUT resetting — so the overlay fills in
without a per-chunk teardown. One deck layer per (entry, selection), zero teardowns
per scan. A new scan (scanKey change) mints a fresh resource; settle → null as before.
The pointsResourceIdentity guard is flipped from "CHANGES identity when the buffer
grows" to "HOLDS identity, bumps a revision". Mechanism verified headlessly; the
visual no-flash on a real streaming scan is pending a browser session with a large
Xenium dataset.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Records A1–A8 complete: four RequestSlot resources, races R1/R2/R3/R5 closed, retryable failures + retry(), cancellation threaded (D8), plan() migration, and the D10 flash fix. Notes the one pending item — visual browser confirmation of the no-flash overlay on a streaming scan. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r policy P1) Selecting gene A, deselecting, then selecting a disjoint gene B drew ALL of A's points until B's scan settled: the matching slot keeps A's batch as `lastGood` (stale), and getLayers used getMatchingResource as the base without checking it covered the CURRENT selection (filterMatched was false, so A rendered unfiltered). getLayers now uses the matched batch as the base only when getLoadedMatchingFeatureCodes covers the current selection; otherwise it falls through to the resident preload filtered to the new gene in-memory (instant, correct gene) while the partial overlay streams B in. Adds a fail-before/pass-after guard: switching to a disjoint selection renders the resident base, not the stale matched batch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… (render policy P2) A6 stabilised only the __partial overlay. The BASE layer still flashed: getLayers drew the resident and matched batches under the same id `layerId` from two different resources, so every resident↔matched transition (select-settle, deselect, the P1 coverage fallback) changed the loader identity and PointsLayer hard-reset — a blank frame. The base is now ONE stable resource per element whose backing batch swaps under it (PointsRendererAdapter.getBaseResource + getBaseRevision, lifting A6's growing-holder pattern to the base). getLayers picks the batch — matched-if-covered-and-non-empty else resident — and feeds it through getBaseResource with a resourceRevision that bumps on each swap; PointsLayer re-reads loadAll on the revision change WITHOUT resetting. No teardown across resident↔matched↔streaming transitions. The resident/matched getLayers branches collapse into one base layer; the __partial overlay and the empty-lock fallback are preserved. PointsLayer.updateState now returns after refreshPreloadedBatch on a revision change, so the signature-filter pass can't filter the stale batch by the new codes during a swap (batch and its row-aligned codes change together). Guards: getBaseResource holds identity across a resident↔matched swap and bumps its revision (pointsResourceIdentity.spec); the useLayerData integration test asserts the same base resource identity persists across the swap while its underlying batch goes from matched to resident. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Testing surfaced three problems that live in the render-time policy, not the
Track A state model:
1. Flat colour on "all features". Colour-by-feature is on by DEFAULT in the
renderer, but both `PointsResolver.plan` and `getLayers` only loaded/threaded
the per-row codes when `featureCodes !== undefined || colorByFeature === true`
— and `colorByFeature` has no UI toggle, so the unselected view threaded no
codes and drew flat (dict-only datasets, whose codes settle through the
rowCodes task, always did). Gate on `colorByFeature !== false` instead, so the
codes load and thread whenever colour is not explicitly disabled.
2. Wrong gene shown on a switch. `preloadedScatterStrategy` kept the PREVIOUS
filtered batch on screen while the new selection's filter ran off-thread — so
switching A->B drew gene A under a gene-B selection for a frame or two. Reuse a
stale filtered batch ONLY when the gene signature is unchanged (cap/row-code
buffer moved, selection did not); otherwise draw nothing until the new filter
lands.
3. Wanted gene dropped when growing a selection. Growing [A]->[A,B] found the
last scan's coverage ({A}) did not cover [A,B], so the base fell all the way to
the resident window and blinked A's out-of-window points out until B settled.
Use the whole-dataset matched batch as the base whenever it covers ANY
still-wanted gene (drawn filtered to `selection ∩ covered`); the overlay streams
the not-yet-covered newcomers in. Never surfaces a deselected gene (the batch
holds only covered genes), never hides a covered one.
Tests: resolver plan contract updated to default-on codes; new vis tests for the
grow case and the all-features colour threading; new strategy tests for the
never-show-the-previous-selection behaviour. core 270 / layers 135 / vis 95 green;
all three packages typecheck + build.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…a (stage 1)
Replace the procedural golden-angle colour in PointsFeatureColorExtension with a
GPU palette lookup: a 1-row RGBA texture, one texel per feature code, sampled with
`texelFetch(pfcPalette, ivec2(code, 0), 0)`. The texture is built on the CPU by
`buildFeaturePalette`, default-filled from the SAME `featureCodeToRgb` the JS
swatches use — so the rendered colours are byte-identical to before. This is the
behaviour-preserving foundation for per-feature colour overrides (stage 2): colour
is now DATA (a patchable texel), not a hard-coded formula.
Mechanics (mirroring LabelsBitmaskTileLayer's texture path):
- The extension creates the texture via `device.createTexture` and binds it with
`model.setBindings({ pfcPalette })`; a 1x1 fallback is bound from initializeState
so the declared sampler always has a binding (an unbound sampler is a draw error).
- The code space is threaded as `featureCodeSpaceSize` (catalog maxCode + 1) from a
new memoised `PointsDataEngine.getFeatureCodeSpaceSize` through getLayers to both
the base and the streaming-overlay points layers; it sizes the LUT and the
in-shader clamp so an out-of-range code mis-colours its tail rather than reading
undefined memory.
- Highlight stays a uniform (`highlightCode`, folded into the pfcColor UBO next to
`paletteWidth`) — it changes every mousemove, so a texture rewrite per frame would
be wasteful.
The GPU output is verified live (no points fixture headlessly); tests lock the pure
palette bytes, the look-preserving default, and the deck shader/prop wiring.
layers 140 / vis 95 green; both packages typecheck + build.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e 2) Build on the palette texture (stage 1) to let a feature be recoloured. A new `PointsLayerConfig.featureColorOverrides` maps feature NAME → [r,g,b]; getLayers resolves it to a stable `code → rgb` map via `PointsDataEngine.getFeatureColorOverrideMap` (memoised on config + catalog identity) and threads it to the base and overlay points layers, where the extension patches those texels into the LUT. Keyed by NAME, not code, on purpose: a feature's code can differ between the resident-preview catalog and the authoritative full one, but its name does not — so an override lands on the right feature once the catalog settles (the dict-only code remapping issue). Panel: the feature swatch is now a colour picker — a transparent native colour input over a swatch showing the feature's effective colour; an override gets a blue ring and a reset (⟲) control. Both are interactive content inside the row label, so operating them does not toggle the feature's selection checkbox. Tests: engine resolves by-name overrides to codes, drops unknown names, returns null (all-default) when none set, and keeps a stable map identity across calls (no per-frame palette rebuild). layers 143 / vis 95 green; both typecheck + build; vis bundle loads clean in the demo. GPU colour output verified live. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The swatch collapsed / misaligned: the outer wrapper was a flex child (so it sized), but the inner swatch span sat in a non-flex parent where `display: inline` ignores width/height, and its border overflowed for want of `box-sizing`. Make the wrapper span itself the swatch — its background is the effective colour, a transparent colour input overlays it — with `display: inline-block` + `box-sizing: border-box` so the 12x12 size and 1px border hold in any context. Verified live on a Xenium transcripts layer: swatches render as aligned 12x12 boxes, vertically centred with the checkbox, and the points render coloured-by-feature from the palette LUT. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wire the existing highlight shader path (the `highlightFeatureCode` uniform, which desaturates + dims every non-matching point) to a UI source: hovering a row in the feature list emphasises that gene's points on the canvas. Highlight is runtime-only UI state (never serialised), so it rides on the `PointsDataEngine` — the mutable external store the feature panel already subscribes to and the render path already reads — rather than the zustand store / renderStack, which the panel↔render path deliberately bypasses. `setHighlightedFeature(key, code)` notifies subscribers (repainting both the panel and the canvas); getLayers reads `getHighlightedFeature(elem.key)` and threads it through PointsLayer → scatter → extension. `PointsResolver.notify` is made public so the facade can request a repaint without a data mutation. The panel sets the highlight on row mouse-enter, clears it on leave, and clears on unmount so an emphasis can't stick. A code < 0 means no highlight (the shader's safe default). Verified live on a Xenium transcripts layer: hovering a gene desaturates + dims the rest of the field, leaving that gene's points vivid. core 270 / layers 144 / vis 95 green; all three typecheck + build; biome clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ntation-stages-993e15
The wild-type transcripts cold load showed nothing for tens of seconds: loadPoints was one shot — fetch the whole capped window, one worker decode, then paint. This decodes ONE ROW GROUP AT A TIME and publishes the growing buffer, so points appear while the rest decodes. Colour, per the dataset shape: - An authoritative INTEGER feature-code column is plain (not dictionary-encoded), so it is safe to read per row group alongside x/y — those datasets stream fully COLOURED from the first chunk. - A dict-only element (feature_name dictionary, e.g. wild-type transcripts) can only stream the axes, because readParquetRowGroup mis-decodes dictionary columns. It streams flat and falls through to the existing whole-part decode to settle codes + catalog, so colour lands at the end. That second decode is the deliberate price of early paint for dict-only data. Mechanics: - `streamPointsGeometryByRowGroup` preallocates at maxRows and appends at an offset cursor; each partial is `subarray` VIEWS over the filled prefix, so a progress tick is O(1) — no re-concatenation (contrast pointsScanChunkProgress, O(chunks²)). - The preload slot passes an onProgress that `emit`s the growing batch, throttled to one repaint per 250k rows; `getPreloadPartialBatch` exposes it. - getLayers falls back to that partial when no resident batch has settled, so the growth flows through the stable base resource (P2) as revision bumps — no teardown. - Gated on the caller wanting progress AND row-group range reads being available; otherwise the one-shot path is unchanged. Verified live on a 12.1M-row Xenium transcripts element: the row-group loop engages and emits partials throughout the load. core 283 / layers 149 / vis 102 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The row-group streaming path passed `featureCodeColumnName` but NOT `featureKey`. The worker gates code extraction on `featureKey` (handleDecodeParquetGeometryCapped), so the streamed batch came back with no per-row codes — and because a code-column element returns the streamed batch as its FINAL result, that element ended up with no codes anywhere: colour-by-feature drew everything one flat colour, both while loading and after. Seen on `transcripts_feature_then_morton`. Pass `featureKey` alongside `featureCodeColumnName`. That is sufficient and needs no extra projection: `resolveRowFeatureCodesFromTable` returns the code column directly when one is named and never touches the (unprojected, dictionary-encoded) name column — which is exactly why streaming a plain int code column per row group is safe in the first place. Also harden the hand-off: the streamed batch is accepted as final only when it ACTUALLY carries codes, not merely because a code column exists. If codes ever fail to come back we now fall through to the one-shot decode — slower but correct — instead of silently settling a permanently colourless batch. Verified live: transcripts_feature_then_morton renders per-feature colours matching the panel swatches again. core 283 / layers 149 / vis 102 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per-row-group reads of the feature-code column can come back with only the column's DISTINCT values rather than one code per row — observed on `transcripts_feature_then_morton`, where a ~100k-row group yielded 4 codes. The row-group path mis-handles dictionary encoding, and that evidently applies to the code column too, not just `feature_name`. So the premise behind "an integer code column is row-group safe, therefore those datasets stream coloured" does not hold for this data. The dangerous part was the write, not the read: `codeBuffer.set(codes, filled)` with a short array left the remaining rows at 0 — a VALID feature code — so those points were confidently MIS-coloured, and the array still passed the "codes are present" check and got settled as the final batch. Now a chunk's codes are used only if there is one per row; otherwise codes are discarded for the whole stream, the partials publish geometry alone, and loadPoints falls through to the one-shot decode for the authoritative codes + catalog. Net behaviour: geometry still streams early (flat), colour is correct once settled — the same contract as a dict-only element. Colour-during-stream therefore remains unsolved for every dataset shape; it needs a per-row-group code read that is actually trustworthy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Colour was flat for most of a large load, in two stages: a uniform peachy colour while points accumulated, then a uniform bright pink for a long stretch, only resolving into real colours at the exact moment the feature stats appeared. That timeline is the tell — the palette width was chasing the CATALOG, which is the last thing to load. The LUT was sized from the catalog's code space. With no catalog the table was ONE texel wide and the shader clamps every code to texel 0 (peach). A small preview catalog then made it a few texels wide, so every code clamped to the LAST texel instead (pink). Only the full catalog made it wide enough to be correct. But the colour of a code is a PURE FUNCTION of the code — the catalog was never an input, only a (late) guess at how wide the table had to be. So size the table to a generous default code space up front (4096 texels, 16 KB) and widen only if a catalog turns out to be larger. Colour is now correct from the first streamed chunk, with no catalog at all. Two supporting fixes to the same class of bug: - initializeState built a hard-coded 1x1 palette instead of using its actual props. The scatter sublayer only mounts once there is a batch to draw, so the code space is often already final at mount and the prop never "changes" — leaving that 1x1 table in place permanently. - updateState now reconciles against the width of the texture it actually holds rather than a prop transition, so a missed update self-heals into a rebuild. Mid-load colour could NOT be visually confirmed here: the camera is not framed during a cold load (pre-existing auto-fit behaviour), so the canvas is off-target while streaming. The new test pins the mechanism — a palette built with no catalog gives distinct codes distinct texels. layers 150 green; lint gate clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The feature panel showed "Loading features…" for the whole catalog load, but the slow part is not the feature list — `listPointsFeaturesWithCounts` resolves the names/codes cheaply and then scans EVERY row group for per-feature counts. The catalog only settled after both, so the panel waited on counts to display a list it already knew. Publish the names-only catalog as a slot partial the moment it is known: - `listPointsFeaturesWithCounts` takes an `onPartialCatalog` callback, invoked before the counts pass. - `ensureFeatureCatalog` emits that partial from the catalog slot. - `getFeatureCatalog` prefers the in-flight partial over an older resident preview — it is the more complete list, just without counts. - The panel only blocks on "Loading features…" when there is nothing at all to show, and says "Counting features…" while the counts fill in. Features are therefore listed, coloured and selectable while counting continues; the count column and count-sorting simply appear when the scan lands. Verified live on transcripts_feature_then_morton: 541 feature rows rendered with swatches while "Counting features…" was still displayed. core 283 / layers 150 / vis 102 green; lint gate clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The streamed points already carry feature codes, so a per-feature tally costs no extra I/O — just one pass over codes already in hand. Accumulate `code -> rows` as each row group lands and publish it on the batch (`featureCodeCounts`), exposed as `getResidentFeatureCounts`. The panel uses it to fill the count column (and sort by it) before the whole-dataset counts scan lands. These are counts over the RESIDENT WINDOW, not the dataset, so they are marked with a leading ">=" and a tooltip saying so — they must never read as dataset totals. Authoritative counts replace them when the catalog scan settles. Note on where the time actually goes: measuring this on a 12.1M-row element, the dominant cost before the feature list appears is `listPointsFeaturesByFeatureColumnScan` (resolving NAMES from the feature column), not the counts pass. So this tally fills the window between "names known" and "counts known", but does not shorten the wait for the list itself. Making the name scan emit partial catalogs per row group is the change that would do that. core 283 / layers 150 / vis 102 green; lint gate clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The oversized-dataset catalog scan read whole parquet parts to collect feature names, because per-row-group reads return blank names for DICTIONARY-typed columns. That cost a full download (~8.8MB for a 1.15M-row file) before the panel could list anything, and it could only ever publish the catalog once, at the end. The blank names were never really about dictionary *page* encoding — plain-utf8 columns with RLE_DICTIONARY pages read back fine. It is the Arrow dictionary *logical type*: readParquetRowGroup emits no DictionaryBatch, so the values are lost (the buffer that looks like indices is the Utf8 offsets buffer). Nothing is recoverable there. ParquetFile.stream() decodes those columns correctly, issues its own range reads, and yields batches within a row group rather than only at row-group boundaries. Projected to the feature column that is ~4KB of an 8.8MB file, so the scan becomes a handful of range requests and can publish the list as it grows. Codes stay compatible: streaming and whole-file both assign codes in first-seen row order, so a streamed prefix agrees with the whole-file scan on every code assigned so far. That is what makes the partials safe to render — a feature's code never moves, so selections and swatches made against a partial stay valid. Gated on runtime and store: the reader panics under Node with an async trap that escapes try/catch, and it needs a fetchable URL, so it is a fast path behind a capability check with the byte-oriented scan intact as fallback. Custom, prefixed and in-memory stores are unaffected. Measured on a 4.5M-row dataset through listPointsFeaturesWithCounts: first names visible 183ms vs 3213ms, full catalog 1482ms vs 3263ms, with names, codes and counts identical to the fallback path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A failed HTTP range request during streaming panics the wasm reader with `RuntimeError: unreachable` AND leaves the promise it came from permanently unsettled. Neither half is catchable: the panic is async so it escapes try/catch, and an unsettled promise raises nothing at all. The scan therefore hung forever, and every consumer waiting on the catalog hung with it — the fallback could not run because nothing threw. Reproduced by serving a 500 to the 8th range request of a stream: `Uncaught RuntimeError: unreachable` followed by an indefinite hang. Bound progress instead of trying to detect the failure. The watchdog spans the whole attempt, not just read(): the panic can land while opening the file or the stream just as easily, and guarding read() alone still allowed fromUrl()/stream() to hang. It resets on each batch, so a legitimately long scan keeps running while a stalled one is abandoned. Also stop awaiting reader.cancel() — it can hang exactly like read(). With the same fault injected, all four failure modes (500, mid-response abort, truncated body, 200-instead-of-206) now recover with the correct 300-feature catalog rather than hanging. The panic is still logged; a wasm trap cannot be suppressed from JS. It is no longer fatal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The reported `RuntimeError: unreachable` is preceded by a 416 Range Not Satisfiable. Logging a real scan shows the reader needs exactly three request shapes: GET bytes=-8 suffix range, footer length GET bytes=-2365 suffix range, footer GET bytes=16550372-17613446 bounded range, column chunk Suffix ranges are the fragile one; plenty of static servers answer 416. The rest of this class already tolerates that — loadParquetFooterBytesForPath falls back to whole-file reads — which is why such a server looks healthy until a reader that fetches on its own is pointed at it. That reader treats a refusal as unreachable, so it panics and never settles. The stall watchdog recovers from this but costs 15s and still logs an uncatchable trap. Probing is better: two 8-byte requests, cached per origin, checking both range shapes return 206 with the expected length. A server that fails either never sees the streaming reader at all. Verified against three servers: healthy streams (269 partials, 1.9s); 416-on-suffix falls back cleanly with the correct 300-feature catalog and no trap; Range-ignoring likewise declines streaming. The watchdog stays as the backstop for a server that passes the probe then misbehaves mid-stream. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The probe fetched without a cache directive, so once any whole-file read had populated the browser cache the browser answered the suffix range itself and the probe reported success for a server that returns 416. Verified against a real deployment: curl shows `bytes=-8` -> 416 while the in-page probe returned true, and the streaming scan then "worked" — but only because all 123MB were sitting in cache from a previous run. That is the failure the reported trap came from: it holds until the entry is evicted or the cache is disabled, and panics after that. It also explains why DevTools changed the behaviour, since "Disable cache" simply stopped the cache from covering for the server. Probing with cache: 'no-store' makes the decision a property of the server alone, so behaviour no longer depends on what happens to be cached. Against that same deployment the verdict is now false, the scan falls back deterministically, returns the correct 541-feature catalog, and logs no trap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Points stayed uncoloured for over two minutes on wild-type transcripts. The cost was not the parquet read: decoding the feature column of a 4M-row file takes 1.6s. Building the per-row codes from it took 59.3s. resolveRowFeatureCodesFromTable asked the Arrow vector for a value per row. On a DICTIONARY column every get() materialises a fresh JS string, so it paid 4M UTF-8 decodes and 4M string hashes to resolve 541 distinct genes. Resolving each chunk's dictionary once and mapping raw indices does the same work with 541 string lookups and a typed-array scan. Measured on the real column (4,000,000 rows / 3,907 chunks / 541 features): 59,281ms -> 58ms, with zero mismatches against the per-row result over every row, not a sample. This also fixes a latent correctness bug. dictionaryStrings() took the FIRST chunk's dictionary and applied it to the whole column, but every parquet column chunk carries its own dictionary — index 0 in one chunk need not be the same gene as index 0 in the next. It was masked because Arrow's get() already returns decoded strings, so the dictionary it built was never consulted. The new path is per chunk, and a regression test covers two chunks whose dictionaries disagree. Worth noting what this leaves: the worker request still had to time out at 120s before falling back, so the visible delay was that timeout plus a main-thread repeat of the same slow loop. Both were symptoms of this. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wild-type transcripts painted quickly but stayed flat for a long time. The preload reads row groups, and readParquetRowGroup cannot decode a DICTIONARY-typed feature_name, so for an element with no integer code column it published geometry with no codes at all: measured on the real 4M-row element, 4 progress callbacks, 0 carrying codes, and no codes on the final result either. Colour could only come from a separate decode afterwards, which is why it lagged so far behind the points. ParquetFile.stream() decodes dictionary columns correctly, so x/y and the feature column can arrive in the same batches. Each batch resolves its chunk dictionaries once and maps raw indices, so a coloured batch costs a lookup table plus a typed-array copy. Measured on that element, same server: streaming 1,165ms 62 partials 62 coloured first colour 209ms fallback 16,534ms 4 partials 0 coloured first colour never Both return all 4,000,000 points; codes are row-aligned with geometry and every code resolves to a catalog entry. The codes are in this path's own space (chunk dictionary order), not the full scan's (row order). That is the existing dictionary-only contract: the preload returns the matching catalog, and the resolver re-expresses the codes against the authoritative one via reconcileRowCodes/remapRowFeatureCodes. Returning that catalog is therefore load-bearing, and a test pins the handoff. Needs a URL-backed store whose server answers suffix ranges; anything else falls through to the row-group path unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two problems on an element small enough to load whole (a 3.7M-row MERFISH single_molecule, under the 4M cap): Selecting a feature sat on "Loading selected features... 0 points so far" and eventually appeared in one go. plan() scheduled a whole-dataset matching scan whenever a selection was active and the element supported scanning, without asking whether it could add anything. With an untruncated preload every matching row is already in memory, so the render path's in-memory filter gives the identical answer instantly — the scan just re-read the whole file to rediscover what was resident. It is now skipped unless the resident batch is truncated, and stays conservative: unknown or in-flight preload still scans. Feature stats never settled. loadFeatureCounts needs an integer feature code column and returns an empty map without one, so for any dictionary-only element — MERFISH cell_type, Xenium feature_name — no counts ever arrived. Both catalog builds that decode every row (the whole-table read and the streaming scan) now tally as they go: no extra I/O, and dataset-wide because they read every row. The old guard was right that counts must not be derived independently of the catalog: for dict-only elements codes are app-assigned, so counts from a separate scan can be keyed to a different code space than the entries they land on. loadFeatureCounts therefore still declines. The tally here is keyed by the very map the catalog assigns codes from, so it cannot disagree; the renamed test now pins both halves. Test fixtures that model matched-vs-resident behaviour were marking the resident batch complete while expecting a scan; they now mark it truncated, which is the only situation where that scan is meaningful. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A read-only store's layout never changes, but nothing remembered it, so every caller re-derived it — and a single points load asks ~20 times (row counts, tiling, row-group extents, schema/column resolution, the streaming reader). Each rederivation re-issued the directory probe and the 404 past the last part, which is the stream of repeated 404s — and, on MDV, repeated "[Errno 21] Is a directory" 500s — visible in the network tab. Three separate places derived the same fact, so caching one was not enough: - `loadParquetDatasetMetadata` (range-reads footers) is now memoized per path. The promise is shared, so concurrent callers collapse to one probe. - `discoverMultipartPartPaths` (whole-file reads — the fallback for stores without range support) now consults that layout first, and only probes when it is genuinely unavailable. It was enumerating part.0, part.1, … again even for elements already known to be a single file. Its own fallback is memoized too. - `resolveParquetRowCount`'s fallback loop re-enumerated inline; it now shares the memoized discovery. Every probe involved goes through the magic-checked `loadParquetFileBytesAtPath`, so it sees exactly the same part set. `loadParquetSchemaBytes` and `loadParquetBytes` walk candidate paths [directory, part.0]. Blind, that order is right — you cannot know which it is without asking — but once the layout is known, leading with the directory is a guaranteed- useless request every time. Both now PEEK at an already-resolved layout and go straight to the real part. Deliberately a peek, not a call: resolving from there would probe the whole sequence, so an element whose footer `readMetadata` cannot parse would pay for both walks (pinned by the existing vtable.spec fallback test, which caught exactly that when I first wrote it as a call). Nulls and rejections are never cached. Since the directory-500 fix a failed probe returns null rather than throwing, so an all-probes-failed run is indistinguishable from "no dataset here" — caching it would strand a real element behind one network blip. Measured on merfish (3.7M points, http-server) over a cold load: directory-path requests 8 -> 2 (2 = the one real discovery probe) trailing part.1 404s 3 -> 1 total points.parquet reqs 39 -> 21 Points still load fully (3,714,642, exact per-feature counts).
Row-group chunks are posted to the points worker with their buffers
TRANSFERRED (zero-copy), which detaches them in this thread. But
`readParquetRowGroupBytesByGroupIndex` returned `part.schemaBytes` — a live
reference into the dataset metadata — so the first transfer detached the
METADATA's buffer, and the next row group posted an already-detached one:
DataCloneError: Failed to execute 'postMessage' on 'Worker':
ArrayBuffer at index 0 is already detached
which aborts the progressive preload ("falling back to a single decode") and
drops the element onto whole-file reads of every part.
This is a regression from 15f84f6. Before that commit each call re-probed and
returned FRESH footer bytes, so transferring them was accidentally safe; caching
the layout made the same buffer be handed out repeatedly and exposed the latent
ownership bug. The invariant is: never transfer a buffer you do not own.
The chunk now copies the footer, so the cache keeps its own. Footers are small
next to the row-group payload, and not transferring would structured-clone —
i.e. copy — just the same.
Test transfers one chunk's schemaBytes and asserts the next read is still
usable, and that the cached metadata survives. It fails without the fix with
exactly the reported detached-buffer error.
NOTE: reproduced and fixed at unit level; I could NOT reproduce the in-app
failure on the datasets available here, whose parts each hold a single row group
(so no part's footer is ever transferred twice). It needs a part with multiple
row groups. The knock-on claim — that this also removes the repeated whole-file
part fetches — follows from the fallback path but is NOT verified end to end.
`Vector.get(i)` reads like an array access but is not. On a multi-chunk vector —
any table assembled from more than one record batch, i.e. every multi-row-group
or multi-part read — Arrow swaps in a prototype whose `get` is
`binarySearch(data, offsets, i)`, so each read walks the chunk offsets. Even the
single-chunk path is a closure dispatch returning a boxed value.
Both scan loops called it per row, per column (x, y, z, and the feature-code
column), over every SCANNED row — the whole dataset, not just the matches. That
is a large part of why showing points for a selected feature takes so long.
Measured over 4M rows, one column:
.get() per row 1 chunk 49ms | 8 chunks 148ms | 64 chunks 244ms
toArray() + index 1 chunk 6ms | 8 chunks 17ms | 64 chunks 5ms
`toArray()` costs 0-2ms: zero-copy for a single chunk, one sequential concat
otherwise. So this is 15-50x on the hot path for a hoist, not a restructure.
A nullable column is materialised once through `get()` with nulls as NaN, so
there is a single indexed loop shape rather than a second slow path. This makes a
non-finite coordinate skipped where the old `typeof x !== 'number'` test emitted
it — a point at NaN cannot render, so that is a fix, but it is a behaviour
change.
Test scans the same rows as a contiguous table and as a two-batch one, asserting
identical output and that matches in the second chunk survive; it asserts the
fixture really is multi-chunk, since otherwise it would prove nothing (verified
separately that the fixture takes the binarySearch prototype path).
NOT addressed: the `xs.push()` into `number[]` then `Float32Array.from()`, worth
a further ~2.5x (27ms -> 10ms per 4M) but needing match counts plumbed through to
size the buffer.
`Table.numRows` is not a field:
get numRows() { return this.data.reduce((n, d) => n + d.length, 0); }
— a fresh closure plus a walk of every chunk. Both scan loops had it in the loop
CONDITION, so it ran per row, and its cost grows with chunk count: exactly the
tables that are already biggest. Measured over 4M rows:
1 chunk 35ms in-condition vs 5ms hoisted (7x)
8 chunks 112ms vs 4ms (30x)
64 chunks 662ms vs 7ms (97x)
This dwarfed the per-row `Vector.get` cost fixed in e5c9033 (244ms at 64 chunks),
which is why that commit alone did not show up end to end.
Measured in-app on a 12.1M-row Xenium transcripts element, selecting one gene
(MALL, 646,132 matching points), 4M-row resident cap, worker enabled:
before: >3 min, plateaued at 234,765 of 646,132 points, never settled
after: 3,473 ms, complete
Credit to the report that `numRows` dominates — it did, by a wide margin.
`vis-demo-alt` runs the demo on a fixed 5180 with --strictPort, for when 5173 is taken by another project. The points-worker note records the remaining allocation cost in the feature scan: the matched coordinates accumulate into `number[]` via push and are then copied into typed arrays, where the request already carries `featureCodeEntries` that could carry counts to size the buffers up front. Implemented next; kept as the author's own framing of the problem.
The scans collected matched coordinates into `number[]` and copied into typed
arrays at the end. That pays three times: each value is boxed as a double (8
bytes against 4), the array reallocates as it grows, and `Float32Array.from`
copies the lot again with both representations live at the peak.
`TypedPointBuffer` writes straight into Float32Array/Int32Array. Isolated, over
3 arrays:
646k rows 4M rows
number[] 19ms 116ms
growable 7ms 76ms (doubling copies dominate at 4M)
exact-sized 6ms 17ms
So the capacity hint, not the typed storage, carries the win at scale. Rather
than plumb catalog counts through the worker protocol, each scan reserves an
exact upper bound per chunk — `min(rows in chunk, remaining cap)` — which is
already known once the chunk is decoded. No protocol change, and it is exact
rather than an estimate. Growth is still handled: a hint can be absent or low,
and being wrong must stay correct rather than truncate.
In-app, 12.1M-row Xenium transcripts, one gene selected, same harness:
gene before after
MALL 3473ms 3644ms
CYP2B6 4027ms 3223ms
TCIM 7753ms 3019ms
Read this as "modestly faster, and much less variable" rather than a clean
speedup: n=1 per cell and the spread is wide. The variance drop is the more
credible effect and is what the change predicts, since it removes the per-scan
garbage that was driving the slow tail. Note the scan invokes the worker PER ROW
GROUP, so per-call match counts are small and the 4M-scale numbers above only
apply to the whole-part fallback.
Tests cover the growth paths the reserve hint can get wrong: no reservation, an
under-estimate, an over-estimate trimmed to the filled prefix, accumulation
across successive reservations, and Int32 codes beyond float32's exact range.
The row-group scan range-read `rowGroup.fileOffset()` for `compressedSize()` —
the WHOLE row group, every column — because parquet-wasm cannot fetch individual
column chunks (docs/parquet-wasm-limitations.md). Column projection only happened
later, at decode time, once the bytes were already down the wire.
On a Xenium `transcripts` element that meant fetching and decompressing all 12
columns (cell_id, transcript_id, fov_name, qv, …) for all 12.1M rows to use
three: x, y and the feature column. Profiling a single-gene selection put ~30% of
the wall clock in those reads and ~50% in the WASM decode of them.
`stream({ columns, rowGroups })` issues its own ranged fetches per COLUMN CHUNK,
so the projection reaches the network. It is the same reader the preload already
uses; the scan differs only in filtering each batch and yielding progress.
Xenium transcripts, 12.1M rows, one gene, worker enabled:
gene before (2 runs) after
CYP2B6 3223 / 4027ms 2289ms
TCIM 3019 / 7753ms 2061ms
MALL 3473 / 3644ms 2047ms
Counts verified against the catalog (568,863 / 495,074 / 646,132). The spread
collapsing from 4.7s to 240ms is the more interesting result: the unexplained
run-to-run variance flagged earlier was largely the whole-row-group fetches.
Two consequences worth knowing:
- Decode moves to the main thread — `stream()` is browser-main-thread only, since
`supportsParquetStreaming()` requires `window` and so is false in the worker.
Main-thread time rose (789ms -> 1459ms) but arrives in 65k-row batches, so the
worst long task is unchanged at ~145ms. Teaching the worker to stream by URL
would recover this; it needs that capability check relaxed for worker scope.
- The byte-oriented worker path stays as the fallback for stores the reader
cannot serve. Tests pin that gate, because getting it wrong fails quietly: a
gene that renders no points, or the slow path kept forever. The multipart case
is explicit — one unservable part declines the whole element rather than
scanning some parts and skipping others.
For a dictionary-only points element — a Xenium `transcripts` carries `feature_name` and no code column, as does merfish `cell_type` — feature codes are APP-ASSIGNED: a first-seen index from whichever catalog scan ran. They are not stable across the preview→full catalog upgrade, across the row-count threshold that picks between catalog paths, or across servers differing in range support. And nothing remapped `config.featureCodes` when the catalog changed: `remapRowFeatureCodes` is core-only and applies to ROW codes, so a stored selection could silently come back meaning a different gene. `featureColorOverrides` was already name-keyed for exactly this reason. This brings the selection into line. - `PointsLayerConfig.featureNames?: string[]` is the durable, serialized form and what the UI writes; `featureCodes` stays for runtime use and for configs written before this, with names taking precedence. - `resolveFeatureSelectionCodes(selection, catalog)` in core resolves names to whatever codes the CURRENT catalog uses, so everything downstream keeps working in codes. `featureNamesForCodes` is the inverse, for writing selections back. - `usePointsFeatureState` now accepts the config (or an array, still, as already-resolved codes) and resolves internally against the catalog it already reads — callers cannot resolve first, since the catalog comes FROM this hook. Unresolvable names (no catalog yet) yield an EMPTY selection, not `undefined`: resolving to "no filter" would flash the whole dataset before the catalog settles. It self-corrects, because `loadedDataRevision` bumps on every resolver settle and re-runs the planning effect. Names absent from the catalog are dropped rather than coerced — a saved config may name genes this element does not have. Verified in-app on merfish: selecting features writes `featureNames: ["VISp_I","VISp_V"]` and renders exactly those bands in their swatch colours. Tests cover the round-trip, the renumbering case the change exists for (showing the code form silently changing meaning), unknown names, the loading window, and names-beat-legacy-codes.
The points feature-filter and colour API was undocumented for the audience that needs it: neither the headless viewer guide nor the MDV release checklist mentioned featureNames, colorByFeature, featureColorOverrides, or PointsFeatureStateProvider. Everything written about it lived in docs/plans and docs/adr, which are design records rather than integration docs. Headless viewer guide gains a "Points: feature selection and colour" section parallel to the existing shapes one: the serializable config fields, and a worked example of building a feature UI from PointsFeatureStateProvider + usePointsFeatureState with each returned field annotated. Both docs lead with the serialization gotcha, because it is the one thing a host can get wrong invisibly: a points element frequently has NO feature-code column in the file (a Xenium `transcripts` carries `feature_name` only), so codes are assigned by the application while building the catalog and are not stable across the preview→full upgrade, across catalog paths, or across servers. `featureNames` is the durable field; `featureCodes` is runtime/back-compat and must not be persisted. Also documented, since both shape host UI design: the catalog arrives in two stages (preview then authoritative, distinguished by `catalogRefining`), and selecting a feature outside the resident window triggers a whole-dataset scan whose progress is exposed via `matchingLoadState`. Release checklist gains the matching "Points v1.1" section and its layer-scope row now says what is actually done (config-driven selection and colour) versus what is not (tiling/index strategy for very large elements). Docs build passes. The two broken anchors it reports are pre-existing in mdv-integration and untouched here.
`supportsParquetStreaming()` required `window`, which is absent in a Worker — so the URL-streaming scan added in be64b65 could only run on the main thread, moving the parquet decode there (789ms -> 1459ms of main-thread time). But the reader needs only `fetch` and WASM, both of which a Worker has: `window` was standing in for "is a browser" and excluded workers by accident. It now accepts a worker scope explicitly, probing `WorkerGlobalScope` (reliable across classic and module workers, unlike `importScripts`). The Node exclusion stays: the reader's async fetch path panics there with an unreachable that escapes try/catch. The scan request grows a stream variant — `streamUrl` + `streamRowGroups` + `streamColumns` — so the worker fetches only the projected columns itself. One request covers a row-group window chosen by the caller, so progress stays granular without the protocol needing streamed responses: each response is simply a chunk. `ParquetFile` is cached per URL in the worker, since `fromUrl` reads the footer and one scan issues several requests against the same file. Xenium transcripts, 12.1M rows, selecting CYP2B6 (568,863 points): byte-oriented worker path 3223-4027ms main-thread stream 2289ms (main thread 1459ms) worker stream 1045ms (main thread 503ms) `batchSize` turned out to be load-bearing and I had omitted it: without it the same code took 9204ms with a 2-row-group window and 3550ms with one request per part, i.e. the per-stream setup cost dominated. With it, the narrow window is both faster and keeps fine-grained progress. Capability tests pin the scope check across worker, browser-main-thread, neither, no-fetch, and Node-with-a-fetch-polyfill, because getting it wrong is silent: too strict and the scan quietly decodes on the main thread, too loose and it panics unrecoverably under Node.
The punchlist entry captures the mechanism while it is fresh: a fallback catalog for a dict-only element cannot produce counts, and ensureFeatureCatalog never re-requests a settled 'full' catalog, so the failure is permanent and immune to remounting the panel. Deliberately recorded rather than fixed, since it has not been reproduced deterministically yet and forcing serverSupportsStreamingRanges false should give a reproduction to fix against.
|
Warning Review limit reached
Next review available in: 16 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 Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThe PR adds durable name-based points selection, progressive Parquet scanning, slot-based request lifecycles, stable streaming render resources, feature-color palette rendering, UI controls, documentation, tests, and release configuration updates. ChangesPoints data and request lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…faster The previous wording implied a speed-up of something that already worked. On main a feature selection on a multi-million-row element frequently never resolved at all, so the timings are evidence that it completes, not the headline.
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/vis/src/SpatialCanvas/PointsFeatureFilterPanel.tsx (1)
415-421: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRow key is the feature code, which this PR documents as unstable.
PointsLayerConfig.featureNamesexists precisely because codes are app-assigned and get remapped when the preview catalog is superseded by the full one. Usingentry.codeas the React key therefore lets a remap reuse a row's DOM/state for a different gene;entry.nameis the stable identity here.Also, the highlight is wired to
onMouseEnter/onMouseLeaveonly — addingonFocus/onBlurgives keyboard users the same emphasis.♻️ Proposed change
- key={entry.code} + key={entry.name} style={{ ...checkboxLabelStyle, opacity: featureRowOpacity(state) }} title={title} onMouseEnter={() => setHighlightedFeature(entry.code)} onMouseLeave={() => setHighlightedFeature(null)} + onFocus={() => setHighlightedFeature(entry.code)} + onBlur={() => setHighlightedFeature(null)}🤖 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/PointsFeatureFilterPanel.tsx` around lines 415 - 421, Update the rendered feature row in the PointsFeatureFilterPanel mapping to use entry.name as the React key instead of the unstable entry.code, and add matching onFocus/onBlur handlers that set and clear the highlighted feature so keyboard focus receives the same emphasis as pointer hover.packages/core/src/engine/PointsResolver.ts (1)
257-294: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon’t re-plan failed points resources on every reconcile.
SpatialEntryStoreonly in-flight-dedupe per task id; oncerequest()rejects, the task id is cleared andplan()re-emits it because the failed slot is notidle,loading, orready. Storeattemptedstate beforeload()starts a task, and make retry explicit throughPointsResolver.retry()(or a retryable flag) so failed rows are not continuously re-run.🤖 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 257 - 294, Update the task planning and execution flow in PointsResolver, including the rowCodes and matching resources, so each task records an attempted state before load starts and a rejected task is not re-emitted during subsequent reconciles. Preserve normal idle/loading/ready behavior, and make retries explicit through PointsResolver.retry() or an equivalent retryable mechanism that clears or resets the attempted state.Source: Learnings
🧹 Nitpick comments (22)
packages/layers/src/pointsScatterLayer.ts (2)
144-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the commented-out
getRadius/updateTriggerslines.Radius now rides
radiusScale; leaving the dead accessor and trigger block invites someone to re-enable a stale contract.Also applies to: 155-157
🤖 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/pointsScatterLayer.ts` at line 144, Remove the commented-out getRadius and updateTriggers lines around the points scatter layer configuration, leaving radius controlled solely through radiusScale and eliminating the stale accessor/trigger block.
29-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
as unknown as ArrayLike<number>double assertion.
Matrix4from@math.gl/core@4.1.0is typed with numeric array indexing and providesgetScale(), so directmatrix[0]/matrix[4]reads and thetypeofprobe are unnecessary. If a tighter semantic match is desired,matrix.getScale()returns the 3D scale vector.🤖 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/pointsScatterLayer.ts` around lines 29 - 42, Update modelMatrixUniformScale to remove the double assertion and use Matrix4’s typed numeric indexing directly, eliminating the unnecessary typeof probe. Preserve the existing scaleX/scaleY geometric-mean calculation and fallback behavior, or use matrix.getScale() if it maintains the same intended 3D scale semantics.Source: Coding guidelines
packages/layers/src/PointsLayer.ts (1)
226-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePost-load filter block is duplicated with
ensurePreloadedBatch.Lines 226-239 repeat lines 193-206 verbatim. Extract a small private helper (e.g.
filterCurrentBatch(batch)) so the two entry points cannot drift.🤖 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/PointsLayer.ts` around lines 226 - 239, The post-load filtering logic is duplicated between the current block and ensurePreloadedBatch. Extract it into a private helper such as filterCurrentBatch(batch), including the awaitingRowCodes check and ensureFilteredBatch call with filterBatchSignature, then invoke that helper from both entry points.packages/layers/src/preloadedScatterStrategy.ts (1)
51-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParsing the signature with
split('|')[0]couples this module tofilterBatchSignature's string layout.If the segment order/separator in
pointsFeatureCodes.tsever changes, this silently degrades into the "wrong gene shown" bug it exists to prevent — no compiler help. Export a small accessor (e.g.geneSegmentOf(signature)) next tofilterBatchSignature, or store the gene signature alongsidefilteredBatchSignaturein layer state.🤖 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/preloadedScatterStrategy.ts` around lines 51 - 55, Decouple preloadedScatterStrategy from the serialized filterBatchSignature layout by exporting and reusing a dedicated gene-signature accessor defined alongside filterBatchSignature. Update the staleGeneSignature calculation in the filtered-batch reuse check to call that accessor instead of split('|')[0], preserving the existing comparison behavior.packages/vis/tests/useLayerData.spec.tsx (2)
293-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
scanPointsElementandcoverableElementare near-identical fixtures.They differ only in the
loadPointsMatchingFeatureCodespredicate. One factory taking asettlesFor: (codes) => booleanpredicate would keep the two behaviours honest and the resident/matched shapes in sync.Also applies to: 375-405
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vis/tests/useLayerData.spec.tsx` around lines 293 - 322, Refactor the duplicated scan fixtures, including scanPointsElement and coverableElement, into a shared factory that accepts a settlesFor predicate for loadPointsMatchingFeatureCodes. Keep the resident and matched data shapes, feature metadata, and other behavior synchronized while preserving each fixture’s distinct settling behavior.
337-341: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueHoist the repeated test helpers
type LoadAllResource,baseResource, andbaseRowCountare declared in both suites (@line 337/@line 420). Move them to module scope so the duplicated local declarations don’t clutter the tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vis/tests/useLayerData.spec.tsx` around lines 337 - 341, Hoist the shared LoadAllResource type and the baseResource and baseRowCount helpers to module scope in useLayerData.spec.tsx, adapting them to accept the required test state or result reference. Remove both duplicated declarations from the suites while preserving their current behavior and usages.packages/layers/tests/pointsRenderAttributes.spec.ts (1)
52-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider one more case:
use3dflip must yield a new wrapper.The doc comment promises stability per
(batch, use3d, colorByFeature), but only the colour dimension is pinned. AbuildPointsDeckData(b, true, false)vs(b, false, false)assertion would lock the positions-rebuild path.🤖 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/pointsRenderAttributes.spec.ts` around lines 52 - 89, Extend the “identity stability” tests around buildPointsDeckData to assert that changing use3d produces distinct wrappers for the same batch and color mode, while repeated calls with each use3d value remain stable. Keep the assertion focused on the positions-rebuild path and the documented (batch, use3d, colorByFeature) identity contract.packages/vis/src/SpatialCanvas/useLayerData.ts (1)
608-620: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueHoist the
featureCodesresolver for readability.The points resolver itself keys matching loads by feature-code value, not array identity, so the fresh array shouldn’t cause replanning by reference. Hoisting the inline
(() => ...)()into a namedfeatureCodesvariable would still be a simple readability cleanup.🤖 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 608 - 620, In the surrounding layer-data logic, hoist the inline feature selection resolution into a named featureCodes variable before constructing config, using resolveFeatureSelectionCodes with config and pointsEngine.getFeatureCatalog(elem.key). Spread featureCodes into the config only when it is defined, preserving the existing featureCodes behavior while removing the inline IIFE.packages/vis/src/SpatialCanvas/PointsFeatureState.tsx (1)
184-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the
as PointsFeatureSelectionassertion.
Array.isArrayalready narrows the false branch toPointsFeatureSelection | undefined, soselection ?? {}types fine on its own ({}satisfies the all-optional interface). If it doesn't compile, a small type guard is preferable to the assertion.As per coding guidelines: "Avoid type assertions (
as ...) in TypeScript when a library overload, local type guard, schema parser, discriminated union, or narrower API contract can express the same fact".♻️ Proposed change
const featureCodes = Array.isArray(selection) ? selection - : resolveFeatureSelectionCodes( - (selection ?? {}) as PointsFeatureSelection, - engine.getFeatureCatalog(key) - ); + : resolveFeatureSelectionCodes(selection ?? {}, engine.getFeatureCatalog(key));🤖 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/PointsFeatureState.tsx` around lines 184 - 189, Remove the `as PointsFeatureSelection` assertion in the `resolveFeatureSelectionCodes` call within the `featureCodes` initialization. Pass `selection ?? {}` directly, relying on the existing `Array.isArray` narrowing; if type checking still fails, introduce a small local type guard rather than restoring the assertion.Source: Coding guidelines
packages/layers/src/pointsFeatureColor.ts (1)
93-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale doc:
widthis nevermaxCode + 1now.
featurePaletteWidthfloors atDEFAULT_FEATURE_PALETTE_WIDTH, so "Texture width = number of codes covered (maxCode + 1). Always ≥ 1" contradicts the implementation.📝 Suggested wording
- /** Texture width = number of codes covered (`maxCode + 1`). Always ≥ 1. */ + /** Texture width = number of codes covered. Always ≥ {`@link` DEFAULT_FEATURE_PALETTE_WIDTH}; + * widened past it only when the catalog's code space is larger. */ width: number;🤖 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/pointsFeatureColor.ts` around lines 93 - 98, Update the FeaturePalette.width documentation to describe the actual featurePaletteWidth behavior: it is the palette texture width, at least DEFAULT_FEATURE_PALETTE_WIDTH, rather than necessarily maxCode + 1. Keep the existing data documentation unchanged.packages/layers/src/engine/PointsDataEngine.ts (1)
295-325: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSingle-slot memo per element key thrashes if two layers share one element with different overrides.
overrideMapMemoholds one entry per element key, but the memo input (overridesByName) is per-layer config. Two points layers over the same element with different overrides alternate cache misses everygetLayers()frame, producing a new map identity each time — which the extension treats as an overrides change and rebuilds the palette texture per frame. Keying the memo by layer id (or storing a small per-source cache) avoids it.🤖 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 295 - 325, The getFeatureColorOverrideMap memoization currently stores only one overridesByName source per element key, causing layers with different overrides to thrash the cache and recreate maps each frame. Update overrideMapMemo and getFeatureColorOverrideMap to retain separate cached results per layer/source identity while still validating the catalog, so alternating layers reuse their existing map identities.packages/layers/src/pointsFeatureColorExtension.ts (1)
146-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated inline
ascasts for the same prop/state shape.
{ featureCodeSpaceSize?: number; featureColorOverrides?: FeatureColorOverrides | null }is asserted in three places, andstate/setShaderModulePropsin two more. Declaring the shapes once (e.g.interface PfcProps/PfcState) and asserting at a single boundary with the existing "deck typing doesn't expose extension props" rationale keeps the assertions local and documented.As per coding guidelines: "if an assertion is unavoidable at an external boundary, keep it local and add a short comment explaining why the compiler cannot prove it".
🤖 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/pointsFeatureColorExtension.ts` around lines 146 - 198, Define reusable PfcProps and PfcState shapes for the extension-specific props and state, then replace the repeated inline assertions in the initialization, updateState, and draw methods with those types. Keep each unavoidable cast local to the deck typing boundary and add a brief comment explaining why the compiler cannot infer the extension fields; also reuse a named type for the setShaderModuleProps boundary instead of an inline assertion.Source: Coding guidelines
packages/core/tests/pointsCatalogSupersession.spec.ts (1)
128-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Array.frominstead of the double cast.
rowCodesis typedArrayLike<number>, and the resolver can legitimately hand back a plain array (the cap-shed path inensureLoadedusesArray.prototype.slice), so asserting toInt32Arrayto make the spread type-check is both unnecessary and potentially untrue. As per coding guidelines: "Avoid type assertions (as); usesatisfies,as const, discriminated unions, and small helpers that return precise types."♻️ Suggested change
- expect([...(rowCodes as ArrayLike<number> as Int32Array)]).toEqual( - codesIn(catalog as PointsFeatureCatalog, ROW_NAMES) - ); + expect(Array.from(rowCodes ?? [])).toEqual(codesIn(catalog as PointsFeatureCatalog, ROW_NAMES));🤖 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/pointsCatalogSupersession.spec.ts` around lines 128 - 132, Update the rowCodes assertion in the points catalog test to convert the ArrayLike<number> result with Array.from instead of the double cast to Int32Array. Preserve the existing equality check against codesIn(catalog as PointsFeatureCatalog, ROW_NAMES) and remove the unnecessary type assertion.Source: Coding guidelines
packages/core/tests/pointsResolver.spec.ts (1)
295-299: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
as neverdefeats the narrowing these assertions are testing.
Resolution.isFailedis a type guard; passingcatalog as neverthrows away the union so the guard proves nothing to the compiler (the subsequentif (catalog.status === 'failed')is doing the real work). Dropping the cast — or aligning the resource's declared type — keeps the test honest. As per coding guidelines: "Avoid type assertions (as); usesatisfies,as const, discriminated unions, and small helpers that return precise types."Also applies to: 569-571
🤖 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 295 - 299, Remove the `as never` assertion from the `Resolution.isFailed` calls in the catalog failure assertions, including the corresponding occurrence near the other referenced lines. Pass the declared catalog/resource value directly so the type guard narrows the discriminated union and the subsequent `catalog.status` and `catalog.error` checks validate the intended types.Source: Coding guidelines
packages/core/tests/vtableDirectoryResponse.spec.ts (1)
26-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
writeMultipartParquetFixtureis a verbatim copy of the one inpackages/core/tests/vtableMultipart.spec.ts.Same for the repeated
(source as unknown as { storeRoot: { store: … } }).storeRoot.storeunwrap (Lines 157, 181, 208, 229, 233). Extracting both into a shared test helper would keep the two specs from drifting and drop five identical casts.Also applies to: 157-157
🤖 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/vtableDirectoryResponse.spec.ts` around lines 26 - 52, The duplicated multipart Parquet fixture and repeated store-root unwraps should be centralized in shared test helpers. Extract writeMultipartParquetFixture into a reusable helper used by both vtableDirectoryResponse.spec.ts and vtableMultipart.spec.ts, and add a shared helper for the storeRoot.store access, replacing all five repeated casts while preserving existing behavior.packages/core/src/engine/RequestSlot.ts (1)
93-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
promiseplaceholder assertion is avoidable.
undefined as unknown as Promise<void>lies about the type for the window between construction and Line 241; making the field optional (promise?: Promise<void>) expresses the same fact honestly and costs only the?? Promise.resolve()you already do at call sites. As per coding guidelines: "Avoid type assertions (as); usesatisfies,as const, discriminated unions, and small helpers that return precise types."♻️ Suggested change
interface InFlight<K> { readonly key: K; readonly controller: AbortController; - /** Assigned synchronously right after construction; never observed before then. */ - promise: Promise<void>; + /** Assigned synchronously right after construction; never observed before then. */ + promise?: Promise<void>; }- const record: InFlight<K> = { - key, - controller, - promise: undefined as unknown as Promise<void>, - }; + const record: InFlight<K> = { key, controller };Also applies to: 201-205
🤖 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/RequestSlot.ts` around lines 93 - 98, Update the InFlight.promise field to be optional instead of using an undefined-to-Promise type assertion, and adjust its initialization and access sites—including the logic around the InFlight construction and the additional references noted near lines 201–205—to handle the absent value via the existing Promise.resolve fallback. Remove the placeholder assertion while preserving synchronous assignment before normal observation.Source: Coding guidelines
packages/core/src/engine/PointsResolver.ts (1)
671-689: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCap-shed leaves
rowCodeskeyed at the old cap when the codes are already short.The
settle(memoryCap, …)on Line 681 only fires whencodes.length > memoryCap. OtherwiserowCodes.settledKeystays at the previous (larger) cap whilepreload.settledKeybecomesmemoryCap, so the nextensureRowFeatureCodessees a mismatch and re-requests codes that are already correct. Re-settling at the new key unconditionally would keep the two slots aligned.🤖 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 671 - 689, Update the cap-shed branch in PointsResolver around slot.settle and entry.rowCodes so rowCodes is always re-settled at memoryCap, even when codes.length is already at or below the cap. Preserve slicing when codes exceed the cap, but otherwise settle the existing codes unchanged to keep rowCodes.settledKey aligned with the preload slot.packages/core/src/pointsFeatures.ts (1)
318-345: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a justification comment for the
chunk.values as ArrayLike<number>assertion.The dictionary fast path is well documented for why it exists, but the type assertion at Line 334 itself has no note on why TypeScript can't prove the element type here (
chunk: Datais unparameterized, sovaluescan't be narrowed from thedictionarycheck alone). A short inline comment would satisfy the "keep it local and explain why" guidance for unavoidable assertions.✏️ Suggested comment
const indices = chunk.values as ArrayLike<number>; + // `chunk` is an unparameterized `Data`, so TS can't narrow `values`'s element + // type from the `dictionary` check above; dictionary indices are always an + // integer-typed array here.As per coding guidelines: "if an assertion is unavoidable at an external boundary, keep it local and add a short comment explaining why the compiler cannot prove it".
🤖 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/pointsFeatures.ts` around lines 318 - 345, Add a short inline comment immediately above the `chunk.values as ArrayLike<number>` assertion in `writeChunkFeatureCodes`, explaining that `chunk` is an unparameterized `Data` and TypeScript cannot infer the numeric element type from the `dictionary` check, so the local assertion is required.Source: Coding guidelines
packages/core/src/workers/points-worker.ts (1)
290-290: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a guard over
request.streamUrl as string.The caller already checked
if (request.streamUrl)before invoking this function (Line 358), but that narrowing doesn't cross the call boundary. A local guard lets TypeScript narrow without an assertion.✏️ Suggested guard
- const url = request.streamUrl as string; + const { streamUrl: url } = request; + if (!url) { + throw new Error('scanStreamByFeatureCodes called without streamUrl'); + }As per coding guidelines: "Avoid type assertions (
as ...) in TypeScript when a library overload, local type guard... can express the same fact".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/workers/points-worker.ts` at line 290, Replace the type assertion in the worker function containing the stream URL handling with a local guard that checks request.streamUrl before using it. Return or otherwise handle the missing URL according to the function’s existing control flow, then let TypeScript narrow the value for subsequent use without `as string`.Source: Coding guidelines
packages/core/src/models/VPointsSource.ts (1)
709-724: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing axis column silently yields (0,0) points, unlike the feature column.
A missing feature column bails out (line 720-724), but a missing axis column
continues and still advancesfilledbyrows— leaving that axis at zero for every streamed point with no warning. Since the projection requestsaxisNames, the only way to hit this is a schema mismatch, and silently placing every point at the origin is worse than falling through to the one-shot decode.♻️ Suggested handling
for (let axis = 0; axis < axisCount; axis += 1) { const column = table.getChild(axisNames[axis]); if (!column) { - continue; + // Same reasoning as the missing feature column below: geometry we + // cannot fill is not partially usable. + return filled > 0 ? snapshot() : null; }🤖 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 709 - 724, Update the axis-column handling in the streaming decode loop around axisBuffers and featureColumn so any missing requested axis column aborts the streaming path and falls back to the existing one-shot decode, rather than continuing and advancing filled with zero-valued coordinates. Preserve the existing feature-column fallback behavior and ensure no partial streamed points are returned for this schema mismatch.packages/core/tests/pointsWorkerScan.spec.ts (1)
258-324: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood chunk-boundary coverage — consider adding the
scanMortonTableInBoundscounterpart.This pins
scanTableByFeatureCodesacross chunks, butscanMortonTableInBoundsgot the samenumericColumnValuesrewrite and has an extra behaviour that changed: the feature-code filter is now skipped entirely when the code column is unavailable (see the note onpointsWorkerScan.tslines 469-475). A case with a filter active and no code column would lock that polarity down.Happy to draft it if useful.
🤖 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/pointsWorkerScan.spec.ts` around lines 258 - 324, Add a multi-chunk test for scanMortonTableInBounds matching the existing scanTableByFeatureCodes coverage, using an active feature-code filter while omitting the code column. Assert that filtering is skipped and all in-bounds rows are returned, including rows from the later chunk, preserving chunk-boundary coordinate indexing.packages/core/src/workers/pointsWorkerProtocol.ts (1)
104-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider modelling the stream variant as a union member rather than three optional siblings.
As written,
streamUrl/parts/rowGroupsare all optional on the same shape, so "mutually exclusive" lives only in the doc comment — and the worker consequently narrows withrequest.streamUrl as string. A nested discriminated shape (e.g.stream?: { url: string; rowGroups?: number[]; columns?: string[] }, or asourcediscriminant) would make the exclusivity checkable and remove that assertion.Not blocking — the runtime guard at the dispatch site is correct today.
🤖 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/pointsWorkerProtocol.ts` around lines 104 - 118, Model the stream variant in the worker request protocol as a discriminated union or nested stream object so it is mutually exclusive with the existing parts/rowGroups payload. Update the dispatch logic and related references to narrow through that typed variant, removing the `request.streamUrl as string` assertion while preserving the current runtime guard and stream fields (`streamRowGroups` and `streamColumns`).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 `@docs/docs/vis/mdv-release-checklist.mdx`:
- Around line 121-127: Document the remaining dictionary-only count failure as
an unresolved v1.1 release risk. Update docs/docs/vis/mdv-release-checklist.mdx
lines 121-127, .changeset/points-feature-scan-and-name-selection.md lines 25-31,
and docs/docs/vis/headless-viewer.mdx lines 342-348 to state that fallback
catalogs may settle successfully while counts remain unavailable, so counts must
not be treated as authoritative. Update docs/plans/resource-resolver-handoff.md
lines 37-38 to scope retryability to failed requests while retaining
successful-but-countless fallback as unresolved.
In `@docs/plans/resource-resolver-handoff.md`:
- Around line 39-43: Synchronize the D8 and D10 status between this handoff
document and docs/plans/points-redesign-punchlist.md. Either update the
punchlist to mark cancellation and the streaming-overlay flash as complete, or
narrow the claims here so both documents consistently describe the remaining
actionable work.
In `@packages/core/src/engine/PointsResolver.ts`:
- Around line 274-277: The rowCodes preload key can diverge from the active
memory-cap window. In packages/core/src/engine/PointsResolver.ts lines 274-277,
update the planning gate to check rowCodes readiness at the preload cap and
include that cap in the task id; in lines 671-689, unconditionally re-settle
rowCodes using the new cap key whenever shedding to a lower cap, regardless of
codes.length versus memoryCap.
In `@packages/core/src/models/VPointsSource.ts`:
- Around line 1935-1954: The helper tallyFeatureCodesFromColumn must skip Morton
sentinel rows consistently with the catalog builders. In
packages/core/src/models/VPointsSource.ts lines 1935-1954, pass the resolved
mortonColumn (or equivalent sentinel-aware flag) to the tally and remove the
unnecessary fallback after featureCodeMapFromCatalog(catalog); in lines
2057-2067, pass skipMortonSentinels: hasMortonColumn to the tally path, matching
accumulateFeatureCatalogFromTable.
In `@packages/core/src/models/VTableSource.ts`:
- Around line 745-786: Update serverSupportsStreamingRanges so transient probe
exceptions do not cache a false result in rangeProbeByOrigin: remove the failed
promise from the map when the probe’s fetch or body-read path throws, while
continuing to cache false for completed 206 responses with invalid byte lengths
or other definitive server responses. Use the existing evictIfCurrent pattern
elsewhere in SpatialDataTableSource if applicable, ensuring only the currently
cached probe is removed.
- Around line 309-318: Guard the metadata-cache promise peek in
orderedParquetCandidatePaths and the corresponding peek in
loadParquetSchemaBytes so rejected promises are caught and ignored, falling back
to getParquetCandidatePaths or the existing candidate path flow. Ensure this
optimization never propagates metadata-loading errors or changes the existing
null/error handling behavior of loadParquetBytes and loadParquetSchemaBytes.
In `@packages/core/src/workers/points-worker.ts`:
- Around line 279-299: Update the ParquetFile promise caching in
scanStreamByFeatureCodes so a rejected ParquetFile.fromUrl(url) promise is
removed from streamFilesByUrl, allowing later scans for the same URL to retry.
Preserve the existing successful-promise reuse behavior and only evict the cache
entry for the URL whose initialization failed.
In `@packages/core/src/workers/pointsWorkerScan.ts`:
- Around line 469-475: Update the feature-filter setup and loop around
rowMatchesFeatureCode so a supplied feature-code column that cannot be
materialised causes the scan to return no results explicitly, rather than
disabling filtering. After that guard, remove the featureCodeValues null-check
from the per-row predicate while preserving normal matching behavior.
In `@packages/layers/src/adapters/PointsRendererAdapter.ts`:
- Around line 232-254: Update buildGrowingResource so both loadAll and
loadInBounds use the current holder batch’s featureCodes. Pass featureCodes
inside the PointData object given to columnarBatchFromPointData, and avoid
closing loadInBounds over the initial base loader by resolving or using the
loader associated with holder.current, including its current base.featureCodes.
- Around line 185-223: Update getBaseResource and its growingBases cache entry
to retain the associated PointsElement, then detect when the requested element
differs from the cached one for the same key. Rebuild and replace the resource,
holder, and revision state for a different element before returning it, while
preserving the existing batch-update behavior for the same element.
In `@packages/layers/src/pointsFeatureColorExtension.ts`:
- Around line 40-44: Update destroyTexture to invoke destroy() when available
and only fall back to delete() when destroy() is absent, ensuring the palette
texture is disposed exactly once while retaining deprecated compatibility.
In `@packages/layers/src/PointsLayer.ts`:
- Around line 216-240: Update refreshPreloadedBatch to capture the current
resourceRevision before awaiting loadAll(), then verify that revision is still
current before applying the result. Skip setState({ preloadedBatch: batch }) and
subsequent filtering when a newer refresh has started, while preserving the
existing capability and batch-format guards.
In `@packages/vis/src/SpatialCanvas/PointsFeatureFilterPanel.tsx`:
- Around line 439-446: Update the color picker in the PointsFeatureFilterPanel
component so onChange only updates a local value for live preview, while
setColorOverride commits the selected color once on blur rather than on every
change event. Preserve the current color display and stopPropagation behavior,
and ensure the committed value reflects the latest locally selected color.
In `@packages/vis/src/SpatialCanvas/useLayerData.ts`:
- Around line 1029-1039: Update the useMatched branch around
getMatchingRowFeatureCodes and baseFilter so a strict coveredSelection never
reaches PointsLayer without row codes; when getMatchingRowFeatureCodes(elem.key)
is unavailable, defer or skip the matched base (or use the resident base)
instead of allowing the unfiltered matched batch to render. Preserve the current
filtering behavior once the row codes are present.
---
Outside diff comments:
In `@packages/core/src/engine/PointsResolver.ts`:
- Around line 257-294: Update the task planning and execution flow in
PointsResolver, including the rowCodes and matching resources, so each task
records an attempted state before load starts and a rejected task is not
re-emitted during subsequent reconciles. Preserve normal idle/loading/ready
behavior, and make retries explicit through PointsResolver.retry() or an
equivalent retryable mechanism that clears or resets the attempted state.
In `@packages/vis/src/SpatialCanvas/PointsFeatureFilterPanel.tsx`:
- Around line 415-421: Update the rendered feature row in the
PointsFeatureFilterPanel mapping to use entry.name as the React key instead of
the unstable entry.code, and add matching onFocus/onBlur handlers that set and
clear the highlighted feature so keyboard focus receives the same emphasis as
pointer hover.
---
Nitpick comments:
In `@packages/core/src/engine/PointsResolver.ts`:
- Around line 671-689: Update the cap-shed branch in PointsResolver around
slot.settle and entry.rowCodes so rowCodes is always re-settled at memoryCap,
even when codes.length is already at or below the cap. Preserve slicing when
codes exceed the cap, but otherwise settle the existing codes unchanged to keep
rowCodes.settledKey aligned with the preload slot.
In `@packages/core/src/engine/RequestSlot.ts`:
- Around line 93-98: Update the InFlight.promise field to be optional instead of
using an undefined-to-Promise type assertion, and adjust its initialization and
access sites—including the logic around the InFlight construction and the
additional references noted near lines 201–205—to handle the absent value via
the existing Promise.resolve fallback. Remove the placeholder assertion while
preserving synchronous assignment before normal observation.
In `@packages/core/src/models/VPointsSource.ts`:
- Around line 709-724: Update the axis-column handling in the streaming decode
loop around axisBuffers and featureColumn so any missing requested axis column
aborts the streaming path and falls back to the existing one-shot decode, rather
than continuing and advancing filled with zero-valued coordinates. Preserve the
existing feature-column fallback behavior and ensure no partial streamed points
are returned for this schema mismatch.
In `@packages/core/src/pointsFeatures.ts`:
- Around line 318-345: Add a short inline comment immediately above the
`chunk.values as ArrayLike<number>` assertion in `writeChunkFeatureCodes`,
explaining that `chunk` is an unparameterized `Data` and TypeScript cannot infer
the numeric element type from the `dictionary` check, so the local assertion is
required.
In `@packages/core/src/workers/points-worker.ts`:
- Line 290: Replace the type assertion in the worker function containing the
stream URL handling with a local guard that checks request.streamUrl before
using it. Return or otherwise handle the missing URL according to the function’s
existing control flow, then let TypeScript narrow the value for subsequent use
without `as string`.
In `@packages/core/src/workers/pointsWorkerProtocol.ts`:
- Around line 104-118: Model the stream variant in the worker request protocol
as a discriminated union or nested stream object so it is mutually exclusive
with the existing parts/rowGroups payload. Update the dispatch logic and related
references to narrow through that typed variant, removing the `request.streamUrl
as string` assertion while preserving the current runtime guard and stream
fields (`streamRowGroups` and `streamColumns`).
In `@packages/core/tests/pointsCatalogSupersession.spec.ts`:
- Around line 128-132: Update the rowCodes assertion in the points catalog test
to convert the ArrayLike<number> result with Array.from instead of the double
cast to Int32Array. Preserve the existing equality check against codesIn(catalog
as PointsFeatureCatalog, ROW_NAMES) and remove the unnecessary type assertion.
In `@packages/core/tests/pointsResolver.spec.ts`:
- Around line 295-299: Remove the `as never` assertion from the
`Resolution.isFailed` calls in the catalog failure assertions, including the
corresponding occurrence near the other referenced lines. Pass the declared
catalog/resource value directly so the type guard narrows the discriminated
union and the subsequent `catalog.status` and `catalog.error` checks validate
the intended types.
In `@packages/core/tests/pointsWorkerScan.spec.ts`:
- Around line 258-324: Add a multi-chunk test for scanMortonTableInBounds
matching the existing scanTableByFeatureCodes coverage, using an active
feature-code filter while omitting the code column. Assert that filtering is
skipped and all in-bounds rows are returned, including rows from the later
chunk, preserving chunk-boundary coordinate indexing.
In `@packages/core/tests/vtableDirectoryResponse.spec.ts`:
- Around line 26-52: The duplicated multipart Parquet fixture and repeated
store-root unwraps should be centralized in shared test helpers. Extract
writeMultipartParquetFixture into a reusable helper used by both
vtableDirectoryResponse.spec.ts and vtableMultipart.spec.ts, and add a shared
helper for the storeRoot.store access, replacing all five repeated casts while
preserving existing behavior.
In `@packages/layers/src/engine/PointsDataEngine.ts`:
- Around line 295-325: The getFeatureColorOverrideMap memoization currently
stores only one overridesByName source per element key, causing layers with
different overrides to thrash the cache and recreate maps each frame. Update
overrideMapMemo and getFeatureColorOverrideMap to retain separate cached results
per layer/source identity while still validating the catalog, so alternating
layers reuse their existing map identities.
In `@packages/layers/src/pointsFeatureColor.ts`:
- Around line 93-98: Update the FeaturePalette.width documentation to describe
the actual featurePaletteWidth behavior: it is the palette texture width, at
least DEFAULT_FEATURE_PALETTE_WIDTH, rather than necessarily maxCode + 1. Keep
the existing data documentation unchanged.
In `@packages/layers/src/pointsFeatureColorExtension.ts`:
- Around line 146-198: Define reusable PfcProps and PfcState shapes for the
extension-specific props and state, then replace the repeated inline assertions
in the initialization, updateState, and draw methods with those types. Keep each
unavoidable cast local to the deck typing boundary and add a brief comment
explaining why the compiler cannot infer the extension fields; also reuse a
named type for the setShaderModuleProps boundary instead of an inline assertion.
In `@packages/layers/src/PointsLayer.ts`:
- Around line 226-239: The post-load filtering logic is duplicated between the
current block and ensurePreloadedBatch. Extract it into a private helper such as
filterCurrentBatch(batch), including the awaitingRowCodes check and
ensureFilteredBatch call with filterBatchSignature, then invoke that helper from
both entry points.
In `@packages/layers/src/pointsScatterLayer.ts`:
- Line 144: Remove the commented-out getRadius and updateTriggers lines around
the points scatter layer configuration, leaving radius controlled solely through
radiusScale and eliminating the stale accessor/trigger block.
- Around line 29-42: Update modelMatrixUniformScale to remove the double
assertion and use Matrix4’s typed numeric indexing directly, eliminating the
unnecessary typeof probe. Preserve the existing scaleX/scaleY geometric-mean
calculation and fallback behavior, or use matrix.getScale() if it maintains the
same intended 3D scale semantics.
In `@packages/layers/src/preloadedScatterStrategy.ts`:
- Around line 51-55: Decouple preloadedScatterStrategy from the serialized
filterBatchSignature layout by exporting and reusing a dedicated gene-signature
accessor defined alongside filterBatchSignature. Update the staleGeneSignature
calculation in the filtered-batch reuse check to call that accessor instead of
split('|')[0], preserving the existing comparison behavior.
In `@packages/layers/tests/pointsRenderAttributes.spec.ts`:
- Around line 52-89: Extend the “identity stability” tests around
buildPointsDeckData to assert that changing use3d produces distinct wrappers for
the same batch and color mode, while repeated calls with each use3d value remain
stable. Keep the assertion focused on the positions-rebuild path and the
documented (batch, use3d, colorByFeature) identity contract.
In `@packages/vis/src/SpatialCanvas/PointsFeatureState.tsx`:
- Around line 184-189: Remove the `as PointsFeatureSelection` assertion in the
`resolveFeatureSelectionCodes` call within the `featureCodes` initialization.
Pass `selection ?? {}` directly, relying on the existing `Array.isArray`
narrowing; if type checking still fails, introduce a small local type guard
rather than restoring the assertion.
In `@packages/vis/src/SpatialCanvas/useLayerData.ts`:
- Around line 608-620: In the surrounding layer-data logic, hoist the inline
feature selection resolution into a named featureCodes variable before
constructing config, using resolveFeatureSelectionCodes with config and
pointsEngine.getFeatureCatalog(elem.key). Spread featureCodes into the config
only when it is defined, preserving the existing featureCodes behavior while
removing the inline IIFE.
In `@packages/vis/tests/useLayerData.spec.tsx`:
- Around line 293-322: Refactor the duplicated scan fixtures, including
scanPointsElement and coverableElement, into a shared factory that accepts a
settlesFor predicate for loadPointsMatchingFeatureCodes. Keep the resident and
matched data shapes, feature metadata, and other behavior synchronized while
preserving each fixture’s distinct settling behavior.
- Around line 337-341: Hoist the shared LoadAllResource type and the
baseResource and baseRowCount helpers to module scope in useLayerData.spec.tsx,
adapting them to accept the required test state or result reference. Remove both
duplicated declarations from the suites while preserving their current behavior
and usages.
🪄 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 Plus
Run ID: b713ff71-4078-46ac-89c2-d0c4e770829d
📒 Files selected for processing (57)
.changeset/points-feature-scan-and-name-selection.md.claude/launch.jsondocs/docs/vis/headless-viewer.mdxdocs/docs/vis/mdv-release-checklist.mdxdocs/plans/points-redesign-punchlist.mddocs/plans/resource-resolver-handoff.mdpackages/core/src/engine/PointsResolver.tspackages/core/src/engine/RequestSlot.tspackages/core/src/engine/index.tspackages/core/src/index.tspackages/core/src/models/VPointsSource.tspackages/core/src/models/VTableSource.tspackages/core/src/models/index.tspackages/core/src/parquetWasmLoader.tspackages/core/src/pointsFeatures.tspackages/core/src/pointsLoadOptions.tspackages/core/src/workers/points-worker.tspackages/core/src/workers/pointsWorkerClient.tspackages/core/src/workers/pointsWorkerProtocol.tspackages/core/src/workers/pointsWorkerScan.tspackages/core/tests/parquetStreamingScope.spec.tspackages/core/tests/parquetWorkerPayload.spec.tspackages/core/tests/pointsCatalogSupersession.spec.tspackages/core/tests/pointsFeatureSelection.spec.tspackages/core/tests/pointsFeatureStreamingCatalog.spec.tspackages/core/tests/pointsFeatures.spec.tspackages/core/tests/pointsPreloadStreaming.spec.tspackages/core/tests/pointsResolver.spec.tspackages/core/tests/pointsRowFeatureCodes.spec.tspackages/core/tests/pointsScanStreamGate.spec.tspackages/core/tests/pointsWorkerScan.spec.tspackages/core/tests/requestSlot.spec.tspackages/core/tests/vtableDirectoryResponse.spec.tspackages/core/tests/vtableMultipart.spec.tspackages/layers/src/PointsLayer.tspackages/layers/src/adapters/PointsRendererAdapter.tspackages/layers/src/engine/PointsDataEngine.tspackages/layers/src/pointsFeatureColor.tspackages/layers/src/pointsFeatureColorExtension.tspackages/layers/src/pointsRenderAttributes.tspackages/layers/src/pointsScatterLayer.tspackages/layers/src/preloadedScatterStrategy.tspackages/layers/tests/pointsDataEngine.spec.tspackages/layers/tests/pointsFeatureColor.spec.tspackages/layers/tests/pointsFeatureColorExtension.spec.tspackages/layers/tests/pointsRenderAttributes.spec.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/index.tsxpackages/vis/src/SpatialCanvas/public.tspackages/vis/src/SpatialCanvas/types.tspackages/vis/src/SpatialCanvas/useLayerData.tspackages/vis/tests/useLayerData.spec.tsx
💤 Files with no reviewable changes (1)
- packages/vis/src/SpatialCanvas/index.tsx
Review pass on #89. All three are the same shape: a cache that cannot tell "the answer is no" from "the request did not complete", and so remembers a blip as a verdict. - The range probe caches per ORIGIN for the life of the page, and a false demotes every element on that origin to whole-file reads. A thrown fetch is not an answer, so it is no longer cached; a real HTTP refusal (416, or a 200 that ignored Range) still is. This is the most likely mechanism behind the intermittent "feature counts never settle" noted in the punchlist: one dropped request during startup and the streaming path is gone until a hard reload, with nothing in the log to say so because the fallback looks healthy. - The worker's ParquetFile.fromUrl cache kept a rejected promise, so a single transient footer read poisoned that URL for the life of the worker. Evict on rejection, matching what loadParquetDatasetMetadata already does. - Two peeks at the resolved part layout awaited the cached promise bare. In loadParquetBytes the await sits in the for..of header, so a rejection escaped the per-candidate try/catch that exists to keep probing — turning a loadable single-file element into a hard error instead of a fall back to the blind candidate order. A peek must never be load-bearing. Both new specs verified red against the unfixed code first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tiled scan looks its feature-code column up by name. When that column is absent from the decoded chunk the filter cannot be applied at all, and the predicate carried the column check as a conjunct — so it was false for every row and the caller got the whole chunk back. One gene selected, the entire dataset drawn in that gene's colour, nothing logged. Bail before the loop instead. An empty result is still wrong, but it is wrong visibly: "my gene has no points" is a bug report, four million plausible points are not. Raised in review on #89 as a regression from the column hoist (e5c9033). It is not — `featureCodeColumn &&` was there before the hoist and the hoist preserved it exactly. The behaviour is worth closing regardless. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Triage of the review. Four acted on, the rest triaged but not yet verified — flagging that split explicitly rather than implying a clean sweep. Fixed (8d1a875, 384931f)Three of the four are one bug wearing three hats: a cache that cannot tell "the answer is no" from "the request did not complete", and so remembers a blip as a verdict.
Each fix has a spec verified red against the unfixed code first. Full suite, Biome and build green. Triaged, not yet verifiedThe remaining Major items are all plausible and none are cheap to confirm by reading:
These need a real dataset in the browser to distinguish a genuine ordering hazard from one the surrounding gates already exclude, so they are for the next pass rather than something to wave through now. The minors (luma On the docstring-coverage warningPushing back on this one. 47.6% is measured against a threshold that counts helpers like |
The suspected cause named in this entry turned out to be the actual one. Noting that the fix makes the failure rarer without making it recoverable, so the item stays open on its own terms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The matched base is chosen when the last-good scan covers any still-wanted gene, and drawn filtered to that covered subset. The comment claimed this "can only ever DROP a deselected gene — never surface an unselected one", which is true of the filter as APPLIED but says nothing about whether it is applied at all. `PointsLayer` cannot filter without row-aligned codes, and `preloadedScatterStrategy` resolves that case by drawing the batch WHOLE. So a matched batch carrying no row codes plus a strict-subset filter is not a no-op: it puts every covered gene back on screen, including the one just deselected. `pointsScanChunkProgress` only sets `featureCodes` when the scan produced them, so a codeless matched batch is a shape the resolver can hand back. Gate the matched base on the layer's own `featureFilterAwaitingRowCodes` — imported from @spatialdata/layers rather than re-derived, so the check and the short-circuit it defends against cannot drift — and fall back to the resident base, which filters in memory from codes it does have. Raised in review on #89. Spec verified red: the narrowed selection drew 4 rows (both genes) instead of 3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/tests/vtableLayoutPeek.spec.ts (1)
46-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep test doubles type-checked instead of asserting away their contracts.
The constructor config needs a typed fixture instead of
as never, and the response doubles should use a minimal structural type, e.g.satisfies { status: number; arrayBuffer(): Promise<ArrayBuffer> }, rather than pretending to beResponse. For the 416 spy, return{ status: 416 }directly instead of casting that object toResponse; the public API accepts anyResponse-like object, so a fullResponseis not required to preserve behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/tests/vtableLayoutPeek.spec.ts` around lines 46 - 52, Replace the constructor configuration cast to never with a properly typed fixture, and type response doubles structurally using only status and arrayBuffer rather than casting them to Response. Apply this in packages/core/tests/vtableLayoutPeek.spec.ts lines 46-52 and packages/core/tests/vtableRangeProbeCache.spec.ts lines 27-28 and 36-40; in packages/core/tests/vtableRangeProbeCache.spec.ts line 62, update the 416 spy to return the minimal { status: 416 } shape without a Response cast.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-feature-scan-and-name-selection.md:
- Around line 17-25: Update the release note’s claim that feature scans “now
complete” to explicitly qualify the known limitation: dictionary-only elements
using a fallback catalog may still leave feature counts permanently unsettled.
Preserve the documented streaming, worker, and performance improvements while
clarifying that this fallback-catalog path is not fully resolved.
In `@packages/core/tests/pointsMortonScanFilter.spec.ts`:
- Around line 15-29: Update the scan helper to type columns with the supported
Arrow column value type and restrict over to featureCodeColumnName and
featureCodes. Remove both as never assertions, construct the table and scan
options with types accepted by scanMortonTableInBounds, and prevent over from
overriding required fields such as table, bounds, buffers, or axis
configuration.
---
Nitpick comments:
In `@packages/core/tests/vtableLayoutPeek.spec.ts`:
- Around line 46-52: Replace the constructor configuration cast to never with a
properly typed fixture, and type response doubles structurally using only status
and arrayBuffer rather than casting them to Response. Apply this in
packages/core/tests/vtableLayoutPeek.spec.ts lines 46-52 and
packages/core/tests/vtableRangeProbeCache.spec.ts lines 27-28 and 36-40; in
packages/core/tests/vtableRangeProbeCache.spec.ts line 62, update the 416 spy to
return the minimal { status: 416 } shape without a Response cast.
🪄 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 Plus
Run ID: 3be17bff-d073-4fca-a500-7ff9f1037cc6
📒 Files selected for processing (11)
.changeset/points-feature-scan-and-name-selection.mddocs/plans/points-redesign-punchlist.mdpackages/core/src/models/VTableSource.tspackages/core/src/workers/points-worker.tspackages/core/src/workers/pointsWorkerScan.tspackages/core/tests/pointsMortonScanFilter.spec.tspackages/core/tests/vtableLayoutPeek.spec.tspackages/core/tests/vtableRangeProbeCache.spec.tspackages/layers/src/index.tspackages/vis/src/SpatialCanvas/useLayerData.tspackages/vis/tests/useLayerData.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/vis/tests/useLayerData.spec.tsx
- packages/vis/src/SpatialCanvas/useLayerData.ts
- docs/plans/points-redesign-punchlist.md
- packages/core/src/workers/points-worker.ts
- packages/core/src/models/VTableSource.ts
- packages/core/src/workers/pointsWorkerScan.ts
| function scan(columns: Record<string, unknown>, over: Record<string, unknown> = {}) { | ||
| const xs = new Float32PointBuffer(); | ||
| const ys = new Float32PointBuffer(); | ||
| const zs = new Float32PointBuffer(); | ||
| scanMortonTableInBounds({ | ||
| table: tableFromArrays(columns as never), | ||
| rowGroupIndex: 1, // past the sentinel window, so no rows are skipped for it | ||
| bounds: { minX: -1e6, minY: -1e6, maxX: 1e6, maxY: 1e6 }, | ||
| axisNames: ['x', 'y'], | ||
| mortonCodeColumnName: 'morton', | ||
| xs, | ||
| ys, | ||
| zs, | ||
| ...over, | ||
| } as never); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the as never assertions and narrow the helper contract.
as never disables validation for both the Arrow table input and the scan options. Since over accepts arbitrary keys and is spread last, future tests could silently replace table, bounds, buffers, or other required fields. Type columns with the supported Arrow column values and restrict over to featureCodeColumnName/featureCodes.
As per coding guidelines, avoid type assertions when a narrower API contract can express the same fact and prefer types matching runtime behavior.
Proposed direction
-function scan(columns: Record<string, unknown>, over: Record<string, unknown> = {}) {
+type ScanColumns = Record<string, Float32Array | Int32Array>;
+type ScanOverrides = Pick<
+ Parameters<typeof scanMortonTableInBounds>[0],
+ 'featureCodeColumnName' | 'featureCodes'
+>;
+
+function scan(columns: ScanColumns, over: ScanOverrides = {}) {
...
- table: tableFromArrays(columns as never),
+ table: tableFromArrays(columns),
...
- } as never);
+ });📝 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.
| function scan(columns: Record<string, unknown>, over: Record<string, unknown> = {}) { | |
| const xs = new Float32PointBuffer(); | |
| const ys = new Float32PointBuffer(); | |
| const zs = new Float32PointBuffer(); | |
| scanMortonTableInBounds({ | |
| table: tableFromArrays(columns as never), | |
| rowGroupIndex: 1, // past the sentinel window, so no rows are skipped for it | |
| bounds: { minX: -1e6, minY: -1e6, maxX: 1e6, maxY: 1e6 }, | |
| axisNames: ['x', 'y'], | |
| mortonCodeColumnName: 'morton', | |
| xs, | |
| ys, | |
| zs, | |
| ...over, | |
| } as never); | |
| type ScanColumns = Record<string, Float32Array | Int32Array>; | |
| type ScanOverrides = Pick< | |
| Parameters<typeof scanMortonTableInBounds>[0], | |
| 'featureCodeColumnName' | 'featureCodes' | |
| >; | |
| function scan(columns: ScanColumns, over: ScanOverrides = {}) { | |
| const xs = new Float32PointBuffer(); | |
| const ys = new Float32PointBuffer(); | |
| const zs = new Float32PointBuffer(); | |
| scanMortonTableInBounds({ | |
| table: tableFromArrays(columns), | |
| rowGroupIndex: 1, // past the sentinel window, so no rows are skipped for it | |
| bounds: { minX: -1e6, minY: -1e6, maxX: 1e6, maxY: 1e6 }, | |
| axisNames: ['x', 'y'], | |
| mortonCodeColumnName: 'morton', | |
| xs, | |
| ys, | |
| zs, | |
| ...over, | |
| }); |
🤖 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/pointsMortonScanFilter.spec.ts` around lines 15 - 29,
Update the scan helper to type columns with the supported Arrow column value
type and restrict over to featureCodeColumnName and featureCodes. Remove both as
never assertions, construct the table and scan options with types accepted by
scanMortonTableInBounds, and prevent over from overriding required fields such
as table, bounds, buffers, or axis configuration.
Source: Coding guidelines
`preloadedBatch` is written after an await, so whichever read resolves LAST wins whether or not it is still current. Capture the loader identity and the revision the read was issued against, and drop the result if either moved. Both parts are load-bearing, for different races: - Revision. The streaming overlay bumps it per chunk, so several reads of one loader can be in flight, and an earlier slower one landing last would shrink the buffer mid-stream. This is the case raised in review on #89. It cannot currently fire: the adapter's `loadAll` snapshots the holder synchronously and never awaits, so its promises settle in call order. That is a property of one loader implementation, not of the `PointsLoader` contract these methods are written against. - Loader identity. A cap raise swaps the loader; `updateState` resets the batch state and starts a fresh read, but a read already in flight against the OLD loader still resolves and overwrites it. A revision check alone does not catch this — revisions are per-holder and can coincide across the swap — and `ensurePreloadedBatch` was exposed to it too, not just the refresh path. Both pinned in pointsLayerLoadOrdering.spec.ts, verified red (10 for 40, and 10 for 99). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Keying the rowCodes slot by memory cap makes an R5 misalignment representable; `plan()` is what has to act on it, and it did not. The gate was `!hasRowFeatureCodes(key)` — i.e. `isReady`, which stays true through a cap raise. Codes settled at 4M were therefore never re-requested and went on masking an 8M batch, index i naming some other point's feature. R5 survived in the one place that decides whether to fix it. Gate on readiness AT the cap the codes would be read at, and put that cap in the task id so a change re-dispatches instead of deduping against the task that already ran at the smaller window. Only reachable when the preload does not carry codes itself — a dict-only element, whose codes come from the `loadRowFeatureCodes` fallback. When the decode supplies them it re-settles them at its own cap for free, which is also why the gate defers while a preload is in flight: asking then would race a second full read of the feature column against it. A first load, with no codes at all, never waits. Review also suggested re-settling rowCodes unconditionally on a cap shed. Not done, and the reasoning is in a comment at the site: codes shorter than the new window are already misaligned, so re-keying them there would assert an alignment they do not have — precisely the lie the key exists to prevent. Leaving the key stale is what makes the new gate re-request them. Tightening the same comparison to `>=` was considered and dropped: codes are `min(rows, theirCap)` long, which makes the equal case unreachable. Punchlist D1 now carries the review's findings as evidence, and records that Effect is an open question rather than a settled "no". Specs verified red against the readiness gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adapter (`PointsRendererAdapter`, `pointsLoader`): - `columnarBatchFromPointData` silently dropped `featureCodes`, so the spread in `buildGrowingResource` that passes them was a no-op. Both `PointData` and the batch declare the field and the batch's doc says it is carried through; fixed at the source rather than the call site. Inert today only because the render path re-supplies codes from props. - `loadInBounds` was bound to the base resolved from the batch the holder held at BUILD time — so viewport queries would answer from the first preload forever, which is the one thing a growing holder exists to prevent. Re-resolves lazily when the holder has moved. No path reaches it today (these resources report `preloaded-columnar`, whose strategy renders from `loadAll` alone); it is wrong the moment D5 points a tiled strategy at a growing resource. - Both growing caches keyed on `key` alone, so a replaced `PointsElement` under the same key kept the old loader. Element identity is stable per `spatialData`, so guarding on it only rebuilds on a real dataset swap — which is precisely the case where the resolver cache is deliberately preserved and the stale loader would otherwise survive. Counts: `tallyFeatureCodesFromColumn` walked morton sentinel rows that every catalog builder it is paired with skips, so counts and entries could disagree about the same catalog. At most four rows, but it is a count of points that are not points, in the number the panel presents as authoritative. Taught the helper to skip them once instead of at each call site. The `?? new Map()` flagged as dead is not: nothing narrows `featureCodeMapFromCatalog`'s optional return here. Texture disposal called `destroy()` AND `delete()`, which is a double free on every luma version exposing both. Prefer `destroy`, fall back. Colour picker committed on every change event — continuous during a drag, each one a config write, palette rebuild and deck layer update. Coalesced to one per animation frame, keeping the live canvas preview the control exists for. Docs: record that a dict-only catalog can settle successfully with no counts, so `retry()` does not repair it — in the release checklist, the headless guide, the changeset and the handoff's retryability claim. Sync D8/D10 in the punchlist with what actually landed rather than leaving both fully deferred. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All 14 findings are now addressed across 8d1a875, 384931f, 615c926, 57f77fd, 4616ec7 and 4614d5f. Full suite green (355 core / 164 layers / 103 vis / 38 avivatorish / 36 zarrextra / 7 react), Biome and build clean. Ten fixed. Every behavioural fix has a spec verified red against the unfixed code first — Four amended rather than applied as suggested, each with reasoning on its thread:
Two smaller corrections: Not verified in-app. These were reasoned and unit-tested, not exercised against a real dataset in the browser. The ones where that would be worth doing before merge are the colour-picker rAF coalescing (a UI-timing change) and the adapter's element-identity rebuild (asserted safe from One pattern across the review worth recording: nearly every finding was the same shape — state arriving out of step with the thing it describes. A cached failure outliving its request; row codes vs the batch they align to; a read landing after its loader was replaced; codes vs the window they mask. None were catchable by types, because in each case the stale value is the correct type — the cache or slot simply has no way to say "this is no longer about what you are asking about". That is now recorded under punchlist D1 as evidence for the state-model rework, along with a note that Effect remains an open question rather than a settled no. |
`react-hooks/refs` rejected reading the overrides through a ref updated in the render body, and it is right to: the value it exists to keep current is a prop, so the ref was only ever compensating for the callback being captured per render. Hold the whole next overrides map in the pending ref instead, with the merge base taken at SCHEDULE time. No ref access during render, and it fixes a case the previous version got wrong: two features recoloured inside one frame now both survive, where before the second overwrote the first's merge. Also drop a pending entry on clear. The queued write carries the whole map, so landing it after a clear put the override straight back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Brings the points feature filter to a working state on large elements, and adds a
serializable selection API for it.
On
main, selecting a feature on a multi-million-row element frequently neverresolved at all: the scan plateaued part-way through and the layer sat there.
Alongside that, feature counts could stick permanently on partial values and
points could draw in the wrong feature's colours. This is not a speed-up of a
working feature — it is the feature starting to work, with the timings below as
evidence that it now completes rather than as the point in themselves.
Most of the changes are motivated by measurements that are not visible in the
diff, so the numbers are inline.
Where it ends up
Selecting one gene from a 12.1M-row Xenium
transcripts, worker enabled, 4M-row cap:mainA note on that baseline, since it is a strong claim.
mainwas not timeddirectly. The >3 min figure was measured with only the
numRowsfix removed —so it still had the
Vector.gethoist thatmainlacks, and it plateauedpart-way through the matched rows rather than completing, consistent with the
120 s worker timeout firing mid-scan.
mainhas neither hoist, so it is boundedbelow by that measurement and is worse in practice.
The intermediate figures quoted further down (3.2 s, 2.3 s, and so on) are
successive states of this branch as each fix landed, not the starting point.
Correctness
RequestSlot.settleaborts the in-flight request, so the resident preview settling underneath a running full scan destroyed it — and nothing re-requests a catalog. Counts stuck on partial values, and the dead scan still remapped row codes, so points drew in the wrong gene's colours. Also fixed the missing supersession check on the catalog loader (theR1discipline the slot documents).DataCloneErroron row-group chunks. They handed out the cached footer buffer; the worker transfers chunk buffers, which detached the cache, so the next row group posted an already-detached buffer. This aborted the progressive preload and dropped the element onto whole-file reads. Regression from the layout-caching commit in this branch.points.parquetpath is often a directory. Static servers 404 it (fine); MDV's Flask returns500 [Errno 21] Is a directory, which the store turns into a throw that escaped beforepart.0.parquetwas ever tried, wedging the element.pointSizerendered ~8300× differently between an element with a millimetre affine and one with an identity transform.API change — selections persist as names
PointsLayerConfig.featureNamesis the durable serialized form. Codes are app-assigned for dictionary-only elements (a Xeniumtranscriptscarriesfeature_nameand no code column), so a stored code could come back meaning a different gene.featureCodesstill works and still takes effect; names win when both are set.resolveFeatureSelectionCodes/featureNamesForCodesare exported from@spatialdata/core.This is the one shape change a downstream host would notice — it is additive, and the old field is still read.
What was making it not finish, in the order it was found
Each of these was measured before being written, and several supersede each other as later measurements came in — the scan is rewritten more than once across the history.
table.numRowsin a loop condition — 97×. It is not a field butdata.reduce((n, d) => n + d.length, 0): a fresh closure and a walk of every chunk, per row. 662 ms vs 7 ms at 64 chunks over 4M rows. This dominated everything else.Vector.get(i)per row — 15–50×. On a multi-chunk vector Arrow swaps in a prototype whosegetisbinarySearchover chunk offsets. Hoisting totoArray()(0–2 ms, zero-copy when single-chunk) removed it.fileOffset()+compressedSize()— every column — because parquet-wasm cannot fetch individual column chunks.ParquetFile.stream({ columns, rowGroups })fetches per column chunk, so the projection reaches the network: 12 columns fetched to use 3.supportsParquetStreaming()requiredwindow, excluding workers by accident, which had forced the streaming scan to decode on the main thread.batchSizeturned out to be load-bearing — omitting it cost 4×.number[]push +Float32Array.from.Known open — please read before approving
Feature counts can settle permanently absent. Recorded in
docs/plans/points-redesign-punchlist.mdwith the full mechanism. In short: a fallback catalog for a dict-only element cannot produce counts, andensureFeatureCatalognever re-requests a settled'full'catalog, so the failure is permanent and not cleared by remounting the panel. It reproduces intermittently and I have deliberately not fixed it blind — forcingserverSupportsStreamingRangesfalse should give a deterministic reproduction to fix against.Verification
pnpm -r --filter='!docs' test: core 338, layers 162, vis 102, react 7, zarrextra 36, avivatorish 38. 0 TypeScript errors, 0 Biome errors repo-wide (including a pre-existing one inavivatorish), all six packages build, docs build.In-app verification against real datasets throughout — Xenium
transcripts(12.1M points, 374 MB, dict-only) and merfish (3.7M) — including before/after timings taken by stashing only the source change so the comparison is like-for-like.Docs:
headless-viewer.mdxgains a points section (config fields, the names-vs-codes rationale, and a workedusePointsFeatureStateexample);mdv-release-checklist.mdxgains the matching entry.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation