Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
} from "@posthog/ui/features/canvas/stores/channelPaneStore";
import { useCurrentChannelStore } from "@posthog/ui/features/canvas/stores/currentChannelStore";
import { useOnboardingStore } from "@posthog/ui/features/onboarding/onboardingStore";
import { useSettingsStore } from "@posthog/ui/features/settings/settingsStore";
import { NavResizeTooltip } from "@posthog/ui/features/sidebar/components/NavResizeTooltip";
import { ProjectSwitcher } from "@posthog/ui/features/sidebar/components/ProjectSwitcher";
import { SidebarMenu } from "@posthog/ui/features/sidebar/components/SidebarMenu";
Expand Down Expand Up @@ -158,8 +159,9 @@ function ChannelsSidebarImpl() {

const channelsLayout = useChannelsLayout();
const peek = useSidebarPeekStore((s) => s.peek);
const revealOnHover = useSettingsStore((s) => s.revealSidebarOnHover);
useSidebarEdgeHoverPeek({
enabled: !open && !isResizing,
enabled: revealOnHover && !open && !isResizing,
peeked: peek,
side: "left",
width,
Expand All @@ -169,8 +171,8 @@ function ChannelsSidebarImpl() {
onClose: () => endSidebarPeek(),
});
useEffect(() => {
if (open) cancelSidebarPeek();
}, [open]);
if (open || !revealOnHover) cancelSidebarPeek();
}, [open, revealOnHover]);
// The peek store is a module-level singleton β€” if this sidebar unmounts
// while peeked (route without it), a stale peek would greet the remount.
useEffect(() => () => cancelSidebarPeek(), []);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,13 +138,15 @@ export function GeneralSettings() {
defaultCloudMessagingMode,
defaultReasoningEffort,
diffOpenMode,
revealSidebarOnHover,
sendMessagesWith,
setAutoConvertLongText,
setDefaultInitialTaskMode,
setDefaultMessagingMode,
setDefaultCloudMessagingMode,
setDefaultReasoningEffort,
setDiffOpenMode,
setRevealSidebarOnHover,
setSendMessagesWith,
} = useSettingsStore();

Expand All @@ -160,6 +162,18 @@ export function GeneralSettings() {
[theme, setTheme],
);

const handleRevealSidebarOnHoverChange = useCallback(
(checked: boolean) => {
track(ANALYTICS_EVENTS.SETTING_CHANGED, {
setting_name: "reveal_sidebar_on_hover",
new_value: checked,
old_value: !checked,
});
setRevealSidebarOnHover(checked);
},
[setRevealSidebarOnHover],
);

