[HOLD for payment 2023-07-21] [$1000] Web - Heading after quote is not rendering #21846

kbecciv · 2023-06-28T21:44:36Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Go a chat and add a comment

> quote
# heading

Notice that heading is displayed in plain text

Expected Result:

Heading should be rendered

Actual Result:

Markdown heading syntax is not rendering

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.32-5
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
Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/8626f832-2e3e-4705-9f42-59b82d7d7172
https://github.com/Expensify/App/assets/93399543/7f95a2a1-6c96-4a48-afbe-712edae38043

Expensify/Expensify Issue URL:
Issue reported by: @eh2077
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1687921581684809

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01e498007bd2f0de5b
Upwork Job ID: 1674345101139329024
Last Price Increase: 2023-07-06

The text was updated successfully, but these errors were encountered:

melvin-bot · 2023-06-28T21:44:41Z

Triggered auto assignment to @conorpendergrast (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

melvin-bot · 2023-06-28T21:44:41Z

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

melvin-bot · 2023-06-28T21:44:43Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot · 2023-06-28T21:44:46Z

Triggered auto assignment to Design team member for new feature review - @shawnborton (NewFeature)

spcheema · 2023-06-28T23:15:53Z

Proposed solution

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

Heading after quote is not rendering

What is the root cause of that problem?
The root cause of the problem is that \n is deliberately removed after a quote string > quoted string.

A new line is added after the quote message which later got deleted using below code

    formatTextForQuote(regex, textToCheck, replacement) {
        if (textToCheck.match(regex)) {
            // Remove '&gt;' and trim each line of quote content
            let textToFormat = textToCheck.split('\n').map(row => row.substr(4).trim()).join('\n'); <====== trim is happening here

            // Remove leading and trailing line breaks
            textToFormat = textToFormat.trim();
            return replacement(textToFormat);
        }
        return textToCheck;
    }

Input string given to modifyTextForQuote function

    &gt; quote\n# heading

so after parsing the text following output is retuned.

<blockquote>quote</blockquote># heading

Now heading regex is looking for # on start of line as below which does not match string <blockquote>quote</blockquote># heading hence it fails to parse heading tag

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

Move heading1 rule to be before quote
Modify heading1 rule to not capture the newline: /^# +(?! )((?:(?!<pre>|\n|\r\n).)+)/gm
Add new rule replaceh1br that repalces <br /> next to <h1> tag

{
      // We're removing <br /> because when <h1> and <br /> occur together, an extra line is added.
      name: 'replaceh1br',
      regex: /<\/h1><br\s*[/]?>/gi,
      replacement: '</h1>',
},

Output generated for a given input

Input: '> quote\n# heading'
Output: '<blockquote>quote</blockquote><h1>heading</h1>'


Input: `> a\n# da\n> test`
Output: '<blockquote>a</blockquote><h1>da</h1><blockquote>test</blockquote>'


Input: `>This is a *quote* that started on a new line.\n# Heading 1 should be captured\nHere is a >quote that did not\n`
Output '<blockquote>This is a <strong>quote</strong> that started on a new line.</blockquote><h1>Heading 1 should be captured</h1>Here is a &gt;quote that did not<br />'

Additional inputs tested

```text > quote # heading > quote ```

> quote
# heading
> quote # heading 
 quote

> quote
# heading
> quote # heading 
 quote 
# heading

> quote
# heading
> quote

# heading after 2 lines
> quote 
# heading

> quote
# heading
> quote #heading
# heading
> quote 
# heading
`
     
```text
> DFFDS
# FDS
*FDDSF*
# SDSAF ~SFASDF~
> quote 
# heading

> quote
# heading
> quote # heading

# heading
# heading
> quote
 #heading
> quote
#heading
# heading

What alternative solutions did you explore? (Optional)

Modify modifyTextForQuote function in order to add new line char when heading is next after the blockquote. It'll let heading rule to parse heading tag as-is

      // check if next text chunk is  heading tag i.e. starts with #  then append new line char \n
      if (Str.startsWith(textSplitted[i], "# ") )
      {
          replacedText = `${replacedText}\n`
      }

After adding \n, parsed output is <blockquote>quote</blockquote><br /><h1>heading</h1> where unnecessary br tag is added between blockquote and h1

Add a new rule to check if a <br /> occurs between blockquote and h1 then remove <br /> tag (we got similar rules replacebr, replacepre). After processing this rule final output is <blockquote>quote</blockquote><h1>heading</h1>.

Code changes required:

   // check if next text chunk is  heading tag `starts with # ` then append new line char \n
   if (Str.startsWith(textSplitted[i], "# ") )
   {
      replacedText = `${replacedText}\n`
   }

   {
      // We're removing <br /> because when <br /> occur between <blockquote> and <h1>, causing an extra line is added.
      name: 'replaceBlockquoteBr',
      regex: /<\/blockquote><br\s*[/]?><h1>/gi,
      replacement: '</blockquote><h1>',
   }

Since newline rule replaces \n with <br />. So regex can modified as <\/blockquote><br\s\/><h1>

spcheema · 2023-06-28T23:20:48Z

@kbecciv It actually a bug that went unnoticed. Kindly read through the root cause mentioned in my proposal and you will have an idea why it’s happening.
Heading regex expects ‘#’ on start of the line which never gets captured.

code0406 · 2023-06-29T01:33:40Z

Proposal

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

heading in the new line in "quote" section is not formatted.

What is the root cause of that problem?

The root cause is that quote rule is applied before heading1 rule.
So the new line tag also was replaced with </blockquote> and heading1 rule is not applied without \n tag.
I explained it in the "Reason-" section.
ReportUtils.getParsedComment / ExpensiMark.replace()

The replace function applies rules to convert the text as HTML.(convert markdown characters to HTML tags)

There are several rules.(not all markdown, ex: There is no heading2, heading3 rules, etc)

Reason -
<blockquote> is applied to only the first line.
image1 is the sample text, image2 is the replaced text. so the new line tag also was replaced with </blockquote>.
Because of it, when applying the heading1 rule is applied at the next. it will be not applied by the rule regex format.

The reportComment where the new line tag "\n" was changed is from "ReportUtils.buildOptimisticAddCommentReportAction()" function.

and it is from ExpensiMark().replace() function like this:

this function applies the rules.

the order of rules is like this:

'quote' and 'heading1', and it is applied by the order in the for loop.

so the new line tag "\n" was replaced with </blockquote> and it was disappeared. The next rule(heading1) is not applied without the new line tag.
This is the root cause. So I shared the fixed video. It proves my root cause is right.

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

By applying the heading1 style first, we can fix this problem like the shared video. The solution is:
1. reorder the rule

and it should not remove "a new line tag". so updated it simply like that now.

because the modifyTextForQuote function splits text with "\n" tag. if "heading1" rule removed "\n" tag like original rule, then modifyTextForQuote function cannot split lines correctly.

2. And then we update modifyTextForQuote
This function splits text with "\n" tag. after processing something, it adds the tag "\n" again.
But the "heading" style is rendered in <div> tag, in the next line on UI. It always is rendered in a line, and the next component will be placed in the next line (UI side)

So an additional new line(unexpectedly) is added.
To fix this issue, we should consider the adding "\n" tag again in this case.

Finally, we can say that is a problem between analyzing with the new line tag in modifyTextForQuote function(split with "\n" and join with "\n" again) and <h1> rendering approach using <div>

Video1(test orignal issue)
https://drive.google.com/file/d/1eOuNAEi_peBYihpwb5hbl1OjzvKJdFon/view?usp=sharing
Video2(all tests)
https://drive.google.com/file/d/1W0G2M_mPHwImeS5lNQUXv9mQjNNxNsFT/view?usp=sharing

What alternative solutions did you explore? (Optional)

we need to update the heading1 rule, considering </blockquote>. because the new line tag "\n" is replaced with </blockquote>.
and we should update the "quote" rule(regex), too.

spcheema · 2023-06-29T02:15:00Z

Test screenshot for #21846 (comment)

conorpendergrast · 2023-06-29T09:11:48Z

I reproduced on OSX desktop and iOS Chrome too. Given that this is markdown, we can probably assume it's affecting all platforms.

melvin-bot · 2023-06-29T09:12:36Z

Job added to Upwork: https://www.upwork.com/jobs/~01e498007bd2f0de5b

melvin-bot · 2023-06-29T09:12:40Z

Current assignee @conorpendergrast is eligible for the External assigner, not assigning anyone new.

melvin-bot · 2023-06-29T09:12:45Z

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

conorpendergrast · 2023-06-29T09:16:05Z

I see this previous discussion, but it doesn't look entirely similar as that was for whitespace on a single line (and this is multi-line, where only one is a block quote)

melvin-bot · 2023-07-14T14:31:42Z

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

melvin-bot · 2023-07-14T14:31:43Z

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

Fix/21846 - Heading after quote is not rendering #22478

If no regressions arise, payment will be issued on 2023-07-21. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-07-14T14:31:45Z

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.
[@conorpendergrast] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

spcheema · 2023-07-15T16:33:42Z

@conorpendergrast @s77rt Please consider following timings for bonus eligibility:

Action	Timing
Issue assigned	2023-07-06T23:43:37Z
`E\Common` PR up for review	2023-07-08T05:31:21Z
C++ approved	2023-07-08T11:25:07Z
PR merged	2023-07-10T20:31:41Z
`E\APP` PR up for review	2023-07-10T22:12:45Z
C++ approved	2023-07-11T05:33:17Z
PR merged	2023-07-12T16:43:13Z

After C++ reviewed & approved, following timeline is waiting for internal reviewer to approve and merge the PR.

Waiting time for E\Common PR approval & merge: 2 days 9 hours 6 minutes 34 seconds
Waiting time for E\APP PR approval & merge: 1 days 11 hours 9 minutes 56 seconds

Total waiting time (internal & merge): 3 days 20hrs approx.

Total time elapsed from issue assigned to second PR merged: 5 days 16 hours 59 minutes 36 seconds

s77rt · 2023-07-15T18:16:29Z

The PR that introduced the bug has been identified: This existed since day one Add h1 markdown expensify-common#468
The offending PR has been commented on: Add h1 markdown expensify-common#468 (comment)
A discussion in #expensify-bugs has been started: I don't think this is needed here, the automatic tests should be enough to prevent such bugs.
Determine if we should create a regression test for this bug: No

conorpendergrast · 2023-07-21T23:20:12Z

I will check in on is one, and confirm before paying. I'm OoO on Monday and Tuesday, and will be back to this on Wednesday 26th with confirmation

melvin-bot · 2023-07-25T20:11:02Z

@cead22, @conorpendergrast, @s77rt, @spcheema Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

cead22 · 2023-07-26T00:29:04Z

Not overdue per @conorpendergrast 's update above

conorpendergrast · 2023-07-26T08:26:37Z

@cead22 and I agree with an exception here as the C and C+ acted with urgency.

Everyone has been paid accordingly, and contracts ended. We're all done, thanks!

spcheema · 2023-07-26T08:28:22Z

Thanks @conorpendergrast @cead22 and @s77rt

kbecciv added Daily KSv2 NewFeature Something to build that is a new item. Bug Something is broken. Auto assigns a BugZero manager. labels Jun 28, 2023

melvin-bot bot assigned conorpendergrast and lschurr Jun 28, 2023

melvin-bot bot added Weekly KSv2 and removed Daily KSv2 Weekly KSv2 labels Jun 28, 2023

melvin-bot bot assigned shawnborton Jun 28, 2023

melvin-bot bot added the Daily KSv2 label Jun 29, 2023

conorpendergrast removed the NewFeature Something to build that is a new item. label Jun 29, 2023

conorpendergrast unassigned lschurr and shawnborton Jun 29, 2023

conorpendergrast added the External Added to denote the issue can be worked on by a contributor label Jun 29, 2023

melvin-bot bot changed the title ~~Web - Heading after quote is not rendering~~ [$1000] Web - Heading after quote is not rendering Jun 29, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 29, 2023

melvin-bot bot assigned s77rt Jun 29, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Jul 14, 2023

melvin-bot bot changed the title ~~[$1000] Web - Heading after quote is not rendering~~ [HOLD for payment 2023-07-21] [$1000] Web - Heading after quote is not rendering Jul 14, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Jul 14, 2023

s77rt mentioned this issue Jul 15, 2023

Add h1 markdown Expensify/expensify-common#468

Merged

melvin-bot bot mentioned this issue Jul 17, 2023

[Snyk] Fix for 3 vulnerabilities #21426

Closed

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 20, 2023

melvin-bot bot added the Overdue label Jul 24, 2023

melvin-bot bot removed the Overdue label Jul 26, 2023

conorpendergrast closed this as completed Jul 26, 2023

MelvinBot mentioned this issue Jan 29, 2024

[Snyk] Fix for 25 vulnerabilities #35341

Closed

50 tasks

melvin-bot bot mentioned this issue Jan 29, 2024

[Snyk] Fix for 25 vulnerabilities #35342

Closed

MelvinBot mentioned this issue Feb 12, 2024

[Snyk] Security upgrade @storybook/react from 6.5.10 to 7.0.0 #36331

Closed

50 tasks

melvin-bot bot mentioned this issue Feb 12, 2024

[Snyk] Security upgrade @storybook/react from 6.5.10 to 7.0.0 #36332

Closed

MelvinBot mentioned this issue Mar 14, 2024

[Snyk] Fix for 3 vulnerabilities #38311

Closed

50 tasks

melvin-bot bot mentioned this issue Mar 14, 2024

[Snyk] Fix for 3 vulnerabilities #38312

Closed

MelvinBot mentioned this issue Mar 22, 2024

[Snyk] Security upgrade @storybook/react from 6.5.10 to 7.0.0 #38809

Closed

50 tasks

melvin-bot bot mentioned this issue Mar 22, 2024

[Snyk] Security upgrade @storybook/react from 6.5.10 to 7.0.0 #38810

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2023-07-21] [$1000] Web - Heading after quote is not rendering #21846

