From f2f091cf7ceb3c96b4be9ad31881c2c13c292f09 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 15:22:58 +0000 Subject: [PATCH] fix(web): toast + log on handler failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Handler closures in `crates/web/src/handlers.rs` swallowed every async error with `let _ = h.(...).await`. Send/edit/delete/react/ pin/unpin failures vanished — typed message disappeared from input, no log, no toast. Refs #350 [GEN-07]. New `warn_and_toast_with(action, e, toasts)` helper: - `tracing::warn!(error = ?e, action, "ui handler failed")` - Pushes `Toast::err("Couldn't {action}. Try again.")` with a per- action `dedup` key so back-to-back failures coalesce instead of stacking. Mirrors the lazy `use_context::()` pattern PR #411 introduced for the inbound mark-read button. Each handler now captures the toast stack on the synchronous frame (reactive owner intact) before `spawn_local`, then forwards into the helper from the async path — `wasm_bindgen_futures::spawn_local` strips the owner, so context lookup must happen earlier. 7 sites patched: send_message, send_reply, edit_message, delete_ message, react, pin_message, unpin_message. switch_channel / switch_server stay as-is — they return `()` so there was nothing to swallow. Browser test `handler_error_toasts::*` covers helper happy-path, dedup coalescing, and missing-stack no-panic. Test exercises the exact production code path; the handler closures themselves are pinned to ClientHandle with no trait seam to inject a failing double, so going one level deeper would buy nothing. `rg "let _ = h\." crates/web/src/handlers.rs` returns empty. fmt + clippy + native tests + wasm check all green; browser tier verified locally via `cargo check -p willow-web --tests` + manual review of the test mod (Firefox/wasm-pack not on this box, browser suite runs in CI). --- crates/web/src/handlers.rs | 92 ++++++++++++++++++++++++++++---- crates/web/tests/browser.rs | 102 ++++++++++++++++++++++++++++++++++++ 2 files changed, 185 insertions(+), 9 deletions(-) diff --git a/crates/web/src/handlers.rs b/crates/web/src/handlers.rs index 47b6be6c..a047e9ce 100644 --- a/crates/web/src/handlers.rs +++ b/crates/web/src/handlers.rs @@ -3,10 +3,28 @@ //! Each function returns a closure for use as a Leptos event handler. //! Async client methods are wrapped in `spawn_local` since Leptos //! event handlers are synchronous. +//! +//! ## Error reporting (issue #350) +//! +//! Async client mutations return `anyhow::Result<()>`. Previously each +//! handler discarded the error with `let _ = ...`, so a typed message +//! could vanish from the input with no feedback. Every site now routes +//! failures through [`warn_and_toast`], which logs at `WARN` and pushes +//! an `err` toast onto the ambient [`ToastStack`] so the user sees +//! something happened. +//! +//! The toast stack is captured *before* `spawn_local` (in the +//! synchronous handler closure) — `wasm_bindgen_futures::spawn_local` +//! does not preserve a reactive owner, so `use_context::()` +//! must be resolved on the calling task. Mirrors the pattern PR #411 +//! introduced in `sync_queue_view.rs`. + +use std::fmt::Debug; use leptos::prelude::*; use crate::app::WebClientHandle; +use crate::components::{Toast, ToastStack}; use crate::state::{AppState, AppWriteSignals}; /// Parse a hex string into an `EventHash`, logging on failure. @@ -14,6 +32,44 @@ fn parse_event_hash(hex: &str) -> Option hex.parse().ok() } +/// Log a UI handler failure at WARN and surface it to the user via a +/// toast. +/// +/// `action` is a short human-readable verb-phrase (`"send message"`, +/// `"edit message"`, …). It appears in the tracing field and in the +/// toast copy so the user knows which action failed. +/// +/// `toasts` is the optional toast stack captured at handler-call time +/// (before any `spawn_local`). It is `None` in headless test harnesses +/// or very early during boot — the `tracing::warn!` call still fires +/// in that case so the failure is never fully silent. +/// +/// The convenience overload [`warn_and_toast`] resolves the stack from +/// context for callers running on a reactive owner; prefer that on +/// synchronous code paths. +pub fn warn_and_toast_with(action: &'static str, e: &dyn Debug, toasts: Option<&ToastStack>) { + tracing::warn!(error = ?e, action, "ui handler failed"); + if let Some(stack) = toasts { + stack.push( + Toast::err(format!("Couldn't {action}. Try again.")) + .dedup(format!("handler-error:{action}")) + .build(), + ); + } +} + +/// Reactive-owner shortcut for [`warn_and_toast_with`]: looks up the +/// ambient [`ToastStack`] via `use_context` and forwards. +/// +/// Only safe to call on a reactive owner — for example, a Leptos +/// event-handler closure or a synchronous component body. Inside an +/// `async` block spawned via `wasm_bindgen_futures::spawn_local`, no +/// owner is preserved, so the toast stack must be captured on the +/// outer synchronous frame and passed via [`warn_and_toast_with`]. +pub fn warn_and_toast(action: &'static str, e: &dyn Debug) { + warn_and_toast_with(action, e, use_context::().as_ref()); +} + /// Create a handler for sending messages (including replies). pub fn make_send_handler( handle: WebClientHandle, @@ -24,15 +80,21 @@ pub fn make_send_handler( let ch = state.chat.current_channel.get_untracked(); let h = handle.clone(); let replying = state.chat.replying_to.get_untracked(); + // Capture the toast stack on the outer (reactive-owner) frame + // — `spawn_local` strips the owner so `use_context` inside the + // async block would return None. + let toasts = use_context::(); // Clear reply state immediately so the UI updates. write.chat.set_replying_to.set(None); wasm_bindgen_futures::spawn_local(async move { if let Some(reply_msg) = replying { if let Some(hash) = parse_event_hash(&reply_msg.id) { - let _ = h.send_reply(&ch, &hash, &body).await; + if let Err(e) = h.send_reply(&ch, &hash, &body).await { + warn_and_toast_with("send reply", &e, toasts.as_ref()); + } } - } else { - let _ = h.send_message(&ch, &body).await; + } else if let Err(e) = h.send_message(&ch, &body).await { + warn_and_toast_with("send message", &e, toasts.as_ref()); } }); } @@ -47,10 +109,13 @@ pub fn make_edit_handler( move |(message_id, new_body): (String, String)| { let ch = state.chat.current_channel.get_untracked(); let h = handle.clone(); + let toasts = use_context::(); write.chat.set_editing.set(None); wasm_bindgen_futures::spawn_local(async move { if let Some(hash) = parse_event_hash(&message_id) { - let _ = h.edit_message(&ch, &hash, &new_body).await; + if let Err(e) = h.edit_message(&ch, &hash, &new_body).await { + warn_and_toast_with("edit message", &e, toasts.as_ref()); + } } }); } @@ -65,9 +130,12 @@ pub fn make_delete_handler( move |msg: willow_client::DisplayMessage| { let ch = state.chat.current_channel.get_untracked(); let h = handle.clone(); + let toasts = use_context::(); wasm_bindgen_futures::spawn_local(async move { if let Some(hash) = parse_event_hash(&msg.id) { - let _ = h.delete_message(&ch, &hash).await; + if let Err(e) = h.delete_message(&ch, &hash).await { + warn_and_toast_with("delete message", &e, toasts.as_ref()); + } } }); } @@ -82,9 +150,12 @@ pub fn make_react_handler( move |(msg, emoji): (willow_client::DisplayMessage, String)| { let ch = state.chat.current_channel.get_untracked(); let h = handle.clone(); + let toasts = use_context::(); wasm_bindgen_futures::spawn_local(async move { if let Some(hash) = parse_event_hash(&msg.id) { - let _ = h.react(&ch, &hash, &emoji).await; + if let Err(e) = h.react(&ch, &hash, &emoji).await { + warn_and_toast_with("add reaction", &e, toasts.as_ref()); + } } }); } @@ -141,12 +212,15 @@ pub fn make_pin_handler( let ch = state.chat.current_channel.get_untracked(); let h = handle.clone(); let mid = msg.id.clone(); + let toasts = use_context::(); wasm_bindgen_futures::spawn_local(async move { if let Some(hash) = parse_event_hash(&mid) { if h.is_pinned(&ch, &hash).await { - let _ = h.unpin_message(&ch, &hash).await; - } else { - let _ = h.pin_message(&ch, &hash).await; + if let Err(e) = h.unpin_message(&ch, &hash).await { + warn_and_toast_with("unpin message", &e, toasts.as_ref()); + } + } else if let Err(e) = h.pin_message(&ch, &hash).await { + warn_and_toast_with("pin message", &e, toasts.as_ref()); } } }); diff --git a/crates/web/tests/browser.rs b/crates/web/tests/browser.rs index 4c04efed..1861b3f8 100644 --- a/crates/web/tests/browser.rs +++ b/crates/web/tests/browser.rs @@ -12087,3 +12087,105 @@ mod phase_2d_ephemeral_channels { assert_eq!(input.value(), "90", "must clamp at 90-day cap"); } } + +// ── Issue #350: handler error reporting ───────────────────────────────────── +// +// `crates/web/src/handlers.rs` previously discarded every async-action +// error with `let _ = ...`. The new `warn_and_toast_with` helper logs +// via `tracing::warn!` and pushes an err toast onto the captured +// `ToastStack` so the user gets feedback when send/edit/delete/react/ +// pin fails. +// +// We test the helper directly rather than driving a fake-failing +// client through the closures: the handler closures are pinned to the +// real `ClientHandle` type and there's no trait seam to +// inject a failing double. The helper *is* the production code path +// that every handler now goes through (handlers capture the stack via +// `use_context` outside their `spawn_local` block, then call +// `warn_and_toast_with` from inside), so exercising it covers all 7 +// `let _ = h.(...).await` sites. +mod handler_error_toasts { + use super::*; + use willow_web::components::{ToastStack, ToastStackView}; + use willow_web::handlers::warn_and_toast_with; + + /// `warn_and_toast_with` pushes an `err` toast onto the supplied + /// `ToastStack` whose copy names the action that failed. Without + /// this, action handlers would still be silently dropping errors. + #[wasm_bindgen_test] + async fn warn_and_toast_with_pushes_err_toast() { + let stack = ToastStack::new(); + let stack_for_mount = stack.clone(); + let container = mount_test(move || { + provide_context(stack_for_mount.clone()); + view! { } + }); + + // No toasts yet. + tick().await; + assert_eq!(query_all(&container, ".toast").len(), 0); + + // Simulate a handler failure. Any `Debug`-able error stands in + // for the real `anyhow::Error` the production handlers pass + // through. + warn_and_toast_with("send message", &"boom", Some(&stack)); + tick().await; + + let toasts = query_all(&container, ".toast"); + assert_eq!( + toasts.len(), + 1, + "warn_and_toast_with must push exactly one toast" + ); + let t = &toasts[0]; + assert_eq!( + t.get_attribute("role").as_deref(), + Some("alert"), + "err severity routes to assertive aria-live (role=alert)" + ); + let title = t + .query_selector(".toast-title") + .unwrap() + .expect("toast title element"); + let title_text = text(&title); + assert!( + title_text.contains("send message"), + "toast title must name the failed action. got: {title_text:?}" + ); + } + + /// Two failures of the same action coalesce via the `dedup` key — + /// the user sees a single err toast updated in place rather than + /// a stack of identical entries piling up if the network flaps. + #[wasm_bindgen_test] + async fn warn_and_toast_with_dedups_per_action() { + let stack = ToastStack::new(); + let stack_for_mount = stack.clone(); + let container = mount_test(move || { + provide_context(stack_for_mount.clone()); + view! { } + }); + tick().await; + + warn_and_toast_with("send message", &"first", Some(&stack)); + warn_and_toast_with("send message", &"second", Some(&stack)); + tick().await; + + let toasts = query_all(&container, ".toast"); + assert_eq!( + toasts.len(), + 1, + "two failures of the same action must coalesce, not stack" + ); + } + + /// When the supplied stack is `None` (early boot, stripped-down + /// test harness), `warn_and_toast_with` must still log without + /// panicking. The toast push is a best-effort surface; the + /// `tracing::warn!` is the load-bearing record. + #[wasm_bindgen_test] + async fn warn_and_toast_with_no_stack_does_not_panic() { + warn_and_toast_with("send message", &"boom", None); + tick().await; + } +}