Fix "Not here" page after saving split dates (#88434)#88472
Fix "Not here" page after saving split dates (#88434)#88472neil-marcellini wants to merge 3 commits into
Conversation
Remove the trailing `:backTo?` optional path parameter from the SPLIT_EXPENSE and SPLIT_EXPENSE_SEARCH route patterns. The optional path slot collided with the split tab names (amount, percentage, date) nested under those routes, so URLs like `create/split-expense/overview/<r>/<t>/<s>/date` were parsed with `backTo=date` instead of nesting the date tab. When `Navigation.goBack(backTo)` later resolved that URL, the resulting state was missing required params and SplitExpensePage rendered the "Not here" (FullPageNotFoundView) screen after saving split dates. `backTo` continues to travel as a query-string parameter via `getUrlWithBackToParam`, and React Navigation still populates `route.params.backTo` from the query string, so all consumer code keeps working unchanged. Adds tests/navigation/splitExpenseRouteShapeTests.ts to lock the fixed route shape in place. Made-with: Cursor
|
(Neil's AI agent) Some extra reviewer context on why this fix is narrow and how What
|
| RELATIONS | backTo |
|
|---|---|---|
| Kind | Static, declarative | Dynamic, per-navigation URL param |
| Lives in | linkingConfig/RELATIONS/*.ts |
The URL itself (?backTo=...) |
| When it wins | Only when backTo is absent |
Whenever present (unless the target is a dynamic-route screen) |
| Good at | Cold loads, deep links, refresh with no prior stack | Preserving "came from X" context when the same RHP is opened from multiple places |
| Weakness | One underlay per screen; can't vary by entry point | URL pollution, duplicated routes across the stack, round-trip parsing bugs (this issue) |
For Melvin's secondary suggestion — adding SPLIT_EXPENSE_CREATE_DATE_RANGE to the right RELATIONS map — the value is purely cold-load / refresh behavior: if you paste the split-dates URL directly into the address bar today, nothing in the maps points at Search or a split navigator, so you'd land on Inbox as the default underlay. Worth doing, but it's a separate bug from the "Not here" one, and it doesn't affect the goBack(backTo) flow that this PR is fixing.
There are also a few non-RHP maps in the same folder: SIDEBAR_TO_SPLIT / SPLIT_TO_SIDEBAR (which sidebar owns which Split navigator), TAB_TO_FULLSCREEN / FULLSCREEN_TO_TAB (which bottom tab each fullscreen navigator belongs to). Same pattern — a small declarative table replacing ad-hoc runtime logic.
Where the "remove backTo" initiative sits
The initiative is alive, well-designed, and partially complete — not finished.
Ratchet in place:
- Design doc: [Design Doc] Replacing backTo / forwardTo with Dynamic URL Navigation #73825 "Replacing backTo / forwardTo with Dynamic URL Navigation" — executed against.
- Dynamic Routes as the replacement mechanism: 54 entries in
DYNAMIC_ROUTESinsrc/ROUTES.tstoday. - ESLint blocks new
getUrlWithBackToParamcalls:selector: 'CallExpression[callee.name="getUrlWithBackToParam"]', message: 'Usage of getUrlWithBackToParam function is prohibited. This is legacy code and no new occurrences should be added...' - ESLint blocks new
backToproperties on screen param lists:selector: 'TSPropertySignature[key.name="backTo"]', message: 'The `backTo` route param is deprecated. Do not add new `backTo` properties to screen param lists...' contributingGuides/NAVIGATION.md— everybackTosection now carries a[!WARNING] Deprecatedbanner pointing first at Dynamic Routes, with a full "Migrating from backTo to dynamic routes" guide.- Tracking issues: [Tracking] Replacing backTo / forwardTo with Dynamic URL Navigation - Release 1 #80515 "Release 1" (OPEN), [Tracking] Replacing backTo / forwardTo with Dynamic URL Navigation - Release 2 #83358 "Release 2" (OPEN). Bodies are placeholders; the actual unit of work is tracked as
BT-XXX-labeled sub-issues / PRs.
What hasn't happened yet:
- ~1,962
backToreferences remain acrosssrc/(including 462 insidetypes.ts, 374 inROUTES.ts— i.e. ~1,100 real call sites across pages). - 221 screen param lists still declare
backTo?:tagged with the "legacy" eslint-disable marker. - 227 other
eslint-disable-next-linecomments grandfathering legacygetUrlWithBackToParamcalls. - 226
getRoute(…, backTo?)signatures still live insrc/ROUTES.ts.
Migration PRs are landing continuously under the BT-… label — just the last couple of weeks have seen QBO Export, NetSuite Parts 1–2, QBO AutoSync, Exit Survey, Categories / Tags, Keyboard Shortcuts, etc.
Why this PR is the narrow fix
Doing the "full" fix for this flow — migrating SPLIT_EXPENSE_CREATE_DATE_RANGE off backTo to a Dynamic Route, adding the appropriate RELATIONS entry, and reworking handleDatePress to stop relying on Navigation.getActiveRoute() — would bring new surface area (and new BT--style migration risk) into a deploy-blocker patch.
The route-shape fix is aligned with the end state (Dynamic Routes don't carry :backTo? in the path) and with the current state (?backTo= query strings continue to work for every grandfathered callsite), so it's a net step toward the initiative without jumping ahead of it. Longer-term cleanup (Dynamic Route migration for this flow, filling in the RELATIONS entry, and replacing the getActiveRoute() → backTo call in handleDatePress) is better done as a follow-up BT- PR rather than piled onto the deploy fix.
Addresses the "Remove backTo from route" TODO that accompanied the :backTo? path-param removal from SPLIT_EXPENSE / SPLIT_EXPENSE_SEARCH. The second half of the #88434 regression: SplitExpensePage.handleDatePress forwarded Navigation.getActiveRoute() straight into backTo. On the Date tab that URL ended in /date, which collides with the nested tab segment once goBack resolves it. We now sanitize that URL through a pure helper, stripSplitTabSuffix, before handing it to the date-range screen as backTo. The selected tab is still restored across the back navigation because OnyxTabNavigator persists it in Onyx. Also locks in both layers of the fix with unit tests. Made-with: Cursor
Renames SplitExpenseCreateDateRagePage to SplitExpenseCreateDateRangePage and updates the matching type, component, test ID, and the require() path in ModalStackNavigators. Made-with: Cursor
|
I'm closing this because we ended up finding the problematic PR and reverting, but this might be a good template for someone to look at for a long-term fix. |
Explanation of Change
(Neil's AI agent)
The
SPLIT_EXPENSEandSPLIT_EXPENSE_SEARCHroutes declared an optional:backTo?path parameter at the end of the path:That trailing slot collided with the nested split tab screens (
amount,percentage,date) registered under those routes insrc/libs/Navigation/linkingConfig/config.ts. Because of the collision, a URL like:could be parsed with
backTo = "date"instead of nesting thedatetab under the parent screen.The regression surfaced in the split-by-date flow:
SplitExpensePage. Current URL ends in/date.handleDatePressnavigates toSPLIT_EXPENSE_CREATE_DATE_RANGEand usesNavigation.getActiveRoute()asbackTo, sobackTois the.../0/dateURL.Navigation.goBack(backTo)runsgetStateFromPath(backTo)to unwind.SplitExpensePagerenderedFullPageNotFoundView("Not here").(Neil's AI agent)
The fix has two complementary layers:
Layer 1 — Remove
:backTo?from the route patterns.getUrlWithBackToParamalready appendsbackToas a query string (?backTo=...); it never fills the:backTo?path slot itself. Dropping the slot from the pattern removes the ambiguity while keepingroute.params.backTopopulated (React Navigation parses query strings into params automatically), so every consumer that readsroute.params.backTo(SplitExpensePage,SplitExpenseCreateDateRagePage,SplitExpenseEditPage) keeps working unchanged.The pattern was originally introduced in #78027 (fixing #78080, "Back button on split RHP show the page twice") as a workaround for stack duplication when navigating Split > Date > Split dates. That PR even added an inline TODO: "TODO: Remove backTo from route once we have find another way to fix navigation issues with tabs." This change completes that TODO.
Layer 2 — Strip trailing tab segments from the
backToURL fed into the date-range screen.SplitExpensePage.handleDatePresspreviously forwardedNavigation.getActiveRoute()straight intobackTo. On the Date tab that URL ends in/date, which is exactly the ambiguous segment that caused "Not here." Even with the route pattern fixed, re-entering that URL intogetStateFromPathis unnecessarily brittle (and still collides with the nesteddatetab route). The new pure helpersrc/libs/stripSplitTabSuffix.tsremoves a trailing/amount,/percentage, or/datesegment from the URL before it's passed asbackTo. The selected tab is still restored across the back navigation becauseOnyxTabNavigatorpersists the active tab in Onyx.(Neil's AI agent)
Notes on scope:
RELATIONSorgetSearchScreenNameForRoutefor the split-expense RHP screens.getMatchingFullScreenRoutealready resolves the underlying fullscreen state frombackTofirst (now unambiguous) and only falls back toRELATIONS/ defaults whenbackTois absent. Adding these screens toSEARCH_TO_RHPwould incorrectly default non-search deep-links to the Search tab; the default fallback to Inbox is the documented behavior and is acceptable for deep-link entry (percontributingGuides/NAVIGATION.md).:backTo?patterns on unrelated search routes — they have a different shape and no tab collision.(Neil's AI agent) Added
tests/navigation/splitExpenseRouteShapeTests.tswhich locks in both layers of the fix:ROUTES.SPLIT_EXPENSE/ROUTES.SPLIT_EXPENSE_SEARCHand theirgetRoutehelpers.stripSplitTabSuffixfor everyCONST.TAB.SPLITvalue, including query-string preservation and mid-path false positives.TDD round trip was verified end-to-end: with both layers of the fix reverted, the 4 route-shape assertions fail; reapplying both brings all 15 tests green.
Fixed Issues
$ #88434
PROPOSAL: N/A (deploy blocker)
Tests
(Neil's AI agent)
Offline tests
Same as Tests — offline/online transitions are not part of this code path. Verify the navigation still works with no network after the initial page load.
QA Steps
(Neil's AI agent) Same as Tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Made with Cursor