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.

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

Updated logic to handle unread messages case #25935

Merged
merged 43 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
fab7506
Updated logic to handle the shouldDisplayNewMarker
gedu Aug 25, 2023
47dd5cd
Sending an event when the user mark as unread a message
gedu Sep 14, 2023
922fbf3
lint fixes
gedu Sep 14, 2023
85dddae
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Sep 21, 2023
b9f1f17
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Sep 22, 2023
ec2e0fb
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Sep 27, 2023
3646848
Listening to the specific reports and clearing unread marker cache
gedu Sep 27, 2023
c71ecae
disable dependency hook, no need to listen to the messageManuallyMarked
gedu Sep 28, 2023
ebdeddb
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Sep 29, 2023
de5b8fe
removed console logs
gedu Sep 29, 2023
c483c10
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Oct 3, 2023
8925515
fixed lint issues
gedu Oct 3, 2023
7149558
Moved the DeviceEventEmitter into the markCommentAsUnread function
gedu Oct 9, 2023
89d15d2
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Oct 9, 2023
748c8ad
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Oct 11, 2023
a434c3a
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Oct 11, 2023
f40773b
Updated scrolling direction and amount
gedu Oct 13, 2023
c784057
Fix to add report into map
gedu Oct 18, 2023
51f8117
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Oct 18, 2023
eee1e83
Fixed green line placement
gedu Oct 18, 2023
f1de924
fixed lint errors
gedu Oct 18, 2023
c8d4853
fixed lint issue
gedu Oct 18, 2023
c9722d9
Fixed test
gedu Oct 20, 2023
8782bf7
fixed lint error
gedu Oct 20, 2023
d3ddefd
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Oct 23, 2023
cea0759
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Oct 25, 2023
3ff86e2
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Oct 30, 2023
3630dac
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Oct 30, 2023
57978b6
updated dependency array
gedu Oct 30, 2023
6546ba5
Prettier fixes
gedu Oct 30, 2023
6b2beed
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Nov 2, 2023
dc93b9e
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Nov 8, 2023
95cc451
Merge branch 'main' into edu/23171_manually_unread_marking
gedu Nov 14, 2023
cdb1b80
reverting changes
gedu Nov 14, 2023
5dde6f2
Fixed comments
gedu Nov 14, 2023
2b83ba6
fixed more comments
gedu Nov 15, 2023
c4541fb
improved the comments
gedu Nov 15, 2023
58e2f9c
added new line after pods
gedu Nov 15, 2023
12a92a7
explaining why we cache
gedu Nov 15, 2023
823f3b7
Update src/pages/home/report/ReportActionsList.js
gedu Nov 15, 2023
d1a347c
Merged main into edu/23171_manually_unread_marking
gedu Nov 16, 2023
0a578dd
Moved out the the prevReportID
gedu Nov 16, 2023
8bb02a4
commenting the new variable
gedu Nov 16, 2023
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
3 changes: 3 additions & 0 deletions src/components/EmojiPicker/EmojiPicker.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ const EmojiPicker = forwardRef((props, ref) => {
});
});
return () => {
if (!emojiPopoverDimensionListener) {
MonilBhavsar marked this conversation as resolved.
Show resolved Hide resolved
return;
}
emojiPopoverDimensionListener.remove();
};
}, [isEmojiPickerVisible, isSmallScreenWidth, emojiPopoverAnchorOrigin]);
Expand Down
3 changes: 2 additions & 1 deletion src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import ExpensiMark from 'expensify-common/lib/ExpensiMark';
import Str from 'expensify-common/lib/str';
import lodashDebounce from 'lodash/debounce';
import lodashGet from 'lodash/get';
import {InteractionManager} from 'react-native';
import {DeviceEventEmitter, InteractionManager} from 'react-native';
import Onyx from 'react-native-onyx';
import _ from 'underscore';
import * as ActiveClientManager from '@libs/ActiveClientManager';
Expand Down Expand Up @@ -937,6 +937,7 @@ function markCommentAsUnread(reportID, reportActionCreated) {
],
},
);
DeviceEventEmitter.emit(`unreadAction_${reportID}`, lastReadTime);
}

