fix(auth): disable Google sign-in button until OAuth script is ready - #8201
fix(auth): disable Google sign-in button until OAuth script is ready#8201bardock-2393 wants to merge 2 commits into
Conversation
useGoogleLogin from @react-oauth/google only returns the login callback, not a ready flag, so clicking before the Google script finishes loading was a silent no-op. Track readiness via GoogleOAuthProvider's onScriptLoadSuccess/onScriptLoadError and disable the button until then.
|
@bardock-2393 is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
github-actions
Bot
added
the
front-end
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 218b944d-d3b7-4291-b097-d4fd50149dd6
📒 Files selected for processing (2)
frontend/web/components/GoogleButton.tsxfrontend/web/components/pages/HomePage.tsx
| disabled={!ready} | ||
| onClick={() => ready && login()} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add regression coverage for the readiness guard.
Before readiness, a click must not invoke login. After readiness, a click must invoke login. Also cover the script-error path so the button remains disabled.
There was a problem hiding this comment.
Leaving this as-is — there's no existing Jest/RTL test coverage anywhere in this repo for GoogleButton or its sibling auth components, and adding new test scaffolding for a single small component would be disproportionate to this fix's scope. Verified the fix manually via code reading (real useGoogleLogin/GoogleOAuthProvider type signatures) instead.
There was a problem hiding this comment.
@bardock-2393 Acknowledged. The current guard is small and directly matches the OAuth readiness contract. Adding test infrastructure only for this change is not necessary.
You are interacting with an AI system.
Optional meant an omitted prop was indistinguishable from "not ready yet" at the type level, silently leaving a future caller's button permanently disabled. There's currently one caller and it already passes ready explicitly.
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #6554
GoogleButtoncalledlogin()fromuseGoogleLoginunconditionally on click. On slow connections, the Google Identity Services script hasn't loaded yet, so the internal token clientuseGoogleLogindepends on isn't ready, and clicking throwsTypeError: Cannot read properties of undefined (reading 'requestAccessToken').useGoogleLoginin the installed version of@react-oauth/google(^0.2.8) only returns the login callback — it doesn't expose a readiness flag itself.GoogleOAuthProviderdoes exposeonScriptLoadSuccess/onScriptLoadErrorcallbacks, soHomePagenow tracks script-load state via those and passes it down toGoogleButtonas areadyprop, which disables the button (and no-ops the click handler defensively) until the script has actually loaded.How did you test this code?
Manually, since this needs a real network to reproduce (slow connection to Google's GSI script). Verified via code reading that:
useGoogleLogin's real type signature (checkednode_modules/@react-oauth/google/dist/index.d.ts) only returns the callback, confirming the issue's originally suggested{ login, ready }destructure doesn't exist for this library version.GoogleOAuthProvideris used in exactly one place in the app (HomePage.tsx), so threading thereadyprop through doesn't affect any other consumer.npx eslint --fixon both touched files — clean.npm run typecheck— confirmed identical error count with and without this change (pre-existing unrelated errors only), so this introduces zero new type errors.