fix(web-shell): polish scheduled task timeline UI - #6386
Conversation
Code ReviewImplementation is clean and well-scoped. The approach — prop drilling The previous blocking issue (TypeScript errors from Minor observation (non-blocking): TestingCI: All checks pass — Test (ubuntu-latest, Node 22.x) ✅, Classify PR ✅, review-pr ✅. Unit tests: PR adds 2 new tests (scheduled task timeline marker, parallel agents locate flash) on top of the existing 33. CI confirms all pass. Tmux visual testing: N/A — web-shell is a browser-based React UI. The visual changes (tick sizing, clock icon, flash targeting) require browser verification per the Reviewer Test Plan. Author confirms macOS testing. 中文说明代码审查实现干净，范围合理。方案——通过 prop drilling 传递 之前的阻塞问题（ 非阻塞观察：时间轴详情 span 上的 测试CI：所有检查通过——Test (ubuntu-latest, Node 22.x) ✅, Classify PR ✅, review-pr ✅。 单元测试：PR 在已有 33 个测试基础上新增 2 个测试（定时任务时间轴标记、并行代理定位高亮）。CI 确认全部通过。 Tmux 可视化测试：不适用——web-shell 是基于浏览器的 React UI。视觉变更（横线尺寸、时钟图标、flash 定位）需按 Reviewer Test Plan 在浏览器中验证。作者确认已在 macOS 上测试。 — Qwen Code · qwen3.7-max |
ReflectionThe implementation quality is solid. The flash targeting refactor from row-level to per-message-bubble is a clean improvement, the timeline tick CSS adjustments match the designer's specs precisely, and the scheduled-task clock icon adds a useful visual marker. The scope is tight, the code follows project conventions, and the tests are thorough. The previous blocking issue — TypeScript errors from accessing The only remaining nit is the dead Approving. ✅ 中文说明总结实现质量扎实。flash 定位从行级别重构到消息气泡级别，干净有效；时间轴横线 CSS 调整精确匹配设计师规格；定时任务时钟图标是有用的视觉标记。范围紧凑，代码遵循项目规范，测试充分。 之前的阻塞问题——在不声明 唯一的小问题是死代码 批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
TypeScript typecheck is broken — isScheduledTaskMessage accesses message.source on DaemonUserMessage, but that type doesn't declare a source field. Quick fix: add source?: string to DaemonUserMessage in client/adapters/messageTypes.ts. Once that's in, this looks ready to ship. 🙏
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Suggestion] ParallelAgentsGroup at MessageList.tsx:2667 does not receive isLocateFlashing. displayItemMatchesLocateTarget correctly returns true for parallel_agents items (line 1355), but the render branch never passes that result to the component. Users clicking "show in transcript" on a tool inside a parallel agents group will scroll to the right place but see no flash feedback. Consider wrapping <ParallelAgentsGroup> in a <div className={isLocateFlashing ? flashStyles.flash : undefined}> or passing the prop through.
— qwen3.7-max via Qwen Code /review
| function isScheduledTaskMessage(message: Message): boolean { | ||
| return ( | ||
| message.role === 'user' && | ||
| (message.source === 'cron' || message.source === 'loop') |
There was a problem hiding this comment.
[Critical] message.source does not exist on type DaemonUserMessage. Running tsc --noEmit produces two errors here:
TS2339: Property 'source' does not exist on type 'DaemonUserMessage'.
DaemonUserMessage (messageTypes.ts:77-82) only declares id, role, content, images?, and inherits timestamp? from DaemonMessageMeta — no source field.
The PR description notes the daemon/source metadata propagation is handled separately, but the type must at least declare source?: string on DaemonUserMessage for this code to compile.
| (message.source === 'cron' || message.source === 'loop') | |
| function isScheduledTaskMessage(message: Message): boolean { | |
| return ( | |
| message.role === 'user' && | |
| ('source' in message) && | |
| (message.source === 'cron' || message.source === 'loop') | |
| ); | |
| } |
Or add source?: string to DaemonUserMessage in messageTypes.ts (the proper fix if the companion PR will populate it).
— qwen3.7-max via Qwen Code /review
| displayItemMatchesLocateTarget(child, target), | ||
| ); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
[Suggestion] displayItemMatchesLocateTarget has no exhaustiveness guard. If a new DisplayItem variant is added to the union, this function will silently return false with no TypeScript error, and the locate flash will degrade without any compile-time signal.
Other functions in this file use a never assertion for exhaustiveness (e.g., isHideableStep). Consider adding one here:
| return false; | |
| if (item.type === 'turn_content') { | |
| return item.items.some((child) => | |
| displayItemMatchesLocateTarget(child, target), | |
| ); | |
| } | |
| const _exhaustive: never = item; | |
| return false; | |
| } |
— qwen3.7-max via Qwen Code /review
| @@ -1636,8 +1691,21 @@ const SessionTimeline = memo(function SessionTimeline({ | |||
| data-testid="session-timeline-detail" | |||
| data-title={entry.label} | |||
There was a problem hiding this comment.
[Suggestion] data-title and data-detail are no longer consumed by CSS — the ::before { content: attr(data-title) } and ::after { content: attr(data-detail) } rules were removed in this PR and replaced with child <span> elements. These attributes are now dead weight that only the test at MessageList.dom.test.tsx:389 still reads.
Consider removing both attributes and updating the test to assert on the child spans' textContent instead.
— qwen3.7-max via Qwen Code /review
| it('marks scheduled task turns in the session timeline', async () => { | ||
| const rectSpy = mockMessageListWidth(1200); | ||
| const c = mount([ | ||
| { ...userMsg('u1'), source: 'cron', content: 'scheduled tracking task' }, |
There was a problem hiding this comment.
[Suggestion] This test fixture spreads source: 'cron' onto a DaemonUserMessage, but DaemonUserMessage does not declare a source property — tsc --noEmit reports TS2353: Object literal may only specify known properties, and 'source' does not exist in type 'DaemonUserMessage'.
The test passes at runtime because the spread bypasses excess property checks, but it masks the fact that the type is incomplete. Once source?: string is added to DaemonUserMessage, this will work correctly.
Also consider adding a test case for source: 'loop' (currently only 'cron' is tested), and adding flash-assertion tests for non-user message types (tool_group, assistant, plan, thinking) since isLocateFlashing is threaded into all of them.
— qwen3.7-max via Qwen Code /review
|
Addressed the parallel_agents locate flash feedback in 9ac7daa by applying the locate flash class around the ParallelAgentsGroup render branch and covering it with a MessageList DOM test. Local verification passed: npx vitest run client/components/MessageList.dom.test.tsx from packages/web-shell, and npm run typecheck from the repo root. |
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. |
| pendingApproval={pendingApproval} | ||
| workspaceCwd={workspaceCwd} | ||
| shellOutputMaxLines={shellOutputMaxLines} | ||
| isLocateFlashing={isLocateFlashing} |
There was a problem hiding this comment.
[Suggestion] isLocateFlashing is forwarded to UserMessage, AssistantMessage, ThinkingMessage, ToolGroup, and PlanMessage — but not to UserShellMessage (line 115). Since isTurnStartMessage treats role === 'user_shell' as a turn start, shell-command turns appear in the session timeline and can be jumped to via scrollToMessage. The old row-level rowFlash covered every row type uniformly; now user_shell turns get no visual locate feedback.
Consider threading isLocateFlashing through UserShellMessage as well (add the prop, apply flashStyles.flash to its outer wrapper).
— qwen3.7-max via Qwen Code /review
| onShowContextDetail, | ||
| headerOffset, | ||
| visibleItems, | ||
| flashTarget, |
There was a problem hiding this comment.
[Suggestion] Adding flashTarget to renderVirtualItem's dependency array means that every locate-flash state change (null → target, target → null — twice per flash cycle) recreates this callback, which causes React to re-execute it for every visible virtual row. The old flashKey approach only appeared in getRowClassName's deps, which changed just the wrapper <div> className without invalidating the render callback.
Additionally, ParallelAgentsGroup is not wrapped in React.memo, so all visible parallel-agent groups re-render unconditionally on flash state changes.
Consider extracting flash-class computation into a lightweight memoized wrapper (e.g., a FlashableParallelAgents component) so that only the affected row re-renders when flash state changes, not the entire visible item tree.
— qwen3.7-max via Qwen Code /review
| scrollIntoView.mockRestore(); | ||
| rectSpy.mockRestore(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[Suggestion] The existing "scrollToMessage auto-expands the collapsed turn that holds the target" test (above this new test) verifies that the turn expands, but does not verify that the locate flash fires after the deferred performScrollToRow(idx + headerOffset, pending) callback runs (line ~2455 in MessageList.tsx). The deferred-flash code path — where the flash target is carried through a collapsed-turn expansion — is entirely untested.
Consider adding await nextFrame() and a flash assertion to the collapsed-turn test, similar to how this new parallel-agents test verifies the flash.
— qwen3.7-max via Qwen Code /review
✅ Real-browser verification —
|
| Metric | Before (5c8af1a1) |
After (PR) | PR spec |
|---|---|---|---|
| Base tick | 10 × 4 px | 10 × 3 px | 10 × 3 ✅ |
| Longest hover tick | 38 px (scaleX 3.8) | 28 px (scaleX 2.8) | 28 × 3 ✅ |
| Hover fan-out neighbours | 30 / 22 px | 22 / 16 px | — |
| Visual gap between ticks | 17 px | 12 px | 12 ✅ |
→ 
Rail tick fan-out on hover: before (4px, 38px longest) → after (3px, 28px longest)
2. Scheduled-task clock icon + single-line detail — works (when source is present)
Rendering a user turn with source: 'cron': exactly 1 timeline detail carries data-scheduled-task="true" and renders the clock SVG; 0 normal turns get an icon. Detail is now white-space: nowrap + text-overflow: ellipsis, rendered height 18.84px = one line (was a 3-line clamp), and scrollWidth(391) > clientWidth(320) confirms real truncation.
Before: no icon, detail wraps to 2 lines · After: clock icon + single-line ellipsis title & detail
3. Locate flash moved into the target message (not the row) — verified for every kind
Driving scrollToMessage in the real browser, for each target exactly one element flashes (animation: message-locate-flash), it sits inside a [data-index] row but is not the row, and no [data-index] row flashes:
| Locate target | Flashing element | Row flashes? |
|---|---|---|
| user | chatBubble |
none |
| assistant | content |
none |
| tool_group | group container | none |
| parallel_agents | wrapper div |
none |
Before → after, the same locate goes from a full-width row wash (1000 px) to a bubble-scoped highlight:
Before: entire row highlighted · After: only the target user bubble
This also independently confirms the
parallel_agentslocate-flash fix (commit9ac7daa) requested earlier is working — the grouped block flashes and its row does not.
Notes for the merge decision
-
One TS error remains in the new test (not CI-gated).
MessageList.dom.test.tsx:429setssource: 'cron'on aDaemonUserMessage, which doesn't declaresource→TS2353. ProductionMessageList.tsxis tsc-clean (theisScheduledTaskMessageparam is defensively typed{ role; source?: string }), and web-shell has notypecheckscript so CI stays green — but the gap is real. The clean fix is the one already suggested in review: addsource?: stringtoDaemonUserMessage(client/adapters/messageTypes.ts). (baseline: the other pre-existing tsc errors inMessageList.test.ts/Markdown.test.tsalso exist on merge-base — not introduced here.) -
The clock icon is dormant in production until
sourceis wired up.isScheduledTaskMessagefires only when ausermessage hassource === 'cron' | 'loop', but the adapter (transcriptToMessages.ts) only ever setssourceon system/error messages (session_branched,prompt_cancelled,turn_error, …) — nevercron/loop, never on user turns. So with real daemon data the marker never appears yet. This matches the PR's own "source metadata propagation is a separate PR" note; the rendering path itself is correct (verified via injectedsource). Fixing note 1 is the prerequisite for that follow-up to light it up.
Verdict: the visual polish (tick proportions, single-line detail) and the locate-flash relocation are correct and live. Recommend landing the one-line source?: string type fix so the new test typechecks and the follow-up can activate the scheduled-task marker.
中文版（点击展开）
✅ 真机浏览器验证 —— fix(web-shell): polish scheduled task timeline UI
在无头 Chromium（Playwright）里渲染真实的 MessageList（不只是 jsdom）验证了 head 提交 9dcffd3c，并与 merge-base 5c8af1a1 做 A/B（把两个改动文件回退到 merge-base 版本再测一遍）。时间轴横线尺寸、定时任务时钟图标、单行详情、定位高亮位置全部读自真实计算后的 CSS。
方法： 全新 worktree + npm ci；harness 挂载真实 MessageList（真实子消息组件与 provider）；用 getBoundingClientRect / getComputedStyle 测量；定位高亮通过真实的 MessageListHandle.scrollToMessage(...) 驱动。
测试套件
- PR 自带 DOM 测试：36/36 通过。
- web-shell 全量套件：1045/1045 通过（69 个文件），无回归。
1. 时间轴横线比例 —— 与设计稿完全一致（真实像素）
| 指标 | Before (5c8af1a1) |
After (PR) | PR 说明 |
|---|---|---|---|
| 基础横线 | 10 × 4 px | 10 × 3 px | 10 × 3 ✅ |
| hover 最长横线 | 38 px (scaleX 3.8) | 28 px (scaleX 2.8) | 28 × 3 ✅ |
| hover 相邻递减 | 30 / 22 px | 22 / 16 px | — |
| 横线视觉间距 | 17 px | 12 px | 12 ✅ |
2. 定时任务时钟图标 + 单行详情 —— 有效（前提是存在 source）
给 source: 'cron' 的 user turn：恰好 1 条详情带 data-scheduled-task="true" 并渲染时钟 SVG；普通 turn 0 个图标。详情改为 nowrap + ellipsis，渲染高度 18.84px = 一行（原来是 3 行 clamp），scrollWidth(391) > clientWidth(320) 证明确实被截断。
3. 定位高亮移入目标消息（不再整行）—— 每种类型都验证
在真机里驱动 scrollToMessage：每个目标恰好 1 个元素闪烁（animation: message-locate-flash），它在 [data-index] 行内部但不是行本身，且没有任何 [data-index] 行闪烁：
| 定位目标 | 闪烁元素 | 行是否闪烁 |
|---|---|---|
| user | chatBubble |
否 |
| assistant | content |
否 |
| tool_group | group 容器 | 否 |
| parallel_agents | 包裹 div |
否 |
Before → After：同一次定位从**整行铺满（1000px）**变成只高亮气泡。同时也独立验证了之前 review 要求的 parallel_agents 定位高亮修复（提交 9ac7daa）确实生效。
合并前的说明
-
新测试里仍有 1 个 TS 报错（但 CI 不拦截）。
MessageList.dom.test.tsx:429给DaemonUserMessage设了source: 'cron'，而该类型没声明source→TS2353。生产代码MessageList.tsxtsc 干净（isScheduledTaskMessage的参数被防御式地写成{ role; source?: string }），且 web-shell 没有typecheck脚本，所以 CI 仍是绿的 —— 但这个类型缺口是真实存在的。建议采用 review 已提出的修法：给DaemonUserMessage（client/adapters/messageTypes.ts）加source?: string。（基线：MessageList.test.ts/Markdown.test.ts里其它 tsc 报错在 merge-base 上也存在，非本 PR 引入。） -
在
source打通之前，时钟图标在生产环境处于休眠状态。isScheduledTaskMessage只有当user消息的source === 'cron' | 'loop'时才触发，但适配器transcriptToMessages.ts只会给 system/error 类消息设置source（session_branched、prompt_cancelled、turn_error等），从不设cron/loop，也从不设在 user turn 上。所以用真实 daemon 数据时该标记目前不会出现 —— 这与 PR 自述「source 元数据透传由另一个 PR 处理」一致；渲染逻辑本身是对的（通过注入source已验证）。修好第 1 点是后续 PR 点亮该标记的前提。
结论： 视觉打磨（横线比例、单行详情）与定位高亮的迁移都正确且已生效。建议合入那行 source?: string 类型修复，让新测试通过 tsc，也为后续激活定时任务标记铺路。
Verified locally with tmux + Playwright against real Chromium. Harness mounts the actual MessageList; measurements are computed-style / bounding-rect reads, A/B vs merge-base.
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution Summary — PR #6386ConflictsThree files had content conflicts when merging 1.
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Adapter gap (not in diff): isScheduledTaskMessage checks message.source on user messages, but transcriptToMessages.ts:198 never propagates meta.source from the transcript block to DaemonUserMessage. The clock icon feature is inert in production — the test passes only because it injects source directly on the fixture. The PR description says daemon/source metadata propagation is out of scope, but the passing test may give future maintainers a false sense of end-to-end coverage.
— qwen3.7-max via Qwen Code /review
| displayItem, | ||
| flashTarget, | ||
| )} | ||
| shellOutputMaxLines={shellOutputMaxLines} |
There was a problem hiding this comment.
[Critical] shellOutputMaxLines is referenced here but never declared, imported, or received as a prop on the MessageList component. tsc reports TS2304 and all 34 rendering tests crash with ReferenceError: shellOutputMaxLines is not defined. This would also crash the web-shell UI in production.
MessageItem declares shellOutputMaxLines: number as a required prop, so it cannot simply be omitted either.
| shellOutputMaxLines={shellOutputMaxLines} |
Fix: either (a) add shellOutputMaxLines to MessageListProps and thread it from the parent, (b) read it from a settings/config hook, or (c) provide a default constant. If it's a reactive value, also add it to the renderDisplayItem dependency array.
— qwen3.7-max via Qwen Code /review
| tools, | ||
| pendingApproval, | ||
| workspaceCwd, | ||
| shellOutputMaxLines, |
There was a problem hiding this comment.
[Critical] shellOutputMaxLines is destructured from props but never used in ToolGroup. ESLint flags this as @typescript-eslint/no-unused-vars. This is likely a side-effect of the MessageList.tsx issue above — once shellOutputMaxLines is properly defined upstream and actually needed here, use it (e.g., pass to a shell output component); otherwise remove it from the destructuring and the interface.
— qwen3.7-max via Qwen Code /review
| {content && ( | ||
| <div className={styles.content}> | ||
| <div | ||
| className={`${styles.content}${ |
There was a problem hiding this comment.
[Suggestion] The same template-literal className concatenation pattern (`${styles.X}${isLocateFlashing ? ` ${flashStyles.flash}` : ''}`) is copy-pasted 6 times across 4 files: AssistantMessage.tsx:54,220, PlanMessage.tsx:37, ToolGroup.tsx:866,1343, UserMessage.tsx:56. Meanwhile, MessageList.tsx already has a local joinClassNames(...) utility.
Extract joinClassNames to a shared utility and replace the 6 sites:
| className={`${styles.content}${ | |
| className={joinClassNames(styles.content, isLocateFlashing && flashStyles.flash)} |
— qwen3.7-max via Qwen Code /review
| useEffect(() => { | ||
| if (!flashKey) return; | ||
| const timer = setTimeout(() => setFlashKey(null), 1600); | ||
| if (!flashTarget) return; |
There was a problem hiding this comment.
[Suggestion] The flash duration is defined in two disconnected places: 1600 in the setTimeout here and 1.6s in MessageLocateFlash.module.css:3. If someone changes one without the other, the flash class is removed mid-animation (visual pop) or lingers invisibly after the animation ends.
Define a shared constant:
const LOCATE_FLASH_DURATION_MS = 1600;Reference it in the setTimeout. For the CSS side, either use a CSS custom property or add a cross-reference comment.
— qwen3.7-max via Qwen Code /review
| return -1; | ||
| } | ||
|
|
||
| function displayItemMatchesLocateTarget( |
There was a problem hiding this comment.
[Suggestion] displayItemMatchesLocateTarget is called unconditionally for every visible virtualized row during the flash window. For turn_content items it recurses into children; for tool_group messages it linearly scans tools (including subTools). With ~20-30 visible rows this is not catastrophic, but it runs in React's render phase (blocking paint) during the scroll animation the user is watching.
Precompute a Set<string> of matching display-item keys in a useMemo keyed on flashTarget, then do O(1) lookups in the render path.
— qwen3.7-max via Qwen Code /review
| stroke-width: 1.4; | ||
| } | ||
|
|
||
| .sessionTimelineDetailsTitleText { |
There was a problem hiding this comment.
[Suggestion] .sessionTimelineDetailsTitleText has text-overflow: ellipsis and white-space: nowrap, but as a flex child it defaults to min-width: auto — it will never shrink below its content width, so the ellipsis will never appear. Long turn labels will visually overflow the timeline popup.
| .sessionTimelineDetailsTitleText { | |
| .sessionTimelineDetailsTitleText { | |
| min-width: 0; | |
| overflow: hidden; | |
| color: var(--foreground); | |
| font-size: 14px; | |
| font-weight: 600; | |
| line-height: 1.35; | |
| text-overflow: ellipsis; | |
| white-space: nowrap; | |
| } |
— qwen3.7-max via Qwen Code /review
| setFlashKey(null); | ||
| requestAnimationFrame(() => setFlashKey(key)); | ||
| setFlashTarget(null); | ||
| requestAnimationFrame(() => setFlashTarget(target)); |
There was a problem hiding this comment.
[Suggestion] setFlashTarget(null) followed by requestAnimationFrame(() => setFlashTarget(target)) triggers two separate React renders. In the common case (no previous flash active), the first render clears a non-existent flash — a wasted render cycle that delays the scroll-to-target.
Consider using a generation counter (e.g. flashGeneration) as a CSS animation key to restart the animation without the null→target double-set, or check a ref to skip the null set when no flash is currently active.
— qwen3.7-max via Qwen Code /review
|
Addressed the actionable #6386 follow-up without pulling in the metadata propagation work from #6385. The scheduled-task marker test is now named/commented as a UI rendering contract once source is present, so it should not imply end-to-end adapter coverage. I also fixed the merge-conflict follow-up that left MessageList passing an undefined shellOutputMaxLines value into MessageItem.\n\nLocal verification from /Users/ethan/Projects/qwen-code-pr-6386: git diff --check; cd packages/web-shell && npx vitest run client/components/MessageList.test.ts client/components/MessageList.dom.test.tsx. Both MessageList suites passed, 126 tests total. A local web-shell build proceeds past the prior shellOutputMaxLines error but then stops on current-main UsageDashboardTab implicit-any diagnostics; that file is unchanged by this PR. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No new Critical issues found. Two new Suggestions below. Build and tests pass locally (typecheck clean across all 7 packages, 36/36 vitest pass).
Note on prior Critical comments: The earlier Critical findings about broken typecheck appear to be false positives. isScheduledTaskMessage uses a structural parameter type { role: Message['role']; source?: string } where source is optional — TypeScript accepts Message objects without source. Similarly, shellOutputMaxLines as a destructured-but-unused prop does not trigger TS errors (noUnusedLocals doesn't apply to destructured properties). These are code quality concerns, not compilation failures.
— qwen3.7-max via Qwen Code /review
| isLocateFlashing={isLocateFlashing} | ||
| /> | ||
| ); | ||
| case 'plan': |
There was a problem hiding this comment.
[Suggestion] The case 'system' branch (line 102) does not forward isLocateFlashing to SystemMessage, while all other updated branches (user, assistant, thinking, tool_group, plan) receive it. displayItemMatchesLocateTarget correctly returns true for system messages by ID, so MessageItem receives isLocateFlashing=true but the system branch silently drops it — the row scrolls into view but no flash appears.
Consider forwarding the prop to SystemMessage (with a matching flash wrapper), or wrapping the case 'system' return in a <div> with the flash class, mirroring the parallel_agents pattern.
— qwen3.7-max via Qwen Code /review
| ); | ||
| } | ||
| if (item.type === 'turn_content') { | ||
| return item.items.some((child) => |
There was a problem hiding this comment.
[Suggestion] This turn_content branch is dead code. displayItemMatchesLocateTarget is called from renderDisplayItem, which already recurses into turn_content children individually (each child is rendered with its own displayItemMatchesLocateTarget(child, flashTarget) call). The function is never invoked with a turn_content item as the argument, so this recursive branch is unreachable.
Consider removing the dead branch, or adding a comment explaining it's defensive code for potential future nested structures.
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
MessageItem.tsx:102 |
SystemMessage branch doesn't forward isLocateFlashing — flash silently no-ops for system messages |
Forward the prop or wrap in a flash div |
MessageList.tsx:1363 |
turn_content branch in displayItemMatchesLocateTarget is dead code — never reached because renderDisplayItem recurses into children individually |
Remove the branch or document as defensive code |
MessageList.dom.test.tsx:424 |
source: 'loop' branch of isScheduledTaskMessage has no test coverage — only 'cron' is tested |
Add a test case for source: 'loop' |
— qwen3.7-max via Qwen Code /review
| pendingApproval={pendingApproval} | ||
| /> | ||
| <div | ||
| className={ |
There was a problem hiding this comment.
[Suggestion] The <div> wrapper around ParallelAgentsGroup is rendered unconditionally — even when flashTarget is null (the steady state >99% of the time), this extra DOM node sits between <MessageTimestamp> and <ParallelAgentsGroup>. Unlike other message types that receive isLocateFlashing as a prop and apply the flash class internally, this approach adds a permanent structural change for a transient visual effect.
Consider either conditionally rendering the wrapper only when flashing, or passing isLocateFlashing to ParallelAgentsGroup (consistent with the prop-drilling pattern used for other message types):
const shouldFlash = displayItemMatchesLocateTarget(displayItem, flashTarget);
const inner = <ParallelAgentsGroup agents={displayItem.agents} pendingApproval={pendingApproval} />;
return (
<MessageTimestamp timestamp={displayItem.timestamp}>
{shouldFlash ? <div className={flashStyles.flash}>{inner}</div> : inner}
</MessageTimestamp>
);— qwen3.7-max via Qwen Code /review
| if (!flashKey) return; | ||
| const timer = setTimeout(() => setFlashKey(null), 1600); | ||
| if (!flashTarget) return; | ||
| const timer = setTimeout(() => setFlashTarget(null), 1600); |
There was a problem hiding this comment.
[Suggestion] The 1600ms setTimeout that clears flashTarget has no test coverage. No test uses vi.useFakeTimers() + vi.advanceTimersByTime(1700) to verify that the flash class/attribute is removed after the timer expires. This is the core lifecycle behavior of the locate-flash feature — if the timer is accidentally removed or the delay changes, messages would flash indefinitely.
Consider adding a test like:
vi.useFakeTimers();
// ... trigger scrollToMessage ...
act(() => { vi.advanceTimersByTime(1700); });
expect(targetMessage?.getAttribute('data-locate-flashing')).toBeUndefined();— qwen3.7-max via Qwen Code /review
| return compact; | ||
| } | ||
|
|
||
| function isScheduledTaskMessage(message: { |
There was a problem hiding this comment.
[Suggestion] isScheduledTaskMessage checks both 'cron' and 'loop' source values (line 634), but the test ("renders scheduled task marker when source is present") only covers source: 'cron'. The 'loop' branch has zero test coverage — a regression that drops or mistypes this condition would pass silently.
Consider adding a second scheduled-task message with source: 'loop' to the test fixture and asserting it also gets data-scheduled-task="true".
— qwen3.7-max via Qwen Code /review
| await nextFrame(); | ||
|
|
||
| const targetMessage = c.querySelector('[data-testid="msg-u2"]'); | ||
| expect(targetMessage?.getAttribute('data-locate-flashing')).toBe('true'); |
There was a problem hiding this comment.
[Suggestion] This test asserts the target message HAS data-locate-flashing="true" but never asserts that non-target sibling messages LACK it. A bug in displayItemMatchesLocateTarget that matches too broadly (e.g., flashing every message in the same turn) would go undetected.
Consider adding negative assertions for at least one adjacent message:
expect(c.querySelector('[data-testid="msg-a2"]')?.getAttribute('data-locate-flashing')).toBeUndefined();— qwen3.7-max via Qwen Code /review
| .sessionTimelineButton { | ||
| position: relative; | ||
| top: -4px; | ||
| top: -6.5px; |
There was a problem hiding this comment.
[Suggestion] With top: -6.5px, button height 16px, item height 3px, and gap 12px, adjacent timeline buttons overlap by 1px vertically:
- Button N bottom:
item_top + (-6.5 + 16) = item_top + 9.5px - Button N+1 top:
item_top + (3 + 12) + (-6.5) = item_top + 8.5px - Overlap: 1px
This can cause hover/click ambiguity between adjacent ticks (both buttons receiving :hover simultaneously, wrong tooltip appearing). Consider adjusting top to -5.5px (0px clearance) or reducing button height to 14px.
— qwen3.7-max via Qwen Code /review
| .sessionTimelineDetails::after { | ||
| content: attr(data-detail); | ||
| display: -webkit-box; | ||
| .sessionTimelineDetailsDetail { |
There was a problem hiding this comment.
[Suggestion] .sessionTimelineDetailsDetail is a grid child (parent .sessionTimelineDetails uses display: grid) with overflow: hidden and text-overflow: ellipsis, but lacks min-width: 0. Grid items default to min-width: auto and won't shrink below their content's intrinsic width — the ellipsis will never trigger for long detail text.
The sibling .sessionTimelineDetailsTitle correctly has min-width: 0 (line 376), making this an oversight.
| .sessionTimelineDetailsDetail { | |
| .sessionTimelineDetailsDetail { | |
| min-width: 0; | |
| overflow: hidden; | |
| color: color-mix(in srgb, var(--muted-foreground) 86%, transparent); | |
| font-size: 13px; | |
| line-height: 1.45; | |
| text-overflow: ellipsis; | |
| white-space: nowrap; | |
| } |
— qwen3.7-max via Qwen Code /review
| @@ -0,0 +1,15 @@ | |||
| .flash { | |||
| border-radius: 8px; | |||
| animation: message-locate-flash 1.6s ease-out; | |||
There was a problem hiding this comment.
[Suggestion] The flash animation module has no @media (prefers-reduced-motion: reduce) override. Users with vestibular disorders or motion sensitivity will see the full 1.6-second background-color animation on every locate action.
Consider adding:
@media (prefers-reduced-motion: reduce) {
.flash {
animation-duration: 0.01ms;
}
}— qwen3.7-max via Qwen Code /review
| case 'plan': | ||
| return <PlanMessage id={message.id} todos={message.todos} />; | ||
| return ( | ||
| <PlanMessage |
There was a problem hiding this comment.
[Suggestion] isLocateFlashing is threaded to UserMessage, AssistantMessage, ThinkingMessage, ToolGroup, and PlanMessage — but not to SystemMessage (line 99) or BtwMessage (line 116). If a locate target resolves to a system or btw message, the row scrolls into view but no visual flash feedback is rendered, creating inconsistent locate UX.
Consider passing isLocateFlashing={isLocateFlashing} to both components and applying flashStyles.flash to their root elements.
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review failed due to API throttling:
[API Error: {"code":"Throttling.BurstRate","message":"Request rate increased too quickly. To ensure system stability, please adjust your client logic to scale requests more smoothly over time.","request_id":"3d685351-5eaa-975d-aa79-be2ebc2895bf"}]
|
Review-state follow-up for the remaining Current head The old blocking request to add So for #6386 I am leaving the completed UI unchanged and treating the remaining |
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ Problem: This is a visual polish PR with concrete design specs — timeline tick proportions (10×3px base, 28×3px hover, 12px spacing), scheduled-task clock icon, and locate-flash targeting. The author has tested on macOS and provides before/after descriptions. The problem is observable design drift, not theoretical. Direction: Aligned. Web-shell UI polish is squarely in scope. No sensitive areas (auth, sandbox, telemetry) touched. CHANGELOG has no direct reference but web-shell visual polish is always relevant. Size: Not applicable — all changes are in Approach: Scope is tight and focused. Three independent changes (tick CSS, clock icon, flash targeting) are all small and directly serve the stated goal. The flash targeting refactor — extracting Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个有具体设计规格的视觉打磨 PR——时间轴横线比例（10×3px 基础，28×3px hover，12px 间距）、定时任务时钟图标、定位高亮范围调整。作者已在 macOS 上测试并提供了 before/after 描述。问题是可观测的设计偏差，不是理论性的。 方向：对齐。Web-shell UI 打磨完全在范围内。未触及敏感区域（auth、sandbox、telemetry）。CHANGELOG 无直接参考但 web-shell 视觉打磨始终相关。 规模：不适用——所有变更在 方案：范围紧凑聚焦。三个独立变更（横线 CSS、时钟图标、flash 定位）都很小且直接服务于目标。flash 定位重构——提取 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
5 participants
What this PR does
This PR polishes the left session timeline and the message-locate flash behavior. The timeline ticks now use the designer-requested proportions: the base tick is 10 x 3px, the longest hover tick is 28 x 3px, and the visual spacing between ticks is 12px. Scheduled-task turns show a clock icon in the timeline detail, and the timeline detail text is constrained to a single line with ellipsis. Message locate flashes now render inside the target message content or user bubble instead of highlighting the full row.
Why it's needed
These visual adjustments were requested by design. The timeline should read as a compact navigation rail with precise tick rhythm, and scheduled-task turns need a distinct visual marker. The locate flash should guide attention to the actual target message without washing the entire transcript row.
Reviewer Test Plan
How to verify
Open a session with at least four turns so the left session timeline is visible. Hover timeline entries and confirm the ticks use the requested 10 x 3px base size, 28 x 3px longest hover length, and 12px visual spacing. Open or replay a scheduled-task turn and confirm the timeline detail shows the clock icon and remains one line. Click a timeline entry and confirm the locate flash appears only on the target message content or user bubble, not across the full row.
Evidence (Before & After)
Before: timeline ticks were thicker/longer than the requested proportions, scheduled-task entries had no clock marker, detail text could occupy multiple lines, and locate flash highlighted the whole message row.
After: timeline ticks match the designer-requested sizing and spacing, scheduled-task entries show a clock icon, detail text stays on one line, and locate flash is confined to the target message content or bubble.
Tested on
Environment (optional)
Local Node.js workspace on macOS. Verified with
cd packages/web-shell && npx vitest run client/components/MessageList.dom.test.tsxandnpm run typecheck.Risk & Scope
Linked Issues
N/A
中文说明
这个 PR 做了什么
这个 PR 打磨左侧会话时间轴和消息定位高亮效果。时间轴横线现在使用设计师要求的比例：基础横线是 10 x 3px，hover 最长横线是 28 x 3px，横线之间的视觉间距是 12px。定时任务 turn 会在时间轴详情里显示时钟图标，时间轴详情文本最多一行并超出省略。消息定位高亮现在只渲染在目标消息内容或用户气泡内，不再高亮整行。
为什么需要
这些视觉调整来自设计要求。时间轴需要呈现为紧凑、节奏精确的导航轨道，定时任务 turn 需要明确的视觉标记。定位高亮应该把注意力引到实际目标消息，而不是铺满整条 transcript row。
Reviewer Test Plan
如何验证
打开一个至少有四个 turn 的 session，让左侧会话时间轴可见。hover 时间轴条目，确认横线符合 10 x 3px 基础尺寸、28 x 3px 最长 hover 长度，以及 12px 视觉间距。打开或回放一个定时任务 turn，确认时间轴详情显示时钟图标并保持单行。点击一个时间轴条目，确认定位高亮只出现在目标消息内容或用户气泡上，而不是整行。
证据（Before & After）
Before：时间轴横线比设计要求更粗/更长，定时任务条目没有时钟标记，详情文本可能占多行，定位高亮会覆盖整条消息 row。
After：时间轴横线符合设计师要求的尺寸和间距，定时任务条目显示时钟图标，详情文本保持一行，定位高亮局限在目标消息内容或气泡内。
Tested on
环境（可选）
macOS 本地 Node.js workspace。已用
cd packages/web-shell && npx vitest run client/components/MessageList.dom.test.tsx和npm run typecheck验证。风险和范围
关联 Issue
N/A