-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[v3] Notifications API #4098
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[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
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Nitpick comments (17)
v3/pkg/services/notifications/notifications_windows.go (4)
1-18
: Consider concurrency safety for the globalNotificationCategories
map.
Because this is a shared global map, concurrent goroutines that register or remove notification categories might cause data races. You may want to protectNotificationCategories
with 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_json
is sizable, synchronous parsing and creation ofUNNotificationAction
objects 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
NotificationOptions
struct 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.ttf
is excluded by!**/dist/**
,!**/*.ttf
v3/examples/notifications/frontend/dist/assets/index-CS0sRFez.js
is excluded by!**/dist/**
v3/examples/notifications/frontend/dist/index.html
is excluded by!**/dist/**
v3/examples/notifications/frontend/dist/javascript.svg
is excluded by!**/dist/**
,!**/*.svg
v3/examples/notifications/frontend/dist/style.css
is excluded by!**/dist/**
v3/examples/notifications/frontend/dist/wails.png
is excluded by!**/dist/**
,!**/*.png
v3/examples/notifications/frontend/public/Inter-Medium.ttf
is excluded by!**/*.ttf
v3/examples/notifications/frontend/public/javascript.svg
is excluded by!**/*.svg
v3/examples/notifications/frontend/public/wails.png
is excluded by!**/*.png
v3/go.sum
is 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.Unmarshal
fails, 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 (likeidentifier
oractionIdentifier
) 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
sendNotification
function 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
v3/pkg/services/notifications/notifications_windows.go (3)
18-18
: Consider concurrency-safe access to the globalNotificationCategories
map.
Multiple methods in this file read from and write to the globalNotificationCategories
map, 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 theService
struct 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_subtitle
for 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
:ServiceName
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
setNotificationCategories
call 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
CheckBundleIdentifier
fails, 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 == nil
during 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
, orremoveNotificationCategory
do 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.14
in 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
checkBundleIdentifier
andrequestUserNotificationAuthorization
return abool
. If you need more detailed error reporting, consider using an extended structure or separate error signaling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ctx
andoptions
appear 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 theData
field.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.SetActivationCallback
is triggered,application.Get().EmitEvent
is 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 windows
ensures 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 whenCategoryID
is unregistered.If
options.CategoryID
has 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
v3/pkg/services/notifications/notifications_windows.go (6)
25-27
: Consider passing a map reference to theService
struct.
WhileNew()
returns a pointer to an emptyService
, it may be more robust to storeNotificationCategories
within 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.CategoryID
isn’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.
loadCategoriesFromRegistry
properly checks forErrNotExist
but 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 globalNotificationCategories
map 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 theService
struct 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 aroundNotificationCategories
to guard reads and writes if the application may register/remove categories from multiple goroutines.
225-248
: Verify registry write success.
saveCategoriesToRegistry
handles 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
$0
is not descriptive. Consider using a more meaningful name likeidentifier
to 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
$0
should be renamed toidentifier
for 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Tabs
andTabItem
from@astrojs/starlight/components
is 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
, andNotificationResult
are 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Body
field (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//Register
to// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 returns
true.
to
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)
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/components
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this moves to pkg/w32/icon.go
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_CYICON
for 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,Startup
checks if notifications are available and if a valid bundle identifier exists. Consider explicitly verifying macOS ≥ 11.0 here (using@available
or 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.
ThecheckBundleIdentifier
function 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_json
so 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.
isNotificationAvailable
returnsNO
for 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 embeddingcategories
inwindowsNotifier
, 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 commonService
struct. 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.Encode
errors. 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.
cleanupChannel
safely 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_once
ensures 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 aroundparseNotificationResponse
and 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.RWMutex
to 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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
windows下系统通知依赖go-toast,但是我使用遇到问题,我在项目中使用了 uiautomation,然而go-toast中的initialize方法中使用了ole.RoInitialize(1),会和 ole.CoInitializeEx 冲突导致uiautomation无法正常使用。 我尝试修复,对 git.sr.ht~jackmordaunt\go-toast\[email protected]\wintoast\bind_windows.go 文件修改initialize方法及调用逻辑:
|
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:
ActivationType
options?Known changes before:
NotificationResponseData
AppData
configLimitations/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-toastAppData
configLinux:
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
CheckBundleIdentifier
since original source for this implementation logged from Obj-C. No need, we can handle in Go.CheckBundleIdentifier
,RequestUserNotificationAuthorization
, andCheckNotificationAuthorization
are available on macOS.I want to do more testing since I had to hack together a timeout to wait forCheckNotificationAuthorization
.Might just reuseRequestUserNotificationAuthorization
if I can.CheckBundleIdentifier
,RequestUserNotificationAuthorization
, andCheckNotificationAuthorization
always returnstrue
.CheckBundleIdentifier
,RequestUserNotificationAuthorization
, andCheckNotificationAuthorization
always returnstrue
.*Identifier should be unique and can be used by other methods to update or remove a specific notification.
Destructive
action option is available on macOS.*Identifier is the toast's "AppID" and presented like a notification name.
Destructive
action option is not available.*Identifier should be unique and can be used by other methods to update or remove a specific notification.
Destructive
action 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
UNUserNotificationCenter
API.Registering needs to only occur once per unique category ID. Reregistering overrides the previous config.
RemoveNotificationCategory
is 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.
UNUserNotificationCenter
API provides a ton of features including removing pending and delivered notifications.RemoveAllPendingNotifications
,RemovePendingNotification
,RemoveAllDeliveredNotifications
, andRemoveDeliveredNotification
are available on macOS.RemoveAllPendingNotifications
,RemovePendingNotification
,RemoveAllDeliveredNotifications
, andRemoveDeliveredNotification
are no-ops.RemoveNotification
removes a single notification by id, matching the identifier.RemoveAllPendingNotifications
,RemovePendingNotification
,RemoveAllDeliveredNotifications
, andRemoveDeliveredNotification
are 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.
OnNotificationResponse
takes a callback thats fires when a notification is interacted with.Events must be used to forward the response to the frontend.
OnNotificationResponse
takes a callback thats fires when a notification is interacted with.Events must be used to forward the response to the frontend.
OnNotificationResponse
takes a callback thats fires when a notification is interacted with.Events must be used to forward the response to the frontend.
Notifications via
UNNotification
API 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.mdx
with details of this PRSummary by CodeRabbit
New Features
Documentation
Chores