Add harness infrastructure for web-shell package - #6517
Merged
Conversation
Collaborator
|
Thanks for the PR! (Re-run — updating prior comments.) Template looks good ✓ (all required sections present; 中文说明 block is present but the translation is empty — minor, not blocking). Problem: This is test infrastructure work, not a bug fix — web-shell previously lacked browser-level E2E testing without real daemon/auth state. The gap is self-evident from the codebase structure. No reproduction needed for infrastructure additions. Direction: Aligned. Test harness and CI jobs for web-shell are clearly within the project's quality goals. Playwright smoke tests + fake daemon is a standard and well-established approach for browser testing without external dependencies. Size: ~74 production logic lines (data-* attributes for test selectors, debug console cleanup, import-type fixes) vs. ~1,645 test lines (mockDaemon: 883, sseTransport: 227, smoke spec: 309, test setup: 102) vs. ~389 CI/config lines. No core paths touched. Not applicable for size thresholds. Approach: Scope feels right for the stated goal. The mock daemon + SSE transport architecture is well-designed. One observation carried forward to Stage 2: the Moving on to code review. 🔍 中文说明感谢贡献！（重新运行 — 更新之前的评论。） 模板完整 ✓ 问题：这是测试基础设施工作，不是 bug 修复 — web-shell 之前缺少不需要真实 daemon/auth 状态的浏览器级 E2E 测试。这个缺口从代码库结构中就可以明显看出。 方向：对齐。web-shell 的测试工具和 CI 任务明确符合项目的质量目标。 规模：约 74 行生产逻辑行（data-* 属性、debug console 清理、import-type 修复），约 1,645 行测试行，约 389 行 CI/配置行。未触及核心路径。不适用规模阈值。 方案：范围对于目标来说合理。mock daemon + SSE transport 架构设计良好。一个观察点转入 Stage 2： 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code Review(Re-run — updating prior review.) Independent proposal: For adding E2E test infrastructure to web-shell, I'd propose: a package-scoped vitest config, Playwright config with webServer, a mock daemon intercepting REST + SSE via Findings (suggestion-level, non-blocking):
Reuse check: The mock daemon (883 lines) is self-contained test utility code. No existing shared module covers this — Playwright's TestingUnit Tests (local, worktree checkout)Typecheck (local)Build (local)Only the existing Vite large chunk warning (index.js 2083 kB) — pre-existing, not introduced by this PR. E2E Smoke Tests (local, Playwright Chromium)tmux Real-Scenario TestingN/A — this PR adds test infrastructure with no user-visible CLI behavior changes. The validation surface is the test suite itself (unit + E2E smoke), both passing above. CI StatusAll checks passing:
中文说明代码审查（重新运行 — 更新之前的审查。） 独立方案： 与 PR 的实现基本一致。 发现（建议级别，非阻塞）：
测试
— Qwen Code · qwen3.7-max |
Collaborator
|
(Re-run — updating prior verdict.) This is solid test infrastructure work. The mock daemon + SSE transport architecture is well-designed — Production code changes are minimal and appropriate: All tests pass locally (79 unit test files / 1275 tests, 6 Playwright smoke tests, build, typecheck). CI is green across the board. The Approval guardrail: Approving. ✅ 中文说明这是扎实的测试基础设施工作。mock daemon + SSE transport 架构设计良好。生产代码改动最小且适当：data-* 属性用于稳定的测试选择器，移除了多余的 console.debug 语句。 所有测试本地通过（79 单元测试文件 / 1275 测试，6 个 Playwright smoke 测试，构建，类型检查）。CI 全部绿色。
批准合并。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
requested changes
Jul 8, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Good test infrastructure work — all 622 unit tests and 5 Playwright smoke tests pass, build succeeds. One blocker: typecheck fails with TS2698 in client/e2e/utils/mockDaemon.ts:78 (...scenario.state.models spreads a non-object type). The main tsconfig.json was not updated to exclude client/e2e/** and client/test/** (only tsconfig.lib.json was). Straightforward fix — either add the exclusions or narrow the spread type. See review comments above for details.
# Conflicts: # packages/web-shell/client/components/ChatEditor.tsx # packages/web-shell/client/components/MessageList.tsx # packages/web-shell/client/components/dialogs/ApprovalModeDialog.tsx # packages/web-shell/client/components/dialogs/DialogShell.tsx # packages/web-shell/client/components/dialogs/ModelDialog.tsx # packages/web-shell/client/components/dialogs/ResumeDialog.tsx # packages/web-shell/client/components/dialogs/ThemeDialog.tsx
Collaborator
Author
Fixed: 975ab64 |
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
requested changes
Jul 8, 2026
Collaborator
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
.github/workflows/e2e.yml |
web-shell-browser-regression job missing permissions block; inherits repo defaults unlike ci.yml jobs |
Add permissions: contents: read to the job |
client/e2e/utils/sseTransport.ts:79-82 |
writeBytes silently drops events when no active controllers exist — no error, no log |
Throw or console.warn when activeControllers() returns empty |
client/e2e/utils/sseTransport.ts:181 |
waitForConnection timeout throws 'SSE connection was not recorded.' with no sessionId or connection count context |
Wrap in try/catch and re-throw with diagnostic info (target sessionId, current connections, harness presence) |
client/e2e/utils/mockDaemon.ts |
Dual response path for GET /session/:id/events (fetch override + page.route) — switching from fetch() to EventSource would silently hit inert SSE comment |
Document which handler the app uses; consider removing the page.route fallback or adding a comment warning about the dual-path |
client/e2e/utils/mockDaemon.ts:415-418 |
promptIdFor falls back to hardcoded 'prompt-e2e'; tests never verify prompt ID correlation |
Extract promptId from the actual prompt POST response, or assert that _meta.promptId is present in the request body |
client/e2e/utils/mockDaemon.ts:413 |
lastEventId watermark computed from static scenario.events only; live SSE events don't advance it |
Append live events to scenario.events in the SSE send() path, or track a separate maxEventId counter |
client/e2e/utils/mockDaemon.ts:596-649 |
Session state mutations (model, approval-mode) mutate shared scenario object — loading a different session sees mutated state |
Clone scenario per session ID, or track per-session state in a Map |
client/e2e/utils/mockDaemon.ts:75 |
applyScenarioCurrentModel exported but never imported by any other file |
Remove export keyword |
client/components/dialogs/SessionRow.tsx:31-33 |
resumeSelector?: boolean prop added to production component solely for test selectors; other data attributes in this PR are unconditional |
Emit data-session-id and data-web-shell-resume-session unconditionally, drop the prop |
client/e2e/utils/mockDaemon.ts |
883 lines of mock daemon logic with zero unit tests; edge cases only tested indirectly through smoke tests | Add mockDaemon.test.ts covering readRequestBody, body validation, 405 responses, applyScenarioCurrentModel |
client/e2e/web-shell.smoke.spec.ts |
All 7 smoke tests cover only happy paths; no negative tests (500 error, SSE disconnect, empty prompt) | Add 2-3 negative tests: daemon 500 → error state UI; SSE disconnect mid-stream → reconnection indicator; empty prompt → no request sent |
client/e2e/web-shell.smoke.spec.ts:113-132 |
"keeps later SSE connections alive" test name is misleading — tests reload reconnection, not concurrent SSE connections | Rename to "re-establishes SSE connection after page reload" |
client/e2e/web-shell.smoke.spec.ts:163-183 |
Permission test verifies POST was sent but doesn't verify panel dismissal after decision | Add await expect(page.locator('[data-web-shell-permission-panel]')).toHaveCount(0) after the decision |
client/e2e/utils/sseTransport.ts:192-195 |
burstEvents sends events via individual page.evaluate round-trips instead of batching |
Batch all frames into a single page.evaluate call |
.github/workflows/ci.yml + e2e.yml |
Playwright browser binaries (~150-200 MB) downloaded fresh every CI run with no caching | Add actions/cache for ~/.cache/ms-playwright keyed on package-lock.json |
client/test/reactHarness.tsx:33-39 |
immediateClipboardWrite() returns a thenable (only .then()), cast as Promise<void> — lacks .catch() and .finally() |
Return Promise.resolve() instead of a custom thenable object |
— qwen3.7-max via Qwen Code /review
wenshao
requested changes
Jul 8, 2026
Co-authored-by: qwen-code-ci-bot <qwen-code-ci@service.alibaba.com>
harbin1053020115
enabled auto-merge
July 9, 2026 03:43
wenshao
reviewed
Jul 9, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Collaborator
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
client/e2e/utils/mockDaemon.ts:598-606 |
Catch-all if (action === 'events' || extra) returns fake 200 SSE for any unrecognized action with an extra path segment, silently masking routing bugs in future tests |
Replace with if (action === 'events') and let unmatched actions fall through to the 404 |
client/e2e/web-shell.smoke.spec.ts:175-183 |
completeReplay() uses getByText('Loading...') which is a no-op — "Loading..." is an aria-label, not visible text; assertion passes immediately |
Use expect(page.locator('[data-web-shell-composer-submit]')).not.toHaveAttribute('aria-label', 'Loading...') or assert message list visibility |
client/e2e/utils/mockDaemon.ts:579 |
approval-mode handler echoes back mode but doesn't update scenario.currentMode, unlike the model handler which calls applyScenarioCurrentModel() |
Update scenario.currentMode and scenario.providers.approvalMode in the handler |
client/components/dialogs/SessionRow.tsx:33,84-85 |
resumeSelector boolean prop gates data-session-id emission, coupling a shared component to a test concern specific to one caller |
Always emit data-session-id on every SessionRow and drop the resumeSelector prop |
client/e2e/utils/mockDaemon.ts:657-661 |
promptIdFor() reads body._meta.promptId but the SDK client never stamps this field; always returns hardcoded 'prompt-e2e' — dead code path |
Remove _meta.promptId extraction or document that the mock assigns a fixed promptId |
client/e2e/utils/sseTransport.ts:95-134 |
Two-layer interception (browser-level for SSE, Node-level for REST) is undocumented; future maintainers adding page.route assertions for SSE will find they never fire |
Add a block comment in installMockDaemon explaining the architecture split |
— qwen3.7-max via Qwen Code /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. Suggestion-level recommendations are in the Suggestion summary comment below.
wenshao
reviewed
Jul 9, 2026
| replayedCount, | ||
| }), | ||
| ); | ||
| await expect(page.getByText('Loading...')).toHaveCount(0); |
Collaborator
There was a problem hiding this comment.
[Suggestion] This helper sends replay_complete and then waits for visible text Loading..., but the transcript loading skeleton is rendered as data-testid="message-list-loading-skeleton" with aria-hidden, while the screen-reader text is Session is still loading.... As a result this assertion can pass immediately and later smoke steps may run while loadingTranscript is still true, which is exactly when prompt submission is blocked.
Suggested change
| await expect(page.getByText('Loading...')).toHaveCount(0); | |
| await expect( | |
| page.locator('[data-testid="message-list-loading-skeleton"]'), | |
| ).toHaveCount(0); |
— GPT-5 Codex via Qwen Code /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. Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x). Suggestion-level recommendations are in the Suggestion summary comment below.
wenshao
approved these changes
Jul 9, 2026
Collaborator
|
Local validation report for PR #6517 at head Environment:
Validation results:
Notes for merge consideration:
中文版本PR #6517 本地验证报告，验证的 head 为 环境：
验证结果：
合并参考：
|
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 9, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM — solid test infrastructure work. Unit tests (79 files/1275 tests), E2E smoke (6 Playwright tests), build, and typecheck all pass locally. ✅
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
6 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
Add harness infrastructure for the web-shell package: package-scoped Vitest config, shared React test setup, Playwright browser smoke harness, fake daemon REST routes, controllable SSE transport, stable UI selectors, and CI jobs for PR smoke plus nightly/manual full regression.
Harden the browser smoke harness by keeping later SSE connections alive after reload/cancel and by synchronizing the fake daemon current model state with the UI.
Why it's needed
web-shell depends on daemon REST/SSE behavior, but browser-level validation previously required real daemon/auth state and had limited CI observability. This PR makes key web-shell workflows reproducible without a real daemon, external network service, or API key, while still producing JUnit, coverage, Playwright trace, screenshot, video, and HTML report artifacts for failures.
Reviewer Test Plan
How to verify
Run npm run test --workspace=packages/web-shell and confirm the package unit tests pass without React act(...) warnings.
Run npm run test:ci --workspace=packages/web-shell and confirm packages/web-shell/junit.xml and packages/web-shell/coverage/coverage-summary.json are produced.
Run npm run test:e2e:smoke --workspace=packages/web-shell and confirm the fake daemon browser smoke covers transcript replay, streamed assistant events, prompt submission, permission decisions, slash commands, resume dialog, model dialog, theme dialog, SSE reconnect behavior, and persisted selected model state.
Optional broader check: run npm run build --workspace=packages/web-shell and npm run test:e2e --workspace=packages/web-shell.
Evidence (Before & After)
N/A for screenshots because this is test harness and CI infrastructure work with no intended user-visible UI change.
Local evidence: npm run test --workspace=packages/web-shell passed 43 test files and 622 tests; npm run test:ci --workspace=packages/web-shell passed and generated JUnit/coverage output; npm run test:e2e:smoke --workspace=packages/web-shell passed 5 Playwright smoke tests; npm run build --workspace=packages/web-shell passed with only the existing Vite large chunk warning; git diff --check passed; pre-commit ran prettier --write and eslint --fix --max-warnings 0.
Tested on
Environment (optional)
Local macOS verification used Node v25.8.2 and npm 11.11.1. Browser smoke uses Playwright Chromium and the new fake daemon harness, so it does not require a real daemon, external network service, or API key.
Risk & Scope
Linked Issues
中文说明