fix(cli): restore SubAgent shortcut focus#3771
Conversation
|
after fixed: Kapture.2026-04-30.at.17.56.02.mp4 |
Known limitation: parallel SubAgent focusThis PR intentionally does not address the case where multiple SubAgents run in parallel — only the first running SubAgent receives Ctrl+E/Ctrl+F shortcut focus. This is a design constraint inherited from #3721 (the Why not fix it here: Solving multi-agent focus switching requires a new UX mechanism (e.g., a focus-cycling shortcut like Ctrl+Tab, or index-based Ctrl+1/2/3). That's a separate design decision beyond the scope of this bug fix, which only restores the single-agent shortcut that #3721 accidentally broke. Tracked as follow-up: #3770 |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
left a comment
There was a problem hiding this comment.
Multi-agent review (4 agents: Correctness, Test Coverage, 3am-oncall, Maintainer). Correctness: no issues. All 3 stated focus constraints (pending-confirmation wins, direct-tool-confirmation blocks, parallel running don't all respond) are correctly enforced; 5-round audit found nothing to fix.
Suggestions in inline comments below. Two additional suggestions reference files outside this diff and don't anchor to a line — they're listed here:
[Suggestion] Stale isFocused JSDoc on receivers — ToolMessage.tsx:345 and AgentExecutionDisplay.tsx:32 (neither file in this diff) still document isFocused as controlling "this tool's subagent confirmation prompt". After this PR, isFocused arriving at AgentExecutionDisplay also gates the Ctrl+E / Ctrl+F useKeypress listener (line 259). The PR updated the inline comment in ToolGroupMessage.tsx but not the receiver props' JSDoc. A future maintainer reading AgentExecutionDisplayProps.isFocused would conclude it's only about confirmations and "fix" the useKeypress({ isActive: data.status === 'running' && isFocused }) to drop isFocused — re-introducing the parallel-subagent dual-toggle bug. Fix: update both JSDocs to: Whether this subagent owns the keyboard input — both for its inline confirmation prompt and for the Ctrl+E / Ctrl+F display-mode shortcuts.
[Suggestion] +N more (ctrl+e to expand) hint vs. ownership are decoupled — AgentExecutionDisplay.tsx:288-295 (and footer at :209-235) show the hint whenever data.status === 'running', NOT gated on isFocused. With two parallel running SubAgents, both display the hint but only the first responds. The UI is genuinely lying — exactly the root-cause class this PR addresses, but for the multi-subagent case the signage stays misleading even after the listener is fixed. Fix: either (a) gate the hint on isFocused, or (b) reword to e.g. "ctrl+e expands the active subagent". Either is acceptable; (a) more precise, (b) lower friction.
— claude-opus-4-7 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
LGTM. Verified all five suggestions and consider them non-blocking — fine to land as-is, but flagging #2 as worth folding into this PR if the edit is low-friction.
Triage of the inline / review-body suggestions
1. gpt-5.5 inline @ ToolGroupMessage.tsx:291 — Ctrl+F double-binding — non-blocking. DefaultAppLayout.tsx:53 is an isAgentTab ? AgentChatView : MainContent either-or, so the SubAgent-bearing ToolGroupMessage and AgentChatView's shell-focus Ctrl+F handler don't co-mount on the common path. Only the nested case (SubAgent inside an agent chat + active PTY) would double-fire, which is genuinely edge.
2. claude-opus-4-7 review body — stale isFocused JSDoc on receivers — non-blocking but worth fixing in this PR if convenient. ToolMessage.tsx:345 and AgentExecutionDisplay.tsx:32 props JSDoc still describe isFocused as the confirmation focus lock only. After this PR that same prop also gates useKeypress({ isActive: data.status === 'running' && isFocused }) in AgentExecutionDisplay.tsx:259. A future maintainer reading the prop doc in isolation could "simplify" the isActive to drop && isFocused, silently re-introducing the parallel-subagent dual-toggle bug from #3721. One-line JSDoc tweak in each file (~5 lines total). Suggested wording:
Whether this subagent owns the keyboard input — both for its inline confirmation prompt and for the Ctrl+E / Ctrl+F display-mode shortcuts.
Up to you — happy to land as-is and follow up, or fold it in here for a slightly safer ship.
3. claude-opus-4-7 review body — hint vs. ownership decoupled — non-blocking, tracked in #3770.
4. claude-opus-4-7 inline @ ToolGroupMessage.tsx:147 — TODO breadcrumb + debug log — non-blocking improvement.
5. claude-opus-4-7 inline @ ToolGroupMessage.test.tsx:275 — test depth — current tests cover static prop wiring well; behavioral integration coverage (real Ctrl+E keypress through the chain, ownership transitions, isFocused={false} parent) can be follow-up.
Verification
- merge-base =
6efcf2b87; PR diff is exactly 2 files / 220 lines origin/main'sAgentExecutionDisplay.tsx:259is already{ isActive: data.status === 'running' && isFocused }, so the new ownership-fallback in this PR does flow through to the actual keypress listener — fix is real- 13/13 CI jobs green
LGTM — folding #2 in or following up is your call.
a776194 to
d29cbce
Compare
|
Follow-up from review: updated the receiver |
wenshao
left a comment
There was a problem hiding this comment.
Re-approving on d29cbce — my prior approval was stale-dismissed by the new commit.
All five points from the earlier triage are addressed:
- #2 (JSDoc footgun) — fixed in this commit. Both
ToolMessage.tsx:342-348andAgentExecutionDisplay.tsx:29-35JSDoc now read "Whether this subagent owns keyboard input for confirmations and Ctrl+E/Ctrl+F display shortcuts", removing the footgun for future maintainers. - #5 (test depth) —
isFocused={false}parent + running SubAgent regression test added; keypress integration / ownership-churn left as follow-up scope, which I'm fine with. - #1, #3, #4 — kept out of this PR with sound rationale (#3770 / scope discipline). Agreed.
CI 13/13 green, real diff vs origin/main is 4 files / +247 -5. LGTM.
Summary
isFocused, but that signal originally represented the SubAgent confirmation focus lock. A normally running SubAgent could therefore showctrl+e to expandwhile the shortcut listener stayed inactive.Validation
/review packages/cli/src/ui/components/subagents/runtime/AgentExecutionDisplay.tsxcan be used to trigger SubAgent display updates, then press Ctrl+E after+N more tool calls (ctrl+e to expand)appears./reviewmanually and press Ctrl+E once a running SubAgent shows the compact expand hint.Scope / Risk
Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Fixes #3763