Repository navigation
fix(web): honor editableFocus in shortcut conditions - #24134
unmanbearpig wants to merge 1 commit into
Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a narrowly scoped shortcut-resolution fix that derives editable focus only when callers omit it, while preserving explicit context and existing default definitions. A dedicated test file covers the changed cases and the runtime impact is limited to focus-aware shortcut conditions. You can add or adjust custom eligibility rules. Learn more. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
Summary
docs/user/keybindings.mdsayseditableFocusis true when a text field, the composer, or another editor has the keyboard. The resolver defaults it tofalse, and four global handlers don't pass it:CommandPalette.tsx:284AppSidebarLayout.tsx:161Sidebar.tsx:2547andLegacySidebar.tsx:3457So
when: "!terminalFocus && !editableFocus"onmod+kstill opens the palette while you type in the composer. Users who want Ctrl-K back for Emacs-style line editing have no way to get it.Instead of adding a focus lookup to every handler, the resolver fills the value in once:
editableFocus, that value is used, as before.mod+z→thread.undoreadseditableFocus, and every handler that acts onthread.undoalready passes it. Default behavior does not change. Only user rules that referenceeditableFocusbehave differently, and they now do what the docs say.effectiveShortcutsForCommand) still don't infer focus.This is a small fix for a documented condition that the code ignores, so it fits the obvious-bug exception in CONTRIBUTING.md.
Evidence
Rule under test:
{ "key": "mod+k", "command": "commandPalette.toggle", "when": "!terminalFocus && !editableFocus" }. The composer is focused with a draft typed, then Ctrl-K is pressed. Web, Browser panel, isolated dev state. Identifiers are redacted.With the fix, Ctrl-K still opens the palette when focus is outside an editor. These captures were taken on
23bd753806, before this branch was rebased. Since then, the only upstream change tokeybindings.tsis #24063 (the Mod+B bold check), which doesn't touch the resolver or this chord.apps/web/src/keybindingsFocus.test.tsexercises real rule compilation and resolution, withisEditableFocusedmocked at its boundary.Before: the test file against unchanged
keybindings.tson9828cad49fgives 6 failed and 2 passed:After:
vp test run apps/web/src/keybindingsFocus.test.ts apps/web/src/keybindings.test.ts apps/web/src/composer-logic.test.tspasses 107 of 107.vp run check:changed upstream/mainpasses format, lint, knip, typecheck, and the tests beside the change.Not tested: the Electron shell on its own (it uses the same renderer). Mobile has its own keyboard dispatch and doesn't use this resolver.
Merge Danger
Door: two-way
Blast Radius: web-shortcuts
Only user rules that mention
editableFocuschange, so default bindings behave as before. The cost is oneclosest()lookup per keydown that goes through the resolver.Implemented by GPT-6.1-Sol through Codex in T3 Code. Reviewed by Claude Opus 5.5 through Claude Code in T3 Code. Published by Claude Opus 5.5 through Claude Code in T3 Code.
🤖 Generated with Claude Code