Details

If I understand correctly, this structure is build by creating a SentencePrefixSearchIndex for each community (or subtree with a root being a child of GENESIS), and having a "map" from every chat, to its corresponding SentencePrefixSearchIndex. But I don't understand why we needed to exclude the thread itself from its chatMentionCandidates, but we don't need to exclude it from this structure? Can you explain please?

Also - I think this code will change a lot after D8833 is refactored.

This revision now requires changes to proceed.Aug 29 2023, 4:51 AM

Discussed this offline:

But I don't understand why we needed to exclude the thread itself from its chatMentionCandidates, but we don't need to exclude it from this structure? Can you explain please?

Excluding the thread in which we mention was being handled by getMentionTypeaheadChatSuggestions introduced in D8910 (see for (const threadID in chatMentionCandidates) loop). But this is going to be modified: to avoid unnecessary for loop we can either extract thread itself from useThreadChatMentionSearchIndex or just to add if condition in getMentionTypeaheadChatSuggestions which filters out current thread id.

btw, I think that getMentionTypeaheadChatSuggestions should not be defined in D8910, but in separate diff...

Rebase after D8833 changes

Harbormaster completed remote builds in B22251: Diff 30548.Aug 30 2023, 6:55 AM

Change useChatMentionSearchIndex

Harbormaster completed remote builds in B22290: Diff 30614.Aug 31 2023, 12:00 AM

inka accepted this revision.Sep 6 2023, 1:07 AM

This revision is now accepted and ready to land.Sep 6 2023, 1:07 AM

tomek requested changes to this revision.Sep 12 2023, 8:05 AM

tomek added inline comments.

lib/shared/thread-utils.js
1780–1794 ↗	(On Diff #30614)	There are two possible performance issues here: Every time there's a change in a value returned by `useChatMentionCandidatesObjAndUtils` we're recomputing all the indexes for all the threads. It would be great if we could somehow figure out which threads should be actually recomputed (by e.g. using memoization). We compute separate indices for each thread, which is really wasteful because most of the threads can be mentioned from a lot of threads. What we can do instead is to have a single search index, from which we get the ids and then filter what threads can be mentioned from a thread.
1803–1810 ↗	(On Diff #30614)	Why do we have to do something special for `genesis` here?

This revision now requires changes to proceed.Sep 12 2023, 8:05 AM

• patryk added inline comments.Sep 19 2023, 3:08 AM

lib/shared/thread-utils.js
1780–1794 ↗	(On Diff #30614)	Right now, this code has a time complexity of `O(\|no_of_communities + no_of_first_genesis_children\| * \|threads_in_community\|)`. If we have an enormous thread store, that might be a problem. To change this code's behavior, `useChatMentionCandidatesObjAndUtils` needs to be refactored. We could add more logic to the current useMemo implementation and use a utility function that stores the previous value (e.g. a trick with useRef and useEffect) and compares objects using `JSON.stringify` if the thread store changes. We don't compute separate indices for each thread, `threadID` in `for` loop is actually a `communityThreadID`, it's confusing so renaming it to `communityThreadID` sounds like a better idea.
1803–1810 ↗	(On Diff #30614)	See the argument: `communityThreadIDForGenesisThreads[threadInfo.id]`.

Is the performance concern pointed out by @tomek going to lead to additional React render cycles? If yes, we should prioritize addressing it. cc @atul

In D8900#271640, @ashoat wrote:

Is the performance concern pointed out by @tomek going to lead to additional React render cycles? If yes, we should prioritize addressing it. cc @atul

It could, because every time something changes inside one community, we recompute every search index, instead of just the one that might be affected. But this shouldn't be a big issue - it's only one, rare render. In the solution I proposed, this issue will still be present.

So overall, my ideas were about improving the performance of an algorithm, without reducing the number of renders (significantly).

This revision is now accepted and ready to land.Sep 20 2023, 3:34 AM

Sort threads by name

Fix typo

Harbormaster completed remote builds in B22856: Diff 31439.Sep 27 2023, 7:06 AM

Harbormaster completed remote builds in B22857: Diff 31440.Sep 27 2023, 7:18 AM

• patryk requested review of this revision.Sep 28 2023, 2:00 AM

Refactor code

Harbormaster completed remote builds in B22887: Diff 31504.Sep 28 2023, 4:08 AM

tomek accepted this revision.Sep 28 2023, 5:11 AM

This revision is now accepted and ready to land.Sep 28 2023, 5:11 AM

Closed by commit rCOMMeadf8c40372f: [lib] Add chat mention SearchIndex selector (authored by • patryk). · Explain WhySep 28 2023, 5:24 AM