[HOLD for payment 2023-05-25] [$1000] Space appears for a brief moment in the LHN message before correcting it if a codeblock has a space at the beginning

[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. Open the app
2. Open any report
3. Write code block with space in beginning eg: Hello test
4. Send the message and observe in LHN that app displays the text with space before and after few seconds, removes the text

Expected Result:

App should not display space before text in LHN

Actual Result:

App displays space before text in LHN for few seconds when we send code block with space before text in it.

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.4
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

space.before.text.in.LHN.for.few.seconds.mp4

Recording.345.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1682331727011769

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016119f8050196b168
• Upwork Job ID: 1651553553942560768
• Last Price Increase: 2023-05-08

[MelvinBot]

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

[sheelyeng]

Hello, I can fix this issue.

[MelvinBot]

📣 @sheelyeng! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[sheelyeng]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/sheelyeng

[sheelyeng]

Proposal

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

• Leading space appears for a brief moment like 0.5 sec in the LHN message when you are adding a new codeblock comment starts with one or more spaces. Here 0.5 sec is actually the duration of API processing. When the API returns response 200, then it's corrected automatically.

What is the root cause of that problem?

• The main reason of this issue is htmlDecode method does not remove the spaces while decoding the html code string to get the text from there. But in API response, it returns the comment string without spacing.

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

• We can resolve the issue by adding a .trim() method when we render the LHN message. This will be just one line change in code. The unit testing file also should be updated accordingly to make sure the solution is working properly.

What alternative solutions did you explore? (Optional)

• The another way is to add .trim() method to the places where it's related to generate the LHN text message, but it needs some updates on several places.

[honnamkuan]

Posting proposal early as per new guidelines

Proposal

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

When LHN last message starts with code block markdown that has leading spaces within the code block e.g. " hello", the text with leading spaces are displayed for a brief moment before the text rendering get updated to text without the leading spaces.
Same issue happens right after adding a new comment, as well as editing last comment.

What is the root cause of that problem?

When we construct the optimisticReport.lastMessageText in
Report.js#addActions

App/src/libs/actions/Report.js

Lines 205 to 212 in 6fb0969

	const lastCommentText = ReportUtils.formatReportLastMessageText(lastAction.message[0].text);
	const optimisticReport = {
	lastVisibleActionCreated: currentTime,
	lastMessageText: Str.htmlDecode(lastCommentText),
	lastActorEmail: currentUserEmail,
	lastReadTime: currentTime,
	};

and
Report.js#editReportComment

App/src/libs/actions/Report.js

Lines 925 to 928 in 6fb0969

	const lastMessageText = ReportUtils.formatReportLastMessageText(reportComment);
	const optimisticReport = {
	lastMessageText: Str.htmlDecode(lastMessageText),
	};

We uses ReportUtils.formatReportLastMessageText to perform formatting for the lastMessageText and then Str.htmlDecode it before saving it to Onyx.

App/src/libs/ReportUtils.js

Lines 489 to 493 in 6fb0969

	function formatReportLastMessageText(lastMessageText) {
	return String(lastMessageText)
	.replace(CONST.REGEX.AFTER_FIRST_LINE_BREAK, '')
	.substring(0, CONST.REPORT.LAST_MESSAGE_TEXT_MAX_LENGTH);
	}

Current ReportUtils.formatReportLastMessageText logic:

• removes text from second line onwards
• and reduce the text be within max length of 200.

=> Note that there is no trimming of leading and trailing spaces

What happened is that existing optimisticReport.lastMessageText construction logic will results in " hello", which get displayed immediately in LHN.
Moments later, when Pusher event comes in, the lastMessageText from server actually has the leading spaces trimmed off, the LHN is then updated with lastMessageText without the leading spaces

e.g. pusher event

[
  {
    "onyxMethod": "merge",
    "key": "report_1346288692300822",
    "value": {
      "reportID": "1346288692300822",
      "maxSequenceNumber": 15,
      "lastVisibleActionCreated": "2023-04-27 08:12:55.117",
      "lastReadTime": "2023-04-27 08:12:55.117",
      "lastMessageText": "hello",
      "lastActorEmail": "[email protected]"
    }
  }
]

Therefore, the root cause is inconsistency between formatting/construction rule between the App and backend server.

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

First, we should update the construction logic of optimisticReport.lastMessageText to include trimming of leading and trailing spaces to align with backend server, by modifying

App/src/libs/ReportUtils.js

Lines 489 to 493 in 6fb0969

	function formatReportLastMessageText(lastMessageText) {
	return String(lastMessageText)
	.replace(CONST.REGEX.AFTER_FIRST_LINE_BREAK, '')
	.substring(0, CONST.REPORT.LAST_MESSAGE_TEXT_MAX_LENGTH);
	}

and

App/src/libs/ReportActionsUtils.js

Lines 161 to 164 in 6fb0969

	return String(messageText)
	.replace(CONST.REGEX.AFTER_FIRST_LINE_BREAK, '')
	.substring(0, CONST.REPORT.LAST_MESSAGE_TEXT_MAX_LENGTH);
	}

