feat(v3:notifications): add sounds, attachments, and more#5333
feat(v3:notifications): add sounds, attachments, and more#5333popaprozac wants to merge 21 commits into
Conversation
…oast Drop the dependency on the high-level go-toast/v2 builder package (git.sr.ht/~jackmordaunt/go-toast/v2) in favour of constructing the toast XML ourselves with encoding/xml and pushing it through the already-used wintoast subpackage. The wintoast subpackage exposes SetAppData, SetActivationCallback, and Push as public API, so no further linkname shims were needed beyond the existing registerFactoryInternal / wintoast.ClassFactory wiring at service Startup (kept so cold-start clicks on toasts pinned in Action Center from a previous run still dispatch to OnNotificationResponse). Behaviour-preserving change. The encoded base64 payload that round-trips NotificationOptions through every <action arguments="..."> attribute and the toast's launch="..." attribute is unchanged, as is the <input id="userText"> channel for reply text. Actions, categories, and ActivationType=foreground all match the previous template output. Net effect: encoding/xml now handles attribute escaping (a small robustness win for titles/bodies containing < > & or quotes), and the Windows backend is positioned to wire the new sound, image, threading, scenario, schedule, and tag-based update fields directly into XML in a follow-up commit without relying on the upstream go-toast struct exposing those fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cross the cgo boundary The send-notification C entry points used to take five separate const-char* parameters (identifier, title, subtitle, body, data_json). Replace them with a single options_json blob produced by json.Marshal of the whole NotificationOptions struct, parsed in Objective-C with NSJSONSerialization. Behaviour-preserving change: title/subtitle/body/userInfo are still populated identically, the default sound is still applied, and the identifier/categoryId still flow through to UNNotificationRequest. New helper functions parseOptions and stringOrEmpty in the .m provide safe typed access; createNotificationContentFromOptions replaces the previous per-arg createNotificationContent. Net effect: future fields (sound, attachments, threadId, interruptionLevel, schedule) can be wired in by reading additional keys from the same dict in createNotificationContentFromOptions and the follow-up trigger path, without growing C signatures or adding more C.CString churn on the Go side. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nLevel, Schedule, and UpdateNotification
Extend NotificationOptions with five additive fields plus a new
UpdateNotification method on NotificationService. Each new field
gracefully degrades on platforms with limited backend support; see the
package godoc for the per-platform support matrix.
Sound: nil = platform default, {silent: true} = no sound,
{name: "..."} = bundled/system named sound. macOS uses
[UNNotificationSound soundNamed:]; Windows emits <audio src="..."/>
(ms-winsoundevent: or ms-appx: scheme); Linux forwards as the freedesktop
sound-name / suppress-sound hints.
Attachments: media files shown inline. macOS supports any UTI via
UNNotificationAttachment; Windows wires the first entry to a
<image placement="hero|appLogoOverride|inline"/> binding; Linux forwards
the first entry as the freedesktop image-path hint.
ThreadID: groups related notifications. macOS sets threadIdentifier;
Windows emits a <header/> element; Linux uses the freedesktop category
hint plus an x-wails-thread-id hint for round-tripping.
InterruptionLevel: passive | active | timeSensitive | critical.
macOS 12+ maps to UNNotificationInterruptionLevel (critical needs the
Critical Alert entitlement). Windows maps to <toast scenario="..."/>
(reminder/urgent). Linux maps to the freedesktop urgency hint.
Schedule: deferred delivery via {delaySeconds} or {at} (Unix seconds).
macOS uses native UNTimeInterval/UNCalendar triggers and persists across
app restarts. Windows + Linux fall back to in-process time.AfterFunc
timers tracked by ID for cancellation via RemovePendingNotification;
these timers are lost on app exit (documented limitation).
UpdateNotification(options): re-posts a notification by ID. macOS auto-
deduplicates on identifier; Linux uses the D-Bus replaces_id parameter
(notify() now scans tracked notifications for a matching options.ID and
threads it through, so SendNotification with a known ID also updates in
place). Windows currently redelivers as a new notification; true replace
requires upstream wintoast tag/group support, tracked as a follow-up.
Validator coverage: invalid InterruptionLevel, schedule with both
fields, schedule with neither, negative schedule values, missing-file
attachments, and empty attachment paths each surface as errors at the
service entry point. Unit tests in notifications_test.go cover every
new path. Existing required-field validation is preserved.
Frontend bindings regenerated; UNRELEASED_CHANGELOG entries added under
Added and Changed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…field Rewrite the example so the test app can exercise sounds, attachments, threading, scheduling, interruption levels, and UpdateNotification directly from the UI on every platform. main.go now bundles the wails logo as a sample image and writes it to os.TempDir() at startup, then exposes the absolute path through a DemoAssets helper service. The frontend uses that path in the NotificationOptions.Attachments field so the demo can attach a real file across macOS/Windows/Linux without a file picker or an extra filesystem dependency. The frontend (index.html + main.ts + style.css) is split into three sections: * Authorization — request and check (macOS gate; Windows/Linux are always-true stubs). * Quick tests — Basic, Complex, Threaded (two with the same threadId), Scheduled (5s delay), Update demo (post → 2s → update), and Cancel last scheduled. * Custom builder — a form for title/subtitle/body/threadId/sound/level/ delay plus checkboxes for sample image attachment and action category. Buttons send / update by last id / remove. Bindings regenerated; the new NotificationSound / NotificationAttachment / NotificationSchedule classes plus interruptionLevel / threadId / schedule / sound / attachments / UpdateNotification entries are checked in. package-lock.json is committed too, matching the convention used by other examples (dock, file-association, etc.) so frontend installs are reproducible inside test VMs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…hint
The new NotificationAttachment.Type field carries different meanings per
platform: on Windows it's a placement hint ("hero"/"appLogoOverride"/
"inline"), on macOS it's an optional UTI such as "public.png". The
example was sending type="hero" by default, which on macOS was passed
through to UNNotificationAttachmentOptionsTypeHintKey verbatim — and
because "hero" is not a valid UTI, UNNotificationAttachment refused to
build the attachment and silently dropped it.
Two-part fix:
1. notifications_darwin.m only forwards Type as a UTI hint when it
actually looks like one (UTI strings always contain a "."). For any
other value we omit the hint and let UNUserNotificationCenter
auto-infer the type from the file extension. Documented inline.
2. The example now sends attachments without a Type, so macOS auto-
infers and Windows uses the inline default. Hero/appLogoOverride
placements still work — callers just opt in by passing the
corresponding Type explicitly.
Also: split the example's status messages out of the response container
into a dedicated #status div, so the notification-response table that
appears after a click/action/reply persists across subsequent button
presses instead of being clobbered by status text.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… attach reuse, IPv4 binding)
1. Notification response handler crashes after click
The current @wailsio/runtime delivers the emitted payload directly as
WailsEvent.data; older runtimes wrapped it in [data]. The previously
committed handler destructured response.data[0], which is undefined on
the new shape and threw "Cannot destructure property 'userInfo' from
null or undefined value" the first time the user clicked a delivered
notification — masking every subsequent interaction. The handler now
accepts both shapes and bails with a console warning if the payload
is genuinely empty.
2. Attaching the sample image only worked once per app launch
macOS UNNotificationAttachment moves the source file into the system's
notification database after delivery. The DemoAssets helper wrote the
bundled wails.png to TempDir once at startup, so the second send found
the path stale and validateNotificationOptions rejected it with a 422
("no such file or directory"). DemoAssets.SampleImagePath now rewrites
the file on every call, keeping subsequent sends self-healing without
requiring a service restart.
3. wails3 dev opens a blank window with "connection refused"
Recent Node resolves the literal hostname "localhost" to ::1 first,
so vite was binding IPv6-only (TCP [::1]:<port>). The wails3 dev
asset proxy explicitly forces tcp4 (to dodge IPv6 issues on Windows),
which produced "dial tcp4 127.0.0.1:<port>: connect: connection
refused" the moment the WebView asked for /, even though preRun's
probe (default dual-stack dialer) reported the dev server was up.
Pin vite to --host 127.0.0.1 in the example's dev:frontend task so
it binds IPv4 and the proxy can reach it.
Other examples in v3/examples/*/build/Taskfile.yml have the same shape
and will likely hit this on similar Node versions; fixing them is out
of scope for this PR but worth a follow-up.
Bindings regenerated to track the SampleImagePath signature change
(now also returns an error from the rewrite); the TS surface is
unchanged because Wails serialises errors through a separate channel.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…attach works The previous fix made DemoAssets.SampleImagePath rewrite the file on the Go side every call, but the frontend was still caching the FIRST promise returned by the binding and reusing it on every subsequent attach — so the Go-side rewrite never re-fired after the initial send. macOS moved the file out of TempDir on delivery, the cached path went stale, and validateNotificationOptions rejected the second send with 422 "no such file or directory". Drop the sampleImagePath cache; let every checkbox-attach round-trip to the Go side so the file is rewritten in place each time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
scheduleDelay was defined in notifications_windows.go (build-tagged //go:build windows) but referenced from notifications_linux.go for the in-process schedule fallback, so the Linux build failed with "undefined: scheduleDelay". Move the helper to notifications.go where it has no build tag and is visible to every backend that wants the in-process schedule fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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:
WalkthroughAdds richer notification options (sound, attachments, thread ID, interruption level, schedule) and an UpdateNotification API; validates new fields; macOS bridge accepts a JSON options blob; Windows builds toast XML via wintoast and adds scheduling; Linux adds D-Bus hints and per-ID scheduling; frontend examples, bindings, and tests updated. ChangesNotification Enhancement & Update Support
Sequence DiagramsequenceDiagram
participant User as User (UI)
participant Frontend as Frontend App
participant Service as Go Service
participant Platform as Platform Layer
participant Native as Native OS API
rect rgba(100, 150, 200, 0.5)
Note over User,Native: Send Notification Flow
User->>Frontend: Click "Send" with options
Frontend->>Frontend: Build NotificationOptions
Frontend->>Service: SendNotification(options)
Service->>Service: Validate options
alt Schedule present
Service->>Platform: Park timer
Platform->>Platform: Timer fires
Platform->>Native: Push notification
else Immediate
Service->>Platform: Push immediately
Platform->>Native: Push notification
end
Native-->>Service: Acknowledged
Service-->>Frontend: Resolve / return ID
end
rect rgba(150, 100, 200, 0.5)
Note over User,Native: Update Notification Flow
User->>Frontend: Click "Update" with ID
Frontend->>Service: UpdateNotification(options with ID)
Service->>Service: Validate options
Service->>Platform: Re-dispatch with same ID
Platform->>Native: Replace or re-send notification
Native-->>Platform: Acknowledged
Platform-->>Frontend: Success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
popaprozac
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
v3/examples/notifications/main.go (2)
38-41: ⚡ Quick win
newDemoAssetscan never return a non-nil error — dead code at the call site.The function body only calls
filepath.Join(os.TempDir(), ...), which never fails.os.TempDir()returns a string without an error return, andfilepath.Joinis infallible, so the(error)in the return signature is vacuous and thelog.Fatalfblock (lines 67–69) is unreachable.Either drop the error return and simplify the call site, or move the
os.WriteFilecall here (making the error meaningful at startup):♻️ Option A — drop vacuous error return
-func newDemoAssets() (*DemoAssets, error) { +func newDemoAssets() *DemoAssets { path := filepath.Join(os.TempDir(), "wails-notifications-sample.png") - return &DemoAssets{sampleImagePath: path}, nil + return &DemoAssets{sampleImagePath: path} }- demo, err := newDemoAssets() - if err != nil { - log.Fatalf("failed to prepare demo assets: %v", err) - } + demo := newDemoAssets()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/examples/notifications/main.go` around lines 38 - 41, The newDemoAssets function currently returns an unused error because filepath.Join and os.TempDir cannot fail; either remove the error from newDemoAssets' signature and return *DemoAssets only (then update its call site to stop checking an error), or make the error meaningful by creating/writing the sample image inside newDemoAssets (use os.WriteFile to write to sampleImagePath and return any write error) so callers (the log.Fatalf at the current call site) can handle it; refer to the function newDemoAssets and the DemoAssets.sampleImagePath field when making the change.
52-57: ⚡ Quick win
SampleImagePathwrites to a shared path without synchronisation.Concurrent frontend invocations (e.g. the user clicks "Send" twice before the first call returns) will both call
os.WriteFileon the same path simultaneously.os.WriteFileis not guaranteed to be atomic — it opens, truncates, then writes — so a concurrent reader (the OS notification system) could observe a truncated file between those steps.In practice the risk is low because both writes carry identical bytes, but since this is example/reference code a
sync.Mutexmakes the intent explicit:🔒 Proposed fix
type DemoAssets struct { sampleImagePath string + mu sync.Mutex }func (d *DemoAssets) SampleImagePath() (string, error) { + d.mu.Lock() + defer d.mu.Unlock() if err := os.WriteFile(d.sampleImagePath, sampleImage, 0o644); err != nil { return "", fmt.Errorf("write sample image: %w", err) } return d.sampleImagePath, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/examples/notifications/main.go` around lines 52 - 57, SampleImagePath writes to d.sampleImagePath without synchronization, allowing concurrent calls to os.WriteFile to interleave and expose truncated content; add a mutex on DemoAssets (e.g., sampleImageMu or writeMu) and lock/unlock it inside SampleImagePath around the write to serialize concurrent writes (acquire before os.WriteFile and release after) so multiple invocations of SampleImagePath cannot write the same path concurrently; update the DemoAssets struct to include the mutex and use it in SampleImagePath.v3/pkg/services/notifications/notifications.go (1)
167-171: ⚡ Quick win
NotificationSoundallows an ambiguous{Silent: true, Name: "non-empty"}combination without validationThe three documented uses (
nil,{Silent:true},{Name:"…"}) are mutually exclusive, but no validation enforces this. Callers who set both fields get platform-dependent, undocumented behavior (whichever field the backend prioritises).♻️ Proposed validation addition inside `validateNotificationOptions`
+ if options.Sound != nil && options.Sound.Silent && options.Sound.Name != "" { + return fmt.Errorf("notification sound: Silent and Name are mutually exclusive") + } + if options.InterruptionLevel != "" {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/pkg/services/notifications/notifications.go` around lines 167 - 171, Add mutual-exclusivity validation for NotificationSound inside validateNotificationOptions: if opts.Sound != nil and opts.Sound.Silent is true while opts.Sound.Name is non-empty, return a validation error (e.g., "NotificationSound: Silent and Name are mutually exclusive"); implement this check in the validateNotificationOptions function so callers cannot pass {Silent:true, Name:"…"} and platform-dependent behavior is avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v3/examples/notifications/frontend/src/main.ts`:
- Around line 299-348: The handler registered with
Events.On("notification:action") builds HTML using string interpolation and
assigns it to footer.innerHTML, which allows XSS via
response.data/userInfo/userText; change the rendering to construct DOM nodes
(document.createElement, appendChild) and set textContent for each key/value (or
run every interpolated value through a proper escaping helper) instead of using
innerHTML, and update the final assignment to append the constructed node(s) to
the footer element rather than assigning footer.innerHTML.
In `@v3/pkg/services/notifications/notifications_linux.go`:
- Around line 118-124: When scheduling a delayed notification via
ln.parkSchedule (the block that calls ln.parkSchedule(options.ID,... ) before
returning), ensure any existing parked timer for the same notification ID is
cancelled/cleared before creating a new one and make the parked callback guard
against races by validating it is still the active entry before deleting or
invoking SendNotification; in practice: in the code paths that create a new
parked timer (parkSchedule), first cancel/remove any existing parked timer for
options.ID, and add a generation token or compare the stored timer entry inside
the callback so an old callback cannot delete or run for a newer timer; apply
the same cancellation/guarding logic to the other similar blocks (around lines
for 147-153 and 288-303) that reuse notification IDs.
In `@v3/pkg/services/notifications/notifications_test.go`:
- Around line 112-141: validateNotificationOptions currently calls os.Stat on
every NotificationAttachment.Path which breaks macOS `file://` URL semantics;
update validateNotificationOptions to detect `file://` URIs (e.g.,
strings.HasPrefix(path, "file://")) and on darwin either convert the URL to a
local filesystem path (URL unescape and drop the scheme) before calling os.Stat
or skip os.Stat for file:// entries and rely on the Darwin bridge, and add a
regression test in TestValidateNotificationOptions_Attachments that creates a
temp file, constructs a `file://` URL to it, and asserts
validateNotificationOptions accepts that attachment on darwin.
In `@v3/pkg/services/notifications/notifications_windows.go`:
- Around line 184-205: The scheduled timer logic in sendOrSchedule causes
ID-collision bugs: a previously-fired timer can delete a newer timer and the
no-schedule path never clears an existing parked timer. Fix by (1) when creating
timer capture the specific timer value and in the callback hold scheduledLock
and only delete wn.scheduledTimers[options.ID] if the current map entry equals
that captured timer (ensuring older callbacks don't remove newer timers); (2)
when falling through to the no-delay path (in sendOrSchedule before calling
pushNow) stop and remove any existing timer for the same NotificationOptions.ID
under scheduledLock so a parked timer cannot fire after an immediate send; keep
use of scheduleDelay, pushNow, wn.scheduledTimers and wn.scheduledLock as the
referenced symbols.
In `@v3/pkg/services/notifications/notifications.go`:
- Around line 362-368: The validation loop for options.Attachments uses os.Stat
directly on NotificationAttachment.Path which wrongly rejects documented
"file://" URLs (e.g., "file:///Users/...") because os.Stat treats them as
literal paths; update the loop in the attachments validation (the for i, a :=
range options.Attachments block) to detect and handle file URLs: if a.Path has
the "file://" scheme, parse it with net/url (url.Parse), extract the file path
(use u.Path, handling any percent-decoding if needed), and then call os.Stat on
that extracted path; otherwise keep the existing behavior and os.Stat the
original a.Path; return parse errors as before.
---
Nitpick comments:
In `@v3/examples/notifications/main.go`:
- Around line 38-41: The newDemoAssets function currently returns an unused
error because filepath.Join and os.TempDir cannot fail; either remove the error
from newDemoAssets' signature and return *DemoAssets only (then update its call
site to stop checking an error), or make the error meaningful by
creating/writing the sample image inside newDemoAssets (use os.WriteFile to
write to sampleImagePath and return any write error) so callers (the log.Fatalf
at the current call site) can handle it; refer to the function newDemoAssets and
the DemoAssets.sampleImagePath field when making the change.
- Around line 52-57: SampleImagePath writes to d.sampleImagePath without
synchronization, allowing concurrent calls to os.WriteFile to interleave and
expose truncated content; add a mutex on DemoAssets (e.g., sampleImageMu or
writeMu) and lock/unlock it inside SampleImagePath around the write to serialize
concurrent writes (acquire before os.WriteFile and release after) so multiple
invocations of SampleImagePath cannot write the same path concurrently; update
the DemoAssets struct to include the mutex and use it in SampleImagePath.
In `@v3/pkg/services/notifications/notifications.go`:
- Around line 167-171: Add mutual-exclusivity validation for NotificationSound
inside validateNotificationOptions: if opts.Sound != nil and opts.Sound.Silent
is true while opts.Sound.Name is non-empty, return a validation error (e.g.,
"NotificationSound: Silent and Name are mutually exclusive"); implement this
check in the validateNotificationOptions function so callers cannot pass
{Silent:true, Name:"…"} and platform-dependent behavior is avoided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 70ef1257-20fb-47c1-bd0d-9f9d27e71f77
⛔ Files ignored due to path filters (5)
v3/examples/notifications/frontend/dist/assets/index-BlRTcGOJ.jsis excluded by!**/dist/**v3/examples/notifications/frontend/dist/assets/index-Dat4utuQ.jsis excluded by!**/dist/**v3/examples/notifications/frontend/dist/index.htmlis excluded by!**/dist/**v3/examples/notifications/frontend/dist/style.cssis excluded by!**/dist/**v3/examples/notifications/frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
v3/UNRELEASED_CHANGELOG.mdv3/examples/notifications/build/Taskfile.ymlv3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/internal/eventcreate.tsv3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/internal/eventdata.d.tsv3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/index.tsv3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/models.tsv3/examples/notifications/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/notifications/notificationservice.tsv3/examples/notifications/frontend/bindings/notifications/demoassets.tsv3/examples/notifications/frontend/bindings/notifications/index.tsv3/examples/notifications/frontend/index.htmlv3/examples/notifications/frontend/public/style.cssv3/examples/notifications/frontend/src/main.tsv3/examples/notifications/main.gov3/pkg/services/notifications/notifications.gov3/pkg/services/notifications/notifications_darwin.gov3/pkg/services/notifications/notifications_darwin.hv3/pkg/services/notifications/notifications_darwin.mv3/pkg/services/notifications/notifications_ios.gov3/pkg/services/notifications/notifications_linux.gov3/pkg/services/notifications/notifications_test.gov3/pkg/services/notifications/notifications_windows.go
…Ls in validator Code review surfaced four real defects; addressing all of them with one shared shape per backend (parkSchedule + cancelScheduled). 1. parkSchedule callbacks could delete the wrong map entry. A timer that fired before its cleanup acquired the lock could lose to a re-park for the same id and end up deleting the freshly-parked successor on its way out, breaking RemovePendingNotification's ability to cancel it. Fix: capture the timer in the closure and only delete the map entry if it still equals the captured timer. Apply the same shape on Linux and Windows. 2. The no-schedule path in SendNotification / SendNotificationWithActions (Linux) and sendOrSchedule (Windows) didn't cancel an already-parked timer for the same id, so an immediate send followed by the timer firing later spawned a duplicate notification. Fix: introduce a cancelScheduled helper that stops and removes any parked timer under the schedule lock, and call it on the immediate path. 3. validateNotificationOptions used os.Stat directly on NotificationAttachment.Path. The package godoc and macOS bridge document file:// URLs as a supported form, but os.Stat treats them as literal paths and rejected them. Fix: detect the file:// prefix, parse with net/url, and stat the resulting filesystem path. 4. Regression test added in TestValidateNotificationOptions_Attachments covering the file:// URL case so the validator change can't silently regress. Native + cross (windows) builds clean. All validator tests pass: TestValidateNotificationOptions_RequiredFields, TestValidateNotificationOptions_InterruptionLevel, TestValidateNotificationOptions_Schedule, TestValidateNotificationOptions_Attachments (including the new file:// URL passes subtest). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This PR has merge conflicts with the current master branch. Please resolve the conflicts before it can be reviewed and merged. Steps to resolve:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v3/UNRELEASED_CHANGELOG.md`:
- Line 27: Rewrite the ambiguous sentence about reply support in the changelog
to the clearer phrasing suggested: replace "Documentation: clarify that the
Windows notifications backend supports reply fields (only Linux does not)." with
"Documentation: clarify that reply fields are supported on macOS and Windows;
Linux does not support replies." Ensure the new sentence appears in the
UNRELEASED changelog section where the original sentence is located.
- Line 25: Update the changelog sentence about the Windows backend rewrite (the
entry mentioning the wintoast subpackage and dropping
git.sr.ht/~jackmordaunt/go-toast/v2) to clarify that while the rewrite enables
tag-based update requests, Windows currently redelivers notifications rather
than performing a true replace; replace the phrase "unlocking ... tag-based
update on Windows" with something like "enabling tag-based update requests
(Windows currently redelivers notifications instead of performing a true
replace)" so the behavior is unambiguous.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dcbb8236-1357-4934-92d7-f831d5d25bab
📒 Files selected for processing (1)
v3/UNRELEASED_CHANGELOG.md
leaanthony
added
Enhancement
Windows 11 test results — PR #5333Platform: Windows 11 Pro (10.0.26100, build 26100) Feature test results
Custom sound (IM), silent mode, and attachment visual were validated at the API level via unit tests. Windows-specific bug found and fixed
The fix falls back to stripping the Fix branch: |
|
Investigating the Linux side of this PR on Ubuntu 24.04 LTS (GNOME 46, X11, x86_64) with Go 1.25. All 9 Linux notification features verified:
Unit tests (4 groups, 13 subtests): all PASS. One Linux-specific note worth considering before merge: On Ubuntu 24.04 with GNOME Shell 46, |
|
This PR has merge conflicts. Please rebase your branch on the latest master and resolve the conflicts before requesting review again. Once the conflicts are resolved and the PR is mergeable, it will be reconsidered for testing. |
|
Reviewed ✅ This is a substantial and well-implemented feature that brings the notifications API to feature parity across platforms. The comprehensive testing on all three platforms, detailed support matrix documentation, and acknowledgment of known limitations show careful consideration. Decision: WANT - Accept for testing CC @leaanthony |
|
Investigating on Windows 11 Pro (24H2, Build 26100). Test environment: Go 1.26.2 · WebView2 147.0.3912.86 Unit tests ( Bug found — Windows-specific test failure: The test constructs On macOS, Build:
New Windows backend verified present: CC @leaanthony |
|
This is a substantial enhancement to the notifications service across all platforms. The feature matrix and implementation look comprehensive, but given the size (21 files, extensive platform-specific changes) and the complexity of notification APIs, I'd like a maintainer to review this before proceeding. CC @leaanthony |
…nOptions; fix XSS in example - validateNotificationOptions: url.Parse rejects "file://C:\path" on Windows (drive letter treated as hostname, colon as invalid port). Now falls back to prefix-strip + filepath.FromSlash when parse fails; also handles the standard three-slash form "file:///C:/path" by stripping the extra leading separator before the drive letter. Fixes the TestValidateNotificationOptions_Attachments Windows CI failure. - notifications example: replace innerHTML table building with DOM API to prevent XSS from arbitrary notification payloads. - UNRELEASED_CHANGELOG: remove premature "tag-based update on Windows" claim (Windows still redelivers); tighten reply-support sentence for clarity. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
Follow-up fixes — commit fd41417Three issues were addressed and pushed to this branch: 1. Windows CI failure —
|
|
@popaprozac - There is so much to love about this! Do you think you could also update the documentation with the new features? |
|
@leaanthony of course! I'll get that done this weekend |
Windows Platform Test Results - PR #5333Tested on: Windows 11 Pro (10.0.26100) Feature Test Matrix
Unit TestsAll 11 tests in Root cause of the earlier CI failure: Review ItemsAll five CodeRabbit actionable comments have been addressed by the PR author:
StatusThe PR is ready for merge from the Windows platform perspective. No blocking issues remain. Taliesin is an AI agent. CC @leaanthony |
|
Tested on macOS (Tart VM, ). Unit tests: All 12 pass. End-to-end notification display: Blocked by environmental limitation — The API surface (sound, threadIdentifier, interruptionLevel, attachment URL parsing) all configure correctly — the blocker is purely the auth grant, not the implementation. CI is green. Manual end-to-end verification on a macOS machine with an interactive GUI session is recommended before merge. CC @leaanthony Taliesin is an AI agent. CC @leaanthony |
|
@leaanthony let me know if we need anything else here to get it across the line |
…ctice for Linux On Ubuntu 24.04 (GNOME Shell 46), org.freedesktop.Notifications is not self-registered at session start. Without a daemon (dunst, mako, etc.) SendNotification fails with a D-Bus service-not-found error. Document this system requirement prominently with install commands and the exact error string, so users can diagnose and fix the issue. Also adds a Linux-specific Best Practice item in the Recommendations section. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
Documentation update: Linux notification daemon dependencyThe PR author's documentation update (commit Problem: Added to the Linux tab in Platform Considerations:
This was the blocker identified during Ubuntu 24.04 testing. The rest of the Linux surface (D-Bus integration, urgency hints, The PR is ready for maintainer review. Taliesin is an AI agent. CC @leaanthony |
- Attachment validator no longer wraps raw os.Stat errors; error message is now "not accessible" to avoid leaking filesystem existence details to the WebView-exposed binding. - Attachment paths must now be absolute; relative plain paths and relative file:// URLs (e.g. file://relative/image.png — which url.Parse parses as Host="relative") are rejected with a clear error. - Linux notify(): image-path D-Bus hint is now restricted to recognised image extensions (.png/.jpg/.jpeg/.gif/.bmp/.svg/.ico/.webp/.tiff/.tif); non-image paths are skipped rather than forwarded to the daemon. - Windows and Linux Shutdown() now cancel and drain scheduledTimers before tearing down their transport layer (COM/wintoast on Windows, D-Bus connection on Linux) so time.AfterFunc callbacks cannot fire against a deregistered activator or closed connection. - Updated and extended notifications_test.go to cover all new validation paths: non-accessible file, relative path, relative file:// URL, and oracle-free error messages.
|
@popaprozac - just pushed some changes to tighten up some loose ends. |
| Attachments: []notifications.NotificationAttachment{ | ||
| { | ||
| ID: "preview", | ||
| Path: "/absolute/path/to/image.png", |
There was a problem hiding this comment.
How will you know what the absolute file path would be on the running machine?
3 participants
Description
Extends the v3
notificationsservice so apps can attach sounds, images, threads, priority levels, scheduled delivery, and update existing notifications by ID — bringing macOS, Windows, and Linux to feature parity (with documentedgraceful degradation per platform).
This started from my own framework's notification implementation; the goal here was to bring the same surface to Wails in a way that fits existing conventions (additive
NotificationOptionsfields, theplatformNotifierinterface, build-tagged platform files, the existing OnNotificationResponse callback pattern).Per-platform support matrix
[UNNotificationSound soundNamed:]<audio src=ms-winsoundevent:...>sound-namehint<audio silent="true"/>suppress-soundhintthreadIdentifier<header>elementcategoryhintreplaces_idDocumented as godoc on the package and as a markdown table in the example app.
Fixes # (no existing issue — happy to file one)
Type of change
All
NotificationOptionsadditions are optional fields;UpdateNotificationis a new method on the service. Existing callers compile and run unchanged.How Has This Been Tested?
End-to-end via the extended example app (
v3/examples/notifications) on:For each platform I exercised:
OnNotificationResponse→app.Event.Emit("notification:action", ...)→ frontend table)Pingon macOS,IMon Windows,bellon Linux)Validator unit tests added in
v3/pkg/services/notifications/notifications_test.gocover required fields, interruption level enum, schedule mutual-exclusion + negative values, and attachment path stat. Run with:go vet ./pkg/services/notifications/...passes for darwin and (cross-compiled) windows targets locally; Linux verified on the test VM.Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PR (v3 changelog entries are added automatically)UNRELEASED_CHANGELOG.mdentries under Added and Changed)notifications_test.gocovers the new validator paths)Notes for reviewers / known follow-ups
A few items I'm flagging up-front since they're real and documented limitations rather than oversights:
wintoastto expose thetag/groupparameters ofToastNotifier.Show()(it currently exposes only the XML push). Worth a separate small PRupstream.
ScheduledToastNotificationAPI in wintoast or a system-bus daemon on Linux.wintoastsubpackage stays as the dependency for COM/WinRT plumbing — that subpackage is the load-bearing part that would be expensive to replace.Summary by CodeRabbit
New Features
Examples / UI
Tests
Documentation