Skip to content

fix(web): honor editableFocus in shortcut conditions - #24134

Open
unmanbearpig wants to merge 1 commit into
pingdotgg:mainfrom
unmanbearpig:fix/editable-shortcuts
Open

unmanbearpig wants to merge 1 commit into
pingdotgg:mainfrom
unmanbearpig:fix/editable-shortcuts

Conversation

@unmanbearpig

Copy link
Copy Markdown

Summary

docs/user/keybindings.md says editableFocus is true when a text field, the composer, or another editor has the keyboard. The resolver defaults it to false, and four global handlers don't pass it:

So when: "!terminalFocus && !editableFocus" on mod+k still 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:

 resolveShortcutCommand(event, keybindings, options)
   context = resolveContext(options)
+  if options.context.editableFocus is undefined and document exists
+    context.editableFocus = isEditableFocused(event.target ?? document.activeElement)
   match rules against context
  • When a caller passes editableFocus, that value is used, as before.
  • Among the defaults, only mod+z → thread.undo reads editableFocus, and every handler that acts on thread.undo already passes it. Default behavior does not change. Only user rules that reference editableFocus behave differently, and they now do what the docs say.
  • Shortcut labels (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.

Before: palette opens over the composer After: composer keeps focus and draft
Before After

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 to keybindings.ts is #24063 (the Mod+B bold check), which doesn't touch the resolver or this chord.

apps/web/src/keybindingsFocus.test.ts exercises real rule compilation and resolution, with isEditableFocused mocked at its boundary.

Before: the test file against unchanged keybindings.ts on 9828cad49f gives 6 failed and 2 passed:

!editableFocus, event from editor     → commandPalette.toggle   expected null, got "commandPalette.toggle"
!editableFocus, event from editor     → sidebar.toggle          expected null, got "sidebar.toggle"
!editableFocus, event from editor     → thread.next             expected null, got "thread.next"
!editableFocus, event from editor     → thread.jump.1           expected null, got "thread.jump.1"
targetless event, editor focused      → commandPalette.toggle   expected null, got "commandPalette.toggle"
editableFocus (positive), from editor → commandPalette.toggle   expected "commandPalette.toggle", got null
explicit caller context wins                                    pass
rule without editableFocus                                      pass

After: vp test run apps/web/src/keybindingsFocus.test.ts apps/web/src/keybindings.test.ts apps/web/src/composer-logic.test.ts passes 107 of 107. vp run check:changed upstream/main passes 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 editableFocus change, so default bindings behave as before. The cost is one closest() 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

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 23, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 0b5507d

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.

@coderabbitai

coderabbitai Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
📥 Commits

Reviewing files that changed from the base of the PR and between 66ee628 and 0b5507d.

📒 Files selected for processing: 2
  • apps/web/src/keybindings.ts
  • apps/web/src/keybindingsFocus.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Shortcut resolution now derives editable-focus state from the event target or document active element when no explicit focus context is supplied. New tests cover focus conditions, missing event targets, and explicit context overrides.

Changes: Shortcut Focus Resolution

Layer / File(s) Summary
Derive editable focus during shortcut resolution
apps/web/src/keybindings.ts, apps/web/src/keybindingsFocus.test.ts
ShortcutEventLike now includes an optional target. When no explicit editableFocus context is supplied, the resolver checks the event target or falls back to document.activeElement. Tests cover focus conditions and explicit overrides.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested labels: size:M

Suggested reviewers: macroscopeapp

Merge Risk: ⚪ Minimal · up to 0b550

The editable-focus change is ready to merge after normal checks; the reported target-handling issue does not establish a shortcut regression.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title is concise, specific, and clearly identifies the main fix: honoring editableFocus in web shortcut conditions.
Description check Passed The description provides the problem, change, scope rationale, evidence, verification results, limitations, and implementation details. Although it uses custom headings instead of the template heading…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description provides the problem, change, scope rationale, evidence, verification results, limitations, and implementation details. Although it uses custom headings instead of the template headings, it covers the required information in sufficient detail.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant