feat(ui): let the user read the full plan from the exit_plan_mode confirmation - #7060
Conversation
…firmation A long plan can exceed the confirmation dialog's height budget; since QwenLM#6882 the truncation is at least visible ('... N more lines not shown ...'), but the user still had to approve a plan they could not fully read — steps past the viewport budget were approved blind. Pressing 'o' now writes the complete plan markdown to a temp file and opens it in the configured editor (the same useLaunchEditor flow the skill-review dialog uses). Viewing the plan neither confirms nor cancels — the dialog stays open. A one-line hint under the plan body advertises the key; the body's height budget shrinks by one row to make room, and editor-launch failures render in the hint's place. Fixes QwenLM#7001
Local verification reportWhat was implementedDirection 1 from the issue (external viewer — explicitly recommended there as the simplest approach), built on existing infrastructure: pressing Details:
Verification 1 — real end-to-end session (screenshots in PR description)Real interactive session, real model (
For contrast, the same flow on released v0.19.10 (screenshot 1) clips the plan with no way to read the missing steps before approving. Verification 2 — unit tests4 new tests: the hint renders on plan confirmations; Verification 3 — static checks
Notes for reviewers
🇨🇳 中文版报告（点击展开）本地验证报告实现内容采用 issue 中明确推荐的方案 1（外部查看器，三个方案中最简单），并完全复用现有基建：在 plan 确认框中按 要点：
验证 1 —— 真实端到端会话（截图见 PR 描述）真实交互会话、真实模型（
对照组：同样流程在已发布的 v0.19.10 上（截图 1），长计划被裁切且批准前没有任何方式阅读缺失步骤。 验证 2 —— 单元测试25 个测试全部通过，其中 4 个为新增：plan 确认框渲染提示行； 验证 3 —— 静态检查
审阅者须知
|
|
Thanks for the PR! Template looks good ✓ — "What this PR does", "Why it's needed", "Reviewer Test Plan" with before/after screenshots all present. Problem: real and well-documented. Issue #7001 identifies a genuine UX gap — the user is asked to approve a plan they cannot fully read when it exceeds the viewport. Before/after screenshots in the PR body make the problem concrete. Direction: aligned. This implements Direction 1 (external viewer) from the issue — the simplest of the three approaches listed there. CHANGELOG shows prior plan-mode work (plan truncation visibility in #6882), so this is a natural follow-up. Note: #7116 offers a competing inline-toggle approach (press Size: not applicable — only Approach: minimal and focused. Two files — one component, one test. Reuses the existing Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — "What this PR does"、"Why it's needed"、"Reviewer Test Plan" 含 before/after 截图均已提供。 问题：真实且有据可查。Issue #7001 指出一个实际的 UX 缺口——当 plan 超过视口时，用户被要求审批一个无法完整阅读的 plan。PR 中的 before/after 截图让问题一目了然。 方向：对齐。实现了 issue 中列出的方向 1（外部查看器——三种方案中最简单的）。CHANGELOG 中有先前 plan 模式的改进（#6882 的截断可见性），本 PR 是自然延续。注意：#7116 提供了竞争方案（内联展开，按 规模：不适用——仅触及 方案：最小且聚焦。两个文件——一个组件，一个测试。复用已有的 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewIndependent proposal: I would have added a keybinding on the plan confirmation dialog to export the full plan text to a temp file and open it in the user's configured editor. The Comparison: The PR's implementation matches my proposal and adds a thoughtful detail I would have missed — reducing the plan viewport height by one row to make room for the hint, so nothing else shifts. The Findings: No critical issues. The implementation is clean, follows project conventions, and reuses existing infrastructure correctly. The One minor note: the temp directory ( Reuse check: Unit Test ResultsAll 25 tests pass, including the 4 new tests for the full-plan viewer: New test coverage:
Real-Scenario TestingTmux end-to-end testing requires an active model session to trigger the 中文说明代码审查独立方案： 我会在 plan 确认对话框上添加一个快捷键，将完整 plan 导出到临时文件并在用户配置的编辑器中打开。 对比： PR 的实现与我的方案一致，并添加了一个我会遗漏的细节——将 plan 视口高度减少一行以容纳提示，这样其他元素不会移动。 发现： 无关键问题。实现干净，遵循项目约定，正确复用现有基础设施。 一个小注意：临时目录（ 复用检查： 单元测试结果25 个测试全部通过，包括 4 个新增的全 plan 查看器测试：
真实场景测试tmux 端到端测试需要活跃的模型会话来触发带有截断 plan 的 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 5/5 — Clean implementation of a real UX gap. Competing approach (#7116) is closed. Maintainer-approved. All CI green. Ready to ship. Re-run note: previous Stage 3 flagged #7116 (inline toggle) as a competing approach worth maintainer consideration. That PR is now closed, so the only reason this wasn't a 5/5 is gone. Everything else still holds — focused 2-file diff, clean reuse of 中文说明信心度: 5/5 — 干净地实现了一个真实的 UX 缺口修复。竞争方案 (#7116) 已关闭。维护者已批准。所有 CI 通过。可以合并。 Re-run 说明：之前的 Stage 3 将 #7116（内联展开）标记为值得维护者考虑的替代方案。该 PR 现已关闭，因此之前不是 5/5 的唯一原因已消除。其他一切仍然成立——聚焦的 2 文件 diff，干净地复用 — 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. ✅
| const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-plan-')); | ||
| const planPath = path.join(dir, 'plan.md'); | ||
| await fs.writeFile(planPath, confirmationDetails.plan); | ||
| await launchEditor(planPath); |
There was a problem hiding this comment.
[Critical] openFullPlanInEditor creates a temp directory via fs.mkdtemp on every o press but never removes it — not after the editor exits, not on error, not on dialog dismissal. The established codebase pattern in text-buffer.ts (lines 2524, 2559, 2626) wraps identical mkdtemp usage in try/finally with fs.rmSync(tmpDir, { recursive: true, force: true }). — Failure scenario: each o press orphans a qwen-plan-XXXXXX/plan.md directory containing the full plan text (potentially sensitive conversation content) under os.tmpdir(). On shared systems other processes can discover and read these files; on tmpfs-backed /tmp they consume RAM until reboot.
| const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-plan-')); | |
| const planPath = path.join(dir, 'plan.md'); | |
| await fs.writeFile(planPath, confirmationDetails.plan); | |
| await launchEditor(planPath); | |
| const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-plan-')); | |
| try { | |
| const planPath = path.join(dir, 'plan.md'); | |
| await fs.writeFile(planPath, confirmationDetails.plan, { mode: 0o600 }); | |
| await launchEditor(planPath); | |
| } finally { | |
| await fs.rm(dir, { recursive: true, force: true }).catch(() => {}); | |
| } |
— qwen3.7-max via Qwen Code /review
| void openPlan().catch((err: unknown) => { | ||
| setPlanViewError(err instanceof Error ? err.message : String(err)); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] The .catch handler that sets planViewError when the editor fails to launch has no test coverage — no test uses launchEditorMock.mockRejectedValueOnce(...) to exercise the error-display path. — Concrete cost: a regression that silently swallows the error (e.g., someone "simplifies" the catch to a no-op) would ship undetected, leaving the user staring at a frozen dialog with no feedback on why the editor never opened.
| void openPlan().catch((err: unknown) => { | |
| setPlanViewError(err instanceof Error ? err.message : String(err)); | |
| }); | |
| void openPlan().catch((err: unknown) => { | |
| setPlanViewError(err instanceof Error ? err.message : String(err)); | |
| }); | |
| // Add test: launchEditorMock.mockRejectedValueOnce(new Error('No editor found')) | |
| // Assert: lastFrame() contains 'No editor found' |
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output. Not reviewed: chunk 1 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
| </Text> | ||
| ) : ( | ||
| <Text color={theme.text.secondary} wrap="truncate"> | ||
| {t('o open full plan in editor')} |
There was a problem hiding this comment.
[Suggestion] The i18n key 'o open full plan in editor' is not registered in any translation file under packages/cli/src/i18n/. The t() function falls back to the key string, so English works, but non-English locales will show untranslated English while all surrounding UI is translated. — Concrete cost: breaks the established convention where all other t() keys in this component (e.g., 'Apply this change?', 'Yes, allow once') are registered in all 9 locale files.
| {t('o open full plan in editor')} | |
| {t('o open full plan in editor')} |
Register the key in en.js and other locale files under packages/cli/src/i18n/.
— qwen3.7-max via Qwen Code /review
|
Is there any way to keep the md preview while opening the plan? |
|
I also submitted #7116 with an inline toggle approach — press |
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/29626839805)._ |
Independent local verification (maintainer-side) — LGTMVerified at the pushed head 1. Unit tests + load-bearing A/B
2. Real end-to-end (built CLI, PTY, real vim)Setup: real PR build — all 12 checks green:
Merge-base build (control) — rebuilt from
Screenshots (real captures from the PTY sessions above)Before — merge-base build: plan truncated, no hint,
After — PR build: same session shape, hint line present:
Pressing
After
3. Code-review notes (non-blocking)
4. Landscape note for the merging maintainerOpen PR #7116 implements Direction 3 from #7001 ( Verdict: ready to merge from my side. 🇨🇳 中文版报告（点击展开）独立本地验证（维护者侧）— LGTM在隔离 worktree 中于推送头 1. 单元测试 + 承重 A/B
2. 真实端到端（构建产物、PTY、真实 vim）环境：真实 PR 构建 —— 12 项检查全绿： 截断提示 merge-base 构建（对照组） —— 从 （截图见上方英文部分，均为上述 PTY 会话的真实捕获。） 3. 代码审查备注（不阻塞）
4. 给合并维护者的 landscape 提示开放 PR #7116 实现了 #7001 的方向 3（ 结论：本侧认为可以合并。 |
|
@qwen-code /triage |
|
Triage re-run completed — approved ✅ The only material change since the last run: competing PR #7116 is now closed. Confidence updated from 4/5 → 5/5. All stage comments reflect the current state. Stage 1, Stage 2, and Stage 3 comments above were updated in place. Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] ToolConfirmationMessage.tsx:122-125 — temp directory created by fs.mkdtemp is never removed after the editor closes (existing blocker from @qwen-code-ci-bot, re-checked: still stands at this commit)
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
Dismissing per maintainer decision. The sole finding (temp dir from fs.mkdtemp not cleaned up) matches existing editor-launch paths in the codebase and was rated non-blocking by the project's own triage (stage 2); OS tmp reaping handles cleanup. Maintainer @wenshao has independently verified the feature end-to-end and approved.
|
@qwen-code /triage |
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. ✅
5 participants
What this PR does
Adds a way to read the full plan from the
exit_plan_modeconfirmation dialog: pressingowrites the complete plan markdown to a temp file and opens it in the configured editor (sameuseLaunchEditorflow the skill-review dialog uses). The dialog stays open — viewing the plan does not confirm or cancel anything — and a one-line hint (o open full plan in editor) is always visible under the plan body, replaced by the error message if the editor fails to launch. The plan body's height budget is reduced by one row to make room for the hint, so nothing else shifts.Why it's needed
PR #6882 made plan truncation visible (
... N more lines not shown (viewport too small) ...), but the user still has to 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 Direction 1 (external viewer — the simplest of the three approaches listed in the issue) so the approval can be informed.Fixes #7001
Reviewer Test Plan
How to verify
/plan <something that produces a 50+ line plan>in a normal-height terminal.exit_plan_modeconfirmation appears with the truncation cue, note the new hint line under the body:o open full plan in editor.o— the full plan opens in the preferred editor (falls back to platform default). Close the editor: the dialog is still open, nothing was confirmed.cd packages/cli && npx vitest run src/ui/components/messages/ToolConfirmationMessage.test.tsx— 25 tests pass, including 4 new ones: hint renders;ostages the FULL plan text and launches the editor without resolving the confirmation;Ctrl+Ois inert;ois inert on non-plan confirmations.Evidence (Before & After)
Real end-to-end sessions (real CLI, real model producing a 55-step plan via
/plan, 110×34 tmux pane, macOS).Before — released v0.19.10: the long plan is clipped and there is no way to read the missing steps before approving:
After — this branch: truncation cue plus the new
o open full plan in editorhint:After — pressing
o: the complete plan opens in the configured editor (vim), all 55 steps present; closing it returns to the still-open dialog:Tested on