[HOLD for Payment 2024-09-09][$250] Wrong parsing when single backticks used in 2 separate places

[m-natarajan]

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: 9.0.17-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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @coleaeason
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1722625457122619

Action Performed:

1. Open any report
2. Send the string "it uses ; separated fields because the line number column could have : in them"

Expected Result:

it uses;separated fields because the line number column could have:in them

Actual Result:

it uses ; separated fields because the line number column could have : in them

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014560c8f3601ebcb2
• Upwork Job ID: 1823820445986093493
• Last Price Increase: 2024-08-14
• Automatic offers:
• ikevin127 | Reviewer | 103588545
• tsa321 | Contributor | 103588546

Issue Owner

Current Issue Owner: @ikevin127

[melvin-bot]

Triggered auto assignment to @JmillsExpensify (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.

[tsa321]

Edited by proposal-police: This proposal was edited at 2024-08-07 07:14:57 UTC.

Proposal

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

Wrong parsing when single backticks with a semicolon character used in 2 separate places

What is the root cause of that problem?

The regex rule for inlineCodeBlock:

https://github.com/Expensify/expensify-common/blob/fea340b9d71bf2415ed1e8745ac60bafb4fc0cfa/lib/ExpensiMark.ts#L193

because of the square bracket in (?![`]) it search for optional character inside the square bracket instead of the back tick character. If inside the code is one of the character in this case ; the result will be incorrect.

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

We could modify the regex to be:

 regex: /(\B|_|)&#x60;((?:&#x60;)*)(?!&#x60;)(.*?\S+?.*?)(?<!&#x60;)((?:&#x60;)*)(&#x60;)(\B|_|)(?!&#x60;|[^<]*<\/pre>|[^<]*<\/video>)/gm,
 replacement: '$1<code>$2$3$4</code>$6',

[melvin-bot]

@JmillsExpensify Huh... This is 4 days overdue. Who can take care of this?

[JmillsExpensify]

Hmm @thienlnam should we resolve this issue as part of on-going maintenance of our markdown library?

[thienlnam]

We've been making some of these external - cc @BartoszGrajdek Looks like this one can also be external right? Or would you like to take it

[BartoszGrajdek]

Looks like this one can also be external right?

Yes, this is something External contributors can definitely work on 🙌🏻

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~014560c8f3601ebcb2

[melvin-bot]

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

[ikevin127]

@tsa321 Thanks for the proposal, noticed quite a few edits there 😅 Unfortunately even though the regex you suggested seems to fix the issue according to the issue's Expected result, it fails the following ❌ 6 tests in
ExpensiMark-HTML-test.js:

• ❌ Test inline code blocks with triple backticks
• ❌ Test inline code with one backtick as content
• ❌ Test inline code with multiple backtick symbols as content
• ❌ Test for codefence with no content
• ❌ Video markdown conversion to html tag › inline code in alt
• ❌ Video markdown conversion to html tag › blockquote in alt

Besides the failing tests, which make the proposed solution unacceptable, I also noticed that given the following input:

it uses`;`separated fields because the line number column could have`:`in them ` ` with space

Once sent, the parsing will have the message look like this:

causing white-space (empty space) inline code-block to not be parsed and instead showing the back-ticks instead of an empty code-block.

If still interested in fixing the issue I suggest cloning the expensify-common repository and running the tests as you iterate and try to come-up with a regex that would fix our issue while not breaking any of the tests (without adjusting the tests to "match" the new regex).

[tsa321]

@ikevin127 Regarding the test:
it uses `;`separated fields because the line number column could have`:`in them ` ` with space

causing white-space (empty space) inline code-block to not be parsed and instead showing the back-ticks instead of an empty code-block.

I’ve updated the regex to:
regex: /(\B|_|)&#x60;(.*?(?![&#x60;])[\S| ].*?)&#x60;(\B|_|)(?!&#x60;|[^<]*<\/pre>)/gm

This will renders inline code with spaces correctly when I send the message. However, it fails the ExpensiMark-HTML-test.js test:

Test for backticks with no content.
The expected result for this test is that inline code blocks with only space should not be parsed.

[tsa321]

Proposal

@ikevin127 I have updated my proposal. The regex and replacement has been updated and now passes all tests.

[ikevin127]

@tsa321's updated proposal looks good to me, all expensify-common tests are 🟢 Passing. The RCA is correct and the proposed solution fixes the issue as per the Expected result.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[ikevin127]

🗒️ Some things to note

Important

These are out of scope for our current issue since they are present on current staging as well.

1. White-space (empty space) inline code-block showing the back-ticks instead of an empty code-block

Given the following input:

it uses`;`separated fields because the line number column could have`:`in them ` ` with space

Once sent, the parsing will have the message look like this:

causing white-space (empty space) inline code-block to not be parsed and instead showing the back-ticks instead of an empty code-block.

2. Nested inline code-block parsing is broken (compared to GitHub's for example)

Given the following input:

```nest 1 `nest 2` nest 1```

Once sent, the parsing will have the message look like this:

For reference this is how GitHub is parsing it:

cc @BartoszGrajdek

[dangrous]

Do we want to open separate issues for those two bugs you noted? Not sure if they exist already.

For this one, the proposal sounds good!

[melvin-bot]

📣 @ikevin127 🎉 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]

📣 @tsa321 🎉 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 📖

[tsa321]

@ikevin127 PR is ready.

[melvin-bot]

@JmillsExpensify @dangrous @ikevin127 @tsa321 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!

[ikevin127]

🟢 expensify-common PR was reviewed, more details in Expensify/expensify-common#785 (comment). 🔖 Will complete the PR Reviewer Checklist on the E/App PR once the expensify-common PR is merged.

[dangrous]

e-c PR is merged!

[BartoszGrajdek]

@ikevin127 sorry for the late response to this comment - it got buried under too many GH notifications 😅

I agree with the 1st note, this is probably something we could think about improving.

As for the 2nd note this is actually something we had a lengthy discussion about right here - let me know if you have access to it. There's no support for an "inline" code block (triple backticks) if you wish to create a code block linebreaks are required between backticks and the content itself.

[dangrous]

Thanks for the input @BartoszGrajdek!

Do you (both) think it's worth it for me to create an issue to look into the first note and fix it? Seems pretty minor but I guess it can't hurt

[BartoszGrajdek]

@dangrous I think we can create one, but as you said it looks like a pretty minor bug.

This is something that we can open to proposals from external contributors. It looks like it only requires some changes in expensify-common (ExpensiMark.ts specifically)

[ikevin127]

⚠️ Production deploy automation failed here -> this should be on [HOLD for Payment 2024-09-09] according to yesterday's production deploy from comment.

cc @dangrous @JmillsExpensify

[ikevin127]

cc @JmillsExpensify

[ikevin127]

cc @JmillsExpensify bump for payment

[ikevin127]

cc @JmillsExpensify bump for payment

[JmillsExpensify]

Payment summary:

• Contributor: @tsa321 $250
• Reviewer: @ikevin127 $250

[JmillsExpensify]

@ikevin127 do we have a BZ checklist for this issue yet?

[ikevin127]

• Not sure whether regression test is needed for this one because on expensify-common PR Fix inline code regex mismatch when there is an escaped backtick character inside the backtick expensify-common#785 we added unit tests, but anyway.

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: Change accepted syntax for code fence expensify-common#690
• [@ikevin127] 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: https://github.com/Expensify/expensify-common/pull/690/files#r1757521200
• [@ikevin127] 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: N/A.
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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.

Regression Test Proposal

1. Open any report.
2. Send the following message:

test `;` test `:` test

4. Verify that the message is displayed correctly as:

   test ; test : test

cc @JmillsExpensify

Do we agree 👍 or 👎.

[JmillsExpensify]

Contributors paid via Upwork and regression test created. I'm closing this one.