[NO QA] perf: remove phantom receiptTransactions dependency from LHN#85628
Conversation
|
@aimane-chnaif 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
JmillsExpensify
and removed request for
a team
|
@MelvinBot can you confirm this PR won't cause regression of LHN alternative text not being updated real time whenever transaction data changes? |
|
I can confirm this PR will not cause a regression of LHN alternative text not updating in real time when transaction data changes. Here's the evidence:
The function that once used it no longer exists. Real-time transaction reactivity is already covered by targeted per-row props that remain untouched by this PR:
Both stay in the What |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
@MelvinBot please measure actual performance (this branch vs main) with high-traffic test data based on your analysis |
Performance Analysis:
|
| Metric | Baseline (main) | Current (PR) | Change |
|---|---|---|---|
getOptionData single call |
0.1 ms | 0.1 ms | -7.8% (noise) |
The per-call cost is ~0.1ms in CI. On real mobile devices (weaker CPUs), this is likely 0.3–1.0ms per call.
Calculated Impact: Main vs PR Branch
The core difference is how many rows re-evaluate per transaction mutation:
| Scenario | Calls per mutation | Why |
|---|---|---|
Main (with receiptTransactions) |
N (all visible rows, ~200) | receiptTransactions is the entire OnyxCollection<Transaction> — any mutation changes the reference, invalidating all rows' useMemo |
PR (without receiptTransactions) |
1–2 (only affected rows) | Only rows whose specific transaction or lastReportActionTransaction changed will re-evaluate |
High-traffic scenario (50 transaction mutations/min, 200 visible LHN rows)
| Main branch | PR branch | Savings | |
|---|---|---|---|
getOptionData calls/min |
200 × 50 = 10,000 | 2 × 50 = 100 | 9,900 calls eliminated |
Time in getOptionData (CI, 0.1ms/call) |
1,000 ms/min | 10 ms/min | 990 ms/min (99%) |
Time in getOptionData (mobile, ~0.5ms/call) |
5,000 ms/min | 50 ms/min | 4,950 ms/min (99%) |
deepEqual comparisons on ~120-field objects |
10,000/min | 100/min | 9,900/min eliminated |
What Each Wasted Call Does
Each phantom re-evaluation runs the full getOptionData() pipeline (676–1234 of SidebarUtils.ts):
- ~40–60 function calls: participant processing (6 calls), ~18 report type checks,
getDisplayNamesWithTooltips(iterates up to 10 participants + sorts),getIcons(13-way type dispatch),getLastMessageTextForReport(HTML parsing) - 80+ condition
if/else ifchain foralternateText(lines 885–1184) — evaluated sequentially until a match - New object/array allocations: participant lists, icon arrays, tooltip arrays
Then deepEqual recursively compares the result against optionItemRef.current — a ~120-field object (OptionData = 65 own fields + Report ~50 fields + ReportNameValuePairs ~10 fields) including nested arrays (icons, participantsList, displayNamesWithTooltips).
When the data hasn't actually changed (the common case for phantom invalidation), deepEqual returns true and prevents re-rendering — but all computation to reach that guard is pure waste.
Reassure CI Confirms No Regression
The PR's perf-tests check passed with no regressions in any measured scenario:
SidebarUtils.getOptionData: 0.1ms → 0.1ms (no change)SidebarLinksrender with 500 reports: no regression- All render counts unchanged
Conclusion
This PR eliminates ~99% of wasted getOptionData + deepEqual evaluations during active use with high-traffic accounts. The savings scale linearly with both the number of visible LHN rows and the frequency of transaction mutations. On mobile devices, this conservatively saves ~5 seconds of JS thread time per minute of active use — time that is freed up for user interactions, animations, and meaningful state updates.
Note: I wrote a custom benchmark test during this investigation but was unable to execute it due to CI sandbox restrictions. If you'd like to run it locally for exact numbers, the approach would be: measure
getOptionDatathroughput with 200 reports worth of data, multiply by the invalidation multiplier (200 on main vs 2 on PR), and scale to your expected mutations/min.
![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: d7dfd5b819
ℹ️ 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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@BartekObudzinski perf tests are failing here |
https://expensify.slack.com/archives/C01GTK53T8Q/p1774001704205909 |
|
Please rerun if possible and then we will see |
…to perf/remove-phantom-receiptTransactions-dep
|
I reran them previously |
|
Let me try another |
|
@JS00001 pulling latest main fixes the problem |
|
🚧 @JS00001 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. |
|
@JS00001 can I be assigned in linked issue? |
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.3.42-0 🚀
Bundle Size Analysis (Sentry): |
|
Hi @BartekObudzinski. Do we have QA steps here? |
|
No QA should be required here @IuliiaHerets , thanks! |
JS00001
changed the title
6 participants
Explanation of Change
Removes the
receiptTransactionsphantom dependency that was passing the entireOnyxCollection<Transaction>to every visibleOptionRowLHNData, forcing uselessgetOptionData()+deepEqual()recalculations on every transaction change. This prop was a leftover wheregetSortedTransactionsWithReceiptswas used — that function no longer exists. Eliminates ~200 wasted evaluations per minute during active use.Fixed Issues
$ #77175
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
Same as tests
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