diff --git a/.changeset/green-rings-notice.md b/.changeset/green-rings-notice.md new file mode 100644 index 0000000000..adae3ccc4d --- /dev/null +++ b/.changeset/green-rings-notice.md @@ -0,0 +1,5 @@ +--- +"roo-cline": patch +--- + +Adds file based editing as experimental feature. diff --git a/packages/types/src/experiment.ts b/packages/types/src/experiment.ts index 10384db8ed..537a9cc687 100644 --- a/packages/types/src/experiment.ts +++ b/packages/types/src/experiment.ts @@ -6,7 +6,7 @@ import type { Keys, Equals, AssertEqual } from "./type-fu.js" * ExperimentId */ -export const experimentIds = ["powerSteering", "multiFileApplyDiff"] as const +export const experimentIds = ["powerSteering", "multiFileApplyDiff", "fileBasedEditing"] as const export const experimentIdsSchema = z.enum(experimentIds) @@ -19,6 +19,7 @@ export type ExperimentId = z.infer export const experimentsSchema = z.object({ powerSteering: z.boolean().optional(), multiFileApplyDiff: z.boolean().optional(), + fileBasedEditing: z.boolean().optional(), }) export type Experiments = z.infer diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 95d12f66aa..fcd97ea236 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -25,6 +25,7 @@ import { TodoItem, getApiProtocol, getModelId, + Experiments, } from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" import { CloudService } from "@roo-code/cloud" @@ -54,7 +55,6 @@ import { McpServerManager } from "../../services/mcp/McpServerManager" import { RepoPerTaskCheckpointService } from "../../services/checkpoints" // integrations -import { DiffViewProvider } from "../../integrations/editor/DiffViewProvider" import { findToolName, formatContentBlockToMarkdown } from "../../integrations/misc/export-markdown" import { RooTerminalProcess } from "../../integrations/terminal/types" import { TerminalRegistry } from "../../integrations/terminal/TerminalRegistry" @@ -92,6 +92,8 @@ import { ApiMessage } from "../task-persistence/apiMessages" import { getMessagesSinceLastSummary, summarizeConversation } from "../condense" import { maybeRemoveImageBlocks } from "../../api/transform/image-cleaning" import { restoreTodoListForTask } from "../tools/updateTodoListTool" +import { IEditingProvider } from "../../integrations/editor/IEditingProvider" +import { EditingProviderFactory } from "../../integrations/editor/EditingProviderFactory" // Constants const MAX_EXPONENTIAL_BACKOFF_SECONDS = 600 // 10 minutes @@ -120,7 +122,7 @@ export type TaskOptions = { task?: string images?: string[] historyItem?: HistoryItem - experiments?: Record + experiments?: Experiments startTask?: boolean rootTask?: Task parentTask?: Task @@ -173,7 +175,7 @@ export class Task extends EventEmitter { browserSession: BrowserSession // Editing - diffViewProvider: DiffViewProvider + editingProvider: IEditingProvider diffStrategy?: DiffStrategy diffEnabled: boolean = false fuzzyMatchThreshold: number @@ -261,7 +263,7 @@ export class Task extends EventEmitter { this.consecutiveMistakeLimit = consecutiveMistakeLimit ?? DEFAULT_CONSECUTIVE_MISTAKE_LIMIT this.providerRef = new WeakRef(provider) this.globalStoragePath = provider.context.globalStorageUri.fsPath - this.diffViewProvider = new DiffViewProvider(this.cwd, this) + this.editingProvider = EditingProviderFactory.createEditingProvider(this.cwd, {}, this) this.enableCheckpoints = enableCheckpoints this.rootTask = rootTask @@ -274,6 +276,23 @@ export class Task extends EventEmitter { TelemetryService.instance.captureTaskCreated(this.taskId) } + // Check experiment asynchronously and editingprovider if needed + provider.getState().then((state) => { + const isFileBasedEditingEnabled = experiments?.isEnabled( + state.experiments ?? {}, + EXPERIMENT_IDS.FILE_BASED_EDITING, + ) + + if (isFileBasedEditingEnabled) { + this.editingProvider = EditingProviderFactory.resetAndCreateNewEditingProvider( + this.cwd, + this.editingProvider, + state.experiments ?? {}, + this, + ) + } + }) + // Only set up diff strategy if diff is enabled if (this.diffEnabled) { // Default to old strategy, will be updated if experiment is enabled @@ -281,7 +300,7 @@ export class Task extends EventEmitter { // Check experiment asynchronously and update strategy if needed provider.getState().then((state) => { - const isMultiFileApplyDiffEnabled = experiments.isEnabled( + const isMultiFileApplyDiffEnabled = experiments?.isEnabled( state.experiments ?? {}, EXPERIMENT_IDS.MULTI_FILE_APPLY_DIFF, ) @@ -763,6 +782,8 @@ export class Task extends EventEmitter { public async resumePausedTask(lastMessage: string) { // Release this Cline instance from paused state. + const context = await this.providerRef.deref()?.getState() + this.editingProvider = EditingProviderFactory.createEditingProvider(this.cwd, context?.experiments ?? {}, this) this.isPaused = false this.emit("taskUnpaused") @@ -1067,8 +1088,8 @@ export class Task extends EventEmitter { try { // If we're not streaming then `abortStream` won't be called - if (this.isStreaming && this.diffViewProvider.isEditing) { - this.diffViewProvider.revertChanges().catch(console.error) + if (this.isStreaming && this.editingProvider.isEditing) { + this.editingProvider.revertChanges().catch(console.error) } } catch (error) { console.error("Error reverting diff changes:", error) @@ -1161,6 +1182,13 @@ export class Task extends EventEmitter { if (this.abort) { throw new Error(`[RooCode#recursivelyMakeRooRequests] task ${this.taskId}.${this.instanceId} aborted`) } + const context = await this.providerRef.deref()?.getState() + this.editingProvider = EditingProviderFactory.resetAndCreateNewEditingProvider( + this.cwd, + this.editingProvider, + context?.experiments ?? {}, + this, + ) if (this.consecutiveMistakeLimit > 0 && this.consecutiveMistakeCount >= this.consecutiveMistakeLimit) { const { response, text, images } = await this.ask( @@ -1303,8 +1331,8 @@ export class Task extends EventEmitter { } const abortStream = async (cancelReason: ClineApiReqCancelReason, streamingFailedMessage?: string) => { - if (this.diffViewProvider.isEditing) { - await this.diffViewProvider.revertChanges() // closes diff view + if (this.editingProvider.isEditing) { + await this.editingProvider.revertChanges() // closes diff view } // if last message is a partial we need to update and save it @@ -1356,7 +1384,7 @@ export class Task extends EventEmitter { this.presentAssistantMessageLocked = false this.presentAssistantMessageHasPendingUpdates = false - await this.diffViewProvider.reset() + await this.editingProvider.reset() // Yields only if the first chunk is successful, otherwise will // allow the user to retry the request (most likely due to rate diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 9aa5a8d7a8..94cf6e67e5 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -1386,6 +1386,7 @@ describe("Cline", () => { }) it("should not create diff strategy when enableDiff is false", async () => { + mockProvider.getState.mockResolvedValue({}) const task = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, diff --git a/src/core/tools/__tests__/applyDiffTool.experiment.spec.ts b/src/core/tools/__tests__/applyDiffTool.experiment.spec.ts index e763125d4a..16cc3b08e5 100644 --- a/src/core/tools/__tests__/applyDiffTool.experiment.spec.ts +++ b/src/core/tools/__tests__/applyDiffTool.experiment.spec.ts @@ -34,7 +34,7 @@ describe("applyDiffTool experiment routing", () => { applyDiff: vi.fn(), getProgressStatus: vi.fn(), }, - diffViewProvider: { + editingProvider: { reset: vi.fn(), }, api: { diff --git a/src/core/tools/__tests__/insertContentTool.spec.ts b/src/core/tools/__tests__/insertContentTool.spec.ts index 5f055fb29a..64a983c180 100644 --- a/src/core/tools/__tests__/insertContentTool.spec.ts +++ b/src/core/tools/__tests__/insertContentTool.spec.ts @@ -82,7 +82,7 @@ describe("insertContentTool", () => { rooIgnoreController: { validateAccess: vi.fn().mockReturnValue(true), }, - diffViewProvider: { + editingProvider: { editType: undefined, isEditing: false, originalContent: "", @@ -180,9 +180,9 @@ describe("insertContentTool", () => { const calledPath = mockedFileExistsAtPath.mock.calls[0][0] expect(toPosix(calledPath)).toContain(testFilePath) expect(mockedFsReadFile).not.toHaveBeenCalled() // Should not read if file doesn't exist - expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(contentToInsert, true) - expect(mockCline.diffViewProvider.editType).toBe("create") - expect(mockCline.diffViewProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true) + expect(mockCline.editingProvider.update).toHaveBeenCalledWith(contentToInsert, true) + expect(mockCline.editingProvider.editType).toBe("create") + expect(mockCline.editingProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true) }) it("creates a new file and inserts content at line 1 (beginning)", async () => { @@ -196,9 +196,9 @@ describe("insertContentTool", () => { const calledPath = mockedFileExistsAtPath.mock.calls[0][0] expect(toPosix(calledPath)).toContain(testFilePath) expect(mockedFsReadFile).not.toHaveBeenCalled() - expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(contentToInsert, true) - expect(mockCline.diffViewProvider.editType).toBe("create") - expect(mockCline.diffViewProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true) + expect(mockCline.editingProvider.update).toHaveBeenCalledWith(contentToInsert, true) + expect(mockCline.editingProvider.editType).toBe("create") + expect(mockCline.editingProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true) }) it("creates an empty new file if content is empty string", async () => { @@ -208,9 +208,9 @@ describe("insertContentTool", () => { const calledPath = mockedFileExistsAtPath.mock.calls[0][0] expect(toPosix(calledPath)).toContain(testFilePath) expect(mockedFsReadFile).not.toHaveBeenCalled() - expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith("", true) - expect(mockCline.diffViewProvider.editType).toBe("create") - expect(mockCline.diffViewProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true) + expect(mockCline.editingProvider.update).toHaveBeenCalledWith("", true) + expect(mockCline.editingProvider.editType).toBe("create") + expect(mockCline.editingProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true) }) it("returns an error when inserting content at an arbitrary line number into a new file", async () => { @@ -227,8 +227,8 @@ describe("insertContentTool", () => { expect(mockCline.consecutiveMistakeCount).toBe(1) expect(mockCline.recordToolError).toHaveBeenCalledWith("insert_content") expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("non-existent file")) - expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() - expect(mockCline.diffViewProvider.pushToolWriteResult).not.toHaveBeenCalled() + expect(mockCline.editingProvider.update).not.toHaveBeenCalled() + expect(mockCline.editingProvider.pushToolWriteResult).not.toHaveBeenCalled() }) }) }) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 78e60cbaa5..61a5ff6d18 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -143,7 +143,7 @@ describe("writeToFileTool", () => { mockCline.rooIgnoreController = { validateAccess: vi.fn().mockReturnValue(true), } - mockCline.diffViewProvider = { + mockCline.editingProvider = { editType: undefined, isEditing: false, originalContent: "", @@ -247,7 +247,7 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { accessAllowed: true }) expect(mockCline.rooIgnoreController.validateAccess).toHaveBeenCalledWith(testFilePath) - expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath) + expect(mockCline.editingProvider.open).toHaveBeenCalledWith(testFilePath) }) }) @@ -256,18 +256,18 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { fileExists: true }) expect(mockedFileExistsAtPath).toHaveBeenCalledWith(absoluteFilePath) - expect(mockCline.diffViewProvider.editType).toBe("modify") + expect(mockCline.editingProvider.editType).toBe("modify") }) it.skipIf(process.platform === "win32")("detects new file and sets editType to create", async () => { await executeWriteFileTool({}, { fileExists: false }) expect(mockedFileExistsAtPath).toHaveBeenCalledWith(absoluteFilePath) - expect(mockCline.diffViewProvider.editType).toBe("create") + expect(mockCline.editingProvider.editType).toBe("create") }) it("uses cached editType without filesystem check", async () => { - mockCline.diffViewProvider.editType = "modify" + mockCline.editingProvider.editType = "modify" await executeWriteFileTool({}) @@ -279,13 +279,13 @@ describe("writeToFileTool", () => { it("removes markdown code block markers from content", async () => { await executeWriteFileTool({ content: testContentWithMarkdown }) - expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith("Line 1\nLine 2", true) + expect(mockCline.editingProvider.update).toHaveBeenCalledWith("Line 1\nLine 2", true) }) it("passes through empty content unchanged", async () => { await executeWriteFileTool({ content: "" }) - expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith("", true) + expect(mockCline.editingProvider.update).toHaveBeenCalledWith("", true) }) it("unescapes HTML entities for non-Claude models", async () => { @@ -313,7 +313,7 @@ describe("writeToFileTool", () => { expect(mockedEveryLineHasLineNumbers).toHaveBeenCalledWith(contentWithLineNumbers) expect(mockedStripLineNumbers).toHaveBeenCalledWith(contentWithLineNumbers) - expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith("line one\nline two", true) + expect(mockCline.editingProvider.update).toHaveBeenCalledWith("line one\nline two", true) }) }) @@ -322,10 +322,10 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { fileExists: false }) expect(mockCline.consecutiveMistakeCount).toBe(0) - expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath) - expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(testContent, true) + expect(mockCline.editingProvider.open).toHaveBeenCalledWith(testFilePath) + expect(mockCline.editingProvider.update).toHaveBeenCalledWith(testContent, true) expect(mockAskApproval).toHaveBeenCalled() - expect(mockCline.diffViewProvider.saveChanges).toHaveBeenCalled() + expect(mockCline.editingProvider.saveChanges).toHaveBeenCalled() expect(mockCline.fileContextTracker.trackFileContext).toHaveBeenCalledWith(testFilePath, "roo_edited") expect(mockCline.didEditFile).toBe(true) }) @@ -350,21 +350,21 @@ describe("writeToFileTool", () => { it("returns early when path is missing in partial block", async () => { await executeWriteFileTool({ path: undefined }, { isPartial: true }) - expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.editingProvider.open).not.toHaveBeenCalled() }) it("returns early when content is undefined in partial block", async () => { await executeWriteFileTool({ content: undefined }, { isPartial: true }) - expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.editingProvider.open).not.toHaveBeenCalled() }) it("streams content updates during partial execution", async () => { await executeWriteFileTool({}, { isPartial: true }) expect(mockCline.ask).toHaveBeenCalled() - expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath) - expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(testContent, false) + expect(mockCline.editingProvider.open).toHaveBeenCalledWith(testFilePath) + expect(mockCline.editingProvider.update).toHaveBeenCalledWith(testContent, false) }) }) @@ -374,19 +374,19 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}) - expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() - expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockCline.editingProvider.revertChanges).toHaveBeenCalled() + expect(mockCline.editingProvider.saveChanges).not.toHaveBeenCalled() }) it("reports user edits with diff feedback", async () => { const userEditsValue = "- old line\n+ new line" - mockCline.diffViewProvider.saveChanges.mockResolvedValue({ + mockCline.editingProvider.saveChanges.mockResolvedValue({ newProblemsMessage: " with warnings", userEdits: userEditsValue, finalContent: "modified content", }) - // Set the userEdits property on the diffViewProvider mock to simulate user edits - mockCline.diffViewProvider.userEdits = userEditsValue + // Set the userEdits property on the editingProvider mock to simulate user edits + mockCline.editingProvider.userEdits = userEditsValue await executeWriteFileTool({}, { fileExists: true }) @@ -399,21 +399,21 @@ describe("writeToFileTool", () => { describe("error handling", () => { it("handles general file operation errors", async () => { - mockCline.diffViewProvider.open.mockRejectedValue(new Error("General error")) + mockCline.editingProvider.open.mockRejectedValue(new Error("General error")) await executeWriteFileTool({}) expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) - expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockCline.editingProvider.reset).toHaveBeenCalled() }) it("handles partial streaming errors", async () => { - mockCline.diffViewProvider.open.mockRejectedValue(new Error("Open failed")) + mockCline.editingProvider.open.mockRejectedValue(new Error("Open failed")) await executeWriteFileTool({}, { isPartial: true }) expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) - expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockCline.editingProvider.reset).toHaveBeenCalled() }) }) }) diff --git a/src/core/tools/applyDiffTool.ts b/src/core/tools/applyDiffTool.ts index f046ba67d2..fca8a4c3c1 100644 --- a/src/core/tools/applyDiffTool.ts +++ b/src/core/tools/applyDiffTool.ts @@ -143,10 +143,10 @@ export async function applyDiffToolLegacy( cline.consecutiveMistakeCountForApplyDiff.delete(relPath) // Show diff view before asking for approval - cline.diffViewProvider.editType = "modify" - await cline.diffViewProvider.open(relPath) - await cline.diffViewProvider.update(diffResult.content, true) - cline.diffViewProvider.scrollToFirstDiff() + cline.editingProvider.editType = "modify" + await cline.editingProvider.open(relPath) + await cline.editingProvider.update(diffResult.content, true) + cline.editingProvider.scrollToFirstDiff() // Check if file is write-protected const isWriteProtected = cline.rooProtectedController?.isWriteProtected(relPath) || false @@ -166,7 +166,7 @@ export async function applyDiffToolLegacy( const didApprove = await askApproval("tool", completeMessage, toolProgressStatus, isWriteProtected) if (!didApprove) { - await cline.diffViewProvider.revertChanges() // Cline likely handles closing the diff view + await cline.editingProvider.revertChanges() // Cline likely handles closing the diff view return } @@ -175,7 +175,7 @@ export async function applyDiffToolLegacy( const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS - await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + await cline.editingProvider.saveChanges(diagnosticsEnabled, writeDelayMs) // Track file edit operation if (relPath) { @@ -191,7 +191,7 @@ export async function applyDiffToolLegacy( } // Get the formatted response message - const message = await cline.diffViewProvider.pushToolWriteResult(cline, cline.cwd, !fileExists) + const message = await cline.editingProvider.pushToolWriteResult(cline, cline.cwd, !fileExists) // Check for single SEARCH/REPLACE block warning const searchBlocks = (diffContent.match(/<<<<<<< SEARCH/g) || []).length @@ -206,13 +206,13 @@ export async function applyDiffToolLegacy( pushToolResult(message + singleBlockNotice) } - await cline.diffViewProvider.reset() + await cline.editingProvider.reset() return } } catch (error) { await handleError("applying diff", error) - await cline.diffViewProvider.reset() + await cline.editingProvider.reset() return } } diff --git a/src/core/tools/insertContentTool.ts b/src/core/tools/insertContentTool.ts index 2b31224400..06b8daeddc 100644 --- a/src/core/tools/insertContentTool.ts +++ b/src/core/tools/insertContentTool.ts @@ -96,8 +96,8 @@ export async function insertContentTool( cline.consecutiveMistakeCount = 0 - cline.diffViewProvider.editType = fileExists ? "modify" : "create" - cline.diffViewProvider.originalContent = fileContent + cline.editingProvider.editType = fileExists ? "modify" : "create" + cline.editingProvider.originalContent = fileContent const lines = fileExists ? fileContent.split("\n") : [] const updatedContent = insertGroups(lines, [ @@ -108,12 +108,12 @@ export async function insertContentTool( ]).join("\n") // Show changes in diff view - if (!cline.diffViewProvider.isEditing) { + if (!cline.editingProvider.isEditing) { await cline.ask("tool", JSON.stringify(sharedMessageProps), true).catch(() => {}) // First open with original content - await cline.diffViewProvider.open(relPath) - await cline.diffViewProvider.update(fileContent, false) - cline.diffViewProvider.scrollToFirstDiff() + await cline.editingProvider.open(relPath) + await cline.editingProvider.update(fileContent, false) + cline.editingProvider.scrollToFirstDiff() await delay(200) } @@ -135,7 +135,7 @@ export async function insertContentTool( approvalContent = updatedContent } - await cline.diffViewProvider.update(updatedContent, true) + await cline.editingProvider.update(updatedContent, true) const completeMessage = JSON.stringify({ ...sharedMessageProps, @@ -150,7 +150,7 @@ export async function insertContentTool( .then((response) => response.response === "yesButtonClicked") if (!didApprove) { - await cline.diffViewProvider.revertChanges() + await cline.editingProvider.revertChanges() pushToolResult("Changes were rejected by the user.") return } @@ -160,7 +160,7 @@ export async function insertContentTool( const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS - await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + await cline.editingProvider.saveChanges(diagnosticsEnabled, writeDelayMs) // Track file edit operation if (relPath) { @@ -170,13 +170,13 @@ export async function insertContentTool( cline.didEditFile = true // Get the formatted response message - const message = await cline.diffViewProvider.pushToolWriteResult(cline, cline.cwd, !fileExists) + const message = await cline.editingProvider.pushToolWriteResult(cline, cline.cwd, !fileExists) pushToolResult(message) - await cline.diffViewProvider.reset() + await cline.editingProvider.reset() } catch (error) { handleError("insert content", error) - await cline.diffViewProvider.reset() + await cline.editingProvider.reset() } } diff --git a/src/core/tools/multiApplyDiffTool.ts b/src/core/tools/multiApplyDiffTool.ts index ec8c77a63b..9f48b47fe9 100644 --- a/src/core/tools/multiApplyDiffTool.ts +++ b/src/core/tools/multiApplyDiffTool.ts @@ -508,10 +508,10 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""} cline.consecutiveMistakeCountForApplyDiff.delete(relPath) // Show diff view before asking for approval (only for single file or after batch approval) - cline.diffViewProvider.editType = "modify" - await cline.diffViewProvider.open(relPath) - await cline.diffViewProvider.update(originalContent!, true) - cline.diffViewProvider.scrollToFirstDiff() + cline.editingProvider.editType = "modify" + await cline.editingProvider.open(relPath) + await cline.editingProvider.update(originalContent!, true) + cline.editingProvider.scrollToFirstDiff() // For batch operations, we've already gotten approval const isWriteProtected = cline.rooProtectedController?.isWriteProtected(relPath) || false @@ -548,7 +548,7 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""} } if (!didApprove) { - await cline.diffViewProvider.revertChanges() + await cline.editingProvider.revertChanges() results.push(`Changes to ${relPath} were not approved by user`) continue } @@ -558,7 +558,7 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""} const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS - await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + await cline.editingProvider.saveChanges(diagnosticsEnabled, writeDelayMs) // Track file edit operation await cline.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) @@ -572,7 +572,7 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""} } // Get the formatted response message - const message = await cline.diffViewProvider.pushToolWriteResult(cline, cline.cwd, !fileExists) + const message = await cline.editingProvider.pushToolWriteResult(cline, cline.cwd, !fileExists) if (partFailHint) { results.push(partFailHint + "\n" + message) @@ -580,7 +580,7 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""} results.push(message) } - await cline.diffViewProvider.reset() + await cline.editingProvider.reset() } catch (error) { const errorMsg = error instanceof Error ? error.message : String(error) updateOperationResult(relPath, { @@ -620,7 +620,7 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""} return } catch (error) { await handleError("applying diff", error) - await cline.diffViewProvider.reset() + await cline.editingProvider.reset() return } } diff --git a/src/core/tools/searchAndReplaceTool.ts b/src/core/tools/searchAndReplaceTool.ts index b6ec3ed39b..24e9eb880f 100644 --- a/src/core/tools/searchAndReplaceTool.ts +++ b/src/core/tools/searchAndReplaceTool.ts @@ -188,27 +188,27 @@ export async function searchAndReplaceTool( } // Initialize diff view - cline.diffViewProvider.editType = "modify" - cline.diffViewProvider.originalContent = fileContent + cline.editingProvider.editType = "modify" + cline.editingProvider.originalContent = fileContent // Generate and validate diff const diff = formatResponse.createPrettyPatch(validRelPath, fileContent, newContent) if (!diff) { pushToolResult(`No changes needed for '${relPath}'`) - await cline.diffViewProvider.reset() + await cline.editingProvider.reset() return } // Show changes in diff view - if (!cline.diffViewProvider.isEditing) { + if (!cline.editingProvider.isEditing) { await cline.ask("tool", JSON.stringify(sharedMessageProps), true).catch(() => {}) - await cline.diffViewProvider.open(validRelPath) - await cline.diffViewProvider.update(fileContent, false) - cline.diffViewProvider.scrollToFirstDiff() + await cline.editingProvider.open(validRelPath) + await cline.editingProvider.update(fileContent, false) + cline.editingProvider.scrollToFirstDiff() await delay(200) } - await cline.diffViewProvider.update(newContent, true) + await cline.editingProvider.update(newContent, true) // Request user approval for changes const completeMessage = JSON.stringify({ @@ -221,9 +221,9 @@ export async function searchAndReplaceTool( .then((response) => response.response === "yesButtonClicked") if (!didApprove) { - await cline.diffViewProvider.revertChanges() + await cline.editingProvider.revertChanges() pushToolResult("Changes were rejected by the user.") - await cline.diffViewProvider.reset() + await cline.editingProvider.reset() return } @@ -232,7 +232,7 @@ export async function searchAndReplaceTool( const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS - await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + await cline.editingProvider.saveChanges(diagnosticsEnabled, writeDelayMs) // Track file edit operation if (relPath) { @@ -242,7 +242,7 @@ export async function searchAndReplaceTool( cline.didEditFile = true // Get the formatted response message - const message = await cline.diffViewProvider.pushToolWriteResult( + const message = await cline.editingProvider.pushToolWriteResult( cline, cline.cwd, false, // Always false for search_and_replace @@ -252,10 +252,10 @@ export async function searchAndReplaceTool( // Record successful tool usage and cleanup cline.recordToolUsage("search_and_replace") - await cline.diffViewProvider.reset() + await cline.editingProvider.reset() } catch (error) { handleError("search and replace", error) - await cline.diffViewProvider.reset() + await cline.editingProvider.reset() } } diff --git a/src/core/tools/writeToFileTool.ts b/src/core/tools/writeToFileTool.ts index fd9d158f3f..8b3f6daf23 100644 --- a/src/core/tools/writeToFileTool.ts +++ b/src/core/tools/writeToFileTool.ts @@ -37,7 +37,7 @@ export async function writeToFileTool( cline.consecutiveMistakeCount++ cline.recordToolError("write_to_file") pushToolResult(await cline.sayAndCreateMissingParamError("write_to_file", "path")) - await cline.diffViewProvider.reset() + await cline.editingProvider.reset() return } @@ -45,7 +45,7 @@ export async function writeToFileTool( cline.consecutiveMistakeCount++ cline.recordToolError("write_to_file") pushToolResult(await cline.sayAndCreateMissingParamError("write_to_file", "content")) - await cline.diffViewProvider.reset() + await cline.editingProvider.reset() return } @@ -63,12 +63,12 @@ export async function writeToFileTool( // Check if file exists using cached map or fs.access let fileExists: boolean - if (cline.diffViewProvider.editType !== undefined) { - fileExists = cline.diffViewProvider.editType === "modify" + if (cline.editingProvider.editType !== undefined) { + fileExists = cline.editingProvider.editType === "modify" } else { const absolutePath = path.resolve(cline.cwd, relPath) fileExists = await fileExistsAtPath(absolutePath) - cline.diffViewProvider.editType = fileExists ? "modify" : "create" + cline.editingProvider.editType = fileExists ? "modify" : "create" } // pre-processing newContent for cases where weaker models might add artifacts like markdown codeblock markers (deepseek/llama) or extra escape characters (gemini) @@ -104,13 +104,13 @@ export async function writeToFileTool( await cline.ask("tool", partialMessage, block.partial).catch(() => {}) // update editor - if (!cline.diffViewProvider.isEditing) { + if (!cline.editingProvider.isEditing) { // open the editor and prepare to stream content in - await cline.diffViewProvider.open(relPath) + await cline.editingProvider.open(relPath) } // editor is open, stream content in - await cline.diffViewProvider.update( + await cline.editingProvider.update( everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, false, ) @@ -143,7 +143,7 @@ export async function writeToFileTool( formatResponse.lineCountTruncationError(actualLineCount, isNewFile, diffStrategyEnabled), ), ) - await cline.diffViewProvider.revertChanges() + await cline.editingProvider.revertChanges() return } @@ -152,25 +152,25 @@ export async function writeToFileTool( // if isEditingFile false, that means we have the full contents of the file already. // it's important to note how cline function works, you can't make the assumption that the block.partial conditional will always be called since it may immediately get complete, non-partial data. So cline part of the logic will always be called. // in other words, you must always repeat the block.partial logic here - if (!cline.diffViewProvider.isEditing) { + if (!cline.editingProvider.isEditing) { // show gui message before showing edit animation const partialMessage = JSON.stringify(sharedMessageProps) await cline.ask("tool", partialMessage, true).catch(() => {}) // sending true for partial even though it's not a partial, cline shows the edit row before the content is streamed into the editor - await cline.diffViewProvider.open(relPath) + await cline.editingProvider.open(relPath) } - await cline.diffViewProvider.update( + await cline.editingProvider.update( everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, true, ) await delay(300) // wait for diff view to update - cline.diffViewProvider.scrollToFirstDiff() + cline.editingProvider.scrollToFirstDiff() // Check for code omissions before proceeding - if (detectCodeOmission(cline.diffViewProvider.originalContent || "", newContent, predictedLineCount)) { + if (detectCodeOmission(cline.editingProvider.originalContent || "", newContent, predictedLineCount)) { if (cline.diffStrategy) { - await cline.diffViewProvider.revertChanges() + await cline.editingProvider.revertChanges() pushToolResult( formatResponse.toolError( @@ -202,14 +202,14 @@ export async function writeToFileTool( ...sharedMessageProps, content: fileExists ? undefined : newContent, diff: fileExists - ? formatResponse.createPrettyPatch(relPath, cline.diffViewProvider.originalContent, newContent) + ? formatResponse.createPrettyPatch(relPath, cline.editingProvider.originalContent, newContent) : undefined, } satisfies ClineSayTool) const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) if (!didApprove) { - await cline.diffViewProvider.revertChanges() + await cline.editingProvider.revertChanges() return } @@ -218,7 +218,7 @@ export async function writeToFileTool( const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS - await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + await cline.editingProvider.saveChanges(diagnosticsEnabled, writeDelayMs) // Track file edit operation if (relPath) { @@ -228,17 +228,17 @@ export async function writeToFileTool( cline.didEditFile = true // used to determine if we should wait for busy terminal to update before sending api request // Get the formatted response message - const message = await cline.diffViewProvider.pushToolWriteResult(cline, cline.cwd, !fileExists) + const message = await cline.editingProvider.pushToolWriteResult(cline, cline.cwd, !fileExists) pushToolResult(message) - await cline.diffViewProvider.reset() + await cline.editingProvider.reset() return } } catch (error) { await handleError("writing file", error) - await cline.diffViewProvider.reset() + await cline.editingProvider.reset() return } } diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 64820beffb..5164d95fa1 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -15,12 +15,13 @@ import { Task } from "../../core/task/Task" import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { DecorationController } from "./DecorationController" +import { IEditingProvider } from "./IEditingProvider" export const DIFF_VIEW_URI_SCHEME = "cline-diff" export const DIFF_VIEW_LABEL_CHANGES = "Original ↔ Roo's Changes" // TODO: https://github.com/cline/cline/pull/3354 -export class DiffViewProvider { +export class DiffViewProvider implements IEditingProvider { // Properties to store the results of saveChanges newProblemsMessage?: string userEdits?: string @@ -187,7 +188,10 @@ export class DiffViewProvider { } } - async saveChanges(diagnosticsEnabled: boolean = true, writeDelayMs: number = DEFAULT_WRITE_DELAY_MS): Promise<{ + async saveChanges( + diagnosticsEnabled: boolean = true, + writeDelayMs: number = DEFAULT_WRITE_DELAY_MS, + ): Promise<{ newProblemsMessage: string | undefined userEdits: string | undefined finalContent: string | undefined @@ -222,22 +226,22 @@ export class DiffViewProvider { // and can address them accordingly. If problems don't change immediately after // applying a fix, won't be notified, which is generally fine since the // initial fix is usually correct and it may just take time for linters to catch up. - + let newProblemsMessage = "" - + if (diagnosticsEnabled) { // Add configurable delay to allow linters time to process and clean up issues // like unused imports (especially important for Go and other languages) // Ensure delay is non-negative const safeDelayMs = Math.max(0, writeDelayMs) - + try { await delay(safeDelayMs) } catch (error) { // Log error but continue - delay failure shouldn't break the save operation console.warn(`Failed to apply write delay: ${error}`) } - + const postDiagnostics = vscode.languages.getDiagnostics() // Get diagnostic settings from state diff --git a/src/integrations/editor/EditingProviderFactory.ts b/src/integrations/editor/EditingProviderFactory.ts new file mode 100644 index 0000000000..a072a3246c --- /dev/null +++ b/src/integrations/editor/EditingProviderFactory.ts @@ -0,0 +1,49 @@ +import { DiffViewProvider } from "./DiffViewProvider" +import { FileWriter } from "./FileWriter" +import { IEditingProvider } from "./IEditingProvider" +import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" +import { Experiments } from "@roo-code/types" +import { Task } from "../../core/task/Task" + +/** + * Factory for creating the appropriate editing provider based on user settings + */ +export class EditingProviderFactory { + /** + * Creates an editing provider based on current VSCode settings + * @param cwd The current working directory + * @param experimentConfig The experiments configuration to check for feature flags + * @param task The current task instance + * @returns The appropriate editing provider (DiffViewProvider or FileWriter) + */ + static createEditingProvider(cwd: string, experimentConfig: Experiments = {}, task: Task): IEditingProvider { + const fileBasedEditing = experiments.isEnabled(experimentConfig, EXPERIMENT_IDS.FILE_BASED_EDITING) + + if (fileBasedEditing) { + return new FileWriter(cwd) + } else { + return new DiffViewProvider(cwd, task) + } + } + + /** + * Resets the current editing provider and creates a new one based on the current working directory + * @param cwd The current working directory + * @param editingProvider The current editing provider instance to reset + * @param experimentConfig The experiments configuration to check for feature flags + * @param task The current task instance + * @returns A new instance of the appropriate editing provider + */ + static resetAndCreateNewEditingProvider( + cwd: string, + editingProvider: IEditingProvider, + experimentConfig: Experiments, + task: Task, + ): IEditingProvider { + // Reset the current editing provider + editingProvider.reset() + + // Create a new instance of the appropriate provider + return this.createEditingProvider(cwd, experimentConfig, task) + } +} diff --git a/src/integrations/editor/FileWriter.ts b/src/integrations/editor/FileWriter.ts new file mode 100644 index 0000000000..4899fb5087 --- /dev/null +++ b/src/integrations/editor/FileWriter.ts @@ -0,0 +1,320 @@ +import * as vscode from "vscode" +import * as path from "path" +import * as fs from "fs/promises" +import stripBom from "strip-bom" +import { XMLBuilder } from "fast-xml-parser" + +import { createDirectoriesForFile } from "../../utils/fs" +import { getReadablePath } from "../../utils/path" +import { formatResponse } from "../../core/prompts/responses" +import { diagnosticsToProblemsString, getNewDiagnostics } from "../diagnostics" +import { ClineSayTool } from "../../shared/ExtensionMessage" +import { Task } from "../../core/task/Task" +import { IEditingProvider } from "./IEditingProvider" +import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" +import delay from "delay" + +/** + * FileWriter provides direct file-system editing without diff views. + * It mirrors the API of DiffViewProvider for seamless integration. + */ +export class FileWriter implements IEditingProvider { + // Properties to store the results of saveChanges + newProblemsMessage?: string + userEdits?: string + editType?: "create" | "modify" + isEditing = false + originalContent: string | undefined + private createdDirs: string[] = [] + private relPath?: string + private newContent?: string + private preDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [] + + constructor(private cwd: string) {} + + /** + * Prepares for editing the given relative path file + * @param relPath The relative file path to prepare for editing + */ + async open(relPath: string): Promise { + this.relPath = relPath + const absolutePath = path.resolve(this.cwd, relPath) + this.isEditing = true + + // Get diagnostics before editing the file + this.preDiagnostics = vscode.languages.getDiagnostics() + + // Check if file exists to set edit type + try { + this.originalContent = await fs.readFile(absolutePath, "utf-8") + this.editType = "modify" + } catch (error) { + this.originalContent = "" + this.editType = "create" + } + + // For new files, create any necessary directories + if (this.editType === "create") { + this.createdDirs = await createDirectoriesForFile(absolutePath) + } + } + + /** + * Updates the file content (writes directly to file system) + * @param accumulatedContent The content to write + * @param isFinal Whether this is the final update + */ + async update(accumulatedContent: string, isFinal: boolean): Promise { + if (!this.relPath) { + throw new Error("Required values not set") + } + + this.newContent = accumulatedContent + const absolutePath = path.resolve(this.cwd, this.relPath) + + if (isFinal) { + // Preserve empty last line if original content had one + const hasEmptyLastLine = this.originalContent?.endsWith("\n") + + if (hasEmptyLastLine && !accumulatedContent.endsWith("\n")) { + accumulatedContent += "\n" + } + + // Write the final content directly to file + await fs.writeFile(absolutePath, this.stripAllBOMs(accumulatedContent), "utf-8") + } + } + + /** + * Finalizes the file changes and returns diagnostics information + * @param diagnosticsEnabled Whether to enable diagnostics (default: true) + * @param writeDelayMs Delay in milliseconds before writing changes (default: 1000) + */ + async saveChanges( + diagnosticsEnabled: boolean = true, + writeDelayMs: number = DEFAULT_WRITE_DELAY_MS, + ): Promise<{ + newProblemsMessage: string | undefined + userEdits: string | undefined + finalContent: string | undefined + }> { + if (!this.relPath || !this.newContent) { + return { newProblemsMessage: undefined, userEdits: undefined, finalContent: undefined } + } + + const absolutePath = path.resolve(this.cwd, this.relPath) + + // Read the actual file content to check if it matches what we wrote + const finalContent = await fs.readFile(absolutePath, "utf-8") + + // Getting diagnostics before and after the file edit is a better approach than + // automatically tracking problems in real-time. This method ensures we only + // report new problems that are a direct result of this specific edit. + // Since these are new problems resulting from Roo's edit, we know they're + // directly related to the work he's doing. This eliminates the risk of Roo + // going off-task or getting distracted by unrelated issues, which was a problem + // with the previous auto-debug approach. Some users' machines may be slow to + // update diagnostics, so this approach provides a good balance between automation + // and avoiding potential issues where Roo might get stuck in loops due to + // outdated problem information. If no new problems show up by the time the user + // accepts the changes, they can always debug later using the '@problems' mention. + // This way, Roo only becomes aware of new problems resulting from his edits + // and can address them accordingly. If problems don't change immediately after + // applying a fix, won't be notified, which is generally fine since the + // initial fix is usually correct and it may just take time for linters to catch up. + + let newProblemsMessage = "" + + if (diagnosticsEnabled) { + // Add configurable delay to allow linters time to process and clean up issues + // like unused imports (especially important for Go and other languages) + // Ensure delay is non-negative + const safeDelayMs = Math.max(0, writeDelayMs) + + try { + await delay(safeDelayMs) + } catch (error) { + // Log error but continue - delay failure shouldn't break the save operation + console.warn(`Failed to apply write delay: ${error}`) + } + + const postDiagnostics = vscode.languages.getDiagnostics() + + const newProblems = await diagnosticsToProblemsString( + getNewDiagnostics(this.preDiagnostics, postDiagnostics), + [ + vscode.DiagnosticSeverity.Error, // only including errors since warnings can be distracting (if user wants to fix warnings they can use the @problems mention) + ], + this.cwd, + ) // Will be empty string if no errors. + + newProblemsMessage = + newProblems.length > 0 ? `\n\nNew problems detected after saving the file:\n${newProblems}` : "" + } + + // In file-based editing, there should be no user edits since we write directly + // But we check if the final content differs from what we intended to write + const normalizedNewContent = this.newContent.replace(/\r\n|\n/g, "\n") + const normalizedFinalContent = finalContent.replace(/\r\n|\n/g, "\n").trimEnd() + + if (normalizedFinalContent !== normalizedNewContent) { + // This shouldn't happen in normal file-based editing, but handle it just in case + const userEdits = formatResponse.createPrettyPatch( + this.relPath.toPosix(), + normalizedNewContent, + normalizedFinalContent, + ) + + this.newProblemsMessage = newProblemsMessage + this.userEdits = userEdits + + return { newProblemsMessage, userEdits, finalContent: normalizedFinalContent } + } else { + this.newProblemsMessage = newProblemsMessage + this.userEdits = undefined + + return { newProblemsMessage, userEdits: undefined, finalContent: normalizedFinalContent } + } + } + + /** + * Formats a standardized XML response for file write operations + * @param task The current task context for sending user feedback + * @param cwd Current working directory for path resolution + * @param isNewFile Whether this is a new file or an existing file being modified + * @returns Formatted message and say object for UI feedback + */ + async pushToolWriteResult(task: Task, cwd: string, isNewFile: boolean): Promise { + if (!this.relPath) { + throw new Error("No file path available in FileWriter") + } + + // Only send user_feedback_diff if userEdits exists (shouldn't happen in file-based editing) + if (this.userEdits) { + // Create say object for UI feedback + const say: ClineSayTool = { + tool: isNewFile ? "newFileCreated" : "editedExistingFile", + path: getReadablePath(cwd, this.relPath), + diff: this.userEdits, + } + + // Send the user feedback + await task.say("user_feedback_diff", JSON.stringify(say)) + } + + // Build XML response + const xmlObj = { + file_write_result: { + path: this.relPath, + operation: isNewFile ? "created" : "modified", + user_edits: this.userEdits ? this.userEdits : undefined, + problems: this.newProblemsMessage || undefined, + notice: { + i: [ + "You do not need to re-read the file, as you have seen all changes", + "Proceed with the task using these changes as the new baseline.", + ...(this.userEdits + ? [ + "If the user's edits have addressed part of the task or changed the requirements, adjust your approach accordingly.", + ] + : []), + ], + }, + }, + } + + const builder = new XMLBuilder({ + format: true, + indentBy: "", + suppressEmptyNode: true, + processEntities: false, + tagValueProcessor: (name, value) => { + if (typeof value === "string") { + // Only escape <, >, and & characters + return value.replace(/&/g, "&").replace(//g, ">") + } + return value + }, + attributeValueProcessor: (name, value) => { + if (typeof value === "string") { + // Only escape <, >, and & characters + return value.replace(/&/g, "&").replace(//g, ">") + } + return value + }, + }) + + return builder.build(xmlObj) + } + + /** + * Reverts changes (deletes new files, restores original content for modified files) + */ + async revertChanges(): Promise { + if (!this.relPath) { + return + } + + const fileExists = this.editType === "modify" + const absolutePath = path.resolve(this.cwd, this.relPath) + + if (!fileExists) { + // Delete the newly created file + try { + await fs.unlink(absolutePath) + } catch (error) { + // File might not exist, ignore error + } + + // Remove only the directories we created, in reverse order + for (let i = this.createdDirs.length - 1; i >= 0; i--) { + try { + await fs.rmdir(this.createdDirs[i]) + } catch (error) { + // Directory might not be empty or not exist, ignore error + } + } + } else { + // Restore original content + await fs.writeFile(absolutePath, this.stripAllBOMs(this.originalContent ?? ""), "utf-8") + } + + // Reset state + this.reset() + } + + /** + * Strips all BOM characters from input string + */ + private stripAllBOMs(input: string): string { + let result = input + let previous + + do { + previous = result + result = stripBom(result) + } while (result !== previous) + + return result + } + + /** + * Resets the FileWriter state + */ + async reset(): Promise { + this.editType = undefined + this.isEditing = false + this.originalContent = undefined + this.newContent = undefined + this.createdDirs = [] + this.relPath = undefined + this.preDiagnostics = [] + this.newProblemsMessage = undefined + this.userEdits = undefined + } + + async scrollToFirstDiff(): Promise { + // No-op for FileWriter, as it doesn't handle diffs + return + } +} diff --git a/src/integrations/editor/IEditingProvider.ts b/src/integrations/editor/IEditingProvider.ts new file mode 100644 index 0000000000..17a56d9a8f --- /dev/null +++ b/src/integrations/editor/IEditingProvider.ts @@ -0,0 +1,65 @@ +import { Task } from "../../core/task/Task" + +/** + * Interface for editing providers (DiffViewProvider, FileWriter, etc.) + * This allows tools to work with different editing strategies seamlessly + */ +export interface IEditingProvider { + // Properties to store the results of saveChanges + newProblemsMessage?: string + userEdits?: string + editType?: "create" | "modify" + isEditing: boolean + originalContent: string | undefined + + /** + * Prepares for editing the given relative path file + * @param relPath The relative file path to open/prepare for editing + */ + open(relPath: string): Promise + + /** + * Updates the content being edited + * @param content The content to apply + * @param isFinal Whether this is the final update + */ + update(content: string, isFinal: boolean): Promise + + /** + * Finalizes the changes and returns diagnostics information + * @param diagnosticsEnabled Whether to enable diagnostics (default: true) + * @param writeDelayMs Delay in milliseconds before writing changes (default: 1000) + */ + saveChanges( + diagnosticsEnabled: boolean, + writeDelayMs: number, + ): Promise<{ + newProblemsMessage: string | undefined + userEdits: string | undefined + finalContent: string | undefined + }> + + /** + * Formats a standardized XML response for file write operations + * @param task The current task context for sending user feedback + * @param cwd Current working directory for path resolution + * @param isNewFile Whether this is a new file or an existing file being modified + * @returns Formatted XML response message + */ + pushToolWriteResult(task: Task, cwd: string, isNewFile: boolean): Promise + + /** + * Reverts changes (cancels the editing operation) + */ + revertChanges(): Promise + + /** + * Resets the provider state + */ + reset(): Promise + + /** + * Scrolls to first diff (diff providers only, no-op for file providers) + */ + scrollToFirstDiff(): void +} diff --git a/src/integrations/editor/__tests__/EditingProviderFactory.spec.ts b/src/integrations/editor/__tests__/EditingProviderFactory.spec.ts new file mode 100644 index 0000000000..70ca66df70 --- /dev/null +++ b/src/integrations/editor/__tests__/EditingProviderFactory.spec.ts @@ -0,0 +1,55 @@ +import { vi, describe, it, expect, beforeEach, afterEach } from "vitest" + +import { EditingProviderFactory } from "../EditingProviderFactory" +import { DiffViewProvider } from "../DiffViewProvider" +import { FileWriter } from "../FileWriter" +import { Task } from "../../../core/task/Task" + +// Mock the providers +vi.mock("../DiffViewProvider", () => ({ + DiffViewProvider: vi.fn(), +})) + +vi.mock("../FileWriter", () => ({ + FileWriter: vi.fn(), +})) + +describe("EditingProviderFactory", () => { + const mockCwd = "/test/cwd" + const mockTask = {} as Task + + beforeEach(() => { + vi.clearAllMocks() + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + describe("createEditingProvider", () => { + it("should create FileWriter when fileBasedEditing is enabled", () => { + const provider = EditingProviderFactory.createEditingProvider(mockCwd, { fileBasedEditing: true }, mockTask) + + expect(FileWriter).toHaveBeenCalledWith(mockCwd) + expect(DiffViewProvider).not.toHaveBeenCalled() + }) + + it("should create DiffViewProvider when fileBasedEditing is disabled", () => { + const provider = EditingProviderFactory.createEditingProvider( + mockCwd, + { fileBasedEditing: false }, + mockTask, + ) + + expect(DiffViewProvider).toHaveBeenCalledWith(mockCwd, mockTask) + expect(FileWriter).not.toHaveBeenCalled() + }) + + it("should create DiffViewProvider when fileBasedEditing is undefined", () => { + const provider = EditingProviderFactory.createEditingProvider(mockCwd, undefined, mockTask) + + expect(DiffViewProvider).toHaveBeenCalledWith(mockCwd, mockTask) + expect(FileWriter).not.toHaveBeenCalled() + }) + }) +}) diff --git a/src/integrations/editor/__tests__/FileWriter.spec.ts b/src/integrations/editor/__tests__/FileWriter.spec.ts new file mode 100644 index 0000000000..fc539cae99 --- /dev/null +++ b/src/integrations/editor/__tests__/FileWriter.spec.ts @@ -0,0 +1,207 @@ +import { vi, describe, it, expect, beforeEach, afterEach } from "vitest" +import * as vscode from "vscode" +import * as fs from "fs/promises" +import * as path from "path" + +import { FileWriter } from "../FileWriter" +import { Task } from "../../../core/task/Task" + +// Mock VSCode API +vi.mock("vscode", () => ({ + workspace: { + getConfiguration: vi.fn(() => ({ + get: vi.fn(), + })), + }, + languages: { + getDiagnostics: vi.fn(() => []), + }, + DiagnosticSeverity: { + Error: 0, + }, +})) + +// Mock fs module +vi.mock("fs/promises", () => ({ + readFile: vi.fn(), + writeFile: vi.fn(), + unlink: vi.fn(), + rmdir: vi.fn(), +})) + +// Mock other dependencies +vi.mock("../../../utils/fs", () => ({ + createDirectoriesForFile: vi.fn(() => Promise.resolve([])), +})) + +vi.mock("../../diagnostics", () => ({ + diagnosticsToProblemsString: vi.fn(() => Promise.resolve("")), + getNewDiagnostics: vi.fn(() => []), +})) + +vi.mock("../../../utils/path", () => ({ + getReadablePath: vi.fn((cwd, relPath) => relPath), +})) + +vi.mock("../../../core/prompts/responses", () => ({ + formatResponse: { + createPrettyPatch: vi.fn(() => "mock-diff"), + }, +})) + +vi.mock("strip-bom", () => ({ + default: vi.fn((content) => content), +})) + +describe("FileWriter", () => { + let fileWriter: FileWriter + const mockCwd = "/test/cwd" + const mockTask = {} as Task + + beforeEach(() => { + vi.clearAllMocks() + fileWriter = new FileWriter(mockCwd) + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + describe("open", () => { + it("should open an existing file for modification", async () => { + const mockContent = "existing content" + vi.mocked(fs.readFile).mockResolvedValue(mockContent) + + await fileWriter.open("test.txt") + + expect(fileWriter.editType).toBe("modify") + expect(fileWriter.originalContent).toBe(mockContent) + expect(fileWriter.isEditing).toBe(true) + }) + + it("should open a new file for creation", async () => { + vi.mocked(fs.readFile).mockRejectedValue(new Error("File not found")) + + await fileWriter.open("new-file.txt") + + expect(fileWriter.editType).toBe("create") + expect(fileWriter.originalContent).toBe("") + expect(fileWriter.isEditing).toBe(true) + }) + + it("should handle viewColumn parameter (ignored)", async () => { + vi.mocked(fs.readFile).mockResolvedValue("content") + + await expect(fileWriter.open("test.txt")).resolves.toBeUndefined() + }) + }) + + describe("update", () => { + beforeEach(async () => { + vi.mocked(fs.readFile).mockResolvedValue("original content") + await fileWriter.open("test.txt") + }) + + it("should write final content to file", async () => { + const content = "new content" + + await fileWriter.update(content, true) + + expect(fs.writeFile).toHaveBeenCalledWith(path.resolve(mockCwd, "test.txt"), content, "utf-8") + }) + + it("should preserve empty last line if original content had one", async () => { + fileWriter.originalContent = "content\n" + const content = "new content" + + await fileWriter.update(content, true) + + expect(fs.writeFile).toHaveBeenCalledWith(path.resolve(mockCwd, "test.txt"), "new content\n", "utf-8") + }) + + it("should not write to file if not final", async () => { + await fileWriter.update("content", false) + + expect(fs.writeFile).not.toHaveBeenCalled() + }) + }) + + describe("saveChanges", () => { + beforeEach(async () => { + vi.mocked(fs.readFile).mockResolvedValue("original content") + await fileWriter.open("test.txt") + await fileWriter.update("new content", true) + }) + + it("should return save results", async () => { + vi.mocked(fs.readFile).mockResolvedValue("new content") + + const result = await fileWriter.saveChanges() + + expect(result).toEqual({ + newProblemsMessage: "", + userEdits: undefined, + finalContent: "new content", + }) + }) + }) + + describe("pushToolWriteResult", () => { + beforeEach(async () => { + vi.mocked(fs.readFile).mockResolvedValue("content") + await fileWriter.open("test.txt") + }) + + it("should return formatted XML response", async () => { + const result = await fileWriter.pushToolWriteResult(mockTask, mockCwd, false) + + expect(result).toContain("") + expect(result).toContain("test.txt") + expect(result).toContain("modified") + }) + + it("should handle new file creation", async () => { + const result = await fileWriter.pushToolWriteResult(mockTask, mockCwd, true) + + expect(result).toContain("created") + }) + }) + + describe("revertChanges", () => { + it("should delete new file and directories", async () => { + vi.mocked(fs.readFile).mockRejectedValue(new Error("File not found")) + await fileWriter.open("new-file.txt") + + await fileWriter.revertChanges() + + expect(fs.unlink).toHaveBeenCalled() + }) + + it("should restore original content for existing file", async () => { + const originalContent = "original content" + vi.mocked(fs.readFile).mockResolvedValue(originalContent) + await fileWriter.open("existing-file.txt") + + await fileWriter.revertChanges() + + expect(fs.writeFile).toHaveBeenCalledWith( + path.resolve(mockCwd, "existing-file.txt"), + originalContent, + "utf-8", + ) + }) + }) + + describe("reset", () => { + it("should reset all state", async () => { + vi.mocked(fs.readFile).mockResolvedValue("content") + await fileWriter.open("test.txt") + + await fileWriter.reset() + + expect(fileWriter.editType).toBeUndefined() + expect(fileWriter.isEditing).toBe(false) + expect(fileWriter.originalContent).toBeUndefined() + }) + }) +}) diff --git a/src/shared/__tests__/experiments.spec.ts b/src/shared/__tests__/experiments.spec.ts index 4a8f06d62a..dc64996140 100644 --- a/src/shared/__tests__/experiments.spec.ts +++ b/src/shared/__tests__/experiments.spec.ts @@ -23,11 +23,21 @@ describe("experiments", () => { }) }) + describe("FILE_BASED_EDITING", () => { + it("is configured correctly", () => { + expect(EXPERIMENT_IDS.FILE_BASED_EDITING).toBe("fileBasedEditing") + expect(experimentConfigsMap.FILE_BASED_EDITING).toMatchObject({ + enabled: false, + }) + }) + }) + describe("isEnabled", () => { it("returns false when POWER_STEERING experiment is not enabled", () => { const experiments: Record = { powerSteering: false, multiFileApplyDiff: false, + fileBasedEditing: false, } expect(Experiments.isEnabled(experiments, EXPERIMENT_IDS.POWER_STEERING)).toBe(false) }) @@ -36,14 +46,25 @@ describe("experiments", () => { const experiments: Record = { powerSteering: true, multiFileApplyDiff: false, + fileBasedEditing: false, } expect(Experiments.isEnabled(experiments, EXPERIMENT_IDS.POWER_STEERING)).toBe(true) }) + it("returns true when experiment FILE_BASED_EDITING is enabled", () => { + const experiments: Record = { + powerSteering: false, + multiFileApplyDiff: false, + fileBasedEditing: true, + } + expect(Experiments.isEnabled(experiments, EXPERIMENT_IDS.FILE_BASED_EDITING)).toBe(true) + }) + it("returns false when experiment is not present", () => { const experiments: Record = { powerSteering: false, multiFileApplyDiff: false, + fileBasedEditing: false, } expect(Experiments.isEnabled(experiments, EXPERIMENT_IDS.POWER_STEERING)).toBe(false) }) diff --git a/src/shared/experiments.ts b/src/shared/experiments.ts index 1edadf654f..67f9b5235f 100644 --- a/src/shared/experiments.ts +++ b/src/shared/experiments.ts @@ -3,6 +3,7 @@ import type { AssertEqual, Equals, Keys, Values, ExperimentId, Experiments } fro export const EXPERIMENT_IDS = { MULTI_FILE_APPLY_DIFF: "multiFileApplyDiff", POWER_STEERING: "powerSteering", + FILE_BASED_EDITING: "fileBasedEditing", } as const satisfies Record type _AssertExperimentIds = AssertEqual>> @@ -16,6 +17,7 @@ interface ExperimentConfig { export const experimentConfigsMap: Record = { MULTI_FILE_APPLY_DIFF: { enabled: false }, POWER_STEERING: { enabled: false }, + FILE_BASED_EDITING: { enabled: false }, } export const experimentDefault = Object.fromEntries( diff --git a/webview-ui/src/components/settings/ExperimentalSettings.tsx b/webview-ui/src/components/settings/ExperimentalSettings.tsx index 53801232ec..34b11950ff 100644 --- a/webview-ui/src/components/settings/ExperimentalSettings.tsx +++ b/webview-ui/src/components/settings/ExperimentalSettings.tsx @@ -36,6 +36,7 @@ export const ExperimentalSettings = ({
+ {/* Existing experimental features */} {Object.entries(experimentConfigsMap) .filter(([key]) => key in EXPERIMENT_IDS) .map((config) => { diff --git a/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx b/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx index 1e5867d3fc..437b91e98e 100644 --- a/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx +++ b/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx @@ -226,6 +226,7 @@ describe("mergeExtensionState", () => { disableCompletionCommand: false, concurrentFileReads: true, multiFileApplyDiff: true, + fileBasedEditing: true, } as Record, } @@ -242,6 +243,7 @@ describe("mergeExtensionState", () => { disableCompletionCommand: false, concurrentFileReads: true, multiFileApplyDiff: true, + fileBasedEditing: true, }) }) }) diff --git a/webview-ui/src/i18n/locales/en/settings.json b/webview-ui/src/i18n/locales/en/settings.json index cfd5b04286..69d0448f9f 100644 --- a/webview-ui/src/i18n/locales/en/settings.json +++ b/webview-ui/src/i18n/locales/en/settings.json @@ -648,6 +648,10 @@ "MULTI_FILE_APPLY_DIFF": { "name": "Enable concurrent file edits", "description": "When enabled, Roo can edit multiple files in a single request. When disabled, Roo must edit files one at a time. Disabling this can help when working with less capable models or when you want more control over file modifications." + }, + "FILE_BASED_EDITING": { + "name": "Enable file-based editing", + "description": "When enabled, Roo can edit files directly using file-based operations. This may improve reliability for large files and advanced workflows, but could cause unexpected behavior. Only enable if you understand the risks." } }, "promptCaching": {