add root supervisor in desktop app #1881
Conversation
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe PR integrates ractor-based actor supervision across the desktop application and plugins. It adds workspace dependencies for ractor and ractor-supervisor, introduces a root supervisor spawned at application startup, and updates listener and local-stt plugins with InitOptions to support optional hierarchical parent supervision via ActorCell linking. Changes
Sequence DiagramsequenceDiagram
participant App as Desktop App
participant RS as Root Supervisor
participant LP as Listener Plugin
participant LS as Local-STT Plugin
App->>RS: spawn_root_supervisor()
RS-->>App: SupervisorRef, Handle
App->>LP: init(InitOptions{parent_supervisor})
LP->>LP: spawn_listener_supervisor(parent)
LP->>RS: link via ActorCell
LP-->>App: TauriPlugin
App->>LS: init(InitOptions{parent_supervisor})
LS->>LS: spawn_stt_supervisor(parent)
LS->>RS: link via ActorCell
LS-->>App: TauriPlugin
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/listener/src/supervisor.rs (1)
11-18: Consider documenting the supervisor configuration rationale.The listener supervisor uses
max_children: 10, max_restarts: 50while the STT supervisor usesmax_children: 1, max_restarts: 100. These differences are likely intentional based on the nature of each supervisor's workload. A brief comment explaining the chosen values would help future maintainers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
apps/desktop/src-tauri/Cargo.toml(1 hunks)apps/desktop/src-tauri/src/lib.rs(3 hunks)apps/desktop/src-tauri/src/supervisor.rs(1 hunks)plugins/listener/src/lib.rs(4 hunks)plugins/listener/src/supervisor.rs(1 hunks)plugins/local-stt/src/lib.rs(5 hunks)plugins/local-stt/src/server/supervisor.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
plugins/local-stt/src/lib.rs (1)
plugins/local-stt/src/server/supervisor.rs (1)
spawn_stt_supervisor(28-41)
apps/desktop/src-tauri/src/lib.rs (3)
apps/desktop/src-tauri/src/supervisor.rs (1)
spawn_root_supervisor(12-30)plugins/listener/src/lib.rs (1)
init(62-98)plugins/local-stt/src/lib.rs (1)
init(57-94)
plugins/listener/src/lib.rs (1)
plugins/listener/src/supervisor.rs (1)
spawn_listener_supervisor(20-33)
⏰ 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). (10)
- GitHub Check: Redirect rules - hyprnote-storybook
- GitHub Check: Header rules - hyprnote-storybook
- GitHub Check: Pages changed - hyprnote-storybook
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: ci (macos, depot-macos-14)
- GitHub Check: fmt
🔇 Additional comments (12)
apps/desktop/src-tauri/Cargo.toml (1)
67-68: LGTM!The workspace dependencies for
ractorandractor-supervisorare correctly added, following the existing pattern used throughout the file.plugins/local-stt/src/server/supervisor.rs (1)
19-41: Clean refactoring and parent linkage implementation.The extraction of
make_supervisor_options()improves readability, and the optional parent linkage pattern is correctly implemented. The supervisor is only linked after successful spawn.apps/desktop/src-tauri/src/lib.rs (2)
17-21: Verify the supervisor handle lifetime.The
_root_supervisor_handleis captured but not held beyond this destructuring. WhileJoinHandledoesn't cancel on drop by default, storing it somewhere (e.g., in app state) would ensure the task can be awaited on shutdown for graceful termination and proper cleanup.
85-94: LGTM!The
InitOptionspattern consistently passes the root supervisor's cell to both plugins, with graceful fallback toNoneif the root supervisor failed to spawn. This enables optional supervision hierarchy while maintaining backward compatibility.plugins/listener/src/supervisor.rs (1)
20-33: LGTM!The
spawn_listener_supervisorfunction correctly implements the optional parent linkage pattern, consistent with the STT supervisor implementation.plugins/local-stt/src/lib.rs (2)
31-34: LGTM!
InitOptionswith#[derive(Default)]provides backward compatibility, allowing callers to useDefault::default()when no parent supervision is needed.
57-94: LGTM!The updated
initfunction correctly acceptsInitOptions, passes the parent supervisor to the async spawn, and handles errors gracefully with logging. The non-blocking async spawn pattern allows plugin setup to complete while the supervisor initializes in the background.apps/desktop/src-tauri/src/supervisor.rs (2)
1-8: LGTM on imports and type aliases.The type aliases provide clean abstraction over the ractor types and align with the pattern used in
plugins/listener/src/supervisor.rs.
12-29: API usage is correct and verified; code follows the established pattern.The review's observation is accurate:
spawn_root_supervisor()returnsOptionwhilespawn_listener_supervisor()returnsResult, and both use the same underlyingDynamicSupervisor::spawn()API. However, this reflects a deliberate design choice rather than an error.Verified facts:
- Both functions use identical
DynamicSupervisor::spawn(...).awaitcalls- The desktop supervisor converts the underlying
ResulttoOptionvia explicit matching- The listener supervisor propagates the
Resultdirectly with?- The local-stt supervisor also uses
Resultpropagation, showing multiple patterns in the codebase- The call site in
lib.rshandlesOptionby mappingNonetoNonevalues without specialized error logicAssessment: The implementation is correct and consistent with the codebase patterns. The suggestion to use
Resultinstead ofOptionis architecturally valid but remains optional—both approaches are used across different supervisors in the project.plugins/listener/src/lib.rs (3)
1-2: LGTM on supervision integration imports and exports.The module structure cleanly separates supervisor logic and re-exports the necessary types for consumers.
Also applies to: 12-12, 17-17
21-21: State fields are now public - verify this is intentional.Making
app,listener_supervisor, andsupervisor_handlepublic exposes internal state. If this is intentional for external access (e.g., by the desktop app or other plugins), this is fine. Otherwise, consider providing accessor methods instead.Also applies to: 23-27
29-32: LGTM on InitOptions.Deriving
Defaultis a good choice for backward compatibility - callers can useInitOptions::default()when no parent supervisor is needed.
No description provided.