lanitochka17 commented 3 months 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: 1.4.83-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: N/A Issue reported by: Applause - Internal Team

Action Performed:

Go to staging.new.expensify.com
Go to workspace chat
Submit an expense with code block as description
Go to transaction thread
Wait for a while
Click Description
Might need to reopen Description a few times to see console error

Expected Result:

Console error will not show up when opening description that contains code block

Actual Result:

Console error shows up when opening description that contains code block. Typing inside the code block creates more console errors

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/Expensify/App/assets/78819774/23a7b87d-e5b3-4bc9-ad94-34bd9a26570c

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~014b73c4727a3648d0
Upwork Job ID: 1802541737368946970
Last Price Increase: 2024-06-17
Automatic offers:

s77rt | Reviewer | 102859088
bernhardoj | Contributor | 102859089

Issue Owner

Current Issue Owner: @kadiealexander

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @kadiealexander (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 months ago

@kadiealexander 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 months ago

We think that this bug might be related to #wave-collect - Release 1

bernhardoj commented 3 months ago

Proposal

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

Console error when open a money request description page that contains code block/fence.

What is the root cause of that problem?

The code block that we receive from the BE contains a \r\n as the newline. In live markdown, we will parse the markdown into tokens and tree and then parsed the tree back to the markdown to compare whether the original markdown equals to the processed markdown. https://github.com/Expensify/react-native-live-markdown/blob/2ada06efda92c81f07044afc1467e77075206059/parser/index.ts#L242-L246

If it's different, then we get the error. As shown in the video, if we have the markdown as

```\r\nHello\r\n```

the processed input will be

```\nHello\r\n```

Why the first \r is gone? Let's first parse the markdown to tree based on the example above.

{
    tag: <>
    children: [{
        tag: "<pre>",
        children: ['Hello\r\n'],
    }],
}

Notice that the children already lost the first \r\n. When we parse the tree back to text, if it's a pre tag, the text is constructed as below:

``` + \n{children} + ```

https://github.com/Expensify/react-native-live-markdown/blob/2ada06efda92c81f07044afc1467e77075206059/parser/index.ts#L156-L160

We add the \n manually to replace the lost \r\n. However, as \r\n is not the same as \n, we got the error. Now, why the first \r\n is lost?

It's because of a markdown to HTML bug(?) in ExpensiMark.

```\r\nHello\r\n```

is parsed to

<pre>Hello\r\n</pre>

The current regex for code fence is /(```(?:\r\n|\n))((?:\s*?(?!(?:\r\n|\n)?```(?!`))[\S])+\s*?(?:\r\n|\n))(```)/g

If matched, the parser will convert it to

<pre>{group}</pre>

group is the second group matched from the regex. The first group is (```(?:\r\n|\n)) and the second group is ((?:\s*?(?!(?:\r\n|\n)?```(?!`))[\S])+\s*?(?:\r\n|\n))

Notice that the first group matches the first line break, that's why the first line break is lost.

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

The simplest and safest way to solve this is to replace every kind of line break (CONST.REGEX.LINE_BREAK) with \n on the description page. https://github.com/Expensify/App/blob/ab7060c959232f8efa45365c5dcbdcb689c59e1b/src/pages/iou/request/step/IOURequestStepDescription.tsx#L168

But if we want to fix the root cause, then we need to:

Update the codefence regex to include the first line break to the second group. updated regex: /(```)((?:\r\n|\n)(?:\s*?(?!(?:\r\n|\n)?```(?!`))[\S])+\s*?(?:\r\n|\n))(```)/g Then, remove the adding the \n manually in live markdown parser. https://github.com/Expensify/react-native-live-markdown/blob/2ada06efda92c81f07044afc1467e77075206059/parser/index.ts#L159

-addChildrenWithStyle(`\n${content}`, 'pre');
+addChildrenWithStyle(content, 'pre');

What alternative solutions did you explore? (Optional)

We can apply the fix in react-native-live-markdown. In parseExpensiMarkToRanges, replace all kinds of newlines to \n of the markdown input. https://github.com/Expensify/react-native-live-markdown/blob/aa5cbefa5e248159f81393b7a2dd8adfcc807e1c/parser/index.ts#L240-L242

const cleanedMarkdown = markdown.replace(/\r\n|\r/g, '\n');

Then replace all usages of markdown in parseExpensiMarkToRanges with cleanedMarkdown

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~014b73c4727a3648d0

melvin-bot[bot] commented 3 months ago

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

nurhesen commented 3 months ago

Contributor details Your Expensify account email: nurhesen@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01769ca3bf311ef116?s=1110580755107926016

Hi. I have solved the problem by converting \r\n to unicode. Just one line of code needed to add

https://github.com/Expensify/App/assets/33550425/a6f6b018-6cfa-4d93-9446-e98a588fb4ad

melvin-bot[bot] commented 3 months ago

📣 @nurhesen! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

nurhesen commented 3 months ago

Contributor details Your Expensify account email: nurhesen@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01769ca3bf311ef116?s=1110580755107926016

Hi. I have solved the problem by converting \r\n to unicode. Just one line of code needed to add

https://github.com/Expensify/App/assets/33550425/0494b4b3-d9b7-4b51-a31b-5063841ccc44

melvin-bot[bot] commented 3 months ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

nurhesen commented 3 months ago

Contributor details Your Expensify account email: nurhesen@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01769ca3bf311ef116

Hi. I have solved the problem by converting \r\n to unicode. Just one line of code needed to add

https://github.com/Expensify/App/assets/33550425/806a8224-3ce6-4d14-b705-00c8e397a069

melvin-bot[bot] commented 3 months ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

nurhesen commented 3 months ago

Contributor details Your Expensify account email: nurhesen@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01769ca3bf311ef116

melvin-bot[bot] commented 3 months ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

nurhesen commented 3 months ago

Proposal

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

Console error shows up when opening description that contains code block

What is the root cause of that problem?

react-native-live-markdown doesn't process \r\n correctly

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

Converting \r\n to unicode fixes the problem. Just adding one line of code needed. Here is the video proof that converting \r\n to unicode solves the problem.

https://github.com/Expensify/App/assets/33550425/128df4f4-dd20-4ea9-8a41-d95346ba3529

What alternative solutions did you explore? (Optional)

Alternatively single "\n"s can be converted in the view to "\r\n" in order to keep the consistency

s77rt commented 3 months ago

@bernhardoj Thanks for the proposal. Your RCA makes sense. But the first newline being consumed is actually expected because the syntax for the code block must have a new line:

```Hello```

This is not a valid code block

```\r\nHello\r\n```

This is a valid code block and the content is Hello

In the returned children: ['Hello\r\n'], I would expect to see only Hello (the second newline should be consumed too but that's not our bug here).

Can we just replace all \r\n of the input with \n but at react-native-live-markdown level?

s77rt commented 3 months ago

@nurhesen Thanks for the proposal. Your RCA is not clear. Can you please elaborate?

bernhardoj commented 3 months ago

Ah, I see. I remember

```Hello```

is a valid code block. I didn't realize it's changed now. (or maybe that's always the case?)

Updated my proposal to replace the new line in react-native-live-markdown.

BartoszGrajdek commented 3 months ago

Hi! I'm Bartosz from SWM react-native-live-markdown team. There were some changes in the process of handling live markdown related bugs, so I'll be helping you with the proposal review to ensure high quality solutions & avoid any regressions in the react-native-live-markdown lib 🙌🏻

I'll try to review existing proposals / triage the bug later today 👀

s77rt commented 3 months ago

@bernhardoj Thanks for the update! That looks good to me.

🎀 👀 🎀 C+ reviewed Link to proposal

cc @BartoszGrajdek

melvin-bot[bot] commented 3 months ago

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

BartoszGrajdek commented 3 months ago

I've looked through @bernhardoj proposal and although it would solve the problem, our NR. 1 rule in react-native-live-markdown is that we shouldn't manipulate the content provided by user in any way (the output stripped of any tags should be the same as the input). If the user has added \r\n himself we should respect that.

We've been handling \r\n properly up until now, so why can't we make codeBlocks/codeFence handle it as well? I believe that it's possible to fix it in expensify-common ExpensiMark.js (here) and later we would remove \n from react-native-live-markdown parser here (just like @bernhardoj has noticed). We should be able to achieve it without causing any regressions and it would also be the cleanest fix.

We can find someone from SWM with the capacity to work on this, but if @bernhardoj has the time and will manage to find a solution that will follow what I just explained and won't introduce any regressions we're more than happy to let him handle it, since we have a bunch of other issues we're working on right now.

Let me know what do you think @s77rt 🙌🏻

s77rt commented 3 months ago

@BartoszGrajdek Makes sense! My only concern is that in the following code:

```\r\nHello\r\n```

The content is Hello and not \r\nHello\r\n as the newlines are part of the codeblock syntax but I suppose this is not a real blocker.

@bernhardoj Can we update the regex to just not consume the newlines? The newlines should be kept though so the following code does not match as it's not a valid code block

```Hello```

Can you also please run the tests to confirm we are not breaking anything

Another solution is to still consider the content correctly as Hello but capture the newlines into a group and add them back when reconstructing the markdown text, this way instead of adding \n here https://github.com/Expensify/react-native-live-markdown/blob/2ada06efda92c81f07044afc1467e77075206059/parser/index.ts#L159 we will add whatever line break we had. This may require some work, let's try and go with the other solution first assuming it's the easiest to implement

bernhardoj commented 3 months ago

Can we update the regex to just not consume the newlines?

```\r\nHello\r\n```

With this example, do you mean that the content is just Hello? We can do that but the live markdown will convert it to (assuming the new line here is removed)

```Hello```

without any newline and we will still get the mismatch error.

s77rt commented 3 months ago

@bernhardoj The content would still have the newlines. Can you please include your previous solution? Moving the newlines to the second group should do it

bernhardoj commented 3 months ago

Added back my prev solution and moved the current solution as the alternative.

s77rt commented 3 months ago

@bernhardoj Thank you! Let's go with that

cc @marcochavezf @BartoszGrajdek

BartoszGrajdek commented 3 months ago

Much better, I also think we can go with that, thank you!

Please tag me in the PR to expensify-common, so that I can test it properly before we merge it 🙏🏻

marcochavezf commented 3 months ago

Cool, thank you guys! Assigning @bernhardoj 🚀

melvin-bot[bot] commented 3 months ago

📣 @s77rt 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 3 months ago

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer 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 📖

bernhardoj commented 3 months ago

Expensify-common PR is ready

cc: @s77rt

BartoszGrajdek commented 3 months ago

@thienlnam this is also an issue related to live markdown, could we add it to the project? 🙏🏻

melvin-bot[bot] commented 3 months ago

@marcochavezf @s77rt @bernhardoj @kadiealexander 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!

melvin-bot[bot] commented 3 months ago

@marcochavezf, @s77rt, @bernhardoj, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

marcochavezf commented 3 months ago

Update: PR still in review

s77rt commented 3 months ago

Not overdue. Waiting on expensify-common PR merge

bernhardoj commented 3 months ago

The react-native-live-markdown PR is ready: https://github.com/Expensify/react-native-live-markdown/pull/413

cc: @s77rt @BartoszGrajdek

bernhardoj commented 3 months ago

App PR is ready

cc: @s77rt

melvin-bot[bot] commented 2 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 2 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.7-8 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/44958

If no regressions arise, payment will be issued on 2024-07-24. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@s77rt requires payment automatic offer (Reviewer)
@bernhardoj requires payment automatic offer (Contributor)

melvin-bot[bot] commented 2 months 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:

[ ] [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@s77rt] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@s77rt] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] [@s77rt] Determine if we should create a regression test for this bug.
[ ] [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

bernhardoj commented 2 months ago

I'll request in ND once payment is due.

s77rt commented 2 months ago

The PR that introduced the bug has been identified: This existed since day one
The offending PR has been commented on: n/a
A discussion in #expensify-bugs has been started: Not needed (unique bug)
Determine if we should create a regression test for this bug: No

kadiealexander commented 2 months ago

@bernhardoj I see you've accepted the Upwork contract, would you prefer to be paid via NewDot?

bernhardoj commented 2 months ago

Yes please, I have ended the UW contract and requested in ND instead.

JmillsExpensify commented 2 months ago

Can I get a payment summary for ND payment approval?

kadiealexander commented 2 months ago

Payouts due:

[x] Contributor: $250 @bernhardoj via NewDot
[x] Contributor+: $250 @s77rt via NewDot

JmillsExpensify commented 2 months ago

$250 approved for @bernhardoj

JmillsExpensify commented 2 months ago

$250 approved for @s77rt

Expensify / App

[HOLD for payment 2024-07-24] [$250] Expense - Console error shows up when opening description that contains code block #43750

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)