fix: make Google auth flow reliable and success-gated#193
fix: make Google auth flow reliable and success-gated#193VarshiniGunti wants to merge 3 commits intoOpenLake:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAuthentication error handling is improved across three components. AuthContext now provides robust Google OAuth token extraction with fallback logic, user-facing alerts for errors, and boolean return values to indicate success or failure. Login and Register components conditionally navigate based on authentication outcomes rather than unconditionally. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
VarshiniGunti
force-pushed
the
fix/google-auth-login-flow
branch
from
79affb1 to
a013b18
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR hardens the Google authentication flow to be success-gated, ensuring navigation only occurs after a successful auth exchange with the backend. Previously, SignInWithGoogle and SignUpWithGoogle could allow navigation even on failure.
Changes:
SignInWithGoogleandSignUpWithGoogleinAuthContext.jsxnow returnboolean(true/false) instead of the Firebase response, with guards for missing Firebase config, missing access tokens, and safe JSON parsing.Login.jsxandRegister.jsxnow conditionally navigate only when the Google auth helper returnstrue.- Removed
navigate()calls from inside the auth helpers, delegating navigation responsibility to the calling components.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
app/src/Context/AuthContext.jsx |
Refactored SignInWithGoogle/SignUpWithGoogle to return boolean success, added token extraction guard, removed internal navigation |
app/src/components/Login.jsx |
Gates navigation on SignInWithGoogle returning true; minor whitespace fix |
app/src/components/Register.jsx |
Gates navigation on SignUpWithGoogle returning true |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/Context/AuthContext.jsx (1)
253-256: Consider usingconsole.errorfor error logging.Using
console.errorinstead ofconsole.logfor caught exceptions improves debuggability by marking them as errors in browser dev tools.♻️ Suggested change
} catch (error) { - console.log(error); + console.error("Google sign-in error:", error); alert("Please try logging in again"); return false; }Same applies to line 302 in
SignUpWithGoogle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/Context/AuthContext.jsx` around lines 253 - 256, Replace the generic console.log calls in the catch blocks with console.error to properly surface exceptions; specifically update the catch in the sign-in flow (the catch block shown around the return false in AuthContext.jsx) and the catch in SignUpWithGoogle so they call console.error(error) instead of console.log(error), leaving the existing alert and return behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/Context/AuthContext.jsx`:
- Around line 268-281: The code is sending the OAuth access token to the backend
(credential?.accessToken / accessToken) instead of the Firebase ID token; update
the registration flow in the same function that creates credential and calls
fetch (symbols: GoogleAuthProvider.credentialFromResult, credential,
accessToken, regresponse) to obtain the ID token (either credential.idToken or
await response.user.getIdToken()) and send that idToken in the POST body to
/api/register/google/ (replace token: accessToken with token: idToken) so the
backend receives the Firebase ID token.
- Around line 221-234: The code is sending the Google OAuth access token
(credential?.accessToken) to the backend, but Firebase Admin expects a Firebase
ID token; replace use of credential?.accessToken (and any variable named
accessToken) with the Firebase ID token obtained from the signed-in user (await
response.user.getIdToken()) and send that ID token in the POST body to
BACKEND+"/api/token/google/"; apply the same change in both places referenced
(the block using GoogleAuthProvider.credentialFromResult(response) and the
SignUpWithGoogle block) so the backend's verify_id_token() receives a Firebase
ID token.
---
Nitpick comments:
In `@app/src/Context/AuthContext.jsx`:
- Around line 253-256: Replace the generic console.log calls in the catch blocks
with console.error to properly surface exceptions; specifically update the catch
in the sign-in flow (the catch block shown around the return false in
AuthContext.jsx) and the catch in SignUpWithGoogle so they call
console.error(error) instead of console.log(error), leaving the existing alert
and return behavior intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff036621-6798-4016-9714-691f61b98b50
📒 Files selected for processing (3)
app/src/Context/AuthContext.jsxapp/src/components/Login.jsxapp/src/components/Register.jsx
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.Unrecognized words (26)These words are not needed and should be removedCRA leetcoderankingccpsSome files were automatically ignored 🙈These sample patterns would exclude them: You should consider adding them to: File matching is via Perl regular expressions. To check these files, more of their words need to be in the dictionary than not. You can use To accept these unrecognized words as correct, update file exclusions, and remove the previously acknowledged and now absent words, you could run the following commands... in a clone of the git@github.com:VarshiniGunti/Leaderboard-Pro.git repository curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/main/apply.pl' |
perl - 'https://github.com/OpenLake/Leaderboard-Pro/actions/runs/23126866054/attempts/1' &&
git commit -m 'Update check-spelling metadata'Available 📚 dictionaries could cover words (expected and unrecognized) not in the 📘 dictionaryThis includes both expected items (592) from .github/actions/spelling/expect.txt and unrecognized words (26)
Consider adding them (in with:
extra_dictionaries: |
cspell:django/dict/django.txt
cspell:software-terms/dict/softwareTerms.txt
cspell:python/src/common/extra.txt
cspell:npm/dict/npm.txt
cspell:fullstack/dict/fullstack.txtTo stop checking additional dictionaries, add (in check_extra_dictionaries: ""Warnings
|
| Count | |
|---|---|
| 1 |
See
If you see a bunch of garbage
If it relates to a ...
well-formed pattern
See if there's a pattern that would match it.
If not, try writing one and adding it to the patterns.txt file.
Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.
Note that patterns can't match multiline strings.
binary-ish string
Please add a file path to the excludes.txt file instead of just accepting the garbage.
File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).
Issue Description
Google authentication is failing inconsistently in the frontend flow. In failure scenarios (invalid Firebase setup, missing Google access token, or non-JSON backend response), the app may still proceed in UI flow, leading to confusing behavior and failed login state.
Expected behavior
Actual behavior
Proposed fix
PR Summary
This PR hardens Google auth flow so it is success-gated and failure-safe.
What changed
AuthContext:SignInWithGoogle/SignUpWithGoogleto return boolean success.Login/Register:Result
Summary by CodeRabbit