Conversation
64873ba to
f75429f
Compare
6c60b0e to
c2cca02
Compare
f719832 to
fa7a3da
Compare
3ea8a3a to
46d78ad
Compare
vpomerleau
left a comment
There was a problem hiding this comment.
(Initial comments, Claude-assisted review)
The navigate()-during-render migration is a lot of ground to cover, and pulling lazy into its own module fixes a real latent bug on the way.
Two things I'd want sorted first, both in the TOTP path — see SigninTotpCode/container.tsx and baseTokenCode.ts. I think they're the same remount approached from two sides.
Dependency notes that don't map onto a changed line:
@testing-library/reactis still15.0.6— peers onreact@^18and pulls@types/react-dom@^18against@types/react@19. Probably what's driving theJSX.Elementchurn here; bumping to^16.3.0and adding@types/react-domtoresolutionsshould help.reactis pinned to19.0.8but@types/reactis19.2.18— types two minors ahead of runtime.@testing-library/react-hooksand@material-ui/corehave no importers left after this. Droppable.@react-pdf/renderer3→4 drops the layout/textkit pins and the new mock stubs the module out, so PDFs have no coverage on either side. Have you opened a generated one?
Also worth a look before this leaves draft: the PR body is still the template, and the RTL box is unchecked while the avatar zoom control becomes a native range input.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Because: - fxa-react/jest.config.js pointed at svg-transform.js, which this branch deleted, so fxa-react:test-unit failed Jest config validation before running a single test - the replacement transformer lived in fxa-settings, so fxa-react and fxa-admin-panel reached into an app package for library test config This commit: - moves fileTransform.js to fxa-react/configs - repoints fxa-react's jest.config.js, the shared rescripts config, and fxa-settings' Jest transform at the new location
Because: - the timer, breadcrumb and new localized string guarded against the global redirect failing, which is a pre-existing assumption React 19 did not change - hardening it needs UX copy and an l10n string, so it belongs in its own change rather than a dependency upgrade This commit: - reduces the INSUFFICIENT_AAL branch to a bare return; the comment already explains why onDismiss must be skipped - clarifies the InputPhoneNumber comment: defaultValues seeds countryCode, setValue keeps it in sync
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 67 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/fxa-settings/src/components/InputText/index.tsx:146
inputRefDOMis typed asRef<HTMLInputElement>(so callers can pass either an object ref or a callback ref), butcombinedRefalways treats it as an object ref by assigning.current. If a caller passes a callback ref, it will never be invoked (and the ref will not be set). Also, assigning.currentviaReact.RefObjectis fragile with React/@types typings.
Consider handling function refs explicitly and using MutableRefObject for the object-ref path (same for inputRef / RHF registration refs).
if (inputRefDOM) {
(inputRefDOM as React.RefObject<HTMLInputElement | null>).current =
element;
}
const refToCall = registrationRefLatest.current || inputRef;
packages/fxa-settings/config/jest/reactPdfMock.js:8
config/jest/reactPdfMock.jswas added, but there are no references to it in the repo (nomoduleNameMapper,jest.mockpath alias, or other import). As-is, it's dead code and its header comment about "@react-pdf/renderer v4" is also potentially misleading given fxa-settings is pinned/patching renderer 3.x.
Either wire this into Jest config (e.g., map @react-pdf/renderer to this file) or remove it to avoid carrying an unused mock implementation.
// @react-pdf/renderer v4 ships ESM-only, which Jest cannot import without
// transforming its entire transitive dependency tree. Since unit tests never
// exercise real PDF generation, we stub the module.
|
React19 is causing breaking changes for recovery key PDF generation. We'll need to find a solution (revert to serving the key in a text file or find an alternate PDF generator) to unblock the React 19 upgrade. I'm following up with product team. |
This pull request
Dependencies
Payments
Settings
Issue that this pull request solves
Closes: PAY-3887
Checklist
Put an
xin the boxes that apply