[V2] Notifications API #4256
[V2] Notifications API #4256
Conversation
WalkthroughThis change introduces a comprehensive, cross-platform notification system for desktop applications. It adds new interfaces, data structures, and implementations for handling notifications on macOS, Linux, and Windows. The update includes Objective-C bindings and logic for macOS, D-Bus-based notification management for Linux, and Windows toast notification integration. The Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Frontend
participant PlatformNative
App->>Frontend: InitializeNotifications()
Frontend->>PlatformNative: Perform platform-specific setup
App->>Frontend: SendNotification(options)
Frontend->>PlatformNative: Show notification (with/without actions)
PlatformNative-->>Frontend: User interacts with notification
Frontend-->>App: OnNotificationResponse(callback)
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
v2/internal/frontend/desktop/windows/winc/icon.go (2)
22-29: Lazy-loaded proc handles are fine but should be validatedConsider checking
Load()errors foruser32.dll/gdi32.dllso that a missing API (very old Windows) surfaces as a clear build-time panic instead of an undefined pointer call.
51-75: Minor: enum values already in w32
BI_RGBandDIB_RGB_COLORSare re-declared here. They already exist inw32. Re-using the central definition avoids drift.v2/internal/frontend/desktop/darwin/Application.m (2)
390-399: Redundant NSString → UTF8 round-trip can be avoided
identifier,title,subtitle,bodyanddata_jsonare already passed in asconst char *.
Re-encoding them intoNSStringonly to convert them straight back tochar *incurs an unnecessary allocation and copies.
It also keeps an autoreleasedNSStringalive only long enough for the synchronous call, which is safe but brittle if the implementation ever becomes async.- NSString *_identifier = safeInit(identifier); - NSString *_title = safeInit(title); - NSString *_subtitle = safeInit(subtitle); - NSString *_body = safeInit(body); - NSString *_data_json = safeInit(data_json); - - [ctx SendNotification:channelID :[_identifier UTF8String] :[_title UTF8String] :[_subtitle UTF8String] :[_body UTF8String] :[_data_json UTF8String]]; + // Pass the original C strings straight through – WailsContext + // converts them to NSString internally. + [ctx SendNotification:channelID + :identifier + :title + :subtitle + :body + :data_json];
401-411: Same round-trip issue forSendNotificationWithActionsThe exact optimisation from the previous comment applies here as well.
- NSString *_identifier = safeInit(identifier); - NSString *_title = safeInit(title); - NSString *_subtitle = safeInit(subtitle); - NSString *_body = safeInit(body); - NSString *_categoryId = safeInit(categoryId); - NSString *_actions_json = safeInit(actions_json); - - [ctx SendNotificationWithActions:channelID :[_identifier UTF8String] :[_title UTF8String] :[_subtitle UTF8String] :[_body UTF8String] :[_categoryId UTF8String] :[_actions_json UTF8String]]; + [ctx SendNotificationWithActions:channelID + :identifier + :title + :subtitle + :body + :categoryId + :actions_json];v2/internal/frontend/desktop/darwin/WailsContext.m (1)
793-799: DuplicateonceTokenshadows the global symbolA file-scope
static dispatch_once_t onceTokenis declared at line 793 and a second
function-scope variable with the same name is redeclared insideEnsureDelegateInitialized
(lines 798-799).
The inner declaration hides the outer one, so the outer variable is effectively unused
(code smell / reader confusion).Remove the global or rename one of them to make intent explicit.
- static dispatch_once_t onceToken; ... - static dispatch_once_t onceToken; + static dispatch_once_t notificationDelegateOnce; ... + static dispatch_once_t notificationDelegateOnce;v2/pkg/runtime/notifications.go (1)
25-33: Variable shadowing the imported package nameInside every helper you create a variable called
frontend, masking the imported
packagefrontend.
While legal, this hurts readability and can trip up IDE imports.- frontend := getFrontend(ctx) - return frontend.InitializeNotifications() + fe := getFrontend(ctx) + return fe.InitializeNotifications()Apply consistently to all helpers in this file.
v2/internal/frontend/desktop/darwin/notifications.go (1)
402-424: Close and recycle per-request channels to avoid leaks
registerChannelallocates an entry in the globalchannelsmap, but the channel is only removed (and never closed) inGetChannel.
For successful requests the receiver keeps the channel indefinitely, which slowly leaks memory.After the sending goroutine finishes reading from
resultCh, close it:// Example after select { case result := <-resultCh: … } - return nil + close(resultCh) + return niland remove the redundant
cleanupChannelhelper (or use it consistently for both timeout and success paths).v2/internal/frontend/desktop/windows/notifications.go (1)
180-188: Avoid iterating over an empty category when none is foundWhen the requested category is absent the code warns but still iterates over
nCategory.Actions, which is the zero value slice.
While harmless, an early return keeps intent clearer:if options.CategoryID == "" || !categoryExists { fmt.Printf("Category '%s' not found, sending basic notification without actions\n", options.CategoryID) return f.SendNotification(options) }Also applies to: 195-200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
v2/go.mod(1 hunks)v2/internal/frontend/desktop/darwin/Application.h(1 hunks)v2/internal/frontend/desktop/darwin/Application.m(1 hunks)v2/internal/frontend/desktop/darwin/WailsContext.h(1 hunks)v2/internal/frontend/desktop/darwin/WailsContext.m(4 hunks)v2/internal/frontend/desktop/darwin/notifications.go(1 hunks)v2/internal/frontend/desktop/linux/frontend.go(1 hunks)v2/internal/frontend/desktop/linux/notifications.go(1 hunks)v2/internal/frontend/desktop/windows/notifications.go(1 hunks)v2/internal/frontend/desktop/windows/winc/icon.go(2 hunks)v2/internal/frontend/frontend.go(2 hunks)v2/pkg/commands/build/build.go(2 hunks)v2/pkg/runtime/notifications.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
v2/pkg/runtime/notifications.go (2)
v2/internal/frontend/frontend.go (5)
NotificationOptions(80-87)NotificationAction(90-94)NotificationCategory(97-103)NotificationResponse(106-115)NotificationResult(119-122)v2/internal/frontend/desktop/darwin/Application.h (11)
IsNotificationAvailable(73-73)RequestNotificationAuthorization(76-76)CheckNotificationAuthorization(77-77)SendNotification(78-78)SendNotificationWithActions(79-79)RegisterNotificationCategory(80-80)RemoveNotificationCategory(81-81)RemoveAllPendingNotifications(82-82)RemovePendingNotification(83-83)RemoveAllDeliveredNotifications(84-84)RemoveDeliveredNotification(85-85)
v2/internal/frontend/desktop/windows/winc/icon.go (1)
v2/internal/frontend/desktop/windows/winc/w32/typedef.go (1)
HBITMAP(191-191)
v2/internal/frontend/desktop/darwin/notifications.go (4)
v2/internal/frontend/frontend.go (5)
Frontend(124-203)NotificationResult(119-122)NotificationOptions(80-87)NotificationCategory(97-103)NotificationResponse(106-115)v2/internal/frontend/desktop/darwin/Application.h (13)
IsNotificationAvailable(73-73)EnsureDelegateInitialized(75-75)CheckBundleIdentifier(74-74)RequestNotificationAuthorization(76-76)CheckNotificationAuthorization(77-77)SendNotification(78-78)SendNotificationWithActions(79-79)RegisterNotificationCategory(80-80)RemoveNotificationCategory(81-81)RemoveAllPendingNotifications(82-82)RemovePendingNotification(83-83)RemoveAllDeliveredNotifications(84-84)RemoveDeliveredNotification(85-85)v2/internal/frontend/desktop/windows/notifications.go (1)
DefaultActionIdentifier(37-37)v2/internal/frontend/desktop/linux/notifications.go (1)
DefaultActionIdentifier(44-44)
🔇 Additional comments (12)
v2/go.mod (1)
54-54: New Windows-toast dependency looks good
go-toast/v2 v2.0.3is the latest stable tag and is pulled in indirectly, which is correct because the package is only referenced from the Windows implementation.
No further action required.v2/internal/frontend/desktop/linux/frontend.go (1)
7-7: Whitespace fix prevents CGO mis-parsingRemoving the trailing space from the
#cgodirective eliminates a subtle compile warning/error on some tool-chains. Good catch.v2/internal/frontend/desktop/darwin/Application.h (2)
72-86: Unifyboolreturn-value style with existing APIEarlier query functions (e.g.
IsFullScreen,IsMinimised) returnconst bool, but the newly-added notification functions returnbool.
Mixing the two styles will eventually confuse users of the C interface and can trigger C++ linkage warnings.-bool IsNotificationAvailable(void *inctx); -… -bool EnsureDelegateInitialized(void *inctx); +const bool IsNotificationAvailable(void *inctx); +… +const bool EnsureDelegateInitialized(void *inctx);Same for the other boolean return types in this block.
[ suggest_nitpick ]
78-80: Parameter order inconsistency may bite the cgo layerThe two “send” helpers differ only by the extra
categoryIdandactions_jsonparameters, but the new parameters are inserted beforedata_json, shifting the tail of the signature.
If any code paths conditionally call one variant over the other throughunsafe.Pointer/syscall.NewCallback, a silent mismatch becomes a runtime memory-smash.Consider re-ordering so that the common tail (
data_json) is always last:-void SendNotificationWithActions(void *inctx, … const char *body, const char *categoryId, const char *actions_json); +void SendNotificationWithActions(void *inctx, … const char *categoryId, const char *actions_json, const char *body);Or introduce a distinct name such as
SendNotificationWithActionsAndDatato make the difference obvious.
[ flag_critical_issue ]v2/internal/frontend/frontend.go (4)
79-87:NotificationOptions.IDlacks “omitempty” – may break marshalled payloadsAll other optional fields in
NotificationOptionscarryomitempty, butIDdoes not.
If callers omit it intentionally, the JSON payload will still include"id":"", which some platforms treat as non-empty and may reject.- ID string `json:"id"` + ID string `json:"id,omitempty"`[ flag_critical_issue ]
98-103: JSON tag drift:categoryIdentifiervscategoryId
NotificationResponse.CategoryIDuses the tagcategoryIdentifier, whereas everywhere else the camel-cased form iscategoryId.
Apart from being inconsistent, a Go ↔ JS round-trip will lose data unless the client is aware of both spellings.- CategoryID string `json:"categoryIdentifier,omitempty"` + CategoryID string `json:"categoryId,omitempty"`[ suggest_essential_refactor ]
189-203: No way to detachOnNotificationResponsecallback – memory leak / race riskThe interface lets clients register a callback but never unregister it.
Long-running apps that hot-reload or re-initialise front-ends will accumulate dangling closures, and concurrent writes to the stored function can race.Suggestion:
- Return an opaque token (e.g.
func()) that removes the handler.- Document that passing
nilreplaces & clears the existing one.-OnNotificationResponse(callback func(result NotificationResult)) +OnNotificationResponse(callback func(result NotificationResult)) (dispose func())[ suggest_essential_refactor ]
199-203:RemoveNotificationduplicates existing concepts – clarify or dropBoth
RemovePendingNotificationandRemoveDeliveredNotificationexist, yet an additional genericRemoveNotificationis declared with no semantics stated.
Unless it is meant to be cross-platform sugar, keep only the explicit variants to avoid ambiguity.[ request_verification ]
v2/internal/frontend/desktop/darwin/WailsContext.h (3)
95-95: Great catch – method now correctly declared as Objective-C instance method
No further action needed.
[ approve_code_changes ]
99-112: Delegate initialisation race: expose idempotent guard or document contract
EnsureDelegateInitializedsuggests callers must invoke it before any other notification API, yet nothing enforces this ordering from the Go side.
If two goroutines attempt to send the very first notification concurrently, the internalUNUserNotificationCenterdelegate setup could run twice.Either:
- Make the call idempotent and safe internally (preferred), or
- Document clearly that
InitializeNotifications()must precede everything else.[ offer_architecture_advice ]
104-107: UTF-8 expectation ofconst char *…JSONparameters is undocumentedThese methods accept raw UTF-8 C strings meant to carry JSON blobs; passing non-UTF-8 or overly long data will break
-[NSString stringWithUTF8String:]and returnnil, leading to undefined behaviour later.Add explicit comment or runtime check that returns an error back through the callback path when JSON decoding fails.
[ suggest_optional_refactor ]v2/internal/frontend/desktop/darwin/WailsContext.m (1)
1010-1057:RemoveNotificationCategorycallback executed off main thread
UNUserNotificationCenterinvokes the completion handler on an internal queue.
captureResultultimately crosses the C-go boundary – ensure it is thread-safe or
dispatch back to the main queue to match the rest of the runtime callbacks.[center getNotificationCategoriesWithCompletionHandler:^(NSSet<UNNotificationCategory *> *categories) { dispatch_async(dispatch_get_main_queue(), ^{ /* existing logic + captureResult */ }); }];
| - (UNMutableNotificationContent *)createNotificationContentWithTitle:(const char *)title subtitle:(const char *)subtitle body:(const char *)body dataJSON:(const char *)dataJSON error:(NSError **)contentError API_AVAILABLE(macos(10.14)) { | ||
| NSString *nsTitle = [NSString stringWithUTF8String:title]; | ||
| NSString *nsSubtitle = subtitle ? [NSString stringWithUTF8String:subtitle] : @""; | ||
| NSString *nsBody = [NSString stringWithUTF8String:body]; | ||
|
|
||
| UNMutableNotificationContent *content = [[UNMutableNotificationContent alloc] init]; | ||
| content.title = nsTitle; | ||
| if (![nsSubtitle isEqualToString:@""]) { | ||
| content.subtitle = nsSubtitle; | ||
| } | ||
| content.body = nsBody; | ||
| content.sound = [UNNotificationSound defaultSound]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Nil-safety for required fields
title and body are required by the macOS API, but stringWithUTF8String: will
crash if a NULL pointer slips through.
Defensive guards make the bridge more robust to future changes in the Go layer.
- NSString *nsTitle = [NSString stringWithUTF8String:title];
+ if (title == NULL) title = "";
+ if (body == NULL) body = "";
+
+ NSString *nsTitle = [NSString stringWithUTF8String:title];| cancel context.CancelFunc | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
cancel context is created but never used – resource/leak risk
A context.WithCancel is allocated (signalCtx, cancel = …) yet cancel() is never invoked nor returned to the caller.
This leaves the attached goroutine (handleSignals) running for the lifetime of the process and makes clean shutdown impossible.
Consider returning the cancel function (or an explicit Close() method) so callers can terminate signal handling when the frontend is torn down.
Also applies to: 65-70
🏁 Script executed:
#!/bin/bash
set -e
# Locate context.WithCancel usage
grep -R "context.WithCancel" -n v2/internal/frontend/desktop/linux/notifications.go
# Locate cancel variable assignment
grep -R "cancel =" -n v2/internal/frontend/desktop/linux/notifications.go
# Locate handleSignals invocation
grep -R "handleSignals" -n v2/internal/frontend/desktop/linux/notifications.go
# Check if cancel() is ever called
grep -R "cancel()" -n v2/internal/frontend/desktop/linux/notifications.go || echo "No cancel() invocation found"Length of output: 633
Ensure cancel() is invoked and signalCtx is used for proper shutdown
The signalCtx, cancel = context.WithCancel(...) at line 66 in
v2/internal/frontend/desktop/linux/notifications.go creates a cancelable context that’s never used or cancelled. As a result, the goroutine in handleSignals runs for the process lifetime, leaking resources and preventing a clean shutdown.
• v2/internal/frontend/desktop/linux/notifications.go:66 — assignment of signalCtx, cancel
• v2/internal/frontend/desktop/linux/notifications.go:424 — invocation of handleSignals(ctx, c)
• No cancel() call elsewhere in the file
Suggested changes:
--- a/v2/internal/frontend/desktop/linux/notifications.go
+++ b/v2/internal/frontend/desktop/linux/notifications.go
@@ 66,7c66,8
- signalCtx, cancel = context.WithCancel(context.Background())
+ signalCtx, cancel = context.WithCancel(context.Background())
+ go f.handleSignals(signalCtx, c)
@@ 420,424c421,425
- go f.handleSignals(ctx, c)
+ // use the cancelable context for signal handling
+ go f.handleSignals(signalCtx, c)And add a cleanup path (e.g., in your Close() or teardown method):
func (f *Frontend) Close() {
// … other cleanup …
cancel()
}By returning or storing the cancel function, callers can terminate handleSignals and avoid leaking the goroutine.
There was a problem hiding this comment.
I am porting this from the v3-alpha implementation where we create services with ServiceShutdown methods. In v2 how do we handle teardown properly?
There was a problem hiding this comment.
🧩 Analysis chain
@popaprozac From my research of the v2 codebase, there isn't a standardized ServiceShutdown pattern like in v3-alpha. Here's how I suggest handling the teardown in v2:
- Add a
CleanupNotifications()method to yourFrontendimplementation:
// CleanupNotifications cleans up notification resources
func (f *Frontend) CleanupNotifications() {
if cancel != nil {
cancel()
cancel = nil
}
if conn != nil {
conn.Close()
conn = nil
}
}-
In v2, you'll need to hook this into the application shutdown flow manually. You have a few options:
a) If this is being used in an application, you can expose a public
Cleanup()method that app developers can call when shutting down their app.b) You could add a call to a runtime event system (if available) to listen for application shutdown.
The key is that you need to add the cleanup method and document how developers should use it to ensure proper resource cleanup.
For now, I suggest implementing the CleanupNotifications() method at minimum, which can later be hooked into the app shutdown flow as appropriate.
🏁 Script executed:
#!/bin/bash
# Let's check if there's a shutdown or close mechanism in the v2 frontend implementation
echo "Searching for shutdown or close methods in frontend implementations:"
echo "======================================================================"
rg -A 5 "(Shutdown|Close|shutdown|close)" v2/internal/frontend/ -t go
echo -e "\nChecking for app lifecycle methods:"
echo "======================================================================"
rg -A 5 "func (\w+) .*App" v2/internal/frontend -t go
echo -e "\nChecking for any cleanup methods or patterns in frontend implementations:"
echo "======================================================================"
rg -A 5 "(cleanup|Cleanup|destroy|Destroy)" v2/internal/frontend -t go
echo -e "\nLooking at context cancellation patterns in v2:"
echo "======================================================================"
rg "context\.WithCancel" v2/ -t go -A 5
Length of output: 48375
🏁 Script executed:
#!/bin/bash
# Let's look at the Linux frontend structure
echo "Linux frontend structure:"
echo "======================================================================"
rg -l "func \(\w+ \*Frontend\)" v2/internal/frontend/desktop/linux/ -t go | xargs cat
echo -e "\nChecking if the Windows and macOS frontends have shutdown patterns:"
echo "======================================================================"
rg "(Shutdown|Close|cleanup|destroy)" v2/internal/frontend/desktop/(windows|darwin) -t go -A 3
echo -e "\nLooking at how app shutdown is handled in v2:"
echo "======================================================================"
rg "Shutdown" v2/pkg/application/app.go -A 10
Length of output: 35679
🏁 Script executed:
#!/bin/bash
# Let's check the application startup and shutdown sequence
echo "Application startup:"
echo "======================================================================"
rg "func \(\w+ \*App\) Run" v2/pkg/application -A 10
echo -e "\nApplication shutdown:"
echo "======================================================================"
rg "func \(\w+ \*App\) Shutdown" v2/pkg/application -A 15
echo -e "\nFrontend initialization:"
echo "======================================================================"
rg "NewFrontend" v2/internal/frontend -A 5
Length of output: 4557
🏁 Script executed:
#!/bin/bash
# Let's look more closely at the Linux frontend
echo "Linux frontend initialization:"
echo "======================================================================"
rg "func (f \*Frontend) Initialise" v2/internal/frontend/desktop/linux -A 10
echo -e "\nApplication shutdown hooks into frontend:"
echo "======================================================================"
rg "frontend.*Shutdown|frontend.*Close" v2/pkg/application -A 5
# Let's see how other resources are cleaned up in the frontend
echo -e "\nOther resource cleanup in Linux frontend:"
echo "======================================================================"
rg "var conn" v2/internal/frontend/desktop/linux -A 5 -B 5
Length of output: 921
@popaprozac After reviewing the v2 codebase, I can see that unlike v3-alpha which uses a ServiceShutdown pattern, v2 doesn't have a standardized shutdown mechanism for frontend implementations.
For your Linux notifications implementation, you should add a CleanupNotifications() method that will be responsible for properly releasing resources:
// CleanupNotifications cleans up notification resources
func (f *Frontend) CleanupNotifications() {
if cancel != nil {
cancel()
cancel = nil
}
if conn != nil {
conn.Close()
conn = nil
}
}Since there isn't a standard place in v2 to hook this teardown, you have a few options:
- Expose this method so application developers can call it manually when shutting down
- Add a runtime wrapper in
v2/pkg/runtime/notifications.golike:func CleanupNotifications(ctx context.Context) { frontend := getFrontend(ctx) if linuxFrontend, ok := frontend.(*linux.Frontend); ok { linuxFrontend.CleanupNotifications() } }
The important thing is to ensure the cancel() function gets called to stop the goroutine and properly clean up the D-Bus connection.
🧬 Code Graph Analysis Results
Relevant snippets from v2/internal/frontend/desktop/linux/notifications.go (full file provided)
- The file defines a
Frontendreceiver with methods to initialize, send, and manage notifications on Linux using D-Bus. - It uses a package-level
cancelvariable of typecontext.CancelFuncwhich is assigned inInitializeNotifications()when creating a cancellable context for signal handling:var cancel context.CancelFunc ... signalCtx, cancel = context.WithCancel(context.Background()) ... go f.handleSignals(signalCtx, c)
- The
handleSignalsmethod listens for D-Bus signals until the context is done. - There is no explicit
ServiceShutdownor teardown method in this file. - The
cancelfunction is stored globally and can be called to stop signal handling goroutine. - Notifications are tracked in a map with locking, and there are methods to remove notifications.
- The
InitializeNotifications()method sets up the D-Bus connection and signal handling.
Relevant snippet from v2/internal/frontend/frontend.go (lines 124-203)
Frontend interface {
...
// Notifications
InitializeNotifications() error
IsNotificationAvailable() bool
RequestNotificationAuthorization() (bool, error)
CheckNotificationAuthorization() (bool, error)
OnNotificationResponse(callback func(result NotificationResult))
SendNotification(options NotificationOptions) error
SendNotificationWithActions(options NotificationOptions) error
RegisterNotificationCategory(category NotificationCategory) error
RemoveNotificationCategory(categoryID string) error
RemoveAllPendingNotifications() error
RemovePendingNotification(identifier string) error
RemoveAllDeliveredNotifications() error
RemoveDeliveredNotification(identifier string) error
RemoveNotification(identifier string) error
}- This interface defines the notification-related methods implemented by platform-specific frontends.
- There is no explicit
ShutdownorServiceShutdownmethod here.
Relevant snippet from v2/pkg/runtime/notifications.go (lines 25-93)
func InitializeNotifications(ctx context.Context) error {
frontend := getFrontend(ctx)
return frontend.InitializeNotifications()
}
func OnNotificationResponse(ctx context.Context, callback func(result frontend.NotificationResult)) {
frontend := getFrontend(ctx)
frontend.OnNotificationResponse(callback)
}
// Other notification functions delegate to frontend implementations.- This package provides runtime wrappers that call the frontend's notification methods.
- No explicit shutdown or teardown function is exposed here.
Summary of relevant information for teardown in v2 Linux notifications:
- The Linux notifications implementation uses a cancellable context (
cancelfunction) to control the lifetime of the signal handling goroutine. - The
cancelfunction is assigned inInitializeNotifications()but there is no explicit method exposed to call it for teardown. - The
Frontendinterface and runtime notification wrappers do not define aShutdownorServiceShutdownmethod. - Proper teardown in v2 likely involves calling the stored
cancel()function to stop signal handling and cleaning up resources, but this must be done explicitly by the caller or by adding a teardown method. - No automatic or built-in teardown method is present in the provided code.
[no_relevant_code_snippets beyond those above]
✏️ Learnings added
Learnt from: popaprozac
PR: wailsapp/wails#4256
File: v2/internal/frontend/desktop/linux/notifications.go:27-28
Timestamp: 2025-04-29T23:54:07.461Z
Learning: In Wails v2, unlike v3-alpha which has a `ServiceShutdown` method for services, there is no standardized teardown pattern for frontend implementations. When implementing features that require cleanup (like goroutines or resources), add explicit cleanup methods (e.g., `CleanupNotifications()`) that handle resource release, context cancellation, and connection closure.
Learnt from: popaprozac
PR: wailsapp/wails#4256
File: v2/internal/frontend/desktop/linux/notifications.go:27-28
Timestamp: 2025-04-29T23:54:07.461Z
Learning: In Wails v2, unlike v3-alpha which has a `ServiceShutdown` method for services, there is no standardized teardown pattern for frontend implementations. When implementing features that require cleanup (like goroutines or resources), add explicit cleanup methods (e.g., `CleanupNotifications()`) that handle resource release, context cancellation, and connection closure.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
v2/internal/frontend/desktop/darwin/WailsContext.m (1)
847-858: Nil-safety for required fields
titleandbodyare required by the macOS API, butstringWithUTF8String:will crash if aNULLpointer slips through.
Defensive guards make the bridge more robust to future changes in the Go layer.- NSString *nsTitle = [NSString stringWithUTF8String:title]; + if (title == NULL) title = ""; + if (body == NULL) body = ""; + + NSString *nsTitle = [NSString stringWithUTF8String:title];v2/internal/frontend/desktop/windows/notifications.go (1)
299-301:⚠️ Potential issue
saveIconToDir()is still a no-op – Windows toasts will silently fall back to a generic iconThe unresolved stub was pointed out in the previous review and is still present. Without writing the icon file,
go-toastcannot embed the application icon in notifications, resulting in a degraded UX.Proposed implementation (imports and handle‐leak fixes included):
@@ -import ( +import ( "encoding/base64" "encoding/json" "runtime" "sync" + "github.com/wailsapp/wails/v2/internal/frontend/desktop/windows/winc" + "github.com/wailsapp/wails/v2/internal/frontend/desktop/windows/winc/w32" ) @@ func (f *Frontend) saveIconToDir() error { - return nil + // Fast-path: file already exists + if _, err := os.Stat(iconPath); err == nil { + return nil + } + + hMod := w32.GetModuleHandle("") + if hMod == 0 { + return fmt.Errorf("GetModuleHandle failed: %w", syscall.GetLastError()) + } + + icon, err := winc.NewIconFromResource(hMod, uint16(3)) // 3 is conventional for app icon + if err != nil { + return fmt.Errorf("failed to retrieve application icon: %w", err) + } + defer icon.Destroy() + + if err := winc.SaveHIconAsPNG(icon.Handle(), iconPath); err != nil { + return fmt.Errorf("failed to save icon as PNG: %w", err) + } + return nil }
🧹 Nitpick comments (2)
v2/internal/frontend/desktop/darwin/WailsContext.m (1)
876-911: Consider refactoring notification sending methods to reduce duplication.The
SendNotificationandSendNotificationWithActionsmethods contain significant duplicate code. Consider extracting the common logic into a shared private helper method to improve maintainability.+- (void) sendNotificationWithRequest:(UNNotificationRequest *)request channelID:(int)channelID API_AVAILABLE(macos(10.14)) { + UNUserNotificationCenter *center = [UNUserNotificationCenter currentNotificationCenter]; + [center addNotificationRequest:request withCompletionHandler:^(NSError * _Nullable error) { + if (error) { + NSString *errorMsg = [NSString stringWithFormat:@"Error: %@", [error localizedDescription]]; + captureResult(channelID, false, [errorMsg UTF8String]); + } else { + captureResult(channelID, true, NULL); + } + }]; +}Then update the sending methods to use this helper:
UNNotificationRequest *request = [UNNotificationRequest requestWithIdentifier:nsIdentifier content:content trigger:trigger]; - -[center addNotificationRequest:request withCompletionHandler:^(NSError * _Nullable error) { - if (error) { - NSString *errorMsg = [NSString stringWithFormat:@"Error: %@", [error localizedDescription]]; - captureResult(channelID, false, [errorMsg UTF8String]); - } else { - captureResult(channelID, true, NULL); - } -}]; +[self sendNotificationWithRequest:request channelID:channelID];Also applies to: 913-951
v2/internal/frontend/desktop/windows/notifications.go (1)
150-152: Replace scatteredfmt.Printfdiagnostics with structured loggingDirect
fmt.Printfwrites go to stdout and are easy to miss or spam CI logs. Consider the standardlogpackage (or the project’s logger) with severity levels so messages can be filtered and redirected.Also applies to: 176-178, 185-186, 409-410
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
v2/internal/frontend/desktop/darwin/Application.m(1 hunks)v2/internal/frontend/desktop/darwin/WailsContext.m(4 hunks)v2/internal/frontend/desktop/linux/notifications.go(1 hunks)v2/internal/frontend/desktop/windows/notifications.go(1 hunks)v2/pkg/commands/build/build.go(2 hunks)v2/pkg/runtime/notifications.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- v2/internal/frontend/desktop/darwin/Application.m
🚧 Files skipped from review as they are similar to previous changes (3)
- v2/pkg/commands/build/build.go
- v2/internal/frontend/desktop/linux/notifications.go
- v2/pkg/runtime/notifications.go
🔇 Additional comments (9)
v2/internal/frontend/desktop/darwin/WailsContext.m (8)
40-47: Clean import and declaration setup for notification support.The code correctly adds conditional imports for UserNotifications framework with macOS version checks and declares the necessary external functions for callback handlers.
721-735: Good platform compatibility and environment checks.The code appropriately checks for both macOS version compatibility (10.14+) and valid bundle identifier, which are both requirements for the notifications API to function properly.
737-791: Well-implemented notification delegate methods.The UNUserNotificationCenterDelegate protocol implementation is thorough, with proper handling of notification presentation and user responses. The response payload construction captures all relevant notification data and includes proper error handling for JSON serialization failures.
793-804: Thread-safe delegate initialization.Using
dispatch_oncefor notification center delegate setup ensures thread safety and prevents multiple initializations, which is an excellent practice.
805-845: Comprehensive authorization handling.The authorization methods include robust error propagation and appropriate version checking, ensuring failures are properly communicated back to the Go layer.
953-1029: Comprehensive notification category handling.The category registration implementation properly handles action settings, including text input fields, destructive actions, and deals with updating existing categories. The code shows good attention to detail with effective error handling.
1031-1078: Clean notification management methods.The notification removal methods provide a complete API surface for managing both pending and delivered notifications, with both individual and bulk removal options.
1089-1089: Minor spacing correction in About method declaration.Small formatting improvement for consistency.
v2/internal/frontend/desktop/windows/notifications.go (1)
354-360: Shadowing bug fixed – categories now deserialise correctlyGood catch renaming the temporary map and assigning it back; this removes the previous logic error. 👍
| if options.Data != nil { | ||
| encodedPayload, err := encodePayload(DefaultActionIdentifier, options) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to encode notification payload: %w", err) | ||
| } | ||
| n.ActivationArguments = encodedPayload | ||
| } |
There was a problem hiding this comment.
Payload is only encoded when options.Data is non-nil – notification ID & metadata lost otherwise
Decoding on activation expects a base-64 payload. For “simple” notifications (no Data), ActivationArguments remains the literal string DEFAULT_ACTION, which fails Base-64 decoding and drops the options.ID/Title etc. Encode the payload unconditionally:
- if options.Data != nil {
- encodedPayload, err := encodePayload(DefaultActionIdentifier, options)
- if err != nil {
- return fmt.Errorf("failed to encode notification payload: %w", err)
- }
- n.ActivationArguments = encodedPayload
- }
+ encodedPayload, err := encodePayload(DefaultActionIdentifier, options)
+ if err != nil {
+ return fmt.Errorf("failed to encode notification payload: %w", err)
+ }
+ n.ActivationArguments = encodedPayloadApply the same change in SendNotificationWithActions() (lines 216-229).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if options.Data != nil { | |
| encodedPayload, err := encodePayload(DefaultActionIdentifier, options) | |
| if err != nil { | |
| return fmt.Errorf("failed to encode notification payload: %w", err) | |
| } | |
| n.ActivationArguments = encodedPayload | |
| } | |
| // Always encode the payload so ActivationArguments contains a valid Base64 string | |
| encodedPayload, err := encodePayload(DefaultActionIdentifier, options) | |
| if err != nil { | |
| return fmt.Errorf("failed to encode notification payload: %w", err) | |
| } | |
| n.ActivationArguments = encodedPayload |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
v2/internal/frontend/desktop/darwin/WailsContext.m (1)
847-851: Nil-safety for required fields (duplicate of prior feedback)
titleandbodymay arrive asNULL;stringWithUTF8String:will crash in that case.
Add guards similar to what you already do forsubtitle.
🧹 Nitpick comments (1)
v2/internal/frontend/desktop/darwin/WailsContext.m (1)
740-748: Provide a sensible presentation fallback for macOS < 11
optionsremains0when the system is 10.14 – 10.15, meaning the notification is silently dropped.
AddingUNNotificationPresentationOptionAlert(and optionallySound) for these versions preserves user-visible behaviour.- UNNotificationPresentationOptions options = 0; + UNNotificationPresentationOptions options = UNNotificationPresentationOptionAlert | + UNNotificationPresentationOptionSound;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/internal/frontend/desktop/darwin/WailsContext.m(4 hunks)
🔇 Additional comments (1)
v2/internal/frontend/desktop/darwin/WailsContext.m (1)
40-47: Guard usages ofUN*APIs with the same compile-time check used for the import
UNUserNotificationCenter,UNNotification*, etc. are referenced throughout the file without#if __MAC_OS_X_VERSION_MAX_ALLOWED >= 101400guards.
When building on an SDK < 10.14 this will still fail at compile-time even though the runtime@availablechecks are present.Consider wrapping the entire notification section (declarations, protocol conformance, and method bodies) in the same macro guard so the file compiles on older SDKs.
Would you run an older macOS SDK build (or CI job) to prove the compilation path is still green?
|
Example Listen to the event on the frontend and 👌 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
v2/internal/frontend/desktop/windows/winc/icon.go (1)
197-211: Optimize the pixel conversion loop.The current pixel conversion code is correct but could be optimized by directly accessing the RGBA image's pixel data through its Pix field rather than using Set() for each pixel.
// Convert DIB to RGBA for y := 0; y < height; y++ { for x := 0; x < width; x++ { // DIB is bottom-up, so we need to invert Y dibIndex := ((height-1-y)*width + x) * 4 + // RGBA image is top-down + imgIndex := (y*width + x) * 4 // BGRA to RGBA - b := bits[dibIndex] - g := bits[dibIndex+1] - r := bits[dibIndex+2] - a := bits[dibIndex+3] - - // Set pixel in the image - img.Set(x, y, color.RGBA{R: r, G: g, B: b, A: a}) + img.Pix[imgIndex] = bits[dibIndex+2] // R + img.Pix[imgIndex+1] = bits[dibIndex+1] // G + img.Pix[imgIndex+2] = bits[dibIndex] // B + img.Pix[imgIndex+3] = bits[dibIndex+3] // A } }This approach is significantly faster as it avoids the overhead of the
Set()method call and color struct creation for each pixel.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v2/internal/frontend/desktop/linux/notifications.go(1 hunks)v2/internal/frontend/desktop/windows/notifications.go(1 hunks)v2/internal/frontend/desktop/windows/winc/icon.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/internal/frontend/desktop/linux/notifications.go
- v2/internal/frontend/desktop/windows/notifications.go
🔇 Additional comments (1)
v2/internal/frontend/desktop/windows/winc/icon.go (1)
213-222: LGTM: File handling is well implementedThe file creation, error handling, and PNG encoding are implemented correctly with proper resource cleanup.
| hdc, _, _ := createCompatibleDC.Call(0) | ||
| if hdc == 0 { | ||
| return syscall.EINVAL | ||
| } | ||
| defer deleteDC.Call(hdc) | ||
|
|
||
| // Select bitmap into DC | ||
| oldBitmap, _, _ := selectObject.Call(hdc, uintptr(iconInfo.HbmColor)) | ||
| defer selectObject.Call(hdc, oldBitmap) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and use module-level procedures.
Update the DC creation and bitmap selection code to use module-level procedures and improve error handling by checking return values.
// Create DC
- hdc, _, _ := createCompatibleDC.Call(0)
+ hdc, _, err := procCreateCompatibleDC.Call(0)
if hdc == 0 {
- return syscall.EINVAL
+ return fmt.Errorf("failed to create compatible DC: %w", err)
}
- defer deleteDC.Call(hdc)
+ defer procDeleteDC.Call(hdc)
// Select bitmap into DC
- oldBitmap, _, _ := selectObject.Call(hdc, uintptr(iconInfo.HbmColor))
+ oldBitmap, _, err := procSelectObject.Call(hdc, uintptr(iconInfo.HbmColor))
+ if oldBitmap == 0 {
+ return fmt.Errorf("failed to select bitmap: %w", err)
+ }
- defer selectObject.Call(hdc, oldBitmap)
+ defer procSelectObject.Call(hdc, oldBitmap)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
v2/internal/frontend/desktop/darwin/WailsContext.m (1)
844-847:⚠️ Potential issueAdd null safety checks for required fields
The
titleandbodyparameters are required for notification content, butstringWithUTF8String:will crash if passed NULL pointers. Whilesubtitlehas a null check, these other required fields don't.Add defensive checks to prevent crashes:
- NSString *nsTitle = [NSString stringWithUTF8String:title]; - NSString *nsSubtitle = subtitle ? [NSString stringWithUTF8String:subtitle] : @""; - NSString *nsBody = [NSString stringWithUTF8String:body]; + if (title == NULL) title = ""; + if (body == NULL) body = ""; + + NSString *nsTitle = [NSString stringWithUTF8String:title]; + NSString *nsSubtitle = subtitle ? [NSString stringWithUTF8String:subtitle] : @""; + NSString *nsBody = [NSString stringWithUTF8String:body];
🧹 Nitpick comments (1)
v2/internal/frontend/desktop/darwin/WailsContext.m (1)
938-1014: Consider batch handling for category operationsThe category registration performs multiple operations (retrieving categories, modifying the set, setting categories) that could cause race conditions if multiple registration requests occur simultaneously.
Consider using a serialized queue or a more atomic approach when modifying notification categories to prevent potential race conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/internal/frontend/desktop/darwin/WailsContext.m(4 hunks)v2/internal/frontend/desktop/windows/winc/icon.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/internal/frontend/desktop/windows/winc/icon.go
🔇 Additional comments (6)
v2/internal/frontend/desktop/darwin/WailsContext.m (6)
849-849: Good fix: Memory leak resolved with autoreleaseThe implementation correctly uses autorelease to prevent memory leaks in a manual reference counting environment.
793-800: Good fix: Delegate initialization improvedThe implementation correctly sets the notification center delegate directly rather than using
dispatch_once, avoiding potential dangling pointer issues if the context is deallocated and recreated.
737-750: Good implementation of notification presentation optionsThe code correctly handles different presentation options based on macOS version, using appropriate availability checks for macOS 11.0+ specific features.
752-791: Well-implemented notification response handlingThe notification response handling is thorough and includes proper error handling for JSON serialization, along with comprehensive payload construction that captures all relevant notification data.
802-824: Thorough authorization implementation with proper error handlingThe notification authorization request implementation includes appropriate version checks, delegate verification, and comprehensive error handling in the completion handler.
721-1063: Overall well-implemented notification systemThe notification implementation provides a comprehensive cross-platform bridge for macOS notifications with proper availability checks, error handling, and memory management throughout.
|
@popaprozac - Could we hardcode the path for this command? |
|
|
Description
Backports Notification API from v3-alpha.
Adds runtime functionality. No JS API at the moment, easy to add?
This includes a self-signing build step for
devandbuild.I know this is a sizable addition so if we want to move forward with this I am happy to write docs etc
Fixes (#1788)
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.If you checked Linux, please specify the distro and version.
Test Configuration
Please paste the output of
wails doctor. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit