[HOLD for payment 2023-03-01] [$1000] Trailing line break is not working inside three backticks messages on the web

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

---

Action Performed:

1. Open app & navigate to any chat
2. Send a message that starts and ends with three backticks and includes a line break before closing 3 backticks

Expected Result:

line break should reflect in the message

Actual Result:

line break is being ignored and there is no space at the end of the message ends.

NOTE

What appears to be happening is that one less line break is shows than added so...
1 line break added doesn't show
2 line breaks added shows 1
and so on, so the deliverable is to fix all instances of "one less line break added" and not just the instance of a single line break not showing

Workaround:

unknown

Platforms:

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

• MacOS / Chrome / Safari

Version Number: 1.2.62-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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Recording.1412.mp4

Expensify/Expensify Issue URL:
Issue reported by: @jayeshmangwani
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1675110851241419

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01888fcd2426613ce9
• Upwork Job ID: 1620562001590956032
• Last Price Increase: 2023-01-31

[melvin-bot]

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[mallenexpensify]

Tested, can confirm it's happening. I updated the OP

NOTE

What appears to be happening is that one less line break is shows than added so...
1 line break added doesn't show
2 line breaks added shows 1
and so on, so the deliverable is to fix all instances of "one less line break added" and not just the instance of a single line break not showing

@jayeshmangwani you'll still be compensated as the bug reporter if we fix this, just wanted to make sure all instances were addressed.

Moving to engineer to confirm this can be External, 90% sure it can...

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01888fcd2426613ce9

[melvin-bot]

Current assignee @mallenexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

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

[eh2077]

Proposal

RCA
Code fence

test

converts to html

<pre><br />test<br /></pre>

But html will not display the last newline represented by the last <br /> tag.

Solution
Convert code fence

test

to html

<pre><br />test<br /><br /></pre>

which will display necessary new trailing line breaks.

And also remove a trailing new line when translating html to markdown, which will avoid an extra trailing new line shown when editing a sent message.

If you want to verify the solution, you can apply the following diff to expensify common module

diff --git a/lib/ExpensiMark.js b/lib/ExpensiMark.js
index 7f94ffe..b6cb064 100644
--- a/lib/ExpensiMark.js
+++ b/lib/ExpensiMark.js
@@ -31,7 +31,7 @@ export default class ExpensiMark {
                 //   will create styling issues so use  
                 replacement: (match, __, textWithinFences) => {
                     const group = textWithinFences.replace(/(?:(?![\n\r])\s)/g, ' ');
-                    return `<pre>${group}</pre>`;
+                    return `<pre>${group}<br /></pre>`;
                 },
             },
 
