From 0852425dd34ac9cc55e18009db9d76b562a21481 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 27 Jun 2024 10:03:56 -0700 Subject: [PATCH 1/8] Create useLastAccessedReportID hook --- src/hooks/useLastAccessedReportID.ts | 38 ++++++++++ .../AppNavigator/ReportScreenIDSetter.ts | 72 +++---------------- 2 files changed, 48 insertions(+), 62 deletions(-) create mode 100644 src/hooks/useLastAccessedReportID.ts diff --git a/src/hooks/useLastAccessedReportID.ts b/src/hooks/useLastAccessedReportID.ts new file mode 100644 index 000000000000..ddec6f43166b --- /dev/null +++ b/src/hooks/useLastAccessedReportID.ts @@ -0,0 +1,38 @@ +import {useMemo} from 'react'; +import {useOnyx} from 'react-native-onyx'; +import {getPolicyEmployeeListByIdWithoutCurrentUser} from '@libs/PolicyUtils'; +import * as ReportUtils from '@libs/ReportUtils'; +import ONYXKEYS from '@src/ONYXKEYS'; +import useActiveWorkspace from './useActiveWorkspace'; +import usePermissions from './usePermissions'; + +/** + * Get the last accessed reportID. + */ +export default function useLastAccessedReportID(shouldOpenOnAdminRoom: boolean) { + const {canUseDefaultRooms} = usePermissions(); + const {activeWorkspaceID} = useActiveWorkspace(); + + const [reports] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {allowStaleData: true}); + const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {allowStaleData: true}); + const [isFirstTimeNewExpensifyUser = false] = useOnyx(ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER); + const [reportMetadata] = useOnyx(ONYXKEYS.COLLECTION.REPORT_METADATA, {allowStaleData: true}); + const [accountID] = useOnyx(ONYXKEYS.SESSION, {selector: (session) => session?.accountID}); + + const policyMemberAccountIDs = useMemo(() => getPolicyEmployeeListByIdWithoutCurrentUser(policies, activeWorkspaceID, accountID), [accountID, activeWorkspaceID, policies]); + + return useMemo( + () => + ReportUtils.findLastAccessedReport( + reports, + !canUseDefaultRooms, + policies, + isFirstTimeNewExpensifyUser, + shouldOpenOnAdminRoom, + reportMetadata, + activeWorkspaceID, + policyMemberAccountIDs, + )?.reportID, + [activeWorkspaceID, canUseDefaultRooms, isFirstTimeNewExpensifyUser, policies, policyMemberAccountIDs, reportMetadata, reports, shouldOpenOnAdminRoom], + ); +} diff --git a/src/libs/Navigation/AppNavigator/ReportScreenIDSetter.ts b/src/libs/Navigation/AppNavigator/ReportScreenIDSetter.ts index 5306f6b55054..e3546946e231 100644 --- a/src/libs/Navigation/AppNavigator/ReportScreenIDSetter.ts +++ b/src/libs/Navigation/AppNavigator/ReportScreenIDSetter.ts @@ -1,57 +1,16 @@ import {useEffect} from 'react'; -import type {OnyxCollection, OnyxEntry} from 'react-native-onyx'; -import {useOnyx} from 'react-native-onyx'; -import useActiveWorkspace from '@hooks/useActiveWorkspace'; -import usePermissions from '@hooks/usePermissions'; -import {getPolicyEmployeeListByIdWithoutCurrentUser} from '@libs/PolicyUtils'; -import * as ReportUtils from '@libs/ReportUtils'; -import ONYXKEYS from '@src/ONYXKEYS'; -import type {Policy, Report, ReportMetadata} from '@src/types/onyx'; +import useLastAccessedReportID from '@hooks/useLastAccessedReportID'; +import Log from '@libs/Log'; import type {ReportScreenWrapperProps} from './ReportScreenWrapper'; type ReportScreenIDSetterProps = ReportScreenWrapperProps; -/** - * Get the most recently accessed report for the user - */ -const getLastAccessedReportID = ( - reports: OnyxCollection, - ignoreDefaultRooms: boolean, - policies: OnyxCollection, - isFirstTimeNewExpensifyUser: OnyxEntry, - openOnAdminRoom: boolean, - reportMetadata: OnyxCollection, - policyID?: string, - policyMemberAccountIDs?: number[], -): string | undefined => { - const lastReport = ReportUtils.findLastAccessedReport( - reports, - ignoreDefaultRooms, - policies, - !!isFirstTimeNewExpensifyUser, - openOnAdminRoom, - reportMetadata, - policyID, - policyMemberAccountIDs, - ); - return lastReport?.reportID; -}; - // This wrapper is responsible for opening the last accessed report if there is no reportID specified in the route params function ReportScreenIDSetter({route, navigation}: ReportScreenIDSetterProps) { - const {canUseDefaultRooms} = usePermissions(); - const {activeWorkspaceID} = useActiveWorkspace(); - - const [reports] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {allowStaleData: true}); - const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {allowStaleData: true}); - const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); - const [isFirstTimeNewExpensifyUser] = useOnyx(ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, {initialValue: false}); - const [reportMetadata] = useOnyx(ONYXKEYS.COLLECTION.REPORT_METADATA, {allowStaleData: true}); - const [accountID] = useOnyx(ONYXKEYS.SESSION, {selector: (session) => session?.accountID}); - + const lastAccessedReportID = useLastAccessedReportID(!!route.params.openOnAdminRoom); useEffect(() => { // Don't update if there is a reportID in the params already - if (route?.params?.reportID) { + if (route.params.reportID) { const reportActionID = route?.params?.reportActionID; const regexValidReportActionID = new RegExp(/^\d*$/); if (reportActionID && !regexValidReportActionID.test(reportActionID)) { @@ -60,26 +19,15 @@ function ReportScreenIDSetter({route, navigation}: ReportScreenIDSetterProps) { return; } - const policyMemberAccountIDs = getPolicyEmployeeListByIdWithoutCurrentUser(policies, activeWorkspaceID, accountID); - - // If there is no reportID in route, try to find last accessed and use it for setParams - const reportID = getLastAccessedReportID( - reports, - !canUseDefaultRooms, - policies, - isFirstTimeNewExpensifyUser, - !!reports?.params?.openOnAdminRoom, - reportMetadata, - activeWorkspaceID, - policyMemberAccountIDs, - ); - // It's possible that reports aren't fully loaded yet // in that case the reportID is undefined - if (reportID) { - navigation.setParams({reportID: String(reportID)}); + if (!lastAccessedReportID) { + return; } - }, [route, navigation, reports, canUseDefaultRooms, policies, isFirstTimeNewExpensifyUser, reportMetadata, activeWorkspaceID, personalDetails, accountID]); + + Log.info(`[ReportScreen] no reportID found in params, setting it to lastAccessedReportID: ${lastAccessedReportID}`); + navigation.setParams({reportID: lastAccessedReportID}); + }, [lastAccessedReportID, navigation, route]); // The ReportScreen without the reportID set will display a skeleton // until the reportID is loaded and set in the route param From f36891df651a8949841ad71a48c68390cd64b30b Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 27 Jun 2024 10:12:47 -0700 Subject: [PATCH 2/8] Supply lastAccessedReportID in initialParams --- src/libs/Navigation/AppNavigator/AuthScreens.tsx | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/libs/Navigation/AppNavigator/AuthScreens.tsx b/src/libs/Navigation/AppNavigator/AuthScreens.tsx index c9773f104393..bdca8259c47e 100644 --- a/src/libs/Navigation/AppNavigator/AuthScreens.tsx +++ b/src/libs/Navigation/AppNavigator/AuthScreens.tsx @@ -4,6 +4,7 @@ import type {OnyxEntry} from 'react-native-onyx'; import Onyx, {withOnyx} from 'react-native-onyx'; import type {ValueOf} from 'type-fest'; import OptionsListContextProvider from '@components/OptionListContextProvider'; +import useLastAccessedReportID from '@hooks/useLastAccessedReportID'; import useOnboardingLayout from '@hooks/useOnboardingLayout'; import useStyleUtils from '@hooks/useStyleUtils'; import useThemeStyles from '@hooks/useThemeStyles'; @@ -76,16 +77,18 @@ const loadReportAvatar = () => require('../../../pages/Rep const loadReceiptView = () => require('../../../pages/TransactionReceiptPage').default; const loadWorkspaceJoinUser = () => require('@pages/workspace/WorkspaceJoinUserPage').default; -function getCentralPaneScreenInitialParams(screenName: CentralPaneName): Partial> { +function shouldOpenOnAdminRoom() { const url = getCurrentUrl(); - const openOnAdminRoom = url ? new URL(url).searchParams.get('openOnAdminRoom') : undefined; + return url ? new URL(url).searchParams.get('openOnAdminRoom') === 'true' : false; +} +function getCentralPaneScreenInitialParams(screenName: CentralPaneName, lastAccessedReportID?: string): Partial> { if (screenName === SCREENS.SEARCH.CENTRAL_PANE) { return {sortBy: CONST.SEARCH.TABLE_COLUMNS.DATE, sortOrder: CONST.SEARCH.SORT_ORDER.DESC}; } - if (screenName === SCREENS.REPORT && openOnAdminRoom === 'true') { - return {openOnAdminRoom: true}; + if (screenName === SCREENS.REPORT) { + return {openOnAdminRoom: shouldOpenOnAdminRoom(), reportID: lastAccessedReportID}; } return undefined; @@ -191,6 +194,7 @@ function AuthScreens({session, lastOpenedPublicRoomID, initialLastUpdateIDApplie const StyleUtils = useStyleUtils(); const {isSmallScreenWidth} = useWindowDimensions(); const {shouldUseNarrowLayout} = useOnboardingLayout(); + const lastAccessedReportID = useLastAccessedReportID(shouldOpenOnAdminRoom()); const screenOptions = getRootNavigatorScreenOptions(isSmallScreenWidth, styles, StyleUtils); const onboardingModalScreenOptions = useMemo(() => screenOptions.onboardingModalNavigator(shouldUseNarrowLayout), [screenOptions, shouldUseNarrowLayout]); const onboardingScreenOptions = useMemo( @@ -461,7 +465,7 @@ function AuthScreens({session, lastOpenedPublicRoomID, initialLastUpdateIDApplie From a77340c0e7fe68c4da1ba89450bd00e5c08d4c44 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 27 Jun 2024 10:21:50 -0700 Subject: [PATCH 3/8] Remove unnecessary ReportScreenWrapper component --- src/components/ScreenWrapper.tsx | 2 +- .../AppNavigator/CENTRAL_PANE_SCREENS.tsx | 2 +- .../AppNavigator/ReportScreenIDSetter.ts | 39 ------------------- .../AppNavigator/ReportScreenWrapper.tsx | 30 -------------- src/pages/home/ReportScreen.tsx | 25 ++++++++++++ 5 files changed, 27 insertions(+), 71 deletions(-) delete mode 100644 src/libs/Navigation/AppNavigator/ReportScreenIDSetter.ts delete mode 100644 src/libs/Navigation/AppNavigator/ReportScreenWrapper.tsx diff --git a/src/components/ScreenWrapper.tsx b/src/components/ScreenWrapper.tsx index 5ebf9b4766d6..1d5d65d9874d 100644 --- a/src/components/ScreenWrapper.tsx +++ b/src/components/ScreenWrapper.tsx @@ -131,7 +131,7 @@ function ScreenWrapper( ) { /** * We are only passing navigation as prop from - * ReportScreenWrapper -> ReportScreen -> ScreenWrapper + * ReportScreen -> ScreenWrapper * * so in other places where ScreenWrapper is used, we need to * fallback to useNavigation. diff --git a/src/libs/Navigation/AppNavigator/CENTRAL_PANE_SCREENS.tsx b/src/libs/Navigation/AppNavigator/CENTRAL_PANE_SCREENS.tsx index 3300fa323181..4d5fe2a878db 100644 --- a/src/libs/Navigation/AppNavigator/CENTRAL_PANE_SCREENS.tsx +++ b/src/libs/Navigation/AppNavigator/CENTRAL_PANE_SCREENS.tsx @@ -16,7 +16,7 @@ const CENTRAL_PANE_SCREENS = { [SCREENS.SETTINGS.SAVE_THE_WORLD]: () => withPrepareCentralPaneScreen(require('../../../pages/TeachersUnite/SaveTheWorldPage').default), [SCREENS.SETTINGS.SUBSCRIPTION.ROOT]: () => withPrepareCentralPaneScreen(require('../../../pages/settings/Subscription/SubscriptionSettingsPage').default), [SCREENS.SEARCH.CENTRAL_PANE]: () => withPrepareCentralPaneScreen(require('../../../pages/Search/SearchPage').default), - [SCREENS.REPORT]: () => withPrepareCentralPaneScreen(require('./ReportScreenWrapper').default), + [SCREENS.REPORT]: () => withPrepareCentralPaneScreen(require('../../../pages/home/ReportScreen').default), } satisfies Screens; export default CENTRAL_PANE_SCREENS; diff --git a/src/libs/Navigation/AppNavigator/ReportScreenIDSetter.ts b/src/libs/Navigation/AppNavigator/ReportScreenIDSetter.ts deleted file mode 100644 index e3546946e231..000000000000 --- a/src/libs/Navigation/AppNavigator/ReportScreenIDSetter.ts +++ /dev/null @@ -1,39 +0,0 @@ -import {useEffect} from 'react'; -import useLastAccessedReportID from '@hooks/useLastAccessedReportID'; -import Log from '@libs/Log'; -import type {ReportScreenWrapperProps} from './ReportScreenWrapper'; - -type ReportScreenIDSetterProps = ReportScreenWrapperProps; - -// This wrapper is responsible for opening the last accessed report if there is no reportID specified in the route params -function ReportScreenIDSetter({route, navigation}: ReportScreenIDSetterProps) { - const lastAccessedReportID = useLastAccessedReportID(!!route.params.openOnAdminRoom); - useEffect(() => { - // Don't update if there is a reportID in the params already - if (route.params.reportID) { - const reportActionID = route?.params?.reportActionID; - const regexValidReportActionID = new RegExp(/^\d*$/); - if (reportActionID && !regexValidReportActionID.test(reportActionID)) { - navigation.setParams({reportActionID: ''}); - } - return; - } - - // It's possible that reports aren't fully loaded yet - // in that case the reportID is undefined - if (!lastAccessedReportID) { - return; - } - - Log.info(`[ReportScreen] no reportID found in params, setting it to lastAccessedReportID: ${lastAccessedReportID}`); - navigation.setParams({reportID: lastAccessedReportID}); - }, [lastAccessedReportID, navigation, route]); - - // The ReportScreen without the reportID set will display a skeleton - // until the reportID is loaded and set in the route param - return null; -} - -ReportScreenIDSetter.displayName = 'ReportScreenIDSetter'; - -export default ReportScreenIDSetter; diff --git a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.tsx b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.tsx deleted file mode 100644 index 692bbf8edde2..000000000000 --- a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.tsx +++ /dev/null @@ -1,30 +0,0 @@ -import type {StackScreenProps} from '@react-navigation/stack'; -import React from 'react'; -import type {AuthScreensParamList} from '@navigation/types'; -import ReportScreen from '@pages/home/ReportScreen'; -import type SCREENS from '@src/SCREENS'; -import ReportScreenIDSetter from './ReportScreenIDSetter'; - -type ReportScreenWrapperProps = StackScreenProps; - -function ReportScreenWrapper({route, navigation}: ReportScreenWrapperProps) { - // The ReportScreen without the reportID set will display a skeleton - // until the reportID is loaded and set in the route param - return ( - <> - - - - ); -} - -ReportScreenWrapper.displayName = 'ReportScreenWrapper'; - -export default ReportScreenWrapper; -export type {ReportScreenWrapperProps}; diff --git a/src/pages/home/ReportScreen.tsx b/src/pages/home/ReportScreen.tsx index 9a0dce44e9ec..017e9f849550 100644 --- a/src/pages/home/ReportScreen.tsx +++ b/src/pages/home/ReportScreen.tsx @@ -23,6 +23,7 @@ import withCurrentReportID from '@components/withCurrentReportID'; import useAppFocusEvent from '@hooks/useAppFocusEvent'; import useDeepCompareRef from '@hooks/useDeepCompareRef'; import useIsReportOpenInRHP from '@hooks/useIsReportOpenInRHP'; +import useLastAccessedReportID from '@hooks/useLastAccessedReportID'; import useLocalize from '@hooks/useLocalize'; import useNetwork from '@hooks/useNetwork'; import usePrevious from '@hooks/usePrevious'; @@ -31,6 +32,7 @@ import useViewportOffsetTop from '@hooks/useViewportOffsetTop'; import useWindowDimensions from '@hooks/useWindowDimensions'; import {getCurrentUserAccountID} from '@libs/actions/Report'; import Timing from '@libs/actions/Timing'; +import Log from '@libs/Log'; import Navigation from '@libs/Navigation/Navigation'; import clearReportNotifications from '@libs/Notification/clearReportNotifications'; import Performance from '@libs/Performance'; @@ -161,6 +163,29 @@ function ReportScreen({ const isLoadingReportOnyx = isLoadingOnyxValue(reportResult); const permissions = useDeepCompareRef(reportOnyx?.permissions); + // check if there's a reportID in the route. If not, set it to the last accessed reportID + const lastAccessedReportID = useLastAccessedReportID(!!route.params.openOnAdminRoom); + useEffect(() => { + // Don't update if there is a reportID in the params already + if (route.params.reportID) { + const reportActionID = route?.params?.reportActionID; + const regexValidReportActionID = new RegExp(/^\d*$/); + if (reportActionID && !regexValidReportActionID.test(reportActionID)) { + navigation.setParams({reportActionID: ''}); + } + return; + } + + // It's possible that reports aren't fully loaded yet + // in that case the reportID is undefined + if (!lastAccessedReportID) { + return; + } + + Log.info(`[ReportScreen] no reportID found in params, setting it to lastAccessedReportID: ${lastAccessedReportID}`); + navigation.setParams({reportID: lastAccessedReportID}); + }, [lastAccessedReportID, navigation, route]); + /** * Create a lightweight Report so as to keep the re-rendering as light as possible by * passing in only the required props. From 28904ba3b19ab6c9020f2967de7e4213c0cfab32 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 27 Jun 2024 10:37:21 -0700 Subject: [PATCH 4/8] Don't pass openOnAdminRoom unless it's true --- src/libs/Navigation/AppNavigator/AuthScreens.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libs/Navigation/AppNavigator/AuthScreens.tsx b/src/libs/Navigation/AppNavigator/AuthScreens.tsx index bdca8259c47e..61a5be28f980 100644 --- a/src/libs/Navigation/AppNavigator/AuthScreens.tsx +++ b/src/libs/Navigation/AppNavigator/AuthScreens.tsx @@ -88,7 +88,10 @@ function getCentralPaneScreenInitialParams(screenName: CentralPaneName, lastAcce } if (screenName === SCREENS.REPORT) { - return {openOnAdminRoom: shouldOpenOnAdminRoom(), reportID: lastAccessedReportID}; + return { + openOnAdminRoom: shouldOpenOnAdminRoom() ? true : undefined, + reportID: lastAccessedReportID, + }; } return undefined; From f5d9540473ea961b2135a9650812863098241b10 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 27 Jun 2024 13:06:09 -0700 Subject: [PATCH 5/8] Update useLastAccessedReportID to use useSyncExternalStore and Onyx.connect directly --- src/hooks/useLastAccessedReportID.ts | 94 +++++++++++++++++++++------- 1 file changed, 71 insertions(+), 23 deletions(-) diff --git a/src/hooks/useLastAccessedReportID.ts b/src/hooks/useLastAccessedReportID.ts index ddec6f43166b..32c7d6f0d292 100644 --- a/src/hooks/useLastAccessedReportID.ts +++ b/src/hooks/useLastAccessedReportID.ts @@ -1,11 +1,64 @@ -import {useMemo} from 'react'; -import {useOnyx} from 'react-native-onyx'; +import {useCallback, useSyncExternalStore} from 'react'; +import type {OnyxCollection} from 'react-native-onyx'; +import Onyx from 'react-native-onyx'; import {getPolicyEmployeeListByIdWithoutCurrentUser} from '@libs/PolicyUtils'; import * as ReportUtils from '@libs/ReportUtils'; import ONYXKEYS from '@src/ONYXKEYS'; +import type {Policy, Report, ReportMetadata} from '@src/types/onyx'; import useActiveWorkspace from './useActiveWorkspace'; import usePermissions from './usePermissions'; +/* + * This hook is used to get the lastAccessedReportID. + * This is a piece of data that's derived from a lot of frequently-changing Onyx values: (reports, reportMetadata, policies, etc...) + * We don't want any component that needs access to the lastAccessedReportID to have to re-render any time any of those values change, just when the lastAccessedReportID changes. + * So we have a custom implementation in this file that leverages useSyncExternalStore to connect to a "store" of multiple Onyx values, and re-render only when the one derived value changes. + */ + +let reports: OnyxCollection = {}; +let reportMetadata: OnyxCollection = {}; +let policies: OnyxCollection = {}; +let accountID: number | undefined; +let isFirstTimeNewExpensifyUser = false; + +function subscribeToOnyxData() { + // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs + const reportsConnection = Onyx.connect({ + key: ONYXKEYS.COLLECTION.REPORT, + waitForCollectionCallback: true, + callback: (value) => (reports = value), + }); + // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs + const reportMetadataConnection = Onyx.connect({ + key: ONYXKEYS.COLLECTION.REPORT_METADATA, + waitForCollectionCallback: true, + callback: (value) => (reportMetadata = value), + }); + // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs + const policiesConnection = Onyx.connect({ + key: ONYXKEYS.COLLECTION.POLICY, + waitForCollectionCallback: true, + callback: (value) => (policies = value), + }); + // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs + const isFirstTimeNewExpensifyUserConnection = Onyx.connect({ + key: ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, + callback: (value) => (isFirstTimeNewExpensifyUser = !!value), + }); + // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs + const accountIDConnection = Onyx.connect({ + key: ONYXKEYS.SESSION, + callback: (value) => (accountID = value?.accountID), + }); + return () => { + Onyx.disconnect(reportsConnection); + Onyx.disconnect(reportMetadataConnection); + Onyx.disconnect(policiesConnection); + Onyx.disconnect(isFirstTimeNewExpensifyUserConnection); + Onyx.disconnect(accountIDConnection); + }; +} + /** * Get the last accessed reportID. */ @@ -13,26 +66,21 @@ export default function useLastAccessedReportID(shouldOpenOnAdminRoom: boolean) const {canUseDefaultRooms} = usePermissions(); const {activeWorkspaceID} = useActiveWorkspace(); - const [reports] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {allowStaleData: true}); - const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {allowStaleData: true}); - const [isFirstTimeNewExpensifyUser = false] = useOnyx(ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER); - const [reportMetadata] = useOnyx(ONYXKEYS.COLLECTION.REPORT_METADATA, {allowStaleData: true}); - const [accountID] = useOnyx(ONYXKEYS.SESSION, {selector: (session) => session?.accountID}); - - const policyMemberAccountIDs = useMemo(() => getPolicyEmployeeListByIdWithoutCurrentUser(policies, activeWorkspaceID, accountID), [accountID, activeWorkspaceID, policies]); + const getSnapshot = useCallback(() => { + const policyMemberAccountIDs = getPolicyEmployeeListByIdWithoutCurrentUser(policies, activeWorkspaceID, accountID); + return ReportUtils.findLastAccessedReport( + reports, + !canUseDefaultRooms, + policies, + isFirstTimeNewExpensifyUser, + shouldOpenOnAdminRoom, + reportMetadata, + activeWorkspaceID, + policyMemberAccountIDs, + )?.reportID; + }, [activeWorkspaceID, canUseDefaultRooms, shouldOpenOnAdminRoom]); - return useMemo( - () => - ReportUtils.findLastAccessedReport( - reports, - !canUseDefaultRooms, - policies, - isFirstTimeNewExpensifyUser, - shouldOpenOnAdminRoom, - reportMetadata, - activeWorkspaceID, - policyMemberAccountIDs, - )?.reportID, - [activeWorkspaceID, canUseDefaultRooms, isFirstTimeNewExpensifyUser, policies, policyMemberAccountIDs, reportMetadata, reports, shouldOpenOnAdminRoom], - ); + // We need access to all the data from these useOnyx calls, but we don't want to re-render the consuming component + // unless the derived value (lastAccessedReportID) changes. To address these, we'll wrap everything with + return useSyncExternalStore(subscribeToOnyxData, getSnapshot); } From 6ed81338812ee7c7f1e095caec696f9d90461109 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 27 Jun 2024 13:44:23 -0700 Subject: [PATCH 6/8] Correctly notify and handle multiple users of the hook --- src/hooks/useLastAccessedReportID.ts | 94 +++++++++++++++++++++++----- 1 file changed, 78 insertions(+), 16 deletions(-) diff --git a/src/hooks/useLastAccessedReportID.ts b/src/hooks/useLastAccessedReportID.ts index 32c7d6f0d292..f647a18422df 100644 --- a/src/hooks/useLastAccessedReportID.ts +++ b/src/hooks/useLastAccessedReportID.ts @@ -15,48 +15,110 @@ import usePermissions from './usePermissions'; * So we have a custom implementation in this file that leverages useSyncExternalStore to connect to a "store" of multiple Onyx values, and re-render only when the one derived value changes. */ +const subscribers: Array<() => void> = []; + let reports: OnyxCollection = {}; let reportMetadata: OnyxCollection = {}; let policies: OnyxCollection = {}; let accountID: number | undefined; let isFirstTimeNewExpensifyUser = false; +let reportsConnection: number; +let reportMetadataConnection: number; +let policiesConnection: number; +let accountIDConnection: number; +let isFirstTimeNewExpensifyUserConnection: number; + +function notifySubscribers() { + subscribers.forEach((subscriber) => subscriber()); +} + function subscribeToOnyxData() { // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs - const reportsConnection = Onyx.connect({ + reportsConnection = Onyx.connect({ key: ONYXKEYS.COLLECTION.REPORT, waitForCollectionCallback: true, - callback: (value) => (reports = value), + callback: (value) => { + reports = value; + notifySubscribers(); + }, }); // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs - const reportMetadataConnection = Onyx.connect({ + reportMetadataConnection = Onyx.connect({ key: ONYXKEYS.COLLECTION.REPORT_METADATA, waitForCollectionCallback: true, - callback: (value) => (reportMetadata = value), + callback: (value) => { + reportMetadata = value; + notifySubscribers(); + }, }); // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs - const policiesConnection = Onyx.connect({ + policiesConnection = Onyx.connect({ key: ONYXKEYS.COLLECTION.POLICY, waitForCollectionCallback: true, - callback: (value) => (policies = value), + callback: (value) => { + policies = value; + notifySubscribers(); + }, }); // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs - const isFirstTimeNewExpensifyUserConnection = Onyx.connect({ - key: ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, - callback: (value) => (isFirstTimeNewExpensifyUser = !!value), + accountIDConnection = Onyx.connect({ + key: ONYXKEYS.SESSION, + callback: (value) => { + accountID = value?.accountID; + notifySubscribers(); + }, }); // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs - const accountIDConnection = Onyx.connect({ - key: ONYXKEYS.SESSION, - callback: (value) => (accountID = value?.accountID), + isFirstTimeNewExpensifyUserConnection = Onyx.connect({ + key: ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, + callback: (value) => { + isFirstTimeNewExpensifyUser = !!value; + notifySubscribers(); + }, }); - return () => { +} + +function unsubscribeFromOnyxData() { + if (reportsConnection) { Onyx.disconnect(reportsConnection); + reportsConnection = 0; + } + if (reportMetadataConnection) { Onyx.disconnect(reportMetadataConnection); + reportMetadataConnection = 0; + } + if (policiesConnection) { Onyx.disconnect(policiesConnection); - Onyx.disconnect(isFirstTimeNewExpensifyUserConnection); + policiesConnection = 0; + } + if (accountIDConnection) { Onyx.disconnect(accountIDConnection); - }; + accountIDConnection = 0; + } + if (isFirstTimeNewExpensifyUserConnection) { + Onyx.disconnect(isFirstTimeNewExpensifyUserConnection); + isFirstTimeNewExpensifyUserConnection = 0; + } +} + +function removeSubscriber(subscriber: () => void) { + const subscriberIndex = subscribers.indexOf(subscriber); + if (subscriberIndex < 0) { + return; + } + subscribers.splice(subscriberIndex, 1); + if (subscribers.length === 0) { + unsubscribeFromOnyxData(); + } +} + +function addSubscriber(subscriber: () => void) { + subscribers.push(subscriber); + if (!reportsConnection) { + subscribeToOnyxData(); + } + return () => removeSubscriber(subscriber); } /** @@ -82,5 +144,5 @@ export default function useLastAccessedReportID(shouldOpenOnAdminRoom: boolean) // We need access to all the data from these useOnyx calls, but we don't want to re-render the consuming component // unless the derived value (lastAccessedReportID) changes. To address these, we'll wrap everything with - return useSyncExternalStore(subscribeToOnyxData, getSnapshot); + return useSyncExternalStore(addSubscriber, getSnapshot); } From 51c85dd132f873cc79c4b0e643dd79c539ad59a7 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 27 Jun 2024 13:45:43 -0700 Subject: [PATCH 7/8] Improve comments --- src/hooks/useLastAccessedReportID.ts | 4 ++-- src/pages/home/ReportScreen.tsx | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/hooks/useLastAccessedReportID.ts b/src/hooks/useLastAccessedReportID.ts index f647a18422df..16a4a6bc2a31 100644 --- a/src/hooks/useLastAccessedReportID.ts +++ b/src/hooks/useLastAccessedReportID.ts @@ -142,7 +142,7 @@ export default function useLastAccessedReportID(shouldOpenOnAdminRoom: boolean) )?.reportID; }, [activeWorkspaceID, canUseDefaultRooms, shouldOpenOnAdminRoom]); - // We need access to all the data from these useOnyx calls, but we don't want to re-render the consuming component - // unless the derived value (lastAccessedReportID) changes. To address these, we'll wrap everything with + // We need access to all the data from these Onyx.connect calls, but we don't want to re-render the consuming component + // unless the derived value (lastAccessedReportID) changes. To address these, we'll wrap everything with useSyncExternalStore return useSyncExternalStore(addSubscriber, getSnapshot); } diff --git a/src/pages/home/ReportScreen.tsx b/src/pages/home/ReportScreen.tsx index 017e9f849550..f1c658b5db07 100644 --- a/src/pages/home/ReportScreen.tsx +++ b/src/pages/home/ReportScreen.tsx @@ -163,7 +163,7 @@ function ReportScreen({ const isLoadingReportOnyx = isLoadingOnyxValue(reportResult); const permissions = useDeepCompareRef(reportOnyx?.permissions); - // check if there's a reportID in the route. If not, set it to the last accessed reportID + // Check if there's a reportID in the route. If not, set it to the last accessed reportID const lastAccessedReportID = useLastAccessedReportID(!!route.params.openOnAdminRoom); useEffect(() => { // Don't update if there is a reportID in the params already From d1781035ed6f98c3faed196f430b90177234763a Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 27 Jun 2024 13:56:40 -0700 Subject: [PATCH 8/8] Use ValidationUtils.isNumeric --- src/libs/ValidationUtils.ts | 2 +- src/pages/home/ReportScreen.tsx | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libs/ValidationUtils.ts b/src/libs/ValidationUtils.ts index 87dcede7f0c9..5fedd5443a89 100644 --- a/src/libs/ValidationUtils.ts +++ b/src/libs/ValidationUtils.ts @@ -407,7 +407,7 @@ function isNumeric(value: string): boolean { if (typeof value !== 'string') { return false; } - return /^\d*$/.test(value); + return CONST.REGEX.NUMBER.test(value); } /** diff --git a/src/pages/home/ReportScreen.tsx b/src/pages/home/ReportScreen.tsx index f1c658b5db07..d8d98fe32d96 100644 --- a/src/pages/home/ReportScreen.tsx +++ b/src/pages/home/ReportScreen.tsx @@ -40,6 +40,7 @@ import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils'; import * as ReportActionsUtils from '@libs/ReportActionsUtils'; import * as ReportUtils from '@libs/ReportUtils'; import shouldFetchReport from '@libs/shouldFetchReport'; +import * as ValidationUtils from '@libs/ValidationUtils'; import type {AuthScreensParamList} from '@navigation/types'; import variables from '@styles/variables'; import * as ComposerActions from '@userActions/Composer'; @@ -169,8 +170,8 @@ function ReportScreen({ // Don't update if there is a reportID in the params already if (route.params.reportID) { const reportActionID = route?.params?.reportActionID; - const regexValidReportActionID = new RegExp(/^\d*$/); - if (reportActionID && !regexValidReportActionID.test(reportActionID)) { + const isValidReportActionID = ValidationUtils.isNumeric(reportActionID); + if (reportActionID && !isValidReportActionID) { navigation.setParams({reportActionID: ''}); } return;