test(web-shell): prevent nested session render loop - #8934
Merged
Conversation
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. The related branch #8872 currently fails Direction: aligned — this is test-harness correctness for the app-level suite, nothing product-facing. Size: not applicable — test-only change, one file, +8/−1, zero production lines. Approach: scope feels right. Keying the mock on the Risk: no elevated risk signals — the only changed file is Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 已观测到，不是理论问题。关联分支 #8872 当前在 方向： 对齐——这是应用级测试桩的正确性修复，不涉及产品行为。 规模： 不适用——仅测试改动，单文件，+8/−1，零生产行。 方案： 范围合理。以 风险： 无升级风险信号——唯一改动的文件是 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Code review Independent proposal before reading the diff: given "the mocked What I verified while reading:
No blockers found. No user-visible or production changes. Testing — this PR's own CI (unattended run: I don't execute PR code; the signal below is the PR's CI via API, fetched once at review time) The unit suite is still running as of this review; the table updates in place once CI settles. The job to watch is Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 中文说明代码审查 先给出独立方案：面对"mock 的 阅读时核实的内容：
未发现阻塞问题。无用户可见或生产代码改动。 测试——本 PR 自身的 CI（无人值守运行：不执行 PR 代码；以下信号来自 PR 的 CI API，审查时一次性获取） 截至本次审查，单元测试套件仍在运行；CI 结束后表格会就地更新。需要关注的是 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — clean, minimal test-harness fix; the only reservation is that the OOM was observed on the related branch's CI (#8872) rather than Stepping back: this matches my independent proposal line for line — there is no materially simpler version of this change. The old pass-through mock was a latent lie: it let nested side-task sessions render against the parent's mocked connection, which is exactly the shared state that loops once a test resolves a nested session. Scoping the mock to the parent session makes the harness honest, and the one added comment tells the next reader why nested providers render nothing. Every line in the diff serves the stated goal; there is no drive-by churn. Nothing user-visible, no production code, no API surface — in six months this reads as a nine-line test fix, which is what it is. Approving, with CI still in flight: 中文说明信心：4/5 —— 干净的测试桩最小修复；唯一的保留意见是 OOM 是在关联分支的 CI（#8872）上观测到的，而不是 整体来看：这个 PR 与我的独立方案逐行一致——不存在更简单的等价改法。旧的透传 mock 是一个潜在的错误建模：它让嵌套 side-task 会话渲染在父会话的 mock 连接上，而一旦测试解析出嵌套会话，正是这份共享状态导致死循环。把 mock 限定到父会话让测试桩变得诚实，新增的一行注释也告诉了后来的读者嵌套 provider 为何不渲染。diff 中每一行都服务于既定目标，没有顺手改动。无用户可见改动、无生产代码、无 API 面变化——六个月后这就是一个九行的测试修复，名副其实。 准备批准，但 CI 尚未结束：审查时 — 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 changes against the PR base. 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. |
yiliang114
approved these changes
Aug 11, 2026
qwen-code-ci-bot
approved these changes
Aug 11, 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. ✅
doudouOUC
reviewed
Aug 11, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
| }: { | ||
| children: ReactNode; | ||
| clientId?: string; | ||
| }) => (clientId?.startsWith('side-task:') ? null : children), |
Collaborator
There was a problem hiding this comment.
[Suggestion] Hardcoded string literal 'side-task:' should be extracted to a named constant to prevent production/test drift — Failure scenario: If the side-task clientId prefix is changed in production code (e.g., SideTaskPanel.tsx) without updating the test mock, the mock silently stops matching — it returns children instead of null — and the infinite React render loop / OOM reappears in CI. The failure mode is a crash or timeout, not a clear assertion error, making it hard to diagnose.
Suggested change
| }) => (clientId?.startsWith('side-task:') ? null : children), | |
| }) => (clientId?.startsWith(SIDE_TASK_CLIENT_ID_PREFIX) ? null : children), |
中文说明
硬编码的字符串字面量 'side-task:' 应该提取为命名常量，防止生产代码与测试之间的漂移。如果 SideTaskPanel.tsx 中的 clientId 前缀在未来被修改，而测试 mock 未同步更新，mock 将静默失效——返回 children 而非 null——导致无限渲染循环 / OOM 在 CI 中重现。
建议从 packages/web-shell/client/constants/sessions.ts 导出一个常量（如 export const SIDE_TASK_CLIENT_ID_PREFIX = 'side-task:'），并在 SideTaskPanel.tsx（clientId 模板字面量）和 App.test.tsx（mock 的 .startsWith() 检查）中引用它。
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
Comment on lines
+432
to
+433
| clientId?: string; | ||
| }) => (clientId?.startsWith('side-task:') ? null : children), |
Collaborator
There was a problem hiding this comment.
[Suggestion] Mock's clientId filtering is not explicitly asserted — a mutation removing the check would pass all assertions locally — Failure scenario: A future refactor removes or weakens the clientId check (e.g., reverting to the old unconditional children render). The side-task tests would still pass their assertions (they check tab buttons and sessionCreated calls, not the mock's rendering), and the OOM would only be detected when CI runs.
Suggested change
| clientId?: string; | |
| }) => (clientId?.startsWith('side-task:') ? null : children), | |
| // Consider adding a test that explicitly verifies the mock's behaviour for | |
| // side-task: clientIds. For example, a test that renders the App with a | |
| // side-task scenario and verifies the DaemonSessionProvider mock does not | |
| // render its children, or a unit-level assertion on the mock function itself. | |
| }); |
中文说明
Mock 的 clientId 过滤行为没有被明确的断言覆盖。如果未来重构移除或削弱了 clientId 检查（例如恢复为旧的无条件 children 渲染），现有的 side-task 测试仍然会通过（它们检查的是标签按钮和 sessionCreated 调用，而非 mock 的渲染行为），OOM 问题只会在 CI 运行时才被发现。
建议添加一个测试来显式验证 mock 对 side-task: clientId 的行为，例如验证 DaemonSessionProvider mock 不会渲染其 children，或者对 mock 函数本身进行单元级断言。
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
Collaborator
|
Released in v0.21.10. |
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
4 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
Limits the Web Shell app test harness's mocked daemon session provider to the parent session it actually models. Nested side-task providers no longer reuse the parent mocked connection.
Why it's needed
When the side-task creation test resolves the nested session, the shared parent connection causes an infinite React update loop. The test process eventually exhausts its heap, which makes the Web Shell CI job fail.
Reviewer Test Plan
How to verify
Run the side-task creation test and the complete app test file with coverage. Confirm that the side task test completes and all 382 app tests pass without exhausting the Node.js heap.
Evidence (Before & After)
Before: the nested side-task session repeatedly rendered with the parent mocked connection until Vitest ran out of memory.
After: the focused side-task test passes, and the complete app test file passes 382/382 with coverage.
Tested on
Environment (optional)
Node.js 22.14.0, Vitest 3.2.4.
Risk & Scope
Linked Issues
Related to #8872.
中文说明
本 PR 做了什么
将 Web Shell 应用测试桩中的 daemon session provider 限制为它实际模拟的父会话。嵌套 side-task provider 不再复用父会话的 mock 连接。
为什么需要
side-task 创建测试解析嵌套会话后，共用的父会话连接会触发 React 无限更新循环，最终耗尽测试进程堆内存，导致 Web Shell CI 失败。
Reviewer 测试计划
如何验证
运行 side-task 创建测试以及带 coverage 的完整应用测试文件，确认 side-task 测试正常结束，并且全部 382 个应用测试通过，不再耗尽 Node.js 堆内存。
前后证据
修复前：嵌套 side-task 会话使用父会话 mock 连接持续重复渲染，直到 Vitest 内存溢出。
修复后：目标 side-task 测试通过，带 coverage 的完整应用测试文件 382/382 通过。
测试平台
环境（可选）
Node.js 22.14.0，Vitest 3.2.4。
风险与范围
关联问题
与 #8872 相关。