Keep last-good fill colours while a colour column's rows load - #119
Merged
Conversation
A `fillColorByColumn` selection whose rows were not ready yet was treated as
"no colours" rather than as a loading state: the vis projection wrote
`fillColorByFeatureId: {}` into the rendered feature-state, so every feature
dropped to the layer's flat fill (shapes) or channel colour (labels) for the
whole load window, and for shapes the caller's own per-feature colours went
with it.
Labels felt it worst. `LabelsResolver` caches rows per element+column with no
cross-column stale value, so an ordinary column SWITCH always has a frame with
no rows at all and the segmentation blinked every time. Shapes was already
partly covered — `ShapesResolver` retains `Resolution.lastGood` across a
refine — leaving first-load and reload.
The entry getters now keep serving the previous entry when the resolver has no
rows, via `lastGoodShapeFillColorEntry` / `lastGoodLabelFillColorEntry`. An
entry records the `elementKey` it was built against and is only re-served for
that same element: feature ids, and especially small-integer label ids, do not
carry across elements. The feature-state merges leave `config.featureState`
untouched when there is no entry at all instead of clearing it.
The stale entry's identity is still the "colours are now different" signal, so
the rebuild fires the moment the real rows settle. A column whose load fails
keeps the last good colours; the failure surfaces through the resolver's
notices as before.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (1)
📝 WalkthroughWalkthroughChangesFill-colour preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/vis/tests/shapesProjection.spec.ts (1)
83-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the avoidable type assertion.
Use
satisfies ShapesLayerConfighere. It provides the required tuple context withoutas unknown as ShapesLayerConfig.As per coding guidelines, avoid type assertions when
satisfiescan express the type.Proposed fix
const withOwnColors = { ...config, featureState: { fillColorByFeatureId: { f1: [9, 8, 7, 255] } }, -} as unknown as ShapesLayerConfig; +} satisfies ShapesLayerConfig;🤖 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/shapesProjection.spec.ts` around lines 83 - 86, Update the withOwnColors configuration object to use satisfies ShapesLayerConfig instead of the avoidable as unknown as ShapesLayerConfig assertion, preserving the existing featureState and fillColorByFeatureId values.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.
Nitpick comments:
In `@packages/vis/tests/shapesProjection.spec.ts`:
- Around line 83-86: Update the withOwnColors configuration object to use
satisfies ShapesLayerConfig instead of the avoidable as unknown as
ShapesLayerConfig assertion, preserving the existing featureState and
fillColorByFeatureId values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df5b7a1c-d97b-4f87-9d31-eb527fa4b263
📒 Files selected for processing (6)
.changeset/keep-last-good-fill-colours.mdpackages/vis/src/SpatialCanvas/labelsProjection.tspackages/vis/src/SpatialCanvas/shapesProjection.tspackages/vis/src/SpatialCanvas/useLayerData.tspackages/vis/tests/labelsProjection.spec.tspackages/vis/tests/shapesProjection.spec.ts
`withOwnColors` spreads the already-typed `config`, so every required field is present at type level; the only reason for `as unknown as` was the RGBA literal widening to `number[]`. Under `satisfies` it is contextually typed as the tuple, so a future change that breaks the shape fails to compile instead of being cast past. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merged
xinaesthete
added a commit
that referenced
this pull request
Aug 12, 2026
…ty of the column (#142) * Apply a fill-colour column switch for a host that edits configs in place Two independent breaks sat between "the user picked a different column" and "the canvas shows it", and only a host that mutates its layer configs hit both. #119 fixed the third link in that chain — the load-window blank — which is why what remained read as "the colours just never change". The change never reached the resolver. `useLayerData`'s reconcile effect is the one place a config change becomes a request, and it was keyed on the identity of `layers` and the configs inside it. That assumes the caller allocates a fresh config per edit; MDV's render-stack adapter deliberately does the opposite, keeping one `LayerConfig` per Stack Entry so a cosmetic edit does not look structural and re-enter geometry loads. Under that caller the effect never re-ran, so the new column was never requested and the entry getters went on correctly serving last-good rows for good. The effect now also depends on `describeResolveInputs`, a value key over exactly the config fields each resolver's `plan()` reads. It is recomputed per render because a mutation is invisible to any memo, and holds scalars and short id lists only — a palette swap or an opacity drag does not move it. The settle never reached React. `SpatialEntryStore` subscribed to its resolvers in its constructor and tore that bridge down in `dispose()`, which the hook calls from an effect cleanup. An effect cleanup is not "the end": StrictMode's dev double-mount runs cleanup and then re-runs the effect against the same memoised store, after which the store was permanently deaf to its own resolvers and every async settle was dropped. Rows that landed after a switch did not repaint until an unrelated re-render happened along. The bridge now attaches on the first listener and detaches on the last, so it is exactly as long-lived as someone caring about it. `getVersion()` becomes a derived sum of the resolvers' versions rather than a counter that bridge maintained, so it stays true whether or not anything is subscribed. Verified against MDV driving only `fillColorByColumn` on a labels layer: switching to a column that has to be fetched now repaints on its own, and the same switch on a build without the reconcile key leaves the old colouring. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Drop the redundant tooltip-fields behavioural test `describeResolveInputs` already pins that the key moves when `tooltipFields` changes, and the effect passes the field into its resolve contexts unconditionally, so the async render only re-proved the wiring the shapes and labels cases prove. Those two stay: they exercise genuinely different resolver designs — `ShapesResolver` caches fill-colour rows per element, `LabelsResolver` per element AND column — and the labels case is the one that was reported. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Make a column's colours a property of the column, not of the view Category indices were assigned in first-seen feature order. A shapes layer walks the loader's geometry order and a labels layer walks the raster's ids, so one `cell_type` column rendered in two different schemes on the two kinds. The existing test for this pinned indices on one kind only; it now builds the same column through both encoders in opposite orders. Ordering by value fixes that, but no positional palette can survive a category being absent from a view — `tumour` really is the second category present when `stroma` is not. So `categoricalPalette` also takes `{ byValue }`, which is the only form an embedding application can use to say "Tumour is red" without knowing which index Tumour will land on. `numericDomain` does the same job for the continuous ramp, whose extent was measured from the loaded features. `featureColorSchemeSignature` now takes the scheme as one object so a new term cannot leave a call site keying on the old set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Let a continuous column carry a real ramp, not just two endpoints The ramps people actually use are not two-stop: viridis, a diverging red/white/blue, and any palette a host has already chosen for the same column in its own UI. Approximating one by its endpoints does not merely look different — it loses the midpoint that made it meaningful. `numericScale: 'symlog'` goes with it. A counts or expression column with its mass near zero and a long tail collapses into the ramp's first stop under a linear position; symlog spreads it. Symmetric rather than plain log because these columns reach zero and below. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Ship sourcemaps, and stop a malformed scheme crashing three frames away Only `core` published an `index.js.map`. A crash in `layers` or `vis` therefore reached a consumer as `Le (…/.vite/deps/@spatialdata_layers.js:396)`, which is not debuggable by anyone — the embedding application has only the built artifact. The colour helpers also trusted their own types. A scheme comes out of a saved Render Stack as JSON, so `categoricalPalette` can be an object without `byValue` and `numericRamp` can have one stop; both returned `undefined` and blew up later inside `rgba`, far from the field that was wrong. They now fall back to the default scheme, which is visible and reportable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Reject a null categorical palette from the named-palette guard `typeof null === 'object'` and `Array.isArray(null)` is false, so `null` passed `isNamedCategoricalPalette` and the destructure that follows threw on the spot — defeating `resolveCategoricalPalette`'s always-returns-a-colour guarantee, which the comment directly above it claims, and taking `featureColorSchemeSignature` down with it through the same guard. `{"categoricalPalette": null}` is a thing JSON says, and these specs come out of a saved Render Stack. Both entry points now fall through to the default scheme. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Correct two comments the colour and store changes left behind `numericRamp` still documented itself as the ramp's two endpoints; it has taken two or more stops since multi-stop ramps landed, and a caller reading only the doc would not know a viridis or diverging palette was expressible. The StrictMode caveat in `useLayerData` described the store as subscribing to its resolvers in its constructor and leaking an inert listener per discarded instance. Neither is true now: the bridge attaches on the first listener, so a store the memo builds and discards holds nothing. Replaced with what the reader of that effect actually needs — its cleanup has to be recoverable, because StrictMode re-runs the effect against the same store. Comments only; no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Say what "falls back to the default scheme" actually means The two malformed-palette tests asserted only that the colours came back defined and distinct — which a fallback returning arbitrary junk would also satisfy. Compare against the same column with no palette at all instead, so the assertion matches the name of the test. Distinctness stays, to keep the comparison from passing vacuously. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A
fillColorByColumnselection whose rows aren't ready yet was treated as no colours rather than as a loading state. The vis projection wrotefillColorByFeatureId: {}into the rendered feature-state, so every feature dropped to the layer's flat fill (shapes) or channel colour (labels) for the whole load window — and for shapes the caller's own per-feature colours went down with it.Labels felt it worst:
LabelsResolvercaches rows per element+column with no cross-column stale value, so an ordinary column switch always has a frame with no rows at all and the segmentation blinked every time. Shapes was already partly covered (ShapesResolverretainsResolution.lastGoodacross a refine), leaving first-load and reload.MDV carried a workaround for this in its own state.
How
lastGoodShapeFillColorEntry/lastGoodLabelFillColorEntryin the projection modules, beside the entry shapes whose invalidation rules they extend. When the resolver has no rows (pending, reloading, failed), the getters keep serving the previous entry instead of dropping toundefined.elementKeythey were built against, and last-good only applies for that same element. Feature ids — and especially small-integer label ids — do not carry across elements, so another element's entry would colour the wrong features.mergeShapeFeatureStateForRender/mergeLabelFeatureStateForRenderreturnconfig.featureStateuntouched when there is no entry at all (a column selected before anything has ever loaded) rather than overwritingfillColorByFeatureIdwith an empty map.The stale entry's identity is still the "the colours are now different" signal the runtime/LUT caches key on, so the rebuild fires the moment the real rows settle.
Reviewer notes
tsc, biome,lint:react(one pre-existing warning inPointsFeatureState.tsx, untouched) and the vis build are green.labels-color-bybrowser scenario drivesLabelsLayerwith a staticfeatureStateand never reaches this code path.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests