fix(map): sync basemap visibility/opacity from the layer control to the store#464
fix(map): sync basemap visibility/opacity from the layer control to the store#464giswqs wants to merge 4 commits into
Conversation
…he store The maplibre-gl-layer-control Background (basemap) group toggled the basemap internally without writing back to the store, so the basemap visibility icon and opacity slider in the left layer panel did not update when the basemap was toggled from the layer control on the right (issue #450). Per-layer toggles already round-trip through the store via the custom-layer adapter; the Background group had no such path. Wire the new onBackgroundVisibilityChange / onBackgroundOpacityChange callbacks (maplibre-gl-layer-control >=0.17.2) to setBasemapVisible / setBasemapOpacity so the store stays the single source of truth and external basemap UI stays in sync. Bump the dependency to ^0.17.2. Fixes #450
✅ 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 selected for processing (2)
📝 WalkthroughWalkthroughIn ChangesLayerControl Background State Sync
E2E Save Dialog Fallback Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewBugs
Quality
Performance
Checked and found clean
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…y-icon-sync # Conflicts: # packages/map/package.json
Code reviewSummaryThe change is small, focused, and correct. The root cause analysis in the PR description is accurate: the right-panel layer control was toggling the basemap purely internally without writing back to the store, so the left panel's visibility icon and opacity slider fell out of sync. Wiring FindingsQuality
CLAUDE.md
What I checked
|
- Move onBackgroundVisibilityChange / onBackgroundOpacityChange after the ...layerControlConfig spread so the wired basemap callbacks can never be silently overridden by a future key in the config object.
| onBackgroundOpacityChange: (opacity) => { | ||
| useAppStore.getState().setBasemapOpacity(opacity); | ||
| }, |
There was a problem hiding this comment.
The opacity value coming from the control is passed straight to the store without any bounds check. setBasemapPaintOpacity handles >= 1 by restoring the original paint value, but if the library ever emits a value below 0 (floating-point edge at the slider boundary), original * this.basemapOpacity would produce a negative opacity — an invalid MapLibre paint value. setLayerOpacity for regular layers has the same gap, so this is consistent, but since we're adding a new path it's a good moment to add a clamp:
| onBackgroundOpacityChange: (opacity) => { | |
| useAppStore.getState().setBasemapOpacity(opacity); | |
| }, | |
| onBackgroundOpacityChange: (opacity) => { | |
| useAppStore.getState().setBasemapOpacity(Math.min(1, Math.max(0, opacity))); | |
| }, |
Code reviewThe change is small and well-reasoned. I traced the full round-trip and checked the surrounding context. Here is what I found. BugsNone confirmed. The feedback-loop concern raised in the PR description was verified: SecurityNone. PerformanceNone beyond the harmless double-write noted above. QualityOpacity value not clamped — low confidence. PR description / package.json discrepancy — informational. The description says "Dependency bumped to CLAUDE.mdNo violations. The change is confined to |
#458 made Save prompt for a file name (via a dialog) in browsers without the File System Access picker — which these specs delete in addInitScript. The save-and-reopen specs clicked Project → Save and waited for a download that never fired, because the name-prompt dialog now intercepts the save, so both tests hit the 60s timeout (failing on main too). Accept the pre-filled default name and confirm the dialog so the download proceeds.
| onBackgroundVisibilityChange: (visible) => { | ||
| useAppStore.getState().setBasemapVisible(visible); | ||
| }, | ||
| onBackgroundOpacityChange: (opacity) => { | ||
| useAppStore.getState().setBasemapOpacity(opacity); | ||
| }, |
There was a problem hiding this comment.
The callbacks are correct and the "no feedback loop" claim holds: updateLayerControlItem sets checkbox.checked / opacity.value via direct property assignment, which does not dispatch change events, so the handler never re-enters.
One minor inefficiency worth being aware of: after the callback fires, the store update causes React to call controller.setBasemapVisible / setBasemapOpacity, which calls applyBasemapVisibility / applyBasemapOpacity again. The library has already applied the change to MapLibre at that point, so this is a redundant (but idempotent) pass over all basemap paint properties. syncLayerControlState() is also re-invoked, iterating all user layers via syncLayerControlLayerStates even though only the background changed. None of this is incorrect — just slightly wasteful on every slider tick or toggle. A guard like early-returning from setBasemapVisible / setBasemapOpacity when the value hasn't changed would avoid it, though the current behaviour is safe.
Also: the comment says the callbacks are placed after the spread "so these wired callbacks always win," implying layerControlConfig could contain competing callbacks. LayerControlConfig is currently typed with only excludeLayers and customLayerAdapters, so there is no real risk — the placement is a forward-compatibility precaution rather than fixing an actual conflict. The comment is accurate but slightly overstated; a future reader adding callbacks to LayerControlConfig might not realise they'd be silently overridden here.
| // Browsers without the File System Access picker (deleted above) prompt for a | ||
| // file name before downloading; accept the pre-filled default and confirm. | ||
| await page | ||
| .getByRole("dialog") | ||
| .getByRole("button", { name: "Save", exact: true }) | ||
| .click(); |
There was a problem hiding this comment.
The pattern is correct — waitForEvent("download") is registered before the menu click so the listener is ready, and the dialog button click gates the actual download, so there's no race.
Small brittleness note: { name: "Save", exact: true } inside the dialog is a hardcoded label. If the button text is ever changed (e.g., translated, renamed to "OK" or "Confirm"), the locator will silently time out instead of giving a clear failure. The same applies to the parallel change in storymap.spec.ts. Not a current bug — just worth noting for future-proofing.
Also, page.getByRole("dialog") will match any open dialog. At this point in the test the only open dialog should be the filename prompt, but if the selector were to match an unexpected dialog (e.g., an error toast styled as a dialog), the click would fail non-obviously. A { name: /save/i } constraint on the dialog role, or a dedicated data-testid, would make the selector tighter — though given the controlled test environment this is low priority.
| // Browsers without the File System Access picker (deleted above) prompt for a | ||
| // file name before downloading; accept the pre-filled default and confirm. | ||
| await page | ||
| .getByRole("dialog") | ||
| .getByRole("button", { name: "Save", exact: true }) | ||
| .click(); |
There was a problem hiding this comment.
Same brittleness as the parallel change in layer-groups.spec.ts: { name: "Save", exact: true } inside a generic getByRole("dialog") is a hardcoded label and an unscoped dialog selector. Both are low-risk given the isolated test environment, but worth keeping in mind if these tests become flaky in CI or if the UI gets internationalised.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
1 participant
Summary
Fixes #450 — the basemap visibility icon (and opacity slider) in the left layer panel did not update when the basemap was toggled from the layer control on the right.
Root cause
The basemap Background group is rendered in two independent places:
basemapVisible/basemapOpacity.maplibre-gl-layer-control) toggled the Background group entirely internally — it mutated the MapLibre style and its own checkbox/slider but never wrote back to the store, and exposed no callback for it.So right→left never synced. Per-layer toggles worked because they round-trip through the store via the custom-layer adapter (
setVisibility/setOpacity); the Background group had no equivalent path.Fix
Upstream adds two callbacks (opengeos/maplibre-gl-layer-control#59, released in 0.17.2):
onBackgroundVisibilityChangeandonBackgroundOpacityChange. This PR wires them inMapController.addLayerControl()tosetBasemapVisible/setBasemapOpacity, so the store remains the single source of truth and the left panel stays in sync. Dependency bumped to^0.17.2.No feedback loop: the store write triggers
MapCanvas→controller.setBasemapVisible→syncLayerControlBackgroundState, which writes the control's internal state directly (it does not re-enter the user-driven handler that fires the callback).Verification
Driven in the real app with Playwright:
0 console errors. The
mappackage typechecks cleanly with the change.Dependency / CI note
maplibre-gl-layer-control@^0.17.2published to npm (upstream PR feat: notify consumers when the Background group is toggled maplibre-gl-layer-control#59) for CI to resolve the dependency. Verified locally against a 0.17.2 build.npm run build/ pre-commitnpm-buildhook currently fails onmainfor unrelated, pre-existing reasons in files this PR does not touch:maplibre-gl-geo-editorspatialExtensionPath(from the merged fix(vector): forward offline spatial-extension path to the Add Vector panel #446) is not yet in any publishedgeo-editorversion, andmaplibre-gl-basemap-controlneeds a fresh install (0.4.0). These are independent of this change.Summary by CodeRabbit