feat(web-shell): allow copying terminal turn errors - #10001
Conversation
|
Thanks for the PR! Template — complete ✓ (all sections filled, bilingual body included). Problem — observed and verified, not theoretical. Linked issue #9996 was triaged and accepted for exploration after checking the current code: terminal Direction — aligned. This is parity with the existing assistant-message Copy action on a surface (Web Shell) that is part of this product; the issue was explicitly scoped to Copy-only, with Branch and footer customization kept out. No auth/sandbox/telemetry/release surface is touched. No upstream CHANGELOG reference, but that's expected — this is a Web Shell–specific surface. Size — small and appropriate: ~34 production logic lines in Approach — the scope feels right. Rather than inventing a new clipboard path, it reuses the shared Risk — no elevated risk signals: none of the changed files match the repo's revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献! 模板 —— 完整 ✓(各节齐全,含中文说明)。 问题 —— 真实且已核实,不是理论性问题。关联 issue #9996 在对照当前代码检查后被接受进入探索阶段:终态 方向 —— 对齐。这是在产品自有表面(Web Shell)上与既有 assistant 消息 Copy 操作看齐;issue 已明确将范围限定为只加 Copy,Branch 和 footer 自定义保持在范围之外。不涉及 auth/沙箱/遥测/发布等表面。上游 CHANGELOG 没有直接参考,但这属于 Web Shell 特有表面,属预期情况。 规模 —— 小而恰当: 方案 —— 范围合理。没有新造剪贴板路径,而是复用共享的 风险 —— 无升级风险信号:改动文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI formed an independent picture first — reuse the shared clipboard helper, mirror the assistant footer's copied-state button, gate it on
No correctness, security, or convention issues found. The un-cleared 2s timeout on unmount matches the existing pattern in four other components and is a no-op in React 18+, so it isn't worth changing. Test evidence (PR's own CI, read via API — no PR code executed)The author is a first-time contributor, so the PR's CI workflows have not run yet — all three sit at
The PR body reports 311 focused tests passing and Sandboxed verification would settle what CI alone cannot: 中文说明代码审查先独立形成预期方案——复用共享剪贴板辅助函数、照搬 assistant footer 的 copied 状态按钮、仅在
未发现正确性、安全性或规范问题。卸载时未清理的 2 秒超时与另外四处既有实现一致,在 React 18+ 中为空操作,不值得改动。 测试证据(读取 PR 自身 CI,未执行 PR 代码)作者是首次贡献者,PR 的 CI 工作流尚未运行——三个均处于 PR 正文报告 311 个聚焦测试通过、 沙箱验证可以补上 CI 无法覆盖的部分: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean static review of a minimal, well-scoped change; the one reservation is that the PR's CI has not executed yet, so test evidence is still outstanding. Stepping back: this is a small PR that does one thing and does it the way the codebase already does it. My independent proposal for the problem was exactly this shape — reuse the shared clipboard helper, mirror the assistant footer's copied-state button, scope it to terminal turn errors only — and the diff matches it, which means no simpler path was missed. Every line serves the goal: no drive-by edits, no scope creep past what #9996 accepted (Branch and footer customization correctly left out). The tests assert the contract rather than the implementation, and the visual scenario plugs into the existing harness instead of inventing a new one. The linked issue is self-reported by the same author, which normally warrants extra skepticism — but it was independently triaged and verified against the code before acceptance, and the gap (assistant messages copyable, terminal errors not) is plainly visible in the current source. Six months from now this reads as a five-minute change that closed a real paper-cut; nothing here will curse its maintainer. I am not approving on the strength of the static review alone, and the reason is procedural, not doubt about the code: all three CI workflows sit at 中文说明信心:4/5 —— 静态审查干净,改动最小且范围恰当;唯一的保留意见是 PR 的 CI 尚未执行,测试证据仍待落实。 退一步看:这是一个只做一件事、且完全按照代码库既有方式去做的小 PR。我对该问题独立构想的方案正是这个形态——复用共享剪贴板辅助函数、照搬 assistant footer 的 copied 状态按钮、仅作用于终态轮次错误——diff 与之吻合,说明没有遗漏更简路径。每一行都服务于目标:无顺手改动,没有超出 #9996 接受范围(Branch 与 footer 自定义被正确地排除在外)。测试断言的是契约而非实现细节,视觉场景接入既有测试框架而非另起炉灶。 关联 issue 由同一作者自报,通常应多一分审慎——但它已经过独立 triage 并对照代码核实后接受,而且这个缺口(assistant 消息可复制、终态错误不可)在当前源码中一目了然。六个月后回看,这是一次五分钟就能理解、解决了真实小痛点的改动,不会给维护者添堵。 我不会仅凭静态审查就批准,原因是程序性的,而非对代码有疑虑:三个 CI 工作流均处于 — Qwen Code · qwen3.8-max Reviewed at |
ytahdn
left a comment
There was a problem hiding this comment.
评审结论 / Verdict
LGTM。在 head 59f09429(scratch clone checkout)独立复核了全部 5 个文件的改动:实现小而稳,与现有 assistant 消息复制按钮的模式完全一致,未发现正确性、可访问性或约定问题。仅有一条可选的测试加固建议。
LGTM. Independently re-verified every change across all 5 files at head 59f09429 (blob-less clone checkout). The change is small, steady, and mirrors the existing assistant-message copy pattern exactly; no correctness, accessibility, or convention issues found. One optional test-hardening suggestion below.
📌 CI 备注 / CI note:截至评审时,该 head 上只有 review-pr / precheck / label 等流水线在跑,实质性测试 lanes 尚未触发(首次贡献者的 workflows 需维护者批准)。静态审查未发现测试会暴露的问题,但建议合并前确认 CI 全绿。As of review time only the review-pr / precheck / label pipelines have run at this head; the substantive test lanes have not been triggered yet (first-time contributor workflows need maintainer approval). Nothing in the static review suggests tests will fail, but please confirm green CI before merging.
独立复核点 / Verified at head
- Hook 顺序安全 / Hook ordering is safe:
useState/useCallback位于 SystemMessage.tsx:69-77,早于第一个 early return(line 78 的mid_turn_message_injected分支),不存在条件 hook 问题。They sit before the first early return (themid_turn_message_injectedbranch at line 78), so there is no conditional-hook hazard. - 门控准确 / Gating is precise:渲染条件为
source === 'turn_error' && variant === 'error'(SystemMessage.tsx:285)。所有parse*短路分支(contextUsage / stats / status / mcp / tasks / goal)都以variant === 'info'为前提,不会拦截 error 变体,因此复制按钮对全部终态 turn error 可达。Everyparse*short-circuit branch requiresvariant === 'info', so none of them can shadow the error variant — the Copy button is reachable for all terminal turn errors. - 与重试入口正交 / Orthogonal to retry:MessageItem.tsx:153 用
showRetryHint && message.retryable === true控制重试提示;复制按钮不依赖retryable,不可重试的终态错误同样可以复制(单测 2 覆盖此场景)。The retry hint is gated onmessage.retryable === true(MessageItem.tsx:153), but Copy does not depend onretryable, so non-retryable terminal errors remain copyable (unit test 2 covers this). - 复制内容 = 展示内容 / Copied text equals displayed text:error 变体渲染为
<pre>{content}</pre>(SystemMessage.tsx:240),handleCopy复制的正是content。对loop_detected这类经getErrorDisplayText做本地化替换的文案(transcriptToMessages.ts),复制的也是替换后的展示文本,保持一致。Errors render as<pre>{content}</pre>andhandleCopycopies exactlycontent; even forloop_detected, wheregetErrorDisplayTextswaps in a localized label, the copied string matches what is displayed. - i18n 键存在 / i18n key exists:
common.copy→ 'Copy'(i18n.tsx:392)/ '复制'(i18n.tsx:3459),双语俱全。 - 视觉测试锚点真实 / Visual-test anchors are real:
data-web-shell-message-row同时存在于 MessageList.tsx:5319(虚拟滚动)与 :5341(非虚拟)两条渲染路径;turn_error事件形状与 SDK 归一化器一致(sdk-typescript/src/daemon/ui/normalizer.ts:212-230 读取data.message/promptId并置source: 'turn_error')。The locator exists on both the virtualized and non-virtualized row paths, and the scenario'sturn_errorevent shape matches the SDK normalizer. - 键盘可达性无回归 / No keyboard-a11y regression:
.actions默认opacity: 0但按钮仍在可聚焦树中;Tab 聚焦触发.message:focus-within使其显现,:focus-visible用背景色替代被移除的 outline。The button stays focusable while hidden; tabbing in triggers:focus-withinto reveal it, and:focus-visiblereplaces the removed outline with a background. - 未清理的 2s timer 与既有实现同构 / The un-cleared 2s timer matches existing code:AssistantMessage.tsx:64-68 等组件采用完全相同的写法(React 18+ 对卸载后 setState 为无警告 no-op),保持现状合理。Identical to AssistantMessage and siblings; fine to keep.
Findings
💡 [可选 / Optional] 视觉测试的可见性断言对 hover 显现没有约束力 — screenshots.spec.ts 里 hover 后断言 getByRole('button', { name: 'Copy' }) 的 toBeVisible(),但 Playwright 的 toBeVisible 只排除 display: none / visibility: hidden / 空 bounding box,opacity: 0 仍算 visible。也就是说即使 .message:hover .actions { opacity: 1 } 规则被破坏,该断言依旧通过,真正兜底的只有截图像素对比。若想显式钉住 hover 显现行为,可在截图前改用 await expect(copyButton).toHaveCSS('opacity', '1')。非阻塞。
The visual spec asserts toBeVisible() on the Copy button after hovering the error row, but Playwright treats opacity: 0 elements as visible — so the assertion would still pass even if the hover-reveal CSS broke, leaving only the pixel diff as a guard. If you want the hover-reveal behavior pinned explicitly, assert toHaveCSS('opacity', '1') before capturing. Non-blocking.
亮点 / Highlights
🎉 完整复用 writeClipboardText / warnClipboardWriteFailure 与 AssistantMessage 的 copied-state 模式,没有引入任何新机制。Clean reuse of the existing clipboard helpers and copied-state pattern — no new machinery.
🎉 三条单测分别钉住:复制内容与展示一致且不触发重试、不可重试终态错误仍有复制入口、普通系统错误无复制按钮——门控契约覆盖得很干净。The three unit tests pin the contract precisely: copy matches display and does not trigger retry; non-retryable terminal errors still get Copy; ordinary system errors get none.
🎉 README「消息操作」一节同步更新了行为边界(错误轮次不支持分支),文档与实现一致。The README section keeps docs aligned with the implemented boundary (no branching for error turns).
ytahdn
left a comment
There was a problem hiding this comment.
LGTM — 已在 head 59f09429 独立复核:实现正确、范围克制,与现有 assistant 消息复制模式完全一致;hook 顺序、门控条件、复制内容与展示一致性、i18n 键、键盘可达性均验证通过。先前评审中的 💡(视觉测试断言可改用 toHaveCSS('opacity', '1'))为非阻塞建议。
LGTM — independently re-verified at head 59f09429: the implementation is correct, well-scoped, and consistent with the existing assistant-message copy pattern; hook ordering, gating, copied-vs-displayed text equality, i18n keys, and keyboard accessibility all check out. The earlier 💡 (visual-test assertion could use toHaveCSS('opacity', '1')) is non-blocking.
Note: as of this approval the substantive CI test lanes have not run at this head yet (first-time contributor workflows pending approval) — please confirm green CI before merging. 备注:截至本 approve,实质性 CI 测试 lanes 尚未在该 head 上运行(首次贡献者 workflow 待批准),合并前请确认 CI 全绿。
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — the Web Shell visual Playwright suite (screenshots.spec.ts is changed by this PR) was not executed in this review: it is a separate dev-server/browser pipeline outside the vitest run, and its CI lane has not been triggered at this head (only 2 checks exist here, both passing).
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/messages/SystemMessage.module.css:183 — [review] Fifth hand-rolled copy-with-feedback implementation; consolidation candidate
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — the Web Shell visual Playwright suite (screenshots.spec.ts is changed by this PR) was not executed in this review: it is a separate dev-server/browser pipeline outside the vitest run, and its CI lane has not been triggered at this head (only 2 checks exist here, both passing)。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — the Web Shell visual Playwright suite (screenshots.spec.ts is changed by this PR) was not executed in this review: it is a separate dev-server/browser pipeline outside the vitest run, and its CI lane has not been triggered at this head (only 2 routing checks exist, both passing).
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/messages/SystemMessage.tsx:115 — [review] Fifth hand-rolled copy-with-feedback implementation; consolidation candidate (re-disclosed from round 2; recorded, not requested)
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — the Web Shell visual Playwright suite (screenshots.spec.ts is changed by this PR) was not executed in this review: it is a separate dev-server/browser pipeline outside the vitest run, and its CI lane has not been triggered at this head (only 2 routing checks exist, both passing)。
收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
chiga0
left a comment
There was a problem hiding this comment.
Round 1 · APPROVE
Scope: 5-file diff read. Not run locally (no toolchain); CI build/test checks have not yet completed — disclosed below.
Checked:
- Copy target matches displayed text:
contentis rendered verbatim in<pre>{displayContent}</pre>forvariant === 'error'.displayContent = visionBridgeContent ?? content. ThevisionBridgeContenttransform only applies whensource === 'vision_bridge_notice'; the guard issource === 'turn_error', sodisplayContent === contentand the copied text exactly matches what's visible. ✓ - Guard conditions:
source === 'turn_error' && variant === 'error'— matches only terminal model-stream failures, not ordinary system errors; consistent with both the PR description and the test named "does not add Copy to ordinary system errors". ✓ - Clipboard fallback chain:
writeClipboardTextis the existing utility already used by other copy entry points. The.catch(warnClipboardWriteFailure)path leavescopied === false, so the check icon is not displayed on failure. Tests confirm both paths. ✓ - Touch device discoverability:
@media (hover: none) { .actions { opacity: 1; } }is present in the diff — the button is always visible on touch. The E2E test verifies this explicitly withwindow.matchMedia('(hover: none)').matches === trueandtoHaveCSS('opacity', '1'). ✓ (see cross-check below) handleCopydependency:[content]— correct;writeClipboardText,warnClipboardWriteFailure, andsetCopiedare all stable.- Test efficacy: 4 focused tests. Deletions would flip: removing the
source === 'turn_error'guard causes test 4 (no source) to find a button; removing thevariant === 'error'guard doesn't change pass/fail on these 4 tests (all 4 usevariant="error"or no variant), but the PR description's scope claim would be violated — acceptable giventurn_errorevents are alwayserrorvariant in practice. - Retry isolation: Copy is in a separate
div.actionsblock after the existing retry hint;onClick={handleCopy}does not callonRetryClick. Test confirmsonRetryClicknot triggered. ✓
Unreviewed: CSS transitions and visual layout; README wording; screenshots.spec.ts scenario structure.
R1-1 [minor] · packages/web-shell/client/components/messages/SystemMessage.tsx line 120
window.setTimeout(() => setCopied(false), 2000);
Each click fires a new 2 s timeout without cancelling the previous one. If the user clicks twice within 2 s, the check icon disappears at first-click + 2 s rather than last-click + 2 s. The indicator may vanish sooner than expected. Storing the timeout ID in a useRef and calling clearTimeout in the then callback before scheduling the new one would fix this. Not a blocker — the button still copies correctly.
Cross-check — existing review bot R1-1 [Critical] — REFUTED
The existing automated review filed a Critical finding that "on touch devices the new Copy action is effectively undiscoverable" because .actions starts at opacity: 0 and is only revealed on hover or focus-within.
This is incorrect. The diff adds @media (hover: none) { .actions { opacity: 1; } } at CSS line 177, which forces the button to always-visible on pointer-less (touch) devices. The E2E test creates a Pixel 7 device context, asserts matchMedia('(hover: none)').matches === true, and confirms toHaveCSS('opacity', '1') before any interaction. The Critical finding does not hold against the current head.
CI disclosure: build/test/lint checks have not yet completed as of review time. The PR author reports that server.test.ts, AuthDialog.test.tsx, and session-swap-telemetry.test.ts fail on current main with no source overlap with this PR — flagging for confirmation once CI finishes.
Reviewed with AI assistance.
ytahdn
left a comment
There was a problem hiding this comment.
增量复评(59f09429 → f90dcc7)/ Incremental re-review
针对 740a0204(触屏可见性修复)与 29d14a77(可见性契约测试)两个新提交,在 head f90dcc74 上逐条复核了第一轮 findings 的修复情况。结论:全部修复属实,未发现新问题。 我上一轮的 💡(toBeVisible 不感知 opacity)也在本轮一并得到解决。
Incremental re-review of the two new commits (740a0204 touch fix, 29d14a77 visibility-contract tests), cross-verified at head f90dcc74. Verdict: all round-1 findings are genuinely addressed; no new issues found. My earlier 💡 about toBeVisible ignoring opacity is resolved as well.
逐条核验 / Finding-by-finding verification
- R1-1(触屏不可发现 / touch discoverability)— 已修复且验证属实:
@media (hover: none) { .actions { opacity: 1 } }与 AssistantMessage.module.css:395-398 的既有.messageFooter触屏先例逐字同形;视觉测试开真实 Pixel 7 context,先在页面内断言matchMedia('(hover: none)').matches === true再断言无交互即opacity: 1——媒体查询前提与效果都被钉住。The fallback is byte-for-byte the same shape as the assistant-footer precedent, and the visual test opens a real Pixel 7 context, verifies the media query matches in-page, then asserts opacity 1 with no interaction. - R1-3(结果分支无测试 / untested outcome branches)— 已修复:成功路径用 fake timers 钉住 Check 图标在点击后立即出现、1999ms 仍在、2000ms 还原为 Copy;失败路径同时让
navigator.clipboard.writeTextreject 与document.execCommand返回 false。对照 utils/clipboard 核实:writeText 拒绝 → execCommand 回退失败 → reject →warnClipboardWriteFailure恰好一次 console.warn,与断言toHaveBeenCalledOnce()及图标保持 Copy 完全吻合。Checked against the clipboard helper: reject → failed execCommand fallback → rejection → exactly one warn. Assertions match the helper's real semantics. - R1-4 / R1-5(
toBeVisible忽略 opacity)与 R2-2(隐藏态未钉住)— 已修复:桌面契约现在端到端钉死——初始opacity: 0→ keyboard focus 显现1(:focus-within)→ blur 还原0→ hover 显现1;720px 路径先mouse.move(0, 0)断言0再 hover 回1。隐藏默认与两种显现路径都有守卫,改动 CSS 任一行都会让断言变红。The hidden default and both reveal paths are now pinned end to end. - R2-3(
locator('..')父级耦合)— 已采纳:actions 容器新增data-web-shell-message-actions锚点(与data-web-shell-message-row命名惯例一致),桌面与触屏断言都直接定位该容器,按钮外层未来加包装也不失效。 - R1-2(timer 重叠 / timer overlap)— 作者延后处理,理由成立:复制本身仍然成功,仅瞬态图标时长偏差,且 AssistantMessage.tsx:64-68 等既有实现行为完全相同;跨组件统一修复应走独立后续 PR。Deferral is sound — the existing copy surfaces share the exact same behavior.
- R2-1(clipboard mock 样板重复)— 作者不接受抽取,理由成立:两份测试都完整捕获原 descriptor 并在 finally 恢复,抽取跨文件 helper 会扩大这个原子 UI 修复的范围。Decline is reasonable for an atomic PR.
合并完整性 / Merge integrity
两次 main 合入带入了 PR 10003(vision bridge notices)与 PR 9993(compact view)对相同文件的改动;在最终 head 上核实:useState/useCallback 仍位于第一个 early return 之前(SystemMessage.tsx:115-123 对 :124),copy 块与门控完好,CSS 与 i18n 键未受影响;第二次合入不包含任何 web-shell 改动。The upstream merges are reconciled correctly — hook ordering, the gated copy block, CSS, and i18n keys are all intact at the final head, and the second merge carries no web-shell changes at all.
CI 备注 / CI note
该 head 上的实质性工作流(Qwen Code CI 单测 / Web-shell Visuals)目前为 action_required,等待维护者批准首次贡献者的 workflow 运行;作者自述的定向 Playwright 2/2 通过尚待 CI 独立确认。建议批准后确认绿再合并。The substantive lanes are pending maintainer approval for this first-time contributor's workflow runs; please confirm green after approving.
ytahdn
left a comment
There was a problem hiding this comment.
增量复审通过(head f90dcc74)。上一轮评审意见中要求修复的项已逐条在最新 head 上核实为真实修复,未发现新问题。
已验证的修复:
- R1-1 触摸设备可见性:
740a0204补上了与AssistantMessage一致的@media (hover: none) { .actions { opacity: 1 } },视觉测试新增真实 Pixel 7 上下文,先确认(hover: none)命中,再在无悬停/点击的情况下断言opacity: 1。 - R1-3 复制结果分支测试:成功路径用 fake timers 断言 Check 图标在点击后及 1999ms 仍在、2000ms 恢复 Copy;新增失败路径测试(writeText reject + execCommand 返回 false),断言图标保持 Copy 且仅有一次 warn。
- R1-4 / R1-5 及我第一轮提出的 opacity 断言问题:不再依赖
toBeVisible()(其不检查 opacity),全部改为对 actions 容器的toHaveCSS('opacity', ...)断言。 - R2-2 隐藏默认态钉定:
29d14a77现在完整钉住桌面端揭示契约——初始0→ 键盘聚焦1→ blur 回0→ hover1;720px 路径先移开指针断言0再 hover 回1。突变体(opacity 恒 1 / 删除揭示选择器)将无法通过。 - R2-3 DOM 耦合:actions 容器新增
data-web-shell-message-actions锚点,桌面与触摸断言都直接定位该容器,不再经locator('..')从按钮反推父级。
关于延后项:R1-2(反馈定时器按最新点击重置)与 R2-1(跨文件 clipboard mock 助手抽取)作者的延后理由成立——前者只影响图标持续时长的瞬态细节且 AssistantMessage 现状相同,后者属于与本原子修复无关的测试重构,适合后续统一处理。
两次与 main 的合并均已核对:第一次合并内容正确对账,第二次合并不含任何 web-shell 改动;增量提交仅触及本 PR 范围。
CI 备注:该 head 上的实质性工作流(单元 / 视觉 / 安全)目前处于等待维护者批准首次贡献者 workflow 运行的状态,尚未在此 head 实际执行;本批准基于静态审查结论,建议批准 workflow 后确认绿灯再合并。
Incremental re-approval at head f90dcc74. Every fix requested in the previous review rounds has been verified as genuinely present at the latest head, and no new issues were found.
Verified fixes:
- R1-1 touch visibility:
740a0204adds the same@media (hover: none) { .actions { opacity: 1 } }fallback used byAssistantMessage, and the visual test now opens a real Pixel 7 context, confirms(hover: none)matches, and assertsopacity: 1without any hover or tap. - R1-3 outcome-branch coverage: the success path uses fake timers to assert the Check icon persists immediately after the click and at 1999ms, reverting to Copy at 2000ms; a new failure-path test rejects
writeTextand stubsexecCommandto return false, then asserts the icon stays Copy and the shared warning runs exactly once. - R1-4 / R1-5 and my own round-1 opacity point: the suite no longer relies on
toBeVisible()(which ignores opacity); all reveal checks asserttoHaveCSS('opacity', ...)on the actions container. - R2-2 hidden-default pinning:
29d14a77pins the full desktop reveal contract — initial0→ keyboard focus1→ blur back to0→ hover1; the 720px path moves the pointer away and asserts0before hovering back to1. The opacity-always-1 and reveal-selector-dropped mutants now fail. - R2-3 DOM coupling: the actions container exposes
data-web-shell-message-actions, and both desktop and touch assertions target it directly instead of deriving the parent from the button vialocator('..').
On the deferrals: the author's reasoning for R1-2 (latest-click feedback timer) and R2-1 (shared clipboard-mock helper) is sound — the former only affects transient icon duration and matches existing AssistantMessage behavior, and the latter is cross-file test refactoring outside this atomic fix, better done consistently later.
Both main merges reconcile cleanly: the first merge's contributions are accounted for, and the second brings zero web-shell changes; the incremental commits stay strictly within this PR's scope.
CI caveat: the substantive workflows at this head (unit / visual / security) are currently awaiting maintainer approval of the first-time contributor's workflow runs and have not executed here. This approval is based on static verification; recommend approving the workflows and confirming green before merging.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — the Web Shell visual Playwright suite (screenshots.spec.ts is changed by this PR) was not executed in this review: it is a separate dev-server/browser pipeline outside the vitest run, and its CI lane has not been triggered at this head. The packages/web-shell vitest suites ran green via diagnostics (4239/4239); the web-shell lib tsc failure at client/App.tsx(8147) reproduces byte-identically at the merge base (pre-existing, A/B-measured via base-tree).
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — the Web Shell visual Playwright suite (screenshots.spec.ts is changed by this PR) was not executed in this review: it is a separate dev-server/browser pipeline outside the vitest run, and its CI lane has not been triggered at this head. The packages/web-shell vitest suites ran green via diagnostics (4239/4239); the web-shell lib tsc failure at client/App.tsx(8147) reproduces byte-identically at the merge base (pre-existing, A/B-measured via base-tree)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@chiga0 @ytahdn Maintainer action is needed to unblock this PR at current head 4881f59:\n\n- Please approve the first-time-contributor workflow runs for Qwen Code CI, Web-shell Visuals, and Security Checks.\n- Please dismiss the stale CHANGES_REQUESTED review from the bot at 59f0942 if appropriate. All 8 review threads are resolved, and the current head has approvals from both of you.\n\nThe branch has been synced with main and is conflict-free. The sponsored sandbox verification was skipped because its model risk screen failed or returned an unparseable result; no PR code ran and this is not a source-code test failure.\n\nGitHub reports the PR as mergeable; these maintainer-only gates are the remaining blockers. Thank you. |
|
@qwen-code /triage |
|
Sandboxed verification: Skipped because the pre-execution risk screen refused this sponsored run: the model risk screen failed or returned an unparseable answer. A maintainer who has reviewed the diff can run the verification manually in a disposable environment. 中文 — 判定:
|
|
Released in v0.23.0. |
What this PR does
Adds a Copy action to terminal Web Shell turn errors. The action uses the existing clipboard behavior, briefly confirms success with a check icon, and copies the exact rendered error text. Retry remains a separate action, non-retryable terminal errors are also copyable, and ordinary system errors do not gain the action.
The visual coverage exercises dark and light themes at desktop and 720 px widths, and the Web Shell documentation now describes the supported message actions.
Why it's needed
Completed assistant messages can already be copied, but terminal errors are rendered as system messages and previously offered no equivalent action. Users therefore had to manually select diagnostics before reporting or searching for a failure. Issue #9996 was accepted for a scoped Web Shell change with Copy only; Branch and assistant footer customization remain intentionally out of scope.
Reviewer Test Plan
How to verify
Open a Web Shell session containing a terminal turn error, hover or keyboard-focus the error row, click Copy, and paste the clipboard contents. The pasted value should exactly match the displayed error text. A retryable error should keep its existing Retry affordance alongside Copy; a non-retryable terminal error should still expose Copy; an ordinary system error should expose neither the new Copy action nor Branch.
Local validation on Node.js 22: 311 focused transcript, message-list, and system-message tests passed; Web Shell lint, typecheck, changed-file formatting, and diff checks passed; the visual scenario passed in dark/light themes at desktop and 720 px widths.
npm run preflightpassed clean/install, formatting, lint, build, and typecheck, then exited during the full test phase on current-main failures in three unrelated CLI test files (server.test.ts,AuthDialog.test.tsx, andsession-swap-telemetry.test.ts); those failures reproduce when the CLI files are run separately and have no source overlap with this PR.Evidence (Before & After)
Before: terminal errors only showed the rendered diagnostic and, for retryable kinds, Retry; there was no Copy action.
After: terminal errors show a footer Copy action on hover or focus without changing Retry behavior. The added visual workflow scenario captures dark/light desktop and narrow states; the Web Shell visual CI job will publish its screenshots on this PR.
Tested on
Environment (optional)
Node.js 22, Vitest, and Playwright Chromium against the local Web Shell visual harness.
Risk & Scope
Linked Issues
Fixes #9996
中文说明
本 PR 做了什么
为 Web Shell 的终态轮次错误增加 Copy 操作。该操作复用现有剪贴板行为,复制成功后短暂显示勾选图标,并复制用户实际看到的错误文本。Retry 仍是独立操作,不可重试的终态错误同样可以复制,普通系统错误不会获得该操作。
视觉覆盖验证深色和浅色主题下的桌面宽度与 720 px 窄屏,Web Shell 文档也补充了支持的消息操作说明。
为什么需要
已完成的 assistant 消息已经可以复制,但终态错误以系统消息渲染,之前没有等价操作。用户在报障或检索错误前只能手动选择诊断文本。Issue #9996 已接受这个范围克制的 Web Shell 改动,并明确只增加 Copy;Branch 和 assistant footer 自定义仍有意保持在范围之外。
Reviewer 测试计划
如何验证
打开一个包含终态轮次错误的 Web Shell 会话,悬停或使用键盘聚焦错误行,点击 Copy,再粘贴剪贴板内容。粘贴值应与显示的错误文本完全一致。可重试错误应在 Copy 之外保留现有 Retry;不可重试的终态错误仍应显示 Copy;普通系统错误不应显示新的 Copy,也不应显示 Branch。
Node.js 22 本地验证:311 个聚焦的 transcript、消息列表和系统消息测试通过;Web Shell lint、typecheck、改动文件格式检查和 diff 检查通过;视觉场景在深色/浅色主题的桌面宽度及 720 px 宽度下通过。
npm run preflight的 clean/install、格式化、lint、build 和 typecheck 阶段均通过,随后在全量测试阶段因当前 main 的三个无关 CLI 测试文件(server.test.ts、AuthDialog.test.tsx和session-swap-telemetry.test.ts)失败而退出;单独运行这些 CLI 文件可复现,且它们与本 PR 没有源码交集。证据(改动前与改动后)
改动前:终态错误只显示诊断文本;对于可重试类型还显示 Retry,但没有 Copy 操作。
改动后:终态错误在悬停或聚焦时显示 footer Copy 操作,Retry 行为保持不变。新增的视觉工作流场景覆盖深色/浅色桌面和窄屏状态;Web Shell 视觉 CI 会在本 PR 上发布截图。
已测试平台
环境(可选)
Node.js 22、Vitest,以及基于本地 Web Shell 视觉测试环境的 Playwright Chromium。
风险与范围
关联 Issue
Fixes #9996