Skip to content

Make a fill-colour column switch apply, and make its colours a property of the column - #142

Merged
xinaesthete merged 8 commits into
mainfrom
claude/mdv-render-stack-column-2170e7
Aug 12, 2026
Merged

Make a fill-colour column switch apply, and make its colours a property of the column#142
xinaesthete merged 8 commits into
mainfrom
claude/mdv-render-stack-column-2170e7

Conversation

@xinaesthete

@xinaesthete xinaesthete commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Follows #119, which kept last-good fill colours through a column's load window. That was not enough: under MDV's render-stack adapter, switching an already-loaded column still failed to paint. Fixing that exposed a second, larger problem — fillColorByColumn could not express what an embedding application means by a colour — and a third, which is that when any of this goes wrong the consumer gets a stack trace they cannot read.

Verified throughout against MDV (feature/spatial_table_association) with a local link:, on a real project.

The apply path

A host that edits its layer configs in place — same object identity, mutated fields — got no reload. useLayerData's reconcile effect keyed on config identity, so a changed fillColorByColumn.columnName was invisible to it: the column name moved, the colours did not.

describeResolveInputs builds a value key from every config field a resolver's plan() actually reads, and the effect keys on that instead. The invariant is stated at the top of the module, because a field added to a resolver's plan and not to the key is a silent no-reload. resolveInputs.spec.ts pins both halves — what must move the key, and what must not, since a key that moved on an opacity drag would put store.reconcile on the drag's critical path for no load at all.

The second half was SpatialEntryStore: it subscribed to its resolvers in the constructor, so under StrictMode's dev double-mount the effect cleanup detached the bridge and the re-run reattached to a different store instance. Resolver settles then reached nobody and the layer never repainted. Subscription is now refcounted on the store's own listeners; getVersion() became a derived sum rather than a field, so it cannot drift from what the resolvers actually report.

Colours belong to the column, not to the view

Three things decided a column's encoding from whatever features happened to load, so two layers over one annotation could disagree about what a colour means — which reads as a data difference, not a bug.

Category order was first-seen. A shapes layer walks the loader's geometry order and a labels layer walks the raster's ids, so the same cell_type column rendered in two different schemes on the two kinds. labelColorEncoding.spec.ts had a test claiming to cover exactly this, but it only pinned indices on one kind; it now builds the same column through both encoders in opposite orders, and fails without the fix. Categories are ordered by value now, with numeric-looking values ordered numerically so cluster 10 follows cluster 9 rather than cluster 1.

No positional palette can survive a category being absent from a view. tumour genuinely is the second category present when stroma is not. So categoricalPalette also takes { byValue: { Tumour: [200, 30, 30] } }, with an optional fallback for values it does not name — 'oklab' by default, so an unnamed category keeps its own hue instead of merging into one bucket. This is the form to prefer in a saved stack, and the only form an application can use to make a layer agree with its own charts: it cannot know, and must not have to know, which index Tumour will land on.

The ramp measured its extent from the loaded features. numericDomain pins it to the column's own range; values outside clamp rather than extrapolate. numericRamp also takes more than two stops now — viridis, a diverging red/white/blue, or whatever a host already uses for the same column elsewhere. Approximating one by its endpoints loses the midpoint that made it meaningful. numericScale: 'symlog' goes with it, for a counts or expression column whose mass sits near zero with a long tail; symmetric rather than plain log because those columns reach zero and below.

featureColorSchemeSignature now takes the scheme as one object rather than three positional arguments, so adding a term to the encoding cannot leave a call site keying on the old set — the failure mode being a layer that keeps serving the previous colours after the scheme changed.

This changes colours for existing categorical configs that relied on the implicit first-seen order. Pass categoricalPalette: { byValue } to fix a scheme in place.

Debuggability

Only core published an index.js.map. A crash inside layers or vis therefore reached a consumer as:

TypeError: Cannot read properties of undefined (reading '0')
    at Le (…/.vite/deps/@spatialdata_layers.js:396:4)
    at We (…/.vite/deps/@spatialdata_layers.js:469:53)

Le is rgba. Nobody can get from that to a cause — an embedding application has only the built artifact to debug against. All five packages now emit sourcemaps.

