fix: KYC flow doesn't trigger after adding bank account when paying another user#92154
fix: KYC flow doesn't trigger after adding bank account when paying another user#92154samranahm wants to merge 13 commits into
Conversation
|
@Eskalifer1 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] |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adjusts the personal bank account clearing logic so that, when entering the international deposit account flow, routing-related data is preserved while transient UI/error state is reset.
Changes:
- Added a
shouldPreserveRoutingDataparameter toclearPersonalBankAccountthat, when true, only clears loading/error/success flags instead of wiping the entire Onyx key. - Updated
AccountFlowEntryPointto callclearPersonalBankAccount(true)to retain routing data on mount.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libs/actions/BankAccounts.ts | Adds optional preserve flag to selectively merge-clear transient state instead of nulling the whole bank account key. |
| src/pages/settings/Wallet/InternationalDepositAccount/subPages/AccountFlowEntryPoint.tsx | Passes true to preserve routing data when clearing on entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: d1e59aa03c
ℹ️ 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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| function clearPersonalBankAccount(shouldPreserveRoutingData = false) { | ||
| /** | ||
| * Clears personal bank account state. Pass `shouldPreserveAccountData=true` to only clear UI/error | ||
| * fields while keeping routing and context fields intact. | ||
| */ | ||
| function clearPersonalBankAccount(shouldPreserveAccountData = false) { | ||
| clearPlaid(); | ||
| if (shouldPreserveRoutingData) { | ||
| if (shouldPreserveAccountData) { |
There was a problem hiding this comment.
I don't think we should do it this way, because we currently have cleanup logic throughout the entire project, so I think we should keep it.
I think that when we added the cleanup logic, we simply forgot that onSuccessFallbackRoute shouldn’t be cleaned up, so I think we should just fix that. If we leave it as it is now, we’ll definitely get regressions and incorrect behavior.
I think it should look something like this:
function clearPersonalBankAccount(preservedData?: Partial<PersonalBankAccount>) {
clearPlaid();
Onyx.set(ONYXKEYS.PERSONAL_BANK_ACCOUNT, preservedData ?? null);
Onyx.set(ONYXKEYS.FORMS.PERSONAL_BANK_ACCOUNT_FORM_DRAFT, null);
clearPersonalBankAccountSetupType();
}And pass it like this:
useEffect(() => {
clearPersonalBankAccount(personalBankAccount?.onSuccessFallbackRoute ? {onSuccessFallbackRoute: personalBankAccount.onSuccessFallbackRoute} : undefined);
}, []);What do you think about that?
There was a problem hiding this comment.
Agreed, that's much cleaner approach. Updated clearPersonalBankAccount to take preservedData?: Partial<PersonalBankAccount> with Onyx.set(preservedData ?? null), and the entry point now passes only onSuccessFallbackRoute. All other call sites stay as a full reset, so no behavior change of any kind there.
…g data and update its usage in AccountFlowEntryPoint
…nkAccount function
Eskalifer1
left a comment
Eskalifer1
left a comment
There was a problem hiding this comment.
Please fill out the Explanation of Change section
Let's also add some information to the Offline tests section, specifically “N/A” and the reason why there are no offline tests here
|
I also think we should update the |
Co-authored-by: Eskalifer1 <artemkryt1800@gmail.com>
|
Thanks @Eskalifer1 for taking a look, I'm looking into this and will update the PR shortly. |
|
Sure! Let me know when this PR will be ready for next round of reviews! |
|
@codex review |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp91873-android-native.movAndroid: mWeb Chrome91873-android-web.moviOS: HybridApp91873-ios-native.moviOS: mWeb Safari91873-ios-web.movMacOS: Chrome / Safari91873-web.mov |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 249dba04e1
ℹ️ 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".
| useEffect(() => { | ||
| clearPersonalBankAccount(); | ||
| }, []); | ||
| clearPersonalBankAccount(onSuccessFallbackRoute ? {onSuccessFallbackRoute} : undefined); |
There was a problem hiding this comment.
When a user starts a Pay/KYC flow that sets onSuccessFallbackRoute and then backs out from this entry point, nothing in the entry-point back path clears PERSONAL_BANK_ACCOUNT; with this change, the stale fallback route is preserved instead of being wiped. If the same user later starts a normal Wallet > Add bank account flow, this line preserves that old route again, and AddPersonalBankAccountPage.exitFlow(true) will call continueSetup(...) instead of just returning to Wallet after success. Please only preserve the fallback for the active KYC-launched flow or clear it when the add-bank flow is abandoned.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is valid concern. I think we should find a solution to this. Although it's unlikely, it's still possible
There was a problem hiding this comment.
Updated the logic to handle this edge case.
There was a problem hiding this comment.
Hi @samranahm, I don't really think that's the approach we want to take. After all, it makes more sense to clear the previous data on entry (which is much more stable) rather than on exit—plus, that's the logic that's been working for a very long time.
Can you check how onSuccessFallbackRoute was being cleared before this useEffect was added? Maybe that will give us a little more information.
There was a problem hiding this comment.
@MelvinBot How can this be fixed using this approach:
const [personalBankAccount, personalBankAccountResult] = useOnyx(ONYXKEYS.PERSONAL_BANK_ACCOUNT);
const isLoadingPersonalBankAccount = isLoadingOnyxValue(personalBankAccountResult)
useEffect(() => {
if (isLoadingPersonalBankAccount) {
return;
}
clearPersonalBankAccount(onSuccessFallbackRoute ? {onSuccessFallbackRoute} : undefined);
}, [isLoadingPersonalBankAccount]);How was onSuccessFallbackRoute cleared before this useEffect was added?
There was a problem hiding this comment.
Short version: onSuccessFallbackRoute has never had a dedicated clear — it's only ever wiped as a side effect of a full clearPersonalBankAccount() (which nulls the whole PERSONAL_BANK_ACCOUNT key). The entry-point mount clear was one of those full wipes, and it was the only thing protecting this screen from a stale route. So the isLoadingOnyxValue version fixes your deps concern, but on its own it does not close the stale-route hole chatgpt-codex-connector flagged in this thread — you need one more piece for that.
Q2 — How was onSuccessFallbackRoute cleared before this useEffect?
There is no route-specific clear anywhere in the codebase. The field's whole lifecycle:
- Set before a pay/KYC flow begins, via
setPersonalBankAccountContinueKYCOnSuccess— called fromSettlementButton,PaymentUtils,Search,WalletPage. - Consumed on success by
exitFlow(true)→continueSetup(it reads the route to navigate; it does not clear it). - Cleared only by full
clearPersonalBankAccount()calls scattered through the flow: theAccountFlowEntryPointmount clear (the long-standing one you're referring to), plusAddBankAccount.tsx:73,AddPersonalBankAccountPage.tsx:52,PersonalInfo.tsx:34-37(KYC continuation),FeesAndTerms.tsx:27(end of flow).
So on main, entering this screen always nuked the entire object. That wipe is simultaneously (a) what kept a stale route from leaking into a later flow, and (b) what wiped the route this PR needs to keep — the bug. That's the tension: the moment you preserve the route on entry, you remove the thing that used to protect against stale routes. Note openPersonalBankAccountSetupView({}) — the generic Wallet → Add bank account entry — does not clear PERSONAL_BANK_ACCOUNT, which is precisely why chatgpt-codex-connector's scenario is reachable.
Q1 — Making the isLoadingOnyxValue approach work
It needs two pieces, not one.
Piece A — the entry-point effect (your version, corrected)
const [personalBankAccount, personalBankAccountResult] = useOnyx(ONYXKEYS.PERSONAL_BANK_ACCOUNT);
const isLoadingPersonalBankAccount = isLoadingOnyxValue(personalBankAccountResult);
const onSuccessFallbackRoute = personalBankAccount?.onSuccessFallbackRoute;
useEffect(() => {
if (isLoadingPersonalBankAccount) {
return;
}
clearPersonalBankAccount(onSuccessFallbackRoute ? {onSuccessFallbackRoute} : undefined);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isLoadingPersonalBankAccount]);with clearPersonalBankAccount(preservedData?: Partial<PersonalBankAccount>) doing Onyx.set(PERSONAL_BANK_ACCOUNT, preservedData ?? null).
Two things to get right:
onSuccessFallbackRoutemust be derived frompersonalBankAccount?.onSuccessFallbackRoute(your snippet references it but doesn't declare it).- The
isLoadingOnyxValueguard is load-bearing, not cosmetic:useOnyxreturnsundefinedwhile the key hydrates. Without the guard, the first render readsundefined→clearPersonalBankAccount(undefined)→Onyx.set(..., null)→ the route is wiped again (the original bug). Gating on hydration guarantees you read the real route before clearing, and keeping the dep array as[isLoadingPersonalBankAccount]only means the re-seed write won't re-trigger the effect (which is also what fixes your duplicate-clean concern). ✅
Piece B — close the stale-route hole (the concern this thread is about)
Piece A alone still preserves the route on every entry, so the abandon path you and chatgpt-codex-connector discussed is still open: set route → land here → preserve → back out (nothing clears it) → later generic flow reuses the stale route.
Since you (rightly) want to keep clear-on-entry rather than move it to unmount, the targeted fix is to wipe fully when the flow is actually abandoned at this entry screen. Wrap the back handler so leaving without proceeding does a full reset:
const handleBackButtonPress = () => {
clearPersonalBankAccount(); // full wipe — abandoning the flow must not leave a stale route
onBackButtonPress();
};and pass handleBackButtonPress to HeaderWithBackButton. That preserves the route while moving forward through the flow, but clears it on a real abandon — satisfying both "clear on entry is stable" and "no stale route."
To be transparent: I verified the data flow and call sites directly from the code (links above), but I did not reproduce the stale-route scenario in a running app — it's plausible and you already agreed it's valid, just flagged here as analysis rather than a confirmed repro.
There was a problem hiding this comment.
@samranahm I think we can try to use this approach
There was a problem hiding this comment.
@Eskalifer1 We have same approach in AddPersonalBankAccountPage to clear the data on unmount, this make sure we clear everything when flow finish to make sure each new flow start fresh.
This is already established approach through out the codebase.
There was a problem hiding this comment.
It's already 1AM for me, so i will take a closer look tomorrow
…fter-adding-account
|
@Eskalifer1 Preserving We actually merge Here's what I found to be the most professional solution Wipe |
@Eskalifer1 This one is not needed because we only care about the I've updated the test step to reflect this and added a new test as well. |
Short answersQ2 — How was Q1 — Can it be fixed with your approach? Yes, but it needs a one‑line signature change to Q2 details: the full lifecycle of
|
|
@MelvinBot i was asking how we can fix this and how it was working before |
…fter-adding-account
|
@Eskalifer1 PR ready for another review, please take a look. Unit test failure is not related to this PR. |
|
Hi @samranahm take a look here pls |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00b560618b
ℹ️ 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".
| clearPersonalBankAccount(); | ||
| }, []); | ||
| // Clear on unmount so fallback route remains available during the next flow and is clear when the flow ends. | ||
| useEffect(() => clearPersonalBankAccount, []); |
There was a problem hiding this comment.
With this cleanup moved entirely to unmount, a stale PERSONAL_BANK_ACCOUNT.onSuccessFallbackRoute can survive into a later non-KYC setup. For example, if a payment/KYC attempt calls setPersonalBankAccountContinueKYCOnSuccess and the user cancels before this entry screen ever unmounts, then later goes through Settings > Wallet > Add bank account > Get reimbursed > United States, this screen no longer clears that old fallback; choosing Plaid/manual only merges setupType, and AddPersonalBankAccountPage.exitFlow(true) will call continueSetup with the stale route after success, triggering the KYC/enable-payments path for a get-reimbursed flow. Preserve the intended fallback for the current KYC flow, but clear stale fallback state when starting an ordinary bank-account flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Hi @samranahm Yes, I think that's a valid concern. The problem here is that it also occurs during cleanup on mount.
I think we should revert to the logic I proposed here. That way, we’ll fix the bug and avoid new regressions.
I'm convinced that cleaning up on unmount is a very bad idea and will introduce a lot of new regressions, since the app has been working this way for months—cleaning up data on mount—and changing that now just because of one small bug is a bad idea.
At the very least, I can say that if the user simply closes the page, the cleanup in useEffect won’t run, so the next time the user starts the flow, all their data will be stale, not just onSuccessFallbackRoute—so this is a bad idea, and that’s just one example.
We need to perform cleanup on mount, as is currently the case.
Perhaps we’ll find a solution once we figure out how cleanup worked before this cleanup was added; please check how it worked back then.
Please do not use AI; it’s very noticeable, and AI might give an incorrect result, which will only confuse us further.
We can add a separate cleanup on unmount just for onSuccessFallbackRoute; this way, we’ll preserve the existing logic and fix the bug:
useEffect(() => {
if (isLoadingPersonalBankAccount) {
return;
}
clearPersonalBankAccount(onSuccessFallbackRoute ? {onSuccessFallbackRoute} : undefined);
}, [isLoadingPersonalBankAccount]);
useEffect(() => () => setPersonalBankAccountContinueKYCOnSuccess(null), []);Let me know what do you think
This comment was marked as outdated.
This comment was marked as outdated.
|
@MelvinBot Here's the bug we're dealing with: Bug: When paying another user with a personal bank account, the KYC flow does not trigger after adding the account. Root Cause We want to fix this by clearing everything in Do you have any idea how we can fix that? Is it good to clear only useEffect(() => () => setPersonalBankAccountContinueKYCOnSuccess(null), []); |
|
Short answer: no.
Root causeYou're managing the lifecycle of a "continue into KYC" intent from a consuming screen's mount/unmount, but the value lives in persistent Onyx. That mismatch is unwinnable:
There's no way to distinguish a freshly-set intent from a stale one by preserving it across a mount, because it's the same persisted key. Recommended fix — reset at the entry, before the intent is writtenMove the reset to the moment a fresh flow begins (race-free, since it runs before navigation), and stop clearing on any screen's mount/unmount. Option A (minimal). Fold the reset into the setter so writing a fresh intent wipes stale state in the same step: function setPersonalBankAccountContinueKYCOnSuccess(onSuccessFallbackRoute: Route) {
clearPersonalBankAccount(); // wipes any stale state, incl. a leftover route
Onyx.merge(ONYXKEYS.PERSONAL_BANK_ACCOUNT, {onSuccessFallbackRoute}); // seeds the fresh intent
}
Option B (most robust). Stop storing I'd ship A for this PR. Why A is race-free (full flow trace)The pay flow sets the intent then navigates, and the destination screen used to clear it on mount:
Folding the clear into the setter ( All call sites set the intent at the very start of the flow, before any bank-account data exists, so clearing there is always safe. Next Steps: Reply with |
|
@samranahm Take a look at Option A that Melvin suggested. What do you think of it? It seems like the best idea is to do the cleanup first and then handle the navigation, but it looks like that would require rewriting quite a bit of code. |
|
@MelvinBot We have two options:
Which of these two options is better? And which of these options falls within the scope of this PR? |
|
Option 1 is better, and the initiation-time cleanup it relies on is required in this PR either way. Why Option 1 wins: both options add the same "clear when the flow is initiated" step — the difference is that Option 2 also keeps a partial "clear everything except On scope — this is the key point: the cleanup-at-initiation is not optional follow-up; it's needed to avoid a regression this PR introduces.
So whichever option you pick, you have to add the initiation-time reset to not regress. Option 1 is just the minimal, clean way to do it — keep it in this PR. Concrete shape of Option 1
Note: Option 2 needs steps 1's-equivalent + 3 too, plus the partial mount-clear — so it's strictly more code for the same result. Next Steps: Reply with |
|
@MelvinBot I understand that we need to add this cleanup everywhere the flow can start. Could you help identify all of those entry points? From my understanding, the cleanup should be added not only in places where we set Link all places where we need to add this cleanup. Maybe we can do something so we don't have to repeat the same thing in every place where logic related to |
|
Mapped every entry point. Short answer: the non‑setter flows really do funnel through a single function ( The two live consumers
All entry points that start the personal‑bank‑account flowA. Sites that SET the route (4):
B. Non‑setter flows — all go through
C. Outliers that raw‑
Why "clear once in
|
|
@samranahm Please take a look at the solutions Melvin has suggested to see which one works best. Do you perhaps have another solution that would work better? |
|
Looking into it, I'll explore each one and will let you know about my findings regarding these. |
|
@samranahm Thank you! I'll be OOO part of the day tomorrow and all next week, so I'll review everything once I'm back. If you have any higher-priority tasks, feel free to work on them now! |
Explanation of Change
When paying another user with a personal bank account, the KYC flow wasn't triggering after adding the account. The cause was
AccountFlowEntryPointcallingclearPersonalBankAccount()on mount, which wiped theonSuccessFallbackRoutethat the pay flow sets before this screen and thatAddPersonalBankAccountPagereads after it, so the app could no longer continue into KYC.To fix this we update the reset on unmount instead of mount, so the route stays available while the flow is open and is cleared only when the user finishes setup or goes back.
Fixed Issues
$ #91873
PROPOSAL: #91873 (comment)
Tests
Precondition:
Test 01
Test 02
Offline tests
N/A – Bank account setup can only be run while online.
QA Steps
Same as test
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Android.Native.mp4
Android: mWeb Chrome
Android.mWeb.Chrome.mp4
iOS: Native
IOS.Native.mp4
iOS: mWeb Safari
IOS.mWeb.Safari.mp4
MacOS: Chrome / Safari
macOS.Chrome.mp4