From 87e7b3eba2db6ffdfe7b13ef195bf3006b4bb6a0 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Fri, 18 Jul 2025 00:32:40 +0000 Subject: [PATCH] feat: add configurable delay for Go diagnostics to prevent premature error reporting - Add diagnosticsDelayMs setting (default: 2000ms) to allow linters time to process - Add diagnosticsEnabled setting to optionally disable diagnostic checking entirely - Update DiffViewProvider.saveChanges() to use configurable delay before checking diagnostics - Update all tool files (writeToFile, searchAndReplace, insertContent, applyDiff, multiApplyDiff) to pass diagnostic settings - Add comprehensive tests for new diagnostic functionality - Fixes issue where Go diagnostics errors were submitted to LLM before linter could clean up unused imports Resolves #5859 --- src/core/tools/applyDiffTool.ts | 6 +- src/core/tools/insertContentTool.ts | 6 +- src/core/tools/multiApplyDiffTool.ts | 6 +- src/core/tools/searchAndReplaceTool.ts | 6 +- src/core/tools/writeToFileTool.ts | 6 +- src/core/webview/ClineProvider.ts | 6 ++ src/core/webview/webviewMessageHandler.ts | 8 ++ src/integrations/editor/DiffViewProvider.ts | 32 ++++--- .../editor/__tests__/DiffViewProvider.spec.ts | 85 +++++++++++++++++++ src/shared/ExtensionMessage.ts | 2 + src/shared/WebviewMessage.ts | 2 + 11 files changed, 149 insertions(+), 16 deletions(-) diff --git a/src/core/tools/applyDiffTool.ts b/src/core/tools/applyDiffTool.ts index f5b4ab7dd3..6caa504ea0 100644 --- a/src/core/tools/applyDiffTool.ts +++ b/src/core/tools/applyDiffTool.ts @@ -170,7 +170,11 @@ export async function applyDiffToolLegacy( } // Call saveChanges to update the DiffViewProvider properties - await cline.diffViewProvider.saveChanges() + const provider = cline.providerRef.deref() + const state = await provider?.getState() + const diagnosticsEnabled = state?.diagnosticsEnabled ?? true + const diagnosticsDelayMs = state?.diagnosticsDelayMs ?? 2000 + await cline.diffViewProvider.saveChanges(diagnosticsEnabled, diagnosticsDelayMs) // Track file edit operation if (relPath) { diff --git a/src/core/tools/insertContentTool.ts b/src/core/tools/insertContentTool.ts index b76769fcf0..3cada2653f 100644 --- a/src/core/tools/insertContentTool.ts +++ b/src/core/tools/insertContentTool.ts @@ -155,7 +155,11 @@ export async function insertContentTool( } // Call saveChanges to update the DiffViewProvider properties - await cline.diffViewProvider.saveChanges() + const provider = cline.providerRef.deref() + const state = await provider?.getState() + const diagnosticsEnabled = state?.diagnosticsEnabled ?? true + const diagnosticsDelayMs = state?.diagnosticsDelayMs ?? 2000 + await cline.diffViewProvider.saveChanges(diagnosticsEnabled, diagnosticsDelayMs) // Track file edit operation if (relPath) { diff --git a/src/core/tools/multiApplyDiffTool.ts b/src/core/tools/multiApplyDiffTool.ts index 8057f77949..b6ad77a58a 100644 --- a/src/core/tools/multiApplyDiffTool.ts +++ b/src/core/tools/multiApplyDiffTool.ts @@ -553,7 +553,11 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""} } // Call saveChanges to update the DiffViewProvider properties - await cline.diffViewProvider.saveChanges() + const provider = cline.providerRef.deref() + const state = await provider?.getState() + const diagnosticsEnabled = state?.diagnosticsEnabled ?? true + const diagnosticsDelayMs = state?.diagnosticsDelayMs ?? 2000 + await cline.diffViewProvider.saveChanges(diagnosticsEnabled, diagnosticsDelayMs) // Track file edit operation await cline.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) diff --git a/src/core/tools/searchAndReplaceTool.ts b/src/core/tools/searchAndReplaceTool.ts index 967d5339ba..83ce1d3696 100644 --- a/src/core/tools/searchAndReplaceTool.ts +++ b/src/core/tools/searchAndReplaceTool.ts @@ -227,7 +227,11 @@ export async function searchAndReplaceTool( } // Call saveChanges to update the DiffViewProvider properties - await cline.diffViewProvider.saveChanges() + const provider = cline.providerRef.deref() + const state = await provider?.getState() + const diagnosticsEnabled = state?.diagnosticsEnabled ?? true + const diagnosticsDelayMs = state?.diagnosticsDelayMs ?? 2000 + await cline.diffViewProvider.saveChanges(diagnosticsEnabled, diagnosticsDelayMs) // Track file edit operation if (relPath) { diff --git a/src/core/tools/writeToFileTool.ts b/src/core/tools/writeToFileTool.ts index 84f8ef807e..18a9d4dc47 100644 --- a/src/core/tools/writeToFileTool.ts +++ b/src/core/tools/writeToFileTool.ts @@ -213,7 +213,11 @@ export async function writeToFileTool( } // Call saveChanges to update the DiffViewProvider properties - await cline.diffViewProvider.saveChanges() + const provider = cline.providerRef.deref() + const state = await provider?.getState() + const diagnosticsEnabled = state?.diagnosticsEnabled ?? true + const diagnosticsDelayMs = state?.diagnosticsDelayMs ?? 2000 + await cline.diffViewProvider.saveChanges(diagnosticsEnabled, diagnosticsDelayMs) // Track file edit operation if (relPath) { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 107122dcb4..951d288496 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1436,6 +1436,8 @@ export class ClineProvider profileThresholds, alwaysAllowFollowupQuestions, followupAutoApproveTimeoutMs, + diagnosticsDelayMs, + diagnosticsEnabled, } = await this.getState() const telemetryKey = process.env.POSTHOG_API_KEY @@ -1555,6 +1557,8 @@ export class ClineProvider hasOpenedModeSelector: this.getGlobalState("hasOpenedModeSelector") ?? false, alwaysAllowFollowupQuestions: alwaysAllowFollowupQuestions ?? false, followupAutoApproveTimeoutMs: followupAutoApproveTimeoutMs ?? 60000, + diagnosticsDelayMs: diagnosticsDelayMs ?? 2000, + diagnosticsEnabled: diagnosticsEnabled ?? true, } } @@ -1638,6 +1642,8 @@ export class ClineProvider alwaysAllowFollowupQuestions: stateValues.alwaysAllowFollowupQuestions ?? false, alwaysAllowUpdateTodoList: stateValues.alwaysAllowUpdateTodoList ?? false, followupAutoApproveTimeoutMs: stateValues.followupAutoApproveTimeoutMs ?? 60000, + diagnosticsDelayMs: stateValues.diagnosticsDelayMs ?? 2000, + diagnosticsEnabled: stateValues.diagnosticsEnabled ?? true, allowedMaxRequests: stateValues.allowedMaxRequests, autoCondenseContext: stateValues.autoCondenseContext ?? true, autoCondenseContextPercent: stateValues.autoCondenseContextPercent ?? 100, diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 2efb2cbdff..e00adb2e54 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -1044,6 +1044,14 @@ export const webviewMessageHandler = async ( await updateGlobalState("writeDelayMs", message.value) await provider.postStateToWebview() break + case "diagnosticsDelayMs": + await updateGlobalState("diagnosticsDelayMs", message.value) + await provider.postStateToWebview() + break + case "diagnosticsEnabled": + await updateGlobalState("diagnosticsEnabled", message.bool ?? true) + await provider.postStateToWebview() + break case "terminalOutputLineLimit": await updateGlobalState("terminalOutputLineLimit", message.value) await provider.postStateToWebview() diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 225e076297..e09b056537 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -4,6 +4,7 @@ import * as fs from "fs/promises" import * as diff from "diff" import stripBom from "strip-bom" import { XMLBuilder } from "fast-xml-parser" +import delay from "delay" import { createDirectoriesForFile } from "../../utils/fs" import { arePathsEqual, getReadablePath } from "../../utils/path" @@ -179,7 +180,7 @@ export class DiffViewProvider { } } - async saveChanges(): Promise<{ + async saveChanges(diagnosticsEnabled: boolean = true, diagnosticsDelayMs: number = 2000): Promise<{ newProblemsMessage: string | undefined userEdits: string | undefined finalContent: string | undefined @@ -214,18 +215,27 @@ 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. - const postDiagnostics = vscode.languages.getDiagnostics() + + 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) + await delay(diagnosticsDelayMs) + + 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. + 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. - const newProblemsMessage = - newProblems.length > 0 ? `\n\nNew problems detected after saving the file:\n${newProblems}` : "" + newProblemsMessage = + newProblems.length > 0 ? `\n\nNew problems detected after saving the file:\n${newProblems}` : "" + } // If the edited content has different EOL characters, we don't want to // show a diff with all the EOL differences. diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index ad1950345b..fc130ce95d 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1,6 +1,12 @@ import { DiffViewProvider, DIFF_VIEW_URI_SCHEME, DIFF_VIEW_LABEL_CHANGES } from "../DiffViewProvider" import * as vscode from "vscode" import * as path from "path" +import delay from "delay" + +// Mock delay +vi.mock("delay", () => ({ + default: vi.fn().mockResolvedValue(undefined), +})) // Mock fs/promises vi.mock("fs/promises", () => ({ @@ -327,4 +333,83 @@ describe("DiffViewProvider", () => { ).toBeUndefined() }) }) + + describe("saveChanges method with diagnostic settings", () => { + beforeEach(() => { + // Setup common mocks for saveChanges tests + ;(diffViewProvider as any).relPath = "test.ts" + ;(diffViewProvider as any).newContent = "new content" + ;(diffViewProvider as any).activeDiffEditor = { + document: { + getText: vi.fn().mockReturnValue("new content"), + isDirty: false, + save: vi.fn().mockResolvedValue(undefined), + }, + } + ;(diffViewProvider as any).preDiagnostics = [] + + // Mock vscode functions + vi.mocked(vscode.window.showTextDocument).mockResolvedValue({} as any) + vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + }) + + it("should apply diagnostic delay when diagnosticsEnabled is true", async () => { + const mockDelay = vi.mocked(delay) + mockDelay.mockClear() + + // Mock closeAllDiffViews + ;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + + const result = await diffViewProvider.saveChanges(true, 3000) + + // Verify delay was called with correct duration + expect(mockDelay).toHaveBeenCalledWith(3000) + expect(vscode.languages.getDiagnostics).toHaveBeenCalled() + expect(result.newProblemsMessage).toBe("") + }) + + it("should skip diagnostics when diagnosticsEnabled is false", async () => { + const mockDelay = vi.mocked(delay) + mockDelay.mockClear() + + // Mock closeAllDiffViews + ;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + + const result = await diffViewProvider.saveChanges(false, 2000) + + // Verify delay was NOT called and diagnostics were NOT checked + expect(mockDelay).not.toHaveBeenCalled() + expect(vscode.languages.getDiagnostics).not.toHaveBeenCalled() + expect(result.newProblemsMessage).toBe("") + }) + + it("should use default values when no parameters provided", async () => { + const mockDelay = vi.mocked(delay) + mockDelay.mockClear() + + // Mock closeAllDiffViews + ;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + + const result = await diffViewProvider.saveChanges() + + // Verify default behavior (enabled=true, delay=2000ms) + expect(mockDelay).toHaveBeenCalledWith(2000) + expect(vscode.languages.getDiagnostics).toHaveBeenCalled() + expect(result.newProblemsMessage).toBe("") + }) + + it("should handle custom delay values", async () => { + const mockDelay = vi.mocked(delay) + mockDelay.mockClear() + + // Mock closeAllDiffViews + ;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + + const result = await diffViewProvider.saveChanges(true, 5000) + + // Verify custom delay was used + expect(mockDelay).toHaveBeenCalledWith(5000) + expect(vscode.languages.getDiagnostics).toHaveBeenCalled() + }) + }) }) diff --git a/src/shared/ExtensionMessage.ts b/src/shared/ExtensionMessage.ts index 98f3aa7d29..a9af6bc180 100644 --- a/src/shared/ExtensionMessage.ts +++ b/src/shared/ExtensionMessage.ts @@ -243,6 +243,8 @@ export type ExtensionState = Pick< taskHistory: HistoryItem[] writeDelayMs: number + diagnosticsDelayMs: number + diagnosticsEnabled: boolean requestDelaySeconds: number enableCheckpoints: boolean diff --git a/src/shared/WebviewMessage.ts b/src/shared/WebviewMessage.ts index 5d6ec0f41c..4b2fbfe738 100644 --- a/src/shared/WebviewMessage.ts +++ b/src/shared/WebviewMessage.ts @@ -107,6 +107,8 @@ export interface WebviewMessage { | "updateMcpTimeout" | "fuzzyMatchThreshold" | "writeDelayMs" + | "diagnosticsDelayMs" + | "diagnosticsEnabled" | "enhancePrompt" | "enhancedPrompt" | "draggedImages"