[HOLD for payment 2023-05-25] [$1000] Chat - Comment disappears when edited in _* text _* format

[kbecciv]

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

---

Action Performed:

1. Go to URL https://staging.new.expensify.com/
2. Login with any account
3. Go to any chat
4. Send any text
5. Edit this text as *text*
6. Edit ones more as _*text*_
7. Edit again as _*text_*

Expected Result:

Comment NOT disappears

Actual Result:

Comment disappears

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.3.1

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

Bug6026844_Recording__4237.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fbc70abb602fff25
• Upwork Job ID: 1651574084753051648
• Last Price Increase: 2023-04-27

[MelvinBot]

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

[MelvinBot]

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

[NicMendonca]

@kbecciv just a heads up, when I refresh, the comment comes back -- does that happen for you as well? Or deletes permanently?

[NicMendonca]

bump @kbecciv

[kbecciv]

@NicMendonca Let me double check with current build, will update you shortly.

[kbecciv]

@NicMendonca Confirmed, the comment comes back after refresh.

Recording.2574.mp4

[NicMendonca]

Okay got it! Super weird.

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[deetergp]

I do think this can be worked in by contributors. I suspect it's very similar to #15302.

@kbecciv I took the liberty of editing your description, wrapping your example text in backticks so it shows as monospace code and isn't parsed by Github's Markdown parsers.

[alitoshmatov]

When edited with _*text_* the backend is pushing empty text message for the update:

Oddly enough, if you add a comment with _*text_* it is pushing correct data:

[eh2077]

Proposal

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

Comment disappears when edited in _* text _*.

What is the root cause of that problem?

To dig the root cause, let's start with adding the following comment

_*test_*

It's translated into html

<em><strong>test</em></strong>

from frontend through method call buildOptimisticAddCommentReportAction => getParsedComment => ExpensiMark.replace.

In method ExpensiMark.replace, the italic rule translates the markdown to

<em>*test</em>*

and then the bold rule translate the above intermediate html to

<em><strong>test</em></strong>

We find the tag <em> and <strong> are overlapping with each other. The backend will correct it to following HTML

<em><strong>test</strong></em>

See this SO discussion about overlapping tags.

So when editing the comment, the html corrected by the backend is translated to

_*test*_

If we change the markdown to

_*test_*

and save it again. The frontend will again translate it into the overlapping html

<em><strong>test</em></strong>

through method call editReportComment => handleUserDeletedLinksInHtml => ExpensiMark.replace.

And as the overlapping html is different from the original html corrected by the backend, so the overlapping html is saved to backend again. But after the backend corrects the overlapping html and compares with original html, it figures out that frontend sends the same html to save. So the backend returns empty text to frontend and the comment disappears on frontend.

So the root cause is that the italic rule and bold rule creates overlapping tags.

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

To fix this issue, I think we can have a method to validate if the text of an element tag includes non-paired tags. For example, if the text g1 of <strong> tag is test</em>, then it contains non-paired tag and we skip to format it.

We can have a method containsNonPairTag, like

/**
 * Check if the input text includes only the open or the close tag of an element. 
 *
 * @param {String} textToCheck - Text to check
 * 
 * @returns {Boolean}
 */
containsNonPairTag(textToCheck) {
    const matchedTagPairs = textToCheck.match(/<(.*)\b[^>]*>[^<>]*<\/\1>/gm) || [];
    const matchedTags = textToCheck.match(/<[^>]*>/gm) || [];
    
    if (matchedTagPairs.length * 2 !== matchedTags.length) {
        return true;
    }
    return false;
}

and use it to by change condition from

replacement: (match, g1) => (g1.includes('<pre>') ? match : `<strong>${g1}</strong>`),

to

replacement: (match, g1) => (g1.includes('<pre>') || this.containsNonPairTag(g1) ? match : `<strong>${g1}</strong>`),

We can apply similar fix for <em>, <strong> and <del> tags.

What alternative solutions did you explore? (Optional)

We can also fix the overlapping HTML from frontend in the same way as the backend does.

[eVoloshchak]

Reviewing the proposals today

[melvin-bot]

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

[zanyrenney]

Did you not hire any of the people to the upwork issue @NicMendonca ? I can't see anyone hired.

[zanyrenney]

Unfortunately - Upwork is having a site outage when I try to hire them

[zanyrenney]

offers sent @eVoloshchak @eh2077

[eh2077]

@zanyrenney Accepted. Thank you!

[zanyrenney]

Going OOO and this is pretty active so needs management by bug0 team member, as per the new process to stay assigned but assign another team member, please can you keep a watch of this. I am OOO until 8th June. Thanks!

[melvin-bot]

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

[Christinadobrzyn]

Catching up here,

@eVoloshchak is C+ - hired in Upwork
@eh2077 is contributor - hired in Upwork

Upwork job - https://www.upwork.com/jobs/~01fbc70abb602fff25

Does the speed bonus apply to this? I can't find the original PR for this fix @deetergp @eVoloshchak @eh2077

Also, do we need a regression test for this?

[deetergp]

@Christinadobrzyn the original PR is here. Not sure if the bonus applies or not.

[Christinadobrzyn]

• Thanks @deetergp! It looks like a speed bonus does not apply: PR created May 8th and merged May 15th

• Paid @eh2077 and @eVoloshchak in Upwork - closed job in Upwork

• @eVoloshchak or @eh2077 I think a regression test would apply here, please feel free to send over the testing steps if you agree! thanks!

[luacmartins]

Have we bumped the package's version in App yet?

[deetergp]

@luacmartins, Short answer: yes. The long answer is hidden in the back-scroll.

[luacmartins]

All I need is the short answer :) Thanks!

[eVoloshchak]

Regression Test Proposal

• Send a message _*text*_
• Edit message to _*text_*
• Verify the message doesn't disappear
• Verify the text is formatted in italics and isn't formatted in bold style

Do we agree 👍 or 👎

[Christinadobrzyn]

Regression test here - https://github.com/Expensify/Expensify/issues/287343

I think this GH is ready to close? @deetergp @luacmartins