Skip to content

[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

Merged
merged 101 commits into from
Apr 13, 2025
Merged

Conversation

popaprozac
Copy link

@popaprozac popaprozac commented Feb 23, 2025

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:

  • Basic notifications
  • Notification actions
  • Notification interaction events
  • JS API

Future features:

  • Sounds
  • Icons/images/attachments
  • Notification priority
  • Windows Toast ActivationType options?
  • Respond to notification events when app is closed (is this possible?)

Known changes before:

  • Refactor/improve error handling
  • Refactor/improve NotificationResponseData
  • go-toast AppData config
  • Linux support?
  • Add optional metadata to basic notifications (macOS)

Limitations/Notes/Thoughts/Questions:

macOS:

  • Your app must have a bundle identifier and be signed for the notification API to work! I try to call this out everywhere here and in the code but this will make the dev experience tougher. Open to suggestions.
  • Minimum version supported macOS 11.0

Windows:

  • To match macOS's ability to add arbitrary data to notifications I am encoding that data and adding it to the button "arguments" and decoding it back out when interacted with. Don't love it but works.
  • 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-toast AppData config

Linux:

  • Tested on Fedora, Ubuntu, and Debian. More importantly is testing desktop environments. Tested on GNOME, MATE, LXDE, Cinnamon, Plasma...when I get the energy I will outline the minor differences but all work!
  • Taking inspiration from prior attempt here. Would love some help!
  • Look at shout to better understand what this will take, would love help

Platform differences:

Feature macOS Windows Linux
Permissions macOS requires explicit user permission to send notifications from an application.
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, and CheckNotificationAuthorization are available on macOS.

I want to do more testing since I had to hack together a timeout to wait for CheckNotificationAuthorization.
Might just reuse RequestUserNotificationAuthorization if I can.
CheckBundleIdentifier, RequestUserNotificationAuthorization, and CheckNotificationAuthorization always returns true. CheckBundleIdentifier, RequestUserNotificationAuthorization, and CheckNotificationAuthorization always returns true.
Notifications Supports:
  • *Identifier
  • Title
  • Subtitle
  • Body
  • Actions
  • Text Input
  • Custom data

*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.
Supports:
  • *Identifier
  • Title
  • Subtitle
  • Body
  • Actions
  • Text Input
  • Custom data

*Identifier is the toast's "AppID" and presented like a notification name.
Destructive action option is not available.
Supports:
  • *Identifier
  • Title
  • Subtitle
  • Body
  • Actions
  • Text Input
  • Custom data

*Identifier should be unique and can be used by other methods to update or remove a specific notification.
Destructive action option is not available.
Notification Categories For more complex notifications you can register a notification category with specified actions and input options.

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.
Implemented matching API that registers notification categories in the Registry on Windows.

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.
Implemented matching API that registers notification categories the user config dir (json).

On service start and shutdown the categories json is updated.

Inputs are not supported.
Removing Notifications Leveraging the UNUserNotificationCenter API provides a ton of features including removing pending and delivered notifications.

RemoveAllPendingNotifications, RemovePendingNotification, RemoveAllDeliveredNotifications, and RemoveDeliveredNotification are available on macOS.
RemoveAllPendingNotifications, RemovePendingNotification, RemoveAllDeliveredNotifications, and RemoveDeliveredNotification are no-ops. RemoveNotification removes a single notification by id, matching the identifier.

RemoveAllPendingNotifications, RemovePendingNotification, RemoveAllDeliveredNotifications, and RemoveDeliveredNotification are no-ops.
Notification Metadata macOS allows custom data to be sent with the notification as userInfo. Implemented a matching API by encoding and decoding the data, appending it to the one field we get back from the notification 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.
Implemented matching API that adds a dbus variant with the data.
Notification Callback 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.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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!

  • Windows
  • macOS
  • Linux
    • Debian 12 with GNOME, Cinnamon, KDE, and more

If you checked Linux, please specify the distro and version.

Test Configuration

# System

┌──────────────────────────────────────────────────┐
| Name          | MacOS                            |
| Version       | 15.3                             |
| ID            | 24D2059                          |
| Branding      | Sequoia                          |
| Platform      | darwin                           |
| Architecture  | arm64                            |
| Apple Silicon | true                             |
| CPU           | Apple M4 Max                     |
| CPU 1         | Apple M4 Max                     |
| CPU 2         | Apple M4 Max                     |
| GPU           | 32 cores, Metal Support: Metal 3 |
| Memory        | 36 GB                            |
└──────────────────────────────────────────────────┘

# Build Environment

┌─────────────────────────────────────────────────────────┐
| Wails CLI    | v3.0.0-dev                               |
| Go Version   | go1.24.1                                 |
| Revision     | 2cb0dac6484a571594931ba6eb3d198095e0436d |
| Modified     | true                                     |
| -buildmode   | exe                                      |
| -compiler    | gc                                       |
| CGO_CFLAGS   |                                          |
| CGO_CPPFLAGS |                                          |
| CGO_CXXFLAGS |                                          |
| CGO_ENABLED  | 1                                        |
| CGO_LDFLAGS  |                                          |
| GOARCH       | arm64                                    |
| GOARM64      | v8.0                                     |
| GOOS         | darwin                                   |
| vcs          | git                                      |
| vcs.modified | true                                     |
| vcs.revision | 2cb0dac6484a571594931ba6eb3d198095e0436d |
| vcs.time     | 2025-03-15T02:13:58Z                     |
└─────────────────────────────────────────────────────────┘

# Dependencies

┌────────────────────────────────────────────────────────────────────────┐
| Xcode cli tools | 2409                                                 |
| npm             | 10.9.2                                               |
| *NSIS           | Not Installed. Install with `brew install makensis`. |
|                                                                        |
└─────────────────────── * - Optional Dependency ────────────────────────┘

# Checking for issues

 SUCCESS  No issues found

# Diagnosis

 SUCCESS  Your system is ready for Wails development!

Checklist:

  • I have updated website/src/pages/changelog.mdx with details of this PR
  • My code follows the general coding style of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Summary by CodeRabbit

  • New Features

    • Introduced a comprehensive, cross-platform desktop notifications system that supports both basic and interactive notifications with action buttons.
    • Enhanced support across Windows, macOS, and Linux to improve the overall user experience.
  • Documentation

    • Added detailed guides and updated the changelog to help users set up, run, and utilize the new notification features.
    • Introduced a new "Notifications" documentation section covering initialization, sending notifications, and handling user interactions.
  • Chores

    • Included licensing information for third-party fonts to ensure proper usage compliance.
    • Added configuration files and task management scripts for building and packaging the application across different platforms.

Copy link
Contributor

coderabbitai bot commented Feb 23, 2025

Walkthrough

This 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

File(s) Change Summary
v3/examples/notifications/frontend/Inter Font License.txt, index.html, package.json, public/style.css, src/main.ts, vite-env.d.ts, tsconfig.json, frontend/bindings/**/*.ts Added new frontend assets, UI components, TypeScript bindings, and configuration files to support the notifications interface and interactivity.
v3/examples/notifications/main.go, v3/go.mod, pkg/services/notifications/*.go, notifications_darwin.*, notifications_windows.go, notifications_linux.go Implemented backend notification services using the Wails framework with platform-specific support, including service initialization, authorization checks, notification dispatch, and response handling.
v3/examples/notifications/Taskfile.yml, v3/examples/notifications/build/**/*.yml, scripts (build.sh, various shell scripts under nfpm) Introduced comprehensive build and packaging configurations and Taskfiles for cross-platform deployment (Darwin, Linux, Windows), covering build, package, run, and asset generation tasks.
v3/examples/notifications/README.md, docs/src/content/docs/changelog.mdx, docs/src/content/docs/learn/notifications.mdx Added documentation for project setup, usage of the notifications system, changelog updates, and API references for the notification features.
v3/pkg/w32/icon.go Added new Windows icon management functionality, including a function to save an HICON as a PNG file with proper error handling.

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
Loading

Poem

I'm a bunny with code so bright,
Hopping through notifications day and night.
With front and back in perfect rhyme,
Cross-platform magic working just fine.
From builds to docs, a joyful spree—
Hoppy coding for you and for me! 🐰✨

Tip

⚡💬 Agentic Chat (Pro Plan, General Availability)
  • We're introducing multi-step agentic chat in review comments and issue comments, within and outside of PR's. This feature enhances review and issue discussions with the CodeRabbit agentic chat by enabling advanced interactions, including the ability to create pull requests directly from comments and add commits to existing pull requests.

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between be1d500 and a8e91e7.

📒 Files selected for processing (1)
  • docs/src/content/docs/changelog.mdx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/src/content/docs/changelog.mdx

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 global NotificationCategories map.
Because this is a shared global map, concurrent goroutines that register or remove notification categories might cause data races. You may want to protect NotificationCategories 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.
If getExeName() 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 calls println(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.
Although New() 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.
If CheckBundleIdentifier() 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.
If actions_json is sizable, synchronous parsing and creation of UNNotificationAction 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

📥 Commits

Reviewing files that changed from the base of the PR and between d5a8f1e and 6d44068.

⛔ 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 by toast.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, if json.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 (like identifier or actionIdentifier) are missing, it could disrupt the flow silently. Consider logging or handling unexpected structures in payload.

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)

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 global NotificationCategories map.
Multiple methods in this file read from and write to the global NotificationCategories 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 the Service 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.
Although application.Get().EmitEvent(...) does not return an error, you might add logs or checks to confirm successful event delivery.


72-86: Unused parameter in SendNotification.
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

📥 Commits

Reviewing files that changed from the base of the PR and between 6d44068 and ab9c460.

📒 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 the New() 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 assigning cDataJSON. This logic silently discards invalid notification data, which might be unexpected for callers. Consistency with other methods like RegisterNotificationCategory (where errors are returned) would help unify error handling.


92-121: Avoid partial fallback in SendNotificationWithActions.

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, or removeNotificationCategory 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

📥 Commits

Reviewing files that changed from the base of the PR and between ab9c460 and 27e512d.

📒 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 and requestUserNotificationAuthorization return a bool. If you need more detailed error reporting, consider using an extended structure or separate error signaling.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 calling CheckBundleIdentifier() 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 and options 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, if json.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

📥 Commits

Reviewing files that changed from the base of the PR and between 27e512d and f8647ff.

📒 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 the Data 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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, when toast.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

📥 Commits

Reviewing files that changed from the base of the PR and between f8647ff and c1230d4.

📒 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 when CategoryID 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 the Service struct.
While New() returns a pointer to an empty Service, it may be more robust to store NotificationCategories 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.
Within ServiceStartup, 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, if options.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 for ErrNotExist 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.
Within getUserText, 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

📥 Commits

Reviewing files that changed from the base of the PR and between c1230d4 and cc760a1.

📒 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 global NotificationCategories 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 the Service 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 around NotificationCategories 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.

@popaprozac
Copy link
Author

popaprozac commented Feb 23, 2025

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 like identifier 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 to identifier 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:

  1. Why the function is not needed on Windows
  2. What developers should do instead on Windows
  3. 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:

  1. The Windows metadata handling workaround using stringified data in activation arguments
  2. How to retrieve metadata when notifications are interacted with on Windows
  3. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 978a982 and c904433.

📒 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7d0f7f4 and af5abda.

📒 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.

@popaprozac
Copy link
Author

popaprozac commented Mar 25, 2025

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 build I left it untouched, we could do the same? And for package you should be signing anyways to use this API and distribute the app. Thoughts?

This was referenced Mar 27, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 Entry

The 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 Clarity

The 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 Section

The 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 returns true. ## Notific...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)


147-188: Platform Considerations Details

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between af5abda and 743882a.

📒 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 Title

The 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 Components

The import of Tabs and TabItem from @astrojs/starlight/components is succinct and appropriate for organizing platform-specific content later in the document.


11-30: Basic Usage and Service Initialization

The "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 Section

The 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 Handling

The 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 Section

This 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 Section

Including 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 Overview

The 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 Definitions

The provided Go type definitions for NotificationOptions, NotificationCategory, NotificationAction, NotificationResponse, and NotificationResult are formatted according to Go conventions and clearly annotated with comments. This will serve as a valuable reference for developers.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 issue

Critical Typo and Syntax Issue in Custom Metadata Example

There are two issues in this code snippet:

  1. The type name is misspelled as NotificaionOptions (line 134) instead of NotificationOptions. This typo duplicates a previously noted issue.
  2. 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 Clear

The 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 Solid

The 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 returns true”—to enhance clarity.

🧰 Tools
🪛 LanguageTool

[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)


66-68: Typo in Introductory Text for Interactive Notifications

The description on line 66 contains a typo: “resgistered” should be corrected to “registered”.


105-125: Notification Response Handling is Appropriately Demonstrated

The 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 Considerations

Within 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

📥 Commits

Reviewing files that changed from the base of the PR and between 743882a and 1022f47.

📒 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-Formed

The front matter (lines 1–3) and title declaration establish the context clearly.


5-6: Import Statement is Correct

The import of { Tabs, TabItem } from the Starlight components is correctly placed for later use in the Tabs section.


56-64: Basic Notification Example Looks Good

The snippet for sending a basic notification is succinct and aligns with the documented API.


69-104: Interactive Notification Setup is Well Illustrated

This 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 Informative

The best practices listed are thorough and provide useful guidance for users working with notifications.


210-304: API Reference and Struct Documentation are Comprehensive

The API Reference tables and the accompanying Go struct definitions are complete and well-organized, offering clear insight into the available methods and data types.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 returns true. ## 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 returns true. ## 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1022f47 and bd5c82e.

📒 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 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 returns true. ## 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

📥 Commits

Reviewing files that changed from the base of the PR and between bd5c82e and b313f2c.

📒 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 returns true. ## Notific...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)

Copy link
Author

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?

Copy link
Member

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
Copy link
Member

@leaanthony leaanthony Mar 29, 2025

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" +
Copy link
Member

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.

Copy link
Author

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)
Copy link
Member

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.

