Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Infinite loader on login #4397

Merged
merged 2 commits into from
Aug 5, 2021
Merged

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Aug 3, 2021

Details

After login,
We call many APIs asynchronously. but it is possible that users can logout before one of these requests is about to make. In this case, Queue will pause as Auth token is not present.
I checked that we pause it

Network.clearRequestQueue();
here.
Now when you enter the password and send that request, the First auth token will be received as this overrides the paused queue. but I think the second call to createTemporaryLogin is not going through. Only after this call is completed loader is stopped and navigation happens.
In Code, the queue is resumed when re-authentication happens but we don't do this on sign up. So if we call Network.unpauseRequestQueue(); in
function signIn(password, twoFactorAuthCode) {
Onyx.merge(ONYXKEYS.ACCOUNT, {...CONST.DEFAULT_ACCOUNT_DATA, loading: true});
API.Authenticate({
useExpensifyLogin: true,
partnerName: CONFIG.EXPENSIFY.PARTNER_NAME,
partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD,
partnerUserID: credentials.login,
partnerUserSecret: password,
twoFactorAuthCode,
email: credentials.login,
})
.then((authenticateResponse) => {
const {authToken, encryptedAuthToken, email} = authenticateResponse;
createTemporaryLogin(authToken, encryptedAuthToken, email);
})
.catch((error) => {
Onyx.merge(ONYXKEYS.ACCOUNT, {error: translateLocal(error.message), loading: false});
});
. It should fix the issue. Also, it does not affect the app logic as Queue should be active after Auth token is received. Even if the queue was already active. resuming it again does hot have any effect.

There was one similar issue like this where the user was blocked on the first stage where the user enters the email which I fixed by forcefully sending that request even if the queue is paused as that did not need auth token but now this issue is happening on the password step which does require Auth token.

Fixed Issues

$ Fixes #4221

Tests | QA Steps

  1. Open the Web app on Browser 1 and browser 2.
  2. Login with USER A on both browsers.
  3. Close the Browser 1 's tab which has the app opened. Don't close the browser.
  4. Go to browser 2 and send 4-5 messages in a row to the USER B.
  5. Go to browser 1 and Press ctrl+shit+T to restore the tab. As soon as you see the App window. Quickly log out before data sync is complete. You should be logging out the USER A.
  6. Now don't reload, Login with User B whom you sent the messages from Browser 2.
  7. You should be logged into the app instead of seeing an infinite loader on the Password form.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

I am not able to reproduce the breaking behavior consistently. If someone can help me do that I will try to show that being fixed here.

Web

Mobile Web

Desktop

iOS

Android

@parasharrajat parasharrajat requested a review from a team as a code owner August 3, 2021 21:51
@MelvinBot MelvinBot requested review from AndrewGable and removed request for a team August 3, 2021 21:51
@roryabraham
Copy link
Contributor

roryabraham commented Aug 4, 2021

I'm still able to reproduce the issue while testing this branch on web:

  1. Log out
  2. Enter email
  3. Enter password
  4. Infinite loading spinner.

@parasharrajat
Copy link
Member Author

Thanks, @roryabraham for the feedback. I will try to reproduce this and update it here.

@parasharrajat
Copy link
Member Author

I found out what is happening. Even I unpause the queue. Old requests that are being retried will pause it again because those requests are checking the old authToken.

if (isAuthTokenRequired(command) && !parameters.authToken) {

Here we check auth token from request data.

The token is only set after call to createTemporaryLogin

setSuccessfulSignInData({...createLoginResponse, encryptedAuthToken});

Thus I checked we can force this request as well. It's safe to use forceNetworkRequest for this call as we pass the token manually to the API.
After that unpaused the queue when the token is set so that other app requests can continue to work.

Pass this check

if (!canMakeRequest(queuedRequest)) {

We can update this

callback: val => authToken = val ? val.authToken : null,
to unpause the queue when the token is set.

or We unpause the queue after this

setSuccessfulSignInData({...createLoginResponse, encryptedAuthToken});
.

As Token is set by now. But I have little doubt that should I wait for Onyx.merge promise to finish.


I have found a way to reproduce this reliably and I will post both videos here (fix and issue).

@parasharrajat
Copy link
Member Author

ISSUE

issue-loader.mp4

FIX

issue-loader-fix.mp4

Seems like the video got corrupted.
Updating the correct reprodcution steps on the Test; ⬆️

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work on web, can no longer reproduce the issue, even with your updated steps.

@parasharrajat
Copy link
Member Author

@AndrewGable All yours.

Copy link
Contributor

@AndrewGable AndrewGable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for hopping on this one so quickly!

@AndrewGable AndrewGable merged commit f693233 into Expensify:main Aug 5, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Aug 5, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@Jag96
Copy link
Contributor

Jag96 commented Aug 5, 2021

CP'ing this so it can be re-tested on the deploy blocker checklist

github-actions bot pushed a commit that referenced this pull request Aug 5, 2021
Fix: Infinite loader on login
(cherry picked from commit f693233)
@OSBotify
Copy link
Contributor

OSBotify commented Aug 5, 2021

🚀 Deployed to staging in version: 1.0.82-5🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Aug 6, 2021

🚀 Deployed to production in version: 1.0.82-7🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Aug 6, 2021

🚀 Deployed to staging in version: 1.0.82-8🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 failure ❌
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@parasharrajat parasharrajat deleted the queueUnlock branch November 20, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite Loading spinner on Login screen
5 participants