[client] Keep the UI running when the notification service fails to start - #6959
[client] Keep the UI running when the notification service fails to start#6959lixmal wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesNotification wrapper integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/ui/notifier.go`:
- Around line 95-99: Add the `//wails:ignore` directive immediately above
`Notifier.OnNotificationResponse` so Wails excludes this callback-based wrapper
from generated bindings, while leaving its Go implementation unchanged.
- Around line 56-92: The Notifier wrapper does not preserve the complete
notifications.NotificationService API. Extend *Notifier with availability-gated
forwarding methods for UpdateNotification, RemoveNotificationCategory,
RemoveAllPendingNotifications, RemovePendingNotification,
RemoveAllDeliveredNotifications, RemoveDeliveredNotification, and
RemoveNotification, matching the underlying service signatures and existing
unavailable behavior; keep the current methods unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 80861e10-c477-4b9d-9be3-8552e4745847
📒 Files selected for processing (5)
client/ui/main.goclient/ui/notifier.goclient/ui/tray.goclient/ui/tray_notify.goclient/ui/tray_update.go
| func (n *Notifier) CheckNotificationAuthorization() (bool, error) { | ||
| if !n.available.Load() { | ||
| return false, errNotificationsUnavailable | ||
| } | ||
| return n.inner.CheckNotificationAuthorization() | ||
| } | ||
|
|
||
| func (n *Notifier) RequestNotificationAuthorization() (bool, error) { | ||
| if !n.available.Load() { | ||
| return false, errNotificationsUnavailable | ||
| } | ||
| return n.inner.RequestNotificationAuthorization() | ||
| } | ||
|
|
||
| // SendNotification delivers a notification, silently dropping it when the | ||
| // backend never started (notifications are best-effort everywhere). | ||
| func (n *Notifier) SendNotification(options notifications.NotificationOptions) error { | ||
| if !n.available.Load() { | ||
| log.Debugf("notifications disabled, dropping %q", options.ID) | ||
| return nil | ||
| } | ||
| return n.inner.SendNotification(options) | ||
| } | ||
|
|
||
| func (n *Notifier) SendNotificationWithActions(options notifications.NotificationOptions) error { | ||
| if !n.available.Load() { | ||
| log.Debugf("notifications disabled, dropping %q", options.ID) | ||
| return nil | ||
| } | ||
| return n.inner.SendNotificationWithActions(options) | ||
| } | ||
|
|
||
| func (n *Notifier) RegisterNotificationCategory(category notifications.NotificationCategory) error { | ||
| if !n.available.Load() { | ||
| return nil | ||
| } | ||
| return n.inner.RegisterNotificationCategory(category) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
rg -n 'UpdateNotification|RemoveNotification(Category)?|Remove(All)?(Pending|Delivered)Notifications?' .Repository: netbirdio/netbird
Length of output: 965
🏁 Script executed:
#!/bin/sh
set -euo pipefail
echo "== files =="
git ls-files | rg '^(client/ui/(notifier|main|.*notifications).*\.go|go\.mod|go\.sum)$|notifications' | head -200
echo
echo "== Locate NotificationService definitions/usages =="
rg -n "NotificationService|type Notify|func .*Notification|RegisterNotificationCategory|requestBackend|available" client/ui shared -S
echo
echo "== notifier.go outline =="
ast-grep outline client/ui/notifier.go --view expanded || true
echo
echo "== notifier.go relevant section =="
cat -n client/ui/notifier.go | sed -n '1,160p'
echo
echo "== main.go registration context =="
cat -n client/ui/main.go | sed -n '300,350p'
echo
echo "== package notification files =="
for f in $(git ls-files | rg 'notifications\.go$|.*notifications.*\.go$' | grep -E 'client/(ui|.*backend|internal)|.*notifications.*'); do
echo "--- $f"
wc -l "$f"
sed -n '1,240p' "$f"
doneRepository: netbirdio/netbird
Length of output: 39965
🏁 Script executed:
#!/bin/sh
set -euo pipefail
echo "== go.mod wails dependency =="
rg -n 'github\.com/wailsapp/wails|v3' go.mod go.sum | head -80
echo
echo "== inspect Wails module cache if present =="
MODCACHE="${GOPATH:-$HOME/go}/pkg/cache/modules"
if [ -d "$MODCACHE" ]; then
fd 'notifications\.go$|notifications.*\.go$' "$MODCACHE/github.com/wailsapp/wails" | head -50 || true
for f in $(fd 'notifications\.go$|notifications.*\.go$' "$MODCACHE/github.com/wailsapp/wails" | head -50); do
echo "--- $f"
wc -l "$f"
rg -n 'type NotificationService|func .*Notification|UpdateNotification|RemoveNotification|Clear|All|Pending|Delivered|NotificationCategory|NotificationResult|NotificationResponse|ServiceStartup' "$f" -C 2
done
else
echo "no wails module cache found at $MODCACHE"
fiRepository: netbirdio/netbird
Length of output: 5010
🌐 Web query:
github.com wailsapp wails v3.0.0-alpha2.117 notifications Service NotificationService UpdateNotification RemoveNotification
💡 Result:
In Wails v3 (including version v3.0.0-alpha2.117), the NotificationService is part of the github.com/wailsapp/wails/v3/pkg/services/notifications package [1][2][3]. This service provides a cross-platform interface for managing system notifications [2]. The requested methods function as follows: - UpdateNotification: This method updates an existing, in-flight notification using its unique ID [2][4]. When calling this method, you provide NotificationOptions containing the updated content [2]. On platforms like macOS and Linux, this typically replaces the notification in place, while on Windows, it may redeliver the notification [4]. - RemoveNotification: The NotificationService supports removing notifications, generally identified by the unique ID provided in the original notification options [2][5]. While specific API availability can vary by platform—for instance, macOS offers more granular control through UNUserNotificationCenter methods like RemoveDeliveredNotification and RemovePendingNotification—the NotificationService abstraction is designed to handle notification lifecycle management [2][4][5]. To use the service, you initialize it via notifications.New() and register it within your application's Service options [2][6]: import "github.com/wailsapp/wails/v3/pkg/application" import "github.com/wailsapp/wails/v3/pkg/services/notifications" // Create the service notifier:= notifications.New // Register the service with the application app:= application.New(application.Options{ Services: []application.Service{ application.NewService(notifier), }, }) For detailed implementation, refer to the official Wails v3 documentation [2][7] or the source code in the v3/pkg/services/notifications directory [1][3].
Citations:
- 1: https://github.com/wailsapp/wails/blob/3ec88c12/v3/pkg/services/notifications/notifications.go
- 2: https://v3.wails.io/features/notifications/overview/
- 3: https://pkg.go.dev/github.com/wailsapp/wails/v3/pkg/services/notifications
- 4: feat(v3:notifications): add sounds, attachments, and more wailsapp/wails#5333
- 5: [v3] Notifications API wailsapp/wails#4098
- 6: https://github.com/wailsapp/wails/blob/3ec88c12/v3/examples/notifications/main.go
- 7: https://v3.wails.io/reference/overview/
🌐 Web query:
pkg.go.dev github.com/wailsapp/wails v3/pkg/services/notifications NotificationService
💡 Result:
The NotificationService in the Wails v3 framework is a cross-platform service located in the github.com/wailsapp/wails/v3/pkg/services/notifications package [1][2]. It is designed to provide desktop applications with the ability to display native system notifications, including basic alerts, interactive notifications with actions, and categorized notification configurations [3][2]. Key aspects of the NotificationService include: Integration: To use the service, you typically create an instance using notifications.New and register it with your Wails application during initialization within the application.Options struct [3][4]. Functional Capabilities: The service supports methods such as SendNotification and SendNotificationWithActions, allowing developers to handle notifications consistently across macOS, Windows, and Linux [3][5][2]. Callback System: NotificationService provides an OnNotificationResponse method, which registers a callback function. This function is invoked when a user interacts with a notification (e.g., clicking a button or responding), allowing the application to process the resulting NotificationResult [1][4][5]. Internal Implementation: The service uses a platform-specific implementation (darwin, linux, or windows) under the hood [6][5]. These implementations manage the platform's native notification APIs and thread-safely bridge interaction events back to the main service, which then triggers the registered user callback [1][6][5]. For detailed usage, developers are encouraged to consult the official Wails v3 documentation [3] or the package documentation on pkg.go.dev [2].
Citations:
- 1: https://github.com/wailsapp/wails/blob/3ec88c12/v3/pkg/services/notifications/notifications.go
- 2: https://pkg.go.dev/github.com/wailsapp/wails/v3/pkg/services/notifications
- 3: https://v3.wails.io/features/notifications/overview/
- 4: https://github.com/wailsapp/wails/blob/3ec88c12/v3/examples/notifications/main.go
- 5: https://github.com/wailsapp/wails/blob/3ec88c12/v3/pkg/services/notifications/notifications_windows.go
- 6: [v3] improve service names for js/ts bindings wailsapp/wails#4405
🌐 Web query:
github.com wailsapp wails v3.0.0-alpha2.117 notifications.go UpdateNotification RemoveNotification RemoveAllPendingNotifications RemoveAllDeliveredNotifications
💡 Result:
In Wails v3.0.0-alpha2.117, the notification management methods you identified—UpdateNotification, RemoveNotification, RemoveAllPendingNotifications, and RemoveAllDeliveredNotifications—are part of the cross-platform notifications service defined in github.com/wailsapp/wails/v3/pkg/services/notifications [1][2]. Key details regarding these methods include: UpdateNotification: This method is used to update an existing "in-flight" notification by its ID [3]. RemoveNotification: In the macOS implementation (notifications_darwin.go), this method exists as a stub that returns nil [4]. The documentation explicitly advises against using it, directing developers to use more specific methods for pending or delivered notifications instead [4]. RemoveAllPendingNotifications and RemoveAllDeliveredNotifications: These are specific management methods for removing notifications from their respective states (pending or delivered) [4]. General Usage: These methods are part of the NotificationService struct [1][2]. Because Wails v3 supports different operating systems (macOS, Windows, and Linux), the underlying implementation handles platform-specific differences [1][2]. For instance, the macOS implementation provides concrete functions for removing notifications by state (pending or delivered) [4]. Note: Ensure you are referencing the v3-specific documentation at https://v3.wails.io/features/notifications/overview/ rather than the v2 runtime documentation, as the API structure differs between versions [5][3][6].
Citations:
- 1: https://github.com/wailsapp/wails/blob/3ec88c12/v3/pkg/services/notifications/notifications.go
- 2: https://pkg.go.dev/github.com/wailsapp/wails/v3/pkg/services/notifications
- 3: https://v3.wails.io/features/notifications/overview/
- 4: https://github.com/wailsapp/wails/blob/3ec88c12/v3/pkg/services/notifications/notifications_darwin.go
- 5: https://pkg.go.dev/github.com/wailsapp/wails/v3@v3.0.0-alpha2.117
- 6: https://wails.io/docs/reference/runtime/notification/
🌐 Web query:
github.com wailsapp/wails/blob/3ec88c12/v3/pkg/services/notifications/notifications.go
💡 Result:
The file located at v3/pkg/services/notifications/notifications.go is part of the Wails v3 framework, which provides a cross-platform service for displaying native system notifications [1][2][3]. Key characteristics and functionality of this package include: Cross-Platform Support: It supports macOS, Windows, and Linux by abstracting platform-specific differences while exposing a unified Go API [1][2]. Core Functionality: - Basic notifications: Supports titles, subtitles, and body text [1][4]. - Interactive notifications: Allows defining categories with actionable buttons [1][2]. - Callback System: Provides a unified response system via OnNotificationResponse to handle user interaction with notifications [2][5]. - Configuration: Uses a NotificationOptions struct for flexible configuration, including unique IDs, data payloads, and interruption levels [1][4]. Usage: To use the service, you must instantiate it (notifications.New) and register it with the Wails application during setup [4][5]. Note that for platform-specific features (such as on macOS), applications may need to be correctly packaged and signed to function as expected [6]. The package path has been standardized to github.com/wailsapp/wails/v3/pkg/services/notifications in recent versions to ensure correct compilation [7].
Citations:
- 1: https://github.com/wailsapp/wails/blob/3ec88c12/v3/pkg/services/notifications/notifications.go
- 2: https://pkg.go.dev/github.com/wailsapp/wails/v3/pkg/services/notifications
- 3: https://v3.wails.io/reference/overview/
- 4: https://v3.wails.io/features/notifications/overview/
- 5: https://github.com/wailsapp/wails/blob/3ec88c12/v3/examples/notifications/main.go
- 6: https://github.com/wailsapp/wails/blob/3ec88c12/v3/pkg/services/notifications/notifications_darwin.go
- 7: docs(v3): fixed notifications pkg path wailsapp/wails#4617
🏁 Script executed:
#!/bin/sh
set -euo pipefail
echo "== raw remote source sections for pinned tags/commits =="
for ref in "3ec88c12" "v3.0.0-alpha2.117"; do
echo "--- reference $ref"
base="https://raw.githubusercontent.com/wailsapp/wails/$ref/v3/pkg/services/notifications"
for f in notifications.go notifications_darwin.go notifications_linux.go notifications_windows.go; do
url="$base/$f"
if command -v curl >/dev/null 2>&1; then
echo "//// curl $url"
curl -sS "$url" | rg -n 'type NotificationService|func .*Notification|UpdateNotification|RemoveNotification|RemoveAll(Pending|Delivered)|RegisterNotificationCategory|SendNotification|NotificationResult|NotificationResponse' -C 2 || true
fi
done
doneRepository: netbirdio/netbird
Length of output: 44794
Preserve the full Wails notification service API on Notifier.
*Notifier now replaces notifications.NotificationService at client/ui/main.go:330, but it only forwards a subset of methods. Wails NotificationService also exposes UpdateNotification, RemoveNotificationCategory, RemoveAllPendingNotifications, RemovePendingNotification, RemoveAllDeliveredNotifications, RemoveDeliveredNotification, and RemoveNotification; omitting these changes the generated frontend contract. Add availability-gated forwarding for the full surface, or treat this change as intentional and update callers/frontend-generated APIs accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/ui/notifier.go` around lines 56 - 92, The Notifier wrapper does not
preserve the complete notifications.NotificationService API. Extend *Notifier
with availability-gated forwarding methods for UpdateNotification,
RemoveNotificationCategory, RemoveAllPendingNotifications,
RemovePendingNotification, RemoveAllDeliveredNotifications,
RemoveDeliveredNotification, and RemoveNotification, matching the underlying
service signatures and existing unavailable behavior; keep the current methods
unchanged.
Source: MCP tools
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
|
1 participant
Describe your changes
Wails treats a service startup error as fatal, so when the notification backend cannot start (an unbundled binary on macOS has no bundle identifier, a headless Linux session has no D-Bus session bus) the whole UI exited on launch. Wrap the notification service so a startup failure only disables notifications.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit