[$250] Android - Secondary Log in - An endless spinner when trying validate the secondary login

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Log in to NewDot with any account
2. Log into OldDot with the same account and add a Secondary Login from Account Settings
3. Open validate secondary log in email Verify secondary email for Expensify and copy the last part of the URL, i.e. /v/16/WSSWMNNM (Everything after the /v/)
4. In a text box - Formulate the URL by using the path obtained from step 3 http://staging.new.expensify.com/v/
5. Navigate to the compiled link

Expected Result:

You should redirected to LHN with no error message displayed

Actual Result:

An endless spinner when trying validate the secondary login

Workaround:

unknown

Platform:

Where is this issue occurring?

• Android

Version Number: v1.2.17-4
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Bug5782073_video_2022-10-18_18-31-21.mp4

Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

View all open jobs on GitHub

[neil-marcellini]

Weird, where did we get these reproduction steps from? Do we support validating a secondary login on NewDot?

[kavimuru]

@neil-marcellini This is from the regression steps https://expensify.testrail.io/index.php?/tests/view/2893709&group_by=cases:section_id&group_order=asc&group_id=229043

[neil-marcellini]

Okay got it thanks for clarifying. I'm able to reproduce the issue. I'm also able to reproduce the issue on iOS. I can't reproduce on Web so maybe it has something to do with the small screen width? I'll send it external because if it's limited to certain platforms then it's probably a front end issue.

Screen.Recording.2022-10-20.at.4.54.21.PM.mov

[melvin-bot]

Triggered auto assignment to @davidcardoza (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[melvin-bot]

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

[davidcardoza]

I can't reproduce the issue on iOS or web. This error seems to be isolated to Android only.

[tgolen]

Waitng for proposals

[wildan-m]

Proposal

Root cause:

<FullScreenLoadingIndicator /> is not dismissed before calling Navigation.navigate(ROUTES.HOME); in validateLogin

Solution

Adding Navigation.goBack(); before navigating to home will resolve the issue.

diff --git a/src/libs/actions/User.js b/src/libs/actions/User.js
index c1f11e3610..8d549fb099 100644
--- a/src/libs/actions/User.js
+++ b/src/libs/actions/User.js
@@ -220,6 +220,7 @@ function validateLogin(accountID, validateCode) {
         accountID,
         validateCode,
     }, {optimisticData});
+    Navigation.goBack();
     Navigation.navigate(ROUTES.HOME);
 }

Another solution is also to pop the navigation when the user logged in or logged out:

diff --git a/src/libs/Navigation/Navigation.js b/src/libs/Navigation/Navigation.js
index e937e50159..9bfd819e12 100644
--- a/src/libs/Navigation/Navigation.js
+++ b/src/libs/Navigation/Navigation.js
@@ -64,7 +64,6 @@ function openDrawer() {
     if (!canNavigate('openDrawer')) {
         return;
     }
-
     navigationRef.current.dispatch(DrawerActions.openDrawer());
     Keyboard.dismiss();
 }
@@ -140,15 +139,14 @@ function navigate(route = ROUTES.HOME) {
     }
 
     if (route === ROUTES.HOME) {
+        // If we're navigating to the signIn page, pop whatever screen is on top
+        // since it's guaranteed that the sign in page will be underneath (since it's the initial route).
+        // Also, if we're coming from a link to validate login (pendingRoute is not null), we want to pop the loading screen.
+        navigationRef.current.dispatch(StackActions.pop());
         if (isLoggedIn && pendingRoute === null) {
             openDrawer();
             return;
         }
-
-        // If we're navigating to the signIn page while logged out, pop whatever screen is on top
-        // since it's guaranteed that the sign in page will be underneath (since it's the initial route).
-        // Also, if we're coming from a link to validate login (pendingRoute is not null), we want to pop the loading screen.
-        navigationRef.current.dispatch(StackActions.pop());
         return;
     }

The second solution might affect another part since Navigation.navigate(ROUTES.HOME) is used in multiple places.

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[eVoloshchak]

@wildan-m, thanks for your proposal!

Moving navigationRef.current.dispatch(StackActions.pop()); would result in it being called every time user is navigating back to root.

