[ECUK In-App 3DS] Prevent unwanted validate code resend and display rate limit errors in MFA flow#86320
Conversation
Remove SET_VALIDATE_CODE(undefined) dispatch from onCodeInput so that typing after an invalid code no longer re-triggers process() and sends an unwanted email. Resend now only happens on explicit button press. Switch requestValidateCodeAction to makeRequestWithSideEffects so Main.tsx can detect send failures (e.g. rate limit) and log them. Display backend errors from VALIDATE_ACTION_CODE.errorFields on ValidateCodePage via FormHelpMessage. Unify resend button to use requestValidateCodeAction instead of the session-based resendValidateCode.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
…/send-validate-code-bugs
JakubKorytko
left a comment
JakubKorytko
left a comment
There was a problem hiding this comment.
Two small comments, otherwise LGTM.
Avoid awaiting requestValidateCodeAction() before navigating to the magic code page. The response is only used for breadcrumb logging, while actual error display relies on Onyx failureData. This removes the unnecessary delay before navigation.
|
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
melvin-bot
bot
requested review from
JmillsExpensify
and removed request for
a team
src/libs/actions/User.ts
Outdated
| ]; | ||
|
|
||
| API.write(WRITE_COMMANDS.RESEND_VALIDATE_CODE, null, {optimisticData, successData, failureData}); | ||
| // eslint-disable-next-line rulesdir/no-api-side-effects-method |
There was a problem hiding this comment.
Please add comment why we need api response callback
There was a problem hiding this comment.
Agree, we need to note that we're only using makeRequestWithSideEffects so we can know the outcome for logging purposes
🔍 PR ReviewOverall this is a well-structured fix that addresses three real problems. The approach is sound and the changes are minimal/focused. A few observations: Looks Good
Minor Observations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 732b15c675
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Clear continuable error when user starts typing after an error | ||
| if (continuableError) { | ||
| dispatch({type: 'SET_VALIDATE_CODE', payload: undefined}); | ||
| dispatch({type: 'CLEAR_CONTINUABLE_ERROR'}); | ||
| } |
There was a problem hiding this comment.
After an INVALID_VALIDATE_CODE continuable error, this handler now clears only continuableError and leaves state.validateCode unchanged. The MFA engine in Context/Main.tsx re-runs on state.validateCode changes, so if the user retries with the same code value (a common retry path after transient failures), SET_VALIDATE_CODE writes the same value and process() is not triggered, making Verify appear to do nothing. This is a regression from the previous behavior where the stored validate code was cleared before retry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This sounds plausible, but probably not a big deal?
chuckdries
left a comment
chuckdries
left a comment
There was a problem hiding this comment.
LGTM! Just need to add a comment then I think we're golden, it works like a charm
macOs chrome
MFA.validatecode.handling.fixed.mp4
src/libs/actions/User.ts
Outdated
| ]; | ||
|
|
||
| API.write(WRITE_COMMANDS.RESEND_VALIDATE_CODE, null, {optimisticData, successData, failureData}); | ||
| // eslint-disable-next-line rulesdir/no-api-side-effects-method |
There was a problem hiding this comment.
Agree, we need to note that we're only using makeRequestWithSideEffects so we can know the outcome for logging purposes
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safaribefore.fix.movdemo.mov |
|
Bug: 2 RBRs appear. Please follow the same pattern in other pages. i.e. contact methods 
|
|
@MelvinBot please suggest code changes based on this feedback |
|
Based on the feedback about 2 RBRs appearing simultaneously, here are the suggested code changes: Problem: Lines 242-244 render three independent Fix 1 — Use a single Replace: {hasContinuableError && <FormHelpMessage message={translate('validateCodeForm.error.incorrectMagicCode')} />}
{hasAccountError && <FormHelpMessage message={getLatestErrorMessage(account)} />}
{hasValidateCodeActionError && <FormHelpMessage message={Object.values(validateCodeActionError).at(0)} />}With: {(hasContinuableError || hasAccountError || hasValidateCodeActionError) && (
<FormHelpMessage
message={
hasContinuableError
? translate('validateCodeForm.error.incorrectMagicCode')
: hasAccountError
? getLatestErrorMessage(account)
: Object.values(validateCodeActionError).at(0)
}
/>
)}This ensures only one Fix 2 — Clear Per const onCodeInput = (text: string) => {
setFormError({});
setCanShowError(false);
setInputCode(text);
// Clear backend errors
if (account?.errors) {
clearAccountMessages();
}
// Clear validate code action errors (e.g. rate limit)
if (hasValidateCodeActionError) {
clearValidateCodeActionError('actionVerified');
}
// Clear continuable error when user starts typing after an error
if (continuableError) {
dispatch({type: 'CLEAR_CONTINUABLE_ERROR'});
}
};Fix 3 — Also update Since const hasError = hasAccountError || hasContinuableError || hasValidateCodeActionError;These three changes together ensure: (a) only one error message displays at a time, (b) all error types clear on user input, and (c) the input field's error styling reflects all error sources. |
We intentionally don't clear validateCodeActionError in onCodeInput. This error comes from the backend rate limit on sending validate codes — typing a new code doesn't change the fact that we're still rate-limited. We only clear it when the user presses the resend button, which is the action that actually retries the request and may succeed after the rate limit window expires. |
Replace multiple FormHelpMessage components with a single OfflineWithFeedback wrapper around the Verify button. Errors now display directly above the submit button with priority: rate limit > invalid code > account error. This matches the pattern used in BaseValidateCodeForm and prevents duplicate error messages from appearing simultaneously.
validateCodeActionError is guaranteed non-empty when
hasValidateCodeActionError is true, making ?? {} dead code.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3270a122ad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (continuableError) { | ||
| dispatch({type: 'SET_VALIDATE_CODE', payload: undefined}); | ||
| dispatch({type: 'CLEAR_CONTINUABLE_ERROR'}); | ||
| } |
There was a problem hiding this comment.
When the user starts typing after an INVALID_VALIDATE_CODE, this block only clears continuableError but leaves state.validateCode unchanged. The MFA engine effect in Context/Main.tsx is keyed on state.validateCode (not continuableError), so if the user retries with the same code value, SET_VALIDATE_CODE does not change the dependency and process() will not run again. In practice, the Verify action can appear to do nothing until the input differs from the previous submitted code.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Once the user enters a valid code, they are immediately redirected; if they enter the same code again, we don't want them to have to confirm twice that it's invalid.
|
Larger padding above button 
Expected: 
|
|
Offline pattern is different from other magic code page. Is it expected? 
Contact methods page: 
|
|
Offline behavior is quite buggy. Please follow repro steps in this video: Screen.Recording.2026-03-26.at.2.00.29.pm.movScreen.Recording.2026-03-26.at.1.59.30.pm.mov |
|
I spent some time figuring out why the offline mode isn't working properly for you, and it turns out it's because of our dev menu, which is messing up the navigation stack for MFA. If you use DevTools to enter offline mode, everything works as expected. I don't plan to fix this in this PR, as it's outside its scope. But it might be worth creating a separate issue for https://dev.new.expensify.com:8082/test-tools Screen.Recording.2026-03-27.at.15.04.05.mp4 |
Replace p5 with ph5+pb5+mt4 on the Verify button so the gap between the error message and the button matches BaseValidateCodeForm (16px via mt4). Remove mb2 from errorRowStyles to avoid double spacing.
|
@aimane-chnaif |
| <OfflineWithFeedback | ||
| shouldDisplayErrorAbove | ||
| errors={errorMessage ? {error: errorMessage} : undefined} | ||
| errorRowStyles={[styles.mh5]} | ||
| style={[styles.w100, styles.mtAuto]} |
There was a problem hiding this comment.
If we decided to use full page offline pattern, is OfflineWithFeedback needed for this button?
Or is it just for showing error?
There was a problem hiding this comment.
Actually, you're right. I added it at the very beginning to keep the styling the same as on the page you sent me, since it uses that component.
But it's definitely too heavy, so I'm switching to the lightweight FormHelpMessage.
There was a problem hiding this comment.
I pushed the changes
OfflineWithFeedback is redundant since the page already uses FullPageOfflineBlockingView. Use FormHelpMessage which provides built-in accessibility (alert role, live region) and is designed for displaying form validation errors.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fe4188c0c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/libs/actions/User.ts
Outdated
| API.write(WRITE_COMMANDS.RESEND_VALIDATE_CODE, null, {optimisticData, successData, failureData}); | ||
| // We are using makeRequestWithSideEffects instead of API.write so that the caller can inspect the response and log failures | ||
| // eslint-disable-next-line rulesdir/no-api-side-effects-method | ||
| return API.makeRequestWithSideEffects(SIDE_EFFECT_REQUEST_COMMANDS.RESEND_VALIDATE_CODE, null, {optimisticData, successData, failureData}); |
There was a problem hiding this comment.
Switching requestValidateCodeAction from API.write to API.makeRequestWithSideEffects changes failure semantics for ResendValidateCode: HTTP/network failures (including 429 throttling) reject the promise before SaveResponseInOnyx can apply failureData, so VALIDATE_ACTION_CODE.errorFields.actionVerified and isLoading are never updated. The previous write path handled this command in SequentialQueue and explicitly applied failure data on throttling; after this change, callers like MFA only attach .then(...), so throttled resend attempts can result in no inline error state and a rejected promise instead of the expected rate-limit feedback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Hm, wouldn't that mean that this PR doesn't work at all thought? 🤔
There was a problem hiding this comment.
Experimentally, this MFA magic code input correctly handles when ResendValidateCode throttles. I guess we should double check other places that might call it though
There was a problem hiding this comment.
I'll check it right away
There was a problem hiding this comment.
Okay, I checked, and the situation isn't as black and white as the Codex makes it out to be.
The implementation using makeRequestWithSideEffects handles 99% of errors, which is why it works in this PR.
The only errors it doesn’t handle are infrastructure errors that actually come in the form of an HTTP status; all others that we send via JSON from the backend work.
However, I’ll actually revert this change because it isn’t really necessary in the context of the entire PR. And it might cause problems in that 1%.
JmillsExpensify
requested review from
joekaufmanexpensify
and removed request for
JmillsExpensify
…/send-validate-code-bugs
…t remnants API.write ensures failureData is reliably applied via SequentialQueue, including on infrastructure-level HTTP 429 errors. Remove RESEND_VALIDATE_CODE from SIDE_EFFECT_REQUEST_COMMANDS and clean up .then() chains in callers.
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Change makes sense from a product perspective
|
@aimane-chnaif |
|
@dariusz-biela Just a small nit - can you update the QA steps to not involve checking network/logs? Should be fine to just verify that no new email arrives in the tester's inbox |
|
🚧 @chuckdries has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/chuckdries in version: 9.3.52-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. The changes here are internal bug fixes to the MFA validate code page — fixing unwanted code resend on typing, displaying rate limit errors inline, and unifying the resend action. These don't alter any user-facing steps, feature names, navigation paths, or button labels documented in the help site. I reviewed the relevant articles:
Both describe the 2FA setup and login flow at a level that isn't affected by these code-level fixes. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.52-9 🚀
|
7 participants
Explanation of Change
Unwanted validate code resend on typing:
onCodeInputdispatchedSET_VALIDATE_CODE(undefined)which re-triggeredprocess()via useEffect. Sinceprocess()saw novalidateCode, it calledrequestValidateCodeAction()— sending a new email the user didn't request. Fixed by removingSET_VALIDATE_CODE(undefined)and keeping onlyCLEAR_CONTINUABLE_ERROR, which is not in theprocess()useEffect deps.Rate limit errors not displayed: Backend errors from
VALIDATE_ACTION_CODE.errorFields.actionVerified(e.g. rate limit) were not shown to the user. Added inline error display onValidateCodePageviaFormHelpMessage. Error state is applied throughfailureDatain Onyx, whichSequentialQueuereliably handles — including infrastructure-level HTTP 429 errors.Unified resend to use
requestValidateCodeAction: The resend button previously used the session-basedresendValidateCode(contactMethod)which hits a different endpoint (RequestNewValidateCode). Unified to userequestValidateCodeAction(ResendValidateCode) — the same endpoint used for the initial send inMain.tsx.Fixed Issues
$ #86285
PROPOSAL:
Tests
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2026-03-25.at.15.32.29.mp4
iOS: Native
Screen.Recording.2026-03-25.at.15.19.59.mp4
MacOS: Chrome / Safari
Screen.Recording.2026-03-25.at.15.11.25.mp4