Page MenuHomePhabricator

[web] Show chats in mention typeahead tooltip
ClosedPublic

Authored by patryk on Aug 25 2023, 3:12 AM.
Tags
None
Referenced Files
F3140757: D8945.id31513.diff
Sun, Nov 3, 5:14 AM
Unknown Object (File)
Sun, Oct 27, 2:04 PM
Unknown Object (File)
Sat, Oct 26, 9:02 AM
Unknown Object (File)
Sun, Oct 20, 4:18 AM
Unknown Object (File)
Fri, Oct 18, 1:54 AM
Unknown Object (File)
Sat, Oct 12, 4:56 AM
Unknown Object (File)
Oct 4 2024, 5:30 AM
Unknown Object (File)
Oct 3 2024, 5:54 AM
Subscribers

Details

Summary

Solution for ENG-4550 and ENG-4553.

This diff enables showing chats in typeahead tooltip.

Depends on D8944.

Test Plan

Test if tooltip:

  1. contains chat names
  2. doesn't contain chat in which we currently are
  3. changes its content depending on text prefix

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

patryk held this revision as a draft.
patryk retitled this revision from [web] Show chats in typeahead tooltip to [web] Show chats in mention typeahead tooltip.Aug 25 2023, 3:58 AM
patryk edited the summary of this revision. (Show Details)
patryk edited the test plan for this revision. (Show Details)
web/chat/chat-input-bar.react.js
641–676

Does it make sense to merge these two together? Something like

const suggestions = React.useMemo(() => {
  if (!typeaheadMatchedStrings) {
    return [];
  }
  const suggestedUsers = getMentionTypeaheadUserSuggestions(
    userSearchIndex,
    props.inputState.typeaheadState.frozenUserMentionsCandidates,
    viewerID,
    typeaheadMatchedStrings.textPrefix,
  );

  const suggestedChats = getMentionTypeaheadChatSuggestions(
    chatMentionSearchIndex,
    props.inputState.typeaheadState.frozenChatMentionsCandidates,
    typeaheadMatchedStrings.textPrefix,
  );

  return [...suggestedUsers, ...suggestedChats];
}, [
  typeaheadMatchedStrings,
  userSearchIndex,
  props.inputState.typeaheadState.frozenUserMentionsCandidates,
  viewerID,
  chatMentionSearchIndex,
]);

Makes sense to me, just one comment in line regarding consolidating three similar useMemo blocks into one, but feel free to disagree if it doesn't make sense

This revision is now accepted and ready to land.Aug 25 2023, 7:48 AM
web/chat/chat-input-bar.react.js
641–676

Yes, my motivation to split this code into three useMemos was that if userSearchIndex changes and chatMentionSearchIndex not, then we would recalculate userSuggestions only, but the only time userSearchIndex and chatMentionSearchIndex would change is when user switches the focused chat, so my intuition was def wrong.

web/input/input-state-container.react.js
178–179 ↗(On Diff #30622)

Why one of them is an array and one is an object?

web/utils/typeahead-utils.js
127–129 ↗(On Diff #30622)

It might make sense to extract this as a function to a more discoverable place.

web/input/input-state-container.react.js
178–179 ↗(On Diff #30622)

frozenChatMentionsCandidates is an object, because it's faster to get thread info from threadID in getMentionTypeaheadChatSuggestions. frozenUserMentionsCandidates is an array, because we handle user mentions differently in getMentionTypeaheadUserSuggestions