Skip to content

test(web-shell): prevent nested session render loop #8934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ytahdn merged 1 commit into from
Aug 11, 2026
Merged

test(web-shell): prevent nested session render loop #8934

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/web-shell/client/App.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,14 @@ vi.mock('@qwen-code/webui/daemon-react-sdk', () => {
};
return {
DAEMON_APPROVAL_MODES: ['default', 'plan', 'auto-edit', 'auto', 'yolo'],
DaemonSessionProvider: ({ children }: { children: ReactNode }) => children,
// This harness only models the parent session connection.
DaemonSessionProvider: ({
children,
clientId,
}: {
children: ReactNode;
clientId?: string;
}) => (clientId?.startsWith('side-task:') ? null : children),

@doudouOUC doudouOUC Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

@doudouOUC doudouOUC Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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)

useActions: () => mockSessionActions,
useConnection: () => mockConnection,
useDaemonSessionOwnerGuard: () => ownerGuard,
Expand Down
Loading