Page MenuHomePhabricator

[web] Update AddUsersList to call useUserSearchIndex
ClosedPublic

Authored by rohan on Dec 8 2023, 5:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 1:42 PM
Unknown Object (File)
Fri, Dec 20, 6:11 AM
Unknown Object (File)
Nov 13 2024, 7:22 AM
Unknown Object (File)
Nov 5 2024, 10:47 PM
Unknown Object (File)
Nov 2 2024, 10:54 PM
Unknown Object (File)
Oct 24 2024, 2:55 AM
Unknown Object (File)
Oct 15 2024, 5:20 AM
Unknown Object (File)
Oct 15 2024, 1:22 AM
Subscribers

Details

Summary

There are several search experiences in the app that do not use get/usePotentialMemberItems. This means we need to update them individually, so I'm separating out diffs to do exactly this to make it easy for reviewers. [[ https://linear.app/comm/issue/ENG-6019/[after-landing]-try-to-update-remaining-search-experiences-to-use | ENG-6019 ]] tracks attempting to update these search experiences to use usePotentialMemberItems. Each diff will affect one file ideally.

This diff affects AddUsersList.

This component is used when adding friends from friend list on web

Addresses ENG-5434

Depends on D10252

Test Plan

Please see the video below where I add friends from the friend list on web

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan published this revision for review.Dec 8 2023, 6:15 AM
web/settings/relationship/add-users-list.react.js
61

There is a very subtle (but dangerous) flaw here

You're creating a new collection on every invocation of values. That means that useUserSearchIndex will need to generate a brand new search index on every render of this component.

This will definitely lead to a perf regression, and can even lead to a crash like ENG-4605 (fixed in D8760)

web/settings/relationship/add-users-list.react.js
61

Ah yeah you're definitely right. I tested this and a new index is getting generated each time the component re-renders. Thanks for catching this!

rohan edited the test plan for this revision. (Show Details)

Memoize values(userInfos)

This revision is now accepted and ready to land.Dec 12 2023, 12:30 AM