[client] Use WinRT COM for Windows toasts#6013
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a notifier abstraction with platform-specific implementations (Windows toast via go-toast with Fyne fallback), wires notifier into UI/service/event code (replacing direct Fyne notifications), packages a toast icon and AppUserModel metadata in installers/shortcuts, and moves autostart registry handling to HKLM with extended uninstall cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Client App
participant Notifier as Notifier (abstraction)
participant WinToast as Windows Toast (go-toast)
participant Fyne as Fyne Notification
App->>Notifier: Send(title, body)
Notifier->>WinToast: attempt push (uses netbird.png, AppUserModelId)
alt WinToast success
WinToast-->>Notifier: pushed
Notifier-->>App: done
else WinToast fail or non-Windows
Notifier->>Fyne: fallback SendNotification(title, body)
Fyne-->>Notifier: delivered
Notifier-->>App: done
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
client/ui/notifier/notifier_windows.go (2)
43-53: Avoid permanent COM disable after a single initialization failure.With
initOnce, one transientSetAppDataerror forces fyne fallback for the rest of the process. Consider allowing retries until initialization succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ui/notifier/notifier_windows.go` around lines 43 - 53, The current use of initOnce and persisting initErr makes a single transient wintoast.SetAppData failure permanently force n.fallback; change the logic so initialization can be retried rather than short-circuiting forever: remove the reliance on initOnce storing initErr as a permanent gate, and instead attempt wintoast.SetAppData when creating or obtaining the notifier (e.g., inside the same function that currently calls initOnce), using a short mutex or retry loop with backoff and limited attempts; on success set a single atomic/boolean flag (e.g., appDataRegistered) so subsequent calls skip repeats, and only fall back to n.fallback when all retry attempts fail. Ensure you update references to initOnce/initErr and preserve n.iconPath, appID, appGUID and n.fallback usage.
69-72: Latch COM failures to reduce repeated warning noise.After a toast push failure, the notifier keeps attempting COM on every call. Consider switching to fallback-only mode after first failure (thread-safe flag) to avoid repeated warn spam.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ui/notifier/notifier_windows.go` around lines 69 - 72, The toast notifier currently retries COM on every call when notification.Push() fails; add a thread-safe latch (e.g., a boolean field on the notifier like comFailed or fallbackOnly) protected with sync/atomic (or a mutex) and check it before attempting notification.Push(); on the first Push() error set the latch (atomic.Store/CompareAndSwap) and from then on skip notification.Push() and call n.fallback.Send(title, body) directly to avoid repeated log.Warnf spam (update the code around notification.Push(), log.Warnf and n.fallback.Send to consult/set this flag).client/ui/notifier/notifier.go (1)
25-27: Add a defensive nil guard in fallback sender.
fyneNotifier.Sendwill panic iff.appis nil (Line 26). A no-op guard makes the fallback safer in edge/test wiring paths.Proposed hardening
func (f *fyneNotifier) Send(title, body string) { + if f == nil || f.app == nil { + return + } f.app.SendNotification(fyne.NewNotification(title, body)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ui/notifier/notifier.go` around lines 25 - 27, fyneNotifier.Send can panic when the receiver's app field is nil; add a defensive nil guard at the start of fyneNotifier.Send that returns immediately (no-op) if f == nil or f.app == nil to avoid panics in edge/test wiring paths, then only call f.app.SendNotification(fyne.NewNotification(title, body)) when f.app is non-nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/installer.nsis`:
- Line 231: The Start Menu shortcut created in installer.nsis must include the
System.AppUserModel.ID (PKEY_AppUserModel_ID) and
System.AppUserModel.ToastActivatorCLSID properties so WinRT/COM toast
notifications work; update the NSIS flow (where CreateShortCut is used) to first
create the shortcut and then call a native helper (or PowerShell/C# one-off)
that opens the .lnk and sets PKEY_AppUserModel_ID to your app AUMID and
System.AppUserModel.ToastActivatorCLSID to your COM activator CLSID using
IPropertyStore/IShellLink (or SHGetPropertyStoreForParsingName) — invoke that
helper from installer.nsis immediately after the File/CreateShortCut step so the
Start Menu entry has both properties set.
- Around line 250-252: The autostart registry entry is written without quoting
the executable path which breaks when $INSTDIR contains spaces; update the
WriteRegStr call that sets the Run value (the code using AUTOSTART_REG_KEY,
APP_NAME, UI_APP_EXE and $INSTDIR) to write a quoted command line (wrap the full
path to ${UI_APP_EXE}.exe in double quotes and preserve any existing args) so
Windows launches the correct executable at logon.
In `@client/netbird.wxs`:
- Line 52: The HKCU RegistryKey for AppUserModelId must be removed from the
existing per-machine component and placed into a new per-user component: delete
the RegistryKey entry Root="HKCU" Key="Software\Classes\AppUserModelId\NetBird"
from the current component that installs Program Files and the service, then add
a new WiX <Component Id="NetBird_AppUserModelId_PerUser" Guid="*" KeyPath="yes">
containing the RegistryKey Root="HKCU"
Key="Software\Classes\AppUserModelId\NetBird" (or a RegistryValue set as
KeyPath) so the HKCU key is the component keypath; hook that component into a
per-user feature or wire it to per-user install logic (or implement Active Setup
/ first-run registration) so the registry is created per user and ICE57 is
satisfied.
---
Nitpick comments:
In `@client/ui/notifier/notifier_windows.go`:
- Around line 43-53: The current use of initOnce and persisting initErr makes a
single transient wintoast.SetAppData failure permanently force n.fallback;
change the logic so initialization can be retried rather than short-circuiting
forever: remove the reliance on initOnce storing initErr as a permanent gate,
and instead attempt wintoast.SetAppData when creating or obtaining the notifier
(e.g., inside the same function that currently calls initOnce), using a short
mutex or retry loop with backoff and limited attempts; on success set a single
atomic/boolean flag (e.g., appDataRegistered) so subsequent calls skip repeats,
and only fall back to n.fallback when all retry attempts fail. Ensure you update
references to initOnce/initErr and preserve n.iconPath, appID, appGUID and
n.fallback usage.
- Around line 69-72: The toast notifier currently retries COM on every call when
notification.Push() fails; add a thread-safe latch (e.g., a boolean field on the
notifier like comFailed or fallbackOnly) protected with sync/atomic (or a mutex)
and check it before attempting notification.Push(); on the first Push() error
set the latch (atomic.Store/CompareAndSwap) and from then on skip
notification.Push() and call n.fallback.Send(title, body) directly to avoid
repeated log.Warnf spam (update the code around notification.Push(), log.Warnf
and n.fallback.Send to consult/set this flag).
In `@client/ui/notifier/notifier.go`:
- Around line 25-27: fyneNotifier.Send can panic when the receiver's app field
is nil; add a defensive nil guard at the start of fyneNotifier.Send that returns
immediately (no-op) if f == nil or f.app == nil to avoid panics in edge/test
wiring paths, then only call f.app.SendNotification(fyne.NewNotification(title,
body)) when f.app is non-nil.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1194379-7983-46b0-a4d6-5f9981f26ed3
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
client/installer.nsisclient/netbird.wxsclient/ui/client_ui.goclient/ui/event/event.goclient/ui/event_handler.goclient/ui/notifier/notifier.goclient/ui/notifier/notifier_other.goclient/ui/notifier/notifier_windows.goclient/ui/profile.gogo.mod
mlsmaycon
previously approved these changes
Apr 28, 2026
lixmal
force-pushed
the
windows-toast-com
branch
from
351a877 to
ec5d1c9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/installer.nsis (1)
332-332: Use an explicit AUMID constant instead of${APP_NAME}for key cleanup.
AppUserModelIdidentity is a protocol contract (NetBird), not a display-name concern. Binding uninstall cleanup to${APP_NAME}risks drift later.Suggested refactor
+!define TOAST_AUMID "NetBird" ... -DeleteRegKey HKCU "Software\Classes\AppUserModelId\${APP_NAME}" +DeleteRegKey HKCU "Software\Classes\AppUserModelId\${TOAST_AUMID}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/installer.nsis` at line 332, Replace the use of ${APP_NAME} in the AppUserModelId registry cleanup with an explicit AUMID constant; specifically, introduce or use a constant like AUMID (e.g., "NetBird") and change the DeleteRegKey call that currently references DeleteRegKey HKCU "Software\Classes\AppUserModelId\${APP_NAME}" to use that AUMID constant, and update any other registry CreateKey/DeleteKey references that point at "Software\Classes\AppUserModelId\${APP_NAME}" so the uninstall always targets the canonical AppUserModelId rather than the mutable ${APP_NAME}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/installer.nsis`:
- Around line 259-260: The installer branch that handles user opt-out of
autostart only deletes the HKLM entry; add a matching deletion of the legacy
HKCU autostart value so the old per-user Run key can't start the UI. In the same
branch where DeleteRegValue HKLM "${AUTOSTART_REG_KEY}" "${APP_NAME}" and
DetailPrint "Autostart not enabled by user" are executed, also call
DeleteRegValue HKCU "${AUTOSTART_REG_KEY}" "${APP_NAME}" (and keep a clear
DetailPrint or combine messages) so both HKLM and HKCU entries referencing
AUTOSTART_REG_KEY/APP_NAME are removed. Ensure the new DeleteRegValue uses the
same AUTOSTART_REG_KEY and APP_NAME symbols to match existing logic.
---
Nitpick comments:
In `@client/installer.nsis`:
- Line 332: Replace the use of ${APP_NAME} in the AppUserModelId registry
cleanup with an explicit AUMID constant; specifically, introduce or use a
constant like AUMID (e.g., "NetBird") and change the DeleteRegKey call that
currently references DeleteRegKey HKCU
"Software\Classes\AppUserModelId\${APP_NAME}" to use that AUMID constant, and
update any other registry CreateKey/DeleteKey references that point at
"Software\Classes\AppUserModelId\${APP_NAME}" so the uninstall always targets
the canonical AppUserModelId rather than the mutable ${APP_NAME}.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa865cc3-d693-42ce-a1b9-fbcb4c028325
📒 Files selected for processing (2)
client/installer.nsisclient/netbird.wxs
🚧 Files skipped from review as they are similar to previous changes (1)
- client/netbird.wxs
lixmal
force-pushed
the
windows-toast-com
branch
from
ec5d1c9 to
f4da010
Compare
|
mlsmaycon
approved these changes
Apr 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Replaces fyne's PowerShell-based Windows notification path with a WinRT COM implementation via
git.sr.ht/~jackmordaunt/go-toast/v2. Removes thepowershell.exewindow flash on every notification and lets toasts use the NetBird icon.notifier.Notifierinterface with a fyne fallback for non-Windows platforms and a WinRT COM implementation for Windows.app.SendNotificationcall sites in the UI through the notifier; decouple the event manager from fyne entirely.NetBirdand a stable activation CLSID so toasts display under the right name and icon.netbird.pngfor the toast icon, clean up the AUMID registry key on uninstall.System.AppUserModel.IDandToastActivatorCLSIDto both shortcuts, ship the toast icon, force-delete the AUMID key on uninstall.Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
User-visible behavior on Windows is the same set of toast notifications, just without the PowerShell window flash and with the app icon shown.
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/__
Summary by CodeRabbit
New Features
Changes
Bug Fixes