From 680cf443f4ca52189e44e7b9e2ab8d7dea571e5d Mon Sep 17 00:00:00 2001
From: Bianca Nenciu <nbianca@users.noreply.github.com>
Date: Mon, 18 Dec 2023 16:46:09 +0200
Subject: [PATCH] FIX: Better infinite scrolling on categories page (#24831)

This commit refactor CategoryList to remove usage of EmberObject,
hopefully make the code more readable and fixes various edge cases with
lazy loaded categories (third level subcategories not being visible,
subcategories not being visible on category page, requesting for more
pages even if the last one did not return any results, etc).

The problems have always been here, but were not visible because a lot
of the processing was handled by the server and then the result was
serialized. With more of these being moved to the client side for the
lazy category loading, the problems became more obvious.
---
 .../discovery/categories-display.gjs          |  11 +-
 .../discourse/app/models/category-list.js     | 111 ++++++++----------
 .../discourse/app/models/category.js          |   6 +-
 .../javascripts/discourse/app/models/site.js  |  10 ++
 app/models/category_list.rb                   |  10 +-
 spec/models/category_list_spec.rb             |  14 +++
 6 files changed, 97 insertions(+), 65 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/components/discovery/categories-display.gjs b/app/assets/javascripts/discourse/app/components/discovery/categories-display.gjs
index 4cccf7712bd..6a9ccd8b24a 100644
--- a/app/assets/javascripts/discourse/app/components/discovery/categories-display.gjs
+++ b/app/assets/javascripts/discourse/app/components/discovery/categories-display.gjs
@@ -75,14 +75,21 @@ export default class CategoriesDisplay extends Component {
     }
   }
 
+  get canLoadMore() {
+    return this.siteSettings.lazy_load_categories && this.args.loadMore;
+  }
+
   <template>
     <PluginOutlet
       @name="above-discovery-categories"
       @connectorTagName="div"
       @outletArgs={{hash categories=@categories topics=@topics}}
     />
