desktop: declare the menu bar, and fold its chords into the keyboard registry - #123
Merged
Conversation
…registry `tauri.conf.json` set no menu, so Tauri installed `Menu::default()` and a dozen chords came with it — ⌘Q ⌘W ⌘M ⌘H ⌥⌘H ⌘Z ⇧⌘Z ⌘X ⌘C ⌘V ⌘A ⌃⌘F. They were live in the window and invisible twice over. Nothing enumerates that default, so no documentation could list them; and AppKit dispatches a menu key equivalent inside `NSApplication.sendEvent`, before the key window's responder chain, so they never reach the webview's keydown and the keyboard registry could not observe them either. Since #118 every *binding* is discoverable by construction — `shortcuts.test.ts` fails on one no row documents — but these are not bindings, so they sat outside that guarantee entirely. K1 promises a keyboard path that is findable. So the menu is B2's own data now. `crates/b2-desktop/src/menu.rs` holds one table — sections, items, and the chord macOS gives each — with two readers: `build`, which is what the window gets, and `chords`, which the new `menu_chords` command hands the UI. The items stay `PredefinedMenuItem`s deliberately: the Edit menu is load-bearing rather than decorative, since those native items are what route cut/copy/paste into the webview. The consequence is that B2 doesn't *choose* these accelerators — muda assigns them and exposes no getter — so the table restates them, and says so. Two departures from the default, neither touching a chord: its Window menu repeats Close Window (⌘W), which already lives in File, and its Help menu is empty on macOS. On the UI side `ui/src/menukeys.ts` is the third keyboard, beside bindings.ts (B2's own) and editorkeys.ts (CodeMirror's). It mirrors the host's declaration for the two jobs a runtime fetch can't do — the suite's gate runs in node with no host to ask, and the sheet has to paint before the first `invoke` resolves — and the mirror is checked against the host at every boot (`menuDrift`), the "change them together" posture `WRITE_CONFLICT_MESSAGE` and `VAULT_CHANGED_EVENT` already use across this seam. What the reader *sees* comes from the host: render.ts passes `state.menuChords` into `shortcuts()`, so the sheet's new "The menu bar" group is the menu the app installed, not the UI's copy of it. The gate is `menuOverlaps`, not more rows in `conflicts()`, and the difference is the point. `conflicts()` asks a same-scope question, because scope is how an inner surface legitimately answers first — the rename field's Esc before the overlay cascade, the Settings rail's ⌃Tab before the Tab trap. Against the menu that move buys nothing: the keystroke is taken before the webview is consulted, so an editor-scoped ⌘Z is not "nearer the user", it is dead. So the comparison ignores scope entirely. Nothing collides today; `menukeys.test.ts` proves the check can fail rather than only that it passes. One thing the new enumeration surfaces, pinned as its own case: the menu takes ⌘Z, ⇧⌘Z and ⌘A from CodeMirror, which binds all three. The note editor's undo, redo and select-all are therefore the webview's native ones rather than CodeMirror's history and selection commands. That was already true; there was nowhere to write it down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015SJgCnKnUTKNHVsSxrXq7h
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe desktop host now defines an explicit native menu, exposes its accelerators through Tauri, and supplies them to UI keyboard validation and rendering. The UI detects menu conflicts and mirror drift, while tests and documentation cover the integration. ChangesMenu chord integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant AppBoot
participant TauriHost
participant KeyboardState
participant SettingsPanel
AppBoot->>TauriHost: invoke menu_chords
TauriHost-->>KeyboardState: return MenuChord[]
KeyboardState->>KeyboardState: run menuDrift
KeyboardState->>SettingsPanel: provide state.menuChords
SettingsPanel->>SettingsPanel: build shortcuts(state.menuChords)
SettingsPanel-->>AppBoot: render host menu rows
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ui/src/main.ts`:
- Around line 4103-4107: Update loadMenuChords so that after assigning the
fetched chords to state.menuChords, it triggers the existing keyboard-panel
render/update mechanism, ensuring an already-open panel refreshes when the
asynchronous host response arrives; apply the same behavior to the corresponding
path around the additional referenced location without changing the initial boot
flow.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 818a8cf7-e669-40ea-ba78-672f979296b8
📒 Files selected for processing (17)
CLAUDE.mdcrates/b2-desktop/CLAUDE.mdcrates/b2-desktop/src/commands.rscrates/b2-desktop/src/main.rscrates/b2-desktop/src/menu.rsdocs/design/invariants.mdui/src/api.tsui/src/bindings.tsui/src/main.tsui/src/menukeys.test.tsui/src/menukeys.tsui/src/render.test.tsui/src/render.tsui/src/shortcuts.test.tsui/src/shortcuts.tsui/src/state.tsui/src/types.ts
Review catch (PR #123). `boot` fires `loadMenuChords` without awaiting it, and `wireEvents` has already bound ⌘, by then — so Settings can be open before the host answers, and the assignment to `state.menuChords` had no repaint behind it. The reader would sit looking at menukeys.ts's mirror, which is the one thing the host list exists to replace. Guarded on `settingsOpen` rather than unconditional: during boot the answer normally lands *before* the first `render()` (a static-data IPC against a vault read and a note list), and painting there would flash the empty shell ahead of the vault. The guard is false at that point, so the boot flow is untouched. Worth noting what the repaint costs when nothing is wrong: nothing. The mirror and the host agree in the healthy case, so the HTML is identical and `paintModal`'s memo skips the swap entirely — the only case where the DOM actually changes is drift, which is the case worth showing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015SJgCnKnUTKNHVsSxrXq7h
This was referenced Jul 29, 2026
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.
tauri.conf.jsonset no menu, so Tauri installedMenu::default()and adozen chords came with it — ⌘Q ⌘W ⌘M ⌘H ⌥⌘H ⌘Z ⇧⌘Z ⌘X ⌘C ⌘V ⌘A ⌃⌘F.
They were live in the window and invisible twice over. Nothing enumerates
that default, so no documentation could list them; and AppKit dispatches
a menu key equivalent inside
NSApplication.sendEvent, before the keywindow's responder chain, so they never reach the webview's keydown and
the keyboard registry could not observe them either. Since #118 every
binding is discoverable by construction —
shortcuts.test.tsfails onone no row documents — but these are not bindings, so they sat outside
that guarantee entirely. K1 promises a keyboard path that is findable.
So the menu is B2's own data now.
crates/b2-desktop/src/menu.rsholdsone table — sections, items, and the chord macOS gives each — with two
readers:
build, which is what the window gets, andchords, which thenew
menu_chordscommand hands the UI. The items stayPredefinedMenuItems deliberately: the Edit menu is load-bearing ratherthan decorative, since those native items are what route cut/copy/paste
into the webview. The consequence is that B2 doesn't choose these
accelerators — muda assigns them and exposes no getter — so the table
restates them, and says so.
Two departures from the default, neither touching a chord: its Window
menu repeats Close Window (⌘W), which already lives in File, and its Help
menu is empty on macOS.
On the UI side
ui/src/menukeys.tsis the third keyboard, besidebindings.ts (B2's own) and editorkeys.ts (CodeMirror's). It mirrors the
host's declaration for the two jobs a runtime fetch can't do — the
suite's gate runs in node with no host to ask, and the sheet has to paint
before the first
invokeresolves — and the mirror is checked againstthe host at every boot (
menuDrift), the "change them together" postureWRITE_CONFLICT_MESSAGEandVAULT_CHANGED_EVENTalready use acrossthis seam. What the reader sees comes from the host: render.ts passes
state.menuChordsintoshortcuts(), so the sheet's new "The menu bar"group is the menu the app installed, not the UI's copy of it.
The gate is
menuOverlaps, not more rows inconflicts(), and thedifference is the point.
conflicts()asks a same-scope question,because scope is how an inner surface legitimately answers first — the
rename field's Esc before the overlay cascade, the Settings rail's ⌃Tab
before the Tab trap. Against the menu that move buys nothing: the
keystroke is taken before the webview is consulted, so an editor-scoped
⌘Z is not "nearer the user", it is dead. So the comparison ignores scope
entirely. Nothing collides today;
menukeys.test.tsproves the check canfail rather than only that it passes.
One thing the new enumeration surfaces, pinned as its own case: the menu
takes ⌘Z, ⇧⌘Z and ⌘A from CodeMirror, which binds all three. The note
editor's undo, redo and select-all are therefore the webview's native
ones rather than CodeMirror's history and selection commands. That was
already true; there was nowhere to write it down.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_015SJgCnKnUTKNHVsSxrXq7h
Summary by CodeRabbit
New Features
Bug Fixes
Tests