Conversation
Because: - The Authorize state was a near-duplicate of cached signin that we were able to consolidate (FXA-13660) - The monolithic Signin component branched internally on several conditions, making routing hard to read and test (FXA-13302) This commit: - Removes the Authorize state; authorization-flow rendering is handled by the cached signin path via the cached page CMS - Splits Signin into SigninCached, SigninThirdParty, and a new SigninDecider that owns the routing decision and SESSION_EXPIRED - Extracts SigninUserLockup (shared avatar+email block) and useCachedSigninLockup (shared CMS + banner state) so cached and third-party views stay in sync - Moves non-routed components under pages/Signin/components/ - Reorganizes tests: SigninDecider owns routing tests; child render behavior lives in SigninCached/SigninThirdParty test files; Signin/index.test.tsx is scoped to the password flow closes FXA-13660 closes FXA-13302 closes FXA-13718
LZoog
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the Settings sign-in flow by removing the dedicated “Authorize” state and splitting the previously monolithic Signin route into smaller, more focused components with a new SigninDecider that owns view routing (password vs cached vs third-party) and SESSION_EXPIRED handoff behavior.
Changes:
- Removed the Authorize CMS page/state and routed authorization-flow rendering through the cached sign-in path.
- Split the sign-in UI into
SigninDecider,SigninCached,SigninThirdParty, and extracted shared UI/state (SigninUserLockup,useCachedSigninLockup). - Reorganized/added tests to match the new component boundaries and updated functional coverage around the authorization/cached behavior.
Reviewed changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-settings/src/pages/Signin/useCachedSigninLockup.ts | New shared hook for cached/third-party “lockup” derived state (CMS/layout/banner). |
| packages/fxa-settings/src/pages/Signin/mocks.tsx | Updated test subject to route through SigninDecider and expanded helper prop typing. |
| packages/fxa-settings/src/pages/Signin/interfaces.ts | Split shared props from routed props; introduced SigninCachedProps / SigninThirdPartyProps. |
| packages/fxa-settings/src/pages/Signin/index.tsx | Scoped to password-entry sign-in view; moved cached/authorize logic out. |
| packages/fxa-settings/src/pages/Signin/index.test.tsx | Trimmed to password flow tests; removed cached/authorize routing assertions. |
| packages/fxa-settings/src/pages/Signin/index.stories.tsx | Removed authorization stories; added cached signed-into-Firefox story. |
| packages/fxa-settings/src/pages/Signin/container.tsx | Switched routed component from Signin to SigninDecider. |
| packages/fxa-settings/src/pages/Signin/container.test.tsx | Updated container tests/mocks to assert SigninDecider usage and new integration methods. |
| packages/fxa-settings/src/pages/Signin/components/SigninUserLockup/index.tsx | New shared avatar+email (+ accessibility info) component. |
| packages/fxa-settings/src/pages/Signin/components/SigninThirdParty/index.tsx | New third-party-only sign-in view for linked-passwordless users. |
| packages/fxa-settings/src/pages/Signin/components/SigninThirdParty/index.test.tsx | New unit tests for third-party-only view rendering. |
| packages/fxa-settings/src/pages/Signin/components/SigninDecider/index.tsx | New routing component deciding cached vs password vs third-party; hoists SESSION_EXPIRED behavior. |
| packages/fxa-settings/src/pages/Signin/components/SigninDecider/index.test.tsx | New routing tests (keys_optional, wants keys, SESSION_EXPIRED handoff). |
| packages/fxa-settings/src/pages/Signin/components/SigninCached/index.tsx | New cached sign-in view; owns cached submission and SESSION_EXPIRED callback. |
| packages/fxa-settings/src/pages/Signin/components/SigninCached/index.test.tsx | New unit tests for cached sign-in UI behavior. |
| packages/fxa-settings/src/models/integrations/relier-interfaces.ts | Removed AuthorizePage from Relier CMS info interface. |
| packages/functional-tests/tests/misc/vpnIntegration.spec.ts | Updated VPN integration test to assert account-switch link is hidden in signed-into-Firefox cached view. |
| libs/shared/cms/src/lib/queries/relying-party/types.ts | Removed AuthorizePage from relying-party query result type. |
| libs/shared/cms/src/lib/queries/relying-party/query.ts | Removed AuthorizePage selection from relying-party GraphQL query. |
| libs/shared/cms/src/lib/queries/relying-party/factories.ts | Removed AuthorizePage from relying-party factory data. |
| libs/shared/cms/src/generated/graphql.ts | Regenerated GraphQL output to reflect AuthorizePage removal from query results. |
| libs/shared/cms/src/generated/gql.ts | Regenerated gql document mapping to reflect AuthorizePage removal from query results. |
Files not reviewed (2)
- libs/shared/cms/src/generated/gql.ts: Language not supported
- libs/shared/cms/src/generated/graphql.ts: Language not supported
Comments suppressed due to low confidence (2)
packages/fxa-settings/src/pages/Signin/components/SigninDecider/index.tsx:92
syncNotDecoupledRequiresPasswordno longer excludes the signed-into-Firefox (authorization) flow. Without the prior!isSignedIntoFirefoxguard, users in an authorization flow on platforms withoutkeys_optional(e.g. older Firefox/mobile) can be forced into the password view even though they’ve already authenticated, reintroducing the behavior the removed Authorize state was covering. Consider including an!isSignedIntoFirefoxcheck (or otherwise special-casing the authorization flow) in this condition.
// Relay browser service login launched in Firefox desktop 135, and the "keys optional"
// capability (Sync decoupling) launched in Fx desktop 147, meaning all Relay service users
// in those Fx versions require a password.
// This also covers Mobile until Sync has been decoupled.
const syncNotDecoupledRequiresPassword =
!useFxAStatusResult.supportsKeysOptionalLogin &&
integration.wantsKeysIfPasswordEntered();
packages/fxa-settings/src/pages/Signin/components/SigninDecider/index.tsx:49
- The doc comment says SESSION_EXPIRED is handled by “flipping to cached signin”, but the implementation flips away from cached (it marks the cached session invalid and routes to password/third-party). Update the comment to match the actual behavior to avoid confusion for future maintainers.
/**
* Decides which of the three signin views the user sees and
* handles SESSION_EXPIRED by flipping to cached signin.
*/
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+82
to
+86
| // Hide "Use a different account" when the user is signed into Firefox Desktop. | ||
| // Users cannot choose another account due to the inability to merge | ||
| // account/sync data (the "merge stop"/warning). | ||
| const hideAccountSwitchLink = | ||
| isSignedIntoFirefox && integration.isFirefoxDesktopClient(); |
Comment on lines
+49
to
+53
| // Hide "Use a different account" when the user is signed into Firefox Desktop. | ||
| // Users cannot choose another account due to the inability to merge | ||
| // account/sync data (the "merge stop"/warning). | ||
| const hideAccountSwitchLink = | ||
| isSignedIntoFirefox && integration.isFirefoxDesktopClient(); |
| import { UseFxAStatusResult } from '../../../../lib/hooks/useFxAStatus'; | ||
| import { MozServices } from '../../../../lib/types'; | ||
| import { useFinishOAuthFlowHandler } from '../../../../lib/oauth/hooks'; | ||
| import { QueryParams } from '../../../..'; |
Comment on lines
+31
to
+35
| // If cachedPageCms is the active page but does not have a CMS entry, | ||
| // fall back to signinPageCms.splitLayout | ||
| const splitLayout = cachedPageCms | ||
| ? cachedPageCms.splitLayout | ||
| : signinPageCms?.splitLayout; |
vpomerleau
reviewed
May 15, 2026
| // "Use a different account" link is hidden when signed into Firefox with | ||
| // a Firefox client + service requested on cached sign-in | ||
| await expect( | ||
| page.getByRole('link', { name: /use a different account/i }) |
Contributor
There was a problem hiding this comment.
Assuming we don't have a state where the wrong cached account might be suggested
|
|
||
| screen.getByRole('heading', { name: 'Sign in' }); | ||
| screen.getByRole('button', { name: 'Sign in' }); | ||
| expect(screen.queryByLabelText('Password')).not.toBeInTheDocument(); |
Contributor
There was a problem hiding this comment.
queryByRole doesn't work for the password input?
| error | ||
| ); | ||
| if (error.errno === AuthUiErrors.SESSION_EXPIRED.errno) { | ||
| // Container hoists us into the password view and carries this message |
Contributor
There was a problem hiding this comment.
password view = signin page? This error could also apply to passwordless accounts IIUC
|
|
||
| /** | ||
| * Decides which of the three signin views the user sees and | ||
| * handles SESSION_EXPIRED by flipping to cached signin. |
Contributor
There was a problem hiding this comment.
handles SESSION_EXPIRED by flipping to cached signin --> this seems incorrect?
| // service doesn't require keys (Sync always requires them), OR | ||
| // - The browser supports "keys optional" (Sync decoupled from other services) | ||
| // | ||
| // Passwordless users always see cached sign-in and are redirected to set a |
Contributor
There was a problem hiding this comment.
They only see cached sign-in if there's a cached session, otherwise they might be directed to SigninThirdParty or Passwordless OTP. Does the passwordless OTP redirect happen from the SigninPage? Wondering if that should also be controlled by the decider.
Contributor
There was a problem hiding this comment.
Flagging that from testing Dan is doing on #20575, it looks like passwordless accounts that should go to cached signin may be instead directed to passwordless otp. (If so, this PR would be a good place to fix)
| // users will be taken to "Set password" after signing in. | ||
| if (hasLinkedAccount && !hasPassword) { | ||
| return ( | ||
| <SigninThirdParty |
Contributor
There was a problem hiding this comment.
This will soon also include Signin with passkey option, not just third party auth. In case that informs naming.
| localizedBannerError, | ||
| } = useCachedSigninLockup({ integration, localizedErrorFromLocationState }); | ||
|
|
||
| // Hide "Use a different account" when the user is signed into Firefox Desktop. |
Contributor
There was a problem hiding this comment.
They could pick a different account from the Google/Apple auth signin flows, so they could still theoretically choose a different account and get blocked down the road.
| isSignedIntoFirefox && integration.isFirefoxDesktopClient(); | ||
|
|
||
| useEffect(() => { | ||
| // NOTE: linked-passwordless users were historically tracked under |
Contributor
There was a problem hiding this comment.
Ticket about these metrics being lumped together: https://mozilla-hub.atlassian.net/browse/FXA-13713
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
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.
Because:
This commit:
closes FXA-13660
closes FXA-13302
closes FXA-13718
Re: 13718, I just got more solid steps to reproduce from Ipsita, I'm pretty sure that I've fixed the issue here since I reproduced on
mainonce and couldn't repro here but going to double check.This diff probably looks larger than it is since much of this was moving things into other files, but this still required a lot of manual checks. Please feel free to double check me that comments and logic etc. was all preserved. I'm open to naming and directory etc. suggestions. I'm kind of torn on changing up our Signin story placements because I think it kind of works to have them in one file for Product and others trying to see all signin states but you could convince me otherwise or to add states etc.
On removing Authorize, and simplifying as much as I could: here's a Claude response but edited by me to help explain: