Skip to content

Skip prepending TabNavigator for public screens in navigation state #89295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
luacmartins merged 2 commits into from
Apr 30, 2026
Merged

Skip prepending TabNavigator for public screens in navigation state #89295

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15020f6
Skip prepending TabNavigator for public screens in getAdaptedState
MelvinBot Apr 30, 2026
5b9d535
Add sync note to PUBLIC_SCREENS comment pointing to PublicScreens.tsx
MelvinBot Apr 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/libs/Navigation/helpers/getAdaptedStateFromPath.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,23 @@ const getRoutesWithIndex = (routes: NavigationPartialRoute[]): PartialState<Navi
/** All tab routes derived from the shared TAB_SCREENS constant. */
const TAB_NAVIGATOR_ROUTES: NavigationPartialRoute[] = TAB_SCREENS.map((name) => ({name}));

/**
* Screens that are registered in PublicScreens (unauthenticated navigator) and should not
* have TabNavigator prepended, because when the user is unauthenticated TabNavigator does
* not exist in the navigator tree and the RESET action would fail.
*
* Keep in sync with the screens registered in PublicScreens.tsx (excluding SCREENS.HOME,
* which doubles as the authenticated home tab, and navigator entries).
*/
const PUBLIC_SCREENS = new Set<string>([

@github-actions github-actions Bot Apr 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ CONSISTENCY-3 (docs)

The PUBLIC_SCREENS set is a hardcoded list that duplicates information already defined elsewhere. The exact same six screens are listed in the PublicScreenName type union in src/libs/Navigation/types.ts (line 3266-3272), and the comment itself acknowledges this by saying "Keep in sync with the screens registered in PublicScreens.tsx" -- a manual sync requirement is a strong indicator of a DRY violation.

Consider creating a single source-of-truth runtime array (e.g., const PUBLIC_SCREEN_NAMES = [SCREENS.VALIDATE_LOGIN, SCREENS.TRANSITION_BETWEEN_APPS, SCREENS.CONNECTION_COMPLETE, SCREENS.BANK_CONNECTION_COMPLETE, SCREENS.UNLINK_LOGIN, SCREENS.SAML_SIGN_IN] as const) in a shared location, then deriving both the PublicScreenName type (via typeof PUBLIC_SCREEN_NAMES[number]) and this Set from it. This eliminates the risk of the two lists drifting out of sync.

Reviewed at: 5b9d535 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

SCREENS.VALIDATE_LOGIN,

@github-actions github-actions Bot Apr 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ CONSISTENCY-3 (docs)

The PUBLIC_SCREENS set duplicates the list of public screens already registered in PublicScreens.tsx and partially reflected in PUBLIC_SCREENS_ROUTES from src/ROUTES.ts. The comment even acknowledges this: "Keep in sync with the screens registered in PublicScreens.tsx" -- a manual sync requirement is a strong signal of duplication that will drift over time.

Consider extracting a shared constant (e.g., an array of public screen names) that both PublicScreens.tsx and this file can import, or derive the set from PUBLIC_SCREENS_ROUTES keys mapped to their corresponding SCREENS constants. This eliminates the manual synchronization burden and prevents the two lists from diverging.

Reviewed at: 5b9d535 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

@situchan situchan Apr 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree but let's do this as follow-up. Since this PR will be CP'ed to staging, the code should be minimal.

SCREENS.TRANSITION_BETWEEN_APPS,
SCREENS.CONNECTION_COMPLETE,
SCREENS.BANK_CONNECTION_COMPLETE,
SCREENS.UNLINK_LOGIN,
SCREENS.SAML_SIGN_IN,
]);

/**
* Builds TabNavigator state with all tabs and the correct selected tab.
* Tab navigators require all routes in the state for proper rendering.
Expand Down Expand Up @@ -355,6 +372,14 @@ function getAdaptedState(state: PartialState<NavigationState<RootNavigatorParamL
return getRoutesWithIndex([getTabNavigatorState({name: NAVIGATORS.REPORTS_SPLIT_NAVIGATOR}), ...currentState.routes]);
}

// Public screens (e.g. ValidateLogin) exist in both PublicScreens and AuthScreens navigators.
// Don't prepend TabNavigator because when the user is unauthenticated, PublicScreens is active
// and TabNavigator doesn't exist — causing the RESET action to fail.
const hasOnlyPublicScreens = currentState.routes.every((route) => PUBLIC_SCREENS.has(route.name));
if (hasOnlyPublicScreens) {
return currentState;
Comment on lines +378 to +380

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve fallback route for authenticated public deeplinks

This unconditional early return for hasOnlyPublicScreens removes the previously injected TAB_NAVIGATOR even when the user is already authenticated, so /v/:accountID/:validateCode can initialize with only ValidateLogin in the root stack. In that state, ValidateLoginPage calls Navigation.goBack() for signed-in users (src/pages/ValidateLoginPage/index.tsx), but there is no back route, so the action is ignored and users can remain stuck on the loading interstitial instead of returning to the app.

Useful? React with 👍 / 👎.

@situchan situchan Apr 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasOnlyPublicScreens check guards this. There's no case of user being authenticated.

}

const defaultFullScreenRoute = getDefaultFullScreenRoute(focusedRoute);

// If not, add the default full screen route.
Expand Down
Loading