fix: use redirect sign-in on mobile to fix Safari login (#4067)

[amitsingh-007]

Check if the Pull Request fulfils these requirements

• Does the extension require a version change?

Greptile Summary

This PR fixes Safari (iOS) login by switching from signInWithPopup to signInWithRedirect on mobile, where popups are silently blocked. A useEffect on mount calls getRedirectResult to complete the sign-in after the browser redirects back to the page.

• auth.ts adds an isMobile param to googleSignIn and exports getGoogleRedirectResult wrapping getRedirectResult.
• page.tsx uses Mantine's useOs() to compute isMobile, passes it to googleSignIn, and adds a mount-time effect to pick up the redirect result and trigger preload on return.

Confidence Score: 5/5

Safe to merge — the redirect flow is correctly implemented and the popup path is unchanged.

Both the redirect and popup paths are logically correct. Auth state is recovered via onAuthStateChanged in AuthProvider and via getRedirectResult in the mount effect.

The handleSignIn flow in page.tsx deserves a second look to clarify which codepath sets shouldPreloadData for mobile vs desktop.

_{Reviews (2): Last reviewed commit: "docs: tighten sign-in comments" | Re-trigger Greptile}

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The useEffect that calls getGoogleRedirectResult runs unconditionally on all platforms; consider gating it with isMobile to avoid unnecessary redirect-result checks and error logging on desktop browsers.
• The mobile/desktop branching for Google sign-in is currently split between the page (useOs/isMobile) and the auth helper; consolidating the OS detection into the auth layer (or passing a more explicit mode enum) would reduce coupling and make the sign-in API easier to use correctly from other callers.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `useEffect` that calls `getGoogleRedirectResult` runs unconditionally on all platforms; consider gating it with `isMobile` to avoid unnecessary redirect-result checks and error logging on desktop browsers.
- The mobile/desktop branching for Google sign-in is currently split between the page (`useOs`/`isMobile`) and the auth helper; consolidating the OS detection into the auth layer (or passing a more explicit mode enum) would reduce coupling and make the sign-in API easier to use correctly from other callers.

## Individual Comments

### Comment 1
<location path="apps/web/src/app/helpers/firebase/auth.ts" line_range="19-24" />
<code_context>

-export const googleSignIn = async () => signInWithPopup(auth, provider);
+// Safari mobile blocks the cross-origin popup flow silently, so redirect there.
+export const googleSignIn = async (isMobile: boolean) => {
+  if (isMobile) {
+    await signInWithRedirect(auth, provider);
+    return null;
+  }
+  return signInWithPopup(auth, provider);
+};
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The `googleSignIn` return type now allows `null`, which could surprise other call sites.

In the mobile branch you now `return null` after `signInWithRedirect`, while the non-mobile branch returns `UserCredential` from `signInWithPopup`, changing the function’s type to `Promise<UserCredential | null>`. That’s harmless here because the result is ignored, but other callers may still assume a non-null credential and dereference it. Consider either making `googleSignIn` explicitly fire-and-forget (`Promise<void`) or moving the redirect-specific logic into a separate function so existing call sites don’t unexpectedly receive `null`.

Suggested implementation:

```typescript
const auth = getAuth(firebaseApp);
const provider = new GoogleAuthProvider();

// Safari mobile blocks the cross-origin popup flow silently, so redirect there.
export const googleSignIn = async (isMobile: boolean): Promise<void> => {
  if (isMobile) {
    await signInWithRedirect(auth, provider);
    return;
  }

  await signInWithPopup(auth, provider);
};

```

1. Update all call sites of `googleSignIn` to stop using its return value, since it now returns `Promise<void>` instead of a `UserCredential`.
2. If any caller currently relies on the `UserCredential` (e.g. `const cred = await googleSignIn(...);`), refactor that logic to either:
   - Call `signInWithPopup` directly where the credential is needed, or
   - Introduce a separate helper (e.g. `googleSignInWithCredential`) that preserves the `Promise<UserCredential>` behavior for non-mobile flows.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 24 rules

✅ Skills: e2e-test-generation

1. Breaking googleSignIn contract ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

googleSignIn now requires an isMobile argument and returns null on the redirect path, changing both
the call signature and the return type contract. Callers that previously relied on the
popup-returned credential (or call it from non-TS contexts) will need to be updated or explicitly
handle the redirect/null case.

Code

apps/web/src/app/helpers/firebase/auth.ts[R18-25]

+// Safari mobile blocks the cross-origin popup flow silently, so redirect there.
+export const googleSignIn = async (isMobile: boolean) => {
+  if (isMobile) {
+    await signInWithRedirect(auth, provider);
+    return null;
+  }
+  return signInWithPopup(auth, provider);
+};

Relevance

⭐⭐ Medium

No historical evidence found on enforcing stable helper signatures/return contracts in auth helpers.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR changes googleSignIn to take a mandatory boolean and to return null for the redirect
branch, which is a different signature and return contract than the previous popup-only helper. The
updated call site passes the flag but does not use the return value, masking the contract change for
that specific usage.

apps/web/src/app/helpers/firebase/auth.ts[18-25]
apps/web/src/app/web-ext/page.tsx[70-85]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`googleSignIn` changed from a no-arg popup flow to a conditional redirect/popup flow with a required boolean parameter and a nullable return on redirect. This makes the helper harder to use safely and increases the chance of future call sites assuming a `UserCredential` return.

### Issue Context
The helper now returns `null` when `signInWithRedirect` is used.

### Fix Focus Areas
- apps/web/src/app/helpers/firebase/auth.ts[18-25]

### Suggested fix
- Prefer splitting into explicit helpers with non-ambiguous contracts, e.g.:
 - `googleSignInWithPopup(): Promise<UserCredential>`
 - `googleSignInWithRedirect(): Promise<void>`
 - (optional) a wrapper `googleSignIn({ mode: 'popup' | 'redirect' })`
- If keeping the current function, consider:
 - making `isMobile` optional with a default (`(isMobile = false) => ...`)
 - explicitly typing the return (`Promise<UserCredential | null>`) and documenting that redirect returns `null`/does not complete normally.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Redirect effect runs twice in dev 🐞 Bug ☼ Reliability

Description

The new mount-only effect calls getGoogleRedirectResult() and will execute twice in development due
to Next's React Strict Mode, causing redundant redirect-result processing (and potentially duplicate
logging/side effects if the underlying call is not fully idempotent). This is a
dev-experience/reliability footgun when testing the redirect flow locally.