The colour helpers also trusted their own types. A scheme arrives from a saved Render Stack, so FeatureCategoricalPaletteSpec is a claim about JSON, not a guarantee: a palette object with no byValue, a list with a hole in it, or a ramp with fewer than two stops each returned undefined and failed several frames later inside the arithmetic that reads rgb[0]. They now fall back to the default scheme. Wrong colours can be seen and reported; that TypeError cannot.

Reviewer notes

  • Three changesets: layers and vis take a minor, core and avivatorish a patch.
  • SpatialEntryStore's constructor no longer attaches resolvers. If anything constructs one and expects notifications without subscribing, it will now go quiet — spatialEntryStoreSubscription.spec.ts covers the dispose-then-resubscribe case that motivated it.
  • featureColorSchemeSignature's signature changed. Both in-repo call sites are updated; it is a cache-key helper, so external use is unlikely but not impossible.
  • The category-order change is the one with visible consequences. It is a behaviour change, deliberately taken, and the changeset says so.

Verification

  • 840 unit tests across 98 files; biome and lint:react clean.
  • Live against MDV on a linked checkout, driving a labels layer over st_day_a_roi_label_map: a categorical obs column paints in the host's own palette with zero per-feature colours computed host-side, a column switch repaints, and a double column renders as a real ramp over its own range instead of one category per distinct float.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added stable, value-based categorical colors and named palette support.
    • Added multi-stop numeric color ramps with linear and symlog scaling, custom domains, clamping, and safe fallbacks.
    • Added numeric color settings for shape and label layers.
  • Bug Fixes
    • In-place layer configuration changes now reliably trigger refreshed data and visual updates.
    • Improved consistency of colors across layers and row orders.
    • Malformed color schemes now still produce valid colors.
  • Chores
    • Published source maps for library builds.

