-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix: Incorrect escaping of spaces in file paths #2565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Yikai-Liao
wants to merge
13
commits into
RooCodeInc:main
from
Yikai-Liao:fix/path-mentions-escaping
Closed
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
694f6dc
fix: correct path escaping for special characters in file paths
Yikai-Liao 10ad203
Add changeset for path escaping fix
Yikai-Liao de935f5
Fix path normalization in mentions to correctly handle Windows paths
Yikai-Liao b011d5e
Fix path handling in mentions with Windows paths and escaped spaces
Yikai-Liao 8902e94
Update .gitignore and refactor mentions handling
Yikai-Liao 5227b1b
Refactor test mocks for mentions handling
Yikai-Liao 244db19
Enhance path handling and test coverage for mentions
Yikai-Liao 718fb82
Add mock for react-i18next and enhance mentions parsing
Yikai-Liao 042c2d0
fix: Resolve path space escaping and add missing path-browserify depe…
Yikai-Liao bab59a1
Fix backslash escaping for CodeQL warnings and update .gitignore
Yikai-Liao f38869e
Implement helper function for path escaping in mentions
Yikai-Liao 6143ccd
Suppressed 'js/incomplete-string-escaping' warnings in:
Yikai-Liao d4e9d20
- fix shouldShowContextMenu when there are -multiple @
Yikai-Liao File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,81 @@ describe("insertMention", () => { | |
| expect(result.newValue).toBe("@test ") | ||
| expect(result.mentionIndex).toBe(0) | ||
| }) | ||
|
|
||
| it("should handle mentions with spaces correctly", () => { | ||
| const result = insertMention("Hello world", 5, "file with spaces.txt") | ||
| expect(result.newValue).toBe("Hello@file\\ with\\ spaces.txt world") | ||
| expect(result.mentionIndex).toBe(5) | ||
| }) | ||
|
|
||
| it("should replace text after last @ symbol with spaces in mention", () => { | ||
| const result = insertMention("Hello @wor world", 8, "file with spaces.txt") | ||
| expect(result.newValue).toBe("Hello @file\\ with\\ spaces.txt world") | ||
| expect(result.mentionIndex).toBe(6) | ||
| }) | ||
|
|
||
| it("should handle file names with multiple consecutive spaces", () => { | ||
| const result = insertMention("Hello world", 5, "file with multiple spaces.txt") | ||
| expect(result.newValue).toBe("Hello@file\\ \\ with\\ \\ \\ multiple\\ \\ \\ \\ spaces.txt world") | ||
| expect(result.mentionIndex).toBe(5) | ||
| }) | ||
|
|
||
| it("should handle file names with special characters", () => { | ||
| const result = insertMention("Hello world", 5, "file-with_special#chars&.txt") | ||
| expect(result.newValue).toBe("Hello@file-with_special#chars&.txt world") | ||
| expect(result.mentionIndex).toBe(5) | ||
| }) | ||
|
|
||
| it("should handle file names with unicode characters", () => { | ||
| const result = insertMention("Hello world", 5, "文件名with空格and字符.txt") | ||
| expect(result.newValue).toBe("Hello@文件名with空格and字符.txt world") | ||
| expect(result.mentionIndex).toBe(5) | ||
| }) | ||
|
|
||
| it("should handle paths with spaces", () => { | ||
| const result = insertMention("Hello world", 5, "path with/spaces/in it/file.txt") | ||
| expect(result.newValue).toBe("Hello@path\\ with/spaces/in\\ it/file.txt world") | ||
| expect(result.mentionIndex).toBe(5) | ||
| }) | ||
|
|
||
| it("should handle cursor at beginning of text", () => { | ||
| const result = insertMention("Hello world", 0, "test.txt") | ||
| expect(result.newValue).toBe("@test.txt Hello world") | ||
| expect(result.mentionIndex).toBe(0) | ||
| }) | ||
|
|
||
| it("should handle cursor at end of text", () => { | ||
| const result = insertMention("Hello world", 11, "test.txt") | ||
| expect(result.newValue).toBe("Hello [email protected] ") | ||
| expect(result.mentionIndex).toBe(11) | ||
| }) | ||
|
|
||
| it("should handle text with multiple @ symbols", () => { | ||
| const result = insertMention("Hello @first and @second world", 19, "test.txt") | ||
| expect(result.newValue).toBe("Hello @first and @test.txt world") | ||
| expect(result.mentionIndex).toBe(17) | ||
| }) | ||
|
|
||
| it("should handle file names already containing backslashes", () => { | ||
| const result = insertMention("Hello world", 5, "file\\with\\backslashes.txt") | ||
| // Backslashes should be preserved and spaces should be escaped | ||
| expect(result.newValue).toBe("Hello@file\\with\\backslashes.txt world") | ||
| expect(result.mentionIndex).toBe(5) | ||
| }) | ||
|
|
||
| it("should handle very long file names", () => { | ||
| const longName = "a".repeat(100) + " " + "b".repeat(100) + ".txt" | ||
| const result = insertMention("Hello world", 5, longName) | ||
| expect(result.newValue).toBe(`Hello@${"a".repeat(100)}\\ ${"b".repeat(100)}.txt world`) | ||
| expect(result.mentionIndex).toBe(5) | ||
| }) | ||
|
|
||
| it("should handle file names already containing escaped spaces", () => { | ||
| const result = insertMention("Hello world", 5, "file\\ with\\ spaces.txt") | ||
| // It should correctly handle the already escaped spaces without double-escaping | ||
| expect(result.newValue).toBe("Hello@file\\\\ with\\\\ spaces.txt world") | ||
| expect(result.mentionIndex).toBe(5) | ||
| }) | ||
| }) | ||
|
|
||
| describe("removeMention", () => { | ||
|
|
@@ -338,3 +413,114 @@ describe("shouldShowContextMenu", () => { | |
| expect(shouldShowContextMenu("@problems", 9)).toBe(true) | ||
| }) | ||
| }) | ||
|
|
||
| /** | ||
| * Integration tests for the complete file mention workflow | ||
| * These tests simulate the actual user flow when selecting files from the list | ||
| * | ||
| * IMPORTANT NOTE ABOUT BACKSLASH ESCAPING IN TESTS: | ||
| * ================================================ | ||
| * The escaping of spaces in file paths is a multi-level process: | ||
| * | ||
| * 1. In JavaScript strings, a single backslash must be written as "\\". | ||
| * So to represent "\" in the string, we write it as "\\". | ||
| * | ||
| * 2. When we want to escape a space in a path, we use "\ " (backslash + space). | ||
| * But in JavaScript code, this must be written as "\\ ". | ||
| * | ||
| * 3. Our escaping pipeline has two phases: | ||
| * - First in convertToMentionPath: spaces -> "\ " | ||
| * - Then in insertMention: "\ " -> "\\ " | ||
| * | ||
| * 4. Due to this multi-level escaping, paths in our expect() statements | ||
| * contain four backslashes "\\\\" for each escaped space. | ||
| * | ||
| * This is NOT a bug but the expected behavior: | ||
| * - In JS, "\\\\" represents two literal backslashes "\\" | ||
| * - When the string is processed, these become the actual double escape | ||
| * that's displayed to the user in their editor | ||
| * | ||
| * Example: | ||
| * - Original path: "/docs/file with spaces.txt" | ||
| * - After convertToMentionPath: "@/docs/file\ with\ spaces.txt" | ||
| * - After insertMention: "@/docs/file\\ with\\ spaces.txt" | ||
| * - In test code expect(): "...@/docs/file\\\\ with\\\\ spaces.txt..." | ||
| */ | ||
| describe("File selection integration tests", () => { | ||
| // Import the convertToMentionPath function to use in the integration tests | ||
| const { convertToMentionPath } = require("../path-mentions") | ||
|
|
||
| it("should correctly handle selection of file with spaces from file list", () => { | ||
| // Step 1: User has text in editor with cursor position | ||
| const originalText = "Check out this file: " | ||
| const cursorPosition = originalText.length | ||
|
|
||
| // Step 2: User selects a file with spaces from the list | ||
| // The file path would be converted to a mention path | ||
| const selectedFilePath = "/Users/username/project/documents/report with spaces.pdf" | ||
| const projectRoot = "/Users/username/project" | ||
| const mentionPath = convertToMentionPath(selectedFilePath, projectRoot) | ||
|
|
||
| // Verify mention path has single-escaped spaces (written with double backslashes in code) | ||
| // Actual: "@/documents/report\ with\ spaces.pdf" | ||
| expect(mentionPath).toBe("@/documents/report\\ with\\ spaces.pdf") | ||
|
|
||
| // Step 3: The mention path is inserted at cursor position | ||
| // We remove the @ since insertMention adds it back | ||
| const result = insertMention(originalText, cursorPosition, mentionPath.substring(1)) | ||
|
|
||
| // Step 4: Verify the final text has proper escaping and format | ||
| // Note: Double backslashes in the actual output require four backslashes in the code | ||
| // Actual output: "Check out this file: @/documents/report\\ with\\ spaces.pdf " | ||
| expect(result.newValue).toBe("Check out this file: @/documents/report\\\\ with\\\\ spaces.pdf ") | ||
| }) | ||
|
|
||
| it("should correctly handle selection of file with special characters from file list", () => { | ||
| // Step 1: User has text in editor with cursor position | ||
| const originalText = "Document reference: " | ||
| const cursorPosition = originalText.length | ||
|
|
||
| // Step 2: User selects a file with special characters | ||
| const selectedFilePath = "/Users/username/project/docs/report#1 (2023) final-v2.pdf" | ||
| const projectRoot = "/Users/username/project" | ||
| const mentionPath = convertToMentionPath(selectedFilePath, projectRoot) | ||
|
|
||
| // Verify mention path has single-escaped spaces (written with double backslashes in code) | ||
| // Actual: "@/docs/report#1\ (2023)\ final-v2.pdf" | ||
| expect(mentionPath).toBe("@/docs/report#1\\ (2023)\\ final-v2.pdf") | ||
|
|
||
| // Step 3: The mention path is inserted at cursor position | ||
| // We remove the @ since insertMention adds it back | ||
| const result = insertMention(originalText, cursorPosition, mentionPath.substring(1)) | ||
|
|
||
| // Step 4: Verify the final text has proper escaping and format | ||
| // Note: Double backslashes in the actual output require four backslashes in the code | ||
| // Actual output: "Document reference: @/docs/report#1\\ (2023)\\ final-v2.pdf " | ||
| expect(result.newValue).toBe("Document reference: @/docs/report#1\\\\ (2023)\\\\ final-v2.pdf ") | ||
| }) | ||
|
|
||
| it("should correctly handle selection of nested file with spaces from file list", () => { | ||
| // Step 1: User has text with existing mentions | ||
| const originalText = "Compare with @/other/file.txt and " | ||
| const cursorPosition = originalText.length | ||
|
|
||
| // Step 2: User selects a nested file with spaces | ||
| const selectedFilePath = "/Users/username/project/nested/folder with spaces/file with spaces.txt" | ||
| const projectRoot = "/Users/username/project" | ||
| const mentionPath = convertToMentionPath(selectedFilePath, projectRoot) | ||
|
|
||
| // Verify mention path has single-escaped spaces (written with double backslashes in code) | ||
| // Actual: "@/nested/folder\ with\ spaces/file\ with\ spaces.txt" | ||
| expect(mentionPath).toBe("@/nested/folder\\ with\\ spaces/file\\ with\\ spaces.txt") | ||
|
|
||
| // Step 3: The mention path is inserted at cursor position | ||
| // We remove the @ since insertMention adds it back | ||
| const result = insertMention(originalText, cursorPosition, mentionPath.substring(1)) | ||
|
|
||
| // Step 4: Verify the final text has proper escaping and format | ||
| // Note: Double backslashes in the actual output require four backslashes in the code | ||
| // Important: The original behavior replaces the existing text after the @ symbol | ||
| // Actual output: "Compare with @/nested/folder\\ with\\ spaces/file\\ with\\ spaces.txt " | ||
| expect(result.newValue).toBe("Compare with @/nested/folder\\\\ with\\\\ spaces/file\\\\ with\\\\ spaces.txt ") | ||
| }) | ||
| }) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High test