[v3] Notifications API #4098
[v3] Notifications API #4098
Conversation
WalkthroughThis pull request introduces a comprehensive notification system into a Wails-based desktop application. It adds new frontend assets including HTML, CSS, TypeScript bindings, and configuration files; implements backend functionality in Go with cross-platform support (Darwin, Windows, and Linux) for notifications; provides an extensive set of build and packaging Taskfiles and scripts for different operating systems; updates documentation and changelog entries; and adds additional utilities such as Windows icon management. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant OSNotification
User->>Frontend: Click "Send Notification" button
Frontend->>Backend: Request notification authorization
Backend->>OSNotification: Check notification permission
OSNotification-->>Backend: Return authorization status
Backend-->>Frontend: Send authorization response
Frontend->>Backend: Trigger notification (basic/with actions)
Backend->>OSNotification: Dispatch notification with details
OSNotification-->>User: Display notification
User->>OSNotification: Interact with notification (e.g., click action)
OSNotification->>Backend: Emit notification response event
Backend-->>Frontend: Forward notification response
Poem
✨ Finishing Touches
🪧 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)
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.
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.
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.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (26)
v3/examples/notifications/build/linux/appimage/build.sh (1)
8-15: Add variable validation to prevent script failures.The script assumes environment variables are set but doesn't validate them. This could lead to cryptic errors if they're missing.
# Define variables APP_DIR="${APP_NAME}.AppDir" +# Validate required environment variables +if [ -z "${APP_NAME}" ]; then + echo "Error: APP_NAME environment variable is not set" + exit 1 +fi + +if [ -z "${APP_BINARY}" ] || [ ! -f "${APP_BINARY}" ]; then + echo "Error: APP_BINARY environment variable is not set or file does not exist" + exit 1 +fi + +if [ -z "${ICON_PATH}" ] || [ ! -f "${ICON_PATH}" ]; then + echo "Error: ICON_PATH environment variable is not set or file does not exist" + exit 1 +fi + +if [ -z "${DESKTOP_FILE}" ] || [ ! -f "${DESKTOP_FILE}" ]; then + echo "Error: DESKTOP_FILE environment variable is not set or file does not exist" + exit 1 +fi + # Create AppDir structure mkdir -p "${APP_DIR}/usr/bin"v3/examples/notifications/frontend/index.html (1)
22-27: Enhance feedback for notification interactions.The notifications UI provides buttons for sending notifications but lacks clear feedback on success/failure.
<div class="controls"> <button class="btn" id="basic">Basic</button> <button class="btn" id="complex">Complex</button> </div> -<div class="footer" id="response"></div> +<div class="footer" id="response"> + <div class="status-indicator"></div> + <div class="notification-history"></div> +</div>v3/examples/notifications/main.go (3)
70-71: Improve error logging format.Using
printlnwithfmt.Errorfis unusual and inconsistent with Go logging practices.- println(fmt.Errorf("checking app notification authorization failed: %s", err)) + log.Printf("checking app notification authorization failed: %s\n", err)This pattern appears multiple times throughout the file (lines 76, 83, 103, 123, 139).
107-108: Replace blocking Sleep with non-blocking alternative.Using
time.Sleepblocks the goroutine. Consider a more robust approach with context-based cancellation.// Delay before sending next notification -time.Sleep(time.Second * 2) +select { +case <-time.After(time.Second * 2): + // Continue execution +case <-app.Context().Done(): + return // Exit if application is shutting down +}
85-88: Handle EmitEvent errors.The
EmitEventmethod might fail, but errors are not being checked.fmt.Printf("Response: %+v\n", result.Response) println("Sending response to frontend...") -app.EmitEvent("notification:action", result.Response) +if err := app.EmitEvent("notification:action", result.Response); err != nil { + log.Printf("Failed to emit notification action event: %s\n", err) +}v3/examples/notifications/frontend/src/main.ts (5)
1-25: Notification handling looks good, but could benefit from centralized authorization handling.The basic notification implementation correctly checks for authorization before sending. The error handling is well-structured with proper try/catch blocks, and the notification includes all necessary properties.
Consider extracting the authorization check into a reusable function since it appears in both notification handlers:
+async function checkNotificationAuthorization() { + const authorized = await Notifications.Service.CheckNotificationAuthorization(); + if (!authorized) { + console.warn("Notifications are not authorized.\n You can attempt to request again or let the user know in the UI.\n"); + } + return authorized; +} document.querySelector("#basic")?.addEventListener("click", async () => { try { - const authorized = await Notifications.Service.CheckNotificationAuthorization(); - if (authorized) { + if (await checkNotificationAuthorization()) { await Notifications.Service.SendNotification({ id: crypto.randomUUID(), title: "Notification Title", subtitle: "Subtitle on macOS and Linux", body: "Body text of notification.", data: {
15-16: Fix inconsistent indentation.There's inconsistent indentation in the data object (tabs instead of spaces).
data: { "user-id": "user-123", - "message-id": "msg-123", - "timestamp": Date.now(), + "message-id": "msg-123", + "timestamp": Date.now(), },
39-41: Fix inconsistent indentation.There's inconsistent indentation in these lines (tabs instead of spaces).
], - hasReplyField: true, - replyPlaceholder: "Message...", - replyButtonTitle: "Reply", + hasReplyField: true, + replyPlaceholder: "Message...", + replyButtonTitle: "Reply", });
52-53: Fix inconsistent indentation.There's inconsistent indentation in these lines (tabs instead of spaces).
data: { "user-id": "user-123", - "message-id": "msg-123", - "timestamp": Date.now(), + "message-id": "msg-123", + "timestamp": Date.now(), },
64-93: Fix typo in console log and consider using a template function for table generation.There's a typo in the console log message and the table generation could be more maintainable.
const unlisten = Events.On("notification:action", (response) => { - console.info(`Recieved a ${response.name} event`); + console.info(`Received a ${response.name} event`); const { userInfo, ...base } = response.data[0]; console.info("Notification Response:"); console.table(base); console.info("Notification Response Metadata:"); console.table(userInfo); - const table = ` - <h5>Notification Response</h5> - <table> - <thead> - ${Object.keys(base).map(key => `<th>${key}</th>`).join("")} - </thead> - <tbody> - ${Object.values(base).map(value => `<td>${value}</td>`).join("")} - </tbody> - </table> - <h5>Notification Metadata</h5> - <table> - <thead> - ${Object.keys(userInfo).map(key => `<th>${key}</th>`).join("")} - </thead> - <tbody> - ${Object.values(userInfo).map(value => `<td>${value}</td>`).join("")} - </tbody> - </table> - `; + + // Helper function to generate HTML table from object + const generateTable = (title, data) => ` + <h5>${title}</h5> + <table> + <thead> + ${Object.keys(data).map(key => `<th>${key}</th>`).join("")} + </thead> + <tbody> + ${Object.values(data).map(value => `<td>${value}</td>`).join("")} + </tbody> + </table> + `; + + const table = generateTable("Notification Response", base) + + generateTable("Notification Metadata", userInfo); + const footer = document.querySelector("#response"); if (footer) footer.innerHTML = table; });v3/examples/notifications/README.md (3)
11-14: Add language specification to code block.Adding a language specifier to the code block improves syntax highlighting and readability.
- ``` + ```bash wails3 dev ```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
11-11: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
19-22: Add language specification to code block.Adding a language specifier to the code block improves syntax highlighting and readability.
- ``` + ```bash wails3 build ```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
19-19: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
33-36: Add language specification to code block.Adding a language specifier to the code block improves syntax highlighting and readability.
- ``` + ```bash go run . ```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
33-33: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
v3/examples/notifications/build/config.yml (3)
47-47: Update placeholder URL in documentation comment.The URL appears to be a placeholder and not an actual resource.
# File Associations -# More information at: https://v3.wails.io/noit/done/yet +# More information at: https://v3.wails.io/guides/file-associationsVerify with the Wails team what the correct URL should be for file associations documentation.
60-62: Add newline at end of file and consider providing example for "Other" data.The file is missing a newline at the end and the "Other" section could use more explanation.
# Other data other: - - name: My Other Data + - name: My Other Data + value: "Example value" # Example of custom metadata +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 62-62: no new line character at the end of file
(new-line-at-end-of-file)
8-14: Update placeholder application information.The configuration includes placeholder information that should be updated for the notification example.
info: - companyName: "My Company" # The name of the company - productName: "My Product" # The name of the application - productIdentifier: "com.mycompany.myproduct" # The unique product identifier - description: "A program that does X" # The application description - copyright: "(c) 2025, My Company" # Copyright text - comments: "Some Product Comments" # Comments + companyName: "Wails" # The name of the company + productName: "Notifications Example" # The name of the application + productIdentifier: "io.wails.examples.notifications" # The unique product identifier + description: "An example application demonstrating notifications in Wails v3" # The application description + copyright: "(c) 2025, Wails Contributors" # Copyright text + comments: "Showcases notification capabilities across platforms" # Comments version: "v0.0.1" # The application versionv3/examples/notifications/build/windows/Taskfile.yml (2)
54-56: Ensure WebView2 bootstrapper generation has error handling.The command to generate the WebView2 bootstrapper should handle potential errors gracefully.
cmds: # Create the Microsoft WebView2 bootstrapper if it doesn't exist - - wails3 generate webview2bootstrapper -dir "{{.ROOT_DIR}}/build/windows/nsis" + - cmd: wails3 generate webview2bootstrapper -dir "{{.ROOT_DIR}}/build/windows/nsis" + ignore_error: true - makensis -DARG_WAILS_{{.ARG_FLAG}}_BINARY="{{.ROOT_DIR}}/{{.BIN_DIR}}/{{.APP_NAME}}.exe" project.nsi
63-63: Add newline at end of file.The file is missing a newline at the end.
run: cmds: - '{{.BIN_DIR}}/{{.APP_NAME}}.exe' +v3/examples/notifications/build/windows/nsis/wails_tools.nsh (2)
153-154: Maintain consistent indentationUse consistent indentation throughout the script - this line uses tabs while most others use spaces.
- # If the admin key exists and is not empty then webview2 is already installed + # If the admin key exists and is not empty then webview2 is already installed
205-207: Document how to configure file associationsThe
wails.associateFilesmacro is empty. Consider adding documentation explaining how developers should customize this section.v3/examples/notifications/build/darwin/Taskfile.yml (1)
73-80: Consider making the run task platform-independentThe
runtask doesn't provide a summary like other tasks. Consider adding a summary and ensuring it works reliably across different environments.run: + summary: Prepares and runs a development version of the application cmds:v3/examples/notifications/build/windows/nsis/project.nsi (1)
102-102: Use application-specific AppData pathThe current path removal could potentially delete data for other applications with the same executable name. Consider using a more specific path.
- RMDir /r "$AppData\${PRODUCT_EXECUTABLE}" # Remove the WebView2 DataPath + RMDir /r "$AppData\${INFO_COMPANYNAME}\${INFO_PRODUCTNAME}" # Remove the application datav3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/models.ts (3)
64-102: Add validation for required fieldsThe
NotificationOptionsclass initializes empty strings for required fields, but doesn't validate if they remain empty after assignment.Consider adding validation to ensure required fields aren't empty:
/** Creates a new NotificationOptions instance. */ constructor($$source: Partial<NotificationOptions> = {}) { if (!("id" in $$source)) { this["id"] = ""; } if (!("title" in $$source)) { this["title"] = ""; } Object.assign(this, $$source); + + // Validate required fields + if (!this.id) { + console.warn("NotificationOptions: id is required"); + } + if (!this.title) { + console.warn("NotificationOptions: title is required"); + } }
18-18: Add comprehensive documentation for platform-specific featuresThe
destructiveproperty is documented as macOS-specific but doesn't explain what it does./** - * (macOS-specific) + * (macOS-specific) When set to true, the action button will be displayed with destructive styling (red text) */ "destructive"?: boolean;
74-77: Document platform limitations forsubtitleand other propertiesWhile the code notes that
subtitleis macOS and Linux only, it would be helpful to document the behavior on Windows or other unsupported platforms./** - * (macOS and Linux only) + * (macOS and Linux only) A secondary title. This field is ignored on Windows. */ "subtitle"?: string;v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/service.ts (1)
49-62: Consider adding platform-specific documentation for notification behavior differencesThe functions for requesting authorization and sending notifications work correctly, but macOS and Windows have different notification behaviors as mentioned in the PR description.
Consider adding platform-specific JSDoc comments to highlight differences between:
- macOS requiring explicit permission while Windows does not
- How notification data is handled differently between platforms
- Any platform-specific limitations users should be aware of
/** + * Requests authorization to display notifications. + * @remarks + * On macOS: User permission is required and this shows a system dialog. + * On Windows: Always returns true as permissions are not required. + * On Linux: Behavior depends on the notification server implementation. * Public methods that delegate to the implementation. */ export function RequestNotificationAuthorization(): $CancellablePromise<boolean> { return $Call.ByID(729898933); } +/** + * Sends a basic notification. + * @param options The notification options + * @remarks + * On macOS: Requires a valid bundle identifier and app signing. + * On Windows: Uses the go-toast library with limited metadata support. + */ export function SendNotification(options: $models.NotificationOptions): $CancellablePromise<void> { return $Call.ByID(2246903123, options); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
v3/examples/notifications/build/appicon.pngis excluded by!**/*.pngv3/examples/notifications/build/windows/icon.icois excluded by!**/*.icov3/examples/notifications/frontend/package-lock.jsonis excluded by!**/package-lock.jsonv3/examples/notifications/frontend/public/typescript.svgis excluded by!**/*.svg
📒 Files selected for processing (30)
v3/examples/notifications/README.md(1 hunks)v3/examples/notifications/Taskfile.yml(1 hunks)v3/examples/notifications/build/Taskfile.yml(1 hunks)v3/examples/notifications/build/config.yml(1 hunks)v3/examples/notifications/build/darwin/Info.dev.plist(1 hunks)v3/examples/notifications/build/darwin/Info.plist(1 hunks)v3/examples/notifications/build/darwin/Taskfile.yml(1 hunks)v3/examples/notifications/build/linux/Taskfile.yml(1 hunks)v3/examples/notifications/build/linux/appimage/build.sh(1 hunks)v3/examples/notifications/build/linux/nfpm/nfpm.yaml(1 hunks)v3/examples/notifications/build/linux/nfpm/scripts/postinstall.sh(1 hunks)v3/examples/notifications/build/linux/nfpm/scripts/postremove.sh(1 hunks)v3/examples/notifications/build/linux/nfpm/scripts/preinstall.sh(1 hunks)v3/examples/notifications/build/linux/nfpm/scripts/preremove.sh(1 hunks)v3/examples/notifications/build/windows/Taskfile.yml(1 hunks)v3/examples/notifications/build/windows/info.json(1 hunks)v3/examples/notifications/build/windows/nsis/project.nsi(1 hunks)v3/examples/notifications/build/windows/nsis/wails_tools.nsh(1 hunks)v3/examples/notifications/build/windows/wails.exe.manifest(1 hunks)v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/index.ts(1 hunks)v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/models.ts(1 hunks)v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/service.ts(1 hunks)v3/examples/notifications/frontend/index.html(1 hunks)v3/examples/notifications/frontend/package.json(1 hunks)v3/examples/notifications/frontend/public/style.css(1 hunks)v3/examples/notifications/frontend/src/main.ts(1 hunks)v3/examples/notifications/frontend/src/vite-env.d.ts(1 hunks)v3/examples/notifications/frontend/tsconfig.json(1 hunks)v3/examples/notifications/go.mod(1 hunks)v3/examples/notifications/main.go(1 hunks)
✅ Files skipped from review due to trivial changes (13)
- v3/examples/notifications/build/linux/nfpm/scripts/postinstall.sh
- v3/examples/notifications/build/linux/nfpm/scripts/preinstall.sh
- v3/examples/notifications/build/linux/nfpm/scripts/postremove.sh
- v3/examples/notifications/build/linux/nfpm/scripts/preremove.sh
- v3/examples/notifications/build/windows/info.json
- v3/examples/notifications/frontend/src/vite-env.d.ts
- v3/examples/notifications/build/windows/wails.exe.manifest
- v3/examples/notifications/build/darwin/Info.dev.plist
- v3/examples/notifications/frontend/tsconfig.json
- v3/examples/notifications/build/linux/nfpm/nfpm.yaml
- v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/index.ts
- v3/examples/notifications/build/darwin/Info.plist
- v3/examples/notifications/go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- v3/examples/notifications/frontend/public/style.css
- v3/examples/notifications/frontend/package.json
🧰 Additional context used
🧬 Code Definitions (2)
v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/service.ts (1)
v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/models.ts (2)
NotificationCategory(38-62)NotificationOptions(67-102)
v3/examples/notifications/main.go (2)
v3/pkg/services/notifications/notifications.go (20)
ns(111-113)ns(119-124)ns(128-136)ns(139-141)ns(144-146)ns(149-151)ns(153-155)ns(157-162)ns(164-169)ns(171-173)ns(175-177)ns(179-181)ns(183-185)ns(187-189)ns(191-193)ns(195-197)Service(47-55)NotificationOptions(80-87)NotificationCategory(71-77)NotificationAction(64-68)v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/models.ts (3)
NotificationOptions(67-102)NotificationCategory(38-62)NotificationAction(11-33)
🪛 LanguageTool
v3/examples/notifications/README.md
[uncategorized] ~47-~47: Loose punctuation mark.
Context: ...h your project structure: - frontend/: Contains your frontend code (HTML, CSS,...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~59-~59: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 2020 characters long)
Context: ...wails3 build`. Happy coding with Wails3! If you encounter any issues or have que...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.17.2)
v3/examples/notifications/README.md
11-11: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
19-19: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
33-33: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 YAMLlint (1.35.1)
v3/examples/notifications/build/config.yml
[error] 62-62: no new line character at the end of file
(new-line-at-end-of-file)
🪛 golangci-lint (1.64.8)
v3/examples/notifications/main.go
19-19: pattern all:frontend/dist: no matching files found
(typecheck)
🔇 Additional comments (27)
v3/examples/notifications/frontend/index.html (1)
13-18: LGTM! Using appropriate Wails URL opening attributes.The
data-wml-openURLattributes are correctly used to handle external URLs in the Wails framework.v3/examples/notifications/main.go (2)
65-142: Add goroutine cancellation mechanism.The goroutine lacks a cancellation mechanism, which could lead to resource leaks if the application shuts down before completion.
91-104: Enhance notification error handling with user feedback.Error handling only logs warnings without informing the user through the UI.
v3/examples/notifications/Taskfile.yml (3)
12-12: Good practice: Using default value for VITE_PORT.Using the default pattern for VITE_PORT is a good practice that allows overriding the port while providing a sensible default.
33-33: LGTM! Dev task using appropriate Wails CLI options.The dev task correctly uses the Wails CLI with appropriate configuration file and port parameters.
10-10:❓ Verification inconclusive
Verify APP_NAME escaping compatibility.
The
APP_NAMEvariable includes escaped spaces which may cause issues depending on how it's used in different OS-specific tasks.
🏁 Script executed:
#!/bin/bash # Check how APP_NAME is used across different taskfiles echo "Checking APP_NAME usage in taskfiles..." grep -r "APP_NAME" ./build/ --include="*.yml"Length of output: 176
Attention: Manual Verification of APP_NAME Escaping is Required
The automated script didn’t yield useful results because the target directory (“./build/”) wasn’t found. As a result, we couldn’t confirm whether the double-backslash in
APP_NAME: "Notifications\\ Demo"is interpreted as intended across OS-specific tasks.
Please manually verify that the escaped value works as expected in your environment. In particular, check that the escaping is compatible with all the task runners or scripts that consume this variable.
v3/examples/notifications/frontend/src/main.ts (1)
95-95: Good cleanup practice.Using the unlisten function in the window.onbeforeunload is a good practice to prevent memory leaks.
v3/examples/notifications/README.md (1)
1-60: Well-structured README with clear instructions.The README provides comprehensive guidance for setting up and running the notification application. It has clear sections for getting started, exploring features, understanding the project structure, and next steps.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~47-~47: Loose punctuation mark.
Context: ...h your project structure: -frontend/: Contains your frontend code (HTML, CSS,...(UNLIKELY_OPENING_PUNCTUATION)
[style] ~59-~59: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 2020 characters long)
Context: ...wails3 build`. Happy coding with Wails3! If you encounter any issues or have que...(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.17.2)
11-11: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
19-19: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
33-33: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
v3/examples/notifications/build/config.yml (1)
1-62: Well-structured configuration file with comprehensive dev mode settings.The configuration file provides detailed settings for development and build processes, with appropriate ignores and command executions. The fileAssociations section is commented out but available for future use.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 62-62: no new line character at the end of file
(new-line-at-end-of-file)
v3/examples/notifications/build/windows/Taskfile.yml (1)
1-63: Well-structured Windows build configuration with appropriate tasks.The Taskfile provides a comprehensive set of tasks for building, packaging, and running the application on Windows. It includes proper dependency management and environment variable configuration.
v3/examples/notifications/build/windows/nsis/project.nsi (1)
1-36: LGTM! Well-structured installer scriptThe installer script is well-organized with clear comments that explain how to use it for debugging and development. The inclusion of Unicode support and HiDPI awareness is good for modern Windows applications.
v3/examples/notifications/build/Taskfile.yml (7)
1-9: LGTM: Basic task setup for Go module managementThe
go:mod:tidytask is well-defined and follows standard practices for maintaining Go module dependencies.
10-22: LGTM: Frontend dependency management with appropriate preconditionsThe npm dependency installation task is well-structured with:
- Proper source and output file tracking
- A helpful precondition check for npm installation
- Clear error message if npm is not installed
24-44: LGTM: Flexible frontend build configurationThe build:frontend task correctly:
- Labels the task with the production status for clarity
- Uses conditional build commands based on the PRODUCTION variable
- Properly depends on frontend dependencies and bindings generation
46-62: LGTM: Comprehensive bindings generationThe generate:bindings task:
- Labels with current build flags
- Tracks appropriate source files including TypeScript/JavaScript
- Properly excludes and includes relevant paths
- Uses clean generation with TypeScript support
63-72: LGTM: Cross-platform icon generationThe task correctly generates both macOS (.icns) and Windows (.ico) format icons from a single source image.
74-81: LGTM: Development server configurationThe dev:frontend task includes necessary frontend dependencies and configures Vite with proper port settings using strictPort for reliability.
82-87: LGTM: Build assets update taskThe update:build-assets task properly configures the application name and config settings for the wails3 tool.
v3/examples/notifications/build/linux/Taskfile.yml (6)
1-27: LGTM: Well-structured Linux build configurationThe build task correctly:
- Depends on common tasks from the parent Taskfile
- Includes appropriate build flags for production vs. development builds
- Sets proper environment variables for Linux cross-compilation
- Uses CGO enabling which is important for certain Linux-specific features
The conditional build flags are well-designed to handle both production (with optimizations) and development modes.
28-39: LGTM: Comprehensive Linux packaging strategyThe package task efficiently orchestrates the creation of all supported Linux package formats (AppImage, deb, rpm, and AUR) in a single command, making distribution easier.
40-58: LGTM: AppImage creation with proper dependenciesThe AppImage creation task:
- Properly depends on a production build
- Generates the required .desktop file
- Copies necessary resources like icons
- Uses the wails3 tool correctly for AppImage generation
59-103: LGTM: Modular packaging tasks for various Linux distributionsThe tasks for different package formats (deb, rpm, AUR) follow a consistent pattern:
- Production build dependency
- Desktop file generation
- Package-specific generation using the appropriate task
The package generation tasks correctly use the wails3 tool with the appropriate format and configuration.
104-116: LGTM: Desktop file generation for Linux integrationThe generate:dotdesktop task:
- Creates the necessary directory structure
- Generates a proper .desktop file with required metadata
- Sets appropriate categories for the application menu
117-120: LGTM: Simple run taskThe run task provides a convenient way to execute the built application.
v3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/service.ts (3)
1-16: LGTM: Proper auto-generated file header and importsThe file correctly:
- Includes a warning not to modify auto-generated code
- Uses appropriate TypeScript module imports
- Properly imports models from a related file
17-19: LGTM: Authorization check implementationThe function appropriately returns a cancellable promise of boolean type to check notification authorization status.
21-48: LGTM: Well-designed notification management functionsThe notification management functions:
- Follow a consistent pattern using
$Call.ByID- Accept appropriate parameters (identifiers, category objects)
- Return cancellable promises with appropriate return types
- Support both individual and bulk notification management
The naming is clear and follows the expected patterns for both macOS and Windows notification paradigms.
| fi | ||
|
|
||
| # Rename the generated AppImage | ||
| mv "${APP_NAME}*.AppImage" "${APP_NAME}.AppImage" |
There was a problem hiding this comment.
Fix the wildcard pattern for the mv command.
The wildcard pattern inside double quotes won't be expanded by the shell. When using wildcards with shell commands, they should not be enclosed in quotes.
- mv "${APP_NAME}*.AppImage" "${APP_NAME}.AppImage"
+ mv ${APP_NAME}*.AppImage "${APP_NAME}.AppImage"📝 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.
| mv "${APP_NAME}*.AppImage" "${APP_NAME}.AppImage" | |
| mv ${APP_NAME}*.AppImage "${APP_NAME}.AppImage" |
| //go:embed all:frontend/dist | ||
| var assets embed.FS |
There was a problem hiding this comment.
Fix the embed directive to properly include frontend assets.
The static analysis warns that no matching files were found for the embedded pattern.
Ensure the frontend assets are built before running the application. The directory structure should be:
v3/examples/notifications/
├── frontend/
│ ├── dist/
│ │ └── ... (built frontend files)
Make sure to run the frontend build process first:
cd frontend && npm run build🧰 Tools
🪛 golangci-lint (1.64.8)
19-19: pattern all:frontend/dist: no matching files found
(typecheck)
|
|
||
| Note: Some examples may be under development during the alpha phase. | ||
|
|
||
| 3. **Explore the documentation**: Visit the [Wails3 documentation](https://v3.wails.io/) for in-depth guides and API references. |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Double-check documentation URL.
The URL points to the Wails3 documentation which is still in development. Verify that this URL will be valid when the PR is merged.
🌐 Web query:
Is https://v3.wails.io/ a valid and accessible URL for Wails v3 documentation?
💡 Result:
The URL https://v3.wails.io/ does not appear to be valid or referenced in the official Wails v3 documentation. Based on search results, the correct URL for Wails v3 documentation is https://v3alpha.wails.io/, which is actively maintained and referenced in multiple guides and release notes[1][4][7][10][13].
This distinction is important because:
- All Wails v3 documentation links explicitly use the
v3alphasubdomain[1][4][7] - The
v3alphaprefix reflects the alpha status of the current v3 release[1][6] - Official GitHub issues and CLI references specifically use
v3alphaURLs[16][17] - The main Wails website (wails.io) still references v2 documentation as the primary stable version[5][11]
For developers working with Wails v3, the valid documentation URL is https://v3alpha.wails.io/. This aligns with the project's current pre-release status and matches all official references in March 2025 documentation[1][4][13].
Citations:
- 1: https://v3alpha.wails.io/whats-new/
- 2: https://pkg.go.dev/github.com/wailsapp/wails
- 3: https://wails.io/docs/guides/signing/
- 4: https://v3alpha.wails.io/guides/custom-templates/
- 5: https://wails.io/docs/gettingstarted/installation
- 6: https://wails.io/blog/the-road-to-wails-v3/
- 7: https://v3alpha.wails.io/guides/cli/
- 8: https://wails.io/community-guide/
- 9: https://wails.io/docs/guides/mac-appstore
- 10: https://v3alpha.wails.io/getting-started/your-first-app/
- 11: https://wails.io
- 12: https://wails.io/docs/reference/options/
- 13: https://v3alpha.wails.io/learn/build/
- 14: undefined: application.NewContextMenu on Wails (v3.0.0-alpha.9). Am I missing something? #4150
- 15: wails dev: Unable to start application #2277
- 16: https://pkg.go.dev/github.com/wailsapp/wails/v3/cmd/wails3
- 17: [v3] wails3 alpha 8 schema changes not working #3935
- 18: https://pkg.go.dev/github.com/wailsapp/wails/v3/plugins/experimental/server
- 19: https://www.waterboards.ca.gov/rwqcb9/water_issues/programs/shipyards_sediment/docs/sediment_cleanup/adt/updates061411/nassco_supp_ar_part8.pdf
- 20: Open URL directly #1516
Update Documentation URL
The documentation link in v3/examples/notifications/README.md (line 39) currently points to https://v3.wails.io/, which is not valid for Wails v3. Please update it to https://v3alpha.wails.io/ to align with the official, maintained documentation references.
| ReadRegStr $0 HKLM "SOFTWARE\WOW6432Node\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}" "pv" | ||
| ${If} $0 != "" | ||
| Goto ok | ||
| ${EndIf} | ||
|
|
||
| ${If} ${REQUEST_EXECUTION_LEVEL} == "user" | ||
| # If the installer is run in user level, check the user specific key exists and is not empty then webview2 is already installed | ||
| ReadRegStr $0 HKCU "Software\Microsoft\EdgeUpdate\Clients{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}" "pv" | ||
| ${If} $0 != "" | ||
| Goto ok | ||
| ${EndIf} |
There was a problem hiding this comment.
Fix Registry Key Path for WebView2 Detection
There's an issue in the WebView2 detection logic when running with user-level permissions. Line 161 is missing a separator between "Clients" and the GUID.
- ReadRegStr $0 HKCU "Software\Microsoft\EdgeUpdate\Clients{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}" "pv"
+ ReadRegStr $0 HKCU "Software\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}" "pv"📝 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.
| ReadRegStr $0 HKLM "SOFTWARE\WOW6432Node\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}" "pv" | |
| ${If} $0 != "" | |
| Goto ok | |
| ${EndIf} | |
| ${If} ${REQUEST_EXECUTION_LEVEL} == "user" | |
| # If the installer is run in user level, check the user specific key exists and is not empty then webview2 is already installed | |
| ReadRegStr $0 HKCU "Software\Microsoft\EdgeUpdate\Clients{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}" "pv" | |
| ${If} $0 != "" | |
| Goto ok | |
| ${EndIf} | |
| ReadRegStr $0 HKLM "SOFTWARE\WOW6432Node\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}" "pv" | |
| ${If} $0 != "" | |
| Goto ok | |
| ${EndIf} | |
| ${If} ${REQUEST_EXECUTION_LEVEL} == "user" | |
| # If the installer is run in user level, check the user specific key exists and is not empty then webview2 is already installed | |
| ReadRegStr $0 HKCU "Software\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}" "pv" | |
| ${If} $0 != "" | |
| Goto ok | |
| ${EndIf} |
| GOOS: darwin | ||
| CGO_ENABLED: 1 | ||
| GOARCH: '{{.ARCH | default ARCH}}' | ||
| CGO_CFLAGS: "-mmacosx-version-min=10.15" | ||
| CGO_LDFLAGS: "-mmacosx-version-min=10.15" | ||
| MACOSX_DEPLOYMENT_TARGET: "10.15" | ||
| PRODUCTION: '{{.PRODUCTION | default "false"}}' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify macOS version compatibility with notifications API
There's a potential version mismatch. The PR objectives state that macOS 11.0 is the minimum required version for notifications, but the build settings target macOS 10.15.
🏁 Script executed:
#!/bin/bash
# Check if notifications API requires macOS 11.0 as stated in PR objectives
echo "Checking if UNNotification API requires macOS 11.0"
find . -type f -name "*.go" | xargs grep -l "UNNotification" | xargs grep -A 5 -B 5 "darwin"Length of output: 6136
Action Required: Update macOS Deployment Target in Taskfile.yml
The build configuration in v3/examples/notifications/build/darwin/Taskfile.yml (lines 25–31) still specifies a macOS version of 10.15. However, the notifications implementation uses Cgo flags that require a minimum version of macOS 11.0 (e.g., -mmacosx-version-min=11.0 in the CFLAGS and LDFLAGS). Since the PR objectives indicate that macOS 11.0 is the minimum supported version for notifications, please update the build settings in this Taskfile accordingly or provide clarification if an alternative approach is intended.
- Update the following settings:
CGO_CFLAGSfrom"-mmacosx-version-min=10.15"to at least"-mmacosx-version-min=11.0"CGO_LDFLAGSfrom"-mmacosx-version-min=10.15"to at least"-mmacosx-version-min=11"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
v3/pkg/services/notifications/notifications_linux.go (5)
55-73: Consider providing an actionable fallback or returning an error when DBus is unavailable.
Currently, the code prints warnings and proceeds if the DBus session bus connection fails. This may conceal unavailability of notifications from the rest of the system. Providing an explicit error return or enabling an in-app notification fallback can help developers handle this scenario more cleanly.
107-109: Enhance recovery when reading categories fails.
The code only logs an error on failing to load notification categories and then continues. You could return an error or provide a clear fallback to ensure the application handles this scenario in a more controlled manner.
137-147: Evaluate Linux-specific notification authorization checks.
While these methods currently return static values, some desktop environments offer limited user-level notification controls. If you want to mirror macOS-like authorization checks, consider detecting system-level preferences or using environment-specific libraries to validate that notifications are not globally disabled.
219-303: Reduce duplicated code between “SendNotification” and “SendNotificationWithActions.”
Both functions share significant logic (e.g., building the notification body, preparing data, sending via D-Bus). Factor out common steps into a helper function to simplify maintenance and reduce potential inconsistencies.
576-627: Handle additional notification close reasons, if desired.
Currently, only reason code 2 (dismissed by user) triggers an event callback. Consider adding logic for reason code 1 (expired) or 3 (closed programmatically) if you need to distinguish or log those outcomes for user or system analytics.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/pkg/services/notifications/notifications_linux.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
v3/pkg/services/notifications/notifications_linux.go (2)
Learnt from: popaprozac
PR: wailsapp/wails#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
PR: wailsapp/wails#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.
🔇 Additional comments (1)
v3/pkg/services/notifications/notifications_linux.go (1)
331-349: Consider proactively removing notifications from the map on success.
Since the code relies on the “NotificationClosed” D-Bus signal to remove entries, if that signal is not received (e.g., due to a DBus disruption), notifications remain in memory. Removing them right after a successful call tocloseNotificationcan prevent stale data from accumulating.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v3/examples/notifications/build/windows/Taskfile.yml (2)
6-31: Windows Build Task Evaluation
Thebuildtask is neatly structured:
- Dependencies: It properly sequences tasks such as tidying Go modules, building the frontend (with dynamic variable mapping for
BUILD_FLAGSandPRODUCTION), and generating icons.- Commands:
- The build command (
go build {{.BUILD_FLAGS}} -o {{.BIN_DIR}}/{{.APP_NAME}}.exe) compiles the executable as expected.- The cleanup commands use platform-specific removal commands (
powershell Remove-item *.sysofor Windows, andrm -f *.sysofor Linux/Darwin). Consider whether you might need additional error handling or logging if these cleanup steps fail.- Variables & Environment: The assignment for
BUILD_FLAGSand the environment variables (GOOS,CGO_ENABLED,GOARCH, andPRODUCTION) is consistent with a Windows build configuration.
Overall, this section meets the build requirements.
46-60: NSIS Installer Creation Task
Thecreate:nsis:installertask is well defined:
- It ensures a production build is used by setting
PRODUCTIONto"true", which is good practice for release builds.- The generation of the Microsoft WebView2 bootstrapper via
wails3 generate webview2bootstrapperis a valuable inclusion to support modern Windows features.- The use of templated variables in the NSIS command (e.g.,
{{.ROOT_DIR}},{{.BIN_DIR}},{{.APP_NAME}}, and the derivedARG_FLAGconfiguration) is appropriate.- Confirm that the conditional expression for
ARG_FLAGcorrectly handles all expected architectures—currently mappingamd64toAMD64and all others toARM64. If more architectures need support in the future, this might require adjustment.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/examples/notifications/build/windows/Taskfile.yml(1 hunks)
🔇 Additional comments (5)
v3/examples/notifications/build/windows/Taskfile.yml (5)
1-2: Version Declaration and YAML Syntax
The Taskfile specifiesversion: '3', which appears standard and should be compatible with your tooling.
3-5: Includes Block Verification
The file includes a common Taskfile via../Taskfile.yml. Please ensure that this common file exists and its tasks do not conflict with the Windows-specific tasks declared here.
33-37: Package Task Review
Thepackagetask is straightforward, delegating the packaging process to thecreate:nsis:installertask. Ensure that the NSIS installer script (project.nsi) is properly configured to handle the production build output.
38-45: Generate.sysoFile Task
This task generates the Windows.sysofile usingwails3 generate sysowith several critical parameters (architecture, icon, manifest, and info JSON).
- Verify that the referenced files (
windows/icon.ico,windows/wails.exe.manifest, andwindows/info.json) exist at the specified paths and are up to date.- The templated variable for architecture (
{{.ARCH}}) should align with your build configurations.
61-64: Run Task Verification
Theruntask simply executes the built application. Double-check that the variablesBIN_DIRandAPP_NAMEare properly set in your environment or upstream task outputs to avoid runtime path issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
v3/pkg/services/notifications/notifications_linux.go (5)
44-57: Good singleton approach, but consider signaling errors on re-init.Using
sync.Onceand a globalNotificationServiceis a common pattern for implementing a singleton. However, if the user attempts to callNew()twice in the same process, the current code silently re-returns the existing instance. As a minor improvement, you could detect re-initialization attempts (e.g., by checking ifNotificationService != nil) and return an explicit error or warning to the caller.
59-81: Handle load failures for categories more strictly.If
ln.loadCategories()fails, the code logs an error message but still proceeds with startup. Consider returning an error if loading categories is essential for correct functionality. This ensures early feedback in case of file corruption or user misconfigurations.
111-175: Consider handling duplicate notification IDs.When sending notifications, the code stores
options.IDinln.notifications. If a second notification with the same ID is sent before the first is closed, it might overwrite the existing entry. If that’s intentional, ignore this feedback. Otherwise, consider validating or generating unique IDs to avoid collisions.
418-440: Revisit buffer size for DBus signals.
make(chan *dbus.Signal, 10)might risk blocking if more than 10 signals arrive at once. While unlikely for many apps, in high-volume notification scenarios it could cause dropped signals or hamper responsiveness. Increasing the buffer size or moving signal processing to another goroutine can help avoid blocking the DBus signal loop.
442-565: Consider handling other close reasons inhandleNotificationClosed.Currently, reason
2triggers the default action but other legitimate close reasons (e.g., reason1– expired) do not raise any callback. This may lead to missed user interactions or events. Consider logging/handling additional reason codes if you want to track or respond to timed-out or programmatically closed notifications as well.v3/pkg/services/notifications/notifications_windows.go (1)
58-116: Validate callback for unexpected or empty arguments.The activation callback handles parsed arguments and user data well. However, consider how you want to handle a scenario if
argsis empty or if parsing yields no valid payload at all. Logging a warning might help debug scenarios where notifications lack expected data.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v3/pkg/services/notifications/notifications_linux.go(1 hunks)v3/pkg/services/notifications/notifications_windows.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
v3/pkg/services/notifications/notifications_windows.go (3)
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_windows.go:91-113
Timestamp: 2025-03-19T19:14:28.339Z
Learning: The JSON quote handling in Windows notifications' activation arguments (v3/pkg/services/notifications/notifications_windows.go) has a known limitation with single-quote collisions that needs to be addressed after initial testing.
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_windows.go:113-164
Timestamp: 2025-03-19T19:14:28.339Z
Learning: In Windows Toast notifications (notifications_windows.go), metadata must be appended to each action's Arguments because the toast activated callback only returns the Arguments string of the clicked action. This is achieved by appending the same escaped JSON data (with single quotes) to all actions using the ":::" delimiter.
Learnt from: popaprozac
PR: wailsapp/wails#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.
v3/pkg/services/notifications/notifications_linux.go (2)
Learnt from: popaprozac
PR: wailsapp/wails#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
PR: wailsapp/wails#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.
🧬 Code Definitions (2)
v3/pkg/services/notifications/notifications_windows.go (1)
v3/pkg/services/notifications/notifications.go (23)
NotificationCategory(71-77)NotificationOptions(80-87)Service(47-55)NotificationService(59-59)NotificationResult(105-108)ns(111-113)ns(119-124)ns(128-136)ns(139-141)ns(144-146)ns(149-151)ns(153-155)ns(157-162)ns(164-169)ns(171-173)ns(175-177)ns(179-181)ns(183-185)ns(187-189)ns(191-193)ns(195-197)getNotificationService(199-203)NotificationResponse(92-101)
v3/pkg/services/notifications/notifications_linux.go (3)
v3/pkg/services/notifications/notifications.go (24)
NotificationCategory(71-77)Service(47-55)notificationServiceOnce(58-58)NotificationService(59-59)NotificationOptions(80-87)NotificationResponse(92-101)NotificationResult(105-108)ns(111-113)ns(119-124)ns(128-136)ns(139-141)ns(144-146)ns(149-151)ns(153-155)ns(157-162)ns(164-169)ns(171-173)ns(175-177)ns(179-181)ns(183-185)ns(187-189)ns(191-193)ns(195-197)getNotificationService(199-203)v3/pkg/services/notifications/notifications_windows.go (1)
New(44-56)v3/pkg/services/notifications/notifications_darwin.go (3)
New(37-59)err(323-323)response(359-359)
🔇 Additional comments (8)
v3/pkg/services/notifications/notifications_linux.go (2)
177-257: Good consistent logic for actions-based notifications.The approach for
SendNotificationWithActionsmirrorsSendNotificationwhile adding category-derived actions. The fallback to a basic notification if the category is missing is a user-friendly default. Nicely done.
83-97:❓ Verification inconclusive
Confirm concurrency safety on shutdown.
Shutdowncallsln.conn.Close()after cancelling the context. If there are still goroutines inhandleSignalsor other methods referencingln.conn, you might encounter unexpected DBus behavior.Could you confirm that all goroutines have exited or will handle a nil/closed connection gracefully? If needed, you can run the following script to search for references to
ln.connoutside ofShutdownand verify their concurrency handling:
🏁 Script executed:
#!/bin/bash rg 'ln\.conn' -A 5Length of output: 3873
Action Required: Confirm Concurrency Safety on Shutdown
Based on our search results, there are multiple code paths referencing
ln.conn—including calls made in functions such ashandleSignals(which is invoked in a separate goroutine). AfterShutdowncancels the context and callsln.conn.Close(), it isn’t immediately clear that all goroutines accessingln.connare properly synchronized or safely handling a closed connection.
- Verify that the
handleSignalsgoroutine (and any other goroutines usingln.conn) correctly exits or checks for cancellation before performing operations onln.conn.- Ensure that no goroutines are left operating on a closed connection or in a race condition scenario.
Please review and confirm that these concurrent accesses are safe—either by ensuring goroutines exit before the connection is closed or by adding appropriate synchronization (e.g., wait groups or nil checks) to avoid unexpected D-Bus behavior.
v3/pkg/services/notifications/notifications_windows.go (6)
44-56: Singleton creation looks good.Using
sync.Oncewith a globalNotificationServiceensures only one Windows notifier instance. This approach is consistent with the macOS and Linux implementations, providing a uniform cross-platform interface.
162-222: Comprehensive approach for action-based notifications.The code nicely encodes per-action payloads, ensuring that each action returns relevant metadata. Good work bridging the Windows Toast API limitation that only returns the clicked action’s argument by appending JSON data to each action argument.
224-249: Registry concurrency handling is clear.
RegisterNotificationCategoryandRemoveNotificationCategoryboth lock, then save categories to the registry. This consistent approach prevents data races when multiple threads register or remove categories concurrently. Good job!
281-295: Robust payload encoding with base64.Wrapping the action ID and user data in a base64-encoded JSON struct is a flexible solution for passing metadata. If you require advanced escaping for complex strings, you can easily evolve this format with minimal breakage to existing code.
403-433: Confirm curly braces in GUID.
generateGUIDreturns a GUID wrapped in curly braces, which you then store in the registry. While typical for Windows, it’s worth verifying that your usage always requires these braces. In some contexts, the system might accept a GUID without braces.
138-160:❓ Verification inconclusive
Basic notifications flow: watch out for Windows argument size limits.
Encoding the notification payload as a base64 string and passing it via
ActivationArgumentsis an effective technique. However, Windows Toast notifications may have size restrictions on the argument string. If yourDatacan be large, you might want to confirm or document that users should keep payloads minimal.
🌐 Web query:
What is the maximum length or size limit for Windows Toast notification arguments?💡 Result:
The maximum length for Windows Toast notification arguments isn't explicitly defined in the provided sources, but related constraints and best practices can be inferred:
Toast XML Payload Size:
- 3 MB on normal internet connections.
- 1 MB on metered connections[4].
Group Identifier (Group Property):
- Historically limited to 16 characters, increased to 64 characters in the Creators Update (Build 15063)[1].
Text Elements:
- Primary text lines are truncated to ~45 characters, with secondary text limited to ~200 characters in total across all lines[11].
- Action Center displays a maximum of 20 recent notifications, which may indirectly affect argument handling in grouped scenarios[9].
For arguments passed via the
launchattribute or APIs, developers should aim to keep values concise (e.g., below 64 characters as a precaution[1][4]) and use lightweight identifiers or tokens rather than large data payloads. If structured data is required, consider storing details server-side and referencing them via a key in the Toast arguments.Always validate argument length and payload size against the target Windows version’s constraints.
Citations:
- 1: https://learn.microsoft.com/en-us/uwp/api/windows.ui.notifications.toastnotification.group?view=winrt-26100
- 2: character limit notification text Windos/BurntToast#221
- 3: https://www.imab.dk/windows-10-toast-notification-script/
- 4: https://learn.microsoft.com/en-us/windows/apps/design/shell/tiles-and-notifications/adaptive-interactive-toasts
- 5: https://forums.windowscentral.com/threads/limit-on-push-toast-notifications.219339/
- 6: https://wetterssource.com/ondemandtoast
- 7: https://forums.ivanti.com/s/article/IDAC-Windows-10-Notifications-are-not-displaying-full-message?language=en_US
- 8: https://www.osdsune.com/home/archive/windows/windows10-toast-notification
- 9: https://www.elevenforum.com/t/how-to-increase-the-notification-limit-in-action-center-beyond-20.28574/
- 10: https://discourse.psappdeploytoolkit.com/t/psadt-toastnotification-extension/4467
- 11: https://github.com/Windos/BurntToast/blob/main/Help/New-BTText.md
Notification Payload Length Constraints
The current implementation correctly encodes your notification payload into a base64 string for the
ActivationArguments. However, please note that Windows toast notifications expect very concise activation arguments. While there isn’t an explicit maximum length documented for the activation arguments, best practices suggest keeping them minimal—generally around or below 64 characters—to prevent any issues with truncation or processing.
- Ensure that if
NotificationOptions.Datacan be large, users are advised (or the code is updated) to keep payloads concise.- Consider adding documentation or runtime validation to warn when the encoded payload might exceed these informal size guidelines.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
v3/examples/notifications/main.go (6)
71-73: Improve error logging pattern.The current error handling pattern uses
println(fmt.Errorf(...)), which is redundant and not idiomatic Go.Use a more standard logging approach:
-println(fmt.Errorf("checking app notification authorization failed: %s", err)) +log.Printf("Checking app notification authorization failed: %s\n", err)Apply this pattern to all error handling blocks in the code for consistency.
Also applies to: 77-79, 84-86, 104-106, 124-126, 140-142
108-109: Add consistent delays between notifications.There's only a delay between the first and second notifications. Adding consistent delays between all notifications would provide a better user experience.
Consider adding delays before sending each notification:
// Delay before sending next notification time.Sleep(time.Second * 2) const CategoryID = "backend-notification-id" err = ns.RegisterNotificationCategory(notifications.NotificationCategory{ // ... category configuration ... }) if err != nil { log.Printf("Creating notification category failed: %s\n", err) } +// Delay before sending next notification +time.Sleep(time.Second * 2) + err = ns.SendNotificationWithActions(notifications.NotificationOptions{ // ... notification configuration ... })
98-102: Extract common data fields to a reusable structure.The notification data structure is duplicated between the two notifications.
Consider extracting the common data fields to a reusable structure:
+// Define common data at the beginning of the goroutine +notificationData := map[string]interface{}{ + "user-id": "user-123", + "message-id": "msg-123", + "timestamp": time.Now().Unix(), +} // Then use it for both notifications err = ns.SendNotification(notifications.NotificationOptions{ ID: "uuid-basic-1", Title: "Notification Title", Subtitle: "Subtitle on macOS and Linux", Body: "Body text of notification.", - Data: map[string]interface{}{ - "user-id": "user-123", - "message-id": "msg-123", - "timestamp": time.Now().Unix(), - }, + Data: notificationData, }) // ... and later ... err = ns.SendNotificationWithActions(notifications.NotificationOptions{ ID: "uuid-with-actions-1", Title: "Actions Notification Title", Subtitle: "Subtitle on macOS and Linux", Body: "Body text of notification with actions.", CategoryID: CategoryID, - Data: map[string]interface{}{ - "user-id": "user-123", - "message-id": "msg-123", - "timestamp": time.Now().Unix(), - }, + Data: notificationData, })Also applies to: 134-138
93-96: Extract notification IDs and titles as constants.Hardcoded string values like notification IDs, titles, and other text elements should be defined as constants for better maintainability.
Consider defining constants for these values:
+// Define constants at the beginning of the file or function +const ( + BasicNotificationID = "uuid-basic-1" + BasicNotificationTitle = "Notification Title" + BasicNotificationSubtitle = "Subtitle on macOS and Linux" + BasicNotificationBody = "Body text of notification." + + ActionsNotificationID = "uuid-with-actions-1" + ActionsNotificationTitle = "Actions Notification Title" + ActionsNotificationSubtitle = "Subtitle on macOS and Linux" + ActionsNotificationBody = "Body text of notification with actions." + + NotificationCategoryID = "backend-notification-id" +) // ... later in the code ... err = ns.SendNotification(notifications.NotificationOptions{ - ID: "uuid-basic-1", - Title: "Notification Title", - Subtitle: "Subtitle on macOS and Linux", + ID: BasicNotificationID, + Title: BasicNotificationTitle, + Subtitle: BasicNotificationSubtitle, // ...Also applies to: 128-131
89-89: Add error handling for EmitEvent call.The EmitEvent call could potentially fail, but there's no error handling.
Consider adding error handling for the EmitEvent call:
-app.EmitEvent("notification:action", result.Response) +err := app.EmitEvent("notification:action", result.Response) +if err != nil { + log.Printf("Failed to emit notification action event: %s\n", err) +}If the EmitEvent method doesn't return an error, disregard this suggestion.
67-144: Separate authorization and notification logic.The authorization check and notification sending are tightly coupled in the same goroutine, making the code less modular and harder to test.
Consider separating the authorization logic from the notification sending:
func checkAndRequestAuthorization(ns *notifications.Service) (bool, error) { authorized, err := ns.CheckNotificationAuthorization() if err != nil { return false, fmt.Errorf("checking app notification authorization failed: %s", err) } if !authorized { authorized, err = ns.RequestNotificationAuthorization() if err != nil { return false, fmt.Errorf("requesting app notification authorization failed: %s", err) } } return authorized, nil } func sendNotifications(app *application.App, ns *notifications.Service) { // Setup notification response handler ns.OnNotificationResponse(func(result notifications.NotificationResult) { // Handle notification response }) // Send notifications // ... } // In main: app.OnApplicationEvent(events.Common.ApplicationStarted, func(event *application.ApplicationEvent) { go func() { authorized, err := checkAndRequestAuthorization(ns) if err != nil { log.Printf("Authorization failed: %v\n", err) return } if authorized { sendNotifications(app, ns) } }() })This separation makes the code more maintainable and easier to test.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/examples/notifications/main.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
v3/examples/notifications/main.go
20-20: pattern all:frontend/dist: no matching files found
(typecheck)
🔇 Additional comments (2)
v3/examples/notifications/main.go (2)
20-21: Fix the embed directive to properly include frontend assets.The static analysis indicates that no matching files were found for the embedded pattern. This is typically due to the frontend assets not being built or not existing at compile time.
Ensure the frontend assets are built before running the application. The directory structure should be:
v3/examples/notifications/ ├── frontend/ │ ├── dist/ │ │ └── ... (built frontend files)Make sure to run the frontend build process first:
cd frontend && npm run build🧰 Tools
🪛 golangci-lint (1.64.8)
20-20: pattern all:frontend/dist: no matching files found
(typecheck)
67-144: Add context cancellation for background goroutine.The goroutine doesn't have a cancellation mechanism, which could lead to resource leaks if the application is terminated while the goroutine is still running.
Consider implementing a cancellation mechanism:
app.OnApplicationEvent(events.Common.ApplicationStarted, func(event *application.ApplicationEvent) { + ctx, cancel := context.WithCancel(context.Background()) + // Cancel the context when the application is about to quit + app.OnApplicationEvent(events.Common.AboutToQuit, func(event *application.ApplicationEvent) { + cancel() + }) // Create a goroutine that spawns desktop notifications from Go go func() { + // Check for cancellation + select { + case <-ctx.Done(): + return + default: + } var authorized bool var err error // Rest of the code... }() })Remember to add
"context"to your imports.
|
To get dev builds working on macOS the example app modifies this cmd. It seems like we could do this for all dev builds on macOS, I don't see a downside. For |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
docs/src/content/docs/changelog.mdx (1)
79-79: Notification Support Changelog EntryThe new changelog entry for adding notification support by [@popaprozac] is clearly indicated. Consider adding a brief note (even a one-liner) summarizing the key features enabled by this change for additional context.
docs/src/content/docs/learn/notifications.mdx (3)
7-10: Introduction Section ClarityThe introduction succinctly explains that Wails offers a cross-platform notification system with interactive capabilities. It might be beneficial to briefly mention that platform-specific behavior (such as authorization on macOS) will be detailed later.
32-48: Notification Authorization SectionThe code snippet effectively demonstrates checking and requesting notification authorization on macOS. In the accompanying explanatory text, consider adding a comma after “Windows and Linux” for readability (e.g. “On Windows and Linux, this always returns
true”).🧰 Tools
🪛 LanguageTool
[uncategorized] ~48-~48: A comma might be missing here.
Context: ...ionAuthorization() } ``` On Windows and Linux this always returnstrue. ## Notific...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
147-188: Platform Considerations DetailsThe use of
<Tabs>and corresponding<TabItem>sections provides a clear comparison of platform behaviors. Note a minor typo in the macOS section: “Require the app to be notorized for distribution” should read “notarized.” Correcting this will improve clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/src/content/docs/changelog.mdx(1 hunks)docs/src/content/docs/learn/notifications.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/content/docs/learn/notifications.mdx
[uncategorized] ~48-~48: A comma might be missing here.
Context: ...ionAuthorization() } ``` On Windows and Linux this always returns true. ## Notific...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🔇 Additional comments (9)
docs/src/content/docs/learn/notifications.mdx (9)
1-3: Front Matter and TitleThe front matter correctly sets the document title as "Notifications" and uses the standard Markdown delimiters. This clear context helps readers immediately understand the document’s purpose.
5-5: Import Statement for UI ComponentsThe import of
TabsandTabItemfrom@astrojs/starlight/componentsis succinct and appropriate for organizing platform-specific content later in the document.
11-30: Basic Usage and Service InitializationThe "Creating the Service" section clearly outlines the initialization process with a well-structured Go code snippet. The registration of the notifications service via
application.NewService(notifier)is clearly demonstrated.
66-103: Interactive Notifications SectionThe interactive notifications example is comprehensive—defining a category, registering it, and then sending a notification with actions. The code is clear and instructive; no issues were found in this segment.
105-125: Notification Responses HandlingThe example for handling notification responses illustrates the callback mechanism clearly, demonstrating how to process user actions and even emit events to the frontend. If desired, you might add a brief remark about handling potential errors within the callback.
189-205: Best Practices SectionThis section clearly outlines the steps users should follow when implementing notifications. The bullet points are succinct and provide valuable guidance. No changes are required here.
205-211: Examples SectionIncluding a direct link to a live [Notifications] example helps readers easily explore practical usage. Ensure that the link remains up-to-date with project structure changes.
211-244: API Reference OverviewThe API Reference tables and accompanying code blocks are detailed and well-organized. They offer clear method descriptions and type definitions, which will be very useful for developers integrating the notification system.
251-304: Structs and Types DefinitionsThe provided Go type definitions for
NotificationOptions,NotificationCategory,NotificationAction,NotificationResponse, andNotificationResultare formatted according to Go conventions and clearly annotated with comments. This will serve as a valuable reference for developers.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/src/content/docs/learn/notifications.mdx (1)
127-145:⚠️ Potential issueCritical Typo and Syntax Issue in Custom Metadata Example
There are two issues in this code snippet:
- The type name is misspelled as
NotificaionOptions(line 134) instead ofNotificationOptions. This typo duplicates a previously noted issue.- The composite literal is missing a comma after the
Bodyfield (after line 138), which is required in Go for multi-line literals.Please correct these to ensure the code compiles correctly.
🧹 Nitpick comments (5)
docs/src/content/docs/learn/notifications.mdx (5)
15-30: Service Initialization Snippet is ClearThe Go code snippet demonstrating initialization of the notification service is clear and concise.
Suggestion: Add a space after the comment marker on line 24 (e.g. change//Registerto// Register) to improve readability.
32-48: Notification Authorization Handling is SolidThe example in this section properly demonstrates checking and requesting authorization for notifications on macOS.
Note: In the explanatory text on line 48, consider adding a comma—“On Windows and Linux, this always returnstrue”—to enhance clarity.🧰 Tools
🪛 LanguageTool
[uncategorized] ~48-~48: A comma might be missing here.
Context: ...ionAuthorization() } ``` On Windows and Linux this always returnstrue. ## Notific...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
66-68: Typo in Introductory Text for Interactive NotificationsThe description on line 66 contains a typo: “resgistered” should be corrected to “registered”.
105-125: Notification Response Handling is Appropriately DemonstratedThe snippet for processing user interactions shows effective use of callbacks and event emission. If feasible, consider logging errors within the callback for even better observability.
146-160: Typo in macOS Platform ConsiderationsWithin the macOS tab (lines 151–160), the bullet point “Require the app to notorized for distribution” (line 154) should use “notarized” instead of “notorized”.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/src/content/docs/learn/notifications.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/content/docs/learn/notifications.mdx
[uncategorized] ~48-~48: A comma might be missing here.
Context: ...ionAuthorization() } ``` On Windows and Linux this always returns true. ## Notific...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🔇 Additional comments (6)
docs/src/content/docs/learn/notifications.mdx (6)
1-3: Front Matter & Title Are Well-FormedThe front matter (lines 1–3) and title declaration establish the context clearly.
5-6: Import Statement is CorrectThe import of
{ Tabs, TabItem }from the Starlight components is correctly placed for later use in the Tabs section.
56-64: Basic Notification Example Looks GoodThe snippet for sending a basic notification is succinct and aligns with the documented API.
69-104: Interactive Notification Setup is Well IllustratedThis code snippet clearly demonstrates how to define a notification category, register it, and then send an interactive notification.
188-204: Best Practices Section is Clear and InformativeThe best practices listed are thorough and provide useful guidance for users working with notifications.
210-304: API Reference and Struct Documentation are ComprehensiveThe API Reference tables and the accompanying Go struct definitions are complete and well-organized, offering clear insight into the available methods and data types.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
docs/src/content/docs/learn/notifications.mdx (6)
11-31: Basic Usage: Service Initialization Example
The example demonstrating how to initialize the notifications service and register it with the application is clear and well-structured.
- Nitpick: On line 24, consider adding a space in the comment (i.e. change "
//Register the service with the application" to "// Register the service with the application") for improved readability.
32-48: Notification Authorization: Code Snippet Clarity
The snippet correctly shows checking for notification authorization and then requesting it if not already granted.
- In a production scenario, it might be useful to also handle the error returned from
RequestNotificationAuthorization().🧰 Tools
🪛 LanguageTool
[uncategorized] ~48-~48: A comma might be missing here.
Context: ...ionAuthorization() } ``` On Windows and Linux this always returnstrue. ## Notific...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
48-48: Punctuation Suggestion
For clarity, consider adding a comma: change
On Windows and Linux this always returnstrue.
to
On Windows and Linux, this always returnstrue.🧰 Tools
🪛 LanguageTool
[uncategorized] ~48-~48: A comma might be missing here.
Context: ...ionAuthorization() } ``` On Windows and Linux this always returnstrue. ## Notific...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
66-104: Interactive Notifications: Clear and Detailed Example
This example comprehensively demonstrates:
- Defining a notification category with actions and reply options.
- Registering the category.
- Sending an interactive notification.
- Nitpick: There is a minor typo in the introductory sentence on line 66 ("resgistered" should be "registered").
105-125: Notification Responses: Callback Handling
The snippet effectively demonstrates how to register a callback for handling notification responses.
- It might help users if you note that necessary packages (e.g.,
fmt) should be imported in a complete example.
147-156: Platform Considerations: Typo Correction
Within the macOS tab (line 155), "notorized" should be corrected to "notarized" to reflect the proper spelling required for app distribution.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/src/content/docs/learn/notifications.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/content/docs/learn/notifications.mdx
[uncategorized] ~48-~48: A comma might be missing here.
Context: ...ionAuthorization() } ``` On Windows and Linux this always returns true. ## Notific...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🔇 Additional comments (9)
docs/src/content/docs/learn/notifications.mdx (9)
1-3: Front Matter Section: Confirmed Title
The front matter is correctly formatted with a proper title declaration. Ensure consistency with similar documentation files.
5-5: Import Statement Check
The import from@astrojs/starlight/componentsis appropriate for using the Tabs and TabItem components in this MDX file.
7-10: Introduction Section: Clear Overview
The introduction provides a concise explanation of the notifications service. The language is clear and informative.
50-64: Basic Notifications Example: Good Demonstration
The basic notifications snippet clearly outlines how to send a notification with the required fields (ID, Title, Subtitle, and Body).
157-188: Platform Considerations: Clear and Organized Layout
The use of Tabs and TabItem components provides a well-organized overview of platform-specific differences. The content is clear and informative.
189-204: Best Practices Section: Comprehensive Guidelines
The section on best practices is detailed and provides actionable guidelines regarding authorization and notification design.
205-211: Examples Section: Useful Call-to-Action
The examples section effectively encourages users to explore further by linking to a practical example.
212-249: API Reference Sections: Well Structured Documentation
The API reference is comprehensive, presenting methods and their descriptions in a clear table format. This will help users quickly understand service management, notification authorization, sending notifications, managing categories, and handling events.
250-304: API Reference: Struct Definitions
The Go code snippets detailing the structs (NotificationOptions, NotificationCategory, NotificationAction, NotificationResponse, and NotificationResult) are clear and well documented.
Ensure that the formatting and indentation remain consistent in the final rendered documentation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/src/content/docs/learn/notifications.mdx (3)
48-48: Punctuation Improvement in Platform Note
Consider adding a comma for improved readability. For example, modify:
On Windows and Linux this always returnstrue
to:
On Windows and Linux, this always returnstrue🧰 Tools
🪛 LanguageTool
[uncategorized] ~48-~48: A comma might be missing here.
Context: ...ionAuthorization() } ``` On Windows and Linux this always returnstrue. ## Notific...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
65-66: Typo in Notification Description
There is a typo in the sentence "These notifications require a notification category to be resgistered first:" – please change "resgistered" to "registered".-... require a notification category to be resgistered first: +... require a notification category to be registered first:
146-158: Correct Spelling in macOS Notification Requirements
In the macOS tab under "Platform Considerations," the bullet point "Require the app to be notorized for distribution" contains a spelling error. Consider changing "notorized" to "notarized".- - Require the app to be notorized for distribution + - Require the app to be notarized for distribution
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/src/content/docs/learn/notifications.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/content/docs/learn/notifications.mdx
[uncategorized] ~48-~48: A comma might be missing here.
Context: ...ionAuthorization() } ``` On Windows and Linux this always returns true. ## Notific...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🔇 Additional comments (1)
docs/src/content/docs/learn/notifications.mdx (1)
1-301: Overall Documentation Quality
The documentation file is well structured and thorough. It explains the initialization, usage, and API reference for the notifications service in detail, covering both basic and interactive notifications as well as platform-specific considerations. Great work on providing comprehensive examples and a clear API reference!🧰 Tools
🪛 LanguageTool
[uncategorized] ~48-~48: A comma might be missing here.
Context: ...ionAuthorization() } ``` On Windows and Linux this always returnstrue. ## Notific...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
There was a problem hiding this comment.
Maybe this moves to pkg/w32/icon.go?
There was a problem hiding this comment.
Yeah, I think so. There's already very similar code there.
| package notifications | ||
|
|
||
| /* | ||
| #cgo CFLAGS: -mmacosx-version-min=11.0 -x objective-c |
There was a problem hiding this comment.
I think this will be problematic because it will fail to compile on systems < 11.
I believe the right approach is to do something like this:
/*
#cgo CFLAGS: -x objective-c
#cgo LDFLAGS: -framework Foundation -framework UserNotifications
#import <Foundation/Foundation.h>
#if __MAC_OS_X_VERSION_MAX_ALLOWED >= 110000
#import <UserNotifications/UserNotifications.h>
#endif
BOOL canSendNotifications() {
// Using @available for runtime check
if (@available(macOS 11.0, *)) {
return YES;
} else {
return NO;
}
}
BOOL sendNotificationIfAvailable(const char* title, const char* body) {
if (@available(macOS 11.0, *)) {
// Implementation using UserNotifications framework
UNUserNotificationCenter *center = [UNUserNotificationCenter currentNotificationCenter];
UNMutableNotificationContent *content = [[UNMutableNotificationContent alloc] init];
content.title = [NSString stringWithUTF8String:title];
content.body = [NSString stringWithUTF8String:body];
UNNotificationRequest *request = [UNNotificationRequest requestWithIdentifier:[[NSUUID UUID] UUIDString]
content:content
trigger:nil];
[center addNotificationRequest:request withCompletionHandler:nil];
return YES;
} else {
// Not available on this OS version
NSLog(@"Notification API requires macOS 11.0 or later");
return NO;
}
}
*/
We can then do things like:
// IsNotificationAvailable checks if notifications are available on this system
func IsNotificationAvailable() bool {
return bool(C.canSendNotifications())
}and give the developer the option of what to do if they are not available.
| func New() *Service { | ||
| notificationServiceOnce.Do(func() { | ||
| if !CheckBundleIdentifier() { | ||
| panic("\nError: Cannot use the notification API in development mode on macOS.\n" + |
There was a problem hiding this comment.
I think it's better if we return this as an error rather than panic. A corrupt bundle at runtime may cause it so the developer may want to handle it as a crash and save some information or show a dialog or something.
There was a problem hiding this comment.
Makes sense and, related to checking compatibility and return an err, do we want to return on calling New or in ServiceStartup? I don't really see a precedent for return (*Service, error)
| return | ||
| } | ||
|
|
||
| dn, ok := ns.impl.(*darwinNotifier) |
There was a problem hiding this comment.
What do you think about having the impl as an interface? That way you wouldn't need to type cast it like this. Just a thought.
There was a problem hiding this comment.
Yeah makes sense. My brain is still conforming to Go norms.
type ChannelHandler interface {
GetChannel(id int) (chan notificationChannel, bool)
}
Something like that so I can handler, ok := ns.impl.(ChannelHandler)?
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
v3/pkg/w32/icon.go (1)
83-84:⚠️ Potential issuePossible metrics mismatch.
Line 84 usesGetSystemMetrics(SM_CXICON)for both width and height. Typically, icon height usesSM_CYICON. If that’s intentional, clarify why; otherwise, useSM_CYICONfor the second call.- iconHeight := GetSystemMetrics(SM_CXICON) + iconHeight := GetSystemMetrics(SM_CYICON)
🧹 Nitpick comments (11)
v3/pkg/services/notifications/notifications_darwin.go (3)
63-71: Validate macOS version requirements upon Startup.
Currently,Startupchecks if notifications are available and if a valid bundle identifier exists. Consider explicitly verifying macOS ≥ 11.0 here (using@availableor a version check) to provide earlier feedback if the OS is unsupported.
86-103: Add contextual logging when user denies authorization.
InRequestNotificationAuthorization, if a user denies permission, there's no console log or user-facing message that clarifies the denial, only a boolean value incaptureResult. Logging (or at least a debug message) could help developers troubleshoot.
124-163: Consider making timeout configurable for SendNotification.
A hardcoded 5-second timeout might be too short in some environments or scenarios. Allowing an optional parameter or a default from configuration can enhance flexibility.v3/pkg/services/notifications/notifications_darwin.h (2)
8-9: Document the bundle identifier requirement more explicitly.
ThecheckBundleIdentifierfunction is crucial for valid macOS notifications. Consider adding a doc comment clarifying that an unsigned or invalid bundle identifier prevents notifications.
13-13: Clarify handling of actions_json in sendNotificationWithActions.
In the header, consider expanding the comments to explain what is expected inactions_jsonso implementers know the required structure (e.g., array of action objects) and potential error cases.v3/pkg/services/notifications/notifications_darwin.m (3)
9-15: Support older macOS versions gracefully.
isNotificationAvailablereturnsNOfor macOS < 11.0 but does not indicate possible fallback solutions (e.g., older notification APIs). If partial support is possible, consider returning an error or performing a fallback.
52-87: Implement debug logs for notification response data.
IndidReceiveNotificationResponse, the code properly marshals data and invokesdidReceiveNotificationResponse(...). Adding debug logs for successful unmarshals can aid troubleshooting.
144-173: Add optional logging of JSON parsing errors in createNotificationContent.
Currently, only the calling function captures or reports the JSON error. Logging it here as well might expedite debugging.v3/pkg/services/notifications/notifications_windows.go (2)
22-28: Consider embedding the categories field within a higher-level Service struct.
By embeddingcategoriesinwindowsNotifier, you limit category states to Windows only. If your design is for each OS-specific struct to manage its own categories, that’s fine—but consider whether you want truly platform-agnostic code by shifting shared logic (like category registration) to a commonServicestruct. This can reduce duplication and keep platform code smaller.
205-219: Prevent repeated payload encoding logic from growing complex.
While the duplication for default and action-based arguments is manageable now, it may expand if new features (e.g., icons, images, metadata) are introduced. Consider extracting the repeated encoding to a small helper method to keep your code DRY.v3/pkg/w32/icon.go (1)
200-209: Add logging or error reporting for PNG encoding failures.
This block directly returnspng.Encodeerrors. Consider logging them if you expect end users to debug potential file system or permission issues.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
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)v3/pkg/w32/icon.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
v3/pkg/services/notifications/notifications_windows.go (3)
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_windows.go:91-113
Timestamp: 2025-03-26T20:18:16.040Z
Learning: The JSON quote handling in Windows notifications' activation arguments (v3/pkg/services/notifications/notifications_windows.go) has a known limitation with single-quote collisions that needs to be addressed after initial testing.
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_windows.go:113-164
Timestamp: 2025-03-26T20:18:16.040Z
Learning: In Windows Toast notifications (notifications_windows.go), metadata must be appended to each action's Arguments because the toast activated callback only returns the Arguments string of the clicked action. This is achieved by appending the same escaped JSON data (with single quotes) to all actions using the ":::" delimiter.
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_windows.go:0-0
Timestamp: 2025-03-26T20:18:16.040Z
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.
v3/pkg/services/notifications/notifications_darwin.go (2)
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-26T20:18:16.040Z
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
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_darwin.go:39-46
Timestamp: 2025-03-26T20:18:16.040Z
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.
🧬 Code Definitions (1)
v3/pkg/services/notifications/notifications_darwin.go (1)
v3/pkg/services/notifications/notifications.go (7)
Service(47-55)NotificationService(59-59)NotificationOptions(80-87)NotificationCategory(71-77)NotificationResult(105-108)NotificationResponse(92-101)DefaultActionIdentifier(89-89)
🔇 Additional comments (5)
v3/pkg/services/notifications/notifications_darwin.go (1)
417-425: Ensure consistent channel cleanup semantics.
cleanupChannelsafely closes the channel. However, ensure that all early returns in preceding methods (likeSendNotification) also invoke cleanup if they create the channel prior to the error. This avoids any chance of leaving channels stranded.Do you want a script to check all paths that create channels and confirm corresponding cleanup calls on errors?
v3/pkg/services/notifications/notifications_darwin.m (1)
94-108: Ensure thread-safety during delegate initialization.
dispatch_onceensures one-time initialization fordelegateInstance. This is safe in this method alone, but verify that no other code path overwritesdelegateInstance.If you suspect concurrency issues, let me know, and I can provide a shell script to scan for any reassignments.
v3/pkg/services/notifications/notifications_windows.go (2)
333-354: Guard against concurrent registry writes across other threads.
Although the docstring says “We assume lock is held by caller,” ensure all writes to the categories registry are guaranteed single-threaded across your application, or a concurrency bug might occur. If you plan on concurrency, consider additional synchronization or a single writer pattern.
80-93:✅ Verification successful
Confirm Windows Toast argument size limits.
When metadata is heavily encoded, Windows Toast may reject large payloads. Ensure you won't exceed argument size limitations, especially for complex notifications.Please run a web search to confirm the maximum size for Windows Toast arguments:
🌐 Web query:
Maximum payload size for Windows 10/11 Toast notification arguments💡 Result:
The maximum payload size for arguments in Windows 10/11 toast notifications is 4 KB (4096 bytes). This limit applies to the entire XML payload structure containing arguments, headers, text, and other elements[13][4].
Key technical constraints:
- Notification payloads exceeding 4KB will fail with "Invalid Payload" errors[13]
- Image URLs in payloads have separate size restrictions (up to 3MB for remote images)[4]
- Character limits apply to specific elements like group IDs (originally 16 characters, later increased to 64)[11]
When designing notifications:
- Minimize XML overhead: Use concise tags and attribute names
- Prioritize critical data: Arguments should contain only essential parameters
- Use compression for large datasets (Base64 encode binary data when necessary)[6]
- Test payload sizes programmatically with PowerShell validation:
$PayloadSize = (Get-Content notification.xml -Raw).Length if ($PayloadSize -gt 4096) { Write-Error "Payload exceeds 4KB limit" }For deployments requiring larger data transfers, implement secondary mechanisms like storing data in registry keys or text files, then reference them via lightweight arguments in the toast[3][6].
Citations:
- 1: https://www.osdsune.com/home/archive/windows/windows10-toast-notification
- 2: https://www.youtube.com/watch?v=9cdEIX-L4PI
- 3: https://github.com/imabdk/Toast-Notification-Script/blob/master/New-ToastNotification.ps1
- 4: https://learn.microsoft.com/en-us/windows/apps/design/shell/tiles-and-notifications/adaptive-interactive-toasts
- 5: https://www.elevenforum.com/t/how-to-increase-the-notification-limit-in-action-center-beyond-20.28574/
- 6: https://smbtothecloud.com/deploy-custom-toast-notifications-with-intune-how-to-run-scripts-from-the-action-buttons-part-1/
- 7: https://www.youtube.com/watch?v=dfbe5Jp40tA
- 8: https://wetterssource.com/ondemandtoast
- 9: https://learn.microsoft.com/en-us/windows/apps/design/shell/tiles-and-notifications/toast-schema
- 10: https://www.imab.dk/windows-10-toast-notification-script/
- 11: https://learn.microsoft.com/en-us/uwp/api/windows.ui.notifications.toastnotification.group?view=winrt-26100
- 12: https://smsagent.blog/2018/06/15/using-windows-10-toast-notifications-with-configmgr-application-deployments/
- 13: https://learn.microsoft.com/en-us/answers/questions/1431719/azure-notification-hub-toast-notification-invalid
- 14: https://forums.ivanti.com/s/article/IDAC-Windows-10-Notifications-are-not-displaying-full-message?language=en_US
- 15: https://www.mdpi.com/2673-6756/2/1/7
- 16: https://github.com/GitHub30/toast-notification-examples/blob/main/README.md
- 17: https://developer.apple.com/documentation/usernotifications/generating-a-remote-notification
- 18: https://byteben.com/bb/deploy-service-announcement-toast-notifications-in-windows-10-with-memcm/
Windows Toast Payload Size Confirmation:
The maximum payload size for Windows 10/11 Toast notification arguments is confirmed to be 4 KB (4096 bytes)—this includes the entire XML structure (arguments, headers, text, etc.). When using heavy metadata or complex notifications, ensure that the generated payload does not exceed this limit to avoid "Invalid Payload" errors. Consider verifying that your implementation (especially aroundparseNotificationResponseand the data passed totoast.SetActivationCallback) keeps the total payload size safely under 4 KB, or implement appropriate trimming or compression as needed.File:
v3/pkg/services/notifications/notifications_windows.go(Lines 80–93)v3/pkg/w32/icon.go (1)
104-129: Check for alpha channel masking in partial transparency icons.
When saving icons with partially transparent regions, the final PNG might display unexpected backgrounds if the mask and color bitmaps differ. Test with icons that have alpha transparency to confirm correctness.
| //export didReceiveNotificationResponse | ||
| func didReceiveNotificationResponse(jsonPayload *C.char, err *C.char) { | ||
| result := NotificationResult{} | ||
|
|
||
| if err != nil { | ||
| errMsg := C.GoString(err) | ||
| result.Error = fmt.Errorf("notification response error: %s", errMsg) | ||
| if ns := getNotificationService(); ns != nil { | ||
| ns.handleNotificationResult(result) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if jsonPayload == nil { | ||
| result.Error = fmt.Errorf("received nil JSON payload in notification response") | ||
| if ns := getNotificationService(); ns != nil { | ||
| ns.handleNotificationResult(result) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| payload := C.GoString(jsonPayload) | ||
|
|
||
| var response NotificationResponse | ||
| if err := json.Unmarshal([]byte(payload), &response); err != nil { | ||
| result.Error = fmt.Errorf("failed to unmarshal notification response: %w", err) | ||
| if ns := getNotificationService(); ns != nil { | ||
| ns.handleNotificationResult(result) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if response.ActionIdentifier == AppleDefaultActionIdentifier { | ||
| response.ActionIdentifier = DefaultActionIdentifier | ||
| } | ||
|
|
||
| result.Response = response | ||
| if ns := getNotificationService(); ns != nil { | ||
| ns.handleNotificationResult(result) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Surface partial payload data on JSON unmarshal failure.
didReceiveNotificationResponse discards all response data if JSON unmarshaling fails. If partial data is still valuable for debugging or fallback, consider returning or logging it instead of discarding everything.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
v3/pkg/services/notifications/notifications_darwin.go (4)
121-122: Document the default 5-second timeout.It's helpful to mention the default 5-second request timeout in the doc comment, similar to the other methods which list their default timeouts:
-// SendNotification sends a basic notification with a unique identifier, title, subtitle, and body. +// SendNotification sends a basic notification with a unique identifier, title, subtitle, and body. +// Defaults to a 5-second timeout for obtaining the system’s response or an error is returned.
163-168: Document the default 5-second timeout for action notifications.Similar to
SendNotification, highlight the default 5-second timeout for calls that include actions:-// SendNotificationWithActions sends a notification with additional actions and inputs. +// SendNotificationWithActions sends a notification with additional actions and inputs. +// Defaults to a 5-second timeout awaiting system confirmation.
278-304: Consider adding asynchronous callbacks for remove commands.Currently, remove methods (e.g.,
RemoveAllPendingNotifications) do not return OS errors or confirmations. Consider adding a channel-based approach similar to category registration to accurately capture success or any potential errors from the system.
404-413: Potential concurrency improvement using sync.RWMutex.If read operations on the channels outnumber writes, consider using a
sync.RWMutexto improve concurrency performance. This is optional but can help scale read-heavy workloads.
📜 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 context used
🧠 Learnings (1)
v3/pkg/services/notifications/notifications_darwin.go (2)
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-26T20:18:16.040Z
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
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_darwin.go:39-46
Timestamp: 2025-03-26T20:18:16.040Z
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.
🔇 Additional comments (2)
v3/pkg/services/notifications/notifications_darwin.go (2)
61-69: Well done handling invalid bundle identifiers gracefully.
347-387: Surface partial payload data on unmarshal failure.This suggestion was raised in a past review. If partial data is valuable for diagnostics, consider returning or logging what was successfully parsed before the unmarshal error occurred.
Description
Adds basic notification support to macOS via UNNotification API, Windows via go-toast, and currently testing on Linux via dbus.
Fixes (#1788)
Notifications.mov
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:
Taking inspiration from prior attempt here. Would love some help!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 forCheckNotificationAuthorization.Might just reuseRequestUserNotificationAuthorizationif I can.CheckBundleIdentifier,RequestUserNotificationAuthorization, andCheckNotificationAuthorizationalways returnstrue.CheckBundleIdentifier,RequestUserNotificationAuthorization, andCheckNotificationAuthorizationalways 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.*Identifier should be unique and can be used by other methods to update or remove a specific notification.
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.
RemoveNotificationCategoryis there if needed.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.
On service start and shutdown the categories json is updated.
Inputs are not supported.
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.RemoveNotificationremoves a single notification by id, matching the identifier.RemoveAllPendingNotifications,RemovePendingNotification,RemoveAllDeliveredNotifications, andRemoveDeliveredNotificationare no-ops.userInfo.ActivationArguments.For this to work we have to add the encoded data to each action for it to be returned when any action is taken.
OnNotificationResponsetakes a callback thats fires when a notification is interacted with.Events must be used to forward the response to the frontend.
OnNotificationResponsetakes a callback thats fires when a notification is interacted with.Events must be used to forward the response to the frontend.
OnNotificationResponsetakes a callback thats fires when a notification is interacted with.Events must be used to forward the response to the frontend.
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
New Features
Documentation
Chores