[HOLD for payment 2024-04-09] [$500] Mark down - Live preview for code block with strikethrough is incorrect

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.45-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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Launch New Expensify app
2. Go to any chat
3. Type Code
4. Note that live mark down preview
5. Send the text

Expected Result:

The live mark down preview for Code should correctly show the actual mark down

Actual Result:

The live mark down preview for Code is incorrect. It shows that the strikethrough is applied, when strikethrough does not work with code block in the sent text

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6397312_1709225866856.Screen_Recording_20240229_224538_New_Expensify.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012e80f0cbfad42ad6
• Upwork Job ID: 1763259471765831680
• Last Price Increase: 2024-02-29

[melvin-bot]

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

[lanitochka17]

We think that this bug might be related to #vip-vsp
CC @quinthar

[lanitochka17]

@puneetlath 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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~012e80f0cbfad42ad6

[melvin-bot]

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

[brandonhenry]

Proposal

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

The issue at hand involves the live markdown preview in the New Expensify app. When a user types code within a chat and applies the strikethrough markdown, the live preview incorrectly displays the strikethrough effect. However, upon sending the text, the sent message does not reflect the strikethrough within the code block. This inconsistency between the live preview and the actual message sent leads to confusion.

What is the root cause of that problem?

The root cause of this problem lies within the markdown rendering logic used. Specifically, the issue arises from the live preview functionality not accurately mirroring the markdown parsing rules that are applied to the text once it is sent. The discrepancy indicates that the parsing rules for strikethrough text are not being consistently applied or that the code block rendering logic in the live preview does not correctly handle strikethrough markdown.

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

To address this issue, we need to make the following changes:

1. Update the Markdown Rendering Logic: Review and update the markdown rendering logic to ensure that strikethrough markdown is not applied within code blocks in the live preview. This involves modifying the parsing rules to ignore strikethrough syntax (~~text~~) within code blocks (`code` or ```code```).

// Pseudo-code example for updating markdown rendering logic
function renderMarkdown(text) {
    // Logic to detect and ignore strikethrough within code blocks
    const updatedText = text.replace(/(```[\s\S]*?```|`[^`]*`)/g, block => {
        // Remove strikethrough syntax inside code blocks
        return block.replace(/~~(.*?)~~/g, '$1');
    });
    return render(updatedText); // Existing function to render markdown
}

[brandonhenry]

I'm not able to reproduce this one

[puneetlath]

@brandonhenry live markdown is only on mobile right now, FYI. I'm able to reproduce it there.

[brandonhenry]

this issue seems related to https://github.com/Expensify/react-native-live-markdown shouldn't this be posted there? @puneetlath

[bernhardoj]

Proposal

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

The live markdown shows a preview of a strikethrough + code fence markdown, but when sent, the strikethrough is not applied.

What is the root cause of that problem?

One of the differences between the live markdown and the sent markdown is that the live markdown set shouldKeepRawInput to true.

In ExpensiMark rule, strikethrough won't be converted to HTML if it includes a <pre> (codefence) tag.
https://github.com/Expensify/expensify-common/blob/77d0b150ba6bfbe7a64b3c3e30b65592b2e58c4a/lib/ExpensiMark.js#L294-L298

However, if shouldKeepRawInput is enabled, it will convert codefence to <pre data-code-raw ...> which fails the condition above
https://github.com/Expensify/expensify-common/blob/77d0b150ba6bfbe7a64b3c3e30b65592b2e58c4a/lib/ExpensiMark.js#L46-L50

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

I'm pretty sure we want to keep the old behavior, that is to prevent strikethrough being converted to HTML when it contains a codefence.

To fix it, instead of checking whether it includes the opening tag (i.e., <pre>), we can check whether it includes the closing tag (i.e., </pre>) as we did for h1 here.

We need to apply the fix to all occurrences (bold, strikethrough, italic, link)

What alternative solutions did you explore? (Optional)

1. Use a regex to check whether it contains pre tag that optionally has an attribute (we can take the codefence regex here).
2. Allow strikethrough (italic, bold, and link) + codefence by removing the <pre> tag checking

[melvin-bot]

@puneetlath, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@puneetlath, @allroundexperts Eep! 4 days overdue now. Issues have feelings too...

[allroundexperts]

Reviewing today.

[allroundexperts]

Thanks for the proposal @brandonhenry. I think you missed to include italics / links in your proposal which in my opinion should be removed as well. Also, I think for consistency, we should follow the pattern already established in the h1 parsing.

As such, I think @bernhardoj's proposal looks better to me.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

@puneetlath, @allroundexperts Huh... This is 4 days overdue. Who can take care of this?

[puneetlath]

Sounds good 👍🏾

[melvin-bot]

❌ There was an error making the offer to @bernhardoj for the Contributor role. The BZ member will need to manually hire the contributor.

[bernhardoj]

The expensify-common PR is here

cc: @allroundexperts

[melvin-bot]

@puneetlath @allroundexperts @bernhardoj 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!

[allroundexperts]

I am reviewing this Melv

[bernhardoj]

Copying the comment from the PR to here for visibility.

Is the next step to open an App PR to apply this expensify-common update?

@puneetlath @allroundexperts we would need to update the expensify-common version in react-native-live-markdown, but I think it would make sense to have the same version of expensify-common in App too.

react-native-live-markdown already has the latest version of expensify-common (dc8ea98), except our recently merged commit (9e47e1f). I think we need a help from SWM team to update the package as react-native-live-markdown latest version is 0.1.28 and if we update it, that would be 0.1.29 which is 24 patch version apart from the current version in the App (0.1.5).

I see that this issue will update the react-native-live-markdown to the latest version, but I don't see the App PR yet (@robertKozik cmiiw). I think we can hold for that issue or ask them to include our expensify-common update to react-native-live-markdown.

[allroundexperts]

Makes sense to me.

[shubham1206agra]

Please hold your issue to #38152 instead.
cc @allroundexperts

[bernhardoj]

Thanks @shubham1206agra. I see that #38152 bumps react-native-live-markdown to a higher version which includes our fix too. Let's hold for that instead.

@puneetlath @allroundexperts

[puneetlath]

Looks like that was merged. Can you test that everything is working as expected with our changes in this issue?

[bernhardoj]

Here is the retesting result, all works fine (as the live markdown is now available on Web, I have included the web result too):

Android

Screen.Recording.2024-03-28.at.11.50.08.mov

iOS

Screen.Recording.2024-03-28.at.11.45.34.mov

Android mWeb

Screen.Recording.2024-03-28.at.11.49.17.mov

iOS mWeb

Screen.Recording.2024-03-28.at.11.47.33.mov

Desktop

Screen.Recording.2024-03-28.at.11.42.49.mov

Web

Screen.Recording.2024-03-28.at.11.40.35.mov

[puneetlath]

Ok great! So we can pay according to when that PR goes to production.

[melvin-bot]

@puneetlath, @allroundexperts, @bernhardoj Still overdue 6 days?! Let's take care of this!

[puneetlath]

Payment summary:

• C - $500 - @bernhardoj - to be paid via Upwork
• C+ - $500 - @allroundexperts - to be paid via NewDot

@bernhardoj can you please accept the Upwork offer? https://www.upwork.com/nx/wm/offer/101349515

[puneetlath]

@bernhardoj has been paid. @allroundexperts please request on NewDot.

Thanks everyone!

[JmillsExpensify]

$500 approved for @allroundexperts