Conversation
Typing anywhere in the chat view now moves focus into the composer and keeps the character that triggered it, so a keystroke is no longer dropped after scrolling, selecting text, or returning to the window. Closes #2312. Routing lives in a pure resolver so the exemption rules are the tested contract: modifier chords, navigation and function keys, another editable target, and interactive controls (buttons, links, menus, modal focus traps) all keep their native behavior. Space is composer input now, so it also stops marking restore-time scroll intent. Input method keystrokes focus without inserting, because the composition owns the text. The new-thread composer shares the same composable, and the focus predicates are shared with the existing scroll-intent guard instead of adding a fourth copy.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zerob13
left a comment
There was a problem hiding this comment.
Review — type-to-focus for the composer
Verdict: Clean PR. No blocking or P1 issues. Approving — the notes below are informational, no action required.
What this adds (for readers who haven't followed #2312)
When you're reading a conversation and just start typing, the keystroke used to be silently dropped unless the composer already had focus — you had to click it first. After this PR, typing any printable character anywhere in the chat view moves focus into the composer and keeps the character you typed, so nothing is lost. It works on the new-thread page too, and it doesn't fire when it shouldn't: shortcuts (Cmd/Ctrl/Alt), navigation and function keys, another open text field (e.g. the chat search box), focused buttons/links/menus, and open dialogs are all left alone. Space now goes into the composer instead of scrolling the transcript — exactly what the issue asked for.
Why the design holds up
- The routing rules are a tested pure function.
resolveComposerTypeToFocusIntenttakes the key event plus three booleans and returns ignore / focus-only / focus-and-insert. Every exemption is pinned by a unit test without mounting a page or an editor. This is the whole risk surface of the feature (stealing a keystroke from something else), and it's now a documented contract. - Ownership is checked before intent. The ordering matters and they got it right: an IME composition running in another field (keyCode 229 bubbles to
window) must not pull focus mid-word, so the editable-target check precedes the IME branch. A test pins that order. - IME handling is the conservative correct choice. Input-method keystrokes (
isComposing/ keyCode 229 /'Process') focus the composer without inserting anything and withoutpreventDefault— the composition owns the text. Non-IME printable chars are inserted via a TipTap text node rather than a string, which is what makes a lone Space survive the parser. Good attention to detail. - No over-design. One pure resolver, one thin composable, one shared predicate module. The predicates (
lib/keyboardFocus.ts) also replace ChatPage's pre-existing inline copy used by the scroll-intent guard — a consolidation, not new abstraction for its own sake. The scroll-architecture test whitelist is updated to match the one newscrollIntoViewcall. - Enable-state wiring is consistent. ChatPage gates on read-only / preparing / pending-tool-interaction; NewThreadPage gates on
isSubmittingInput, which is the same signal that drives its:editableprop — so there's no window where a key ispreventDefaulted and then swallowed by an inert composer. ChatPage never bindseditable, so it's always true when the box is rendered. - Tests are proportionate. ~420 lines across a resolver test, a predicate test, a wiring/lifecycle test, and two small additions to existing suites. CI is green including
test-renderer.
Minor observations (no action needed)
- Dead-key accents: on layouts where an accented char is two keystrokes (dead key + letter), the first key is
'Dead'→ ignored (correct), but the second lands as a plain focus-and-insert of the bare letter — the user getseinstead oféon the very first keystroke after losing focus. Niche enough to leave; if it ever gets reported, routingDeadto focus-only would fix it. - Test overlap: the composable test re-covers several routing cases the resolver test already pins. Mild duplication, justified by the wiring/scope-lifecycle coverage it adds; not worth trimming.
- Space as a scroll key is gone from the chat view for keyboard-only users; PageUp/PageDown and the arrows remain. This is the behavior the issue specified, just noting the trade-off for anyone who misses it.
References
src/renderer/src/features/chat-page/model/typeToFocus.ts— the routing contract (ignore / focus-only / focus-and-insert)src/renderer/src/features/chat-page/composables/useComposerTypeToFocus.ts— window keydown wiring, shared by both pagessrc/renderer/src/lib/keyboardFocus.ts— canonical editable/interactive predicates, reused by the scroll-intent guardsrc/renderer/src/components/chat/ChatInputBox.vue—focusAndInsertText(~L914), text-node insertsrc/renderer/src/features/chat-page/ChatPage.vue— enable-state computed (~L1390), Space removed from restore-scroll-intent keyssrc/renderer/src/pages/NewThreadPage.vue— same composable,!isSubmittingInputgatetest/renderer/features/chat-page/model/typeToFocus.test.ts,test/renderer/lib/keyboardFocus.test.ts,test/renderer/features/chat-page/composables/useComposerTypeToFocus.test.ts— the exemption matrix
Summary
Implements #2312. When the chat session is editable and no control holds the keyboard, typing anywhere in the view moves focus into the composer — and the character that triggered the focus lands in it, so nothing is dropped. An existing draft is appended to at its end. The new-thread composer gets the same behavior.
BEFORE / AFTER (key routing)
Design notes
resolveComposerTypeToFocusIntenttakes the event plus three booleans and returns ignore / focus-only / focus-and-insert. The exemption contract is the whole feature, so it is unit-tested without mounting a page or an editor.keyCode === 229bubbles towindow) would have pulled focus into the composer mid-word. Editable target, interactive focus, and the enabled flag are now all checked ahead of the IME branch, and a test pins that order. The IME branch must still precede the printable-character test, because'Process'is seven code units long.compositionstartis not the entry point, contrary to the issue's suggestion: with focus on a non-editable element no composition ever starts, so there is no event to wait for. The entry point has to bekeydown, identifying IME keystrokes byisComposing/keyCode === 229/key === 'Process'.focusAndInsertTextreusesfocusInput'sfocus()+setCaretToEnd()pair, so an existing draft is appended to at the end, then inserts a{ type: 'text' }node. A node rather than a string, because a lone space would otherwise be collapsed by the parser — and a separate method rather thaninsertRecognizedText, which trims and drops empty strings.SESSION_RESTORE_SCROLL_INTENT_KEYS; it is composer input now, and the claimed key ispreventDefaulted so the transcript does not scroll. Making it explicit means the two mechanisms no longer depend on listener registration order.lib/keyboardFocus.tsis the new canonical pair, andChatPage's local copy now imports from it.useChatSearchanduseSidebarSessionShortcutskeep theirs — different semantics, own tests — so this is a consolidation of one call site, not a sweep.[tabindex]. The message scroll container istabindex="0", and it is exactly where focus lands after clicking empty space — the main scenario for this feature.aria-modal. reka-ui 2.10.4 writesrole="dialog"/role="alertdialog"but neveraria-modal, so the existing sidebar guard's[role="dialog"][aria-modal="true"]selector matches essentially nothing. The new predicate matches on role plus the shadcndata-slotmarkers and the popper wrappers.useEventListener(window, 'keydown')rather than joininguseChatPageEventBridge; both pages call it, and it detaches with the component scope.Verification
pnpm run format:check,pnpm run lint(0 warnings, 0 errors),pnpm run typecheck(node + web),pnpm run architecture:renderer-baseline:check— all clean.contenteditable="false"is not editable), and the composable's wiring (insert, space, IME focus-only, editable/interactive/disabled exemption, fallback when the composer cannot insert, scope disposal).scrollIntoViewinChatInputBox.vue(2 → 3); the entry now carries a comment that all three scroll the composer itself, not the message list.RUN_PROVIDER_INTEGRATION=trueand a live model, which does not fit a keyboard-routing behavior.Needs manual verification — Windows and Linux keyboard input method testing
Automated tests cannot cover this: whether an input method delivers its first keystroke to a window whose focus is on a non-editable element is a platform behavior, not something readable from the source. Please verify on Windows and Linux (and macOS) before this leaves draft.
For each platform, with a Chinese (or other IME) input method active:
nfor 你 leaves annbehind)?keydownreports for that first keystroke —key,keyCode,isComposing. (keyCode === 229/key === 'Process'means the IME branch is taken and only focus happens; a plain Latinkeymeans the character is inserted literally.)If a stray character shows up on any platform, the fix is a targeted one: remember the character inserted by type-to-focus, and if
compositionstartarrives in the composer within ~300 ms, undo that single insertion. That mitigation is deliberately not in this PR — it adds complexity and a (small) risk of deleting a legitimate character, so it should only land if a platform actually needs it. Report the findings here and I will add it or leave it out.Also worth a manual pass on the same platforms: plain English typing keeps the first character; Space types a space instead of scrolling; typing is not stolen while the chat search box or a dialog input has focus;
ArrowUp/Down,PageUp/Down,Home/End,Esc,TabandCmd/Ctrl+C/V/A/Fkeep their native behavior.Known gaps
@,€, …) are rejected by the modifier exemption, so they do not trigger focus. If that matters, the exemption can special-caseevent.getModifierState('AltGraph').key === 'Dead'and are ignored. They produce no character of their own, so inserting one would be wrong anyway.contenteditableattribute literal ProseMirror injects was not runtime-verified. The predicate uses the broadest form ([contenteditable]:not([contenteditable="false"])) plusrole="textbox"as a second net, so either literal matches.