[$250] Chat - After editing nested quote message, the markdown is removed

[lanitochka17]

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: 9.0.24-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4884121
Email or phone of affected tester (no customers): applausetester+vd_web082324@applause.expensifail.com
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Log in
3. Go to any chat
4. Send a nested quote message, like "> > test"
5. Verify that it looks as expected on the chat
6. Edit the message

Expected Result:

The markdown should still be displayed after editing the message

Actual Result:

The quote markdown effect is removed after editing a nested quote message

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

Add any screenshot/video evidence

Bug6580735_1724447327796.bandicam_2024-08-23_17-01-00-816.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a7ea8bf9646df697
• Upwork Job ID: 1829295392648638850
• Last Price Increase: 2024-08-29

Issue Owner

Current Issue Owner: @fedirjh

[melvin-bot]

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

[lanitochka17]

@twisterdotcom 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

[lanitochka17]

We think that this bug might be related to #vip-vsp

[bernhardoj]

Proposal

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

After editing a nested quote markdown, the quote markdown is removed.

What is the root cause of that problem?

When we send > > test and edit the message, the markdown becomes >> test because we add the > without space and only add the space after the >. We can change that but I want to focus on a real issue explained below.
https://github.com/Expensify/expensify-common/blob/d11466167bc562447d2dfdad624145040153efae/lib/ExpensiMark.ts#L624

A quote markdown requires a space after >, for example, > test. But if we have a nested quote, >> test, there is an inconsistent logic when parsing it. If we see the quote markdown regex, we allow the nested quote without spaces between >, so >> test is a valid nested quote.
https://github.com/Expensify/expensify-common/blob/d11466167bc562447d2dfdad624145040153efae/lib/ExpensiMark.ts#L376

modifyTextForQuote is responsible to process the matched text (>> test) into the HTML form. It splits the text by new line and process each line whether it should be converted to a quote HTML or not. On each line, it checks whether the line starts with > or not.
https://github.com/Expensify/expensify-common/blob/d11466167bc562447d2dfdad624145040153efae/lib/ExpensiMark.ts#L1138

This shows that the logic doesn't cover the case of nested quote yet. So, you don't need to edit the message but just sends >> test and it won't be converted to a quote (the live markdown converts it to quote though because it doesn't get through modifyTextForQuote processing).

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

To allow nested quote have optional space between >, we can update the logic to accept either space or another > after > (we can use regex to shorten it, ^>(>| ))

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

This will convert it to markdown for below inputs:

> > test
>> test

[ChavdaSachin]

Edited by proposal-police: This proposal was edited at 2024-08-30 20:43:57 UTC.

Proposal

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

Editing quote markdown message does not work as desired

What is the root cause of that problem?

RCA for the issue is -
HTML to markdown parsing rule does not reproduce equivalent Markdown.
There are 2 issues -

• when editing nested quotes, returned text does not contain space in between two '>' s.
• when editing multiline quotes, when preceding line have more quote than following line, no '>' for following line is returned

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

So, to tackle this we need to completely redesign

