Skip to content

Conversation

@Yikai-Liao
Copy link
Contributor

@Yikai-Liao Yikai-Liao commented Apr 13, 2025

Description

This PR addresses the issues reported in Issue #2564 by implementing the following changes:

  • Fix escaping issues when file paths contain spaces and special characters
  • Improve handling of multi-level escaping in path mentions
  • Add comprehensive tests for path and context mentions with special characters
  • Add detailed documentation about backslash escaping logic

Context

This change fixes a bug where file paths containing spaces or special characters were not properly escaped, leading to failures in mention highlighting and file access. The problem affects utility functions like convertToMentionPath and insertMention, causing inconsistent behavior across different operating systems. Resolving this ensures better reliability for users dealing with common file naming conventions, improving overall user experience in path referencing.

Implementation

The implementation involved updating the escaping logic in path-mentions.ts and context-mentions.ts to handle spaces and special characters more accurately. Specifically:

  • Modified convertToMentionPath to correctly escape spaces using single backslashes and prevent double-escaping by checking for existing escapes.
  • Enhanced insertMention to maintain consistent path formatting during insertion, ensuring that multi-level escapes are processed sequentially without adding extra backslashes.
  • Added new test cases in context-mentions.test.ts and path-mentions.test.ts to cover edge cases like paths with multiple spaces, special characters, and pre-escaped strings.
  • Included JSDoc comments and inline documentation in the affected files to explain the new escaping logic, making it easier for future developers to understand and maintain.

These changes were made with minimal refactoring to avoid breaking existing functionality, focusing on targeted fixes for the reported issues.

How to Test

