fix: resolve diff editor issues with markdown preview associations (#4946) (#4980)

* fix: resolve diff editor issues with markdown preview associations (#4946)

- Pre-open files as text documents before executing diff command to avoid preview mode
- Update closeAllDiffViews to identify diff tabs by label pattern
- Add comprehensive tests for the new behavior

This ensures that files with custom editor associations (like markdown preview)
can be properly edited using Roo Code's diff editor tools.

* refactor: extract diff view label separator as shared constant

- Add DIFF_VIEW_LABEL_SEPARATOR constant to prevent breaking logic if string changes
- Update all usages in DiffViewProvider.ts and test file
- Addresses PR review feedback for better maintainability

* refactor: improve diff view label constant to include full text

- Rename DIFF_VIEW_LABEL_SEPARATOR to DIFF_VIEW_LABEL_CHANGES
- Include full 'Original ↔ Roo's Changes' text in constant for better maintainability
- Update all usages in DiffViewProvider.ts and test file
- Addresses feedback to make constant changes less awkward

Author: daniel-lxs

src/integrations/editor/DiffViewProvider.ts

@@ -15,6 +15,7 @@ import { Task } from "../../core/task/Task"
 import { DecorationController } from "./DecorationController"
 
 export const DIFF_VIEW_URI_SCHEME = "cline-diff"
+export const DIFF_VIEW_LABEL_CHANGES = "Original ↔ Roo's Changes"
 
 // TODO: https://github.com/cline/cline/pull/3354
 export class DiffViewProvider {
@@ -384,12 +385,25 @@ export class DiffViewProvider {
 	private async closeAllDiffViews(): Promise<void> {
 		const closeOps = vscode.window.tabGroups.all
 			.flatMap((group) => group.tabs)
-			.filter(
-				(tab) =>
+			.filter((tab) => {
+				// Check for standard diff views with our URI scheme
+				if (
 					tab.input instanceof vscode.TabInputTextDiff &&
 					tab.input.original.scheme === DIFF_VIEW_URI_SCHEME &&
-					!tab.isDirty,
-			)
+					!tab.isDirty
+				) {
+					return true
+				}
+
+				// Also check by tab label for our specific diff views
+				// This catches cases where the diff view might be created differently
+				// when files are pre-opened as text documents
+				if (tab.label.includes(DIFF_VIEW_LABEL_CHANGES) && !tab.isDirty) {
+					return true
+				}
+
+				return false
+			})
 			.map((tab) =>
 				vscode.window.tabGroups.close(tab).then(
 					() => undefined,
@@ -487,17 +501,22 @@ export class DiffViewProvider {
 				}),
 			)
 
-			// Execute the diff command
-			vscode.commands
-				.executeCommand(
-					"vscode.diff",
-					vscode.Uri.parse(`${DIFF_VIEW_URI_SCHEME}:${fileName}`).with({
-						query: Buffer.from(this.originalContent ?? "").toString("base64"),
-					}),
-					uri,
-					`${fileName}: ${fileExists ? "Original ↔ Roo's Changes" : "New File"} (Editable)`,
-					{ preserveFocus: true },
-				)
+			// Pre-open the file as a text document to ensure it doesn't open in preview mode
+			// This fixes issues with files that have custom editor associations (like markdown preview)
+			vscode.window
+				.showTextDocument(uri, { preview: false, viewColumn: vscode.ViewColumn.Active })
+				.then(() => {
+					// Execute the diff command after ensuring the file is open as text
+					return vscode.commands.executeCommand(
+						"vscode.diff",
+						vscode.Uri.parse(`${DIFF_VIEW_URI_SCHEME}:${fileName}`).with({
+							query: Buffer.from(this.originalContent ?? "").toString("base64"),
+						}),
+						uri,
+						`${fileName}: ${fileExists ? `${DIFF_VIEW_LABEL_CHANGES}` : "New File"} (Editable)`,
+						{ preserveFocus: true },
+					)
+				})
 				.then(
 					() => {
 						// Command executed successfully, now wait for the editor to appear

src/integrations/editor/__tests__/DiffViewProvider.spec.ts

@@ -1,31 +1,86 @@
-import { DiffViewProvider } from "../DiffViewProvider"
+import { DiffViewProvider, DIFF_VIEW_URI_SCHEME, DIFF_VIEW_LABEL_CHANGES } from "../DiffViewProvider"
 import * as vscode from "vscode"
+import * as path from "path"
+
+// Mock fs/promises
+vi.mock("fs/promises", () => ({
+	readFile: vi.fn().mockResolvedValue("file content"),
+	writeFile: vi.fn().mockResolvedValue(undefined),
+}))
+
+// Mock utils
+vi.mock("../../../utils/fs", () => ({
+	createDirectoriesForFile: vi.fn().mockResolvedValue([]),
+}))
+
+// Mock path
+vi.mock("path", () => ({
+	resolve: vi.fn((cwd, relPath) => `${cwd}/${relPath}`),
+	basename: vi.fn((path) => path.split("/").pop()),
+}))
 
 // Mock vscode
 vi.mock("vscode", () => ({
 	workspace: {
 		applyEdit: vi.fn(),
+		onDidOpenTextDocument: vi.fn(() => ({ dispose: vi.fn() })),
+		textDocuments: [],
+		fs: {
+			stat: vi.fn(),
+		},
 	},
 	window: {
 		createTextEditorDecorationType: vi.fn(),
+		showTextDocument: vi.fn(),
+		onDidChangeVisibleTextEditors: vi.fn(() => ({ dispose: vi.fn() })),
+		tabGroups: {
+			all: [],
+			close: vi.fn(),
+		},
+		visibleTextEditors: [],
+	},
+	commands: {
+		executeCommand: vi.fn(),
+	},
+	languages: {
+		getDiagnostics: vi.fn(() => []),
 	},
 	WorkspaceEdit: vi.fn().mockImplementation(() => ({
 		replace: vi.fn(),
 		delete: vi.fn(),
 	})),
+	ViewColumn: {
+		Active: 1,
+		Beside: 2,
+		One: 1,
+		Two: 2,
+		Three: 3,
+		Four: 4,
+		Five: 5,
+		Six: 6,
+		Seven: 7,
+		Eight: 8,
+		Nine: 9,
+	},
 	Range: vi.fn(),
 	Position: vi.fn(),
 	Selection: vi.fn(),
 	TextEditorRevealType: {
 		InCenter: 2,
 	},
+	TabInputTextDiff: class TabInputTextDiff {},
+	Uri: {
+		file: vi.fn((path) => ({ fsPath: path })),
+		parse: vi.fn((uri) => ({ with: vi.fn(() => ({})) })),
+	},
 }))
 
 // Mock DecorationController
 vi.mock("../DecorationController", () => ({
 	DecorationController: vi.fn().mockImplementation(() => ({
 		setActiveLine: vi.fn(),
 		updateOverlayAfterLine: vi.fn(),
+		addLines: vi.fn(),
 		clear: vi.fn(),
 	})),
 }))
@@ -60,7 +115,11 @@ describe("DiffViewProvider", () => {
 			revealRange: vi.fn(),
 		}
 		;(diffViewProvider as any).activeLineController = { setActiveLine: vi.fn(), clear: vi.fn() }
-		;(diffViewProvider as any).fadedOverlayController = { updateOverlayAfterLine: vi.fn(), clear: vi.fn() }
+		;(diffViewProvider as any).fadedOverlayController = {
+			updateOverlayAfterLine: vi.fn(),
+			addLines: vi.fn(),
+			clear: vi.fn(),
+		}
 	})
 
 	describe("update method", () => {
@@ -93,4 +152,179 @@ describe("DiffViewProvider", () => {
 			expect(mockWorkspaceEdit.replace).toHaveBeenCalledWith(expect.anything(), expect.anything(), "New content")
 		})
 	})
+
+	describe("open method", () => {
+		it("should pre-open file as text document before executing diff command", async () => {
+			// Setup
+			const mockEditor = {
+				document: {
+					uri: { fsPath: `${mockCwd}/test.md` },
+					getText: vi.fn().mockReturnValue(""),
+					lineCount: 0,
+				},
+				selection: {
+					active: { line: 0, character: 0 },
+					anchor: { line: 0, character: 0 },
+				},
+				edit: vi.fn().mockResolvedValue(true),
+				revealRange: vi.fn(),
+			}
+
+			// Track the order of calls
+			const callOrder: string[] = []
+
+			// Mock showTextDocument to track when it's called
+			vi.mocked(vscode.window.showTextDocument).mockImplementation(async (uri, options) => {
+				callOrder.push("showTextDocument")
+				expect(options).toEqual({ preview: false, viewColumn: vscode.ViewColumn.Active })
+				return mockEditor as any
+			})
+
+			// Mock executeCommand to track when it's called
+			vi.mocked(vscode.commands.executeCommand).mockImplementation(async (command) => {
+				callOrder.push("executeCommand")
+				expect(command).toBe("vscode.diff")
+				return undefined
+			})
+
+			// Mock workspace.onDidOpenTextDocument to trigger immediately
+			vi.mocked(vscode.workspace.onDidOpenTextDocument).mockImplementation((callback) => {
+				// Trigger the callback immediately with the document
+				setTimeout(() => {
+					callback({ uri: { fsPath: `${mockCwd}/test.md` } } as any)
+				}, 0)
+				return { dispose: vi.fn() }
+			})
+
+			// Mock window.visibleTextEditors to return our editor
+			vi.mocked(vscode.window).visibleTextEditors = [mockEditor as any]
+
+			// Set up for file
+			;(diffViewProvider as any).editType = "modify"
+
+			// Execute open
+			await diffViewProvider.open("test.md")
+
+			// Verify that showTextDocument was called before executeCommand
+			expect(callOrder).toEqual(["showTextDocument", "executeCommand"])
+
+			// Verify that showTextDocument was called with preview: false
+			expect(vscode.window.showTextDocument).toHaveBeenCalledWith(
+				expect.objectContaining({ fsPath: `${mockCwd}/test.md` }),
+				{ preview: false, viewColumn: vscode.ViewColumn.Active },
+			)
+
+			// Verify that the diff command was executed
+			expect(vscode.commands.executeCommand).toHaveBeenCalledWith(
+				"vscode.diff",
+				expect.any(Object),
+				expect.any(Object),
+				`test.md: ${DIFF_VIEW_LABEL_CHANGES} (Editable)`,
+				{ preserveFocus: true },
+			)
+		})
+
+		it("should handle showTextDocument failure", async () => {
+			// Mock showTextDocument to fail
+			vi.mocked(vscode.window.showTextDocument).mockRejectedValue(new Error("Cannot open file"))
+
+			// Mock workspace.onDidOpenTextDocument
+			vi.mocked(vscode.workspace.onDidOpenTextDocument).mockReturnValue({ dispose: vi.fn() })
+
+			// Mock window.onDidChangeVisibleTextEditors
+			vi.mocked(vscode.window.onDidChangeVisibleTextEditors).mockReturnValue({ dispose: vi.fn() })
+
+			// Set up for file
+			;(diffViewProvider as any).editType = "modify"
+
+			// Try to open and expect rejection
+			await expect(diffViewProvider.open("test.md")).rejects.toThrow(
+				"Failed to execute diff command for /mock/cwd/test.md: Cannot open file",
+			)
+		})
+	})
+
+	describe("closeAllDiffViews method", () => {
+		it("should close diff views including those identified by label", async () => {
+			// Mock tab groups with various types of tabs
+			const mockTabs = [
+				// Normal diff view
+				{
+					input: {
+						constructor: { name: "TabInputTextDiff" },
+						original: { scheme: DIFF_VIEW_URI_SCHEME },
+						modified: { fsPath: "/test/file1.ts" },
+					},
+					label: `file1.ts: ${DIFF_VIEW_LABEL_CHANGES} (Editable)`,
+					isDirty: false,
+				},
+				// Diff view identified by label (for pre-opened files)
+				{
+					input: {
+						constructor: { name: "TabInputTextDiff" },
+						original: { scheme: "file" }, // Different scheme due to pre-opening
+						modified: { fsPath: "/test/file2.md" },
+					},
+					label: `file2.md: ${DIFF_VIEW_LABEL_CHANGES} (Editable)`,
+					isDirty: false,
+				},
+				// Regular file tab (should not be closed)
+				{
+					input: {
+						constructor: { name: "TabInputText" },
+						uri: { fsPath: "/test/file3.js" },
+					},
+					label: "file3.js",
+					isDirty: false,
+				},
+				// Dirty diff view (should not be closed)
+				{
+					input: {
+						constructor: { name: "TabInputTextDiff" },
+						original: { scheme: DIFF_VIEW_URI_SCHEME },
+						modified: { fsPath: "/test/file4.ts" },
+					},
+					label: `file4.ts: ${DIFF_VIEW_LABEL_CHANGES} (Editable)`,
+					isDirty: true,
+				},
+			]
+
+			// Make tabs appear as TabInputTextDiff instances
+			mockTabs.forEach((tab) => {
+				if (tab.input.constructor.name === "TabInputTextDiff") {
+					Object.setPrototypeOf(tab.input, vscode.TabInputTextDiff.prototype)
+				}
+			})
+
+			// Mock the tabGroups getter
+			Object.defineProperty(vscode.window.tabGroups, "all", {
+				get: () => [
+					{
+						tabs: mockTabs as any,
+					},
+				],
+				configurable: true,
+			})
+
+			const closedTabs: any[] = []
+			vi.mocked(vscode.window.tabGroups.close).mockImplementation((tab) => {
+				closedTabs.push(tab)
+				return Promise.resolve(true)
+			})
+
+			// Execute closeAllDiffViews
+			await (diffViewProvider as any).closeAllDiffViews()
+
+			// Verify that only the appropriate tabs were closed
+			expect(closedTabs).toHaveLength(2)
+			expect(closedTabs[0].label).toBe(`file1.ts: ${DIFF_VIEW_LABEL_CHANGES} (Editable)`)
+			expect(closedTabs[1].label).toBe(`file2.md: ${DIFF_VIEW_LABEL_CHANGES} (Editable)`)
+
+			// Verify that the regular file and dirty diff were not closed
+			expect(closedTabs.find((t) => t.label === "file3.js")).toBeUndefined()
+			expect(
+				closedTabs.find((t) => t.label === `file4.ts: ${DIFF_VIEW_LABEL_CHANGES} (Editable)` && t.isDirty),
+			).toBeUndefined()
+		})
+	})
 })