Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: re-calc the marker when msgs are deleted #42742

Merged
merged 8 commits into from
Jun 20, 2024
Merged

fix: re-calc the marker when msgs are deleted #42742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6e8dc5d
fix: re-calc the marker when msgs are deleted
dominictb May 29, 2024
844e6bf
fix: lint
dominictb May 29, 2024
88f5d9d
fix: set the unread marker logic
dominictb Jun 3, 2024
6e1cb4f
fix: prettier
dominictb Jun 3, 2024
5941b95
Merge branch 'main' into fix/41935
dominictb Jun 10, 2024
cc42221
fix: move the set markerLastReadTime outside the loop func
dominictb Jun 10, 2024
fbad958
chore: add unit test
dominictb Jun 10, 2024
b7f75ef
fix: update lint for test
dominictb Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/pages/home/ReportScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,6 +704,7 @@ function ReportScreen({
<View
style={[styles.flex1, styles.justifyContentEnd, styles.overflowHidden]}
onLayout={onListLayout}
testID="report-actions-view-wrapper"
>
{shouldShowReportActionList && (
<ReportActionsView
Expand Down
32 changes: 25 additions & 7 deletions src/pages/home/report/ReportActionsList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, 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, 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
Expand Down Expand Up @@ -490,21 +492,37 @@ 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<string | undefined>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to save the lastReadTime in a ref twice? It is already being saved here:

App/src/pages/home/report/ReportActionsList.tsx

Line 198 in b7f75ef

const lastReadTimeRef = useRef(report.lastReadTime);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arosiclair in my proposal #41935 (comment) it should have been explained clearly. In short, the original issue here is because the lastReadTimeRef is tied tightly with report.lastReadTime, so in case we re-calculate the unread marker, the lastReadTimeRef has been updated with report.lastReadTime value, causing the unread marker to disappear.

My main solution is that we will persist another lastReadTimeRef, called markerLastReadTimeRef which is tied to the currentUnreadMarker, i.e: it will be set with lastReadTimeRef.current if the unread marker first appears during calculation (changing from undefined -> defined value), and it shouldn't change its own value if the unread marker still exists in the screen. We'll clear the markerLastReadTimeRef if the unread marker disappears from the screen (changing from defined value -> undefined/null)

Hope that's clear!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so in case we re-calculate the unread marker, the lastReadTimeRef has been updated with report.lastReadTime value, causing the unread marker to disappear.

Where exactly is this happening? Is there a way we can prevent that from happening? From what I understand, the marker time should only change when the user marks a message as unread. So any other change would be unintended.

Let's try fixing the original lastReadTimeRef before adding another one since this functionality is already confusing and complex as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arosiclair as explained in my proposal as well as here, the lastReadTimeRef.current has already been set to the report.lastReadTime the first time the unread marker is calculated, and it stays the same after that (or changes if the report.lastReadTime changes). Hence, we couldn't use lastReadTimeRef to re-calc the marker in case the sortedVisibleReportActions changes.

(And you already approved my proposal 😅 )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I hired you for the job but your proposal was not fully satisfactory. Please work with me to get this PR in a better place.

I'm asking these questions again, please answer them directly: When the message is deleted, where exactly is the lastReadTimeRef being cleared? Is there a way we can prevent that from happening?

If the answer is yes (I believe it is), then we shouldn't need to save the time for the marker twice. Instead we can just fix the issue where lastReadTimeRef is being cleared when it shouldn't be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the message is deleted, where exactly is the lastReadTimeRef being cleared?

The lastReadTimeRef is tied to the report.lastReadTime in here. The thing is, when you open a report, the unread marker will be calculated using the old report.lastReadTime (which is saved in lastReadTimeRef.current) in here. However, after a short while, the OpenReport API (see the image below) has updated the report.lastReadTime to a new timestamp (which is larger than the last message timestamp).

image

However, since the unread marker has already been set, in here the calculateUnreadMarker function will still render the unread marker. Now when a message associated with the unread marker is deleted, we face 2 issues:

  1. The shouldDisplayNewMarker here is trying to compare the message with the outdated currentUnreadMarker (which associated message has been deleted).
  2. Even if we fix the (1) issue, we couldn't use lastReadTimeRef.current to calculate new marker since the report.lastReadTime has been updated to a timestamp larger than the latest message timestamp as mentioned above

Is there a way we can prevent that from happening?

We'll still need a FE change to fix (1)

For (2) either we can fix the report.lastReadTime in the OpenReport API (which I think this involves lots of complication, as report.lastReadTime is used elsewhere, e.g: showing the unread status of the report in LHN), or we can employ the solution in this PR (which is fully FE, more simple and elegant which doesn't touch the report.lastReadTime value)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arosiclair hope that's clear enough for you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. I took a closer look at this component and realized it needs a bit of a refactor to fix this issue properly and I can't really communicate that through PR comments. I'm going to merge your changes since they work and are covered with tests but I'm also going to make a follow up issue to clean this up.


useEffect(() => {
if (currentUnreadMarker) {
return;
}
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, false)) {
return;
}
markerFound = true;
if (!currentUnreadMarker && currentUnreadMarker !== reportAction.reportActionID) {
if (!currentUnreadMarker || currentUnreadMarker !== reportAction.reportActionID) {
cacheUnreadMarkers.set(report.reportID, reportAction.reportActionID);
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);
}
Expand Down Expand Up @@ -573,7 +591,7 @@ function ReportActionsList({
displayAsGroup={ReportActionsUtils.isConsecutiveActionMadeByPreviousActor(sortedVisibleReportActions, index)}
mostRecentIOUReportActionID={mostRecentIOUReportActionID}
shouldHideThreadDividerLine={shouldHideThreadDividerLine}
shouldDisplayNewMarker={shouldDisplayNewMarker(reportAction, index)}
shouldDisplayNewMarker={shouldDisplayNewMarker(reportAction, index, markerLastReadTimeRef.current, true)}
shouldDisplayReplyDivider={sortedVisibleReportActions.length > 1}
isFirstVisibleReportAction={firstVisibleReportActionID === reportAction.reportActionID}
shouldUseThreadDividerLine={shouldUseThreadDividerLine}
Expand Down
46 changes: 46 additions & 0 deletions tests/ui/UnreadIndicatorsTest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,52 @@ describe('Unread Indicators', () => {
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();
})
.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();
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()
Expand Down
Loading