From 8ae4296e40dd4b32d42aea38d6d74c3e6dfa0ee9 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Tue, 29 Jul 2025 16:12:20 -0500 Subject: [PATCH 1/2] fix: show diff view before approval when PREVENT_FOCUS_DISRUPTION is disabled - Fixed insertContentTool to show diff view before asking for approval - Fixed multiApplyDiffTool for single file operations to show diff before approval - Fixed searchAndReplaceTool to show diff view before asking for approval - Ensures users can see changes before approving them when experiment is off This fixes the issue where users had to approve changes blindly without seeing the diff first. --- src/core/tools/insertContentTool.ts | 69 ++++++------ src/core/tools/multiApplyDiffTool.ts | 144 ++++++++++++++++--------- src/core/tools/searchAndReplaceTool.ts | 62 ++++++----- 3 files changed, 169 insertions(+), 106 deletions(-) diff --git a/src/core/tools/insertContentTool.ts b/src/core/tools/insertContentTool.ts index a42eb6f6f9..30340a440a 100644 --- a/src/core/tools/insertContentTool.ts +++ b/src/core/tools/insertContentTool.ts @@ -136,42 +136,51 @@ export async function insertContentTool( approvalContent = updatedContent } - const completeMessage = JSON.stringify({ - ...sharedMessageProps, - diff, - content: approvalContent, - lineNumber: lineNumber, - isProtected: isWriteProtected, - } satisfies ClineSayTool) - - const didApprove = await cline - .ask("tool", completeMessage, isWriteProtected) - .then((response) => response.response === "yesButtonClicked") - - if (!didApprove) { - if (!isPreventFocusDisruptionEnabled) { - await cline.diffViewProvider.revertChanges() + if (isPreventFocusDisruptionEnabled) { + // Direct file write without diff view + const completeMessage = JSON.stringify({ + ...sharedMessageProps, + diff, + content: approvalContent, + lineNumber: lineNumber, + isProtected: isWriteProtected, + } satisfies ClineSayTool) + + const didApprove = await cline + .ask("tool", completeMessage, isWriteProtected) + .then((response) => response.response === "yesButtonClicked") + + if (!didApprove) { + pushToolResult("Changes were rejected by the user.") + return } - pushToolResult("Changes were rejected by the user.") - return - } - if (isPreventFocusDisruptionEnabled) { - // Direct file write without diff view or opening the file + // Save directly without showing diff view or opening the file await cline.diffViewProvider.saveDirectly(relPath, updatedContent, false, diagnosticsEnabled, writeDelayMs) } else { // Original behavior with diff view - // Show changes in diff view - if (!cline.diffViewProvider.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 delay(200) - } - + // Show diff view BEFORE asking for approval + await cline.diffViewProvider.open(relPath) await cline.diffViewProvider.update(updatedContent, true) + cline.diffViewProvider.scrollToFirstDiff() + + const completeMessage = JSON.stringify({ + ...sharedMessageProps, + diff, + content: approvalContent, + lineNumber: lineNumber, + isProtected: isWriteProtected, + } satisfies ClineSayTool) + + const didApprove = await cline + .ask("tool", completeMessage, isWriteProtected) + .then((response) => response.response === "yesButtonClicked") + + if (!didApprove) { + await cline.diffViewProvider.revertChanges() + pushToolResult("Changes were rejected by the user.") + return + } // Call saveChanges to update the DiffViewProvider properties await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) diff --git a/src/core/tools/multiApplyDiffTool.ts b/src/core/tools/multiApplyDiffTool.ts index 80f4c9a054..ac9ace0527 100644 --- a/src/core/tools/multiApplyDiffTool.ts +++ b/src/core/tools/multiApplyDiffTool.ts @@ -525,61 +525,109 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""} isProtected: isWriteProtected, } - // If single file, ask for approval + // If single file, handle based on PREVENT_FOCUS_DISRUPTION setting let didApprove = true if (operationsToApprove.length === 1) { - const diffContents = diffItems.map((item) => item.content).join("\n\n") - const operationMessage = JSON.stringify({ - ...sharedMessageProps, - diff: diffContents, - } satisfies ClineSayTool) - - let toolProgressStatus - - if (cline.diffStrategy && cline.diffStrategy.getProgressStatus) { - toolProgressStatus = cline.diffStrategy.getProgressStatus( - { - ...block, - params: { ...block.params, diff: diffContents }, - }, - { success: true }, + if (isPreventFocusDisruptionEnabled) { + // Direct approval without showing diff view + const diffContents = diffItems.map((item) => item.content).join("\n\n") + const operationMessage = JSON.stringify({ + ...sharedMessageProps, + diff: diffContents, + } satisfies ClineSayTool) + + let toolProgressStatus + + if (cline.diffStrategy && cline.diffStrategy.getProgressStatus) { + toolProgressStatus = cline.diffStrategy.getProgressStatus( + { + ...block, + params: { ...block.params, diff: diffContents }, + }, + { success: true }, + ) + } + + // Check if file is write-protected + const isWriteProtected = cline.rooProtectedController?.isWriteProtected(relPath) || false + didApprove = await askApproval("tool", operationMessage, toolProgressStatus, isWriteProtected) + + if (!didApprove) { + results.push(`Changes to ${relPath} were not approved by user`) + continue + } + + // Direct file write without diff view or opening the file + cline.diffViewProvider.editType = "modify" + cline.diffViewProvider.originalContent = await fs.readFile(absolutePath, "utf-8") + await cline.diffViewProvider.saveDirectly( + relPath, + originalContent!, + false, + diagnosticsEnabled, + writeDelayMs, ) - } + } else { + // Show diff view BEFORE asking for approval + cline.diffViewProvider.editType = "modify" + await cline.diffViewProvider.open(relPath) + await cline.diffViewProvider.update(originalContent!, true) + cline.diffViewProvider.scrollToFirstDiff() + + const diffContents = diffItems.map((item) => item.content).join("\n\n") + const operationMessage = JSON.stringify({ + ...sharedMessageProps, + diff: diffContents, + } satisfies ClineSayTool) + + let toolProgressStatus + + if (cline.diffStrategy && cline.diffStrategy.getProgressStatus) { + toolProgressStatus = cline.diffStrategy.getProgressStatus( + { + ...block, + params: { ...block.params, diff: diffContents }, + }, + { success: true }, + ) + } - // Check if file is write-protected - const isWriteProtected = cline.rooProtectedController?.isWriteProtected(relPath) || false - didApprove = await askApproval("tool", operationMessage, toolProgressStatus, isWriteProtected) - } + // Check if file is write-protected + const isWriteProtected = cline.rooProtectedController?.isWriteProtected(relPath) || false + didApprove = await askApproval("tool", operationMessage, toolProgressStatus, isWriteProtected) - if (!didApprove) { - if (!isPreventFocusDisruptionEnabled) { - await cline.diffViewProvider.revertChanges() - } - results.push(`Changes to ${relPath} were not approved by user`) - continue - } + if (!didApprove) { + await cline.diffViewProvider.revertChanges() + results.push(`Changes to ${relPath} were not approved by user`) + continue + } - if (isPreventFocusDisruptionEnabled) { - // Direct file write without diff view or opening the file - cline.diffViewProvider.editType = "modify" - cline.diffViewProvider.originalContent = await fs.readFile(absolutePath, "utf-8") - await cline.diffViewProvider.saveDirectly( - relPath, - originalContent!, - false, - diagnosticsEnabled, - writeDelayMs, - ) + // Call saveChanges to update the DiffViewProvider properties + await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + } } else { - // Original behavior with diff view - // 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() - - // Call saveChanges to update the DiffViewProvider properties - await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + // Batch operations - already approved above + if (isPreventFocusDisruptionEnabled) { + // Direct file write without diff view or opening the file + cline.diffViewProvider.editType = "modify" + cline.diffViewProvider.originalContent = await fs.readFile(absolutePath, "utf-8") + await cline.diffViewProvider.saveDirectly( + relPath, + originalContent!, + false, + diagnosticsEnabled, + writeDelayMs, + ) + } else { + // Original behavior with diff view + cline.diffViewProvider.editType = "modify" + await cline.diffViewProvider.open(relPath) + await cline.diffViewProvider.update(originalContent!, true) + cline.diffViewProvider.scrollToFirstDiff() + + // Call saveChanges to update the DiffViewProvider properties + await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + } } // Track file edit operation diff --git a/src/core/tools/searchAndReplaceTool.ts b/src/core/tools/searchAndReplaceTool.ts index 09b644f931..c180acdbce 100644 --- a/src/core/tools/searchAndReplaceTool.ts +++ b/src/core/tools/searchAndReplaceTool.ts @@ -210,40 +210,46 @@ export async function searchAndReplaceTool( EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, ) - // Request user approval for changes - const completeMessage = JSON.stringify({ - ...sharedMessageProps, - diff, - isProtected: isWriteProtected, - } satisfies ClineSayTool) - const didApprove = await cline - .ask("tool", completeMessage, isWriteProtected) - .then((response) => response.response === "yesButtonClicked") - - if (!didApprove) { - if (!isPreventFocusDisruptionEnabled) { - await cline.diffViewProvider.revertChanges() + if (isPreventFocusDisruptionEnabled) { + // Direct approval without showing diff view + const completeMessage = JSON.stringify({ + ...sharedMessageProps, + diff, + isProtected: isWriteProtected, + } satisfies ClineSayTool) + const didApprove = await cline + .ask("tool", completeMessage, isWriteProtected) + .then((response) => response.response === "yesButtonClicked") + + if (!didApprove) { + pushToolResult("Changes were rejected by the user.") + await cline.diffViewProvider.reset() + return } - pushToolResult("Changes were rejected by the user.") - await cline.diffViewProvider.reset() - return - } - if (isPreventFocusDisruptionEnabled) { // Direct file write without diff view or opening the file await cline.diffViewProvider.saveDirectly(validRelPath, newContent, false, diagnosticsEnabled, writeDelayMs) } else { - // Original behavior with diff view - // Show changes in diff view - if (!cline.diffViewProvider.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 delay(200) - } - + // Show diff view BEFORE asking for approval + await cline.diffViewProvider.open(validRelPath) await cline.diffViewProvider.update(newContent, true) + cline.diffViewProvider.scrollToFirstDiff() + + const completeMessage = JSON.stringify({ + ...sharedMessageProps, + diff, + isProtected: isWriteProtected, + } satisfies ClineSayTool) + const didApprove = await cline + .ask("tool", completeMessage, isWriteProtected) + .then((response) => response.response === "yesButtonClicked") + + if (!didApprove) { + await cline.diffViewProvider.revertChanges() + pushToolResult("Changes were rejected by the user.") + await cline.diffViewProvider.reset() + return + } // Call saveChanges to update the DiffViewProvider properties await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) From b38c905404771bc303567bb4e2792374c0c79cd6 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Tue, 29 Jul 2025 16:26:19 -0500 Subject: [PATCH 2/2] refactor: reduce code duplication in file editing tools - Consolidated approval flow logic in searchAndReplaceTool, insertContentTool, and multiApplyDiffTool - Moved common code outside conditional blocks - Reduced code duplication while maintaining the same functionality - Makes the code more maintainable and easier to understand --- src/core/tools/insertContentTool.ts | 66 +++++++--------- src/core/tools/multiApplyDiffTool.ts | 103 ++++++++++--------------- src/core/tools/searchAndReplaceTool.ts | 57 ++++++-------- 3 files changed, 94 insertions(+), 132 deletions(-) diff --git a/src/core/tools/insertContentTool.ts b/src/core/tools/insertContentTool.ts index 30340a440a..b5e85dea30 100644 --- a/src/core/tools/insertContentTool.ts +++ b/src/core/tools/insertContentTool.ts @@ -136,52 +136,42 @@ export async function insertContentTool( approvalContent = updatedContent } - if (isPreventFocusDisruptionEnabled) { - // Direct file write without diff view - const completeMessage = JSON.stringify({ - ...sharedMessageProps, - diff, - content: approvalContent, - lineNumber: lineNumber, - isProtected: isWriteProtected, - } satisfies ClineSayTool) - - const didApprove = await cline - .ask("tool", completeMessage, isWriteProtected) - .then((response) => response.response === "yesButtonClicked") - - if (!didApprove) { - pushToolResult("Changes were rejected by the user.") - return - } - - // Save directly without showing diff view or opening the file - await cline.diffViewProvider.saveDirectly(relPath, updatedContent, false, diagnosticsEnabled, writeDelayMs) - } else { - // Original behavior with diff view - // Show diff view BEFORE asking for approval + // Prepare the approval message (same for both flows) + const completeMessage = JSON.stringify({ + ...sharedMessageProps, + diff, + content: approvalContent, + lineNumber: lineNumber, + isProtected: isWriteProtected, + } satisfies ClineSayTool) + + // Show diff view if focus disruption prevention is disabled + if (!isPreventFocusDisruptionEnabled) { await cline.diffViewProvider.open(relPath) await cline.diffViewProvider.update(updatedContent, true) cline.diffViewProvider.scrollToFirstDiff() + } - const completeMessage = JSON.stringify({ - ...sharedMessageProps, - diff, - content: approvalContent, - lineNumber: lineNumber, - isProtected: isWriteProtected, - } satisfies ClineSayTool) - - const didApprove = await cline - .ask("tool", completeMessage, isWriteProtected) - .then((response) => response.response === "yesButtonClicked") + // Ask for approval (same for both flows) + const didApprove = await cline + .ask("tool", completeMessage, isWriteProtected) + .then((response) => response.response === "yesButtonClicked") - if (!didApprove) { + if (!didApprove) { + // Revert changes if diff view was shown + if (!isPreventFocusDisruptionEnabled) { await cline.diffViewProvider.revertChanges() - pushToolResult("Changes were rejected by the user.") - return } + pushToolResult("Changes were rejected by the user.") + await cline.diffViewProvider.reset() + return + } + // Save the changes + if (isPreventFocusDisruptionEnabled) { + // Direct file write without diff view or opening the file + await cline.diffViewProvider.saveDirectly(relPath, updatedContent, false, diagnosticsEnabled, writeDelayMs) + } else { // Call saveChanges to update the DiffViewProvider properties await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } diff --git a/src/core/tools/multiApplyDiffTool.ts b/src/core/tools/multiApplyDiffTool.ts index ac9ace0527..db514d2b64 100644 --- a/src/core/tools/multiApplyDiffTool.ts +++ b/src/core/tools/multiApplyDiffTool.ts @@ -528,38 +528,53 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""} // If single file, handle based on PREVENT_FOCUS_DISRUPTION setting let didApprove = true if (operationsToApprove.length === 1) { - if (isPreventFocusDisruptionEnabled) { - // Direct approval without showing diff view - const diffContents = diffItems.map((item) => item.content).join("\n\n") - const operationMessage = JSON.stringify({ - ...sharedMessageProps, - diff: diffContents, - } satisfies ClineSayTool) - - let toolProgressStatus - - if (cline.diffStrategy && cline.diffStrategy.getProgressStatus) { - toolProgressStatus = cline.diffStrategy.getProgressStatus( - { - ...block, - params: { ...block.params, diff: diffContents }, - }, - { success: true }, - ) - } + // Prepare common data for single file operation + const diffContents = diffItems.map((item) => item.content).join("\n\n") + const operationMessage = JSON.stringify({ + ...sharedMessageProps, + diff: diffContents, + } satisfies ClineSayTool) + + let toolProgressStatus + if (cline.diffStrategy && cline.diffStrategy.getProgressStatus) { + toolProgressStatus = cline.diffStrategy.getProgressStatus( + { + ...block, + params: { ...block.params, diff: diffContents }, + }, + { success: true }, + ) + } + + // Set up diff view + cline.diffViewProvider.editType = "modify" + + // Show diff view if focus disruption prevention is disabled + if (!isPreventFocusDisruptionEnabled) { + await cline.diffViewProvider.open(relPath) + await cline.diffViewProvider.update(originalContent!, true) + cline.diffViewProvider.scrollToFirstDiff() + } else { + // For direct save, we still need to set originalContent + cline.diffViewProvider.originalContent = await fs.readFile(absolutePath, "utf-8") + } - // Check if file is write-protected - const isWriteProtected = cline.rooProtectedController?.isWriteProtected(relPath) || false - didApprove = await askApproval("tool", operationMessage, toolProgressStatus, isWriteProtected) + // Ask for approval (same for both flows) + const isWriteProtected = cline.rooProtectedController?.isWriteProtected(relPath) || false + didApprove = await askApproval("tool", operationMessage, toolProgressStatus, isWriteProtected) - if (!didApprove) { - results.push(`Changes to ${relPath} were not approved by user`) - continue + if (!didApprove) { + // Revert changes if diff view was shown + if (!isPreventFocusDisruptionEnabled) { + await cline.diffViewProvider.revertChanges() } + results.push(`Changes to ${relPath} were not approved by user`) + continue + } + // Save the changes + if (isPreventFocusDisruptionEnabled) { // Direct file write without diff view or opening the file - cline.diffViewProvider.editType = "modify" - cline.diffViewProvider.originalContent = await fs.readFile(absolutePath, "utf-8") await cline.diffViewProvider.saveDirectly( relPath, originalContent!, @@ -568,40 +583,6 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""} writeDelayMs, ) } else { - // Show diff view BEFORE asking for approval - cline.diffViewProvider.editType = "modify" - await cline.diffViewProvider.open(relPath) - await cline.diffViewProvider.update(originalContent!, true) - cline.diffViewProvider.scrollToFirstDiff() - - const diffContents = diffItems.map((item) => item.content).join("\n\n") - const operationMessage = JSON.stringify({ - ...sharedMessageProps, - diff: diffContents, - } satisfies ClineSayTool) - - let toolProgressStatus - - if (cline.diffStrategy && cline.diffStrategy.getProgressStatus) { - toolProgressStatus = cline.diffStrategy.getProgressStatus( - { - ...block, - params: { ...block.params, diff: diffContents }, - }, - { success: true }, - ) - } - - // Check if file is write-protected - const isWriteProtected = cline.rooProtectedController?.isWriteProtected(relPath) || false - didApprove = await askApproval("tool", operationMessage, toolProgressStatus, isWriteProtected) - - if (!didApprove) { - await cline.diffViewProvider.revertChanges() - results.push(`Changes to ${relPath} were not approved by user`) - continue - } - // Call saveChanges to update the DiffViewProvider properties await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } diff --git a/src/core/tools/searchAndReplaceTool.ts b/src/core/tools/searchAndReplaceTool.ts index c180acdbce..50f4868b50 100644 --- a/src/core/tools/searchAndReplaceTool.ts +++ b/src/core/tools/searchAndReplaceTool.ts @@ -210,47 +210,38 @@ export async function searchAndReplaceTool( EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, ) - if (isPreventFocusDisruptionEnabled) { - // Direct approval without showing diff view - const completeMessage = JSON.stringify({ - ...sharedMessageProps, - diff, - isProtected: isWriteProtected, - } satisfies ClineSayTool) - const didApprove = await cline - .ask("tool", completeMessage, isWriteProtected) - .then((response) => response.response === "yesButtonClicked") - - if (!didApprove) { - pushToolResult("Changes were rejected by the user.") - await cline.diffViewProvider.reset() - return - } - - // Direct file write without diff view or opening the file - await cline.diffViewProvider.saveDirectly(validRelPath, newContent, false, diagnosticsEnabled, writeDelayMs) - } else { - // Show diff view BEFORE asking for approval + const completeMessage = JSON.stringify({ + ...sharedMessageProps, + diff, + isProtected: isWriteProtected, + } satisfies ClineSayTool) + + // Show diff view if focus disruption prevention is disabled + if (!isPreventFocusDisruptionEnabled) { await cline.diffViewProvider.open(validRelPath) await cline.diffViewProvider.update(newContent, true) cline.diffViewProvider.scrollToFirstDiff() + } - const completeMessage = JSON.stringify({ - ...sharedMessageProps, - diff, - isProtected: isWriteProtected, - } satisfies ClineSayTool) - const didApprove = await cline - .ask("tool", completeMessage, isWriteProtected) - .then((response) => response.response === "yesButtonClicked") + const didApprove = await cline + .ask("tool", completeMessage, isWriteProtected) + .then((response) => response.response === "yesButtonClicked") - if (!didApprove) { + if (!didApprove) { + // Revert changes if diff view was shown + if (!isPreventFocusDisruptionEnabled) { await cline.diffViewProvider.revertChanges() - pushToolResult("Changes were rejected by the user.") - await cline.diffViewProvider.reset() - return } + pushToolResult("Changes were rejected by the user.") + await cline.diffViewProvider.reset() + return + } + // Save the changes + if (isPreventFocusDisruptionEnabled) { + // Direct file write without diff view or opening the file + await cline.diffViewProvider.saveDirectly(validRelPath, newContent, false, diagnosticsEnabled, writeDelayMs) + } else { // Call saveChanges to update the DiffViewProvider properties await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) }