Refactor handlers for all notifications#2328
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughNotification events were centralized and extended (Confirm, Accept, Dismiss, Timeout). Platform crates (gpui, macOS, Linux) add setup hooks for new events and wire them to FFI/timeout/accept paths. A theme module was added for GPUI. Analytics integration was refactored to a manager-wrapped Analytics struct; plugin handlers emit analytics on notification events. Changes
Sequence Diagram(s)sequenceDiagram
participant Notifier as Notification (OS)
participant Platform as Platform crate (gpui/macos/linux)
participant Handler as plugins/notification::handler
participant Analytics as plugins/analytics::Analytics
participant App as App / Main Window
Note over Notifier,Platform: User action or timeout generates event
Notifier->>Platform: emit NotificationEvent (Confirm/Accept/Dismiss/Timeout)
Platform->>Handler: invoke registered handler (setup_notification_* handlers)
Handler->>App: try show main window (non-fatal)
Handler->>Analytics: build AnalyticsPayload and call analytics().event(...)
Analytics-->>Handler: ack (async)
Handler-->>Platform: (optional) call platform-specific callbacks (CONFIRM_CB/ACCEPT_CB/DISMISS_CB/TIMEOUT_CB)
Note over Platform,App: On timeout acceptance/dismiss, platform may also run internal dismiss/close logic
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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. Comment |
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/notification-gpui/src/lib.rs (1)
78-89: Registered callbacks are never invoked when events occur.The setup functions store callbacks in
CONFIRM_CB,ACCEPT_CB,DISMISS_CB, andTIMEOUT_CB, but whenNotificationEventvariants are matched here, the code only callsclose_windowwithout invoking the corresponding callbacks. Compare this to the Linux implementation wherecall_timeout_handlerandcall_dismiss_handlerare explicitly called.Consider invoking the callbacks before closing the window:
cx.subscribe( &toast_entity, move |_, event: &NotificationEvent, cx| match event { - NotificationEvent::Accept - | NotificationEvent::Dismiss - | NotificationEvent::Confirm - | NotificationEvent::Timeout => { + NotificationEvent::Accept => { + if let Some(cb) = ACCEPT_CB.lock().unwrap().as_ref() { + cb(/* notification id */); + } + close_window(cx, window_id); + } + NotificationEvent::Dismiss => { + if let Some(cb) = DISMISS_CB.lock().unwrap().as_ref() { + cb(/* notification id */); + } + close_window(cx, window_id); + } + NotificationEvent::Confirm => { + if let Some(cb) = CONFIRM_CB.lock().unwrap().as_ref() { + cb(/* notification id */); + } + close_window(cx, window_id); + } + NotificationEvent::Timeout => { + if let Some(cb) = TIMEOUT_CB.lock().unwrap().as_ref() { + cb(/* notification id */); + } close_window(cx, window_id); } }, )Note: You'll need to track and pass the notification ID to invoke the callbacks correctly.
🧹 Nitpick comments (6)
plugins/notification/Cargo.toml (1)
21-21: The "legacy" feature is properly defined as the default feature.The feature exists in crates/notification/Cargo.toml and enables platform-specific notification implementations (macOS and Linux). This is part of the notification system refactoring to support both legacy and new event type paths.
Consider documenting the purpose and lifecycle of this feature flag—specifically whether the "legacy" feature is temporary during the refactoring period or permanent for ongoing backward compatibility.
plugins/notification/src/handler.rs (1)
5-39: Optional: Consider reducing duplication across handler blocks.The four handler blocks follow a similar pattern. While the current implementation is clear and maintainable, you could consider extracting common logic if this pattern grows.
crates/notification-macos/src/lib.rs (1)
61-92: Consider handling invalid UTF-8 gracefully at the FFI boundary.The
to_str().unwrap()calls will panic if the Swift side passes invalid UTF-8. While this matches the existing pattern forrust_on_notification_confirmandrust_on_notification_dismiss, panicking in an extern "C" function invoked from Swift is undefined behavior and could crash the application unexpectedly.Consider using
to_string_lossy()for robustness:pub unsafe extern "C" fn rust_on_notification_accept(id_ptr: *const c_char) { if let Some(cb) = ACCEPT_CB.lock().unwrap().as_ref() { - let id = unsafe { CStr::from_ptr(id_ptr) } - .to_str() - .unwrap() - .to_string(); + let id = unsafe { CStr::from_ptr(id_ptr) } + .to_string_lossy() + .into_owned(); cb(id); } }Note: This applies to all four extern functions for consistency.
crates/notification-gpui/src/theme.rs (1)
51-62: Button colors are not theme-aware.The
close_button_*andaction_button_*colors are identical for both light and dark themes. This may cause contrast issues—for example, a dark close button background (hsla(0., 0., 0., 0.5)) may not be visible against a dark notification background. If this is intentional, no change needed.plugins/analytics/src/ext.rs (2)
45-70:app_identifierandbundle_idare set to identical values.Both properties are assigned
manager.config().identifier.clone()(lines 47 and 49). If they're meant to be the same, consider removing one to reduce redundancy. If they should differ,bundle_idmay need a different source.fn enrich_payload(manager: &M, payload: &mut hypr_analytics::AnalyticsPayload) { let app_version = env!("APP_VERSION"); let app_identifier = manager.config().identifier.clone(); let git_hash = manager.get_git_hash(); - let bundle_id = manager.config().identifier.clone(); payload .props .entry("app_version".into()) .or_insert(app_version.into()); payload .props .entry("app_identifier".into()) .or_insert(app_identifier.into()); payload .props .entry("git_hash".into()) .or_insert(git_hash.into()); - payload - .props - .entry("bundle_id".into()) - .or_insert(bundle_id.into()); }
86-101: Inconsistent error handling foris_disabled()across methods.In
event()(line 16) andevent_fire_and_forget()(line 33), errors fromis_disabled()are silently handled viaunwrap_or(true). However, inset_properties()(line 90), the error is propagated with?. This inconsistency could lead to unexpected behavior differences.Consider aligning the behavior:
pub async fn set_properties( &self, payload: hypr_analytics::PropertiesPayload, ) -> Result<(), crate::Error> { - if !self.is_disabled()? { + if self.is_disabled().unwrap_or(true) { + return Ok(()); + } + let machine_id = hypr_host::fingerprint(); let client = self.manager.state::<crate::ManagedState>(); client .set_properties(machine_id, payload) .await .map_err(crate::Error::HyprAnalytics)?; - } Ok(()) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
crates/notification-gpui/examples/test_notification.rs(1 hunks)crates/notification-gpui/src/event.rs(0 hunks)crates/notification-gpui/src/lib.rs(4 hunks)crates/notification-gpui/src/theme.rs(1 hunks)crates/notification-gpui/src/toast.rs(7 hunks)crates/notification-interface/src/lib.rs(1 hunks)crates/notification-linux/src/impl.rs(3 hunks)crates/notification-linux/src/lib.rs(1 hunks)crates/notification-macos/src/lib.rs(4 hunks)crates/notification-macos/swift-lib/src/lib.swift(4 hunks)crates/notification/src/lib.rs(1 hunks)plugins/analytics/src/commands.rs(3 hunks)plugins/analytics/src/ext.rs(3 hunks)plugins/analytics/src/lib.rs(1 hunks)plugins/notification/Cargo.toml(1 hunks)plugins/notification/src/handler.rs(1 hunks)plugins/notification/src/lib.rs(2 hunks)plugins/windows/src/ext.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/notification-gpui/src/event.rs
🧰 Additional context used
📓 Path-based instructions (1)
plugins/*/src/lib.rs
📄 CodeRabbit inference engine (plugins/AGENTS.md)
After updating commands in
plugins/<NAME>/src/lib.rs, runcodegen, updateplugins/<NAME>/permissions/default.toml, andapps/desktop/src-tauri/capabilities/default.json
Files:
plugins/notification/src/lib.rsplugins/analytics/src/lib.rs
🧠 Learnings (2)
📚 Learning: 2025-11-27T11:40:22.782Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: plugins/AGENTS.md:0-0
Timestamp: 2025-11-27T11:40:22.782Z
Learning: Applies to plugins/*/src/lib.rs : After updating commands in `plugins/<NAME>/src/lib.rs`, run `codegen`, update `plugins/<NAME>/permissions/default.toml`, and `apps/desktop/src-tauri/capabilities/default.json`
Applied to files:
plugins/notification/src/lib.rsplugins/analytics/src/commands.rs
📚 Learning: 2025-11-26T06:09:32.213Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: crates/notification-linux/AGENTS.md:0-0
Timestamp: 2025-11-26T06:09:32.213Z
Learning: Applies to crates/notification-linux/**/*notification-linux*/**/*.{rs,toml} : Use gtk3 (not gtk4) because webkit2gtk-4.1 links to gtk3
Applied to files:
crates/notification/src/lib.rscrates/notification-linux/src/impl.rscrates/notification-gpui/src/theme.rscrates/notification-gpui/examples/test_notification.rscrates/notification-gpui/src/lib.rsplugins/notification/Cargo.tomlcrates/notification-linux/src/lib.rscrates/notification-gpui/src/toast.rs
🧬 Code graph analysis (11)
plugins/notification/src/lib.rs (1)
plugins/notification/src/handler.rs (1)
init(4-40)
crates/notification/src/lib.rs (3)
crates/notification-gpui/src/lib.rs (2)
setup_notification_accept_handler(35-40)setup_notification_timeout_handler(42-47)crates/notification-linux/src/impl.rs (2)
setup_notification_accept_handler(35-40)setup_notification_timeout_handler(42-47)crates/notification-macos/src/lib.rs (2)
setup_notification_accept_handler(36-41)setup_notification_timeout_handler(43-48)
crates/notification-linux/src/impl.rs (3)
crates/notification-gpui/src/lib.rs (2)
setup_notification_accept_handler(35-40)setup_notification_timeout_handler(42-47)crates/notification-macos/src/lib.rs (2)
setup_notification_accept_handler(36-41)setup_notification_timeout_handler(43-48)crates/notification/src/lib.rs (2)
setup_notification_accept_handler(96-108)setup_notification_timeout_handler(110-122)
plugins/notification/src/handler.rs (1)
crates/notification/src/lib.rs (4)
setup_notification_confirm_handler(82-94)setup_notification_accept_handler(96-108)setup_notification_dismiss_handler(68-80)setup_notification_timeout_handler(110-122)
crates/notification-macos/src/lib.rs (2)
crates/notification-gpui/src/lib.rs (2)
setup_notification_accept_handler(35-40)setup_notification_timeout_handler(42-47)crates/notification/src/lib.rs (2)
setup_notification_accept_handler(96-108)setup_notification_timeout_handler(110-122)
crates/notification-gpui/examples/test_notification.rs (3)
crates/notification-gpui/src/toast.rs (1)
new(21-28)crates/notification-gpui/src/lib.rs (1)
show(64-107)crates/notification/src/lib.rs (1)
show(27-55)
crates/notification-gpui/src/lib.rs (3)
crates/notification-gpui/src/toast.rs (2)
theme(30-33)new(21-28)crates/notification-macos/src/lib.rs (4)
setup_notification_dismiss_handler(22-27)setup_notification_confirm_handler(29-34)setup_notification_accept_handler(36-41)setup_notification_timeout_handler(43-48)crates/notification/src/lib.rs (4)
setup_notification_dismiss_handler(68-80)setup_notification_confirm_handler(82-94)setup_notification_accept_handler(96-108)setup_notification_timeout_handler(110-122)
plugins/analytics/src/ext.rs (3)
plugins/analytics/src/commands.rs (4)
event(5-13)set_disabled(29-36)is_disabled(40-44)set_properties(17-25)crates/analytics/src/lib.rs (2)
event(22-50)set_properties(52-88)crates/host/src/lib.rs (1)
fingerprint(12-18)
plugins/analytics/src/commands.rs (1)
plugins/analytics/src/ext.rs (1)
is_disabled(80-84)
crates/notification-linux/src/lib.rs (1)
crates/notification-linux/src/impl.rs (6)
setup_notification_accept_handler(35-40)setup_notification_confirm_handler(28-33)setup_notification_dismiss_handler(21-26)setup_notification_timeout_handler(42-47)show(145-180)show(330-340)
crates/notification-gpui/src/toast.rs (1)
crates/notification-gpui/src/theme.rs (1)
colors(32-63)
⏰ 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: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
🔇 Additional comments (25)
plugins/windows/src/ext.rs (1)
85-85: LGTM! Analytics routing updated correctly.The change routes analytics events through the new
analytics()accessor, aligning with the broader refactor. Error handling is preserved.plugins/analytics/src/lib.rs (1)
90-90: LGTM! Test updated for new analytics API.The test now correctly uses the
analytics()accessor to emit events, validating the new API pattern.crates/notification-gpui/examples/test_notification.rs (1)
4-14: LGTM! Example updated to use local notification API.The example now correctly uses
notification_gpui::Notificationandnotification_gpui::show, aligning with the crate's own implementation. The removal of the.url()call is consistent with the updatedNotificationAPI.plugins/notification/src/lib.rs (2)
6-6: LGTM! Good separation of concerns.Extracting handler setup to a dedicated module improves maintainability.
29-29: LGTM! Handler initialization delegated cleanly.The inline handler setup has been properly extracted to
handler::init(), improving code organization.plugins/analytics/src/commands.rs (1)
9-12: LGTM! Analytics routing updated correctly.All command functions now route through the
analytics()accessor while preserving error handling and return types.crates/notification-linux/src/lib.rs (1)
8-9: LGTM! API expanded with new event handlers.The addition of
setup_notification_accept_handlerandsetup_notification_timeout_handlerexpands the Linux notification API consistently with the broader event handling refactor.crates/notification-interface/src/lib.rs (1)
1-7: LGTM! Centralized event type with appropriate derives.The new
NotificationEventenum consolidates event types (Confirm, Accept, Dismiss, Timeout) into a single source of truth. The derives are well-suited for an event enum.crates/notification/src/lib.rs (1)
95-122: LGTM!The new
setup_notification_accept_handlerandsetup_notification_timeout_handlerfunctions follow the established pattern from the existing dismiss/confirm handlers. The feature flag and platform-specific conditional compilation is correctly applied, consistent with the platform implementations shown in the relevant code snippets.crates/notification-gpui/src/toast.rs (3)
5-5: LGTM!The import changes correctly centralize event types from
hypr_notification_interfaceand theme types fromcrate::theme, improving code organization and cross-crate consistency.Also applies to: 11-11
83-154: LGTM!The rendering logic now properly delegates color selection to
self.theme.colors(), eliminating duplicated light/dark mode logic. The event emissions use the correctNotificationEvent::DismissandNotificationEvent::Acceptvariants from the interface crate.
233-235: NotificationEvent::Accept variant is correctly defined and used.The
Acceptvariant exists in theNotificationEventenum defined incrates/notification-interface/src/lib.rs. The code at lines 233-235 properly emits this variant, which aligns with the interface definition. BothConfirmandAcceptvariants are available in the enum, maintaining semantic distinction as designed.crates/notification-macos/src/lib.rs (1)
18-20: LGTM!The new static callback registries (
ACCEPT_CB,TIMEOUT_CB) and setup functions follow the established pattern from the existing confirm/dismiss handlers consistently.Also applies to: 36-48
crates/notification-macos/swift-lib/src/lib.swift (3)
16-23: LGTM!The timer-based dismissal now correctly routes through
dismissWithTimeout(), which emits the timeout event before performing the standard dismiss animation. This properly distinguishes timeout-based dismissals from user-initiated ones.Also applies to: 46-51
267-279: LGTM!The
ActionButton.mouseDowncorrectly emits the accept event viarustOnNotificationAcceptand then callsnotification.dismiss()directly (notdismissWithUserAction()), which appropriately avoids emitting both accept and dismiss events for the same user action.
768-776: LGTM!The
@_silgen_namedeclarations correctly map to the corresponding Rust extern "C" functions, completing the Swift-to-Rust interop for accept and timeout events.crates/notification-gpui/src/theme.rs (2)
24-30:Systemtheme defaults to light mode without detecting OS preference.The
is_light()method treatsNotificationTheme::Systemthe same asLight, which means notifications won't adapt to the OS dark mode setting when using the default theme. If this is intentional to match existing behavior, consider documenting it; otherwise, you may want to query the actual system appearance.
1-64: LGTM on the overall structure.The
NotificationThemeenum andNotificationColorsstruct provide a clean abstraction for theming, and thecolors()method centralizes color selection logic that was previously scattered across rendering code.crates/notification-linux/src/impl.rs (3)
95-98: Timeout handler integration looks correct.The timeout callback is invoked before window dismissal, and the
idis properly cloned for use in the closure. This aligns with the expected event flow.
16-47: Callback registration is consistent with other platforms.The static callbacks and setup functions follow the same pattern as the macOS and GPUI implementations, ensuring cross-platform consistency.
55-59:call_accept_handleris never invoked.The function is defined but has no call sites in the codebase, unlike
call_timeout_handlerwhich is called at line 96. This indicates either missing integration for an "Accept" button action or dead code that should be removed.crates/notification-gpui/src/lib.rs (2)
93-105: Timeout event emission is correctly implemented.The async spawn properly waits for the timeout duration and emits the
Timeoutevent to the toast entity. The entity clone is necessary for the move into the async block.
15-47: Callback storage and setup functions are consistent across platforms.The implementation mirrors the Linux and macOS versions, maintaining cross-platform consistency for the notification handler registration pattern.
plugins/analytics/src/ext.rs (2)
4-7: Clean refactor from trait methods to wrapper struct.The
Analyticsstruct with theAnalyticsPluginExttrait provides a cleaner API surface. The lifetime binding ensures the wrapper doesn't outlive the manager, and thePhantomDatacorrectly handles the unused runtime type parameter.Also applies to: 104-120
30-43: Fire-and-forget implementation is correct.The method properly clones the client to satisfy the
'staticrequirement of the spawned task. Silent error handling (let _ =) is appropriate for this fire-and-forget use case.
1 participant
No description provided.