fix: use decodeURIComponent in openFile (#5504)

* fix: use decodeURIComponent in openFile

* feat: add error handling for decodeURIComponent and tests

- Added try-catch block around decodeURIComponent to handle invalid escape sequences
- Falls back to original path if decoding fails
- Added comprehensive unit tests for the openFile function
- Tests cover invalid URI encoding, valid encoding, and various edge cases

* fix: update test to handle dynamic workspace paths in CI

* fix: handle Windows path separators in open-file tests

---------

Co-authored-by: Vivek Soni <[email protected]>
Co-authored-by: Daniel Riccio <[email protected]>

Author: 3 people

src/integrations/misc/__tests__/open-file.spec.ts

@@ -0,0 +1,240 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"
+import * as vscode from "vscode"
+import * as path from "path"
+import * as os from "os"
+import { openFile } from "../open-file"
+
+// Mock vscode module
+vi.mock("vscode", () => ({
+	Uri: {
+		file: vi.fn((path: string) => ({ fsPath: path })),
+	},
+	workspace: {
+		fs: {
+			stat: vi.fn(),
+			writeFile: vi.fn(),
+		},
+		openTextDocument: vi.fn(),
+	},
+	window: {
+		showTextDocument: vi.fn(),
+		showErrorMessage: vi.fn(),
+		tabGroups: {
+			all: [],
+		},
+		activeTextEditor: undefined,
+	},
+	commands: {
+		executeCommand: vi.fn(),
+	},
+	FileType: {
+		Directory: 2,
+		File: 1,
+	},
+	Selection: vi.fn((startLine: number, startChar: number, endLine: number, endChar: number) => ({
+		start: { line: startLine, character: startChar },
+		end: { line: endLine, character: endChar },
+	})),
+	TabInputText: vi.fn(),
+}))
+
+// Mock utils
+vi.mock("../../utils/path", () => {
+	const nodePath = require("path")
+	return {
+		arePathsEqual: vi.fn((a: string, b: string) => a === b),
+		getWorkspacePath: vi.fn(() => {
+			// In tests, we need to return a consistent workspace path
+			// The actual workspace is /Users/roocode/rc2 in local, but varies in CI
+			const cwd = process.cwd()
+			// If we're in the src directory, go up one level to get workspace root
+			if (cwd.endsWith("/src")) {
+				return nodePath.dirname(cwd)
+			}
+			return cwd
+		}),
+	}
+})
+
+// Mock i18n
+vi.mock("../../i18n", () => ({
+	t: vi.fn((key: string, params?: any) => {
+		// Return the key without namespace prefix to match actual behavior
+		if (key.startsWith("common:")) {
+			return key.replace("common:", "")
+		}
+		return key
+	}),
+}))
+
+describe("openFile", () => {
+	beforeEach(() => {
+		vi.clearAllMocks()
+		vi.spyOn(console, "warn").mockImplementation(() => {})
+	})
+
+	afterEach(() => {
+		vi.restoreAllMocks()
+	})
+
+	describe("decodeURIComponent error handling", () => {
+		it("should handle invalid URI encoding gracefully", async () => {
+			const invalidPath = "test%ZZinvalid.txt" // Invalid percent encoding
+			const mockDocument = { uri: { fsPath: invalidPath } }
+
+			vi.mocked(vscode.workspace.fs.stat).mockResolvedValue({
+				type: vscode.FileType.File,
+				ctime: 0,
+				mtime: 0,
+				size: 0,
+			})
+			vi.mocked(vscode.workspace.openTextDocument).mockResolvedValue(mockDocument as any)
+			vi.mocked(vscode.window.showTextDocument).mockResolvedValue({} as any)
+
+			await openFile(invalidPath)
+
+			// Should log a warning about decode failure
+			expect(console.warn).toHaveBeenCalledWith(
+				"Failed to decode file path: URIError: URI malformed. Using original path.",
+			)
+
+			// Should still attempt to open the file with the original path
+			expect(vscode.workspace.openTextDocument).toHaveBeenCalled()
+			expect(vscode.window.showErrorMessage).not.toHaveBeenCalled()
+		})
+
+		it("should successfully decode valid URI-encoded paths", async () => {
+			const encodedPath = "./%5Btest%5D/file.txt" // [test] encoded
+			const decodedPath = "./[test]/file.txt"
+			const mockDocument = { uri: { fsPath: decodedPath } }
+
+			vi.mocked(vscode.workspace.fs.stat).mockResolvedValue({
+				type: vscode.FileType.File,
+				ctime: 0,
+				mtime: 0,
+				size: 0,
+			})
+			vi.mocked(vscode.workspace.openTextDocument).mockResolvedValue(mockDocument as any)
+			vi.mocked(vscode.window.showTextDocument).mockResolvedValue({} as any)
+
+			await openFile(encodedPath)
+
+			// Should not log any warnings
+			expect(console.warn).not.toHaveBeenCalled()
+
+			// Should use the decoded path - verify it contains the decoded brackets
+			// On Windows, the path will include backslashes instead of forward slashes
+			const expectedPathSegment = process.platform === "win32" ? "[test]\\file.txt" : "[test]/file.txt"
+			expect(vscode.Uri.file).toHaveBeenCalledWith(expect.stringContaining(expectedPathSegment))
+			expect(vscode.workspace.openTextDocument).toHaveBeenCalled()
+			expect(vscode.window.showErrorMessage).not.toHaveBeenCalled()
+		})
+
+		it("should handle paths with special characters that need encoding", async () => {
+			const pathWithSpecialChars = "./[brackets]/file with spaces.txt"
+			const mockDocument = { uri: { fsPath: pathWithSpecialChars } }
+
+			vi.mocked(vscode.workspace.fs.stat).mockResolvedValue({
+				type: vscode.FileType.File,
+				ctime: 0,
+				mtime: 0,
+				size: 0,
+			})
+			vi.mocked(vscode.workspace.openTextDocument).mockResolvedValue(mockDocument as any)
+			vi.mocked(vscode.window.showTextDocument).mockResolvedValue({} as any)
+
+			await openFile(pathWithSpecialChars)
+
+			// Should work without errors
+			expect(console.warn).not.toHaveBeenCalled()
+			expect(vscode.workspace.openTextDocument).toHaveBeenCalled()
+			expect(vscode.window.showErrorMessage).not.toHaveBeenCalled()
+		})
+
+		it("should handle already decoded paths without double-decoding", async () => {
+			const normalPath = "./normal/file.txt"
+			const mockDocument = { uri: { fsPath: normalPath } }
+
+			vi.mocked(vscode.workspace.fs.stat).mockResolvedValue({
+				type: vscode.FileType.File,
+				ctime: 0,
+				mtime: 0,
+				size: 0,
+			})
+			vi.mocked(vscode.workspace.openTextDocument).mockResolvedValue(mockDocument as any)
+			vi.mocked(vscode.window.showTextDocument).mockResolvedValue({} as any)
+
+			await openFile(normalPath)
+
+			// Should work without errors
+			expect(console.warn).not.toHaveBeenCalled()
+			expect(vscode.workspace.openTextDocument).toHaveBeenCalled()
+			expect(vscode.window.showErrorMessage).not.toHaveBeenCalled()
+		})
+	})
+
+	describe("error handling", () => {
+		it("should show error message when file does not exist", async () => {
+			const nonExistentPath = "./does/not/exist.txt"
+
+			vi.mocked(vscode.workspace.fs.stat).mockRejectedValue(new Error("File not found"))
+
+			await openFile(nonExistentPath)
+
+			expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("errors.could_not_open_file")
+		})
+
+		it("should handle generic errors", async () => {
+			const testPath = "./test.txt"
+
+			vi.mocked(vscode.workspace.fs.stat).mockRejectedValue("Not an Error object")
+
+			await openFile(testPath)
+
+			expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("errors.could_not_open_file")
+		})
+	})
+
+	describe("directory handling", () => {
+		it("should reveal directories in explorer", async () => {
+			const dirPath = "./components"
+
+			vi.mocked(vscode.workspace.fs.stat).mockResolvedValue({
+				type: vscode.FileType.Directory,
+				ctime: 0,
+				mtime: 0,
+				size: 0,
+			})
+
+			await openFile(dirPath)
+
+			expect(vscode.commands.executeCommand).toHaveBeenCalledWith(
+				"revealInExplorer",
+				expect.objectContaining({ fsPath: expect.stringContaining("components") }),
+			)
+			expect(vscode.commands.executeCommand).toHaveBeenCalledWith("list.expand")
+			expect(vscode.workspace.openTextDocument).not.toHaveBeenCalled()
+		})
+	})
+
+	describe("file creation", () => {
+		it("should create new files when create option is true", async () => {
+			const newFilePath = "./new/file.txt"
+			const content = "Hello, world!"
+
+			vi.mocked(vscode.workspace.fs.stat).mockRejectedValue(new Error("File not found"))
+			vi.mocked(vscode.workspace.openTextDocument).mockResolvedValue({} as any)
+			vi.mocked(vscode.window.showTextDocument).mockResolvedValue({} as any)
+
+			await openFile(newFilePath, { create: true, content })
+
+			// On Windows, the path will include backslashes instead of forward slashes
+			const expectedPathSegment = process.platform === "win32" ? "new\\file.txt" : "new/file.txt"
+			expect(vscode.workspace.fs.writeFile).toHaveBeenCalledWith(
+				expect.objectContaining({ fsPath: expect.stringContaining(expectedPathSegment) }),
+				Buffer.from(content, "utf8"),
+			)
+			expect(vscode.workspace.openTextDocument).toHaveBeenCalled()
+		})
+	})
+})

src/integrations/misc/open-file.ts

@@ -12,9 +12,19 @@ interface OpenFileOptions {
 
 export async function openFile(filePath: string, options: OpenFileOptions = {}) {
 	try {
+		// Store the original path for error messages before any modifications
+		const originalFilePathForError = filePath
+
+		// Try to decode the URI component, but if it fails, use the original path
+		try {
+			filePath = decodeURIComponent(filePath)
+		} catch (decodeError) {
+			// If decoding fails (e.g., invalid escape sequences), continue with the original path
+			console.warn(`Failed to decode file path: ${decodeError}. Using original path.`)
+		}
+
 		const workspaceRoot = getWorkspacePath()
 		const homeDir = os.homedir()
-		const originalFilePathForError = filePath // Keep original for error messages
 
 		const attemptPaths: string[] = []