Skip to content

Fix: Add hasOnceLoadedReportActions guard to all readNewestAction call sites #81462

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
MelvinBot wants to merge 2 commits into
base: main
Choose a base branch
from
+55 −10
Open

Fix: Add hasOnceLoadedReportActions guard to all readNewestAction call sites #81462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db2f291
Fix: Add hasOnceLoadedReportActions guard to all readNewestAction cal…
MelvinBot Feb 5, 2026
70d2297
Fix: Add hasOnceLoadedReportActions parameter to test cases
MelvinBot Feb 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ function MoneyRequestReportActionsList({

const [session] = useOnyx(ONYXKEYS.SESSION, {canBeMissing: false});
const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${getNonEmptyStringOnyxID(reportID)}`, {canBeMissing: true});
const [reportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`, {canBeMissing: true});
const shouldShowHarvestCreatedAction = isHarvestCreatedExpenseReport(reportNameValuePairs?.origin, reportNameValuePairs?.originalID);
const [offlineModalVisible, setOfflineModalVisible] = useState(false);
const [isDownloadErrorModalVisible, setIsDownloadErrorModalVisible] = useState(false);
Expand Down Expand Up @@ -326,6 +327,12 @@ function MoneyRequestReportActionsList({
if (!isFocused) {
return;
}

// Do not try to mark the report as read if the report has not been loaded and shared with the user
if (!reportMetadata?.hasOnceLoadedReportActions) {
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.
Expand All @@ -341,7 +348,7 @@ function MoneyRequestReportActionsList({
}
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [report.lastVisibleActionCreated, transactionThreadReport?.lastVisibleActionCreated, report.reportID, isVisible]);
}, [report.lastVisibleActionCreated, transactionThreadReport?.lastVisibleActionCreated, report.reportID, isVisible, reportMetadata?.hasOnceLoadedReportActions]);

useEffect(() => {
if (!isVisible || !isFocused) {
Expand All @@ -351,6 +358,11 @@ function MoneyRequestReportActionsList({
return;
}

// Do not try to mark the report as read if the report has not been loaded and shared with the user
if (!reportMetadata?.hasOnceLoadedReportActions) {
return;
}

// In case the user read new messages (after being inactive) with other device we should
// show marker based on report.lastReadTime
const newMessageTimeReference = lastMessageTime.current && report.lastReadTime && lastMessageTime.current > report.lastReadTime ? userActiveSince.current : report.lastReadTime;
Expand All @@ -373,7 +385,7 @@ function MoneyRequestReportActionsList({
// We will mark the report as read in the above case which marks the LHN report item as read while showing the new message
// marker for the chat messages received while the user wasn't focused on the report or on another browser tab for web.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isFocused, isVisible]);
}, [isFocused, isVisible, reportMetadata?.hasOnceLoadedReportActions]);

/**
* The index of the earliest message that was received while offline
Expand Down Expand Up @@ -443,6 +455,7 @@ function MoneyRequestReportActionsList({
// We additionally track the top offset to be able to scroll to the new transaction when it's added
scrollingVerticalTopOffset.current = contentOffset.y;
},
hasOnceLoadedReportActions: !!reportMetadata?.hasOnceLoadedReportActions,
});

useEffect(() => {
Expand Down Expand Up @@ -629,8 +642,11 @@ function MoneyRequestReportActionsList({

reportScrollManager.scrollToEnd();
readActionSkipped.current = false;
readNewestAction(report.reportID);
}, [setIsFloatingMessageCounterVisible, hasNewestReportAction, reportScrollManager, report.reportID]);
// Do not try to mark the report as read if the report has not been loaded and shared with the user
if (reportMetadata?.hasOnceLoadedReportActions) {
readNewestAction(report.reportID);
}
}, [setIsFloatingMessageCounterVisible, hasNewestReportAction, reportScrollManager, report.reportID, reportMetadata?.hasOnceLoadedReportActions]);

const scrollToNewTransaction = useCallback(
(pageY: number) => {
Expand Down
6 changes: 5 additions & 1 deletion src/pages/inbox/ReportScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -940,9 +940,13 @@ function ReportScreen({route, navigation, isInSidePanel = false}: ReportScreenPr
if (!!report?.lastReadTime || !isTaskReport(report)) {
return;
}
// Do not try to mark the report as read if the report has not been loaded and shared with the user
if (!reportMetadata?.hasOnceLoadedReportActions) {
return;
}
// After creating the task report then navigating to task detail we don't have any report actions and the last read time is empty so We need to update the initial last read time when opening the task report detail.
readNewestAction(report?.reportID);
}, [report]);
}, [report, reportMetadata?.hasOnceLoadedReportActions]);

// Reset the ref when navigating to a different report
useEffect(() => {
Expand Down
15 changes: 12 additions & 3 deletions src/pages/inbox/report/ReportActionsList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,7 @@ function ReportActionsList({
setShouldScrollToEndAfterLayout(false);
}
},
hasOnceLoadedReportActions: !!reportMetadata?.hasOnceLoadedReportActions,
});

useEffect(() => {
Expand Down Expand Up @@ -430,6 +431,11 @@ function ReportActionsList({
return;
}

// Do not try to mark the report as read if the report has not been loaded and shared with the user
if (!reportMetadata?.hasOnceLoadedReportActions) {
return;
}

if (!isVisible || !isFocused) {
if (!lastMessageTime.current) {
lastMessageTime.current = lastAction?.created ?? '';
Expand Down Expand Up @@ -464,7 +470,7 @@ function ReportActionsList({
// We will mark the report as read in the above case which marks the LHN report item as read while showing the new message
// marker for the chat messages received while the user wasn't focused on the report or on another browser tab for web.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isFocused, isVisible]);
}, [isFocused, isVisible, reportMetadata?.hasOnceLoadedReportActions]);

const prevHandleReportChangeMarkAsRead = useRef<() => void>(null);
const prevHandleAppVisibilityMarkAsRead = useRef<() => void>(null);
Expand Down Expand Up @@ -631,8 +637,11 @@ function ReportActionsList({
}
reportScrollManager.scrollToBottom();
readActionSkipped.current = false;
readNewestAction(report.reportID);
}, [setIsFloatingMessageCounterVisible, hasNewestReportAction, reportScrollManager, report.reportID, backTo]);
// Do not try to mark the report as read if the report has not been loaded and shared with the user
if (reportMetadata?.hasOnceLoadedReportActions) {
readNewestAction(report.reportID);
}
}, [setIsFloatingMessageCounterVisible, hasNewestReportAction, reportScrollManager, report.reportID, backTo, reportMetadata?.hasOnceLoadedReportActions]);

/**
* Calculates the ideal number of report actions to render in the first render, based on the screen height and on
Expand Down
14 changes: 12 additions & 2 deletions src/pages/inbox/report/useReportUnreadMessageScrollTracking.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ type Args = {

/** Callback to call on every scroll event */
onTrackScrolling: (event: NativeSyntheticEvent<NativeScrollEvent>) => void;

/** Whether the report actions have been loaded at least once */
hasOnceLoadedReportActions: boolean;
};

export default function useReportUnreadMessageScrollTracking({
Expand All @@ -32,14 +35,16 @@ export default function useReportUnreadMessageScrollTracking({
onTrackScrolling,
unreadMarkerReportActionIndex,
isInverted,
hasOnceLoadedReportActions,
}: Args) {
const [isFloatingMessageCounterVisible, setIsFloatingMessageCounterVisible] = useState(false);
const isFocused = useIsFocused();
const ref = useRef<{previousViewableItems: ViewToken[]; reportID: string; unreadMarkerReportActionIndex: number; isFocused: boolean}>({
const ref = useRef<{previousViewableItems: ViewToken[]; reportID: string; unreadMarkerReportActionIndex: number; isFocused: boolean; hasOnceLoadedReportActions: boolean}>({
reportID,
unreadMarkerReportActionIndex,
previousViewableItems: [],
isFocused: true,
hasOnceLoadedReportActions: false,
});
Comment on lines +42 to 48
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Feb 5, 2026

Choose a reason for hiding this comment

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

P2 Badge Initialize ref with current hasOnceLoadedReportActions

When hasOnceLoadedReportActions is already true on first render, the ref is still initialized to false, so the initial onViewableItemsChanged call can skip readNewestAction even though the report is fully loaded. If the viewable items don’t change afterward (e.g., user opens the report and doesn’t scroll), the skipped read may never be retried, leaving the report unread until another scroll occurs. Initializing the ref with the current prop value avoids this race on first render.

Useful? React with 👍 / 👎.

// We want to save the updated value on ref to use it in onViewableItemsChanged
// because FlatList requires the callback to be stable and we cannot add a dependency on the useCallback.
Expand All @@ -52,6 +57,10 @@ export default function useReportUnreadMessageScrollTracking({
ref.current.isFocused = isFocused;
Copy link
Contributor

@github-actions github-actions bot Feb 5, 2026

Choose a reason for hiding this comment

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

❌ PERF-6 (docs)

The hasOnceLoadedReportActions value is being synced from a prop to a ref using useEffect. This creates unnecessary complexity and potential stale value issues.

Suggested fix: Eliminate the useEffect and the ref field. Instead, pass hasOnceLoadedReportActions as a parameter directly in the onViewableItemsChanged callback:

// In the onViewableItemsChanged callback:
if (unreadActionVisible && readActionSkippedRef.current && hasOnceLoadedReportActions) {
    readActionSkippedRef.current = false;
    readNewestAction(ref.current.reportID);
}

Since onViewableItemsChanged is already a stable callback (as indicated by the eslint-disable comment), and the callback is manually triggered in the useEffect when dependencies change, the latest hasOnceLoadedReportActions value will always be available when the callback executes.

Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

Copy link
Contributor Author

@MelvinBot MelvinBot Feb 5, 2026

Choose a reason for hiding this comment

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

Thanks for the review! However, I believe the current implementation is correct and follows the established pattern in this file.

The onViewableItemsChanged callback is intentionally memoized with an empty dependency array (line 132: }, []) to keep it stable for FlatList, as required for optimal performance. This is explicitly documented in the comments at lines 49-50 and 129-131:

// We want to save the updated value on ref to use it in onViewableItemsChanged
// because FlatList requires the callback to be stable and we cannot add a dependency on the useCallback.

Because the callback never re-creates, using hasOnceLoadedReportActions directly as a closure variable would capture the initial value and never update when the prop changes. The ref pattern is the correct approach to access current values from within a stable callback.

This same pattern is already used in this file for:

  • reportID (lines 51-54)
  • isFocused (lines 56-58)
  • unreadMarkerReportActionIndex (line 137)

The new useEffect for hasOnceLoadedReportActions (lines 60-62) follows exactly the same pattern, ensuring the callback always has access to the latest value via ref.current.hasOnceLoadedReportActions at line 123.

}, [isFocused]);

useEffect(() => {
ref.current.hasOnceLoadedReportActions = hasOnceLoadedReportActions;
}, [hasOnceLoadedReportActions]);

/**
* On every scroll event we want to:
* Show/hide the latest message pill when user is scrolling back/forth in the history of messages.
Expand Down Expand Up @@ -110,7 +119,8 @@ export default function useReportUnreadMessageScrollTracking({
}

// if we're scrolled closer than the offset and read action has been skipped then mark message as read
if (unreadActionVisible && readActionSkippedRef.current) {
// Do not try to mark the report as read if the report has not been loaded and shared with the user
if (unreadActionVisible && readActionSkippedRef.current && ref.current.hasOnceLoadedReportActions) {
// eslint-disable-next-line no-param-reassign
readActionSkippedRef.current = false;
readNewestAction(ref.current.reportID);
Expand Down
6 changes: 6 additions & 0 deletions tests/unit/useReportUnreadMessageScrollTrackingTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ describe('useReportUnreadMessageScrollTracking', () => {
unreadMarkerReportActionIndex: -1,
isInverted: true,
onTrackScrolling: onTrackScrollingMockFn,
hasOnceLoadedReportActions: true,
}),
);

Expand Down Expand Up @@ -69,6 +70,7 @@ describe('useReportUnreadMessageScrollTracking', () => {
isInverted: true,
unreadMarkerReportActionIndex: -1,
onTrackScrolling: onTrackScrollingMockFn,
hasOnceLoadedReportActions: true,
}),
);

Expand All @@ -95,6 +97,7 @@ describe('useReportUnreadMessageScrollTracking', () => {
isInverted: true,
unreadMarkerReportActionIndex: 1,
onTrackScrolling: onTrackScrollingMockFn,
hasOnceLoadedReportActions: true,
}),
);

Expand Down Expand Up @@ -126,6 +129,7 @@ describe('useReportUnreadMessageScrollTracking', () => {
unreadMarkerReportActionIndex: -1,
isInverted: true,
onTrackScrolling: onTrackScrollingMockFn,
hasOnceLoadedReportActions: true,
}),
);

Expand All @@ -151,6 +155,7 @@ describe('useReportUnreadMessageScrollTracking', () => {
unreadMarkerReportActionIndex: 1,
isInverted: true,
onTrackScrolling: onTrackScrollingMockFn,
hasOnceLoadedReportActions: true,
}),
);

Expand Down Expand Up @@ -180,6 +185,7 @@ describe('useReportUnreadMessageScrollTracking', () => {
unreadMarkerReportActionIndex: 1,
isInverted: true,
onTrackScrolling: onTrackScrollingMockFn,
hasOnceLoadedReportActions: true,
}),
);

Expand Down
Loading