fix(react): refresh the cached user after updateMail - #196
Conversation
updateMail was a bare passthrough to the API hook: unlike updateUser, verifyMail and every other sibling in this context, it neither refreshed the cached user nor toggled isUserUpdating. The endpoint answers with an empty body, so the refreshed user has to be fetched explicitly. Consumers therefore kept a user whose mail was still null after a successful update, re-rendered the same "enter your email" step and re-submitted the same address — which the API answers with 403 TFA_REQUIRED once the account carries a mail. The missing isUserUpdating also left callers unable to disable their submit button, so the confirm action stayed clickable while a request was in flight. Both were lost in the switch from changeMail to updateMail in #473; changeMail delegated to updateUser and got the refresh and the flag for free.
The refresh added in the previous commit rejected updateMail when the follow-up getUser() failed, even though the mail had already been changed server-side. Consumers would then present a successful change as an error, the user would re-submit the same address, and the account now carrying a mail would answer 403 TFA_REQUIRED — the exact loop this change set exists to remove, just on a narrower path. The refresh is now best-effort; the update result itself still propagates.
…t apply Review found that the refresh could drive a consumer into an endless update loop. PUT /v2/user/mail only persists the address when the account has none; once one exists it answers 202 and holds the new address until the emailed code is verified, so the refetched user still carries the OLD address. A consumer whose effect watches the user object and compares it against a target address — which is exactly what the services widget does for its mail parameter — then sees a new object identity with the condition still unmet, calls updateMail again, and repeats. Each pass sends another verification mail and invalidates the previous code, so the flow can never be completed. The refresh now keeps the previous object when the address did not move, so an update that changed nothing cannot re-trigger those effects.
The identity guard adopted the refetched value whenever the address differed, which included the case where the refetch resolved undefined: an OK response whose body does not parse resolves that way, and the cached user was then wiped even though the mail update itself had succeeded. That contradicted the best-effort contract of this refresh, under which a failed refetch leaves the user untouched. The previous value is now kept unless a real user came back.
|
Took 5 review passes to reach zero defects. What changed along the way, since the diff looks much smaller than the path to it:
|
Keep the two non-obvious constraints, best-effort and stable identity when the address did not move, without restating the chain below.
The refresh callback treated a cleared state as "no previous value to compare against" and wrote the fetched user back. A getUser() response arriving after the session ended therefore put the signed-out user into the context again, without a valid token behind it. updateMail is the only mutation in this file that lacks the `if (!user) return;` guard its siblings carry, because it was a bare passthrough until this branch gave it a state write.
TaprootFreak
left a comment
TaprootFreak
left a comment
There was a problem hiding this comment.
Reviewed with an independent second-model pass and verified locally on a dedicated build host.
Verified
lerna run format:check,lint,build— clean across all four packages
Changed on this branch (f9c3fbf)
The refresh callback used !prev as "nothing to compare against" and wrote the fetched user back on that branch. Since the logout effect (user.context.tsx:74-80) sets setUser(undefined), a getUser() response arriving after the session ended put the signed-out user back into the context, with no valid token behind it. That path did not exist before this branch, because updateMail was a bare passthrough with no state write — and it is still the only mutation in the file without the if (!user) return; guard its siblings carry. The condition is now prev && refreshed && refreshed.mail !== prev.mail; the !prev branch had no useful case anyway.
Caveat on the evidence
packages/react has no test script (per CONTRIBUTING, only core and bip322-multisig do), so this fix is covered by compiler and linter but by no runtime test. Adding one would mean introducing test infrastructure to the package first — a separate change, not something to smuggle in here.
Left open deliberately
isUserUpdating is a single shared boolean, so a finally can clear it while a parallel mutation is still in flight. That is the existing pattern of the whole file, which updateMail merely joins; a pending counter belongs in its own PR.
Also confirmed: getUser is memoised through useUser -> useApi, so the added dependency does not rebuild updateMail on every render, and no consumer carries it in an effect dependency list.
Disclosure: I contributed the commit above, so this approval covers my own change as well.
|
@Danswar Heads-up: I pushed one commit to your branch (f9c3fbf) while reviewing. The
|
2 participants
Problem
updateMailinUserContextProvideris a bare passthrough:Every sibling in the same file —
updateUser,verifyMail,renameAddress,updateCallSettings— setsisUserUpdatingand writes the returned user back into state.updateMaildoes neither, and becausePUT /v2/user/mailanswers with an empty body there is nothing tosetUserfrom: the refreshed user has to be fetched.Two consequences for consumers:
user.mailstaysnullafter a successful update. A screen that renders its email step onuser.mail == nullre-renders unchanged and the user submits the same address again. The API requires 2FA once an account carries a mail, so every re-submit comes back403 TFA_REQUIRED. In production this produced 1206 such 403s in 30 days against 19 successful sets.isUserUpdatingnever flips, so a consumer passing it asisLoadingto a submit button leaves that button enabled for the whole in-flight request, and a double click fires two concurrentPUTs.Both were lost in #74, which replaced
changeMailwithupdateMailhere.changeMaildelegated toupdateUser, which set the flag and refreshed the user; the replacement (return updateMailApi(mail);) inherited neither. The downstream consumer switched over six days later in DFXswiss/services#473.Change
updateMailnow setsisUserUpdating, awaits the update, fetches the refreshed user into state, and clears the flag.Two guards on that refresh, both added after review found the naive version unsafe:
getUser()fails — or resolves nothing, since an OK response whose body does not parse resolvesundefined— the cached user is left as it was and the call still succeeds. The mail has already been changed server-side, so rejectingupdateMailwould present a success as an error and push the caller back into the re-submit loop this PR exists to remove. The result of the update itself still propagates, so a consumer catching403 TFA_REQUIREDis unaffected.PUT /v2/user/mailonly persists the address when the account has none; once one exists it answers202and holds the new address until the emailed code is verified — so the refetched user still carries the old address. A consumer whose effect watches theuserobject and compares it against a target address would then see a fresh identity with its condition still unmet, callupdateMailagain, and repeat: an unbounded loop that mails a new verification code every pass and invalidates the previous one. Returning the previous object when nothing changed makes that unreachable.The signature stays
(mail: string) => Promise<void>, so this is behaviour-compatible for existing callers; a caller that ignored the cache before simply gets a correct one now.Verification
npx lerna run lint,format:checkandbuildall pass across the four packages.No version bump or CHANGELOG entry: per CONTRIBUTING, releases are cut by the publish workflow and feature PRs must not hand-bump.
Related