VIDCS-3273: Fix audio output selection crash in android Waiting Room #23

maikthomas · 2025-01-22T14:48:06Z

What is this PR doing?

Fixes crash when selecting audio output device in waiting room on android
Change UI to allow audio output UI even when not supported by showing "System Default" and allowing Test Speaker to run

How should this be manually tested?

Need to test quite thoroughly on different browsers including android and iOS
Test audio output UI in waiting room
Test Speaker Test in waiting room
Test audio output UI in Meeting Room
Test Speaker Test in Meeting Room

On supported devices:

set non-default audio output device
unplug device
open audio settings
expect new device (default) to be selected

What are the relevant tickets?

Resolves VIDCS-3273

[ ] Resolves a Known Issue.
[ ] If yes, did you remove the item from the docs/KNOWN_ISSUES.md?
[ ] Resolves an item reported in Issues.
If yes, which issue? Issue Number?
[ ] If yes, did you close the item in Issues?

maikthomas

🧑‍🍳

maikthomas · 2025-01-24T15:37:21Z

frontend/package.json

-    "@types/react": "^18.0.28",
+    "@types/react": "^18.3.3",


This was me trying to get non-empty react types DefinitelyTyped/DefinitelyTyped#13251 (comment)

maikthomas · 2025-01-24T15:38:24Z

frontend/src/Context/AudioOutputProvider/useAudioOutput/useAudioOutput.tsx

+import {
+  getActiveAudioOutputDevice as getVonageActiveAudioOutputDevice,
+  setAudioOutputDevice as setVonageAudioOutputDevice,
+} from '@vonage/client-sdk-video';
 import { useCallback, useEffect, useState } from 'react';
