diff --git a/src/core/tools/UpdateTodoListTool.ts b/src/core/tools/UpdateTodoListTool.ts index a895f08ca7..795903bb38 100644 --- a/src/core/tools/UpdateTodoListTool.ts +++ b/src/core/tools/UpdateTodoListTool.ts @@ -32,7 +32,12 @@ export class UpdateTodoListTool extends BaseTool<"update_todo_list"> { const historyHasMetadata = Array.isArray(previousFromHistory) && previousFromHistory.some( - (t) => t?.subtaskId !== undefined || t?.tokens !== undefined || t?.cost !== undefined, + (t) => + t?.subtaskId !== undefined || + t?.tokens !== undefined || + t?.cost !== undefined || + t?.added !== undefined || + t?.removed !== undefined, ) const previousTodos: TodoItem[] = @@ -77,6 +82,8 @@ export class UpdateTodoListTool extends BaseTool<"update_todo_list"> { subtaskId: t.subtaskId, tokens: t.tokens, cost: t.cost, + added: t.added, + removed: t.removed, })) const approvalMsg = JSON.stringify({ @@ -215,45 +222,55 @@ function normalizeStatus(status: string | undefined): TodoStatus { } /** - * Preserve metadata (subtaskId, tokens, cost) from previous todos onto next todos. + * Preserve metadata (subtaskId, tokens, cost, added, removed) from previous todos onto next todos. * * Matching strategy (in priority order): - * 1. **ID match**: If both todos have an `id` field and they match exactly, preserve metadata. + * 1. **Subtask ID match**: If the next todo has a `subtaskId`, match against previous todos with + * the same `subtaskId`. This is the most stable identifier when content (and derived IDs) + * changes. + * 2. **ID match**: If both todos have an `id` field and they match exactly, preserve metadata. * This handles the common case where ID is stable across updates. - * 2. **Content match with position awareness**: For todos without matching IDs, fall back to - * content-based matching. Duplicates are matched in order (first unmatched previous with - * same content gets matched to first unmatched next with same content). + * 3. **Content match with position awareness**: For todos without matching subtask IDs or IDs, + * fall back to content-based matching. Duplicates are matched in order (first unmatched + * previous with same content gets matched to first unmatched next with same content). * - * This approach ensures metadata survives status changes (which can alter the derived ID) + * This approach ensures metadata survives status/content changes (which can alter the derived ID) * and handles duplicates deterministically. */ function preserveTodoMetadata(nextTodos: TodoItem[], previousTodos: TodoItem[]): TodoItem[] { const safePrevious = previousTodos ?? [] const safeNext = nextTodos ?? [] - // Build ID -> todo mapping for O(1) lookup - const previousById = new Map() - for (const prev of safePrevious) { - if (prev?.id && typeof prev.id === "string") { - // Only store the first occurrence for each ID (handle duplicates deterministically) - if (!previousById.has(prev.id)) { - previousById.set(prev.id, prev) - } - } - } - // Track which previous todos have been used (by their index) to avoid double-matching const usedPreviousIndices = new Set() - // Build content -> queue mapping for fallback (content-based matching) - // Each queue entry includes the original index for tracking + // Build lookup maps for matching strategies. + // - Subtask ID: may have duplicates; match in order for determinism. + // - ID: should be unique; store first occurrence. + // - Content: may have duplicates; match in order for determinism. + const previousBySubtaskId = new Map>() + const previousById = new Map() const previousByContent = new Map>() + for (let i = 0; i < safePrevious.length; i++) { const prev = safePrevious[i] - if (!prev || typeof prev.content !== "string") continue - const list = previousByContent.get(prev.content) - if (list) list.push({ todo: prev, index: i }) - else previousByContent.set(prev.content, [{ todo: prev, index: i }]) + if (!prev) continue + + if (typeof prev.subtaskId === "string") { + const list = previousBySubtaskId.get(prev.subtaskId) + if (list) list.push({ todo: prev, index: i }) + else previousBySubtaskId.set(prev.subtaskId, [{ todo: prev, index: i }]) + } + + if (typeof prev.id === "string" && !previousById.has(prev.id)) { + previousById.set(prev.id, { todo: prev, index: i }) + } + + if (typeof prev.content === "string") { + const list = previousByContent.get(prev.content) + if (list) list.push({ todo: prev, index: i }) + else previousByContent.set(prev.content, [{ todo: prev, index: i }]) + } } return safeNext.map((next) => { @@ -262,19 +279,29 @@ function preserveTodoMetadata(nextTodos: TodoItem[], previousTodos: TodoItem[]): let matchedPrev: TodoItem | undefined = undefined let matchedIndex: number | undefined = undefined - // Strategy 1: Try ID-based matching first (most reliable) - if (next.id && typeof next.id === "string") { - const byId = previousById.get(next.id) - if (byId) { - // Find the index of this todo in the original array - const idx = safePrevious.findIndex((p) => p === byId) - if (idx !== -1 && !usedPreviousIndices.has(idx)) { - matchedPrev = byId - matchedIndex = idx + // Strategy 0: Try subtaskId-based matching first (most stable across content/ID changes) + if (typeof next.subtaskId === "string") { + const candidates = previousBySubtaskId.get(next.subtaskId) + if (candidates) { + for (const candidate of candidates) { + if (!usedPreviousIndices.has(candidate.index)) { + matchedPrev = candidate.todo + matchedIndex = candidate.index + break + } } } } + // Strategy 1: Try ID-based matching first (most reliable) + if (!matchedPrev && next.id && typeof next.id === "string") { + const byId = previousById.get(next.id) + if (byId && !usedPreviousIndices.has(byId.index)) { + matchedPrev = byId.todo + matchedIndex = byId.index + } + } + // Strategy 2: Fall back to content-based matching if ID didn't match if (!matchedPrev && typeof next.content === "string") { const candidates = previousByContent.get(next.content) @@ -298,6 +325,8 @@ function preserveTodoMetadata(nextTodos: TodoItem[], previousTodos: TodoItem[]): subtaskId: next.subtaskId ?? matchedPrev.subtaskId, tokens: next.tokens ?? matchedPrev.tokens, cost: next.cost ?? matchedPrev.cost, + added: next.added ?? matchedPrev.added, + removed: next.removed ?? matchedPrev.removed, } } diff --git a/src/core/tools/__tests__/updateTodoListTool.spec.ts b/src/core/tools/__tests__/updateTodoListTool.spec.ts index 8ed33c10e5..c67730e6c8 100644 --- a/src/core/tools/__tests__/updateTodoListTool.spec.ts +++ b/src/core/tools/__tests__/updateTodoListTool.spec.ts @@ -292,4 +292,259 @@ describe("UpdateTodoListTool.execute", () => { }), ) }) + + it("should treat added/removed as metadata and prefer history todos when present", async () => { + const md = "[ ] Task 1" + + const previousFromMemory = parseMarkdownChecklist(md) + const previousFromHistory: TodoItem[] = previousFromMemory.map((t) => ({ + ...t, + added: 10, + removed: 3, + })) + + const task = { + todoList: previousFromMemory, + clineMessages: [ + { + type: "ask", + ask: "tool", + text: JSON.stringify({ tool: "updateTodoList", todos: previousFromHistory }), + }, + ], + consecutiveMistakeCount: 0, + recordToolError: vi.fn(), + didToolFailInCurrentTurn: false, + say: vi.fn(), + } as any + + const tool = new UpdateTodoListTool() + await tool.execute({ todos: md }, task, { + pushToolResult: vi.fn(), + handleError: vi.fn(), + askApproval: vi.fn().mockResolvedValue(true), + removeClosingTag: vi.fn(), + toolProtocol: "xml", + }) + + expect(task.todoList).toHaveLength(1) + expect(task.todoList[0]).toEqual( + expect.objectContaining({ + content: "Task 1", + added: 10, + removed: 3, + }), + ) + }) + + it("should preserve metadata by subtaskId even when content (and derived id) changes", async () => { + // This test simulates the "user edited todo list" flow. The tool re-applies metadata + // after approval; subtaskId should be used as the primary match when content/id changes. + const md = "[ ] Old text" + + const previousFromMemory: TodoItem[] = parseMarkdownChecklist(md).map((t) => ({ + ...t, + subtaskId: "subtask-1", + tokens: 123, + cost: 0.01, + added: 10, + removed: 3, + })) + + const task = { + todoList: previousFromMemory, + clineMessages: [], + consecutiveMistakeCount: 0, + recordToolError: vi.fn(), + didToolFailInCurrentTurn: false, + say: vi.fn(), + } as any + + // Simulate user-edited todo list with updated content and a different id, but the same subtaskId. + const userEditedTodos: TodoItem[] = [ + { + id: "new-id", + content: "New text", + status: "completed", + subtaskId: "subtask-1", + // tokens/cost/added/removed intentionally omitted to verify preservation + }, + ] + + const tool = new UpdateTodoListTool() + await tool.execute({ todos: md }, task, { + pushToolResult: vi.fn(), + handleError: vi.fn(), + askApproval: vi.fn().mockImplementation(async () => { + setPendingTodoList(userEditedTodos) + return true + }), + removeClosingTag: vi.fn(), + toolProtocol: "xml", + }) + + expect(task.todoList).toHaveLength(1) + expect(task.todoList[0]).toEqual( + expect.objectContaining({ + id: "new-id", + content: "New text", + status: "completed", + subtaskId: "subtask-1", + tokens: 123, + cost: 0.01, + added: 10, + removed: 3, + }), + ) + }) + + it("should preserve added/removed through normalization", async () => { + const md = "[x] Task 1" + + const previousFromMemory: TodoItem[] = parseMarkdownChecklist("[ ] Task 1").map((t) => ({ + ...t, + added: 10, + removed: 3, + })) + + const task = { + todoList: previousFromMemory, + clineMessages: [], + consecutiveMistakeCount: 0, + recordToolError: vi.fn(), + didToolFailInCurrentTurn: false, + say: vi.fn(), + } as any + + const tool = new UpdateTodoListTool() + await tool.execute({ todos: md }, task, { + pushToolResult: vi.fn(), + handleError: vi.fn(), + askApproval: vi.fn().mockResolvedValue(true), + removeClosingTag: vi.fn(), + toolProtocol: "xml", + }) + + expect(task.todoList).toHaveLength(1) + expect(task.todoList[0]).toEqual( + expect.objectContaining({ + content: "Task 1", + status: "completed", + added: 10, + removed: 3, + }), + ) + }) + + it("should not cross-contaminate metadata when no subtaskId is present", async () => { + const initialMd = "[ ] Task 1\n[ ] Task 2" + const md = "[x] Task 1\n[ ] Task 2" // status changes for Task 1 -> derived id changes + + const previousFromMemory: TodoItem[] = parseMarkdownChecklist(initialMd).map((t) => + t.content === "Task 1" + ? { ...t, tokens: 111, cost: 0.11, added: 11, removed: 1 } + : { ...t, tokens: 222, cost: 0.22, added: 22, removed: 2 }, + ) + + const task = { + todoList: previousFromMemory, + clineMessages: [], + consecutiveMistakeCount: 0, + recordToolError: vi.fn(), + didToolFailInCurrentTurn: false, + say: vi.fn(), + } as any + + const tool = new UpdateTodoListTool() + await tool.execute({ todos: md }, task, { + pushToolResult: vi.fn(), + handleError: vi.fn(), + askApproval: vi.fn().mockResolvedValue(true), + removeClosingTag: vi.fn(), + toolProtocol: "xml", + }) + + expect(task.todoList).toHaveLength(2) + + const task1 = task.todoList.find((t: TodoItem) => t.content === "Task 1") + const task2 = task.todoList.find((t: TodoItem) => t.content === "Task 2") + + expect(task1).toEqual( + expect.objectContaining({ + content: "Task 1", + status: "completed", + tokens: 111, + cost: 0.11, + added: 11, + removed: 1, + }), + ) + + expect(task2).toEqual( + expect.objectContaining({ + content: "Task 2", + status: "pending", + tokens: 222, + cost: 0.22, + added: 22, + removed: 2, + }), + ) + }) + + it("should not preserve metadata when content changes and there is no subtaskId", async () => { + const initialMd = "[ ] Task 1\n[ ] Task 2" + const md = "[x] Task 1 (updated)\n[ ] Task 2" + + const previousFromMemory: TodoItem[] = parseMarkdownChecklist(initialMd).map((t) => + t.content === "Task 1" + ? { ...t, tokens: 111, cost: 0.11, added: 11, removed: 1 } + : { ...t, tokens: 222, cost: 0.22, added: 22, removed: 2 }, + ) + + const task = { + todoList: previousFromMemory, + clineMessages: [], + consecutiveMistakeCount: 0, + recordToolError: vi.fn(), + didToolFailInCurrentTurn: false, + say: vi.fn(), + } as any + + const tool = new UpdateTodoListTool() + await tool.execute({ todos: md }, task, { + pushToolResult: vi.fn(), + handleError: vi.fn(), + askApproval: vi.fn().mockResolvedValue(true), + removeClosingTag: vi.fn(), + toolProtocol: "xml", + }) + + expect(task.todoList).toHaveLength(2) + + const updated = task.todoList.find((t: TodoItem) => t.content === "Task 1 (updated)") + const task2 = task.todoList.find((t: TodoItem) => t.content === "Task 2") + + expect(updated).toEqual( + expect.objectContaining({ + content: "Task 1 (updated)", + status: "completed", + }), + ) + expect(updated?.tokens).toBeUndefined() + expect(updated?.cost).toBeUndefined() + expect(updated?.added).toBeUndefined() + expect(updated?.removed).toBeUndefined() + + expect(task2).toEqual( + expect.objectContaining({ + content: "Task 2", + status: "pending", + tokens: 222, + cost: 0.22, + added: 22, + removed: 2, + }), + ) + }) }) diff --git a/webview-ui/src/components/chat/TodoListDisplay.tsx b/webview-ui/src/components/chat/TodoListDisplay.tsx index bbc187bacb..c3da1c717b 100644 --- a/webview-ui/src/components/chat/TodoListDisplay.tsx +++ b/webview-ui/src/components/chat/TodoListDisplay.tsx @@ -105,7 +105,8 @@ export function TodoListDisplay({ todos, subtaskDetails, onSubtaskClick }: TodoL {!isCollapsed && (
    {todos.map((todo, idx: number) => { - const icon = getTodoIcon(todo.status as TodoStatus) + const todoStatus = (todo.status as TodoStatus) ?? "pending" + const icon = getTodoIcon(todoStatus) const isClickable = Boolean(todo.subtaskId && onSubtaskClick) const subtaskById = subtaskDetails && todo.subtaskId @@ -115,16 +116,33 @@ export function TodoListDisplay({ todos, subtaskDetails, onSubtaskClick }: TodoL const displayCost = todo.cost ?? subtaskById?.cost const shouldShowCost = typeof displayTokens === "number" && typeof displayCost === "number" - const displayAdded = todo.added ?? subtaskById?.added - const displayRemoved = todo.removed ?? subtaskById?.removed - const hasValidSubtaskLink = typeof todo.subtaskId === "string" && todo.subtaskId.length > 0 - const shouldShowLineChanges = - hasValidSubtaskLink && (Number.isFinite(displayAdded) || Number.isFinite(displayRemoved)) + const todoAddedIsFinite = typeof todo.added === "number" && Number.isFinite(todo.added) + const todoRemovedIsFinite = typeof todo.removed === "number" && Number.isFinite(todo.removed) - const hasAdded = - typeof displayAdded === "number" && Number.isFinite(displayAdded) && displayAdded > 0 - const hasRemoved = - typeof displayRemoved === "number" && Number.isFinite(displayRemoved) && displayRemoved > 0 + const displayAdded = todoAddedIsFinite ? todo.added : subtaskById?.added + const displayRemoved = todoRemovedIsFinite ? todo.removed : subtaskById?.removed + + const displayAddedIsFinite = typeof displayAdded === "number" && Number.isFinite(displayAdded) + const displayRemovedIsFinite = + typeof displayRemoved === "number" && Number.isFinite(displayRemoved) + const hasValidSubtaskLink = typeof todo.subtaskId === "string" && todo.subtaskId.length > 0 + + // Upstream aggregation may coerce missing stats to 0. + // To avoid showing misleading `+0/−0` for in-progress/pending rows, + // only render 0 while running if it was explicitly provided on the todo itself. + const canRenderAdded = + displayAddedIsFinite && + (todoStatus === "completed" || displayAdded !== 0 || todoAddedIsFinite) + const canRenderRemoved = + displayRemovedIsFinite && + (todoStatus === "completed" || displayRemoved !== 0 || todoRemovedIsFinite) + + const shouldShowLineChanges = hasValidSubtaskLink && (canRenderAdded || canRenderRemoved) + + const isAddedPositive = canRenderAdded && (displayAdded as number) > 0 + const isRemovedPositive = canRenderRemoved && (displayRemoved as number) > 0 + const isAddedZero = canRenderAdded && displayAdded === 0 + const isRemovedZero = canRenderRemoved && displayRemoved === 0 return (
  • (itemRefs.current[idx] = el)} className={cn( "font-light flex flex-row gap-2 items-start min-h-[20px] leading-normal mb-2", - todo.status === "in_progress" && "text-vscode-charts-yellow", - todo.status !== "in_progress" && todo.status !== "completed" && "opacity-60", + todoStatus === "in_progress" && "text-vscode-charts-yellow", + todoStatus !== "in_progress" && todoStatus !== "completed" && "opacity-60", )}> {icon} - {hasAdded ? `+${displayAdded}` : "\u00A0"} + {canRenderAdded ? `+${displayAdded}` : "\u00A0"} - {hasRemoved ? `−${displayRemoved}` : "\u00A0"} + {canRenderRemoved ? `−${displayRemoved}` : "\u00A0"} )} diff --git a/webview-ui/src/components/chat/__tests__/TodoListDisplay.spec.tsx b/webview-ui/src/components/chat/__tests__/TodoListDisplay.spec.tsx index d4a8d349f7..67650153f1 100644 --- a/webview-ui/src/components/chat/__tests__/TodoListDisplay.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/TodoListDisplay.spec.tsx @@ -206,6 +206,117 @@ describe("TodoListDisplay", () => { expect(screen.getByText("−9")).toBeInTheDocument() }) + it("shows +0/−0 for completed subtask when fallback metrics are explicitly zero", () => { + const todosMissingDirectLineChanges = [ + { + id: "1", + content: "Task 1: Zero changes", + status: "completed", + subtaskId: "subtask-1", + }, + ] + const subtaskDetailsWithZeroLineChanges: SubtaskDetail[] = [ + { + id: "subtask-1", + name: "Task 1: Zero changes", + tokens: 1, + cost: 0.01, + added: 0, + removed: 0, + status: "completed", + hasNestedChildren: false, + }, + ] + + render( + , + ) + + // Expand + const header = screen.getByText("1 to-dos done") + fireEvent.click(header) + + const addedEl = screen.getByText("+0") + const removedEl = screen.getByText("−0") + + expect(addedEl).toBeInTheDocument() + expect(removedEl).toBeInTheDocument() + + // Zero values should be visually muted (not green/red emphasized) + expect(addedEl.className).toContain("opacity-50") + expect(addedEl.className).not.toContain("text-vscode-charts-green") + + expect(removedEl.className).toContain("opacity-50") + expect(removedEl.className).not.toContain("text-vscode-charts-red") + }) + + it("in-progress: does not show +0/−0 when zeros only come from fallback", () => { + const todosMissingDirectLineChanges = [ + { + id: "1", + content: "Task 1: Zero changes (running)", + status: "in_progress", + subtaskId: "subtask-1", + }, + ] + const subtaskDetailsWithZeroLineChanges: SubtaskDetail[] = [ + { + id: "subtask-1", + name: "Task 1: Zero changes (running)", + tokens: 1, + cost: 0.01, + added: 0, + removed: 0, + status: "active", + hasNestedChildren: false, + }, + ] + + render( + , + ) + + // Expand + const header = screen.getByText("Task 1: Zero changes (running)") + fireEvent.click(header) + + expect(screen.queryByText("+0")).not.toBeInTheDocument() + expect(screen.queryByText("−0")).not.toBeInTheDocument() + }) + + it("in-progress: shows +0/−0 when explicitly present on todo", () => { + const todosWithDirectLineChanges = [ + { + id: "1", + content: "Task 1: Zero changes (explicit)", + status: "in_progress", + subtaskId: "subtask-1", + added: 0, + removed: 0, + }, + ] + + render() + + // Expand + const header = screen.getByText("Task 1: Zero changes (explicit)") + fireEvent.click(header) + + const addedEl = screen.getByText("+0") + const removedEl = screen.getByText("−0") + expect(addedEl).toBeInTheDocument() + expect(removedEl).toBeInTheDocument() + + expect(addedEl.className).toContain("opacity-50") + expect(removedEl.className).toContain("opacity-50") + }) + it("falls back to subtaskDetails when todo added/removed are missing", () => { const todosMissingDirectLineChanges = [ {