• quote rule with -
  Updated Rule

            {
                name: 'quote',
                regex: /<(blockquote|q)(?:"[^"]*"|'[^']*'|[^'">])*>(([\s\S]*?)<\/\1>)+(?![^<]*(<\/pre>|<\/code>))/gi,
                replacement: (_extras, _match, _g1,g2) => {
                    // We remove the line break before heading inside quote to avoid adding extra line
                    let resultString = _match
                        .replace(/\n?(<h1># )/g, '$1')
                        .replace(/(<h1>|<\/h1>)+/g, '\n')
                        .replace(/(<\/blockquote>)+/g, (match)=> { match + '\n' })
                        .trim()
                        .split('\n');
                    // Wrap each string in the array with <blockquote> and </blockquote>
                    let depth =0;
                    resultString = resultString.map((line) => {
                        let modifiedLine = line;
                            depth+= modifiedLine.match(/<blockquote>/gi)?.length || 0;
                            modifiedLine = modifiedLine.replace(/<blockquote>/gi, '');
                            modifiedLine = modifiedLine.replace(/<\/blockquote>/gi, '');
                            let resultString = `${'<blockquote>'.repeat(depth)}${modifiedLine}${'</blockquote>'.repeat(depth)}`;
                            depth-=line.match(/<\/blockquote>/gi)?.length || 0;
                            return resultString;
                    });
                    resultString = resultString
                        .map((text) => {
                        let modifiedText = text;
                        let depth;
                        do {
                            depth = (modifiedText.match(/<blockquote>/gi) || []).length;
                            modifiedText = modifiedText.replace(/<blockquote>/gi, '');
                            modifiedText = modifiedText.replace(/<\/blockquote>/gi, '');
                        } while (/<blockquote>/i.test(modifiedText));
                        return `${'> '.repeat(depth)}${modifiedText}`;
                    })
                     .join('\n');
                    return `<blockquote>${resultString}</blockquote>`;
                },
            },

• this line with -
  Updated logic

                if ((textSplitted[i].match(/^( *(> )+)+ /gm)  || isLastBlockquote) && !insideCodefence) {

What alternative solutions did you explore? (Optional)

Changing this rule slightly would provide correct markdown equvalent.
Change Here from -

return ${'>'.repeat(depth)} ${modifiedText};
To -

return ${'> '.repeat(depth)}${modifiedText};
changed blankspace position.
We could also use same approach with little bit of modification if we want to add support for multiple '>'s without space in between.

[melvin-bot]

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

[melvin-bot]

@twisterdotcom Eep! 4 days overdue now. Issues have feelings too...

[twisterdotcom]

Agree, this one was simple to reproduce. Apologies for the time, I am just back from OOO.

[melvin-bot]

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

[melvin-bot]

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

[ChavdaSachin]

updated proposal, added alternative solution which looks more sensible to me

[melvin-bot]

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

[twisterdotcom]

Bump @fedirjh on these proposals

[fedirjh]

@bernhardoj's Proposal looks good to me. Let's use the regex as it covers different variations of nested quotes with spaces.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[ChavdaSachin]

@fedirjh It looks like two blockquotes must be separated by space is recommended design feature. So, we might need a design review on this one. check #45154 (comment) .

[bernhardoj]

When we send > > test and edit the message, the markdown becomes >> test because we add the > without space and only add the space after the >. We can change that but I want to focus on a real issue explained below.
https://github.com/Expensify/expensify-common/blob/d11466167bc562447d2dfdad624145040153efae/lib/ExpensiMark.ts#L624

Just a note that if we require space for each >, I already write an explanation about that in the root cause section.

[Skalakid]

Hello, today I continued fixing HTML to markdown parsing logic. Tomorrow I will clean up the code and push the changes to the PR so we can start merging it

[melvin-bot]

@cead22, @twisterdotcom, @fedirjh, @Skalakid Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[Skalakid]

Hello, I'm adding last touches to my PR with the solution for this issue. Will open it for the review today

[melvin-bot]

@cead22, @twisterdotcom, @fedirjh, @Skalakid Eep! 4 days overdue now. Issues have feelings too...

[Skalakid]

PR is waiting for the C+ review

[melvin-bot]

@cead22, @twisterdotcom, @fedirjh, @Skalakid Whoops! This issue is 2 days overdue. Let's get this updated quick!

[twisterdotcom]

This was merged, so it is waiting on a deploy now?

[melvin-bot]

@cead22, @twisterdotcom, @fedirjh, @Skalakid 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[Skalakid]

The PR has been merged. We will bump the Expensify-common together with the next Live Markdown bump since some new breaking changes were introduced there

[melvin-bot]

@cead22, @twisterdotcom, @fedirjh, @Skalakid Still overdue 6 days?! Let's take care of this!

[Skalakid]

Waiting for this PR to be merged, since it bumps the Live Markdown and expensify-common libraries

[melvin-bot]

@cead22, @twisterdotcom, @fedirjh, @Skalakid 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[twisterdotcom]

Would be nice to get an update on this one. Is it likely be getting daily updates, or should we make it weekly for now if we're beholden to some other deployment schedules?

[Skalakid]

The solution is ready; we are only waiting to bump the libraries in the app. As I wrote above today, this will be done by this PR. For now, we can make it weekly, but the changes should be merged this week if nothing critical appear

[fedirjh]

@Skalakid Do the changes from this PR (Expensify/expensify-common#820) be shipped with that PR #53627 ?

[Skalakid]

@fedirjh yes, they bumped the expensify-common to the version that contains my changes

[melvin-bot]

This issue has not been updated in over 15 days. @cead22, @twisterdotcom, @fedirjh, @Skalakid 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!

[fedirjh]

This seems to be fixed. I think this can be closed.

[twisterdotcom]

Wait, with no payment?