feat(desktop): let people turn off the sidebar hover reveal - #95523
Open
posthog[bot] wants to merge 1 commit into
Open
posthog[bot] wants to merge 1 commit into
posthog[bot] wants to merge 1 commit into
Conversation
The collapsed sidebar slides out whenever the pointer crosses a 24px band at the window edge, and it dims the content behind a scrim while it is out. There was no way to stop it. Add a "Reveal the sidebar on hover" preference under Appearance. It is on by default, so nothing changes for people who like the current behavior. When it is off, both hover triggers stay quiet — the window edge and the title-bar toggle — and the toggle button and Cmd+B remain the ways to open the sidebar. Toggling the preference emits the standard setting-changed event, so we can finally measure how many people want the hover reveal off. Generated-By: PostHog Desktop Task-Id: 3b41770e-dae5-4037-808b-83e6b5e0f2cb
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Contributor
Author
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 1 should fix, 2 consider. Published 3 findings (view the review). |
Contributor
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Contributor
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes. |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved.
Contained, additive desktop UI preference with matching tests; no risky-territory surface (no auth/billing/API/CI/dependency changes), so no independent assurance is required.
- 7 of the 7 changed files are governed by
products/desktop/AGENT_APPROVALS.md.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 52L, 5F substantive, 85L/7F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (85L, 7F, single-area, feat) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 1387578 · reviewed head 1387578 |
HostHog preview —
|
Contributor
Author
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| <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." |
Contributor
Author
There was a problem hiding this comment.
Render the platform-specific sidebar shortcut
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_SIDEBARis"mod+b"(packages/ui/src/features/command/keyboard-shortcuts.ts:23), andGlobalEventHandlers.tsx:210binds it withuseHotkeysfrom react-hotkeys-hook, which resolvesmodto Meta on macOS and to Ctrl on other platforms. So the real key on Windows and Linux is Ctrl+B, not the⌘Bin the new description atGeneralSettings.tsx:287. - Found: the desktop app ships Windows and Linux builds —
apps/code/electron-builder.ts:104targetsnsisand:118targetsAppImage,deb,rpm— so non-Mac users do read this row. - Found: the repo already solves this.
formatKeymapsmodto⌘orCtrlfrom theisMacflag (keyboard-shortcuts.ts:363,packages/ui/src/utils/platform.ts), andNavResizeTooltip.tsx:16renders this exact shortcut withformatHotkey(SHORTCUTS.TOGGLE_LEFT_SIDEBAR). Other copy branches the same way, for exampleProjectSwitcher.tsx:378andReportChatSidebar.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>
Comment on lines
+289
to
+293
| <Switch | ||
| size="sm" | ||
| checked={revealSidebarOnHover} | ||
| onCheckedChange={handleRevealSidebarOnHoverChange} | ||
| /> |
Contributor
Author
There was a problem hiding this comment.
Give the switch an accessible name
Why we think it's a valid issue
- Checked: the quill
Switchprimitive, theSettingsCardRowmarkup that wraps it, every otherSwitchin the settings feature, and the labeling patterns the repo already uses. - Found: the premise holds.
SettingsCardRowrenders itslabelas a bare<span>with nohtmlForand noid(packages/ui/src/features/settings/components/SettingsCard.tsx:82-84), and quill'sSwitchis a Base UISwitch.Rootthat rendersrole="switch"with no intrinsic name (packages/quill/packages/primitives/src/switch.tsx:14-22). The new control atGeneralSettings.tsx:289-293passes onlysize,checked, andonCheckedChange, 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:55usearia-label, andAutoArchiveSettingsDialog.tsx:137-143gets its name from a quillFieldlabel bound byid. A test relies on that name atAutoArchiveSettingsDialog.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 inDiscordSettings.tsx:109,AdvancedSettings.tsx:54, andHarnessSettings.tsx:243all omit a name. The cause sits inSettingsCardRow, 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 inSettingsCardRowrather 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>
|
|
||
| const toggleSidebar = useSidebarStore((s) => s.toggle); | ||
| const sidebarPeek = useSidebarPeekStore((s) => s.peek); | ||
| const revealSidebarOnHover = useSettingsStore((s) => s.revealSidebarOnHover); |
Contributor
Author
There was a problem hiding this comment.
Wait for settings hydration before enabling hover
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.
useSettingsStorepersists throughelectronStorage(settingsStore.ts:653), whosegetItemis async — it awaits host-storage registration and then an IPC read (packages/ui/src/shell/rendererStorage.ts:120-146), and the comment at:105-113states these stores are created before the host composition root finishes. The default isrevealSidebarOnHover: true(settingsStore.ts:596), so a savedfalseis absent until the read lands. - Found: the race reaches the user who wants it least. Both triggers also need a collapsed sidebar, and
useSidebarStorepasses nostorageoption (packages/ui/src/features/sidebar/sidebarStore.ts:69-160), so it hydrates synchronously from localStorage. A person who works collapsed therefore hasopen === falseon the first render whilerevealSidebarOnHoverstill reads the defaulttrue. 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.
_hasHydratedexists (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-430gated only on!sidebarOpen, andChannelsSidebarmounts only whenhasSidebar(__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 callsbeginSidebarPeek()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 insideChannelsSidebar. - 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-175cancels 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>
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
onMouseEntertoo.Changes
ChannelsSidebarand the title-bar toggle'sonMouseEnterin__root.tsx. Turning it off while the sidebar is peeked out also closes it.SETTING_CHANGEDevent, which is the count the report could not get.How did you test this code?
useSidebarEdgeHoverPeek.test.ts: a documentmousemoveinto the edge band callsonRevealwhen the hook is enabled and does not when it is disabled. This is the path the preference now controls; nothing covered it before, so a change that stopped honoringenabledwould have silently broken the preference.revealSidebarOnHoverto the parameterized rehydrate case insettingsStore.test.ts. A preference dropped frompartializewould come back on after a restart, which is exactly the annoyance being fixed.@posthog/uivitest suite,tsc --noEmit, and biome.Automatic notifications
Docs update
None. No doc lists the desktop settings rows.
🤖 Agent context
Autonomy: Fully autonomous
/writing-simplified-technical-english. Repo guidance read for the desktop frontend and the settings feature.SETTING_CHANGEDevent answers the question the report actually asked, at a far lower volume.gh pr list --state openfound no other open PR on the sidebar peek.Created with PostHog Desktop from this inbox report.