to include trim().

However, that alone is not sufficient, because the input text can contains space in html encoded format e.g.   /   which does not get handled by trim().

To achieve what we need, we also need to ensure decoding happen before trim (currently htmlDecode is done at last step):

    return Str.htmlDecode(String(lastMessageText))
        .replace(CONST.REGEX.AFTER_FIRST_LINE_BREAK, '')
        .substring(0, CONST.REPORT.LAST_MESSAGE_TEXT_MAX_LENGTH)
        .trim();

Since htmlDecode is changed to be done within formatReportLastMessageText, we can remove usage ofStr.htmlDecode from

App/src/libs/actions/Report.js

Line 209 in 6fb0969

lastMessageText: Str.htmlDecode(lastCommentText),

and

App/src/libs/actions/Report.js

Line 927 in 6fb0969

lastMessageText: Str.htmlDecode(lastMessageText),

as they are no longer required

Test results:

Screen.Recording.2023-05-01.at.9.43.26.PM.mov

What alternative solutions did you explore? (Optional)

Add trim() to text displayed in LHN list

App/src/components/LHNOptionsList/OptionRowLHN.js

Line 192 in 6fb0969

{optionItem.alternateText}

which would works visually, but that approach does not resolve the discrepancies between optimistic data and actual pushed events from server, so I would avoid that.

[sheelyeng]

@honnamkuan , I think, logically there is no difference whether we add trim(). Because it will be rendered before we get response from Pusher event. Furthermore, your solution does not work. I added a few comments to make your solution working.

In case you want to add trim() method on other places, then you should add it several places not only for this one formatReportLastMessageText.

Additional places you should add the trim() method

• when you are adding a new comment
  

  App/src/libs/actions/Report.js

  Line 209 in 6fb0969

  lastMessageText: Str.htmlDecode(lastCommentText),

• when you are editing the last comment
  You also should add it here as well: (This is for editing an existing last comment)
  

  App/src/libs/actions/Report.js

  Line 927 in 6fb0969

  lastMessageText: Str.htmlDecode(lastMessageText),

• when you removed the last comment
  

  App/src/libs/ReportActionsUtils.js

  Line 160 in 6fb0969

  const messageText = parser.htmlToText(htmlText);

But as you can see we need to add the trim() method at least more than 2 places.
That's the reason I suggested adding the trim() method here:

App/src/components/LHNOptionsList/OptionRowLHN.js

Line 192 in 6fb0969

{optionItem.alternateText}

There would be a better solution to make the repo more professional.
So feel free to correct me if I am wrong. It would be a great chance for me to learn more about the project.

[honnamkuan]

@sheelyeng I believe my solution works well, I have added a Test results: section with video recording in my proposal.

No doubt your solution of trimming in the UI works as well.

It is just that my preference would be to ensure Onyx data stays consistent in both optimistic and success data to avoid surprises in the future, even if it involves making more code changes.

I guess if this issue is open to External, it would be up to the C+ reviewers to review and decide which proposal approach they prefer.

[miljakljajic]

Reproducible for me:
https://user-images.githubusercontent.com/28715117/234852522-be8c7b26-2f61-43ed-910a-3a39c71d6661.mp4

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~016119f8050196b168

[MelvinBot]

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

[MelvinBot]

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

