[$500] Deeplinks - Infinite loading displayed when user navigated on non-existing chat deeplink

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

Precondition: app should be killed

1. Open app
2. Open any existing coversation
3. Hide the app
4. Open non-existing coversation deeplink https://staging.new.expensify.com/r/1234

Expected Result:

User should be navigated on "You don't have access to this chat" page

Actual Result:

Infinite loading displayed

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

Bug6326513_1703637346495.Rpreplay_Final1703617339.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bad93f473888207b
• Upwork Job ID: 1739822788800614400
• Last Price Increase: 2024-01-24
• Automatic offers:
• tienifr | Contributor | 28121883

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @garrettmknight (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 - @sobitneupane (External)

[tienifr]

Proposal

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

Infinite loading displayed

What is the root cause of that problem?

We're reusing ReportScreen even though the reportID in route changes.

See here, when the reportID in route changes, the same ReportScreen instance will be used. So the wasReportAccessibleRef in here is still true, even though now it's a new report and it should be evaluated again. This causes the shouldShowNotFoundPage here to always be false, preventing the not found screen from opening.

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

Add route reportID as key of ReportScreen.

We can do this by modify this to

<ReportScreen route={route} key={getReportID(route)}/>

(The getReportID is the same as here)

What alternative solutions did you explore? (Optional)

If we want to keep the ReportScreen render shared between different reportIDs, we need to reset the wasReportAccessibleRef to false when the reportID from route change (and potentially the firstRenderRef to true as well), and modify the condition here so that it will not set wasReportAccessibleRef to false if the reportID of report and reportID from route doesn't match.

[wildan-m]

Proposal

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

Deeplinks - Infinite loading displayed when user navigated on non-existing chat deeplink

What is the root cause of that problem?

In native, ReportScreen is not actually unmounted when there is a change in reportID.

When the reportID known to be invalid it will hit this check in ReportScreen.

onyxReportID === prevReport.reportID && (!onyxReportID

onyxReportID and prevReport.reportID will be undefined.

This will cause infinite loading since isLoadingInitialReportActions never false and wasReportAccessibleRef.current will always be true.

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

When reportID known to be invalid, we can mark wasReportAccessibleRef.current and isLoadingInitialReportActions to false

        if ((onyxReportID === prevReport.reportID && (!onyxReportID || onyxReportID === routeReportID))) {
            if(!onyxReportID)
            {
                wasReportAccessibleRef.current = false;
                Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`, {isLoadingInitialReportActions: false});
            }
            return;
        }

[garrettmknight]

@sobitneupane any thoughts on these proposals?

[melvin-bot]

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

[wildan-m]

My proposal updated with better solution

[garrettmknight]

@sobitneupane can you give this one a look?

[melvin-bot]

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

[sobitneupane]

Sorry for the delay. I will review proposals shortly.

[sobitneupane]

Thanks for the proposal @tienifr and @wildan-m

Both of the proposal solves the issue.

@wildan-m In your proposal, I don't think we need to set isLoadingInitialReportActions in onyx as the reportID is invalid and most probably we don't store metadata for invalid report.

Can we add wasReportAccessibleRef.current = false; here before returning? That will probably solve the issue.

App/src/pages/home/ReportScreen.js

Lines 203 to 208 in fe49e4f

	useEffect(() => {
	if (!report || !report.reportID || shouldHideReport) {
	return;
	}
	wasReportAccessibleRef.current = true;
	}, [shouldHideReport, report]);

[wildan-m]

@sobitneupane I've tried that, but it will cause not found page on previously accessible report.

1. Open a report A
2. go to android/ios home. minimize the app (without exit)
3. Open a new deeplink with invalid report ID
4. It should show not found page
5. Press back header button, it will show not found page once more where the expected page should be report A

I think @tienifr proposal to add key to the ReportScreen is the best solution.

[wildan-m]

@sobitneupane also my last proposed solution has a glitch. it will show not found page for a brief moment when firstly joined public room

[melvin-bot]

@garrettmknight, @sobitneupane Whoops! This issue is 2 days overdue. Let's get this updated quick!

[garrettmknight]

@sobitneupane good to assign @tienfr here then?

[melvin-bot]

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

[melvin-bot]

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

[marcochavezf]

Sounds good, thanks @sobitneupane for the review, assigning @tienifr 🚀

[melvin-bot]

📣 @tienifr 🎉 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 📖

[tienifr]

PR ready for review #35228.

[melvin-bot]

This issue has not been updated in over 15 days. @garrettmknight, @marcochavezf, @sobitneupane, @tienifr 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!

[sobitneupane]

I am expecting updates from @tienifr in the PR.

[sobitneupane]

The issue was resolved by #36477 PR. So, we are going to close the PR #35228 created for this issue.

[marcochavezf]

Oh then the issue is no longer reproducible in main, correct?

[tienifr]

@marcochavezf That's true.

[garrettmknight]

Gotcha, gonna close in that case.

[tienifr]

@garrettmknight Although the issue was fixed somewhere else, efforts have been made to implement, test and review carefully the PR. Please consider payment for this issue. Thanks!

[tienifr]

@garrettmknight Can you check the above ^. Thanks 🙏

[garrettmknight]

Hey @tienifr - missed the first comment. I think half payment is fair. I'll process now.

Summary of Payments:

• Contributor: @tienifr $250 paid via Upwork
• Reviewer: @sobitneupane $250

[garrettmknight]

@sobitneupane can you request payment when you get a chance?

[JmillsExpensify]

$250 approved for @sobitneupane based on summary.