[Awaiting checklist completion] [$500] Chat - User suggestion lost when hit space after entering user's first name

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.3.89-6

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

Action Performed:

1. Navigate to staging.new.expensify.com
2. Go to #announce room with several members
3. Enter "@" in the compose box
4. Start typing Fist and Last name of the user using space

Expected Result:

Suggestion list should remain when user use space to divide First and Last name

Actual Result:

User suggestion lost when hit space after entering user's first name into chat composers

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• Windows: Chrome
• MacOS: Desktop

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

Windows: Chrome

Bug6248030_1698095877043.Recording__1293.mp4

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d7e3cf200819dc8c
• Upwork Job ID: 1716576553756684288
• Last Price Increase: 2023-10-23
• Automatic offers:
• graylewis | Contributor | 27484138

Issue Owner

Current Issue Owner: @allroundexperts

[melvin-bot]

Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01d7e3cf200819dc8c

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts (External)

[graylewis]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When a user's display name contains a space, the suggestions cause by mentioning a user with an @ sign stop working after the space

What is the root cause of that problem?

Parsing the @ is done by splitting words by the ONST.REGEX.SPACE_OR_EMOJI regex. The last item in the array of words generated from the .split() call is the only part of your text considered when generating suggestions.

What changes do you think we should make in order to solve the problem?

In the case of parsing mentions (i.e. the original logic is OK for the emoji parsing but not mention parsing), instead of only checking the last word, we should check the last x words, where x is a reasonable amount of spaces in a name we should support searching.

Like so (edit of lines 201 on from src/pages/home/report/ReportActionCompose/SuggestionMention.js):

            const leftString = value.substring(0, suggestionEndIndex);
            const words = leftString.split(CONST.REGEX.SPACE_OR_EMOJI);
            const lastWord = _.last(words);
            const secondToLastWord = words[words.length - 3];

            let atSignIndex;
            let atSignWord;
        
            if (lastWord.startsWith('@')) {
                atSignIndex = leftString.lastIndexOf(lastWord);
                atSignWord = lastWord;
            } else if (secondToLastWord && secondToLastWord.startsWith('@')) {
                atSignIndex = leftString.lastIndexOf(secondToLastWord);
                atSignWord = secondToLastWord;
            }

            const prefix = atSignWord.substring(1);

            const nextState = {
                suggestedMentions: [],
                atSignIndex,
                mentionPrefix: prefix,
            };

            const isCursorBeforeTheMention = valueAfterTheCursor.startsWith(atSignWord);

            if (!isCursorBeforeTheMention && isMentionCode(atSignWord)) {
                const suggestions = getMentionOptions(personalDetails, prefix);
                nextState.suggestedMentions = suggestions;
                nextState.shouldShowSuggestionMenu = !_.isEmpty(suggestions);
            }

Alternative Solutions:

Change composer parsing altogether such that it uses more complex signals to determine what is the currently active segment of text (i.e. timing or something else) rather than by relying on spaces.

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Chat - User suggestion lost when hit space after entering user's first name

What is the root cause of that problem?

We're extracting the lastWord (split by space) to figure out it's the mention or not

App/src/pages/home/report/ReportActionCompose/SuggestionMention.js

Line 206 in cc3c51c

const lastWord = _.last(words);

In this case, the text is @name1 name2, so lastWord is name2 and does not start with @ sign

-> The mention suggestion is not shown

What changes do you think we should make in order to solve the problem?

We should not split the text by space to detect the mention, instead of that, we should use @

