[HOLD for payment 2024-04-05] Android - Chat - When offline, if a user requests money and then deletes the message, the page keeps loading

[kbecciv]

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

---

Version Number: 1.4.56.0
Reproducible in staging?: y
Reproducible in production?: n
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4447619
Issue reported by: Applause - Internal Team

Action Performed:

1. Launch app
2. Tap on a new 1:1 report
3. Go offline
4. Send a message
5. Delete the message
6. Note the message is striked and no skeleton loading in the page
7. Tap plus icon and request money
8. Complete the request money flow
9. Send a message
10. Delete the message
11. Now note the page behaviour

Expected Result:

When offline, after requesting money, if a user sends and deletes a message, the deleted message should be struck through, and the rest of the messages on the page should display without any issues.

Actual Result:

When offline, after requesting money, if a user sends and deletes a message, the deleted message is struck through. However, the above messages and the entire page keep loading, displaying a skeleton.

Workaround:

n/a

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

Add any screenshot/video evidence

Bug6423554_1711133956935.chin.mp4

View all open jobs on GitHub

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @NikkiWines (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[kbecciv]

We think that this bug might be related to #vip-vsb

[NikkiWines]

When testing on dev, this behavior is reproducible for me on web as well. @kbecciv can you confirm if this is specific to Android or a regression across all platforms?

[NikkiWines]

I have a suspicion that this is tied to the changes added by #30269. It also seems like similar behavior to another existing blocker #38781.

@roryabraham looks like you have context for both the PR and the associated blocker, thoughts?

[roryabraham]

Yeah, looks quite possibly like the same root cause. Let's CP and retest.

[bernhardoj]

Proposal

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

The other messages disappear and the skeleton shows when deleting a message.

What is the root cause of that problem?

The other messages disappear because getContinuousReportActionChain only includes some of the messages.

App/src/libs/ReportActionsUtils.ts

Lines 307 to 308 in 191d932

	return sortedReportActions.slice(startIndex, endIndex + 1);
	}

When calculating the endIndex, we check whether the current action previousReportActionID is the same as the previous action previousReportActionID.

App/src/libs/ReportActionsUtils.ts

Lines 283 to 292 in 191d932

	while (
	(endIndex < sortedReportActions.length - 1 && sortedReportActions[endIndex].previousReportActionID === sortedReportActions[endIndex + 1].reportActionID) ||
	!!sortedReportActions[endIndex + 1]?.whisperedToAccountIDs?.length ||
	!!sortedReportActions[endIndex]?.whisperedToAccountIDs?.length ||
	sortedReportActions[endIndex]?.actionName === CONST.REPORT.ACTIONS.TYPE.ROOMCHANGELOG.INVITE_TO_ROOM ||
	sortedReportActions[endIndex + 1]?.actionName === CONST.REPORT.ACTIONS.TYPE.CLOSED ||
	sortedReportActions[endIndex + 1]?.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED
	) {
	endIndex++;
	}

However, a newly optimistic added action doesn't have previousReportActionID yet, so the condition always fails.

The initial value of endIndex (and startIndex) is calculated here by finding the index of the action that the pendingAction value is not add,

App/src/libs/ReportActionsUtils.ts

Line 268 in 191d932

index = sortedReportActions.findIndex((obj) => obj.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD);

and because we delete the first action, the pending action is updated to delete, so the endIndex value is 0.

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

Based on this comment, the reason we check for pending action add is that the action doesn't have the previousReportActionID value yet which we rely on the BE to give us the value.

App/src/libs/ReportActionsUtils.ts

Lines 297 to 298 in 191d932

	// 2. If the first condition fails, it then checks if the previous item has a pendingAction of 'add'.
	// This additional check is to include recently sent messages that might not yet be part of the established sequence.

But as mentioned above, when deleting the message, the pending action value is overwritten. The cause of the issue is the same as this one where we rely on the pending action, but it's overwritten. To overcome the issue, we introduce a new property called isOptimisticAction.

So, the solution is to replace the pending action check with isOptimisticAction.
Sorry, we shouldn't replace it but rather add another check for isOptimisticAction.

App/src/libs/ReportActionsUtils.ts

Line 268 in 191d932

index = sortedReportActions.findIndex((obj) => obj.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD);

index = sortedReportActions.findIndex((obj) => ... || !obj.isOptimisticAction);

App/src/libs/ReportActionsUtils.ts

Line 301 in 191d932

sortedReportActions[startIndex - 1]?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD ||

... ||
sortedReportActions[startIndex - 1]?.isOptimisticAction

What alternative solutions did you explore? (Optional)

Optimistically set previousReportActionID to a new action, but doesn't sound good because it's possible the action list is not in order when the app is still loading or while offline.

[bernhardoj]

The issue still happens, please check my proposal for the solution.

[roryabraham]

@bernhardoj I like your alternate solution better - it seems like optimistic actions could be seamlessly integrated into the chain by setting the previousReportActionID to be the ID of the newest action on a report whenever creating a new one optimistically.

[roryabraham]

doesn't sound good because it's possible the action list is not in order when the app is still loading or while offline

Not sure I'm following the concern here @bernhardoj

[roryabraham]

This will be fixed by #38968. I do think including previousReportActionID in optimistic actions is a better solution, but too big a refactor to CP

[bernhardoj]

Not sure I'm following the concern here @bernhardoj

So, I'm thinking of 2 cases where the optimistic previousReportActionID could be wrong.

1. When you open a chat that the report actions are not loaded yet (empty, 0 length). In this case, the first optimistic comment previousReportActionID would be empty.
2. When you open a chat that has incomplete/outdated report actions

EDIT: Btw, do we really to check for the DELETE pending action too? I think the reason we check for ADD pending action is because the action doesn't have previousReportActionID yet, but the pending action may be overwritten with either DELETE or UPDATE, so we need to rely on isOptimisticAction too.

EDIT 2: I just remember isOptimisticAction is currently only available when adding a message/attachment and task, so we may need to add isOptimisticAction for other user-generated actions that can be deleted, such as money requests (I think money request is the only one left).

[roryabraham]

When you open a chat that the report actions are not loaded yet (empty, 0 length). In this case, the first optimistic comment previousReportActionID would be empty

That's actually a very good point

[roryabraham]

verified that this is fixed on staging, going to close it out now.

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

• [CP Stg] Make getContinuousReportActionChain consider any optimistic action #38968

If no regressions arise, payment will be issued on 2024-04-05. 🎊