fix(web-shell): move the hover timestamp off the message text - #11380
Conversation
`MessageTimestamp`'s hover chip is absolutely positioned at the row's top-right with a 92%-opaque background, so it paints over the first line of the message underneath and lets the glyphs bleed through it. The first line of a wrapped message runs the full column width, so it is almost always the one that gets hit: in the web-shell visual preview for #11267 the turn-error copy "...before the answer finished." renders as "finisheu." beneath an 09:18:16 tip, which reads as a corrupted error message rather than as an overlay. Anchor the chip at the bottom-right instead and make its background opaque. The last line of a wrapped block is a ragged remainder, and rows that carry an action bar - the turn-error Copy button is one - leave that corner empty outright, so the common case no longer overlaps text at all; where it still does, an opaque chip with a border reads as an overlay instead of as broken characters. `.chatTip` and `.copyButton` share the same translucent background over content that `.chatActions` pulls up by -24px, so they get the same treatment. This makes overlap rare, not impossible - a message ending in a full-width code block or table still gets the chip in its corner. Reserving space in flow was rejected on purpose: adding padding on hover reflows the transcript, and a permanent float notch shrinks every code block that establishes its own formatting context.
The visuals job renders the PR head, then reinstalls dependencies and renders the merge-base, so the two passes photograph the same view minutes apart. Any view showing a wall-clock time therefore differed on every run for no reason other than when it was taken. On #11267 that was the entire preview: the only view the compose step reported as CHANGED was `terminal-turn-error-copy-narrow-dark` at exactly the 0.02% threshold, and the whole diff was a 09:09:28 timestamp against a 09:18:16 one. The narrow viewport is half the pixels of the wide one, which is why the same ~100 timestamp pixels cleared the threshold there and were skipped at 0.01% everywhere else - so the noise also crowded out any real change the PR made. Pin `Date.now()` / `new Date()` with `page.clock.setFixedTime` from `gotoSession` and `gotoNewSession`, which every visuals spec navigates through, including the Pixel 7 touch page. `setFixedTime` fakes only clock readings and keeps timers running, so replay, streaming and `freezeLoopingAnimations` are unaffected; elapsed-duration UI settles on a constant instead of drifting, which is the point. This is the same class of fix as `freezeLoopingAnimations`, and `harness.spec.ts` pins its contract the same way. `formatTimestamp` renders local time, so pin `timezoneId: 'UTC'` alongside it. CI runners are already UTC, so this changes no baseline there; it is what makes a local capture match them. Contexts created directly from the browser do not inherit `use` from the config, so the flow-recording context and the touch context repeat it.
|
|
|
Thanks for the PR — this one arrived with its evidence attached, so I checked the evidence instead of taking it on trust. Note on timing: the branch moved twice while this pass was running ( Template ✓ — every required section is present, including a Test Plan that names the visuals job as the oracle. The Problem: observed, and independently corroborated. None of the following rests on the description's word:
The one link I could not verify: outbound image fetches are blocked in this environment, so I never looked at the pixels themselves — the " Direction: aligned. Web-shell UI is a first-class surface with its own conventions section in AGENTS.md, and the visuals preview is review infrastructure — a preview that spends its one slot on a clock reading degrades the review of every future web-shell PR, not just this one. Reference signal: claude-code's CHANGELOG carries a Size: not applicable — no core-infrastructure paths ( Approach: the scope feels right and I would not split it. Two logically independent fixes would normally draw a "please split this" — but they are coupled in practice here: without the clock pin, this PR's own preview would judge the CSS move against timestamp noise, which is the exact failure being fixed. Landing them together is the defensible call. I also agree with rejecting in-flow space reservation: hover-time padding reflows the transcript, and a permanent float notch shrinks every code block that establishes its own formatting context. Putting The branch has grown two further changes since it opened, and both are worth a word:
One piece of hygiene, non-blocking but now worth fixing: the description has not kept up with the branch. It still opens "Two independent fixes" and never mentions Risk: no elevated risk signals — the changed paths do not match the revert-correlated set (re-run against the six-file list at this head). Moving on to code review. 🔍 中文说明感谢贡献！这个 PR 自带证据，所以我是去核对证据，而不是照单全收。 时间说明：本轮运行期间分支动了两次（ 模板 ✓ —— 必需章节齐全，Test Plan 也明确把 visuals job 当作验证依据。Risk & Scope 下没有逐字照抄 问题：已观测到，且可独立佐证。 以下没有一条依赖 PR 描述的自述：
唯一没能核实的一环：这个环境禁止外网取图，所以我没有真的看到那些像素 —— " 方向： 对齐。Web-shell UI 是一等界面，AGENTS.md 里有专门的约定章节；而 visuals preview 属于 review 基础设施 —— 一个把唯一名额花在时钟读数上的 preview，会拖累之后每一个 web-shell PR 的 review，不只是这一个。参考信号：claude-code 的 CHANGELOG 里恰好有针对同一"transcript 时间戳"界面的 规模： 不适用 —— 没有触及核心基础设施路径（ 方案： 范围合理，我不会要求拆分。两个逻辑上独立的修复通常会被要求"请拆开"——但在这里它们是实际耦合的：没有时钟固定，本 PR 自己的 preview 就会拿时间戳噪声来评判这次 CSS 移动，而那正是被修掉的故障。一起落地是站得住脚的选择。 我也认同排除"在文档流里预留空间"：hover 时加 padding 会让 transcript 重排，常驻的 float 缺口会让每个自成格式化上下文的代码块变窄。把 分支在开启之后又多了两处改动，都值得一说：
一点规范性问题，不阻塞但现在值得修：描述没跟上分支。它开头仍写"两个互相独立的修复"，从未提到 风险： 无升级风险信号 —— 变更路径未命中与 revert 相关的文件集合（已针对当前 head 的六文件列表重新跑过）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
🖼️ 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 |
The first pass froze captures at 2026-01-01, which is BEFORE dates the
fixtures hardcode. `formatRelativeTime` measures `Date.now() - value`, so the
channel editor's pairing requests, dated 2026-07-28, came out negative and
collapsed to "just now" where the real clock rendered "7/28/2026" -- visible
as the only unexplained CHANGED pair in this PR's own preview
(`channel-editor-existing-{dark,light}`).
Move the constant to 2027-01-01. Future-dating is the safe direction: every
fixture then reads as older than now, which is what all of them mean, and the
relative formatter lands in the same branch the live clock did.
`git-branch-picker` seeded `computedAt` from `Date.now()`, which in a spec is
Node's real clock rather than the page's frozen one. The status therefore
looked newer than the branch listing and armed a reconcile re-fetch that no
real session performs; it now derives from the same constant. Both rules are
written into the constant's docstring for the next fixture.
Code reviewIndependent proposal first. From the title and the "Why it's needed" section alone, I would have made the chip's background opaque to kill the bleed-through, and re-anchored it to a corner that collides less often than the top-right — the first line of a wrapped paragraph is the one line guaranteed to span the column, the last line is a ragged remainder. I would have rejected reserving space in flow, because hover-time padding reflows the transcript. For the capture noise I would have pinned the page clock in the shared navigation helpers, pinned the timezone beside it, and added a test that pins the contract. The PR does all of that and catches two things my proposal missed: No blockers. What I verified rather than assumed:
The third commit is the substantive part of this review, and it holds up. Freezing at I also checked that the new rules actually hold for the suite as it stands, which is the part a docstring can't assert for itself:
Two residuals, both non-blocking, neither a request to change this diff.
A small bonus the description doesn't claim: Enrichments skipped deliberately. No sequence diagram (this is a CSS re-anchor plus a harness clock pin, not a new multi-step runtime flow) and no changed-files table (six files, every one already named above). Testing evidenceThis was an unattended CI run ( Heads-up on the head I reviewed. The branch moved twice mid-pass, so this reflects This PR's own visual preview published while the pass was running, and it decomposes cleanly. It was rendered for the earlier head
An earlier commit's Classification — pre-existing, owned by the base commit:
Not verified, and why: the pixels themselves. Outbound image fetches are blocked in this environment, so I read the preview's changed-view list — CI-generated metadata — but never looked at a single composite image. That means "the chip now sits below the text and the error copy reads intact" is inferred from the four turn-error views changing plus the CSS I read, not seen; and the " Sandboxed verification would settle what the preview cannot: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Two PR workflow runs ( 中文说明代码审查先写独立方案。 只看标题和"为什么需要"，我会把 chip 背景改成完全不透明以消除字形透出，并把它从右上角挪到碰撞概率更低的角 —— 换行段落里第一行必然占满整列，最后一行是参差不齐的余量。我会排除在文档流里预留空间，因为 hover 时加 padding 会让 transcript 重排。至于截图噪声，我会在共享导航辅助函数里固定页面时钟、同时固定时区，并加一个把契约钉住的测试。这个 PR 全部做到了，而且抓到我方案里漏掉的两点： 没有阻塞项。 以下是我核实过、而非默认成立的：
第三个 commit 是本轮审查的实质部分，而它站得住。 固定在 我也核实了新规则对当前 suite 是否真的成立 —— 这是 docstring 自己无法保证的部分：
两点残留，均不阻塞，也不是要求改这个 diff。
一个描述里没有声称的小增益： 刻意省略的附加内容。 没有时序图（这是一次 CSS 重锚加 harness 时钟固定，不是新的多步运行时流程），也没有变更文件表（六个文件，上文都已点名）。 测试证据这是一次无人值守的 CI 运行（ 关于我所审查的 head。 本轮期间分支动了两次，所以这里对应的是 本 PR 自己的 visual preview 在本轮运行期间发布了，而且可以完整拆解。 它是为较早的 head
较早某个 commit 的 判定 —— 既有问题，归属 base commit：
未核实的部分及原因： 像素本身。本环境禁止外网取图，所以我读的是 preview 的变更视图清单（CI 生成的元数据），而一张合成图都没看过。这意味着"chip 现在位于文本下方、错误文案完整可读"是从四个 turn-error 视图发生变更加上我读过的 CSS 推出来的，不是看见的；而" 沙箱化验证可以补上 preview 补不了的部分： CI 表格只抓取一次、未轮询 —— 撰写时有两个 PR workflow run（ — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — clean across every stage and the evidence held up under independent checking; held back from 5 only because CI is still in flight and I never saw the rendered pixels. Stepping back: this is a small PR that did the hard part properly. My own independent proposal — opaque background, re-anchor to a corner that collides less, reject in-flow reservation, pin the clock in the shared navigation helpers — is a subset of what landed. The PR caught two things I missed on first pass ( That last part is why I'd maintain this without cursing anyone. A harness that freezes a clock is a harness that will silently break on the next fixture with a hardcoded date, and the failure shows up as a mystery CHANGED view in someone's preview six weeks from now. The docstring is what converts that from an investigation back into a rule. The author's own preview surfaced the What I actually checked, rather than accepted: the bottom-right corner the chip moves into is vacant (the only competing Four things I'd want the author to know, none of them blocking:
On the two audiences: a user hovering a message whose first line fills the column currently reads corrupted text, which is a real legibility bug and not a cosmetic preference; and maintainers reviewing web-shell PRs get their one preview slot spent on clock noise, which quietly degrades every future review of that surface. Both are worth fixing, and the fix is 38 lines of production change. Verdict: approve. CI was still running when this posted — two PR workflow runs ( 中文说明Confidence: 4/5 —— 各阶段都干净，证据在独立核对下站得住；没给到 5 分只因为 CI 仍在进行，而我始终没看到渲染出的像素。 退一步看：这是个小 PR，但把难的部分做对了。我自己的独立方案 —— 不透明背景、重锚到碰撞更少的角、排除在文档流里预留空间、在共享导航辅助函数里固定时钟 —— 是最终落地内容的子集。这个 PR 抓到了我第一遍漏掉的两点（ 最后这一点正是我日后维护它不会骂人的原因。一个冻结时钟的 harness，就是一个会在下一个带硬编码日期的 fixture 上静默出问题的 harness，而故障表现为六周后某人 preview 里一张来路不明的 CHANGED 视图。那段 docstring 把这件事从"需要调查"变回"有规则可循"。作者自己的 preview 暴露了 我真正核对过、而非照单接受的：chip 移入的右下角是空的（消息组件里唯一与之竞争的 四件希望作者知道的事，都不阻塞：
关于两类受众：用户 hover 一条首行占满列宽的消息时，现在读到的是损坏的文本，这是真实的可读性 bug，不是审美偏好；而维护者审查 web-shell PR 时，唯一的 preview 名额被时钟噪声花掉，这会悄悄拖累之后每一次对该界面的审查。两者都值得修，而修复只用了 38 行生产代码改动。 结论：批准。本文发布时 CI 仍在运行 —— 两个 PR workflow run（ — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Picks up main's Prettier fix for packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts, which this branch's base predates and which failed Lint & Static here without this branch having touched the file.
The visual harness injects a fake `turn_error` so the error row's Copy button can be photographed, but its copy was written to read like a real incident -- "The model provider closed the response stream before the answer finished. Retry the request or copy these details when reporting the failure." One test produces four screenshots (two themes x two viewports), so every web-shell preview shows that sentence four times, and readers keep taking the preview for a live failure rather than for mock data. It is the top question this preview generates. Nothing about the scenario needs realistic copy; it needs an error row. Say what the row is instead, and follow the string in the two locators that filter on it.
…tip move Three of the changes on this branch were not root-caused before being made, and this reverts them to what the reported symptom actually justifies. The tip no longer moves to the bottom-right. The symptom -- "finished." reading as "finisheu." -- comes entirely from the chip's 92%-opaque background letting the glyph beneath bleed through it, so the fix is the one word that makes it opaque. Repositioning was a second, bundled change: more correct in that it stops covering a word at all, but not what was reported, and it is what churned six screenshots. `.chatTip` and `.copyButton` carry the identical translucent background over content, so they change with it. The turn-error fixture keeps its original copy. Rewording it fixed no bug and measurably backfired: the touch pair started differing BECAUSE of the edit, so the preview went from four red screenshots to six, and the replacement text was a line longer. The red rows are the subject of the only visual test that hovers a message row -- the Copy affordance shipped in #10001 -- not evidence of a failure, and no wording makes them look otherwise. The `timezoneId: 'UTC'` pins go too. CI runners are already UTC, so they changed no baseline and only served a local reproduction nobody had asked for. The `harness.spec.ts` clock case goes with them: `freezeWallClock` is a one-line wrapper, so the case asserted that Playwright's own `setFixedTime` works. `freezeLoopingAnimations` keeps its case because that function has real logic to pin. What survives is what measurement supports: the opaque background, the frozen clock with its future-dated constant, and the `git-branch-picker` fixture that the freeze would otherwise desynchronise.
…tions The touch variant renders under `hover: none`, so `.row:hover > .tip` never fires and the hover timestamp is absent from those two screenshots entirely. A run that moved the chip and repainted its background scored both of them at 0% diff, which is direct evidence they carry no coverage of it -- they only contributed two full-height red error images to every web-shell preview. The behaviour they were meant to protect is the one #10001 shipped: on a touch device the row actions stay visible with no hover to reveal them. That is asserted directly (`toHaveCSS('opacity', '1')` plus a visible Copy button) and those assertions stay. Only the captures go. The four remaining turn-error captures stay for now: they are the only views in the suite where the hover timestamp lands on real text, so they are the sole visual evidence for the opacity fix on this branch.
…ssage Reverting the chip to the top-right was wrong, and zooming into the captured pixels is what showed it. The chip's lower edge cuts horizontally through the first line of the message; it does not cover the last word so much as decapitate it, removing a letter's ascender while the bowl stays visible underneath. That is the whole of the reported symptom: "finished." reads as "finisheu." because only the bowl of the d survives. An opaque background therefore cannot fix it. The faint remnant of the ascender currently showing through the 92% chip is the only hint the letter is a d at all -- painting the chip solid erases that hint and leaves a cleaner "u". The previous commit would have made the reported symptom slightly worse. Anchoring at the bottom-right is what removes the overlap: the last line of a wrapped block is a ragged remainder, and rows carrying an action bar leave that corner empty outright. Opacity is kept for its own reason -- a glyph bleeding through any overlay reads as broken text -- and the border makes the chip legible as an overlay where it does still land on content.
This scenario is the only visual test that hovers a message row, which it must do to reveal the Copy affordance shipped in #10001. The hover timestamp is `opacity: 0` until `.row:hover`, so those captures were the only place in the whole suite where a hover-timestamp change could appear -- and the six screenshots they produced meant every web-shell preview led with full-height red error images, whatever the PR had actually touched. Readers repeatedly took the preview for a live failure. They have now done the one job that justified keeping them: run 34231051803 proved the overlap fix at pixel level, with the d in "finished." regaining its ascender at full h-height while the base panel still shows "finisheu.". That evidence is recorded in the PR description, where it does not have to be re-rendered on every unrelated push. The behaviour stays pinned by assertion, not by image: seven opacity checks across the default viewport, the 720px viewport and the Pixel 7 touch context cover reveal on hover, reveal on focus, hide on blur, and always-visible actions under `hover: none`. A capture never verified any of that; Playwright does not diff these images, the preview bot does, and the preview is a review aid rather than a test. The scenario, the fixture and every assertion remain. Only the captures go.
yiliang114
changed the title
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 1c": did not read SubAgentPanel.tsx 's sub-tool row markup to confirm the row label can reach the right edge beneath .toolTimeTip — finding 1's overlap is verifie…; "agent 6a": no browser run — the ~494-day duration in the cockpit capture is computed from taskDuration 's source and the fixture constants, not observed in a rendered PNG…; "agent 6a": did not confirm whether AgentWorkflow is mounted in workflow-page.spec.ts 's captures (only in session-workflow.spec.ts 's cockpit, per that spec's own "sti…; "agent 1a": could not read the installed Playwright typings/source to confirm clock.setFixedTime semantics from the package itself — node_modules is absent in this revi…; "agent 1a": did not confirm whether the Node-side now: Date.now() in the mock daemon's tasks response ( e2e/utils/mockDaemon.ts:2002 ) reaches any rendered label in the…, and 3 more.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| * the row keeps it visible at the scroll edges. `pointer-events: none` lets | ||
| * clicks and inner hover interactions (tool expand, links) pass through. | ||
| * | ||
| * Anchored at the BOTTOM-right, and fully opaque. Both halves matter, and the |
There was a problem hiding this comment.
[Suggestion] This comment block now states two opposite anchors for the same chip. The sentence the diff leaves at line 11 still reads "Anchored inside the message's top-right corner rather than floating above it", the paragraph added here says "Anchored at the BOTTOM-right", and the declaration below is bottom: 2px. Line 11 is also the sentence carrying the clipping rationale that is still true (.list is overflow-y: auto, so the chip has to stay inside the row box), which is what makes it read as the block's thesis. Someone debugging chip placement takes it as ground truth, and "restoring" top: 2px to match the documented anchor puts the chip back over the first line of the message — reintroducing the finished. → finisheu. decapitation this PR was opened to remove, with the retained sentence as the authority for doing it.
Witness:
$ git show HEAD:packages/web-shell/client/components/MessageTimestamp.module.css | grep -n …
11: * Anchored inside the message's top-right corner rather than floating above
17: * Anchored at the BOTTOM-right, and fully opaque. Both halves matter, and the
45: bottom: 2px;
Dropping the corner from line 11 leaves the block stating one anchor and keeps the rationale that still holds:
* Anchored inside the message row rather than floating above it: the
* message list (`.list`) is `overflow-y: auto`, so a tooltip spilling
* outside the row box would be clipped or trigger a scrollbar.The overflow rationale at line 12 is the premise this reword must not lose — .list is overflow-y: auto, so a tooltip spilling outside the row box would be clipped or trigger a scrollbar.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| stashCount: 0, | ||
| operation: null, | ||
| computedAt: Date.now(), | ||
| // Node's real clock here would sit months from the page's frozen one |
There was a problem hiding this comment.
[Suggestion] The reason recorded here is the opposite of what the code does. FIXED_CAPTURE_TIME is 2027-01-01T09:00:00.000Z, so with the real clock at 2026-09 a Node-stamped computedAt would sit about 115 days below listingFetchedAt, which BranchPickerPopover.tsx:396 stamps from the frozen page clock — the status would look older, not newer than the branch listing. The gate at BranchPickerPopover.tsx:469 is if (at === undefined || at <= listingFetchedAt) return;, so the old Date.now() value short-circuited too, and the new value (exact equality) satisfies the same <=. The reconcile re-fetch the comment describes was already unreachable, and this fixture edit is inert today; the claimed mechanism only becomes true after January 2027, when real time passes the constant. The cost is that the next author reasons inverted about which side of listingFetchedAt a real-clock value falls on — "restoring" Date.now() believing it triggers a reconcile, or chasing a re-fetch in this capture that the guard makes unreachable.
Witness:
PROBE BASE fixture (Date.now() in Node): computedAt=1788882290761 listingFetchedAt=1798794000000 relation=computedAt<=listingFetchedAt workspaceGitBranchesCalls=1
PROBE PR fixture (FIXED_CAPTURE_TIME): computedAt=1798794000000 listingFetchedAt=1798794000000 relation=computedAt<=listingFetchedAt workspaceGitBranchesCalls=1
PROBE DISCRIMINATOR (FIXED + 60s): computedAt=1798794060000 listingFetchedAt=1798794000000 relation=computedAt>listingFetchedAt workspaceGitBranchesCalls=2
# three-arm probe mounting the real BranchPickerPopover with only Date faked;
# the discriminator arm proves the comparator is live, so 1/1 is a measured no-op
Keeping the edit and dropping the causal claim:
// Derived from the frozen capture instant per the harness rule: a value
// meant to be "now" must not come from Node's real clock. The page stamps
// listingFetchedAt from the frozen clock, so a real-clock value sits months
// below it and the reconcile branch is unreachable in captures either way.The wording has to stay consistent with the gate it describes — BranchPickerPopover.tsx:469, if (at === undefined || at <= listingFetchedAt) return;, with listingFetchedAt set at :396 from Date.now(); the guard is inclusive, so equality does not reconcile, and reconciledAtRef at :470 makes it once-per-status. If the intent is instead to keep the reconcile path exercisable in captures, that needs an explicit offset (FIXED_CAPTURE_TIME.getTime() + 1), not this value.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| * timestamp pixels cleared the threshold there and were skipped at 0.01% | ||
| * everywhere else. | ||
| * | ||
| * `setFixedTime` fakes only clock READINGS, not timers, so replay, streaming, |
There was a problem hiding this comment.
[Suggestion] This assurance is not accurate about the mechanism, and the part it omits is the part that will cost someone a debugging session. setFixedTime does not fake only clock readings: Playwright installs its clock with no toFake config, so the whole originals list is replaced — setTimeout/setInterval/requestAnimationFrame/requestIdleCallback/Date/performance/Intl (the 1.61.1 build actually used here also fakes AbortSignal). Timers and rAF do still fire and performance.now() still advances, so replay, streaming and freezeLoopingAnimations behave normally in outcome — but performance.mark/measure return throwaway FakePerformanceEntry objects and getEntries() comes back empty, which means the React-19 measure-stripping guard at client/index.html:65-88 wraps a stub in every capture and no capture can ever show a measure-storm regression. Separately, every Date.now()-delta window in the app is pinned permanently shut: useMessages.ts:672-680 (a 404ing background agent never reaches MISSING_BACKGROUND_AGENT_GRACE_MISSES, so it never flips to failed), session-catalog-store.ts:1411 and :389 (retry never due, snapshots never stale), workspace-session-live-state.ts:274 and :368-372 (retry and reconcile permanently throttled). No visual spec seeds those states today, so nothing fails now; the cost lands on whoever adds the first capture that needs one of those windows to settle — it hangs inside gotoSession and fails as a bare expect timeout while this docstring tells them timers, replay and streaming all behave normally, which at 3 AM reads as an app regression rather than a frozen clock.
Witness:
=== BASE (no page.clock) === === PR (page.clock.setFixedTime) ===
setTimeoutSource: native setTimeoutSource: (...args) => api[method].apply(api, args)
rafSource: native rafSource: (...args) => api[method].apply(api, args)
entriesAfterMark: 4 entriesAfterMark: 0
idleDeadline: { timeRemaining: 16.3 } idleDeadline: { timeRemaining: 0 }
dateNowDelta: 296 dateNowDelta: 0
timerFired: true rafFired: true timerFired: true rafFired: true
# and inside a real capture (gotoSession -> cockpit spec, Chromium):
{ "pwClockPresent": "object", "dateIsFake": true, "entriesAfterMark": 0, "measureEntries": 0,
"markSource": "(name) => new FakePerformanceEntry(name, \"mark\", 0",
"measureSource": "function () {\n var options = arguments[1];\n var devtools =\n o" }
Stating what the install does, and naming the exception:
* `setFixedTime` installs Playwright's full fake clock: `Date` readings are
* pinned, timers and `requestAnimationFrame` keep advancing in real time but
* are re-driven by the clock controller, `performance.mark`/`measure`/
* `getEntries*` are stubbed, and every `Date.now()`-delta window in the app
* (retry backoff, grace-miss counting, catalog staleness) is pinned shut and
* never elapses — a scenario that depends on one settling must seed
* already-expired timestamps or drive `page.clock.fastForward`/`runFor`.
If only Date readings need pinning, a narrower page.addInitScript overriding Date.now() and the no-arg constructor leaves performance, Intl, rAF and rIC native. Either way harness.ts:87's requirement stands — Date.now() and new Date() must both stay pinned, since client/utils/formatRelativeTime.ts:6 and client/utils/formatDateTime.ts:7 each read one — and freezeLoopingAnimations in this same file awaits two native requestAnimationFrame callbacks, which must keep firing. For reference, the timeouts a hung capture would hit are expect: { timeout: 10_000 } and test timeout: 60_000 in playwright.visuals.config.ts.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| * rendering an elapsed duration settles on a constant instead of drifting, | ||
| * which is the point. | ||
| */ | ||
| export async function freezeWallClock(page: Page): Promise<void> { |
There was a problem hiding this comment.
[Suggestion] freezeWallClock is the determinism step every capture now depends on, and nothing tests it. It runs only implicitly, from gotoSession/gotoNewSession, and no assertion anywhere in client/e2e/visuals/** reads the page clock — so dropping the call from either helper, moving it after page.goto, or adding a navigation helper that forgets it leaves all 45 visuals tests and all 6687 web-shell unit tests green while timestamped captures silently resume drifting between the base and head passes. That is the false-positive CHANGED preview documented in this function's own docstring, surfacing on every later PR as noise with no failing gate to point at the cause. harness.spec.ts:10-13 already states the house rule for exactly this shape: freezeLoopingAnimations "runs only implicitly via captureScreenshot, so pin its contract explicitly here".
Witness:
grep for time-of-day strings / 2027 / "just now" across client/e2e/visuals/**
-> matches only harness comments
npm test --workspace=packages/web-shell -> 288 files, 6687 tests passed
npx playwright test --config playwright.visuals.config.ts -> 45 passed (run twice at ed3e34c4)
# no assertion in either suite reads the page clock, and the compose step that
# consumes the determinism is non-blocking: "::warning::compose failed; publishing raw after-shots"
A case beside the existing one in harness.spec.ts would pin it:
await freezeWallClock(page);
expect(await page.evaluate(() => Date.now())).toBe(FIXED_CAPTURE_TIME.getTime());
await page.goto(url); // the persistence screenshots.spec.ts:832's split view relies on
expect(await page.evaluate(() => Date.now())).toBe(FIXED_CAPTURE_TIME.getTime());
await page.evaluate(() => new Promise((r) => setTimeout(r, 10))); // timers still runpage.clock is the browser-context clock rather than the page's (node_modules/playwright-core/lib/client/page.js:73: this.clock = this._browserContext.clock;), so the assertion has to be made on the page that gotoSession/gotoNewSession navigates — a second page in the same context seeing a frozen clock proves nothing about the helper having been called for it — and harness.spec.ts:29 depends on page.waitForTimeout(100) really elapsing, so the new case must not assert timers are frozen. Putting it in harness.spec.ts also keeps it off the base-arm allowlist (.github/workflows/web-shell-visuals.yml:177-179), so it adds no merge-base render time. The mutation that proves the pin works: remove page.clock.setFixedTime(...) from freezeWallClock, or move the freeze after the navigation, and the new case must go red — and if the mechanism is ever swapped for clock.install() + pauseAt(), the timer half must go red too.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| await captureScreenshot(page, `goal-usage-limited-${theme}`); | ||
| }); | ||
|
|
||
| // Assertions only, no captures. This scenario injects a fake turn_error so |
There was a problem hiding this comment.
[Suggestion] Removing these six captures leaves the chip this PR restyles with no observer anywhere in the repo. .tip is opacity: 0 until .row:hover, and after this change hover() appears nowhere else in client/e2e/visuals/, so no capture ever paints it; the assertions kept here all target [data-web-shell-message-actions] (a different component, SystemMessage.tsx:338), and MessageTimestamp.test.tsx asserts only the tip's existence, its HH:mm:ss text shape and the toolGroupSpacing class — never its geometry or its background. So reverting .tip to top: 2px, or restoring the 92% color-mix background, keeps both suites green, and because the compose step enumerates only afterNames, the deleted views simply disappear from the preview with no failure and no signal. The finished. → finisheu. decapitation this PR exists to fix could ship again the same way it was originally reported — through the preview — with nothing left to show it.
Witness:
npm test --workspace=packages/web-shell -> exit 0, 288 files / 6687 tests passed
npx playwright test --config playwright.visuals.config.ts -> 45 passed, twice at ed3e34c4
# no assertion in either suite reads .tip's geometry or background;
# a base-tree capture shows the chip "23:04:13" over the error row's first line at top-right,
# and no head capture contains a hovered chip.
# repo-wide grep: "terminal-turn-error-copy" survives only inside a harness.ts comment.
Keeping the full-viewport captures deleted and gating the chip where the hover already happens preserves both goals — after await errorRow.hover(), locate the chip (errorRow.locator('span[aria-hidden="true"]')), assert toHaveCSS('opacity', '1'), and assert from its boundingBox() that its bottom edge sits inside the row's bottom padding and does not intersect the message text box. If a pixel view is wanted as well, an element-scoped errorRow.screenshot() (or hovering a wrapped assistant row in the existing session transcript scenario) puts the chip back in the preview as a small strip instead of four full-height red pages.
Three existing facts bound the fix: this file at line 152 records that the preview "was dominated by four full-height red error images regardless of" the change, so full-viewport captures of this scenario must not come back; .github/scripts/web-shell-visuals-compose.mjs:107 (if (!hasBefore) return true;) publishes any newly added capture name unconditionally as NEW on every run, so keep it to one view — two images across themes, named <view>-<theme>.png per compose.mjs:11 — rather than the four removed; and .github/workflows/web-shell-visuals.yml caps an artifact directory at MAX_FILES=40 and deletes everything past the 40th file. Whichever form it takes, the guard needs a mutation that proves it: revert .tip to top: 2px or to the translucent color-mix and confirm the new assertion or capture goes red — today nothing in either suite does.
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
… clock Captures render at a fixed instant, so a fixture dated after it silently reads as "just now": `formatRelativeTime` measures `Date.now() - value`, a future value yields a negative age, and that lands in the `mins < 1` branch. This already happened on this branch. The first frozen constant sat at 2026-01-01, before the channel editor's 2026-07-28 pairing requests, and turned "7/28/2026" into "just now" across four views. Nothing failed -- no assertion covers that label -- and it was caught only by reading a preview diff by eye. Scan the visuals specs for ISO date literals and require each to precede FIXED_CAPTURE_TIME. The docstring already asked for this; a comment does not fail a build. `harness.ts` is skipped because it is where the constant itself is written, so its literal is the boundary rather than a fixture. The scan reaches further than the fixture that caused the incident: it covers eleven literals across the suite, eight of them a 2026-07-03 pair I had not been aware of. Verified in both directions -- silent against the tree as it stands, and flagging a planted 2027-06-01 literal. Not a general date linter. It catches ISO strings in fixtures, which is the shape the regression actually took; an epoch number is already opaque enough that nobody reads it as a date.
Lint & Static died on `Cannot find module .github/scripts/check-lint-gate-freshness.mjs`. The workflow definition comes from the merge ref and references that script; the checked-out tree was this branch's head, which is fourteen commits behind main and predates it. Merging main supplies the file.
… clock Captures render at a fixed instant, so a fixture dated after it silently reads as "just now": `formatRelativeTime` measures `Date.now() - value`, a future value yields a negative age, and that lands in the `mins < 1` branch. This already happened here. The first frozen constant sat at 2026-01-01, before the channel editor's 2026-07-28 pairing requests, and turned "7/28/2026" into "just now" across four views. Nothing failed -- no assertion covers that label -- and it surfaced only by reading a preview diff by eye. Scan the visuals specs for ISO date literals and require each to precede FIXED_CAPTURE_TIME. The constant's docstring already asked for this, and a docstring does not fail a build. `harness.ts` is skipped because it is where the constant itself is written, so its literal is the boundary, not a fixture. The scan reaches further than the fixture that caused the incident: eleven literals across the suite, eight of them a 2026-07-03 group that was not on my radar. Checked in both directions before committing -- silent against the tree as it stands, and flagging a planted 2027-06-01 literal. Not a general date linter. It catches ISO strings in fixtures, which is the shape this regression actually took; an epoch number is already opaque enough that nobody reads it as a date. b162c19 was meant to carry this and landed empty: the scratch copy of the file was overwritten by a later extraction step before it was hashed, so the commit recorded no change while its message described one. History is append-only on an open PR here, so it stays in the log; this commit is the content.
github-actions
Bot
added
the
review/self-reported
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-1 comment block states two opposite anchors for the chip — still stands, already reported (comment 3959980405)
- R1-2 recorded reason for the computedAt change is inverted — still stands, already reported (comment 3959980430)
- R1-3 setFixedTime docstring overstates what stays normal — still stands, already reported (comment 3959980440)
- R1-4 freezeWallClock has no contract test — still stands, already reported (comment 3959980451)
- R1-5 restyled chip left with no deliberate observer — still stands, already reported (comment 3959980459)
2 candidate finding(s) this round's reviewers re-derived matched entries already carried on this PR and were set aside before verification (R1-1, R1-4) — a matched posted finding is ruled in the previous-round status as always, and a matched deferral stays on the standing deferral record.
Not explored to full depth (tool budget reached): "agent 1a": whether the workflow/tasks captures render task ages from the mock daemon's Node-stamped now: Date.now() (mockDaemon.ts:2002) against the page's frozen clock …; "agent 6b": I did not confirm whether the mock daemon's Node-derived times ( mockDaemon.ts:2002 now: Date.now() in the tasks response, :2246 in reduceMockGoal ) reach…; "agent 6b": I did not render the bottom-right chip in a browser, so finding 4's overlap is derived from the CSS/JSX box geometry only, not observed.; "agent 5": whether mockDaemon.ts:2002 's now: Date.now() and :2246 's timestamp reach a *rendered* label in a visuals capture — I traced the tasks handler only as …; "agent 3b": none — every check above ran to completion..
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/e2e/visuals/screenshots.spec.ts:150 — [review] the comment's claim that turn-error is the only place a hover-timestamp change could show up is false: parallel-agents-expanded paints the chip via residual hover from…
— qwen3.8-max via Qwen Code /review (v0.23.1)
| // nothing failed, and it was caught only by reading a preview diff by eye. | ||
| test('every hardcoded fixture date precedes the frozen capture clock', () => { | ||
| const here = dirname(fileURLToPath(import.meta.url)); | ||
| const sources = readdirSync(here).filter( |
There was a problem hiding this comment.
[Suggestion] The new guard is named every hardcoded fixture date precedes the frozen capture clock, but it scans only this directory and matches only quoted YYYY-MM-DDTHH:MM:SS[.mmm][Z] literals — so the fixture date the captures actually render escapes it, as do three other ways of writing one.
client/e2e/utils/mockDaemon.ts:200 holds const now = '2026-07-03T00:00:00.000Z' and stamps createdAt/updatedAt/generatedAt on the default scenario's session summaries. screenshots.spec.ts:56-84 installs that scenario with no sessions override, so those rows render into session-transcript-<theme>.png through SessionRow.tsx:102 and formatRelativeTime, whose first branch is if (mins < 1) return t('time.justNow'). A routine fixture refresh that dates now at or after 2027-01-01T09:00:00Z therefore collapses every one of those labels to "just now" while expect(offenders).toEqual([]) stays green, because readdirSync(here) never opens that file. The same blind spot covers epoch-millis fixtures inside this directory (const T0 = 1_756_100_000_000; at workflow-page.spec.ts:23 and session-workflow.spec.ts:32 — the numeric separators also hide it from a \d{13} pattern), date-only ('2027-06-01') and offset-suffixed ('…+08:00') literals the regex cannot match, and the harness docstring's second rule (harness.ts:80-82: a value meant to be "now" must derive from the constant rather than Date.now()), which is enforced nowhere and had to be hand-applied at git-branch-picker.spec.ts:59. That is the silent regression this test's own comment says it exists to end.
Either put the invariant at the producer — move FIXED_CAPTURE_TIME into a leaf module both layers can import and derive mockDaemon's fixture clock from it — or widen the scan:
const roots = [here, join(here, '..', 'utils')];
const sources = roots.flatMap((dir) =>
readdirSync(dir).filter(
(file) =>
file.endsWith('.ts') && join(dir, file) !== join(here, 'harness.ts'),
),
);The exclusion has to become path-based rather than a bare filename once more than one directory is scanned.
Witness:
probe on a byte copy of packages/web-shell/client, driving this file unmodified:
ARM 0 pristine -> 1 passed (694ms)
ARM 1 utils/mockDaemon.ts:200 = '2027-06-01T00:00:00.000Z' -> 1 passed (603ms) <- guard blind
ARM 2 the same future date inside visuals/screenshots.spec.ts -> 1 failed
harness.spec.ts:66:1 > every hardcoded fixture date precedes the frozen capture clock
> 90 | expect(offenders).toEqual([]); - Array [] + Array [ ...1 offender... ]
ARM 2 proves the test is live, so ARM 1's pass is directory scope, not pattern scope.
sweep with the guard's own regex over the real files: 11 matches in visuals/screenshots.spec.ts
(all pre-boundary), 1 match in utils/mockDaemon.ts -- a file readdirSync(here) never opens --
and 0 matches for either T0 epoch fixture.
Two existing facts bound the fix. FIXED_CAPTURE_TIME's own literal at harness.ts:84 is the boundary and the comparison is >=, so that file must stay excluded, as this test's comment at lines 69-70 already records; and a widened scan must keep green the non-fixture Playwright clock installations at web-shell.workspace-overview.spec.ts:377-378 and web-shell.live-state-poll-interval.spec.ts:53-54, and must not start flagging dates quoted in prose (line 64 of this file carries 2026-07-28 inside a comment). The acceptance criterion is the mutation in the witness above: set mockDaemon.ts:200 to a post-2027 date and confirm this test goes red — today it stays green.
— qwen3.8-max via Qwen Code /review (v0.23.1)
| * | ||
| * Opacity still matters on its own: at 92% a glyph beneath any overlay bleeds | ||
| * through it, which reads as broken text rather than as a chip. `.chatTip` and | ||
| * `.copyButton` sit over content too (`.chatActions` pulls itself up by -24px) |
There was a problem hiding this comment.
[Suggestion] This paragraph declares a sweep of the overlays that sit over content — ".chatTip and .copyButton sit over content too … and carry the same fix" — but a third chip, documented as mirroring this one, was not swept. .toolTimeTip in client/components/messages/tools/SubAgentPanel.module.css:206-218 keeps the exact geometry the block above condemns (top: 2px; right: 4px; z-index: 5, over the row's first line) and declares no background at all, which is strictly worse than the 92%-translucent chip this diff calls "broken text rather than as a chip".
The overlap is structural rather than speculative. SubToolTime (SubAgentPanel.tsx:95-99) puts the tip inside .toolTimeRow (position: relative) wrapping ToolLine; ToolChrome.module.css:230 gives .line padding: 2px 0 with no horizontal padding, so the header's content box reaches the row's right edge, where ToolGroup.tsx:605-606 render .lineArg (flex: 1; overflow: hidden; text-overflow: ellipsis; white-space: nowrap) and .lineElapsed (margin-left: auto; flex-shrink: 0, non-empty for kind === 'agent' or any active status per ToolGroup.tsx:617-621 — that is every running sub-tool, independent of description length). Vertically the tip occupies roughly y=2-17px of a ~20px single-line row, so two texts composite with nothing between them. It is reachable in the app surface and not only the embeddable one: ToolGroup.tsx:2029 passes hideHeader, so :1365-1367 renders <SubAgentPanel hideHeader defaultExpanded inline /> down through SubAgentTools and SubToolLine to SubToolTime. The symptom this PR exists to end — a timestamp reported as corrupted text in the transcript — keeps arriving from Tools rows, and the PR's own verification step ("Hover a transcript message whose first line fills the column width. No character is covered") passes while the nested row beside it still covers characters. After this diff the two mirroring chips also sit at opposite corners with different opacity.
Give the nested chip the same two halves, in place:
.toolTimeRow > .toolTimeTip {
position: absolute;
bottom: 2px;
right: 4px;
z-index: 5;
color: var(--muted-foreground);
background: var(--background);
/* …the rest unchanged… */
}Witness:
witness: not run -- a browser render of a hovered sub-tool row would have measured the pixel
intersection, and it was the closest capability. `qwen review scratch-tree` returned
available: false (the repo-local git config carries two dangling includeIf.gitdir pointers, which
that command refuses to create checkouts under); no visuals capture hovers any row after this diff
(the only .hover() sites in client/e2e/visuals/*.ts are screenshots.spec.ts:177,183, and this diff
removes the captureScreenshot calls that followed them); and no sub-agent fixture exists to drive
one from (grep subTools|subContent over client/e2e/utils/*.ts and visuals/*.ts -> 0 hits).
The declarations and markup quoted above were instead read verbatim at the reviewed commit e85c1dfb.
The edit has to stay inside its own class pair: SubAgentPanel.module.css:196-200 records that it is "Deliberately a separate class pair from the message-level MessageTimestamp (.row/.tip)" because the Tools list nests inside a message already wrapped by MessageTimestamp — that rationale argues against sharing the hover rule across the two layers, not against giving the nested chip a background and a bottom anchor. Nothing in the repo pins either chip's CSS today (SubAgentPanel.test.tsx:108-132 asserts only that formatTimestamp(startTime) appears in the row text, and after this diff no capture hovers a row), so if you take this it is worth adding a hover assertion or an element-scoped capture on a sub-tool row that goes red when the tip is put back over the text — today no test does.
— qwen3.8-max via Qwen Code /review (v0.23.1)
| // falls into the `mins < 1` branch. Not hypothetical -- an earlier constant | ||
| // turned the channel editor's 2026-07-28 pairing requests into "just now", | ||
| // nothing failed, and it was caught only by reading a preview diff by eye. | ||
| test('every hardcoded fixture date precedes the frozen capture clock', () => { |
There was a problem hiding this comment.
[Suggestion] This invariant has no gating CI home. The only config that collects it feeds one workflow step that cannot fail a check, so a violation reaches nobody as a red build.
Asking each real collector which files it sees: vitest.config.ts:28 excludes e2e/**, so npm run test:ci:workspaces (ci.yml:764) never collects this test; playwright.config.ts:11 sets testIgnore: '**/visuals/**', so npm run test:e2e:smoke (ci.yml:1496) never collects it either. The only config that does is playwright.visuals.config.ts, whose sole workflow consumer is web-shell-visuals.yml — and that step (id: after_capture, :107-112) is continue-on-error: true, deliberately, per :96-106: "a single failing/timing-out scenario must NOT discard the whole preview … The job stays green". The base arm at :185 passes an explicit four-spec allowlist whose comment at :177-178 says this file "stay[s] out".
So when someone dates a fixture past FIXED_CAPTURE_TIME: the guard fails, steps.after_capture.outcome is failure, the job still concludes success, web-shell-visuals-publish.yml:40 publishes because it gates on that conclusion, and the line it prints is "One or more scenarios failed to render" — a message about rendering, not about a date invariant — while Playwright keeps running the remaining files and those captures still publish with every relative label collapsed to "just now". That is the same silent outcome this test's own comment says it exists to prevent.
The check is pure Node filesystem work with no browser dependency, so it can run where it gates — as a vitest case in the web-shell package suite, which test:ci:workspaces already runs. If it stays here, the comment should record that a violation surfaces as "One or more scenarios failed to render" in the preview comment rather than as a red check, so nobody assumes it blocks.
Witness:
vitest list --config vitest.config.ts -> 6712 test names; grep -i 'harness|visuals' -> none
playwright test --config playwright.config.ts --grep @smoke --list
-> 58 tests in 10 files; grep -c 'visuals/' -> 0
playwright test --config playwright.visuals.config.ts --list
-> 46 tests in 6 files, including
harness.spec.ts:66:1 > every hardcoded
fixture date precedes the frozen capture clock
grep -rl 'test:e2e:visuals|playwright.visuals.config' .github/workflows
-> web-shell-visuals.yml (only)
web-shell-visuals.yml:107-112 -> id: after_capture, continue-on-error: true
web-shell-visuals-publish.mjs:230/251/267 -> "One or more scenarios failed to render"
Any relocation has to avoid the step that is continue-on-error on purpose: web-shell-visuals.yml:107-112 exists so that "a single failing/timing-out scenario must NOT discard the whole preview", so routing a gating check through it would either defeat that intent or inherit its non-gating behaviour.
— qwen3.8-max via Qwen Code /review (v0.23.1)
… gating home Review found eight issues on this branch. Three were comments asserting things the code does not do, and comments that confidently state the opposite of the behaviour are worse than no comment. The CSS block kept its original opening sentence, "Anchored inside the message's top-right corner", above a paragraph declaring the BOTTOM-right anchor and a `bottom: 2px` rule. The retained sentence also carries the overflow rationale that is still true, so it reads as the block's thesis -- someone "restoring" `top: 2px` to match it puts the chip back over the first line and reintroduces the exact decapitation this branch exists to remove. Only the corner is dropped; the overflow reason stays. The `git-branch-picker` fixture comment claimed a Node-stamped `computedAt` would look NEWER than the branch listing and arm a reconcile re-fetch. The constant is future-dated, so a real-clock value sits months BELOW `listingFetchedAt`, and the gate is `at <= listingFetchedAt` -- the branch was already unreachable and the edit is inert today. The comment now says what the edit is for (the harness rule about "now" values) and what would actually exercise the reconcile path. The `freezeWallClock` docstring said `setFixedTime` fakes clock readings and not timers. It installs Playwright's full fake clock: timers, rAF, rIC, `performance` and `Intl` are all replaced. Outcomes are unaffected today, but `performance.mark`/`measure` are stubbed and every `Date.now()`-delta window in the app is pinned permanently shut. Nothing seeds those states, so nothing fails -- until a scenario needs one to elapse and hangs as a bare expect timeout while the docstring insists timers behave normally. `.toolTimeTip` in SubAgentPanel mirrors this chip over a sub-tool line, was missed by a paragraph claiming to sweep the overlays, and was strictly worse: same `top: 2px` geometry, no `background` at all, so two texts composited directly. It gets the same treatment, and the paragraph now names it. The remaining three findings share a root: nothing observes any of this. `.tip` is `opacity: 0` until hover and no capture paints it any more, so reverting the anchor or the background keeps every suite green. `freezeWallClock` runs only implicitly and no assertion reads the page clock, so dropping the call resumes the drift silently. And the date guard sat in `harness.spec.ts`, which no gating config collects -- `vitest.config.ts` excludes `e2e/**`, `playwright.config.ts` ignores `visuals/**`, and the one workflow step that does run it is deliberately `continue-on-error: true` so a failing scenario cannot discard the preview. `visual-capture-contracts.test.ts` is a plain vitest file outside `e2e/`, collected by `test:ci:workspaces`, pinning all three: fixture dates precede the frozen clock, both navigation helpers freeze before they navigate, and every hover chip stays bottom-anchored and opaque. The date scan also now reads `e2e/utils`, where `mockDaemon.ts` holds the date the default scenario's session rows actually render -- scanning only `visuals/` missed it, which is what the review caught. Its reach is still ISO string literals; the comment says so rather than implying the name's full promise. `FIXED_CAPTURE_TIME` moves to `constants.ts` so the guard can import it without pulling in `@playwright/test`. Each guard was checked in both directions before committing: silent against the tree as it stands, and firing on a planted 2027-06-01 date in `mockDaemon.ts`, a `freezeWallClock` moved after `page.goto`, and a `.tip` reverted to `top`.
Local verification report — real browser, merge-base A/BVerified Verdict: the three claims hold, and two of them hold more strongly than the description says. Recommend merge. Two optional follow-ups and one factual correction to the PR body are at the end; none of them block. 1. The overlap, and the counter-intuitive partTo measure this rather than eyeball it I rendered a fourth arm as ground truth — the same build with
At the wide viewport the column puts three ascenders under the chip, not one —
The touch claim is exact. The two touch captures are byte-identical across all three CSS arms ( Regression sweep. Five body shapes × 2 themes, each hovered, same ground-truth oracle (toolbar band excluded — it carries an unrelated nondeterministic gauge needle): merge-base obscures glyphs only in the wrapped-prose shape (95–97 px); this PR obscures 0 in every shape. I also appended a full-width trailing block into a hovered row and measured the chip ∩ block rectangle directly: merge-base 482 px², this PR 0. I could not construct the residual overlap the PR documents, because every 2. The clock: it does cross the threshold, not just approach itHolding the code and CSS completely fixed and varying only the wall clock the capture renders at:
The first row reproduces the numbers quoted in the description, including why narrow crosses while wide is skipped. The second shows this is not a near-miss: with an ordinary CI time gap the clock alone manufactures a CHANGED view and puts a red error screenshot in front of a reviewer who changed nothing.
Run-to-run, 4 full suite runs per arm. Byte noise on the turn-error views: merge-base 198–226 px, PR head 0–14 px (the residual 14 is the toolbar gauge needle, 0% after denoise). Byte-stable views overall: 40/69 → 46/69. Under the compose oracle no turn-error view is CHANGED by clock noise at PR head. Timers do keep running. All 46 visuals tests pass at PR head, including the replay, streaming and flow-video specs and the existing 3. Repo checks at PR head
The surviving turn-error assertions are load-bearing, not decorative — I mutated 4. One correction to the description (in the PR's favour)
Not quite. So the hover chip keeps capture coverage after the turn-error shots go. Worth fixing the sentence, because the coverage story is better than the PR claims.
5. Two optional follow-ups(a) The new guard enforces one of the two rules its docstring states. Mutation matrix, each literal added to a scanned spec:
The last one is the one that matters: the Scope is also narrower than it looks: (b) The 6. Pre-existing flake this PR does not address (not a blocker, not caused here)Running the same arm twice and scoring with the compose oracle still reports CHANGED views, in both arms:
The cockpit diff is a few-pixel horizontal layout shift, not a time label — a settle/layout race, unrelated to the clock. So the narrow claim in the test plan ("no view CHANGED purely because of a clock reading") is exactly right and now holds, but the preview will not be quiet until that one is chased too. Worth its own issue. 中文版报告本地验证报告 — 真实浏览器 + merge-base A/B在两个独立 worktree（各自 结论：三项主张全部成立，其中两项比描述里说的还要更强。建议合并。 文末有两条可选后续项和一处对 PR 描述的事实更正，均不阻塞。 1. 遮挡问题，以及其中反直觉的那一半为了「测量」而不是「目测」，我额外渲染了一个基准真值臂：同一构建但
宽视口下这一列会把三个上升部压在 chip 底下， 触屏那条主张完全准确。 两张 touch 截图在三个 CSS 臂之间逐字节相同（ 回归排查。 5 种正文形态 × 2 主题，逐一 hover，同一基准真值 oracle（排除工具栏条带 —— 那里有个与本 PR 无关的、非确定性的仪表指针）：merge-base 只在 wrapped-prose 形态遮挡（95–97 px）；本 PR 在所有形态下都是 0。我还往悬停行里追加了一个满宽尾块，直接测 chip ∩ 尾块的矩形交集：merge-base 482 px²，本 PR 0。PR 自己记录的残余遮挡场景我没能构造出来 —— 视觉 fixture 能触达的每个 2. 时钟：不是逼近阈值，是真的越过固定代码与 CSS，只改截图渲染时的墙上时钟：
第一行复现了描述中引用的数字，也复现了「为什么 narrow 越线而 wide 被跳过」。第二行说明这不是擦边：在寻常的 CI 时间间隔下，仅凭时钟就能凭空造出一个 CHANGED 视图，把一张满屏红色报错图推到一个什么都没改的评审者面前。 每臂 4 轮完整套件的轮间噪声。 turn-error 视图的逐字节噪声：merge-base 198–226 px，PR head 0–14 px（残留的 14 px 是工具栏仪表指针，降噪后为 0%）。整体逐字节稳定的视图数：40/69 → 46/69。在 compose oracle 下，PR head 已无任何 turn-error 视图因时钟噪声被判 CHANGED。 定时器确实仍在运行。 PR head 上 46 个 visuals 测试全通过，包含 replay、streaming、flow 录像用例以及既有的 3. PR head 上的仓库检查
保留下来的 turn-error 断言是「承重」的，不是摆设：我对 4. 对描述的一处更正（对 PR 有利）
并非如此。 也就是说，删掉 turn-error 截图后，悬停 chip 仍保有截图覆盖。这句话值得改一下，因为实际的覆盖情况比 PR 自述的要好。 5. 两条可选后续项(a) 新守卫测试只落实了它文档里两条规则中的一条。 变异矩阵（把字面量加进被扫描的 spec）：
最后一条最要紧：本 PR 修的 扫描范围也比看起来窄： (b) 6. 本 PR 未处理的既有抖动（不阻塞，也非本 PR 引入）把同一个臂跑两遍再用 compose oracle 判定，两个臂都仍会报出 CHANGED：
cockpit 的差异是几像素的水平位移，不是时间标签 —— 属于布局/稳定竞态，与时钟无关。所以测试计划里那句窄口径的主张（「不会再有视图纯粹因为时钟读数被判 CHANGED」）完全正确且现在成立；但在这一项也被追查之前，预览仍不会安静。建议单开 issue。 |
Moving `.toolTimeTip` to the bottom of its row was inert, and the comment saying it 'inherits its fix for the same reason' overstated it the same way three comments this branch already had to correct. The row has no vertical slack to move into. `.line` is `padding: 2px 0` around 13px text, so roughly 17-24px tall, and `.lineArg` is `white-space: nowrap` so it never wraps to a second line. The chip is about 19px once padding and a border are added -- as tall as the row it sits in. `bottom: 2px` therefore lands where `top: 2px` already was, and the padding and border I added only made the chip taller than the row on the narrow end of that range. What was actually wrong there is that the chip declared no `background` at all, so its digits overprinted the row's text rather than covering it. That is the whole fix: one declaration, no geometry change. The text under the chip is still covered -- cleanly now instead of overprinted -- and the comment says so rather than implying the overlap is gone. `.tip` keeps its bottom anchor, which does work: a message wraps, so its last line is a ragged remainder and rows carrying an action bar leave that corner empty outright. The contract test splits along the same line: every chip must be opaque, only `.tip` must be bottom-anchored. Asserting a bottom anchor for a row that has nowhere to put one would have pinned a cosmetic no-op as if it were the fix.
The comment left where the captures were removed said this scenario's screenshots were "the only place a hover-timestamp change could show up". That is false, and this PR's own preview disproves it: the parallel agents test leaves the cursor on the group header after `summary.click()`, so `parallel-agents-expanded` paints the chip through residual hover -- and it is the only view still reported as CHANGED here, at 0.08%, precisely because the chip moved. The distinction that survives is deliberate versus incidental. This test is the only one that hovers on purpose; the parallel agents coverage is a side effect of a click and would vanish the moment that test stopped clicking. Neither is what pins the chip now -- visual-capture-contracts.test.ts is.
|
All nine findings from rounds 1 and 2 are addressed, including the deferred one. Each was verified against the code before acting rather than taken on trust — and every one of them held. Three of them were comments asserting the opposite of what the code does. Those are worse than no comment: R1-1's retained sentence also carried the still-true overflow rationale, so it read as the block's thesis, and "restoring"
R1-1 / R1-2 / R1-3 — comments corrected. Only the corner is dropped from R1-1's sentence; the overflow reason it also carries stays. R1-2 now says what the edit is actually for (the harness rule about "now" values) and notes that the reconcile branch is unreachable either way, since a real-clock value sits below a future-dated R1-4 / R1-5 / R2-3 — one root cause, one fix. Nothing observed any of this:
R2-1 — the gap was real and is closed. The scan now reads R2-2 — the finding was right, and my first fix overstated itself. Deferred finding — accepted. The comment claimed this scenario's captures were the only place a hover-timestamp change could appear. This PR's own preview disproves it: Not claimed: the clock freeze cannot be demonstrated on this PR. The base pass still renders from main, without it, so the two sides necessarily differ. It is provable only once this lands, and the Reviewer Test Plan says so. 中文两轮 review 的 9 条意见（含 deferred 那条）全部处理完毕。每条都先对照代码核实再动手，结果全部成立。 其中 3 条是注释在陈述与代码相反的事实 —— 这比没有注释更危险：R1-1 被保留的那句同时承载着仍然成立的 overflow 理由，会被当成整段的论点，后人"照注释修正"回
R1-1 / R1-2 / R1-3 —— 注释已改正。 R1-1 只删掉"右上角"，仍然成立的 overflow 理由保留。R1-2 改为说明这次改动的真实目的（harness 关于"now"值的规则），并指出 reconcile 分支两种取值下都不可达 —— 真实时钟的值低于未来化的 R1-4 / R1-5 / R2-3 —— 同一个根因，同一个修复。 这些改动没有任何东西在观察：
R2-1 —— 缺口真实存在，已补。 扫描范围现在包含 R2-2 —— 意见成立，而我的第一版修复自己也夸大了。 deferred 那条 —— 接受。 该注释宣称这个 scenario 的截图是唯一能显现悬浮时间戳改动的地方。本 PR 自己的 preview 就是反证： 未作声明的一点：冻结时钟无法在本 PR 上被证明。base 那一趟仍从 main 渲染、不含该改动，所以两侧必然不同。只有合并之后才可验证，Reviewer Test Plan 里已写明。 |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Approval blockers: none.
Scope
Reviewed all 7 changed files at HEAD (358be30). No working tree — execution tier not run. Cross-file: BranchPickerPopover.tsx not read (see below). CI config not read — CI gating claim for visual-capture-contracts.test.ts accepted on the file's own comment.
What was checked
CSS (MessageTimestamp.module.css, SubAgentPanel.module.css): .tip moves from top: 2px to bottom: 2px; .chatTip, .copyButton, .toolTimeTip all gain background: var(--background). Matches stated intent: move chip off first text line, make all chip backgrounds fully opaque.
Clock freeze (harness.ts): freezeWallClock inserted before page.goto in both gotoSession and gotoNewSession. FIXED_CAPTURE_TIME set to 2027-01-01, future-dating all 2026 fixture dates so they read as old rather than "just now".
Contract tests (visual-capture-contracts.test.ts): Three tests verified:
- ISO-literal fixture dates before FIXED_CAPTURE_TIME — scope limitation (epoch-millis/date-only strings) acknowledged inline.
- Both nav helpers call freezeWallClock before page.goto — correct indexOf ordering in source slice.
- .tip must carry bottom: anchor and var(--background); .toolTimeTip must carry var(--background) — CSS is read from disk at test time.
Minor finding (body only)
F1 — visual-capture-contracts.test.ts line 115: The chip-opacity contract covers .tip and .toolTimeTip but not .chatTip and .copyButton, both of which receive the same background fix in this PR. Reverting their backgrounds would not be caught by the test. Low impact — those chips appear in existing screenshot coverage and the regression was originally reported through preview inspection, not automated assertion.
Cross-check against prior reviews
Rounds 1-2 (qwen-code-ci-bot, SHAs ed3e34c / e85c1df, all Suggestion severity):
- R1-1 (comment said "top-right" and "bottom-right"): Fixed at current HEAD — the old "top-right corner" text was replaced with "inside the message row".
- R1-2 (computedAt comment reason claimed inverted): Cannot rule — the comment states the real clock is "months BELOW the future-dated constant" (which would be correct given today is 2026-09 and the constant is 2027-01), but tracing BranchPickerPopover.tsx:396 would require reading a file not in the diff. Plausibly correct; bounded impact if wrong.
- R1-3 (setFixedTime docstring accuracy): Reviewed — the docstring explicitly enumerates setTimeout, setInterval, rAF, requestIdleCallback, performance, Intl as replaced, then notes two concrete behavioural consequences. Does not appear to overstate; no material omission found.
- R1-4 (freezeWallClock has no contract test): Fixed — visual-capture-contracts.test.ts test 2 checks both helpers freeze before navigating.
- R1-5 (restyled chip has no deliberate observer): Fixed — visual-capture-contracts.test.ts test 3 checks .tip anchor and background at every test run.
- R2-2 (.toolTimeTip not swept): Fixed — SubAgentPanel.module.css adds background: var(--background) to .toolTimeTip.
- R2-3 (contract test CI gating): Cannot rule — CI config not read. File comment claims collected by npm run test:ci:workspaces which gates; no counter-evidence found.
Unreviewed dimensions
- Execution tier not run (no working tree).
- BranchPickerPopover.tsx not read — R1-2 cannot be fully settled.
- mockDaemon.ts epoch-millis timestamps (now: Date.now() at line ~2002) reaching rendered labels: flagged by prior reviewers, not traced here.
- CI config not read for R2-3.
Reviewed with AI assistance.
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
What this PR does / 本 PR 做了什么
Two things, both in the web-shell visual layer. (1) The hover timestamp chip (.tip) moves from top: 2px to bottom: 2px and becomes fully opaque (var(--background) + a 1px border), because at the top-right its lower edge cut horizontally through the first line of a wrapped message — the one line that reliably spans the full column — decapitating a letter's ascender so "finished." read as "finisheu." and kept being reported as a corrupted error message. The same opaque background is applied to .chatTip and .copyButton (both sit over content via .chatActions' -24px pull-up) and to SubAgentPanel's .toolTimeTip, which previously had no background at all. (2) The visuals pipeline now renders at a frozen clock (FIXED_CAPTURE_TIME), and the turn-error scenario stops producing screenshots while keeping every assertion.
两件事，都在 web-shell 的视觉层。（1）悬停时间戳小条（.tip）从 top: 2px 移到 bottom: 2px 并改为完全不透明（var(--background) + 1px 边框），因为在右上角时它的下缘会横切过一条换行消息的首行 —— 而首行正是可靠地占满整列宽度的那一行 —— 把字母的上伸部削掉，于是 "finished." 看起来像 "finisheu."，一直被当成错误消息损坏上报。同样的不透明背景也应用到 .chatTip 和 .copyButton（两者都因 .chatActions 的 -24px 上拉而压在内容之上），以及 SubAgentPanel 里此前根本没有 background 的 .toolTimeTip。（2）视觉流水线现在以冻结时钟（FIXED_CAPTURE_TIME）渲染，且 turn-error 场景不再产出截图、但保留全部断言。
Verified against the head tree (358be30) / 已对照 head tree 核对
Note the head moved past the last triage pass; the contract test is now a source-scanning vitest guard (visual-capture-contracts.test.ts), not the runtime-clock test an earlier pass described. I reviewed the current form.
注意 head 已越过上一轮 triage；契约测试现在是一个扫描源码的 vitest 守卫（visual-capture-contracts.test.ts），而非早前一轮所描述的运行时时钟测试。我审的是当前形态。
- The clock freeze reaches every capture, in the right order.
freezeWallClock(page.clock.setFixedTime) is called in bothgotoSessionandgotoNewSessionbeforepage.goto, which is the order Playwright requires. Every visuals spec navigates through one of those two helpers; the one spec that navigates a second time (the split-view deep link) does it on the same page aftergotoSession, so it inherits the installed fake clock. The twobrowser.newContextsites are the two that needtimezoneIdrepeated (contexts created directly offbrowserdo not inherit the config'suse). - 时钟冻结覆盖到每一次截图，且顺序正确。
freezeWallClock（page.clock.setFixedTime）在gotoSession和gotoNewSession里都在page.goto之前调用，这正是 Playwright 要求的顺序。每个 visuals spec 都经由这两个 helper 之一导航；唯一二次导航的 spec（分屏深链）是在gotoSession之后于同一页面上做的，因此继承了已装好的假时钟。两个browser.newContext调用点正是需要各自重复timezoneId的两处（直接从browser建的 context 不继承 config 的use）。 - Future-dating the constant is the correct direction, and the reason is real.
formatRelativeTimemeasuresDate.now() - value; a fixture dated after the frozen instant yields a negative age and collapses to "just now".FIXED_CAPTURE_TIMEis2027-01-01, and every absolute date hardcoded undere2e/visuals/is earlier (2026-07-03,2026-07-28), so all relative labels read as older-than-now, which is what each fixture means. Thegit-branch-pickerchange (computedAt: Date.now()→FIXED_CAPTURE_TIME.getTime()) is the subtler catch:Date.now()in a spec is Node's clock, months below the page's frozen one, which armed a reconcile re-fetch no real session performs. - 把常量设成未来时间是正确方向，理由也是真实的。
formatRelativeTime计算Date.now() - value；晚于冻结时刻的 fixture 会得到负龄并塌缩成 "just now"。FIXED_CAPTURE_TIME是2027-01-01，而e2e/visuals/下所有硬编码的绝对日期都更早（2026-07-03、2026-07-28），因此所有相对标签都读作"早于现在"，正是每个 fixture 想表达的。git-branch-picker的改动（computedAt: Date.now()→FIXED_CAPTURE_TIME.getTime()）是更微妙的一处：spec 里的Date.now()是 Node 的时钟，比页面冻结时钟低好几个月，会触发一次真实会话不会做的 reconcile 重新拉取。 - Dropping the turn-error captures does not blind the fix. Those were the only captures that hovered a message row, but Playwright does not diff them — the preview bot does, and a preview is a review aid, not a test. The reveal/hide behaviour stays pinned by seven opacity assertions across the default viewport, the 720px viewport and the Pixel 7 touch context, and the new contract test pins the chip's anchor and background directly from the CSS source. Reverting
.tiptotopor restoring a translucent background now fails a gating vitest run rather than staying silently green. - 删掉 turn-error 截图不会让这个修复失去保护。 它们曾是唯一悬停消息行的截图，但 Playwright 并不对这些图做 diff —— 是预览机器人在 diff，而预览是审阅辅助、不是测试。显示/隐藏行为仍由七个 opacity 断言锁定，覆盖默认视口、720px 视口和 Pixel 7 触摸 context；新的契约测试直接从 CSS 源码锁住小条的锚点与背景。现在把
.tip改回top或恢复半透明背景，会让一个把门的 vitest 失败，而不是悄悄保持绿色。 - The contract test is honestly scoped and lives where it can actually gate. It sits outside
e2e/on purpose —vitest.config.tsexcludese2e/**,playwright.config.tsignores**/visuals/**, and the one workflow step collecting the visuals suite iscontinue-on-error: true, so an invariant asserted in there could be violated with no red build. This file is collected bytest:ci:workspaces, which gates. Its own comments call it "a floor rather than a proof" (the ISO scan cannot see epoch-millis or date-only strings), which is the right framing. - 契约测试的范围界定诚实，且放在真正能把门的位置。 它有意置于
e2e/之外 ——vitest.config.ts排除e2e/**，playwright.config.ts忽略**/visuals/**，而唯一收集 visuals 套件的那个 workflow 步骤是continue-on-error: true，所以在那里断言的不变量被破坏时构建也不会变红。本文件由会把门的test:ci:workspaces收集。它自己的注释称之为"下限而非证明"（ISO 扫描看不到 epoch-millis 或纯日期字符串），这个定位是对的。
Non-blocking residuals / 非阻塞遗留
- 🟢 The shared mock daemon still seeds Node-clock values the frozen-clock rule would want derived, and the guard cannot see them.
mockDaemon.ts:2002(now: Date.now()into thetaskspayload) andmockDaemon.ts:2246(const timestamp = Date.now()into goal snapshots) both survive at this head. The contract test does scane2e/utils/, but only for ISO string literals, so these epoch-millisDate.now()calls are invisible to it — exactly the "floor not proof" gap the test names. Whether either reaches a captured pixel depends on whether the workflow/goal views render those values relatively (candidate consumerToolGroup.tsxtakes an optionalnow?: number); I could not settle that statically. If one does, the symptom is already known and has an oracle: an unexplained CHANGED pair in this PR's own preview. Worth converting both toFIXED_CAPTURE_TIME.getTime()for consistency, or noting why they are exempt. - 🟢 共享 mock daemon 仍在播种 Node 时钟的值，而冻结时钟规则本希望它们由常量派生，且守卫看不到它们。
mockDaemon.ts:2002（把now: Date.now()塞进tasks载荷）和mockDaemon.ts:2246（const timestamp = Date.now()塞进 goal 快照）在本 head 都还在。契约测试确实扫描e2e/utils/，但只找 ISO 字符串字面量，所以这两个 epoch-millis 的Date.now()调用它看不到 —— 正是测试自己点明的"下限非证明"缺口。它们是否会落到某个截图像素上，取决于 workflow/goal 视图是否以相对方式渲染这些值（候选消费者ToolGroup.tsx收一个可选的now?: number）；这一点我无法静态定论。若真会落到，其症状已知且有判据：本 PR 自己预览里一对无法解释的 CHANGED。建议把这两处也改成FIXED_CAPTURE_TIME.getTime()以保持一致，或注明它们为何豁免。 - 🟢 Rule one ("an absolute date must be earlier than this instant") cannot express a fixture that is future-dated by design. The day a visuals fixture seeds a scheduled task's next run (
ScheduledTasksDialogcomputessoonest - Date.now()), the 2027 constant renders it overdue and the constant has to move again. No such fixture exists today, so this is a docstring note, not a defect. - 🟢 规则一（"绝对日期必须早于该时刻"）无法表达一个设计上就应在未来的 fixture。 哪天某个 visuals fixture 要播种一个计划任务的下一次运行时间（
ScheduledTasksDialog计算soonest - Date.now()），2027 这个常量就会把它渲染成逾期，常量又得再往后挪。今天还没有这样的 fixture，所以这是给 docstring 的备注，不是缺陷。 - 🟢 The guard's file-name builder has a harmless posix-ism.
readSourcesnames entries viadir.split('/client/')[1]; on a Windows checkoutjoinyields backslashes, so the split misses and the label becomesundefined/<file>. It is only interpolated into offender messages (which must be empty for the test to pass), so it never affects a green run and a real offender would still fail correctly — just with an ugly path. Cosmetic. - 🟢 守卫的文件名构造有个无害的 posix 假设。
readSources用dir.split('/client/')[1]命名条目；在 Windows 检出下join产生反斜杠，split 匹配不到、标签会变成undefined/<file>。它只被插进 offender 消息里（而测试要通过就必须 offender 为空），所以绝不影响绿色运行，真出现 offender 也仍会正确失败 —— 只是路径难看。属外观问题。 - 🟢 The bottom-right anchor trades a guaranteed collision for a rare one, and the author says so. A message whose last block is a full-width code block or table still gets the chip in its corner; reserving space in flow was rejected (hover-time padding reflows the transcript; a permanent float notch shrinks every code block). This is a strict improvement over the top-right, where the first line always spans the column. No change requested — just recording the tradeoff is deliberate and documented.
- 🟢 右下锚点把一个必然的碰撞换成了一个罕见的碰撞，作者也这么说。 一条消息若最后一个块是占满宽度的代码块或表格，小条仍会落在它的角上；在流中预留空间的方案被否了（悬停时加 padding 会重排整个 transcript；永久的 float 缺口会压缩每个代码块）。相比右上角（首行总是占满整列）这是严格改进。不要求改动 —— 只是记录这个取舍是有意的、且已写明。
CI / 持续集成
Green on head 358be30: Test (ubuntu, Node 22.x), Lint & Static, Integration Tests (no-AK), Desktop Shell (ubuntu + windows), Capture web-shell visuals, and web-shell E2E Smoke. The visuals capture passing is the load-bearing one for this PR and it is green. Only the bot's review-pr lane is pending; nothing red.
head 358be30 上全绿：Test (ubuntu, Node 22.x)、Lint & Static、Integration Tests (no-AK)、Desktop Shell (ubuntu + windows)、Capture web-shell visuals，以及 web-shell E2E Smoke。对本 PR 而言最关键的就是 visuals capture 通过，而它是绿的。只有机器人的 review-pr 通道在跑；无红。
Approving: no Critical, no Important. This is a cosmetic CSS re-anchor plus visual-test determinism work, with no production runtime logic beyond CSS, well-documented, and gated by a new contract test that fails where the old suite would have stayed silently green. The residuals above are all non-blocking notes for follow-up.
批准：无 Critical、无 Important。这是一次外观层面的 CSS 重锚加上视觉测试确定性的工作，除 CSS 外无生产运行时逻辑，文档充分，并由一个新的契约测试把门 —— 在旧套件会悄悄保持绿色的地方它会失败。上面的遗留都是非阻塞的后续备注。
|
Released in v0.23.2. |
6 participants
What this PR does
The hover timestamp in the web-shell transcript sits on top of the first line of the message it belongs to and chops the tops off the letters underneath. It moves to the bottom-right, and its background becomes opaque.
Alongside it, visual captures now render at a fixed clock, and the turn-error scenario stops producing screenshots (its assertions all stay).
Why it's needed
The overlap
MessageTimestamp's.tipis absolutely positioned attop: 2px; right: 4pxwith a 92%-opaque background. The first line of a wrapped message is the one line that reliably runs the full column width, so the chip lands on it almost every time — and it does not cover the last word so much as decapitate it. Its lower edge cuts horizontally through the glyphs, taking a letter's ascender while the bowl stays visible below:...before the answer finisheu....before the answer finished.The
dreads as aubecause only its bowl survives. This kept being reported as a corrupted error message in web-shell previews.Worth recording, because it inverts the obvious fix: making the background opaque does not repair this. The faint remnant of the ascender showing through the 92% chip is the only hint the letter is a
dat all; painting the chip solid erases that hint and leaves an even cleaneru. Moving the chip off the text is what fixes it. Opacity is kept for its own reason — a glyph bleeding through any overlay reads as broken text — and.chatTipand.copyButtonsit over content too (.chatActionspulls itself up by-24px), so they get it as well.The clock
The visuals job renders the PR head, reinstalls dependencies, then renders the merge-base, so the two passes photograph the same view minutes apart. Every view showing a wall-clock time therefore differed on every run for no reason but when it was taken. On #11267 that was the entire preview: the only view the compose step reported as CHANGED was
terminal-turn-error-copy-narrow-dark, at exactly the 0.02% threshold, and the whole diff was09:09:28against09:18:16. The narrow viewport is half the pixels of the wide one, which is why the same ~100 timestamp pixels cleared the threshold there and were skipped at 0.01% everywhere else — so the noise also crowded out any real change that PR made.page.clock.setFixedTimefromgotoSession/gotoNewSession, which every visuals spec navigates through. It fakes only clock readings and keeps timers running, so replay, streaming andfreezeLoopingAnimationsare unaffected. The constant is deliberately future-dated:formatRelativeTimemeasuresDate.now() - value, and an earlier constant turned the channel editor's2026-07-28pairing requests into "just now".git-branch-pickerseededcomputedAtfrom Node's real clock, months from the page's frozen one, which armed a reconcile re-fetch no real session performs; it now derives from the same constant.The captures
The turn-error scenario is the only visual test that hovers a message row — it must, to reveal the Copy affordance shipped in #10001. Since
.tipisopacity: 0until.row:hover, its screenshots were the only place in the entire suite where a hover-timestamp change could appear. Six of them (two themes × wide/narrow/touch), so every web-shell preview led with full-height red error images no matter what the PR touched, and readers repeatedly took the preview for a live failure.The behaviour is pinned by assertion instead: seven opacity checks across the default viewport, the 720px viewport and the Pixel 7 touch context cover reveal-on-hover, reveal-on-focus, hide-on-blur, and always-visible actions under
hover: none. A capture never verified any of that — Playwright does not diff these images, the preview bot does, and the preview is a review aid rather than a test. The scenario, the fixture and every assertion remain; only the captures go.Reviewer Test Plan
How to verify
npm run test:e2e:visuals --workspace=packages/web-shell— the turn-error assertions still pass with no captures produced.Evidence (Before & After)
Captured by run 34231051803 on
b6db7f9d, the last run before the captures were removed. Left panel is the PR base, right panel is this PR:Zoomed to the pixel, the base panel renders
finisheu.beneath a13:30:53chip while this PR rendersfinished.with thedat fullh-height and the chip clear of the text at the bottom-right. The wide and light variants agree.That run also confirmed the future-dated clock: every
channel-editorview returned to 0% diff, having previously flipped7/28/2026to "just now".Tested on
Environment (optional)
No test, build, or typecheck was run locally; this machine is not where these get verified. What was checked here:
prettier --checkon every touched file, andpage.clock.setFixedTime(time: number|string|Date)confirmed present in the installedplaywright-coretypes, whose doc line reads "keeps all the timers running". Everything else is CI's, and the visuals job supplied the evidence above.Risk & Scope
.tipchanges several visual baselines. That is the intended change and was visible in the run linked above.Linked Issues
Fixes #11442.
Both problems surfaced in the visual preview on #11267 — neither is caused by it, and this PR does not depend on it. The turn-error scenario whose captures are dropped here comes from #10001.
The capture-clock half has no separate issue: it is preview tooling rather than shipped behaviour, and the measurement that motivates it is quoted above.