-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: auto-insert space before @mentions when needed #8064
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
base: main
Are you sure you want to change the base?
Conversation
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards and I still missed the obvious bugs.
| let newValue: string | ||
| let mentionIndex: number | ||
|
|
||
| if (lastAtIndex !== -1) { |
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.
Critical issue: This logic replaces everything after the FIRST @ found in the text, which causes data loss when multiple mentions exist.
For example, if the text is "Check @/file1.txt and @" and we're inserting a new mention at the second @, this will replace from the first @ instead.
Consider finding the @ that's actually at or near the cursor position instead of using lastIndexOf on the entire beforeCursor text. You might want to look for the @ closest to the cursor position that doesn't already have a complete mention after it.
| // The function finds the @ at position 6 and replaces from there | ||
| // This is not the desired behavior for this case | ||
| // We should insert a new mention after "and" | ||
| expect(result.newValue).toBe("Check @/file2.txt ") |
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.
This test actually demonstrates the bug - it expects the first mention to be replaced when adding a second mention, which is incorrect behavior. The expected result should be "Check @/file1.txt and @/file2.txt " with the new mention inserted after "and", not replacing the first mention.
Is this the intended behavior? It seems like it would cause users to lose their first mention when trying to add a second one.
| let totalLength = 0 | ||
|
|
||
| // Check if we need to add a space before the first mention | ||
| const textBefore = inputValue.slice(0, cursorPosition) |
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.
This space-checking logic is duplicated in the insertMention function. Since insertMention already handles adding space before @mentions, do we need this logic here as well?
Consider whether both places need this or if it should be centralized in one location to avoid potential inconsistencies.
| // This is a side effect of the regex pattern, but not the intended use case | ||
| // The removeMention function is meant to be called when backspace is pressed | ||
| // right after a complete mention | ||
| expect(result.newText).toBe("Check th/to/file.txt here") |
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.
This test shows that calling removeMention from the middle of a mention produces unexpected results (removes partial text). While documented as "not the intended use case", could this lead to confusing behavior if triggered accidentally?
Might be worth adding a guard to check if we're actually at the end of a mention before attempting removal.
| newValue = beforeCursor + "@" + processedValue + " " + afterCursor | ||
| mentionIndex = position | ||
| // Check if we need to add a space before the mention | ||
| const needsSpaceBefore = beforeCursor.length > 0 && !beforeCursor.endsWith(" ") && !beforeCursor.endsWith("\n") |
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.
Minor suggestion: The space-checking logic (checking for trailing space or newline) is repeated here and at line 63. Consider extracting this into a small utility function like needsSpaceBeforeMention(text: string): boolean to reduce duplication and make the intent clearer.
|
It has been tested and works as expected |
daniel-lxs
left a comment
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.
LGTM, thank you @NaccOll for opening the issue!
Status: Posted 2 inline comments focusing on safer active-mention replacement and consistent space insertion before @mentions. Awaiting author feedback.
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
| let newValue: string | ||
| let mentionIndex: number | ||
|
|
||
| if (lastAtIndex !== -1) { |
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.
Active mention detection: this replaces starting at the last "@" before the cursor even if the caret is no longer inside that mention token. If the user already typed a complete mention earlier and the cursor is elsewhere, selecting a new item can overwrite the earlier mention. Replace only when there is no whitespace between the last "@" and the cursor; otherwise insert a new mention.
| if (lastAtIndex !== -1) { | |
| const isActiveMention = lastAtIndex !== -1 && !/\s/.test(beforeCursor.slice(lastAtIndex + 1)) | |
| if (isActiveMention) { |
Fix it with Roo Code or mention @roomote and request a fix.
Summary
This PR fixes issue #8063 where @mentions would not parse correctly when typed directly after text without a space. The issue was introduced in PR #7876 which restricted @mention parsing to only work at line-start or after whitespace to prevent accidental parsing in pasted logs.
Problem
When users:
The @mentions would not be parsed correctly because the regex requires whitespace before the @ symbol.
Solution
While keeping the regex restriction from PR #7876 (which correctly prevents parsing in pasted logs), this PR modifies the UI behavior to automatically insert a space before @ when needed:
insertTextIntoTextareahandler already had this logicChanges
ChatTextArea.tsxto check and add space before @ when dragging/dropping filesinsertMentionfunction incontext-mentions.tsto auto-insert space when neededTesting
Screenshots
Before: Users had to manually add space before @ for mentions to work
After: Space is automatically inserted when needed, making the UX seamless
Fixes #8063
Important
Fixes issue #8063 by auto-inserting space before
@mentionswhen needed, ensuring correct parsing without altering regex restrictions.@mentionswhen needed inChatTextArea.tsxandcontext-mentions.ts.insertMentionincontext-mentions.tsto handle space insertion logic.ChatTextArea.tsxto check and add space before@during drag/drop and mention insertion.context-mentions.spec.tsfor auto-space insertion behavior.This description was created by
for 7ca6046. You can customize this summary. It will automatically update as commits are pushed.