From 4e42654c4f0a250492ebb12dcbd6e51b0133ec65 Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Tue, 5 Aug 2025 14:09:07 +0530 Subject: [PATCH 01/12] Introduce disposables to track sub vms and event listeners --- src/viewmodels/base/Disposables.ts | 69 ++++++++++++++++++++++++ test/viewmodels/base/Disposables-test.ts | 57 ++++++++++++++++++++ 2 files changed, 126 insertions(+) create mode 100644 src/viewmodels/base/Disposables.ts create mode 100644 test/viewmodels/base/Disposables-test.ts diff --git a/src/viewmodels/base/Disposables.ts b/src/viewmodels/base/Disposables.ts new file mode 100644 index 00000000000..ccda8b1fb0f --- /dev/null +++ b/src/viewmodels/base/Disposables.ts @@ -0,0 +1,69 @@ +/* +Copyright 2025 New Vector Ltd. + +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial +Please see LICENSE files in the repository root for full details. +*/ + +import type { EventEmitter } from "events"; + +/** + * Something that needs to be eventually disposed. This can be: + * - A function that does the disposing + * - An object containing a dispose method which does the disposing + */ +export type DisposableItem = { dispose: () => void } | (() => void); + +/** + * This class provides a way for the view-model to track any resource + * that it needs to eventually relinquish. + */ +export class Disposables { + private readonly disposables: DisposableItem[] = []; + private _isDisposed: boolean = false; + + /** + * Relinquish all tracked disposable values + */ + public dispose(): void { + this.throwIfDisposed(); + this._isDisposed = true; + for (const disposable of this.disposables) { + if (typeof disposable === "function") { + disposable(); + } else { + disposable.dispose(); + } + } + } + + /** + * Track a value that needs to be eventually relinquished + */ + public track(disposable: T): T { + this.throwIfDisposed(); + this.disposables.push(disposable); + return disposable; + } + + /** + * Add an event listener that will be removed on dispose + */ + public trackListener(emitter: EventEmitter, event: string, callback: (...args: unknown[]) => void): void { + emitter.on(event, callback); + this.track(() => { + emitter.off(event, callback); + }); + } + + private throwIfDisposed(): void { + if (this.isDisposed) throw new Error("Disposable is already disposed"); + } + + /** + * Whether this disposable has been disposed + */ + public get isDisposed(): boolean { + return this._isDisposed; + } +} diff --git a/test/viewmodels/base/Disposables-test.ts b/test/viewmodels/base/Disposables-test.ts new file mode 100644 index 00000000000..577374a644e --- /dev/null +++ b/test/viewmodels/base/Disposables-test.ts @@ -0,0 +1,57 @@ +/* +Copyright 2025 New Vector Ltd. +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial +Please see LICENSE files in the repository root for full details. +*/ + +import { EventEmitter } from "events"; + +import { Disposables } from "../../../src/viewmodels/base/Disposables"; + +describe("Disposable", () => { + it("isDisposed is true after dispose() is called", () => { + const disposables = new Disposables(); + expect(disposables.isDisposed).toEqual(false); + disposables.dispose(); + expect(disposables.isDisposed).toEqual(true); + }); + + it("dispose() calls the correct disposing function", () => { + const disposables = new Disposables(); + + const item1 = { + foo: 5, + dispose: jest.fn(), + }; + disposables.track(item1); + + const item2 = jest.fn(); + disposables.track(item2); + + disposables.dispose(); + + expect(item1.dispose).toHaveBeenCalledTimes(1); + expect(item2).toHaveBeenCalledTimes(1); + }); + + it("Throws error if acting on already disposed disposables", () => { + const disposables = new Disposables(); + disposables.dispose(); + expect(() => { + disposables.track(jest.fn); + }).toThrow(); + }); + + it("Removes tracked event listeners on dispose", () => { + const disposables = new Disposables(); + const emitter = new EventEmitter(); + + const fn = jest.fn(); + disposables.trackListener(emitter, "FooEvent", fn); + emitter.emit("FooEvent"); + expect(fn).toHaveBeenCalled(); + + disposables.dispose(); + expect(emitter.listenerCount("FooEvent", fn)).toEqual(0); + }); +}); From 3bbb62d3462ee12fb62d885e7c6a74fa9d06ec29 Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Tue, 5 Aug 2025 14:10:41 +0530 Subject: [PATCH 02/12] Remove old code --- src/viewmodels/base/BaseViewModel.ts | 32 +------------------ src/viewmodels/base/ViewModelSubscriptions.ts | 18 +---------- 2 files changed, 2 insertions(+), 48 deletions(-) diff --git a/src/viewmodels/base/BaseViewModel.ts b/src/viewmodels/base/BaseViewModel.ts index 942cb6d210f..0aee874bbdf 100644 --- a/src/viewmodels/base/BaseViewModel.ts +++ b/src/viewmodels/base/BaseViewModel.ts @@ -16,10 +16,7 @@ export abstract class BaseViewModel implements ViewModel { protected constructor(props: P, initialSnapshot: T) { this.props = props; - this.subs = new ViewModelSubscriptions( - this.addDownstreamSubscriptionWrapper, - this.removeDownstreamSubscriptionWrapper, - ); + this.subs = new ViewModelSubscriptions(); this.snapshot = new Snapshot(initialSnapshot, () => { this.subs.emit(); }); @@ -29,33 +26,6 @@ export abstract class BaseViewModel implements ViewModel { return this.subs.add(listener); }; - /** - * Wrapper around the abstract subscribe callback as we can't assume that the subclassed method - * has a bound `this` context. - */ - private addDownstreamSubscriptionWrapper = (): void => { - this.addDownstreamSubscription(); - }; - - /** - * Wrapper around the abstract unsubscribe callback as we can't call pass an abstract method directly - * in the constructor. - */ - private removeDownstreamSubscriptionWrapper = (): void => { - this.removeDownstreamSubscription(); - }; - - /** - * Called when the first listener subscribes: the subclass should set up any necessary subscriptions - * to call this.subs.emit() when the snapshot changes. - */ - protected abstract addDownstreamSubscription(): void; - - /** - * Called when the last listener unsubscribes: the subclass should clean up any subscriptions. - */ - protected abstract removeDownstreamSubscription(): void; - /** * Returns the current snapshot of the view model. */ diff --git a/src/viewmodels/base/ViewModelSubscriptions.ts b/src/viewmodels/base/ViewModelSubscriptions.ts index 97373fcf9fd..a713782aecd 100644 --- a/src/viewmodels/base/ViewModelSubscriptions.ts +++ b/src/viewmodels/base/ViewModelSubscriptions.ts @@ -6,20 +6,11 @@ Please see LICENSE files in the repository root for full details. */ /** - * Utility class for view models to manage suscriptions to their updates + * Utility class for view models to manage subscriptions to their updates */ export class ViewModelSubscriptions { private listeners = new Set<() => void>(); - /** - * @param subscribeCallback Called when the first listener subscribes. - * @param unsubscribeCallback Called when the last listener unsubscribes. - */ - public constructor( - private subscribeCallback: () => void, - private unsubscribeCallback: () => void, - ) {} - /** * Subscribe to changes in the view model. * @param listener Will be called whenever the snapshot changes. @@ -27,15 +18,8 @@ export class ViewModelSubscriptions { */ public add = (listener: () => void): (() => void) => { this.listeners.add(listener); - if (this.listeners.size === 1) { - this.subscribeCallback(); - } - return () => { this.listeners.delete(listener); - if (this.listeners.size === 0) { - this.unsubscribeCallback(); - } }; }; From 2cd2a4a8ef62a5889db1736475070e5b0eaedfa5 Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Tue, 5 Aug 2025 14:11:08 +0530 Subject: [PATCH 03/12] Use disposable in BaseViewModel --- src/viewmodels/base/BaseViewModel.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/viewmodels/base/BaseViewModel.ts b/src/viewmodels/base/BaseViewModel.ts index 0aee874bbdf..c6a9b05e156 100644 --- a/src/viewmodels/base/BaseViewModel.ts +++ b/src/viewmodels/base/BaseViewModel.ts @@ -6,6 +6,7 @@ Please see LICENSE files in the repository root for full details. */ import { type ViewModel } from "../../shared-components/ViewModel"; +import { Disposables } from "./Disposables"; import { Snapshot } from "./Snapshot"; import { ViewModelSubscriptions } from "./ViewModelSubscriptions"; @@ -13,6 +14,7 @@ export abstract class BaseViewModel implements ViewModel { protected subs: ViewModelSubscriptions; protected snapshot: Snapshot; protected props: P; + protected disposables = new Disposables(); protected constructor(props: P, initialSnapshot: T) { this.props = props; @@ -32,4 +34,11 @@ export abstract class BaseViewModel implements ViewModel { public getSnapshot = (): T => { return this.snapshot.current; }; + + /** + * Relinquish any resources held by this view-model. + */ + public dispose(): void { + this.disposables.dispose(); + } } From d86dcb1b7b8ddcfabbfd265daea54a4015930680 Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Tue, 5 Aug 2025 14:11:33 +0530 Subject: [PATCH 04/12] Update vm so that the listener is tracked through disposable --- src/viewmodels/event-tiles/TextualEventViewModel.ts | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/viewmodels/event-tiles/TextualEventViewModel.ts b/src/viewmodels/event-tiles/TextualEventViewModel.ts index 40121ecf2b7..dd8132e7ae3 100644 --- a/src/viewmodels/event-tiles/TextualEventViewModel.ts +++ b/src/viewmodels/event-tiles/TextualEventViewModel.ts @@ -17,18 +17,11 @@ export class TextualEventViewModel extends BaseViewModel { const content = textForEvent(this.props.mxEvent, MatrixClientPeg.safeGet(), true, this.props.showHiddenEvents); this.snapshot.set({ content }); }; - - protected addDownstreamSubscription = (): void => { - this.props.mxEvent.on(MatrixEventEvent.SentinelUpdated, this.setTextFromEvent); - }; - - protected removeDownstreamSubscription = (): void => { - this.props.mxEvent.off(MatrixEventEvent.SentinelUpdated, this.setTextFromEvent); - }; } From 4caf52abf3d32cf75c34c04b4aa3719db0e40c64 Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Mon, 11 Aug 2025 10:42:44 +0530 Subject: [PATCH 05/12] No-op on dispose call instead of throwing error --- src/viewmodels/base/Disposables.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/viewmodels/base/Disposables.ts b/src/viewmodels/base/Disposables.ts index ccda8b1fb0f..a712a120fb1 100644 --- a/src/viewmodels/base/Disposables.ts +++ b/src/viewmodels/base/Disposables.ts @@ -26,7 +26,7 @@ export class Disposables { * Relinquish all tracked disposable values */ public dispose(): void { - this.throwIfDisposed(); + if (this.isDisposed) return; this._isDisposed = true; for (const disposable of this.disposables) { if (typeof disposable === "function") { From ce2f9ae32a2f39028fa4860d27de545f8fc5e6fa Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Mon, 11 Aug 2025 10:43:36 +0530 Subject: [PATCH 06/12] Throw error in trackListener as well --- src/viewmodels/base/Disposables.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/viewmodels/base/Disposables.ts b/src/viewmodels/base/Disposables.ts index a712a120fb1..77df53d097b 100644 --- a/src/viewmodels/base/Disposables.ts +++ b/src/viewmodels/base/Disposables.ts @@ -50,6 +50,7 @@ export class Disposables { * Add an event listener that will be removed on dispose */ public trackListener(emitter: EventEmitter, event: string, callback: (...args: unknown[]) => void): void { + this.throwIfDisposed(); emitter.on(event, callback); this.track(() => { emitter.off(event, callback); From c3f18791899d2acac003c984c71283bf60219c16 Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Mon, 11 Aug 2025 11:01:32 +0530 Subject: [PATCH 07/12] Fix audio player vm --- src/viewmodels/audio/AudioPlayerViewModel.ts | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/viewmodels/audio/AudioPlayerViewModel.ts b/src/viewmodels/audio/AudioPlayerViewModel.ts index 208cd7e907a..e028ba78609 100644 --- a/src/viewmodels/audio/AudioPlayerViewModel.ts +++ b/src/viewmodels/audio/AudioPlayerViewModel.ts @@ -77,6 +77,9 @@ export class AudioPlayerViewModel public constructor(props: Props) { super(props, AudioPlayerViewModel.computeSnapshot(props.playback, props.mediaName)); + this.disposables.trackListener(props.playback, UPDATE_EVENT, this.setSnapshot); + // There is no unsubscribe method in SimpleObservable + this.props.playback.clockInfo.liveData.onUpdate(this.setSnapshot); // Don't wait for the promise to complete - it will emit a progress update when it // is done, and it's not meant to take long anyhow. @@ -97,15 +100,6 @@ export class AudioPlayerViewModel } } - protected addDownstreamSubscription(): void { - this.props.playback.on(UPDATE_EVENT, this.setSnapshot); - // There is no unsubscribe method in SimpleObservable - this.props.playback.clockInfo.liveData.onUpdate(this.setSnapshot); - } - protected removeDownstreamSubscription(): void { - this.props.playback.off(UPDATE_EVENT, this.setSnapshot); - } - /** * Sets the snapshot and emits an update to subscribers. */ From 1927a4b2356e99863a3b02095988ba07bbe4d151 Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Mon, 11 Aug 2025 11:03:40 +0530 Subject: [PATCH 08/12] Expose isDisposed through base vm --- src/viewmodels/base/BaseViewModel.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/viewmodels/base/BaseViewModel.ts b/src/viewmodels/base/BaseViewModel.ts index c6a9b05e156..a9ed6d2dc9b 100644 --- a/src/viewmodels/base/BaseViewModel.ts +++ b/src/viewmodels/base/BaseViewModel.ts @@ -41,4 +41,11 @@ export abstract class BaseViewModel implements ViewModel { public dispose(): void { this.disposables.dispose(); } + + /** + * Whether this view-model has been disposed. + */ + public get isDisposed(): boolean { + return this.disposables.isDisposed; + } } From c08e756854b8ce58e3afc3c2cc9abb2a9fbf769a Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Mon, 11 Aug 2025 15:09:20 +0530 Subject: [PATCH 09/12] Dispose AudioPlayerViewModel --- src/components/views/messages/MAudioBody.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/views/messages/MAudioBody.tsx b/src/components/views/messages/MAudioBody.tsx index d96a9368d3c..56c162b0d01 100644 --- a/src/components/views/messages/MAudioBody.tsx +++ b/src/components/views/messages/MAudioBody.tsx @@ -74,6 +74,7 @@ export default class MAudioBody extends React.PureComponent public componentWillUnmount(): void { this.state.playback?.destroy(); + this.state.audioPlayerVm?.dispose(); } protected get showFileBody(): boolean { From 14af8170374d68f742e287c77a5d4a3b8567adaf Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Tue, 12 Aug 2025 13:33:59 +0530 Subject: [PATCH 10/12] Add hook to auto dispose viewmodels --- .../base/useAutoDisposedViewModel.ts | 64 +++++++++++++++++++ .../base/useAutoDisposedViewModel-test.ts | 47 ++++++++++++++ 2 files changed, 111 insertions(+) create mode 100644 src/viewmodels/base/useAutoDisposedViewModel.ts create mode 100644 test/viewmodels/base/useAutoDisposedViewModel-test.ts diff --git a/src/viewmodels/base/useAutoDisposedViewModel.ts b/src/viewmodels/base/useAutoDisposedViewModel.ts new file mode 100644 index 00000000000..64300fcd734 --- /dev/null +++ b/src/viewmodels/base/useAutoDisposedViewModel.ts @@ -0,0 +1,64 @@ +/* +Copyright 2024 New Vector Ltd. +Copyright 2022 The Matrix.org Foundation C.I.C. + +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial +Please see LICENSE files in the repository root for full details. +*/ + +import { useEffect, useState } from "react"; + +import type { BaseViewModel } from "./BaseViewModel"; + +type VmCreator> = () => B; + +/** + * Instantiate a view-model that gets disposed when the calling react component unmounts. + * In other words, this hook ties the lifecycle of a view-model to the lifecycle of a + * react component. + * + * @param vmCreator A function that returns a view-model instance + * @returns view-model instance from vmCreator + * @example + * const vm = useAutoDisposedViewModel(() => new FooViewModel({prop1, prop2, ...}); + */ +export function useAutoDisposedViewModel>(vmCreator: VmCreator): B { + /** + * The view-model instance may be replaced by a different instance in some scenarios. + * We want to be sure that whatever react component called this hook gets re-rendered + * when this happens, hence the state. + */ + const [viewModel, setViewModel] = useState(vmCreator); + + /** + * Our intention here is to ensure that the dispose method of the view-model gets called + * when the component that uses this hook unmounts. + * We can do that by combining a useEffect cleanup with an empty dependency array. + */ + useEffect(() => { + let toDispose = viewModel; + + /** + * Because we use react strict mode, react will run our effects twice in dev mode to make + * sure that they are pure. + * This presents a complication - the vm instance that we created in our state initializer + * will get disposed on the first cleanup. + * So we'll recreate the view-model if it's already disposed. + */ + if (viewModel.isDisposed) { + const newViewModel = vmCreator(); + // Change toDispose so that we don't end up disposing the already disposed vm. + toDispose = newViewModel; + setViewModel(newViewModel); + } + return () => { + // Dispose the view-model when this component unmounts + toDispose.dispose(); + }; + + // eslint-disable-next-line react-compiler/react-compiler + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + return viewModel; +} diff --git a/test/viewmodels/base/useAutoDisposedViewModel-test.ts b/test/viewmodels/base/useAutoDisposedViewModel-test.ts new file mode 100644 index 00000000000..d77b3a9c374 --- /dev/null +++ b/test/viewmodels/base/useAutoDisposedViewModel-test.ts @@ -0,0 +1,47 @@ +/* +Copyright 2025 New Vector Ltd. +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial +Please see LICENSE files in the repository root for full details. +*/ + +import { renderHook } from "jest-matrix-react"; + +import { BaseViewModel } from "../../../src/viewmodels/base/BaseViewModel"; +import { useAutoDisposedViewModel } from "../../../src/viewmodels/base/useAutoDisposedViewModel"; + +class TestViewModel extends BaseViewModel<{ count: number }, { initial: number }> { + constructor(props: { initial: number }) { + super(props, { count: props.initial }); + } + + public increment() { + const newCount = this.getSnapshot().count + 1; + this.snapshot.set({ count: newCount }); + } +} + +describe("useAutoDisposedViewModel", () => { + it("should return view-model", () => { + const vmCreator = () => new TestViewModel({ initial: 0 }); + const { result } = renderHook(() => useAutoDisposedViewModel(vmCreator)); + const vm = result.current; + expect(vm).toBeInstanceOf(TestViewModel); + expect(vm.isDisposed).toStrictEqual(false); + }); + + it("should dispose view-model on unmount", () => { + const vmCreator = () => new TestViewModel({ initial: 0 }); + const { result, unmount } = renderHook(() => useAutoDisposedViewModel(vmCreator)); + const vm = result.current; + vm.increment(); + unmount(); + expect(vm.isDisposed).toStrictEqual(true); + }); + + it("should recreate view-model on react strict mode", async () => { + const vmCreator = () => new TestViewModel({ initial: 0 }); + const output = renderHook(() => useAutoDisposedViewModel(vmCreator), { reactStrictMode: true }); + const vm = output.result.current; + expect(vm.isDisposed).toStrictEqual(false); + }); +}); From a0a03148e43431bb9a1f3303934ac379dedf3b21 Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Tue, 12 Aug 2025 13:34:40 +0530 Subject: [PATCH 11/12] Use hook in TextualEventFactory --- src/events/EventTileFactory.tsx | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/events/EventTileFactory.tsx b/src/events/EventTileFactory.tsx index 57cafcbab15..aeba203ca9f 100644 --- a/src/events/EventTileFactory.tsx +++ b/src/events/EventTileFactory.tsx @@ -45,6 +45,7 @@ import ModuleApi from "../modules/Api"; import { TextualEventViewModel } from "../viewmodels/event-tiles/TextualEventViewModel"; import { TextualEventView } from "../shared-components/event-tiles/TextualEventView"; import { ElementCallEventType } from "../call-types"; +import { useAutoDisposedViewModel } from "../viewmodels/base/useAutoDisposedViewModel"; // Subset of EventTile's IProps plus some mixins export interface EventTileTypeProps @@ -79,10 +80,15 @@ const LegacyCallEventFactory: Factory ); const CallEventFactory: Factory = (ref, props) => ; -export const TextualEventFactory: Factory = (ref, props) => { - const vm = new TextualEventViewModel(props); + +const TextualEventComponent: React.FC = (props) => { + const vm = useAutoDisposedViewModel(() => new TextualEventViewModel(props)); return ; }; +export const TextualEventFactory: Factory = (ref, props) => { + return ; +}; + const VerificationReqFactory: Factory = (_ref, props) => ; const HiddenEventFactory: Factory = (ref, props) => ; From a76654ff221b4cfe4b1770c1752d8317d39b1a07 Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Tue, 12 Aug 2025 13:44:55 +0530 Subject: [PATCH 12/12] Fix license --- src/viewmodels/base/useAutoDisposedViewModel.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/viewmodels/base/useAutoDisposedViewModel.ts b/src/viewmodels/base/useAutoDisposedViewModel.ts index 64300fcd734..356324cf5b7 100644 --- a/src/viewmodels/base/useAutoDisposedViewModel.ts +++ b/src/viewmodels/base/useAutoDisposedViewModel.ts @@ -1,7 +1,5 @@ /* -Copyright 2024 New Vector Ltd. -Copyright 2022 The Matrix.org Foundation C.I.C. - +Copyright 2025 New Vector Ltd. SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE files in the repository root for full details. */