Code

apps/web/src/app/web-ext/page.tsx[R37-49]

+  // Mobile sign-in uses a redirect, which reloads the page and loses the
+  // shouldPreloadData state set in handleSignIn — recover it on return.
+  useEffect(() => {
+    getGoogleRedirectResult()
+      .then((result) => {
+        if (result?.user) {
+          setShouldPreloadData(true);
+        }
+      })
+      .catch((error: unknown) => {
+        console.error('Redirect sign-in failed', error);
+      });
+  }, []);

Relevance

⭐ Low

Similar “extra safety” reliability suggestions in web-ext page were repeatedly rejected (e.g.,
try/finally guards).

PR-#3908

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The page adds a useEffect(..., []) that calls getGoogleRedirectResult(). The app has
reactStrictMode: true configured, which means mount effects run twice during development,
resulting in duplicate invocations of the redirect-result logic while testing locally.

apps/web/src/app/web-ext/page.tsx[37-49]
apps/web/next.config.ts[23-31]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
In development, React Strict Mode runs mount effects twice. The redirect-result handler effect will therefore call `getGoogleRedirectResult()` twice, which is unnecessary and can complicate local debugging of the redirect flow.

### Issue Context
Next config enables `reactStrictMode: true`.

### Fix Focus Areas
- apps/web/src/app/web-ext/page.tsx[37-49]
- apps/web/next.config.ts[23-31]

### Suggested fix
- Make the effect idempotent under Strict Mode by guarding with a ref:
 - `const ran = useRef(false);`
 - early-return if `ran.current` is true; otherwise set it true and proceed.
- Alternatively, move redirect-result handling into a single shared place (e.g., AuthProvider) so it is naturally centralized and less likely to be duplicated across pages.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[coderabbitai]

📝 Walkthrough

Walkthrough

Firebase Google authentication now selects redirect sign-in for mobile devices while retaining popup sign-in elsewhere. A redirect-result helper was added. The web extension page detects the operating system, processes redirect authentication results, supports test credential sign-in, passes the mobile flag to Google sign-in, updates preload state after successful authentication, and logs authentication failures.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly states the main change: mobile Safari login now uses redirect sign-in.
Description check	✅ Passed	The description follows the required template and includes the version-change question, with the PR summary providing the relevant context.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}