fix(layout): persist Browser and Comments panel toggles - #1940
Conversation
The Settings -> Layout toggles for the Browser and Comments right panels acted only on the live right-panel registry. Nothing was written to `desktopSettings.layout`, and both registration hooks unconditionally opened their panel on mount, so every launch reopened a panel the user had turned off. Startup mode made no difference: reopening the last project restores layers and the camera, never these panels. Both panels now have a persisted `layout.browserPanelVisible` / `layout.commentsPanelVisible` setting (defaulting to on, so settings saved before the keys existed keep today's behavior), the registration hooks seed from it, and the Settings toggles write it. The toggles apply live rather than on Save, so they patch the dialog draft as well; the draft is snapshotted when the dialog opens and Save writes it wholesale, which would otherwise revert the toggle the user just made. Reset moves the panels too, since those two rows render the live registry state. Fixes #1935
|
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 (6)
📝 WalkthroughWalkthroughChangesPanel visibility persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change persists the Browser and Comments panel visibility settings while preserving existing defaults and behavior; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant SettingsDialog
participant DesktopSettingsStore
participant PanelRegistration
participant PanelRegistry
SettingsDialog->>DesktopSettingsStore: save Browser or Comments visibility
SettingsDialog->>PanelRegistry: apply live panel visibility
PanelRegistration->>DesktopSettingsStore: read persisted visibility on startup
PanelRegistration->>PanelRegistry: register panel with persisted state
PanelRegistry->>DesktopSettingsStore: persist user close or reopen action
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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}
🔍 Cloudflare PR preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const resetLayoutSettings = () => { | ||
| updateDraftLayoutSettings(DEFAULT_DESKTOP_LAYOUT_SETTINGS); | ||
| // The Browser/Comments checkboxes render the live registry state, not the | ||
| // draft, so reset has to move the panels themselves or those two rows would | ||
| // ignore the button. | ||
| toggleBrowserPanel(DEFAULT_DESKTOP_LAYOUT_SETTINGS.browserPanelVisible); | ||
| toggleCommentsPanel(DEFAULT_DESKTOP_LAYOUT_SETTINGS.commentsPanelVisible); | ||
| }; |
There was a problem hiding this comment.
resetLayoutSettings writes browserPanelVisible/commentsPanelVisible straight to the persisted store via toggleBrowserPanel/toggleCommentsPanel (through applyPanelVisibility → updateSavedLayoutSettings), while the other four layout fields only land in draftDesktopSettings and are not persisted until saveSettings runs.
That means clicking Reset and then Cancel produces a partial reset: the two panel-visibility settings are permanently changed (survive the dialog close), but layerPanelVisible/stylePanelVisible/toolbarLabels/showProjectInfo silently revert to whatever was last saved, since the draft is discarded and re-seeded from the store the next time the dialog opens (line ~620). A user who resets and then backs out would end up with a half-applied reset with no obvious indication of that split.
Confidence: medium — this follows directly from the code, but it may be an accepted trade-off of the "panels apply live" design described in the PR.
| // Browser and Comments are dockable right panels, so their checkboxes read the | ||
| // live registry state (the user can also close them from their own header) | ||
| // while the toggle writes the matching persisted layout setting. The setting | ||
| // is what their registration hooks seed from on the next launch, so the | ||
| // toggle survives a restart (#1935). | ||
| const rightPanelState = useRightPanelState(); | ||
| const browserPanelOpen = rightPanelState.visibleIds.includes(BROWSER_PANEL_ID); | ||
| const commentsPanelOpen = rightPanelState.visibleIds.includes(COMMENTS_PANEL_ID); | ||
| // These apply live rather than on Save, so the draft is patched alongside the | ||
| // saved settings: the draft was snapshotted when the dialog opened, and Save | ||
| // writes it wholesale, which would otherwise revert the toggle the user just | ||
| // made in this same dialog. | ||
| const applyPanelVisibility = ( | ||
| key: "browserPanelVisible" | "commentsPanelVisible", | ||
| visible: boolean, | ||
| ) => { | ||
| updateSavedLayoutSettings({ [key]: visible }); | ||
| updateDraftLayoutSettings({ [key]: visible }); | ||
| }; |
There was a problem hiding this comment.
The Browser/Comments checkboxes are bound to browserPanelOpen/commentsPanelOpen, which read the live right-panel registry (rightPanelState.visibleIds), not the draft settings. That registry can also change from outside this dialog — e.g. the user closes the panel from its own header, which is explicitly designed to be session-only and not written back to layout.browserPanelVisible/commentsPanelVisible (per the PR description).
Net effect: if a user closes a panel from its header, then opens Settings → Layout, the checkbox shows unchecked (matching the live state) even though the persisted setting is still true. If they then click Save Settings without touching that checkbox, the save writes the untouched draft value (true), so the panel reopens on the next launch — silently contradicting what the checkbox displayed in the dialog they just saved from.
This looks intentional per the PR's stated design, but it's a real display/persistence divergence worth confirming is the desired UX. Confidence: low-medium.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Review follow-up. The Browser/Comments rows in the Layout dialog applied live while the other four rows waited for Save, which left two seams: Reset followed by Cancel stuck those two flags while reverting the rest, and the rows rendered live registry state that Save did not write, so closing a panel from its own header showed an unchecked box the dialog then saved back as visible. Both rows are now draft-backed like their neighbours. The draft seeds them from the live registry when the dialog opens, so a panel closed from its header still shows unchecked and Save persists exactly what was displayed; Reset and Cancel behave as they do for every other row. Save applies the committed values to the registry, skipping panels already in the requested state so saving an untouched dialog cannot collapse an expanded panel. The Settings dropdown keeps applying on the spot and persisting in one step, since it has no Save to wait for.
|
Both review findings were real and are fixed in c0678c2. The underlying cause of each was the same: I had left the Browser/Comments rows in the Layout dialog applying live while the other four rows waited for Save, so they sat outside that dialog's draft/Save/Cancel contract. Both rows are now draft-backed like their neighbours:
Save applies the committed values to the registry, guarded on The Settings dropdown toggles are unchanged: they have no Save to wait for, so they move the panel and persist the preference in one step, matching the other live entries in that menu. On the test-coverage note: agreed that the interactive logic is untested, and it is the existing convention here that these dialogs are covered through the normalize layer. The behaviors above were verified by driving the built app instead, in both dark and light themes. Full frontend suite (6040 passing), build, and pre-commit are green. |
| // Browser and Comments are dockable panels the user can also close from the | ||
| // panel's own header, which is a session action that writes no setting. Seed | ||
| // those two rows from the live registry rather than the stored value so the | ||
| // dialog opens showing what is actually on screen; Save then persists | ||
| // exactly what the checkboxes showed. | ||
| seededSettings.layout = { | ||
| ...seededSettings.layout, | ||
| browserPanelVisible: isRightPanelVisible(BROWSER_PANEL_ID), | ||
| commentsPanelVisible: isRightPanelVisible(COMMENTS_PANEL_ID), | ||
| }; | ||
| setDraftDesktopSettings(seededSettings); |
There was a problem hiding this comment.
This seeds the draft's browserPanelVisible/commentsPanelVisible from the live registry (isRightPanelVisible) rather than from the persisted desktopSettings.layout. That's intentional for showing "what's on screen" when the Layout tab is visited, but it has a side effect: saveSettings() (line ~1226) writes layout: draftDesktopSettings.layout wholesale on every Save, for any section of the dialog.
So the flow "user closes Browser/Comments from the panel's own header (a session-only action per the docstrings in useRegisterBrowserPanel.ts/useRegisterCommentsPanel.ts, which explicitly say this 'is not written back as a preference') → later opens Settings for something unrelated (e.g. map preferences, language) → clicks Save" will silently persist the panel as permanently hidden, even though the user never touched the Layout tab or these checkboxes. That contradicts the stated design intent and could surprise a user who intended the closure to be session-only.
Confidence: medium — this is a real, reachable path (PluginRightPanel.tsx's header close button calls closeRightPanel), but whether it's considered a bug vs. accepted "what's on screen becomes what's saved" behavior is a product-intent question worth confirming.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
Review follow-up. Seeding the dialog draft from the live registry fixed the checkbox lying about what Save would write, but it moved the seam rather than closing it: a panel closed from its own header stayed a session-only state that an unrelated Save then converted into a permanent preference. Both directions now go through one place. `registerPersistedRightPanel` seeds the panel from its setting at registration and subscribes to the registry afterwards, writing every later visibility change back. For these panels closing is not a transient collapse, it removes the rail entry entirely and only Settings can restore it, so it is a preference however it was reached. With the setting and the registry always in step, the checkbox cannot disagree with either, and there is no session-only state left for a Save to silently promote. Two registry events are deliberately not mirrored: the emit from registration (the panel is legitimately not visible yet, so the subscription is attached after the seed) and the emit from unregistering on unmount, which would otherwise persist false on every teardown. Being displaced by another panel is not a close, so it writes nothing. The dialog rows keep the draft semantics of the previous commit, and Save re-applies them to the registry, skipping a panel already in the requested state. `tests/persisted-right-panel.test.ts` covers the seed, the header close, the collapse and displacement non-events, and the teardown guard; the module imports the registry subpath so it stays a leaf the suite can load without the plugin barrel.
|
Good catch, and it is the right read: seeding the draft from the live registry closed the previous round's finding but only moved the seam. A panel closed from its own header was still session-only state that an unrelated Save then promoted to a permanent preference. Fixed in 66280e0 by removing the session-only state rather than trying to guard the Save path. The new Two registry events are deliberately not mirrored, both called out in the module doc:
Being displaced by another panel is not a close (the registry keeps a displaced panel in On the coverage point, which was fair on both rounds: Verified in the built app in both themes: header close persists and survives reload; the dropdown and dialog checkboxes agree; Reset then Cancel persists nothing; Reset then Save reopens and persists; expanding does not flip the setting and an unrelated Save leaves an expanded panel expanded. 6050 frontend tests passing, build and pre-commit green. |
| const unsubscribe = subscribeRightPanels(() => { | ||
| if (!getRightPanel(registration.id)) return; | ||
| setPanelVisibleInSettings(key, isRightPanelVisible(registration.id)); | ||
| }); |
There was a problem hiding this comment.
Minor/low confidence: subscribeRightPanels fires on every registry mutation (any panel opening/closing/moving dock, not just this one), so this callback re-reads and attempts to persist this panel's own visibility on each such event. It's harmless today because setPanelVisibleInSettings bails when the value already matches, but it does mean unrelated churn in the panel registry (e.g. a plugin panel changing dock) triggers redundant getRightPanel/isRightPanelVisible lookups and a store-equality check for both Browser and Comments each time. Not worth blocking on, just flagging as a spot to scope the subscription if this ever becomes hot (e.g. frequent dock dragging).
Code reviewI traced the full data flow: the new Bugs: None found with reasonable confidence. Security: No issues — this is local UI/layout preference state with no external input. Performance:
Quality:
CLAUDE.md: No violations found — the leaf-module export pattern, strict-boolean settings normalization, and test placement all match existing conventions in this repo. Overall this is a well-scoped, carefully reasoned fix with solid test coverage for the seed/mirror/round-trip/back-compat behaviors; I only had one very minor, low-confidence efficiency note to flag inline. |
Fixes #1935
Problem
The Settings → Layout toggles for the Browser and Comments panels acted only on the live right-panel registry. Nothing was written to
desktopSettings.layout, and both registration hooks unconditionally calledopenRightPanel(...)on mount, so every launch reopened a panel the user had turned off.The startup mode was a red herring: "Reopen the last project" restores layers and the camera, never these two panels. The other Layout settings (Layers panel, Style panel, toolbar labels, project info) were already persisted and did survive a restart.
Fix
DesktopLayoutSettingsgainsbrowserPanelVisibleandcommentsPanelVisible, both defaulting totrueand normalized with the same strict-boolean rule as the existing keys, so settings saved before these keys existed keep today's behavior.useRegisterBrowserPanel/useRegisterCommentsPanelseed the panel from that setting on mount instead of always opening it. They read the store rather than subscribing, so closing a panel from its own header is still a session-level action and is not written back as a preference.layoutwholesale, which would otherwise revert the toggle the user just made in that same dialog.Verification
Driven in the real app with Playwright, with
us_cities.geojsonloaded, in both dark and light themes:commentsPanelVisible: falsein storage; after reload the Comments rail is gone and Browser is untouched.false(the stale-draft path that would have reverted it).openRightPanel: no right panel registered).tests/layout-panel-settings.test.tscovers the defaults, the round-trip of a disabled panel, the back-compat fallback for settings saved before the keys existed, and rejection of non-boolean values. Full frontend suite (6040 passing),npm run build, and pre-commit are green.Summary by CodeRabbit
New Features
Bug Fixes