From 3be5e6909a86a33a9c7c749c2378b14ad1ab243c Mon Sep 17 00:00:00 2001 From: Oscar Franco Date: Fri, 25 Aug 2023 16:36:54 +0200 Subject: [PATCH 1/3] Separate ReportScreenContext to prevent cyclic re-renders --- .../ReportActionItemEmojiReactions.js | 4 +- src/hooks/useReportScrollManager/index.js | 4 +- .../useReportScrollManager/index.native.js | 4 +- src/pages/home/ReportScreen.js | 188 +++++++++--------- src/pages/home/ReportScreenContext.js | 9 +- src/pages/home/report/ReportActionItem.js | 4 +- src/pages/home/report/ReportActionsView.js | 7 +- 7 files changed, 110 insertions(+), 110 deletions(-) diff --git a/src/components/Reactions/ReportActionItemEmojiReactions.js b/src/components/Reactions/ReportActionItemEmojiReactions.js index ec2755f1a5dd..4f590b43e180 100644 --- a/src/components/Reactions/ReportActionItemEmojiReactions.js +++ b/src/components/Reactions/ReportActionItemEmojiReactions.js @@ -13,7 +13,7 @@ import EmojiReactionsPropTypes from './EmojiReactionsPropTypes'; import Tooltip from '../Tooltip'; import ReactionTooltipContent from './ReactionTooltipContent'; import * as EmojiUtils from '../../libs/EmojiUtils'; -import ReportScreenContext from '../../pages/home/ReportScreenContext'; +import {ReactionListContext} from '../../pages/home/ReportScreenContext'; const propTypes = { emojiReactions: EmojiReactionsPropTypes, @@ -41,7 +41,7 @@ const defaultProps = { }; function ReportActionItemEmojiReactions(props) { - const {reactionListRef} = useContext(ReportScreenContext); + const reactionListRef = useContext(ReactionListContext); const popoverReactionListAnchor = useRef(null); let totalReactionCount = 0; diff --git a/src/hooks/useReportScrollManager/index.js b/src/hooks/useReportScrollManager/index.js index 0cf09146553c..9a3303504b92 100644 --- a/src/hooks/useReportScrollManager/index.js +++ b/src/hooks/useReportScrollManager/index.js @@ -1,8 +1,8 @@ import {useContext, useCallback} from 'react'; -import ReportScreenContext from '../../pages/home/ReportScreenContext'; +import {ActionListContext} from '../../pages/home/ReportScreenContext'; function useReportScrollManager() { - const {flatListRef} = useContext(ReportScreenContext); + const flatListRef = useContext(ActionListContext); /** * Scroll to the provided index. On non-native implementations we do not want to scroll when we are scrolling because diff --git a/src/hooks/useReportScrollManager/index.native.js b/src/hooks/useReportScrollManager/index.native.js index 35af064cb062..d44a40222ca5 100644 --- a/src/hooks/useReportScrollManager/index.native.js +++ b/src/hooks/useReportScrollManager/index.native.js @@ -1,8 +1,8 @@ import {useContext, useCallback} from 'react'; -import ReportScreenContext from '../../pages/home/ReportScreenContext'; +import {ActionListContext} from '../../pages/home/ReportScreenContext'; function useReportScrollManager() { - const {flatListRef} = useContext(ReportScreenContext); + const flatListRef = useContext(ActionListContext); /** * Scroll to the provided index. diff --git a/src/pages/home/ReportScreen.js b/src/pages/home/ReportScreen.js index 83f0e4a6d506..16284c2bd9c5 100644 --- a/src/pages/home/ReportScreen.js +++ b/src/pages/home/ReportScreen.js @@ -33,7 +33,7 @@ import getIsReportFullyVisible from '../../libs/getIsReportFullyVisible'; import MoneyRequestHeader from '../../components/MoneyRequestHeader'; import MoneyReportHeader from '../../components/MoneyReportHeader'; import * as ComposerActions from '../../libs/actions/Composer'; -import ReportScreenContext from './ReportScreenContext'; +import {ActionListContext, ReactionListContext} from './ReportScreenContext'; import TaskHeaderActionButton from '../../components/TaskHeaderActionButton'; import DragAndDropProvider from '../../components/DragAndDrop/Provider'; @@ -276,111 +276,107 @@ class ReportScreen extends React.Component { } return ( - - - + + - - {headerView} - {ReportUtils.isTaskReport(this.props.report) && this.props.isSmallScreenWidth && ReportUtils.isOpenTaskReport(this.props.report) && ( - - - - + + {headerView} + {ReportUtils.isTaskReport(this.props.report) && this.props.isSmallScreenWidth && ReportUtils.isOpenTaskReport(this.props.report) && ( + + + + + - - )} - - {Boolean(this.props.accountManagerReportID) && ReportUtils.isConciergeChatReport(this.props.report) && this.state.isBannerVisible && ( - - )} - - { - // Rounding this value for comparison because they can look like this: 411.9999694824219 - const skeletonViewContainerHeight = Math.round(event.nativeEvent.layout.height); - - // Only set state when the height changes to avoid unnecessary renders - if (reportActionsListViewHeight === skeletonViewContainerHeight) return; - - // The height can be 0 if the component unmounts - we are not interested in this value and want to know how much space it - // takes up so we can set the skeleton view container height. - if (skeletonViewContainerHeight === 0) { - return; - } - reportActionsListViewHeight = skeletonViewContainerHeight; - this.setState({skeletonViewContainerHeight}); - }} - > - {this.isReportReadyForDisplay() && !isLoadingInitialReportActions && !isLoading && ( - )} - - {/* Note: The report should be allowed to mount even if the initial report actions are not loaded. If we prevent rendering the report while they are loading then - we'll unnecessarily unmount the ReportActionsView which will clear the new marker lines initial state. */} - {(!this.isReportReadyForDisplay() || isLoadingInitialReportActions || isLoading) && ( - - )} - - {this.isReportReadyForDisplay() && ( - <> - + {Boolean(this.props.accountManagerReportID) && ReportUtils.isConciergeChatReport(this.props.report) && this.state.isBannerVisible && ( + + )} + + { + // Rounding this value for comparison because they can look like this: 411.9999694824219 + const skeletonViewContainerHeight = Math.round(event.nativeEvent.layout.height); + + // Only set state when the height changes to avoid unnecessary renders + if (reportActionsListViewHeight === skeletonViewContainerHeight) return; + + // The height can be 0 if the component unmounts - we are not interested in this value and want to know how much space it + // takes up so we can set the skeleton view container height. + if (skeletonViewContainerHeight === 0) { + return; + } + reportActionsListViewHeight = skeletonViewContainerHeight; + this.setState({skeletonViewContainerHeight}); + }} + > + {this.isReportReadyForDisplay() && !isLoadingInitialReportActions && !isLoading && ( + - - )} + )} - {!this.isReportReadyForDisplay() && ( - - )} - - - - - + {/* Note: The report should be allowed to mount even if the initial report actions are not loaded. If we prevent rendering the report while they are loading then + we'll unnecessarily unmount the ReportActionsView which will clear the new marker lines initial state. */} + {(!this.isReportReadyForDisplay() || isLoadingInitialReportActions || isLoading) && ( + + )} + + {this.isReportReadyForDisplay() && ( + <> + + + )} + + {!this.isReportReadyForDisplay() && ( + + )} + + + + + + ); } } diff --git a/src/pages/home/ReportScreenContext.js b/src/pages/home/ReportScreenContext.js index 2f79d6ae9432..0be1882699f4 100644 --- a/src/pages/home/ReportScreenContext.js +++ b/src/pages/home/ReportScreenContext.js @@ -1,4 +1,9 @@ import {createContext} from 'react'; -const ReportScreenContext = createContext(); -export default ReportScreenContext; +const ActionListContext = createContext(); +const ReactionListContext = createContext(); + +export { + ActionListContext, + ReactionListContext +} \ No newline at end of file diff --git a/src/pages/home/report/ReportActionItem.js b/src/pages/home/report/ReportActionItem.js index e5b199d1c994..4a0c0d7bd3ba 100644 --- a/src/pages/home/report/ReportActionItem.js +++ b/src/pages/home/report/ReportActionItem.js @@ -64,7 +64,7 @@ import * as PersonalDetailsUtils from '../../../libs/PersonalDetailsUtils'; import ReportActionItemBasicMessage from './ReportActionItemBasicMessage'; import * as store from '../../../libs/actions/ReimbursementAccount/store'; import * as BankAccounts from '../../../libs/actions/BankAccounts'; -import ReportScreenContext from '../ReportScreenContext'; +import { ReactionListContext } from '../ReportScreenContext'; import Permissions from '../../../libs/Permissions'; const propTypes = { @@ -127,7 +127,7 @@ function ReportActionItem(props) { const [isContextMenuActive, setIsContextMenuActive] = useState(ReportActionContextMenu.isActiveReportAction(props.action.reportActionID)); const [isHidden, setIsHidden] = useState(false); const [moderationDecision, setModerationDecision] = useState(CONST.MODERATION.MODERATOR_DECISION_APPROVED); - const {reactionListRef} = useContext(ReportScreenContext); + const reactionListRef = useContext(ReactionListContext); const textInputRef = useRef(); const popoverAnchorRef = useRef(); const downloadedPreviews = useRef([]); diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index da475e61f749..70eb65ade3ea 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -19,7 +19,7 @@ import * as ReportActionsUtils from '../../../libs/ReportActionsUtils'; import reportPropTypes from '../../reportPropTypes'; import PopoverReactionList from './ReactionList/PopoverReactionList'; import getIsReportFullyVisible from '../../../libs/getIsReportFullyVisible'; -import ReportScreenContext from '../ReportScreenContext'; +import { ReactionListContext } from '../ReportScreenContext'; const propTypes = { /** The report currently being looked at */ @@ -54,10 +54,9 @@ const defaultProps = { }; function ReportActionsView(props) { - const context = useContext(ReportScreenContext); useCopySelectionHelper(); - + const reactionListRef = useContext(ReactionListContext) const didLayout = useRef(false); const didSubscribeToReportTypingEvents = useRef(false); const hasCachedActions = useRef(_.size(props.reportActions) > 0); @@ -189,7 +188,7 @@ function ReportActionsView(props) { loadMoreChats={loadMoreChats} policy={props.policy} /> - + ); } From 6125c381cde29277f150f1a4156a6b96af05f150 Mon Sep 17 00:00:00 2001 From: Oscar Franco Date: Mon, 28 Aug 2023 13:44:09 +0200 Subject: [PATCH 2/3] Get rid of full list re-render when marking messages as unread --- src/pages/home/report/ReportActionsList.js | 23 +++++++++++----------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js index 7f897ee825fb..1c0c88e20ec4 100644 --- a/src/pages/home/report/ReportActionsList.js +++ b/src/pages/home/report/ReportActionsList.js @@ -114,9 +114,9 @@ function ReportActionsList({ const readActionSkipped = useRef(false); const reportActionSize = useRef(sortedReportActions.length); - // Considering that renderItem is enclosed within a useCallback, marking it as "read" twice will retain the value as "true," preventing the useCallback from re-executing. - // However, if we create and listen to an object, it will lead to a new useCallback execution. - const [messageManuallyMarked, setMessageManuallyMarked] = useState({read: false}); + // This state is used to force a re-render when the user manually marks a message as unread + // by using a timestamp you can force re-renders without having to worry about if another message was marked as unread before + const [messageManuallyMarkedUnread, setMessageManuallyMarkedUnread] = useState(0); const [isFloatingMessageCounterVisible, setIsFloatingMessageCounterVisible] = useState(false); const animatedStyles = useAnimatedStyle(() => ({ opacity: opacity.value, @@ -163,15 +163,14 @@ function ReportActionsList({ useEffect(() => { const didManuallyMarkReportAsUnread = report.lastReadTime < DateUtils.getDBTime() && ReportUtils.isUnread(report); - if (!didManuallyMarkReportAsUnread) { - setMessageManuallyMarked({read: false}); - return; + if (didManuallyMarkReportAsUnread) { + // Clearing the current unread marker so that it can be recalculated + currentUnreadMarker.current = null; + setMessageManuallyMarkedUnread(new Date().getTime()); + } else { + setMessageManuallyMarkedUnread(0); } - // Clearing the current unread marker so that it can be recalculated - currentUnreadMarker.current = null; - setMessageManuallyMarked({read: true}); - // We only care when a new lastReadTime is set in the report // eslint-disable-next-line react-hooks/exhaustive-deps }, [report.lastReadTime]); @@ -230,7 +229,7 @@ function ReportActionsList({ const isCurrentMessageUnread = isMessageUnread(reportAction, report.lastReadTime); shouldDisplayNewMarker = isCurrentMessageUnread && !isMessageUnread(nextMessage, report.lastReadTime); - if (!messageManuallyMarked.read) { + if (!messageManuallyMarkedUnread) { shouldDisplayNewMarker = shouldDisplayNewMarker && reportAction.actorAccountID !== Report.getCurrentUserAccountID(); } const canDisplayMarker = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < userActiveSince.current : true; @@ -272,7 +271,7 @@ function ReportActionsList({ /> ); }, - [report, hasOutstandingIOU, sortedReportActions, mostRecentIOUReportActionID, messageManuallyMarked], + [report, hasOutstandingIOU, sortedReportActions, mostRecentIOUReportActionID, messageManuallyMarkedUnread], ); // Native mobile does not render updates flatlist the changes even though component did update called. From be7ea564639ef27e0ee8052c948d94f3119211b5 Mon Sep 17 00:00:00 2001 From: Oscar Franco Date: Mon, 28 Aug 2023 16:31:16 +0200 Subject: [PATCH 3/3] Linting --- src/pages/home/ReportScreenContext.js | 5 +---- src/pages/home/report/ReportActionItem.js | 2 +- src/pages/home/report/ReportActionsView.js | 5 ++--- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/pages/home/ReportScreenContext.js b/src/pages/home/ReportScreenContext.js index 0be1882699f4..1e8d30cf7585 100644 --- a/src/pages/home/ReportScreenContext.js +++ b/src/pages/home/ReportScreenContext.js @@ -3,7 +3,4 @@ import {createContext} from 'react'; const ActionListContext = createContext(); const ReactionListContext = createContext(); -export { - ActionListContext, - ReactionListContext -} \ No newline at end of file +export {ActionListContext, ReactionListContext}; diff --git a/src/pages/home/report/ReportActionItem.js b/src/pages/home/report/ReportActionItem.js index 4a0c0d7bd3ba..fe2ea3acf691 100644 --- a/src/pages/home/report/ReportActionItem.js +++ b/src/pages/home/report/ReportActionItem.js @@ -64,7 +64,7 @@ import * as PersonalDetailsUtils from '../../../libs/PersonalDetailsUtils'; import ReportActionItemBasicMessage from './ReportActionItemBasicMessage'; import * as store from '../../../libs/actions/ReimbursementAccount/store'; import * as BankAccounts from '../../../libs/actions/BankAccounts'; -import { ReactionListContext } from '../ReportScreenContext'; +import {ReactionListContext} from '../ReportScreenContext'; import Permissions from '../../../libs/Permissions'; const propTypes = { diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 70eb65ade3ea..25d01a6953d1 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -19,7 +19,7 @@ import * as ReportActionsUtils from '../../../libs/ReportActionsUtils'; import reportPropTypes from '../../reportPropTypes'; import PopoverReactionList from './ReactionList/PopoverReactionList'; import getIsReportFullyVisible from '../../../libs/getIsReportFullyVisible'; -import { ReactionListContext } from '../ReportScreenContext'; +import {ReactionListContext} from '../ReportScreenContext'; const propTypes = { /** The report currently being looked at */ @@ -54,9 +54,8 @@ const defaultProps = { }; function ReportActionsView(props) { - useCopySelectionHelper(); - const reactionListRef = useContext(ReactionListContext) + const reactionListRef = useContext(ReactionListContext); const didLayout = useRef(false); const didSubscribeToReportTypingEvents = useRef(false); const hasCachedActions = useRef(_.size(props.reportActions) > 0);