fix(notifications): address code-review P2s (NS fallback cfg-gate + SAFETY docs) - #33
Merged
Merged
Conversation
… docs) - Cfg-gate the `tauri-plugin-notification` fallback blocks in show_recording_started_notification and show_recording_stopped_notification to #[cfg(not(target_os = "macos"))]. On macOS those paths would route through the deprecated NSUserNotification API (which doesn't deliver banners) and could race with our UNUserNotificationCenterDelegate via last-writer-wins setDelegate:. On macOS, if the manager fails to initialize, log a warn and drop the notification rather than silently deliver via the broken path. - Add per-unsafe-block SAFETY comments throughout macos_un.rs: the define_class! attributes (super = NSObject, NSObjectProtocol, UNUserNotificationCenterDelegate, method selector), the msg_send![super, init] in BannerDelegate::new, and the error_message helper (including a /// # Safety doc). Closes todos/001, todos/002 from the review of PR #32. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
AzimovS
added a commit
that referenced
this pull request
Apr 21, 2026
`UNUserNotificationCenter` dereferences `NSBundle.mainBundle` and throws `NSInternalInconsistencyException: bundleProxyForCurrentProcess is nil` when the executable is launched directly (e.g. `pnpm run tauri dev`, which runs `target/debug/meetily` rather than the bundled `.app`). Since commits #32/#33 migrated macOS notifications from the legacy `NSUserNotification` path to UN, every `tauri dev` run crashes at startup. Skip the UN calls when `current_exe()` has no `.app` ancestor. `request_authorization` returns `Err` (not `Ok(false)`) so `manager.rs` falls into its existing `Err` arm and does not persist `system_permission_granted = false` — the dev run and the bundled `.app` share `~/Library/Application Support/com.meetily.ai/`, so persisting `false` would silently suppress every real notification until the user re-granted consent. `show()` returns `Ok(())`; callers do not store state on success. Bundled `.app` behavior is unchanged.
AzimovS
added a commit
that referenced
this pull request
Apr 24, 2026
Prepares a release containing the work merged since v0.1.15: - feat(detection): mic-activity meeting auto-detection on macOS (#35) - feat(transcription): retry with backoff + in-transcript failure placeholder (#39) - feat(summary): TownHall template (#30), current template name in dropdown (#31), specificity prompt tweak (#37) - feat(remote): test-connection button + model selection surface improvements (#28, #29) - fix(notifications): migrate to UNUserNotificationCenter (#32), SAFETY + fallback race fixes (#33), unbundled-dev crash guard (#38), drop OS recording banners and flip auto_save default to false (#36) - chore(about): drop Zackriya services CTA (#40) Behavior change to call out in release notes: fresh installs (and users with no stored recording_preferences.json) now default auto_save to false — audio files are not written to disk unless the user opts in via Recording Settings. Existing users with saved preferences are unaffected. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
Follow-up to PR #32. Addresses two P2 findings from the post-merge code review that were committed locally but didn't make it into the merged squash. Re-applying as a standalone PR for clean traceability.
No runtime behavior change for the macOS banner happy path — this is hardening + rot prevention.
Changes
1. Cfg-gate the
tauri-plugin-notificationNS fallback branches on macOSFile:
frontend/src-tauri/src/notifications/commands.rsThe fallback blocks in
show_recording_started_notificationandshow_recording_stopped_notificationdirectly callapp_handle.notification().builder().title(...).body(...).show()when the notification manager fails to initialize. On macOS this routes through the deprecatedNSUserNotificationpath — which is exactly the broken path PR #32 set out to avoid. Worse, if a future version oftauri-plugin-notificationinstalls its ownUNUserNotificationCenterDelegate, it would race with ourBannerDelegatevia last-writer-winssetDelegate:, silently unwiring thewillPresent → Banner|List|Soundhook.Fix: wrap the fallback blocks with
#[cfg(not(target_os = "macos"))]. On macOS, if the manager fails to init, log awarn!and drop the notification rather than deliver via the broken path. Also cfg-gate theNotificationExtimport.2. Expand SAFETY comments on unsafe boundaries in
macos_un.rsFile:
frontend/src-tauri/src/notifications/macos_un.rsAdd
// SAFETY:comments or/// # Safetydocs to every unsafe site:define_class!block: super-class, NSObjectProtocol conformance, UNUserNotificationCenterDelegate conformance, and#[unsafe(method(...))]selector ABImsg_send![super(this), init]inBannerDelegate::newerror_message(error)inside the RcBlock closuresunsafe fn error_messageitself (added/// # Safetydoc)Testing
cargo checkclean on macOS (same preexisting warnings, no new ones)Post-Deploy Monitoring & Validation
No additional operational monitoring required: comments-only changes on macOS behavioral paths, and cfg-gated fallback branches on Windows/Linux (no change from pre-merge behavior). The only observable difference is a new
warn!log line on macOS when the notification manager fails to init — which has always been a degraded edge case path, previously silently failing via NS.Related
todos/001-complete-p2-cfg-gate-plugin-fallback-branches-macos.md,todos/002-complete-p2-add-safety-comments-unsafe-ffi-macos-un.md🤖 Generated with Claude Code