event worker and meeting detector #1410
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRemoves automatic startup of event notifications during desktop init; refactors the notifications plugin to channel-based handler with new detect/event/handler modules and DetectState; updates settings UI to show a post-toggle confirmation toast; tweaks macOS notification visuals and examples; adds two plugin dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Tauri App
participant Plugin as Notification Plugin (core)
participant Handler as NotificationHandler (worker)
participant Detect as DetectState
participant EventMon as event::monitor
participant Hypr as hypr_detect
participant Notif as hypr_notification
rect #F0F8FF
App->>Plugin: manage(State::new(app_handle))
App->>Plugin: RunEvent::Ready
Plugin->>Detect: DetectState::start() (if detect enabled)
Plugin->>EventMon: spawn monitor task (if event enabled)
end
rect #F5FFF5
Hypr-->>Detect: DetectEvent (callback)
Detect->>Handler: send(NotificationTrigger::Detect)
Handler->>Handler: decide (window visibility, timing)
alt should notify
Handler->>Notif: show(notification payload)
else ignore
Handler-->>Handler: noop / log
end
end
sequenceDiagram
autonumber
participant UI as Settings UI
participant Ext as plugins::ext
participant Detect as DetectState
participant EventMon as event::monitor
UI->>Ext: toggle detect/event setting
Ext->>UI: mutation onSuccess
Ext->>UI: show "You're all set!" (10s) when enabled
Ext->>Detect: start()/stop()
Ext->>EventMon: spawn/cancel monitor task
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/notification/src/event.rs (1)
21-24: Fix tracing attribute: string literal required; also rename NAKE → NAME.
#[tracing::instrument(name = ...)]requires a string literal, not a const. Current name const is also misspelled.-const EVENT_NOTIFICATION_WORKER_NAKE: &str = "event_notification_worker"; +const EVENT_NOTIFICATION_WORKER_NAME: &str = "event_notification_worker"; -#[tracing::instrument(skip(ctx), name = EVENT_NOTIFICATION_WORKER_NAKE)] +#[tracing::instrument(skip(ctx), name = "event_notification_worker")] pub async fn perform_event_notification(_job: Job, ctx: Data<WorkerState>) -> Result<(), Error> { ... } - WorkerBuilder::new(EVENT_NOTIFICATION_WORKER_NAKE) + WorkerBuilder::new(EVENT_NOTIFICATION_WORKER_NAME)Also applies to: 79-80
🧹 Nitpick comments (8)
plugins/notification/src/event.rs (2)
1-1: Remove unused import.
WorkerFactoryFnisn’t used; keep imports clean per guidelines.-use apalis::prelude::{Data, Error, WorkerBuilder, WorkerFactoryFn}; +use apalis::prelude::{Data, Error, WorkerBuilder};
25-26: Log message vs schedule frequency.You run every minute (
"0 * * * * *") and look 5 minutes ahead. Consider clarifying the log to “Checking next 5 minutes (runs every minute)” to avoid confusion.apps/desktop/src/components/settings/views/notifications.tsx (1)
51-56: Consider awaiting start before showing the preview notification.If
notificationCommands.*return Promises, the preview may fire before the loop has started. MakeonSuccessasync and await the start call.- onSuccess: (active) => { + onSuccess: async (active) => { eventNotification.refetch(); if (active) { - notificationCommands.startEventNotification(); + await notificationCommands.startEventNotification(); notificationCommands.showNotification({ title: "You're all set!", message: "This is how notifications look.", timeout: { secs: 10, nanos: 0 }, url: null, });- onSuccess: (active) => { + onSuccess: async (active) => { detectNotification.refetch(); if (active) { - notificationCommands.startDetectNotification(); + await notificationCommands.startDetectNotification(); notificationCommands.showNotification({ title: "You're all set!", message: "This is how notifications look.", timeout: { secs: 10, nanos: 0 }, url: null, });If these APIs are synchronous, ignore this suggestion.
Also applies to: 76-81
plugins/notification/src/ext.rs (2)
95-97: Consider graceful shutdown of the monitor.Abort is fine, but if monitor spawns child tasks or does cleanup, you may want a cooperative stop signal inside crate::event::monitor for a clean exit.
111-116: Avoid blocking joins while holding SharedState lock.stop() may join a worker thread; doing this under the Mutex can block other callers. Consider spawning stop() on a blocking thread or refactoring stop() to release the lock before the join.
plugins/notification/src/detect.rs (2)
37-46: Set a notification key to leverage built-in dedup.Without a key, dedupe in hypr_notification is bypassed, risking spammy repeats if mic flaps.
Apply:
- hypr_notification::show( - &hypr_notification::Notification::builder() + hypr_notification::show( + &hypr_notification::Notification::builder() .title("Meeting detected") .message("Based on your microphone activity") .url("hypr://hyprnote.com/app/new?record=true") .timeout(std::time::Duration::from_secs(30)) + .key("meeting-detected".into()) .build(), );
81-83: Rename public method or make it private._pub fn is_running with a leading underscore is odd for a public API. Prefer is_running() or make it private if unused.
crates/notification-macos/swift-lib/src/lib.swift (1)
483-483: Consider adjusting the trailing constraint for better visual balance.The trailing constraint was reduced from -12 to -10, which might create visual inconsistency with the leading constraint of 12. This could result in uneven padding.
- contentView.trailingAnchor.constraint(equalTo: effectView.trailingAnchor, constant: -10), + contentView.trailingAnchor.constraint(equalTo: effectView.trailingAnchor, constant: -12),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
apps/desktop/src-tauri/src/lib.rs(0 hunks)apps/desktop/src/components/settings/views/notifications.tsx(2 hunks)crates/notification-macos/examples/test_notification.rs(3 hunks)crates/notification-macos/swift-lib/src/lib.swift(3 hunks)crates/notification/src/lib.rs(1 hunks)plugins/notification/Cargo.toml(1 hunks)plugins/notification/src/detect.rs(1 hunks)plugins/notification/src/event.rs(1 hunks)plugins/notification/src/ext.rs(4 hunks)plugins/notification/src/lib.rs(3 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src-tauri/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
crates/notification/src/lib.rsapps/desktop/src/components/settings/views/notifications.tsxplugins/notification/src/detect.rscrates/notification-macos/examples/test_notification.rsplugins/notification/src/event.rsplugins/notification/src/ext.rsplugins/notification/src/lib.rs
🧬 Code graph analysis (4)
plugins/notification/src/detect.rs (3)
plugins/notification/src/lib.rs (1)
new(25-30)crates/notification/src/lib.rs (1)
show(12-37)crates/detect/src/lib.rs (1)
new_callback(19-24)
crates/notification-macos/examples/test_notification.rs (2)
crates/notification-macos/swift-lib/src/lib.swift (1)
show(289-300)crates/notification-macos/src/lib.rs (1)
show(15-28)
plugins/notification/src/ext.rs (2)
plugins/notification/src/event.rs (1)
monitor(74-90)plugins/notification/src/commands.rs (1)
stop_detect_notification(58-62)
plugins/notification/src/lib.rs (2)
plugins/notification/src/detect.rs (1)
new(11-18)plugins/notification/src/ext.rs (5)
state(71-71)state(80-80)state(92-92)state(104-104)state(112-112)
⏰ 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). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
🔇 Additional comments (25)
plugins/notification/src/event.rs (1)
47-49: Potential type mismatch on.key(&event.id); pass&strorStringexplicitly.If the builder takes
impl Into<String>,&Stringwon’t match. Useevent.id.as_str()or clone.- .key(&event.id) + .key(event.id.as_str())Likely an incorrect or invalid review comment.
crates/notification-macos/examples/test_notification.rs (4)
8-12: Aqua appearance import/use looks good.Using
NSAppearancewithns_string!is correct for opting into Aqua in the sample.
40-43: Setting Aqua appearance conditionally is fine.The conditional guard protects older environments; no issues.
55-55: Longer demo timeout is reasonable.30s helps QA interactions; consistent with the sample app usage.
59-59: Extended sleep aligns with timeout.Keeps the sample process alive long enough for interaction.
crates/notification/src/lib.rs (1)
8-8: No-op formatting change.Whitespace-only; safe to keep.
plugins/notification/src/ext.rs (4)
41-42: Behavior change: default now off (event notifications). Confirm UX.Switching to unwrap_or(false) disables event notifications by default for fresh installs/missing keys. If intentional, ensure onboarding/UI conveys this and no regressions for existing users with absent store entries.
58-59: Behavior change: default now off (detect notifications). Confirm UX.Same as above for detect notifications. Verify this aligns with “no auto-start” goal and migration expectations.
103-108: LGTM: detect_state provides a simpler start path.Delegating to DetectState::start() improves encapsulation and eliminates callback plumbing here.
70-88: MSRV and Tokio Runtime Requirements VerifiedThe project’s Cargo.toml declares
rust-version = "1.88.0", satisfying the MSRV ≥ 1.75 requirement. All invocations ofstart_event_notificationoccur inside an active Tokio runtime (either viatokio::spawninplugins/notification/src/lib.rsor awaited within Tauri’s async command inplugins/notification/src/commands.rs). No changes are needed.plugins/notification/src/detect.rs (3)
54-58: Callback-to-channel bridge: OK, but verify Detector::start semantics.If Detector::start can fail or invoke callbacks after stop(), handle errors or guard tx.send with running state as needed.
67-76: Good stop order and cleanup.Stopping the detector before dropping tx, then joining the worker ensures rx loop exits deterministically.
86-90: Drop-based cleanup is a solid safety net.Ensures no background threads leak if the state is dropped unexpectedly.
plugins/notification/src/lib.rs (5)
21-22: State holds DetectState: good encapsulation.This removes ad-hoc detector handling from callers and centralizes lifecycle management.
24-30: Constructor pattern is cleaner than Default here.Explicit new(app_handle) makes dependencies obvious and avoids partial init.
61-68: Autostart gating for detect: LGTM.Respects user setting and logs outcome. Matches the “no auto-start” objective.
69-77: Async spawn for event monitor is fine; pair with idempotent start.Given the ext.rs fix to avoid duplicate spawns, this path looks good.
50-50: Init is now Wry-specific—no internal generic calls found; please verify external consumers and CI
- I ran ripgrep for both
notification::init<…>andnotification::init(across the repository and found no matches, indicating no remaining internalinit<R>usages.- Since this plugin is likely consumed by downstream applications and CI workflows outside this repo, please manually verify all those consumers are updated to call the new
init()(which returnsTauriPlugin<Wry>) and that no generic-init invocations remain.- Double-check any example projects, integration tests, and CI pipelines that reference this plugin crate to avoid unexpected breakages.
crates/notification-macos/swift-lib/src/lib.swift (7)
242-256: LGTM! Enhanced visual styling for action buttons.The styling updates improve the visual appearance with proper contrast, drop shadow effects, and platform-specific bezel coloring. The dark gray text on light background provides good readability.
475-475: LGTM! Button text change improves user experience.Changing the button text from "Open" to "Take Notes" provides clearer context about the action being performed when a URL is present.
508-518: LGTM! Icon size improvements enhance visual hierarchy.The icon container size increase from 36x36 to 42x42 and inner icon from 24x24 to 32x32 creates better visual prominence and improves the overall notification appearance.
523-523: LGTM! Text spacing adjustment improves readability.Increasing the text stack spacing from 3 to 4 points provides better visual separation between the title and body text.
529-534: LGTM! Title typography enhancements improve readability.The font size increase from 14 to 16 with semibold weight makes the title more prominent and easier to read in the notification interface.
537-541: LGTM! Body text styling creates better visual hierarchy.The lighter font weight and smaller size (12pt) for the body text, combined with the single-line limitation, creates a clean and focused notification layout.
550-568: Ensure consistent button layout across varying content lengthsThe notification uses a fixed width of 360 pt (Config.notificationWidth at lib.swift:281) and the action button plus its leading gap are configured with
.requiredcontent-hugging and compression-resistance priorities (lib.swift:554–557, 561–568). This setup should preserve the button’s full width and push any overflow into the message label, but it’s worth verifying manually:• Confirm that very long button titles do not get clipped or overlap adjacent views
• Check behavior when the notification body text is lengthy—ensure truncation or line-wrapping happens as expected
• Test edge cases such as multi-word titles with no whitespace and maximum character countsPlease run your UI tests or manual smoke tests on macOS to validate these scenarios.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
plugins/notification/src/ext.rs (1)
86-97: Log monitor failures in the spawned task.You already abort existing tasks (nice). Also log errors from event::monitor for observability.
- s.worker_handle = Some(tokio::runtime::Handle::current().spawn(async move { - let _ = crate::event::monitor(crate::event::WorkerState { - db, - user_id, - notification_tx, - }) - .await; - })); + s.worker_handle = Some(tokio::runtime::Handle::current().spawn(async move { + if let Err(e) = crate::event::monitor(crate::event::WorkerState { + db, + user_id, + notification_tx, + }) + .await + { + tracing::error!(?e, "event_notification_monitor_failed"); + } + }));
🧹 Nitpick comments (5)
plugins/notification/src/handler.rs (1)
33-35: Name the worker thread for easier debugging.Helps trace panics and thread dumps.
- let handle = std::thread::spawn(vec![] , move || { + let handle = std::thread::Builder::new() + .name("notification_handler".into()) + .spawn(move || { Self::worker_loop(rx, app_handle); - }); + }) + .expect("spawn notification_handler");plugins/notification/src/event.rs (2)
24-27: Typo in constant name: use EVENT_NOTIFICATION_WORKER_NAME.Minor readability fix; update usages accordingly.
-const EVENT_NOTIFICATION_WORKER_NAKE: &str = "event_notification_worker"; +const EVENT_NOTIFICATION_WORKER_NAME: &str = "event_notification_worker";And:
-#[tracing::instrument(skip(ctx), name = EVENT_NOTIFICATION_WORKER_NAKE)] +#[tracing::instrument(skip(ctx), name = EVENT_NOTIFICATION_WORKER_NAME)]-WorkerBuilder::new(EVENT_NOTIFICATION_WORKER_NAKE) +WorkerBuilder::new(EVENT_NOTIFICATION_WORKER_NAME)
47-51: Defensive clamp for minutes_until_start.Clock skew could make this negative; clamp to non-negative before sending.
- let minutes_until_start = event + let mut minutes_until_start = event .start_date .signed_duration_since(Utc::now()) .num_minutes(); + if minutes_until_start < 0 { + minutes_until_start = 0; + }plugins/notification/src/ext.rs (1)
84-85: Avoid unwrap on notification_tx acquisition.sender() can be None if the handler was stopped; unwrap() will panic. Return an Error instead.
Example (adjust Error variant as appropriate):
- let notification_tx = s.notification_handler.sender().unwrap(); + let notification_tx = match s.notification_handler.sender() { + Some(tx) => tx, + None => { + tracing::error!("notification_tx_unavailable"); + return Err(Error::Other("notification handler not running".into())); + } + };Please confirm the concrete Error variant to use here.
plugins/notification/src/detect.rs (1)
22-31: Handle missing notification_tx gracefully.start() unwraps the Option and will panic if DetectState was constructed when the handler had no sender. Prefer early-return with an Error.
- let notification_tx = self.notification_tx.as_ref().unwrap().clone(); + let Some(notification_tx) = self.notification_tx.as_ref().cloned() else { + return Err(Error::Other("notification channel unavailable".into())); + };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
apps/desktop/src/locales/en/messages.po(2 hunks)apps/desktop/src/locales/ko/messages.po(2 hunks)crates/detect/src/lib.rs(1 hunks)crates/whisper-local/src/model.rs(0 hunks)plugins/notification/Cargo.toml(1 hunks)plugins/notification/src/detect.rs(1 hunks)plugins/notification/src/event.rs(3 hunks)plugins/notification/src/ext.rs(4 hunks)plugins/notification/src/handler.rs(1 hunks)plugins/notification/src/lib.rs(3 hunks)
💤 Files with no reviewable changes (1)
- crates/whisper-local/src/model.rs
✅ Files skipped from review due to trivial changes (3)
- apps/desktop/src/locales/en/messages.po
- apps/desktop/src/locales/ko/messages.po
- plugins/notification/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/notification/Cargo.toml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
crates/detect/src/lib.rsplugins/notification/src/detect.rsplugins/notification/src/event.rsplugins/notification/src/handler.rsplugins/notification/src/ext.rs
🧬 Code graph analysis (4)
plugins/notification/src/detect.rs (3)
plugins/notification/src/handler.rs (4)
std(31-31)new(30-41)stop(125-131)drop(135-137)plugins/notification/src/lib.rs (1)
new(27-36)crates/detect/src/lib.rs (5)
start(27-27)start(49-51)new_callback(19-24)stop(28-28)stop(53-55)
plugins/notification/src/event.rs (1)
plugins/notification/src/handler.rs (1)
std(31-31)
plugins/notification/src/handler.rs (3)
plugins/windows/js/bindings.gen.ts (1)
HyprWindow(73-73)plugins/notification/src/detect.rs (3)
new(12-17)stop(40-46)drop(54-56)plugins/notification/src/lib.rs (1)
new(27-36)
plugins/notification/src/ext.rs (2)
plugins/notification/src/event.rs (1)
monitor(67-83)plugins/notification/src/commands.rs (1)
stop_detect_notification(58-62)
⏰ 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). (2)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
🔇 Additional comments (6)
crates/detect/src/lib.rs (1)
10-15: Deriving Clone on DetectEvent is appropriate.This enables cheap forwarding across channels without ownership gymnastics. No issues found.
plugins/notification/src/handler.rs (1)
72-75: Align meeting detector timeout with PR objective (30s).PR states 30 seconds; code uses 60 seconds.
- .timeout(std::time::Duration::from_secs(60)) + .timeout(std::time::Duration::from_secs(30))plugins/notification/src/event.rs (1)
52-61: LGTM: channel handoff replaces UI coupling.Clean separation: workers only emit triggers; UI building centralized in handler.
plugins/notification/src/ext.rs (2)
36-42: Defaulting toggles to false matches “opt-in” requirement.Good change; aligns with PR.
117-120: Detect state start/stop wiring looks correct.Start is idempotent via internal stop; stop clears detector. Good encapsulation.
Also applies to: 125-128
plugins/notification/src/detect.rs (1)
19-38: LGTM: forwarding detect events over MPSC decouples runtime from UI.Clean callback usage with tracing on send failure; stop-before-start avoids duplicate detectors.
| #[derive(Debug, Clone)] | ||
| pub enum NotificationTrigger { | ||
| Detect(NotificationTriggerDetect), | ||
| Event(NotificationTriggerEvent), | ||
| } |
There was a problem hiding this comment.
Add a Shutdown variant to allow clean worker termination.
Without an explicit shutdown signal, stop() may block forever on join() if other Sender clones still exist (DetectState, WorkerState), because rx.recv() never returns Err. Add a Shutdown message and break the loop on receipt.
Apply:
#[derive(Debug, Clone)]
pub enum NotificationTrigger {
Detect(NotificationTriggerDetect),
Event(NotificationTriggerEvent),
+ Shutdown,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[derive(Debug, Clone)] | |
| pub enum NotificationTrigger { | |
| Detect(NotificationTriggerDetect), | |
| Event(NotificationTriggerEvent), | |
| } | |
| #[derive(Debug, Clone)] | |
| pub enum NotificationTrigger { | |
| Detect(NotificationTriggerDetect), | |
| Event(NotificationTriggerEvent), | |
| Shutdown, | |
| } |
🤖 Prompt for AI Agents
In plugins/notification/src/handler.rs around lines 6 to 10, add a Shutdown
variant to the NotificationTrigger enum and update the handler loop to match it
and break/exit cleanly on receipt; specifically, modify the enum to include
Shutdown, update all match arms that handle NotificationTrigger to include a
Shutdown branch that stops processing and breaks the receive loop (or returns),
and adjust any callers/senders to be able to send NotificationTrigger::Shutdown
for clean worker termination so that stop()'s join does not block when other
Sender clones still exist.
| fn worker_loop(rx: Receiver<NotificationTrigger>, app_handle: AppHandle<tauri::Wry>) { | ||
| while let Ok(trigger) = rx.recv() { | ||
| match trigger { | ||
| NotificationTrigger::Detect(t) => { | ||
| Self::handle_detect_event(&app_handle, t); | ||
| } | ||
| NotificationTrigger::Event(e) => { | ||
| Self::handle_calendar_event(&app_handle, e); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Break the worker loop on Shutdown to avoid join deadlocks.
- while let Ok(trigger) = rx.recv() {
+ while let Ok(trigger) = rx.recv() {
match trigger {
NotificationTrigger::Detect(t) => {
Self::handle_detect_event(&app_handle, t);
}
NotificationTrigger::Event(e) => {
Self::handle_calendar_event(&app_handle, e);
}
+ NotificationTrigger::Shutdown => {
+ break;
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn worker_loop(rx: Receiver<NotificationTrigger>, app_handle: AppHandle<tauri::Wry>) { | |
| while let Ok(trigger) = rx.recv() { | |
| match trigger { | |
| NotificationTrigger::Detect(t) => { | |
| Self::handle_detect_event(&app_handle, t); | |
| } | |
| NotificationTrigger::Event(e) => { | |
| Self::handle_calendar_event(&app_handle, e); | |
| } | |
| } | |
| } | |
| } | |
| fn worker_loop(rx: Receiver<NotificationTrigger>, app_handle: AppHandle<tauri::Wry>) { | |
| while let Ok(trigger) = rx.recv() { | |
| match trigger { | |
| NotificationTrigger::Detect(t) => { | |
| Self::handle_detect_event(&app_handle, t); | |
| } | |
| NotificationTrigger::Event(e) => { | |
| Self::handle_calendar_event(&app_handle, e); | |
| } | |
| NotificationTrigger::Shutdown => { | |
| break; | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In plugins/notification/src/handler.rs around lines 47 to 58, the worker_loop
currently treats all NotificationTrigger variants the same and never exits on a
Shutdown signal, which can cause the thread to hang on join; update the match to
explicitly handle a NotificationTrigger::Shutdown (or add it if missing) by
breaking out of the loop (or returning) when received, and ensure any other
variants are processed as before so the thread can terminate cleanly and avoid
join deadlocks.
| .key(&format!( | ||
| "event_{}_{}", | ||
| trigger.event_id, | ||
| trigger.minutes_until_start < 3 | ||
| )) | ||
| .title(trigger.event_name.clone()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a stable notification key = event.id for deduplication.
Including a boolean in the key creates multiple keys per event and defeats dedup goals.
- .key(&format!(
- "event_{}_{}",
- trigger.event_id,
- trigger.minutes_until_start < 3
- ))
+ .key(&trigger.event_id)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .key(&format!( | |
| "event_{}_{}", | |
| trigger.event_id, | |
| trigger.minutes_until_start < 3 | |
| )) | |
| .title(trigger.event_name.clone()) | |
| .key(&trigger.event_id) | |
| .title(trigger.event_name.clone()) |
🤖 Prompt for AI Agents
In plugins/notification/src/handler.rs around lines 98 to 103, the notification
key currently includes a boolean (minutes_until_start < 3) which creates
multiple keys per event and breaks deduplication; change the key to a stable
identifier based only on the event id (e.g., "event_<event_id>" or
event_id.to_string()) so the same event always maps to a single notification
key, leaving title and other fields unchanged.
| pub fn stop(&mut self) { | ||
| self.tx = None; | ||
|
|
||
| if let Some(handle) = self.handle.take() { | ||
| let _ = handle.join(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Send Shutdown before joining the thread.
Ensure the receiver unblocks and exits deterministically; otherwise join() may hang if other Senders are still alive.
- pub fn stop(&mut self) {
- self.tx = None;
-
- if let Some(handle) = self.handle.take() {
- let _ = handle.join();
- }
- }
+ pub fn stop(&mut self) {
+ if let Some(tx) = self.tx.take() {
+ let _ = tx.send(NotificationTrigger::Shutdown);
+ }
+ if let Some(handle) = self.handle.take() {
+ let _ = handle.join();
+ }
+ }🤖 Prompt for AI Agents
In plugins/notification/src/handler.rs around lines 125 to 131, the stop()
method currently drops the sender before joining the worker thread which can
leave the receiver blocked; instead, take the sender and send an explicit
shutdown message (or enum variant) to the worker so the receiver unblocks
deterministically, then drop the sender and join the thread. Use Option::take on
self.tx to extract the sender, call send(shutdown_message) and ignore the Result
if needed, then proceed to take and join the handle.
Summary by cubic
Adds a microphone-based meeting detector and reworks event notifications to start from the plugin based on user settings. Refreshes the macOS notification UI and makes both notification types opt-in by default.
New Features
Refactors