/**
Expand Down
72 changes: 55 additions & 17 deletions src/pages/home/report/ReportActionsList.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {useRoute} from '@react-navigation/native';
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react';
import {DeviceEventEmitter} from 'react-native';
import Animated, {useAnimatedStyle, useSharedValue, withTiming} from 'react-native-reanimated';
import _ from 'underscore';
import InvertedFlatList from '@components/InvertedFlatList';
Expand Down Expand Up @@ -82,16 +83,23 @@ const defaultProps = {
const VERTICAL_OFFSET_THRESHOLD = 200;
const MSG_VISIBLE_THRESHOLD = 250;

// Seems that there is an architecture issue that prevents us from using the reportID with useRef
// the useRef value gets reset when the reportID changes, so we use a global variable to keep track
let prevReportID = null;

// In the component we are subscribing to the arrival of new actions.
// As there is the possibility that there are multiple instances of a ReportScreen
// for the same report, we only ever want one subscription to be active, as
// the subscriptions could otherwise be conflicting.
const newActionUnsubscribeMap = {};

// Caching the reportID and reportActionID for unread markers ensures persistent tracking
// across multiple reports, preserving the green line placement and allowing retrieval
// of the relevant reportActionID for displaying the green line.
// We need to persist it across reports because there are at least 3 ReportScreen components created so the
// internal states are resetted or recreated.
const cacheUnreadMarkers = new Map();
Copy link
Contributor

Choose a reason for hiding this comment

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

@gedu @MonilBhavsar Hey I'm working on a bug related to unread markers and I'm having trouble wrapping my head around this cache. What problem/scenario does it fix specifically? Like is there a scenario where when we switch away from a report and then navigate back, we want the unread marker to persist?


// Seems that there is an architecture issue that prevents us from using the reportID with useRef
// the useRef value gets reset when the reportID changes, so we use a global variable to keep track
let prevReportID = null;

/**
* Create a unique key for each action in the FlatList.
* We use the reportActionID that is a string representation of a random 64-bit int, which should be
Expand Down Expand Up @@ -137,12 +145,21 @@ function ReportActionsList({
const route = useRoute();
const opacity = useSharedValue(0);
const userActiveSince = useRef(null);
const [currentUnreadMarker, setCurrentUnreadMarker] = useState(null);
const unreadActionSubscription = useRef(null);
const markerInit = () => {
if (!cacheUnreadMarkers.has(report.reportID)) {
return null;
}
return cacheUnreadMarkers.get(report.reportID);
};
const [currentUnreadMarker, setCurrentUnreadMarker] = useState(markerInit);
const scrollingVerticalOffset = useRef(0);
const readActionSkipped = useRef(false);
const hasHeaderRendered = useRef(false);
const hasFooterRendered = useRef(false);
const reportActionSize = useRef(sortedReportActions.length);
const lastReadTimeRef = useRef(report.lastReadTime);

const linkedReportActionID = lodashGet(route, 'params.reportActionID', '');

// This state is used to force a re-render when the user manually marks a message as unread
Expand Down Expand Up @@ -186,25 +203,41 @@ function ReportActionsList({
return;
}

cacheUnreadMarkers.delete(report.reportID);
reportActionSize.current = sortedReportActions.length;
setCurrentUnreadMarker(null);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [sortedReportActions.length, report.reportID]);

useEffect(() => {
const didManuallyMarkReportAsUnread = report.lastReadTime < DateUtils.getDBTime() && ReportUtils.isUnread(report);
if (didManuallyMarkReportAsUnread) {
// Clearing the current unread marker so that it can be recalculated
setCurrentUnreadMarker(null);
setMessageManuallyMarkedUnread(new Date().getTime());
if (!userActiveSince.current || report.reportID !== prevReportID) {
return;
}

if (!messageManuallyMarkedUnread && lastReadTimeRef.current && lastReadTimeRef.current < report.lastReadTime) {
cacheUnreadMarkers.delete(report.reportID);
}
lastReadTimeRef.current = report.lastReadTime;
setMessageManuallyMarkedUnread(0);

// We only care when a new lastReadTime is set in the report
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [report.lastReadTime]);
}, [report.lastReadTime, report.reportID]);

useEffect(() => {
// If the reportID changes, we reset the userActiveSince to null, we need to do it because
// this component doesn't unmount when the reportID changes
if (unreadActionSubscription.current) {
unreadActionSubscription.current.remove();
unreadActionSubscription.current = null;
}

// Listen to specific reportID for unread event and set the marker to new message
unreadActionSubscription.current = DeviceEventEmitter.addListener(`unreadAction_${report.reportID}`, (newLastReadTime) => {
cacheUnreadMarkers.delete(report.reportID);
lastReadTimeRef.current = newLastReadTime;
setCurrentUnreadMarker(null);
setMessageManuallyMarkedUnread(new Date().getTime());
});
}, [report.reportID]);

useEffect(() => {
// Why are we doing this, when in the cleanup of the useEffect we are already calling the unsubscribe function?
Expand Down Expand Up @@ -303,17 +336,21 @@ function ReportActionsList({
let shouldDisplay = false;
if (!currentUnreadMarker) {
const nextMessage = sortedReportActions[index + 1];
const isCurrentMessageUnread = isMessageUnread(reportAction, report.lastReadTime);
shouldDisplay = isCurrentMessageUnread && (!nextMessage || !isMessageUnread(nextMessage, report.lastReadTime));
const isCurrentMessageUnread = isMessageUnread(reportAction, lastReadTimeRef.current);
shouldDisplay = isCurrentMessageUnread && (!nextMessage || !isMessageUnread(nextMessage, lastReadTimeRef.current));
if (!messageManuallyMarkedUnread) {
shouldDisplay = shouldDisplay && reportAction.actorAccountID !== Report.getCurrentUserAccountID();
}
if (shouldDisplay) {
cacheUnreadMarkers.set(report.reportID, reportAction.reportActionID);
}
} else {
shouldDisplay = reportAction.reportActionID === currentUnreadMarker;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

NAB, we can remove this line

return shouldDisplay;
},
[currentUnreadMarker, sortedReportActions, report.lastReadTime, messageManuallyMarkedUnread],
[currentUnreadMarker, sortedReportActions, report.reportID, messageManuallyMarkedUnread],
);

useEffect(() => {
Expand All @@ -327,13 +364,14 @@ function ReportActionsList({
}
markerFound = true;
if (!currentUnreadMarker && currentUnreadMarker !== reportAction.reportActionID) {
cacheUnreadMarkers.set(report.reportID, reportAction.reportActionID);
setCurrentUnreadMarker(reportAction.reportActionID);
}
});
if (!markerFound) {
setCurrentUnreadMarker(null);
}
}, [sortedReportActions, report.lastReadTime, messageManuallyMarkedUnread, shouldDisplayNewMarker, currentUnreadMarker]);
}, [sortedReportActions, report.lastReadTime, report.reportID, messageManuallyMarkedUnread, shouldDisplayNewMarker, currentUnreadMarker]);

const renderItem = useCallback(
({item: reportAction, index}) => (
Expand Down
18 changes: 14 additions & 4 deletions tests/ui/UnreadIndicatorsTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {addSeconds, format, subMinutes, subSeconds} from 'date-fns';
import {utcToZonedTime} from 'date-fns-tz';
import lodashGet from 'lodash/get';
import React from 'react';
import {AppState, Linking} from 'react-native';
import {AppState, DeviceEventEmitter, Linking} from 'react-native';
import Onyx from 'react-native-onyx';
import App from '../../src/App';
import CONFIG from '../../src/CONFIG';
Expand Down Expand Up @@ -78,7 +78,7 @@ function scrollUpToRevealNewMessagesBadge() {
function isNewMessagesBadgeVisible() {
const hintText = Localize.translateLocal('accessibilityHints.scrollToNewestMessages');
const badge = screen.queryByAccessibilityHint(hintText);
return Math.round(badge.props.style.transform[0].translateY) === 10;
return Math.round(badge.props.style.transform[0].translateY) === -40;
}

/**
Expand Down Expand Up @@ -249,8 +249,12 @@ describe('Unread Indicators', () => {
signInAndGetAppWithUnreadChat()
// Navigate to the unread chat from the sidebar
.then(() => navigateToSidebarOption(0))
// Navigate to the unread chat from the sidebar
.then(() => navigateToSidebarOption(0))
.then(() => {
// Verify the unread indicator is present
const newMessageLineIndicatorHintText = Localize.translateLocal('accessibilityHints.newMessageLineIndicator');
const unreadIndicator = screen.queryAllByLabelText(newMessageLineIndicatorHintText);
expect(unreadIndicator).toHaveLength(1);
})
.then(() => {
expect(areYouOnChatListScreen()).toBe(false);
// Then navigate back to the sidebar
Expand All @@ -259,9 +263,15 @@ describe('Unread Indicators', () => {
.then(() => {
// Verify the LHN is now open
expect(areYouOnChatListScreen()).toBe(true);

// Tap on the chat again
return navigateToSidebarOption(0);
})
.then(() => {
// Sending event to clear the unread indicator cache, given that the test doesn't behave as the app
Copy link
Contributor

Choose a reason for hiding this comment

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

given that the test doesn't behave as the app
What does this mean?

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 there is a single panel, in the tests, seems that it keeps all components in memory, so I need to reset the state to make it work, "simulating" the same as what the app does

DeviceEventEmitter.emit(`unreadAction_${REPORT_ID}`, format(new Date(), CONST.DATE.FNS_DB_FORMAT_STRING));
return waitForBatchedUpdatesWithAct();
})
.then(() => {
// Verify the unread indicator is not present
const newMessageLineIndicatorHintText = Localize.translateLocal('accessibilityHints.newMessageLineIndicator');
Expand Down
Loading