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 e94a66e1db..2bebb510ac 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -802,15 +802,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 26955663ee..57bbaba32b 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" @@ -158,6 +160,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) // The save waiting for the host's answer, and the form state it sent. The "unsaved changes" flag stays on until the @@ -252,9 +261,68 @@ 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. + // Retire a submitted baseline once the extension state confirms that value, and retire the + // edit session for that experiment with it: drop the id from the change set and let the + // confirmed live value stand in the cache. Without this, an experiment this view edited and + // saved stays in the change set, so a later unrelated Save compares this view's cached copy + // against a newer live value written elsewhere and posts the older copy back over it. + // A confirmation retires the session only while the cache still holds the submitted value: + // a toggle made after the save is a newer intent than the host's answer, so it keeps its own + // cache copy and its change-set entry, and the next Save posts it. + useEffect(() => { + const live = extensionState.experiments ?? experimentDefault + const confirmations: { id: ExperimentId; submitted: boolean; liveValue: boolean }[] = [] + for (const [id, submitted] of submittedExperiments.current) { + const liveValue = live[id] ?? experimentDefault[id] + if (liveValue !== submitted) { + continue + } + confirmations.push({ id, submitted, liveValue }) + } + const adopted = new Map() + for (const [id, liveValue] of Object.entries(live)) { + const experimentId = id as ExperimentId + if (submittedExperiments.current.has(experimentId) || changedExperiments.current.has(experimentId)) { + continue + } + adopted.set(experimentId, liveValue) + } + if (confirmations.length === 0 && adopted.size === 0) { + return + } + setCachedState((prevState) => { + // The cached copy is only readable here, and both ref deletes are idempotent, so React + // invoking this updater twice retires the same entries it would retire once. + const experiments = { ...prevState.experiments } as Record + for (const confirmation of confirmations) { + const cached = experiments[confirmation.id] ?? experimentDefault[confirmation.id] + submittedExperiments.current.delete(confirmation.id) + if (cached !== confirmation.submitted) { + // Newer than what was submitted: keep the user's copy and keep the entry in the + // change set, so the next Save still posts it instead of the host's older answer. + continue + } + changedExperiments.current.delete(confirmation.id) + if (experiments[confirmation.id] !== confirmation.liveValue) { + experiments[confirmation.id] = confirmation.liveValue + } + } + for (const [id, liveValue] of adopted) { + if (experiments[id] !== liveValue) { + experiments[id] = liveValue + } + } + return { ...prevState, experiments } + }) + }, [extensionState.experiments]) + // Bust the cache when settings are imported. useEffect(() => { if (!settingsImportedAt || handledSettingsImportedAt.current === settingsImportedAt) { @@ -262,6 +330,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]) @@ -325,6 +397,7 @@ const SettingsView = forwardRef(({ onDone, t } setChangeDetected(true) + changedExperiments.current.add(id) return { ...prevState, experiments: { ...prevState.experiments, [id]: enabled } } }) }, []) @@ -412,6 +485,22 @@ const SettingsView = forwardRef(({ onDone, t setPendingSaveId(saveId) } + // 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", requestId: saveId, @@ -488,7 +577,7 @@ const SettingsView = forwardRef(({ onDone, t imageGenerationProvider, openRouterImageApiKey, openRouterImageGenerationSelectedModel, - experiments, + experiments: Object.keys(experimentChanges).length > 0 ? experimentChanges : undefined, customSupportPrompts, }, }) @@ -499,6 +588,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) + } + } // With a time or date edit the flag is cleared when the host reports the save (see below). A failed save // leaves the user's edits in place to retry. if (!saveId) setChangeDetected(false) @@ -547,6 +645,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.experimentRetirement.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.experimentRetirement.spec.tsx new file mode 100644 index 0000000000..f38efecab0 --- /dev/null +++ b/webview-ui/src/components/settings/__tests__/SettingsView.experimentRetirement.spec.tsx @@ -0,0 +1,221 @@ +import type { ExperimentId } from "@roo-code/types" +import { EXPERIMENT_IDS } from "@roo/experiments" +import { act, fireEvent, screen, waitFor } from "@testing-library/react" +import { QueryClient } from "@tanstack/react-query" +import { beforeEach, describe, expect, it, vi } from "vitest" + +import { renderWithExtensionState } from "@/utils/test-utils" +import { useExtensionState } from "@src/context/ExtensionStateContext" +import { vscode } from "@src/utils/vscode" + +import SettingsView from "../SettingsView" + +vi.mock("@src/utils/vscode", () => ({ vscode: { postMessage: vi.fn() } })) +vi.mock("@/utils/TelemetryClient", () => ({ telemetryClient: { capture: vi.fn() } })) + +const postMessage = vi.mocked(vscode.postMessage) + +const posted = (type: string) => + postMessage.mock.calls + .map((call) => call[0] as { type: string; updatedSettings?: { experiments?: Record } }) + .filter((message) => message.type === type) + +type HostApi = { + setExperimentEnabled: (id: ExperimentId, enabled: boolean) => void + setCurrentApiConfigName: (name: string) => void +} + +let hostApi: HostApi | undefined + +function HostProbe() { + const state = useExtensionState() + hostApi = { + setExperimentEnabled: state.setExperimentEnabled, + setCurrentApiConfigName: state.setCurrentApiConfigName, + } + return null +} + +const IMAGE_LABEL = "settings:experimental.IMAGE_GENERATION.name" +const FOCUS_LABEL = "settings:experimental.PREVENT_FOCUS_DISRUPTION.name" +const IMAGE_ID = EXPERIMENT_IDS.IMAGE_GENERATION as ExperimentId +const FOCUS_ID = EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION as ExperimentId + +const renderExperimentalTab = () => + renderWithExtensionState( + <> + + + , + { + queryClient: new QueryClient({ defaultOptions: { queries: { retry: false } } }), + state: { experiments: { [IMAGE_ID]: false } }, + }, + ) + +/** The checkbox of the setting whose own label carries the given translation key. */ +const settingCheckbox = (labelKey: string) => + Array.from(document.querySelectorAll("input[type=checkbox]")).find((element) => { + const label = element.closest("label") + return !!label && String(label.textContent || "").includes(labelKey) + }) + +const requireCheckbox = (labelKey: string) => { + const checkbox = settingCheckbox(labelKey) + if (!checkbox) { + throw new Error("no checkbox rendered for " + labelKey) + } + return checkbox +} + +const toggleExperiment = async (labelKey: string) => { + await act(async () => { + fireEvent.click(requireCheckbox(labelKey)) + }) +} + +const clickSave = async () => { + await waitFor(() => expect(screen.getByTestId("save-button")).toBeEnabled()) + fireEvent.click(screen.getByTestId("save-button")) +} + +const openExperimentalTab = async () => { + renderExperimentalTab() + await waitFor(() => expect(screen.getByTestId("tab-experimental")).toBeInTheDocument()) + fireEvent.click(screen.getByTestId("tab-experimental")) + await waitFor(() => expect(settingCheckbox(IMAGE_LABEL)).toBeTruthy()) +} + +describe("SettingsView - experiment edits after the host confirms them", () => { + beforeEach(() => { + postMessage.mockClear() + hostApi = undefined + }) + + it("does not write a confirmed experiment back over a newer value from elsewhere", async () => { + await openExperimentalTab() + + // This view edits one experiment and saves it. + await toggleExperiment(IMAGE_LABEL) + await clickSave() + const firstSave = posted("updateSettings").at(-1) + expect(firstSave?.updatedSettings?.experiments).toEqual({ [IMAGE_ID]: true }) + + // The host confirms that value, and afterwards a different webview turns the same + // experiment off while this view is no longer editing it. + await act(async () => { + hostApi?.setExperimentEnabled(IMAGE_ID, true) + }) + await act(async () => { + hostApi?.setExperimentEnabled(IMAGE_ID, false) + }) + + // An unrelated save must not resurrect this view's copy of that experiment. + await toggleExperiment(FOCUS_LABEL) + await clickSave() + const secondExperiments = posted("updateSettings").at(-1)?.updatedSettings?.experiments ?? {} + expect(secondExperiments).not.toHaveProperty(String(IMAGE_ID)) + expect(secondExperiments).toEqual({ [FOCUS_ID]: false }) + }, 20000) + + it("keeps an experiment intent the host has not confirmed yet", async () => { + await openExperimentalTab() + + // Save posts the intent while the host state still reports the old value, so neither the + // submitted baseline nor the local copy may be dropped before that value arrives. + await toggleExperiment(IMAGE_LABEL) + await clickSave() + const firstSave = posted("updateSettings").at(-1) + expect(firstSave?.updatedSettings?.experiments).toEqual({ [IMAGE_ID]: true }) + expect(requireCheckbox(IMAGE_LABEL).checked).toBe(true) + + await toggleExperiment(FOCUS_LABEL) + await clickSave() + const secondExperiments = posted("updateSettings").at(-1)?.updatedSettings?.experiments ?? {} + expect(secondExperiments).toEqual({ [FOCUS_ID]: false }) + expect(requireCheckbox(IMAGE_LABEL).checked).toBe(true) + }, 20000) + + it("does not carry a pending experiment edit into another profile's save", async () => { + await openExperimentalTab() + + // An experiment is edited but never saved, and then the settings view switches to a + // different api configuration, which resets the edit session. + await toggleExperiment(IMAGE_LABEL) + await waitFor(() => expect(screen.getByTestId("save-button")).toBeEnabled()) + await act(async () => { + hostApi?.setCurrentApiConfigName("another-profile") + }) + await waitFor(() => expect(screen.getByTestId("save-button")).toBeDisabled()) + + // The next save belongs to the new session and posts only its own edit. + await toggleExperiment(FOCUS_LABEL) + await clickSave() + const experiments = posted("updateSettings").at(-1)?.updatedSettings?.experiments ?? {} + expect(experiments).not.toHaveProperty(String(IMAGE_ID)) + expect(experiments).toEqual({ [FOCUS_ID]: false }) + }, 20000) + + it("re-adopts a saved experiment when the host answer arrives after a profile change", async () => { + await openExperimentalTab() + + // The save is posted, but a profile snapshot arrives before the host answer reaches this + // view, so the cache reverts to the live value and the session resets. + await toggleExperiment(IMAGE_LABEL) + await clickSave() + const firstSave = posted("updateSettings").at(-1) + expect(firstSave?.updatedSettings?.experiments).toEqual({ [IMAGE_ID]: true }) + await act(async () => { + hostApi?.setCurrentApiConfigName("another-profile") + }) + await waitFor(() => expect(screen.getByTestId("save-button")).toBeDisabled()) + expect(requireCheckbox(IMAGE_LABEL).checked).toBe(false) + + // The answer to the earlier save then arrives: the persisted value is the saved one, so + // the view has to show it and must not post the reverted value back on a later save. + await act(async () => { + hostApi?.setExperimentEnabled(IMAGE_ID, true) + }) + await waitFor(() => expect(requireCheckbox(IMAGE_LABEL).checked).toBe(true)) + await toggleExperiment(FOCUS_LABEL) + await clickSave() + const experiments = posted("updateSettings").at(-1)?.updatedSettings?.experiments ?? {} + expect(experiments).not.toHaveProperty(String(IMAGE_ID)) + expect(experiments).toEqual({ [FOCUS_ID]: false }) + }, 20000) + + it("keeps a toggle made after the save when the host confirms the earlier value", async () => { + await openExperimentalTab() + + // Save posts the experiment on, and the user toggles it back off before the host answers. + await toggleExperiment(IMAGE_LABEL) + await clickSave() + const firstSave = posted("updateSettings").at(-1) + expect(firstSave?.updatedSettings?.experiments).toEqual({ [IMAGE_ID]: true }) + await toggleExperiment(IMAGE_LABEL) + expect(requireCheckbox(IMAGE_LABEL).checked).toBe(false) + + // The answer to the earlier save arrives: it confirms the value that was submitted, not the + // value the user wants now, so it may neither flip the checkbox nor retire the edit. + await act(async () => { + hostApi?.setExperimentEnabled(IMAGE_ID, true) + }) + await waitFor(() => expect(requireCheckbox(IMAGE_LABEL).checked).toBe(false)) + + // The newer intent is still an edit this view owes the host, so the next save posts it. + await clickSave() + const secondSave = posted("updateSettings").at(-1) + expect(secondSave?.updatedSettings?.experiments).toEqual({ [IMAGE_ID]: false }) + + // Once the host confirms that value, the session is retired and a later save leaves the + // experiment out of the payload entirely. + await act(async () => { + hostApi?.setExperimentEnabled(IMAGE_ID, false) + }) + await toggleExperiment(FOCUS_LABEL) + await clickSave() + const thirdExperiments = posted("updateSettings").at(-1)?.updatedSettings?.experiments ?? {} + expect(thirdExperiments).not.toHaveProperty(String(IMAGE_ID)) + expect(thirdExperiments).toEqual({ [FOCUS_ID]: false }) + }, 20000) +}) diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx index d8958f87c6..808f0bfc5d 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -52,17 +52,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" ? ( ) : ( - ), @@ -196,8 +197,13 @@ vi.mock("@/components/ui", () => ({ data-testid={dataTestId} /> ), - Button: ({ children, onClick, variant, className, "data-testid": dataTestId }: any) => ( - ), @@ -1056,6 +1062,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()