@qwen-code /triage

Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs.

Thanks for the triage check — updated the PR description to follow the template: What this PR does / Why it's needed, a structured Reviewer Test Plan (How to verify · Before/After evidence · Tested-on OS table), Risk & Scope, Linked Issues, and the full <details> 中文说明.

@qwen-code /triage

Addressed the /qreview + /review findings in bada65bf55:

• [Critical] id collision (todos.ts) — the running state is now keyed by id + content (todoStateKey), so a later plan's reused id (ACP positional ids / per-plan renumbering) no longer diffs against a previous plan's stale status and drops events. Added a regression test (two plans both using id 1 with different content).
• Context re-render churn (App.tsx) — todoTimeline is now a signature-cached Map (todoTimelineSignature + a ref), so streaming ticks that don't touch a todo snapshot keep the provider value referentially stable; todo/plan rows no longer re-render per token.
• completed/total unused / two sources of truth — dropped from TodoSnapshotDiff entirely; consumers compute the count locally.
• Unparseable payload (ToolGroup) — falls back to the raw result summary when extractTodosFromToolCall returns undefined (with a test).
• Stale "step time" comments — removed from TodoView.tsx, TodoView.module.css, and the TodoTimelineContext JSDoc.
• Test coverage — added PlanMessage.test.tsx (collapse/expand, progress header, plan-keyed diff, all-done / pending-focus / empty fallbacks), extractTodosFromToolCall tests (args / rawOutput.todos / rawOutput.entries / non-todo guard), and a mixed plan+tool computeTodoTimeline test.
• Deferred (minor) — sharing a single message walk between getFloatingTodos and computeTodoTimeline: each is O(messages) and cheap, and the re-render cost (the real concern) is now fixed.

All 286 web-shell tests pass; eslint + typecheck clean; app + lib build green. Resolving the threads below.

Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs.

Local verification incl. real-browser render (merge reference)

Verified a1dee77a1e on Linux (Node 22.22.2): full web-shell suite, targeted mutation on the two highest-risk fixes, and — since this is a rendering change — a real headless-Chromium render (Playwright) of the actual ToolGroup → TodoView components against a todo_write (wire name) tool call, not just jsdom.

Verdict: works end-to-end in a real browser, the review findings (incl. the [Critical] id-collision) are addressed at this head, and the diff-keying gaps are bounded and explicitly tested. Good to merge.

1. Real-browser render (headless Chromium via Playwright)

Mounted the real components with a 3-item todo_write snapshot (1 completed / 1 in-progress / 1 pending) + a timeline diff, served via Vite, driven in Chromium. No page/console errors.

Collapsed (default):

▸ ✓ TodoWrite 1/3
   ● Implement parser
   ◐ Add unit tests

→ N/M header 1/3, ▸ chevron, ● just-completed + ◐ just-started diff glyphs, current step shown, the pending item hidden.

Expanded (after a real click on the header):

▾ ✓ TodoWrite 1/3
   ● Implement parser     (struck-through)
   ◐ Add unit tests
   ○ Wire up CLI

→ ▾ chevron, full list incl. the ○ pending item, all three ● ◐ ○ glyphs, completed item rendered with strikethrough (real CSS applied — beyond what jsdom shows). Matches the PR's Before/After exactly. Detection by the wire name todo_write (kind think) is what makes this rich block appear at all.

2. Test suite — green

