feat(settings): regroup Settings + Floating Navigator inline shortcut + cross-window pin sync#106
Merged
Conversation
… + eng + design review)
…s-window pin sync Regroup /settings into concern-based sections (Tasks, Sound, Appearance, Brain Dump, Floating Navigator, Application, Updates), splitting the PreferencesSettings + FloatingWindowSettings monoliths into focused per-concern components, each under a semantic <h2> section heading. - §6a regroup: web-common labels in page.tsx; Electron labels inside ElectronSettingsPage (after the isElectron gate, so web shows no orphan headers); D1 flatten (Caption is the sole section heading); D2 fold "Settings Window" into Application; Arch-1 relocate show-on-all-desktops. - §6b Floating Navigator inline toggle-shortcut: reads/writes the canonical shortcuts.toggleFloatingNavigator (no separate mirror, so it never drifts from a generic-keybind rebind) via 2 IPC handlers + preload + Zod + types, mirroring the BrainDump rebind UI. - §6c remove Electron-leaning BrainDump appearance items from web Preferences (moved into the Electron Brain Dump card). - §6e conflict-substitution fix (Design B): reject a silently substituted accelerator by reading getRegisteredShortcuts() back and restoring the previous binding; shared applyShortcutRebind helper for both toggles. - §6d keep-on-top cross-window sync: WindowManager broadcasts the change to the floating window so its own pin button live-updates instead of lying until relaunch (RED-test-proven on the consume side; emit side unit-tested). - CQ-1 extract useShortcutCapture hook (shared by BrainDump + Floating). Gates: pnpm validate (661 unit + typecheck + build + lint + packages + theme) and the electron suite (320 tests) all green.
Add a live-Electron E2E that drives the canonical Settings-bridge setter (floatingPanels.setFloatingNavigatorAlwaysOnTop) and asserts the floating window receives the broadcast as a DOM CustomEvent. This proves the one §6d link unit tests can't reach: main-process broadcast -> preload module-level forward -> page window event, over real IPC. Auth-independent (the preload forward is module-scoped, so it fires whether the floating page shows the signed-in navigator or the signed-out front-door card; the CI harness is signed out). Links 1/2/4 stay unit-covered (WindowManager.always-on-top + FloatingNavigator.test.tsx).
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #106 +/- ##
==========================================
+ Coverage 66.89% 67.18% +0.28%
==========================================
Files 133 138 +5
Lines 4308 4376 +68
Branches 1172 1195 +23
==========================================
+ Hits 2882 2940 +58
- Misses 1212 1218 +6
- Partials 214 218 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Floating Navigator shortcut configuration and always-on-top broadcast plumbing, then restructures settings into sectioned web and Electron layouts with new focused preference components. ChangesFloating Navigator settings: always-on-top sync, shortcut config, and settings page reorganization
Sequence Diagram(s)sequenceDiagram
participant SettingsUI as Settings Renderer
participant MainProcess as WindowManager (main)
participant FloatingPreload as preload-floating
participant FloatingUI as FloatingNavigator DOM
SettingsUI->>MainProcess: electronAPI.floatingPanels.setFloatingNavigatorAlwaysOnTop(false)
MainProcess->>MainProcess: BrowserWindow.setAlwaysOnTop(false)
MainProcess->>FloatingPreload: typedSend("floating-window-always-on-top-changed", {alwaysOnTop: false})
FloatingPreload->>FloatingUI: dispatchEvent(CustomEvent "floating-window-always-on-top-changed")
FloatingUI->>FloatingUI: useInitialEffect listener: setIsAlwaysOnTop(false)
sequenceDiagram
participant FloatingNavigatorSettings as FloatingNavigatorSettings
participant useShortcutCapture as useShortcutCapture hook
participant PreloadBridge as electronAPI.floatingPanels
participant MainIPC as main.ts IPC handler
participant applyShortcutRebind as applyShortcutRebind
FloatingNavigatorSettings->>useShortcutCapture: capture(accelerator)
useShortcutCapture->>useShortcutCapture: optimistic setShortcut(accelerator)
useShortcutCapture->>PreloadBridge: setFloatingNavigatorShortcut(accelerator)
PreloadBridge->>MainIPC: IPC floating-config-set-shortcut
MainIPC->>applyShortcutRebind: applyShortcutRebind(rebinder, id, accelerator, previous)
applyShortcutRebind-->>MainIPC: true / false (conflict)
MainIPC-->>PreloadBridge: boolean result
PreloadBridge-->>useShortcutCapture: resolved boolean
alt resolved false (conflict)
useShortcutCapture->>useShortcutCapture: rollback to lastGoodShortcutRef + onError(CONFLICT_MESSAGE)
else resolved true
useShortcutCapture->>useShortcutCapture: update lastGoodShortcutRef
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
electron/__tests__/ipc-contract.test.ts (1)
150-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a max-length boundary assertion for this new shortcut schema test.
This case currently checks type/arity only; it should also assert that over-limit accelerators are rejected.
Proposed test addition
it('accepts empty string (disable shortcut) for floating-config-set-shortcut', () => { const setShortcut = IPC_ARG_SCHEMAS['floating-config-set-shortcut'] // Empty string is the "disable the global toggle" sentinel. expect(() => setShortcut.parse([''])).not.toThrow() expect(() => setShortcut.parse(['CommandOrControl+3'])).not.toThrow() + expect(() => setShortcut.parse(['x'.repeat(65)])).toThrow(ZodError) // A malicious renderer cannot smuggle a non-string past the trust boundary. expect(() => setShortcut.parse([null])).toThrow(ZodError) expect(() => setShortcut.parse([])).toThrow(ZodError) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/__tests__/ipc-contract.test.ts` around lines 150 - 159, The new shortcut schema test in IPC_ARG_SCHEMAS['floating-config-set-shortcut'] only checks type and arity; extend it to also verify that overly long accelerator strings are rejected. Update the existing it('accepts empty string...') case to include a max-length boundary assertion for setShortcut.parse, using a clearly over-limit string, so the test covers both valid shortcuts and the length guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/main.ts`:
- Around line 1476-1490: The rollback in this conflict path is using the stale
braindump.shortcut mirror instead of the canonical shortcut value. In the
shortcut handling block around applyShortcutRebind() and the shortcutManager
flow, read and pass the canonical shortcuts.toggleBrainDump value for rollback
so a conflict restores the real live binding, not an outdated mirror entry.
Ensure the fallback path in this update logic keeps the canonical key as the
source of truth even when braindump.shortcut is out of sync.
In `@electron/utils/applyShortcutRebind.ts`:
- Around line 48-63: The rollback in applyShortcutRebind() is not verified, so a
failed or substituted restore can still leave the shortcut state inconsistent.
Update the rollback path to check the return value/effective registration from
the second rebinder.updateShortcuts() call, using the existing previous,
didRegister, and getRegisteredShortcuts() logic, and only return false once
you’ve confirmed the prior accelerator was actually restored; otherwise
handle/report the restore failure explicitly.
In `@plans/2026-06-24-settings-regroup-and-floating-shortcut.md`:
- Line 14: Resolve the markdownlint failures in the plan doc by cleaning up
blockquote spacing, code-span spacing, fenced code block language annotations,
and table formatting so the document passes MD028/MD038/MD040/MD058. Update the
affected markdown sections in the plan content, especially the examples around
the shortcut flow and table rows, and ensure all fenced blocks use an explicit
language tag via the relevant markdown sections rather than leaving them
unlabeled.
In `@src/components/settings/SoundPreferences.tsx`:
- Around line 139-142: The `SoundPreferences` timbre audition path is firing an
async `previewTimbre()` without handling failures, which can leave rejected
promises unhandled while Redux still updates. Update the timbre selection flow
in `SoundPreferences` so the `previewTimbre(timbre.id, soundVolume)` call is
awaited or wrapped in explicit error handling, and make sure any failure from
`previewTimbre` is caught and logged/handled locally without breaking the user
gesture path.
In `@src/hooks/useShortcutCapture.ts`:
- Around line 84-91: The capture flow in useShortcutCapture treats persist()
returning undefined as a successful save, which incorrectly commits an
accelerator that was never persisted. Update the persist result handling in
capture() so that the branch for ok == null (or otherwise undefined) also
reverts to lastGoodShortcutRef.current and calls onError, similar to the
existing KEYBINDING_CONFLICT_MESSAGE path. Keep the success assignment to
lastGoodShortcutRef.current only for a definite true result.
---
Nitpick comments:
In `@electron/__tests__/ipc-contract.test.ts`:
- Around line 150-159: The new shortcut schema test in
IPC_ARG_SCHEMAS['floating-config-set-shortcut'] only checks type and arity;
extend it to also verify that overly long accelerator strings are rejected.
Update the existing it('accepts empty string...') case to include a max-length
boundary assertion for setShortcut.parse, using a clearly over-limit string, so
the test covers both valid shortcuts and the length guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c2832235-cf55-488f-bb34-2f0d0c6a1b88
📒 Files selected for processing (40)
e2e/electron/floating-always-on-top-sync.spec.tselectron/WindowManager.tselectron/__tests__/WindowManager.always-on-top.test.tselectron/__tests__/applyShortcutRebind.test.tselectron/__tests__/ipc-contract.test.tselectron/ipc/ipc-schemas.tselectron/main.tselectron/preload-floating.tselectron/preload.tselectron/types/electron-api.d.tselectron/types/ipc.tselectron/utils/applyShortcutRebind.tsplans/2026-06-24-settings-regroup-and-floating-shortcut.mdsrc/app/settings/page.tsxsrc/components/ThemeSelector.tsxsrc/components/electron/AppUpdateSettings.tsxsrc/components/electron/BrainDumpAppearance.test.tsxsrc/components/electron/BrainDumpAppearance.tsxsrc/components/electron/BrainDumpSettings.tsxsrc/components/electron/ElectronSettingsPage.test.tsxsrc/components/electron/ElectronSettingsPage.tsxsrc/components/electron/FloatingNavigatorSettings.test.tsxsrc/components/electron/FloatingNavigatorSettings.tsxsrc/components/electron/FloatingPanelToggle.test.tsxsrc/components/electron/FloatingPanelToggle.tsxsrc/components/electron/FloatingWindowSettings.test.tsxsrc/components/electron/FloatingWindowSettings.tsxsrc/components/electron/StartupWindowSettings.tsxsrc/components/floating-navigator/FloatingNavigator.test.tsxsrc/components/floating-navigator/FloatingNavigator.tsxsrc/components/settings/PreferencesSettings.test.tsxsrc/components/settings/PreferencesSettings.tsxsrc/components/settings/SettingsSection.tsxsrc/components/settings/SoundPreferences.test.tsxsrc/components/settings/SoundPreferences.tsxsrc/components/settings/TaskPreferences.test.tsxsrc/components/settings/TaskPreferences.tsxsrc/hooks/useFloatingPanelPreference.tssrc/hooks/useShortcutCapture.test.tssrc/hooks/useShortcutCapture.ts
💤 Files with no reviewable changes (4)
- src/components/electron/FloatingWindowSettings.test.tsx
- src/components/settings/PreferencesSettings.tsx
- src/components/electron/FloatingWindowSettings.tsx
- src/components/settings/PreferencesSettings.test.tsx
Resolve the actionable findings from CodeRabbit's review of PR #106: - main.ts (Major): source the BrainDump rebind rollback target from the CANONICAL shortcuts.toggleBrainDump, not the braindump.shortcut mirror, so a conflict restores the real live binding even when the generic keybind UI rebound the canonical key without updating the mirror. - preload.ts (Major): getFloatingNavigatorShortcut now surfaces a load failure instead of returning '' — '' is the real "intentionally unbound" value, so masking an IPC error as '' would seed the wrong shortcut into rollback state. The sole consumer already catches and shows "Failed to load shortcut". - applyShortcutRebind.ts (Major): verify the rollback restore via a shared restorePreviousOrThrow helper and throw when it fails, so a conflict can't silently leave persisted config and the live registration out of sync. (updateShortcuts({id:''}) returns true, so empty-previous never spuriously throws — confirmed against ShortcutManager.updateShortcuts.) - useShortcutCapture.ts (Minor): treat persist()->undefined (no desktop bridge) as a no-op revert, not a successful save, so an un-persisted accelerator is never committed as the new last-good rollback target. - ipc-contract.test.ts (Nitpick): assert an over-length accelerator is rejected. - plan doc (Minor): markdownlint — tag the 4 ASCII diagram fences as `text` and fix the MD028 blockquote blank. Skipped (false positive): the previewTimbre() .catch finding — previewTimbre -> playTimbre wraps its whole body (incl. the await) in a try/catch and is documented "never throws", so a .catch would be unreachable and contradicts the engine's own documented call pattern.
Collaborator
Author
CodeRabbit review — resolution (commit f6bc42e)Thanks for the review. All 6 actionable findings + the nitpick are addressed in
Local gate green on CI-matched Node 24.13.0: |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plans/2026-06-24-settings-regroup-and-floating-shortcut.md (1)
235-235: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the MD038 code-span spacing at Line 235.
The code span has an internal trailing space (
requested===''), which violates markdownlint MD038. Remove the space before the closing backtick.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plans/2026-06-24-settings-regroup-and-floating-shortcut.md` at line 235, The markdown table entry in the settings regroup plan has a code span spacing issue: the `requested==='' ` token contains an extra trailing space that triggers MD038. Update the same table row by removing the space inside that inline code span so the `requested`, `bound`, and conflict-check text remain unchanged, and verify the surrounding note still reads correctly.Source: Linters/SAST tools
🧹 Nitpick comments (1)
src/hooks/useShortcutCapture.test.ts (1)
64-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten the no-error assertion.
not.toHaveBeenCalledWith(KEYBINDING_CONFLICT_MESSAGE)only rules out the conflict copy; the test would still pass if the hook erroneously surfaced the genericSHORTCUT_SAVE_FAILED_MESSAGE. Since the no-bridge branch should emit only the initialonError(null), assert that explicitly.♻️ Stronger assertion
expect(result.current.shortcut).toBe('Alt+Space') - expect(onError).not.toHaveBeenCalledWith(KEYBINDING_CONFLICT_MESSAGE) + expect(onError).toHaveBeenCalledTimes(1) + expect(onError).toHaveBeenCalledWith(null)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useShortcutCapture.test.ts` around lines 64 - 67, The test in useShortcutCapture.test should assert the exact no-error behavior for the no-bridge path, not just exclude KEYBINDING_CONFLICT_MESSAGE. Update the expectation around onError so it verifies the hook only emits the initial null/error-clear call and does not surface SHORTCUT_SAVE_FAILED_MESSAGE or any other error; use the existing useShortcutCapture and onError assertions to make the intent explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@plans/2026-06-24-settings-regroup-and-floating-shortcut.md`:
- Line 235: The markdown table entry in the settings regroup plan has a code
span spacing issue: the `requested==='' ` token contains an extra trailing space
that triggers MD038. Update the same table row by removing the space inside that
inline code span so the `requested`, `bound`, and conflict-check text remain
unchanged, and verify the surrounding note still reads correctly.
---
Nitpick comments:
In `@src/hooks/useShortcutCapture.test.ts`:
- Around line 64-67: The test in useShortcutCapture.test should assert the exact
no-error behavior for the no-bridge path, not just exclude
KEYBINDING_CONFLICT_MESSAGE. Update the expectation around onError so it
verifies the hook only emits the initial null/error-clear call and does not
surface SHORTCUT_SAVE_FAILED_MESSAGE or any other error; use the existing
useShortcutCapture and onError assertions to make the intent explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d1298131-c1e5-444d-8851-b6d1209a1e36
📒 Files selected for processing (8)
electron/__tests__/applyShortcutRebind.test.tselectron/__tests__/ipc-contract.test.tselectron/main.tselectron/preload.tselectron/utils/applyShortcutRebind.tsplans/2026-06-24-settings-regroup-and-floating-shortcut.mdsrc/hooks/useShortcutCapture.test.tssrc/hooks/useShortcutCapture.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- electron/tests/applyShortcutRebind.test.ts
- electron/utils/applyShortcutRebind.ts
- electron/tests/ipc-contract.test.ts
- electron/main.ts
- src/hooks/useShortcutCapture.ts
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
2 participants
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.
Summary
Regroups Settings into concern-based sections, adds a Floating Navigator inline
toggle-shortcut, and keeps the floating pin in sync across windows.
(
Tasks/Sound/Appearance), each under a real<section>header(
SettingsSection). The old headerlessPreferencesSettingscard is retired.Preferences; they now live only in the Electron settings page.
(
FloatingNavigatorSettings+useShortcutCapture), plus the tray acceleratordisplay and the Zod IPC schema for the new channel.
useShortcutCapturehook (VSCode-style chord capture) shared by bothshortcut rows.
applyShortcutRebindreads the effective binding back via
getRegisteredShortcuts()and rejects asilently substituted fallback as a conflict, restoring the previous accel so
the UI never shows a key the user did not choose.
the in-window toggle broadcasts
floating-window-always-on-top-changed; thefloating preload re-dispatches it as a DOM
CustomEventso the pin buttonre-syncs its
aria-pressed.QA
Everything runnable locally is green:
pnpm validate— unit + lint + build + typecheck + theme → 🟢pnpm test:electron— 320 pass / 33 files 🟢 (incl.WindowManager.always-on-top= §6d broadcast,
ipc-contract= §6b channels,applyShortcutRebind4 tests = §6e,ShortcutManager)Coverage by area:
applyShortcutRebind.test.ts— 4 tests incl. "rejects a silently substituted fallback and restores the previous binding" (full unit coverage)FloatingNavigatorSettings+useShortcutCapture(7 tests); main viaipc-contract+ tray (in the 320)ElectronSettingsPagenull-on-web guard; the 3 web panels carry no unstable-depuseEffect(no render loop); CI web E2E walks the routee2e/electron/floating-always-on-top-sync.spec.ts, gated by this PR's CI electron E2EHonest residual (structurally not locally provable — release-time)
CSP blocks the branch renderer, the packaged app loads the prod renderer, and the
prod-build harness floating window is signed out (no pin button). The new seam
spec proves the real main→preload→DOM IPC hop; the visual end-to-end is
release-time packaged native QA — same precedent as keep-on-top feat(electron): Keep-on-top preference for Floating Navigator + BrainDump #92
("native always-on-top smoke OWED at release").
Notes
shortcuts.toggleFloatingNavigatordirectly — nofloating.shortcutmirror.Strictly avoids drift; behavior identical-or-better than the mirrored sketch.
unbumped, the release bump is a separate
chore(release)+ lightweightv*tag).Decision for this cycle: after merge, bump + cut an Electron release so the
main-process halves (§6d / §6e / §6b tray+Zod) reach packaged users now. The
release-time packaged native QA runs at that tag.
Test plan
pnpm validate🟢pnpm test:electron320 🟢e2e.electron(incl. newfloating-always-on-top-syncseam spec)e2e.webSummary by CodeRabbit
New Features
Bug Fixes
Tests