Fix duplicate workspaces being created on every sign in by arosiclair · Pull Request #10122 · Expensify/App

arosiclair · 2022-07-26T21:01:11Z

After transitioning from OldDot to NewDot to create a new Workspace, signing out and signing back in would continue creating a new workspace. This PR addresses this by not relying on the initial URL

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/217505

Tests

Log into a new OldDot account with no policies
Use the "Get started with our free plan" inbox task to create a Workspace
Verify the workspace is successfully created in NewDot
Sign out of NewDot
Log back in and verify another workspace is NOT created
Sign out again and leave the tab open
Return to OldDot and navigate to Settings > Policies > Group
Create another free plan policy
Verify only 1 additional workspace (2 in total) is created in the second NewDot tab.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Same as Tests

Verify that no errors appear in the JS console

Screenshots

Web

Screen.Recording.2022-07-26.at.5.02.34.PM.mov

Mobile Web

Desktop

iOS

Android

…ened with a /transition deep link

danieldoglas · 2022-07-26T21:20:39Z

LMK when its good to review!

…being opened with a /transition deep link" This reverts commit 7890c7c.

…eens for creating Workspaces

marcaaron · 2022-07-28T00:25:53Z

        }
-        App.setCurrentURL(path);
+
+        this.setState({currentPath});


NAB, I still feel icky about triggering a re-render of the NavigationContainer - but can't really back up this concern with any facts or evidence about why we should not do it so not gonna block on it.

I see what you mean but for the record, child components only re-render if their props change. So it's safe to re-render the root component (or any super high ancestor) of an app so long as it doesn't cascade a massive change of props down the whole app.

I think it doesn't matter much in this case because we are preventing re-renders in AuthScreens 😄

