From d7ee57c8eb1ba5bd5686d0dc6af9a05eb812d648 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Thu, 27 Jul 2023 09:26:18 +0200 Subject: [PATCH 1/9] memoize scrollToBottom function --- src/hooks/useReportScrollManager/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/hooks/useReportScrollManager/index.js b/src/hooks/useReportScrollManager/index.js index 13e7471f6eb1..0cf09146553c 100644 --- a/src/hooks/useReportScrollManager/index.js +++ b/src/hooks/useReportScrollManager/index.js @@ -1,4 +1,4 @@ -import {useContext} from 'react'; +import {useContext, useCallback} from 'react'; import ReportScreenContext from '../../pages/home/ReportScreenContext'; function useReportScrollManager() { @@ -22,13 +22,13 @@ function useReportScrollManager() { /** * Scroll to the bottom of the flatlist. */ - const scrollToBottom = () => { + const scrollToBottom = useCallback(() => { if (!flatListRef.current) { return; } flatListRef.current.scrollToOffset({animated: false, offset: 0}); - }; + }, [flatListRef]); return {ref: flatListRef, scrollToIndex, scrollToBottom}; } From 39a6e6a4eed5581b5ff93ee80079ac2455e2936d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Thu, 27 Jul 2023 09:26:28 +0200 Subject: [PATCH 2/9] add dependencies to useEffect --- src/pages/home/report/ReportActionsView.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 22d2fcf08655..f0c8f01eeb28 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -123,6 +123,7 @@ function ReportActionsView(props) { // eslint-disable-next-line react-hooks/exhaustive-deps }, []); + const scrollToBottom = reportScrollManager.scrollToBottom; useEffect(() => { // This callback is triggered when a new action arrives via Pusher and the event is emitted from Report.js. This allows us to maintain // a single source of truth for the "new action" event instead of trying to derive that a new action has appeared from looking at props. @@ -132,7 +133,7 @@ function ReportActionsView(props) { // If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where // they are now in the list. if (isFromCurrentUser) { - reportScrollManager.scrollToBottom(); + scrollToBottom(); // If the current user sends a new message in the chat we clear the new marker since they have "read" the report setNewMarkerReportActionID(''); } else if (isReportFullyVisible) { @@ -159,8 +160,7 @@ function ReportActionsView(props) { Report.unsubscribeFromReportChannel(props.report.reportID); }; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [isReportFullyVisible, newMarkerReportActionID, props.report.reportID, scrollToBottom]); useEffect(() => { const prevNetwork = prevNetworkRef.current; From 5917a4f25692064fcae8bedd7afda5bbc51a0455 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Thu, 27 Jul 2023 11:01:12 +0200 Subject: [PATCH 3/9] fix isFocused --- src/pages/home/report/ReportActionsView.js | 58 +++++++++++++--------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index f0c8f01eeb28..65f11903b05b 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -1,8 +1,9 @@ -import React, {useRef, useState, useEffect, useContext, useMemo} from 'react'; +import React, {useRef, useState, useEffect, useContext, useMemo, useCallback} from 'react'; import PropTypes from 'prop-types'; import _ from 'underscore'; import lodashGet from 'lodash/get'; import lodashCloneDeep from 'lodash/cloneDeep'; +import {useIsFocused} from '@react-navigation/native'; import * as Report from '../../../libs/actions/Report'; import reportActionPropTypes from './reportActionPropTypes'; import Visibility from '../../../libs/Visibility'; @@ -21,7 +22,6 @@ import ReportActionsList from './ReportActionsList'; import * as ReportActionsUtils from '../../../libs/ReportActionsUtils'; import * as ReportUtils from '../../../libs/ReportUtils'; import reportPropTypes from '../../reportPropTypes'; -import withNavigationFocus from '../../../components/withNavigationFocus'; import PopoverReactionList from './ReactionList/PopoverReactionList'; import getIsReportFullyVisible from '../../../libs/getIsReportFullyVisible'; import ReportScreenContext from '../ReportScreenContext'; @@ -63,30 +63,38 @@ function ReportActionsView(props) { useCopySelectionHelper(); - const reportScrollManager = useReportScrollManager(); + const {scrollToBottom} = useReportScrollManager(); const didLayout = useRef(false); const didSubscribeToReportTypingEvents = useRef(false); - const unsubscribeVisibilityListener = useRef(null); const hasCachedActions = useRef(_.size(props.reportActions) > 0); const [isFloatingMessageCounterVisible, setIsFloatingMessageCounterVisible] = useState(false); - const [newMarkerReportActionID, setNewMarkerReportActionID] = useState(ReportUtils.getNewMarkerReportActionID(props.report, props.reportActions)); + + // We use the newMarkerReport ID in a network subscription, we don't want to constantly re-create + // the subscription (as we want to avoid loosing events), so we use a ref to store the value in addition. + // As the value is also needed for UI updates, we also store it in state. + const [newMarkerReportActionID, _setNewMarkerReportActionID] = useState(ReportUtils.getNewMarkerReportActionID(props.report, props.reportActions)); + const newMarkerReportActionIDRef = useRef(newMarkerReportActionID); + const setNewMarkerReportActionID = useCallback((value) => { + newMarkerReportActionIDRef.current = value; + _setNewMarkerReportActionID(value); + }, []); const currentScrollOffset = useRef(0); const mostRecentIOUReportActionID = useRef(ReportActionsUtils.getMostRecentIOURequestActionID(props.reportActions)); - const unsubscribeFromNewActionEvent = useRef(null); - const prevReportActionsRef = useRef(props.reportActions); const prevReportRef = useRef(props.report); const prevNetworkRef = useRef(props.network); const prevIsSmallScreenWidthRef = useRef(props.isSmallScreenWidth); + const isFocused = useIsFocused(); + /** * @returns {Boolean} */ - const isReportFullyVisible = useMemo(() => getIsReportFullyVisible(props.isFocused), [props.isFocused]); + const isReportFullyVisible = useMemo(() => getIsReportFullyVisible(isFocused), [isFocused]); const openReportIfNecessary = () => { // If the report is optimistic (AKA not yet created) we don't need to call openReport again @@ -98,7 +106,8 @@ function ReportActionsView(props) { }; useEffect(() => { - unsubscribeVisibilityListener.current = Visibility.onVisibilityChange(() => { + console.debug('[Hanno] ReportActionsView: ', props.report.reportID, {isReportFullyVisible, isFocused}); + const unsubscribeVisibilityListener = Visibility.onVisibilityChange(() => { if (!isReportFullyVisible) { return; } @@ -110,25 +119,24 @@ function ReportActionsView(props) { } }); return () => { - if (!unsubscribeVisibilityListener.current) { + if (!unsubscribeVisibilityListener) { return; } - unsubscribeVisibilityListener.current(); + unsubscribeVisibilityListener(); }; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [isReportFullyVisible, isFocused, props.report, props.report.reportID, setNewMarkerReportActionID]); useEffect(() => { openReportIfNecessary(); // eslint-disable-next-line react-hooks/exhaustive-deps }, []); - const scrollToBottom = reportScrollManager.scrollToBottom; useEffect(() => { // This callback is triggered when a new action arrives via Pusher and the event is emitted from Report.js. This allows us to maintain // a single source of truth for the "new action" event instead of trying to derive that a new action has appeared from looking at props. - unsubscribeFromNewActionEvent.current = Report.subscribeToNewActionEvent(props.report.reportID, (isFromCurrentUser, newActionID) => { - const isNewMarkerReportActionIDSet = !_.isEmpty(newMarkerReportActionID); + console.debug('[Hanno] ReportActionsView: subscribe to id', props.report.reportID, {isReportFullyVisible}); + const unsubscribeFromNewActionEvent = Report.subscribeToNewActionEvent(props.report.reportID, (isFromCurrentUser, newActionID) => { + const isNewMarkerReportActionIDSet = !_.isEmpty(newMarkerReportActionIDRef.current); // If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where // they are now in the list. @@ -154,13 +162,15 @@ function ReportActionsView(props) { }); return () => { - if (unsubscribeFromNewActionEvent.current) { - unsubscribeFromNewActionEvent.current(); + console.debug('[Hanno] ReportActionsView unmount with id', props.report.reportID); + if (unsubscribeFromNewActionEvent) { + console.debug('[Hanno] ReportActionsView: unsubscribe from id', props.report.reportID); + unsubscribeFromNewActionEvent(); } Report.unsubscribeFromReportChannel(props.report.reportID); }; - }, [isReportFullyVisible, newMarkerReportActionID, props.report.reportID, scrollToBottom]); + }, [isReportFullyVisible, props.report.reportID, scrollToBottom, setNewMarkerReportActionID]); useEffect(() => { const prevNetwork = prevNetworkRef.current; @@ -203,7 +213,7 @@ function ReportActionsView(props) { setNewMarkerReportActionID(ReportUtils.getNewMarkerReportActionID(props.report, props.reportActions)); prevReportActionsRef.current = props.reportActions; - }, [props.report, props.reportActions]); + }, [props.report, props.reportActions, setNewMarkerReportActionID]); useEffect(() => { // If the last unread message was deleted, remove the *New* green marker and the *New Messages* notification at scroll just as the deletion starts. @@ -222,7 +232,7 @@ function ReportActionsView(props) { if (newMarkerReportActionID !== ReportUtils.getNewMarkerReportActionID(props.report, reportActionsWithoutPendingOne)) { setNewMarkerReportActionID(ReportUtils.getNewMarkerReportActionID(props.report, reportActionsWithoutPendingOne)); } - }, [props.report, props.reportActions, props.network, newMarkerReportActionID]); + }, [props.report, props.reportActions, props.network, newMarkerReportActionID, setNewMarkerReportActionID]); useEffect(() => { const prevReport = prevReportRef.current; @@ -234,7 +244,7 @@ function ReportActionsView(props) { } // update ref with current report prevReportRef.current = props.report; - }, [props.report, props.reportActions]); + }, [props.report, props.reportActions, setNewMarkerReportActionID]); useEffect(() => { // Ensures subscription event succeeds when the report/workspace room is created optimistically. @@ -270,7 +280,7 @@ function ReportActionsView(props) { }; const scrollToBottomAndMarkReportAsRead = () => { - reportScrollManager.scrollToBottom(); + scrollToBottom(); Report.readNewestAction(props.report.reportID); }; @@ -413,4 +423,4 @@ function arePropsEqual(oldProps, newProps) { const MemoizedReportActionsView = React.memo(ReportActionsView, arePropsEqual); -export default compose(Performance.withRenderTrace({id: ' rendering'}), withWindowDimensions, withNavigationFocus, withLocalize, withNetwork())(MemoizedReportActionsView); +export default compose(Performance.withRenderTrace({id: ' rendering'}), withWindowDimensions, withLocalize, withNetwork())(MemoizedReportActionsView); From df2fc05d32ee1b7ba715f102337789067deb6a48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Thu, 27 Jul 2023 13:25:19 +0200 Subject: [PATCH 4/9] fix issue with conflicting subscriptions --- src/pages/home/report/ReportActionsView.js | 56 ++++++++++++++-------- 1 file changed, 37 insertions(+), 19 deletions(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 65f11903b05b..5f039d54a2ab 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -58,8 +58,15 @@ const defaultProps = { policy: null, }; +// In the component we are subscribing to the arrvical of new actions. +// As there is the possibility that there are multiple instances of a ReportScreen +// for the same report, we only ever want one subscription to be active, as +// the subscriptions could otherwise be conflicting. +const newActionUnsubscribeMap = {}; + function ReportActionsView(props) { const context = useContext(ReportScreenContext); + const componentInstanceId = useRef(_.uniqueId('reportActionsView_')).current; useCopySelectionHelper(); @@ -90,6 +97,7 @@ function ReportActionsView(props) { const prevIsSmallScreenWidthRef = useRef(props.isSmallScreenWidth); const isFocused = useIsFocused(); + const reportID = props.report.reportID; /** * @returns {Boolean} @@ -102,11 +110,10 @@ function ReportActionsView(props) { return; } - Report.openReport(props.report.reportID); + Report.openReport(reportID); }; useEffect(() => { - console.debug('[Hanno] ReportActionsView: ', props.report.reportID, {isReportFullyVisible, isFocused}); const unsubscribeVisibilityListener = Visibility.onVisibilityChange(() => { if (!isReportFullyVisible) { return; @@ -124,7 +131,7 @@ function ReportActionsView(props) { } unsubscribeVisibilityListener(); }; - }, [isReportFullyVisible, isFocused, props.report, props.report.reportID, setNewMarkerReportActionID]); + }, [isReportFullyVisible, isFocused, props.report, reportID, setNewMarkerReportActionID]); useEffect(() => { openReportIfNecessary(); @@ -132,10 +139,19 @@ function ReportActionsView(props) { }, []); useEffect(() => { + // Why ware we doing this, when in the cleanup of the useEffect we are already calling the unsubscribe function? + // Answer: On web, when navigating to another report screen, the previous report screen doesn't get unmounted, + // meaning that the cleanup might not get called. When we then open a report we had open again, a new + // ReportScreen will get created. Thus, we have to cancel the earlier subscription of the previous screen, + // because the two subscriptions could conflict! + const previousSubUnsubscribe = newActionUnsubscribeMap[reportID]; + if (previousSubUnsubscribe) { + previousSubUnsubscribe(); + } + // This callback is triggered when a new action arrives via Pusher and the event is emitted from Report.js. This allows us to maintain // a single source of truth for the "new action" event instead of trying to derive that a new action has appeared from looking at props. - console.debug('[Hanno] ReportActionsView: subscribe to id', props.report.reportID, {isReportFullyVisible}); - const unsubscribeFromNewActionEvent = Report.subscribeToNewActionEvent(props.report.reportID, (isFromCurrentUser, newActionID) => { + const unsubscribe = Report.subscribeToNewActionEvent(reportID, (isFromCurrentUser, newActionID) => { const isNewMarkerReportActionIDSet = !_.isEmpty(newMarkerReportActionIDRef.current); // If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where @@ -149,7 +165,7 @@ function ReportActionsView(props) { // If the user is scrolled up and no new line marker is set we will set it otherwise we will do nothing so the new marker // stays in it's previous position. if (currentScrollOffset.current === 0) { - Report.readNewestAction(props.report.reportID); + Report.readNewestAction(reportID); setNewMarkerReportActionID(''); } else if (!isNewMarkerReportActionIDSet) { // The report is not in view and we received a comment from another user while the new marker is not set @@ -160,17 +176,19 @@ function ReportActionsView(props) { setNewMarkerReportActionID(newActionID); } }); - - return () => { - console.debug('[Hanno] ReportActionsView unmount with id', props.report.reportID); - if (unsubscribeFromNewActionEvent) { - console.debug('[Hanno] ReportActionsView: unsubscribe from id', props.report.reportID); - unsubscribeFromNewActionEvent(); + const cleanup = () => { + if (unsubscribe) { + unsubscribe(); } + Report.unsubscribeFromReportChannel(reportID); + }; - Report.unsubscribeFromReportChannel(props.report.reportID); + newActionUnsubscribeMap[reportID] = cleanup; + + return () => { + cleanup(); }; - }, [isReportFullyVisible, props.report.reportID, scrollToBottom, setNewMarkerReportActionID]); + }, [componentInstanceId, isReportFullyVisible, reportID, scrollToBottom, setNewMarkerReportActionID]); useEffect(() => { const prevNetwork = prevNetworkRef.current; @@ -182,7 +200,7 @@ function ReportActionsView(props) { if (isReportFullyVisible) { openReportIfNecessary(); } else { - Report.reconnect(props.report.reportID); + Report.reconnect(reportID); } } // update ref with current network state @@ -253,10 +271,10 @@ function ReportActionsView(props) { // Existing reports created will have empty fields for `pendingFields`. const didCreateReportSuccessfully = !props.report.pendingFields || (!props.report.pendingFields.addWorkspaceRoom && !props.report.pendingFields.createChat); if (!didSubscribeToReportTypingEvents.current && didCreateReportSuccessfully) { - Report.subscribeToReportTypingEvents(props.report.reportID); + Report.subscribeToReportTypingEvents(reportID); didSubscribeToReportTypingEvents.current = true; } - }, [props.report, didSubscribeToReportTypingEvents]); + }, [props.report, didSubscribeToReportTypingEvents, reportID]); /** * Retrieves the next set of report actions for the chat once we are nearing the end of what we are currently @@ -276,12 +294,12 @@ function ReportActionsView(props) { } // Retrieve the next REPORT.ACTIONS.LIMIT sized page of comments - Report.readOldestAction(props.report.reportID, oldestReportAction.reportActionID); + Report.readOldestAction(reportID, oldestReportAction.reportActionID); }; const scrollToBottomAndMarkReportAsRead = () => { scrollToBottom(); - Report.readNewestAction(props.report.reportID); + Report.readNewestAction(reportID); }; /** From 3cbc752d3b49c5e45bed8f8e5b3587b30d0e6d06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Thu, 27 Jul 2023 13:54:24 +0200 Subject: [PATCH 5/9] fix comment --- src/pages/home/report/ReportActionsView.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 5f039d54a2ab..b03641c0caa3 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -58,7 +58,7 @@ const defaultProps = { policy: null, }; -// In the component we are subscribing to the arrvical of new actions. +// In the component we are subscribing to the arrival of new actions. // As there is the possibility that there are multiple instances of a ReportScreen // for the same report, we only ever want one subscription to be active, as // the subscriptions could otherwise be conflicting. From 6ad9a837b1ee38779d96f4c1bae805e5266bdadb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Thu, 27 Jul 2023 13:55:19 +0200 Subject: [PATCH 6/9] remove reportId from deps --- src/pages/home/report/ReportActionsView.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index b03641c0caa3..923c00cc7eec 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -131,7 +131,7 @@ function ReportActionsView(props) { } unsubscribeVisibilityListener(); }; - }, [isReportFullyVisible, isFocused, props.report, reportID, setNewMarkerReportActionID]); + }, [isReportFullyVisible, isFocused, props.report, setNewMarkerReportActionID]); useEffect(() => { openReportIfNecessary(); From 890c01da614f20b4454bd312e823dcf33e0c7b9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Thu, 27 Jul 2023 14:04:24 +0200 Subject: [PATCH 7/9] fix comment --- src/pages/home/report/ReportActionsView.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 923c00cc7eec..f6150d435083 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -139,7 +139,7 @@ function ReportActionsView(props) { }, []); useEffect(() => { - // Why ware we doing this, when in the cleanup of the useEffect we are already calling the unsubscribe function? + // Why are we doing this, when in the cleanup of the useEffect we are already calling the unsubscribe function? // Answer: On web, when navigating to another report screen, the previous report screen doesn't get unmounted, // meaning that the cleanup might not get called. When we then open a report we had open again, a new // ReportScreen will get created. Thus, we have to cancel the earlier subscription of the previous screen, From 5a2656c7a5e640bf8831397aecf27cbeefed9b5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Thu, 27 Jul 2023 14:06:19 +0200 Subject: [PATCH 8/9] fix comment --- src/pages/home/report/ReportActionsView.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index f6150d435083..bc1bfc742261 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -141,9 +141,11 @@ function ReportActionsView(props) { useEffect(() => { // Why are we doing this, when in the cleanup of the useEffect we are already calling the unsubscribe function? // Answer: On web, when navigating to another report screen, the previous report screen doesn't get unmounted, - // meaning that the cleanup might not get called. When we then open a report we had open again, a new + // meaning that the cleanup might not get called. When we then open a report we had open already previosuly, a new // ReportScreen will get created. Thus, we have to cancel the earlier subscription of the previous screen, // because the two subscriptions could conflict! + // In case we return to the previous screen (e.g. by web back navigation) the useEffect for that screen would + // fire again, as the focus has changed and will set up the subscription correctly again. const previousSubUnsubscribe = newActionUnsubscribeMap[reportID]; if (previousSubUnsubscribe) { previousSubUnsubscribe(); From 8967d168a978829df851b2b1fcfc5698632aa3ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Thu, 27 Jul 2023 14:15:20 +0200 Subject: [PATCH 9/9] remove debug code --- src/pages/home/report/ReportActionsView.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index bc1bfc742261..5005b70fb633 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -66,7 +66,6 @@ const newActionUnsubscribeMap = {}; function ReportActionsView(props) { const context = useContext(ReportScreenContext); - const componentInstanceId = useRef(_.uniqueId('reportActionsView_')).current; useCopySelectionHelper(); @@ -190,7 +189,7 @@ function ReportActionsView(props) { return () => { cleanup(); }; - }, [componentInstanceId, isReportFullyVisible, reportID, scrollToBottom, setNewMarkerReportActionID]); + }, [isReportFullyVisible, reportID, scrollToBottom, setNewMarkerReportActionID]); useEffect(() => { const prevNetwork = prevNetworkRef.current;