capture newline as the code fence content #730

bernhardoj · 2024-06-25T05:42:52Z

Fixed Issues

$ Expensify/App#43750

Tests

What unit/integration tests cover your change? What autoQA tests cover your change?
Updated existing unit test for codefence.
What tests did you perform that validates your changed worked?
To test this in App,

open live markdown repo, apply the expensi-mark and live-markdown fix
run cd parser && npm run build
copy the parser/react-native-live-markdown-parser to App node_modules/@expensify/react-native-live-markdown/lib/parser/react-native-live-markdown-parser.js.

Submit an expense with code block as description
Open the transaction thread
Open the description
Verify there is no console error of react-native-live-markdown

Web

Screen.Recording.2024-06-25.at.13.32.56.mov

Desktop

Screen.Recording.2024-06-25.at.19.59.10.mov

iOS mWeb

Screen.Recording.2024-06-25.at.20.02.21.mov

iOS Native

Screen.Recording.2024-06-25.at.20.06.46.mov

Android mWeb

Screen.Recording.2024-06-25.at.20.08.08.mov

Android Native

Screen.Recording.2024-06-25.at.20.12.54.mov

QA

Same as Tests above

bernhardoj · 2024-06-25T05:43:48Z

@BartoszGrajdek here is the PR for Expensify/App#43750

BartoszGrajdek · 2024-06-25T10:16:58Z

Testing today!

BartoszGrajdek

Looks good to me - please attach some test videos for other platforms, because these changes affect all of them 🙏🏻

bernhardoj · 2024-06-25T12:13:40Z

Done 👍

s77rt · 2024-06-25T13:30:42Z

__tests__/ExpensiMark-HTML-test.js

