Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🚨 EDIT: the problem is only with emulator, but leaving this comment to make sure we're aware of this problem I'm opening the PR for review, despite finding an issue on Android web Video: Screen.Recording.2026-04-01.at.10.16.19.mp4I'm still investigating the issue, but I don't want to block the PR on that. I also checked main - it's occurring there as well, both in non-USD flow and in the missing personal details. It's only happening for the pages that use the useSubPage hook and logic. It wasn't happening for sure when we worked on and reviewed the PR for non-USD flow. Looks like some changes later affected these flows. Video for missing details flow on main: Screen.Recording.2026-04-01.at.12.27.05.mp4
|
| <SelectionList | ||
| data={codeOptions} | ||
| ref={selectionListRef} | ||
| data={isReady ? codeOptions : []} |
There was a problem hiding this comment.
Why do we use isReady here? It dont see any issue when enabling autofocus
There was a problem hiding this comment.
there was a problem with focus and the animation that couln't be resolved like above (shouldDelayFocus) - without it we had a janky/cut off animation for this step. To fix this we delay rendering the full list until after the page transition animation completes and also delay the focus.
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-deprecated | ||
| const {componentToRender: SubStep, isEditing, screenIndex, nextScreen, prevScreen, moveTo, goToTheLastStep} = useSubStep({bodyContent, startFrom: 0, onFinished: submit}); | ||
| const {CurrentPage, isEditing, pageIndex, nextPage, prevPage, moveTo} = useSubPage<SubPageProps>({pages, startFrom: 0, onFinished: submit, buildRoute}); |
There was a problem hiding this comment.
same as above, if pages contain 1 item only, do we need to use useSubPage?
Screen.Recording.2026-04-01.at.22.38.48.mov |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78ba0f5a37
ℹ️ 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/pages/ReimbursementAccount/USD/ConnectBankAccount/components/FinishChatCard.tsx
Show resolved
Hide resolved
@dukenv0307 I will try to fix it, but BeneficialOwners step is a bit complicated - you would need either go to the ubo's list or is anyone an ubo. You can check on staging - we always navigate back to the first step EDIT: fixed |
|
@dukenv0307 I think it's ready for re-review |
|
Great, lemme take a look again |
| const [shouldDisplayChildItems, setShouldDisplayChildItems] = useState(false); | ||
| const {translate} = useLocalize(); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
I think we should use useFocusEffect
There was a problem hiding this comment.
This is intentionally only on mount - we're deferring the list render until the navigation transition finishes. useFocusEffect would make it run on every focus, resetting isReady each time and showing an empty list unnecessarily when the user navigates back.
There was a problem hiding this comment.
@koko57 It'll cause the inconsistency because if we go back, other inputs will be focused automatically
Screen.Recording.2026-04-02.at.22.48.32.mov
There was a problem hiding this comment.
@dukenv0307 I don't understand - how this would affect other inputs
There was a problem hiding this comment.
this is only a change that will work on this step
There was a problem hiding this comment.
and if you meant that the input on the IndustryCode step won't be focused when you go back to this step - no worries - it is focused correctly
Screen.Recording.2026-04-02.at.18.11.03.mp4
There was a problem hiding this comment.
and if you meant that the input on the IndustryCode step won't be focused when you go back to this step - no worries - it is focused correctly
That's weird, it doesn't work on my side
There was a problem hiding this comment.
@arosiclair can you please trigger the adhoc build?
There was a problem hiding this comment.
@dukenv0307 you're right - I checked once again and the focus is not happening. Previously I entered the flow from the partially setup bank account so the page was not in the stack, so it mounted
There was a problem hiding this comment.
@dukenv0307 done! thanks for pointing this out!
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-04-02.at.22.10.18.movAndroid: mWeb ChromeScreen.Recording.2026-04-02.at.21.53.25.moviOS: HybridAppScreen.Recording.2026-04-02.at.21.56.57.moviOS: mWeb SafariScreen.Recording.2026-04-02.at.21.47.50.movMacOS: Chrome / SafariScreen.Recording.2026-04-02.at.21.42.59.mov |
|
@koko57 Great job. I left 1 minor comment, the rest looks good |
@dukenv0307 this one #86645 (comment) ? - I've already answered #86645 (comment) |
|
SpellCheck fail for unrelated change |
|
🚧 @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! 🧪🧪
|
|
@koko57 Thank you. I've approved the PR but we still have 1 failed check. I assume it doesn't come from our PR |
|
@dukenv0307 yes, it seems that it's from another PR. I will merge with main once again in a while |
|
@dukenv0307 after merging with main the spell check was still failing, so I fixed it here (although it's not from our PR) |
| * @param onSubmit - callback that navigates to the next step | ||
| * @returns markSubmitting - call this right after firing the API action | ||
| */ | ||
| export default function useReimbursementAccountSubmit(onSubmit?: () => void) { |
There was a problem hiding this comment.
| export default function useReimbursementAccountSubmit(onSubmit?: () => void) { | |
| export default function useReimbursementAccountSubmitCallback(onSubmit?: () => void) { |
| if (isEditing) { | ||
| Navigation.goBack(buildRoute(SUB_PAGE_NAMES.CONFIRMATION)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is this necessary or should the confirmation page already be the previous route in the history?
| if (currentSubPage === SUB_PAGE_NAMES.IS_USER_UBO) { | ||
| setDraftValues(ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM, {ownsMoreThan25Percent: value}); | ||
|
|
||
| // User is an owner but there are 4 other owners already added, so we remove last one |
There was a problem hiding this comment.
Can we keep this comment?
| } else if (currentSubPage === SUB_PAGE_NAMES.IS_ANYONE_ELSE_UBO) { | ||
| navigateBackToSubPage(SUB_PAGE_NAMES.IS_USER_UBO); | ||
| } else if (currentSubPage === SUB_PAGE_NAMES.UBOS_LIST && !canAddMoreUBOS) { | ||
| navigateBackToSubPage(SUB_PAGE_NAMES.IS_ANYONE_ELSE_UBO); | ||
| } else if (currentSubPage === SUB_PAGE_NAMES.UBOS_LIST && isAnyoneElseUBO) { | ||
| navigateBackToSubPage(SUB_PAGE_NAMES.ARE_THERE_MORE_UBOS); | ||
| } else if (currentSubPage === SUB_PAGE_NAMES.UBOS_LIST && isUserUBO && !isAnyoneElseUBO) { | ||
| navigateBackToSubPage(SUB_PAGE_NAMES.IS_ANYONE_ELSE_UBO); |
There was a problem hiding this comment.
Why are these still necessary? Would the previous route in the history not be correct in these cases?
| useFocusEffect( | ||
| useCallback(() => { | ||
| const timeout = setTimeout(() => { | ||
| setIsReady(true); | ||
| selectionListRef.current?.focusTextInput(); | ||
| }, CONST.ANIMATED_TRANSITION); | ||
|
|
||
| return () => clearTimeout(timeout); | ||
| }, []), | ||
| ); |
There was a problem hiding this comment.
Can you add a comment explaining why this is necessary?
| INCORPORATION_DATE: 'incorporation-date', | ||
| INCORPORATION_STATE: 'incorporation-state', | ||
| INCORPORATION_CODE: 'incorporation-code', |
There was a problem hiding this comment.
Can you update these routes to match what we have in the doc? A few others seem to differ as well
Explanation of Change
Fixed Issues
$ #79048
PROPOSAL: -
Tests
Prerequisites: being an admin of at least one workspace, the workspace tested should have USD as the workspace currency
Partially setup bank accounts
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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-04-01.at.10.18.15.mp4
Android: mWeb Chrome
iOS: Native
Screen.Recording.2026-03-31.at.16.17.32.mp4
Screen.Recording.2026-03-31.at.16.15.19.mp4
iOS: mWeb Safari
Screen.Recording.2026-03-30.at.16.46.15.mp4
MacOS: Chrome / Safari
Screen.Recording.2026-03-30.at.14.41.57.mp4