test(web-shell): fix brand test breakage from the compact-footer interaction by wenshao · Pull Request #11544 · QwenLM/qwen-code

wenshao · 2026-09-10T07:47:43Z

What this PR does

Fixes two web-shell test breakages on main: the brand sidebar tests now render at a sidebar width above the compact-foot breakpoint when they assert the version tooltip, and the workspace-session loading test no longer assumes the brand fetch's timing relative to its load assertion.

Why it's needed

Main's Test job fails on every PR right now. Two recent merges interact: the branding feature (#11244) added brand sidebar tests that assert the [title="<name> v<version>"] tooltip, but the compact-footer change (#11470) intentionally removes the version label below its 344px breakpoint and the default test width is 260px, so the tooltip is never rendered in those tests. The same branding change also made the workspace provider fetch GET /brand beside GET /capabilities on a deferred channel; the loading test's exact fetch-list assertion races with it — consistently red under CI's scheduling, consistently green on a fast local machine.

Verified broken on origin/main locally before this change, and verified green after: full web-shell suite 300 files / 7113 tests pass.

Reviewer Test Plan

How to verify

On current main, run the two files: cd packages/web-shell && npx vitest run client/components/sidebar/WebShellSidebar.brand.test.tsx fails 2 brand tests deterministically; npx vitest run (full suite, which adds scheduling pressure) fails the 6 workspace-loading variants. With this PR both are green.

Evidence (Before & After)

N/A

Tested on

OS	Status
🍏 macOS	✅
🪟 Windows	⚠️
🐧 Linux	⚠️

Environment (optional)

N/A

Risk & Scope

Main risk or tradeoff: none — test-only changes; no production behavior changes. The compact-mode hiding of the version label (the fix(web-shell): hide the sidebar version label at the compact footer breakpoint #11470 behavior) is preserved and not re-tested here.
Not validated / out of scope: whether the version tooltip should also exist in compact mode is a product question for fix(web-shell): hide the sidebar version label at the compact footer breakpoint #11470's owner, deliberately left alone.
Breaking changes / migration notes: none.

Linked Issues

Refs #11244, #11470.

中文说明

修复 main 上当前两个 web-shell 测试破坏:品牌 sidebar 测试在断言版本 tooltip 时改在 compact footer 断点以上的宽度渲染;workspace 加载测试不再假定品牌拉取与其加载断言的相对时序。

背景:#11244(品牌可配置)新增的品牌测试断言 [title="<名称> v<版本>"] tooltip,但 #11470(compact footer)在 344px 断点以下有意移除版本标签,而测试默认宽度 260px,所以 tooltip 根本不会渲染。同一品牌变更还让 workspace provider 在 GET /capabilities 旁异步拉取 GET /brand,与加载测试的精确请求列表断言产生时序竞争 —— CI 调度下必红,本地快机上必绿。

本地已在 origin/main 上复现失败,修复后 web-shell 全套件 300 文件 / 7113 测试全绿。

…raction Two test-side breakages from main's recent merges: - The brand sidebar tests assert the version tooltip, which the compact footer intentionally hides below its breakpoint; render those two cases above the breakpoint. - The workspace loading test asserts the exact fetch list, but the brand fetch rides beside capabilities on its own deferred channel, so whether it lands before the assertion is scheduling-dependent (red in CI, green on a fast machine); filter it out of the assertion, keeping the "loads exactly once" pin.

qwen-code-ci-bot · 2026-09-10T07:48:16Z

✅ Qwen Triage finished — view run. See the stage comments in this thread for the result.

✅ Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

qwen-code-ci-bot · 2026-09-10T08:01:27Z

Thanks for the PR — unbreaking main's Test job is exactly the kind of change that should jump the queue.

Template looks good ✓ — all required headings, the Tested-on table, Risk & Scope, and the Chinese <details> are all there.

Problem: real and confirmed, not theoretical. I checked the base tree rather than taking the description's word for it. packages/web-shell/client/components/sidebar/WebShellSidebar.tsx carries SIDEBAR_FOOTER_COMPACT_WIDTH = 344 against SIDEBAR_DEFAULT_WIDTH = 260, so at the default test width the footer version label — the element the [title="… v1.2.3"] assertions query — genuinely isn't rendered. And the loading test's fetch mock has no /brand route, so the deferred brand fetch falls through to its throw new Error('Unexpected request: …') guard while the assertion at line 199 pins an exact call list. Main's red job is tracked in #11534 (Test (ubuntu-latest, Node 22.x), failed in step Run tests and generate reports), so the failure is observed CI history, not a hypothesis.

Direction: aligned. Test-only, restores a signal every other PR depends on, and it deliberately leaves #11470's compact-mode behaviour alone instead of re-litigating it — the right call, and the PR says so explicitly.

One thing to settle before this goes further, though: #11542 is already open against the exact same two files, and it's green. It's the autofix lane's PR for #11534 — Lint & Static success, Test (ubuntu-latest, Node 22.x) success. Both PRs edit the same lines (expect(calls).toEqual(['GET /capabilities']) and the two brand tooltip tests), so only one of them can merge as-is; the second will conflict. @wenshao you're better placed than the gate to decide which one lands — I've reviewed both below and given my read in the code review comment.

Size: not applicable. Both changed files are *.test.tsx (+14/−2), so production logic lines = 0 and no core path is touched.

Approach: the sidebar half is surgical and matches how the sibling WebShellSidebar.footer-version.test.tsx already handles this breakpoint. The loading half I'd question — it filters GET /brand out of the assertion instead of giving the mock a /brand route, which leaves the underlying nondeterminism in place rather than removing it. Detail in the review comment.

Risk: no elevated risk signals — Stage 1e matched nothing, both files are tests.

Moving on to code review, with the #11542 overlap flagged for a human call. 🔍

中文说明

感谢贡献 —— 修复 main 上 Test job 的红灯正是应该优先处理的改动。

模板完整 ✓ —— 所有必需小节、Tested-on 表格、Risk & Scope 以及中文说明都齐全。

问题：真实且已确认，不是理论性的。 我核对的是基线代码，而不是只信 PR 描述。WebShellSidebar.tsx 里 SIDEBAR_FOOTER_COMPACT_WIDTH = 344，而 SIDEBAR_DEFAULT_WIDTH = 260，所以在默认测试宽度下，footer 版本标签（也就是 [title="… v1.2.3"] 断言查询的元素）确实不会渲染。加载测试的 fetch mock 没有 /brand 路由，因此异步的品牌拉取会落到 throw new Error('Unexpected request: …') 兜底分支，而第 199 行的断言又锁死了精确的请求列表。main 的红灯已由 #11534 跟踪（Test (ubuntu-latest, Node 22.x)，在 Run tests and generate reports 步骤失败），所以这是已观测到的 CI 历史，不是假设。

方向：对齐。 纯测试改动，恢复了其他所有 PR 都依赖的信号，并且有意不去动 #11470 的 compact 模式行为 —— 这个取舍是对的，PR 里也写明了。

但有一件事需要先定：#11542 已经针对完全相同的两个文件开着，而且它是绿的。 那是 #11534 的 autofix 产出 —— Lint & Static 成功，Test (ubuntu-latest, Node 22.x) 成功。两个 PR 改的是同样的行（expect(calls).toEqual(['GET /capabilities']) 和两个品牌 tooltip 测试），所以原样只能合入其中一个，另一个必然冲突。@wenshao 这个取舍你比 gate 更合适决定 —— 两个我都看了，判断写在代码审查评论里。

规模： 不适用。两个改动文件都是 *.test.tsx（+14/−2），生产逻辑行数为 0，也未触及核心路径。

方案： sidebar 那一半改得很精准，和同级 WebShellSidebar.footer-version.test.tsx 处理该断点的既有写法一致。加载测试那一半我有疑问 —— 它把 GET /brand 从断言里过滤掉，而不是给 mock 补一个 /brand 路由，这样底层的时序不确定性还在，只是被绕过了。细节见审查评论。

风险： 无升级风险信号 —— Stage 1e 未命中，两个文件都是测试。

进入代码审查，同时把 #11542 的重叠标出来交由人工定夺。 🔍

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at ce53ba18cf7f195411d455d40ed44af67c217a30 · re-run with @qwen-code /triage}

