Skip to content

fix(node): route capture/record toasts through App.ShowToast - #378

Merged
shanselman merged 5 commits into
openclaw:masterfrom
AlexAlves87:fix/node-service-show-toast-routing
May 14, 2026
Merged

fix(node): route capture/record toasts through App.ShowToast#378
shanselman merged 5 commits into
openclaw:masterfrom
AlexAlves87:fix/node-service-show-toast-routing

Conversation

@AlexAlves87

Copy link
Copy Markdown
Contributor

Summary

NodeService had its own ShowToast helper that called ToastContentBuilder.Show() directly. This bypassed the sound preference (None / Subtle / Default) and the 30-second deduplication window both implemented in App.ShowToast.

All screen-capture, screen-record, and camera toasts were affected — including toasts that fire on attacker-controllable triggers (screen.snapshot throttled once per 10 s).

Changes

  • Remove NodeService.ShowToast
  • Add NodeService.ToastRequested event (EventHandler<ToastContentBuilder>)
  • Replace all direct toast calls in capture/record handlers with ToastRequested?.Invoke
  • Subscribe in App and delegate to App.ShowToast, so sound preferences and dedup are honoured

Fixes #342.

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com

AlexAlves87 and others added 2 commits May 14, 2026 09:08
NodeService.ShowToast called ToastContentBuilder.Show() directly,
bypassing the user's sound preference (None/Subtle/Default) and the
30-second deduplication window implemented in App.ShowToast.

Replace the private helper with a ToastRequested event; App subscribes
and delegates to its own ShowToast, so sound and dedup are honoured for
all screen-capture, screen-record, and camera toasts.

Fixes openclaw#342.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@shanselman

Copy link
Copy Markdown
Collaborator

Good direction routing these through App.ShowToast, but this changes the failure semantics for node capture/record commands.

NodeService.ShowToast() used to swallow toast errors. After this PR, ToastRequested?.Invoke(...) can throw synchronously because App.ShowToast(builder) ultimately calls builder.Show() without a local try/catch. That means a toast-platform failure can now fail screen.snapshot, replace the intended camera InvalidOperationException, or mask the original capture/recording error.

Suggested fix: keep notification failures non-fatal at the app boundary:

private void OnNodeToastRequested(object? sender, ToastContentBuilder builder)
{
    try
    {
        ShowToast(builder);
    }
    catch (Exception ex)
    {
        Logger.Warn($"Failed to show node toast: {ex.Message}");
    }
}

A small unit seam/test would be ideal, but the main requirement is that toast delivery failures must not change node command results.

AlexAlves87 and others added 3 commits May 14, 2026 21:15
A toast-platform exception propagating through ToastRequested?.Invoke
could fail screen.snapshot, replace the intended camera error, or mask
the original capture/record exception. Wrap ShowToast in a try/catch so
notification failures are logged but never change node command results.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extracts a NonFatalAction.Run helper to OpenClaw.Shared so the
try/catch behaviour in OnNodeToastRequested is independently
testable without instantiating App or NodeService.

Three unit tests cover: exception not propagated, error message
forwarded to onError callback, and success path leaves onError
uncalled.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ToastRequested fires from async node workers (screen capture/record,
camera blocked) on the thread pool. App.ShowToast reads and modifies
_recentToastKeys (a plain Dictionary) without synchronisation, which
races with UI-thread toasts and can corrupt dedup state or throw.

Re-queue through _dispatcherQueue so ShowToast always runs on the UI
thread, consistent with how OnSystemNotify already handles the same
pattern via NodeService._dispatcherQueue.TryEnqueue.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@AlexAlves87

Copy link
Copy Markdown
Contributor Author

Added the try/catch so toast failures are non-fatal. Also fixed a thread-safety gap: the capture/record handlers fire ToastRequested from async workers, but ShowToast touches _recentToastKeys (a plain Dictionary) without synchronisation. Marshalled OnNodeToastRequested through _dispatcherQueue, consistent with how OnSystemNotify already handles this. Added a unit seam (NonFatalAction.Run) with three tests covering the non-fatal dispatch behaviour.

@shanselman
shanselman merged commit 09cd1f9 into openclaw:master May 14, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[deep-review] C2: NodeService.ShowToast bypasses user sound preferences for capture/record toasts

2 participants