-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Feat: Display unread marker for messages that were received while offline #49480
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
Merged
tylerkaraszewski
merged 26 commits into
Expensify:main
from
margelo:@chrispader/unread-marker-not-displayed
Oct 31, 2024
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
c7d3751
feat: unread marker for messages while offline
chrispader 64644ac
Merge branch 'Expensify:main' into @chrispader/unread-marker-not-disp…
chrispader fcc738c
Merge branch 'main' into @chrispader/unread-marker-not-displayed
chrispader 6d4ff5a
add comments
chrispader 077ba40
add comment and fix logic
chrispader 51bd4c2
Merge branch 'main' into @chrispader/unread-marker-not-displayed
chrispader 3c4ad02
fix: don't show unread marker for own messages
chrispader 31dfa77
Merge branch 'main' into @chrispader/unread-marker-not-displayed
chrispader 744e54c
re-structure offline message detection logic
chrispader 9a639a7
Merge branch 'main' into @chrispader/unread-marker-not-displayed
chrispader d7465bf
simplify offline message check
chrispader dd99fe7
simplify code
chrispader 0678a25
WIP: improve unread marker check
chrispader 1900337
fix: unread marker not shown
chrispader 40d0acf
Merge branch 'main' into @chrispader/unread-marker-not-displayed
chrispader 9d66dbb
fix: move offline/online at logic into seperate hook and defer values
chrispader c446f1f
fix: use ref instead of state for lastOffline/lastOnline at
chrispader e524d38
fix: move ReportAction util functions
chrispader 35e071b
Merge branch 'main' into @chrispader/unread-marker-not-displayed
chrispader 3a06c85
fix: fix and simplify offline message check
chrispader 91bbc8f
fix: wrong parameters
chrispader 4cf4ff5
fix: remove dependency array eslint exception
chrispader 9c86695
fix: re-implement offline message logic
chrispader 2af086d
Merge branch 'main' into @chrispader/unread-marker-not-displayed
chrispader 9c4344c
Merge branch 'main' into @chrispader/unread-marker-not-displayed
chrispader d0f6d94
fix: wrong condition in early return
chrispader File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| import type {MutableRefObject} from 'react'; | ||
| import {useEffect, useRef} from 'react'; | ||
| import DateUtils from '@libs/DateUtils'; | ||
| import useLocalize from './useLocalize'; | ||
| import useNetwork from './useNetwork'; | ||
|
|
||
| type UseNetworkWithOfflineStatus = {isOffline: boolean; lastOfflineAt: MutableRefObject<Date | undefined>; lastOnlineAt: MutableRefObject<Date | undefined>}; | ||
|
|
||
| export default function useNetworkWithOfflineStatus(): UseNetworkWithOfflineStatus { | ||
| const {isOffline} = useNetwork(); | ||
| const {preferredLocale} = useLocalize(); | ||
|
|
||
| // The last time/date the user went/was offline. If the user was never offline, it is set to undefined. | ||
| const lastOfflineAt = useRef(isOffline ? DateUtils.getLocalDateFromDatetime(preferredLocale) : undefined); | ||
|
|
||
| // The last time/date the user went/was online. If the user was never online, it is set to undefined. | ||
| const lastOnlineAt = useRef(isOffline ? undefined : DateUtils.getLocalDateFromDatetime(preferredLocale)); | ||
|
|
||
| useEffect(() => { | ||
| if (isOffline) { | ||
| lastOfflineAt.current = DateUtils.getLocalDateFromDatetime(preferredLocale); | ||
| } else { | ||
| lastOnlineAt.current = DateUtils.getLocalDateFromDatetime(preferredLocale); | ||
| } | ||
| }, [isOffline, preferredLocale]); | ||
|
|
||
| return {isOffline, lastOfflineAt, lastOnlineAt}; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ import {AUTOSCROLL_TO_TOP_THRESHOLD} from '@components/InvertedFlatList/BaseInve | |
| import {usePersonalDetails} from '@components/OnyxProvider'; | ||
| import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; | ||
| import useLocalize from '@hooks/useLocalize'; | ||
| import useNetwork from '@hooks/useNetwork'; | ||
| import useNetworkWithOfflineStatus from '@hooks/useNetworkWithOfflineStatus'; | ||
| import usePrevious from '@hooks/usePrevious'; | ||
| import useReportScrollManager from '@hooks/useReportScrollManager'; | ||
| import useResponsiveLayout from '@hooks/useResponsiveLayout'; | ||
|
|
@@ -123,14 +123,6 @@ function keyExtractor(item: OnyxTypes.ReportAction): string { | |
| return item.reportActionID; | ||
| } | ||
|
|
||
| function isMessageUnread(message: OnyxTypes.ReportAction, lastReadTime?: string): boolean { | ||
| if (!lastReadTime) { | ||
| return !ReportActionsUtils.isCreatedAction(message); | ||
| } | ||
|
|
||
| return !!(message && lastReadTime && message.created && lastReadTime < message.created); | ||
| } | ||
|
|
||
| const onScrollToIndexFailed = () => {}; | ||
|
|
||
| function ReportActionsList({ | ||
|
|
@@ -162,7 +154,8 @@ function ReportActionsList({ | |
| const {windowHeight} = useWindowDimensions(); | ||
| const {isInNarrowPaneModal, shouldUseNarrowLayout} = useResponsiveLayout(); | ||
|
|
||
| const {isOffline} = useNetwork(); | ||
| const {preferredLocale} = useLocalize(); | ||
| const {isOffline, lastOfflineAt, lastOnlineAt} = useNetworkWithOfflineStatus(); | ||
| const route = useRoute<RouteProp<AuthScreensParamList, typeof SCREENS.REPORT>>(); | ||
| const reportScrollManager = useReportScrollManager(); | ||
| const userActiveSince = useRef<string>(DateUtils.getDBTime()); | ||
|
|
@@ -226,32 +219,81 @@ function ReportActionsList({ | |
| }, [report.reportID]); | ||
|
|
||
| const prevUnreadMarkerReportActionID = useRef<string | null>(null); | ||
| /** | ||
| * Whether a message is NOT from the active user and it was received while the user was offline. | ||
| */ | ||
| const wasMessageReceivedWhileOffline = useCallback( | ||
| (message: OnyxTypes.ReportAction) => | ||
| !ReportActionsUtils.wasActionTakenByCurrentUser(message) && | ||
| ReportActionsUtils.wasActionCreatedWhileOffline(message, isOffline, lastOfflineAt.current, lastOnlineAt.current, preferredLocale), | ||
|
Contributor
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. Coming from #55560, We missed a case here, when we get an optimistic message from Concierge offline, we fixed it by returning false if the message is optimistic |
||
| [isOffline, lastOfflineAt, lastOnlineAt, preferredLocale], | ||
| ); | ||
|
|
||
| /** | ||
| * The index of the earliest message that was received while offline | ||
| */ | ||
| const earliestReceivedOfflineMessageIndex = useMemo(() => { | ||
| // Create a list of (sorted) indices of message that were received while offline | ||
| const receviedOfflineMessages = sortedReportActions.reduce<number[]>((acc, message, index) => { | ||
| if (wasMessageReceivedWhileOffline(message)) { | ||
| acc[index] = index; | ||
| } | ||
|
|
||
| return acc; | ||
| }, []); | ||
|
|
||
| // The last index in the list is the earliest message that was received while offline | ||
| return receviedOfflineMessages.at(-1); | ||
| }, [sortedReportActions, wasMessageReceivedWhileOffline]); | ||
|
|
||
| /** | ||
| * The reportActionID the unread marker should display above | ||
| */ | ||
| const unreadMarkerReportActionID = useMemo(() => { | ||
| const shouldDisplayNewMarker = (reportAction: OnyxTypes.ReportAction, index: number): boolean => { | ||
| const shouldDisplayNewMarker = (message: OnyxTypes.ReportAction, index: number): boolean => { | ||
| const nextMessage = sortedVisibleReportActions.at(index + 1); | ||
| const isCurrentMessageUnread = isMessageUnread(reportAction, unreadMarkerTime); | ||
| const isNextMessageRead = !nextMessage || !isMessageUnread(nextMessage, unreadMarkerTime); | ||
| const shouldDisplay = isCurrentMessageUnread && isNextMessageRead && !ReportActionsUtils.shouldHideNewMarker(reportAction); | ||
| const isWithinVisibleThreshold = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < (userActiveSince.current ?? '') : true; | ||
| const isNextMessageUnread = !!nextMessage && ReportActionsUtils.isReportActionUnread(nextMessage, unreadMarkerTime); | ||
|
|
||
| // If the current message is the earliest message received while offline, we want to display the unread marker above this message. | ||
| const isEarliestReceivedOfflineMessage = index === earliestReceivedOfflineMessageIndex; | ||
| if (isEarliestReceivedOfflineMessage && !isNextMessageUnread) { | ||
| return true; | ||
| } | ||
|
|
||
| const isWithinVisibleThreshold = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? message.created < (userActiveSince.current ?? '') : true; | ||
|
|
||
| // If the unread marker should be hidden or is not within the visible area, don't show the unread marker. | ||
| if (ReportActionsUtils.shouldHideNewMarker(message) || !isWithinVisibleThreshold) { | ||
| return false; | ||
| } | ||
|
|
||
| const isCurrentMessageUnread = ReportActionsUtils.isReportActionUnread(message, unreadMarkerTime); | ||
|
|
||
| // If the current message is read or the next message is unread, don't show the unread marker. | ||
| if (!isCurrentMessageUnread || isNextMessageUnread) { | ||
| return false; | ||
| } | ||
|
|
||
| // If no unread marker exists, don't set an unread marker for newly added messages from the current user. | ||
| const isFromCurrentUser = accountID === (ReportActionsUtils.isReportPreviewAction(reportAction) ? !reportAction.childLastActorAccountID : reportAction.actorAccountID); | ||
| const isNewMessage = !prevSortedVisibleReportActionsObjects[reportAction.reportActionID]; | ||
| const isFromCurrentUser = accountID === (ReportActionsUtils.isReportPreviewAction(message) ? !message.childLastActorAccountID : message.actorAccountID); | ||
| const isNewMessage = !prevSortedVisibleReportActionsObjects[message.reportActionID]; | ||
|
|
||
| // The unread marker will show if the action's `created` time is later than `unreadMarkerTime`. | ||
| // The `unreadMarkerTime` has already been updated to match the optimistic action created time, | ||
| // but once the new action is saved on the backend, the actual created time will be later than the optimistic one. | ||
| // Therefore, we also need to prevent the unread marker from appearing for previously optimistic actions. | ||
| const isPreviouslyOptimistic = !!prevSortedVisibleReportActionsObjects[reportAction.reportActionID]?.isOptimisticAction && !reportAction.isOptimisticAction; | ||
| const isPreviouslyOptimistic = !!prevSortedVisibleReportActionsObjects[message.reportActionID]?.isOptimisticAction && !message.isOptimisticAction; | ||
| const shouldIgnoreUnreadForCurrentUserMessage = !prevUnreadMarkerReportActionID.current && isFromCurrentUser && (isNewMessage || isPreviouslyOptimistic); | ||
|
|
||
| return shouldDisplay && isWithinVisibleThreshold && !shouldIgnoreUnreadForCurrentUserMessage; | ||
| return !shouldIgnoreUnreadForCurrentUserMessage; | ||
| }; | ||
|
|
||
| // If there are message that were recevied while offline, | ||
| // we can skip checking all messages later than the earliest recevied offline message. | ||
| const startIndex = earliestReceivedOfflineMessageIndex ?? 0; | ||
|
|
||
| // Scan through each visible report action until we find the appropriate action to show the unread marker | ||
| for (let index = 0; index < sortedVisibleReportActions.length; index++) { | ||
| for (let index = startIndex; index < sortedVisibleReportActions.length; index++) { | ||
| const reportAction = sortedVisibleReportActions.at(index); | ||
|
|
||
| // eslint-disable-next-line react-compiler/react-compiler | ||
|
|
@@ -261,7 +303,7 @@ function ReportActionsList({ | |
| } | ||
|
|
||
| return null; | ||
| }, [sortedVisibleReportActions, unreadMarkerTime, accountID, prevSortedVisibleReportActionsObjects]); | ||
| }, [accountID, earliestReceivedOfflineMessageIndex, prevSortedVisibleReportActionsObjects, sortedVisibleReportActions, unreadMarkerTime]); | ||
| prevUnreadMarkerReportActionID.current = unreadMarkerReportActionID; | ||
|
|
||
| /** | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Coming from #55990. We missed a case here, where if you received a message from a chat that's not open, then go offline and open that report, the new message marker won't show. This is because
lastOnlineAtwill be undefined here as it's stored in component state. More details.