qwen-code-ci-bot · 2026-09-10T08:03:50Z

Code review

Before reading the diff I wrote down what I'd do myself, from the two failure mechanisms alone: for the sidebar, mount above the 344px footer breakpoint in the tests that query the version tooltip, the way the sibling footer-version suite already does; for the loading test, give the fetch mock a /brand route so the deferred brand fetch settles deterministically — a 404 being the natural choice, since production treats 404 as the definitive "no brand here" answer and skips its retry. That baseline matters below, because this PR matches it on one file and diverges on the other.

WebShellSidebar.brand.test.tsx — correct, and I'd change nothing. I verified each link in the chain rather than assuming it: the literal qwen-code-web-shell-sidebar-width matches SIDEBAR_WIDTH_STORAGE_KEY in the component (that constant isn't exported, and both WebShellSidebar.footer-version.test.tsx and WebShellSidebar.collapse-persist.test.tsx already hardcode the same literal, so this follows the file-local convention instead of inventing a new one). 400 clears SIDEBAR_FOOTER_COMPACT_WIDTH = 344 and sits inside [SIDEBAR_MIN_WIDTH 220, SIDEBAR_MAX_WIDTH 420], so it isn't clamped back under the breakpoint. The sidebar reads its persisted width on mount and the setItem precedes renderSidebar, so it takes effect. The file's beforeEach calls window.localStorage.clear(), so setting the width inside two tests can't leak into the other ten. And exactly two tests in the file query [title="… v1.2.3"] — lines 203 and 211 — which are precisely the two this patches, so the fix is complete for this file rather than partial. Setting it per-test instead of in beforeEach is also the more surgical of the two options: it changes the render width only where an assertion depends on it.

WorkspaceSessionProvider.loading.test.tsx — the filter holds, but it treats the symptom. The assertion itself is sound: calls.push happens before the mock's pathname dispatch, so GET /brand is recorded whenever the fetch is issued, and filtering it out makes the expectation robust to brand landing before capabilities, after it, once, or twice under StrictMode — all four collapse to ['GET /capabilities']. It still pins what the test exists to pin, that the load fetched capabilities exactly once and issued no other unexpected request.

What it stops doing is pinning brand at all, and the part I'd actually change is one level down: the mock still has no /brand route, so every one of the six it.each variants falls through to throw new Error('Unexpected request: /brand'). In DaemonWorkspaceProvider, only a DaemonHttpError with status 404 settles the fetch — anything else is treated as unknown-but-retryable — so that thrown error arms the real BRAND_RETRY_DELAY_MS = 2000 timer and drives the provider down its retry path on every variant. It's bounded: invalidateBrandFetch clears the timer and the effect cleanup calls it, so nothing survives the unmount at line 255. But inside a single test the timer is live, and if a variant takes longer than two seconds to reach its unmount — not a stretch on the contended runners where this test was already flaky, and the whole reason the assertion started racing — the retry fires mid-test, pushes a second GET /brand into calls, and logs [web-shell] brand could not be fetched after a retry. The filter tolerates that outcome; a four-line /brand → 404 handler removes it at the source and exercises the path production's own comment calls the definitive answer. That is the trade worth naming: this version is more robust to timing and asserts less, the mocking version is deterministic and asserts more.

