Refactor Netsuite import custom segments/record to use useSubPage#80996
Refactor Netsuite import custom segments/record to use useSubPage#80996bernhardoj wants to merge 18 commits intoExpensify:mainfrom
Conversation
|
@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] |
|
We need either this PR or #80995 to be merged first, so we can remove |
| import type {MenuItemProps} from '@components/MenuItem'; | ||
| import type {OfflineWithFeedbackProps} from '@components/OfflineWithFeedback'; | ||
| import type {SelectorType} from '@components/SelectionScreen'; | ||
| import {SubPageProps} from '@hooks/useSubPage/types'; |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The import statement is inconsistent with the rest of the file. All other type imports use import type syntax for type-only imports, but this one uses a regular import.
Suggested fix:
import type {SubPageProps} from '@hooks/useSubPage/types';This follows TypeScript best practices and maintains consistency with the existing import patterns in the file.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Code Review CompleteI've completed a comprehensive review of PR #80996 which refactors the NetSuite import custom segments/record flow from Review SummaryChanges Reviewed:
Key Observations:
The refactoring appears technically sound and consistent with the codebase patterns. The changes improve the navigation structure by using URL-based sub-pages instead of index-based substeps. |
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.
|
|
No product considerations, removing my review |
| pages, | ||
| startFrom, | ||
| onFinished: handleFinishStep, | ||
| buildRoute: (pageName, action) => ROUTES.POLICY_ACCOUNTING_NETSUITE_IMPORT_CUSTOM_SEGMENT_ADD.getRoute(route.params.policyID, pageName, action), |
There was a problem hiding this comment.
can we use policyID from policy.id? then we don't need to pass route to NetSuiteImportAddCustomSegmentContent
There was a problem hiding this comment.
policy.id can be undefined when the onyx is loading
| return ( | ||
| <NetSuiteImportAddCustomSegmentContent | ||
| policy={policy} | ||
| route={route} |
There was a problem hiding this comment.
Can we pass the policyID only?
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-04.at.16.03.15.movAndroid: mWeb ChromeScreen.Recording.2026-02-04.at.15.47.34.moviOS: HybridAppScreen.Recording.2026-02-04.at.15.57.13.moviOS: mWeb SafariScreen.Recording.2026-02-04.at.15.48.10.movMacOS: Chrome / SafariScreen.Recording.2026-02-04.at.15.45.24.mov |
|
Let's merge #80995 first, so we can remove CustomFieldSubStepWithPolicy type. |
|
Is it still on hold? |
|
Yes, we still on hold for #80995 |
|
@dukenv0307 removed the hold |
|
@bernhardoj We got some lint errors
|
Screen.Recording.2026-03-09.at.09.34.23.mov |
Looks like a github issue showing the old lint error. |
| animationTypeForReplace: 'push', | ||
| }, | ||
| [SCREENS.WORKSPACE.ACCOUNTING.NETSUITE_IMPORT_CUSTOM_SEGMENT_ADD]: { | ||
| animationTypeForReplace: 'push', |
There was a problem hiding this comment.
Observe that the navigation transition is incorrect
I know why this happened now. This is because we changed the animation type from replace to push. We did this for missing personal details and import custom list too.
When we go back to a screen that doesn't exist on the stack, we use replace, and by default, we set the animation to pop.
But to solve the wrong animation after the isRedirecting transition of useSubPage, we change the animation to push.
I think we should rethink of a solution to this navigation animation for useSubPage.
cc: @MrMuzyk @arosiclair
|
What is the next step here? |
|
I think we can skip the navigation transition issue for now, but we still need to solve #84475 first |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-03-13.at.10.02.37.movAndroid: mWeb ChromeScreen.Recording.2026-03-13.at.09.56.35.moviOS: HybridAppScreen.Recording.2026-03-13.at.10.08.21.moviOS: mWeb SafariScreen.Recording.2026-03-13.at.09.52.01.movMacOS: Chrome / SafariScreen.Recording.2026-03-13.at.09.44.46.mov |
Is this still broken? |
@arosiclair Yes, since we set the animation to "push" |
|
I reproduced the issue in other flows as well so let's fix it in #85431 |
…segments-n-records
|
The nav animation PR is merged. I think we can continue this one |
|
@bernhardoj just have to fix types |
|
@bernhardoj kindly bump |
|
Oh, missed this one. Thanks for the bump. Fixed. |

Explanation of Change
Fixed Issues
$ #79046
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Prerequisite: accounting feature is enabled and Netsuite is connected on the workspace
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
android.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios.mweb.mp4
MacOS: Chrome / Safari
web.mp4