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

lanitochka17 commented 3 weeks ago

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:

Go to https://staging.new.expensify.com/
Log in
Go to any chat
Send a nested quote message, like "> > test"
Verify that it looks as expected on the chat
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
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/3a9ad135-4bdb-4575-9683-e8f2a9a03643

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[bot] commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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

bernhardoj commented 2 weeks ago

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], '&gt; ') || Str.startsWith(textSplitted[i], '&gt;&gt;') || isLastBlockquote) && !insideCodefence) {

This will convert it to markdown for below inputs:

> > test
>> test

ChavdaSachin commented 2 weeks ago

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(/^( *(&gt; )+)+ /gm)  || isLastBlockquote) && !insideCodefence) {

What alternative solutions did you explore? (Optional)

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[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

twisterdotcom commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

ChavdaSachin commented 2 weeks ago

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

melvin-bot[bot] commented 1 week ago

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

twisterdotcom commented 1 week ago

Bump @fedirjh on these proposals

fedirjh commented 1 week ago

@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[bot] commented 1 week ago

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

ChavdaSachin commented 1 week ago

@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 https://github.com/Expensify/App/issues/45154#issuecomment-2222162593 .

bernhardoj commented 1 week ago

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.

ChavdaSachin commented 1 week ago

Proposal

updated @fedirjh would you please take a look at my updated proposal. since we are missing out on multiline quote problem without completely modifying HTMLToMarkdown rule for 'quote' as I suggested

https://github.com/user-attachments/assets/32296991-dc32-484d-adee-166947d9e1e0

melvin-bot[bot] commented 1 week ago

@cead22 @twisterdotcom @fedirjh 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!

twisterdotcom commented 4 days ago

Bump on this @fedirjh.

melvin-bot[bot] commented 4 days ago

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

fedirjh commented 4 days ago

It looks like two blockquotes must be separated by space is recommended design feature. So, we might need a design review on this one

Good catch @ChavdaSachin, but it seems like the other issue is stale for some time now.

cc @BrtqKr, what do you think about allowing nested quotes with spaces since we do that in the composer live Markdown ?

https://github.com/user-attachments/assets/133bf7b8-6988-443c-83f2-10aadd3d33ed

melvin-bot[bot] commented 9 hours ago

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

Expensify / App