Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a redirect-loop/incorrect redirect behavior when users navigate directly to /signup?email=... (bypassing the email-first flow), ensuring signup can proceed to email confirmation instead of bouncing back to /signin. It also adds functional coverage for direct /signup entry in both Sync OAuth and passwordless-enabled scenarios.
Changes:
- Add a
useRefguard inSignupContainerto prevent repeated email status checks that can cause redirect loops. - Add a functional test covering Sync OAuth signup starting at
/signup?email=.... - Add a functional test covering passwordless redirect behavior for direct
/signup?email=...when passwordless is enabled.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/fxa-settings/src/pages/Signup/container.tsx | Prevents repeated accountStatusByEmail checks during signup renders by adding a ref-based one-time guard. |
| packages/functional-tests/tests/react-conversion/signup.spec.ts | Adds a Sync OAuth functional test for navigating directly to /signup with an email query param and completing signup. |
| packages/functional-tests/tests/passwordless/signinPasswordless.spec.ts | Adds a passwordless edge-case test for direct /signup?email=... redirecting into the OTP flow when enabled. |
Comments suppressed due to low confidence (1)
packages/fxa-settings/src/pages/Signup/container.tsx:103
accountStatusByEmailis awaited inside an async IIFE without any error handling. If the request fails/rejects, this can produce an unhandled promise rejection, and becauseattemptedEmailStatusCheck.currentis set to true before the await, the check will never retry (and may leave the user stuck on the signup form). Wrap the call in try/catch and decide on a fallback (e.g., navigate back to email-first or show an error), and only permanently set the ref once the attempt has been handled (or reset it on failure).
attemptedEmailStatusCheck.current = true;
const { exists, hasLinkedAccount, hasPassword, passwordlessSupported } =
await authClient.accountStatusByEmail(queryParamModel.email, {
thirdPartyAuthStatus: true,
clientId: integration.getClientId(),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { exists, hasLinkedAccount, hasPassword, passwordlessSupported } = | ||
| await authClient.accountStatusByEmail(queryParamModel.email, { | ||
| thirdPartyAuthStatus: true, |
There was a problem hiding this comment.
The passwordlessSupported value fetched here is later used to redirect new users into the passwordless OTP signup flow. Unlike the email-first/signin containers, this path doesn’t appear to be gated by the client-side config.featureFlags.passwordlessEnabled (or any override param), which can cause direct /signup?email=... navigations to enter passwordless when the feature is disabled. Consider applying the same feature-flag gating used elsewhere before redirecting.
| useEffect(() => { | ||
| (async () => { |
There was a problem hiding this comment.
useEffect is still declared without a dependency array, so it will execute after every render (even if the useRef guard prevents most work). For clarity and to avoid accidental re-runs if the guard changes, consider adding an explicit dependency array (or [] with an exhaustive-deps rationale, consistent with other containers).
There was a problem hiding this comment.
This suggestion makes sense to me, would it work? By adding an empty dependency array, [], this useEffect will only run once on component mount.
| await authClient.accountStatusByEmail(queryParamModel.email, { | ||
| thirdPartyAuthStatus: true, |
There was a problem hiding this comment.
accountStatusByEmail supports a service parameter and the auth-server uses it when deciding passwordless eligibility/allowlisting. Unlike the Index/Signin containers, this call omits service: integration.getService(), which can cause passwordlessSupported to be false when the client is allowlisted only for specific services. Consider passing service here for consistent and correct eligibility checks.
StaberindeZA
left a comment
There was a problem hiding this comment.
r+wc. Just the one comment added to the Copilot comment. Otherwise looks good.
…ests Because: - Navigating directly to /signup?email=... caused a redirect loop - The useEffect in SignupContainer had no dependency array, re-running every render - After account creation, accountStatusByEmail returned exists: true and redirected to /signin instead of allowing navigation to /confirm_signup_code This commit: - Adds a useRef guard and empty dependency array in SignupContainer to ensure the email status check runs only once on mount - Passes service param to accountStatusByEmail for correct passwordless eligibility - Adds functional tests for password signup via Sync OAuth RP navigating directly to /signup?email=... and passwordless redirect behavior Fixes FXA-13309
Because
/signup?email=...(bypassing the email-first page) caused a redirect loopuseEffectinSignupContainerhad no dependency array, so it re-ran on every renderaccountStatusByEmailreturnedexists: trueand redirected to/signininstead of allowing navigation to/confirm_signup_codeThis pull request
useRefguard inSignupContainer(container.tsx) to ensure the email status check runs only once, preventing the redirect loop/signup?email=...(signup.spec.ts)Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-13309
Checklist
Other information
How to test:
yarn start)http://localhost:3030/signup?email=newuser@restmail.netConfig change needed for passwordless test: Set
passwordlessEnabled: trueinpackages/fxa-content-server/server/config/local.jsonunderfeatureFlags(already enabled in CI).