fix(web-shell,core): clear leaked test-run async under the unhandled-error gate - #10655
Merged
Conversation
Copy buttons flip to a check mark and schedule a bare setTimeout(() => setCopied(false)) to reset it. When a test file's environment is torn down within that window the callback throws ReferenceError: window is not defined out of react-dom's state dispatch, and since the unit suites fail on unhandled errors an all-green run exits 1 — main's own Test (ubuntu) hits this through AssistantMessage.test.tsx. A new useCopiedFlash hook owns the transient flag: flash() sets it and schedules the reset, a re-flash restarts the window instead of letting the older timer cut the newer feedback short, and unmount clears the pending timer. The six call sites with the bare pattern (assistant, system and timestamp messages, both Markdown code-block buttons, the Git log dialog) adopt it; SessionOverviewPanel's SessionIdCell already carried the cleanup. Two tests pin the cleanup via vi.getTimerCount() after unmount and one pins the restarted window.
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed bug, not theoretical — I verified the cited CI evidence. Run 33407726599 (attempt 1) Direction: aligned — this is a CI-stability fix that also happens to be correct React unmount hygiene. No product-direction surface. No direct CHANGELOG reference (internal test hygiene, nothing user-visible). Size: not applicable — no core paths touched (all changes are in Approach: scope feels right. I grepped the base tree: Risk: no elevated risk signals (no high-risk path matches). Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，不是理论问题——我核实了引用的 CI 证据。Run 33407726599（attempt 1）的 方向：对齐——这是一个 CI 稳定性修复，同时也是正确的 React 卸载清理。无产品方向影响。CHANGELOG 无直接引用（内部测试卫生，无用户可见变化）。 规模：不适用——未触及核心路径（全部改动在 方案：范围合理。我在基线代码里 grep 过： 风险：无升级风险信号（未命中任何高风险路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewBefore reading the diff: with six identical bare What I checked against the base tree:
One non-blocking edge worth naming since it sits in the same failure family this PR closes: if the clipboard promise resolves after unmount, No blockers. Small, focused, convention-following (collocated tests, license headers, hook placement in Testing evidenceCI on the reviewed commit is still in flight; this is a web-shell-only change so the CI profile narrowed the matrix — the ubuntu Test job carries the unit suite while macOS/Windows are skipped by profile. Within that job the lint steps (ESLint, actionlint, shellcheck, yamllint) already passed and Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The claim under test is a test-suite leak fix, and the unit suite is itself the oracle — the discriminating tests pin the cleanup, and the cited before-failure (run 33407726599) came from exactly this suite. So the green-or-red verdict comes from the pending check above, no separate lane needed. No user-visible behaviour is claimed ( 中文说明代码审查看 diff 之前的独立设想：六处完全相同的裸 对照基线逐项核实：
一个不阻塞的边界，因同属本 PR 闭合的失败家族而点名：若剪贴板 promise 在卸载之后才 resolve， 无阻塞项。改动小而聚焦、遵循约定（测试共置、许可头、hook 放在 测试证据评审提交上的 CI 仍在进行；这是仅涉 web-shell 的改动，CI profile 收窄了矩阵——ubuntu Test job 承载单测套件，macOS/Windows 按 profile 跳过。该 job 内 lint 步骤（ESLint、actionlint、shellcheck、yamllint）已通过， 待验证的命题是测试套件泄漏修复，而单测套件本身就是 oracle——判别性测试钉住了清理逻辑，引用的 before 失败（run 33407726599）正出自这个套件。因此绿/红结论由上表未决的 check 给出，无需另开验证通道。无用户可见行为主张（真实场景测试 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — verified failure, minimal fix, discriminating tests; the only thing left is CI landing green. This one is easy to be sure about. The failure is real — I read the unhandled-error section of the cited run's log myself rather than taking the description at face value, and it matches line for line, stack frame at CI on 中文说明置信度：5/5 —— 失败已核实、修复最小化、测试具判别性；只等 CI 变绿。 这个 PR 很容易确信。失败是真实的——我没有照单全收描述，而是亲自读了引用 run 日志的 unhandled-error 段落，逐字吻合，包括
— Qwen Code · qwen3.8-max Reviewed at |
Collaborator
🖼️ 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 5 render-shaping files:
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 |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
approved these changes
Aug 31, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
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
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): client/hooks/useCopiedFlash.test.tsx — no such file or directory; client/components/MessageTimestamp.test.tsx — no such file or directory; client/components/dialogs/GitLogDialog.test.tsx — no such file or directory; 773 tests green — this review observed 5272, 503 passed; Tests 5406 passed — this review observed 5272, 503 passed.
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：client/hooks/useCopiedFlash.test.tsx — no such file or directory; client/components/MessageTimestamp.test.tsx — no such file or directory; client/components/dialogs/GitLogDialog.test.tsx — no such file or directory; 773 tests green — this review observed 5272, 503 passed; Tests 5406 passed — this review observed 5272, 503 passed。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Comment on lines
+24
to
+28
| const flash = useCallback(() => { | ||
| setCopied(true); | ||
| window.clearTimeout(timerRef.current); | ||
| timerRef.current = window.setTimeout(() => setCopied(false), resetMs); | ||
| }, [resetMs]); |
Collaborator
There was a problem hiding this comment.
[Suggestion] R1-1: flash() invoked after the component has unmounted schedules a reset timer that the already-run unmount cleanup never clears, so the hook's documented guarantee — the pending reset is cleared on unmount — does not hold on the async entry path all six call sites use. Every call site invokes flashCopied() from writeClipboardText(...).then(...), and clipboard.ts documents that writeText can stay pending until the user answers the clipboard permission prompt. If the user clicks Copy and the component unmounts while the prompt is open (navigation, closing the Git Log dialog), the one-shot cleanup has already run when the promise settles, and flash() schedules an orphan setTimeout(() => setCopied(false), resetMs) that survives teardown — the same leaked-timer class this hook exists to prevent. That is benign in production today (the orphan callback is a setCopied no-op), but any future test that unmounts before the promise settles re-creates the post-teardown failure mode this PR exists to kill, since dangerouslyIgnoreUnhandledErrors is false on Linux CI.
Witness:
probe (fake timers: render → unmount → invoke captured flash()):
intact code: timers pending after post-unmount flash(): 1
with mounted-ref fix: timers pending after post-unmount flash(): 0
useCopiedFlash.test.tsx 3/3 green in both runs
Suggested fix — track mounted state inside the hook and bail in flash:
const mountedRef = useRef(true);
useEffect(() => {
mountedRef.current = true;
return () => {
mountedRef.current = false;
window.clearTimeout(timerRef.current);
};
}, []);
const flash = useCallback(() => {
if (!mountedRef.current) return;
setCopied(true);
...
}, [resetMs]);Note that call sites list flashCopied in their own useCallback deps (e.g. MessageTimestamp.tsx }, [copyText, flashCopied]); in this diff), so the fix must keep flash referentially stable across renders — a ref-based guard does. If you add the guard, please also add a case in useCopiedFlash.test.tsx that flashes, unmounts, then calls the captured flash() again and asserts vi.getTimerCount() is 0 — and confirm that assertion goes red with the guard removed.
中文说明
组件卸载后 flash() 再被调用时，会调度一个重置计时器，而已执行的卸载清理永远不会清掉它，因此 hook 文档中「未决重置会在卸载时被清理」的保证在六个调用点共用的异步入口路径上不成立。所有调用点都在 writeClipboardText(...).then(...) 里调用 flashCopied()，而 clipboard.ts 说明在剪贴板写入权限未决时 writeText 会一直挂起、直到用户回应权限弹窗。用户点击复制后组件在弹窗期间卸载（跳转页面、关闭 Git Log 对话框）时，一次性清理已经执行，promise 随后 settle 并调用 flash()，调度出一个活过环境拆除的孤儿 setTimeout(() => setCopied(false), resetMs)——正是这个 hook 要消灭的泄漏计时器类别。生产环境目前无害（孤儿回调只是 setCopied 空操作），但未来任何在 promise settle 前卸载组件的测试都会重新触发本 PR 要消灭的拆除后失败路径（Linux CI 上 dangerouslyIgnoreUnhandledErrors 为 false）。
建议修复：在 hook 内跟踪挂载状态，flash 开头对已卸载直接返回（见上方代码）。注意各调用点把 flashCopied 列入了自己的 useCallback 依赖（如本 diff 中 MessageTimestamp.tsx 的 }, [copyText, flashCopied]);），修复必须保持 flash 跨渲染的引用稳定——基于 ref 的守卫可以做到。如果加了这个守卫，请在 useCopiedFlash.test.tsx 补一条用例：flash、卸载、再次调用捕获的 flash() 并断言 vi.getTimerCount() 为 0——并确认删掉守卫时该断言变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Comment on lines
+19
to
+22
| function Probe({ resetMs }: { resetMs?: number }) { | ||
| latest = useCopiedFlash(resetMs); | ||
| return null; | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] R1-4: The hook's resetMs parameter — used in production by GitLogDialog.tsx as useCopiedFlash(1500) — is exercised by no test anywhere, even though this Probe already declares a resetMs prop that no test ever sets. Every test mounts the probe without resetMs, so only the 2000ms default path runs: hardcoding 2000 in the hook's setTimeout keeps the entire suite green (verified mutant: 27/27 pass), so GitLogDialog's commit-SHA copy feedback would silently stretch from 1.5s to 2s with no failing test to notice — and the unused prop tells the next reader that custom delays are covered when they are not.
Witness:
hardcoded-2000 mutant: Tests 1 failed | 27 passed
(the 1 failure is a new resetMs=500 probe: expected true to be false;
all 27 pre-existing tests green)
intact hook: the same probe passes
GitLogDialog.test.tsx grep copied|1500|advanceTimers|clipboard: zero matches
Suggested fix — re-render the probe with a custom delay (the harness root is reusable):
it('resets after a custom delay', () => {
act(() => {
root.render(<Probe resetMs={500} />);
});
act(() => latest![1]());
expect(latest![0]).toBe(true);
act(() => {
vi.advanceTimersByTime(499);
});
expect(latest![0]).toBe(true);
act(() => {
vi.advanceTimersByTime(1);
});
expect(latest![0]).toBe(false);
});The new case itself is the acceptance criterion — replacing resetMs with a hardcoded 2000 in useCopiedFlash.ts must make it fail.
中文说明
hook 的 resetMs 参数——生产环境里 GitLogDialog.tsx 以 useCopiedFlash(1500) 使用——没有任何测试覆盖，尽管这个 Probe 已经声明了一个从未被任何测试赋值的 resetMs prop。所有测试挂载探针时都不传 resetMs，因此只有 2000ms 默认路径被执行：把 2000 硬编码进 setTimeout 后整个套件依然全绿（已验证的变异体：27/27 通过），Git log 对话框的复制反馈会从 1.5 秒悄悄拉长到 2 秒而无任何测试察觉——未使用的 prop 还会让后来的读者误以为自定义时长已有覆盖。
建议修复：给探针传入自定义时长重新渲染（见上方代码，harness 的 root 可复用）。新增用例本身就是验收标准——把 useCopiedFlash.ts 里的 resetMs 替换成硬编码 2000 必须让它变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Collaborator
Review & verification report — head
|
qqqys
approved these changes
Aug 31, 2026
qqqys
left a comment
qqqys
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed and verified at head ede5006: no Critical — full report above (issuecomment-5482531723). CI green on this head; all six copy-feedback timer sites converted with correct unmount cleanup; 228/228 related unit tests green locally.
wenshao
changed the title
Collaborator
|
Released in v0.23.0. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Clears the "copied" feedback reset timer on unmount for every copy button in the Web Shell. A new
useCopiedFlashhook owns the transient flag (flash()sets it and schedules the reset, a re-flash restarts the window, unmount clears the pending timer), and the six call sites that scheduled a baresetTimeout(() => setCopied(false), …)—AssistantMessage,SystemMessage,MessageTimestamp, both code-block copy buttons inMarkdown, andGitLogDialog— now use it.SessionOverviewPanel'sSessionIdCellalready carried this cleanup; the hook applies the same pattern everywhere.Second instance of the same class (96c53f1):
packages/core'sarchive-safety.test.tsawaitstar.c, but the async file writer can emit a trailing write after its promise resolves (a hard-link entry's jobDone racing the stream end) — CI hitError: write after endthrough it twice in a row, again with every test green. The fixtures now pack withsync: true; assertions unchanged.Why it's needed
Test (ubuntu)runs fail with all tests green: the copy tests click a copy button, the 2 s reset timer outlives the test file's jsdom environment, and the callback then throwsReferenceError: window is not definedout ofreact-dom's state dispatch. Since #10443 aligned the unit suites on failing for unhandled errors, that single leaked timer turns an all-green run red — seen onmainitself (run 33407726599'sRun tests and generate reportsstep ends withTest Files 241 passed,Tests 5406 passed,Errors 1 error, exit 1, originating inAssistantMessage.test.tsx) and on any branch that merges currentmain.Reviewer Test Plan
How to verify
main,cd packages/web-shell && npx vitest run --config vitest.config.tscan end withErrors 1 error(ReferenceError: window is not definedatAssistantMessage.tsx:67) despite all tests passing — the timing depends on whether the worker tears the environment down within 2 s of the copy test. On this branch the run ends with no unhandled errors.npx vitest run client/hooks/useCopiedFlash.test.tsx client/components/messages client/components/MessageTimestamp.test.tsx client/components/dialogs/GitLogDialog.test.tsx— 34 files / 773 tests green.useCopiedFlash › clears the pending reset on unmountandAssistantMessage copy reset timer › leaves no pending reset timer behind on unmountboth assertvi.getTimerCount() === 0after unmount — removing the hook's cleanup line turns exactly these two red (verified).restarts the reset window on a re-flashpins that a second copy is not cut short by the first copy's reset.Evidence (Before & After)
N/A (no user-visible change). Before: CI run 33407726599 on
Test (ubuntu)—Tests 5406 passed,Errors 1 error, exit code 1. After: the same full suite ends with no unhandled errors.Tested on
Environment (optional)
Unit tests only (vitest/jsdom).
Risk & Scope
flashCopiedcallback where handlers use it.Linked Issues
The unhandled-error gate landed in #10443; the affected copy button family was extended by #10001.
中文说明
本 PR 做了什么
为 Web Shell 所有复制按钮在卸载时清理「已复制」反馈的重置计时器。新增
useCopiedFlashhook 承载这个瞬时标志（flash()置位并调度重置、再次 flash 重开窗口、卸载清掉未决计时器），六个原先裸调setTimeout(() => setCopied(false), …)的调用点——AssistantMessage、SystemMessage、MessageTimestamp、Markdown里两个代码块复制按钮、GitLogDialog——全部换用。SessionOverviewPanel的SessionIdCell本就带此清理；hook 把同一范式推广到所有位置。同类第二实例（96c53f1）：
packages/core的archive-safety.test.ts虽然await tar.c，但其异步文件写入器可能在 promise 落定后再补一笔写入（硬链接条目的 jobDone 与流结束竞争）——CI 连续两次经它触发Error: write after end，同样测试全绿。夹具改为sync: true打包；断言不变。为什么需要
Test (ubuntu)在测试全绿的情况下失败：复制测试点击按钮后，2 秒重置计时器活过了测试文件的 jsdom 环境，回调随后在 react-dom 的状态派发里抛出ReferenceError: window is not defined。#10443 统一让单测套件对 unhandled error 判败后，这一个泄漏计时器就能把全绿运行打红——main自身已中招（run 33407726599 的测试步骤以Test Files 241 passed、Tests 5406 passed、Errors 1 error、exit 1 结束，错误源自AssistantMessage.test.tsx），合入当前main的分支同样会中。评审验证计划
如何验证
main上cd packages/web-shell && npx vitest run --config vitest.config.ts可能以Errors 1 error（AssistantMessage.tsx:67的window is not defined）结束——是否触发取决于 worker 是否在复制测试后 2 秒内拆除环境。本分支上运行以零 unhandled error 结束。npx vitest run client/hooks/useCopiedFlash.test.tsx client/components/messages client/components/MessageTimestamp.test.tsx client/components/dialogs/GitLogDialog.test.tsx——34 文件 / 773 用例全绿。useCopiedFlash › clears the pending reset on unmount与AssistantMessage copy reset timer › leaves no pending reset timer behind on unmount都断言卸载后vi.getTimerCount() === 0——删掉 hook 的清理行恰好这两条变红（已验证）。restarts the reset window on a re-flash钉住第二次复制不被第一次的重置截断。证据（Before & After）
N/A（无用户可见变化）。Before：CI run 33407726599 的
Test (ubuntu)——Tests 5406 passed、Errors 1 error、退出码 1。After：同一全量套件以零 unhandled error 结束。测试平台
macOS ✅；Windows⚠️ 未测；Linux ⚠️ 未测（CI 覆盖）。
环境
仅单元测试（vitest/jsdom）。
风险与范围
flashCopied的 handler 依赖数组补入了这个稳定回调。关联 Issue
unhandled-error 门由 #10443 引入；受影响的复制按钮族由 #10001 扩展。