-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Speech to speech: Mute functionality #5688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
3c5d7e7
aec1249
14da8a4
f1259fb
f35f158
a8e96b8
f266871
ab82cb6
c08c225
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import useWebChatAPIContext from './internal/useWebChatAPIContext'; | ||
|
|
||
| /** | ||
| * Hook to mute voice mode (stops microphone input but keeps connection alive with silent chunks). | ||
| * The session remains active and can be unmuted to resume listening. | ||
| */ | ||
| export default function useMuteVoice(): () => void { | ||
| return useWebChatAPIContext().muteVoice; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import useWebChatAPIContext from './internal/useWebChatAPIContext'; | ||
|
|
||
| /** | ||
| * Hook to unmute voice mode (resumes microphone input after muting). | ||
| * This reactivates speech-to-speech listening. | ||
| */ | ||
| export default function useUnmuteVoice(): () => void { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you considered a single hook?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Initially I created single hook to return voice with state but then William suggested to keep individual hook for each export and same followed for start voice, stop voice and voice state
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deeper reason... one-liner: Let's say, in parallel universe, we have
The behavior will become quite undeterministic... says, we have a microphone button that essentially a push button like When the user click on the push button, it will not be pushed/checked because the getter is still returning This is the main reason why In this case, if the "mute" is instant/synchronous, which I believe so, we should do it the normal way, i.e. |
||
| return useWebChatAPIContext().unmuteVoice; | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ export function VoiceRecorderBridge(): null { | |||||
| const [voiceState] = useVoiceState(); | ||||||
| const postVoiceActivity = usePostVoiceActivity(); | ||||||
|
|
||||||
| const muted = voiceState === 'muted'; | ||||||
| // Derive recording state from voiceState - recording is active when not idle | ||||||
| const recording = voiceState !== 'idle'; | ||||||
|
|
||||||
|
|
@@ -29,7 +30,13 @@ export function VoiceRecorderBridge(): null { | |||||
| [postVoiceActivity] | ||||||
| ); | ||||||
|
|
||||||
| const { record } = useRecorder(handleAudioChunk); | ||||||
| const { record, mute } = useRecorder(handleAudioChunk); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sort.
Suggested change
|
||||||
|
|
||||||
| useEffect(() => { | ||||||
| if (muted) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious, how about Can we put it here so things is symmetric and easier to debug? |
||||||
| return mute(); | ||||||
| } | ||||||
| }, [mute, muted]); | ||||||
|
|
||||||
| useEffect(() => { | ||||||
| if (recording) { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,13 +36,16 @@ const mockWorkletNode = { | |
| port: mockWorkletPort | ||
| }; | ||
|
|
||
| const mockSourceNode = { | ||
| connect: jest.fn(), | ||
| disconnect: jest.fn() | ||
| }; | ||
|
|
||
| const mockAudioContext = { | ||
| audioWorklet: { | ||
| addModule: jest.fn().mockResolvedValue(undefined) | ||
| }, | ||
| createMediaStreamSource: jest.fn(() => ({ | ||
| connect: jest.fn() | ||
| })), | ||
| createMediaStreamSource: jest.fn(() => mockSourceNode), | ||
| destination: {}, | ||
| resume: jest.fn().mockResolvedValue(undefined), | ||
| state: 'running' | ||
|
|
@@ -218,4 +221,74 @@ describe('useRecorder', () => { | |
| }); | ||
| }); | ||
| }); | ||
|
|
||
| test('should return mute function', () => { | ||
| render(<HookApp onAudioChunk={onAudioChunk} />); | ||
| expect(typeof hookData?.mute).toBe('function'); | ||
| }); | ||
|
|
||
| test('should send MUTE command and stop media stream when mute is called', async () => { | ||
| render(<HookApp onAudioChunk={onAudioChunk} />); | ||
|
|
||
| // Start recording first | ||
| act(() => { | ||
| hookData?.record(); | ||
| }); | ||
|
|
||
| await waitFor(() => { | ||
| expect(mockWorkletPort.postMessage).toHaveBeenCalledWith({ command: 'START' }); | ||
| }); | ||
|
|
||
| // Clear mocks to isolate mute behavior | ||
| mockWorkletPort.postMessage.mockClear(); | ||
| mockTrack.stop.mockClear(); | ||
| mockSourceNode.disconnect.mockClear(); | ||
|
|
||
| // Call mute | ||
| act(() => { | ||
| hookData?.mute(); | ||
| }); | ||
|
|
||
| // Should send MUTE command to worklet | ||
| expect(mockWorkletPort.postMessage).toHaveBeenCalledWith({ command: 'MUTE' }); | ||
| // Should stop media stream tracks (mic indicator OFF) | ||
| expect(mockTrack.stop).toHaveBeenCalledTimes(1); | ||
| // Should disconnect source node | ||
| expect(mockSourceNode.disconnect).toHaveBeenCalledTimes(1); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should also test if the stream is zeroed out. |
||
| }); | ||
|
|
||
| test('should return unmute function from mute() that sends UNMUTE and restarts media stream', async () => { | ||
| render(<HookApp onAudioChunk={onAudioChunk} />); | ||
|
|
||
| // Start recording first | ||
| act(() => { | ||
| hookData?.record(); | ||
| }); | ||
|
|
||
| await waitFor(() => { | ||
| expect(mockWorkletPort.postMessage).toHaveBeenCalledWith({ command: 'START' }); | ||
| }); | ||
|
|
||
| // Call mute and get unmute function | ||
| let unmute: (() => void) | undefined; | ||
| act(() => { | ||
| unmute = hookData?.mute(); | ||
| }); | ||
|
|
||
| // Clear mocks to isolate unmute behavior | ||
| mockWorkletPort.postMessage.mockClear(); | ||
| mockMediaDevices.getUserMedia.mockClear(); | ||
|
|
||
| // Call unmute | ||
| act(() => { | ||
| unmute?.(); | ||
| }); | ||
|
|
||
| // Should send UNMUTE command to worklet | ||
| expect(mockWorkletPort.postMessage).toHaveBeenCalledWith({ command: 'UNMUTE' }); | ||
| // Should restart media stream | ||
| await waitFor(() => { | ||
| expect(mockMediaDevices.getUserMedia).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should test it no longer send out zeroes. |
||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need to extend the main context instead of relying on a separate provider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
following the same pattern which we did for "useStartVoice" and "useStopVoice"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think EugeneO's concern is valid. We are slowly dissecting the
APIComposer,CoreComposerinto smallerXXXComposerto improve performance and enabling plug-and-play.Why performance... if one thing in the composer context changed, it will propagate to every component that call
useContext(context)to subscribe.Says, if
styleOptionschanged, theAPIComposercontext would change, and every component that relies onuseMuteVoice()will be re-rendered even if they did not calluseStyleOptions.Can you move it to
<SpeechToSpeechComposer>?