Solution for ENG-4551.
This diff introduces a custom data structure for chat mentions. We require this type of data structure to efficiently retrieve potential chat mention candidates.
Details
Details
- Reviewers
tomek inka - Commits
- rCOMM07be3385ef50: [lib] Introduce chat mention candidates getters
Use useChatMentionCandidatesObj in web/chat/chat.react.js and check if data structure is valid:
- Object keys should contain all ids available in redux store
- Each object value should be another object with threadID as keys and ResolvedThreadInfo as value.
- This inner object should contain threadIDs within the community without key (= threadID)
- Check GENESIS chats if they contain only chats that are inside the top level that is below GENESIS.
Example:
Thread store:
- GENESIS
- chat1
- chat1.1
- chat2
- chat1
- Test community
- chat3
- chat5
- chat4
- chat3
Valid object:
{chat1: {chat1.1}, {chat2: {}}, Test community: {chat3, chat5, chat4}, chat3: {Test community, chat4, chat5}, ...}
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- patryk/chat-mention-decoding
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Answers for potential questions:
Why this code is not in thread-selectors.js file?
To obtain UI thread name, we need to use useResolvedThreadInfosObj hook to convert ThreadInfo to ResolvedThreadInfo.
How does getChatMentionCandidates work?
Basic idea is to group chats within the same community. For GENESIS chats, we want to group them by first child of GENESIS. To achieve that, we need to do DFS on our “thread tree”.
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1637–1647 ↗ | (On Diff #30100) | First for loop groups chats within their communities. This part of the code creates an object for the community thread. When we want to add a chat to its community object, we use the community property in threadInfo (see lines 1702-1703). |
| 1670–1672 ↗ | (On Diff #30100) | We don't include already visited thread or GENESIS community thread in threadTraversePath, but we need to obtain them to get visited threads object (result[lastThreadInTraversePathParentID]) |
| 1673–1681 ↗ | (On Diff #30100) | This handles the case when inner thread in thread tree has parentThreadID set to null or parentThreadID thread does not exist in thread store. This might happen when our thread tree is corrupted (and we might have our tree corrupted: when we delete inner thread we still don't update parentThreadID property so it might refer to a thread which doesn't exist). |
| 1692–1694 ↗ | (On Diff #30100) | This line will be executed when we stop our DFS on inner node which parentThreadID property references thread which doesn't exist and is visited for the first time. |
| 1702–1703 ↗ | (On Diff #30100) | When adding a new thread to community object, we pass a reference to result[currentThreadID] rather than object copy. This object will be copied later, when we will filter out child object with id equal to currentThreadID. |
| 1706–1709 ↗ | (On Diff #30100) | Second for loop removes currentThreadID object. We need to do this, because we shouldn't be able to mention a chat in which we currently are. |
| 1739–1752 ↗ | (On Diff #30100) | Why do we need this? When we create pending sidebar it is not included in our chatMentionCandidates object. However we still need to obtain information about what chats are visible for the user. Solution to this problem is to use containingThreadID rather than id of a thread. If containingThreadID is not defined, empty object is returned. |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1742–1751 ↗ | (On Diff #30100) | We add threadInfo.containingThreadID thread info, because it is not available in chatMentionCandidates[threadInfo.containingThreadID]. |
Comment Actions
If I understand correctly, your structure will for every chat in a community contain an almost identical object: an object of all chats from this community excluding the one that is the key. This is very memory inefficient.
It would be better if you could think of a more efficient way of obtaining mention candidates. Since the chat can basically mention any chat in its community (or subtree with root being a child of GENESIS), wouldn't it be enough to have a collection of sets/some other collection of all chats in a community, and a chat would just check the set corresponding to its community?
@tomek suggested that you connect to the production keyserver that likely has a bigger database than your local setup to test your solution.
Also - what happens when someone tries to mention a chat in GENESIS?
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1638–1641 ↗ | (On Diff #30100) | If a child of GENESIS was already processed, we don't add GENESIS to the structure at all. But if GENESIS is the first to be processed, we do add it to the structure. I don't think it's supposed to be like this? |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1714–1717 ↗ | (On Diff #30100) | We typically return an object, not an array |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1638–1641 ↗ | (On Diff #30100) | If a child of GENESIS is processed first, then GENESIS community would be added in condition below (L1643-1646). |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1699–1702 ↗ | (On Diff #30547) | I think this causes a child of a removed chat to be considered a "root" of its tree. So if I had
And we remove "chat 1", then "subchat 1" is considered a root, and cannot mention for example "subchannel 2" |
| 1705–1707 ↗ | (On Diff #30547) | I think this can be merged with the condition above |
| 1638–1641 ↗ | (On Diff #30100) | Will we get to that line though? There is a continue statement... And wouldn't line 1644 throw an error then, since currentThreadCommunity is null for GENESIS? |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1638–1641 ↗ | (On Diff #30100) | Ohh, I get it now. My bad, you are right. |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1756–1757 ↗ | (On Diff #30613) | We don't usually define two variables in one let/const |
| 1757–1767 ↗ | (On Diff #30613) | You could also do it like this. |