[HOLD for payment 2023-06-09] [$1000] Composer input is empty when editing the word [block] in chat

[kavimuru]

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. Log into staging.new.expensify.com
2. Click into any chat
3. Type the word [block] and send it as a message
4. Click the pencil icon to edit the comment
5. Notice the composer input is empty

Expected Result:

The composer input should be text [block] in editing mode

Actual Result:

The composer input is empty

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
Reproducible in staging?: y
Reproducible in production?: y
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

Recording.304.mp4

Screen.Recording.2023-04-21.at.5.32.28.AM.mov

Expensify/Expensify Issue URL:
Issue reported by: @eh2077
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681920140787049

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a0c0a4619ff68e29
• Upwork Job ID: 1650565300774252544
• Last Price Increase: 2023-04-24

[MelvinBot]

Triggered auto assignment to @maddylewis (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

[MelvinBot]

@maddylewis Whoops! This issue is 2 days overdue. Let's get this updated quick!

[maddylewis]

oh, i see. it's if you literally type the word [block] and then try to edit:

2023-04-24_13-40-01.mp4

[MelvinBot]

Triggered auto assignment to @cead22 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[maddylewis]

hiya - just confirming that this can be fixed externally before i add the corresponding label!

[cead22]

@maddylewis yes this can be fixed externally, so you can apply the label. Can you update the issue description to make it extra clear what the steps to reproduce should be, and remove any boilerplate info from there?

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[eh2077]

Proposal

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

Editing comment [block] shows empty in the composer.

What is the root cause of that problem?

When editing comment [block], we translate it into markdown here

App/src/pages/home/report/ReportActionItemMessageEdit.js

Line 91 in f50afdf

const draftMessage = parser.htmlToMarkdown(this.props.draftMessage).trim();

through method htmlToMarkdown. The string [block] is used as a special separator to handle line break between markdown block elements. From method replaceBlockWithNewline, we split input htmlString by separator [block] and handle line breaks between markdown block elements. In this case, the input htmlString is equal the the separator [block], so it's treated as the separator and is eliminated to empty string, which results in empty composer in editing mode.

So the root cause of this issue is that we choose string [block] which is a relatively common user input string as the separator to handle line break between markdown block elements.

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

To fix this issue, we should avoid to have a separator that possibly conflicts with normal/visual user input text.

1. We can choose the zero width space character \u200B to wrap string block instead using bracket []. So we use string \u200Bblock\u200B as the separator to handle line break between markdown block elements. We can search string [block] in ExpensiMark.js and replace all with \u200Bblock\u200B.

2. To fix the special case of pasting html includes ​block​ (note \u200B is encoded as ​ in html), we can replace \u200Bblock\u200B as block before applying html-to-markdown rules here, like

   generatedMarkdown = generatedMarkdown.replace(/\u200Bblock\u200B/g, 'block');

   So the separator \u200Bblock\u200B won't conflict with user input during subsequent html-to-markdown rules translation.

   Note that we call htmlToMarkdown to convert html into markdown when we pasting html into composer.

The new separator string \u200Bblock\u200B has following advantages

1. It won't conflict with user input that includes string [block].
2. It's safe to replace(/\u200Bblock\u200B/g, 'block') before html-to-markdown translation as the zero width character is not visible and removing it can ensure no conflicts with normal user input.
3. The implementation of method htmlToMarkdown is self-sufficient compared to the alternative solution as described below.

We can also simplify the separator a little bit by using block\u200B.

What alternative solutions did you explore? (Optional)

We can also use separator, like [block] (escaping brackets) as

1. We always escape user input [block] to [block] before saving it to backend, so see getParsedComment and ExpensiMark.replace
2. For pasting html case, both [block] and [block] are not conflicted with the separator
3. For pasting text case, [block] is decoded to [block] here

While method htmlToMarkdown isn't self-sufficient as it requires the potential separator [block] in the input html to be decoded or escaped outside of the method.

[MelvinBot]

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

[tienifr]

Proposal

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

The composer is empty if editing the text [block] that was sent.

What is the root cause of that problem?

The root cause is from here https://github.com/Expensify/expensify-common/blob/e93e1eb448ad6bdbde911fd6239f70d5e749635e/lib/ExpensiMark.js#L531, we use [block] as a separator in place of a number of html tags as can be seen in https://github.com/Expensify/expensify-common/blob/e93e1eb448ad6bdbde911fd6239f70d5e749635e/lib/ExpensiMark.js#L528, then we have logic to replace [block] by new lines in https://github.com/Expensify/expensify-common/blob/e93e1eb448ad6bdbde911fd6239f70d5e749635e/lib/ExpensiMark.js#L475.

So the problem is the [block] text entered by the user happens to be the one used internally by us.

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

A question is: why does it happen for block but doesn't happen if we edit raw html tags like <h1>, <em> that were sent?

That's because we have the logic to escape the special characters in https://github.com/Expensify/expensify-common/blob/e93e1eb448ad6bdbde911fd6239f70d5e749635e/lib/ExpensiMark.js#L333, so <h1> entered by the user will become <h1>, thus will never collide with the HTML tags that we convert from the markdown.

We should do the same for the [block] special string that we use internally. We can replace all instances of [block] by <block>, it will never collide with the user input because it has special html characters (<, >). It will also look logical because we're replacing a number of legit HTML tags by <block> for further processing in https://github.com/Expensify/expensify-common/blob/e93e1eb448ad6bdbde911fd6239f70d5e749635e/lib/ExpensiMark.js#L528, so <h1> -> <block> makes more sense to me than [block].

Aside from this, we should also modify the https://github.com/Expensify/expensify-common/blob/3cdaa947fe77016206c15e523017cd50678f2359/lib/str.js#L315 so that it will strip all HTMLs excluding our reserved <block>, so that <block> won't be strip out in here. The function name can be changed to clarify that it won't strip <block>.

What alternative solutions did you explore? (Optional)

As long as we replace [block] by any string that has a HTML special character, it will most likely work. I prefer the <, > pair since it looks logical and already proven since with the current logic we can process raw HTML tags sent by the user without any issue.

Another potential solution is to remove all occurrences of [block] altogether, and we'll split the HTML for processing by using regex ([block] actually represents a group of regex anyway). This will be a bit more complex.

[rushatgabhane]

@eh2077 @tienifr Are there any other similar issues that you can find by looking at the code? Let's fix all of them in one go.

[eh2077]

Hi @rushatgabhane, I think [block] is the only special string that might conflict with user's input in the expensify-common lib.

[tienifr]

yeah, agree with @eh2077 👍

[maddylewis]

apologies! sending offers now!

[melvin-bot]

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

[maddylewis]

hiya @flaviadefaria - all that's needed here is:

• process payment for @tienifr (offer here - https://www.upwork.com/nx/wm/offer/25014584) - completed on June 14
• process payment for @rushatgabhane (offer here - https://www.upwork.com/nx/wm/offer/25014435)
• bump @rushatgabhane to complete the BZ checklist if need be
• create a GH for a regression test update if applicable

I'll be back on June 20 - thanks!

[flaviadefaria]

@rushatgabhane can you please complete the list here so that I can issue your payment? Thanks!

[flaviadefaria]

Still waiting for @rushatgabhane to fill the tasks I mentioned above so that I can issue payment and close this.

[rushatgabhane]

Hi, could we please change this to a weekly? I won't be able to complete the checklist until Friday 😅

[maddylewis]

all good! i will keep at daily just for my own organization. but, no worries if you don't complete the checklist until Friday 👍

[flaviadefaria]

@maddylewis since you're back I'll unassign myself from this issue.

[maddylewis]

@rushatgabhane - let us know know if you think a regression test update is required for this one - thanks!

[rushatgabhane]

@maddylewis no regression test required. This can never happen again

[rushatgabhane]

Checklist

1. The PR that introduced the bug has been identified. Link to the PR: Not a bug but a known drawback of the implementation.

2. [@rushatgabhane] 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: not a bug.

3. [@rushatgabhane] 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:
   [@rushatgabhane]: NA

4.Determine if we should create a regression test for this bug. NO

[maddylewis]

thanks @rushatgabhane - just waiting for you to accept the offer I sent you and then i will close this out 👍

[rushatgabhane]

Hey @maddylewis , Anu is setting up global payments through expensify for us. It might take another week or two, and we'll have the clarity of next steps for payment.

Until then, we could make this a weekly issue? I hope this is okay :)

https://expensify.slack.com/archives/C02NK2DQWUX/p1687451792686509?thread_ts=1686871517.189609&cid=C02NK2DQWUX

[maddylewis]

of course! thank you for letting me know :)

moving to weekly.

[rushatgabhane]

Jul 5 is when I'll get the confirmation if manual requests worked as expected

[rushatgabhane]

Hi @maddylewis, I made a manual request on new dot - https://staging.new.expensify.com/r/5901048936724743

[rushatgabhane]

This is paid by Anu, and can be closed.