MelvinBot
commented
1 year ago Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Closed kavimuru closed 1 year ago
MelvinBot
commented
1 year ago Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
MelvinBot
commented
1 year ago Platforms in OP are ✅)
adelekennedy
commented
1 year ago I haven't been able to find a dupe for this one (though my searching might be failing me) and still reproducible
MelvinBot
commented
1 year ago Job added to Upwork: https://www.upwork.com/jobs/~01a0e20796efbb701d
MelvinBot
commented
1 year ago Current assignee @adelekennedy is eligible for the External assigner, not assigning anyone new.
MelvinBot
commented
1 year ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External)
MelvinBot
commented
1 year ago Triggered auto assignment to @deetergp (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
ntdiary
commented
1 year ago ```
# test
```Send a code block message like the above one, then edit the message and add 2 line break before # and save changes. this message will be hidden.
When we update with the same content as the original message, the backend will push an empty html data, it means that this message is deleted and will be hidden.
The AddComment data:
The UpdateComment data:
when we save changes , the above condition is false, so the code isn't interrupted, and then backend will push a empty html data to frontend. (parsedOriginalCommentHTML is <pre># test<br /><br /></pre>, and htmlForNewComment.trim() is <pre><br /># test<br /><br /></pre>)
When making comparisons, we should have consistent conversion rules.
I'm not sure why the changes in @marktoman 's commit are different from his proposal, maybe I'm missing something?
IMO, his original proposal is fine: (convert <pre> to ```\n)
ExpensiMark.js:
{
name: 'codeFence',
regex: /<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)(\n?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
replacement: (match, g1, g2, g3) => `\`\`\`\n${g2}${g3 || '\n'}\`\`\``,
},related comment and pr: https://github.com/Expensify/App/issues/14694#issuecomment-1411287892 https://github.com/Expensify/expensify-common/pull/496/files
I'm not sure if the backend acts for any other purpose, maybe we could push the same content instead of empty data if the comment remains the same?
The problem of pushing empty message may be treated as a separate bug.
eh2077
commented
1 year ago Editing a code block message by adding 2 line break in the message will hide the message after saving.
When deciding to save an edited message, we do markdown-to-html, html-to-markdown and markdown-to-html conversion to check if need to save the edited message. In this case, this branch condition is false while htmlForNewComment is same with the previous backend saved value and then the backend will return empty data resulting in hiding the message from frontend.
To fix this issue, we should ensure code fence markdown-to-html conversion is equal to markdown-to-html => html-to-markdown => markdown-to-html conversion. We can improve html-to-markdown conversion rule for code fence from
https://github.com/Expensify/expensify-common/blob/a2942d035614bf4bed90ffe2114d6ee828ee96da/lib/ExpensiMark.js#L274
replacement: (match, g1, g2, g3, g4) => `\`\`\`${g2 || '\n'}${g3}${g4 || '\n'}\`\`\``,to
replacement: (match, g1, g2, g3, g4) => `\`\`\`${g2 ? '\n\n' : '\n'}${g3}${g4 || '\n'}\`\`\``,NA
aimane-chnaif
commented
1 year ago I think issue description needs updated a bit. This happens on all text (not only header text) inside code block.
The weird thing in this video is that when refresh Report page (either go to another tab or to switch chat and come back), disappeared message shows again.
MelvinBot
commented
1 year ago @deetergp, @adelekennedy, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
deetergp
commented
1 year ago I have not been able to reproduce the disappearing behavior in either staging, nor production. The number of line breaks I put before the text does not stay consistent, but the code block never ceases to be visible. Can I get a re-test from someone else?
aimane-chnaif
commented
1 year ago @deetergp reproducible step needs update a bit more clear. At first, I also couldn't reproduce following step guided in GH description but clearly reproducible following the example in slack convo
deetergp
commented
1 year ago I don't know why it is acting differently today, since what I did yesterday is essentially what you just linked @aimane-chnaif, but today it's behaving the way described here in this GH. In any event…
I'm not sure if the backend acts for any other purpose, maybe we could push the same content instead of empty data if the comment remains the same?
I think that's probably true, @ntdiary, since simply returning early here blanks out the content of the comment, which is not the desired outcome, but I think that is also a separate bug.
when we save changes , the above condition is
false, so the code isn't interrupted, and then backend will push a empty html data to frontend. (parsedOriginalCommentHTMLis<pre># test<br /><br /></pre>, andhtmlForNewComment.trim()is<pre><br /># test<br /><br /></pre>)
But isn't the issue here that htmlForNewComment.trim() ought to be <pre><br /><br /># test<br /><br /></pre> if we are indeed intending on rendering what the client intends to display? And in this instance, shouldn't parsedOriginalCommentHTML be <pre><br /># test<br /><br /></pre>?
adelekennedy
commented
1 year ago still reproducible
ntdiary
commented
1 year ago But isn't the issue here that
htmlForNewComment.trim()ought to be<pre><br /><br /># test<br /><br /></pre>if we are indeed intending on rendering what the client intends to display? And in this instance, shouldn't parsedOriginalCommentHTML be<pre><br /># test<br /><br /></pre>?
@deetergp, yeah, I agree with you, these are the expected and correct values. I meant to say that in the current implementation, our frontend code doesn't find that ```\n\n\n# test\n\n``` will be converted to the same html value as the last message (correct value: <pre><br /># test<br /><br /></pre>).
I think that's probably true, @ntdiary, since simply returning early here blanks out the content of the comment, which is not the desired outcome, but I think that is also a separate bug.
Ideally, if the frontend finds that the edited content is the same as the previous one, it won't send an update request.
But now the values on both sides of the equal sign are clearly wrong.
parsedOriginalCommentHTML currently is <pre># test<br /><br /></pre>, but it should be <pre><br /># test<br /><br /></pre>.
htmlForNewComment.trim() currently is <pre><br /># test<br /><br /></pre>, but it should be <pre><br /><br /># test<br /><br /></pre>.
IMO, the frontend and backend are related. If we only fix this issue on the frontend, the problem of pushing empty message won't be reproducible. It will reappear in the future along with some conversion problem. So I think the frontend issue certainly needs to be addressed, and it would be better if the backend issue could be addressed together. 😄
MelvinBot
commented
1 year ago @deetergp @adelekennedy @aimane-chnaif 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!
MelvinBot
commented
1 year ago @deetergp @adelekennedy @aimane-chnaif 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!
deetergp
commented
1 year ago @ntdiary I am not sure I agree that there is a backend component to this, I think the issue is that we don't ever end up calling the backend when we edit the message.
After testing @eh2077's proposal, I can verify that it fixes the issue. When I type the following:
This is what it looks like in the DB:
And when I edit to add the extra line break:
This is what it ends up looking like in the database, which is the expected result:
Unless @aimane-chnaif objects, I think we should go with @eh2077 proposal.
ntdiary
commented
1 year ago @deetergp, I'm not against fixing this problem on the frontend, I'm just saying that the changes on the backend are an optimization and it's optional. 😅 Have you tried this pattern?
replacement: (match, g1, g2, g3) => `\`\`\`\n${g2}${g3 || '\n'}\`\`\``I think what we need is to convert <pre> to ```\n，or maybe I'm missing something?
deetergp
commented
1 year ago @ntdiary I am still unclear on why you think there is a backend component to this. Additionally, your pattern eliminates the extra line break after the text when you edit the message again, which is not the case with the pattern @eh2077 is proposing.
ntdiary
commented
1 year ago @deetergp, eh, maybe I didn't express myself clearly, editing messages has nothing to do with the backend. I meant fixing the problem of pushing empty message is an optimization. You think it's a separate bug, and I think it probably won't reproduce alone after fixing the pattern problem.
Could you please share your case? I'm not sure where the line break was eliminated.
https://user-images.githubusercontent.com/8579651/223737905-fe2142c1-a23c-42f1-adf5-1b81a6ca4bed.mp4
Eh, @deetergp, sorry, here is the rule:
{
name: 'codeFence',
regex: /<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)(\n?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
replacement: (match, g1, g2, g3) => `\`\`\`\n${g2}${g3 || '\n'}\`\`\``,
},Ahh! With the updated regex, your replacements work quite well @ntdiary. If you would amend your proposal, I'm inclined to go with it unless @aimane-chnaif disagrees.
ntdiary
commented
1 year ago @deetergp thank you very much, have updated my proposal.🙂
aimane-chnaif
commented
1 year ago I am checking & testing various cases now. Will update soon
eh2077
commented
1 year ago Thank C+ team for reviewing proposals!
I think this is an interesting case about proposal competition. I proposed a workable solution earlier before @ntdiary while later @ntdiary posted a clearer but, to some degree, similar solution. I tend to believe my solution could also get improved to the same clean result during PR preparation or PR review process.
@aimane-chnaif and @deetergp Would you mind sharing your thoughts on this?
situchan
commented
1 year ago test
test
While converting html to markdown, 1 line break is removed. -> this is the main root cause
So when edit message with 2 line breaks, it's actually converted to 1 line break which becomes same as original message.
https://github.com/Expensify/App/blob/b08bf4883d9fdf0624dcd86798933743f4b31161/src/libs/actions/Report.js#L826
Frontend triggers comparison between old and new html and treat this as different message because both messages are updated to 1 line break removed ones (old message: no line break, new message: 1 line break)
And trigger UpdateComment api with 1 line removed message which is same as old one.
Backend treats this as same message and send empty html which lead to deletion in frontend.
While converting html to markdown, 1 line break is removed
Then why this happens?
For the above example case:
g2 = \n
g3 = test
g4 = \n
https://github.com/Expensify/expensify-common/blob/714aac9b66b7731e0dc96a7956f12684635c6c41/lib/ExpensiMark.js#L267-L271
{
name: 'codeFence',
regex: /<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>(\n?)([\s\S]*?)(\n?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
replacement: (match, g1, g2, g3, g4) => `\`\`\`${g2 || '\n'}${g3}${g4 || '\n'}\`\`\``,
},After this replacement, html becomes '''\ntest\n'''. The expected result should be '''\n\ntest\n'''
So the issue came from this wrong replacement logic.
Then why is this considered wrong replacement?
The original issue is when convert markdown to html.
i.e.
In above example, converted html is <pre><br />test<br /></pre>.
The correct html should be <pre><br /><br />test<br /></pre>
So in another word, <pre> includes <br/> while </pre> doesn't include <br/>
NOTE: This should be workaround since we keep wrong logic of converting markdown to html.
We should correct replacement logic in codeFence when convert html to markdown
regex: /<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>(\n?)([\s\S]*?)(\n?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
- replacement: (match, g1, g2, g3, g4) => `\`\`\`${g2 || '\n'}${g3}${g4 || '\n'}\`\`\``,
+ replacement: (match, g1, g2, g3, g4) => `\`\`\`${g2}\n${g3}\n\`\`\``,
Code explanation:
Inside code block, line break should always be added at the beginning and at the end.
g2 || '\n' was wrong. 1 line break is always removed in g3 (because <pre> includes <br/>) so we should use ${g2}\n (or \n${g2}).
g4 || '\n' is redundant because g4 is always \n or empty. We can simply use \n.
Addon: We can do optimization in this solution
prefix (\n?) equals to g2 and we can remove this in both sides.
Final regex and replacement:
regex: /<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)(\n?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
replacement: (match, g1, g2) => `\`\`\`\n${g2}\n\`\`\``,Fix logic of converting markdown to html.
So above example should be correctly converted to <pre><br /><br />test<br /></pre> so that it could be aligned with user input.
https://github.com/Expensify/expensify-common/blob/714aac9b66b7731e0dc96a7956f12684635c6c41/lib/ExpensiMark.js#L20-L36
{
name: 'codeFence',
// < is a backtick symbol we are matching on three of them before then after a new line character
regex: /(```[\n]?)((?:\s*?(?![\n]?```(?!`))[\S])+\s*?)((?=\n?)```)/g,
// We're using a function here to perform an additional replace on the content
// inside the backticks because Android is not able to use <pre> tags and does
// not respect whitespace characters at all like HTML does. We do not want to mess
// with the new lines here since they need to be converted into <br>. And we don't
// want to do this anywhere else since that would break HTML.
// will create styling issues so use  
replacement: (match, __, textWithinFences) => {
const group = textWithinFences.replace(/(?:(?![\n\r])\s)/g, ' ');
return `<pre>${group}</pre>`;
},
},The solution is to
<pre>${group}</pre>;<pre>${g1}${group}</pre>;
Code explanation:
First line break is always removed from g2 (textWithinFences) so we should add it. But no need if no line break exists (i.e. test'''). So it's better to add below logic 👇 rather than always adding \n to match original markdown.
We currently don't know if line break exists at the start. By using (), we can get that from 2nd parameter (g1) and add that before g2 (textWithinFences).
The main challenge of this solution: The issue is in render.
This video shows how below markdown renders:
test
There are total 5 spans: first 2 span is line break, middle one is test, last 2 span is line break.
We should remove first 1 span in render which should already be included in <pre>.
aimane-chnaif
commented
1 year ago @ntdiary your solution causes regression - last line break is removed when edit (please check video below). This works fine on production. Also, your regex is deprecated. You are not supposed to update regex in your proposal, right?
Before:
After:
aimane-chnaif
commented
1 year ago I don't wanna accept any proposals which don't clearly explain their solution - why their updated regex or updated replacement logic fixes this issue. This is kind of code diff which is forbidden in proposal step.
ntdiary
commented
1 year ago This works fine on production. Also, your regex is deprecated. You are not supposed to update regex in your proposal, right?
@aimane-chnaif, I don't understand what you mean. Why my regex is deprecated? and I shouldn't update the regex in my proposal?
What I recommend in my proposal is the rule from this comment.
In subsequent communications, @deetergp said that the extra line break after the text is eliminated when he edited the message again with my proposal. Then I uploaded a video to show that it worked and hoped he would share his case.
Before he replied again, It occurred to me that it was probably because he had only modified the replacement and not the regex.
I thought that event though I had listed the links to that comment and PR, I should take responsibility for not make it clear directly that the regex should be reverted to the version in that comment. So I said sorry to him and provided the regex directly.
IMO, the merged PR should not eliminate the line break after the <pre>. Because both the original message (in the input box) and the html rendered from it(after sending) obviously had extra line break before the text. So I said that his original proposal is fine (The original is meant to be relative to the version in his subsequent PR, not the first version of that comment).
Personally, I think that this issue is just that the conversion rules are complicated. maybe I'm wrong : ). If you feel that the other proposals are better or more sensible, please feel free to choose them. I just hope there are no misunderstandings in our communication (I'm still learning to communicate in English, I've learned a lot in this community over the past year, and also thanks for DeepL and Google Translator).
aimane-chnaif
commented
1 year ago @ntdiary Sorry I got confused because there were no explanations about your code diff.
This is your proposal
And I thought this was same as before https://github.com/Expensify/expensify-common/pull/496
This is latest code in expensify-common repo:
Original:
/<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
After https://github.com/Expensify/expensify-common/pull/496:
/<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>(\n?)([\s\S]*?)(\n?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
Your proposal:
/<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)(\n?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
So what I got confused is that Original = Your proposal because it partially reverts https://github.com/Expensify/expensify-common/pull/496 and I thought you proposed based on old codebase before https://github.com/Expensify/expensify-common/pull/496
ntdiary
commented
1 year ago @aimane-chnaif, wow, I got it. Thank you for your patience in explaining and sorry for the vagueness in my proposal. I'll try to have a clearer description in my future proposals as well. 😄
MelvinBot
commented
1 year ago @deetergp @adelekennedy @aimane-chnaif this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!
aimane-chnaif
commented
1 year ago still testing proposals in various cases sorry for delay but need patience to avoid any regressions
aimane-chnaif
commented
1 year ago Thanks for contributions everyone.
origin: (before https://github.com/Expensify/expensify-common/pull/496)
regex: /<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
replacement: (match, g1, g2) => `\`\`\`\n${g2}\n\`\`\``,main: (after https://github.com/Expensify/expensify-common/pull/496)
regex: /<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>(\n?)([\s\S]*?)(\n?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
replacement: (match, g1, g2, g3, g4) => `\`\`\`${g2 || '\n'}${g3}${g4 || '\n'}\`\`\``,eh2077:
regex: /<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>(\n?)([\s\S]*?)(\n?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
replacement: (match, g1, g2, g3, g4) => `\`\`\`${g2 ? '\n\n' : '\n'}${g3}${g4 || '\n'}\`\`\``,ntdiary:
regex: /<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)(\n?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
replacement: (match, g1, g2, g3) => `\`\`\`\n${g2}${g3 || '\n'}\`\`\``,situchan:
regex: /<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)(\n?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
replacement: (match, g1, g2) => `\`\`\`\n${g2}\n\`\`\``,I tested and confirmed all 3 solutions are working fine and they are similar. I understand that they are a workaround which overcomes line break removed issue when convert markdown to html. I think this is intentional to render correctly as explained by @situchan in optional solution. But I don't suggest that solution because it will break all existing code block and affect live users. And fixing in render level will also be a workaround.
While @eh2077 is the first to propose working solution, I am more inclined towards @situchan's proposal because they explained the root cause in low level (why it happens) and provided optimised solution with clear explanation (especially I like explanation of g1, g2, g3, ... meanings). Others explained the root cause in high level which is more like how it happens.
@deetergp I will leave it to you to assign who.
deetergp
commented
1 year ago I appreciate everyone's input in this discussion! I agree with @aimane-chnaif that @situchan's proposal is the clearest and most detailed of the bunch and as a result am inclined to go with it.
MelvinBot
commented
1 year ago 📣 @situchan You have been assigned to this job by @deetergp! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑💻 Keep in mind: Code of Conduct | Contributing 📖
adelekennedy
commented
1 year ago Hired! Contributor: @situchan C++: @aimane-chnaif
reporting bonus will be paid to: @harshad2711
MelvinBot
commented
1 year ago Reviewing label has been removed, please complete the "BugZero Checklist".
MelvinBot
commented
1 year ago The solution for this issue has been :rocket: deployed to production :rocket: in version 1.2.88-2 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2023-03-31. :confetti_ball:
After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
MelvinBot
commented
1 year ago BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
MelvinBot
commented
1 year ago @deetergp, @adelekennedy, @aimane-chnaif, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
adelekennedy
commented
1 year ago @harshad2711 will you apply for the reporting bonus here?
aimane-chnaif
commented
1 year ago @adelekennedy this is eligible for timeline bonus. PR was merged within 5 days but 2 days were weekend
adelekennedy
commented
1 year ago @aimane-chnaif shoot - good catch, adding 50% bonus
harshad2711
commented
1 year ago @harshad2711 will you apply for the reporting bonus here?
Thanks @adelekennedy , I have applied
MelvinBot
commented
1 year ago @deetergp, @adelekennedy, @aimane-chnaif, @situchan Eep! 4 days overdue now. Issues have feelings too...
MelvinBot
commented
1 year ago @deetergp, @adelekennedy, @aimane-chnaif, @situchan Huh... This is 4 days overdue. Who can take care of this?
deetergp
commented
1 year ago Still on hold
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
# hello)Expected Result:
Message should not be hidden and it should display edited message
Actual Result:
Message will be hidden after a second
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.74-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 Notes/Photos/Videos:
https://user-images.githubusercontent.com/43996225/220152930-bd1234a7-3ea3-44a7-b6e8-124eedd74e75.mp4
https://user-images.githubusercontent.com/43996225/220152932-ed5b9a3a-c049-48cf-b9a7-4a7e560bf2dd.mp4
https://user-images.githubusercontent.com/43996225/220152944-819f42d3-0990-4d5f-ae53-c06811438308.mov
Expensify/Expensify Issue URL: Issue reported by: @harshad2711 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1676710142677019
View all open jobs on GitHub
Upwork Automation - Do Not Edit