[eh2077]

Proposal

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

Space appears for a brief moment in the LHN message before correcting it if a codeblock has a space at the beginning.

What is the root cause of that problem?

When sending a code block with leading spaces

```   test```

We build optimistic report data through method buildOptimisticAddCommentReportAction and parse the markdown code fence into html through method ExpensiMark.replace. In the codeFence-to-html rule, we replace whitespace with html code  .

In method buildOptimisticAddCommentReportAction, the code block is translated into message html string

<pre>&#32;&#32;&#32;test</pre>

and the message html string is converted to message text string here through method ExpensiMark.htmlToText.

   test

Then we format optimistic data of lastMessageText for LHN here

App/src/libs/actions/Report.js

Lines 205 to 209 in 8d661d2

	const lastCommentText = ReportUtils.formatReportLastMessageText(lastAction.message[0].text);
	const optimisticReport = {
	lastVisibleActionCreated: currentTime,
	lastMessageText: Str.htmlDecode(lastCommentText),

by extracting the first row of message text,    test, and decode it with Str.htmlDecode to get lastMessageText, ' test'.

So before backend return the trimmed lastMessageText, the LHN message will display ' test'.

When editing the code block message, we also convert message html string into text string, extract the first line and decode it with Str.htmlDecode to get optimistic lastMessageText. So same issue with adding comment.

Based on the above analysis, the root cause of this issue can be that, in method ExpensiMark.htmlToText, we don't replace   back to space and trim the text before return.

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

To fix this issue, we can replace   back to space and trim the text before return in method ExpensiMark.htmlToText.

We can add a new rule to htmlToTextRules to replace   to space, like

{
    name: 'replaceClassicSpaces',
    regex: / /gi,
    replacement: ' ',
}

And trim the text before return here.

What alternative solutions did you explore? (Optional)

N/A

[MelvinBot]

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

[robertjchen]

Thanks for the proposals! I like @honnamkuan 's approach in addressing it at the root formatReportLastMessageText when it goes into Onyx to keep things consistent (It looks like we do indeed trim in the backend).

While I understand @sheelyeng 's point about doing it at the UI level, I think the former approach would help us avoid bugs in the future.

What do you think @rushatgabhane ? 🙏

[rushatgabhane]

hi @robertjchen i agree.

@honnamkuan's proposal will prevent any surprises in the future.
Let's go with it

[eh2077]

Hi @robertjchen @rushatgabhane , I think my proposal #17896 (comment) explains the root cause better and provides a more fundamental solution. Can I get a feedback from you on my proposal? Thank you!

[honnamkuan]

@miljakljajic thank you. I accepted the offer.
I guess this issue can be assigned to me now and the Help Wanted label can be taken off

[melvin-bot]

📣 @honnamkuan You have been assigned to this job by @robertjchen!
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]

📣 @gabrialva! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[honnamkuan]

@robertjchen @rushatgabhane the PR is ready for review, thanks.

[miljakljajic]

I'll be OOO at EC3 and then taking some time off for a wedding. I'll reassign to another BZ member to process the payments. Contracts have been extended, just need to pay when the time comes!

[melvin-bot]

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

[rushatgabhane]

PR merged yesterday

[melvin-bot]

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

[melvin-bot]

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

• align lastMessageText formatting with backend by trimming leading and trailing spaces #18822

If no regressions arise, payment will be issued on 2023-05-25. 🎊

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]

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:

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

[sakluger]

It looks like this was merged 2-3 biz days after assignment, so it qualifies for the efficiency bonus 🎉

@rushatgabhane can you please complete the BugZero checklist? Once you do, I'll complete payments thorugh upwork.

[rushatgabhane]

1. The PR that introduced the bug has been identified. Link to the PR: Not applicable. This is a simple whitespace trim fix that was never implemented on the frontend.

2. 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: N.A.

3. 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.

4. Determine if we should create a regression test for this bug. - No

5. 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 - N.A.

[rushatgabhane]

@sakluger completed the checklist! Thanks!
Please hire on account - https://www.upwork.com/freelancers/~01212e8255b02ae924

[sakluger]

Thanks @rushatgabhane, and thanks everyone for the quick work on this one! Closing out.