@@ -2220,7 +2220,7 @@ describe('room mentions', () => {

    test('room mention inside code block should not be rendered', () => {
        const testString = '```\n#room\n```';
-        const resultString = '<pre>#room<br /></pre>';
+        const resultString = '<pre><br />#room<br /></pre>';


I believe this is unwanted. HTML BR collapsing works for <br /></pre> but not for <pre><br /> https://codesandbox.io/s/silly-brattain-dz3rrw

Interesting. The App PreRenderer works fine though, but I think we may want to avoid this. Maybe we can put the new line info on the <pre> tag like:

<pre data-newline="\r\n">

and live markdown can get the new line from data-newline.

That would work but it won't work if we have mixed newlines. Technically the live markdown works correctly and the console error is a false positive. Maybe we can ignore the error if the difference between the original text and the reconstructed one is only the new lines? Though ultimately we want the reconstructed message to be the same

That would work but it won't work if we have mixed newlines

Hmm, you're right.

Maybe we can ignore the error if the difference between the original text and the reconstructed one is only the new lines?

I think we can replace all the new lines to \n only when doing the comparison. This way, we don't alter the user input at all, but we need @BartoszGrajdek thoughts on this one.

What if we added the line break just in rawInputReplacement? The returned value of it is coming only into react-native-live-markdown input, so it won't affect how we treat code blocks in the chat itself, just the input.

We could just treat the linebreak before code block content as a separate group and use it only in rawInputReplacement.

Yeah, that works.

@s77rt do you agree with the above suggestion?

Sounds good to me but can you please test this against react-native-live-markdown and verify that we don't get extra newlines

Yeah, I don't see any extra newlines

Let's go with that then 👍

s77rt · 2024-06-28T16:10:21Z

lib/ExpensiMark.ts

@@ -129,21 +129,21 @@ export default class ExpensiMark {
                name: 'codeFence',

                // &#x60; is a backtick symbol we are matching on three of them before then after a new line character
-                regex: /(&#x60;&#x60;&#x60;(?:\r\n|\n))((?:\s*?(?!(?:\r\n|\n)?&#x60;&#x60;&#x60;(?!&#x60;))[\S])+\s*?(?:\r\n|\n))(&#x60;&#x60;&#x60;)/g,
+                regex: /(&#x60;&#x60;&#x60;(\r\n|\n))((?:\s*?(?!(?:\r\n|\n)?&#x60;&#x60;&#x60;(?!&#x60;))[\S])+\s*?(?:\r\n|\n))(&#x60;&#x60;&#x60;)/g,


NAB. This has 4 capturing groups but we only need 2. Can you make the other two non-capturing groups?

I think there is 6 groups in total including the negative lookahead and we use 3, but I will leave it as it is.

s77rt · 2024-06-28T16:12:34Z

lib/ExpensiMark.ts

                    const group = textWithinFences.replace(/(?:(?![\n\r])\s)/g, '&#32;');
                    return `<pre>${group}</pre>`;
                },
-                rawInputReplacement: (_extras, _match, _g1, textWithinFences) => {
+                rawInputReplacement: (_extras, _match, _g1, g2, textWithinFences) => {


Rename the group variable that has the newline to something more fitting / explaining what the groups holds e.g. newLineCharacter

Right, updated!

s77rt · 2024-07-01T12:22:16Z

@BartoszGrajdek @marcochavezf Can you please review/approve if the changes looks good to you

BartoszGrajdek · 2024-07-01T14:12:15Z

Hi! Code changes look good to me, I'll just try testing it inside react-native-live-markdown lib tomorrow morning before approving to ensure no regressions are introduced.

I'm currently working on an issue with higher priority, hence the delay 🙏🏻

BartoszGrajdek

LGTM - Works well in react-native-live-markdown ✅

github-actions · 2024-07-02T14:28:10Z

🚀Published to npm in v2.0.35

bernhardoj · 2024-07-02T14:29:18Z

Will prepare the App PR tomorrow!

capture newline as the code fence content

10445d6

bernhardoj requested a review from a team as a code owner June 25, 2024 05:42

melvin-bot bot requested review from carlosmiceli and removed request for a team June 25, 2024 05:43

Merge branch 'main' into fix/code-fence-content

7e55027

BartoszGrajdek reviewed Jun 25, 2024

View reviewed changes

s77rt suggested changes Jun 25, 2024

View reviewed changes

marcochavezf self-requested a review June 26, 2024 16:47

add the newline as pre content for raw input only

3ab10bf

s77rt suggested changes Jun 28, 2024

View reviewed changes

marcochavezf mentioned this pull request Jun 28, 2024

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

Closed

6 tasks

update group name

4cbb0ce

s77rt approved these changes Jun 29, 2024

View reviewed changes

BartoszGrajdek approved these changes Jul 2, 2024

View reviewed changes

carlosmiceli approved these changes Jul 2, 2024

View reviewed changes

carlosmiceli merged commit a6bc4c1 into Expensify:main Jul 2, 2024
6 checks passed

bernhardoj mentioned this pull request Jul 3, 2024

Update expensify-common and remove appending the newline to the codefence content Expensify/react-native-live-markdown#413

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

capture newline as the code fence content #730

capture newline as the code fence content #730

bernhardoj commented Jun 25, 2024 •

edited

Loading

bernhardoj commented Jun 25, 2024 •

edited

Loading

BartoszGrajdek commented Jun 25, 2024

BartoszGrajdek left a comment

bernhardoj commented Jun 25, 2024

s77rt Jun 25, 2024

bernhardoj Jun 25, 2024 •

edited

Loading

s77rt Jun 25, 2024

bernhardoj Jun 26, 2024

BartoszGrajdek Jun 26, 2024

bernhardoj Jun 26, 2024

s77rt Jun 26, 2024

bernhardoj Jun 27, 2024

s77rt Jun 27, 2024

bernhardoj Jun 28, 2024

s77rt Jun 28, 2024

bernhardoj Jun 29, 2024

s77rt Jun 28, 2024

bernhardoj Jun 29, 2024

s77rt commented Jul 1, 2024

BartoszGrajdek commented Jul 1, 2024

BartoszGrajdek left a comment

github-actions bot commented Jul 2, 2024

bernhardoj commented Jul 2, 2024

capture newline as the code fence content #730

capture newline as the code fence content #730

Conversation

bernhardoj commented Jun 25, 2024 • edited Loading

Fixed Issues

Tests

QA

bernhardoj commented Jun 25, 2024 • edited Loading

BartoszGrajdek commented Jun 25, 2024

BartoszGrajdek left a comment

Choose a reason for hiding this comment

bernhardoj commented Jun 25, 2024

Choose a reason for hiding this comment

bernhardoj Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s77rt commented Jul 1, 2024

BartoszGrajdek commented Jul 1, 2024

BartoszGrajdek left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 2, 2024

bernhardoj commented Jul 2, 2024

bernhardoj commented Jun 25, 2024 •

edited

Loading

bernhardoj commented Jun 25, 2024 •

edited

Loading

bernhardoj Jun 25, 2024 •

edited

Loading