To verify the fixes, follow these steps:

  1. Clone the repository and check out the fix/path-mentions-escaping branch.
  2. Create a test file with spaces in its path (e.g., test folder with spaces/test file with spaces.txt).
  3. In the editor, type @ to trigger the mention functionality and select the test file.
  4. Verify that the path is correctly escaped (e.g., @/test\ folder\ with\ spaces/test\ file\ with\ spaces.txt).
  5. Attempt to access the file via the mention link and confirm it opens without errors.
  6. Run the tests using npx jest webview-ui/src/utils/__tests__/context-mentions.test.ts and npx jest webview-ui/src/utils/__tests__/path-mentions.test.ts to ensure all new test cases pass.
  7. Test with files containing special characters (e.g., #, @) and pre-escaped paths to confirm no double-escaping occurs.

If any issues persist, check the console for errors and ensure your environment matches the one in the issue (e.g., Windows or Unix-based OS).


Important

Fixes incorrect escaping of spaces and special characters in file paths, with updated tests and documentation.

  • Behavior:
    • Fixes incorrect escaping of spaces and special characters in file paths in insertMention and convertToMentionPath.
    • Ensures paths are correctly formatted across different operating systems.
  • Tests:
    • Adds tests in context-mentions.test.ts and path-mentions.test.ts for paths with spaces, special characters, and pre-escaped strings.
    • Includes integration tests for file mention workflows.
  • Documentation:
    • Updates inline comments and JSDoc in context-mentions.ts and path-mentions.ts to explain multi-level escaping logic.

This description was created by Ellipsis for 694f6dc. It will automatically update as commits are pushed.

- Fix escaping issues when file paths contain spaces and special characters

- Improve handling of multi-level escaping in path mentions

- Add comprehensive tests for path and context mentions with special characters

- Add detailed documentation about backslash escaping logic
@changeset-bot
Copy link

changeset-bot bot commented Apr 13, 2025

🦋 Changeset detected

Latest commit: d4e9d20

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
roo-cline Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 13, 2025
const fullPath = `C:\\Users\\user\\project\\${longDirectoryName}\\${longFileName}`

expect(convertToMentionPath(fullPath, "C:\\Users\\user\\project")).toBe(
`@/${longDirectoryName}/${longFileName.replace(/ /g, "\\ ")}`,

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding High test

This does not escape backslash characters in the input.
@dosubot dosubot bot added bug Something isn't working documentation Improvements or additions to documentation labels Apr 13, 2025
@mrubens
Copy link
Collaborator

mrubens commented Apr 13, 2025

Hi, thank you for working on this! Unfortunately I'm not sure it's working for me though:

Screenshot 2025-04-13 at 10 15 51 AM

I also took a brief attempt at this at #2363 as well in case that's helpful, but I got distracted and didn't have time to finish. Let me know if I can help at all or if I'm doing something wrong while testing this!

@Yikai-Liao
Copy link
Contributor Author

Yikai-Liao commented Apr 13, 2025

@mrubens After writing the fix, I only add several unit test based on my understanding of this project. But I don't know how to actually test it as a plugin in vscode. Maybe you could help me with that.

@mrubens
Copy link
Collaborator

mrubens commented Apr 13, 2025

This commit addresses an issue with path handling in the mentions feature, specifically for Windows platforms. The primary fixes include:

1. Modified the mention regex in context-mentions.ts to use greedy matching for paths, ensuring complete path capture including multiple spaces.
2. Updated path handling in mentions/index.ts to properly process relative paths and remove leading slashes before resolving.
3. Fixed XML tag display to maintain consistent path format in output.
4. Added detailed comments explaining the regex pattern and path handling logic.
5. Enhanced test suite with additional cases for complex paths and cross-platform path handling.

The issue was causing paths with spaces to be truncated at the first space when using mentions on Windows, resulting in failed file lookups. These changes ensure consistent path handling across platforms while maintaining the original path format in display output.
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 13, 2025
- Added compiled files and directories to .gitignore to prevent unnecessary tracking.
- Removed global type declarations for mockOpenFile from mentions/index.ts to streamline the code.
- Enhanced error handling in openMention function to ensure proper error messages are displayed when workspace paths are not found.
- Updated tests to improve coverage for various file path scenarios, including international characters and escaped spaces.
- Ensured consistent path normalization across different platforms in mentions handling.
- Updated mock implementations for file system operations to use async/await for better readability and consistency.
- Enhanced mock functions to return promises, ensuring proper handling of asynchronous operations in tests.
- Improved path normalization checks in tests to ensure compatibility with various path formats, including escaped spaces and international characters.
- Adjusted test cases to verify correct behavior for file and directory mentions, including edge cases with special characters and long paths.
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Apr 13, 2025
- Improved the extraction logic in `extractFilePath` to handle special cases for escaped spaces and Windows-style paths.
- Updated test cases to cover various scenarios, including paths with consecutive escaped spaces, international characters, and adjacent mentions.
- Refactored mock implementations in tests for better clarity and consistency, ensuring proper handling of asynchronous operations.
- Enhanced error handling for invalid paths and permission issues, providing clearer feedback in test results.
@Yikai-Liao
Copy link
Contributor Author

@mrubens Thanks, I can install it noe. After several commits, I find that roo code now could generate and parse all kinds of path. But the highlight in UI is incorrect(only highlight @/). I try to add a test to find out why but failed. Could you help me with that?

- Introduced a mock implementation for `react-i18next` to simplify testing.
- Enhanced the `parseMentionsFromText` and `extractFilePath` functions to improve handling of paths with escaped spaces and special characters.
- Added comprehensive unit tests for mentions parsing, covering various scenarios including escaped spaces, international characters, and multiple mentions.
- Updated the `highlightMentions` function to utilize the new parsing logic, ensuring accurate highlighting of mentions in the chat interface.
- Improved debugging information in the context menu logic to assist in identifying valid mentions.
@Yikai-Liao
Copy link
Contributor Author

Oh, nice, I have fixed the highlight locally 🎉

…ndency

- Refined space escaping logic in `context-mentions.ts` and `path-mentions.ts` to handle potential edge cases and improve robustness, addressing concerns about escaped characters.
- Added `path-browserify` and `@types/path-browserify` to `webview-ui/package.json` to resolve missing dependency and type errors.
@Yikai-Liao
Copy link
Contributor Author

Yikai-Liao commented Apr 14, 2025

@mrubens Sorry, but it seems that I can't deal with the CodeQL Check by my self. I have no idea how to move forward.

Comment on lines +67 to +72
const formattedValue = value
.replace(/\\\\\\\\/g, "\\DOUBLE_BACKSLASH") // First preserve actual double backslashes
.replace(/\\\\ /g, "\\ESCAPED_SPACE") // Temporarily replace already escaped spaces
// ADDED: Escape standalone backslashes not part of the above patterns
.replace(/(?<!\\\\)\\\\(?! |\\\\)/g, '\\\\\\\\') // Makes \ -> \\
.replace(/ /g, "\\ ") // Escape normal spaces (makes ' ' -> '\\ ')

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding High

This does not escape backslash characters in the input.
- Added `escapePath` function to encapsulate multi-level escaping for backslashes and spaces in file paths.
- Updated `convertToMentionPath` to utilize the new helper function, improving code clarity and consistency in path handling.
- Ensured robust handling of paths with escaped characters across different scenarios.
- `context-mentions.ts`: The multi-step replace logic correctly handles backslash escaping, but may confuse static analysis.
- `path-mentions.test.ts`: Warning triggered on a string literal within a test assertion.
@Yikai-Liao
Copy link
Contributor Author

Yikai-Liao commented Apr 14, 2025

I further fixed "show context window". I found that if there are multiple @, this won't work in new @

But I still can't deal with the CodeQL warning @mrubens

@hannesrudolph hannesrudolph moved this from New to PR [Pre Approval Review] in Roo Code Roadmap Apr 14, 2025
@Yikai-Liao
Copy link
Contributor Author

Could anyone give me some suggestions on this PR?

@Yikai-Liao
Copy link
Contributor Author

@mrubens Could you review my PR and give me some advice?

@mrubens
Copy link
Collaborator

mrubens commented Apr 30, 2025

Thank you for the PR! We just merged #3044 to fix this, so going to close this one.

@mrubens mrubens closed this Apr 30, 2025
@github-project-automation github-project-automation bot moved this from PR [Pre Approval Review] to Done in Roo Code Roadmap Apr 30, 2025
SmartManoj pushed a commit to SmartManoj/Raa-Code that referenced this pull request May 6, 2025
@Yikai-Liao
Copy link
Contributor Author

Sorry for the late reply. But I found that something fixed in my pr is still there now. For example, if I have multiple "@", roo code will give some totally wrong highlights for these "@". But I don't have time to fix it in the current branch. So any plan about this now? @mrubens

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect escaping of spaces in file paths causes mention highlighting and file access failures

2 participants