[HOLD for payment 2023-12-29] [$500] Chat–Name of message owner disappears from DM for a second when delete first message in Offline

[izarutskaya]

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: v1.4.4-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: Applause-Internal Team
Slack conversation: @

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Log in with any account.
3. Disable the internet connection in the main testing device
4. Choose the first of several messages you already sent back-to-back
5. Verify the chat header with your name appears only on your first message
6. Hover over that first message to view the action menu
7. Click/Tap on the Trash icon to delete the message
8. Go Online

Expected Result:

The message has been deleted

Actual Result:

The name of the message owner disappears from conversation for a second when delete first message in Offline and go back to Online.

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

Bug6292645_1701145916396.The_name_of_the_message_owner_disappears.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012bb87b742d38cd4b
• Upwork Job ID: 1729458938833915904
• Last Price Increase: 2023-11-28
• Automatic offers:
• shubham1206agra | Contributor | 28062142
• tienifr | Contributor | 28062150

Issue Owner

Current Issue Owner: @kevinksullivan

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~012bb87b742d38cd4b

[melvin-bot]

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

[melvin-bot]

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]

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

[AmjedNazzal]

@izarutskaya Can you please check the screenshots/videos section, it seemed to have not uploaded correctly.

[bernhardoj]

Proposal

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

When we send a message, it will show who sent it, and if we send another message within 5 minutes, it will be grouped and the 2nd (or later) message won't show who sent it anymore. If we delete the first message that has that info while offline, coming back offline makes it disappears along with the first message.

What is the root cause of that problem?

The logic to decide whether to show the user info is coming from ReportActionsUtils.isConsecutiveActionMadeByPreviousActor which will check the current and previous action sent time difference.

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

Line 76 in 48e0c1e

displayAsGroup={ReportActionsUtils.isConsecutiveActionMadeByPreviousActor(sortedReportActions, index)}

App/src/libs/ReportActionsUtils.ts

Lines 258 to 261 in 48e0c1e

	// Comments are only grouped if they happen within 5 minutes of each other
	if (new Date(currentAction.created).getTime() - new Date(previousAction.created).getTime() > 300000) {
	return false;
	}

