[v2] Notifications API #4256
[v2] Notifications API #4256
Conversation
|
Warning Rate limit exceeded@leaanthony has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 11 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a cross‑platform desktop notifications subsystem: new frontend types and runtime wrappers, platform implementations for macOS (Obj‑C bridge + cgo), Windows (toasts + registry/COM + icon helper), and Linux (D‑Bus), docs, a macOS post‑build codesign step, and an indirect Go 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
alt macOS
FE->>OS: Schedule UNNotificationRequest (UNUserNotificationCenter)
else Windows
FE->>OS: Show toast (COM activation args / registry)
else Linux
FE->>OS: Notify via D-Bus org.freedesktop.Notifications
end
Note over OS: Notification displayed
OS-->>FE: Activation / Action / Dismiss / Signal
FE->>RT: Normalize NotificationResult (async)
RT->>CB: Invoke registered callback with NotificationResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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. 👍
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? |
|
|
|
@leaanthony I fear our window for this came and went. Was so looking forward to closing #1788 😅. Should I go ahead and close? |
dosubot
bot
added
the
size:XXL
|
I was looking back at this and found a bug on Windows where if you don't pass arbitrary |
|
@leaanthony fixed en/decoding issue for this PR and opened #4450 for v3 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
v2/internal/frontend/desktop/darwin/WailsContext.m (4)
835-851: Auth status handling OK; consider provisional status (future-proofing)Optional: treat provisional/ephemeral (if ever exposed on macOS SDKs) as authorized. Can be deferred.
882-912: Default/fallback foridentifier(optional hardening)If
identifieris ever NULL, build a UUID to avoid a crash or invalid request.Apply this diff:
- NSString *nsIdentifier = [NSString stringWithUTF8String:identifier]; + NSString *nsIdentifier = identifier + ? [NSString stringWithUTF8String:identifier] + : [NSUUID UUID].UUIDString;
913-945: Sameidentifierhardening and error path LGTMMirror the
identifierNULL-guard here as well; otherwise looks solid.
947-1023: Support foreground actions (optional)Allow actions JSON to specify
"foreground": trueto bring the app to front when tapped.Apply this diff:
- BOOL destructive = [actionDict[@"destructive"] boolValue]; + BOOL destructive = [actionDict[@"destructive"] boolValue]; + BOOL foreground = [actionDict[@"foreground"] boolValue]; @@ - if (destructive) options |= UNNotificationActionOptionDestructive; + if (destructive) options |= UNNotificationActionOptionDestructive; + if (foreground) options |= UNNotificationActionOptionForeground;
📜 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 (2)
v2/go.mod(1 hunks)v2/internal/frontend/desktop/darwin/WailsContext.m(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/go.mod
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_windows.go:0-0
Timestamp: 2025-03-24T06:54:22.127Z
Learning: popaprozac prefers to focus on getting the Notifications API functionality working first and may consider code cleanup/refactoring for payload encoding logic in notifications_windows.go at a later time.
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
📚 Learning: 2025-03-23T00:41:39.612Z
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_darwin.go:39-46
Timestamp: 2025-03-23T00:41:39.612Z
Learning: For the macOS notifications implementation in Wails, an early panic is used when the bundle identifier check fails rather than returning an error, because the Objective-C code would crash later anyway. The panic provides clear instructions to developers about bundling and signing requirements.
Applied to files:
v2/internal/frontend/desktop/darwin/WailsContext.m
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (10)
v2/internal/frontend/desktop/darwin/WailsContext.m (10)
8-8: Include looks correctNo issues with the new include. Matches the added notification surface.
40-47: Confirm C-string lifetime in extern callbacks
UTF8Stringreturns a pointer only valid while the backing NSString is alive. Please confirm thatcaptureResultanddidReceiveNotificationResponsesynchronously copy their string arguments. This file elsewhere releases temporary NSStrings immediately after the callback, implying that contract.
729-736: Availability check LGTMClear, simple guard for macOS 10.14+.
738-745: Bundle identifier check is fine; ensure it’s enforced upstreamReturning NO is OK here. Please ensure the Go layer calls this early and fails fast (as v3 does), otherwise the Obj-C layer may crash later when scheduling notifications.
802-809: Delegate setup LGTMAssigning the
UNUserNotificationCenterdelegate each time is safer than a one-time capture and avoids dangling references.
811-833: Authorization request flow LGTMHappy path and error propagation via
captureResultlooks correct.
1025-1051: Category removal flow LGTMCorrectly reconciles the category set and reports result.
1052-1072: Removal APIs LGTMDirect calls to remove pending/delivered notifications look correct.
1083-1101: About dialog change is cosmeticNo functional concerns.
853-880: Nil-safety for required fields (title,body)
stringWithUTF8String:can crash if passed a NULL pointer. Guardtitleandbodysimilarly tosubtitle.Apply this diff:
- NSString *nsTitle = [NSString stringWithUTF8String:title]; - NSString *nsSubtitle = subtitle ? [NSString stringWithUTF8String:subtitle] : @""; - NSString *nsBody = [NSString stringWithUTF8String:body]; + if (title == NULL) title = ""; + if (body == NULL) body = ""; + if (subtitle == NULL) subtitle = ""; + + NSString *nsTitle = [NSString stringWithUTF8String:title] ?: @""; + NSString *nsSubtitle = [NSString stringWithUTF8String:subtitle] ?: @""; + NSString *nsBody = [NSString stringWithUTF8String:body] ?: @"";
| - (void)userNotificationCenter:(UNUserNotificationCenter *)center | ||
| willPresentNotification:(UNNotification *)notification | ||
| withCompletionHandler:(void (^)(UNNotificationPresentationOptions options))completionHandler API_AVAILABLE(macos(10.14)) { | ||
| UNNotificationPresentationOptions options = 0; | ||
|
|
||
| if (@available(macOS 11.0, *)) { | ||
| // These options are only available in macOS 11.0+ | ||
| options = UNNotificationPresentationOptionList | | ||
| UNNotificationPresentationOptionBanner | | ||
| UNNotificationPresentationOptionSound; | ||
| } | ||
|
|
||
| completionHandler(options); | ||
| } | ||
|
|
There was a problem hiding this comment.
Foreground notifications are suppressed on 10.14–10.15 — add fallback
When the app is in the foreground, pre–Big Sur should use Alert | Sound. Returning 0 hides the notification entirely.
Apply this diff:
- UNNotificationPresentationOptions options = 0;
-
- if (@available(macOS 11.0, *)) {
+ UNNotificationPresentationOptions options;
+ if (@available(macOS 11.0, *)) {
// These options are only available in macOS 11.0+
options = UNNotificationPresentationOptionList |
UNNotificationPresentationOptionBanner |
UNNotificationPresentationOptionSound;
- }
-
+ } else {
+ options = UNNotificationPresentationOptionAlert | UNNotificationPresentationOptionSound;
+ }
+
completionHandler(options);📝 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.
| - (void)userNotificationCenter:(UNUserNotificationCenter *)center | |
| willPresentNotification:(UNNotification *)notification | |
| withCompletionHandler:(void (^)(UNNotificationPresentationOptions options))completionHandler API_AVAILABLE(macos(10.14)) { | |
| UNNotificationPresentationOptions options = 0; | |
| if (@available(macOS 11.0, *)) { | |
| // These options are only available in macOS 11.0+ | |
| options = UNNotificationPresentationOptionList | | |
| UNNotificationPresentationOptionBanner | | |
| UNNotificationPresentationOptionSound; | |
| } | |
| completionHandler(options); | |
| } | |
| - (void)userNotificationCenter:(UNUserNotificationCenter *)center | |
| willPresentNotification:(UNNotification *)notification | |
| withCompletionHandler:(void (^)(UNNotificationPresentationOptions options))completionHandler API_AVAILABLE(macos(10.14)) { | |
| UNNotificationPresentationOptions options; | |
| if (@available(macOS 11.0, *)) { | |
| // These options are only available in macOS 11.0+ | |
| options = UNNotificationPresentationOptionList | | |
| UNNotificationPresentationOptionBanner | | |
| UNNotificationPresentationOptionSound; | |
| } else { | |
| options = UNNotificationPresentationOptionAlert | UNNotificationPresentationOptionSound; | |
| } | |
| completionHandler(options); | |
| } |
🤖 Prompt for AI Agents
In v2/internal/frontend/desktop/darwin/WailsContext.m around lines 746 to 760,
the willPresentNotification handler currently sets
UNNotificationPresentationOptions to 0 for pre-Big Sur macOS (10.14–10.15) which
suppresses foreground notifications; change the logic so that if
@available(macOS 11.0, *) you set List | Banner | Sound as before, otherwise set
UNNotificationPresentationOptionAlert | UNNotificationPresentationOptionSound
(the Alert|Sound fallback) before calling completionHandler(options); ensure you
reference the correct enum constants for older macOS SDKs and keep the
availability checks intact.
| - (void)userNotificationCenter:(UNUserNotificationCenter *)center | ||
| didReceiveNotificationResponse:(UNNotificationResponse *)response | ||
| withCompletionHandler:(void (^)(void))completionHandler API_AVAILABLE(macos(10.14)) { | ||
|
|
||
| NSMutableDictionary *payload = [NSMutableDictionary dictionary]; | ||
|
|
||
| [payload setObject:response.notification.request.identifier forKey:@"id"]; | ||
| [payload setObject:response.actionIdentifier forKey:@"actionIdentifier"]; | ||
| [payload setObject:response.notification.request.content.title ?: @"" forKey:@"title"]; | ||
| [payload setObject:response.notification.request.content.body ?: @"" forKey:@"body"]; | ||
|
|
||
| if (response.notification.request.content.categoryIdentifier) { | ||
| [payload setObject:response.notification.request.content.categoryIdentifier forKey:@"categoryIdentifier"]; | ||
| } | ||
|
|
||
| if (response.notification.request.content.subtitle) { | ||
| [payload setObject:response.notification.request.content.subtitle forKey:@"subtitle"]; | ||
| } | ||
|
|
||
| if (response.notification.request.content.userInfo) { | ||
| [payload setObject:response.notification.request.content.userInfo forKey:@"userInfo"]; | ||
| } | ||
|
|
||
| if ([response isKindOfClass:[UNTextInputNotificationResponse class]]) { | ||
| UNTextInputNotificationResponse *textResponse = (UNTextInputNotificationResponse *)response; | ||
| [payload setObject:textResponse.userText forKey:@"userText"]; | ||
| } | ||
|
|
||
| NSError *error = nil; | ||
| NSData *jsonData = [NSJSONSerialization dataWithJSONObject:payload options:0 error:&error]; | ||
| if (error) { | ||
| NSString *errorMsg = [NSString stringWithFormat:@"Error: %@", [error localizedDescription]]; | ||
| didReceiveNotificationResponse(NULL, [errorMsg UTF8String]); | ||
| } else { | ||
| NSString *jsonString = [[NSString alloc] initWithData:jsonData encoding:NSUTF8StringEncoding]; | ||
| didReceiveNotificationResponse([jsonString UTF8String], NULL); | ||
| } | ||
|
|
||
| completionHandler(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Memory leak: jsonString is never released (MRC build)
[[NSString alloc] initWithData:...] returns a retained object. Autorelease it (consistent with the rest of the file) to avoid leaking on every response.
Apply this diff:
- NSString *jsonString = [[NSString alloc] initWithData:jsonData encoding:NSUTF8StringEncoding];
+ NSString *jsonString = [[[NSString alloc] initWithData:jsonData encoding:NSUTF8StringEncoding] autorelease];
didReceiveNotificationResponse([jsonString UTF8String], NULL);🤖 Prompt for AI Agents
In v2/internal/frontend/desktop/darwin/WailsContext.m around lines 761 to 801,
the NSString *jsonString created with [[NSString alloc] initWithData:encoding:]
is retained and never released under MRC; change its creation to return an
autoreleased object (consistent with the rest of the file) by calling
autorelease on the allocated string (or alternatively release it after calling
didReceiveNotificationResponse) so the string is not leaked.
leaanthony
left a comment
leaanthony
left a comment
There was a problem hiding this comment.
Amazing stuff @popaprozac! Could you please update the docs with the new options/settings please? 🙏 Oh and also update the Changelog :-)
|
Hello! Of course. I've been away from this for a bit but will get to it this week! |
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
website/docs/guides/notifications.mdx(1 hunks)website/docs/reference/runtime/notification.mdx(1 hunks)website/src/pages/changelog.mdx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_windows.go:0-0
Timestamp: 2025-03-24T06:54:22.127Z
Learning: popaprozac prefers to focus on getting the Notifications API functionality working first and may consider code cleanup/refactoring for payload encoding logic in notifications_windows.go at a later time.
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
Applied to files:
website/docs/reference/runtime/notification.mdxwebsite/docs/guides/notifications.mdx
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
Applied to files:
website/docs/guides/notifications.mdx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: semgrep-cloud-platform/scan
|
@leaanthony I'm having some issues as I go through this. When I checkout this branch and use it locally I am running into some build constraint issues I can't for the life of me understand where the issue is but will update us when this is ready again for review. Had Claude help with the docs, will need to clean up its mistakes. Thanks 🐰 |
|
Looks like some recent changes to and then in Am I missing something? |
|
@leaanthony thoughts? |
|
|
I've got a fix for the build issue locally. Will see if I can push it tonight. If you see this before then, the fix is simply swapping the const references with const data e.g. 0x800... |
|
@leaanthony I'll take a look at this asap, thanks. Need to clean up documentation too... |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v2/internal/frontend/desktop/darwin/notifications.go (2)
65-67: Remove unused interface.The
ChannelHandlerinterface is defined but never used. TheGetChannelmethod is implemented directly on*Frontendwithout implementing this interface.🔎 Proposed removal
-type ChannelHandler interface { - GetChannel(id int) (chan notificationChannel, bool) -} -
390-398: Consider adding panic recovery to callback goroutine.The callback is invoked in a goroutine without panic recovery. If user-provided callback code panics, it will crash the application.
🔎 Proposed addition of panic recovery
func handleNotificationResult(result frontend.NotificationResult) { callbackLock.Lock() callback := notificationResultCallback callbackLock.Unlock() if callback != nil { - go callback(result) + go func() { + defer func() { + if r := recover(); r != nil { + // Log panic but don't crash the app + fmt.Fprintf(os.Stderr, "panic in notification callback: %v\n", r) + } + }() + callback(result) + }() } }Note: You'll need to import
osif you add logging.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/internal/frontend/desktop/darwin/notifications.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_windows.go:0-0
Timestamp: 2025-03-24T06:54:22.127Z
Learning: popaprozac prefers to focus on getting the Notifications API functionality working first and may consider code cleanup/refactoring for payload encoding logic in notifications_windows.go at a later time.
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_darwin.go:39-46
Timestamp: 2025-03-23T00:41:39.612Z
Learning: For the macOS notifications implementation in Wails, an early panic is used when the bundle identifier check fails rather than returning an error, because the Objective-C code would crash later anyway. The panic provides clear instructions to developers about bundling and signing requirements.
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_windows.go:91-113
Timestamp: 2025-02-24T06:08:55.645Z
Learning: The JSON quote handling in Windows notifications' activation arguments (v3/pkg/services/notifications/notifications_windows.go) has a known limitation with single-quote collisions that needs to be addressed after initial testing.
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
Applied to files:
v2/internal/frontend/desktop/darwin/notifications.go
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
Applied to files:
v2/internal/frontend/desktop/darwin/notifications.go
📚 Learning: 2025-03-23T00:41:39.612Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_darwin.go:39-46
Timestamp: 2025-03-23T00:41:39.612Z
Learning: For the macOS notifications implementation in Wails, an early panic is used when the bundle identifier check fails rather than returning an error, because the Objective-C code would crash later anyway. The panic provides clear instructions to developers about bundling and signing requirements.
Applied to files:
v2/internal/frontend/desktop/darwin/notifications.go
📚 Learning: 2025-04-29T23:54:07.488Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4256
File: v2/internal/frontend/desktop/linux/notifications.go:27-28
Timestamp: 2025-04-29T23:54:07.488Z
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.
Applied to files:
v2/internal/frontend/desktop/darwin/notifications.go
🧬 Code graph analysis (1)
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/pkg/runtime/notifications.go (18)
NotificationResult(23-23)InitializeNotifications(25-28)IsNotificationAvailable(30-33)RequestNotificationAuthorization(35-38)CheckNotificationAuthorization(40-43)SendNotification(45-48)NotificationOptions(10-10)SendNotificationWithActions(50-53)RegisterNotificationCategory(55-58)NotificationCategory(16-16)RemoveNotificationCategory(60-63)RemoveAllPendingNotifications(65-68)RemovePendingNotification(70-73)RemoveAllDeliveredNotifications(75-78)RemoveDeliveredNotification(80-83)RemoveNotification(85-88)OnNotificationResponse(90-93)NotificationResponse(19-19)v2/internal/frontend/desktop/windows/notifications.go (1)
DefaultActionIdentifier(38-38)v2/internal/frontend/desktop/linux/notifications.go (1)
DefaultActionIdentifier(44-44)
🔇 Additional comments (13)
v2/internal/frontend/desktop/darwin/notifications.go (13)
1-27: LGTM: Build setup and imports are correct.The build constraints, cgo configuration, and framework linking are properly configured for macOS notifications. The conditional linking of UserNotifications for macOS 11.0+ is appropriate.
29-58: LGTM: Thread-safe state management.The package-level state is properly protected with appropriate mutexes. The use of
sync.RWMutexforcurrentFrontendis well-suited for the read-heavy access pattern.
69-94: LGTM: Proper initialization sequence.The initialization validates system availability and bundle identifier before setting up the notification infrastructure. Error handling is appropriate for the v2 API surface.
96-128: LGTM: Authorization flow is well-structured.The authorization methods properly use timeouts and channels for async operations. The timeout durations (3 minutes for request, 15 seconds for check) are reasonable.
131-216: LGTM: Notification sending methods are correct.Both
SendNotificationandSendNotificationWithActionsproperly manage C memory with deferred cleanup, handle JSON marshaling errors, and use appropriate timeouts for async operations.
220-285: LGTM: Category management is properly implemented.The category registration correctly handles optional reply fields with conditional C string allocation, and JSON marshaling for actions is properly error-checked.
315-324: LGTM: Platform-specific stub is well-documented.The stub correctly maintains cross-platform interface compatibility. The comment clearly directs users to platform-specific alternatives.
326-330: LGTM: Callback registration is thread-safe.
332-353: LGTM: C callback bridge is correctly implemented.The exported function safely handles nil frontend and missing channels. The channel cleanup (via
GetChannel) prevents double-delivery.
355-388: LGTM: Response parsing and error handling are thorough.The notification response handler properly validates the payload, unmarshals JSON, and normalizes Apple's default action identifier for cross-platform consistency. Error handling covers all failure modes.
402-419: LGTM: Channel registration is thread-safe.The defensive re-initialization of the channels map (lines 407-410) provides safety if methods are called before
InitializeNotifications. The buffered channel (size 1) prevents goroutine leaks.
421-448: LGTM: Channel cleanup is safe.
GetChannelcorrectly deletes the channel before returning it, preventing double-delivery.cleanupChannelsafely handles nil map and missing channels.
287-313: These removal methods correctly implement the underlying macOS API.The unconditional
nilreturn is appropriate because the underlying Objective-C functions do not return errors, and internal OS-level failures are ignored. This pattern matches the v3 design where removal operations are fire-and-forget. No changes needed.
|
@leaanthony I've been super busy but think I got this back where it needs to be. Lmk if I am missing anything critical |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
website/docs/reference/runtime/notification.mdx (1)
165-167: Linux reply-field support incorrectly marked as supported.The table shows Linux supports
HasReplyField,ReplyPlaceholder, andReplyButtonTitle(✅), but line 277 explicitly states "User text input not supported" for Linux. TheNotificationResponsetable (line 220) correctly shows Linux does NOT supportUserText. Remove the ✅ marks in the Linux column for these three reply-related fields to match the documented limitations.🔎 Proposed fix
| Field | Description | Win | Mac | Lin | |------------------|------------------------------------------------|-----|-----|-----| | ID | Unique identifier for the category | ✅ | ✅ | ✅ | | Actions | Array of action buttons | ✅ | ✅ | ✅ | -| HasReplyField | Whether to include a text input field | ✅ | ✅ | ✅ | -| ReplyPlaceholder | Placeholder text for the input field | ✅ | ✅ | ✅ | -| ReplyButtonTitle | Text for the reply button | ✅ | ✅ | ✅ | +| HasReplyField | Whether to include a text input field | ✅ | ✅ | | +| ReplyPlaceholder | Placeholder text for the input field | ✅ | ✅ | | +| ReplyButtonTitle | Text for the reply button | ✅ | ✅ | |
🧹 Nitpick comments (1)
website/docs/reference/runtime/notification.mdx (1)
17-17: Consider adding subject for grammatical completeness.The phrase "Should be called during app startup" is grammatically incomplete. Consider changing it to "It should be called during app startup" for better readability.
🔎 Proposed fix
-Initializes the notification system. Should be called during app startup. +Initializes the notification system. It should be called during app startup.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/docs/guides/notifications.mdxwebsite/docs/reference/runtime/notification.mdx
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_windows.go:0-0
Timestamp: 2025-03-24T06:54:22.127Z
Learning: popaprozac prefers to focus on getting the Notifications API functionality working first and may consider code cleanup/refactoring for payload encoding logic in notifications_windows.go at a later time.
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_darwin.go:39-46
Timestamp: 2025-03-23T00:41:39.612Z
Learning: For the macOS notifications implementation in Wails, an early panic is used when the bundle identifier check fails rather than returning an error, because the Objective-C code would crash later anyway. The panic provides clear instructions to developers about bundling and signing requirements.
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_windows.go:91-113
Timestamp: 2025-02-24T06:08:55.645Z
Learning: The JSON quote handling in Windows notifications' activation arguments (v3/pkg/services/notifications/notifications_windows.go) has a known limitation with single-quote collisions that needs to be addressed after initial testing.
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
Applied to files:
website/docs/guides/notifications.mdxwebsite/docs/reference/runtime/notification.mdx
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
Applied to files:
website/docs/guides/notifications.mdxwebsite/docs/reference/runtime/notification.mdx
📚 Learning: 2025-04-29T23:54:07.488Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4256
File: v2/internal/frontend/desktop/linux/notifications.go:27-28
Timestamp: 2025-04-29T23:54:07.488Z
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.
Applied to files:
website/docs/guides/notifications.mdx
📚 Learning: 2025-03-23T00:41:39.612Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_darwin.go:39-46
Timestamp: 2025-03-23T00:41:39.612Z
Learning: For the macOS notifications implementation in Wails, an early panic is used when the bundle identifier check fails rather than returning an error, because the Objective-C code would crash later anyway. The panic provides clear instructions to developers about bundling and signing requirements.
Applied to files:
website/docs/guides/notifications.mdx
📚 Learning: 2025-03-24T06:54:22.127Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_windows.go:0-0
Timestamp: 2025-03-24T06:54:22.127Z
Learning: popaprozac prefers to focus on getting the Notifications API functionality working first and may consider code cleanup/refactoring for payload encoding logic in notifications_windows.go at a later time.
Applied to files:
website/docs/guides/notifications.mdx
📚 Learning: 2025-02-24T06:23:10.967Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications_windows.go:113-164
Timestamp: 2025-02-24T06:23:10.967Z
Learning: In Windows Toast notifications (notifications_windows.go), metadata must be appended to each action's Arguments because the toast activated callback only returns the Arguments string of the clicked action. This is achieved by appending the same escaped JSON data (with single quotes) to all actions using the ":::" delimiter.
Applied to files:
website/docs/reference/runtime/notification.mdx
🪛 LanguageTool
website/docs/reference/runtime/notification.mdx
[style] ~17-~17: To form a complete sentence, be sure to include a subject.
Context: ...s Initializes the notification system. Should be called during app startup. Go: `Ini...
(MISSING_IT_THERE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run Go Tests (macos-latest, 1.23)
- GitHub Check: Run Go Tests (ubuntu-24.04, 1.23)
- GitHub Check: Run Go Tests (ubuntu-22.04, 1.23)
- GitHub Check: Run Go Tests (windows-latest, 1.23)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
website/docs/guides/notifications.mdx (1)
1-233: Excellent comprehensive guide for the Notifications API.This documentation provides a thorough, well-structured guide covering all aspects of the cross-platform notifications system. The code examples demonstrate proper error handling, the platform considerations are accurate and consistent, and the best practices section provides valuable guidance. The progressive structure from basic to advanced usage makes it easy to follow.
website/docs/reference/runtime/notification.mdx (1)
1-319: API reference documentation is comprehensive and well-structured.The documentation thoroughly covers all runtime functions, types, platform-specific behavior, and action identifiers with clear Go signatures and descriptions. The examples demonstrate proper usage patterns, and the platform considerations help developers understand cross-platform differences. After addressing the Linux reply-field inconsistency, this will be an excellent reference.
|
2 participants
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
✏️ Tip: You can customize this high-level summary in your review settings.