Page MenuHomePhabricator
Differential D10229

[lib] Convert getPotentialMemberItems into a hook
ClosedPublic
Actions

Authored by rohan on Dec 7 2023, 11:22 AM.
Tags
None
Referenced Files
F3323270: D10229.id34555.diff
Wed, Nov 20, 3:28 AM
Unknown Object (File)
Tue, Nov 12, 3:35 PM
Unknown Object (File)
Tue, Nov 5, 10:47 PM
Unknown Object (File)
Wed, Oct 23, 12:55 AM
Unknown Object (File)
Oct 14 2024, 3:28 AM
Unknown Object (File)
Oct 14 2024, 3:28 AM
Unknown Object (File)
Oct 14 2024, 3:28 AM
Unknown Object (File)
Oct 14 2024, 3:28 AM
View All 68 Files
Subscribers
ashoat
tomek

Details

Reviewers
atul
ginsu
inka
Commits
rCOMMf68b983ca9ad: [lib] Convert getPotentialMemberItems into a hook
Summary

This should be a no-op change in terms of the results that getPotentialMemberItems used to return. I'm simply converting it into a hook so I can call a hook that will construct a search index in the next diff, necessary to make it possible to prefix-search for users by their ENS names.

Addresses ENG-5402

Test Plan

Did some manual testing to confirm that there was no change in the search results returned (both by console.log'ing before and after, as well as searching for users on the app)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan created this revision.Dec 7 2023, 11:22 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptDec 7 2023, 11:22 AM
rohan added a child revision: D10231: [lib/web/native] Introduce hook that constructs a resolved user search index and update usePotentialMemberItems.Dec 7 2023, 11:30 AM
Harbormaster completed remote builds in B24861: Diff 34385.Dec 7 2023, 12:01 PM
rohan requested review of this revision.Dec 7 2023, 12:01 PM
ginsu added a comment.Dec 7 2023, 12:09 PM

About to go to lunch and will give a more quality review later, but just saw the blockedRelationshipsStatuses set and I think there maybe some code that you can reuse.

lib/shared/search-utils.js
46–50

IIRC I think there may be a preexisting blockedRelationshipsStatuses set in lib already. I remember having to use something similar for user profiles.

rohan added inline comments.Dec 7 2023, 1:13 PM
lib/shared/search-utils.js
46–50

Oh good call. I couldn't see a set, but I did find relationshipBlockedInEitherDirection which includes all 3

userRelationshipStatus.BLOCKED_BY_VIEWER,
userRelationshipStatus.BLOCKED_VIEWER,
userRelationshipStatus.BOTH_BLOCKED

so I can use that probably

rohan updated this revision to Diff 34394.Dec 7 2023, 1:16 PM

Use relationshipBlockedInEitherDirection instead of redefining a blocked set

Harbormaster completed remote builds in B24870: Diff 34394.Dec 7 2023, 2:04 PM
ginsu requested changes to this revision.Dec 7 2023, 3:01 PM
ginsu added inline comments.
lib/shared/search-utils.js
49–50 ↗(On Diff #34394)

It looks like in every case we are getting searchIndex from a useSelector hook in every case and userInfo from a useSelector in most cases (I wonder if in ChatThreadComposer) if it's also using a useSelector in parent component and getting passed down as a prop.

Regardless at the very least we should factor out searchIndex as a param and just call it directly in here

66–84 ↗(On Diff #34394)

Wouldn't hurt to also memoize these guys too (containgThreadID is probably fine to not memoize since that is a string)

86–92 ↗(On Diff #34394)

We should memoize this and the related logic below

133 ↗(On Diff #34394)

We should memoize this guy too

This revision now requires changes to proceed.Dec 7 2023, 3:01 PM
rohan updated this revision to Diff 34402.Dec 7 2023, 6:54 PM
rohan marked 4 inline comments as done.

Address feedback

lib/shared/search-utils.js
49–50 ↗(On Diff #34394)

I end up removing the provided searchIndex param in the next diff (D10231) since it will no longer be needed, so if it makes sense to you I'll leave it as is in this diff to prevent adding code that's going to get deleted anyways!

Let me know if you disagree though

Harbormaster completed remote builds in B24878: Diff 34402.Dec 7 2023, 7:21 PM
ginsu added inline comments.Dec 11 2023, 11:45 AM
lib/shared/search-utils.js
102 ↗(On Diff #34402)

sorry I missed this in my first review cycle, but we should factor this out into its own useCallback

49–50 ↗(On Diff #34394)

Okay that makes sense, thanks for letting me know!

ginsu requested changes to this revision.Dec 11 2023, 11:47 AM

putting this back in your queue

This revision now requires changes to proceed.Dec 11 2023, 11:47 AM
rohan updated this revision to Diff 34555.Dec 12 2023, 3:37 PM

Spoke with @ginsu and addressed feedback following that

Harbormaster completed remote builds in B25014: Diff 34555.Dec 12 2023, 3:56 PM
ginsu accepted this revision.Dec 13 2023, 9:21 PM

thanks for addressing my feedback @rohan

This revision is now accepted and ready to land.Dec 13 2023, 9:21 PM
Closed by commit rCOMMf68b983ca9ad: [lib] Convert getPotentialMemberItems into a hook (authored by rohan). · Explain WhyDec 15 2023, 8:33 AM
This revision was automatically updated to reflect the committed changes.
rohan added a commit: rCOMMf68b983ca9ad: [lib] Convert getPotentialMemberItems into a hook.

Revision Contents
Changeset List

PathSize
lib/
shared/
178 lines
native/
chat/
32 lines
24 lines
settings/
32 lines
community-creation/
30 lines
web/
chat/
26 lines
modals/
threads/
members/
32 lines

Diff 34385

View Options

lib/shared/search-utils.js

Loading...
View Options

native/chat/compose-subchannel.react.js

Loading...
View Options

native/chat/message-list-container.react.js

Loading...
View Options

native/chat/settings/add-users-modal.react.js

Loading...
View Options

native/community-creation/community-creation-members.react.js

Loading...
View Options

web/chat/chat-thread-composer.react.js

Loading...
View Options

web/modals/threads/members/add-members-modal.react.js

Loading...