xinaesthete and others added 5 commits August 11, 2026 16:24
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>
`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>
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>
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>
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>
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@xinaesthete, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 46 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1e915b02-c226-484c-a288-aa8c2c8165a4

📥 Commits

Reviewing files that changed from the base of the PR and between 20abefc and 9675132.

📒 Files selected for processing (3)
  • packages/layers/tests/featureColorEncoding.spec.ts
  • packages/vis/src/SpatialCanvas/types.ts
  • packages/vis/src/SpatialCanvas/useLayerData.ts
📝 Walkthrough

Walkthrough

The PR adds resolver-aware reconciliation for in-place layer configuration edits. It also adds deterministic categorical colors, configurable numeric ramps, malformed-scheme fallbacks, projection integration, sourcemap builds, tests, and package changesets.

Changes

Reactive rendering and color encoding

Layer / File(s) Summary
Resolver subscription lifecycle
packages/core/src/engine/SpatialEntryStore.ts, packages/core/tests/spatialEntryStoreSubscription.spec.ts
SpatialEntryStore attaches resolver subscriptions while listeners exist, detaches them after the final unsubscribe, and derives versions from resolver versions. Tests cover notification forwarding, disposal, and shared subscriptions.
Resolver-input reconciliation
packages/vis/src/SpatialCanvas/resolveInputs.ts, packages/vis/src/SpatialCanvas/useLayerData.ts, packages/vis/tests/resolveInputs.spec.ts, packages/vis/tests/useLayerData.spec.tsx
Visible resolver-relevant layer configuration is serialized into a reconciliation key. In-place fill-column changes now request new columns for shapes and labels.
Feature color encoding
packages/layers/src/featureColorEncoding.ts, packages/layers/src/index.ts, packages/layers/tests/featureColorEncoding.spec.ts
Categorical colors use sorted values and named palette lookups. Numeric colors support pinned domains, multi-stop ramps, linear and symlog scales, clamping, validation, and deterministic signatures.
Color projection integration
packages/layers/src/labelColorEncoding.ts, packages/layers/src/shapeColorEncoding.ts, packages/vis/src/SpatialCanvas/types.ts, packages/vis/src/SpatialCanvas/*Projection.ts, packages/layers/tests/*ColorEncoding.spec.ts, packages/vis/tests/labelsProjection.spec.ts
Shape and label configurations accept numeric domain and scale options. Projections pass complete color schemes to signature generation and forward numeric settings to color builders.
Build and release metadata
vite.config.base.ts, packages/layers/vite.config.ts, .changeset/*
Vite library builds emit sourcemaps. Changesets record package releases for the resolver, color, malformed-scheme, and sourcemap changes.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant LayerConfig
  participant describeResolveInputs
  participant useLayerData
  participant SpatialEntryStore
  participant Resolver

  LayerConfig->>describeResolveInputs: mutate resolver-relevant configuration
  describeResolveInputs->>useLayerData: return resolveInputsKey
  useLayerData->>SpatialEntryStore: replan reconciliation
  SpatialEntryStore->>Resolver: resolve updated inputs
  Resolver->>SpatialEntryStore: settle with updated version
  SpatialEntryStore->>useLayerData: notify reconciliation state
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: applying fill-colour column switches and making colour encodings depend on column values.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/mdv-render-stack-column-2170e7

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (4)
packages/core/tests/spatialEntryStoreSubscription.spec.ts (1)

51-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the derived getVersion.

The stub returns a constant 0, so the sum in SpatialEntryStore.getVersion is always 0. The tests pin the subscription half of the change but not the version half. The doc comment claims the version is correct with a listener, without one, and across a dispose. Nothing here proves that.

Make the stub's version mutable and assert the three states.

💚 Proposed test addition
-      getVersion: () => 0,
+      getVersion: () => version,
     },
+    bumpVersion: () => {
+      version += 1;
+    },

Add let version = 0; next to the listeners set, then add a test:

it('derives its version from the resolvers, with or without a listener', () => {
  const labels = notifyingResolver();
  const store = storeWith(labels.resolver);

  const before = store.getVersion();
  labels.bumpVersion();
  expect(store.getVersion()).toBe(before + 1);

  const unsubscribe = store.subscribe(vi.fn());
  labels.bumpVersion();
  expect(store.getVersion()).toBe(before + 2);

  unsubscribe();
  labels.bumpVersion();
  expect(store.getVersion()).toBe(before + 3);
});
🤖 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/spatialEntryStoreSubscription.spec.ts` at line 51, Make
the notifyingResolver test stub’s version mutable by adding a version variable
and have getVersion return it, with bumpVersion incrementing it. Add coverage
for SpatialEntryStore.getVersion that verifies resolver version changes are
reflected before subscribing, while subscribed, and after unsubscribe/dispose,
preserving the existing listener tests.
packages/vis/src/SpatialCanvas/resolveInputs.ts (1)

60-78: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value

Document that LabelsResolver.plan() ignores channels. It reads only tooltipFields and fillColorByColumn, so add the same explanatory comment as the image branch.

🤖 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/resolveInputs.ts` around lines 60 - 78, In the
labels branch of the config.type switch, add an explanatory comment documenting
that LabelsResolver.plan() ignores channels and reads only tooltipFields and
fillColorByColumn. Keep the existing joinIds and fillColorByColumn handling
unchanged, matching the comment style used in the image branch.
packages/vis/src/SpatialCanvas/labelsProjection.ts (1)

88-89: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Consider a delimiter in the label signature, as shapesProjection.ts uses.

Line 89 joins the parts with the empty string. getShapeFillColorSignature in packages/vis/src/SpatialCanvas/shapesProjection.ts line 86 joins the same kind of parts with '\u0001'. Without a delimiter, two different configs can produce one signature, for example columnName: 'typeauto' with mode: 'categorical' against columnName: 'type' with mode: 'autocategorical'. The second is not a valid mode today, so the collision is not currently reachable.

The change on line 88 makes the scheme term a variable-length JSON string, which widens the concatenation surface. Aligning the two signature builders removes the class of problem and makes them consistent.

♻️ Proposed change
   const scheme = featureColorSchemeSignature(config.fillColorByColumn);
-  return [config.fillColorByColumn.columnName, mode, scheme].join('');
+  return [config.fillColorByColumn.columnName, mode, scheme].join('\u0001');
🤖 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/labelsProjection.ts` around lines 88 - 89,
Update the signature construction in the labels projection function around
featureColorSchemeSignature to join columnName, mode, and scheme with the same
'\u0001' delimiter used by getShapeFillColorSignature in shapesProjection.ts,
preserving the existing part order.
packages/layers/src/featureColorEncoding.ts (1)

