Skip to content

fix: make the Comments panel opt-in from Settings #1638

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
giswqs merged 5 commits into from
Aug 2, 2026
Merged

fix: make the Comments panel opt-in from Settings #1638

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e67af60
fix: make the Comments panel opt-in from Settings
giswqs Aug 2, 2026
453e726
fix: let dock panels coexist in shared rails
giswqs Aug 2, 2026
2ab7b1c
Address review feedback
giswqs Aug 2, 2026
ef78f30
Address review feedback
giswqs Aug 2, 2026
870653f
Address review feedback
giswqs Aug 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 74 additions & 37 deletions apps/geolibre-desktop/src/components/layout/DesktopShell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,14 @@
el.className = "contents";
return el;
});
const activePanelId = useRightPanelState().activeId;
const rightPanelState = useRightPanelState();
const activePanelId = rightPanelState.activeId;
const replaceStylePanelIds = rightPanelState.visibleIds.filter(
(id) => rightPanelState.panelDocks[id] === "replace-style",
);
const replaceLayersPanelIds = rightPanelState.visibleIds.filter(
(id) => rightPanelState.panelDocks[id] === "replace-layers",
);
const activePanel = activePanelId ? getRightPanel(activePanelId) : undefined;
// The plugins in VIEWER_BLOCKED_PLUGIN_IDS paint drawing and editing controls
// onto the map, which the read-only viewer preset cannot hide the way it
Expand Down Expand Up @@ -1374,7 +1381,7 @@
}
});
return () => setKmlFileImportHandler(null);
}, [addImportedVectorLayers, confirmLargeVectorDataset, t]);

Check warning on line 1384 in apps/geolibre-desktop/src/components/layout/DesktopShell.tsx

View workflow job for this annotation

GitHub Actions / Build and test 

React Hook useEffect has an unnecessary dependency: 'confirmLargeVectorDataset'. Either exclude it or remove the dependency array. Outer scope values like 'confirmLargeVectorDataset' aren't valid dependencies because mutating them doesn't re-render the component