vitest run over packages/web-shell: 291 passed (19 files). (A first run showed 5 build-artifact.test.ts failures purely because dist/ wasn't built yet; they pass once npm run build completes — app + lib build is green.)

3. Mutation — the two key fixes are non-vacuous

So the detection-by-name fix and the id+content keying (the [Critical] review finding) are each genuinely guarded.

4. Review findings + known limits

• The /qreview + /review items are addressed at this head: the [Critical] id-collision (id+content keying), the context re-render churn (signature-cached timeline Map), the dropped completed/total duplicate, the unparseable-payload fallback, and the stale step-time comments — each with a test.
• The two diff-keying gaps are honestly documented in todoStateKey's JSDoc and pinned by tests (…known gap): a mid-task reword on a stable id, and two unrelated plans reusing both id and exact content. Both degrade only to "the collapsed diff omits one event" while the expanded checklist stays correct — an acceptable trade for the daemon path, where ids are not globally unique.

Conclusion: end-to-end correct in a real browser, fully tested (incl. the regression that the [Critical] fix prevents), with bounded, documented edge cases. Good to merge.

▸ ✓ TodoWrite 1/3
   ● Implement parser
   ◐ Add unit tests

▾ ✓ TodoWrite 1/3
   ● Implement parser     (删除线)
   ◐ Add unit tests
   ○ Wire up CLI

@qwen-code /triage

Thanks for the PR @wenshao!

Template looks good ✓ — all required sections present, bilingual, with Before/After evidence and OS table.

On direction: This fixes a real bug — the web-shell daemon path sends todo_write as the wire name but the inline renderer matched the literal string todowrite, so the rich todo UI never activated and the floating panel stayed empty. That's a genuine regression in the daemon experience. The collapsible rendering on top is a natural UX companion, not a bolt-on. CHANGELOG shows steady web-shell and todo-related work (sticky todo panel, Option+Enter shortcuts, hooks), so this area is clearly active and aligned.

On approach: The scope feels right. The bug fix (isTodoWriteToolName matching both names) is minimal and necessary. The collapsible UI with status diffs is a reasonable companion — it ships in the same PR because it touches the same rendering path. The shared TodoView component reused by both ToolGroup and PlanMessage avoids duplication. The computeTodoTimeline with id+content keying addresses a real constraint (ids aren't globally unique across plans in the daemon path), and the documented edge cases (mid-task reword, content collision) are honest tradeoffs.

One thing worth noting: the TodoTimelineContext provider wraps the entire content area, but only todo/plan rows consume it. The signature-cached Map with useRef keeps this stable across streaming ticks — good call on preventing re-render churn.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

2a. Code Review

Independent proposal (before reading the diff):
I'd fix the name mismatch by adding an isTodoWriteToolName() helper matching both todo_write and todowrite. Each todo_write call should get its own tool_group so it doesn't merge with adjacent tool calls. For the UX side, a collapsible component with a progress count (completed/total) that defaults to showing just the current step. For status diffs, walk the transcript in order and track transitions per item.

Comparison with the PR:
The PR's approach matches my proposal almost exactly — isTodoWriteToolName(), standalone groups via isStandalone(), a shared TodoView with TodoEventSummary (collapsed) and TodoFullList (expanded), and computeTodoTimeline for transitions. The id+content keying (todoStateKey) goes beyond my proposal by addressing the real problem of non-unique ids across plans — good call.

Findings:

No critical blockers. The code is well-structured and the previous review round's findings (id collision, context re-render, duplicate completed/total) are all addressed at this head with regression tests.

A few observations (non-blocking):

• isTodoWriteToolName normalizes to lowercase internally, but in ToolGroup.tsx the caller already does tool.toolName.toLowerCase() before passing. Harmless (idempotent), just a minor redundancy.
• The extractTodosFromToolCall guard !isTodoWriteToolName(tool.toolName) && tool.kind !== 'other' means a non-todo tool with kind: 'other' would still be checked for todo arrays. This is the original behavior — the PR correctly preserves it.
• The todoStateKey using JSON.stringify([id, content]) is a pragmatic choice. The two documented edge cases (mid-task reword, content collision) degrade gracefully — collapsed diff omits one event, expanded list stays correct.

2b. Testing

Unit tests (applied PR diff to worktree, ran locally):

$ cd packages/web-shell && npx vitest run

 ✓ utils/todos.test.ts (33 tests)
 ✓ adapters/transcriptToMessages.test.ts (88 tests)
 ✓ components/messages/ToolGroup.test.tsx (16 tests)
 ✓ components/messages/PlanMessage.test.tsx (6 tests)
 ... (19 files total)

 Test Files  19 passed (19)
      Tests  292 passed (292)
   Duration  5.66s

Lint: Clean on all 6 changed source files.

CI (GitHub Actions): macOS ✅, Ubuntu ✅, Lint ✅, CodeQL ✅, Windows pending.

Real-browser testing: tmux-based testing doesn't apply here — the web-shell is a browser-based UI rendered by Vite, not a terminal TUI. The qwen CLI in tmux would test the Ink-based terminal renderer, not the web-shell components. The author's own Playwright/headless-Chromium verification (posted in this comment) covers real-browser rendering: collapsed view shows ▸ TodoWrite 1/3 with ●/◐ glyphs, expanded view shows the full numbered list with strikethrough on completed items, matching the PR's Before/After exactly. Mutation testing confirms the two key fixes (name detection, id+content keying) are non-vacuous — reverting each causes specific test failures.

Test Files  19 passed (19)
     Tests  292 passed (292)
  Duration  5.66s

— Qwen Code · qwen3.7-max

Stepping back: this PR fixes a genuine regression in the web-shell daemon path (todo rendering broken due to name mismatch) and pairs it with a well-designed collapsible UI. The implementation is straightforward — a shared TodoView component, a timeline computed once per snapshot change, and clean separation between collapsed (event diff) and expanded (full list) views.

The code reads like someone who thought about the problem, not someone who threw abstractions at it. The id+content keying for todoStateKey is the right call given the daemon path's non-unique ids, and the two documented edge cases (reword, content collision) are honestly traded. The signature-cached Map for TodoTimelineContext is a pragmatic fix for the streaming re-render problem without over-engineering it.

My independent proposal matched the PR's approach, and the PR went further in the right ways (id+content keying, mutation-tested key fixes). 292 tests pass, lint is clean, CI green on macOS + Ubuntu. The author's own Playwright verification covers real-browser rendering with mutation testing to prove the fixes aren't vacuous.

This is ready to ship. ✅

— Qwen Code · qwen3.7-max

Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs.