From c52f485375c7d18b78913f579b05ac93e73c3de8 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 2 Jul 2026 15:51:38 +0000 Subject: [PATCH 01/43] Show New marker when user marks their own message as unread Co-authored-by: Olly --- .../report/shouldDisplayNewMarkerOnReportAction.ts | 12 ++++-------- tests/unit/ReportActionsUtilsTest.ts | 4 ++-- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts index eb14e111fc88..09fb368bfaf0 100644 --- a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts +++ b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts @@ -46,7 +46,6 @@ const shouldDisplayNewMarkerOnReportAction = ({ prevSortedVisibleReportActionsObjects, isScrolledOverThreshold, isOffline, - prevUnreadMarkerReportActionID, hasWindowFocus = true, }: ShouldDisplayNewMarkerOnReportActionParams): boolean => { const isNextMessageUnread = !!nextMessage && isReportActionUnread(nextMessage, unreadMarkerTime); @@ -86,13 +85,10 @@ const shouldDisplayNewMarkerOnReportAction = ({ const shouldIgnoreUnreadForCurrentUserMessage = isNewMessage || isPreviouslyOptimistic; if (isFromCurrentUser) { - // When an existing marker is being relocated (e.g. after the original unread message is deleted), - // allow the marker to land on a self-authored action. - // Otherwise, never anchor the "New" marker above a self-authored action on first open/re-entry. - if (prevUnreadMarkerReportActionID) { - return !shouldIgnoreUnreadForCurrentUserMessage; - } - return false; + // Only suppress the "New" marker for a self-authored message that was just sent (newly added or still + // transitioning from an optimistic action). An existing self-authored action that the user explicitly + // marked as unread should anchor the marker even when no marker exists yet (e.g. on first open/re-entry). + return !shouldIgnoreUnreadForCurrentUserMessage; } return !isNewMessage || isScrolledOverThreshold || !hasWindowFocus; diff --git a/tests/unit/ReportActionsUtilsTest.ts b/tests/unit/ReportActionsUtilsTest.ts index 15b9667f024b..51fdd7dfc8e0 100644 --- a/tests/unit/ReportActionsUtilsTest.ts +++ b/tests/unit/ReportActionsUtilsTest.ts @@ -5466,7 +5466,7 @@ describe('ReportActionsUtils', () => { ).toBe(false); }); - it('returns false when message is from current user and is already present (not new, not optimistic) and no existing marker', () => { + it('returns true when the current user explicitly marks their own already-present message as unread and no marker exists yet', () => { const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'existing-action-id'}); const prevSortedVisibleReportActionsObjects = { [message.reportActionID]: makeAction({actorAccountID: currentUserAccountID, reportActionID: 'existing-action-id'}), @@ -5479,7 +5479,7 @@ describe('ReportActionsUtils', () => { prevUnreadMarkerReportActionID: null, isOffline: false, }), - ).toBe(false); + ).toBe(true); }); it('returns true when message is from current user, already present, and marker is being relocated after deletion', () => { From 669f3f533a3267054da1ddcec85b1de0292159a1 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Wed, 15 Jul 2026 16:35:45 +0000 Subject: [PATCH 02/43] Re-trigger CI (flaky iOS build) Co-authored-by: Olly From 5f489a4f68242d4f37bcbff23140c742bb57d69c Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 23 Jul 2026 14:57:52 +0000 Subject: [PATCH 03/43] Anchor New marker on self-marked-unread action via stable reportActionID Persist the explicitly marked-unread reportActionID on the report so the New marker can anchor on a self-authored action across the optimistic to confirmed transition, where all timestamp signals (lastReadTime, unreadMarkerTime, created) drift and isReportActionUnread wrongly reports the action as read. Co-authored-by: Olly --- .../MoneyRequestReportActionsList.tsx | 1 + src/hooks/useUnreadMarker.ts | 6 ++ src/libs/actions/Report/index.ts | 3 + .../shouldDisplayNewMarkerOnReportAction.ts | 18 ++++++ src/types/onyx/Report.ts | 4 ++ tests/unit/ReportActionsUtilsTest.ts | 59 +++++++++++++++++++ 6 files changed, 91 insertions(+) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx index 4105ad51d902..96b81f22366f 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx @@ -425,6 +425,7 @@ function MoneyRequestReportActionsList({onLayout}: MoneyRequestReportListProps) isScrolledOverThreshold: scrollingVerticalBottomOffset.current >= CONST.REPORT.ACTIONS.ACTION_VISIBLE_THRESHOLD, isOffline, isReversed: true, + manuallyMarkedUnreadReportActionID: report?.manuallyMarkedUnreadReportActionID, hasWindowFocus: Visibility.hasFocus(), }); diff --git a/src/hooks/useUnreadMarker.ts b/src/hooks/useUnreadMarker.ts index 4c5812d182c1..b21138363980 100644 --- a/src/hooks/useUnreadMarker.ts +++ b/src/hooks/useUnreadMarker.ts @@ -41,6 +41,7 @@ type UseUnreadMarkerResult = { }; const lastReadTimeSelector = (report: OnyxTypes.Report | undefined) => report?.lastReadTime ?? ''; +const manuallyMarkedUnreadReportActionIDSelector = (report: OnyxTypes.Report | undefined) => report?.manuallyMarkedUnreadReportActionID ?? null; function useUnreadMarker({ reportID, @@ -60,6 +61,10 @@ function useUnreadMarker({ }); const reportLastReadTime = reportLastReadTimeValue ?? ''; + const [manuallyMarkedUnreadReportActionID] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, { + selector: manuallyMarkedUnreadReportActionIDSelector, + }); + const [unreadMarkerTime, setUnreadMarkerTime] = useState(reportLastReadTime); if (unreadMarkerTime === '' && reportLastReadTime !== '') { @@ -121,6 +126,7 @@ function useUnreadMarker({ isReversed: false, isAnonymousUser, prevUnreadMarkerReportActionID, + manuallyMarkedUnreadReportActionID, hasWindowFocus: Visibility.hasFocus(), }); // Pagination is anchored to the oldest unread on first open; that anchor does not change when the user diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 657ad3192921..dd336271c38f 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -2769,6 +2769,7 @@ function readNewestAction(reportID: string | undefined, isReportActionsLoaded: b key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, value: { lastReadTime, + manuallyMarkedUnreadReportActionID: null, }, }, ]; @@ -2845,6 +2846,7 @@ function markCommentAsUnread(reportID: string | undefined, reportActions: OnyxEn const reportValue = { lastReadTime, + manuallyMarkedUnreadReportActionID: reportAction?.reportActionID ?? null, ...(lastActorAccountID && {lastActorAccountID}), }; @@ -2871,6 +2873,7 @@ function markCommentAsUnread(reportID: string | undefined, reportActions: OnyxEn value: { lastReadTime: report?.lastReadTime ?? null, lastActorAccountID: report?.lastActorAccountID ?? null, + manuallyMarkedUnreadReportActionID: report?.manuallyMarkedUnreadReportActionID ?? null, }, }, ]; diff --git a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts index 09fb368bfaf0..2ebf05aa5ae4 100644 --- a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts +++ b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts @@ -29,6 +29,9 @@ type ShouldDisplayNewMarkerOnReportActionParams = { /** The reportActionID of the current unread marker, if one exists */ prevUnreadMarkerReportActionID?: string | null; + + /** The reportActionID the user explicitly marked as unread, if any */ + manuallyMarkedUnreadReportActionID?: string | null; /** Whether the app window is focused */ hasWindowFocus?: boolean; }; @@ -46,6 +49,7 @@ const shouldDisplayNewMarkerOnReportAction = ({ prevSortedVisibleReportActionsObjects, isScrolledOverThreshold, isOffline, + manuallyMarkedUnreadReportActionID, hasWindowFocus = true, }: ShouldDisplayNewMarkerOnReportActionParams): boolean => { const isNextMessageUnread = !!nextMessage && isReportActionUnread(nextMessage, unreadMarkerTime); @@ -60,6 +64,15 @@ const shouldDisplayNewMarkerOnReportAction = ({ return false; } + // The user explicitly marked THIS action as unread. Anchor the marker here regardless of the + // timestamp-based check below: once an optimistic self-message confirms, unreadMarkerTime, + // lastReadTime, and created all converge on (or drift past) the confirmed `created`, so + // isReportActionUnread wrongly reports it as read. The stored reportActionID is the only signal + // stable across that transition. Still yield to a more-recent unread message. + if (!!manuallyMarkedUnreadReportActionID && message.reportActionID === manuallyMarkedUnreadReportActionID && !isNextMessageUnread) { + return true; + } + const isCurrentMessageUnread = isReportActionUnread(message, unreadMarkerTime); // If the current message is read or the next message is unread, don't show the unread marker. @@ -126,6 +139,9 @@ type GetUnreadMarkerReportActionParams = { /** The reportActionID of the current unread marker, if one exists */ prevUnreadMarkerReportActionID?: string | null; + + /** The reportActionID the user explicitly marked as unread, if any */ + manuallyMarkedUnreadReportActionID?: string | null; /** Whether the app window is focused */ hasWindowFocus?: boolean; }; @@ -145,6 +161,7 @@ const getUnreadMarkerReportAction = ({ isReversed, isAnonymousUser = false, prevUnreadMarkerReportActionID, + manuallyMarkedUnreadReportActionID, hasWindowFocus = true, }: GetUnreadMarkerReportActionParams): [string | null, number] => { if (isAnonymousUser) { @@ -186,6 +203,7 @@ const getUnreadMarkerReportAction = ({ isScrolledOverThreshold, isOffline, prevUnreadMarkerReportActionID, + manuallyMarkedUnreadReportActionID, hasWindowFocus, }); diff --git a/src/types/onyx/Report.ts b/src/types/onyx/Report.ts index 61abcfc73dce..1f9344c51b9e 100644 --- a/src/types/onyx/Report.ts +++ b/src/types/onyx/Report.ts @@ -131,6 +131,10 @@ type Report = OnyxCommon.OnyxValueWithOfflineFeedback< /** The time when user read the last message */ lastReadTime?: string; + /** reportActionID the user explicitly marked as unread. Stable across the optimistic→confirmed + * transition, unlike lastReadTime, so the "New" marker can anchor on a self-authored action. */ + manuallyMarkedUnreadReportActionID?: string | null; + /** The sequence number of the last report visit */ lastReadSequenceNumber?: number; diff --git a/tests/unit/ReportActionsUtilsTest.ts b/tests/unit/ReportActionsUtilsTest.ts index 51fdd7dfc8e0..75570869fddf 100644 --- a/tests/unit/ReportActionsUtilsTest.ts +++ b/tests/unit/ReportActionsUtilsTest.ts @@ -5539,6 +5539,65 @@ describe('ReportActionsUtils', () => { }), ).toBe(true); }); + + it('anchors the marker on the explicitly marked-unread action even after its confirmed created drifts before unreadMarkerTime', () => { + // Simulates the offline→online case: an optimistic self-message the user marked unread confirms + // with a `created` that lands before unreadMarkerTime, so the timestamp check reads it as "read". + // The stable manuallyMarkedUnreadReportActionID must still anchor the marker here. + const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'marked-action-id', pendingAction: null, created: '2023-01-01 09:00:00.000'}); + const prevSortedVisibleReportActionsObjects = { + [message.reportActionID]: makeAction({ + actorAccountID: currentUserAccountID, + reportActionID: 'marked-action-id', + pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD, + }), + }; + expect( + shouldDisplayNewMarkerOnReportAction({ + ...baseParams, + message, + prevSortedVisibleReportActionsObjects, + manuallyMarkedUnreadReportActionID: 'marked-action-id', + isOffline: false, + }), + ).toBe(true); + }); + + it('does not anchor the marker on a just-sent self-message when no action is marked unread', () => { + // Same confirmed self-message, but nothing is marked unread. The stable-id override is skipped and + // the existing just-sent suppression applies, keeping the #91443 fix intact. + const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'confirmed-action-id', pendingAction: null}); + const prevSortedVisibleReportActionsObjects = { + [message.reportActionID]: makeAction({ + actorAccountID: currentUserAccountID, + reportActionID: 'confirmed-action-id', + pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD, + }), + }; + expect( + shouldDisplayNewMarkerOnReportAction({ + ...baseParams, + message, + prevSortedVisibleReportActionsObjects, + manuallyMarkedUnreadReportActionID: null, + isOffline: false, + }), + ).toBe(false); + }); + + it('yields the marker to a more-recent unread message even when an older action is marked unread', () => { + const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'marked-action-id', created: '2023-01-01 11:00:00.000'}); + const nextMessage = makeAction({created: '2023-01-01 11:30:00.000'}); + expect( + shouldDisplayNewMarkerOnReportAction({ + ...baseParams, + message, + nextMessage, + manuallyMarkedUnreadReportActionID: 'marked-action-id', + isOffline: false, + }), + ).toBe(false); + }); }); describe('getUnreadMarkerReportAction', () => { From 5513cf78441455e670af04d2c618301a76b8cc35 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 23 Jul 2026 15:12:52 +0000 Subject: [PATCH 04/43] Fix: register manuallyMarkedUnreadReportActionID in Report key enumerations Co-authored-by: Olly --- src/libs/DebugUtils.ts | 1 + src/selectors/Report.ts | 1 + src/types/utils/whitelistedReportKeys.ts | 1 + 3 files changed, 3 insertions(+) diff --git a/src/libs/DebugUtils.ts b/src/libs/DebugUtils.ts index bae429cab5c7..bcfec0c02880 100644 --- a/src/libs/DebugUtils.ts +++ b/src/libs/DebugUtils.ts @@ -596,6 +596,7 @@ function validateReportDraftProperty(key: keyof Report | keyof ReportNameValuePa lastMessageText: CONST.RED_BRICK_ROAD_PENDING_ACTION, lastVisibleActionCreated: CONST.RED_BRICK_ROAD_PENDING_ACTION, lastReadTime: CONST.RED_BRICK_ROAD_PENDING_ACTION, + manuallyMarkedUnreadReportActionID: CONST.RED_BRICK_ROAD_PENDING_ACTION, lastReadSequenceNumber: CONST.RED_BRICK_ROAD_PENDING_ACTION, lastMentionedTime: CONST.RED_BRICK_ROAD_PENDING_ACTION, policyAvatar: CONST.RED_BRICK_ROAD_PENDING_ACTION, diff --git a/src/selectors/Report.ts b/src/selectors/Report.ts index 5e288100b635..cb369b00f026 100644 --- a/src/selectors/Report.ts +++ b/src/selectors/Report.ts @@ -110,6 +110,7 @@ type ExcludedFields = ValidReportKeys< 'lastMessageText', 'lastVisibleActionCreated', 'lastReadTime', + 'manuallyMarkedUnreadReportActionID', 'lastReadSequenceNumber', 'lastMentionedTime', 'lastVisibleActionLastModified', diff --git a/src/types/utils/whitelistedReportKeys.ts b/src/types/utils/whitelistedReportKeys.ts index 8a05d84e0893..74e6ebfc9abd 100644 --- a/src/types/utils/whitelistedReportKeys.ts +++ b/src/types/utils/whitelistedReportKeys.ts @@ -15,6 +15,7 @@ type WhitelistedReport = OnyxCommon.OnyxValueWithOfflineFeedback< lastMessageText: unknown; lastVisibleActionCreated: unknown; lastReadTime: unknown; + manuallyMarkedUnreadReportActionID: unknown; lastReadSequenceNumber: unknown; lastMentionedTime: unknown; policyAvatar: unknown; From 5e78e877d99aafa882ce0f76860c0a0231d6832f Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 23 Jul 2026 15:27:07 +0000 Subject: [PATCH 05/43] Fix: add manuallyMarkedUnreadReportActionID case to validateReportDraftProperty switch Co-authored-by: Olly --- src/libs/DebugUtils.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libs/DebugUtils.ts b/src/libs/DebugUtils.ts index bcfec0c02880..355b3b7e1f41 100644 --- a/src/libs/DebugUtils.ts +++ b/src/libs/DebugUtils.ts @@ -422,6 +422,7 @@ function validateReportDraftProperty(key: keyof Report | keyof ReportNameValuePa case 'lastMessageText': case 'lastVisibleActionCreated': case 'lastReadTime': + case 'manuallyMarkedUnreadReportActionID': case 'lastMentionedTime': case 'policyAvatar': case 'policyName': From 233a43f7b45aa101a16b957dbf03a7ebba8d6a52 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Fri, 24 Jul 2026 10:31:31 +0000 Subject: [PATCH 06/43] Restore prevUnreadMarkerReportActionID guard for self-authored actions Co-authored-by: Olly --- .../shouldDisplayNewMarkerOnReportAction.ts | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts index c3ea1937c76c..57766f1ea940 100644 --- a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts +++ b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts @@ -50,6 +50,7 @@ const shouldDisplayNewMarkerOnReportAction = ({ prevSortedVisibleReportActionsObjects, isScrolledOverThreshold, isOffline, + prevUnreadMarkerReportActionID, manuallyMarkedUnreadReportActionID, hasWindowFocus = true, }: ShouldDisplayNewMarkerOnReportActionParams): boolean => { @@ -99,10 +100,16 @@ const shouldDisplayNewMarkerOnReportAction = ({ const shouldIgnoreUnreadForCurrentUserMessage = isNewMessage || isPreviouslyOptimistic; if (isFromCurrentUser) { - // Only suppress the "New" marker for a self-authored message that was just sent (newly added or still - // transitioning from an optimistic action). An existing self-authored action that the user explicitly - // marked as unread should anchor the marker even when no marker exists yet (e.g. on first open/re-entry). - return !shouldIgnoreUnreadForCurrentUserMessage; + // For a self-authored action, only move/keep the "New" marker when one already exists in this session + // (`prevUnreadMarkerReportActionID` is set). The explicit mark-as-unread case is handled earlier by the + // stable `manuallyMarkedUnreadReportActionID` check, which anchors the marker on first open/re-entry + // regardless of this guard. Without this guard, a persisted self-authored action (e.g. a reimbursable + // toggle) whose timestamps have drifted past `lastReadTime` would wrongly show the marker on a cold + // open/re-entry — the regression from Expensify/App#91940. + if (prevUnreadMarkerReportActionID) { + return !shouldIgnoreUnreadForCurrentUserMessage; + } + return false; } return !isNewMessage || isScrolledOverThreshold || !hasWindowFocus; From 8c48e5055aa0d1523cfd63937daa55a503467234 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Fri, 24 Jul 2026 11:32:40 +0000 Subject: [PATCH 07/43] Update unit test to assert self-authored cold-open guard Co-authored-by: Olly --- tests/unit/ReportActionsUtilsTest.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/unit/ReportActionsUtilsTest.ts b/tests/unit/ReportActionsUtilsTest.ts index bf746f406940..69c4419a3ce9 100644 --- a/tests/unit/ReportActionsUtilsTest.ts +++ b/tests/unit/ReportActionsUtilsTest.ts @@ -5471,7 +5471,11 @@ describe('ReportActionsUtils', () => { ).toBe(false); }); - it('returns true when the current user explicitly marks their own already-present message as unread and no marker exists yet', () => { + it('returns false for a self-authored already-present action on a cold open when no marker exists and it was not explicitly marked unread (Expensify/App#91940 guard)', () => { + // A persisted self-authored action (e.g. a reimbursable toggle) whose timestamp reads as unread must + // NOT anchor the marker on a cold open/re-entry, where prevUnreadMarkerReportActionID is null. The + // explicit mark-as-unread case is handled separately via manuallyMarkedUnreadReportActionID (covered + // by the tests below), so this guard prevents the #91940 regression without affecting it. const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'existing-action-id'}); const prevSortedVisibleReportActionsObjects = { [message.reportActionID]: makeAction({actorAccountID: currentUserAccountID, reportActionID: 'existing-action-id'}), @@ -5484,7 +5488,7 @@ describe('ReportActionsUtils', () => { prevUnreadMarkerReportActionID: null, isOffline: false, }), - ).toBe(true); + ).toBe(false); }); it('returns true when message is from current user, already present, and marker is being relocated after deletion', () => { From 4f7bc5750c9f64c3cba3646d1af1d90eee678ad3 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 30 Jul 2026 10:57:44 +0000 Subject: [PATCH 08/43] Only skip auto-read on reconnect when the marked action was previously optimistic Replaces the broad manuallyMarkedUnreadReportActionID presence-guard with a targeted one: latch whether the manually-marked action was ever seen optimistic (offline just-sent), since the optimistic->confirmed merge clears isOptimisticAction/pendingAction on the same key before the read effect re-fires on reconnect. Only bail from the auto-read in that case, so a genuinely-newer message still auto-reads normally. Applied to both useMarkAsRead and the MoneyRequestReportActionsList inline copy. Co-authored-by: Olly --- .../MoneyRequestReportActionsList.tsx | 23 ++++++++++++ src/hooks/useMarkAsRead.ts | 23 ++++++++++++ tests/unit/useMarkAsReadTest.ts | 36 +++++++++++++++++++ 3 files changed, 82 insertions(+) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx index 5b26ee73edc7..f61cdbaea9fe 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx @@ -233,6 +233,22 @@ function MoneyRequestReportActionsList({onLayout}: MoneyRequestReportListProps) const hasNewestReportAction = lastAction?.created === lastVisibleActionCreated; const userActiveSince = useRef(DateUtils.getDBTime()); + // Latches whether the action the user manually marked unread was ever seen in an optimistic (just-sent, offline) + // state. The optimistic→confirmed merge clears isOptimisticAction/pendingAction on the same key, and that confirm + // is what re-fires the read effect below on reconnect — so we must record it beforehand, while still offline. + const markedActionWasOptimisticRef = useRef(false); + useEffect(() => { + const markedID = report?.manuallyMarkedUnreadReportActionID; + if (!markedID) { + markedActionWasOptimisticRef.current = false; + return; + } + const markedAction = visibleReportActions.find((action) => action.reportActionID === markedID); + if (markedAction?.isOptimisticAction || markedAction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) { + markedActionWasOptimisticRef.current = true; + } + }, [report?.manuallyMarkedUnreadReportActionID, visibleReportActions]); + const reportActionIDs = useMemo(() => { return reportActions?.map((action) => action.reportActionID) ?? []; }, [reportActions]); @@ -375,6 +391,13 @@ function MoneyRequestReportActionsList({onLayout}: MoneyRequestReportListProps) return; } + // The user marked an optimistic self-message unread while offline; when it confirms on reconnect its `created` + // shifts, re-running this effect. Don't auto-read it away — that would clear manuallyMarkedUnreadReportActionID + // and drop the "New" marker (native always hits this, since Visibility.hasFocus() is hard-coded true there). + if (report?.manuallyMarkedUnreadReportActionID && markedActionWasOptimisticRef.current) { + return; + } + if (isUnread(report, transactionThreadReport, isReportArchived) || (lastAction && isCurrentActionUnread(report, lastAction, visibleReportActions))) { // On desktop, when the notification center is displayed, isVisible will return false. // Currently, there's no programmatic way to dismiss the notification center panel. diff --git a/src/hooks/useMarkAsRead.ts b/src/hooks/useMarkAsRead.ts index 4408ff3a1b6e..262dcec43a43 100644 --- a/src/hooks/useMarkAsRead.ts +++ b/src/hooks/useMarkAsRead.ts @@ -66,6 +66,22 @@ function useMarkAsRead({reportID, report, transactionThreadReport, sortedVisible const lastMessageTime = useRef(null); const didMarkReportAsReadInitially = useRef(false); + // Latches whether the action the user manually marked unread was ever seen in an optimistic (just-sent, offline) + // state. The optimistic→confirmed merge clears isOptimisticAction/pendingAction on the same key, and that confirm + // is what re-fires the read effect below on reconnect — so we must record it beforehand, while still offline. + const markedActionWasOptimisticRef = useRef(false); + useEffect(() => { + const markedID = report?.manuallyMarkedUnreadReportActionID; + if (!markedID) { + markedActionWasOptimisticRef.current = false; + return; + } + const markedAction = sortedVisibleReportActions.find((action) => action.reportActionID === markedID); + if (markedAction?.isOptimisticAction || markedAction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) { + markedActionWasOptimisticRef.current = true; + } + }, [report?.manuallyMarkedUnreadReportActionID, sortedVisibleReportActions]); + const lastAction = sortedVisibleReportActions.at(0); const isReportUnreadValue = isUnread(report, transactionThreadReport, isReportArchived) || (!!lastAction && isCurrentActionUnread(report, lastAction)); @@ -104,6 +120,13 @@ function useMarkAsRead({reportID, report, transactionThreadReport, sortedVisible return; } + // The user marked an optimistic self-message unread while offline; when it confirms on reconnect its `created` + // shifts, re-running this effect. Don't auto-read it away — that would clear manuallyMarkedUnreadReportActionID + // and drop the "New" marker (native always hits this, since Visibility.hasFocus() is hard-coded true there). + if (report?.manuallyMarkedUnreadReportActionID && markedActionWasOptimisticRef.current) { + return; + } + const isLastActionUnread = !!lastAction && isCurrentActionUnread(report, lastAction, sortedVisibleReportActions); if (!isUnread(report, transactionThreadReport, isReportArchived) && !isLastActionUnread) { return; diff --git a/tests/unit/useMarkAsReadTest.ts b/tests/unit/useMarkAsReadTest.ts index 19bc0238994b..ded62c811c9e 100644 --- a/tests/unit/useMarkAsReadTest.ts +++ b/tests/unit/useMarkAsReadTest.ts @@ -137,4 +137,40 @@ describe('useMarkAsRead', () => { expect(readNewestAction).toHaveBeenCalledTimes(1); expect(readNewestAction).toHaveBeenCalledWith(REPORT_ID, false); }); + + it('does not auto-read on report change when the manually-marked action was previously optimistic (offline mark → reconnect)', () => { + const markedActionID = '100'; + const reportWithMark = {...REPORT, manuallyMarkedUnreadReportActionID: markedActionID} as OnyxEntry; + // The user marked their just-sent (optimistic, offline) message unread. + const optimisticAction = {reportActionID: markedActionID, created: '2023-01-01 11:00:00.000', isOptimisticAction: true} as OnyxTypes.ReportAction; + + const {rerender} = renderHook((props: Parameters[0]) => useMarkAsRead(props), { + initialProps: { + reportID: REPORT_ID, + report: reportWithMark, + transactionThreadReport: undefined, + sortedVisibleReportActions: [optimisticAction], + isScrolledToEnd: true, + hasNewerActions: false, + }, + }); + + readNewestAction.mockClear(); + + // Reconnect: the action confirms — isOptimisticAction is cleared and its created shifts to server time, + // which changes lastVisibleActionCreated and re-fires the report-change read effect. + const confirmedAction = {reportActionID: markedActionID, created: '2023-01-01 10:59:59.000'} as OnyxTypes.ReportAction; + rerender({ + reportID: REPORT_ID, + report: {...reportWithMark, lastVisibleActionCreated: '2023-01-01 10:59:59.000'} as OnyxEntry, + transactionThreadReport: undefined, + sortedVisibleReportActions: [confirmedAction], + isScrolledToEnd: true, + hasNewerActions: false, + }); + + // The marker must survive: the confirm must not trigger readNewestAction, which would clear + // manuallyMarkedUnreadReportActionID and drop the "New" marker. + expect(readNewestAction).not.toHaveBeenCalled(); + }); }); From 601ef78611ea6f63ff23281187a6138e2d883d21 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 30 Jul 2026 14:37:59 +0000 Subject: [PATCH 09/43] Revert "Only skip auto-read on reconnect when the marked action was previously optimistic" This reverts commit 4f7bc5750c9f64c3cba3646d1af1d90eee678ad3. --- .../MoneyRequestReportActionsList.tsx | 23 ------------ src/hooks/useMarkAsRead.ts | 23 ------------ tests/unit/useMarkAsReadTest.ts | 36 ------------------- 3 files changed, 82 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx index f61cdbaea9fe..5b26ee73edc7 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx @@ -233,22 +233,6 @@ function MoneyRequestReportActionsList({onLayout}: MoneyRequestReportListProps) const hasNewestReportAction = lastAction?.created === lastVisibleActionCreated; const userActiveSince = useRef(DateUtils.getDBTime()); - // Latches whether the action the user manually marked unread was ever seen in an optimistic (just-sent, offline) - // state. The optimistic→confirmed merge clears isOptimisticAction/pendingAction on the same key, and that confirm - // is what re-fires the read effect below on reconnect — so we must record it beforehand, while still offline. - const markedActionWasOptimisticRef = useRef(false); - useEffect(() => { - const markedID = report?.manuallyMarkedUnreadReportActionID; - if (!markedID) { - markedActionWasOptimisticRef.current = false; - return; - } - const markedAction = visibleReportActions.find((action) => action.reportActionID === markedID); - if (markedAction?.isOptimisticAction || markedAction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) { - markedActionWasOptimisticRef.current = true; - } - }, [report?.manuallyMarkedUnreadReportActionID, visibleReportActions]); - const reportActionIDs = useMemo(() => { return reportActions?.map((action) => action.reportActionID) ?? []; }, [reportActions]); @@ -391,13 +375,6 @@ function MoneyRequestReportActionsList({onLayout}: MoneyRequestReportListProps) return; } - // The user marked an optimistic self-message unread while offline; when it confirms on reconnect its `created` - // shifts, re-running this effect. Don't auto-read it away — that would clear manuallyMarkedUnreadReportActionID - // and drop the "New" marker (native always hits this, since Visibility.hasFocus() is hard-coded true there). - if (report?.manuallyMarkedUnreadReportActionID && markedActionWasOptimisticRef.current) { - return; - } - if (isUnread(report, transactionThreadReport, isReportArchived) || (lastAction && isCurrentActionUnread(report, lastAction, visibleReportActions))) { // On desktop, when the notification center is displayed, isVisible will return false. // Currently, there's no programmatic way to dismiss the notification center panel. diff --git a/src/hooks/useMarkAsRead.ts b/src/hooks/useMarkAsRead.ts index 262dcec43a43..4408ff3a1b6e 100644 --- a/src/hooks/useMarkAsRead.ts +++ b/src/hooks/useMarkAsRead.ts @@ -66,22 +66,6 @@ function useMarkAsRead({reportID, report, transactionThreadReport, sortedVisible const lastMessageTime = useRef(null); const didMarkReportAsReadInitially = useRef(false); - // Latches whether the action the user manually marked unread was ever seen in an optimistic (just-sent, offline) - // state. The optimistic→confirmed merge clears isOptimisticAction/pendingAction on the same key, and that confirm - // is what re-fires the read effect below on reconnect — so we must record it beforehand, while still offline. - const markedActionWasOptimisticRef = useRef(false); - useEffect(() => { - const markedID = report?.manuallyMarkedUnreadReportActionID; - if (!markedID) { - markedActionWasOptimisticRef.current = false; - return; - } - const markedAction = sortedVisibleReportActions.find((action) => action.reportActionID === markedID); - if (markedAction?.isOptimisticAction || markedAction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) { - markedActionWasOptimisticRef.current = true; - } - }, [report?.manuallyMarkedUnreadReportActionID, sortedVisibleReportActions]); - const lastAction = sortedVisibleReportActions.at(0); const isReportUnreadValue = isUnread(report, transactionThreadReport, isReportArchived) || (!!lastAction && isCurrentActionUnread(report, lastAction)); @@ -120,13 +104,6 @@ function useMarkAsRead({reportID, report, transactionThreadReport, sortedVisible return; } - // The user marked an optimistic self-message unread while offline; when it confirms on reconnect its `created` - // shifts, re-running this effect. Don't auto-read it away — that would clear manuallyMarkedUnreadReportActionID - // and drop the "New" marker (native always hits this, since Visibility.hasFocus() is hard-coded true there). - if (report?.manuallyMarkedUnreadReportActionID && markedActionWasOptimisticRef.current) { - return; - } - const isLastActionUnread = !!lastAction && isCurrentActionUnread(report, lastAction, sortedVisibleReportActions); if (!isUnread(report, transactionThreadReport, isReportArchived) && !isLastActionUnread) { return; diff --git a/tests/unit/useMarkAsReadTest.ts b/tests/unit/useMarkAsReadTest.ts index ded62c811c9e..19bc0238994b 100644 --- a/tests/unit/useMarkAsReadTest.ts +++ b/tests/unit/useMarkAsReadTest.ts @@ -137,40 +137,4 @@ describe('useMarkAsRead', () => { expect(readNewestAction).toHaveBeenCalledTimes(1); expect(readNewestAction).toHaveBeenCalledWith(REPORT_ID, false); }); - - it('does not auto-read on report change when the manually-marked action was previously optimistic (offline mark → reconnect)', () => { - const markedActionID = '100'; - const reportWithMark = {...REPORT, manuallyMarkedUnreadReportActionID: markedActionID} as OnyxEntry; - // The user marked their just-sent (optimistic, offline) message unread. - const optimisticAction = {reportActionID: markedActionID, created: '2023-01-01 11:00:00.000', isOptimisticAction: true} as OnyxTypes.ReportAction; - - const {rerender} = renderHook((props: Parameters[0]) => useMarkAsRead(props), { - initialProps: { - reportID: REPORT_ID, - report: reportWithMark, - transactionThreadReport: undefined, - sortedVisibleReportActions: [optimisticAction], - isScrolledToEnd: true, - hasNewerActions: false, - }, - }); - - readNewestAction.mockClear(); - - // Reconnect: the action confirms — isOptimisticAction is cleared and its created shifts to server time, - // which changes lastVisibleActionCreated and re-fires the report-change read effect. - const confirmedAction = {reportActionID: markedActionID, created: '2023-01-01 10:59:59.000'} as OnyxTypes.ReportAction; - rerender({ - reportID: REPORT_ID, - report: {...reportWithMark, lastVisibleActionCreated: '2023-01-01 10:59:59.000'} as OnyxEntry, - transactionThreadReport: undefined, - sortedVisibleReportActions: [confirmedAction], - isScrolledToEnd: true, - hasNewerActions: false, - }); - - // The marker must survive: the confirm must not trigger readNewestAction, which would clear - // manuallyMarkedUnreadReportActionID and drop the "New" marker. - expect(readNewestAction).not.toHaveBeenCalled(); - }); }); From f76655a0a798745efd8f28dbdd0fa95e52e8e5df Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 30 Jul 2026 16:03:05 +0000 Subject: [PATCH 10/43] Clear manuallyMarkedUnreadReportActionID when the current user sends a message Sending a message advances lastReadTime via addComment's optimistic report but did not clear the stored manual-unread id, so the New marker override kept anchoring on the old action until another user's message triggered readNewestAction (the only path that nulled the field). Clear it alongside the lastReadTime reset. Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 4 ++++ tests/actions/ReportTest.ts | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 14c61c7ef01c..a820cd79f5bf 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -950,6 +950,10 @@ function addActions({ lastMessageHtml: lastCommentText, lastActorAccountID: currentUserAccountID, lastReadTime: currentTime, + // Sending a message advances the read pointer, so any prior manual-unread intent is now stale. Clear the stored + // id alongside lastReadTime; otherwise the "New" marker override keeps anchoring on the old action (the field is + // only ever nulled by readNewestAction, which this send path doesn't call). + manuallyMarkedUnreadReportActionID: null, lastActionType: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT, }; diff --git a/tests/actions/ReportTest.ts b/tests/actions/ReportTest.ts index 6517be85e3e1..9169284fbc3a 100644 --- a/tests/actions/ReportTest.ts +++ b/tests/actions/ReportTest.ts @@ -672,6 +672,9 @@ describe('actions/Report', () => { expect(ReportUtils.isUnreadWithMention(report)).toBe(true); expect(report?.lastReadTime).toBe(DateUtils.subtractMillisecondsFromDateTime(reportActionCreatedDate, 1)); + // And the manually-marked action id is stored so the "New" marker can anchor on it + expect(report?.manuallyMarkedUnreadReportActionID).toBe('1'); + // When a new comment is added by the current user currentTime = DateUtils.getDBTime(); @@ -694,6 +697,10 @@ describe('actions/Report', () => { expect(toZonedTime(report?.lastReadTime ?? '', UTC).getTime()).toBeGreaterThanOrEqual(toZonedTime(currentTime, UTC).getTime()); expect(report?.lastMessageText).toBe('Current User Comment 1'); + // And sending the comment clears the manual-unread intent, so the stale "New" marker doesn't persist + // (Onyx.merge with a null value deletes the key, so it reads back as undefined) + expect(report?.manuallyMarkedUnreadReportActionID).toBeUndefined(); + // When another comment is added by the current user currentTime = DateUtils.getDBTime(); Report.addComment({ From 1b0eb1e269c3563b22da1b60da01d2da7fff1cf2 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 30 Jul 2026 20:34:09 +0000 Subject: [PATCH 11/43] Skip reconnect auto-read only for an optimistic manually-unread action Marking an optimistic self-message unread while offline dropped the New marker on reconnect: the optimistic->confirmed merge shifts the action's created, which re-runs handleReportChangeMarkAsRead, and on native (Visibility.hasFocus() is always true) that fires readNewestAction, nulling manuallyMarkedUnreadReportActionID. Instead of bailing whenever the field is set (which would suppress auto-read for genuinely newer messages too), latch whether the marked action was ever seen optimistic - the optimism flag is cleared on the same key at confirm time, so it must be captured beforehand - and skip the reconnect auto-read only in that case. Applied to both the useMarkAsRead hook and the money-request report list's inline copy of the logic. Co-authored-by: Olly --- .../MoneyRequestReportActionsList.tsx | 24 +++++++++++++++++++ src/hooks/useMarkAsRead.ts | 24 +++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx index 5b26ee73edc7..1bb07e05c897 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx @@ -225,6 +225,7 @@ function MoneyRequestReportActionsList({onLayout}: MoneyRequestReportListProps) const scrollingVerticalBottomOffset = useRef(0); const tailIndicatorHeightRef = useRef(0); const readActionSkipped = useRef(false); + const markedActionWasOptimisticRef = useRef(false); const stickToBottomRef = useRef(false); const stickToBottomTimeoutRef = useRef(null); // Set when the user taps "Latest messages"; the report is marked as read only once the scroll actually reaches the bottom. @@ -370,11 +371,34 @@ function MoneyRequestReportActionsList({onLayout}: MoneyRequestReportListProps) return unsubscribe; }, []); + // Latch whether the action the user manually marked unread was ever seen in an optimistic (pending) state. + // On reconnect the optimistic->confirmed merge clears isOptimisticAction/pendingAction on the same action key, + // and that confirmation re-fires the read effect below - so by then the flag is already gone. We record it + // here, while the action is still pending, so the read effect can honor the manual-unread intent. + useEffect(() => { + const markedReportActionID = report?.manuallyMarkedUnreadReportActionID; + if (!markedReportActionID) { + markedActionWasOptimisticRef.current = false; + return; + } + const markedAction = visibleReportActions.find((reportAction) => reportAction.reportActionID === markedReportActionID); + if (markedAction?.isOptimisticAction || markedAction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) { + markedActionWasOptimisticRef.current = true; + } + }, [report?.manuallyMarkedUnreadReportActionID, visibleReportActions]); + useEffect(() => { if (!isFocused) { return; } + // The user marked an optimistic self-message unread while offline. When it confirms on reconnect its + // created shifts, re-running this effect; auto-reading here would clear manuallyMarkedUnreadReportActionID + // and drop the New marker. Honor the manual-unread intent for the action that was pending when marked. + if (report?.manuallyMarkedUnreadReportActionID && markedActionWasOptimisticRef.current) { + return; + } + if (isUnread(report, transactionThreadReport, isReportArchived) || (lastAction && isCurrentActionUnread(report, lastAction, visibleReportActions))) { // On desktop, when the notification center is displayed, isVisible will return false. // Currently, there's no programmatic way to dismiss the notification center panel. diff --git a/src/hooks/useMarkAsRead.ts b/src/hooks/useMarkAsRead.ts index 4408ff3a1b6e..04aaa0a0d671 100644 --- a/src/hooks/useMarkAsRead.ts +++ b/src/hooks/useMarkAsRead.ts @@ -65,6 +65,7 @@ function useMarkAsRead({reportID, report, transactionThreadReport, sortedVisible const userActiveSince = useRef(DateUtils.getDBTime()); const lastMessageTime = useRef(null); const didMarkReportAsReadInitially = useRef(false); + const markedActionWasOptimisticRef = useRef(false); const lastAction = sortedVisibleReportActions.at(0); const isReportUnreadValue = isUnread(report, transactionThreadReport, isReportArchived) || (!!lastAction && isCurrentActionUnread(report, lastAction)); @@ -96,6 +97,22 @@ function useMarkAsRead({reportID, report, transactionThreadReport, sortedVisible readNewestAction(reportID, isReportActionsLoaded); }, [isReportUnreadValue, reportID, isReportActionsLoaded]); + // Latch whether the action the user manually marked unread was ever seen in an optimistic (pending) state. + // On reconnect the optimistic->confirmed merge clears isOptimisticAction/pendingAction on the same action key, + // and that confirmation is what re-fires handleReportChangeMarkAsRead - so by then the flag is already gone. + // We record it here, while the action is still pending, so the read effect can honor the manual-unread intent. + useEffect(() => { + const markedReportActionID = report?.manuallyMarkedUnreadReportActionID; + if (!markedReportActionID) { + markedActionWasOptimisticRef.current = false; + return; + } + const markedAction = sortedVisibleReportActions.find((reportAction) => reportAction.reportActionID === markedReportActionID); + if (markedAction?.isOptimisticAction || markedAction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) { + markedActionWasOptimisticRef.current = true; + } + }, [report?.manuallyMarkedUnreadReportActionID, sortedVisibleReportActions]); + const didMarkOnReportChangeRef = useRef(false); const handleReportChangeMarkAsRead = useEffectEvent(() => { @@ -104,6 +121,13 @@ function useMarkAsRead({reportID, report, transactionThreadReport, sortedVisible return; } + // The user marked an optimistic self-message unread while offline. When it confirms on reconnect its + // created shifts, re-running this effect; auto-reading here would clear manuallyMarkedUnreadReportActionID + // and drop the New marker. Honor the manual-unread intent for the action that was pending when marked. + if (report?.manuallyMarkedUnreadReportActionID && markedActionWasOptimisticRef.current) { + return; + } + const isLastActionUnread = !!lastAction && isCurrentActionUnread(report, lastAction, sortedVisibleReportActions); if (!isUnread(report, transactionThreadReport, isReportArchived) && !isLastActionUnread) { return; From efed22e8bd5463f925a00b2bed500276e470e5aa Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 30 Jul 2026 20:48:43 +0000 Subject: [PATCH 12/43] Revert "Skip reconnect auto-read only for an optimistic manually-unread action" This reverts commit 1b0eb1e269c3563b22da1b60da01d2da7fff1cf2. --- .../MoneyRequestReportActionsList.tsx | 24 ------------------- src/hooks/useMarkAsRead.ts | 24 ------------------- 2 files changed, 48 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx index 1bb07e05c897..5b26ee73edc7 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx @@ -225,7 +225,6 @@ function MoneyRequestReportActionsList({onLayout}: MoneyRequestReportListProps) const scrollingVerticalBottomOffset = useRef(0); const tailIndicatorHeightRef = useRef(0); const readActionSkipped = useRef(false); - const markedActionWasOptimisticRef = useRef(false); const stickToBottomRef = useRef(false); const stickToBottomTimeoutRef = useRef(null); // Set when the user taps "Latest messages"; the report is marked as read only once the scroll actually reaches the bottom. @@ -371,34 +370,11 @@ function MoneyRequestReportActionsList({onLayout}: MoneyRequestReportListProps) return unsubscribe; }, []); - // Latch whether the action the user manually marked unread was ever seen in an optimistic (pending) state. - // On reconnect the optimistic->confirmed merge clears isOptimisticAction/pendingAction on the same action key, - // and that confirmation re-fires the read effect below - so by then the flag is already gone. We record it - // here, while the action is still pending, so the read effect can honor the manual-unread intent. - useEffect(() => { - const markedReportActionID = report?.manuallyMarkedUnreadReportActionID; - if (!markedReportActionID) { - markedActionWasOptimisticRef.current = false; - return; - } - const markedAction = visibleReportActions.find((reportAction) => reportAction.reportActionID === markedReportActionID); - if (markedAction?.isOptimisticAction || markedAction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) { - markedActionWasOptimisticRef.current = true; - } - }, [report?.manuallyMarkedUnreadReportActionID, visibleReportActions]); - useEffect(() => { if (!isFocused) { return; } - // The user marked an optimistic self-message unread while offline. When it confirms on reconnect its - // created shifts, re-running this effect; auto-reading here would clear manuallyMarkedUnreadReportActionID - // and drop the New marker. Honor the manual-unread intent for the action that was pending when marked. - if (report?.manuallyMarkedUnreadReportActionID && markedActionWasOptimisticRef.current) { - return; - } - if (isUnread(report, transactionThreadReport, isReportArchived) || (lastAction && isCurrentActionUnread(report, lastAction, visibleReportActions))) { // On desktop, when the notification center is displayed, isVisible will return false. // Currently, there's no programmatic way to dismiss the notification center panel. diff --git a/src/hooks/useMarkAsRead.ts b/src/hooks/useMarkAsRead.ts index 04aaa0a0d671..4408ff3a1b6e 100644 --- a/src/hooks/useMarkAsRead.ts +++ b/src/hooks/useMarkAsRead.ts @@ -65,7 +65,6 @@ function useMarkAsRead({reportID, report, transactionThreadReport, sortedVisible const userActiveSince = useRef(DateUtils.getDBTime()); const lastMessageTime = useRef(null); const didMarkReportAsReadInitially = useRef(false); - const markedActionWasOptimisticRef = useRef(false); const lastAction = sortedVisibleReportActions.at(0); const isReportUnreadValue = isUnread(report, transactionThreadReport, isReportArchived) || (!!lastAction && isCurrentActionUnread(report, lastAction)); @@ -97,22 +96,6 @@ function useMarkAsRead({reportID, report, transactionThreadReport, sortedVisible readNewestAction(reportID, isReportActionsLoaded); }, [isReportUnreadValue, reportID, isReportActionsLoaded]); - // Latch whether the action the user manually marked unread was ever seen in an optimistic (pending) state. - // On reconnect the optimistic->confirmed merge clears isOptimisticAction/pendingAction on the same action key, - // and that confirmation is what re-fires handleReportChangeMarkAsRead - so by then the flag is already gone. - // We record it here, while the action is still pending, so the read effect can honor the manual-unread intent. - useEffect(() => { - const markedReportActionID = report?.manuallyMarkedUnreadReportActionID; - if (!markedReportActionID) { - markedActionWasOptimisticRef.current = false; - return; - } - const markedAction = sortedVisibleReportActions.find((reportAction) => reportAction.reportActionID === markedReportActionID); - if (markedAction?.isOptimisticAction || markedAction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) { - markedActionWasOptimisticRef.current = true; - } - }, [report?.manuallyMarkedUnreadReportActionID, sortedVisibleReportActions]); - const didMarkOnReportChangeRef = useRef(false); const handleReportChangeMarkAsRead = useEffectEvent(() => { @@ -121,13 +104,6 @@ function useMarkAsRead({reportID, report, transactionThreadReport, sortedVisible return; } - // The user marked an optimistic self-message unread while offline. When it confirms on reconnect its - // created shifts, re-running this effect; auto-reading here would clear manuallyMarkedUnreadReportActionID - // and drop the New marker. Honor the manual-unread intent for the action that was pending when marked. - if (report?.manuallyMarkedUnreadReportActionID && markedActionWasOptimisticRef.current) { - return; - } - const isLastActionUnread = !!lastAction && isCurrentActionUnread(report, lastAction, sortedVisibleReportActions); if (!isUnread(report, transactionThreadReport, isReportArchived) && !isLastActionUnread) { return; From 31cacdd6e221848d0cc6153a107a0aa29401d63d Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 30 Jul 2026 21:17:24 +0000 Subject: [PATCH 13/43] Revert "Clear manuallyMarkedUnreadReportActionID when the current user sends a message" This reverts commit f76655a0a798745efd8f28dbdd0fa95e52e8e5df. --- src/libs/actions/Report/index.ts | 4 ---- tests/actions/ReportTest.ts | 7 ------- 2 files changed, 11 deletions(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index a820cd79f5bf..14c61c7ef01c 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -950,10 +950,6 @@ function addActions({ lastMessageHtml: lastCommentText, lastActorAccountID: currentUserAccountID, lastReadTime: currentTime, - // Sending a message advances the read pointer, so any prior manual-unread intent is now stale. Clear the stored - // id alongside lastReadTime; otherwise the "New" marker override keeps anchoring on the old action (the field is - // only ever nulled by readNewestAction, which this send path doesn't call). - manuallyMarkedUnreadReportActionID: null, lastActionType: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT, }; diff --git a/tests/actions/ReportTest.ts b/tests/actions/ReportTest.ts index 9169284fbc3a..6517be85e3e1 100644 --- a/tests/actions/ReportTest.ts +++ b/tests/actions/ReportTest.ts @@ -672,9 +672,6 @@ describe('actions/Report', () => { expect(ReportUtils.isUnreadWithMention(report)).toBe(true); expect(report?.lastReadTime).toBe(DateUtils.subtractMillisecondsFromDateTime(reportActionCreatedDate, 1)); - // And the manually-marked action id is stored so the "New" marker can anchor on it - expect(report?.manuallyMarkedUnreadReportActionID).toBe('1'); - // When a new comment is added by the current user currentTime = DateUtils.getDBTime(); @@ -697,10 +694,6 @@ describe('actions/Report', () => { expect(toZonedTime(report?.lastReadTime ?? '', UTC).getTime()).toBeGreaterThanOrEqual(toZonedTime(currentTime, UTC).getTime()); expect(report?.lastMessageText).toBe('Current User Comment 1'); - // And sending the comment clears the manual-unread intent, so the stale "New" marker doesn't persist - // (Onyx.merge with a null value deletes the key, so it reads back as undefined) - expect(report?.manuallyMarkedUnreadReportActionID).toBeUndefined(); - // When another comment is added by the current user currentTime = DateUtils.getDBTime(); Report.addComment({ From 58825671bda46b96aad14a8941e368ea52255a43 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 30 Jul 2026 21:58:23 +0000 Subject: [PATCH 14/43] Clear manually-unread marker by not bailing before readNewestAction Co-authored-by: Olly --- src/hooks/useMarkAsRead.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/hooks/useMarkAsRead.ts b/src/hooks/useMarkAsRead.ts index 4408ff3a1b6e..d23f93e3e3ec 100644 --- a/src/hooks/useMarkAsRead.ts +++ b/src/hooks/useMarkAsRead.ts @@ -105,7 +105,10 @@ function useMarkAsRead({reportID, report, transactionThreadReport, sortedVisible } const isLastActionUnread = !!lastAction && isCurrentActionUnread(report, lastAction, sortedVisibleReportActions); - if (!isUnread(report, transactionThreadReport, isReportArchived) && !isLastActionUnread) { + // When the user manually marked an action unread, isUnread/isLastActionUnread can both be false (e.g. a + // self-authored action), which would bail before readNewestAction runs and leave manuallyMarkedUnreadReportActionID + // set — so the marker never clears. Fall through in that case so the read path can clear it. + if (report?.manuallyMarkedUnreadReportActionID == null && !isUnread(report, transactionThreadReport, isReportArchived) && !isLastActionUnread) { return; } const isFromNotification = route?.params?.referrer === CONST.REFERRER.NOTIFICATION; From 6dfac6a3254380c92b773c952010f0b2b265f3cc Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 30 Jul 2026 22:28:39 +0000 Subject: [PATCH 15/43] Keep unread marker on the marked message when a newer self-message is sent Co-authored-by: Olly --- .../inbox/report/shouldDisplayNewMarkerOnReportAction.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts index 57766f1ea940..4c9bb73387c7 100644 --- a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts +++ b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts @@ -97,7 +97,11 @@ const shouldDisplayNewMarkerOnReportAction = ({ const isPreviouslyOptimistic = (isPendingAdd(prevSortedVisibleReportActionsObjects[message.reportActionID]) && !isPendingAdd(message)) || (!!prevSortedVisibleReportActionsObjects[message.reportActionID]?.isOptimisticAction && !message.isOptimisticAction); - const shouldIgnoreUnreadForCurrentUserMessage = isNewMessage || isPreviouslyOptimistic; + // While a manual mark-as-unread is active, the marked action is the sole anchor (handled by the + // `manuallyMarkedUnreadReportActionID` check above, which returns before this branch). Ignore unread + // for every *other* self-authored message so a newer self-message sent after the mark can't steal the + // marker off the marked one. When no manual mark exists this term is false, so #91940 behavior is unchanged. + const shouldIgnoreUnreadForCurrentUserMessage = isNewMessage || isPreviouslyOptimistic || !!manuallyMarkedUnreadReportActionID; if (isFromCurrentUser) { // For a self-authored action, only move/keep the "New" marker when one already exists in this session From 79bdb15b9ff132b5bf0194238d8d5d1ceaf9e8a2 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Fri, 31 Jul 2026 08:20:09 +0000 Subject: [PATCH 16/43] Anchor manual-unread marker on the marked action regardless of adjacent unread state Co-authored-by: Olly --- .../inbox/report/shouldDisplayNewMarkerOnReportAction.ts | 6 ++++-- tests/unit/ReportActionsUtilsTest.ts | 7 +++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts index 4c9bb73387c7..1ca703c007ef 100644 --- a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts +++ b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts @@ -70,8 +70,10 @@ const shouldDisplayNewMarkerOnReportAction = ({ // timestamp-based check below: once an optimistic self-message confirms, unreadMarkerTime, // lastReadTime, and created all converge on (or drift past) the confirmed `created`, so // isReportActionUnread wrongly reports it as read. The stored reportActionID is the only signal - // stable across that transition. Still yield to a more-recent unread message. - if (!!manuallyMarkedUnreadReportActionID && message.reportActionID === manuallyMarkedUnreadReportActionID && !isNextMessageUnread) { + // stable across that transition. The marked action is the oldest unread by construction + // (markCommentAsUnread sets lastReadTime = its created - 1ms), so it remains the correct anchor + // even when newer messages arrive after the mark. + if (!!manuallyMarkedUnreadReportActionID && message.reportActionID === manuallyMarkedUnreadReportActionID) { return true; } diff --git a/tests/unit/ReportActionsUtilsTest.ts b/tests/unit/ReportActionsUtilsTest.ts index 69c4419a3ce9..2a10b6ed8758 100644 --- a/tests/unit/ReportActionsUtilsTest.ts +++ b/tests/unit/ReportActionsUtilsTest.ts @@ -5594,7 +5594,10 @@ describe('ReportActionsUtils', () => { ).toBe(false); }); - it('yields the marker to a more-recent unread message even when an older action is marked unread', () => { + it('keeps the marker on the explicitly marked-unread action even when a newer message is present', () => { + // The marked action is the oldest unread by construction (lastReadTime = its created - 1ms), so it + // stays the anchor regardless of an adjacent unread message — a newer message arriving after the mark + // must not steal the marker off the message the user deliberately marked unread. const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'marked-action-id', created: '2023-01-01 11:00:00.000'}); const nextMessage = makeAction({created: '2023-01-01 11:30:00.000'}); expect( @@ -5605,7 +5608,7 @@ describe('ReportActionsUtils', () => { manuallyMarkedUnreadReportActionID: 'marked-action-id', isOffline: false, }), - ).toBe(false); + ).toBe(true); }); }); From 0a8dfe8caedd3d48f1528d77d55638f0a71e8822 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Fri, 31 Jul 2026 10:29:34 +0000 Subject: [PATCH 17/43] Remove manuallyMarkedUnreadReportActionID guard from handleReportChangeMarkAsRead early return Co-authored-by: Olly --- src/hooks/useMarkAsRead.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/hooks/useMarkAsRead.ts b/src/hooks/useMarkAsRead.ts index d23f93e3e3ec..4408ff3a1b6e 100644 --- a/src/hooks/useMarkAsRead.ts +++ b/src/hooks/useMarkAsRead.ts @@ -105,10 +105,7 @@ function useMarkAsRead({reportID, report, transactionThreadReport, sortedVisible } const isLastActionUnread = !!lastAction && isCurrentActionUnread(report, lastAction, sortedVisibleReportActions); - // When the user manually marked an action unread, isUnread/isLastActionUnread can both be false (e.g. a - // self-authored action), which would bail before readNewestAction runs and leave manuallyMarkedUnreadReportActionID - // set — so the marker never clears. Fall through in that case so the read path can clear it. - if (report?.manuallyMarkedUnreadReportActionID == null && !isUnread(report, transactionThreadReport, isReportArchived) && !isLastActionUnread) { + if (!isUnread(report, transactionThreadReport, isReportArchived) && !isLastActionUnread) { return; } const isFromNotification = route?.params?.referrer === CONST.REFERRER.NOTIFICATION; From ecab87ebce018e00604ce4dbd4081f37df1ca0b1 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Tue, 18 Aug 2026 08:15:46 +0000 Subject: [PATCH 18/43] Retain new marker through auto-read and make manual-unread the sole marker anchor Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 6 +++- .../shouldDisplayNewMarkerOnReportAction.ts | 33 ++++++++++--------- tests/unit/ReportActionsUtilsTest.ts | 29 ++++++++++++++++ 3 files changed, 51 insertions(+), 17 deletions(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 14c61c7ef01c..7fc63d430b1a 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -2855,7 +2855,11 @@ function readNewestAction(reportID: string | undefined, isReportActionsLoaded: b key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, value: { lastReadTime, - manuallyMarkedUnreadReportActionID: null, + // Intentionally do NOT clear `manuallyMarkedUnreadReportActionID` here. An explicit + // mark-as-unread should keep its "New" marker anchored even after the report is auto-read + // (readNewestAction fires whenever the report is focused/visible). The marker is instead + // reconciled when the report is re-loaded via openReport, which returns the server's + // authoritative value for this field. }, }, ]; diff --git a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts index 1ca703c007ef..1253ceb1e8c5 100644 --- a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts +++ b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts @@ -54,6 +54,19 @@ const shouldDisplayNewMarkerOnReportAction = ({ manuallyMarkedUnreadReportActionID, hasWindowFocus = true, }: ShouldDisplayNewMarkerOnReportActionParams): boolean => { + // The user explicitly marked an action as unread. While a manual mark is active, the marked action is + // the *sole* anchor for the marker: show it only on the marked action and suppress it on every other + // action (newer self-messages, other users' messages, the earliest offline message), regardless of the + // timestamp-based checks below. Anchoring by the stored reportActionID is stable across the + // optimistic->confirmed transition, where unreadMarkerTime, lastReadTime, and created all converge on + // (or drift past) the confirmed `created` and isReportActionUnread would wrongly report the marked + // action as read. The marked action is the oldest unread by construction (markCommentAsUnread sets + // lastReadTime = its created - 1ms), so it stays the correct anchor even when newer messages arrive + // after the mark. `shouldHideNewMarker` is still honored so the marker isn't anchored on a pending-delete action. + if (manuallyMarkedUnreadReportActionID) { + return message.reportActionID === manuallyMarkedUnreadReportActionID && !shouldHideNewMarker(message, isOffline); + } + const isNextMessageUnread = !!nextMessage && isReportActionUnread(nextMessage, unreadMarkerTime); // If the current message is the earliest message received while offline, we want to display the unread marker above this message. @@ -66,17 +79,6 @@ const shouldDisplayNewMarkerOnReportAction = ({ return false; } - // The user explicitly marked THIS action as unread. Anchor the marker here regardless of the - // timestamp-based check below: once an optimistic self-message confirms, unreadMarkerTime, - // lastReadTime, and created all converge on (or drift past) the confirmed `created`, so - // isReportActionUnread wrongly reports it as read. The stored reportActionID is the only signal - // stable across that transition. The marked action is the oldest unread by construction - // (markCommentAsUnread sets lastReadTime = its created - 1ms), so it remains the correct anchor - // even when newer messages arrive after the mark. - if (!!manuallyMarkedUnreadReportActionID && message.reportActionID === manuallyMarkedUnreadReportActionID) { - return true; - } - const isCurrentMessageUnread = isReportActionUnread(message, unreadMarkerTime); // If the current message is read or the next message is unread, don't show the unread marker. @@ -99,11 +101,10 @@ const shouldDisplayNewMarkerOnReportAction = ({ const isPreviouslyOptimistic = (isPendingAdd(prevSortedVisibleReportActionsObjects[message.reportActionID]) && !isPendingAdd(message)) || (!!prevSortedVisibleReportActionsObjects[message.reportActionID]?.isOptimisticAction && !message.isOptimisticAction); - // While a manual mark-as-unread is active, the marked action is the sole anchor (handled by the - // `manuallyMarkedUnreadReportActionID` check above, which returns before this branch). Ignore unread - // for every *other* self-authored message so a newer self-message sent after the mark can't steal the - // marker off the marked one. When no manual mark exists this term is false, so #91940 behavior is unchanged. - const shouldIgnoreUnreadForCurrentUserMessage = isNewMessage || isPreviouslyOptimistic || !!manuallyMarkedUnreadReportActionID; + // This branch is only reached when no manual mark-as-unread is active (the check at the top of the + // function returns early while one is). Ignore unread for a self-authored message that is new or was + // just optimistic, preserving the #91940 behavior for cold opens. + const shouldIgnoreUnreadForCurrentUserMessage = isNewMessage || isPreviouslyOptimistic; if (isFromCurrentUser) { // For a self-authored action, only move/keep the "New" marker when one already exists in this session diff --git a/tests/unit/ReportActionsUtilsTest.ts b/tests/unit/ReportActionsUtilsTest.ts index 2a10b6ed8758..b877d4d14696 100644 --- a/tests/unit/ReportActionsUtilsTest.ts +++ b/tests/unit/ReportActionsUtilsTest.ts @@ -5610,6 +5610,35 @@ describe('ReportActionsUtils', () => { }), ).toBe(true); }); + + it('returns false for any action that is not the marked one while a manual mark is active (sole anchor)', () => { + // While a manual mark-as-unread is active the marked action is the sole anchor. Every other action + // must be suppressed, even an unread message from another user that would otherwise qualify. + const message = makeAction({actorAccountID: 99, reportActionID: 'other-action-id', created: '2023-01-01 11:00:00.000'}); + expect( + shouldDisplayNewMarkerOnReportAction({ + ...baseParams, + message, + manuallyMarkedUnreadReportActionID: 'marked-action-id', + isOffline: false, + }), + ).toBe(false); + }); + + it('returns false for the earliest-received-offline message while a different action is marked unread', () => { + // The manual mark takes precedence over the earliest-received-offline branch, so a non-matching + // action is suppressed even when it is the earliest message received while offline. + const message = makeAction({actorAccountID: 99, reportActionID: 'offline-action-id', created: '2023-01-01 11:00:00.000'}); + expect( + shouldDisplayNewMarkerOnReportAction({ + ...baseParams, + message, + isEarliestReceivedOfflineMessage: true, + manuallyMarkedUnreadReportActionID: 'marked-action-id', + isOffline: false, + }), + ).toBe(false); + }); }); describe('getUnreadMarkerReportAction', () => { From ff1c4416be1c4321aaf0c916d929545623f13435 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Tue, 18 Aug 2026 08:33:54 +0000 Subject: [PATCH 19/43] Clear manuallyMarkedUnreadReportActionID on completed openReport reload Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 7fc63d430b1a..c21dccba8d65 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -1600,6 +1600,11 @@ function openReport(params: OpenReportActionParams) { errorFields: { notFound: null, }, + // An explicit mark-as-unread keeps its "New" marker anchored across auto-read + // (readNewestAction no longer clears it). Clear it here, once the report has actually + // been re-loaded from the server, so the marker is reconciled on reload rather than + // persisting indefinitely. + manuallyMarkedUnreadReportActionID: null, }, }, { From d2af5d9fa46cfb0bd267565fd4ed6fbaddad1e16 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Tue, 18 Aug 2026 10:42:30 +0000 Subject: [PATCH 20/43] Reconcile manual unread marker only on genuine report reload, not every openReport Co-authored-by: Olly --- config/eslint/eslint.seatbelt.tsv | 2 +- src/libs/actions/Report/index.ts | 27 +++++++++++++++++++++++---- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/config/eslint/eslint.seatbelt.tsv b/config/eslint/eslint.seatbelt.tsv index 3dcd1b037b7d..ff57a8917c30 100644 --- a/config/eslint/eslint.seatbelt.tsv +++ b/config/eslint/eslint.seatbelt.tsv @@ -933,7 +933,7 @@ "../../src/libs/actions/Report/index.ts" "@typescript-eslint/no-deprecated/reportAction.originalMessage" 1 "../../src/libs/actions/Report/index.ts" "@typescript-eslint/no-unsafe-type-assertion" 19 "../../src/libs/actions/Report/index.ts" "no-restricted-syntax" 7 -"../../src/libs/actions/Report/index.ts" "rulesdir/no-onyx-connect" 4 +"../../src/libs/actions/Report/index.ts" "rulesdir/no-onyx-connect" 5 "../../src/libs/actions/ReportLayout.ts" "@typescript-eslint/no-unsafe-type-assertion" 1 "../../src/libs/actions/ReportLayout.ts" "no-restricted-syntax" 1 "../../src/libs/actions/RequestConflictUtils.ts" "@typescript-eslint/no-unsafe-type-assertion" 2 diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index c21dccba8d65..b7490ad74a74 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -224,6 +224,7 @@ import type { Report, ReportAction, ReportAttributesDerivedValue, + ReportLoadingState, ReportNextStepDeprecated, ReportUserIsTyping, SidePanelContext, @@ -512,6 +513,18 @@ Onyx.connect({ }, }); +// RAM-only per-report loading state. `hasOnceLoadedReportActions` is false until the first successful +// openReport of the session and resets only on a genuine reload (page refresh / cold start), so it's the +// signal for "has this report already been loaded this session" — used by openReport below to reconcile the +// manual unread marker only on a true reload, not on every navigation. +let allReportLoadingStates: OnyxCollection; +Onyx.connect({ + key: ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE, + callback: (value) => { + allReportLoadingStates = value; + }, +}); + let allPersonalDetails: OnyxEntry = {}; Onyx.connect({ key: ONYXKEYS.PERSONAL_DETAILS_LIST, @@ -1542,6 +1555,11 @@ function openReport(params: OpenReportActionParams) { const participantAccountIDList = participants.map((p) => p.accountID).filter((id): id is number => id !== undefined); const existingReportName = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.reportName; const isCreatingNewReport = !isEmptyObject(newReportObject); + // Whether this report has already been loaded once this session (before this call). openReport fires on + // every navigation into a report, not just the first open, so we only reconcile the manual unread marker + // when this is a genuine first load / reload (page refresh or cold start resets this RAM-only flag). On a + // revisit the flag is already true, so we leave the marker untouched and it doesn't suddenly disappear. + const wasReportAlreadyLoaded = !!allReportLoadingStates?.[`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${reportID}`]?.hasOnceLoadedReportActions; const optimisticReport: Partial> = (hasReportActions ?? reportActionsExist(reportID)) || !existingReportName ? {} : {reportName: existingReportName}; const optimisticData: Array< @@ -1601,10 +1619,11 @@ function openReport(params: OpenReportActionParams) { notFound: null, }, // An explicit mark-as-unread keeps its "New" marker anchored across auto-read - // (readNewestAction no longer clears it). Clear it here, once the report has actually - // been re-loaded from the server, so the marker is reconciled on reload rather than - // persisting indefinitely. - manuallyMarkedUnreadReportActionID: null, + // (readNewestAction no longer clears it). Reconcile it only on a genuine first load / reload + // of this report — when it had not already been loaded this session. openReport also fires on + // every revisit (navigate-back, route change, thread rejoin); clearing on those would make the + // marker suddenly disappear, so we leave it untouched there and only clear on a true reload. + ...(wasReportAlreadyLoaded ? {} : {manuallyMarkedUnreadReportActionID: null}), }, }, { From d166efe936da2bef5a5be236e17ef214b867d78b Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Tue, 18 Aug 2026 13:53:08 +0000 Subject: [PATCH 21/43] Clear manual unread marker on the second open of the report, not the first Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index b7490ad74a74..e8a2d76cb716 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -1555,10 +1555,11 @@ function openReport(params: OpenReportActionParams) { const participantAccountIDList = participants.map((p) => p.accountID).filter((id): id is number => id !== undefined); const existingReportName = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.reportName; const isCreatingNewReport = !isEmptyObject(newReportObject); - // Whether this report has already been loaded once this session (before this call). openReport fires on - // every navigation into a report, not just the first open, so we only reconcile the manual unread marker - // when this is a genuine first load / reload (page refresh or cold start resets this RAM-only flag). On a - // revisit the flag is already true, so we leave the marker untouched and it doesn't suddenly disappear. + // Whether this report had already been loaded once this session before this call. openReport fires on + // every navigation into a report, not just the first open, so we use this RAM-only flag to tell the first + // open apart from a re-open: a manual unread marker is kept on the first open and cleared on the second + // (and later) open. A page refresh or cold start resets this flag, so the marker survives one more open + // after a reload. const wasReportAlreadyLoaded = !!allReportLoadingStates?.[`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${reportID}`]?.hasOnceLoadedReportActions; const optimisticReport: Partial> = (hasReportActions ?? reportActionsExist(reportID)) || !existingReportName ? {} : {reportName: existingReportName}; @@ -1619,11 +1620,12 @@ function openReport(params: OpenReportActionParams) { notFound: null, }, // An explicit mark-as-unread keeps its "New" marker anchored across auto-read - // (readNewestAction no longer clears it). Reconcile it only on a genuine first load / reload - // of this report — when it had not already been loaded this session. openReport also fires on - // every revisit (navigate-back, route change, thread rejoin); clearing on those would make the - // marker suddenly disappear, so we leave it untouched there and only clear on a true reload. - ...(wasReportAlreadyLoaded ? {} : {manuallyMarkedUnreadReportActionID: null}), + // (readNewestAction no longer clears it) and across the first open of the report, so the user + // sees the marker they created. Clear it on the second (and later) open of the report — + // `hasOnceLoadedReportActions` is already true by then — so the marker shows once and is then + // reconciled away when the user re-opens the report. A page refresh / cold start resets this + // RAM-only flag, so the marker survives one more open after a reload. + ...(wasReportAlreadyLoaded ? {manuallyMarkedUnreadReportActionID: null} : {}), }, }, { From de87a273e54ce00b504a7c7eae8778e9342662df Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Tue, 18 Aug 2026 14:19:05 +0000 Subject: [PATCH 22/43] Clear manual unread marker on navigate-away-and-back, not on refresh or re-fire Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 55 ++++++++++++++------------ src/pages/inbox/ReportFetchHandler.tsx | 16 ++++++++ 2 files changed, 46 insertions(+), 25 deletions(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index e8a2d76cb716..79816d9e2e7a 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -224,7 +224,6 @@ import type { Report, ReportAction, ReportAttributesDerivedValue, - ReportLoadingState, ReportNextStepDeprecated, ReportUserIsTyping, SidePanelContext, @@ -513,17 +512,23 @@ Onyx.connect({ }, }); -// RAM-only per-report loading state. `hasOnceLoadedReportActions` is false until the first successful -// openReport of the session and resets only on a genuine reload (page refresh / cold start), so it's the -// signal for "has this report already been loaded this session" — used by openReport below to reconcile the -// manual unread marker only on a true reload, not on every navigation. -let allReportLoadingStates: OnyxCollection; -Onyx.connect({ - key: ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE, - callback: (value) => { - allReportLoadingStates = value; - }, -}); +// RAM-only set of reportIDs the user has navigated away from this session. It is populated when the report +// screen blurs/unmounts (see `flagReportNavigatedAway`) and consumed by `openReport` to clear a manual unread +// marker on the *return* trip only. A blur is the one signal that uniquely identifies "navigated away and back +// to the chat": it does not fire on the multiple `openReport` calls of a single visit, and — being RAM-only — +// it is empty after a page refresh, so the marker survives a refresh and is only cleared by a real navigation. +const reportsNavigatedAwayFrom = new Set(); + +/** + * Records that the user has navigated away from the given report. Called from the report screen when it blurs + * or unmounts. The next `openReport` for this report will clear its manual unread marker and drop it from the set. + */ +function flagReportNavigatedAway(reportID: string | undefined) { + if (!reportID) { + return; + } + reportsNavigatedAwayFrom.add(reportID); +} let allPersonalDetails: OnyxEntry = {}; Onyx.connect({ @@ -1555,12 +1560,12 @@ function openReport(params: OpenReportActionParams) { const participantAccountIDList = participants.map((p) => p.accountID).filter((id): id is number => id !== undefined); const existingReportName = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.reportName; const isCreatingNewReport = !isEmptyObject(newReportObject); - // Whether this report had already been loaded once this session before this call. openReport fires on - // every navigation into a report, not just the first open, so we use this RAM-only flag to tell the first - // open apart from a re-open: a manual unread marker is kept on the first open and cleared on the second - // (and later) open. A page refresh or cold start resets this flag, so the marker survives one more open - // after a reload. - const wasReportAlreadyLoaded = !!allReportLoadingStates?.[`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${reportID}`]?.hasOnceLoadedReportActions; + // Whether the user navigated away from this report and is now coming back to it. The flag is set only when + // the report screen blurs/unmounts (see `flagReportNavigatedAway`), so it is true on a genuine return trip + // but false on the first open, on the repeated openReport calls of a single visit, and after a page refresh + // (the set is RAM-only). We consume it here to clear a manual unread marker only on that return trip. + const didNavigateBackToReport = reportsNavigatedAwayFrom.has(reportID); + reportsNavigatedAwayFrom.delete(reportID); const optimisticReport: Partial> = (hasReportActions ?? reportActionsExist(reportID)) || !existingReportName ? {} : {reportName: existingReportName}; const optimisticData: Array< @@ -1619,13 +1624,12 @@ function openReport(params: OpenReportActionParams) { errorFields: { notFound: null, }, - // An explicit mark-as-unread keeps its "New" marker anchored across auto-read - // (readNewestAction no longer clears it) and across the first open of the report, so the user - // sees the marker they created. Clear it on the second (and later) open of the report — - // `hasOnceLoadedReportActions` is already true by then — so the marker shows once and is then - // reconciled away when the user re-opens the report. A page refresh / cold start resets this - // RAM-only flag, so the marker survives one more open after a reload. - ...(wasReportAlreadyLoaded ? {manuallyMarkedUnreadReportActionID: null} : {}), + // An explicit mark-as-unread keeps its "New" marker anchored while the user stays in the report + // (readNewestAction no longer clears it, and the repeated openReport calls of a single visit + // don't either), so the user sees the marker they created. It is reconciled away only when the + // user navigates away and comes back — `didNavigateBackToReport` is true only on that return + // trip. A page refresh leaves the RAM-only set empty, so the marker survives a refresh. + ...(didNavigateBackToReport ? {manuallyMarkedUnreadReportActionID: null} : {}), }, }, { @@ -8389,6 +8393,7 @@ export { leaveRoom, markAsManuallyExported, markCommentAsUnread, + flagReportNavigatedAway, navigateToAndOpenChildReport, navigateToAndOpenReport, navigateToAndOpenReportWithAccountIDs, diff --git a/src/pages/inbox/ReportFetchHandler.tsx b/src/pages/inbox/ReportFetchHandler.tsx index 1c0763cb29f1..40c11341b461 100644 --- a/src/pages/inbox/ReportFetchHandler.tsx +++ b/src/pages/inbox/ReportFetchHandler.tsx @@ -37,6 +37,7 @@ import type {ReportsSplitNavigatorParamList, RightModalNavigatorParamList} from import { clearStaleDMRecoveryTargetByTargetReportID, createTransactionThreadReport, + flagReportNavigatedAway, joinReportViaSecureLink, openReport, readNewestAction, @@ -333,6 +334,21 @@ function ReportFetchHandler() { }; }, []); + // Record when the user navigates away from this report so the next openReport can clear a manual unread + // marker on the return trip (see `flagReportNavigatedAway`). We flag on blur (wide layout keeps the screen + // mounted) and on unmount / reportID change (narrow layout tears it down), covering both navigation shapes. + // Staying in the report never flags it, so the marker the user created is not wiped mid-session. + useEffect(() => { + if (!prevIsFocused || isFocused) { + return; + } + flagReportNavigatedAway(reportIDFromRoute); + }, [isFocused, prevIsFocused, reportIDFromRoute]); + + useEffect(() => { + return () => flagReportNavigatedAway(reportIDFromRoute); + }, [reportIDFromRoute]); + // `isLoadingInitialReportActions` is memory-only and is not reset between navigations. A prior failed // fetch leaves a stale `false` that can make ReportNotFoundGuard show "not here" before the fetch below // re-runs. When opening a report whose actions were never successfully loaded, mark it as loading again so From 22e8fc7111e0f8c0dec7139a1544f1d9926b1d5c Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Wed, 26 Aug 2026 09:52:23 +0000 Subject: [PATCH 23/43] Move manual unread marker clear from successData to optimisticData in openReport Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 79816d9e2e7a..6eab3daf063a 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -1566,7 +1566,20 @@ function openReport(params: OpenReportActionParams) { // (the set is RAM-only). We consume it here to clear a manual unread marker only on that return trip. const didNavigateBackToReport = reportsNavigatedAwayFrom.has(reportID); reportsNavigatedAwayFrom.delete(reportID); - const optimisticReport: Partial> = (hasReportActions ?? reportActionsExist(reportID)) || !existingReportName ? {} : {reportName: existingReportName}; + const optimisticReport: Partial> = + (hasReportActions ?? reportActionsExist(reportID)) || !existingReportName ? {} : {reportName: existingReportName}; + + // An explicit mark-as-unread keeps its "New" marker anchored while the user stays in the report + // (readNewestAction no longer clears it, and the repeated openReport calls of a single visit don't + // either), so the user sees the marker they created. It is reconciled away only when the user navigates + // away and comes back — `didNavigateBackToReport` is true only on that return trip. A page refresh leaves + // the RAM-only set empty, so the marker survives a refresh. This is a purely client-side decision, so it + // lives in optimisticData: it must apply immediately and offline, and must not be dropped if openReport + // never succeeds (the flag is already consumed above). We deliberately do NOT restore it in failureData — + // resurrecting a marker the user has already moved past would be wrong. + if (didNavigateBackToReport) { + optimisticReport.manuallyMarkedUnreadReportActionID = null; + } const optimisticData: Array< OnyxUpdate< @@ -1624,12 +1637,6 @@ function openReport(params: OpenReportActionParams) { errorFields: { notFound: null, }, - // An explicit mark-as-unread keeps its "New" marker anchored while the user stays in the report - // (readNewestAction no longer clears it, and the repeated openReport calls of a single visit - // don't either), so the user sees the marker they created. It is reconciled away only when the - // user navigates away and comes back — `didNavigateBackToReport` is true only on that return - // trip. A page refresh leaves the RAM-only set empty, so the marker survives a refresh. - ...(didNavigateBackToReport ? {manuallyMarkedUnreadReportActionID: null} : {}), }, }, { From 4d5e09dca136f1a28aa63d834d76164ad5c33f85 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Wed, 26 Aug 2026 10:38:08 +0000 Subject: [PATCH 24/43] Clear manuallyMarkedUnreadReportActionID on every openReport Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 43 +++++--------------------- src/pages/inbox/ReportFetchHandler.tsx | 16 ---------- 2 files changed, 7 insertions(+), 52 deletions(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 6eab3daf063a..3dbefd5fb78b 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -512,24 +512,6 @@ Onyx.connect({ }, }); -// RAM-only set of reportIDs the user has navigated away from this session. It is populated when the report -// screen blurs/unmounts (see `flagReportNavigatedAway`) and consumed by `openReport` to clear a manual unread -// marker on the *return* trip only. A blur is the one signal that uniquely identifies "navigated away and back -// to the chat": it does not fire on the multiple `openReport` calls of a single visit, and — being RAM-only — -// it is empty after a page refresh, so the marker survives a refresh and is only cleared by a real navigation. -const reportsNavigatedAwayFrom = new Set(); - -/** - * Records that the user has navigated away from the given report. Called from the report screen when it blurs - * or unmounts. The next `openReport` for this report will clear its manual unread marker and drop it from the set. - */ -function flagReportNavigatedAway(reportID: string | undefined) { - if (!reportID) { - return; - } - reportsNavigatedAwayFrom.add(reportID); -} - let allPersonalDetails: OnyxEntry = {}; Onyx.connect({ key: ONYXKEYS.PERSONAL_DETAILS_LIST, @@ -1560,26 +1542,16 @@ function openReport(params: OpenReportActionParams) { const participantAccountIDList = participants.map((p) => p.accountID).filter((id): id is number => id !== undefined); const existingReportName = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.reportName; const isCreatingNewReport = !isEmptyObject(newReportObject); - // Whether the user navigated away from this report and is now coming back to it. The flag is set only when - // the report screen blurs/unmounts (see `flagReportNavigatedAway`), so it is true on a genuine return trip - // but false on the first open, on the repeated openReport calls of a single visit, and after a page refresh - // (the set is RAM-only). We consume it here to clear a manual unread marker only on that return trip. - const didNavigateBackToReport = reportsNavigatedAwayFrom.has(reportID); - reportsNavigatedAwayFrom.delete(reportID); const optimisticReport: Partial> = (hasReportActions ?? reportActionsExist(reportID)) || !existingReportName ? {} : {reportName: existingReportName}; - // An explicit mark-as-unread keeps its "New" marker anchored while the user stays in the report - // (readNewestAction no longer clears it, and the repeated openReport calls of a single visit don't - // either), so the user sees the marker they created. It is reconciled away only when the user navigates - // away and comes back — `didNavigateBackToReport` is true only on that return trip. A page refresh leaves - // the RAM-only set empty, so the marker survives a refresh. This is a purely client-side decision, so it - // lives in optimisticData: it must apply immediately and offline, and must not be dropped if openReport - // never succeeds (the flag is already consumed above). We deliberately do NOT restore it in failureData — - // resurrecting a marker the user has already moved past would be wrong. - if (didNavigateBackToReport) { - optimisticReport.manuallyMarkedUnreadReportActionID = null; - } + // Clear any manual unread marker whenever the report is (re)opened. An explicit mark-as-unread keeps its + // "New" marker anchored while the user stays in the report (readNewestAction no longer clears it), and the + // marker is reconciled away the next time openReport runs — e.g. navigating away and back, or a refresh. + // This is a purely client-side decision, so it lives in optimisticData: it applies immediately and offline. + // We deliberately do NOT restore it in failureData — resurrecting a marker the user has already moved past + // would be wrong. + optimisticReport.manuallyMarkedUnreadReportActionID = null; const optimisticData: Array< OnyxUpdate< @@ -8400,7 +8372,6 @@ export { leaveRoom, markAsManuallyExported, markCommentAsUnread, - flagReportNavigatedAway, navigateToAndOpenChildReport, navigateToAndOpenReport, navigateToAndOpenReportWithAccountIDs, diff --git a/src/pages/inbox/ReportFetchHandler.tsx b/src/pages/inbox/ReportFetchHandler.tsx index 40c11341b461..1c0763cb29f1 100644 --- a/src/pages/inbox/ReportFetchHandler.tsx +++ b/src/pages/inbox/ReportFetchHandler.tsx @@ -37,7 +37,6 @@ import type {ReportsSplitNavigatorParamList, RightModalNavigatorParamList} from import { clearStaleDMRecoveryTargetByTargetReportID, createTransactionThreadReport, - flagReportNavigatedAway, joinReportViaSecureLink, openReport, readNewestAction, @@ -334,21 +333,6 @@ function ReportFetchHandler() { }; }, []); - // Record when the user navigates away from this report so the next openReport can clear a manual unread - // marker on the return trip (see `flagReportNavigatedAway`). We flag on blur (wide layout keeps the screen - // mounted) and on unmount / reportID change (narrow layout tears it down), covering both navigation shapes. - // Staying in the report never flags it, so the marker the user created is not wiped mid-session. - useEffect(() => { - if (!prevIsFocused || isFocused) { - return; - } - flagReportNavigatedAway(reportIDFromRoute); - }, [isFocused, prevIsFocused, reportIDFromRoute]); - - useEffect(() => { - return () => flagReportNavigatedAway(reportIDFromRoute); - }, [reportIDFromRoute]); - // `isLoadingInitialReportActions` is memory-only and is not reset between navigations. A prior failed // fetch leaves a stale `false` that can make ReportNotFoundGuard show "not here" before the fetch below // re-runs. When opening a report whose actions were never successfully loaded, mark it as loading again so From a1d047db9f10cd3fbb1ecb8734a83f3d73bfefb7 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Wed, 26 Aug 2026 11:02:49 +0000 Subject: [PATCH 25/43] Revert "Clear manuallyMarkedUnreadReportActionID on every openReport" This reverts commit 4d5e09dca136f1a28aa63d834d76164ad5c33f85. --- src/libs/actions/Report/index.ts | 43 +++++++++++++++++++++----- src/pages/inbox/ReportFetchHandler.tsx | 16 ++++++++++ 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 3dbefd5fb78b..6eab3daf063a 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -512,6 +512,24 @@ Onyx.connect({ }, }); +// RAM-only set of reportIDs the user has navigated away from this session. It is populated when the report +// screen blurs/unmounts (see `flagReportNavigatedAway`) and consumed by `openReport` to clear a manual unread +// marker on the *return* trip only. A blur is the one signal that uniquely identifies "navigated away and back +// to the chat": it does not fire on the multiple `openReport` calls of a single visit, and — being RAM-only — +// it is empty after a page refresh, so the marker survives a refresh and is only cleared by a real navigation. +const reportsNavigatedAwayFrom = new Set(); + +/** + * Records that the user has navigated away from the given report. Called from the report screen when it blurs + * or unmounts. The next `openReport` for this report will clear its manual unread marker and drop it from the set. + */ +function flagReportNavigatedAway(reportID: string | undefined) { + if (!reportID) { + return; + } + reportsNavigatedAwayFrom.add(reportID); +} + let allPersonalDetails: OnyxEntry = {}; Onyx.connect({ key: ONYXKEYS.PERSONAL_DETAILS_LIST, @@ -1542,16 +1560,26 @@ function openReport(params: OpenReportActionParams) { const participantAccountIDList = participants.map((p) => p.accountID).filter((id): id is number => id !== undefined); const existingReportName = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.reportName; const isCreatingNewReport = !isEmptyObject(newReportObject); + // Whether the user navigated away from this report and is now coming back to it. The flag is set only when + // the report screen blurs/unmounts (see `flagReportNavigatedAway`), so it is true on a genuine return trip + // but false on the first open, on the repeated openReport calls of a single visit, and after a page refresh + // (the set is RAM-only). We consume it here to clear a manual unread marker only on that return trip. + const didNavigateBackToReport = reportsNavigatedAwayFrom.has(reportID); + reportsNavigatedAwayFrom.delete(reportID); const optimisticReport: Partial> = (hasReportActions ?? reportActionsExist(reportID)) || !existingReportName ? {} : {reportName: existingReportName}; - // Clear any manual unread marker whenever the report is (re)opened. An explicit mark-as-unread keeps its - // "New" marker anchored while the user stays in the report (readNewestAction no longer clears it), and the - // marker is reconciled away the next time openReport runs — e.g. navigating away and back, or a refresh. - // This is a purely client-side decision, so it lives in optimisticData: it applies immediately and offline. - // We deliberately do NOT restore it in failureData — resurrecting a marker the user has already moved past - // would be wrong. - optimisticReport.manuallyMarkedUnreadReportActionID = null; + // An explicit mark-as-unread keeps its "New" marker anchored while the user stays in the report + // (readNewestAction no longer clears it, and the repeated openReport calls of a single visit don't + // either), so the user sees the marker they created. It is reconciled away only when the user navigates + // away and comes back — `didNavigateBackToReport` is true only on that return trip. A page refresh leaves + // the RAM-only set empty, so the marker survives a refresh. This is a purely client-side decision, so it + // lives in optimisticData: it must apply immediately and offline, and must not be dropped if openReport + // never succeeds (the flag is already consumed above). We deliberately do NOT restore it in failureData — + // resurrecting a marker the user has already moved past would be wrong. + if (didNavigateBackToReport) { + optimisticReport.manuallyMarkedUnreadReportActionID = null; + } const optimisticData: Array< OnyxUpdate< @@ -8372,6 +8400,7 @@ export { leaveRoom, markAsManuallyExported, markCommentAsUnread, + flagReportNavigatedAway, navigateToAndOpenChildReport, navigateToAndOpenReport, navigateToAndOpenReportWithAccountIDs, diff --git a/src/pages/inbox/ReportFetchHandler.tsx b/src/pages/inbox/ReportFetchHandler.tsx index 1c0763cb29f1..40c11341b461 100644 --- a/src/pages/inbox/ReportFetchHandler.tsx +++ b/src/pages/inbox/ReportFetchHandler.tsx @@ -37,6 +37,7 @@ import type {ReportsSplitNavigatorParamList, RightModalNavigatorParamList} from import { clearStaleDMRecoveryTargetByTargetReportID, createTransactionThreadReport, + flagReportNavigatedAway, joinReportViaSecureLink, openReport, readNewestAction, @@ -333,6 +334,21 @@ function ReportFetchHandler() { }; }, []); + // Record when the user navigates away from this report so the next openReport can clear a manual unread + // marker on the return trip (see `flagReportNavigatedAway`). We flag on blur (wide layout keeps the screen + // mounted) and on unmount / reportID change (narrow layout tears it down), covering both navigation shapes. + // Staying in the report never flags it, so the marker the user created is not wiped mid-session. + useEffect(() => { + if (!prevIsFocused || isFocused) { + return; + } + flagReportNavigatedAway(reportIDFromRoute); + }, [isFocused, prevIsFocused, reportIDFromRoute]); + + useEffect(() => { + return () => flagReportNavigatedAway(reportIDFromRoute); + }, [reportIDFromRoute]); + // `isLoadingInitialReportActions` is memory-only and is not reset between navigations. A prior failed // fetch leaves a stale `false` that can make ReportNotFoundGuard show "not here" before the fetch below // re-runs. When opening a report whose actions were never successfully loaded, mark it as loading again so From 3e501fc3e0b83cfd2ba3571396371c1026bd8a31 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Wed, 26 Aug 2026 11:05:37 +0000 Subject: [PATCH 26/43] Also clear manual unread marker on page refresh, not only navigate-back Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 34 ++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 6eab3daf063a..0564f39df765 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -224,6 +224,7 @@ import type { Report, ReportAction, ReportAttributesDerivedValue, + ReportLoadingState, ReportNextStepDeprecated, ReportUserIsTyping, SidePanelContext, @@ -530,6 +531,18 @@ function flagReportNavigatedAway(reportID: string | undefined) { reportsNavigatedAwayFrom.add(reportID); } +// RAM-only per-report loading state. `hasOnceLoadedReportActions` is false until the first successful +// openReport of the session and resets only on a genuine reload (page refresh / cold start), so it's the +// signal for "has this report already been loaded this session" — used by openReport below to also clear the +// manual unread marker on a page refresh, on top of the navigate-away-and-back case above. +let allReportLoadingStates: OnyxCollection; +Onyx.connect({ + key: ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE, + callback: (value) => { + allReportLoadingStates = value; + }, +}); + let allPersonalDetails: OnyxEntry = {}; Onyx.connect({ key: ONYXKEYS.PERSONAL_DETAILS_LIST, @@ -1563,21 +1576,26 @@ function openReport(params: OpenReportActionParams) { // Whether the user navigated away from this report and is now coming back to it. The flag is set only when // the report screen blurs/unmounts (see `flagReportNavigatedAway`), so it is true on a genuine return trip // but false on the first open, on the repeated openReport calls of a single visit, and after a page refresh - // (the set is RAM-only). We consume it here to clear a manual unread marker only on that return trip. + // (the set is RAM-only). We consume it here to clear a manual unread marker on that return trip. const didNavigateBackToReport = reportsNavigatedAwayFrom.has(reportID); reportsNavigatedAwayFrom.delete(reportID); + // Whether this is the first load of the report this session. `hasOnceLoadedReportActions` is RAM-only, so a + // page refresh / cold start resets it to falsy — that's how we detect a refresh here. A manual unread marker + // can only be non-null on a first load if it was persisted from before the refresh, so clearing it here + // clears the marker on a page refresh while leaving genuine first opens (marker already null) untouched. + const isFirstLoadAfterRefresh = !allReportLoadingStates?.[`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${reportID}`]?.hasOnceLoadedReportActions; const optimisticReport: Partial> = (hasReportActions ?? reportActionsExist(reportID)) || !existingReportName ? {} : {reportName: existingReportName}; // An explicit mark-as-unread keeps its "New" marker anchored while the user stays in the report // (readNewestAction no longer clears it, and the repeated openReport calls of a single visit don't - // either), so the user sees the marker they created. It is reconciled away only when the user navigates - // away and comes back — `didNavigateBackToReport` is true only on that return trip. A page refresh leaves - // the RAM-only set empty, so the marker survives a refresh. This is a purely client-side decision, so it - // lives in optimisticData: it must apply immediately and offline, and must not be dropped if openReport - // never succeeds (the flag is already consumed above). We deliberately do NOT restore it in failureData — - // resurrecting a marker the user has already moved past would be wrong. - if (didNavigateBackToReport) { + // either), so the user sees the marker they created. It is reconciled away in two cases: when the user + // navigates away and comes back (`didNavigateBackToReport`), and on a page refresh (`isFirstLoadAfterRefresh`). + // This is a purely client-side decision, so it lives in optimisticData: it must apply immediately and + // offline, and must not be dropped if openReport never succeeds (the navigate-away flag is already consumed + // above). We deliberately do NOT restore it in failureData — resurrecting a marker the user has already + // moved past would be wrong. + if (didNavigateBackToReport || isFirstLoadAfterRefresh) { optimisticReport.manuallyMarkedUnreadReportActionID = null; } From 825fb5e84271c4b85229f40ca7b8ff019ef7556f Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Wed, 26 Aug 2026 13:23:39 +0000 Subject: [PATCH 27/43] Fix failing unread-marker tests: move marker when marked action is deleted; update useOnyx mock Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 3 +-- .../shouldDisplayNewMarkerOnReportAction.ts | 13 ++++++++++++- tests/unit/useUnreadMarkerTest.ts | 18 +++++++++++------- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 255eaf12c4a1..27b30d33c312 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -1735,8 +1735,7 @@ function openReport(params: OpenReportActionParams) { // can only be non-null on a first load if it was persisted from before the refresh, so clearing it here // clears the marker on a page refresh while leaving genuine first opens (marker already null) untouched. const isFirstLoadAfterRefresh = !allReportLoadingStates?.[`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${reportID}`]?.hasOnceLoadedReportActions; - const optimisticReport: Partial> = - hasReportActions || !existingReportName ? {} : {reportName: existingReportName}; + const optimisticReport: Partial> = hasReportActions || !existingReportName ? {} : {reportName: existingReportName}; // An explicit mark-as-unread keeps its "New" marker anchored while the user stays in the report // (readNewestAction no longer clears it, and the repeated openReport calls of a single visit don't diff --git a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts index 1253ceb1e8c5..243514ebea35 100644 --- a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts +++ b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts @@ -183,6 +183,17 @@ const getUnreadMarkerReportAction = ({ return [null, -1]; } + // The stable manual-mark anchor is only valid while the marked action is still present and not pending + // deletion. Once it is deleted, keeping the anchor would leave every visible action failing the + // `reportActionID === manuallyMarkedUnreadReportActionID` check, so the marker would vanish instead of + // moving on. Drop the anchor in that case so the timestamp-based scan below can move the marker to the + // next unread message. + const manuallyMarkedUnreadReportAction = manuallyMarkedUnreadReportActionID + ? visibleReportActions.find((action) => action.reportActionID === manuallyMarkedUnreadReportActionID) + : undefined; + const activeManuallyMarkedUnreadReportActionID = + manuallyMarkedUnreadReportAction && !shouldHideNewMarker(manuallyMarkedUnreadReportAction, isOffline) ? manuallyMarkedUnreadReportActionID : null; + const startIndex = isReversed ? visibleReportActions.length - 1 : (earliestReceivedOfflineMessageIndex ?? 0); const endIndex = isReversed ? (earliestReceivedOfflineMessageIndex ?? 0) : visibleReportActions.length; const step = isReversed ? -1 : 1; @@ -218,7 +229,7 @@ const getUnreadMarkerReportAction = ({ isScrolledOverThreshold, isOffline, prevUnreadMarkerReportActionID, - manuallyMarkedUnreadReportActionID, + manuallyMarkedUnreadReportActionID: activeManuallyMarkedUnreadReportActionID, hasWindowFocus, }); diff --git a/tests/unit/useUnreadMarkerTest.ts b/tests/unit/useUnreadMarkerTest.ts index 9eecbdf44d47..7501487284a5 100644 --- a/tests/unit/useUnreadMarkerTest.ts +++ b/tests/unit/useUnreadMarkerTest.ts @@ -28,13 +28,16 @@ jest.mock('@hooks/useIsAnonymousUser', () => ({ default: () => mockIsAnonymousUser, })); -// The hook subscribes to `${ONYXKEYS.COLLECTION.REPORT}${reportID}` with a selector that returns -// `lastReadTime`. The implementation is set in beforeEach so it can use ONYXKEYS freely (a jest.mock -// factory cannot reference out-of-scope variables). -const mockUseOnyx = jest.fn<[string], [string]>(); +// The hook subscribes to `${ONYXKEYS.COLLECTION.REPORT}${reportID}` twice, each with its own selector +// (one for `lastReadTime`, one for `manuallyMarkedUnreadReportActionID`). The mock applies the passed +// selector to a fake report so each call returns the value it actually reads. The implementation is set +// in beforeEach so it can use ONYXKEYS freely (a jest.mock factory cannot reference out-of-scope variables). +type FakeReport = Pick; +type UseOnyxOptions = {selector?: (value: FakeReport | undefined) => unknown}; +const mockUseOnyx = jest.fn<[unknown], [string, UseOnyxOptions?]>(); jest.mock('@hooks/useOnyx', () => ({ __esModule: true, - default: (key: string) => mockUseOnyx(key), + default: (key: string, options?: UseOnyxOptions) => mockUseOnyx(key, options), })); function makeAction(reportActionID: string, overrides: Partial = {}): OnyxTypes.ReportAction { @@ -66,9 +69,10 @@ describe('useUnreadMarker', () => { mockIsAnonymousUser = false; mockLastReadTime = LAST_READ_TIME; mockLastReadTimeByReportID = {}; - mockUseOnyx.mockImplementation((key) => { + mockUseOnyx.mockImplementation((key, options) => { const reportID = key.replace(ONYXKEYS.COLLECTION.REPORT, ''); - return [mockLastReadTimeByReportID[reportID] ?? mockLastReadTime]; + const report: FakeReport = {lastReadTime: mockLastReadTimeByReportID[reportID] ?? mockLastReadTime}; + return [options?.selector ? options.selector(report) : report]; }); }); From 81fda36bfba14251807695c57fafede958f91212 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Wed, 26 Aug 2026 14:59:34 +0000 Subject: [PATCH 28/43] Don't populate prevUnreadMarkerReportActionID from the manual-mark anchor Co-authored-by: Olly --- src/hooks/useUnreadMarker.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/hooks/useUnreadMarker.ts b/src/hooks/useUnreadMarker.ts index 45fae5ee2cef..e125f4333d81 100644 --- a/src/hooks/useUnreadMarker.ts +++ b/src/hooks/useUnreadMarker.ts @@ -134,7 +134,12 @@ function useUnreadMarker({ const [unreadMarkerReportActionID, unreadMarkerReportActionIndex]: [string | null, number] = oldestUnreadReportActionMarker && (scanned[0] === null || scanned[0] === oldestUnreadReportActionMarker[0]) ? oldestUnreadReportActionMarker : scanned; - if (prevUnreadMarkerReportActionID !== unreadMarkerReportActionID) { + // `prevUnreadMarkerReportActionID` gates the self-authored-message branch in + // `shouldDisplayNewMarkerOnReportAction`, which only runs once no manual mark is active. Don't let the + // manual-mark anchor populate it: otherwise, after the manual mark is cleared, a persisted self-authored + // action would wrongly keep the "New" marker (the Expensify/App#91940 regression) because this value is + // still truthy from the mark. Only track markers set by the timestamp-based scan. + if (prevUnreadMarkerReportActionID !== unreadMarkerReportActionID && unreadMarkerReportActionID !== manuallyMarkedUnreadReportActionID) { setPrevUnreadMarkerReportActionID(unreadMarkerReportActionID); } From 3dbc70cb97730a02f625227a1419b77e8b50fe07 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 27 Aug 2026 11:16:17 +0000 Subject: [PATCH 29/43] Move self-message marker-hop guard into the display decision Drop the manual-mark exclusion from prevUnreadMarkerReportActionID bookkeeping in useUnreadMarker and express the #91940 guard directly in shouldDisplayNewMarkerOnReportAction via isDifferentUnread, so a self-authored marker can't hop from one self action to another. Co-authored-by: Olly --- src/hooks/useUnreadMarker.ts | 12 +++--- .../shouldDisplayNewMarkerOnReportAction.ts | 12 ++++-- tests/unit/ReportActionsUtilsTest.ts | 39 +++++++++++++++++++ 3 files changed, 53 insertions(+), 10 deletions(-) diff --git a/src/hooks/useUnreadMarker.ts b/src/hooks/useUnreadMarker.ts index e125f4333d81..efb97d5ed447 100644 --- a/src/hooks/useUnreadMarker.ts +++ b/src/hooks/useUnreadMarker.ts @@ -134,12 +134,12 @@ function useUnreadMarker({ const [unreadMarkerReportActionID, unreadMarkerReportActionIndex]: [string | null, number] = oldestUnreadReportActionMarker && (scanned[0] === null || scanned[0] === oldestUnreadReportActionMarker[0]) ? oldestUnreadReportActionMarker : scanned; - // `prevUnreadMarkerReportActionID` gates the self-authored-message branch in - // `shouldDisplayNewMarkerOnReportAction`, which only runs once no manual mark is active. Don't let the - // manual-mark anchor populate it: otherwise, after the manual mark is cleared, a persisted self-authored - // action would wrongly keep the "New" marker (the Expensify/App#91940 regression) because this value is - // still truthy from the mark. Only track markers set by the timestamp-based scan. - if (prevUnreadMarkerReportActionID !== unreadMarkerReportActionID && unreadMarkerReportActionID !== manuallyMarkedUnreadReportActionID) { + // `prevUnreadMarkerReportActionID` records the action the marker was last anchored on and gates the + // self-authored-message branch in `shouldDisplayNewMarkerOnReportAction`, which only runs once no manual + // mark is active. The #91940 regression (a persisted self-authored action wrongly keeping the "New" + // marker after the marker moves) is prevented there via the `isDifferentUnread` check, so we simply track + // whatever the marker last landed on here. + if (prevUnreadMarkerReportActionID !== unreadMarkerReportActionID) { setPrevUnreadMarkerReportActionID(unreadMarkerReportActionID); } diff --git a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts index 243514ebea35..0fe2cba4fc0c 100644 --- a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts +++ b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts @@ -104,15 +104,19 @@ const shouldDisplayNewMarkerOnReportAction = ({ // This branch is only reached when no manual mark-as-unread is active (the check at the top of the // function returns early while one is). Ignore unread for a self-authored message that is new or was // just optimistic, preserving the #91940 behavior for cold opens. - const shouldIgnoreUnreadForCurrentUserMessage = isNewMessage || isPreviouslyOptimistic; + const prevMarkedReportAction = prevUnreadMarkerReportActionID ? prevSortedVisibleReportActionsObjects[prevUnreadMarkerReportActionID] : undefined; + const isPreviouslyUnreadFromCurrentUser = currentUserAccountID === prevMarkedReportAction?.actorAccountID; + // So essentially, the previously unread cannot move from one new self-user-action to another. Once a + // self-authored action holds the marker, keep it there rather than letting it hop to a different + // self-authored action (e.g. a persisted reimbursable toggle) — the regression from Expensify/App#91940. + const isDifferentUnread = isPreviouslyUnreadFromCurrentUser && prevMarkedReportAction?.reportActionID !== message.reportActionID; + const shouldIgnoreUnreadForCurrentUserMessage = isNewMessage || isPreviouslyOptimistic || isDifferentUnread; if (isFromCurrentUser) { // For a self-authored action, only move/keep the "New" marker when one already exists in this session // (`prevUnreadMarkerReportActionID` is set). The explicit mark-as-unread case is handled earlier by the // stable `manuallyMarkedUnreadReportActionID` check, which anchors the marker on first open/re-entry - // regardless of this guard. Without this guard, a persisted self-authored action (e.g. a reimbursable - // toggle) whose timestamps have drifted past `lastReadTime` would wrongly show the marker on a cold - // open/re-entry — the regression from Expensify/App#91940. + // regardless of this guard. if (prevUnreadMarkerReportActionID) { return !shouldIgnoreUnreadForCurrentUserMessage; } diff --git a/tests/unit/ReportActionsUtilsTest.ts b/tests/unit/ReportActionsUtilsTest.ts index 946f3a5e7362..90678968d836 100644 --- a/tests/unit/ReportActionsUtilsTest.ts +++ b/tests/unit/ReportActionsUtilsTest.ts @@ -6132,6 +6132,45 @@ describe('ReportActionsUtils', () => { ).toBe(true); }); + it('does not move the marker from one self-authored action to a different self-authored action', () => { + // The previously marked action was a persisted self-authored message. A different self-authored + // action must not steal the "New" marker off it (the Expensify/App#91940 hop), so `isDifferentUnread` + // suppresses the marker here even though this action reads as unread. + const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-b'}); + const prevMarkedAction = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-a'}); + const prevSortedVisibleReportActionsObjects = { + [prevMarkedAction.reportActionID]: prevMarkedAction, + [message.reportActionID]: makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-b'}), + }; + expect( + shouldDisplayNewMarkerOnReportAction({ + ...baseParams, + message, + prevSortedVisibleReportActionsObjects, + prevUnreadMarkerReportActionID: 'self-action-a', + isOffline: false, + }), + ).toBe(false); + }); + + it('keeps the marker on the same self-authored action it was previously anchored on', () => { + // When the previously marked action is the action being evaluated, `isDifferentUnread` is false, so a + // persisted self-authored action that already holds the marker keeps it. + const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-a'}); + const prevSortedVisibleReportActionsObjects = { + [message.reportActionID]: makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-a'}), + }; + expect( + shouldDisplayNewMarkerOnReportAction({ + ...baseParams, + message, + prevSortedVisibleReportActionsObjects, + prevUnreadMarkerReportActionID: 'self-action-a', + isOffline: false, + }), + ).toBe(true); + }); + it('returns true when an unread message from another user is new and the list is scrolled over the threshold', () => { const message = makeAction({reportActionID: 'other-new-id'}); expect( From 98a8e1d0c9ea47f71b98185371318b16a101f3cd Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 27 Aug 2026 12:31:11 +0000 Subject: [PATCH 30/43] Update readNewestAction comment to reflect the openReport clearing mechanism Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 27b30d33c312..b14f05c609f0 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -3125,9 +3125,10 @@ function readNewestAction(reportID: string | undefined, isReportActionsLoaded: b lastReadTime, // Intentionally do NOT clear `manuallyMarkedUnreadReportActionID` here. An explicit // mark-as-unread should keep its "New" marker anchored even after the report is auto-read - // (readNewestAction fires whenever the report is focused/visible). The marker is instead - // reconciled when the report is re-loaded via openReport, which returns the server's - // authoritative value for this field. + // (readNewestAction fires whenever the report is focused/visible), so it stays put for the + // duration of the visit that created it. The marker is instead cleared client-side in + // openReport's optimisticData when the user navigates away and comes back, or on a page + // refresh (see the `didNavigateBackToReport || isFirstLoadAfterRefresh` handling there). }, }, ]; From 2657b78cfdefef270c7f7453f2c057b777f9be57 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 27 Aug 2026 13:46:59 +0000 Subject: [PATCH 31/43] Only suppress self-marker hop while previous anchor is still present Fixes the UnreadIndicators 'move marker when unread message is deleted' test: isDifferentUnread now yields when the previous anchor was deleted, so the marker can relocate to the next unread self-authored message. Co-authored-by: Olly --- .../shouldDisplayNewMarkerOnReportAction.ts | 16 +++++++++- tests/unit/ReportActionsUtilsTest.ts | 31 ++++++++++++++++--- 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts index 0fe2cba4fc0c..5dff231a9461 100644 --- a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts +++ b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts @@ -31,6 +31,9 @@ type ShouldDisplayNewMarkerOnReportActionParams = { /** The reportActionID of the current unread marker, if one exists */ prevUnreadMarkerReportActionID?: string | null; + /** Whether the action `prevUnreadMarkerReportActionID` points to is still present (not deleted/hidden) */ + isPrevUnreadMarkerReportActionPresent?: boolean; + /** The reportActionID the user explicitly marked as unread, if any */ manuallyMarkedUnreadReportActionID?: string | null; /** Whether the app window is focused */ @@ -51,6 +54,7 @@ const shouldDisplayNewMarkerOnReportAction = ({ isScrolledOverThreshold, isOffline, prevUnreadMarkerReportActionID, + isPrevUnreadMarkerReportActionPresent = false, manuallyMarkedUnreadReportActionID, hasWindowFocus = true, }: ShouldDisplayNewMarkerOnReportActionParams): boolean => { @@ -109,7 +113,9 @@ const shouldDisplayNewMarkerOnReportAction = ({ // So essentially, the previously unread cannot move from one new self-user-action to another. Once a // self-authored action holds the marker, keep it there rather than letting it hop to a different // self-authored action (e.g. a persisted reimbursable toggle) — the regression from Expensify/App#91940. - const isDifferentUnread = isPreviouslyUnreadFromCurrentUser && prevMarkedReportAction?.reportActionID !== message.reportActionID; + // This only applies while that previous anchor is still present: if it was deleted, the marker must be + // allowed to relocate to the next unread message. + const isDifferentUnread = isPrevUnreadMarkerReportActionPresent && isPreviouslyUnreadFromCurrentUser && prevMarkedReportAction?.reportActionID !== message.reportActionID; const shouldIgnoreUnreadForCurrentUserMessage = isNewMessage || isPreviouslyOptimistic || isDifferentUnread; if (isFromCurrentUser) { @@ -198,6 +204,13 @@ const getUnreadMarkerReportAction = ({ const activeManuallyMarkedUnreadReportActionID = manuallyMarkedUnreadReportAction && !shouldHideNewMarker(manuallyMarkedUnreadReportAction, isOffline) ? manuallyMarkedUnreadReportActionID : null; + // Whether the action the marker was previously anchored on is still present (not deleted/hidden). This + // distinguishes "the anchor was deleted, so let the marker relocate to the next unread message" from + // "the anchor is still around, so a different self-authored action must not steal the marker". + const isPrevUnreadMarkerReportActionPresent = prevUnreadMarkerReportActionID + ? visibleReportActions.some((action) => action.reportActionID === prevUnreadMarkerReportActionID && !shouldHideNewMarker(action, isOffline)) + : false; + const startIndex = isReversed ? visibleReportActions.length - 1 : (earliestReceivedOfflineMessageIndex ?? 0); const endIndex = isReversed ? (earliestReceivedOfflineMessageIndex ?? 0) : visibleReportActions.length; const step = isReversed ? -1 : 1; @@ -233,6 +246,7 @@ const getUnreadMarkerReportAction = ({ isScrolledOverThreshold, isOffline, prevUnreadMarkerReportActionID, + isPrevUnreadMarkerReportActionPresent, manuallyMarkedUnreadReportActionID: activeManuallyMarkedUnreadReportActionID, hasWindowFocus, }); diff --git a/tests/unit/ReportActionsUtilsTest.ts b/tests/unit/ReportActionsUtilsTest.ts index 90678968d836..56dc82583d09 100644 --- a/tests/unit/ReportActionsUtilsTest.ts +++ b/tests/unit/ReportActionsUtilsTest.ts @@ -6132,10 +6132,10 @@ describe('ReportActionsUtils', () => { ).toBe(true); }); - it('does not move the marker from one self-authored action to a different self-authored action', () => { - // The previously marked action was a persisted self-authored message. A different self-authored - // action must not steal the "New" marker off it (the Expensify/App#91940 hop), so `isDifferentUnread` - // suppresses the marker here even though this action reads as unread. + it('does not move the marker from one self-authored action to a different self-authored action while the previous anchor is still present', () => { + // The previously marked action was a persisted self-authored message that is still present. A different + // self-authored action must not steal the "New" marker off it (the Expensify/App#91940 hop), so + // `isDifferentUnread` suppresses the marker here even though this action reads as unread. const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-b'}); const prevMarkedAction = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-a'}); const prevSortedVisibleReportActionsObjects = { @@ -6148,11 +6148,33 @@ describe('ReportActionsUtils', () => { message, prevSortedVisibleReportActionsObjects, prevUnreadMarkerReportActionID: 'self-action-a', + isPrevUnreadMarkerReportActionPresent: true, isOffline: false, }), ).toBe(false); }); + it('moves the marker to another self-authored action once the previous anchor has been deleted', () => { + // When the previous self-authored anchor is no longer present (deleted), the marker must be allowed to + // relocate to the next unread self-authored message, so `isDifferentUnread` does not suppress it. + const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-b'}); + const prevMarkedAction = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-a'}); + const prevSortedVisibleReportActionsObjects = { + [prevMarkedAction.reportActionID]: prevMarkedAction, + [message.reportActionID]: makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-b'}), + }; + expect( + shouldDisplayNewMarkerOnReportAction({ + ...baseParams, + message, + prevSortedVisibleReportActionsObjects, + prevUnreadMarkerReportActionID: 'self-action-a', + isPrevUnreadMarkerReportActionPresent: false, + isOffline: false, + }), + ).toBe(true); + }); + it('keeps the marker on the same self-authored action it was previously anchored on', () => { // When the previously marked action is the action being evaluated, `isDifferentUnread` is false, so a // persisted self-authored action that already holds the marker keeps it. @@ -6166,6 +6188,7 @@ describe('ReportActionsUtils', () => { message, prevSortedVisibleReportActionsObjects, prevUnreadMarkerReportActionID: 'self-action-a', + isPrevUnreadMarkerReportActionPresent: true, isOffline: false, }), ).toBe(true); From 5e43f7d77b27c6228547c445a2ad3eaba9ab65e2 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 27 Aug 2026 14:05:51 +0000 Subject: [PATCH 32/43] Discard ignored navigateToConciergeChat promise in fire-and-forget callers navigateToConciergeChat became Promise on main; these three callers passed it straight to a void-returning handler, tripping no-misused-promises once main was merged in. Wrap the calls so the ignored promise is discarded. Co-authored-by: Olly --- .../USD/ConnectBankAccount/ConnectBankAccount.tsx | 4 +++- src/pages/settings/AboutPage/AboutPage.tsx | 4 +++- .../companyCards/WorkspaceCompanyCardsFeedPendingPage.tsx | 6 +++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/pages/ReimbursementAccount/USD/ConnectBankAccount/ConnectBankAccount.tsx b/src/pages/ReimbursementAccount/USD/ConnectBankAccount/ConnectBankAccount.tsx index 345f22090036..6d3faa285747 100644 --- a/src/pages/ReimbursementAccount/USD/ConnectBankAccount/ConnectBankAccount.tsx +++ b/src/pages/ReimbursementAccount/USD/ConnectBankAccount/ConnectBankAccount.tsx @@ -62,7 +62,9 @@ function ConnectBankAccount({onBackButtonPress, setShouldShowConnectedVerifiedBa const [isSelfTourViewed] = useOnyx(ONYXKEYS.NVP_ONBOARDING, {selector: hasSeenTourSelector}); const {accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); - const handleNavigateToConciergeChat = () => navigateToConciergeChat(conciergeReportID, introSelected, currentUserAccountID, isSelfTourViewed, betas, true); + const handleNavigateToConciergeChat = () => { + navigateToConciergeChat(conciergeReportID, introSelected, currentUserAccountID, isSelfTourViewed, betas, true); + }; const bankAccountState = reimbursementAccount?.achData?.state ?? ''; const pendingAction = reimbursementAccount?.pendingAction; diff --git a/src/pages/settings/AboutPage/AboutPage.tsx b/src/pages/settings/AboutPage/AboutPage.tsx index f360778bd116..91ec2738ef10 100644 --- a/src/pages/settings/AboutPage/AboutPage.tsx +++ b/src/pages/settings/AboutPage/AboutPage.tsx @@ -118,7 +118,9 @@ function AboutPage() { translationKey: 'initialSettingsPage.aboutPage.reportABug', icon: icons.Bug, sentryLabel: CONST.SENTRY_LABEL.SETTINGS_ABOUT.REPORT_A_BUG, - action: waitForNavigate(() => navigateToConciergeChat(conciergeReportID, introSelected, currentUserAccountID, isSelfTourViewed, betas, false)), + action: waitForNavigate(() => { + navigateToConciergeChat(conciergeReportID, introSelected, currentUserAccountID, isSelfTourViewed, betas, false); + }), }, ]; diff --git a/src/pages/workspace/companyCards/WorkspaceCompanyCardsFeedPendingPage.tsx b/src/pages/workspace/companyCards/WorkspaceCompanyCardsFeedPendingPage.tsx index b96be42d0261..1ed10d23a6f4 100644 --- a/src/pages/workspace/companyCards/WorkspaceCompanyCardsFeedPendingPage.tsx +++ b/src/pages/workspace/companyCards/WorkspaceCompanyCardsFeedPendingPage.tsx @@ -38,7 +38,11 @@ function WorkspaceCompanyCardsFeedPendingPage() { > {translate('workspace.moreFeatures.companyCards.pendingFeedDescription')} - navigateToConciergeChat(conciergeReportID, introSelected, currentUserAccountID, isSelfTourViewed, betas, false)}> + { + navigateToConciergeChat(conciergeReportID, introSelected, currentUserAccountID, isSelfTourViewed, betas, false); + }} + > {' '} {CONST.CONCIERGE_CHAT_NAME} From 01c874428b96bca71d9f993d8662b679d824dea6 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 27 Aug 2026 16:45:04 +0000 Subject: [PATCH 33/43] Don't reassert manuallyMarkedUnreadReportActionID in markAsUnread successData openReport clears the marker optimistically on navigate-back/refresh. Reasserting the id in successData resurrected it when a queued (offline) mark request completed on reconnect. The optimistic value persists via MERGE on its own. Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 19ce24b96605..66de69140109 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -3231,11 +3231,19 @@ function markCommentAsUnread(reportID: string | undefined, reportActions: OnyxEn }, ]; + // Do NOT reassert `manuallyMarkedUnreadReportActionID` in successData. `openReport` clears it optimistically + // when the user navigates away and comes back (or refreshes); if this request is still queued at that point + // (e.g. the mark happened offline), reasserting the id when it completes on reconnect would resurrect a + // marker the user has already moved past. The optimistic value set above persists on its own — the server + // response is a MERGE that never carries this client-only field — so there is nothing to reassert here. const successData: Array> = [ { onyxMethod: Onyx.METHOD.MERGE, key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, - value: reportValue, + value: { + lastReadTime, + ...(lastActorAccountID && {lastActorAccountID}), + }, }, ]; From 16ac539cfb2112bf367cb7f6a14c09ebaf0d7bcf Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Tue, 8 Sep 2026 19:34:32 +0000 Subject: [PATCH 34/43] Apply review suggestions: trim marker comments; clear manual unread mark on explicit Mark as read Co-authored-by: Olly --- src/hooks/useUnreadMarker.ts | 6 +----- src/libs/actions/Report/index.ts | 10 +++------- .../report/shouldDisplayNewMarkerOnReportAction.ts | 5 ++--- 3 files changed, 6 insertions(+), 15 deletions(-) diff --git a/src/hooks/useUnreadMarker.ts b/src/hooks/useUnreadMarker.ts index efb97d5ed447..8e3c4e660c17 100644 --- a/src/hooks/useUnreadMarker.ts +++ b/src/hooks/useUnreadMarker.ts @@ -134,11 +134,7 @@ function useUnreadMarker({ const [unreadMarkerReportActionID, unreadMarkerReportActionIndex]: [string | null, number] = oldestUnreadReportActionMarker && (scanned[0] === null || scanned[0] === oldestUnreadReportActionMarker[0]) ? oldestUnreadReportActionMarker : scanned; - // `prevUnreadMarkerReportActionID` records the action the marker was last anchored on and gates the - // self-authored-message branch in `shouldDisplayNewMarkerOnReportAction`, which only runs once no manual - // mark is active. The #91940 regression (a persisted self-authored action wrongly keeping the "New" - // marker after the marker moves) is prevented there via the `isDifferentUnread` check, so we simply track - // whatever the marker last landed on here. + // Track whatever the marker last landed on. The self-message guard lives in shouldDisplayNewMarkerOnReportAction. if (prevUnreadMarkerReportActionID !== unreadMarkerReportActionID) { setPrevUnreadMarkerReportActionID(unreadMarkerReportActionID); } diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 66de69140109..ff7e113c58aa 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -1738,7 +1738,7 @@ function openReport(params: OpenReportActionParams) { const optimisticReport: Partial> = hasReportActions || !existingReportName ? {} : {reportName: existingReportName}; // An explicit mark-as-unread keeps its "New" marker anchored while the user stays in the report - // (readNewestAction no longer clears it, and the repeated openReport calls of a single visit don't + // (auto-reads no longer clear it, and the repeated openReport calls of a single visit don't // either), so the user sees the marker they created. It is reconciled away in two cases: when the user // navigates away and comes back (`didNavigateBackToReport`), and on a page refresh (`isFirstLoadAfterRefresh`). // This is a purely client-side decision, so it lives in optimisticData: it must apply immediately and @@ -3137,12 +3137,8 @@ function readNewestAction(reportID: string | undefined, isReportActionsLoaded: b key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, value: { lastReadTime, - // Intentionally do NOT clear `manuallyMarkedUnreadReportActionID` here. An explicit - // mark-as-unread should keep its "New" marker anchored even after the report is auto-read - // (readNewestAction fires whenever the report is focused/visible), so it stays put for the - // duration of the visit that created it. The marker is instead cleared client-side in - // openReport's optimisticData when the user navigates away and comes back, or on a page - // refresh (see the `didNavigateBackToReport || isFirstLoadAfterRefresh` handling there). + // Auto-reads keep a manual unread mark. The explicit "Mark as read" clears it here, openReport clears it on return or refresh. + ...(shouldResetUnreadMarker && {manuallyMarkedUnreadReportActionID: null}), }, }, ]; diff --git a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts index 5dff231a9461..08174f4ad7fb 100644 --- a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts +++ b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts @@ -110,9 +110,8 @@ const shouldDisplayNewMarkerOnReportAction = ({ // just optimistic, preserving the #91940 behavior for cold opens. const prevMarkedReportAction = prevUnreadMarkerReportActionID ? prevSortedVisibleReportActionsObjects[prevUnreadMarkerReportActionID] : undefined; const isPreviouslyUnreadFromCurrentUser = currentUserAccountID === prevMarkedReportAction?.actorAccountID; - // So essentially, the previously unread cannot move from one new self-user-action to another. Once a - // self-authored action holds the marker, keep it there rather than letting it hop to a different - // self-authored action (e.g. a persisted reimbursable toggle) — the regression from Expensify/App#91940. + // Once a self-authored action holds the marker, keep it there rather than letting it hop to a different + // self-authored action (e.g. a persisted reimbursable toggle) - the regression from Expensify/App#91940. // This only applies while that previous anchor is still present: if it was deleted, the marker must be // allowed to relocate to the next unread message. const isDifferentUnread = isPrevUnreadMarkerReportActionPresent && isPreviouslyUnreadFromCurrentUser && prevMarkedReportAction?.reportActionID !== message.reportActionID; From 75b2104ca229b350fc9de50804c25fa62fd46cf6 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Tue, 8 Sep 2026 19:47:04 +0000 Subject: [PATCH 35/43] Add readNewestAction coverage for manual unread mark clearing Co-authored-by: Olly --- artifacts/00-initial.png | Bin 0 -> 3420 bytes artifacts/00b-check.png | Bin 0 -> 3420 bytes artifacts/00c-after-wait.png | Bin 0 -> 3420 bytes tests/actions/ReportTest.ts | 52 +++++++++++++++++++++++++++++++++++ 4 files changed, 52 insertions(+) create mode 100644 artifacts/00-initial.png create mode 100644 artifacts/00b-check.png create mode 100644 artifacts/00c-after-wait.png diff --git a/artifacts/00-initial.png b/artifacts/00-initial.png new file mode 100644 index 0000000000000000000000000000000000000000..da2538124197bf44b507ca673801f7a1ad8032ac GIT binary patch literal 3420 zcmeAS@N?(olHy`uVBq!ia0y~yU { }); }); + describe('readNewestAction', () => { + const READ_NEWEST_REPORT_ID = '9001'; + + /** Puts the report in the state left behind by an explicit mark-as-unread. */ + async function givenManuallyMarkedUnreadReport() { + global.fetch = TestHelper.createGlobalFetchMock(); + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${READ_NEWEST_REPORT_ID}`, { + ...createRandomReport(Number(READ_NEWEST_REPORT_ID), undefined), + reportID: READ_NEWEST_REPORT_ID, + manuallyMarkedUnreadReportActionID: '1', + }); + await waitForBatchedUpdates(); + } + + function getManuallyMarkedUnreadReportActionID() { + return new Promise((resolve) => { + const connection = Onyx.connect({ + key: `${ONYXKEYS.COLLECTION.REPORT}${READ_NEWEST_REPORT_ID}`, + callback: (reportVal) => { + Onyx.disconnect(connection); + resolve(reportVal?.manuallyMarkedUnreadReportActionID); + }, + }); + }); + } + + it('should keep the manual unread mark when the report is auto-read', async () => { + // Given a report the user explicitly marked as unread + await givenManuallyMarkedUnreadReport(); + + // When the report is auto-read (focused/visible), i.e. without resetting the unread marker + Report.readNewestAction(READ_NEWEST_REPORT_ID, true); + await waitForBatchedUpdates(); + + // Then the "New" marker stays anchored on the action the user marked + expect(await getManuallyMarkedUnreadReportActionID()).toBe('1'); + }); + + it('should clear the manual unread mark when the user explicitly marks the report as read', async () => { + // Given a report the user explicitly marked as unread + await givenManuallyMarkedUnreadReport(); + + // When the user picks "Mark as read" from the LHN context menu, which resets the unread marker + Report.readNewestAction(READ_NEWEST_REPORT_ID, true, true); + await waitForBatchedUpdates(); + + // Then the "New" marker is no longer anchored on that action. A `null` in an Onyx MERGE removes + // the key, so the field reads back as `undefined` rather than `null`. + expect(await getManuallyMarkedUnreadReportActionID()).toBeUndefined(); + }); + }); + describe('updateDescription', () => { const currentUserAccountID = 1; it('should not call UpdateRoomDescription API if the description is not changed', async () => { From b5b7c7afe114b06aa1630a656dbc99e98ad9324e Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Tue, 8 Sep 2026 20:07:16 +0000 Subject: [PATCH 36/43] Revert readNewestAction manual-unread clearing per review; keep marker on explicit Mark as read Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 5 ++--- tests/actions/ReportTest.ts | 8 ++++---- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index ff7e113c58aa..caf4069ca6b1 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -1738,7 +1738,7 @@ function openReport(params: OpenReportActionParams) { const optimisticReport: Partial> = hasReportActions || !existingReportName ? {} : {reportName: existingReportName}; // An explicit mark-as-unread keeps its "New" marker anchored while the user stays in the report - // (auto-reads no longer clear it, and the repeated openReport calls of a single visit don't + // (readNewestAction no longer clears it, and the repeated openReport calls of a single visit don't // either), so the user sees the marker they created. It is reconciled away in two cases: when the user // navigates away and comes back (`didNavigateBackToReport`), and on a page refresh (`isFirstLoadAfterRefresh`). // This is a purely client-side decision, so it lives in optimisticData: it must apply immediately and @@ -3137,8 +3137,7 @@ function readNewestAction(reportID: string | undefined, isReportActionsLoaded: b key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, value: { lastReadTime, - // Auto-reads keep a manual unread mark. The explicit "Mark as read" clears it here, openReport clears it on return or refresh. - ...(shouldResetUnreadMarker && {manuallyMarkedUnreadReportActionID: null}), + // Reads keep a manual unread mark, including the explicit "Mark as read". openReport clears it on return or refresh. }, }, ]; diff --git a/tests/actions/ReportTest.ts b/tests/actions/ReportTest.ts index 273454df2c61..d87506089ea2 100644 --- a/tests/actions/ReportTest.ts +++ b/tests/actions/ReportTest.ts @@ -3355,7 +3355,7 @@ describe('actions/Report', () => { expect(await getManuallyMarkedUnreadReportActionID()).toBe('1'); }); - it('should clear the manual unread mark when the user explicitly marks the report as read', async () => { + it('should keep the manual unread mark when the user explicitly marks the report as read', async () => { // Given a report the user explicitly marked as unread await givenManuallyMarkedUnreadReport(); @@ -3363,9 +3363,9 @@ describe('actions/Report', () => { Report.readNewestAction(READ_NEWEST_REPORT_ID, true, true); await waitForBatchedUpdates(); - // Then the "New" marker is no longer anchored on that action. A `null` in an Onyx MERGE removes - // the key, so the field reads back as `undefined` rather than `null`. - expect(await getManuallyMarkedUnreadReportActionID()).toBeUndefined(); + // Then the "New" marker still stays anchored on that action. Only openReport clears the mark, on + // return to the report or on a refresh. + expect(await getManuallyMarkedUnreadReportActionID()).toBe('1'); }); }); From 36a44ea93d2f04770431c0dd6ab07c63120198a8 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Tue, 8 Sep 2026 20:29:46 +0000 Subject: [PATCH 37/43] Roll back to 01c874428b96bca71d9f993d8662b679d824dea6 Reverts 16ac539cfb2, 75b2104ca22, and b5b7c7afe11, restoring the tree to 01c874428b9 exactly. Requested by Ollyws. Co-authored-by: Olly --- artifacts/00-initial.png | Bin 3420 -> 0 bytes artifacts/00b-check.png | Bin 3420 -> 0 bytes artifacts/00c-after-wait.png | Bin 3420 -> 0 bytes src/hooks/useUnreadMarker.ts | 6 +- src/libs/actions/Report/index.ts | 7 ++- .../shouldDisplayNewMarkerOnReportAction.ts | 5 +- tests/actions/ReportTest.ts | 52 ------------------ 7 files changed, 14 insertions(+), 56 deletions(-) delete mode 100644 artifacts/00-initial.png delete mode 100644 artifacts/00b-check.png delete mode 100644 artifacts/00c-after-wait.png diff --git a/artifacts/00-initial.png b/artifacts/00-initial.png deleted file mode 100644 index da2538124197bf44b507ca673801f7a1ad8032ac..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 3420 zcmeAS@N?(olHy`uVBq!ia0y~yU { }); }); - describe('readNewestAction', () => { - const READ_NEWEST_REPORT_ID = '9001'; - - /** Puts the report in the state left behind by an explicit mark-as-unread. */ - async function givenManuallyMarkedUnreadReport() { - global.fetch = TestHelper.createGlobalFetchMock(); - await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${READ_NEWEST_REPORT_ID}`, { - ...createRandomReport(Number(READ_NEWEST_REPORT_ID), undefined), - reportID: READ_NEWEST_REPORT_ID, - manuallyMarkedUnreadReportActionID: '1', - }); - await waitForBatchedUpdates(); - } - - function getManuallyMarkedUnreadReportActionID() { - return new Promise((resolve) => { - const connection = Onyx.connect({ - key: `${ONYXKEYS.COLLECTION.REPORT}${READ_NEWEST_REPORT_ID}`, - callback: (reportVal) => { - Onyx.disconnect(connection); - resolve(reportVal?.manuallyMarkedUnreadReportActionID); - }, - }); - }); - } - - it('should keep the manual unread mark when the report is auto-read', async () => { - // Given a report the user explicitly marked as unread - await givenManuallyMarkedUnreadReport(); - - // When the report is auto-read (focused/visible), i.e. without resetting the unread marker - Report.readNewestAction(READ_NEWEST_REPORT_ID, true); - await waitForBatchedUpdates(); - - // Then the "New" marker stays anchored on the action the user marked - expect(await getManuallyMarkedUnreadReportActionID()).toBe('1'); - }); - - it('should keep the manual unread mark when the user explicitly marks the report as read', async () => { - // Given a report the user explicitly marked as unread - await givenManuallyMarkedUnreadReport(); - - // When the user picks "Mark as read" from the LHN context menu, which resets the unread marker - Report.readNewestAction(READ_NEWEST_REPORT_ID, true, true); - await waitForBatchedUpdates(); - - // Then the "New" marker still stays anchored on that action. Only openReport clears the mark, on - // return to the report or on a refresh. - expect(await getManuallyMarkedUnreadReportActionID()).toBe('1'); - }); - }); - describe('updateDescription', () => { const currentUserAccountID = 1; it('should not call UpdateRoomDescription API if the description is not changed', async () => { From d3fe77507cc9d3ad869992c788b0c1d12c963b2e Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Tue, 8 Sep 2026 20:50:27 +0000 Subject: [PATCH 38/43] Trim the self-authored marker-hop comment per review Co-authored-by: Olly --- .../inbox/report/shouldDisplayNewMarkerOnReportAction.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts index 5dff231a9461..08174f4ad7fb 100644 --- a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts +++ b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts @@ -110,9 +110,8 @@ const shouldDisplayNewMarkerOnReportAction = ({ // just optimistic, preserving the #91940 behavior for cold opens. const prevMarkedReportAction = prevUnreadMarkerReportActionID ? prevSortedVisibleReportActionsObjects[prevUnreadMarkerReportActionID] : undefined; const isPreviouslyUnreadFromCurrentUser = currentUserAccountID === prevMarkedReportAction?.actorAccountID; - // So essentially, the previously unread cannot move from one new self-user-action to another. Once a - // self-authored action holds the marker, keep it there rather than letting it hop to a different - // self-authored action (e.g. a persisted reimbursable toggle) — the regression from Expensify/App#91940. + // Once a self-authored action holds the marker, keep it there rather than letting it hop to a different + // self-authored action (e.g. a persisted reimbursable toggle) - the regression from Expensify/App#91940. // This only applies while that previous anchor is still present: if it was deleted, the marker must be // allowed to relocate to the next unread message. const isDifferentUnread = isPrevUnreadMarkerReportActionPresent && isPreviouslyUnreadFromCurrentUser && prevMarkedReportAction?.reportActionID !== message.reportActionID; From d1a2d52754a5f7b873ff8d8b800f6a0b826d08bf Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Tue, 8 Sep 2026 20:59:14 +0000 Subject: [PATCH 39/43] Trim the prevUnreadMarkerReportActionID tracking comment per review Co-authored-by: Olly --- src/hooks/useUnreadMarker.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/hooks/useUnreadMarker.ts b/src/hooks/useUnreadMarker.ts index efb97d5ed447..8e3c4e660c17 100644 --- a/src/hooks/useUnreadMarker.ts +++ b/src/hooks/useUnreadMarker.ts @@ -134,11 +134,7 @@ function useUnreadMarker({ const [unreadMarkerReportActionID, unreadMarkerReportActionIndex]: [string | null, number] = oldestUnreadReportActionMarker && (scanned[0] === null || scanned[0] === oldestUnreadReportActionMarker[0]) ? oldestUnreadReportActionMarker : scanned; - // `prevUnreadMarkerReportActionID` records the action the marker was last anchored on and gates the - // self-authored-message branch in `shouldDisplayNewMarkerOnReportAction`, which only runs once no manual - // mark is active. The #91940 regression (a persisted self-authored action wrongly keeping the "New" - // marker after the marker moves) is prevented there via the `isDifferentUnread` check, so we simply track - // whatever the marker last landed on here. + // Track whatever the marker last landed on. The self-message guard lives in shouldDisplayNewMarkerOnReportAction. if (prevUnreadMarkerReportActionID !== unreadMarkerReportActionID) { setPrevUnreadMarkerReportActionID(unreadMarkerReportActionID); } From f03b44507746149372edbc1a4c5218d5e0df7674 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Tue, 8 Sep 2026 21:07:29 +0000 Subject: [PATCH 40/43] Trim the readNewestAction manual-unread comment per review Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 66de69140109..36ff946d35c8 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -3137,12 +3137,7 @@ function readNewestAction(reportID: string | undefined, isReportActionsLoaded: b key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, value: { lastReadTime, - // Intentionally do NOT clear `manuallyMarkedUnreadReportActionID` here. An explicit - // mark-as-unread should keep its "New" marker anchored even after the report is auto-read - // (readNewestAction fires whenever the report is focused/visible), so it stays put for the - // duration of the visit that created it. The marker is instead cleared client-side in - // openReport's optimisticData when the user navigates away and comes back, or on a page - // refresh (see the `didNavigateBackToReport || isFirstLoadAfterRefresh` handling there). + // Auto-reads keep a manual unread mark. The explicit "Mark as read" clears it here, openReport clears it on return or refresh. }, }, ]; From 3b117ad3a6608513a3d9715e486ef6e0d13e3350 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Tue, 8 Sep 2026 21:55:18 +0000 Subject: [PATCH 41/43] Use connectWithoutView for the RAM-only report loading state to fix ESLint Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index b6381229871f..98c1b9d7c700 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -544,8 +544,9 @@ function flagReportNavigatedAway(reportID: string | undefined) { // openReport of the session and resets only on a genuine reload (page refresh / cold start), so it's the // signal for "has this report already been loaded this session" — used by openReport below to also clear the // manual unread marker on a page refresh, on top of the navigate-away-and-back case above. +// We use connectWithoutView because this is only read inside the `openReport` action, never during render. let allReportLoadingStates: OnyxCollection; -Onyx.connect({ +Onyx.connectWithoutView({ key: ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE, callback: (value) => { allReportLoadingStates = value; From c4a3edfc35ed2cd2292541bec0e50bcd02a2de41 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Thu, 10 Sep 2026 21:38:47 +0000 Subject: [PATCH 42/43] Pass hasOnceLoadedReportActions to openReport instead of reading it from Onyx Co-authored-by: Olly --- src/libs/actions/Report/index.ts | 36 +++++++------- src/pages/inbox/ReportFetchHandler.tsx | 4 ++ tests/actions/ReportTest.ts | 69 ++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 19 deletions(-) diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 98c1b9d7c700..1882537341cd 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -244,7 +244,6 @@ import type { Report, ReportAction, ReportAttributesDerivedValue, - ReportLoadingState, ReportUserIsTyping, SidePanelContext, Transaction, @@ -377,6 +376,14 @@ type OpenReportActionParams = { hasReportActions: boolean | undefined; + /** + * Whether this report's actions have already been loaded at least once this session, read from the RAM-only + * report loading state. Only the report screen knows this and only it needs to pass it: a falsy value means + * a page refresh / cold start, which is when a manual unread marker is cleared. Callers that open a report + * for any other reason omit it, and the marker is left alone. + */ + hasOnceLoadedReportActions?: boolean; + /** Whether opening the report should update its read state. Set to false when fetching report data without the user actually viewing the conversation */ shouldMarkAsRead?: boolean; @@ -540,19 +547,6 @@ function flagReportNavigatedAway(reportID: string | undefined) { reportsNavigatedAwayFrom.add(reportID); } -// RAM-only per-report loading state. `hasOnceLoadedReportActions` is false until the first successful -// openReport of the session and resets only on a genuine reload (page refresh / cold start), so it's the -// signal for "has this report already been loaded this session" — used by openReport below to also clear the -// manual unread marker on a page refresh, on top of the navigate-away-and-back case above. -// We use connectWithoutView because this is only read inside the `openReport` action, never during render. -let allReportLoadingStates: OnyxCollection; -Onyx.connectWithoutView({ - key: ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE, - callback: (value) => { - allReportLoadingStates = value; - }, -}); - let allPersonalDetails: OnyxEntry = {}; Onyx.connect({ key: ONYXKEYS.PERSONAL_DETAILS_LIST, @@ -1703,6 +1697,9 @@ function openReport(params: OpenReportActionParams) { isSelfTourViewed, hasCompletedGuidedSetupFlow, hasReportActions, + // Defaults to true so that only the report screen, which actually passes this, can clear a manual unread + // marker. Every other caller opens a report for an unrelated reason and must leave the marker untouched. + hasOnceLoadedReportActions = true, shouldMarkAsRead = true, conciergeChat, } = params; @@ -1721,11 +1718,12 @@ function openReport(params: OpenReportActionParams) { // (the set is RAM-only). We consume it here to clear a manual unread marker on that return trip. const didNavigateBackToReport = reportsNavigatedAwayFrom.has(reportID); reportsNavigatedAwayFrom.delete(reportID); - // Whether this is the first load of the report this session. `hasOnceLoadedReportActions` is RAM-only, so a - // page refresh / cold start resets it to falsy — that's how we detect a refresh here. A manual unread marker - // can only be non-null on a first load if it was persisted from before the refresh, so clearing it here - // clears the marker on a page refresh while leaving genuine first opens (marker already null) untouched. - const isFirstLoadAfterRefresh = !allReportLoadingStates?.[`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${reportID}`]?.hasOnceLoadedReportActions; + // Whether this is the first load of the report this session. The report screen passes its RAM-only + // `hasOnceLoadedReportActions`, so a page refresh / cold start resets it to falsy — that's how we detect a + // refresh here. A manual unread marker can only be non-null on a first load if it was persisted from before + // the refresh, so clearing it here clears the marker on a page refresh while leaving genuine first opens + // (marker already null) untouched. + const isFirstLoadAfterRefresh = !hasOnceLoadedReportActions; const optimisticReport: Partial> = hasReportActions || !existingReportName ? {} : {reportName: existingReportName}; // An explicit mark-as-unread keeps its "New" marker anchored while the user stays in the report diff --git a/src/pages/inbox/ReportFetchHandler.tsx b/src/pages/inbox/ReportFetchHandler.tsx index dddce3b4bc30..a638a11de9bb 100644 --- a/src/pages/inbox/ReportFetchHandler.tsx +++ b/src/pages/inbox/ReportFetchHandler.tsx @@ -218,6 +218,10 @@ function ReportFetchHandler() { participants: dmParticipants, betas, hasReportActions, + // openReport clears a manual unread marker when this is falsy, which is how a page refresh / cold + // start is detected. This screen is the only place that opens the report the user is looking at, so + // it is the only caller that passes it. + hasOnceLoadedReportActions: reportLoadingState.hasOnceLoadedReportActions, currentUserAccountID, isSelfTourViewed, hasCompletedGuidedSetupFlow, diff --git a/tests/actions/ReportTest.ts b/tests/actions/ReportTest.ts index a4fd788509bd..a5d80af4746b 100644 --- a/tests/actions/ReportTest.ts +++ b/tests/actions/ReportTest.ts @@ -5258,6 +5258,75 @@ describe('actions/Report', () => { }); }); + describe('openReport with hasOnceLoadedReportActions', () => { + /** Puts a manual unread mark on the report, the way markCommentAsUnread does, so we can watch openReport clear it. */ + async function givenAManualUnreadMark(reportID: string) { + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {reportID, manuallyMarkedUnreadReportActionID: 'marked-action-id'}); + await waitForBatchedUpdates(); + } + + async function getManualUnreadMark(reportID: string) { + const report = await getOnyxValue(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`); + return report?.manuallyMarkedUnreadReportActionID; + } + + it('should clear the manual unread mark when false, because a falsy value means a page refresh / cold start', async () => { + global.fetch = TestHelper.createGlobalFetchMock(); + const REPORT_ID = 'unreadMarkRefresh'; + await givenAManualUnreadMark(REPORT_ID); + + Report.openReport({ + conciergeChat: undefined, + reportID: REPORT_ID, + introSelected: undefined, + betas: undefined, + hasReportActions: true, + currentUserAccountID: 1, + hasOnceLoadedReportActions: false, + }); + await waitForBatchedUpdates(); + + expect(await getManualUnreadMark(REPORT_ID)).toBeFalsy(); + }); + + it('should keep the manual unread mark when true, so the marker survives the repeated openReport calls of a single visit', async () => { + global.fetch = TestHelper.createGlobalFetchMock(); + const REPORT_ID = 'unreadMarkSameVisit'; + await givenAManualUnreadMark(REPORT_ID); + + Report.openReport({ + conciergeChat: undefined, + reportID: REPORT_ID, + introSelected: undefined, + betas: undefined, + hasReportActions: true, + currentUserAccountID: 1, + hasOnceLoadedReportActions: true, + }); + await waitForBatchedUpdates(); + + expect(await getManualUnreadMark(REPORT_ID)).toBe('marked-action-id'); + }); + + it('should keep the manual unread mark when omitted, so callers other than the report screen never clear it', async () => { + global.fetch = TestHelper.createGlobalFetchMock(); + const REPORT_ID = 'unreadMarkOtherCaller'; + await givenAManualUnreadMark(REPORT_ID); + + Report.openReport({ + conciergeChat: undefined, + reportID: REPORT_ID, + introSelected: undefined, + betas: undefined, + hasReportActions: true, + currentUserAccountID: 1, + }); + await waitForBatchedUpdates(); + + expect(await getManualUnreadMark(REPORT_ID)).toBe('marked-action-id'); + }); + }); + describe('openReport with participants', () => { it('should send passed participants as emailList/accountIDList so the server can resolve a stale optimistic reportID', async () => { global.fetch = TestHelper.createGlobalFetchMock(); From 3e967d6498a231c4e0b264859428658fca7a6bb6 Mon Sep 17 00:00:00 2001 From: "Olly (via MelvinBot)" Date: Tue, 15 Sep 2026 12:39:07 +0000 Subject: [PATCH 43/43] Make the unread-marker comments more concise Co-authored-by: Olly --- src/hooks/useUnreadMarker.ts | 1 - src/libs/actions/Report/index.ts | 59 +++++++------------ src/pages/inbox/ReportFetchHandler.tsx | 12 ++-- .../shouldDisplayNewMarkerOnReportAction.ts | 40 ++++--------- src/types/onyx/Report.ts | 4 +- tests/actions/ReportTest.ts | 2 +- tests/unit/ReportActionsUtilsTest.ts | 37 +++++------- tests/unit/useUnreadMarkerTest.ts | 7 +-- 8 files changed, 58 insertions(+), 104 deletions(-) diff --git a/src/hooks/useUnreadMarker.ts b/src/hooks/useUnreadMarker.ts index d71cd75f2204..acd16b98c70c 100644 --- a/src/hooks/useUnreadMarker.ts +++ b/src/hooks/useUnreadMarker.ts @@ -141,7 +141,6 @@ function useUnreadMarker({ const [unreadMarkerReportActionID, unreadMarkerReportActionIndex]: [string | null, number] = oldestUnreadReportActionMarker && (scanned[0] === null || scanned[0] === oldestUnreadReportActionMarker[0]) ? oldestUnreadReportActionMarker : scanned; - // Track whatever the marker last landed on. The self-message guard lives in shouldDisplayNewMarkerOnReportAction. if (prevUnreadMarkerReportActionID !== unreadMarkerReportActionID) { setPrevUnreadMarkerReportActionID(unreadMarkerReportActionID); } diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 2f1c5548f392..f817ba94edf8 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -380,10 +380,9 @@ type OpenReportActionParams = { hasReportActions: boolean | undefined; /** - * Whether this report's actions have already been loaded at least once this session, read from the RAM-only - * report loading state. Only the report screen knows this and only it needs to pass it: a falsy value means - * a page refresh / cold start, which is when a manual unread marker is cleared. Callers that open a report - * for any other reason omit it, and the marker is left alone. + * Whether this report's actions loaded at least once this session (RAM-only, so falsy means a page refresh / + * cold start — when a manual unread marker is cleared). Only the report screen passes it; other callers omit + * it to leave the marker alone. */ hasOnceLoadedReportActions?: boolean; @@ -533,17 +532,12 @@ Onyx.connect({ }, }); -// RAM-only set of reportIDs the user has navigated away from this session. It is populated when the report -// screen blurs/unmounts (see `flagReportNavigatedAway`) and consumed by `openReport` to clear a manual unread -// marker on the *return* trip only. A blur is the one signal that uniquely identifies "navigated away and back -// to the chat": it does not fire on the multiple `openReport` calls of a single visit, and — being RAM-only — -// it is empty after a page refresh, so the marker survives a refresh and is only cleared by a real navigation. +// RAM-only set of reportIDs the user navigated away from this session, so `openReport` can clear a manual +// unread marker on the return trip only. A blur uniquely identifies that trip: it doesn't fire on the repeated +// openReport calls of a single visit, and being RAM-only it is empty after a refresh. const reportsNavigatedAwayFrom = new Set(); -/** - * Records that the user has navigated away from the given report. Called from the report screen when it blurs - * or unmounts. The next `openReport` for this report will clear its manual unread marker and drop it from the set. - */ +/** Records that the user navigated away from the report, so the next `openReport` clears its manual unread marker. */ function flagReportNavigatedAway(reportID: string | undefined) { if (!reportID) { return; @@ -1702,8 +1696,7 @@ function openReport(params: OpenReportActionParams) { isSelfTourViewed, hasCompletedGuidedSetupFlow, hasReportActions, - // Defaults to true so that only the report screen, which actually passes this, can clear a manual unread - // marker. Every other caller opens a report for an unrelated reason and must leave the marker untouched. + // Defaults to true so only the report screen, the one caller that passes it, can clear a manual unread marker. hasOnceLoadedReportActions = true, shouldMarkAsRead = true, conciergeChat, @@ -1716,28 +1709,18 @@ function openReport(params: OpenReportActionParams) { const participantAccountIDList = participants.map((p) => p.accountID).filter((id): id is number => id !== undefined); const existingReportName = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.reportName; const isCreatingNewReport = !isEmptyObject(newReportObject); - // Whether the user navigated away from this report and is now coming back to it. The flag is set only when - // the report screen blurs/unmounts (see `flagReportNavigatedAway`), so it is true on a genuine return trip - // but false on the first open, on the repeated openReport calls of a single visit, and after a page refresh - // (the set is RAM-only). We consume it here to clear a manual unread marker on that return trip. + // True only on a genuine return trip: `flagReportNavigatedAway` sets it on blur/unmount, so it is false on the + // first open, on the repeated openReport calls of a single visit, and after a refresh (the set is RAM-only). const didNavigateBackToReport = reportsNavigatedAwayFrom.has(reportID); reportsNavigatedAwayFrom.delete(reportID); - // Whether this is the first load of the report this session. The report screen passes its RAM-only - // `hasOnceLoadedReportActions`, so a page refresh / cold start resets it to falsy — that's how we detect a - // refresh here. A manual unread marker can only be non-null on a first load if it was persisted from before - // the refresh, so clearing it here clears the marker on a page refresh while leaving genuine first opens - // (marker already null) untouched. + // A refresh resets the report screen's RAM-only `hasOnceLoadedReportActions`, which is how we detect one here. + // A genuine first open has no marker to clear, so this only affects a marker persisted from before the refresh. const isFirstLoadAfterRefresh = !hasOnceLoadedReportActions; const optimisticReport: Partial> = hasReportActions || !existingReportName ? {} : {reportName: existingReportName}; - // An explicit mark-as-unread keeps its "New" marker anchored while the user stays in the report - // (readNewestAction no longer clears it, and the repeated openReport calls of a single visit don't - // either), so the user sees the marker they created. It is reconciled away in two cases: when the user - // navigates away and comes back (`didNavigateBackToReport`), and on a page refresh (`isFirstLoadAfterRefresh`). - // This is a purely client-side decision, so it lives in optimisticData: it must apply immediately and - // offline, and must not be dropped if openReport never succeeds (the navigate-away flag is already consumed - // above). We deliberately do NOT restore it in failureData — resurrecting a marker the user has already - // moved past would be wrong. + // A manual mark-as-unread keeps its marker anchored while the user stays in the report, and is cleared only on + // a return trip or a refresh. This is a client-side decision, so it goes in optimisticData to apply immediately + // and offline. It is deliberately not restored in failureData — that would resurrect a marker already moved past. if (didNavigateBackToReport || isFirstLoadAfterRefresh) { optimisticReport.manuallyMarkedUnreadReportActionID = null; } @@ -3215,13 +3198,14 @@ function readNewestAction(reportID: string | undefined, isReportActionsLoaded: b const lastReadTime = getDBTimeWithSkew(); + // Deliberately leaves `manuallyMarkedUnreadReportActionID` alone so an auto-read doesn't wipe a marker the + // user created. `openReport` clears it on a return trip or a refresh. const optimisticData: Array> = [ { onyxMethod: Onyx.METHOD.MERGE, key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, value: { lastReadTime, - // Auto-reads keep a manual unread mark. The explicit "Mark as read" clears it here, openReport clears it on return or refresh. }, }, ]; @@ -3310,11 +3294,10 @@ function markCommentAsUnread(reportID: string | undefined, reportActions: OnyxEn }, ]; - // Do NOT reassert `manuallyMarkedUnreadReportActionID` in successData. `openReport` clears it optimistically - // when the user navigates away and comes back (or refreshes); if this request is still queued at that point - // (e.g. the mark happened offline), reasserting the id when it completes on reconnect would resurrect a - // marker the user has already moved past. The optimistic value set above persists on its own — the server - // response is a MERGE that never carries this client-only field — so there is nothing to reassert here. + // Deliberately omits `manuallyMarkedUnreadReportActionID`. If this request is still queued when `openReport` + // clears the marker (e.g. the mark happened offline), reasserting the id on reconnect would resurrect a marker + // the user has moved past. The optimistic value above persists on its own, since the server MERGE never + // carries this client-only field. const successData: Array> = [ { onyxMethod: Onyx.METHOD.MERGE, diff --git a/src/pages/inbox/ReportFetchHandler.tsx b/src/pages/inbox/ReportFetchHandler.tsx index e7395fe33168..4c87a2b06996 100644 --- a/src/pages/inbox/ReportFetchHandler.tsx +++ b/src/pages/inbox/ReportFetchHandler.tsx @@ -236,9 +236,8 @@ function ReportFetchHandler() { betas, personalDetails, hasReportActions, - // openReport clears a manual unread marker when this is falsy, which is how a page refresh / cold - // start is detected. This screen is the only place that opens the report the user is looking at, so - // it is the only caller that passes it. + // Falsy means a page refresh / cold start, which is when openReport clears a manual unread marker. + // This screen opens the report the user is looking at, so it is the only caller that passes it. hasOnceLoadedReportActions: reportLoadingState.hasOnceLoadedReportActions, currentUserAccountID, isSelfTourViewed, @@ -437,10 +436,9 @@ function ReportFetchHandler() { }; }, []); - // Record when the user navigates away from this report so the next openReport can clear a manual unread - // marker on the return trip (see `flagReportNavigatedAway`). We flag on blur (wide layout keeps the screen - // mounted) and on unmount / reportID change (narrow layout tears it down), covering both navigation shapes. - // Staying in the report never flags it, so the marker the user created is not wiped mid-session. + // Record navigating away so the next openReport can clear a manual unread marker on the return trip. We flag + // on blur (wide layout keeps the screen mounted) and on unmount / reportID change (narrow layout tears it + // down). Staying in the report never flags it, so the user's marker is not wiped mid-session. useEffect(() => { if (!prevIsFocused || isFocused) { return; diff --git a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts index ab9c7853c9df..614c85116b00 100644 --- a/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts +++ b/src/pages/inbox/report/shouldDisplayNewMarkerOnReportAction.ts @@ -58,15 +58,10 @@ const shouldDisplayNewMarkerOnReportAction = ({ hasWindowFocus = true, newMessageBoundaryTime, }: ShouldDisplayNewMarkerOnReportActionParams): boolean => { - // The user explicitly marked an action as unread. While a manual mark is active, the marked action is - // the *sole* anchor for the marker: show it only on the marked action and suppress it on every other - // action (newer self-messages, other users' messages, the earliest offline message), regardless of the - // timestamp-based checks below. Anchoring by the stored reportActionID is stable across the - // optimistic->confirmed transition, where unreadMarkerTime, lastReadTime, and created all converge on - // (or drift past) the confirmed `created` and isReportActionUnread would wrongly report the marked - // action as read. The marked action is the oldest unread by construction (markCommentAsUnread sets - // lastReadTime = its created - 1ms), so it stays the correct anchor even when newer messages arrive - // after the mark. `shouldHideNewMarker` is still honored so the marker isn't anchored on a pending-delete action. + // While a manual mark is active, the marked action is the sole anchor: every other action is suppressed. + // We anchor by reportActionID rather than timestamp because `created` shifts on the optimistic->confirmed + // transition and would wrongly read as already-read. The marked action is the oldest unread by construction + // (markCommentAsUnread sets lastReadTime = its created - 1ms), so it stays correct as newer messages arrive. if (manuallyMarkedUnreadReportActionID) { return message.reportActionID === manuallyMarkedUnreadReportActionID && !shouldHideNewMarker(message, isOffline); } @@ -105,23 +100,16 @@ const shouldDisplayNewMarkerOnReportAction = ({ const isPreviouslyOptimistic = (isPendingAdd(prevSortedVisibleReportActionsObjects[message.reportActionID]) && !isPendingAdd(message)) || (!!prevSortedVisibleReportActionsObjects[message.reportActionID]?.isOptimisticAction && !message.isOptimisticAction); - // This branch is only reached when no manual mark-as-unread is active (the check at the top of the - // function returns early while one is). Ignore unread for a self-authored message that is new or was - // just optimistic, preserving the #91940 behavior for cold opens. const prevMarkedReportAction = prevUnreadMarkerReportActionID ? prevSortedVisibleReportActionsObjects[prevUnreadMarkerReportActionID] : undefined; const isPreviouslyUnreadFromCurrentUser = currentUserAccountID === prevMarkedReportAction?.actorAccountID; - // Once a self-authored action holds the marker, keep it there rather than letting it hop to a different - // self-authored action (e.g. a persisted reimbursable toggle) - the regression from Expensify/App#91940. - // This only applies while that previous anchor is still present: if it was deleted, the marker must be - // allowed to relocate to the next unread message. + // Once a self-authored action holds the marker, don't let a different self-authored action steal it (the + // Expensify/App#91940 hop). Only while that anchor is still present — if it was deleted, the marker must relocate. const isDifferentUnread = isPrevUnreadMarkerReportActionPresent && isPreviouslyUnreadFromCurrentUser && prevMarkedReportAction?.reportActionID !== message.reportActionID; const shouldIgnoreUnreadForCurrentUserMessage = isNewMessage || isPreviouslyOptimistic || isDifferentUnread; if (isFromCurrentUser) { - // For a self-authored action, only move/keep the "New" marker when one already exists in this session - // (`prevUnreadMarkerReportActionID` is set). The explicit mark-as-unread case is handled earlier by the - // stable `manuallyMarkedUnreadReportActionID` check, which anchors the marker on first open/re-entry - // regardless of this guard. + // Only move/keep the marker on a self-authored action when one already exists in this session. + // An explicit mark-as-unread bypasses this guard via the early return at the top of the function. if (prevUnreadMarkerReportActionID) { return !shouldIgnoreUnreadForCurrentUserMessage; } @@ -200,20 +188,16 @@ const getUnreadMarkerReportAction = ({ return [null, -1]; } - // The stable manual-mark anchor is only valid while the marked action is still present and not pending - // deletion. Once it is deleted, keeping the anchor would leave every visible action failing the - // `reportActionID === manuallyMarkedUnreadReportActionID` check, so the marker would vanish instead of - // moving on. Drop the anchor in that case so the timestamp-based scan below can move the marker to the - // next unread message. + // Drop the manual anchor once the marked action is deleted, otherwise no action would match it and the + // marker would vanish instead of relocating via the timestamp scan below. const manuallyMarkedUnreadReportAction = manuallyMarkedUnreadReportActionID ? visibleReportActions.find((action) => action.reportActionID === manuallyMarkedUnreadReportActionID) : undefined; const activeManuallyMarkedUnreadReportActionID = manuallyMarkedUnreadReportAction && !shouldHideNewMarker(manuallyMarkedUnreadReportAction, isOffline) ? manuallyMarkedUnreadReportActionID : null; - // Whether the action the marker was previously anchored on is still present (not deleted/hidden). This - // distinguishes "the anchor was deleted, so let the marker relocate to the next unread message" from - // "the anchor is still around, so a different self-authored action must not steal the marker". + // Lets the caller tell "the anchor was deleted, so relocate the marker" apart from "the anchor is still + // around, so another self-authored action must not steal it". const isPrevUnreadMarkerReportActionPresent = prevUnreadMarkerReportActionID ? visibleReportActions.some((action) => action.reportActionID === prevUnreadMarkerReportActionID && !shouldHideNewMarker(action, isOffline)) : false; diff --git a/src/types/onyx/Report.ts b/src/types/onyx/Report.ts index 00eb15edc00f..639cdb4faa6a 100644 --- a/src/types/onyx/Report.ts +++ b/src/types/onyx/Report.ts @@ -132,8 +132,8 @@ type Report = OnyxCommon.OnyxValueWithOfflineFeedback< /** The time when user read the last message */ lastReadTime?: string; - /** reportActionID the user explicitly marked as unread. Stable across the optimistic→confirmed - * transition, unlike lastReadTime, so the "New" marker can anchor on a self-authored action. */ + /** reportActionID the user explicitly marked as unread. Unlike lastReadTime it is stable across the + * optimistic→confirmed transition, so the "New" marker can anchor on a self-authored action. */ manuallyMarkedUnreadReportActionID?: string | null; /** The sequence number of the last report visit */ diff --git a/tests/actions/ReportTest.ts b/tests/actions/ReportTest.ts index 4fa437bb9551..4a565194806f 100644 --- a/tests/actions/ReportTest.ts +++ b/tests/actions/ReportTest.ts @@ -5446,7 +5446,7 @@ describe('actions/Report', () => { }); describe('openReport with hasOnceLoadedReportActions', () => { - /** Puts a manual unread mark on the report, the way markCommentAsUnread does, so we can watch openReport clear it. */ + /** Puts a manual unread mark on the report, the way markCommentAsUnread does. */ async function givenAManualUnreadMark(reportID: string) { await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {reportID, manuallyMarkedUnreadReportActionID: 'marked-action-id'}); await waitForBatchedUpdates(); diff --git a/tests/unit/ReportActionsUtilsTest.ts b/tests/unit/ReportActionsUtilsTest.ts index c59d23e5f4c0..8a04bf1ca6db 100644 --- a/tests/unit/ReportActionsUtilsTest.ts +++ b/tests/unit/ReportActionsUtilsTest.ts @@ -6257,10 +6257,8 @@ describe('ReportActionsUtils', () => { }); it('returns false for a self-authored already-present action on a cold open when no marker exists and it was not explicitly marked unread (Expensify/App#91940 guard)', () => { - // A persisted self-authored action (e.g. a reimbursable toggle) whose timestamp reads as unread must - // NOT anchor the marker on a cold open/re-entry, where prevUnreadMarkerReportActionID is null. The - // explicit mark-as-unread case is handled separately via manuallyMarkedUnreadReportActionID (covered - // by the tests below), so this guard prevents the #91940 regression without affecting it. + // A persisted self-authored action (e.g. a reimbursable toggle) that reads as unread must not anchor + // the marker on a cold open. An explicit mark-as-unread is handled separately, by the tests below. const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'existing-action-id'}); const prevSortedVisibleReportActionsObjects = { [message.reportActionID]: makeAction({actorAccountID: currentUserAccountID, reportActionID: 'existing-action-id'}), @@ -6293,9 +6291,8 @@ describe('ReportActionsUtils', () => { }); it('does not move the marker from one self-authored action to a different self-authored action while the previous anchor is still present', () => { - // The previously marked action was a persisted self-authored message that is still present. A different - // self-authored action must not steal the "New" marker off it (the Expensify/App#91940 hop), so - // `isDifferentUnread` suppresses the marker here even though this action reads as unread. + // The previous anchor is still present, so `isDifferentUnread` stops this action stealing the marker + // off it (the Expensify/App#91940 hop) even though it reads as unread. const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-b'}); const prevMarkedAction = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-a'}); const prevSortedVisibleReportActionsObjects = { @@ -6315,8 +6312,7 @@ describe('ReportActionsUtils', () => { }); it('moves the marker to another self-authored action once the previous anchor has been deleted', () => { - // When the previous self-authored anchor is no longer present (deleted), the marker must be allowed to - // relocate to the next unread self-authored message, so `isDifferentUnread` does not suppress it. + // The previous anchor was deleted, so the marker must be free to relocate to the next unread message. const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-b'}); const prevMarkedAction = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-a'}); const prevSortedVisibleReportActionsObjects = { @@ -6336,8 +6332,7 @@ describe('ReportActionsUtils', () => { }); it('keeps the marker on the same self-authored action it was previously anchored on', () => { - // When the previously marked action is the action being evaluated, `isDifferentUnread` is false, so a - // persisted self-authored action that already holds the marker keeps it. + // The action being evaluated is the previous anchor, so `isDifferentUnread` is false and it keeps the marker. const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-a'}); const prevSortedVisibleReportActionsObjects = { [message.reportActionID]: makeAction({actorAccountID: currentUserAccountID, reportActionID: 'self-action-a'}), @@ -6397,9 +6392,8 @@ describe('ReportActionsUtils', () => { }); it('anchors the marker on the explicitly marked-unread action even after its confirmed created drifts before unreadMarkerTime', () => { - // Simulates the offline→online case: an optimistic self-message the user marked unread confirms - // with a `created` that lands before unreadMarkerTime, so the timestamp check reads it as "read". - // The stable manuallyMarkedUnreadReportActionID must still anchor the marker here. + // The offline→online case: the confirmed `created` lands before unreadMarkerTime, so the timestamp + // check reads the action as "read" and only the stable id can still anchor the marker. const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'marked-action-id', pendingAction: null, created: '2023-01-01 09:00:00.000'}); const prevSortedVisibleReportActionsObjects = { [message.reportActionID]: makeAction({ @@ -6420,8 +6414,8 @@ describe('ReportActionsUtils', () => { }); it('does not anchor the marker on a just-sent self-message when no action is marked unread', () => { - // Same confirmed self-message, but nothing is marked unread. The stable-id override is skipped and - // the existing just-sent suppression applies, keeping the #91443 fix intact. + // Same confirmed self-message, but with nothing marked unread the just-sent suppression still applies, + // keeping the #91443 fix intact. const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'confirmed-action-id', pendingAction: null}); const prevSortedVisibleReportActionsObjects = { [message.reportActionID]: makeAction({ @@ -6442,9 +6436,8 @@ describe('ReportActionsUtils', () => { }); it('keeps the marker on the explicitly marked-unread action even when a newer message is present', () => { - // The marked action is the oldest unread by construction (lastReadTime = its created - 1ms), so it - // stays the anchor regardless of an adjacent unread message — a newer message arriving after the mark - // must not steal the marker off the message the user deliberately marked unread. + // The marked action is the oldest unread by construction (lastReadTime = its created - 1ms), so a + // newer message arriving after the mark must not steal the marker off it. const message = makeAction({actorAccountID: currentUserAccountID, reportActionID: 'marked-action-id', created: '2023-01-01 11:00:00.000'}); const nextMessage = makeAction({created: '2023-01-01 11:30:00.000'}); expect( @@ -6459,8 +6452,7 @@ describe('ReportActionsUtils', () => { }); it('returns false for any action that is not the marked one while a manual mark is active (sole anchor)', () => { - // While a manual mark-as-unread is active the marked action is the sole anchor. Every other action - // must be suppressed, even an unread message from another user that would otherwise qualify. + // The marked action is the sole anchor, so even an unread message from another user is suppressed. const message = makeAction({actorAccountID: 99, reportActionID: 'other-action-id', created: '2023-01-01 11:00:00.000'}); expect( shouldDisplayNewMarkerOnReportAction({ @@ -6473,8 +6465,7 @@ describe('ReportActionsUtils', () => { }); it('returns false for the earliest-received-offline message while a different action is marked unread', () => { - // The manual mark takes precedence over the earliest-received-offline branch, so a non-matching - // action is suppressed even when it is the earliest message received while offline. + // The manual mark takes precedence over the earliest-received-offline branch. const message = makeAction({actorAccountID: 99, reportActionID: 'offline-action-id', created: '2023-01-01 11:00:00.000'}); expect( shouldDisplayNewMarkerOnReportAction({ diff --git a/tests/unit/useUnreadMarkerTest.ts b/tests/unit/useUnreadMarkerTest.ts index 9239cc36c29d..a052962217ea 100644 --- a/tests/unit/useUnreadMarkerTest.ts +++ b/tests/unit/useUnreadMarkerTest.ts @@ -29,10 +29,9 @@ jest.mock('@hooks/useIsAnonymousUser', () => ({ default: () => mockIsAnonymousUser, })); -// The hook subscribes to `${ONYXKEYS.COLLECTION.REPORT}${reportID}` twice, each with its own selector -// (one for `lastReadTime`, one for `manuallyMarkedUnreadReportActionID`). The mock applies the passed -// selector to a fake report so each call returns the value it actually reads. The implementation is set -// in beforeEach so it can use ONYXKEYS freely (a jest.mock factory cannot reference out-of-scope variables). +// The hook subscribes to `${ONYXKEYS.COLLECTION.REPORT}${reportID}` twice with different selectors, so the mock +// applies the passed selector to a fake report. The implementation is set in beforeEach so it can use ONYXKEYS +// freely (a jest.mock factory cannot reference out-of-scope variables). type FakeReport = Pick; type UseOnyxOptions = {selector?: (value: FakeReport | undefined) => unknown}; const mockUseOnyx = jest.fn<[unknown], [string, UseOnyxOptions?]>();