From c245b7439822e551cd40fece852ca92eb9243ae6 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 30 Aug 2022 11:39:32 +0800 Subject: [PATCH] FIX: Avoid leaking TopicTrackingState listeners due to sidebar (#18131) In eb12daa7f8793092bf4fe0c180a04fa3ea2d13e1 when adding community section support for anonymous users, we changed the `sectionLinks` property into a getter method. This meant that if the getter method was called again after the community section has been rendered, we would end up reintializing the section links classes. As part of the initialisation, some section links would setup a TopicTrackingState onStateChange listener. However, the listener is only removed when the entire community section is removed which resulted in us leaking the onStateChange listeners. This commit reverts the `sectionLinks` from being defined as a getter method into a property which is only set once when the community section is being constructor. Also, we changed it such that the community section will register the listener instead of each section link since it makes cleaning up much easier to reason about. No tests have been added for this commit because the original bug is not possible after this change and we already have an existing tests ensuring that TopicTrackingState change listeners are cleaned up when the community section is destroyed. Internal ref: /t/73224 --- .../sidebar/anonymous/community-section.js | 14 ++--- .../sidebar/common/community-section.js | 63 +++++++++++++------ .../sidebar/user/community-section.js | 32 +++++----- .../sidebar/base-community-section-link.js | 5 ++ .../everything-section-link.js | 24 +++---- .../community-section/tracked-section-link.js | 15 ++--- 6 files changed, 82 insertions(+), 71 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/sidebar/anonymous/community-section.js b/app/assets/javascripts/discourse/app/components/sidebar/anonymous/community-section.js index faef94d5dc6..a37ec7795f7 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/anonymous/community-section.js +++ b/app/assets/javascripts/discourse/app/components/sidebar/anonymous/community-section.js @@ -7,18 +7,16 @@ import UsersSectionLink from "discourse/lib/sidebar/common/community-section/use import BadgesSectionLink from "discourse/lib/sidebar/common/community-section/badges-section-link"; export default class SidebarAnonymousCommunitySection extends SidebarCommonCommunitySection { - constructor() { - super(...arguments); - - this.defaultMoreSectionLinks = [GroupsSectionLink, BadgesSectionLink]; - - this.defaultMoreSecondarySectionLinks = []; - - this.defaultMainSectionLinks = [ + get defaultMainSectionLinks() { + return [ EverythingSectionLink, UsersSectionLink, AboutSectionLink, FAQSectionLink, ]; } + + get defaultMoreSectionLinks() { + return [GroupsSectionLink, BadgesSectionLink]; + } } diff --git a/app/assets/javascripts/discourse/app/components/sidebar/common/community-section.js b/app/assets/javascripts/discourse/app/components/sidebar/common/community-section.js index 333a29f5d46..64a74edf18c 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/common/community-section.js +++ b/app/assets/javascripts/discourse/app/components/sidebar/common/community-section.js @@ -13,45 +13,68 @@ export default class SidebarCommunitySection extends Component { @service appEvents; @service siteSettings; - // Override in child - defaultMainSectionLinks = []; - defaultAdminMainSectionLinks = []; - defaultMoreSectionLinks = []; - defaultMoreSecondarySectionLinks = []; headerActionsIcon; headerActions; + sectionLinks; + moreSectionLinks; + moreSecondarySectionLinks; + callbackId; - get moreSectionLinks() { - return [...this.defaultMoreSectionLinks, ...customSectionLinks].map( - (sectionLinkClass) => { - return this.#initializeSectionLink(sectionLinkClass); - } - ); - } + constructor() { + super(...arguments); - get moreSecondarySectionLinks() { - return [ + this.moreSectionLinks = [ + ...this.defaultMoreSectionLinks, + ...customSectionLinks, + ].map((sectionLinkClass) => { + return this.#initializeSectionLink(sectionLinkClass); + }); + + this.moreSecondarySectionLinks = [ ...this.defaultMoreSecondarySectionLinks, ...secondaryCustomSectionLinks, ].map((sectionLinkClass) => { return this.#initializeSectionLink(sectionLinkClass); }); - } - get mainSectionLinks() { - return this.currentUser?.staff + const mainSectionLinks = this.currentUser?.staff ? [...this.defaultMainSectionLinks, ...this.defaultAdminMainSectionLinks] : [...this.defaultMainSectionLinks]; - } - get sectionLinks() { - return this.mainSectionLinks.map((sectionLinkClass) => { + this.sectionLinks = mainSectionLinks.map((sectionLinkClass) => { return this.#initializeSectionLink(sectionLinkClass); }); + + this.callbackId = this.topicTrackingState.onStateChange(() => { + this.sectionLinks.forEach((sectionLink) => { + sectionLink.onTopicTrackingStateChange(); + }); + }); } willDestroy() { this.sectionLinks.forEach((sectionLink) => sectionLink.teardown()); + this.topicTrackingState.offStateChange(this.callbackId); + } + + // Override in child + get defaultMainSectionLinks() { + return []; + } + + // Override in child + get defaultAdminMainSectionLinks() { + return []; + } + + // Override in child + get defaultMoreSectionLinks() { + return []; + } + + // Override in child + get defaultMoreSecondarySectionLinks() { + return []; } #initializeSectionLink(sectionLinkClass) { diff --git a/app/assets/javascripts/discourse/app/components/sidebar/user/community-section.js b/app/assets/javascripts/discourse/app/components/sidebar/user/community-section.js index 2c46db8a403..1ba644a618c 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/user/community-section.js +++ b/app/assets/javascripts/discourse/app/components/sidebar/user/community-section.js @@ -21,22 +21,6 @@ export default class SidebarUserCommunitySection extends SidebarCommonCommunityS constructor() { super(...arguments); - this.defaultMoreSectionLinks = [ - GroupsSectionLink, - UsersSectionLink, - BadgesSectionLink, - ]; - - this.defaultMoreSecondarySectionLinks = [AboutSectionLink, FAQSectionLink]; - - this.defaultMainSectionLinks = [ - EverythingSectionLink, - TrackedSectionLink, - MyPostsSectionLink, - ]; - - this.defaultAdminMainSectionLinks = [AdminSectionLink]; - this.headerActionsIcon = "plus"; this.headerActions = [ @@ -47,6 +31,22 @@ export default class SidebarUserCommunitySection extends SidebarCommonCommunityS ]; } + get defaultMainSectionLinks() { + return [EverythingSectionLink, TrackedSectionLink, MyPostsSectionLink]; + } + + get defaultAdminMainSectionLinks() { + return [AdminSectionLink]; + } + + get defaultMoreSectionLinks() { + return [GroupsSectionLink, UsersSectionLink, BadgesSectionLink]; + } + + get defaultMoreSecondarySectionLinks() { + return [AboutSectionLink, FAQSectionLink]; + } + @action composeTopic() { const composerArgs = { diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/base-community-section-link.js b/app/assets/javascripts/discourse/app/lib/sidebar/base-community-section-link.js index 65af90de12e..e41a3e91fb1 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/base-community-section-link.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/base-community-section-link.js @@ -16,6 +16,11 @@ export default class BaseCommunitySectionLink { this.siteSettings = siteSettings; } + /** + * Called when state has changed in the TopicTrackingState service + */ + onTopicTrackingStateChange() {} + /** * Called when community-section component is torn down. */ diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/common/community-section/everything-section-link.js b/app/assets/javascripts/discourse/app/lib/sidebar/common/community-section/everything-section-link.js index 8071140d03a..8cba16fe4f0 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/common/community-section/everything-section-link.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/common/community-section/everything-section-link.js @@ -1,34 +1,26 @@ import I18n from "I18n"; import { tracked } from "@glimmer/tracking"; -import { bind } from "discourse-common/utils/decorators"; import BaseSectionLink from "discourse/lib/sidebar/base-community-section-link"; export default class EverythingSectionLink extends BaseSectionLink { @tracked totalUnread = 0; @tracked totalNew = 0; - callbackId = null; constructor() { super(...arguments); - - if (this.currentUser) { - this.callbackId = this.topicTrackingState.onStateChange( - this._refreshCounts - ); - - this._refreshCounts(); - } + this.#refreshCounts(); } - teardown() { - if (this.callbackId) { - this.topicTrackingState.offStateChange(this.callbackId); - } + onTopicTrackingStateChange() { + this.#refreshCounts(); } - @bind - _refreshCounts() { + #refreshCounts() { + if (!this.currentUser) { + return; + } + this.totalUnread = this.topicTrackingState.countUnread(); if (this.totalUnread === 0) { diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/tracked-section-link.js b/app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/tracked-section-link.js index 6b89cf81299..1898b6f6802 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/tracked-section-link.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/tracked-section-link.js @@ -1,30 +1,23 @@ import I18n from "I18n"; import { tracked } from "@glimmer/tracking"; -import { bind } from "discourse-common/utils/decorators"; import BaseSectionLink from "discourse/lib/sidebar/base-community-section-link"; import { isTrackedTopic } from "discourse/lib/topic-list-tracked-filter"; export default class TrackedSectionLink extends BaseSectionLink { @tracked totalUnread = 0; @tracked totalNew = 0; - callbackId = null; constructor() { super(...arguments); - - this.callbackId = this.topicTrackingState.onStateChange( - this._refreshCounts - ); - this._refreshCounts(); + this.#refreshCounts(); } - teardown() { - this.topicTrackingState.offStateChange(this.callbackId); + onTopicTrackingStateChange() { + this.#refreshCounts(); } - @bind - _refreshCounts() { + #refreshCounts() { this.totalUnread = this.topicTrackingState.countUnread({ customFilterFn: isTrackedTopic, });