-
Notifications
You must be signed in to change notification settings - Fork 137
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
[No QA] Add rule to recognize user @here
mentions
#532
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.
Approving since the changes are the same as in #524 which I previously approved.
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.
Approving since the changes are the same as in #524 which I previously approved.
@here
mentions@here
mentions
Verified the code changes, compared to the old PR. |
Adding |
PR for updating the version is here: Expensify/App#18655 |
Thanks @mananjadhav would you be able to do the C+ checklist for the App PR. Since it wasn't needed for this one. |
|
||
// We don't support mention inside code or codefence styling, | ||
// for example using `@[email protected]` or ```@[email protected]``` will be invalid. | ||
const containsInvalidTag = /(<code>|<pre>|`)/g.test(mention); |
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.
beep boop 🤖 🐧
dropping a friendly note that this PR didn't implement mention inside codeblocks correctly, causing the bug - Expensify/App#18983
If the mention is not at the start of the string it won't be included into the regex match, causing the bug.
*/ | ||
{ | ||
name: 'userMentions', | ||
regex: new RegExp(`((`|<code>|<pre>)\\s*?)?[\`.a-zA-Z]?@+${CONST.REG_EXP.EMAIL_PART}`, 'gm'), |
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.
👋 Dropping a note about this causing a regression in Expensify/App#18857. Essentially just missed a case where mention is inside of a link. More details are here
*/ | ||
{ | ||
name: 'hereMentions', | ||
regex: /((`|<code>|<pre>)\s*?)?[`.a-zA-Z]?@here\b/gm, |
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 regex caused the bug here because a word boundary \b
does not include _
.
* @param {String} mention | ||
* @returns {bool} | ||
*/ | ||
isValidMention(mention) { |
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.
Dropping a note that this has caused Expensify/App#20085
Missed an edge case where [email protected]@gmail.com
was rendered as mention
@puneetlath @mananjadhav
Fixed Issues
$ Expensify/App#17882
Tests
@john
,@david
The user mention should be replaced as mention and the @here should be @here
QA
Same as tests