const handleAutoConvertLongTextChange = useCallback(
(value: AutoConvertLongText) => {
track(ANALYTICS_EVENTS.SETTING_CHANGED, {
Expand Down Expand Up @@ -267,6 +281,18 @@ export function GeneralSettings() {
</SettingsCardRow>
</SettingsCard>
)}
<SettingsCard>
<SettingsCardRow
label="Reveal the sidebar on hover"
description="Slide the collapsed sidebar out when the pointer reaches the left edge. Turn this off to open it only with the toggle or ⌘B."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Render the platform-specific sidebar shortcut

should_fix bug

Why we think it's a valid issue
  • Checked: the shortcut definition, the hotkey binding, the formatter, the ship targets of the desktop app, and how other copy in this package renders the same key.
  • Found: SHORTCUTS.TOGGLE_LEFT_SIDEBAR is "mod+b" (packages/ui/src/features/command/keyboard-shortcuts.ts:23), and GlobalEventHandlers.tsx:210 binds it with useHotkeys from react-hotkeys-hook, which resolves mod to Meta on macOS and to Ctrl on other platforms. So the real key on Windows and Linux is Ctrl+B, not the ⌘B in the new description at GeneralSettings.tsx:287.
  • Found: the desktop app ships Windows and Linux builds β€” apps/code/electron-builder.ts:104 targets nsis and :118 targets AppImage, deb, rpm β€” so non-Mac users do read this row.
  • Found: the repo already solves this. formatKey maps mod to ⌘ or Ctrl from the isMac flag (keyboard-shortcuts.ts:363, packages/ui/src/utils/platform.ts), and NavResizeTooltip.tsx:16 renders this exact shortcut with formatHotkey(SHORTCUTS.TOGGLE_LEFT_SIDEBAR). Other copy branches the same way, for example ProjectSwitcher.tsx:378 and ReportChatSidebar.tsx:302.
  • Impact: every Windows and Linux user who opens this settings row gets a key that their keyboard does not have. The switch still works, and the copy also names the toggle button, so the person is not dead-ended; the defect is a wrong instruction, not a broken feature. The fix is one call to an existing helper.
Issue description

The description always shows ⌘B. The registered shortcut is mod+b. Windows and Linux users receive an incorrect instruction because their shortcut is Ctrl+B.

Suggested fix

Build the description with formatHotkey(SHORTCUTS.TOGGLE_LEFT_SIDEBAR). This matches NavResizeTooltip and stays correct on each supported platform.

Prompt to fix with AI (copy-paste)
## Context
@products/desktop/packages/ui/src/features/settings/sections/GeneralSettings.tsx#L287

<issue_description>
The description always shows ⌘B. The registered shortcut is mod+b. Windows and Linux users receive an incorrect instruction because their shortcut is Ctrl+B.
</issue_description>

<issue_validation>
- **Checked:** the shortcut definition, the hotkey binding, the formatter, the ship targets of the desktop app, and how other copy in this package renders the same key.
- **Found:** `SHORTCUTS.TOGGLE_LEFT_SIDEBAR` is `"mod+b"` (`packages/ui/src/features/command/keyboard-shortcuts.ts:23`), and `GlobalEventHandlers.tsx:210` binds it with `useHotkeys` from react-hotkeys-hook, which resolves `mod` to Meta on macOS and to Ctrl on other platforms. So the real key on Windows and Linux is Ctrl+B, not the `⌘B` in the new description at `GeneralSettings.tsx:287`.
- **Found:** the desktop app ships Windows and Linux builds β€” `apps/code/electron-builder.ts:104` targets `nsis` and `:118` targets `AppImage`, `deb`, `rpm` β€” so non-Mac users do read this row.
- **Found:** the repo already solves this. `formatKey` maps `mod` to `⌘` or `Ctrl` from the `isMac` flag (`keyboard-shortcuts.ts:363`, `packages/ui/src/utils/platform.ts`), and `NavResizeTooltip.tsx:16` renders this exact shortcut with `formatHotkey(SHORTCUTS.TOGGLE_LEFT_SIDEBAR)`. Other copy branches the same way, for example `ProjectSwitcher.tsx:378` and `ReportChatSidebar.tsx:302`.
- **Impact:** every Windows and Linux user who opens this settings row gets a key that their keyboard does not have. The switch still works, and the copy also names the toggle button, so the person is not dead-ended; the defect is a wrong instruction, not a broken feature. The fix is one call to an existing helper.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Build the description with formatHotkey(SHORTCUTS.TOGGLE_LEFT_SIDEBAR). This matches NavResizeTooltip and stays correct on each supported platform.
</potential_solution>

>
<Switch
size="sm"
checked={revealSidebarOnHover}
onCheckedChange={handleRevealSidebarOnHoverChange}
/>
Comment on lines +289 to +293

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Give the switch an accessible name

consider accessibility

Why we think it's a valid issue
  • Checked: the quill Switch primitive, the SettingsCardRow markup that wraps it, every other Switch in the settings feature, and the labeling patterns the repo already uses.
  • Found: the premise holds. SettingsCardRow renders its label as a bare <span> with no htmlFor and no id (packages/ui/src/features/settings/components/SettingsCard.tsx:82-84), and quill's Switch is a Base UI Switch.Root that renders role="switch" with no intrinsic name (packages/quill/packages/primitives/src/switch.tsx:14-22). The new control at GeneralSettings.tsx:289-293 passes only size, checked, and onCheckedChange, so a screen reader announces a switch with no name.
  • Found: the repo has both patterns. Some switches carry a name β€” ClaudeSubscriptionSettings.tsx:158, QuickAskSettings.tsx:178, PersonalizationSettings.tsx:55 use aria-label, and AutoArchiveSettingsDialog.tsx:137-143 gets its name from a quill Field label bound by id. A test relies on that name at AutoArchiveSettingsDialog.test.tsx:30.
  • Found: but every sibling row in this card stack has the same gap. The Mission Control switch directly above (GeneralSettings.tsx:276-280), the prevent-sleep switch (:464-468), and the switches in DiscordSettings.tsx:109, AdvancedSettings.tsx:54, and HarnessSettings.tsx:243 all omit a name. The cause sits in SettingsCardRow, which never binds its label to the control.
  • Impact: confirmed for screen-reader users: the row's purpose is not announced. The switch stays focusable and operable, and sighted use is unaffected, so nothing breaks functionally.
  • Priority: lowered to consider. The finding is real and the fix is one attribute, but the new row only repeats the pattern of every neighbor, and the durable fix belongs in SettingsCardRow rather than in this small feature diff.
Issue description

The new Switch has no accessible name. SettingsCardRow renders the visible label as a sibling span, so screen readers announce an unnamed switch.

Suggested fix

Add aria-label="Reveal the sidebar on hover" to the Switch. Alternatively, connect the visible label with aria-labelledby.

Prompt to fix with AI (copy-paste)
## Context
@products/desktop/packages/ui/src/features/settings/sections/GeneralSettings.tsx#L289-293

<issue_description>
The new Switch has no accessible name. SettingsCardRow renders the visible label as a sibling span, so screen readers announce an unnamed switch.
</issue_description>

<issue_validation>
- **Checked:** the quill `Switch` primitive, the `SettingsCardRow` markup that wraps it, every other `Switch` in the settings feature, and the labeling patterns the repo already uses.
- **Found:** the premise holds. `SettingsCardRow` renders its `label` as a bare `<span>` with no `htmlFor` and no `id` (`packages/ui/src/features/settings/components/SettingsCard.tsx:82-84`), and quill's `Switch` is a Base UI `Switch.Root` that renders `role="switch"` with no intrinsic name (`packages/quill/packages/primitives/src/switch.tsx:14-22`). The new control at `GeneralSettings.tsx:289-293` passes only `size`, `checked`, and `onCheckedChange`, so a screen reader announces a switch with no name.
- **Found:** the repo has both patterns. Some switches carry a name β€” `ClaudeSubscriptionSettings.tsx:158`, `QuickAskSettings.tsx:178`, `PersonalizationSettings.tsx:55` use `aria-label`, and `AutoArchiveSettingsDialog.tsx:137-143` gets its name from a quill `Field` label bound by `id`. A test relies on that name at `AutoArchiveSettingsDialog.test.tsx:30`.
- **Found:** but every sibling row in this card stack has the same gap. The Mission Control switch directly above (`GeneralSettings.tsx:276-280`), the prevent-sleep switch (`:464-468`), and the switches in `DiscordSettings.tsx:109`, `AdvancedSettings.tsx:54`, and `HarnessSettings.tsx:243` all omit a name. The cause sits in `SettingsCardRow`, which never binds its label to the control.
- **Impact:** confirmed for screen-reader users: the row's purpose is not announced. The switch stays focusable and operable, and sighted use is unaffected, so nothing breaks functionally.
- **Priority:** lowered to `consider`. The finding is real and the fix is one attribute, but the new row only repeats the pattern of every neighbor, and the durable fix belongs in `SettingsCardRow` rather than in this small feature diff.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Add aria-label="Reveal the sidebar on hover" to the Switch. Alternatively, connect the visible label with aria-labelledby.
</potential_solution>

</SettingsCardRow>
</SettingsCard>
</SettingsSection>

<SettingsSection
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ const SETTINGS_SEARCH_INDEX: SettingsSearchEntry[] = [
label: "Mission Control overlay",
keywords: ["macos", "logo"],
},
{
category: "general",
label: "Reveal the sidebar on hover",
keywords: ["sidebar", "peek", "hover", "edge", "collapsed"],
},
{
category: "general",
label: "Start in",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ describe("feature settingsStore cloud selections", () => {
["slotMachineMode", false, true],
["dismissibleUpdateBanners", false, true],
["showSidebarWorktrees", false, true],
["revealSidebarOnHover", true, false],
] as const)("rehydrates %s", async (field, initial, persisted) => {
getItem.mockResolvedValue(
JSON.stringify({ state: { [field]: persisted }, version: 0 }),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,11 @@ export interface SettingsStore {
// start a task in. Opt-in: off by default to keep the sidebar uncluttered.
showSidebarWorktrees: boolean;
setShowSidebarWorktrees: (enabled: boolean) => void;
// Slides the collapsed sidebar out when the pointer reaches the window edge
// or the title-bar toggle. On by default; off leaves the toggle and Cmd+B as
// the only ways to open it.
revealSidebarOnHover: boolean;
setRevealSidebarOnHover: (enabled: boolean) => void;

// Experimental / misc
hedgehogMode: boolean;
Expand Down Expand Up @@ -588,6 +593,9 @@ export const useSettingsStore = create<SettingsStore>()(
showSidebarWorktrees: false,
setShowSidebarWorktrees: (enabled) =>
set({ showSidebarWorktrees: enabled }),
revealSidebarOnHover: true,
setRevealSidebarOnHover: (enabled) =>
set({ revealSidebarOnHover: enabled }),

// Experimental / misc
hedgehogMode: false,
Expand Down Expand Up @@ -733,6 +741,7 @@ export const useSettingsStore = create<SettingsStore>()(

// Sidebar
showSidebarWorktrees: state.showSidebarWorktrees,
revealSidebarOnHover: state.revealSidebarOnHover,

// Experimental / misc
hedgehogMode: state.hedgehogMode,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ import {
PEEK_REVEAL_THRESHOLD,
shouldCloseOnExit,
shouldRevealOnEdge,
useSidebarEdgeHoverPeek,
} from "@posthog/ui/primitives/hooks/useSidebarEdgeHoverPeek";
import { describe, expect, it } from "vitest";
import { renderHook } from "@testing-library/react";
import { describe, expect, it, vi } from "vitest";

describe("shouldRevealOnEdge", () => {
const threshold = PEEK_REVEAL_THRESHOLD;
Expand Down Expand Up @@ -39,3 +41,31 @@ describe("shouldCloseOnExit", () => {
expect(shouldCloseOnExit({ pointer, width, margin })).toBe(expected);
});
});

describe("useSidebarEdgeHoverPeek", () => {
const moveTo = (clientX: number): void => {
document.dispatchEvent(new MouseEvent("mousemove", { clientX }));
};

it.each([
["reveals while enabled", true, 1],
["stays closed while disabled", false, 0],
])("%s", (_name, enabled, calls) => {
const onReveal = vi.fn();
renderHook(() =>
useSidebarEdgeHoverPeek({
enabled,
peeked: false,
side: "left",
width: 240,
onReveal,
onClose: vi.fn(),
}),
);

moveTo(400);
moveTo(2);

expect(onReveal).toHaveBeenCalledTimes(calls);
});
});
4 changes: 3 additions & 1 deletion products/desktop/packages/ui/src/router/routes/__root.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import { useInboxDeepLink } from "@posthog/ui/features/inbox/hooks/useInboxDeepL
import { useIntegrations } from "@posthog/ui/features/integrations/useIntegrations";
import { useLoopDeepLink } from "@posthog/ui/features/loops/hooks/useLoopDeepLink";
import { useScoutDeepLink } from "@posthog/ui/features/scouts/hooks/useScoutDeepLink";
import { useSettingsStore } from "@posthog/ui/features/settings/settingsStore";
import { useSetupDiscovery } from "@posthog/ui/features/setup/useSetupDiscovery";
import { NAV_RAIL_WIDTH } from "@posthog/ui/features/sidebar/constants";
import {
Expand Down Expand Up @@ -218,6 +219,7 @@ function RootLayout() {

const toggleSidebar = useSidebarStore((s) => s.toggle);
const sidebarPeek = useSidebarPeekStore((s) => s.peek);
const revealSidebarOnHover = useSettingsStore((s) => s.revealSidebarOnHover);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wait for settings hydration before enabling hover

consider bug

Why we think it's a valid issue
  • Checked: the storage backend behind each store, the default values, both peek triggers, the scrim's render gate, and the hydration idiom the repo already uses.
  • Found: the hydration premise holds. useSettingsStore persists through electronStorage (settingsStore.ts:653), whose getItem is async β€” it awaits host-storage registration and then an IPC read (packages/ui/src/shell/rendererStorage.ts:120-146), and the comment at :105-113 states these stores are created before the host composition root finishes. The default is revealSidebarOnHover: true (settingsStore.ts:596), so a saved false is absent until the read lands.
  • Found: the race reaches the user who wants it least. Both triggers also need a collapsed sidebar, and useSidebarStore passes no storage option (packages/ui/src/features/sidebar/sidebarStore.ts:69-160), so it hydrates synchronously from localStorage. A person who works collapsed therefore has open === false on the first render while revealSidebarOnHover still reads the default true. A pointer in the 24px band, or on the title-bar toggle at __root.tsx:353, reveals the sidebar and dims the scrim.
  • Found: the fix is idiomatic here. _hasHydrated exists (settingsStore.ts:349, :648-649) and other preference-gated UI waits on it β€” TeachingTip.tsx:72, adapterSubscription.ts:133, customInstructionsSync.contribution.ts:35.
  • Found: the second half of the claim is partly misattributed. The scrim renders in __root.tsx:419-430 gated only on !sidebarOpen, and ChannelsSidebar mounts only when hasSidebar (__root.tsx:434), so a peek begun from the title bar on a sidebar-less route does persist. But that path is unchanged by this diff: master calls beginSidebarPeek() from the same handler with no preference guard, so the new code only makes the trigger fire less often. The edge band cannot fire there at all, because the hook lives inside ChannelsSidebar.
  • Impact: confirmed but transient. A person who turned the reveal off can still get one slide-out plus scrim during startup. On any route with the sidebar, the effect at ChannelsSidebar.tsx:173-175 cancels the peek as soon as hydration flips the value, so the visible result is a short flash of the behavior they disabled.
  • Priority: lowered to consider. The window is short, it needs the pointer to sit at the edge during startup, and the state repairs itself on hydration. The stuck-scrim outcome that would make this worse exists on master already and is not introduced by this change.
Issue description

electronStorage hydrates asynchronously. Before hydration finishes, revealSidebarOnHover has its default true value. A user who saved false can still trigger hover during startup. On routes without ChannelsSidebar, the scrim can remain visible because no mounted effect cancels the peek.

Suggested fix

Read _hasHydrated with the preference. Require both values here and in useSidebarEdgeHoverPeek. Cancel the peek when hydration resolves to false. Add a delayed-hydration test with a saved false value.

Prompt to fix with AI (copy-paste)
## Context
@products/desktop/packages/ui/src/router/routes/__root.tsx#L222
@products/desktop/packages/ui/src/router/routes/__root.tsx#L353

<issue_description>
electronStorage hydrates asynchronously. Before hydration finishes, revealSidebarOnHover has its default true value. A user who saved false can still trigger hover during startup. On routes without ChannelsSidebar, the scrim can remain visible because no mounted effect cancels the peek.
</issue_description>

<issue_validation>
- **Checked:** the storage backend behind each store, the default values, both peek triggers, the scrim's render gate, and the hydration idiom the repo already uses.
- **Found:** the hydration premise holds. `useSettingsStore` persists through `electronStorage` (`settingsStore.ts:653`), whose `getItem` is async β€” it awaits host-storage registration and then an IPC read (`packages/ui/src/shell/rendererStorage.ts:120-146`), and the comment at `:105-113` states these stores are created before the host composition root finishes. The default is `revealSidebarOnHover: true` (`settingsStore.ts:596`), so a saved `false` is absent until the read lands.
- **Found:** the race reaches the user who wants it least. Both triggers also need a collapsed sidebar, and `useSidebarStore` passes no `storage` option (`packages/ui/src/features/sidebar/sidebarStore.ts:69-160`), so it hydrates synchronously from localStorage. A person who works collapsed therefore has `open === false` on the first render while `revealSidebarOnHover` still reads the default `true`. A pointer in the 24px band, or on the title-bar toggle at `__root.tsx:353`, reveals the sidebar and dims the scrim.
- **Found:** the fix is idiomatic here. `_hasHydrated` exists (`settingsStore.ts:349`, `:648-649`) and other preference-gated UI waits on it β€” `TeachingTip.tsx:72`, `adapterSubscription.ts:133`, `customInstructionsSync.contribution.ts:35`.
- **Found:** the second half of the claim is partly misattributed. The scrim renders in `__root.tsx:419-430` gated only on `!sidebarOpen`, and `ChannelsSidebar` mounts only when `hasSidebar` (`__root.tsx:434`), so a peek begun from the title bar on a sidebar-less route does persist. But that path is unchanged by this diff: master calls `beginSidebarPeek()` from the same handler with no preference guard, so the new code only makes the trigger fire less often. The edge band cannot fire there at all, because the hook lives inside `ChannelsSidebar`.
- **Impact:** confirmed but transient. A person who turned the reveal off can still get one slide-out plus scrim during startup. On any route with the sidebar, the effect at `ChannelsSidebar.tsx:173-175` cancels the peek as soon as hydration flips the value, so the visible result is a short flash of the behavior they disabled.
- **Priority:** lowered to `consider`. The window is short, it needs the pointer to sit at the edge during startup, and the state repairs itself on hydration. The stuck-scrim outcome that would make this worse exists on master already and is not introduced by this change.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Read _hasHydrated with the preference. Require both values here and in useSidebarEdgeHoverPeek. Cancel the peek when hydration resolves to false. Add a delayed-hydration test with a saved false value.
</potential_solution>

// Toggling makes any hover-peek redundant (opening replaces the overlay;
// closing must not leave it lingering under the pointer).
const handleToggleSidebar = (): void => {
Expand Down Expand Up @@ -348,7 +350,7 @@ function RootLayout() {
aria-label="Toggle sidebar"
onClick={handleToggleSidebar}
onMouseEnter={() => {
if (!sidebarOpen) beginSidebarPeek();
if (revealSidebarOnHover && !sidebarOpen) beginSidebarPeek();
}}
>
{sidebarOpen ? (
Expand Down
Loading