feat(shortcuts): registry + conventional ⌘/Ctrl defaults with Alt aliases (P1) - #9555
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS One registry replaces three drifting chord copies; every moved default keeps its old chord live as an alias, so the whole scheme is a one-line-reversible decision. Suggestions
[DESIGN-REVIEWED] ea88090 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of I have everything I need. All counts are run: 0 product writers of First-Principles-Verdict: CONCERNS The override resolver ships with zero writers — nothing in the product can create an override until P3 — and a second chord-helper family lands beside Not justified as shipped
What this change shipsInventory (10 items) — 8 justifiedIntent: let users press the conventional ⌘/Ctrl chords (⌘N = new session, per a named user ask and the #4608 plan) instead of the Option/Alt originals — an ADDITION. Capped at 10; the repo-standard screenshot harness script (442
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] ea88090 |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of The rendering is clean and the blind reader parsed every changed element correctly. I have what I need to adjudicate. UX-Verdict: CONCERNS New chords read correctly cold, but the Electron menu changes ship unscreenshotted and "New chat" beside "Close session" still reads as two different objects. Watch
Evidence gaps
[UX-REVIEWED] ea88090 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsNo findings. The sole candidate claims the browser-reserved primary (⌘N/⌘W/Ctrl+N/Ctrl+W) fires in a browser host despite the "Desktop app only" display tag. Falsified: in mainstream browsers the chord never reaches the page (the tag is accurate), and in an edge host that does deliver it, firing [OPUS-REVIEWED] ea88090 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
ed13b72 to
1159c9c
Compare
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
73db4c1 to
412dd0e
Compare
412dd0e to
3fe2486
Compare
3fe2486 to
8c9f054
Compare
…ases (P1) Refs #4608. One table (src/lib/shortcutRegistry.ts) now owns every built-in chord: per-platform default, legacy aliases, dispatch kind, browser-reserved flag, and user overrides (mc-shortcut-overrides, #4488 pattern generalized). DEFAULT_SHORTCUTS is derived from it and useKeyboardShortcuts asks the registry which id a keystroke is instead of spelling e.code per branch. Conventional defaults, old chord kept as an alias for one release: New session ⌘N / Ctrl+N (alias ⌥⇧N / Alt+Shift+N) Close session ⌘W / Ctrl+W (alias ⌥⇧W / Alt+Shift+W) Shortcuts help ⌘/ / Ctrl+/ (alias ⌥K / Alt+K) Settings ⌘, / Ctrl+, (alias ⌥, / Alt+,) Electron: "New Connection Window…" moves off CmdOrCtrl+N (to CmdOrCtrl+Alt+N) so ⌘N reaches the renderer; Windows/Linux Window menu written out with Close on Ctrl+Shift+W so Ctrl+W is the renderer's. Browser hosts show the alias first for browser-reserved chords with a hint. ⌘/Ctrl chords yield to an embedded terminal and to a handler that already claimed the key. Everything else keeps its chord byte-for-byte; families (digit/letter jumps, MRU, arrows, panel nav, brackets, instances) stay code-driven for now.
8c9f054 to
ea88090
Compare
Refs #4608 — P1 of the shortcut plan in #4608 (comment) (registry + conventional defaults + aliases; no rebind UI yet). Also answers Joseph Dombroski's Slack ask: "command+N seems like it should be new session, not new connection".
What changes
One registry,
website/src/lib/shortcutRegistry.ts. Every built-in chord is one record: default chord per platform (mac/other), legacy aliases, dispatch kind, browser-reserved flag.DEFAULT_SHORTCUTS(the display list the Alt+K modal, Settings → Shortcuts and the nav hover hints read) is now derived from it, anduseKeyboardShortcutsasks the registry which id a keystroke is (matchShortcutEvent) instead of spellinge.code === 'KeyN' && e.shiftKeyper branch. User overrides (mc-shortcut-overrides,null= unbound, change event) are the #4488 panel-toggle pattern generalized; the resolver honours them today so the P3 Settings UI is a form, not a migration.The code-driven FAMILIES (digit/letter jumps, MRU walk, Alt+arrows, panel nav, bracket cycle, instance switcher) keep their contextual handler branches; the registry describes them for display and never claims them.
Conventional defaults (⌘ on macOS / Ctrl elsewhere), old Option/Alt chord kept as an alias for one release:
Everything else is byte-for-byte the same chord. Aliases render muted after an "or" in the modal and in Settings.
Electron (
website/electron/app-menu.js): "New Connection Window…" moves fromCmdOrCtrl+NtoCmdOrCtrl+Alt+Nso ⌘N reaches the renderer (this was the chord Joseph hit). On Windows/Linux the Window menu is written out with Close onCtrl+Shift+W(VS Code / Chrome convention) instead of the stockwindowMenurole's Ctrl+W; macOS keeps the stock role, which has no Close entry. "New Window" keeps ⌘⇧N on macOS.Browser hosts: ⌘N/Ctrl+N and ⌘W/Ctrl+W are browser-reserved (new window / close tab — the page never sees them). In a browser the modal shows the alias first and the reserved chord muted with a hint line; in the desktop app the conventional chord leads. The registry marks these with
browserReserved.Terminal / editor deference: a ⌘/Ctrl registry chord yields to an embedded terminal (Ctrl+W is kill-word, Ctrl+N next-history) and to a handler that already
preventDefaulted (CodeMirror's Ctrl+/), the same rule the bracket chords and panel toggles apply. Alias chords keep their shipped behaviour.Not in this PR (later phases, see the issue comment)
Screenshots
Fixture harness (
scripts/capture-shortcuts-registry.mjs, real built SPA + stubbed API), same script onorigin/mainand on this branch.The desktop-app row fakes the Electron preload bridge (
window.kirocrew) and a macOS platform in the same harness, so it shows the shipped desktop state: ⌘N / ⌘W leading, ⌘ glyphs, no demotion. The Electron menu changes (New Connection Window… on ⌘⌥N, Win/Linux Window › Close on Ctrl+Shift+W) are native menus the harness cannot render; they are pinned byelectron/test/app-menu.test.js. In the browser-host rows the browser-reserved rows (New chat, Close session) show the Alt alias first with ⌘N/⌘W muted plus the hint line; in the desktop app ⌘N / ⌘W lead. Ctrl+/ and Ctrl+, lead with Alt+K / Alt+, muted.Tests (written, not run locally — CI runs them)
src/test/shortcutRegistry.test.ts(new): table invariants (unique ids, label-key parity withSHORTCUT_LABEL_KEY, everyregistryid has a handler action — source-parsed drift guard, no chord collisions across defaults + aliases per platform), key tokens, exact-modifier matching per platform, override load/validate/replace-aliases/unbind.src/test/useKeyboardShortcuts.test.tsx: Ctrl+N / Ctrl+W / Ctrl+/ fire and claim; aliases still fire; superset chords miss; terminal yield; override replaces default + aliases; null override unbinds.src/test/settingsShortcut.test.tsx: Ctrl+, accepted on Windows/Linux, Meta+, rejected; def shape nowmeta+ alias.src/test/extensionSeams.test.tsx: theRESERVED_PANEL_CODESdrift guard now unions the registry's non-shift Alt chords (they are no longercode ===literals in the handler).electron/test/app-menu.test.js: no menu item claims Cmd/Ctrl+N or Cmd/Ctrl+W; New Connection Window… onCmdOrCtrl+Alt+N; Win/Linux Window menu shape with Close on Ctrl+Shift+W; mac keeps the role.Local gates run:
tsc --noEmit -p tsconfig.app.json(clean on touched files),eslinton touched files,I18N_BASE_REF=origin/main node scripts/i18n-check.mjs(19 checks PASS),node --checkon the Electron files.i18n
One new key
components.shortcutsModal.browser_reserved_hint(12 hand-maintained catalogs + en-XA regenerated).pages.settings.shortcutsPanel.turn_keyboard_shortcuts_on_or_off_globallychanges its placeholder from{{mod}} + Kto{{chord}}in every catalog (text-level replacement) because the reference chord is now ⌘/ with ⌥K as alias.Review responses (head
ea88090d8)confirmCloseSessiondefaults off): applied. The ⌘W/Ctrl+W chord now always confirms unless the session is idle (sidebar lane inference — a running turn, a dynamic workflow, an armed goal loop between cycles, running sub-agents or a pending approval all count), regardless of the setting; an idle session honours the setting (it is losslessly reopenable from the sidebar's older-sessions list). The ⌥⇧W alias keeps its shipped behaviour. Tests added.setShortcutOverride/clearShortcutOverrideare deleted; the module ships the read side only (loader + resolver, which the handler and display surfaces consume today). The writer lands with the P3 Settings → Keyboard UI that is its only caller.quickSearchShortcut.ts: not applied here, deliberately. The registry'sChordcarriesctrl(literal Control — ⌃G, ⌃1–9 on macOS) which the quick-searchChordcannot express, and the two matchers differ on Numpad digits and punctuation. Re-pointingpanelToggleShortcuts.ts/useCommandPalette.tsonto the registry matcher is the right end state, but it changes two shipped rebindable surfaces and belongs to P3 (where the panel toggles and Search Everywhere become registry entries and the four twins are deleted). Noted in the Make built-in keyboard shortcuts rebindable — fixed Option/Ctrl chords collide with macOS cursor movement #4608 plan.DESKTOP APPtag on the chord itself (reusing the existingdesktop_appcatalog string), so it no longer reads like a second working alias; legacy aliases stay plain-muted. The group hint line stays as the long-form reason.app-menu.test.js.DESKTOP APPtag stays.window.confirmare OS surfaces outside the page). The menu shape is pinned byelectron/test/app-menu.test.js; the confirm is the existingclose_this_sessionprompt, pinned by the new hook tests.pages.chatSidebar.new_chat/close_session) so the reference and the buttons cannot drift; the chat-vs-session vocabulary split is product-wide and pre-existing, and fixing it in the sidebar would fix it here for free.DESKTOP APP ONLY(new keydesktop_app_only, 13 catalogs + en-XA);browser_reserved_hintreworded to plural ("these shortcuts… the alternatives") in every catalog.fragment/multi-unit; the tag and the footer's "always works" are block elements now (the codebase's own "a div, not a span" pattern), which ends the inline run.titletooltip on the demoted chord removed (the explainer is inline right below). The Ctrl+W habit watch is the Design-lane item already addressed (confirm on a running session; idle sessions are reopenable). Native menu captures: see round 2.slot.running(false between loop cycles); it reuses the sidebar'sinferLanewith the same extras the sidebar computes (goalLoops, workflow runs, sub-agent counts/approvals), so a session in any non-idle lane — Working, Waiting, Needs approval — confirms beforedeleteSlot. Test added for the goal-loop-between-cycles case.