refactor: resolves user mentions in message text to display names#233
refactor: resolves user mentions in message text to display names#233psyberck wants to merge 2 commits intokorotovsky:masterfrom
Conversation
chriscoey
left a comment
There was a problem hiding this comment.
Nice work — the placeholder approach is clean, follows the existing URL protection pattern, and the test coverage is solid. A few suggestions:
1. Display name vs username
u.Name is the Slack username (e.g., chris.coey), but what users see in the UI is Profile.DisplayName or RealName. Consider:
name := u.Profile.DisplayName
if name == "" {
name = u.RealName
}
if name == "" {
name = u.Name
}This matches how Slack itself renders mentions.
2. Pipe-label format
Slack sometimes encodes mentions as <@U12345|display_name> (with a pipe and label). The current regex <@([UW][A-Z0-9]+)> won't match these. A small tweak:
regexp.MustCompile(`<@([UW][A-Z0-9]+)(?:\|[^>]+)?>`)3. Pre-compile regex
userMentionRegex is compiled inside filterSpecialChars on every call. Moving it to a package-level var avoids repeated compilation:
var userMentionRegex = regexp.MustCompile(`<@([UW][A-Z0-9]+)(?:\|[^>]+)?>`)(Same applies to the URL and clean regexes, but that's a pre-existing issue — not your problem to fix here.)
4. Minor: slack.User dependency in text package
Threading userMaps into filterSpecialChars adds a slack-go/slack import to the text package, which was previously independent of Slack types. An alternative would be resolving mentions in convertMessagesFromHistory before calling ProcessText, keeping the text package stateless. But the placeholder approach is well-contained and consistent with how URLs are handled, so this is a trade-off I think is acceptable.
Overall this is in good shape. The test cases are thorough — particularly liked the "normal text same to user ID" and "invalid Slack user mention format" cases. Would be great to see this merged.
|
Thanks for the thorough comments. @chriscoey:). I will update the code accordingly. |
|
Hey @chriscoey, I have refactored the implementations to resolve above concerns and feel like it's much cleaner. Let me know if there are more I can improve. Thanks! |
Summary (Close Issue #222 )
Reading messages via
conversations_history,conversations_search_messages, orconversations_replies, user mentions embedded in message text will be displayed as usernames.Implementations
ProcessTextnow delegates entirely tofilterSpecialCharsinstead of having its own mention-handling logicfilterSpecialCharsgained auserMaps map[string]slack.Userparameter — user mention resolution (<@U...> → @name) happens in a single pass alongside link/special-char processingPros
Cons
filterSpecialCharsis no longer pure: it now takes external state (userMaps), making it harder to test in isolation from user datafilterSpecialCharsremains untouchedTest plan
Let me know what you think, thanks!