Fix login and onboarding flows#2390
Conversation
|
@afzalsayed96 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
This pull request is being automatically deployed with Vercel (learn more). docs – ./apps/docs🔍 Inspect: https://vercel.com/cal/docs/9xhTruzRTJagGHEKF4xoj9L3oDFL [Deployment for 5b6f9ee canceled] calendso – ./apps/web🔍 Inspect: https://vercel.com/cal/calendso/Fi2q2T3YwMfCuuETx26D9dDHxuJV [Deployment in progress for 5b6f9ee] |
| useEffect(() => { | ||
| user && setRedirecting(shouldShowOnboarding(user)); | ||
| }, [router, user]); | ||
| const isRedirectingToOnboarding = user && shouldShowOnboarding(user); |
There was a problem hiding this comment.
I believe state is unnecessary here as any change in query will cause a rerender and isRedirectingToOnboarding can be considered a derived or computed state from query.data
alishaz-polymath
left a comment
alishaz-polymath
left a comment
There was a problem hiding this comment.
Minor change requested, and additional information requested. It is an improvement on the current state.
Co-authored-by: Syed Ali Shahbaz <52925846+alishaz-polymath@users.noreply.github.com>
joeauyeung
left a comment
joeauyeung
left a comment
There was a problem hiding this comment.
Hey, great PR! The transitions are now smooth. Just building off of @alishaz-polymath's question about why some of the class names have changed.
zomars
left a comment
zomars
left a comment
There was a problem hiding this comment.
Reverted unnecessary diffs. Other than that code looks good and tested. Thank you for your contribution 🙏
alishaz-polymath
left a comment
alishaz-polymath
left a comment
There was a problem hiding this comment.
Thank you for your contribution. Tested, works! 🙏
|
@zomars @alishaz-polymath thanks for the review and merging. Good call on the early return 🙌 I usually view diffs in GitHub with the hide whitespaces option checked for the same reason.
|
Thanks for the tip, although locally we don't have that option unfortunately. |
|
@afzalsayed96 Thank you for this contribution. Quite helpful! |
6 participants
What does this PR do?
When you sign up you can notice a slight jitter in the UI with invalid state while redirecting to
/get-startedroute.Screen.Recording.2022-04-05.at.11.41.35.PM.mov
Similarly when you navigate to a protected page such as
/event-typeswithout being logged in you can see a glimpse of the invalid stateScreen.Recording.2022-04-06.at.12.06.06.AM.mov
The root cause of this issue is how
useRedirectToLoginIfUnauthenticatedanduseRedirectToOnboardingIfNeededhooks are implemented. They both employuseEffecthook to redirect. However,useEffectis run after a render is completed. Hence, we see a glimpse of invalid state as the loading of session/me query finishes butuseEffectis yet to run.The solution is for unauth redirection is simply to check for
sessionvalue and only then render protected UI. And similarly for onboarding user redirect, it's imperative to check both the result & loading state ofmequery before rendering anything.With the above changes, we make sure that protected UI is only shown after consulting the appropriate loading states and the data itself and we get a clean jitter-free transition.
Type of change
How should this be tested?
Unsure