From a3c6f86ba4e0a54dc78f9dd3c8b17ae5ba628b3c Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 27 Aug 2026 11:43:04 +0800 Subject: [PATCH 01/16] 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/16] 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/16] 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/16] =?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/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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 () => { From 395dc1e8004213d233ebe3a68ba44ccf0c7e4ece Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 8 Nov 2026 08:29:09 +0800 Subject: [PATCH 14/16] fix(webview): retire confirmed experiment edits from the change set An experiment stayed in SettingsView's change set after the host confirmed its value, so a later unrelated Save compared this view's cached copy against a newer live value and posted the older copy back over it. A confirmed experiment now leaves the change set and the confirmed value takes its place in the cache. Experiments that are still being edited, or submitted and not yet confirmed, keep their cached value, so a newer unsaved edit is never dropped, and a value the host confirms after a profile change is adopted rather than written back. Adds SettingsView.experimentRetirement.spec.tsx: four tests. Two of them fail at 41bcddb98d (the stale re-post, and the reverted checkbox after a profile change) and pass here; the other two pin the pending-intent and profile-reset contracts. They also kill the CallExpression mutant on changedExperiments.current.clear() at SettingsView.tsx:264 that the mutation advisory reported as surviving. Answers the review rows at SettingsView.tsx:455 and SettingsView.tsx:268. This push also re-runs this branch's required contexts: the workflow cannot be re-run from the API (403 "Must have admin rights to Repository."). --- .../src/components/settings/SettingsView.tsx | 36 +++- ...SettingsView.experimentRetirement.spec.tsx | 186 ++++++++++++++++++ 2 files changed, 220 insertions(+), 2 deletions(-) create mode 100644 webview-ui/src/components/settings/__tests__/SettingsView.experimentRetirement.spec.tsx diff --git a/webview-ui/src/components/settings/SettingsView.tsx b/webview-ui/src/components/settings/SettingsView.tsx index c42991b388..956d32a6c2 100644 --- a/webview-ui/src/components/settings/SettingsView.tsx +++ b/webview-ui/src/components/settings/SettingsView.tsx @@ -268,12 +268,44 @@ const SettingsView = forwardRef(({ onDone, t }, [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. + // Experiments that are still being edited, or submitted and not yet confirmed, keep their + // cached value, so a newer unsaved edit is never dropped. useEffect(() => { + const live = extensionState.experiments ?? experimentDefault + const confirmed = new Map() for (const [id, value] of submittedExperiments.current) { - if ((extensionState.experiments?.[id] ?? experimentDefault[id]) === value) { - submittedExperiments.current.delete(id) + const liveValue = live[id] ?? experimentDefault[id] + if (liveValue !== value) { + continue } + submittedExperiments.current.delete(id) + changedExperiments.current.delete(id) + confirmed.set(id, liveValue) } + for (const [id, liveValue] of Object.entries(live)) { + const experimentId = id as ExperimentId + if (submittedExperiments.current.has(experimentId) || changedExperiments.current.has(experimentId)) { + continue + } + confirmed.set(experimentId, liveValue) + } + if (confirmed.size === 0) { + return + } + setCachedState((prevState) => { + const experiments = { ...prevState.experiments } as Record + for (const [id, liveValue] of confirmed) { + if (experiments[id] !== liveValue) { + experiments[id] = liveValue + } + } + return { ...prevState, experiments } + }) }, [extensionState.experiments]) // Bust the cache when settings are imported. 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..7f9e4bfba0 --- /dev/null +++ b/webview-ui/src/components/settings/__tests__/SettingsView.experimentRetirement.spec.tsx @@ -0,0 +1,186 @@ +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) +}) From 710a443d1f08390eea9641592c3b47c944ced084 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 8 Nov 2026 09:15:22 +0800 Subject: [PATCH 15/16] chore(webview): re-trigger the ubuntu unit leg on an unchanged tree The ubuntu platform-unit-test leg for this head (job 128576671104) reported 188 test files and 2,498 tests all passed with no FAIL line in the log, and exited 1 on a single unhandled rejection: EnvironmentTeardownError: [vitest-worker]: Closing rpc while "onUserConsoleLog" was pending raised in the @roo-code/vscode-webview:test:coverage target; the windows leg 128576670981 was cancelled downstream of it by the matrix fail-fast. The same leg was green at 41bcddb98d on the same runner, and the failing target's own command (vitest run --coverage over webview-ui) exits 0 here on the pinned Node 22.23.4 with the same 188 files / 2,498 tests. This commit changes no files. It exists because POST /actions/runs//rerun-failed-jobs answers 403 "Must have admin rights to Repository" for us, so a push is the only re-trigger available. The commit points at the same tree as 395dc1e80, so the gates already run on that tree - vitest src/components/settings 61 files / 787 tests, whole-webview coverage 188 files / 2,498 tests, eslint --max-warnings=0 on both changed files, tsc --noEmit, all on pinned Node 22.23.4 and repeated on box Node 25.9.0 - carry over unchanged. No gate is re-run for this push beyond the repository's own pre-push check-types, which runs on the pinned Node 22.23.4 on PATH. From caabac7fab523dd8230e884bf2dffe5111c4382b Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 8 Nov 2026 09:51:11 +0800 Subject: [PATCH 16/16] fix(webview): retire a confirmed experiment only while the cache still holds it The confirmation effect gated retirement on one comparison: the live value against the value this view submitted. The cache copy was never consulted, so a toggle made after the save - which re-adds the id to the change set and writes the newer value - was overwritten when the host answer for the earlier value arrived, and its change-set entry was deleted in the same step. Since the save payload is built by iterating the change set, that next save then posted nothing for the experiment and the user's newer intent was lost. Retire the session inside the state updater, which is the only place the authoritative cached copy is readable: when the cache still equals the submitted value the session ends and the confirmed live value stands; when it does not, the submitted baseline is retired while the change-set entry survives, so the next save posts the newer value instead of the host's older answer. Both ref deletes are idempotent, so React invoking the updater twice retires the same entries it would retire once - that is what makes the decision safe to take inside the updater rather than outside it. Test: keeps a toggle made after the save when the host confirms the earlier value. Against the previous head it fails with "expected true to be false" on the checkbox; with this fix the file is 5 tests green on pinned Node 22.23.4 and on box Node 25.9.0, webview-ui src/components/settings is 61 files / 788 tests, and the package's own leg command (vitest run --coverage) is 188 files / 2,499 tests with no failing file on 22.23.4. --- .../src/components/settings/SettingsView.tsx | 37 +++++++++++++------ ...SettingsView.experimentRetirement.spec.tsx | 35 ++++++++++++++++++ 2 files changed, 61 insertions(+), 11 deletions(-) diff --git a/webview-ui/src/components/settings/SettingsView.tsx b/webview-ui/src/components/settings/SettingsView.tsx index 956d32a6c2..57bbaba32b 100644 --- a/webview-ui/src/components/settings/SettingsView.tsx +++ b/webview-ui/src/components/settings/SettingsView.tsx @@ -273,33 +273,48 @@ const SettingsView = forwardRef(({ onDone, t // 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. - // Experiments that are still being edited, or submitted and not yet confirmed, keep their - // cached value, so a newer unsaved edit is never dropped. + // 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 confirmed = new Map() - for (const [id, value] of submittedExperiments.current) { + const confirmations: { id: ExperimentId; submitted: boolean; liveValue: boolean }[] = [] + for (const [id, submitted] of submittedExperiments.current) { const liveValue = live[id] ?? experimentDefault[id] - if (liveValue !== value) { + if (liveValue !== submitted) { continue } - submittedExperiments.current.delete(id) - changedExperiments.current.delete(id) - confirmed.set(id, liveValue) + 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 } - confirmed.set(experimentId, liveValue) + adopted.set(experimentId, liveValue) } - if (confirmed.size === 0) { + 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 [id, liveValue] of confirmed) { + 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 } diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.experimentRetirement.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.experimentRetirement.spec.tsx index 7f9e4bfba0..f38efecab0 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.experimentRetirement.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.experimentRetirement.spec.tsx @@ -183,4 +183,39 @@ describe("SettingsView - experiment edits after the host confirms them", () => { 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) })