feat(web-shell): show GitHub-style state icons for session PR bindings - #10258
Merged
Conversation
The session tooltip listed every bound PR with a neutral icon and a text state suffix. Switch to GitHub's visual vocabulary (open=green pull-request icon, merged=purple merge icon, closed=red closed-PR icon) so state is scannable at a glance, keeping an sr-only state label for screen readers. State-less bindings stay neutral.
Collaborator
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: a real UX gap, not a theoretical one — session PR bindings (#9729) surface state only as a trailing " · Merged" / " · Closed" text suffix on otherwise identical tooltip icons, and the session-row badge is a uniform pill that carries no state at all. The "before" is the shipped behavior today, documented with before/after screenshots. Direction: aligned. Green-open / purple-merged / red-closed is GitHub's native state vocabulary, and the Web Shell already speaks it — Size: not core — 8 files under Approach: minimal and well scoped. One shared state → (icon glyph, color class, localized label) component used by both surfaces, state names kept accessible via aria-label (badge) and sr-only text (tooltip), colors from existing theme tokens. The three commits since the last triage pass extend the same mapping to the badge, align the tooltip link color, and centralize the label — a coherent continuation of the same goal, no unrelated edits, nothing to cut. Risk: no elevated risk signals; none of the changed files match revert-correlated paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实的 UX 缺口，而非理论性问题——会话 PR 绑定（#9729）目前仅以尾随的 " · Merged" / " · Closed" 文字后缀表达状态，图标全部相同；会话行 badge 则是完全不含状态信息的统一药丸。"Before" 就是当前已上线的行为，且附有 before/after 截图。 方向：对齐。绿 open / 紫 merged / 红 closed 是 GitHub 的原生状态词汇，Web Shell 内部已在用—— 规模：非核心——仅 方案：最小且范围清晰。一个共享的 state →（图标 + 颜色类 + 本地化标签）组件同时服务两个界面，状态名通过 aria-label（badge）与 sr-only 文本（tooltip）保留无障碍信息，颜色取自现有主题 token。上次审查后的三个提交把同一映射扩展到 badge、对齐 tooltip 链接颜色并集中管理标签——是同一目标的自然延续，无无关改动，没有可砍的部分。 风险：无升级风险信号；改动文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewRe-reviewed at the new head — three commits landed since the last pass: the session badge gets the same state icons, the tooltip PR link color aligns with the badge, and the icon/label mapping is centralized into one shared component. My independent baseline for the combined goal ("binding state scannable at a glance on both surfaces") is exactly this shape: a single state → (glyph, color, label) table shared by badge and tooltip, a neutral pill, and accessibility preserved. The diff matches it. Verified against the code at this head:
Nothing blocking. Test evidence (PR's own CI at
|
| Check | Conclusion |
|---|---|
Test (ubuntu-latest, Node 22.x) |
✅ success |
web-shell E2E Smoke (ubuntu-latest, Node 22.x) |
✅ success |
Desktop Shell (ubuntu-22.04) |
✅ success |
Desktop Shell (windows-2022) |
✅ success |
Dependency CVE audit |
✅ success |
Secret scan (TruffleHog) |
✅ success |
Capture web-shell visuals (ubuntu-latest, Node 22.x) |
✅ success |
Classify PR |
✅ success |
Post Coverage Comment (ubuntu-latest, 22.x) |
✅ success |
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。
Third-party verification in this thread: an independent tmux E2E report at this exact head ran the two updated suites (10/10 pass) and a static review with no blocking findings, noting pixel-level rendering is the visuals lane's job. The author's Playwright computed-color numbers (badge open rgb(72, 187, 120), merged rgb(139, 92, 246), etc.) are the author's claim — consistent with the tokens used, but not independently re-run here; this review never executes PR code.
Sandboxed verification would settle the remaining claim: @qwen-code /tmux — with the visuals preview unavailable at this head, the actually-rendered colored icons on the badge and the hover tooltip rest on class-mapping tests and specificity math rather than pixels; the tmux lane drives the real web-shell surface and can capture what users see.
中文说明
代码审查（已在新 head 上重审）：上次审查后新增三个提交——会话 badge 获得相同的状态图标、tooltip 的 PR 链接颜色与 badge 对齐、图标/标签映射集中到一个共享组件。我的独立基线（"两个界面都能一眼扫读绑定状态"）正是这个形态：单一 state →（图标 + 颜色 + 标签）表同时服务 badge 与 tooltip、中性药丸、无障碍保留。diff 与基线一致，并逐点核实：STATE_ICONS 覆盖 DaemonSessionPrInfo.state 全部取值（SDK 运行时校验器只允许 open/merged/closed/缺省），satisfies Record<NonNullable<...>> 强制未来新增状态同时更新图标与标签；无状态绑定两个界面均渲染中性图标。双写类选择器特异性 (0,2,0) 正确压过 .sessionDetailsRow svg 的默认灰色 (0,1,1)，CSS 注释已说明原因。颜色沿用包内先例（--success-color、--destructive、旧 badge 同款 --color-accent-fg 兜底）；badge 药丸经 --muted-foreground 转为中性，删除了冗余的 merged 专用调灰类，全仓无 sessionPrBadgeMerged 孤儿引用。无障碍保留：badge aria-label 仅对 merged/closed 追加状态后缀，tooltip 以 sr-only 承载、可见文本保持裸标签，图标均 aria-hidden，两个 i18n key 在 en/zh 词表中均存在。新增的 .sessionDetailsRow a 规则只影响 PR 链接——folder/branch/status 行均为 <span>，tooltip 中无其他链接变色。测试在两个界面上钉住字形、颜色类与无障碍标签，含无状态兜底与状态被换/丢的回归用例。无可复用的现成映射（GitHubPrsDialog 面向含 draft 的 GitHub API 状态，语义不同），无顺手改动。无阻塞问题。
测试证据：本 head 的 PR CI 全绿——ubuntu 单测、web-shell E2E smoke、两个 Desktop Shell、CVE 审计、密钥扫描均通过。macOS/Windows 测试为跳过：ci.yml 在修复长期 Windows 失败前把这两条通道只挂在 merge_group 触发，属仓库策略而非本 PR 信号。唯一在跑的是机器人自己的 pull_request_target 审查任务，非 PR CI。如实说明一点：视觉通道检查为绿，但本 head 无法产出 before/after 预览——workspace sidebar 场景以 Playwright strict-mode 冲突失败（两行同名 "Run auth migration"）。已拉取任务日志核对：merge-base 上的 base 渲染以完全相同的错误失败于同样两个场景，属既有 fixture 噪音，非本 PR 引入。后果是本 head 没有仓库侧的像素级证据，渲染效果由钉住类映射的单测 + 静态特异性推算 + 下方第三方报告支撑。
第三方验证：本线程中一份独立 tmux E2E 报告在同一 head 上运行两个更新套件（10/10 通过）并做了静态审查，无阻塞发现。作者给出的 Playwright 计算颜色数值是作者声明——与所用 token 一致，但本审查不执行 PR 代码，未独立复跑。
沙盒验证：@qwen-code /tmux 可补上最后一块——视觉预览在本 head 不可用的情况下，badge 与悬停 tooltip 上实际渲染的彩色图标目前由类映射测试与特异性推算支撑，tmux 通道可驱动真实界面截取用户所见。
— Qwen Code · qwen3.8-max
Reviewed at bef232ce097d6b078a629ee1437bcf2bc1d78647 · re-run with @qwen-code /triage
Collaborator
|
Confidence: 4/5 — clean review, green CI, ready to ship; the one reservation is evidentiary, not in the code: the visuals lane's pre-existing fixture failure blocks its preview at this head, so the rendered icons rest on pinned tests rather than pixels. Re-running the reflection at the new head changed my mind in the PR's favor on one point: the badge extension is the more valuable half of this change. The tooltip was already readable if you hovered and read the suffix; the badge is always visible in a long session list, and "which sessions still have an open PR" is now answerable at a glance without hovering. The centralization into one shared component is the right structure — a fourth state (or a design tweak) is one table edit, applied to both surfaces at once. Everything I could verify checks out: the state domain is exhaustively covered against the SDK's runtime validator, the specificity math is deterministic and documented, accessibility information moves but is not lost, no orphaned references, and the tests fail without the diff. CI is settled green on this commit — unit suite, E2E smoke, both desktop shells, security checks. A maintainer approval already stands on the PR, and the independent tmux E2E report in this thread ran the suites at this exact head with no blocking findings. The 4 (not 5) is honesty about one gap I cannot close from here: with the visuals preview unavailable (the pre-existing Approving, pinned to the reviewed commit. 中文说明Confidence 4/5：审查干净、CI 全绿、可以合入；唯一的保留是证据层面的，而非代码问题——视觉通道因既有 fixture 失败在本 head 无法产出预览，渲染效果由钉住的测试支撑而非像素截图。 在新 head 上重新反思后，有一点让我更认可这个方向：badge 扩展其实是这次改动里更有价值的部分。tooltip 原本悬停细读后缀也能读，而 badge 在长会话列表里常驻可见，"哪些会话还挂着 open PR" 现在不用悬停就能一眼回答。集中到一个共享组件也是正确结构——未来新增第四种状态（或调整设计）只需改一张表，两个界面同时生效。 能核实的都核实了：状态域相对 SDK 运行时校验器穷尽覆盖，特异性换算是确定性的且有注释，无障碍信息被迁移而非丢失，无孤儿引用，测试去掉 diff 必挂。CI 已在本提交上落绿——单测、E2E smoke、两个桌面壳、安全检查。PR 上已有一位维护者批准，线程中的独立 tmux E2E 报告也在同一 head 上跑过套件、无阻塞发现。 给 4 分（而非 5 分）是如实说明一个我无法从这里补上的缺口：视觉预览不可用（既有的 批准，锚定到已审查的提交。 — 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 / afterFull-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. |
The session-row badge showed the same accent pill for every bound PR and only dimmed merged ones. It now renders the shared GitHub-style state icon (open=green pull-request, merged=purple merge, closed=red closed-pull-request, state-less=neutral) on a neutral pill, matching the tooltip and GitHub's own list convention, and the badge aria-label carries the merged/closed state name. The icon/color mapping is extracted into a shared component used by both surfaces.
The tooltip PR rows used the global link blue while the session-row badge renders muted text, and a blue/purple link next to the merged state icon's purple reads as two competing state colors. Give the tooltip link the same muted tone as the badge text (brightened on hover).
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. Suggestions are inline.
Test Plan (not a blocker): components/SessionPrBadge.test.tsx — no such file or directory; components/sidebar/SessionDetailsTooltip.test.tsx — no such file or directory.
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：components/SessionPrBadge.test.tsx — no such file or directory; components/sidebar/SessionDetailsTooltip.test.tsx — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Two review findings: the tooltip sr-only state text was concatenated onto the PR label with no separator, fusing the accessible name into "Pull Request #9500Merged"; and the state-to-label mapping was hand-written in both consumers, so a future fourth state would silently emit no screen-reader label. The sr-only text now carries the same " · " separator as the badge aria-label, and the label lookup lives beside STATE_ICONS in the shared component — keyed over the state union via satisfies, so a new state fails compilation until both icon and label mappings are updated.
Collaborator
tmux E2E test report (head
|
yiliang114
approved these changes
Aug 27, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed at head bef232c:
SessionPrStateIconmaps state → GitHub-style lucide icons behind asatisfies Record<NonNullable<state>>table, so a future state forces both icon and localized label to update together; state-less bindings keep the neutral glyph.- Accessibility is preserved on both surfaces: icons are
aria-hidden, the badge aria-label gains the localized state suffix, and the tooltip moves the merged/closed suffix into ansr-onlyspan while the visible text stays the bare PR label (consistent with the existingsr-onlyusage in the shadcn-derived ui components). - The doubled-class specificity trick in the icon CSS is documented against the
.sessionDetailsRow svgdefault, badge tone is unified to the muted palette (dropping the separate merged-dim class), and the tooltip link tone change avoids clashing with the purple merge icon. - Tests pin the icon classes per state, the color classes, and the aria-label suffix behavior on both open and state-less bindings.
CI note: the ubuntu test lane and the web-shell visual capture are still running at the time of approval — merge should wait for those to land green.
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 301 passed · 0 failed · 301 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：301 通过 · 0 失败 · 301 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10258 — Deep VerificationVerdict: 中文摘要
Central claim + A/B load-bearing proofCentral claim: both PR-binding surfaces (session-row badge, session-details tooltip) render the shared Secondary claims: (a) the doubled-class state color rules (0,2,0) outrank the tooltip row's default muted icon color Harness:
The 17/25 flips are exactly the claimed behavior delta (svg presence, glyph + state class per state, sr-only vs visible suffix, aria-label suffix, neutral pill); every invariant check (anchor text, link set, row order, Control hygiene: the PR leaves Mutation / vacuity matrix
No mutant survived. One coverage asymmetry surfaced (see F1): under M3 the badge suite stayed green — the tooltip's exact CSS cascade oracle
Reviewer Test Plan walkthrough
Gates
FindingsF1 — Suggestion (coverage gap, introduced by PR): the badge aria-label separator is unpinned. Under M3 (separator removed) the badge suite stays green because F2 — Low (pre-existing, attributed): mid-test failures cascade into the next tooltip test. Any assertion failure inside F3 — Low (pre-existing, advisory): i18n keys are loosely typed. Not covered
MethodologyEnvironment: CI verify container ( Flakiness gate logEvidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
approved these changes
Aug 27, 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. ✅
qqqys
approved these changes
Aug 27, 2026
qqqys
left a comment
qqqys
left a comment
Collaborator
There was a problem hiding this comment.
Approving. Independent review at this head (bef232c, incl. the fourth commit) found no blocking (Critical) issues: the state→icon/color table is exhaustive via satisfies so a new state forces icon+label together, state-less bindings render neutral, and accessibility is preserved (aria-label state suffix on the badge, sr-only suffix on the tooltip, aria-hidden icons); no ref-forwarding/Radix-asChild surface touched. Local tmux run at this head: SessionPrBadge.test.tsx + SessionDetailsTooltip.test.tsx 10/10 green (incl. the swapped/dropped-state regression pins). The bot has approved at this head and all ran CI lanes are green; concur.
wenshao
commented
Aug 27, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 4": none — I completed the walk of all changed files and their call sites within budget.; "agent 6b": could not execute npx vitest run for the two changed test files in this worktree (no node_modules here), so finding 1's no-op behavior was established by pr….
Test Plan (not a blocker): components/SessionPrBadge.test.tsx — no such file or directory; components/sidebar/SessionDetailsTooltip.test.tsx — no such file or directory.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/SessionPrBadge.test.tsx:68 (+2 locations) — [review] both "state-less binding carries no state colour" assertions read svg.className, which is an SVGAnimatedString, so they can never fail — a mutant that…packages/web-shell/client/components/SessionPrStateIcon.module.css:9 — [review] merged's colour reads var(--color-accent-fg), which nothing in the repo defines, so it is always the #8b5cf6 fallback and never follows light/dark theming, unli…packages/web-shell/client/components/SessionPrBadge.test.tsx:71 — [review] no badge test pairs a multi-PR binding with differing states, so pointing both new state reads at openable[0] instead of latest keeps 61/61 green while the pill show…
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 4"：none — I completed the walk of all changed files and their call sites within budget.；"agent 6b"：could not execute npx vitest run for the two changed test files in this worktree (no node_modules here), so finding 1's no-op behavior was established by pr…。
Test Plan（非阻断）：components/SessionPrBadge.test.tsx — no such file or directory; components/sidebar/SessionDetailsTooltip.test.tsx — no such file or directory。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-flash via Qwen Code /review (v0.22.2)
| t: (key: string) => string, | ||
| state?: DaemonSessionPrInfo['state'], | ||
| ): string | undefined { | ||
| if (state !== 'merged' && state !== 'closed') return undefined; |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] R2-1: sessionPrStateLabel re-hardcodes which states carry a label (if (state !== 'merged' && state !== 'closed') return undefined;) instead of consulting STATE_ICONS[state].labelKey, and labelKey is declared optional in the satisfies target. The table is therefore not the source of truth for labels — the guard is — so the invariant this function's JSDoc and the HEAD commit message both claim ("a new state forces both the icon and the label mapping to be updated together") holds only for the icon half.
Measured in a scratch tree: widen DaemonSessionPrInfo['state'] with a fourth value (draft is already modelled upstream in packages/core/src/utils/github-prs.ts:31). satisfies does fail compilation until an entry is added — and then draft: { Icon, className, labelKey: 'sidebar.sessionPrStateDraft' } type-checks clean while sessionPrStateLabel(t, 'draft') still yields undefined, because the !== guard discards the table row. Both consumers gate on that return value (SessionPrBadge.tsx:51, SessionDetailsTooltip.tsx:173), so the new state ships a distinct coloured glyph and no state name for assistive tech, with tsc and the suite green. This is the residual half of last round's centralization ask: R1-2 is fixed between the two consumers, and the hand-written case set moved into this one function instead of disappearing. sessionPrStateLabel also has no direct unit test anywhere in the repo — the only references are the two production call sites.
Witness — probe output: widening the keyed union without an entry gives error TS1360 ... Property 'draft' is missing; adding the entry with a labelKey gives tsc clean and the probe prints ["draft",null] while the icon path renders a state class. Making labelKey required ({ Icon; className; labelKey: string | undefined }, with open writing labelKey: undefined) turns the same entry into TS2741 and keeps today's behaviour fingerprint identical ([[open,null],[merged,Merged],[closed,Closed],[undefined,null]], both changed test files 10/10 green).
One trap for the naive rewrite: STATE_ICONS[state].labelKey alone does not compile against the current table (as const leaves open without the property), and with an optional labelKey it throws Cannot read properties of undefined (reading 'labelKey') on an out-of-union value where today's code degrades to undefined — hence the ?. below.
Suggested change
| if (state !== 'merged' && state !== 'closed') return undefined; | |
| } as const satisfies Record< | |
| NonNullable<DaemonSessionPrInfo['state']>, | |
| { Icon: typeof GitPullRequestIcon; className: string; labelKey: string | undefined } | |
| >; |
…and then read the label off the table:
Suggested change
| if (state !== 'merged' && state !== 'closed') return undefined; | |
| const key = state ? STATE_ICONS[state]?.labelKey : undefined; | |
| return key ? t(key) : undefined; |
(open gains an explicit labelKey: undefined.) If the guard stays as-is instead, please correct the JSDoc and the commit-message claim, which currently advertise a coupling nothing enforces. Acceptance criterion for the fix: in SessionPrBadge.test.tsx, "appends the state name to the aria-label for merged and closed PRs" must go red if open's table entry is changed to carry a real key — under today's guard-based code that mutation stays green, which is the proof that the guard and not the table controls the label.
中文说明
sessionPrStateLabel 把"哪些状态需要标签"重新硬编码了一遍（if (state !== 'merged' && state !== 'closed') return undefined;），没有去读 STATE_ICONS[state].labelKey，而 labelKey 在 satisfies 约束里是可选属性。也就是说，这张表并不是标签的唯一真值来源——真正的闸门是这个 guard——所以本函数 JSDoc 与 HEAD commit message 都声称的不变量（"新增状态会同时强制更新图标和标签映射"）只在图标那一半成立。
在一次性 scratch 树中实测：给 DaemonSessionPrInfo['state'] 增加第四个值（draft 在上游 packages/core/src/utils/github-prs.ts:31 已有建模）。satisfies 确实会在补表之前编译失败——但补上 draft: { Icon, className, labelKey: 'sidebar.sessionPrStateDraft' } 之后类型完全通过，而 sessionPrStateLabel(t, 'draft') 仍返回 undefined，因为 !== guard 直接把表项丢弃了。两个消费方都依赖这个返回值（SessionPrBadge.tsx:51、SessionDetailsTooltip.tsx:173），于是新状态会带着一个有颜色的图标上线，而辅助技术拿不到任何状态名，tsc 和测试全绿。这正是上一轮"集中化"诉求的残留：R1-2 在两个消费方之间已经修好，但手写的状态判断只是搬进了这个函数，并没有消失。此外 sessionPrStateLabel 在仓库里没有任何直接单测，只有两个生产调用点。
证据（探针输出）：扩宽联合类型但不补表 → error TS1360 ... Property 'draft' is missing；补上带 labelKey 的表项 → tsc 通过且探针打印 ["draft",null]。把 labelKey 改成必填（{ Icon; className; labelKey: string | undefined }，open 显式写 labelKey: undefined）后，同样的表项会报 TS2741，而当前行为指纹保持不变（[[open,null],[merged,Merged],[closed,Closed],[undefined,null]]，两个改动的测试文件 10/10 通过）。
一个坑：只写 STATE_ICONS[state].labelKey 在当前表上无法编译（as const 让 open 没有该属性）；若 labelKey 仍是可选，遇到越界值会抛 Cannot read properties of undefined (reading 'labelKey')，而现在的代码会退化成 undefined——所以上面的方案保留了 ?.。如果坚持保留 guard，请同时修正 JSDoc 与 commit message 中那个并不存在的编译约束。验收标准：修好之后，在 SessionPrBadge.test.tsx 的 "appends the state name to the aria-label for merged and closed PRs" 里把 open 的表项改成带真实 key，该用例必须变红；而按现在这份 guard 代码，同样的改动是绿的——这恰好证明控制标签的是 guard，不是那张表。
— qwen3.8-flash via Qwen Code /review (v0.22.2)
|
|
||
| // State lives in the icon; visible text stays the bare PR label, with an | ||
| // sr-only " · State" suffix so screen readers keep the information. | ||
| expect(byNumber(9500)?.textContent).toBe('Pull Request #9500 · Merged'); |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] R2-2: The three textContent assertions below replaced the old visible-suffix checks, but textContent reads the anchor's whole text and is byte-identical whether or not the suffix <span> in SessionDetailsTooltip.tsx:174 carries className="sr-only". The comment directly above them states the property under test ("State lives in the icon; visible text stays the bare PR label"), and that is exactly the half nothing pins: the one mechanism that makes this PR's tooltip claim true is asserted by nothing.
Failure scenario: drop className="sr-only" from that suffix span — or lose it to the web-shell CSS-scoping pass — and the row visibly reverts to Pull Request #9500 · Merged sitting next to the new purple merge icon, the "Before" state this PR exists to remove, while the suite stays green. Witness — ran exactly that mutant against this PR's own harness: ✓ components/SessionPrBadge.test.tsx (2 tests), ✓ components/sidebar/SessionDetailsTooltip.test.tsx (8 tests), 10/10 green, with the anchor's textContent identical in both arms and the span's className "" (mutant) vs "sr-only" (clean). Nothing else covers it: the only sr-only assertions in web-shell tests are at lines 326-328 of this file, on the [aria-live="polite"] copy-feedback node, and no e2e/visual spec touches the tooltip PR rows.
Suggested change
| expect(byNumber(9500)?.textContent).toBe('Pull Request #9500 · Merged'); | |
| expect(byNumber(9500)?.querySelector('span.sr-only')?.textContent).toBe(' · Merged'); | |
| expect(byNumber(9501)?.querySelector('span.sr-only')?.textContent).toBe(' · Closed'); | |
| expect(byNumber(9502)?.querySelector('span.sr-only')).toBeNull(); |
Acceptance criterion: the new span.sr-only query must go red the moment className="sr-only" is removed from the suffix span — the same assertion pattern this file already uses at line 326.
中文说明
下面这三条 textContent 断言取代了旧的可见文本后缀检查，但 textContent 读取的是整个锚文本：无论 SessionDetailsTooltip.tsx:174 的那个 <span> 是否带 className="sr-only"，结果都逐字节相同。紧贴在它们上方的注释写明了待测性质（"状态由图标承载，可见文本保持为纯粹的 PR 标签"），而这恰恰是没有任何断言覆盖的那一半——让这个 PR 的 tooltip 主张成立的唯一机制没有被任何测试钉住。
失败场景：把该后缀 span 上的 className="sr-only" 去掉（或者在 web-shell 的 CSS 作用域处理中丢失），这一行就会重新显示成 Pull Request #9500 · Merged，紧挨着新的紫色 merge 图标——也就是本 PR 要消除的 "Before" 状态——而测试依旧全绿。证据——正是在本 PR 自己的测试设施上跑了这个变异体：✓ components/SessionPrBadge.test.tsx (2 tests)、✓ components/sidebar/SessionDetailsTooltip.test.tsx (8 tests)，10/10 通过；两个分支里锚文本的 textContent 完全一致，而 span 的 className 分别是 ""（变异体）和 "sr-only"（原始代码）。别处也没有覆盖：本文件里唯一的 sr-only 断言在 326-328 行，针对的是 [aria-live="polite"] 的复制反馈节点；e2e/视觉用例也完全没有触及 tooltip 的 PR 行。
验收标准：一旦从后缀 span 上移除 className="sr-only"，新加的 span.sr-only 查询必须变红——这也是本文件 326 行已经在使用的断言写法。
— qwen3.8-flash via Qwen Code /review (v0.22.2)
Collaborator
|
Released in v0.22.3. |
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
Sessions bound to GitHub pull requests (#9729) now show the PR state in GitHub's visual vocabulary in both places the binding appears in the Web Shell sidebar. In the session details tooltip, each bound PR row previously used the same neutral pull-request icon with a text suffix ("· Merged" / "· Closed"); it now shows an open binding as a green pull-request icon, a merged binding as a purple merge icon, and a closed binding as a red closed-pull-request icon, with the visible text staying the bare PR label in the same muted tone as the badge text (the global link blue read as a competing color next to the merged icon's purple). On the session row itself, the PR number badge previously rendered the same accent-colored pill for every state and only dimmed merged PRs; it now renders the same three state icons on a neutral pill, so state is visible in the session list without hovering. Bindings whose state has not been fetched yet keep the neutral pull-request glyph with no color on both surfaces, and screen readers get the state name via sr-only text (tooltip) and the badge's aria-label. The icon/color mapping lives in one shared component used by both surfaces.
Why it's needed
Green open / purple merged / red closed is the state vocabulary GitHub users already know, and this feature exists precisely for people who bind GitHub PRs to sessions — GitHub itself uses three different glyphs for the three states. Encoding state in both the icon glyph and its color makes the bindings scannable at a glance and keeps the information accessible to color-blind users, where a uniform icon plus a trailing text suffix required reading every row, and a single accent pill conveyed no state at all. The badge matters most here: it is always visible in a list of 20+ sessions, so it answers "which sessions still have an open PR" without hovering. The same convention already exists elsewhere in the Web Shell (the GitHub PRs dialog colors open PRs green), so this also aligns the sidebar with existing in-app behavior.
Reviewer Test Plan
How to verify
With sessions bound to PRs in different states (open, merged, closed, and if possible a binding whose state has not been refreshed yet): (1) look at the session rows in the sidebar — the PR number badge should show a green pull-request icon for open, a purple merge icon for merged, a red closed-pull-request icon for closed, and a neutral uncolored icon when no state snapshot exists, all on the same neutral pill; (2) hover a session — the tooltip rows should show the same three icons, with no "· Merged" / "· Closed" text suffix. The collocated unit tests pin exactly this icon/class mapping on both surfaces and pass (10/10):
cd packages/web-shell && npx vitest run client/components/SessionPrBadge.test.tsx client/components/sidebar/SessionDetailsTooltip.test.tsx. Note: the state color rules intentionally use a doubled-class selector so they outrank the tooltip row's default muted icon color — worth a glance in the rendered UI that the icons are actually colored, not gray.Evidence (Before & After)
Session list badge — Before: the same accent-purple pill for every state (merged only dimmed). After: GitHub state icon on a neutral pill (open=green, merged=purple, closed=red, state-less=neutral).
Session details tooltip — Before: one neutral pull-request icon with a "· Merged" text suffix. After: purple merge icon and the bare PR label (sr-only keeps the state for screen readers).
Captured in Chromium against the Playwright mock-daemon harness with four bound PRs (open / merged / closed / state-less). Computed icon colors verified in the same run: badge open
rgb(72, 187, 120), badge mergedrgb(139, 92, 246), badge closed = the--destructivered, state-less neutral gray, and tooltip mergedrgb(139, 92, 246)— confirming the state colors outrank the tooltip row's default muted icon color. Unit tests:✓ components/SessionPrBadge.test.tsx (2 tests),✓ components/sidebar/SessionDetailsTooltip.test.tsx (8 tests).Tested on
Environment (optional)
Unit tests via vitest in packages/web-shell; no live daemon required.
Risk & Scope
Linked Issues
Follow-up polish to #9729.
中文说明
这个 PR 做了什么
绑定到 GitHub PR 的会话（#9729）现在在 Web Shell 侧栏展示绑定的两个位置都以 GitHub 的视觉词汇显示 PR 状态。会话详情悬浮提示（tooltip）中，此前每个绑定 PR 行使用同一个中性 pull-request 图标并以文字后缀（"· Merged" / "· Closed"）呈现状态；现在 open 绑定显示绿色 pull-request 图标，merged 显示紫色 merge 图标，closed 显示红色 closed-pull-request 图标，可见文本保持为纯粹的 PR 标签，并采用与 badge 文字一致的 muted 色调（全局链接蓝紫在 merged 图标的紫色旁边像另一种状态色）。会话行上的 PR 号 badge 此前对所有状态渲染同一个强调色药丸、仅对 merged 调灰；现在在相同的中性药丸上渲染同样的三个状态图标，无需悬停即可在会话列表中看到状态。尚未获取到状态快照的绑定在两个界面上都保持中性 pull-request 字形、不带颜色；屏幕阅读器通过 sr-only 文本（tooltip）和 badge 的 aria-label 获取状态名称。图标/颜色映射抽取为一个共享组件，两个界面共用。
为什么需要
绿 open / 紫 merged / 红 closed 是 GitHub 用户早已熟知的状态词汇，而这个功能恰恰服务于把 GitHub PR 绑定到会话的用户——GitHub 自己对三个状态使用的就是三个不同字形。把状态同时编码在图标形状和颜色上，让绑定一眼可扫读，也让色盲用户依然能区分状态——此前统一的图标加尾随文字后缀需要逐行阅读，单一强调色药丸则完全不含状态信息。badge 在这里最重要：它在 20+ 会话的列表中常驻可见，不用悬停就能回答"哪些会话还有 open 的 PR"。同样的约定在 Web Shell 其他位置已经存在（GitHub PRs 对话框把 open PR 标为绿色），因此这也让侧栏与应用内既有行为对齐。
评审者测试计划
如何验证
准备绑定了不同状态 PR（open、merged、closed，以及如有可能，一个状态尚未刷新的绑定）的会话：(1) 看侧栏会话行——PR 号 badge 应对 open 显示绿色 pull-request 图标、merged 显示紫色 merge 图标、closed 显示红色 closed-pull-request 图标、无状态快照时显示中性无色图标，药丸底色统一为中性；(2) 悬停会话——tooltip 行应显示同样的三个图标，且不再带 "· Merged" / "· Closed" 文字后缀。collocated 单元测试在两个界面上精确定住了这套图标/样式映射并通过（10/10）：
cd packages/web-shell && npx vitest run client/components/SessionPrBadge.test.tsx client/components/sidebar/SessionDetailsTooltip.test.tsx。注意：状态颜色规则有意使用了双写类选择器，以压过 tooltip 行的默认灰色图标颜色——建议在渲染后的 UI 里确认图标确实是彩色的而非灰色。证据（Before & After）
会话列表 badge——Before：所有状态都是同一个强调紫药丸（仅 merged 调灰）。After：中性药丸上的 GitHub 状态图标（open=绿、merged=紫、closed=红、无状态=中性）。
会话详情悬浮提示——Before：一个中性 pull-request 图标加 "· Merged" 文字后缀。After：紫色 merge 图标加纯 PR 标签（sr-only 为屏幕阅读器保留状态）。
在 Chromium 中基于 Playwright mock-daemon 设施截取，包含四个绑定 PR（open / merged / closed / 无状态）。同一次运行验证了图标计算颜色：badge open
rgb(72, 187, 120)、badge mergedrgb(139, 92, 246)、badge closed 为--destructive红、无状态为中性灰、tooltip mergedrgb(139, 92, 246)——确认状态颜色压过了 tooltip 行的默认灰色图标颜色。单测：✓ components/SessionPrBadge.test.tsx (2 tests)、✓ components/sidebar/SessionDetailsTooltip.test.tsx (8 tests)。测试平台
macOS ✅ 已测（单测）；Windows⚠️ 未测；Linux ⚠️ 未测（CI 覆盖三平台）。
环境（可选）
packages/web-shell 内的 vitest 单元测试；无需 live daemon。
风险与范围
关联 Issue
#9729 的后续打磨。