Merge pull request #4013 from element-hq/valere/fix_repeated_click_to_unmute

feat(mute): add syncing state and disable toggle during async mute

src/state/CallViewModel/localMember/Publisher.test.ts

@@ -63,6 +63,7 @@ function createMockLocalTrack(source: Track.Source): LocalTrack {
 
 function createMockMuteState(enabled$: BehaviorSubject<boolean>): {
   enabled$: BehaviorSubject<boolean>;
+  syncing$: BehaviorSubject<boolean>;
   setHandler: (h: (enabled: boolean) => void) => void;
   unsetHandler: () => void;
 } {
@@ -70,6 +71,7 @@ function createMockMuteState(enabled$: BehaviorSubject<boolean>): {
 
   const ms = {
     enabled$,
+    syncing$: new BehaviorSubject(false),
     setHandler: vi.fn().mockImplementation((h: (enabled: boolean) => void) => {
       currentHandler = h;
     }),

src/state/CallViewModel/localMember/Publisher.ts

@@ -112,27 +112,49 @@ export class Publisher {
     this.logger.info("Local track published", localTrackPublication);
     const lkRoom = this.connection.livekitRoom;
     if (!this.shouldPublish) {
+      this.logger.debug("Not publishing, pausing upstream");
       this.pauseUpstreams(lkRoom, [localTrackPublication.source]).catch((e) => {
         this.logger.error(`Failed to pause upstreams`, e);
       });
     }
-    // also check the mute state and apply it
     if (localTrackPublication.source === Track.Source.Microphone) {
-      const enabled = this.muteStates.audio.enabled$.value;
-      lkRoom.localParticipant.setMicrophoneEnabled(enabled).catch((e) => {
-        this.logger.error(
-          `Failed to enable microphone track, enabled:${enabled}`,
-          e,
-        );
-      });
+      const muteState = this.muteStates.audio;
+      // skip this if a sync is in progress: enabled$ still reflects the old
+      // state while the handler is mid-flight, so the handler itself will apply
+      // the correct mute state once it completes.
+      if (!muteState.syncing$.value) {
+        const enabled = muteState.enabled$.value;
+        if (!enabled) {
+          this.logger.info(
+            "Local audio track just published but muted meanwhile, setting enabled to false",
+          );
+          lkRoom.localParticipant.setMicrophoneEnabled(false).catch((e) => {
+            this.logger.error(
+              `Failed to enable microphone track, enabled:${enabled}`,
+              e,
+            );
+          });
+        }
+      }
     } else if (localTrackPublication.source === Track.Source.Camera) {
-      const enabled = this.muteStates.video.enabled$.value;
-      lkRoom.localParticipant.setCameraEnabled(enabled).catch((e) => {
-        this.logger.error(
-          `Failed to enable camera track, enabled:${enabled}`,
-          e,
-        );
-      });
+      const muteState = this.muteStates.video;
+      // skip this if a sync is in progress: enabled$ still reflects the old
+      // state while the handler is mid-flight, so the handler itself will apply
+      // the correct mute state once it completes.
+      if (!muteState.syncing$.value) {
+        const enabled = muteState.enabled$.value;
+        if (!enabled) {
+          this.logger.info(
+            "Local video track just published but muted meanwhile, setting enabled to false",
+          );
+          lkRoom.localParticipant.setCameraEnabled(false).catch((e) => {
+            this.logger.error(
+              `Failed to enable camera track, enabled:${enabled}`,
+              e,
+            );
+          });
+        }
+      }
     }
   }
   /**

src/state/MuteStates.test.ts

@@ -92,6 +92,75 @@ describe("MuteState", () => {
     await flushPromises();
     expect(lastEnabled).toBe(true);
   });
+
+  test("should ignore toggle while syncing but still process set requests", async () => {
+    const deviceStub = {
+      available$: constant(
+        new Map<string, DeviceLabel>([
+          ["mic", { type: "name", name: "Microphone" }],
+        ]),
+      ),
+      selected$: constant({ id: "mic" }),
+      select(): void {},
+    } as unknown as MediaDevice<DeviceLabel, SelectedDevice>;
+
+    const muteState = new MuteState(
+      testScope,
+      deviceStub,
+      false,
+      constant(false),
+    );
+
+    const first = Promise.withResolvers<boolean>();
+    const second = Promise.withResolvers<boolean>();
+    const handler = vi
+      .fn<(desired: boolean) => Promise<boolean>>()
+      .mockImplementationOnce(async () => first.promise)
+      .mockImplementationOnce(async () => second.promise);
+    muteState.setHandler(handler);
+
+    const syncingValues: boolean[] = [];
+    muteState.syncing$.subscribe((syncing) => {
+      syncingValues.push(syncing);
+    });
+
+    let setEnabled: ((enabled: boolean) => void) | null = null;
+    muteState.setEnabled$.subscribe((setter) => {
+      setEnabled = setter;
+    });
+    let toggle: (() => void) | null = null;
+    muteState.toggle$.subscribe((toggleFn) => {
+      toggle = toggleFn;
+    });
+
+    await flushPromises();
+
+    // Start syncing by requesting unmute.
+    toggle!();
+    await flushPromises();
+    expect(handler).toHaveBeenCalledTimes(1);
+    expect(handler).toHaveBeenNthCalledWith(1, true);
+
+    // Toggle requests are ignored while syncing.
+    toggle!();
+    await flushPromises();
+    expect(handler).toHaveBeenCalledTimes(1);
+
+    // setEnabled still updates latest desired state while syncing (push-to-talk).
+    setEnabled!(false);
+    await flushPromises();
+    expect(handler).toHaveBeenCalledTimes(1);
+
+    first.resolve(true);
+    await flushPromises();
+    expect(handler).toHaveBeenCalledTimes(2);
+    expect(handler).toHaveBeenNthCalledWith(2, false);
+
+    second.resolve(false);
+    await flushPromises();
+    expect(syncingValues).toContain(true);
+    expect(syncingValues.at(-1)).toBe(false);
+  });
 });
 
 describe("MuteStates", () => {

src/state/MuteStates.ts

@@ -30,6 +30,7 @@ import { type Behavior, constant } from "./Behavior";
 
 interface MuteStateData {
   enabled$: Observable<boolean>;
+  syncing$: Observable<boolean>;
   set: ((enabled: boolean) => void) | null;
   toggle: (() => void) | null;
 }
@@ -79,33 +80,40 @@ export class MuteState<Label, Selected> {
           this.handler$.value(false).catch((err) => {
             logger.error("MuteState-disable: handler error", err);
           });
-          return { enabled$: of(false), set: null, toggle: null };
+          return {
+            enabled$: of(false),
+            syncing$: of(false),
+            set: null,
+            toggle: null,
+          };
         }
 
         // Assume the default value only once devices are actually connected
         let enabled = this.enabledByDefault;
         const set$ = new Subject<boolean>();
         const toggle$ = new Subject<void>();
+        const syncing$ = new BehaviorSubject(false);
         const desired$ = merge(set$, toggle$.pipe(map(() => !enabled)));
         const enabled$ = new Observable<boolean>((subscriber) => {
           subscriber.next(enabled);
           let latestDesired = this.enabledByDefault;
-          let syncing = false;
 
           const sync = async (): Promise<void> => {
-            if (enabled === latestDesired) syncing = false;
-            else {
+            if (enabled === latestDesired) {
+              syncing$.next(false);
+            } else {
               const previouslyEnabled = enabled;
+              syncing$.next(true);
               enabled = await firstValueFrom(
                 this.handler$.pipe(
                   switchMap(async (handler) => handler(latestDesired)),
                 ),
               );
               if (enabled === previouslyEnabled) {
-                syncing = false;
+                syncing$.next(false);
               } else {
                 subscriber.next(enabled);
-                syncing = true;
+                syncing$.next(true);
                 sync().catch((err) => {
                   // TODO: better error handling
                   logger.error("MuteState: handler error", err);
@@ -116,21 +124,28 @@ export class MuteState<Label, Selected> {
 
           const s = desired$.subscribe((desired) => {
             latestDesired = desired;
-            if (syncing === false) {
-              syncing = true;
+            if (syncing$.value === false) {
+              syncing$.next(true);
               sync().catch((err) => {
                 // TODO: better error handling
                 logger.error("MuteState: handler error", err);
               });
             }
           });
-          return (): void => s.unsubscribe();
+          return (): void => {
+            s.unsubscribe();
+            syncing$.complete();
+          };
         });
 
         return {
           set: (enabled: boolean): void => set$.next(enabled),
-          toggle: (): void => toggle$.next(),
+          toggle: (): void => {
+            if (syncing$.value) return;
+            toggle$.next();
+          },
           enabled$,
+          syncing$,
         };
       }),
     ),
@@ -147,6 +162,10 @@ export class MuteState<Label, Selected> {
     this.data$.pipe(map(({ toggle }) => toggle)),
   );
 
+  public readonly syncing$: Behavior<boolean> = this.scope.behavior(
+    this.data$.pipe(switchMap(({ syncing$ }) => syncing$)),
+  );
+
   public constructor(
     private readonly scope: ObservableScope,
     private readonly device: MediaDevice<Label, Selected>,