FIX: Avoid leaking TopicTrackingState listeners due to sidebar (#18131)

In eb12daa7f8 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
This commit is contained in:
Alan Guo Xiang Tan 2022-08-30 11:39:32 +08:00 committed by GitHub
parent 417f156f6d
commit c245b74398
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 82 additions and 71 deletions

View File

@ -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"; import BadgesSectionLink from "discourse/lib/sidebar/common/community-section/badges-section-link";
export default class SidebarAnonymousCommunitySection extends SidebarCommonCommunitySection { export default class SidebarAnonymousCommunitySection extends SidebarCommonCommunitySection {
constructor() { get defaultMainSectionLinks() {
super(...arguments); return [
this.defaultMoreSectionLinks = [GroupsSectionLink, BadgesSectionLink];
this.defaultMoreSecondarySectionLinks = [];
this.defaultMainSectionLinks = [
EverythingSectionLink, EverythingSectionLink,
UsersSectionLink, UsersSectionLink,
AboutSectionLink, AboutSectionLink,
FAQSectionLink, FAQSectionLink,
]; ];
} }
get defaultMoreSectionLinks() {
return [GroupsSectionLink, BadgesSectionLink];
}
} }

View File

@ -13,45 +13,68 @@ export default class SidebarCommunitySection extends Component {
@service appEvents; @service appEvents;
@service siteSettings; @service siteSettings;
// Override in child
defaultMainSectionLinks = [];
defaultAdminMainSectionLinks = [];
defaultMoreSectionLinks = [];
defaultMoreSecondarySectionLinks = [];
headerActionsIcon; headerActionsIcon;
headerActions; headerActions;
sectionLinks;
moreSectionLinks;
moreSecondarySectionLinks;
callbackId;
get moreSectionLinks() { constructor() {
return [...this.defaultMoreSectionLinks, ...customSectionLinks].map( super(...arguments);
(sectionLinkClass) => {
this.moreSectionLinks = [
...this.defaultMoreSectionLinks,
...customSectionLinks,
].map((sectionLinkClass) => {
return this.#initializeSectionLink(sectionLinkClass); return this.#initializeSectionLink(sectionLinkClass);
} });
);
}
get moreSecondarySectionLinks() { this.moreSecondarySectionLinks = [
return [
...this.defaultMoreSecondarySectionLinks, ...this.defaultMoreSecondarySectionLinks,
...secondaryCustomSectionLinks, ...secondaryCustomSectionLinks,
].map((sectionLinkClass) => { ].map((sectionLinkClass) => {
return this.#initializeSectionLink(sectionLinkClass); return this.#initializeSectionLink(sectionLinkClass);
}); });
}
get mainSectionLinks() { const mainSectionLinks = this.currentUser?.staff
return this.currentUser?.staff
? [...this.defaultMainSectionLinks, ...this.defaultAdminMainSectionLinks] ? [...this.defaultMainSectionLinks, ...this.defaultAdminMainSectionLinks]
: [...this.defaultMainSectionLinks]; : [...this.defaultMainSectionLinks];
}
get sectionLinks() { this.sectionLinks = mainSectionLinks.map((sectionLinkClass) => {
return this.mainSectionLinks.map((sectionLinkClass) => {
return this.#initializeSectionLink(sectionLinkClass); return this.#initializeSectionLink(sectionLinkClass);
}); });
this.callbackId = this.topicTrackingState.onStateChange(() => {
this.sectionLinks.forEach((sectionLink) => {
sectionLink.onTopicTrackingStateChange();
});
});
} }
willDestroy() { willDestroy() {
this.sectionLinks.forEach((sectionLink) => sectionLink.teardown()); 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) { #initializeSectionLink(sectionLinkClass) {

View File

@ -21,22 +21,6 @@ export default class SidebarUserCommunitySection extends SidebarCommonCommunityS
constructor() { constructor() {
super(...arguments); super(...arguments);
this.defaultMoreSectionLinks = [
GroupsSectionLink,
UsersSectionLink,
BadgesSectionLink,
];
this.defaultMoreSecondarySectionLinks = [AboutSectionLink, FAQSectionLink];
this.defaultMainSectionLinks = [
EverythingSectionLink,
TrackedSectionLink,
MyPostsSectionLink,
];
this.defaultAdminMainSectionLinks = [AdminSectionLink];
this.headerActionsIcon = "plus"; this.headerActionsIcon = "plus";
this.headerActions = [ 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 @action
composeTopic() { composeTopic() {
const composerArgs = { const composerArgs = {

View File

@ -16,6 +16,11 @@ export default class BaseCommunitySectionLink {
this.siteSettings = siteSettings; this.siteSettings = siteSettings;
} }
/**
* Called when state has changed in the TopicTrackingState service
*/
onTopicTrackingStateChange() {}
/** /**
* Called when community-section component is torn down. * Called when community-section component is torn down.
*/ */

View File

@ -1,34 +1,26 @@
import I18n from "I18n"; import I18n from "I18n";
import { tracked } from "@glimmer/tracking"; import { tracked } from "@glimmer/tracking";
import { bind } from "discourse-common/utils/decorators";
import BaseSectionLink from "discourse/lib/sidebar/base-community-section-link"; import BaseSectionLink from "discourse/lib/sidebar/base-community-section-link";
export default class EverythingSectionLink extends BaseSectionLink { export default class EverythingSectionLink extends BaseSectionLink {
@tracked totalUnread = 0; @tracked totalUnread = 0;
@tracked totalNew = 0; @tracked totalNew = 0;
callbackId = null;
constructor() { constructor() {
super(...arguments); super(...arguments);
this.#refreshCounts();
if (this.currentUser) {
this.callbackId = this.topicTrackingState.onStateChange(
this._refreshCounts
);
this._refreshCounts();
}
} }
teardown() { onTopicTrackingStateChange() {
if (this.callbackId) { this.#refreshCounts();
this.topicTrackingState.offStateChange(this.callbackId); }
}
#refreshCounts() {
if (!this.currentUser) {
return;
} }
@bind
_refreshCounts() {
this.totalUnread = this.topicTrackingState.countUnread(); this.totalUnread = this.topicTrackingState.countUnread();
if (this.totalUnread === 0) { if (this.totalUnread === 0) {

View File

@ -1,30 +1,23 @@
import I18n from "I18n"; import I18n from "I18n";
import { tracked } from "@glimmer/tracking"; import { tracked } from "@glimmer/tracking";
import { bind } from "discourse-common/utils/decorators";
import BaseSectionLink from "discourse/lib/sidebar/base-community-section-link"; import BaseSectionLink from "discourse/lib/sidebar/base-community-section-link";
import { isTrackedTopic } from "discourse/lib/topic-list-tracked-filter"; import { isTrackedTopic } from "discourse/lib/topic-list-tracked-filter";
export default class TrackedSectionLink extends BaseSectionLink { export default class TrackedSectionLink extends BaseSectionLink {
@tracked totalUnread = 0; @tracked totalUnread = 0;
@tracked totalNew = 0; @tracked totalNew = 0;
callbackId = null;
constructor() { constructor() {
super(...arguments); super(...arguments);
this.#refreshCounts();
this.callbackId = this.topicTrackingState.onStateChange(
this._refreshCounts
);
this._refreshCounts();
} }
teardown() { onTopicTrackingStateChange() {
this.topicTrackingState.offStateChange(this.callbackId); this.#refreshCounts();
} }
@bind #refreshCounts() {
_refreshCounts() {
this.totalUnread = this.topicTrackingState.countUnread({ this.totalUnread = this.topicTrackingState.countUnread({
customFilterFn: isTrackedTopic, customFilterFn: isTrackedTopic,
}); });