[HOLD for payment 2023-07-21] [$1000] Web - Heading after quote is not rendering #21846

kbecciv commented Jun 28, 2023 •

edited by conorpendergrast

Loading

melvin-bot bot commented Jun 28, 2023

melvin-bot bot commented Jun 28, 2023

melvin-bot bot commented Jun 28, 2023 •

edited by conorpendergrast

Loading

melvin-bot bot commented Jun 28, 2023

spcheema commented Jun 28, 2023 •

edited by cead22

Loading

spcheema commented Jun 28, 2023 •

edited

Loading

code0406 commented Jun 29, 2023 •

edited

Loading

spcheema commented Jun 29, 2023

conorpendergrast commented Jun 29, 2023 •

edited

Loading

melvin-bot bot commented Jun 29, 2023

melvin-bot bot commented Jun 29, 2023

melvin-bot bot commented Jun 29, 2023

conorpendergrast commented Jun 29, 2023

melvin-bot bot commented Jul 14, 2023

melvin-bot bot commented Jul 14, 2023 •

edited by conorpendergrast

Loading

melvin-bot bot commented Jul 14, 2023

spcheema commented Jul 15, 2023 •

edited

Loading

s77rt commented Jul 15, 2023

conorpendergrast commented Jul 21, 2023

melvin-bot bot commented Jul 25, 2023

