From 18c542dc027088d25e2a142aeddb620c2e3a9520 Mon Sep 17 00:00:00 2001
From: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Date: Mon, 8 May 2023 11:26:28 +0800
Subject: [PATCH] PERF: Avoid triggering TopicTrackingState change callbacks
 unnecessarily (#21425)

What is the problem?

The TopicTrackingState is a service on the client side that is used to store
state of topics which is new or has unread posts for a given user. The state
is updated via various means and the one in concern here is whenever we load
a new topic list from the server. When a topic list is loaded from the server,
we sync this new topic list with the states in TopicTrackingState. There is also
a hard limit on the number of states that is stored by TopicTrackingState for
performance reasons and the limit is currently set to 4000. It was noticed that
once this limit has been reached, syncing a topic list with TopicTrackingState can
result in the registered state change callbacks to be called unnecessarily. This
is because during `TopicTrackingState#sync` we call `TopicTrackingState#removeTopic`
if the topic in question is neither new or unread to a user. However, `TopicTrackingState#removeTopic`
would call `TopicTrackingState#_afterStateChange` even if nothing was removed.

What is the fix?

This commit fixes the problem by checking that `TopicTrackingState#_afterStateChange` is only
called in `TopicTrackingState#removeTopic` when a topic is actually removed.
---
 .../app/models/topic-tracking-state.js        |  5 ++--
 .../unit/models/topic-tracking-state-test.js  | 27 +++++++++++++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/models/topic-tracking-state.js b/app/assets/javascripts/discourse/app/models/topic-tracking-state.js
index 39ea6edda58..82f719536de 100644
--- a/app/assets/javascripts/discourse/app/models/topic-tracking-state.js
+++ b/app/assets/javascripts/discourse/app/models/topic-tracking-state.js
@@ -397,8 +397,9 @@ const TopicTrackingState = EmberObject.extend({
    * @method removeTopic
    */
   removeTopic(topicId) {
-    this.states.delete(this._stateKey(topicId));
-    this._afterStateChange();
+    if (this.states.delete(this._stateKey(topicId))) {
+      this._afterStateChange();
+    }
   },
 
   /**
diff --git a/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js b/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js
index 3f03c156407..162ed0546e0 100644
--- a/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js
+++ b/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js
@@ -335,6 +335,12 @@ module("Unit | Model | topic-tracking-state", function (hooks) {
     trackingState.loadStates([{ topic_id: 111 }, { topic_id: 222 }]);
     trackingState.set("_trackedTopicLimit", 1);
 
+    let stateChangeCallbackCalledTimes = 0;
+
+    trackingState.onStateChange(() => {
+      stateChangeCallbackCalledTimes += 1;
+    });
+
     const list = {
       topics: [
         this.store.createRecord("topic", {
@@ -344,15 +350,36 @@ module("Unit | Model | topic-tracking-state", function (hooks) {
           unread_posts: 0,
           prevent_sync: false,
         }),
+        this.store.createRecord("topic", {
+          id: 333,
+          unseen: false,
+          seen: true,
+          unread_posts: 0,
+          prevent_sync: false,
+        }),
+        this.store.createRecord("topic", {
+          id: 444,
+          unseen: false,
+          seen: true,
+          unread_posts: 0,
+          prevent_sync: false,
+        }),
       ],
     };
 
     trackingState.sync(list, "unread");
+
     assert.notOk(
       trackingState.states.has("t111"),
       "expect state for topic 111 to be deleted"
     );
 
+    assert.equal(
+      stateChangeCallbackCalledTimes,
+      1,
+      "callback is only called once"
+    );
+
     trackingState.loadStates([{ topic_id: 111 }, { topic_id: 222 }]);
     trackingState.set("_trackedTopicLimit", 5);
     trackingState.sync(list, "unread");