Repository navigation
Optimize top-N report selection in option list building - #96748
mjasikowski merged 18 commits into
Conversation
… directly into createFilteredOptionList
dariusz-biela
left a comment
There was a problem hiding this comment.
This looks good. I think we should also apply the precomputed-key optimization to optionsOrderAndGroupBy(), which is used by getValidOptions() in the Share flow. It currently recalculates recentReportComparator during heap comparisons, while both helpers solve the same bounded top-N problem.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@ikevin127 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] |
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-07-27.at.17.41.29.mov |
|
@jmgraa It's a stale-branch artifact, not from this PR. Your diff only touches Could you merge
|
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM
Only have two 🟡 non-blocker comments (see above) and one
🧪 Tests I ran to validate PRs changes:
✅ 1. Recency and self-DM ordering (online): Open New chat (+) on a high-traffic account. Confirm most-recent chats are at the top and your self-DM (Notes) sits near the top even if stale. This exercises the top-N heap selection and reportSortComparator priority.
✅ 2. Search returns all matches (online): Open Search, type a name that matches an older or archived chat, confirm it appears; clear the query and confirm the recent list restores. This validates the isSearching branch that bypasses the cap.
✅ 3. Offline parity + Share flow (offline): Turn off network, repeat New chat and Share-into-Expensify, confirm the same destination chats load, are pickable, and no console error fires. This covers ShareTab's optionsOrderBy call and the Array.from empty-group path.
Reassure perf: baseline (main) vs this PR
Headline: createFilteredOptionList, the function this PR optimizes, drops 44.4 ms → 23.6 ms (−46.8%), and its search-mode variant 17.0 ms → 11.9 ms (−30.0%). This corroborates the PR description's numbers (different absolute ms because of machine/fixture, same direction and magnitude).
measureFunctionmean, single run each. Negative delta = faster
| Scenario | Baseline | This PR | Δ | % | |
|---|---|---|---|---|---|
createFilteredOptionList |
44.4 ms | 23.6 ms | −20.8 ms | −46.8% | 🟢 |
createFilteredOptionList with isSearching: true |
17.0 ms | 11.9 ms | −5.1 ms | −30.0% | 🟢 |
getSearchOptions with isSearching: true |
1.2 ms | 1.0 ms | −0.2 ms | −16.7% | 🟢 |
getSearchOptions |
6.8 ms | 5.8 ms | −1.0 ms | −14.7% | 🟢 |
getShareDestinationOptions |
6.6 ms | 5.8 ms | −0.8 ms | −12.1% | 🟢 |
| empty search term with selected options + mocked personal details | 1.7 ms | 1.6 ms | −0.1 ms | −5.9% | ⚪ |
getFilteredOptions with empty search value |
0.4 ms | 0.4 ms | 0.0 ms | 0.0% | ⚪ |
getFilteredOptions with search value |
1.9 ms | 2.0 ms | +0.1 ms | +5.3% | ⚪ |
| worst case: search term matching subset of selected / recent reports / personal details | 13.4 ms | 14.5 ms | +1.1 ms | +8.2% | ⚪ |
|
🚧 mjasikowski 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/mjasikowski in version: 9.4.46-0 🚀
|
|
🤖 I reviewed the changes in this PR against Expensify's help site content under No help site changes are required. This PR is a pure internal performance optimization of the option-list building logic — it refactors the heap-based top-N report selection in As noted in your own PR summary, aside from an internal If you believe a specific help article does describe behavior affected here, let me know which one and I'll take another look. |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.46-10 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Optimizes report selection in
createFilteredOptionListfor accounts with large report lists.optionsOrderByandoptionsOrderAndGroupBynow decorate each option with its pre-computed sort key (sharedDecoratedOptionHeaphelper), so the comparator runs exactly once per option instead of on every O(log n) heap comparison and every eviction check, cutting redundant work when picking the top N reports.Top-N selection happens directly in
createFilteredOptionListviaoptionsOrderBywithreportSortComparator, keeping the same rules: self-DM first, then non-archived, then most recent; search mode still returns all reports.optionsOrderByitself is now implemented asoptionsOrderAndGroupBywith no separators, removing the duplicated heap logic between the two functions.Two fixes along the way:
optionsOrderAndGroupBynow respectsreversed: truewhen a heap is at capacity - previously the eviction check always compared with>, so reversed callers kept the newest options instead of the oldest. Covered by a new unit test.limit <= 0early return inoptionsOrderAndGroupByreturned a sparse array (Array(n).map()skips empty slots), which could crash callers destructuring the groups; it now returns N+1 real empty arrays viaArray.from.Aside from the
reversed: truefix, no user-facing behavior change - same reports, same order, less work during option-list building. Unit tests cover comparator call counts, grouping, limits, and reversed ordering.createFilteredOptionList (tests/perf-test/OptionsListUtils.perf-test.ts)
Fixed Issues
$ #95335
PROPOSAL:
Tests
In addition, the component should behave just as it does in production
Offline tests
Same as tests
QA Steps
Same as tests
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_1.mov
iOS: Native
ios_1.mov
MacOS: Chrome / Safari
web_1.mov