[v2] Notifications API#4256
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a cross‑platform desktop Notifications API: new runtime/frontend types and wrappers, native implementations for macOS (Obj‑C + cgo), Windows (toasts, registry/COM, icon helper), and Linux (D‑Bus), docs and changelog, plus a Darwin post‑build codesign and an indirect go-toast dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App (Go)
participant RT as Runtime wrapper
participant FE as Frontend (platform)
participant OS as OS Notification Center
participant CB as Registered callback
App->>RT: InitializeNotifications / SendNotification(options)
RT->>FE: Forward call to platform frontend
alt macOS
FE->>OS: Request auth / Schedule UNNotificationRequest
else Windows
FE->>OS: Show toast via COM / Activation setup
else Linux
FE->>OS: Send via D‑Bus org.freedesktop.Notifications
end
OS-->>FE: Activation / Action / Dismiss / Signal
FE->>RT: Normalize NotificationResult (async)
RT->>CB: Invoke registered callback with NotificationResult
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
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 */ }); }];
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. 👍
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.
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? |
|
|
|
@leaanthony I fear our window for this came and went. Was so looking forward to closing #1788 😅. Should I go ahead and close? |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@v2/internal/frontend/desktop/windows/notifications.go`:
- Around line 74-84: The registry key returned by registry.CreateKey (variable
key) is not closed if key.SetStringValue fails; change the code in the block
that creates the CLSID key to call defer key.Close() immediately after
successful creation of key so the key is always closed on any early return
(remove the later manual key.Close() call), i.e., add defer key.Close() right
after the registry.CreateKey call in the notifications.go code that handles the
CLSID LocalServer32 setup.
- Around line 437-450: handleNotificationResult currently takes a write lock and
invokes notificationResultCallback even when it's nil (causing a recovered panic
each time); change callbackLock.Lock/Unlock to RLock/RUnlock and read
notificationResultCallback into a local variable, then if the local callback is
nil simply return without starting the goroutine or relying on recover;
reference the symbols handleNotificationResult, notificationResultCallback, and
callbackLock and ensure the nil check happens before spawning the goroutine so
only non-nil callbacks are invoked.
🧹 Nitpick comments (2)
v2/internal/frontend/desktop/windows/notifications.go (2)
165-184: Errors and warnings written to stdout instead of stderr.Lines 167, 192, and 200 use
fmt.Printffor error/warning messages, while Line 445 correctly usesfmt.Fprintf(os.Stderr, ...). Diagnostic output should go to stderr consistently.
254-260: Unnecessary type conversion on Line 257.
category.HasReplyFieldis alreadybool; thebool(...)cast is redundant.Proposed fix
- HasReplyField: bool(category.HasReplyField), + HasReplyField: category.HasReplyField,
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@v2/internal/frontend/desktop/windows/notifications.go`:
- Around line 312-322: In saveIconToDir, ensure the HICON from w32.ExtractIcon
is released by adding a defer w32.DestroyIcon(hIcon) after verifying hIcon != 0
inside the iconOnce.Do closure; keep the current flow that calls
winc.SaveHIconAsPNG(hIcon, iconPath) and sets iconErr, but guarantee DestroyIcon
runs regardless of save success so the GDI handle (hIcon) is not leaked while
still using iconOnce, exePath, iconPath and iconErr as currently implemented.
🧹 Nitpick comments (3)
v2/internal/frontend/desktop/windows/notifications.go (3)
6-24: Import groups are split non-idiomatically.Standard library imports (
fmt,os,path/filepath,unsafe) are split across two groups with external packages in between. Go convention is a single stdlib group followed by a single external group.♻️ Suggested grouping
import ( "encoding/base64" "encoding/json" + "fmt" + "os" + "path/filepath" "sync" + _ "unsafe" // for go:linkname wintoast "git.sr.ht/~jackmordaunt/go-toast/v2/wintoast" "github.com/google/uuid" "github.com/wailsapp/wails/v2/internal/frontend" "github.com/wailsapp/wails/v2/internal/frontend/desktop/windows/winc" "github.com/wailsapp/wails/v2/internal/frontend/desktop/windows/winc/w32" - - "fmt" - "os" - "path/filepath" - _ "unsafe" // for go:linkname - "git.sr.ht/~jackmordaunt/go-toast/v2" "golang.org/x/sys/windows/registry" )
165-168: Diagnostic output goes tostdoutviafmt.Printf— preferstderror a logger.Lines 167, 192, 200, and 430 use
fmt.Printffor error/warning messages. These will intermingle with normal application stdout. At minimum, usefmt.Fprintf(os.Stderr, ...)for consistency with the panic logging on Line 449.Also applies to: 190-193, 199-201, 429-431
254-260: Nit: redundantbool()cast on Line 257.
category.HasReplyFieldis alreadybool; the conversion is a no-op.- HasReplyField: bool(category.HasReplyField), + HasReplyField: category.HasReplyField,
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@leaanthony I think we are in a good spot. Tested this on macOS, Windows, and Linux and it looks to be working as expected. |
|
I have a local version working with JS api wired up. Is that something we want to include? |
That would be amazing, HOWEVER, I just seen how long this PR has been running so if it's a reasonably small change I think it'd be worth it? |
|
@leaanthony added the JS stuff and had Claude help out with updating the docs/comments around it. Did a few passes, tested (quickly) on all 3 OSes, and I think we are good but would love another set of eyes |
|
Amazing 😍 Did you address the Rabbit's comments? I'm not sure if the conversation gets resolved automatically or if the mobile GitHub app even works properly with them 😩 |
|
I just did a second pass and tidied up the last that I could see of the nitpicks. Let me test again on the diff platforms when I get a chance |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@leaanthony tested the JS api on the 3 platforms and everything looks good to go on my end 👍 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@v2/internal/frontend/desktop/windows/winc/icon.go`:
- Around line 155-188: The code currently selects iconInfo.HbmColor into the
compatible DC (hdc) via procSelectObject and then calls procGetDIBits with that
same DC, which violates the Win32 requirement that the bitmap passed to
GetDIBits not be selected into a DC; change the flow to obtain a screen DC using
w32.GetDC(0) (and later release it with w32.ReleaseDC), remove/selecting the
bitmap into the compatible DC (remove the procSelectObject/select and its
defer), and call procGetDIBits using the screen DC instead of hdc (keep using
iconInfo.HbmColor, procGetDIBits, and BITMAPINFO setup as-is); ensure the screen
DC is released in a defer to avoid leaks.
In `@website/docs/reference/runtime/notification.mdx`:
- Around line 354-361: NotificationOptions and NotificationResponse use
inconsistent JSON keys for the same field (NotificationOptions has `categoryId`
while NotificationResponse has `categoryIdentifier`); update the struct tags to
be consistent (pick one canonical JSON key and apply it to both structs) or, if
the mismatch is intentional, add a clear comment in both structs
(NotificationOptions and NotificationResponse) documenting the reason for the
naming difference and the mapping clients should expect so JS/TS consumers
aren't confused.
🧹 Nitpick comments (5)
v2/internal/frontend/desktop/windows/winc/icon.go (1)
209-218: DeferredClosemay silently lose write errors.If
png.Encodesucceeds butoutFile.Close()fails (e.g., OS-level flush failure), the error is lost. Consider capturing the close error:Proposed fix
- // Create output file - outFile, err := os.Create(filePath) - if err != nil { - return err - } - defer outFile.Close() - - // Encode and save the image - return png.Encode(outFile, img) + // Create output file + outFile, err := os.Create(filePath) + if err != nil { + return err + } + + // Encode and save the image + if err := png.Encode(outFile, img); err != nil { + outFile.Close() + return err + } + return outFile.Close()v2/internal/frontend/desktop/windows/notifications.go (1)
27-39: Consider documenting thread-safety expectations for the global state.The global variables (
categories,appName,appGUID,iconPath,exePath) are written duringInitializeNotificationsand read during send/register operations. Thecategoriesmap is protected bycategoriesLock, but the string variables (appName,appGUID,iconPath,exePath) are only written during init and then read — this is safe assumingInitializeNotificationsis called once at startup before any concurrent sends. A brief comment noting this "write-once at init" invariant would prevent future confusion.v2/internal/frontend/desktop/linux/notifications.go (3)
527-543: UseRLockinstead ofLockwhen only reading the callback.
handleNotificationResultonly readsnotificationResultCallback— it never writes to it. Using a write lock here unnecessarily blocks concurrent notification result delivery.♻️ Proposed fix
func handleNotificationResult(result frontend.NotificationResult) { - callbackLock.Lock() + callbackLock.RLock() callback := notificationResultCallback - callbackLock.Unlock() + callbackLock.RUnlock() if callback != nil {
129-134: Silently swallowedjson.Marshalerror for user data.If
json.Marshal(options.Data)fails, the notification is sent without the user's data and no error or log entry is produced. The same pattern appears inSendNotificationWithActions(lines 215-220). Consider at least logging the error so developers can diagnose missing data.♻️ Proposed fix (apply similarly at line 217)
if options.Data != nil { userData, err := json.Marshal(options.Data) - if err == nil { - hints["x-user-data"] = dbus.MakeVariant(string(userData)) + if err != nil { + f.logger.Warning("Failed to marshal notification user data: %v", err) + } else { + hints["x-user-data"] = dbus.MakeVariant(string(userData)) } }
78-89: Consider clearingnotificationsmap on cleanup to avoid stale entries on re-init.After
CleanupNotifications, stale entries in thenotificationsmap reference D-Bus IDs from the old connection. IfInitializeNotificationsis called again, a new D-Bus connection may reuse IDs, leading to incorrect response data. Clearing the map during cleanup avoids this edge case.♻️ Proposed fix
func (f *Frontend) CleanupNotifications() { if cancel != nil { cancel() cancel = nil } if conn != nil { conn.Close() conn = nil } + + notificationsLock.Lock() + notifications = make(map[uint32]*notificationData) + notificationsLock.Unlock() }
|
@leaanthony anything we need here? |
|
All good! Thanks for your patience! |
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
New Features
Build
Documentation
Chores