-import { getActiveAudioOutputDevice, setAudioOutputDevice } from '@vonage/client-sdk-video';

 export type AudioDeviceId = string | null | undefined;

 export type AudioOutputContextType = {
-  audioOutput: string | undefined | null;
-  setAudioOutput: (deviceId: AudioDeviceId) => Promise<void>;
+  currentAudioOutputDevice: string | undefined | null;
+  setAudioOutputDevice: (deviceId: AudioDeviceId) => Promise<void>;


Some light renaming/refactoring here. It was super confusing to know what updateAudioOutput setAudioOutput , setAudioOutputDevice etc all did and how they differed

maikthomas · 2025-01-24T15:42:20Z

frontend/src/components/MeetingRoom/AudioInputOutputDevices/InputDevices/InputDevices.tsx

 const InputDevices = ({ handleToggle, customLightBlueColor }: InputDevicesProps): ReactElement => {
-  const { publisher, isPublishing } = usePublisherContext();
-  const [devicesAvailable, setDevicesAvailable] = useState<Device[] | null>(null);
-  const { allMediaDevices } = useDevices();
-  const [options, setOptions] = useState<string[]>([]);
+  const { publisher } = usePublisherContext();
+  const {
+    allMediaDevices: { audioInputDevices },
+  } = useDevices();

-  useEffect(() => {
-    setDevicesAvailable(allMediaDevices.audioInputDevices);
-  }, [publisher, allMediaDevices, devicesAvailable, isPublishing]);
-  const changeAudioSource = (deviceId: string) => {
-    publisher?.setAudioSource(deviceId);
-  };
-
-  useEffect(() => {
-    if (devicesAvailable) {
-      const audioDevicesAvailable: string[] = devicesAvailable.map((availableDevice: Device) => {
-        return availableDevice.label;
-      });
-      setOptions(audioDevicesAvailable);
-    }
-  }, [devicesAvailable]);
+  const options = audioInputDevices.map((availableDevice: Device) => {
+    return availableDevice.label;
+  });



These affects and state were not needed and caused multiple renders and chained state effects.
The state comes directly from useDevices, and the state we need can be derived from that state directly, without setting state again. This is how the react docs tells us to do things.

maikthomas · 2025-01-24T15:43:04Z

frontend/src/components/MeetingRoom/AudioInputOutputDevices/OutputDevices/OutputDevices.tsx

-  const { audioOutput, setAudioOutput } = useAudioOutputContext();
-  const [devicesAvailable, setDevicesAvailable] = useState<AudioOutputDevice[] | null>(null);
-  const { allMediaDevices } = useDevices();
-
-  useEffect(() => {
-    setDevicesAvailable(allMediaDevices.audioOutputDevices);
-  }, [allMediaDevices]);
+  const { currentAudioOutputDevice, setAudioOutputDevice } = useAudioOutputContext();
+  const {
+    allMediaDevices: { audioOutputDevices },
+  } = useDevices();

-  const changeAudioOutput = async (deviceId: string) => {
-    await setAudioOutputDevice(deviceId);
-    setAudioOutput(deviceId);
-  };
+  const isAudioOutputSupported = isGetActiveAudioOutputDeviceSupported();

-  useEffect(() => {
-    const getActiveAudioOutputDeviceLabel = async () => {
-      const activeOutputDevice = await getActiveAudioOutputDevice();
-      setAudioOutput(activeOutputDevice.deviceId);
-    };
-    getActiveAudioOutputDeviceLabel();
-  }, [setAudioOutput]);
+  const availableDevices = isAudioOutputSupported ? audioOutputDevices : defaultOutputDevices;

-  const handleChangeAudioOutput = (event: MouseEvent<HTMLLIElement>) => {
+  const handleChangeAudioOutput = async (event: MouseEvent<HTMLLIElement>) => {


Same as the above file, a lot of this was not needed, made testing hard, and caused multiple render chains before the state was right, when the state could be derived from props/hooks directly.

maikthomas · 2025-01-24T15:55:11Z

frontend/src/Context/AudioOutputProvider/useAudioOutput/useAudioOutput.tsx

+  useEffect(() => {
+    // Add an event listener to update device list when the list changes (such as plugging or unplugging a device)
+    mediaDevices.addEventListener('devicechange', updateCurrentAudioOutputDevice);
+
+    return () => {
+      // Remove the event listener when component unmounts
+      mediaDevices.removeEventListener('devicechange', updateCurrentAudioOutputDevice);
+    };
+  }, [mediaDevices, updateCurrentAudioOutputDevice]);


I added this event listener to update the current device when the device list changes. This keeps the UI in sync if the current device was removed

cpettet

Looks good so far! Just have a few comments/questions. Let me know what you think!

cpettet · 2025-01-28T19:12:17Z

frontend/src/components/SoundTest/SoundTest.spec.tsx

+      play: playMock,
+      pause: pauseMock,
+      currentTime: 9001,
+      setSinkId: vi.fn(),


Should this be undefined? The test name doesn't match the mock.

Great catch! Luckily when I updated the value the test still passes 😅

cpettet · 2025-01-28T19:34:27Z

frontend/src/components/MeetingRoom/AudioInputOutputDevices/OutputDevices/OutputDevices.tsx

-          const isSelected = device.deviceId === audioOutput;
+      <MenuList data-testid="output-devices">
+        {availableDevices?.map((device: AudioOutputDevice) => {
+          // If audio output device selection is not support we show the default device as selected


behei-vonage

impressive job on this one! left a few questions, mostly just curious

behei-vonage · 2025-01-28T19:51:55Z

frontend/src/components/MeetingRoom/AudioInputOutputDevices/AudioInputOutputDevice.spec.tsx

+  mockGetAudioOutputDevices,
+  mockSetAudioOutputDevice,
+  mockGetActiveAudioOutputDevice,
+} = vi.hoisted(() => {


sorry just curious what's the benefit of using hoisted() here? why don't just mock()?

I want access to the mock functions (vi.fn()) that I use for the mocked object properties of the SDK, however if I define them inside vi.mock(() => { //here then they won't be accesible outside that scope.
If I just define them in the file then they won't have been declared yet when vi.mock( is called because mock is hoisted to the top of the file above the imports and all declarations.
That's why vitest exposes the vi.hoisted util to allow you to declare things before the vimock()s are called.
See the docs:

The call to vi.mock is hoisted, so it doesn't matter where you call it. It will always be executed before all imports. If you need to reference some variables outside of its scope, you can define them inside vi.hoisted and reference them inside vi.mock.

https://vitest.dev/api/vi#vi-mock

thanks for the explanation 🙏

behei-vonage · 2025-01-28T19:53:20Z

frontend/src/components/MeetingRoom/AudioInputOutputDevices/AudioInputOutputDevice.spec.tsx

@@ -61,12 +100,22 @@ describe('AudioInputOutputDevice Component', () => {
    );

    const outputDevicesElement = screen.getByTestId('output-devices');
-    expect(outputDevicesElement).toBeInTheDocument();
+    await waitFor(() => expect(outputDevicesElement.children).to.have.length(3));
+    const firstChild = outputDevicesElement.firstChild as HTMLOptionElement;


nit: can firstChild be renamed to something more self-explanatory? something like firstDeviceElement?

firstChild is an inherited property in the DOM. It's unlikely we'll be able to rename that one 😄

oh, thanks for the info! we can make consts be whatever we'd like them to be, though 🙂 it's just a nit though so if Mike prefers to keep it as a firstChild I'm okay with that

I've removed this variable and inlined the query.

It's a tricky one because firstDeviceElement or something like that might be more understandable, but this is a test and we are testing the firstChild. Whether that firstChild is a deviceElement depends on if the code is working 🤷

behei-vonage · 2025-01-28T19:53:57Z

frontend/src/components/MeetingRoom/AudioInputOutputDevices/AudioInputOutputDevice.spec.tsx

+
+    // Check initial list is correct
+    await waitFor(() => expect(outputDevicesElement.children).to.have.length(3));
+    const firstChild = outputDevicesElement.firstChild as HTMLOptionElement;


same nit here with the Child if you end up changing it 🙏

removed this variable

maikthomas

Responded to comments

maikthomas · 2025-01-29T11:53:30Z

frontend/src/components/MeetingRoom/AudioInputOutputDevices/AudioInputOutputDevice.spec.tsx

@@ -61,12 +100,22 @@ describe('AudioInputOutputDevice Component', () => {
    );

    const outputDevicesElement = screen.getByTestId('output-devices');
-    expect(outputDevicesElement).toBeInTheDocument();
+    await waitFor(() => expect(outputDevicesElement.children).to.have.length(3));
+    const firstChild = outputDevicesElement.firstChild as HTMLOptionElement;


I've removed this variable and inlined the query.

It's a tricky one because firstDeviceElement or something like that might be more understandable, but this is a test and we are testing the firstChild. Whether that firstChild is a deviceElement depends on if the code is working 🤷

maikthomas · 2025-01-29T11:53:40Z

frontend/src/components/MeetingRoom/AudioInputOutputDevices/AudioInputOutputDevice.spec.tsx

+
+    // Check initial list is correct
+    await waitFor(() => expect(outputDevicesElement.children).to.have.length(3));
+    const firstChild = outputDevicesElement.firstChild as HTMLOptionElement;


removed this variable

maikthomas · 2025-01-29T11:59:51Z

frontend/src/components/MeetingRoom/AudioInputOutputDevices/AudioInputOutputDevice.spec.tsx

+  mockGetAudioOutputDevices,
+  mockSetAudioOutputDevice,
+  mockGetActiveAudioOutputDevice,
+} = vi.hoisted(() => {


I want access to the mock functions (vi.fn()) that I use for the mocked object properties of the SDK, however if I define them inside vi.mock(() => { //here then they won't be accesible outside that scope.
If I just define them in the file then they won't have been declared yet when vi.mock( is called because mock is hoisted to the top of the file above the imports and all declarations.
That's why vitest exposes the vi.hoisted util to allow you to declare things before the vimock()s are called.
See the docs:

The call to vi.mock is hoisted, so it doesn't matter where you call it. It will always be executed before all imports. If you need to reference some variables outside of its scope, you can define them inside vi.hoisted and reference them inside vi.mock.

https://vitest.dev/api/vi#vi-mock

maikthomas · 2025-01-29T12:00:40Z

frontend/src/components/SoundTest/SoundTest.spec.tsx

+      play: playMock,
+      pause: pauseMock,
+      currentTime: 9001,
+      setSinkId: vi.fn(),


Great catch! Luckily when I updated the value the test still passes 😅

cpettet

LGTM Great job!

behei-vonage

LGTM!

dwivedisachin · 2025-01-30T18:06:03Z

Tested on Android device (Samsung S21 FE) LGTM!! 🚀

Try different approach for audio output device

1d2e635

maikthomas added the WIP Work in progress label Jan 22, 2025

maikthomas added 6 commits January 22, 2025 17:20

save

b9262c8

Refactor and extend tests

b847d7f

Extend test

8fe2e1c

Fix unsupported test case

467cac6

Add test for setSinkId being undefined

d04f57c

Fix tests again

449f9c8

dwivedisachin changed the base branch from main to develop January 24, 2025 10:22

dwivedisachin changed the title ~~VIDCS-3273: Fix audio output selection crash in android Waiting Room~~ VIDCS-3273: Fix audio output selection crash in android Waiting Room Jan 24, 2025

maikthomas added 4 commits January 24, 2025 12:04

Refactor audioOutputContext and add device change listener

e0747b0

Add test for updating device list

a01c20c

Fix imporst

445a82c

add comment

199c7a0

maikthomas commented Jan 24, 2025

View reviewed changes

maikthomas removed the WIP Work in progress label Jan 27, 2025

Merge branch 'develop' into mthomas/VIDCS-3273-waiting-room-output

ebacbbf

cpettet reviewed Jan 28, 2025

View reviewed changes

behei-vonage reviewed Jan 28, 2025

View reviewed changes

maikthomas added 2 commits January 29, 2025 13:01

remove variable

87abb97

typo and test fix

43bb33f

maikthomas commented Jan 29, 2025

View reviewed changes

cpettet approved these changes Jan 29, 2025

View reviewed changes

behei-vonage approved these changes Jan 29, 2025

View reviewed changes

v-kpheng merged commit acd7d37 into develop Jan 30, 2025
6 checks passed

v-kpheng deleted the mthomas/VIDCS-3273-waiting-room-output branch January 30, 2025 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VIDCS-3273: Fix audio output selection crash in android Waiting Room #23

VIDCS-3273: Fix audio output selection crash in android Waiting Room #23

maikthomas commented Jan 22, 2025

maikthomas left a comment

maikthomas Jan 24, 2025

maikthomas Jan 24, 2025

maikthomas Jan 24, 2025

maikthomas Jan 24, 2025

maikthomas Jan 24, 2025

cpettet left a comment

cpettet Jan 28, 2025

maikthomas Jan 29, 2025

cpettet Jan 28, 2025

behei-vonage left a comment

behei-vonage Jan 28, 2025

maikthomas Jan 29, 2025

behei-vonage Jan 29, 2025

behei-vonage Jan 28, 2025

cpettet Jan 28, 2025 •

edited

Loading

behei-vonage Jan 28, 2025

maikthomas Jan 29, 2025

behei-vonage Jan 28, 2025

maikthomas Jan 29, 2025

maikthomas left a comment

maikthomas Jan 29, 2025

maikthomas Jan 29, 2025

maikthomas Jan 29, 2025

maikthomas Jan 29, 2025

cpettet left a comment

behei-vonage left a comment

dwivedisachin commented Jan 30, 2025 •

edited

Loading

VIDCS-3273: Fix audio output selection crash in android Waiting Room #23

VIDCS-3273: Fix audio output selection crash in android Waiting Room #23

Conversation

maikthomas commented Jan 22, 2025

What is this PR doing?

How should this be manually tested?

What are the relevant tickets?

maikthomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpettet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behei-vonage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpettet Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maikthomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpettet left a comment

Choose a reason for hiding this comment

behei-vonage left a comment

Choose a reason for hiding this comment

dwivedisachin commented Jan 30, 2025 • edited Loading

cpettet Jan 28, 2025 •

edited

Loading

dwivedisachin commented Jan 30, 2025 •

edited

Loading