Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 83 additions & 9 deletions crates/web/src/handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,73 @@
//! 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::<ToastStack>()`
//! 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.
fn parse_event_hash(hex: &str) -> Option<willow_client::willow_state::EventHash> {
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::<ToastStack>().as_ref());
}

/// Create a handler for sending messages (including replies).
pub fn make_send_handler(
handle: WebClientHandle,
Expand All @@ -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::<ToastStack>();
// 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());
}
});
}
Expand All @@ -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::<ToastStack>();
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());
}
}
});
}
Expand All @@ -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::<ToastStack>();
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());
}
}
});
}
Expand All @@ -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::<ToastStack>();
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());
}
}
});
}
Expand Down Expand Up @@ -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::<ToastStack>();
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());
}
}
});
Expand Down
102 changes: 102 additions & 0 deletions crates/web/tests/browser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<IrohNetwork>` 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.<action>(...).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! { <ToastStackView/> }
});

// 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! { <ToastStackView/> }
});
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;
}
}