Page MenuHomePhabricator

[web] Call searchUsers in chat composer
AbandonedPublic

Authored by kuba on Oct 24 2022, 6:32 AM.
Tags
None
Referenced Files
F3390281: D5463.diff
Fri, Nov 29, 11:08 PM
Unknown Object (File)
Wed, Nov 27, 1:11 AM
Unknown Object (File)
Tue, Nov 26, 5:55 PM
Unknown Object (File)
Tue, Nov 26, 5:55 PM
Unknown Object (File)
Tue, Nov 26, 5:45 PM
Unknown Object (File)
Tue, Nov 26, 5:45 PM
Unknown Object (File)
Tue, Nov 26, 5:31 PM
Unknown Object (File)
Mon, Nov 25, 3:46 AM
Subscribers

Details

Reviewers
tomek
michal
Summary

ENG-1988
Adds a call to searchUsers in chat composer so a new chat can be created with users that are not in userStore.
Results from searchUsers are merged with userStore, and are treated as users that the viewer doesn't have a relationship with.
To enable chat creation, forceAddMembers is now set to true in threadCreationResponder.

Test Plan
  • Test if users that are not in userStore display in the search list
  • Try selecting/ unselecting them and leaving chat composer when they are selected
  • Try creating a new chat with multiple users that are not in userStore

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision now requires changes to proceed.Nov 4 2022, 8:16 AM
web/chat/chat-message-list-container.react.js
42–44 ↗(On Diff #18025)

(Probably because it reduces unnecessary computation)

Update

web/chat/chat-message-list-container.react.js
42–44 ↗(On Diff #18025)

@ashoat is correct, we only have to do this conversion once

53–62 ↗(On Diff #18025)

Good point, merged

Some additional notes in the inline comments.

lib/selectors/user-selectors.js
116 ↗(On Diff #18097)

Refactored this function out of userInfoSelectorForPotentialMembers so I don't duplicate code when filtering userInfos from keyserver.

web/chat/chat-message-list-container.react.js
52 ↗(On Diff #18097)

We need to store usernames alongside the userIDs (because we might not have some of these users in userStore so we can't access their usernames later).

Because of this, we need to stop keeping the state in navInfo.selectedUserList (we only store ids there). Now the navInfo is only used for the initialization, and later it's only updated to reflect the state in userInfoInputArray.

153 ↗(On Diff #18097)

This useEffect combines the two previous ones.

web/chat/chat-thread-composer.react.js
82 ↗(On Diff #18097)

Logic for getting userInfos from the keyserver. callSearchUsers doesn't return relationships, but we can assume that if we don't have someone in the userStore we don't have a relationship with them. Because of that we need to remove users that we already have in the userStore.

128 ↗(On Diff #18097)

Instead of updating the navInfo we can just use setUserInfoInputArray. These changes will be dispatched in the useEffect in chat-message-list-container.react.js.

tomek requested changes to this revision.Nov 8 2022, 5:24 AM
tomek added inline comments.
web/chat/chat-message-list-container.react.js
117–118 ↗(On Diff #18097)

We should exit early

120–121 ↗(On Diff #18097)

It might be a good idea to extract some of these to separate memos so that we don't recompute them on every effect call

141–144 ↗(On Diff #18097)

Is it possible for payload to be empty here?

web/chat/chat-thread-composer.react.js
66–68 ↗(On Diff #18097)

It seems like this approach is the same as in other places where we call this endpoint, but it is not what we should do. Instead we should use dispatchActionPromise so that appropriate actions are dispatched.

77–85 ↗(On Diff #18097)

Are you sure this is correct? We're filtering out the users that are in userInfos but then merge with otherUserInfos - it may be correct, but could you explain why is that?

91 ↗(On Diff #18097)

Why do we need to clone the index? What is the approach used in other similar places - do we do something similar there?

This revision now requires changes to proceed.Nov 8 2022, 5:24 AM
web/chat/chat-message-list-container.react.js
141–144 ↗(On Diff #18097)

Good point. When we add a new user the newSelectedUserIDs changes which triggers this dispatch and updates the navInfo. Then the userEffect runs again because selectedUserIDs is updated but now it's equal to newSelectedUserIDs and the payload is empty.

Should just surround the dispatch with an if?

web/chat/chat-thread-composer.react.js
77–85 ↗(On Diff #18097)

We want to remove users from serverSearchUserInfos that blocked the viewer but the useServerCall(searchUsers) doesn't return the relationship status.

We can assume that we have a relationship with someone only if we have them in the userStore. So we remove users that are in userInfos and that leaves us with only the users that we don't have a relationship with.

Users that were removed but didn't blocks us are included in the otherUserInfos. So when we merge these two values we should get a list of users that didn't block us from both userStore and keyserver.

91 ↗(On Diff #18097)

Modifying searchIndex mutates it, so to keep it pure I had to clone it. I'm doing this because getPotentialMemberItems requires SearchIndex with all the potential users.

I've just looked at how this is solved in other places and there we just use the results from the useServerCall(searchUsers) directly. We can use a similar approach here, we would just have to modify getPotentialMemberItems to take an additional array of users that skip the SearchIndex.

michal requested review of this revision.Nov 9 2022, 3:37 AM
tomek requested changes to this revision.Nov 10 2022, 7:39 AM
tomek added inline comments.
web/chat/chat-message-list-container.react.js
141–144 ↗(On Diff #18097)

Yes, I think so

web/chat/chat-thread-composer.react.js
77–85 ↗(On Diff #18097)

ok, that makes sense

91 ↗(On Diff #18097)

Cloning the index on every change seems really wasteful - usually we would need to only add or remove a single entry. But regardless, taking a similar approach that we have in other places is a really good idea, unless there are good reasons not to do so.

This revision now requires changes to proceed.Nov 10 2022, 7:39 AM
kuba added a reviewer: michal.
kuba marked 4 inline comments as done.

Rebase to master

kuba marked an inline comment as done.

Removed deepclone, surrounded dispatch with if in case of empty payload

Used function from the lib instead of rewriting it

lib/shared/search-utils.js
81–87 ↗(On Diff #23051)

This won't happen now but I think we should check if we aren't adding duplicates from the searchIndexes.

web/chat/chat-message-list-container.react.js
123–124 ↗(On Diff #23051)

Per tomek's comment:

It might be a good idea to extract some of these to separate memos so that we don't recompute them on every effect call

web/chat/chat-thread-composer.react.js
91–93 ↗(On Diff #23051)

This can be simplified.

kuba marked 3 inline comments as done.

Fixed according to Michal notes

tomek requested changes to this revision.Mar 7 2023, 3:52 AM
tomek added inline comments.
web/chat/chat-thread-composer.react.js
67 ↗(On Diff #23221)

We should avoid calling server directly. A better approach is to do it indirectly by using useDispatchActionPromise hook.

103–107 ↗(On Diff #23221)

It is confusing to me why do we need to modify this function add accept two search indexes. Instead of calling the server, creating index based on response, then searching using this index, we could just use what server returned to us.

This revision now requires changes to proceed.Mar 7 2023, 3:52 AM