-
Notifications
You must be signed in to change notification settings - Fork 2.6k
bug: space in folder and file name #2767
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
Conversation
|
|
Fixes #2361 |
|
@ellipsis run review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to 910e9ba in 57 seconds
More details
- Looked at
172lines of code in4files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. src/core/mentions/__tests__/index.test.ts:154
- Draft comment:
Consider restoring the fs spies (readFile and stat) after each test to avoid side effects. Adding cleanup in an afterEach could improve test isolation. - Reason this comment was not posted:
Confidence changes required:70%<= threshold70%
None
2. src/shared/context-mentions.ts:56
- Draft comment:
Ensure the updated regex handles all edge cases, especially trailing punctuation. The detailed comments are helpful, but additional unit tests for corner cases (e.g., multiple spaces and punctuation at the end) may enhance coverage. - Reason this comment was not posted:
Confidence changes required:50%<= threshold70%
None
3. webview-ui/src/utils/context-mentions.ts:271
- Draft comment:
In the shouldShowContextMenu function, verifying the first character after '@' with startsWith(' ') correctly allows spaces in file paths, but consider a comment clarifying why only the first space is checked. - Reason this comment was not posted:
Confidence changes required:30%<= threshold70%
None
4. webview-ui/src/utils/context-mentions.ts:244
- Draft comment:
When deduplicating items, ensure that undefined values for item.value are handled explicitly to avoid possible key collisions. Consider adding a fallback value for key generation. - Reason this comment was not posted:
Confidence changes required:60%<= threshold70%
None
5. webview-ui/src/utils/context-mentions.ts:247
- Draft comment:
The deduplication logic uses the raw value (item.value) as the key. This does not normalize file paths (e.g., 'src/test.ts' vs '/src/test.ts'). Consider normalizing the key (for instance, ensuring a leading slash) so that duplicates differing only by a slash are properly removed. - 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.
6. webview-ui/src/utils/context-mentions.ts:269
- Draft comment:
In shouldShowContextMenu, the condition for slash commands checks for any whitespace in the text (using text.includes(' ')). This may be too strict if slash commands are allowed to contain spaces. Please verify this logic matches the intended behavior. - 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.
7. webview-ui/src/utils/context-mentions.ts:93
- Draft comment:
The getContextMenuOptions function has a complex block for handling slash commands (modes) using fuzzy search. For better modularity and easier maintenance, consider extracting this logic into a separate helper function. - Reason this comment was not posted:
Confidence changes required:70%<= threshold70%
None
8. src/shared/context-mentions.ts:55
- Draft comment:
The updated regex and extensive inline comments greatly improve clarity. For future maintainability, consider breaking the regex into smaller, named components or helper functions. - Reason this comment was not posted:
Confidence changes required:30%<= threshold70%
None
Workflow ID: wflow_WyXpmawh4UjGoCmT
You can teach Ellipsis what to look for with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| Mention regex: | ||
| - **Purpose**: | ||
| - To identify and highlight specific mentions in text that start with '@'. | ||
| - **Purpose**: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kunwarVivek can you pleaes remove the unnecessary changes to spaces and comments to focus the PR? Minor tweak. When using AI to write fixes its usually good practice to remove any uneeded changes to lines like this.
|
@dosu Review |
|
I don't think directly matching the normal space character is a good idea. I think that we need to use escape space in path,just like what we do in terminal. And this is what I do in my pr #2565 |
|
Thank you for the PR! Just tried it out, a few thoughts:
Screen.Recording.2025-04-20.at.10.09.48.PM.mov |
|
Thank you for the PR! We just merged #3044 to fix this, so going to close this one. |
Context
Implementation
Screenshots
How to Test
Get in Touch
Important
Fix handling of file and folder paths with spaces in mentions and context menu logic.
mentionRegexincontext-mentions.tsto handle file and folder paths with spaces.shouldShowContextMenu()incontext-mentions.tsto allow paths with spaces.index.test.tsfor parsing file and folder paths with spaces.context-mentions.test.tsto verify context menu behavior for paths with spaces.context-mentions.tsto reflect regex changes.This description was created by
for 910e9ba. It will automatically update as commits are pushed.