595-602: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Simplify the category-index construction.

The map is populated with a placeholder 0 for every value, then the keys are read back, sorted, and the indices are overwritten. The intermediate state is not used. Build the sorted list first, then populate the map once.

The current code is correct. This is a readability suggestion only.

♻️ Proposed simplification
-  const categoryIndexByValue = new Map<string, number>();
-  for (const value of new Set(nonEmptyValues)) {
-    categoryIndexByValue.set(value, 0);
-  }
-  const orderedValues = Array.from(categoryIndexByValue.keys()).sort(compareCategoryValues);
-  for (const [categoryIndex, value] of orderedValues.entries()) {
-    categoryIndexByValue.set(value, categoryIndex);
-  }
+  const orderedValues = Array.from(new Set(nonEmptyValues)).sort(compareCategoryValues);
+  const categoryIndexByValue = new Map<string, number>(
+    orderedValues.map((value, categoryIndex) => [value, categoryIndex])
+  );
🤖 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/featureColorEncoding.ts` around lines 595 - 602, In the
category-index construction around categoryIndexByValue, sort the unique
nonEmptyValues first and then create the map by assigning each ordered value its
index once. Remove the placeholder-population and keys-readback steps while
preserving compareCategoryValues ordering and the resulting index mapping.
🤖 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/column-colour-not-view-colour.md:
- Around line 2-3: Update the changeset entry for `@spatialdata/layers` to a major
release because the publicly exported featureColorSchemeSignature API no longer
accepts its previous three positional arguments; alternatively, restore a
compatible overload while retaining the new signature and keep `@spatialdata/vis`
release classification unchanged unless required.

In `@packages/layers/src/featureColorEncoding.ts`:
- Around line 173-177: Update isNamedCategoricalPalette to explicitly reject
null before treating the value as an object, so resolveCategoricalPalette and
assignFeatureColors preserve their always-returns-a-colour behavior for null
palettes. Add the malformed-scheme regression test covering a null palette and
verify fallback colors are defined.

In `@packages/vis/src/SpatialCanvas/types.ts`:
- Around line 54-55: Update the documentation for the numericRamp property in
FeatureNumericRampSpec to describe a continuous ramp with two or more RGB 0–255
stops, rather than only its endpoints. Keep the type and behavior unchanged.

In `@packages/vis/src/SpatialCanvas/useLayerData.ts`:
- Around line 625-641: Update the StrictMode note immediately above the effect
to reflect the listener-driven resolver attachment in SpatialEntryStore,
removing the outdated claim that constructor-time subscriptions leak listeners
from discarded instances. Preserve the existing explanation of why the effect
and dependency handling remain correct.

---

Nitpick comments:
In `@packages/core/tests/spatialEntryStoreSubscription.spec.ts`:
- Line 51: Make the notifyingResolver test stub’s version mutable by adding a
version variable and have getVersion return it, with bumpVersion incrementing
it. Add coverage for SpatialEntryStore.getVersion that verifies resolver version
changes are reflected before subscribing, while subscribed, and after
unsubscribe/dispose, preserving the existing listener tests.

In `@packages/layers/src/featureColorEncoding.ts`:
- Around line 595-602: In the category-index construction around
categoryIndexByValue, sort the unique nonEmptyValues first and then create the
map by assigning each ordered value its index once. Remove the
placeholder-population and keys-readback steps while preserving
compareCategoryValues ordering and the resulting index mapping.

In `@packages/vis/src/SpatialCanvas/labelsProjection.ts`:
- Around line 88-89: Update the signature construction in the labels projection
function around featureColorSchemeSignature to join columnName, mode, and scheme
with the same '\u0001' delimiter used by getShapeFillColorSignature in
shapesProjection.ts, preserving the existing part order.

In `@packages/vis/src/SpatialCanvas/resolveInputs.ts`:
- Around line 60-78: In the labels branch of the config.type switch, add an
explanatory comment documenting that LabelsResolver.plan() ignores channels and
reads only tooltipFields and fillColorByColumn. Keep the existing joinIds and
fillColorByColumn handling unchanged, matching the comment style used in the
image branch.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 34d3a4f6-47ad-42cd-bd8d-e72e51ff74bf

📥 Commits

Reviewing files that changed from the base of the PR and between e822563 and c2d9f84.

📒 Files selected for processing (22)
  • .changeset/apply-path-for-in-place-config-edits.md
  • .changeset/column-colour-not-view-colour.md
  • .changeset/publish-sourcemaps-and-survive-bad-schemes.md
  • packages/core/src/engine/SpatialEntryStore.ts
  • packages/core/tests/spatialEntryStoreSubscription.spec.ts
  • packages/layers/src/featureColorEncoding.ts
  • packages/layers/src/index.ts
  • packages/layers/src/labelColorEncoding.ts
  • packages/layers/src/shapeColorEncoding.ts
  • packages/layers/tests/featureColorEncoding.spec.ts
  • packages/layers/tests/labelColorEncoding.spec.ts
  • packages/layers/tests/shapeColorEncoding.spec.ts
  • packages/layers/vite.config.ts
  • packages/vis/src/SpatialCanvas/labelsProjection.ts
  • packages/vis/src/SpatialCanvas/resolveInputs.ts
  • packages/vis/src/SpatialCanvas/shapesProjection.ts
  • packages/vis/src/SpatialCanvas/types.ts
  • packages/vis/src/SpatialCanvas/useLayerData.ts
  • packages/vis/tests/labelsProjection.spec.ts
  • packages/vis/tests/resolveInputs.spec.ts
  • packages/vis/tests/useLayerData.spec.tsx
  • vite.config.base.ts

Comment thread .changeset/column-colour-not-view-colour.md
Comment thread packages/layers/src/featureColorEncoding.ts
Comment thread packages/vis/src/SpatialCanvas/types.ts Outdated
Comment thread packages/vis/src/SpatialCanvas/useLayerData.ts
`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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/layers/tests/featureColorEncoding.spec.ts (1)

