Stop bundling luma.gl into @spatialdata/layers; cover labels colour-by in CI

[xinaesthete]

Started as a hunt for a labels colour-by regression that turned out not to exist — colour-by works on the deployed docs demo, both categorical and numeric, and survives hovering. What the hunt did find is a packaging defect in the published @spatialdata/layers artifact.

The bug

packages/layers externalized the six specifiers its source imports and nothing else, so @luma.gl/core, /engine and /shadertools (plus @probe.gl/*) were pulled in transitively by the layers that build their own Model and shipped inside dist/index.js. 238 kB down to 92 kB now that they are not.

Size was the least of it. deck.gl, Viv and this package must share ONE luma runtime. A consumer that also loaded deck.gl got two ShaderAssembler classes, and ShaderAssembler.getDefaultShaderAssembler() is a static — so "the default shader assembler" meant different objects to deck and to Viv.

That matters because Viv's VivShaderAssembler builds itself by copying that default's modules and hook functions once, at construction, and never re-syncs. It could therefore copy from an assembler deck had never registered anything on, and lose deck's DECKGL_FILTER_* hooks entirely. Every Viv-derived layer — labels included — then failed to compile its vertex shader, reporting only:

luma.gl: Module layer not found
luma.gl: WebGL Link error during link-error: Vertex shader is not compiled.

Nothing surfaces through deck's onError. The externals are now whole families by regex, matching what @spatialdata/vis has done all along.

The consumer half of the same requirement: the browser harness aliased @deck.gl/core to dist/index.cjs, which split @luma.gl/shadertools a second way — CJS through deck, ESM through @vivjs/extensions. It now resolves the ESM entry. Both halves are load-bearing; restoring either alone leaves labels unable to draw, which I verified.

The test that found it

labels-color-by is a new scenario in the existing production-browser job. It hands LabelsLayer a synthetic two-label raster and a featureState, at full fill opacity and zero stroke width so the drawn pixel is the feature colour, then samples the drawing buffer inside onAfterRender — the only point it is readable without preserveDrawingBuffer. It asserts the two bands come back as the LUT's colours, and fails explicitly on the case where both come back in the channel colour, which is what "colour-by does nothing" looks like.

Verified red by forcing useFeatureColors = 0 in LabelsBitmaskTileLayer.draw:

label 1 drew in the channel colour — feature colouring did not reach the shader

Synthetic rather than fixture-backed on purpose: what is under test is featureState → LUT → texture → shader, and a real store puts loading, tiling and transforms in front of all of it.

The harness now serves several scenarios from one bundle via ?scenario=, rather than one HTML entry each — the build keeps code splitting off to dodge a Rolldown panic in apache-arrow, and multiple entries into a single chunk is what turns it back on.

Not covered

The load path and the associated-table column projection. That wants an obs column added in python/v0.7.2, since the blobs table carries only instance_id and region — neither eligible — so labels colour-by isn't reachable through the UI on that fixture at all.

Follow-up

The duplicated luma runtime was our bug and is fixed here, but VivShaderAssembler snapshotting mutable global state once is what turns any such duplication into a silent, total render failure — and it has two further failure modes that need no duplication at all (deck clears and re-adds _hookFunctions on every LayerManager construction; the GLSL/WGSL hook choice is baked into the snapshot). I have an upstream issue drafted for hms-dbmi/viv, not yet filed.

Verification

• pnpm test:browser:production — both specs pass
• pnpm test:unit — 795 passing
• pnpm build clean; lint:biome shows only the pre-existing noExplicitAny warnings

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Improved rendering compatibility by ensuring a single shared graphics runtime is used across related visualization packages.
  • Fixed label color rendering scenarios and added validation for expected colors and runtime errors.

• Tests

  • Added browser coverage for label color rendering and polygon shape visualization.
  • Added checks for WebGL readiness, rendered frames, loading states, errors, and visual output.

• Chores

  • Reduced the published bundle size by externalizing shared visualization dependencies.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR externalizes dependency families from the layers bundle and aligns browser builds with a single luma.gl runtime. It adds selectable polygon and label rendering scenarios, plus a Playwright regression test for label color-by rendering.

Changes

Runtime externalization and browser validation

Layer / File(s)	Summary
Dependency-family externalization .changeset/layers-external-luma.md, packages/layers/vite.config.ts, tests/production/browser/vite.config.ts	The library build externalizes dependency families with regular expressions. The browser build resolves the ESM @deck.gl/core entry. The changeset documents the runtime changes.
Selectable production scenarios tests/production/browser/src.tsx, tests/production/browser/polygonShapesScenario.tsx	The browser entry selects polygon-shapes or labels-color-by from the query string. The polygon scenario fetches and validates NGFF metadata before rendering the fixture with Deck.gl.
Label color rendering regression tests/production/browser/labelsColorByContract.ts, tests/production/browser/labelsColorByScenario.tsx, tests/production/browser/labels-color-by.spec.ts	The label scenario renders a synthetic two-region raster, samples canvas pixels, and records render diagnostics. The Playwright test verifies the expected label colors and rendered frames without runtime errors.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• Taylor-CCB-Group/SpatialData.js#25: Both PRs change Vite dependency externalization with family-level patterns.
• Taylor-CCB-Group/SpatialData.js#92: Both PRs address production browser testing and Deck/luma runtime compatibility.
• Taylor-CCB-Group/SpatialData.js#95: The new label color regression test exercises the LabelsLayer feature-coloring behavior introduced there.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes both primary changes: externalizing luma.gl and adding CI coverage for labels color-by rendering.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/labels-colour-by-regression-cf2c26

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

tests/production/browser/polygonShapesScenario.tsx (1)

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

Validate the NGFF fixture metadata with a schema instead of an unchecked type annotation.

response.json() returns any. The parameter annotation on metadata imposes a shape without validating it at runtime. If the fixture's JSON structure changes, this silently type-checks while failing at a different point (or not failing at all if attributes is missing a different way than expected).

Use the same strict-schema approach already established for NGFF-shaped data in this codebase, rather than an ad hoc typed literal.

Based on coding guidelines, "Prefer a strict schema aligned with specs (e.g. NGFF) over parsing 'whatever might show up.'"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/production/browser/polygonShapesScenario.tsx` around lines 32 - 37,
Replace the unchecked metadata annotation in the response.json() handling within
the fixture readiness chain with the established strict NGFF schema parser used
elsewhere in the codebase. Parse and validate the JSON before checking
attributes['encoding-type'], while preserving the existing ngff:shapes
validation error and active/setFixtureReady behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/production/browser/labelsColorByScenario.tsx`:
- Around line 103-120: Update buildLayer’s channelColors construction to avoid
the inline tuple assertion, preferably by adding a small typed RGB-tuple helper
that destructures CHANNEL_COLOR and returns [r, g, b]. If retaining the
assertion instead, add a concise local comment explaining why spreading
CHANNEL_COLOR widens the tuple type.

In `@tests/production/browser/src.tsx`:
- Around line 19-21: Update isScenarioName to validate scenario membership using
an own-property check on scenarios, while preserving the null guard and type
predicate so inherited keys such as constructor are rejected before
scenarios[requested] is indexed.

---

Nitpick comments:
In `@tests/production/browser/polygonShapesScenario.tsx`:
- Around line 32-37: Replace the unchecked metadata annotation in the
response.json() handling within the fixture readiness chain with the established
strict NGFF schema parser used elsewhere in the codebase. Parse and validate the
JSON before checking attributes['encoding-type'], while preserving the existing
ngff:shapes validation error and active/setFixtureReady behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: cfdafb57-0649-4d4d-b8f0-8ad9e3295cf5

📥 Commits

Reviewing files that changed from the base of the PR and between 423448b and 7ed9e77.

📒 Files selected for processing (8)

• .changeset/layers-external-luma.md
• packages/layers/vite.config.ts
• tests/production/browser/labels-color-by.spec.ts
• tests/production/browser/labelsColorByContract.ts
• tests/production/browser/labelsColorByScenario.tsx
• tests/production/browser/polygonShapesScenario.tsx
• tests/production/browser/src.tsx
• tests/production/browser/vite.config.ts