Migrate YearPickerModal to a @react-navigation modal screen#91106
Migrate YearPickerModal to a @react-navigation modal screen#91106trasnake87 wants to merge 5 commits into
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
@parasharrajat 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9781e56e2a
ℹ️ 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".
| entryScreens: [ | ||
| SCREENS.SETTINGS.PROFILE.DATE_OF_BIRTH, | ||
| SCREENS.SETTINGS.PROFILE.STATUS_CLEAR_AFTER_DATE, | ||
| SCREENS.MONEY_REQUEST.STEP_DATE, | ||
| SCREENS.MONEY_REQUEST.STEP_TIME, | ||
| SCREENS.MONEY_REQUEST.STEP_TIME_EDIT, | ||
| SCREENS.MONEY_REQUEST.SPLIT_EXPENSE_CREATE_DATE_RANGE, | ||
| SCREENS.SEARCH.ROOT, | ||
| SCREENS.SEARCH.ADVANCED_FILTERS_DATE_RHP, | ||
| SCREENS.SEARCH.EDIT_MULTIPLE_DATE_RHP, | ||
| SCREENS.CHRONOS_SCHEDULE_OOO_ROOT, | ||
| SCREENS.SCHEDULE_CALL, | ||
| ], |
There was a problem hiding this comment.
Add missing YEAR_SELECTOR entry screens for DatePicker flows
CalendarPicker now always opens the year chooser through a dynamic route, and getStateFromPath() only authorizes that route when the current screen is in DYNAMIC_ROUTES.YEAR_SELECTOR.entryScreens; otherwise it falls back as an unauthorized dynamic route. The new allowlist here omits several screens that still render DatePicker (SCREENS.SETTINGS.WALLET.PERSONAL_CARD_EDIT_TRANSACTION_START_DATE, SCREENS.WORKSPACE.COMPANY_CARDS_ASSIGN_CARD_TRANSACTION_START_DATE, SCREENS.WORKSPACE.COMPANY_CARD_EDIT_TRANSACTION_START_DATE, and SCREENS.DEBUG.DYNAMIC_DETAILS_DATE_TIME_PICKER_PAGE), so tapping the calendar year on those pages will not navigate to the new year selector.
Useful? React with 👍 / 👎.
|
Good catch on the entry screens. Rather than extend the allowlist, I switched Reasoning: Added a regression test in |
83b38b6 to
2a4c5ce
Compare
Replace the react-native-modal based YearPickerModal, rendered inside CalendarPicker, with a dynamic @react-navigation route so it animates consistently with the surrounding native stack. - Add the DYNAMIC_YEAR_SELECTOR dynamic route and the standalone DynamicYearSelectorPage screen (year list + search), built from route query params. - Lift the picker result out of CalendarPicker local state into a transient Onyx key, tagged with a per-instance contextID so the correct CalendarPicker consumes it (range pickers mount two). - CalendarPicker navigates to the year route and applies the returned year; hosts that render it inside a popover pass onBeforeOpenYearPicker to dismiss the popover before the year screen is shown. - Remove YearPickerModal and the now-unused year list state. - Update CalendarPicker tests for the navigation flow and add coverage for the year selection action.
2a4c5ce to
364652d
Compare
CalendarPicker is a generic component reached from many screens (date input fields, DateSelectPopup, RangeDatePicker, DatePresetFilterBase, ScheduleCallPage, ...). The previous in-place YearPickerModal had no screen restriction, so a hand-maintained entryScreens allowlist silently broke the year picker on omitted screens (e.g. company/personal card transaction start date pages, debug datetime picker). Use the '*' wildcard so the year selector stays reachable from every CalendarPicker host and does not regress when new date-input screens are added. Added a regression test asserting the route remains unrestricted.
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.
|
364652d to
883930e
Compare
|
Fix the checklist @trasnake87 and add vids for all platforms. |
|
Added the Android: mWeb Chrome video and ticked the offline-steps box (the section was already in the body, just hadn't been checked). iOS to follow. |
| const [currentDateView, setCurrentDateView] = useState(() => getInitialCurrentDateView(value, minDate, maxDate)); | ||
| const [isYearPickerVisible, setIsYearPickerVisible] = useState(false); | ||
| const [isMonthPickerVisible, setIsMonthPickerVisible] = useState(false); | ||
| const [fallbackContextID] = useState(() => `calendarPicker-${Math.random().toString(36).slice(2)}`); |
There was a problem hiding this comment.
Good call on the unused useState setter. I switched the fallback identifier to useId() rather than useRef: contextID is read during render, and the repo's React Compiler lint rules reject reading ref.current during render. useId() gives a stable per-instance id with no unused setter, so it's the cleaner fit here. (426a100)
| } | ||
| const {year} = selectedYearResult; | ||
| clearCalendarPickerSelectedYear(); | ||
| requestAnimationFrame(() => setCurrentDateView((prev) => setYear(new Date(prev), year))); |
There was a problem hiding this comment.
why we need requestAnimationFrame here?
There was a problem hiding this comment.
The effect reacts to the year that the year picker screen writes into Onyx. Calling setCurrentDateView synchronously in the effect body trips react-hooks/set-state-in-effect (the cascading-render rule), so it's deferred one frame. I added a comment in 426a100 making that explicit.
| onSelected={handleSingleDateSelected} | ||
| minDate={CONST.CALENDAR_PICKER.MIN_DATE} | ||
| maxDate={CONST.CALENDAR_PICKER.MAX_DATE} | ||
| pickerContextID="searchSingleDate" |
There was a problem hiding this comment.
I think we can just leave these to CalendarPicker auto-calculated values. What is the benefit of passing static values?
There was a problem hiding this comment.
fallbackContextID is regenerated each time a CalendarPicker mounts. This host's popover is dismissed when the year picker opens, so the CalendarPicker unmounts and remounts fresh when it's reopened — a regenerated fallback wouldn't match the contextID the year picker tagged its result with in Onyx, and the selection would be dropped. A stable static id keeps that round-trip intact (and the range picker needs two distinct ids so its calendars don't pick up each other's result).
There was a problem hiding this comment.
Then we should remove the fallback ID as it is prone to this bug. we should make sure the contextID is provided from parent.
There was a problem hiding this comment.
Done in cd94220 — pickerContextID is now required on CalendarPicker, the useId() fallback and the pickerContextID ?? fallbackContextID coalescence are gone, and useEffect / openYearPicker reference pickerContextID directly. All five product call sites (ScheduleCallPage, DatePresetFilterBase, RangeDatePicker × 2, DatePickerModal) were already passing stable ids, so this just tightens the contract. Added a small CalendarPickerForTest wrapper in tests/unit/CalendarPickerTest.tsx so existing tests that don't exercise the contextID still satisfy the required prop.
There was a problem hiding this comment.
Done in cd94220 — pickerContextID is now a required prop on CalendarPicker (no internal useId fallback), so every host has to provide a stable ID explicitly. The DatePresetFilterBase call site still passes pickerContextID="searchSingleDate" since that is now the explicit parent-provided ID.
| forceVerticalCalendars?: boolean; | ||
|
|
||
| /** Called right before navigating to the year picker screen (used by popover hosts to dismiss themselves) */ | ||
| onBeforeOpenYearPicker?: () => void; |
There was a problem hiding this comment.
Maybe instead of this, you can call Modal.closeTop from modal action file.
There was a problem hiding this comment.
Done in 426a100 — CalendarPicker now calls Modal.closeTop() directly instead of taking a close callback from each host. I gated it behind a boolean prop (shouldCloseModalOnYearPickerOpen): the wide-screen popover hosts set it, but the full-screen mobile DateSelectPopup overlay deliberately stays mounted while the year picker is open, so the dismissal needs to stay opt-in rather than fire unconditionally from CalendarPicker.
|
@parasharrajat I've added the iOS native recording to the Screenshots/Videos section. The flow shows opening Search → Date filter → Custom date and tapping the year label, which now navigates to the migrated Web/Android boxes were already ticked with their respective recordings; iOS row is now ticked too. |
| // previous in-place YearPickerModal had no screen restriction. Use '*' so the year selector | ||
| // remains reachable from every CalendarPicker host and doesn't silently break when new | ||
| // date-input screens are added (matches KEYBOARD_SHORTCUTS / EXIT_SURVEY_* generic flows). | ||
| entryScreens: ['*'], |
There was a problem hiding this comment.
Add a getRoute handler here to add queryParams.
There was a problem hiding this comment.
Done in cd94220 — added a getRoute on YEAR_SELECTOR that takes {contextID, currentYear, minYear, maxYear} and goes through getUrlWithParams('year-selector', …) so encoding stays in one place and matches the declared queryParams.
There was a problem hiding this comment.
Done in cd94220 — YEAR_SELECTOR now has a typed getRoute({contextID, currentYear, minYear, maxYear}) handler that builds the query params via getUrlWithParams.
| closeTop(); | ||
| } | ||
| Navigation.navigate( | ||
| createDynamicRoute(`${DYNAMIC_ROUTES.YEAR_SELECTOR.path}?contextID=${encodeURIComponent(contextID)}¤tYear=${currentYearView}&minYear=${minYear}&maxYear=${maxYear}`), |
There was a problem hiding this comment.
Use the getRoute handler here to create the route path
There was a problem hiding this comment.
Done in cd94220 — openYearPicker now calls DYNAMIC_ROUTES.YEAR_SELECTOR.getRoute({contextID: pickerContextID, currentYear: currentYearView, minYear, maxYear}) and passes that into createDynamicRoute. Existing CalendarPickerTest assertions (year-selector, contextID=…, currentYear=…) still pass.
There was a problem hiding this comment.
Done in cd94220 — this site now uses DYNAMIC_ROUTES.YEAR_SELECTOR.getRoute({contextID: pickerContextID, currentYear: currentYearView, minYear, maxYear}) instead of the manual string concatenation.
Make pickerContextID required so every CalendarPicker host owns a stable id; remove the useId-based fallback that regenerated on mount and could mis-route the year picker's selection back to a remounted instance. Add a getRoute handler on the YEAR_SELECTOR dynamic route and use it from CalendarPicker.openYearPicker so the path/queryParams stay co-located with the route definition.
Explanation of Change
YearPickerModalwas areact-native-modal(RIGHT_DOCKED) rendered insideCalendarPicker, driven by localisYearPickerVisible/currentDateViewstate. Becausereact-native-modalmounts and animates independently of the native stack, it animated inconsistently with the surrounding@react-navigationscreens (the problem described in the parent migration #53493).This migrates the year picker to a dynamic
@react-navigationmodal route, following the pattern established by PR #88915 (DynamicExpenseLimitTypeSelectorPage):DYNAMIC_ROUTES.YEAR_SELECTORand a standaloneDynamicYearSelectorPagescreen (the year list + search input, built from route query params), wired throughROUTES,SCREENS,ModalStackNavigators,linkingConfig, and navigationtypes.CalendarPicker's local state into a transient Onyx key (CALENDAR_PICKER_SELECTED_YEAR), tagged with a per-instancecontextID.CalendarPickeris rendered in multiple places, andRangeDatePickermounts two instances simultaneously, so the result is matched back to the instance that opened the picker viacontextIDand cleared once consumed.CalendarPickernow navigates to the year route (viacreateDynamicRoute) and applies the returned year to the displayed date. Hosts that renderCalendarPickerinside a popover (DatePickerModal, the Search date dropdown) passonBeforeOpenYearPickerso the popover is dismissed before the year screen is shown, instead of stacking a screen behind areact-native-modal.YearPickerModaland the now-unused year-list state inCalendarPicker.Fixed Issues
$ #90469
PROPOSAL: #90469 (comment)
Tests
Offline tests
The year selector is client-side navigation and writes only to a transient local Onyx value; behavior is identical offline.
QA Steps
Same as tests.
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
and_91106_trim.mp4
Android: mWeb Chrome
web_91106_mweb_20260521_040950.mp4
iOS: Native
ios_91106.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
web_91106_20260519_202020.mp4