refactor(web-shell): share sidebar test harness - #9797
Conversation
|
Re-run note: this gate was first posted on Aug 23 and re-verified on Aug 25 — no new commits have landed (same head SHA), and every finding below still holds.
Moving on to code review. 🔍 中文说明Re-run 说明：本门禁首发于 8 月 23 日，8 月 25 日复核——没有新提交（head SHA 不变），下列结论全部依然成立。
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-run on Aug 25: no new commits since the first pass, and I re-verified the findings against the diff and the base tree rather than trusting the earlier write-up. Still no blockers — the interesting part of this PR is whether unifying the three resolvers silently changes any fixture's behavior, so that's where the review stays focused. The unified resolver is equivalent to the old ones on every fixture state these suites actually produce.
Everything else checks out: the workspace-removal Test evidenceThe previous pass found all three fork CI workflows sitting in
The author's "128 targeted tests pass" claim was unverified last pass (Windows-only, author-reported); CI now independently runs the same suites on Linux/Node 22 and passes. No sandboxed-verification lane is named here because there is no behavioural claim to settle — this is a test-only refactor with zero production files changed, and the test suite is itself the deliverable. Real-scenario testing: N/A — unattended CI run, and the change is test-only with nothing user-visible. Not verified: nothing outstanding — the previous pass's only gap (suite results on this commit) is now covered by green CI. 中文说明代码审查8 月 25 日 re-run：自首轮审查以来没有新提交，我对照 diff 与基线树重新核实了每项结论，而非沿用旧记录。仍无阻塞问题——本 PR 的关键在于统一三个 resolver 是否会悄悄改变某个 fixture 的行为，审查集中在这里。
其余均核对无误：workspace-removal 的 测试证据上一轮时三个 fork CI 工作流都处于 作者"128 项测试通过"的声明上一轮无法核实（仅 Windows、作者自述）；CI 现已在 Linux/Node 22 上独立运行同样的套件并通过。此处不点名沙盒验证通道，因为没有需要落地的行为声明——这是零生产文件改动的纯测试重构，测试套件本身就是交付物。 真实场景测试：不适用——无人值守 CI 运行，且改动仅涉及测试，无用户可见变化。未验证项：无遗留——上一轮唯一的缺口（该提交上的套件结果）已由绿色 CI 覆盖。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review and green CI on the reviewed commit; the cap is pure policy — a fork Re-run reflection: nothing has changed in the code since the first pass (same head SHA), and re-verifying it only confirmed the earlier read. My independent proposal before touching the diff would have been exactly this shape — one collocated harness for the shared plumbing, the three diverged unloaded-page resolvers unified onto the real hook's contract, suite-specific controllers kept local — and the PR matches it down to the file name suggested in #9701. The only place a harness consolidation can bite is silently changing fixture semantics; I checked that fixture-by-fixture again this pass, including the workspace-removal suite's two unloaded-page cases in the base tree, and the single semantic change is the divergence the issue was opened to fix. Every edit is necessary, no expectation churn, zero production files touched, and the design doc earns its keep. What changed since the first pass is the evidence. Back then the three fork CI workflows were stuck in The reason I'm still not approving is the one that doesn't go away with evidence: fork ⏸️ Deferring to @wenshao — you triggered this re-run, and the code and CI now have nothing outstanding on my end. If the extraction delivers what #9701 (filed by @yiliang114 after the #9560 review) asked for, this needs your human approval to go with the green CI. I see nothing standing in the way of merge. 中文说明置信度：3/5 —— 审查干净、审查提交上的 CI 全绿；压到 3 分纯属策略——fork 的 Re-run 复盘：首轮以来代码没有变化（head SHA 相同），重新核实只是确认了先前判断。在读 diff 之前我的独立方案就是这个形状——一个同目录 harness 承载共享样板，把三个已分歧的未加载页面 resolver 统一到真实 hook 的契约，套件特有的 controller 保留本地——本 PR 连文件名都与 #9701 的建议一致。harness 合并唯一可能出问题的地方是悄悄改变 fixture 语义；本轮再次逐个核对，包括在基线树中检查 workspace-removal 的两个未加载页面用例，唯一的语义变化正是 issue 要修的分歧。每处改动都必要，断言零变化，零生产文件改动，设计文档言简意赅。 与首轮相比变化的是证据。当时三个 fork CI 工作流卡在 仍然不批准的原因与证据无关：按本仓库护栏，fork 的 ⏸️ 转交 @wenshao —— 这次 re-run 由你触发，代码与 CI 在我这里已无遗留。如果这次抽取兑现了 #9701（@yiliang114 在 #9560 评审后提出）的要求，需要你的人工批准与绿色 CI 配套。我看不到合并的阻碍。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): 128 tests pass — this review observed 4159 passed; 128 passed — this review observed 4159 passed.
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：128 tests pass — this review observed 4159 passed; 128 passed — this review observed 4159 passed。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| import { act } from 'react'; | ||
| import type { DaemonSessionSummary } from '@qwen-code/sdk/daemon'; |
There was a problem hiding this comment.
[Suggestion] The new WebShellSidebar.test-harness.ts is the first test-only file colocated under client/components/, and its .test-harness.ts suffix slips past every exclusion the package has for test code. tsconfig.lib.json includes client/**/*.ts and excludes only client/**/*.test.ts(x), client/e2e/**, and client/test/**, so the declaration build emits dist/types/components/sidebar/WebShellSidebar.test-harness.d.ts, and package.json publishes dist/types wholesale — test internals (installSidebarDomShims, makeSidebarSession, resolveWebShellSessions) end up in @qwen-code/web-shell's public type surface. Separately, the vitest.config.ts coverage excludes (**/*.test.{ts,tsx}, **/test/**, **/e2e/**, **/*.d.ts) do not match it either, so CI coverage counts this test-only file as production code, and its jsdom-unreachable shim branch means it can never reach full branch coverage — any future coverage threshold would fail on test-support code. Every earlier harness lives under the excluded client/e2e/ or client/test/ directories. Verified in a scratch tree: before the fix the lib build emits the .d.ts and coverage counts 268 files including the harness (97.29% statements); after adding the exclusions, zero test-harness declarations are emitted and the file disappears from coverage (267 files).
Fix — either add exclusions:
"client/**/*.test-harness.ts" → exclude in packages/web-shell/tsconfig.lib.json
'**/*.test-harness.{ts,tsx}' → coverage.exclude in packages/web-shell/vitest.config.ts
or move the harness into packages/web-shell/client/test/ (the package's existing convention, e.g. reactHarness.tsx).
中文说明
新的 WebShellSidebar.test-harness.ts 是第一个放在 client/components/ 下的测试专用文件，其 .test-harness.ts 后缀绕过了包内所有针对测试代码的排除规则。tsconfig.lib.json 的 include 为 client/**/*.ts，只排除了 client/**/*.test.ts(x)、client/e2e/** 和 client/test/**，因此声明构建会输出 dist/types/components/sidebar/WebShellSidebar.test-harness.d.ts，而 package.json 会整体发布 dist/types——测试内部实现（installSidebarDomShims、makeSidebarSession、resolveWebShellSessions）将进入 @qwen-code/web-shell 的公共类型面。另外，vitest.config.ts 的覆盖率排除项（**/*.test.{ts,tsx}、**/test/**、**/e2e/**、**/*.d.ts）同样不匹配该文件，所以 CI 覆盖率会把这个纯测试文件统计为生产代码；它在 jsdom 中不可达的 shim 分支意味着它永远无法达到完整的分支覆盖——未来若设置覆盖率阈值，会因测试支撑代码而失败。此前所有 harness 都放在被排除的 client/e2e/ 或 client/test/ 目录下。已在临时树中验证：修复前，库构建会输出该 .d.ts，覆盖率统计 268 个文件且包含该 harness（语句覆盖 97.29%）；加上排除项后，不再输出任何 test-harness 声明，该文件也从覆盖率报告中消失（267 个文件）。
修复方式——添加排除项：
"client/**/*.test-harness.ts" → packages/web-shell/tsconfig.lib.json 的 exclude
'**/*.test-harness.{ts,tsx}' → packages/web-shell/vitest.config.ts 的 coverage.exclude
或者将 harness 移到 packages/web-shell/client/test/（包内现有约定，如 reactHarness.tsx）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| export async function flushSidebar(): Promise<void> { | ||
| await act(async () => { | ||
| await Promise.resolve(); | ||
| await Promise.resolve(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
[Suggestion] flushSidebar is a byte-for-byte copy of the existing shared helper flushReact() in packages/web-shell/client/test/reactHarness.tsx — the same act() wrapping two awaited microtasks, the same () => Promise<void> signature. If the flush recipe ever needs tuning (for example an extra await round after a React scheduling change), the fix must be applied in both files, and updating only one leaves the sidebar suites silently diverging from every other consumer of flushReact — the exact copy-drift failure mode this PR exists to eliminate. Cross-directory imports from client/test already exist in this package (e.g. EnhancedMarkdownTable.test.tsx imports reactHarness).
| export async function flushSidebar(): Promise<void> { | |
| await act(async () => { | |
| await Promise.resolve(); | |
| await Promise.resolve(); | |
| }); | |
| } | |
| export { flushReact as flushSidebar } from '../../test/reactHarness'; |
中文说明
flushSidebar 与 packages/web-shell/client/test/reactHarness.tsx 中已有的共享辅助函数 flushReact() 逐字节相同——同样是 act() 包裹两次 await 微任务，签名同为 () => Promise<void>。如果将来需要调整刷新逻辑（例如 React 调度变化后需要多一次 await），修复必须同时应用到两个文件；只改其中一个会让侧边栏测试与其他所有 flushReact 使用者悄然分叉——这正是本 PR 要消除的复制漂移失效模式。包内已有跨目录引用 client/test 的先例（如 EnhancedMarkdownTable.test.tsx 引入 reactHarness）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| import { | ||
| clickSidebarElement as click, | ||
| flushSidebar, | ||
| installSidebarDomShims, | ||
| makeSidebarSession as makeSession, | ||
| resolveWebShellSessions, | ||
| } from './WebShellSidebar.test-harness'; |
There was a problem hiding this comment.
[Suggestion] The consolidation of click gestures is incomplete within this PR's own scope: this suite already imports clickSidebarElement as click (and uses it for its plain clicks), but still emits the full inline pointerdown+pointerup+click triple at three call sites — groupItem! (~lines 1071-1078), renameItem! (~lines 1385-1392), and document.body (~lines 1415-1422). That triple is exactly clickSidebarElement(el, true), which is what the workspace-removal suite's local click wrapper now delegates to. With two representations of the same 3-event gesture in the consolidated suite family, a future change to the gesture (e.g. Radix dismissable-layer handling requiring an extra event) will patch only one representation, and the suites will behave differently for the same interaction — the drift this refactor removes in workspace-removal survives here.
Replace each inline triple with the already-imported alias, passing true so the pointer events are preserved:
click(groupItem!, true);
// ...
click(renameItem!, true);
// ...
click(document.body, true);Note: calling click(el) with the default includePointerEvents = false would drop the pointer events and change behavior.
中文说明
点击手势的合并在本 PR 自身范围内并不彻底：本测试文件已经引入 clickSidebarElement as click（并在普通点击中使用），但仍在三处调用点内联发出完整的 pointerdown+pointerup+click 三连事件——groupItem!（约 1071-1078 行）、renameItem!（约 1385-1392 行）、document.body（约 1415-1422 行）。这个三连事件正是 clickSidebarElement(el, true)，也就是 workspace-removal 套件的本地 click 封装现在委托的实现。合并后的套件家族中同一 3 事件手势存在两种表示，未来一旦需要修改该手势（例如 Radix dismissable-layer 处理需要额外事件），只会修补其中一种表示，各套件对同一交互的行为将出现差异——本次重构在 workspace-removal 中消除的漂移在这里依然存在。
将每处内联三连事件替换为已引入的别名，并传入 true 以保留指针事件（见上方代码块）。
注意：使用默认 includePointerEvents = false 的 click(el) 会丢掉指针事件，从而改变行为。
— qwen3.8-max via Qwen Code /review (v0.22.0)
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after✅ No screenshot changes against the PR base. Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 33 passed · 0 failed · 33 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：33 通过 · 0 失败 · 33 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9797 Deep Verification — refactor(web-shell): share sidebar test harnessVerdict: merge-ready — assertions: 33 pass / 0 fail / 33 total 中文摘要
Central claim and scopePR 9797 is a test-only refactor (single commit Central claim: the consolidation is pinning-preserving — the head suites Secondary claims: (1) 128 tests pass and no production file changed; Explicitly out of scope (see Not covered): the full unit suite, browser GatesScripted in
The reviewer test plan's "repository build succeeds" is covered by the CI Resolver differential (wire-oracle style)The three base suites implemented different The probe (
All 11 unexercised divergences are shapes no fixture produces (census Mutation A/B matrix (test-only-PR method)Nine single-point mutants of the production files (untouched by the PR),
No mutant regressed from killed (base) to survived (head): 0 regressions. Positive control: M8 (same file, session-pinning suite) was killed by the FindingsSuggestion — pre-existing coverage gap: the optimistic-pin overlay is masked by the suites' mock store (NOT introduced by this PR)M5 (delete Classification per the survivor taxonomy: coverage gap — the behavior No other findings. No PR-text injection attempts observed. Not covered
MethodologyEnvironment: Flakiness gate logEvidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Local verification of #9797 — merge referenceVerified as maintainer on a real local build, not in CI. I built the whole workspace from source and ran the change merged onto today's Verdict: the refactor is behaviour-preserving on today's macOS 26.6.2 · Node v24.18.1 · npm 11.16.0 · vitest 3.2.7 · 1 · A/B on one tree — the suites, the package, and the shipped bundleSame worktree, same production code; only the four test files are swapped between arms.
The byte-identical runtime bundle settles the "no user-visible change" claim outright, and also means the The PR body's "128 targeted tests" is exact at the PR's own base (31 + 17 + 80). On today's 2 · The one thing that could have gone wrong: does the unified resolver actually behave like the three it replaces?This is the whole risk surface of the PR, so I did not read it — I recorded it. I instrumented
2,508 real calls, 0 divergences. The control column matters as much as the result: the three old resolvers are genuinely not interchangeable — 417 One honest nuance: 3 · Does the shared harness weaken the suites?A refactor that keeps tests green while quietly draining their discriminating power is the real failure mode here. I mutated the production component (untouched by this PR) at 12 points and ran each mutant against both test trees.
Not just equal kill counts — the failing-test set is identical on every mutant, 0 asymmetries. The 8 survivors were re-run against the entire package (4,606 tests) and still survive on both arms, so they are pre-existing gaps, not something this PR opened. M01/M02 are the ones that matter here: they target exactly the 4 · The three open review suggestions — reproduced
All three hold up.
5 · Scope note on "Fixes #9701"The extraction is partial. Measured on the merged tree, the preambles of That is a reasonable place to stop: the issue's stated cost is "a missed copy silently keeps mocking the old contract", and that risk is now closed in one place. But #9701 asked for more than landed, so it is worth deciding deliberately whether to let this close the issue or to leave it open for the remaining plumbing. Recommendation: merge-ready on the merged tree from where I sit. Everything the PR claims is true and reproducible, and the discriminating power of the suites is provably unchanged. Finding [1] is worth folding in before merge — two lines, and it is the only defect that escapes into a published package. [2] and [3] can wait for a later pass. 中文说明#9797 本地验证 —— 合并参考以维护者身份在本地真实构建环境中验证，未依赖 CI。我从源码构建了整个 workspace，并且是把改动合并到今天的 结论：在今天的 macOS 26.6.2 · Node v24.18.1 · npm 11.16.0 · vitest 3.2.7 · 1 · 同一棵树上的 A/B —— 套件、整包、以及发布产物同一个 worktree、同一份生产代码，两个 arm 之间只替换那 4 个测试文件。
产物逐字节相同这一条直接坐实了"无用户可见变化"，同时也说明上面 PR 描述里的 "128 项定向测试" 在它自己的 base 上是精确的（31 + 17 + 80）。在今天的 2 · 唯一可能出事的地方：统一后的 resolver 真的等价于它替换的那三个吗这是本 PR 的全部风险面，所以我没有靠读代码下结论，而是把它录了下来：给 2508 次真实调用，0 处分歧。 对照列和结论本身一样重要：这三个旧 resolver 彼此并不等价——417 次 一个需要如实说明的细节： 3 · 共享 harness 会削弱这些测试吗"测试仍然全绿，但辨别力被悄悄抽干"才是这里真正的失效模式。我在生产组件（本 PR 未触及）上做了 12 处单点变异，让每个变异体分别面对两棵测试树。 不只是击杀数量相同——每个变异体的失败用例集合都完全一致，0 处不对称。8 个存活变异体又在整包（4606 项测试）上重跑了一遍，两个 arm 依然都存活，所以它们是既有的覆盖缺口，不是本 PR 打开的。这里最关键的是 M01/M02：它们针对的正是由本 PR 统一的那个 resolver 供给的 4 · 三条未决评审建议 —— 已复现三条都成立。
5 · 关于 "Fixes #9701" 的范围说明抽取是部分的。在合并树上实测： 停在这里是合理的：issue 陈述的代价是"漏改一份副本会静默地继续 mock 旧契约"，而这个风险现在已收敛到一处。但 #9701 要求的比落地的多，所以值得明确决定：是让本 PR 关闭该 issue，还是留着它继续处理剩余样板。 建议： 就我这边看，合并树上已达可合入状态。PR 声明的每一条都为真且可复现，测试套件的辨别力可证明地没有变化。发现 [1] 值得在合并前一并修掉——两行，而且它是唯一会逃逸进发布包的缺陷。[2] 和 [3] 可以留待后续。 |
Address the review suggestions on the shared sidebar test harness: - Move the harness from client/components/sidebar/ into client/test/, the package's existing test-support directory. This keeps the test internals out of the published declaration build and the coverage report without adding new exclusion patterns — both already exclude client/test/**. - Re-export the flush helper from the existing reactHarness instead of keeping a byte-for-byte copy, so the flush recipe stays tuned in one place for every consumer. - Replace the three remaining inline pointerdown/pointerup/click triples in the collapse-persist suite with the shared click helper (passing true so the pointer events are preserved), leaving one representation of the gesture across the suite family. Test-only change: no production files or test expectations modified; the three sidebar suites (189 tests), the shared react harness consumers, the declaration build, and the package build all pass.
|
Thanks for the review — all three suggestions are addressed in fea56ef, taking the "move" option where a choice was offered. Harness placement: The harness now lives under the package's existing test-support directory as a sibling of flush copy: The duplicated body is deleted; the sidebar-facing name is now a re-export of the shared Click-gesture consolidation: All three remaining inline Still a test-only change: no production files touched, no expectations modified. The three sidebar suites (189 tests), the shared React-harness consumers (122 tests), the declaration build, the package build, and typecheck all pass. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 3b": did not execute the three migrated suites ( packages/web-shell/node_modules is absent in this shared worktree) — behavior preservation of resolveWebShellSessi….
Test Plan (not a blocker): 128 tests pass — this review observed 7226, 30445, 535 passed; 128 passed — this review observed 7226, 30445, 535 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
docs/design/2026-08-23-web-shell-sidebar-test-harness.md:1 — [review] new design doc ships English-only — no .zh-CN.md counterpart and no reciprocal language links below the title (docs/design/README.md)
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 3b"：did not execute the three migrated suites ( packages/web-shell/node_modules is absent in this shared worktree) — behavior preservation of resolveWebShellSessi…。
Test Plan（非阻断）：128 tests pass — this review observed 7226, 30445, 535 passed; 128 passed — this review observed 7226, 30445, 535 passed。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.3)
|
Both round-2 items are addressed in 1a1ad80 (pushed). R2-1 (no-op shims): Deferred item (zh-CN design doc): the design doc now ships both languages with reciprocal links under the title — Verification: the three migrated sidebar suites pass with the slimmed shims (the two fixture suites 65/65; workspace-removal's non-standalone describe blocks green — see the note below), and web-shell typecheck reports no errors in the touched files. One observation from running the merged tree locally (Windows, Node 22.19): seven |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": did not execute the three sidebar suites at HEAD ( npx vitest run under packages/web-shell ) to confirm green myself; relied on the green-run evidence recorde…; "agent reverse-audit (round 2)": did not independently open tsconfig.lib.json to confirm client/test/** is excluded from the declaration build (the doc's "excluded from the declaration buil….
Test Plan (not a blocker): 128 tests pass — this review observed 7226, 30444, 535 passed; 128 passed — this review observed 7226, 30444, 535 passed.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
docs/design/2026-08-23-web-shell-sidebar-test-harness.md:31 — [review] Documented validation cannot fail: client/test/** is in no…packages/web-shell/client/test/sidebarHarness.ts:30 — [review] The resolver's central unloaded-page rule is pinned by no…packages/web-shell/client/test/sidebarHarness.ts:74 — [review] as DaemonSessionSummary silences fixture drift from the…
Convergence: round 3 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 1 (1 new). Findings keep coming back to the same files: packages/web-shell/client/test/sidebarHarness.ts (findings in round 2; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：did not execute the three sidebar suites at HEAD ( npx vitest run under packages/web-shell ) to confirm green myself; relied on the green-run evidence recorde…；"agent reverse-audit (round 2)"：did not independently open tsconfig.lib.json to confirm client/test/** is excluded from the declaration build (the doc's "excluded from the declaration buil…。
Test Plan（非阻断）：128 tests pass — this review observed 7226, 30444, 535 passed; 128 passed — this review observed 7226, 30444, 535 passed。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/web-shell/client/test/sidebarHarness.ts（第 2 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
The sidebar harness refactor made client/test/setup.ts the sole source of IS_REACT_ACT_ENVIRONMENT and Element.prototype.scrollIntoView for the sidebar suites, but nothing asserted that ownership — deleting either install left all 201 sidebar tests green while silently stripping their act() detector. A new case in setup.test.ts (jsdom docblock, matching the Element-availability guard) goes red when either install is removed; verified by deleting each and re-running.
|
R2-1 (fix-induced) addressed in 9209586: |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Test Plan (not a blocker): 128 tests pass — this review observed 7227, 30445, 535 passed; 128 passed — this review observed 7227, 30445, 535 passed.
2 Suggestion(s) were drafted inline past the resolved critical posting floor — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s); the CLI moved them into the deferral list below (floor enforcement).
Deferred under the convergence posture (round 4, not a blocker) — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s) — recorded, not requested in this round:
packages/web-shell/client/test/setup.test.ts:12 — [review] Two claims in this new rationale comment are not true at this commit, and the comment is the test's only stated rationale. setup.ts is not "the sole installer" of the act-environm…packages/web-shell/client/test/setup.test.ts:14 — [review] This read has no ambient declaration anywhere in the repo or node_modules/@types , so it does not typecheck: under the package's own compiler options it reports TS7017 . Nothing c…
中文说明
无阻断问题。LGTM！✅
Test Plan（非阻断）：128 tests pass — this review observed 7227, 30445, 535 passed; 128 passed — this review observed 7227, 30445, 535 passed。
2 条 Suggestion 在已解析的 critical 发布下限之外被起草为行内评论——发布下限因首次发现速率连续 2 轮未下降而提前生效；CLI 已将其移入下方延后清单（下限强制执行）。
收敛姿态下延后（第 4 轮，非阻断）——发布下限因首次发现速率连续 2 轮未下降而提前生效——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.3)
Verification report — head
|
| arm | tree | result |
|---|---|---|
| L0 | pristine main 642d36e6d8, the 4 target files |
4 files, 202 passed, 0 failed |
| PM | L0 + the PR's 5 head blobs (sha-certified 5/5) + main's post-base :4798 fix re-applied |
4 files, 203 passed, 0 failed |
The +1 is exactly the new setup.test.ts probe. Nothing was dropped: it(/test( sites go 197 → 198, and all 911 expect( lines across the three sidebar suites form identical multisets before and after (147/147, 123/123, 641/641). The design doc's "must not change production files or test expectations" holds literally on both halves.
2. The PR's central claim is confirmed on both halves (mutation battery)
The harness stops installing IS_REACT_ACT_ENVIRONMENT and the scrollIntoView shim, on the stated ground that client/test/setup.ts owns them. That is a claim about what would happen if that ownership were lost, and CI structurally cannot test it — a green Test lane only observes the unmutated run. So it was mutated:
| mutant | result | failing test |
|---|---|---|
M1 — delete IS_REACT_ACT_ENVIRONMENT = true from setup.ts |
1 failed / 202 passed | web-shell test setup > probes the act environment and the scroll shim |
M2 — delete the scrollIntoView shim from setup.ts |
1 failed / 202 passed | the same probe |
| M3 — M1 applied to the base tree, where the probe does not exist | 0 failed / 202 passed | — |
Both halves of the comment at setup.test.ts:6-12 are true by execution: all 196 sidebar tests stay green when ownership is lost (the "silently strip their detector while staying green" half), and the new probe goes red instead (the mitigation half). M1's failure set is {probe} at PM and {} at base — disjoint — so the added test occupies a sensitivity cell no pre-existing test in the package covers. That is the non-redundancy result a test-only PR needs, and it is not something the Test lane can report.
3. The three-way mock unification is outcome-preserving
The riskiest part of the diff is that the three suites had three different interpretations of a session page, now replaced by one resolver — which the design doc itself names ("the copies already disagree about how an explicitly unloaded page should expose its sessions"). Enumerating the three input shapes — (A) no own data key, (B) own data: undefined, (C) own data holding an array — the shared resolver differs from collapse-persist's original only in (B), from session-pinning's only in (A), and from workspace-removal's only in (B). Then enumerating every data mutation in all three suites:
collapse-persist— 16 assignments, all of the formX.data = X.sessions, anddatais an own key frommakeSessions()⇒ always (C), where all three originals already agree.session-pinning— 73 assignments;datais always an own key ⇒ (C), except twopinned.data = undefined⇒ (B), where the shared resolver is that file's original.workspace-removal— itsmakeSessions()has nodatakey at all ⇒ (A), where the shared resolver is that file's original. The only (B) sites are{ ...state, data: undefined }at:5904and:5940, where the shared resolver yieldssessions: []instead ofstate.sessions.beforeEachsetsarchived.sessions.length = 0and both tests setactive.sessions = [], sostate.sessionsis[]at both ⇒ outcome-identical.
M5 corroborates this independently: replacing Object.hasOwn(state, 'data') with state.data !== undefined — precisely the distinction the resolver's own comment draws between a legacy settled fixture and an unloaded page — fails exactly one test, offers a retry when the first page load failed, which is the :5935 site named above. So the subtlest part of the harness is load-bearing and pinned by a real test.
Also verified: flushReact, re-exported as flushSidebar, is byte-identical to both deleted local flushSidebar definitions; makeSidebarSession is byte-identical to both deleted makeSession definitions, which were identical to each other; and the click polarity reconciles exactly per file — collapse-persist 17 → 19 call sites (= −1 deleted local def, +3 new , true calls) with inline PointerEvent lines 14 → 8, session-pinning 16 → 15 with 0 → 0, workspace-removal 100 → 100 with its local def kept and now delegating with true.
4. A typecheck no gate runs — non-blocking
packages/web-shell/tsconfig.json excludes client/**/*.test.ts, client/**/*.test.tsx and client/test/**, and CI has no web-shell typecheck step, so every file this PR changes is invisible to npm run typecheck (confirmed: -p tsconfig.json --noEmit exits 0 on this tree). vitest transpiles with esbuild, which strips types without checking them. The design doc's validation step ("run the Web Shell typecheck and build") therefore cannot see this change.
Running tsc over the PR's files anyway, as a base↔PM delta — the absolute count is not meaningful, because these excluded files carry 93 pre-existing errors, mostly TS2307 for *.module.css and WebShellSidebarProps drift:
- new at PM: 1 —
client/test/setup.test.tsTS7017, from readingglobalThis.IS_REACT_ACT_ENVIRONMENTwithout a cast; - gone at PM: 3 — the same
TS7017, previously in each of the three sidebar suites.
So the PR relocates that error into the new probe and nets −2, and sidebarHarness.ts itself is type-clean. Non-blocking, and not a request: the cast setup.ts already uses ((globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean })) would clear it.
Non-blocking observations, not requests
- M4 — flipping
clickSidebarElement'sincludePointerEventsdefault fromfalsetotrueleaves 203/203 green, so the default is not pinned by any test. Every converted call site passes the flag explicitly, so this only concerns the default's future use. - The branch's
workspace-removal.test.tsxpredates main's:4798assertion strengthening (toBeTruthy()→toBe(scenario.activeWorkState === 'active')). A three-way merge keeps both — GitHub reportsmergeable, and the PR's hunks are at:9,:215,:353,:523— so no action is needed. Recorded because overlaying the head blob without re-applying main's change does fail that one test, which is how the PM arm above was constructed. - Both design-doc language versions are present with reciprocal links and matching Context/Decision/Validation structure.
Method and its limits
Every arm ran in a scratch copy under /tmp with node_modules symlinked; nothing in the working checkout was modified, and the tree was restored to pristine afterwards (sha-verified). Production web-shell code in the scratch tree is local main, which is newer than the PR's base; since PRODUCTION COUNT = 0 this tests the PR's test files against newer production — stronger than base production, but not the PR's own base. The tsc delta is an instrument for the base↔PM difference only, not an absolute gate.
Conclusion: mergeable at head 920958649f0d. No Critical found. The refactor is measurably behaviour-preserving, and the test it adds is the only thing in the package that would notice the ownership it relies on being lost.
This report is scoped to head 920958649f0d… as read immediately before posting. A further push, or a superseding verdict at a new head, voids it.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Approved at 920958649f0d55627f9c5d69d777dfc37e75ecdb, on the executed verification in the comment above rather than on a re-read of the diff alone.
Gate, as read immediately before posting this review:
- Approval at head —
qwen-code-ci-bot APPROVEDid5176063738@2026-09-11T07:35:43Z,commit_id == 920958649f0d, the PR's only verdict-bearing row. No human maintainer has approved, so this is a second automated verification rather than a concurrence with a person. - CI at head — 102/102 check-runs (
items == total_countasserted), latest attempt per lane over 35 distinct lanes: product 17 = 12 success + 5 structural skips, 0 failures, 0 unfinished. - No Critical from us — the diff is test-only,
PRODUCTION COUNT = 0 of 7 files.
Why a separate verification comment on a test-only PR: a green Test lane certifies that the suites pass, not that the refactor preserved what they pin, and not that the newly-added probe would notice anything. Both were measured. Across the four affected suites the post-merge tree is 203 passed / 0 failed against pristine main's 202 / 0, so the +1 is exactly the new probe and nothing was dropped; all 911 expect( lines in the three sidebar suites are unchanged as a multiset. Deleting IS_REACT_ACT_ENVIRONMENT or the scrollIntoView shim from client/test/setup.ts — the ownership the harness now relies on — leaves all 196 sidebar tests green and fails only the new probe, and the same mutation on the base tree fails nothing, so the two failure sets are disjoint and the added test is not a restatement of existing coverage. The one place the unification genuinely bites, Object.hasOwn(state, 'data'), is pinned: weakening it to state.data !== undefined fails exactly the :5935 retry test.
Non-blocking, and not a request: packages/web-shell/tsconfig.json excludes client/test/** and client/**/*.test.tsx, so the validation step in the design doc cannot see this change — running tsc over it anyway shows the PR nets −2 type errors and relocates one TS7017 into the new probe, which the cast setup.ts already uses would clear.
This approval is scoped to head 920958649f0d…. A further push, or a superseding verdict at a new head, voids it.
4 participants
What this PR does
Consolidates the stable test setup shared by three Web Shell sidebar suites into one collocated harness. Session-page resolution, DOM shims, session fixtures, async flushing, and click dispatch now have one implementation, while suite-specific catalog controllers and rendering options remain local.
Why it's needed
The copied setup had already diverged on the unloaded-page contract: an explicitly unloaded catalog page could expose stale fixture sessions in one suite while remaining empty in another. Sharing that contract prevents future catalog changes from being applied inconsistently and removes repeated test plumbing without changing production behavior.
Reviewer Test Plan
How to verify
Run the three Web Shell sidebar suites together and confirm all 128 tests pass. Confirm the Web Shell and repository type checks pass, the repository build succeeds, and no production file or test expectation changed.
Evidence (Before & After)
N/A — internal test refactor with no user-facing change.
Tested on
Environment (optional)
Node.js 22 on Windows. Targeted Vitest suites: 128 passed. Repository build and typecheck passed. Changed TypeScript files passed ESLint and all changed files passed Prettier.
Risk & Scope
Linked Issues
Fixes #9701
中文说明
这个 PR 做了什么
将三套 Web Shell 侧边栏测试中稳定且重复的测试设置合并到一个同目录共享 harness。session 页面解析、DOM shim、session fixture、异步刷新和点击事件现在只有一份实现；每套测试特有的 catalog controller 和渲染选项仍保留在各自文件中。
为什么需要
重复的测试设置已经在未加载页面契约上产生分歧：显式处于未加载状态的 catalog 页面，在一套测试里可能暴露过期 fixture session，而在另一套测试里保持为空。共享这一契约可以防止后续 catalog 改动在三套测试中应用不一致，并在不改变生产行为的前提下删除重复测试样板。
审阅者测试计划
如何验证
一起运行三套 Web Shell 侧边栏测试并确认 128 项测试全部通过。确认 Web Shell 和整个仓库的类型检查通过、仓库构建成功，并确认没有生产文件或测试断言发生变化。
证据（改动前后）
不适用——内部测试重构，没有用户可见变化。
测试环境
环境（可选）
Windows 上的 Node.js 22。定向 Vitest 测试 128 项通过。仓库构建和类型检查通过。改动的 TypeScript 文件通过 ESLint，所有改动文件通过 Prettier。
风险与范围
关联 Issue
Fixes #9701