Page MenuHomePhabricator

[lib/web/native] Show and search resolved ENS names in @ mentioning toolip
ClosedPublic

Authored by rohan on Dec 18 2023, 9:22 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 1:34 PM
Unknown Object (File)
Wed, Dec 18, 2:10 AM
Unknown Object (File)
Sun, Dec 8, 3:34 AM
Unknown Object (File)
Nov 8 2024, 10:02 PM
Unknown Object (File)
Nov 8 2024, 7:17 PM
Unknown Object (File)
Nov 8 2024, 7:17 PM
Unknown Object (File)
Nov 8 2024, 7:17 PM
Unknown Object (File)
Nov 8 2024, 7:17 PM
Subscribers

Details

Summary

In this diff, we modify the @ mentioning tooltip for users to display resolved ENS names and also to allow users to type out ENS names and still get a match. For example, for a user imbatman.eth, if I type @ followed by '0x', I should see imbatman.eth. I should also be able to see this if I type '@im' now.

Because of these changes, we're able to remove unused code in user-selectors.

Bolding ENS name mentions will come in the next diff

Depends on D10386

Addresses https://linear.app/comm/issue/ENG-5073/update-client-to-detect-ens-names-for-wallet-addresses

Test Plan

Please see the videos below for both web, native and native (prod build)

web:

native:

native (prod build):

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan edited the test plan for this revision. (Show Details)
rohan requested review of this revision.Dec 18 2023, 9:42 AM
native/chat/chat-input-bar.react.js
1313 ↗(On Diff #34806)

Let me know if I'm missing something, but userMentionsCandidates doesn't appear to be memoized. Won't that cause perf issues?

native/chat/chat-input-bar.react.js
1313 ↗(On Diff #34806)

I think you're right, I didn't notice that it wasn't memoized

Memoize userMentionsCandidates

You fixed it for web but not native (I actually didn't catch web – glad you did)

Separately, wonder if we just want to have a hook like useThreadChatMentionCandidates instead of memoizing the results of getUserMentionsCandidates in both places

native/chat/chat-input-bar.react.js
1266–1269 ↗(On Diff #34810)

Doesn't this need to be memoized?

You fixed it for web but not native (I actually didn't catch web – glad you did)

Oh sorry about that, I don't know what I was thinking to not do it for both even though it was called out here.

Separately, wonder if we just want to have a hook like useThreadChatMentionCandidates instead of memoizing the results of getUserMentionsCandidates in both places

Yeah that works better - though I was taking another look at getUserMentionsCandidates and I'm not sure if there's a use for memoization here. I don't think this is computationally expensive, but feel free to let me know if you disagree!

function getUserMentionsCandidates(
  threadInfo: ThreadInfo,
  parentThreadInfo: ?ThreadInfo,
): $ReadOnlyArray<RelativeMemberInfo> {
  if (threadInfo.type !== threadTypes.SIDEBAR) {
    return threadInfo.members;
  }
  if (parentThreadInfo) {
    return parentThreadInfo.members;
  }
  // This scenario should not occur unless the user logs out while looking at a
  // sidebar. In that scenario, the Redux store may be cleared before ReactNav
  // finishes transitioning away from the previous screen
  return [];
}

Memoization isn't just about avoiding the performance cost of rerunning the code. What's usually more important is avoiding regenerating non-primitives (like arrays and objects). By avoiding regenerating these, we reduce a bunch of expensive React renders.

Memoization isn't just about avoiding the performance cost of rerunning the code. What's usually more important is avoiding regenerating non-primitives (like arrays and objects). By avoiding regenerating these, we reduce a bunch of expensive React renders.

Got it, thanks for the clarification - makes perfect sense here

getUserMentionsCandidates --> useUserMentionsCandidates

inka requested changes to this revision.Dec 21 2023, 1:32 AM
inka added inline comments.
lib/shared/mention-utils.js
128

Isn't this recalculating for every user in usersInThread?

native/chat/chat-input-bar.react.js
1304–1309

According to this notion doc we want to avoid ternary conditions spanning multiple lines. Please turn this into an if with early return

1325

It there a reason why this cannot be turned into a hook just like getMentionTypeaheadUserSuggestions? If it can, can you either do it, or create a followup task?

This revision now requires changes to proceed.Dec 21 2023, 1:32 AM
rohan added inline comments.
lib/shared/mention-utils.js
128

Oh yeah thanks for catching that, I'll update that

native/chat/chat-input-bar.react.js
1325

No reason that it can't, I'll make a follow up task to avoid complicating this diff: https://linear.app/comm/issue/ENG-6278/convert-getmentiontypeaheadusersuggestions-into-a-hook

rohan marked an inline comment as done.
  1. Rewrite typeaheadMatchedStrings on both web and native to avoid ternary
  2. Prevent search index from recalculating search results for each usersInThread

Remove console.log from testing

inka added inline comments.
lib/shared/mention-utils.js
111 ↗(On Diff #34914)

Is it necessary to use the whole typeaheadMatchedStrings? It makes this code dependant on textBeforeAtSymbol, which it doesn't have to be. So this will be recalculated if the user changes the text before @
I don't think the memo is very expensive, but doesn't that result in the typeahead flickering or changing position?

This revision is now accepted and ready to land.Dec 21 2023, 10:10 AM

Address feedback

lib/shared/mention-utils.js
111 ↗(On Diff #34914)

The reason I did it like this was because typeaheadMatchedStrings?.query (whether it's an empty string or not) will always give back mention candidates. We only return an empty array if typeaheadMatchedStrings is undefined (you can see this in the removed code down below in chat-input-bar).

Since the hook needs to be run top-level and not conditionally, I couldn't only call it if typeaheadMatchedStrings is defined. So instead, I passed in typeaheadMatchedStrings to this hook and returned [] if it's not defined here.

I've not noticed any flickering / position changing. But I could change the dependency array in the useMemo to be query string instead of the entire typeaheadMatchedStrings object