App: [HOLD for payment 2023-11-30] [$2000] Newline is removed from code block with a character before it

worked

I think this should be re-opened. Because of this inconsistency, more critical bugs happening. i.e.

• #18928

cc: @cead22 @twisterdotcom

@cead22 What do you think about the opposite case? when copying this case from another resource to a new dot do we care about it? What is the reason for using different Markdown rules?

Can you ask these in #expensify-open-source?

I’ve sent an offer of $250 as a bonus for the investigation @eh2077. Thanks for your work on this here.

What do you think? Increase the price then?

yes, we can cap to 2k based on difficulty

We can wait for more proposals here, we only agreed to take them yesterday.

I will deep into @eh2077’s proposal but yes we’re open for more proposals.

Same as yesterday, could not start the discussion because I was too focused on some N6.99 N7 P.R.'s and a deploy blocker.

Will try again to start the discussion tomorrow.

@bernhardoj I think you meant to tag @eh2077 this happened to me the second time today 😂

@PauloGasparSv makes sense, but that was old discussion. Can you confirm again on slack before close?

Sure thing @0xmiroslav, will create a new thread in expensify-open-source (and link the previous one + this issue + cc you guys) so we can involve more people in the discussion and propose that we only fix Case 1.

I just need some time to read all the previous threads and make sure I’m not missing anything or mentioning something we already discussed, so I’ll create it after lunch Today!

Personally, I think we should line break always at the start and end of code block. No reason we should do nothing despite of inconsistency between start and end, | and other characters, copy with selection and from context menu.

Regardless whether we want to handle the 2nd issue, I think we should handle the 1st case as it’s only happening to | character which is not consistent with other character and @eh2077 already have a straightforward and simple solution to it.

@PauloGasparSv that old issue was reported by me 🙂 and I was already aware of that . I double confirmed because it’s stated in the issue description as Case 2. If we still don’t wanna fix it, let’s remove that case.

There are 2 issues with different root causes. Copy paste issue (Case 1) is related to | character only. Edit issue (Case 2) happens on all characters. @PauloGasparSv @twisterdotcom should we also consider Case 2 as bug and fix it here?

Proposal

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

Two issues need to solve

1. Copy and paste code block message wth pipe character before misses a line break
2. ‘```’ isn’t displayed in new line in editing mode

What is the root cause of that problem?

Click to see RCA of the first issue

From Web(Chrome/Safari), when we copy message through ctrl/cmd+c, we set html selection to clipboard here.

We send following two new comments to dig the root cause

|
```
code
```

A
```
code
```

like

We add a log before this line and copy the above message through ctrl/cmd+c. The selection html is shown as below

If we paste in composer, then we convert html to markdown here

HTML selection

<comment><div>| </div><pre><span>code</span></pre></comment>

is converted into markdown

| ```
code
```

While HTML selection

<comment><div>A </div><pre><span>code</span></pre></comment>

is converted into markdown

A 
```
code
```

To figure out why the pipe | character case behaves differently, we can have a closer look at htmlToMarkdown method. The div tag(replaced with [block]) should also be translated into a line break for pipe | character case in replaceBlockWithNewLine. But there’s a flaw in this regex that it can match pipe | character. So text.match(/[\n|>][>]?[\s]?$/) returns true and skips to add a line break. See regex testing picture below

The above is the RCA of copy and paste issue for pipe | character case.

Let’s move forward to dig the line break issue in editing mode.

As the line break missing issue in editing mode is same for having pipe | character case, let’s send following message to dig the root cause

A
```
code
```

We translate markdown text into html here and save following message html to backend.

A <pre>code<br /></pre>

When replacing markdown to html, we remove the line break after A and replace it with a space ’ ’ using this rule. That’s why there’s an extra space after A in the message html.

When editing a sent message, we call htmlToMarkdown to convert message html to markdown text here. The message html of above sent comment is

A <pre>code<br /></pre>

which is much simpler than the html actually rendered in the page.

In method htmlToMarkdown, the above message html is translated into following markdown text by applying newline rule and codeFence rule.

A ```
code
```

The root cause is the way we translate codefence from markdown-to-html and from html-to-markdown.

When digging the root cause, I went through this closed issue https://github.com/Expensify/App/issues/12783 which explains the root cause and ends up do nothing.

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

To fix the first issue for pipe | character case, we can remove the unnecessary pipe | character in this regex.

To fix the second issue, we can improve the regex rule to translate pre tag into code fence markdown. We handle line break before <pre> by improving the codeFence rule.

The key idea is to sort out cases before the <pre> tag and handle line break of each case.

The improved regex rule will look like

{
    name: 'codeFence',
    // We use to (<\/(\w+)>| )? to capture the closing tag or a space before <pre> tag
    // if g1 is undefined then the prefix is empty string
    regex: /(<\/(\w+)>| )?<(pre)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)(\n?)<\/\3>(?![^<]*(<\/pre>|<\/code>))/gi,
    replacement: (match, g1, g2, g3, g4) => {
        let prefix = '';
        if (g1) {
            if (g1 === ' ') {
                // We're using a trick to handle line break before <pre> tag, see 'replacebr' rule. It's like a magic to make line break between <pre> behave consistently on Web and native platforms.  Note that only text copied from Expensify App has space before <pre> tag. 
                // see https://meliorence.github.io/react-native-render-html/api/renderhtmlprops#enableexperimentalbrcollapsing
                // see https://github.com/Expensify/App/blob/91e2120e647aa9ee22171b6dbb772b4aebe64992/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js#L16
                prefix = '\n';
            } else if (/div|comment|h1|h2|h3|h4|h5|h6|p|li|blockquote/i.test(g2)) {
                // We just add the closing block type tag back
                prefix = `</${g2}>`;
            } else {
                // g1 is inline closing tag and we insert a line break after it
                prefix = `</${g2}>\n`;
            }
        }
        return `${prefix}\`\`\`\n${g4}\n\`\`\``;
    },
}

By doing so, we can pass all existing tests of Expensify-common and solve a bunch of twists related to this feature.

What alternative solutions did you explore? (Optional)

I also agree with @iwiznia to save the original text entered by user to solve this issue as he commented here.

Proposal

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

The ``` is at the same line as | when we write it in a different line. We wanted to make it still in their own line.

What is the root cause of that problem?

In Markdown, the | symbol is used to separate columns in a table, and it is not intended to have a line break when it is copied and pasted. When you copy and paste a table created in Markdown, the | symbol is used to indicate the boundaries between columns, and it is preserved in the copied text.

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

We need to manually check if this symbol exists and add a <br /> in HTML and \n in text format.

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief, and avoid jargon. Feel free to use images, charts, or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

There are several libraries out there to handle this problem, but it requires more changes in the code and I didn’t recommend this method.