Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR polishes password-setting and post-verification behavior for VPN / non-decoupled Sync scenarios by ensuring the correct CMS copy and password confirmation UI are shown, and by adjusting VPN-specific routing after signup confirmation.
Changes:
- Prefer CMS description copy on the PostVerify Set Password page for all arrival paths, with hardcoded text as fallback.
- Update Signup password-confirmation logic to depend on whether a password is required for key derivation (
requiresPasswordForLogin), not only on Sync. - Avoid navigating mobile VPN users to
/post_verify/service_welcometo prevent a brief “blip” before the client closes the webview; add tests/stories/mocks updates.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-settings/src/pages/Signup/mocks.tsx | Extends signup mocks to include requiresPasswordForLogin and adjusts OAuth-native mock signature. |
| packages/fxa-settings/src/pages/Signup/interfaces.ts | Extends signup integration types to include requiresPasswordForLogin. |
| packages/fxa-settings/src/pages/Signup/index.tsx | Uses requiresPasswordForLogin(supportsKeysOptionalLogin) to decide whether to show repeat-password confirmation. |
| packages/fxa-settings/src/pages/Signup/index.test.tsx | Updates expectations and adds coverage for keys-optional vs non-optional password confirmation behavior. |
| packages/fxa-settings/src/pages/Signup/container.test.tsx | Updates the integration mock to include requiresPasswordForLogin. |
| packages/fxa-settings/src/pages/Signup/ConfirmSignupCode/index.tsx | Skips service_welcome navigation for mobile VPN to avoid UI “blip”. |
| packages/fxa-settings/src/pages/Signup/ConfirmSignupCode/index.test.tsx | Adds test to ensure no navigation occurs for mobile VPN. |
| packages/fxa-settings/src/pages/PostVerify/SetPassword/index.tsx | Renders CMS description regardless of arrival reason; falls back to reason-specific hardcoded copy. |
| packages/fxa-settings/src/pages/PostVerify/SetPassword/index.test.tsx | Adds assertions for CMS-vs-fallback copy across OTP/passkey paths; refactors CMS test data usage. |
| packages/fxa-settings/src/pages/PostVerify/SetPassword/index.stories.tsx | Adds OTP-with-CMS story variant. |
| packages/fxa-settings/src/pages/mocks.tsx | Adds PostVerifySetPasswordPage to shared MOCK_CMS_INFO. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
118
to
121
| requiresKeys: () => true, | ||
| wantsKeys: () => true, | ||
| requiresPasswordForLogin: () => requiresPasswordForLogin, | ||
| getCmsInfo: () => cmsInfo, |
Contributor
Author
There was a problem hiding this comment.
I had something like this originally, and then I opted to simplify it. I'd rather leave it as-is where you just pass in requiresPasswordForLogin depending on what you're testing.
…Sync isn't decoupled, service_welcome routing Because: - The CMS text for passwordless OTP users when Sync is not decoupled is not rendered on the set password page - The repeat password field is not displayed when Sync is not decoupled - Mobile users should not be navigated to the service welcome screen for VPN because the browser automatically closes it This commit: - Renders the CMS text and repeat password fields accordingly - Returns a fixed requiresPasswordForLogin value from signup mock instead of rederiving it - Adds a Mobile check and does not navigate those users to the service welcome screen closes FXA-14047 closes FXA-14088
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-14047
closes FXA-14088