Prefer get methods to return single user states unless specified active (#7834)

* Prefer get methods to return single user states unless specified active

* Improve comment

libs/common/spec/fake-account-service.ts

@@ -48,32 +48,24 @@ export class FakeAccountService implements AccountService {
   }
 
   async addAccount(userId: UserId, accountData: AccountInfo): Promise<void> {
-    // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
+    const current = this.accountsSubject["_buffer"][0] ?? {};
-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
+    this.accountsSubject.next({ ...current, [userId]: accountData });
-    this.mock.addAccount(userId, accountData);
+    await this.mock.addAccount(userId, accountData);
   }
 
   async setAccountName(userId: UserId, name: string): Promise<void> {
-    // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
+    await this.mock.setAccountName(userId, name);
-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    this.mock.setAccountName(userId, name);
   }
 
   async setAccountEmail(userId: UserId, email: string): Promise<void> {
-    // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
+    await this.mock.setAccountEmail(userId, email);
-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    this.mock.setAccountEmail(userId, email);
   }
 
   async setAccountStatus(userId: UserId, status: AuthenticationStatus): Promise<void> {
-    // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
+    await this.mock.setAccountStatus(userId, status);
-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    this.mock.setAccountStatus(userId, status);
   }
 
   async switchAccount(userId: UserId): Promise<void> {
-    // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
+    await this.mock.switchAccount(userId);
-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    this.mock.switchAccount(userId);
   }
 }

libs/common/src/platform/state/implementations/default-state.provider.spec.ts

@@ -1,5 +1,10 @@
+/**
+ * need to update test environment so structuredClone works appropriately
+ * @jest-environment ../shared/test.environment.ts
+ */
 import { of } from "rxjs";
 
+import { trackEmissions } from "../../../../spec";
 import { FakeAccountService, mockAccountServiceWith } from "../../../../spec/fake-account-service";
 import {
   FakeActiveUserStateProvider,
@@ -7,6 +12,7 @@ import {
   FakeGlobalStateProvider,
   FakeSingleUserStateProvider,
 } from "../../../../spec/fake-state-provider";
+import { AuthenticationStatus } from "../../../auth/enums/authentication-status";
 import { UserId } from "../../../types/guid";
 import { DeriveDefinition } from "../derive-definition";
 import { KeyDefinition } from "../key-definition";
@@ -44,30 +50,46 @@ describe("DefaultStateProvider", () => {
   });
 
   describe("getUserState$", () => {
+    const accountInfo = { email: "email", name: "name", status: AuthenticationStatus.LoggedOut };
     const keyDefinition = new KeyDefinition<string>(new StateDefinition("test", "disk"), "test", {
       deserializer: (s) => s,
     });
 
-    it("should get the state for the active user if no userId is provided", () => {
+    it("should follow the specified user if userId is provided", async () => {
-      const state = sut.getUserState$(keyDefinition);
+      const state = singleUserStateProvider.getFake(userId, keyDefinition);
-      expect(state).toBe(activeUserStateProvider.get(keyDefinition).state$);
+      state.nextState("value");
+      const emissions = trackEmissions(sut.getUserState$(keyDefinition, userId));
+
+      state.nextState("value2");
+      state.nextState("value3");
+
+      expect(emissions).toEqual(["value", "value2", "value3"]);
     });
 
-    it("should not return state for a single user if no userId is provided", () => {
+    it("should follow the current active user if no userId is provided", async () => {
-      const state = sut.getUserState$(keyDefinition);
+      accountService.activeAccountSubject.next({ id: userId, ...accountInfo });
-      expect(state).not.toBe(singleUserStateProvider.get(userId, keyDefinition).state$);
+      const state = singleUserStateProvider.getFake(userId, keyDefinition);
+      state.nextState("value");
+      const emissions = trackEmissions(sut.getUserState$(keyDefinition));
+
+      state.nextState("value2");
+      state.nextState("value3");
+
+      expect(emissions).toEqual(["value", "value2", "value3"]);
     });
 
-    it("should get the state for the provided userId", () => {
+    it("should continue to follow the state of the user that was active when called, even if active user changes", async () => {
-      const userId = "user" as UserId;
+      const state = singleUserStateProvider.getFake(userId, keyDefinition);
-      const state = sut.getUserState$(keyDefinition, userId);
+      state.nextState("value");
-      expect(state).toBe(singleUserStateProvider.get(userId, keyDefinition).state$);
+      const emissions = trackEmissions(sut.getUserState$(keyDefinition));
-    });
 
-    it("should not get the active user state if userId is provided", () => {
+      accountService.activeAccountSubject.next({ id: "newUserId" as UserId, ...accountInfo });
-      const userId = "user" as UserId;
+      const newUserEmissions = trackEmissions(sut.getUserState$(keyDefinition));
-      const state = sut.getUserState$(keyDefinition, userId);
+      state.nextState("value2");
-      expect(state).not.toBe(activeUserStateProvider.get(keyDefinition).state$);
+      state.nextState("value3");
+
+      expect(emissions).toEqual(["value", "value2", "value3"]);
+      expect(newUserEmissions).toEqual([null]);
     });
   });
 

libs/common/src/platform/state/implementations/default-state.provider.ts

@@ -1,4 +1,4 @@
-import { Observable } from "rxjs";
+import { Observable, switchMap, take } from "rxjs";
 
 import { UserId } from "../../../types/guid";
 import { DerivedStateDependencies } from "../../../types/state";
@@ -25,7 +25,10 @@ export class DefaultStateProvider implements StateProvider {
     if (userId) {
       return this.getUser<T>(userId, keyDefinition).state$;
     } else {
-      return this.getActive<T>(keyDefinition).state$;
+      return this.activeUserId$.pipe(
+        take(1),
+        switchMap((userId) => this.getUser<T>(userId, keyDefinition).state$),
+      );
     }
   }
 

libs/common/src/platform/state/state.provider.ts

@@ -22,6 +22,9 @@ export abstract class StateProvider {
   /**
    * Gets a state observable for a given key and userId.
    *
+   * @remarks If userId is falsy the observable returned will point to the currently active user _and not update if the active user changes_.
+   * This is different to how `getActive` works and more similar to `getUser` for whatever user happens to be active at the time of the call.
+   *
    * @param keyDefinition - The key definition for the state you want to get.
    * @param userId - The userId for which you want the state for. If not provided, the state for the currently active user will be returned.
    */