Skip to content

test(web-shell): align workspace sidebar visual smoke - #7107

Merged
yiliang114 merged 1 commit into
QwenLM:mainfrom
yiliang114:cx/fix-web-shell-visual-primary-assertion
Jul 17, 2026
Merged

test(web-shell): align workspace sidebar visual smoke#7107
yiliang114 merged 1 commit into
QwenLM:mainfrom
yiliang114:cx/fix-web-shell-visual-primary-assertion

Conversation

@yiliang114

Copy link
Copy Markdown
Collaborator

What this PR does

Aligns the workspace sidebar visual smoke test with the current UI contract. It continues to verify that both workspaces are grouped by basename and that the primary workspace session loads, without expecting the removed Primary badge.

Why it's needed

#7035 removed the redundant Primary badge, but #7041 subsequently added a visual assertion that still expected it. This caused the Web-shell Visuals check to fail in both themes across unrelated PRs.

Reviewer Test Plan

How to verify

Run npm run test:e2e:visuals --workspace=packages/web-shell -- --grep "workspace sidebar" and confirm that the dark and light scenarios both pass while still rendering both workspace groups and the primary workspace session.

Evidence (Before & After)

  • Before: the dark and light scenarios timed out waiting for Primary2 failed.
  • After: the same targeted command completes with 2 passed.
  • The sidebar regression suite completes with 20 passed.
  • Web Shell lint, typecheck, and build pass.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ CI pending

Environment (optional)

Node.js 22.22.0 with Playwright Chromium.

Risk & Scope

  • Main risk or tradeoff: This removes one visual assertion, but the asserted badge is intentionally absent and that contract is already covered by the sidebar component regression test. Workspace grouping and session-settling assertions remain.
  • Not validated / out of scope: The full visual suite was not run locally; Windows and Linux are left to CI.
  • Breaking changes / migration notes: None.

Linked Issues

Related: #7035, #7041

中文说明

此 PR 做什么

让 workspace sidebar 视觉测试与当前 UI 行为保持一致:继续验证两个 workspace 按目录名分组,以及主 workspace 的 session 正常加载;不再查找已经删除的 Primary 标签。

为什么需要

#7035 删除了冗余的 Primary 标签,但随后合入的 #7041 新增了要求该标签存在的视觉断言,导致多个无关 PR 的 Web-shell Visuals 检查在亮色和暗色主题下同时失败。

Reviewer Test Plan

如何验证

运行 npm run test:e2e:visuals --workspace=packages/web-shell -- --grep "workspace sidebar",确认亮色和暗色场景均通过,并确认两个 workspace 分组和主 workspace session 仍正常渲染。

证据(Before & After)

  • Before:亮色、暗色场景均因等待 Primary 超时,2 failed
  • After:相同定向命令 2 passed
  • 侧栏回归测试:20 passed
  • Web Shell lint、typecheck 和 build 均通过。

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ CI pending

Environment

Node.js 22.22.0,Playwright Chromium。

Risk & Scope

  • 主要风险:减少了一个视觉断言;但该标签不存在是当前明确契约,侧栏组件回归测试已经覆盖,workspace 分组和 session settle 断言仍保留。
  • 未验证/范围外:未在本地运行完整视觉测试套件;Windows/Linux 留给 CI。
  • Breaking changes:无。

Linked Issues

Related: #7035, #7041

@yiliang114
yiliang114 enabled auto-merge July 17, 2026 10:44
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

🖼️ web-shell visual preview

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

Screenshots · before / after

workspace-sidebar-dark before/after

workspace-sidebar-light before/after

Full-resolution recordings (.webm) are attached to the workflow run.

Qwen Code · web-shell visuals

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: Observed and confirmed. PR #7035 removed the Primary badge from the sidebar component (there's even a unit test asserting it's gone — WebShellSidebar.workspace-removal.test.tsx:970). PR #7041 subsequently added a visual assertion that still expected that badge, causing the workspace sidebar visual smoke test to time out in both themes. The before/after evidence is clear: 2 failed → 2 passed.

Direction: Aligned. This fixes a broken test on main — not a feature, not hardening. The test was asserting a UI element that was intentionally removed. Straightforward.

Size: Not applicable — 0 production lines changed, test file only (+2/−10).

Approach: Minimal and focused. One assertion removed, comments simplified to match the current UI contract. The remaining assertions (workspace grouping by basename, session settling) still cover the meaningful behavior. No scope creep.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:已观测并确认。 PR #7035 移除了侧栏组件中的 Primary 标签(甚至有单元测试断言它已不存在——WebShellSidebar.workspace-removal.test.tsx:970)。随后合入的 PR #7041 新增了一个视觉断言仍在检查该标签,导致 workspace sidebar 视觉冒烟测试在两个主题下均超时。Before/after 证据明确:2 failed → 2 passed。

