Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Three moderate redirect correctness and privacy issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Moves subscription redirects from Backbone into the feature-gated React settings app while preserving payment routing and authentication behavior.
Changes:
- Adds the
SubscriptionsRedirectpage and tests. - Registers gated subscription routes.
- Exposes subscription configuration to settings.
Required changes:
- Use the app’s resolved
metricsEnabledvalue to avoid forwarding flow identifiers when metrics preferences are unknown. - Preserve literal
+characters when forwarding query parameters. - Detect
?signin=by parameter presence rather than its value.
| File | Description |
|---|---|
packages/fxa-settings/src/pages/SubscriptionsRedirect/index.tsx |
Implements subscription redirect and token logic. |
packages/fxa-settings/src/pages/SubscriptionsRedirect/index.test.tsx |
Tests redirect and authentication scenarios. |
packages/fxa-settings/src/lib/config.ts |
Defines subscription configuration. |
packages/fxa-settings/src/components/App/index.tsx |
Registers subscription routes. |
packages/fxa-content-server/server/lib/routes/react-app/index.js |
Adds routes to the rollout group. |
packages/fxa-content-server/server/lib/beta-settings.js |
Exposes subscription settings. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Six moderate issues remain in rollout routing, authentication, token issuance, metrics propagation, and query preservation.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
Resolved since last review (1)
| routes: reactRoute.getRoutes([ | ||
| 'subscriptions', | ||
| 'subscriptions/products/[\\w_]+', | ||
| ]), |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved moderate issues affect rollout, telemetry opt-out behavior, and duplicate redirect execution.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
Resolved since last review (1)
|
The ai-fixme pipeline used both review rounds on this PR, so it stops here. One review comment is still open: guard the redirect effect with a ref so that StrictMode does not request two OAuth tokens in development. A human needs to decide on that fix and review the PR. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Product redirects and staged client-side handoff are incomplete, while the redirect effect can issue duplicate OAuth tokens.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 6
Open (6)
Post-auth handoff remains incomplete for eligible routes · New Product subscription URLs bypass the React route group · New Missing React route for subscription product management URLs · New Subscription redirect builder ignores product and RP parameters · New Guard redirect effect against duplicate StrictMode execution Post-verification routes remain on Backbone without full rollout
| brandMessagingMode: config.get('brandMessagingMode'), | ||
| glean: { ...config.get('glean'), appDisplayVersion: config.get('version') }, | ||
| redirectAllowlist: config.get('redirect_check.allow_list'), | ||
| subscriptions: config.get('subscriptions'), |
| postVerifyOtherRoutes: { | ||
| featureFlagOn: showReactApp.postVerifyOtherRoutes, | ||
| routes: [], | ||
| routes: reactRoute.getRoutes(['subscriptions']), |
| path="/subscriptions" | ||
| element={<SubscriptionsRedirect {...{ isSignedIn }} />} |
| const base = usePaymentsNextSubscriptionManagement | ||
| ? `${config.servers.paymentsNext.url}/subscriptions/landing` | ||
| : `${managementUrl}/subscriptions`; | ||
| const query = params.toString(); | ||
| const url = `${base}${query ? `?${query}` : ''}`; |
431f9c8 to
a95cf7f
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The Backbone router and frontend rollout configuration do not yet hand enrolled users to the React route.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 6
Open (6)
Enable staged rollout for the subscriptions route · New Subscription redirect builder ignores product and RP parameters Missing React route for subscription product management URLs Product subscription URLs bypass the React route group Post-auth handoff remains incomplete for eligible routes Post-verification routes remain on Backbone without full rollout
Resolved since last review (1)
| postVerifyOtherRoutes: { | ||
| featureFlagOn: showReactApp.postVerifyOtherRoutes, | ||
| routes: [], | ||
| routes: reactRoute.getRoutes(['subscriptions']), |
| flow: MetricsFlow | null; | ||
| metricsEnabled: boolean; | ||
| }) { | ||
| const { managementUrl, usePaymentsNextSubscriptionManagement } = |
There was a problem hiding this comment.
suggestion: remove usePaymentsNextSubscriptionManagement flag and managementUrl.
This boolean was added as part of the SP3 cutover. All traffic now goes to the SP3 sub manage page, so this feature flag can be removed.
| useEffect(() => { | ||
| if (redirected.current) { | ||
| return; | ||
| } | ||
| redirected.current = true; | ||
| const { url, unauthenticatedUrl } = getSubscriptionsRedirect({ | ||
| config, | ||
| flow: getMetricsFlow(), | ||
| metricsEnabled: currentAccount()?.metricsEnabled !== false, | ||
| }); | ||
|
|
||
| if (!isSignedIn) { | ||
| hardNavigate(unauthenticatedUrl); | ||
| return; | ||
| } | ||
|
|
||
| const { managementClientId, managementScopes, managementTokenTTL } = | ||
| config.subscriptions; | ||
| authClient | ||
| .createOAuthToken(sessionToken()!, managementClientId, { | ||
| scope: managementScopes, | ||
| ttl: managementTokenTTL, | ||
| }) | ||
| .then(({ access_token }) => | ||
| hardNavigate(`${url}#accessToken=${encodeURIComponent(access_token)}`) | ||
| ) | ||
| .catch(() => hardNavigate(unauthenticatedUrl)); | ||
| }, [authClient, config, isSignedIn]); |
There was a problem hiding this comment.
suggestion: remove this logic and just redirect to {payments-next}/subscriptions/landing
Much of this logic has been refactored and moved to payments-next and should no longer be the responsibility of content-server.
## Because - `/subscriptions` still runs in Backbone, so it blocks the move of post-verify routes to React. - The earlier plan kept this route in Backbone until FXA-6117. This PR moves it now instead, behind the staged rollout flag. ## This pull request - Adds a `SubscriptionsRedirect` page in fxa-settings that sends `/subscriptions` to payments-next `subscriptions/landing`, with flow params. - Serves the route from React through `postVerifyOtherRoutes`, with `fullProdRollout: false`. - Hands Backbone `/subscriptions` navigations to React with `createReactOrBackboneViewHandler`. ## Issue that this pull request solves Closes: FXA-6661
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The React redirect ignores the subscription-management rollout flag and always sends users to Payments Next.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (7)
Honor rollout flag for subscription management destination · New Enable staged rollout for the subscriptions route Subscription redirect builder ignores product and RP parameters Missing React route for subscription product management URLs Product subscription URLs bypass the React route group Post-auth handoff remains incomplete for eligible routes Post-verification routes remain on Backbone without full rollout
| getSubscriptionsRedirect({ | ||
| paymentsNextUrl: config.servers.paymentsNext.url, | ||
| flow: getMetricsFlow(), | ||
| metricsEnabled: currentAccount()?.metricsEnabled !== false, |


Because
/subscriptionsstill runs in Backbone, so it blocks the move of post-verify routes to React.This pull request
SubscriptionsRedirectpage in fxa-settings for/subscriptions.usePaymentsNextSubscriptionManagementpickssubscriptions/landing.postVerifyOtherRoutesin the content-server react-app routes, withfullProdRollout: false.subscriptionsconfig from content-server to fxa-settings.Issue that this pull request solves
Closes: FXA-6661
Checklist
Put an
xin the boxes that applyHow to review (Optional)
getSubscriptionsRedirectinSubscriptionsRedirect/index.tsx.App/index.tsx, then the page and its tests.Screenshots (Optional)
Other information (Optional)
fullProdRollout: falsestays. A reviewer asked to change it, but the epic rolls these routes out in stages.eslinton the changed files andtsc --noEmiton fxa-settings exit 0./subscriptions/products/:productIdis out of scope because it has no users. The Backbone route and view for it stay for now.