Support mentioning filenames with spaces #2363
Conversation
|
|
@ellipsis-agent review this |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 8955d45 in 1 minute and 20 seconds
More details
- Looked at
1410lines of code in8files - Skipped
0files when reviewing. - Skipped posting
12drafted comments based on config settings.
1. src/core/mentions/__tests__/index.test.ts:80
- Draft comment:
Good test for handling escaped spaces. The use of explicit path unescaping is clear. Consider adding a comment that explains why the leading slash is sliced (for clarity). - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%
None
2. src/core/mentions/index.ts:70
- Draft comment:
When using error.message in catch blocks (e.g., lines 70, 101, 125), consider type-checking or casting error to 'Error' to ensure .message exists. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. src/core/mentions/index.ts:150
- Draft comment:
The helper function 'unescapeSpaces' is clear. Consider exporting it if it might be useful in other modules. - Reason this comment was not posted:
Confidence changes required:30%<= threshold70%
None
4. webview-ui/src/utils/context-mentions.ts:303
- Draft comment:
The negative lookbehind regex for detecting unescaped spaces is neat. Verify that your target environments support this syntax, as it requires relatively recent JS engines. - Reason this comment was not posted:
Confidence changes required:70%<= threshold70%
None
5. webview-ui/src/utils/context-mentions.ts:241
- Draft comment:
When concatenating search results, consider adding comments explaining that display values are intentionally not escaped. - Reason this comment was not posted:
Confidence changes required:20%<= threshold70%
None
6. webview-ui/src/utils/path-mentions.ts:25
- Draft comment:
The conversion logic for mention paths is clear. Double-check that the normalization (backslashes to forward slashes) handles all edge cases on Windows. - Reason this comment was not posted:
Confidence changes required:30%<= threshold70%
None
7. webview-ui/src/utils/context-mentions.ts:112
- Draft comment:
In the mode handling branch (lines 111-143), ensure that the fuzzy search via Fzf remains performant with a large number of modes. Consider adding tests if not already present. - Reason this comment was not posted:
Confidence changes required:40%<= threshold70%
None
8. webview-ui/src/utils/context-mentions.ts:32
- Draft comment:
Consider extracting the logic for processing and escaping file paths (lines 32-38) into a dedicated helper function. This would improve readability and reusability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold70%
None
9. webview-ui/src/utils/context-mentions.ts:302
- Draft comment:
The negative lookbehind regex used on line 303 (/(?<!\)\s/) to detect unescaped spaces is modern and improves clarity; please ensure the target runtime supports lookbehinds. - Reason this comment was not posted:
Confidence changes required:33%<= threshold70%
None
10. webview-ui/src/utils/context-mentions.ts:273
- Draft comment:
In the deduplication logic (lines 273-279), consider handling cases where 'item.value' might be undefined. Currently, using undefined in the deduplication key (e.g., 'git-undefined') might unintentionally deduplicate distinct items. - Reason this comment was not posted:
Confidence changes required:33%<= threshold70%
None
11. webview-ui/src/utils/path-mentions.ts:12
- Draft comment:
The 'escapeSpaces' function (line 12) re-escapes spaces even if they are already escaped. This behavior (resulting in double-escaping) is noted in tests—please confirm this is the intended behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold70%
None
12. webview-ui/src/utils/path-mentions.ts:43
- Draft comment:
When the absolute path equals the current working directory, this function returns '@/'. Verify that returning an empty relative path (i.e., '@/') is the desired outcome. - Reason this comment was not posted:
Confidence changes required:33%<= threshold70%
None
Workflow ID: wflow_9PuqPTg5PjssHHD4
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // but the primary test is the output of insertMention. | ||
| jest.mock("../path-mentions", () => ({ | ||
| ...jest.requireActual("../path-mentions"), // Keep original convertToMentionPath etc. | ||
| escapeSpaces: jest.fn((path) => path.replace(/ /g, "\\ ")), // Mock implementation |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, we need to ensure that the escapeSpaces function properly escapes backslashes before escaping spaces. This can be achieved by first replacing backslashes with double backslashes and then replacing spaces with \ . This ensures that any existing backslashes in the input string are correctly escaped.
| @@ -15,3 +15,3 @@ | ||
| ...jest.requireActual("../path-mentions"), // Keep original convertToMentionPath etc. | ||
| escapeSpaces: jest.fn((path) => path.replace(/ /g, "\\ ")), // Mock implementation | ||
| escapeSpaces: jest.fn((path) => path.replace(/\\/g, "\\\\").replace(/ /g, "\\ ")), // Mock implementation | ||
| })) |
| * @returns A path with spaces escaped | ||
| */ | ||
| export function escapeSpaces(path: string): string { | ||
| return path.replace(/ /g, "\\ ") |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, we need to ensure that the escapeSpaces function also escapes backslashes in the input string. This can be done by first escaping all backslashes and then escaping spaces. We will use a regular expression with the global flag to replace all occurrences of backslashes with double backslashes and then replace all spaces with backslashes followed by a space.
| @@ -11,3 +11,4 @@ | ||
| export function escapeSpaces(path: string): string { | ||
| return path.replace(/ /g, "\\ ") | ||
| path = path.replace(/\\/g, "\\\\"); | ||
| return path.replace(/ /g, "\\ "); | ||
| } |
github-project-automation
bot
moved this from PR [Greenlit]
to Done
in Roo Code Roadmap
Important
Add support for handling file paths with spaces in mentions by escaping spaces and updating related logic and tests.
index.tsandcontext-mentions.ts.parseMentionsandopenMentionfunctions updated to handle escaped spaces in paths.context-mentions.tsupdated to recognize escaped spaces.index.test.tsandcontext-mentions.test.tsto verify handling of paths with escaped spaces.insertMention,removeMention, andgetContextMenuOptionsincontext-mentions.test.tsto ensure correct behavior with escaped spaces.escapeSpacesfunction inpath-mentions.tsto handle space escaping in paths.convertToMentionPathupdated to escape spaces in paths.This description was created by
for 8955d45. It will automatically update as commits are pushed.