fix(web-shell): suppress abort toasts on preflight rejection - #11676
Conversation
Route the two preflight-failure toast sites in App.tsx through
reportError instead of hand-rolling pushToast('error', formatError(...)).
reportError already suppresses AbortError, daemon-turn errors and
already-dispatched notices, so a daemon teardown that rejects a
submission with an AbortError now stays silent instead of surfacing a
"failed" toast for a submission the user cancelled.
reportError is hoisted above sendPrompt/enqueuePrompt (it previously sat
below both, which would be a TDZ error for their dep arrays), and the
now-redundant console.warn in each preflight catch is dropped to avoid
double-logging a real failure (reportError already logs via
console.error).
Also documents the prepareSubmit / onSubmitBefore contract: a
rejection's Error.message is shown to the user, so hosts must localize
it.
Fixes #11432
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-issue-patrol/jmtx7igejyc
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ — every required section is present, including the Chinese translation and a test plan that names the actual command and its red/green states. Problem: observed, not theoretical. #11432 is open, and it was itself deferred out of #11171's review thread with exact line references and the two witnesses named up front. This is the second of the two halves that issue was split into; the first (localizing the rewind preflight errors in the VS Code companion) merged as #11628 on 2026-09-11. So Direction: aligned. The complaint in #11432 is that these two catch blocks hand-roll Size: not applicable — Approach: scope feels right, and I could not find a smaller version of it. Two things worth stating precisely, neither blocking:
Risk: no elevated risk signals — neither changed file matches the high-revert-correlation paths. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ —— 所有必需章节齐全,包含中文翻译,以及一份写明了实际命令和红/绿状态的测试方案。 问题: 已观测,非理论性。#11432 处于 open 状态,它本身就是从 #11171 的 review 线程中延后出来的,带有精确的行号引用,并预先点名了两个 witness 测试。本 PR 是该 issue 拆成的两半中的后半;前半(在 VS Code companion 中本地化 rewind preflight 错误)已于 2026-09-11 以 #11628 合并。所以 方向: 对齐。#11432 指出的问题是:这两处 catch 手写了 规模: 不适用 —— 方案: 范围合理,我没有找到更小的实现版本。有两点值得说得更精确,均非阻塞项:
风险: 无升级风险信号 —— 两个改动文件均未命中高回滚相关性路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewNo blockers. The diff does one thing and does it completely — I went looking for the three ways this kind of change usually breaks, and none of them are present. The dependency-array swap is complete, not just mechanical. Replacing The hoist is safe. Both preflight callbacks are genuinely covered. The suppression widening does not regress #9911. Routing these sites through The witnesses are load-bearing. They are structural mirrors of the two passing #9911 tests directly above them in the same One nit, restated from Stage 1 because it is the only substantive trade-off: with the TestingThis run carries CI check results read from the GitHub API — no PR code was built, run, or checked out. Real-scenario tmux capture is N/A here: this is a Web Shell (browser-embedded) surface, not the TUI, and the visual side is covered by the Nothing is red. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Not verified, and why: the red half of the test plan — that both witnesses fail on Sandboxed verification would settle exactly that gap: 中文说明代码审查无阻塞项。这个 diff 只做一件事,而且做得完整 —— 我专门去找这类改动通常会出问题的三个地方,结果一个都没有出现。 依赖数组的替换是完整的,不只是机械替换。 在两个 上提是安全的。 两个 preflight 回调确实都被覆盖。 抑制范围的扩大没有让 #9911 回退。 把这两个站点改走 witness 测试是有效的(load-bearing)。 它们是同一个 一个小点(Stage 1 已提,这里重复是因为它是唯一实质性的取舍): 测试本次运行携带的是通过 GitHub API 读取的 CI 检查结果 —— 没有构建、运行或 checkout 任何 PR 代码。 真实场景的 tmux 抓取在此为 N/A:这是 Web Shell(浏览器内嵌)界面,不是 TUI,视觉部分由 目前没有红项。 (CI 结果表见上方英文部分,未在此重复。) 未验证的部分及原因:测试方案中的红态那一半 —— 即两个 witness 在 沙箱化验证恰好能补上这个缺口: — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — clean across every stage; the only two nits are a slightly overstated sentence in the description and one lost log line, neither in the code path that matters. Before reading the diff I wrote down what I would have done with this: reuse the file's existing error-to-toast helper at both preflight sites rather than teaching each catch what an abort looks like, and accept the hoist that reuse forces. The diff is that, so there is no simpler alternative I am sitting on. That left one real question — whether the hoist and the dependency-array swaps were done correctly — and they were, in the specific sense that I could name every symbol What I would have bet on breaking did not. Swapping a dependency in two The judgement call worth naming is the suppression widening. Routing these two sites through the shared helper means daemon-turn errors and already-dispatched notices are silenced here as well, which is more than "suppress abort toasts" promises. I think it is right: all three suppressions are brand or name checks, the localized error that #11628 re-throws carries none of those brands, and in every silenced case the user already has another signal or there is nothing to report. That is also what keeps #9911 intact — a real host failure still says something. If a maintainer disagrees, the narrow alternative is an explicit abort check at each site, at the cost of the duplication this PR exists to remove. The witnesses are the strongest part. They mirror the two passing #9911 tests above them in everything but the abort brand and the negated assertion, and those mirrors are what stop them passing for the wrong reason — they prove the session-current guards are true in that harness and that Six months from now this reads as one helper doing its job at two more call sites, which is the version of this file I would want. Approving, with the note that the lost CI is still running on this commit — 2 中文说明Confidence: 4/5 —— 各阶段都干净;仅有的两个小点是描述里一句略有夸大的表述,以及少了一行日志,两者都不在关键代码路径上。 在读 diff 之前,我先写下了自己会怎么做:在两个 preflight 站点复用文件里已有的 error-to-toast helper,而不是教每个 catch 认识什么是 abort,并接受这种复用所必须付出的上提。diff 就是这个方案,所以我手上并没有一个更简的替代方案被压着不说。这样就只剩下一个真正的问题 —— 上提和依赖数组的替换是否做得正确 —— 答案是正确的,具体体现在:我能点名 我原本以为最可能出问题的地方没有出问题。在两个 值得点明的判断点是抑制范围的扩大。把这两个站点改走共用 helper,意味着这里连 daemon-turn 错误和已派发通知也一起静默了,这超出了「suppress abort toasts」所承诺的范围。我认为这是对的:三个抑制判断都基于 brand 或 name,#11628 重新抛出的那个已本地化错误不带任何这些 brand,而且在每一种被静默的情况下,用户要么已经有别的提示,要么本来就没有可报告的内容。这也正是 #9911 得以保持完整的原因 —— 真实的宿主失败仍然会给出提示。如果维护者不同意,更窄的替代方案是在每个站点各写一个显式的 abort 判断,代价是重新引入本 PR 意在消除的重复。 witness 测试是这个 PR 最有力的部分。除了 abort brand 和取反的断言之外,它们与紧邻其上、已经通过的两个 #9911 测试完全镜像,而这些镜像正是它们不会「因错误的原因通过」的保障 —— 它们证明了在该测试环境中会话当前性 guard 为真、且 六个月后再看,这就是一处 helper 在另外两个调用点履行职责而已,而这正是我希望这个文件成为的样子。同意合并,并附一点说明:为「用户切换会话之后才到达的 preflight 失败」而丢失的那条 本 commit 的 CI 仍在运行 —— 有 2 个 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-3 preflight rejections lost their unconditional console trace on the guard-false and suppressed branches (packages/web-shell/client/App.tsx:9862, queued twin :10644) — already reported (comment 5638719974, qwen-triage stage=3 by qwen-cod…
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": none — but I did not open #11432 itself (no gh call); my read of the incident's shape is inherited from the counter-frame entry on the findings list, not in….
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Test Plan (not a blocker): client/App.test.tsx — no such file or directory; 2 passed — this review observed 7432, 30786, 569 passed; 911 passed — this review observed 7432, 30786, 569 passed.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 1)":none — but I did not open #11432 itself (no gh call); my read of the incident's shape is inherited from the counter-frame entry on the findings list, not in…。
未审查:反向审计——评审时间预算不足,未能开始第 4 轮。
Test Plan(非阻断):client/App.test.tsx — no such file or directory; 2 passed — this review observed 7432, 30786, 569 passed; 911 passed — this review observed 7432, 30786, 569 passed。
— qwen3.8-max@e6bf8ffe via Qwen Code /review (v0.23.3)
| * If this callback rejects, the submission is cancelled and the rejection's | ||
| * `Error.message` is surfaced to the user, so hosts must localize it. |
There was a problem hiding this comment.
[Suggestion] R1-1: This new contract sentence promises unconditionally that a rejection's Error.message reaches the user, but the same commit routes both preflight catches through reportError, which returns at App.tsx:9723-9729 before formatError, console.error and pushToast for three error classes — AbortError, daemon-turn and already-dispatched — and gates everything else behind admissionOwnerIsCurrent() / submissionSessionIsCurrent(). Before this change an AbortError rejection did toast, so the sentence documents behaviour the same commit removes. @qwen-code/web-shell is published and no .md under packages/web-shell documents either prop, which makes this JSDoc the entire host-facing contract.
It bites a host that signals a real preflight failure by aborting its own work — controller.abort(new Error('Upload timed out')), or rethrowing a DOMException named AbortError out of a deadline-guarded fetch. Trusting this contract, such a host does not surface the reason itself; reportError returns early, so there is no toast, no console.error, and — because this diff also deleted the console.warn in both catches — no console trace either. The submission becomes a silent no-op while the contract asserts the opposite, leaving the user in front of a composer that appeared to do nothing, which is the #9911 symptom the comment three lines below the call site still warns about. The sentence also overstates the non-Error case: formatError falls back to a hardcoded English string, so Promise.reject('rewind failed') surfaces unlocalized copy despite "hosts must localize it".
Witness:
formatError("rewind failed", "Message could not be submitted") = "Message could not be submitted"
formatError(<EmbeddedApp wrapper>, ...) = "Failed to edit the message. Please try again."
(formatError copied verbatim from App.tsx at the reviewed commit and driven outside the worktree. No component-level probe was possible — qwen review scratch-tree reported available: false.)
| * If this callback rejects, the submission is cancelled and the rejection's | |
| * `Error.message` is surfaced to the user, so hosts must localize it. | |
| * If this callback rejects, the submission is cancelled and the rejection's | |
| * `Error.message` is surfaced to the user, so hosts must localize it. | |
| * Rejections that are cancellations rather than failures — an error whose | |
| * `name` is `AbortError`, a daemon-turn error, or one Web Shell has already | |
| * surfaced — are suppressed and logged nowhere, so do not use those shapes | |
| * for a failure the user must see. The message is also dropped once the user | |
| * has left the session this submission belonged to, and a rejection that is | |
| * not an `Error` surfaces Web Shell's own English fallback instead. |
Whatever wording you settle on has to name all three suppressions reportError applies (if (isAbortError(error)) return; if (isDaemonTurnError(error)) { return; } if (isAlreadyDispatched(error)) { return; }, App.tsx:9723-9729) rather than a narrower "aborts only", since both preflight catches now share the full helper, and it must keep promising that a real rejection's localized message reaches the user — App.test.tsx:21698 asserts expect(onToast).toHaveBeenCalledWith('error', 'The original message can no longer be edited.'). The onSubmitBefore twin at :1391-1392 needs the same addition.
中文说明
这句新增的契约描述无条件地承诺「rejection 的 Error.message 会展示给用户」,但同一个 commit 把两处 preflight catch 都改走 reportError;对三类错误(AbortError、daemon-turn、already-dispatched)它会在 App.tsx:9723-9729 提前返回,既不走 formatError,也不走 console.error 和 pushToast,其余情况还要再受 admissionOwnerIsCurrent() / submissionSessionIsCurrent() 的限制。改动之前 AbortError 的 rejection 是会弹 toast 的,所以这句话描述的正是同一个 commit 刚刚移除的行为。@qwen-code/web-shell 是已发布包,而 packages/web-shell 下没有任何 .md 说明这两个 prop,因此这段 JSDoc 就是面向宿主的全部契约。
它会这样咬人:宿主用「中止自己的工作」来表达一次真实的 preflight 失败——例如 controller.abort(new Error('Upload timed out')),或把受 deadline 保护的 fetch 抛出的、name 为 AbortError 的 DOMException 直接重抛。宿主信任这段契约,于是自己没有呈现原因;而 reportError 提前返回,既没有 toast、也没有 console.error,又因为本次 diff 同时删掉了两处 catch 里的 console.warn,连控制台痕迹也不剩。提交变成一次静默的空操作,契约却声称相反,用户停在「看起来什么都没发生」的 composer 前——正是调用点下方三行注释仍在警告的 #9911 症状。这句话对非 Error 的情况同样夸大:formatError 会回落到硬编码的英文串,所以尽管写着「宿主必须本地化」,Promise.reject('rewind failed') 展示的是未本地化的文案。
Witness(证据)
formatError("rewind failed", "Message could not be submitted") = "Message could not be submitted"
formatError(<EmbeddedApp wrapper>, ...) = "Failed to edit the message. Please try again."
(formatError 是从被审 commit 的 App.tsx 原样复制、在 worktree 之外运行的。没有做组件级 probe —— qwen review scratch-tree 返回 available: false。)
建议的修改见上方 suggestion 代码块(severity marker 与 suggestion 块只保留在英文半区,避免重复渲染)。
无论最终采用哪种措辞,都必须点名 reportError 实际施加的全部三类抑制(if (isAbortError(error)) return; if (isDaemonTurnError(error)) { return; } if (isAlreadyDispatched(error)) { return; },App.tsx:9723-9729),而不能窄化成「只抑制 abort」,因为两处 preflight catch 现在共用完整的 helper;同时仍须承诺真实 rejection 的本地化消息会到达用户——App.test.tsx:21698 断言 expect(onToast).toHaveBeenCalledWith('error', 'The original message can no longer be edited.')。:1391-1392 上 onSubmitBefore 的同款描述也需要同样的补充。
— qwen3.8-max@e6bf8ffe via Qwen Code /review (v0.23.3)
Independent verification report — measured at head
|
| file | head blob | bytes | base blob | bytes |
|---|---|---|---|---|
packages/web-shell/client/App.tsx |
d0fa84ea9087851db715ec4e914248d752b5fc50 |
744,432 | 803eca3bcfb6a473f03fe1f0fba07e911a33a89b |
744,010 |
packages/web-shell/client/App.test.tsx |
90916c088239a414762c6c092dd72dbe3a9fed24 |
1,198,375 | — (test-only, +40/−0) | — |
Both head blobs equal pulls/11676/files[].sha exactly.
ARM A — head, both files at head (green).
npx vitest run --config vitest.config.ts App.test.tsx -t "rejects with an AbortError" →
Test Files 1 passed (1) / Tests 2 passed | 909 skipped (911) / exit 0 / 6.86 s.
ARM B — production reverted, tests kept at head (red). App.tsx swapped to the base blob 803eca3bcfb6… (re-verified by git hash-object after the copy) while App.test.tsx stayed at 90916c088239…. Same command →
Test Files 1 failed (1) / Tests 2 failed | 909 skipped (911) / exit 1 / 7.07 s, and both failures are the exact payload the bot said nobody had observed:
FAIL App.test.tsx > App session callbacks > stays silent when preparation rejects with an AbortError
AssertionError: expected "spy" to not be called at all, but actually been called 1 times
1st spy call: Array [ "error", "cancelled" ]
❯ App.test.tsx:21751:25
FAIL App.test.tsx > App session callbacks > stays silent when a queued preparation rejects with an AbortError
AssertionError: expected "spy" to not be called at all, but actually been called 1 times
1st spy call: Array [ "error", "cancelled" ]
❯ App.test.tsx:21772:25
⇒ The two witnesses are load-bearing, not vacuous. They fail on the pre-change production code with precisely onToast('error', 'cancelled') and pass at head, so the delta is attributable to this diff and to nothing else in the tree. Each also asserts its own precondition (expect(prepareSubmit).toHaveBeenCalled()) and that the submission was genuinely cancelled (sendPrompt / rawEnqueuePrompt not called), so neither can pass by never reaching the catch. App.tsx was restored to d0fa84ea9087… afterwards and the restore was hash-verified.
3. The full file at head, and the bot's Test-Plan mismatch reconciled
The bot's review row reports client/App.test.tsx — no such file or directory, and reads the author's 2 passed / 911 passed claims as disagreeing with an observed 7432, 30786, 569 passed. Measured at head, unfiltered:
Test Files 1 passed (1) / Tests 911 passed (911) / exit 0 / 197.99 s.
So both of the author's numbers are correct and are simply about different scopes: 911 passed is the whole file (which collects exactly 911 tests), and 2 passed is the two new witnesses under a name filter. 7432 / 30786 / 569 cannot be this file's totals — it collects 911 — so they read as repo-wide or multi-suite figures from whatever the bot ran instead; that clause is an inference about the bot's invocation, not a measurement. What is measured is only that this file collects 911 tests and passes 911 of them. The no such file or directory is a path artefact of the bot's own invocation — packages/web-shell/vitest.config.ts sets root: 'client', so the path relative to that root is App.test.tsx, not client/App.test.tsx. Non-blocking, but worth stating because it means the bot's "Test Plan mismatch" is not evidence against the author's report; on the contrary, both claimed numbers reproduce exactly.
4. Over-suppression — the #9911 regression the changed comments themselves warn about
Routing both preflight catch sites through reportError widens what gets silenced, so the question is whether a real host failure can now vanish. It cannot: reportError suppresses exactly three classes and each requires a narrow brand that a host's localized rewind Error cannot carry by accident — isAbortError (App.tsx:2350-2355) requires error instanceof DOMException || error instanceof Error and error.name === 'AbortError'; isAlreadyDispatched (:2361, marker test at :2365) requires _alreadyDispatched === true; isDaemonTurnError (packages/sdk-typescript/src/daemon/DaemonClient.ts:583, marker type at :574) requires _daemonTurnError === true.
The file pins both polarities with an adjacent known-positive control: surfaces a queued preparation rejection instead of cancelling silently at App.test.tsx:21709-21733 asserts a real localized rejection still toasts ('error', 'The original message can no longer be edited.'), sitting immediately above the two new silence assertions and differing from them only in the abort brand and the negated assertion. It is among the 911 passing at head. So the silence is scoped to cancellations, not to host failures — which is the whole point of the #11432 fix.
5. The hoist and the two dep-array swaps, measured rather than inferred
reportError moves from ~:10306 up to :9721 so the two callbacks below it can reference it. Two failure modes were checked directly:
- TDZ — refuted by declaration order.
reportError's own dep array evaluates[pushToast]at:9734, andconst pushToast = useCallback(is at:4004— 5,717 lines earlier. The three predicates it calls are hoisted module-levelfunctiondeclarations. (Moving a declaration earlier can only ever create a TDZ if its own dependencies sit later; that is the one thing to measure, and it does not.) - Stale-closure /
exhaustive-deps— refuted by measuring each callback body, not by range arithmetic. Both changed dep arrays swappushToast→reportError. The enclosing callback for the direct-submit site isconst sendPrompt = useCallback(at:9736, andpushToastappears zero times between:9736and its changed call at:9863whilereportErrordoes appear. For the queued site the enclosing callback isconst enqueuePrompt = useCallback(at:10512, andpushToastappears zero times between:10512and its changed call at:10645whilereportErrordoes. Each array therefore drops a symbol its body no longer references and adds the one it does — the correct transformation, not a lint suppression.- Instrument note, so the reader can weigh this correctly:
eslint.config.js:64appliesreactHooks.configs['recommended-latest'], in whichexhaustive-depsis a warning. The greenLint & Static (ubuntu-latest, Node 22.x)lane therefore corroborates this but cannot prove it. The body measurement above is the proof.
- Instrument note, so the reader can weigh this correctly:
6. CI census at head
78/78 check-runs enumerated with fetched == total_count == 78 asserted and page 2 returning items=0. Histogram {success: 17, skipped: 61} — 0 failing, 0 in progress, 0 queued. Product lanes, named:
| lane | result |
|---|---|
Test (ubuntu-latest, Node 22.x) — the lane that runs the two witnesses |
success 17:52:22Z → 18:14:49Z |
Lint & Static (ubuntu-latest, Node 22.x) |
success |
Integration Tests (no-AK, No Sandbox) |
success |
web-shell E2E Smoke (ubuntu-latest, Node 22.x) |
success 18:14:52Z → 18:28:04Z |
Capture web-shell visuals (ubuntu-latest, Node 22.x) |
success |
Desktop Shell (ubuntu-22.04) / Desktop Shell (windows-2022) |
success / success |
review-pr |
success 17:57:20Z → 19:59:47Z |
triage |
success |
Test (windows-latest…), Test (macos-latest…), Integration Tests (CLI, No Sandbox) |
skipped — structural, by workflow filter, not by this diff |
The remaining 26 lane names are the automation fan-out (route, authorize, precheck-pr, review-config, resolve-pr, delay-automatic-review, ack-review-request, Signal the reviewed fork PR, remove-suspicious-attachments, minimize, publish-resolution, publish-tmux, publish-verify, fallback-comment, review-address, review-scan, build-cli, retry-command, takeover-command, takeover-ack, issue-autofix, tmux-testing, verify, Classify PR, assign, label) and are excluded from both polarities.
Rollup disagreement recorded rather than resolved: REST reads review_decision=null mergeable_state=blocked while mergeable=true and the row set carries an at-head APPROVED. Only rows are treated as evidence here, per the standing convention that a rollup can hide a failure in either direction.
7. The one open finding is Suggestion-level, and I concur with that severity
Inline R1-1 (App.tsx:1381, comment 3992879886) is that the new JSDoc sentence promises unconditionally that a rejection's Error.message is surfaced, while reportError suppresses aborts, daemon-turn errors and already-dispatched notices. That is a genuine doc/behaviour divergence on an exported public prop (WebShellProps.prepareSubmit and onSubmitBefore both gained the sentence), and the fix is a one-line qualification of the JSDoc. It does not block: hosts' localized messages are still surfaced for every non-abort rejection, which is the behaviour the #9911 fix depends on and which §4's adjacent control pins. I have no Critical to add and no stylistic comment to make.
8. What would change this conclusion
A head move past ba1a7ffcf597a4f103d49ad089d8b07c4fa0e351 voids the A/B above (the base arm would need re-running against the new base). A newer at-head CHANGES_REQUESTED would close the approval leg while leaving the measurements standing.
Approval. Unlike a report posted while a bot review lane is still in flight, here the lane that produced the at-head APPROVED has itself concluded (review-pr success 19:59:47Z, step 13 Run review completed/success, both fallback writer steps skipped) and the review it produced filed 0 Criticals, so no in-flight bot lane can supersede the approval leg. An APPROVE is therefore submitted in the same round as this report rather than deferred to a later one. Both statements are time-scoped to the state read immediately before posting; this comment itself carries no approval — the approval is the separate review row that accompanies it.
中文说明
结论:在上述 head(ba1a7ffcf597)可以合入。
为什么不是 tmux E2E 报告: 本次改动的 packages/web-shell/client/App.tsx 在整个仓库中只有一个值导入方(client/index.tsx:6),其余均为类型导入;packages/cli/src 与 packages/core/src 中对 web-shell/client 的引用为 0 个 import(只有一处注释和测试里的 glob 字符串字面量)。因此 Ink TUI 根本不会加载被改动的文件,tmux 无法观测该行为。ci-bot 在评论 5638706275 中也独立作出了同样判断。浏览器侧已由本 head 上全绿的 web-shell E2E Smoke 与 Capture web-shell visuals 两条通道覆盖,本地再跑 Playwright 只是重复,不是增量证据。
本报告的非重复核心,正是 ci-bot 自己点明无法验证的那一半。 它在 5638706275 与 5638719974 两处说明:测试计划的「红半边」(两个 witness 在 main 上应当以 onToast 被调用为 ["error","cancelled"] 而失败)只是作者的陈述,因为该 gate 不执行 PR 代码。本次两半都实际执行了:
- 树构造:拉取
refs/pull/11676/head并以 detached 方式 checkout 出 PR 自己的 head 树(与 CI 检出的树一致),而非把 blob 覆盖到本地 main 上——本地642d36e6d8正是本 PR 的 merge-base,与 head 相差 313 个文件,两个被改动文件的 base blob 相等性 2/2 失败,覆盖式构造会被污染。依赖接线:根node_modules1,148 条链接,@qwen-code/*24/24 全部重新指向 head 树内部,逐包node_modules14 条,正确跳过 1 个(head 树中不存在的packages/webui)。@qwen-code/sdk/daemon解析到构建产物,且packages/sdk-typescript与 main 相差 4 个文件,因此 SDK 与前置的@qwen-code/acp-bridge都在 head 树内从源码构建,未链接 main 的dist。 - ARM A(head,绿):
2 passed | 909 skipped (911),退出码 0。 - ARM B(仅回退生产代码,测试保持 head,红):
2 failed | 909 skipped (911),退出码 1,两处失败载荷均为Array [ "error", "cancelled" ](App.test.tsx:21751与:21772)——与 ci-bot 说「无人观测过」的载荷逐字一致。
⇒ 两个 witness 是承重的、非空洞的:改动前失败、改动后通过,且各自都先断言了使自己能走到 catch 的前置条件。
顺带澄清 ci-bot 的 Test Plan 「不一致」: 全文件在 head 上实测为 911 passed (911),退出码 0。作者声称的 911 passed 与 2 passed 都正确,只是口径不同(前者是整个文件,后者是按名过滤的两个新 witness)。7432 / 30786 / 569 不可能是本文件的数字(本文件只收集 911 个用例),读起来像是 bot 改跑了别的范围所得的总数——这一句是关于 bot 调用方式的推断,不是实测;实测的只有「本文件收集 911、通过 911」。no such file or directory 则是路径口径问题:vitest 配置里 root: 'client',相对该 root 的路径是 App.test.tsx 而非 client/App.test.tsx。因此该「不一致」并不构成对作者的反证。
过度抑制(#9911 回归风险): reportError 只抑制三类错误,且每类都需要狭窄的内部标记(name === 'AbortError'、_alreadyDispatched === true、_daemonTurnError === true),宿主本地化的 rewind 错误不会偶然携带。文件中紧邻两个新断言之上的 surfaces a queued preparation rejection instead of cancelling silently(:21709-21733)断言真实本地化拒绝仍然弹出 ('error','The original message can no longer be edited.'),构成同文件相邻的已知阳性对照,且它在 head 的 911 个通过用例之中。
hoist 与依赖数组: reportError 上移到 :9721,其自身依赖 pushToast 声明于 :4004(早 5,717 行),无 TDZ。两处依赖数组把 pushToast 换成 reportError,经直接测量:sendPrompt(:9736 起)与 enqueuePrompt(:10512 起)的函数体内 pushToast 出现 0 次、reportError 出现,故两处替换均正确。注意 eslint.config.js:64 使用的 recommended-latest 中 exhaustive-deps 是 warning,所以 Lint & Static 全绿只能佐证、不能证明——证明来自上面的函数体测量。
CI: 枚举 78/78 条 check-run(fetched == total_count,第 2 页 items=0),直方图 {success: 17, skipped: 61},0 失败 / 0 进行中 / 0 排队;产品通道 9 绿 + 3 个结构性跳过,另有 26 个自动化通道名按惯例在两个方向上都排除。REST 汇总字段与行集不一致(review_decision=null mergeable_state=blocked 而 mergeable=true 且存在 at-head APPROVED),此处只以行为证据。
唯一未决发现是 Suggestion 级(R1-1,App.tsx:1381:新增的 JSDoc 无条件承诺 rejection 的 Error.message 会被呈现,而实现会抑制三类),我同意其严重级别,不构成阻塞。我没有 Critical 要补充。
关于 approve: 与「bot 审查通道仍在飞行中时发报告」的情形不同,此处产生 at-head APPROVED 的通道本身已结束(review-pr success,19:59:47Z,step 13 Run review completed/success,两个兜底写入步骤均 skipped),且其产出的审查提交了 0 个 Critical,因此不存在能推翻批准leg的在飞通道。批准与本报告在同一轮提交,而非推迟到下一轮。以上均为「发帖前即时读取的状态」,本评论自身不携带批准,批准是随附的独立 review 行。
qqqys
left a comment
There was a problem hiding this comment.
Independent verification at head ba1a7ffcf597a4f103d49ad089d8b07c4fa0e351: no Critical found. Full evidence in the accompanying comment.
The non-duplicative part is the executed A/B of the two new witnesses, which the bot named twice as the half it could not observe. Both arms were run in the PR's own head tree (refs/pull/11676/head, fetched and checked out detached — never a blob overlay onto local main, whose 642d36e6d8 is this PR's merge-base and differs from head on 313 files):
- head, both files at head:
2 passed | 909 skipped (911), exit 0. - production reverted to base blob
803eca3bcfb6, tests kept at head90916c088239:2 failed | 909 skipped (911), exit 1, both failures carryingonToastcalled as["error", "cancelled"]atApp.test.tsx:21751and:21772.
So the witnesses are load-bearing rather than vacuous. The unfiltered file is 911 passed (911), exit 0, which reconciles the Test-Plan "mismatch": the author's 911 passed and 2 passed are both correct at different scopes, while the 7432 / 30786 / 569 the bot observed cannot be this file's totals (it collects 911) and so appear to come from a different scope — that last part is an inference about the bot's invocation, not a measurement.
Over-suppression (the #9911 risk the changed comments themselves name) is closed by an adjacent known-positive control in the same file: surfaces a queued preparation rejection instead of cancelling silently — a real localized rejection still toasts ('error', 'The original message can no longer be edited.'), and it is among the 911 passing at head. The reportError hoist and both pushToast → reportError dep-array swaps were verified by measuring each enclosing callback body (sendPrompt at :9736, enqueuePrompt at :10512; pushToast appears zero times in either) rather than by range arithmetic, and pushToast is declared at :4004, 5,717 lines above the hoisted reportError, so there is no TDZ.
CI: 78/78 check-runs enumerated, {success: 17, skipped: 61}, 0 failing / 0 in progress; product lanes 9 green plus 3 structural skips. The review-pr lane that posted the at-head APPROVED has itself concluded success (job 103366066456, 19:59:47Z, step 13 Run review completed) and its review filed 0 Criticals, so no in-flight bot lane can supersede the approval leg.
The remaining inline R1-1 (the new JSDoc promising unconditionally that a rejection's Error.message is surfaced, while reportError suppresses aborts, daemon-turn errors and already-dispatched notices) is Suggestion-level on an exported public prop and does not block; a one-line JSDoc qualification would close it.
Note this is a Web Shell (browser-embedded) surface with no TUI consumer — App.tsx has exactly one value importer, client/index.tsx:6, and zero imports from packages/cli/src or packages/core/src — so a tmux-driven run is structurally incapable of exercising it, and the substitute instrument is named in the report rather than quietly skipped.
Time-scoped to the state read immediately before submitting.
|
Released in v0.23.4. |
What this PR does
Completes the web-shell half of #11432 by routing the two preflight-failure toast sites in
packages/web-shell/client/App.tsxthrough the file's existingreportErrorhelper instead of hand-rollingpushToast('error', formatError(err, ...)).reportErrorsuppressesAbortError, daemon-turn errors and already-dispatched notices before surfacing anything, so a daemon teardown that rejects a submission with anAbortErrornow stays silent instead of showing a "failed" toast for a submission the user cancelled.To do this,
reportErroris hoisted abovesendPrompt/enqueuePrompt— its declaration previously sat below both, which would be a TDZ error once their dependency arrays referenced it. The now-redundantconsole.warnin each preflight catch is dropped, becausereportErroralready logs the error viaconsole.errorand keeping the warn would double-log every real failure (this is the "reportError double-logging" item the CI triage on #11432 asked to fold into scope). TheprepareSubmit/onSubmitBeforeprop contract now states that a rejection'sError.messageis shown to the user, so hosts must localize it.Why it's needed
#11432 was deferred from #11171's review thread as two halves. Half 1 (localizing rewind preflight errors in the VS Code companion) landed in #11628. This PR is the remaining half: suppressing the abort toast, so a cancelled submission doesn't surface a spurious "failed" toast.
Reviewer Test Plan
How to verify
A witness test asserts
onToastis not called whenprepareSubmitrejects with anAbortError(both direct and queued paths). Onmainthis fails — the toast fires with the rawError.message— and passes after the fix.Red (before fix): 2 failed —
onToastcalled with["error", "cancelled"].Green (after fix): 2 passed.
Evidence (Before & After)
N/A — the change is a toast being suppressed, pinned by unit tests rather than a screenshot.
Tested on
Environment
Local:
cd packages/web-shell && npx vitest run client/App.test.tsx(911 passed) andnpx tsc -p tsconfig.json --noEmit(0 errors), pluseslintandprettier --checkon the two changed files.Risk & Scope
AbortErrorpreflight rejection now logs nothing to the console (reportErrorreturns early on abort). That is the intended "suppress abort" behavior; real failures still log viareportError'sconsole.error.Linked Issues
Fixes #11432
中文说明
本 PR 做了什么
完成 #11432 的 web-shell 半边:把
packages/web-shell/client/App.tsx里两处 preflight 失败 toast 从手写的pushToast('error', formatError(err, ...))改为复用文件内已有的reportError。reportError会先抑制AbortError、daemon-turn 错误和已派发通知,所以 daemon 关闭时用AbortError拒绝提交的场景现在会静默,而不再为一个「用户已取消」的提交弹出「failed」toast。为此把
reportError上提到sendPrompt/enqueuePrompt之前——它原来声明在两者之后,一旦这两个回调的依赖数组引用它就会触发 TDZ 错误。两处 preflight catch 里原本的console.warn被移除,因为reportError自己会通过console.error记录错误,保留 warn 会让每次真实失败都重复打两条日志(这正是 #11432 的 CI triage 要求并入 scope 的「reportError 重复日志」项)。prepareSubmit/onSubmitBefore的 prop 契约也补充说明:rejection 的Error.message会展示给用户,宿主必须本地化它。为什么需要
#11432 从 #11171 的 review 线程延后为两件套。半边 1(VS Code companion 里本地化 rewind preflight 错误)已在 #11628 落地。本 PR 是剩下的半边:抑制 abort toast,让被取消的提交不再弹出一个误导性的「failed」toast。
Reviewer 测试方案
如何验证
新增 witness 测试断言:当
prepareSubmit用AbortErrorreject 时,onToast不被调用(直发与排队两条路径都覆盖)。在main上该断言失败——toast 会带着原始Error.message弹出——修复后通过。修复前(红态):2 条失败——
onToast被调用且参数为["error", "cancelled"]。修复后(绿态):2 条通过。
证据(Before & After)
N/A——这是一处被抑制的 toast,用单元测试钉住行为,不涉及截图。
测试平台
环境
本机:
cd packages/web-shell && npx vitest run client/App.test.tsx(911 条全部通过)与npx tsc -p tsconfig.json --noEmit(0 错误),另对两个改动文件跑了eslint与prettier --check。风险与范围
AbortError的 preflight 拒绝现在不会再往控制台打日志(reportError在 abort 时提前返回)。这是「抑制 abort」的预期行为;真实失败仍会通过reportError的console.error记录。关联 Issue
Fixes #11432