On the overlap with #11542, which is the reason I'm not approving: that PR makes the mocking fix — /brand returns 404, and the assertion pins the exact list including StrictMode's double brand fetch — and it applies the width in the file's beforeEach at 360 rather than per-test at 400. Its Lint & Static and Test (ubuntu-latest, Node 22.x) are both green. Neither PR is wrong, but they edit the same lines, so this is a "which one lands" decision, not a "fix this and merge" one. If this PR is the one that lands, I'd suggest taking the /brand handler from #11542 and keeping this PR's per-test width, which is the tighter of the two.

Test evidence

This is an unattended CI run, so per the gate's rules I did not build or execute anything from this branch — the evidence below is this PR's own CI, read through the API on the reviewed commit. Test (ubuntu-latest, Node 22.x) is the one check that can actually prove the fix, and it was still in progress at review time; I'm reporting it as pending rather than guessing at it.

Final CI results for ce53ba1 (auto-updated by the triage finalize job after CI completed):

Check	Conclusion
`Lint & Static (ubuntu-latest, Node 22.x)`	❌ failure
`Capture web-shell visuals (ubuntu-latest, Node 22.x)`	✅ success
`Classify PR`	✅ success
`Desktop Shell (ubuntu-22.04)`	✅ success
`Desktop Shell (windows-2022)`	✅ success
`Integration Tests (no-AK, No Sandbox)`	✅ success
`Test (ubuntu-latest, Node 22.x)`	✅ success
`web-shell E2E Smoke (ubuntu-latest, Node 22.x)`	✅ success

_{One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。}

The Lint & Static red is not a lint error in this diff, and it is not pre-existing infra noise either — it's branch staleness, and it's specific to this PR. The failing step is .github/scripts/check-lint-gate-freshness.mjs, not ESLint or Prettier; the job never reached a lint rule. Its output:

