Notification fix 2 #1310
Notification fix 2 #1310
Conversation
📝 WalkthroughWalkthroughThe function responsible for event notifications now includes additional logging at key steps and wraps the notification display call in a panic-catching block. This ensures that any panics during notification display are logged as errors, while successful notifications are logged as informational messages. No public interfaces or function signatures were changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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 comments)
Other keywords and placeholders
Documentation and Community
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
No issues found across 1 file. Review in cubic
![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 (2)
plugins/notification/src/worker.rs (2)
25-25: Consider lowering to debug to reduce noiseThis runs every minute; “checking for events” at info level may be too chatty. Suggest using debug, or include structured fields if you need it at info.
42-42: Add structured context and consider deduping notificationsInclude event_id (and maybe event start time) as structured fields for traceability. Also, given this job runs every minute with a 5‑minute window, you may re-notify the same event multiple times. Consider a de-duplication guard.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/notification/src/worker.rs(2 hunks)
🧰 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:
plugins/notification/src/worker.rs
⏰ 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: cubic · AI code reviewer
- GitHub Check: ci (windows, windows-latest)
plugins/notification/src/worker.rs
Outdated
| if let Err(e) = std::panic::catch_unwind(|| { | ||
| hypr_notification2::show(hypr_notification2::Notification { | ||
| title: "Meeting starting in 5 minutes".to_string(), | ||
| message: event.name.clone(), | ||
| url: Some(format!( | ||
| "hypr://hyprnote.com/notification?event_id={}", | ||
| event.id | ||
| )), | ||
| timeout: Some(std::time::Duration::from_secs(10)), | ||
| }); | ||
| }) { | ||
| tracing::error!(" Notification panic: {:?}", e); | ||
| } else { | ||
| tracing::info!(" Notification shown"); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Fix compile error when logging panic payload; ensure unwind-safety; remove stray spaces; and justify the policy exception
- Box<dyn Any + Send> from catch_unwind doesn’t implement Debug;
{:?}won’t compile. Downcast to &str/String before logging. - Use AssertUnwindSafe if the closure captures non-UnwindSafe refs.
- Remove leading whitespace in log messages.
- Per org guideline “No error handling” for rs files: this introduces panic handling. If this is necessary (known FFI/UI panic), add a minimal “why” comment to justify the exception, or drop the catch_unwind entirely.
Proposed patch for this block:
- if let Err(e) = std::panic::catch_unwind(|| {
- hypr_notification2::show(hypr_notification2::Notification {
- title: "Meeting starting in 5 minutes".to_string(),
- message: event.name.clone(),
- url: Some(format!(
- "hypr://hyprnote.com/notification?event_id={}",
- event.id
- )),
- timeout: Some(std::time::Duration::from_secs(10)),
- });
- }) {
- tracing::error!(" Notification panic: {:?}", e);
- } else {
- tracing::info!(" Notification shown");
- }
+ // Why: hypr_notification2::show has been observed to panic on some platforms;
+ // guard to keep the worker alive (exception to the "no error handling" guideline).
+ if let Err(panic_payload) = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+ hypr_notification2::show(hypr_notification2::Notification {
+ title: "Meeting starting in 5 minutes".to_string(),
+ message: event.name.clone(),
+ url: Some(format!("hypr://hyprnote.com/notification?event_id={}", event.id)),
+ timeout: Some(std::time::Duration::from_secs(10)),
+ });
+ })) {
+ let panic_msg = if let Some(s) = panic_payload.downcast_ref::<&str>() {
+ *s
+ } else if let Some(s) = panic_payload.downcast_ref::<String>() {
+ s.as_str()
+ } else {
+ "unknown panic payload"
+ };
+ tracing::error!(event_id = %event.id, panic_message = %panic_msg, "Notification panicked");
+ } else {
+ tracing::info!(event_id = %event.id, "Notification shown");
+ }If the policy exception isn’t acceptable, remove the catch_unwind and let failures surface (or handle a Result from show if available).
Fix panic logging and enforce unwind-safety in the notification worker
The current use of catch_unwind won’t compile (Box<dyn Any + Send> isn’t Debug), and we should explicitly mark the closure as unwind-safe, remove stray spaces in the logs, and document why we’re breaking the “no error handling” guideline.
• File: plugins/notification/src/worker.rs (lines 44–58)
• Change logging of panic payload to a downcasted string
• Wrap the closure in AssertUnwindSafe
• Trim leading whitespace from log messages
• Add a brief comment justifying the catch_unwind usage
- if let Err(e) = std::panic::catch_unwind(|| {
- hypr_notification2::show(hypr_notification2::Notification {
- title: "Meeting starting in 5 minutes".to_string(),
- message: event.name.clone(),
- url: Some(format!(
- "hypr://hyprnote.com/notification?event_id={}",
- event.id
- )),
- timeout: Some(std::time::Duration::from_secs(10)),
- });
- }) {
- tracing::error!(" Notification panic: {:?}", e);
- } else {
- tracing::info!(" Notification shown");
- }
+ // Rationale: `hypr_notification2::show` can panic (FFI/UI edge cases).
+ // We catch unwinds to keep the worker alive (exception to the “no error handling” policy).
+ if let Err(panic_payload) = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+ hypr_notification2::show(hypr_notification2::Notification {
+ title: "Meeting starting in 5 minutes".to_string(),
+ message: event.name.clone(),
+ url: Some(format!("hypr://hyprnote.com/notification?event_id={}", event.id)),
+ timeout: Some(std::time::Duration::from_secs(10)),
+ });
+ })) {
+ let panic_msg = if let Some(s) = panic_payload.downcast_ref::<&str>() {
+ *s
+ } else if let Some(s) = panic_payload.downcast_ref::<String>() {
+ s.as_str()
+ } else {
+ "unknown panic payload"
+ };
+ tracing::error!(event_id = %event.id, panic_message = %panic_msg, "Notification panicked");
+ } else {
+ tracing::info!(event_id = %event.id, "Notification shown");
+ }📝 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.
| if let Err(e) = std::panic::catch_unwind(|| { | |
| hypr_notification2::show(hypr_notification2::Notification { | |
| title: "Meeting starting in 5 minutes".to_string(), | |
| message: event.name.clone(), | |
| url: Some(format!( | |
| "hypr://hyprnote.com/notification?event_id={}", | |
| event.id | |
| )), | |
| timeout: Some(std::time::Duration::from_secs(10)), | |
| }); | |
| }) { | |
| tracing::error!(" Notification panic: {:?}", e); | |
| } else { | |
| tracing::info!(" Notification shown"); | |
| } | |
| // Rationale: `hypr_notification2::show` can panic (FFI/UI edge cases). | |
| // We catch unwinds to keep the worker alive (exception to the “no error handling” policy). | |
| if let Err(panic_payload) = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { | |
| hypr_notification2::show(hypr_notification2::Notification { | |
| title: "Meeting starting in 5 minutes".to_string(), | |
| message: event.name.clone(), | |
| url: Some(format!("hypr://hyprnote.com/notification?event_id={}", event.id)), | |
| timeout: Some(std::time::Duration::from_secs(10)), | |
| }); | |
| })) { | |
| let panic_msg = if let Some(s) = panic_payload.downcast_ref::<&str>() { | |
| *s | |
| } else if let Some(s) = panic_payload.downcast_ref::<String>() { | |
| s.as_str() | |
| } else { | |
| "unknown panic payload" | |
| }; | |
| tracing::error!(event_id = %event.id, panic_message = %panic_msg, "Notification panicked"); | |
| } else { | |
| tracing::info!(event_id = %event.id, "Notification shown"); | |
| } |
🤖 Prompt for AI Agents
In plugins/notification/src/worker.rs lines 44 to 58, fix the panic logging by
downcasting the panic payload to a string for proper Debug output, wrap the
closure passed to catch_unwind with AssertUnwindSafe to enforce unwind-safety,
remove leading spaces from the tracing log messages, and add a brief comment
explaining why catch_unwind is used despite the usual no error handling
guideline.
Summary by cubic
Improved event notification reliability by catching panics when showing notifications and adding more logging for better debugging.