diff --git a/src/core/tools/__tests__/applyDiffTool.experiments.spec.ts b/src/core/tools/__tests__/applyDiffTool.experiments.spec.ts new file mode 100644 index 0000000000..fa2a7949c1 --- /dev/null +++ b/src/core/tools/__tests__/applyDiffTool.experiments.spec.ts @@ -0,0 +1,220 @@ +import fs from "fs/promises" + +import type { DiffResult, ToolResponse } from "../../../shared/tools" +import type { Task } from "../../task/Task" + +type Experiments = Record + +interface ProviderFixture { + getState: () => Promise<{ experiments?: Experiments }> +} + +interface DiffViewFixture { + editType: string | undefined + originalContent: string + open: (path: string) => Promise + update: (content: string, final: boolean) => Promise + reset: () => Promise + revertChanges: () => Promise + saveChanges: (diagnosticsEnabled?: boolean, writeDelayMs?: number) => Promise + saveDirectly: ( + path: string, + content: string, + open?: boolean, + diagnosticsEnabled?: boolean, + writeDelayMs?: number, + ) => Promise + scrollToFirstDiff: () => void + pushToolWriteResult: () => Promise +} + +/** The structural slice of Task this tool touches; the single cast to Task happens at construction. */ +interface TaskFixture { + cwd: string + consecutiveMistakeCount: number + consecutiveMistakeCountForApplyDiff: Map + didEditFile: boolean + api: { getModel: () => { id: string } } + rooIgnoreController: { validateAccess: (path: string) => boolean } + rooProtectedController: { isWriteProtected: (path: string) => boolean } + providerRef: { deref: () => ProviderFixture | undefined } + diffViewProvider: DiffViewFixture + fileContextTracker: { trackFileContext: (path: string, source: string) => Promise } + say: (type: string, text?: string) => Promise + recordToolError: (tool: string, error?: string) => void + sayAndCreateMissingParamError: (tool: string, param: string) => Promise + processQueuedMessages: () => void + diffStrategy: { + getName: () => string + applyDiff: (original: string, diff: string) => Promise + } +} +import { fileExistsAtPath } from "../../../utils/fs" +import type { ToolCallbacks } from "../BaseTool" +import { applyDiffTool } from "../ApplyDiffTool" + +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { instance: { captureDiffApplicationError: vi.fn() } }, +})) + +vi.mock("fs/promises", () => ({ + default: { + readFile: vi.fn().mockResolvedValue(""), + }, +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockResolvedValue(true), +})) + +vi.mock("../../../utils/path", () => ({ + getReadablePath: vi.fn().mockReturnValue("test/file.txt"), +})) + +vi.mock("../../prompts/responses", () => ({ + formatResponse: { + toolError: vi.fn((message: string) => `Error: ${message}`), + rooIgnoreError: vi.fn((filePath: string) => `Access denied: ${filePath}`), + createPrettyPatch: vi.fn(() => "mock-diff"), + }, +})) + +vi.mock("../../diff/stats", () => ({ + sanitizeUnifiedDiff: vi.fn((diff: string) => diff), + computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })), +})) + +vi.mock("vscode", () => ({ + window: { showWarningMessage: vi.fn() }, + env: { openExternal: vi.fn() }, + Uri: { parse: vi.fn() }, +})) + +describe("ApplyDiffTool focus-disruption routing (L2)", () => { + const testFilePath = "test/file.txt" + const diffContent = "<<<<<<< SEARCH\nline one\n=======\nline one edited\n>>>>>>> REPLACE" + + let fixture: TaskFixture + let mockTask: Task + let mockAskApproval: ReturnType> + let mockHandleError: ReturnType> + let pushedResult: ToolResponse | undefined + let approved: boolean + + beforeEach(() => { + vi.clearAllMocks() + pushedResult = undefined + approved = true + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + vi.mocked(fs.readFile).mockResolvedValue("line one\nline two\nline three\n") + + mockAskApproval = vi.fn().mockImplementation(async () => approved) + mockHandleError = vi.fn().mockResolvedValue(undefined) + + fixture = { + cwd: "/workspace", + api: { getModel: () => ({ id: "claude-test" }) }, + consecutiveMistakeCount: 0, + consecutiveMistakeCountForApplyDiff: new Map(), + didEditFile: false, + rooIgnoreController: { validateAccess: vi.fn().mockReturnValue(true) }, + rooProtectedController: { isWriteProtected: vi.fn().mockReturnValue(false) }, + providerRef: { deref: vi.fn().mockReturnValue({ getState: vi.fn().mockResolvedValue({}) }) }, + diffViewProvider: { + editType: undefined, + originalContent: "", + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + reset: vi.fn().mockResolvedValue(undefined), + revertChanges: vi.fn().mockResolvedValue(undefined), + saveChanges: vi.fn().mockResolvedValue(undefined), + saveDirectly: vi.fn().mockResolvedValue(undefined), + scrollToFirstDiff: vi.fn(), + pushToolWriteResult: vi.fn().mockResolvedValue("Changes successfully applied."), + }, + fileContextTracker: { trackFileContext: vi.fn().mockResolvedValue(undefined) }, + say: vi.fn().mockResolvedValue(undefined), + recordToolError: vi.fn(), + sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing param error"), + processQueuedMessages: vi.fn(), + diffStrategy: { + getName: () => "test", + applyDiff: vi.fn().mockResolvedValue({ success: true, content: "updated content" } as DiffResult), + }, + } + // One documented construction-site assertion: every member above is individually typed. + mockTask = fixture as unknown as Task + }) + + function setExperiments(experiments: Record) { + mockTask.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ experiments }), + }) as unknown as Task["providerRef"]["deref"] + } + + async function runApplyDiff() { + await applyDiffTool.execute({ path: testFilePath, diff: diffContent }, mockTask, { + askApproval: mockAskApproval, + handleError: mockHandleError, + toolCallId: "tool-call-1", + pushToolResult: vi.fn((result: ToolResponse) => { + pushedResult = result + }), + }) + expect(mockHandleError).not.toHaveBeenCalled() + } + + it("uses the direct chat-diff save when no experiment value is stored", async () => { + setExperiments({}) + + await runApplyDiff() + + expect(mockTask.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + "updated content", + false, + true, + 0, + ) + expect(mockTask.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockTask.didEditFile).toBe(true) + expect(String(pushedResult)).toContain("Changes successfully applied.") + }) + + it("uses the diff-editor path when the user opted out", async () => { + setExperiments({ preventFocusDisruption: false }) + + await runApplyDiff() + + expect(mockTask.diffViewProvider.open).toHaveBeenCalledWith(testFilePath) + expect(mockTask.diffViewProvider.update).toHaveBeenCalledWith("updated content", true) + expect(mockTask.diffViewProvider.saveChanges).toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + expect(String(pushedResult)).toContain("Changes successfully applied.") + }) + + it("does not save when the direct chat-diff approval is rejected", async () => { + approved = false + setExperiments({}) + + await runApplyDiff() + + expect(mockAskApproval).toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + expect(mockTask.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(pushedResult).toBeUndefined() + }) + + it("reverts the diff editor and does not save when the opt-out approval is rejected", async () => { + approved = false + setExperiments({ preventFocusDisruption: false }) + + await runApplyDiff() + + expect(mockTask.diffViewProvider.revertChanges).toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + }) +}) diff --git a/src/core/tools/__tests__/applyPatchTool.experiments.spec.ts b/src/core/tools/__tests__/applyPatchTool.experiments.spec.ts new file mode 100644 index 0000000000..c875559e73 --- /dev/null +++ b/src/core/tools/__tests__/applyPatchTool.experiments.spec.ts @@ -0,0 +1,262 @@ +import { fileExistsAtPath } from "../../../utils/fs" +import { isPathOutsideWorkspace } from "../../../utils/pathUtils" +import type { Task } from "../../task/Task" +import { ApplyPatchTool } from "../ApplyPatchTool" + +vi.mock("fs/promises", () => ({ + default: { + readFile: vi.fn().mockResolvedValue("original file content\n"), + writeFile: vi.fn().mockResolvedValue(undefined), + mkdir: vi.fn().mockResolvedValue(undefined), + unlink: vi.fn().mockResolvedValue(undefined), + }, +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockResolvedValue(true), +})) + +vi.mock("../../../utils/pathUtils", () => ({ + isPathOutsideWorkspace: vi.fn().mockReturnValue(false), +})) + +vi.mock("../../../utils/path", () => ({ + getReadablePath: vi.fn((_cwd: string, p: string) => p), +})) + +vi.mock("../../prompts/responses", () => ({ + formatResponse: { + toolError: vi.fn((message: string) => `Error: ${message}`), + rooIgnoreError: vi.fn((filePath: string) => `Access denied: ${filePath}`), + createPrettyPatch: vi.fn(() => "mock-diff"), + }, +})) + +vi.mock("../../diff/stats", () => ({ + sanitizeUnifiedDiff: vi.fn((diff: string) => diff), + computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })), +})) + +type Experiments = Record + +interface ProviderFixture { + getState: () => Promise<{ experiments?: Experiments }> +} + +interface DiffViewFixture { + editType: string | undefined + originalContent: string | undefined + open: (path: string) => Promise + update: (content: string, final: boolean) => Promise + reset: () => Promise + revertChanges: () => Promise + saveChanges: (diagnosticsEnabled?: boolean, writeDelayMs?: number) => Promise + saveDirectly: ( + path: string, + content: string, + open?: boolean, + diagnosticsEnabled?: boolean, + writeDelayMs?: number, + ) => Promise + scrollToFirstDiff: () => void + pushToolWriteResult: () => Promise +} + +interface TaskFixture { + cwd: string + consecutiveMistakeCount: number + didEditFile: boolean + rooIgnoreController: { validateAccess: (path: string) => boolean } + rooProtectedController: { isWriteProtected: (path: string) => boolean } + providerRef: { deref: () => ProviderFixture | undefined } + diffViewProvider: DiffViewFixture + fileContextTracker: { trackFileContext: (path: string, source: string) => Promise } + say: (type: string, text?: string) => Promise + recordToolError: (tool: string, error?: string) => void + recordToolUsage: (tool: string) => void + processQueuedMessages: () => void +} + +describe("ApplyPatchTool focus-disruption routing (L2)", () => { + const addPatch = `*** Begin Patch +*** Add File: src/new.ts ++line one +*** End Patch` + + const updatePatch = `*** Begin Patch +*** Update File: src/existing.ts +@@ +-original file content ++updated file content +*** End Patch` + + let fixture: TaskFixture + let tool: ApplyPatchTool + let approved: boolean + + beforeEach(() => { + vi.clearAllMocks() + approved = true + vi.mocked(fileExistsAtPath).mockResolvedValue(false) + vi.mocked(isPathOutsideWorkspace).mockReturnValue(false) + + fixture = { + cwd: "/workspace/project", + consecutiveMistakeCount: 0, + didEditFile: false, + rooIgnoreController: { validateAccess: vi.fn().mockReturnValue(true) }, + rooProtectedController: { isWriteProtected: vi.fn().mockReturnValue(false) }, + providerRef: { deref: vi.fn() }, + diffViewProvider: { + editType: undefined, + originalContent: undefined, + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + reset: vi.fn().mockResolvedValue(undefined), + revertChanges: vi.fn().mockResolvedValue(undefined), + saveChanges: vi.fn().mockResolvedValue(undefined), + saveDirectly: vi.fn().mockResolvedValue(undefined), + scrollToFirstDiff: vi.fn(), + pushToolWriteResult: vi.fn().mockResolvedValue("Changes successfully applied."), + }, + fileContextTracker: { trackFileContext: vi.fn().mockResolvedValue(undefined) }, + say: vi.fn().mockResolvedValue(undefined), + recordToolError: vi.fn(), + recordToolUsage: vi.fn(), + processQueuedMessages: vi.fn(), + } + tool = new ApplyPatchTool() + }) + + const createProvider = (experiments: Experiments): ProviderFixture => ({ + getState: vi.fn().mockResolvedValue({ experiments }), + }) + + function setExperiments(experiments: Experiments) { + fixture.providerRef.deref = vi.fn().mockReturnValue(createProvider(experiments)) + } + + async function runPatch(patch: string) { + const pushToolResult = vi.fn() + const handleError = vi.fn().mockResolvedValue(undefined) + await tool.execute({ patch }, fixture as unknown as Task, { + askApproval: vi.fn().mockImplementation(async () => approved), + handleError, + pushToolResult, + toolCallId: "tool-call-1", + }) + return { pushToolResult, handleError } + } + + it("adds a file through the direct save when no experiment value is stored", async () => { + setExperiments({}) + vi.mocked(fileExistsAtPath).mockResolvedValue(false) + + const { pushToolResult, handleError } = await runPatch(addPatch) + + expect(fixture.diffViewProvider.saveDirectly).toHaveBeenCalledWith("src/new.ts", "line one\n", true, true, 0) + expect(fixture.diffViewProvider.open).not.toHaveBeenCalled() + expect(fixture.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(fixture.didEditFile).toBe(true) + expect(pushToolResult).toHaveBeenCalledWith("Changes successfully applied.") + expect(handleError).not.toHaveBeenCalled() + }) + + it("adds a file through the diff editor when the user opted out", async () => { + setExperiments({ preventFocusDisruption: false }) + vi.mocked(fileExistsAtPath).mockResolvedValue(false) + + const { pushToolResult, handleError } = await runPatch(addPatch) + + expect(fixture.diffViewProvider.open).toHaveBeenCalledWith("src/new.ts") + expect(fixture.diffViewProvider.update).toHaveBeenCalledWith("line one\n", true) + expect(fixture.diffViewProvider.saveChanges).toHaveBeenCalled() + expect(fixture.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + expect(pushToolResult).toHaveBeenCalledWith("Changes successfully applied.") + expect(handleError).not.toHaveBeenCalled() + }) + + it("updates a file through the direct save when no experiment value is stored", async () => { + setExperiments({}) + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + + const { pushToolResult, handleError } = await runPatch(updatePatch) + + expect(fixture.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + "src/existing.ts", + "updated file content\n", + false, + true, + 0, + ) + expect(fixture.diffViewProvider.open).not.toHaveBeenCalled() + expect(fixture.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(fixture.didEditFile).toBe(true) + expect(pushToolResult).toHaveBeenCalledWith("Changes successfully applied.") + expect(handleError).not.toHaveBeenCalled() + }) + + it("updates a file through the diff editor when the user opted out", async () => { + setExperiments({ preventFocusDisruption: false }) + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + + const { pushToolResult, handleError } = await runPatch(updatePatch) + + expect(fixture.diffViewProvider.open).toHaveBeenCalledWith("src/existing.ts") + expect(fixture.diffViewProvider.update).toHaveBeenCalledWith("updated file content\n", true) + expect(fixture.diffViewProvider.saveChanges).toHaveBeenCalled() + expect(fixture.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + expect(pushToolResult).toHaveBeenCalledWith("Changes successfully applied.") + expect(handleError).not.toHaveBeenCalled() + }) + + it("rejects an add-file patch without saving when the experiment is unset", async () => { + approved = false + setExperiments({}) + vi.mocked(fileExistsAtPath).mockResolvedValue(false) + + const { pushToolResult } = await runPatch(addPatch) + + expect(fixture.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + expect(fixture.diffViewProvider.open).not.toHaveBeenCalled() + expect(pushToolResult).toHaveBeenCalledWith("Changes were rejected by the user.") + }) + + it("reverts the diff editor and does not save a rejected add-file patch when opted out", async () => { + approved = false + setExperiments({ preventFocusDisruption: false }) + vi.mocked(fileExistsAtPath).mockResolvedValue(false) + + await runPatch(addPatch) + + expect(fixture.diffViewProvider.open).toHaveBeenCalled() + expect(fixture.diffViewProvider.revertChanges).toHaveBeenCalled() + expect(fixture.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(fixture.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + }) + + it("rejects an update-file patch without saving when the experiment is unset", async () => { + approved = false + setExperiments({}) + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + + const { pushToolResult } = await runPatch(updatePatch) + + expect(fixture.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + expect(fixture.diffViewProvider.open).not.toHaveBeenCalled() + expect(pushToolResult).toHaveBeenCalledWith("Changes were rejected by the user.") + }) + + it("reverts the diff editor and does not save a rejected update-file patch when opted out", async () => { + approved = false + setExperiments({ preventFocusDisruption: false }) + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + + await runPatch(updatePatch) + + expect(fixture.diffViewProvider.open).toHaveBeenCalled() + expect(fixture.diffViewProvider.revertChanges).toHaveBeenCalled() + expect(fixture.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(fixture.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + }) +}) diff --git a/src/core/tools/__tests__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index 95a67f04d7..e03b9fdb57 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, }), }), } @@ -578,6 +580,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 fbb14d7ed2..a0b412d971 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, }), }), } @@ -362,6 +364,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 65311462cb..5071309795 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, }), }), } @@ -331,6 +333,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 7cb9d01585..66161aaedc 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 ( @@ -365,6 +369,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 a740ebed5c..243e90d794 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 }, diff --git a/webview-ui/src/components/settings/SettingsView.tsx b/webview-ui/src/components/settings/SettingsView.tsx index 6100a373e1..f8fd28d9fc 100644 --- a/webview-ui/src/components/settings/SettingsView.tsx +++ b/webview-ui/src/components/settings/SettingsView.tsx @@ -42,6 +42,8 @@ import { ImageGenerationProvider, } from "@roo-code/types" +import { experimentDefault } from "@roo/experiments" + import { vscode } from "@src/utils/vscode" import { cn } from "@src/lib/utils" import { useAppTranslation } from "@src/i18n/TranslationContext" @@ -150,6 +152,13 @@ const SettingsView = forwardRef(({ onDone, t const prevMode = useRef(mode) const handledSettingsImportedAt = useRef(undefined) const confirmDialogHandler = useRef<() => void>() + // Experiment ids toggled during this edit session. Only these are sent on Save: the + // extension merges the received map over the stored experiments, so writing the whole + // live map would freeze every other experiment at today's default. + const changedExperiments = useRef(new Set()) + // Last value submitted per experiment. Until the extension state reflects it, this is the + // comparison baseline: comparing against the (stale) host state could drop a follow-up edit. + const submittedExperiments = useRef(new Map()) const [cachedState, setCachedState] = useState(() => extensionState) @@ -235,9 +244,21 @@ const SettingsView = forwardRef(({ onDone, t setCachedState((prevCachedState) => ({ ...prevCachedState, ...extensionState })) prevApiConfigName.current = currentApiConfigName prevMode.current = mode + changedExperiments.current.clear() + // The edit session resets here too: a pending submitted baseline no longer applies. + submittedExperiments.current.clear() setChangeDetected(false) }, [currentApiConfigName, mode, extensionState]) + // Drop a submitted baseline once the extension state confirms that value. + useEffect(() => { + for (const [id, value] of submittedExperiments.current) { + if ((extensionState.experiments?.[id] ?? experimentDefault[id]) === value) { + submittedExperiments.current.delete(id) + } + } + }, [extensionState.experiments]) + // Bust the cache when settings are imported. useEffect(() => { if (!settingsImportedAt || handledSettingsImportedAt.current === settingsImportedAt) { @@ -245,6 +266,10 @@ const SettingsView = forwardRef(({ onDone, t } handledSettingsImportedAt.current = settingsImportedAt + changedExperiments.current.clear() + // Imported state replaces the settings the pending baseline was compared against: drop it so + // it cannot suppress a later save of the newly imported value. + submittedExperiments.current.clear() setCachedState((prevCachedState) => ({ ...prevCachedState, ...extensionState })) setChangeDetected(false) }, [settingsImportedAt, extensionState]) @@ -308,6 +333,7 @@ const SettingsView = forwardRef(({ onDone, t } setChangeDetected(true) + changedExperiments.current.add(id) return { ...prevState, experiments: { ...prevState.experiments, [id]: enabled } } }) }, []) @@ -382,6 +408,22 @@ const SettingsView = forwardRef(({ onDone, t const handleSubmit = () => { if (isSettingValid) { + // Only experiments that actually differ from the live state are persisted, so a + // future default flip still reaches users who have saved other settings before. + const experimentChanges: Record = {} + for (const id of changedExperiments.current) { + const value = experiments?.[id] ?? experimentDefault[id] + // Baseline: the last submitted value while the host has not confirmed it yet, then + // the live state/default. This keeps a follow-up edit made before the state round-trip + // from being compared against stale host values. + const baseline = submittedExperiments.current.has(id) + ? submittedExperiments.current.get(id) + : (extensionState.experiments?.[id] ?? experimentDefault[id]) + if (value !== baseline) { + experimentChanges[id] = value + } + } + vscode.postMessage({ type: "updateSettings", updatedSettings: { @@ -454,7 +496,7 @@ const SettingsView = forwardRef(({ onDone, t imageGenerationProvider, openRouterImageApiKey, openRouterImageGenerationSelectedModel, - experiments, + experiments: Object.keys(experimentChanges).length > 0 ? experimentChanges : undefined, customSupportPrompts, }, }) @@ -465,6 +507,15 @@ const SettingsView = forwardRef(({ onDone, t vscode.postMessage({ type: "telemetrySetting", text: telemetrySetting }) vscode.postMessage({ type: "debugSetting", bool: cachedState.debug }) + // Remember what was submitted until the extension state confirms it. + for (const id of changedExperiments.current) { + const submitted = experimentChanges[id] + if (submitted === undefined) { + submittedExperiments.current.delete(id) + } else { + submittedExperiments.current.set(id, submitted) + } + } setChangeDetected(false) } } @@ -488,6 +539,8 @@ const SettingsView = forwardRef(({ onDone, t if (confirm) { // Discard changes: Reset state and flag setCachedState(extensionState) // Revert to original state + submittedExperiments.current.clear() + changedExperiments.current.clear() setChangeDetected(false) // Reset change flag confirmDialogHandler.current?.() // Execute the pending action (e.g., tab switch) } diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx index 0835f255f9..79e27f74ef 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -46,17 +46,18 @@ vi.mock("../ApiConfigManager", () => ({ })) vi.mock("@vscode/webview-ui-toolkit/react", () => ({ - VSCodeButton: ({ children, onClick, appearance, "data-testid": dataTestId }: any) => + VSCodeButton: ({ children, onClick, appearance, disabled, "data-testid": dataTestId }: any) => appearance === "icon" ? ( ) : ( - ), @@ -190,8 +191,13 @@ vi.mock("@/components/ui", () => ({ data-testid={dataTestId} /> ), - Button: ({ children, onClick, variant, className, "data-testid": dataTestId }: any) => ( - ), @@ -980,6 +986,265 @@ describe("SettingsView - Blanket Auto-Deny", () => { }) }) +describe("SettingsView - Experiments", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("persists only the experiment the user toggled, not the whole map", async () => { + const { activateTab, getSettingsContent } = renderSettingsView({ experiments: { imageGeneration: false } }) + activateTab("experimental") + + const saveButton = screen.getByTestId("save-button") + // The experiment toggle alone must enable Save: no unrelated setting change is needed. + await waitFor(() => expect(saveButton).toBeDisabled()) + + const label = await waitFor(() => { + const content = getSettingsContent() + return within(content).getByText("settings:experimental.PREVENT_FOCUS_DISRUPTION.name").closest("label") + }) + expect(label).not.toBeNull() + const checkbox = label!.querySelector('input[type="checkbox"]') + expect(checkbox).not.toBeNull() + fireEvent.click(checkbox as HTMLInputElement) + await waitFor(() => expect(saveButton).toBeEnabled()) + + // Buffered only: nothing is persisted before Save. + expect(vscode.postMessage).not.toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + updatedSettings: expect.objectContaining({ experiments: expect.anything() }), + }), + ) + + fireEvent.click(saveButton) + + // Only the toggled key is sent: the host merges the received map over the stored + // experiments, so sending the whole live map would freeze other defaults. + expect(vscode.postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + updatedSettings: expect.objectContaining({ + // The control starts enabled by default; toggling it off is the change. + experiments: { preventFocusDisruption: false }, + }), + }), + ) + }) + + it("submits the toggled-on value when a stored false is rendered", async () => { + renderWithExtensionState(, { + state: { experiments: { preventFocusDisruption: false } }, + }) + + const saveButton = screen.getByTestId("save-button") + await waitFor(() => expect(saveButton).toBeDisabled()) + + const label = await waitFor(() => { + const content = screen.getByTestId("settings-content") + return within(content).getByText("settings:experimental.PREVENT_FOCUS_DISRUPTION.name").closest("label") + }) + const checkbox = label!.querySelector('input[type="checkbox"]') as HTMLInputElement + // The stored false is hydrated, so the control starts unchecked. + expect(checkbox).not.toBeChecked() + + fireEvent.click(checkbox) + await waitFor(() => expect(saveButton).toBeEnabled()) + fireEvent.click(saveButton) + + expect(vscode.postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + updatedSettings: expect.objectContaining({ experiments: { preventFocusDisruption: true } }), + }), + ) + }) + + it("keeps a follow-up experiment edit when the previous save is not yet confirmed", async () => { + const { activateTab, getSettingsContent } = renderSettingsView({ experiments: { imageGeneration: false } }) + activateTab("experimental") + + const saveButton = screen.getByTestId("save-button") + await waitFor(() => expect(saveButton).toBeDisabled()) + const label = await waitFor(() => { + const content = getSettingsContent() + return within(content).getByText("settings:experimental.PREVENT_FOCUS_DISRUPTION.name").closest("label") + }) + const checkbox = label!.querySelector('input[type="checkbox"]') as HTMLInputElement + + // Save the default-on -> false change, then (before the host state round-trip) toggle it + // back on. The second Save must compare against the submitted false, not the stale default. + fireEvent.click(checkbox) + await waitFor(() => expect(saveButton).toBeEnabled()) + fireEvent.click(saveButton) + fireEvent.click(checkbox) + await waitFor(() => expect(saveButton).toBeEnabled()) + fireEvent.click(saveButton) + + const updateSettingsCalls = vi + .mocked(vscode.postMessage) + .mock.calls.filter(([message]) => (message as { type?: string }).type === "updateSettings") + .map( + ([message]) => + (message as { updatedSettings: { experiments?: Record } }).updatedSettings, + ) + .filter((settings) => settings.experiments !== undefined) + expect(updateSettingsCalls).toHaveLength(2) + expect(updateSettingsCalls[0]?.experiments).toEqual({ preventFocusDisruption: false }) + expect(updateSettingsCalls[1]?.experiments).toEqual({ preventFocusDisruption: true }) + }) + + it("clears the pending submitted baseline when settings are imported", async () => { + const { activateTab, getSettingsContent } = renderSettingsView({ experiments: { imageGeneration: false } }) + activateTab("experimental") + + const saveButton = screen.getByTestId("save-button") + await waitFor(() => expect(saveButton).toBeDisabled()) + const label = await waitFor(() => { + const content = getSettingsContent() + return within(content).getByText("settings:experimental.PREVENT_FOCUS_DISRUPTION.name").closest("label") + }) + const checkbox = label!.querySelector('input[type="checkbox"]') as HTMLInputElement + + // Save default-on -> false to create a pending submitted baseline of false. + fireEvent.click(checkbox) + await waitFor(() => expect(saveButton).toBeEnabled()) + fireEvent.click(saveButton) + + // Import replaces the settings: the pending baseline must be dropped. + act(() => { + mockPostMessage({ settingsImportedAt: Date.now(), experiments: {} }) + }) + await waitFor(() => expect(checkbox).toBeChecked()) + await waitFor(() => expect(saveButton).toBeDisabled()) + + // Toggling off now equals the dropped baseline; if it were still pending, the save would be + // skipped. The import must have cleared it so the change is persisted. + fireEvent.click(checkbox) + await waitFor(() => expect(saveButton).toBeEnabled()) + fireEvent.click(saveButton) + + const experimentPayloads = vi + .mocked(vscode.postMessage) + .mock.calls.filter(([message]) => (message as { type?: string }).type === "updateSettings") + .map( + ([message]) => + (message as { updatedSettings: { experiments?: Record } }).updatedSettings, + ) + .filter((settings) => settings.experiments !== undefined) + expect(experimentPayloads).toHaveLength(2) + expect(experimentPayloads[1]?.experiments).toEqual({ preventFocusDisruption: false }) + }) + + it("clears the pending submitted baseline on an API/profile change", async () => { + const { activateTab, getSettingsContent } = renderSettingsView({ experiments: { imageGeneration: false } }) + activateTab("experimental") + + const saveButton = screen.getByTestId("save-button") + await waitFor(() => expect(saveButton).toBeDisabled()) + const label = await waitFor(() => { + const content = getSettingsContent() + return within(content).getByText("settings:experimental.PREVENT_FOCUS_DISRUPTION.name").closest("label") + }) + const checkbox = label!.querySelector('input[type="checkbox"]') as HTMLInputElement + + // Create a pending submitted baseline of false. + fireEvent.click(checkbox) + await waitFor(() => expect(saveButton).toBeEnabled()) + fireEvent.click(saveButton) + + // An API/profile change resets the settings edit session. + act(() => { + mockPostMessage({ currentApiConfigName: "other-config" }) + }) + await waitFor(() => expect(checkbox).toBeChecked()) + await waitFor(() => expect(saveButton).toBeDisabled()) + + // The reset must have dropped the pending baseline so this change is persisted. + fireEvent.click(checkbox) + await waitFor(() => expect(saveButton).toBeEnabled()) + fireEvent.click(saveButton) + + const experimentPayloads = vi + .mocked(vscode.postMessage) + .mock.calls.filter(([message]) => (message as { type?: string }).type === "updateSettings") + .map( + ([message]) => + (message as { updatedSettings: { experiments?: Record } }).updatedSettings, + ) + .filter((settings) => settings.experiments !== undefined) + expect(experimentPayloads.length).toBeGreaterThanOrEqual(2) + expect(experimentPayloads[experimentPayloads.length - 1]?.experiments).toEqual({ + preventFocusDisruption: false, + }) + }) + + it("clears the pending submitted baseline on a confirmed discard", async () => { + const { activateTab, getSettingsContent } = renderSettingsView({ experiments: { imageGeneration: false } }) + activateTab("experimental") + + const saveButton = screen.getByTestId("save-button") + await waitFor(() => expect(saveButton).toBeDisabled()) + const label = await waitFor(() => { + const content = getSettingsContent() + return within(content).getByText("settings:experimental.PREVENT_FOCUS_DISRUPTION.name").closest("label") + }) + const checkbox = label!.querySelector('input[type="checkbox"]') as HTMLInputElement + + // Create a pending submitted baseline of false. + fireEvent.click(checkbox) + await waitFor(() => expect(saveButton).toBeEnabled()) + fireEvent.click(saveButton) + + // Make a new unsaved edit after the Save, then confirm Discard: the dialog must open and + // its confirmation is what resets the edit session. + fireEvent.click(checkbox) + await waitFor(() => expect(saveButton).toBeEnabled()) + fireEvent.click(screen.getByText("settings:common.done")) + await waitFor(() => expect(screen.getByTestId("alert-dialog-action")).toBeInTheDocument()) + fireEvent.click(screen.getByTestId("alert-dialog-action")) + await waitFor(() => expect(checkbox).toBeChecked()) + await waitFor(() => expect(saveButton).toBeDisabled()) + + // The reset must have dropped the pending baseline so this change is persisted. + fireEvent.click(checkbox) + await waitFor(() => expect(saveButton).toBeEnabled()) + fireEvent.click(saveButton) + + const experimentPayloads = vi + .mocked(vscode.postMessage) + .mock.calls.filter(([message]) => (message as { type?: string }).type === "updateSettings") + .map( + ([message]) => + (message as { updatedSettings: { experiments?: Record } }).updatedSettings, + ) + .filter((settings) => settings.experiments !== undefined) + expect(experimentPayloads.length).toBeGreaterThanOrEqual(2) + expect(experimentPayloads[experimentPayloads.length - 1]?.experiments).toEqual({ + preventFocusDisruption: false, + }) + }) + + it("does not persist the experiments map when no experiment changed", () => { + const { activateTab, getSettingsContent } = renderSettingsView({ experiments: { imageGeneration: false } }) + + // A real non-experimental change guarantees Save is enabled and the handler runs. + activateTab("autoApprove") + fireEvent.click(within(getSettingsContent()).getByTestId("always-allow-execute-toggle")) + + activateTab("experimental") + fireEvent.click(screen.getByTestId("save-button")) + + const updateSettingsCalls = vi + .mocked(vscode.postMessage) + .mock.calls.filter(([message]) => (message as { type?: string }).type === "updateSettings") + expect(updateSettingsCalls.length).toBe(1) + expect( + (updateSettingsCalls[0][0] as { updatedSettings: Record }).updatedSettings.experiments, + ).toBeUndefined() + }) +}) + describe("SettingsView - openAiStrictToolSchemas save round trip", () => { beforeEach(() => { vi.clearAllMocks()