[HOLD for payment 2023-07-26] [$2000] Desktop - App is show endless spinner when tap popup Open app in "New Expensify

[kbecciv]

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

---

Issue found when executing PR https://github.com/Expensify/App/pull/16869

Action Performed:

1. Open staging.expensify.com, login with account A.
2. Open staging.new.expensify.com and login with same account.
3. Download and open the staging Desktop App, if you haven't already.
4. Click on the concierge button at bottom right on staging.expensify.com/inbox
5. Tap on the "Open app in "New Expensify" popup.

Expected Result:

Desktop App should be open and user login.

Actual Result:

Desktop App is show endless spinner.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [ ] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [ ] MacOS / Chrome / Safari
• [x] MacOS / Desktop

Version Number: 1.2.99.4

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/231794818-9e4fb26c-ecfb-4945-9586-dd6cf5cbf65b.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0151b55ece236c38ac
• Upwork Job ID: 1646650899463368704
• Last Price Increase: 2023-04-24

Issue Owner

Current Issue Owner: @mallenexpensify

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] 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
• [x] 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.
• [x] 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.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0151b55ece236c38ac

[MelvinBot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @s77rt (Internal)

[mallenexpensify]

Was able to reproduce, you might not need to do 'all' the steps, maybe try

• Have staging version of desktop open, working and logged in with User A
• Go to staging.expensify.com on web, log in with User A,
• Click concierge button in bottom right
• Click 'Open New Expensify' in the app
• Observe spinner.

Reckon this is internal. Refreshing 'fixed' it. I didn't see any errors in the console either

[s77rt]

I was able to reproduce too. Unrelated but on staging.new.expensify.com we are not getting redirected to the concierge chat.

[MelvinBot]

Triggered auto assignment to @MariaHCD (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[mallenexpensify]

Eeeps, didn't know (or forgot) I also needed to apply Engineering along with Internal. @MariaHCD can you take a look plz?

[MariaHCD]

I'm able to reproduce the issue as well. As an additional note, if you're already signed in on Desktop, you get a Session Expired error instead of a loading spinner:

[MariaHCD]

After doing some more testing, I see that the infinite spinner isn't always reproducible, I tested several times and was able to load the desktop app without issues:

https://user-images.githubusercontent.com/12268372/232421505-46ea1d75-7ca1-4622-9748-23eddef00801.mov

[MariaHCD]

So it looks like the "Open in New Expensify" button from web does work (the requests to SignInWithShortLivedAuthToken and OpenApp succeed, internal logs here) but seems to fail when you've just newly/recently logged into your account on OldDot.

I did notice that when the infinite spinner was shown on desktop, the requests to SignInWithShortLivedAuthToken and OpenApp were not sent:

https://user-images.githubusercontent.com/12268372/232423058-f11c8b00-59d2-4d5e-b56c-6e2c5cae0d75.mov

So that makes me think that perhaps the App is not sending those requests when opened from web. In this case, I think this can be an external issue.

[MelvinBot]

Current assignee @mallenexpensify is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

Current assignee @s77rt is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

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

[MelvinBot]

Upwork job price has been updated to $2000

[mallenexpensify]

I did a quick-double here on the price as I'd like to see this solved well-before ExpensiCon which is weeks away. We don't want attendees clicking links and getting spinners

[hoangzinh]

Proposal

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

Desktop - App is show endless spinner when tap popup Open app in "New Expensify

What is the root cause of that problem?

The root cause is we haven't removed the "/transition" route from history in this case => it causes us get stuck with loading indicator in page LogOutPreviousUserPage. More details (in case Desktop App is not logged with any user):

1. When navigate from OldDot -> NewDot, our route is "/transition"
2. According to PublicScreen route stack, it will render LogInWithShortLivedAuthTokenPage
3. After authenticate with short-live authToken, according to AuthScreen route stack, it will render LogOutPreviousUserPage
4. And after sign-in again with short-live authToken, we do nothing => get stuck in this component LogOutPreviousUserPage

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

We have solved this problem in several places. Ie here 1 or here 2

We can apply same logic after API signInWithShortLivedAuthToken called (after this line). More details:

• Call Navigation.dismissModal when navigation ready => to dismiss /transition route for OldDot to NewDot transitions
• (Optional) Check if exitTo exists in URL params => navigate to this route

[MelvinBot]

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.

[s77rt]

@hoangzinh Thanks for the proposal. I don't think your RCA is entirely correct LogOutPreviousUserPage indeed does not perform any navigation so we are as you said stuck. But I think it's by design not to perform navigation as the purpose of that component is to solely to handle user session. The navigation is handled by setUpPoliciesAndNavigate.

[s77rt]

I think the bug is a regression from https://github.com/Expensify/App/pull/16043. That PR adds the /transition prefix to all deeplinks.

• So if the url is https://staging.new.expensify.com/transition?exitTo=GOODPLACE
• It will become https://staging.new.expensify.com/transition?exitTo=%2Ftransition%3FexitTo%3DGOODPLACE

That means setUpPoliciesAndNavigate will redirect us to another transition page which will only show the loader.

Refreshing the page works because we are refreshing the new transition url which contains the correct exitTo path (cc @mallenexpensify https://github.com/Expensify/App/issues/17391#issuecomment-1507715567).

---

It may be obvious to simply not to add the transition prefix if the url already starts with that, but I think something is wrong with that PR, raised the concern here https://github.com/Expensify/App/pull/16043#issuecomment-1512468210.

[hoangzinh]

@s77rt you're right. I got noticed in the exitTo request param is weird during investigation and look into how it's built in above PR. But I thought it's another issue.

[hoangzinh]

@s77rt will we let the author of this PR fix or I need to update my proposal again?

[mallenexpensify]

@s77rt , thanks for the breakdown above, it makes more sense to me now.

[s77rt]

@hoangzinh Feel free to update your proposal. But I think it may be better to revert the PR since it was build on a wrong concept; The /transition root is for OLDDOT -> NEWDOT and not for WEB -> DESKTOP.

cc @pecanoro @tgolen @hayata-suenaga @thesahindia Need some help here since you reviewed that PR.

[hayata-suenaga]

@ntdiary do you also have idea how to solve this as you're touching the same area?

[pecanoro]

The original author created another PR to clean up the faulty PR: here. @ntdiary do you think that PR will solve this regression as well?

[ntdiary]

The original author created another PR to clean up the faulty PR: https://github.com/Expensify/App/pull/17452. @ntdiary do you think that PR will solve this regression as well?

@pecanoro, actually, I don't think it will. And as for this issue, I don't think it's a regression from #16043.

Here are related PRs:

• #16383 This PR aims to remove the redirection ui from deep link component, and leave some code that no longer used.
• #16043 This PR aims to sync the user's session when the use opens desktop app from the web app.
• #16869 This PR aims to allow all routes to display the popup window that opens the desktop app.
• #17452 This PR aims to clean up the useless code in PR #16383 and #16043, and does not change the business logic.

@ntdiary do you also have idea how to solve this as you're touching the same area?

@hayata-suenaga, yeah. Like before PR #17452, when we open NewDot from OldDot, the popup window doesn't appear. Before:

IMO, when opening NewDot from OldDot, we need to confirm whether the popup should appear or not. How many similar cases are there, and do they all need to display the popup window?

If not, we should exclude these links(e.g./transition url) in the deep link component. If yes, and if these cases need to log in with short-lived auth token first, then we should display the popup window after the login is complete.

Just for this issue, we can exclude /transition url and then display the popup window in the setUpPoliciesAndNavigate: the demo code may be like this:

It might be easier to fix this after PR #17452 and #17364 are merged.

[s77rt]

@ntdiary I think it's a regression from that PR because only that PR adds the /transition prefix which breaks all desktop deeplinks for olddot links. Are you planning to remove that prefix? I don't think we should use it since it's not intended for desktop deeplinks but only for olddot to newdot transition.

[ntdiary]

@ntdiary I think it's a regression from that PR because only that PR adds the /transition prefix which breaks all desktop deeplinks for olddot links. Are you planning to remove that prefix? I don't think we should use it since it's not intended for desktop deeplinks but only for olddot to newdot transition.

@s77rt, no, we have a plan to create a new BeginDeepLinkRedirect command. Why are you think it's a regression? Or am I understanding the definition of regression wrong? The related feature was never implemented, and removing it won't help fix this issue. Or you think we should use another way to sync the user session between web app and desktop app?

[s77rt]

@ntdiary I think the timeline is a bit confusing :sweat_smile:. All what I'm trying to say is if that PR never existed the bug would not occur. You can try do the following:

1. Get back to version 1.2.93-0 (before PR was deployed)
2. Apply https://github.com/Expensify/App/pull/16869 to enable deeplinks for all urls
3. Verify deeplinks for OLDDOT urls works

In such cases where:

• PR1 introduced a bug that is not reproducible
• PR2 revealed the bug of PR1 I think we trace back the regression to PR1 since it's the offending PR.

---

I'm only against the idea of using one thing for two different things. The /transition root has nothing to do with deeplinks. If we can keep the same logic but use a dedicated root for WEB -> DESKTOP that would be better. But that's just my personal opinion. Whatever the team sees as a better fit works for me.

I hope my concerns are clear here :+1:

[hoangzinh]

agree with @s77rt. If we want to use existing one, we have to ensure it won't break the old flow (unfortunately, it only reveal with PR2). But even it won't break the old flow, the name of route makes confusing for us, ROUTES.TRANSITION_FROM_OLD_DOT but using also for open deep links WEB -> DESKTOP.

agree again with @s77rt, the quick fix is having a dedicated route for open deep links.

[ntdiary]

@s77rt, Thanks a lot for your detailed explanation. I don't think we can guarantee that a PR can be fully responsible for future breaking changes.

As for oldDot --> newDot and Web --> Desktop. They are both essentially trying to sync the the user session between the two systems and are highly similar. I think what we actually need to solve is to pass a correct link to the desktop app, adding a route doesn't help the problem. And if the name of route makes confusing for us, we could also consider changing its name to avoid confusion. Maybe here we need more inputs from the team.

[ntdiary]

One more thing, I only remember that the short-lived auth token is valid for 1 minute, but I don't remember whether it has limited its sign-in times. Although in my past experience, a temporary token should only be verified once for security reasons, but from your case above, it seems that it can be used to sign in repeatedly within 1 minute.

[s77rt]

@ntdiary I understand your point. I just want to clear the ambiguity. At this current state if you navigate code you may find isTransitioningFromOldDot being true but it's actually not, we are not transitioning from old dot, but probably from a deeplink. If simply changing the names would be enough, let's do it.

Or we can break this up into two parts:

1. Sync process
2. Transition process

For deeplinks we go through sync process. For old dot transition we go through the transition process that under the hood makes use of the sync process.

[ntdiary]

Or we can break this up into two parts:

1. Sync process
2. Transition process

For deeplinks we go through sync process. For old dot transition we go through the transition process that under the hood makes use of the sync process.

@s77rt, sorry, I'm afraid I haven't fully understood this part. Could you please explain in more detail what the new sync process and new transition process will do? Or what is the difference between them and the existing logic? AFAIK, the transition page is responsible for the sync of the user session, and the App.setUpPoliciesAndNavigate is responsible for the navigation. These two parts (sync and navigation) are needed whether opening newDot from oldDot or opening desktop app from web app. And thank you for your thoughts on these issues. I think we will definitely reach a consensus and have a good solution in the end. 🍻

[s77rt]

@ntdiary Thanks for the quick follow up. If the deeplink process and the transition process are identical then we can just rename the variables to reflect that.

• Current: /transition is only used for olddot -> newdot
• Suggested: /transition is used to sync user session from olddot to newdot or from web to desktop

So the change will only revolve around clearing the comments and changing variable names accordingly. Then to fix this issue we can just skip adding the /transition prefix if the url already starts with that.

Does that sound good enough?

[ntdiary]

@s77rt, that sounds good to me, and it won't be a difficult thing. : )

[MelvinBot]

@pecanoro, @s77rt, @mallenexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[s77rt]

@pecanoro @mallenexpensify @tgolen Can you please help move this forward?

• Should this be done as a part of https://github.com/Expensify/App/pull/17452?
• Should we consider this as a follow up of https://github.com/Expensify/App/issues/15651? technically the offending PR's regression period ended on April 12 and this issue was created on April 13. So a no?

[tgolen]

I'd prefer doing this as part of cleaning up the deep link code.

[MelvinBot]

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

[pecanoro]

Ok, let's assign the issue to @ntdiary so we can handle it as part of https://github.com/Expensify/App/pull/17452

[MelvinBot]

📣 @ntdiary You have been assigned to this job by @pecanoro! Please apply to this job in Upwork 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 📖

[mallenexpensify]

@ntdiary can you please apply and reply here with your first name once you have? https://www.upwork.com/jobs/~0151b55ece236c38ac

[ntdiary]

@mallenexpensify, thank you for the reminder. I have applied, my name is wentao

[mallenexpensify]

@ntdiary and @s77rt , can you please accept the job when you get a chance?
https://www.upwork.com/jobs/~0151b55ece236c38ac

[ntdiary]

@mallenexpensify, I have accepted.

---

I want to confirm again, if the current newDot page is navigated from oldDot, do we really want to show the popup window? Because there is another case to create a workspace, I think if the user opens the desktop app from the popup, the workspace should not be created repeatedly, right?

cc @s77rt

[s77rt]

@mallenexpensify Accepted!

---

@ntdiary Right, it's probably best to exclude non-idempotent call to action buttons.

cc @tgolen

[MelvinBot]

@pecanoro, @ntdiary, @s77rt, @mallenexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[s77rt]

Not overdue. Work is being done on https://github.com/Expensify/App/issues/17987