feat(colorbar): stack-order fix, stacking direction, and resizable panel (#884) - #901
Conversation
…884) Bump maplibre-gl-components to 0.25.2, which groups colorbars sharing a corner into one control so updating a colorbar no longer jumps it to the top of the stack, and adds a stacking-direction option (vertical or horizontal) for multiple colorbars. Mirror the new stackOrientation field in the saved project state so the choice round-trips on reopen. Fixes #884
✅ Deploy Preview for geolibre-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe colorbar state now includes ChangesColorbar stack orientation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
⚡ Cloudflare Pages preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewThis is a focused, clean change — a version bump to pick up an upstream bug fix plus the minimal GeoLibre-side plumbing to expose the new BugsNothing found. The normalization SecurityNothing found. This is pure data serialization; no user-controlled input flows to any execution path that could cause injection. PerformanceNothing found. Quality
CLAUDE.md
|
Export normalizeColorbarState and add unit tests that a horizontal stack choice persists, missing/unknown values fall back to vertical, and the state round-trips cleanly. Addresses review feedback on PR #901.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall the implementation is sound. The core fix (package bump + |
- Add an explicit "vertical" pass-through test so the two valid stackOrientation values are isolated (a `=== "vertical"` typo would otherwise pass every existing case). - Use a non-empty customColors in the round-trip input so the normalizer's empty-string substitution no longer obscures the intent. - Trim the file header comment to a concise three lines.
Code reviewOverall this is a clean, well-scoped fix. The normalization logic is correct, backward-compat defaulting is solid, and the round-trip idempotency test is a nice touch. Four findings follow, ranging from a medium-confidence correctness note to minor nits. Bugs / Correctness
Quality
Security / Performance / CLAUDE.mdNothing to raise. The |
- Add an @internal JSDoc tag to normalizeColorbarState clarifying it is exported only for unit testing. - Add a test covering null/undefined/non-object input (the early-return branch). - Drop the file header comment; the describe label and test names cover it.
| } | ||
|
|
||
| function normalizeColorbarState( | ||
| /** @internal Exported only so the project-state normalizer can be unit-tested. */ |
There was a problem hiding this comment.
Minor wording nit: normalizeColorbarState is one of the project-state normalizers, so the phrase "so the project-state normalizer can be unit-tested" is slightly circular. Consider:
| /** @internal Exported only so the project-state normalizer can be unit-tested. */ | |
| /** @internal Exported only for unit testing. */ |
There was a problem hiding this comment.
Leaving this open for a maintainer call. These three round-3 comments are all subjective style nits (wording of an @internal note, splitting one assertion group into separate it() blocks, and an informational observation about idempotent deepEqual) with no functional or correctness impact. The prior round's comment-block guidance also reversed itself between rounds, so I'm stopping here rather than churning the branch on marginal stylistic preferences. Happy to apply any of these if you'd like.
| it("returns undefined for null/undefined/non-object input", () => { | ||
| assert.equal(normalizeColorbarState(null), undefined); | ||
| assert.equal(normalizeColorbarState(undefined), undefined); | ||
| assert.equal(normalizeColorbarState("nope"), undefined); | ||
| }); |
There was a problem hiding this comment.
Nit (test quality): three independent assertions in one it() means a failure message identifies the test but not the failing case. Splitting each into its own it() gives a more precise failure signal:
| it("returns undefined for null/undefined/non-object input", () => { | |
| assert.equal(normalizeColorbarState(null), undefined); | |
| assert.equal(normalizeColorbarState(undefined), undefined); | |
| assert.equal(normalizeColorbarState("nope"), undefined); | |
| }); | |
| it("returns undefined for null input", () => { | |
| assert.equal(normalizeColorbarState(null), undefined); | |
| }); | |
| it("returns undefined for undefined input", () => { | |
| assert.equal(normalizeColorbarState(undefined), undefined); | |
| }); | |
| it("returns undefined for non-object input", () => { | |
| assert.equal(normalizeColorbarState("nope"), undefined); | |
| }); |
There was a problem hiding this comment.
Leaving this open for a maintainer call. These three round-3 comments are all subjective style nits (wording of an @internal note, splitting one assertion group into separate it() blocks, and an informational observation about idempotent deepEqual) with no functional or correctness impact. The prior round's comment-block guidance also reversed itself between rounds, so I'm stopping here rather than churning the branch on marginal stylistic preferences. Happy to apply any of these if you'd like.
| it("round-trips a horizontal choice through a second normalization", () => { | ||
| const once = normalizeColorbarState({ | ||
| visible: true, | ||
| colorbars: [ | ||
| { | ||
| mode: "named", | ||
| colormap: "viridis", | ||
| customColors: "#440154, #31688e, #21918c, #90d743, #fde725", | ||
| vmin: 0, | ||
| vmax: 100, | ||
| label: "Depth", | ||
| units: "", | ||
| orientation: "vertical", | ||
| colorbarPosition: "bottom-right", | ||
| }, | ||
| ], | ||
| stackOrientation: "horizontal", | ||
| }); | ||
| const twice = normalizeColorbarState(once); | ||
| assert.equal(twice?.stackOrientation, "horizontal"); | ||
| assert.deepEqual(twice, once); | ||
| }); |
There was a problem hiding this comment.
assert.deepEqual(twice, once) passes correctly (idempotent), but is slightly surprising on first read: the top-level form fields (mode, colormap, label, vmin, vmax, etc.) in once are set to their defaults — not derived from colorbars[0] — because the original input has no top-level form fields. So once.label === "" even though colorbars[0].label === "Depth".
The test is correct, but a short inline comment would help future readers understand why deepEqual holds without surprise:
| it("round-trips a horizontal choice through a second normalization", () => { | |
| const once = normalizeColorbarState({ | |
| visible: true, | |
| colorbars: [ | |
| { | |
| mode: "named", | |
| colormap: "viridis", | |
| customColors: "#440154, #31688e, #21918c, #90d743, #fde725", | |
| vmin: 0, | |
| vmax: 100, | |
| label: "Depth", | |
| units: "", | |
| orientation: "vertical", | |
| colorbarPosition: "bottom-right", | |
| }, | |
| ], | |
| stackOrientation: "horizontal", | |
| }); | |
| const twice = normalizeColorbarState(once); | |
| assert.equal(twice?.stackOrientation, "horizontal"); | |
| assert.deepEqual(twice, once); | |
| }); | |
| it("round-trips a horizontal choice through a second normalization", () => { | |
| // Top-level form fields in `once` come from defaults (the input has no top-level | |
| // mode/label/etc.), so `once.label` will be "" regardless of colorbars[0].label. | |
| // The deepEqual check verifies that normalizeColorbarState is idempotent once the | |
| // state is already normalized. | |
| const once = normalizeColorbarState({ | |
| visible: true, | |
| colorbars: [ | |
| { | |
| mode: "named", | |
| colormap: "viridis", | |
| customColors: "#440154, #31688e, #21918c, #90d743, #fde725", | |
| vmin: 0, | |
| vmax: 100, | |
| label: "Depth", | |
| units: "", | |
| orientation: "vertical", | |
| colorbarPosition: "bottom-right", | |
| }, | |
| ], | |
| stackOrientation: "horizontal", | |
| }); | |
| const twice = normalizeColorbarState(once); | |
| assert.equal(twice?.stackOrientation, "horizontal"); | |
| assert.deepEqual(twice, once); | |
| }); |
There was a problem hiding this comment.
Leaving this open for a maintainer call. These three round-3 comments are all subjective style nits (wording of an @internal note, splitting one assertion group into separate it() blocks, and an informational observation about idempotent deepEqual) with no functional or correctness impact. The prior round's comment-block guidance also reversed itself between rounds, so I'm stopping here rather than churning the branch on marginal stylistic preferences. Happy to apply any of these if you'd like.
Code reviewReviewed the dependency bump to BugsNone found. The normalization logic is correct: SecurityNone. No user input reaches PerformanceNone. The change is additive (one extra string field in a state object); no new hot paths. Quality
CLAUDE.mdThe new test is picked up by |
Bump maplibre-gl-components to 0.25.3, which adds two bottom-corner resize grips to the colorbar GUI panel (matching the HTML and data panels). Drag either grip to grow the panel toward the map interior.
giswqs
changed the title
| it("coerces an unknown stack orientation to vertical", () => { | ||
| const normalized = normalizeColorbarState({ | ||
| visible: true, | ||
| colorbars: [], | ||
| stackOrientation: "diagonal", | ||
| }); | ||
| assert.equal(normalized?.stackOrientation, "vertical"); | ||
| }); |
There was a problem hiding this comment.
The "coerces unknown" test only covers a string value ("diagonal"). Project files are arbitrary JSON, so stackOrientation could also arrive as a number, boolean, or array. Since the normalization uses strict equality (=== "horizontal"), these all correctly fall through to "vertical" — but a quick additional case would make that intent explicit and guard against a future refactor that adds || typeof candidate.stackOrientation === "number" etc.
| it("coerces an unknown stack orientation to vertical", () => { | |
| const normalized = normalizeColorbarState({ | |
| visible: true, | |
| colorbars: [], | |
| stackOrientation: "diagonal", | |
| }); | |
| assert.equal(normalized?.stackOrientation, "vertical"); | |
| }); | |
| it("coerces an unknown stack orientation to vertical", () => { | |
| for (const bad of ["diagonal", 42, true, null, [], {}]) { | |
| const normalized = normalizeColorbarState({ | |
| visible: true, | |
| colorbars: [], | |
| stackOrientation: bad, | |
| }); | |
| assert.equal(normalized?.stackOrientation, "vertical", `expected vertical for ${JSON.stringify(bad)}`); | |
| } | |
| }); |
There was a problem hiding this comment.
Leaving open for a maintainer call. The code already handles non-string values correctly via strict === "horizontal" (number/boolean/array all fall through to vertical), so this is a marginal coverage nit rather than a defect. I'm holding off on more test-only churn here since the review has converged to stylistic nits on this file across several rounds.
| it("round-trips a horizontal choice through a second normalization", () => { | ||
| const once = normalizeColorbarState({ | ||
| visible: true, | ||
| colorbars: [ | ||
| { | ||
| mode: "named", | ||
| colormap: "viridis", | ||
| customColors: "#440154, #31688e, #21918c, #90d743, #fde725", | ||
| vmin: 0, | ||
| vmax: 100, | ||
| label: "Depth", | ||
| units: "", | ||
| orientation: "vertical", | ||
| colorbarPosition: "bottom-right", | ||
| }, | ||
| ], | ||
| stackOrientation: "horizontal", | ||
| }); | ||
| const twice = normalizeColorbarState(once); | ||
| assert.equal(twice?.stackOrientation, "horizontal"); | ||
| assert.deepEqual(twice, once); | ||
| }); |
There was a problem hiding this comment.
The assert.deepEqual(twice, once) at the end is a useful idempotency check. One thing to be aware of: the input to the first normalization has no top-level entry-level fields (mode, colormap, etc.), so once gets these from defaults via normalizeColorbarEntry. The deep-equal passes because normalizeColorbarEntry(once) reads those same default values back from once and produces an identical object.
If ComponentColorbarGuiState ever gains a computed/ephemeral field (e.g. a _dirty flag set only by setState), this assertion could start failing for unrelated reasons. It's a minor fragility. If idempotency is the goal, targeted field assertions (assert.equal(twice?.colorbars[0]?.colormap, once?.colorbars[0]?.colormap) etc.) are more resilient — though the current broad check is a net positive for catching regressions.
There was a problem hiding this comment.
Acknowledged, and it's intentional: the test is an idempotency check on already-normalized state. The top-level form fields come from defaults because the input has no top-level fields, and deepEqual(twice, once) verifies normalization is stable. Leaving open for your call rather than reworking the test further.
Code reviewReviewed the diff (81 additions, 11 deletions across 5 files): dependency bump, BugsNone found. The normalization logic SecurityNone found. PerformanceNone found. The version bump is a minor feature/bug-fix release with no reported performance changes. Quality
CLAUDE.md
Overall the PR is clean and well-scoped. The normalization is correct, backward-compat, and well-tested. The two inline notes are minor nits. |
1 participant
Summary
Colorbar plugin improvements from #884, plus a resizable config panel:
Issue 1 (separate Name vs Label field) was scoped by the reporter to a later release and is not included.
How
All three live in the upstream
maplibre-gl-componentspackage:This PR bumps
maplibre-gl-componentsto^0.25.3inapps/geolibre-desktopandpackages/plugins, and mirrors the newstackOrientationfield in the saved colorbar project state so a horizontal choice round-trips on reopen.Testing
npm run buildand scopedpre-commit run --files ...(incl. the npm-build hook) pass; addedtests/colorbar-state-normalization.test.tsfor the stackOrientation round-trip.Fixes #884
Summary by CodeRabbit
stackOrientationnormalization, including backward-compatible defaults, invalid-value coercion, and idempotency.^0.25.3.