Fix: Add hasOnceLoadedReportActions guard to all readNewestAction call sites#81462
Fix: Add hasOnceLoadedReportActions guard to all readNewestAction call sites#81462MonilBhavsar merged 10 commits intomainfrom
Conversation
…l sites The previous fix (PR #74925) only added the hasOnceLoadedReportActions check to one location in ReportActionsList.tsx. This left 7+ other code paths that could call readNewestAction before the report was shared, causing 401 Unauthorized errors from UpdateRNVPLastReadTime. This commit adds the same guard pattern to all remaining call sites: - ReportActionsList.tsx: handleAppVisibilityMarkAsRead, scrollToBottomAndMarkReportAsRead - MoneyRequestReportActionsList.tsx: all 3 effects/callbacks that call readNewestAction - useReportUnreadMessageScrollTracking.ts: onViewableItemsChanged callback - ReportScreen.tsx: task report useEffect The fix ensures the hooks re-run when hasOnceLoadedReportActions changes from false to true, so reports are correctly marked as read once loaded. Co-authored-by: Monil Bhavsar <MonilBhavsar@users.noreply.github.com>
|
@Krishna2323 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team
|
🤖 Check Failure Analysis The failing Failing Check: Unchecked Items (11 total):
Analysis: Recommended Action:
This is not something I can fix automatically since it requires manual testing verification. Please update the PR description checklist to pass the CI check. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -52,6 +57,10 @@ export default function useReportUnreadMessageScrollTracking({ | |||
| ref.current.isFocused = isFocused; | |||
There was a problem hiding this comment.
❌ 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.
There was a problem hiding this comment.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db2f291ab7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const ref = useRef<{previousViewableItems: ViewToken[]; reportID: string; unreadMarkerReportActionIndex: number; isFocused: boolean; hasOnceLoadedReportActions: boolean}>({ | ||
| reportID, | ||
| unreadMarkerReportActionIndex, | ||
| previousViewableItems: [], | ||
| isFocused: true, | ||
| hasOnceLoadedReportActions: false, | ||
| }); |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
The comment is technically correct, @MelvinBot could you please check?
There was a problem hiding this comment.
The review comment identifies a real inconsistency in the ref initialization pattern, but it is not an actual bug in practice. Here's why:
The concern
The ref is initialized with hasOnceLoadedReportActions: false (hardcoded) rather than using the prop value. If hasOnceLoadedReportActions is already true on first render, the ref would be stale until the useEffect runs.
Why it doesn't cause a bug
The only place ref.current.hasOnceLoadedReportActions is read is in onViewableItemsChanged at line 123:
if (unreadActionVisible && readActionSkippedRef.current && ref.current.hasOnceLoadedReportActions) {For the stale false to cause a missed readNewestAction call, both conditions would need to be true simultaneously:
readActionSkippedRef.current === trueref.current.hasOnceLoadedReportActions === false(stale)
But readActionSkippedRef.current can only be set to true inside handleReportChangeMarkAsRead / handleAppVisibilityMarkAsRead in ReportActionsList.tsx and MoneyRequestReportActionsList.tsx — and all of those code paths are already guarded by:
if (!reportMetadata?.hasOnceLoadedReportActions) {
return;
}So by the time readActionSkippedRef.current is ever true, hasOnceLoadedReportActions was already true in the parent component, and the sync useEffect will have already updated the ref (since the parent's useEffect that sets readActionSkipped = true runs after the hook's useEffect that syncs the ref).
Additionally, onViewableItemsChanged is fired by FlatList asynchronously after layout — not during the synchronous render phase — so effects will always have executed before it fires.
Recommendation
That said, initializing the ref with the prop value (hasOnceLoadedReportActions instead of false) is a harmless defensive improvement that would make the code more consistent — reportID and unreadMarkerReportActionIndex are already initialized from their prop values, while isFocused and hasOnceLoadedReportActions use hardcoded defaults. Making this change would improve code clarity even though it doesn't fix an actual bug.
The hook was updated to require hasOnceLoadedReportActions but the test file wasn't updated to include this new required parameter in all test cases, causing typecheck failures.
|
Fixed the failing typecheck! 🔧 Issue: The Fix: Added The CI should pass now. |
|
🤖 Check Failure Analysis The failing Unchecked items requiring manual verification:
Resolution: The PR author needs to complete their testing on each platform and check off the appropriate boxes in the PR description. These are manual verification steps that cannot be automatically fixed. Note: All other checks (ESLint, TypeScript, tests, etc.) are either passing or still pending. The only failure is the author checklist. |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@MonilBhavsar could you please check this and let me know whether I should review it or not? Thanks! |
|
PR doesn’t need product input as some kind of BugBot PR. Unassigning and unsubscribing myself. |
Yes, this needs review |
|
Reviewing... |
|
Fixed the failing |
|
The failing check is This is not a code issue. The check requires a reviewer to post the Reviewer Checklist template as a comment on the PR. Since no reviewer has posted it yet, the check fails. All code-related checks are passing: ESLint, typecheck, tests (all 8 jobs), builds, Prettier, spellcheck, perf-tests, and verifySignedCommits. To resolve: A reviewer needs to copy the Reviewer Checklist template and post it as a comment on this PR. |
|
Will complete the checklist once this is resolved. |
|
The failing Analysis: The error is To resolve: A reviewer needs to copy and post the Reviewer Checklist as a comment on this PR. This is not something that can be fixed by pushing a code change. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariweb_chrome_2.mp4web_chrome.mp4 |
|
I'll take a look at this comment today #81462 (comment) cc @Krishna2323 |
@MelvinBot By this, do you mean that no unread marker should be shown when I open the report? |
|
Good question — there are actually two separate unread indicators, and they behave differently: 1. LHN bold text / unread dot (sidebar)This is driven by
2. In-chat "New" divider (the line between messages)This is driven by a local React state ( What the test step meansThe test step "Verify unread messages are correctly marked as read when the report loads" means:
The key thing to verify is that the LHN correctly transitions from unread → read once the report loads, and that |
…second parameter Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
Updated Changes:
|
src/libs/actions/Report/index.ts
Outdated
| * Currently, the unread indicator needs to be reset only when users mark a report as read. | ||
| */ | ||
| function readNewestAction(reportID: string | undefined, shouldResetUnreadMarker = false) { | ||
| function readNewestAction(reportID: string | undefined, hasOnceLoadedReportActions = true, shouldResetUnreadMarker = false) { |
There was a problem hiding this comment.
@MelvinBot The whole point of moving it to the second position was to make it required, but it still optional.
There was a problem hiding this comment.
Good catch! Removed the default value = true so hasOnceLoadedReportActions is now a required boolean parameter. All call sites already pass it explicitly, so no other changes were needed.
Fixed in commit 1cccc11.
|
@MelvinBot One more thing to fix:
|
Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
Updated the two test files for the
|
|
The failing The latest commit ( |
Krishna2323
left a comment
Krishna2323
left a comment
There was a problem hiding this comment.
Recent changes looks good to me and works well.
Monosnap.screencast.2026-02-20.17-37-18.mp4
|
@Krishna2323 the marker didn't appear this time at 1:13, as it did previously. |
|
@MonilBhavsar In this video, the new message wasn’t in the visible area, which is why the marker appeared. Should the marker still be shown even if the new message is already visible? cc: @MelvinBot |
|
Based on the code, the existing behavior is designed to not show the marker when the new message is already visible. There are three layers that prevent this:
So the intended behavior is: if the user is scrolled to the bottom (within 250px), new messages should NOT trigger the marker. The marker should only appear when the user is scrolled away from the bottom, indicating there are unread messages below the visible area. If the video shows the marker appearing when the message is already visible, that would be a bug — likely a timing issue where the stored |
No, only visible when new message is out of the view |
|
@Krishna2323 @MonilBhavsar how is this looking? |
|
@MelvinBot Please resolve the conflicts. I think we’re good to merge this then, @MonilBhavsar? |
…ction401Error Co-authored-by: Krishna <Krishna2323@users.noreply.github.com> # Conflicts: # src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx
|
Resolved the merge conflict and pushed the merge commit. Conflict: All other files ( |
|
@Krishna2323 Did you test fully? there has been bunch of changes |
|
@mountiny Yes, I tested it again here: #81462 (review) . The recent changes only made |
|
@MonilBhavsar was this comment addressed? #81462 (comment) if yes, feel free to merge |
|
Looks good, merging |
|
🚧 @MonilBhavsar has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.3.27-0 🚀
|
|
This PR got deployed to production, no comment b/c there was a connection issue during deploy (link) |
7 participants
Explanation of Change
This PR fixes recurring 401 Unauthorized errors from
UpdateRNVPLastReadTimeby adding thehasOnceLoadedReportActionsguard to all code paths that callreadNewestAction.Background: PR #74925 previously fixed this issue by adding a check in
ReportActionsList.tsxto prevent callingreadNewestActionbefore the report was loaded/shared with the user. However, the error returned because there were 7+ other code paths that could still callreadNewestActionwithout this guard, causing the race condition withOpenReport.Changes made:
handleAppVisibilityMarkAsReadandscrollToBottomAndMarkReportAsReadreportMetadatahook and guards to all 3 locations that callreadNewestActionhasOnceLoadedReportActionsparameter and check inonViewableItemsChangedThe fix ensures:
readNewestActionis skipped when report actions haven't loaded yethasOnceLoadedReportActionsbecomestrue(ensuring reports are marked as read once properly loaded)Fixed Issues
$ https://github.com/Expensify/Expensify/issues/557858
PROPOSAL:
Tests
ReadNewestActionOffline tests
N/A - This fix is about race conditions between API calls, not offline behavior.
QA Steps
Same as Tests above.
Monitor the BugBot issue at https://github.com/Expensify/Expensify/issues/557858 to verify the error rate drops after deployment.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A - No UI changes
Android: mWeb Chrome
N/A - No UI changes
iOS: Native
N/A - No UI changes
iOS: mWeb Safari
N/A - No UI changes
MacOS: Chrome / Safari
N/A - No UI changes