[HOLD for payment 2023-05-25] [$1000] Line break before heading is missing in editing mode

[kavimuru]

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

---

Action Performed:

1. Go to any chat
2. Send a new comment

test

# heading

3. Edit the comment and notice the text in editing composer is

test
# heading

Expected Result:

The line break before heading shouldn't be missing

Actual Result:

The line break before heading is missing

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.5-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

Recording.363.mp4

Screen.Recording.2023-04-25.at.10.07.09.AM.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ea44f382867687f1
• Upwork Job ID: 1651431771500584960
• Last Price Increase: 2023-04-27

[MelvinBot]

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

[MelvinBot]

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

[dukenv0307]

Proposal

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

Line break before heading is missing in editing mode

What is the root cause of that problem?

Draft message is display by htmlToMarkdown function. htmlToMarkdown function use htmlToMarkdownRules to convert html tag to mark down

App/src/pages/home/report/ReportActionItemMessageEdit.js

Line 85 in f3fae40

const draftMessage = parser.htmlToMarkdown(this.props.draftMessage).trim();

And htmlToMarkdownRules in ExpensiMark lib has a rule for h1 tag:

name: 'heading1',
regex: /\s*<(h1)(?:"[^"]*"|'[^']*'|[^'">])*>(.*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))\s*/gi,
replacement: '[block]# $2[block]',

With this regex it replace \s*<h1> by [block]#. So all line break before heading is replaced by one line break.

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

Replace regex rule for h1 in htmlToMarkdownRules by /<(h1)(?:"[^"]*"|'[^']*'|[^'">])*>(.*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi

What alternative solutions did you explore? (Optional)

NA

Result

Screencast.2023-04-26.17.17.42.mp4

[kadiealexander]

Was able to reproduce:

2023-04-27_15-38-08.mp4

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[eh2077]

Proposal

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

Line break before heading is missing in editing mode

What is the root cause of that problem?

When editing a comment, we call method htmlToMarkdown to convert draftMessage which is equal to message[0].html to markdown, see

App/src/pages/home/report/ReportActionItemMessageEdit.js

Line 85 in 0262812

const draftMessage = parser.htmlToMarkdown(this.props.draftMessage).trim();

The initial draft message

test<br /><h1>heading</h1>

is converted to

test\n<h1>heading</h1>

by applying newline rule
and then it's converted into

test[block]# heading[block]

by applying html-heading1-to-markdown-heading rule.
And finally the first special separator [block] is converted to newline \n in method replaceBlockWithNewLine
So we get following text in the editing composer

test
# heading

Based on the above analysis, the root cause is that the line break before heading is eliminated in html-heading1-to-markdown-heading rule.
The regex

regex: /\s*<(h1)(?:"[^"]*"|'[^']*'|[^'">])*>(.*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))\s*/gi,

matches the \n before <h1> and eliminates it in the replacement function.

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

To fix this issue, we should retain line break before heading1. To achieve this, we should match whitespaces but not newlines before string <h1>.
The current prefix regex \s*

We can replace the prefix regex \s* with [^\S\r\n]* to skip newlines, see SO link about the new regex.

So the new regex for html-heading1-to-markdown-heading rule is

regex: /[^\S\r\n]*<(h1)(?:"[^"]*"|'[^']*'|[^'">])*>(.*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))\s*/gi,

What alternative solutions did you explore? (Optional)

Can we just remove the prefix regex \s*?
I don't think it's a good idea as the method htmlToMarkdown accepts html string as input. So it's possible to have an input that need to remove normal spaces before <h1>, like for html string input

this line has a <h1>heading1</h1> in the middle of the line

which is test case from the lib expensify-common.

[eVoloshchak]

@dukenv0307, I don't think we should remove the \s* prefix, that's essentially reverting Expensify/expensify-common@58f5320

@eh2077, thank's for the analysis and a detailed explanation, I think that's the correct approach.
I did also found a similar issue, could you take a look at this please?

test

# heading

test

is turned into

test

# heading
test

[eh2077]

@eVoloshchak Sure. I think we can fix the issue of removing line break after heading as well if it’s not an intended feature the product team want.

[kadiealexander]

Yeah let's fix that and prevent the automatic removal of line breaks too, please!

[eh2077]

Yeah let's fix that and prevent the automatic removal of line breaks too, please!

Hi @eVoloshchak , as @kadiealexander has confirmed we should fix the removing ending line breaks issue too, I think we can apply the same regex piece to fix both removing line breaks issue together. And I found another reason that we should them is that we apply newline rule before heading rule, so it's obviously buggy to removing starting and ending line breaks of heading.

[eVoloshchak]

@eh2077, sounds good, let's get the PR going
@kadiealexander, could you assign @eh2077 to the job please?

[melvin-bot]

📣 @eh2077 You have been assigned to this job by @kadiealexander!
Please apply to this job in Upwork 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 📖

[melvin-bot]

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:

• [@eVoloshchak] The PR that introduced the bug has been identified. Link to the PR:
• [@eVoloshchak] 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:
• [@eVoloshchak] 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:
• [@eVoloshchak] Determine if we should create a regression test for this bug.
• [@eVoloshchak] 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.
• [@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[kadiealexander]

@eVoloshchak please don't forget the checklist above!

Assigned: May 8th 2023 10:52am (GMT+12)
First approved by C+: May 11th 5:11am (GMT+12)
Merged: May 14th 2023 10:32am (GMT+12)

Offers sent to @eh2077 for job + reporting bonus, and @eVoloshchak for C+.

[kadiealexander]

Reassigning to get this paid tomorrow as I'm OOO.

[melvin-bot]

Triggered auto assignment to @Christinadobrzyn (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[melvin-bot]

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

[kadiealexander]

Thanks @Christinadobrzyn!

[mountiny]

Lets add the bonus as i was at AppJS

[Christinadobrzyn]

Paid out the jobs in Upwork

@eh2077 - reporting bonus + speed bonus + contributor
@eVoloshchak speed bonus + C+

@eVoloshchak - can you let me know about a regression test? Thanks!

[mountiny]

@eVoloshchak can you please follow up on the checklist above

@Christinadobrzyn @eVoloshchak I dont think we need a regression test for this

[kadiealexander]

Thanks for helping with this while I as away @Christinadobrzyn!!

[eVoloshchak]

Will finish the BZ checklist tomorrow
I think the regression test is needed here, this is something that could easily break, will propose a solution

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: Add h1 markdown expensify-common#468

• 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/468/files#r1212251731

• 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: This is a typical markdown issue, we already have an item in the reviewer checklist to catch similar issues sooner If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.

• Determine if we should create a regression test for this bug. Thinking about it again, we don't need a regression test I think. We already have unit tests for this, no need to have a redundant manual test for something that's already automated

[Christinadobrzyn]

Sounds good! Thanks for clarifying about the regression test @eVoloshchak - I think this can be closed as complete. right?

cc @mountiny

[kadiealexander]

I agree @Christinadobrzyn, @mountiny if you disagree please reopen.