mirror of
https://github.com/discourse/discourse.git
synced 2024-11-25 10:20:58 -06:00
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.
This commit is contained in:
parent
580f60d61d
commit
1106e4ad09
@ -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);
|
||||
});
|
||||
}
|
||||
}
|
||||
|
@ -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) =>
|
||||
|
@ -95,6 +95,7 @@ export default class CommunitySection {
|
||||
if (this.callbackId) {
|
||||
this.topicTrackingState.offStateChange(this.callbackId);
|
||||
}
|
||||
|
||||
[...this.links, ...this.moreLinks].forEach((sectionLink) => {
|
||||
sectionLink.teardown?.();
|
||||
});
|
||||
|
@ -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
|
||||
|
@ -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 &&
|
||||
|
@ -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,
|
||||
|
Loading…
Reference in New Issue
Block a user