Fix/windows desktop notifications fixes gen2 - #3
bernhardkaindl wants to merge 7 commits into
Conversation
The show_native_notification command only handled Linux and macOS. On Windows it returned an error, forcing the frontend to fall through to window.Notification (WebKit API). WebView2's Notification.permission reports 'denied' even when the WinRT toast API is available, so the app never appeared in Windows Settings > System > Notifications and the settings toggle was stuck showing 'Desktop notifications are blocked.' - Add tauri-winrt-notification as a Windows-specific dependency - Add a windows module in notifications.rs that posts WinRT toasts using the app's Tauri identifier as AppUserModelID (this is what registers the app with Windows notification settings) - Handle click actions through WinRT Activated handler, forwarding to the same native-notification-activated event that Linux uses - Add isWindowsPlatform() helper to platform.ts - Skip WebView2 Notification.permission check on Windows in getDesktopNotificationPermissionState() — use the Tauri plugin's isPermissionGranted() which queries native WinRT status - Route Windows through the native show_native_notification path in sendDesktopNotification() - Skip the Tauri plugin's onAction listener on Windows (click actions come through the native WinRT event instead) Fixes block#6377 Signed-off-by: Fatima Nur <fatimanur424@example.com>
Problem ------- Windows notification delivery and activation had three related gaps: - permission repair could still be in flight when the first notification was sent, - clicking a normal message notification could open an empty reply branch, - broad live channel subscriptions updated only the projected message cache, so an active channel could miss the message until reload. Solution -------- - Await and confirm desktop notification permission before feed, DM, and dedicated thread-reply delivery. - Carry explicit timeline-versus-thread intent in notification targets and channel routes. - Merge visible live channel messages into the authoritative channel window store before projection. - Add focused regression coverage for permission sequencing, activation routing, notify-while-viewing, and live window projection. Signed-off-by: Bernhard Kaindl <bernhardkaindl7@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
Critical notification delivery and activation issues, along with multiple routing and error-handling defects, remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR enables Windows-native WinRT notifications and improves permission sequencing, activation routing, and live channel projection.
Changes:
- Adds Windows detection and native toast delivery.
- Gates notifications on permission readiness.
- Adds explicit timeline/thread routing.
- Projects live channel updates into the message window.
- Adds regression tests and Windows dependencies.
File summaries
| File | Summary |
|---|---|
desktop/src/shared/lib/platform.ts |
Windows platform detection |
desktop/src/shared/lib/platform.test.mjs |
Platform tests |
desktop/src/features/notifications/use-feed-desktop-notifications.ts |
Feed permission sequencing |
desktop/src/features/notifications/use-feed-desktop-notifications.test.mjs |
Feed sequencing tests |
desktop/src/features/notifications/lib/target.ts |
Notification routing targets |
desktop/src/features/notifications/lib/target.test.mjs |
Target tests |
desktop/src/features/notifications/lib/desktop.ts |
Permission and native notification handling |
desktop/src/features/notifications/lib/desktop.test.mjs |
Permission tests |
desktop/src/features/channels/useLiveChannelUpdates.ts |
Live window projection |
desktop/src/features/channels/useLiveChannelUpdates.test.mjs |
Live update tests |
desktop/src/features/channels/ui/useChannelRouteTarget.ts |
Timeline routing |
desktop/src/features/channels/ui/channelSearchKeys.ts |
Route search keys |
desktop/src/features/channels/ui/ChannelScreen.types.ts |
Route target prop |
desktop/src/features/channels/ui/ChannelScreen.tsx |
Route prop wiring |
desktop/src/app/useAppShellDesktopNotifications.ts |
Notification delivery sequencing |
desktop/src/app/routes/channels.$channelId.tsx |
Route parsing |
desktop/src/app/routes/ChannelRouteScreen.tsx |
Route target wiring |
desktop/src/app/navigation/useAppNavigation.ts |
Navigation options |
desktop/src/app/AppShell.helpers.ts |
Activation routing |
desktop/src/app/AppShell.helpers.test.mjs |
Activation tests |
desktop/src-tauri/src/commands/notifications.rs |
Windows toast implementation |
desktop/src-tauri/Cargo.toml |
Native notification dependency |
desktop/src-tauri/Cargo.lock |
Dependency lock update |
Review details
Suppressed comments (8)
desktop/src-tauri/src/commands/notifications.rs:143
Toast::new(&app_id)only creates a notifier for an AppUserModelID that Windows already knows; this code never registers the identifier with a Start Menu shortcut/installer or repairs that registration. On unpackaged Tauri builds,xyz.block.buzz.appcan therefore be rejected as unregistered and every toast is dropped. Register or self-heal the AUMID before using it.
let app_id = app.config().identifier.clone();
std::thread::spawn(move || {
let app_clone = app.clone();
let result = Toast::new(&app_id)
desktop/src/app/routes/ChannelRouteScreen.tsx:329
- This forwards
targetMessageViewonly through the ordinaryChannelScreenpath. WhenprojectHomeis present, the earlierProjectChannelHomereturn does not accept or forward this prop to itsChannelScreen, so a root-message notification in a project channel falls back to the default route handling and opens a reply panel instead of staying in the timeline.
targetMessageView={targetMessageView}
desktop/src/app/useAppShellDesktopNotifications.ts:94
ensureDesktopNotificationPermissionGranted()can reject when the native permission request fails, but this.thenhas no rejection handler and the surroundingvoiddrops it. A permission failure therefore becomes an unhandled rejection and the DM notification is lost; handle or queue the event at this boundary, or propagate it to a durable retry owner.
void ensureDesktopNotificationPermissionGranted().then(
async (permissionGranted) => {
desktop/src/app/useAppShellDesktopNotifications.ts:141
ensureDesktopNotificationPermissionGranted()can reject when the native permission request fails, but this.thenhas no rejection handler and the surroundingvoiddrops it. A permission failure therefore becomes an unhandled rejection and the thread-reply notification is lost; handle or queue the event at this boundary, or propagate it to a durable retry owner.
void ensureDesktopNotificationPermissionGranted().then(
async (permissionGranted) => {
desktop/src/features/channels/ui/useChannelRouteTarget.ts:124
targetMessageView === "timeline"is checked only after!targetMessage.parentId. A notification withopenInThread: falsecan still carry root/reply tags (the new target test does), so after the newgoChannel(...messageView: "timeline")route is loaded this effect falls through togetThreadRouteTargetand opens the branch anyway. Move this guard before the parent check so the explicit timeline intent wins.
if (targetMessageView === "timeline") {
handledThreadRouteTargetRef.current = targetKey;
return;
desktop/src/features/channels/useLiveChannelUpdates.test.mjs:190
- Please assert the rendered
channelMessagesKeycache here, not onlyliveOverlay. The production fix isprojectChannelWindowMessages(...); removing that call leaves this overlay assertion green while the timeline cache remains stale, so the regression test is not falsifiable against the user-visible behavior.
assert.deepEqual(
h.queryClient
.getQueryData(h.channelWindowKey("channel-0"))
.liveOverlay.map((item) => item.id),
["mention"],
);
desktop/src/features/channels/useLiveChannelUpdates.ts:372
- The added regression only asserts
liveOverlayon the window store. It never asserts the projectedchannelMessagesKey(nor a loaded authoritative page), so removingprojectChannelWindowMessages(queryClient, channelId)would still pass while the original “miss until reload” bug returns. Assert the rendered message cache, ideally with an existing authoritative page, so this production seam is falsifiable.
projectChannelWindowMessages(queryClient, channelId);
desktop/src/features/notifications/use-feed-desktop-notifications.ts:209
- This new batch awaits permission repair, which can reject when the native request fails, but the returned promise is discarded here. That produces an unhandled rejection and silently loses the feed items; handle the failure at an owner that records/retries the pending batch rather than using
voidhere.
void deliverFeedNotificationBatch(
newItems,
autoRequestPermissionIfNeeded,
- Files reviewed: 22/23 changed files
- Comments generated: 8
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Bernhard Kaindl <bernhardkaindl7@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
Six moderate review findings remain unresolved in routing, live projection, and permission handling.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
desktop/src/features/channels/useLiveChannelUpdates.ts:364
isUnreadTriggerKindis intentionally narrower than the authoritative timeline set: it excludes visible diff/system/job/huddle rows and auxiliary events. The broad subscription still adds those events tochannelMessagesKey, but this guard omits them fromchannelWindowKey, so the next authoritative projection can discard them and the active channel misses the event until reload. Merge every row/aux kind recognized by the channel-window subscription, not only unread-trigger kinds.
desktop/src/app/routes/ChannelRouteScreen.tsx:329
- This forwards the new timeline intent only to the ordinary
ChannelScreenbranch. For a project-home channel, the earlier branch rendersProjectChannelHomewithtargetMessageIdbut notargetMessageView, and its nestedChannelScreentherefore follows the default root-message behavior and can open the reply panel. Forward the view mode throughProjectChannelHomeas well so notification clicks have the same behavior in project channels.
targetForumReplyId={targetReplyId}
targetMessageEvents={targetMessageEvents}
targetMessageId={targetMessageId}
targetMessageView={targetMessageView}
desktop/src/features/notifications/lib/desktop.ts:168
- The Windows branch collapses every
isPermissionGranted() === falseresult to"default", but the native API only returns a boolean, so a user-denied system permission is indistinguishable from an unprompted one. After denial, every DM/thread callback will callrequestPermission()again, and the settings hook never observes"denied"to disable the feature. Preserve/cache a terminal native denial (or otherwise stop retrying it per event) so a persistent block does not trigger repeated permission requests.
if (isTauri() && isWindowsPlatform()) {
try {
return (await isPermissionGranted()) ? "granted" : "default";
} catch {
return "default";
desktop/src/features/notifications/lib/target.ts:46
- Broadcast replies are intentionally treated as timeline rows (
isThreadReplyexcludesisBroadcastReplyand the route helper returns the reply id), but this condition only checks for a root tag. A feed notification for a broadcast reply therefore setsopenInThread: true, sends activation through the thread/search path, and can open the wrong view. Exclude broadcast replies when deriving this flag, using the existingisBroadcastReplyhelper.
openInThread: getThreadReference(item.tags).rootId !== null,
desktop/src/features/notifications/use-feed-desktop-notifications.ts:107
- This catch converts a permission-state/request failure into
false, but the feed effect adds every item toseenItemIdsRefand persists those IDs before awaiting the batch. A transient permission IPC failure therefore suppresses the current batch and permanently prevents those feed items from being retried. Distinguish an explicit denial from an operational error and retain or durably retry the items on the latter instead of returning a terminalfalse.
} catch (error) {
console.warn("Failed to request desktop notification permission", error);
return false;
- Files reviewed: 25/26 changed files
- Comments generated: 1
- Review effort level: Lite
Signed-off-by: Bernhard Kaindl <bernhardkaindl7@gmail.com>
|
Addressed all five suppressed findings from review 5190904492 in 0f89114:
I also made background settings refreshes catch and log native IPC failures, while explicit user toggles retain their visible error path. Validation: 35 focused frontend tests, TypeScript, Biome, Rust formatting, native notification test, E2E build, and the focused project-home Playwright regression all pass. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate issues remain in Windows activation and registration, live-channel projection, and notification delivery.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (6)
desktop/src/features/channels/ui/useChannelRouteTarget.ts:127
- This explicit timeline branch is bypassed by the handled-target guard when the same
channelId:messageIdwas previously processed as a thread target:targetKeydoes not includetargetMessageView, so a later notification withmessageView=timelinereturns at line 112 and leaves the existing thread panel open. Include the view intent in the handled key (or evaluate timeline intent before that guard) so repeated clicks with different intents are routed independently.
// Explicit timeline intent outranks ancestry-based thread navigation.
if (targetMessageView === "timeline") {
handledThreadRouteTargetRef.current = targetKey;
return;
desktop/src/features/channels/useLiveChannelUpdates.ts:382
- This hook subscribes every member channel, so it now creates/updates a window for each background channel on every timeline or auxiliary event.
mergeLiveChannelWindowEventonly deduplicates and has no retention bound, allowingliveOverlay/liveAux(and the projected message list) to grow for a busy channel for the lifetime of its cache while also doing projection work per event. Add bounded eviction or limit this merge to the active channel while preserving the race fix.
const nextWindow = mergeLiveChannelWindowEvent(
currentWindow,
event,
isTimelineRow,
);
desktop/src/features/notifications/lib/desktop.ts:468
- On Windows,
getDesktopNotificationPermissionState()reaches the nativeinvokepath and can reject when the WinRT query cannot be scheduled or completed. Because this await is outside the deliverytry/catch, fire-and-forget callers such as reminders and community-join alerts receive an unhandled rejection instead of the promised boolean delivery result, and the notification is dropped. Treat permission-query failures asfalsehere (and log them) sosendDesktopNotificationpreserves its failure contract.
if ((await getDesktopNotificationPermissionState()) !== "granted") {
return false;
}
desktop/src/features/notifications/use-feed-desktop-notifications.test.mjs:104
- This test does not bind the single-flight claim to the production request path: the injected
requestAccessstub returns the same promise on every call, so it passes even ifrequestDesktopNotificationAccessstops sharingpendingPermissionRequest;requestCalls === 2actually allows duplicate underlying requests. Exercise the production request function (or a seam that counts the OS request) and assert that the underlying request happens once.
test("a concurrent feed batch joins the pending permission request", async () => {
const attempt = { hasRequested: false };
let permissionStateChecks = 0;
let requestCalls = 0;
let releaseRequest;
const request = new Promise((resolve) => {
releaseRequest = resolve;
});
const getPermissionState = async () => {
permissionStateChecks++;
return "default";
};
const requestAccess = () => {
requestCalls++;
return request;
};
desktop/src/features/notifications/use-feed-desktop-notifications.ts:98
- This caught delivery failure is not durable: the hook clears
inFlightItemIdsRefand only persistshandledIds, while a remount treats every item currently in the feed as initial history and marks it seen. If the app exits after this exception, the item inretryableIdsis therefore never retried, despite the new retry path. Persist retryable IDs (or a durable retry record) and consume them on initialization, or propagate the failure instead of relying only on the next in-process feed update.
} catch (error) {
console.warn("Failed to deliver feed notification", item.id, error);
return { id: item.id, delivered: false };
}
desktop/src/features/notifications/use-feed-desktop-notifications.ts:284
- The generation fence is advanced only when
normalizedPubkeychanges, butenabledis also a dependency of this effect. If the user disables desktop notifications while the permission request is pending, the existing batch continues and can callsendDesktopNotificationafter the setting is off; a hung request can also leave its IDs permanently ininFlightItemIdsRef. Invalidate/cancel the generation on disable and re-checkenabledbefore delivery.
const generation = notificationGenerationRef.current;
void deliverFeedNotificationBatch(
- Files reviewed: 31/32 changed files
- Comments generated: 3
- Review effort level: Lite
Signed-off-by: Bernhard Kaindl <bernhardkaindl7@gmail.com>
|
All six suppressed findings from review 5191170956 were addressed in commit 91c3809:
|
Signed-off-by: Bernhard Kaindl <bernhardkaindl7@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved findings remain around Windows registration/FFI, permission guards, durable delivery retries, activation draining, and retry-state bounds.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (7)
Previously missed (1) — in code that hasn't changed since the last review.
desktop/src/app/useAppShellDesktopNotifications.ts:96
- The permission promise can outlive the state that scheduled it. If the user disables desktop notifications or switches away from this mounted surface while the OS permission request is pending, this continuation still sends the DM because it never re-checks current state or a generation/cancellation token; guard the continuation before calling
sendDesktopNotification.
This issue also appears on line 142 of the same file.
desktop/src-tauri/src/commands/notifications.rs:193
- This adds a new
unsafeFFI call in the production notification path. The repository's native code policy does not allow introducing unsafe production code; please route the AUMID operation through an approved safe abstraction or otherwise redesign this Windows integration before merging.
use windows_sys::Win32::System::Registry::{
desktop/src/app/useAppShellDesktopNotifications.ts:104
- When native delivery returns
false, this handler just exits, butuseLiveChannelUpdatesrecords the event ID inseenNotificationEventIdsRefbefore invoking this callback. A transient Windows permission/WinRT/IPC failure therefore permanently drops a DM notification and it cannot be retried; keep a durable retry record or only mark the event seen after successful delivery.
if (!didSend) return;
desktop/src/app/useAppShellDesktopNotifications.ts:143
- The permission promise can outlive the state that scheduled it. If the user disables desktop notifications or switches away from this mounted surface while the OS permission request is pending, this continuation still sends the thread reply because it never re-checks current state or a generation/cancellation token; guard the continuation before calling
sendDesktopNotification.
if (!permissionGranted) return;
const didSend = await sendDesktopNotification({
desktop/src/features/notifications/lib/desktop.ts:326
- If the native event listener setup rejects,
nativeUnlistenstays null and this initial drain is skipped. A cold-start Windows toast click can then remain in the Rust queue when the already-visible window produces no later focus or visibility event; drain the queue independently of whether the listener registration succeeded.
if (nativeUnlisten && usesActivationQueue) {
desktop/src/features/notifications/lib/desktop.ts:497
- This newly added Windows native post can fail transiently, but the returned
falseis discarded by the DM and thread-reply callers. Unlike the feed path, those live notifications have no durable retry, so a WinRT error permanently loses the event; connect this failure to a retryable delivery path instead of treating it as terminal.
if (
isTauri() &&
(isLinuxPlatform() || isMacPlatform() || isWindowsPlatform())
) {
try {
desktop/src/features/notifications/use-feed-desktop-notifications.ts:339
- Only the serialized retry list is capped; this in-memory
Setis never pruned, so every item whose delivery keeps failing remains resident and a long-lived desktop session can grow it without bound. Prune the set to the same 500-item limit before persisting it.
- Files reviewed: 35/36 changed files
- Comments generated: 4
- Review effort level: Lite
…delivery Replace raw notification registration FFI with WinSafe and forbid unsafe code in the notification module. Preserve existing notification opt-outs and shortcut arguments, with native regression tests. Fence feed and live deliveries on current settings, persist bounded DM/thread retry records, and test Windows activation drains even when listener setup fails. Revert the AGENTS.md addition from 91c3809; local Windows guidance stays excluded. Signed-off-by: Bernhard Kaindl <bernhardkaindl7@gmail.com>
Review 5191367484 follow-upPublished in 3589226. The three remaining formal threads have fix-specific replies; the registration failure-propagation thread was already addressed by e520143. All seven suppressed comments
Validation on Windows
The eight-line |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate notification reliability, retry, routing, and cache-growth issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
desktop/src/app/AppShell.helpers.ts:214
- When
openInThreadis true this path still usesopenSearchHit, which seedstoSearchHit(target)beforeChannelRouteScreenhas fetched the real reply. That synthetic event has no reply tags, souseChannelRouteTargetcan classify the reply as a root, open a new/empty branch, and mark the route target handled before hydration replaces it. Defer the cache projection until the authoritative target/ancestor fetch completes (or preserve the reply ancestry in the cached event), and cover an actual thread-notification activation.
!target.openInThread &&
target.kind !== KIND_FORUM_POST &&
target.kind !== KIND_FORUM_COMMENT
) {
desktop/src/features/channels/useLiveChannelUpdates.ts:362
- This broad subscription updates every cached channel's
channelMessagesKeywith every live event, including ordinary thread replies and auxiliary events, but neithermergeTimelineCacheMessagesnor this path imposes a size bound.reconcileChannelWindowMessagesdeliberately retains cache-only replies, so a long-running session can accumulate an unbounded per-channel array and repeatedly format those events; cap/evict this cache or keep the authoritative window as the only bounded live store.
queryClient.setQueryData<RelayEvent[]>(
channelMessagesKey(channelId),
(current) => {
if (!current) {
return current;
}
return mergeTimelineCacheMessages(current, event);
},
desktop/src/features/notifications/use-feed-desktop-notifications.ts:392
- Retry IDs are retained until delivery succeeds or permission is denied, but no attempt count, backoff, or terminal state is stored. If native delivery remains unavailable, each feed refresh or app mount will retry the same item forever; the 500-ID cap bounds storage but not repeated delivery work. Persist retry metadata and quarantine/drop an item after a terminal limit while retaining whatever durable recovery record the product requires.
desktop/src/features/notifications/useLiveNotificationDelivery.ts:53 restore()runs while the delivery effect is mounting, so a truncated or otherwise malformed localStorage journal makes this hook throw before it can deliver new notifications (and can fail the app shell's notification setup). Treat malformed JSON and invalid entries as an empty/filtered journal, matching the feed retry readers, rather than propagating storage corruption into React effects. Please also add a regression test for malformed journal data.
desktop/src/features/notifications/useLiveNotificationDelivery.ts:55- The retry budget is reset to zero for every restored record, while exhausted entries remain in the journal. A permanent native failure (for example, WinRT remaining unavailable) therefore causes the same alert to be attempted five more times on every app mount indefinitely. Persist a terminal/exhausted state or otherwise quarantine records after a total retry limit so a durable journal does not become an unbounded cross-session retry loop.
- Files reviewed: 36/37 changed files
- Comments generated: 1
- Review effort level: Lite
| const MAX_PENDING = 500; | ||
| const MAX_ATTEMPTS = 5; |
Local recreation to get a Copilot review of block /buzz/pull/7614:
fix(desktop): enable Windows-native winrt notifications and make them reliable
Potentially better PR (to be tested, reads very compentent and well-tested, but needs to be verified by a 2nd party):
This PR combines @fatima-n09's PR
with my fixes to make them reliable and fix the remaining issues found.
Fixes
Obsoletes
Related to
Problems
Solution