refactor(settings): split the passkey sign-in hook into steps - #21249
Draft
vpomerleau wants to merge 3 commits into
Draft
vpomerleau wants to merge 3 commits into
vpomerleau wants to merge 3 commits into
Conversation
Because: - The hook returned React elements and mixed three progress flags. - Surface handling was spread over three switches on one union. This commit: - Moves usePasskeySignIn to lib/hooks/usePasskeySignIn; it now returns banner state. - Collapses isLoading/isNavigating/inFlight into one status plus a re-entry ref. - Replaces the surface switches with one PASSKEY_SIGNIN_SURFACES table. - Keeps the FTL-bound banner builders in lib/passkeys/signin-flow.ts. - Callers render the banner themselves.
Because: - usePasskeySignIn's click handler ran the WebAuthn ceremony, the account lookup, the wrap-offer probe and the navigation in one 300-line closure, so each branch could only be tested by mounting the whole hook. This commit: - Extract runPasskeyAssertion (ceremony, PRF handling, server completion, WebAuthn error banners) to lib/passkeys/signin-assertion.ts. - Extract resolveSignedInAccount (account lookup, merge gate, persistence) to lib/passkeys/signin-account.ts. - Extract stashPasskeyWrapOffer and the stored-wrap probe to lib/passkeys/wrap/offer.ts. - Add a shared useMounted hook; move PasskeySignInAuthClient to signin-flow. Closes #FXA-14557
Because: - The extracted steps can be tested without mounting the hook, and the hook test was carrying their branch tables. This commit: - Add signin-assertion, signin-account and wrap/offer tests covering the WebAuthn error banners, PRF handling, service resolution, account lookup, merge gate and wrap-offer probe. - Trim the same cases from the usePasskeySignIn test, keeping wiring and status coverage.
5 tasks
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
This security-sensitive authentication refactor spans ceremony, persistence, navigation, and sensitive-data cleanup, with an unresolved regression-coverage gap.
Pull request overview
Refactors passkey sign-in into independently testable steps while preserving existing behavior.
Changes:
- Extracts assertion, account resolution, and wrap-offer logic.
- Moves orchestration into
usePasskeySignIn. - Updates consumers and focused tests.
File summaries
| File | Description |
|---|---|
| packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/index.tsx | Uses the new hook and banner model. |
| packages/fxa-settings/src/pages/Signin/index.tsx | Uses the new hook and banner model. |
| packages/fxa-settings/src/pages/Signin/components/SigninAlternativeAuthOptions/index.tsx | Updates alternative sign-in integration. |
| packages/fxa-settings/src/pages/Index/index.tsx | Updates index-page passkey integration. |
| packages/fxa-settings/src/lib/passkeys/wrap/offer.ts | Extracts wrap-offer handling. |
| packages/fxa-settings/src/lib/passkeys/wrap/offer.test.ts | Tests wrap-offer decisions. |
| packages/fxa-settings/src/lib/passkeys/signin-flow.ts | Retains shared passkey types and helpers. |
| packages/fxa-settings/src/lib/passkeys/signin-flow.test.tsx | Trims tests moved to extracted modules. |
| packages/fxa-settings/src/lib/passkeys/signin-assertion.ts | Extracts WebAuthn assertion handling. |
| packages/fxa-settings/src/lib/passkeys/signin-assertion.test.ts | Tests assertion behavior. |
| packages/fxa-settings/src/lib/passkeys/signin-account.ts | Extracts account lookup and persistence. |
| packages/fxa-settings/src/lib/passkeys/signin-account.test.ts | Tests account resolution. |
| packages/fxa-settings/src/lib/hooks/usePasskeySignIn/index.ts | Orchestrates the sign-in steps. |
| packages/fxa-settings/src/lib/hooks/usePasskeySignIn/index.test.tsx | Tests hook orchestration. |
| packages/fxa-settings/src/lib/hooks/useMounted/index.ts | Adds mounted-state tracking. |
| packages/fxa-settings/src/lib/hooks/index.ts | Exports the mounted-state hook. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const inFlight = useRef(false); | ||
| // A ceremony the user walked away from must not leave material behind, | ||
| // nor overwrite what a ceremony started on the next page has stashed. | ||
| const mounted = useMounted(); |
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
usePasskeySignInran the WebAuthn ceremony, the account lookup, the wrap-offer probe and navigation in one 300-line click handler, so every branch could only be tested by mounting the whole hook.This pull request
lib/passkeys/signin-flow.tstolib/hooks/usePasskeySignIn.runPasskeyAssertion(ceremony, PRF handling, server completion, WebAuthn error banners) tolib/passkeys/signin-assertion.ts.resolveSignedInAccount(account lookup, merge gate, persistence) andstashPasskeyWrapOffer(stored-wrap probe and opt-in stash).useMountedhook.Issue that this pull request solves
Closes: FXA-14557
Checklist
Put an
xin the boxes that applyHow to review (Optional)
lib/hooks/usePasskeySignIn/index.tsreads as the flow; compare it against the three step modules.git show -M), the second is the extraction, the third is tests only.completeSignInin the hook carries the WebChannel ordering andperformNavigationrules verbatim from the old inline call.Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
No behaviour change. Stacked on FXA-13151 and must merge before the passwordless Sync sign-in PR, which rebases onto it.