Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Existing capitalized attribution and already-signed-in visits are not handled correctly.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds default pairing attribution for direct /pair visits during sign-in redirects.
Changes:
- Defines the
fxa_pairflowentrypoint. - Preserves existing attribution when redirecting.
- Adds redirect attribution tests.
File summaries
| File | Description |
|---|---|
| packages/fxa-settings/src/pages/Pair/Index/index.tsx | Applies attribution to sign-in redirects. |
| packages/fxa-settings/src/pages/Pair/Index/index.test.tsx | Tests default and existing entrypoints. |
| packages/fxa-settings/src/lib/pairing-attribution.ts | Defines the pairing entrypoint constant. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+333
to
+335
| if (!search.has('entrypoint')) { | ||
| search.set('entrypoint', PAIR_FLOW_ENTRYPOINT); | ||
| } |
Comment on lines
+333
to
+335
| if (!search.has('entrypoint')) { | ||
| search.set('entrypoint', PAIR_FLOW_ENTRYPOINT); | ||
| } |
## Because
- A verified Sync user returns early, before the OAuth redirect block, so the stash for the pairing approval page was `{}`.
- `IntegrationFactory` copies `entryPoint` to `entrypoint` only when both keys are present, so `/pair?entryPoint=fx-view` reached the page with no entrypoint and got relabeled `fxa_pairflow`.
- Review on #21255 asked for the default to cover every consumer, not just the redirect.
## This pull request
- Sets the `fxa_pairflow` default in the shared `pairingAttribution` memo, so the sign-in redirect, the stash, and the download URL all pick it up.
- Checks the URL attribution before falling back to the default, so a capital-P `entryPoint` value survives.
- Drops the entrypoint default from the OAuth redirect block in `index.tsx`.
- Folds the three redirect entrypoint tests into one `it.each` covering the URL, the integration, and a capital-P URL param.
- Adds a test for the stash on a signed-in direct visit.
## Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-14556
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
{}.IntegrationFactorycopiesentryPointtoentrypointonly when both keys are present, so/pair?entryPoint=fx-viewreached the page with no entrypoint and got relabeledfxa_pairflow.This pull request
fxa_pairflowdefault in the sharedpairingAttributionmemo, so the sign-in redirect, the stash, and the download URL all pick it up.entryPointvalue survives.index.tsx.it.eachcovering the URL, the integration, and a capital-P URL param.Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-14556
Checklist
Put an
xin the boxes that applyHow to review (Optional)
pairingAttributionmemo inPair/Index/index.tsx.Screenshots (Optional)
No UI changes.
Other information (Optional)
Ran locally:
npx jest src/pages/Pair/Index(fxa-settings): 82 passed, 0 failed.tsc -p packages/fxa-settings/tsconfig.json --noEmitandnpx nx lint fxa-settings: 0 errors.I did not run the functional tests locally.
Heads up for metrics owners: the stash is never empty now, so
applyPairingAttribution()no longer reaches itspreferencesfallback branch. A pairing that starts fromabout:preferencesinside the 30 minute TTL now reportsfxa_pairflowon the approval page.