-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix workspace navigation after distance settings deep link #98423
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
33caa8e
f907579
0382bb3
6533c3d
c48a179
4d6fa6b
de7a6f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,7 +134,21 @@ function shouldChangeToMatchingFullScreen( | |
| return newFocusedRoute?.name === SCREENS.SETTINGS.SUBSCRIPTION.ADD_PAYMENT_CARD && lastActiveScreen !== SCREENS.SETTINGS.SUBSCRIPTION.ROOT; | ||
| } | ||
|
|
||
| export {isSwitchingTabsWithinTabNavigator, getActiveScreenInRoute, shouldChangeToMatchingFullScreen, isNavigatingToReportActionWithinSameReport}; | ||
| function getMatchingFullScreenRouteParams( | ||
| matchingFullScreenRoute: NavigationPartialRoute, | ||
| ): NavigationPartialRoute['params'] | {screen: string; params: NavigationPartialRoute['params'] | undefined} { | ||
| const lastRoute = matchingFullScreenRoute.state?.routes?.at(-1); | ||
| if (!lastRoute) { | ||
| return matchingFullScreenRoute.params; | ||
| } | ||
|
|
||
| return { | ||
| screen: lastRoute.name, | ||
| params: lastRoute.state ? {...lastRoute.params, state: lastRoute.state} : lastRoute.params, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This now preserves nested
|
||
| }; | ||
| } | ||
|
|
||
| export {isSwitchingTabsWithinTabNavigator, getActiveScreenInRoute, getMatchingFullScreenRouteParams, shouldChangeToMatchingFullScreen, isNavigatingToReportActionWithinSameReport}; | ||
|
|
||
| export default function linkTo(navigation: NavigationContainerRef<RootNavigatorParamList> | null, path: Route, options?: LinkToOptions) { | ||
| if (!navigation) { | ||
|
|
@@ -254,14 +268,11 @@ export default function linkTo(navigation: NavigationContainerRef<RootNavigatorP | |
| navigation.dispatch(additionalAction); | ||
| } else { | ||
| // Navigate within the existing TAB_NAVIGATOR (tab switch) rather than pushing a new one. | ||
| const lastRouteInMatchingFullScreen = matchingFullScreenRoute.state?.routes?.at(-1); | ||
| const additionalAction = CommonActions.navigate({ | ||
| name: NAVIGATORS.TAB_NAVIGATOR, | ||
| params: { | ||
| screen: matchingFullScreenRoute.name, | ||
| params: lastRouteInMatchingFullScreen | ||
| ? {screen: lastRouteInMatchingFullScreen.name, params: lastRouteInMatchingFullScreen.params} | ||
| : matchingFullScreenRoute.params, | ||
| params: getMatchingFullScreenRouteParams(matchingFullScreenRoute), | ||
| }, | ||
| }); | ||
| navigation.dispatch(additionalAction); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| import {render, waitFor} from '@testing-library/react-native'; | ||
|
|
||
| import {openPolicyInitialPage} from '@libs/actions/Policy/Policy'; | ||
| import Navigation from '@libs/Navigation/Navigation'; | ||
|
|
||
| import {WorkspaceInitialPage} from '@pages/workspace/WorkspaceInitialPage'; | ||
|
|
||
| import type * as ReactNavigation from '@react-navigation/native'; | ||
|
|
||
| import React from 'react'; | ||
|
|
||
| import createMock from '../utils/createMock'; | ||
|
|
||
| const mockFullPageNotFoundView = jest.fn(({children}: {children: React.ReactNode}) => children); | ||
| let mockActiveRoute: string | undefined; | ||
| let mockIsWorkspacesTabFocused = true; | ||
|
|
||
| jest.mock('@libs/actions/Policy/Policy', () => ({ | ||
| clearErrors: jest.fn(), | ||
| openPolicyInitialPage: jest.fn(), | ||
| removeWorkspace: jest.fn(), | ||
| })); | ||
|
|
||
| jest.mock('@libs/Navigation/Navigation', () => ({ | ||
| closeRHPFlow: jest.fn(), | ||
| isNavigationReady: () => Promise.resolve(), | ||
| })); | ||
|
|
||
| jest.mock('@react-navigation/native', () => { | ||
| const navigation = jest.requireActual<typeof ReactNavigation>('@react-navigation/native'); | ||
| return { | ||
| ...navigation, | ||
| useFocusEffect: (callback: () => void) => callback(), | ||
| useIsFocused: () => true, | ||
| useNavigationState: () => mockActiveRoute, | ||
| }; | ||
| }); | ||
|
|
||
| jest.mock('@hooks/useCardFeedErrors', () => () => ({shouldShowRbrForWorkspaceAccountID: {}})); | ||
| jest.mock('@hooks/useCurrencyList', () => ({useCurrencyListActions: () => ({convertToDisplayString: jest.fn()})})); | ||
| jest.mock('@hooks/useGetReceiptPartnersIntegrationData', () => () => ({shouldShowEnterCredentialsError: false})); | ||
| jest.mock('@hooks/useIsWorkspacesTabFocused', () => () => mockIsWorkspacesTabFocused); | ||
| jest.mock('@hooks/useLazyAsset', () => ({ | ||
| useMemoizedLazyExpensifyIcons: () => new Proxy({}, {get: () => 'icon'}), | ||
| })); | ||
| jest.mock('@hooks/useLocalize', () => () => ({translate: (key: string) => key})); | ||
| jest.mock('@hooks/useNetwork', () => jest.fn()); | ||
| jest.mock('@hooks/useOnyx', () => () => [undefined]); | ||
| jest.mock('@hooks/usePermissions', () => () => ({isBetaEnabled: () => false})); | ||
| jest.mock('@hooks/usePolicyConnectionsPrefetch', () => jest.fn()); | ||
| jest.mock('@hooks/useResponsiveLayout', () => () => ({shouldUseNarrowLayout: true})); | ||
| jest.mock('@hooks/useSingleExecution', () => () => ({singleExecution: (callback: () => void) => callback, isExecuting: false})); | ||
| jest.mock('@hooks/useThemeStyles', () => () => ({ | ||
| flexColumn: {}, | ||
| mh3: {}, | ||
| mt3: {}, | ||
| overflowVisible: {}, | ||
| pb4: {}, | ||
| pb14: {}, | ||
| ph5: {}, | ||
| pv2: {}, | ||
| sectionMenuItem: () => ({}), | ||
| })); | ||
| jest.mock('@hooks/useWaitForNavigation', () => () => (callback: () => void) => callback); | ||
| jest.mock('@hooks/useWorkspaceAccountID', () => jest.fn()); | ||
|
|
||
| jest.mock('@components/BlockingViews/FullPageNotFoundView', () => (props: {children: React.ReactNode; shouldShow: boolean}) => mockFullPageNotFoundView(props)); | ||
| jest.mock('@components/HeaderWithBackButton', () => jest.fn()); | ||
| jest.mock('@components/HighlightableMenuItem', () => jest.fn()); | ||
| jest.mock('@components/Navigation/TabBarBottomContent', () => jest.fn()); | ||
| jest.mock( | ||
| '@components/OfflineWithFeedback', | ||
| () => | ||
| ({children}: {children: React.ReactNode}) => | ||
| children, | ||
| ); | ||
| jest.mock( | ||
| '@components/ScreenWrapper', | ||
| () => | ||
| ({children}: {children: React.ReactNode}) => | ||
| children, | ||
| ); | ||
| jest.mock( | ||
| '@components/ScrollView', | ||
| () => | ||
| ({children}: {children: React.ReactNode}) => | ||
| children, | ||
| ); | ||
|
|
||
| describe('WorkspaceInitialPage', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| mockActiveRoute = undefined; | ||
| mockIsWorkspacesTabFocused = true; | ||
| }); | ||
|
|
||
| it('waits for route params before fetching policy data, showing Not Found, or closing the RHP', async () => { | ||
| const props = createMock<React.ComponentProps<typeof WorkspaceInitialPage>>({route: {params: undefined}}); | ||
| render(<WorkspaceInitialPage {...props} />); | ||
|
|
||
| expect(openPolicyInitialPage).not.toHaveBeenCalled(); | ||
| expect(mockFullPageNotFoundView).toHaveBeenCalledWith(expect.objectContaining({shouldShow: false})); | ||
| await waitFor(() => expect(Navigation.closeRHPFlow).not.toHaveBeenCalled()); | ||
| }); | ||
|
|
||
| it('evaluates closing the RHP independently for each policy', async () => { | ||
| mockActiveRoute = 'inaccessible-workspace-route'; | ||
| const firstProps = createMock<React.ComponentProps<typeof WorkspaceInitialPage>>({route: {params: {policyID: 'policy-1'}}}); | ||
| const {rerender} = render(<WorkspaceInitialPage {...firstProps} />); | ||
| await waitFor(() => expect(Navigation.closeRHPFlow).toHaveBeenCalledTimes(1)); | ||
|
|
||
| const secondProps = createMock<React.ComponentProps<typeof WorkspaceInitialPage>>({route: {params: {policyID: 'policy-2'}}}); | ||
| rerender(<WorkspaceInitialPage {...secondProps} />); | ||
| await waitFor(() => expect(Navigation.closeRHPFlow).toHaveBeenCalledTimes(2)); | ||
| }); | ||
|
|
||
| it('does not retain Not Found after the route policy changes', () => { | ||
| const firstProps = createMock<React.ComponentProps<typeof WorkspaceInitialPage>>({route: {params: {policyID: 'policy-1'}}}); | ||
| const {rerender} = render(<WorkspaceInitialPage {...firstProps} />); | ||
| expect(mockFullPageNotFoundView).toHaveBeenLastCalledWith(expect.objectContaining({shouldShow: true})); | ||
|
|
||
| mockIsWorkspacesTabFocused = false; | ||
| const secondProps = createMock<React.ComponentProps<typeof WorkspaceInitialPage>>({route: {params: {policyID: 'policy-2'}}}); | ||
| rerender(<WorkspaceInitialPage {...secondProps} />); | ||
|
|
||
| expect(mockFullPageNotFoundView).toHaveBeenLastCalledWith(expect.objectContaining({shouldShow: false})); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the existing
TAB_NAVIGATORwas created by a nested push,getRehydratedTabNavigatorStateAfterPush()stores the consumed target asroute.params.statewithout ascreenkey (lines 176–182). This call does not remove that hint becausewithSanitizedDeepLinkParams()only sanitizes params containing a stringscreen, so creating a workspace afterward can still let React Navigation replay the previous nested workspace state overstaleTabState. Sanitize the state-only form here as well before remounting.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. The state-only hint comes from the separate nested-push flow, while this PR addresses stale
params.screenhints from the reported distance-settings flow. Expanding the shared sanitizer would increase this regression fix’s blast radius, so I propose handling the state-only form in a focused follow-up with dedicated coverage.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @sumo-slonik