Skip to content

feat(shortcuts): registry + conventional ⌘/Ctrl defaults with Alt aliases (P1) - #9555

Merged
chenmingwei23 merged 1 commit into
mainfrom
feat/shortcuts-registry-conventional-defaults
Sep 9, 2026
Merged

feat(shortcuts): registry + conventional ⌘/Ctrl defaults with Alt aliases (P1)#9555
chenmingwei23 merged 1 commit into
mainfrom
feat/shortcuts-registry-conventional-defaults

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

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, and useKeyboardShortcuts asks the registry which id a keystroke is (matchShortcutEvent) instead of spelling e.code === 'KeyN' && e.shiftKey per 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:

action new alias
New session ⌘N / Ctrl+N ⌥⇧N / Alt+Shift+N
Close session ⌘W / Ctrl+W ⌥⇧W / Alt+Shift+W
Keyboard shortcuts help ⌘/ / Ctrl+/ ⌥K / Alt+K
Settings ⌘, (unchanged) / Ctrl+, ⌥, / Alt+,

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 from CmdOrCtrl+N to CmdOrCtrl+Alt+N so ⌘N reaches the renderer (this was the chord Joseph hit). On Windows/Linux the Window menu is written out with Close on Ctrl+Shift+W (VS Code / Chrome convention) instead of the stock windowMenu role'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)

  • P2: ⌘↩ as the alternate busy action (queue ↔ steer) + a merged Send key settings card.
  • P3: Settings → Keyboard record / clear / reset UI, conflict detection, families' modifier rebindable.
  • The ⌘1–9 question (sessions vs. remote instances, which own ⌘1–6 today) — flagged for the owner in the issue.

Screenshots

Fixture harness (scripts/capture-shortcuts-registry.mjs, real built SPA + stubbed API), same script on origin/main and on this branch.

before (main) after (this branch)
Alt+K reference, light modal before light modal after light
Alt+K reference, dark modal before dark modal after dark
Settings → Shortcuts, light settings before light settings after light
Alt+K reference, desktop app (macOS host), light / dark desktop mac light desktop mac dark
Alt+K reference, desktop app (macOS host), footer uncropped, light / dark desktop mac footer light desktop mac footer dark
Alt+K reference, desktop app (Windows/Linux host), light / dark desktop linux light desktop linux dark
Settings → Shortcuts, dark settings before dark settings after dark

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 by electron/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 with SHORTCUT_LABEL_KEY, every registry id 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 now meta + alias.
  • src/test/extensionSeams.test.tsx: the RESERVED_PANEL_CODES drift guard now unions the registry's non-shift Alt chords (they are no longer code === literals in the handler).
  • electron/test/app-menu.test.js: no menu item claims Cmd/Ctrl+N or Cmd/Ctrl+W; New Connection Window… on CmdOrCtrl+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), eslint on touched files, I18N_BASE_REF=origin/main node scripts/i18n-check.mjs (19 checks PASS), node --check on 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_globally changes its placeholder from {{mod}} + K to {{chord}} in every catalog (text-level replacement) because the reference chord is now ⌘/ with ⌥K as alias.

