MVVM Subscription issue test demo

src/events/EventTileFactory.tsx

@@ -44,7 +44,7 @@ import { ElementCall } from "../models/Call";
 import { type IBodyProps } from "../components/views/messages/IBodyProps";
 import ModuleApi from "../modules/Api";
 import { TextualEventViewModel } from "../viewmodels/event-tiles/TextualEventViewModel";
-import { TextualEvent } from "../shared-components/event-tiles/TextualEvent";
+import { TextualEventView } from "../shared-components/event-tiles/TextualEventView";
 
 // Subset of EventTile's IProps plus some mixins
 export interface EventTileTypeProps
@@ -81,7 +81,7 @@ const LegacyCallEventFactory: Factory<FactoryProps & { callEventGrouper: LegacyC
 const CallEventFactory: Factory = (ref, props) => <CallEvent ref={ref} {...props} />;
 export const TextualEventFactory: Factory = (ref, props) => {
     const vm = new TextualEventViewModel(props);
-    return <TextualEvent vm={vm} />;
+    return <TextualEventView vm={vm} />;
 };
 const VerificationReqFactory: Factory = (_ref, props) => <MKeyVerificationRequest {...props} />;
 const HiddenEventFactory: Factory = (ref, props) => <HiddenBody ref={ref} {...props} />;

src/shared-components/ViewModelSubIssueDemo.test.tsx

@@ -0,0 +1,182 @@
+/*
+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 React, { useState } from "react";
+import EventEmitter from "events";
+import { fireEvent, render } from "jest-matrix-react";
+
+import { BaseViewModel } from "../viewmodels/base/BaseViewModel";
+import type { ViewModel } from "./ViewModel";
+import { useViewModel } from "./useViewModel";
+
+/**
+ * View-models will need to listen to events so that it can do some necessary recalculation.
+ * Let's create an example of such an event emitter.
+ * Once start() is called and until stop() is called, MessageEmitter will continuously emit a "NEW_MESSAGE"
+ * event every 500ms.
+ */
+class MessageEmitter extends EventEmitter {
+    public count = 0;
+    private timeout?: NodeJS.Timeout;
+
+    public constructor() {
+        super();
+    }
+
+    public start(): void {
+        this.timeout = setInterval(() => {
+            this.count = this.count + 1;
+            this.emit("NEW_MESSAGE");
+        }, 500);
+    }
+
+    public stop(): void {
+        clearInterval(this.timeout);
+    }
+}
+
+/**
+ * We're going to create a message counter component that just renders the total message count.
+ */
+
+interface MessageCounterViewModelProps {
+    /**
+     * An emitter that lets the vm know when a new message has arrived.
+     */
+    emitter: MessageEmitter;
+}
+
+interface MessageCounterViewState {
+    /**
+     * The number of messages in this room
+     */
+    count: number;
+}
+
+/**
+ * This view model is written with the API that we have today.
+ */
+class MessageCounterViewModel extends BaseViewModel<MessageCounterViewState, MessageCounterViewModelProps> {
+    public constructor(props: MessageCounterViewModelProps) {
+        super(props, { count: 0 });
+    }
+
+    private onMessage = (): void => {
+        // Increase the count by 1 on new message event
+        const count = this.snapshot.current.count + 1;
+        this.snapshot.set({ count });
+    };
+
+    protected addDownstreamSubscription(): void {
+        this.props.emitter.on("NEW_MESSAGE", this.onMessage);
+    }
+
+    protected removeDownstreamSubscription(): void {
+        this.props.emitter.off("NEW_MESSAGE", this.onMessage);
+    }
+}
+
+/**
+ * This is the fixed version of above with the lifetime of the event listeners being
+ * equal to the lifetime of the view-model itself.
+ */
+class MessageCounterViewModelFixed extends BaseViewModel<MessageCounterViewState, MessageCounterViewModelProps> {
+    public constructor(props: MessageCounterViewModelProps) {
+        super(props, { count: 0 });
+        this.props.emitter.on("NEW_MESSAGE", this.onMessage);
+    }
+
+    private onMessage = (): void => {
+        // Increase the count by 1 on new message event
+        const count = this.snapshot.current.count + 1;
+        this.snapshot.set({ count });
+    };
+
+    protected addDownstreamSubscription(): void {}
+
+    protected removeDownstreamSubscription(): void {}
+}
+
+interface MessageCounterViewProps {
+    vm: ViewModel<MessageCounterViewState>;
+}
+const MessageCounterView: React.FC<MessageCounterViewProps> = ({ vm }) => {
+    const snapshot = useViewModel(vm);
+    // Nothing too interesting, just render the count from the vm.
+    return <div>{snapshot.count}</div>;
+};
+
+const emitter = new MessageEmitter();
+
+const RoomTileView = ({ vm }: { vm: ViewModel<MessageCounterViewState> }): React.ReactNode => {
+    const [hovered, setHovered] = useState(false);
+    // const vm = useMemo(() => new MessageCounterViewModelFixed({ emitter }), []);
+
+    /**
+     * This is similar to the room tile in the room list.
+     * It shows the count when your mouse cursor is outside the tile.
+     * It shows some icon (say 'i') when the mouse cursor is inside the tile. In our
+     * actual room tile in element web, this would be the notification options icon.
+     */
+    return (
+        <div
+            className="root"
+            onMouseEnter={() => {
+                setHovered(true);
+            }}
+            onMouseLeave={() => {
+                setHovered(false);
+            }}
+        >
+            <div className="avatar">F</div>
+            <div className="name">Foo Room</div>
+            <div className="icon">{hovered ? <div>i</div> : <MessageCounterView vm={vm} />}</div>
+        </div>
+    );
+};
+
+it.each([
+    ["Unfixed ViewModel", new MessageCounterViewModel({ emitter })],
+    ["Fixed ViewModel", new MessageCounterViewModelFixed({ emitter })],
+])(
+    "view has stale state demo, vm type = %s",
+    async (_, vm) => {
+        // 1. First let's just render our component
+        render(<RoomTileView vm={vm} />);
+        // 2. Let's instruct the emitter to start spawning new events
+        emitter.start();
+        // 3. Let's wait three seconds so that the counts can actually increment.
+        await new Promise((r) => setTimeout(r, 3000));
+        // 4. Stopping the emitter while we do our assert
+        emitter.stop();
+        // 5. We haven't moved our mouse inside the tile, so we expect the count
+        // in MessageCounterView component to match the total number of events that
+        // have been emitted so far.
+        expect(document.querySelector(".icon")).toHaveTextContent(`${emitter.count}`);
+
+        // 6. Let's start emitting events again
+        emitter.start();
+        // 7. This time, we're going to move the cursor into the tile
+        fireEvent.mouseEnter(document.querySelector(".root")!);
+        // 8. So now we expect the icon to be shown instead of message counter
+        expect(document.querySelector(".icon")).toHaveTextContent("i");
+        // 9. Let's say that the mouse is inside for 3 seconds
+        await new Promise((r) => setTimeout(r, 3000));
+        // 10. Let's move the cursor out of the tile
+        fireEvent.mouseLeave(document.querySelector(".root")!);
+        // 11. As before, the icon should be unmounted and the message counter should be shown again.
+        expect(document.querySelector(".icon")).not.toHaveTextContent("i");
+        // 12. Stop the emitter before assert
+        emitter.stop();
+
+        // 13. This is where the issue arises. We would expect the message counter to show the correct
+        // count. But it's going to be incorrect because the view model did not respond to any of the
+        // events from the emitter while the view was unmounted (when the icon 'i' was rendered).
+        expect(document.querySelector(".icon")).toHaveTextContent(`${emitter.count}`);
+    },
+    30000,
+);

src/shared-components/event-tiles/TextualEvent/TextualEvent.stories.tsx → src/shared-components/event-tiles/TextualEventView/TextualEventView.stories.tsx

@@ -8,15 +8,15 @@ Please see LICENSE files in the repository root for full details.
 import React from "react";
 import { type Meta, type StoryFn } from "@storybook/react-vite";
 
-import { TextualEvent as TextualEventComponent } from "./TextualEvent";
+import { TextualEventView as TextualEventComponent } from "./TextualEventView";
 import { MockViewModel } from "../../MockViewModel";
 
 export default {
     title: "Event/TextualEvent",
     component: TextualEventComponent,
     tags: ["autodocs"],
     args: {
-        vm: new MockViewModel("Dummy textual event text"),
+        vm: new MockViewModel({ content: "Dummy textual event text" }),
     },
 } as Meta<typeof TextualEventComponent>;
 

src/shared-components/event-tiles/TextualEvent/TextualEvent.test.tsx → src/shared-components/event-tiles/TextualEventView/TextualEventView.test.tsx

@@ -9,11 +9,11 @@ import { composeStories } from "@storybook/react-vite";
 import { render } from "jest-matrix-react";
 import React from "react";
 
-import * as stories from "./TextualEvent.stories.tsx";
+import * as stories from "./TextualEventView.stories.tsx";
 
 const { Default } = composeStories(stories);
 
-describe("TextualEvent", () => {
+describe("TextualEventView", () => {
     it("renders a textual event", () => {
         const { container } = render(<Default />);
         expect(container).toMatchSnapshot();

src/shared-components/event-tiles/TextualEvent/TextualEvent.tsx → src/shared-components/event-tiles/TextualEventView/TextualEventView.tsx

@@ -10,14 +10,15 @@ import React, { type ReactNode, type JSX } from "react";
 import { type ViewModel } from "../../ViewModel";
 import { useViewModel } from "../../useViewModel";
 
-export type TextualEventViewSnapshot = string | ReactNode;
+export type TextualEventViewSnapshot = {
+    content: string | ReactNode;
+};
 
 export interface Props {
     vm: ViewModel<TextualEventViewSnapshot>;
 }
 
-export function TextualEvent({ vm }: Props): JSX.Element {
-    const contents = useViewModel(vm);
-
-    return <div className="mx_TextualEvent">{contents}</div>;
+export function TextualEventView({ vm }: Props): JSX.Element {
+    const snapshot = useViewModel(vm);
+    return <div className="mx_TextualEvent">{snapshot.content}</div>;
 }

src/shared-components/event-tiles/TextualEvent/__snapshots__/TextualEvent.test.tsx.snap → src/shared-components/event-tiles/TextualEventView/__snapshots__/TextualEventView.test.tsx.snap

@@ -1,6 +1,6 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`TextualEvent renders a textual event 1`] = `
+exports[`TextualEventView renders a textual event 1`] = `
 <div>
   <div
     class="mx_TextualEvent"

src/shared-components/event-tiles/TextualEvent/index.ts → src/shared-components/event-tiles/TextualEventView/index.ts

@@ -5,4 +5,4 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com
 Please see LICENSE files in the repository root for full details.
 */
 
-export { TextualEvent } from "./TextualEvent";
+export { TextualEventView } from "./TextualEventView";

src/viewmodels/SubscriptionViewModel.ts → src/viewmodels/base/BaseViewModel.ts

@@ -5,17 +5,24 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com
 Please see LICENSE files in the repository root for full details.
 */
 
-import { type ViewModel } from "../shared-components/ViewModel";
+import { type ViewModel } from "../../shared-components/ViewModel";
+import { Snapshot } from "./Snapshot";
 import { ViewModelSubscriptions } from "./ViewModelSubscriptions";
 
-export abstract class SubscriptionViewModel<T> implements ViewModel<T> {
+export abstract class BaseViewModel<T, P> implements ViewModel<T> {
     protected subs: ViewModelSubscriptions;
+    protected snapshot: Snapshot<T>;
+    protected props: P;
 
-    protected constructor() {
+    protected constructor(props: P, initialSnapshot: T) {
+        this.props = props;
         this.subs = new ViewModelSubscriptions(
             this.addDownstreamSubscriptionWrapper,
             this.removeDownstreamSubscriptionWrapper,
         );
+        this.snapshot = new Snapshot(initialSnapshot, () => {
+            this.subs.emit();
+        });
     }
 
     public subscribe = (listener: () => void): (() => void) => {
@@ -52,5 +59,7 @@ export abstract class SubscriptionViewModel<T> implements ViewModel<T> {
     /**
      * Returns the current snapshot of the view model.
      */
-    public abstract getSnapshot: () => T;
+    public getSnapshot = (): T => {
+        return this.snapshot.current;
+    };
 }

src/viewmodels/base/Snapshot.ts

@@ -0,0 +1,43 @@
+/*
+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.
+*/
+
+/**
+ * This is the output of the viewmodel that the view consumes.
+ * Updating snapshot through this object will make react re-render
+ * components.
+ */
+export class Snapshot<T> {
+    public constructor(
+        private snapshot: T,
+        private emit: () => void,
+    ) {}
+
+    /**
+     * Replace current snapshot with a new snapshot value.
+     * @param snapshot New snapshot value
+     */
+    public set(snapshot: T): void {
+        this.snapshot = snapshot;
+        this.emit();
+    }
+
+    /**
+     * Update a part of the current snapshot by merging into the existing snapshot.
+     * @param snapshot A subset of the snapshot to merge into the current snapshot.
+     */
+    public merge(snapshot: Partial<T>): void {
+        this.snapshot = { ...this.snapshot, ...snapshot };
+        this.emit();
+    }
+
+    /**
+     * The current value of the snapshot.
+     */
+    public get current(): T {
+        return this.snapshot;
+    }
+}

src/viewmodels/event-tiles/TextualEventViewModel.ts

@@ -10,29 +10,25 @@ import { MatrixEventEvent } from "matrix-js-sdk/src/matrix";
 import { type EventTileTypeProps } from "../../events/EventTileFactory";
 import { MatrixClientPeg } from "../../MatrixClientPeg";
 import { textForEvent } from "../../TextForEvent";
-import { type TextualEventViewSnapshot } from "../../shared-components/event-tiles/TextualEvent/TextualEvent";
-import { SubscriptionViewModel } from "../SubscriptionViewModel";
+import { type TextualEventViewSnapshot } from "../../shared-components/event-tiles/TextualEventView/TextualEventView";
+import { BaseViewModel } from "../base/BaseViewModel";
 
-export class TextualEventViewModel extends SubscriptionViewModel<TextualEventViewSnapshot> {
-    public constructor(private eventTileProps: EventTileTypeProps) {
-        super();
+export class TextualEventViewModel extends BaseViewModel<TextualEventViewSnapshot, EventTileTypeProps> {
+    public constructor(props: EventTileTypeProps) {
+        super(props, { content: "" });
+        this.setTextFromEvent();
     }
 
-    protected addDownstreamSubscription = (): void => {
-        this.eventTileProps.mxEvent.on(MatrixEventEvent.SentinelUpdated, this.subs.emit);
+    private setTextFromEvent = (): void => {
+        const content = textForEvent(this.props.mxEvent, MatrixClientPeg.safeGet(), true, this.props.showHiddenEvents);
+        this.snapshot.set({ content });
     };
 
-    protected removeDownstreamSubscription = (): void => {
-        this.eventTileProps.mxEvent.off(MatrixEventEvent.SentinelUpdated, this.subs.emit);
+    protected addDownstreamSubscription = (): void => {
+        this.props.mxEvent.on(MatrixEventEvent.SentinelUpdated, this.setTextFromEvent);
     };
 
-    public getSnapshot = (): TextualEventViewSnapshot => {
-        const text = textForEvent(
-            this.eventTileProps.mxEvent,
-            MatrixClientPeg.safeGet(),
-            true,
-            this.eventTileProps.showHiddenEvents,
-        );
-        return text;
+    protected removeDownstreamSubscription = (): void => {
+        this.props.mxEvent.off(MatrixEventEvent.SentinelUpdated, this.setTextFromEvent);
     };
 }