Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 44 additions & 1 deletion src/core/tools/__tests__/editFileTool.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, boolean>,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}),
}),
}
Expand Down Expand Up @@ -576,6 +578,47 @@ describe("editFileTool", () => {
})
})

describe("focus disruption default (L2: chat-diff is the default approval path)", () => {
it("saves via saveDirectly without opening the diff editor when no experiment value is stored", async () => {
mockAskApproval.mockResolvedValue(true)
// No stored experiment value: the default (flipped to true in L2) resolves
// to the chat-diff path.
mockTask.providerRef.deref.mockReturnValue({
getState: vi.fn().mockResolvedValue({
diagnosticsEnabled: true,
writeDelayMs: 1000,
experiments: {},
}),
})

await executeEditFileTool()

expect(mockTask.diffViewProvider.saveDirectly).toHaveBeenCalled()
expect(mockTask.diffViewProvider.open).not.toHaveBeenCalled()
expect(mockTask.diffViewProvider.saveChanges).not.toHaveBeenCalled()
expect(mockTask.didEditFile).toBe(true)
})

it("saves via saveDirectly when the user has explicitly enabled the experiment", async () => {
mockAskApproval.mockResolvedValue(true)
// Explicit stored true: same chat-diff routing as the default.
mockTask.providerRef.deref.mockReturnValue({
getState: vi.fn().mockResolvedValue({
diagnosticsEnabled: true,
writeDelayMs: 1000,
experiments: { preventFocusDisruption: true },
}),
})

await executeEditFileTool()

expect(mockTask.diffViewProvider.saveDirectly).toHaveBeenCalled()
expect(mockTask.diffViewProvider.open).not.toHaveBeenCalled()
expect(mockTask.diffViewProvider.saveChanges).not.toHaveBeenCalled()
expect(mockTask.didEditFile).toBe(true)
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.
})

describe("partial block handling", () => {
it("handles partial block without errors after path stabilizes", async () => {
// Path stabilization requires two consecutive calls with the same path
Expand Down
37 changes: 36 additions & 1 deletion src/core/tools/__tests__/editTool.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, boolean>,
}),
}),
}
Expand Down Expand Up @@ -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
Expand Down
45 changes: 44 additions & 1 deletion src/core/tools/__tests__/searchReplaceTool.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, boolean>,
}),
}),
}
Expand Down Expand Up @@ -330,6 +332,47 @@ describe("searchReplaceTool", () => {
})
})

describe("focus disruption default (L2: chat-diff is the default approval path)", () => {
it("saves via saveDirectly without opening the diff editor when no experiment value is stored", async () => {
mockAskApproval.mockResolvedValue(true)
// No stored experiment value: the default (flipped to true in L2) resolves
// to the chat-diff path.
mockCline.providerRef.deref.mockReturnValue({
getState: vi.fn().mockResolvedValue({
diagnosticsEnabled: true,
writeDelayMs: 1000,
experiments: {},
}),
})

await executeSearchReplaceTool()

expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalled()
expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled()
expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled()
expect(mockCline.didEditFile).toBe(true)
})

it("saves via saveDirectly when the user has explicitly enabled the experiment", async () => {
mockAskApproval.mockResolvedValue(true)
// Explicit stored true: same chat-diff routing as the default.
mockCline.providerRef.deref.mockReturnValue({
getState: vi.fn().mockResolvedValue({
diagnosticsEnabled: true,
writeDelayMs: 1000,
experiments: { preventFocusDisruption: true },
}),
})

await executeSearchReplaceTool()

expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalled()
expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled()
expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled()
expect(mockCline.didEditFile).toBe(true)
})
})

describe("partial block handling", () => {
it("handles partial block without errors after path stabilizes", async () => {
// Path stabilization requires two consecutive calls with the same path
Expand Down
45 changes: 45 additions & 0 deletions src/core/tools/__tests__/writeToFileTool.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
}),
}),
}
Expand All @@ -156,6 +159,7 @@ describe("writeToFileTool", () => {
userEdits: null,
finalContent: "final content",
}),
saveDirectly: vi.fn().mockResolvedValue(undefined),
scrollToFirstDiff: vi.fn(),
updateDiagnosticSettings: vi.fn(),
pushToolWriteResult: vi.fn().mockImplementation(async function (
Expand Down Expand Up @@ -364,6 +368,47 @@ describe("writeToFileTool", () => {
})
})

describe("focus disruption default (L2: chat-diff is the default approval path)", () => {
it("saves via saveDirectly without opening the diff editor when no experiment value is stored", async () => {
mockAskApproval.mockResolvedValue(true)
// No stored experiment value: the default (flipped to true in L2) resolves
// to the chat-diff path.
mockCline.providerRef.deref.mockReturnValue({
getState: vi.fn().mockResolvedValue({
diagnosticsEnabled: true,
writeDelayMs: 1000,
experiments: {},
}),
})

await executeWriteFileTool()

expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalled()
expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled()
expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled()
expect(mockCline.didEditFile).toBe(true)
})

it("saves via saveDirectly when the user has explicitly enabled the experiment", async () => {
mockAskApproval.mockResolvedValue(true)
// Explicit stored true: same chat-diff routing as the default.
mockCline.providerRef.deref.mockReturnValue({
getState: vi.fn().mockResolvedValue({
diagnosticsEnabled: true,
writeDelayMs: 1000,
experiments: { preventFocusDisruption: true },
}),
})

await executeWriteFileTool()

expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalled()
expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled()
expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled()
expect(mockCline.didEditFile).toBe(true)
})
})

describe("file operations", () => {
it("successfully creates new files with full workflow", async () => {
await executeWriteFileTool({}, { fileExists: false })
Expand Down
5 changes: 3 additions & 2 deletions src/integrations/editor/__tests__/DiffViewProvider.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -811,15 +811,16 @@ describe("DiffViewProvider", () => {
expect(result.finalContent).toBe("new content")
})

it("should not open file when openWithoutFocus is false", async () => {
it("should not open file when openWithoutFocus is false (focus not stolen: in-memory document only)", async () => {
await diffViewProvider.saveDirectly("test.ts", "new content", false, true, 1000)

// Verify file was written
const fs = await import("fs/promises")
expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8")

// Verify file was NOT opened
// Verify file was NOT opened in the editor, and the document is loaded in memory only
expect(vscode.window.showTextDocument).not.toHaveBeenCalled()
expect(vscode.workspace.openTextDocument).toHaveBeenCalledWith(vscode.Uri.file(`${mockCwd}/test.ts`))
})

it("should skip diagnostics when diagnosticsEnabled is false", async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand All @@ -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)
})
})
4 changes: 2 additions & 2 deletions src/shared/__tests__/experiments.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
})
})
Expand Down
5 changes: 4 additions & 1 deletion src/shared/experiments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ interface ExperimentConfig {
}

export const experimentConfigsMap: Record<ExperimentKey, ExperimentConfig> = {
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 },
Expand Down
Loading