Review responses (head ea88090d8)

  • Design — Ctrl+W habit collision (Ctrl+W closed the window in the previous Win/Linux desktop release; confirmCloseSession defaults 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.
  • First Principles — override write API with zero consumers: applied. setShortcutOverride / clearShortcutOverride are 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.
  • First Principles — second chord-helper family beside quickSearchShortcut.ts: not applied here, deliberately. The registry's Chord carries ctrl (literal Control — ⌃G, ⌃1–9 on macOS) which the quick-search Chord cannot express, and the two matchers differ on Numpad digits and punctuation. Re-pointing panelToggleShortcuts.ts / useCommandPalette.ts onto 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.
  • First Principles — ⌘W / ⌘/ / Ctrl+, are convention-only: correct that only ⌘N has a named report. They are in this PR because the design comment on Make built-in keyboard shortcuts rebindable — fixed Option/Ctrl chords collide with macOS cursor movement #4608 proposes them as one coherent ⌘-scheme and every old chord stays live as an alias; whether to keep them is the owner's call on the issue, and dropping any one is a one-line registry edit.
  • UX — muted chords mean two things: applied. A browser-reserved chord demoted in a browser host now carries an inline DESKTOP APP tag on the chord itself (reusing the existing desktop_app catalog 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.
  • UX — "Ctrl + / always works" overpromises: applied. Superseded by round 3 below: the footer now names only the unconditional chord.
  • UX — evidence gaps: desktop-app (macOS host) screenshots added, light + dark (row above). The native Electron menus cannot be rendered by the harness; the menu shape is asserted by app-menu.test.js.
  • UX (round 2) — explainer lands under the wrong row: applied. The browser-reserved hint now renders directly under the last demoted row ("Close session"), in both the modal and Settings → Shortcuts, instead of at the group's end. The DESKTOP APP tag stays.
  • UX (round 2) — mac desktop footer cropped: applied; uncropped footer captures added (row above) showing "Enable shortcuts", "⌘/ or ⌥K always works" and the Ctrl-digits toggle.
  • UX (round 2) — Electron native menus and the ⌘W confirm unseen: not capturable by the harness (native menus and window.confirm are OS surfaces outside the page). The menu shape is pinned by electron/test/app-menu.test.js; the confirm is the existing close_this_session prompt, pinned by the new hook tests.
  • UX (round 2) — "New chat" vs "Close session" wording: not changed here. Both labels deliberately reuse the sidebar's own catalog strings (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.
  • UX (round 3) — "always works" must not name a chord that yields: applied. The footer and the Settings toggle description now list only the unconditional Option/Alt alias ("Alt + K always works" / "⌥K always works"); the ⌘/ primary stays on the row itself. If a P3 rebind removes the alias, the line falls back to whatever primary is bound.
  • UX (round 3) — tag self-sufficiency + hint plural: applied. Tag is now DESKTOP APP ONLY (new key desktop_app_only, 13 catalogs + en-XA); browser_reserved_hint reworded to plural ("these shortcuts… the alternatives") in every catalog.
  • i18n render gate (E2E job): the "or" + tag pair in one inline run was flagged as 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.
  • Electron native-menu screenshots: still not producible by the harness (see round 2).
  • UX (round 4): applied the two actionable items — Windows/Linux desktop-host screenshots added (Ctrl+N / Ctrl+W leading, no tag), and the redundant title tooltip 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.
  • GPT (blocking, security-class) — Ctrl/Cmd+W silently retires an armed goal loop: applied. The confirm gate no longer reads slot.running (false between loop cycles); it reuses the sidebar's inferLane with 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 before deleteSlot. Test added for the goal-loop-between-cycles case.

@CrysisDeu
CrysisDeu requested a review from a team September 8, 2026 21:41
@CrysisDeu
CrysisDeu requested a review from a team as a code owner September 8, 2026 21:41
@CrysisDeu
CrysisDeu requested a review from buluoray September 8, 2026 21:41
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of ea88090d81a37de9cd8746aa25fd4502f8084f2d — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of ea88090d81a37de9cd8746aa25fd4502f8084f2d — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have everything I need. All counts are run: 0 product writers of mc-shortcut-overrides (grep over website/ excluding tests), 5 twinned chord symbols between quickSearchShortcut.ts and the new registry, 442 sibling capture-*.mjs scripts making the harness repo-standard, and the one rewritten pin (settingsShortcut.test.tsx Ctrl+, rejection) is declared with the pin's own reason addressed.

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 quickSearchShortcut.ts.

Not justified as shipped

  1. Hand-edited mc-shortcut-overrides rebinds/unbinds shortcuts today — zero consumers: grep found 0 product writers of the key or SHORTCUT_OVERRIDES_EVENT; its only support is "so the P3 Settings UI is a form, not a migration".
  2. The registry's Chord/chordMatchesEvent/isValidChord/normalizeChord/eventKeyToken — duplicate of website/src/lib/quickSearchShortcut.ts: 5 twinned symbols, second spelling acknowledged and deferred to P3.

What this change ships

Inventory (10 items) — 8 justified

Intent: 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 capture-*.mjs siblings) also rides along, declared.

  1. ⌘N / Ctrl+N now starts a new session; ⌥⇧N stays as a muted alias — justified
  2. ⌘W / Ctrl+W now closes the session; ⌥⇧W alias — justified
  3. ⌘/ / Ctrl+/ now opens the shortcuts reference; ⌥K alias; footer copy follows — justified
  4. Ctrl+, now opens Settings on Windows/Linux (rewrites the pinned rejection; the pin's reason — the shell menu owns it — is answered in the diff and declared) — justified
  5. Closing a busy session with ⌘W/Ctrl+W now always asks first, whatever the confirm setting — justified
  6. Desktop menu: "New Connection Window…" moved ⌘N → ⌘⌥N so ⌘N reaches the page — justified
  7. Desktop Windows/Linux: window close moved Ctrl+W → Ctrl+Shift+W (Window menu written out) — justified
  8. In a browser the reserved ⌘N/⌘W rows demote: alias leads, muted chord + hint line (one new i18n key) — justified
  9. Hand-edited localStorage overrides are honored with no UI to set them — zero consumers (0 writers counted)
  10. One registry module now defines every chord, adding a second chord type/matcher/validator family — duplicate of website/src/lib/quickSearchShortcut.ts (deferred to P3 with a named reason)

Watch

  • Override machinery (loadShortcutOverrides, isValidChord, normalizeChord, the override branch of resolveShortcut, useShortcutBindings' two listeners, both constants): every producer was already deleted in a prior review round; the surviving read side runs on every keystroke to return {}. Zero option today costs nobody anything; kept only "so we can later". Reversible, hence not a blocker. Clears when: the P3 Settings → Keyboard UI lands as the first counted writer, or the read side moves to that PR.
  • Second chord family: quickSearchShortcut.ts twins are live-consumed (panelToggleShortcuts.ts, usePanelToggleShortcuts.ts, useCommandPalette.ts), so both spellings must now stay correct in parallel; the divergence is real (ctrl flag, Numpad handling). Clears when: P3 deletes the quick-search twins as the Make built-in keyboard shortcuts rebindable — fixed Option/Ctrl chords collide with macOS cursor movement #4608 plan records.

Subtractions

  • Drop the override branch: resolveShortcut returns entry.defaults[platform] unconditionally, deleting loadShortcutOverrides/isValidChord/normalizeChord/SHORTCUT_OVERRIDES_KEY/SHORTCUT_OVERRIDES_EVENT (~70 lines) and shrinking useShortcutBindings to a constant; P3 reintroduces them beside their only writer.

[FIRST-PRINCIPLES-REVIEWED] ea88090

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of ea88090d81a37de9cd8746aa25fd4502f8084f2d — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • Chat/session vocabulary collision on the PR's two headline rows: the blind reader — "I can't tell whether a 'chat' and a 'session' are the same thing" — and the PR body itself calls the row labeled "New chat" "New session". Every reference viewer hits it, comprehension friction, every time. Smallest fix: one noun for the pair (pages.chatSidebar.new_chat / close_session).
  • On Win/Linux desktop, Ctrl+W closed the window last release and now closes an idle session with no confirmation (confirmCloseSession defaults off; the diff's guard covers non-idle only) — the reader's exact fear: "it might throw away what I'm doing". Low loss but habitual hands will hit it; consider confirming or offering reopen on the idle path too.

Evidence gaps

  • The native menu changes ("New Connection Window…" moving to ⌘⌥N; Win/Linux Window › Close on Ctrl+Shift+W) appear in no committed screenshot — the harness can't render native menus, so a manual screenshot of both menus would close this.

[UX-REVIEWED] ea88090

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed ea88090d81a37de9cd8746aa25fd4502f8084f2d — this comment is updated in place on each push.

Review details

No 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 new-chat/close-chat is the intended action, not a wrong outcome — the advertised alias (Alt+Shift+N/W) works either way, and close-chat guards non-idle sessions behind confirm while an idle close is losslessly reopenable. The only residual is a cosmetic label being conservative; there is no crash, data loss, or security consequence, and (c) does not resolve to an observable wrong outcome. Its own confidence line is "low," and it does not reach 80 on independent re-derivation.

[OPUS-REVIEWED] ea88090

Verdict parsed from the review's SHA-scoped output markers for commit ea88090d81a37de9cd8746aa25fd4502f8084f2d.

False positive or not applicable? A repository writer can comment:
/ai-review override fable ea88090d81a37de9cd8746aa25fd4502f8084f2d: <one-sentence reason>

@CrysisDeu
CrysisDeu force-pushed the feat/shortcuts-registry-conventional-defaults branch from ed13b72 to 1159c9c Compare September 8, 2026 22:07
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of ea88090d81a37de9cd8746aa25fd4502f8084f2d and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] ea88090

False positive or not applicable? A repository writer can comment:
/ai-review override gpt ea88090d81a37de9cd8746aa25fd4502f8084f2d: <one-sentence reason>

@CrysisDeu
CrysisDeu force-pushed the feat/shortcuts-registry-conventional-defaults branch 4 times, most recently from 73db4c1 to 412dd0e Compare September 8, 2026 23:17
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 8, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/shortcuts-registry-conventional-defaults branch from 412dd0e to 3fe2486 Compare September 8, 2026 23:35
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 8, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/shortcuts-registry-conventional-defaults branch from 3fe2486 to 8c9f054 Compare September 9, 2026 00:16
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 9, 2026
…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.
@CrysisDeu
CrysisDeu force-pushed the feat/shortcuts-registry-conventional-defaults branch from 8c9f054 to ea88090 Compare September 9, 2026 00:40
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 9, 2026
@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 9, 2026
@bolichen97
bolichen97 enabled auto-merge (squash) September 9, 2026 07:22
@chenmingwei23
chenmingwei23 merged commit 8543abf into main Sep 9, 2026
68 of 74 checks passed
@chenmingwei23
chenmingwei23 deleted the feat/shortcuts-registry-conventional-defaults branch September 9, 2026 08:16
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants