fix(web): fall back to redirect sign-in when the popup is blocked - #4118
Merged
Conversation
signInWithPopup awaits gapi + the /__/auth iframe ping before calling window.open; on prod the proxied authDomain makes that slow enough to outlive Chrome's 5s user-activation budget, so window.open returns null and sign-in fails with auth/popup-blocked. Use the standard pattern instead: try popup first everywhere, and on auth/popup-blocked fall back to signInWithRedirect, which needs no popup permission and works via the same-origin /__/auth proxy. This one path also covers iOS Safari (blocks popups by default), so drop the useOs/isMobile sniffing and apps/web's @mantine/hooks dependency. Also gitignore .claude/settings.json (local plugin config). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
googleSignIn, you only fall back to redirect forauth/popup-blocked; consider whether other popup-related Firebase errors (e.g. environment-specific codes) should trigger the same fallback instead of being rethrown to improve robustness across browsers. - Since
googleSignInno longer takes anisMobileflag, double-check any other callers or platform-specific assumptions to ensure there are no remaining code paths that expect mobile-only redirect behaviour.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `googleSignIn`, you only fall back to redirect for `auth/popup-blocked`; consider whether other popup-related Firebase errors (e.g. environment-specific codes) should trigger the same fallback instead of being rethrown to improve robustness across browsers.
- Since `googleSignIn` no longer takes an `isMobile` flag, double-check any other callers or platform-specific assumptions to ensure there are no remaining code paths that expect mobile-only redirect behaviour.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Check if the Pull Request fulfils these requirements
Greptile Summary
The PR changes Google authentication to try popup sign-in first and fall back to redirect sign-in when Firebase reports that the popup was blocked.
/web-ext..gitignore.Confidence Score: 5/5
The PR appears safe to merge with no concrete changed-code defect identified.
The popup-blocked path now transitions to the existing redirect flow, redirect results remain processed after reload, and the removed OS-detection dependency is no longer referenced.
Reviews (1): Last reviewed commit: "fix(web): fall back to redirect sign-in ..." | Re-trigger Greptile
Context used: