fix(listener): improve error handling in session lifecycle#1924
fix(listener): improve error handling in session lifecycle#1924
Conversation
- Remove .unwrap() on SessionEvent::emit in ext.rs - treat as non-fatal - Don't treat Recorder's encode failures as fatal in post_stop - Don't treat UI-emit failures in ListenerActor as fatal - Make Listener's websocket tasks bail if sending ListenerMsg fails 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. |
📝 WalkthroughWalkthroughError handling refactoring across listener plugin components replaces crash-prone patterns (unwrap, error propagation) with explicit error logging and graceful continuation in StreamResponse emission, encoding operations, and SessionEvent emission paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 (3)
plugins/listener/src/ext.rs (1)
114-120: Guarded SessionEvent emits align with the goal of avoiding panicsWrapping the
SessionEvent::*emits inif let Err(error)and logging instead of unwrapping gives you best‑effort notifications without taking down the session when the Tauri event layer misbehaves. Control flow in bothstart_sessionandstop_sessionremains sensible.If you want richer observability later, consider adding
session_idinto the log context so failures can be correlated to specific sessions.Please double‑check that the tauri/tauri_specta
Event::emiterror conditions you care about are indeed non‑fatal (e.g., closed window) so logging‑and‑continue is acceptable for your UX guarantees.Also applies to: 146-148, 165-167
plugins/listener/src/actors/recorder.rs (1)
161-179: Encoding errors are now non‑fatal; confirm whether rename/remove should behave similarlyThe new
matcharoundencode_wav_to_vorbis_filenicely logs failures and preserves the WAV as a fallback without failing the actor, which matches the comment intent.Note that
std::fs::rename(&temp_ogg_path, &st.ogg_path)?andstd::fs::remove_file(&st.wav_path)?still propagate IO errors via?. That means filesystem issues at this point can still causepost_stopto return an error. If the broader goal is “shutdown should not fail just because finalization/cleanup had issues,” you might want to treat these similarly (log + best‑effort) rather than bubbling them.Please verify whether your supervision strategy expects
post_stopto succeed even on finalization/cleanup IO hiccups; if not, current behavior is fine, otherwise consider matching and logging instead of?.plugins/listener/src/actors/listener.rs (1)
530-533: Breaking stream loops onsend_messagefailure is reasonable; confirm ractor semanticsIn both the finalize path and the main stream loop, you now treat
myself.send_message(ListenerMsg::StreamResponse(response)).is_err()as a signal to log and break out of the loop. That’s a sensible way to stop the RX task once the actor is no longer able to receive messages, and it avoids panics.This relies on
ActorRef::send_messageonly erroring when the actor is gone or shutting down. If ractor can also return errors for transient reasons (e.g., mailbox full with an error instead of backpressure), this would prematurely terminate the stream. If it only fails on shutdown, the new behavior is spot on.Please confirm ractor’s
send_messageerror semantics. If you find any non‑terminal cases, consider distinguishing them (e.g., via error variants) so you only break the loop when the actor is truly gone.Also applies to: 563-566
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/listener/src/actors/listener.rs(3 hunks)plugins/listener/src/actors/recorder.rs(1 hunks)plugins/listener/src/ext.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: plyght
Repo: fastrepl/hyprnote PR: 921
File: plugins/location-connectivity/src/error.rs:1-41
Timestamp: 2025-06-06T16:31:46.457Z
Learning: In the location-connectivity plugin (plugins/location-connectivity/), comprehensive error handling via LocationConnectivityError is necessary despite the "no error handling" guideline, as the plugin performs system-level operations (WiFi detection, persistent storage) that require proper error propagation to the frontend.
⏰ 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). (7)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
- GitHub Check: ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: ci (macos, depot-macos-14)
🔇 Additional comments (1)
plugins/listener/src/actors/listener.rs (1)
129-136: Guarding StreamResponse event emission prevents crashes on frontend failuresSwitching
SessionEvent::StreamResponseemission toif let Err(error)with a structured log means transcript streaming won’t panic the listener if the Tauri event emission fails (e.g., window gone). That’s consistent with the rest of the PR’s error‑handling strategy.If you ever see these logs frequently in production, it might be worth adding counters/metrics so you can distinguish one‑off UI issues from systemic event emission problems.
Please briefly confirm that, for your UX, it’s acceptable to keep the listener actor alive even when event emission starts failing (i.e., you don’t need to stop the session in that case).
1 participant
Summary
Implements error handling improvements for the listener plugin based on code review feedback. The changes make the session lifecycle more robust by treating certain failures as non-fatal:
ext.rs: Replace
.unwrap()onSessionEvent::emit()with error logging - prevents panics if Tauri fails to emit events (e.g., window closed, runtime shutting down)recorder.rs: Don't treat WAV-to-OGG encode failures as fatal in
post_stop- keeps the WAV file as fallback instead of triggering supervisor restarts during shutdownlistener.rs:
StreamResponseemit failures as non-fatal (log instead of propagate error)send_messageto the actor fails - prevents task leaks when the actor is killed/panickedReview & Testing Checklist for Human
Notes
Link to Devin run: https://app.devin.ai/sessions/b8820a375ae045bea131b4e4af47cb0e
Requested by: yujonglee (@yujonglee)
These changes address issues 2.1, 2.2, 2.3, and 2.4 from the code review.