[No QA] Perf: lazily compute B2B invoice report lookup on payment instead of every render#85813
Conversation
|
@ChavdaSachin 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] |
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.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88a54242ac
ℹ️ 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".
|
Would review within an hour, @TMisiukiewicz could you take a look at the codex review in the mean time. |
|
@ChavdaSachin IMO codex concern is not valid - The old code passed |
Reviewer Checklist
Screenshots/VideosN/A |
| if (!report || !isInvoiceRoom(report)) { | ||
| return false; | ||
| } | ||
| const rnvp = allReportNameValuePairs?.[`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report.reportID}`]; |
There was a problem hiding this comment.
I am afraid that refactors like this goes in the opposite direction of removing Onyx.connect.
Do we have some other alternative?
cc: @tgolen
There was a problem hiding this comment.
An alternative is a solution that I just opened a draft PR for: #85949. It isolates logic and Onyx calls, giving similar results (unless user has tens of rendered Pay buttons). Should we move forward with decomposition PR only or ship both just in case to avoid the perf edge cases?
There was a problem hiding this comment.
Oh that is a nice PR!
Do you mean the changes here are temporary or do you plan to use this in the decomposition PR as well?
There was a problem hiding this comment.
Yeah If we decide to ship this PR as-is, I'll also include this change in #85949. This PR removes the optimistic computation of a value that is only needed when Pay button is pressed - without it, we'd still compute it for every Pay button even if user does not plan to press it
There was a problem hiding this comment.
Yeah, I will wait for Tim's thoughts, but I think once we get to allReportNameValuePairs and allReports in the Onyx.connect issues we will end up reverting back to the previous way without realizing it.
There was a problem hiding this comment.
I'd prefer to avoid shipping this PR as-is since it will just create more work that needs to be refactored later.
Explanation of Change
useParticipantsInvoiceReporthook was subscribing to ALL reports on every render of every money request preview item, but the result was only used when clicking Pay on an invoice. This caused unnecessary re-renders and Onyx subscriptions across all visible invoice preview items.Moved the B2B invoice report lookup into
getPayMoneyRequestParamsin IOU actions so it only runs at payment time, not on every render. Removed the hook fromMoneyRequestReportPreviewContentandMoneyReportHeaderentirely.Results on iOS when opening expenses report using Jason's state:
ManualOpenReport2400ms -> 2100msMoneyRequestReportPreviewContentrender time 412ms -> 195msFixed Issues
$ #77173
PROPOSAL:
Tests
No tests needed, pure refactor
Offline tests
N/A
QA Steps
No tests needed, pure refactor
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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