Repository navigation
Pass hasReportActions to openReport (p4) - #96864
luacmartins merged 2 commits into
Conversation
|
@eVoloshchak 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] |
![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: 6f88da6584
ℹ️ 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".
| selector: (reportActions) => { | ||
| return policyReports?.reduce( | ||
| (acc, curr) => { | ||
| acc[curr.reportID] = !!reportActions?.[curr.reportID]; |
There was a problem hiding this comment.
For admins opening Workspace > Rooms, useOnyx(ONYXKEYS.COLLECTION.REPORT_ACTIONS) returns a collection keyed by the full Onyx key, e.g. ${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID} as used by reportActionsExist, but this selector indexes it with the bare room ID. That makes every cached room evaluate to false, so the new openReport(..., hasReportActions: ...) call behaves as if rooms with existing actions have none and reintroduces the incorrect optimistic report update this parameter is meant to avoid.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The selector returns it with the reportID as the index. Invalid review.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }, | ||
| [policyID, reportNameValuePairs], | ||
| ); | ||
| const [hasReportActions] = useOnyx(ONYXKEYS.COLLECTION.REPORT_ACTIONS, { |
There was a problem hiding this comment.
❌ PERF-11 (docs)
This useOnyx subscribes to the entire REPORT_ACTIONS collection (one of the largest collections in the app) and its selector returns a Record<string, boolean> mapped across every policy report. Because a selector is used, Onyx runs deepEqual on this whole map on every report-action change anywhere in the app, and the selector also closes over the large external policyReports dataset and re-iterates it on each of those unrelated updates — compounding the cost. A mapped/transformed collection output like this is exactly the case PERF-11 warns against: the deepEqual is expensive and there is no meaningful re-render savings.
Instead, subscribe without a selector and build the lookup inline (or read presence per-report where it's needed), so Onyx uses the cheap shallowEqual on raw references:
const [reportActions] = useOnyx(ONYXKEYS.COLLECTION.REPORT_ACTIONS);
// where the room action is built:
const hasReportActions = !!reportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`];
openReport({reportID: report.reportID, introSelected, betas, shouldMarkAsRead: false, hasReportActions});This avoids the per-report mapped object and the deepEqual over the full collection.
Reviewed at: 6f88da6 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
I don't think the deepEqual will be expensive here because it's a simple boolean map.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
eVoloshchak
left a comment
eVoloshchak
left a comment
There was a problem hiding this comment.
B.Prerequisite: have an expense
- Open an expense > Amount
- Copy the link
- Open it on an account without access to the report
- Verify there is no error (it'll show a not found page)
BUG
When you open a report you don't have access to (or a non-existing one) for the first time, not found page flashes multiple times. Verified this isn't present on staging
Screen.Recording.2026-07-26.at.18.15.45.mov
Screen.Recording.2026-07-26.at.18.11.11.mov
The same is true for cases C and D (a task and a message from another user)
Screen.Recording.2026-07-26.at.18.17.48.mov
Screen.Recording.2026-07-26.at.18.16.55.mov
|
Hmm, I can't repro it. Can you try again? I just merged with main w.mp4It's unlikely this PR causes that issue because |
|
This is still reproducible, but now it's also reproducible on staging, so not related to this PR, probably related to some recent changes |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppscreen-20260728-182531-1785255872819.mp4Android: mWeb ChromeScreen.Recording.2026-07-28.at.18.58.27.moviOS: HybridAppScreen.Recording.2026-07-28.at.17.46.36.moviOS: mWeb SafariScreen.Recording.2026-07-28.at.18.56.53.movMacOS: Chrome / SafariScreen.Recording.2026-07-28.at.18.59.06.mov |
|
🚧 luacmartins 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/luacmartins in version: 9.4.46-0 🚀
|
|
I reviewed the changes in this PR and no help site changes are required under This PR is a purely internal code refactor. It passes a new The change is invisible to end users:
Since there's nothing user-facing to document, I did not create a draft docs PR. @bernhardoj, since this change isn't user-facing, no help site PR was created. If you believe any of these changes do surface a documentable behavior difference, let me know and I'll draft the docs PR. |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.46-10 🚀
Bundle Size Analysis (Sentry): |
5 participants
Explanation of Change
Fixed Issues
$ #73651
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
A.
Prerequisite: have a workspace as admin and workspace rooms beta
B.
Prerequisite: have an expense
C.
Prerequisite: have a task
D.
Prerequisite: have a message sent from another user
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
iOS: mWeb Safari
MacOS: Chrome / Safari
2.mp4
1.mp4
3.mp4
4.mp4