From 1106e4ad09eb325d23800df5d77084ae2c41eb6b Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Mon, 15 May 2023 16:43:41 +0900 Subject: [PATCH] FIX: Sidebar custom sections rendering perf degrades over time (#21552) What is the problem? The main problem here is that we were incorrectly registering the same `onStateChange` callback with `TopicTrackingState` each time a user reads a post. When a user reads a post, the state in `TopicTrackingState` is updated and it triggers all the `onStateChange` callbacks which have been registered. In the `CommunitySection` class, we register a callback which would then call the `onTopicTrackingStateChange` method for each link in the class. For the `EverythingSectionLink` class, this would lookup the state in `TopicTrackingState` to get a new count of unread/new topics and update the `totalUnread` and `totalNew` properties which are tracked. For some reason that I have yet to figure out, updating the either of the tracked properties would result in Ember rerendering the entire `{{#each this.sections as |section|}}` in `component/sidebar/user/custom-sections.hbs` template. Note that `this.sections` refers to a `@cached` getter in the `SidebarUserCustomSections` class. The problem is that the `sections` getter is initializing a new bunch of sidebar sections related classes without calling the teardown function. As a result, we end up registering new `onStateChange` callbacks in `TopicTrackingState` in `CommunitySection` without removing the old ones. Over time, the number of callbacks build up and we end up slowing down the application. While we do not know the reason why defining a getter for the `sections` is causing the entire block to re-render, I realized that it is dangerous to use a getter for `sections` here since we have very little control on when the cached is broken. Instead, I moved the `sections` getter to a tracked property instead where the property is updated via `appEvents`. With this change, updating the tracked properties in `EverythingSectionLink` is no longer triggering a complete re-render of the said block above. We also now call `teardown` on the section objects that has been initialised before updating the `sections` property. --- .../sidebar/user/custom-sections.js | 37 ++++++++++++++++--- .../app/controllers/sidebar-section-form.js | 7 ++-- .../app/lib/sidebar/community-section.js | 1 + .../community-section/review-section-link.js | 21 +++++------ .../javascripts/discourse/app/models/user.js | 6 +++ .../sidebar-user-community-section-test.js | 14 +++++++ 6 files changed, 65 insertions(+), 21 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/sidebar/user/custom-sections.js b/app/assets/javascripts/discourse/app/components/sidebar/user/custom-sections.js index 8e8cc0478e1..48355a1addf 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/user/custom-sections.js +++ b/app/assets/javascripts/discourse/app/components/sidebar/user/custom-sections.js @@ -2,9 +2,12 @@ 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 { cached } from "@glimmer/tracking"; 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; @@ -15,21 +18,45 @@ export default class SidebarUserCustomSections extends Component { @service site; @service siteSettings; + @tracked sections = []; + 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?.(); }); } - @cached - get sections() { - return this.currentUser.sidebarSections.map((section) => { + _refreshSections() { + this.#teardown(); + this.#initSections(); + } + + #initSections() { + this.sections = this.currentUser.sidebarSections.map((section) => { switch (section.section_type) { case "community": const systemSection = new CommunitySection({ @@ -62,7 +89,7 @@ export default class SidebarUserCustomSections extends Component { @bind _refresh() { return ajax("/sidebar_sections.json", {}).then((json) => { - this.currentUser.set("sidebar_sections", json.sidebar_sections); + this.currentUser.updateSidebarSections(json.sidebar_sections); }); } } diff --git a/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js b/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js index 71467d82c89..bc83fef10f4 100644 --- a/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js +++ b/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js @@ -268,8 +268,7 @@ export default Controller.extend(ModalFunctionality, { }), }) .then((data) => { - this.currentUser.set( - "sidebar_sections", + this.currentUser.updateSidebarSections( this.currentUser.sidebar_sections.concat(data.sidebar_section) ); this.send("closeModal"); @@ -310,7 +309,7 @@ export default Controller.extend(ModalFunctionality, { return section; } ); - this.currentUser.set("sidebar_sections", newSidebarSections); + this.currentUser.updateSidebarSections(newSidebarSections); this.send("closeModal"); }) .catch((e) => @@ -360,7 +359,7 @@ export default Controller.extend(ModalFunctionality, { this.currentUser.sidebar_sections.filter((section) => { return section.id !== data["sidebar_section"].id; }); - this.currentUser.set("sidebar_sections", newSidebarSections); + this.currentUser.updateSidebarSections(newSidebarSections); this.send("closeModal"); }) .catch((e) => diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/community-section.js b/app/assets/javascripts/discourse/app/lib/sidebar/community-section.js index 0a6d705a74a..2927e9be503 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/community-section.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/community-section.js @@ -95,6 +95,7 @@ export default class CommunitySection { if (this.callbackId) { this.topicTrackingState.offStateChange(this.callbackId); } + [...this.links, ...this.moreLinks].forEach((sectionLink) => { sectionLink.teardown?.(); }); diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/review-section-link.js b/app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/review-section-link.js index d2f8f3e1725..1ac7d1dabff 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/review-section-link.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/review-section-link.js @@ -12,21 +12,18 @@ export default class ReviewSectionLink extends BaseSectionLink { super(...arguments); this._refreshCanDisplay(); - if (this.shouldDisplay) { - this.appEvents.on( - "user-reviewable-count:changed", - this._refreshCanDisplay - ); - } + + this.appEvents?.on( + "user-reviewable-count:changed", + this._refreshCanDisplay + ); } teardown() { - if (this.shouldDisplay) { - this.appEvents.off( - "user-reviewable-count:changed", - this._refreshCanDisplay - ); - } + this.appEvents?.off( + "user-reviewable-count:changed", + this._refreshCanDisplay + ); } @bind diff --git a/app/assets/javascripts/discourse/app/models/user.js b/app/assets/javascripts/discourse/app/models/user.js index d225239173e..11a00cbee68 100644 --- a/app/assets/javascripts/discourse/app/models/user.js +++ b/app/assets/javascripts/discourse/app/models/user.js @@ -51,6 +51,7 @@ 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, @@ -1161,6 +1162,11 @@ 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 && diff --git a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-community-section-test.js b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-community-section-test.js index 90c2c5d7a11..7a3558fd67f 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-community-section-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-community-section-test.js @@ -791,6 +791,14 @@ acceptance("Sidebar - Logged on user - Community Section", function (needs) { "shows suffix indicator for unread posts on everything link" ); + const topicTrackingState = this.container.lookup( + "service:topic-tracking-state" + ); + + const initialCallbackCount = Object.keys( + topicTrackingState.stateChangeCallbacks + ).length; + // simulate reading topic 2 await publishToMessageBus("/unread", { topic_id: 2, @@ -809,6 +817,12 @@ acceptance("Sidebar - Logged on user - Community Section", function (needs) { "shows suffix indicator for new topics on categories link" ); + assert.equal( + Object.keys(topicTrackingState.stateChangeCallbacks).length, + initialCallbackCount, + "it does not add a new topic tracking state callback when the topic is read" + ); + // simulate reading topic 1 await publishToMessageBus("/unread", { topic_id: 1,