Page MenuHomePhabricator
Differential D10460

[lib/web/native] Update ChatMentionCandidates type to include a rawChatName
ClosedPublic
Actions

Authored by rohan on Dec 26 2023, 9:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 31, 1:23 AM
Unknown Object (File)
Thu, Oct 31, 1:23 AM
Unknown Object (File)
Thu, Oct 31, 1:23 AM
Unknown Object (File)
Thu, Oct 31, 1:23 AM
Unknown Object (File)
Thu, Oct 31, 1:23 AM
Unknown Object (File)
Thu, Oct 31, 1:23 AM
Unknown Object (File)
Thu, Oct 31, 1:23 AM
Unknown Object (File)
Oct 3 2024, 2:26 PM
View All 88 Files
Subscribers
ashoat
tomek

Details

Reviewers
atul
ginsu
inka
Commits
rCOMM9e834fe59da5: [lib/web/native] Update ChatMentionCandidates type to include a rawChatName
Summary

When a user adds some ENS users to a chat, then clears the name, we can @-mention the chat by their ENS names (since we use the thread name from the ResolvedThread when constructing the search index for chat mentions).

The way we can allow people to @-mention this chat by the ENS users' wallet addresses (i.e. @0x should match the chat) is by including the raw chat name (before it's resolved) in our chat mention candidates object. This diff makes the necessary type updates and changes to deal with Flow. The next diff will utilize this new rawChatName and handle the entity text resolution (if necessary) when creating the chat mention search index.

Part of ENG-5436

Depends on D10459

Test Plan

