Require ObservableScopes of state holders to be specified explicitly

Previously we had a ViewModel class which was responsible for little more than creating an ObservableScope. However, since this ObservableScope would be created implicitly upon view model construction, it became a tad bit harder for callers to remember to eventually end the scope (as you wouldn't just have to remember to end ObservableScopes, but also to destroy ViewModels). Requiring the scope to be specified explicitly by the caller also makes it possible for the caller to reuse the scope for other purposes, reducing the number of scopes mentally in flight that need tending to, and for all state holders (not just view models) to be handled uniformly by helper functions such as generateKeyed$.

src/reactions/ReactionsReader.test.tsx

@@ -23,7 +23,7 @@ import {
   localRtcMember,
 } from "../utils/test-fixtures";
 import { getBasicRTCSession } from "../utils/test-viewmodel";
-import { withTestScheduler } from "../utils/test";
+import { testScope, withTestScheduler } from "../utils/test";
 import { ElementCallReactionEventType, ReactionSet } from ".";
 
 afterEach(() => {
@@ -37,6 +37,7 @@ test("handles a hand raised reaction", () => {
   withTestScheduler(({ schedule, expectObservable }) => {
     renderHook(() => {
       const { raisedHands$ } = new ReactionsReader(
+        testScope(),
         rtcSession.asMockedSession(),
       );
       schedule("ab", {
@@ -85,6 +86,7 @@ test("handles a redaction", () => {
   withTestScheduler(({ schedule, expectObservable }) => {
     renderHook(() => {
       const { raisedHands$ } = new ReactionsReader(
+        testScope(),
         rtcSession.asMockedSession(),
       );
       schedule("abc", {
@@ -148,6 +150,7 @@ test("handles waiting for event decryption", () => {
   withTestScheduler(({ schedule, expectObservable }) => {
     renderHook(() => {
       const { raisedHands$ } = new ReactionsReader(
+        testScope(),
         rtcSession.asMockedSession(),
       );
       schedule("abc", {
@@ -217,6 +220,7 @@ test("hands rejecting events without a proper membership", () => {
   withTestScheduler(({ schedule, expectObservable }) => {
     renderHook(() => {
       const { raisedHands$ } = new ReactionsReader(
+        testScope(),
         rtcSession.asMockedSession(),
       );
       schedule("ab", {
@@ -261,7 +265,10 @@ test("handles a reaction", () => {
 
   withTestScheduler(({ schedule, time, expectObservable }) => {
     renderHook(() => {
-      const { reactions$ } = new ReactionsReader(rtcSession.asMockedSession());
+      const { reactions$ } = new ReactionsReader(
+        testScope(),
+        rtcSession.asMockedSession(),
+      );
       schedule(`abc`, {
         a: () => {},
         b: () => {
@@ -317,7 +324,10 @@ test("ignores bad reaction events", () => {
 
   withTestScheduler(({ schedule, expectObservable }) => {
     renderHook(() => {
-      const { reactions$ } = new ReactionsReader(rtcSession.asMockedSession());
+      const { reactions$ } = new ReactionsReader(
+        testScope(),
+        rtcSession.asMockedSession(),
+      );
       schedule("ab", {
         a: () => {},
         b: () => {
@@ -439,7 +449,10 @@ test("that reactions cannot be spammed", () => {
 
   withTestScheduler(({ schedule, expectObservable }) => {
     renderHook(() => {
-      const { reactions$ } = new ReactionsReader(rtcSession.asMockedSession());
+      const { reactions$ } = new ReactionsReader(
+        testScope(),
+        rtcSession.asMockedSession(),
+      );
       schedule("abcd", {
         a: () => {},
         b: () => {

src/reactions/ReactionsReader.ts

@@ -18,7 +18,7 @@ import {
   EventType,
   RoomEvent as MatrixRoomEvent,
 } from "matrix-js-sdk";
-import { BehaviorSubject, delay, type Subscription } from "rxjs";
+import { BehaviorSubject, delay } from "rxjs";
 
 import {
   ElementCallReactionEventType,
@@ -28,6 +28,7 @@ import {
   type RaisedHandInfo,
   type ReactionInfo,
 } from ".";
+import { type ObservableScope } from "../state/ObservableScope";
 
 export const REACTION_ACTIVE_TIME_MS = 3000;
 
@@ -54,12 +55,13 @@ export class ReactionsReader {
    */
   public readonly reactions$ = this.reactionsSubject$.asObservable();
 
-  private readonly reactionsSub: Subscription;
-
-  public constructor(private readonly rtcSession: MatrixRTCSession) {
+  public constructor(
+    private readonly scope: ObservableScope,
+    private readonly rtcSession: MatrixRTCSession,
+  ) {
     // Hide reactions after a given time.
-    this.reactionsSub = this.reactionsSubject$
-      .pipe(delay(REACTION_ACTIVE_TIME_MS))
+    this.reactionsSubject$
+      .pipe(delay(REACTION_ACTIVE_TIME_MS), this.scope.bind())
       .subscribe((reactions) => {
         const date = new Date();
         const nextEntries = Object.fromEntries(
@@ -71,15 +73,38 @@ export class ReactionsReader {
         this.reactionsSubject$.next(nextEntries);
       });
 
+    // TODO: Convert this class to the functional reactive style and get rid of
+    // all this manual setup and teardown for event listeners
+
     this.rtcSession.room.on(MatrixRoomEvent.Timeline, this.handleReactionEvent);
+    this.scope.onEnd(() =>
+      this.rtcSession.room.off(
+        MatrixRoomEvent.Timeline,
+        this.handleReactionEvent,
+      ),
+    );
+
     this.rtcSession.room.on(
       MatrixRoomEvent.Redaction,
       this.handleReactionEvent,
     );
+    this.scope.onEnd(() =>
+      this.rtcSession.room.off(
+        MatrixRoomEvent.Redaction,
+        this.handleReactionEvent,
+      ),
+    );
+
     this.rtcSession.room.client.on(
       MatrixEventEvent.Decrypted,
       this.handleReactionEvent,
     );
+    this.scope.onEnd(() =>
+      this.rtcSession.room.client.off(
+        MatrixEventEvent.Decrypted,
+        this.handleReactionEvent,
+      ),
+    );
 
     // We listen for a local echo to get the real event ID, as timeline events
     // may still be sending.
@@ -87,11 +112,23 @@ export class ReactionsReader {
       MatrixRoomEvent.LocalEchoUpdated,
       this.handleReactionEvent,
     );
+    this.scope.onEnd(() =>
+      this.rtcSession.room.off(
+        MatrixRoomEvent.LocalEchoUpdated,
+        this.handleReactionEvent,
+      ),
+    );
 
-    rtcSession.on(
+    this.rtcSession.on(
       MatrixRTCSessionEvent.MembershipsChanged,
       this.onMembershipsChanged,
     );
+    this.scope.onEnd(() =>
+      this.rtcSession.off(
+        MatrixRTCSessionEvent.MembershipsChanged,
+        this.onMembershipsChanged,
+      ),
+    );
 
     // Run this once to ensure we have fetched the state from the call.
     this.onMembershipsChanged([]);
@@ -309,31 +346,4 @@ export class ReactionsReader {
       this.removeRaisedHand(targetUser);
     }
   };
-
-  /**
-   * Stop listening for events.
-   */
-  public destroy(): void {
-    this.rtcSession.off(
-      MatrixRTCSessionEvent.MembershipsChanged,
-      this.onMembershipsChanged,
-    );
-    this.rtcSession.room.off(
-      MatrixRoomEvent.Timeline,
-      this.handleReactionEvent,
-    );
-    this.rtcSession.room.off(
-      MatrixRoomEvent.Redaction,
-      this.handleReactionEvent,
-    );
-    this.rtcSession.room.client.off(
-      MatrixEventEvent.Decrypted,
-      this.handleReactionEvent,
-    );
-    this.rtcSession.room.off(
-      MatrixRoomEvent.LocalEchoUpdated,
-      this.handleReactionEvent,
-    );
-    this.reactionsSub.unsubscribe();
-  }
 }