From 4f2d8c26d98fa3f41d7bf71afd00311acae21176 Mon Sep 17 00:00:00 2001 From: Heri Setiawan Date: Mon, 25 Sep 2023 17:58:28 +0700 Subject: [PATCH 1/5] Fix unread marker showing double lines on task comment --- src/pages/home/report/ReportActionsList.js | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js index f3f40d34a0f5..91e96f953165 100644 --- a/src/pages/home/report/ReportActionsList.js +++ b/src/pages/home/report/ReportActionsList.js @@ -112,7 +112,7 @@ function ReportActionsList({ const {translate} = useLocalize(); const {isOffline} = useNetwork(); const opacity = useSharedValue(0); - const userActiveSince = useRef(null); + const [userActiveSince, setUserActiveSince] = useState(null); const [currentUnreadMarker, setCurrentUnreadMarker] = useState(null); const scrollingVerticalOffset = useRef(0); const readActionSkipped = useRef(false); @@ -135,16 +135,14 @@ function ReportActionsList({ // If the reportID changes, we reset the userActiveSince to null, we need to do it because // the parent component is sending the previous reportID even when the user isn't active // on the report - if (userActiveSince.current && prevReportID && prevReportID !== report.reportID) { - userActiveSince.current = null; - } else { - userActiveSince.current = DateUtils.getDBTime(); + if (prevReportID === report.reportID) { + setUserActiveSince(DateUtils.getDBTime()); } prevReportID = report.reportID; }, [report.reportID]); useEffect(() => { - if (!userActiveSince.current || report.reportID !== prevReportID) { + if (!userActiveSince || report.reportID !== prevReportID) { return; } @@ -163,7 +161,7 @@ function ReportActionsList({ reportActionSize.current = sortedReportActions.length; setCurrentUnreadMarker(null); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [sortedReportActions.length, report.reportID]); + }, [sortedReportActions.length, report.reportID, userActiveSince]); useEffect(() => { const didManuallyMarkReportAsUnread = report.lastReadTime < DateUtils.getDBTime() && ReportUtils.isUnread(report); @@ -202,6 +200,7 @@ function ReportActionsList({ return; } reportScrollManager.scrollToBottom(); + setUserActiveSince(DateUtils.getDBTime()); }); const cleanup = () => { @@ -284,8 +283,7 @@ function ReportActionsList({ if (!messageManuallyMarked.read) { shouldDisplayNewMarker = shouldDisplayNewMarker && reportAction.actorAccountID !== Report.getCurrentUserAccountID(); } - const canDisplayMarker = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < userActiveSince.current : true; - + const canDisplayMarker = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < userActiveSince : true; if (!currentUnreadMarker && shouldDisplayNewMarker && canDisplayMarker) { setCurrentUnreadMarker(reportAction.reportActionID); } @@ -305,7 +303,7 @@ function ReportActionsList({ /> ); }, - [report, hasOutstandingIOU, sortedReportActions, mostRecentIOUReportActionID, messageManuallyMarked, shouldHideThreadDividerLine, currentUnreadMarker], + [report, hasOutstandingIOU, sortedReportActions, mostRecentIOUReportActionID, messageManuallyMarked, shouldHideThreadDividerLine, currentUnreadMarker, userActiveSince], ); // Native mobile does not render updates flatlist the changes even though component did update called. From 249cd27b7bd65b0b78f13235505f3240e83140d3 Mon Sep 17 00:00:00 2001 From: Heri Setiawan Date: Tue, 26 Sep 2023 10:00:03 +0700 Subject: [PATCH 2/5] Fix unit test for unread marker --- src/pages/home/report/ReportActionsList.js | 70 ++++++++++++---------- 1 file changed, 37 insertions(+), 33 deletions(-) diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js index 91e96f953165..31cfa739468c 100644 --- a/src/pages/home/report/ReportActionsList.js +++ b/src/pages/home/report/ReportActionsList.js @@ -112,7 +112,7 @@ function ReportActionsList({ const {translate} = useLocalize(); const {isOffline} = useNetwork(); const opacity = useSharedValue(0); - const [userActiveSince, setUserActiveSince] = useState(null); + const userActiveSince = useRef(null); const [currentUnreadMarker, setCurrentUnreadMarker] = useState(null); const scrollingVerticalOffset = useRef(0); const readActionSkipped = useRef(false); @@ -135,14 +135,16 @@ function ReportActionsList({ // If the reportID changes, we reset the userActiveSince to null, we need to do it because // the parent component is sending the previous reportID even when the user isn't active // on the report - if (prevReportID === report.reportID) { - setUserActiveSince(DateUtils.getDBTime()); + if (userActiveSince.current && prevReportID && prevReportID !== report.reportID) { + userActiveSince.current = null; + } else { + userActiveSince.current = DateUtils.getDBTime(); } prevReportID = report.reportID; }, [report.reportID]); useEffect(() => { - if (!userActiveSince || report.reportID !== prevReportID) { + if (!userActiveSince.current || report.reportID !== prevReportID) { return; } @@ -161,7 +163,7 @@ function ReportActionsList({ reportActionSize.current = sortedReportActions.length; setCurrentUnreadMarker(null); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [sortedReportActions.length, report.reportID, userActiveSince]); + }, [sortedReportActions.length, report.reportID]); useEffect(() => { const didManuallyMarkReportAsUnread = report.lastReadTime < DateUtils.getDBTime() && ReportUtils.isUnread(report); @@ -200,7 +202,6 @@ function ReportActionsList({ return; } reportScrollManager.scrollToBottom(); - setUserActiveSince(DateUtils.getDBTime()); }); const cleanup = () => { @@ -267,43 +268,46 @@ function ReportActionsList({ ); /** - * @param {Object} args - * @param {Number} args.index - * @returns {React.Component} + * Evaluate new unread marker visibility for each of the report actions. + * @returns boolean */ - const renderItem = useCallback( - ({item: reportAction, index}) => { - let shouldDisplayNewMarker = false; + + const shouldDisplayNewMarker = useCallback( + (reportAction, index) => { + let shouldDisplay = false; if (!currentUnreadMarker) { const nextMessage = sortedReportActions[index + 1]; const isCurrentMessageUnread = isMessageUnread(reportAction, report.lastReadTime); - shouldDisplayNewMarker = isCurrentMessageUnread && !isMessageUnread(nextMessage, report.lastReadTime); - + shouldDisplay = isCurrentMessageUnread && !isMessageUnread(nextMessage, report.lastReadTime); if (!messageManuallyMarked.read) { - shouldDisplayNewMarker = shouldDisplayNewMarker && reportAction.actorAccountID !== Report.getCurrentUserAccountID(); - } - const canDisplayMarker = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < userActiveSince : true; - if (!currentUnreadMarker && shouldDisplayNewMarker && canDisplayMarker) { - setCurrentUnreadMarker(reportAction.reportActionID); + shouldDisplay = shouldDisplay && reportAction.actorAccountID !== Report.getCurrentUserAccountID(); } } else { - shouldDisplayNewMarker = reportAction.reportActionID === currentUnreadMarker; + shouldDisplay = reportAction.reportActionID === currentUnreadMarker; } - return ( - - ); + if (shouldDisplay) { + setCurrentUnreadMarker(reportAction.reportActionID); + } + return shouldDisplay; }, - [report, hasOutstandingIOU, sortedReportActions, mostRecentIOUReportActionID, messageManuallyMarked, shouldHideThreadDividerLine, currentUnreadMarker, userActiveSince], + [currentUnreadMarker, sortedReportActions, report.lastReadTime, messageManuallyMarked.read], + ); + + const renderItem = useCallback( + ({item: reportAction, index}) => ( + + ), + [report, hasOutstandingIOU, sortedReportActions, mostRecentIOUReportActionID, shouldHideThreadDividerLine, shouldDisplayNewMarker], ); // Native mobile does not render updates flatlist the changes even though component did update called. From db64659a93a4e1d8df02b97e22f40d792037c389 Mon Sep 17 00:00:00 2001 From: Heri Setiawan Date: Sat, 30 Sep 2023 06:33:27 +0700 Subject: [PATCH 3/5] Refactor unread marker effect --- src/pages/home/report/ReportActionsList.js | 140 +++++++++++---------- 1 file changed, 76 insertions(+), 64 deletions(-) diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js index 31cfa739468c..0163a7ff2b4f 100644 --- a/src/pages/home/report/ReportActionsList.js +++ b/src/pages/home/report/ReportActionsList.js @@ -32,6 +32,9 @@ const propTypes = { /** The ID of the most recent IOU report action connected with the shown report */ mostRecentIOUReportActionID: PropTypes.string, + /** The report metadata loading states */ + isLoadingReportActions: PropTypes.bool, + /** Are we loading more report actions? */ isLoadingMoreReportActions: PropTypes.bool, @@ -61,6 +64,7 @@ const defaultProps = { personalDetails: {}, onScroll: () => {}, mostRecentIOUReportActionID: '', + isLoadingReportActions: false, isLoadingMoreReportActions: false, ...withCurrentUserPersonalDetailsDefaultProps, }; @@ -96,6 +100,8 @@ function isMessageUnread(message, lastReadTime) { function ReportActionsList({ report, + isLoadingReportActions, + isLoadingMoreReportActions, sortedReportActions, windowHeight, onScroll, @@ -117,10 +123,11 @@ function ReportActionsList({ const scrollingVerticalOffset = useRef(0); const readActionSkipped = useRef(false); const reportActionSize = useRef(sortedReportActions.length); + const firstRenderRef = useRef(true); - // 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, @@ -129,7 +136,6 @@ function ReportActionsList({ useEffect(() => { opacity.value = withTiming(1, {duration: 100}); }, [opacity]); - const [skeletonViewHeight, setSkeletonViewHeight] = useState(0); useEffect(() => { // If the reportID changes, we reset the userActiveSince to null, we need to do it because @@ -167,14 +173,14 @@ function ReportActionsList({ useEffect(() => { const didManuallyMarkReportAsUnread = report.lastReadTime < DateUtils.getDBTime() && ReportUtils.isUnread(report); - if (!didManuallyMarkReportAsUnread) { - setMessageManuallyMarked({read: false}); + if (didManuallyMarkReportAsUnread) { + // Clearing the current unread marker so that it can be recalculated + setCurrentUnreadMarker(null); + setMessageManuallyMarkedUnread(new Date().getTime()); return; } - // Clearing the current unread marker so that it can be recalculated - setCurrentUnreadMarker(null); - setMessageManuallyMarked({read: true}); + setMessageManuallyMarkedUnread(0); // We only care when a new lastReadTime is set in the report // eslint-disable-next-line react-hooks/exhaustive-deps @@ -268,46 +274,44 @@ function ReportActionsList({ ); /** - * Evaluate new unread marker visibility for each of the report actions. - * @returns boolean + * @param {Object} args + * @param {Number} args.index + * @returns {React.Component} */ - - const shouldDisplayNewMarker = useCallback( - (reportAction, index) => { - let shouldDisplay = false; + const renderItem = useCallback( + ({item: reportAction, index}) => { + let shouldDisplayNewMarker = false; if (!currentUnreadMarker) { const nextMessage = sortedReportActions[index + 1]; const isCurrentMessageUnread = isMessageUnread(reportAction, report.lastReadTime); - shouldDisplay = isCurrentMessageUnread && !isMessageUnread(nextMessage, report.lastReadTime); - if (!messageManuallyMarked.read) { - shouldDisplay = shouldDisplay && reportAction.actorAccountID !== Report.getCurrentUserAccountID(); + shouldDisplayNewMarker = isCurrentMessageUnread && !isMessageUnread(nextMessage, report.lastReadTime); + + if (!messageManuallyMarkedUnread) { + shouldDisplayNewMarker = shouldDisplayNewMarker && reportAction.actorAccountID !== Report.getCurrentUserAccountID(); + } + const canDisplayMarker = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < userActiveSince.current : true; + + if (!currentUnreadMarker && shouldDisplayNewMarker && canDisplayMarker) { + setCurrentUnreadMarker(reportAction.reportActionID); } } else { - shouldDisplay = reportAction.reportActionID === currentUnreadMarker; - } - if (shouldDisplay) { - setCurrentUnreadMarker(reportAction.reportActionID); + shouldDisplayNewMarker = reportAction.reportActionID === currentUnreadMarker; } - return shouldDisplay; + return ( + + ); }, - [currentUnreadMarker, sortedReportActions, report.lastReadTime, messageManuallyMarked.read], - ); - - const renderItem = useCallback( - ({item: reportAction, index}) => ( - - ), - [report, hasOutstandingIOU, sortedReportActions, mostRecentIOUReportActionID, shouldHideThreadDividerLine, shouldDisplayNewMarker], + [report, hasOutstandingIOU, sortedReportActions, mostRecentIOUReportActionID, messageManuallyMarkedUnread, shouldHideThreadDividerLine, currentUnreadMarker], ); // Native mobile does not render updates flatlist the changes even though component did update called. @@ -316,6 +320,36 @@ function ReportActionsList({ const hideComposer = ReportUtils.shouldDisableWriteActions(report); const shouldShowReportRecipientLocalTime = ReportUtils.canShowReportRecipientLocalTime(personalDetailsList, report, currentUserPersonalDetails.accountID) && !isComposerFullSize; + const renderFooter = useCallback(() => { + // Skip this hook on the first render, as we are not sure if more actions are going to be loaded + // Therefore showing the skeleton on footer might be misleading + if (firstRenderRef.current) { + firstRenderRef.current = false; + return null; + } + + if (isLoadingMoreReportActions) { + return ; + } + + // Make sure the oldest report action loaded is not the first. This is so we do not show the + // skeleton view above the created action in a newly generated optimistic chat or one with not + // that many comments. + const lastReportAction = _.last(sortedReportActions) || {}; + if (isLoadingReportActions && lastReportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED) { + return ; + } + + return null; + }, [isLoadingMoreReportActions, isLoadingReportActions, sortedReportActions, isOffline]); + + const onLayoutInner = useCallback( + (event) => { + onLayout(event); + }, + [onLayout], + ); + return ( <> { - if (report.isLoadingMoreReportActions) { - return ; - } - - // Make sure the oldest report action loaded is not the first. This is so we do not show the - // skeleton view above the created action in a newly generated optimistic chat or one with not - // that many comments. - const lastReportAction = _.last(sortedReportActions) || {}; - if (report.isLoadingReportActions && lastReportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED) { - return ( - - ); - } - - return null; - }} + ListFooterComponent={renderFooter} keyboardShouldPersistTaps="handled" - onLayout={(event) => { - setSkeletonViewHeight(event.nativeEvent.layout.height); - onLayout(event); - }} + onLayout={onLayoutInner} onScroll={trackVerticalScrolling} extraData={extraData} /> From a1c06dd52bea9dfec20e0edfa576c9453098af7a Mon Sep 17 00:00:00 2001 From: Heri Setiawan Date: Sat, 30 Sep 2023 06:49:28 +0700 Subject: [PATCH 4/5] Resolve conflict --- src/pages/home/report/ReportActionsList.js | 67 +++++++++++++--------- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js index 0163a7ff2b4f..508ffef68c8c 100644 --- a/src/pages/home/report/ReportActionsList.js +++ b/src/pages/home/report/ReportActionsList.js @@ -274,44 +274,55 @@ function ReportActionsList({ ); /** - * @param {Object} args - * @param {Number} args.index - * @returns {React.Component} + * Evaluate new unread marker visibility for each of the report actions. + * @returns boolean */ - const renderItem = useCallback( - ({item: reportAction, index}) => { - let shouldDisplayNewMarker = false; + + const shouldDisplayNewMarker = useCallback( + (reportAction, index) => { + let shouldDisplay = false; if (!currentUnreadMarker) { const nextMessage = sortedReportActions[index + 1]; const isCurrentMessageUnread = isMessageUnread(reportAction, report.lastReadTime); - shouldDisplayNewMarker = isCurrentMessageUnread && !isMessageUnread(nextMessage, report.lastReadTime); - + shouldDisplay = isCurrentMessageUnread && !isMessageUnread(nextMessage, report.lastReadTime); if (!messageManuallyMarkedUnread) { - shouldDisplayNewMarker = shouldDisplayNewMarker && reportAction.actorAccountID !== Report.getCurrentUserAccountID(); - } - const canDisplayMarker = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < userActiveSince.current : true; - - if (!currentUnreadMarker && shouldDisplayNewMarker && canDisplayMarker) { - setCurrentUnreadMarker(reportAction.reportActionID); + shouldDisplay = shouldDisplay && reportAction.actorAccountID !== Report.getCurrentUserAccountID(); } } else { - shouldDisplayNewMarker = reportAction.reportActionID === currentUnreadMarker; + shouldDisplay = reportAction.reportActionID === currentUnreadMarker; } - return ( - - ); + return shouldDisplay; }, - [report, hasOutstandingIOU, sortedReportActions, mostRecentIOUReportActionID, messageManuallyMarkedUnread, shouldHideThreadDividerLine, currentUnreadMarker], + [currentUnreadMarker, sortedReportActions, report.lastReadTime, messageManuallyMarkedUnread], + ); + + useEffect(() => { + // Iterate through the report actions and set appropriate unread marker. + // This is to avoid a warning of: + // Cannot update a component (ReportActionsList) while rendering a different component (CellRenderer). + _.each(sortedReportActions, (reportAction, index) => { + if (!shouldDisplayNewMarker(reportAction, index)) { + return; + } + setCurrentUnreadMarker(reportAction.reportActionID); + }); + }, [sortedReportActions, report.lastReadTime, messageManuallyMarkedUnread, shouldDisplayNewMarker]); + + const renderItem = useCallback( + ({item: reportAction, index}) => ( + + ), + [report, hasOutstandingIOU, sortedReportActions, mostRecentIOUReportActionID, shouldHideThreadDividerLine, shouldDisplayNewMarker], ); // Native mobile does not render updates flatlist the changes even though component did update called. From 49ca73a4569c854efc8c51886f42dbc50a69bbf2 Mon Sep 17 00:00:00 2001 From: Heri Setiawan Date: Tue, 3 Oct 2023 15:42:17 +0700 Subject: [PATCH 5/5] Resolve conflicts --- src/pages/home/report/ReportActionsList.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js index 463a7e339bed..9e9865425e18 100644 --- a/src/pages/home/report/ReportActionsList.js +++ b/src/pages/home/report/ReportActionsList.js @@ -319,6 +319,7 @@ function ReportActionsList({ reportAction={reportAction} index={index} report={report} + linkedReportActionID={linkedReportActionID} hasOutstandingIOU={hasOutstandingIOU} sortedReportActions={sortedReportActions} mostRecentIOUReportActionID={mostRecentIOUReportActionID} @@ -326,7 +327,7 @@ function ReportActionsList({ shouldDisplayNewMarker={shouldDisplayNewMarker(reportAction, index)} /> ), - [report, hasOutstandingIOU, sortedReportActions, mostRecentIOUReportActionID, shouldHideThreadDividerLine, shouldDisplayNewMarker], + [report, linkedReportActionID, hasOutstandingIOU, sortedReportActions, mostRecentIOUReportActionID, shouldHideThreadDividerLine, shouldDisplayNewMarker], ); // Native mobile does not render updates flatlist the changes even though component did update called.