Repository navigation
Refactor(v3): macOS async notification methods - #4840
popaprozac wants to merge 8 commits into
Conversation
WalkthroughNotification authorization methods were converted from synchronous returns to asynchronous, callback-based signatures across NotificationService and all platform implementations; ServiceName was updated to the new package path. Changes
Sequence Diagram(s)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)
PN->>OS: perform platform-specific authorization request
OS-->>PN: return 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
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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://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.
What are the implications of the current method? Surely it's not much of a problem? I asked Claude to do a quick review and picked up on a few things. Are you sure you want to go with this approach? It seems fraught with danger 😅 Code Review Summary: PR #4840 What The PR Does Issues Identified
// v3/examples/notifications/frontend/src/main.ts
const authorized = await NotificationService.RequestNotificationAuthorization();JavaScript cannot pass a Go function as a parameter. The generated TypeScript bindings expect a return value, not a callback.
go func() {
result := <-resultCh // Blocks indefinitely if no response
callback(result.Success, result.Error)
}()
|
|
The current method on macOS has a timeout before returning false if the user didn't interact with the notification to grant authorization. Having a callback ensures anytime the user decides to grant notification, even after letting it sit in their notification center for 2 hours, we get a signal when that happens. You could build a habit of always requesting authorization before running any notification code but this seems less idiomatic for the platform. macOS provides us several signals/events and notification authorization should be treated the same. We could split the difference and add an Issues:
|
|
I think it's ok to ask permission at the start - many apps do this. If they don't grant it after a certain time then notifications won't work. I think that's fine. Notifications are runtime features so asking permissions just before they happen seems weird. We could have an app-level flag indicating if permissions are granted or not (true on Linux and Windows by default?). |
|
I follow and that seems reasonable enough to me. I'll look at adding the app-level flag. Because of this conversation I went down a rabbit hole looking at how to support callbacks in method bindings... |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @v3/UNRELEASED_CHANGELOG.md:
- Around line 22-23: Update the changelog entry to mark this as a breaking
change and clarify platform scope: change the line about "Update notifications
authorization methods on macOS to callbacks" to a **BREAKING** entry that states
RequestNotificationAuthorization() and CheckNotificationAuthorization() now use
the callback signature func(bool, error) across all platforms (noting the
implementation is macOS-specific and other platforms are stubs) and add a brief
migration note instructing callers to replace synchronous return-value usage
with the new callback pattern.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/UNRELEASED_CHANGELOG.md
🧰 Additional context used
🧠 Learnings (1)
📓 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.
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.
⏰ 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)
| <!-- Changes in existing functionality --> | ||
| - Update notifications authorization methods on macOS to callbacks |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual changelog file to see the full context
cd /tmp/repo && cat -n v3/UNRELEASED_CHANGELOG.md | head -40Repository: wailsapp/wails
Length of output: 117
🏁 Script executed:
# Search for notification authorization related files and changes
cd /tmp/repo && find . -type f -name "*.go" | xargs grep -l "RequestNotificationAuthorization\|CheckNotificationAuthorization" | head -20Repository: wailsapp/wails
Length of output: 117
🏁 Script executed:
# Look for changelog entries related to notifications
cd /tmp/repo && rg -i "notification.*authorization" -A 2 -B 2 v3/UNRELEASED_CHANGELOG.mdRepository: wailsapp/wails
Length of output: 117
🏁 Script executed:
# Get current directory and list files
pwd && ls -la | head -20Repository: wailsapp/wails
Length of output: 1281
🏁 Script executed:
# Check if v3/UNRELEASED_CHANGELOG.md exists
ls -la v3/UNRELEASED_CHANGELOG.md 2>/dev/null && wc -l v3/UNRELEASED_CHANGELOG.mdRepository: wailsapp/wails
Length of output: 163
🏁 Script executed:
# Read the changelog file to see context around lines 22-23
cat -n v3/UNRELEASED_CHANGELOG.md | head -40Repository: wailsapp/wails
Length of output: 1476
🏁 Script executed:
# Search for notification authorization related code changes
rg "RequestNotificationAuthorization|CheckNotificationAuthorization" -t go -B 2 -A 5Repository: wailsapp/wails
Length of output: 7841
🏁 Script executed:
# Search for recent changes related to notifications in go files
rg -i "notification.*callback|callback.*notification" -t go -B 2 -A 2Repository: wailsapp/wails
Length of output: 8856
🏁 Script executed:
# Look for notification files in v3 directory
find v3 -type f -name "*.go" | xargs grep -l -i "notification" | head -20Repository: wailsapp/wails
Length of output: 846
🏁 Script executed:
# Check git history to see what the previous API looked like
git log --oneline -20 -- v3/pkg/services/notifications/Repository: wailsapp/wails
Length of output: 134
🏁 Script executed:
# Check if there are any git diffs showing the change from return values to callbacks
git log -p --all -S "RequestNotificationAuthorization" -- v3/pkg/services/notifications/ | head -100Repository: wailsapp/wails
Length of output: 40
🏁 Script executed:
# Look for any documentation about the notification authorization API
find v3 -type f \( -name "*.md" -o -name "*.go" \) | xargs grep -l "RequestNotificationAuthorization" | grep -E "\.(md|go)$"Repository: wailsapp/wails
Length of output: 317
🏁 Script executed:
# Check the commit that introduced this change
git show 08c6f2c6 --statRepository: wailsapp/wails
Length of output: 50371
🏁 Script executed:
# Look at the merge commit details
git log -1 08c6f2c6Repository: wailsapp/wails
Length of output: 261
🏁 Script executed:
# Check the branch that was merged
git log --oneline 08c6f2c6^..08c6f2c6Repository: wailsapp/wails
Length of output: 259
🏁 Script executed:
# Check if there are any related PRs or issues in git log
git log --all --oneline | grep -i "notification\|callback" | head -20Repository: wailsapp/wails
Length of output: 134
🏁 Script executed:
# Look for the main branch to see what the previous API looked like
git log --all --oneline v3-alpha -- v3/pkg/services/notifications/ | head -20Repository: wailsapp/wails
Length of output: 88
🏁 Script executed:
# Let's check the current state of the notifications interface definition more carefully
cat -n v3/pkg/services/notifications/notifications.go | head -50Repository: wailsapp/wails
Length of output: 2214
🏁 Script executed:
# Check the example app to see how the new API is used
cat -n v3/examples/notifications/main.goRepository: wailsapp/wails
Length of output: 3544
🏁 Script executed:
# Check if there's any documentation about notifications in v3
find v3 -type f -name "*.md" | xargs grep -l "RequestNotificationAuthorization\|notification" 2>/dev/nullRepository: wailsapp/wails
Length of output: 298
🏁 Script executed:
# Look for any migration guidance or breaking changes documentation
find . -type f -name "*.md" | xargs grep -l "breaking.*notification\|notification.*breaking" 2>/dev/nullRepository: wailsapp/wails
Length of output: 42
Mark this changelog entry as a breaking change and clarify the platform scope.
All platforms' RequestNotificationAuthorization() and CheckNotificationAuthorization() methods now use a callback signature func(bool, error) instead of return values. Any code using the synchronous API will break. While the actual authorization functionality is macOS-specific (other platforms have stubs), the API change is global.
Update the changelog entry to explicitly indicate this is a breaking change and explain the migration path. For example:
- **BREAKING:** Refactored notification authorization to callback-based API across all platforms—`RequestNotificationAuthorization()` and `CheckNotificationAuthorization()` now accept a callback instead of returning values
🤖 Prompt for AI Agents
In @v3/UNRELEASED_CHANGELOG.md around lines 22 - 23, Update the changelog entry
to mark this as a breaking change and clarify platform scope: change the line
about "Update notifications authorization methods on macOS to callbacks" to a
**BREAKING** entry that states RequestNotificationAuthorization() and
CheckNotificationAuthorization() now use the callback signature func(bool,
error) across all platforms (noting the implementation is macOS-specific and
other platforms are stubs) and add a brief migration note instructing callers to
replace synchronous return-value usage with the new callback pattern.
|
@leaanthony I think we are good to close this. You can call the sync |
2 participants
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.