476-480: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace the direct as never with a documented invalid-JSON boundary.

featureColorSchemeSignature has no normalizer. Use a local @ts-expect-error with a comment explaining that saved JSON can contain null, or route the value through one unknown fixture helper.

🤖 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/featureColorEncoding.spec.ts` around lines 476 - 480,
Update the test invoking featureColorSchemeSignature to avoid the direct as
never cast. Represent the persisted invalid-JSON null boundary with a local
`@ts-expect-error` and an explanatory comment, or reuse a single unknown-based
fixture helper if one already exists, while preserving the assertion that null
categoricalPalette does not throw.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/layers/tests/featureColorEncoding.spec.ts`:
- Around line 464-474: Strengthen the test around malformed(null) to verify the
actual default scheme, not merely defined distinct colors. Compare its result
with the established default-scheme result or assert the documented default
colors, while preserving the existing null-palette coverage.

---

Nitpick comments:
In `@packages/layers/tests/featureColorEncoding.spec.ts`:
- Around line 476-480: Update the test invoking featureColorSchemeSignature to
avoid the direct as never cast. Represent the persisted invalid-JSON null
boundary with a local `@ts-expect-error` and an explanatory comment, or reuse a
single unknown-based fixture helper if one already exists, while preserving the
assertion that null categoricalPalette does not throw.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a380fca0-e33c-4067-83c7-4eaed8ac5a62

📥 Commits

Reviewing files that changed from the base of the PR and between c2d9f84 and 20abefc.

📒 Files selected for processing (2)
  • packages/layers/src/featureColorEncoding.ts
  • packages/layers/tests/featureColorEncoding.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/layers/src/featureColorEncoding.ts

Comment thread packages/layers/tests/featureColorEncoding.spec.ts
xinaesthete and others added 2 commits August 12, 2026 09:26
`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>
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>
@xinaesthete
xinaesthete merged commit a0a3cc4 into main Aug 12, 2026
7 checks passed
@xinaesthete
xinaesthete deleted the claude/mdv-render-stack-column-2170e7 branch August 12, 2026 08:40
@github-actions github-actions Bot mentioned this pull request Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant