-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 spinner on OldDot transition #5552
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solution makes sense to me. I am a little worried that this code might be difficult to understand looking back so it might be good to add additional comments in the shouldComponentUpdate()
method.
@@ -235,11 +231,15 @@ class AuthScreens extends React.Component { | |||
return true; | |||
} | |||
|
|||
if (isTransitionOrNewWorkspacePage(this.props.currentURL) && !isTransitionOrNewWorkspacePage(nextProps.currentURL)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add some comments here to explain this check? I see that we want to prevent updating AuthScreens
when moving from /transition
or /workspace/new
to another route, but unsure why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, my reasoning here was since I added the currentURL
Onyx key, that will change any time we navigate anywhere:
- Opening settings modal
- Switching reports
- Etc...
And by default we probably don't want to re-render the whole component tree from AuthScreens
down every time the route changes. The only time we do want to force a re-render and trigger componentDidUpdate
is when we are moving from transition
or workspace/new
to anywhere else, because that means it's now safe to load policies without risking overwriting a newly-created policy. Overall this design doesn't feel great, but I think it works.
If that all makes sense, I could add a comment like this:
/*
* We don't want to re-render this component every time the `currentURL` prop changes,
* but we do want to re-render when we're switching from `transition` or `workspace/new` to any other page.
* Re-rendering in that case will re-trigger `componentDidUpdate` and `loadPoliciesBehindBeta`,
* which we only want to do after we're done with the `transition` and `workspace/new` pages.
* If we don't wait to load the policies until after we're done with those pages,
* we may accidentally overwrite the newly-created policy and land on an infinite loading spinner.
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way we could maybe simplify this code without breaking the E/App philosophy would be to trigger this.loadPoliciesBehindBeta
directly here in shouldComponentUpdate
, but in general I'm hesitant to put side-effects in shouldComponentUpdate
. Not sure I can explain why exactly, it just feels like a bad pattern. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to leaving the above comment for an explanation but I also think it could use a bit of re-working and additional lines to help with clarity. e.g.
We don't want to re-render this component every time the
currentURL
prop changes because xyz
* We want to only load policy info if: | ||
* - We are on the free plan beta | ||
* - We are not on the transition or new-workspace pages (if we load policies while creating a new one we may accidentally overwrite the new policy due to race condition) | ||
* - We have not already loaded policies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we just keep calling this method anytime the component updates until all the conditions are met?
Seems fine but also kind of inside out to me. This might be off base, but I feel like we are starting to get into some territory where Onyx's async philosophy is hurting us in the code readability department.
Would there be any worry about a race condition or even a need to explain it if we could have something like...
const promises = [getBetas()];
if (shouldCreateWorkspace) {
promises.push(createPolicy());
}
Promise.all(promises)
.then(() => {
getPolicyList();
getPolicySummaries();
});
Not sure if we can actually do this or where this logic would run. We can't do it in componentDidMount()
because we are probably on /transition
and not /workspace/new
when that happens. Then again we don't have to switch to a route called /workspace/new
and could instead trigger policy creation with a parameter instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I agree, and was suggesting something similar in this thread. In cases when we're just displaying a full-screen loading indicator while waiting for things to happen, our Onyx async philosophy doesn't seem to serve us well/leads to overly-complicated code.
I think that this whole flow could be simplified by just consolidating everything into a single /transition
page (i.e: no workspace/new
). It would just display a loading indicator, and depending on URL params would do any number of things that need to happen before we can actually display the app. It waits for all those things to be done using promises, and then displays the exitTo
page or falls back on the homepage (optionally with an error growl).
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this conversation here.
Just double-checking @roryabraham so that I'm not holding anything up, is this PR ready to be reviewed again or are we waiting for #5614 to get merged? |
Sorry for the lackluster communication here @nickmurray47 – discussion is ongoing in the linked issue |
Closing this out in preference of #5706 |
Details
This PR fixes two related race conditions:
LoginWithShortLivedTokenPage
race condition explanationSession.signInWithShortLivedToken
API.createLogin
or betas load, LogInWithShortLivedTokenPage immediately runsthis.setState({hasRun: true});
componentDidUpdate
,this.state.hasRun
is true, and then we are stuck in LogInWithShortLivedTokenPage / infinite spinner forever.LoginWithShortLivedTokenPage
race condition fixGet rid of
hasRun
state just navigate toexitTo
once betas are loaded and we are logged into the correct account.AuthScreens
race condition explanationAuthScreens
race condition fixDon't load policies until after we've finished with the
transition
andworkspace/new
pages. That way, the new workspace won't be overwritten.Fixed Issues
$ #5518
Tests
setTimeout
of1000
around these lines. This consistently reproduced theAuthScreens
race condition before these changes.Set up my company for free
.QA Steps
freeCard
beta, such aschivito.pw
Set up my company for free
Tested On
Screenshots
Web
Mobile Web
Screen.Recording.2021-09-27.at.6.15.58.PM.mov
Desktop
iOS
Android