[$500] Markdown - Sending a message with two ">" characters replaces one ">" with a whitespace

[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.4.23-0
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Open a conversation
2. Send a quote markdown message with one ">" before a text
3. Send another quote markdown message with two ">" characters before a text
4. Click on edit on the message sent on step 2
5. Click on edit on the message sent on step 3 and compare it with step 4's result

Expected Result:

One of the ">" characters should not be replaced with a white space

Actual Result:

One of the ">" characters becomes replaced by a whitespace

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

Add any screenshot/video evidence

Bug6336731_1704809430448.quote.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010d9e66996671f8b8
• Upwork Job ID: 1744725330977787904
• Last Price Increase: 2024-01-23

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~010d9e66996671f8b8

[melvin-bot]

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

[melvin-bot]

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

[ghost]

I am interested to work on it

[vadymbokatov]

Proposal

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

When sending a message containing double ">" in any conversations, the second ">" gets replaced by an empty spaced.

What is the root cause of that problem?

The problem is comming from the expensify-common package. More specifically, the ExpensiMark component. While trying to find the root cause, I came across this function:

function getParsedComment(text: string): string {
    const parser = new ExpensiMark();
    return text.length <= CONST.MAX_MARKUP_LENGTH ? parser.replace(text) : lodashEscape(text);
}

Located in /src/libs/ReportUtils.ts. It is using the parser.replace method which is coming from import ExpensiMark from 'expensify-common/lib/ExpensiMark'; and is causing this issue. I will update my proposal with more details.

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

The parser.replace method from ExpensiMark package should be updated as such to prevent removing the second ">".

Update:

More specifically, by replacing the following regex inside the ExpensiMark package, I managed to fix this issue:

// Old Regex
/^>( )?/gm

// Suggested Regex
/^(>){,1}( )?/gm

This way, we assure that the match happens only once and both of the $gt; texts don't get replaced.

What alternative solutions did you explore? (Optional)

None.

[vadymbokatov]

Proposal

Updated

[bernhardoj]

Proposal

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

Sending quote markdown with double >> will not show the second >

What is the root cause of that problem?

When we are creating a message with markdown, it will call the ExpensiMark parser replace function. For quote, there are 2 cases, keeping any whitespace or trimming all whitespace.
https://github.com/Expensify/expensify-common/blob/23c34c6d37d8b46a3d0a1fd287da4551416bc9dd/lib/ExpensiMark.js#L209-L212

By default, adding a new comment will call the parser replace with trimming all whitespace. If we trim all whitespace, it will call modifyTextForQuote which will then call formatTextForQuote. In formatTextForQuote, we want to remove the first > for each line and trim it. This logic was added in Expensify/expensify-common#521
https://github.com/Expensify/expensify-common/blob/23c34c6d37d8b46a3d0a1fd287da4551416bc9dd/lib/ExpensiMark.js#L814-L819

If we send > >a, the result will be >a.

This worked fine before, but then, it will continue by calling the rules replacement logic that will also remove the > which was added by this later PR. This PR is the one that added whether we want to keep the whitespace or not.
https://github.com/Expensify/expensify-common/blob/23c34c6d37d8b46a3d0a1fd287da4551416bc9dd/lib/ExpensiMark.js#L214-L224

So the final message becomes a and wrapped with <blockquote> tag.

In summary, we have 2 cases of quote, keeping the whitespace or trimming it. Both cases will call the replacement logic which will remove the >, but for the trim case, we have another logic to remove > too (before calling the replacement logic), so the > is removed twice.

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

To fix this, we should have the > removing logic in one place and that is in the replacement function.

1. In formatTextForQuote, delete the > removing logic and pass the text unmodified to the replacement logic
   https://github.com/Expensify/expensify-common/blob/23c34c6d37d8b46a3d0a1fd287da4551416bc9dd/lib/ExpensiMark.js#L813-L820
   This will ensure the > removing logic only happens once.
2. In the replacement logic, we will set isStartingWithSpace to true only if shouldKeepRawInput is true, and also we want to trim the textToReplace if shouldKeepRawInput is true
   https://github.com/Expensify/expensify-common/blob/23c34c6d37d8b46a3d0a1fd287da4551416bc9dd/lib/ExpensiMark.js#L217-L223

let textToReplace = g1.replace(/^>( )?/gm, (match, g2) => {
    isStartingWithSpace = !!g2 && shouldKeepRawInput;
    return '';
});
if (!shouldKeepRawInput) {
    textToReplace = textToReplace.trim()
}

We need to trim it here because we already remove the > removing logic from formatTextForQuote.

[mallenexpensify]

Checking on summin' internally
https://expensify.slack.com/archives/C01SKUP7QR0/p1704850748783939

[melvin-bot]

@ntdiary, @mallenexpensify Eep! 4 days overdue now. Issues have feelings too...

[suneox]

Proposal

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

Markdown - Sending a message with two ">" characters replaces one ">" with a whitespace

What is the root cause of that problem?

Before submit message we have getParsedComment, and this function is using ExpensiMark to replace the current message to markdown at this line
The rule for quote is replace > 2 times on both method process and replacement.
At step process when formatTextForQuote we remove 4 first characters > at this line after that we continue to excute replacement function has provide by quote rule in this function we continue to replace > at this line

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

At function formatTextForQuote we should only repare the content for replacement function so we will remove logic to remove 4 first characters substr(4) at this line
change to

let textToFormat = textToCheck.split('\n').map(row => row.trim()).join('\n');

What alternative solutions did you explore? (Optional)

[mallenexpensify]

Checking a different spot internally to see what we want to do here
https://expensify.slack.com/archives/C066HJM2CAZ/p1705366484642439

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

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

[ntdiary]

Checking a different spot internally to see what we want to do here https://expensify.slack.com/archives/C066HJM2CAZ/p1705366484642439

Waiting for the result of the internal discussion.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[mallenexpensify]

Going to close for now since it's a tiny bug and not directly related to a VIP or wave project on our roadmap