The lint gate changed on 'main' after this branch last incorporated it:

  - .github/workflows/ci.yml: 17990c330da8 fix(desktop): realign the release test with the new signing step (#11522) (2026-09-10)

This lane checks out the branch head alone, so its green proves the branch
passes the gate AS THE BRANCH DEFINES IT — with the files above, that gate
is stale. Merge or rebase 'main' into this branch and push to
re-validate under the current gate.

So the remedy is a merge or rebase of main and a push, not a code change. Worth doing regardless of the #11542 decision, because Test (ubuntu-latest) also only runs against this branch's own stale view of the gate until then. For comparison, #11542's head is green on Lint & Static, which is consistent with that branch being current.

Not verified: that the two suites are actually green on CI. The check that would show it hadn't reported when this review ran. The description's "full web-shell suite 300 files / 7113 tests pass" and the before/after reproduction on origin/main are the author's local results on macOS — I'm citing them as the author's claim, not as evidence, and the Tested-on table marks Windows and Linux as not tested.

Sandboxed verification would settle the part CI can't: @qwen-code /verify — a green Test (ubuntu-latest) run shows these two files pass once, but the claim this PR actually makes is that it removes a scheduling-dependent race ("consistently red under CI's scheduling, consistently green on a fast local machine"), and one green run cannot distinguish a fixed race from a race that didn't fire. An A/B against the base build would show the two files failing without the diff and passing with it, and repeated runs would show whether the loading test is stable now that its brand fetch still arms a live 2s retry timer per variant. /tmux is not the lane here — there's no TUI surface, these are web-shell unit tests.

中文说明

代码审查

在读 diff 之前，我先只根据两个失败机制写下了自己的方案：sidebar 那边，在需要查询版本 tooltip 的测试里以高于 344px footer 断点的宽度挂载，就像同级的 footer-version 测试套件已有的做法；加载测试那边，给 fetch mock 补一个 /brand 路由，让异步的品牌拉取确定性落地 —— 选 404 最自然，因为生产代码把 404 当作"确定没有品牌"的答案并跳过重试。这个基线在下面很关键，因为本 PR 在一个文件上与它一致，在另一个文件上不一致。

WebShellSidebar.brand.test.tsx —— 正确，我不会改任何东西。 链条上每一环我都核对过，没有靠假设：字面量 qwen-code-web-shell-sidebar-width 与组件里的 SIDEBAR_WIDTH_STORAGE_KEY 一致（该常量没有导出，而 WebShellSidebar.footer-version.test.tsx 和 WebShellSidebar.collapse-persist.test.tsx 都已经硬编码同一个字面量，所以这是沿用文件内的既有约定，而不是新造一套）。400 高于 SIDEBAR_FOOTER_COMPACT_WIDTH = 344，且落在 [SIDEBAR_MIN_WIDTH 220, SIDEBAR_MAX_WIDTH 420] 区间内，因此不会被夹回断点以下。sidebar 在挂载时读取持久化宽度，而 setItem 在 renderSidebar 之前，所以设置会生效。文件的 beforeEach 调用了 window.localStorage.clear()，因此在两个测试内设置宽度不会泄漏到其余十个测试。而文件中恰好只有两个测试查询 [title="… v1.2.3"] —— 第 203 行和第 211 行 —— 正是本 PR 修改的这两个，所以对这个文件而言修复是完整的而非部分的。按测试单独设置而不是放进 beforeEach，也是两个选项中更精准的那个：只在断言真正依赖宽度的地方改变渲染宽度。

WorkspaceSessionProvider.loading.test.tsx —— 过滤能站得住，但处理的是症状。 断言本身没问题：calls.push 发生在 mock 的 pathname 分派之前，所以品牌拉取一旦发出就会记录 GET /brand，把它过滤掉之后，无论品牌请求落在 capabilities 之前、之后、一次、还是 StrictMode 下两次，四种情况都收敛为 ['GET /capabilities']。它仍然锁住了这个测试存在的目的 —— 加载过程恰好拉取一次 capabilities，且没有发出其他意外请求。

它不再做的是对 brand 的任何约束。而我真正想改的地方在下一层：mock 仍然没有 /brand 路由，所以六个 it.each 变体每一个都会落到 throw new Error('Unexpected request: /brand')。在 DaemonWorkspaceProvider 中，只有状态码为 404 的 DaemonHttpError 才会让拉取落定 —— 其他一切都按"未知但可重试"处理 —— 因此这个抛出的错误会启动真实的 BRAND_RETRY_DELAY_MS = 2000 定时器，让每个变体都走上重试路径。影响是有界的：invalidateBrandFetch 会清掉定时器，effect 的 cleanup 会调用它，所以不会有东西活过第 255 行的 unmount。但在单个测试内部这个定时器是活的，如果某个变体超过两秒才走到 unmount —— 在这个测试本来就不稳定的高负载 runner 上并不夸张，而这正是断言开始竞态的原因 —— 重试就会在测试中途触发，往 calls 里推入第二个 GET /brand，并打出 [web-shell] brand could not be fetched after a retry 日志。过滤能容忍这个结果；而四行的 /brand → 404 处理则从源头消除它，并且走的是生产代码注释里称之为"确定性答案"的那条路径。这就是值得点明的取舍：本版本对时序更鲁棒但断言更少，mock 版本确定性更强且断言更多。

关于与 #11542 的重叠，这也是我不批准的原因：那个 PR 做的正是 mock 方案 —— /brand 返回 404，断言锁死包含 StrictMode 双次品牌拉取的精确列表 —— 并且把宽度设在文件的 beforeEach 里、值为 360，而不是按测试设为 400。它的 Lint & Static 和 Test (ubuntu-latest, Node 22.x) 都是绿的。两个 PR 都没有错，但它们改的是同样的行，所以这是"合哪一个"的决定，而不是"改完就能合"的决定。如果最终合入的是本 PR，我建议从 #11542 取 /brand 处理，同时保留本 PR 按测试设置宽度的写法 —— 后者是两者中更紧凑的。

测试证据

这是无人值守的 CI 运行，因此按 gate 规则我没有构建或执行本分支的任何代码 —— 下面的证据是通过 API 读取的、本 PR 自己在被审查 commit 上的 CI 结果。Test (ubuntu-latest, Node 22.x) 是唯一能真正证明修复的检查，而它在审查时仍在运行；我如实报告为 pending，不去猜测结果。

Lint & Static 的红灯不是本 diff 的 lint 错误，但也不是既有的基础设施噪音 —— 它是分支过期，而且是本 PR 特有的。失败的步骤是 .github/scripts/check-lint-gate-freshness.mjs，不是 ESLint 或 Prettier；这个 job 根本没走到 lint 规则。它的输出说明 main 上的 .github/workflows/ci.yml 在本分支最后一次同步之后发生了变化（17990c330da8，#11522），因此需要把 main merge 或 rebase 进来再推一次。无论 #11542 的取舍如何，这一步都值得做，因为在那之前 Test (ubuntu-latest) 也只是针对本分支自己那份过期的 gate 视图在跑。作为对照，#11542 的 head 在 Lint & Static 上是绿的，这与那个分支是最新的相吻合。

未验证：这两个套件在 CI 上是否真的绿。 能说明这一点的检查在本次审查运行时还没有出结果。描述里的"web-shell 全套件 300 文件 / 7113 测试通过"以及在 origin/main 上的 before/after 复现，是作者在 macOS 上的本地结果 —— 我把它作为作者的主张引用，而不是证据，且 Tested-on 表格把 Windows 和 Linux 标为未测试。

沙箱验证可以补上 CI 补不了的那部分：@qwen-code /verify —— Test (ubuntu-latest) 变绿只能说明这两个文件通过了一次，但本 PR 真正的主张是它消除了一个依赖调度的竞态（"CI 调度下必红，本地快机上必绿"），而一次绿灯无法区分"竞态已修复"和"竞态这次没触发"。与基线构建做 A/B 能显示这两个文件在没有 diff 时失败、有 diff 时通过；重复运行则能显示，在每个变体仍会启动一个 2 秒重试定时器的前提下，加载测试现在是否稳定。这里 /tmux 不是合适的通道 —— 没有 TUI 界面，这些是 web-shell 单元测试。

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at ce53ba18cf7f195411d455d40ed44af67c217a30 · re-run with @qwen-code /triage}

qwen-code-ci-bot · 2026-09-10T08:06:00Z

Confidence: 3/5 — the sidebar fix is correct and I'd merge it as written; the loading-test fix trades assertion strength for timing robustness, and #11542 already fixes both files and is green, so which one lands is a human call rather than a gate call.

⏸️ Deferring — not approving, not requesting changes.

Stepping back: the problem is real and I confirmed it independently rather than accepting the framing — #11534 tracks main's red Test (ubuntu-latest, Node 22.x), and both mechanisms are visible in the base tree, the 344px compact breakpoint against the 260px default test width, and a /brand fetch with no matching mock route. The diff is minimal, two files, no drive-by edits, no scope creep, and the comments explain the why instead of narrating the what. Unbreaking the suite every other PR depends on is worth doing promptly. This is not a PR I'm deferring because I found something wrong with it.

Two things keep me from approving.

The first is arithmetic, not judgement: #11542 edits the same lines in the same two files and its Lint & Static and Test (ubuntu-latest, Node 22.x) are both green. Two open PRs, one mergeable. Picking between them means weighing this PR's per-test width against that one's beforeEach width, and this PR's filtered assertion against that one's mocked /brand route plus exact call list — a maintainer's call about which signal the suite should carry going forward, not something a gate should settle by merging whichever review finished first.

