From 6e8dc5d99abe3a1a2ac00b8983e3963b4f7e7058 Mon Sep 17 00:00:00 2001 From: dominictb Date: Wed, 29 May 2024 16:02:29 +0700 Subject: [PATCH 1/7] fix: re-calc the marker when msgs are deleted Signed-off-by: dominictb --- src/pages/home/report/ReportActionsList.tsx | 22 ++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/pages/home/report/ReportActionsList.tsx b/src/pages/home/report/ReportActionsList.tsx index c700fea4fb85..480ecefd8f59 100644 --- a/src/pages/home/report/ReportActionsList.tsx +++ b/src/pages/home/report/ReportActionsList.tsx @@ -447,12 +447,14 @@ function ReportActionsList({ * Evaluate new unread marker visibility for each of the report actions. */ const shouldDisplayNewMarker = useCallback( - (reportAction: OnyxTypes.ReportAction, index: number): boolean => { + (reportAction: OnyxTypes.ReportAction, index: number, lastReadTime?: string): boolean => { + // if lastReadTime is null, use the lastReadTimeRef.current + const baseLastReadTime = lastReadTime ?? lastReadTimeRef.current; let shouldDisplay = false; if (!currentUnreadMarker) { const nextMessage = sortedVisibleReportActions[index + 1]; - const isCurrentMessageUnread = isMessageUnread(reportAction, lastReadTimeRef.current); - shouldDisplay = isCurrentMessageUnread && (!nextMessage || !isMessageUnread(nextMessage, lastReadTimeRef.current)) && !ReportActionsUtils.shouldHideNewMarker(reportAction); + const isCurrentMessageUnread = isMessageUnread(reportAction, baseLastReadTime); + shouldDisplay = isCurrentMessageUnread && (!nextMessage || !isMessageUnread(nextMessage, baseLastReadTime)) && !ReportActionsUtils.shouldHideNewMarker(reportAction); if (shouldDisplay && !messageManuallyMarkedUnread) { const isWithinVisibleThreshold = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < (userActiveSince.current ?? '') : true; // Prevent displaying a new marker line when report action is of type "REPORT_PREVIEW" and last actor is the current user @@ -490,19 +492,29 @@ function ReportActionsList({ return ReportUtils.isExpenseReport(report) || ReportUtils.isIOUReport(report); }, [parentReportAction, report, sortedVisibleReportActions]); + // storing the last read time used to render the unread marker + const markerLastReadTimeRef = useRef(); + + useEffect(() => { + if (!currentUnreadMarker) { + markerLastReadTimeRef.current = undefined; + } + }, [currentUnreadMarker]); + const calculateUnreadMarker = useCallback(() => { // 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). let markerFound = false; sortedVisibleReportActions.forEach((reportAction, index) => { - if (!shouldDisplayNewMarker(reportAction, index)) { + if (!shouldDisplayNewMarker(reportAction, index, markerLastReadTimeRef.current)) { return; } markerFound = true; if (!currentUnreadMarker && currentUnreadMarker !== reportAction.reportActionID) { cacheUnreadMarkers.set(report.reportID, reportAction.reportActionID); setCurrentUnreadMarker(reportAction.reportActionID); + markerLastReadTimeRef.current = lastReadTimeRef.current; } }); if (!markerFound && !linkedReportActionID) { @@ -573,7 +585,7 @@ function ReportActionsList({ displayAsGroup={ReportActionsUtils.isConsecutiveActionMadeByPreviousActor(sortedVisibleReportActions, index)} mostRecentIOUReportActionID={mostRecentIOUReportActionID} shouldHideThreadDividerLine={shouldHideThreadDividerLine} - shouldDisplayNewMarker={shouldDisplayNewMarker(reportAction, index)} + shouldDisplayNewMarker={shouldDisplayNewMarker(reportAction, index, markerLastReadTimeRef.current)} shouldDisplayReplyDivider={sortedVisibleReportActions.length > 1} isFirstVisibleReportAction={firstVisibleReportActionID === reportAction.reportActionID} shouldUseThreadDividerLine={shouldUseThreadDividerLine} From 844e6bf753294068ac5b42bafc3d4409315b378c Mon Sep 17 00:00:00 2001 From: dominictb Date: Wed, 29 May 2024 16:30:09 +0700 Subject: [PATCH 2/7] fix: lint Signed-off-by: dominictb --- src/pages/home/report/ReportActionsList.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/pages/home/report/ReportActionsList.tsx b/src/pages/home/report/ReportActionsList.tsx index 480ecefd8f59..5889b943efd9 100644 --- a/src/pages/home/report/ReportActionsList.tsx +++ b/src/pages/home/report/ReportActionsList.tsx @@ -496,9 +496,10 @@ function ReportActionsList({ const markerLastReadTimeRef = useRef(); useEffect(() => { - if (!currentUnreadMarker) { - markerLastReadTimeRef.current = undefined; + if (currentUnreadMarker) { + return; } + markerLastReadTimeRef.current = undefined; }, [currentUnreadMarker]); const calculateUnreadMarker = useCallback(() => { From 88f5d9dbb71142324d9c5ef6fe67a2ed8811ec9f Mon Sep 17 00:00:00 2001 From: dominictb Date: Mon, 3 Jun 2024 19:28:19 +0700 Subject: [PATCH 3/7] fix: set the unread marker logic Signed-off-by: dominictb --- src/pages/home/report/ReportActionsList.tsx | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/pages/home/report/ReportActionsList.tsx b/src/pages/home/report/ReportActionsList.tsx index 5889b943efd9..225e2811ac49 100644 --- a/src/pages/home/report/ReportActionsList.tsx +++ b/src/pages/home/report/ReportActionsList.tsx @@ -447,11 +447,11 @@ function ReportActionsList({ * Evaluate new unread marker visibility for each of the report actions. */ const shouldDisplayNewMarker = useCallback( - (reportAction: OnyxTypes.ReportAction, index: number, lastReadTime?: string): boolean => { + (reportAction: OnyxTypes.ReportAction, index: number, lastReadTime?: string, shouldCheckWithCurrentUnreadMarker?: boolean): boolean => { // if lastReadTime is null, use the lastReadTimeRef.current const baseLastReadTime = lastReadTime ?? lastReadTimeRef.current; let shouldDisplay = false; - if (!currentUnreadMarker) { + if (!currentUnreadMarker || shouldCheckWithCurrentUnreadMarker) { const nextMessage = sortedVisibleReportActions[index + 1]; const isCurrentMessageUnread = isMessageUnread(reportAction, baseLastReadTime); shouldDisplay = isCurrentMessageUnread && (!nextMessage || !isMessageUnread(nextMessage, baseLastReadTime)) && !ReportActionsUtils.shouldHideNewMarker(reportAction); @@ -468,7 +468,7 @@ function ReportActionsList({ } else { shouldDisplay = reportAction.reportActionID === currentUnreadMarker; } - + return shouldDisplay; }, [currentUnreadMarker, sortedVisibleReportActions, report.reportID, messageManuallyMarkedUnread], @@ -508,16 +508,19 @@ function ReportActionsList({ // Cannot update a component (ReportActionsList) while rendering a different component (CellRenderer). let markerFound = false; sortedVisibleReportActions.forEach((reportAction, index) => { - if (!shouldDisplayNewMarker(reportAction, index, markerLastReadTimeRef.current)) { + if (!shouldDisplayNewMarker(reportAction, index, markerLastReadTimeRef.current, false)) { return; } markerFound = true; - if (!currentUnreadMarker && currentUnreadMarker !== reportAction.reportActionID) { + if (!currentUnreadMarker || currentUnreadMarker !== reportAction.reportActionID) { cacheUnreadMarkers.set(report.reportID, reportAction.reportActionID); + if(!markerLastReadTimeRef.current) { + markerLastReadTimeRef.current = lastReadTimeRef.current; + } setCurrentUnreadMarker(reportAction.reportActionID); - markerLastReadTimeRef.current = lastReadTimeRef.current; } }); + if (!markerFound && !linkedReportActionID) { setCurrentUnreadMarker(null); } @@ -586,7 +589,7 @@ function ReportActionsList({ displayAsGroup={ReportActionsUtils.isConsecutiveActionMadeByPreviousActor(sortedVisibleReportActions, index)} mostRecentIOUReportActionID={mostRecentIOUReportActionID} shouldHideThreadDividerLine={shouldHideThreadDividerLine} - shouldDisplayNewMarker={shouldDisplayNewMarker(reportAction, index, markerLastReadTimeRef.current)} + shouldDisplayNewMarker={shouldDisplayNewMarker(reportAction, index, markerLastReadTimeRef.current, true)} shouldDisplayReplyDivider={sortedVisibleReportActions.length > 1} isFirstVisibleReportAction={firstVisibleReportActionID === reportAction.reportActionID} shouldUseThreadDividerLine={shouldUseThreadDividerLine} From 6e1cb4fe29c5ea97c08fee06d98489defdf9acdf Mon Sep 17 00:00:00 2001 From: dominictb Date: Mon, 3 Jun 2024 19:39:53 +0700 Subject: [PATCH 4/7] fix: prettier Signed-off-by: dominictb --- src/pages/home/report/ReportActionsList.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pages/home/report/ReportActionsList.tsx b/src/pages/home/report/ReportActionsList.tsx index 225e2811ac49..f5ce0f134d8e 100644 --- a/src/pages/home/report/ReportActionsList.tsx +++ b/src/pages/home/report/ReportActionsList.tsx @@ -451,7 +451,7 @@ function ReportActionsList({ // if lastReadTime is null, use the lastReadTimeRef.current const baseLastReadTime = lastReadTime ?? lastReadTimeRef.current; let shouldDisplay = false; - if (!currentUnreadMarker || shouldCheckWithCurrentUnreadMarker) { + if (!currentUnreadMarker || !shouldCheckWithCurrentUnreadMarker) { const nextMessage = sortedVisibleReportActions[index + 1]; const isCurrentMessageUnread = isMessageUnread(reportAction, baseLastReadTime); shouldDisplay = isCurrentMessageUnread && (!nextMessage || !isMessageUnread(nextMessage, baseLastReadTime)) && !ReportActionsUtils.shouldHideNewMarker(reportAction); @@ -468,7 +468,7 @@ function ReportActionsList({ } else { shouldDisplay = reportAction.reportActionID === currentUnreadMarker; } - + return shouldDisplay; }, [currentUnreadMarker, sortedVisibleReportActions, report.reportID, messageManuallyMarkedUnread], @@ -514,7 +514,7 @@ function ReportActionsList({ markerFound = true; if (!currentUnreadMarker || currentUnreadMarker !== reportAction.reportActionID) { cacheUnreadMarkers.set(report.reportID, reportAction.reportActionID); - if(!markerLastReadTimeRef.current) { + if (!markerLastReadTimeRef.current) { markerLastReadTimeRef.current = lastReadTimeRef.current; } setCurrentUnreadMarker(reportAction.reportActionID); From cc42221e591e4dfa8d4ddfbb7997b0860091c0a8 Mon Sep 17 00:00:00 2001 From: dominictb Date: Mon, 10 Jun 2024 11:17:56 +0700 Subject: [PATCH 5/7] fix: move the set markerLastReadTime outside the loop func Signed-off-by: dominictb --- src/pages/home/report/ReportActionsList.tsx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/pages/home/report/ReportActionsList.tsx b/src/pages/home/report/ReportActionsList.tsx index f5ce0f134d8e..cfd31382a6d4 100644 --- a/src/pages/home/report/ReportActionsList.tsx +++ b/src/pages/home/report/ReportActionsList.tsx @@ -514,13 +514,15 @@ function ReportActionsList({ markerFound = true; if (!currentUnreadMarker || currentUnreadMarker !== reportAction.reportActionID) { cacheUnreadMarkers.set(report.reportID, reportAction.reportActionID); - if (!markerLastReadTimeRef.current) { - markerLastReadTimeRef.current = lastReadTimeRef.current; - } setCurrentUnreadMarker(reportAction.reportActionID); } }); + // if marker can be found, set the markerLastReadTimeRef to the last read time if necessary + if (markerFound && !markerLastReadTimeRef.current) { + markerLastReadTimeRef.current = lastReadTimeRef.current; + } + if (!markerFound && !linkedReportActionID) { setCurrentUnreadMarker(null); } From fbad95862dfc91476fafc1f43bb384b23020755c Mon Sep 17 00:00:00 2001 From: dominictb Date: Mon, 10 Jun 2024 19:29:48 +0700 Subject: [PATCH 6/7] chore: add unit test Signed-off-by: dominictb --- src/pages/home/ReportScreen.tsx | 1 + tests/ui/UnreadIndicatorsTest.tsx | 57 +++++++++++++++++++++++++++++-- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/pages/home/ReportScreen.tsx b/src/pages/home/ReportScreen.tsx index c557229aca72..1de9fc14dfbc 100644 --- a/src/pages/home/ReportScreen.tsx +++ b/src/pages/home/ReportScreen.tsx @@ -704,6 +704,7 @@ function ReportScreen({ {shouldShowReportActionList && ( | null = null; + /** * Sets up a test with a logged in user that has one unread chat from another user. Returns the test instance. */ function signInAndGetAppWithUnreadChat(): Promise { // Render the App and sign in as a test user. - render(); + renderResult = render(); return waitForBatchedUpdatesWithAct() .then(async () => { await waitForBatchedUpdatesWithAct(); @@ -462,7 +465,57 @@ describe('Unread Indicators', () => { expect(screen.getAllByText('C User')[0]).toBeOnTheScreen(); expect(displayNameTexts[1]?.props?.style?.fontWeight).toBe(FontUtils.fontWeight.bold); expect(screen.getByText('B User')).toBeOnTheScreen(); - })); + }) + ); + it('Delete a chat message and verify the unread indicator is moved', async () => { + const getUnreadIndicator = () => { + const newMessageLineIndicatorHintText = Localize.translateLocal('accessibilityHints.newMessageLineIndicator'); + return screen.queryAllByLabelText(newMessageLineIndicatorHintText); + } + + return signInAndGetAppWithUnreadChat() + .then(() => navigateToSidebarOption(0)) + .then(async () => await act(() => transitionEndCB?.())) + .then(async () => { + const reportActionsViewWrapper = await renderResult?.findByTestId('reportActionsViewWrapper'); + if(reportActionsViewWrapper) { + act(() => { + fireEvent(reportActionsViewWrapper, 'onLayout', { nativeEvent: { layout: { x: 0, y: 0, width: 100, height: 100 } } }); + }) + } + return waitForBatchedUpdates(); + }) + .then(() => { + // Verify the new line indicator is present, and it's before the action with ID 4 + const unreadIndicator = getUnreadIndicator(); + expect(unreadIndicator).toHaveLength(1); + const reportActionID = unreadIndicator[0]?.props?.['data-action-id']; + expect(reportActionID).toBe('4'); + + // simulate delete comment event from Pusher + PusherHelper.emitOnyxUpdate([ + { + onyxMethod: Onyx.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`, + value: { + '4': { + message: [] + } + } + }, + ]); + return waitForBatchedUpdates(); + }) + .then(() => { + // Verify the new line indicator is now before the action with ID 5 + return waitFor(() => { + const unreadIndicator = getUnreadIndicator(); + expect(unreadIndicator).toHaveLength(1); + const reportActionID = unreadIndicator[0]?.props?.['data-action-id']; + expect(reportActionID).toBe('5'); + }) + }) + }) xit('Manually marking a chat message as unread shows the new line indicator and updates the LHN', () => signInAndGetAppWithUnreadChat() From b7f75efb4686cb6677a286f096af7988b0442765 Mon Sep 17 00:00:00 2001 From: dominictb Date: Mon, 10 Jun 2024 23:48:05 +0700 Subject: [PATCH 7/7] fix: update lint for test Signed-off-by: dominictb --- src/pages/home/ReportScreen.tsx | 2 +- tests/ui/UnreadIndicatorsTest.tsx | 97 ++++++++++++++----------------- 2 files changed, 46 insertions(+), 53 deletions(-) diff --git a/src/pages/home/ReportScreen.tsx b/src/pages/home/ReportScreen.tsx index 1de9fc14dfbc..3de4200080b0 100644 --- a/src/pages/home/ReportScreen.tsx +++ b/src/pages/home/ReportScreen.tsx @@ -704,7 +704,7 @@ function ReportScreen({ {shouldShowReportActionList && ( | null = null; - /** * Sets up a test with a logged in user that has one unread chat from another user. Returns the test instance. */ function signInAndGetAppWithUnreadChat(): Promise { // Render the App and sign in as a test user. - renderResult = render(); + render(); return waitForBatchedUpdatesWithAct() .then(async () => { await waitForBatchedUpdatesWithAct(); @@ -465,57 +462,53 @@ describe('Unread Indicators', () => { expect(screen.getAllByText('C User')[0]).toBeOnTheScreen(); expect(displayNameTexts[1]?.props?.style?.fontWeight).toBe(FontUtils.fontWeight.bold); expect(screen.getByText('B User')).toBeOnTheScreen(); + })); + it('Delete a chat message and verify the unread indicator is moved', async () => { + const getUnreadIndicator = () => { + const newMessageLineIndicatorHintText = Localize.translateLocal('accessibilityHints.newMessageLineIndicator'); + return screen.queryAllByLabelText(newMessageLineIndicatorHintText); + }; + + return signInAndGetAppWithUnreadChat() + .then(() => navigateToSidebarOption(0)) + .then(async () => act(() => transitionEndCB?.())) + .then(async () => { + const reportActionsViewWrapper = await screen.findByTestId('report-actions-view-wrapper'); + if (reportActionsViewWrapper) { + fireEvent(reportActionsViewWrapper, 'onLayout', {nativeEvent: {layout: {x: 0, y: 0, width: 100, height: 100}}}); + } + return waitForBatchedUpdates(); }) - ); - it('Delete a chat message and verify the unread indicator is moved', async () => { - const getUnreadIndicator = () => { - const newMessageLineIndicatorHintText = Localize.translateLocal('accessibilityHints.newMessageLineIndicator'); - return screen.queryAllByLabelText(newMessageLineIndicatorHintText); - } - - return signInAndGetAppWithUnreadChat() - .then(() => navigateToSidebarOption(0)) - .then(async () => await act(() => transitionEndCB?.())) - .then(async () => { - const reportActionsViewWrapper = await renderResult?.findByTestId('reportActionsViewWrapper'); - if(reportActionsViewWrapper) { - act(() => { - fireEvent(reportActionsViewWrapper, 'onLayout', { nativeEvent: { layout: { x: 0, y: 0, width: 100, height: 100 } } }); - }) - } - return waitForBatchedUpdates(); - }) - .then(() => { - // Verify the new line indicator is present, and it's before the action with ID 4 + .then(() => { + // Verify the new line indicator is present, and it's before the action with ID 4 + const unreadIndicator = getUnreadIndicator(); + expect(unreadIndicator).toHaveLength(1); + const reportActionID = unreadIndicator[0]?.props?.['data-action-id']; + expect(reportActionID).toBe('4'); + + // simulate delete comment event from Pusher + PusherHelper.emitOnyxUpdate([ + { + onyxMethod: Onyx.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`, + value: { + '4': { + message: [], + }, + }, + }, + ]); + return waitForBatchedUpdates(); + }) + .then(() => + // Verify the new line indicator is now before the action with ID 5 + waitFor(() => { const unreadIndicator = getUnreadIndicator(); - expect(unreadIndicator).toHaveLength(1); const reportActionID = unreadIndicator[0]?.props?.['data-action-id']; - expect(reportActionID).toBe('4'); - - // simulate delete comment event from Pusher - PusherHelper.emitOnyxUpdate([ - { - onyxMethod: Onyx.METHOD.MERGE, - key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`, - value: { - '4': { - message: [] - } - } - }, - ]); - return waitForBatchedUpdates(); - }) - .then(() => { - // Verify the new line indicator is now before the action with ID 5 - return waitFor(() => { - const unreadIndicator = getUnreadIndicator(); - expect(unreadIndicator).toHaveLength(1); - const reportActionID = unreadIndicator[0]?.props?.['data-action-id']; - expect(reportActionID).toBe('5'); - }) - }) - }) + expect(reportActionID).toBe('5'); + }), + ); + }); xit('Manually marking a chat message as unread shows the new line indicator and updates the LHN', () => signInAndGetAppWithUnreadChat()