-    {{#if this.siteSettings.lazy_load_categories}}
-      <LoadMore @selector=".category" @action={{@loadMore}}>
+    {{#if this.canLoadMore}}
+      <LoadMore
+        @selector=".category:not(.muted-categories *)"
+        @action={{@loadMore}}
+      >
         <this.categoriesComponent
           @categories={{@categories}}
           @topics={{@topics}}
diff --git a/app/assets/javascripts/discourse/app/models/category-list.js b/app/assets/javascripts/discourse/app/models/category-list.js
index 22c7ee205c5..2311eed3a7c 100644
--- a/app/assets/javascripts/discourse/app/models/category-list.js
+++ b/app/assets/javascripts/discourse/app/models/category-list.js
@@ -1,5 +1,4 @@
 import ArrayProxy from "@ember/array/proxy";
-import EmberObject from "@ember/object";
 import { ajax } from "discourse/lib/ajax";
 import { number } from "discourse/lib/formatter";
 import PreloadStore from "discourse/lib/preload-store";
@@ -9,83 +8,79 @@ import Topic from "discourse/models/topic";
 import { bind } from "discourse-common/utils/decorators";
 import I18n from "discourse-i18n";
 
-const MAX_CATEGORIES_LIMIT = 25;
-
 const CategoryList = ArrayProxy.extend({
   init() {
+    this.set("content", this.categories || []);
     this._super(...arguments);
-    this.set("content", []);
     this.set("page", 1);
+    this.set("fetchedLastPage", false);
   },
 
   @bind
   async loadMore() {
-    if (this.isLoading || this.lastPage) {
+    if (this.isLoading || this.fetchedLastPage) {
       return;
     }
 
     this.set("isLoading", true);
 
-    const data = { page: this.page + 1, limit: MAX_CATEGORIES_LIMIT };
-    if (this.parentCategory) {
-      data.parent_category_id = this.parentCategory.id;
-    }
+    const data = { page: this.page + 1 };
     const result = await ajax("/categories.json", { data });
+
     this.set("page", data.page);
-
-    result.category_list.categories.forEach((c) => {
-      const record = Site.current().updateCategory(c);
-      this.categories.pushObject(record);
-    });
-
+    if (result.category_list.categories.length === 0) {
+      this.set("fetchedLastPage", true);
+    }
     this.set("isLoading", false);
 
-    if (result.category_list.categories.length === 0) {
-      this.set("lastPage", true);
-    }
-
     const newCategoryList = CategoryList.categoriesFrom(this.store, result);
-    this.categories.pushObjects(newCategoryList.categories);
+    newCategoryList.forEach((c) => this.categories.pushObject(c));
   },
 });
 
 CategoryList.reopenClass({
-  categoriesFrom(store, result) {
-    const categories = CategoryList.create({ store });
-    const list = Category.list();
-
-    let statPeriod = "all";
-    const minCategories = result.category_list.categories.length * 0.66;
-
-    ["week", "month"].some((period) => {
-      const filteredCategories = result.category_list.categories.filter(
-        (c) => c[`topics_${period}`] > 0
-      );
-      if (filteredCategories.length >= minCategories) {
-        statPeriod = period;
-        return true;
-      }
-    });
+  categoriesFrom(store, result, parentCategory = null) {
+    // Find the period that is most relevant
+    const statPeriod =
+      ["week", "month"].find(
+        (period) =>
+          result.category_list.categories.filter(
+            (c) => c[`topics_${period}`] > 0
+          ).length >=
+          result.category_list.categories.length * 0.66
+      ) || "all";
 
+    // Update global category list to make sure that `findById` works as
+    // expected later
     result.category_list.categories.forEach((c) =>
-      categories.pushObject(this._buildCategoryResult(c, list, statPeriod))
+      Site.current().updateCategory(c)
     );
 
+    const categories = CategoryList.create({ store });
+    result.category_list.categories.forEach((c) => {
+      c = this._buildCategoryResult(c, statPeriod);
+      if (
+        !c.parent_category_id ||
+        c.parent_category_id === parentCategory?.id
+      ) {
+        categories.pushObject(c);
+      }
+    });
     return categories;
   },
 
-  _buildCategoryResult(c, list, statPeriod) {
+  _buildCategoryResult(c, statPeriod) {
     if (c.parent_category_id) {
-      c.parentCategory = list.findBy("id", c.parent_category_id);
+      c.parentCategory = Category.findById(c.parent_category_id);
     }
 
     if (c.subcategory_list) {
       c.subcategories = c.subcategory_list.map((subCategory) =>
-        this._buildCategoryResult(subCategory, list, statPeriod)
+        this._buildCategoryResult(subCategory, statPeriod)
       );
     } else if (c.subcategory_ids) {
-      c.subcategories = c.subcategory_ids.map((scid) =>
-        list.findBy("id", parseInt(scid, 10))
+      c.subcategories = c.subcategory_ids.map((subCategoryId) =>
+        Category.findById(parseInt(subCategoryId, 10))
       );
     }
 
@@ -99,7 +94,6 @@ CategoryList.reopenClass({
     }
 
     const stat = c[`topics_${statPeriod}`];
-
     if ((statPeriod === "week" || statPeriod === "month") && stat > 0) {
       const unit = I18n.t(`categories.topic_stat_unit.${statPeriod}`);
 
@@ -122,7 +116,7 @@ CategoryList.reopenClass({
       c.pickAll = true;
     }
 
-    if (Site.currentProp("mobileView")) {
+    if (Site.current().mobileView) {
       c.statTotal = I18n.t("categories.topic_stat_all_time", {
         count: c.topics_all_time,
         number: `<span class="value">${number(c.topics_all_time)}</span>`,
@@ -137,26 +131,25 @@ CategoryList.reopenClass({
   listForParent(store, category) {
     return ajax(
       `/categories.json?parent_category_id=${category.get("id")}`
-    ).then((result) => {
-      return EmberObject.create({
+    ).then((result) =>
+      CategoryList.create({
         store,
-        categories: this.categoriesFrom(store, result),
+        categories: this.categoriesFrom(store, result, category),
         parentCategory: category,
-      });
-    });
+      })
+    );
   },
 
   list(store) {
-    const getCategories = () => ajax("/categories.json");
-    return PreloadStore.getAndRemove("categories_list", getCategories).then(
-      (result) => {
-        return CategoryList.create({
-          store,
-          categories: this.categoriesFrom(store, result),
-          can_create_category: result.category_list.can_create_category,
-          can_create_topic: result.category_list.can_create_topic,
-        });
-      }
+    return PreloadStore.getAndRemove("categories_list", () =>
+      ajax("/categories.json")
+    ).then((result) =>
+      CategoryList.create({
+        store,
+        categories: this.categoriesFrom(store, result),
+        can_create_category: result.category_list.can_create_category,
+        can_create_topic: result.category_list.can_create_topic,
+      })
     );
   },
 });
diff --git a/app/assets/javascripts/discourse/app/models/category.js b/app/assets/javascripts/discourse/app/models/category.js
index 1308c539f42..6bd2bea61fb 100644
--- a/app/assets/javascripts/discourse/app/models/category.js
+++ b/app/assets/javascripts/discourse/app/models/category.js
@@ -78,9 +78,9 @@ const Category = RestModel.extend({
     }
   },
 
-  @discourseComputed("subcategories")
-  isParent(subcategories) {
-    return subcategories && subcategories.length > 0;
+  @discourseComputed("has_children", "subcategories")
+  isParent(hasChildren, subcategories) {
+    return hasChildren || (subcategories && subcategories.length > 0);
   },
 
   @discourseComputed("subcategories")
diff --git a/app/assets/javascripts/discourse/app/models/site.js b/app/assets/javascripts/discourse/app/models/site.js
index 9d071d1c64d..fadcda4541f 100644
--- a/app/assets/javascripts/discourse/app/models/site.js
+++ b/app/assets/javascripts/discourse/app/models/site.js
@@ -128,6 +128,16 @@ const Site = RestModel.extend({
         "parentCategory",
         this.categoriesById[newCategory.parent_category_id]
       );
+      newCategory.set(
+        "subcategories",
+        this.categories.filterBy("parent_category_id", categoryId)
+      );
+      if (newCategory.parentCategory) {
+        if (!newCategory.parentCategory.subcategories) {
+          newCategory.parentCategory.set("subcategories", []);
+        }
+        newCategory.parentCategory.subcategories.pushObject(newCategory);
+      }
       return newCategory;
     }
   },
diff --git a/app/models/category_list.rb b/app/models/category_list.rb
index 1baad99f283..6240256c265 100644
--- a/app/models/category_list.rb
+++ b/app/models/category_list.rb
@@ -156,7 +156,15 @@ class CategoryList
     notification_levels = CategoryUser.notification_levels_for(@guardian.user)
     default_notification_level = CategoryUser.default_notification_level
 
-    if @options[:parent_category_id].blank?
+    if SiteSetting.lazy_load_categories
+      subcategory_ids = {}
+      Category
+        .secured(@guardian)
+        .where(parent_category_id: @categories.map(&:id))
+        .pluck(:id, :parent_category_id)
+        .each { |id, parent_id| (subcategory_ids[parent_id] ||= []) << id }
+      @categories.each { |c| c.subcategory_ids = subcategory_ids[c.id] || [] }
+    elsif @options[:parent_category_id].blank?
       subcategory_ids = {}
       subcategory_list = {}
       to_delete = Set.new
diff --git a/spec/models/category_list_spec.rb b/spec/models/category_list_spec.rb
index ec3a46de1ed..94e33058ef8 100644
--- a/spec/models/category_list_spec.rb
+++ b/spec/models/category_list_spec.rb
@@ -374,4 +374,18 @@ RSpec.describe CategoryList do
       expect(category_list.categories[-1].custom_field_preloaded?("bob")).to be_falsey
     end
   end
+
+  describe "lazy_load_categories" do
+    fab!(:category) { Fabricate(:category, user: admin) }
+    fab!(:subcategory) { Fabricate(:category, user: admin, parent_category: category) }
+
+    before { SiteSetting.lazy_load_categories = true }
+
+    it "returns categories with subcategory_ids" do
+      expect(category_list.categories.size).to eq(3)
+      expect(
+        category_list.categories.find { |c| c.id == category.id }.subcategory_ids,
+      ).to contain_exactly(subcategory.id)
+    end
+  end
 end