Expensify
[$250] Chat - After editing nested quote message, the markdown is removed
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: 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): [email protected] 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
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
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.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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
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
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.
@twisterdotcom Whoops! This issue is 2 days overdue. Let's get this updated quick!
@twisterdotcom Eep! 4 days overdue now. Issues have feelings too...
Agree, this one was simple to reproduce. Apologies for the time, I am just back from OOO.
Job added to Upwork: https://www.upwork.com/jobs/~01a7ea8bf9646df697
Triggered auto assignment to Contributor-plus team member for initial proposal review - @fedirjh (External)
@twisterdotcom, @fedirjh Whoops! This issue is 2 days overdue. Let's get this updated quick!
@bernhardoj's Proposal looks good to me. Let's use the regex as it covers different variations of nested quotes with spaces.
🎀 👀 🎀 C+ reviewed
Triggered auto assignment to @cead22, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@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 .
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.
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
@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!
@cead22, @twisterdotcom, @fedirjh Eep! 4 days overdue now. Issues have feelings too...
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
@cead22, @twisterdotcom, @fedirjh Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
cc @BrtqKr Friendly bump for https://github.com/Expensify/App/issues/47951#issuecomment-2339349412
@cead22, @twisterdotcom, @fedirjh Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@cead22 @twisterdotcom @fedirjh this issue is now 4 weeks old, please consider:
- Finding a contributor to fix the bug
- Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
- If you have any questions, don't hesitate to start a discussion in #expensify-open-source
Thanks!
Same same, and I'm going OOO tomorrow, in case you need to find another engineer @twisterdotcom
cc @BrtqKr Friendly bump for https://github.com/Expensify/App/issues/47951#issuecomment-2339349412
Bumped in Slack: https://expensify.slack.com/archives/C06BDSWLDPB/p1727971180783039