[NoQA] stabilize stableReportSelector and more reports in the ReportActionItem tree#93421
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e0d598d2f
ℹ️ 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 shouldRenderViewBasedOnAction = useTableReportViewActionRenderConditionals(action); | ||
|
|
||
| const [chatReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${getNonEmptyStringOnyxID(report?.chatReportID)}`); | ||
| const [chatReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${getNonEmptyStringOnyxID(report?.chatReportID)}`, {selector: getStableReportSelector}); |
There was a problem hiding this comment.
Use per-row selector identity for report subscriptions
When a report has many visible actions, every ReportActionItem subscribes to the same chat report key with the module-level selector. Onyx's snapshot cache keys by Onyx key plus selector function identity (the same reason ReportActionsList now wraps its selector), so all rows share one cache entry and can continue replacing each other's selected report object when chat heartbeat fields change. That leaves the row tree re-rendering on the high-volume updates this patch is trying to filter; use a per-instance stable selector here and for the other new row-level report subscriptions.
Useful? React with 👍 / 👎.
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.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@QichenZhu 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a92b53e8d
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7927499f2
ℹ️ 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 stableReportSelector = (reportEntry: OnyxEntry<OnyxTypes.Report>) => getStableReportSelector(reportEntry); | ||
| const [originalReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${originalReportID}`, {selector: stableReportSelector}); |
There was a problem hiding this comment.
Memoize original report selector
When an action has an originalReportID (thread parent or merged single-transaction views), this creates a fresh selector function on every render, so Onyx sees a new selector identity/cache key each time and cannot reuse the stable projected report reference. That reintroduces the reference churn this change is trying to remove for any hover/edit/state update in these rows; define the selector with useCallback(..., []) as in ReportActionsList before passing it to useOnyx.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
react compiler handles that
There was a problem hiding this comment.
I reverted stableReportSelector here. I don't see benefit in Profiler benchmarks. originalReport looks to be stable
|
If this needs my review/testing, I'll get to it this weekend. |
|
@QichenZhu I kept the changes minimal based on the Profiler results, which pointed to specific props causing re-renders. You can verify this by comparing Profiler results after sending a few messages if needed |
|
No product review needed |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-native.movAndroid: mWeb Chromeandroid-web.moviOS: HybridAppios-native.moviOS: mWeb Safariios-web.mov |
|
🚧 @cristipaval 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. |
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 9.4.8-0 🚀
Bundle Size Analysis (Sentry): |
Help site review — no changes requiredI reviewed the changes in this PR against the help site files under Conclusion: No help site documentation changes are required, so I did not create a draft PR. This PR is a purely internal performance/memoization refactor of the
There are no changes to user-facing behavior, copy, UI labels, feature names, tab names, settings, or buttons. The only file/prop additions are internal React props ( @LukasMod, since no help site changes are required, there is no linked help site PR to review. If you believe a docs update is warranted here, let me know what behavior should be documented and I'll create the draft PR. |
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 9.4.8-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.4.8-3 🚀
|















Explanation of Change
Stabilizing report subscriptions in the ReportActionItem tree
Wasted re-renders on the report screen were traced to
report-shapeduseOnyxsubscriptionshanding back new references with deep-equal content on nearly every render. Three root causes, three fixes.
ReportActionsList.tsxOnyx keys its snapshot cache by selector function identity (
cacheKey = ${onyxKey}_${selectorID}).A shared module-level
getStableReportSelector→ every consumer on the same report key shares onecache slot → they overwrite each other's deep-equal-but-distinct result objects → reference churns.
useCallback(…, [])gives the hook a unique, stable selector identity → its own cache slot →stable reference until projected content actually changes.
ReportActionItem.tsxiouReportpreviously subscribed to the full report, so it re-rendered on everylast*heartbeatfield (
lastMessageText,lastVisibleActionCreated,lastReadTime, …) — which change on everymessage send.
getStableReportSelectorstrips those fields, so it only updates on meaningful change.ReportActionItem.tsx→ActionContentRouter.tsxThe
originalReportsubscription was moved out ofReportActionItemand intoActionContentRouter,the only component that consumes it (and the
originalReportprop was dropped).ReportActionItemno longer re-renders whenoriginalReportchanges — only the leaf that actuallyreads it does.
chatReportsubscription out of every row —ReportActionItem.tsxchatReportis only ever read inside thedismissErrorevent handler (cleanUpMoneyRequest), never onthe render path. Yet every row in the list subscribed to it independently — and since all rows in a list
share the same
report(so the samechatReportID), that was N identical subscriptions to one key,each re-running the selector on every update to it.
Fixed Issues
$ #93434
PROPOSAL:
Tests
No tests, simple selector/memoization change to make reference stable.
Offline tests
QA Steps
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari