Page MenuHomePhabricator

[web] Update SubchannelMembers to call useUserSearchIndex
ClosedPublic

Authored by rohan on Dec 8 2023, 5:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 17 2024, 10:08 PM
Unknown Object (File)
Mar 7 2024, 9:05 AM
Unknown Object (File)
Mar 7 2024, 6:53 AM
Unknown Object (File)
Mar 7 2024, 6:04 AM
Unknown Object (File)
Mar 7 2024, 5:10 AM
Unknown Object (File)
Mar 7 2024, 5:00 AM
Unknown Object (File)
Dec 29 2023, 3:44 PM
Unknown Object (File)
Dec 29 2023, 8:43 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 SubchannelMembers.

This component is used when adding subchannel members during creation on web

Addresses ENG-5434

Depends on D10249

Test Plan

Please see the video below where I show the modal when adding subchannel members

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 8 2023, 6:08 AM
Harbormaster failed remote builds in B24887: Diff 34417!
rohan requested review of this revision.Dec 8 2023, 6:15 AM

Same comment as in previous diff - even though we were using userStoreSearchIndex we were only displaying the users from this thread. There is some logic in MembersList that takes care of this, that can probably be simplified now. Let me know if you think it should be left as is though, I can see that we could want that. Just wanted to flag that this should be considered

In D10250#298189, @inka wrote:

Same comment as in previous diff - even though we were using userStoreSearchIndex we were only displaying the users from this thread. There is some logic in MembersList that takes care of this, that can probably be simplified now. Let me know if you think it should be left as is though, I can see that we could want that. Just wanted to flag that this should be considered

Thanks for pointing it out - I responded to the comment in D10249 as well. I see what you mean about the logic in MembersList.

I think it'll still be good to keep as-is because all this diff does is reduce the size of the search index to the members in the parent thread (alongside making it possible to search by their ENS name). The code in MembersList that filters will still need to additionally include/exclude user IDs from the search index based on the search text.

Let me know if you disagree though

Hold on, looking at MembersList it seems we use searchResults to create parentMemberList but also otherMemberList - a list of members of the community that are not in the parent thread:

image.png (714×756 px, 49 KB)

Doesn't your code make it so that the otherMemberList is always empty?
If you have a community that looks like this:

  • "Community chat" members: user1, user2, user3
    • "Channel" members: user1, user2
      • "Subchannel" members: user1

And search for "user" in "Add members" modal in "Subchannel"
Isn't Not in parent chat: section empty even though it should contain user3?

inka requested changes to this revision.Dec 14 2023, 1:44 AM

Putting this back in your queue as discussed on yesterday's retro. If you find that my comment is incorrect and requires no action, please re-request review.

This revision now requires changes to proceed.Dec 14 2023, 1:44 AM
In D10250#298764, @inka wrote:

Doesn't your code make it so that the otherMemberList is always empty?

I tried the example you were talking about but didn't see an issue where the otherMemberList was empty.

Creating the subchannels:

Searching in the subchannel "Add Members" modal:

Also reading the code, I think maybe there's a misunderstanding:

I think MembersList (in subchannel-members-list.react.js) is used during subchannel creation, where we list 1. users in parent channel and 2. users in the community root

For adding members to an already existing subchannel, it looks like we use AddMembersModal. That uses a different search usePotentialMemberItems that is a little more complicated and was updated in D10231, but there's no regressions introduced there.


So I think we're fine in terms of showing users. I did notice when testing that in your scenario, searching for users not in the parent thread wasn’t possible since now the search index only included members of the parent thread (before it was the entire userStore). I'll update this diff to construct the search index using the community thread info members instead

parentThread --> communityThread

Looks like @inka is giving this a thorough review, so resigning to remove from queue. Feel free to re-assign me as reviewer if there's something specific I can be useful with.

You're right, the problem was not in Add members but in the subchannel creation modal

This revision is now accepted and ready to land.Dec 15 2023, 2:31 AM