Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The route can be unavailable with its feature flag disabled, and required functional coverage and lastLogin persistence remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Migrates the subscription finish-account-setup password flow from Backbone to React while preserving the existing email-link URL.
Changes:
- Adds React routing, query validation, account setup, redirects, and subscription-specific UI.
- Updates shared password components, localization, stories, and telemetry documentation.
- Removes the obsolete Backbone route, view, template, and tests.
File summaries
| File | Summary | Review findings |
|---|---|---|
packages/fxa-shared/metrics/glean/fxa-ui-metrics.yaml |
Documents the subscription telemetry reason. |
— |
packages/fxa-settings/src/pages/PostVerify/SetPassword/mocks.tsx |
Adds subscription flow mocks. | — |
packages/fxa-settings/src/pages/PostVerify/SetPassword/interfaces.ts |
Adds subscription page properties and form support. | — |
packages/fxa-settings/src/pages/PostVerify/SetPassword/index.tsx |
Renders subscription-specific password setup UI. | — |
packages/fxa-settings/src/pages/PostVerify/SetPassword/index.test.tsx |
Tests subscription password setup behavior. | — |
packages/fxa-settings/src/pages/PostVerify/SetPassword/index.stories.tsx |
Adds subscription Storybook states. | — |
packages/fxa-settings/src/pages/PostVerify/SetPassword/finish-account-setup-container.tsx |
Validates link parameters, completes setup, stores the account, and redirects. | Moderate (1 vote): Persist lastLogin when storing the account.Moderate (1 vote): Add functional coverage for a generated subscription finish-setup link. |
packages/fxa-settings/src/pages/PostVerify/SetPassword/finish-account-setup-container.test.tsx |
Tests the finish-account-setup container. | — |
packages/fxa-settings/src/pages/PostVerify/SetPassword/en.ftl |
Adds subscription password setup strings. | — |
packages/fxa-settings/src/models/pages/post-verify/index.ts |
Exports post-verify page models. | — |
packages/fxa-settings/src/models/pages/post-verify/finish-account-setup-query-params.ts |
Validates email-link query parameters. | — |
packages/fxa-settings/src/models/pages/post-verify/finish-account-setup-query-params.test.ts |
Tests query parameter validation. | — |
packages/fxa-settings/src/components/PasswordStrengthInline/index.tsx |
Supports subscription password criteria. | — |
packages/fxa-settings/src/components/LinkDamaged/index.tsx |
Handles damaged finish-setup links. | — |
packages/fxa-settings/src/components/LinkDamaged/en.ftl |
Adds damaged-link localization. | — |
packages/fxa-settings/src/components/FormSetupAccount/interfaces.ts |
Expands password form type support. | — |
packages/fxa-settings/src/components/FormPasswordWithInlineCriteria/index.tsx |
Adds subscription form behavior and labels. | — |
packages/fxa-settings/src/components/FormPasswordWithInlineCriteria/en.ftl |
Adds subscription form localization. | — |
packages/fxa-settings/src/components/App/index.tsx |
Registers the React route. | Moderate (1 vote): Add functional coverage for the live finish-account-setup link and response flow. |
packages/fxa-content-server/server/lib/routes/react-app/index.js |
Registers the content-server React route. | Critical (3 votes): The URL has no handler when postVerifyThirdPartyAuthRoutes is disabled; use an unconditional route group or retain a fallback.Moderate (1 vote): Add functional or Playwright coverage for a generated subscription finish-setup link. |
packages/fxa-content-server/app/tests/test_start.js |
Removes registration of the deleted Backbone test. | — |
packages/fxa-content-server/app/tests/spec/views/post_verify/finish_account_setup/set_password.js |
Deletes obsolete Backbone view tests. | — |
packages/fxa-content-server/app/scripts/views/post_verify/finish_account_setup/set_password.js |
Deletes the Backbone view. | — |
packages/fxa-content-server/app/scripts/templates/post_verify/finish_account_setup/set_password.mustache |
Deletes the Backbone template. | — |
packages/fxa-content-server/app/scripts/lib/router.js |
Removes the Backbone route entry. | — |
Review details
Suppressed comments (4)
packages/fxa-content-server/server/lib/routes/react-app/index.js:159
- The unit tests mock the query-param hook, the page, and
AuthClient.finishSetup, so they do not exercise the live/post_verify/finish_account_setup/set_passwordURL through content-server routing or verify a real subscription finish-setup link can submit successfully. Since this is a live email entry point and the Backbone fallback is deleted, add a Playwright/functional test using a stub subscription account and its generated email link (the repository already has a parallel, auto-cleanedTestAccountTrackerfixture).
// The subscription "finish account setup" email link. The URL is
// fixed by links already sitting in inboxes.
'post_verify/finish_account_setup/set_password',
packages/fxa-settings/src/components/App/index.tsx:698
- Please add a functional test for a real
post_verify/finish_account_setup/set_passwordlink before merging (or track it as a required follow-up). The new tests mount the container directly, so they do not verify that the content-server route reaches this lazy route or that a real/account/finish_setupresponse completes the one-use email flow; the existing Playwright suite has post-verify coverage but no test for this path.
{/* The subscription "finish account setup" email link. */}
<Route
path="/post_verify/finish_account_setup/set_password/*"
element={<FinishAccountSetupContainer {...{ integration }} />}
/>
packages/fxa-settings/src/pages/PostVerify/SetPassword/finish-account-setup-container.tsx:48
- Please persist
lastLoginwhen storing this newly signed-in account.lastStoredAccount()only considers accounts with alastLogintimestamp (src/lib/cache.ts:90-101), and the index/sign-in flows use it whencurrentAccountUidis unavailable, so this account cannot be recovered after that storage-eviction path; the other React auth flows setlastLogin: Date.now().
storeAccountData({
packages/fxa-settings/src/pages/PostVerify/SetPassword/finish-account-setup-container.tsx:42
- The new tests mock the query model,
SetPassword, andauthClient.finishSetup, so they cannot exercise the actual email URL through the content-server route group and/account/finish_setup. This replaces the only live handler forsmtp.accountFinishSetupUrlwhile deleting the Backbone implementation; add a functional test that follows a generated subscription finish-setup link and completes the flow before removing the fallback.
const { uid, sessionToken, verified } = await authClient.finishSetup(
- Files reviewed: 25/25 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Both set-password routes plus service-welcome ride the | ||
| // The subscription "finish account setup" email link. The URL is | ||
| // fixed by links already sitting in inboxes. | ||
| 'post_verify/finish_account_setup/set_password', |
bcolsson
requested changes
Sep 18, 2026
|
|
||
| set-password-subscription-heading = Create a { -product-mozilla-account } | ||
| # $productName (String) - the name of the subscribed product, e.g. Mozilla VPN | ||
| set-password-subscription-heading-with-product = Create a { -product-mozilla-account } <span>Continue to { $productName }</span> |
Contributor
There was a problem hiding this comment.
Does this need to be one string and/or can we split this to two strings?
5 tasks
bcolsson
approved these changes
Sep 21, 2026
Contributor
Author
|
Confirmed with subplat, these pages are not used anymore. Do not port them to React and remove from content-server. |
…sword page ## Because - SubPlat confirmed the `post_verify/finish_account_setup/set_password` page is not used any more. - The ticket first asked for a React port. That port is no longer worth the work, so the page goes away instead. ## This pull request - Deletes the Backbone view `set_password.js` and its mustache template. - Deletes the view spec and its `require` line in `test_start.js`. - Removes the route from `router.js`. - Removes the path from `FRONTEND_ROUTES` in `content-server-routes.js`. - Reverts the React port from the earlier commits on this branch. `fxa-settings` and `fxa-shared` match `main` with no diff. ## Issue that this pull request solves Closes: FXA-14380
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The removed route is still linked from active subscription reminder emails, causing those links to return 404.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
| 'post_verify/cad_qr/scan_code', | ||
| 'post_verify/cad_qr/connected', | ||
| 'post_verify/finish_account_setup/set_password', | ||
| 'post_verify/newsletters/add_newsletters', |
Contributor
Author
There was a problem hiding this comment.
I'm going to file a follow up ticket to remove those as well. Lets just keep this PR focused for now.
Contributor
There was a problem hiding this comment.
@vbudhram should we add a redirect for these though? If someone clicks on an old email that links-out here, can we just take them to our create_password page which will redirect to signin if they set a password?
Contributor
Author
There was a problem hiding this comment.
@LZoog Probably not worth it since these emails have not gone out in over a year
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
post_verify/finish_account_setup/set_passwordpage is not used any more.This pull request
set_password.jsand its mustache template.requireline intest_start.js.router.js.FRONTEND_ROUTESincontent-server-routes.js.fxa-settingsandfxa-sharedmatchmainwith no diff.Issue that this pull request solves
Closes: FXA-14380
Checklist
Put an
xin the boxes that applyHow to review (Optional)
router.jsandcontent-server-routes.js.fxa-auth-serverstill builds this URL for subscription reminder emails, so the links in those emails now go nowhere. See the follow-up note below.Screenshots (Optional)
None. This pull request only deletes a page.
Other information (Optional)
Follow-up for the subscription platform team, not in this pull request: retire the smtp config entry in
fxa-auth-serverconfig/index.tsand thesubscriptionAccountReminderFirstandsubscriptionAccountReminderSecondtemplates that link to this page.Dead code left in place, all in
fxa-content-server, now orphaned:app/scripts/models/verification/set-password.js, thefinishSetupchain inmodels/user.js,models/account.js, andlib/fxa-client.js, andVat.register('jwt')inlib/vat.js.Local checks:
eslintonfxa-content-serverexits 0.npx nx lint fxa-settingsexits 0.fxa-content-serverhas no local unit test script, so CI runs that suite. No functional test names this page. The stack was not started.