Merged
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
arosiclair
requested changes
Jan 12, 2026
melvin-bot
bot
requested review from
JmillsExpensify and
thesahindia
and removed request for
a team and
JmillsExpensify
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| {pageName: CONST.MISSING_PERSONAL_DETAILS.PAGE_NAME.CONFIRM, component: Confirmation}, | ||
| ]; | ||
|
|
||
| function findPageIndex(pages: typeof formPages, pageName?: string): number { |
Contributor
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The findPageIndex function is duplicated from src/hooks/useSubPage/index.tsx. This violates the DRY principle and increases maintenance overhead.
Remove this duplicated function and use the one from the hook file directly, or export it from a shared utility file:
// In src/hooks/useSubPage/utils.ts (new file)
export function findPageIndex<TProps extends SubPageProps>(pages: Array<PageConfig<TProps>>, pageName?: string): number {
if (!pageName) {
return 0;
}
const index = pages.findIndex((page) => page.pageName === pageName);
return index !== -1 ? index : 0;
}
// In MissingPersonalDetailsContent.tsx
import {findPageIndex} from @hooks/useSubPage/utils;![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: 06ef42f187
ℹ️ 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".
Comment on lines
75
to
78
| const handleBackButtonPress = () => { | ||
| if (isEditing) { | ||
| goToTheLastStep(); | ||
| ref.current?.moveTo(lastScreenIndex); | ||
|
|
||
| Navigation.goBack(); | ||
| return; |
There was a problem hiding this comment.
In edit mode the handler calls Navigation.goBack() with no fallback. If the user refreshes on a /missing-personal-details/<step>/edit URL (or lands there directly), the navigation state has no history, so goBack() becomes a no-op and the back arrow stops working, leaving the user stuck in edit mode unless they submit. Consider navigating to the confirmation page (or using goBack with a backToRoute) so the back button still works after refresh.
Useful? React with 👍 / 👎.
Contributor
|
on it now |
dukenv0307
reviewed
Jan 22, 2026
| import variables from '@styles/variables'; | ||
| import CONST from '@src/CONST'; | ||
| import Icon from './Icon'; | ||
| import {Checkmark} from './Icon/Expensicons'; |
Contributor
There was a problem hiding this comment.
Let's use useMemoizedLazyExpensifyIcons
dukenv0307
reviewed
Jan 22, 2026
src/hooks/useSubPage/index.tsx
Outdated
| } | ||
|
|
||
| const index = pages.findIndex((page) => page.pageName === pageName); | ||
| return index !== -1 ? index : 0; |
Contributor
There was a problem hiding this comment.
Can the index be -1? In findLastPageIndex, the lastIndex can be -1
Contributor
Author
There was a problem hiding this comment.
Yes, it can if it won't find the page. Im not sure Im getting what you're hinting at here
Contributor
There was a problem hiding this comment.
Sorry for this confusion, I mean findPageIndex can't return -1, but findLastPageIndex can -> this's an inconsistency
Contributor
Author
There was a problem hiding this comment.
Ok, I get it know. I'll update it to return 0 in such case too. It will be consistent with the other method.
dukenv0307
reviewed
Jan 22, 2026
| {pageName: CONST.MISSING_PERSONAL_DETAILS.PAGE_NAME.CONFIRM, component: Confirmation}, | ||
| ]; | ||
|
|
||
| function findPageIndex(pages: typeof formPages, pageName?: string): number { |
Contributor
Author
There was a problem hiding this comment.
Moved it to new util file
Contributor
|
@MrMuzyk can you please take a look at these bot's comments? |
Contributor
Author
|
@dukenv0307 Yes, I wanted to fix everything after you complete your review too :) |
Contributor
|
@MrMuzyk I tested your branch and didn't see any issues. Once you complete fixing the comments above, we're good to approve |
Contributor
Author
|
Ive addressed all the comments and its ready for review again |
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-01-22.at.12.08.30.movAndroid: mWeb ChromeScreen.Recording.2026-01-22.at.11.55.05.moviOS: HybridAppScreen.Recording.2026-01-22.at.12.09.58.moviOS: mWeb SafariScreen.Recording.2026-01-22.at.11.54.21.movMacOS: Chrome / SafariScreen.Recording.2026-01-22.at.11.52.27.mov |
dukenv0307
approved these changes
Jan 23, 2026
arosiclair
requested changes
Jan 23, 2026
Contributor
arosiclair
left a comment
arosiclair
left a comment
There was a problem hiding this comment.
Looks great just a small change
contributingGuides/NAVIGATION.md
Outdated
|
|
||
| ## Multi-step flows with URL synchronization | ||
|
|
||
| Multi-step flows (wizards, forms with multiple screens) should use URL-based navigation via the `useSubPage` hook. This approach ensures browser navigation works correctly and page refreshes preserve the current position. |
Contributor
There was a problem hiding this comment.
Suggested change
| Multi-step flows (wizards, forms with multiple screens) should use URL-based navigation via the `useSubPage` hook. This approach ensures browser navigation works correctly and page refreshes preserve the current position. | |
| Multi-step flows (wizards, forms with multiple screens) should use URL-based navigation via the `useSubPage` hook or via basic navigation between plain static routes. This approach ensures browser navigation works correctly and page refreshes preserve the current position. |
I think it's better to mention the alternative earlier here
contributingGuides/NAVIGATION.md
Outdated
| - Form flows with multiple sections | ||
| - Settings configuration flows | ||
|
|
||
| **Usage of the hook is not an explicit requirement. Plain static routes with basic navigation between them is also an acceptable approach.** |
Contributor
There was a problem hiding this comment.
Suggested change
| **Usage of the hook is not an explicit requirement. Plain static routes with basic navigation between them is also an acceptable approach.** |
arosiclair
approved these changes
Jan 23, 2026
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
43 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/arosiclair in version: 9.3.8-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.8-3 🚀
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Fixed Issues
$ #79039
PROPOSAL:
Tests
Same as QA steps
Offline tests
QA Steps
Precondition:
Add shipping detailsPR 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
android.mp4
Android: mWeb Chrome
android-web.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios.web.mp4
MacOS: Chrome / Safari
web.mp4