方向:对齐。 这是修复 main 上的一个坏测试——不是功能,不是加固。测试断言了一个被有意移除的 UI 元素。很直接。

规模:不适用——0 行生产代码改动,仅测试文件(+2/−10)。

方案:最小且聚焦。移除一个断言,简化注释以匹配当前 UI 契约。剩余断言(按目录名分组 workspace、session 加载)仍覆盖有意义的行为。没有范围蔓延。

进入代码审查 🔍

Qwen Code · qwen3.7-max

Reviewed at 9e1df28aea657d95e3b019eee37735221644cf1a · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: Given the problem (visual test asserts a Primary badge that was removed in #7035), I would remove the stale await expect(sidebar.getByText('Primary', { exact: true })).toBeVisible() line and update the surrounding comments to stop referencing the badge. The workspace-grouping and session-settling assertions should stay — they cover the actual sidebar behavior.

Comparison with diff: The PR does exactly this. No more, no less. The diff is clean — one assertion removed, comments simplified to match the current UI contract. No scope creep, no drive-by changes.

No issues found. The change is correct and minimal.

Testing

Tmux real-user testing is not applicable here — this is a Playwright visual test fix, not a CLI behavior change. The CI Playwright runs are the relevant evidence:

Visual tests (PR head 9e1df28, job 87865333321):

[10/24] [chromium] › client/e2e/visuals/screenshots.spec.ts:344:5 › web-shell screenshots (dark) › workspace sidebar
[20/24] [chromium] › client/e2e/visuals/screenshots.spec.ts:344:5 › web-shell screenshots (light) › workspace sidebar
  24 passed (57.7s)

E2E Smoke (PR head 9e1df28, job 87870897111):

  14 passed (51.9s)

Both workspace sidebar scenarios (dark and light) pass. The full visual suite (24 tests) and E2E smoke suite (14 tests) are green.

中文说明

代码审查

独立方案: 针对问题(视觉测试断言了一个在 #7035 中已移除的 Primary 标签),我会删除过时的 await expect(sidebar.getByText('Primary', { exact: true })).toBeVisible() 行,并更新周围注释以不再引用该标签。Workspace 分组和 session 加载断言应保留——它们覆盖了实际的侧栏行为。

与 diff 对比: PR 完全符合这个方案。不多不少。Diff 干净——移除一个断言,简化注释以匹配当前 UI 契约。没有范围蔓延,没有夹带改动。

未发现问题。改动正确且最小。

测试

Tmux 真实用户测试不适用于此——这是一个 Playwright 视觉测试修复,不是 CLI 行为变更。CI Playwright 运行结果是相关证据:

视觉测试(PR head 9e1df28,job 87865333321):24 passed (57.7s)
E2E 冒烟测试(PR head 9e1df28,job 87870897111):14 passed (51.9s)

两个 workspace sidebar 场景(暗色和亮色)均通过。完整视觉测试套件(24 个测试)和 E2E 冒烟测试套件(14 个测试)均为绿色。

Qwen Code · qwen3.7-max

Reviewed at 9e1df28aea657d95e3b019eee37735221644cf1a · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — Clean across every stage; this is exactly the kind of focused, well-evidenced fix that keeps CI green.

The problem is real and verified: two PRs crossed paths (#7035 removed the badge, #7041 added a test expecting it), and this PR resolves the resulting breakage with the minimum necessary change. The diff does one thing — removes a stale assertion and aligns comments — and the CI confirms it works (24 visual tests passed, 14 E2E smoke tests passed, both themes). The sidebar component regression test already guards the "no Primary badge" contract, so the remaining visual assertions (workspace grouping, session settling) are the meaningful ones.

Approving. ✅

中文说明

置信度:5/5 — 各阶段均干净;这正是保持 CI 绿色的那种聚焦、有充分证据的修复。

问题真实且已验证:两个 PR 交叉合入(#7035 移除了标签,#7041 新增了期望该标签的测试),本 PR 以最小必要改动解决了由此产生的断裂。Diff 只做一件事——移除过时断言并对齐注释——CI 确认其有效(24 个视觉测试通过,14 个 E2E 冒烟测试通过,两个主题均通过)。侧栏组件回归测试已守护"无 Primary 标签"契约,因此保留的视觉断言(workspace 分组、session 加载)才是有意义的。

批准合入。✅

Qwen Code · qwen3.7-max

Reviewed at 9e1df28aea657d95e3b019eee37735221644cf1a · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

LGTM, looks ready to ship. ✅

@yiliang114
yiliang114 added this pull request to the merge queue Jul 17, 2026
Merged via the queue into QwenLM:main with commit 16a10fb Jul 17, 2026
43 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants