getUserState$ Helper Improvements (#8267)

* Block Sending Null to `getUser`

* Update Comments & Tests

Co-authored-by: Matt Gibson <mgibson@bitwarden.com>

* Update Comment

* Update Fake

---------

Co-authored-by: Matt Gibson <mgibson@bitwarden.com>

libs/common/spec/fake-state-provider.ts

@@ -1,5 +1,5 @@
 import { mock } from "jest-mock-extended";
-import { Observable, map } from "rxjs";
+import { Observable, map, of, switchMap, take } from "rxjs";
 
 import {
   GlobalState,
@@ -171,7 +171,30 @@ export class FakeStateProvider implements StateProvider {
     if (userId) {
       return this.getUser<T>(userId, keyDefinition).state$;
     }
-    return this.getActive<T>(keyDefinition).state$;
+
+    return this.getActive(keyDefinition).state$;
+  }
+
+  getUserStateOrDefault$<T>(
+    keyDefinition: KeyDefinition<T> | UserKeyDefinition<T>,
+    config: { userId: UserId | undefined; defaultValue?: T },
+  ): Observable<T> {
+    const { userId, defaultValue = null } = config;
+    if (isUserKeyDefinition(keyDefinition)) {
+      this.mock.getUserStateOrDefault$(keyDefinition, config);
+    } else {
+      this.mock.getUserStateOrDefault$(keyDefinition, config);
+    }
+    if (userId) {
+      return this.getUser<T>(userId, keyDefinition).state$;
+    }
+
+    return this.activeUserId$.pipe(
+      take(1),
+      switchMap((userId) =>
+        userId != null ? this.getUser(userId, keyDefinition).state$ : of(defaultValue),
+      ),
+    );
   }
 
   async setUserState<T>(

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

@@ -2,9 +2,9 @@
  * need to update test environment so structuredClone works appropriately
  * @jest-environment ../shared/test.environment.ts
  */
-import { of } from "rxjs";
+import { Observable, of } from "rxjs";
 
-import { trackEmissions } from "../../../../spec";
+import { awaitAsync, trackEmissions } from "../../../../spec";
 import { FakeAccountService, mockAccountServiceWith } from "../../../../spec/fake-account-service";
 import {
   FakeActiveUserStateProvider,
@@ -49,47 +49,111 @@ describe("DefaultStateProvider", () => {
     });
   });
 
+  describe.each([
+    [
+      "getUserState$",
+      (keyDefinition: KeyDefinition<string>, userId?: UserId) =>
+        sut.getUserState$(keyDefinition, userId),
+    ],
+    [
+      "getUserStateOrDefault$",
+      (keyDefinition: KeyDefinition<string>, userId?: UserId) =>
+        sut.getUserStateOrDefault$(keyDefinition, { userId: userId }),
+    ],
+  ])(
+    "Shared behavior for %s",
+    (
+      _testName: string,
+      methodUnderTest: (
+        keyDefinition: KeyDefinition<string>,
+        userId?: UserId,
+      ) => Observable<string>,
+    ) => {
+      const accountInfo = { email: "email", name: "name", status: AuthenticationStatus.LoggedOut };
+      const keyDefinition = new KeyDefinition<string>(new StateDefinition("test", "disk"), "test", {
+        deserializer: (s) => s,
+      });
+
+      it("should follow the specified user if userId is provided", async () => {
+        const state = singleUserStateProvider.getFake(userId, keyDefinition);
+        state.nextState("value");
+        const emissions = trackEmissions(methodUnderTest(keyDefinition, userId));
+
+        state.nextState("value2");
+        state.nextState("value3");
+
+        expect(emissions).toEqual(["value", "value2", "value3"]);
+      });
+
+      it("should follow the current active user if no userId is provided", async () => {
+        accountService.activeAccountSubject.next({ id: userId, ...accountInfo });
+        const state = singleUserStateProvider.getFake(userId, keyDefinition);
+        state.nextState("value");
+        const emissions = trackEmissions(methodUnderTest(keyDefinition));
+
+        state.nextState("value2");
+        state.nextState("value3");
+
+        expect(emissions).toEqual(["value", "value2", "value3"]);
+      });
+
+      it("should continue to follow the state of the user that was active when called, even if active user changes", async () => {
+        const state = singleUserStateProvider.getFake(userId, keyDefinition);
+        state.nextState("value");
+        const emissions = trackEmissions(methodUnderTest(keyDefinition));
+
+        accountService.activeAccountSubject.next({ id: "newUserId" as UserId, ...accountInfo });
+        const newUserEmissions = trackEmissions(sut.getUserState$(keyDefinition));
+        state.nextState("value2");
+        state.nextState("value3");
+
+        expect(emissions).toEqual(["value", "value2", "value3"]);
+        expect(newUserEmissions).toEqual([null]);
+      });
+    },
+  );
+
   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 follow the specified user if userId is provided", async () => {
+    it("should not emit any values until a truthy user id is supplied", async () => {
+      accountService.activeAccountSubject.next(null);
       const state = singleUserStateProvider.getFake(userId, keyDefinition);
-      state.nextState("value");
+      state.stateSubject.next([userId, "value"]);
-      const emissions = trackEmissions(sut.getUserState$(keyDefinition, userId));
 
-      state.nextState("value2");
+      const emissions = trackEmissions(sut.getUserState$(keyDefinition));
-      state.nextState("value3");
 
-      expect(emissions).toEqual(["value", "value2", "value3"]);
+      await awaitAsync();
-    });
+
+      expect(emissions).toHaveLength(0);
 
-    it("should follow the current active user if no userId is provided", async () => {
       accountService.activeAccountSubject.next({ id: userId, ...accountInfo });
-      const state = singleUserStateProvider.getFake(userId, keyDefinition);
-      state.nextState("value");
-      const emissions = trackEmissions(sut.getUserState$(keyDefinition));
 
-      state.nextState("value2");
+      await awaitAsync();
-      state.nextState("value3");
 
-      expect(emissions).toEqual(["value", "value2", "value3"]);
+      expect(emissions).toEqual(["value"]);
+    });
+  });
+
+  describe("getUserStateOrDefault$", () => {
+    const keyDefinition = new KeyDefinition<string>(new StateDefinition("test", "disk"), "test", {
+      deserializer: (s) => s,
     });
 
-    it("should continue to follow the state of the user that was active when called, even if active user changes", async () => {
+    it("should emit default value if no userId supplied and first active user id emission in falsy", async () => {
-      const state = singleUserStateProvider.getFake(userId, keyDefinition);
+      accountService.activeAccountSubject.next(null);
-      state.nextState("value");
-      const emissions = trackEmissions(sut.getUserState$(keyDefinition));
 
-      accountService.activeAccountSubject.next({ id: "newUserId" as UserId, ...accountInfo });
+      const emissions = trackEmissions(
-      const newUserEmissions = trackEmissions(sut.getUserState$(keyDefinition));
+        sut.getUserStateOrDefault$(keyDefinition, {
-      state.nextState("value2");
+          userId: undefined,
-      state.nextState("value3");
+          defaultValue: "I'm default!",
+        }),
+      );
 
-      expect(emissions).toEqual(["value", "value2", "value3"]);
+      expect(emissions).toEqual(["I'm default!"]);
-      expect(newUserEmissions).toEqual([null]);
     });
   });
 

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

@@ -1,4 +1,4 @@
-import { Observable, switchMap, take } from "rxjs";
+import { Observable, filter, of, switchMap, take } from "rxjs";
 
 import { UserId } from "../../../types/guid";
 import { DerivedStateDependencies } from "../../../types/state";
@@ -30,12 +30,30 @@ export class DefaultStateProvider implements StateProvider {
       return this.getUser<T>(userId, keyDefinition).state$;
     } else {
       return this.activeUserId$.pipe(
+        filter((userId) => userId != null), // Filter out null-ish user ids since we can't get state for a null user id
         take(1),
         switchMap((userId) => this.getUser<T>(userId, keyDefinition).state$),
       );
     }
   }
 
+  getUserStateOrDefault$<T>(
+    keyDefinition: KeyDefinition<T> | UserKeyDefinition<T>,
+    config: { userId: UserId | undefined; defaultValue?: T },
+  ): Observable<T> {
+    const { userId, defaultValue = null } = config;
+    if (userId) {
+      return this.getUser<T>(userId, keyDefinition).state$;
+    } else {
+      return this.activeUserId$.pipe(
+        take(1),
+        switchMap((userId) =>
+          userId != null ? this.getUser<T>(userId, keyDefinition).state$ : of(defaultValue),
+        ),
+      );
+    }
+  }
+
   async setUserState<T>(
     keyDefinition: KeyDefinition<T> | UserKeyDefinition<T>,
     value: T,

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

@@ -24,8 +24,11 @@ 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_.
+   * @remarks If userId is falsy the observable returned will attempt to 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.
+   * If no user happens to be active at the time this method is called with a falsy userId then this observable will not emit a value until
+   * a user becomes active. If you are not confident a user is active at the time this method is called, you may want to pipe a call to `timeout`
+   * or instead call {@link getUserStateOrDefault$} and supply a value you would rather have given in the case of no passed in userId and no active user.
    *
    * @note consider converting your {@link KeyDefinition} to a {@link UserKeyDefinition} for additional features.
    *
@@ -37,14 +40,49 @@ 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_.
+   * @remarks If userId is falsy the observable returned will attempt to 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.
+   * If no user happens to be active at the time this method is called with a falsy userId then this observable will not emit a value until
+   * a user becomes active. If you are not confident a user is active at the time this method is called, you may want to pipe a call to `timeout`
+   * or instead call {@link getUserStateOrDefault$} and supply a value you would rather have given in the case of no passed in userId and no active user.
    *
    * @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.
    */
   abstract getUserState$<T>(keyDefinition: UserKeyDefinition<T>, userId?: UserId): Observable<T>;
 
+  /**
+   * Gets a state observable for a given key and userId
+   *
+   * @remarks If userId is falsy the observable return will first attempt to point to the currently active user but will not follow subsequent active user changes,
+   * if there is no immediately available active user, then it will fallback to returning a default value in an observable that immediately completes.
+   *
+   * @note consider converting your {@link KeyDefinition} to a {@link UserKeyDefinition} for additional features.
+   *
+   * @param keyDefinition - The key definition for the state you want to get.
+   * @param config.userId - The userId for which you want the state for. If not provided, the state for the currently active user will be returned.
+   * @param config.defaultValue - The default value that should be wrapped in an observable if no active user is immediately available and no truthy userId is passed in.
+   */
+  abstract getUserStateOrDefault$<T>(
+    keyDefinition: KeyDefinition<T>,
+    config: { userId: UserId | undefined; defaultValue?: T },
+  ): Observable<T>;
+
+  /**
+   * Gets a state observable for a given key and userId
+   *
+   * @remarks If userId is falsy the observable return will first attempt to point to the currently active user but will not follow subsequent active user changes,
+   * if there is no immediately available active user, then it will fallback to returning a default value in an observable that immediately completes.
+   *
+   * @param keyDefinition - The key definition for the state you want to get.
+   * @param config.userId - The userId for which you want the state for. If not provided, the state for the currently active user will be returned.
+   * @param config.defaultValue - The default value that should be wrapped in an observable if no active user is immediately available and no truthy userId is passed in.
+   */
+  abstract getUserStateOrDefault$<T>(
+    keyDefinition: UserKeyDefinition<T>,
+    config: { userId: UserId | undefined; defaultValue?: T },
+  ): Observable<T>;
+
   /**
    * Sets the state for a given key and userId.
    *