cead22 commented Jul 26, 2023

conorpendergrast commented Jul 26, 2023

spcheema commented Jul 26, 2023

[HOLD for payment 2023-07-21] [$1000] Web - Heading after quote is not rendering #21846

[HOLD for payment 2023-07-21] [$1000] Web - Heading after quote is not rendering #21846

Comments

kbecciv commented Jun 28, 2023 • edited by conorpendergrast Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Jun 28, 2023

melvin-bot bot commented Jun 28, 2023

melvin-bot bot commented Jun 28, 2023 • edited by conorpendergrast Loading

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Jun 28, 2023

spcheema commented Jun 28, 2023 • edited by cead22 Loading

Proposed solution

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

Output generated for a given input

What alternative solutions did you explore? (Optional)

spcheema commented Jun 28, 2023 • edited Loading

code0406 commented Jun 29, 2023 • edited Loading

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)

spcheema commented Jun 29, 2023

conorpendergrast commented Jun 29, 2023 • edited Loading

melvin-bot bot commented Jun 29, 2023

melvin-bot bot commented Jun 29, 2023

melvin-bot bot commented Jun 29, 2023

conorpendergrast commented Jun 29, 2023

melvin-bot bot commented Jul 14, 2023

melvin-bot bot commented Jul 14, 2023 • edited by conorpendergrast Loading

melvin-bot bot commented Jul 14, 2023

spcheema commented Jul 15, 2023 • edited Loading

s77rt commented Jul 15, 2023

conorpendergrast commented Jul 21, 2023

melvin-bot bot commented Jul 25, 2023

cead22 commented Jul 26, 2023

conorpendergrast commented Jul 26, 2023

spcheema commented Jul 26, 2023

kbecciv commented Jun 28, 2023 •

edited by conorpendergrast

Loading

melvin-bot bot commented Jun 28, 2023 •

edited by conorpendergrast

Loading

spcheema commented Jun 28, 2023 •

edited by cead22

Loading

spcheema commented Jun 28, 2023 •

edited

Loading

code0406 commented Jun 29, 2023 •

edited

Loading

conorpendergrast commented Jun 29, 2023 •

edited

Loading

melvin-bot bot commented Jul 14, 2023 •

edited by conorpendergrast

Loading

spcheema commented Jul 15, 2023 •

edited

Loading