Skip to content
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.

Sign up for GitHub

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

fix: Chat - App closes mention suggestions when cursor is between double @ #29239

Merged
merged 7 commits into from
Oct 17, 2023
Merged

fix: Chat - App closes mention suggestions when cursor is between double @ #29239

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fd994ae
Organize suggestions RegExps;
paultsimura Oct 10, 2023
9013b09
Merge branch 'main' into fix/28754-suggestion-between-ats
paultsimura Oct 10, 2023
b3804d3
Revert variable renaming
paultsimura Oct 11, 2023
d7ec776
Update RegExp comment
paultsimura Oct 11, 2023
64df898
Merge branch 'main' into fix/28754-suggestion-between-ats
paultsimura Oct 12, 2023
f78bc4c
Merge branch 'main' into fix/28754-suggestion-between-ats
paultsimura Oct 12, 2023
3c5a381
Merge branch 'main' into fix/28754-suggestion-between-ats
paultsimura Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions src/CONST.ts
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these are just refactors or something changed for this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this issue, I have introduced the MENTION_BREAKER RegExp – it differs a tiny bit from the SPECIAL_CHAR_OR_EMOJI in my original proposal. It allows to use symbols _ and ~ in the mention, as they might be present in usernames (especially the _).

All the rest is just refactoring.

