Add deck.gl labels rendering to SpatialCanvas - #22
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (25)
📝 WalkthroughWalkthroughAdds multi-channel labels support across layers and vis: new GLSL shaders, a GPU-backed LabelsBitmaskTileLayer, a Composite LabelsLayer (single- and multiscale), vis renderer/hooks/UI integration, tooltip utilities, typing/workspace alias updates, and documentation noting Viv compatibility follow-ups. Changes
Sequence DiagramsequenceDiagram
participant Config as User Config
participant Hook as useLayerData
participant Loader as LabelsLoader
participant Layer as LabelsLayer
participant Tile as LabelsBitmaskTileLayer
participant GPU as Shader
Config->>Hook: request labels layer (loader, selections)
Hook->>Loader: createImageLoader(url / multiscale)
Loader-->>Hook: loader + channel metadata (colors, axis sizes, selections)
Hook->>Layer: instantiate LabelsLayer (props, modelMatrix)
Layer->>Tile: request tile / raster data (single or multiscale)
Tile->>Loader: getTile / getRaster for each channel/selection
Tile->>Tile: upload per-channel arrays -> r32float textures
Tile->>GPU: bind textures + per-channel uniforms
GPU->>GPU: sample channels, detect edges, composite fragments
GPU-->>Tile: rendered frame
Tile-->>Layer: frame complete
Layer-->>Hook: layer ready / load state update
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/layers/src/labelsBitmaskLayerShaders.ts (1)
67-86: Consider documenting the edge detection constants.Line 70 uses
150.0 * strokeWidth * scaleFactoras a multiplier for edge pixel offset. This magic number affects stroke appearance at different zoom levels. A brief comment explaining the derivation or intended behavior would help future maintainers tune this value.📝 Suggested comment
float getEdgeMask(sampler2D dataTex, vec2 coord, float sampledData, float strokeWidth) { vec2 coordDx = dFdx(coord); vec2 coordDy = dFdy(coord); + // Scale factor converts strokeWidth to screen-space pixels for edge detection. + // The base multiplier (150.0) was tuned for typical zoom ranges. float edgePixels = max(1.0, 150.0 * strokeWidth * labelsBitmask.scaleFactor);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/layers/src/labelsBitmaskLayerShaders.ts` around lines 67 - 86, Add a short inline comment above the magic multiplier in getEdgeMask explaining why 150.0 is used and how it interacts with strokeWidth and labelsBitmask.scaleFactor to compute edgePixels (i.e., it scales the sampling offset to maintain visible stroke thickness across zoom/scale levels), note expected units/range and any tradeoffs (performance vs. visual quality) and mention where to adjust if different stroke behavior is desired.packages/layers/src/LabelsLayer.ts (2)
27-27: Type assertion for Viv workaround.The
as anycast here is documented as a temporary Viv workaround (per lines 118-120). Consider adding a brief inline comment referencing the upstream issue or the documentation inspatial-canvas-status.mdxso this can be cleaned up when Viv fixes land.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/layers/src/LabelsLayer.ts` at line 27, Add a brief inline comment next to the existing type assertion `const UntypedTileLayer = TileLayer as any;` explaining that the `as any` cast is a temporary workaround for a Viv typing issue, and include a reference to the upstream Viv issue or to `spatial-canvas-status.mdx` so maintainers can remove the cast once Viv fixes land; keep the comment short and placed on the same line or immediately above the declaration for visibility.
158-162: Minor: Consider validating shape array length.Line 162 destructures
shape.slice(-2)assuming at least 2 elements. Ifshapeexists but is malformed (length < 2), this could produceundefinedvalues.🛡️ Optional defensive check
const base = getBaseLoader(loader); - if (!base?.shape) { + if (!base?.shape || base.shape.length < 2) { return null; } const [height, width] = base.shape.slice(-2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/layers/src/LabelsLayer.ts` around lines 158 - 162, The current destructuring of base.shape.slice(-2) in LabelsLayer (after calling getBaseLoader) assumes shape has at least two dimensions; add a defensive check that verifies base.shape.length >= 2 before slicing/destructuring (or early return null) to avoid undefined height/width when shape is malformed; reference the getBaseLoader call, the base variable and the base.shape usage and update the code path that currently does const [height, width] = base.shape.slice(-2) to validate length and handle the error/early-return.packages/vis/src/SpatialCanvas/useLayerData.ts (1)
660-666: Consider extracting7as a shared constant.The max label channels limit (7) is hardcoded here and in
LabelsBitmaskTileLayer.ts(MAX_LABEL_CHANNELS = 7). If these values diverge, the shader will receive mismatched channel counts.♻️ Suggested approach
Export
MAX_LABEL_CHANNELSfrom the layers package and import it here:+import { MAX_LABEL_CHANNELS } from '@spatialdata/layers'; ... const selections = clampVivSelectionsToAxes( buildDefaultSelection({ labels: loaderObj.labels, shape: loaderObj.shape, }), axisSizes - ).slice(0, 7); + ).slice(0, MAX_LABEL_CHANNELS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vis/src/SpatialCanvas/useLayerData.ts` around lines 660 - 666, The literal "7" used to slice selections in useLayerData.ts should be replaced with a shared constant to avoid divergence with LabelsBitmaskTileLayer.ts; export MAX_LABEL_CHANNELS from the layers package (or from LabelsBitmaskTileLayer.ts where it's currently defined) and import that constant into useLayerData.ts, then replace .slice(0, 7) with .slice(0, MAX_LABEL_CHANNELS) so clampVivSelectionsToAxes/buildDefaultSelection consume the same channel limit as the shader layer.packages/layers/src/LabelsBitmaskTileLayer.ts (3)
46-48: Empty constructor can be removed.This constructor only calls
super(...args)without additional logic. TypeScript/ES classes automatically invoke the parent constructor when no constructor is defined.♻️ Suggested removal
- constructor(...args: any[]) { - super(...args); - } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/layers/src/LabelsBitmaskTileLayer.ts` around lines 46 - 48, Remove the no-op constructor from the LabelsBitmaskTileLayer class: the constructor in LabelsBitmaskTileLayer that only calls super(...args) should be deleted so the class uses the inherited constructor automatically; edit the LabelsBitmaskTileLayer class to remove the constructor method and ensure no other code relies on an explicit constructor in that class.
33-35: Type workaround is acceptable given Viv constraints.The PR description notes local workarounds for Viv layer prop issues. Casting
XRLayer as anyavoids fighting upstream type definitions while preserving runtime behavior. Consider adding a brief comment explaining the rationale for future maintainers.📝 Suggested comment
+// XRLayer's prop types are incompatible with our extended props; cast to any +// until upstream Viv exposes a proper extension point. const UntypedXRLayer = XRLayer as any;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/layers/src/LabelsBitmaskTileLayer.ts` around lines 33 - 35, Add a short inline comment above the cast explaining why XRLayer is cast to any (i.e., known Viv typing mismatch and intentional runtime-safe workaround) so future maintainers understand rationale; place this comment next to the UntypedXRLayer = XRLayer as any line and mention that the cast preserves runtime behavior while avoiding upstream type issues used by LabelsBitmaskTileLayer.
1-5: Consider exportingMAX_LABEL_CHANNELSfor consumers.
useLayerData.tshardcodes7when slicing selections. Exporting this constant allows consumers to stay in sync without duplicating magic numbers.♻️ Suggested export
-const MAX_LABEL_CHANNELS = 7; +export const MAX_LABEL_CHANNELS = 7;Then re-export from
packages/layers/src/index.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/layers/src/LabelsBitmaskTileLayer.ts` around lines 1 - 5, Export the MAX_LABEL_CHANNELS constant from LabelsBitmaskTileLayer (add export to the existing MAX_LABEL_CHANNELS) and re-export it from the package entry (packages/layers/src/index.ts), then update useLayerData.ts to import and use that exported MAX_LABEL_CHANNELS instead of hardcoding the literal 7 so consumers stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/layers/src/LabelsBitmaskTileLayer.ts`:
- Around line 58-73: The dataToTexture method casts unknown to ArrayLike<number>
and constructs a Float32Array without validation, which can silently fail or
corrupt textures; update dataToTexture to validate that the incoming data is an
ArrayLike<number> (or a TypedArray/number[]), that its length equals
width*height (or width*height*channels if applicable), and throw or return a
clear error if invalid; when valid, create a Float32Array from the input
(preserving TypedArray inputs) and then call this.context.device.createTexture
with that array (referencing dataToTexture, Float32Array, ArrayLike<number>, and
this.context.device.createTexture to locate the change).
---
Nitpick comments:
In `@packages/layers/src/labelsBitmaskLayerShaders.ts`:
- Around line 67-86: Add a short inline comment above the magic multiplier in
getEdgeMask explaining why 150.0 is used and how it interacts with strokeWidth
and labelsBitmask.scaleFactor to compute edgePixels (i.e., it scales the
sampling offset to maintain visible stroke thickness across zoom/scale levels),
note expected units/range and any tradeoffs (performance vs. visual quality) and
mention where to adjust if different stroke behavior is desired.
In `@packages/layers/src/LabelsBitmaskTileLayer.ts`:
- Around line 46-48: Remove the no-op constructor from the
LabelsBitmaskTileLayer class: the constructor in LabelsBitmaskTileLayer that
only calls super(...args) should be deleted so the class uses the inherited
constructor automatically; edit the LabelsBitmaskTileLayer class to remove the
constructor method and ensure no other code relies on an explicit constructor in
that class.
- Around line 33-35: Add a short inline comment above the cast explaining why
XRLayer is cast to any (i.e., known Viv typing mismatch and intentional
runtime-safe workaround) so future maintainers understand rationale; place this
comment next to the UntypedXRLayer = XRLayer as any line and mention that the
cast preserves runtime behavior while avoiding upstream type issues used by
LabelsBitmaskTileLayer.
- Around line 1-5: Export the MAX_LABEL_CHANNELS constant from
LabelsBitmaskTileLayer (add export to the existing MAX_LABEL_CHANNELS) and
re-export it from the package entry (packages/layers/src/index.ts), then update
useLayerData.ts to import and use that exported MAX_LABEL_CHANNELS instead of
hardcoding the literal 7 so consumers stay in sync.
In `@packages/layers/src/LabelsLayer.ts`:
- Line 27: Add a brief inline comment next to the existing type assertion `const
UntypedTileLayer = TileLayer as any;` explaining that the `as any` cast is a
temporary workaround for a Viv typing issue, and include a reference to the
upstream Viv issue or to `spatial-canvas-status.mdx` so maintainers can remove
the cast once Viv fixes land; keep the comment short and placed on the same line
or immediately above the declaration for visibility.
- Around line 158-162: The current destructuring of base.shape.slice(-2) in
LabelsLayer (after calling getBaseLoader) assumes shape has at least two
dimensions; add a defensive check that verifies base.shape.length >= 2 before
slicing/destructuring (or early return null) to avoid undefined height/width
when shape is malformed; reference the getBaseLoader call, the base variable and
the base.shape usage and update the code path that currently does const [height,
width] = base.shape.slice(-2) to validate length and handle the
error/early-return.
In `@packages/vis/src/SpatialCanvas/useLayerData.ts`:
- Around line 660-666: The literal "7" used to slice selections in
useLayerData.ts should be replaced with a shared constant to avoid divergence
with LabelsBitmaskTileLayer.ts; export MAX_LABEL_CHANNELS from the layers
package (or from LabelsBitmaskTileLayer.ts where it's currently defined) and
import that constant into useLayerData.ts, then replace .slice(0, 7) with
.slice(0, MAX_LABEL_CHANNELS) so clampVivSelectionsToAxes/buildDefaultSelection
consume the same channel limit as the shader layer.
🪄 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
Run ID: ffa67f89-ec9b-439f-808a-acfb6ceb0c01
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
docs/docs/vis/spatial-canvas-status.mdxpackages/layers/package.jsonpackages/layers/src/LabelsBitmaskTileLayer.tspackages/layers/src/LabelsLayer.tspackages/layers/src/index.tspackages/layers/src/labelsBitmaskLayerShaders.tspackages/layers/src/spatialLayerProps.tspackages/layers/vite.config.tspackages/vis/src/SpatialCanvas/SpatialViewer.tsxpackages/vis/src/SpatialCanvas/VivSpatialViewer.tsxpackages/vis/src/SpatialCanvas/index.tsxpackages/vis/src/SpatialCanvas/renderers/imageRenderer.tspackages/vis/src/SpatialCanvas/renderers/index.tspackages/vis/src/SpatialCanvas/renderers/labelsRenderer.tspackages/vis/src/SpatialCanvas/types.tspackages/vis/src/SpatialCanvas/useLayerData.tspackages/vis/tsconfig.jsonpackages/vis/vite.config.ts
Comment on lines
+58
to
+73
| dataToTexture(data: unknown, width: number, height: number) { | ||
| return this.context.device.createTexture({ | ||
| width, | ||
| height, | ||
| dimension: '2d', | ||
| data: new Float32Array(data as ArrayLike<number>), | ||
| mipmaps: false, | ||
| sampler: { | ||
| minFilter: 'nearest', | ||
| magFilter: 'nearest', | ||
| addressModeU: 'clamp-to-edge', | ||
| addressModeV: 'clamp-to-edge', | ||
| }, | ||
| format: 'r32float', | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Add runtime validation for data before casting.
The function accepts unknown but immediately casts to ArrayLike<number>. If the upstream loader returns an unexpected shape, this will fail silently or produce corrupt textures.
🛡️ Suggested defensive check
dataToTexture(data: unknown, width: number, height: number) {
+ if (!data || !ArrayBuffer.isView(data) && !Array.isArray(data)) {
+ throw new Error('dataToTexture: expected ArrayLike<number>');
+ }
return this.context.device.createTexture({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dataToTexture(data: unknown, width: number, height: number) { | |
| return this.context.device.createTexture({ | |
| width, | |
| height, | |
| dimension: '2d', | |
| data: new Float32Array(data as ArrayLike<number>), | |
| mipmaps: false, | |
| sampler: { | |
| minFilter: 'nearest', | |
| magFilter: 'nearest', | |
| addressModeU: 'clamp-to-edge', | |
| addressModeV: 'clamp-to-edge', | |
| }, | |
| format: 'r32float', | |
| }); | |
| } | |
| dataToTexture(data: unknown, width: number, height: number) { | |
| if (!data || !ArrayBuffer.isView(data) && !Array.isArray(data)) { | |
| throw new Error('dataToTexture: expected ArrayLike<number>'); | |
| } | |
| return this.context.device.createTexture({ | |
| width, | |
| height, | |
| dimension: '2d', | |
| data: new Float32Array(data as ArrayLike<number>), | |
| mipmaps: false, | |
| sampler: { | |
| minFilter: 'nearest', | |
| magFilter: 'nearest', | |
| addressModeU: 'clamp-to-edge', | |
| addressModeV: 'clamp-to-edge', | |
| }, | |
| format: 'r32float', | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/layers/src/LabelsBitmaskTileLayer.ts` around lines 58 - 73, The
dataToTexture method casts unknown to ArrayLike<number> and constructs a
Float32Array without validation, which can silently fail or corrupt textures;
update dataToTexture to validate that the incoming data is an ArrayLike<number>
(or a TypedArray/number[]), that its length equals width*height (or
width*height*channels if applicable), and throw or return a clear error if
invalid; when valid, create a Float32Array from the input (preserving TypedArray
inputs) and then call this.context.device.createTexture with that array
(referencing dataToTexture, Float32Array, ArrayLike<number>, and
this.context.device.createTexture to locate the change).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/layers/src/LabelsLayer.ts (1)
10-24: Use a discriminated union forloaderinstead ofunknown.This file directly calls
getRaster,getTile, accessestileSize,shape, andonTileErroron the loader without type safety, forcing multipleas anycasts (lines 250, 269, 308, 321). Replaceloader: unknownwith a proper union:
- Single-scale:
{ getRaster: (...) => ...; shape: number[]; tileSize: number; onTileError?: (...) => void; }- Multi-scale: Array of the above
This moves the runtime contract to the type boundary, eliminating unsafe casts and making
isMultiscaleLoaderproperly typed.Also applies to lines 29–35.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/layers/src/LabelsLayer.ts` around lines 10 - 24, LabelsLayerProps currently types loader as unknown which forces unsafe casts; change loader to a discriminated union of a single-scale loader interface (exposing getRaster/getTile, shape: number[], tileSize: number, optional onTileError) or an array of that interface for multiscale, then update isMultiscaleLoader to narrow that union so call sites (getRaster, getTile, tileSize, shape, onTileError) can be accessed without any casts; reference the LabelsLayerProps type and the isMultiscaleLoader predicate, and ensure functions/methods that used loader (calls around getRaster/getTile and properties tileSize, shape, onTileError) use the narrowed type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/layers/src/labelsBitmaskLayerShaders.ts`:
- Around line 138-144: The conditionals in the blend chain use `==` on `vec4`
(e.g., `val1 == fragColor || val1 == vec4(0.0)`), which yields a bvec4 and thus
is invalid with `||`/`?:`; replace those vector equality checks with scalar
boolean tests using GLSL vector predicate functions—e.g., use `all(equal(val1,
fragColor))` and `all(equal(val1, vec4(0.0)))` (or test `val1.a == 0.0` if you
only care about alpha) for each occurrence involving fragColor and valN, keeping
the rest of the ternary expression intact for `fragColor = ...` lines for val1
through val6. Ensure you update every occurrence (val1..val6) to use
`all(equal(...))` so the condition is a scalar bool.
In `@packages/layers/src/LabelsBitmaskTileLayer.ts`:
- Around line 87-93: When a tile resolves with no channel data, clear
this.state.textures after deleting GPU textures so draw() won't rebind deleted
textures; in LabelsBitmaskTileLayer update the no-data branch that currently
only resets _setNewTexturesFromLoadThisFrame to also set this.state.textures =
{} (or null) after the deletion loop, and apply the same change in the other
similar branch that deletes textures (the block around the second deletion) so
both code paths remove references to deleted textures.
- Around line 87-93: this.state.textures may contain the same texture object
under multiple keys (e.g. missing channels are backfilled with
textures.channel0), so the current loop can call delete() on the same instance
multiple times; before calling tex?.delete?.() in the cleanup block referenced
by this.state.textures and delete(), deduplicate the texture objects (e.g.
collect non-null texture references into a Set using the same identity) and then
iterate that Set to call delete() once per unique texture instance, retaining
the existing null-safe checks.
In `@packages/layers/src/LabelsLayer.ts`:
- Around line 109-114: The layer currently hard-codes pickable: false in the
getSubLayerProps call inside LabelsLayer (e.g., the sublayer id
'single-scale-labels-bitmask' and the similar block around line ~291), which
prevents deck.gl from invoking onHover/onClick; change the pickable property to
be conditional (pickable: Boolean(onClick || onHover) or equivalent) so that
when interaction handlers (onClick/onHover) are functions the sublayer becomes
pickable, leaving it false otherwise; update both occurrences in LabelsLayer.ts
(the blocks that spread getSubLayerProps with id 'single-scale-labels-bitmask'
and the other render path) to use this conditional pickable logic.
---
Nitpick comments:
In `@packages/layers/src/LabelsLayer.ts`:
- Around line 10-24: LabelsLayerProps currently types loader as unknown which
forces unsafe casts; change loader to a discriminated union of a single-scale
loader interface (exposing getRaster/getTile, shape: number[], tileSize: number,
optional onTileError) or an array of that interface for multiscale, then update
isMultiscaleLoader to narrow that union so call sites (getRaster, getTile,
tileSize, shape, onTileError) can be accessed without any casts; reference the
LabelsLayerProps type and the isMultiscaleLoader predicate, and ensure
functions/methods that used loader (calls around getRaster/getTile and
properties tileSize, shape, onTileError) use the narrowed type.
🪄 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
Run ID: 89f14c50-095b-4859-afd2-b8025d62aa62
📒 Files selected for processing (6)
packages/layers/src/LabelsBitmaskTileLayer.tspackages/layers/src/LabelsLayer.tspackages/layers/src/labelsBitmaskLayerShaders.tspackages/vis/src/SpatialCanvas/SpatialViewer.tsxpackages/vis/src/SpatialCanvas/VivSpatialViewer.tsxpackages/vis/src/SpatialCanvas/useLayerData.ts
💤 Files with no reviewable changes (2)
- packages/vis/src/SpatialCanvas/VivSpatialViewer.tsx
- packages/vis/src/SpatialCanvas/SpatialViewer.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vis/src/SpatialCanvas/useLayerData.ts
Comment on lines
+87
to
+93
| if (this.state.textures) { | ||
| Object.values( | ||
| this.state.textures as Record<string, { delete?: () => void } | null> | ||
| ).forEach((tex) => { | ||
| tex?.delete?.(); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Clear state.textures when a tile resolves with no channel data.
This method deletes the previous GPU textures up front, but the no-data branch only resets _setNewTexturesFromLoadThisFrame. this.state.textures still points at the deleted objects, so draw() can keep rebinding invalid textures on later frames.
💡 Suggested fix
- (this as any)._setNewTexturesFromLoadThisFrame?.(null);
+ (this as any)._setNewTexturesFromLoadThisFrame?.(null);
+ this.setState({ textures: null });Also applies to: 115-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/layers/src/LabelsBitmaskTileLayer.ts` around lines 87 - 93, When a
tile resolves with no channel data, clear this.state.textures after deleting GPU
textures so draw() won't rebind deleted textures; in LabelsBitmaskTileLayer
update the no-data branch that currently only resets
_setNewTexturesFromLoadThisFrame to also set this.state.textures = {} (or null)
after the deletion loop, and apply the same change in the other similar branch
that deletes textures (the block around the second deletion) so both code paths
remove references to deleted textures.
Deduplicate texture deletes before reloading channels.
Missing channels are backfilled with textures.channel0, so this.state.textures can contain the same texture object under several keys. This loop deletes every entry, which can call delete() multiple times on the same instance during the next reload.
💡 Suggested fix
if (this.state.textures) {
- Object.values(
- this.state.textures as Record<string, { delete?: () => void } | null>
- ).forEach((tex) => {
- tex?.delete?.();
- });
+ const previousTextures = Object.values(
+ this.state.textures as Record<string, { delete?: () => void } | null>
+ );
+ for (const tex of new Set(previousTextures.filter(Boolean))) {
+ tex?.delete?.();
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/layers/src/LabelsBitmaskTileLayer.ts` around lines 87 - 93,
this.state.textures may contain the same texture object under multiple keys
(e.g. missing channels are backfilled with textures.channel0), so the current
loop can call delete() on the same instance multiple times; before calling
tex?.delete?.() in the cleanup block referenced by this.state.textures and
delete(), deduplicate the texture objects (e.g. collect non-null texture
references into a Set using the same identity) and then iterate that Set to call
delete() once per unique texture instance, retaining the existing null-safe
checks.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/layers/src/labelsBitmaskLayerShaders.ts (1)
179-185:⚠️ Potential issue | 🔴 CriticalFix the blend condition to use a scalar boolean.
vec4 == vec4returns abvec4in GLSL ES 3.00, so these||/ ternary conditions do not compile. Useall(equal(...))or just test alpha for the zero-color fast path.Suggested fix
- fragColor = (val1 == fragColor || val1 == vec4(0.0)) ? fragColor : vec4(mix(fragColor, val1, val1.a).rgb, max(fragColor.a, val1.a)); - fragColor = (val2 == fragColor || val2 == vec4(0.0)) ? fragColor : vec4(mix(fragColor, val2, val2.a).rgb, max(fragColor.a, val2.a)); - fragColor = (val3 == fragColor || val3 == vec4(0.0)) ? fragColor : vec4(mix(fragColor, val3, val3.a).rgb, max(fragColor.a, val3.a)); - fragColor = (val4 == fragColor || val4 == vec4(0.0)) ? fragColor : vec4(mix(fragColor, val4, val4.a).rgb, max(fragColor.a, val4.a)); - fragColor = (val5 == fragColor || val5 == vec4(0.0)) ? fragColor : vec4(mix(fragColor, val5, val5.a).rgb, max(fragColor.a, val5.a)); - fragColor = (val6 == fragColor || val6 == vec4(0.0)) ? fragColor : vec4(mix(fragColor, val6, val6.a).rgb, max(fragColor.a, val6.a)); + fragColor = ((val1.a == 0.0) || all(equal(val1, fragColor))) ? fragColor : vec4(mix(fragColor, val1, val1.a).rgb, max(fragColor.a, val1.a)); + fragColor = ((val2.a == 0.0) || all(equal(val2, fragColor))) ? fragColor : vec4(mix(fragColor, val2, val2.a).rgb, max(fragColor.a, val2.a)); + fragColor = ((val3.a == 0.0) || all(equal(val3, fragColor))) ? fragColor : vec4(mix(fragColor, val3, val3.a).rgb, max(fragColor.a, val3.a)); + fragColor = ((val4.a == 0.0) || all(equal(val4, fragColor))) ? fragColor : vec4(mix(fragColor, val4, val4.a).rgb, max(fragColor.a, val4.a)); + fragColor = ((val5.a == 0.0) || all(equal(val5, fragColor))) ? fragColor : vec4(mix(fragColor, val5, val5.a).rgb, max(fragColor.a, val5.a)); + fragColor = ((val6.a == 0.0) || all(equal(val6, fragColor))) ? fragColor : vec4(mix(fragColor, val6, val6.a).rgb, max(fragColor.a, val6.a));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/layers/src/labelsBitmaskLayerShaders.ts` around lines 179 - 185, The GLSL comparisons like "val1 == fragColor" produce bvec4 and break the ternary/|| logic; update each blend condition for fragColor with val0..val6 to use a scalar boolean such as all(equal(valN, fragColor)) or a fast alpha test like (valN.a == 0.0) so the expressions become scalar booleans before the ||/?: operators; modify the lines that set fragColor (referencing fragColor and val0..val6) to use one of these scalar tests consistently for all valN entries.packages/layers/src/LabelsLayer.ts (1)
110-116:⚠️ Potential issue | 🟠 MajorMake the labels sublayers pickable when interaction is enabled.
deck.gl only emits picking info for
pickablelayers. Withpickable: falsein both paths, label hover/click stays disabled even when handlers are supplied upstream.Suggested fix
this.getSubLayerProps({ id: 'single-scale-labels-bitmask', - pickable: false, + pickable: typeof onClick === 'function' || typeof onHover === 'function', ...(typeof onClick === 'function' ? { onClick } : {}), ...(typeof onHover === 'function' ? { onHover } : {}), }),return new MultiscaleLabelsTileLayer( this.getSubLayerProps({ id: 'labels', - pickable: false, + pickable: typeof onClick === 'function' || typeof onHover === 'function', visible, }),return new SingleScaleLabelsLayer( this.getSubLayerProps({ id: 'labels', - pickable: false, + pickable: typeof onClick === 'function' || typeof onHover === 'function', visible, }),Also applies to: 297-323
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/layers/src/LabelsLayer.ts` around lines 110 - 116, Labels sublayers are created with pickable: false which prevents deck.gl picking events from firing even when interaction handlers are provided; update the sublayer creation in LabelsLayer (e.g., where LabelsBitmaskTileLayer is instantiated) to set pickable: true whenever an onClick or onHover handler is supplied (check both code paths, including the other sublayer creation around the 297-323 region), by computing pickable based on typeof onClick === 'function' || typeof onHover === 'function' and merging that into the getSubLayerProps call so pickable is true when interaction is enabled.packages/layers/src/LabelsBitmaskTileLayer.ts (1)
89-118:⚠️ Potential issue | 🟠 MajorReset
state.texturesand dedupe deletes during reload.After the channel0 backfill, several entries can point at the same GPU texture. This cleanup loop deletes every entry, and the no-data path keeps
this.state.texturespointing at deleted objects, so a laterdraw()can rebind freed textures.Suggested fix
if (this.state.textures) { - Object.values( - this.state.textures as Record<string, { delete?: () => void } | null> - ).forEach((tex) => { - tex?.delete?.(); - }); + const previousTextures = Object.values( + this.state.textures as Record<string, { delete?: () => void } | null> + ).filter(Boolean); + for (const tex of new Set(previousTextures)) { + tex?.delete?.(); + } } @@ - (this as any)._setNewTexturesFromLoadThisFrame?.(null); + (this as any)._setNewTexturesFromLoadThisFrame?.(null); + this.setState({ textures: null });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/layers/src/LabelsBitmaskTileLayer.ts` around lines 89 - 118, The cleanup currently deletes each entry in this.state.textures which can delete the same GPU texture multiple times and leaves this.state.textures referencing deleted textures; in LabelsBitmaskTileLayer change the deletion loop to dedupe by collecting unique texture objects from this.state.textures (e.g., using a Set) and call delete on each unique texture exactly once, then clear/reset this.state.textures (set to null or {}) before creating new textures so the no-data path and later draw() cannot rebind freed textures; keep using dataToTexture, _setNewTexturesFromLoadThisFrame and setState as currently named when assigning the new textures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vis/src/SpatialCanvas/useLayerData.ts`:
- Around line 105-115: LabelsLoaderData currently lacks per-channel IDs so the
UI synthesizes `${layerId}:labels:${index}` at render time which breaks identity
when channels are reordered/duplicated; add an ids: string[] property to
LabelsLoaderData and generate stable unique IDs exactly once when the label
loader defaults are created inside useLayerData (the same place other defaults
like colors, channelsVisible, channelOpacities are initialized), then pass these
ids through to LabelsChannelPanel instead of recreating them at render; ensure
IDs are not derived from runtime indices (use a one-time UUID or similarly
stable generator at default-creation) and update any code that reads channels to
use labelsLoaderData.ids for channel identity.
---
Duplicate comments:
In `@packages/layers/src/labelsBitmaskLayerShaders.ts`:
- Around line 179-185: The GLSL comparisons like "val1 == fragColor" produce
bvec4 and break the ternary/|| logic; update each blend condition for fragColor
with val0..val6 to use a scalar boolean such as all(equal(valN, fragColor)) or a
fast alpha test like (valN.a == 0.0) so the expressions become scalar booleans
before the ||/?: operators; modify the lines that set fragColor (referencing
fragColor and val0..val6) to use one of these scalar tests consistently for all
valN entries.
In `@packages/layers/src/LabelsBitmaskTileLayer.ts`:
- Around line 89-118: The cleanup currently deletes each entry in
this.state.textures which can delete the same GPU texture multiple times and
leaves this.state.textures referencing deleted textures; in
LabelsBitmaskTileLayer change the deletion loop to dedupe by collecting unique
texture objects from this.state.textures (e.g., using a Set) and call delete on
each unique texture exactly once, then clear/reset this.state.textures (set to
null or {}) before creating new textures so the no-data path and later draw()
cannot rebind freed textures; keep using dataToTexture,
_setNewTexturesFromLoadThisFrame and setState as currently named when assigning
the new textures.
In `@packages/layers/src/LabelsLayer.ts`:
- Around line 110-116: Labels sublayers are created with pickable: false which
prevents deck.gl picking events from firing even when interaction handlers are
provided; update the sublayer creation in LabelsLayer (e.g., where
LabelsBitmaskTileLayer is instantiated) to set pickable: true whenever an
onClick or onHover handler is supplied (check both code paths, including the
other sublayer creation around the 297-323 region), by computing pickable based
on typeof onClick === 'function' || typeof onHover === 'function' and merging
that into the getSubLayerProps call so pickable is true when interaction is
enabled.
🪄 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
Run ID: 6b33a8e1-2046-4983-8a1e-f15b7f9a7a32
📒 Files selected for processing (8)
packages/layers/src/LabelsBitmaskTileLayer.tspackages/layers/src/LabelsLayer.tspackages/layers/src/labelsBitmaskLayerShaders.tspackages/vis/src/SpatialCanvas/LabelsChannelPanel.tsxpackages/vis/src/SpatialCanvas/index.tsxpackages/vis/src/SpatialCanvas/renderers/labelsRenderer.tspackages/vis/src/SpatialCanvas/types.tspackages/vis/src/SpatialCanvas/useLayerData.ts
✅ Files skipped from review due to trivial changes (1)
- packages/vis/src/SpatialCanvas/renderers/labelsRenderer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vis/src/SpatialCanvas/types.ts
xinaesthete
force-pushed
the
codex/add-labels-deckgl-layer
branch
from
April 13, 2026 14:56
0d73074 to
adba2f7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/vis/src/SpatialCanvas/VivSpatialViewer.tsx (1)
190-197: Return a fully shapedVivViewStatehere instead of asserting one.
getDefaultInitialViewState()only gives you{ target, zoom }, so this branch currently omitswidth/heightand relies onas VivViewStateto hide the mismatch. Making both init paths build the same object shape keeps the runtime consistent and drops one assertion.Suggested cleanup
const defaultState = getDefaultInitialViewState(firstLayerWithLoader.loader, { width: this.props.width, height: this.props.height, }, 0, false, firstLayerWithLoader.modelMatrix); return { ...defaultState, id: this.viewId, - } as VivViewState; + width: this.props.width, + height: this.props.height, + };As per coding guidelines,
**/*.{ts,tsx}: “Prefer types that match runtime behavior” and “Avoid type assertions (as).”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vis/src/SpatialCanvas/VivSpatialViewer.tsx` around lines 190 - 197, The branch currently casts the slim result of getDefaultInitialViewState(...) to VivViewState, omitting runtime fields like width/height; instead build and return a complete VivViewState object: call getDefaultInitialViewState(...) to get {target, zoom}, then compose and return { ...defaultState, width: this.props.width, height: this.props.height, id: this.viewId, modelMatrix: firstLayerWithLoader.modelMatrix, /* include any other VivViewState fields required at runtime */ } without using as VivViewState so both init paths produce the same shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vis/src/SpatialCanvas/VivSpatialViewer.tsx`:
- Around line 310-325: The vivLayers entries created from layersForImage must
include the view-specific vivId suffix so they pass layerFilter; update the loop
that pushes cloned layers (the vivLayers.push(layer.clone({ id:
`${layer.id}-${imageLayerProps.id}` }))) to use the withVivId helper (same way
extraLayersWithVivId/deckPropsLayersWithVivId do) when cloning non-ScaleBarLayer
items, e.g. produce ids using withVivId(layer, imageLayerProps.id or
getVivId(viewport.id) as appropriate) before pushing to vivLayers; keep the
ScaleBarLayer special-case (scaleBarAdded) behavior unchanged.
---
Nitpick comments:
In `@packages/vis/src/SpatialCanvas/VivSpatialViewer.tsx`:
- Around line 190-197: The branch currently casts the slim result of
getDefaultInitialViewState(...) to VivViewState, omitting runtime fields like
width/height; instead build and return a complete VivViewState object: call
getDefaultInitialViewState(...) to get {target, zoom}, then compose and return {
...defaultState, width: this.props.width, height: this.props.height, id:
this.viewId, modelMatrix: firstLayerWithLoader.modelMatrix, /* include any other
VivViewState fields required at runtime */ } without using as VivViewState so
both init paths produce the same shape.
🪄 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
Run ID: 9ad242b0-fac4-4b8b-b974-d0c67c386d67
📒 Files selected for processing (15)
packages/core/src/index.tspackages/core/src/tooltip.tspackages/core/src/transformations/transformations.tspackages/core/tests/transformations.spec.tspackages/layers/src/LabelsBitmaskTileLayer.tspackages/layers/src/LabelsLayer.tspackages/layers/src/spatialLayerProps.tspackages/vis/src/SpatialCanvas/SpatialFeatureTooltip.tsxpackages/vis/src/SpatialCanvas/TooltipFieldsPanel.tsxpackages/vis/src/SpatialCanvas/VivSpatialViewer.tsxpackages/vis/src/SpatialCanvas/index.tsxpackages/vis/src/SpatialCanvas/renderers/shapesRenderer.tspackages/vis/src/SpatialCanvas/types.tspackages/vis/src/SpatialCanvas/useLayerData.tspackages/vis/src/viv-typing-fixes.d.ts
✅ Files skipped from review due to trivial changes (2)
- packages/core/tests/transformations.spec.ts
- packages/vis/src/viv-typing-fixes.d.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/vis/src/SpatialCanvas/types.ts
- packages/layers/src/spatialLayerProps.ts
- packages/layers/src/LabelsLayer.ts
👮 Files not reviewed due to content moderation or server errors (8)
- packages/core/src/tooltip.ts
- packages/core/src/index.ts
- packages/layers/src/LabelsBitmaskTileLayer.ts
- packages/vis/src/SpatialCanvas/renderers/shapesRenderer.ts
- packages/vis/src/SpatialCanvas/index.tsx
- packages/vis/src/SpatialCanvas/SpatialFeatureTooltip.tsx
- packages/vis/src/SpatialCanvas/TooltipFieldsPanel.tsx
- packages/vis/src/SpatialCanvas/useLayerData.ts
This was referenced May 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SpatialCanvasvisandlayerspackage config to work with the tightened toolchainTesting
pnpm --filter @spatialdata/layers buildpnpm --filter @spatialdata/vis buildSummary by CodeRabbit
New Features
Documentation