@@ -268,7 +268,7 @@ export default class ExpensiMark {
             {
                 name: 'codeFence',
                 regex: /<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
-                replacement: '```\n$2\n```',
+                replacement: '```\n$2```',
             },
             {
                 name: 'anchor',

[marktoman]

Proposal

Problem

The regular expression that matches the code fence block consumes the last new line.

Solution

First rule

Fix the expression to match the newline the same way it does currently, but without consuming it.

Replace:
https://github.com/Expensify/expensify-common/blob/aaf49708420187a13ebe3bdb97b67104e6a6bcfc/lib/ExpensiMark.js#L24

With:

+ regex: /(```[\n]?)((?:\s*?(?![\n]?```(?!`))[\S])+\s*?)((?=\n?)```)/g,

The difference is replacing \n? with (?=\n?)

---

Second rule

When editing a comment with a code block, the second codeFence rule always adds a new line. The motivation is to ensure that the ending triple tick occupies a separate line.

The other proposal changes it to not add a new line because it assumes that the comment was added by the first rule, which has been changed to always add a new line in that proposal.

I believe that neither is a reliable solution. Each rule should work independently of the other and not always add lines with no relation to the existing lines in the block.

In the case of the first rule, it means not changing the existing lines. In the case of the second rule, it means to add a new line only if the ending triple tick would not occupy a separate line.

Replace
https://github.com/Expensify/expensify-common/blob/70d6f3526f04f8494bc9ef50f0fff75ba98272c8/lib/ExpensiMark.js#L270
with

+ regex: /<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)(\n?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,

, which adds (\n?),

and the line below it with

+ replacement: (match, g1, g2, g3) =>  '```\n' + g2 + (g3 || '\n') + '```',

, which adds (g3 || '\n').

[mananjadhav]

@marktoman's proposal looks cleaner and simple to implement. I think we should go with that.

@bondydaa what do you think?

[bondydaa]

Yeah I think we'd prefer to fix this with the regex. that one is a doosy

so I would also suggest making sure we update the tests https://github.com/Expensify/expensify-common/blob/aaf49708420187a13ebe3bdb97b67104e6a6bcfc/__tests__/ExpensiMark-Markdown-test.js to also verify the extra line(s) being persevered.

[melvin-bot]

📣 @marktoman You have been assigned to this job by @bondydaa!
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 📖

[eh2077]

Hi @mananjadhav and @bondydaa

I think my proposal will be the better solution because it avoids breaking following regression cases

1. Send following code fence and edit the sent message. There will be no extra newline in the editing markdown text

```
test
```

2. Send following code fence and edit the send message. There will be only one trailing new line in the editing markdown text.

```
test

```

I also recorded a video to show it

codefence-regression.mp4

As the video shows, my proposal will ensure the line break of both the html and markdown are correct.

But @marktoman 's proposal will bring regression for markdown line break issue.

Really hope you would take the above into consideration.

Thanks for the reviewing!

[marktoman]

@eh2077 That is another unrelated bug, which is when you edit a code block, it adds a new line. Your solution strips every last line whether it should be there or not, so while it appears to fix both problems, in reality it introduces regression for every case where the last line is supposed to be there.

[marktoman]

I've added a solution for the other bug.

[eh2077]

@eh2077 That is another unrelated bug, which is when you edit a code block, it adds a new line. Your solution strips every last line whether it should be there or not, so while it appears to fix both problems, in reality it introduces regression for every case where the last line is supposed to be there.

@marktoman

The extra line break <br /> I add will ensure to translate markdown with right trailing line break from html. So my proposal will not bring the regression you mentioned.

I'd like to compare the differences of our proposal through following cases based on the latest updated version of your proposal.

Case 1

Currently by sending a new message, below code fence

```
test

```

is translated to

<pre><br />test<br /></pre>

and both our solution will translate the markdown into

<pre><br />test<br /><br /></pre>

Note the html will be saved by the backend and for frontend UI rendering. So both of our proposals will save new structure data to backend for new messages.

Case 2

Currently if you edit a sent message, there'll be an additional trailing line break in the markdown. As you pointed out, this is an another unrelated bug. Both of our proposals will fix it for new saved messages and historical messages.

Case 3

Currently if you edit a sent message and save changes(no matter has changes or not), no extra trailing line break will be added to html to be saved. Based on your latest updated, both of our proposals will not break it.

Case 4

For historical saved code fence messages, both of our proposal can't get enough information to fix the potential previous missing trailing line break. If you want to test it, you can send the following two code fence block before applying the patch

```
original code fence with two trailing line breaks, which will show one trailing line break


```

```
original code fence with three trailing line breaks, which will show two trailing line breaks



```

Conclusion

Though both of the proposals fix the issue, my proposal has the following advantages

1. Simple. It's easy to understand and doesn’t add extra complexity to the magic regex expression
2. More efficient. It'll be more efficient without adding extra complex regex matching

cc @mananjadhav @bondydaa

[mallenexpensify]

@marktoman @jayeshmangwani @mananjadhav can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01888fcd2426613ce9

[mananjadhav]

Accepted @mallenexpensify

[mananjadhav]

@bondydaa I was looking at the original PR that caused this issue and it looks like the first newline handling was done in Expensify/expensify-common@36531c0 (PR Expensify/expensify-common#376). But I wouldn't mark it is a regression PR because the regex did what was expected as per the GH issue.

Let me know what you think.

[bondydaa]

👍 yeah I agree with that assessment.

[marktoman]

I left an Upwork-specific question on the contract proposal @mallenexpensify

[mallenexpensify]

Replied there @marktoman

[marktoman]

Accepted @mallenexpensify

[mallenexpensify]

Thanks for the test steps @marktoman

@mananjadhav do you have read-only access to TestRail yet? If so, can you access this - https://expensify.testrail.io/index.php?/cases/view/1971213 ?
If so, do you agree with the proposed addition below?

1. Update title from "Markup" to "Markdown"
2. Add new section "Code Block"
3. Add a new comment:

```
text

```

4. Verify that that the rendered box has an empty line below "text".
5. Edit the comment and verify that the text is the same as 1.
6. Repeat 1-3 for the below, but instead, make sure that it has no added line below "text":

```
text
```

[mallenexpensify]

@marktoman it appears you were hired on 2/1 and the PR was merged on 2/20 which is 13 biz days. We have

Merged PR more than 9 business days after assignment - 50% penalty

Before I subtract 50% from the pay for you and @mananjadhav , do either of you have reasoning to provide why it shouldn't be deducted? Thanks

[marktoman]

@mallenexpensify There were two PRs: 1) to expensify-common, 2) to App. The first one merged quickly and covered everything discussed here. On the second PR, I found a separate issue. I would expect the second problem to occur as a new issue at some point if I would not solve it under this one.

[mananjadhav]

@mallenexpensify As @marktoman mentioned, there were two PRs and here's link of the problem they reported in rendering, which took sometime to resolve. Plus I was out sick during that period that I wasn't helpful in resolve it faster.

[mallenexpensify]

Thanks for the context. @mananjadhav what do you propose the payment breakdown should be here?

[mananjadhav]

Quick question @mallenexpensify, if we would've put the issue on Hold when we found the issue, would it still impact the payment here? If the answer is that it wouldn't have impacted, then would be great if we can make an exception here and do the 1000$ payout. If the answer is, it would still impact during the hold period, then I think it's fair to have the 50% deduction penalty.

[mallenexpensify]

Thanks @mananjadhav , this appears to be an edge case because of the non-E/App repo and because the PR was put on hold which slowed things down. I propose we do the full $1000 payout for @marktoman and @mananjadhav for C+. @bondydaa and/or @mananjadhav can you add a comment to confirm you're OK with this then I'll issue payments.

[mananjadhav]

I am definitely okay ;) Should we wait for @bondydaa to confirm?

Ohh and I recently asked how do we want to treat two PRs, I think @mallenexpensify you should look at the response. (which might warrant for a deduction)

Response is:

Some of the issues require updating the fork/expensify-common/upstream fix and then creating the App PR

The efficiency bonus is for completing an issue, and for repos in our control (like forks we maintain, expensify-common etc), that’s irrespective of however many PRs it takes to complete the issue.

[MelvinBot]

@mananjadhav, @bondydaa, @mallenexpensify, @marktoman Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[MelvinBot]

@mananjadhav, @bondydaa, @mallenexpensify, @marktoman Whoops! This issue is 2 days overdue. Let's get this updated quick!

[mallenexpensify]

Thanks @mananjadhav , I didn't piece 2-2 together when I saw that Slack post last week. I get the reasoning behind 'repos we control', I'm unsure we make it clear in CONTRIBUTING.md or other guides how we manage multiple PRs (nor.. if we have to, at least right now). The one part I'm unsure on is that the two PRs weren't able to be worked on in tandem, correct? If that's so, then the contributor shouldn't be penalized while they're waiting for one PR to be merged or hit production because they're able to post the next PR. @bondydaa can you take a peek when you're back on tomorrow?

[mananjadhav]

I think the difference here is we fixed the reported issue, and also a probable future issue and that held the PR. The more we discuss, I feel we shouldn't penalize for solving it right. Still waiting for @bondydaa to chime in on this one.

[mananjadhav]

@mallenexpensify @bondydaa do we have any update here? Been pending since long.

[bondydaa]

Sorry was OOO most of the past 2 weeks.

Yep I don't think we should reduce the pay here, any delays here were due to the 2 PRs or people being OOO so not really in the control of those involved so I think $1000 makes sense.

[mallenexpensify]

Thanks @bondydaa and everyone else for the patience

@jayeshmangwani paid $250 for reporting
@mananjadhav paid $1000 for C+
@marktoman paid $1000 for contributor

📕

[mananjadhav]

Thanks @mallenexpensify and @bondydaa

[marktoman]

@mallenexpensify

The one part I'm unsure on is that the two PRs weren't able to be worked on in tandem, correct? If that's so, then the contributor shouldn't be penalized while they're waiting for one PR to be merged or hit production because they're able to post the next PR.

Correct, they cannot. I tried to parallelize them on another issue but had to wait. (Everything clicked there, including C+ staying up till 4 am to match the opposite timezone, the internal engineers being immediately responsive, and me having a commit and everything uploaded before being assigned, so no decrease there, but only just.)