[HOLD for payment 2023-06-28] [$1000] Web - Chat - Inconsistent parsing upon wrapping mentioning message with link

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open staging.new.expensify.com
2. Send this message to a chat : @concierge@expensify.com
3. Send this message to a chat : @concierge@expensify.com

Expected Result:

Both message should consistently either changed into link or mention

Actual Result:

While mentioning message without styling turned onto URL, mentioning message with styling turned onto mention-like message, but when clicked turned to be URL as well, and could cause confusion for the user

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.22.0

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

Notes/Photos/Videos: Any additional supporting documentation

Mention.Formatted.mp4

Recording.2928.mp4

Expensify/Expensify Issue URL:

Issue reported by: @kerupuksambel

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1685404731770449

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01934ef45479ce0794
• Upwork Job ID: 1668225766110015488
• Last Price Increase: 2023-06-12

[melvin-bot]

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

[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

[eh2077]

Proposal

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

Inconsistent parsing upon wrapping mentioning message with link. For example

[@test@gmail.com](google.com)

is rendered as user mention but input

[*@test@gmail.com*](google.com)

is rendered as user mention. Both cases shouldn't render user mention.

What is the root cause of that problem?

First, we apply markdown-link-to-html rule to the input

[*@test@gmail.com*](google.com)

and output following html converting bold syntax of the link text at the same time inside method modifyTextForUrlLinks

<a href="https://google.com" target="_blank" rel="noreferrer noopener"><strong>@test@gmail.com</strong></a>

Second, the userMentioins-to-html rule is unexpectedly applied to the output anchor tag above.

So the root cause is that the regex of user mention

new RegExp(`[\`.a-zA-Z]?@+${CONST.REG_EXP.EMAIL_PART}(?![^<]*(<\\/pre>|<\\/code>|<\\/a>))`, 'gm'),

matches @test@gmail.com inside the text of anchor tag

<a href="https://google.com" target="_blank" rel="noreferrer noopener"><strong>@test@gmail.com</strong></a>

We have a negative lookahead group, the ending group (?![^<]*(<\\/pre>|<\\/code>|<\\/a>)), that possibly skip to match the user mention @test@gmail.com inside the anchor tag.

The ending negative lookahead group (?![^<]*(<\\/pre>|<\\/code>|<\\/a>)) doesn't match string

<strong>@test@gmail.com</strong></a>

So, the ending negative lookahead group can't skip to match the user mention @test@gmail.com wrapped by strong tag inside the anchor tag.

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

To fix this issue, we can improve the ending negative lookahead group (?![^<]*(<\\/pre>|<\\/code>|<\\/a>)) of the regex of user mention to skip <strong> , <em>, <del> and code tags inside the <a> tag.

We can use a new ending negative lookahead group

(?!((?:(?!<a).)+)?<\\/a>|[^<]*(<\\/pre>|<\\/code>))

where includes two alternative subgroups inside it, ((?:(?!<a).)+)?<\\/a> and [^<]*(<\\/pre>|<\\/code>).

The first alternative group ((?:(?!<a).)+)?<\\/a> is responsible to fix this issue. It works as follows

1. If we find substring <a before substring </a>, then we assume that the matched user mention is not inside <a> tag
2. If we find substring </a> before substring <a, then we assume that the matched user mention is inside <a> tag and skip this match

See regex test screenshot

So the new regex of user mention will be

new RegExp(`[\`.a-zA-Z]?@+${CONST.REG_EXP.EMAIL_PART}(?!((?:(?!<a).)+)?<\\/a>|[^<]*(<\\/pre>|<\\/code>))`, 'gm'),

By doing so, input

[*@test@gmail.com*](google.com)

is translated into html not rendering user mention syntax

<a href="https://google.com" target="_blank" rel="noreferrer noopener"><strong>@test@gmail.com</strong></a>

and won't break existing unit test cases of Expensify-common lib.

The regex of @here rule have similar issue, so we can apply same fix to it as well.

What alternative solutions did you explore? (Optional)

N/A

[dummy-1111]

Proposal

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

User mention inside a link is parsed inconsistently.
For example:
@concierge@expensify.com
@concierge@expensify.com

What is the root cause of that problem?

As you can see below, user mention cannot be parsed when it's inside any html tag.

    regex: new RegExp(`[\`.a-zA-Z]?@+${CONST.REG_EXP.EMAIL_PART}(?![^<]*(<\\/pre>|<\\/code>|<\\/a>))`, 'gm'),

The main reason of this issue is that we parse link before parsing user mentions

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

I think both should be parsed as normal link. In order to do that We need to change the user-mention rule.

    {
        name: 'userMentions',
        regex: new RegExp(`((?:(?:\\W)|^)(_?))@${CONST.REG_EXP.EMAIL_PART}(\\2)(?![^<]*(<\\/pre>|<\\/code>|<\\/a>))`, 'gm'),
        replacement: (match, g1, g2, g3, g4) => {
            if (!Str.isValidMention(match)) {
                return match;
            }
            return `${g1}<mention-user>@${g3}</mention-user>${g4}`;
        },
    },

Result

19997_mac_chrome.mp4

What alternative solutions did you explore? (Optional)

[abekkala]

I wasn't able to reproduce

[abekkala]

I even tried copy/paste for @concierge@expensify.com and _@concierge@expensify.com_
And did not have the same outcome as the issue states

[dummy-1111]

I even tried copy/paste for @concierge@expensify.com and _@concierge@expensify.com_ And did not have the same outcome as the issue states

You need to try with
[@concierge@expensify.com](https://google.com/)
[_@concierge@expensify.com_](https://google.com/)

You can check that here in result section

[eh2077]

Hi @abekkala , I think the reproduction steps is not clear enough. We should use underscore _ to wrap the text part of the link to reproduce it. For example we can use below input

[@concierge@expensify.com](https://google.com/)

Only wrap the text part of link
[_@concierge@expensify.com_](https://google.com/)

to duplicate it, see demo video

Screen.Recording.2023-06-04.at.9.15.52.AM.mov

[eh2077]

@abekkala I can still reproduce it. I think we should reopen this issue.

I would also cc @puneetlath as you're most familiar with the mention feature.

[puneetlath]

Hm, yes if it's still a bug then let's fix.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01934ef45479ce0794

[melvin-bot]

Current assignee @abekkala is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

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

[dummy-1111]

Proposal

Updated

[s77rt]

@eh2077 Thanks for the proposal. Your RCA is correct and the fix is acceptable, we can probably go with that as that's how we used to fix a similar issue on autolink regex. But can we fix this in a simpler way without modifying the regex? i.e. move the userMentions regex to be before both link and bold/italic/strikethrough rules?

[s77rt]

@s-alves10 Thanks for the proposal. You RCA is not really correct, if it was this issue wouldn't even exist but I see you updated your proposal recently so I assume the RCA is outdated. As for the fix I guess it would work but it's better not to match the regex at all rather than matching and rejecting, or even better avoid any regex changes by altering the order of rules if possible (same note given above).

[dummy-1111]

@s77rt
Yes, I suggested as follows at first

We can solve the issue by simply changing the rule order.
Put the user mention rule and here mention rule before the email rule

But I thought mention-user parsing has some problems and needs to be fixed. In addition, changing rule order may have other issues. That's why I changed the solution.

[eh2077]

@s77rt Thanks for reviewing proposals!

But can we fix this in a simpler way without modifying the regex? i.e. move the userMentions regex to be before both link and bold/italic/strikethrough rules?

I think, technically, it's possible to achieve it while as

1. We don't want to parse mentions(including user mention and @here mention) inside <a> tag and
2. The current regex of user mention actually depends on the parsing order to skip parsing within <a> tag, see the regex of userMentions

regex: new RegExp(`[\`.a-zA-Z]?@+${CONST.REG_EXP.EMAIL_PART}(?![^<]*(<\\/pre>|<\\/code>|<\\/a>))`, 'gm'),

If we want to fix this issue through changing the order, we'll need to figure out a new way to skip parsing mentions within markdown link syntax, like

[@username@expensify.com](expensify.com)

So, I think re-using the method we have explored previously will be easier.

[s77rt]

@s-alves10 Thanks for the update. I have checked the order of rules. The correct order should be:

1. Link
2. Mentions
3. Bold/Italic/Strikethrough

This is actually the current order already, except that the link rule calls modifyTextForUrlLinks which applies bold/italic/strikethrough rules and cause the mentions rule to mismatch.

[s77rt]

@eh2077 Thanks for the follow up. I have provided some info regarding the order of rules above. I think it's indeed safer and easier to follow the same approach we did with autolink and that is to alter mentions regex as you suggested. Let's go with that.

🎀 👀 🎀 C+ reviewed

cc @johnmlee101

[melvin-bot]

📣 @eh2077 You have been assigned to this job by @johnmlee101!
Please apply to this job in Upwork 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 📖

[eh2077]

@s77rt @johnmlee101 The PR Expensify/expensify-common#549 for expensify-common repo is ready. Please help to review it when you get time, thanks.

[melvin-bot]

🎯 ⚡️ Woah @s77rt / @eh2077, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @eh2077 got assigned: 2023-06-13 18:41:47 Z
• when the PR got merged: 2023-06-15 14:24:41 UTC

On to the next one 🚀

[melvin-bot]

@johnmlee101, @abekkala, @s77rt, @eh2077 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[s77rt]

Not overdue. PR is merged and waiting for deployment.

[eh2077]

Not overdue. PR is merged and waiting for deployment.

@s77rt The PR has been deployed to the production because it occasionally shared the same commit with the other PR #20769. @johnmlee101 pointed it out here #20767 (comment).

So, actually, the payment date of this issue should be same as issue #20111.

Sorry for the confusion caused!

cc @abekkala @johnmlee101

[s77rt]

Oh right, so we have to track this one manually

[abekkala]

PAYMENTS TO BE MADE JUNE 28

• Issue reported by: @kerupuksambel [$250]
• Selected Proposal for fix: @eh2077 [$1000] + [$500 PR bonus]
• C+ Review: @s77rt [$1000] + [$500 PR bonus]

[abekkala]

@kerupuksambel @eh2077 @s77rt - contract payment offers have been sent!

[eh2077]

@abekkala Accepted. Thank you!

[abekkala]

@eh2077 payment sent and contract ended - thank you! 🎉

[s77rt]

@abekkala Accepted!

[abekkala]

@s77rt payment sent and contract ended - thank you! 🎉

[kerupuksambel]

@kerupuksambel @eh2077 @s77rt - contract payment offers have been sent!

Accepted! @abekkala

[abekkala]

@kerupuksambel payment sent and contract ended - thank you! 🎉

Closing