feat(shortcuts): app-wide keyboard shortcuts and the help registry (#243) - #276
Conversation
Issue #243 asked for shortcuts "wherever they make sense" without a listener per page. What existed was three separate conventions: the new-conversation hook had its own idea of a press (`event.code`, refusing Ctrl/Cmd), the project switcher compared `event.key` and shipped its own chord string, and the chat page matched `/` inline. Two spellings of one rule is how a chord starts disagreeing with the hint that advertises it, so the matching, the labels and the catalogue live here from now on — `isShortcutPress`, `shortcutChord`, `SHORTCUTS` — and the components listen through them. Decisions worth naming: - `code` for letter chords, the produced character for punctuation. The handed-over plan matched `?` on `code: "Slash"`, which is dead on a German keyboard: `?` is Shift+ß there and `code: "Slash"` belongs to `-`. Character-matched chords (`?`, `/`, `,`) work on whichever layout the person brought, which is the entire point of a help key. - AltGr is refused by refusing Ctrl+Alt — how Windows reports it — because a bare `altKey` check fires on every `{`, `[`, `}` and `@` a European layout types with it. - Shift must match exactly on physical-key chords and is ignored on character chords: producing `?` requires Shift on every layout that has it, so the character is the whole claim there. - `scope: "global" | "surface"` says who answers. The destinations and `?` are the shortcuts layer's own; Ctrl/Cmd+K, Alt+N, Alt+S, `/` and Esc are answered where they act — the switcher, the chat, the sidebar, dialogs — but are registered here all the same, so the help view cannot document a chord the app does not answer or miss one it does. - Destination entries carry a `path`, not a permission callback: the gate is `canAccessRoute`, the rule the sidebar and the manager guard already use, instead of the plan's second spelling of the same rule. - `Alt+C` for Chat is new. The plan gave the buddy (`Alt+U`) a chord while the sidebar's busiest entry stayed unlabelled, even though `/buddy` is Chat's own second half. Part of #243.
`GlobalShortcuts` is mounted once in the signed-in shell: one window listener answers every `scope: "global"` entry — the seven destinations and `?` — rather than a listener per page, because a shortcut that only works on the page you are already on is not a shortcut. Destinations go through `canAccessRoute` before navigating: a chord is a convenience, never a way around the access policy, so a hire pressing `Alt+P` is refused like a hire typing the URL. The help view filters the same way, since advertising a shortcut the profile may not use is the same leak in a friendlier font. The help itself is `ui/Modal`, so the focus trap, Escape-to-close, scroll lock and focus restoration are the ones every other dialog already has. The chords render as real text — a help nobody can hear is not the help this dialog exists to be (the hover chip stays `aria-hidden`, this does not) — and the surface-owned chords are listed alongside the global ones, because the dialog is the one place to learn they exist. `?` is ignored while any other dialog is open: two `z-50` overlays would otherwise appear at once and a single Escape would close both. `App` mounts the layer only while signed in — the login screen has no destinations to jump between, and the help would advertise routes a visitor cannot reach. Part of #243.
- `SidebarNavLink` takes a `shortcut` and draws `ui/ShortcutHint` next to the label. The entries get their chord from `navigationShortcut(path)` rather than strings typed at the call site, so an entry's hint and the keypress that answers it read the same line of the registry. The chip stays `aria-hidden` and the link's own `title` carries the copy a screen reader hears — once. The icon-only Settings button has no room for a chip, so its title carries `Alt + ,`, the arrangement the project switcher's trigger already uses. - `Alt+S` works the drawer the mobile header's button works — one toggle for both ways in. There is no desktop collapse to reach yet (#244), and inventing a second behaviour for wide screens now would only have to be undone there. - `ProjectSwitcher` and `useNewConversationShortcut` lose their bespoke listeners: the chord and what counts as a press come from the registry through `useShortcutListener`. The switcher's old synthetic-event guard becomes the matcher's own refusal of events without a `code` (a real KeyboardEvent always has one; `new Event("keydown")` from a browser extension never will). Each component keeps its own gate — `isSwitcherEnabled`, the assistant's `enabled` — because those say what is on screen, which is knowledge the registry does not have. Closes #243.
…modals Review of #276 turned up four places where the registry did not yet do what the PR claimed. All four trace back to the matcher being stricter than the behaviour it replaced, not to the registry idea itself. Ctrl/Cmd+K in a text field The dialog's chord came out of a hand-rolled window listener that never asked where the keystroke landed, so it worked mid-typing; the shared matcher's typing guard silently narrowed that. SWITCH_PROJECT_SHORTCUT now declares `allowInInput`, which restores the old behaviour and keeps the PR's "anywhere" promise honest. A text field can never produce Ctrl/Cmd+K itself, so nothing is stolen from a composer, and the guard still keeps plain character chords out of one. Global chords while a modal surface is up Alt+H mid-form unmounted the dialog being filled in, because the dialog check only guarded `?`. Every global chord now stops at an open `aria-modal` element — the precise line between modal surfaces (`ui/Modal`, the canvas covers, the side panel, the celebration overlays) and the deliberately non-modal overlays (the buddy dock, the popover-style dialogs) whose whole point is living alongside the page. Keying off `role="dialog"` alone would freeze every chord whenever the buddy dock sat open, which is most of a session. Denied chords are still owned by the app `canAccessRoute` returned before `preventDefault`, so a hire pressing Alt+P had the keystroke fall through to whatever else was listening, browser included. A matched chord is now consumed first and only then discarded — the "spoken for either way" the comment always claimed. Alt+, reaches macOS NAVIGATE_SETTINGS matched the character alone, and macOS turns Option+, into a literal "≤" — the chord was dead there. A shortcut may now declare both a `code` and a `key` and either spelling answers: the character where the layout produces it, the physical key where a modifier rewrites it. The AZERTY note gets a correction too — the comma moves to the `KeyM` position there, which is exactly why the character has to stay the primary spelling. Tests cover each fix at the level it broke: a bubbling keypress at an input for `allowInInput`, a modal-vs-popover stand-in for the surface guard, the `!defaultPrevented` observable for key consumption, and the "≤"/`Comma` pair for the both-spellings matcher.
… ChatPage The second half of the #276 review: places where the registry described behaviour it did not own, and two accessibility gaps in what it renders. ChatPage answers FOCUS_COMPOSER_SHORTCUT through useShortcutListener now The registry row described a listener ChatPage kept privately, with its own copy of the typing guard — editing the item changed nothing. The item is exported (barrel included) and ChatPage listens through `useShortcutListener`, so the chord, the guard and the help row read one definition. The chord rides in the accessible name, not only in the title `aria-label` wins the accessible-name computation, so the Settings button and the project switcher trigger announced without their chords while the `title` reached only the mouse tooltip. Both now carry the chord inside their `aria-label`; sidebar links keep the `title` arrangement, their name already comes from their content. Help dialog columns balance at two columns Navigation holds the left column alone and Actions + General stack beside it, instead of a flat map that left the fourth grid cell empty under a lone General on every screen wide enough for two columns. KEY_LABELS loses its speculative rows Only `Escape` is ever looked up: character chords are labelled from their own `key`, and no registered chord uses `Period`/`Semicolon`/`Backquote`/`Space` or a digit code. Deleted rather than kept "just in case" — a wrong label in the help view is worse than a plain one, and entries can come back the day a chord actually needs them.
kiranfin
left a comment
kiranfin
left a comment
There was a problem hiding this comment.
Review
The registry as a single source for listener, hint and help dialog is a good structure, and isShortcutPress (AltGr refusal, auto-repeat guard, key for ?, both spellings for Alt+,) is well thought through. The tests for the registry and the modal are thorough. The main problem is the modal guard: it currently disables every global chord on the Board, because a closed drawer still counts as an open modal. The remaining concerns are about chords acting while an overlay is open, text-field detection, and the old Ctrl+K behaviour changing quietly.
🔴 Should be resolved before merge
1. The modal guard treats a closed, always-mounted SidePanel as an open modal, so no global chord works on the Board (src/features/shortcuts/hooks/useGlobalShortcuts.ts:18,57, src/components/ui/SidePanel.tsx:195, src/pages/BoardPage.tsx:1349)
useGlobalShortcuts returns as soon as document.querySelector('[aria-modal="true"]') matches anything. SidePanel sets aria-modal="true" unconditionally and is kept mounted while closed so the backdrop can fade (it is only aria-hidden and inert then). BoardPage mounts <BoardChainPanel> permanently, and that always renders a SidePanel (isOpen={cardId !== null && subject !== null}). On the Board the selector therefore always matches, and every scope: "global" chord (Alt+H/B/C/U/K/P/, and ?) is swallowed (preventDefault runs before the check) without doing anything. Any other page that keeps a SidePanel or DetailsSideDrawer mounted while closed has the same problem; only drawers wrapped in PanelPresence (for example the Starter Work issues sheet) are safe.
Suggested fix: render aria-modal on SidePanel only while open (aria-modal={isOpen ? "true" : undefined}; a closed, aria-hidden panel is not a modal anyway), and make the selector defensive as well: [aria-modal="true"]:not([aria-hidden="true"]):not([inert]). Add a test that mounts a closed SidePanel and asserts a navigation chord still works.
🟡 Worth a look
2. Only scope: "global" chords respect an open modal, so surface chords still act behind or on top of one (useGlobalShortcuts.ts:46-57, src/features/shortcuts/hooks/useShortcutListener.ts:20-30)
The aria-modal guard lives only in useGlobalShortcuts. useShortcutListener has no such check, so Ctrl/Cmd+K, Alt+N, Alt+S and / all fire while a dialog is open. The concrete case is the one the guard's own comment wants to prevent: open the help with ?, press Ctrl+K, and the project switcher opens on top of it. ui/Modal registers its Escape handler on document per open modal, so one Escape closes both. Alt+N behind a modal starts a new conversation under the dialog in the same way. Either apply the modal check inside useShortcutListener (with an opt-out for Ctrl/Cmd+K if opening the switcher over a dialog is intended) or document the split explicitly.
3. Text-field detection is broader than "typing", so chords die on checkboxes, radios and sliders (src/features/easter-eggs/lib/keyTargets.ts, used by isShortcutPress)
isTypingTarget matches every INPUT, not only text-like ones. After clicking a checkbox, radio button or range slider, focus stays on that input and Alt+… chords and ? are ignored although nothing is being typed. The helper was written for the easter-egg games, where that was fine. For the shortcuts, restrict the guard to text-like input types (text, search, email, url, password, number, tel, and so on) plus textarea and contenteditable. A <select> is currently not treated as a text field, which is fine.
4. Ctrl/Cmd+K moves from event.key to event.code, which changes behaviour for non-QWERTY layouts (src/features/shortcuts/shortcuts.ts, SWITCH_PROJECT_SHORTCUT, and ProjectSwitcher.tsx)
The old handler matched the produced character k. The registry now matches code: "KeyK". On Dvorak or Colemak the key labelled K is elsewhere, so the advertised "Ctrl + K" stops working, and the physical KeyK position triggers the switcher (and swallows the event) for whatever letter sits there. The same applies to all the Alt+letter chords, but there it is a new feature, while for Ctrl+K it is a regression of an existing shortcut. Consider declaring both code and key for it (the matcher already accepts either), the way Alt+, does.
5. Alt+S toggles the mobile drawer state on desktop widths too (src/components/layout/SideBar.tsx:476-480)
The listener flips isMobileSidebarOpen regardless of viewport. On a wide screen nothing visible happens, but the state is now true, so resizing below lg shows the drawer already open with its overlay. The chord is also preventDefaulted for nothing on desktop (it is the History menu accelerator in Firefox). Gate the listener on the drawer actually being reachable (a matchMedia check, or only when the header button is displayed), or leave it out of the help until #244 exists.
6. Modifier handling is looser than the docs say for key-matched chords (src/features/shortcuts/shortcuts.ts:321)
if (!shortcut.key && …) skips the Shift check for every chord that declares a key. That is intended for ?, but it also applies to Alt+, (which declares key: ","), so Alt+Shift+, navigates to Settings. Harmless in practice, but the comment above the check only justifies it for ?. Restrict the Shift exemption to chords without altKey/ctrlOrMeta, or state the wider behaviour.
🟢 Nits / cleanup
- Behaviour with overlays, for the record. Modal or
SidePanelopen: global chords are blocked as intended (see finding 2 for the surface chords). Buddy dock: not modal on purpose, chords work. Mobile sidebar drawer: not modal, chords work, but a navigation chord changes the route and leaves the drawer and its overlay open, because only link clicks callonNavigate(SideBar.tsx:468). Closing the drawer onpathnamechange would fix it. - A
ui/Modalthat is playing its exit animation stays in the DOM throughAnimatePresenceand keepsaria-modal="true", so chords are briefly blocked right after closing a dialog. Short, but it will feel like a dropped keypress if someone presses a chord immediately after Escape. useGlobalShortcutscallspreventDefault()before the modal and access checks. This is documented and reasonable, but it also means a PM-only chord likeAlt+Pis silently swallowed for a hire with no feedback. Fine to keep, maybe worth a line in the help ("only for roles that can open it").- Tests: there is no case for a closed
SidePanelin the DOM (finding 1), a surface chord while a modal is open (finding 2), a chord with focus on a checkbox or radio (finding 3),Ctrl+Kon a layout wherekeyandcodediffer (finding 4), orAlt+Sat desktop width (finding 5).
Overall this is clean and well documented, but finding 1 makes the feature unusable on the Board and possibly on other pages, so I would treat it as a blocker. Findings 2 and 3 are worth fixing here as well, since they concern the same guard. Findings 4 to 6 can be accepted as follow-ups.
Found in review of #276: the guard asked "is any aria-modal element in the document?" — and `ui/SidePanel` keeps a closed panel mounted so its backdrop can fade. It was `aria-hidden` and `inert`, but the selector did not care, and the Board mounts one for the whole visit. Every global chord (Alt+H/B/C/U/K/P/, and `?`) was matched, preventDefaulted and then dropped for as long as the page stayed open — silent, total, on the page people live in. Two layers, because either alone could rot: - `ui/SidePanel` now renders `aria-modal` only while it is actually open. A closed, inert panel is not a modal — saying so was wrong before it was ever harmful. - The guard moved to `lib/modalSurface.ts` and its selector is defensive as well: `[aria-modal="true"]:not([aria-hidden="true"]):not([inert])`. The same review also showed the guard covered only the global chords, so Ctrl/Cmd+K could still stack the project switcher on the help dialog, and Alt+N could start a conversation under a dialog still asking its question. The check now lives in `useShortcutListener` too — one rule, both listeners, no opt-outs. Two smaller findings in the matcher: - The typing guard used the games' `isTypingTarget`, which calls every `<input>` a text field. Focus parked on a settings checkbox or slider silenced the page's chords until focus moved on. Shortcuts now use a stricter `isTextEntryTarget` (text-like types, textarea, contenteditable); the games keep the wider predicate, because arrows belong to a focused radio group and Space to a focused switch. - Shift was exempt for every chord with a `key`, not only bare character chords like `?`, so `Alt+Shift+,` was also `Alt+,`. The exemption now requires a chord with no modifiers of its own. And the switcher chord gets its character spelling back: the pre-registry listener matched the produced `k`; `code: "KeyK"` alone answers whichever letter now sits at that position on Dvorak/Colemak — and swallows it. Declaring both (the matcher's either-spelling rule) restores the old behaviour without losing the layout-proof code; the label still prints `Ctrl + K`. Tests: a closed panel in the document (the real SidePanel's rendered shape plus a defensive stand-in), a surface chord with a dialog open, focus on checkbox/radio/range vs text-like inputs, the Dvorak key/code split, and the tightened Shift rule.
…ndows The tail of the #276 review — chords and attributes acting on things that are not there. Alt+S listened at every width, but the drawer only exists below `lg`. On a wide screen the chord flipped state nobody could see and left it flipped, so the next resize opened the drawer uninvited — and on Firefox, where Alt+S is the History menu, the swallow was taken from the browser for a no-op. The listener now runs only while the drawer is reachable, so above `lg` the keystroke falls through to the browser instead of being consumed. A navigation chord moves the route without any link being clicked, so nothing ran `onNavigate` and the mobile drawer stayed open — with its overlay — over the new page. The drawer now closes when the pathname changes, deferred to a microtask to stay clear of `react-hooks/set-state-in-effect`. `ui/Modal` now drops `aria-modal` for the length of its exit animation: during the fade the node is still in the DOM (AnimatePresence), and a stale `aria-modal` kept global chords dead for exactly that window — a dropped keypress for whoever types fast right after Escape. `ui/SidePanel` got the same treatment in the previous commit, for the harder reason. Tests: Alt+S at desktop width (event falls through unconsumed, drawer state untouched, viewport mock restored for the rest of the suite) and the drawer closing on a chord-style route change.
kiranfin
left a comment
kiranfin
left a comment
There was a problem hiding this comment.
Re-review
Checked the two new commits against my previous findings. All of them are addressed:
- Board blocker:
SidePanelandModalonly setaria-modalwhile open, and the shared selector inmodalSurface.tsadditionally ignoresaria-hidden/inertoverlays. Both the cause and the guard are covered. - Surface chords (
Ctrl/Cmd+K,Alt+N,Alt+S,/) now stand down behind an open modal throughuseShortcutListener. isTextEntryTargetno longer treats checkboxes, radios and sliders as text fields.Ctrl/Cmd+Kdeclarescodeandkeyagain, andAlt+Shift+,no longer matchesAlt+,.Alt+Sonly listens belowlg, and the drawer closes on route change.
tsc -b and eslint on the touched files are clean. I could not run the new SideBar tests locally (localStorage is undefined in my Node 26 environment, the same tests fail on dev), so I have not verified those.
Small remaining nits, none blocking:
TEXT_INPUT_TYPESdoes not includedate,timeanddatetime-local. The date fields inChatComposerwould let/focus the composer while someone is typing a date.Ctrl/Cmd+Kstill also answers on the physicalKeyKposition for Dvorak/Colemak. It works on the labelled key now, so this is cosmetic.- The
Promise.resolve().then(...)in theSideBareffect only sidestepsreact-hooks/set-state-in-effect. The render-time state pattern used elsewhere in the repo would be cleaner.
…y-escalation-preview-edit (#235) David's second backmerge. `311` now carries dev's app-wide keyboard-shortcut work (#276, #268, and the focus/shortcut fixes that followed) plus the sidebar/modal touches that came with them — a **clean merge, no conflicts**: none of those commits touch the buddy files #235 reworks, so our wiring, the card's own dismissal call sites and the reset-site clears all stay as they were after e0d1d31. Contents: 26 files, +1,867/−83, dominated by the new `features/shortcuts/` registry, its modal and their tests. Nothing of ours was rewritten for this one — the resolution work of the previous backmerge (their files + our wiring, the memo-safe row props, both reset behaviours) already meets this base. Gate on the merged tree — `npm run try` green on Node 22: format:check, build (`tsc -b` included), lint; unit 344 files / 3321 tests; a11y 57 files / 78 tests. Buddy + a11y + the incoming shortcuts suite alone: 87 files / 307 tests, so #235's own suites still pass against their memoised rows and the dismissal API.
2 participants
Closes #243 · base
dev· branchfeature/243-app-wide-keyboard-shortcutsEvery keyboard chord in the app, defined once and answered wherever it makes sense — plus one help dialog that lists them all.
Alt+H·Alt+B·Alt+C·Alt+U·Alt+K·Alt+P·Alt+,?Ctrl/Cmd+KAlt+NAlt+Slg(#244 will give the same chord the desktop collapse)/·EscSidebar entries show their chord on hover/focus (chip +
titlefor screen readers), and the help is discoverable by pressing?.Why these chords
Ctrl+Nopens a window and a page cannot refuse it). The Alt row is free for H/B/C/U/K/P/S/,on Chrome, Edge and Firefox on Windows. Named trade: on macOSOption+N/Option+Uare dead keys (ñ/ü) — documented in the code, acceptable for a Windows-first team.?is Shift+ß on a German keyboard (code: "Minus") and Shift+/ on a US one (code: "Slash") — a code-based?is dead for half this team. Same for/and,. Letter chords keep the app'sevent.codeconvention.altKeycheck fires on every{,[,},@a European layout types.Alt+N, which fires while composing on purpose: the moment somebody most wants a fresh conversation is halfway through typing into the wrong one.canAccessRoute. A hire pressingAlt+Pis refused exactly like one typing the URL — the same rule the sidebar and the manager guard use, and the same filter hides the row from the help.What the review of the handed-over plan changed
?oncode:"Slash"→ matched askey:"?"(dead on QWERTZ).requiresPermission(profile, canManage)callbacks → dropped in favour ofpath+canAccessRoute.Ctrl/Cmd+KandAlt+Nwere listed as "existing" shortcuts while implemented by components — both now listen through the shared registry matcher, so their hints and handlers cannot drift.Escstays documented-only: 30+ dialogs and popovers already own it locally (several checkingevent.defaultPrevented), and a global handler would break layered cases.bg-app-surface-raiseddoes not exist in the token set — the keycaps use real tokens./buddy(it lights up Chat), so Buddy is help-only; Chat — the busiest entry — got theAlt+Cthe plan left it without.git checkout dev && git pullin the shared checkout → worktree (workspace rule).Base
dev, not #268: #268 touchesAddSourceFlow.tsxand its test — no shared file with this branch. Stacking on an open hotfix would couple our mergeability to it for zero shared code.Not in this PR
Alt+Sworks the drawer; Add a collapsible and resizable sidebar #244 owns the collapse and this chord will land on it without changes.Verification
npm run trygreen: format check, build, eslint, unit suite, a11y suite.Alt+Sdrawer tests, sidebar-link hint tests.code-lessKeyboardEvent; browsers always setcode, so it now sends a realistic event (the synthetic-event guard moved into the matcher).