Repository navigation
feat(native-chat): focus the message box when a chat appears - #19868
Conversation
Opening a native chat left focus nowhere, so you had to click the composer before typing. Nothing in the chat surface focused it on open; the only existing focus calls were reactive (typing on the bridge pane background, picker acceptance, attachments, dictation), and the structured pane had none of those. useNativeChatComposerRevealFocus focuses the composer on the reveal edge, covering a new chat tab, a worktree-create landing in chat, the chat-view toggle, and switching back to an existing chat tab. Mount is the wrong signal: retained panes hide with display:none + inert and never unmount on a tab switch. It reuses the existing composer handle and shouldPreserveEditableFocus rather than adding a parallel path, and retries across a bounded run of frames because Tiptap publishes its adapter after mount and Radix restores a closing dialog's trigger in a setTimeout(0). Two supporting changes: - isFocusedGroup, from activeGroupIdByWorktree. On worktree activation both columns of a split flip visible in the same commit, so without it two revealed chats fight over the caret. The bridge route already had this bit as controller.isActive; only the structured overlay needed it. - focusRuntimeTerminalSurface bails on a chat-covered pane. Its DOM-path twin already declines chat view via data-terminal-chat-view, but the runtime path focused the covered xterm unconditionally and pulled the caret out of the composer. Returns true, not false: false sends the caller to the DOM fallback, which for a structured tab id focuses an unrelated tab's xterm.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change propagates Merge Risk: ⚪ Minimal · up to The change adds reveal-edge chat composer autofocus while preserving user-selected focus and preventing covered terminals from reclaiming it. Current evidence shows the behavior is ready to merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description is detailed and covers the change, rationale, testing, visual proof, AI disclosure, risks, and checklist. However, the required Linked Issue section does not contain an issue link or a
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 8ad25015-4853-49f4-865f-cf5bb9a6f370
📒 Files selected for processing (15)
src/renderer/src/components/native-chat/NativeChatResolvedView.tsxsrc/renderer/src/components/native-chat/NativeChatStructuredSession.test.tsxsrc/renderer/src/components/native-chat/NativeChatStructuredSession.tsxsrc/renderer/src/components/native-chat/NativeChatView.tsxsrc/renderer/src/components/native-chat/StructuredAgentSessionPaneOverlayLayer.test.tsxsrc/renderer/src/components/native-chat/StructuredAgentSessionPaneOverlayLayer.tsxsrc/renderer/src/components/native-chat/native-chat-composer-reveal-focus.test.tsxsrc/renderer/src/components/native-chat/native-chat-view-types.tssrc/renderer/src/components/native-chat/use-native-chat-composer-reveal-focus.tssrc/renderer/src/components/terminal-pane/TerminalPaneNativeChatPortal.tsxsrc/renderer/src/components/terminal-pane/pane-helpers.tssrc/renderer/src/lib/workspace-activation-terminal-focus.test.tssrc/renderer/src/runtime/focus-runtime-terminal-surface-chat-view.test.tssrc/renderer/src/runtime/sync-runtime-graph-terminal-registration-ownership.test.tssrc/renderer/src/runtime/sync-runtime-graph.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one minor test-hygiene nit inline.
Reviewed changes
useNativeChatComposerRevealFocushook — focuses the composer on theisVisible && isFocusedGroupreveal edge (not mount, since retained panes never unmount), deferring one frame and retrying over a bounded run of frames until the caret actually lands in the pane.isFocusedGroupprop threading — a new required prop across all three native-chat view types; the structured overlay derives it fromactiveGroupIdByWorktreewhile the bridge route reuses the controller's existingisActive.focusRuntimeTerminalSurfacechat-view guard — bails (return true) when the active pane is covered by the chat portal, so worktree activation no longer focuses the covered xterm and pulls the caret out of the composer; mirrors the DOM path'sdata-terminal-chat-viewbail.- Test coverage — 14 hook tests (including latch and in-pane-check isolation), a runtime-guard suite, plus updates to the overlay-layer, workspace-activation, and registration-ownership tests.
I traced the focus flow end-to-end: the reveal-edge latching, the isFocusedGroup derivation on both routes (StructuredAgentSessionPaneOverlayLayer.tsx and the bridge route's controller.isActive), and the runtime guard's true-vs-false return contract against the DOM fallback. The runtime guard checks the active pane rather than a requested leafId, but that matches the DOM path's data-terminal-chat-view (activePaneIsChatLeaf) semantics, so it is not a new regression. Ran the four touched/new test suites locally (31 tests green).
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| @@ -0,0 +1,248 @@ | |||
| // @vitest-environment happy-dom | |||
There was a problem hiding this comment.
The createRoot + act harness in this file doesn't set globalThis.IS_REACT_ACT_ENVIRONMENT = true, so every run emits The current testing environment is not configured to support act(...) warnings (visible in the test output; tests still pass). Setting it once at module top silences the noise and matches the convention used by NativeChatTranscriptChrome.test.tsx and native-chat-working-status-shared-clock.test.tsx.
| // @vitest-environment happy-dom | |
| // @vitest-environment happy-dom | |
| globalThis.IS_REACT_ACT_ENVIRONMENT = true |
|
Final Electron QA evidence — rerun after the latest uncommitted review fixes settled Identity and launch:
ORCA_BACKGROUND_LAUNCH=1 REMOTE_DEBUGGING_PORT=9343 ORCA_DEV_USER_DATA_PATH=/tmp/orca-electron-profile-umibozu3-final.EfzEJL node config/scripts/run-electron-vite-dev.mjs -c /tmp/orca-electron-vite-umibozu3-5182.mjs
command playwright-cli -s=umibozu3_final attach --cdp=http://127.0.0.1:9343
command playwright-cli -s=umibozu3_final eval "(async () => JSON.stringify(await window.api.app.getIdentity()))()"
Observed behavior:
Final source status/mtimes stayed unchanged through this rerun. A transient host network change occurred while the worker was idle, so I reloaded the hidden renderer before the final scenarios; the recovered final page reported 0 renderer console errors and one unrelated outdated-React-Grab warning. Gap: I did not submit the draft to an agent, so this covers rendered reveal/focus and input routing rather than a billed/model response; the requested Grok attempt remains unavailable because its TUI reported zero remaining usage before execution. |
There was a problem hiding this comment.
ℹ️ No new issues in this delta — the split-ownership, user-intent, and runtime-leaf changes are clean. One prior test-hygiene nit remains unaddressed (inline).
Reviewed changes
- Split-group focus ownership —
TerminalPaneNativeChatPortal.tsxnow passesisFocusedGroup={isActive && activePaneIsChatLeaf}instead ofisActivealone, so only the chat leaf's column claims the caret and an active split terminal sibling keeps focus. - User-intent cancellation in the reveal hook —
use-native-chat-composer-reveal-focus.tsregisterspointerdownandTab/Shift+Tablisteners (capture phase) that cancel pending retries via afinish()helper; printable keys still don't cancel the initial claim, and delayed programmatic focus restoration is still reclaimed. - Explicit-leaf resolution in
focusRuntimeTerminalSurface—sync-runtime-graph.tsresolves a requestedleafIdand activates the requested pane withfocus: !paneIsCoveredByNativeChat(requestedPane), so a covered chat leaf activates without focusing its xterm while an uncovered split terminal leaf in the same tab still takes focus. - New and updated tests —
TerminalPaneNativeChatPortal.test.tsxownership checks, four covered/uncovered leaf-activation cases infocus-runtime-terminal-surface-chat-view.test.ts, plus hook tests isolating user-intent cancellation from delayed programmatic restoration.
I traced the delta end-to-end: the isFocusedGroup derivation against activePaneIsChatLeaf (use-terminal-pane-projection.ts:160), the runtime path's new leafId branch against resolveLeafIdForManager and paneIsCoveredByNativeChat, and the hook's cleanup/finish() lifecycle. The new tests assert exact values ({ focus: false } / { focus: true }, toBe(1)/toBe(2)), not loose predicates, and each would fail on regression.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| @@ -0,0 +1,290 @@ | |||
| // @vitest-environment happy-dom | |||
There was a problem hiding this comment.
Still unaddressed from the prior review: the createRoot + act harness in this file doesn't set globalThis.IS_REACT_ACT_ENVIRONMENT = true, so every run emits The current testing environment is not configured to support act(...) warnings (tests still pass).
| // @vitest-environment happy-dom | |
| // @vitest-environment happy-dom | |
| globalThis.IS_REACT_ACT_ENVIRONMENT = true |
There was a problem hiding this comment.
Acknowledged. This is nonfunctional test-harness warning hygiene, and the tests plus CI pass. I am leaving it out to keep this review commit limited to meaningful behavior and regression coverage.
Review status: CLEAN — recommend landThe review/fix commit is 3ad37df. Two adversarial review rounds and the closing readiness audit found no remaining in-scope P0, P1, or P2 issue. Issues fixed
Functional correctnessPASS. New-chat typing, switch-away/back focus, covered-chat activation, and mixed chat/terminal split routing are covered. The final Electron run visibly proved that typing lands in a fresh composer without a click, focus returns after switching away and back, and a revealed mixed split preserves the active terminal sibling and routes input to its PTY. Final Electron screenshots and backing state: #19868 (comment) Architecture and precedentPASS. Focus ownership and direct delivery follow the active group/leaf model: the selected leaf is activated, and only the surface that actually owns that leaf may claim focus. The bounded six-frame verification is a deliberate local lifecycle adaptation for retained panes and delayed focus restoration; it is not used as a replacement for ownership. Performance and resourcesPASS. At most six animation-frame attempts run per reveal, normally two, with only one frame pending at once. Temporary document listeners are removed on success, cancellation, exhaustion, dependency change, and unmount. No polling, subprocess churn, broad scans, caches, persistent growth, or hot-path fanout was added. Validation
Compatibility and remaining gapsNo mobile-facing UI, persistence, RPC/wire, process, shell, path, or platform-specific behavior changed, so mobile emulator, Windows/WSL live UI, and SSH live QA were inapplicable. The Electron run validated focus and input routing rather than submitting a billed model request. The requested Grok lane had no remaining usage, so the final Electron validation was completed by the fallback QA worker. Remaining in-scope issues: none. |
ELI5
You open a chat and the cursor isn't in the message box, so you have to click before you can type. Now it's already there.
What Changed
A new hook,
useNativeChatComposerRevealFocus, focuses the chat composer whenever a chat pane reveals it. It's called from both chat views, which already held acomposerRef, so nothing new had to be plumbed.It fires on the reveal edge, not on mount. Structured panes are retained — hidden with
display:none+inertand never unmounted on a tab switch (RetainedPaneHost.tsx:145-157) — so a mount-only effect would miss every switch-back. Covered events: a new chat tab, a worktree-create that lands in chat, the Cmd/Ctrl+Shift+J view toggle, and re-selecting an existing chat tab.It reuses the existing
NativeChatComposerHandle.focus()seam and the existingshouldPreserveEditableFocuspredicate (pane-helpers.ts, now exported) rather than adding a parallel mechanism. No Tiptapautofocus— that would be a second, competing path.The first attempt is deferred one frame and retried over a bounded run of frames, because two things can beat a same-tick take: Tiptap publishes its imperative adapter only after
EditorContentattaches, and Radix'sFocusScoperestores a closing dialog's trigger in asetTimeout(0). The verification stops once focus remains in the pane, so the happy path is two frames. If a closing dialog restores focus afterward, the next frame reclaims the composer. Pointer input or Tab/Shift+Tab navigation cancels the remaining attempts so user intent wins; printable keys do not cancel the initial claim.Two supporting changes:
isFocusedGroup, derived fromactiveGroupIdByWorktree. On worktree activationisWorktreeActiveflips once for the whole layer, so in a 2-column split both chat panes go visible in the same commit and would fight over the caret. The bridge route already had this bit ascontroller.isActive(TerminalPaneOverlayLayer.tsx:142); only the structured overlay needed it added.A focus-steal fix in
focusRuntimeTerminalSurface. Legacy chat is aterminaltab withviewMode: 'chat'whose xterm stays mounted underneath. The DOM-path twin already declines chat view viadata-terminal-chat-view(focus-terminal-tab-surface.ts:74-77), but the runtime path — whichqueueWorkspaceActivationTerminalFocustries first — focused the covered xterm unconditionally and pulled the caret back out of the composer. The runtime path now resolves any explicit leaf before applying that policy. It activates an uncovered requested leaf with{ focus: true }, and a covered requested chat leaf with{ focus: false }; that preserves leaf selection without focusing the hidden xterm. Returningtruestill prevents the DOM fallback from targeting an unrelated xterm.Why
Nothing in the chat surface focused the composer on open. The only existing focus calls were reactive — typing on the bridge pane background, accepting a picker item, resolving an attachment, starting dictation — and the structured pane had none of them. The view-mode toggle ended at
toggleTabViewModeand focused nothing.A pane-local reveal effect was chosen over a store flag like the browser's
pendingAddressBarFocusByTabId. That flag carries a one-shot creation intent across a mount gap and is consumed so revisits don't refocus — the opposite of what's wanted here. It would need re-arming at eight sites, each a silent bug if missed, all re-deriving what the pane layer already knows.Linked Issue
No existing open issue covers this; it came from a direct request. Happy to file one and link it if that's preferred.
Visual Proof
Same flow both times: open a chat from the tab-bar "+" → Claude, then type
typingwithout clicking anything.BEFORE —
document.activeElementisBODY, the six keystrokes go nowhere, composer still shows its placeholder:AFTER — focus is the composer's ProseMirror contenteditable and the text lands in it:
Captured on macOS against this branch, in one app session, with only the hook toggled between the two shots — so the difference is attributable to this change and not to setup drift.
Testing
Ran in the real app (macOS) on isolated CDP ports and throwaway profiles, driving the actual tab controls. The final current-source Electron evidence covers the settled source:
agent-sessionchat.document.activeElementonBODY. Re-enabled, it focuses again.Automated:
native-chat-composer-reveal-focus.test.tsx— 17 tests: reveal/focused-group edges, split ownership, latch/re-arm behavior, bounded delayed-restoration recovery, preserving editable focus, pointer and Tab cancellation, immediate printable-key behavior, and covered-xterm handoff.focus-runtime-terminal-surface-chat-view.test.ts— explicit covered and uncovered leaf activation, including mixed splits.TerminalPaneNativeChatPortal.test.tsx— active-leaf focus ownership on both PTY-backed and structured routes.workspace-activation-terminal-focus.test.ts,StructuredAgentSessionPaneOverlayLayer.test.tsx, and fixed container-less fake panes insync-runtime-graph-terminal-registration-ownership.test.ts.Each guard was ablated individually and confirmed to turn its test red. Worth calling out: the first version of the hook test passed for the wrong reason — the four guards masked each other pairwise, so removing the latch or the in-pane check changed nothing. Two tests were added that isolate them.
Gates on the final review source:
pnpm tcclean, 7 focused files / 61 tests green (including 17 hook and 2 portal cases),check:code-quality:changedreports 0 new findings, oxlint clean, andgit diff --checkclean. Post-push CI is green: 20 successful checks, 14 intentionally skipped, and no failures or pending checks.Not covered: Linux and Windows were not exercised (no platform-specific code in this change — no keyboard modifiers, no paths). SSH/remote was not exercised live; the runtime-path guard is the piece that matters there and it is unit-tested. The batch worktree-create case — several worktrees in sequence, where the composer must not steal the modal's next name field — is covered by unit test but was not exercised live.
AI Disclosure
Claude Opus 4.5 via Claude Code and Codex.
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
metaKeyhardcoding, no path handling, no shell.canSendstill reflects the mobile presence-lock; a locked composer is non-editable, sofocus()returns false and the bounded retry simply expires.isFocusedGroupis a required prop deliberately — an omitted prop would silently disable the feature for a route, so the typecheck is the forcing function.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)