When we go online, the first message that we optimistically delete disappears thanks to OfflineWithFeedback, but the first message report action object actually still exists (assuming the delete action API isn't complete yet) in the Onyx.

So, when we compare the 2nd message with the 1st message sent time, it's obvious that the user info won't show. After the API completes, the 1st message is deleted from the Onyx and the 2nd message is now the 1st message and because the previous action is CREATED action, the user info is shown.

But this is not the real root cause. If we see the logic on how we retrieve the previous action, we already skip any action that has a pending delete while ONLINE.

App/src/libs/ReportActionsUtils.ts

Lines 226 to 237 in 48e0c1e

	function findPreviousAction(reportActions: ReportAction[] | null, actionIndex: number): OnyxEntry<ReportAction> {
	if (!reportActions) {
	return null;
	}
	for (let i = actionIndex + 1; i < reportActions.length; i++) {
	// Find the next non-pending deletion report action, as the pending delete action means that it is not displayed in the UI, but still is in the report actions list.
	// If we are offline, all actions are pending but shown in the UI, so we take the previous action, even if it is a delete.
	if (isNetworkOffline || reportActions[i].pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {
	return reportActions[i];
	}
	}

The problem is, that we memorized ReportActionsListItemRenderer

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

Line 93 in 48e0c1e

export default memo(ReportActionsListItemRenderer);

which only rerenders when the props change, but the network status (isNetworkOffline) is coming from the util file itself,

App/src/libs/ReportActionsUtils.ts

Lines 49 to 53 in 48e0c1e

	let isNetworkOffline = false;
	Onyx.connect({
	key: ONYXKEYS.NETWORK,
	callback: (val) => (isNetworkOffline = val?.isOffline ?? false),
	});

so even when the network status changes, the component won't rerender.

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

Subscribe the network status in ReportActionsListItemRenderer and pass it to ReportActionsUtils.isConsecutiveActionMadeByPreviousActor > ReportActionsUtils.findPreviousAction.

const {isOffline} = useNetwork();
ReportActionsUtils.isConsecutiveActionMadeByPreviousActor(actions, index, isOffline);

[tienifr]

Proposal

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

Chat–Name of message owner disappears from DM for a second when delete first message in Offline

What is the root cause of that problem?

we decide to display the chat-name based on displayAsGroup here

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

Line 76 in 9b1f465

displayAsGroup={ReportActionsUtils.isConsecutiveActionMadeByPreviousActor(sortedReportActions, index)}

in

App/src/libs/ReportActionsUtils.ts

Line 234 in 48e0c1e

if (isNetworkOffline || reportActions[i].pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {

we already have the logic to check isNetworkOffline, but isConsecutiveActionMadeByPreviousActor function is not called when the users go online because in ReportActionsListItemRenderer we did not listen for network change

-> displayAsGroup is not updated immediately. When API DeleteComment calls successfully that makes sortedReportActions get changed -> ReportActionsListItemRenderer is re-rendered -> displayAsGroup is updated

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

We don't need to pass whole sortedReportActions to ReportActionsListItemRenderer because this component is used to render only 1 item and we just need it to get displayAsGroup -> it can cause this component re-renders unexpectedly.

Instead of that we should pass

displayAsGroup={ReportActionsUtils.isConsecutiveActionMadeByPreviousActor(sortedReportActions, index)}

in

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

Line 378 in 0c78c5b

<ReportActionsListItemRenderer

and remove this line

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

Line 384 in 0c78c5b

sortedReportActions={sortedReportActions}

then use displayAsGroup in here

displayAsGroup={displayAsGroup}

For now, we can make sure displayAsGroup will be updated after uses go online because we already listen for network change in ReportActionsList

Result

Screen.Recording.2023-11-28.at.23.39.15.mov

[Santhosh-Sellavel]

@tienifr's proposal LGTM!
C+ Reviewed
🎀 👀 🎀

[melvin-bot]

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

[bernhardoj]

@Santhosh-Sellavel hi, I think the best practice here is to pass the network status (or any onyx data) to the function. It will ensure every function usage is always reactive to the network changes instead of trying to guess what Onyx we need to subscribe to on the component.

Correct example:

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

Line 325 in ef3d8cd

() => ReportActionsUtils.getFirstVisibleReportActionID(sortedReportActions, isOffline) === currentUnreadMarker,

It just happens that the ReportActionsList has the network status hooks.

@tienifr probably has a performance improvement, but that's not what this issue is about. We should stick to the initial scope of the issue.

We previously had a lot of reactive issues in the past because of relying on Onyx.connect inside of util file which I raised here

cc: @Julesssss

[tienifr]

Currently, we're relying on Onyx.connect in many util functions. When data from Onyx is updated, it will be updated in util file first, then updated in react component, so when the component re-render, we can make sure we use the correct data in util function.

[Julesssss]

Thanks both. @bernhardoj I see your comment was mostly ignored, so I'm going to discuss it internally.

I will use this private link to bookmark the conversation

[Santhosh-Sellavel]

@Julesssss Any update?

[Julesssss]

It was discussed here, and it looks like we're aligned on the proposal to not use Onyx.connect. Lets pass required data to components with withOnyx.

[kbecciv]

@AmjedNazzal Re-uploaded the video.

[tienifr]

It was discussed here, and it looks like we're aligned on the proposal to not use Onyx.connect. Lets pass required data to components with withOnyx.

Whatever the solution is, I think we still need to remove sortedReportActions in ReportActionsListItemRenderer. We should calculate displayAsGroup in here. In ReportActionsList we already listen for network change so we don't need to pass isOffline to it, but if we don't want to lean on isOffline from Onyx.connect, we can pass it from components with withOnyx. What do you think @Julesssss ?

[Julesssss]

@tienifr good spot, that sounds reasonable.

[tienifr]

PR ready for review #33207.

[Julesssss]

PR merged, awaiting payment

[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.4.15-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: Message header disappears when delete message in offline #33207

If no regressions arise, payment will be issued on 2023-12-29. 🎊

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

For reference, here are some details about the assignees on this issue:

• @shubham1206agra requires payment automatic offer (Contributor)
• @tienifr requires payment automatic offer (Contributor)

[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:

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

[melvin-bot]

@Julesssss, @kevinksullivan, @shubham1206agra, @tienifr Huh... This is 4 days overdue. Who can take care of this?

[shubham1206agra]

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:

• [@shubham1206agra] The PR that introduced the bug has been identified. Link to the PR: [NoQA] Memoize expensive calls in ReportActionsList renderItem #27957
• [@shubham1206agra] 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/App/pull/27957/files#r1440618886
• [@shubham1206agra] 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: No need as they are optimizing performance and forgot to add offline tests.
• [@shubham1206agra] Determine if we should create a regression test for this bug. Yes
• [@shubham1206agra] 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.

1. Go to App
2. Send someone messages back-to-back.
3. Disable the internet connection.
4. Choose the first of several messages you already sent back-to-back
5. Verify the chat header with your name appears only on your first message
6. Hover over that first message to view the action menu
7. Click/Tap on the Trash icon to delete the message
8. Go Online

• [@kevinksullivan] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

@Julesssss, @kevinksullivan, @shubham1206agra, @tienifr Still overdue 6 days?! Let's take care of this!

[shubham1206agra]

@kevinksullivan Bump for the payment here

[melvin-bot]

@Julesssss, @kevinksullivan, @shubham1206agra, @tienifr 10 days overdue. Is anyone even seeing these? Hello?

[Julesssss]

Not overdue, just awaiting payment

[melvin-bot]

@Julesssss, @kevinksullivan, @shubham1206agra, @tienifr 12 days overdue. Walking. Toward. The. Light...

[Julesssss]

Waiting for payment

[kevinksullivan]

Thanks for your patience, everyone! Paid out.