Copy link
Author

@popaprozac popaprozac Mar 30, 2025

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)?

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 issue

Possible metrics mismatch.
Line 84 uses GetSystemMetrics(SM_CXICON) for both width and height. Typically, icon height uses SM_CYICON. If that’s intentional, clarify why; otherwise, use SM_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.
In RequestNotificationAuthorization, if a user denies permission, there's no console log or user-facing message that clarifies the denial, only a boolean value in captureResult. 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.
The checkBundleIdentifier 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 in actions_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 returns NO 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.
In didReceiveNotificationResponse, the code properly marshals data and invokes didReceiveNotificationResponse(...). 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 embedding categories in windowsNotifier, 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 common Service 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 returns png.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

📥 Commits

Reviewing files that changed from the base of the PR and between b313f2c and 9a41b0c.

📒 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 (like SendNotification) 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 for delegateInstance. This is safe in this method alone, but verify that no other code path overwrites delegateInstance.

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:

  1. Minimize XML overhead: Use concise tags and attribute names
  2. Prioritize critical data: Arguments should contain only essential parameters
  3. Use compression for large datasets (Base64 encode binary data when necessary)[6]
  4. 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:


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 around parseNotificationResponse and the data passed to toast.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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9a41b0c and 75dbe85.

📒 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.

leaanthony and others added 2 commits April 12, 2025 12:56
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@leaanthony leaanthony merged commit afb4bd9 into wailsapp:v3-alpha Apr 13, 2025
6 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Apr 29, 2025
15 tasks
@lujihong
Copy link

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方法及调用逻辑:

func initialize() (func(), error) {
	runtime.LockOSThread()

	err := ole.RoInitialize(1)
	if err != nil {
		runtime.UnlockOSThread()
		if oleErr, ok := err.(*ole.OleError); ok && oleErr.Code() == 0x80010106 {
			return nil, fmt.Errorf("RoInitialize: RPC_E_CHANGED_MODE — thread previously initialized with different mode")
		}
		return nil, fmt.Errorf("RoInitialize: %w", err)
	}

	release := func() {
		ole.RoUninitialize()
		runtime.UnlockOSThread()
	}
	return release, nil
}

func pushCOM(appID, xml string) (err error) {
	//.........

	release, err := initialize()
	if err != nil {
		return err
	}
	defer release()

	//.........
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants