Page MenuHomePhabricator

[web] Show chats in mention typeahead tooltip
ClosedPublic

Authored by patryk on Aug 25 2023, 3:12 AM.
Tags
None
Referenced Files
F1865034: D8945.id.diff
Sun, May 26, 9:06 AM
Unknown Object (File)
Tue, Apr 30, 12:28 AM
Unknown Object (File)
Sun, Apr 28, 12:24 AM
Unknown Object (File)
Apr 25 2024, 3:00 PM
Unknown Object (File)
Apr 14 2024, 2:28 PM
Unknown Object (File)
Apr 8 2024, 8:14 AM
Unknown Object (File)
Mar 31 2024, 8:33 AM
Unknown Object (File)
Mar 31 2024, 8:30 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
Lint Not Applicable
Unit
Tests Not Applicable

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 ↗(On Diff #30309)

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 ↗(On Diff #30309)

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