DEV: More efficiently trigger topic tracking state on state change (#16952)

* When loading topics in bulk, only trigger state change callbacks after
all the topics have been loaded and we determine that state has actually
changed.

* State change callbacks are also only triggered when state has changed.

The use of JSON.stringify might raise some performance concerns here as this is a
performance sensitive codepath. However, I measured the time for each
`_setState` function call locally, by wrapping the function call with
`performance.now()`, and did not see any significant overhead.

app/assets/javascripts/discourse/app/models/topic-tracking-state.js

@@ -423,7 +423,17 @@ const TopicTrackingState = EmberObject.extend({
 
     // make sure all the state is up to date with what is accurate
     // from the server
-    list.topics.forEach(this._syncStateFromListTopic);
+    const newStates = [];
+
+    for (const topic of list.topics) {
+      const newState = this._newStateFromListTopic(topic);
+
+      if (newState) {
+        newStates.push(newState);
+      }
+    }
+
+    this.loadStates(newStates);
 
     // correct missing states, safeguard in case message bus is corrupt
     if (this._shouldCompensateState(list, filter, queryParams)) {
@@ -610,14 +620,38 @@ const TopicTrackingState = EmberObject.extend({
   },
 
   loadStates(data) {
-    (data || []).forEach((topic) => {
-      this.modifyState(topic, topic);
+    if (!data || data.length === 0) {
+      return;
+    }
+
+    const modified = data.every((topic) => {
+      return this._setState({ topic, data: topic, skipAfterStateChange: true });
     });
+
+    if (modified) {
+      this._afterStateChange();
+    }
+  },
+
+  _setState({ topic, data, skipAfterStateChange }) {
+    const stateKey = this._stateKey(topic);
+    const oldState = this.states.get(stateKey);
+
+    if (!oldState || JSON.stringify(oldState) !== JSON.stringify(data)) {
+      this.states.set(stateKey, data);
+
+      if (!skipAfterStateChange) {
+        this._afterStateChange();
+      }
+
+      return true;
+    } else {
+      return false;
+    }
   },
 
   modifyState(topic, data) {
-    this.states.set(this._stateKey(topic), data);
-    this._afterStateChange();
+    this._setState({ topic, data });
   },
 
   modifyStateProp(topic, prop, data) {
@@ -661,16 +695,13 @@ const TopicTrackingState = EmberObject.extend({
   // topic from the list (e.g. updates category, highest read post
   // number, tags etc.)
   @bind
-  _syncStateFromListTopic(topic) {
+  _newStateFromListTopic(topic) {
     const state = this.findState(topic.id) || {};
 
     // make a new copy so we aren't modifying the state object directly while
     // we make changes
     const newState = { ...state };
 
-    newState.topic_id = topic.id;
-    newState.notification_level = topic.notification_level;
-
     // see ListableTopicSerializer for unread_posts/unseen and other
     // topic property logic
     if (topic.unseen) {
@@ -687,7 +718,16 @@ const TopicTrackingState = EmberObject.extend({
       return;
     }
 
-    newState.highest_post_number = topic.highest_post_number;
+    newState.topic_id = topic.id;
+
+    if (topic.notification_level) {
+      newState.notification_level = topic.notification_level;
+    }
+
+    if (topic.highest_post_number) {
+      newState.highest_post_number = topic.highest_post_number;
+    }
+
     if (topic.category) {
       newState.category_id = topic.category.id;
     }
@@ -696,7 +736,7 @@ const TopicTrackingState = EmberObject.extend({
       newState.tags = topic.tags;
     }
 
-    this.modifyState(topic.id, newState);
+    return newState;
   },
 
   // this stops sync of tracking state when list is filtered, in the past this
@@ -791,12 +831,12 @@ const TopicTrackingState = EmberObject.extend({
       }
     }
 
-    const old = this.findState(data);
+    const old = { ...this.findState(data) };
 
     if (data.message_type === "latest") {
       this.notifyIncoming(data);
 
-      if ((old && old.tags) !== data.payload.tags) {
+      if (old.tags !== data.payload.tags) {
         this.modifyStateProp(data, "tags", data.payload.tags);
         this.incrementMessageCount();
       }
@@ -814,7 +854,6 @@ const TopicTrackingState = EmberObject.extend({
         // we have, and then substitute inferred values for last_read_post_number
         // and notification_level. Any errors will be corrected when a
         // topic-list is loaded which includes the topic.
-
         let payload = data.payload;
 
         if (old) {

app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js

@@ -23,6 +23,23 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) {
     this.clock.restore();
   });
 
+  test("bulk loading states only calls onStateChange callback once", function (assert) {
+    const trackingState = TopicTrackingState.create();
+    let stateCallbackCalled = 0;
+
+    trackingState.onStateChange(() => {
+      stateCallbackCalled += 1;
+    });
+
+    trackingState.loadStates([
+      { topic_id: 1 },
+      { topic_id: 2 },
+      { topic_id: 3 },
+    ]);
+
+    assert.strictEqual(stateCallbackCalled, 1, "callback is only called once");
+  });
+
   test("tag counts", function (assert) {
     const trackingState = TopicTrackingState.create();
 
@@ -338,13 +355,54 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) {
     );
   });
 
+  test("sync - no changes to state", function (assert) {
+    const trackingState = TopicTrackingState.create();
+
+    trackingState.loadStates([
+      { topic_id: 111, last_read_post_number: null },
+      { topic_id: 222, last_read_post_number: null },
+    ]);
+
+    let stateCallbackCalled = 0;
+
+    trackingState.onStateChange(() => {
+      stateCallbackCalled += 1;
+    });
+
+    const list = {
+      topics: [
+        Topic.create({
+          id: 111,
+          last_read_post_number: null,
+          unseen: true,
+        }),
+        Topic.create({
+          id: 222,
+          last_read_post_number: null,
+          unseen: true,
+        }),
+      ],
+    };
+
+    trackingState.sync(list, "unread");
+
+    assert.strictEqual(stateCallbackCalled, 0, "callback is not called");
+  });
+
   test("sync - updates state to match list topic for unseen and unread/new topics", function (assert) {
     const trackingState = TopicTrackingState.create();
+
     trackingState.loadStates([
       { topic_id: 111, last_read_post_number: 0 },
       { topic_id: 222, last_read_post_number: 1 },
     ]);
 
+    let stateCallbackCalled = 0;
+
+    trackingState.onStateChange(() => {
+      stateCallbackCalled += 1;
+    });
+
     const list = {
       topics: [
         Topic.create({
@@ -385,6 +443,8 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) {
       17,
       "last_read_post_number is highest_post_number - (unread + new)"
     );
+
+    assert.strictEqual(stateCallbackCalled, 1, "callback is only called once");
   });
 
   test("sync - states missing from the topic list are updated based on the selected filter", function (assert) {
@@ -476,11 +536,14 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) {
       });
 
       test("state is modified and callback is called", function (assert) {
-        let stateCallbackCalled = false;
+        let stateCallbackCalled = 0;
+
         trackingState.onStateChange(() => {
-          stateCallbackCalled = true;
+          stateCallbackCalled += 1;
         });
+
         publishToMessageBus(`/unread`, unreadTopicPayload);
+
         assert.deepEqual(
           trackingState.findState(111),
           {
@@ -496,9 +559,10 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) {
           },
           "topic state updated"
         );
+
         assert.strictEqual(
           stateCallbackCalled,
-          true,
+          1,
           "state change callback called"
         );
       });
@@ -597,6 +661,7 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) {
           message_type: "dismiss_new",
           payload: { topic_ids: [112] },
         });
+
         assert.strictEqual(trackingState.findState(112).is_seen, true);
       });