[HOLD for payment 2023-11-30] Chat not loading after redirection from OldDot

[francoisl]

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: v1.4.0-3
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): N/A
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

Context

We're testing a feature to redirect users that sign up on Expensify Classic (OldDot) to New Expensify. Sometimes, upon being redirected to NewDot, the app doesn't fully load, and the chat view shows the loading animation.

Action Performed:

1. Open staging.expensify.com
2. Click on "Email" > enter any email address from a public domain that +classicredirect (that's what enables the redirection for now, while we test). For example: [email protected] > click Next > Join
3. At that point you'll be redirected to NewDot, and the app should load on the Concierge chat. Sometimes, the Concierge chat doesn't get displayed

The issue doesn't always happen (maybe 1 in 10 times in my personal experience), so you might have to test multiple times in a row with different accounts.

Expected Result:

The Concierge chat should be displayed.

Actual Result:

The Concierge chat is not displayed, there's a loading animation in the chat view instead.

Workaround:

Platforms:

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

redirect1.mov

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @kevinksullivan

[melvin-bot]

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

[koko57]

I can help with this issue 🙂

[danielrvidal]

@koko57 let us know how we can help diagnose! We're trying to get this one cleared asap.

[roryabraham]

The problem appears to be LogOutPreviousUserPage. If the user wasn't already signed in with a different account, and the page is missing the shouldForceLogin route param, it will just display a spinner forever. Unless I'm missing something this seems like the problem?

[roryabraham]

There are two places in OldDot PHP where this redirection happens AFAICT:

• for new users
• here

And in neither case is shouldForceLogin included in the URL

[roryabraham]

This is very consistently reproducible for me if you sign in as a new user on OldDot without being signed in to an account on NewDot

[roryabraham]

Ok, I think I've figured out the race condition at play here...

"Happy" path (haven't observed yet)

1. Redirect from OldDot
2. PublicScreens mounts.
3. LoginWithShortLivedAuthToken page renders:

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

   Lines 23 to 27 in 5d45f0f

   	<RootStack.Screen
   	name={SCREENS.TRANSITION_BETWEEN_APPS}
   	options={defaultScreenOptions}
   	component={LogInWithShortLivedAuthTokenPage}
   	/>

4. Session.signInWithShortLivedAuthToken happens
5. Sign in completes and LoginWithShortLivedAuthToken page renders again, the useEffect runs, and exitTo navigation happens. Note: I think there are potentially multiple reasons this might not happen:
   • As soon as sign-in completes, the PublicScreens will unmount and AuthScreens will mount. I think that consistently happens before LoginWithShortLivedAuthToken re-renders, because it's further up the tree
   • There's nothing in the props.route that will change as a result of calling Session.signInWithShortLivedAuthToken. So even if the component re-renders, the useEffect won't

What actually happens 100% of the time AFAICT

1. Redirect from OldDot
2. PublicScreens mounts
3. LoginWithShortLivedAuthToken page renders:

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

   Lines 23 to 27 in 5d45f0f

   	<RootStack.Screen
   	name={SCREENS.TRANSITION_BETWEEN_APPS}
   	options={defaultScreenOptions}
   	component={LogInWithShortLivedAuthTokenPage}
   	/>

4. Session.signInWithShortLivedAuthToken happens
5. PublicScreens unmounts and AuthScreens mounts.
6. LogOutPreviousUserPage renders
   1. [this redundant call to Session.signOutAndRedirectToSignIn() never happens (which, we don't want it to happen anyways...)
   2. shouldForceLogin is false.
   3. So literally nothing else happens and we get an infinite spinner

[roryabraham]

Quick and dirty fix: #31522

However, I think that some work to clean up and simplify this flow would be valuable. We can have only one transition page that works if a user is:

1. Already signed in
2. Signed in to the wrong account
3. Provided an exitTo, or not
4. etc...

and then we could create E2E tests for all those flows. I noticed a number of redundancies and seemingly unnecessary complexities while looking through this code.

[quinthar]

If this issue only happens when you're already signed in, I think this isn't a blocker

[koko57]

tried to reproduce the bug many times (with changing the network throttling, being logged in as another user or logged out) but I couldn't reproduce it even once.

I think Rory's right - the user is stuck on LogOutPreviousUserPage.

[koko57]

@roryabraham in what scenarios shouldForceLogin is sent with the URL?

[roryabraham]

All that's left here is C+ payment to @allroundexperts for the PR

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.2-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [CP Staging] Prevent infinite spinner in transition page #31522

If no regressions arise, payment will be issued on 2023-11-30. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @koko57 does not require payment (Contractor)
• @allroundexperts does not require payment (Eligible for Manual Requests)

[melvin-bot]

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:

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

[melvin-bot]

@kevinksullivan, @koko57, @allroundexperts, @roryabraham Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@kevinksullivan, @koko57, @allroundexperts, @roryabraham Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@kevinksullivan, @koko57, @allroundexperts, @roryabraham Still overdue 6 days?! Let's take care of this!

[roryabraham]

Friendly bump to process payment here @kevinksullivan

[kevinksullivan]

Ah it's just @allroundexperts , so summarizing payment:

• @allroundexperts : $500

[allroundexperts]

@kevinksullivan I think it should be $500.

[kevinksullivan]

Thanks @allroundexperts , I'll let you submit the request and close this out.

[JmillsExpensify]

$500 payment to @allroundexperts based on summary above.