Fix not here page after moving IOU report to workspace#87167
Fix not here page after moving IOU report to workspace#87167
Conversation
When moving an IOU report to a workspace, the REPORT_PREVIEW action is
completely removed from Onyx (set to null). The existing cleanup effect
only handled the case where the action existed but was hidden/deleted
(isLinkedActionDeleted). When the action was nulled out entirely,
linkedAction became undefined but isLinkedActionDeleted stayed false
(since it requires linkedAction to be truthy), leaving the stale
reportActionIDFromRoute in the URL and showing a "not here" page.
Expand the effect to also clear the route param when linkedAction is
null (completely removed) while wasEverVisible is true, after loading
has completed.
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
…-onyx Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Fixed the failing typecheck: Note: This is a pre-existing issue on |
| if (!isActionGone) { | ||
| return; | ||
| } | ||
| Navigation.setParams({reportActionID: ''}); |
There was a problem hiding this comment.
@MelvinBot This is no-op because report action was deleted while in modal which doesn't have reportActionID in navigation route at all.
There was a problem hiding this comment.
Good catch! Navigation.setParams targets the focused route by default, so when a modal is open it's a no-op since the modal route has no reportActionID.
Fixed by passing route.key to all three setParams calls so they always target the report screen's route regardless of which screen is focused.
Pass route.key to Navigation.setParams so it targets the report screen's route instead of the currently focused route. Without this, setParams is a no-op when a modal is focused because the modal route has no reportActionID param. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
| if (!isActionGone) { | ||
| return; | ||
| } | ||
| Navigation.setParams({reportActionID: ''}, route.key); |
There was a problem hiding this comment.
@MelvinBot this still doesn't work
The action 'SET_PARAMS' with payload {"params":{"reportActionID":""}} was not handled by any navigator.
There was a problem hiding this comment.
The issue was that SET_PARAMS only searches the flat routes array of each navigator. When the report screen route is nested inside a split navigator and a modal is focused, the action never reaches the correct navigator.
Fixed by adding a findParentNavigatorKey helper in Navigation.ts that walks the navigation state tree to find which navigator directly contains the route, then passes its key as target in the dispatch. This ensures the action is delivered to the right navigator regardless of what's currently focused.
SET_PARAMS only searches the flat routes array of each navigator. When a route is nested inside a split navigator and a modal is focused, the action never reaches the correct navigator. Add findParentNavigatorKey helper that walks the navigation state tree to find which navigator directly contains the route, then pass its key as `target` so the action is dispatched to the right place. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot fix conflict and update Tests steps as per #85753 (comment) |
Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Merged main to resolve the conflict in |
src/libs/Navigation/Navigation.ts
Outdated
| * This is needed because SET_PARAMS only searches flat routes arrays, so dispatching | ||
| * without `target` fails for routes nested inside split navigators. | ||
| */ | ||
| function findParentNavigatorKey(routeKey: string, state?: NavigationState): string | undefined { |
|
@MelvinBot the bug is still reproducible on mobile Screen.Recording.2026-04-06.at.7.09.09.pm.mov |
- Test findParentNavigatorKey: direct children, nested routes, deep nesting, sibling navigators, not-found cases - Test LinkedActionNotFoundGuard: action removal clears route param with route.key, no navigation when never visible, no navigation while loading, bypass when no reportActionID - Export findParentNavigatorKey for testability Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Added unit tests in 5179468:
|
Gate shouldShowNotFoundLinkedAction on !wasEverVisible so the not-found page is never shown when an action was previously visible and then removed from Onyx. On mobile (NativeStackView), the synchronous render commits the not-found UI before the cleanup useEffect can fire setParams to clear the route param. By preventing the not-found page from showing in this case, the effect handles navigation cleanly on all platforms. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Pushed a fix in c0db3f6. Root cause: When the linked action is removed from Onyx, Fix: Gate |
|
@MelvinBot author checklist is failing. And pull main |
|
@MelvinBot the root cause of this bug is that Example: https://dev.new.expensify.com:8082/r/2247619693499951/1461598516100574753 |
…AfterMovingIOUToWorkspace Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Done! Two things fixed:
|
… is removed When a linked report action is removed (e.g. REPORT_PREVIEW nulled after moving an IOU to a workspace), the current route's reportActionID is cleared but sibling screens still carry the old reportActionID in their backTo param. Pressing back on those screens navigates to the stale deep-link, causing a "not here" page. Add Navigation.cleanStaleBackToParam() to walk the navigation state tree and strip the stale reportActionID from any backTo params referencing the removed action. Called from LinkedActionNotFoundGuard's cleanup effect. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Good catch — the Fix: Added |
| * reportActionID. Pressing back on those screens would navigate to the stale | ||
| * deep-link, causing a "not here" page. This function patches those params. | ||
| */ | ||
| function cleanStaleBackToParam(reportID: string, reportActionID: string) { |
|
Unit tests are already included in this PR:
All test jobs (1-8) are passing in CI. Is there a specific scenario you'd like additional test coverage for? |
|
@MelvinBot add unit test for Author checklist is still failing |
…ard test Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
melvin-bot
bot
requested review from
aimane-chnaif and
joekaufmanexpensify
and removed request for
a team,
aimane-chnaif and
joekaufmanexpensify
|
@aimane-chnaif 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] |
melvin-bot
bot
requested review from
aimane-chnaif
and removed request for
a team
|
Done! PR has been marked as ready for review. I also cleaned up the duplicate checklist items from the PR body. |
Code ReviewOverall: Looks good. The approach is sound — detecting when a linked action disappears from Onyx (not just deleted/hidden), clearing the stale Minor Items
What Looks Good
|
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
@aimane-chnaif can you look into fixing the author checklist? |
I think there's bug in automation. 
|
|
MelvinBot has made 15 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Feels like this is getting quite complicated but I did not see anything specifically wrong here. I would like to get @WojtekBoman review here if possible before going ahead
|
Reviewing it 🕵️ |
src/libs/Navigation/Navigation.ts
Outdated
|
|
||
| /** | ||
| * Find the navigator state key that directly contains a route with the given key. | ||
| * This is needed because SET_PARAMS only searches flat routes arrays, so dispatching | ||
| * without `target` fails for routes nested inside split navigators. | ||
| */ | ||
| function findParentNavigatorKey(routeKey: string, state?: NavigationState): string | undefined { | ||
| const navState = state ?? navigationRef.getRootState(); | ||
| if (!navState?.routes) { | ||
| return undefined; | ||
| } | ||
| for (const route of navState.routes) { | ||
| if (route.key === routeKey) { | ||
| return navState.key; | ||
| } | ||
| if (route.state?.routes) { | ||
| const found = findParentNavigatorKey(routeKey, route.state as NavigationState); | ||
| if (found) { | ||
| return found; | ||
| } | ||
| } | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
To me, we can fix this issue without adding this method, let's pass targetKey as a param to Navigation.setParams, it can be read from navigation.getState()?.key in ReportScreen
There was a problem hiding this comment.
To me, we can fix this issue without adding this method, let's pass targetKey as a param to
Navigation.setParams, it can be read fromnavigation.getState()?.keyin ReportScreen
@MelvinBot let's try that
There was a problem hiding this comment.
Done! Removed findParentNavigatorKey entirely and added an optional targetKey parameter to Navigation.setParams instead. In LinkedActionNotFoundGuard, the navigator key is now read via useNavigation().getState()?.key and passed directly. Also updated cleanStaleBackToParam to track navigator keys during its walk and pass them through. All 146 navigation tests pass.
There was a problem hiding this comment.
And it's possible that the same report screen is multiply stacked in the navigation. (on mobile)
If reportActionID was removed while in other navigation route, will this remove all reportActionIDs in multiple report screens?
There was a problem hiding this comment.
@aimane-chnaif If it's called only from ReportScreen it should return the key of the reports screen parent route. I believe it should work, but if the bug is still reproducible with reading it this way, we can add this method findParentNavigatorKey.
However I would suggest to:
- add it to navigation helpers package and not export it from Navigation.ts
- don't call this method directly from setParams, if we want to use this method, let's call it before
setParamsand passtargetKeytosetParams.
Additionally, please let's cleanup Navigation.cleanStaleBackToParam. I think we should keep it close to ReportScreen, remove it from Navigation.ts and definitely rename it as this method is mainly used for report params and function name does not indicate it's a method specific for report params.
src/libs/Navigation/Navigation.ts
Outdated
| // When a routeKey is provided, find its parent navigator and set `target` so the | ||
| // action is delivered directly to that navigator. Without this, SET_PARAMS fails | ||
| // for routes nested inside split navigators (e.g. when a modal is focused). | ||
| const navigatorKey = routeKey ? findParentNavigatorKey(routeKey) : undefined; |
There was a problem hiding this comment.
Don't read it using an additional function, just add a new optional param to this method called targetKey
There was a problem hiding this comment.
Done in the latest commit — setParams now accepts an optional targetKey as its third parameter.
Instead of auto-discovering the parent navigator by walking the state tree, accept an optional targetKey parameter on Navigation.setParams. Callers read the navigator key via navigation.getState()?.key and pass it directly. This is simpler and avoids the tree-walk helper entirely. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
@aimane-chnaif 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] |
| // Also strip the stale reportActionID from any `backTo` params on sibling routes | ||
| // (e.g. the IOU report screen that was navigated to FROM this deep link). | ||
| if (reportIDFromRoute) { | ||
| Navigation.cleanStaleBackToParam(reportIDFromRoute, reportActionIDFromRoute); |
There was a problem hiding this comment.
I think we know routes which should be cleaned, so maybe let's just find this specific route and clean it's backTo, instead of going through the whole state
|
@aimane-chnaif did you test if its working? |
4 participants
Explanation of Change
When moving an IOU report to a workspace, the
REPORT_PREVIEWaction is completely removed from Onyx (set tonullviaOnyx.METHOD.MERGE). The existing cleanup effect inLinkedActionNotFoundGuardonly handled the case where the linked action existed but was hidden/deleted (isLinkedActionDeleted). When the action was removed entirely,linkedActionbecameundefinedbutisLinkedActionDeletedstayedfalse(since it requireslinkedActionto be truthy), leaving the stalereportActionIDFromRoutein the URL and causing a "not here" page.This PR expands the cleanup effect to also clear the route param when
linkedActionisnull(completely removed from Onyx) whilewasEverVisibleistrueand loading has completed.Fixed Issues
$ #85753
PROPOSAL: #85753 (comment)
Tests
Precondition:
Offline tests
N/A - This bug only occurs when an API call (changing workspace) removes the action from Onyx. The navigation fix is purely client-side route param cleanup.
QA Steps
Same as tests above - reproduce on staging where the deep link feature is enabled.
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./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari