Page MenuHomePhabricator

[lib] Introduce chat mention utilities for plain text
ClosedPublic

Authored by patryk on Aug 29 2023, 5:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jul 12, 11:48 AM
Unknown Object (File)
Fri, Jul 12, 11:48 AM
Unknown Object (File)
Fri, Jul 12, 11:47 AM
Unknown Object (File)
Fri, Jul 12, 11:46 AM
Unknown Object (File)
Fri, Jul 12, 11:45 AM
Unknown Object (File)
Fri, Jul 12, 11:44 AM
Unknown Object (File)
Fri, Jul 12, 10:23 AM
Unknown Object (File)
Fri, Jul 12, 10:23 AM
Subscribers

Details

Summary

This diff introduces new chat mention utilities which will be needed when handling notification text.

Depends on D8945, D8910.

Test Plan

Tested later in the stack.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

patryk held this revision as a draft.
patryk edited the test plan for this revision. (Show Details)
patryk added reviewers: tomek, inka, rohan.
lib/shared/mention-utils.js
67 ↗(On Diff #30687)

Why the beginning of the line?

97 ↗(On Diff #30687)

I don't think this is necessary. Read this. Since strings are immutable primitives, you cannot change them. You always create a new string and just change what your variable points at.

106 ↗(On Diff #30687)

What is ET` ${thread} `? What type is it even? what....

107–109 ↗(On Diff #30687)

Can you explain what this does?

lib/shared/mention-utils.js
67 ↗(On Diff #30687)
106 ↗(On Diff #30687)
// ET is a JS tag function used in template literals, eg. ET`something`
// It allows you to compose raw text and "entities" together

taken from entity-text.js. It's just an utility which can render threads uiName without using a hook. It also has other features which I encourage you to explore in entity-text.js file.

107–109 ↗(On Diff #30687)

Replace raw chat mention with current thread name when user "sees" the thread, otherwise render alternative/default text.

Remove unused utilities and introduce renderChatMentionsWithAltText

We shouldn't use entityTextToRawString directly. I'm not sure what the alternatives are here, and what the added complexity will be. Please investigate and get back to us

lib/shared/mention-utils.js
106 ↗(On Diff #30687)

entityTextToRawString does not handle ENS resolution, and as such we can't use it here. This is going to add a ton of complexity unfortunately.

You'll need to either use useEntityTextAsString (hook) or getEntityTextAsString (async function).

If you opt for a hook, it's probably easier to use useResolvedThreadInfo.

Thought about this some more... I suspect you haven't given much thought to ENS names, and to properly handle it will need updates across the whole stack, from the search index, to the display of results, to the representation of the "markdown", to the final rendering of the tag.

If it's true that you haven't given much thought to it, it would probably be best to handle this as part of ENG-2199. Can you add a comment there? Please make sure to link any prior discussion (such as this diff, and anywhere else ENS names or EntityText have been discussed for your project).

I'm no longer using entityTextToRawString since I only need to extract alternative text. Please take a look at the latest diff, 31258. Throughout the entire stack, I use the useResolvedThreadInfo hook. Also, I'm not handling ENS names in this stack; user mentions are handled as they were before. The only change was made in the search index -> however this change does affect mentioning in the input bar only. I can add a comment that the new search index needs to be refactored to support ENS names.

I'm no longer using entityTextToRawString since I only need to extract alternative text. Please take a look at the latest diff, 31258. Throughout the entire stack, I use the useResolvedThreadInfo hook.

Oops, sorry!! Thanks for clarifying, that was my bad.

Also, I'm not handling ENS names in this stack; user mentions are handled as they were before.

ENS names don't only appear to user mentions. They also appear in chat mentions. If you have a nameless chat with two friends and one of them is an ENS user, then the uiName includes both of those users' usernames, and as such it needs to be ENS-resolved.

I can add a comment that the new search index needs to be refactored to support ENS names.

That would be appreciated! It would be helpful if you could share code pointers to places that will need to be updated in order for the chat-mentioning code to support ENS resolution.

Important context in D9007 - we just want to use the alt text for now

This revision is now accepted and ready to land.Sep 22 2023, 7:43 AM

Don't forget to add a comment to ENG-2199 before landing!