Wait for TabNavigator mount before resolving waitForProtectedRoutes#91796
Open
MelvinBot wants to merge 3 commits into
Open
Wait for TabNavigator mount before resolving waitForProtectedRoutes#91796MelvinBot wants to merge 3 commits into
MelvinBot wants to merge 3 commits into
Conversation
navContainsProtectedRoutes previously only checked state.routeNames for CONCIERGE, but routeNames reflects declared screens, not mounted ones. TabNavigator appears in routeNames simultaneously with Concierge, so waitForProtectedRoutes resolved before TabNavigator's child router was registered. Deep link navigation dispatched during this window produced an unhandled NAVIGATE action error. Now additionally verify that the TabNavigator route exists in state.routes with stale === false (set by useNavigationBuilder after mount), ensuring the navigator can handle nested navigation actions. Co-authored-by: daledah <daledah@users.noreply.github.com>
7 tasks
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.
|
Contributor
|
@MelvinBot update your checklist:
|
Contributor
Author
|
Updated the checklist as requested:
|
Contributor
|
@MelvinBot please update step 5 in your checklist to |
Contributor
Author
|
Updated QA step 5 to: |
Contributor
|
@MelvinBot mark as ready |
Contributor
Author
|
PR marked as ready for review. |
melvin-bot
Bot
requested review from
JmillsExpensify
and removed request for
a team
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeN/A iOS: HybridAppios-resize.mp4iOS: mWeb SafariN/A MacOS: Chrome / Safariweb-resize.mp4 |
Contributor
|
@MelvinBot Your fix only covers the |
…gator mounts The subscribe listener forwards URLs directly to React Navigation via listener(url). When TabNavigator exists in routes but hasn't finished mounting (stale !== false), this dispatch hits an unregistered child router and throws an unhandled NAVIGATE action error. Skip forwarding in this window — deep links for protected screens are already handled by openReportFromDeepLink via waitForProtectedRoutes(). Co-authored-by: daledah <daledah@users.noreply.github.com>
Contributor
Author
|
Added the |
|
@yuwenmemon 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // error. Protected-screen deep links will be handled separately by | ||
| // openReportFromDeepLink via waitForProtectedRoutes(). | ||
| const state = navigationRef.current?.getRootState(); | ||
| const tabRoute = state?.routes?.find((route) => route.name === NAVIGATORS.TAB_NAVIGATOR); |
Contributor
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The TabNavigator-is-ready check (find the tab route, verify state?.stale === false) is duplicated between this file (lines 41-42) and navContainsProtectedRoutes in Navigation.ts (lines 710-711). If the readiness criteria change (e.g., checking additional navigators), both sites must be updated in lockstep.
Consider extracting a shared helper like isTabNavigatorReady(state) and calling it from both locations:
// In a shared location (e.g., Navigation.ts or a navigation utils file)
function isTabNavigatorReady(state: State | undefined): boolean {
const tabRoute = state?.routes?.find((route) => route.name === NAVIGATORS.TAB_NAVIGATOR);
return tabRoute?.state?.stale === false;
}Then in subscribe.ts:
if (!isTabNavigatorReady(state)) {
return;
}And in navContainsProtectedRoutes:
return isTabNavigatorReady(state);Reviewed at: 57796ec | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Contributor
Author
There was a problem hiding this comment.
Done — extracted isTabNavigatorReady and updated both call sites to use it.
The TabNavigator readiness check (find route, verify stale === false) was duplicated between subscribe.ts and navContainsProtectedRoutes in Navigation.ts. Extract into a shared helper so the criteria only need to be updated in one place. Co-authored-by: daledah <daledah@users.noreply.github.com>
yuwenmemon
approved these changes
May 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
During the deep link login flow,
waitForProtectedRoutes()resolves as soon asCONCIERGEappears instate.routeNames. However,routeNamesreflects declared screen names — not mounted navigators. SinceTabNavigatoris loaded lazily viagetComponent, there's a window whererouteNamesincludes it but its child router hasn't runuseNavigationBuilderyet. Navigation dispatched during this window produces an unhandledNAVIGATEaction error.This PR adds an additional check in
navContainsProtectedRoutesto verify that theTabNavigatorroute's nested state hasstale === false, which only becomes true after its child router mounts and callsuseNavigationBuilder. This ensureswaitForProtectedRoutes()doesn't resolve untilTabNavigatorcan actually handle nested navigation actions.Fixed Issues
$ #91777
PROPOSAL: #91777 (comment)
Tests
Same as QA Tests.
Offline tests
Same as QA Tests.
QA Steps
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