Fix recorder start/stop failures and debounce toggle shortcuts#644
Fix recorder start/stop failures and debounce toggle shortcuts#644Korben00 wants to merge 3 commits intocjpais:mainfrom
Conversation
- Refactor AudioRecorder::stop() to use map_err for cleaner error handling - Simplify shortcut debounce logic and remove unnecessary variable extraction - Remove extraneous whitespace in audio manager - Add missing closing brace and comment in shortcut handler
- Extract magic number 250 to DEBOUNCE_MS constant - Move active_toggles update inside lock scope to prevent race conditions - Fix brace alignment and remove duplicate closing braces
|
Debouncing could make sense as a UX improvement, but it doesn't directly fix race conditions - it can mask them and make them harder to test. |
virenpepper
left a comment
virenpepper
left a comment
There was a problem hiding this comment.
hey, i reviewed this fix in detail and verified it compiles and works
the root cause is in recorder.rs - the consumer loop blocks on sample_rx.recv() waiting for audio, and only checks commands after receiving a sample. if the audio stream stops producing samples (race condition, device issue), stop() blocks forever because the command is never processed...
- recv_timeout(50ms) instead of blocking recv() - critical fix
- 5s timeout on stop() response - prevents infinite hang
- debouncing in handler (250ms) - prevents rapid triggers
- ui state changes after recording actually starts - correct state management
- early exit in on_end if not recording - prevents invalid stop calls
one minor note: 250ms debounce might feel slightly sluggish for power users, we should prob update to 150/200ms if possible
LGTM nice work! this should fix #641
|
Closing in favor of #824, thank you Korben for submitting this! Sorry I didn't merge earlier |
4 participants
I noticed that in some cases the recorder could fail the start/stop and leave the app in an inconsistent state (microstream stopped, toggle stuck). So I reinforced error management by restoring the microphone stream when it’s fairing, and I stabilized the toggle shortcut with a debounce to avoid double triggers.
Related Issues/Discussions
Fixes #
Discussion: #641
Testing
AI Assistance
If AI was used: