daonanchao/vibe-kanban
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Play notification sound when approval needed (vibe-kanban) (#1463)

Browse Source 
* 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<RwLock<Config>>` 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<RwLock<Config>>` 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<RwLock<Config>>`
   - 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<RwLock<Config>>` 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<RwLock<Config>>` 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
This commit is contained in:
Louis Knight-Webb
2025-12-09 09:57:42 +00:00
committed by GitHub
parent 8846e6621c
commit 21d175bccc
4 changed files with 72 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files

25
crates/local-deployment/src/container.rs
View File

@@ -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<SharePublisher, RemoteClientNotConfigured>,
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,
)
}

19
crates/services/src/services/approvals/executor_approvals.rs
View File

@@ -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<Self> {
pub fn new(
approvals: Approvals,
db: DBService,
notification_service: NotificationService,
execution_process_id: Uuid,
) -> Arc<Self> {
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) {

31
crates/services/src/services/container.rs
View File

@@ -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<ContainerRef, ContainerError>;
@@ -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<RwLock<Config>>,
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

50
crates/services/src/services/notification.rs
View File

@@ -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<RwLock<Config>>,
}
/// Cache for WSL root path from PowerShell
static WSL_ROOT_PATH_CACHE: OnceLock<Option<String>> = 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<RwLock<Config>>) -> 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;
}