[$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?

• [x] Android / native
• [x] Android / Chrome
• [x] iOS / native
• [x] iOS / Safari
• [x] MacOS / Chrome / Safari
• [x] 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

https://user-images.githubusercontent.com/93399543/233667721-50f2be6f-2069-48ad-a565-67ad27598d99.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.

https://user-images.githubusercontent.com/93399543/234054742-7e3faf86-3609-415c-b81f-2179ff18ebca.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 https://github.com/Expensify/App/issues/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

[eVoloshchak]

@eh2077, awesome job digging this one! This is a weird one indeed

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.

Isn't that the root cause? What would happen if the backend did return the message instead of an empty string?

[eh2077]

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.

Isn't that the root cause? What would happen if the backend did return the message instead of an empty string?

@eVoloshchak Thanks for reviewing my proposal. Yes, I think it would be the root cause in some degree. The backend does two things here, correcting the invalid html and deciding to send a pusher with empty message to frontend, see below about how the pusher message looks like

[info] [Report] Handled onyxApiUpdate event sent by Pusher - [{"onyxMethod":"merge","key":"reportActions_8192476963090779","value":{"8307691491001029080":{"message":[{"type":"COMMENT","html":"","text":"","isEdited":false,"whisperedTo":[]}]}}},{"onyxMethod":"merge","key":"report_8192476963090779","value":{"lastMessageText":"testa","lastMessageHtml":"<em><strong>testa</strong></em>","isEdited":false,"lastActionCreated":"2023-05-01 23:31:07.032","lastActorEmail":"[email protected]"}}]
clientLoggingCallback — Log.js:55

I think correcting/validating the message sent by frontend is normal and necessary. While what the backend should return if frontend post the same data without change? I agree returning empty data looks weird. I think the backend can consider to return error with proper http code. I found a related SO discussion FYI.

But I think the frontend fix is also necessary.

[eVoloshchak]

While what the backend should return if frontend post the same data without change? I agree returning empty data looks weird. I think the backend can consider to return error with proper http code. I found a related SO discussion FYI.

Yes, I agree with this.

But I think the frontend fix is also necessary.

Could you elaborate a bit more on this? If we change the back end to return an error code instead of an empty string, wouldn't this issue be resolved?

[eh2077]

But I think the frontend fix is also necessary.

Could you elaborate a bit more on this? If we change the back end to return an error code instead of an empty string, wouldn't this issue be resolved?

Hmm, I just feel we usually don't like to autocorrect the user input text and always display user input as it's. In this case the backend will auto fix the html of user markdown input

_*text_*

So, the sent message in the editing mode is

_*test*_

Then what about user input like

_italic *and_ bold*

The autocorrect feature provided by the backend maybe confusing to end users. So, I think it'll be better if frontend and backend behave consistently. In my proposal, I suggested to let frontend not to create the unusual html like

<em>italic <strong>and</em> bold</strong>

[eVoloshchak]

In this case the backend will auto fix the html of user markdown input

What if we don't update the message if backend has returned an error?

1. Send a message (text)
2. Edit the message to *text*
3. Backend has returned an error since data is the same
4. We do nothing, since the message hasn't changed

[eh2077]

In this case the backend will auto fix the html of user markdown input

What if we don't update the message if backend has returned an error?

1. Send a message (text)
2. Edit the message to *text*
3. Backend has returned an error since data is the same
4. We do nothing, since the message hasn't changed

Oh, yes, I think the comment disappear issue can be fixed by doing so.

[eVoloshchak]

@deetergp, as per @eh2077's proposal

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.

Which is causing front end to delete the message. Can the backend return an error code instead?

[deetergp]

It seems a bit silly that we are fixing this on the back end at all—we should be taking care of it on the client. We are in the planning stages of doing a live preview of markdown, and the only way for that to really work is for the rendering to be done on the front end. I think for now we should go with @eh2077's proposal and just skip rendering invalid markdown will suffice. I'd wager that we'll end up ripping out the backend code that corrects invalid Markdown before much longer…

[melvin-bot[bot]]

@deetergp @eVoloshchak @NicMendonca this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[eVoloshchak]

I think for now we should go with @eh2077's proposal and just skip rendering invalid markdown will suffice. I'd wager that we'll end up ripping out the backend code that corrects invalid Markdown before much longer…

Agree, let's proceed with @eh2077's proposal @deetergp, could you please assign @eh2077 to this job?

[melvin-bot[bot]]

📣 @eh2077 You have been assigned to this job by @NicMendonca! 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]

@eVoloshchak @deetergp The PR https://github.com/Expensify/expensify-common/pull/530 for Expensify common repo is ready for review. Please help to review it at your convenience, thanks!

[NicMendonca]

Not overdue. PR is in review

[melvin-bot[bot]]

@deetergp @eVoloshchak @NicMendonca @eh2077 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[NicMendonca]

Looks like PR s now on hold: https://github.com/Expensify/App/issues/17214#issuecomment-1547873609