[$250] Android - Track expense - "It's not here" page shows up briefly when deleting a track expense

[IuliiaHerets]

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: 9.0.68-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5277524
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

Precondition: Login to the Hybrid App with an Expensifail account.

Steps

1. Open self DM
2. Create a new track expense
3. Open the expense report
4. Click on the Header and click delete
5. Confirm the delete

Expected Result:

"It's not here" page will not show up when deleting a track expense.

Actual Result:

"It's not here" page shows up briefly when deleting a track expense.

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6679551_1732832209586.delete_track_expense.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021862432764600523584
• Upwork Job ID: 1862432764600523584
• Last Price Increase: 2024-11-29
• Automatic offers:
• shubham1206agra | Reviewer | 105156932
• ikevin127 | Contributor | 105156935

Issue Owner

Current Issue Owner: @shubham1206agra

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @youssef-lr (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot]

💬 A slack conversation has been started in #expensify-open-source

[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 @nkuoch (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot]

💬 A slack conversation has been started in #expensify-open-source

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

[IuliiaHerets]

Production

video_2024-11-29_10-42-11.mp4

[melvin-bot]

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

[melvin-bot]

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

[bernhardoj]

The not found shows because shouldShowNotFoundPage, contentShown.current, and isFocused is true.

App/src/pages/home/report/withReportOrNotFound.tsx

Lines 88 to 93 in decb695

	// If the content was shown, but it's not anymore, that means the report was deleted, and we are probably navigating out of this screen.
	// Return null for this case to avoid rendering FullScreenLoadingIndicator or NotFoundPage when animating transition.
	// eslint-disable-next-line react-compiler/react-compiler
	if (shouldShowNotFoundPage && contentShown.current && !isFocused) {
	return null;
	}

I added the isFocused in #51920 so we don't render null if the screen is still focused to fix #51388. It's been deployed a month ago, so not sure what causes this. My guess is that, previously, when we delete the expense, the screen is blurred already before the report onyx data is deleted.

The comment stated

If the content was shown, but it's not anymore, that means the report was deleted, and we are probably navigating out of this screen.

which means the condition is actually not 100% reliable because we assume the user is probably navigating out of the screen. I think what we can do is delay the delete report similar to this recent PR.

[shubham1206agra]

@mountiny Can we do that ^?

[bernhardoj]

We can delete track, expense/ios, and task, but the issue only happens on track and expense/iou.

[ikevin127]

Edited by proposal-police: This proposal was edited at 2024-11-30 04:36:47 UTC.

Proposal

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

"It's not here" page shows up briefly when deleting a track expense.

What is the root cause of that problem?

The RC of this issue comes from the fix implemented by this PR:

• Fix blank view when report is deleted #51920

specifically the added !isFocused check here:

App/src/pages/home/report/withReportOrNotFound.tsx

Line 91 in 1a6f48e

if (shouldShowNotFoundPage && contentShown.current && !isFocused) {

which when we delete the track expense is false (initially) -> causing the if to not pass -> resulting in the only passing if being the shouldShowNotFoundPage one here:

App/src/pages/home/report/withReportOrNotFound.tsx

Lines 99 to 101 in 1a6f48e

	if (shouldShowNotFoundPage) {
	return <NotFoundPage isReportRelatedPage />;
	}

which renders the It's not here page in our case.

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

Revert the changes implemented by PR #51920 which would fix this issue and from my tests looks like the other issue (#51388) would not be reproducible anymore even with the fix implemented by the offending PR being reverted.

Note

As one can note regarding the other issue's result video below, we do indeed see a Not found page screen briefly (similar to this issue) after reverting the fix but I don't consider that an issue given we're in Debug mode and more importantly the behaviour is present on current staging (meaning no tradeoff).

What alternative solutions did you explore? (Optional)

As an alternative we can remove the contentShown implementation from withReportOrNotFound.tsx and change the first if to:

if (shouldShowNotFoundPage && !isFocused) {
    return null;
}

this would fix our issue, #51388 and the scenario detailed in #53301 (comment).

Results

Android (Native / mWeb)

Current issue	Other issue
our-issue.mp4	other-issue.mp4

[bernhardoj]

#51388 not only fixes the debug page issue, but also this case:

User A invite user B to a room
User B open the room details page
User A removes B from room
User B sees blank page

[shubham1206agra]

Ok, I think the adding !isFocused is causing problems, so we can revert.
@ikevin127 Do you have any way to fix #51388? Maybe force close the details page in this condition.

[ikevin127]

@shubham1206agra A way to fix #51388 as well would be to remove the contentShown implementation and have the first if condition simply as:

                if (shouldShowNotFoundPage && !isFocused) {
                    return null;
                }

This would fix all 3 issues, with #51388 looking like this once you are removed from a private room:

Android: mWeb Chrome

screen-20241129-205253.mp4

I was able to trace down the contentShown implementation to this PRs changes where the author mentions in comment:

if the user leaves the thread, the not found page will appear briefly in closing RHP. This is supposed to fix it. But it has some problems. If I remember correctly it was the only use case

If we're fine with the behaviour shown above in that issue's case then I leave it to you guys to decide whether we can move forward with this proposal.

Updated proposal

• added the solution detailed above as an alternative

[shubham1206agra]

if the user leaves the thread, the not found page will appear briefly in closing RHP. This is supposed to fix it. But it has some problems. If I remember correctly it was the only use case

This is a similar problem as stated in this issue. So I don't think we want to revive an old bug here.

[shubham1206agra]

I remember one solution using useState, and usePrevious where we can set a flag on these commands, and use the previous value / loading indicator instead if these flags are set to true.

Then, if the user closes the RHP, the state gets reset. So no problem will arise there.

[ikevin127]

This is a similar problem as stated in this issue. So I don't think we want to revive an old bug here.

@shubham1206agra The problem is that our current issue (this one) has the not found page showing because of the !isFocused check, which by the way - right now on staging if we test #51388, we still see a not found page for a split second, meaning the contentShown implementation doesn't really work (at least not in this case).

That's why I suggested to simply remove it since we don't have a specific case which we know of that the contentShown implementation fixes / improves, and the only scenario where we still see the not found page is the Debug mode issue which being in Debug mode - shouldn't be a significant issue that we see the not found page for a split second (which we already see right now on staging even with the current implementation).

I already looked at implementations based on state but I did not want to propose that since we would increase the render count of all pages wrapped by withReportOrNotFound. I stand by the alternative solution detailed in #53301 (comment), let us know if you think that can be a working solution in this case and whether it passes all the test scenarios on your side.

[shubham1206agra]

Lets go with @ikevin127's alternative proposal.

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignees @nkuoch and @youssef-lr are eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

📣 @shubham1206agra 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

[ikevin127]

PR will be opened in ~1 hour.

♻️ Update: Debugging the solution as it doesn't seem to work as expected.

[ikevin127]

@shubham1206agra While preparing the PR I tested the proposed alternative solution again (without any console.logs added) and it does not seem to fix our issue, having the same issue with the !isFocused check as described in the RCA. Not sure why having a console.log above the first if logging !isFocused seems to fix the issue but when the log is removed it doesn't 🤔

Not sure how this behaved on your side but the issue is still consistently reproducible on my side with the alternative fix.

Therefore I went back to the main solution, removed the !isFocused check and re-tested:

• this issue
• issue #51388
• test case mentioned in #53301 (comment)

and all tests pass on my side on all platforms where the issues were present.

Therefore I decided to move forward with the PR and main solution - making sure to add the tests for the other issues as well and you can let me know in the PR if they all pass on your side as well.

[ikevin127]

♻️ Status update: PR has been open and ready for review for two days - awaiting review.

[ikevin127]

♻️ Status update: PR has been open and ready for review for 4 days - still awaiting review from @shubham1206agra.

[shubham1206agra]

Sorry for the delay
I will review this today

[ikevin127]

♻️ Status update: PR is pending review once again after some code changes following first review.

♻️ Status update 12/16/2024: Working on the PR, one more test case needs to pass from one of the previous issues.

♻️ Status update 12/18/2024: PR is pending review once again after latest code changes related to one of the failing tests, more details in #53408 (comment).

[melvin-bot]

This issue has not been updated in over 15 days. @nkuoch, @youssef-lr, @RachCHopkins, @ikevin127, @shubham1206agra eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!