Page MenuHomePhabricator

[lib] Introduce chat mention match and parse utilities
ClosedPublic

Authored by patryk on Aug 16 2023, 5:10 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jul 18, 7:42 AM
Unknown Object (File)
Fri, Jul 12, 1:35 PM
Unknown Object (File)
Fri, Jul 12, 1:35 PM
Unknown Object (File)
Fri, Jul 12, 1:34 PM
Unknown Object (File)
Fri, Jul 12, 1:32 PM
Unknown Object (File)
Fri, Jul 12, 1:32 PM
Unknown Object (File)
Fri, Jul 12, 1:32 PM
Unknown Object (File)
Fri, Jul 12, 1:30 PM
Subscribers

Details

Summary

Solution for ENG-4557.

This diff introduces decoding utilities which are going to be used in rules.react.js files both on web and native.

Depends on D8833.

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.

Add unescapeClosingSquareBrackets function

Introduce encoding and decoding utility functions

Remove encodeChatMentionText

patryk edited the test plan for this revision. (Show Details)
patryk added reviewers: tomek, inka, rohan.
patryk edited the test plan for this revision. (Show Details)

What if thread name contains escaped characters? Are these characters escaped on render? Example: Chat \name \\

No, these characters are not escaped on render. The only character which is going to be escaped is ], because there might be a chat name like this: Chat name ]] ]] and regex would not include all of closing brackets (that's why we need decodeChatMentionText and encodeChatMentionText, so when user clicks a chat in typeahead tooltip, before raw chat mention is saved, all ] characters are escaped (\])).

lib/shared/markdown.js
226 ↗(On Diff #30070)

Regex explanation:

  • (?<!\\\\) - Negative lookbehind assertion. It asserts that what precedes the current position in the string is not a backslash (\). This is used to ensure that there is no escaping backslash before the @ symbol.
  • (@\\[\\[([0-9,|]+):(${validChatNameRegexString}?)(?<!\\\\)\\]\\]) - Main part of the regex. Inner negative lookbehind makes sure that if the user escapes ] character, whole expression is not matched. For example this expression would not match: @[[123:test\]].
229–231 ↗(On Diff #30070)

Soon I will introduce encodeChatMentionText which will encode all ] in chat mention text when chat candidate is clicked in typeahead tooltip.

Move regex and decode function to mention-utils.js

Match/parse seems good, just one comment inline that I won't block this diff on.

Could you also maybe add some unit tests for the regex in markdown.test.js alongside this diff since it's pretty difficult to review? They don't need to be super complicated but since the regex itself is pretty hard to understand, it'd be a good way to make sure all cases are accounted for

lib/shared/markdown.js
229–237 ↗(On Diff #30132)

For a relatively simple case like this, we could probably just handle it like the other simple cases directly in web/markdown/rules.react.js and native/markdown/rules.react.js using the SimpleMarkdown.inlineRegex(SharedMarkdown.chatMentionRegex) method

Going to cc @ashoat though since this was feedback he gave to me in D5346 and I'm not sure if it's worth blocking this diff for

This revision is now accepted and ready to land.Aug 21 2023, 5:32 PM
lib/shared/markdown.js
229–237 ↗(On Diff #30132)

Ah yeah, good call there – I agree

inka requested changes to this revision.Sep 6 2023, 2:37 AM
inka added inline comments.
lib/shared/mention-utils.js
39 ↗(On Diff #30684)

I don't think you want it to always be at the beginning of the line?
If I'm right please amend D9057 to test for a mention indie of a message

This revision now requires changes to proceed.Sep 6 2023, 2:37 AM
lib/shared/mention-utils.js
39 ↗(On Diff #30684)

Not sure if this is relevant, but simple-markdown always parses RegExps at the "start of the line". It process tokens at the start of the text, and then removes those tokens before processing the new set of tokens

lib/shared/mention-utils.js
39 ↗(On Diff #30684)

@ashoat comment explains why this regex is checking if mention is at the beginning of the line. D9005 modifies chatMentionRegexString and removes ^. We do sth similar for user mentions, see markdownUserMentionRegex. I can rename chatMentionRegex to markdownChatMentionRegex to distinguish "global" chat mention regex from markdown regex.

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