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.
Details
- 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
web/chat/chat-message-list-container.react.js | ||
---|---|---|
42–44 | (Probably because it reduces unnecessary computation) |
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. |
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? |
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. |
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. |
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:
|
web/chat/chat-thread-composer.react.js | ||
91–93 ↗ | (On Diff #23051) | This can be simplified. |
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. |