From a3c6f86ba4e0a54dc78f9dd3c8b17ae5ba628b3c Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 27 Aug 2026 11:43:04 +0800 Subject: [PATCH 01/13] feat(experiments): make chat-diff the default approval path (L2, #1375) Flips the PREVENT_FOCUS_DISRUPTION experiment default from false to true so the chat-diff approval path (approve in chat, save directly, no diff-editor focus) is the default. The diff-editor path remains available by toggling the experiment off; the storage key is unchanged so saved values are preserved. Production call sites already route through experiments.isEnabled(... ?? {}), so the flip applies automatically. Test-only changes pin the legacy path explicitly where base mocks relied on the old default. --- src/core/tools/__tests__/editFileTool.spec.ts | 4 +- src/core/tools/__tests__/editTool.spec.ts | 37 ++++++++++++++++++- .../tools/__tests__/searchReplaceTool.spec.ts | 4 +- .../tools/__tests__/writeToFileTool.spec.ts | 3 ++ .../editor/__tests__/DiffViewProvider.spec.ts | 5 ++- ...experiments-preventFocusDisruption.spec.ts | 12 +++--- src/shared/__tests__/experiments.spec.ts | 4 +- src/shared/experiments.ts | 5 ++- 8 files changed, 60 insertions(+), 14 deletions(-) diff --git a/src/core/tools/__tests__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index 1ff8d52a8d..30244560f4 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, }), }), } diff --git a/src/core/tools/__tests__/editTool.spec.ts b/src/core/tools/__tests__/editTool.spec.ts index a5f665b9e5..8be210fe7a 100644 --- a/src/core/tools/__tests__/editTool.spec.ts +++ b/src/core/tools/__tests__/editTool.spec.ts @@ -111,7 +111,9 @@ describe("editTool", () => { getState: vi.fn().mockResolvedValue({ diagnosticsEnabled: true, writeDelayMs: 1000, - experiments: {}, + // Pin the legacy diff-editor path explicitly: the PREVENT_FOCUS_DISRUPTION default + // flipped to true (L2, plan #33), so these tests must opt out. + experiments: { preventFocusDisruption: false } as Record, }), }), } @@ -361,6 +363,39 @@ describe("editTool", () => { }) }) + describe("focus disruption default (L2: chat-diff is the default approval path)", () => { + it("saves via saveDirectly without opening the diff editor when no experiment value is stored", async () => { + mockAskApproval.mockResolvedValue(true) + // No stored experiment value: the default (flipped to true in L2) resolves + // to the chat-diff path. + mockTask.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: {}, + }), + }) + + await executeEditTool() + + expect(mockTask.diffViewProvider.saveDirectly).toHaveBeenCalled() + expect(mockTask.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockTask.didEditFile).toBe(true) + }) + + it("still uses the diff-editor path when the user has opted out (stored false)", async () => { + mockAskApproval.mockResolvedValue(true) + // Base mock pins experiments: { preventFocusDisruption: false } (legacy path). + + await executeEditTool() + + expect(mockTask.diffViewProvider.open).toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveChanges).toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + }) + }) + describe("partial block handling", () => { it("handles partial block without errors after path stabilizes", async () => { // Path stabilization requires two consecutive calls with the same path diff --git a/src/core/tools/__tests__/searchReplaceTool.spec.ts b/src/core/tools/__tests__/searchReplaceTool.spec.ts index 5cf10790d4..d83c5c5aa5 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, }), }), } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 52a7e3c052..62c00427fb 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 }, }), }), } diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index aee88f4061..469fcffa2f 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).toHaveBeenCalled() }) 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 }, From 165bff4dd9ae126788fb60942d35d13bff6dcd48 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 27 Aug 2026 12:18:16 +0800 Subject: [PATCH 02/13] test(experiments): cover unset-experiment default routing in all write tool suites (L2, #1375) --- src/core/tools/__tests__/editFileTool.spec.ts | 22 ++++++++++++++++++ .../tools/__tests__/searchReplaceTool.spec.ts | 22 ++++++++++++++++++ .../tools/__tests__/writeToFileTool.spec.ts | 23 +++++++++++++++++++ .../editor/__tests__/DiffViewProvider.spec.ts | 2 +- 4 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/core/tools/__tests__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index 30244560f4..cbececbbd0 100644 --- a/src/core/tools/__tests__/editFileTool.spec.ts +++ b/src/core/tools/__tests__/editFileTool.spec.ts @@ -578,6 +578,28 @@ 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) + }) + }) + 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 d83c5c5aa5..981caa350e 100644 --- a/src/core/tools/__tests__/searchReplaceTool.spec.ts +++ b/src/core/tools/__tests__/searchReplaceTool.spec.ts @@ -332,6 +332,28 @@ 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) + }) + }) + 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 62c00427fb..47234ad253 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -159,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 ( @@ -367,6 +368,28 @@ 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) + }) + }) + 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 469fcffa2f..36e698f0d2 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -820,7 +820,7 @@ describe("DiffViewProvider", () => { // 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).toHaveBeenCalled() + expect(vscode.workspace.openTextDocument).toHaveBeenCalledWith(vscode.Uri.file(`${mockCwd}/test.ts`)) }) it("should skip diagnostics when diagnosticsEnabled is false", async () => { From c0ef476be42ec3eb7314934c9d25c8ac07370319 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 27 Aug 2026 12:25:25 +0800 Subject: [PATCH 03/13] test(experiments): add explicit enabled-state routing tests for chat-diff default (L2, #1375) --- src/core/tools/__tests__/editFileTool.spec.ts | 19 +++++++++++++++++++ .../tools/__tests__/searchReplaceTool.spec.ts | 19 +++++++++++++++++++ .../tools/__tests__/writeToFileTool.spec.ts | 19 +++++++++++++++++++ 3 files changed, 57 insertions(+) diff --git a/src/core/tools/__tests__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index cbececbbd0..2d88649548 100644 --- a/src/core/tools/__tests__/editFileTool.spec.ts +++ b/src/core/tools/__tests__/editFileTool.spec.ts @@ -598,6 +598,25 @@ describe("editFileTool", () => { 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", () => { diff --git a/src/core/tools/__tests__/searchReplaceTool.spec.ts b/src/core/tools/__tests__/searchReplaceTool.spec.ts index 981caa350e..10b8c9028a 100644 --- a/src/core/tools/__tests__/searchReplaceTool.spec.ts +++ b/src/core/tools/__tests__/searchReplaceTool.spec.ts @@ -352,6 +352,25 @@ describe("searchReplaceTool", () => { 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", () => { diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 47234ad253..37a6b7b102 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -388,6 +388,25 @@ describe("writeToFileTool", () => { 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", () => { From d038774387d7fb39e73afe140b436d5f4ac2c038 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sun, 30 Aug 2026 12:33:09 +0800 Subject: [PATCH 04/13] =?UTF-8?q?chore(ci):=20empty=20commit=20=E2=80=94?= =?UTF-8?q?=20re-trigger=20CI=20and=20the=20CodeRabbit=20current-head=20re?= =?UTF-8?q?view=20gate=20(no=20code=20change)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From 5b80cbf13431e6279bbe2e653a770403cc85d24e Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 27 Oct 2026 06:45:09 +0800 Subject: [PATCH 05/13] fix(settings): persist only the experiments the user toggled Human review follow-up: SettingsView used to send the whole live experiments map on every Save, so the extension persisted every experiment at its then-current default and a later default flip could not reach users who had saved before. Track the ids toggled during the edit session and send only those (baseline compared against live state or experimentDefault); omit the key when nothing changed. The handler already merges partial maps over the stored experiments. Tests cover a default-on experiment toggled off and a no-change Save; SettingsView.spec 24 passed; webview tsc clean. --- .../src/components/settings/SettingsView.tsx | 25 +++++++++- .../settings/__tests__/SettingsView.spec.tsx | 49 +++++++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-) diff --git a/webview-ui/src/components/settings/SettingsView.tsx b/webview-ui/src/components/settings/SettingsView.tsx index 61ba954dd5..c87cb6c735 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,10 @@ 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()) const [cachedState, setCachedState] = useState(() => extensionState) @@ -234,6 +240,7 @@ const SettingsView = forwardRef(({ onDone, t setCachedState((prevCachedState) => ({ ...prevCachedState, ...extensionState })) prevApiConfigName.current = currentApiConfigName prevMode.current = mode + changedExperiments.current.clear() setChangeDetected(false) }, [currentApiConfigName, mode, extensionState]) @@ -244,6 +251,7 @@ const SettingsView = forwardRef(({ onDone, t } handledSettingsImportedAt.current = settingsImportedAt + changedExperiments.current.clear() setCachedState((prevCachedState) => ({ ...prevCachedState, ...extensionState })) setChangeDetected(false) }, [settingsImportedAt, extensionState]) @@ -307,6 +315,7 @@ const SettingsView = forwardRef(({ onDone, t } setChangeDetected(true) + changedExperiments.current.add(id) return { ...prevState, experiments: { ...prevState.experiments, [id]: enabled } } }) }, []) @@ -381,6 +390,18 @@ 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] + // Compare against the live/default baseline, not a bare false: a user toggling + // a default-on experiment off is a real change that must be persisted. + if (value !== (extensionState.experiments?.[id] ?? experimentDefault[id])) { + experimentChanges[id] = value + } + } + vscode.postMessage({ type: "updateSettings", updatedSettings: { @@ -452,7 +473,7 @@ const SettingsView = forwardRef(({ onDone, t imageGenerationProvider, openRouterImageApiKey, openRouterImageGenerationSelectedModel, - experiments, + experiments: Object.keys(experimentChanges).length > 0 ? experimentChanges : undefined, customSupportPrompts, }, }) @@ -463,6 +484,7 @@ const SettingsView = forwardRef(({ onDone, t vscode.postMessage({ type: "telemetrySetting", text: telemetrySetting }) vscode.postMessage({ type: "debugSetting", bool: cachedState.debug }) + changedExperiments.current.clear() setChangeDetected(false) } } @@ -486,6 +508,7 @@ const SettingsView = forwardRef(({ onDone, t if (confirm) { // Discard changes: Reset state and flag setCachedState(extensionState) // Revert to original state + 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 528cbda497..698ca822c5 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -912,3 +912,52 @@ describe("SettingsView - Blanket Auto-Deny", () => { ) }) }) + +describe("SettingsView - Experiments", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("persists only the experiment the user toggled, not the whole map", () => { + const { activateTab, getSettingsContent } = renderSettingsView({ experiments: { imageGeneration: false } }) + + activateTab("experimental") + const content = getSettingsContent() + + // Toggle the chat-diff experiment through the settings control. + const label = 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) + + fireEvent.click(screen.getByTestId("save-button")) + + // 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("does not persist the experiments map when no experiment changed", () => { + const { activateTab } = renderSettingsView({ experiments: { imageGeneration: false } }) + + 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() + }) +}) From 56ad004cde88772d11c857ba7adb29e427fb0218 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 27 Oct 2026 07:25:19 +0800 Subject: [PATCH 06/13] fix(settings): keep submitted experiment ids until the host state reflects them Review follow-ups: handleSubmit no longer clears the changed-experiments set right after posting, so a second Save compares against the last submitted baseline instead of stale extensionState; the set still clears on host re-sync and settings import. The experiment Save tests now also change a non-experimental setting first so Save is enabled and the submit handler runs deterministically. SettingsView.spec 33 passed; webview tsc clean. --- webview-ui/src/components/settings/SettingsView.tsx | 3 ++- .../settings/__tests__/SettingsView.spec.tsx | 10 +++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/webview-ui/src/components/settings/SettingsView.tsx b/webview-ui/src/components/settings/SettingsView.tsx index 68d0f45f3f..561cff7ad4 100644 --- a/webview-ui/src/components/settings/SettingsView.tsx +++ b/webview-ui/src/components/settings/SettingsView.tsx @@ -486,7 +486,8 @@ const SettingsView = forwardRef(({ onDone, t vscode.postMessage({ type: "telemetrySetting", text: telemetrySetting }) vscode.postMessage({ type: "debugSetting", bool: cachedState.debug }) - changedExperiments.current.clear() + // Keep the submitted ids until the extension state reflects them: a second Save in + // the same tick must compare against the last submitted values, not stale host state. setChangeDetected(false) } } diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx index 6273798266..adaa2cfdaa 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -988,6 +988,10 @@ describe("SettingsView - Experiments", () => { it("persists only the experiment the user toggled, not the whole map", () => { const { activateTab, getSettingsContent } = renderSettingsView({ experiments: { imageGeneration: false } }) + // Make a non-experimental change first so Save is enabled and the submit handler runs. + activateTab("autoApprove") + fireEvent.click(within(getSettingsContent()).getByTestId("always-allow-execute-toggle")) + activateTab("experimental") const content = getSettingsContent() @@ -1014,7 +1018,11 @@ describe("SettingsView - Experiments", () => { }) it("does not persist the experiments map when no experiment changed", () => { - const { activateTab } = renderSettingsView({ experiments: { imageGeneration: false } }) + 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")) From ed047f0a3713899a90a6c6d1ea05ad8979141307 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 27 Oct 2026 07:42:13 +0800 Subject: [PATCH 07/13] test(settings): forward Save disabled state and prove the experiment toggle alone enables it Review follow-up: the test-local Button mock forwards disabled; the experiment Save test asserts Save is disabled on load, that toggling preventFocusDisruption alone enables it, and that the payload carries only the toggled experiment. SettingsView.spec 33 passed; webview tsc clean. --- .../settings/__tests__/SettingsView.spec.tsx | 35 +++++++++++-------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx index adaa2cfdaa..aa4718f34d 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) => ( - ), @@ -985,24 +991,25 @@ describe("SettingsView - Experiments", () => { vi.clearAllMocks() }) - it("persists only the experiment the user toggled, not the whole map", () => { + it("persists only the experiment the user toggled, not the whole map", async () => { const { activateTab, getSettingsContent } = renderSettingsView({ experiments: { imageGeneration: false } }) - - // Make a non-experimental change first so Save is enabled and the submit handler runs. - activateTab("autoApprove") - fireEvent.click(within(getSettingsContent()).getByTestId("always-allow-execute-toggle")) - activateTab("experimental") - const content = getSettingsContent() - // Toggle the chat-diff experiment through the settings control. - const label = within(content).getByText("settings:experimental.PREVENT_FOCUS_DISRUPTION.name").closest("label") + 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()) - fireEvent.click(screen.getByTestId("save-button")) + 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. From 00d6ec6916f9c1263164016d9497a24fd0f391eb Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 27 Oct 2026 07:52:42 +0800 Subject: [PATCH 08/13] test(settings): cover buffered experiments and the stored-false reversed toggle Review follow-up: the experiment Save test asserts nothing is persisted before Save, keeps the default-on to false case, and adds a stored-false hydrated control toggled on that submits true. SettingsView.spec 34 passed; webview tsc clean. --- .../settings/__tests__/SettingsView.spec.tsx | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx index aa4718f34d..50201fe35f 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -1009,6 +1009,14 @@ describe("SettingsView - Experiments", () => { 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 @@ -1024,6 +1032,34 @@ describe("SettingsView - Experiments", () => { ) }) + 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("does not persist the experiments map when no experiment changed", () => { const { activateTab, getSettingsContent } = renderSettingsView({ experiments: { imageGeneration: false } }) From d0e245a597a9ccdcab02971c514018025d2cb6d5 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 27 Oct 2026 08:11:37 +0800 Subject: [PATCH 09/13] fix(experiments): compare against the last submitted value and cover ApplyDiffTool routing Review follow-ups: SettingsView keeps a last-submitted per-experiment baseline until the extension state confirms it, with a regression test for a follow-up toggle before the state round-trip. New tool-level ApplyDiffTool spec covers chat-diff vs diff-editor success/rejection routing for experiments {} and {preventFocusDisruption:false}. SettingsView.spec 35 passed; applyDiffTool.experiments.spec 4 passed; tsc clean. --- .../applyDiffTool.experiments.spec.ts | 169 ++++++++++++++++++ .../src/components/settings/SettingsView.tsx | 33 +++- .../settings/__tests__/SettingsView.spec.tsx | 34 ++++ 3 files changed, 231 insertions(+), 5 deletions(-) create mode 100644 src/core/tools/__tests__/applyDiffTool.experiments.spec.ts 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..2f576be797 --- /dev/null +++ b/src/core/tools/__tests__/applyDiffTool.experiments.spec.ts @@ -0,0 +1,169 @@ +import fs from "fs/promises" + +import type { DiffResult, ToolResponse } from "../../../shared/tools" +import type { Task } from "../../task/Task" +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 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) + + mockTask = { + 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), + }, + } 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() + }) + + 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/webview-ui/src/components/settings/SettingsView.tsx b/webview-ui/src/components/settings/SettingsView.tsx index 561cff7ad4..9be17ea07e 100644 --- a/webview-ui/src/components/settings/SettingsView.tsx +++ b/webview-ui/src/components/settings/SettingsView.tsx @@ -156,6 +156,9 @@ const SettingsView = forwardRef(({ onDone, t // 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) @@ -245,6 +248,15 @@ const SettingsView = forwardRef(({ onDone, t 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) { @@ -396,9 +408,13 @@ const SettingsView = forwardRef(({ onDone, t const experimentChanges: Record = {} for (const id of changedExperiments.current) { const value = experiments?.[id] ?? experimentDefault[id] - // Compare against the live/default baseline, not a bare false: a user toggling - // a default-on experiment off is a real change that must be persisted. - if (value !== (extensionState.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 } } @@ -486,8 +502,15 @@ const SettingsView = forwardRef(({ onDone, t vscode.postMessage({ type: "telemetrySetting", text: telemetrySetting }) vscode.postMessage({ type: "debugSetting", bool: cachedState.debug }) - // Keep the submitted ids until the extension state reflects them: a second Save in - // the same tick must compare against the last submitted values, not stale host state. + // 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) } } diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx index 50201fe35f..920ee2caad 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -1060,6 +1060,40 @@ describe("SettingsView - Experiments", () => { ) }) + 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("does not persist the experiments map when no experiment changed", () => { const { activateTab, getSettingsContent } = renderSettingsView({ experiments: { imageGeneration: false } }) From a8b349d71f8a825e7748c11713c48d9ae7931408 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 27 Oct 2026 08:35:37 +0800 Subject: [PATCH 10/13] test(experiments): type the ApplyDiffTool fixture and clear pending baselines on import Review follow-ups: structural TaskFixture types replace the unchecked member casts with one documented construction-site assertion and a typed provider stub; the opt-out success test asserts the pushed result; SettingsView clears the last-submitted experiment baseline when settings are imported, with a regression test. applyDiffTool.experiments 4 passed; SettingsView.spec 36 passed; tsc clean. --- .../applyDiffTool.experiments.spec.ts | 55 ++++++++++++++++++- .../src/components/settings/SettingsView.tsx | 3 + .../settings/__tests__/SettingsView.spec.tsx | 42 ++++++++++++++ 3 files changed, 98 insertions(+), 2 deletions(-) diff --git a/src/core/tools/__tests__/applyDiffTool.experiments.spec.ts b/src/core/tools/__tests__/applyDiffTool.experiments.spec.ts index 2f576be797..fa2a7949c1 100644 --- a/src/core/tools/__tests__/applyDiffTool.experiments.spec.ts +++ b/src/core/tools/__tests__/applyDiffTool.experiments.spec.ts @@ -2,6 +2,53 @@ 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" @@ -47,6 +94,7 @@ 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> @@ -63,7 +111,7 @@ describe("ApplyDiffTool focus-disruption routing (L2)", () => { mockAskApproval = vi.fn().mockImplementation(async () => approved) mockHandleError = vi.fn().mockResolvedValue(undefined) - mockTask = { + fixture = { cwd: "/workspace", api: { getModel: () => ({ id: "claude-test" }) }, consecutiveMistakeCount: 0, @@ -93,7 +141,9 @@ describe("ApplyDiffTool focus-disruption routing (L2)", () => { getName: () => "test", applyDiff: vi.fn().mockResolvedValue({ success: true, content: "updated content" } as DiffResult), }, - } as unknown as Task + } + // One documented construction-site assertion: every member above is individually typed. + mockTask = fixture as unknown as Task }) function setExperiments(experiments: Record) { @@ -141,6 +191,7 @@ describe("ApplyDiffTool focus-disruption routing (L2)", () => { 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 () => { diff --git a/webview-ui/src/components/settings/SettingsView.tsx b/webview-ui/src/components/settings/SettingsView.tsx index 9be17ea07e..fdb332f72c 100644 --- a/webview-ui/src/components/settings/SettingsView.tsx +++ b/webview-ui/src/components/settings/SettingsView.tsx @@ -265,6 +265,9 @@ 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]) diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx index 920ee2caad..ac5ac66356 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -1094,6 +1094,48 @@ describe("SettingsView - Experiments", () => { 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("does not persist the experiments map when no experiment changed", () => { const { activateTab, getSettingsContent } = renderSettingsView({ experiments: { imageGeneration: false } }) From cebc4feacb99424274bc34d2bde8bc1de4a9b39f Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 27 Oct 2026 09:04:24 +0800 Subject: [PATCH 11/13] fix(experiments): clear pending baselines on session reset and cover ApplyPatchTool routing Review follow-ups: submittedExperiments clears on API/profile-mode resync, import, and confirmed discard, with regression tests for the two reset paths; new tool-layer ApplyPatchTool spec covers add-file and update-file direct vs diff-editor routing for experiments {} and {preventFocusDisruption:false}. SettingsView.spec 38 passed; applyPatchTool.experiments 4 passed; tsc clean. --- .../applyPatchTool.experiments.spec.ts | 201 ++++++++++++++++++ .../src/components/settings/SettingsView.tsx | 3 + .../settings/__tests__/SettingsView.spec.tsx | 85 ++++++++ 3 files changed, 289 insertions(+) create mode 100644 src/core/tools/__tests__/applyPatchTool.experiments.spec.ts 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..78629e256b --- /dev/null +++ b/src/core/tools/__tests__/applyPatchTool.experiments.spec.ts @@ -0,0 +1,201 @@ +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) { + await tool.execute({ patch }, fixture as unknown as Task, { + askApproval: vi.fn().mockImplementation(async () => approved), + handleError: vi.fn().mockResolvedValue(undefined), + pushToolResult: vi.fn(), + toolCallId: "tool-call-1", + }) + } + + it("adds a file through the direct save when no experiment value is stored", async () => { + setExperiments({}) + vi.mocked(fileExistsAtPath).mockResolvedValue(false) + + 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) + }) + + it("adds a file through the diff editor when the user opted out", async () => { + setExperiments({ preventFocusDisruption: false }) + vi.mocked(fileExistsAtPath).mockResolvedValue(false) + + 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() + }) + + it("updates a file through the direct save when no experiment value is stored", async () => { + setExperiments({}) + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + + 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) + }) + + it("updates a file through the diff editor when the user opted out", async () => { + setExperiments({ preventFocusDisruption: false }) + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + + 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() + }) +}) diff --git a/webview-ui/src/components/settings/SettingsView.tsx b/webview-ui/src/components/settings/SettingsView.tsx index fdb332f72c..f8fd28d9fc 100644 --- a/webview-ui/src/components/settings/SettingsView.tsx +++ b/webview-ui/src/components/settings/SettingsView.tsx @@ -245,6 +245,8 @@ const SettingsView = forwardRef(({ onDone, t 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]) @@ -537,6 +539,7 @@ 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 ac5ac66356..683d135209 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -1136,6 +1136,91 @@ describe("SettingsView - Experiments", () => { 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) + + // Confirming Discard resets the settings edit session. + fireEvent.click(screen.getByText("settings:common.done")) + 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 } }) From 11913c97f3774c153f04f6f50223f3ee156ff256 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 27 Oct 2026 09:17:09 +0800 Subject: [PATCH 12/13] test(experiments): cover ApplyPatchTool rejections and the confirmed-discard reset Review follow-ups: ApplyPatchTool tests add rejected add/update cases for unset and opted-out experiments (no save on rejection, diff editor reverted on opt-out) and verify the approved success message via pushToolResult/handleError; the confirmed-discard test makes a new unsaved edit, asserts the dialog opens, then confirms the reset. applyPatchTool.experiments 8 passed; SettingsView.spec 38 passed. --- .../applyPatchTool.experiments.spec.ts | 71 +++++++++++++++++-- .../settings/__tests__/SettingsView.spec.tsx | 6 +- 2 files changed, 70 insertions(+), 7 deletions(-) diff --git a/src/core/tools/__tests__/applyPatchTool.experiments.spec.ts b/src/core/tools/__tests__/applyPatchTool.experiments.spec.ts index 78629e256b..cc4a27b913 100644 --- a/src/core/tools/__tests__/applyPatchTool.experiments.spec.ts +++ b/src/core/tools/__tests__/applyPatchTool.experiments.spec.ts @@ -137,43 +137,50 @@ describe("ApplyPatchTool focus-disruption routing (L2)", () => { } 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: vi.fn().mockResolvedValue(undefined), - pushToolResult: vi.fn(), + 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) - await runPatch(addPatch) + 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) - await runPatch(addPatch) + 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) - await runPatch(updatePatch) + const { pushToolResult, handleError } = await runPatch(updatePatch) expect(fixture.diffViewProvider.saveDirectly).toHaveBeenCalledWith( "src/existing.ts", @@ -191,11 +198,63 @@ describe("ApplyPatchTool focus-disruption routing (L2)", () => { setExperiments({ preventFocusDisruption: false }) vi.mocked(fileExistsAtPath).mockResolvedValue(true) - await runPatch(updatePatch) + 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/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx index 683d135209..79e27f74ef 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -1196,8 +1196,12 @@ describe("SettingsView - Experiments", () => { await waitFor(() => expect(saveButton).toBeEnabled()) fireEvent.click(saveButton) - // Confirming Discard resets the settings edit session. + // 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()) From 46d2868cf6df80bbb368485f21325481fd948c01 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 27 Oct 2026 09:31:55 +0800 Subject: [PATCH 13/13] test(experiments): assert the update-file direct-save success result Lint follow-up: the update-file direct-save test now asserts pushToolResult received the success message and handleError was not called (the destructured mocks must be used). applyPatchTool.experiments 8 passed; eslint 0. --- src/core/tools/__tests__/applyPatchTool.experiments.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/core/tools/__tests__/applyPatchTool.experiments.spec.ts b/src/core/tools/__tests__/applyPatchTool.experiments.spec.ts index cc4a27b913..c875559e73 100644 --- a/src/core/tools/__tests__/applyPatchTool.experiments.spec.ts +++ b/src/core/tools/__tests__/applyPatchTool.experiments.spec.ts @@ -192,6 +192,8 @@ describe("ApplyPatchTool focus-disruption routing (L2)", () => { 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 () => {