marcaaron · 2022-07-28T00:27:26Z

    shouldComponentUpdate(nextProps) {
+        if (this.props.currentPath !== nextProps.currentPath) {
+            App.setUpPoliciesAndNavigate(nextProps.session, nextProps.currentPath);
+        }


Ah haha. My immediate thought was that it would be better to do this in componentDidUpdate() vs shouldComponentUpdate(). As latter is used to tell the component whether it should re-render and not really for side effects (though I guess we are doing similar stuff elsewhere :oh-nothing:).

The code on line 143 is preventing the AuthScreens from updating when the currentPath changes (which solves my concern shared in the original issue about unnecessary re-rendering). But also explains why we are not using componentDidUpdate() because it will only update if the isSmallScreenWidth props change. 😄

Another option would be to allow the component to update when the currentPath changes and then we could use componentDidUpdate(), but that would trigger a re-render.

If we still prefer this logic to be here and not in the NavigationRoot I'd maybe add a comment or two to explain why we are doing this and why it needs to happen in shouldComponentUpdate()

Yeah doing this here in shouldComponentUpdate() is a little micro-optimization. I'll add a comment to make that clear 👍🏽

So to make sure that I understand this correctly, App.setUpPoliciesAndNavigate will be called once the NavigationRoot passes down the currentPath, which ensures that it only runs when the navigation is ready and the user is signed in. If that's correct then it looks good to me.

I think that would make this redundant, but it's probably wise to leave the check just in case.

App/src/libs/actions/App.js

Lines 176 to 178 in 0c2c2f6

if (!session || !currentPath) {

return;

}

Actually wouldn't we be calling this any time the path changes? 🤔 and isn't that like every state change for the navigator? We just want it to run once though don't we?

I just came across this code today while investigating #10271 and I think it's a bad idea to put this logic inside of shouldComponentUpdate(). It's an anti-pattern (not what the purpose of the lifecycle method is for) and adds a side-effect in a place that's not really appropriate.

It also leads to this:

Actually wouldn't we be calling this any time the path changes? 🤔 and isn't that like every state change for the navigator? We just want it to run once though don't we?

Which I think is a bad idea and not what is wanted.

Do you mean this proposal? Not sure if you can see it or not. I can see it from the conversation page of the PR.

I was afraid someone was gonna ask me for a suggestion on how to fix it 😓

Honestly, I'm a bit lost at everything that was done in this PR and I don't really understand how it fixes the original problem. I am going back now to read the GH closer as I see you struggled with understanding this in the beginning too. I'll see if I can follow along there.

OK, I think I followed some of that. It looks like at some point in the conversation everyone agreed that window.location.href would work the best and be the most simple, but then, it was never attempted?

I think we should go that route. I agree with you that on native platforms, it can be a no-op and that's fine.

// index.js function getCurrentUrl() { return window.location.href; } // index.native.js function getCurrentUrl() { return ''; }

I wouldn't mind seeing how simple that would make all of this. Would it mean that all the changes in this PR can be reverted? I think that would be great if it's possible.

Oh nevermind my old proposal is here in the issue :D https://github.com/Expensify/Expensify/issues/217505#issuecomment-1196040735

@arosiclair as a matter of cleanup, could you look into switching this to window.location.href and see if that could simplify things and remove this side-effect?

Yeah we can take the window.location.href approach since this can be messy. It's be a pretty simple change so I'll submit another PR and add you guys as reviewers

marcaaron · 2022-07-28T00:29:13Z

Oops sorry did not see it was [WIP]. What @danieldoglas said ! 😄

neil-marcellini

Looks great, just need to update one comment. Thanks for fixing this!

To be extra careful it would be good to do the regression testing yourself, but QA will test on staging regardless so it's probably fine.

neil-marcellini · 2022-07-28T18:24:11Z

    shouldComponentUpdate(nextProps) {
+        if (this.props.currentPath !== nextProps.currentPath) {
+            App.setUpPoliciesAndNavigate(nextProps.session, nextProps.currentPath);
+        }


So to make sure that I understand this correctly, App.setUpPoliciesAndNavigate will be called once the NavigationRoot passes down the currentPath, which ensures that it only runs when the navigation is ready and the user is signed in. If that's correct then it looks good to me.

I think that would make this redundant, but it's probably wise to leave the check just in case.

App/src/libs/actions/App.js

Lines 176 to 178 in 0c2c2f6

if (!session || !currentPath) {

return;

}

arosiclair · 2022-07-28T20:08:09Z

Alright more issues 🙃. URLSearchParams is unsupported on iOS. So to fix it I silenced the error with a try-catch and added an early return for non-transition links. What do you guys think? Since this is a web flow should we leave it as is or dig for a cross-platform package/polyfill for parsing params?

danieldoglas

LGTM

neil-marcellini · 2022-07-28T20:17:16Z

+    try {
+        const params = new URLSearchParams(currentPath);
+        exitTo = params.get('exitTo');
+    } catch (error) {
+        // URLSearchParams is unsupported on iOS so we catch th error and
+        // silence it here since this is primarily a Web flow
+        return;
+    }


How did you encounter this error. Why didn't we have an error before?

On a side note, the currentPath is not a URL and I wonder if that makes a difference. I did some tests on this string in the JS console and it works fine to access the params except for accessing the accountID, which I don't think we do here.
E.x

Neil currentPath = /transition?accountID=5&email=neil%2Btrand2%40gmail.com&encryptedAuthToken=9BKwuqHcEe%2FMs1kslRzP5bp3duTochIFdP2uuv0Gbzmg6%2BJkbqb4I%2BWFaBdT%2Bxlj9xx6rQ%2FJqynKnl8FwtsILbGa4QilsHS10tVihZiyMpRQCAYyN9u3oZF3izhC63%2Foy4HDfqfeidzoVvNIGtLuM1fSGqXQTMya0Mxppuk6IwnQG5Mh4E0e49fZPW5RMIovKfHOyMh75dN4ike11aN2emz8cXBysmg9VjJN2rNcXlN04DDS5UZhwFzJrNbrCJGKKTi5fsk70X53ogw2hBXiee3N%2FfH6pGRVlN6MBqzjknb3UcUz5K6L6al5EJSFlzvYiXJhOb2lXiiOSOcIPwU01vZ3B0RgI8WkDYhpYW6gOm06%2Fo%2BucO9IHqjJlHfRoVoNDx%2FmSAOIpAM156sdMISDpGUAQ0H1tshQo%2BC9CcJ9SUY2QV%2BiZo5PrrYwTC2Z3OL8c%2FwPA6dFCWnJ8H1gV1CyiOKVrGzlQnzLB6EKNfZJMGzTgWtkdZfBra86IPUsuxkJ0JC7jvS306w3PjhkxQZNZg9dMEWgbCOXKfuxROFzcRdMphVoeLQrhTR8STRVQXOo6eNstl3P4BmLe9X5jITxWk2kzFrePgo%2BPrPmG4DaQVfzuUhi5xBMtsI64QAbWiFllx%2BH3fx9GAC3a3JOa7%2B9lVOs3o5oSJRX1%2F9FYfjPFYtGVzDGpSP9hwrUjSCCZHP%2FZJF9eiY9W6poJtTkke%2BCWAHEhDKANoWwGyJNvCxBVvTrzcmodyt0QrKBXkXRHFvODBgdHkEC99HhxPIiUjoGOymQys%2BLWHUdNc0ImybLBNmz5rNS5TRLa9Tj%2FkdF5tQ3Z%2BVKJBjsjfV8iiS%2B9bWUikm5oJ5t%2FFs5bB1bANbIq71ODsxFFzDiPDyCUkY8w5r%2B&exitTo=workspace%2Fnew&shortLivedToken=CF2A83E7D96D2ABF7C7A5E7E32CB9E6099BC2B4FF801E1F5A9C682050037D7B0E38750E8FC325B270EFF41741709391C0AE662A29B753DE1B96AC3833719C119AC75954486297163B3EC7C4FB00C3CDA4265A2634F330248F6A8EC983ED9A5921FF2D137653C812AD78DEBA32855DA84C1CC126ECCFBBFDA3F40D2AF0D61A1BD7407EFCA472E47E1E7EA153C7530FDDB7DBD30C311E13422F82208AB2EEB52F2D78A7CC9A53A83EC177E50276F3B1376B7482EF1F97577B9999EF6CED61F26F454B17A484661CF0411E33D1DB968D9E09D187AC74BE080CC0F066260259A36DABAB4DE4C8830104CF19AC172087EBB0992D5EC0262C26770A02B7212033C7287B810D51CDE518FF933D6B5758B3FF9043463B65D60450AC7EE60CF97C8B832CA15245A0FEB64422B9FDAD1929F676B1A

Since we were null checking the url and the url for Linking.getInitialUrl() is always null on native, iOS never hit this code before.

I'm seeing your accountID issue. Looks like we must create a full URL object and pull the search query using the search property. I'll combine the currentPath with a dummy domain to do this.

Since we were null checking the url and the url for Linking.getInitialUrl() is always null on native, iOS never hit this code before.

Aaaah ok, that makes sense.

I'm seeing your accountID issue. Looks like we must create a full URL object and pull the search query using the search property.

At this point accountID doesn't matter but it would be good to have a solution that works for all params in case we need it in the future. Why do we need to create a URL object? We could create a url string and pass in into `URLSearchParams'. I don't know what the convention is here.

I'll combine the currentPath with a dummy domain to do this.

You should use CONST.ACTIVE_EXPENSIFY_URL.

I played around with it more in the JS console and it looks like this SO answer is the way to go. I don't think the "bugs" they mention are a problem.

Nice find I'll try that

neil-marcellini · 2022-07-28T20:33:39Z

+        return;
+    }
+    if (!isLoggingInAsNewUser && exitTo) {
+        Navigation.isNavigationReady()


NAB: We can probably get rid of this now that it only runs once the navigation is ready. You'll have to test. Could be a follow up PR.

Agree with removing this if we can.

neil-marcellini

Getting very close! You can also remove all of this mess for tracking the navigation readiness, including this part.

neil-marcellini

~~I approve, other than this one little thing.~~ Edit: never mind it was a glitch with "View changes" since last viewed.

neil-marcellini

Good to go.

OSBotify · 2022-08-02T14:22:41Z

🚀 Deployed to staging by @arosiclair in version: 1.1.87-6 🚀

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

OSBotify · 2022-08-04T20:50:30Z

🚀 Deployed to production by @yuwenmemon in version: 1.1.87-9 🚀

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

tgolen · 2022-10-11T08:12:11Z

Thank you!

…

On Mon, Aug 22, 2022 at 4:37 PM Andrew Rosiclair ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/libs/Navigation/AppNavigator/AuthScreens.js <#10122 (comment)>: > @@ -133,6 +136,10 @@ class AuthScreens extends React.Component { } shouldComponentUpdate(nextProps) { + if (this.props.currentPath !== nextProps.currentPath) { + App.setUpPoliciesAndNavigate(nextProps.session, nextProps.currentPath); + } Yeah we can take the window.location.href approach since this can be messy. It's be a pretty simple change so I'll submit another PR and add you guys as reviewers — Reply to this email directly, view it on GitHub <#10122 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMAB3WHEOTZHEJPMWQCE3V2OGCLANCNFSM54XKGCTA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

fix AuthScreens creating a new policy on every sign in after being op…

7890c7c

…ened with a /transition deep link

arosiclair requested a review from a team as a code owner July 26, 2022 21:01

arosiclair self-assigned this Jul 26, 2022

melvin-bot Bot requested review from danieldoglas and removed request for a team July 26, 2022 21:01

arosiclair added 2 commits July 27, 2022 16:14

Revert "fix AuthScreens creating a new policy on every sign in after …

0dc0ebd

…being opened with a /transition deep link" This reverts commit 7890c7c.

track currentPath in NavigationRoot state and pass it down to AuthScr…

2a0fd70

…eens for creating Workspaces

arosiclair requested review from marcaaron and neil-marcellini July 27, 2022 21:58

marcaaron requested changes Jul 28, 2022

View reviewed changes

arosiclair added 2 commits July 28, 2022 12:44

comment about shouldComponentUpdate optimization

e8faa4d

remove unused CURRENT_URL Onyx key

0c2c2f6

arosiclair force-pushed the arosiclair-duplicate-workspaces branch from 5f402c5 to 0c2c2f6 Compare July 28, 2022 16:52

marcaaron previously approved these changes Jul 28, 2022

View reviewed changes

neil-marcellini requested changes Jul 28, 2022

View reviewed changes

arosiclair added 3 commits July 28, 2022 15:49

fix crash for URLSearchParams on iOS

3478593

update jsdoc

cbf8db0

early return if there's no exitTo param

e5081a2

arosiclair dismissed marcaaron’s stale review via e5081a2 July 28, 2022 19:59

arosiclair requested review from marcaaron and neil-marcellini July 28, 2022 20:08

danieldoglas previously approved these changes Jul 28, 2022

View reviewed changes

neil-marcellini reviewed Jul 28, 2022

View reviewed changes

arosiclair added 2 commits July 29, 2022 17:15

improved url search params parsing

d5178b3

remove unnecessary navigation ready callback

e891e71

arosiclair dismissed danieldoglas’s stale review via e891e71 July 29, 2022 21:26

arosiclair requested a review from neil-marcellini July 29, 2022 21:30

arosiclair requested a review from danieldoglas July 29, 2022 21:30

neil-marcellini reviewed Jul 29, 2022

View reviewed changes

Comment thread src/libs/actions/App.js

arosiclair added 2 commits August 1, 2022 12:15

removed unused navigation ready code

2dc22a4

Merge branch 'main' into arosiclair-duplicate-workspaces

374dbd0

arosiclair changed the title ~~[WIP] Fix duplicate workspaces being created on every sign in~~ Fix duplicate workspaces being created on every sign in Aug 1, 2022

arosiclair requested a review from neil-marcellini August 1, 2022 17:06

neil-marcellini reviewed Aug 1, 2022

View reviewed changes

neil-marcellini approved these changes Aug 1, 2022

View reviewed changes

arosiclair merged commit 0668878 into main Aug 2, 2022

arosiclair deleted the arosiclair-duplicate-workspaces branch August 2, 2022 13:07

OSBotify mentioned this pull request Aug 2, 2022

Deploy Checklist: New Expensify 2022-08-01 #10169

Closed

95 tasks

tgolen mentioned this pull request Aug 17, 2022

mweb - OlDdot - User gets infinite loading when opening a WS from old dot app #10271

Closed

Conversation

arosiclair commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

danieldoglas commented Jul 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Jul 28, 2022

Uh oh!

neil-marcellini left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arosiclair commented Jul 28, 2022

Uh oh!

danieldoglas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neil-marcellini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arosiclair commented Jul 26, 2022 •

edited

Loading

neil-marcellini left a comment •

edited

Loading