Original file line number Diff line number Diff line change
Expand Up @@ -1265,6 +1265,8 @@ const CONST = {
CARD_EXPIRATION_DATE: /^(0[1-9]|1[0-2])([^0-9])?([0-9]{4}|([0-9]{2}))$/,
ROOM_NAME: /^#[\p{Ll}0-9-]{1,80}$/u,

// eslint-disable-next-line max-len, no-misleading-character-class
EMOJI: /[\p{Extended_Pictographic}\u200d\u{1f1e6}-\u{1f1ff}\u{1f3fb}-\u{1f3ff}\u{e0020}-\u{e007f}\u20E3\uFE0F]|[#*0-9]\uFE0F?\u20E3/gu,
// eslint-disable-next-line max-len, no-misleading-character-class
EMOJIS: /[\p{Extended_Pictographic}](\u200D[\p{Extended_Pictographic}]|[\u{1F3FB}-\u{1F3FF}]|[\u{E0020}-\u{E007F}]|\uFE0F|\u20E3)*|[\u{1F1E6}-\u{1F1FF}]{2}|[#*0-9]\uFE0F?\u20E3/gu,

Expand All @@ -1283,18 +1285,26 @@ const CONST = {
HAS_COLON_ONLY_AT_THE_BEGINNING: /^:[^:]+$/,
HAS_AT_MOST_TWO_AT_SIGNS: /^@[^@]*@?[^@]*$/,

SPECIAL_CHAR_OR_EMOJI:
// eslint-disable-next-line no-misleading-character-class
/[\n\s,/?"{}[\]()&_~^%\\;`$=#<>!*\p{Extended_Pictographic}\u200d\u{1f1e6}-\u{1f1ff}\u{1f3fb}-\u{1f3ff}\u{e0020}-\u{e007f}\u20E3\uFE0F]|[#*0-9]\uFE0F?\u20E3/gu,
SPECIAL_CHAR: /[,/?"{}[\]()&^%;`$=#<>!*]/g,

get SPECIAL_CHAR_OR_EMOJI() {
return new RegExp(`[_~\\n\\s]|${this.SPECIAL_CHAR.source}|${this.EMOJI.source}`, 'gu');
},

SPACE_OR_EMOJI:
// eslint-disable-next-line no-misleading-character-class
/(\s+|(?:[\p{Extended_Pictographic}\u200d\u{1f1e6}-\u{1f1ff}\u{1f3fb}-\u{1f3ff}\u{e0020}-\u{e007f}\u20E3\uFE0F]|[#*0-9]\uFE0F?\u20E3)+)/gu,
get SPACE_OR_EMOJI() {
return new RegExp(`(\\s+|(?:${this.EMOJI.source})+)`, 'gu');
},

// Define the regular expression pattern to find a potential end of a mention suggestion:
// It might be a space, a newline character, an emoji, or a special character (excluding underscores, which might be used in usernames)
paultsimura marked this conversation as resolved.
Show resolved Hide resolved
get MENTION_BREAKER() {
return new RegExp(`[\\n\\s]|${this.SPECIAL_CHAR.source}|${this.EMOJI.source}`, 'gu');
},

// Define the regular expression pattern to match a string starting with an at sign and ending with a space or newline character
MENTION_REPLACER:
// eslint-disable-next-line no-misleading-character-class
/^@[^\n\r]*?(?=$|[\s,/?"{}[\]()&^%\\;`$=#<>!*\p{Extended_Pictographic}\u200d\u{1f1e6}-\u{1f1ff}\u{1f3fb}-\u{1f3ff}\u{e0020}-\u{e007f}\u20E3\uFE0F]|[#*0-9]\uFE0F?\u20E3)/u,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we changed the SPECIAL_CHAR regex and removed _ & ~. Now this regex is also affected. What are the test steps for this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paultsimura

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parasharrajat I don't see how MENTION_REPLACER was affected by this. It did not contain _ & ~ before my changes and does not contain it now.

Also, we did not change the SPECIAL_CHAR regex – I just created it and reused in places where it already was. So none of the pre-existing regex-es should have been affected. Please correct me if I'm wrong or I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, my bad. This is sleep, talking. Please ignore.

get MENTION_REPLACER() {
return new RegExp(`^@[^\\n\\r]*?(?=$|\\s|${this.SPECIAL_CHAR.source}|${this.EMOJI.source})`, 'u');
},

MERGED_ACCOUNT_PREFIX: /^(MERGED_\d+@)/,

Expand Down
14 changes: 7 additions & 7 deletions src/pages/home/report/ReportActionCompose/SuggestionMention.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,17 +188,17 @@ function SuggestionMention({
}

const valueAfterTheCursor = value.substring(selectionEnd);
const indexOfFirstWhitespaceCharOrEmojiAfterTheCursor = valueAfterTheCursor.search(CONST.REGEX.NEW_LINE_OR_WHITE_SPACE_OR_EMOJI);
const indexOfFirstSpecialCharOrEmojiAfterTheCursor = valueAfterTheCursor.search(CONST.REGEX.MENTION_BREAKER);

let indexOfLastNonWhitespaceCharAfterTheCursor;
if (indexOfFirstWhitespaceCharOrEmojiAfterTheCursor === -1) {
// we didn't find a whitespace/emoji after the cursor, so we will use the entire string
indexOfLastNonWhitespaceCharAfterTheCursor = value.length;
let suggestionEndIndex;
if (indexOfFirstSpecialCharOrEmojiAfterTheCursor === -1) {
// We didn't find a special char/whitespace/emoji after the cursor, so we will use the entire string
suggestionEndIndex = value.length;
} else {
indexOfLastNonWhitespaceCharAfterTheCursor = indexOfFirstWhitespaceCharOrEmojiAfterTheCursor + selectionEnd;
suggestionEndIndex = indexOfFirstSpecialCharOrEmojiAfterTheCursor + selectionEnd;
}

const leftString = value.substring(0, indexOfLastNonWhitespaceCharAfterTheCursor);
const leftString = value.substring(0, suggestionEndIndex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caused a regression when pressing the left/right arrow keys after typing '@', the text selection does not move, #39102

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eh2077 I think you should keep digging – this is not the correct PR for the mentioned issue. This one was released half a year ago, and your issue is quite fresh (it wasn't even happening in production).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @paultsimura, thanks for your comment. I commented here to complete the checklist (the one before closing out an issue), so the linked issue has been fixed in production.

I believe that issue was introduced after this PR. Can you take a look at your recording -MacOS: Chrome / Safari?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're talking about the selection (and the search term) not changing – it's the expected behavior: #29239 (comment)

From what I see, your issue was about the cursor not moving, not the selection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's unexpected behavior as we want highlighted text to be updated when moving cursor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you get this confirmed with the internal team? At the time I implemented this PR, this was the expected behavior confirmed with the team.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #39102 (comment)

const words = leftString.split(CONST.REGEX.SPACE_OR_EMOJI);
const lastWord = _.last(words);

Expand Down
Loading