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.

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}}

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,
+      })
     );
   },
 });

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")

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;
     }
   },

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

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