From 17338db3d3369588329ec8386c04888f6ce10830 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Wed, 5 Aug 2026 19:37:17 +0100 Subject: [PATCH 1/8] Apply a fill-colour column switch for a host that edits configs in place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../apply-path-for-in-place-config-edits.md | 40 ++++++ packages/core/src/engine/SpatialEntryStore.ts | 55 +++++-- .../spatialEntryStoreSubscription.spec.ts | 134 +++++++++++++++++ .../vis/src/SpatialCanvas/resolveInputs.ts | 81 +++++++++++ .../vis/src/SpatialCanvas/useLayerData.ts | 27 +++- packages/vis/tests/resolveInputs.spec.ts | 129 +++++++++++++++++ packages/vis/tests/useLayerData.spec.tsx | 135 ++++++++++++++++++ 7 files changed, 592 insertions(+), 9 deletions(-) create mode 100644 .changeset/apply-path-for-in-place-config-edits.md create mode 100644 packages/core/tests/spatialEntryStoreSubscription.spec.ts create mode 100644 packages/vis/src/SpatialCanvas/resolveInputs.ts create mode 100644 packages/vis/tests/resolveInputs.spec.ts diff --git a/.changeset/apply-path-for-in-place-config-edits.md b/.changeset/apply-path-for-in-place-config-edits.md new file mode 100644 index 00000000..609d572f --- /dev/null +++ b/.changeset/apply-path-for-in-place-config-edits.md @@ -0,0 +1,40 @@ +--- +'@spatialdata/core': patch +'@spatialdata/vis': patch +--- + +Make a fill-colour column (or tooltip field) switch actually apply for a host that +edits its layer configs in place. + +Two independent breaks sat between "the user picked a different column" and "the +canvas shows it", and a host only hit them together. #119 fixed the third thing in +that chain — the load-window blank — which is why the remaining two read as "the +colours just never change". + +**The change never reached the resolver.** `useLayerData`'s reconcile effect is the +one place a config change turns into 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: the new column +was never requested, `getShapeFillColorEntry` / `getLabelFillColorEntry` went on +correctly serving last-good rows, and last-good was all there would ever be. The +effect now also depends on `describeResolveInputs` — a value key over exactly the +config fields each resolver's `plan()` reads, recomputed per render because a +mutation is invisible to any memo. It holds scalars and short id lists only; a +palette swap or an opacity drag does not move it, so nothing replans on a slider. + +**The settle never reached React.** `SpatialEntryStore` subscribed to its resolvers +in its constructor and tore that bridge down in `dispose()` — which `useLayerData` +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. Every async +settle from then on was dropped, so rows that landed after a switch did not repaint +until an unrelated re-render (a pan) came along. The bridge is now attached on the +first listener and detached on the last, so it is exactly as long-lived as someone +caring about it and survives any number of remounts. `getVersion()` became a derived +sum of the resolvers' versions rather than a counter the bridge maintained, so it +stays true whether or not anything is subscribed. + +No public API change. Verified against MDV driving only `fillColorByColumn` on a +labels layer: switching to a column that has to be fetched now repaints on its own. diff --git a/packages/core/src/engine/SpatialEntryStore.ts b/packages/core/src/engine/SpatialEntryStore.ts index c2a183b0..30ffd5a8 100644 --- a/packages/core/src/engine/SpatialEntryStore.ts +++ b/packages/core/src/engine/SpatialEntryStore.ts @@ -29,28 +29,63 @@ export class SpatialEntryStore { private readonly unsubscribes: Array<() => void> = []; /** One AbortController per in-flight task id. Superseding cancels the old one. */ private readonly inFlight = new Map(); - private version = 0; constructor(resolvers: ResolverRegistry) { this.resolvers = resolvers; - // The store's version is the sum of its parts: any resolver mutating is a - // reason for React to re-read. - for (const resolver of Object.values(resolvers)) { + } + + /** + * The store's version is the sum of its parts: any resolver mutating is a reason + * for React to re-read. The bridge that carries that is tied to HAVING LISTENERS, + * not to construction — because the store outlives the effect that consumes it. + * + * Subscribing in the constructor and tearing down in `dispose` looks equivalent + * and is not. `dispose()` runs from a React effect cleanup, and 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. A constructor-time bridge cannot be + * rebuilt, so from that moment the store was permanently deaf to its own resolvers + * — every async settle after mount was dropped, and a fill-colour column whose + * rows landed after the switch never repainted until an unrelated re-render (a + * pan) happened to come along. Attaching on the first listener and detaching on + * the last makes the bridge exactly as long-lived as someone caring about it, and + * survives any number of remounts. + */ + private attachResolvers(): void { + if (this.unsubscribes.length > 0) return; + for (const resolver of Object.values(this.resolvers)) { this.unsubscribes.push(resolver.subscribe(() => this.notify())); } } + private detachResolvers(): void { + for (const unsubscribe of this.unsubscribes) unsubscribe(); + this.unsubscribes.length = 0; + } + subscribe = (listener: () => void): (() => void) => { this.listeners.add(listener); + if (this.listeners.size === 1) this.attachResolvers(); return () => { this.listeners.delete(listener); + if (this.listeners.size === 0) this.detachResolvers(); }; }; - getVersion = (): number => this.version; + /** + * The sum of its parts, DERIVED rather than counted. + * + * A counter incremented from the notification bridge would only be correct while + * something was subscribed — and the bridge is now listener-driven, so that is not + * always. Summing the resolvers' own versions makes this a pure read of the state + * it describes: true with a listener, without one, and across a dispose. + */ + getVersion = (): number => { + let version = 0; + for (const resolver of Object.values(this.resolvers)) version += resolver.getVersion(); + return version; + }; private notify(): void { - this.version += 1; for (const listener of this.listeners) { listener(); } @@ -137,9 +172,13 @@ export class SpatialEntryStore { this.resolvers[kind]?.evict(elementKey); } + /** + * Release everything this store owns. Not a one-way door: a later `subscribe` + * re-attaches the resolver bridge, which is what lets a StrictMode remount (or any + * effect that re-runs against the same store) recover instead of going silent. + */ dispose(): void { - for (const unsubscribe of this.unsubscribes) unsubscribe(); - this.unsubscribes.length = 0; + this.detachResolvers(); for (const controller of this.inFlight.values()) controller.abort(); this.inFlight.clear(); for (const resolver of Object.values(this.resolvers)) resolver.dispose(); diff --git a/packages/core/tests/spatialEntryStoreSubscription.spec.ts b/packages/core/tests/spatialEntryStoreSubscription.spec.ts new file mode 100644 index 00000000..fef43de2 --- /dev/null +++ b/packages/core/tests/spatialEntryStoreSubscription.spec.ts @@ -0,0 +1,134 @@ +import { describe, expect, it, vi } from 'vitest'; +import { + type EntryResources, + Resolution, + type ResolveTask, + SpatialEntryStore, +} from '../src/engine/index.js'; + +/** + * The store's bridge to its resolvers, under the lifecycle React actually gives it. + * + * `SpatialEntryStore` is memoised by `useLayerData` and disposed from an effect + * cleanup — and an effect cleanup is not "the end". StrictMode's dev double-mount + * runs cleanup and then re-runs the effect against the same store instance. When the + * bridge was built in the constructor and torn down in `dispose`, that sequence left + * the store permanently deaf: resolvers went on loading and settling, and nothing + * downstream ever heard about it. The symptom was a fill-colour column that loaded + * and then never painted until an unrelated re-render came along. + * + * These tests drive that sequence directly, with a resolver stub whose only job is to + * emit one settle. + */ + +/** A resolver that does nothing but let a test fire its settle notification. */ +function notifyingResolver() { + const listeners = new Set<() => void>(); + return { + resolver: { + kind: 'labels' as const, + blockingResources: [] as const, + plan: (): readonly ResolveTask[] => [], + load: async () => {}, + snapshot: (): EntryResources => ({ + entryId: 'e', + elementKey: 'k', + resources: {}, + notices: [], + bounds: null, + revision: 0, + }), + evict: () => {}, + dispose: () => { + listeners.clear(); + }, + subscribe: (listener: () => void) => { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; + }, + getVersion: () => 0, + }, + /** Stand in for a load settling — what `finally { this.notify() }` does. */ + settle: () => { + for (const listener of listeners) listener(); + }, + listenerCount: () => listeners.size, + }; +} + +function storeWith(labels: ReturnType['resolver']) { + const inert = { ...labels, subscribe: () => () => {} }; + return new SpatialEntryStore({ + points: inert, + shapes: inert, + images: inert, + labels, + }); +} + +describe('SpatialEntryStore — the resolver notification bridge', () => { + it('forwards a resolver settle to its listeners', () => { + const labels = notifyingResolver(); + const store = storeWith(labels.resolver); + const onChange = vi.fn(); + store.subscribe(onChange); + + labels.settle(); + + expect(onChange).toHaveBeenCalledTimes(1); + }); + + it('still forwards a settle after dispose + resubscribe (the StrictMode remount)', () => { + // THE regression. React runs cleanup then re-runs the effect against the same + // memoised store; nothing about that says the store is finished. + const labels = notifyingResolver(); + const store = storeWith(labels.resolver); + const onChange = vi.fn(); + + const unsubscribe = store.subscribe(onChange); + unsubscribe(); + store.dispose(); + store.subscribe(onChange); + + labels.settle(); + + expect(onChange).toHaveBeenCalledTimes(1); + }); + + it('holds no resolver subscription while nobody is listening', () => { + // The bridge exists for listeners. With none, it must not pin the resolver — + // otherwise a discarded store keeps a live edge into a resolver it no longer owns. + const labels = notifyingResolver(); + const store = storeWith(labels.resolver); + + expect(labels.listenerCount()).toBe(0); + + const unsubscribe = store.subscribe(vi.fn()); + expect(labels.listenerCount()).toBe(1); + + unsubscribe(); + expect(labels.listenerCount()).toBe(0); + }); + + it('attaches once for many listeners, and detaches only when the last one goes', () => { + const labels = notifyingResolver(); + const store = storeWith(labels.resolver); + const first = vi.fn(); + const second = vi.fn(); + + const unsubscribeFirst = store.subscribe(first); + const unsubscribeSecond = store.subscribe(second); + expect(labels.listenerCount()).toBe(1); + + unsubscribeFirst(); + expect(labels.listenerCount()).toBe(1); + + labels.settle(); + expect(second).toHaveBeenCalledTimes(1); + + unsubscribeSecond(); + expect(labels.listenerCount()).toBe(0); + }); +}); diff --git a/packages/vis/src/SpatialCanvas/resolveInputs.ts b/packages/vis/src/SpatialCanvas/resolveInputs.ts new file mode 100644 index 00000000..01b90b17 --- /dev/null +++ b/packages/vis/src/SpatialCanvas/resolveInputs.ts @@ -0,0 +1,81 @@ +/** + * A value key over the layer-config fields that drive LOADING. + * + * `useLayerData`'s reconcile effect is the one place a config change turns into a + * request to the Resource Resolver. Keying that effect on the identity of `layers` + * (or of the configs inside it) assumes the caller allocates a fresh config object + * per edit — and one important caller does the opposite on purpose. + * + * MDV's render-stack adapter keeps ONE `LayerConfig` object per Stack Entry and + * patches it in place, so that a cosmetic edit (opacity, a channel colour) does not + * look like a structural change and re-enter async geometry loads. Under that + * caller, `layers` and every config in it hold their identity across an edit, so an + * identity-keyed effect never re-runs: the resolver is never asked for the new + * column, `getShapeFillColorEntry` / `getLabelFillColorEntry` correctly keep serving + * the last-good rows (#119), and the layer paints the PREVIOUS column's colours for + * good. The identity discipline the render path needs and the change detection the + * load path needs are different jobs; this key does the second one by value. + * + * Cheap on purpose. It is recomputed every render — that is the whole point, since + * a mutation is invisible to any memo — so it holds scalars and short id lists only. + * Nothing here may serialise a payload that scales with the data: a categorical + * palette, a colour map, a feature catalog. Those affect how a resource is DRAWN, + * not whether it must be loaded, and the projections already handle them by value. + * + * INVARIANT: every config field that a resolver's `plan()` reads must appear here. + * The lines below mirror, one for one, what each `plan()` looks at today — + * `ShapesResolver` and `LabelsResolver` (tooltip fields, fill-colour column), + * `PointsResolver` (memory cap, feature selection, colour-by), and `ImagesResolver` + * (nothing: it plans its loader off element identity alone, which `elementMap` + * already covers). A resolver that starts planning off a new field must add it here + * in the same change, or that field gets exactly the bug described above. + */ + +import type { LayerConfig } from './types'; + +/** Field separator. Neither a layer id, element key nor column name may contain it. */ +const FIELD = '\u0001'; +/** Item separator, for the short id lists (tooltip fields, feature selections). */ +const ITEM = '\u0000'; + +function joinIds(ids: readonly (string | number)[] | undefined): string { + return ids && ids.length > 0 ? ids.join(ITEM) : ''; +} + +/** + * Serialise the load-relevant inputs of the visible layers, in render order. + * + * Invisible layers are omitted rather than encoded, matching the reconcile effect: + * hiding a layer withdraws its resolve context, which is itself a change of key. + */ +export function describeResolveInputs( + layers: Record, + layerOrder: readonly string[] +): string { + const parts: string[] = []; + for (const layerId of layerOrder) { + const config = layers[layerId]; + if (!config?.visible) continue; + parts.push(layerId, config.type, config.elementKey); + switch (config.type) { + case 'shapes': + case 'labels': + parts.push(joinIds(config.tooltipFields), config.fillColorByColumn?.columnName ?? ''); + break; + case 'points': + parts.push( + String(config.pointsMemoryCap ?? ''), + // Both selection forms: names are the durable one and take precedence, but + // codes still drive a config written before names existed. + joinIds(config.featureNames), + joinIds(config.featureCodes), + String(config.colorByFeature ?? '') + ); + break; + case 'image': + // `ImagesResolver.plan` reads no config — only whether the loader is idle. + break; + } + } + return parts.join(FIELD); +} diff --git a/packages/vis/src/SpatialCanvas/useLayerData.ts b/packages/vis/src/SpatialCanvas/useLayerData.ts index 5ecd319b..cc77c34b 100644 --- a/packages/vis/src/SpatialCanvas/useLayerData.ts +++ b/packages/vis/src/SpatialCanvas/useLayerData.ts @@ -60,6 +60,7 @@ import { } from './labelsProjection'; import { renderLabelsLayer } from './renderers/labelsRenderer'; import { renderShapesLayer } from './renderers/shapesRenderer'; +import { describeResolveInputs } from './resolveInputs'; import { createNonOwningResolver } from './resolvers/nonOwningResolver'; import { ImagesResolver, LabelsResolver } from './resolvers/RasterResolvers'; import { @@ -416,6 +417,11 @@ export function useLayerData( // eslint-disable-next-line react-hooks/refs -- intentional latest-`layers` mirror consumed during render, see comment above layersRef.current = layers; + // Recomputed every render, NOT memoised on `layers`: a caller that patches its + // configs in place changes nothing this hook could memoise on. It is a handful of + // scalars per visible layer — see `describeResolveInputs` for what may live in it. + const resolveInputsKey = describeResolveInputs(layers, layerOrder); + const [layerLoadStates, setLayerLoadStates] = useState>({}); // Bumped on every resolver settle. The reconcile effect depends on it so that an // async settle (e.g. the preload landing, which flips `supportsFeatureScan`) re-runs @@ -616,12 +622,23 @@ export function useLayerData( // element resolution changes without `layers`/`store` changing — e.g. a coordinate // system switch that makes a previously unavailable element resolvable. The map is // memoised on `availableElements`, so this adds no per-render churn. + // + // And on `resolveInputsKey`, which is what makes the effect fire for a caller that + // edits its configs IN PLACE (MDV's render-stack adapter does, deliberately). For + // such a caller neither `layers` nor the config objects inside it ever change + // identity, so identity deps alone leave a switched fill-colour column or tooltip + // field never requested at all. See `describeResolveInputs`. useEffect(() => { // Bare reference: `loadedDataRevision` is a re-trigger, not a value we read. A // resolver settle (the preload landing flips `supportsFeatureScan`) must replan so // the scan/row-codes tasks get emitted; touching it here declares that dependency // honestly to exhaustive-deps. The plan/load dedup makes the extra runs convergent. void loadedDataRevision; + // Bare reference, same reason: the contexts below are read from `layers`, so the + // key is never a value this body uses — it is the only thing that CHANGES when a + // caller mutates those configs in place. Declaring it here is what makes it a + // legitimate dependency rather than an "unnecessary" one. + void resolveInputsKey; const contexts: AnyResolveContext[] = []; for (const layerId of layerOrder) { const config = layers[layerId]; @@ -694,7 +711,15 @@ export function useLayerData( // the list because the name→code resolution above reads its catalog. The catalog // ARRIVING is covered by `loadedDataRevision` (bumped on every resolver settle), // which is what re-resolves a name selection that could not be resolved yet. - }, [layers, layerOrder, store, elementMapValue, loadedDataRevision, pointsEngine]); + }, [ + layers, + layerOrder, + resolveInputsKey, + store, + elementMapValue, + loadedDataRevision, + pointsEngine, + ]); // --- Shapes projection memos (Renderer Adapter side, kept in vis) ------------- diff --git a/packages/vis/tests/resolveInputs.spec.ts b/packages/vis/tests/resolveInputs.spec.ts new file mode 100644 index 00000000..ecde4123 --- /dev/null +++ b/packages/vis/tests/resolveInputs.spec.ts @@ -0,0 +1,129 @@ +import { describe, expect, it } from 'vitest'; +import { describeResolveInputs } from '../src/SpatialCanvas/resolveInputs.js'; +import type { LayerConfig } from '../src/SpatialCanvas/types.js'; + +/** + * The key has two jobs and they pull against each other: it must move for every + * config change that starts a LOAD (or an in-place caller never gets its data), and + * it must NOT move for a cosmetic one (or every opacity drag replans the world). + * Both halves are pinned here. + */ + +const shapes = (extra: Partial> = {}): LayerConfig => ({ + id: 'layer-1', + type: 'shapes', + elementKey: 'cells', + visible: true, + opacity: 1, + ...extra, +}); + +const key = (config: LayerConfig) => describeResolveInputs({ [config.id]: config }, [config.id]); + +describe('describeResolveInputs — what starts a load', () => { + it('moves when the fill-colour column changes', () => { + expect( + key(shapes({ fillColorByColumn: { columnName: 'colA', mode: 'categorical' } })) + ).not.toBe(key(shapes({ fillColorByColumn: { columnName: 'colB', mode: 'categorical' } }))); + }); + + it('moves when the fill-colour column is cleared', () => { + expect( + key(shapes({ fillColorByColumn: { columnName: 'colA', mode: 'categorical' } })) + ).not.toBe(key(shapes())); + }); + + it('moves when the tooltip fields change', () => { + expect(key(shapes({ tooltipFields: ['a'] }))).not.toBe(key(shapes({ tooltipFields: ['b'] }))); + }); + + it('moves when the element behind a layer changes', () => { + expect(key(shapes())).not.toBe(key(shapes({ elementKey: 'nuclei' }))); + }); + + it('moves when a layer is hidden', () => { + expect(key(shapes())).not.toBe(key(shapes({ visible: false }))); + }); + + it('moves for every points field a scan is planned from', () => { + const points = (extra: Partial>): LayerConfig => ({ + id: 'layer-p', + type: 'points', + elementKey: 'transcripts', + visible: true, + opacity: 1, + ...extra, + }); + const base = key(points({})); + + expect(key(points({ pointsMemoryCap: 1_000 }))).not.toBe(base); + expect(key(points({ featureNames: ['EPCAM'] }))).not.toBe(base); + expect(key(points({ featureCodes: [3] }))).not.toBe(base); + expect(key(points({ colorByFeature: false }))).not.toBe(base); + }); + + it('separates two layers that would otherwise concatenate into the same key', () => { + // `a` + `bc` must not read as `ab` + `c`: without a field separator, moving a + // character across the boundary would be invisible to the effect. + const one = describeResolveInputs( + { a: shapes({ id: 'a', tooltipFields: ['x'] }), bc: shapes({ id: 'bc' }) }, + ['a', 'bc'] + ); + const two = describeResolveInputs( + { ab: shapes({ id: 'ab', tooltipFields: ['x'] }), c: shapes({ id: 'c' }) }, + ['ab', 'c'] + ); + + expect(one).not.toBe(two); + }); +}); + +describe('describeResolveInputs — what does not', () => { + // The effect it keys runs `store.reconcile` for every entry. A key that moved on + // a slider drag would put that on the drag's critical path for no load at all. + it('ignores opacity, colours and stroke — nothing there is loaded', () => { + const base = key(shapes()); + + expect(key(shapes({ opacity: 0.3 }))).toBe(base); + expect(key(shapes({ fillColor: [1, 2, 3, 4] }))).toBe(base); + expect(key(shapes({ strokeColor: [1, 2, 3, 4], strokeWidth: 9 }))).toBe(base); + }); + + it('ignores the colour SCHEME of the fill column — same rows, different encoding', () => { + const withColumn = shapes({ fillColorByColumn: { columnName: 'colA', mode: 'categorical' } }); + const withPalette = shapes({ + fillColorByColumn: { + columnName: 'colA', + mode: 'categorical', + categoricalPalette: { A: [1, 2, 3] }, + }, + }); + + expect(key(withPalette)).toBe(key(withColumn)); + }); + + it('ignores per-feature state — a hidden or recoloured feature loads nothing new', () => { + expect( + key( + shapes({ + featureState: { hiddenFeatureIds: ['c1'], fillColorByFeatureId: { c2: [1, 2, 3, 4] } }, + }) + ) + ).toBe(key(shapes())); + }); + + it('ignores image channel state — the images resolver plans off the element alone', () => { + const image = ( + channels?: Extract['channels'] + ): LayerConfig => ({ + id: 'layer-i', + type: 'image', + elementKey: 'morphology', + visible: true, + opacity: 1, + ...(channels ? { channels } : {}), + }); + + expect(key(image({ colors: [[255, 0, 0]], contrastLimits: [[0, 100]] }))).toBe(key(image())); + }); +}); diff --git a/packages/vis/tests/useLayerData.spec.tsx b/packages/vis/tests/useLayerData.spec.tsx index 6d1aaf29..215cfd77 100644 --- a/packages/vis/tests/useLayerData.spec.tsx +++ b/packages/vis/tests/useLayerData.spec.tsx @@ -620,3 +620,138 @@ describe('useLayerData — the hover highlight channel', () => { expect(getRenders()).toBe(baseline); }); }); + +describe('useLayerData — a caller that mutates its layer configs in place', () => { + // MDV's render-stack adapter keeps ONE `LayerConfig` object per stack entry and + // patches it in place, so a cosmetic edit does not re-enter async geometry loads. + // The `layers` record it hands over therefore keeps its identity across an edit — + // which means config identity is NOT a "the load inputs changed" signal, and any + // load the hook plans off that identity silently never happens. + // + // The symptom that got here: switching an already-loaded shapes/labels layer to a + // different `fillColorByColumn` left the colours on the previous column forever. + // The projection had no rows for the new column, so (correctly, per #119) it kept + // serving the last-good entry — but nothing ever asked the resolver to load the + // new column, so "last good" was all there would ever be. + function tableSpatialData() { + const columnValues: Record = { + region: ['cells', 'cells'], + colA: ['a', 'a'], + colB: ['b', 'b'], + }; + const loadObsColumns = vi.fn(async (names: string[]) => + names.map((name) => columnValues[name] ?? []) + ); + const table = { + getTableKeys: () => ({ region: ['cells'], regionKey: 'region' }), + loadObsIndex: vi.fn(async () => ['c1', 'c2']), + loadObsColumns, + }; + const spatialData = { + getAssociatedTable: vi.fn(() => ['table', table]), + } as unknown as SpatialData; + return { spatialData, loadObsColumns }; + } + + /** Every column name the hook has asked the associated table for, in order. */ + const requestedColumns = (loadObsColumns: ReturnType): string[] => + loadObsColumns.mock.calls.flatMap((call) => call[0] as string[]); + + it('loads the new fill-colour column when a shapes config is switched in place', async () => { + const { spatialData, loadObsColumns } = tableSpatialData(); + const elements: ElementsByType = { ...EMPTY_ELEMENTS, shapes: [shapesElement('cells')] }; + // ONE config object, ONE record — both keep their identity for the whole test, + // exactly as they do under the render-stack adapter. + const config: LayerConfig = { + ...shapesConfig('layer-1', 'cells'), + fillColorByColumn: { columnName: 'colA', mode: 'categorical' }, + }; + const layers = { 'layer-1': config }; + const layerOrder = Object.keys(layers); + + const { rerender } = renderHook(() => + useLayerData(layers, layerOrder, elements, null, spatialData) + ); + + await waitFor(() => { + expect(requestedColumns(loadObsColumns)).toContain('colA'); + }); + + // The switch the user makes in the panel: same config object, new column. + config.fillColorByColumn = { columnName: 'colB', mode: 'categorical' }; + rerender(); + + await waitFor(() => { + expect(requestedColumns(loadObsColumns)).toContain('colB'); + }); + }); + + it('loads the new fill-colour column when a labels config is switched in place', async () => { + const { spatialData, loadObsColumns } = tableSpatialData(); + const labels: AvailableElement = { + key: 'segmentation', + type: 'labels', + // The loader load will fail (no real zarr behind it) and that is fine: the + // fill-colour column is a resource of its own and must load regardless. + element: { key: 'segmentation' } as unknown as AvailableElement['element'], + transform: new Matrix4(), + }; + const elements: ElementsByType = { ...EMPTY_ELEMENTS, labels: [labels] }; + const config: LayerConfig = { + id: 'layer-l', + type: 'labels', + elementKey: 'segmentation', + visible: true, + opacity: 1, + fillColorByColumn: { columnName: 'colA', mode: 'categorical' }, + }; + const layers = { 'layer-l': config }; + const layerOrder = Object.keys(layers); + + const { rerender } = renderHook(() => + useLayerData(layers, layerOrder, elements, null, spatialData) + ); + + await waitFor(() => { + expect(requestedColumns(loadObsColumns)).toContain('colA'); + }); + + config.fillColorByColumn = { columnName: 'colB', mode: 'categorical' }; + rerender(); + + await waitFor(() => { + expect(requestedColumns(loadObsColumns)).toContain('colB'); + }); + }); + + it('loads the new tooltip fields when they are switched in place', async () => { + // Same defect, different resource: the tooltip columns are planned from the same + // config the colour column is. + const { spatialData, loadObsColumns } = tableSpatialData(); + const shapes = shapesElement('cells'); + // The tooltip path aligns table rows to feature ids, so it needs this too. + (shapes.element as { loadFeatureIds?: unknown }).loadFeatureIds = vi.fn(async () => [ + 'c1', + 'c2', + ]); + const elements: ElementsByType = { ...EMPTY_ELEMENTS, shapes: [shapes] }; + const config: LayerConfig = { ...shapesConfig('layer-1', 'cells'), tooltipFields: ['colA'] }; + const layers = { 'layer-1': config }; + const layerOrder = Object.keys(layers); + + const { rerender } = renderHook(() => + useLayerData(layers, layerOrder, elements, null, spatialData) + ); + + await waitFor(() => { + expect(requestedColumns(loadObsColumns)).toContain('colA'); + }); + + config.tooltipFields = ['colB']; + rerender(); + + await waitFor(() => { + expect(requestedColumns(loadObsColumns)).toContain('colB'); + }); + }); +}); From 0dcba6286c8a13a70c7d68e7f2fefba9512a8780 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Wed, 5 Aug 2026 21:10:23 +0100 Subject: [PATCH 2/8] Drop the redundant tooltip-fields behavioural test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- packages/vis/tests/useLayerData.spec.tsx | 31 ------------------------ 1 file changed, 31 deletions(-) diff --git a/packages/vis/tests/useLayerData.spec.tsx b/packages/vis/tests/useLayerData.spec.tsx index 215cfd77..4ea06048 100644 --- a/packages/vis/tests/useLayerData.spec.tsx +++ b/packages/vis/tests/useLayerData.spec.tsx @@ -723,35 +723,4 @@ describe('useLayerData — a caller that mutates its layer configs in place', () expect(requestedColumns(loadObsColumns)).toContain('colB'); }); }); - - it('loads the new tooltip fields when they are switched in place', async () => { - // Same defect, different resource: the tooltip columns are planned from the same - // config the colour column is. - const { spatialData, loadObsColumns } = tableSpatialData(); - const shapes = shapesElement('cells'); - // The tooltip path aligns table rows to feature ids, so it needs this too. - (shapes.element as { loadFeatureIds?: unknown }).loadFeatureIds = vi.fn(async () => [ - 'c1', - 'c2', - ]); - const elements: ElementsByType = { ...EMPTY_ELEMENTS, shapes: [shapes] }; - const config: LayerConfig = { ...shapesConfig('layer-1', 'cells'), tooltipFields: ['colA'] }; - const layers = { 'layer-1': config }; - const layerOrder = Object.keys(layers); - - const { rerender } = renderHook(() => - useLayerData(layers, layerOrder, elements, null, spatialData) - ); - - await waitFor(() => { - expect(requestedColumns(loadObsColumns)).toContain('colA'); - }); - - config.tooltipFields = ['colB']; - rerender(); - - await waitFor(() => { - expect(requestedColumns(loadObsColumns)).toContain('colB'); - }); - }); }); From 84a4532de9296cedbc9245da0cbee6b179bfa4be Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Fri, 7 Aug 2026 14:06:51 +0100 Subject: [PATCH 3/8] Make a column's colours a property of the column, not of the view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .changeset/column-colour-not-view-colour.md | 39 ++++ packages/layers/src/featureColorEncoding.ts | 188 +++++++++++++--- packages/layers/src/index.ts | 3 + packages/layers/src/labelColorEncoding.ts | 5 + packages/layers/src/shapeColorEncoding.ts | 5 + .../layers/tests/featureColorEncoding.spec.ts | 201 +++++++++++++++++- .../layers/tests/labelColorEncoding.spec.ts | 38 +++- .../layers/tests/shapeColorEncoding.spec.ts | 13 +- .../vis/src/SpatialCanvas/labelsProjection.ts | 9 +- .../vis/src/SpatialCanvas/shapesProjection.ts | 6 +- packages/vis/src/SpatialCanvas/types.ts | 16 +- .../vis/src/SpatialCanvas/useLayerData.ts | 3 + packages/vis/tests/labelsProjection.spec.ts | 8 +- 13 files changed, 474 insertions(+), 60 deletions(-) create mode 100644 .changeset/column-colour-not-view-colour.md diff --git a/.changeset/column-colour-not-view-colour.md b/.changeset/column-colour-not-view-colour.md new file mode 100644 index 00000000..79669004 --- /dev/null +++ b/.changeset/column-colour-not-view-colour.md @@ -0,0 +1,39 @@ +--- +'@spatialdata/layers': minor +'@spatialdata/vis': minor +--- + +Make a column's colours a property of the column, not of the features that loaded. + +Three things decided the encoding from whatever happened to be in view, so two +layers over one annotation could disagree about what a colour means — which reads +as a data difference rather than as a bug: + +- 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 + the same `cell_type` column rendered in two different schemes on the two kinds. + (`labelColorEncoding.spec.ts` claimed to cover this, but only pinned the indices + on one kind; it now actually builds the column through both.) Categories are now + ordered by value, with numeric-looking values ordered numerically so cluster 10 + follows cluster 9 rather than cluster 1. +- Positional palettes cannot survive a category being **absent from a view** at + all: `tumour` genuinely is the second category present when `stroma` is not. + `categoricalPalette` therefore also accepts `{ 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 embedding + application can use to make a layer agree with its own charts. +- The continuous ramp measured its extent from the loaded features. `numericDomain` + pins it to the column's own range; values outside clamp rather than extrapolate. + +`featureColorSchemeSignature` now takes the scheme as one object +(`featureColorSchemeSignature(config.fillColorByColumn)`) rather than three +positional arguments, so adding a term to the encoding cannot leave a call site +silently keying on the old set — the failure mode there being a layer that keeps +serving the previous colours after the scheme changed. Named palettes are +serialised in sorted key order, since object key order is insertion order and a +host rebuilding its palette each render need not insert in a stable one. + +**Colours will change** for existing categorical configs that relied on the +implicit first-seen order. Pass `categoricalPalette: { byValue }` to fix a scheme +in place. diff --git a/packages/layers/src/featureColorEncoding.ts b/packages/layers/src/featureColorEncoding.ts index 3baafb95..a198d7ba 100644 --- a/packages/layers/src/featureColorEncoding.ts +++ b/packages/layers/src/featureColorEncoding.ts @@ -76,9 +76,38 @@ export function featureColorAt( ]; } +/** + * A category's colour named by the category itself, rather than by its position. + * + * This is the only form that survives the data changing. The other two are + * positional — a colour is whatever the Nth category gets — and "the Nth category" + * is a property of the features that happened to load, not of the column. Two + * layers over the same annotation therefore disagree the moment their feature sets + * differ, which is the normal case: a shapes layer walks the loader's geometry + * order and a labels layer walks its raster ids. + * + * It is also the only form a host can use to say what it means. A viewer embedded + * in an application whose user has already chosen "Tumour is red" cannot express + * that as a list, because it does not know — and must not have to know — which + * index `Tumour` will land on. + * + * Values are the column's cells in canonical string form (see + * {@link normalizeFeatureCellValue}), so a numeric category is `'3'`, not `3`. + */ +export interface FeatureNamedCategoricalPalette { + byValue: Readonly>; + /** + * Colour for a value the map does not name. Defaults to `'oklab'`, which gives + * each unnamed category its own hue — an unnamed category stays visible and + * distinguishable rather than silently merging into a single "other" bucket. + */ + fallback?: 'oklab' | FeatureRgbColor; +} + /** * How to colour categories. JSON-serializable on purpose — this travels in a saved - * layer config, so it is a name or a plain list of colours, never a function. + * layer config, so it is a name, a plain list of colours, or a plain object, never + * a function. * * - `'oklab'` — the points colour-by-feature scheme: OKLCh at fixed lightness and * chroma, hue stepped by the golden angle. **Unbounded** — every @@ -86,11 +115,29 @@ export function featureColorAt( * annotation does not repeat colours. The default. * - a list — your own colours, cycled. An empty list falls back to `'oklab'` * rather than colouring nothing. + * - a map — {@link FeatureNamedCategoricalPalette}. Prefer this whenever you + * know what the categories are; the two positional forms depend on + * which features loaded. */ -export type FeatureCategoricalPaletteSpec = 'oklab' | readonly FeatureRgbColor[]; +export type FeatureCategoricalPaletteSpec = + | 'oklab' + | readonly FeatureRgbColor[] + | FeatureNamedCategoricalPalette; export type FeatureNumericRampSpec = readonly [FeatureRgbColor, FeatureRgbColor]; +/** + * The values the ramp's endpoints stand for, `[low, high]`. + * + * Without one, the extent is measured from the features that loaded — so the same + * column reads as a different scale on a layer covering a subset, and the colours + * of two layers over one annotation are not comparable. Pin it to the column's own + * range (which the store knows and the render does not) whenever you have it. + * + * Values outside the domain clamp to its endpoints rather than extrapolating. + */ +export type FeatureNumericDomain = readonly [number, number]; + export const DEFAULT_FEATURE_CATEGORICAL_PALETTE: FeatureCategoricalPaletteSpec = 'oklab'; export const DEFAULT_FEATURE_NUMERIC_RAMP: FeatureNumericRampSpec = [ @@ -98,54 +145,97 @@ export const DEFAULT_FEATURE_NUMERIC_RAMP: FeatureNumericRampSpec = [ [255, 220, 0], ]; +function isNamedCategoricalPalette( + spec: FeatureCategoricalPaletteSpec +): spec is FeatureNamedCategoricalPalette { + return typeof spec === 'object' && !Array.isArray(spec); +} + /** - * Turn a palette spec into `categoryIndex → colour`. + * Turn a palette spec into `(value, categoryIndex) → colour`. * * A function rather than an array because `'oklab'` has no length: its colour is a * pure function of the index, so there is no table to run out of. That is the whole * point of making it the default — the cycling of a fixed list is invisible in the * render (two cell types simply share a colour) and so is exactly the kind of bug * that survives review. + * + * Both arguments are passed because the spec decides which one is authoritative: a + * named palette answers from the value, the positional forms from the index. */ export function resolveCategoricalPalette( spec: FeatureCategoricalPaletteSpec = DEFAULT_FEATURE_CATEGORICAL_PALETTE -): (categoryIndex: number) => FeatureRgbColor { +): (value: string, categoryIndex: number) => FeatureRgbColor { if (spec === 'oklab') { - return featureCodeToRgb; + return (_value, categoryIndex) => featureCodeToRgb(categoryIndex); + } + if (isNamedCategoricalPalette(spec)) { + const { byValue, fallback = 'oklab' } = spec; + const colorForUnnamed = fallback === 'oklab' ? featureCodeToRgb : (_index: number) => fallback; + return (value, categoryIndex) => byValue[value] ?? colorForUnnamed(categoryIndex); } const colors = spec; if (colors.length === 0) { - return featureCodeToRgb; + return (_value, categoryIndex) => featureCodeToRgb(categoryIndex); } - return (categoryIndex) => colors[categoryIndex % colors.length]; + return (_value, categoryIndex) => colors[categoryIndex % colors.length]; +} + +/** Everything about a column's encoding that is not the column itself. */ +export interface FeatureColorScheme { + categoricalPalette?: FeatureCategoricalPaletteSpec; + numericRamp?: FeatureNumericRampSpec; + numericDomain?: FeatureNumericDomain; + missingValues?: FeatureMissingValueOptions; } /** * Stable serialisation of a scheme, for projection cache keys. * + * Takes the whole scheme as one object so that adding a term to the encoding + * cannot leave a call site silently keying on the old set — the failure mode being + * a layer that keeps serving the previous colours after the scheme changed. + * * The missing-value policy belongs here too: changing a sentinel or how a missing - * feature renders changes colours without touching the column, so a key that - * omitted it would keep serving the previous table. + * feature renders changes colours without touching the column. */ -export function featureColorSchemeSignature( - categoricalPalette?: FeatureCategoricalPaletteSpec, - numericRamp?: FeatureNumericRampSpec, - missingValues?: FeatureMissingValueOptions -): string { +export function featureColorSchemeSignature({ + categoricalPalette, + numericRamp, + numericDomain, + missingValues, +}: FeatureColorScheme = {}): string { if ( categoricalPalette === undefined && numericRamp === undefined && + numericDomain === undefined && missingValues === undefined ) { return ''; } return JSON.stringify([ - categoricalPalette ?? null, + serializeCategoricalPalette(categoricalPalette), numericRamp ?? null, + numericDomain ?? null, missingValues ? [missingValues.treatAsMissing ?? null, missingValues.render ?? null] : null, ]); } +/** + * A named palette is serialised in sorted key order, because object key order is + * insertion order and a caller rebuilding the same map per render need not insert + * in a stable one. Two equal maps have to produce one string, or the layer rebuilds + * its whole colour buffer on renders where nothing changed. + */ +function serializeCategoricalPalette(spec: FeatureCategoricalPaletteSpec | undefined): unknown { + if (spec === undefined) return null; + if (!isNamedCategoricalPalette(spec)) return spec; + return [ + Object.entries(spec.byValue).sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)), + spec.fallback ?? null, + ]; +} + /** * A cell rendered as the canonical string form; `''` means "no usable value". * @@ -301,6 +391,8 @@ export interface AssignFeatureColorsOptions { alpha: number; categoricalPalette?: FeatureCategoricalPaletteSpec; numericRamp?: FeatureNumericRampSpec; + /** Pin the ramp's endpoints instead of measuring them. See {@link FeatureNumericDomain}. */ + numericDomain?: FeatureNumericDomain; /** * What the store declares this column to be. Supply it whenever you have it — * `'auto'` trusts it in preference to sniffing the values. See @@ -310,6 +402,31 @@ export interface AssignFeatureColorsOptions { missingValues?: FeatureMissingValueOptions; } +/** + * Category ordering, and so — for the positional palettes — category colour. + * + * Sorted rather than first-seen. First-seen order is a property of the features + * that loaded, not of the column, so it made the same annotation render in + * different colours on a shapes layer and a labels layer over one table, and made a + * saved config's colours drift whenever the data behind it changed. + * + * Numeric-looking values sort numerically, so cluster `10` follows cluster `9` + * rather than cluster `1`; those are the commonest positional categories and + * lexicographic order makes their palette look shuffled. Anything else sorts by + * code unit — deliberately not `localeCompare`, whose answer depends on the + * environment's locale data and would make the colours machine-dependent. + */ +function compareCategoryValues(a: string, b: string): number { + const numericA = featureNumericValue(a); + const numericB = featureNumericValue(b); + if (numericA !== undefined && numericB !== undefined && numericA !== numericB) { + return numericA - numericB; + } + if (numericA !== undefined && numericB === undefined) return -1; + if (numericA === undefined && numericB !== undefined) return 1; + return a < b ? -1 : a > b ? 1 : 0; +} + /** * Colour every feature from its column value. * @@ -319,8 +436,11 @@ export interface AssignFeatureColorsOptions { * of a dense colour buffer — "unannotated" and "annotated with the first palette * entry" have to stay distinguishable. * - * Category indices are assigned in first-seen feature order, so the same column - * yields the same colours for a given element regardless of which kind draws it. + * The encoding is a function of the COLUMN, not of the features that loaded: + * categories are ordered by {@link compareCategoryValues} and the ramp can be + * pinned with `numericDomain`. That is what lets the same annotation render the + * same way on a shapes layer and on a labels layer over the same table, and what + * makes a saved config's colours mean the same thing next time it is opened. */ export function assignFeatureColors({ values, @@ -328,6 +448,7 @@ export function assignFeatureColors({ alpha, categoricalPalette, numericRamp = DEFAULT_FEATURE_NUMERIC_RAMP, + numericDomain, columnKind, missingValues, }: AssignFeatureColorsOptions): Array { @@ -361,7 +482,9 @@ export function assignFeatureColors({ const numericValues = values.map((value) => isMissing(value) ? undefined : featureNumericValue(value) ); - const extent = getFiniteExtent(numericValues); + // A pinned domain is used even when nothing in view falls inside it: the point + // of pinning is that the scale does not depend on what loaded. + const extent = numericDomain ?? getFiniteExtent(numericValues); if (!extent) { for (let index = 0; index < values.length; index += 1) applyMissing(index); return colors; @@ -382,19 +505,36 @@ export function assignFeatureColors({ return colors; } + // Two passes: the category set has to be complete and ordered before any colour + // is assigned, because a positional palette's answer for the first feature + // depends on categories that may only appear near the end of the column. const categoryIndexByValue = new Map(); + 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); + } + + // The palette is consulted once per CATEGORY rather than once per feature — a + // categorical column is a handful of distinct values over potentially millions of + // rows. Each feature still gets its own tuple: these are handed out to callers + // that store them per feature, and sharing one array between a category's + // features would make any in-place edit recolour all of them. + const rgbByValue = new Map(); + for (const [value, categoryIndex] of categoryIndexByValue) { + rgbByValue.set(value, colorForCategory(value, categoryIndex)); + } + for (let index = 0; index < values.length; index += 1) { const value = values[index]; if (isMissing(value)) { applyMissing(index); continue; } - let categoryIndex = categoryIndexByValue.get(value); - if (categoryIndex === undefined) { - categoryIndex = categoryIndexByValue.size; - categoryIndexByValue.set(value, categoryIndex); - } - colors[index] = rgba(colorForCategory(categoryIndex), alpha); + const rgb = rgbByValue.get(value); + if (rgb) colors[index] = rgba(rgb, alpha); } return colors; diff --git a/packages/layers/src/index.ts b/packages/layers/src/index.ts index efa0eb90..d7f61b56 100644 --- a/packages/layers/src/index.ts +++ b/packages/layers/src/index.ts @@ -42,9 +42,12 @@ export type { AssignFeatureColorsOptions, FeatureCategoricalPaletteSpec, FeatureColorBuffer, + FeatureColorScheme, FeatureColumnKind, FeatureFillColorMode, FeatureMissingValueOptions, + FeatureNamedCategoricalPalette, + FeatureNumericDomain, FeatureNumericRampSpec, FeatureRgbaColor, FeatureRgbColor, diff --git a/packages/layers/src/labelColorEncoding.ts b/packages/layers/src/labelColorEncoding.ts index 096decb3..8f51563a 100644 --- a/packages/layers/src/labelColorEncoding.ts +++ b/packages/layers/src/labelColorEncoding.ts @@ -39,6 +39,7 @@ import { type FeatureColumnKind, type FeatureFillColorMode, type FeatureMissingValueOptions, + type FeatureNumericDomain, type FeatureNumericRampSpec, type FeatureRgbaColor, type FeatureRgbColor, @@ -213,6 +214,8 @@ export interface BuildLabelFillColorByFeatureIdOptions { alpha?: number; categoricalPalette?: FeatureCategoricalPaletteSpec; numericRamp?: FeatureNumericRampSpec; + /** Pin the ramp's endpoints instead of measuring them across the loaded features. */ + numericDomain?: FeatureNumericDomain; /** What the store declares the column to be; `'auto'` trusts it over the values. */ columnKind?: FeatureColumnKind; missingValues?: FeatureMissingValueOptions; @@ -232,6 +235,7 @@ export function buildLabelFillColorByFeatureId({ alpha = 255, categoricalPalette, numericRamp, + numericDomain, columnKind, missingValues, }: BuildLabelFillColorByFeatureIdOptions): Record { @@ -250,6 +254,7 @@ export function buildLabelFillColorByFeatureId({ alpha, ...(categoricalPalette ? { categoricalPalette } : {}), ...(numericRamp ? { numericRamp } : {}), + ...(numericDomain ? { numericDomain } : {}), ...(columnKind ? { columnKind } : {}), ...(missingValues ? { missingValues } : {}), }); diff --git a/packages/layers/src/shapeColorEncoding.ts b/packages/layers/src/shapeColorEncoding.ts index 7779c387..258c223f 100644 --- a/packages/layers/src/shapeColorEncoding.ts +++ b/packages/layers/src/shapeColorEncoding.ts @@ -14,6 +14,7 @@ import { type FeatureColumnKind, type FeatureFillColorMode, type FeatureMissingValueOptions, + type FeatureNumericDomain, type FeatureNumericRampSpec, type FeatureRgbaColor, type FeatureRgbColor, @@ -35,6 +36,8 @@ export interface BuildShapeFillColorByFeatureIdOptions { alpha: number; categoricalPalette?: FeatureCategoricalPaletteSpec; numericRamp?: FeatureNumericRampSpec; + /** Pin the ramp's endpoints instead of measuring them across the loaded features. */ + numericDomain?: FeatureNumericDomain; /** What the store declares the column to be; `'auto'` trusts it over the values. */ columnKind?: FeatureColumnKind; missingValues?: FeatureMissingValueOptions; @@ -54,6 +57,7 @@ export function buildShapeFillColorByFeatureId({ alpha, categoricalPalette, numericRamp, + numericDomain, columnKind, missingValues, }: BuildShapeFillColorByFeatureIdOptions): Record { @@ -70,6 +74,7 @@ export function buildShapeFillColorByFeatureId({ alpha, ...(categoricalPalette ? { categoricalPalette } : {}), ...(numericRamp ? { numericRamp } : {}), + ...(numericDomain ? { numericDomain } : {}), ...(columnKind ? { columnKind } : {}), ...(missingValues ? { missingValues } : {}), }); diff --git a/packages/layers/tests/featureColorEncoding.spec.ts b/packages/layers/tests/featureColorEncoding.spec.ts index 56abc4e7..2fcccbb3 100644 --- a/packages/layers/tests/featureColorEncoding.spec.ts +++ b/packages/layers/tests/featureColorEncoding.spec.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest'; import { assignFeatureColors, featureColorAt, + featureColorSchemeSignature, resolveFeatureFillColorMode, } from '../src/featureColorEncoding'; @@ -85,11 +86,12 @@ describe('missing-value policy', () => { missingValues: { treatAsMissing: ['NA'] }, }); - expect(withSentinel[0]).toEqual(RED); + // Two real categories take the first two palette slots between them: the + // sentinel never entered the category set, so it did not consume one. (Which + // of the two is slot 0 is decided by value order — 'stroma' before 'tumour'.) + expect(withSentinel[0]).toEqual([0, 255, 0, 255]); expect(withSentinel[1]).toBeUndefined(); - // 'stroma' is the SECOND real category, not the third — the sentinel never - // entered the category set, so it did not consume a palette slot. - expect(withSentinel[2]).toEqual([0, 255, 0, 255]); + expect(withSentinel[2]).toEqual(RED); }); it('matches sentinels after trimming, case-insensitively', () => { @@ -162,3 +164,194 @@ describe('featureColorAt bounds', () => { expect(featureColorAt(padded, 1)).toBeUndefined(); }); }); + +/** + * The encoding must be a function of the COLUMN, not of the features that + * happened to load. Everything here is a case where it used not to be, and where + * the symptom was two views of one annotation disagreeing about what a colour + * means — which reads as a data difference, not as a bug. + */ +describe('an encoding that does not depend on which features loaded', () => { + const palette: [number, number, number][] = [ + [255, 0, 0], + [0, 255, 0], + [0, 0, 255], + ]; + + it('gives a category the same colour whatever order the features arrive in', () => { + const forward = assignFeatureColors({ + values: ['tumour', 'stroma'], + mode: 'categorical', + alpha: 255, + categoricalPalette: palette, + }); + const reversed = assignFeatureColors({ + values: ['stroma', 'tumour'], + mode: 'categorical', + alpha: 255, + categoricalPalette: palette, + }); + + expect(forward[0]).toEqual(reversed[1]); + expect(forward[1]).toEqual(reversed[0]); + }); + + it('does not shift a category when another one is absent from the view', () => { + // A layer over a subset that happens to contain no `stroma` must still draw + // `tumour` in the colour the full view draws it in. + const all = assignFeatureColors({ + values: ['alpha', 'stroma', 'tumour'], + mode: 'categorical', + alpha: 255, + categoricalPalette: palette, + }); + const subset = assignFeatureColors({ + values: ['alpha', 'tumour'], + mode: 'categorical', + alpha: 255, + categoricalPalette: palette, + }); + + expect(subset[0]).toEqual(all[0]); + // Positional palettes cannot survive this — `tumour` genuinely is the second + // category present. Naming the colours is the only fix, which is what the + // named-palette tests below cover; here we pin the shift so it stays visible. + expect(subset[1]).not.toEqual(all[2]); + }); + + it('orders numeric-looking categories numerically, not lexicographically', () => { + // Cluster 10 belongs after cluster 9. Under string order it lands between 1 + // and 2, and a 12-cluster annotation renders with a shuffled palette. + const colors = assignFeatureColors({ + values: ['1', '2', '10'], + mode: 'categorical', + alpha: 255, + categoricalPalette: palette, + }); + + expect(colors).toEqual([ + [255, 0, 0, 255], + [0, 255, 0, 255], + [0, 0, 255, 255], + ]); + }); + + it('holds the ramp to a pinned domain instead of the loaded extent', () => { + const pinned = { mode: 'continuous' as const, alpha: 255, numericDomain: [0, 10] as const }; + + // The same value, on two layers covering different parts of the column. + const full = assignFeatureColors({ values: ['0', '3', '10'], ...pinned }); + const subset = assignFeatureColors({ values: ['0', '3'], ...pinned }); + + expect(subset[1]).toEqual(full[1]); + // And without the domain it does not hold: in the subset, `3` is the top of + // the range rather than three tenths of the way up it. + const unpinned = assignFeatureColors({ + values: ['0', '3'], + mode: 'continuous', + alpha: 255, + }); + expect(unpinned[1]).not.toEqual(full[1]); + }); + + it('clamps values outside a pinned domain rather than extrapolating', () => { + const colors = assignFeatureColors({ + values: ['-100', '0', '10', '900'], + mode: 'continuous', + alpha: 255, + numericDomain: [0, 10], + }); + + expect(colors[0]).toEqual(colors[1]); + expect(colors[3]).toEqual(colors[2]); + }); +}); + +describe('a palette that names its categories', () => { + it('colours by value, so two views agree even on different category sets', () => { + const byValue = { tumour: [200, 30, 30] as [number, number, number] }; + const all = assignFeatureColors({ + values: ['alpha', 'stroma', 'tumour'], + mode: 'categorical', + alpha: 255, + categoricalPalette: { byValue }, + }); + const subset = assignFeatureColors({ + values: ['tumour'], + mode: 'categorical', + alpha: 255, + categoricalPalette: { byValue }, + }); + + expect(all[2]).toEqual([200, 30, 30, 255]); + expect(subset[0]).toEqual(all[2]); + }); + + it('gives an unnamed category its own hue rather than merging them', () => { + const colors = assignFeatureColors({ + values: ['tumour', 'stroma', 'other'], + mode: 'categorical', + alpha: 255, + categoricalPalette: { byValue: { tumour: [200, 30, 30] } }, + }); + + expect(colors[1]).not.toEqual(colors[2]); + }); + + it('honours an explicit fallback colour for everything unnamed', () => { + const colors = assignFeatureColors({ + values: ['tumour', 'stroma', 'other'], + mode: 'categorical', + alpha: 255, + categoricalPalette: { byValue: { tumour: [200, 30, 30] }, fallback: [90, 90, 90] }, + }); + + expect(colors[0]).toEqual([200, 30, 30, 255]); + expect(colors[1]).toEqual([90, 90, 90, 255]); + expect(colors[2]).toEqual([90, 90, 90, 255]); + }); + + it('does not name missing values into a category', () => { + // A named palette must not resurrect a sentinel: `isMissing` runs first, so an + // entry for the sentinel string is simply never consulted. + const colors = assignFeatureColors({ + values: ['tumour', 'NA'], + mode: 'categorical', + alpha: 255, + categoricalPalette: { byValue: { tumour: [200, 30, 30], NA: [1, 2, 3] } }, + missingValues: { treatAsMissing: ['NA'] }, + }); + + expect(colors[1]).toBeUndefined(); + }); +}); + +describe('featureColorSchemeSignature', () => { + it('separates schemes that differ only in a pinned domain', () => { + expect(featureColorSchemeSignature({ numericDomain: [0, 10] })).not.toBe( + featureColorSchemeSignature({ numericDomain: [0, 20] }) + ); + }); + + it('reads two equal named palettes as one scheme whatever order they were built in', () => { + // Object key order is insertion order, and a host rebuilding its palette per + // render need not insert in a stable one. A signature that moved would rebuild + // the whole colour buffer on renders where nothing changed. + const a = featureColorSchemeSignature({ + categoricalPalette: { byValue: { tumour: [1, 2, 3], stroma: [4, 5, 6] } }, + }); + const b = featureColorSchemeSignature({ + categoricalPalette: { byValue: { stroma: [4, 5, 6], tumour: [1, 2, 3] } }, + }); + + expect(a).toBe(b); + }); + + it('still separates named palettes that differ in a colour', () => { + expect( + featureColorSchemeSignature({ categoricalPalette: { byValue: { tumour: [1, 2, 3] } } }) + ).not.toBe( + featureColorSchemeSignature({ categoricalPalette: { byValue: { tumour: [9, 9, 9] } } }) + ); + }); +}); diff --git a/packages/layers/tests/labelColorEncoding.spec.ts b/packages/layers/tests/labelColorEncoding.spec.ts index 74613673..54a8ce15 100644 --- a/packages/layers/tests/labelColorEncoding.spec.ts +++ b/packages/layers/tests/labelColorEncoding.spec.ts @@ -8,6 +8,7 @@ import { resolveHighlightedLabel, } from '../src/labelColorEncoding'; import { featureCodeToRgb } from '../src/pointsFeatureColor'; +import { buildShapeFillColorByFeatureId } from '../src/shapeColorEncoding'; /** A small fixed palette, for tests whose subject is row alignment rather than * colour choice — the default scheme is procedural, so colours must be pinned. */ @@ -44,29 +45,44 @@ describe('label fill colour encoding', () => { }); expect(colors).toEqual({ - '1': [0, 0, 255, 255], - '2': [0, 255, 0, 255], - '3': [0, 0, 255, 255], + // `stroma` sorts before `tumour`, so it takes palette slot 0. + '1': [0, 255, 0, 255], + '2': [0, 0, 255, 255], + '3': [0, 255, 0, 255], }); }); it('gives a label the same colour the same category gets on a shapes layer', () => { - const shared = { + // The two kinds walk their features in orders neither controls: a labels layer + // walks the raster's ids, a shapes layer walks the loader's geometry. Here they + // walk the SAME two rows in OPPOSITE orders, which is the whole test — under + // first-seen category indices `tumour` would be slot 0 on one kind and slot 1 + // on the other, and one annotation would render in two different colour schemes. + const column = ['tumour', 'stroma']; + + const labelColors = buildLabelFillColorByFeatureId({ rowIds: ['1', '2'], rowIndexByFeatureId: new Map([ ['1', 0], ['2', 1], ]), - column: ['tumour', 'stroma'], - mode: 'categorical' as const, - }; + column, + mode: 'categorical', + }); + const shapeColors = buildShapeFillColorByFeatureId({ + featureIds: ['stroma-shape', 'tumour-shape'], + rowIndexByFeatureIndex: new Int32Array([1, 0]), + column, + mode: 'categorical', + alpha: 255, + }); + expect(labelColors['1']).toEqual(shapeColors['tumour-shape']); + expect(labelColors['2']).toEqual(shapeColors['stroma-shape']); // Default scheme, no palette passed: the OkLab colours points uses for codes // 0 and 1. One scheme across points, shapes and labels. - expect(buildLabelFillColorByFeatureId(shared)).toEqual({ - '1': [...featureCodeToRgb(0), 255], - '2': [...featureCodeToRgb(1), 255], - }); + expect(labelColors['2']).toEqual([...featureCodeToRgb(0), 255]); + expect(labelColors['1']).toEqual([...featureCodeToRgb(1), 255]); }); it('uses the same palette and ramp as shapes for the same column', () => { diff --git a/packages/layers/tests/shapeColorEncoding.spec.ts b/packages/layers/tests/shapeColorEncoding.spec.ts index 35c89617..7b864904 100644 --- a/packages/layers/tests/shapeColorEncoding.spec.ts +++ b/packages/layers/tests/shapeColorEncoding.spec.ts @@ -25,9 +25,11 @@ describe('shape fill colour encoding', () => { }); expect(colors).toEqual({ - 'cell-a': [0, 0, 255, 180], - 'cell-b': [0, 255, 0, 180], - 'cell-c': [0, 0, 255, 180], + // Categories are ordered by value, not by which feature was seen first, so + // `type-x` takes palette slot 0 even though `type-y` is drawn first. + 'cell-a': [0, 255, 0, 180], + 'cell-b': [0, 0, 255, 180], + 'cell-c': [0, 255, 0, 180], 'cell-d': [255, 0, 255, 180], }); }); @@ -122,8 +124,9 @@ describe('shape fill colour encoding', () => { }); expect(colors).toEqual({ - 'circle-a': [0, 0, 255, 180], - 'circle-b': [0, 255, 0, 180], + // Row 1 is `type-y` (slot 1), row 0 is `type-x` (slot 0). + 'circle-a': [0, 255, 0, 180], + 'circle-b': [0, 0, 255, 180], }); }); diff --git a/packages/vis/src/SpatialCanvas/labelsProjection.ts b/packages/vis/src/SpatialCanvas/labelsProjection.ts index 6cbd7a16..322368d9 100644 --- a/packages/vis/src/SpatialCanvas/labelsProjection.ts +++ b/packages/vis/src/SpatialCanvas/labelsProjection.ts @@ -85,11 +85,7 @@ export function getLabelFillColorSignature(config: LayerConfig | undefined): str const mode: LabelFillColorMode = config.fillColorByColumn.mode; // The scheme is part of the key: swapping a palette changes every colour without // touching the column, so a column-only key would keep serving the old colours. - const scheme = featureColorSchemeSignature( - config.fillColorByColumn.categoricalPalette, - config.fillColorByColumn.numericRamp, - config.fillColorByColumn.missingValues - ); + const scheme = featureColorSchemeSignature(config.fillColorByColumn); return [config.fillColorByColumn.columnName, mode, scheme].join(''); } @@ -137,6 +133,9 @@ export function buildLabelFillColorEntry( ? { categoricalPalette: fillColorByColumn.categoricalPalette } : {}), ...(fillColorByColumn.numericRamp ? { numericRamp: fillColorByColumn.numericRamp } : {}), + ...(fillColorByColumn.numericDomain + ? { numericDomain: fillColorByColumn.numericDomain } + : {}), }), rowsSource: rows, }; diff --git a/packages/vis/src/SpatialCanvas/shapesProjection.ts b/packages/vis/src/SpatialCanvas/shapesProjection.ts index 19f7d5d7..18540bd5 100644 --- a/packages/vis/src/SpatialCanvas/shapesProjection.ts +++ b/packages/vis/src/SpatialCanvas/shapesProjection.ts @@ -77,11 +77,7 @@ export function getShapeFillColorSignature(config: LayerConfig | undefined): str const mode: ShapeFillColorMode = config.fillColorByColumn.mode; // The scheme is part of the key: swapping a palette changes every colour without // touching the column, so a column-only key would keep serving the old colours. - const scheme = featureColorSchemeSignature( - config.fillColorByColumn.categoricalPalette, - config.fillColorByColumn.numericRamp, - config.fillColorByColumn.missingValues - ); + const scheme = featureColorSchemeSignature(config.fillColorByColumn); return [ config.fillColorByColumn.columnName, mode, diff --git a/packages/vis/src/SpatialCanvas/types.ts b/packages/vis/src/SpatialCanvas/types.ts index 6d49c48f..b4edfd9d 100644 --- a/packages/vis/src/SpatialCanvas/types.ts +++ b/packages/vis/src/SpatialCanvas/types.ts @@ -7,6 +7,7 @@ import type { SpatialElement } from '@spatialdata/core'; import type { FeatureCategoricalPaletteSpec, FeatureMissingValueOptions, + FeatureNumericDomain, FeatureNumericRampSpec, LabelFillColorMode, ShapeFillColorMode, @@ -40,12 +41,23 @@ export interface FillColorByColumn { /** * Categorical scheme. Defaults to `'oklab'` — the same unbounded golden-angle * OKLCh scheme points uses for colour-by-feature, so a column with more - * categories than a fixed list has colours does not repeat them. Pass your own - * RGB list to override it; the list cycles. + * categories than a fixed list has colours does not repeat them. + * + * Pass `{ byValue }` to name the colours — `{ Tumour: [200, 30, 30] }` — which is + * the form to prefer in a saved stack, and the only one an embedding application + * can use to make this layer agree with its own charts. An RGB list is also + * accepted and cycles, but it is positional: which category gets which entry + * depends on the categories present. */ categoricalPalette?: FeatureCategoricalPaletteSpec; /** Endpoints of the continuous ramp, `[low, high]` as RGB 0–255. */ numericRamp?: FeatureNumericRampSpec; + /** + * The values those endpoints stand for. Defaults to the extent of the features + * that loaded — pin it to the column's own range when you know it, so two layers + * over different parts of one annotation stay comparable. + */ + numericDomain?: FeatureNumericDomain; /** * What counts as missing in this column, and how a feature with no value should * render — keep the layer default, hide it, or take an explicit colour. diff --git a/packages/vis/src/SpatialCanvas/useLayerData.ts b/packages/vis/src/SpatialCanvas/useLayerData.ts index cc77c34b..adfee981 100644 --- a/packages/vis/src/SpatialCanvas/useLayerData.ts +++ b/packages/vis/src/SpatialCanvas/useLayerData.ts @@ -814,6 +814,9 @@ export function useLayerData( ? { categoricalPalette: fillColorByColumn.categoricalPalette } : {}), ...(fillColorByColumn.numericRamp ? { numericRamp: fillColorByColumn.numericRamp } : {}), + ...(fillColorByColumn.numericDomain + ? { numericDomain: fillColorByColumn.numericDomain } + : {}), }), rowsSource: rows, renderSource: renderData, diff --git a/packages/vis/tests/labelsProjection.spec.ts b/packages/vis/tests/labelsProjection.spec.ts index 0cdc9dcb..2d7dd061 100644 --- a/packages/vis/tests/labelsProjection.spec.ts +++ b/packages/vis/tests/labelsProjection.spec.ts @@ -60,8 +60,8 @@ describe('buildLabelFillColorEntry', () => { ); expect(entry?.fillColorByFeatureId).toEqual({ - '1': [0, 0, 255, 255], - '2': [0, 255, 0, 255], + '1': [0, 255, 0, 255], + '2': [0, 0, 255, 255], }); }); @@ -81,8 +81,8 @@ describe('buildLabelFillColorEntry', () => { ); expect(entry?.fillColorByFeatureId).toEqual({ - '1': [10, 20, 30, 255], - '2': [40, 50, 60, 255], + '1': [40, 50, 60, 255], + '2': [10, 20, 30, 255], }); }); From dfe71e92cfceeff307c7916aa6174a5cee01b4d6 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Fri, 7 Aug 2026 14:11:46 +0100 Subject: [PATCH 4/8] Let a continuous column carry a real ramp, not just two endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .changeset/column-colour-not-view-colour.md | 9 +++ packages/layers/src/featureColorEncoding.ts | 74 ++++++++++++++++- packages/layers/src/index.ts | 1 + packages/layers/src/labelColorEncoding.ts | 5 ++ packages/layers/src/shapeColorEncoding.ts | 5 ++ .../layers/tests/featureColorEncoding.spec.ts | 81 +++++++++++++++++++ .../vis/src/SpatialCanvas/labelsProjection.ts | 1 + packages/vis/src/SpatialCanvas/types.ts | 6 ++ .../vis/src/SpatialCanvas/useLayerData.ts | 3 + 9 files changed, 181 insertions(+), 4 deletions(-) diff --git a/.changeset/column-colour-not-view-colour.md b/.changeset/column-colour-not-view-colour.md index 79669004..e3cc13b2 100644 --- a/.changeset/column-colour-not-view-colour.md +++ b/.changeset/column-colour-not-view-colour.md @@ -26,6 +26,15 @@ as a data difference rather than as a bug: - The continuous 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, spaced evenly across the domain, +because the ramps people actually use are not two-stop — viridis, a diverging +red/white/blue, or whatever a host has already chosen for the same column in its +own UI. Approximating one by its endpoints loses the midpoint that made it +meaningful. `numericScale: 'symlog'` goes with it: a counts or expression column +whose mass sits near zero with a long tail collapses into the first stop under a +linear position. Symmetric log rather than plain log, because these columns reach +zero and below. + `featureColorSchemeSignature` now takes the scheme as one object (`featureColorSchemeSignature(config.fillColorByColumn)`) rather than three positional arguments, so adding a term to the encoding cannot leave a call site diff --git a/packages/layers/src/featureColorEncoding.ts b/packages/layers/src/featureColorEncoding.ts index a198d7ba..e4f379bf 100644 --- a/packages/layers/src/featureColorEncoding.ts +++ b/packages/layers/src/featureColorEncoding.ts @@ -124,7 +124,32 @@ export type FeatureCategoricalPaletteSpec = | readonly FeatureRgbColor[] | FeatureNamedCategoricalPalette; -export type FeatureNumericRampSpec = readonly [FeatureRgbColor, FeatureRgbColor]; +/** + * The colours a continuous column ramps through, low to high. + * + * Two or more stops, spaced evenly across the domain and interpolated in RGB. Two + * is the common case and the default; more exists because the ramps people + * actually want are not two-stop — viridis, a diverging red/white/blue, and any + * palette a host has already chosen for the same column elsewhere in its own UI + * all need more, and approximating them with their endpoints does not just look + * different, it loses the midpoint that made them meaningful. + */ +export type FeatureNumericRampSpec = readonly [ + FeatureRgbColor, + FeatureRgbColor, + ...FeatureRgbColor[], +]; + +/** + * How a value's position along the ramp is measured. + * + * - `'linear'` (default) — position is proportional to the value. + * - `'symlog'` — proportional to `sign(v)·log(1+|v|)`, so a column whose mass sits + * near zero with a long tail (counts, expression) spreads out instead of + * collapsing into the ramp's first stop. Symmetric log rather than plain log + * because it is defined at and below zero, which real columns reach. + */ +export type FeatureNumericScale = 'linear' | 'symlog'; /** * The values the ramp's endpoints stand for, `[low, high]`. @@ -186,6 +211,7 @@ export interface FeatureColorScheme { categoricalPalette?: FeatureCategoricalPaletteSpec; numericRamp?: FeatureNumericRampSpec; numericDomain?: FeatureNumericDomain; + numericScale?: FeatureNumericScale; missingValues?: FeatureMissingValueOptions; } @@ -203,12 +229,14 @@ export function featureColorSchemeSignature({ categoricalPalette, numericRamp, numericDomain, + numericScale, missingValues, }: FeatureColorScheme = {}): string { if ( categoricalPalette === undefined && numericRamp === undefined && numericDomain === undefined && + numericScale === undefined && missingValues === undefined ) { return ''; @@ -217,6 +245,7 @@ export function featureColorSchemeSignature({ serializeCategoricalPalette(categoricalPalette), numericRamp ?? null, numericDomain ?? null, + numericScale ?? null, missingValues ? [missingValues.treatAsMissing ?? null, missingValues.render ?? null] : null, ]); } @@ -283,6 +312,39 @@ function interpolateRgb( ]; } +/** + * Sample a multi-stop ramp at `t ∈ [0, 1]`, stops spaced evenly. + * + * `t` at exactly 1 has to land on the last stop rather than reading past it, which + * is what the `length - 2` clamp is for — the top of the domain is the value most + * likely to be looked at, and reading past the end would silently return the last + * segment interpolated at 1 anyway on some inputs and `undefined` on others. + */ +function sampleRamp(stops: FeatureNumericRampSpec, t: number): FeatureRgbColor { + const clamped = Math.max(0, Math.min(1, t)); + const scaled = clamped * (stops.length - 1); + const lowIndex = Math.min(Math.floor(scaled), stops.length - 2); + return interpolateRgb(stops[lowIndex], stops[lowIndex + 1], scaled - lowIndex); +} + +/** Symmetric log, defined at and below zero. Matches d3's `scaleSymlog` at C = 1. */ +function symlog(value: number): number { + return Math.sign(value) * Math.log1p(Math.abs(value)); +} + +/** Where a value sits in `[min, max]`, as `t ∈ [0, 1]` before clamping. */ +function rampPosition(value: number, min: number, max: number, scale: FeatureNumericScale): number { + if (scale === 'symlog') { + const low = symlog(min); + const high = symlog(max); + // A degenerate domain has no position to report; the midpoint is the one + // answer that does not imply the value is at an extreme of a range it is not + // actually spread over. + return high === low ? 0.5 : (symlog(value) - low) / (high - low); + } + return max === min ? 0.5 : (value - min) / (max - min); +} + function getFiniteExtent(values: Array): [number, number] | undefined { let min = Number.POSITIVE_INFINITY; let max = Number.NEGATIVE_INFINITY; @@ -393,6 +455,8 @@ export interface AssignFeatureColorsOptions { numericRamp?: FeatureNumericRampSpec; /** Pin the ramp's endpoints instead of measuring them. See {@link FeatureNumericDomain}. */ numericDomain?: FeatureNumericDomain; + /** How position along the ramp is measured. See {@link FeatureNumericScale}. */ + numericScale?: FeatureNumericScale; /** * What the store declares this column to be. Supply it whenever you have it — * `'auto'` trusts it in preference to sniffing the values. See @@ -449,6 +513,7 @@ export function assignFeatureColors({ categoricalPalette, numericRamp = DEFAULT_FEATURE_NUMERIC_RAMP, numericDomain, + numericScale = 'linear', columnKind, missingValues, }: AssignFeatureColorsOptions): Array { @@ -490,7 +555,6 @@ export function assignFeatureColors({ return colors; } const [min, max] = extent; - const range = max - min; for (let index = 0; index < values.length; index += 1) { const value = numericValues[index]; if (value === undefined) { @@ -499,8 +563,10 @@ export function assignFeatureColors({ applyMissing(index); continue; } - const t = range === 0 ? 0.5 : (value - min) / range; - colors[index] = rgba(interpolateRgb(numericRamp[0], numericRamp[1], t), alpha); + colors[index] = rgba( + sampleRamp(numericRamp, rampPosition(value, min, max, numericScale)), + alpha + ); } return colors; } diff --git a/packages/layers/src/index.ts b/packages/layers/src/index.ts index d7f61b56..5ab97c67 100644 --- a/packages/layers/src/index.ts +++ b/packages/layers/src/index.ts @@ -49,6 +49,7 @@ export type { FeatureNamedCategoricalPalette, FeatureNumericDomain, FeatureNumericRampSpec, + FeatureNumericScale, FeatureRgbaColor, FeatureRgbColor, } from './featureColorEncoding'; diff --git a/packages/layers/src/labelColorEncoding.ts b/packages/layers/src/labelColorEncoding.ts index 8f51563a..dda6da84 100644 --- a/packages/layers/src/labelColorEncoding.ts +++ b/packages/layers/src/labelColorEncoding.ts @@ -41,6 +41,7 @@ import { type FeatureMissingValueOptions, type FeatureNumericDomain, type FeatureNumericRampSpec, + type FeatureNumericScale, type FeatureRgbaColor, type FeatureRgbColor, normalizeFeatureCellValue, @@ -216,6 +217,8 @@ export interface BuildLabelFillColorByFeatureIdOptions { numericRamp?: FeatureNumericRampSpec; /** Pin the ramp's endpoints instead of measuring them across the loaded features. */ numericDomain?: FeatureNumericDomain; + /** How position along the ramp is measured; `'linear'` by default. */ + numericScale?: FeatureNumericScale; /** What the store declares the column to be; `'auto'` trusts it over the values. */ columnKind?: FeatureColumnKind; missingValues?: FeatureMissingValueOptions; @@ -236,6 +239,7 @@ export function buildLabelFillColorByFeatureId({ categoricalPalette, numericRamp, numericDomain, + numericScale, columnKind, missingValues, }: BuildLabelFillColorByFeatureIdOptions): Record { @@ -255,6 +259,7 @@ export function buildLabelFillColorByFeatureId({ ...(categoricalPalette ? { categoricalPalette } : {}), ...(numericRamp ? { numericRamp } : {}), ...(numericDomain ? { numericDomain } : {}), + ...(numericScale ? { numericScale } : {}), ...(columnKind ? { columnKind } : {}), ...(missingValues ? { missingValues } : {}), }); diff --git a/packages/layers/src/shapeColorEncoding.ts b/packages/layers/src/shapeColorEncoding.ts index 258c223f..bb27482f 100644 --- a/packages/layers/src/shapeColorEncoding.ts +++ b/packages/layers/src/shapeColorEncoding.ts @@ -16,6 +16,7 @@ import { type FeatureMissingValueOptions, type FeatureNumericDomain, type FeatureNumericRampSpec, + type FeatureNumericScale, type FeatureRgbaColor, type FeatureRgbColor, normalizeFeatureCellValue, @@ -38,6 +39,8 @@ export interface BuildShapeFillColorByFeatureIdOptions { numericRamp?: FeatureNumericRampSpec; /** Pin the ramp's endpoints instead of measuring them across the loaded features. */ numericDomain?: FeatureNumericDomain; + /** How position along the ramp is measured; `'linear'` by default. */ + numericScale?: FeatureNumericScale; /** What the store declares the column to be; `'auto'` trusts it over the values. */ columnKind?: FeatureColumnKind; missingValues?: FeatureMissingValueOptions; @@ -58,6 +61,7 @@ export function buildShapeFillColorByFeatureId({ categoricalPalette, numericRamp, numericDomain, + numericScale, columnKind, missingValues, }: BuildShapeFillColorByFeatureIdOptions): Record { @@ -75,6 +79,7 @@ export function buildShapeFillColorByFeatureId({ ...(categoricalPalette ? { categoricalPalette } : {}), ...(numericRamp ? { numericRamp } : {}), ...(numericDomain ? { numericDomain } : {}), + ...(numericScale ? { numericScale } : {}), ...(columnKind ? { columnKind } : {}), ...(missingValues ? { missingValues } : {}), }); diff --git a/packages/layers/tests/featureColorEncoding.spec.ts b/packages/layers/tests/featureColorEncoding.spec.ts index 2fcccbb3..107428d6 100644 --- a/packages/layers/tests/featureColorEncoding.spec.ts +++ b/packages/layers/tests/featureColorEncoding.spec.ts @@ -355,3 +355,84 @@ describe('featureColorSchemeSignature', () => { ); }); }); + +describe('a ramp with more than two stops', () => { + const diverging: [number, number, number][] = [ + [0, 0, 255], + [255, 255, 255], + [255, 0, 0], + ]; + + it('passes through the middle stop at the middle of the domain', () => { + // The whole reason to allow more than two: a diverging ramp's midpoint is the + // meaning. Interpolating its endpoints alone would put grey-purple here. + const colors = assignFeatureColors({ + values: ['0', '5', '10'], + mode: 'continuous', + alpha: 255, + numericRamp: diverging, + numericDomain: [0, 10], + }); + + expect(colors).toEqual([ + [0, 0, 255, 255], + [255, 255, 255, 255], + [255, 0, 0, 255], + ]); + }); + + it('lands on the last stop at the top of the domain, not past it', () => { + const colors = assignFeatureColors({ + values: ['10'], + mode: 'continuous', + alpha: 255, + numericRamp: diverging, + numericDomain: [0, 10], + }); + + expect(colors[0]).toEqual([255, 0, 0, 255]); + }); + + it('spreads a long tail with a symlog scale', () => { + const values = ['0', '1', '10', '1000']; + const opts = { + mode: 'continuous' as const, + alpha: 255, + numericRamp: diverging, + numericDomain: [0, 1000] as const, + }; + + const linear = assignFeatureColors({ values, ...opts }); + const log = assignFeatureColors({ values, ...opts, numericScale: 'symlog' as const }); + + /** Largest per-channel difference — how far apart two colours actually look. */ + const apart = (a?: number[], b?: number[]) => + Math.max(...[0, 1, 2].map((i) => Math.abs((a?.[i] ?? 0) - (b?.[i] ?? 0)))); + + // Linear: 1 and 10 both sit within 1% of the bottom of the domain, so the + // whole low end of the column collapses into one indistinguishable colour. + expect(apart(linear[1], linear[0])).toBeLessThan(10); + expect(apart(linear[2], linear[0])).toBeLessThan(10); + // Symlog pulls them apart into colours a reader can actually tell apart. + expect(apart(log[1], log[0])).toBeGreaterThan(40); + expect(apart(log[2], log[1])).toBeGreaterThan(40); + // The endpoints still pin to the ends of the ramp. + expect(log[0]).toEqual([0, 0, 255, 255]); + expect(log[3]).toEqual([255, 0, 0, 255]); + }); + + it('handles a domain that crosses zero, where a plain log could not', () => { + const colors = assignFeatureColors({ + values: ['-100', '0', '100'], + mode: 'continuous', + alpha: 255, + numericRamp: diverging, + numericDomain: [-100, 100], + numericScale: 'symlog', + }); + + expect(colors[0]).toEqual([0, 0, 255, 255]); + expect(colors[1]).toEqual([255, 255, 255, 255]); + expect(colors[2]).toEqual([255, 0, 0, 255]); + }); +}); diff --git a/packages/vis/src/SpatialCanvas/labelsProjection.ts b/packages/vis/src/SpatialCanvas/labelsProjection.ts index 322368d9..73562c04 100644 --- a/packages/vis/src/SpatialCanvas/labelsProjection.ts +++ b/packages/vis/src/SpatialCanvas/labelsProjection.ts @@ -136,6 +136,7 @@ export function buildLabelFillColorEntry( ...(fillColorByColumn.numericDomain ? { numericDomain: fillColorByColumn.numericDomain } : {}), + ...(fillColorByColumn.numericScale ? { numericScale: fillColorByColumn.numericScale } : {}), }), rowsSource: rows, }; diff --git a/packages/vis/src/SpatialCanvas/types.ts b/packages/vis/src/SpatialCanvas/types.ts index b4edfd9d..4fb5a88c 100644 --- a/packages/vis/src/SpatialCanvas/types.ts +++ b/packages/vis/src/SpatialCanvas/types.ts @@ -9,6 +9,7 @@ import type { FeatureMissingValueOptions, FeatureNumericDomain, FeatureNumericRampSpec, + FeatureNumericScale, LabelFillColorMode, ShapeFillColorMode, ShapeStrokeWidthUnits, @@ -58,6 +59,11 @@ export interface FillColorByColumn { * over different parts of one annotation stay comparable. */ numericDomain?: FeatureNumericDomain; + /** + * How a value's position along the ramp is measured — `'linear'` by default, or + * `'symlog'` for a column whose mass sits near zero with a long tail. + */ + numericScale?: FeatureNumericScale; /** * What counts as missing in this column, and how a feature with no value should * render — keep the layer default, hide it, or take an explicit colour. diff --git a/packages/vis/src/SpatialCanvas/useLayerData.ts b/packages/vis/src/SpatialCanvas/useLayerData.ts index adfee981..1aa3564f 100644 --- a/packages/vis/src/SpatialCanvas/useLayerData.ts +++ b/packages/vis/src/SpatialCanvas/useLayerData.ts @@ -817,6 +817,9 @@ export function useLayerData( ...(fillColorByColumn.numericDomain ? { numericDomain: fillColorByColumn.numericDomain } : {}), + ...(fillColorByColumn.numericScale + ? { numericScale: fillColorByColumn.numericScale } + : {}), }), rowsSource: rows, renderSource: renderData, From c2d9f84db640e21e3bf47e984d80f6cc95da3b9c Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Tue, 11 Aug 2026 16:16:31 +0100 Subject: [PATCH 5/8] Ship sourcemaps, and stop a malformed scheme crashing three frames away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ...lish-sourcemaps-and-survive-bad-schemes.md | 21 +++++++ packages/layers/src/featureColorEncoding.ts | 26 ++++++-- .../layers/tests/featureColorEncoding.spec.ts | 59 +++++++++++++++++++ packages/layers/vite.config.ts | 3 + vite.config.base.ts | 4 ++ 5 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 .changeset/publish-sourcemaps-and-survive-bad-schemes.md diff --git a/.changeset/publish-sourcemaps-and-survive-bad-schemes.md b/.changeset/publish-sourcemaps-and-survive-bad-schemes.md new file mode 100644 index 00000000..f498207f --- /dev/null +++ b/.changeset/publish-sourcemaps-and-survive-bad-schemes.md @@ -0,0 +1,21 @@ +--- +'@spatialdata/layers': patch +'@spatialdata/vis': patch +'@spatialdata/avivatorish': patch +'@spatialdata/react': patch +--- + +Publish sourcemaps, and survive a colour scheme that does not match its own type. + +`core` shipped `index.js.map`; `layers`, `vis`, `avivatorish` and `react` did not. +A crash inside one of them reached a consumer as +`Le (…/.vite/deps/@spatialdata_layers.js:396)` — an esbuild-minified name with +nothing to map it back to. An embedding application has only the built artifact to +debug against, so it has to carry a map. + +`resolveCategoricalPalette` and the ramp sampler now always return a colour. A +scheme arrives from a saved Render Stack, so its type is a claim about JSON rather +than a guarantee: a palette object with no `byValue`, a list with a hole in it, or +a ramp with fewer than two stops all used to return `undefined` and fail several +frames later in the arithmetic that reads `rgb[0]`. Wrong colours can be seen and +reported; that `TypeError` cannot. diff --git a/packages/layers/src/featureColorEncoding.ts b/packages/layers/src/featureColorEncoding.ts index e4f379bf..113e39da 100644 --- a/packages/layers/src/featureColorEncoding.ts +++ b/packages/layers/src/featureColorEncoding.ts @@ -187,23 +187,35 @@ function isNamedCategoricalPalette( * * Both arguments are passed because the spec decides which one is authoritative: a * named palette answers from the value, the positional forms from the index. + * + * **Always returns a colour.** A spec arrives from a saved Render Stack — JSON, so + * the type is a claim, not a guarantee — and returning `undefined` for one it does + * not recognise puts the failure several frames away, in the arithmetic that reads + * `rgb[0]`. That is the shape of crash you cannot diagnose from a stack trace. An + * unrecognised spec falls back to the default scheme instead: wrong colours are + * visible and reportable, a `TypeError` deep in a bundled dependency is not. */ export function resolveCategoricalPalette( spec: FeatureCategoricalPaletteSpec = DEFAULT_FEATURE_CATEGORICAL_PALETTE ): (value: string, categoryIndex: number) => FeatureRgbColor { + const byIndex = (_value: string, categoryIndex: number) => featureCodeToRgb(categoryIndex); if (spec === 'oklab') { - return (_value, categoryIndex) => featureCodeToRgb(categoryIndex); + return byIndex; } if (isNamedCategoricalPalette(spec)) { const { byValue, fallback = 'oklab' } = spec; + if (!byValue || typeof byValue !== 'object') { + return byIndex; + } const colorForUnnamed = fallback === 'oklab' ? featureCodeToRgb : (_index: number) => fallback; return (value, categoryIndex) => byValue[value] ?? colorForUnnamed(categoryIndex); } const colors = spec; - if (colors.length === 0) { - return (_value, categoryIndex) => featureCodeToRgb(categoryIndex); + if (!Array.isArray(colors) || colors.length === 0) { + return byIndex; } - return (_value, categoryIndex) => colors[categoryIndex % colors.length]; + return (_value, categoryIndex) => + colors[categoryIndex % colors.length] ?? byIndex('', categoryIndex); } /** Everything about a column's encoding that is not the column itself. */ @@ -321,6 +333,12 @@ function interpolateRgb( * segment interpolated at 1 anyway on some inputs and `undefined` on others. */ function sampleRamp(stops: FeatureNumericRampSpec, t: number): FeatureRgbColor { + // The two-stop minimum is a type-level claim about JSON that came out of a saved + // Render Stack, so it is checked. With one stop the arithmetic below indexes -1 + // and the failure surfaces as `low[0]` of undefined, several frames from here. + if (!Array.isArray(stops) || stops.length < 2) { + return stops?.[0] ?? DEFAULT_FEATURE_NUMERIC_RAMP[0]; + } const clamped = Math.max(0, Math.min(1, t)); const scaled = clamped * (stops.length - 1); const lowIndex = Math.min(Math.floor(scaled), stops.length - 2); diff --git a/packages/layers/tests/featureColorEncoding.spec.ts b/packages/layers/tests/featureColorEncoding.spec.ts index 107428d6..c5298d3a 100644 --- a/packages/layers/tests/featureColorEncoding.spec.ts +++ b/packages/layers/tests/featureColorEncoding.spec.ts @@ -436,3 +436,62 @@ describe('a ramp with more than two stops', () => { expect(colors[2]).toEqual([255, 0, 0, 255]); }); }); + +/** + * A scheme arrives from a saved Render Stack, so its type is a claim about JSON + * rather than a guarantee. Every case here used to reach the colour arithmetic and + * fail there — `Cannot read properties of undefined (reading '0')`, several frames + * from the malformed field, inside a bundled dependency. Wrong colours can be + * reported by whoever sees them; that TypeError cannot. + */ +describe('a scheme that does not match its own type', () => { + const malformed = (categoricalPalette: unknown) => + assignFeatureColors({ + values: ['tumour', 'stroma'], + mode: 'categorical', + alpha: 255, + categoricalPalette: categoricalPalette as never, + }); + + it('falls back to the default scheme for a palette object with no byValue', () => { + const colors = malformed({ fallback: 'oklab' }); + + expect(colors[0]).toBeDefined(); + expect(colors[1]).toBeDefined(); + expect(colors[0]).not.toEqual(colors[1]); + }); + + it('survives a list with a hole in it', () => { + const colors = malformed([[1, 2, 3], undefined]); + + expect(colors[0]).toBeDefined(); + expect(colors[1]).toBeDefined(); + }); + + it('survives a ramp with fewer stops than its type allows', () => { + const one = assignFeatureColors({ + values: ['0', '5', '10'], + mode: 'continuous', + alpha: 255, + numericRamp: [[7, 8, 9]] as never, + }); + + expect(one).toEqual([ + [7, 8, 9, 255], + [7, 8, 9, 255], + [7, 8, 9, 255], + ]); + }); + + it('survives an empty ramp', () => { + const none = assignFeatureColors({ + values: ['0', '10'], + mode: 'continuous', + alpha: 255, + numericRamp: [] as never, + }); + + expect(none[0]).toBeDefined(); + expect(none[1]).toBeDefined(); + }); +}); diff --git a/packages/layers/vite.config.ts b/packages/layers/vite.config.ts index bfcc07ac..6a31a03f 100644 --- a/packages/layers/vite.config.ts +++ b/packages/layers/vite.config.ts @@ -12,6 +12,9 @@ export default defineConfig({ }, build: { outDir: resolve(__dirname, 'dist'), + // See the note in `vite.config.base.ts`: a consumer debugging a crash in here + // has only the built artifact, so it has to carry a map back to source. + sourcemap: true, lib: { entry: resolve(__dirname, 'src/index.ts'), name: 'SpatialDataLayers', diff --git a/vite.config.base.ts b/vite.config.base.ts index de0f2aee..d6cc6bff 100644 --- a/vite.config.base.ts +++ b/vite.config.base.ts @@ -67,6 +67,10 @@ export function defineViteConfig(options: DefineConfigOptions) { ], build: { outDir: path.resolve(pkgRoot, 'dist'), + // Published so a consumer's stack trace names our source, not `Le` at + // `.vite/deps/@spatialdata_layers.js:396`. An embedding application debugs + // against the built artifact — it is the only form of this code it has. + sourcemap: true, lib: { entry: path.resolve(pkgRoot, 'src/index.ts'), name: libName, From 20abefccf68ac40867464b568907304535e3cf09 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Wed, 12 Aug 2026 09:22:23 +0100 Subject: [PATCH 6/8] Reject a null categorical palette from the named-palette guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- packages/layers/src/featureColorEncoding.ts | 10 +++++++++- .../layers/tests/featureColorEncoding.spec.ts | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/packages/layers/src/featureColorEncoding.ts b/packages/layers/src/featureColorEncoding.ts index 113e39da..e2328040 100644 --- a/packages/layers/src/featureColorEncoding.ts +++ b/packages/layers/src/featureColorEncoding.ts @@ -170,10 +170,18 @@ export const DEFAULT_FEATURE_NUMERIC_RAMP: FeatureNumericRampSpec = [ [255, 220, 0], ]; +/** + * `null` is rejected explicitly, not incidentally: `typeof null === 'object'` and + * `Array.isArray(null)` is false, so without this it passes as a named palette and + * the destructure in {@link resolveCategoricalPalette} throws on the spot — + * defeating that function's whole always-returns-a-colour guarantee, and taking + * {@link featureColorSchemeSignature} with it. `{"categoricalPalette": null}` is a + * thing JSON says, so a saved Render Stack can say it. + */ function isNamedCategoricalPalette( spec: FeatureCategoricalPaletteSpec ): spec is FeatureNamedCategoricalPalette { - return typeof spec === 'object' && !Array.isArray(spec); + return spec !== null && typeof spec === 'object' && !Array.isArray(spec); } /** diff --git a/packages/layers/tests/featureColorEncoding.spec.ts b/packages/layers/tests/featureColorEncoding.spec.ts index c5298d3a..6a31b030 100644 --- a/packages/layers/tests/featureColorEncoding.spec.ts +++ b/packages/layers/tests/featureColorEncoding.spec.ts @@ -461,6 +461,24 @@ describe('a scheme that does not match its own type', () => { expect(colors[0]).not.toEqual(colors[1]); }); + it('falls back to the default scheme for a null palette', () => { + // The sharp edge: `typeof null === 'object'` and `Array.isArray(null)` is + // false, so `null` reads as a named palette to any guard that does not say + // otherwise — and then the destructure throws before any colour is assigned. + // `{"categoricalPalette": null}` is a thing JSON says. + const colors = malformed(null); + + expect(colors[0]).toBeDefined(); + expect(colors[1]).toBeDefined(); + expect(colors[0]).not.toEqual(colors[1]); + }); + + it('takes a signature for a null palette instead of throwing on one', () => { + // Same root cause, different entry point: the signature helper narrows with + // the same guard, so a null palette used to take the cache key down with it. + expect(() => featureColorSchemeSignature({ categoricalPalette: null as never })).not.toThrow(); + }); + it('survives a list with a hole in it', () => { const colors = malformed([[1, 2, 3], undefined]); From 8cd6779931b7b8061afd1aa459d76b5e15d7058a Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Wed, 12 Aug 2026 09:26:34 +0100 Subject: [PATCH 7/8] Correct two comments the colour and store changes left behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- packages/vis/src/SpatialCanvas/types.ts | 9 +++++++-- packages/vis/src/SpatialCanvas/useLayerData.ts | 12 ++++++------ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/vis/src/SpatialCanvas/types.ts b/packages/vis/src/SpatialCanvas/types.ts index 4fb5a88c..19e0b229 100644 --- a/packages/vis/src/SpatialCanvas/types.ts +++ b/packages/vis/src/SpatialCanvas/types.ts @@ -51,10 +51,15 @@ export interface FillColorByColumn { * depends on the categories present. */ categoricalPalette?: FeatureCategoricalPaletteSpec; - /** Endpoints of the continuous ramp, `[low, high]` as RGB 0–255. */ + /** + * The colours a continuous column ramps through, low to high: two or more RGB + * 0–255 stops, spaced evenly across the domain. Two is the default; pass more to + * carry a real ramp — viridis, a diverging red/white/blue — whose midpoint is + * part of what it means. + */ numericRamp?: FeatureNumericRampSpec; /** - * The values those endpoints stand for. Defaults to the extent of the features + * The values the ramp's ends stand for. Defaults to the extent of the features * that loaded — pin it to the column's own range when you know it, so two layers * over different parts of one annotation stay comparable. */ diff --git a/packages/vis/src/SpatialCanvas/useLayerData.ts b/packages/vis/src/SpatialCanvas/useLayerData.ts index 1aa3564f..ef94b5a8 100644 --- a/packages/vis/src/SpatialCanvas/useLayerData.ts +++ b/packages/vis/src/SpatialCanvas/useLayerData.ts @@ -598,12 +598,12 @@ export function useLayerData( // Compiler (`'use no memo'`) — the compiler otherwise memoizes JSX built from these // resolver getters and never repaints on a late async settle. // - // Caveat: `SpatialEntryStore` subscribes to its resolvers in its constructor, which - // runs inside the `useMemo` above. Under React StrictMode's dev-only double-invoke a - // discarded store instance leaks one listener on the (never-disposed) points - // resolver per rebuild; each such listener only calls a dead store's `notify()` - // (empty listener set), so it is inert. Harmless in production; noted so it is not - // mistaken for a real leak. + // The cleanup below is why the store's resolver bridge is tied to having listeners + // rather than to construction. An effect cleanup is not "the end": StrictMode's + // dev double-mount runs it and then re-runs the effect against the SAME memoised + // store, so `dispose()` here has to be recoverable — `subscribe` re-attaches. It + // also means a store the `useMemo` above builds and discards never subscribed to + // anything, so it holds nothing to leak. See `SpatialEntryStore.attachResolvers`. useEffect(() => { const unsubscribe = store.subscribe(notifyLoadedDataChanged); return () => { From 967513230be84b07c16d5f80b19630eca05695d3 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Wed, 12 Aug 2026 09:32:42 +0100 Subject: [PATCH 8/8] Say what "falls back to the default scheme" actually means MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../layers/tests/featureColorEncoding.spec.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/packages/layers/tests/featureColorEncoding.spec.ts b/packages/layers/tests/featureColorEncoding.spec.ts index 6a31b030..9568aaf1 100644 --- a/packages/layers/tests/featureColorEncoding.spec.ts +++ b/packages/layers/tests/featureColorEncoding.spec.ts @@ -445,19 +445,27 @@ describe('a ramp with more than two stops', () => { * reported by whoever sees them; that TypeError cannot. */ describe('a scheme that does not match its own type', () => { + const values = ['tumour', 'stroma']; + const malformed = (categoricalPalette: unknown) => assignFeatureColors({ - values: ['tumour', 'stroma'], + values, mode: 'categorical', alpha: 255, categoricalPalette: categoricalPalette as never, }); + /** + * What the same column looks like with no palette at all. "Falls back" means + * these exact colours: a scheme the caller cannot read should be indistinguishable + * from one they never wrote, not merely something that avoided throwing. + */ + const defaultScheme = assignFeatureColors({ values, mode: 'categorical', alpha: 255 }); + it('falls back to the default scheme for a palette object with no byValue', () => { const colors = malformed({ fallback: 'oklab' }); - expect(colors[0]).toBeDefined(); - expect(colors[1]).toBeDefined(); + expect(colors).toEqual(defaultScheme); expect(colors[0]).not.toEqual(colors[1]); }); @@ -468,8 +476,7 @@ describe('a scheme that does not match its own type', () => { // `{"categoricalPalette": null}` is a thing JSON says. const colors = malformed(null); - expect(colors[0]).toBeDefined(); - expect(colors[1]).toBeDefined(); + expect(colors).toEqual(defaultScheme); expect(colors[0]).not.toEqual(colors[1]); });