feat: Refator USD flow to use useSubPage#90302
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
melvin-bot
Bot
requested review from
dukenv0307 and
flaviadefaria
and removed request for
a team and
dukenv0307
|
@dukenv0307 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] |
![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: b6d3e94ddb
ℹ️ 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".
| (onfidoData: OnfidoData) => { | ||
| verifyIdentityForBankAccount(Number(bankAccountID), {...onfidoData, applicantID: onfidoApplicantID}, policyID); | ||
| updateReimbursementAccountDraft({isOnfidoSetupComplete: true}); | ||
| onSubmit?.(); |
There was a problem hiding this comment.
Calling onSubmit immediately in handleOnfidoSuccess advances to the Company step before verifyIdentityForBankAccount finishes, so a failed/slow API write (e.g., offline or backend error) can still move the user forward with identity verification not actually persisted. Previously this step transition was gated by the backend-updated reimbursement state; this change removes that guard and can leave users in later steps with an invalid verification state.
Useful? React with 👍 / 👎.
|
No new product considerations - removing my assignment and unsubscribing. |
|
I'll review it. don't know why Melvin removed me |
|
on it now |
Screen.Recording.2026-05-19.at.09.22.59.mov |
|
Looks good Screen.Recording.2026-05-19.at.09.37.32.mov |
Found the root cause, working on a fix right now |
|
@dukenv0307 I've fixed the error you've spotted and lint issue. Should be good to review further |
|
@MrMuzyk I can still reproduce the date picker issue on Safari Screen.Recording.2026-05-19.at.23.17.03.mov |
|
Let me double check 🤔 |
|
I think the page is having some issues right now so I'll get back to it tomorrow morning |
|
I wasn't able to fix it and Im not sure whether we want to spend more time on it now. While it is poor UX and should be fixed it is only under very specific condition on a single platform. It doesn't block user from interacting with the app and modal still can be closed if you click anywhere outside of it. In order to fix it on all other platforms I've used existing mechanism that was already implemented but not in use for this component - Im almost certain that the same bug can be reproduced in other places that use this prop. @arosiclair Could we maybe give it the same treatment as we did with flicker bug? Open a separate issue for it and revisit it later? |
|
Yeah I think that makes sense since the bug is not critical - it only occurs on mWeb Safari and you can just dismiss the popup when it happens. Made an issue here: #91311 |
|
@dukenv0307 Did everything else work fine? |
|
on it now |
|
🚧 @arosiclair has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
I requested ad-hoc testing from QA here |
|
@MrMuzyk There are some conflicts, can you please take a look? |
Will do soon |
@arosiclair Hi, how is the testing going? |
|
No update yet. I just bumped them |
|
FYI Im off tomorrow so will get back to it on wednesday |
|
Regression is completed, and no issues were found. |
There was a problem hiding this comment.
Can we add docs to both of these functions that explain what we can use this for?
|
|
||
| type Screens = Partial<Record<Screen, () => React.ComponentType>>; | ||
|
|
||
| // Reimbursement Account flow animations glitch on low-end Android devices in Chrome mWeb https://github.com/Expensify/App/issues/87658 so we intentionally disable them |
There was a problem hiding this comment.
| // Reimbursement Account flow animations glitch on low-end Android devices in Chrome mWeb https://github.com/Expensify/App/issues/87658 so we intentionally disable them |
Let's move this down to the disabling line
| [SCREENS.TWO_FACTOR_AUTH.SUCCESS]: { | ||
| animationTypeForReplace: 'push', | ||
| }, | ||
| ...(IS_MOBILE_CHROME ? Object.fromEntries(REIMBURSEMENT_ACCOUNT_FLOW_SCREENS.map((screen) => [screen, {animation: Animations.NONE}])) : {}), |
There was a problem hiding this comment.
| ...(IS_MOBILE_CHROME ? Object.fromEntries(REIMBURSEMENT_ACCOUNT_FLOW_SCREENS.map((screen) => [screen, {animation: Animations.NONE}])) : {}), | |
| // Reimbursement Account flow animations show glitches on low-end Android devices in Chrome mWeb so we intentionally disable them here. | |
| // See https://github.com/Expensify/App/issues/89915 | |
| ...(IS_MOBILE_CHROME ? Object.fromEntries(REIMBURSEMENT_ACCOUNT_FLOW_SCREENS.map((screen) => [screen, {animation: Animations.NONE}])) : {}), |
| // After a disconnect, wait for the reset API to finish before navigating to the entry point. | ||
| const prevPendingActionRef = useRef(pendingAction); | ||
| useEffect(() => { | ||
| const prev = prevPendingActionRef.current; | ||
| prevPendingActionRef.current = pendingAction; | ||
| if (prev === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE && pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) { | ||
| Navigation.navigate(ROUTES.BANK_ACCOUNT_WITH_STEP_TO_OPEN.getRoute({policyID, backTo})); | ||
| } | ||
| }, [pendingAction, policyID, backTo]); | ||
|
|
||
| // While the disconnect is in flight, show the existing flow loader so the success screen doesn't re-render with cleared bank data | ||
| if (pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) { | ||
| return <ReimbursementAccountLoadingIndicator onBackButtonPress={onBackButtonPress} />; | ||
| } |
There was a problem hiding this comment.
I'm a bit confused by this code. What scenario is this handling?
There was a problem hiding this comment.
When user connected the account and decided to immediately disconnected it. This case stopped working after our refactor and while QAs didnt yet find it I did when testing and fixed it
This reverts commit 2a6276e. # Conflicts: # src/ROUTES.ts # src/components/Navigation/DebugTabView.tsx # src/libs/Navigation/AppNavigator/ModalStackNavigators/index.tsx
MrMuzyk
force-pushed
the
feat/79048-restore
branch
from
0930dbb to
e9a2719
Compare
|
🚧 @arosiclair 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. |
6 participants
Explanation of Change
Restores reverted #88193 together with fixes for newly found bugs
Fixed Issues
$ #79048
PROPOSAL:
Tests
Same as QA steps +
Scenario A:
Scenario B:
Scenario C:
Offline tests
QA Steps
Note
Known issues:
Prerequisites: being an admin of at least one workspace, the workspace tested should have USD as the workspace currency
Partially setup bank accounts
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.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
ios-native.mp4
iOS: mWeb Safari
ios-web.mp4
MacOS: Chrome / Safari
web.mp4