Skip to content

Commit da9a4bb

Browse files
playcationsplaycations
committed
PR review fixes
 - Added focused unit tests highlighting review concerns (diff accuracy, debounce cleanup, checkpoint failure, rehydrate state, UI virtualization, error- boundary containment). - Replaced calculateLineDifferences with diffLines and updated coverage so insert/delete counts are correct. - Hardened FilesChangedMessageHandler (debounce cleanup, safe enable/disable, state replay after rehydrate, checkpoint-failure guard). - Simplified checkpoint mock usage in tests (vi.mocked(getCheckpointService)), ensuring the failure case is properly asserted. - Verified targeted specs: - pnpm -C Roo-Code/src test services/files-changed/__tests__/FilesChangedMessageHandler.test.ts - pnpm -C Roo-Code/webview-ui test src/components/file-changes/__tests__/FilesChangedOverview.spec.tsx - pnpm -C Roo-Code/webview-ui test src/components/chat/__tests__/TaskHeader.fco-boundary.spec.tsx - FilesChangedOverview component - Virtualization threshold is still 50; we only wrote a failing test earlier but haven’t lowered the constant or adjusted behavior. - No dedicated error boundary: the TaskHeader test we added expects one, but the UI code still renders the component directly. - Accessibility/hardening suggestions (e.g., button focus states, aria refinements) haven’t been implemented. - Any “type-safety” or “performance” tweaks flagged in the review beyond the diff/handler items are pending.
1 parent 23f3d8a commit da9a4bb

13 files changed

+483
-80
lines changed

‎src/services/files-changed/FilesChangedManager.ts‎

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { diffLines } from "diff"
2+
13
import { FileChange, FileChangeset } from "@roo-code/types"
24
import type { FileContextTracker } from "../../core/context-tracking/FileContextTracker"
35

