Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The blocked-send test does not verify the intended success/failure telemetry behavior.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Pre-sends passwordless OTPs so blocked requests remain on the email form.
Changes:
- Sends OTP before passwordless navigation and passes
codeSent. - Handles blocked sends through the existing error banner.
- Updates related navigation and error tests.
File summaries
| File | Description |
|---|---|
pages/Index/container.tsx |
Adds pre-navigation OTP sending and telemetry ordering. |
pages/Index/container.test.tsx |
Tests successful, failed, and blocked sends. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 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
The new-account passwordless success and blocked-error paths lack focused unit coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Resolved since last review (1)
| isSignup: true, | ||
| }, | ||
| }); | ||
| await goToPasswordlessCode({ isSignup: true }); |
## Because - The email form sent the user to the code page before it asked for a code. The code page then sent the first code on mount. - An admin blocklist hit (`REQUEST_BLOCKED`, errno 125) left the user on the code page with no code and no way forward. - Every passwordless test used an existing account, so the new account branch had no coverage. ## This pull request - Sends the first passwordless code from the Index container before it navigates, and passes `codeSent: true` so the code page skips its mount send. - Rethrows `REQUEST_BLOCKED` into the existing `handleEmailSubmissionError` path, so the banner shows on the email form and the flow stays there. - Returns false for any other send failure, so the flow navigates and the code page sends again and reports the error. - Moves `GleanMetrics.emailFirst.submitSuccess` after `handleSuccessNavigation`, so a blocked send records `submitFail` only. - Adds two container tests for the new account branch. One test covers a good send, the other covers a blocked send. ## Issue that this pull request solves Closes: https://mozilla-hub.atlassian.net/browse/FXA-13688
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Retrying every non-blocked send failure can invalidate an already-emailed OTP and duplicate rate-limit usage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
| } catch (error: any) { | ||
| if (error.errno === AuthUiErrors.REQUEST_BLOCKED.errno) { | ||
| throw error; | ||
| } | ||
| return false; |
vbudhram
left a comment
vbudhram
left a comment
There was a problem hiding this comment.
Low risk change with good coverage, 👍🏽
Because
REQUEST_BLOCKED, errno 125) left the user on the code page with no code and no way forward.This pull request
codeSent: trueso the code page skips its mount send.REQUEST_BLOCKEDinto the existinghandleEmailSubmissionErrorpath, so the banner shows on the email form and the flow stays there.GleanMetrics.emailFirst.submitSuccessafterhandleSuccessNavigation, so a blocked send recordssubmitFailonly.Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-13688
Checklist
Put an
xin the boxes that applyHow to review (Optional)
src/pages/Index/container.tsxandsrc/pages/Index/container.test.tsx.sendPasswordlessCodeandgoToPasswordlessCodefirst, then the new tests.codeSentflag thatSigninPasswordlessCodereads, and the order of the Glean events.Screenshots (Optional)
Other information (Optional)
node scripts/test.js --testPathPattern "src/pages/Index": 54 passed, 0 failed.npx nx lint fxa-settings: exit 0.