fix: OAuth Button Loader State Not Reset on Browser Back Navigation. #2568
Conversation
|
|
|
|
@Srayash is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
apps/dashboard/app/auth/sign-up/oauth-signup.tsx (2)
Line range hint
27-30: Enhance error handling with more specific error messages.While the error handling covers the basics, it could be more informative and type-safe.
Consider this enhanced implementation:
- } catch (cause) { - console.error(cause); + } catch (error: unknown) { + console.error('[OAuth Error]:', { + provider, + error, + timestamp: new Date().toISOString() + }); setIsLoading(null); - toast.error("Something went wrong, please try again."); + toast.error( + error instanceof Error + ? `Authentication failed: ${error.message}` + : "Something went wrong with authentication, please try again." + );
Line range hint
34-48: Prevent potential race conditions and improve accessibility.The current implementation could be susceptible to race conditions from rapid clicks, and the loading state could be more accessible.
Consider these improvements:
- <OAuthButton onClick={() => oauthSignIn("oauth_github")}> + <OAuthButton + onClick={() => oauthSignIn("oauth_github")} + disabled={isLoading !== null} + aria-busy={isLoading === "oauth_github"} + > {isLoading === "oauth_github" ? ( - <Loading className="w-6 h-6" /> + <Loading className="w-6 h-6" aria-hidden="true" /> ) : ( <GitHub className="w-6 h-6" /> )} GitHub </OAuthButton> - <OAuthButton onClick={() => oauthSignIn("oauth_google")}> + <OAuthButton + onClick={() => oauthSignIn("oauth_google")} + disabled={isLoading !== null} + aria-busy={isLoading === "oauth_google"} + >apps/dashboard/app/auth/sign-in/oauth-signin.tsx (2)
Line range hint
19-33: Consider implementing navigation state management.The current implementation might benefit from a more robust navigation state management approach. Consider using React's
useEffectfor cleanup:const oauthSignIn = async (provider: OAuthStrategy) => { if (!signInLoaded) { return null; } try { setIsLoading(provider); await signIn.authenticateWithRedirect({ strategy: provider, redirectUrl: "/auth/sso-callback", redirectUrlComplete: "/apis", }); - setIsLoading(null); setLastUsed(provider === "oauth_google" ? "google" : "github"); } catch (err) { console.error(err); setIsLoading(null); toast.error((err as Error).message); } }; + + React.useEffect(() => { + return () => { + // Reset loading state when component unmounts (including navigation) + setIsLoading(null); + }; + }, []);
28-28: Ensure consistent state management.The
setLastUsedcall aftersetIsLoading(null)might be unnecessary if navigation occurs, as the component will unmount.Consider moving the
setLastUsedcall beforesetIsLoading(null)to maintain a more logical state update sequence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/dashboard/app/auth/sign-in/oauth-signin.tsx (1 hunks)
- apps/dashboard/app/auth/sign-up/oauth-signup.tsx (1 hunks)
🔇 Additional comments (2)
apps/dashboard/app/auth/sign-up/oauth-signup.tsx (1)
Line range hint
1-48: Verify consistent implementation across sign-in and sign-up flows.Let's ensure the proposed changes are consistently implemented across both OAuth components.
✅ Verification successful
Let me gather more information about the implementation details of both OAuth components.
Let me try a different approach to get the OAuth SignIn implementation:
Let me try one final search to get the complete implementation of the OAuth SignIn component:
Inconsistency found in redirect URLs between sign-in and sign-up flows
The verification revealed that while both OAuth components use the same
/auth/sso-callbackforredirectUrl, they have differentredirectUrlCompletevalues:
- Sign-up: redirects to
/new- Sign-in: redirects to
/apisThis difference appears intentional as it directs users to different pages based on their authentication flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent implementation across OAuth components # and verify OAuthButton component properties # Check OAuth components implementation echo "Checking OAuth components implementation..." rg -l "OAuthSign(In|Up)" apps/dashboard/app/auth/ # Verify OAuthButton component properties echo "Checking OAuthButton component properties..." ast-grep --pattern 'interface OAuthButtonProps { $$$ }'Length of output: 517
Script:
#!/bin/bash # Check implementation details of both OAuth components echo "Checking OAuth SignIn implementation..." rg -A 30 "export function OAuthSignIn" apps/dashboard/app/auth/sign-in/oauth-signin.tsx echo "\nChecking OAuth SignUp implementation..." rg -A 30 "export function OAuthSignUp" apps/dashboard/app/auth/sign-up/oauth-signup.tsx # Check OAuthButton implementation echo "\nChecking OAuthButton implementation..." rg -A 10 "export function OAuthButton" apps/dashboard/app/auth/oauth-button.tsxLength of output: 1461
Script:
#!/bin/bash # Using ast-grep to find the OAuth components implementation echo "Checking OAuth SignIn implementation..." ast-grep --pattern 'export function OAuthSignIn() { $$$ }' echo "Checking OAuthButton implementation..." ast-grep --pattern 'export function OAuthButton($$$) { $$$ }' # Also check the imports to ensure consistency echo "Checking imports in OAuth SignIn..." rg "^import.*from" apps/dashboard/app/auth/sign-in/oauth-signin.tsx echo "Checking imports in OAuth SignUp..." rg "^import.*from" apps/dashboard/app/auth/sign-up/oauth-signup.tsxLength of output: 1331
Script:
#!/bin/bash # Search for the complete OAuth SignIn implementation with more context echo "Checking complete OAuth SignIn implementation..." rg -U "export function OAuthSignIn.*?}" apps/dashboard/app/auth/sign-in/oauth-signin.tsx -A 50 -m 1 # Also check the redirectUrl and redirectUrlComplete values echo "Checking redirect URLs usage..." rg "redirectUrl.*|redirectUrlComplete.*" apps/dashboard/app/auth/Length of output: 706
apps/dashboard/app/auth/sign-in/oauth-signin.tsx (1)
27-27: Verify the timing of loading state reset.While setting
isLoadingto null after redirect initiation addresses the back navigation issue, it might cause the loading spinner to disappear prematurely during a successful authentication flow.Consider these alternative approaches:
- Use
beforeunloadevent listener to reset state on navigation- Implement a cleanup function in a
useEffecthook
Srayash
changed the title
Srayash
force-pushed
the
Srayash/Issue#2536
branch
from
05a3cbb
to
c351091
Compare
|
hey @chronark kindly review and give feedback or merge. |
|
Awarding Srayash: 50 points 🕹️ Well done! Check out your new contribution on oss.gg/Srayash |
What does this PR do?
set Loader state to null after the await is over.
Fixes #2536
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit