[HOLD for payment 2023-04-27] [$1000] App crashes on editing and deleting the emoji messages on #admins on Android in an Offline mode

[kavimuru]

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:

1. Go to android app and turn off your wifi
2. Go to any admins room of any workspace
3. Send a message with only emojis -> 3-4 emojis (do not include text)
4. Long press the message and immediately click on edit message and add one emoji
5. Click on save
6. Now immediately delete the message and notice that the app crashes

Expected Result:

App should not crash on deleting the edited messages only with emojis on Android

Actual Result:

App crashes on deleting the edited messages with only emojis on Android offline

Workaround:

unknown

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:
Reproducible in staging?: needs reproduction
Reproducible in production?: needs reproduction
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

crash.mp4

Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1680859522702889

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01aafa0e43a1c87311
• Upwork Job ID: 1646091515798622208
• Last Price Increase: 2023-04-12

[MelvinBot]

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

[alexpensify]

This one is wild because after I enable cell/wifi service, then the edited emoji list from the offline is there.

[MelvinBot]

Triggered auto assignment to @techievivek (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[alexpensify]

@techievivek - Assigning to engineering to get feedback on how we want this to function before assigning External.

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[tienifr]

Proposal

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

When we send a message in offline mode, then immediately delete it, the app crashes.
I think we should update the bug description, since it's happening everywhere (not just #admin channel, but also any chat which was not preloaded in Onyx), and with any text being sent (not only emojis)

What is the root cause of that problem?

First, let's look at the message deletion logic in Report.js

It contains a logic to check if the item being deleted is the last visible message, then it would search for previous message to update the latest message in LHN. This logic calls the getLastVisibleMessageText() function.

Now let's look at the getLastVisibleMessageText() function

App/src/libs/ReportActionsUtils.js

Lines 145 to 158 in bcada91

	function getLastVisibleMessageText(reportID, actionsToMerge = {}) {
	const lastVisibleAction = getLastVisibleAction(reportID, actionsToMerge);
	const message = lodashGet(lastVisibleAction, ['message', 0]);
	if (isReportMessageAttachment(message)) {
	return CONST.ATTACHMENT_MESSAGE_TEXT;
	}
	const htmlText = lodashGet(lastVisibleAction, 'message[0].html', '');
	const parser = new ExpensiMark();
	const messageText = parser.htmlToText(htmlText);
	return String(messageText)
	.replace(CONST.REGEX.AFTER_FIRST_LINE_BREAK, '')
	.substring(0, CONST.REPORT.LAST_MESSAGE_TEXT_MAX_LENGTH);
	}

Notice the getLastVisibleAction() function call. In a normal scenario with network connection, this function would return a list of previous message, where the new latest message could be obtained. The list contains previous chat messages, which also includes a dummy welcome message at the beginning of the chat. For example, in a case where we are online, and we try to send a text to a new chat, then immediately delete it, the result of the getLastVisibleAction() function would be

[{"style": "strong", "text": "__fake__", "type": "TEXT"}]

That's why in online mode, the app always works when we try to delete messages.

Now let's look at the scenario when we try to delete a message in offline mode. When we access a chat which was not preloaded in Onyx, no previous messages were loaded in Onyx, which causes the getLastVisibleAction() function to return [], since it's using allReportActions from Onyx. Because the getLastVisibleMessageText() function use the first item in the array as the latest message, this value would be undefined, which trigger the app crash because the app is expecting the latest message value to have a text field in order to be rendered correctly in LHN. This is the reason that it will work in cases where the messages are already in Onyx, like this

and will crash in cases where the messages are not loaded yet, like this (notice the chat is still loading)

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

To solve this problem, we could add a guard logic in getLastVisibleMessageText() to return '' if the message is undefined, something like

function getLastVisibleMessageText(reportID, actionsToMerge = {}) {
    const lastVisibleAction = getLastVisibleAction(reportID, actionsToMerge);
    const message = lodashGet(lastVisibleAction, ['message', 0]);
+   if (!message)
+      return ''

Furthermore (and optionally), shall we decide for the LHN to not update the previous message if the deleted message is null, we could add another logic inside deleteReportComment() function to only update the last message if it is a valid text

        optimisticReport = {
-           lastMessageText,
            lastVisibleActionCreated,
        };
+      if (lastMessageText !== '') 
+         optimisticReport.lastMessageText = lastMessageText

This will make the LHN to keep the message, instead of deleting it and replace with the default message (i.e the first dummy welcome message). The text will be kept until it's back online again, the user access the report and it will be updated as the actual latest message. Since we are in offline mode, I think there is no proper solution to get the last message which were replaced with our new sent-and-deleted message.

Alternative solutions

Alternatively, in the getLastVisibleMessageText() function, we could add a simple logic to check if message is null or undefined, then we will reassign it to a dummy message, which could be the first dummy welcome message like described above, or something like [Deleted message].

Another way to do it is to add a check logic in the getLastVisibleAction() function to prevent it to return an empty array. If the status is offline and the history is empty, we will append a dummy message

Result

Working well after the fix

https://drive.google.com/file/d/1GGFZKVP3QjHKOSbgwYJUd_UKvXt-uOKb/view?usp=sharing

[MelvinBot]

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

[techievivek]

Not overdue, waiting for @s77rt to take a look at the proposal(s). 😄

[s77rt]

Sorry for the delay, will review asap.

[s77rt]

@tienifr Thanks for the proposal. Your RCA is correct. The suggested fix looks good to me. I just have two minor optimisations to consider while raising the PR:

1. getLastVisibleAction will return -Infinity if visibleActions is empty but this function is supposed to return an object. Let's make an early return with an empty object for such case.
2. Instead of checking for falsy message, let's just default to an empty object on lodashGet.

🎀 👀 🎀 C+ reviewed

cc @techievivek

[tienifr]

@s77rt Thanks for the review!

For your 1st optimization, I agree we should make an early return with empty object to prevent returning undefined when there is no visible actions.

For your 2nd optimization, we should keep the check for falsy message. Returning an empty object will not be enough since the isReportMessageAttachment() function, which use the falsy message, is assuming the message would have text and html field. I don't want to modify this function, so the falsy message check should be kept.

For final confirmation, the changes in the PR would be

1. (Your suggestion) Change default to empty object in lodashGet to prevent falsy message object
2. (Your suggestion + my proposal) Default to an empty object on lodashGet and add a check to return empty string when the aforementioned object is empty
3. (My proposal) Add a logic inside deleteReportComment() function to only update the last message only if it is a valid text (not an empty string). This will prevent the LHN preview text to update when there is no message history.

Thoughts?

[tienifr]

The PR is ready for review: #17450.

[alexpensify]

The PR is still under review but moving along.

[tienifr]

Regression Test Proposal

Bug: App crashes on message deletion in empty chat in Offline mode

Proposed Test Steps:

1. Login with any account
2. Reload the page to make sure all chat data in Onyx is cleared
3. Turn off your internet connection
4. Go to any chat
5. Verify that no message has been loaded in the chat (i.e. skeleton view)
6. Send any message
7. Delete the message
8. Verify that the app does not crash and the message should show strike-through

Screencast.from.14-04-2023.15.49.46.webm

Do we 👍 or 👎

[s77rt]

@tienifr You no longer need to provide regression test proposals. It's a part of C+ process now.

[MelvinBot]

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

[MelvinBot]

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

• Fix: App crashes on message deletion in empty chat in Offline mode #17450

If no regressions arise, payment will be issued on 2023-04-27. 🎊

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

[MelvinBot]

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:
• [@alexpensify] 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.
• [@alexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/277726

[s77rt]

• The PR that introduced the bug has been identified: Use correct lastMessageText for attachments #15182
• The offending PR has been commented on: Use correct lastMessageText for attachments #15182 (comment)
• A discussion in #expensify-bugs has been started: https://expensify.slack.com/archives/C049HHMV9SM/p1682036605310669

---

Regression Test Proposal

1. Open App
2. Login with an old account (had previous chats with other users)
3. Turn off your internet connection
4. Go to any old chat
5. Verify no message is loaded
6. Verify a skeleton loader is shown
7. Send a message
8. Delete that message
9. Verify the message is strike-through
10. Verify the App didn't crash

[techievivek]

The regression test looks good to me. @alexpensify can we add this to testRail. Thanks

[alexpensify]

Done! Following this SO process (https://stackoverflowteams.com/c/expensify/questions/363/364#364), I created the GH to add to testrails.

[alexpensify]

BugZero Checklist is complete-- now waiting for the payment date to start that process.

[alexpensify]

Looks like we are all clear here, I'll work on the payment process tomorrow.

[alexpensify]

I saw this in another GH and want to give it a try here to ensure that the payment is correct.

We have the following payouts. I see there is a 50% bonus because it was assigned here on April 17 and was merged into production on April 20:

Issue reporter: $250 - @priya-zha
Contributor: $1,500 - @tienifr
Contributor+: $1,500- @s77rt

@techievivek or @s77rt - Can you please 👍🏼 if you agree and I'll complete the next steps in Upwork. Thank you!

[s77rt]

@alexpensify It's actually $1,500 not $2,000.

BTW, The bonus timeline is from contributor being assigned to PR getting merged (and not necessary deployed to production). So it's from April 17th to April 19th.

[alexpensify]

@s77rt - Thank you for checking my numbers. I've updated the amounts in my comment. Also, thank you for clarifying it's not production but merging. I was looking at the merge action but said production. 🤕 I'll work on these payments in Upwork.

[alexpensify]

Alright, all payments have been prepared in Upwork and are pending approval from the contributors. I'll finish the process as soon as everyone signs off in Upwork. Thanks!

[alexpensify]

@tienifr - please accept the job in Upwork so I can complete the payment process, thank you.

[tienifr]

@alexpensify hi, I tried to accept the offer but it says the offer has been withdrawn, can you help double check?

Thanks!

[alexpensify]

@tienifr - Not sure what happened in Upwork, but I sent over a new offer. Please accept and I can complete the payment. Thanks!

[tienifr]

@alexpensify accepted, thanks!

[alexpensify]

Closing because the project is done and everyone has been paid! Great work here!