Ran flow and manually tested @-mentioning to make sure no React errors logged in the console and that everythig still worked as expected.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan created this revision.Dec 26 2023, 9:58 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptDec 26 2023, 9:58 AM
rohan added a child revision: D10461: [lib] Include the rawChatName when constructing the chat mention search index.Dec 26 2023, 10:02 AM
rohan added inline comments.Dec 26 2023, 10:05 AM
lib/components/chat-mention-provider.react.js
196 ↗(On Diff #35007)

I need to figure out one Flow issue here

rohan updated this revision to Diff 35009.Dec 26 2023, 10:27 AM
  1. Remove $FlowFixMe
  2. Fix TypeError: Cannot read property 'rawChatName' of undefined in markdown.js
Harbormaster completed remote builds in B25338: Diff 35007.Dec 26 2023, 10:30 AM
Harbormaster completed remote builds in B25340: Diff 35009.Dec 26 2023, 11:01 AM
rohan requested review of this revision.Dec 26 2023, 11:01 AM
ashoat added a comment.Dec 26 2023, 11:16 AM

Just wondering, are we supporting something like "@userone.eth and 0x12312321" matching a chat whose ENS-resolved name is "userone.eth and usertwo.eth"?

rohan added a comment.Dec 26 2023, 11:54 AM
In D10460#302348, @ashoat wrote:

Just wondering, are we supporting something like "@userone.eth and 0x12312321" matching a chat whose ENS-resolved name is "userone.eth and usertwo.eth"?

With this stack, it's one or the other. So @userone.eth and usertwo.eth or @0x1831 and 0x384731.

If we wanted to allow mixing, some changes to how we either add entries to the search index or how we construct the search index for chat mentions will probably need to be made

ashoat added a comment.Dec 26 2023, 12:44 PM

Makes sense. I think that's okay for now. It's good to at least continue supporting the old format for continuity, but I imagine in the future people will mostly start using the ENS-resolved format

Will let one of the actual reviewers review this one as I'm not as familiar with the callsites

lib/components/chat-mention-provider.react.js
83 ↗(On Diff #35009)

It looks like we never mutate the inner object, so this change should be possible

inka added a comment.Dec 28 2023, 4:23 AM

This code is quite complex, there are many indirect calls here. But from what I can tell it seems that the changes in typeachead-utils [web] change the shape of actions returned from getMentionTypeaheadTooltipActions. The actionButtonContent filed seems to be only used by getMentionTypeaheadTooltipButtons. But you don't seem to be updating getMentionTypeaheadTooltipButtons in the next diff, that says is the last diff of this stack. Am I missing something?
Same for native, just that the things are called differently

Whereas the data returned from useMentionTypeaheadChatSuggestions seems to be only consumed by mentionTypeaheadTooltipActions/getMentionTypeaheadTooltipActions which you also don't change.

I understand the changes to getChatMentionCandidates / useChatMentionCandidatesObjAndUtils, because in the next diff you change useChatMentionSearchIndex, which is being passed data from useChatMentionCandidatesObjAndUtils. But what are the other changes for?

lib/types/thread-types.js
493 ↗(On Diff #35009)

Can we extract and name this type? Like ChatMentionCandiate or something. it's used in many places and I feel it would make it more readable
And have a field with this type in MentionTypeaheadChatSuggestionItem instead of threadInfo and rawChatName separately.
Then the change to parseChatMention will not be necessary

rohan updated this revision to Diff 35050.Dec 28 2023, 9:00 AM

Address feedback (will respond to comments separately). Mainly factored out individual types and updated the code to reflect that

Harbormaster completed remote builds in B25371: Diff 35050.Dec 28 2023, 9:23 AM
inka added inline comments.Dec 28 2023, 10:10 AM
native/chat/mention-typeahead-tooltip-button.react.js
31–32 ↗(On Diff #35050)

Nit

web/utils/typeahead-utils.js
201–202 ↗(On Diff #35050)

Nit

rohan added a comment.Dec 28 2023, 10:57 AM
In D10460#302669, @inka wrote:

I understand the changes to getChatMentionCandidates / useChatMentionCandidatesObjAndUtils, because in the next diff you change useChatMentionSearchIndex, which is being passed data from useChatMentionCandidatesObjAndUtils. But what are the other changes for?

I read through your comment and it makes sense to me. I took a second look at my changes and realized I was mistakenly passing rawChatName around the codebase even though the type didn't require it (I previously did it based on some old changes where flow thought it had to be included everywhere).

After the most recent diff update where I fixed the types, I've been able to revert a lot of the unnecessary changes. Thanks for calling it out - I'll update it now

rohan updated this revision to Diff 35058.Dec 28 2023, 10:57 AM

Address feedback on removing unnecessary changes

rohan added inline comments.Dec 28 2023, 10:59 AM
lib/shared/markdown.js
292–293 ↗(On Diff #35058)

This change is required because the type ChatMentionCandidate is an object with both threadInfo and rawChatName. We cannot destructure as that leads to the app crashing if, for instance, chatMentionCandidates[capture[3]]; is undefined

lib/shared/mention-utils.js
158 ↗(On Diff #35058)

Same comment as above - this change is necessary since ChatMentionCandidate is now an object type that contains both threadInfo and rawChatName

Harbormaster completed remote builds in B25379: Diff 35058.Dec 28 2023, 11:30 AM
inka accepted this revision.Dec 29 2023, 1:55 AM
This revision is now accepted and ready to land.Dec 29 2023, 1:55 AM
Closed by commit rCOMM9e834fe59da5: [lib/web/native] Update ChatMentionCandidates type to include a rawChatName (authored by rohan). · Explain WhyDec 29 2023, 8:14 AM
This revision was automatically updated to reflect the committed changes.
rohan added a commit: rCOMM9e834fe59da5: [lib/web/native] Update ChatMentionCandidates type to include a rawChatName.

Revision Contents
Changeset List

PathSize
lib/
components/
42 lines
shared/
3 lines
2 lines
types/
6 lines

Diff 35094

View Options

lib/components/chat-mention-provider.react.js

Loading...
View Options

lib/shared/markdown.js

Loading...
View Options

lib/shared/mention-utils.js

Loading...
View Options

lib/types/thread-types.js

Loading...