[v3] Notifications API #4098
[v3] Notifications API #4098
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a comprehensive notification subsystem for a Wails application. It adds frontend assets (HTML, CSS, and JavaScript) to demonstrate notifications, along with autogenerated bindings that define notification types and service methods. New backend files implement cross-platform notification services for macOS (Darwin) and Windows, and a license file for the Inter font is included. Minor dependency updates and import reordering complement the new functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Service
participant OS
User->>Frontend: Click "Send Complex Notification"
Frontend->>Service: sendComplexNotification()
Service->>OS: Register category & send notification with actions
OS-->>Service: Notification delivered
OS-->>Service: User interacts with notification
Service->>Frontend: Emit notificationResponse event
Frontend-->>User: Update UI with response
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (17)
v3/pkg/services/notifications/notifications_windows.go (4)
1-18: Consider concurrency safety for the globalNotificationCategoriesmap.
Because this is a shared global map, concurrent goroutines that register or remove notification categories might cause data races. You may want to protectNotificationCategorieswith a mutex or switch to a thread-safe data structure.
72-86: Validate Windows AppID usage for push notifications.
Some Windows environments require a valid AppID/package identity to properly display notifications. If the user is running an unpackaged or side-loaded app, the notifications might fail silently. Consider instructing users on how to configure a valid AppID on Windows.Do you want me to open a new issue to explore consistent Windows notifications for unpackaged apps?
179-200: Add fallback handling if the executable name is empty.
IfgetExeName()returns an empty string, the formatted registry path becomes invalid. Provide a default or return an error to avoid registry write failures.- registryPath := fmt.Sprintf(`SOFTWARE\%s\NotificationCategories`, appName) + if appName == "" { + return errors.New("failed to save categories to registry: empty executable name") + } + registryPath := fmt.Sprintf(`SOFTWARE\\%s\\NotificationCategories`, appName)
201-225: Remove debugging print statement.
Line 203 callsprintln(appName), which may add unintended noise to logs. Remove the line or wrap it in a debug flag.- println(appName)v3/pkg/services/notifications/notifications_darwin.go (3)
20-29: Clarify packaging requirement in the function docstring.
AlthoughNew()returns a valid service struct, the notifications require a properly signed bundle. Consider adding a warning or docstring note to guide developers.
31-37: Provide a fallback or enhanced error message if the bundle identifier is missing.
Currently, the code just returns an error, preventing usage in dev mode. You could allow no-op notifications or log additional instructions for developers on packaging/signing.
48-70: Give clearer user feedback for notifications permissions.
IfCheckBundleIdentifier()fails, users might be unaware of how to fix it. Consider returning a more descriptive error or guiding them to properly configure app signing.v3/pkg/services/notifications/notifications_darwin.m (2)
63-69: Confirm correct delegate usage in multi-service scenarios.
The code sets the delegate once globally. If any code re-initializes or replaces this delegate, notifications may not reach the intended handlers.
198-274: Review performance when dealing with large action sets.
Ifactions_jsonis sizable, synchronous parsing and creation ofUNNotificationActionobjects might lead to slow UI responses. Evaluate chunked or asynchronous processing if scale is a concern.v3/pkg/services/notifications/notifications_darwin.h (1)
8-18: Add documentation for function parameters and return values.The function declarations lack documentation describing parameter types, return values, and their purposes. This documentation is crucial for maintainability and understanding the expected behavior of each function.
Add documentation comments above each function. For example:
+/** + * Checks if the application has a valid bundle identifier. + * @return true if bundle identifier is valid, false otherwise. + */ bool checkBundleIdentifier(void); +/** + * Requests authorization for sending notifications. + * @param completion Callback to be invoked after authorization request (void* for ObjC block) + * @return true if request was initiated successfully, false otherwise + */ bool requestUserNotificationAuthorization(void *completion);v3/examples/notifications/frontend/main.js (3)
10-10: Fix typos in notification text.There are spelling errors in the notification text: "Notificaiton" appears twice.
- await Notifications.SendNotification("some-uuid-fronted", "Frontend Notificaiton", "", "Notificaiton sent through JS!"); + await Notifications.SendNotification("some-uuid-frontend", "Frontend Notification", "", "Notification sent through JS!");
33-33: Fix typo in notification message.There's a spelling error in the question: "rainging".
- body: "Is it rainging today where you are?", + body: "Is it raining today where you are?",
29-41: Use UUID generation for notification IDs.Using hardcoded UUIDs could lead to notification conflicts.
+ const uuid = crypto.randomUUID(); await Notifications.SendNotificationWithActions({ - id: "some-uuid-complex", + id: uuid, title: "Complex Frontend Notification",v3/pkg/services/notifications/notifications.go (3)
7-12: Use type definitions instead of type aliases for better extensibility.Type aliases (using
=) limit the ability to add methods or implement interfaces.-type NotificationAction = struct { +type NotificationAction struct { ID string `json:"id"` Title string `json:"title"` Destructive bool `json:"destructive,omitempty"` AuthenticationRequired bool `json:"authenticationRequired,omitempty"` }
33-43: Add documentation for NotificationResponseData type.The type lacks documentation explaining its purpose and field usage.
-// NotificationResponseData +// NotificationResponseData represents the response data structure for a notification interaction. +// It contains information about the notification that was interacted with, including any +// user input or action taken. type NotificationResponseData = struct {
23-31: Add validation methods for required notification fields.The
NotificationOptionsstruct has required fields (ID, Title, Body) but no validation.Add a validation method:
type NotificationOptions struct { ID string `json:"id"` Title string `json:"title"` Subtitle string `json:"subtitle,omitempty"` Body string `json:"body"` CategoryID string `json:"categoryId,omitempty"` Data map[string]interface{} `json:"data,omitempty"` } + +// Validate ensures all required fields are present +func (n NotificationOptions) Validate() error { + if n.ID == "" { + return fmt.Errorf("notification ID is required") + } + if n.Title == "" { + return fmt.Errorf("notification title is required") + } + if n.Body == "" { + return fmt.Errorf("notification body is required") + } + return nil +}v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/service.js (1)
13-20: Consider enhancing error handling for notification operations.While the implementation is clean, the promise-based API could benefit from explicit error handling documentation. For critical operations like permission requests and notification sending, consumers should know what errors to expect.
Add error types to JSDoc comments:
/** * RequestUserNotificationAuthorization requests permission for notifications. * @returns {Promise<boolean> & { cancel(): void }} + * @throws {Error} When permission request fails */Also applies to: 70-77, 79-90, 92-100
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
v3/examples/notifications/frontend/dist/Inter-Medium.ttfis excluded by!**/dist/**,!**/*.ttfv3/examples/notifications/frontend/dist/assets/index-CS0sRFez.jsis excluded by!**/dist/**v3/examples/notifications/frontend/dist/index.htmlis excluded by!**/dist/**v3/examples/notifications/frontend/dist/javascript.svgis excluded by!**/dist/**,!**/*.svgv3/examples/notifications/frontend/dist/style.cssis excluded by!**/dist/**v3/examples/notifications/frontend/dist/wails.pngis excluded by!**/dist/**,!**/*.pngv3/examples/notifications/frontend/public/Inter-Medium.ttfis excluded by!**/*.ttfv3/examples/notifications/frontend/public/javascript.svgis excluded by!**/*.svgv3/examples/notifications/frontend/public/wails.pngis excluded by!**/*.pngv3/go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
v3/examples/notifications/frontend/Inter Font License.txt(1 hunks)v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/index.js(1 hunks)v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/models.js(1 hunks)v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/service.js(1 hunks)v3/examples/notifications/frontend/index.html(1 hunks)v3/examples/notifications/frontend/main.js(1 hunks)v3/examples/notifications/frontend/package.json(1 hunks)v3/examples/notifications/frontend/public/style.css(1 hunks)v3/examples/notifications/main.go(1 hunks)v3/examples/services/main.go(1 hunks)v3/go.mod(1 hunks)v3/internal/runtime/desktop/@wailsio/runtime/package.json(1 hunks)v3/pkg/application/context_application_event.go(2 hunks)v3/pkg/services/notifications/notifications.go(1 hunks)v3/pkg/services/notifications/notifications_darwin.go(1 hunks)v3/pkg/services/notifications/notifications_darwin.h(1 hunks)v3/pkg/services/notifications/notifications_darwin.m(1 hunks)v3/pkg/services/notifications/notifications_windows.go(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- v3/examples/services/main.go
- v3/internal/runtime/desktop/@wailsio/runtime/package.json
- v3/examples/notifications/frontend/index.html
- v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/index.js
- v3/examples/notifications/frontend/package.json
- v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/models.js
- v3/examples/notifications/frontend/public/style.css
🧰 Additional context used
🪛 LanguageTool
v3/examples/notifications/frontend/Inter Font License.txt
[typographical] ~9-~9: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...------ SIL OPEN FONT LICENSE Version 1.1 - 26 February 2007 -----------------------...
(DASH_RULE)
🔇 Additional comments (6)
v3/pkg/services/notifications/notifications_windows.go (1)
29-47: Verify potential concurrency issues for the user interaction callback.
The callback set bytoast.SetActivationCallback(...)is global. If multiple notifications are sent concurrently, it’s worth ensuring that the callback can handle parallel invocations without data races or shared global state conflicts.v3/pkg/services/notifications/notifications_darwin.go (1)
195-208: Add logging or handling for malformed notification responses.
Here, ifjson.Unmarshalfails, the code returns silently. Log an error or provide a fallback if the JSON payload is invalid.v3/pkg/services/notifications/notifications_darwin.m (1)
25-47: Add extra validation when building the response dictionary.
If critical keys (likeidentifieroractionIdentifier) are missing, it could disrupt the flow silently. Consider logging or handling unexpected structures inpayload.v3/pkg/services/notifications/notifications_darwin.h (1)
11-12: Consider adding parameter validation for notification content.The
sendNotificationfunction accepts raw string pointers without validation. This could lead to crashes if null pointers are passed.Consider adding validation functions or documenting the requirement for non-null parameters. For example:
+/** + * Sends a notification with the specified content. + * @param identifier Must not be NULL. Unique identifier for the notification. + * @param title Must not be NULL. Title of the notification. + * @param subtitle Can be NULL. Subtitle of the notification. + * @param body Must not be NULL. Body text of the notification. + * @param completion Callback to be invoked after sending (void* for ObjC block) + */ void sendNotification(const char *identifier, const char *title, const char *subtitle, const char *body, void *completion);v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/service.js (1)
1-11: LGTM! Well-structured TypeScript configuration and imports.The file is properly configured with TypeScript checking and includes necessary imports. The auto-generation notice helps prevent manual modifications.
v3/examples/notifications/frontend/Inter Font License.txt (1)
1-93: LGTM! Proper font licensing included.The Inter font license is correctly included with all required terms and conditions under the SIL Open Font License.
🧰 Tools
🪛 LanguageTool
[typographical] ~9-~9: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...------ SIL OPEN FONT LICENSE Version 1.1 - 26 February 2007 -----------------------...(DASH_RULE)
| void sendNotificationWithActions(const char *identifier, const char *title, const char *subtitle, | ||
| const char *body, const char *categoryId, const char *actions_json, void *completion) { | ||
| ensureDelegateInitialized(); | ||
|
|
||
| NSString *nsIdentifier = [NSString stringWithUTF8String:identifier]; | ||
| NSString *nsTitle = [NSString stringWithUTF8String:title]; | ||
| NSString *nsSubtitle = subtitle ? [NSString stringWithUTF8String:subtitle] : @""; | ||
| NSString *nsBody = [NSString stringWithUTF8String:body]; | ||
| NSString *nsCategoryId = [NSString stringWithUTF8String:categoryId]; | ||
|
|
||
| NSMutableDictionary *customData = [NSMutableDictionary dictionary]; | ||
| if (actions_json) { | ||
| NSString *actionsJsonStr = [NSString stringWithUTF8String:actions_json]; | ||
| NSData *jsonData = [actionsJsonStr dataUsingEncoding:NSUTF8StringEncoding]; | ||
| NSError *error = nil; | ||
| NSDictionary *parsedData = [NSJSONSerialization JSONObjectWithData:jsonData options:0 error:&error]; | ||
| if (!error && parsedData) { | ||
| [customData addEntriesFromDictionary:parsedData]; | ||
| } | ||
| } | ||
|
|
||
| UNUserNotificationCenter *center = [UNUserNotificationCenter currentNotificationCenter]; | ||
|
|
||
| UNMutableNotificationContent *content = [[UNMutableNotificationContent alloc] init]; | ||
| content.title = nsTitle; | ||
| if (![nsSubtitle isEqualToString:@""]) { | ||
| content.subtitle = nsSubtitle; | ||
| } | ||
| content.body = nsBody; | ||
| content.sound = [UNNotificationSound defaultSound]; | ||
| content.categoryIdentifier = nsCategoryId; | ||
|
|
||
| // Add custom data if available | ||
| if (customData.count > 0) { | ||
| content.userInfo = customData; | ||
| } | ||
|
|
||
| UNTimeIntervalNotificationTrigger *trigger = [UNTimeIntervalNotificationTrigger triggerWithTimeInterval:1 repeats:NO]; | ||
|
|
||
| UNNotificationRequest *request = [UNNotificationRequest requestWithIdentifier:nsIdentifier content:content trigger:trigger]; | ||
|
|
||
| [center addNotificationRequest:request withCompletionHandler:^(NSError * _Nullable error) { | ||
| if (completion != NULL) { | ||
| void (^callback)(NSError *) = completion; | ||
| callback(error); | ||
| } | ||
| }]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Surface errors from sending notifications with actions.
The completion handler may receive a non-nil NSError. Consider logging the error or returning it to the caller, ensuring failures are not silently overlooked.
| const timeElement = document.getElementById('time'); | ||
| const notificationsElement = document.getElementById('notifications'); |
There was a problem hiding this comment.
Add error handling for DOM element access.
Direct getElementById calls could return null if elements don't exist.
-const timeElement = document.getElementById('time');
-const notificationsElement = document.getElementById('notifications');
+const timeElement = document.getElementById('time');
+const notificationsElement = document.getElementById('notifications');
+
+if (!timeElement || !notificationsElement) {
+ console.error('Required DOM elements not found');
+ throw new Error('Required DOM elements not found');
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const timeElement = document.getElementById('time'); | |
| const notificationsElement = document.getElementById('notifications'); | |
| const timeElement = document.getElementById('time'); | |
| const notificationsElement = document.getElementById('notifications'); | |
| if (!timeElement || !notificationsElement) { | |
| console.error('Required DOM elements not found'); | |
| throw new Error('Required DOM elements not found'); | |
| } |
v3/examples/notifications/main.go
Outdated
| go func() { | ||
| time.Sleep(time.Second * 5) | ||
| // Sometime later check if you are still authorized | ||
| granted, err := NotificationService.CheckNotificationAuthorization() | ||
| if err != nil { | ||
| log.Default().Printf("WARNING: %s\n", err) | ||
| } | ||
| println(granted) | ||
|
|
||
| // Sometime later remove delivered notification by identifier | ||
| err = NotificationService.RemoveDeliveredNotification("some-uuid") | ||
| if err != nil { | ||
| log.Default().Printf("WARNING: %s\n", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add graceful shutdown for background goroutines.
The goroutines might be interrupted during application shutdown. Consider implementing a cancellation mechanism.
+ ctx, cancel := context.WithCancel(context.Background())
+ defer cancel()
go func() {
+ select {
+ case <-ctx.Done():
+ return
+ case <-time.After(time.Second * 5):
- time.Sleep(time.Second * 5)
// Rest of the code...
+ }
}()Also applies to: 115-126
v3/examples/notifications/main.go
Outdated
| app.OnApplicationEvent(events.Mac.ApplicationDidFinishLaunching, func(event *application.ApplicationEvent) { | ||
| // Request pemission to send notifications | ||
| granted, err := NotificationService.RequestUserNotificationAuthorization() | ||
| if err != nil { | ||
| log.Default().Printf("WARNING: %s\n", err) | ||
| } | ||
|
|
||
| if granted { | ||
| // Send notification with no actions | ||
| err = NotificationService.SendNotification("some-uuid", "Title", "", "body!") | ||
| if err != nil { | ||
| log.Default().Printf("WARNING: %s\n", err) | ||
| } | ||
|
|
||
| err = NotificationService.RegisterNotificationCategory(notifications.NotificationCategory{ | ||
| ID: "MESSAGE_CATEGORY", | ||
| Actions: []notifications.NotificationAction{ | ||
| {ID: "VIEW_ACTION", Title: "View"}, | ||
| {ID: "MARK_READ_ACTION", Title: "Mark as Read"}, | ||
| {ID: "DELETE_ACTION", Title: "Delete", Destructive: true}, | ||
| }, | ||
| HasReplyField: true, | ||
| ReplyPlaceholder: "Type your reply...", | ||
| ReplyButtonTitle: "Reply", | ||
| }) | ||
| if err != nil { | ||
| log.Default().Printf("WARNING: %s\n", err) | ||
| } | ||
|
|
||
| err = NotificationService.SendNotificationWithActions(notifications.NotificationOptions{ | ||
| ID: "some-other-uuid", | ||
| Title: "New Message", | ||
| Subtitle: "From: Jane Doe", | ||
| Body: "Is it raining today where you are?", | ||
| CategoryID: "MESSAGE_CATEGORY", | ||
| Data: map[string]interface{}{ | ||
| "messageId": "msg-123", | ||
| "senderId": "user-123", | ||
| "timestamp": time.Now().Unix(), | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| log.Default().Printf("WARNING: %s\n", err) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and user feedback.
The error handling only logs warnings without informing the user. For a better user experience, consider propagating these errors to the UI.
if err != nil {
- log.Default().Printf("WARNING: %s\n", err)
+ log.Default().Printf("WARNING: %s\n", err)
+ // Notify user through UI
+ app.NewErrorNotification("Notification Error", fmt.Sprintf("Failed to send notification: %v", err))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| app.OnApplicationEvent(events.Mac.ApplicationDidFinishLaunching, func(event *application.ApplicationEvent) { | |
| // Request pemission to send notifications | |
| granted, err := NotificationService.RequestUserNotificationAuthorization() | |
| if err != nil { | |
| log.Default().Printf("WARNING: %s\n", err) | |
| } | |
| if granted { | |
| // Send notification with no actions | |
| err = NotificationService.SendNotification("some-uuid", "Title", "", "body!") | |
| if err != nil { | |
| log.Default().Printf("WARNING: %s\n", err) | |
| } | |
| err = NotificationService.RegisterNotificationCategory(notifications.NotificationCategory{ | |
| ID: "MESSAGE_CATEGORY", | |
| Actions: []notifications.NotificationAction{ | |
| {ID: "VIEW_ACTION", Title: "View"}, | |
| {ID: "MARK_READ_ACTION", Title: "Mark as Read"}, | |
| {ID: "DELETE_ACTION", Title: "Delete", Destructive: true}, | |
| }, | |
| HasReplyField: true, | |
| ReplyPlaceholder: "Type your reply...", | |
| ReplyButtonTitle: "Reply", | |
| }) | |
| if err != nil { | |
| log.Default().Printf("WARNING: %s\n", err) | |
| } | |
| err = NotificationService.SendNotificationWithActions(notifications.NotificationOptions{ | |
| ID: "some-other-uuid", | |
| Title: "New Message", | |
| Subtitle: "From: Jane Doe", | |
| Body: "Is it raining today where you are?", | |
| CategoryID: "MESSAGE_CATEGORY", | |
| Data: map[string]interface{}{ | |
| "messageId": "msg-123", | |
| "senderId": "user-123", | |
| "timestamp": time.Now().Unix(), | |
| }, | |
| }) | |
| if err != nil { | |
| log.Default().Printf("WARNING: %s\n", err) | |
| } | |
| } | |
| }) | |
| app.OnApplicationEvent(events.Mac.ApplicationDidFinishLaunching, func(event *application.ApplicationEvent) { | |
| // Request pemission to send notifications | |
| granted, err := NotificationService.RequestUserNotificationAuthorization() | |
| if err != nil { | |
| log.Default().Printf("WARNING: %s\n", err) | |
| } | |
| if granted { | |
| // Send notification with no actions | |
| err = NotificationService.SendNotification("some-uuid", "Title", "", "body!") | |
| if err != nil { | |
| log.Default().Printf("WARNING: %s\n", err) | |
| // Notify user through UI | |
| app.NewErrorNotification("Notification Error", fmt.Sprintf("Failed to send notification: %v", err)) | |
| } | |
| err = NotificationService.RegisterNotificationCategory(notifications.NotificationCategory{ | |
| ID: "MESSAGE_CATEGORY", | |
| Actions: []notifications.NotificationAction{ | |
| {ID: "VIEW_ACTION", Title: "View"}, | |
| {ID: "MARK_READ_ACTION", Title: "Mark as Read"}, | |
| {ID: "DELETE_ACTION", Title: "Delete", Destructive: true}, | |
| }, | |
| HasReplyField: true, | |
| ReplyPlaceholder: "Type your reply...", | |
| ReplyButtonTitle: "Reply", | |
| }) | |
| if err != nil { | |
| log.Default().Printf("WARNING: %s\n", err) | |
| } | |
| err = NotificationService.SendNotificationWithActions(notifications.NotificationOptions{ | |
| ID: "some-other-uuid", | |
| Title: "New Message", | |
| Subtitle: "From: Jane Doe", | |
| Body: "Is it raining today where you are?", | |
| CategoryID: "MESSAGE_CATEGORY", | |
| Data: map[string]interface{}{ | |
| "messageId": "msg-123", | |
| "senderId": "user-123", | |
| "timestamp": time.Now().Unix(), | |
| }, | |
| }) | |
| if err != nil { | |
| log.Default().Printf("WARNING: %s\n", err) | |
| } | |
| } | |
| }) |
| @@ -3,6 +3,7 @@ module github.com/wailsapp/wails/v3 | |||
| go 1.24.0 | |||
There was a problem hiding this comment.
Fix Go version specification.
The specified Go version 1.24.0 does not exist. The latest stable version is Go 1.22.
-go 1.24.0
+go 1.22📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go 1.24.0 | |
| go 1.22 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
v3/pkg/services/notifications/notifications_windows.go (3)
18-18: Consider concurrency-safe access to the globalNotificationCategoriesmap.
Multiple methods in this file read from and write to the globalNotificationCategoriesmap, potentially from different goroutines. This could lead to data races or inconsistent state.Consider adding a synchronization mechanism (e.g. a
sync.RWMutex) around map operations or storing categories within theServicestruct to ensure controlled access.Also applies to: 88-125, 127-138, 140-144, 180-203, 205-231
29-47: Consider logging or handling potential issues when emitting events.
Althoughapplication.Get().EmitEvent(...)does not return an error, you might add logs or checks to confirm successful event delivery.
72-86: Unused parameter inSendNotification.
The underscore parameter for subtitle is never used. Consider removing it or naming it_subtitlefor clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v3/examples/notifications/frontend/main.js(1 hunks)v3/pkg/services/notifications/notifications_windows.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v3/examples/notifications/frontend/main.js
🔇 Additional comments (14)
v3/pkg/services/notifications/notifications_windows.go (14)
1-4: No issues with Windows build tag and package statement.
5-16: Imports look correct.
20-22: No issues with theNew()function.
24-27:ServiceNameis well-defined.
49-52: Clean shutdown logic.
The approach of saving categories on shutdown is straightforward.
54-70: Appropriate Windows stubs.
These methods properly return defaults for macOS-only features.
88-125: Graceful fallback to a basic notification.
When the category ID is not registered, sending a simple notification is a nice default that avoids errors.
127-138: Category registration logic looks good.
Updates the in-memory map and saves to the registry without issues.
140-144: Smooth category removal.
The code correctly deletes from the map and persists changes to the registry.
146-168: Windows stubs for macOS-only notification removal.
These no-op methods clearly communicate platform differences.
170-178: Helper for retrieving executable name.
Implementation is concise and handles errors gracefully.
180-203: Registry write logic appears correct.
The code handles key creation, JSON marshaling, and error propagation appropriately.
205-231: Registry read logic is well-structured.
The code properly checks for missing keys, unmarshals the JSON data, and handles errors.
233-240: User input extraction is straightforward.
Matches data fields against"userText"as expected.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
v3/pkg/services/notifications/notifications_darwin.m (3)
117-117: Consider increasing the timeout for slow systems.The 3-second timeout might be insufficient on slower systems or under heavy load.
- dispatch_semaphore_wait(semaphore, dispatch_time(DISPATCH_TIME_NOW, 3 * NSEC_PER_SEC)); + dispatch_semaphore_wait(semaphore, dispatch_time(DISPATCH_TIME_NOW, 5 * NSEC_PER_SEC));
165-212: Reduce code duplication with sendNotification.This function shares significant code with
sendNotification. Consider extracting common logic into a helper function.Example refactor:
+static UNNotificationRequest* createNotificationRequest(NSString *identifier, NSString *title, + NSString *subtitle, NSString *body, NSDictionary *customData, NSString *categoryId) { + UNMutableNotificationContent *content = [[UNMutableNotificationContent alloc] init]; + content.title = title; + if (subtitle && ![subtitle isEqualToString:@""]) { + content.subtitle = subtitle; + } + content.body = body; + content.sound = [UNNotificationSound defaultSound]; + if (categoryId) { + content.categoryIdentifier = categoryId; + } + if (customData.count > 0) { + content.userInfo = customData; + } + + UNTimeIntervalNotificationTrigger *trigger = + [UNTimeIntervalNotificationTrigger triggerWithTimeInterval:1 repeats:NO]; + + return [UNNotificationRequest requestWithIdentifier:identifier + content:content + trigger:trigger]; +}
288-289: Add error handling for category updates.The
setNotificationCategoriescall could fail but errors are not captured.- [center setNotificationCategories:updatedCategories]; + [center setNotificationCategories:updatedCategories withCompletionHandler:^(NSError * _Nullable error) { + if (error) { + NSLog(@"Error updating notification categories: %@", error); + } + }];v3/pkg/services/notifications/notifications_darwin.go (5)
31-37: Consider logging additional details for missing bundle identifier.When
CheckBundleIdentifierfails, the service returns a generic error. Providing additional context to the user (e.g., which bundle identifier is missing or invalid) may help in troubleshooting packaging or signing issues.
48-55: Clarify behavior when JSON data marshalling fails.If
json.Marshal(options.Data)fails, the code silently skips attaching data to the notification. Consider returning an error or logging the issue to help diagnose malformed payloads.
65-90: Validate JSON marshalling consistency.The function checks whether
err == nilduring JSON marshalling before assigningcDataJSON. This logic silently discards invalid notification data, which might be unexpected for callers. Consistency with other methods likeRegisterNotificationCategory(where errors are returned) would help unify error handling.
92-121: Avoid partial fallback inSendNotificationWithActions.If the category is not registered, the function gracefully degrades to a basic notification. This is sensible, but consider logging an informational message when the category is missing. It would help detect invalid usage in cases where the category was supposed to be registered beforehand.
204-210: Handle JSON unmarshal errors with logging or telemetry.Currently, if JSON unmarshalling fails, the function returns immediately. Consider logging or reporting the error to simplify troubleshooting incorrect notification response payloads.
v3/pkg/services/notifications/notifications_darwin.h (1)
12-18: Assess error handling for notification management functions.Functions such as
sendNotificationWithActions,registerNotificationCategory, orremoveNotificationCategorydo not return errors at the C level. If any internal OS-level failures occur, they will be ignored. Explore returning status codes or using Objective-C exceptions to provide more informative feedback.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v3/pkg/services/notifications/notifications_darwin.go(1 hunks)v3/pkg/services/notifications/notifications_darwin.h(1 hunks)v3/pkg/services/notifications/notifications_darwin.m(1 hunks)
🔇 Additional comments (5)
v3/pkg/services/notifications/notifications_darwin.m (3)
71-83: Well-implemented bundle identifier check!The function provides clear error messaging and actionable steps for developers to resolve bundling issues.
157-162: Surface errors from sending notifications.The completion handler may receive a non-nil
NSError. Consider logging the error or returning it to the caller, ensuring failures are not silently overlooked.
315-335: Clean implementation of notification removal functions!The functions are well-organized, focused, and follow consistent patterns.
v3/pkg/services/notifications/notifications_darwin.go (1)
5-9: Confirm macOS version support.You are compiling with
-mmacosx-version-min=10.14in both CFLAGS and LDFLAGS, which ensures compatibility with macOS 10.14 and later. If you intend to support older macOS versions, you may need to adjust this setting or provide appropriate fallbacks.Would you like me to generate a script to search your documentation or codebase for references to older macOS versions?
v3/pkg/services/notifications/notifications_darwin.h (1)
8-11: Verify boolean return type usage.Functions like
checkBundleIdentifierandrequestUserNotificationAuthorizationreturn abool. If you need more detailed error reporting, consider using an extended structure or separate error signaling.
| NSError *error = nil; | ||
| NSData *jsonData = [NSJSONSerialization dataWithJSONObject:payload options:0 error:&error]; | ||
| if (!error) { | ||
| NSString *jsonString = [[NSString alloc] initWithData:jsonData encoding:NSUTF8StringEncoding]; | ||
| didReceiveNotificationResponse([jsonString UTF8String]); | ||
| } |
There was a problem hiding this comment.
Improve error handling in JSON serialization.
The error from JSON serialization is silently ignored, which could lead to missing notification responses.
Apply this diff to handle the error:
NSError *error = nil;
NSData *jsonData = [NSJSONSerialization dataWithJSONObject:payload options:0 error:&error];
if (!error) {
NSString *jsonString = [[NSString alloc] initWithData:jsonData encoding:NSUTF8StringEncoding];
didReceiveNotificationResponse([jsonString UTF8String]);
+ } else {
+ NSLog(@"Error serializing notification response: %@", error);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| NSError *error = nil; | |
| NSData *jsonData = [NSJSONSerialization dataWithJSONObject:payload options:0 error:&error]; | |
| if (!error) { | |
| NSString *jsonString = [[NSString alloc] initWithData:jsonData encoding:NSUTF8StringEncoding]; | |
| didReceiveNotificationResponse([jsonString UTF8String]); | |
| } | |
| NSError *error = nil; | |
| NSData *jsonData = [NSJSONSerialization dataWithJSONObject:payload options:0 error:&error]; | |
| if (!error) { | |
| NSString *jsonString = [[NSString alloc] initWithData:jsonData encoding:NSUTF8StringEncoding]; | |
| didReceiveNotificationResponse([jsonString UTF8String]); | |
| } else { | |
| NSLog(@"Error serializing notification response: %@", error); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
v3/pkg/services/notifications/notifications_darwin.go (5)
22-24: Consider caching the bundle identifier check.
Repeatedly callingCheckBundleIdentifier()in every function can be avoided if you store and verify the result upon initialization (if the bundle ID won’t change at runtime). This might enhance performance and maintainability.func New() *Service { - return &Service{} + return &Service{ + isBundleIdentifierValid: CheckBundleIdentifier(), + } }
26-29: Extract repeated service name string into a constant.
Defining a constant for"github.com/wailsapp/wails/v3/services/notifications"can help avoid typos and make updates easier.
31-37: Consider removing unused parameters or documenting usage.
ctxandoptionsappear unused. If they're required by the interface, consider documenting their purpose or future use.
65-90: Handle JSON marshalling errors to avoid silent failures.
Currently, ifjson.Marshal(options.Data)fails, the error is ignored, and the notification proceeds without the data payload. Logging or returning an error would help diagnose payload issues.jsonData, err := json.Marshal(options.Data) if err == nil { cDataJSON = C.CString(string(jsonData)) defer C.free(unsafe.Pointer(cDataJSON)) } else { + fmt.Printf("Warning: Failed to marshal notification data: %v\n", err) }
204-217: Consider logging JSON unmarshal errors.
In case the payload cannot be deserialized, it might be beneficial to log or handle the error instead of silently returning.if err := json.Unmarshal([]byte(payload), &response); err != nil { - return + fmt.Printf("Warning: Failed to unmarshal notification response: %v\n", err) + return }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/pkg/services/notifications/notifications_darwin.go(1 hunks)
🔇 Additional comments (13)
v3/pkg/services/notifications/notifications_darwin.go (13)
1-9: Confirm the minimum macOS requirement.
Currently, the build tags and cgo flags specify a minimum macOS version of 11.0. If you need to support older macOS versions, please confirm that this requirement aligns with your user base.
10-18: Imports look good.
No issues found.
39-42: Shutdown logic is acceptable.
No issues found with this placeholder implementation.
44-46: Bundle identifier discussion already covered.
This function is part of the earlier suggestion to cache or centralize the result.
48-55: Authorization request logic looks correct.
The check for a valid bundle ID and straightforward return of the authorization result are well-implemented.
57-63: Notification authorization check is solid.
No further concerns; the return signature and validation are appropriate.
92-121: Same suggestion for ignoring JSON marshalling errors.
Please refer to the previous comment about handling errors when marshaling theDatafield.
123-150: Category registration logic is well-structured.
The function handles JSON marshalling errors, which is commendable.
152-162: Removal of a notification category looks good.
No issues found in this straightforward flow.
164-171: No issues for removing all pending notifications.
Implementation details appear consistent.
173-182: Pending notification removal logic is solid.
Validates bundle ID and then removes if valid.
184-191: All delivered notifications removal appears correct.
The process matches functionality in the other removal methods.
193-202: Delivered notification removal is properly handled.
No concerns with these lines.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
v3/pkg/services/notifications/notifications_windows.go (4)
29-47: Minor caution about event emission error handling.In
ServiceStartup, whentoast.SetActivationCallbackis triggered,application.Get().EmitEventis used to broadcast notification responses. Although this generally works well, consider at least logging or handling any errors returned by the event emission mechanism, especially if the framework or downstream consumers can fail under heavy load or invalid event data.
49-52: Graceful handling of registry-save failures.If
saveCategoriesToRegistry()fails, the error bubbles up, but there is currently no logging or fallback. Consider logging the error to help diagnose potential registry permission or system-level issues upon shutdown.
180-203: Persisting categories to registry works well but may be costly in bulk operations.The
saveCategoriesToRegistry()approach reliably syncs changes to the registry each time a category is registered or removed. However, if an application registers/removes many categories in quick succession, repeated calls could be a performance hit. You may want to batch updates or provide a manual "commit" step to minimize overhead.
205-231: Resilience against malformed registry data.
loadCategoriesFromRegistry()gracefully handles cases where the key doesn't exist. However, if there's a partial write or corrupted JSON string, the JSON unmarshal call might return an error. Though you already return it, consider logging it or falling back to an empty map to keep notifications functional in case the stored data is invalid.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v3/pkg/services/notifications/notifications.go(1 hunks)v3/pkg/services/notifications/notifications_windows.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v3/pkg/services/notifications/notifications.go
🔇 Additional comments (4)
v3/pkg/services/notifications/notifications_windows.go (4)
1-16: Good overall structure for a platform-specific implementation.The use of
//go:build windowsensures that this file is only compiled on Windows. The initial import statements and package declaration are straightforward and clearly separate out the Windows-specific logic for notifications.
72-86: Notification logic appears correct.The straightforward usage of “go-toast” with
n.Push()here is clear and properly handles both success and error states. This ensures that the basic notification flow is stable on Windows.
88-91: Confirm fallback behavior whenCategoryIDis unregistered.If
options.CategoryIDhas not been registered, this method still proceeds by assigning an empty category, resulting in a basic notification. You might consider logging a warning or error if the developer is expecting actions but calls this without registering a category, as a friendly diagnostic.
233-240: Clear logic for extracting user-supplied text.The utility function
getUserText()is concise and easy to follow, returning early when the required key is found, and defaulting to an empty string if not present. This is a clean approach for parsing user data in Windows toast notifications.
| // RegisterNotificationCategory registers a new NotificationCategory to be used with SendNotificationWithActions. | ||
| func (ns *Service) RegisterNotificationCategory(category NotificationCategory) error { | ||
| NotificationCategories[category.ID] = NotificationCategory{ | ||
| ID: category.ID, | ||
| Actions: category.Actions, | ||
| HasReplyField: bool(category.HasReplyField), | ||
| ReplyPlaceholder: category.ReplyPlaceholder, | ||
| ReplyButtonTitle: category.ReplyButtonTitle, | ||
| } | ||
|
|
||
| return saveCategoriesToRegistry() | ||
| } | ||
|
|
||
| // RemoveNotificationCategory removes a previously registered NotificationCategory. | ||
| func (ns *Service) RemoveNotificationCategory(categoryId string) error { | ||
| delete(NotificationCategories, categoryId) | ||
| return saveCategoriesToRegistry() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent potential data race on category registration/removal.
Calls to RegisterNotificationCategory (lines 127-138) and RemoveNotificationCategory (lines 140-144) modify NotificationCategories and then immediately persist to the registry. In a multi-threaded scenario, these updates could overlap. Consider coordinating these writes with a mutex and ensuring the map is only modified while locked.
| "golang.org/x/sys/windows/registry" | ||
| ) | ||
|
|
||
| var NotificationCategories map[string]NotificationCategory = make(map[string]NotificationCategory) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider encapsulating this global map to avoid concurrency pitfalls.
Defining NotificationCategories as a global variable may lead to data races if multiple goroutines attempt to register or remove categories concurrently. A more robust approach is to store it within the Service struct and protect access with a mutex, ensuring thread-safety if the application or service is accessed concurrently.
-var NotificationCategories map[string]NotificationCategory = make(map[string]NotificationCategory)
+type Service struct {
+ mu sync.Mutex
+ NotificationCategories map[string]NotificationCategory
+}
func New() *Service {
- return &Service{}
+ return &Service{
+ NotificationCategories: make(map[string]NotificationCategory),
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var NotificationCategories map[string]NotificationCategory = make(map[string]NotificationCategory) | |
| package notifications | |
| import "sync" | |
| type NotificationCategory struct { | |
| // existing fields... | |
| } | |
| type Service struct { | |
| mu sync.Mutex | |
| NotificationCategories map[string]NotificationCategory | |
| } | |
| func New() *Service { | |
| return &Service{ | |
| NotificationCategories: make(map[string]NotificationCategory), | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
v3/pkg/services/notifications/notifications_windows.go (6)
25-27: Consider passing a map reference to theServicestruct.
WhileNew()returns a pointer to an emptyService, it may be more robust to storeNotificationCategorieswithin the struct instead of keeping it global. This approach would simplify thread-safety and reduce global state.
34-61: Validate callback data and manage unsupported fields more explicitly.
WithinServiceStartup, you parse notification data (lines 47-53) and handle user text (lines 54-56). If future Windows toast mechanisms or user-provided data structures change, consider more defensive error handling or logging for unexpected data, ensuring resilience.
86-111: Ensure user expectations match ignored fields.
On Windows, certain fields (subtitle, category ID, etc.) are ignored. If the caller relies on them, highlight in the documentation or logs that these options have no effect on this platform to reduce confusion.
113-162: Fallback to basic notification if category isn’t found—add a warning.
Currently, ifoptions.CategoryIDisn’t recognized, the code silently reverts to a basic notification. This might surprise users expecting specific actions or a reply field. Consider logging a warning or returning an error to indicate that a category was missing.
250-276: Add logging on registry load.
loadCategoriesFromRegistryproperly checks forErrNotExistbut might benefit from logging when categories are loaded or from providing more context upon partial failures. This can simplify debugging user issues with missing or corrupted registry entries.
278-285: Use short-circuit logic for readability.
WithingetUserText, you can directly return once a matching key is found. This is already concise, but something like:-func getUserText(data []toast.UserData) (string, bool) { - for _, d := range data { - if d.Key == "userText" { - return d.Value, true - } - } - return "", false +func getUserText(data []toast.UserData) (text string, found bool) { + for _, d := range data { + if d.Key == "userText" { + return d.Value, true + } + } + return "", false }improves clarity by naming returns while preserving existing behavior.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/pkg/services/notifications/notifications_windows.go(1 hunks)
🔇 Additional comments (5)
v3/pkg/services/notifications/notifications_windows.go (5)
1-4: Looks good!
The build constraint and package declaration are clear and correct.
18-18: Global map may cause data races if accessed concurrently.
This globalNotificationCategoriesmap can be a source of concurrency issues in multi-threaded scenarios. Even though it might not currently be accessed concurrently, consider wrapping it with a mutex or storing it as part of theServicestruct to avoid future data races.
68-78: No-op stubs for macOS-specific checks look fine.
These stub functions correctly return defaults, as there is no equivalent Windows concept for bundle identifiers or authorization.
164-181: Concurrent category registration and removal can cause data races.
Similar to the global map definition, these functions modify shared state. Use a mutex aroundNotificationCategoriesto guard reads and writes if the application may register/remove categories from multiple goroutines.
225-248: Verify registry write success.
saveCategoriesToRegistryhandles opening, creating, and writing to the registry. While the error handling here is good, consider logging successes/failures—especially if categories fail to write. This helps diagnose deployment or permission issues on Windows systems.
|
I was getting frustrated that no arbitrary data could be set on Windows Toasts. It's hacky but I was thinking the only thing we get back from interacting with the toast is the activation arguments on the toast itself or action button pressed. So what I am thinking is, if data is passed in with the toast, stringify and append it to the activation argument string with a separator for every defined action. When we receive the notification response we separate it back out before emitting it. See: cc760a1 This allows us to mirror macOS behavior and gets us metadata from the toast. Otherwise getting back the text input and action button name is kinda useless when we don't know anything else about the toast. I am not confident I want to keep this yet but would like some feedback. The example is up-to-date with these features to try. Should work on macOS and Windows. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/service.js (4)
53-62: Improve parameter naming for clarity.The parameter
$0is not descriptive. Consider using a more meaningful name likeidentifierto match the documentation.-export function RemoveDeliveredNotification($0) { - let $resultPromise = /** @type {any} */($Call.ByID(149440045, $0)); +export function RemoveDeliveredNotification(identifier) { + let $resultPromise = /** @type {any} */($Call.ByID(149440045, identifier));
74-83: Improve parameter naming for consistency.Similarly, the parameter
$0should be renamed toidentifierfor consistency with other functions.-export function RemovePendingNotification($0) { - let $resultPromise = /** @type {any} */($Call.ByID(3872412470, $0)); +export function RemovePendingNotification(identifier) { + let $resultPromise = /** @type {any} */($Call.ByID(3872412470, identifier));
13-93: Enhance documentation for platform-specific behavior.The JSDoc comments for Windows stubs could be more informative about the platform differences. Consider adding:
- Why the function is not needed on Windows
- What developers should do instead on Windows
- Minimum OS version requirements for macOS (11.0 as mentioned in PR objectives)
95-117: Document Windows metadata handling limitations and workarounds.Based on the PR objectives and comments, Windows Toast notifications have limitations with metadata handling. The documentation should be updated to include:
- The Windows metadata handling workaround using stringified data in activation arguments
- How to retrieve metadata when notifications are interacted with on Windows
- Platform-specific differences in notification interaction responses
Example documentation addition for
SendNotificationWithActions:/** * SendNotificationWithActions sends a notification with additional actions and inputs. * A NotificationCategory must be registered with RegisterNotificationCategory first. The `CategoryID` must match the registered category. * If a NotificationCategory is not registered a basic notification will be sent. * * Platform-specific notes: * - Windows: Metadata is handled by stringifying data into activation arguments * - Windows: Interaction responses provide limited context compared to macOS * - macOS: Full support for subtitle, category id, and metadata * * @param {$models.NotificationOptions} options * @returns {Promise<void> & { cancel(): void }} */v3/examples/notifications/main.go (1)
78-81: Consider abstracting platform-specific UUID handling.The UUID handling differs between platforms. Consider extracting this logic into a helper function for better maintainability.
+func getNotificationID(baseID string) string { + if application.Get().Environment().OS == "darwin" { + return baseID + } + return "Wails Notification Demo" +} // Usage -var uuid1 string = "Wails Notification Demo" -if application.Get().Environment().OS == "darwin" { - uuid1 = "uuid1" -} +var uuid1 = getNotificationID("uuid1")Also applies to: 96-99
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/models.js(1 hunks)v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/service.js(1 hunks)v3/examples/notifications/frontend/main.js(1 hunks)v3/examples/notifications/main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v3/examples/notifications/frontend/main.js
- v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/models.js
🔇 Additional comments (5)
v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/service.js (1)
1-12: LGTM! Well-structured file setup with proper type checking.The file header and imports are well-organized with appropriate TypeScript configuration and clear documentation about the auto-generated nature of the file.
v3/examples/notifications/main.go (4)
3-22: LGTM! Well-structured imports and properly documented embedded assets.The code follows best practices for embedding frontend assets and organizing imports.
68-126: Add graceful shutdown for background goroutines.The goroutine might be interrupted during application shutdown.
128-135: LGTM! Proper error handling for application run.The code correctly handles potential errors during application execution.
33-45:❓ Verification inconclusive
Verify bundle identifier for macOS notifications.
According to the PR objectives, macOS requires a valid bundle identifier for notifications to function. Consider adding validation or documentation for this requirement.
🏁 Script executed:
#!/bin/bash # Check if Info.plist exists and contains a bundle identifier fd -e plist "Info.plist" | xargs -I {} sh -c 'echo "Checking {}:"; grep -A 1 "CFBundleIdentifier" "{}"'Length of output: 1055
Action Required: Ensure a Valid Bundle Identifier for macOS Notifications
It appears that while several examples include a properly set CFBundleIdentifier in their Info.plist files (e.g., in v3/examples/build/build/Info.plist and v3/examples/dev/build/Info.plist), there isn’t evidence of an Info.plist in the notifications example. According to the PR objectives, macOS notifications require a valid bundle identifier to function correctly.
- Please verify that the notifications example either includes its own Info.plist with a valid CFBundleIdentifier or clearly documents how a consumer should supply one.
- If no dedicated Info.plist exists for the notifications sample, consider adding validation logic or including documentation that outlines the steps to set a proper bundle identifier during the build or configuration phase.
v3/examples/notifications/main.go
Outdated
| app.OnEvent("notificationResponse", func(event *application.CustomEvent) { | ||
| data, _ := json.Marshal(event.Data) | ||
| fmt.Printf("%s\n", string(data)) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle JSON marshaling errors.
The error from json.Marshal is silently ignored. Consider handling the error to ensure data integrity.
app.OnEvent("notificationResponse", func(event *application.CustomEvent) {
- data, _ := json.Marshal(event.Data)
- fmt.Printf("%s\n", string(data))
+ data, err := json.Marshal(event.Data)
+ if err != nil {
+ log.Printf("Failed to marshal notification response: %v\n", err)
+ return
+ }
+ fmt.Printf("%s\n", string(data))
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| app.OnEvent("notificationResponse", func(event *application.CustomEvent) { | |
| data, _ := json.Marshal(event.Data) | |
| fmt.Printf("%s\n", string(data)) | |
| }) | |
| app.OnEvent("notificationResponse", func(event *application.CustomEvent) { | |
| data, err := json.Marshal(event.Data) | |
| if err != nil { | |
| log.Printf("Failed to marshal notification response: %v\n", err) | |
| return | |
| } | |
| fmt.Printf("%s\n", string(data)) | |
| }) |
|
Here is where we are at:
|
…to notifications_darwin
|
Okay another update
Windows:
Linux:
All platforms:
Almost there! |
Co-authored-by: Lea Anthony <[email protected]>
Description
Adds basic notification support to macOS via UNNotification API, Windows via go-toast, and currently testing on Linux via dbus.
Fixes (#1788)
Current implementation:
Future features:
ActivationTypeoptions?Known changes before:
NotificationResponseDataAppDataconfigLimitations/Notes/Thoughts/Questions:
macOS:
Windows:
Should we include the icon by default to the toast config? macOS provides the app icon by default in the notification banner.Need to work to better understand Windows app GUIDs for go-toastAppDataconfigLinux:
Look at shout to better understand what this will take, would love helpPlatform differences:
The app requires a valid bundle identifier and needs to be signed.
Currently the dev build or binary will not launch and warn in the console. We probably want this to not block but just warn with no-op to any calls?
I am going to slightly refactor
CheckBundleIdentifiersince original source for this implementation logged from Obj-C. No need, we can handle in Go.CheckBundleIdentifier,RequestUserNotificationAuthorization, andCheckNotificationAuthorizationare available on macOS.I want to do more testing since I had to hack together a timeout to wait for
CheckNotificationAuthorization.Might just reuse
RequestUserNotificationAuthorizationif I can.CheckBundleIdentifier,RequestUserNotificationAuthorization,and
CheckNotificationAuthorizationalways returnstrue.*Identifier should be unique and can be used by other methods to update or remove a specific notification.
Destructiveaction option is available on macOS.*Identifier is the toast's "AppID" and presented like a notification name.
Destructiveaction option is not available.Registered notification categories persist for the application, even between unauthorization and reauthorization of notifications for the app. This is handled by
UNUserNotificationCenterAPI.Registering needs to only occur once per unique category ID. Reregistering overrides the previous config.
For simplicity, only a single text field is currently configurable. Maybe make name more generic? Currently referred to as a reply field. If set to true you can customize the text input placeholder and button.
On service start and shutdown the registry is updated.
To match the macOS implementation if reply field is set to true a single text input is added with the customization features. It will add an action button with the reply button title text provided.
UNUserNotificationCenterAPI provides a ton of features including removing pending and delivered notifications.RemoveAllPendingNotifications,RemovePendingNotification,RemoveAllDeliveredNotifications, andRemoveDeliveredNotificationare available on macOS.RemoveAllPendingNotifications,RemovePendingNotification,RemoveAllDeliveredNotifications, andRemoveDeliveredNotificationare no-ops.userInfo.Initially I explored this when implementing notifications with actions but maybe we should allow this for basic notifications as well?
didReceiveNotificationResponseis provided by theUNUserNotificationCenterand calls back to Go to send the response to the provided callback.notificationResponseevent.Like macOS we send the response to the provided callback.
Notifications via
UNNotificationAPI is robust. The Windows Toast API supports a lot of customization options for visual appearance but interaction is not nearly as feature rich. Open to other options on Windows to close the gap!I want to call it out here that I found this example tiny-usernotifications-example to prove this could all work!
Type of change
Please select the option that is relevant.
How Has This Been Tested?
I have been staring at this a lot and tested the API through Go and JS but I am sure I have missed something. Example is included in the v3/examples dir. Help!
If you checked Linux, please specify the distro and version.
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Chores