fix(buy): stop the completion mail step from silently swallowing its result

[Danswar]

Problem

BuyCompletion renders MailEdit when the account has no mail yet — the "Is this email address correct? / [Change] [Confirm]" step at the end of a buy. Two defects make it a dead end.

The success path does nothing. buy-completion.tsx passed:

onSubmit={(email) => (!email || email.length === 0) && close()}

For a non-empty address that is false && close() — a no-op. MailEdit keeps showConfirmation true and pendingEmail set, so it re-renders the identical screen. user.mail is still stale (the react package's updateMail does not refresh it), so BuyCompletion stays in the MailEdit branch too. The user gets no confirmation, no navigation, nothing — and clicks Confirm again.

The second click's error is discarded. The account now has a mail, so the API requires 2FA and answers 403 {"code":"TFA_REQUIRED", ...}. saveUser only handled the merged-account 401 and rethrew everything else into a promise nobody catches — an unhandled rejection with zero UI change, which invites another click. The Confirm button is never disabled either: it receives isLoading={isUserUpdating}, and that flag is never set for this call.

Production, PUT /v2/user/mail over 30 days: 1206 TFA_REQUIRED against 19 successful sets. A representative session:

16:48:51  PUT /v2/user/mail  200      <- the address was saved here
16:49:00  PUT /v2/user/mail  403
16:49:24  PUT /v2/user/mail  403      (x17 more over two minutes)

Reported by a partner-wallet user who saw the 403 and assumed the step had failed. It had not — their address was stored on the first click.

Changes

Buy completion mail step

• buy-completion.tsx closes on a successful submit, which is what its own Close button does in the branch for accounts that already have a mail.
• Local isSaving state disables Confirm while the request is in flight, so the fix does not depend on a @dfx.swiss/react release landing first.
• Two outcomes that retrying can never resolve now get their own short screen with an OK button that ends the step, instead of the generic ErrorHint — whose fixed opening line ("Something went wrong. Please try again…") is the wrong instruction for both:
  • TFA_REQUIRED — the account already carries an address. This step can only ever set a first one; changing an existing one additionally needs a code mailed to the new address, and this component has no field to enter one.
  • 409 "account already exists - account merge request sent" — reachable on the first-set path too. Retrying only sends another merge mail; the user has to follow the link. Reuses the hint the account and KYC screens already show for this case.
• close() falls back to the account screen when closeServices has nothing to do. On /buy/info (navigateOnClose={false}, not a widget, not in an iframe, no redirect URI) it executed nothing, and blanking the component left an empty body with the back button suppressed. That state predates this branch, but this PR routes three more outcomes into it.
• A stale error no longer survives the Change button.
• The one new string is added to the German, French and Italian translations.

Mail app-parameter (settings.context.tsx) — a second source of the same 403s

useEffect(() => {
  if (user && mail && user.mail !== mail) updateUserMail(mail);
}, [user, mail]);

The comparison is raw, but the API normalises the address on write (@Transform(Util.toLowerCaseTrim) plus a toLowerCase() in doUpdateUserMail) and the mail parameter is never normalised on the client. So ?mail=John.Doe@Example.com — or a padded a@b.com — never satisfies the guard: the effect re-submits on every change of the user object, and once the address is stored each of those is another 403 TFA_REQUIRED. The parameter is persisted to local storage, so it survives reloads.

The guard now normalises the same way the server does, and remembers the address it already submitted. That bound also covers the two cases normalising alone cannot close: a value the API rejects outright, and a change accepted with 202 that stays pending mail verification, where the stored address legitimately does not move.

Why not redirect to /2fa

An earlier revision of this PR did redirect, mirroring kyc.screen.tsx. Review showed that is worse than the dead end it replaces: setRedirect records only the pathname (navigation.hook.ts), while the completion view is component-local state (buy.screen.tsx showsCompletion, buy-info.screen.tsx). Returning from /2fa therefore remounts an empty buy form — typed address gone, completion screen unreachable, no explanation. Routing to /account/mail instead was considered; it sends the user on an errand they usually do not need, since reaching this branch at all means the account already has a valid address.

Known limitation (pre-existing, not addressed here)

When the account already has a mail and a valid 2FA log exists, PUT /v2/user/mail answers 202 and mails a verification code instead of 403. updateMail is typed Promise<void> and every 2xx resolves identically, so the client cannot distinguish 202 from 200 and will close as if the address were stored — when it is only pending. Narrow (it needs a stale user.mail, a recent same-IP 2FA log, and a different address) and unchanged by this PR. It applies to the MailEdit step only — the app-parameter path above is bounded by its submitted-address guard. Closing it properly needs the status code surfaced through @dfx.swiss/react plus the token field this component lacks; tracked for a follow-up rather than smuggled in here.

Verification

The four CI commands pass locally on Node 20.20.2 / npm 10.8.2: npm run lint, npm run test (62 suites, 617 tests), npm run build:dev and npm run widget:dev. npx tsc -p tsconfig.build.json --noEmit, the type check the review bot runs, is also clean.

widget:dev is the build that matters for strictness: scripts/build-widget.sh does not pin CI, so GitHub Actions runs it with warnings-as-errors. scripts/build.sh pins CI=false inline, in the repo, so build:dev never enforces that gate anywhere.

(A bare npx tsc --noEmit is not a usable gate in this repo and is not part of CI: tsconfig.json sets "typeRoots": ["typings"], so @types/jest never resolves and the test files produce thousands of Cannot find name 'describe' errors on develop as well. None of them touch the files in this PR.)

Related

• fix(user): accept an unchanged mail without 2FA and harden mail verification api#4439 (open) — will let an unchanged address through without demanding 2FA, making a retry harmless server-side. This PR does not depend on it.
• fix(react): refresh the cached user after updateMail packages#196 — updateMail refreshes the cached user and sets isUserUpdating. This PR does not wait on it.

[Danswar]

Took 5 review passes to reach zero defects. The diff is small; the path to it was not, so here is what changed and why — several of these were regressions introduced by earlier revisions of this PR.

1. The first attempt redirected to /2fa on TFA_REQUIRED. That is worse than the dead end it replaced: setRedirect records only the pathname, while the completion view is component-local state, so returning remounts an empty buy form with the typed address gone. Replaced with an in-place explanation.
2. That explanation then went through ErrorHint, whose fixed opening line is "Something went wrong. Please try again." — directly contradicting it, with Confirm still enabled and no way off the screen. It now has its own short screen with an OK button, the same treatment the merge hint gets.
3. The 409 "account already exists" case was left on the generic error path, where "please try again" only sends another merge mail.
4. The mail app-parameter guard was fixed twice. Making the comparison case-insensitive was only half of it: the server applies trim and lowercase, so a padded parameter kept the guard permanently open — and the parameter is persisted to local storage, so that survives reloads. It now normalises identically and remembers the address it submitted, which also bounds the 202 pending-verification case.
5. close() is a genuine no-op on /buy/info — navigateOnClose={false}, and closeServices only acts as a widget, in an iframe, or with a redirect URI. It then blanked the component with the back button suppressed. Pre-existing, but this PR routed three more outcomes into it, so it now falls back to the account screen.

Two claims in this description were also wrong and have been corrected: a Close button that is not rendered in that branch, and npx tsc --noEmit listed as passing when it fails repo-wide on develop (a typeRoots issue unrelated to these files).

npm run lint, npm run test (62 suites / 617 tests), npx tsc -p tsconfig.build.json --noEmit, npm run build:dev and npm run widget:dev all pass on Node 20.

[TaprootFreak]

Reviewed with an independent second-model pass, then verified locally on a dedicated build host.

Verified

• tsc -p tsconfig.build.json --noEmit, lint, build:dev and widget:dev — all clean (widget:dev is the one that matters, since it runs without the pinned CI=false and treats warnings as errors)
• Full suite: 64 suites, 622 tests green
• Mutation-tested both new files: removing the fallback navigation fails exactly falls back to the account without blanking the completion when no close channel fired; removing the retry release fails exactly retries the normalized parameter after a transient rate-limit error. Neighbours stay green.
• One suite (transaction-document-error) failed once under heavy load on the build host and passes both in isolation and in a full rerun — a timing flake, not a defect.

Changed on this branch (37d5cf7, ae58301)

1. The new fallback keyed on isEmbedded/canClose, which describe the host rather than whether closeServices did anything. Two reachable holes stayed open: a widget host that leaves the optional close callback unbound (closeServices only does props.closeCallback?.(...)), and a stored redirect URI discarded as unsafe (canClose is redirectUri != null, but the sink drops it at !isSafeRedirectUri). Both left the screen blank with the back button suppressed. closeServices now returns whether a channel actually fired.
2. Three callers returned that value from Promise<void> functions, which does not compile (TS2322 in sell.screen.tsx:493, sell-info.screen.tsx:231, payment-info.tsx:113) — adjusted to a plain call plus return.
3. The mail-parameter guard was set before the request and never released, so one offline blip or a 429 — a status this route is about to start returning once DFXswiss/api#4439 lands — lost the parameter for the rest of the session. It now releases on anything transient and keeps blocking only what retrying cannot fix; the rejection is handled instead of escaping unhandled.

Note on the API-side counterpart
That coupling is worth stating explicitly: api#4439 introduces the rate limit that makes the transient case reachable in the first place. The two PRs declare each other optional, which is true for the dead end itself, but this particular interaction only appears once both are live.

Left open
No Playwright baselines for the two new screens. Not treated as a blocker: none of the 149 existing baselines covers the mail step, and the three specs mentioning "email address" only assert on page text without a screenshot, so nothing existing goes stale. Adding focused cases would still be worthwhile.

Disclosure: I contributed the commits above, so this approval covers my own changes as well.

[TaprootFreak]

@Danswar Heads-up: I pushed two commits to your branch (37d5cf7, ae58301) while reviewing.

The !navigateOnClose && !isEmbedded && !canClose fallback keys on the host rather than on whether closeServices did anything, so two cases still blanked the screen: a widget host that leaves the optional close callback unbound, and a stored redirect URI that gets discarded as unsafe. closeServices now returns whether a channel actually fired. Three callers returned that value from Promise<void> functions, which does not compile — adjusted.

Second commit: the mail-parameter guard was set before the request and never released, so a single offline blip or a 429 lost the parameter for the session. Worth noting since #4439 is about to start returning 429 on that route.

Lint, type-check, build:dev, widget:dev and all 622 tests green; both new test files mutation-checked. Approved.