From 9ffc778491845f0ff2304a2118246a9d5d7848dd Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Fri, 6 Feb 2026 17:12:44 -0700 Subject: [PATCH] fix: audit improvements for batch tool-call UI - Make batchConsecutive() generic instead of ClineMessage-specific - Add batch-aware button text for edit-file batches ("Save All"/"Deny All") - Add dedicated list-batch/edit-batch i18n keys (stop reusing read-batch) - Add JSON.parse defense-in-depth in all three synthesizers - Fix mixed list_files batch icon to default to FolderTree - Add 6 missing test cases (all-match, immutability, spy, single-dir) --- webview-ui/src/components/chat/ChatRow.tsx | 12 +++- webview-ui/src/components/chat/ChatView.tsx | 33 +++++++++-- .../BatchListFilesPermission.spec.tsx | 24 ++++++++ webview-ui/src/i18n/locales/en/chat.json | 16 +++++ .../utils/__tests__/batchConsecutive.spec.ts | 58 ++++++++++++++++--- webview-ui/src/utils/batchConsecutive.ts | 30 ++++------ 6 files changed, 140 insertions(+), 33 deletions(-) diff --git a/webview-ui/src/components/chat/ChatRow.tsx b/webview-ui/src/components/chat/ChatRow.tsx index 0772d52f15..b4342f2edf 100644 --- a/webview-ui/src/components/chat/ChatRow.tsx +++ b/webview-ui/src/components/chat/ChatRow.tsx @@ -743,12 +743,20 @@ export const ChatRowContent = ({ case "listFilesTopLevel": case "listFilesRecursive": { const isRecursive = tool.tool === "listFilesRecursive" - const DirIcon = isRecursive ? FolderTree : ListTree - const dirIconLabel = isRecursive ? "Folder tree icon" : "List files icon" // Check if this is a batch directory listing request const isBatchDirRequest = message.type === "ask" && tool.batchDirs && Array.isArray(tool.batchDirs) + // When batching, check if all dirs share the same recursive value + const allTopLevel = tool.batchDirs?.every((d: { recursive: boolean }) => !d.recursive) + const DirIcon = isBatchDirRequest && !allTopLevel ? FolderTree : isRecursive ? FolderTree : ListTree + const dirIconLabel = + isBatchDirRequest && !allTopLevel + ? "Folder tree icon" + : isRecursive + ? "Folder tree icon" + : "List files icon" + if (isBatchDirRequest) { return ( <> diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index cc9560250f..ff64e06068 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -312,6 +312,14 @@ const ChatViewComponent: React.ForwardRefRenderFunction { // All 3 dirs should be inside this container expect(container?.querySelectorAll(".flex.items-center.gap-2")).toHaveLength(mockDirs.length) }) + + it("renders a single directory", () => { + const singleDir = [ + { + key: "apps/cli", + path: "apps/cli", + recursive: false, + isOutsideWorkspace: false, + }, + ] + + render( + + + , + ) + + expect(screen.getByText("apps/cli")).toBeInTheDocument() + + // Single directory should still be rendered inside the container + const bordered = screen.getByText("apps/cli").closest(".border.border-border.rounded-md") + expect(bordered).toBeInTheDocument() + expect(bordered?.querySelectorAll(".flex.items-center.gap-2")).toHaveLength(1) + }) }) diff --git a/webview-ui/src/i18n/locales/en/chat.json b/webview-ui/src/i18n/locales/en/chat.json index 04d8769c0b..4313a04db5 100644 --- a/webview-ui/src/i18n/locales/en/chat.json +++ b/webview-ui/src/i18n/locales/en/chat.json @@ -74,6 +74,22 @@ "title": "Deny All" } }, + "list-batch": { + "approve": { + "title": "Approve All" + }, + "deny": { + "title": "Deny All" + } + }, + "edit-batch": { + "approve": { + "title": "Save All" + }, + "deny": { + "title": "Deny All" + } + }, "runCommand": { "title": "Run", "tooltip": "Execute this command" diff --git a/webview-ui/src/utils/__tests__/batchConsecutive.spec.ts b/webview-ui/src/utils/__tests__/batchConsecutive.spec.ts index 3bb5a69d2f..b3919fdbd6 100644 --- a/webview-ui/src/utils/__tests__/batchConsecutive.spec.ts +++ b/webview-ui/src/utils/__tests__/batchConsecutive.spec.ts @@ -1,17 +1,21 @@ -import type { ClineMessage } from "@roo-code/types" - import { batchConsecutive } from "../batchConsecutive" -/** Helper: create a minimal ClineMessage with an identifiable text field. */ -function msg(text: string, type: ClineMessage["type"] = "say"): ClineMessage { +interface TestItem { + ts: number + type: string + text: string +} + +/** Helper: create a minimal test item with an identifiable text field. */ +function msg(text: string, type = "say"): TestItem { return { ts: Date.now(), type, text } } -/** Predicate: matches messages whose text starts with "match". */ -const isMatch = (m: ClineMessage) => !!m.text?.startsWith("match") +/** Predicate: matches items whose text starts with "match". */ +const isMatch = (m: TestItem) => !!m.text?.startsWith("match") -/** Synthesize: merges a batch into a single message with a "BATCH:" marker. */ -const synthesizeBatch = (batch: ClineMessage[]): ClineMessage => ({ +/** Synthesize: merges a batch into a single item with a "BATCH:" marker. */ +const synthesizeBatch = (batch: TestItem[]): TestItem => ({ ...batch[0], text: `BATCH:${batch.map((m) => m.text).join(",")}`, }) @@ -71,4 +75,42 @@ describe("batchConsecutive", () => { expect(result[3].text).toBe("other-2") expect(result[4].text).toBe("BATCH:match-5,match-6") }) + + test("all items match → single synthetic message", () => { + const items = [msg("match-1"), msg("match-2"), msg("match-3")] + const result = batchConsecutive(items, isMatch, synthesizeBatch) + expect(result).toHaveLength(1) + expect(result[0].text).toBe("BATCH:match-1,match-2,match-3") + }) + + test("does not mutate the input array", () => { + const items = [msg("match-1"), msg("match-2")] + const original = [...items] + batchConsecutive(items, isMatch, synthesizeBatch) + expect(items).toHaveLength(2) + expect(items).toEqual(original) + }) + + test("returns a new array, not the same reference", () => { + const items = [msg("a"), msg("b")] + const result = batchConsecutive(items, isMatch, synthesizeBatch) + expect(result).not.toBe(items) + }) + + test("synthesize callback receives the correct batches", () => { + const spy = vi.fn(synthesizeBatch) + const items = [msg("match-1"), msg("match-2"), msg("other"), msg("match-3"), msg("match-4")] + batchConsecutive(items, isMatch, spy) + expect(spy).toHaveBeenCalledTimes(2) + expect(spy.mock.calls[0][0]).toHaveLength(2) + expect(spy.mock.calls[1][0]).toHaveLength(2) + }) + + test("batch at the end of the array", () => { + const items = [msg("other"), msg("match-1"), msg("match-2")] + const result = batchConsecutive(items, isMatch, synthesizeBatch) + expect(result).toHaveLength(2) + expect(result[0].text).toBe("other") + expect(result[1].text).toBe("BATCH:match-1,match-2") + }) }) diff --git a/webview-ui/src/utils/batchConsecutive.ts b/webview-ui/src/utils/batchConsecutive.ts index e030ad0c70..336d8a74a6 100644 --- a/webview-ui/src/utils/batchConsecutive.ts +++ b/webview-ui/src/utils/batchConsecutive.ts @@ -1,29 +1,23 @@ -import type { ClineMessage } from "@roo-code/types" - /** - * Walk a message array and batch runs of consecutive messages that match - * `predicate` into synthetic messages produced by `synthesize`. + * Walk an item array and batch runs of consecutive items that match + * `predicate` into synthetic items produced by `synthesize`. * * - Runs of length 1 are passed through unchanged. - * - Runs of length >= 2 are replaced by a single synthetic message. - * - Non-matching messages are preserved in-order. + * - Runs of length >= 2 are replaced by a single synthetic item. + * - Non-matching items are preserved in-order. */ -export function batchConsecutive( - messages: ClineMessage[], - predicate: (msg: ClineMessage) => boolean, - synthesize: (batch: ClineMessage[]) => ClineMessage, -): ClineMessage[] { - const result: ClineMessage[] = [] +export function batchConsecutive(items: T[], predicate: (item: T) => boolean, synthesize: (batch: T[]) => T): T[] { + const result: T[] = [] let i = 0 - while (i < messages.length) { - if (predicate(messages[i])) { + while (i < items.length) { + if (predicate(items[i])) { // Collect consecutive matches into a batch - const batch: ClineMessage[] = [messages[i]] + const batch: T[] = [items[i]] let j = i + 1 - while (j < messages.length && predicate(messages[j])) { - batch.push(messages[j]) + while (j < items.length && predicate(items[j])) { + batch.push(items[j]) j++ } @@ -35,7 +29,7 @@ export function batchConsecutive( i = j } else { - result.push(messages[i]) + result.push(items[i]) i++ } }