fix: batch audio processing bugs in listener2#2150
Conversation
- Fix check-then-act race: Set batch state synchronously at start of runBatch - Fix message ordering race: Compute percentage in streaming task and pass with response - Increase timeout from 5s to 30s for large audio files - Fix silent message send failures: Log errors when send_message fails - Fix error handling: Preserve original error info in ext.rs instead of discarding - Add UI feedback for unsupported provider: Throw error instead of silent console.error Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe PR modifies batch processing error handling and state management across the desktop application and listener plugin. Changes include replacing fallback error paths with thrown exceptions, updating batch state management calls, redesigning the StreamResponse message structure to include precomputed percentage values, and increasing timeout thresholds. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src/store/zustand/listener/general.ts (1)
330-337: EarlyhandleBatchStartedis good for race-avoidance; ensure double-call is safeMarking the batch as started before wiring the listener/issuing
runBatchhelps avoid a race where a secondrunBatchcould start while the first is still initializing. However,handleBatchStartedis now invoked both here (Line 336) and again on the"batchStarted"event (Lines 360–362), so it may be called twice for the samesessionId.Please make sure
handleBatchStartedis idempotent (or guard the event-path call when state already reflects a started batch) so repeated invocations don’t regress percentage/progress state or counters.Also applies to: 360-363
plugins/listener2/src/ext.rs (1)
121-129: Event emission now fails gracefully instead of panicking; consider aligning AM pathMapping
BatchStarted/BatchResponse.emit(&app)failures tocrate::Error::BatchStartFailed(...)(Lines 121–129, 142–152, 158–166, 180–189) is a solid improvement overunwrap(): batch runs for Deepgram/Soniox will now surface structured errors instead of crashing the plugin on event-delivery issues.One follow-up you may want (for consistency and robustness):
- The AM provider path still calls
.emit(&app).unwrap()forBatchStarted. If emitting that event can fail in practice, it would be safer to apply the same.map_err(...)?pattern there so all providers share the same failure semantics.Functionally this change looks good as-is; the above is mostly about consistency across providers.
Also applies to: 142-152, 158-166, 180-189
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/hooks/useRunBatch.ts(1 hunks)apps/desktop/src/store/zustand/listener/general.ts(1 hunks)plugins/listener2/src/batch.rs(8 hunks)plugins/listener2/src/ext.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Agent implementations should use TypeScript and follow the established architectural patterns defined in the agent framework
Agent communication should use defined message protocols and interfaces
Files:
apps/desktop/src/store/zustand/listener/general.tsapps/desktop/src/hooks/useRunBatch.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/store/zustand/listener/general.tsapps/desktop/src/hooks/useRunBatch.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: desktop_ci (macos, depot-macos-14)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: fmt
- GitHub Check: Devin
🔇 Additional comments (2)
plugins/listener2/src/batch.rs (1)
12-24: Streamed batch responses now carry a precomputed 0–1 progress ratio; verify consumer expectationsThe new flow looks coherent:
audio_duration_secsis derived once fromframe_count / sample_rate, with sane 0.0 fallback for invalid metadata (Lines 238–245).process_batch_streamcomputespercentage = compute_percentage(&response, audio_duration_secs)and clamps it to[0.0, 1.0], returning0.0when the transcript end can’t be determined (Lines 336–342, 377–383).BatchMsg::StreamResponsecarries{ response: Box<StreamResponse>, percentage: f64 }, andBatchState::emit_streamed_responseemits this viaBatchResponseStreamed(Lines 16–24, 47–59, 126–139).- All
send_messagecalls are now wrapped with error logging, and a timeout of 30 seconds is used per response (Lines 12, 305–371), which is a safer, more debuggable behavior.Two things to double-check:
- Consumer semantics of
percentage– It is a normalized fraction in[0.0, 1.0], but the field name reads like “percentage”. Please confirm downstream UI/state (e.g.,handleBatchResponseStreamed) expects a 0–1 ratio and doesn’t multiply again or assume 0–100.- Emission frequency – Because
BatchActor::handleonly callsemit_streamed_responsewhenis_finalis true (Lines 132–139), you will only get a singleBatchResponseStreamedemission per batch. If callers expect incremental progress updates on intermediate transcripts, thisis_finalgate would need to be relaxed.If both are intended, the implementation here looks solid.
Also applies to: 47-59, 126-139, 238-245, 287-375, 377-383
apps/desktop/src/hooks/useRunBatch.ts (1)
46-49: Switch from silent fallback to thrown error improves visibility; ensure callers handle the rejectionThrowing an
Errorwhenproviderisnull(Lines 63–67) is a good change compared to silently returning: unsupported batch providers will now surface clearly to the caller.Please confirm that all callsites of this hook either:
awaitthe returned promise inside atry/catch, or- attach a
.catch(...)so this new failure mode doesn’t become an unhandled promise rejection in the UI.
Also applies to: 51-67
1 participant
Summary
Fixes several bugs in the batch audio processing pipeline for listener2, identified through code review with Claude CLI.
Key changes:
Fix check-then-act race condition (general.ts): Call
handleBatchStartedsynchronously at the start ofrunBatchinstead ofclearBatchSession, preventing two concurrent calls from both passing the mode check.Fix message ordering race (batch.rs): Compute percentage directly in the streaming task and pass it with
StreamResponseinstead of sending a separateStreamAudioDurationmessage. This eliminates the race whereStreamResponsecould arrive beforeStreamAudioDuration, causing 0% progress.Increase timeout from 5s to 30s (batch.rs): The 5-second timeout was too aggressive for large audio files with gaps between transcription chunks.
Fix silent message send failures (batch.rs): Log errors when
send_messagefails instead of silently ignoring withlet _ =.Preserve original error info (ext.rs): Use
|e|instead of|_|inmap_errcalls to include actual error details.Add UI feedback for unsupported provider (useRunBatch.ts): Throw an error instead of silently returning with
console.error.Review & Testing Checklist for Human
clearBatchSessiontohandleBatchStartedin general.ts:336 - confirm this correctly prevents the race condition without breaking expected state flowRecommended test plan:
Notes