const addDroppedPhotos = useCallback(
(result: GeotaggedPhotoResult | null): number => {
Expand Down Expand Up @@ -1650,7 +1657,7 @@
disposed = true;
unlisten?.();
};
}, [

Check warning on line 1660 in apps/geolibre-desktop/src/components/layout/DesktopShell.tsx

View workflow job for this annotation

GitHub Actions / Build and test 

React Hook useEffect has a missing dependency: 't'. Either include it or remove the dependency array
clearDropMessageLater,
finishDrop,
addDroppedRasters,
Expand Down Expand Up @@ -1794,7 +1801,7 @@
clearDropMessageLater();
}
},
[

Check warning on line 1804 in apps/geolibre-desktop/src/components/layout/DesktopShell.tsx

View workflow job for this annotation

GitHub Actions / Build and test 

React Hook useCallback has a missing dependency: 't'. Either include it or remove the dependency array
clearDropMessageLater,
finishDrop,
addDroppedRasters,
Expand Down Expand Up @@ -2087,6 +2094,7 @@
key={replaceLayersPanelId}
side="layers"
pluginId={replaceLayersPanelId}
additionalPanelIds={replaceLayersPanelIds}
Comment thread
giswqs marked this conversation as resolved.
Outdated
pluginContentEl={dockContentEl}
pluginWidth={pluginPanelWidth}
onPluginWidthChange={setPluginPanelWidth}
Expand Down Expand Up @@ -2312,42 +2320,71 @@
{/* Same as the left dock: a map-only / hidden-panels embed skips the
entire right side-dock (Style, plugin panels, and their shared rail). */}
{layoutOptions.panelsHidden || layoutOptions.viewer ? null : replaceStylePanelId ? (
// Shared-rail mode (issue #765): the plugin panel shares the Style
// sidebar surface, so a single rail lists both the workbench and Style
// instead of the two positional plugin slots flanking the Style panel.
<SectionErrorBoundary
label="Shared right sidebar"
displayName={t("shell.section.sharedRightSidebar")}
>
<SharedSidebar
// Key by the active panel id so switching between two replace-style
// plugins remounts the sidebar, resetting its per-panel local state
// (the Style opt-in) rather than carrying the previous plugin over.
key={replaceStylePanelId}
side="style"
pluginId={replaceStylePanelId}
pluginContentEl={dockContentEl}
pluginWidth={pluginPanelWidth}
onPluginWidthChange={setPluginPanelWidth}
builtinVisible={layoutOptions.stylePanelVisible}
builtinTitle={t("sharedRail.style")}
builtinIcon={<SlidersHorizontal className="h-4 w-4" />}
// Mirror the standalone Style panel's autoCollapse triggers so the
// notebook / story-map presentation collapses Style here too.
// `autoCollapsedPanel` is omitted because it is always null in a
// shared-rail mode (the panel is the sole active one).
forceBuiltinCollapsed={notebookOpen || storymapPresenting}
renderBuiltin={({ collapsed, onCollapsedChange }) => (
<StylePanel
mapControllerRef={mapControllerRef}
onResizeStart={startStylePanelResize}
collapsed={collapsed}
onCollapsedChange={onCollapsedChange}
hideOwnRail
/>
)}
/>
</SectionErrorBoundary>
<>
{/* Shared-rail panels such as Comments must not remove the ordinary
positional docks: enabled Web Services panels still live in
left/right-of-style and need their vertical rail entries. */}
<SectionErrorBoundary
label="Plugin panel (left of Style)"
displayName={t("shell.section.pluginPanelLeftOfStyle")}
>
<PluginRightPanel
dock="left-of-style"
contentEl={dockContentEl}
width={pluginPanelWidth}
onWidthChange={setPluginPanelWidth}
/>
</SectionErrorBoundary>
<SectionErrorBoundary
label="Shared right sidebar"
displayName={t("shell.section.sharedRightSidebar")}
>
<SharedSidebar
// Key by the active panel id so switching between two replace-style
// plugins remounts the sidebar, resetting its per-panel local state
// (the Style opt-in) rather than carrying the previous plugin over.
key={replaceStylePanelId}

@github-actions github-actions Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that multiple panels can be simultaneously visible in the shared Style rail (Comments plus any of the many plugins now defaulting to replace-style), replaceStylePanelId — and therefore this key — changes on every click between two already-visible rail entries, not just on the rare merge/detach action the original comment describes. Each switch fully remounts SharedSidebar, including the wrapped <StylePanel> and SharedSidebar's own builtinOptedIn state.

That interacts badly with the new auto-expand effect in StylePanel.tsx (previousSelectedLayerId = useRef(selectedLayerId) + effect gating on a change from that ref): the ref reinitializes to the current selectedLayerId on every remount, so if a layer is already selected when the remount happens (the common case — a user had a layer selected, then merely clicked to a different panel in the shared rail), the effect sees no "change" and never expands Style. The panel lands collapsed even though a real layer is selected, defeating the "selecting a layer re-expands the Style panel" behavior this PR adds.

Confidence: medium — the remount mechanics and the ref's mount-time initialization are clear from the code; the perceived severity depends on how often users actually have two replace-style panels open at once (Comments + a Web Services plugin).

@giswqs giswqs Aug 2, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not changing this one — the remount is deliberate and the outcome it produces is the intended layout. Clicking a second shared-rail entry makes that panel the expanded surface, so pluginExpanded is true and Style is collapsed regardless of builtinOptedIn; the reset ref only matters once the user collapses the plugin again, and at that point Style being on the rail is the documented "both collapsed" resting state rather than a defeated auto-expand. Selecting any layer after that still expands Style, because the selection is a real change against the ref. Re-selecting the same layer is a no-op by design (the effect fires on change, not on click). Happy to revisit if the collapse-on-switch reads wrong in use.

side="style"
pluginId={replaceStylePanelId}
additionalPanelIds={replaceStylePanelIds}
pluginContentEl={dockContentEl}
pluginWidth={pluginPanelWidth}
onPluginWidthChange={setPluginPanelWidth}
builtinVisible={layoutOptions.stylePanelVisible}
builtinTitle={t("sharedRail.style")}
builtinIcon={<SlidersHorizontal className="h-4 w-4" />}
// Mirror the standalone Style panel's autoCollapse triggers so the
// notebook / story-map presentation collapses Style here too.
// `autoCollapsedPanel` is omitted because it is always null in a
// shared-rail mode (the panel is the sole active one).
forceBuiltinCollapsed={notebookOpen || storymapPresenting}
renderBuiltin={({ collapsed, onCollapsedChange }) => (
<StylePanel
mapControllerRef={mapControllerRef}
onResizeStart={startStylePanelResize}
collapsed={collapsed}
onCollapsedChange={onCollapsedChange}
// Controlled mode ignores autoCollapse for collapsing (the
// rail owns that via forceBuiltinCollapsed); it is passed so
// a layer selection cannot expand Style over the notebook.
autoCollapse={notebookOpen || storymapPresenting}
hideOwnRail
/>
)}
/>
</SectionErrorBoundary>
<SectionErrorBoundary
label="Plugin panel (right of Style)"
displayName={t("shell.section.pluginPanelRightOfStyle")}
>
<PluginRightPanel
dock="right-of-style"
contentEl={dockContentEl}
width={pluginPanelWidth}
onWidthChange={setPluginPanelWidth}
/>
</SectionErrorBoundary>
</>
) : (
<>
<SectionErrorBoundary
Expand Down
31 changes: 30 additions & 1 deletion apps/geolibre-desktop/src/components/layout/SettingsDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import {
Locate,
MapPinned,
LayoutPanelTop,
MessageSquare,
Moon,
Palette,
PanelLeft,
Expand Down Expand Up @@ -84,6 +85,7 @@ import {
} from "../../hooks/useDesktopSettings";
import { useLanguage } from "../../hooks/useLanguage";
import { BROWSER_PANEL_ID } from "../../hooks/useRegisterBrowserPanel";
import { COMMENTS_PANEL_ID } from "../../hooks/useRegisterCommentsPanel";
import { useRightPanelState } from "../../hooks/useRightPanels";
import type { ThemeMode } from "../../hooks/useThemeMode";
import { isTauri } from "../../lib/is-tauri";
Expand Down Expand Up @@ -390,7 +392,9 @@ export function SettingsDialog({
// The Browser is a dockable right panel (open/close via the registry), not a
// persisted layout preference, so its Layout toggle acts on the live registry
// state directly rather than through the draft settings.
const browserPanelOpen = useRightPanelState().activeId === BROWSER_PANEL_ID;
const rightPanelState = useRightPanelState();
const browserPanelOpen = rightPanelState.visibleIds.includes(BROWSER_PANEL_ID);
const commentsPanelOpen = rightPanelState.visibleIds.includes(COMMENTS_PANEL_ID);
// Show it collapsed on the shared Layers rail, matching its default state, so
// re-enabling from Settings doesn't jump to an expanded panel that buries the
// Layers panel.
Expand All @@ -402,6 +406,14 @@ export function SettingsDialog({
closeRightPanel(BROWSER_PANEL_ID);
}
};
const toggleCommentsPanel = (show: boolean) => {
if (show) {
openRightPanel(COMMENTS_PANEL_ID);
collapseRightPanel(COMMENTS_PANEL_ID);
} else {
closeRightPanel(COMMENTS_PANEL_ID);
}
};
Comment thread
giswqs marked this conversation as resolved.
// A field a deep-link asked us to focus once its section renders; cleared
// after the focus lands so a later open without a focus request stays put.
const [pendingFocus, setPendingFocus] = useState<SettingsFocusTarget | null>(null);
Expand Down Expand Up @@ -1264,6 +1276,13 @@ export function SettingsDialog({
>
{t("settings.layout.showBrowserPanel")}
</DropdownMenuCheckboxItem>
<DropdownMenuCheckboxItem
checked={commentsPanelOpen}
onCheckedChange={(checked: boolean) => toggleCommentsPanel(checked === true)}
onSelect={(event: Event) => event.preventDefault()}
>
{t("settings.layout.showCommentsPanel")}
</DropdownMenuCheckboxItem>
<DropdownMenuSeparator />
<DropdownMenuItem
onSelect={() => {
Expand Down Expand Up @@ -1738,6 +1757,16 @@ export function SettingsDialog({
<FolderTree className="h-4 w-4 text-muted-foreground" />
<span>{t("settings.layout.showBrowserPanel")}</span>
</label>
<label className="flex items-center gap-3 rounded-md border p-3 text-sm">
<input
className="h-4 w-4"
type="checkbox"
checked={commentsPanelOpen}
onChange={(event) => toggleCommentsPanel(event.target.checked)}
/>
<MessageSquare className="h-4 w-4 text-muted-foreground" />
<span>{t("settings.layout.showCommentsPanel")}</span>
</label>
</div>
{showsAdvancedNotices(desktopSettings.uiProfile) ? (
<div className="rounded-md border bg-muted/40 p-3 text-xs text-muted-foreground">
Expand Down
115 changes: 64 additions & 51 deletions apps/geolibre-desktop/src/components/panels/PluginRightPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
closeRightPanel,
collapseRightPanel,
getRightPanel,
listRightPanels,
moveActiveRightPanelDock,
openRightPanel,
type RightPanelDock,
Expand All @@ -15,10 +16,8 @@ import {
Combine,
PanelLeft,
PanelLeftClose,
PanelLeftOpen,
PanelRight,
PanelRightClose,
PanelRightOpen,
X,
} from "lucide-react";
import {
Expand Down Expand Up @@ -81,7 +80,7 @@ interface PluginRightPanelProps {
*/
export function PluginRightPanel({ dock, contentEl, width, onWidthChange }: PluginRightPanelProps) {
const { t } = useTranslation();
const { activeId, collapsed, dock: activeDock } = useRightPanelState();
const { activeId, collapsed, dock: activeDock, panelDocks } = useRightPanelState();
const contentRef = useRef<HTMLDivElement | null>(null);

const panel = activeId ? getRightPanel(activeId) : undefined;
Expand All @@ -96,6 +95,11 @@ export function PluginRightPanel({ dock, contentEl, width, onWidthChange }: Plug
// move buttons and no rail of its own; its collapsed entry lives in that single
// shared rail instead.
const isSharedRail = dock === "replace-style" || dock === "replace-layers";
const dockPanels = isSharedRail
? []
: listRightPanels().filter((candidate) =>
candidate.id === activeId ? activeDock === dock : panelDocks[candidate.id] === dock,
);
Comment thread
giswqs marked this conversation as resolved.
Comment thread
giswqs marked this conversation as resolved.
Comment thread
giswqs marked this conversation as resolved.

// Adopt the shared content host (rendered once by the shell) into this slot
// while it owns the panel. appendChild moves the element, so stepping the
Expand All @@ -112,10 +116,56 @@ export function PluginRightPanel({ dock, contentEl, width, onWidthChange }: Plug
wrapper.appendChild(contentEl);
}, [matched, contentEl, collapsed]);

if (!matched || !panel) return null;
const panelRail =
dockPanels.length > 0 ? (
<aside
aria-label={t("pluginPanel.collapsedLabel", { title: t("toolbar.menu.plugins") })}
className={`flex h-11 w-full shrink-0 items-center gap-1 overflow-x-auto border-t bg-card px-2 md:h-auto md:w-11 md:flex-col md:overflow-x-visible md:overflow-y-auto md:border-t-0 md:px-0 md:py-2 ${isLayersSide ? "md:border-e" : "md:border-s"}`}
>
{dockPanels.map((candidate) => {
const candidateIcon =
candidate.icon && isImageSource(candidate.icon) ? (
<img src={candidate.icon} alt="" className="h-4 w-4 object-contain" />
) : isLayersSide ? (
<PanelLeft className="h-4 w-4" />
) : (
<PanelRight className="h-4 w-4" />
);
const expanded = candidate.id === activeId && !collapsed;
return (
<button
key={candidate.id}
type="button"
aria-pressed={expanded}
title={candidate.title}
aria-label={candidate.title}
onClick={() =>
expanded ? collapseRightPanel(candidate.id) : openRightPanel(candidate.id)
}
className={`flex items-center gap-2 rounded px-1.5 py-1.5 md:flex-col md:px-1 md:py-2 ${
expanded
? "bg-accent text-accent-foreground"
: "text-muted-foreground hover:bg-accent/50 hover:text-foreground"
}`}
>
{candidateIcon}
<span className="text-[10px] font-semibold uppercase tracking-wide md:[writing-mode:vertical-rl] md:rotate-180">
{candidate.title}
</span>
</button>
);
})}
</aside>
) : null;

if (!matched || !panel) return panelRail;
// When collapsed in shared-rail mode the host's single shared rail shows this
// panel's entry, so render nothing here (no second rail beside Style).
if (isSharedRail && collapsed) return null;
// Collapsed at a positional dock: `panelRail` already carries this panel's
// entry (alongside any other panel docked here), so nothing else renders and
// everything below this point runs expanded.
if (collapsed) return panelRail;

const handleResizeStart = (event: ReactPointerEvent<HTMLDivElement>) => {
event.preventDefault();
Expand Down Expand Up @@ -147,15 +197,6 @@ export function PluginRightPanel({ dock, contentEl, width, onWidthChange }: Plug
el.addEventListener("pointercancel", handleEnd);
};

const railIcon =
panel.icon && isImageSource(panel.icon) ? (
<img src={panel.icon} alt="" className="h-4 w-4 object-contain" />
) : isLayersSide ? (
<PanelLeft className="h-4 w-4" />
) : (
<PanelRight className="h-4 w-4" />
);

const borderSide = isLayersSide ? "md:border-e" : "md:border-s";
// Dock names describe the LTR arrangement, so the visual move-left/right
// actions and their guards swap in a right-to-left layout.
Expand All @@ -164,48 +205,19 @@ export function PluginRightPanel({ dock, contentEl, width, onWidthChange }: Plug
const canMoveRight = activeDock !== (isRtl ? "left-of-layers" : "right-of-style");

return (
<aside
aria-label={collapsed ? t("pluginPanel.collapsedLabel", { title: panel.title }) : panel.title}
style={{ "--plugin-right-panel-width": `${width}px` } as CSSProperties}
className={
collapsed
? `flex h-11 w-full shrink-0 items-center gap-2 border-t bg-card px-2 md:h-auto md:w-11 md:flex-col md:border-t-0 md:py-2 ${borderSide}`
: `relative flex max-h-[min(24rem,42vh)] supports-[max-height:1dvh]:max-h-[min(24rem,42dvh)] w-full shrink-0 flex-col border-t bg-card max-md:absolute max-md:inset-x-0 max-md:bottom-0 max-md:z-30 max-md:shadow-xl md:max-h-none md:w-[var(--plugin-right-panel-width)] md:border-t-0 ${borderSide}`
}
>
{!collapsed ? (
<>
<aside
aria-label={panel.title}
style={{ "--plugin-right-panel-width": `${width}px` } as CSSProperties}
className={`relative flex max-h-[min(24rem,42vh)] supports-[max-height:1dvh]:max-h-[min(24rem,42dvh)] w-full shrink-0 flex-col border-t bg-card max-md:absolute max-md:inset-x-0 max-md:bottom-0 max-md:z-30 max-md:shadow-xl md:max-h-none md:w-[var(--plugin-right-panel-width)] md:border-t-0 ${borderSide}`}
>
<div
role="separator"
aria-orientation="vertical"
aria-label={t("pluginPanel.resize")}
className={`absolute ${isLayersSide ? "-end-1 border-e" : "-start-1 border-s"} top-0 z-20 hidden h-full w-2 cursor-col-resize touch-none select-none border-transparent hover:border-primary md:block`}
onPointerDown={handleResizeStart}
/>
) : null}
{collapsed ? (
<>
<Button
variant="ghost"
size="icon"
className="h-8 w-8"
title={t("pluginPanel.expand")}
aria-label={t("pluginPanel.expand")}
onClick={() => openRightPanel(activeId)}
>
{isLayersSide ? (
<PanelLeftOpen className="h-4 w-4" />
) : (
<PanelRightOpen className="h-4 w-4" />
)}
</Button>
<div className="flex items-center gap-2 text-muted-foreground md:mt-3 md:flex-col">
{railIcon}
<span className="text-[10px] font-semibold uppercase tracking-wide md:[writing-mode:vertical-rl] md:rotate-180">
{panel.title}
</span>
</div>
</>
) : (
<div className="flex items-center justify-between border-b px-3 py-1.5">
<span className="truncate text-sm font-semibold">{panel.title}</span>
<div className="flex items-center gap-1">
Expand Down Expand Up @@ -301,8 +313,9 @@ export function PluginRightPanel({ dock, contentEl, width, onWidthChange }: Plug
</Button>
</div>
</div>
)}
<div ref={contentRef} className={collapsed ? "hidden" : "min-h-0 flex-1 overflow-auto"} />
</aside>
<div ref={contentRef} className="min-h-0 flex-1 overflow-auto" />
</aside>
{panelRail}
</>
);
}
Loading
Loading