[Awaiting checklist] [$250] IOU - App crashed after deleting IOU and relaunching app

[lanitochka17]

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.75-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: N/A
Issue reported by: Applause - Internal Team

Action Performed:

1. Open the app and log in
2. Open any 1:1 chat
3. Tap + > Submit expense
4. Open the created IOU and delete it
5. Navigate back to LHN
6. Open the same chat again
7. Kill the app and open it again

Expected Result:

The app opens and LHN is displayed

Actual Result:

The app crashes

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

Add any screenshot/video evidence

Bug6489440_1716471167435.video_2024-05-23_09-31-16.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0160e20b7c9a05d3c9
• Upwork Job ID: 1793688592596738048
• Last Price Increase: 2024-05-30
• Automatic offers:
• ahmedGaber93 | Reviewer | 102740724
• abzokhattab | Contributor | 102740725

Issue Owner

Current Issue Owner: @ahmedGaber93

[melvin-bot]

Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@twisterdotcom FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #wave-collect - Release 1

[abzokhattab]

Proposal

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

App crashed after deleting IOU and relaunching app

What is the root cause of that problem?

after removing the IOU, the returned report action of the deleted IOU here is null so the null value cannot be deconstructed thus the app crashes:

App/src/hooks/useReportIDs.tsx

Lines 77 to 94 in 06f4dab

	const reportActionsSelector = (reportActions: OnyxEntry<OnyxTypes.ReportActions>): ReportActionsSelector =>
	(reportActions &&
	Object.values(reportActions).map((reportAction) => {
	const {reportActionID, actionName, errors = [], originalMessage} = reportAction;
	const decision = reportAction.message?.[0]?.moderationDecision?.decision;
	return {
	reportActionID,
	actionName,
	errors,
	message: [
	{
	moderationDecision: {decision},
	},
	] as Message[],
	originalMessage,
	};
	})) as ReportActionsSelector;

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

we need to filter null report actions first. so before the .map we need to add .filter(Boolean)

        Object.values(reportActions) .filter(Boolean).map((reportAction) => {

and do this in other locations if needed inside the ReportUtils.ts file

POC

Screen.Recording.2024-05-23.at.6.42.45.PM.mov

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0160e20b7c9a05d3c9

[melvin-bot]

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

[aksh1t]

Proposal

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

After deleting an IOU and navigating back to the chat screen, the app crashes on restart.

What is the root cause of that problem?

When deleting an IOU, we set its report action id to null here:

App/src/libs/actions/IOU.ts

Lines 5320 to 5326 in 8375abe

	if (shouldDeleteIOUReport) {
	successData.push({
	onyxMethod: Onyx.METHOD.SET,
	key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport?.reportID}`,
	value: null,
	});
	}

This then causes a null check crash here because we are trying to read a null value:

App/src/hooks/useReportIDs.tsx

Line 80 in 8375abe

const {reportActionID, actionName, errors = [], originalMessage} = reportAction;

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

We can check for null values and filter them out before we process them. Something like this:

Object.values(reportActions).filter(action => action != null).map((reportAction) => {

What alternative solutions did you explore? (Optional)

Another thing I explored was to try removing the report id completely from Onyx when deleting, but it seems that's not how Onyx is meant to work. null ing out values is how they are supposed to be deleted, so a null check is the right way to fix this.

[ahmedGaber93]

Reviewing tomorrow

[tienifr]

This seems to be a deep Onyx issue, such null value shouldn't have been there in the first place, filtering as suggested above is workaround.

[ahmedGaber93]

@abzokhattab @aksh1t Thanks for the proposals.

I think we need to include the offending PR in the root cause to make sure we are doing the right solution.

[ahmedGaber93]

Waiting on proposals updates.

[ahmedGaber93]

Waiting on proposals.

[ahmedGaber93]

@tienifr can you dig into the code and find it?

[abzokhattab]

I think the root cause is still valid..

here are my findings:

1. when the user deletes a request, the backend returns set: reportAction_XXXX:null which then sets the report action to the null object.
2. now in current issue, since we get only the values of the actions: Object.values(reportActions) this results in an array with null like this [null] thus filtering out null values in the selector here is the way to go.

we might need to do the same here as well

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[ahmedGaber93]

I do some investigation into onyx, I can't find the offending PR, but I found a few PRs talk about changing the way of removing null values. This issue Expensify/react-native-onyx#553 is for performance purpose related to #42654, but it changes the way that onyx used to remove null values, and it was merged in version 2.0.43. I test this issue with onyx version 2.0.43 and it works fine.

So this issue is not reproduced on the last version of onyx 2.0.43. And based on this comment Expensify/react-native-onyx#554 (review) we are going to bump onyx version on next few days.

@twisterdotcom I think we can hold this for #42654 which will bump onyx version to 2.0.43 that fix this issue.
Also, this issue causes a crash that prevent the user from opening the app, I am not sure if it should be a deploy blocker or not. If it is a deploy blocker and the onyx version bump will take a long time, we can move with the proposal above.

[twisterdotcom]

Great investigation @ahmedGaber93.

[ahmedGaber93]

Bump onyx PR #42772 still not merged.

[melvin-bot]

@twisterdotcom, @ahmedGaber93 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[ahmedGaber93]

Bump onyx PR #42772 still not merged

[mountiny]

Actually sorry, this is not a blocker given this issue existed before, just the RCA changed

But lets CP this as the fix is related to the Onyx update that is in staging.

@abzokhattab are you able to raise the PR quickly?

[abzokhattab]

Yes no problem will work on it tomorrow morning

[abzokhattab]

The PR is ready!

[ahmedGaber93]

PR has been reviewed by C+

[hayata-suenaga]

The PR was merged 🚀

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

• Fix app crash on loading null reportActions #43814

If no regressions arise, payment will be issued on 2024-06-28. 🎊

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

• @ahmedGaber93 requires payment automatic offer (Reviewer)
• @abzokhattab 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:

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

[twisterdotcom]

Payment Summary:

• @ahmedGaber93 paid $250 here (Reviewer)
• @abzokhattab paid $250 here (Contributor)

Waiting on the checklist @ahmedGaber93

[ahmedGaber93]

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:

• [@ahmedGaber93] The PR that introduced the bug has been identified. Link to the PR: N/A
• [@ahmedGaber93] 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
• [@ahmedGaber93] 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
• [@ahmedGaber93] Determine if we should create a regression test for this bug. Yes
• [@ahmedGaber93] 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.

Regression Test Proposal

1. Open any 1:1 chat
2. Tap + > Submit expense
3. Open the created IOU and delete it
4. Navigate back to LHN
5. Open the same chat again
6. Make sure the app is not crashed and the IOU report is deleted successfully.

Do we agree 👍 or 👎