diff --git a/src/events/EventTileFactory.tsx b/src/events/EventTileFactory.tsx index 9d0daadb80a..9187e05546b 100644 --- a/src/events/EventTileFactory.tsx +++ b/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 ; export const TextualEventFactory: Factory = (ref, props) => { const vm = new TextualEventViewModel(props); - return ; + return ; }; const VerificationReqFactory: Factory = (_ref, props) => ; const HiddenEventFactory: Factory = (ref, props) => ; diff --git a/src/shared-components/ViewModelSubIssueDemo.test.tsx b/src/shared-components/ViewModelSubIssueDemo.test.tsx new file mode 100644 index 00000000000..7f320517319 --- /dev/null +++ b/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 { + 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 { + 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; +} +const MessageCounterView: React.FC = ({ vm }) => { + const snapshot = useViewModel(vm); + // Nothing too interesting, just render the count from the vm. + return
{snapshot.count}
; +}; + +const emitter = new MessageEmitter(); + +const RoomTileView = ({ vm }: { vm: ViewModel }): 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 ( +
{ + setHovered(true); + }} + onMouseLeave={() => { + setHovered(false); + }} + > +
F
+
Foo Room
+
{hovered ?
i
: }
+
+ ); +}; + +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(); + // 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, +); diff --git a/src/shared-components/event-tiles/TextualEvent/TextualEvent.stories.tsx b/src/shared-components/event-tiles/TextualEventView/TextualEventView.stories.tsx similarity index 80% rename from src/shared-components/event-tiles/TextualEvent/TextualEvent.stories.tsx rename to src/shared-components/event-tiles/TextualEventView/TextualEventView.stories.tsx index 1746bc14b23..836641a84c4 100644 --- a/src/shared-components/event-tiles/TextualEvent/TextualEvent.stories.tsx +++ b/src/shared-components/event-tiles/TextualEventView/TextualEventView.stories.tsx @@ -8,7 +8,7 @@ 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 { @@ -16,7 +16,7 @@ export default { component: TextualEventComponent, tags: ["autodocs"], args: { - vm: new MockViewModel("Dummy textual event text"), + vm: new MockViewModel({ content: "Dummy textual event text" }), }, } as Meta; diff --git a/src/shared-components/event-tiles/TextualEvent/TextualEvent.test.tsx b/src/shared-components/event-tiles/TextualEventView/TextualEventView.test.tsx similarity index 84% rename from src/shared-components/event-tiles/TextualEvent/TextualEvent.test.tsx rename to src/shared-components/event-tiles/TextualEventView/TextualEventView.test.tsx index b1ef5e8f525..5d2dd912efb 100644 --- a/src/shared-components/event-tiles/TextualEvent/TextualEvent.test.tsx +++ b/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(); expect(container).toMatchSnapshot(); diff --git a/src/shared-components/event-tiles/TextualEvent/TextualEvent.tsx b/src/shared-components/event-tiles/TextualEventView/TextualEventView.tsx similarity index 63% rename from src/shared-components/event-tiles/TextualEvent/TextualEvent.tsx rename to src/shared-components/event-tiles/TextualEventView/TextualEventView.tsx index 1dec80905ec..fa18a4c599b 100644 --- a/src/shared-components/event-tiles/TextualEvent/TextualEvent.tsx +++ b/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; } -export function TextualEvent({ vm }: Props): JSX.Element { - const contents = useViewModel(vm); - - return
{contents}
; +export function TextualEventView({ vm }: Props): JSX.Element { + const snapshot = useViewModel(vm); + return
{snapshot.content}
; } diff --git a/src/shared-components/event-tiles/TextualEvent/__snapshots__/TextualEvent.test.tsx.snap b/src/shared-components/event-tiles/TextualEventView/__snapshots__/TextualEventView.test.tsx.snap similarity index 70% rename from src/shared-components/event-tiles/TextualEvent/__snapshots__/TextualEvent.test.tsx.snap rename to src/shared-components/event-tiles/TextualEventView/__snapshots__/TextualEventView.test.tsx.snap index 186e5f0afd8..a0be215af7b 100644 --- a/src/shared-components/event-tiles/TextualEvent/__snapshots__/TextualEvent.test.tsx.snap +++ b/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`] = `
implements ViewModel { +export abstract class BaseViewModel implements ViewModel { protected subs: ViewModelSubscriptions; + protected snapshot: Snapshot; + 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 implements ViewModel { /** * Returns the current snapshot of the view model. */ - public abstract getSnapshot: () => T; + public getSnapshot = (): T => { + return this.snapshot.current; + }; } diff --git a/src/viewmodels/base/Snapshot.ts b/src/viewmodels/base/Snapshot.ts new file mode 100644 index 00000000000..e8d0b7412ce --- /dev/null +++ b/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 { + 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): void { + this.snapshot = { ...this.snapshot, ...snapshot }; + this.emit(); + } + + /** + * The current value of the snapshot. + */ + public get current(): T { + return this.snapshot; + } +} diff --git a/src/viewmodels/ViewModelSubscriptions.ts b/src/viewmodels/base/ViewModelSubscriptions.ts similarity index 100% rename from src/viewmodels/ViewModelSubscriptions.ts rename to src/viewmodels/base/ViewModelSubscriptions.ts diff --git a/src/viewmodels/event-tiles/TextualEventViewModel.ts b/src/viewmodels/event-tiles/TextualEventViewModel.ts index d2f56482d70..40121ecf2b7 100644 --- a/src/viewmodels/event-tiles/TextualEventViewModel.ts +++ b/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 { - public constructor(private eventTileProps: EventTileTypeProps) { - super(); +export class TextualEventViewModel extends BaseViewModel { + 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); }; } diff --git a/test/viewmodels/base/Snapshot-test.ts b/test/viewmodels/base/Snapshot-test.ts new file mode 100644 index 00000000000..796caa65ab4 --- /dev/null +++ b/test/viewmodels/base/Snapshot-test.ts @@ -0,0 +1,41 @@ +/* +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 { Snapshot } from "../../../src/viewmodels/base/Snapshot"; + +interface TestSnapshot { + key1: string; + key2: number; + key3: boolean; +} + +describe("Snapshot", () => { + it("should accept an initial value", () => { + const snapshot = new Snapshot({ key1: "foo", key2: 5, key3: false }, jest.fn()); + expect(snapshot.current).toStrictEqual({ key1: "foo", key2: 5, key3: false }); + }); + + it("should call emit callback when state changes", () => { + const emit = jest.fn(); + const snapshot = new Snapshot({ key1: "foo", key2: 5, key3: false }, emit); + snapshot.merge({ key3: true }); + expect(emit).toHaveBeenCalledTimes(1); + }); + + it("should swap out entire snapshot on set call", () => { + const snapshot = new Snapshot({ key1: "foo", key2: 5, key3: false }, jest.fn()); + const newValue = { key1: "bar", key2: 8, key3: true }; + snapshot.set(newValue); + expect(snapshot.current).toStrictEqual(newValue); + }); + + it("should merge partial snapshot on merge call", () => { + const snapshot = new Snapshot({ key1: "foo", key2: 5, key3: false }, jest.fn()); + snapshot.merge({ key2: 10 }); + expect(snapshot.current).toStrictEqual({ key1: "foo", key2: 10, key3: false }); + }); +}); diff --git a/test/viewmodels/event-tiles/TextualEventViewModel-test.ts b/test/viewmodels/event-tiles/TextualEventViewModel-test.ts index 92037df9fc4..735ecd25420 100644 --- a/test/viewmodels/event-tiles/TextualEventViewModel-test.ts +++ b/test/viewmodels/event-tiles/TextualEventViewModel-test.ts @@ -8,10 +8,16 @@ Please see LICENSE files in the repository root for full details. import { MatrixEvent, MatrixEventEvent } from "matrix-js-sdk/src/matrix"; import { TextualEventViewModel } from "../../../src/viewmodels/event-tiles/TextualEventViewModel"; +import { stubClient } from "../../test-utils"; + +jest.mock("../../../src/TextForEvent.tsx", () => ({ + textForEvent: jest.fn().mockReturnValue("Test Message"), +})); describe("TextualEventViewModel", () => { it("should update when the sentinel updates", () => { const fakeEvent = new MatrixEvent({}); + stubClient(); const vm = new TextualEventViewModel({ showHiddenEvents: false,