[Hold #44262] [$250] Chat-Quote Markdown without space is applied as a quote in sent chat but not in compose box

[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!

---

Version Number: 1.4.80-14
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. Navigate to any chat
2. Type >hey (There shouldn't be a space between ">" and "hey")
3. In a second line type > hey (with a space between ">" and "hey")
4. In a third line line type > hey (with two spaces between ">" and "hey")
5. Send the message

Expected Result:

The first line shouldn't be rendered as a quote markdown. It should be rendered as '>hey'

Actual Result:

The first line without space is rendered as a quote.

Workaround:

unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6505597_1717793936542.2024-06-07_23_11_32.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01352a9a30d580bacb
• Upwork Job ID: 1800195684107077831
• Last Price Increase: 2024-06-17

Issue Owner

Current Issue Owner: @sobitneupane

[melvin-bot]

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[kavimuru]

@trjExpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

[kavimuru]

We think this bug might be related to #vip-vsb

[devguest07]

##Proposal

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

Chat-Quote Markdown without space is applied as a quote in sent chat but not in the compose box.

What is the root cause of that problem?

When typing in the composer, the text is saved to the database (Onyx) with the getParsedComment function.

getParsedComment uses a parser from expensify-common that converts markdown into HTML.

The issue lies in the getParsedComment function performing incorrect transformations. For example, a text like >w/n> w should be transformed to:

>w
<blockquote>w</blockquote>

However, it is being transformed to <blockquote>W<br />c</blockquote>.

The function responsible for this transformation in expensify-common needs to be reviewed.

The text input contains > which is captured by this regex:

https://github.com/Expensify/expensify-common/blob/6c07092648afd712251e25d05d54fcb303fda618/lib/ExpensiMark.js#L259

When the regex matches, the text is split into textSplitted and each line is processed separately.

https://github.com/Expensify/expensify-common/blob/6c07092648afd712251e25d05d54fcb303fda618/lib/ExpensiMark.js#L976

The issue occurs in this condition: if (Str.startsWith(textSplitted[i], '>') && !insideCodefence) {

https://github.com/Expensify/expensify-common/blob/6c07092648afd712251e25d05d54fcb303fda618/lib/ExpensiMark.js#L985

This condition determines whether to process the line with > or not. In our case, a line like >test should not be processed as a quote.

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

To resolve the issue, we need to add another condition to this line:

if (Str.startsWith(textSplitted[i], '>') && !insideCodefence) {

We also need to check if the > is followed by a space or there's nothing after a consecutive >

We should maintain examples like > test, >>> test, > > >test, etc.

The following adjustment can be made:

// Check for the last occurrence of consecutive > and get the character after it
const match = textSplitted[i].match(/(>)+/);
let charAfterLastGt = '';
let nothingAfterGt = false

if (match) {
  const lastIndex = match.index + match[0].length;
  charAfterLastGt = textSplitted[i].charAt(lastIndex); // The character after the last consecutive >
  nothingAfterGt = lastIndex >= textSplitted[i].length;
}

// We only want to modify lines starting with > that is not codefence
// also if the chart after the last `>` is a space or there's nothing after the `>`        
if (Str.startsWith(textSplitted[i], '>') && !insideCodefence && (charAfterLastGt === ' ' || nothingAfterGt)) {                 
  textToFormat += `${textSplitted[i]}\n`;
} else {

Tested on expensify-common with success.

43323-1.mp4

What alternative solutions did you explore?

[trjExpensify]

Can repro, moving External.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01352a9a30d580bacb

[melvin-bot]

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

[sobitneupane]

@devguest07 Thanks for your proposal.

We should maintain examples like > test, >>> test, > > >test, etc.
(!/[a-zA-Z]/.test(charAfterGt)

I understand that we need to consider hierarchal quotes but won't it causes inconsistency when the character is not an alphabet. For example >12, >>, >& and so on.

[devguest07]

@sobitneupane Sure, I can adjust the charAfterGt in my proposal to fit all those cases.

The idea is, after the > when it's not a space or a second >, we should not proceed to make it a quote.

Additionally, we can add examples like >12, >>, and >& to the tests inside expensify-common.

[sobitneupane]

Sure, I can adjust the charAfterGt in my proposal to fit all those cases.

@devguest07 Can you please update your proposal to reflect the changes required?

[devguest07]

@sobitneupane proposal updated

I reversed the logic, instead of seeking if there's some letter after the >, I am now making sure there's a space after a > and it's not some > alone.

I added a test to ExpensiMark

test('Test markdown quotes without spaces with a second line with a space', () => {
    const testString = '>test\n> test';
    const resultString = '&gt;test<br /><blockquote>test</blockquote>';
    expect(parser.replace(testString)).toBe(resultString);
});

and it's passed successfully.

[badeggg]

Proposal

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

Inconsistency of display for same comment in composer box and chat history. Specifically:

>a
> b
> c

What is the root cause of that problem?

Regex /^(?:&gt;)+ +(?! )(?![^<]*(?:<\/pre>|<\/code>))([^\v\n\r]+)/gm is causing this condition if (match !== null) to true for comment:

>a
> b
> c

And condition Str.startsWith(textSplitted[i], '>') is causing >a appended to textToFormat for further quote handling.

Which is not identical for same comment text in composer box. In composer box, >a is not regarded as quote.

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

We can either let composer box recognize >a as quote, or let chat history do not recognize >a as quote. Based on this comment of @dannymcclain, I think the design team prefer the former one(recognize >a as quote).

To achieve former behavior, we should change regex from:

const regex = /^(?:&gt;)+ +(?! )(?![^<]*(?:<\/pre>|<\/code>))([^\v\n\r]+)/gm;

to

const regex = /^(?:&gt;)+ *(?! )(?![^<]*(?:<\/pre>|<\/code>))([^\v\n\r]+)/gm;

, from matching '1 or more' spaces to matching '0 or more' spaces.

Similar change should be applied to this regex too.

To achieve later behavior, we should change '>' to:

'> '

, matching one space after >

What alternative solutions did you explore? (Optional)

N/A

[sobitneupane]

This issue might be in the scope of #42082. Asked here

[melvin-bot]

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

[sobitneupane]

@trjExpensify Let's hold this issue as it will probably be covered by #42082

[BrtqKr]

Probably the same as I mentioned here. I'll let you know if it's not

[trjExpensify]

Put it on hold!

[BrtqKr]

Ok, unlike the second issue it's unrelated, but it looks like a regex swap, so I can switch it in that PR I've mentioned

[trjExpensify]

Great! Let's link that PR to this issue as well then. :)

[melvin-bot]

@trjExpensify, @sobitneupane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[trjExpensify]

I think we might be good here?

[trjExpensify]

@BrtqKr confirming this issue can be closed?

[BrtqKr]

@trjExpensify it still has to be enabled in the expensify/app, but after this thing yes

[trjExpensify]

Okay great, I'm going to update the issue number we're held on.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

This issue has not been updated in over 15 days. @trjExpensify, @sobitneupane eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[trjExpensify]

Deployed to prod three days ago: #44262 (comment)

So we can close this.