From f1313cb982b70bd76bb0907d0f7f10a217d20932 Mon Sep 17 00:00:00 2001 From: Andrew Rosiclair Date: Wed, 21 Jun 2023 15:26:29 -0400 Subject: [PATCH 1/6] wait for the navigation container to be ready before navigating --- .../subscribeToReportCommentPushNotifications.js | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.js b/src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.js index 905397da1738..7c8caeb87466 100644 --- a/src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.js +++ b/src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.js @@ -1,6 +1,4 @@ -import {Linking} from 'react-native'; import Onyx from 'react-native-onyx'; -import CONST from '../../../CONST'; import PushNotification from '.'; import ROUTES from '../../../ROUTES'; import Log from '../../Log'; @@ -17,16 +15,13 @@ export default function subscribeToReportCommentPushNotifications() { // Open correct report when push notification is clicked PushNotification.onSelected(PushNotification.TYPE.REPORT_COMMENT, ({reportID}) => { - if (Navigation.canNavigate('navigate')) { + Navigation.isNavigationReady().then(() => { // If a chat is visible other than the one we are trying to navigate to, then we need to navigate back if (Navigation.getActiveRoute().slice(1, 2) === ROUTES.REPORT && !Navigation.isActiveRoute(`r/${reportID}`)) { Navigation.goBack(); } + Navigation.navigate(ROUTES.getReportRoute(reportID)); - } else { - // Navigation container is not yet ready, use deeplinking to open to correct report instead - Navigation.setDidTapNotification(); - Linking.openURL(`${CONST.DEEPLINK_BASE_URL}${ROUTES.getReportRoute(reportID)}`); - } + }); }); } From 9e7036a3d696d352230cf183e510b741e3fcbbc3 Mon Sep 17 00:00:00 2001 From: Andrew Rosiclair Date: Wed, 21 Jun 2023 15:32:49 -0400 Subject: [PATCH 2/6] fix navigation ready callback by using onStateChange --- src/Expensify.js | 8 +------- src/libs/Navigation/NavigationRoot.js | 12 +++++++++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Expensify.js b/src/Expensify.js index e7c830ff2029..c774371dfcfd 100644 --- a/src/Expensify.js +++ b/src/Expensify.js @@ -23,7 +23,6 @@ import compose from './libs/compose'; import withLocalize, {withLocalizePropTypes} from './components/withLocalize'; import * as User from './libs/actions/User'; import NetworkConnection from './libs/NetworkConnection'; -import Navigation from './libs/Navigation/Navigation'; import DeeplinkWrapper from './components/DeeplinkWrapper'; import PopoverReportActionContextMenu from './pages/home/report/ContextMenu/PopoverReportActionContextMenu'; import * as ReportActionContextMenu from './pages/home/report/ContextMenu/ReportActionContextMenu'; @@ -111,12 +110,7 @@ function Expensify(props) { ActiveClientManager.init(); }; - const setNavigationReady = useCallback(() => { - setIsNavigationReady(true); - - // Navigate to any pending routes now that the NavigationContainer is ready - Navigation.setIsNavigationReady(); - }, []); + const setNavigationReady = useCallback(() => setIsNavigationReady(true), []); const onSplashHide = useCallback(() => { setIsSplashHidden(true); diff --git a/src/libs/Navigation/NavigationRoot.js b/src/libs/Navigation/NavigationRoot.js index 4f32178ba5bd..b8b82706a4c4 100644 --- a/src/libs/Navigation/NavigationRoot.js +++ b/src/libs/Navigation/NavigationRoot.js @@ -48,18 +48,25 @@ function parseAndLogRoute(state) { } else { Log.info('Navigating to route', false, {path: currentPath}); } - - Navigation.setIsNavigationReady(); } function NavigationRoot(props) { useFlipper(navigationRef); const navigationStateRef = useRef(undefined); + const onReadyCalled = useRef(false); const updateSavedNavigationStateAndLogRoute = (state) => { navigationStateRef.current = state; props.updateCurrentReportId(state); parseAndLogRoute(state); + + // NavigationContainer's onReady callback is unreliable (it will fire but navigationRef.isReady() will still be false) + // So we use this onStateChange callback instead + if (!onReadyCalled.current) { + Navigation.setIsNavigationReady(); + props.onReady(); + onReadyCalled.current = true; + } }; return ( @@ -67,7 +74,6 @@ function NavigationRoot(props) { key={props.isSmallScreenWidth ? 'small' : 'big'} onStateChange={updateSavedNavigationStateAndLogRoute} initialState={navigationStateRef.current} - onReady={props.onReady} theme={navigationTheme} ref={navigationRef} linking={linkingConfig} From 73005a44e47449b1026e4b28fb33fe71780dd6a9 Mon Sep 17 00:00:00 2001 From: Andrew Rosiclair Date: Wed, 21 Jun 2023 15:53:26 -0400 Subject: [PATCH 3/6] improve logging for notification handling --- .../subscribeToReportCommentPushNotifications.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.js b/src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.js index 7c8caeb87466..d6ba39095ce7 100644 --- a/src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.js +++ b/src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.js @@ -8,19 +8,25 @@ import Navigation from '../../Navigation/Navigation'; * Setup reportComment push notification callbacks. */ export default function subscribeToReportCommentPushNotifications() { - PushNotification.onReceived(PushNotification.TYPE.REPORT_COMMENT, ({reportID, onyxData}) => { - Log.info('[Report] Handled event sent by Airship', false, {reportID}); + PushNotification.onReceived(PushNotification.TYPE.REPORT_COMMENT, ({reportID, reportActionID, onyxData}) => { + Log.info('[PushNotification] Handled event sent by Airship', false, {reportID, reportActionID}); Onyx.update(onyxData); }); // Open correct report when push notification is clicked - PushNotification.onSelected(PushNotification.TYPE.REPORT_COMMENT, ({reportID}) => { + PushNotification.onSelected(PushNotification.TYPE.REPORT_COMMENT, ({reportID, reportActionID}) => { + if (!reportID) { + Log.warn('[PushNotification] This push notification has no reportID'); + } + + Log.info('[PushNotification] onSelected() - called', false, {reportID, reportActionID}); Navigation.isNavigationReady().then(() => { // If a chat is visible other than the one we are trying to navigate to, then we need to navigate back if (Navigation.getActiveRoute().slice(1, 2) === ROUTES.REPORT && !Navigation.isActiveRoute(`r/${reportID}`)) { Navigation.goBack(); } + Log.info('[PushNotification] onSelected() - Navigation is ready. Navigating...', false, {reportID, reportActionID}); Navigation.navigate(ROUTES.getReportRoute(reportID)); }); }); From 5d2930b3732004166004414af19466cd8307d0ff Mon Sep 17 00:00:00 2001 From: Andrew Rosiclair Date: Fri, 23 Jun 2023 12:08:49 -0400 Subject: [PATCH 4/6] Revert "fix navigation ready callback by using onStateChange" This reverts commit 9e7036a3d696d352230cf183e510b741e3fcbbc3. --- src/Expensify.js | 8 +++++++- src/libs/Navigation/NavigationRoot.js | 12 +++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Expensify.js b/src/Expensify.js index c774371dfcfd..e7c830ff2029 100644 --- a/src/Expensify.js +++ b/src/Expensify.js @@ -23,6 +23,7 @@ import compose from './libs/compose'; import withLocalize, {withLocalizePropTypes} from './components/withLocalize'; import * as User from './libs/actions/User'; import NetworkConnection from './libs/NetworkConnection'; +import Navigation from './libs/Navigation/Navigation'; import DeeplinkWrapper from './components/DeeplinkWrapper'; import PopoverReportActionContextMenu from './pages/home/report/ContextMenu/PopoverReportActionContextMenu'; import * as ReportActionContextMenu from './pages/home/report/ContextMenu/ReportActionContextMenu'; @@ -110,7 +111,12 @@ function Expensify(props) { ActiveClientManager.init(); }; - const setNavigationReady = useCallback(() => setIsNavigationReady(true), []); + const setNavigationReady = useCallback(() => { + setIsNavigationReady(true); + + // Navigate to any pending routes now that the NavigationContainer is ready + Navigation.setIsNavigationReady(); + }, []); const onSplashHide = useCallback(() => { setIsSplashHidden(true); diff --git a/src/libs/Navigation/NavigationRoot.js b/src/libs/Navigation/NavigationRoot.js index 98c73564c04c..ee389e530d0f 100644 --- a/src/libs/Navigation/NavigationRoot.js +++ b/src/libs/Navigation/NavigationRoot.js @@ -48,12 +48,13 @@ function parseAndLogRoute(state) { } else { Log.info('Navigating to route', false, {path: currentPath}); } + + Navigation.setIsNavigationReady(); } function NavigationRoot(props) { useFlipper(navigationRef); const navigationStateRef = useRef(undefined); - const onReadyCalled = useRef(false); const updateSavedNavigationStateAndLogRoute = (state) => { if (!state) { @@ -62,14 +63,6 @@ function NavigationRoot(props) { navigationStateRef.current = state; props.updateCurrentReportId(state); parseAndLogRoute(state); - - // NavigationContainer's onReady callback is unreliable (it will fire but navigationRef.isReady() will still be false) - // So we use this onStateChange callback instead - if (!onReadyCalled.current) { - Navigation.setIsNavigationReady(); - props.onReady(); - onReadyCalled.current = true; - } }; return ( @@ -77,6 +70,7 @@ function NavigationRoot(props) { key={props.isSmallScreenWidth ? 'small' : 'big'} onStateChange={updateSavedNavigationStateAndLogRoute} initialState={navigationStateRef.current} + onReady={props.onReady} theme={navigationTheme} ref={navigationRef} linking={linkingConfig} From 66aba8da50f449b26c0a3aabce99159bad0301a2 Mon Sep 17 00:00:00 2001 From: Andrew Rosiclair Date: Fri, 23 Jun 2023 15:30:01 -0400 Subject: [PATCH 5/6] safely get current route name to fix error --- src/libs/Navigation/Navigation.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libs/Navigation/Navigation.js b/src/libs/Navigation/Navigation.js index 167f2d9eea96..c4e25b6288c8 100644 --- a/src/libs/Navigation/Navigation.js +++ b/src/libs/Navigation/Navigation.js @@ -148,7 +148,8 @@ function dismissModal(targetReportID) { * @returns {String} */ function getActiveRoute() { - const currentRouteHasName = navigationRef.current && navigationRef.current.getCurrentRoute().name; + const currentRoute = navigationRef.current && navigationRef.current.getCurrentRoute(); + const currentRouteHasName = lodashGet(currentRoute, 'name', false); if (!currentRouteHasName) { return ''; } From b103132e38c0a52e862e3ae51018e38a2c7c067f Mon Sep 17 00:00:00 2001 From: Andrew Rosiclair Date: Fri, 23 Jun 2023 15:42:00 -0400 Subject: [PATCH 6/6] catch and log navigation errors --- .../subscribeToReportCommentPushNotifications.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.js b/src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.js index d6ba39095ce7..5c3d37ff9c92 100644 --- a/src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.js +++ b/src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.js @@ -21,13 +21,17 @@ export default function subscribeToReportCommentPushNotifications() { Log.info('[PushNotification] onSelected() - called', false, {reportID, reportActionID}); Navigation.isNavigationReady().then(() => { - // If a chat is visible other than the one we are trying to navigate to, then we need to navigate back - if (Navigation.getActiveRoute().slice(1, 2) === ROUTES.REPORT && !Navigation.isActiveRoute(`r/${reportID}`)) { - Navigation.goBack(); - } + try { + // If a chat is visible other than the one we are trying to navigate to, then we need to navigate back + if (Navigation.getActiveRoute().slice(1, 2) === ROUTES.REPORT && !Navigation.isActiveRoute(`r/${reportID}`)) { + Navigation.goBack(); + } - Log.info('[PushNotification] onSelected() - Navigation is ready. Navigating...', false, {reportID, reportActionID}); - Navigation.navigate(ROUTES.getReportRoute(reportID)); + Log.info('[PushNotification] onSelected() - Navigation is ready. Navigating...', false, {reportID, reportActionID}); + Navigation.navigate(ROUTES.getReportRoute(reportID)); + } catch (error) { + Log.alert('[PushNotification] onSelected() - failed', {reportID, reportActionID, error: error.message}); + } }); }); }