Perf/spend filters hover - #97697
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
51bb726 to
49096a3
Compare
Hovering a filter row in the Search filters popover remounted its whole content and rebuilt the option lists from scratch, which blocked the main thread for over a second per row on accounts with large personal details. - Switch the content pane to a debounced rested filter, so sweeping the cursor across rows moves only the highlight. - Keep the last three rested filter contents mounted inside <Activity>, so returning to one toggles visibility instead of remounting. - Memoize the three full passes over the personal details list (createOptionList, getValidOptions, the UserSelector list copy) with a comparator that walks one level into rebuilt record arguments, since the snapshot-aware useOnyx rebuilds collection identities on Search pages every render. - Keep the option list Onyx subscriptions warm for the popover lifetime.
49096a3 to
b745fb8
Compare
Drop the keep-warm Onyx subscription, the module-level memoization of the personal detail derivations and the equivalentArgsComparator they needed. Freezing the form values of backgrounded contents and comparing props on them keeps the same panes from re-rendering, so the caches are unnecessary. Explain why React.memo is still needed next to React Compiler: the compiler caches the whole mountedFilters.map() result in a single slot, so it cannot bail out on an individual element.
Building an option per personal details entry and filtering the whole option list are pure derivations of Onyx data, so a remounted filter content can reuse the previous result instead of walking the collection again. Both are memoized at module level behind a new equivalentArgsComparator in @libs/memoize, which compares memoization keys argument by argument: shallowly first, and one level deeper for plain objects that are rebuilt on every call from unchanged sources. Measured on an account with 100k personal details, sweeping and resting on every filter row: 4.2s of blocked JS down to 1.7s.
| * The two-level fallback below reads arguments through `Object.keys`, which only describes plain objects - for other | ||
| * types (Set, Map, Date, class instances) it returns an empty list and any two instances would look identical. | ||
| */ | ||
| const isPlainObject = (value: unknown): value is Record<string, unknown> => { |
There was a problem hiding this comment.
Can't we use lodash-es/isPlainObject instead ?
There was a problem hiding this comment.
Yes, switching to it.
| ACCESSIBILITY_ANNOUNCEMENT_DEBOUNCE_TIME: 1000, | ||
| SUGGESTION_DEBOUNCE_TIME: 100, | ||
| /** How long the cursor has to stay on an advanced filter row before its content is rendered */ | ||
| SEARCH_FILTER_HOVER_INTENT_DELAY: 80, |
There was a problem hiding this comment.
Can be a potential accessibility issue, if user navigates with keyboard they can land in old pane and then it switches ?
There was a problem hiding this comment.
Good catch, you're right, I'm fixing it.
The 80ms delay is currently applied to both mouse hover and keyboard focus. It makes sense for hover, because it avoids rendering filters you only pass over with the cursor. For keyboard navigation, every focus change is intentional, so the delay just causes the details pane to lag behind the selected row.
| }); | ||
| const [contentVersions, setContentVersions] = useState<Partial<Record<SearchFilter['key'], number>>>({}); | ||
| if (mountedFilters.at(-1) !== restedFilter) { | ||
| if (mountedFilters.includes(restedFilter) && formAtLastRest[restedFilter] !== searchAdvancedFiltersForm) { |
There was a problem hiding this comment.
NAB: Not sure if I understad this correct but in a scenario:
- We open filter
- Hover & Select
From(Frommemo) - Hover
To(Tomemo) - Hover back on
From- the objects identities are different and we get full remount?
If this is correct, can it be changed to prevent this ?
There was a problem hiding this comment.
You read it correctly — it's intentional.
UserSelector snapshots the pre-selected accountIDs on mount to keep them pinned at the top (#61414). Keeping the pane mounted would make that snapshot stale, so newly selected users would not float to the top when returning to the view.
I did try an isActive prop that re-snapshots the selection when the pane becomes visible, but dropped that approach since it would require threading the prop through multiple components just to reach UserSelector.
There was a problem hiding this comment.
yes intentional but only when list size is large (>= 12)
| * Building an option per personal details entry is the expensive step of this hook and depends only on Onyx values. | ||
| * Arguments are compared one level deep because the maps are rebuilt each render from those same unchanged values. | ||
| */ | ||
| const memoizedCreateOptionList = memoize(createOptionList, { |
There was a problem hiding this comment.
Does this memoize survive logout ?
There was a problem hiding this comment.
It does - great catch, fixing it.
| if (mountedFilters.includes(restedFilter) && formAtLastRest[restedFilter] !== searchAdvancedFiltersForm) { | ||
| setContentVersions({...contentVersions, [restedFilter]: (contentVersions[restedFilter] ?? 0) + 1}); | ||
| } | ||
| setFormAtLastRest({...formAtLastRest, [restedFilter]: searchAdvancedFiltersForm}); |
There was a problem hiding this comment.
NAB: You want to keep 3 latest forms, but in theory this object can grow to how many restedFilter s variants there are
There was a problem hiding this comment.
I’ll update it to cap the maps like mountedFilters.
- Keyboard focus no longer goes through the hover intent delay. Moving focus is deliberate and never sweeps across rows, so its content is shown right away instead of replacing the pane a keyboard user has already moved into. - The memoized personal detail caches are released on sign-out, so option lists built for the previous account do not stay in memory. Needed a clear() on the memoize cache, which it did not expose. - formAtLastRest and contentVersions are pruned to the mounted filters instead of gaining an entry per visited filter. - equivalentArgsComparator uses lodash/isPlainObject instead of a local copy.
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🚧 mountiny 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! 🧪🧪
|
4 participants
Explanation of Change
What was wrong
Every hover over a filter row remounted the whole content pane (
key={baseFilterKey}forces a full remount), and the people filters (From, To, Attendee) rebuilt the entire personal detail option list from scratch on every mount: several full O(n) passes over thepersonalDetailsListcollection (building options, filtering, sorting, cloning). On accounts with a large number of contacts this blocked the JS thread for hundreds of milliseconds per hover, so sweeping the cursor down the filter list froze the popover and the highlight lagged several rows behind the cursor.What this PR changes
SEARCH_FILTER_HOVER_INTENT_DELAY(80 ms). Sweeping across rows no longer renders a content pane per row.MAX_MOUNTED_FILTER_CONTENTS(3) most recently visited filter contents stay mounted and are hidden instead of unmounted, with LRU eviction. Returning to a recently used filter toggles visibility instead of paying the full remount cost, and it preserves the pane state (e.g. a typed search term). A backgrounded content keeps the form values it had at its last visit, so moving between filters does not re-render it; a content whose values went stale is remounted on the way back in.createOptionListinusePersonalDetailOptions, andgetValidOptionsplus the selected-state mapping inusePersonalDetailSearchSelector, are pure derivations of Onyx data, so they are memoized at module level and survive a remount. They use a newequivalentArgsComparatorexported from@libs/memoize, which compares memoization keys argument by argument: shallowly first, and one level deeper for plain objects that are rebuilt on every call from unchanged sources (e.g. a mapped Onyx collection).usePersonalDetailOptionsalso skips its whole derivation chain while it is loading or disabled.Measurements
Chrome, dev build, an account seeded with 100k personal details. One run = sweeping the cursor over all 12 filter rows without stopping (3 passes each way), then resting on each row, then alternating between From and To. The metric is the total
longtaskduration - how long the JS thread was blocked - summed over the whole run. Variants were interleaved rather than measured in blocks, and each number below is a mean over 2-5 runs.mainmaincreateOptionList(3)getValidOptionsand selected-state mapping (3)The debounce alone accounts for most of it: it takes the fast-sweep phase from 9.1 s of blocked JS to 0.
Two further changes were built and measured, and are deliberately not part of this PR because they did not earn their complexity:
UserSelectorAbsolute values are inflated by the dev build; the ordering and the ratios are the part to trust.
Before / after comparison
performance.filters.mp4
Fixed Issues
$ #97455
PROPOSAL:
Tests
Best tested on an account with a large number of contacts (e.g. a High Traffic account), where the freeze was clearly visible before this change.
Offline tests
Unnecessary - this is a rendering performance change only, no network interaction is affected.
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.