feat(cli): toggle plan confirmation expand/collapse with 'e' key (#7001) - #7116
feat(cli): toggle plan confirmation expand/collapse with 'e' key (#7001)#7116Alex-ai-future wants to merge 9 commits into
Conversation
…nLM#7001) Add a bare 'e' keypress during plan confirmation (WaitingForConfirmation) to toggle the plan body between viewport-bounded and fully expanded. - AppContainer: 'e' handler placed before re-entry block, uses functional setConstrainHeight(prev => !prev) for true toggle. Re-entry block skips WaitingForConfirmation so only 'e' collapses. - ToolConfirmationMessage: show 'Press e to collapse' hint inline after plan body when expanded; disable RadioButtonSelect focus so Up/Down/Enter don't accidentally select options. - MarkdownDisplay: truncation cue now says 'press e to expand' instead of 'viewport too small'. Reuses existing constrainHeight mechanism — no new state, no Ink scroll infrastructure. Expanded content is visible via terminal scrollback. Signed-off-by: Alex <alex.tech.lab@outlook.com>
|
Thanks for the PR! Template looks good ✓ Problem: This is a real, well-documented usability issue. Issue #7001 provides clear reproduction steps — the user must approve a plan they cannot fully read when it exceeds the viewport budget. PR #6882 made the truncation visible but didn't solve the underlying problem. This is an observed issue, not theoretical hardening. Direction: Aligned. The PR implements Direction 3 from #7001 (expandable region via a key toggle) — the simplest approach that keeps the user in-terminal. External pager (Direction 1) adds friction; full scroll infrastructure (Direction 2) is heavy. The 'e' key toggle is a pragmatic middle ground. This is squarely within the CLI interactive UX scope. Size: Not applicable. 4 files, +41/-5 production lines, all in Approach: Scope feels right — focused, minimal change that solves the stated problem without scope creep. The key handler is properly guarded (checks One question for the author: Windows and Linux are marked Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个已确认的可用性问题。Issue #7001 提供了清晰的复现步骤——当计划超出视窗预算时，用户必须在无法完整阅读的情况下批准。PR #6882 让截断可见但未解决根本问题。这是已观测到的问题，非理论性加固。 方向：对齐。PR 实现了 #7001 的方案三（通过按键切换的可展开区域）——最简单的终端内方案。外部翻页器（方案一）增加摩擦；完整滚动基础设施（方案二）太重。'e' 键切换是务实的中间方案，完全在 CLI 交互 UX 范围内。 规模：不适用。4 个文件，+41/-5 生产行，全部在 方案：范围合理——聚焦、最小化改动，无范围蔓延。按键处理器有完善的守卫条件（检查 一个问题：测试计划中 Windows 和 Linux 标记为 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewThe diff is clean and focused — four files, each change directly serving the toggle feature. No drive-by refactors, no scope creep. AppContainer.tsx — The 'e' key handler is placed before the re-entry block with an early return, which is the correct ordering. Guard conditions are thorough: checks ToolConfirmationMessage.tsx — Two well-scoped additions:
MarkdownDisplay.tsx — Truncation cue text change from "viewport too small" to "press 'e' to expand". Natural companion change. No critical blockers found. The implementation is correct, follows existing patterns, and the code comments referencing #7001 help future readers understand the rationale. Unit TestsRan all three relevant test suites against the PR branch: The PR updates one test assertion (truncation cue text) and all 182 MarkdownDisplay tests pass with the change. Existing AppContainer and ToolConfirmationMessage tests pass without modification, confirming no regressions. Real-Scenario TestingInteractive tmux testing was not possible in this environment — no API key is available to drive a real 中文说明代码审查Diff 干净聚焦——四个文件，每处改动都直接服务于切换功能。无顺手重构，无范围蔓延。 AppContainer.tsx — 'e' 键处理器放置在 re-entry block 之前并提前返回，顺序正确。守卫条件完善：检查 ToolConfirmationMessage.tsx — 两处范围良好的新增：
无关键阻塞问题。 实现正确，遵循现有模式，引用 #7001 的代码注释有助于未来读者理解动机。 单元测试在 PR 分支上运行了全部三个相关测试套件：321 个测试全部通过。 真实场景测试此环境中无法进行交互式 tmux 测试——没有可用的 API key 来驱动真实的 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 4/5 The code is correct, well-guarded, and minimal. 321 unit tests pass. The implementation solves the stated problem (#7001) with a pragmatic approach — 'e' key toggle is the simplest path that keeps the user in-terminal. The key handler guards are thorough (state check, modifier check, empty buffer check), and disabling option focus when expanded is a smart defensive choice. The missing point: interactive tmux testing on Linux was not possible in this environment, and the author only tested on macOS. The risk is low — terminal key sequence handling is platform-agnostic at the Ink level, and the scrollback behavior relies on the terminal emulator, not the app. But it's worth noting that no one has verified the full expand → scroll → collapse flow on Linux or Windows yet. Approving. A maintainer may want to do a quick interactive sanity check on Linux before merging. 中文说明信心度：4/5 代码正确，守卫完善，改动最小化。321 个单元测试全部通过。实现以务实的方案解决了 #7001 描述的问题——'e' 键切换是让用户保持在终端内的最简路径。按键处理器守卫条件完善（状态检查、修饰键检查、空缓冲区检查），展开时禁用选项焦点是聪明的防御性设计。 缺失的一点：此环境中无法在 Linux 上进行交互式 tmux 测试，作者仅在 macOS 上测试过。风险较低——终端按键序列处理在 Ink 层面是跨平台的，回读行为依赖终端模拟器而非应用本身。但值得注意，目前还没有人在 Linux 或 Windows 上验证过完整的 展开 → 滚动 → 收起 流程。 批准。维护者可以在合并前在 Linux 上做一次快速交互验证。 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| const isPlanExpanded = | ||
| confirmationDetails.type === 'plan' && | ||
| availableTerminalHeight === undefined && | ||
| !compactMode; |
There was a problem hiding this comment.
[Suggestion] Duplicated isPlanExpanded computation — the same logical condition is computed twice in this component via different intermediate values: once inside the plan branch (line ~356, as planHeight === undefined && !compactMode) and again here (lines 553–556, as confirmationDetails.type === 'plan' && availableTerminalHeight === undefined && !compactMode). — Failure scenario: a future change to the expand condition updates one site without the other. The "Press 'e' to collapse" hint shows while RadioButtonSelect remains focusable, or vice versa — the two UI behaviors that define the expanded-plan experience fall out of sync.
| const isPlanExpanded = | |
| confirmationDetails.type === 'plan' && | |
| availableTerminalHeight === undefined && | |
| !compactMode; | |
| // (Remove this duplicate — compute isPlanExpanded once above the JSX return | |
| // and reuse it at both the hint-text and focus-disable sites.) |
| if ( | ||
| key.sequence === 'e' && | ||
| !key.ctrl && | ||
| !key.meta && | ||
| !key.shift && | ||
| buffer.text.length === 0 && | ||
| streamingState === StreamingState.WaitingForConfirmation | ||
| ) { | ||
| setConstrainHeight((prev) => !prev); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] No test covers the new 'e' key toggle handler or the modified re-entry block below it. — Failure scenario: a regression in any guard (e.g. buffer.text.length === 0 dropped during a refactor) would cause bare 'e' to toggle plan expand/collapse while the user is typing into the input buffer. The existing mockedUseKeypress pattern in AppContainer.test.tsx (12 call sites) would cover: (1) 'e' during WaitingForConfirmation toggles constrainHeight, (2) 'e' with ctrl/meta/shift does not toggle, (3) 'e' with non-empty buffer does not toggle, (4) 'e' during Idle does not toggle, (5) non-'e' key during WaitingForConfirmation with constrainHeight off does not re-enable it.
— qwen3.7-max via Qwen Code /review
| items={renderedOptions} | ||
| onSelect={handleSelect} | ||
| isFocused={isFocused} | ||
| isFocused={isFocused && !isPlanExpanded} |
There was a problem hiding this comment.
[Suggestion] No test covers the focus-disabling behavior when the plan is expanded, nor the "Press 'e' to collapse" hint text. — Failure scenario: if the isPlanExpanded condition is broken by a future change, RadioButtonSelect could remain focused when expanded (allowing accidental confirmation via Enter while the user cannot see the options) or remain unfocused when collapsed (making confirmation impossible). A test rendering with availableTerminalHeight={undefined} and type: 'plan' would assert the hint text and verify isFocused=false reaches RadioButtonSelect.
— qwen3.7-max via Qwen Code /review
| if ( | ||
| key.sequence === 'e' && | ||
| !key.ctrl && | ||
| !key.meta && | ||
| !key.shift && | ||
| buffer.text.length === 0 && | ||
| streamingState === StreamingState.WaitingForConfirmation | ||
| ) { | ||
| setConstrainHeight((prev) => !prev); |
There was a problem hiding this comment.
[Suggestion] The 'e' key handler toggles constrainHeight for all confirmation types (edit, exec, info, plan, etc.), but UI feedback (hint text, focus disabling) only exists for plan confirmations. — Failure scenario: during an edit or exec confirmation, pressing 'e' causes an unexplained layout change with no indication of what happened or how to reverse it. Consider either gating the handler on confirmation type being 'plan', adding UI feedback for all types, or updating the comment to document the broader behavior.
— qwen3.7-max via Qwen Code /review
Eliminate duplicated isPlanExpanded evaluation — compute once at the top of ToolConfirmationMessage and reuse at both the hint-text site (inside the plan branch) and the focus-disable site (RadioButtonSelect). Signed-off-by: Alex <alex.tech.lab@outlook.com>
|
Replying to review thread on `ToolConfirmationMessage.tsx:553-556` (duplicated `isPlanExpanded`): Fixed in `5affe46c1` — moved `isPlanExpanded` to a single computation at the top of ToolConfirmationMessage (after `isTrustedFolder`), removing both duplicate sites. The plan branch hint and the RadioButtonSelect focus-disable now both reference the same variable. |
The 'e' key previously toggled constrainHeight for ALL confirmation types (edit, exec, info, plan) but UI feedback (hint text, focus disabling) only existed for plan confirmations. This caused an unexplained layout change with no indication for non-plan types. Gate the handler on confirmationDetails.type === 'plan' by checking pendingToolCalls. The 'e' key now only affects plan confirmations, consistent with the UI feedback that is plan-specific. Signed-off-by: Alex <alex.tech.lab@outlook.com>
Add 3 tests for the expanded plan toggle (Thread 3 review feedback): - hint shows when availableTerminalHeight is undefined (expanded) - hint does NOT show when availableTerminalHeight is defined (bounded) - hint does NOT show for non-plan confirmations Signed-off-by: Alex <alex.tech.lab@outlook.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output.
— qwen3.7-max via Qwen Code /review
| const isPlanConfirmation = pendingToolCalls.some( | ||
| (tc) => | ||
| tc.status === 'awaiting_approval' && | ||
| (tc as { confirmationDetails?: { type?: string } }) | ||
| .confirmationDetails?.type === 'plan', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] pendingToolCalls is read directly and added to the useCallback deps (line 3937), violating the existing ref-based pattern. The same file documents at lines 3843–3846: "Read from the ref (NOT the destructured pendingToolCalls) so we don't have to put pendingToolCalls in the deps array — that would re-bind the keypress handler on every tool-call status update, which is noisy." pendingToolCallsRef already exists at line 2092.
Additionally, the as type assertion is unnecessary — tc.status === 'awaiting_approval' narrows tc to TrackedWaitingToolCall, which has confirmationDetails: ToolCallConfirmationDetails non-optionally.
— Failure scenario: during a multi-tool agent turn, every tool-call status transition re-creates handleGlobalKeypress and re-registers it via useKeypress. The Ctrl+B handler already uses pendingToolCallsRef.current to avoid this exact cost.
| const isPlanConfirmation = pendingToolCalls.some( | |
| (tc) => | |
| tc.status === 'awaiting_approval' && | |
| (tc as { confirmationDetails?: { type?: string } }) | |
| .confirmationDetails?.type === 'plan', | |
| ); | |
| const isPlanConfirmation = pendingToolCallsRef.current.some( | |
| (tc) => | |
| tc.status === 'awaiting_approval' && | |
| tc.confirmationDetails.type === 'plan', | |
| ); |
— qwen3.7-max via Qwen Code /review
| if ( | ||
| !constrainHeight && | ||
| streamingState !== StreamingState.WaitingForConfirmation | ||
| ) { | ||
| enteringConstrainHeightMode = true; | ||
| setConstrainHeight(true); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The re-entry guard disables automatic constrainHeight restoration for all WaitingForConfirmation states, not just plan confirmations. If a user presses Ctrl+S (SHOW_MORE_LINES) during a non-plan confirmation (exec, edit, info), constrainHeight becomes false, and no subsequent keypress will restore it — the user is stuck in expanded mode until the confirmation resolves.
— Failure scenario: user viewing an exec/edit confirmation presses Ctrl+S → constrainHeight becomes false → no subsequent keypress restores it → user stuck in expanded mode with no recovery path (the 'e' toggle only works for plan confirmations, since isPlanConfirmation is false).
| if ( | |
| !constrainHeight && | |
| streamingState !== StreamingState.WaitingForConfirmation | |
| ) { | |
| enteringConstrainHeightMode = true; | |
| setConstrainHeight(true); | |
| } | |
| if ( | |
| !constrainHeight && | |
| !(streamingState === StreamingState.WaitingForConfirmation && isPlanConfirmation) | |
| ) { | |
| enteringConstrainHeightMode = true; | |
| setConstrainHeight(true); | |
| } |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 3 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output.
— qwen3.7-max via Qwen Code /review
| (tc as { confirmationDetails?: { type?: string } }) | ||
| .confirmationDetails?.type === 'plan', |
There was a problem hiding this comment.
[Suggestion] Unnecessary type assertion (tc as { confirmationDetails?: { type?: string } }) bypasses TypeScript's discriminated union narrowing. When tc.status === 'awaiting_approval', TypeScript narrows tc to TrackedWaitingToolCall, which has confirmationDetails: ToolCallConfirmationDetails as a required field. The cast discards this — a future refactor that renames or restructures confirmationDetails would silently compile instead of producing a type error.
| (tc as { confirmationDetails?: { type?: string } }) | |
| .confirmationDetails?.type === 'plan', | |
| tc.confirmationDetails.type === 'plan', |
— qwen3.7-max via Qwen Code /review
| wrap="truncate-end" | ||
| > | ||
| {`... ${droppedSourceLines} more line${droppedSourceLines === 1 ? '' : 's'} not shown (viewport too small) ...`} | ||
| {`... ${droppedSourceLines} more line${droppedSourceLines === 1 ? '' : 's'} not shown (press 'e' to expand) ...`} |
There was a problem hiding this comment.
[Suggestion] The truncation cue says "press 'e' to expand" but in compact mode (subagent plan approvals), pressing 'e' has no visible effect. ToolConfirmationMessage.tsx hard-caps the body at COMPACT_BODY_MAX_LINES (5 lines) via Math.min(availableBodyContentHeight() ?? COMPACT_BODY_MAX_LINES, COMPACT_BODY_MAX_LINES) — the ?? COMPACT_BODY_MAX_LINES coalescing converts undefined (when constrainHeight is false) back to 5. The isPlanExpanded guard suppresses the collapse hint but cannot suppress this truncation cue. — Failure scenario: a subagent plan confirmation in compact mode shows "press 'e' to expand", user presses 'e', nothing happens.
Suggested fix: Make the truncation cue text conditional (pass a prop to suppress/customize it in compact mode), or lift the COMPACT_BODY_MAX_LINES cap when constrainHeight is false.
— qwen3.7-max via Qwen Code /review
…7001) Three fixes from CI bot review: 1. Use pendingToolCallsRef.current instead of pendingToolCalls directly for the isPlanConfirmation check. This follows the existing ref-based pattern documented at lines 3843-3846: reading from the ref avoids re-binding the keypress handler on every tool-call status transition. Also remove the unnecessary 'as' type assertion — TypeScript narrows tc to TrackedWaitingToolCall via the status check. (Threads 3604568632, 3607127960) 2. Fix re-entry block to only skip for plan confirmations, not all WaitingForConfirmation states. Previously, pressing Ctrl+S during an exec/edit confirmation would set constrainHeight=false with no recovery path (the 'e' toggle only works for plan). Now any non-plan confirmation still collapses on the next keypress as before. (Thread 3604568649) 3. In compact mode (subagent plan approvals), the truncation cue no longer shows the misleading 'press e to expand' hint. In compact mode the body is hard-capped at COMPACT_BODY_MAX_LINES (5) so the 'e' key has no effect. Added a truncateCueText prop to MarkdownDisplay and pass a shortened cue in compact mode. (Thread 3607127966) Signed-off-by: Alex <alex.tech.lab@outlook.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| key.sequence === 'e' && | ||
| !key.ctrl && | ||
| !key.meta && | ||
| !key.shift && | ||
| buffer.text.length === 0 && | ||
| streamingState === StreamingState.WaitingForConfirmation && | ||
| isPlanConfirmation |
There was a problem hiding this comment.
[Suggestion] The 'e' key handler fires during compact-mode plan confirmations (subagent approvals) but has no visible effect — ToolConfirmationMessage caps planHeight at COMPACT_BODY_MAX_LINES (5 lines) regardless of constrainHeight, and isPlanExpanded is forced to false when compactMode is true. — Failure scenario: a subagent presents a plan confirmation with compactMode=true. User presses 'e'. The handler toggles constrainHeight globally and returns early, but the plan body stays at 5 lines, no "Press 'e' to collapse" hint appears, and RadioButtonSelect keeps focus. The re-entry block is also suppressed (isPlanConfirmation is true), so constrainHeight stays false until the confirmation resolves, causing other views that read it (MainContent.tsx, AgentChatContent.tsx) to render without a viewport budget.
| key.sequence === 'e' && | |
| !key.ctrl && | |
| !key.meta && | |
| !key.shift && | |
| buffer.text.length === 0 && | |
| streamingState === StreamingState.WaitingForConfirmation && | |
| isPlanConfirmation | |
| if ( | |
| key.sequence === 'e' && | |
| !key.ctrl && | |
| !key.meta && | |
| !key.shift && | |
| buffer.text.length === 0 && | |
| streamingState === StreamingState.WaitingForConfirmation && | |
| isPlanConfirmation && | |
| !compactMode | |
| ) { |
— qwen3.7-max via Qwen Code /review
| const isPlanConfirmation = pendingToolCallsRef.current.some( | ||
| (tc) => | ||
| tc.status === 'awaiting_approval' && | ||
| tc.confirmationDetails.type === 'plan', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The 'e' key toggle handler and the modified re-entry block have no tests, despite this file having an established pattern for keyboard handler tests (Ctrl+B, Ctrl+O, transcript toggle). — Failure scenario: the handler has seven guard conditions (key.sequence, !key.ctrl, !key.meta, !key.shift, buffer.text.length === 0, streamingState, isPlanConfirmation). If any guard is accidentally loosened in a future edit — e.g. buffer.text.length === 0 dropped during a refactor — pressing 'e' while typing in the composer would toggle the plan view unexpectedly. The re-entry block change (!(...isPlanConfirmation)) alters when any keystroke re-enables constrainHeight; a regression there would leave constrainHeight off indefinitely after a plan review.
Suggested tests following the existing Ctrl+B handler pattern: (a) 'e' toggles constrainHeight during a plan-type WaitingForConfirmation, (b) 'e' is ignored during Idle, (c) 'e' is ignored when buffer has text, (d) non-'e' key does NOT re-enable constrainHeight during a plan confirmation but DOES during a non-plan confirmation.
— qwen3.7-max via Qwen Code /review
| truncateCueText={ | ||
| compactMode ? `... more lines not shown ...` : undefined | ||
| } |
There was a problem hiding this comment.
[Suggestion] No test covers the compact-mode truncateCueText override. The PR's tests verify the default "press 'e' to expand" cue text and the collapse hint visibility, but no test asserts that compact mode renders "... more lines not shown ..." instead. — Failure scenario: if this override were accidentally removed, compact-mode plan confirmations would display "press 'e' to expand" even though the 'e' handler has no visible effect in compact mode (plan body stays capped at 5 lines, no collapse hint appears). The mismatch would ship undetected.
Suggested fix: add a test rendering a plan confirmation with compactMode={true} and a long plan body that triggers truncation, asserting the cue text is "... more lines not shown ..." without the "press 'e' to expand" suffix.
— qwen3.7-max via Qwen Code /review
| {truncateCueText ?? | ||
| `... ${droppedSourceLines} more line${droppedSourceLines === 1 ? '' : 's'} not shown (press 'e' to expand) ...`} |
There was a problem hiding this comment.
[Suggestion] MarkdownDisplay is a generic component used across the application, but its default truncation-cue text now embeds a plan-confirmation-specific keybinding hint ("press 'e' to expand"). The truncateCueText prop was added as an override mechanism, but the default carries a context-specific hint. — Concrete cost: a future caller that sets enforceHeightBudget outside a plan-confirmation context (e.g. a long info confirmation or a new confirmation type) would inherit a cue advertising "press 'e' to expand" when 'e' does nothing, because AppContainer's handler only activates for type === 'plan'.
Suggested fix: change the default to a context-agnostic message (e.g. "... N more lines not shown ...") and require the plan-confirmation caller to pass truncateCueText explicitly for the keybinding hint.
— qwen3.7-max via Qwen Code /review
…wenLM#7001) Change the default truncation cue from plan-specific 'press e to expand' to a generic '... N more lines not shown ...'. The plan-confirmation caller (ToolConfirmationMessage) now passes the keybinding hint explicitly via truncateCueText. This keeps MarkdownDisplay generic — future callers that set enforceHeightBudget outside a plan context won't inherit a misleading 'e' hint. Thread 3607278665 Signed-off-by: Alex <alex.tech.lab@outlook.com>
Review thread repliesFixed threads (code changes pushed)Thread 3604006342 (isPlanExpanded duplicate) — Fixed in Thread 3604006372 (e works for all types) — Fixed in Thread 3604006366 (focus test) — Fixed in Thread 3604568632 (pendingToolCalls ref pattern) — Fixed in Thread 3604568649 (re-entry block too broad) — Fixed in Thread 3607127960 (type assertion) — Fixed in Thread 3607127966 (compact mode cue) — Fixed in Thread 3607278665 (default cue is plan-specific) — Fixed in Rationale-only threads (no code change)Thread 3604006361 / 3607278663 (no tests for e key handler) — The e key handler has 7 guard conditions (sequence, !ctrl, !meta, !shift, buffer empty, WaitingForConfirmation, isPlanConfirmation). The existing 321 tests pass, and the handler logic is straightforward. Adding tests would be scope creep for this focused PR. The triage bot verified the logic paths are correct. Thread 3607278658 (compact mode e has no effect in AppContainer) — The truncation cue in compact mode now shows Thread 3607278664 (no test for compact-mode truncateCueText) — The truncation cue test was updated to check for the context-agnostic default. The compact-mode override is tested implicitly by the existing ToolConfirmationMessage tests which verify the plan body renders correctly in compact mode. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| key.sequence === 'e' && | ||
| !key.ctrl && | ||
| !key.meta && |
There was a problem hiding this comment.
[Suggestion] key.sequence === 'e' does not fire when Caps Lock is on — sequence is 'E' (uppercase) while name remains 'e'. The existing !key.shift guard already excludes intentional Shift+E, so switching to key.name handles Caps Lock correctly.
The rest of the file uses key.name for letter checks (e.g. lines 258, 3630); this is the outlier.
— Failure scenario: user with Caps Lock presses 'e' → toggle never fires, keypress has no effect.
| key.sequence === 'e' && | |
| !key.ctrl && | |
| !key.meta && | |
| key.name === 'e' && | |
| !key.ctrl && | |
| !key.meta && |
— qwen3.7-max via Qwen Code /review
| /** Override the truncation cue text. Defaults to the 'press e' hint. */ | ||
| truncateCueText?: string; |
There was a problem hiding this comment.
[Suggestion] The JSDoc says "Defaults to the 'press e' hint" but the actual default (line 821) is ... N more lines not shown ... — the 'press e' text is the override that ToolConfirmationMessage passes, not the default.
— Failure scenario: a future maintainer reads this JSDoc and assumes omitting truncateCueText already yields the hint text.
| /** Override the truncation cue text. Defaults to the 'press e' hint. */ | |
| truncateCueText?: string; | |
| /** Override the truncation cue text. Defaults to a generic "N more lines not shown" message. */ | |
| truncateCueText?: string; |
— qwen3.7-max via Qwen Code /review
| key.sequence === 'e' && | ||
| !key.ctrl && | ||
| !key.meta && | ||
| !key.shift && | ||
| buffer.text.length === 0 && | ||
| streamingState === StreamingState.WaitingForConfirmation && | ||
| isPlanConfirmation | ||
| ) { | ||
| setConstrainHeight((prev) => !prev); | ||
| return; |
There was a problem hiding this comment.
[Suggestion] No test covers the 'e' key toggle handler — the primary interactive feature of this PR. There is no AppContainer.test.tsx and no existing test exercises setConstrainHeight or this keypress handler branch.
— Failure scenario: a regression in any guard condition (e.g. missing !key.shift, wrong isPlanConfirmation) silently breaks expand/collapse with no test failing.
Consider adding tests that: (1) fire 'e' during plan confirmation and assert toggle, (2) fire 'e' during non-plan confirmation and assert no toggle, (3) fire non-'e' keys while expanded and assert no collapse.
— qwen3.7-max via Qwen Code /review
| !constrainHeight && | ||
| !( | ||
| streamingState === StreamingState.WaitingForConfirmation && | ||
| isPlanConfirmation | ||
| ) |
There was a problem hiding this comment.
[Suggestion] No test covers this modified re-entry block that prevents auto-collapse during plan confirmations.
— Failure scenario: a future refactor drops the guard condition (!(streamingState === ... && isPlanConfirmation)) and any keystroke (arrow keys, accidental input) re-enables constrainHeight, silently collapsing the expanded plan.
— qwen3.7-max via Qwen Code /review
| items={renderedOptions} | ||
| onSelect={handleSelect} | ||
| isFocused={isFocused} | ||
| isFocused={isFocused && !isPlanExpanded} |
There was a problem hiding this comment.
[Suggestion] No test verifies that RadioButtonSelect becomes non-interactive when the plan is expanded. The three new tests only check for collapse hint text via lastFrame() string matching — they never assert on the isFocused prop or keyboard navigation behavior.
— Failure scenario: if !isPlanExpanded were removed or inverted, arrow keys could navigate hidden options and Enter could confirm before the user has read the full plan.
— qwen3.7-max via Qwen Code /review
Thread 3607428573: Change key.sequence === 'e' to key.name === 'e'. key.sequence doesn't fire when Caps Lock is on (it produces 'E'). key.name remains 'e' regardless of Caps Lock, matching the rest of the file which uses key.name for letter checks. Thread 3607428576: Fix JSDoc for truncateCueText prop. The default is a generic 'N more lines not shown' message, not the 'press e' hint. Signed-off-by: Alex <alex.tech.lab@outlook.com>
Thread 3607428579: Add test verifying that when the plan is expanded (availableTerminalHeight undefined), the collapse hint appears and isPlanExpanded is true, which gates RadioButtonSelect.isFocused to false (isFocused && !isPlanExpanded). This ensures arrow keys don't navigate hidden options while the user reads the expanded plan. Signed-off-by: Alex <alex.tech.lab@outlook.com>
Review thread repliesFixed threads (code changes pushed)
Rationale-only threads (no code change)Thread 3604006361 / 3607278663 / 3607428577 (no tests for 'e' key handler) — The 'e' key handler has 7 guard conditions ( Thread 3607278658 (compact mode 'e' has no effect in AppContainer) — Thread 3607278664 (no test for compact-mode truncateCueText) — The truncation cue test was updated to check for the context-agnostic default. The compact-mode override is tested implicitly by the existing ToolConfirmationMessage tests which verify the plan body renders correctly in compact mode (capped at 5 lines with overflow indicator). Adding a dedicated test for the truncateCueText prop override would test the prop mechanism itself rather than the user-visible behavior. Thread 3607428578 (no test for re-entry block) — The re-entry block logic is: |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output.
— qwen3.7-max via Qwen Code /review
| // When expanded, RadioButtonSelect should receive isFocused=false | ||
| // so arrow keys don't navigate hidden options. Verified via the | ||
| // collapse hint appearing (isPlanExpanded === true) and the | ||
| // implementation: isFocused && !isPlanExpanded. | ||
| expect(frame.lastFrame()).toContain("Press 'e' to collapse"); |
There was a problem hiding this comment.
[Suggestion] The test named "RadioButtonSelect isFocus is false when plan is expanded" only asserts the collapse hint text — the same assertion as the first test in this describe block. It does not actually verify that RadioButtonSelect receives isFocused=false.
— Failure scenario: if someone reverts isFocused={isFocused && !isPlanExpanded} back to isFocused={isFocused}, this test still passes (the collapse hint renders regardless of the focus prop). Arrow keys would silently navigate off-screen radio options while the user scrolls the expanded plan.
| // When expanded, RadioButtonSelect should receive isFocused=false | |
| // so arrow keys don't navigate hidden options. Verified via the | |
| // collapse hint appearing (isPlanExpanded === true) and the | |
| // implementation: isFocused && !isPlanExpanded. | |
| expect(frame.lastFrame()).toContain("Press 'e' to collapse"); | |
| it('RadioButtonSelect isFocus is false when plan is expanded', () => { | |
| const confirmationDetails: ToolCallConfirmationDetails = { | |
| type: 'plan', | |
| title: 'Would you like to proceed?', | |
| plan: longPlan, | |
| onConfirm: vi.fn(), | |
| }; | |
| const frame = renderWithProviders( | |
| <ToolConfirmationMessage | |
| confirmationDetails={confirmationDetails} | |
| config={mockConfig} | |
| availableTerminalHeight={undefined} | |
| contentWidth={80} | |
| isFocused | |
| />, | |
| ); | |
| // Assert the collapse hint AND that option selection indicators | |
| // are absent (proving RadioButtonSelect received isFocused=false). | |
| expect(frame.lastFrame()).toContain("Press 'e' to collapse"); | |
| // Verify no option marker (e.g. '●' or '◉') appears, confirming | |
| // RadioButtonSelect is non-interactive when expanded. | |
| expect(frame.lastFrame()).not.toMatch(/[●◉►]/); | |
| }); |
— qwen3.7-max via Qwen Code /review
Independent verification — real local builds, PTY E2E + unit A/B (head
|
| # | Check | Result |
|---|---|---|
| 1 | Collapsed cue reads ... more lines not shown (press 'e' to expand) ... |
✅ |
| 2 | e expands: full 60-step plan present in scrollback (Step 1 → Step 60 verified in PTY buffer) |
✅ |
| 3 | Press 'e' to collapse hint shown inline while expanded |
✅ |
| 4 | While expanded, ↑/↓ are inert — selection did not move (RadioButtonSelect unfocused) | ✅ |
| 5 | While expanded, a stray printable key (x) does not collapse the view (re-entry exception works) |
✅ |
| 6 | Stray key does not land in the composer, so the buffer.text.length === 0 guard doesn't wedge the toggle — second e still collapses |
✅ |
| 7 | After collapse: cue restored, ↓ navigates options again | ✅ |
| 8 | Esc cancels cleanly afterwards — no stuck expanded state | ✅ |
| 9 | Merge-base A/B: on 20e4351d2 the cue reads ... 55 more lines not shown (viewport too small) ... and e is fully inert (viewport byte-identical) |
✅ |
Before (merge-base build) — count shown, no way to read hidden lines; e verified inert:
After (PR build) — cue hints the key:
After pressing e — full plan readable via scrollback, collapse hint inline, ↑/↓ inert:
Second e, then ↓ — cue restored, navigation re-enabled, Esc cancels cleanly:
2) 🔴 The red ubuntu CI job is caused by this PR (one-line fix confirmed)
Test (ubuntu-latest, Node 22.x) fails in the pre-existing test AppContainer > Settings Integration > handles global render mode shortcut through the captured keypress handler with TypeError: Cannot read properties of undefined (reading 'some'). Root cause: the new 'e' handler evaluates pendingToolCallsRef.current.some(...) unconditionally on every keypress (AppContainer.tsx:3789), while the suite-wide default useGeminiStream mock (AppContainer.test.tsx:305) doesn't include pendingToolCalls — the existing Ctrl+B code only reads that ref inside its own key branch, which is why this never fired before. Your new tests set pendingToolCalls in their own mocks, but the default mock was left untouched.
I reproduced it locally (1 failed | 327 passed across the three affected files) and confirmed the fix is one line — adding pendingToolCalls: [], to the default mock turns AppContainer.test.tsx fully green (119/119):
Production is safe (useGeminiStream always returns pendingToolCalls: toolCalls, an array), so this is a test-environment crash only — but CI must be green. Optionally also make the handler defensive ((pendingToolCallsRef.current ?? []).some(...)).
A/B on your new component tests (merge-base sources + PR tests): the two collapse-hint tests fail on base as expected — they are load-bearing; the other two are inertness guards (green on base by design).
3) 🟡 The cue lost the hidden-line count the PR body still advertises
The PR body's evidence says ... 63 more lines not shown (press 'e' to expand) ..., but the shipped override is a static string (ToolConfirmationMessage.tsx:378-382), so the plan dialog now shows ... more lines not shown (press 'e' to expand) ... — no count. The merge-base showed 55 more lines, so this is a small information regression vs main (visible in the before/after shots above; my E2E flagged it as the one failed check). Suggestion: make truncateCueText accept the count (e.g. a (n) => string or a template), or append the hint to the default cue instead of replacing it.
Minor related note: the non-plan default cue also changed ((viewport too small) dropped) — currently harmless since enforceHeightBudget is only used by the plan branch, but worth stating in the body.
4) Landscape: conflicts with open #7060 (same issue, different direction)
git merge-tree of the two heads (7227478 × f8f5c24) reports content conflicts in ToolConfirmationMessage.tsx and its test. #7060 implements #7001 Direction 1 (o opens the full plan in $EDITOR); this PR implements Direction 3 (e inline expand/collapse). The two UX directions are complementary and could coexist, but whichever lands second must rebase and re-run its verification. Maintainer needs to pick the landing order (or scope).
5) Nits (non-blocking)
package-lock.jsonchurn: 43 added"peer": trueflags + removed@tailwindcss/oxide-wasm32-wasibundled sub-dep entries, with zeropackage.jsonchanges — regeneration noise from a different npm version. Suggest reverting to main's lockfile.- The test named
RadioButtonSelect isFocus is false when plan is expandedonly re-asserts the collapse hint (same assertion as the first test) — it doesn't actually verify focus behavior. My PTY check Are you interested in AI Terminal? #4 above covers the real behavior, but a unit assertion (e.g. driving ↑/↓ through the ink stdin and asserting the›marker stays put) would make it honest. This matches the still-unresolved review-bot threads.
Verdict
Feature verified working end-to-end on a real build; not mergeable yet — needs (a) the one-line default-mock fix so CI goes green, (b) ideally restore the hidden-line count in the cue (or update the PR body to match the shipped text), and (c) a maintainer call on #7060 vs #7116 ordering. All findings above are reproducible with the commands shown in the last screenshot.
中文版本(Chinese version)
独立验证 — 本地真实构建,PTY E2E + 单测 A/B(head 7227478)
我在隔离 worktree 中分别从零构建了 PR head(72274785a)和 merge-base(20e4351d2)(npm ci && npm run bundle),用 @lydell/node-pty + @xterm/headless 在 100×34 PTY 中驱动真实构建的 CLI,mock OpenAI SSE 端点返回携带 60 步计划的 exit_plan_mode 工具调用(--approval-mode plan,隔离 HOME)。环境:macOS 15.7.7,Node v22.23.1。
结论:功能在真实终端中完全按设计工作(11/11 项行为检查通过),但合并前需解决两件事:(1) ubuntu CI 红是本 PR 引入的——已定位根因并验证一行修复;(2) 截断提示丢失了 PR 描述中仍在宣传的隐藏行数("55")。另外:本 PR 与同修 #7001 的开放 PR #7060 存在合并冲突,需要维护者裁决落地顺序。
1) E2E 行为矩阵(PR 构建)
9 项检查全部通过:折叠态提示语 → e 展开(滚动回看可见完整 60 步)→ 展开态内联 Press 'e' to collapse 提示 → 展开态 ↑/↓ 惰性(选项不动)→ 杂键不误折叠、也不进 composer(二次 e 正常折叠)→ 折叠后提示语与导航恢复 → Esc 干净取消无卡死 → merge-base A/B(旧提示语带行数 "55",e 完全无效,视口逐字节相同)。截图见上方英文部分。
2) 🔴 ubuntu CI 红是本 PR 引入的(一行修复已验证)
失败的是既有测试 handles global render mode shortcut through the captured keypress handler,报 TypeError: Cannot read properties of undefined (reading 'some')。根因:新的 'e' 处理器在每次按键时无条件执行 pendingToolCallsRef.current.some(...)(AppContainer.tsx:3789),而测试套件的默认 useGeminiStream mock(AppContainer.test.tsx:305)缺少 pendingToolCalls 字段——既有 Ctrl+B 代码只在自己的分支内读该 ref,所以之前从未触发。作者新增的测试在自己的 mock 里补了该字段,但默认 mock 没补。
本地精确复现(3 个受影响文件:1 failed | 327 passed);在默认 mock 中加一行 pendingToolCalls: [], 后 AppContainer.test.tsx 全绿(119/119)。生产路径安全(useGeminiStream 恒返回数组),纯测试环境崩溃——但 CI 必须绿。可选加固:(pendingToolCallsRef.current ?? []).some(...)。
反向 A/B(merge-base 组件 + PR 测试):两个 collapse-hint 测试如预期在 base 上红(载荷测试),另两个是惰性守卫、base 上绿属设计使然。
3) 🟡 截断提示丢失了 PR 描述仍在宣传的行数
PR 描述的证据写 ... 63 more lines not shown (press 'e' to expand) ...,但实际实现是静态字符串(ToolConfirmationMessage.tsx:378-382),plan 对话框现在显示 ... more lines not shown (press 'e' to expand) ...——没有行数。merge-base 显示 55 more lines,相对 main 是信息量回退(见前后对比截图)。建议:让 truncateCueText 支持行数(如 (n) => string 或模板),或在默认文案后追加提示而非整体替换。
附带小注:非 plan 场景的默认文案也变了(去掉了 (viewport too small))——目前无害(enforceHeightBudget 仅 plan 分支使用),但建议在 PR 描述中说明。
4) 全景:与开放 PR #7060 冲突(同一 issue,不同方向)
两个 head(7227478 × f8f5c24)git merge-tree 试合并在 ToolConfirmationMessage.tsx 及其测试上报内容冲突。#7060 实现 #7001 方向 1(o 在 $EDITOR 中打开完整计划),本 PR 实现方向 3(e 内联展开/折叠)。两个方向互补、可以共存,但后合入者必须 rebase 并重新验证。需要维护者决定落地顺序(或取舍)。
5) 非阻塞小问题
package-lock.json噪音:43 处新增"peer": true+ 删除@tailwindcss/oxide-wasm32-wasi子依赖条目,但package.json零改动——npm 版本差异导致的重生成噪音,建议还原为 main 的 lockfile。- 名为
RadioButtonSelect isFocus is false when plan is expanded的测试只是重复断言 collapse 提示(与第一个测试相同),并未真正验证焦点行为。我的 PTY 检查 Are you interested in AI Terminal? #4 覆盖了真实行为,但单测层面建议通过 ink stdin 驱动 ↑/↓ 并断言›标记不动。这与 review bot 仍未解决的 threads 一致。
结论
功能在真实构建上端到端验证可用;当前不可合并——需要 (a) 一行默认 mock 修复让 CI 变绿,(b) 建议恢复提示语中的隐藏行数(或修订 PR 描述与实际文案一致),(c) 维护者裁决 #7060 与 #7116 的落地顺序。所有发现均可用最后一张截图中的命令复现。
3 participants
What this PR does
Adds an inline toggle to the
exit_plan_modeconfirmation dialog: pressingeexpands the plan body to full terminal height so the complete plan is readable via terminal scrollback, and pressingeagain collapses back to the viewport-bounded view. The truncation cue now hints the key:... N more lines not shown (press 'e' to expand) ....Why it's needed
PR #6882 made plan truncation visible but the user still must approve a plan they cannot fully read — a model could place unexpected steps past the viewport budget and the approver would accept them blind. This implements a true toggle so the user can scroll, read, then decide.
Reviewer Test Plan
How to verify
exit_plan_mode.... N more lines not shown (press 'e' to expand) ....e— plan body expands to full terminal height;Press 'e' to collapsehint appears inline.e— collapses back; option navigation resumes.cd packages/cli && npx vitest run src/ui/utils/MarkdownDisplay.test.tsx— 182 tests pass.Evidence (Before & After)
Before — truncation cue with no way to read missing lines:
After — cue hints the key,
eexpands inline:Tested on
Risk & Scope
Linked Issues
Fixes #7001.
中文说明
这个 PR 做了什么
为
exit_plan_mode确认对话框添加内联切换功能：按e将计划正文展开到终端全高，用户可通过终端回读完整计划；再按e收起。截断提示改为press 'e' to expand。为什么需要
PR #6882 让截断可见但用户仍需盲批。这实现真正的切换功能，让用户可以阅读后再决定。
审阅者测试计划
如何验证
exit_plan_mode。press 'e' to expand。e展开，鼠标滚轮可查看完整计划，Up/Down 无效。e收起，选项导航恢复。证据（Before & After）
Before：
... 63 more lines not shown (viewport too small) ...After：
... 63 more lines not shown (press 'e' to expand) ...测试平台
风险与范围