feat(netcdf): fit, colormap, and band combination for local grids - #1708
Conversation
…rids Adding a local NetCDF/HDF layer left the camera where it was and used the renderer's stock 0-300 color limits, so real data such as an EMIT scene was off screen and washed out, and "Zoom to layer" had no extent to fly to. Local grids are now colormapped in the browser and added as image overlays, which also sidesteps a shader-uniform crash in @carbonplan/zarr-layer that blanks Zarr layers on Mesa drivers. Hyperspectral cubes gain an RGB band combination picked by wavelength, and the colormap catalogue is now shared with the Style panel's raster symbology instead of a much shorter list.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds local and remote axis-aware NetCDF rendering, RGB and colormapped image composition, editable image symbology, shared colormap loading, pixel identification, profile charts, cloud bounds metadata, and inline-image history accounting. ChangesNetCDF rendering
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AddNetcdfDialog
participant RemoteNetcdfFile
participant LocalNetcdfFile
participant ImageOverlayLayer
participant useNetcdfIdentify
participant NetcdfProfileStore
participant NetcdfProfilePanel
User->>AddNetcdfDialog: Select source, variable, axes, and rendering mode
AddNetcdfDialog->>RemoteNetcdfFile: Load direct URL metadata when applicable
AddNetcdfDialog->>LocalNetcdfFile: Read grid or compose image
LocalNetcdfFile-->>AddNetcdfDialog: Return image, bounds, and metadata
AddNetcdfDialog->>ImageOverlayLayer: Add rendered NetCDF layer
User->>useNetcdfIdentify: Click image cell
useNetcdfIdentify->>NetcdfProfileStore: Store sampled profile
NetcdfProfileStore-->>NetcdfProfilePanel: Notify profile chart
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Cloudflare PR preview
|
There was a problem hiding this comment.
Pull request overview
This PR significantly improves how GeoLibre ingests and renders local NetCDF/HDF grids, adding robust defaults (camera fit + percentile-based color limits), a CPU-backed single-band image overlay path, and an RGB composite path for hyperspectral cubes. It also unifies colormap selection across the app by sharing a full ramp catalogue (with swatches) via a new hook, and adds a Style panel section to re-style baked NetCDF image layers after creation.
Changes:
- Extend the local NetCDF/HDF reader to expose axis metadata, compute reliable grid bounds, and derive robust color limits (percentiles) for initial rendering.
- Add CPU-side composition for single-band colormapped images and RGB composites, with UI support in the Add NetCDF dialog and post-add symbology controls in the Style panel.
- Normalize sprite-sampled colormap stops to hex and share a unified ramp catalogue across raster and NetCDF pickers.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/local-netcdf.test.ts | Adds unit tests for bounds, percentile clim, RGB composition, colormapped images, and axis handling. |
| tests/colormap-colors.test.ts | Adds tests for normalizing rgb()/rgba() ramp stops to hex. |
| packages/plugins/src/plugins/maplibre-components.ts | Extends NetCDF layer options (bounds + colormap names) and persists bounds onto the store layer record. |
| packages/plugins/src/plugins/local-netcdf.ts | Implements bounds computation, percentile clim, RGB/colormap image composition, and axis/metadata extraction for local NetCDF/HDF. |
| packages/plugins/src/plugins/colormap-colors.ts | Normalizes sprite-sampled colormap stops to #rrggbb and exports the helper. |
| packages/plugins/src/index.ts | Exports the new NetCDF helpers/types and the ramp color normalizer. |
| packages/core/src/store.ts | Extends image overlay layer creation options to carry sourceKind and merged metadata. |
| apps/geolibre-desktop/src/lib/netcdf-image-symbology.ts | Introduces retained in-memory grid storage + rebake helpers for NetCDF image overlays and Style-panel integration. |
| apps/geolibre-desktop/src/i18n/locales/en.json | Adds new strings for NetCDF band-combination and symbology UI. |
| apps/geolibre-desktop/src/hooks/useColormapRamps.ts | New hook that builds a unified, swatched colormap catalogue shared by multiple pickers. |
| apps/geolibre-desktop/src/components/panels/StylePanel.tsx | Shows a NetCDF symbology section for baked NetCDF image layers. |
| apps/geolibre-desktop/src/components/panels/RasterSymbologySection.tsx | Switches raster ramp picker to the shared colormap catalogue hook. |
| apps/geolibre-desktop/src/components/panels/NetcdfSymbologySection.tsx | New Style-panel section to re-colormap baked NetCDF image overlays and adjust limits. |
| apps/geolibre-desktop/src/components/layout/AddNetcdfDialog.tsx | Adds colormap selection, RGB band-combination controls, and chooses between image vs Zarr render paths for local NetCDF. |
Suppressed comments (1)
apps/geolibre-desktop/src/components/panels/NetcdfSymbologySection.tsx:47
- Calling setState during render (
if (draftLayerId !== layer.id) { ... }) is a React anti-pattern and can trigger warnings and extra renders. Synchronize the text fields when the selected layer changes using useEffect instead, and drop the extra draftLayerId state.
// The layer a draft belongs to; selecting a different layer must reload the
// fields rather than keep the previous layer's numbers.
const [draftLayerId, setDraftLayerId] = useState(layer.id);
if (draftLayerId !== layer.id) {
setDraftLayerId(layer.id);
setMinText(String(applied.clim[0]));
setMaxText(String(applied.clim[1]));
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,133 @@ | |||
| import { useAppStore, type GeoLibreLayer } from "@geolibre/core"; | |||
| import { Button, ColorRampSelect, Input, Label, Separator } from "@geolibre/ui"; | |||
| import { useState } from "react"; | |||
There was a problem hiding this comment.
Not adopting the useEffect refactor, so no new import is needed. The render-phase setState is React's documented "adjusting state when props change" pattern (a layer switch resets the draft text fields); moving it to an effect would render one frame showing the previous layer's numbers. Leaving as is.
There was a problem hiding this comment.
Disagreeing — no useEffect import is needed, so leaving this open in case you want to weigh in. This comment is about a suggested refactor of the render-phase state reset (NetcdfSymbologySection.tsx:46-57), but that pattern is intentional: comparing a draftLayerId state to the current prop and calling setState during render is React's documented way to adjust state when props change, and it is preferred over a useEffect — it re-renders before the browser paints, where an effect would flash the previous layer's values first. The reset was extended in 2533d7a (it now also clears pendingSymbology), still without an effect. useState is the only React import this file needs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@apps/geolibre-desktop/src/components/layout/AddNetcdfDialog.tsx`:
- Around line 352-366: Update the RGB branch in AddNetcdfDialog’s
addImageOverlayLayer call to pass an explicit non-KML sourceKind for the NetCDF
composite, matching the source-kind convention used by the single-band branch.
Keep the existing bounds and overlay behavior unchanged, and ensure the value
does not use the default "kml-ground-overlay" marker.
- Around line 149-156: Update the hasTimeAxis calculation near useImagePath to
detect a time dimension from either axes or leadingDims, using leadingDims as
the fallback when axes is empty or unavailable. Preserve the existing
case-insensitive TIME_DIMENSION_NAMES matching so time-enabled cubes continue
using the Zarr renderer.
- Around line 565-618: Update the rendering around the rgbMode branch so RGB
channel selectors are shown in addition to, not instead of, the existing
leading-dimension pickers. Exclude only the RGB axis from the regular
leadingDims picker list to avoid duplicating it, while keeping all other
dimensions editable through dimIndex for handleSubmit.
- Around line 750-762: Update wavelengthsInNanometres to recognize the missing
unit spellings: nanometre, micrometre, micrometres, microns, and micrometer.
Hoist the nanometre and micrometre unit sets to module scope so they are created
once, while preserving the existing conversion and null behavior.
In `@apps/geolibre-desktop/src/components/panels/NetcdfSymbologySection.tsx`:
- Around line 51-57: The apply handler in NetcdfSymbologySection currently
performs bakeNetcdfImage and encodeImageOverlay synchronously, blocking controls
for large grids. Defer this work until after the interaction renders and expose
a pending state while it runs, ensuring colormap and reverse-control changes
remain responsive and updates are applied only after the bake completes.
In `@apps/geolibre-desktop/src/components/panels/StylePanel.tsx`:
- Around line 4651-4662: Update the StylePanel branch around
NetcdfSymbologySection to derive hasNetcdfSymbology from the layer source kind
and getNetcdfImageSource(layer.id), then show the no-controls message when that
value is false, including reloaded NetCDF layers without an in-memory grid. Wrap
the NetCDF content in the same ScrollArea and pe-5 clearance used by sibling
branches so the tall symbology controls can scroll without overflow.
In `@apps/geolibre-desktop/src/lib/netcdf-image-symbology.ts`:
- Around line 160-168: Inspect pruneHistoryBySize and its temporal partialize
path for image data URLs such as those produced by encodeImageOverlay and stored
in layers. Ensure the size-pruning calculation includes these overlay strings,
then apply the existing history-size limit so repeated updateLayer symbology
changes cannot retain unbounded PNG snapshots; avoid changing image encoding
unless the pruning logic cannot account for the payload.
- Around line 44-50: Update the useAppStore subscription that prunes sources to
track the previous state.layers reference and return immediately when the
reference is unchanged. Only allocate the live layer-ID Set and remove stale
source IDs after the layers reference changes, while preserving the existing
pruning behavior.
In `@packages/plugins/src/plugins/colormap-colors.ts`:
- Around line 57-59: Update the numeric channel parsing in normalizeRampColor so
every parsed channel is validated with Number.isFinite() before rgbToHex() is
called. For malformed tokens such as "." or "1..2", return the original color
input unchanged, preserving the documented `#rrggbb` contract; add coverage for
rgb(., 0, 0) passing through unchanged.
In `@packages/plugins/src/plugins/local-netcdf.ts`:
- Around line 788-818: Update buildRgbImage to decode the NetCDF variable once
before constructing channels, then slice each requested index from that shared
decoded array instead of calling readPlane for every channel. Preserve the
existing selector, axis handling, coordinate reads, and RGB composition while
ensuring readPlane or its underlying reader/info.make path is not repeated for
the three channels.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 49c771e9-0b10-4408-b7ae-a2d722bbeaea
📒 Files selected for processing (14)
apps/geolibre-desktop/src/components/layout/AddNetcdfDialog.tsxapps/geolibre-desktop/src/components/panels/NetcdfSymbologySection.tsxapps/geolibre-desktop/src/components/panels/RasterSymbologySection.tsxapps/geolibre-desktop/src/components/panels/StylePanel.tsxapps/geolibre-desktop/src/hooks/useColormapRamps.tsapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/netcdf-image-symbology.tspackages/core/src/store.tspackages/plugins/src/index.tspackages/plugins/src/plugins/colormap-colors.tspackages/plugins/src/plugins/local-netcdf.tspackages/plugins/src/plugins/maplibre-components.tstests/colormap-colors.test.tstests/local-netcdf.test.ts
🔍 GitHub Pages PR preview
|
The Layers panel never offered Identify for a NetCDF layer, because the button is gated on a native layer id these image overlays do not have, so there was no way to read a value off the map. A click now reads the retained grid directly, and for a cube it also walks the band axis at that pixel to chart a spectral signature against wavelength. Reading a profile needs the source file, so it stays open for the layer's lifetime, capped to one cube at a time.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@apps/geolibre-desktop/src/components/panels/StylePanel.tsx`:
- Line 4661: Pass the selected layer’s id into NetcdfProfilePanel from the
StylePanel render, then update NetcdfProfilePanel to accept that layerId and
filter subscribed readings by reading.layerId. Render no profile content when no
readings match the selected layer, while preserving existing behavior for
matching readings.
In `@apps/geolibre-desktop/src/hooks/useNetcdfIdentify.ts`:
- Around line 70-78: Add English locale entries for the NetCDF identify labels
used by the popup, then use the `t` translation function in `useNetcdfIdentify`
for the `"no data"`, `"lon, lat"`, and `"row, col"` values instead of hardcoded
strings. Ensure the hook obtains `t` from `react-i18next` and preserves the
existing formatting for translated labels and coordinates.
In `@apps/geolibre-desktop/src/lib/netcdf-image-symbology.ts`:
- Around line 123-130: Update readNetcdfProfile to catch errors thrown by
state.profile.file.readProfile and return null for failures, while preserving
the existing null result when state or profile is unavailable and the successful
read path otherwise unchanged.
In `@packages/plugins/src/plugins/local-netcdf.ts`:
- Around line 927-939: Memoize decoded NetCDF-3 variables on the Netcdf3File
instance so repeated reads reuse one decoded array per variable. Add a
fullVariable helper or equivalent cache-backed path, replace direct
getDataVariable/info.make calls in readProfile and readPlane with it, and clear
the cache in close().
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 38c87f6f-8145-47f3-b1e1-af2b6d315798
📒 Files selected for processing (15)
apps/geolibre-desktop/src/components/layout/AddNetcdfDialog.tsxapps/geolibre-desktop/src/components/layout/DesktopShell.tsxapps/geolibre-desktop/src/components/panels/LayerPanel.tsxapps/geolibre-desktop/src/components/panels/NetcdfProfilePanel.tsxapps/geolibre-desktop/src/components/panels/StylePanel.tsxapps/geolibre-desktop/src/hooks/useNetcdfIdentify.tsapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/netcdf-image-symbology.tsapps/geolibre-desktop/src/lib/netcdf-profile-store.tspackages/core/src/types.tspackages/map/src/MapCanvas.tsxpackages/plugins/src/index.tspackages/plugins/src/plugins/local-netcdf.tstests/local-netcdf.test.tstests/netcdf-profile-store.test.ts
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- Never pick a time axis as the RGB band axis: a (time, lat, lon) stack offered a "combine three bands" control that actually combined three dates. Prefer an axis with wavelength units, else the first non-time axis. - Fall back to the variable's own dimension names for time detection, so a cube still reaches the Zarr path when listAxes throws and `axes` is empty. - Set an explicit sourceKind on the RGB overlay; it was defaulting to "kml-ground-overlay", which every consumer of that marker would act on. - Keep the other leading-dimension pickers visible in RGB mode, since handleSubmit still reads them and they were pinned at index 0. - Show the "no controls" message when a NetCDF layer has no retained grid (after a project reload, or for an RGB composite), and wrap the branch in ScrollArea so a tall symbology section can scroll. - Offer Identify only when the grid is actually readable, and mark these layers pixelIdentify so the tooltip matches the COG wording. - Charge inline data URLs against the undo-history budget: it counted only GeoJSON features, so each re-bake pushed a multi-megabyte snapshot the pruner scored as free. - Skip the layer-prune subscriber unless the layer array changed, and drop the subscription once nothing is registered; it was rebuilding a Set of every layer id on each pointer move. - Defer the symbology re-bake so the colormap and reverse controls repaint before the CPU colormap and PNG encode run. - Cover the remaining wavelength unit spellings, and reject malformed rgb() channels rather than emitting "#NaN...".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/geolibre-desktop/src/components/panels/NetcdfSymbologySection.tsx (1)
35-67: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScope deferred symbology state to its layer.
If the selected layer changes before the timer runs,
pendingSymbologystill applies the previous layer's colormap and limits to the new panel. An older timer can also clear pending state from a newer update. Store the targetlayer.idwith the pending value and use a generation token before clearing it.Proposed fix
-const [pendingSymbology, setPendingSymbology] = useState<NetcdfImageSymbology | null>(null); -const applied = pendingSymbology ?? netcdfImageSymbology(layer, source?.dataClim ?? [0, 1]); +const [pendingSymbology, setPendingSymbology] = useState<{ + layerId: string; + value: NetcdfImageSymbology; +} | null>(null); +const bakeGeneration = useRef(0); +const storedSymbology = netcdfImageSymbology(layer, source?.dataClim ?? [0, 1]); +const applied = + pendingSymbology?.layerId === layer.id ? pendingSymbology.value : storedSymbology; ... - setPendingSymbology(next); + const generation = ++bakeGeneration.current; + setPendingSymbology({ layerId: layer.id, value: next }); window.setTimeout(() => { ... - setPendingSymbology(null); + if (generation === bakeGeneration.current) setPendingSymbology(null); }, 0);🤖 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 `@apps/geolibre-desktop/src/components/panels/NetcdfSymbologySection.tsx` around lines 35 - 67, Update the pending state and apply timer in NetcdfSymbologySection so pendingSymbology is associated with the target layer.id, and only use it when it matches the current layer. Add a generation token for scheduled updates, and clear pending state only when the completing timer is still the latest generation and targets the current layer, preventing stale timers or layer changes from affecting newer state.
🤖 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.
Outside diff comments:
In `@apps/geolibre-desktop/src/components/panels/NetcdfSymbologySection.tsx`:
- Around line 35-67: Update the pending state and apply timer in
NetcdfSymbologySection so pendingSymbology is associated with the target
layer.id, and only use it when it matches the current layer. Add a generation
token for scheduled updates, and clear pending state only when the completing
timer is still the latest generation and targets the current layer, preventing
stale timers or layer changes from affecting newer state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c1e5ff3e-e484-421f-a6b8-635a3368a15a
📒 Files selected for processing (9)
apps/geolibre-desktop/src/components/layout/AddNetcdfDialog.tsxapps/geolibre-desktop/src/components/panels/LayerPanel.tsxapps/geolibre-desktop/src/components/panels/NetcdfSymbologySection.tsxapps/geolibre-desktop/src/components/panels/StylePanel.tsxapps/geolibre-desktop/src/lib/netcdf-image-symbology.tspackages/core/src/history.tspackages/plugins/src/plugins/colormap-colors.tstests/colormap-colors.test.tstests/undo-redo.test.ts
|
All 7 inline comments posted. Now the final summary. Code reviewBugs
Performance
CLAUDE.md
Quality
|
- Scope the spectral profile chart to the selected layer, so selecting a second NetCDF layer no longer shows the first one's spectra under its heading. - Catch a throw from readProfile: the caller reads it from a timer, where an exception would surface as an uncaught error on every click. - Memoize the NetCDF-3 whole-variable decode, which readPlane and readProfile were repeating on each call, and clear it on close. - Track and cancel the deferred profile read, so a stale one cannot overwrite a newer reading after the identify target changes. - Route the identify popup labels through t(). - Drop placeholder units such as EMIT's "unitless" from the readout and the chart's value label.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/geolibre-desktop/src/hooks/useNetcdfIdentify.ts (1)
116-129: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear stale profile data when a profile read fails.
readNetcdfProfilereturnsnullafter a failed read, but this callback leaves the external store unchanged whenprofileis falsy. After a successful click, a later failed read leaves the previous spectrum visible while the popup shows the new pixel. Clear the reading before scheduling a new profile read and when the callback receivesnull.Proposed fix
if (!state.profile) { setNetcdfProfileReading(null); return; } + setNetcdfProfileReading(null); profileTimeout = window.setTimeout(() => { profileTimeout = null; const profile = readNetcdfProfile(activeLayerId, pixel.row, pixel.column); if (profile) { setNetcdfProfileReading({ layerId: activeLayerId, variable: state.variable, units: state.units, lng: pixel.lng, lat: pixel.lat, profile, }); + } else { + setNetcdfProfileReading(null); } }, 0);🤖 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 `@apps/geolibre-desktop/src/hooks/useNetcdfIdentify.ts` around lines 116 - 129, Update the profile-read flow in the hook containing the setNetcdfProfileReading callback to clear the existing reading before scheduling a new read, and also clear it when readNetcdfProfile returns null. Preserve the successful setNetcdfProfileReading update for valid profiles while ensuring failed reads cannot leave stale spectrum data visible.
🤖 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 `@apps/geolibre-desktop/src/components/panels/NetcdfProfilePanel.tsx`:
- Around line 96-97: Update the axisLabel construction in NetcdfProfilePanel to
pass first.profile.axis.units through displayUnits before interpolating it into
the chart label. Preserve the existing label format while ensuring unitless,
“1”, and “n/a” axis units are normalized consistently with valueUnits.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/hooks/useNetcdfIdentify.ts`:
- Around line 116-129: Update the profile-read flow in the hook containing the
setNetcdfProfileReading callback to clear the existing reading before scheduling
a new read, and also clear it when readNetcdfProfile returns null. Preserve the
successful setNetcdfProfileReading update for valid profiles while ensuring
failed reads cannot leave stale spectrum data visible.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 971c74f6-35eb-4649-84a1-406240657e77
📒 Files selected for processing (6)
apps/geolibre-desktop/src/components/panels/NetcdfProfilePanel.tsxapps/geolibre-desktop/src/components/panels/StylePanel.tsxapps/geolibre-desktop/src/hooks/useNetcdfIdentify.tsapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/netcdf-image-symbology.tspackages/plugins/src/plugins/local-netcdf.ts
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
The core image-compositing math (percentile clim, RGB/colormap compositing, longitude roll + decimation layout, grid-pixel lookup) was traced through carefully and is well covered by the accompanying unit tests; no correctness issues found there. |
Pointing the Cloud source at a raw .nc URL failed, because it only accepted a kerchunk manifest, so a chunked file on object storage had no way in. A direct NetCDF/HDF URL now opens through a worker that mounts it lazily and faults in only the byte ranges a read touches. Opening an EMIT observation file and listing its variables costs ~12 MB of 36 MB; a band plane of the 1.1 GB reflectance cube costs ~135 MB rather than the whole file. The worker imports the reader module directly rather than the package barrel, which reaches maplibre-gl and other modules that touch window while evaluating and would leave the worker never starting. It also announces readiness, so a module that fails to load surfaces as an error instead of a dialog stuck on "Loading...". Reads are sequential synchronous range requests with no readahead, so they are latency-bound; that is the trade for not downloading a cube far larger than the slice wanted.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/geolibre-desktop/src/components/layout/AddNetcdfDialog.tsx (2)
461-504: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the remote base name against a URL that does not parse.
isNetcdfFileUrlaccepts a relative path: it catches thenew URL(...)failure and falls back to string splitting. Line 468 callsnew URL(dataset.url)without that guard. For such an input the constructor throws insidehandleSubmit, and the user gets aTypeErrormessage instead of a layer.Reuse the same tolerant parse used by
isNetcdfFileUrl.🐛 Proposed fix
- const baseName = - (dataset.kind === "local" ? fileName : new URL(dataset.url).pathname) - .split(/[\\/]/) - .pop() || "netcdf"; + const remotePath = (url: string) => { + try { + return new URL(url).pathname; + } catch { + return url.split(/[?#]/)[0]; + } + }; + const baseName = + (dataset.kind === "local" ? fileName : remotePath(dataset.url)) + .split(/[\\/]/) + .pop() || "netcdf";🤖 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 `@apps/geolibre-desktop/src/components/layout/AddNetcdfDialog.tsx` around lines 461 - 504, Update the baseName calculation in handleSubmit to reuse the tolerant URL/path parsing behavior from isNetcdfFileUrl instead of calling new URL(dataset.url) unconditionally. Ensure relative remote paths fall back to string splitting and still produce the bare filename without throwing.
505-564: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winHandle a remote dataset that takes the Zarr path.
The submit branches do not cover one reachable combination:
dataset.kind === "remote"withhasTimeAxis === true.
useImagePathisdataset !== null && !rgbMode && !hasTimeAxis, so it is false for a remote time cube.rgbModeis false when the user keeps single-band mode.- The final branch requires
dataset.kind === "local"to callbuildLayerRefs.No branch runs.
handleSubmitthen reaches Line 577 and closes the dialog withreset(). The user sees the dialog close and no layer is added, with no error.(time, lat, lon)is a common remote NetCDF shape, so this path is easy to hit.Pick one behavior and implement it: bake the selected slice as an image overlay for remote cubes as well, or report an explicit error that a remote time cube is not supported yet.
🐛 Proposed fix (bake the selected slice for remote cubes)
- } else if (dataset.kind === "local") { + } else if (dataset.kind === "remote") { + // The Zarr renderer needs a synchronous store, which a remote + // range-request dataset cannot provide; bake the selected slice + // instead of silently adding nothing. + throw new Error(t("addData.netcdf.errorRemoteTimeCube")); + } else { const built = dataset.file.buildLayerRefs(variable, selector);🤖 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 `@apps/geolibre-desktop/src/components/layout/AddNetcdfDialog.tsx` around lines 505 - 564, Update handleSubmit so the reachable remote time-cube case (dataset.kind === "remote" with hasTimeAxis === true and single-band mode) is handled instead of falling through to reset(). Reuse the existing datasetGrid, bakeNetcdfImage, addImageOverlayLayer, registerNetcdfLayer, and fitBounds flow from the useImagePath branch to bake and add the selected slice for remote datasets, while preserving the existing local buildLayerRefs path.
🤖 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 `@apps/geolibre-desktop/src/hooks/useNetcdfIdentify.ts`:
- Around line 125-140: Update the profile-read flow around cancelProfile and
readNetcdfProfile to use an AbortController: abort the existing controller
before starting a replacement read, pass its signal through readNetcdfProfile,
state.profile.read, and the remote worker, and abort it during cleanup. Preserve
the existing cancelled-result guard while ensuring aborted requests do not
continue remote range work.
In `@apps/geolibre-desktop/src/i18n/locales/en.json`:
- Around line 701-702: Update the kerchunkUrlLabel translation to describe both
accepted kerchunk reference URLs and direct NetCDF/HDF file URLs, aligning it
with kerchunkUrlHelp.
In `@apps/geolibre-desktop/src/lib/netcdf-remote-client.ts`:
- Around line 83-109: Update the ready promise and worker.onerror flow to retain
a startup reject callback, invoke it immediately with the worker failure when
the worker fails before readiness, and clear pending requests as currently done.
Store the readiness timeout handle and cancel it when the ready promise is
resolved, while preserving normal ready-message handling and timeout rejection.
In `@packages/plugins/src/plugins/local-netcdf.ts`:
- Around line 1156-1164: Update openRemoteNetcdf to perform a single-byte HTTP
range preflight before calling Hdf5NetcdfFile.openLazy. Reject unless the
response status is 206 and its Content-Range confirms the requested byte range,
then preserve the existing Web Worker validation and only mount the remote file
after successful validation.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/components/layout/AddNetcdfDialog.tsx`:
- Around line 461-504: Update the baseName calculation in handleSubmit to reuse
the tolerant URL/path parsing behavior from isNetcdfFileUrl instead of calling
new URL(dataset.url) unconditionally. Ensure relative remote paths fall back to
string splitting and still produce the bare filename without throwing.
- Around line 505-564: Update handleSubmit so the reachable remote time-cube
case (dataset.kind === "remote" with hasTimeAxis === true and single-band mode)
is handled instead of falling through to reset(). Reuse the existing
datasetGrid, bakeNetcdfImage, addImageOverlayLayer, registerNetcdfLayer, and
fitBounds flow from the useImagePath branch to bake and add the selected slice
for remote datasets, while preserving the existing local buildLayerRefs path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a3d1f264-1425-40a0-934a-5e277d624450
📒 Files selected for processing (10)
apps/geolibre-desktop/src/components/layout/AddNetcdfDialog.tsxapps/geolibre-desktop/src/hooks/useNetcdfIdentify.tsapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/netcdf-image-symbology.tsapps/geolibre-desktop/src/lib/netcdf-remote-client.tsapps/geolibre-desktop/src/workers/netcdf-remote.worker.tspackages/plugins/package.jsonpackages/plugins/src/index.tspackages/plugins/src/plugins/local-netcdf.tstests/netcdf-remote-client.test.ts
| worker.onerror = (event) => { | ||
| // A worker-level failure (a bad module load) never resolves the in-flight | ||
| // request, so fail them all rather than hanging the dialog. | ||
| const error = new Error(event.message || "The NetCDF reader worker failed."); | ||
| for (const entry of pending.values()) entry.reject(error); | ||
| pending.clear(); | ||
| }; |
There was a problem hiding this comment.
Bug (medium confidence): if the worker crashes/errors after it has already opened successfully (e.g. mid-readGrid, well past the ready-timeout window), this handler rejects only the currently in-flight requests but never calls worker.terminate() — the dead worker (and its ~1GB WASM heap for a large cube) stays around until the file is later closed through normal app lifecycle (dialog reset / layer removal), which may not happen for a while.
Compounding this: send() (below, line ~111) creates a new pending entry with no timeout at all. If the worker silently stalls (not a hard crash — no onerror fires, just hangs) during a later readGrid/readProfile/listAxes call, that promise never settles, and the dialog/panel is stuck indefinitely (e.g. "Adding...") with no error surfaced to the user, since only the initial ready promise has WORKER_READY_TIMEOUT_MS.
Consider terminating the worker in onerror too, and applying a timeout to send()'s requests the same way ready has one.
There was a problem hiding this comment.
Half done in 7e2b635, leaving this open for the second half.
Fixed: onerror now calls worker.terminate() after rejecting pending requests, so a crashed worker no longer holds its WASM heap until the next dialog reset or layer removal. The same commit also lets onerror reject the startup wait, which previously sat out the full 20 s ready timeout because pending is empty before the first send.
Not done — needs a judgement call: a blanket send() timeout. The documented timings for this path are ~20 s for one band plane of a 1.1 GB EMIT scene and ~35 s for a whole-band-axis profile at one pixel (see the note on openRemoteNetcdf), and those are on a good connection — the reads are latency-bound with no readahead. Any timeout short enough to catch a stall would abort legitimate reads on a slow link, and one generous enough to be safe (several minutes) barely improves on the current behaviour. Distinguishing "stalled" from "slow" really wants progress reporting from the worker (bytes faulted in, say) rather than a wall-clock cap, which is more than this PR should take on. Happy to file it as a follow-up if you agree.
| @@ -176,6 +325,9 @@ async function loadH5wasm(): Promise<H5wasmModule> { | |||
| * A local HDF5/NetCDF-4 file backed by h5wasm. | |||
| */ | |||
| class Hdf5NetcdfFile implements LocalNetcdfFile { | |||
There was a problem hiding this comment.
Test coverage gap (high confidence). Hdf5NetcdfFile is the largest class in this diff (~470 lines) and is the backend for this PR's headline scenario — EMIT hyperspectral cubes, NetCDF-4 groups, and the dimension-scale name resolution called out in the PR description ("Dimension names are read from NetCDF-4 dimension scales, so axes show as bands, lat, lon"). But tests/local-netcdf.test.ts only ever opens sample-nc3.nc/sample-nc3-xy.nc (classic NetCDF-3, via Netcdf3File) — every describe/it in that file goes through openLocalNetcdf(fixture("sample-nc3*.nc")). Hdf5NetcdfFile itself (dimension-scale resolution, group traversal, openLazy/remote reading) has zero direct test coverage.
Given the frontend coverage floor is a ratchet (per CLAUDE.md) and this is precisely the path the PR's own test plan exercised manually against a 1.1GB real file rather than in CI, it'd be worth adding a small .h5/.nc4 fixture and at least one Hdf5NetcdfFile-path test (dimension names, a 3-D variable listing) so this doesn't regress silently.
There was a problem hiding this comment.
Agreed and not done — flagging rather than quietly skipping, so leaving this open.
You're right that Hdf5NetcdfFile carries this PR's headline path and has no direct coverage: every case in tests/local-netcdf.test.ts goes through openLocalNetcdf(fixture("sample-nc3*.nc")), i.e. Netcdf3File. Adding an .h5 fixture plus tests for dimension-scale name resolution and group traversal is real work with its own review surface, and it is not a defect in the diff, so I've left it out of a review-response commit rather than expanding this PR by another test module.
Two things this round did add nearby, for what it's worth: six tests for the new assertByteServing range preflight, and two for the remote client's worker startup handling (tests/netcdf-remote-client.test.ts) — so the remote client half is now covered even though the h5wasm backend is not. Coverage currently sits at 82.13% lines / 84.55% branches, above the floors.
Happy to do the .h5 fixture as a follow-up PR if you want it tracked.
| /** Min/max of cell centres, expanded by half the mean cell size. */ | ||
| function centresToEdges(values: ArrayLike<number>): [number, number] { | ||
| let min = Infinity; | ||
| let max = -Infinity; | ||
| for (let i = 0; i < values.length; i++) { | ||
| const v = Number(values[i]); | ||
| if (!Number.isFinite(v)) continue; | ||
| if (v < min) min = v; | ||
| if (v > max) max = v; | ||
| } | ||
| if (!Number.isFinite(min) || !Number.isFinite(max)) return [0, 0]; | ||
| // A one-cell axis has no spacing to derive; leave it as a degenerate point | ||
| // rather than inventing a width. | ||
| const half = values.length > 1 ? (max - min) / (values.length - 1) / 2 : 0; | ||
| return [min - half, max + half]; | ||
| } |
There was a problem hiding this comment.
Edge case (medium confidence): if a coordinate axis is entirely non-finite (NaN/Infinity — e.g. a corrupt file, or a grid whose lat/lon happens to be read from a bad slice), min/max stay at their Infinity/-Infinity seed values, and this falls through to return [0, 0]. gridBounds then returns effectively [0, 0, 0, 0] (clamped), silently placing the layer at Null Island instead of surfacing an error — this feeds directly into appApi.fitBounds?.(image.bounds) in AddNetcdfDialog, so the camera would fly to (0,0) with no indication anything went wrong.
The Zarr-store path (acceptCoordinate/valuesWithin) guards against this before a grid is ever used, but gridBounds/imageLayout.frame() (used by the RGB and colormapped-image paths) call centresToEdges directly with no equivalent all-finite check. Worth considering whether gridBounds should throw/return null on a fully-degenerate axis rather than silently defaulting to the origin.
There was a problem hiding this comment.
Leaving this open — I'd rather you make the call than change an exported function's contract on my own read.
Having traced it: an all-non-finite axis cannot reach gridBounds from either reader. readCoordinate runs every candidate through acceptCoordinate, whose valuesWithin rejects an array containing any non-finite value, so such a file fails with NO_COORDINATES_MESSAGE before a grid is built — as you note for the Zarr path, but it is the same gate on the image path, since both go through readCoordinate. So the Null Island fitBounds needs a direct external call to gridBounds/composeColormappedImage with a fabricated axis.
That leaves a trade: making an exported function throw where it currently returns is a breaking change for any plugin calling it, to guard an input the app's own paths already reject. I've left it as-is. If you'd like it hardened anyway, throwing from centresToEdges when it finds no finite value is a two-line change and I'll push it.
For contrast, the sibling finding — a partially non-finite axis in imageLayout — I did fix (2533d7a), because there the wrong answer was a silently mirrored raster rather than an obviously-wrong extent.
|
All inline comments are posted. Here's the final summary. Code reviewBugs
Performance
Quality
Security
CLAUDE.md
|
- Give the NetCDF identify popup MapCanvas's `geolibre-identify-popup` class. Without it MapLibre's own always-white `.maplibregl-popup-content` survives while the rows inherit the theme foreground, so the readout was white-on-white in dark mode (reported separately from the bot findings). - Re-run `useNetcdfIdentify`'s effect on `mapReadyGeneration`. A ref's `.current` becoming non-null does not retrigger an effect, so an identify target set before the map finished loading never got a click handler. - Warm a colormap before baking a NetCDF image, in both the Add dialog and the Style panel. Sprite ramps resolve asynchronously and nothing re-bakes when one lands, so picking an unwarmed ramp persisted as viridis pixels under the chosen name, with no self-healing path. - Reset `pendingSymbology` when the Style panel switches NetCDF layers, and seed the clim fields from the new layer's own record rather than from the dropped pending value. - Normalize the profile chart's axis units through `displayUnits`, so a coordinate variable declaring `unitless`/`1` no longer prints that in the axis label. - Derive image row/column orientation from a coordinate axis' first and last *finite* values. A `NaN` endpoint made every comparison false, reading a descending axis as ascending and mirroring the raster under a `gridBounds` extent that still came out right. Covered by a regression test. - Fail `openRemoteNetcdfFile`'s startup wait from `worker.onerror`, and release the readiness timer once ready arrives. `pending` is empty before the first send, so a module-load failure sat out the whole 20 s timeout. - Preflight a one-byte range request in `openRemoteNetcdf` before mounting. A server that ignores `Range` answers whole, which emscripten accepts, turning a lazy mount into a blocking full download with no error. Gated on the 206 status, which CORS cannot hide, with `Content-Range` checked only when exposed. - Widen `kerchunkUrlLabel` to match the field's help text, which now documents direct NetCDF/HDF URLs as well as kerchunk references. - Document why the identify readout and the profile chart format values at different precision, rather than merging two helpers with different jobs.
Findings from the review summary that were not attached to an inline thread. - Order `selectVariable`'s reads per call. `opGen` only moves when the dialog resets or its source changes, so it cannot order two reads of the same dataset: switching variables while a read was outstanding let whichever finished last seed the axes and RGB defaults, from a variable the user had already moved off. - Clear only the identify target's own profile readings on an off-grid click, via a new `clearNetcdfProfileReadingsForLayer`. The list holds one layer at a time, so clearing it wholesale took another layer's still-charted readings with it whenever the user switched targets before landing a hit on the new one. - Terminate the worker in `netcdf-remote-client`'s `onerror`. Nothing can be read through it afterwards — every later `send` would wait on a reply that cannot come — and it owns a lazy-filesystem mount. - Spread the *current* layer record in `NetcdfSymbologySection.apply`'s deferred write rather than the one captured at render, so a concurrent edit to the same layer inside that window is no longer silently reverted. Awaiting the colormap warm widened that window, which is what made this worth fixing now. - Drop the dead `LocalNetcdfImage` type import. Left `gridBounds` alone: an all-non-finite axis cannot reach it from either reader, because `acceptCoordinate` rejects a coordinate array containing any non-finite value (`valuesWithin`) and throws `NO_COORDINATES_MESSAGE` first. The Null Island bounds would need a direct external call, and making an exported function throw is the riskier change.
|
Good, the inline comment tool is loaded and ready to use once the reviews land. |
Summary
bands, lat, lonrather thandim_0, dim_1, dim_2.imageoverlays. This also sidesteps an upstream bug in@carbonplan/zarr-layer0.7.0, whoseshift_xuniform lookup throws on drivers that eliminate it (Mesa, so most Linux Intel/AMD machines), leaving Zarr layers permanently blank. Cloud kerchunk layers and cubes with a time axis stay on the Zarr renderer so the Time Slider can still drive them.useColormapRampshook. A new NetCDF symbology section in the Style panel exposes colormap, reverse, and color limits after the fact.rgb(r, g, b)strings and parsed to black by every hex-based consumer, including the raster panel's custom-ramp seeding.Test plan
npm run test:frontend), with new cases for grid bounds, percentile color limits, RGB composition, colormapped images, dimension axes, and ramp color normalizationpre-commit run --files <changed>clean (oxfmt, eslint, npm build)(lat, lon), layer fits the map, renders with the selected ramp, "Zoom to layer" returns to the extentSummary by CodeRabbit
New Features
Improvements