Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 6 additions & 20 deletions src/hooks/useSubPage/index.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import {useRoute} from '@react-navigation/native';
import {useNavigation, useRoute} from '@react-navigation/native';
import type {ComponentType} from 'react';
import {useEffect} from 'react';
import Navigation from '@libs/Navigation/Navigation';
import {findLastPageIndex, findPageIndex} from '@libs/SubPageUtils';
import type {SubPageProps, UseSubPageProps} from './types';

const AMOUNT_OF_FRAMES_TO_WAIT_FOR = 20;

/**
* @param pages - array of objects with pageName and component to display in each page
* @param onFinished - callback triggered after finishing the last page
Expand All @@ -16,34 +14,22 @@ const AMOUNT_OF_FRAMES_TO_WAIT_FOR = 20;
* @param buildRoute - function that returns the route for a given page name and optional action
*/
export default function useSubPage<TProps extends SubPageProps>({pages, onFinished, startFrom = 0, skipPages = [], onPageChange = () => {}, buildRoute}: UseSubPageProps<TProps>) {
const navigation = useNavigation();
const route = useRoute();
const params = route.params as {subPage?: string; action?: 'edit'} | undefined;
const urlPageName = params?.subPage;
const isEditing = params?.action === 'edit';

const startPageName = pages.at(startFrom)?.pageName;
const isRedirecting = !urlPageName && !!startPageName;
const startPageName = startFrom >= 0 ? pages.at(startFrom)?.pageName : undefined;
const isRedirecting = !urlPageName && (!!startPageName || startFrom < 0);

useEffect(() => {
if (!isRedirecting) {
return;
}

let requestID: number;
const waitFrames = (framesLeft: number) => {
if (framesLeft <= 0) {
Navigation.navigate(buildRoute(startPageName), {forceReplace: true});
return;
}
requestID = requestAnimationFrame(() => waitFrames(framesLeft - 1));
};

requestID = requestAnimationFrame(() => waitFrames(AMOUNT_OF_FRAMES_TO_WAIT_FOR));

return () => {
cancelAnimationFrame(requestID);
};
}, [isRedirecting, startPageName, buildRoute]);
navigation.setParams({subPage: startPageName} as Record<string, unknown>);

Choose a reason for hiding this comment

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

P2 Badge Wait for startFrom to settle before setting the initial subPage

When a useSubPage screen opens without :subPage, this now calls setParams() on the first effect tick and permanently makes urlPageName truthy. In MissingPersonalDetailsContent, startFrom depends on shouldCollectPIN, which is still derived from ONYXKEYS.CARD_LIST after mount (src/pages/MissingPersonalDetails/MissingPersonalDetailsContent.tsx:66-90), and getInitialSubPage() only routes to PIN once that flag is true (src/pages/MissingPersonalDetails/utils.ts:26-48). If the card list arrives after the first render, the hook has already locked the route to the earlier page and will never redirect again, so UK/EU card users can reopen the flow on confirm instead of the required PIN step.

Useful? React with 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's very unlikely as ONYXKEYS.CARD_LIST is fetched before we show this screen. @dukenv0307 wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Replied on Slack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've discussed in slack and in result Ive added a simple check that should prevent this

}, [isRedirecting, startPageName, navigation]);

const currentPageName = urlPageName ?? startPageName ?? pages.at(0)?.pageName;
const pageIndex = findPageIndex(pages, currentPageName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,15 +88,6 @@ const OPTIONS_PER_SCREEN: Partial<Record<Screen, PlatformStackNavigationOptions>
[SCREENS.TRAVEL.WORKSPACE_ADDRESS]: {
animationTypeForReplace: 'push',
},
[SCREENS.MISSING_PERSONAL_DETAILS]: {
animationTypeForReplace: 'push',
},
[SCREENS.WORKSPACE.ACCOUNTING.NETSUITE_IMPORT_CUSTOM_LIST_ADD]: {
animationTypeForReplace: 'push',
},
[SCREENS.SETTINGS.ADD_BANK_ACCOUNT]: {
animationTypeForReplace: 'push',
},
[SCREENS.MULTIFACTOR_AUTHENTICATION.MAGIC_CODE]: {
animationTypeForReplace: 'push',
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ function MissingPersonalDetailsContent({privatePersonalDetails, draftValues, hea
const values = useMemo(() => normalizeCountryCode(getSubPageValues(privatePersonalDetails, draftValues)) as PersonalDetailsForm, [privatePersonalDetails, draftValues]);

const startFrom = useMemo(() => {
if (shouldCollectPIN === undefined) {
return -1;
}
const initialPage = getInitialSubPage(values, shouldCollectPIN, PIN);
return findPageIndex<CustomSubPageProps>(formPages, initialPage);
}, [formPages, values, shouldCollectPIN, PIN]);
Expand Down
11 changes: 4 additions & 7 deletions tests/unit/useSubPageTest.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,16 @@ jest.mock('@libs/Navigation/Navigation', () => ({
goBack: jest.fn(),
}));

jest.spyOn(global, 'requestAnimationFrame').mockImplementation((callback) => {
callback(0);
return 0;
});

type AnyRoute = RouteProp<Record<string, Record<string, unknown> | undefined>, string>;
const mockUseRoute = jest.fn<AnyRoute, []>();
const mockSetParams = jest.fn();
jest.mock('@react-navigation/native', () => {
// eslint-disable-next-line @typescript-eslint/consistent-type-imports
const actualNav = jest.requireActual<typeof import('@react-navigation/native')>('@react-navigation/native');
return {
...actualNav,
useRoute: () => mockUseRoute(),
useNavigation: () => ({setParams: mockSetParams}),
};
});

Expand Down Expand Up @@ -135,7 +132,7 @@ describe('useSubPage hook', () => {
expect(result.current.CurrentPage).toBe(mockSubPageTwo);
});

it('navigates to startFrom page on mount when no subPage param is present', () => {
it('sets params to startFrom page on mount when no subPage param is present', () => {
const pages = createMockPages();
const buildRoute = createBuildRoute();

Expand All @@ -148,7 +145,7 @@ describe('useSubPage hook', () => {
}),
);

expect(Navigation.navigate).toHaveBeenCalledWith(buildRoute('page3'), {forceReplace: true});
expect(mockSetParams).toHaveBeenCalledWith({subPage: 'page3'});
});

it('returns isRedirecting true when no subPage param is present in URL', () => {
Expand Down
Loading