Optimize top-N report selection in option list building#96748
Conversation
… directly into createFilteredOptionList
dariusz-biela
left a comment
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!
|
| // If limit is 0 or negative, return N+1 empty arrays | ||
| if (limit !== undefined && limit <= 0) { | ||
| return {options: Array(separators.length + 1).map(() => []), hasMore}; | ||
| } |
There was a problem hiding this comment.
The limit <= 0 early return returns a sparse array rather than N + 1 empty arrays. Array(n) creates empty slots, and .map() skips them, so with two separators the result has length 3 but result.options[0] is undefined.
This can cause getValidOptions() to crash after destructuring the result and accessing selfDMChats.length.
Although this issue already exists on main, this PR adds a test for exactly this path (does not call comparator when limit is 0), but the test only checks comparator calls and therefore does not catch the invalid result.
Could we change this to:
return {options: Array.from({length: separators.length + 1}, () => []), hasMore};and strengthen the test with:
expect(result.options).toStrictEqual([[], [], []]);There was a problem hiding this comment.
Could you please update the PR description so it reflects the current implementation and scope?
-
The PR description still references the removed selectTopReportsForOptionList helper and should be updated to reflect the current implementation.
-
The PR fixes optionsOrderAndGroupBy() behavior for reversed=true, so this semantic change should be documented and covered by a test.
-
Once you've fixed
return {options: Array(separators.length + 1).map(() => []), hasMore};, please mention this in the PR description as well.
| pushDecoratedOption(decoratedOption) { | ||
| if (limit === undefined || heap.size() < limit) { | ||
| heap.push(decoratedOption); | ||
| return false; | ||
| } | ||
|
|
||
| const peekedValue = heap.peek(); | ||
| if (!peekedValue) { | ||
| throw new Error('Heap is empty, cannot peek value'); | ||
| } | ||
|
|
||
| if (reversed ? decoratedOption.key < peekedValue.key : decoratedOption.key > peekedValue.key) { | ||
| heap.pop(); | ||
| heap.push(decoratedOption); | ||
| } | ||
|
|
||
| return true; | ||
| }, |
There was a problem hiding this comment.
Could we make the boolean contract of pushDecoratedOption() more explicit? It returns true when the heap was already at capacity, rather than when the option was pushed, so a name such as pushAndCheckHasMore() or a named {hasMore} result would make the call site easier to understand.
2 participants
Explanation of Change
Optimizes report selection in createFilteredOptionList for accounts with large report lists.
optionsOrderBy now pre-computes each item’s sort key once instead of re-running the comparator on every heap comparison, cutting redundant work when picking the top N reports.
Top-N selection is extracted into selectTopReportsForOptionList with the same rules: self-DM first, then non-archived, then most recent; search mode still returns all reports. Unit tests cover the new helper.
No user-facing behavior change - same reports, same order, less work during option-list building.
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