feat(macos): add wait_for_action and on_close - #1
yasumorishima wants to merge 6 commits into
Conversation
Wire up the synchronous response flow exposed by mac-notification-sys so that NotificationHandle::wait_for_action and NotificationHandle::on_close behave on macOS the same way they do on XDG. Notifications carrying any action() are now delivered when the user awaits a response, mirroring the blocking semantics of the underlying crate; fire-and-forget notifications keep the existing behaviour. Move the shared types (ActionResponse, CloseReason, CloseHandler, ActionResponseHandler) from src/xdg/mod.rs into a new src/action.rs so they can be re-exported at the crate root and shared between platforms. Adds examples/mac_actions.rs and updates the platform-support table. Refs: hoodie#186
📝 WalkthroughWalkthroughAdds a new cross-platform action module and types, re-exports them at crate root, updates macOS backend to defer delivery for notifications with actions and provide blocking ChangesNotification Action Handler Support
Sequence DiagramsequenceDiagram
actor User
participant App as Application Code
participant Handle as NotificationHandle (pending)
participant MacSys as mac_notification_sys
participant OS as macOS Notification Center
App->>Handle: create notification with actions & call show()
Handle->>App: return pending handle
App->>Handle: call wait_for_action(callback)
Handle->>MacSys: show_blocking(wait_for_click=true)
MacSys->>OS: display notification
User->>OS: click action or close
OS->>MacSys: deliver NotificationResponse
MacSys-->>Handle: return response
Handle->>App: invoke callback with ActionResponse::Custom or ActionResponse::Closed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/macos.rs`:
- Around line 79-82: The current match arm for
Ok(NotificationResponse::ActionButton(label)) uses
identifier_for_label(&label).map_or_else(String::new, str::to_owned) which
returns an empty string on lookup miss; change it to fall back to the clicked
label instead. Update the map_or_else call in src/macos.rs (the
NotificationResponse::ActionButton arm and the identifier_for_label call) so
that the closure returns the original label (converted/owned) when
identifier_for_label returns None, rather than String::new().
🪄 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: 55200545-11db-4bcc-897e-4817062d4838
📒 Files selected for processing (7)
examples/mac_actions.rssrc/action.rssrc/lib.rssrc/macos.rssrc/xdg/dbus_rs.rssrc/xdg/mod.rssrc/xdg/zbus_rs.rs
Per CodeRabbit suggestion: returning an empty string on a missed lookup hides what the user actually clicked. Falling back to the label itself lets callers handle the unknown case via the closures default arm.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Replace the dead-code dummy function with #[allow(unused_imports)] on the use statement. ActionResponse appears only in a rustdoc link in macos.rs, so the import is needed for link resolution but never referenced from code.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/macos.rs`:
- Around line 181-198: The else branch in show_notification currently defers
delivery for notifications with actions (returning NotificationHandle::pending),
making .show() a no-op; instead build the mac_notification_sys::Notification
with actions (use its .action(...) API when iterating notification.actions),
call .send() so the notification is displayed immediately, and then return
NotificationHandle::new(notification.clone()) so existing fire-and-forget call
sites still see the notification; remove or repurpose
NotificationHandle::pending only for cases where you truly want deferred
delivery and keep NotificationHandle::new as the default for sent notifications.
- Around line 110-114: The on_close implementation currently synthesizes
CloseReason::Dismissed for non-pending handles; instead, only invoke the
CloseHandler when the macOS notification actually closes (i.e., after observing
the OS close event or after show_blocking returns a real close result). Update
on_close (and the show_blocking usage) so that: if self.pending is true, call
show_blocking and only call handler.call(...) with the observed close reason
returned by show_blocking/OS callback; if self.pending is false, do NOT
synthesize CloseReason::Dismissed—either register the handler to be invoked by
the real OS close callback or return/reject the unsupported case (do not call
handler.call early). Ensure you reference on_close, show_blocking, CloseHandler,
and CloseReason::Dismissed when making the change.
🪄 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: 949b2c36-2956-4888-acdb-a9bae181dd16
📒 Files selected for processing (2)
examples/mac_actions.rssrc/macos.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/mac_actions.rs
| pub fn on_close<A>(self, handler: impl CloseHandler<A>) { | ||
| if self.pending { | ||
| let _ = self.show_blocking(); | ||
| } | ||
| handler.call(CloseReason::Dismissed); |
There was a problem hiding this comment.
on_close() fires before any real close event for non-pending handles.
When pending is false, this reports CloseReason::Dismissed immediately even though a normal show() notification may still be on screen, and a scheduled notification has not closed yet at all. That breaks the CloseHandler contract and can trigger cleanup/state transitions too early. Please avoid calling the handler unless macOS has actually observed the notification closing, or explicitly reject unsupported cases instead of synthesizing a close event.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/macos.rs` around lines 110 - 114, The on_close implementation currently
synthesizes CloseReason::Dismissed for non-pending handles; instead, only invoke
the CloseHandler when the macOS notification actually closes (i.e., after
observing the OS close event or after show_blocking returns a real close
result). Update on_close (and the show_blocking usage) so that: if self.pending
is true, call show_blocking and only call handler.call(...) with the observed
close reason returned by show_blocking/OS callback; if self.pending is false, do
NOT synthesize CloseReason::Dismissed—either register the handler to be invoked
by the real OS close callback or return/reject the unsupported case (do not call
handler.call early). Ensure you reference on_close, show_blocking, CloseHandler,
and CloseReason::Dismissed when making the change.
| pub(crate) fn show_notification(notification: &Notification) -> Result<NotificationHandle> { | ||
| let mut n = mac_notification_sys::Notification::default(); | ||
| n.title(notification.summary.as_str()) | ||
| .message(¬ification.body) | ||
| .maybe_subtitle(notification.subtitle.as_deref()) | ||
| .maybe_sound(notification.sound_name.as_deref()); | ||
| if notification.actions.is_empty() { | ||
| let mut n = mac_notification_sys::Notification::default(); | ||
| n.title(notification.summary.as_str()) | ||
| .message(¬ification.body) | ||
| .maybe_subtitle(notification.subtitle.as_deref()) | ||
| .maybe_sound(notification.sound_name.as_deref()); | ||
|
|
||
| if let Some(ref image_path) = notification.path_to_image { | ||
| n.content_image(image_path); | ||
| } | ||
| if let Some(ref image_path) = notification.path_to_image { | ||
| n.content_image(image_path); | ||
| } | ||
|
|
||
| n.send()?; | ||
| n.send()?; | ||
|
|
||
| Ok(NotificationHandle::new(notification.clone())) | ||
| Ok(NotificationHandle::new(notification.clone())) | ||
| } else { | ||
| Ok(NotificationHandle::pending(notification.clone())) | ||
| } |
There was a problem hiding this comment.
Don't turn show() into a no-op for notifications with actions.
This branch defers delivery entirely, so .action(...).show() no longer displays anything on macOS unless the caller later invokes wait_for_action() or on_close(). That is a behavioral regression for existing fire-and-forget call sites that ignore the returned handle, and it breaks the core expectation that show() actually shows the notification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/macos.rs` around lines 181 - 198, The else branch in show_notification
currently defers delivery for notifications with actions (returning
NotificationHandle::pending), making .show() a no-op; instead build the
mac_notification_sys::Notification with actions (use its .action(...) API when
iterating notification.actions), call .send() so the notification is displayed
immediately, and then return NotificationHandle::new(notification.clone()) so
existing fire-and-forget call sites still see the notification; remove or
repurpose NotificationHandle::pending only for cases where you truly want
deferred delivery and keep NotificationHandle::new as the default for sent
notifications.
|
Closing this preflight PR. It exists only to run CI and CodeRabbit on the fork before opening the upstream PR, and that job is done. The branch is kept; closing these keeps "disappeared from my open PR list" a reliable signal that an upstream PR was merged. |
2 participants
Summary
Wires the synchronous response flow exposed by
mac-notification-sysintonotify-rust'sNotificationHandle::wait_for_actionandNotificationHandle::on_closeso that listening for action clicks works on macOS the same way it does on XDG. Refs hoodie#186.Why this is small in scope
mac-notification-sys0.6.12 already exposes:NotificationResponse(Click / ActionButton / CloseButton / Reply / None)wait_for_click(true)synchronous flowMainButton::SingleAction/DropdownActions/Responsenotify-rustwas not wiring any of this up:src/macos.rs::show_notificationonly forwarded title/body/subtitle/sound/image and discarded theNotificationResponse. This PR is mostly plumbing.Changes
src/action.rsmodule — hoistsActionResponse,CloseReason,CloseHandlerandActionResponseHandlerout ofsrc/xdg/mod.rsso they can be re-exported at the crate root and shared between platforms.src/macos.rs— addsNotificationHandle::wait_for_actionandNotificationHandle::on_close. Mapsmac_notification_sys::NotificationResponse::ActionButton(label)back to the identifier originally passed toNotification::action(identifier, label).src/macos.rs::show_notification— interactive notifications (those with anyaction()configured) are now deferred untilwait_for_action/on_closeis invoked, sincemac_notification_sys::Notification::sendis blocking and only returns once the user interacts. Fire-and-forget notifications keep the existing behaviour.src/xdg/{mod,dbus_rs,zbus_rs}.rs— switch fromsuper::{ActionResponse, ...}tocrate::{ActionResponse, ...}to use the hoisted definitions.src/lib.rs— re-exports the shared types at the crate root, updates the platform-support table to mark macOS forwait_for_action,on_close, andaction, and documents the macOS-specific blocking semantics.examples/mac_actions.rs— macOS counterpart ofexamples/actions.rs, with both single-action and dropdown variants.Why defer interactive sends?
mac_notification_sys::Notification::sendis synchronous — it delivers the notification and blocks until the user dismisses it or activates an action. TheNotificationResponseis only available after that blocking call returns. The XDG flow expectsshow()to return a handle quickly and the response to be observed later viawait_for_action.Two ways to reconcile this on macOS:
show()and store the response on the handle. This makesshow()itself blocking on macOS, which silently breaks any caller that expectedshow()to return promptly.wait_for_action/on_close. Interactive notifications still go through a singlesendcall, but at the point where the caller is already prepared to block. Fire-and-forget notifications (noaction()) keep the eager-send behaviour.This PR takes option (2). Happy to switch to (1) or to a different design (e.g. an explicit
wait()builder method) — the wiring itself is the same either way, and this is the question I'd most like maintainer input on.What I am NOT changing
tauri-winrt-notificationvs the newwin32_notif). Out of scope here.wait_for_action(|action: &str|)signature is preserved as-is, including the"__closed"magic string. Milestone 5 (Feature/milestone 5 hoodie/notify-rust#266) plans to migrate this toFnOnce(ActionResponse); this PR doesn't move the API yet so it stays compatible with current code on both XDG and macOS.Limitations / things to verify
cargo test --no-runandclippyonmacos-latestare the actual confidence signal here. The XDG side has been verified locally withcargo check --no-default-features --features zandcargo clippy --no-default-features --features z -- -D warnings(clean).NotificationResponse, soon_closealways reportsCloseReason::Dismissed. Documented at the call site.Click(body click) on a notification with at least one configured action is currently mapped to the first action's identifier. Open to changing this to"__default"or to always emitting"__closed"if you'd prefer.Test plan
cargo check --features z(Linux) — cleancargo clippy --features z -- -D warnings(Linux) — cleancargo check(macOS, via CI) — pendingcargo test --no-run(macOS, via CI) — pendingcargo clippy -- -D warnings(macOS, via CI) — pendingcargo run --example mac_actionson a real Mac — out of my reach, would appreciate a sanity checkDraft for now — opening so CodeRabbit / CodeQL can have a look and so the macOS CI matrix runs.
Summary by CodeRabbit
New Features
Documentation