Migrate NetSuiteCustomListSelectorModal to a @react-navigation modal screen#91321
Migrate NetSuiteCustomListSelectorModal to a @react-navigation modal screen#91321trasnake87 wants to merge 3 commits into
Conversation
melvin-bot
Bot
requested review from
Pujan92 and
joekaufmanexpensify
and removed request for
a team and
Pujan92
![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: 493bdc3cb0
ℹ️ 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".
| shouldShowRightIcon | ||
| title={value} | ||
| description={translate('workspace.netsuite.import.importCustomFields.customLists.fields.listName')} | ||
| onPress={() => Navigation.navigate(ROUTES.POLICY_ACCOUNTING_NETSUITE_IMPORT_CUSTOM_LIST_SELECTOR.getRoute(policyID))} |
There was a problem hiding this comment.
onPress builds the selector route from policy?.id, which is optional at render time. If the user taps the picker before the policy object is hydrated (slow Onyx load / offline restore), this generates workspaces/undefined/... and opens the selector with an invalid workspace context instead of the current flow. This is a regression from the previous in-place modal behavior; use the guaranteed route policyID from the parent screen (or block navigation until it exists) to avoid invalid deep links.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — policy?.id was undefined for the brief window before the policy Onyx record hydrated, so an early tap could have produced workspaces/undefined/....
Pushed 1d02a29 to drive the picker route from policyIDParam — the parent's route URL param, which is set at screen mount well before the Onyx hydration completes. CustomFieldSubPageWithPolicy now carries policyIDParam, NetSuiteImportAddCustomList/SegmentContent pass it through, ChooseCustomListStep forwards it to the picker, and the picker accepts policyID?: string directly (no more policy?.id). onPress is also a no-op when policyID is falsy, so even the prop-not-yet-arrived edge case can never construct an undefined deep link.
Added tests/ui/NetSuiteCustomListPickerTest.tsx with two cases: a defined policyID navigates to the expected route, and an undefined policyID does not call Navigation.navigate at all. Both pass; negative-control verified (swapping the guard to if (false) causes the second test to fail with the exact workspaces/undefined/... regression).
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.
|
The NetSuiteCustomListPicker built its target route from policy?.id, which is only populated after the policy Onyx record finishes hydrating. If the picker is tapped before that, Navigation.navigate is called with workspaces/undefined/... and lands on an invalid workspace context. Plumb policyIDParam (the route param read by the parent page) down through CustomFieldSubPageWithPolicy -> ChooseCustomListStep -> the picker so the target route uses the value that is already guaranteed at the parent screen mount. Bail out of onPress when policyID is missing so we never produce an "undefined" deep link even before the prop arrives.
trasnake87
force-pushed
the
netsuite-customlist-selector-90471
branch
from
48a4f1e to
1016198
Compare
|
No product review needed |
2 participants
Explanation of Change
Migrates
NetSuiteCustomListSelectorModalfromreact-native-modal(via the shared<Modal type={RIGHT_DOCKED}>) to a dedicated@react-navigationRHP screen, matching the pattern used byDynamicExpenseLimitTypeSelectorPage(#73203) andWorkspaceCreateTaxValuePage(#90346).Key changes:
POLICY_ACCOUNTING_NETSUITE_IMPORT_CUSTOM_LIST_SELECTOR(routeworkspaces/:policyID/accounting/netsuite/import/custom-list/list-selector) as a sibling to the existingcustom-list/newparent so it doesn't collide with the parent's:subPage?parameter.NETSUITE_IMPORT_CUSTOM_LIST_SELECTORscreen constant, registered it on the Settings modal stack navigator, wired it intolinkingConfig/config.ts,WORKSPACE_TO_RHP, andSettingsNavigatorParamList.NetSuiteCustomListSelectorPage.tsxreads the policy viausePolicy, the current selection fromNETSUITE_CUSTOM_LIST_ADD_FORM_DRAFT, and on select writes bothlistNameandinternalIDto that draft, then callsNavigation.goBack().FormProviderin the parentChooseCustomListSteppicks both fields up reactively, so there's no callback plumbing.AccessOrNotFoundWrapperwithADMIN/CONTROLaccess andARE_CONNECTIONS_ENABLED, mirroring the prior modal's gating.NetSuiteCustomListPicker.tsxwith a statelessMenuItemWithTopDescriptionthat navigates to the new RHP. TheinternalIDInputIDandonInputChangeprops are no longer needed because the new page writes to the draft directly.internalIDInputID={INPUT_IDS.INTERNAL_ID}prop fromInputWrapperinChooseCustomListStep.tsx.NetSuiteCustomListSelectorModal.tsx; it had no other callers.The existing draft-clearing logic in
NetSuiteImportAddCustomListContent.tsxcontinues to handle teardown on submit/back, so no new effect cleanup is required on the selector page.Added jest tests in
tests/ui/NetSuiteCustomListSelectorPageTest.tsxcovering: (1) row construction + selected-row marking from the form draft, (2)setDraftValueswriting bothlistNameandinternalIDplusNavigation.goBackon row select, (3) empty option set + no-results header message under a non-matching search. Verified the test catches a regression by temporarily writingWRONGinstead ofitem.idforINTERNAL_ID— test fails as expected; restored, all 3 pass.Fixed Issues
$ #90471
PROPOSAL: #90471 (comment)
Tests
Add→Custom list.Choose custom liststep, tap the list picker row.No results foundheader appears.Choose custom liststep shows the selected list name.Offline tests
Same as Tests above with the device offline. The selector reads from the cached policy and draft Onyx state, so the picker should open, render the cached list rows, accept a selection, and persist it to the draft. The optimistic UI behavior matches the pre-migration modal exactly.
QA Steps
Choose custom liststep with the chosen value preserved.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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop