DEV: Use components to manage custom sidebar sections lifecycle (#21574)

What is the problem?

Previously the `sections` getter was initializing duplicate `lib/sidebar/(community-)section` instances every time it was evaluated. This change in identity was causing Ember's `{{#each` helper to totally rerender every section whenever the getter was evaluated.

What is the fix?

This commit refactors things to lean on Ember's components for state/lifecycle management. The `{{#each` loop is done over the source data, which is guaranteed to only change identity when there is a real config change. Individual section components are initialized for each section, and are responsible for constructing and tearing down their own `lib/sidebar/(community-)section` instances.

This commit also updates `lib/sidebar/(community-)section` to support service injection rather than passing service references around.

Co-authored-by: David Taylor <david@taylorhq.com>

app/assets/javascripts/discourse/app/components/sidebar/anonymous/custom-sections.hbs

@@ -1,53 +0,0 @@
-<div class="sidebar-custom-sections">
-  {{#each this.sections as |section|}}
-    <Sidebar::Section
-      @sectionName={{section.slug}}
-      @headerLinkText={{section.decoratedTitle}}
-      @collapsable={{true}}
-    >
-
-      {{#if section.displayShortSiteDescription}}
-        <Sidebar::SectionMessage>
-          {{section.siteSettings.short_site_description}}
-        </Sidebar::SectionMessage>
-      {{/if}}
-
-      {{#each section.links as |link|}}
-        {{#if link.shouldDisplay}}
-          {{#if link.external}}
-            <Sidebar::SectionLink
-              @linkName={{link.name}}
-              @content={{replace-emoji link.text}}
-              @prefixType="icon"
-              @prefixValue={{link.prefixValue}}
-              @href={{link.value}}
-            />
-          {{else}}
-            <Sidebar::SectionLink
-              @shouldDisplay={{link.shouldDisplay}}
-              @href={{link.href}}
-              @title={{link.title}}
-              @currentWhen={{link.currentWhen}}
-              @badgeText={{link.badgeText}}
-              @suffixCSSClass={{link.suffixCSSClass}}
-              @suffixValue={{link.suffixValue}}
-              @suffixType={{link.suffixType}}
-              @linkName={{link.name}}
-              @route={{link.route}}
-              @model={{link.model}}
-              @models={{link.models}}
-              @query={{link.query}}
-              @content={{replace-emoji link.text}}
-              @prefixType="icon"
-              @prefixValue={{link.prefixValue}}
-            />
-          {{/if}}
-        {{/if}}
-      {{/each}}
-
-      {{#if section.moreLinks}}
-        <Sidebar::MoreSectionLinks @sectionLinks={{section.moreLinks}} />
-      {{/if}}
-    </Sidebar::Section>
-  {{/each}}
-</div>

app/assets/javascripts/discourse/app/components/sidebar/anonymous/custom-sections.js

@@ -1,30 +1,5 @@
-import Component from "@glimmer/component";
-import { inject as service } from "@ember/service";
-import Section from "discourse/lib/sidebar/section";
-import CommunitySection from "discourse/lib/sidebar/community-section";
+import SidebarCustomSection from "discourse/components/sidebar/common/custom-sections";
 
-export default class SidebarAnonymousCustomSections extends Component {
-  @service router;
-  @service site;
-  @service siteSettings;
-
-  get sections() {
-    return this.site.anonymous_sidebar_sections?.map((section) => {
-      let klass;
-      switch (section.section_type) {
-        case "community":
-          klass = CommunitySection;
-          break;
-        default:
-          klass = Section;
-      }
-
-      return new klass({
-        section,
-        currentUser: this.currentUser,
-        router: this.router,
-        siteSettings: this.siteSettings,
-      });
-    });
-  }
+export default class SidebarAnonymousCustomSections extends SidebarCustomSection {
+  anonymous = true;
 }

app/assets/javascripts/discourse/app/components/sidebar/common/custom-section.hbs

@@ -0,0 +1,74 @@
+<Sidebar::Section
+  @sectionName={{this.section.slug}}
+  @headerLinkText={{this.section.decoratedTitle}}
+  @collapsable={{true}}
+  @headerActions={{this.section.headerActions}}
+  @headerActionsIcon={{this.section.headerActionIcon}}
+  @class={{this.section.dragCss}}
+>
+
+  {{#if this.section.displayShortSiteDescription}}
+    <Sidebar::SectionMessage>
+      {{this.siteSettings.short_site_description}}
+    </Sidebar::SectionMessage>
+  {{/if}}
+
+  {{#each this.section.links as |link|}}
+    {{#if link.shouldDisplay}}
+      {{#if link.external}}
+        <Sidebar::SectionLink
+          @linkName={{link.name}}
+          @content={{replace-emoji link.text}}
+          @prefixType="icon"
+          @prefixValue={{link.prefixValue}}
+          @href={{link.value}}
+          @class={{link.linkDragCss}}
+          {{draggable
+            didStartDrag=link.didStartDrag
+            didEndDrag=link.didEndDrag
+            dragMove=link.dragMove
+          }}
+        />
+      {{else}}
+        <Sidebar::SectionLink
+          @shouldDisplay={{link.shouldDisplay}}
+          @href={{link.href}}
+          @title={{link.title}}
+          @linkName={{link.name}}
+          @route={{link.route}}
+          @model={{link.model}}
+          @models={{link.models}}
+          @query={{link.query}}
+          @content={{replace-emoji link.text}}
+          @badgeText={{link.badgeText}}
+          @prefixType="icon"
+          @prefixValue={{link.prefixValue}}
+          @suffixCSSClass={{link.suffixCSSClass}}
+          @suffixValue={{link.suffixValue}}
+          @suffixType={{link.suffixType}}
+          @currentWhen={{link.currentWhen}}
+          @class={{link.linkDragCss}}
+          {{(if
+            link.didStartDrag
+            (modifier
+              "draggable"
+              didStartDrag=link.didStartDrag
+              didEndDrag=link.didEndDrag
+              dragMove=link.dragMove
+            )
+          )}}
+        />
+      {{/if}}
+    {{/if}}
+  {{/each}}
+
+  {{#if this.section.moreLinks}}
+    {{#if this.isDesktopDropdownMode}}
+      {{#each this.section.moreLinks as |sectionLink|}}
+        <Sidebar::MoreSectionLink @sectionLink={{sectionLink}} />
+      {{/each}}
+    {{else if this.section.moreLinks}}
+      <Sidebar::MoreSectionLinks @sectionLinks={{this.section.moreLinks}} />
+    {{/if}}
+  {{/if}}
+</Sidebar::Section>

app/assets/javascripts/discourse/app/components/sidebar/common/custom-section.js

@@ -0,0 +1,46 @@
+import { getOwner } from "@ember/application";
+import Component from "@glimmer/component";
+import { tracked } from "@glimmer/tracking";
+import { inject as service } from "@ember/service";
+
+import Section from "discourse/lib/sidebar/section";
+import CommunitySection from "discourse/lib/sidebar/community-section";
+
+export default class SidebarCustomSection extends Component {
+  @service site;
+  @service siteSettings;
+
+  @tracked section;
+
+  constructor() {
+    super(...arguments);
+    this.section = this.#initializeSection();
+  }
+
+  willDestroy() {
+    this.section.teardown?.();
+    super.willDestroy();
+  }
+
+  get isDesktopDropdownMode() {
+    const headerDropdownMode =
+      this.siteSettings.navigation_menu === "header dropdown";
+
+    return !this.site.mobileView && headerDropdownMode;
+  }
+
+  #initializeSection() {
+    let sectionClass = Section;
+
+    switch (this.args.sectionData.section_type) {
+      case "community":
+        sectionClass = CommunitySection;
+        break;
+    }
+
+    return new sectionClass({
+      section: this.args.sectionData,
+      owner: getOwner(this),
+    });
+  }
+}

app/assets/javascripts/discourse/app/components/sidebar/common/custom-sections.hbs

@@ -0,0 +1,5 @@
+<div class="sidebar-custom-sections">
+  {{#each this.sections as |section|}}
+    <Sidebar::Common::CustomSection @sectionData={{section}} />
+  {{/each}}
+</div>

app/assets/javascripts/discourse/app/components/sidebar/common/custom-sections.js

@@ -0,0 +1,22 @@
+import Component from "@glimmer/component";
+import { inject as service } from "@ember/service";
+
+export default class SidebarCustomSection extends Component {
+  @service currentUser;
+  @service router;
+  @service messageBus;
+  @service appEvents;
+  @service topicTrackingState;
+  @service site;
+  @service siteSettings;
+
+  anonymous = false;
+
+  get sections() {
+    if (this.anonymous) {
+      return this.site.anonymous_sidebar_sections;
+    } else {
+      return this.currentUser.sidebarSections;
+    }
+  }
+}

app/assets/javascripts/discourse/app/components/sidebar/user/custom-sections.hbs

@@ -1,71 +0,0 @@
-<div class="sidebar-custom-sections">
-  {{#each this.sections as |section|}}
-    <Sidebar::Section
-      @sectionName={{section.slug}}
-      @headerLinkText={{section.decoratedTitle}}
-      @collapsable={{true}}
-      @headerActions={{section.headerActions}}
-      @headerActionsIcon={{section.headerActionIcon}}
-      @class={{section.dragCss}}
-    >
-      {{#each section.links as |link|}}
-        {{#if link.shouldDisplay}}
-          {{#if link.external}}
-            <Sidebar::SectionLink
-              @linkName={{link.name}}
-              @content={{replace-emoji link.text}}
-              @prefixType="icon"
-              @prefixValue={{link.prefixValue}}
-              @href={{link.value}}
-              @class={{link.linkDragCss}}
-              {{draggable
-                didStartDrag=link.didStartDrag
-                didEndDrag=link.didEndDrag
-                dragMove=link.dragMove
-              }}
-            />
-          {{else}}
-            <Sidebar::SectionLink
-              @shouldDisplay={{link.shouldDisplay}}
-              @href={{link.href}}
-              @title={{link.title}}
-              @linkName={{link.name}}
-              @route={{link.route}}
-              @model={{link.model}}
-              @models={{link.models}}
-              @query={{link.query}}
-              @content={{replace-emoji link.text}}
-              @badgeText={{link.badgeText}}
-              @prefixType="icon"
-              @prefixValue={{link.prefixValue}}
-              @suffixCSSClass={{link.suffixCSSClass}}
-              @suffixValue={{link.suffixValue}}
-              @suffixType={{link.suffixType}}
-              @currentWhen={{link.currentWhen}}
-              @class={{link.linkDragCss}}
-              {{(if
-                link.didStartDrag
-                (modifier
-                  "draggable"
-                  didStartDrag=link.didStartDrag
-                  didEndDrag=link.didEndDrag
-                  dragMove=link.dragMove
-                )
-              )}}
-            />
-          {{/if}}
-        {{/if}}
-      {{/each}}
-
-      {{#if this.isDesktopDropdownMode}}
-        {{#each section.moreLinks as |sectionLink|}}
-          <Sidebar::MoreSectionLink @sectionLink={{sectionLink}} />
-        {{/each}}
-      {{else}}
-        {{#if section.moreLinks}}
-          <Sidebar::MoreSectionLinks @sectionLinks={{section.moreLinks}} />
-        {{/if}}
-      {{/if}}
-    </Sidebar::Section>
-  {{/each}}
-</div>

app/assets/javascripts/discourse/app/components/sidebar/user/custom-sections.js

@@ -1,95 +1,22 @@
-import Component from "@glimmer/component";
-import { inject as service } from "@ember/service";
 import { ajax } from "discourse/lib/ajax";
 import { bind } from "discourse-common/utils/decorators";
-import Section from "discourse/lib/sidebar/section";
-import CommunitySection from "discourse/lib/sidebar/community-section";
-import { tracked } from "@glimmer/tracking";
-
-export const REFRESH_CUSTOM_SIDEBAR_SECTIONS_APP_EVENT_NAME =
-  "sidebar:refresh-custom-sections";
-
-export default class SidebarUserCustomSections extends Component {
-  @service currentUser;
-  @service router;
-  @service messageBus;
-  @service appEvents;
-  @service topicTrackingState;
-  @service site;
-  @service siteSettings;
-
-  @tracked sections = [];
+import SidebarCustomSection from "discourse/components/sidebar/common/custom-sections";
 
+export default class SidebarUserCustomSections extends SidebarCustomSection {
   constructor() {
     super(...arguments);
+
     this.messageBus.subscribe("/refresh-sidebar-sections", this._refresh);
-
-    this.appEvents.on(
-      REFRESH_CUSTOM_SIDEBAR_SECTIONS_APP_EVENT_NAME,
-      this,
-      this._refreshSections
-    );
-
-    this.#initSections();
   }
 
   willDestroy() {
-    this.appEvents.off(
-      REFRESH_CUSTOM_SIDEBAR_SECTIONS_APP_EVENT_NAME,
-      this,
-      this._refreshSections
-    );
-
     this.messageBus.unsubscribe("/refresh-sidebar-sections");
-    this.#teardown();
-  }
-
-  #teardown() {
-    return this.sections.forEach((section) => {
-      section.teardown?.();
-    });
-  }
-
-  _refreshSections() {
-    this.#teardown();
-    this.#initSections();
-  }
-
-  #initSections() {
-    this.sections = this.currentUser.sidebarSections.map((section) => {
-      switch (section.section_type) {
-        case "community":
-          const systemSection = new CommunitySection({
-            section,
-            currentUser: this.currentUser,
-            router: this.router,
-            appEvents: this.appEvents,
-            topicTrackingState: this.topicTrackingState,
-            siteSettings: this.siteSettings,
-          });
-          return systemSection;
-          break;
-        default:
-          return new Section({
-            section,
-            currentUser: this.currentUser,
-            router: this.router,
-          });
-      }
-    });
-  }
-
-  get isDesktopDropdownMode() {
-    const headerDropdownMode =
-      this.siteSettings.navigation_menu === "header dropdown";
-
-    return !this.site.mobileView && headerDropdownMode;
   }
 
   @bind
   _refresh() {
     return ajax("/sidebar_sections.json", {}).then((json) => {
-      this.currentUser.updateSidebarSections(json.sidebar_sections);
+      this.currentUser.set("sidebar_sections", json.sidebar_sections);
     });
   }
 }

app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js

@@ -268,7 +268,8 @@ export default Controller.extend(ModalFunctionality, {
       }),
     })
       .then((data) => {
-        this.currentUser.updateSidebarSections(
+        this.currentUser.set(
+          "sidebar_sections",
           this.currentUser.sidebar_sections.concat(data.sidebar_section)
         );
         this.send("closeModal");
@@ -309,7 +310,7 @@ export default Controller.extend(ModalFunctionality, {
             return section;
           }
         );
-        this.currentUser.updateSidebarSections(newSidebarSections);
+        this.currentUser.set("sidebar_sections", newSidebarSections);
         this.send("closeModal");
       })
       .catch((e) =>
@@ -359,7 +360,7 @@ export default Controller.extend(ModalFunctionality, {
                 this.currentUser.sidebar_sections.filter((section) => {
                   return section.id !== data["sidebar_section"].id;
                 });
-              this.currentUser.updateSidebarSections(newSidebarSections);
+              this.currentUser.set("sidebar_sections", newSidebarSections);
               this.send("closeModal");
             })
             .catch((e) =>

app/assets/javascripts/discourse/app/lib/sidebar/community-section.js

@@ -1,7 +1,8 @@
 import I18n from "I18n";
 import SectionLink from "discourse/lib/sidebar/section-link";
 import Composer from "discourse/models/composer";
-import { getOwner } from "discourse-common/lib/get-owner";
+import { getOwner, setOwner } from "@ember/application";
+import { inject as service } from "@ember/service";
 import { tracked } from "@glimmer/tracking";
 import { action } from "@ember/object";
 import { next } from "@ember/runloop";
@@ -21,6 +22,7 @@ import {
 } from "discourse/lib/sidebar/custom-community-section-links";
 
 const LINKS_IN_BOTH_SEGMENTS = ["/review"];
+
 const SPECIAL_LINKS_MAP = {
   "/latest": EverythingSectionLink,
   "/new": EverythingSectionLink,
@@ -35,25 +37,20 @@ const SPECIAL_LINKS_MAP = {
 };
 
 export default class CommunitySection {
+  @service appEvents;
+  @service currentUser;
+  @service router;
+  @service topicTrackingState;
+  @service siteSettings;
+
   @tracked links;
   @tracked moreLinks;
 
-  constructor({
-    section,
-    currentUser,
-    router,
-    topicTrackingState,
-    appEvents,
-    siteSettings,
-  }) {
+  constructor({ section, owner }) {
+    setOwner(this, owner);
+
     this.section = section;
-    this.router = router;
-    this.currentUser = currentUser;
     this.slug = section.slug;
-    this.topicTrackingState = topicTrackingState;
-    this.appEvents = appEvents;
-    this.siteSettings = siteSettings;
-    this.section_type = section.section_type;
 
     this.callbackId = this.topicTrackingState?.onStateChange(() => {
       this.links.forEach((link) => {
@@ -67,28 +64,37 @@ export default class CommunitySection {
       .concat(secondaryCustomSectionLinks)
       .map((link) => this.#initializeSectionLink(link, { inMoreDrawer: true }));
 
-    this.links = this.section.links
-      .filter(
-        (link) =>
-          link.segment === "primary" ||
-          LINKS_IN_BOTH_SEGMENTS.includes(link.value)
-      )
-      .map((link) => {
-        return this.#generateLink(link);
-      })
-      .filter((link) => link);
+    this.links = this.section.links.reduce((filtered, link) => {
+      if (
+        link.segment === "primary" ||
+        LINKS_IN_BOTH_SEGMENTS.includes(link.value)
+      ) {
+        const generatedLink = this.#generateLink(link);
+
+        if (generatedLink) {
+          filtered.push(generatedLink);
+        }
+      }
+
+      return filtered;
+    }, []);
 
     this.moreLinks = this.section.links
-      .filter(
-        (link) =>
+      .reduce((filtered, link) => {
+        if (
           link.segment === "secondary" ||
           LINKS_IN_BOTH_SEGMENTS.includes(link.value)
-      )
-      .map((link) => {
-        return this.#generateLink(link, true);
-      })
-      .concat(this.apiLinks)
-      .filter((link) => link);
+        ) {
+          const generatedLink = this.#generateLink(link, true);
+
+          if (generatedLink) {
+            filtered.push(generatedLink);
+          }
+        }
+
+        return filtered;
+      }, [])
+      .concat(this.apiLinks);
   }
 
   teardown() {
@@ -103,6 +109,7 @@ export default class CommunitySection {
 
   #generateLink(link, inMoreDrawer = false) {
     const sectionLinkClass = SPECIAL_LINKS_MAP[link.value];
+
     if (sectionLinkClass) {
       return this.#initializeSectionLink(sectionLinkClass, inMoreDrawer);
     } else {

app/assets/javascripts/discourse/app/lib/sidebar/section.js

@@ -4,17 +4,22 @@ import { iconHTML } from "discourse-common/lib/icon-library";
 import { htmlSafe } from "@ember/template";
 import SectionLink from "discourse/lib/sidebar/section-link";
 import { tracked } from "@glimmer/tracking";
+import { setOwner } from "@ember/application";
+import { inject as service } from "@ember/service";
 import { bind } from "discourse-common/utils/decorators";
 import { ajax } from "discourse/lib/ajax";
 
 export default class Section {
+  @service currentUser;
+  @service router;
+
   @tracked dragCss;
   @tracked links;
 
-  constructor({ section, currentUser, router }) {
+  constructor({ section, owner }) {
+    setOwner(this, owner);
+
     this.section = section;
-    this.router = router;
-    this.currentUser = currentUser;
     this.slug = section.slug;
 
     this.links = this.section.links.map((link) => {

app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/my-posts-section-link.js

@@ -13,6 +13,7 @@ export default class MyPostsSectionLink extends BaseSectionLink {
 
   constructor() {
     super(...arguments);
+
     if (this.shouldDisplay) {
       this.appEvents.on(
         USER_DRAFTS_CHANGED_EVENT,
@@ -23,7 +24,13 @@ export default class MyPostsSectionLink extends BaseSectionLink {
   }
 
   teardown() {
-    this.appEvents.off(USER_DRAFTS_CHANGED_EVENT, this, this._updateDraftCount);
+    if (this.shouldDisplay) {
+      this.appEvents.off(
+        USER_DRAFTS_CHANGED_EVENT,
+        this,
+        this._updateDraftCount
+      );
+    }
   }
 
   _updateDraftCount() {

app/assets/javascripts/discourse/app/models/user.js

@@ -51,7 +51,6 @@ import {
   showUserTip,
 } from "discourse/lib/user-tips";
 import { dependentKeyCompat } from "@ember/object/compat";
-import { REFRESH_CUSTOM_SIDEBAR_SECTIONS_APP_EVENT_NAME } from "discourse/components/sidebar/user/custom-sections";
 
 export const SECOND_FACTOR_METHODS = {
   TOTP: 1,
@@ -1162,11 +1161,6 @@ const User = RestModel.extend({
     this.appEvents.trigger("user-reviewable-count:changed", count);
   },
 
-  updateSidebarSections(sections) {
-    this.set("sidebar_sections", sections);
-    this.appEvents.trigger(REFRESH_CUSTOM_SIDEBAR_SECTIONS_APP_EVENT_NAME);
-  },
-
   isInDoNotDisturb() {
     return (
       this.do_not_disturb_until &&

app/assets/javascripts/discourse/tests/acceptance/sidebar-user-community-section-test.js

@@ -47,6 +47,7 @@ acceptance("Sidebar - Logged on user - Community Section", function (needs) {
 
   test("clicking on section header button", async function (assert) {
     await visit("/");
+
     await click(
       ".sidebar-section[data-section-name='community'] .sidebar-section-header-button"
     );

app/assets/javascripts/discourse/tests/acceptance/sidebar-user-test.js

@@ -27,7 +27,7 @@ acceptance(
 );
 
 acceptance(
-  "Sidebar - Logged on user - Experimental sidebar and hamburger setting enabled - Sidebar disabled",
+  "Sidebar - Logged on user - header dropdown navigation menu enabled",
   function (needs) {
     needs.user();