The second is that the loading-test half leaves the nondeterminism in place. Filtering GET /brand out of the assertion makes the expectation robust to the race, which is a legitimate fix, but the mock still throws on /brand, so all six variants arm the provider's real two-second retry timer and travel its retryable-failure path. It's bounded — the effect cleanup clears the timer — but a slow variant can still fire that retry mid-test and emit a stray warning. My own proposal for this file was to give the mock a 404 for /brand, because that's the one outcome production treats as definitive; #11542 does exactly that. If this PR is the one that lands, taking that four-line handler and keeping this PR's tighter per-test width would, I think, be the best of both.

Also blocking in the mundane sense: Lint & Static is red on the branch-staleness gate, so a merge or rebase of main is needed before this can go in regardless of the above.

What would move me to approve: a decision that this is the PR that lands (and #11542 closes), the rebase that clears the freshness gate, and Test (ubuntu-latest, Node 22.x) green on the new head. If the loading-test approach stays as a filter, I'd want that to be a deliberate choice on the record rather than an accident of which PR got reviewed first — the assertion currently doesn't pin brand at all, and six months from now that's the kind of gap someone re-discovers the hard way.

On the escalation itself: I tried to resolve an accountable owner deterministically and there isn't one to hand — QWEN_MAINTAINER_HANDLE is unset, the PR carries no labels so no area in the ownership map matches, no area claims the packages/web-shell/ path prefix, and there are no prior human reviews to fall back to. Rather than guess a login I'm putting this in front of @wenshao as the author, who has admin on the repo and is the right person to arbitrate against the autofix lane's #11542.

中文说明

Confidence: 3/5 —— sidebar 的修复是正确的，照原样我就会合；加载测试的修复用断言强度换了时序鲁棒性，而 #11542 已经修了同样两个文件并且是绿的，所以合哪个是人工决定，不是 gate 能定的。

⏸️ 暂缓 —— 不批准，也不 request changes。

退一步看：问题是真实的，而且我是独立核实的，没有直接接受 PR 的叙述 —— #11534 跟踪着 main 上红灯的 Test (ubuntu-latest, Node 22.x)，两个机制在基线代码里都能看到：344px 的 compact 断点对上 260px 的默认测试宽度，以及一个没有对应 mock 路由的 /brand 拉取。diff 很小，两个文件，没有夹带改动，没有范围蔓延，注释解释的是"为什么"而不是复述"做了什么"。恢复其他所有 PR 都依赖的测试套件值得尽快做。我暂缓这个 PR，不是因为它有什么问题。

有两点让我不批准。

第一点是算术问题，不是判断问题：#11542 改的是同样两个文件里同样的行，而它的 Lint & Static 和 Test (ubuntu-latest, Node 22.x) 都是绿的。两个开着的 PR，只能合一个。在它们之间取舍，意味着要权衡本 PR 按测试设置宽度与那个 PR 在 beforeEach 里设置宽度、本 PR 的过滤断言与那个 PR 的 mock /brand 路由加精确调用列表 —— 这是维护者关于"测试套件往后该承载哪种信号"的决定，不该由 gate 按"谁的审查先跑完"来定。

第二点是加载测试那一半把不确定性留在了原地。把 GET /brand 从断言里过滤掉确实让预期对竞态鲁棒，这是一个合理的修法，但 mock 仍然会在 /brand 上抛错，所以六个变体全都会启动生产代码里那个真实的两秒重试定时器，并走它的可重试失败路径。影响是有界的 —— effect cleanup 会清掉定时器 —— 但一个跑得慢的变体仍可能在测试中途触发那次重试并打出多余告警。我自己对这个文件的方案是给 mock 的 /brand 返回 404，因为那是生产代码唯一当作确定性结果处理的情形；#11542 做的正是这件事。如果最终合入的是本 PR，我认为取那个四行处理、同时保留本 PR 更紧凑的按测试设宽度，会是两者中最好的组合。

还有一个通俗意义上的阻塞：Lint & Static 因分支过期而红灯，所以无论上面怎么定，都需要先 merge 或 rebase main 才能合入。

能让我转为批准的条件：确定由本 PR 落地（并关掉 #11542）、清掉过期 gate 的 rebase，以及新 head 上 Test (ubuntu-latest, Node 22.x) 变绿。如果加载测试最终仍采用过滤方案，我希望那是一个记录在案的明确选择，而不是"哪个 PR 先被审查"的偶然结果 —— 当前断言对 brand 完全没有约束，六个月后这类缺口往往会被人以比较难受的方式重新发现。

关于升级路径：我尝试确定性地解析出一个责任人，但手头没有 —— QWEN_MAINTAINER_HANDLE 未设置，PR 没有标签因此所有权映射里没有 area 命中，没有 area 声明 packages/web-shell/ 路径前缀，也没有既往的人工 review 可作兜底。与其猜一个 login，我把这件事交给作者 @wenshao —— 他在本仓库有 admin 权限，也是与 autofix 通道的 #11542 做取舍的合适人选。

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at ce53ba18cf7f195411d455d40ed44af67c217a30 · re-run with @qwen-code /triage}

qwen-code-ci-bot · 2026-09-10T08:06:42Z

🖼️ web-shell visual preview

Rendered against a mock daemon (no real backend): the PR base vs this PR head ce53ba1. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

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-ci-bot · 2026-09-10T10:09:01Z

Qwen Code review timed out. Qwen review timed out after 5400 seconds (of the 90-minute budget). For large PRs, retry with a longer timeout by commenting: @qwen-code /review --timeout=360. See workflow logs.

wenshao · 2026-09-10T10:50:27Z

Local verification report — PR #11544

