[$500] Infinite loading when accessing a non-existing workspace

[m-natarajan]

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.41-2
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: @hayata-suenaga
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1707848696684749

Action Performed:

1. Log out
2. Go to some WS link, e.g. https://staging.new.expensify.com/workspace/adsasddssad/rateandunit (doesn't matter if policy exist or not)
3. Log in

Expected Result:

Shouldn't be infinite loading

Actual Result:

Infinite loading appears

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019c756122ca1f4027
• Upwork Job ID: 1757527427083399168
• Last Price Increase: 2024-02-13
• Automatic offers:
• fedirjh | Reviewer | 0
• bernhardoj | Contributor | 0

Issue Owner

Current Issue Owner: @fedirjh

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[abzokhattab]

the issue is not reproducible on the latest main.

[kosmydel]

the issue is not reproducible on the latest main.

I can still reproduce it.

[hayata-suenaga]

I can also still reproduce this

[bernhardoj]

Proposal

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

If we open a nonexistent workspace page after login, the app will load indefinitely.

What is the root cause of that problem?

When we open a nonexistent workspace page after login, the OpenApp request is never called. It will only be called if isReadyToOpenApp promise is resolved.

App/src/libs/actions/App.ts

Line 151 in 574157e

isReadyToOpenApp.then(() => {

App.confirmReadyToOpenApp is used to resolve the promise and it currently is called in both ReportScreenIDSetter and SidebarLinks. When we open the workspace page after login, the report screen isn't added to the navigation stack, so App.confirmReadyToOpenApp is never called in ReportScreenIDSetter.

In SidebarLinks, App.confirmReadyToOpenApp is called only on small screen.

App/src/pages/home/sidebar/SidebarLinks.js

Lines 54 to 59 in b0b651b

	useEffect(() => {
	if (!isSmallScreenWidth) {
	return;
	}
	App.confirmReadyToOpenApp();
	}, [isSmallScreenWidth]);

Because the promise isn't resolved, the app shows the loading indicator infinitely.

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

SidebarLinks is placed inside the bottom tab navigator which is always exist in the nav stack, so I think we can just have one place to call App.confirmReadyToOpenApp inside SidebarLinks. So, we can just remove the small screen condition.

App/src/pages/home/sidebar/SidebarLinks.js

Lines 54 to 59 in b0b651b

	useEffect(() => {
	if (!isSmallScreenWidth) {
	return;
	}
	App.confirmReadyToOpenApp();
	}, [isSmallScreenWidth]);

[fedirjh]

When we open the workspace page after login, the report screen isn't added to the navigation stack, so App.confirmReadyToOpenApp is never called in ReportScreenIDSetter.

@bernhardoj Your proposal makes sense. I am just wondering why ReportScreenIDSetter is never mounted? is it mounted after the reports are loaded? And are the reports loaded after OpenApp is called?

[bernhardoj]

ReportScreenIDSetter will be mounted alongside ReportScreen.

App/src/libs/Navigation/AppNavigator/ReportScreenWrapper.tsx

Lines 13 to 25 in 634f766

	return (
	<>
	<ReportScreen
	// @ts-expect-error Error will be resolved after ReportScreen migration to TypeScript
	route={route}
	navigation={navigation}
	/>
	<ReportScreenIDSetter
	route={route}
	navigation={navigation}
	/>
	</>
	);

If you are aware, before the ideal nav, in CustomRouter, we have a logic to make sure the report screen (specifically CentralPaneNavigator) is always added to the nav stack on large screen. (old code below)

App/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.js

Lines 71 to 72 in 1682be4

	// Make sure that there is at least one CentralPaneNavigator (ReportScreen by default) in the state if this is a wide layout
	if (!isAtLeastOneCentralPaneNavigatorInState(partialState) && !options.getIsSmallScreenWidth()) {

CentralPaneNavigator now includes workspace screen (overview, reimburse, etc.).

App/src/libs/Navigation/AppNavigator/Navigators/CentralPaneNavigator/BaseCentralPaneNavigator.tsx

Lines 36 to 53 in 634f766

	return (
	<Stack.Navigator screenOptions={options}>
	<Stack.Screen
	name={SCREENS.REPORT}
	// We do it this way to avoid adding the url params to url
	initialParams={{openOnAdminRoom: openOnAdminRoom === 'true' || undefined}}
	component={ReportScreenWrapper}
	/>
	{Object.entries(workspaceSettingsScreens).map(([screenName, componentGetter]) => (
	<Stack.Screen
	key={screenName}
	name={screenName as keyof Screens}
	getComponent={componentGetter}
	/>
	))}
	</Stack.Navigator>
	);

The new CustomRouter logic now making sure to add a full screen or central pane navigator to the nav stack

App/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.ts

Lines 54 to 84 in 634f766

	const pathFromCurrentState = getPathFromState(state, linkingConfig.config);
	const {adaptedState: templateState} = getAdaptedStateFromPath(pathFromCurrentState, linkingConfig.config);
	if (!templateState) {
	return;
	}
	const templateFullScreenRoute = templateState.routes.find((route) => route.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR);
	// If templateFullScreenRoute is defined, and full screen route is not in the state, we need to add it.
	if (templateFullScreenRoute) {
	insertRootRoute(state, templateFullScreenRoute);
	return;
	}
	const topmostCentralPaneRoute = state.routes.filter((route) => route.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR).at(-1);
	const templateCentralPaneRoute = templateState.routes.find((route) => route.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR);
	const topmostCentralPaneRouteExtracted = getTopmostCentralPaneRoute(state);
	const templateCentralPaneRouteExtracted = getTopmostCentralPaneRoute(templateState as State<RootStackParamList>);
	// If there is no templateCentralPaneRoute, we don't have anything to add.
	if (!templateCentralPaneRoute) {
	return;
	}
	// If there is no topmostCentralPaneRoute in the state and template state has one, we need to add it.
	if (!topmostCentralPaneRoute) {
	insertRootRoute(state, templateCentralPaneRoute);
	return;
	}

In this case, visiting /workspace/{policyID}/rateandunit adds WorkspaceReimburse (belongs to CentralPaneNavigator) to the stack

[fedirjh]

@bernhardoj's proposal looks good to me. Let's proceed with it.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @fedirjh 🎉 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]

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

[bernhardoj]

PR is ready

cc: @fedirjh

[slafortune]

Looks like the automated system didn't update. Deployed to production on Feb 28 - 7 holding day means payment would be due on March 6th

[fedirjh]

@blimpich Could you please remove the Reviewing label and bump to Daily as payment is due? Thanks

[blimpich]

@slafortune can you handle payment here?

[slafortune]

Sorry I lost sight of this one! PAID!

@fedirjh can you please complete the checklist?

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:

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

[fedirjh]

BugZero Checklist:

• Link to the PR: [Wave 8] Ideal nav  #33280
• Link to comment: [Wave 8] Ideal nav  #33280 (comment)
• Link to discussion: N/A
• Determine if we should create a regression test for this bug: Yes

Regression Test Proposal

1. Log out
2. Go to some WS link, e.g. https://staging.new.expensify.com/workspace/adsasddssad/rateandunit (doesn't matter if policy exist or not)
3. Log in
4. Verify the loading indicator is shown. Wait for a few seconds for the app to load
5. Verify the app loads complete and a not found page or workspace page is shown

• Do we agree 👍 or 👎

[slafortune]

Thanks @fedirjh !