Fix: Add hasOnceLoadedReportActions guard to all readNewestAction call sites#81462
Fix: Add hasOnceLoadedReportActions guard to all readNewestAction call sites#81462
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 👍 / 👎.
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.
|
2 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