Recreate: Migrate InitialListValueSelectorModal to dynamic @react-navigation route (with regression fixes)#94014
Conversation
…egression fixes Re-applies the migration of InitialListValueSelectorModal to a @react-navigation dynamic route (originally #91627) and adds fixes for the regressions it introduced: - Issue 93138: redirect back to the Add field page when the dynamic Initial value page is opened via refresh/deeplink without available list values, instead of rendering an empty picker. - Issue 93141: drop includePaddingTop={false} so the page no longer overlaps the device status bar on native (the prop was only safe inside the old RIGHT_DOCKED modal). - Issue 93186: sync the draft initial value back into the form so the validation error clears after a value is selected on the dynamic route. Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
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.
|
…icator Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
Fixed the failing Root cause: Fix: Passed <FullScreenLoadingIndicator
reasonAttributes={{
context: "DynamicReportFieldsInitialListValuePage",
isLoadingFormDraft: !!isLoadingFormDraft,
}}
/>Verified locally with The other failing check, |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-06-21.at.12.03.39.AM.moviOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-06-20.at.11.47.44.PM.mov |
|
@MelvinBot #93186 is still not fixed. |
…ute (issue 93186) Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
Good catch — the previous attempt at the Why it was still broken: the menu item only synced the draft value into the form when Fix: detect the draft change via a ref instead of comparing against the (already-synced) Local checks passed: prettier, lint (clean on the changed file), and |
|
@MelvinBot Mark it as ready for review |
melvin-bot
Bot
requested review from
joekaufmanexpensify and
thelullabyy
and removed request for
a team
|
@thelullabyy 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] |
|
🤖 I checked the CI status — all checks are now passing, so no fix is needed. The only The |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| onBackButtonPress={() => Navigation.goBack(backPath)} | ||
| /> | ||
| {isLoadingFormDraft || shouldRedirectToCreatePage ? ( | ||
| <FullScreenLoadingIndicator reasonAttributes={{context: 'DynamicReportFieldsInitialListValuePage', isLoadingFormDraft: !!isLoadingFormDraft}} /> |
There was a problem hiding this comment.
❌ UI-1 (docs)
FullScreenLoadingIndicator is rendered inside the same <ScreenWrapper> tree as the <HeaderWithBackButton> (lines 74-77). Because navigation (the back button) is visible alongside the loader, the user already has an escape route if loading hangs — so a full-screen loader is the wrong indicator here. This matches the UI-1 "Incorrect" example of FullscreenLoadingIndicator and HeaderWithBackButton living under the same JSX tree.
Use an ActivityIndicator centered in the content area instead, keeping the header rendered during loading:
{isLoadingFormDraft || shouldRedirectToCreatePage ? (
<View style={[styles.flex1, styles.justifyContentCenter, styles.alignItemsCenter]}>
<ActivityIndicator size={CONST.ACTIVITY_INDICATOR_SIZE.LARGE} />
</View>
) : (
// ...
)}Reviewed at: 8ac54ef | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| // required error lingers. We can't detect the change by comparing the draft against `value` (they're already in | ||
| // sync by the time this runs), so we track the previous draft value and push it through onInputChange whenever it | ||
| // changes. onInputChange triggers the form's onValidate, which clears the stale error. | ||
| const previousDraftInitialValue = useRef(draftInitialValue); |
There was a problem hiding this comment.
❌ PERF-10 (docs)
This useEffect exists solely to push a value up to the parent form via the onInputChange callback whenever the Onyx draft's initialValue changes. Using an effect to mirror state into a parent callback is the parent-communication anti-pattern PERF-10 targets — it adds an extra render pass, requires the manual useRef previous-value bookkeeping, and makes the data flow indirect.
Prefer having the parent (the create page / FormProvider) read the draft initialValue from Onyx directly and feed it into the form value, so the value flows down as a prop rather than being echoed back up from the child through an effect. That removes the need for this effect and the previousDraftInitialValue ref entirely.
Reviewed at: 8ac54ef | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
2 participants
Explanation of Change
This recreates PR #91627 (which was merged and then reverted) and additionally fixes the three regressions it introduced.
Original migration (recreated):
Migrates
InitialListValueSelectorModalfrom areact-native-modal-backed<Modal>to a@react-navigationdynamic route screen, as part of the parent migration effort (#53493).WORKSPACE_REPORT_FIELDS_INITIAL_LIST_VALUEwith path suffixinitial-list-valueandREPORT_FIELDS_CREATEas its entry screenDynamicReportFieldsInitialListValuePagethat reads from / writes to theWORKSPACE_REPORT_FIELDS_FORMdraft via Onyx, preserving the existing toggle behavior (re-selecting clears the value)useDynamicBackPathfor correct back navigation including deeplink scenariosInitialListValueSelector/index.tsxto a stateless menu item that navigates to the dynamic route viacreateDynamicRouteInitialListValueSelectorModal.tsxRegression fixes added on top:
includePaddingTop={false}from the old modal. That was only safe inside the previousRIGHT_DOCKED<Modal>; on a standalone native screen it removes the top safe-area padding and overlaps the status bar. Removed the prop so the default top padding applies.onInputChange, so theFormProvidervalidation error was never cleared. The menu item now syncs the draft initial value back into the form, which updates the form value and clears the existing error.Fixed Issues
$ #90472
$ #93138
$ #93141
$ #93186
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxScreenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screenshots/Videosundefined