Fix duplicate workspaces being created on every sign in #10122
Conversation
…ened with a /transition deep link
melvin-bot
bot
requested review from
danieldoglas
and removed request for
a team
|
LMK when its good to review! |
…being opened with a /transition deep link" This reverts commit 7890c7c.
…eens for creating Workspaces
| } | ||
| App.setCurrentURL(path); | ||
|
|
||
| this.setState({currentPath}); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think it doesn't matter much in this case because we are preventing re-renders in AuthScreens 😄
| @@ -133,6 +136,10 @@ class AuthScreens extends React.Component { | |||
| } | |||
|
|
|||
| shouldComponentUpdate(nextProps) { | |||
| if (this.props.currentPath !== nextProps.currentPath) { | |||
| App.setUpPoliciesAndNavigate(nextProps.session, nextProps.currentPath); | |||
| } | |||
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
Yeah doing this here in shouldComponentUpdate() is a little micro-optimization. I'll add a comment to make that clear 👍🏽
There was a problem hiding this comment.
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.
Lines 176 to 178 in 0c2c2f6
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh nevermind my old proposal is here in the issue :D https://github.com/Expensify/Expensify/issues/217505#issuecomment-1196040735
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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
|
Oops sorry did not see it was |
arosiclair
force-pushed
the
arosiclair-duplicate-workspaces
branch
from
5f402c5
to
0c2c2f6
Compare
There was a problem hiding this comment.
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.
| @@ -133,6 +136,10 @@ class AuthScreens extends React.Component { | |||
| } | |||
|
|
|||
| shouldComponentUpdate(nextProps) { | |||
| if (this.props.currentPath !== nextProps.currentPath) { | |||
| App.setUpPoliciesAndNavigate(nextProps.session, nextProps.currentPath); | |||
| } | |||
There was a problem hiding this comment.
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.
Lines 176 to 178 in 0c2c2f6
|
Alright more issues 🙃. |
src/libs/actions/App.js
Outdated
| 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; | ||
| } |
There was a problem hiding this comment.
How did you encounter this error. Why didn't we have an error before?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
accountIDissue. Looks like we must create a fullURLobject and pull the search query using thesearchproperty.
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
currentPathwith a dummy domain to do this.
You should use CONST.ACTIVE_EXPENSIFY_URL.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Nice find I'll try that
src/libs/actions/App.js
Outdated
| return; | ||
| } | ||
| if (!isLoggingInAsNewUser && exitTo) { | ||
| Navigation.isNavigationReady() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agree with removing this if we can.
There was a problem hiding this comment.
Getting very close! You can also remove all of this mess for tracking the navigation readiness, including this part.
arosiclair
changed the title
|
🚀 Deployed to staging by @arosiclair in version: 1.1.87-6 🚀
|
|
🚀 Deployed to production by @yuwenmemon in version: 1.1.87-9 🚀
|
|
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:
***@***.***>
|
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
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Same as Tests
Screenshots
Web
Screen.Recording.2022-07-26.at.5.02.34.PM.mov
Mobile Web
Desktop
iOS
Android