I built a real three-arm environment on Linux and ran the two suites this PR touches, plus instrumented and soak probes. The breakage this PR describes is real and this PR does fix it — but the same breakage was already fixed on main by #11530, which merged 3.5 minutes after this PR opened. My recommendation is to close this PR as superseded, and I've included the evidence for the one place where the two fixes genuinely disagree so the choice is an informed one rather than a "whoever landed first" one.

Harness

	commit	what it is
A	`ecae037176`	this PR's merge-base — also the commit in main-CI-failure issue #11528
B	`ce53ba18cf`	this PR's head
C	`424e40cc1f`	current `origin/main` (contains #11530 = `2488d12d8b`)

Separate git worktrees, real npm install from the shared lockfile (identical across all three arms), real npm run build for the core → acp-bridge → sdk chain that @qwen-code/sdk/daemon resolves to. fetchBrand() in DaemonWorkspaceProvider.tsx is byte-identical between arms B and C (md5 fb602a58…), so the arms differ only in the test fixture — the comparison below is apples-to-apples.

1. The problem is real, and this PR fixes it

Arm A fails exactly as described — 6 loading variants on the extra GET /brand, 2 sidebar tooltip assertions on expected null not to be null. Arm B is 16/16. No complaint about the diagnosis or the fix.

2. …but `main` is already green

Arm C (current origin/main) passes both suites 16/16, and the full web-shell package passes 302 files / 7150 tests.
Every open Main CI failed issue — Main CI failed: Qwen Code CI on ecae0371767e #11528, Main CI failed: Qwen Code CI on 53964ef7e272 #11534, Main CI failed: Qwen Code CI on d8baa8730fc6 #11535, Main CI failed: Qwen Code CI on b1ac3e297023 #11539, Main CI failed: Qwen Code CI on 1097b9fe0af3 #11543, Main CI failed: Qwen Code CI on b3d022b35724 #11546 — is on a commit that predates 2488d12d8b. No failure has been recorded on any commit that contains the fix.
The red Lint & Static on this PR is not a lint error: it's the lint-gate freshness check reporting that .github/workflows/ci.yml moved on main (17990c330da8) after this branch last incorporated it.

This wasn't a careless duplicate — this PR opened at 07:47:43Z and #11530 merged at 07:51:15Z. It's a race, and it lost by four minutes.

3. The loading-test rationale doesn't survive measurement

This is the one substantive disagreement between the two fixes, and it's worth resolving on evidence rather than on which merged first.

The PR body says the loading assertion is "consistently red under CI's scheduling, consistently green on a fast local machine", and changes the pin to calls.filter(c => c !== 'GET /brand') on that basis. Measured:

On the merge-base, unpressured, on a 16-core host: 20/20 runs fail, all 6 variants, every time. The brand fetch is issued before the assertion 100% of the time. The original pin wasn't racing — it was simply stale.
With vitest pinned to 2 cores against 6 busy-loop hogs (3.8× wall-clock slowdown, ~3.0 s → ~11.5 s per run), main's stricter pin ['GET /capabilities', 'GET /brand'] passed 25/25. The PR's filtered pin also passed 25/25.

So the looser pin buys no robustness I could measure, and it gives up the assertion that /brand is fetched at all.

4. Fixture fidelity: the two fixes are not equivalent

This PR filters GET /brand out of the assertion but never gives the mock a /brand route — so every brand fetch lands on the mock's fallthrough, throw new Error(`Unexpected request: ${url.pathname}`). Instrumenting both the mock and the provider:

	mock fallthrough throws	`brand retry ARMED`	`brand SETTLED via 404`
#11544	9	6	0
#11530 (on `main`)	0	0	6

Under this PR, all 6 loading tests drive the provider down its transport-failure branch and leave a BRAND_RETRY_DELAY_MS = 2000 timer armed, cleaned up only by the unmount. #11530's 404 fixture models an older daemon without the route — the one definitive "no brand here" answer production actually handles — and settles immediately.

In fairness: this is latent, not active. I tried to make the armed timer fire — pinned to a single core against 10 CPU hogs, the slowest loading test still ran 900 ms against the 2000 ms delay, and the retry never fired in any run. It's a fixture-fidelity difference, not a bug I can demonstrate breaking anything.

5. What merging this would actually do

WebShellSidebar.brand.test.tsx auto-merges into a double fix: main's beforeEach already sets the width to 360, and this PR's per-test setItem(..., '400') layers on top of it. Both are above the 344 px breakpoint, so it's inert duplication.
WorkspaceSessionProvider.loading.test.tsx is a hard conflict between the two assertions — resolving it means picking one, and §3–4 say the one on main is the better of the two.

Recommendation

Close #11544 as superseded by #11530. The diagnosis was right, the sidebar fix is equivalent to what landed, and the loading fix is the weaker of the two on the evidence above.

Two related cleanups while you're here:

fix(web-shell): realign two suites with shipped behavior to unbreak main CI (#11534) #11542 is in the same position and is now a strict no-op: both of its test files are byte-identical to main. It can be closed too.
Main CI failed: Qwen Code CI on 53964ef7e272 #11534 / Main CI failed: Qwen Code CI on d8baa8730fc6 #11535 / Main CI failed: Qwen Code CI on b1ac3e297023 #11539 / Main CI failed: Qwen Code CI on 1097b9fe0af3 #11543 / Main CI failed: Qwen Code CI on b3d022b35724 #11546 / Main CI failed: Qwen Code CI on ecae0371767e #11528 all carry this signature on pre-fix commits and should close once a main run on a post-2488d12d8b commit is green.

The one thing genuinely worth a second look, independent of this PR: main's pin now encodes StrictMode's remount double (['GET /capabilities', 'GET /brand', 'GET /brand']). That's more informative than the filtered version but also more coupled to React's StrictMode behavior. If that coupling bothers you, it's a two-line follow-up on main, not a reason to keep this branch alive.

Scope of this verification: Linux only (Node 22.22.2, 16 cores). Both suites are jsdom-level and platform-independent, but I did not run the macOS or Windows lanes. Figures are terminal captures of the actual runs; the harness is reproducible from the arm commits above.

中文版

PR #11544 本地验证报告

我在 Linux 上搭建了真实的三臂环境，跑了本 PR 涉及的两个 suite，并补了插桩探针与压力 soak。本 PR 描述的破坏真实存在，本 PR 也确实修好了它 —— 但同样的破坏已经被 #11530 在 main 上修复，而 #11530 在本 PR 开启后 3.5 分钟就合入了。 我的建议是以「已被取代」关闭本 PR；同时我把两种修法真正分歧的那一处证据也列了出来，让这个取舍基于证据，而不是"谁先合入"。

环境

	commit	说明
A	`ecae037176`	本 PR 的 merge-base，也是 main CI 失败 issue #11528 对应的提交
B	`ce53ba18cf`	本 PR 的 head
C	`424e40cc1f`	当前 `origin/main`（已含 #11530 = `2488d12d8b`）

独立 git worktree，用共享 lockfile（三臂完全一致）真实 npm install，并真实构建 @qwen-code/sdk/daemon 所依赖的 core → acp-bridge → sdk 链。B 臂与 C 臂的 DaemonWorkspaceProvider.tsx 中 fetchBrand() 字节相同（md5 fb602a58…），两臂只有测试 fixture 不同，因此下面的对比是同条件对照。

1. 问题真实存在，本 PR 也确实修好了

A 臂的失败与描述完全一致：6 个 loading 变体因多出的 GET /brand 失败，2 个 sidebar tooltip 断言因 expected null not to be null 失败。B 臂 16/16。对诊断和修法本身没有异议。

2. 但 main 已经是绿的

C 臂（当前 origin/main）两个 suite 16/16，整个 web-shell 包 302 文件 / 7150 测试全绿。
所有仍开着的 Main CI failed issue —— Main CI failed: Qwen Code CI on ecae0371767e #11528、Main CI failed: Qwen Code CI on 53964ef7e272 #11534、Main CI failed: Qwen Code CI on d8baa8730fc6 #11535、Main CI failed: Qwen Code CI on b1ac3e297023 #11539、Main CI failed: Qwen Code CI on 1097b9fe0af3 #11543、Main CI failed: Qwen Code CI on b3d022b35724 #11546 —— 对应的提交都早于 2488d12d8b。没有任何一个失败发生在包含该修复的提交上。
本 PR 上红掉的 Lint & Static 并不是 lint 错误：它是 lint gate 新鲜度检查，报告 .github/workflows/ci.yml 在本分支上次同步 main 之后发生了变化（17990c330da8）。

这不是粗心的重复提交：本 PR 开于 07:47:43Z，#11530 合于 07:51:15Z，是一次撞车，输了四分钟。

3. loading 测试的改动理由经不起实测

这是两种修法唯一实质分歧的地方，值得用证据而不是先后顺序来定。

PR 描述称该断言"CI 调度下必红，本地快机上必绿"，并据此把断言改为 calls.filter(c => c !== 'GET /brand')。实测结果：

在 merge-base 上、无压力、16 核机器：20/20 次全部失败，6 个变体每次都失败。brand 请求 100% 在断言之前发出。原断言不是在竞态，只是过期了。
把 vitest 用 taskset 绑到 2 核并施加 6 个忙循环（3.8 倍墙钟减速，单轮 ~3.0 s → ~11.5 s）：main 上更严格的断言 ['GET /capabilities', 'GET /brand'] 25/25 全过；本 PR 的过滤式断言同样 25/25。

也就是说，放宽后的断言并未换来可测量的稳健性，反而放弃了"/brand 确实被请求过"这一断言。

4. Fixture 保真度：两种修法并不等价

本 PR 把 GET /brand 从断言中过滤掉，却没有给 mock 加 /brand 路由 —— 于是每一次 brand 请求都落到 mock 的兜底 throw new Error(`Unexpected request: ${url.pathname}`) 上。对 mock 和 provider 同时插桩后：

	mock 兜底 throw	`brand retry ARMED`	`brand SETTLED via 404`
#11544	9	6	0
#11530（main）	0	0	6

在本 PR 下，6 个 loading 测试全部把 provider 驱入传输失败分支，并留下一个 BRAND_RETRY_DELAY_MS = 2000 的计时器，只能靠 unmount 清理。#11530 的 404 fixture 模拟的是没有该路由的旧 daemon —— 也就是生产代码真正处理的那个"确实没有 brand"的确定性答案 —— 会立即结算。

但要说公道话：这是潜在问题，不是已发生的问题。 我试图让这个计时器真的触发 —— 绑到单核、加 10 个 CPU 忙循环，最慢的 loading 测试仍只有 900 ms，远低于 2000 ms 的重试延迟，任何一轮都没有触发重试。它是 fixture 保真度差异，而不是我能证明会弄坏什么的 bug。

5. 真的合入会发生什么

WebShellSidebar.brand.test.tsx 会自动合并成双重修复：main 的 beforeEach 已经把宽度设为 360，本 PR 逐测试的 setItem(..., '400') 叠在上面。两者都在 344px 断点之上，属于无效重复。
WorkspaceSessionProvider.loading.test.tsx 是硬冲突，必须二选一；而按第 3、4 节，main 上那一版更优。

建议

以「被 #11530 取代」关闭 #11544。 诊断是对的，sidebar 的修法与已落地的等价，loading 的修法按上述证据是两者中较弱的一个。

顺带两个清理：

fix(web-shell): realign two suites with shipped behavior to unbreak main CI (#11534) #11542 处境相同，且现在是严格意义上的空操作：它的两个测试文件与 main 字节相同，可以一并关闭。
Main CI failed: Qwen Code CI on 53964ef7e272 #11534 / Main CI failed: Qwen Code CI on d8baa8730fc6 #11535 / Main CI failed: Qwen Code CI on b1ac3e297023 #11539 / Main CI failed: Qwen Code CI on 1097b9fe0af3 #11543 / Main CI failed: Qwen Code CI on b3d022b35724 #11546 / Main CI failed: Qwen Code CI on ecae0371767e #11528 都是这个签名、且都在修复之前的提交上，等 2488d12d8b 之后的 main 跑绿即可关闭。

有一件事确实值得单独再看，与本 PR 无关：main 上的断言现在把 StrictMode 的重挂载双请求写死了（['GET /capabilities', 'GET /brand', 'GET /brand']）。这比过滤版本信息量更大，但也更耦合 React StrictMode 的行为。如果介意这层耦合，那是 main 上两行的后续改动，而不是保留本分支的理由。

本次验证范围： 仅 Linux（Node 22.22.2，16 核）。两个 suite 都是 jsdom 级、与平台无关，但我没有跑 macOS 与 Windows 通道。图为实际运行的终端截图，环境可按上表的三个 commit 复现。

wenshao · 2026-09-10T10:52:12Z

Addendum — the duplicate set is larger than I listed.

The autofix pipeline filed one PR per red-main push while the fix was in flight, so there are three siblings of this PR, not one. I md5'd both touched test files in each against origin/main (424e40cc1f):

PR	state	`loading.test.tsx`	`brand.test.tsx`	net effect
#11542	open	identical to `main`	identical to `main`	no-op
#11557	open	identical to `main`	identical to `main`	no-op
#11549	already closed	—	—	—

Careful with the three-dot diff on these: git diff --stat origin/main...pr11557 still reports +17 −1, because their merge-base predates 2488d12d8b. The content is already on main — compare the file blobs, not the diff stat.

So: #11542 and #11557 can be closed as no-ops, and this PR (#11544) closed as superseded per the report above.

中文版

补充 —— 重复的 PR 不止一个。

修复在途期间，autofix 流水线为每一次 main 变红都开了一个 PR，因此本 PR 有三个兄弟而不是一个。我对每个 PR 的两个测试文件与 origin/main（424e40cc1f）做了 md5 比对：

PR	状态	`loading.test.tsx`	`brand.test.tsx`	净效果
#11542	open	与 `main` 相同	与 `main` 相同	空操作
#11557	open	与 `main` 相同	与 `main` 相同	空操作
#11549	已关闭	—	—	—

注意三点式 diff 在这里有陷阱：git diff --stat origin/main...pr11557 仍会报 +17 −1，因为它们的 merge-base 早于 2488d12d8b。内容其实已经在 main 上了 —— 要比对文件 blob，而不是 diff stat。

因此：#11542 与 #11557 可作为空操作关闭，本 PR（#11544）按上面的报告作为「已被取代」关闭。

wenshao closed this Sep 10, 2026

Uh oh!

test(web-shell): fix brand test breakage from the compact-footer interaction - #11544

test(web-shell): fix brand test breakage from the compact-footer interaction#11544
wenshao wants to merge 1 commit into
mainfrom
fix/web-shell-brand-tests-ci

wenshao commented Sep 10, 2026

Uh oh!

qwen-code-ci-bot commented Sep 10, 2026 •

edited

Loading

Uh oh!

qwen-code-ci-bot commented Sep 10, 2026

Uh oh!

qwen-code-ci-bot commented Sep 10, 2026 •

edited

Loading

Uh oh!

qwen-code-ci-bot commented Sep 10, 2026

Uh oh!

qwen-code-ci-bot commented Sep 10, 2026

Uh oh!

qwen-code-ci-bot commented Sep 10, 2026

Uh oh!

wenshao commented Sep 10, 2026

PR #11544 本地验证报告

环境

1. 问题真实存在，本 PR 也确实修好了

2. 但 main 已经是绿的

3. loading 测试的改动理由经不起实测

4. Fixture 保真度：两种修法并不等价

5. 真的合入会发生什么

建议

Uh oh!

wenshao commented Sep 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

wenshao commented Sep 10, 2026

What this PR does

Why it's needed

Reviewer Test Plan

How to verify

Evidence (Before & After)

Tested on

Environment (optional)

Risk & Scope

Linked Issues

Uh oh!

qwen-code-ci-bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qwen-code-ci-bot commented Sep 10, 2026

Uh oh!

qwen-code-ci-bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review

Test evidence

Uh oh!

qwen-code-ci-bot commented Sep 10, 2026

Uh oh!

qwen-code-ci-bot commented Sep 10, 2026

🖼️ web-shell visual preview

Screenshots · before / after

Uh oh!

qwen-code-ci-bot commented Sep 10, 2026

Uh oh!

wenshao commented Sep 10, 2026

Local verification report — PR #11544

Harness

1. The problem is real, and this PR fixes it

2. …but main is already green

3. The loading-test rationale doesn't survive measurement

4. Fixture fidelity: the two fixes are not equivalent

5. What merging this would actually do

Recommendation

PR #11544 本地验证报告

环境

1. 问题真实存在，本 PR 也确实修好了

2. 但 main 已经是绿的

3. loading 测试的改动理由经不起实测

4. Fixture 保真度：两种修法并不等价

5. 真的合入会发生什么

建议

Uh oh!

wenshao commented Sep 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qwen-code-ci-bot commented Sep 10, 2026 •

edited

Loading

qwen-code-ci-bot commented Sep 10, 2026 •

edited

Loading

2. …but `main` is already green