DEV: better highlighting of mentions (#28403)

This commit improves the hilight-ing of mentions in posts and chat messages.

- `@here` and `@all` will generate a `<a class="mention --wide">`
- bots will generate a `<a class="mention --bot">`
- current user will generate a `<a class="mention --current">`

To achieve this change the following value transformer has been added: "mentions-class". It will be run in posts and chat messages after the mention is rendered.

A bug were bots were not considered in mentioned users has also been fixed as part of this PR.

app/assets/javascripts/discourse/app/instance-initializers/highlight-users.js

@@ -0,0 +1,21 @@
+import { withPluginApi } from "discourse/lib/plugin-api";
+
+export default {
+  initialize() {
+    withPluginApi("1.36.0", (api) => {
+      api.registerValueTransformer("mentions-class", ({ value, context }) => {
+        const { user } = context;
+
+        if (user.id < 0) {
+          value.push("--bot");
+        } else if (user.id === api.getCurrentUser()?.id) {
+          value.push("--current");
+        } else if (user.username === "here" || user.username === "all") {
+          value.push("--wide");
+        }
+
+        return value;
+      });
+    });
+  },
+};

app/assets/javascripts/discourse/app/lib/transformer/registry.js

@@ -7,6 +7,7 @@ export const VALUE_TRANSFORMERS = Object.freeze([
   // use only lowercase names
   "category-description-text",
   "category-display-name",
+  "mentions-class",
   "header-notifications-avatar-size",
   "home-logo-href",
   "home-logo-image-url",

app/assets/javascripts/discourse/app/widgets/post-cooked.js

@@ -4,6 +4,7 @@ import { isValidLink } from "discourse/lib/click-track";
 import { number } from "discourse/lib/formatter";
 import highlightHTML, { unhighlightHTML } from "discourse/lib/highlight-html";
 import highlightSearch from "discourse/lib/highlight-search";
+import { applyValueTransformer } from "discourse/lib/transformer";
 import {
   destroyUserStatusOnMentions,
   updateUserStatusOnMention,
@@ -66,16 +67,15 @@ export default class PostCooked {
     // todo should be a better way of detecting if it is composer preview
     this._isInComposerPreview = !this.decoratorHelper;
 
-    const cookedDiv = this._computeCooked();
-    this.cookedDiv = cookedDiv;
+    this.cookedDiv = this._computeCooked();
 
-    this._insertQuoteControls(cookedDiv);
-    this._showLinkCounts(cookedDiv);
-    this._applySearchHighlight(cookedDiv);
-    this._initUserStatusOnMentions();
-    this._decorateAndAdopt(cookedDiv);
+    this._insertQuoteControls(this.cookedDiv);
+    this._showLinkCounts(this.cookedDiv);
+    this._applySearchHighlight(this.cookedDiv);
+    this._decorateMentions();
+    this._decorateAndAdopt(this.cookedDiv);
 
-    return cookedDiv;
+    return this.cookedDiv;
   }
 
   destroy() {
@@ -365,9 +365,7 @@ export default class PostCooked {
 
     if (
       (this.attrs.firstPost || this.attrs.embeddedPost) &&
-      this.ignoredUsers &&
-      this.ignoredUsers.length > 0 &&
-      this.ignoredUsers.includes(this.attrs.username)
+      this.ignoredUsers?.includes?.(this.attrs.username)
     ) {
       cookedDiv.classList.add("post-ignored");
       cookedDiv.innerHTML = I18n.t("post.ignored");
@@ -378,44 +376,65 @@ export default class PostCooked {
     return cookedDiv;
   }
 
-  _initUserStatusOnMentions() {
+  _decorateMentions() {
     if (!this._isInComposerPreview) {
-      this._trackMentionedUsersStatus();
-      this._rerenderUserStatusOnMentions();
+      destroyUserStatusOnMentions();
     }
+
+    this._extractMentions().forEach(({ mentions, user }) => {
+      if (!this._isInComposerPreview) {
+        this._trackMentionedUserStatus(user);
+        this._rerenderUserStatusOnMentions(mentions, user);
+      }
+
+      const classes = applyValueTransformer("mentions-class", [], {
+        user,
+      });
+
+      mentions.forEach((mention) => {
+        mention.classList.add(...classes);
+      });
+    });
   }
 
-  _rerenderUserStatusOnMentions() {
-    destroyUserStatusOnMentions();
-    this._post()?.mentioned_users?.forEach((user) =>
-      this._rerenderUserStatusOnMention(this.cookedDiv, user)
-    );
-  }
-
-  _rerenderUserStatusOnMention(postElement, user) {
-    const href = getURL(`/u/${user.username.toLowerCase()}`);
-    const mentions = postElement.querySelectorAll(`a.mention[href="${href}"]`);
-
+  _rerenderUserStatusOnMentions(mentions, user) {
     mentions.forEach((mention) => {
       updateUserStatusOnMention(
-        getOwnerWithFallback(this._post()),
+        getOwnerWithFallback(this),
         mention,
         user.status
       );
     });
   }
 
-  _trackMentionedUsersStatus() {
-    this._post()?.mentioned_users?.forEach((user) => {
-      user.statusManager?.trackStatus?.();
-      user.on?.("status-changed", this, "_rerenderUserStatusOnMentions");
+  _rerenderUsersStatusOnMentions() {
+    this._extractMentions().forEach(({ mentions, user }) => {
+      this._rerenderUserStatusOnMentions(mentions, user);
     });
   }
 
+  _extractMentions() {
+    return (
+      this._post()?.mentioned_users?.map((user) => {
+        const href = getURL(`/u/${user.username.toLowerCase()}`);
+        const mentions = this.cookedDiv.querySelectorAll(
+          `a.mention[href="${href}"]`
+        );
+
+        return { user, mentions };
+      }) || []
+    );
+  }
+
+  _trackMentionedUserStatus(user) {
+    user.statusManager?.trackStatus?.();
+    user.on?.("status-changed", this, "_rerenderUsersStatusOnMentions");
+  }
+
   _stopTrackingMentionedUsersStatus() {
     this._post()?.mentioned_users?.forEach((user) => {
       user.statusManager?.stopTrackingStatus?.();
-      user.off?.("status-changed", this, "_rerenderUserStatusOnMentions");
+      user.off?.("status-changed", this, "_rerenderUsersStatusOnMentions");
     });
   }
 

app/assets/javascripts/discourse/tests/acceptance/transformers/mentions-class-test.js

@@ -0,0 +1,13 @@
+import { visit } from "@ember/test-helpers";
+import { test } from "qunit";
+import { acceptance } from "discourse/tests/helpers/qunit-helpers";
+
+acceptance("mentions-class transformer", function (needs) {
+  needs.user();
+
+  test("applying a value transformation", async function (assert) {
+    await visit("/t/internationalization-localization/280");
+
+    assert.dom(".mention[href='/u/eviltrout']").hasClass("--current");
+  });
+});

app/assets/javascripts/discourse/tests/fixtures/topic.js

@@ -667,6 +667,7 @@ export default {
           avatar_template: "/images/avatar.png",
           uploaded_avatar_id: 5253,
           created_at: "2013-02-07T14:02:07.869Z",
+          mentioned_users: [{id: 19, username: "eviltrout"}],
           cooked:
             '<p><aside class="quote" data-post="3" data-topic="280"><div class="title">\n<div class="quote-controls"></div>\n<img width="20" height="20" src="/user_avatar/meta.discourse.org/codinghorror/40/5297.png" class="avatar">codinghorror said:</div>\n<blockquote><p>So you could replace that lookup table with the "de" one to get German.</p></blockquote></aside></p>\n\n<p>The problem I see here is that  this file is likely two grow and change massively over the next couple months, and tracking these changes in order to keep a localized file up to date is going to be a <em>bitch</em>. </p>\n\n<p>I wonder where there is a tool that can compare two yml structures and point out which nodes are missing? That would help keep track of new strings.</p>\n\n<p>Re keeping track of <em>changed</em> strings, <a class="mention" href="/u/eviltrout">@eviltrout</a> I found this very interesting: <a href="http://stackoverflow.com/questions/4232922/why-do-people-use-plain-english-as-translation-placeholders" rel="nofollow">http://stackoverflow.com/questions/4232922/why-do-people-use-plain-english-as-translation-placeholders</a> if plain English placeholders were used, any change in strings would lead to a <em>new</em> node in the yml file, making keeping the translation up to date easier. Maybe worth thinking about in the future.</p>',
           post_number: 5,

app/assets/stylesheets/common/foundation/mixins.scss

@@ -262,6 +262,18 @@ $hpad: 0.65em;
   border-radius: 0.6em;
   text-decoration: none;
   text-wrap: nowrap;
+
+  &.--bot {
+    background: var(--highlight-low-or-medium);
+  }
+
+  &.--wide {
+    background: var(--highlight);
+  }
+
+  &.--current {
+    background: var(--tertiary-low);
+  }
 }
 
 @mixin nav-active() {

plugins/chat/assets/javascripts/discourse/components/chat-message.gjs

@@ -15,9 +15,11 @@ import { eq, not } from "truth-helpers";
 import DButton from "discourse/components/d-button";
 import concatClass from "discourse/helpers/concat-class";
 import optionalService from "discourse/lib/optional-service";
+import { applyValueTransformer } from "discourse/lib/transformer";
 import { updateUserStatusOnMention } from "discourse/lib/update-user-status-on-mention";
 import isZoomed from "discourse/lib/zoom-check";
 import discourseDebounce from "discourse-common/lib/debounce";
+import getURL from "discourse-common/lib/get-url";
 import discourseLater from "discourse-common/lib/later";
 import { bind } from "discourse-common/utils/decorators";
 import I18n from "discourse-i18n";
@@ -95,11 +97,6 @@ export default class ChatMessage extends Component {
     };
   });
 
-  constructor() {
-    super(...arguments);
-    this.initMentionedUsers();
-  }
-
   get pane() {
     return this.threadContext ? this.chatThreadPane : this.chatChannelPane;
   }
@@ -214,6 +211,8 @@ export default class ChatMessage extends Component {
   @action
   didInsertMessage(element) {
     this.messageContainer = element;
+    this.initMentionedUsers();
+    this.decorateMentions(element);
     this.debounceDecorateCookedMessage();
     this.refreshStatusOnMentions();
   }
@@ -239,6 +238,46 @@ export default class ChatMessage extends Component {
     );
   }
 
+  initMentionedUsers() {
+    this.args.message.mentionedUsers.forEach((user) => {
+      if (!user.statusManager.isTrackingStatus()) {
+        user.statusManager.trackStatus();
+        user.on("status-changed", this, "refreshStatusOnMentions");
+      }
+    });
+  }
+
+  decorateMentions(cooked) {
+    if (this.args.message.channel.allowChannelWideMentions) {
+      const wideMentions = [...cooked.querySelectorAll("span.mention")];
+      MENTION_KEYWORDS.forEach((keyword) => {
+        const mentions = wideMentions.filter((node) => {
+          return node.textContent.trim() === `@${keyword}`;
+        });
+
+        const classes = applyValueTransformer("mentions-class", [], {
+          user: { username: keyword },
+        });
+
+        mentions.forEach((mention) => {
+          mention.classList.add(...classes);
+        });
+      });
+    }
+
+    this.args.message.mentionedUsers.forEach((user) => {
+      const href = getURL(`/u/${user.username.toLowerCase()}`);
+      const mentions = cooked.querySelectorAll(`a.mention[href="${href}"]`);
+      const classes = applyValueTransformer("mentions-class", [], {
+        user,
+      });
+
+      mentions.forEach((mention) => {
+        mention.classList.add(...classes);
+      });
+    });
+  }
+
   @action
   decorateCookedMessage(message) {
     schedule("afterRender", () => {
@@ -248,18 +287,6 @@ export default class ChatMessage extends Component {
     });
   }
 
-  @action
-  initMentionedUsers() {
-    this.args.message.mentionedUsers.forEach((user) => {
-      if (user.statusManager.isTrackingStatus()) {
-        return;
-      }
-
-      user.statusManager.trackStatus();
-      user.on("status-changed", this, "refreshStatusOnMentions");
-    });
-  }
-
   get show() {
     return (
       !this.args.message?.deletedAt ||

plugins/chat/assets/javascripts/discourse/initializers/chat-setup.js

@@ -3,7 +3,6 @@ import { getOwnerWithFallback } from "discourse-common/lib/get-owner";
 import { replaceIcon } from "discourse-common/lib/icon-library";
 import { bind } from "discourse-common/utils/decorators";
 import I18n from "discourse-i18n";
-import { MENTION_KEYWORDS } from "discourse/plugins/chat/discourse/components/chat-message";
 import { clearChatComposerButtons } from "discourse/plugins/chat/discourse/lib/chat-composer-buttons";
 import ChannelHashtagType from "discourse/plugins/chat/discourse/lib/hashtag-types/channel";
 import ChatHeaderIcon from "../components/chat/header/icon";
@@ -164,24 +163,6 @@ export default {
           document.body.classList.remove("chat-drawer-active");
         }
       });
-
-      api.decorateChatMessage(function (chatMessage, chatChannel) {
-        if (!this.currentUser) {
-          return;
-        }
-
-        const highlightable = [`@${this.currentUser.username}`];
-        if (chatChannel.allowChannelWideMentions) {
-          highlightable.push(...MENTION_KEYWORDS.map((k) => `@${k}`));
-        }
-
-        chatMessage.querySelectorAll(".mention").forEach((node) => {
-          const mention = node.textContent.trim();
-          if (highlightable.includes(mention)) {
-            node.classList.add("highlighted", "valid-mention");
-          }
-        });
-      });
     });
   },
 

plugins/chat/assets/stylesheets/common/chat-message.scss

@@ -70,7 +70,8 @@
       width: 100%;
     }
 
-    a.mention {
+    a.mention,
+    span.mention.--wide {
       @include mention;
 
       &.highlighted {
@@ -79,7 +80,7 @@
     }
 
     // unlinked, invalid mention
-    span.mention {
+    span.mention:not(--wide) {
       color: var(--primary-high);
     }
 

plugins/chat/lib/chat/parsed_mentions.rb

@@ -86,7 +86,7 @@ module Chat
     end
 
     def chat_users
-      User.distinct.joins(:user_option).real.where(user_options: { chat_enabled: true })
+      User.distinct.joins(:user_option).where(user_options: { chat_enabled: true })
     end
 
     def mentionable_groups

plugins/chat/spec/lib/chat/parsed_mentions_spec.rb

@@ -145,6 +145,15 @@ RSpec.describe Chat::ParsedMentions do
       expect(result).to contain_exactly(channel_member_1.username, channel_member_2.username)
     end
 
+    it "returns bots who were mentioned directly" do
+      message = create_message("mentioning @system")
+
+      mentions = described_class.new(message)
+      result = mentions.direct_mentions.pluck(:username)
+
+      expect(result).to contain_exactly(Discourse.system_user.username)
+    end
+
     it "returns a user when self-mentioning" do
       message = create_message("Hey @#{channel_member_1.username}", user: channel_member_1)
 

plugins/chat/spec/system/chat_channel_spec.rb

@@ -176,7 +176,8 @@ RSpec.describe "Chat channel", type: :system do
       Fabricate(
         :chat_message,
         chat_channel: channel_1,
-        message: "hello @here @all @#{current_user.username} @#{other_user.username} @unexisting",
+        message:
+          "hello @here @all @#{current_user.username} @#{other_user.username} @unexisting @system",
         user: other_user,
       )
     end
@@ -191,14 +192,12 @@ RSpec.describe "Chat channel", type: :system do
     it "highlights the mentions" do
       chat_page.visit_channel(channel_1)
 
-      expect(page).to have_selector(".mention.highlighted.valid-mention", text: "@here")
-      expect(page).to have_selector(".mention.highlighted.valid-mention", text: "@all")
-      expect(page).to have_selector(
-        ".mention.highlighted.valid-mention",
-        text: "@#{current_user.username}",
-      )
+      expect(page).to have_selector(".mention.--wide", text: "@here")
+      expect(page).to have_selector(".mention.--wide", text: "@all")
+      expect(page).to have_selector(".mention.--current", text: "@#{current_user.username}")
       expect(page).to have_selector(".mention", text: "@#{other_user.username}")
       expect(page).to have_selector(".mention", text: "@unexisting")
+      expect(page).to have_selector(".mention.--bot", text: "@system")
     end
 
     it "renders user status on mentions" do