RestForAll supervisor in listener plugin #1904
Conversation
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis PR replaces a controller-based actor architecture with a session-supervised architecture. The ControllerActor and ControllerParams are deleted, replaced by SessionParams and SessionContext. Public APIs migrate to accept SessionParams. Plugin initialization is simplified by removing InitOptions. Audio buffering is introduced in SourceActor, and session supervision is now explicit using Supervisor with named child actors. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Tauri App
participant Ext as ListenerPluginExt
participant Sup as SessionSupervisor
participant Src as SourceActor
participant Lst as ListenerActor
participant Rec as RecorderActor
App->>Ext: start_session(SessionParams)
Ext->>Ext: Check guard.session_supervisor
Ext->>Ext: Resolve app_dir
Ext->>Ext: Create SessionContext
Ext->>Sup: spawn_session_supervisor(ctx)
Sup->>Src: Initialize SourceActor
Sup->>Lst: Initialize ListenerActor<br/>(with backoff strategy)
Sup->>Rec: Initialize RecorderActor<br/>(if enabled)
Src-->>Ext: SourceActor running
Lst-->>Ext: ListenerActor running
Rec-->>Ext: RecorderActor running
Ext->>Ext: Store supervisor & handle
Ext->>App: Emit RunningActive event
rect rgba(100, 200, 150, 0.3)
Note over Src,Lst: Audio Buffering Path
Src->>Src: Generate audio chunks
Src->>Lst: Send audio
alt Listener unavailable
Src->>Src: Buffer to AudioBuffer
Src->>Src: Log buffering event
else Listener recovered
Src->>Src: Flush buffer_to_listener
Src->>Lst: Send buffered chunks
end
end
App->>Ext: stop_session()
Ext->>Src: GetSessionId message
Src-->>Ext: Return session_id
Ext->>App: Emit Finalizing event
Ext->>Sup: Stop supervisor
Ext->>Ext: Await handle completion
Ext->>App: Emit Inactive event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (6)
plugins/listener/src/actors/mod.rs (1)
9-12: Redundant conditional compilation for identical values.Both branches of the
#[cfg]attribute defineSAMPLE_RATEas16 * 1000. If the value is the same regardless of the target OS, the conditional compilation is unnecessary.-#[cfg(target_os = "macos")] -pub const SAMPLE_RATE: u32 = 16 * 1000; -#[cfg(not(target_os = "macos"))] -pub const SAMPLE_RATE: u32 = 16 * 1000; +pub const SAMPLE_RATE: u32 = 16 * 1000;plugins/listener/src/ext.rs (4)
131-135: Consider handling potential emit failures gracefully.The
.unwrap()onSessionEvent::RunningActive.emit()could panic if the app handle is in an invalid state. While unlikely, consider using.ok()or logging the error instead.- SessionEvent::RunningActive { - session_id: params.session_id, - } - .emit(&guard.app) - .unwrap(); + if let Err(e) = SessionEvent::RunningActive { + session_id: params.session_id, + } + .emit(&guard.app) + { + tracing::error!(error = ?e, "failed_to_emit_running_active"); + }
160-164: Same concern withFinalizingevent emission.- SessionEvent::Finalizing { session_id } - .emit(&guard.app) - .unwrap(); + if let Err(e) = SessionEvent::Finalizing { session_id }.emit(&guard.app) { + tracing::error!(error = ?e, "failed_to_emit_finalizing"); + }
179-183: Same concern withInactiveevent emission.- SessionEvent::Inactive { session_id } - .emit(&guard.app) - .unwrap(); + if let Err(e) = SessionEvent::Inactive { session_id }.emit(&guard.app) { + tracing::error!(error = ?e, "failed_to_emit_inactive"); + }
170-172: JoinHandle error is silently ignored.If the supervisor task panicked,
handle.awaitwould return aJoinError. Currently this is discarded withlet _ = .... Consider logging if the supervisor terminated unexpectedly.if let Some(handle) = guard.supervisor_handle.take() { - let _ = handle.await; + if let Err(e) = handle.await { + tracing::warn!(error = ?e, "supervisor_task_join_error"); + } }plugins/listener/src/supervisor.rs (1)
100-100: Consider makingChannelModeconfigurable viaSessionParams.
ChannelMode::Dualis hardcoded here. If this is intentional for all sessions, consider adding a comment explaining why. Otherwise, consider adding it toSessionParamsfor flexibility.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockplugins/listener/js/bindings.gen.tsis excluded by!**/*.gen.ts
📒 Files selected for processing (9)
apps/desktop/src-tauri/src/lib.rs(1 hunks)apps/desktop/src/store/zustand/listener/general.ts(4 hunks)plugins/listener/src/actors/controller.rs(0 hunks)plugins/listener/src/actors/mod.rs(1 hunks)plugins/listener/src/actors/source.rs(7 hunks)plugins/listener/src/commands.rs(2 hunks)plugins/listener/src/ext.rs(5 hunks)plugins/listener/src/lib.rs(3 hunks)plugins/listener/src/supervisor.rs(1 hunks)
💤 Files with no reviewable changes (1)
- plugins/listener/src/actors/controller.rs
🧰 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.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.ts
🧬 Code graph analysis (4)
apps/desktop/src-tauri/src/lib.rs (1)
plugins/listener/src/lib.rs (1)
init(54-75)
plugins/listener/src/ext.rs (2)
plugins/listener/src/supervisor.rs (1)
spawn_session_supervisor(60-162)plugins/listener/src/actors/source.rs (2)
name(111-113)handle(154-203)
plugins/listener/src/supervisor.rs (1)
plugins/local-stt/src/server/supervisor.rs (1)
make_supervisor_options(19-26)
plugins/listener/src/lib.rs (1)
plugins/listener/src/supervisor.rs (1)
session_supervisor_name(37-39)
⏰ 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). (4)
- GitHub Check: ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: ci (macos, depot-macos-14)
- GitHub Check: ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: fmt
🔇 Additional comments (21)
plugins/listener/src/actors/mod.rs (1)
20-23: LGTM!Making
datapublic is appropriate to support the new audio buffering mechanism inSourceActor.plugins/listener/src/actors/source.rs (3)
539-545: Buffer overflow drops audio without recovery option.When the buffer exceeds
MAX_BUFFER_CHUNKS(150), the oldest chunk is silently dropped. While the warning log is good, consider whether 150 chunks is sufficient for your expectedListenerActorunavailability duration, or if alternative strategies (e.g., writing to disk) should be considered for longer outages.How long can
ListenerActorrealistically be unavailable? With typical chunk sizes, 150 chunks may represent only a few seconds of audio.
29-37: LGTM on theGetSessionIdmessage addition.The replacement of
GetModewithGetSessionIdaligns with the session-based architecture refactoring.
526-558: Clean AudioBuffer implementation.The ring-buffer-style
AudioBufferwith bounded size and overflow handling is well-structured. The use ofVecDequeis appropriate for FIFO semantics.plugins/listener/src/commands.rs (2)
1-1: LGTM!Import correctly updated to use the new
SessionParamstype from the supervisor module.
52-58: LGTM!The
start_sessioncommand signature properly updated to acceptSessionParams, consistent with the API migration across the codebase.apps/desktop/src-tauri/src/lib.rs (1)
83-84: LGTM!The simplified
tauri_plugin_listener::init()call aligns with the new session-scoped supervision model. The plugin now manages its own session supervisor internally rather than accepting a parent supervisor.apps/desktop/src/store/zustand/listener/general.ts (3)
8-14: LGTM!Import correctly updated to use
SessionParamsfrom@hypr/plugin-listener.
49-53: LGTM!The
GeneralActions.starttype signature properly updated to acceptSessionParams.
83-84: LGTM!The
startSessionEffectfunction signature correctly updated to match the new parameter type.plugins/listener/src/ext.rs (2)
92-146: Well-structured session lifecycle management.The
start_sessionimplementation properly:
- Guards against concurrent sessions
- Resolves the app directory with error handling
- Manages UI state (tray) on both success and failure paths
- Creates a comprehensive
SessionContextwith timing information
40-48: LGTM on SourceActor migration.The controller actor references are correctly replaced with
SourceActor/SourceMsg, and the error variant properly updated.plugins/listener/src/lib.rs (4)
15-15: LGTM!The export change aligns with the shift from a global dynamic supervisor to session-scoped supervisors.
21-25: LGTM!Using
ActorCellfor the session supervisor is appropriate when you only need lifecycle control rather than typed message passing.
27-35: LGTM!The synchronous implementation is appropriate here. The
is_some()check is trivial and doesn't require async.
54-75: LGTM!The simplified initialization properly defers supervisor spawning to session start, which aligns with the session-scoped architecture.
plugins/listener/src/supervisor.rs (5)
1-12: LGTM!The imports are well-organized and appropriate for the session supervisor functionality.
28-39: LGTM!The
SessionContextstruct appropriately encapsulates runtime context for the session. UsingInstantfor internal timing is correct.
41-48: Supervision strategy choice looks appropriate.
RestForOnecorrectly models the dependency chain: ifSourceActorfails, all downstream actors restart; ifListenerActorfails, it and theRecorderActorrestart. The stricter restart limits (10 in 30s vs the 100 in 180s inlocal-stt) seem intentional for session-scoped fail-fast behavior.
50-58: LGTM!The exponential backoff with no delay for the first restart and capped at 16 seconds provides a good balance between fast recovery and preventing restart storms.
154-162: LGTM!The supervisor spawning is clean, and the return type correctly provides both the
ActorCellfor lifecycle control and theJoinHandlefor awaiting completion.
No description provided.