FIX: Correctly highlight new topic-list-items in glimmer (#27623)

app/assets/javascripts/discourse/app/components/topic-list-item.js

@@ -337,7 +337,7 @@ export default Component.extend({
 
       this.element.classList.add("highlighted");
       this.element.setAttribute(
-        "data-islastviewedtopic",
+        "data-is-last-viewed-topic",
         opts.isLastViewedTopic
       );
       this.element.addEventListener("animationend", () => {

app/assets/javascripts/discourse/app/components/topic-list/topic-list-item.gjs

@@ -4,6 +4,7 @@ import { on } from "@ember/modifier";
 import { action } from "@ember/object";
 import didInsert from "@ember/render-modifiers/modifiers/did-insert";
 import { service } from "@ember/service";
+import { modifier } from "ember-modifier";
 import { eq, gt } from "truth-helpers";
 import PluginOutlet from "discourse/components/plugin-outlet";
 import ActionList from "discourse/components/topic-list/action-list";
@@ -41,6 +42,25 @@ export default class TopicListItem extends Component {
   @service site;
   @service siteSettings;
 
+  highlightIfNeeded = modifier((element) => {
+    if (this.args.topic.id === this.historyStore.get("lastTopicIdViewed")) {
+      element.dataset.isLastViewedTopic = true;
+
+      this.highlightRow(element).then(() =>
+        this.historyStore.delete("lastTopicIdViewed")
+      );
+
+      if (this.shouldFocusLastVisited) {
+        element.querySelector(".main-link .title")?.focus();
+      }
+    } else if (this.args.topic.get("highlight")) {
+      // highlight new topics that have been loaded from the server or the one we just created
+      this.highlightRow(element).then(() =>
+        this.args.topic.set("highlight", false)
+      );
+    }
+  });
+
   constructor() {
     super(...arguments);
 
@@ -125,30 +145,19 @@ export default class TopicListItem extends Component {
     DiscourseURL.routeTo(href || topic.url);
   }
 
-  highlight(element, isLastViewedTopic) {
+  highlightRow(element) {
-    element.classList.add("highlighted");
+    return new Promise((resolve) => {
-    element.setAttribute("data-islastviewedtopic", isLastViewedTopic);
+      element.addEventListener(
-    element.addEventListener(
+        "animationend",
-      "animationend",
+        () => {
-      () => element.classList.remove("highlighted"),
+          element.classList.remove("highlighted");
-      { once: true }
+          resolve();
-    );
+        },
+        { once: true }
+      );
 
-    if (isLastViewedTopic && this.shouldFocusLastVisited) {
+      element.classList.add("highlighted");
-      element.querySelector(".main-link .title")?.focus();
+    });
-    }
-  }
-
-  @action
-  highlightIfNeeded(element) {
-    if (this.args.topic.id === this.historyStore.get("lastTopicIdViewed")) {
-      this.historyStore.delete("lastTopicIdViewed");
-      this.highlight(element, true);
-    } else if (this.args.topic.highlight) {
-      // highlight new topics that have been loaded from the server or the one we just created
-      this.args.topic.set("highlight", false);
-      this.highlight(element, false);
-    }
   }
 
   @action
@@ -254,7 +263,7 @@ export default class TopicListItem extends Component {
     <tr
       {{! template-lint-disable no-invalid-interactive }}
       {{didInsert this.applyTitleDecorators}}
-      {{didInsert this.highlightIfNeeded}}
+      {{this.highlightIfNeeded}}
       {{on "keydown" this.keyDown}}
       {{on "click" this.click}}
       data-topic-id={{@topic.id}}

app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js

@@ -679,7 +679,7 @@ export default {
     );
     if (!selected) {
       selected = articles.find(
-        (element) => element.dataset.islastviewedtopic === "true"
+        (element) => element.dataset.isLastViewedTopic === "true"
       );
     }
 

spec/fabricators/topic_fabricator.rb

@@ -61,9 +61,7 @@ Fabricator(:read_topic, from: :topic) do
   transient :current_user
 
   before_create do |topic, transient|
-    if !transient[:current_user]
+    raise "read_topic fabricator requires the `current_user` param" if !transient[:current_user]
-      raise "new_reply_topic fabricator requires the `current_user` param"
-    end
   end
 
   after_create do |topic, transient|

spec/system/page_objects/components/topic_list.rb

@@ -30,6 +30,10 @@ module PageObjects
         page.has_no_css?(topic_list_item_class(topic))
       end
 
+      def has_highlighted_topic?(topic)
+        page.has_css?("#{topic_list_item_class(topic)}.highlighted")
+      end
+
       def has_topic_checkbox?(topic)
         page.has_css?("#{topic_list_item_class(topic)} input#bulk-select-#{topic.id}")
       end
@@ -79,6 +83,14 @@ module PageObjects
         "#{TOPIC_LIST_ITEM_SELECTOR}[data-topic-id='#{topic.id}']"
       end
 
+      def had_new_topics_alert?
+        page.has_css?(".show-more.has-topics")
+      end
+
+      def click_new_topics_alert
+        find(".show-more.has-topics").click
+      end
+
       private
 
       def topic_list_item_closed(topic)

spec/system/topic_list/glimmer_spec.rb

@@ -4,14 +4,15 @@ describe "glimmer topic list", type: :system do
   fab!(:user)
   fab!(:group) { Fabricate(:group, users: [user]) }
 
+  let(:topic_list) { PageObjects::Components::TopicList.new }
+  let(:topic_page) { PageObjects::Pages::Topic.new }
+
   before do
     SiteSetting.experimental_glimmer_topic_list_groups = group.name
     sign_in(user)
   end
 
   describe "/latest" do
-    let(:topic_list) { PageObjects::Components::TopicList.new }
-
     it "shows the list" do
       Fabricate.times(5, :topic)
       visit("/latest")
@@ -21,8 +22,6 @@ describe "glimmer topic list", type: :system do
   end
 
   describe "/new" do
-    let(:topic_list) { PageObjects::Components::TopicList.new }
-
     it "shows the list and the toggle buttons" do
       SiteSetting.experimental_new_new_view_groups = group.name
       Fabricate(:topic)
@@ -55,8 +54,6 @@ describe "glimmer topic list", type: :system do
   end
 
   describe "suggested topics" do
-    let(:topic_page) { PageObjects::Pages::Topic.new }
-
     it "shows the list" do
       topic1 = Fabricate(:post).topic
       topic2 = Fabricate(:post).topic
@@ -73,4 +70,33 @@ describe "glimmer topic list", type: :system do
       ).to eq("3")
     end
   end
+
+  describe "topic highlighting" do
+    # TODO: Those require `Capybara.disable_animation = false`
+
+    skip "highlights newly received topics" do
+      Fabricate(:read_topic, current_user: user)
+
+      visit("/latest")
+
+      new_topic = Fabricate(:post).topic
+      TopicTrackingState.publish_new(new_topic)
+
+      topic_list.had_new_topics_alert?
+      topic_list.click_new_topics_alert
+
+      topic_list.has_highlighted_topic?(new_topic)
+    end
+
+    skip "highlights the previous topic after navigation" do
+      topic = Fabricate(:read_topic, current_user: user)
+
+      visit("/latest")
+      topic_list.visit_topic(topic)
+      expect(topic_page).to have_topic_title(topic.title)
+      page.go_back
+
+      topic_list.has_highlighted_topic?(topic)
+    end
+  end
 end