Details
Tested later in the stack.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
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:
|
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. |
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 |
lib/shared/markdown.js | ||
---|---|---|
229–237 ↗ | (On Diff #30132) | Ah yeah, good call there – I agree |
lib/shared/mention-utils.js | ||
---|---|---|
39 | 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 | @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. |