From 82d6420e31849a1d55414a7234492cf98ab0bda4 Mon Sep 17 00:00:00 2001
From: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Date: Tue, 4 Jul 2023 11:36:39 +0800
Subject: [PATCH] PERF: Paginate loading of tags in edit nav menu tags modal
 (#22380)

What is the problem?

Before this change, we were relying on the  `/tags` endpoint which
returned all the tags that are visible to a give user on the site leading to potential performance problems.
The attribute keys of the response also changes based on the `tags_listed_by_group` site setting.

What is the fix?

This commit fixes the problems listed above by creating a dedicate `#list` action in the
`TagsController` to handle the listing of the tags in the edit
navigation menu tags modal. This is because the `TagsController#index`
action was created specifically for the `/tags` route and the response
body does not really map well to what we need. The `TagsController#list`
action added here is also much safer since the response is paginated and
we avoid loading a whole bunch of tags upfront.
---
 .../discourse/app/adapters/list-tag.js        |   7 +
 .../edit-navigation-menu/categories-modal.hbs |   1 +
 .../sidebar/edit-navigation-menu/modal.hbs    |   2 +-
 .../edit-navigation-menu/tags-modal.hbs       |  15 +-
 .../edit-navigation-menu/tags-modal.js        |  87 ++++++-----
 .../sidebar-user-tags-section-test.js         |  10 --
 .../sidebar/edit-navigation-menu/modal.scss   |   2 +-
 .../edit-navigation-menu/tags-modal.scss      |   2 +-
 .../edit-navigation-menu/tags-modal.scss      |   2 +-
 app/controllers/tags_controller.rb            |  41 +++++
 config/routes.rb                              |   1 +
 spec/requests/tags_controller_spec.rb         | 144 ++++++++++++++++++
 .../editing_sidebar_tags_navigation_spec.rb   |  22 +--
 13 files changed, 268 insertions(+), 68 deletions(-)
 create mode 100644 app/assets/javascripts/discourse/app/adapters/list-tag.js

diff --git a/app/assets/javascripts/discourse/app/adapters/list-tag.js b/app/assets/javascripts/discourse/app/adapters/list-tag.js
new file mode 100644
index 00000000000..7f89508e908
--- /dev/null
+++ b/app/assets/javascripts/discourse/app/adapters/list-tag.js
@@ -0,0 +1,7 @@
+import RESTAdapter from "discourse/adapters/rest";
+
+export default class extends RESTAdapter {
+  pathFor(_store, _type, findArgs) {
+    return this.appendQueryParams("/tags/list", findArgs);
+  }
+}
diff --git a/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/categories-modal.hbs b/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/categories-modal.hbs
index cab218e9d0f..0f508a99d83 100644
--- a/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/categories-modal.hbs
+++ b/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/categories-modal.hbs
@@ -1,4 +1,5 @@
 <Sidebar::EditNavigationMenu::Modal
+  @class="sidebar__edit-navigation-menu__categories-modal"
   @title="sidebar.categories_form_modal.title"
   @disableSaveButton={{this.saving}}
   @save={{this.save}}
diff --git a/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/modal.hbs b/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/modal.hbs
index fbd1698befc..5a6d3242473 100644
--- a/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/modal.hbs
+++ b/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/modal.hbs
@@ -1,5 +1,5 @@
 <DModal
-  class="sidebar__edit-navigation-menu__modal"
+  class={{concat-class "sidebar__edit-navigation-menu__modal" @class}}
   @title={{i18n @title}}
   @closeModal={{@closeModal}}
 >
diff --git a/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/tags-modal.hbs b/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/tags-modal.hbs
index 9f5089c0507..1d999e98f61 100644
--- a/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/tags-modal.hbs
+++ b/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/tags-modal.hbs
@@ -1,4 +1,5 @@
 <Sidebar::EditNavigationMenu::Modal
+  @class="sidebar__edit-navigation-menu__tags-modal"
   @title="sidebar.tags_form_modal.title"
   @saving={{this.saving}}
   @save={{this.save}}
@@ -19,9 +20,13 @@
     {{#if this.tagsLoading}}
       <div class="spinner"></div>
     {{else}}
-      {{#if (gt this.filteredTags.length 0)}}
-        {{#each this.filteredTags as |tag|}}
-          <div class="sidebar-tags-form__tag" data-tag-name={{tag.name}}>
+      {{#if (gt this.tags.length 0)}}
+        {{#each this.tags as |tag|}}
+          <div
+            class="sidebar-tags-form__tag"
+            data-tag-name={{tag.name}}
+            {{did-insert this.didInsertTag}}
+          >
             <Input
               id={{concat "sidebar-tags-form__input--" tag.name}}
               class="sidebar-tags-form__input"
@@ -40,7 +45,7 @@
                 </span>
 
                 <span class="sidebar-tags-form__tag-label-count">
-                  ({{tag.count}})
+                  ({{tag.topic_count}})
                 </span>
               </p>
             </label>
@@ -53,4 +58,6 @@
       {{/if}}
     {{/if}}
   </form>
+
+  <ConditionalLoadingSpinner @condition={{this.tags.loadingMore}} />
 </Sidebar::EditNavigationMenu::Modal>
\ No newline at end of file
diff --git a/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/tags-modal.js b/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/tags-modal.js
index dd42d5d506a..ce7c0c6ac53 100644
--- a/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/tags-modal.js
+++ b/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/tags-modal.js
@@ -25,74 +25,80 @@ export default class extends Component {
   }
 
   async #loadTags() {
-    // This is loading all tags upfront and there is no pagination for it. However, this is what we are doing for the
-    // `/tags` route as well so we have decided to kick this can of worms down the road for now.
+    this.tagsLoading = true;
+
+    const findArgs = {};
+
+    if (this.filter !== "") {
+      findArgs.filter = this.filter;
+    }
+
+    if (this.onlySelected) {
+      findArgs.only_tags = this.selectedTags.join(",");
+    }
+
+    if (this.onlyUnselected) {
+      findArgs.exclude_tags = this.selectedTags.join(",");
+    }
+
     await this.store
-      .findAll("tag")
-      .then((result) => {
-        const tags = [...result.content];
-
-        if (result.extras) {
-          if (result.extras.tag_groups) {
-            result.extras.tag_groups.forEach((tagGroup) => {
-              tagGroup.tags.forEach((tag) => {
-                tags.push(tag);
-              });
-            });
-          }
-        }
-
-        this.tags = tags.sort((a, b) => {
-          return a.name.localeCompare(b.name);
-        });
-
+      .findAll("listTag", findArgs)
+      .then((tags) => {
         this.tagsLoading = false;
+        this.tags = tags;
       })
       .catch((error) => {
         popupAjaxError(error);
       });
   }
 
-  get filteredTags() {
-    return this.tags.reduce((acc, tag) => {
-      if (this.onlySelected) {
-        if (this.selectedTags.includes(tag.name) && this.#matchesFilter(tag)) {
-          acc.push(tag);
-        }
-      } else if (this.onlyUnselected) {
-        if (!this.selectedTags.includes(tag.name) && this.#matchesFilter(tag)) {
-          acc.push(tag);
-        }
-      } else if (this.#matchesFilter(tag)) {
-        acc.push(tag);
+  @action
+  didInsertTag(element) {
+    const tagName = element.dataset.tagName;
+    const lastTagName = this.tags.content[this.tags.content.length - 1].name;
+
+    if (tagName === lastTagName) {
+      if (this.observer) {
+        this.observer.disconnect();
+      } else {
+        this.observer = new IntersectionObserver(
+          (entries) => {
+            entries.forEach((entry) => {
+              if (entry.isIntersecting) {
+                this.tags.loadMore();
+              }
+            });
+          },
+          {
+            root: document.querySelector(".modal-body"),
+            threshold: 1.0,
+          }
+        );
       }
 
-      return acc;
-    }, []);
-  }
-
-  #matchesFilter(tag) {
-    return (
-      this.filter.length === 0 || tag.name.toLowerCase().includes(this.filter)
-    );
+      this.observer.observe(element);
+    }
   }
 
   @action
   resetFilter() {
     this.onlySelected = false;
     this.onlyUnselected = false;
+    this.#loadTags();
   }
 
   @action
   filterSelected() {
     this.onlySelected = true;
     this.onlyUnselected = false;
+    this.#loadTags();
   }
 
   @action
   filterUnselected() {
     this.onlySelected = false;
     this.onlyUnselected = true;
+    this.#loadTags();
   }
 
   @action
@@ -102,6 +108,7 @@ export default class extends Component {
 
   #performFiltering(filter) {
     this.filter = filter.toLowerCase();
+    this.#loadTags();
   }
 
   @action
diff --git a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-tags-section-test.js b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-tags-section-test.js
index b4a76324961..80193b5ca06 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-tags-section-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-tags-section-test.js
@@ -104,16 +104,6 @@ acceptance("Sidebar - Logged on user - Tags section", function (needs) {
     );
   });
 
-  test("clicking on section header button", async function (assert) {
-    await visit("/");
-
-    await click(
-      ".sidebar-section[data-section-name='tags'] .sidebar-section-header-button"
-    );
-
-    assert.true(exists(".sidebar-tags-form"), "it shows the tags form modal");
-  });
-
   test("tags section is displayed with site's top tags when user has not added any tags and there are no default tags configured", async function (assert) {
     updateCurrentUser({
       sidebar_tags: [],
diff --git a/app/assets/stylesheets/common/components/sidebar/edit-navigation-menu/modal.scss b/app/assets/stylesheets/common/components/sidebar/edit-navigation-menu/modal.scss
index a774429c075..d34b85d6eec 100644
--- a/app/assets/stylesheets/common/components/sidebar/edit-navigation-menu/modal.scss
+++ b/app/assets/stylesheets/common/components/sidebar/edit-navigation-menu/modal.scss
@@ -1,6 +1,6 @@
 .sidebar__edit-navigation-menu__modal {
   .modal-body {
-    min-height: 50vh;
+    min-height: 25vh;
   }
 }
 
diff --git a/app/assets/stylesheets/desktop/components/sidebar/edit-navigation-menu/tags-modal.scss b/app/assets/stylesheets/desktop/components/sidebar/edit-navigation-menu/tags-modal.scss
index 450b2443d63..d24fa57f48c 100644
--- a/app/assets/stylesheets/desktop/components/sidebar/edit-navigation-menu/tags-modal.scss
+++ b/app/assets/stylesheets/desktop/components/sidebar/edit-navigation-menu/tags-modal.scss
@@ -1,4 +1,4 @@
-.sidebar-tags-form-modal {
+.sidebar__edit-navigation-menu__tags-modal {
   .modal-inner-container {
     min-width: var(--modal-max-width);
   }
diff --git a/app/assets/stylesheets/mobile/components/sidebar/edit-navigation-menu/tags-modal.scss b/app/assets/stylesheets/mobile/components/sidebar/edit-navigation-menu/tags-modal.scss
index f9e442d70ce..33fc08abcf2 100644
--- a/app/assets/stylesheets/mobile/components/sidebar/edit-navigation-menu/tags-modal.scss
+++ b/app/assets/stylesheets/mobile/components/sidebar/edit-navigation-menu/tags-modal.scss
@@ -1,4 +1,4 @@
-.sidebar-tags-form-modal {
+.sidebar__edit-navigation-menu__tags-modal {
   .modal-inner-container {
     width: 35em;
   }
diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb
index 12d58237a11..2e2f9827207 100644
--- a/app/controllers/tags_controller.rb
+++ b/app/controllers/tags_controller.rb
@@ -27,6 +27,7 @@ class TagsController < ::ApplicationController
                   update_notifications
                   personal_messages
                   info
+                  list
                 ]
 
   before_action :fetch_tag, only: %i[info create_synonyms destroy_synonym]
@@ -99,6 +100,46 @@ class TagsController < ::ApplicationController
     end
   end
 
+  LIST_LIMIT = 51
+
+  def list
+    offset = params[:offset].to_i || 0
+    tags = guardian.can_admin_tags? ? Tag.all : Tag.used_tags_in_regular_topics(guardian)
+
+    load_more_query_params = { offset: offset + 1 }
+
+    if filter = params[:filter]
+      tags = tags.where("LOWER(tags.name) ILIKE ?", "%#{filter.downcase}%")
+      load_more_query_params[:filter] = filter
+    end
+
+    if only_tags = params[:only_tags]
+      tags = tags.where("LOWER(tags.name) IN (?)", only_tags.split(",").map(&:downcase))
+      load_more_query_params[:only_tags] = only_tags
+    end
+
+    if exclude_tags = params[:exclude_tags]
+      tags = tags.where("LOWER(tags.name) NOT IN (?)", exclude_tags.split(",").map(&:downcase))
+      load_more_query_params[:exclude_tags] = exclude_tags
+    end
+
+    tags_count = tags.count
+    tags = tags.order("LOWER(tags.name) ASC").limit(LIST_LIMIT).offset(offset * LIST_LIMIT)
+
+    load_more_url = URI("/tags/list.json")
+    load_more_url.query = URI.encode_www_form(load_more_query_params)
+
+    render_serialized(
+      tags,
+      TagSerializer,
+      root: "list_tags",
+      meta: {
+        total_rows_list_tags: tags_count,
+        load_more_list_tags: load_more_url.to_s,
+      },
+    )
+  end
+
   Discourse.filters.each do |filter|
     define_method("show_#{filter}") do
       @tag_id = params[:tag_id].force_encoding("UTF-8")
diff --git a/config/routes.rb b/config/routes.rb
index 1d40c3b9abb..14e91fcdb39 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -1480,6 +1480,7 @@ Discourse::Application.routes.draw do
       get "/" => "tags#index"
       get "/filter/list" => "tags#index"
       get "/filter/search" => "tags#search"
+      get "/list" => "tags#list"
       get "/personal_messages/:username" => "tags#personal_messages",
           :constraints => {
             username: RouteFormat.username,
diff --git a/spec/requests/tags_controller_spec.rb b/spec/requests/tags_controller_spec.rb
index 238e6cae103..979017cff48 100644
--- a/spec/requests/tags_controller_spec.rb
+++ b/spec/requests/tags_controller_spec.rb
@@ -1441,4 +1441,148 @@ RSpec.describe TagsController do
       expect(tag_user.notification_level).to eq(NotificationLevels.all[:muted])
     end
   end
+
+  describe "#list" do
+    fab!(:tag3) do
+      Fabricate(:tag, name: "tag3").tap { |tag| Fabricate.times(1, :topic, tags: [tag]) }
+    end
+
+    fab!(:tag2) do
+      Fabricate(:tag, name: "tag2").tap { |tag| Fabricate.times(1, :topic, tags: [tag]) }
+    end
+
+    fab!(:tag1) do
+      Fabricate(:tag, name: "tag").tap { |tag| Fabricate.times(1, :topic, tags: [tag]) }
+    end
+
+    fab!(:tag_not_used_in_topics) { Fabricate(:tag, name: "tag4") }
+
+    it "should return 403 for an anonymous user" do
+      get "/tags/list.json"
+
+      expect(response.status).to eq(403)
+    end
+
+    it "should return 404 when tagging is disabled" do
+      SiteSetting.tagging_enabled = false
+
+      sign_in(user)
+
+      get "/tags/list.json"
+
+      expect(response.status).to eq(404)
+    end
+
+    it "should only return tags used in topics for non admin users" do
+      stub_const(TagsController, "LIST_LIMIT", 2) do
+        sign_in(user)
+
+        get "/tags/list.json"
+
+        expect(response.status).to eq(200)
+
+        expect(response.parsed_body["list_tags"].map { |tag| tag["name"] }).to eq(
+          [tag1.name, tag2.name],
+        )
+
+        expect(response.parsed_body["meta"]["total_rows_list_tags"]).to eq(3)
+        expect(response.parsed_body["meta"]["load_more_list_tags"]).to eq(
+          "/tags/list.json?offset=1",
+        )
+
+        get response.parsed_body["meta"]["load_more_list_tags"]
+
+        expect(response.status).to eq(200)
+        expect(response.parsed_body["list_tags"].map { |tag| tag["name"] }).to eq([tag3.name])
+        expect(response.parsed_body["meta"]["total_rows_list_tags"]).to eq(3)
+
+        expect(response.parsed_body["meta"]["load_more_list_tags"]).to eq(
+          "/tags/list.json?offset=2",
+        )
+      end
+    end
+
+    it "should return all tags for admin users" do
+      stub_const(TagsController, "LIST_LIMIT", 2) do
+        sign_in(admin)
+
+        get "/tags/list.json"
+
+        expect(response.status).to eq(200)
+
+        expect(response.parsed_body["list_tags"].map { |tag| tag["name"] }).to eq(
+          [tag1.name, tag2.name],
+        )
+
+        expect(response.parsed_body["meta"]["total_rows_list_tags"]).to eq(4)
+
+        expect(response.parsed_body["meta"]["load_more_list_tags"]).to eq(
+          "/tags/list.json?offset=1",
+        )
+
+        get response.parsed_body["meta"]["load_more_list_tags"]
+
+        expect(response.status).to eq(200)
+
+        expect(response.parsed_body["list_tags"].map { |tag| tag["name"] }).to eq(
+          [tag3.name, tag_not_used_in_topics.name],
+        )
+
+        expect(response.parsed_body["meta"]["total_rows_list_tags"]).to eq(4)
+
+        expect(response.parsed_body["meta"]["load_more_list_tags"]).to eq(
+          "/tags/list.json?offset=2",
+        )
+      end
+    end
+
+    it "accepts a `filter` param and filters the tags by tag name" do
+      sign_in(user)
+
+      get "/tags/list.json", params: { filter: "3" }
+
+      expect(response.status).to eq(200)
+
+      expect(response.parsed_body["list_tags"].map { |tag| tag["name"] }).to eq([tag3.name])
+      expect(response.parsed_body["meta"]["total_rows_list_tags"]).to eq(1)
+
+      expect(response.parsed_body["meta"]["load_more_list_tags"]).to eq(
+        "/tags/list.json?offset=1&filter=3",
+      )
+    end
+
+    it "accepts a `only_tags` param and filters the tags by the given tags" do
+      sign_in(user)
+
+      get "/tags/list.json", params: { only_tags: "#{tag1.name},#{tag3.name}" }
+
+      expect(response.status).to eq(200)
+
+      expect(response.parsed_body["list_tags"].map { |tag| tag["name"] }).to eq(
+        [tag1.name, tag3.name],
+      )
+
+      expect(response.parsed_body["meta"]["total_rows_list_tags"]).to eq(2)
+
+      expect(response.parsed_body["meta"]["load_more_list_tags"]).to eq(
+        "/tags/list.json?offset=1&only_tags=#{tag1.name}%2C#{tag3.name}",
+      )
+    end
+
+    it "accepts a `exclude_tags` params and filters tags excluding the given tags" do
+      sign_in(user)
+
+      get "/tags/list.json", params: { exclude_tags: "#{tag1.name},#{tag3.name}" }
+
+      expect(response.status).to eq(200)
+
+      expect(response.parsed_body["list_tags"].map { |tag| tag["name"] }).to eq([tag2.name])
+
+      expect(response.parsed_body["meta"]["total_rows_list_tags"]).to eq(1)
+
+      expect(response.parsed_body["meta"]["load_more_list_tags"]).to eq(
+        "/tags/list.json?offset=1&exclude_tags=#{tag1.name}%2C#{tag3.name}",
+      )
+    end
+  end
 end
diff --git a/spec/system/editing_sidebar_tags_navigation_spec.rb b/spec/system/editing_sidebar_tags_navigation_spec.rb
index 973719055ad..3111dc8c922 100644
--- a/spec/system/editing_sidebar_tags_navigation_spec.rb
+++ b/spec/system/editing_sidebar_tags_navigation_spec.rb
@@ -88,16 +88,6 @@ RSpec.describe "Editing sidebar tags navigation", type: :system do
     include_examples "a user can edit the sidebar tags navigation", true
   end
 
-  it "displays the all tags in the modal when `tags_listed_by_group` site setting is true" do
-    SiteSetting.tags_listed_by_group = true
-
-    visit "/latest"
-
-    modal = sidebar.click_edit_tags_button
-
-    expect(modal).to have_tag_checkboxes([tag1, tag2, tag3, tag4])
-  end
-
   it "allows a user to filter the tags in the modal by the tag's name" do
     visit "/latest"
 
@@ -186,4 +176,16 @@ RSpec.describe "Editing sidebar tags navigation", type: :system do
 
     expect(modal).to have_tag_checkboxes([tag1, tag2, tag3, tag4])
   end
+
+  it "loads more tags when the user scrolls views the last tag in the modal and there is more tags to load" do
+    stub_const(TagsController, "LIST_LIMIT", 2) do
+      visit "/latest"
+
+      expect(sidebar).to have_tags_section
+
+      modal = sidebar.click_edit_tags_button
+
+      expect(modal).to have_tag_checkboxes([tag1, tag2, tag3, tag4])
+    end
+  end
 end