feat(shortcuts): add keyboard shortcut settings - #155
Conversation
Settings gains a Shortcuts section that lists every host binding and every active plugin contribution, grouped by owner, with search, inline key capture, per-row Change/Reset and Reset all. Overrides persist in the device-local buzz-shortcut-bindings.v1 preference and are resolved by the dispatcher at match time. Relation to the plugin architecture: the plugin-facing contract is unchanged. Plugins keep calling ctx.shortcuts.register with their default binding via @buzz/author and never see, store or re-register for an override. The page reads the dispatcher's own registries (hostSnapshot/hostSubscribe for host bindings, snapshot/subscribe for plugin contributions), so it cannot drift from what fires and follows plugin enable/disable live. Overrides are keyed by the registry identity the dispatcher already uses (bare id for host bindings, pluginId/id for contributions), so they survive disable, re-enable and replacement, and orphaned entries are ignored rather than deleted. Host chords remain reserved: the page refuses chords already used by any listed shortcut and warns about chords the message editor handles locally. There is no Settings extension point and no Rust catalog change; this is host-owned feature code modelled on Appearance. Docs updated accordingly. FOUNDATION files touched (minimal, additive, behaviour-preserving): - src/features/shortcuts/service.ts: optional BindingOverrides constructor argument consulted at match time (falls back to the registered binding); host-only hostSnapshot/hostSubscribe so the Settings page can list host bindings and their defaults. Plugin snapshot/subscribe and matching rules are unchanged. - src/features/shortcuts/bindings.ts: export isKeyBinding (the existing per-binding validation) and add sameBinding; matches() accepts an alias array so effective bindings resolve in one place. Validation semantics are unchanged. - src/app/services.ts: construct/dispose the shortcut bindings store, pass it to ShortcutsService, and expose it on AppServices. - src/app/App.tsx: hand the shortcuts service and bindings store to Settings. Provisional components awaiting a design pass (plain black-and-white on standard tokens, kept outside src/shared/design-system/ui/, marked with a DESIGN PASS PENDING file comment and data-design-pass="pending"): - src/features/shortcuts/KeyCombo.tsx: key-combo <kbd> chip. - src/features/shortcuts/KeyCaptureControl.tsx: inline key-capture control. Also adds formatBinding (glyphs in Control/Option/Shift/Command order on Apple platforms, Ctrl+Shift+K style elsewhere, accessible plain-words label) and uses it for the search and terminal hints; unit tests for the store, formatter, dispatcher overrides and the page; a Playwright journey rebinding the example plugin's shortcut; and the Settings tab-order assertion. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Addresses the review of the rebindable shortcut settings.
Override lookup can no longer throw or allocate on keydown. The store keeps
overrides on a null-prototype record and prebuilds one frozen alias list per
key in a Map, so resolve() returns readonly KeyBinding[] and a host id such as
"constructor" yields a binding or nothing, never Object.prototype's function.
matches() takes only an alias array; the dispatcher treats anything that is
not a nonempty binding list as the registered default, so a malformed store
entry or resolver cannot stop dispatch. normalizeShortcut now returns
NormalizedShortcut (binding always an array), which the registries use; the
plugin-facing Shortcuts/RegisteredShortcut types are unchanged.
Capture and conflict rules: Escape cancels whatever modifiers are held instead
of being refused or saved; the editor-shadowed warning list now covers
mod+Shift+Y and mod+Shift+Home/End; copy, cut, paste and select-all chords are
refused because a match would preventDefault them everywhere, and close-window
and quit chords are refused in the desktop (Tauri) build. Rows whose effective
chord another listed shortcut also answers to show a monochrome text-subtle
"Also used by …" line, surfacing conflicts that arise after capture when a
plugin is re-enabled or installed. Per-row Reset returns focus to that row's
Change button. The host group id is namespaced ("host", "plugin:<id>") so a
plugin with manifest id "buzz" cannot collide.
Tests cover the prototype-named host id and misbehaving resolver in the
dispatcher, the store's inherited-property immunity and lookup identity,
Escape with modifiers, Dead/Unidentified refusal, the clipboard and desktop
refusals, the widened editor warnings, the shared-chord marker, focus after
Reset, the group id collision, an Option/Shift chord captured on Apple
platforms pinning the composed-character behaviour, and the search trigger
hint following a rebind. Docs gain a Known limitations note describing the
Option-chord composed-character behaviour and the intended event.code fix.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Use the existing platform modifier to assert exact spoken labels for the default K and rebound U shortcuts. Preserve dispatch, conflict, persistence and reset coverage without changing production formatting or browser timeouts. Revert the unrelated 15-second bundled-pages timeout introduced by f19ec7d; it does not address the deterministic Linux label mismatch. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Remove the shortcut search field, its filtering state, and the introductory instructions while preserving grouped shortcut editing and reset controls. Update component coverage to assert the simplified settings view. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Address the three review findings on shortcut settings: warn when accepted modified Enter chords are handled by the message editor, reuse HOST_SHORTCUT_ORDER.search in PageSearch, and ignore AltGraph during capture without changing the dispatcher's safety guard or simplified settings UI. Add regressions for both platform mappings, actual host registration order, and capture through persistence to dispatch, including AltGraph rejection and continued support for ordinary Control+Alt chords. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Reuse the shared Input and neutral notices, wrap capture actions before titles collapse, and let Tab leave capture without invoking plugin bindings. Keep Escape cancellation discoverable and formatting safe for restored prototype-named keys. Replace xterm's hard-coded default exemption with a private synchronous handoff of the original keydown to the existing dispatcher. Preserve live overrides, eligibility and lifetime ownership without changing the plugin API or ordinary editor precedence. Add restore-to-shell-render regressions, dispatch and keyboard-exit coverage, and two browser cases for focus/layout and real focused-xterm rebinding. Verified 61 focused tests, 79 design tests, 36 Chromium/WebKit checks, TypeScript and design checks/build. Native/package acceptance and hosted CI remain deferred. Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
REVIEW CLEAR: no actionable merge blockers found under the documented contract at 690b5a9e2ba9aa84a997dfea4187ce8fdce1b445 against base 877ae2a6749221dd6850d5695b5c8de023a239e3.
Reviewed host/plugin registration and lifetime, preference persistence/recovery, rebind/reset/conflicts, capture/focus, Settings/Search integration and the terminal handoff. Inline notes cover an optional accessibility improvement and the concrete Linux control-key consequences of the explicitly documented app-first terminal policy. I am not requesting a new terminal-policy API or reversing reset/default behavior.
Validation: source-only review; no repository code was executed. Existing exact-head CI passed all 11 executed checks; Windows native validation was skipped. Native desktop/Linux PTY and OS-keyboard acceptance, actual screen-reader output and the provisional design pass remain unverified. The terminal fixture does not cover the full host-shortcut collision case. This is a comment, not approval or release certification.
| bindings.set(target.key, isDefault ? null : binding); | ||
| setEditing(null); | ||
| // EditableInput handles Enter locally regardless of the held modifiers. |
There was a problem hiding this comment.
P3, nonblocking: expose ordinary save/reset confirmation to assistive technology. A normal successful capture clears the notice and removes the input, then focus returns to the same “Change shortcut…” button used after Escape cancellation. The new chord is accessible in the row, but neither that static text nor “Modified” announces the successful transition; reset has the same limitation. Consider a short polite status message containing the saved/restored chord, or an equivalent description on the focused control, and a component assertion for normal save/reset. This is source-observed markup behavior, not a tested claim about a particular screen reader.
| terminal.attachCustomKeyEventHandler((event) => { | ||
| const host = element.ownerDocument.defaultView; | ||
| return !disposed && element.isConnected && host | ||
| ? forwardTerminalKey(host, event) | ||
| : true; |
There was a problem hiding this comment.
Nonblocking compatibility note: app-first dispatch also takes Linux terminal control chords. With default bindings and the terminal focused, this handoff runs global-search for Ctrl+K (PageSearch.tsx:69–76); Ctrl+[ runs Back when canGoBack (app/shortcuts.ts:99–106). The dispatcher prevents the original event and this handler returns false, so xterm does not process those keys. Users therefore lose the usual terminal kill-to-end-of-line / Ctrl+[ Escape behavior in those states. Base special-cased only the terminal toggle.
This matches the PR’s explicit “eligible app shortcuts … win” contract in docs/plugin-architecture.md, so I am not calling for a policy reversal. Please make these concrete Linux consequences visible in terminal documentation and include them in attended acceptance. The added renderer journey registers only the terminal plugin, not these host shortcuts, so its green result does not validate this tradeoff. App-side interception is source-verified; real Linux PTY behavior was not exercised in this review.
…search-send * origin/main: Connect attachments to existing message delivery (#176) perf: preserve unchanged thread row identities (#171) perf: cache markdown preparation by content (#172) Add safe attachment upload groundwork (#150) feat: add sampling profiler launch modes (#148) feat(channels): remove DMs from the sidebar (#157) Distinguish namesake agents and selected recipients (#142) feat(channels): move diagnostics into Channel Settings (#163) Replace warning banners with shared Base UI toasts (#164) feat(shortcuts): add keyboard shortcut settings (#155) fix(channels): give floating unread cue an opaque panel surface (#153) feat(communities): add BUZZ_DEV_OPEN_RELAY to open the default relay on fresh dev ports (#151) Restore recipient avatars beside the composer mention tool (#162) Fix startup inventory duplication and late panel scroll shifts (#160) feat(channels): add channel creation (#138) Standardize Button and IconButton with Buzz design tokens (#145) Signed-off-by: Zach Marley <zmarley@squareup.com>
Summary
Testing
CI correction (
5e1173d)Shift Commandon Apple,Control Shiftelsewhere. Production formatting and shortcut behavior are unchanged.f19ec7d's unrelated 15-second bundled-pages timeout; restore the original default without changing its body or wait deadlines.Validation at
5e1173dbin/pnpm test:browser tests/browser/shortcuts.spec.mjs --project chromium --project webkit --no-deps, 9.6s wall). Hooks passed formatting, types, 642 related tests and design guards. Bundled-pages passed in 4.55s.f19ec7drun → current run (same commands/config; different runner executions, not a controlled performance benchmark):Slowest-file evidence remains unrelated to this fix:
message-navigation.spec.mjsin both browser lanes (Chromium 55.06s → 54.97s; WebKit 93.63s → 76.45s), andunread-startup.test.tsin Vitest (28.46s → 19.58s elapsed per file). Current slowest individual tests are the presence thread-author test (15.76s Chromium / 22.03s WebKit) and unread evidence-deadline test (10.02s Vitest). No performance improvement is inferred from these runner-variable samples.Remaining scope
This correction leaves the other branch's timeout change and the unrelated presence/settings investigations untouched. No local full scan or Windows/native GUI acceptance was performed. PR remains draft pending the feature's human/code-owner review.