From 9f2168f78ea531cc3d2f9b58b645d671bfdecc0c Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Mon, 15 Jun 2026 00:44:15 +0800 Subject: [PATCH] feat(web-shell): collapsible TodoWrite history with status diff (#5109) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(web-shell): collapsible TodoWrite history with status diff Inline todo_write updates rendered as a generic, non-collapsible tool row crammed in with surrounding tool calls — the todo-specific renderer was effectively dead code because the detector matched the literal "todowrite" while the wire name is "todo_write" (kind "think"). - Detect the todo tool by name (todo_write / todowrite) instead of relying on the unrelated tool kind, reviving the rich rendering and also populating the floating todo panel that was empty on the daemon path. - Render each update as its own standalone group, collapsed by default to the per-snapshot diff (just-completed / just-started items) or the current step, expanding to the full checklist; the header shows completed/total. - Use consistent status glyphs (●/◐/○) in both collapsed and expanded views via a shared TodoView component used by ToolGroup and PlanMessage. * fix(web-shell): scope todo diff per task identity; address review - [Critical] computeTodoTimeline keyed its running state on todo.id alone, but ids aren't globally unique (ACP assigns positional ids, models renumber per plan), so a later plan diffed against a previous plan's stale terminal status and silently dropped events in the collapsed view. Key on id+content so distinct tasks stay separate; add an id-reuse regression test. - Stabilize the TodoTimelineContext value with a signature-cached Map so streaming ticks that don't touch a todo snapshot no longer re-render every todo/plan row. - Drop the unused completed/total from TodoSnapshotDiff (consumers compute the count locally — single source of truth). - Fall back to the raw result summary when a todo_write payload is unparseable. - Add PlanMessage rendering tests and extractTodosFromToolCall coverage; remove stale "step time" comments left after dropping per-step timing. * test(web-shell): document todo-diff keying limits; cover signature Follow-up to review on the id+content keying in computeTodoTimeline: - Document the two rare trade-offs in the todoStateKey doc (a mid-task reword on a stable id, and unrelated plans reusing both id and content), both degrading to "the collapsed diff omits one event" while the expanded list stays correct. - Pin behavior with tests: an item carried over and completed in a later turn (which id+content handles but a user-turn reset would drop), plus the two documented gaps. - Add todoTimelineSignature tests: stable across non-todo edits; changes on any id/status/content change. * refactor(web-shell): isolate plan context read; expand todo test coverage Address follow-up review: - Extract PlanEventSummary as the sole TodoTimelineContext consumer, mirroring ToolGroup's TodoToolBody so the memo-shielded PlanMessage stays stable when the timeline Map reference changes. - Note the spurious-`started` axis of the reword trade-off in the todoStateKey doc (a reword can drop a completion or emit a stray start). - Add direct isTodoWriteToolName tests (incl. the `todowrite` ACP variant) and a todoTimelineSignature empty-transcript test. --- packages/web-shell/client/App.tsx | 293 ++++++++++-------- .../adapters/transcriptToMessages.test.ts | 30 +- .../client/adapters/transcriptToMessages.ts | 10 +- .../client/components/MessageItem.tsx | 2 +- .../messages/PlanMessage.module.css | 54 ++-- .../components/messages/PlanMessage.test.tsx | 134 ++++++++ .../components/messages/PlanMessage.tsx | 66 ++-- .../components/messages/TodoView.module.css | 59 ++++ .../client/components/messages/TodoView.tsx | 115 +++++++ .../components/messages/ToolGroup.test.tsx | 109 ++++++- .../client/components/messages/ToolGroup.tsx | 119 +++---- .../messages/tools/ToolChrome.module.css | 31 +- packages/web-shell/client/utils/todos.test.ts | 266 +++++++++++++++- packages/web-shell/client/utils/todos.ts | 140 ++++++++- 14 files changed, 1137 insertions(+), 291 deletions(-) create mode 100644 packages/web-shell/client/components/messages/PlanMessage.test.tsx create mode 100644 packages/web-shell/client/components/messages/TodoView.module.css create mode 100644 packages/web-shell/client/components/messages/TodoView.tsx diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index b5e7008abb..968329496e 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -120,7 +120,12 @@ import { import { TASKS_STATUS_ACTIVE_EVENT } from './components/messages/TasksStatusMessage'; import { BtwMessage } from './components/messages/BtwMessage'; import type { ACPToolCall, Message, PermissionRequest } from './adapters/types'; -import { getFloatingTodos } from './utils/todos'; +import { + computeTodoTimeline, + getFloatingTodos, + todoTimelineSignature, + type TodoSnapshotDiff, +} from './utils/todos'; import { ThemeProvider } from './themeContext'; import { WebShellThemeId, @@ -140,6 +145,16 @@ import styles from './App.module.css'; export const CompactModeContext = createContext(false); +/** + * Per-snapshot status diffs (keyed by tool callId or plan message id), so a + * history row can render what changed in that snapshot without re-deriving it + * from the whole transcript. Empty by default so a row rendered outside the + * provider still falls back gracefully. + */ +export const TodoTimelineContext = createContext>( + new Map(), +); + const MODES_CYCLE = DAEMON_APPROVAL_MODES; const MAX_DISPLAYED_QUEUED_PROMPTS = 3; const MAX_QUEUED_PROMPT_PREVIEW_CHARS = 240; @@ -672,6 +687,22 @@ export function App({ () => getFloatingTodos(messages), [messages], ); + // Keep the timeline Map referentially stable across streaming ticks that + // don't touch any todo snapshot. The Map is a context value, so a fresh + // reference would re-render every todo/plan row regardless of memoization; + // only rebuild when the todo snapshots themselves change. + const todoTimelineRef = useRef<{ + signature: string; + timeline: Map; + } | null>(null); + const todoTimeline = useMemo(() => { + const signature = todoTimelineSignature(messages); + const cached = todoTimelineRef.current; + if (cached && cached.signature === signature) return cached.timeline; + const timeline = computeTodoTimeline(messages); + todoTimelineRef.current = { signature, timeline }; + return timeline; + }, [messages]); const floatingTodos = useStableArray( floatingTodosState.todos, (t) => `${t.id}:${t.status}:${t.content}`, @@ -2552,138 +2583,140 @@ export function App({ -
- - {authInlineOpen && ( - { - store.dispatch([ - type === 'error' - ? { type: 'error', text } - : { type: 'status', text }, - ]); - }} - onClose={() => setAuthInlineOpen(false)} - /> - )} - {approvalModeInlineOpen && ( - setApprovalModeInlineOpen(false)} - /> - )} - {modelInlineMode && ( - setModelInlineMode(null)} - /> - )} - {agentsInlineMode && ( - - store.dispatch([{ type: 'status', text }]) - } - onClose={() => setAgentsInlineMode(null)} - /> - )} - {memoryInlineOpen && ( - { - store.dispatch([{ type, text }]); - }} - onClose={() => setMemoryInlineOpen(false)} - /> - )} - {settingsInlineOpen && ( - setSettingsInlineOpen(false)} - onLanguageChange={handleSettingsLanguageChange} - onThemeChange={handleThemeChange} - onSubDialog={(key) => { - setSettingsInlineOpen(false); - if (key === 'fastModel') - setModelInlineMode('fast'); - else if (key === 'tools.approvalMode') - setApprovalModeInlineOpen(true); - }} - /> - )} - - ) : undefined + +
+ style={dialogOpen ? { visibility: 'hidden' } : undefined} + > + + {authInlineOpen && ( + { + store.dispatch([ + type === 'error' + ? { type: 'error', text } + : { type: 'status', text }, + ]); + }} + onClose={() => setAuthInlineOpen(false)} + /> + )} + {approvalModeInlineOpen && ( + setApprovalModeInlineOpen(false)} + /> + )} + {modelInlineMode && ( + setModelInlineMode(null)} + /> + )} + {agentsInlineMode && ( + + store.dispatch([{ type: 'status', text }]) + } + onClose={() => setAgentsInlineMode(null)} + /> + )} + {memoryInlineOpen && ( + { + store.dispatch([{ type, text }]); + }} + onClose={() => setMemoryInlineOpen(false)} + /> + )} + {settingsInlineOpen && ( + setSettingsInlineOpen(false)} + onLanguageChange={handleSettingsLanguageChange} + onThemeChange={handleThemeChange} + onSubDialog={(key) => { + setSettingsInlineOpen(false); + if (key === 'fastModel') + setModelInlineMode('fast'); + else if (key === 'tools.approvalMode') + setApprovalModeInlineOpen(true); + }} + /> + )} + + ) : undefined + } + tailKey={ + agentsInlineMode || + memoryInlineOpen || + modelInlineMode || + authInlineOpen || + approvalModeInlineOpen || + settingsInlineOpen + ? `inline-${authInlineOpen ? 'auth' : 'none'}-${modelInlineMode ?? 'none'}-${agentsInlineMode ?? 'none'}-${memoryInlineOpen ? 'memory' : 'none'}-${approvalModeInlineOpen ? 'approval' : 'none'}-${settingsInlineOpen ? 'settings' : 'none'}` + : undefined + } + // The approval-mode/model pickers and the settings panel are + // reachable by mouse from the status bar, so they reveal + // themselves when opened while the user is scrolled up; the + // agents/memory panels keep the user's scroll position. + autoScrollTailIntoView={ + approvalModeInlineOpen || + modelInlineMode !== null || + settingsInlineOpen + } + virtualScrollThreshold={virtualScrollThreshold} + /> - {btwMessage?.role === 'btw' && ( -
- -
- )} + {btwMessage?.role === 'btw' && ( +
+ +
+ )} - -
-
+ +
+
+ diff --git a/packages/web-shell/client/adapters/transcriptToMessages.test.ts b/packages/web-shell/client/adapters/transcriptToMessages.test.ts index f062fa86dc..0e4a4f3a17 100644 --- a/packages/web-shell/client/adapters/transcriptToMessages.test.ts +++ b/packages/web-shell/client/adapters/transcriptToMessages.test.ts @@ -229,8 +229,9 @@ describe('transcriptBlocksToDaemonMessages', () => { }), ]); - // Adjacent tool blocks share one tool_group (Native CLI batch parity), - // but each TodoWrite call keeps its own tool entry and todo payload. + // Each TodoWrite update stands alone in its own group (it renders as a + // self-contained collapsible checklist), rather than merging with adjacent + // tool calls. expect(messages).toEqual([ { id: 'tg-todo-1', @@ -241,6 +242,13 @@ describe('transcriptBlocksToDaemonMessages', () => { callId: 'todo-call-1', toolName: 'TodoWrite', }), + ], + }, + { + id: 'tg-todo-2', + role: 'tool_group', + timestamp: 2, + tools: [ expect.objectContaining({ callId: 'todo-call-2', toolName: 'TodoWrite', @@ -332,6 +340,24 @@ describe('transcriptBlocksToDaemonMessages', () => { ]); }); + it('never merges todo_write updates into or after a regular tool_group', () => { + const messages = transcriptBlocksToDaemonMessages([ + toolBlock('t1', 'tc1', 'completed', 1, { toolName: 'Read' }), + toolBlock('todo-1', 'todo-call-1', 'completed', 2, { + toolName: 'todo_write', + toolKind: 'think', + rawInput: { todos: [{ id: '1', content: 'A', status: 'in_progress' }] }, + }), + toolBlock('t2', 'tc2', 'completed', 3, { toolName: 'Edit' }), + ]); + + expect(messages).toMatchObject([ + { role: 'tool_group', tools: [{ callId: 'tc1' }] }, + { role: 'tool_group', tools: [{ callId: 'todo-call-1' }] }, + { role: 'tool_group', tools: [{ callId: 'tc2' }] }, + ]); + }); + it('does not merge real tool calls into synthetic raw-shell groups', () => { const messages = transcriptBlocksToDaemonMessages([ textBlock('u1', 'user', 'run it', 1), diff --git a/packages/web-shell/client/adapters/transcriptToMessages.ts b/packages/web-shell/client/adapters/transcriptToMessages.ts index 70d1e29b53..47b5d1709d 100644 --- a/packages/web-shell/client/adapters/transcriptToMessages.ts +++ b/packages/web-shell/client/adapters/transcriptToMessages.ts @@ -21,6 +21,7 @@ import type { DaemonMessageTodoItem, DaemonUserMessage, } from './messageTypes.js'; +import { isTodoWriteToolName } from '../utils/todos.js'; interface PermissionToolInfo { title?: string; @@ -509,13 +510,18 @@ function appendToolCallMessage( // // Synthetic raw-shell groups (pushed by the `shell` block fallback) use the // bare block id without the `tg-` prefix and never absorb real tool calls. + // Sub-agent calls and todo_write updates each stand alone in their own group + // box instead of being crammed in with the tools around them: an agent renders + // an expandable panel, and a todo update is its own collapsible checklist. + const isStandalone = (t: DaemonMessageToolCall) => + isSubAgentToolCall(t) || isTodoWriteToolName(t.toolName); const last = messages[messages.length - 1]; if ( last && last.role === 'tool_group' && last.id.startsWith('tg-') && - !isSubAgentToolCall(toolCall) && - !last.tools.some(isSubAgentToolCall) + !isStandalone(toolCall) && + !last.tools.some(isStandalone) ) { last.tools.push(toolCall); return; diff --git a/packages/web-shell/client/components/MessageItem.tsx b/packages/web-shell/client/components/MessageItem.tsx index a0a9762ab2..bddec9ad40 100644 --- a/packages/web-shell/client/components/MessageItem.tsx +++ b/packages/web-shell/client/components/MessageItem.tsx @@ -69,7 +69,7 @@ export const MessageItem = memo(function MessageItem({ /> ); case 'plan': - return ; + return ; case 'system': return ( { + const { createContext } = await import('react'); + return { TodoTimelineContext: createContext(new Map()) }; +}); + +const { PlanMessage } = await import('./PlanMessage'); +const { TodoTimelineContext } = await import('../../App'); + +( + globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean } +).IS_REACT_ACT_ENVIRONMENT = true; + +const mounted: Array<{ root: Root; container: HTMLElement }> = []; + +afterEach(() => { + for (const { root, container } of mounted.splice(0)) { + act(() => root.unmount()); + container.remove(); + } +}); + +function todo( + id: string, + content: string, + status: TodoItem['status'], +): TodoItem { + return { id, content, status }; +} + +function renderPlan( + id: string, + todos: TodoItem[], + timeline?: Map, +): HTMLElement { + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + act(() => { + root.render( + + + + + , + ); + }); + mounted.push({ root, container }); + return container; +} + +function click(el: Element): void { + act(() => { + el.dispatchEvent(new MouseEvent('click', { bubbles: true })); + }); +} + +const TODOS = [ + todo('1', 'First task', 'completed'), + todo('2', 'Second task', 'in_progress'), + todo('3', 'Third task', 'pending'), +]; + +describe('PlanMessage', () => { + it('collapses to the current step with a progress count', () => { + const container = renderPlan('p1', TODOS); + expect(container.textContent).toContain('1/3'); + expect(container.textContent).toContain('Second task'); + expect(container.textContent).not.toContain('Third task'); + expect(container.textContent).toContain('▸'); + }); + + it('expands to the full list on click', () => { + const container = renderPlan('p1', TODOS); + const chevron = [...container.querySelectorAll('span')].find( + (s) => s.textContent === '▸', + ); + click(chevron!.parentElement!); + expect(container.textContent).toContain('First task'); + expect(container.textContent).toContain('Second task'); + expect(container.textContent).toContain('Third task'); + expect(container.textContent).toContain('▾'); + }); + + it('shows the plan-keyed diff when a timeline is present', () => { + const timeline = new Map([ + [ + 'p1', + { + events: [ + { kind: 'completed', id: '1', content: 'First task' }, + { kind: 'started', id: '2', content: 'Second task' }, + ], + }, + ], + ]); + const container = renderPlan('p1', TODOS, timeline); + expect(container.textContent).toContain('First task'); + expect(container.textContent).toContain('Second task'); + expect(container.textContent).not.toContain('Third task'); + }); + + it('shows an all-done summary when every item is completed', () => { + const container = renderPlan('p1', [ + todo('1', 'First task', 'completed'), + todo('2', 'Second task', 'completed'), + ]); + expect(container.textContent).toContain('2/2'); + expect(container.textContent).toContain('All tasks completed'); + }); + + it('falls back to the first pending item when nothing is in progress', () => { + const container = renderPlan('p1', [ + todo('1', 'First task', 'pending'), + todo('2', 'Second task', 'pending'), + ]); + expect(container.textContent).toContain('0/2'); + expect(container.textContent).toContain('First task'); + expect(container.textContent).not.toContain('Second task'); + }); + + it('renders nothing for an empty plan', () => { + const container = renderPlan('p1', []); + expect(container.textContent).toBe(''); + }); +}); diff --git a/packages/web-shell/client/components/messages/PlanMessage.tsx b/packages/web-shell/client/components/messages/PlanMessage.tsx index eb0f38ce5f..2182ecba57 100644 --- a/packages/web-shell/client/components/messages/PlanMessage.tsx +++ b/packages/web-shell/client/components/messages/PlanMessage.tsx @@ -1,53 +1,57 @@ -import { memo } from 'react'; +import { memo, useContext, useState } from 'react'; import type { TodoItem } from '../../adapters/types'; -import { getTodoStatusIcon } from '../../utils/todos'; +import { TodoTimelineContext } from '../../App'; +import { TodoEventSummary, TodoFullList } from './TodoView'; import { useI18n } from '../../i18n'; import styles from './PlanMessage.module.css'; interface PlanMessageProps { + id: string; todos: TodoItem[]; } -function getStatusClass(status: TodoItem['status']): string { - switch (status) { - case 'completed': - return styles.completed; - case 'in_progress': - return styles.inProgress; - case 'pending': - return ''; - } +// Isolating the context read here (mirroring ToolGroup's TodoToolBody) keeps the +// memo-shielded PlanMessage from re-rendering when the timeline Map reference +// changes — only this small summary does. +function PlanEventSummary({ id, todos }: PlanMessageProps) { + const timeline = useContext(TodoTimelineContext); + const events = timeline.get(id)?.events ?? []; + return ; } export const PlanMessage = memo(function PlanMessage({ + id, todos, }: PlanMessageProps) { const { t } = useI18n(); + const [expanded, setExpanded] = useState(false); if (todos.length === 0) return null; - // Size the number column to the widest index so the status markers stay - // aligned once the list grows past 9 items. - const numColumnWidth = `${String(todos.length).length + 1}ch`; + const total = todos.length; + const completed = todos.filter((td) => td.status === 'completed').length; return (
-
{t('plan.title')}
-
- {todos.map((todo, index) => ( -
- - {index + 1}. - - - {getTodoStatusIcon(todo.status)} - - {todo.content} -
- ))} -
+ + {expanded ? ( + + ) : ( + + )}
); }); diff --git a/packages/web-shell/client/components/messages/TodoView.module.css b/packages/web-shell/client/components/messages/TodoView.module.css new file mode 100644 index 0000000000..aefd7678af --- /dev/null +++ b/packages/web-shell/client/components/messages/TodoView.module.css @@ -0,0 +1,59 @@ +.list, +.summary { + display: flex; + flex-direction: column; + gap: 2px; +} + +.row { + display: flex; + align-items: baseline; + gap: 6px; + font-size: 13px; + line-height: 1.5; +} + +.num { + flex-shrink: 0; + text-align: right; + color: var(--text-dimmed); + font-variant-numeric: tabular-nums; +} + +.icon { + width: 14px; + flex-shrink: 0; + color: var(--text-dimmed); +} + +.text { + color: var(--text-secondary); +} + +/* The collapsed summary keeps each row to one line so a long task title never + wraps; the expanded list stays free to wrap and show the full text. */ +.summary .text { + flex: 1; + min-width: 0; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} + +.inProgress .icon, +.inProgress .text { + color: var(--accent-color); +} + +.completed .icon { + color: var(--success-color); +} + +/* Strike-through belongs to the persistent expanded list, where a completed + item lingers as crossed-off. A summary row is an event ("just completed") + that reads as an accomplishment, so it stays in normal text. */ +.list .completed .text { + color: var(--text-dimmed); + text-decoration: line-through; + text-decoration-color: var(--text-dimmed); +} diff --git a/packages/web-shell/client/components/messages/TodoView.tsx b/packages/web-shell/client/components/messages/TodoView.tsx new file mode 100644 index 0000000000..00a071587b --- /dev/null +++ b/packages/web-shell/client/components/messages/TodoView.tsx @@ -0,0 +1,115 @@ +import type { TodoItem } from '../../adapters/types'; +import { getTodoStatusIcon, type TodoEvent } from '../../utils/todos'; +import { useI18n } from '../../i18n'; +import styles from './TodoView.module.css'; + +function statusClass(status: TodoItem['status']): string { + switch (status) { + case 'completed': + return styles.completed; + case 'in_progress': + return styles.inProgress; + case 'pending': + return ''; + } +} + +/** + * Collapsed view: the change a single snapshot introduced — items that just + * completed and items that just started. With no tracked change (an unchanged + * re-emit, or a snapshot rendered without a timeline) it falls back to the + * current focus item so the row is never empty. + */ +export function TodoEventSummary({ + todos, + events, +}: { + todos: TodoItem[]; + events: readonly TodoEvent[]; +}) { + const { t } = useI18n(); + + if (events.length === 0) { + const allCompleted = + todos.length > 0 && todos.every((td) => td.status === 'completed'); + if (allCompleted) { + return ( +
+
+ + {t('todo.allDone')} +
+
+ ); + } + const current = + todos.find((td) => td.status === 'in_progress') ?? + todos.find((td) => td.status === 'pending'); + if (!current) return null; + return ( +
+
+ + {current.content} +
+
+ ); + } + + return ( +
+ {events.map((event) => ( +
+ + {event.content} +
+ ))} +
+ ); +} + +/** Expanded view: the full list. `numbered` adds the 1. 2. 3. index column. */ +export function TodoFullList({ + todos, + numbered = false, +}: { + todos: TodoItem[]; + numbered?: boolean; +}) { + // Size the number column to the widest index so the markers stay aligned once + // the list grows past 9 items. + const numColumnWidth = `${String(todos.length).length + 1}ch`; + return ( +
+ {todos.map((todo, index) => ( +
+ {numbered && ( + + {index + 1}. + + )} + + {todo.content} +
+ ))} +
+ ); +} diff --git a/packages/web-shell/client/components/messages/ToolGroup.test.tsx b/packages/web-shell/client/components/messages/ToolGroup.test.tsx index cafd4606fd..c89f87150a 100644 --- a/packages/web-shell/client/components/messages/ToolGroup.test.tsx +++ b/packages/web-shell/client/components/messages/ToolGroup.test.tsx @@ -5,14 +5,19 @@ import { createRoot, type Root } from 'react-dom/client'; import { I18nProvider } from '../../i18n'; import type { ACPToolCall } from '../../adapters/types'; -// ToolGroup imports App only for CompactModeContext; loading the real App -// module would pull the whole application graph into this unit test. +// ToolGroup imports App only for CompactModeContext and TodoTimelineContext; +// loading the real App module would pull the whole application graph into this +// unit test. vi.mock('../../App', async () => { const { createContext } = await import('react'); - return { CompactModeContext: createContext(false) }; + return { + CompactModeContext: createContext(false), + TodoTimelineContext: createContext(new Map()), + }; }); const { ToolGroup } = await import('./ToolGroup'); +const { TodoTimelineContext } = await import('../../App'); ( globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean } @@ -321,7 +326,105 @@ describe('auto-collapse on finish', () => { render('completed'); expect(container.textContent).toContain('SubToolMarker'); }); +}); +function makeTodoTool(): ACPToolCall { + return { + callId: 'call-todo-1', + toolName: 'todo_write', + status: 'completed', + kind: 'think', + args: { + todos: [ + { id: '1', content: 'First task', status: 'completed' }, + { id: '2', content: 'Second task', status: 'in_progress' }, + { id: '3', content: 'Third task', status: 'pending' }, + ], + }, + }; +} + +function renderTodoTool( + tool: ACPToolCall, + timeline?: Map, +): HTMLElement { + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + act(() => { + root.render( + + + + + , + ); + }); + mounted.push({ root, container }); + return container; +} + +describe('todo_write tool rendering', () => { + it('detects the todo_write wire name and collapses to the current step', () => { + const container = renderTodoTool(makeTodoTool()); + // Collapsed by default: only the current (in_progress) step and the + // progress count show; other items stay hidden until expanded. + expect(container.textContent).toContain('1/3'); + expect(container.textContent).toContain('Second task'); + expect(container.textContent).not.toContain('Third task'); + expect(container.textContent).toContain('▸'); + }); + + it('expands to the full list on click', () => { + const container = renderTodoTool(makeTodoTool()); + const chevron = [...container.querySelectorAll('span')].find( + (s) => s.textContent === '▸', + ); + click(chevron!.parentElement!); + expect(container.textContent).toContain('First task'); + expect(container.textContent).toContain('Second task'); + expect(container.textContent).toContain('Third task'); + expect(container.textContent).toContain('▾'); + }); + + it('shows the snapshot diff when a timeline is present', () => { + const timeline = new Map([ + [ + 'call-todo-1', + { + events: [ + { kind: 'completed', id: '1', content: 'First task' }, + { kind: 'started', id: '2', content: 'Second task' }, + ], + }, + ], + ]); + const container = renderTodoTool(makeTodoTool(), timeline); + // The collapsed diff: just-completed item (●), just-started item (◐), and + // pending items still hidden — same status glyphs as the expanded list. + expect(container.textContent).toContain('●'); + expect(container.textContent).toContain('First task'); + expect(container.textContent).toContain('◐'); + expect(container.textContent).toContain('Second task'); + expect(container.textContent).not.toContain('Third task'); + }); + + it('falls back to the result summary when the todo payload is unparseable', () => { + // Malformed args (todos is a string) → no list to render; the row must not + // be blank — it shows the raw result summary instead. + const container = renderTodoTool({ + callId: 'call-todo-bad', + toolName: 'todo_write', + status: 'completed', + kind: 'think', + args: { todos: 'oops not an array' }, + rawOutput: { output: 'Todos updated summary line' }, + }); + expect(container.textContent).toContain('Todos updated summary line'); + }); +}); + +describe('user-expanded tool persistence', () => { it('keeps a tool the user manually expanded open when it completes', () => { const container = document.createElement('div'); document.body.appendChild(container); diff --git a/packages/web-shell/client/components/messages/ToolGroup.tsx b/packages/web-shell/client/components/messages/ToolGroup.tsx index cbea5873ba..282229c74a 100644 --- a/packages/web-shell/client/components/messages/ToolGroup.tsx +++ b/packages/web-shell/client/components/messages/ToolGroup.tsx @@ -13,7 +13,11 @@ import { SubAgentPanel } from './tools/SubAgentPanel'; import { DiffView } from './tools/DiffView'; import { ToolApproval } from './ToolApproval'; import { parseAnsi, hasAnsi } from '../../utils/ansi'; -import { extractTodosFromToolCall, getTodoStatusIcon } from '../../utils/todos'; +import { + extractTodosFromToolCall, + isTodoWriteToolName, +} from '../../utils/todos'; +import { TodoEventSummary, TodoFullList } from './TodoView'; import { formatDurationMs, formatElapsed, @@ -37,7 +41,7 @@ import { toolContainsCallId, } from './toolFormatting'; import { useI18n } from '../../i18n'; -import { CompactModeContext } from '../../App'; +import { CompactModeContext, TodoTimelineContext } from '../../App'; import { type ToolHeaderExtraRenderInfo, type ToolHeaderKind, @@ -333,58 +337,33 @@ function getWriteContent(tool: ACPToolCall): string { return ''; } -function TodoWriteContent({ tool }: { tool: ACPToolCall }) { - const todos = extractTodosFromToolCall(tool); - if (todos) { - return ( -
- {todos.map((todo, i) => ( -
- {getTodoStatusIcon(todo.status)} {todo.content} -
- ))} -
- ); - } - - const text = extractText(tool) || ''; - const lines = text.split('\n').filter((l) => l.trim()); - if (lines.length === 0) return null; - +// Collapsed by default: the diff of this todo_write call (just-completed and +// just-started items), expanding to the full list on click. The per-snapshot +// diff comes from the timeline context, so this is isolated in its own +// component — only todo rows subscribe and re-render when the timeline changes, +// not every tool row. +function TodoToolBody({ + tool, + todos, + expanded, +}: { + tool: ACPToolCall; + todos: TodoItem[]; + expanded: boolean; +}) { + const timeline = useContext(TodoTimelineContext); + const events = timeline.get(tool.callId)?.events ?? []; return ( -
- {lines.map((line, i) => { - const isCompleted = line.startsWith('●'); - const isInProgress = line.startsWith('◐'); - const cls = isCompleted - ? styles.todoDone - : isInProgress - ? styles.todoActive - : styles.todoPending; - return ( -
- {line} -
- ); - })} +
+ {expanded ? ( + + ) : ( + + )}
); } -function getTodoClass(status: TodoItem['status']): string { - switch (status) { - case 'completed': - return styles.todoDone; - case 'in_progress': - return styles.todoActive; - case 'pending': - return styles.todoPending; - } -} - interface ToolLineProps { tool: ACPToolCall; approval?: PermissionRequest | null; @@ -478,7 +457,7 @@ function getToolHeaderKind(tool: ACPToolCall): ToolHeaderKind { if (isSubAgentToolCall(tool)) return 'agent'; if (isShellToolName(name)) return 'shell'; if (isWebFetchToolName(name)) return 'fetch'; - if (name === 'todowrite') return 'todo'; + if (isTodoWriteToolName(name)) return 'todo'; if (name === 'read' || name === 'read_file' || name === 'readfile') return 'read'; if (name === 'edit' || name === 'editfile') return 'edit'; @@ -800,13 +779,20 @@ export const ToolLine = memo(function ToolLine({ : formatElapsed(tool.startTime, tool.endTime); const name = tool.toolName.toLowerCase(); - const isTodo = name === 'todowrite'; - // A row expands when it has detail output (bash/diff/read content) or when - // its description is long enough to be ellipsised. When a long description is - // expanded we move it out of the header into a wrapped block below, so the - // header drops its single-line copy. + const isTodo = isTodoWriteToolName(name); + const todoItems = isTodo ? extractTodosFromToolCall(tool) : undefined; + const hasTodoList = !!todoItems && todoItems.length > 0; + const todoCompleted = todoItems + ? todoItems.filter((td) => td.status === 'completed').length + : 0; + // A row expands when it has a todo list to reveal, detail output + // (bash/diff/read content), or a description long enough to be ellipsised. + // When a long description is expanded we move it out of the header into a + // wrapped block below, so the header drops its single-line copy. const descExpandable = !isTodo && isDescriptionExpandable(description); - const expandable = !isTodo && (hasExpandableContent(tool) || descExpandable); + const expandable = isTodo + ? hasTodoList + : hasExpandableContent(tool) || descExpandable; const relocateDescription = expanded && descExpandable; // Whether the expanded row renders a kind-specific detail view. When it does // not (e.g. grep/glob/web_fetch with a long description), keep the result @@ -837,18 +823,33 @@ export const ToolLine = memo(function ToolLine({ {displayName} + {isTodo && hasTodoList && ( + + {todoCompleted}/{todoItems!.length} + + )}
- {isTodo && } + {isTodo && hasTodoList && ( + + )} + {/* Todo tool whose payload couldn't be parsed (e.g. malformed args): + fall back to the raw result summary so the row isn't blank. */} + {isTodo && !hasTodoList && result && ( +
{result}
+ )} {relocateDescription && (
{description}
)} diff --git a/packages/web-shell/client/components/messages/tools/ToolChrome.module.css b/packages/web-shell/client/components/messages/tools/ToolChrome.module.css index 02f123e536..2d1aab2014 100644 --- a/packages/web-shell/client/components/messages/tools/ToolChrome.module.css +++ b/packages/web-shell/client/components/messages/tools/ToolChrome.module.css @@ -191,30 +191,15 @@ background: var(--info-bg); } -.todoList { - padding: 6px 0 4px 24px; - display: flex; - flex-direction: column; - gap: 2px; -} - -.todoItem { - font-size: 13px; - font-family: var(--font-mono); - line-height: 1.6; - padding-left: 4px; -} - -.todoDone { - color: var(--text-primary); -} - -.todoActive { - font-weight: 600; -} - -.todoPending { +.todoProgress { color: var(--text-dimmed); + font-size: 12px; + flex-shrink: 0; + font-variant-numeric: tabular-nums; +} + +.todoBody { + padding: 4px 0 2px 24px; } .compactGroup { diff --git a/packages/web-shell/client/utils/todos.test.ts b/packages/web-shell/client/utils/todos.test.ts index ce1877902f..32d8a781ae 100644 --- a/packages/web-shell/client/utils/todos.test.ts +++ b/packages/web-shell/client/utils/todos.test.ts @@ -1,11 +1,27 @@ import { describe, expect, it } from 'vitest'; -import type { Message, TodoItem } from '../adapters/types'; -import { getFloatingTodos, getTodoStatusIcon, getTodoWindow } from './todos'; +import type { ACPToolCall, Message, TodoItem } from '../adapters/types'; +import { + computeTodoTimeline, + extractTodosFromToolCall, + getFloatingTodos, + getTodoStatusIcon, + getTodoWindow, + isTodoWriteToolName, + todoTimelineSignature, +} from './todos'; function todo(id: string, status: TodoItem['status']): TodoItem { return { id, content: `task ${id}`, status }; } +function item( + id: string, + content: string, + status: TodoItem['status'], +): TodoItem { + return { id, content, status }; +} + function planMessage(id: string, todos: TodoItem[]): Message { return { id, role: 'plan', todos }; } @@ -17,8 +33,9 @@ function todoWriteMessage(id: string, todos: TodoItem[]): Message { tools: [ { callId: `call-${id}`, - toolName: 'TodoWrite', + toolName: 'todo_write', status: 'completed', + kind: 'think', args: { todos }, }, ], @@ -129,6 +146,249 @@ describe('getFloatingTodos', () => { }); }); +describe('computeTodoTimeline', () => { + it('emits started and completed events across snapshots', () => { + const timeline = computeTodoTimeline([ + planMessage('p1', [todo('1', 'in_progress'), todo('2', 'pending')]), + planMessage('p2', [todo('1', 'completed'), todo('2', 'in_progress')]), + ]); + + expect(timeline.get('p1')).toEqual({ + events: [{ kind: 'started', id: '1', content: 'task 1' }], + }); + expect(timeline.get('p2')).toEqual({ + events: [ + { kind: 'completed', id: '1', content: 'task 1' }, + { kind: 'started', id: '2', content: 'task 2' }, + ], + }); + }); + + it('emits a completed event when an item skips in_progress', () => { + const timeline = computeTodoTimeline([ + planMessage('p1', [todo('1', 'pending')]), + planMessage('p2', [todo('1', 'completed')]), + ]); + + expect(timeline.get('p1')?.events).toEqual([]); + expect(timeline.get('p2')?.events).toEqual([ + { kind: 'completed', id: '1', content: 'task 1' }, + ]); + }); + + it('does not replay completions for items first seen already completed', () => { + const timeline = computeTodoTimeline([ + planMessage('p1', [todo('1', 'completed'), todo('2', 'in_progress')]), + ]); + + expect(timeline.get('p1')).toEqual({ + events: [{ kind: 'started', id: '2', content: 'task 2' }], + }); + }); + + it('produces no events for an unchanged re-emitted snapshot', () => { + const timeline = computeTodoTimeline([ + planMessage('p1', [todo('1', 'in_progress')]), + planMessage('p2', [todo('1', 'in_progress')]), + ]); + + expect(timeline.get('p2')?.events).toEqual([]); + }); + + it('tracks todo_write tool-call snapshots, keyed by callId', () => { + const timeline = computeTodoTimeline([ + todoWriteMessage('m1', [todo('1', 'in_progress')]), + todoWriteMessage('m2', [todo('1', 'completed')]), + ]); + + expect(timeline.get('call-m1')?.events).toEqual([ + { kind: 'started', id: '1', content: 'task 1' }, + ]); + expect(timeline.get('call-m2')?.events).toEqual([ + { kind: 'completed', id: '1', content: 'task 1' }, + ]); + }); + + it('ignores messages that carry no todo snapshot', () => { + const timeline = computeTodoTimeline([ + userMessage('u1'), + assistantMessage('a1'), + todoWriteMessage('m1', [todo('1', 'in_progress')]), + ]); + + expect(timeline.size).toBe(1); + expect(timeline.get('call-m1')?.events).toEqual([ + { kind: 'started', id: '1', content: 'task 1' }, + ]); + }); + + it('does not diff a reused id against a previous, unrelated plan', () => { + // Both plans number their first item "1" (positional/per-plan numbering), + // but they are different tasks. Plan A leaves "1" in_progress; plan B's "1" + // must still register its own start and completion rather than being + // suppressed by plan A's stale id-"1" status. + const timeline = computeTodoTimeline([ + planMessage('a', [item('1', 'Set up project', 'in_progress')]), + userMessage('u1'), + planMessage('b1', [item('1', 'Write the report', 'in_progress')]), + planMessage('b2', [item('1', 'Write the report', 'completed')]), + ]); + + expect(timeline.get('b1')?.events).toEqual([ + { kind: 'started', id: '1', content: 'Write the report' }, + ]); + expect(timeline.get('b2')?.events).toEqual([ + { kind: 'completed', id: '1', content: 'Write the report' }, + ]); + }); + + it('tracks the same item across a tool call and a later plan snapshot', () => { + const timeline = computeTodoTimeline([ + todoWriteMessage('m1', [todo('1', 'in_progress')]), + planMessage('p1', [todo('1', 'completed')]), + ]); + + expect(timeline.get('call-m1')?.events).toEqual([ + { kind: 'started', id: '1', content: 'task 1' }, + ]); + expect(timeline.get('p1')?.events).toEqual([ + { kind: 'completed', id: '1', content: 'task 1' }, + ]); + }); + + it('tracks an item that carries over and completes in a later turn', () => { + // id+content keys the same task across a user turn, so a "continue" turn + // that finishes a carried-over item still surfaces the completion (a + // user-turn reset would drop this). + const timeline = computeTodoTimeline([ + planMessage('p1', [item('1', 'Build feature', 'in_progress')]), + userMessage('u1'), + planMessage('p2', [item('1', 'Build feature', 'completed')]), + ]); + + expect(timeline.get('p2')?.events).toEqual([ + { kind: 'completed', id: '1', content: 'Build feature' }, + ]); + }); + + // Documented limitation of id+content keying: a mid-task reword reads as a new + // task, so its completion is treated as first-seen and omitted from the diff. + it('omits the completion when a todo is reworded on the same id (known gap)', () => { + const timeline = computeTodoTimeline([ + planMessage('p1', [item('1', 'Write report', 'in_progress')]), + planMessage('p2', [item('1', 'Write the final report', 'completed')]), + ]); + + expect(timeline.get('p2')?.events).toEqual([]); + }); + + // Documented limitation: two unrelated plans reusing both id AND content + // collide, so the second plan's completion is suppressed. + it('collides when unrelated plans reuse the same id and content (known gap)', () => { + const timeline = computeTodoTimeline([ + planMessage('a', [item('1', 'Run tests', 'completed')]), + userMessage('u1'), + planMessage('b', [item('1', 'Run tests', 'completed')]), + ]); + + expect(timeline.get('b')?.events).toEqual([]); + }); +}); + +describe('todoTimelineSignature', () => { + it('is unchanged by edits to non-todo messages', () => { + const a = todoTimelineSignature([ + planMessage('p1', [todo('1', 'in_progress')]), + assistantMessage('a1'), + ]); + const b = todoTimelineSignature([ + planMessage('p1', [todo('1', 'in_progress')]), + { id: 'a1', role: 'assistant', content: 'different text' }, + ]); + expect(b).toBe(a); + }); + + it('changes when an item id, status, or content changes', () => { + const base = todoTimelineSignature([ + planMessage('p1', [todo('1', 'in_progress')]), + ]); + const status = todoTimelineSignature([ + planMessage('p1', [todo('1', 'completed')]), + ]); + const id = todoTimelineSignature([ + planMessage('p1', [todo('2', 'in_progress')]), + ]); + const content = todoTimelineSignature([ + planMessage('p1', [item('1', 'reworded', 'in_progress')]), + ]); + expect(status).not.toBe(base); + expect(id).not.toBe(base); + expect(content).not.toBe(base); + }); + + it('is empty for a transcript with no todo snapshots', () => { + expect(todoTimelineSignature([])).toBe(''); + expect(todoTimelineSignature([userMessage('u1')])).toBe(''); + }); +}); + +describe('isTodoWriteToolName', () => { + it.each(['todo_write', 'todowrite', 'TodoWrite', 'TODO_WRITE'])( + 'matches %s', + (name) => { + expect(isTodoWriteToolName(name)).toBe(true); + }, + ); + + it.each(['read', 'edit', 'write_file', ''])('rejects %s', (name) => { + expect(isTodoWriteToolName(name)).toBe(false); + }); +}); + +describe('extractTodosFromToolCall', () => { + function toolCall(overrides: Partial): ACPToolCall { + return { + callId: 'c1', + toolName: 'todo_write', + status: 'completed', + kind: 'think', + ...overrides, + }; + } + + it('reads todos from args', () => { + const todos = extractTodosFromToolCall( + toolCall({ args: { todos: [item('1', 'A', 'pending')] } }), + ); + expect(todos).toEqual([{ id: '1', content: 'A', status: 'pending' }]); + }); + + it('reads todos from rawOutput.todos', () => { + const todos = extractTodosFromToolCall( + toolCall({ rawOutput: { todos: [item('1', 'A', 'in_progress')] } }), + ); + expect(todos?.map((t) => t.status)).toEqual(['in_progress']); + }); + + it('reads todos from rawOutput.entries', () => { + const todos = extractTodosFromToolCall( + toolCall({ rawOutput: { entries: [item('1', 'A', 'completed')] } }), + ); + expect(todos?.map((t) => t.status)).toEqual(['completed']); + }); + + it('returns undefined for a non-todo tool even if it carries a todos array', () => { + const todos = extractTodosFromToolCall( + toolCall({ + toolName: 'read', + kind: 'read', + args: { todos: [item('1', 'A', 'pending')] }, + }), + ); + expect(todos).toBeUndefined(); + }); +}); + describe('getTodoStatusIcon', () => { it('maps each status to its glyph', () => { expect(getTodoStatusIcon('completed')).toBe('●'); diff --git a/packages/web-shell/client/utils/todos.ts b/packages/web-shell/client/utils/todos.ts index 0216c092dd..fecff350a5 100644 --- a/packages/web-shell/client/utils/todos.ts +++ b/packages/web-shell/client/utils/todos.ts @@ -1,5 +1,15 @@ import type { ACPToolCall, Message, TodoItem } from '../adapters/types'; +/** + * The todo tool is registered as `todo_write` on the wire, but older paths and + * the ACP plan bridge use `todowrite`. Match both so detection never hinges on + * the (unrelated) tool `kind`, which is `think` for this tool. + */ +export function isTodoWriteToolName(name: string): boolean { + const normalized = name.toLowerCase(); + return normalized === 'todo_write' || normalized === 'todowrite'; +} + export function parseTodoItemsFromEntries( entries: readonly unknown[], ): TodoItem[] | undefined { @@ -22,8 +32,7 @@ export function parseTodoItemsFromEntries( export function extractTodosFromToolCall( tool: ACPToolCall, ): TodoItem[] | undefined { - const toolName = tool.toolName.toLowerCase(); - if (toolName !== 'todowrite' && tool.kind !== 'other') { + if (!isTodoWriteToolName(tool.toolName) && tool.kind !== 'other') { return undefined; } @@ -119,6 +128,133 @@ export function getFloatingTodos( return { todos, allCompleted, sourceMessageId, sourceCallId }; } +/** A status transition surfaced for a single todo snapshot. */ +export interface TodoEvent { + kind: 'started' | 'completed'; + id: string; + content: string; +} + +/** What changed in one todo snapshot relative to the conversation so far. */ +export interface TodoSnapshotDiff { + events: TodoEvent[]; +} + +interface TodoSnapshot { + /** Key the diff is stored under: tool callId, or plan message id. */ + key: string; + todos: TodoItem[]; +} + +/** + * Identity used to track an item across snapshots. Folds content into the key + * because todo ids are NOT globally unique: the ACP bridge assigns positional + * ids (`plan-0`, `plan-1`, …) and models restart numbering at `1, 2, 3` for each + * new `todo_write` plan, so a later, unrelated list reuses an earlier list's + * ids. Keying on id alone would diff a new plan's items against a previous + * plan's stale terminal status; id+content keeps distinct tasks separate, and — + * unlike a user-turn reset — it still tracks a list correctly when it spans + * turns (a "continue" turn that completes an item carried over from before). + * + * Two rare cases this trades for, both only affecting the collapsed diff while + * the expanded list stays correct: + * - A todo reworded on a stable id reads as a new task. Reworded while still + * `in_progress` it emits a spurious `started`; reworded straight to + * `completed` (`1 "Write report"` → `1 "Write the final report" completed`) + * the completion is treated as first-seen and dropped. + * - Two unrelated plans that reuse both the id AND the exact content (a generic + * recurring todo like `"Run tests"`) still collide. + */ +function todoStateKey(todo: TodoItem): string { + return JSON.stringify([todo.id, todo.content]); +} + +/** + * The todo snapshots carried by one message, in order. In the web-shell daemon + * path todos arrive as `todo_write` tool calls; the ACP bridge instead emits + * `plan` messages. Handle both so the timeline works regardless of source. + */ +function todoSnapshotsOf(message: Message): TodoSnapshot[] { + if (message.role === 'plan') { + return [{ key: message.id, todos: message.todos }]; + } + if (message.role === 'tool_group') { + const snapshots: TodoSnapshot[] = []; + for (const tool of message.tools) { + const todos = extractTodosFromToolCall(tool); + if (todos) snapshots.push({ key: tool.callId, todos }); + } + return snapshots; + } + return []; +} + +/** + * Walk the todo snapshots in order and, for each one, derive what changed + * relative to the running state: which items just started and which just + * completed. + * + * Keyed by snapshot id (tool callId or plan message id) so a history row can + * look up its own diff. Only transitions actually witnessed produce events — an + * item first seen already completed (e.g. a restored session's opening + * snapshot) is recorded silently so its old completion is not replayed as if it + * just happened. + */ +export function computeTodoTimeline( + messages: readonly Message[], +): Map { + const result = new Map(); + const lastStatus = new Map(); + + for (const message of messages) { + for (const { key, todos } of todoSnapshotsOf(message)) { + const events: TodoEvent[] = []; + + for (const todo of todos) { + const stateKey = todoStateKey(todo); + const prev = lastStatus.get(stateKey); + if (todo.status === 'in_progress' && prev !== 'in_progress') { + events.push({ kind: 'started', id: todo.id, content: todo.content }); + } else if ( + todo.status === 'completed' && + prev !== 'completed' && + prev !== undefined + ) { + events.push({ + kind: 'completed', + id: todo.id, + content: todo.content, + }); + } + lastStatus.set(stateKey, todo.status); + } + + result.set(key, { events }); + } + } + + return result; +} + +/** + * A cheap signature of the todo snapshots in a transcript: each snapshot's key + * plus its items' id, status, and content. App memoizes the timeline on this so + * the context provider value stays referentially stable across unrelated + * streaming ticks (which would otherwise re-render every todo/plan row that + * consumes the timeline). + */ +export function todoTimelineSignature(messages: readonly Message[]): string { + const parts: string[] = []; + for (const message of messages) { + for (const { key, todos } of todoSnapshotsOf(message)) { + parts.push( + JSON.stringify([key, todos.map((t) => [t.id, t.status, t.content])]), + ); + } + } + return parts.join('\n'); +} + export interface TodoWindow { start: number; end: number;