-
Notifications
You must be signed in to change notification settings - Fork 17
Add mic-inactivity detection UX and related state fixes #230
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
Conversation
Removed comments related to audio chunk timestamps and inactivity checks.
Removed commented-out code related to mic activity tracking and inactivity monitoring.
Removed comment about microphone audio status.
| last_suffix = pcm_data.slice(-(pcm_data.length % 128)) | ||
|
|
||
| // update last audio timestamp and mark that we've received at least one audio chunk | ||
| this.lastAudioTimestamp = Date.now(); |
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.
Date.now() is subject to the user's device time changing (not monotonic). Perhaps performance.now() would make sense instead?
| const state: any = store.getState(); | ||
| const listening = state.ControlReducer?.listening === true; | ||
| const micNoAudio = state.ControlReducer?.micNoAudio === true; | ||
| if (listening) { |
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.
Isn't this checked before showing the snackbar? Is it necessary to fetch state from redux here?
We generally want to limit the use of getState() outside of redux.
| const thresholdMs = 3000; | ||
| if (this.inactivityInterval == null) { | ||
| const { store } = require('../../../store'); | ||
| this.inactivityInterval = setInterval(() => { |
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.
It feels to me like a timeout would be more natural here rather than an interval.
e.g. Set timeout of thresholdMs and reset timeout every time audio chunk is received. On timeout fired, micNoAudio to true.
Semantically, micNoAudio means no audio received for thresholdMs.
| const thresholdMs = 3000; | ||
| try { | ||
| if (listening) { | ||
| try { (window as any).__hasReceivedAudio = false; } catch (e) {} |
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 hesitate to pollute to window namespace with additional props. I don't think it is necessary to do so in order to implement mic activity.
It seems to me that the logic for setting the micNoAudio flag can be fully encapsulated within the recognizers themselves. This portion here would simply be fetching listening and micNoAudio from redux and showing/hiding the snackbar if both are true in useEffect. Am I missing something?
Implemented user-visible connection/status feedback and a "mic on but no audio received" detector, plus a few small state/UX fixes discovered during integration