feat: configurable notifications + webhook (Slack/Discord)#47
Merged
kienbui1995 merged 1 commit intomainfrom Apr 13, 2026
Merged
feat: configurable notifications + webhook (Slack/Discord)#47kienbui1995 merged 1 commit intomainfrom
kienbui1995 merged 1 commit intomainfrom
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds config-backed notifications: Changes
Sequence Diagram(s)sequenceDiagram
participant TUI as "TUI / run_tui"
participant Config as "RuntimeConfig"
participant OS as "OS notifier\n(notify-send / osascript)"
participant Task as "Tokio task\n(reqwest)"
participant Webhook as "External Webhook"
TUI->>Config: read notifications & webhook fields
alt notifications == true
TUI->>OS: emit bell / desktop notification
end
alt notification_webhook is Some(url)
TUI->>Task: spawn async POST task(url, payload)
Task->>Webhook: POST {"text":"magic-code: turn complete"}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (1)
mc/crates/mc-cli/src/main.rs (1)
515-526: Consider logging webhook failures for debuggability.Webhook errors are silently discarded, which can make troubleshooting difficult if users configure an invalid URL or encounter network issues. A trace-level log would help without being noisy.
♻️ Suggested improvement
if let Some(ref url) = config.notification_webhook { let url = url.clone(); tokio::spawn(async move { let client = reqwest::Client::new(); - let _ = client + if let Err(e) = client .post(&url) .json(&serde_json::json!({"text": "magic-code: turn complete"})) .send() - .await; + .await + { + tracing::debug!("webhook notification failed: {e}"); + } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-cli/src/main.rs` around lines 515 - 526, The webhook POST in the tokio::spawn block currently discards errors; update the async task that creates reqwest::Client::new() and calls .post(&url).json(...).send().await to inspect the Result and log failures at trace level (e.g., via tracing::trace! or log::trace!), including the error details and a brief context like "notification_webhook POST failed" and the notification identifier (but avoid logging sensitive payload); reference config.notification_webhook, the tokio::spawn async closure, and the .send().await call to locate where to add the Result handling and trace logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mc/crates/mc-config/src/types.rs`:
- Around line 160-163: The new RuntimeConfig fields notifications and
notification_webhook are never read from TOML because DefaultLayer lacks them
and from_layers hardcodes values; update DefaultLayer to include pub
notifications: Option<bool> and pub notification_webhook: Option<String>, then
in RuntimeConfig::from_layers where layers are iterated (the layer merge loop
inside from_layers) read and merge these fields from each layer (e.g., if
layer.notifications.is_some() set the accumulating value, same for
notification_webhook) and finally use the merged values when constructing Self
in from_layers instead of the hardcoded literals so TOML-provided values
override defaults.
---
Nitpick comments:
In `@mc/crates/mc-cli/src/main.rs`:
- Around line 515-526: The webhook POST in the tokio::spawn block currently
discards errors; update the async task that creates reqwest::Client::new() and
calls .post(&url).json(...).send().await to inspect the Result and log failures
at trace level (e.g., via tracing::trace! or log::trace!), including the error
details and a brief context like "notification_webhook POST failed" and the
notification identifier (but avoid logging sensitive payload); reference
config.notification_webhook, the tokio::spawn async closure, and the
.send().await call to locate where to add the Result handling and trace logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c67f115-dcd0-4374-b519-92cd7ebecdeb
⛔ Files ignored due to path filters (1)
mc/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
mc/crates/mc-cli/Cargo.tomlmc/crates/mc-cli/src/main.rsmc/crates/mc-config/src/types.rs
Comment on lines
+160
to
+163
| /// Enable desktop/bell notifications when agent finishes. Default: true. | ||
| pub notifications: bool, | ||
| /// Webhook URL to POST when agent completes a turn (Slack/Discord/custom). | ||
| pub notification_webhook: Option<String>, |
There was a problem hiding this comment.
Configuration fields are not read from TOML layers — settings will have no effect.
The notifications and notification_webhook fields are added to RuntimeConfig, but they're hardcoded in from_layers (lines 296-297) without reading from any layer. The DefaultLayer struct is missing these fields, so the TOML config example from the PR description won't work:
[default]
notifications = false # This will be ignored!
notification_webhook = "https://..." # This will be ignored!To fix this:
- Add the fields to
DefaultLayerwithOption<>wrappers - Read and merge them in the
from_layersloop
🐛 Proposed fix
Add fields to DefaultLayer:
pub struct DefaultLayer {
pub provider: Option<String>,
pub model: Option<String>,
pub max_tokens: Option<u32>,
pub permission_mode: Option<String>,
+ pub notifications: Option<bool>,
+ pub notification_webhook: Option<String>,
#[serde(default)]
pub tool_permissions: std::collections::HashMap<String, String>,
}Add merging logic in from_layers (inside the layer loop):
+ let mut notifications: Option<bool> = None;
+ let mut notification_webhook: Option<String> = None;
// ... in the for loop:
+ if let Some(v) = layer.default.notifications {
+ notifications = Some(v);
+ }
+ if let Some(ref v) = layer.default.notification_webhook {
+ notification_webhook = Some(v.clone());
+ }Then in the Self { ... } block:
- notifications: true,
- notification_webhook: None,
+ notifications: notifications.unwrap_or(true),
+ notification_webhook,Also applies to: 296-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-config/src/types.rs` around lines 160 - 163, The new
RuntimeConfig fields notifications and notification_webhook are never read from
TOML because DefaultLayer lacks them and from_layers hardcodes values; update
DefaultLayer to include pub notifications: Option<bool> and pub
notification_webhook: Option<String>, then in RuntimeConfig::from_layers where
layers are iterated (the layer merge loop inside from_layers) read and merge
these fields from each layer (e.g., if layer.notifications.is_some() set the
accumulating value, same for notification_webhook) and finally use the merged
values when constructing Self in from_layers instead of the hardcoded literals
so TOML-provided values override defaults.
- notifications: true/false in config (default: true)
- notification_webhook: optional URL for Slack/Discord/custom
- Bell + desktop notification gated by config
- Webhook POSTs {"text": "magic-code: turn complete"} on each turn
197 tests, 0 warnings.
kienbui1995
force-pushed
the
feat/notification-config
branch
from
03a482a to
cdbaded
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Notification Config
notificationstruenotification_webhookWebhook payload:
{"text": "magic-code: turn complete"}— compatible with Slack incoming webhooks and Discord webhooks.Config example:
197 tests, 0 warnings.
Summary by CodeRabbit
New Features
Chores