Skip to content

Commit 73e88bd

Browse files
roomoteroomote
committed
feat: Add file-based editing mode to bypass diff view (#6001)
- Created IEditingProvider interface to abstract file editing operations - Implemented FileWriter class for direct file system writes without diff view - Updated Task class to use IEditingProvider interface instead of DiffViewProvider directly - Added fileBasedEditing setting to global settings and UI - Updated all file editing tools to use the new interface - Added comprehensive tests for FileWriter class - Updated existing tests to work with the new interface This feature allows users to skip the diff view and apply edits directly to files, which is useful for users who prefer to review changes in their own editor or version control system.
1 parent 9fce90b commit 73e88bd

22 files changed

+822
-115
lines changed

‎packages/types/src/global-settings.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,7 @@ export const globalSettingsSchema = z.object({
107107

108108
rateLimitSeconds: z.number().optional(),
109109
diffEnabled: z.boolean().optional(),
110+
fileBasedEditing: z.boolean().optional(),
110111
fuzzyMatchThreshold: z.number().optional(),
111112
experiments: experimentsSchema.optional(),
112113

‎src/core/task/Task.ts‎

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,8 @@ import { RepoPerTaskCheckpointService } from "../../services/checkpoints"
5454

5555
// integrations
5656
import { DiffViewProvider } from "../../integrations/editor/DiffViewProvider"
57+
import { FileWriter } from "../../integrations/editor/FileWriter"
58+
import { IEditingProvider } from "../../integrations/editor/IEditingProvider"
5759
import { findToolName, formatContentBlockToMarkdown } from "../../integrations/misc/export-markdown"
5860
import { RooTerminalProcess } from "../../integrations/terminal/types"
5961
import { TerminalRegistry } from "../../integrations/terminal/TerminalRegistry"
@@ -172,7 +174,7 @@ export class Task extends EventEmitter<ClineEvents> {
172174
browserSession: BrowserSession
173175

174176
// Editing
175-
diffViewProvider: DiffViewProvider
177+
editingProvider: IEditingProvider
176178
diffStrategy?: DiffStrategy
177179
diffEnabled: boolean = false
178180
fuzzyMatchThreshold: number
@@ -260,7 +262,28 @@ export class Task extends EventEmitter<ClineEvents> {
260262
this.consecutiveMistakeLimit = consecutiveMistakeLimit ?? DEFAULT_CONSECUTIVE_MISTAKE_LIMIT
261263
this.providerRef = new WeakRef(provider)
262264
this.globalStoragePath = provider.context.globalStorageUri.fsPath
263-
this.diffViewProvider = new DiffViewProvider(this.cwd)
265+
266+
// Default to DiffViewProvider initially
267+
this.editingProvider = new DiffViewProvider(this.cwd)
268+
269+
// Initialize editing provider based on settings
270+
if (provider.getState) {
271+
provider
272+
.getState()
273+
.then((state) => {
274+
const fileBasedEditing = state?.fileBasedEditing ?? false
275+
if (fileBasedEditing) {
276+
this.editingProvider = new FileWriter(this.cwd)
277+
} else {
278+
this.editingProvider = new DiffViewProvider(this.cwd)
279+
}
280+
})
281+
.catch((error) => {
282+
console.error("Failed to get provider state for editing provider initialization:", error)
283+
// Keep the default DiffViewProvider
284+
})
285+
}
286+
264287
this.enableCheckpoints = enableCheckpoints
265288

266289
this.rootTask = rootTask
@@ -1066,8 +1089,8 @@ export class Task extends EventEmitter<ClineEvents> {
10661089

10671090
try {
10681091
// If we're not streaming then `abortStream` won't be called
1069-
if (this.isStreaming && this.diffViewProvider.isEditing) {
1070-
this.diffViewProvider.revertChanges().catch(console.error)
1092+
if (this.isStreaming && this.editingProvider.isEditing) {
1093+
this.editingProvider.revertChanges().catch(console.error)
10711094
}
10721095
} catch (error) {
10731096
console.error("Error reverting diff changes:", error)
@@ -1296,8 +1319,8 @@ export class Task extends EventEmitter<ClineEvents> {
12961319
}
12971320

12981321
const abortStream = async (cancelReason: ClineApiReqCancelReason, streamingFailedMessage?: string) => {
1299-
if (this.diffViewProvider.isEditing) {
1300-
await this.diffViewProvider.revertChanges() // closes diff view
1322+
if (this.editingProvider.isEditing) {
1323+
await this.editingProvider.revertChanges() // closes diff view
13011324
}
13021325

13031326
// if last message is a partial we need to update and save it
@@ -1349,7 +1372,7 @@ export class Task extends EventEmitter<ClineEvents> {
13491372
this.presentAssistantMessageLocked = false
13501373
this.presentAssistantMessageHasPendingUpdates = false
13511374

1352-
await this.diffViewProvider.reset()
1375+
await this.editingProvider.reset()
13531376

13541377
// Yields only if the first chunk is successful, otherwise will
13551378
// allow the user to retry the request (most likely due to rate

‎src/core/tools/__tests__/applyDiffTool.experiment.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ describe("applyDiffTool experiment routing", () => {
3434
applyDiff: vi.fn(),
3535
getProgressStatus: vi.fn(),
3636
},
37-
diffViewProvider: {
37+
editingProvider: {
3838
reset: vi.fn(),
3939
},
4040
api: {

‎src/core/tools/__tests__/insertContentTool.spec.ts‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ describe("insertContentTool", () => {
8282
rooIgnoreController: {
8383
validateAccess: vi.fn().mockReturnValue(true),
8484
},
85-
diffViewProvider: {
85+
editingProvider: {
8686
editType: undefined,
8787
isEditing: false,
8888
originalContent: "",
@@ -179,9 +179,9 @@ describe("insertContentTool", () => {
179179
const calledPath = mockedFileExistsAtPath.mock.calls[0][0]
180180
expect(toPosix(calledPath)).toContain(testFilePath)
181181
expect(mockedFsReadFile).not.toHaveBeenCalled() // Should not read if file doesn't exist
182-
expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(contentToInsert, true)
183-
expect(mockCline.diffViewProvider.editType).toBe("create")
184-
expect(mockCline.diffViewProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true)
182+
expect(mockCline.editingProvider.update).toHaveBeenCalledWith(contentToInsert, true)
183+
expect(mockCline.editingProvider.editType).toBe("create")
184+
expect(mockCline.editingProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true)
185185
})
186186

187187
it("creates a new file and inserts content at line 1 (beginning)", async () => {
@@ -195,9 +195,9 @@ describe("insertContentTool", () => {
195195
const calledPath = mockedFileExistsAtPath.mock.calls[0][0]
196196
expect(toPosix(calledPath)).toContain(testFilePath)
197197
expect(mockedFsReadFile).not.toHaveBeenCalled()
198-
expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(contentToInsert, true)
199-
expect(mockCline.diffViewProvider.editType).toBe("create")
200-
expect(mockCline.diffViewProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true)
198+
expect(mockCline.editingProvider.update).toHaveBeenCalledWith(contentToInsert, true)
199+
expect(mockCline.editingProvider.editType).toBe("create")
200+
expect(mockCline.editingProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true)
201201
})
202202

203203
it("creates an empty new file if content is empty string", async () => {
@@ -207,9 +207,9 @@ describe("insertContentTool", () => {
207207
const calledPath = mockedFileExistsAtPath.mock.calls[0][0]
208208
expect(toPosix(calledPath)).toContain(testFilePath)
209209
expect(mockedFsReadFile).not.toHaveBeenCalled()
210-
expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith("", true)
211-
expect(mockCline.diffViewProvider.editType).toBe("create")
212-
expect(mockCline.diffViewProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true)
210+
expect(mockCline.editingProvider.update).toHaveBeenCalledWith("", true)
211+
expect(mockCline.editingProvider.editType).toBe("create")
212+
expect(mockCline.editingProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true)
213213
})
214214

215215
it("returns an error when inserting content at an arbitrary line number into a new file", async () => {
@@ -226,8 +226,8 @@ describe("insertContentTool", () => {
226226
expect(mockCline.consecutiveMistakeCount).toBe(1)
227227
expect(mockCline.recordToolError).toHaveBeenCalledWith("insert_content")
228228
expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("non-existent file"))
229-
expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled()
230-
expect(mockCline.diffViewProvider.pushToolWriteResult).not.toHaveBeenCalled()
229+
expect(mockCline.editingProvider.update).not.toHaveBeenCalled()
230+
expect(mockCline.editingProvider.pushToolWriteResult).not.toHaveBeenCalled()
231231
})
232232
})
233233
})

‎src/core/tools/__tests__/writeToFileTool.spec.ts‎

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ describe("writeToFileTool", () => {
143143
mockCline.rooIgnoreController = {
144144
validateAccess: vi.fn().mockReturnValue(true),
145145
}
146-
mockCline.diffViewProvider = {
146+
mockCline.editingProvider = {
147147
editType: undefined,
148148
isEditing: false,
149149
originalContent: "",
@@ -246,7 +246,7 @@ describe("writeToFileTool", () => {
246246
await executeWriteFileTool({}, { accessAllowed: true })
247247

248248
expect(mockCline.rooIgnoreController.validateAccess).toHaveBeenCalledWith(testFilePath)
249-
expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath)
249+
expect(mockCline.editingProvider.open).toHaveBeenCalledWith(testFilePath)
250250
})
251251
})
252252

@@ -255,18 +255,18 @@ describe("writeToFileTool", () => {
255255
await executeWriteFileTool({}, { fileExists: true })
256256

257257
expect(mockedFileExistsAtPath).toHaveBeenCalledWith(absoluteFilePath)
258-
expect(mockCline.diffViewProvider.editType).toBe("modify")
258+
expect(mockCline.editingProvider.editType).toBe("modify")
259259
})
260260

261261
it.skipIf(process.platform === "win32")("detects new file and sets editType to create", async () => {
262262
await executeWriteFileTool({}, { fileExists: false })
263263

264264
expect(mockedFileExistsAtPath).toHaveBeenCalledWith(absoluteFilePath)
265-
expect(mockCline.diffViewProvider.editType).toBe("create")
265+
expect(mockCline.editingProvider.editType).toBe("create")
266266
})
267267

268268
it("uses cached editType without filesystem check", async () => {
269-
mockCline.diffViewProvider.editType = "modify"
269+
mockCline.editingProvider.editType = "modify"
270270

271271
await executeWriteFileTool({})
272272

@@ -278,13 +278,13 @@ describe("writeToFileTool", () => {
278278
it("removes markdown code block markers from content", async () => {
279279
await executeWriteFileTool({ content: testContentWithMarkdown })
280280

281-
expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith("Line 1\nLine 2", true)
281+
expect(mockCline.editingProvider.update).toHaveBeenCalledWith("Line 1\nLine 2", true)
282282
})
283283

284284
it("passes through empty content unchanged", async () => {
285285
await executeWriteFileTool({ content: "" })
286286

287-
expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith("", true)
287+
expect(mockCline.editingProvider.update).toHaveBeenCalledWith("", true)
288288
})
289289

290290
it("unescapes HTML entities for non-Claude models", async () => {
@@ -312,7 +312,7 @@ describe("writeToFileTool", () => {
312312

313313
expect(mockedEveryLineHasLineNumbers).toHaveBeenCalledWith(contentWithLineNumbers)
314314
expect(mockedStripLineNumbers).toHaveBeenCalledWith(contentWithLineNumbers)
315-
expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith("line one\nline two", true)
315+
expect(mockCline.editingProvider.update).toHaveBeenCalledWith("line one\nline two", true)
316316
})
317317
})
318318

@@ -321,10 +321,10 @@ describe("writeToFileTool", () => {
321321
await executeWriteFileTool({}, { fileExists: false })
322322

323323
expect(mockCline.consecutiveMistakeCount).toBe(0)
324-
expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath)
325-
expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(testContent, true)
324+
expect(mockCline.editingProvider.open).toHaveBeenCalledWith(testFilePath)
325+
expect(mockCline.editingProvider.update).toHaveBeenCalledWith(testContent, true)
326326
expect(mockAskApproval).toHaveBeenCalled()
327-
expect(mockCline.diffViewProvider.saveChanges).toHaveBeenCalled()
327+
expect(mockCline.editingProvider.saveChanges).toHaveBeenCalled()
328328
expect(mockCline.fileContextTracker.trackFileContext).toHaveBeenCalledWith(testFilePath, "roo_edited")
329329
expect(mockCline.didEditFile).toBe(true)
330330
})
@@ -349,21 +349,21 @@ describe("writeToFileTool", () => {
349349
it("returns early when path is missing in partial block", async () => {
350350
await executeWriteFileTool({ path: undefined }, { isPartial: true })
351351

352-
expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled()
352+
expect(mockCline.editingProvider.open).not.toHaveBeenCalled()
353353
})
354354

355355
it("returns early when content is undefined in partial block", async () => {
356356
await executeWriteFileTool({ content: undefined }, { isPartial: true })
357357

358-
expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled()
358+
expect(mockCline.editingProvider.open).not.toHaveBeenCalled()
359359
})
360360

361361
it("streams content updates during partial execution", async () => {
362362
await executeWriteFileTool({}, { isPartial: true })
363363

364364
expect(mockCline.ask).toHaveBeenCalled()
365-
expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath)
366-
expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(testContent, false)
365+
expect(mockCline.editingProvider.open).toHaveBeenCalledWith(testFilePath)
366+
expect(mockCline.editingProvider.update).toHaveBeenCalledWith(testContent, false)
367367
})
368368
})
369369

@@ -373,19 +373,19 @@ describe("writeToFileTool", () => {
373373

374374
await executeWriteFileTool({})
375375

376-
expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled()
377-
expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled()
376+
expect(mockCline.editingProvider.revertChanges).toHaveBeenCalled()
377+
expect(mockCline.editingProvider.saveChanges).not.toHaveBeenCalled()
378378
})
379379

380380
it("reports user edits with diff feedback", async () => {
381381
const userEditsValue = "- old line\n+ new line"
382-
mockCline.diffViewProvider.saveChanges.mockResolvedValue({
382+
mockCline.editingProvider.saveChanges.mockResolvedValue({
383383
newProblemsMessage: " with warnings",
384384
userEdits: userEditsValue,
385385
finalContent: "modified content",
386386
})
387387
// Set the userEdits property on the diffViewProvider mock to simulate user edits
388-
mockCline.diffViewProvider.userEdits = userEditsValue
388+
mockCline.editingProvider.userEdits = userEditsValue
389389

390390
await executeWriteFileTool({}, { fileExists: true })
391391

@@ -398,21 +398,21 @@ describe("writeToFileTool", () => {
398398

399399
describe("error handling", () => {
400400
it("handles general file operation errors", async () => {
401-
mockCline.diffViewProvider.open.mockRejectedValue(new Error("General error"))
401+
mockCline.editingProvider.open.mockRejectedValue(new Error("General error"))
402402

403403
await executeWriteFileTool({})
404404

405405
expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error))
406-
expect(mockCline.diffViewProvider.reset).toHaveBeenCalled()
406+
expect(mockCline.editingProvider.reset).toHaveBeenCalled()
407407
})
408408

409409
it("handles partial streaming errors", async () => {
410-
mockCline.diffViewProvider.open.mockRejectedValue(new Error("Open failed"))
410+
mockCline.editingProvider.open.mockRejectedValue(new Error("Open failed"))
411411

412412
await executeWriteFileTool({}, { isPartial: true })
413413

414414
expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error))
415-
expect(mockCline.diffViewProvider.reset).toHaveBeenCalled()
415+
expect(mockCline.editingProvider.reset).toHaveBeenCalled()
416416
})
417417
})
418418
})

‎src/core/tools/applyDiffTool.ts‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -143,10 +143,12 @@ export async function applyDiffToolLegacy(
143143
cline.consecutiveMistakeCountForApplyDiff.delete(relPath)
144144

145145
// Show diff view before asking for approval
146-
cline.diffViewProvider.editType = "modify"
147-
await cline.diffViewProvider.open(relPath)
148-
await cline.diffViewProvider.update(diffResult.content, true)
149-
cline.diffViewProvider.scrollToFirstDiff()
146+
cline.editingProvider.editType = "modify"
147+
await cline.editingProvider.open(relPath)
148+
await cline.editingProvider.update(diffResult.content, true)
149+
if (cline.editingProvider.scrollToFirstDiff) {
150+
cline.editingProvider.scrollToFirstDiff()
151+
}
150152

151153
// Check if file is write-protected
152154
const isWriteProtected = cline.rooProtectedController?.isWriteProtected(relPath) || false
@@ -166,7 +168,7 @@ export async function applyDiffToolLegacy(
166168
const didApprove = await askApproval("tool", completeMessage, toolProgressStatus, isWriteProtected)
167169

168170
if (!didApprove) {
169-
await cline.diffViewProvider.revertChanges() // Cline likely handles closing the diff view
171+
await cline.editingProvider.revertChanges() // Cline likely handles closing the diff view
170172
return
171173
}
172174

@@ -175,7 +177,7 @@ export async function applyDiffToolLegacy(
175177
const state = await provider?.getState()
176178
const diagnosticsEnabled = state?.diagnosticsEnabled ?? true
177179
const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS
178-
await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
180+
await cline.editingProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
179181

180182
// Track file edit operation
181183
if (relPath) {
@@ -191,21 +193,21 @@ export async function applyDiffToolLegacy(
191193
}
192194

193195
// Get the formatted response message
194-
const message = await cline.diffViewProvider.pushToolWriteResult(cline, cline.cwd, !fileExists)
196+
const message = await cline.editingProvider.pushToolWriteResult(cline, cline.cwd, !fileExists)
195197

196198
if (partFailHint) {
197199
pushToolResult(partFailHint + message)
198200
} else {
199201
pushToolResult(message)
200202
}
201203

202-
await cline.diffViewProvider.reset()
204+
await cline.editingProvider.reset()
203205

204206
return
205207
}
206208
} catch (error) {
207209
await handleError("applying diff", error)
208-
await cline.diffViewProvider.reset()
210+
await cline.editingProvider.reset()
209211
return
210212
}
211213
}

0 commit comments

Comments
 (0)