From 21d175bccc3f938985e49be3ee6a6099700c6662 Mon Sep 17 00:00:00 2001 From: Louis Knight-Webb Date: Tue, 9 Dec 2025 09:57:42 +0000 Subject: [PATCH] Play notification sound when approval needed (vibe-kanban) (#1463) * The implementation is complete. Here's a summary of the changes: ## Summary Added notification sound when approval is needed, using the same notification system as task finalization. **Files modified:** 1. **`crates/services/src/services/approvals/executor_approvals.rs`** - Added `config: Arc>` field to `ExecutorApprovalBridge` - Added imports for `Config`, `NotificationService`, and `RwLock` - Added notification call in `request_tool_approval()` after creating the approval request 2. **`crates/local-deployment/src/container.rs`** - Passed `self.config.clone()` to `ExecutorApprovalBridge::new()` When an approval is requested, the notification will display: - **Title:** "Approval Needed" - **Message:** "Tool '{tool_name}' requires approval" The notification uses the user's configured sound file and respects their notification settings (sound enabled/disabled, push enabled/disabled). * Cleanup script changes for task attempt af6bac2d-a3ba-4c6a-ae35-3a54bd05212f * Done! Here's a summary of the refactored implementation: ## Summary Refactored `NotificationService` to hold config internally and added approval notification sound: **Files modified:** 1. **`crates/services/src/services/notification.rs`** - Added `config: Arc>` field - Added `new(config)` constructor - Changed `notify` and `notify_execution_halted` from static to instance methods - Added internal `send_notification` helper method 2. **`crates/services/src/services/container.rs`** - Updated `finalize_task` trait method to accept `&NotificationService` instead of `&Arc>` - Removed unused `Config` import 3. **`crates/local-deployment/src/container.rs`** - Added `notification_service: NotificationService` field to `LocalContainerService` - Initialize service in `new()` with config - Updated all `finalize_task` calls to pass `&self.notification_service` - Updated `ExecutorApprovalBridge::new()` to pass `notification_service` 4. **`crates/services/src/services/approvals/executor_approvals.rs`** - Replaced `config: Arc>` with `notification_service: NotificationService` - Updated constructor and notification call to use the service The notification uses the user's configured sound and settings, and triggers when a tool requires approval. * Cleanup script changes for task attempt af6bac2d-a3ba-4c6a-ae35-3a54bd05212f * Done! The refactoring is complete. Here's a summary: ## Summary Refactored `NotificationService` to be accessed via the `ContainerService` trait, making it cleaner: **Changes made:** 1. **`crates/services/src/services/container.rs`** - Added `fn notification_service(&self) -> &NotificationService` to the trait - Simplified `finalize_task` signature to `(&self, share_publisher, ctx)` - removed the notification_service parameter - `finalize_task` now calls `self.notification_service().notify_execution_halted(ctx)` 2. **`crates/local-deployment/src/container.rs`** - Implemented `fn notification_service(&self) -> &NotificationService` returning `&self.notification_service` - Updated all 4 `finalize_task` call sites to use the simplified signature The notification service is now a first-class member of the `ContainerService` trait, following the same pattern as `db()`, `git()`, and `share_publisher()`. * Cleanup script changes for task attempt af6bac2d-a3ba-4c6a-ae35-3a54bd05212f * Done! Here's a summary of all the changes: ## Final Summary **Feature implemented:** Play notification sound when approval is needed. **Files modified:** 1. **`crates/services/src/services/notification.rs`** - Added `config: Arc>` field and `new()` constructor - Removed `notify_execution_halted` method (moved application logic out) - Kept only generic `notify(title, message)` method 2. **`crates/services/src/services/container.rs`** - Added `fn notification_service(&self) -> &NotificationService` to trait - Moved notification logic into `finalize_task`: - Skip notification if process was killed - Build title/message based on execution status - Call `self.notification_service().notify(...)` 3. **`crates/local-deployment/src/container.rs`** - Added `notification_service: NotificationService` field - Implemented `notification_service()` accessor - Simplified `finalize_task` calls (removed notification_service parameter) - Pass `notification_service.clone()` to `ExecutorApprovalBridge` 4. **`crates/services/src/services/approvals/executor_approvals.rs`** - Added `notification_service: NotificationService` field - Calls `self.notification_service.notify("Approval Needed", ...)` when approval is requested The `NotificationService` is now a clean, reusable service that only handles notifications without any application-specific logic. * Cleanup script changes for task attempt af6bac2d-a3ba-4c6a-ae35-3a54bd05212f --- crates/local-deployment/src/container.rs | 25 +++++----- .../services/approvals/executor_approvals.rs | 19 ++++++- crates/services/src/services/container.rs | 31 ++++++++++-- crates/services/src/services/notification.rs | 50 ++++++------------- 4 files changed, 72 insertions(+), 53 deletions(-) diff --git a/crates/local-deployment/src/container.rs b/crates/local-deployment/src/container.rs index 677f1aff..48f36afc 100644 --- a/crates/local-deployment/src/container.rs +++ b/crates/local-deployment/src/container.rs @@ -52,6 +52,7 @@ use services::services::{ diff_stream::{self, DiffStreamHandle}, git::{Commit, DiffTarget, GitService}, image::ImageService, + notification::NotificationService, queued_message::QueuedMessageService, share::SharePublisher, worktree_manager::{WorktreeCleanup, WorktreeManager}, @@ -80,6 +81,7 @@ pub struct LocalContainerService { approvals: Approvals, queued_message_service: QueuedMessageService, publisher: Result, + notification_service: NotificationService, } impl LocalContainerService { @@ -97,6 +99,7 @@ impl LocalContainerService { ) -> Self { let child_store = Arc::new(RwLock::new(HashMap::new())); let interrupt_senders = Arc::new(RwLock::new(HashMap::new())); + let notification_service = NotificationService::new(config.clone()); let container = LocalContainerService { db, @@ -110,6 +113,7 @@ impl LocalContainerService { approvals, queued_message_service, publisher, + notification_service, }; container.spawn_worktree_cleanup().await; @@ -424,9 +428,7 @@ impl LocalContainerService { ); // Manually finalize task since we're bypassing normal execution flow - container - .finalize_task(&config, publisher.as_ref().ok(), &ctx) - .await; + container.finalize_task(publisher.as_ref().ok(), &ctx).await; } } @@ -469,9 +471,7 @@ impl LocalContainerService { { tracing::error!("Failed to start queued follow-up: {}", e); // Fall back to finalization if follow-up fails - container - .finalize_task(&config, publisher.as_ref().ok(), &ctx) - .await; + container.finalize_task(publisher.as_ref().ok(), &ctx).await; } } else { // Execution failed or was killed - discard the queued message and finalize @@ -480,14 +480,10 @@ impl LocalContainerService { ctx.task_attempt.id, ctx.execution_process.status ); - container - .finalize_task(&config, publisher.as_ref().ok(), &ctx) - .await; + container.finalize_task(publisher.as_ref().ok(), &ctx).await; } } else { - container - .finalize_task(&config, publisher.as_ref().ok(), &ctx) - .await; + container.finalize_task(publisher.as_ref().ok(), &ctx).await; } } @@ -819,6 +815,10 @@ impl ContainerService for LocalContainerService { self.publisher.as_ref().ok() } + fn notification_service(&self) -> &NotificationService { + &self.notification_service + } + async fn git_branch_prefix(&self) -> String { self.config.read().await.git_branch_prefix.clone() } @@ -976,6 +976,7 @@ impl ContainerService for LocalContainerService { ExecutorApprovalBridge::new( self.approvals.clone(), self.db.clone(), + self.notification_service.clone(), execution_process.id, ) } diff --git a/crates/services/src/services/approvals/executor_approvals.rs b/crates/services/src/services/approvals/executor_approvals.rs index dbbd9882..fb8e096b 100644 --- a/crates/services/src/services/approvals/executor_approvals.rs +++ b/crates/services/src/services/approvals/executor_approvals.rs @@ -7,19 +7,26 @@ use serde_json::Value; use utils::approvals::{ApprovalRequest, ApprovalStatus, CreateApprovalRequest}; use uuid::Uuid; -use crate::services::approvals::Approvals; +use crate::services::{approvals::Approvals, notification::NotificationService}; pub struct ExecutorApprovalBridge { approvals: Approvals, db: DBService, + notification_service: NotificationService, execution_process_id: Uuid, } impl ExecutorApprovalBridge { - pub fn new(approvals: Approvals, db: DBService, execution_process_id: Uuid) -> Arc { + pub fn new( + approvals: Approvals, + db: DBService, + notification_service: NotificationService, + execution_process_id: Uuid, + ) -> Arc { Arc::new(Self { approvals, db, + notification_service, execution_process_id, }) } @@ -50,6 +57,14 @@ impl ExecutorApprovalService for ExecutorApprovalBridge { .await .map_err(ExecutorApprovalError::request_failed)?; + // Play notification sound when approval is needed + self.notification_service + .notify( + "Approval Needed", + &format!("Tool '{}' requires approval", tool_name), + ) + .await; + let status = waiter.clone().await; if matches!(status, ApprovalStatus::Pending) { diff --git a/crates/services/src/services/container.rs b/crates/services/src/services/container.rs index 38f78ccf..5d17f30b 100644 --- a/crates/services/src/services/container.rs +++ b/crates/services/src/services/container.rs @@ -41,7 +41,6 @@ use utils::{ use uuid::Uuid; use crate::services::{ - config::Config, git::{GitService, GitServiceError}, notification::NotificationService, share::SharePublisher, @@ -79,6 +78,8 @@ pub trait ContainerService { fn share_publisher(&self) -> Option<&SharePublisher>; + fn notification_service(&self) -> &NotificationService; + fn task_attempt_to_current_dir(&self, task_attempt: &TaskAttempt) -> PathBuf; async fn create(&self, task_attempt: &TaskAttempt) -> Result; @@ -161,7 +162,6 @@ pub trait ContainerService { /// Finalize task execution by updating status to InReview and sending notifications async fn finalize_task( &self, - config: &Arc>, share_publisher: Option<&SharePublisher>, ctx: &ExecutionContext, ) { @@ -181,8 +181,31 @@ pub trait ContainerService { tracing::error!("Failed to update task status to InReview: {e}"); } } - let notify_cfg = config.read().await.notifications.clone(); - NotificationService::notify_execution_halted(notify_cfg, ctx).await; + + // Skip notification if process was intentionally killed by user + if matches!(ctx.execution_process.status, ExecutionProcessStatus::Killed) { + return; + } + + let title = format!("Task Complete: {}", ctx.task.title); + let message = match ctx.execution_process.status { + ExecutionProcessStatus::Completed => format!( + "✅ '{}' completed successfully\nBranch: {:?}\nExecutor: {}", + ctx.task.title, ctx.task_attempt.branch, ctx.task_attempt.executor + ), + ExecutionProcessStatus::Failed => format!( + "❌ '{}' execution failed\nBranch: {:?}\nExecutor: {}", + ctx.task.title, ctx.task_attempt.branch, ctx.task_attempt.executor + ), + _ => { + tracing::warn!( + "Tried to notify attempt completion for {} but process is still running!", + ctx.task_attempt.id + ); + return; + } + }; + self.notification_service().notify(&title, &message).await; } /// Cleanup executions marked as running in the db, call at startup diff --git a/crates/services/src/services/notification.rs b/crates/services/src/services/notification.rs index 931a35fd..3e7f70a1 100644 --- a/crates/services/src/services/notification.rs +++ b/crates/services/src/services/notification.rs @@ -1,52 +1,32 @@ -use std::sync::OnceLock; +use std::sync::{Arc, OnceLock}; -use db::models::execution_process::{ExecutionContext, ExecutionProcessStatus}; +use tokio::sync::RwLock; use utils; -use crate::services::config::SoundFile; +use crate::services::config::{Config, NotificationConfig, SoundFile}; /// Service for handling cross-platform notifications including sound alerts and push notifications #[derive(Debug, Clone)] -pub struct NotificationService {} -use crate::services::config::NotificationConfig; +pub struct NotificationService { + config: Arc>, +} /// Cache for WSL root path from PowerShell static WSL_ROOT_PATH_CACHE: OnceLock> = OnceLock::new(); impl NotificationService { - pub async fn notify_execution_halted(mut config: NotificationConfig, ctx: &ExecutionContext) { - // If the process was intentionally killed by user, suppress sound - if matches!(ctx.execution_process.status, ExecutionProcessStatus::Killed) { - config.sound_enabled = false; - } - - let title = format!("Task Complete: {}", ctx.task.title); - let message = match ctx.execution_process.status { - ExecutionProcessStatus::Completed => format!( - "✅ '{}' completed successfully\nBranch: {:?}\nExecutor: {}", - ctx.task.title, ctx.task_attempt.branch, ctx.task_attempt.executor - ), - ExecutionProcessStatus::Failed => format!( - "❌ '{}' execution failed\nBranch: {:?}\nExecutor: {}", - ctx.task.title, ctx.task_attempt.branch, ctx.task_attempt.executor - ), - ExecutionProcessStatus::Killed => format!( - "🛑 '{}' execution cancelled by user\nBranch: {:?}\nExecutor: {}", - ctx.task.title, ctx.task_attempt.branch, ctx.task_attempt.executor - ), - _ => { - tracing::warn!( - "Tried to notify attempt completion for {} but process is still running!", - ctx.task_attempt.id - ); - return; - } - }; - Self::notify(config, &title, &message).await; + pub fn new(config: Arc>) -> Self { + Self { config } } /// Send both sound and push notifications if enabled - pub async fn notify(config: NotificationConfig, title: &str, message: &str) { + pub async fn notify(&self, title: &str, message: &str) { + let config = self.config.read().await.notifications.clone(); + Self::send_notification(&config, title, message).await; + } + + /// Internal method to send notifications with a given config + async fn send_notification(config: &NotificationConfig, title: &str, message: &str) { if config.sound_enabled { Self::play_sound_notification(&config.sound_file).await; }