diff --git a/src/core/tools/__tests__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index 1ff8d52a8d..2d88649548 100644 --- a/src/core/tools/__tests__/editFileTool.spec.ts +++ b/src/core/tools/__tests__/editFileTool.spec.ts @@ -115,7 +115,9 @@ describe("editFileTool", () => { getState: vi.fn().mockResolvedValue({ diagnosticsEnabled: true, writeDelayMs: 1000, - experiments: {}, + // Pin the legacy diff-editor path explicitly: the PREVENT_FOCUS_DISRUPTION default + // flipped to true (L2, plan #33), so these tests must opt out. + experiments: { preventFocusDisruption: false } as Record, }), }), } @@ -576,6 +578,47 @@ describe("editFileTool", () => { }) }) + describe("focus disruption default (L2: chat-diff is the default approval path)", () => { + it("saves via saveDirectly without opening the diff editor when no experiment value is stored", async () => { + mockAskApproval.mockResolvedValue(true) + // No stored experiment value: the default (flipped to true in L2) resolves + // to the chat-diff path. + mockTask.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: {}, + }), + }) + + await executeEditFileTool() + + expect(mockTask.diffViewProvider.saveDirectly).toHaveBeenCalled() + expect(mockTask.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockTask.didEditFile).toBe(true) + }) + + it("saves via saveDirectly when the user has explicitly enabled the experiment", async () => { + mockAskApproval.mockResolvedValue(true) + // Explicit stored true: same chat-diff routing as the default. + mockTask.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }) + + await executeEditFileTool() + + expect(mockTask.diffViewProvider.saveDirectly).toHaveBeenCalled() + expect(mockTask.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockTask.didEditFile).toBe(true) + }) + }) + describe("partial block handling", () => { it("handles partial block without errors after path stabilizes", async () => { // Path stabilization requires two consecutive calls with the same path diff --git a/src/core/tools/__tests__/editTool.spec.ts b/src/core/tools/__tests__/editTool.spec.ts index a5f665b9e5..8be210fe7a 100644 --- a/src/core/tools/__tests__/editTool.spec.ts +++ b/src/core/tools/__tests__/editTool.spec.ts @@ -111,7 +111,9 @@ describe("editTool", () => { getState: vi.fn().mockResolvedValue({ diagnosticsEnabled: true, writeDelayMs: 1000, - experiments: {}, + // Pin the legacy diff-editor path explicitly: the PREVENT_FOCUS_DISRUPTION default + // flipped to true (L2, plan #33), so these tests must opt out. + experiments: { preventFocusDisruption: false } as Record, }), }), } @@ -361,6 +363,39 @@ describe("editTool", () => { }) }) + describe("focus disruption default (L2: chat-diff is the default approval path)", () => { + it("saves via saveDirectly without opening the diff editor when no experiment value is stored", async () => { + mockAskApproval.mockResolvedValue(true) + // No stored experiment value: the default (flipped to true in L2) resolves + // to the chat-diff path. + mockTask.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: {}, + }), + }) + + await executeEditTool() + + expect(mockTask.diffViewProvider.saveDirectly).toHaveBeenCalled() + expect(mockTask.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockTask.didEditFile).toBe(true) + }) + + it("still uses the diff-editor path when the user has opted out (stored false)", async () => { + mockAskApproval.mockResolvedValue(true) + // Base mock pins experiments: { preventFocusDisruption: false } (legacy path). + + await executeEditTool() + + expect(mockTask.diffViewProvider.open).toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveChanges).toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + }) + }) + describe("partial block handling", () => { it("handles partial block without errors after path stabilizes", async () => { // Path stabilization requires two consecutive calls with the same path diff --git a/src/core/tools/__tests__/searchReplaceTool.spec.ts b/src/core/tools/__tests__/searchReplaceTool.spec.ts index 5cf10790d4..10b8c9028a 100644 --- a/src/core/tools/__tests__/searchReplaceTool.spec.ts +++ b/src/core/tools/__tests__/searchReplaceTool.spec.ts @@ -113,7 +113,9 @@ describe("searchReplaceTool", () => { getState: vi.fn().mockResolvedValue({ diagnosticsEnabled: true, writeDelayMs: 1000, - experiments: {}, + // Pin the legacy diff-editor path explicitly: the PREVENT_FOCUS_DISRUPTION default + // flipped to true (L2, plan #33), so these tests must opt out. + experiments: { preventFocusDisruption: false } as Record, }), }), } @@ -330,6 +332,47 @@ describe("searchReplaceTool", () => { }) }) + describe("focus disruption default (L2: chat-diff is the default approval path)", () => { + it("saves via saveDirectly without opening the diff editor when no experiment value is stored", async () => { + mockAskApproval.mockResolvedValue(true) + // No stored experiment value: the default (flipped to true in L2) resolves + // to the chat-diff path. + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: {}, + }), + }) + + await executeSearchReplaceTool() + + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalled() + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(true) + }) + + it("saves via saveDirectly when the user has explicitly enabled the experiment", async () => { + mockAskApproval.mockResolvedValue(true) + // Explicit stored true: same chat-diff routing as the default. + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }) + + await executeSearchReplaceTool() + + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalled() + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(true) + }) + }) + describe("partial block handling", () => { it("handles partial block without errors after path stabilizes", async () => { // Path stabilization requires two consecutive calls with the same path diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 52a7e3c052..37a6b7b102 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -137,6 +137,9 @@ describe("writeToFileTool", () => { getState: vi.fn().mockResolvedValue({ diagnosticsEnabled: true, writeDelayMs: 1000, + // Pin the legacy diff-editor path explicitly: the PREVENT_FOCUS_DISRUPTION default + // flipped to true (L2, plan #33), so these tests must opt out. + experiments: { preventFocusDisruption: false }, }), }), } @@ -156,6 +159,7 @@ describe("writeToFileTool", () => { userEdits: null, finalContent: "final content", }), + saveDirectly: vi.fn().mockResolvedValue(undefined), scrollToFirstDiff: vi.fn(), updateDiagnosticSettings: vi.fn(), pushToolWriteResult: vi.fn().mockImplementation(async function ( @@ -364,6 +368,47 @@ describe("writeToFileTool", () => { }) }) + describe("focus disruption default (L2: chat-diff is the default approval path)", () => { + it("saves via saveDirectly without opening the diff editor when no experiment value is stored", async () => { + mockAskApproval.mockResolvedValue(true) + // No stored experiment value: the default (flipped to true in L2) resolves + // to the chat-diff path. + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: {}, + }), + }) + + await executeWriteFileTool() + + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalled() + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(true) + }) + + it("saves via saveDirectly when the user has explicitly enabled the experiment", async () => { + mockAskApproval.mockResolvedValue(true) + // Explicit stored true: same chat-diff routing as the default. + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }) + + await executeWriteFileTool() + + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalled() + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(true) + }) + }) + describe("file operations", () => { it("successfully creates new files with full workflow", async () => { await executeWriteFileTool({}, { fileExists: false }) diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index aee88f4061..36e698f0d2 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -811,15 +811,16 @@ describe("DiffViewProvider", () => { expect(result.finalContent).toBe("new content") }) - it("should not open file when openWithoutFocus is false", async () => { + it("should not open file when openWithoutFocus is false (focus not stolen: in-memory document only)", async () => { await diffViewProvider.saveDirectly("test.ts", "new content", false, true, 1000) // Verify file was written const fs = await import("fs/promises") expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") - // Verify file was NOT opened + // Verify file was NOT opened in the editor, and the document is loaded in memory only expect(vscode.window.showTextDocument).not.toHaveBeenCalled() + expect(vscode.workspace.openTextDocument).toHaveBeenCalledWith(vscode.Uri.file(`${mockCwd}/test.ts`)) }) it("should skip diagnostics when diagnosticsEnabled is false", async () => { diff --git a/src/shared/__tests__/experiments-preventFocusDisruption.spec.ts b/src/shared/__tests__/experiments-preventFocusDisruption.spec.ts index e9f96c7ce7..44a6e237c0 100644 --- a/src/shared/__tests__/experiments-preventFocusDisruption.spec.ts +++ b/src/shared/__tests__/experiments-preventFocusDisruption.spec.ts @@ -5,13 +5,13 @@ describe("PREVENT_FOCUS_DISRUPTION experiment", () => { expect(EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION).toBe("preventFocusDisruption") }) - it("should have PREVENT_FOCUS_DISRUPTION in experimentConfigsMap", () => { + it("should have PREVENT_FOCUS_DISRUPTION enabled by default (chat-diff is the default approval path)", () => { expect(experimentConfigsMap.PREVENT_FOCUS_DISRUPTION).toBeDefined() - expect(experimentConfigsMap.PREVENT_FOCUS_DISRUPTION.enabled).toBe(false) + expect(experimentConfigsMap.PREVENT_FOCUS_DISRUPTION.enabled).toBe(true) }) - it("should have PREVENT_FOCUS_DISRUPTION in experimentDefault", () => { - expect(experimentDefault.preventFocusDisruption).toBe(false) + it("should have PREVENT_FOCUS_DISRUPTION enabled in experimentDefault", () => { + expect(experimentDefault.preventFocusDisruption).toBe(true) }) it("should correctly check if PREVENT_FOCUS_DISRUPTION is enabled", () => { @@ -23,8 +23,8 @@ describe("PREVENT_FOCUS_DISRUPTION experiment", () => { const enabledConfig = { preventFocusDisruption: true } expect(experiments.isEnabled(enabledConfig, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION)).toBe(true) - // Test when experiment is not in config (should use default) + // Test when experiment is not in config (should use default — now enabled) const emptyConfig = {} - expect(experiments.isEnabled(emptyConfig, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION)).toBe(false) + expect(experiments.isEnabled(emptyConfig, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION)).toBe(true) }) }) diff --git a/src/shared/__tests__/experiments.spec.ts b/src/shared/__tests__/experiments.spec.ts index f2261a5c09..6e2f75090c 100644 --- a/src/shared/__tests__/experiments.spec.ts +++ b/src/shared/__tests__/experiments.spec.ts @@ -6,10 +6,10 @@ import { EXPERIMENT_IDS, experimentConfigsMap, experiments as Experiments } from describe("experiments", () => { describe("PREVENT_FOCUS_DISRUPTION", () => { - it("is configured correctly", () => { + it("is configured correctly (chat-diff is the default approval path)", () => { expect(EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION).toBe("preventFocusDisruption") expect(experimentConfigsMap.PREVENT_FOCUS_DISRUPTION).toMatchObject({ - enabled: false, + enabled: true, }) }) }) diff --git a/src/shared/experiments.ts b/src/shared/experiments.ts index ae538b9138..f8fb132394 100644 --- a/src/shared/experiments.ts +++ b/src/shared/experiments.ts @@ -19,7 +19,10 @@ interface ExperimentConfig { } export const experimentConfigsMap: Record = { - PREVENT_FOCUS_DISRUPTION: { enabled: false }, + // Chat-diff is the default approval path (plan issue #33, L2): writes are saved + // without opening/refocusing the diff editor. The diff-editor path remains + // available by toggling this experiment off (storage key unchanged). + PREVENT_FOCUS_DISRUPTION: { enabled: true }, IMAGE_GENERATION: { enabled: false }, RUN_SLASH_COMMAND: { enabled: false }, CUSTOM_TOOLS: { enabled: false },