Conversation
There was a problem hiding this comment.
🟡 Changes recommended
OAuth-native invalid tokens remain on an unusable page without triggering the recovery flow.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Handles invalid session tokens during TOTP status checks on the sign-in token-code page.
Changes:
- Catches TOTP status lookup failures and redirects non-native OAuth flows.
- Adds tests for invalid-token and other error paths.
- Stabilizes the mocked auth client.
File summaries
| File | Description |
|---|---|
container.tsx |
Handles TOTP lookup errors and updates effect dependencies. |
container.test.tsx |
Tests redirect behavior across integration and error types. |
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 catch can still produce an unhandled rejection for nullish rejection values.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
Resolved since last review (1)
## Because - The catch block around `checkTotpTokenExists` reads `err.errno` directly. - The auth client can reject with null or undefined. The read then throws a TypeError inside the async effect, and the error escapes as an unhandled rejection on `/signin_token_code`. - Sentry records this error as FXA-CONTENT-QWA. ## This pull request - Reads `err?.errno` in the `checkTotpTokenExists` catch block in `container.tsx`. - Adds a test in `container.test.tsx` for a rejection with an undefined reason. ## Issue that this pull request solves Closes: https://mozilla-hub.atlassian.net/browse/FXA-13110
| getClientId: () => 'sync', | ||
| isSync: () => isSync, | ||
| wantsKeys: () => false, | ||
| requiresKeys: () => false, |
There was a problem hiding this comment.
Seems fine, but is this applicable to changes on container.tsx?
There was a problem hiding this comment.
Looks like this was added for the mock completeness, I'll keep it in since low risk.
Because
checkTotpTokenExistsreadserr.errnodirectly./signin_token_code.This pull request
err?.errnoin thecheckTotpTokenExistscatch block incontainer.tsx.container.test.tsxfor a rejection with an undefined reason.Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-13110
Checklist
Put an
xin the boxes that applyHow to review (Optional)
checkTotpTokenExistsincontainer.tsx.container.tsx, thencontainer.test.tsx.errnokeeps the same behavior.Screenshots (Optional)
None. This change does not modify the user interface.
Other information (Optional)
fxa-settingsSigninTokenCode suites: 32 passed, 0 failed.nx lint fxa-settings: exit 0.TypeError: Cannot read properties of undefined (reading 'errno').