Repository navigation
refactor: extract useMoneyRequestReportVisibleActions and adapt useMarkAsRead/useUnreadMarker - #100025
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
…compose/money-request-report-transactions-a2
…compose/money-request-report-transactions-a2 # Conflicts: # src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73fad601de
ℹ️ 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".
| hasOnceLoadedReportActions: !!reportLoadingState?.hasOnceLoadedReportActions, | ||
| }); | ||
|
|
||
| const {markNewestActionAsRead, completeSkippedMarkAsRead} = useMarkAsRead({ |
There was a problem hiding this comment.
Gate the initial read on the visible list position
When an unread money-request report opens around a linked historical action, the list can have hasNewerActions set or be positioned away from the end. This newly adopted hook nevertheless runs its initial effect and calls readNewestAction without checking either hasNewerActions or isScrolledToEnd, advancing lastReadTime past newer messages the user has not seen; the removed inline implementation conditioned the read on being at the bottom. Gate the initial read for this surface on the supplied pagination and scroll state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Looks like a valid concern here as step 5 fails as demonstrated in test video below.
Precondition:
• Account A (Submitter / Viewer)
• Account B (Approver / Co-worker)
• An expense report with enough items (e.g. 5+ expenses/transactions) so that the comments section at the bottom is pushed off-screen below the viewport.
Test Scenario:
1 Log in to Account A and Account B in separate browser windows or devices.
2 In Account A, navigate to the expense report for which we will post a new comment from Account B.
3 In Account B, open the expense report and post a new comment (e.g. "Testing unread marker").
4 In Account A, look at the Left Hand Navigation (LHN)
5 Verify the expense report item is bold and remains unread. (FAILS)
6 Scroll down to the bottom of the list (or tap the "Latest messages" button)
7 Verify that the green "New" unread marker line is visible above the new comment from Account B. Once at the bottom, the unread marker clears and the report is marked as read.
100025-web-chrome-issue-001.mp4
There was a problem hiding this comment.
Fixed. Now working same as staging.
Nagranie.z.ekranu.2026-09-10.o.11.30.58.mov
ReviewThe decomposition itself looks good — the hook boundaries are sensible and 1. Read/unread semantics change for the money-request viewThe deleted inline effects and
The first row is the one I'd verify by hand: opening an unread expense report while parked at the top of a long list, or in a background tab, now consumes the unread state where it previously deferred. That is exactly what PR test steps 2 and 3 exercise, so it is worth an explicit pass. 2.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19f21ed12f
ℹ️ 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".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| hasOnceLoadedReportActions: !!reportLoadingState?.hasOnceLoadedReportActions, | ||
| }); | ||
|
|
||
| const {markNewestActionAsRead, completeSkippedMarkAsRead} = useMarkAsRead({ |
There was a problem hiding this comment.
Fixed. Now working same as staging.
Nagranie.z.ekranu.2026-09-10.o.11.30.58.mov
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@sosek108 Thanks for the updated PR. The issue with the web version is fixed but native versions still do not show the expense report item as bold in LHN. Here are the videos for this. 100025-ios-hybrid-issue-001.mp4100025-android-hybrid-issue-001.mp4 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp100025-android-hybrid-001.mp4Android: mWeb Chrome100025-mweb-android-001.mp4iOS: HybridApp100025-ios-hybrid-001.mp4iOS: mWeb Safari100025-mweb-safari-001.mp4MacOS: Chrome / Safari100025-web-chrome-001.mp4 |
Isn't it that because you're logged in on the same account on both devices and in browser? Nagranie.z.ekranu.2026-09-11.o.11.56.10.mov |
It does not seem so. I just tried again with the user logged in only on ios native version but the LHN did not turn bold on receiving a new message. 100025-web-chrome-issue-002.mp4 |
So can you confirm that you have the same account on both Mobile and Web? If so, this behaves the same way as on staging / prod. I can guess only by Avatar - same letter, same colour. |
Oh! Sorry about this. That was silly of me. I didn't notice that the accounts were same. And thanks for pointing it out. |
mountiny
left a comment
There was a problem hiding this comment.
Just one thing to please keep in mind in other prs and you can tag the comments in another pr
There was a problem hiding this comment.
Can you make sure al the tests follow the Given, When, Then comments structures
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@MelvinBot please create payment issue for @rojiphil |
|
Payment issue created: #101204
One thing to fix: this PR merged today and hasn't reached staging or production yet (OSBotify said it deploys to staging after the next production release). The view run · no recording available |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.80-0 🚀
|
|
🤖 No help site changes are required for this PR. I did not create a docs PR. Why: This is a pure internal refactor. It moves logic between files and adds tests — it changes no user-facing behavior, copy, or UI. What I checkedFiles changed (9):
No documentation triggers found:
Help site articles under If you expected a user-visible change from one of the other PRs in this 10-PR stack, point me at that PR number and I'll scope the docs work there instead. @sosek108, no help site PR was created because no documentation changes are required — so there is nothing to review or mark |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.80-0 🚀
Bundle Size Analysis (Sentry): |
|
@jponikarchuk Step 6 is only for dev. I've updated QA steps so there is no more commotion |

Explanation of Change
This is PR 3 of a 10-PR stack decomposing the MoneyRequestReport list components (
MoneyRequestReportActionsList/MoneyRequestReportTransactionList) into focused hooks and presentational sub-components.This slice extracts the visible-actions derivation into a dedicated hook and replaces the monolith's inline unread-marker / mark-as-read logic with the hooks already shared with the chat list:
Performance test

We can see reduced number of renders for unrelated report's actions.
Fixed Issues
$ #99919
PROPOSAL:
Tests
npx jest tests/unit/useUnreadMarkerTest.ts tests/unit/useMarkAsReadTest.ts tests/ui/MoneyRequestReportActionsListRejectModalTest.tsxand verify they passOffline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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
Android: mWeb Chrome
iOS: Native
Nagranie.z.ekranu.2026-09-9.o.10.15.03.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Nagranie.z.ekranu.2026-09-9.o.10.07.35.mov