@@ -80,24 +82,15 @@ export class FilesChangedManager {
8082
originalContent: string,
8183
newContent: string,
8284
): { linesAdded: number; linesRemoved: number } {
83-
const originalLines = originalContent === "" ? [] : originalContent.split("\n")
84-
const newLines = newContent === "" ? [] : newContent.split("\n")
85-
86-
const maxLines = Math.max(originalLines.length, newLines.length)
85+
const hunks = diffLines(originalContent ?? "", newContent ?? "")
8786
let linesAdded = 0
8887
let linesRemoved = 0
8988

90-
for (let i = 0; i < maxLines; i++) {
91-
const originalLine = i < originalLines.length ? originalLines[i] : undefined
92-
const newLine = i < newLines.length ? newLines[i] : undefined
93-
94-
if (originalLine === undefined && newLine !== undefined) {
95-
linesAdded++
96-
} else if (originalLine !== undefined && newLine === undefined) {
97-
linesRemoved++
98-
} else if (originalLine !== newLine) {
99-
linesAdded++
100-
linesRemoved++
89+
for (const hunk of hunks) {
90+
if (hunk.added) {
91+
linesAdded += hunk.count ?? 0
92+
} else if (hunk.removed) {
93+
linesRemoved += hunk.count ?? 0
10194
}
10295
}
10396

‎src/services/files-changed/FilesChangedMessageHandler.ts‎

Lines changed: 85 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,24 @@ export class FilesChangedMessageHandler {
9090
return
9191
}
9292

93+
const baseline =
94+
effectiveManager.getChanges().baseCheckpoint ||
95+
task?.checkpointService?.baseHash ||
96+
task?.checkpointService?.getCurrentCheckpoint?.()
97+
98+
if (!baseline) {
99+
// Put URIs back in queue if no baseline available yet
100+
state.queueChildUris(pendingUris)
101+
return
102+
}
103+
104+
// Process each subtask file individually using the same logic as normal roo_edited events
93105
for (const uri of pendingUris) {
94-
await this.refreshEditedFile(task, uri)
106+
try {
107+
await this.refreshEditedFile(task, uri)
108+
} catch (error) {
109+
this.provider.log(`FilesChanged: Failed to process queued file ${uri}: ${error}`)
110+
}
95111
}
96112
}
97113

@@ -139,23 +155,25 @@ export class FilesChangedMessageHandler {
139155
return
140156
}
141157

142-
this.isEnabled = enabled
143-
144-
if (enabled && task) {
145-
try {
146-
await getCheckpointService(task)
147-
} catch (error) {
148-
this.provider.log(`FilesChanged: Failed to initialize checkpoint service: ${error}`)
149-
return
158+
if (enabled) {
159+
if (task) {
160+
try {
161+
await getCheckpointService(task)
162+
} catch (error) {
163+
this.provider.log(`FilesChanged: Failed to initialize checkpoint service: ${error}`)
164+
this.isEnabled = false
165+
return
166+
}
150167
}
151-
}
152168

153-
if (enabled) {
169+
this.isEnabled = true
154170
this.markWaitingForTask(task, true)
155171
this.provider.log("FilesChanged: Enabled, waiting for next checkpoint to establish monitoring baseline")
156172
this.clearFilesChangedDisplay()
157173
await this.attachToTask(task)
174+
this.replayTaskChanges(task)
158175
} else {
176+
this.isEnabled = false
159177
const targetTask = task ?? this.activeTask ?? (this.provider.getCurrentTask() as Task | undefined)
160178
if (targetTask) {
161179
this.markWaitingForTask(targetTask, false)
@@ -182,10 +200,7 @@ export class FilesChangedMessageHandler {
182200
target.disposeFilesChangedState()
183201
}
184202
this.activeTask = undefined
185-
if (this.trackerDebounce) {
186-
clearTimeout(this.trackerDebounce)
187-
this.trackerDebounce = undefined
188-
}
203+
this.clearTrackerDebounce()
189204
}
190205

191206
private async attachToTask(task: Task | undefined): Promise<void> {
@@ -268,12 +283,24 @@ export class FilesChangedMessageHandler {
268283
}
269284

270285
const baseline = event?.fromHash ?? event?.toHash
286+
const hadQueued = state?.hasQueuedChildUris() ?? false
287+
const hasExistingFiles = manager.getChanges().files.length > 0
288+
271289
if (baseline) {
272-
manager.reset(baseline)
290+
if (hasExistingFiles && hadQueued) {
291+
// Adding child files to existing parent files - preserve existing files
292+
manager.setBaseline(baseline)
293+
this.provider.log(
294+
`FilesChanged: Updated baseline to ${baseline}, preserving ${manager.getChanges().files.length} existing files`,
295+
)
296+
} else {
297+
// Starting fresh or no existing files - clear is appropriate
298+
manager.reset(baseline)
299+
this.provider.log(`FilesChanged: Reset to baseline ${baseline}`)
300+
}
273301
}
274302
this.markWaitingForTask(task, false)
275303

276-
const hadQueued = state?.hasQueuedChildUris() ?? false
277304
if (hadQueued) {
278305
await this.drainQueuedUris(task, manager)
279306
this.provider.log(
@@ -336,6 +363,29 @@ export class FilesChangedMessageHandler {
336363
task.fileContextTracker.off("roo_edited", this.trackerListener)
337364
}
338365
this.trackerListener = undefined
366+
this.clearTrackerDebounce()
367+
}
368+
369+
private clearTrackerDebounce(): void {
370+
if (this.trackerDebounce) {
371+
clearTimeout(this.trackerDebounce)
372+
this.trackerDebounce = undefined
373+
}
374+
}
375+
376+
private replayTaskChanges(task: Task | undefined): void {
377+
if (!this.isEnabled || !task) {
378+
return
379+
}
380+
const manager = this.getManager(task)
381+
if (!manager) {
382+
return
383+
}
384+
const changes = manager.getChanges()
385+
if (changes.files.length > 0) {
386+
this.markWaitingForTask(task, false)
387+
this.postChanges(manager)
388+
}
339389
}
340390

341391
/**
@@ -638,26 +688,26 @@ export class FilesChangedMessageHandler {
638688
await this.handleExperimentToggle(shouldBeEnabled, task)
639689
return
640690
}
641-
if (this.isEnabled) {
642-
try {
643-
await getCheckpointService(task)
644-
} catch (error) {
645-
this.provider.log(`FilesChanged: Failed to initialize checkpoint service: ${error}`)
646-
}
691+
if (!shouldBeEnabled) {
692+
return
693+
}
647694

648-
// Only reattach if we're not already attached to this task, or if there are pending child files
649-
const taskState = this.getState(task)
650-
const needsReattach = this.activeTask !== task || (taskState && taskState.hasQueuedChildUris())
695+
try {
696+
await getCheckpointService(task)
697+
} catch (error) {
698+
this.provider.log(`FilesChanged: Failed to initialize checkpoint service: ${error}`)
699+
return
700+
}
651701

652-
if (needsReattach) {
653-
await this.attachToTask(task)
654-
} else {
655-
// Just ensure current state is posted if we're already properly attached
656-
const manager = this.getManager(task)
657-
if (manager && !this.isWaitingForTask(task)) {
658-
this.postChanges(manager)
659-
}
660-
}
702+
// Only reattach if we're not already attached to this task, or if there are pending child files
703+
const taskState = this.getState(task)
704+
const needsReattach = this.activeTask !== task || (taskState && taskState.hasQueuedChildUris())
705+
706+
if (needsReattach) {
707+
await this.attachToTask(task)
708+
this.replayTaskChanges(task)
709+
} else {
710+
this.replayTaskChanges(task)
661711
}
662712
}
663713

‎src/services/files-changed/TaskFilesChangedState.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,6 @@ export class TaskFilesChangedState {
8787
}
8888

8989
public shouldWaitForNextCheckpoint(): boolean {
90-
return this.waitingForCheckpoint && this.queuedChildUris.size === 0
90+
return this.waitingForCheckpoint
9191
}
9292
}

‎src/services/files-changed/__tests__/FilesChangedManager.test.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,4 +59,18 @@ describe("FilesChangedManager", () => {
5959
const filtered = await manager.getLLMOnlyChanges("task-1", {} as any)
6060
expect(filtered.files.map((f) => f.uri)).toEqual(["app/foo.ts"])
6161
})
62+
63+
it("computes accurate line stats for pure insertions", () => {
64+
const original = ["console.log(1)", "console.log(2)", "console.log(3)"].join("\n")
65+
const updated = [
66+
"import { log } from 'node:console'",
67+
"console.log(1)",
68+
"console.log(2)",
69+
"console.log(3)",
70+
].join("\n")
71+
72+
const result = FilesChangedManager.calculateLineDifferences(original, updated)
73+
74+
expect(result).toEqual({ linesAdded: 1, linesRemoved: 0 })
75+
})
6276
})

‎src/services/files-changed/__tests__/FilesChangedMessageHandler.test.ts‎

Lines changed: 102 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,9 @@ import { FilesChangedMessageHandler } from "../FilesChangedMessageHandler"
44
import { FilesChangedManager } from "../FilesChangedManager"
55
import { TaskFilesChangedState } from "../TaskFilesChangedState"
66
import type { Task } from "../../../core/task/Task"
7+
import { getCheckpointService } from "../../../core/checkpoints"
78

8-
vi.mock("../../core/checkpoints", () => ({
9+
vi.mock("../../../core/checkpoints", () => ({
910
getCheckpointService: vi.fn(async () => ({})),
1011
}))
1112

@@ -43,7 +44,9 @@ describe("FilesChangedMessageHandler", () => {
4344

4445
beforeEach(() => {
4546
vi.useFakeTimers()
47+
vi.mocked(getCheckpointService).mockReset()
4648
checkpointService = new MockCheckpointService()
49+
vi.mocked(getCheckpointService).mockResolvedValue(checkpointService as any)
4750
fileContextTracker = new MockFileContextTracker()
4851
posts = []
4952

@@ -280,6 +283,104 @@ describe("FilesChangedMessageHandler", () => {
280283
expect(message?.filesChanged?.files?.map((f: any) => f.uri).sort()).toEqual(["app/bar.ts", "app/foo.ts"])
281284
})
282285

286+
it("does not clear queued diff updates when rehydrating task state", async () => {
287+
await handler.handleExperimentToggle(true, provider.getCurrentTask())
288+
await emitBaseline()
289+
290+
fileContextTracker.emit("roo_edited", "app/foo.ts")
291+
await advance()
292+
293+
posts.length = 0
294+
const freshState = new TaskFilesChangedState()
295+
freshState.cloneFrom(taskState)
296+
const freshTask = {
297+
taskId: "task-1",
298+
checkpointService,
299+
fileContextTracker,
300+
ensureFilesChangedState: vi.fn(() => freshState),
301+
getFilesChangedState: vi.fn(() => freshState),
302+
disposeFilesChangedState: vi.fn(() => freshState.dispose()),
303+
} as unknown as Task
304+
305+
const previousGetter = provider.getCurrentTask
306+
provider.getCurrentTask = () => freshTask
307+
await handler.applyExperimentsToTask(freshTask)
308+
provider.getCurrentTask = previousGetter
309+
310+
const latest = getLatestFilesMessage()
311+
expect((latest?.filesChanged?.files ?? []).length).toBeGreaterThan(0)
312+
})
313+
314+
it("clears pending tracker debounce when switching tasks", async () => {
315+
await handler.handleExperimentToggle(true, provider.getCurrentTask())
316+
await emitBaseline()
317+
318+
fileContextTracker.emit("roo_edited", "app/foo.ts")
319+
expect(vi.getTimerCount()).toBe(1)
320+
321+
const nextState = new TaskFilesChangedState()
322+
const nextTask = {
323+
taskId: "task-2",
324+
checkpointService: new MockCheckpointService(),
325+
fileContextTracker: new MockFileContextTracker(),
326+
ensureFilesChangedState: vi.fn(() => nextState),
327+
getFilesChangedState: vi.fn(() => nextState),
328+
disposeFilesChangedState: vi.fn(() => nextState.dispose()),
329+
} as unknown as Task
330+
331+
await handler.applyExperimentsToTask(nextTask)
332+
333+
expect(vi.getTimerCount()).toBe(0)
334+
})
335+
336+
it("logs and aborts enable when checkpoint service initialization fails", async () => {
337+
vi.mocked(getCheckpointService).mockRejectedValueOnce(new Error("nope"))
338+
339+
await handler.handleExperimentToggle(true, provider.getCurrentTask())
340+
341+
expect(provider.log).toHaveBeenCalledWith(expect.stringContaining("Failed to initialize checkpoint service"))
342+
expect(fileContextTracker.listenerCount("roo_edited")).toBe(0)
343+
})
344+
345+
it("maintains existing files when queued child URIs drain immediately", async () => {
346+
await handler.handleExperimentToggle(true, provider.getCurrentTask())
347+
await emitBaseline()
348+
349+
checkpointService.getDiff.mockResolvedValueOnce([
350+
{
351+
paths: { relative: "app/foo.ts", newFile: false, deletedFile: false },
352+
content: { before: "console.log(1)\n", after: "console.log(2)\n" },
353+
},
354+
])
355+
checkpointService.getDiffStats.mockResolvedValueOnce({ "app/foo.ts": { insertions: 1, deletions: 0 } })
356+
357+
fileContextTracker.emit("roo_edited", "app/foo.ts")
358+
await advance()
359+
posts.length = 0
360+
361+
const combinedDiff = [
362+
{
363+
paths: { relative: "app/foo.ts", newFile: false, deletedFile: false },
364+
content: { before: "console.log(2)\n", after: "console.log(3)\n" },
365+
},
366+
{
367+
paths: { relative: "child.ts", newFile: true, deletedFile: false },
368+
content: { before: "", after: "export const child = 1\n" },
369+
},
370+
]
371+
checkpointService.getDiff.mockResolvedValue(combinedDiff)
372+
checkpointService.getDiffStats.mockResolvedValue({
373+
"app/foo.ts": { insertions: 1, deletions: 0 },
374+
"child.ts": { insertions: 1, deletions: 0 },
375+
})
376+
377+
handler.queueChildFiles(provider.getCurrentTask(), "child-task", ["child.ts"])
378+
await vi.waitUntil(() => Boolean(getLatestFilesMessage()?.filesChanged))
379+
380+
const uris = (getLatestFilesMessage()?.filesChanged?.files ?? []).map((f: any) => f.uri)
381+
expect(uris.sort()).toEqual(["app/foo.ts", "child.ts"].sort())
382+
})
383+
283384
it("reattaches listeners when switching to a subtask", async () => {
284385
await handler.handleExperimentToggle(true, provider.getCurrentTask())
285386
checkpointService.emit("checkpoint", { fromHash: "commit-A", toHash: "commit-B" })

‎webview-ui/src/components/chat/TaskHeader.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import { ContextWindowProgress } from "./ContextWindowProgress"
2424
import { Mention } from "./Mention"
2525
import { TodoListDisplay } from "./TodoListDisplay"
2626
import FilesChangedOverview from "../file-changes/FilesChangedOverview"
27+
import ErrorBoundary from "../ErrorBoundary"
2728

2829
export interface TaskHeaderProps {
2930
task: ClineMessage
@@ -328,7 +329,9 @@ const TaskHeader = ({
328329
)}
329330
</div>
330331
<TodoListDisplay todos={todos ?? (task as any)?.tool?.todos ?? []} />
331-
<FilesChangedOverview />
332+
<ErrorBoundary>
333+
<FilesChangedOverview />
334+
</ErrorBoundary>
332335
<CloudUpsellDialog open={isOpen} onOpenChange={closeUpsell} onConnect={handleConnect} />
333336
</div>
334337
)

‎webview-ui/src/components/chat/__tests__/ChatRow.run-slash-command.spec.tsx‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,13 @@ vi.mock("react-i18next", () => ({
1616
return translations[key] || key
1717
},
1818
}),
19+
withTranslation: () => (Component: any) => {
20+
Component.defaultProps = {
21+
...Component.defaultProps,
22+
t: (key: string) => key,
23+
}
24+
return Component
25+
},
1926
Trans: ({ i18nKey, children }: { i18nKey: string; children?: React.ReactNode }) => {
2027
return <>{children || i18nKey}</>
2128
},

0 commit comments

Comments
 (0)