-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Changes from 12 commits
fab7506
47dd5cd
922fbf3
85dddae
b9f1f17
ec2e0fb
3646848
c71ecae
ebdeddb
de5b8fe
c483c10
8925515
7149558
89d15d2
748c8ad
a434c3a
f40773b
c784057
51f8117
eee1e83
f1de924
c8d4853
c9722d9
8782bf7
d3ddefd
cea0759
3ff86e2
3630dac
57978b6
6546ba5
6b2beed
dc93b9e
95cc451
cdb1b80
5dde6f2
2b83ba6
c4541fb
58e2f9c
12a92a7
823f3b7
d1a347c
0a578dd
8bb02a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ import React from 'react'; | |
import _ from 'underscore'; | ||
import ExpensiMark from 'expensify-common/lib/ExpensiMark'; | ||
import lodashGet from 'lodash/get'; | ||
import {DeviceEventEmitter} from 'react-native'; | ||
import * as Expensicons from '../../../../components/Icon/Expensicons'; | ||
import * as Report from '../../../../libs/actions/Report'; | ||
import * as Download from '../../../../libs/actions/Download'; | ||
|
@@ -251,7 +252,8 @@ export default [ | |
shouldShow: (type, reportAction, isArchivedRoom, betas, anchor, isChronosReport, reportID, isPinnedChat, isUnreadChat) => | ||
type === CONTEXT_MENU_TYPES.REPORT_ACTION || (type === CONTEXT_MENU_TYPES.REPORT && !isUnreadChat), | ||
onPress: (closePopover, {reportAction, reportID}) => { | ||
Report.markCommentAsUnread(reportID, reportAction.created); | ||
const lastReadTime = Report.markCommentAsUnread(reportID, reportAction.created); | ||
DeviceEventEmitter.emit(`unreadAction_${reportID}`, lastReadTime); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we inside from inside the action? I think that would be better since we won't be requiring the action to return stuff. |
||
if (closePopover) { | ||
hideContextMenu(true, ReportActionComposeFocusManager.focus); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,27 +2,28 @@ import PropTypes from 'prop-types'; | |
import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react'; | ||
import Animated, {useAnimatedStyle, useSharedValue, withTiming} from 'react-native-reanimated'; | ||
import _ from 'underscore'; | ||
import {DeviceEventEmitter} from 'react-native'; | ||
import {useRoute} from '@react-navigation/native'; | ||
import lodashGet from 'lodash/get'; | ||
import compose from '../../../libs/compose'; | ||
import styles from '../../../styles/styles'; | ||
import * as ReportUtils from '../../../libs/ReportUtils'; | ||
import * as Report from '../../../libs/actions/Report'; | ||
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions'; | ||
import withCurrentUserPersonalDetails, {withCurrentUserPersonalDetailsPropTypes, withCurrentUserPersonalDetailsDefaultProps} from '../../../components/withCurrentUserPersonalDetails'; | ||
import CONST from '../../../CONST'; | ||
import InvertedFlatList from '../../../components/InvertedFlatList'; | ||
import {withPersonalDetails} from '../../../components/OnyxProvider'; | ||
import ReportActionsSkeletonView from '../../../components/ReportActionsSkeletonView'; | ||
import withCurrentUserPersonalDetails, {withCurrentUserPersonalDetailsDefaultProps, withCurrentUserPersonalDetailsPropTypes} from '../../../components/withCurrentUserPersonalDetails'; | ||
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions'; | ||
import variables from '../../../styles/variables'; | ||
import reportActionPropTypes from './reportActionPropTypes'; | ||
import useLocalize from '../../../hooks/useLocalize'; | ||
import useNetwork from '../../../hooks/useNetwork'; | ||
import useReportScrollManager from '../../../hooks/useReportScrollManager'; | ||
import DateUtils from '../../../libs/DateUtils'; | ||
import * as ReportUtils from '../../../libs/ReportUtils'; | ||
import * as Report from '../../../libs/actions/Report'; | ||
import compose from '../../../libs/compose'; | ||
import styles from '../../../styles/styles'; | ||
import variables from '../../../styles/variables'; | ||
import reportPropTypes from '../../reportPropTypes'; | ||
import FloatingMessageCounter from './FloatingMessageCounter'; | ||
import ReportActionsListItemRenderer from './ReportActionsListItemRenderer'; | ||
import reportActionPropTypes from './reportActionPropTypes'; | ||
|
||
const propTypes = { | ||
/** The report currently being looked at */ | ||
|
@@ -74,16 +75,15 @@ 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 = {}; | ||
|
||
// We cache the unread markers for each report, because the unread marker isn't | ||
// kept between reports. | ||
const cacheUnreadMarkers = new Map(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
/** | ||
* 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 | ||
|
@@ -122,10 +122,19 @@ function ReportActionsList({ | |
const route = useRoute(); | ||
const opacity = useSharedValue(0); | ||
const userActiveSince = useRef(null); | ||
const [currentUnreadMarker, setCurrentUnreadMarker] = useState(null); | ||
const prevReportID = useRef(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 reportActionSize = useRef(sortedReportActions.length); | ||
const lastReadRef = useRef(report.lastReadTime); | ||
MonilBhavsar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const firstRenderRef = useRef(true); | ||
const linkedReportActionID = lodashGet(route, 'params.reportActionID', ''); | ||
|
||
|
@@ -145,16 +154,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 (userActiveSince.current && prevReportID && prevReportID !== report.reportID) { | ||
if (userActiveSince.current && prevReportID.current && prevReportID.current !== report.reportID) { | ||
userActiveSince.current = null; | ||
} else { | ||
userActiveSince.current = DateUtils.getDBTime(); | ||
} | ||
prevReportID = report.reportID; | ||
prevReportID.current = report.reportID; | ||
}, [report.reportID]); | ||
|
||
useEffect(() => { | ||
if (!userActiveSince.current || report.reportID !== prevReportID) { | ||
if (!userActiveSince.current || report.reportID !== prevReportID.current) { | ||
return; | ||
} | ||
|
||
|
@@ -170,25 +179,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.current) { | ||
return; | ||
} | ||
|
||
if (!messageManuallyMarkedUnread && lastReadRef.current && lastReadRef.current < report.lastReadTime) { | ||
MonilBhavsar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cacheUnreadMarkers.delete(report.reportID); | ||
} | ||
lastReadRef.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; | ||
} | ||
|
||
// Need to listen for the specific reportID, otherwise we could be listening to all the reports | ||
MonilBhavsar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
unreadActionSubscription.current = DeviceEventEmitter.addListener(`unreadAction_${report.reportID}`, (newLastReadTime) => { | ||
cacheUnreadMarkers.delete(report.reportID); | ||
lastReadRef.current = newLastReadTime; | ||
setCurrentUnreadMarker(null); | ||
setMessageManuallyMarkedUnread(new Date().getTime()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't seen this event driven pattern used in our app before. Would you mind explaining why this is needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was added here point 3. |
||
}); | ||
}, [report.reportID]); | ||
|
||
useEffect(() => { | ||
// Why are we doing this, when in the cleanup of the useEffect we are already calling the unsubscribe function? | ||
|
@@ -285,19 +310,23 @@ function ReportActionsList({ | |
const renderItem = useCallback( | ||
({item: reportAction, index}) => { | ||
let shouldDisplayNewMarker = false; | ||
|
||
if (!currentUnreadMarker) { | ||
const nextMessage = sortedReportActions[index + 1]; | ||
const isCurrentMessageUnread = isMessageUnread(reportAction, report.lastReadTime); | ||
shouldDisplayNewMarker = isCurrentMessageUnread && !isMessageUnread(nextMessage, report.lastReadTime); | ||
const isCurrentMessageUnread = isMessageUnread(reportAction, lastReadRef.current); | ||
let canDisplayNewMarker = isCurrentMessageUnread && !isMessageUnread(nextMessage, lastReadRef.current); | ||
|
||
MonilBhavsar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!messageManuallyMarkedUnread) { | ||
shouldDisplayNewMarker = shouldDisplayNewMarker && reportAction.actorAccountID !== Report.getCurrentUserAccountID(); | ||
canDisplayNewMarker = canDisplayNewMarker && reportAction.actorAccountID !== Report.getCurrentUserAccountID(); | ||
} | ||
const canDisplayMarker = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < userActiveSince.current : true; | ||
|
||
if (!currentUnreadMarker && shouldDisplayNewMarker && canDisplayMarker) { | ||
let isMessageInScope = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < userActiveSince.current : true; | ||
if (messageManuallyMarkedUnread) { | ||
isMessageInScope = true; | ||
} | ||
if (!currentUnreadMarker && canDisplayNewMarker && isMessageInScope) { | ||
cacheUnreadMarkers.set(report.reportID, reportAction.reportActionID); | ||
setCurrentUnreadMarker(reportAction.reportActionID); | ||
shouldDisplayNewMarker = true; | ||
} | ||
} else { | ||
shouldDisplayNewMarker = reportAction.reportActionID === currentUnreadMarker; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird. I'm not sure if we're returning unrelated parameters from an action elsewhere. A better way would be to just use onyx to get
lastReadTime
from the report.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was for emitting the even, but I will move the emitting into the function, so it won't need to return it