Calling Navigation.goBack() in validateLogin would actually work. But this feels like a workaround.

The proposal should answer the question of why is this happening?
Why ValidateLoginPage is displayed on top, even when navigating away from it?

[tgolen]

Still working on proposals above. Bump @wildan-m for the questions in the previous comment.

[wildan-m]

Proposal (Updated)

Root Cause

ValidateLoginPage and MainDrawer is <RootStack.Navigator> screen and Navigation.navigate(ROUTES.HOME) is directly open the drawer when the user is logged in:

App/src/libs/Navigation/Navigation.js

Lines 142 to 147 in a2eda4d

	if (route === ROUTES.HOME) {
	if (isLoggedIn && pendingRoute === null) {
	openDrawer();
	return;
	}

I think we can't directly trigger openDrawer when the parent navigator is a Stack navigator

I've tried it here:
https://snack.expo.dev/@muhliswildan/nesting-navigators-%7C-react-navigation

Solution

We can replace the dispatch with navigationRef.current.dispatch(StackActions.replace(SCREENS.HOME)) if it comes from ValidateLogin page.

git diff src/libs/Navigation/Navigation.js
diff --git a/src/libs/Navigation/Navigation.js b/src/libs/Navigation/Navigation.js
index e937e50159..dbe4665bda 100644
--- a/src/libs/Navigation/Navigation.js
+++ b/src/libs/Navigation/Navigation.js
@@ -6,6 +6,7 @@ import Onyx from 'react-native-onyx';
 import Log from '../Log';
 import linkTo from './linkTo';
 import ROUTES from '../../ROUTES';
+import SCREENS from '../../SCREENS';
 import DeprecatedCustomActions from './DeprecatedCustomActions';
 import ONYXKEYS from '../../ONYXKEYS';
 import linkingConfig from './linkingConfig';
@@ -141,6 +142,11 @@ function navigate(route = ROUTES.HOME) {
 
     if (route === ROUTES.HOME) {
         if (isLoggedIn && pendingRoute === null) {
+            if(navigationRef.current?.getCurrentRoute()?.name === "ValidateLogin")
+            {
+                navigationRef.current.dispatch(StackActions.replace(SCREENS.HOME));
+                return;
+            }
             openDrawer();
             return;
         }

cc: @eVoloshchak @tgolen

[melvin-bot]

@tgolen, @davidcardoza, @eVoloshchak Whoops! This issue is 2 days overdue. Let's get this updated quick!

[tgolen]

Thanks for the updated proposal.

ValidateLoginPage and MainDrawer is <RootStack.Navigator> screen and Navigation.navigate(ROUTES.HOME) is directly open the drawer when the user is logged in:

Sorry, I'm not really following this. It kind of looks like words are missing or the grammar is wrong enough that I'm not understanding the reason. Can you try to reword this, please?

I think we can't directly trigger openDrawer when the parent navigator is a Stack navigator

This sounds like a good guess, but let's make sure that's the problem. Also, if this is indeed the root cause of the problem, can the fix be something that looks at the parent navigator to see if it's a stack navigator or not? I don't like how the proposal only references the ValidateLogin route, since it sounds like it could apply to any route where the parent navigator is a stack navigator.

[wildan-m]

Proposal (Updated)

@tgolen

Sorry, I'm not really following this. It kind of looks like words are missing or the grammar is wrong enough that I'm not understanding the reason. Can you try to reword this, please?

Please have a look at this hierarchy

App/src/libs/Navigation/AppNavigator/AuthScreens.js

Lines 162 to 199 in 4f92e57

	<RootStack.Navigator
	mode="modal"
	// We are disabling the default keyboard handling here since the automatic behavior is to close a
	// keyboard that's open when swiping to dismiss a modal. In those cases, pressing the back button on
	// a header will briefly open and close the keyboard and crash Android.
	// eslint-disable-next-line react/jsx-props-no-multi-spaces
	keyboardHandlingEnabled={false}
	>
	{/* The MainDrawerNavigator contains the SidebarScreen and ReportScreen */}
	<RootStack.Screen
	name={SCREENS.HOME}
	options={{
	headerShown: false,
	title: 'New Expensify',
	// prevent unnecessary scrolling
	cardStyle: {
	overflow: 'hidden',
	height: '100%',
	},
	}}
	getComponent={() => {
	const MainDrawerNavigator = require('./MainDrawerNavigator').default;
	return MainDrawerNavigator;
	}}
	/>
	<RootStack.Screen
	name="ValidateLogin"
	options={{
	headerShown: false,
	title: 'New Expensify',
	}}
	getComponent={() => {
	const ValidateLoginPage = require('../../../pages/ValidateLoginPage').default;
	return ValidateLoginPage;
	}}
	/>

MainDrawer and ValidateLogin are RootStack navigator screens.

ValidatLoginPage will call this function

App/src/pages/ValidateLoginPage.js

Line 23 in 91acada

User.validateLogin(accountID, validateCode);

which will dispatch the screen with the home screen:

App/src/libs/actions/User.js

Line 226 in 91acada

Navigation.navigate(ROUTES.HOME);

But this navigate function directly opens the drawer

App/src/libs/Navigation/Navigation.js

Lines 142 to 146 in 91acada

	if (route === ROUTES.HOME) {
	if (isLoggedIn && pendingRoute === null) {
	openDrawer();
	return;
	}

Since ValidateLoginPage is a stack navigator screen and MainDrawer is a child of StackNavigator, openDrawer will not work. The MainDrawer is not loaded/stacked yet.

can the fix be something that looks at the parent navigator to see if it's a stack navigator or not?

I've tweaked the code to include all of the root routes. Please have a look:

git diff src/libs/Navigation/Navigation.js
diff --git a/src/libs/Navigation/Navigation.js b/src/libs/Navigation/Navigation.js
index e937e50159..06e71d173c 100644
--- a/src/libs/Navigation/Navigation.js
+++ b/src/libs/Navigation/Navigation.js
@@ -6,6 +6,7 @@ import Onyx from 'react-native-onyx';
 import Log from '../Log';
 import linkTo from './linkTo';
 import ROUTES from '../../ROUTES';
+import SCREENS from '../../SCREENS';
 import DeprecatedCustomActions from './DeprecatedCustomActions';
 import ONYXKEYS from '../../ONYXKEYS';
 import linkingConfig from './linkingConfig';
@@ -141,6 +142,13 @@ function navigate(route = ROUTES.HOME) {
 
     if (route === ROUTES.HOME) {
         if (isLoggedIn && pendingRoute === null) {
+            const rootRoutes = navigationRef.current.getRootState().routeNames;
+            if(rootRoutes.indexOf(navigationRef.current.getCurrentRoute()?.name) !== -1)
+            {
+                navigationRef.current.dispatch(StackActions.popToTop(SCREENS.HOME));
+                Keyboard.dismiss();
+                return;
+            }
             openDrawer();
             return;
         }

[tgolen]

Ah, great. Thank you for the more thorough proposal! I think the solution is looking better and I like how it's a bit more generalized now. I was talking with @marcaaron about this issue and he has a lot of knowledge about the navigators, so I think getting his perspective on this proposal would be ideal.

[marcaaron]

Really appreciate the investigation here so far. However, since validating a secondary login has a workaround (can validate via OldDot) and since we are actively discussing deprecating the use of the drawer navigator. I'm not sure we should invest anymore time or energy into short term fixes that are difficult to understand and create a high barrier to entry for our internal engineers and contributors.

[eVoloshchak]

Not overdue, the discussion is in progress on whether to use the drawer navigator

[eVoloshchak]

Not overdue

[davidcardoza]

Based on @marcaaron's latest assessment of the issue I think we are good to close this issue. If anyone disagrees please reopen the issue.

[kavimuru]

@davidcardoza Issue is reproducible in Web, mWeb and Desktop as well. Are these navigation issues are on Hold now?

Secondary-Login-Endless-Spinner.mp4

[davidcardoza]

I'll lean on @marcaaron's expertise here because I am not sure if we're still actively looking to deprecate the use of the drawer navigator, which would potentially make this not a priority.

[tgolen]

I'm pretty sure our plan is still to deprecate the drawer navigator. This is all being handled in #11768 so I will keep this closed.