From 35a868738d997b14dd9edfd0594a04ed8fffe1cd Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Thu, 13 Jul 2023 09:18:40 +0700 Subject: [PATCH 1/7] Fix leaving a room from multiple devices --- src/pages/home/ReportScreen.js | 34 ++++++++++++++++++---- src/pages/home/report/ReportActionsView.js | 15 ++-------- 2 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/pages/home/ReportScreen.js b/src/pages/home/ReportScreen.js index 69d9467567bd..9042b8375a9f 100644 --- a/src/pages/home/ReportScreen.js +++ b/src/pages/home/ReportScreen.js @@ -132,6 +132,7 @@ class ReportScreen extends React.Component { this.state = { skeletonViewContainerHeight: reportActionsListViewHeight, isBannerVisible: true, + isReportRemoved: false, }; this.firstRenderRef = React.createRef(); this.firstRenderRef.current = reportActionsListViewHeight === 0; @@ -160,13 +161,24 @@ class ReportScreen extends React.Component { if (ReportUtils.shouldHideComposer(this.props.report, this.props.errors)) { EmojiPickerAction.hideEmojiPicker(true); } + const onyxReportID = this.props.report.reportID; + const prevOnyxReportID = prevProps.report.reportID; + const routeReportID = getReportID(this.props.route); + + // navigate to concierge when the room removed from another device (e.g. user leaving a room) + if (prevOnyxReportID && prevOnyxReportID === routeReportID && !onyxReportID) { + Navigation.goBack(); + Report.navigateToConciergeChat(); + // isReportRemoved will prevent showing when navigating + this.setState({isReportRemoved: true}); + return; + } + // If you already have a report open and are deeplinking to a new report on native, // the ReportScreen never actually unmounts and the reportID in the route also doesn't change. // Therefore, we need to compare if the existing reportID is the same as the one in the route // before deciding that we shouldn't call OpenReport. - const onyxReportID = this.props.report.reportID; - const routeReportID = getReportID(this.props.route); - if (onyxReportID === prevProps.report.reportID && (!onyxReportID || onyxReportID === routeReportID)) { + if (onyxReportID === prevOnyxReportID && (!onyxReportID || onyxReportID === routeReportID)) { return; } @@ -207,6 +219,17 @@ class ReportScreen extends React.Component { } fetchReportIfNeeded() { + // this function will also be called from child component (ReportActionsView), + // props might be undefined when it's called from a child element for the first time + if(!this.props){ + return; + } + + // If the report is optimistic (AKA not yet created) we don't need to call openReport again + if (this.props.report.isOptimisticReport) { + return; + } + const reportIDFromPath = getReportID(this.props.route); // Report ID will be empty when the reports collection is empty. @@ -218,7 +241,7 @@ class ReportScreen extends React.Component { // It possible that we may not have the report object yet in Onyx yet e.g. we navigated to a URL for an accessible report that // is not stored locally yet. If props.report.reportID exists, then the report has been stored locally and nothing more needs to be done. // If it doesn't exist, then we fetch the report from the API. - if (this.props.report.reportID && this.props.report.reportID === getReportID(this.props.route)) { + if (this.props.report.reportID && this.props.report.reportID === reportIDFromPath) { return; } @@ -266,7 +289,7 @@ class ReportScreen extends React.Component { shouldEnableKeyboardAvoidingView={this.props.isFocused} > )} diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 9c2099b55e75..194f5759963a 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -86,15 +86,6 @@ function ReportActionsView(props) { */ const isReportFullyVisible = useMemo(() => getIsReportFullyVisible(props.isFocused), [props.isFocused]); - const openReportIfNecessary = () => { - // If the report is optimistic (AKA not yet created) we don't need to call openReport again - if (props.report.isOptimisticReport) { - return; - } - - Report.openReport(props.report.reportID); - }; - useEffect(() => { unsubscribeVisibilityListener.current = Visibility.onVisibilityChange(() => { if (!isReportFullyVisible) { @@ -117,7 +108,7 @@ function ReportActionsView(props) { }, []); useEffect(() => { - openReportIfNecessary(); + props.fetchReportIfNeeded(); // eslint-disable-next-line react-hooks/exhaustive-deps }, []); @@ -168,7 +159,7 @@ function ReportActionsView(props) { const wasNetworkChangeDetected = lodashGet(prevNetwork, 'isOffline') && !lodashGet(props.network, 'isOffline'); if (wasNetworkChangeDetected) { if (isReportFullyVisible) { - openReportIfNecessary(); + props.fetchReportIfNeeded(); } else { Report.reconnect(props.report.reportID); } @@ -186,7 +177,7 @@ function ReportActionsView(props) { const didReportBecomeVisible = isReportFullyVisible && didScreenSizeIncrease; if (didReportBecomeVisible) { setNewMarkerReportActionID(ReportUtils.isUnread(props.report) ? ReportUtils.getNewMarkerReportActionID(props.report, props.reportActions) : ''); - openReportIfNecessary(); + props.fetchReportIfNeeded(); } // update ref with current state prevIsSmallScreenWidthRef.current = props.isSmallScreenWidth; From 6f8df887d905b7e985aceebbd799e1b6716a031d Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Thu, 13 Jul 2023 09:41:46 +0700 Subject: [PATCH 2/7] Add fetchReportIfNeeded propTypes --- src/pages/home/report/ReportActionsView.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 194f5759963a..a432749d250d 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -49,6 +49,9 @@ const propTypes = { avatar: PropTypes.string, }), + /** The function to re-fetch the report when required */ + fetchReportIfNeeded: PropTypes.func.isRequired, + ...windowDimensionsPropTypes, ...withLocalizePropTypes, }; From 6d88260ccbb7e77bce838d863c321d7649996bc8 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Thu, 13 Jul 2023 15:07:31 +0700 Subject: [PATCH 3/7] revert reportactionsview --- src/pages/home/report/ReportActionsView.js | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index a432749d250d..362e77f253d9 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -49,9 +49,6 @@ const propTypes = { avatar: PropTypes.string, }), - /** The function to re-fetch the report when required */ - fetchReportIfNeeded: PropTypes.func.isRequired, - ...windowDimensionsPropTypes, ...withLocalizePropTypes, }; @@ -89,6 +86,15 @@ function ReportActionsView(props) { */ const isReportFullyVisible = useMemo(() => getIsReportFullyVisible(props.isFocused), [props.isFocused]); + const openReportIfNecessary = () => { + // If the report is optimistic (AKA not yet created) we don't need to call openReport again + if (props.report.isOptimisticReport) { + return; + } + + Report.openReport(props.report.reportID); + }; + useEffect(() => { unsubscribeVisibilityListener.current = Visibility.onVisibilityChange(() => { if (!isReportFullyVisible) { @@ -111,7 +117,7 @@ function ReportActionsView(props) { }, []); useEffect(() => { - props.fetchReportIfNeeded(); + openReportIfNecessary(); // eslint-disable-next-line react-hooks/exhaustive-deps }, []); @@ -162,7 +168,7 @@ function ReportActionsView(props) { const wasNetworkChangeDetected = lodashGet(prevNetwork, 'isOffline') && !lodashGet(props.network, 'isOffline'); if (wasNetworkChangeDetected) { if (isReportFullyVisible) { - props.fetchReportIfNeeded(); + openReportIfNecessary(); } else { Report.reconnect(props.report.reportID); } @@ -180,7 +186,7 @@ function ReportActionsView(props) { const didReportBecomeVisible = isReportFullyVisible && didScreenSizeIncrease; if (didReportBecomeVisible) { setNewMarkerReportActionID(ReportUtils.isUnread(props.report) ? ReportUtils.getNewMarkerReportActionID(props.report, props.reportActions) : ''); - props.fetchReportIfNeeded(); + openReportIfNecessary(); } // update ref with current state prevIsSmallScreenWidthRef.current = props.isSmallScreenWidth; @@ -409,4 +415,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, withNavigationFocus, withLocalize, withNetwork())(MemoizedReportActionsView); \ No newline at end of file From c9bdef2c82e0ee8ac7aeb9b0b292ad1a772d3a9e Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Thu, 13 Jul 2023 15:11:17 +0700 Subject: [PATCH 4/7] refine report removed logic, remove unnecessary props pass --- src/pages/home/ReportScreen.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pages/home/ReportScreen.js b/src/pages/home/ReportScreen.js index 9042b8375a9f..5cdd1c316c92 100644 --- a/src/pages/home/ReportScreen.js +++ b/src/pages/home/ReportScreen.js @@ -166,7 +166,8 @@ class ReportScreen extends React.Component { const routeReportID = getReportID(this.props.route); // navigate to concierge when the room removed from another device (e.g. user leaving a room) - if (prevOnyxReportID && prevOnyxReportID === routeReportID && !onyxReportID) { + // the report will not really null when removed, it will have defaultProps properties and values + if (prevOnyxReportID && prevOnyxReportID === routeReportID && !onyxReportID && _.isEqual(this.props.report, defaultProps.report)) { Navigation.goBack(); Report.navigateToConciergeChat(); // isReportRemoved will prevent showing when navigating @@ -363,7 +364,6 @@ class ReportScreen extends React.Component { isComposerFullSize={this.props.isComposerFullSize} parentViewHeight={this.state.skeletonViewContainerHeight} policy={policy} - fetchReportIfNeeded={this.fetchReportIfNeeded} /> )} From d808984c740b644e10d82c38d9816b0198611ddd Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Thu, 13 Jul 2023 15:16:20 +0700 Subject: [PATCH 5/7] revert fetchReportIfNeeded function --- src/pages/home/ReportScreen.js | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/pages/home/ReportScreen.js b/src/pages/home/ReportScreen.js index 5cdd1c316c92..f1bb53b1e618 100644 --- a/src/pages/home/ReportScreen.js +++ b/src/pages/home/ReportScreen.js @@ -220,17 +220,6 @@ class ReportScreen extends React.Component { } fetchReportIfNeeded() { - // this function will also be called from child component (ReportActionsView), - // props might be undefined when it's called from a child element for the first time - if(!this.props){ - return; - } - - // If the report is optimistic (AKA not yet created) we don't need to call openReport again - if (this.props.report.isOptimisticReport) { - return; - } - const reportIDFromPath = getReportID(this.props.route); // Report ID will be empty when the reports collection is empty. @@ -242,7 +231,7 @@ class ReportScreen extends React.Component { // It possible that we may not have the report object yet in Onyx yet e.g. we navigated to a URL for an accessible report that // is not stored locally yet. If props.report.reportID exists, then the report has been stored locally and nothing more needs to be done. // If it doesn't exist, then we fetch the report from the API. - if (this.props.report.reportID && this.props.report.reportID === reportIDFromPath) { + if (this.props.report.reportID && this.props.report.reportID === getReportID(this.props.route)) { return; } From b78bb1803eb71b90d0867b3fecfa37b512874771 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Thu, 13 Jul 2023 15:43:34 +0700 Subject: [PATCH 6/7] Run Prettier --- 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 362e77f253d9..9c2099b55e75 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -415,4 +415,4 @@ function arePropsEqual(oldProps, newProps) { const MemoizedReportActionsView = React.memo(ReportActionsView, arePropsEqual); -export default compose(Performance.withRenderTrace({id: ' rendering'}), withWindowDimensions, withNavigationFocus, withLocalize, withNetwork())(MemoizedReportActionsView); \ No newline at end of file +export default compose(Performance.withRenderTrace({id: ' rendering'}), withWindowDimensions, withNavigationFocus, withLocalize, withNetwork())(MemoizedReportActionsView); From d77aed93d6023161f8caeb9f7aa3d95de96aa376 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Fri, 14 Jul 2023 01:42:08 +0700 Subject: [PATCH 7/7] Handle optimistic case --- src/pages/home/ReportScreen.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/pages/home/ReportScreen.js b/src/pages/home/ReportScreen.js index f1bb53b1e618..98f365e041b7 100644 --- a/src/pages/home/ReportScreen.js +++ b/src/pages/home/ReportScreen.js @@ -167,7 +167,15 @@ class ReportScreen extends React.Component { // navigate to concierge when the room removed from another device (e.g. user leaving a room) // the report will not really null when removed, it will have defaultProps properties and values - if (prevOnyxReportID && prevOnyxReportID === routeReportID && !onyxReportID && _.isEqual(this.props.report, defaultProps.report)) { + if ( + prevOnyxReportID && + prevOnyxReportID === routeReportID && + !onyxReportID && + // non-optimistic case + (_.isEqual(this.props.report, defaultProps.report) || + // optimistic case + (prevProps.report.statusNum === CONST.REPORT.STATUS.OPEN && this.props.report.statusNum === CONST.REPORT.STATUS.CLOSED)) + ) { Navigation.goBack(); Report.navigateToConciergeChat(); // isReportRemoved will prevent showing when navigating