Refactor(v3): macOS async notification methods #4840
Conversation
WalkthroughNotification authorization methods were changed from synchronous return values to asynchronous callback-based signatures across the NotificationService and all platform implementations; the service identifier path was updated to the new package location. Changes
Sequence Diagram(s)%%{init: {"themeVariables": {"actorBackground":"#E8F5E9","actorBorder":"#2E7D32","noteBackground":"#FFF3E0","noteBorder":"#FB8C00"}}}%%
sequenceDiagram
participant App as Client
participant NS as NotificationService
participant PN as platformNotifier
participant OS as PlatformOS
App->>NS: RequestNotificationAuthorization(callback)
NS->>PN: RequestNotificationAuthorization(callback)
note right of PN `#FFF3E0`: Platform-specific work\n(may be async)
PN->>OS: ask system for permission
OS-->>PN: result (async)
PN-->>NS: invoke callback(authorized, err)
NS-->>App: callback invoked with (authorized, err)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
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 |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/pkg/services/notifications/notifications_darwin.go (1)
102-103: Redundant cleanup call is safe but potentially confusing.
GetChannel(called viacaptureResult) already removes the channel from the map before sending the result. ThecleanupChannel(id)call here will find the channel absent and do nothing. The comment acknowledges this ("GetChannel may have already removed it"), so this is intentional defensive coding.🔎 Optional: Remove redundant cleanup or clarify flow
If
captureResultalways removes the channel viaGetChannel, this cleanup is truly redundant. You could either:
- Remove the
cleanupChannel(id)call since it's a no-op in the success path- Keep it for defensive purposes (current approach) but ensure the comment is accurate
Current approach is fine for safety.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
v3/pkg/services/notifications/notifications.gov3/pkg/services/notifications/notifications_darwin.gov3/pkg/services/notifications/notifications_ios.gov3/pkg/services/notifications/notifications_linux.gov3/pkg/services/notifications/notifications_windows.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_darwin.go:39-46
Timestamp: 2025-03-23T00:41:39.612Z
Learning: For the macOS notifications implementation in Wails, an early panic is used when the bundle identifier check fails rather than returning an error, because the Objective-C code would crash later anyway. The panic provides clear instructions to developers about bundling and signing requirements.
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_windows.go:0-0
Timestamp: 2025-03-24T06:54:22.127Z
Learning: popaprozac prefers to focus on getting the Notifications API functionality working first and may consider code cleanup/refactoring for payload encoding logic in notifications_windows.go at a later time.
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
Applied to files:
v3/pkg/services/notifications/notifications_darwin.gov3/pkg/services/notifications/notifications_windows.gov3/pkg/services/notifications/notifications.gov3/pkg/services/notifications/notifications_ios.gov3/pkg/services/notifications/notifications_linux.go
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
Applied to files:
v3/pkg/services/notifications/notifications_darwin.gov3/pkg/services/notifications/notifications_windows.gov3/pkg/services/notifications/notifications.gov3/pkg/services/notifications/notifications_ios.gov3/pkg/services/notifications/notifications_linux.go
📚 Learning: 2025-03-24T06:54:22.127Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_windows.go:0-0
Timestamp: 2025-03-24T06:54:22.127Z
Learning: popaprozac prefers to focus on getting the Notifications API functionality working first and may consider code cleanup/refactoring for payload encoding logic in notifications_windows.go at a later time.
Applied to files:
v3/pkg/services/notifications/notifications_darwin.gov3/pkg/services/notifications/notifications_windows.go
📚 Learning: 2025-04-29T23:54:07.488Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4256
File: v2/internal/frontend/desktop/linux/notifications.go:27-28
Timestamp: 2025-04-29T23:54:07.488Z
Learning: In Wails v2, unlike v3-alpha which has a `ServiceShutdown` method for services, there is no standardized teardown pattern for frontend implementations. When implementing features that require cleanup (like goroutines or resources), add explicit cleanup methods (e.g., `CleanupNotifications()`) that handle resource release, context cancellation, and connection closure.
Applied to files:
v3/pkg/services/notifications/notifications_windows.gov3/pkg/services/notifications/notifications.go
🧬 Code graph analysis (5)
v3/pkg/services/notifications/notifications_darwin.go (1)
v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/notificationservice.ts (2)
RequestNotificationAuthorization(52-54)CheckNotificationAuthorization(17-19)
v3/pkg/services/notifications/notifications_windows.go (1)
v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/notificationservice.ts (2)
RequestNotificationAuthorization(52-54)CheckNotificationAuthorization(17-19)
v3/pkg/services/notifications/notifications.go (2)
v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/notificationservice.ts (2)
RequestNotificationAuthorization(52-54)CheckNotificationAuthorization(17-19)v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/index.ts (1)
NotificationService(6-6)
v3/pkg/services/notifications/notifications_ios.go (1)
v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/notificationservice.ts (2)
RequestNotificationAuthorization(52-54)CheckNotificationAuthorization(17-19)
v3/pkg/services/notifications/notifications_linux.go (1)
v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/notificationservice.ts (2)
RequestNotificationAuthorization(52-54)CheckNotificationAuthorization(17-19)
⏰ 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: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
🔇 Additional comments (8)
v3/pkg/services/notifications/notifications_ios.go (1)
38-50: LGTM! iOS stub implementation correctly adopts the callback-based API.The nil guards prevent potential panics, and immediately invoking callbacks with
(true, nil)is appropriate stub behavior for iOS where native bridge implementation is pending.v3/pkg/services/notifications/notifications_windows.go (1)
154-168: LGTM! Windows stub correctly implements the callback-based API.The nil guards and immediate callback invocation with
(true, nil)are appropriate since user authorization is macOS-specific. The comments accurately document this behavior.v3/pkg/services/notifications/notifications_darwin.go (2)
88-106: Async callback pattern looks good, but goroutine may leak if native code fails to respond.The removal of timeouts is intentional for user-facing authorization dialogs, but if the Objective-C callback never fires (e.g., due to a bug or edge case), the goroutine will block indefinitely on
<-resultCh.Consider whether this is acceptable given the PR's goal of removing timeouts, or if a safeguard (like a very long timeout with error callback) might be warranted for robustness.
108-126: CheckNotificationAuthorization follows the same async pattern - consistent implementation.Same considerations apply as
RequestNotificationAuthorization. The async callback approach is appropriate for the refactor goals.v3/pkg/services/notifications/notifications_linux.go (1)
99-113: LGTM! Linux stub correctly implements the callback-based API.The nil guards and immediate callback invocation with
(true, nil)are appropriate since authorization is macOS-specific. Consistent with Windows and iOS implementations.v3/pkg/services/notifications/notifications.go (3)
29-30: Interface updated to callback-based pattern - breaking change for implementers.The
platformNotifierinterface now requires callback-based signatures. All platform implementations in this PR correctly implement the new signatures. This is a clean interface change.
111-113: Unable to verify ServiceName path change claim without codebase access.The review comment asserts that the ServiceName path changed and represents a breaking change, but the repository could not be accessed for verification. Without being able to examine the codebase history and service registration patterns, I cannot confirm:
- What the previous service path was (if one existed)
- Whether this is actually a change or new code
- If existing code hardcodes references that need updates
- Whether this change is documented as breaking in the PR context
The review should clarify what the old path was and demonstrate concrete instances where code references it.
149-155: Verify callback parameter compatibility with Wails v3 binding generator.The review concern is technically valid: Wails v3 binding generation does not support callback function types (
func(bool, error)) as method parameters—these cannot be serialized across the bridge. The framework instead requires event-based patterns usingruntime.On/Oncefor asynchronous callbacks. Confirm whether the actual method signatures currently accept callbacks (incompatible) or have been refactored to use return values or event-based patterns (compatible). If callbacks are present, they must be replaced with the event-driven approach per Wails v3's binding constraints.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
v3/UNRELEASED_CHANGELOG.md (1)
23-23: Clarify that the API change affects all platforms.The changelog entry states "on macOS" but the method signatures changed across all platforms (darwin, iOS, Linux, Windows) to use callbacks. While the async behavior is primarily relevant for macOS, the breaking API change affects all platform implementations.
🔎 Suggested clarification
-- Update notifications authorization methods on macOS to callbacks +- Update notification authorization methods to use callbacks (breaking change)Or, if you prefer to emphasize the macOS motivation:
-- Update notifications authorization methods on macOS to callbacks +- Update notification authorization methods to async callbacks for macOS (all platforms affected)docs/src/content/docs/features/notifications/overview.mdx (2)
37-63: LGTM! Consider clarifying platform-specific requirements vs API design.The callback-based authorization flow is correctly documented with proper error handling. The nested callback pattern appropriately demonstrates the async authorization workflow.
One minor suggestion: Line 37 could be slightly clearer by separating the macOS-specific requirement from the cross-platform API design. For example: "Notifications on macOS require user authorization. The authorization methods use callbacks on all platforms:" This would clarify that while the requirement is macOS-specific, the callback-based API is used consistently across platforms.
236-239: Consider clarifying cross-platform availability in API descriptions.The API reference descriptions mention "(macOS)" which might suggest these methods are macOS-only. Consider clarifying that:
- These methods are available on all platforms
- The asynchronous behavior is primarily for macOS
- Windows/Linux invoke callbacks immediately (as noted in line 65)
🔎 Suggested enhancement
-| `RequestNotificationAuthorization(callback func(bool, error))` | Requests permission to display notifications (macOS). Callback is invoked asynchronously when the user responds. | -| `CheckNotificationAuthorization(callback func(bool, error))` | Checks current notification authorization status (macOS). Callback is invoked asynchronously with the result. | +| `RequestNotificationAuthorization(callback func(bool, error))` | Requests permission to display notifications. Callback is invoked asynchronously on macOS when the user responds; immediately on Windows/Linux. | +| `CheckNotificationAuthorization(callback func(bool, error))` | Checks current notification authorization status. Callback is invoked asynchronously on macOS; immediately on Windows/Linux with `authorized = true`. |
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/src/content/docs/features/notifications/overview.mdxv3/UNRELEASED_CHANGELOG.mdv3/pkg/services/notifications/notifications_darwin.go
🚧 Files skipped from review as they are similar to previous changes (1)
- v3/pkg/services/notifications/notifications_darwin.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_windows.go:0-0
Timestamp: 2025-03-24T06:54:22.127Z
Learning: popaprozac prefers to focus on getting the Notifications API functionality working first and may consider code cleanup/refactoring for payload encoding logic in notifications_windows.go at a later time.
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_darwin.go:39-46
Timestamp: 2025-03-23T00:41:39.612Z
Learning: For the macOS notifications implementation in Wails, an early panic is used when the bundle identifier check fails rather than returning an error, because the Objective-C code would crash later anyway. The panic provides clear instructions to developers about bundling and signing requirements.
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
Applied to files:
docs/src/content/docs/features/notifications/overview.mdx
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
Applied to files:
docs/src/content/docs/features/notifications/overview.mdx
📚 Learning: 2025-12-29T08:02:06.122Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4839
File: docs/src/content/docs/reference/window.mdx:616-620
Timestamp: 2025-12-29T08:02:06.122Z
Learning: In Wails v3, document window creation using app.Window.New() and app.Window.NewWithOptions(...). Do not show or reference app.NewWebviewWindow() or app.NewWebviewWindowWithOptions(...). The Application struct exposes a Window field of type *WindowManager that provides these methods. When updating docs, replace examples accordingly and mention that WindowManager methods create and configure new windows.
Applied to files:
docs/src/content/docs/features/notifications/overview.mdx
🔇 Additional comments (1)
docs/src/content/docs/features/notifications/overview.mdx (1)
65-65: Excellent clarification of cross-platform behavior.This note clearly explains how Windows and Linux handle the callback-based API, helping developers understand the behavior differences while maintaining API consistency.
1 participant
Description
Looking back at the implementation, especially for requesting macOS notification authorization it was done in a blocking way which doesn't make sense. We should register a callback so the user can authorize/deny the request async.
Updated the service name for imports.
Will be updating notifications docs and examples asap. I should probably update this in the v2 PR too 😅
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.If you checked Linux, please specify the distro and version.
Test Configuration
Please paste the output of
wails doctor. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.