1. Remove \s in MENTION_BREAKER
2. Update logic to detect mention:

            const leftString = value.substring(0, suggestionEndIndex);
            const lastIndexOfMentionSignal = leftString.lastIndexOf('@') // find the last `@`
            const lastMention = lastIndexOfMentionSignal>=0 ? leftString.substring(lastIndexOfMentionSignal) : '' // last mention
            const numberOfSpace = lastMention.split(" ").length - 1 // number of space after `@`
            let atSignIndex;
            if (lastMention) {
                atSignIndex = lastIndexOfMentionSignal
            }

            const prefix = lastMention.substring(1);

            const nextState = {
                suggestedMentions: [],
                atSignIndex,
                mentionPrefix: prefix,
            };

            const isCursorBeforeTheMention = valueAfterTheCursor.startsWith(lastMention);

            if (!isCursorBeforeTheMention && lastMention && numberOfSpace<=5) {

In the code change above, I use the additional condition numberOfSpace to count the spaces after @ signal, if it's over than 5, we don't allow to show mention (like what Slack did), so we don't need to call getMentionOptions in this case

Result

Screen.Recording.2023-10-24.at.19.05.39.mov

[twisterdotcom]

Two proposals please @allroundexperts.

[allroundexperts]

In my opinion, we should not change the character we're using to split the keyword. It makes the solution more complex. What @graylewis has proposed seems much more simple. I think we should go with there proposal.

I also think that most names won't have more than 2 spaces so we should just check for the last 2 words. @twisterdotcom and the assigned internal engineer can confirm this though.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @techievivek, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[tienifr]

@allroundexperts What happens if we allow 4-5 spaces in the name? The solution you choose may add the redundant codes

[allroundexperts]

@tienifr I think we can also do that dynamically in the PR IF requirement changes to something like 5 spaces in name.

[techievivek]

I also think that most names won't have more than 2 spaces so we should just check for the last 2 words.

I agree that, in most situations, names won't contain more than 2 spaces. Even if there are more than 2 spaces, this would still create a unique name, and mentioner should be able to select the user.

[melvin-bot]

📣 @allroundexperts Please request via NewDot manual requests for the Reviewer role ($500)

[melvin-bot]

📣 @graylewis 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[graylewis]

Just accepted the offer on Upwork. I should be able to get a PR submitted by tomorrow evening CEST

[graylewis]

@techievivek Quick clarifying point, I have the code working with all edge cases I can think of. There is one small detail that I felt I should ask about before submitting my PR.

Here's the relevant snippet with the updated code:

            const leftString = value.substring(0, suggestionEndIndex);
            const words = leftString.split(CONST.REGEX.SPACE_OR_EMOJI);
            const lastWord = _.last(words);
            const secondToLastWord = words[words.length - 3];    

            let atSignIndex;
            let suggestionWord
            let prefix;

            if (lastWord.startsWith('@')) {
                atSignIndex = leftString.lastIndexOf(lastWord);
                suggestionWord = lastWord;

                prefix = suggestionWord.substring(1);
            } else if (secondToLastWord && secondToLastWord.startsWith('@')) {
                atSignIndex = leftString.lastIndexOf(secondToLastWord);
                suggestionWord = secondToLastWord + ' ' + lastWord;

                prefix = suggestionWord.substring(1);
            } else {
                prefix = lastWord.substring(1);
            }

This code works great for all the edge cases I can think of, but it also reveals another issue. When the entire display name is supplied to the MentionSuggestions component, the getStyledTextArray() function which handles rendering the display name of the suggestion returns a lowercase string. I'm not sure why this design decision was made, but regardless it will need to be re-engineered for this edge case.

With almost the whole name:

With the whole name:

This doesn't seem like a regression, but rather an edge case that wasn't considered in the design of the getStyledTextArray function. It also doesn't seem like it's in the scope of this bug report to fix.

What should I do in this situation? Am I now responsible for this separate bug that is revealed (but not caused by) my fix?

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.5-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Mention search with last name #31435

If no regressions arise, payment will be issued on 2023-12-07. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @graylewis requires payment offer (Contributor)
• @allroundexperts does not require payment (Eligible for Manual Requests)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR:
• [@allroundexperts] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@allroundexperts] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@allroundexperts] Determine if we should create a regression test for this bug.
• [@allroundexperts] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@twisterdotcom] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[twisterdotcom]

Payment Summary:

@graylewis paid $500 here (Contributor)
@allroundexperts owed $500 via Manual Requests

Awaiting checklist completion

[allroundexperts]

This one had a regression @twisterdotcom. Should be 1/2.

[twisterdotcom]

Refund requsted for $250 from @graylewis: https://www.upwork.com/nx/wm/give-refund/f/contracts/35329365/give-refund

New Payment Summary:
@graylewis paid $250 ($500 here minus $250 here (Contributor))
@allroundexperts owed $250 via Manual Requests

[graylewis]

@twisterdotcom Refund given

[twisterdotcom]

Great. @allroundexperts do we need regression testing steps here?

[allroundexperts]

@twisterdotcom I think this was more of a feature request. As such, the checklist / regression test is not needed here.

[sakluger]

Reopening this since we have a pesky regression that we are thinking stems from here. @situchan has recommended here that we revert #31435, which was connected to this issue.

@twisterdotcom @techievivek thoughts?

[JmillsExpensify]

$250 payment for @allroundexperts based on comment above.

[twisterdotcom]

@sakluger let's move this to slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1706196697256569

[twisterdotcom]

Okay cool, so we won't revert this actual PR, but we can fix it all as one in the new issue. I don't think we would penalise @graylewis or @allroundexperts for this one.