From a119ca4586e8dee604d7ed99f28adb5b4577bbf3 Mon Sep 17 00:00:00 2001 From: Eric Wheeler Date: Thu, 15 May 2025 22:10:34 -0700 Subject: [PATCH 1/2] refactor: Add pushToolWriteResult method to DiffViewProvider Previously, each tool file contained duplicate code for formatting file write responses and conditionally sending user_feedback_diff messages. This led to inconsistent implementations and made changes difficult to maintain. This refactoring centralizes the response formatting and messaging logic in the DiffViewProvider class, which now: - Stores results from saveChanges() in class properties - Only sends user_feedback_diff when user edits exist - Configures XMLBuilder with no indentation for cleaner output Tool files now make a single method call instead of duplicating logic. Fixes: #3647 Signed-off-by: Eric Wheeler --- src/core/tools/applyDiffTool.ts | 36 ++------- src/core/tools/insertContentTool.ts | 35 ++------- src/core/tools/searchAndReplaceTool.ts | 35 ++------- src/core/tools/writeToFileTool.ts | 32 ++------ src/integrations/editor/DiffViewProvider.ts | 82 ++++++++++++++++++++- 5 files changed, 111 insertions(+), 109 deletions(-) diff --git a/src/core/tools/applyDiffTool.ts b/src/core/tools/applyDiffTool.ts index 9fbe1e1de1..dc0f277564 100644 --- a/src/core/tools/applyDiffTool.ts +++ b/src/core/tools/applyDiffTool.ts @@ -8,7 +8,6 @@ import { ToolUse, RemoveClosingTag } from "../../shared/tools" import { formatResponse } from "../prompts/responses" import { AskApproval, HandleError, PushToolResult } from "../../shared/tools" import { fileExistsAtPath } from "../../utils/fs" -import { addLineNumbers } from "../../integrations/misc/extract-text" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { telemetryService } from "../../services/telemetry/TelemetryService" import { unescapeHtmlEntities } from "../../utils/text-normalization" @@ -163,7 +162,8 @@ export async function applyDiffTool( return } - const { newProblemsMessage, userEdits, finalContent } = await cline.diffViewProvider.saveChanges() + // Call saveChanges to update the DiffViewProvider properties + await cline.diffViewProvider.saveChanges() // Track file edit operation if (relPath) { @@ -178,33 +178,13 @@ export async function applyDiffTool( partFailHint = `But unable to apply all diff parts to file: ${absolutePath}, silently use tool to check newest file version and re-apply diffs\n` } - if (userEdits) { - await cline.say( - "user_feedback_diff", - JSON.stringify({ - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(cline.cwd, relPath), - diff: userEdits, - } satisfies ClineSayTool), - ) - - pushToolResult( - `The user made the following updates to your content:\n\n${userEdits}\n\n` + - partFailHint + - `The updated content, which includes both your original modifications and the user's edits, has been successfully saved to ${relPath.toPosix()}. Here is the full, updated content of the file, including line numbers:\n\n` + - `\n${addLineNumbers( - finalContent || "", - )}\n\n\n` + - `Please note:\n` + - `1. You do not need to re-write the file with these changes, as they have already been applied.\n` + - `2. Proceed with the task using this updated file content as the new baseline.\n` + - `3. If the user's edits have addressed part of the task or changed the requirements, adjust your approach accordingly.` + - `${newProblemsMessage}`, - ) + // Get the formatted response message + const message = await cline.diffViewProvider.pushToolWriteResult(cline, cline.cwd, !fileExists) + + if (partFailHint) { + pushToolResult(partFailHint + message) } else { - pushToolResult( - `Changes successfully applied to ${relPath.toPosix()}:\n\n${newProblemsMessage}\n` + partFailHint, - ) + pushToolResult(message) } await cline.diffViewProvider.reset() diff --git a/src/core/tools/insertContentTool.ts b/src/core/tools/insertContentTool.ts index bcb217326a..bf4c6dfe4f 100644 --- a/src/core/tools/insertContentTool.ts +++ b/src/core/tools/insertContentTool.ts @@ -128,7 +128,8 @@ export async function insertContentTool( return } - const { newProblemsMessage, userEdits, finalContent } = await cline.diffViewProvider.saveChanges() + // Call saveChanges to update the DiffViewProvider properties + await cline.diffViewProvider.saveChanges() // Track file edit operation if (relPath) { @@ -137,34 +138,14 @@ export async function insertContentTool( cline.didEditFile = true - if (!userEdits) { - pushToolResult( - `The content was successfully inserted in ${relPath.toPosix()} at line ${lineNumber}.${newProblemsMessage}`, - ) - await cline.diffViewProvider.reset() - return - } - - await cline.say( - "user_feedback_diff", - JSON.stringify({ - tool: "insertContent", - path: getReadablePath(cline.cwd, relPath), - diff: userEdits, - lineNumber: lineNumber, - } satisfies ClineSayTool), + // Get the formatted response message + const message = await cline.diffViewProvider.pushToolWriteResult( + cline, + cline.cwd, + false, // Always false for insert_content ) - pushToolResult( - `The user made the following updates to your content:\n\n${userEdits}\n\n` + - `The updated content has been successfully saved to ${relPath.toPosix()}. Here is the full, updated content of the file:\n\n` + - `\n${finalContent}\n\n\n` + - `Please note:\n` + - `1. You do not need to re-write the file with these changes, as they have already been applied.\n` + - `2. Proceed with the task using this updated file content as the new baseline.\n` + - `3. If the user's edits have addressed part of the task or changed the requirements, adjust your approach accordingly.` + - `${newProblemsMessage}`, - ) + pushToolResult(message) await cline.diffViewProvider.reset() } catch (error) { diff --git a/src/core/tools/searchAndReplaceTool.ts b/src/core/tools/searchAndReplaceTool.ts index 417e2046df..4921bafff9 100644 --- a/src/core/tools/searchAndReplaceTool.ts +++ b/src/core/tools/searchAndReplaceTool.ts @@ -211,7 +211,8 @@ export async function searchAndReplaceTool( return } - const { newProblemsMessage, userEdits, finalContent } = await cline.diffViewProvider.saveChanges() + // Call saveChanges to update the DiffViewProvider properties + await cline.diffViewProvider.saveChanges() // Track file edit operation if (relPath) { @@ -220,34 +221,14 @@ export async function searchAndReplaceTool( cline.didEditFile = true - if (!userEdits) { - pushToolResult(`The content was successfully replaced in ${relPath}.${newProblemsMessage}`) - await cline.diffViewProvider.reset() - return - } - - await cline.say( - "user_feedback_diff", - JSON.stringify({ - tool: "appliedDiff", - path: getReadablePath(cline.cwd, relPath), - diff: userEdits, - } satisfies ClineSayTool), + // Get the formatted response message + const message = await cline.diffViewProvider.pushToolWriteResult( + cline, + cline.cwd, + false, // Always false for search_and_replace ) - // Format and send response with user's updates - const resultMessage = [ - `The user made the following updates to your content:\n\n${userEdits}\n\n`, - `The updated content has been successfully saved to ${validRelPath.toPosix()}. Here is the full, updated content of the file:\n\n`, - `\n${finalContent}\n\n\n`, - `Please note:\n`, - `1. You do not need to re-write the file with these changes, as they have already been applied.\n`, - `2. Proceed with the task using the updated file content as the new baseline.\n`, - `3. If the user's edits have addressed part of the task or changed the requirements, adjust your approach accordingly.`, - newProblemsMessage, - ].join("") - - pushToolResult(resultMessage) + pushToolResult(message) // Record successful tool usage and cleanup cline.recordToolUsage("search_and_replace") diff --git a/src/core/tools/writeToFileTool.ts b/src/core/tools/writeToFileTool.ts index 2c37f95b74..c58d807abe 100644 --- a/src/core/tools/writeToFileTool.ts +++ b/src/core/tools/writeToFileTool.ts @@ -8,7 +8,7 @@ import { formatResponse } from "../prompts/responses" import { ToolUse, AskApproval, HandleError, PushToolResult, RemoveClosingTag } from "../../shared/tools" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath } from "../../utils/fs" -import { addLineNumbers, stripLineNumbers, everyLineHasLineNumbers } from "../../integrations/misc/extract-text" +import { stripLineNumbers, everyLineHasLineNumbers } from "../../integrations/misc/extract-text" import { getReadablePath } from "../../utils/path" import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { detectCodeOmission } from "../../integrations/editor/detect-omission" @@ -208,7 +208,8 @@ export async function writeToFileTool( return } - const { newProblemsMessage, userEdits, finalContent } = await cline.diffViewProvider.saveChanges() + // Call saveChanges to update the DiffViewProvider properties + await cline.diffViewProvider.saveChanges() // Track file edit operation if (relPath) { @@ -217,31 +218,10 @@ export async function writeToFileTool( cline.didEditFile = true // used to determine if we should wait for busy terminal to update before sending api request - if (userEdits) { - await cline.say( - "user_feedback_diff", - JSON.stringify({ - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(cline.cwd, relPath), - diff: userEdits, - } satisfies ClineSayTool), - ) + // Get the formatted response message + const message = await cline.diffViewProvider.pushToolWriteResult(cline, cline.cwd, !fileExists) - pushToolResult( - `The user made the following updates to your content:\n\n${userEdits}\n\n` + - `The updated content, which includes both your original modifications and the user's edits, has been successfully saved to ${relPath.toPosix()}. Here is the full, updated content of the file, including line numbers:\n\n` + - `\n${addLineNumbers( - finalContent || "", - )}\n\n\n` + - `Please note:\n` + - `1. You do not need to re-write the file with these changes, as they have already been applied.\n` + - `2. Proceed with the task using this updated file content as the new baseline.\n` + - `3. If the user's edits have addressed part of the task or changed the requirements, adjust your approach accordingly.` + - `${newProblemsMessage}`, - ) - } else { - pushToolResult(`The content was successfully saved to ${relPath.toPosix()}.${newProblemsMessage}`) - } + pushToolResult(message) await cline.diffViewProvider.reset() diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index b6a5b9ac3b..9dc2a2aa77 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -3,11 +3,14 @@ import * as path from "path" import * as fs from "fs/promises" import * as diff from "diff" import stripBom from "strip-bom" +import { XMLBuilder } from "fast-xml-parser" import { createDirectoriesForFile } from "../../utils/fs" -import { arePathsEqual } from "../../utils/path" +import { arePathsEqual, 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 { DecorationController } from "./DecorationController" @@ -15,6 +18,9 @@ export const DIFF_VIEW_URI_SCHEME = "cline-diff" // TODO: https://github.com/cline/cline/pull/3354 export class DiffViewProvider { + // Properties to store the results of saveChanges + newProblemsMessage?: string + userEdits?: string editType?: "create" | "modify" isEditing = false originalContent: string | undefined @@ -235,13 +241,87 @@ export class DiffViewProvider { normalizedEditedContent, ) + // Store the results as class properties for formatFileWriteResponse to use + this.newProblemsMessage = newProblemsMessage + this.userEdits = userEdits + return { newProblemsMessage, userEdits, finalContent: normalizedEditedContent } } else { // No changes to Roo's edits. + // Store the results as class properties for formatFileWriteResponse to use + this.newProblemsMessage = newProblemsMessage + this.userEdits = undefined + return { newProblemsMessage, userEdits: undefined, finalContent: normalizedEditedContent } } } + /** + * Formats a standardized XML response for file write operations + * + * @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 DiffViewProvider") + } + + // Only send user_feedback_diff if userEdits exists + 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 in the diff above.", + "Proceed with the task using these changes as the new baseline.", + "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) + } + async revertChanges(): Promise { if (!this.relPath || !this.activeDiffEditor) { return From c91d5642bbb20afe7a825424c50df969bfd55f6f Mon Sep 17 00:00:00 2001 From: Eric Wheeler Date: Thu, 15 May 2025 22:20:33 -0700 Subject: [PATCH 2/2] fix: conditionally show user edits message in file write response Make the 'If the user's edits have addressed part of the task...' message conditional based on whether there are actual user edits. This prevents showing irrelevant guidance when no user edits were made to the file. Signed-off-by: Eric Wheeler --- src/integrations/editor/DiffViewProvider.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 9dc2a2aa77..15021372fd 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -290,9 +290,13 @@ export class DiffViewProvider { problems: this.newProblemsMessage || undefined, notice: { i: [ - "You do not need to re-read the file, as you have seen all changes in the diff above.", + "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.", - "If the user's edits have addressed part of the task or changed the requirements, adjust your approach accordingly.", + ...(this.userEdits + ? [ + "If the user's edits have addressed part of the task or changed the requirements, adjust your approach accordingly.", + ] + : []), ], }, },