From 584d023b491d95403b7568bd9cf29e6a36fbe831 Mon Sep 17 00:00:00 2001 From: Duncan Date: Thu, 20 Aug 2026 21:50:48 -0400 Subject: [PATCH 01/10] fix(desktop): bound thread /query and surface load errors, not false-empty Long threads sometimes never loaded (permanent skeleton) or silently rendered a failed fetch as "No replies in this branch yet". Two defects: - The shared reqwest client sets no timeout, so a stalled/half-open /query HTTP request hangs forever. Add a 30s per-request deadline on both /query builders (scoped per-request, not client-level, because the client also serves STT/TTS downloads, builderlab auth, and the media proxy). Set above the 25s WS history timeout so a slow-but-live relay is not cut off early. Timeouts classify to the stable "relay unreachable: request timed out" string. - ChannelScreen consumed only isPending/data; a terminal error fell through to the empty state with no recovery. Plumb isError + refetch through to MessageThreadPanel and paint an explicit "Couldn't load replies" + Retry card. A pure selectThreadRepliesSurface helper pins the precedence so a terminal error never resolves to empty and cached rows stay visible non-destructively under a later error. To stay under the desktop file-size ratchet, move relay.rs's inline test module to relay/tests.rs and extract the reply empty/error cards (MessageThreadReplyState) and the per-row branch-highlight derivation (selectThreadRowHighlight) out of the panel, each with unit coverage. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/relay.rs | 399 +--------------- desktop/src-tauri/src/relay/tests.rs | 424 ++++++++++++++++++ .../src/features/channels/ui/ChannelPane.tsx | 4 + .../features/channels/ui/ChannelPane.types.ts | 2 + .../features/channels/ui/ChannelScreen.tsx | 4 + .../lib/threadReplyHighlight.test.mjs | 86 ++++ .../messages/lib/threadReplyHighlight.ts | 45 ++ .../messages/lib/timelineSnapshot.test.mjs | 121 +++++ .../features/messages/lib/timelineSnapshot.ts | 68 +++ .../messages/ui/MessageThreadPanel.tsx | 76 ++-- .../messages/ui/MessageThreadReplyState.tsx | 63 +++ 11 files changed, 873 insertions(+), 419 deletions(-) create mode 100644 desktop/src-tauri/src/relay/tests.rs create mode 100644 desktop/src/features/messages/lib/threadReplyHighlight.test.mjs create mode 100644 desktop/src/features/messages/lib/threadReplyHighlight.ts create mode 100644 desktop/src/features/messages/ui/MessageThreadReplyState.tsx diff --git a/desktop/src-tauri/src/relay.rs b/desktop/src-tauri/src/relay.rs index bd3fefb1259..4d2e9cf6fe9 100644 --- a/desktop/src-tauri/src/relay.rs +++ b/desktop/src-tauri/src/relay.rs @@ -16,6 +16,19 @@ const DEFAULT_RELAY_WS_URL: &str = "ws://localhost:3000"; // classifier keys on. Extracted to a const so a test can pin that contract. const MALFORMED_RESPONSE_MESSAGE: &str = "relay returned malformed response: not valid JSON"; +// Per-request deadline for the `POST /query` HTTP bridge, covering both the +// header exchange and full body consumption. The shared `http_client` sets no +// client-level timeout — deliberately, because it is also used for long-running +// STT/TTS model downloads, builderlab auth, and the media proxy — so a stalled +// or half-open `/query` connection would otherwise leave the request pending +// forever, hanging the caller (e.g. a thread-history load that never resolves +// and shows a permanent skeleton). A per-request timeout scoped to `/query` +// bounds that without affecting the client's other users. A timeout surfaces +// through `classify_request_error` as the stable `"relay unreachable: request +// timed out"` string. Set above the 25s WS history timeout so a slow-but-live +// relay is not cut off before the WebSocket path would be. +const QUERY_REQUEST_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30); + fn configured_env_var(name: &str) -> Option { std::env::var(name) .ok() @@ -334,6 +347,7 @@ pub async fn query_relay_at( .post(&url) .header("Authorization", auth) .header("Content-Type", "application/json") + .timeout(QUERY_REQUEST_TIMEOUT) .body(body_bytes) .send() .await @@ -362,7 +376,8 @@ pub async fn query_relay_at_with_keys( .http_client .post(&url) .header("Authorization", auth) - .header("Content-Type", "application/json"); + .header("Content-Type", "application/json") + .timeout(QUERY_REQUEST_TIMEOUT); if let Some(tag) = auth_tag { request = request.header("x-auth-tag", tag); } @@ -611,384 +626,4 @@ pub async fn submit_signed_event_with_keys( // ── Tests ─────────────────────────────────────────────────────────────────── #[cfg(test)] -mod tests { - use super::{ - build_profile_event, classify_intercepted_response, effective_agent_relay_url, - extract_retry_in_hint, parse_command_response, relay_http_base_url, - MALFORMED_RESPONSE_MESSAGE, - }; - use serde::Deserialize; - - // ── extract_retry_in_hint ──────────────────────────────────────────────── - - #[test] - fn extracts_hint_from_429_body() { - assert_eq!( - extract_retry_in_hint(r#"{"error":"rate-limited: quota exceeded; retry in 4s"}"#), - Some(4) - ); - } - - #[test] - fn extracts_hint_when_no_json_wrapper() { - assert_eq!(extract_retry_in_hint("retry in 30s"), Some(30)); - } - - #[test] - fn returns_none_when_no_hint_present() { - assert_eq!( - extract_retry_in_hint(r#"{"error":"rate-limited: quota exceeded"}"#), - None - ); - assert_eq!(extract_retry_in_hint(""), None); - } - - #[test] - fn overlong_digit_string_returns_none() { - // A digit sequence that exceeds u64::MAX cannot be parsed; the function - // must return None (→ caller uses the default) rather than panicking. - assert_eq!( - extract_retry_in_hint("retry in 99999999999999999999999s"), - None - ); - } - - // ── relay_error_message: hint capping ──────────────────────────────────── - // - // Verify that an oversized relay hint is capped in the returned message - // string, not just inside `activate_rate_limit()`. This guarantees every - // consumer — including the TS gate via `applyTauriRateLimitIfNeeded` — - // receives the capped value rather than the raw untrusted relay value. - - #[tokio::test] - async fn oversized_hint_is_capped_in_relay_error_message_string() { - use crate::relay_admission::{reset_rate_limit_gate, MAX_HINT_SECONDS, TEST_SERIAL}; - use std::io::{Read as _, Write as _}; - - let _serial = TEST_SERIAL.lock().await; - reset_rate_limit_gate(); - - // Use a std::net listener on a std::thread — the same pattern as the - // relay_admission loopback tests. This avoids two races that cause CI - // failures with tokio::net + into_std(): - // 1. No request read: the client is still sending when the response - // arrives → hyper `UnexpectedMessage`/`Canceled` under load. - // 2. into_std() leaves the socket in nonblocking mode → write_all - // may return WouldBlock and silently drop the response. - let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); - let addr = listener.local_addr().unwrap(); - - // Serve a 429 with a hint far exceeding MAX_HINT_SECONDS (300). - let oversized = 1_000_000u64; - let body = format!(r#"{{"error":"rate-limited: quota exceeded; retry in {oversized}s"}}"#); - let body_len = body.len(); - std::thread::spawn(move || { - if let Ok((mut stream, _)) = listener.accept() { - // Read the request first so the client finishes sending before - // we write the response — mirrors relay_admission.rs pattern. - let mut buf = [0u8; 4096]; - let _ = stream.read(&mut buf); - let response = format!( - "HTTP/1.1 429 Too Many Requests\r\nContent-Type: application/json\r\nContent-Length: {body_len}\r\nConnection: close\r\n\r\n{body}" - ); - let _ = stream.write_all(response.as_bytes()); - let _ = stream.flush(); - } - }); - - let client = reqwest::Client::new(); - let response = client - .get(format!("http://{addr}/")) - .send() - .await - .expect("request must succeed"); - - let msg = super::relay_error_message(response).await; - - // The message must embed the CAPPED hint, not the raw 1 000 000. - assert_eq!( - msg, - format!("relay rate-limited: retry in {MAX_HINT_SECONDS}s"), - "relay_error_message must embed the capped hint, not the raw untrusted value" - ); - assert!( - !msg.contains(&oversized.to_string()), - "raw oversized hint must not appear in the message string" - ); - reset_rate_limit_gate(); - } - - // ── effective_agent_relay_url: legacy pin ignored ───────────────────────── - - #[test] - fn stored_relay_pin_is_ignored() { - // Zero-touch cutover (#2122): a creation-era per-record relay pin is - // parsed and persisted but never consulted — the workspace relay wins. - assert_eq!( - effective_agent_relay_url("wss://relay.other.com", "wss://staging.example.com"), - "wss://staging.example.com" - ); - } - - #[test] - fn empty_relay_resolves_to_workspace() { - // A never-set record resolves to the active workspace relay at read-time, - // so a stale stored default can never make it load-bearing. - assert_eq!( - effective_agent_relay_url("", "wss://staging.example.com"), - "wss://staging.example.com" - ); - } - - #[test] - fn whitespace_only_relay_resolves_to_workspace() { - // Whitespace-only behaves identically — no value survives. - assert_eq!( - effective_agent_relay_url(" ", "wss://staging.example.com"), - "wss://staging.example.com" - ); - } - - // ── relay_http_base_url scheme conversion ──────────────────────────────── - - #[test] - fn loopback_ws_localhost_preserves_authority() { - // Tenant host-binding keys off the HTTP Host/authority. The desktop must - // not rewrite localhost to 127.0.0.1, or local dev HTTP calls target a - // different unmapped community than the WebSocket URL. - assert_eq!( - relay_http_base_url("ws://localhost:3000"), - "http://localhost:3000" - ); - } - - #[test] - fn loopback_trailing_slash_removed_authority_preserved() { - assert_eq!( - relay_http_base_url("ws://localhost:3000/"), - "http://localhost:3000" - ); - } - - #[test] - fn remote_wss_host_unchanged() { - assert_eq!( - relay_http_base_url("wss://relay.example.com"), - "https://relay.example.com" - ); - } - - #[test] - fn loopback_ipv4_literal_unchanged() { - assert_eq!( - relay_http_base_url("ws://127.0.0.1:3000"), - "http://127.0.0.1:3000" - ); - } - - #[test] - fn localhost_substring_host_unchanged() { - assert_eq!( - relay_http_base_url("ws://localhost.evil.com:3000"), - "http://localhost.evil.com:3000" - ); - } - - #[test] - fn loopback_wss_localhost_preserves_authority() { - assert_eq!( - relay_http_base_url("wss://localhost:3000"), - "https://localhost:3000" - ); - } - - // ── classify_intercepted_response ──────────────────────────────────────── - - #[test] - fn intercepted_cloudflare_host_returns_some() { - let result = classify_intercepted_response("sqprod.cloudflareaccess.com", "text/html"); - assert!(result.is_some()); - let msg = result.unwrap(); - assert!( - msg.starts_with("relay unreachable:"), - "should have unreachable prefix" - ); - assert!(msg.contains("Cloudflare"), "should mention Cloudflare"); - } - - #[test] - fn intercepted_cloudflare_apex_host_returns_some() { - // The apex domain itself should also match. - let result = classify_intercepted_response("cloudflareaccess.com", "application/json"); - assert!(result.is_some()); - let msg = result.unwrap(); - assert!(msg.starts_with("relay unreachable:")); - assert!(msg.contains("Cloudflare")); - } - - #[test] - fn intercepted_non_cloudflare_html_returns_some() { - let result = - classify_intercepted_response("proxy.corporate.example", "text/html; charset=utf-8"); - assert!(result.is_some()); - let msg = result.unwrap(); - assert!(msg.starts_with("relay unreachable:")); - } - - #[test] - fn normal_relay_json_returns_none() { - let result = classify_intercepted_response("relay.myapp.example.com", "application/json"); - assert!(result.is_none()); - } - - #[test] - fn content_type_case_insensitive() { - // Uppercase content-type must still be detected. - let result = classify_intercepted_response("proxy.example.com", "TEXT/HTML"); - assert!(result.is_some()); - assert!(result.unwrap().starts_with("relay unreachable:")); - } - - #[test] - fn evil_suffix_does_not_match_cloudflare() { - // A host whose suffix happens to contain the Cloudflare string but is - // not actually a subdomain must NOT match. - let result = classify_intercepted_response( - "notcloudflareaccess.com.evil.example", - "application/json", - ); - assert!( - result.is_none(), - "false suffix match should not trigger Cloudflare branch" - ); - } - - // classify_request_error requires a real reqwest::Error (not publicly - // constructable) — tested indirectly through integration; skipped here. - - // ── parse_json_response malformed-body contract ────────────────────────── - - #[test] - fn malformed_response_message_stays_off_unreachable_bucket() { - // A reached-but-malformed 2xx body is not a connectivity failure. If this - // message ever regains the "relay unreachable:" prefix, the frontend - // classifier would misroute it as unreachable — pin that it never does. - assert!( - !MALFORMED_RESPONSE_MESSAGE.starts_with("relay unreachable:"), - "malformed-response message must not match the unreachable prefix" - ); - } - - // ── parse_command_response ─────────────────────────────────────────────── - - #[derive(Debug, Deserialize, PartialEq)] - struct ChannelCreated { - channel_id: String, - } - - #[test] - fn parse_command_response_decodes_typed_payload() { - let msg = r#"response:{"channel_id":"abc123"}"#; - let parsed: ChannelCreated = parse_command_response(msg).expect("should parse"); - assert_eq!( - parsed, - ChannelCreated { - channel_id: "abc123".to_string() - } - ); - } - - #[test] - fn parse_command_response_accepts_raw_json_fallback() { - // Backward-compat: relays that emit raw JSON (no prefix) still work. - let msg = r#"{"channel_id":"abc"}"#; - let parsed: ChannelCreated = parse_command_response(msg).expect("fallback parse"); - assert_eq!( - parsed, - ChannelCreated { - channel_id: "abc".to_string() - } - ); - } - - #[test] - fn parse_command_response_rejects_invalid_prefixed_json() { - let msg = "response:not-json"; - let result: Result = parse_command_response(msg); - assert!(result.is_err()); - assert!(result.unwrap_err().contains("response parse failed")); - } - - #[test] - fn parse_command_response_rejects_garbage() { - let msg = "totally not json or response"; - let result: Result = parse_command_response(msg); - assert!(result.is_err()); - } - - // ── build_profile_event ────────────────────────────────────────────────── - - /// Generate a valid NIP-OA auth tag JSON string signed by a fresh owner key - /// and addressed to `agent_keys`. - /// - /// Uses `nostr_compat` (nostr 0.36) for the owner keys because - /// `buzz_sdk_pkg::nip_oa::compute_auth_tag` expects nostr 0.36 types. - /// The agent pubkey is bridged via hex encoding. - fn make_valid_auth_tag(agent_keys: &nostr::Keys) -> String { - let owner_keys = nostr::Keys::generate(); - let agent_pubkey_hex = agent_keys.public_key().to_hex(); - let agent_compat_pubkey = - nostr::PublicKey::from_hex(&agent_pubkey_hex).expect("valid hex pubkey should parse"); - buzz_sdk_pkg::nip_oa::compute_auth_tag(&owner_keys, &agent_compat_pubkey, "") - .expect("compute_auth_tag should not fail with distinct keys") - } - - #[test] - fn profile_event_with_valid_auth_tag() { - let agent_keys = nostr::Keys::generate(); - let tag_json = make_valid_auth_tag(&agent_keys); - let event = build_profile_event(&agent_keys, "TestBot", None, Some(&tag_json)) - .expect("should succeed with a valid auth tag"); - - // Exactly one "auth" tag must be present. - let auth_tags: Vec<_> = event - .tags - .iter() - .filter(|t| t.as_slice().first().map(|s| s.as_str()) == Some("auth")) - .collect(); - assert_eq!(auth_tags.len(), 1, "expected exactly 1 auth tag"); - - // Must be a kind:0 (Metadata) event. - assert_eq!(event.kind, nostr::Kind::Metadata); - } - - #[test] - fn profile_event_without_auth_tag() { - let agent_keys = nostr::Keys::generate(); - let event = build_profile_event(&agent_keys, "TestBot", None, None) - .expect("should succeed without an auth tag"); - - // No "auth" tags should be present. - let auth_tags: Vec<_> = event - .tags - .iter() - .filter(|t| t.as_slice().first().map(|s| s.as_str()) == Some("auth")) - .collect(); - assert_eq!(auth_tags.len(), 0, "expected no auth tags"); - - assert_eq!(event.kind, nostr::Kind::Metadata); - } - - #[test] - fn profile_event_rejects_invalid_auth_tag() { - let agent_keys = nostr::Keys::generate(); - // Structurally valid JSON array but with a bogus signature — verification must fail. - let bad_json = format!(r#"["auth","{}","","{}"]"#, "a".repeat(64), "b".repeat(128)); - let result = build_profile_event(&agent_keys, "TestBot", None, Some(&bad_json)); - assert!(result.is_err(), "should reject an invalid auth tag"); - assert!( - result.unwrap_err().contains("verification failed"), - "error message should mention verification failure" - ); - } -} +mod tests; diff --git a/desktop/src-tauri/src/relay/tests.rs b/desktop/src-tauri/src/relay/tests.rs new file mode 100644 index 00000000000..3bf45900797 --- /dev/null +++ b/desktop/src-tauri/src/relay/tests.rs @@ -0,0 +1,424 @@ +//! Unit tests for the relay HTTP/command bridge helpers. +//! Extracted from `relay.rs` to keep that module under the file-size ratchet. + +use super::{ + build_profile_event, classify_intercepted_response, effective_agent_relay_url, + extract_retry_in_hint, parse_command_response, relay_http_base_url, MALFORMED_RESPONSE_MESSAGE, +}; +use serde::Deserialize; + +// ── extract_retry_in_hint ──────────────────────────────────────────────── + +#[test] +fn extracts_hint_from_429_body() { + assert_eq!( + extract_retry_in_hint(r#"{"error":"rate-limited: quota exceeded; retry in 4s"}"#), + Some(4) + ); +} + +#[test] +fn extracts_hint_when_no_json_wrapper() { + assert_eq!(extract_retry_in_hint("retry in 30s"), Some(30)); +} + +#[test] +fn returns_none_when_no_hint_present() { + assert_eq!( + extract_retry_in_hint(r#"{"error":"rate-limited: quota exceeded"}"#), + None + ); + assert_eq!(extract_retry_in_hint(""), None); +} + +#[test] +fn overlong_digit_string_returns_none() { + // A digit sequence that exceeds u64::MAX cannot be parsed; the function + // must return None (→ caller uses the default) rather than panicking. + assert_eq!( + extract_retry_in_hint("retry in 99999999999999999999999s"), + None + ); +} + +// ── relay_error_message: hint capping ──────────────────────────────────── +// +// Verify that an oversized relay hint is capped in the returned message +// string, not just inside `activate_rate_limit()`. This guarantees every +// consumer — including the TS gate via `applyTauriRateLimitIfNeeded` — +// receives the capped value rather than the raw untrusted relay value. + +#[tokio::test] +async fn oversized_hint_is_capped_in_relay_error_message_string() { + use crate::relay_admission::{reset_rate_limit_gate, MAX_HINT_SECONDS, TEST_SERIAL}; + use std::io::{Read as _, Write as _}; + + let _serial = TEST_SERIAL.lock().await; + reset_rate_limit_gate(); + + // Use a std::net listener on a std::thread — the same pattern as the + // relay_admission loopback tests. This avoids two races that cause CI + // failures with tokio::net + into_std(): + // 1. No request read: the client is still sending when the response + // arrives → hyper `UnexpectedMessage`/`Canceled` under load. + // 2. into_std() leaves the socket in nonblocking mode → write_all + // may return WouldBlock and silently drop the response. + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + + // Serve a 429 with a hint far exceeding MAX_HINT_SECONDS (300). + let oversized = 1_000_000u64; + let body = format!(r#"{{"error":"rate-limited: quota exceeded; retry in {oversized}s"}}"#); + let body_len = body.len(); + std::thread::spawn(move || { + if let Ok((mut stream, _)) = listener.accept() { + // Read the request first so the client finishes sending before + // we write the response — mirrors relay_admission.rs pattern. + let mut buf = [0u8; 4096]; + let _ = stream.read(&mut buf); + let response = format!( + "HTTP/1.1 429 Too Many Requests\r\nContent-Type: application/json\r\nContent-Length: {body_len}\r\nConnection: close\r\n\r\n{body}" + ); + let _ = stream.write_all(response.as_bytes()); + let _ = stream.flush(); + } + }); + + let client = reqwest::Client::new(); + let response = client + .get(format!("http://{addr}/")) + .send() + .await + .expect("request must succeed"); + + let msg = super::relay_error_message(response).await; + + // The message must embed the CAPPED hint, not the raw 1 000 000. + assert_eq!( + msg, + format!("relay rate-limited: retry in {MAX_HINT_SECONDS}s"), + "relay_error_message must embed the capped hint, not the raw untrusted value" + ); + assert!( + !msg.contains(&oversized.to_string()), + "raw oversized hint must not appear in the message string" + ); + reset_rate_limit_gate(); +} + +// ── effective_agent_relay_url: legacy pin ignored ───────────────────────── + +#[test] +fn stored_relay_pin_is_ignored() { + // Zero-touch cutover (#2122): a creation-era per-record relay pin is + // parsed and persisted but never consulted — the workspace relay wins. + assert_eq!( + effective_agent_relay_url("wss://relay.other.com", "wss://staging.example.com"), + "wss://staging.example.com" + ); +} + +#[test] +fn empty_relay_resolves_to_workspace() { + // A never-set record resolves to the active workspace relay at read-time, + // so a stale stored default can never make it load-bearing. + assert_eq!( + effective_agent_relay_url("", "wss://staging.example.com"), + "wss://staging.example.com" + ); +} + +#[test] +fn whitespace_only_relay_resolves_to_workspace() { + // Whitespace-only behaves identically — no value survives. + assert_eq!( + effective_agent_relay_url(" ", "wss://staging.example.com"), + "wss://staging.example.com" + ); +} + +// ── relay_http_base_url scheme conversion ──────────────────────────────── + +#[test] +fn loopback_ws_localhost_preserves_authority() { + // Tenant host-binding keys off the HTTP Host/authority. The desktop must + // not rewrite localhost to 127.0.0.1, or local dev HTTP calls target a + // different unmapped community than the WebSocket URL. + assert_eq!( + relay_http_base_url("ws://localhost:3000"), + "http://localhost:3000" + ); +} + +#[test] +fn loopback_trailing_slash_removed_authority_preserved() { + assert_eq!( + relay_http_base_url("ws://localhost:3000/"), + "http://localhost:3000" + ); +} + +#[test] +fn remote_wss_host_unchanged() { + assert_eq!( + relay_http_base_url("wss://relay.example.com"), + "https://relay.example.com" + ); +} + +#[test] +fn loopback_ipv4_literal_unchanged() { + assert_eq!( + relay_http_base_url("ws://127.0.0.1:3000"), + "http://127.0.0.1:3000" + ); +} + +#[test] +fn localhost_substring_host_unchanged() { + assert_eq!( + relay_http_base_url("ws://localhost.evil.com:3000"), + "http://localhost.evil.com:3000" + ); +} + +#[test] +fn loopback_wss_localhost_preserves_authority() { + assert_eq!( + relay_http_base_url("wss://localhost:3000"), + "https://localhost:3000" + ); +} + +// ── classify_intercepted_response ──────────────────────────────────────── + +#[test] +fn intercepted_cloudflare_host_returns_some() { + let result = classify_intercepted_response("sqprod.cloudflareaccess.com", "text/html"); + assert!(result.is_some()); + let msg = result.unwrap(); + assert!( + msg.starts_with("relay unreachable:"), + "should have unreachable prefix" + ); + assert!(msg.contains("Cloudflare"), "should mention Cloudflare"); +} + +#[test] +fn intercepted_cloudflare_apex_host_returns_some() { + // The apex domain itself should also match. + let result = classify_intercepted_response("cloudflareaccess.com", "application/json"); + assert!(result.is_some()); + let msg = result.unwrap(); + assert!(msg.starts_with("relay unreachable:")); + assert!(msg.contains("Cloudflare")); +} + +#[test] +fn intercepted_non_cloudflare_html_returns_some() { + let result = + classify_intercepted_response("proxy.corporate.example", "text/html; charset=utf-8"); + assert!(result.is_some()); + let msg = result.unwrap(); + assert!(msg.starts_with("relay unreachable:")); +} + +#[test] +fn normal_relay_json_returns_none() { + let result = classify_intercepted_response("relay.myapp.example.com", "application/json"); + assert!(result.is_none()); +} + +#[test] +fn content_type_case_insensitive() { + // Uppercase content-type must still be detected. + let result = classify_intercepted_response("proxy.example.com", "TEXT/HTML"); + assert!(result.is_some()); + assert!(result.unwrap().starts_with("relay unreachable:")); +} + +#[test] +fn evil_suffix_does_not_match_cloudflare() { + // A host whose suffix happens to contain the Cloudflare string but is + // not actually a subdomain must NOT match. + let result = + classify_intercepted_response("notcloudflareaccess.com.evil.example", "application/json"); + assert!( + result.is_none(), + "false suffix match should not trigger Cloudflare branch" + ); +} + +// classify_request_error requires a real reqwest::Error (not publicly +// constructable) — tested indirectly through integration; skipped here. + +// ── /query per-request timeout → classified error ──────────────────────── +// +// A stalled `/query` connection (headers never arrive) must not hang the +// caller forever. The per-request `.timeout(...)` on the `/query` builders +// bounds the wait, and the resulting `reqwest::Error` must classify to the +// stable `"relay unreachable: request timed out"` string the frontend keys +// on. This exercises the real timeout path with a short deadline against a +// loopback server that accepts the connection but never sends a response. +#[tokio::test] +async fn stalled_query_request_times_out_with_classified_error() { + use std::io::Read as _; + + // A listener that accepts the connection and then holds it open without + // ever writing a response — the "headers never arrive" stall. + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + let handle = std::thread::spawn(move || { + if let Ok((mut stream, _)) = listener.accept() { + // Drain the request but deliberately never respond, then hold + // the socket until the client aborts on its own timeout. + let mut buf = [0u8; 4096]; + let _ = stream.read(&mut buf); + std::thread::sleep(std::time::Duration::from_secs(2)); + } + }); + + let client = reqwest::Client::new(); + let err = client + .post(format!("http://{addr}/query")) + .timeout(std::time::Duration::from_millis(200)) + .body("[]") + .send() + .await + .expect_err("request must fail on the per-request timeout"); + + assert!(err.is_timeout(), "stalled request must be a timeout error"); + assert_eq!( + super::classify_request_error(&err), + "relay unreachable: request timed out", + "a timed-out /query must surface the stable classified string" + ); + + let _ = handle.join(); +} + +// ── parse_json_response malformed-body contract ────────────────────────── + +#[test] +fn malformed_response_message_stays_off_unreachable_bucket() { + // A reached-but-malformed 2xx body is not a connectivity failure. If this + // message ever regains the "relay unreachable:" prefix, the frontend + // classifier would misroute it as unreachable — pin that it never does. + assert!( + !MALFORMED_RESPONSE_MESSAGE.starts_with("relay unreachable:"), + "malformed-response message must not match the unreachable prefix" + ); +} + +// ── parse_command_response ─────────────────────────────────────────────── + +#[derive(Debug, Deserialize, PartialEq)] +struct ChannelCreated { + channel_id: String, +} + +#[test] +fn parse_command_response_decodes_typed_payload() { + let msg = r#"response:{"channel_id":"abc123"}"#; + let parsed: ChannelCreated = parse_command_response(msg).expect("should parse"); + assert_eq!( + parsed, + ChannelCreated { + channel_id: "abc123".to_string() + } + ); +} + +#[test] +fn parse_command_response_accepts_raw_json_fallback() { + // Backward-compat: relays that emit raw JSON (no prefix) still work. + let msg = r#"{"channel_id":"abc"}"#; + let parsed: ChannelCreated = parse_command_response(msg).expect("fallback parse"); + assert_eq!( + parsed, + ChannelCreated { + channel_id: "abc".to_string() + } + ); +} + +#[test] +fn parse_command_response_rejects_invalid_prefixed_json() { + let msg = "response:not-json"; + let result: Result = parse_command_response(msg); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("response parse failed")); +} + +#[test] +fn parse_command_response_rejects_garbage() { + let msg = "totally not json or response"; + let result: Result = parse_command_response(msg); + assert!(result.is_err()); +} + +// ── build_profile_event ────────────────────────────────────────────────── + +/// Generate a valid NIP-OA auth tag JSON string signed by a fresh owner key +/// and addressed to `agent_keys`. +/// +/// Uses `nostr_compat` (nostr 0.36) for the owner keys because +/// `buzz_sdk_pkg::nip_oa::compute_auth_tag` expects nostr 0.36 types. +/// The agent pubkey is bridged via hex encoding. +fn make_valid_auth_tag(agent_keys: &nostr::Keys) -> String { + let owner_keys = nostr::Keys::generate(); + let agent_pubkey_hex = agent_keys.public_key().to_hex(); + let agent_compat_pubkey = + nostr::PublicKey::from_hex(&agent_pubkey_hex).expect("valid hex pubkey should parse"); + buzz_sdk_pkg::nip_oa::compute_auth_tag(&owner_keys, &agent_compat_pubkey, "") + .expect("compute_auth_tag should not fail with distinct keys") +} + +#[test] +fn profile_event_with_valid_auth_tag() { + let agent_keys = nostr::Keys::generate(); + let tag_json = make_valid_auth_tag(&agent_keys); + let event = build_profile_event(&agent_keys, "TestBot", None, Some(&tag_json)) + .expect("should succeed with a valid auth tag"); + + // Exactly one "auth" tag must be present. + let auth_tags: Vec<_> = event + .tags + .iter() + .filter(|t| t.as_slice().first().map(|s| s.as_str()) == Some("auth")) + .collect(); + assert_eq!(auth_tags.len(), 1, "expected exactly 1 auth tag"); + + // Must be a kind:0 (Metadata) event. + assert_eq!(event.kind, nostr::Kind::Metadata); +} + +#[test] +fn profile_event_without_auth_tag() { + let agent_keys = nostr::Keys::generate(); + let event = build_profile_event(&agent_keys, "TestBot", None, None) + .expect("should succeed without an auth tag"); + + // No "auth" tags should be present. + let auth_tags: Vec<_> = event + .tags + .iter() + .filter(|t| t.as_slice().first().map(|s| s.as_str()) == Some("auth")) + .collect(); + assert_eq!(auth_tags.len(), 0, "expected no auth tags"); + + assert_eq!(event.kind, nostr::Kind::Metadata); +} + +#[test] +fn profile_event_rejects_invalid_auth_tag() { + let agent_keys = nostr::Keys::generate(); + // Structurally valid JSON array but with a bogus signature — verification must fail. + let bad_json = format!(r#"["auth","{}","","{}"]"#, "a".repeat(64), "b".repeat(128)); + let result = build_profile_event(&agent_keys, "TestBot", None, Some(&bad_json)); + assert!(result.is_err(), "should reject an invalid auth tag"); + assert!( + result.unwrap_err().contains("verification failed"), + "error message should mention verification failure" + ); +} diff --git a/desktop/src/features/channels/ui/ChannelPane.tsx b/desktop/src/features/channels/ui/ChannelPane.tsx index bccc163ed40..958b0ba738d 100644 --- a/desktop/src/features/channels/ui/ChannelPane.tsx +++ b/desktop/src/features/channels/ui/ChannelPane.tsx @@ -151,6 +151,8 @@ export const ChannelPane = React.memo(function ChannelPane({ threadHeadMessage, threadMessages, threadMessagesPending = false, + threadMessagesError = false, + onRetryThreadReplies, threadPanelWidthPx, threadScrollTargetId, threadTypingPubkeys, @@ -833,6 +835,8 @@ export const ChannelPane = React.memo(function ChannelPane({ widthPx={threadPanelWidthPx} threadReplies={threadMessages} threadRepliesPending={threadMessagesPending} + threadRepliesError={threadMessagesError} + onRetryThreadReplies={onRetryThreadReplies} threadUnreadCount={threadUnreadCounts?.get( threadHeadMessage.id, )} diff --git a/desktop/src/features/channels/ui/ChannelPane.types.ts b/desktop/src/features/channels/ui/ChannelPane.types.ts index 760ef58073b..10f05c2c145 100644 --- a/desktop/src/features/channels/ui/ChannelPane.types.ts +++ b/desktop/src/features/channels/ui/ChannelPane.types.ts @@ -170,6 +170,8 @@ export type ChannelPaneProps = { threadAllMessages: TimelineMessage[]; threadMessages: MainTimelineEntry[]; threadMessagesPending?: boolean; + threadMessagesError?: boolean; + onRetryThreadReplies?: () => void; threadPanelWidthPx: number; threadTypingPubkeys: string[]; threadReplyTargetMessage: TimelineMessage | null; diff --git a/desktop/src/features/channels/ui/ChannelScreen.tsx b/desktop/src/features/channels/ui/ChannelScreen.tsx index 68df9bc05c6..c6803604720 100644 --- a/desktop/src/features/channels/ui/ChannelScreen.tsx +++ b/desktop/src/features/channels/ui/ChannelScreen.tsx @@ -955,6 +955,10 @@ export function ChannelScreen({ threadHeadMessage={displayedThreadHeadMessage} threadMessages={displayedThreadMessages} threadMessagesPending={threadRepliesQuery.isPending} + threadMessagesError={threadRepliesQuery.isError} + onRetryThreadReplies={() => { + void threadRepliesQuery.refetch(); + }} threadPanelWidthPx={threadPanelWidthPx} threadTypingPubkeys={threadTypingPubkeys} threadReplyTargetMessage={displayedThreadReplyTargetMessage} diff --git a/desktop/src/features/messages/lib/threadReplyHighlight.test.mjs b/desktop/src/features/messages/lib/threadReplyHighlight.test.mjs new file mode 100644 index 00000000000..bad72b33395 --- /dev/null +++ b/desktop/src/features/messages/lib/threadReplyHighlight.test.mjs @@ -0,0 +1,86 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; +import { selectThreadRowHighlight } from "./threadReplyHighlight.ts"; + +// The hovered branch spans rows in the half-open range (startIndex, endIndex]. +const branch = { id: "b", depth: 1, startIndex: 2, endIndex: 5 }; + +test("row-highlight: null branch highlights nothing", () => { + assert.deepEqual( + selectThreadRowHighlight({ + branch: null, + index: 3, + messageId: "x", + messageDepth: 2, + showGuides: true, + }), + { + isBranchOwner: false, + isInsideBranch: false, + isDirectChild: false, + lineDepths: undefined, + }, + ); +}); + +test("row-highlight: the branch owner is flagged but is not inside its own range", () => { + const h = selectThreadRowHighlight({ + branch, + index: 2, + messageId: "b", + messageDepth: 1, + showGuides: true, + }); + assert.equal(h.isBranchOwner, true); + // startIndex is excluded, so the owner row itself is not "inside". + assert.equal(h.isInsideBranch, false); + assert.equal(h.lineDepths, undefined); +}); + +test("row-highlight: a direct child inside the branch draws the guide line", () => { + const h = selectThreadRowHighlight({ + branch, + index: 3, + messageId: "c", + messageDepth: 2, + showGuides: true, + }); + assert.equal(h.isInsideBranch, true); + assert.equal(h.isDirectChild, true); + assert.deepEqual(h.lineDepths, [1]); +}); + +test("row-highlight: a deeper descendant is inside but not a direct child", () => { + const h = selectThreadRowHighlight({ + branch, + index: 4, + messageId: "d", + messageDepth: 3, + showGuides: true, + }); + assert.equal(h.isInsideBranch, true); + assert.equal(h.isDirectChild, false); +}); + +test("row-highlight: a row past endIndex is outside the branch", () => { + const h = selectThreadRowHighlight({ + branch, + index: 6, + messageId: "e", + messageDepth: 2, + showGuides: true, + }); + assert.equal(h.isInsideBranch, false); +}); + +test("row-highlight: guides suppressed → no line depths even inside the branch", () => { + const h = selectThreadRowHighlight({ + branch, + index: 3, + messageId: "c", + messageDepth: 2, + showGuides: false, + }); + assert.equal(h.isInsideBranch, true); + assert.equal(h.lineDepths, undefined); +}); diff --git a/desktop/src/features/messages/lib/threadReplyHighlight.ts b/desktop/src/features/messages/lib/threadReplyHighlight.ts new file mode 100644 index 00000000000..0ce446daecb --- /dev/null +++ b/desktop/src/features/messages/lib/threadReplyHighlight.ts @@ -0,0 +1,45 @@ +/** + * The hovered collapse-branch range, or null when nothing is hovered. Rows whose + * index falls inside `(startIndex, endIndex]` belong to the branch. + */ +export type HighlightedThreadBranch = { + id: string; + depth: number; + startIndex: number; + endIndex: number; +} | null; + +/** Per-row branch-highlight flags derived from the hovered collapse branch. */ +export type ThreadRowHighlight = { + isBranchOwner: boolean; + isInsideBranch: boolean; + isDirectChild: boolean; + lineDepths: number[] | undefined; +}; + +/** + * Which highlight decorations a reply row shows for the hovered collapse branch. + * Pure so the index-range logic is unit-testable without rendering the panel. + */ +export function selectThreadRowHighlight({ + branch, + index, + messageId, + messageDepth, + showGuides, +}: { + branch: HighlightedThreadBranch; + index: number; + messageId: string; + messageDepth: number; + showGuides: boolean; +}): ThreadRowHighlight { + const isBranchOwner = branch?.id === messageId; + const isInsideBranch = + branch != null && index > branch.startIndex && index <= branch.endIndex; + const isDirectChild = + isInsideBranch && branch != null && messageDepth === branch.depth + 1; + const lineDepths = + showGuides && isInsideBranch && branch ? [branch.depth] : undefined; + return { isBranchOwner, isInsideBranch, isDirectChild, lineDepths }; +} diff --git a/desktop/src/features/messages/lib/timelineSnapshot.test.mjs b/desktop/src/features/messages/lib/timelineSnapshot.test.mjs index a0374fbe2b0..5334d58875f 100644 --- a/desktop/src/features/messages/lib/timelineSnapshot.test.mjs +++ b/desktop/src/features/messages/lib/timelineSnapshot.test.mjs @@ -14,6 +14,7 @@ import { selectLatestMessageKey, selectTimelineBodySurface, selectTimelineIntroSurface, + selectThreadRepliesSurface, } from "./timelineSnapshot.ts"; // Local-midnight unix-second timestamps so isSameDay (local time) is stable @@ -399,6 +400,126 @@ test("deferred-render: keys the empty decision off the live count, not deferred" assert.equal(selectDeferredListRenderState(0, 1), "pending"); }); +// ── selectThreadRepliesSurface ────────────────────────────────────────────── +// PR-1 defect 2: a terminal thread-load error must NEVER be presented as the +// authoritative "No replies in this branch yet" empty state. These pin the +// paint precedence that gates that in MessageThreadPanel. + +test("thread-surface: pending query paints the skeleton", () => { + assert.equal( + selectThreadRepliesSurface({ + isPending: true, + isError: false, + renderState: "empty", + }), + "skeleton", + ); +}); + +test("thread-surface: terminal error with no data paints error, never empty", () => { + // The core false-empty guard: the load failed (isError) and there is nothing + // cached (renderState "empty"). This MUST be "error" so the UI shows + // "Couldn't load replies" + Retry instead of an authoritative empty thread. + const surface = selectThreadRepliesSurface({ + isPending: false, + isError: true, + renderState: "empty", + }); + assert.equal(surface, "error"); + assert.notEqual(surface, "empty"); +}); + +test("thread-surface: page-2 failure with no committed rows never claims empty", () => { + // A later-page fetch rejects the whole attempt; partial rows are never + // committed, so the deferred+live lists are empty and isError is set. The + // surface must be "error", never "empty" — the thread is not known-empty. + assert.equal( + selectThreadRepliesSurface({ + isPending: false, + isError: true, + renderState: "empty", + }), + "error", + ); +}); + +test("thread-surface: cached rows stay visible even under a load error", () => { + // An error with cached replies (renderState "list") keeps painting the rows + // non-destructively rather than blanking them for the error card. + assert.equal( + selectThreadRepliesSurface({ + isPending: false, + isError: true, + renderState: "list", + }), + "list", + ); +}); + +test("thread-surface: successful empty load paints the empty state", () => { + // No error, genuinely no replies → the real empty affordance is correct. + assert.equal( + selectThreadRepliesSurface({ + isPending: false, + isError: false, + renderState: "empty", + }), + "empty", + ); +}); + +test("thread-surface: retry success renders the reply list", () => { + // After a Retry re-fetch succeeds, isError clears and rows commit + // (renderState "list") → the list body paints, replacing the error card. + assert.equal( + selectThreadRepliesSurface({ + isPending: false, + isError: false, + renderState: "list", + }), + "list", + ); +}); + +test("thread-surface: streaming-in rows paint nothing (pending), not empty", () => { + assert.equal( + selectThreadRepliesSurface({ + isPending: false, + isError: false, + renderState: "pending", + }), + "pending", + ); +}); + +test("thread-surface: huddle transcripts collapse non-list surfaces to pending", () => { + // Huddle transcripts flatten replies into the chat timeline, so they never + // show the skeleton/error/empty affordances — only the list body or nothing. + for (const isError of [false, true]) { + for (const renderState of ["empty", "pending"]) { + assert.equal( + selectThreadRepliesSurface({ + isPending: false, + isError, + renderState, + isHuddleTranscript: true, + }), + "pending", + ); + } + } + // The list body still paints for a transcript with rows. + assert.equal( + selectThreadRepliesSurface({ + isPending: false, + isError: false, + renderState: "list", + isHuddleTranscript: true, + }), + "list", + ); +}); + test("timeline-body-surface: loading and deferred-pending both paint the single static skeleton", () => { assert.equal( selectTimelineBodySurface({ diff --git a/desktop/src/features/messages/lib/timelineSnapshot.ts b/desktop/src/features/messages/lib/timelineSnapshot.ts index 3bfd9349476..4e23fb22453 100644 --- a/desktop/src/features/messages/lib/timelineSnapshot.ts +++ b/desktop/src/features/messages/lib/timelineSnapshot.ts @@ -209,6 +209,74 @@ export function selectTimelineBodySurface({ return renderState; } +/** + * Which surface the thread-reply body should paint, in strict precedence. + * + * Extracted as a pure function so the load-bearing invariant — a terminal fetch + * error must NEVER be shown as the "empty" (no-replies) state — is unit-tested + * without a DOM. The precedence mirrors the JSX branch order in + * `MessageThreadPanel`: + * + * 1. "skeleton" → the query is still pending (first load, no cache) + * 2. "list" → the deferred snapshot has rows; paint them (even under a + * later error, cached replies stay visible non-destructively) + * 3. "error" → the load terminally failed and there is nothing to show; + * paint "Couldn't load replies" + Retry, never the empty state + * 4. "empty" → the load succeeded and the branch is genuinely empty + * 5. "pending" → deferred is empty but the live list has content; paint + * nothing yet (rows are streaming in on the deferred commit) + * + * Huddle transcripts flatten replies into the chat timeline and never show the + * skeleton/error/empty affordances, so their non-list surfaces collapse to + * "pending" (render nothing). + */ +export type ThreadRepliesSurface = + | "skeleton" + | "list" + | "error" + | "empty" + | "pending"; + +export function selectThreadRepliesSurface({ + isPending, + isError, + renderState, + isHuddleTranscript = false, +}: { + isPending: boolean; + isError: boolean; + renderState: DeferredListRenderState; + isHuddleTranscript?: boolean; +}): ThreadRepliesSurface { + const surface = resolveThreadRepliesSurface({ + isPending, + isError, + renderState, + }); + return isHuddleTranscript && surface !== "list" ? "pending" : surface; +} + +function resolveThreadRepliesSurface({ + isPending, + isError, + renderState, +}: { + isPending: boolean; + isError: boolean; + renderState: DeferredListRenderState; +}): ThreadRepliesSurface { + if (isPending) { + return "skeleton"; + } + if (renderState === "list") { + return "list"; + } + if (isError) { + return "error"; + } + return renderState; +} + export type TimelineMessageDelta = "prepend" | "append" | "replace" | "none"; export function classifyTimelineMessageDelta({ diff --git a/desktop/src/features/messages/ui/MessageThreadPanel.tsx b/desktop/src/features/messages/ui/MessageThreadPanel.tsx index d2650c84ac4..8c495091b4f 100644 --- a/desktop/src/features/messages/ui/MessageThreadPanel.tsx +++ b/desktop/src/features/messages/ui/MessageThreadPanel.tsx @@ -41,12 +41,20 @@ import { import type { ThreadDepthGuideAction } from "./MessageRow"; import { MessageThreadRow } from "./MessageThreadRow"; import { MessageThreadSummaryRow } from "./MessageThreadSummaryRow"; +import { + ThreadRepliesEmptyCard, + ThreadRepliesErrorCard, +} from "./MessageThreadReplyState"; import { TypingIndicatorRow } from "./TypingIndicatorRow"; import { UnreadDivider } from "./UnreadDivider"; import { useComposerHeightPadding } from "./useComposerHeightPadding"; import { useStableSendToChannel } from "./useStableSendToChannel"; import { useAnchoredScroll } from "./useAnchoredScroll"; -import { selectDeferredListRenderState } from "@/features/messages/lib/timelineSnapshot"; +import { + selectDeferredListRenderState, + selectThreadRepliesSurface, +} from "@/features/messages/lib/timelineSnapshot"; +import { selectThreadRowHighlight } from "@/features/messages/lib/threadReplyHighlight"; type MessageThreadPanelProps = ThreadPanelLayoutProps & { channel: Channel | null; @@ -106,6 +114,10 @@ type MessageThreadPanelProps = ThreadPanelLayoutProps & { threadHead: TimelineMessage | null; threadReplies: MainTimelineEntry[]; threadRepliesPending?: boolean; + /** True when the thread-reply query terminally failed (all retries exhausted). */ + threadRepliesError?: boolean; + /** Retries the failed thread-reply load; wired to the query's `refetch`. */ + onRetryThreadReplies?: () => void; threadUnreadCount?: number; threadReplyUnreadCounts?: ReadonlyMap; threadTypingPubkeys: string[]; @@ -233,6 +245,8 @@ export function MessageThreadPanel({ videoReviewPresentation, threadReplies, threadRepliesPending = false, + threadRepliesError = false, + onRetryThreadReplies, threadUnreadCount, threadReplyUnreadCounts, threadTypingPubkeys, @@ -356,6 +370,13 @@ export function MessageThreadPanel({ deferredThreadReplies.length, threadReplies.length, ); + // One paint decision: a terminal error never falls through to "No replies". + const repliesSurface = selectThreadRepliesSurface({ + isPending: threadRepliesPending, + isError: threadRepliesError, + renderState: repliesRenderState, + isHuddleTranscript, + }); const threadHeadSummary = React.useMemo(() => { if (!threadHeadId) { return null; @@ -629,7 +650,7 @@ export function MessageThreadPanel({ className={cn(THREAD_PANEL_MESSAGE_GUTTER_CLASS, "pb-3 pt-0")} data-testid="message-thread-replies" > - {threadRepliesPending && !isHuddleTranscript ? ( + {repliesSurface === "skeleton" ? (
- ) : repliesRenderState === "list" ? ( + ) : repliesSurface === "list" ? ( visibleThreadHeadSummary ? (
0 && entry.message.id === firstUnreadReplyId; - const isHighlightedBranchOwner = - highlightedBranch?.id === entry.message.id; - const isInsideHighlightedBranch = - highlightedBranch != null && - index > highlightedBranch.startIndex && - index <= highlightedBranch.endIndex; - const isDirectChildOfHighlightedBranch = - isInsideHighlightedBranch && - highlightedBranch != null && - index > highlightedBranch.startIndex && - index <= highlightedBranch.endIndex && - entry.message.depth === highlightedBranch.depth + 1; - const highlightedLineDepths = - shouldShowThreadBranchGuides && - isInsideHighlightedBranch && - highlightedBranch - ? [highlightedBranch.depth] - : undefined; + const highlight = selectThreadRowHighlight({ + branch: highlightedBranch, + index, + messageId: entry.message.id, + messageDepth: entry.message.depth, + showGuides: shouldShowThreadBranchGuides, + }); return (
) - ) : repliesRenderState === "empty" && !isHuddleTranscript ? ( - // Only show the empty state when the thread is GENUINELY empty. - // Keying off `deferredThreadReplies` would flash "No replies" for a - // frame while a non-empty list streams in on the deferred commit. -
-

- No replies in this branch yet -

-

- Reply in the thread to continue this branch. -

-
+ ) : repliesSurface === "error" ? ( + + ) : repliesSurface === "empty" ? ( + ) : // "pending": deferred list is empty but the live list has content — // rows are streaming in on the deferred commit. Paint nothing rather // than flashing the empty state. diff --git a/desktop/src/features/messages/ui/MessageThreadReplyState.tsx b/desktop/src/features/messages/ui/MessageThreadReplyState.tsx new file mode 100644 index 00000000000..e52731b2e69 --- /dev/null +++ b/desktop/src/features/messages/ui/MessageThreadReplyState.tsx @@ -0,0 +1,63 @@ +import { Button } from "@/shared/ui/button"; + +/** + * Terminal empty/error states for the thread reply region. + * + * These are the two non-list, non-loading outcomes of a thread-reply load. They + * live here (rather than inline in `MessageThreadPanel`) so the load-bearing + * distinction between them stays legible: a genuinely empty branch and a failed + * fetch look similar but must never be confused — see `selectThreadRepliesSurface`. + */ + +/** + * A terminal load failure. This must NEVER be painted as the empty state — that + * silently presents a broken fetch as an authoritative "no replies" and offers + * no recovery. Any cached replies still render via the panel's "list" branch, so + * this only surfaces when the failed load left nothing to show. + */ +export function ThreadRepliesErrorCard({ onRetry }: { onRetry?: () => void }) { + return ( +
+

+ Couldn't load replies +

+

+ The thread history didn't load. Check your connection and try + again. +

+ {onRetry ? ( + + ) : null} +
+ ); +} + +/** + * A branch that genuinely has no replies (the load succeeded and returned none). + * Only ever painted off the committed render state, never the raw deferred list, + * so it can't flash while a non-empty list streams in on the deferred commit. + */ +export function ThreadRepliesEmptyCard() { + return ( +
+

+ No replies in this branch yet +

+

+ Reply in the thread to continue this branch. +

+
+ ); +} From 5d4d5fa1cea0a61dada47bfcc76afb2e745d714a Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 21 Aug 2026 10:52:01 -0400 Subject: [PATCH 02/10] test(desktop): close thread-load flake fix test seams The prior tests could stay green while their production fixes were reverted. Route both /query builders through one send_query_request helper that owns the per-request timeout, and drive that real helper from the stalled-loopback test under an outer tokio timeout guard so a lost timeout hangs and the guard fails fast. Extract the panel's terminal reply surface into an exported ThreadRepliesTerminalCard and mount-test error/empty/pending + Retry, so reverting the panel breaks the import. Move two pure branch-guide helpers to threadPanel.ts to keep the panel under the file-size ratchet. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/relay.rs | 60 +++++---- desktop/src-tauri/src/relay/tests.rs | 48 +++++--- .../src/features/messages/lib/threadPanel.ts | 56 +++++++++ .../messages/ui/MessageThreadPanel.tsx | 77 ++++-------- .../ui/MessageThreadReplyState.test.mjs | 116 ++++++++++++++++++ 5 files changed, 264 insertions(+), 93 deletions(-) create mode 100644 desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs diff --git a/desktop/src-tauri/src/relay.rs b/desktop/src-tauri/src/relay.rs index 4d2e9cf6fe9..adf401d8dca 100644 --- a/desktop/src-tauri/src/relay.rs +++ b/desktop/src-tauri/src/relay.rs @@ -341,23 +341,15 @@ pub async fn query_relay_at( let body_bytes = serde_json::to_vec(filters).map_err(|e| format!("filter serialization failed: {e}"))?; let auth = build_nip98_auth_header(&Method::POST, &url, &body_bytes, state)?; - - let response = state - .http_client - .post(&url) - .header("Authorization", auth) - .header("Content-Type", "application/json") - .timeout(QUERY_REQUEST_TIMEOUT) - .body(body_bytes) - .send() - .await - .map_err(|e| classify_request_error(&e))?; - - if !response.status().is_success() { - return Err(relay_error_message(response).await); - } - - parse_json_response(response).await + send_query_request( + &state.http_client, + &url, + &auth, + None, + body_bytes, + QUERY_REQUEST_TIMEOUT, + ) + .await } pub async fn query_relay_at_with_keys( @@ -372,12 +364,38 @@ pub async fn query_relay_at_with_keys( let body_bytes = serde_json::to_vec(filters).map_err(|e| format!("filter serialization failed: {e}"))?; let auth = build_nip98_auth_header_for_keys(keys, &Method::POST, &url, &body_bytes)?; - let mut request = state - .http_client - .post(&url) + send_query_request( + &state.http_client, + &url, + &auth, + auth_tag, + body_bytes, + QUERY_REQUEST_TIMEOUT, + ) + .await +} + +/// Issue an authenticated `POST /query` and parse the response, applying the +/// per-request `timeout` that bounds a stalled or half-open relay connection. +/// +/// Both `/query` builders funnel through this one helper so the timeout can +/// never be applied to one builder and dropped from the other, and so a test +/// can drive the real send/timeout/classify path with a short deadline against +/// a stalled loopback. A timeout surfaces through `classify_request_error` as +/// the stable `"relay unreachable: request timed out"` string. +async fn send_query_request( + http_client: &reqwest::Client, + url: &str, + auth: &str, + auth_tag: Option<&str>, + body_bytes: Vec, + timeout: std::time::Duration, +) -> Result, String> { + let mut request = http_client + .post(url) .header("Authorization", auth) .header("Content-Type", "application/json") - .timeout(QUERY_REQUEST_TIMEOUT); + .timeout(timeout); if let Some(tag) = auth_tag { request = request.header("x-auth-tag", tag); } diff --git a/desktop/src-tauri/src/relay/tests.rs b/desktop/src-tauri/src/relay/tests.rs index 3bf45900797..6a46fc1ebce 100644 --- a/desktop/src-tauri/src/relay/tests.rs +++ b/desktop/src-tauri/src/relay/tests.rs @@ -255,14 +255,21 @@ fn evil_suffix_does_not_match_cloudflare() { // ── /query per-request timeout → classified error ──────────────────────── // // A stalled `/query` connection (headers never arrive) must not hang the -// caller forever. The per-request `.timeout(...)` on the `/query` builders -// bounds the wait, and the resulting `reqwest::Error` must classify to the -// stable `"relay unreachable: request timed out"` string the frontend keys -// on. This exercises the real timeout path with a short deadline against a -// loopback server that accepts the connection but never sends a response. +// caller forever. Both production `/query` builders funnel through +// `send_query_request`, which owns the per-request `.timeout(...)`; this test +// drives that exact helper against a loopback server that accepts the +// connection but never responds. It asserts two things the frontend depends +// on: (1) the helper returns instead of hanging, and (2) the failure is the +// stable `"relay unreachable: request timed out"` classified string. +// +// The outer `tokio::time::timeout` is the regression guard: if the production +// `.timeout(...)` is ever removed from `send_query_request`, this call would +// hang forever, so the guard fires and the test fails fast rather than +// stalling CI. A short 200ms deadline keeps the happy path fast. #[tokio::test] async fn stalled_query_request_times_out_with_classified_error() { use std::io::Read as _; + use std::time::Duration; // A listener that accepts the connection and then holds it open without // ever writing a response — the "headers never arrive" stall. @@ -274,23 +281,32 @@ async fn stalled_query_request_times_out_with_classified_error() { // the socket until the client aborts on its own timeout. let mut buf = [0u8; 4096]; let _ = stream.read(&mut buf); - std::thread::sleep(std::time::Duration::from_secs(2)); + std::thread::sleep(Duration::from_secs(2)); } }); let client = reqwest::Client::new(); - let err = client - .post(format!("http://{addr}/query")) - .timeout(std::time::Duration::from_millis(200)) - .body("[]") - .send() - .await - .expect_err("request must fail on the per-request timeout"); + let url = format!("http://{addr}/query"); + let result = tokio::time::timeout( + Duration::from_secs(5), + super::send_query_request( + &client, + &url, + "Nostr test-auth", + None, + b"[]".to_vec(), + Duration::from_millis(200), + ), + ) + .await + .expect( + "send_query_request must honor its per-request timeout and resolve within 5s; \ + if this guard fires, the production .timeout(...) was lost", + ); - assert!(err.is_timeout(), "stalled request must be a timeout error"); + let err = result.expect_err("a stalled /query must surface an error, not succeed"); assert_eq!( - super::classify_request_error(&err), - "relay unreachable: request timed out", + err, "relay unreachable: request timed out", "a timed-out /query must surface the stable classified string" ); diff --git a/desktop/src/features/messages/lib/threadPanel.ts b/desktop/src/features/messages/lib/threadPanel.ts index ebc1b35ae7e..6dc1190910d 100644 --- a/desktop/src/features/messages/lib/threadPanel.ts +++ b/desktop/src/features/messages/lib/threadPanel.ts @@ -542,3 +542,59 @@ export function buildThreadPanelData( expandedReplyIds, ); } + +function hasLaterVisibleSibling( + entries: readonly MainTimelineEntry[], + entryIndex: number, +): boolean { + const depth = entries[entryIndex]?.message.depth; + if (depth == null) { + return false; + } + + for (let index = entryIndex + 1; index < entries.length; index += 1) { + const nextDepth = entries[index].message.depth; + if (nextDepth <= depth) { + return nextDepth === depth; + } + } + + return false; +} + +/** + * Depths at which a vertical thread-branch guide should continue past `message` + * because an ancestor on its path still has a later visible sibling. Pure so + * the branch-guide geometry is unit-tested without the panel. + */ +export function getActiveContinuationDepths({ + ancestors, + entries, + index, + message, +}: { + ancestors: readonly { index: number; message: TimelineMessage }[]; + entries: readonly MainTimelineEntry[]; + index: number; + message: TimelineMessage; +}): number[] { + const depths: number[] = []; + + for (const ancestor of ancestors) { + if (ancestor.message.depth === 0) { + continue; + } + + const childDepth = ancestor.message.depth + 1; + const pathChild = + message.depth === childDepth + ? { index, message } + : ancestors.find((candidate) => candidate.message.depth === childDepth); + + if (pathChild && hasLaterVisibleSibling(entries, pathChild.index)) { + depths.push(ancestor.message.depth); + } + } + + return depths; +} diff --git a/desktop/src/features/messages/ui/MessageThreadPanel.tsx b/desktop/src/features/messages/ui/MessageThreadPanel.tsx index 8c495091b4f..cd91b59905b 100644 --- a/desktop/src/features/messages/ui/MessageThreadPanel.tsx +++ b/desktop/src/features/messages/ui/MessageThreadPanel.tsx @@ -4,6 +4,7 @@ import { ArrowDown } from "lucide-react"; import { HuddleTranscriptIntro } from "@/features/huddle/components/HuddleTranscriptIntro"; import { buildThreadSummaryFromVisibleEntries, + getActiveContinuationDepths, hasNestedThreadBranches, type MainTimelineEntry, } from "@/features/messages/lib/threadPanel"; @@ -143,55 +144,21 @@ type MessageThreadPanelProps = ThreadPanelLayoutProps & { const EMPTY_THREAD_REPLIES: MainTimelineEntry[] = []; const THREAD_PANEL_SUMMARY_INDENT_OFFSET_REM = 0; -function hasLaterVisibleSibling( - entries: readonly MainTimelineEntry[], - entryIndex: number, -): boolean { - const depth = entries[entryIndex]?.message.depth; - if (depth == null) { - return false; - } - - for (let index = entryIndex + 1; index < entries.length; index += 1) { - const nextDepth = entries[index].message.depth; - if (nextDepth <= depth) { - return nextDepth === depth; - } - } - - return false; -} - -function getActiveContinuationDepths({ - ancestors, - entries, - index, - message, +// The reply region's terminal (non-skeleton, non-list) surface. A terminal +// fetch "error" renders the retry card, NEVER the "empty" "No replies" state — +// that is the false-empty guard this whole change exists to hold. "pending" +// paints nothing (rows are streaming in on the deferred commit). Exported so +// the surface→card mapping is mount-tested without the panel's composer stack. +export function ThreadRepliesTerminalCard({ + surface, + onRetry, }: { - ancestors: readonly { index: number; message: TimelineMessage }[]; - entries: readonly MainTimelineEntry[]; - index: number; - message: TimelineMessage; -}): number[] { - const depths: number[] = []; - - for (const ancestor of ancestors) { - if (ancestor.message.depth === 0) { - continue; - } - - const childDepth = ancestor.message.depth + 1; - const pathChild = - message.depth === childDepth - ? { index, message } - : ancestors.find((candidate) => candidate.message.depth === childDepth); - - if (pathChild && hasLaterVisibleSibling(entries, pathChild.index)) { - depths.push(ancestor.message.depth); - } - } - - return depths; + surface: "error" | "empty" | "pending"; + onRetry?: () => void; +}) { + if (surface === "error") return ; + if (surface === "empty") return ; + return null; } export function MessageThreadPanel({ @@ -816,14 +783,12 @@ export function MessageThreadPanel({ })}
) - ) : repliesSurface === "error" ? ( - - ) : repliesSurface === "empty" ? ( - - ) : // "pending": deferred list is empty but the live list has content — - // rows are streaming in on the deferred commit. Paint nothing rather - // than flashing the empty state. - null} + ) : ( + + )}
diff --git a/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs b/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs new file mode 100644 index 00000000000..c094fd0765b --- /dev/null +++ b/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs @@ -0,0 +1,116 @@ +/** + * Mount regressions for the thread reply region terminal surface, wired + * through the panel-owned ThreadRepliesTerminalCard. + * + * Bug this pins: a terminal thread-replies fetch error used to fall through to + * the "No replies in this branch yet" empty card, silently presenting a broken + * load as an authoritative empty branch with no recovery. The fix maps a + * terminal error surface to the retry card and NEVER the empty card, and routes + * the Retry button back to the query's refetch. selectThreadRepliesSurface + * (unit tested in timelineSnapshot.test.mjs) picks the surface; this file + * proves the panel's consumer renders the right card and fires retry. + * + * Mutation proof: ThreadRepliesTerminalCard is exported from + * MessageThreadPanel.tsx, the production consumer of + * threadRepliesError/onRetryThreadReplies. Reverting the panel to its pre-fix + * state removes this export, so the import throws and every test here fails. + * Mounting the full panel is infeasible in node:test (its Tiptap composer / + * React Query stack is unavailable, see MessageComposerAutoSend.test.mjs), so + * the terminal-surface consumer is extracted to this cheap-to-mount component. + * + * CI surface: pnpm test (node:test with @testing-library/react over JSDOM). + */ + +import assert from "node:assert/strict"; +import { after, afterEach, before, test } from "node:test"; + +import { JSDOM } from "jsdom"; + +const dom = new JSDOM("", { + url: "http://localhost", +}); + +before(() => { + Object.assign(globalThis, { + document: dom.window.document, + HTMLElement: dom.window.HTMLElement, + IS_REACT_ACT_ENVIRONMENT: true, + window: dom.window, + }); + dom.window.matchMedia = () => ({ + matches: false, + addEventListener() {}, + removeEventListener() {}, + }); +}); + +afterEach(async () => { + const { cleanup } = await import("@testing-library/react"); + cleanup(); +}); + +after(() => dom.window.close()); + +async function renderCard(props) { + const { createElement } = await import("react"); + const { render } = await import("@testing-library/react"); + const { ThreadRepliesTerminalCard } = await import( + "./MessageThreadPanel.tsx" + ); + return render(createElement(ThreadRepliesTerminalCard, props)); +} + +test("terminal error renders the retry card, never the empty card", async () => { + const { screen } = await import("@testing-library/react"); + await renderCard({ surface: "error", onRetry: () => {} }); + + assert.ok( + screen.getByTestId("message-thread-replies-error"), + "a terminal error must render the error card", + ); + assert.equal( + document.body.textContent.includes("No replies in this branch yet"), + false, + "a terminal error must NEVER render the empty state", + ); +}); + +test("Retry button invokes the supplied refetch callback", async () => { + const { fireEvent, screen } = await import("@testing-library/react"); + let retryCount = 0; + await renderCard({ + surface: "error", + onRetry: () => { + retryCount += 1; + }, + }); + + fireEvent.click(screen.getByTestId("message-thread-replies-retry")); + + assert.equal(retryCount, 1, "clicking Retry must call the refetch callback"); +}); + +test("genuine empty surface renders the empty card, not the error card", async () => { + const { screen } = await import("@testing-library/react"); + await renderCard({ surface: "empty" }); + + assert.ok( + document.body.textContent.includes("No replies in this branch yet"), + "a genuine empty branch must render the empty card", + ); + assert.equal( + screen.queryByTestId("message-thread-replies-error"), + null, + "a genuine empty branch must NOT render the error card", + ); +}); + +test("pending surface paints nothing", async () => { + const { container } = await renderCard({ surface: "pending" }); + + assert.equal( + container.textContent, + "", + "the pending surface must render nothing while rows stream in", + ); +}); From 428d1f5c8f90dfa6efbb367b6369a66f84e935f6 Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 21 Aug 2026 11:17:33 -0400 Subject: [PATCH 03/10] test(desktop): bind panel wiring for thread terminal reply card MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mount tests exercised the exported ThreadRepliesTerminalCard directly but never observed the panel's call site, so the panel could drop the card for an unconditional empty state — restoring the false-empty load bug — with every test still green. Add a source tripwire that fails if the terminal branch stops rendering ThreadRepliesTerminalCard fed by both repliesSurface and onRetryThreadReplies. Full panel mount can't reach that JSX (Tiptap composer / React Query stack), so the source is the binding. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../ui/MessageThreadReplyState.test.mjs | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs b/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs index c094fd0765b..74f559ea212 100644 --- a/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs +++ b/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs @@ -18,11 +18,23 @@ * React Query stack is unavailable, see MessageComposerAutoSend.test.mjs), so * the terminal-surface consumer is extracted to this cheap-to-mount component. * + * Wiring guard: mounting the exported card proves the surface→card mapping, + * but not that the panel still ROUTES its real repliesSurface / retry callback + * through it — the panel could drop the card and render an unconditional empty + * state, restoring the false-empty bug while these mount tests stay green. The + * final test is a structural source tripwire that fails if the panel's terminal + * branch stops rendering ThreadRepliesTerminalCard fed by both + * surface={repliesSurface} and onRetry={onRetryThreadReplies}. Full panel mount + * can't observe this call site, so the source is the binding. + * * CI surface: pnpm test (node:test with @testing-library/react over JSDOM). */ import assert from "node:assert/strict"; +import fs from "node:fs"; +import path from "node:path"; import { after, afterEach, before, test } from "node:test"; +import { fileURLToPath } from "node:url"; import { JSDOM } from "jsdom"; @@ -114,3 +126,38 @@ test("pending surface paints nothing", async () => { "the pending surface must render nothing while rows stream in", ); }); + +test("panel routes its real reply surface + retry through the terminal card", () => { + // Structural tripwire: the mount tests above prove the card maps surfaces to + // the right subcards, but they never observe the panel's call site. This + // guard binds that call site — if the terminal branch stops rendering + // fed by both the live repliesSurface and the + // retry callback (e.g. reverted to an unconditional empty card), the panel + // false-empty bug is back with these tests still green. Full panel mount + // can't reach this JSX (Tiptap composer / React Query stack), so the source + // is the only place to bind it. + const panelPath = path.join( + path.dirname(fileURLToPath(import.meta.url)), + "MessageThreadPanel.tsx", + ); + const source = fs.readFileSync(panelPath, "utf8"); + + // Isolate the JSX open tag (not the `function ThreadRepliesTerminalCard` + // definition) and its prop list. + const openTag = source.match(/]*\/>/s); + assert.ok( + openTag, + "the panel's terminal reply branch must render ", + ); + const props = openTag[0].replace(/\s+/g, " "); + assert.match( + props, + /surface=\{repliesSurface\}/, + "the terminal card must be fed the live repliesSurface", + ); + assert.match( + props, + /onRetry=\{onRetryThreadReplies\}/, + "the terminal card must be fed the panel's retry callback", + ); +}); From ae56b6e181bb725c33c32c42b55fc1b7cf37b761 Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 21 Aug 2026 11:43:38 -0400 Subject: [PATCH 04/10] test(desktop): bind thread reply-region branching in a mounted unit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A source-text guard on the panel's terminal card proved the tag existed but could not prove the terminal branch reached it — wrapping the intact card in a dead conditional restored the false-empty bug while the guard stayed green. Move the surface→content dispatch out of MessageThreadPanel into an exported ThreadReplyRegion, fed the live surface, retry callback, and render callbacks for the heavy skeleton/list branches. The error≠empty decision now lives inside a cheap-to-mount unit, so the mount test exercises the real branching and an unwire drops the whole region instead of leaving a silently dead card. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../messages/ui/MessageThreadPanel.tsx | 329 +++++++++--------- .../ui/MessageThreadReplyState.test.mjs | 108 +++--- .../messages/ui/MessageThreadReplyState.tsx | 36 ++ 3 files changed, 240 insertions(+), 233 deletions(-) diff --git a/desktop/src/features/messages/ui/MessageThreadPanel.tsx b/desktop/src/features/messages/ui/MessageThreadPanel.tsx index cd91b59905b..77898683b4f 100644 --- a/desktop/src/features/messages/ui/MessageThreadPanel.tsx +++ b/desktop/src/features/messages/ui/MessageThreadPanel.tsx @@ -42,10 +42,7 @@ import { import type { ThreadDepthGuideAction } from "./MessageRow"; import { MessageThreadRow } from "./MessageThreadRow"; import { MessageThreadSummaryRow } from "./MessageThreadSummaryRow"; -import { - ThreadRepliesEmptyCard, - ThreadRepliesErrorCard, -} from "./MessageThreadReplyState"; +import { ThreadReplyRegion } from "./MessageThreadReplyState"; import { TypingIndicatorRow } from "./TypingIndicatorRow"; import { UnreadDivider } from "./UnreadDivider"; import { useComposerHeightPadding } from "./useComposerHeightPadding"; @@ -144,23 +141,6 @@ type MessageThreadPanelProps = ThreadPanelLayoutProps & { const EMPTY_THREAD_REPLIES: MainTimelineEntry[] = []; const THREAD_PANEL_SUMMARY_INDENT_OFFSET_REM = 0; -// The reply region's terminal (non-skeleton, non-list) surface. A terminal -// fetch "error" renders the retry card, NEVER the "empty" "No replies" state — -// that is the false-empty guard this whole change exists to hold. "pending" -// paints nothing (rows are streaming in on the deferred commit). Exported so -// the surface→card mapping is mount-tested without the panel's composer stack. -export function ThreadRepliesTerminalCard({ - surface, - onRetry, -}: { - surface: "error" | "empty" | "pending"; - onRetry?: () => void; -}) { - if (surface === "error") return ; - if (surface === "empty") return ; - return null; -} - export function MessageThreadPanel({ channel, channelId, @@ -617,178 +597,183 @@ export function MessageThreadPanel({ className={cn(THREAD_PANEL_MESSAGE_GUTTER_CLASS, "pb-3 pt-0")} data-testid="message-thread-replies" > - {repliesSurface === "skeleton" ? ( -
- - -
- ) : repliesSurface === "list" ? ( - visibleThreadHeadSummary ? ( + (
- + +
- ) : ( -
- {threadReplyRenderItems.map((item) => { - const { - collapseDepthGuideActions, - connectsToVisibleChild, - continuationDepths, - entry, - index, - isContinuation, - } = item; - const showUnreadDivider = - index > 0 && entry.message.id === firstUnreadReplyId; - const highlight = selectThreadRowHighlight({ - branch: highlightedBranch, - index, - messageId: entry.message.id, - messageDepth: entry.message.depth, - showGuides: shouldShowThreadBranchGuides, - }); - return ( -
- {showUnreadDivider ? : null} - + visibleThreadHeadSummary ? ( +
+ +
+ ) : ( +
+ {threadReplyRenderItems.map((item) => { + const { + collapseDepthGuideActions, + connectsToVisibleChild, + continuationDepths, + entry, + index, + isContinuation, + } = item; + const showUnreadDivider = + index > 0 && entry.message.id === firstUnreadReplyId; + const highlight = selectThreadRowHighlight({ + branch: highlightedBranch, + index, + messageId: entry.message.id, + messageDepth: entry.message.depth, + showGuides: shouldShowThreadBranchGuides, + }); + return ( +
- {entry.summary ? ( - + {showUnreadDivider ? : null} + - ) : null} -
- ); - })} -
- ) - ) : ( - - )} + {entry.summary ? ( + + ) : null} +
+ ); + })} +
+ ) + } + /> diff --git a/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs b/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs index 74f559ea212..20876e4504f 100644 --- a/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs +++ b/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs @@ -1,40 +1,31 @@ /** - * Mount regressions for the thread reply region terminal surface, wired - * through the panel-owned ThreadRepliesTerminalCard. + * Mount regressions for the thread reply region, wired through the + * panel-owned ThreadReplyRegion dispatcher. * * Bug this pins: a terminal thread-replies fetch error used to fall through to * the "No replies in this branch yet" empty card, silently presenting a broken * load as an authoritative empty branch with no recovery. The fix maps a * terminal error surface to the retry card and NEVER the empty card, and routes * the Retry button back to the query's refetch. selectThreadRepliesSurface - * (unit tested in timelineSnapshot.test.mjs) picks the surface; this file - * proves the panel's consumer renders the right card and fires retry. + * (unit tested in timelineSnapshot.test.mjs) picks the surface; this file mounts + * ThreadReplyRegion — the component that OWNS the surface→content branching — + * and drives every surface through it. * - * Mutation proof: ThreadRepliesTerminalCard is exported from - * MessageThreadPanel.tsx, the production consumer of - * threadRepliesError/onRetryThreadReplies. Reverting the panel to its pre-fix - * state removes this export, so the import throws and every test here fails. - * Mounting the full panel is infeasible in node:test (its Tiptap composer / - * React Query stack is unavailable, see MessageComposerAutoSend.test.mjs), so - * the terminal-surface consumer is extracted to this cheap-to-mount component. - * - * Wiring guard: mounting the exported card proves the surface→card mapping, - * but not that the panel still ROUTES its real repliesSurface / retry callback - * through it — the panel could drop the card and render an unconditional empty - * state, restoring the false-empty bug while these mount tests stay green. The - * final test is a structural source tripwire that fails if the panel's terminal - * branch stops rendering ThreadRepliesTerminalCard fed by both - * surface={repliesSurface} and onRetry={onRetryThreadReplies}. Full panel mount - * can't observe this call site, so the source is the binding. + * Why this component, not the panel: MessageThreadPanel delegates its whole + * reply region to ThreadReplyRegion, passing the live repliesSurface, its retry + * callback, and render callbacks for the two heavy (skeleton/list) branches. The + * error≠empty decision lives inside ThreadReplyRegion, so mounting it exercises + * the real production branching — an unwire in the panel drops the region + * entirely rather than leaving a silently-dead card. Mounting the full panel is + * infeasible in node:test (its Tiptap composer / React Query stack is + * unavailable, see MessageComposerAutoSend.test.mjs); the render callbacks keep + * that heavy construction in the panel and out of this cheap mount. * * CI surface: pnpm test (node:test with @testing-library/react over JSDOM). */ import assert from "node:assert/strict"; -import fs from "node:fs"; -import path from "node:path"; import { after, afterEach, before, test } from "node:test"; -import { fileURLToPath } from "node:url"; import { JSDOM } from "jsdom"; @@ -63,18 +54,28 @@ afterEach(async () => { after(() => dom.window.close()); -async function renderCard(props) { +// Sentinels for the two heavy branches the panel owns. If ThreadReplyRegion +// ever routes error/empty/pending through a render callback, these appear where +// a card is expected and the assertions catch it. +const SKELETON_MARK = "SKELETON_BRANCH_MARKER"; +const LIST_MARK = "LIST_BRANCH_MARKER"; + +async function renderRegion(props) { const { createElement } = await import("react"); const { render } = await import("@testing-library/react"); - const { ThreadRepliesTerminalCard } = await import( - "./MessageThreadPanel.tsx" + const { ThreadReplyRegion } = await import("./MessageThreadReplyState.tsx"); + return render( + createElement(ThreadReplyRegion, { + renderSkeleton: () => createElement("div", null, SKELETON_MARK), + renderList: () => createElement("div", null, LIST_MARK), + ...props, + }), ); - return render(createElement(ThreadRepliesTerminalCard, props)); } test("terminal error renders the retry card, never the empty card", async () => { const { screen } = await import("@testing-library/react"); - await renderCard({ surface: "error", onRetry: () => {} }); + await renderRegion({ surface: "error", onRetry: () => {} }); assert.ok( screen.getByTestId("message-thread-replies-error"), @@ -90,7 +91,7 @@ test("terminal error renders the retry card, never the empty card", async () => test("Retry button invokes the supplied refetch callback", async () => { const { fireEvent, screen } = await import("@testing-library/react"); let retryCount = 0; - await renderCard({ + await renderRegion({ surface: "error", onRetry: () => { retryCount += 1; @@ -104,7 +105,7 @@ test("Retry button invokes the supplied refetch callback", async () => { test("genuine empty surface renders the empty card, not the error card", async () => { const { screen } = await import("@testing-library/react"); - await renderCard({ surface: "empty" }); + await renderRegion({ surface: "empty" }); assert.ok( document.body.textContent.includes("No replies in this branch yet"), @@ -118,7 +119,7 @@ test("genuine empty surface renders the empty card, not the error card", async ( }); test("pending surface paints nothing", async () => { - const { container } = await renderCard({ surface: "pending" }); + const { container } = await renderRegion({ surface: "pending" }); assert.equal( container.textContent, @@ -127,37 +128,22 @@ test("pending surface paints nothing", async () => { ); }); -test("panel routes its real reply surface + retry through the terminal card", () => { - // Structural tripwire: the mount tests above prove the card maps surfaces to - // the right subcards, but they never observe the panel's call site. This - // guard binds that call site — if the terminal branch stops rendering - // fed by both the live repliesSurface and the - // retry callback (e.g. reverted to an unconditional empty card), the panel - // false-empty bug is back with these tests still green. Full panel mount - // can't reach this JSX (Tiptap composer / React Query stack), so the source - // is the only place to bind it. - const panelPath = path.join( - path.dirname(fileURLToPath(import.meta.url)), - "MessageThreadPanel.tsx", - ); - const source = fs.readFileSync(panelPath, "utf8"); +test("skeleton surface renders the panel's skeleton branch", async () => { + const { container } = await renderRegion({ surface: "skeleton" }); - // Isolate the JSX open tag (not the `function ThreadRepliesTerminalCard` - // definition) and its prop list. - const openTag = source.match(/]*\/>/s); - assert.ok( - openTag, - "the panel's terminal reply branch must render ", - ); - const props = openTag[0].replace(/\s+/g, " "); - assert.match( - props, - /surface=\{repliesSurface\}/, - "the terminal card must be fed the live repliesSurface", + assert.equal( + container.textContent, + SKELETON_MARK, + "the skeleton surface must render the panel's skeleton branch", ); - assert.match( - props, - /onRetry=\{onRetryThreadReplies\}/, - "the terminal card must be fed the panel's retry callback", +}); + +test("list surface renders the panel's list branch", async () => { + const { container } = await renderRegion({ surface: "list" }); + + assert.equal( + container.textContent, + LIST_MARK, + "the list surface must render the panel's list branch", ); }); diff --git a/desktop/src/features/messages/ui/MessageThreadReplyState.tsx b/desktop/src/features/messages/ui/MessageThreadReplyState.tsx index e52731b2e69..1af71cf7b29 100644 --- a/desktop/src/features/messages/ui/MessageThreadReplyState.tsx +++ b/desktop/src/features/messages/ui/MessageThreadReplyState.tsx @@ -1,3 +1,6 @@ +import type { ReactNode } from "react"; + +import type { ThreadRepliesSurface } from "@/features/messages/lib/timelineSnapshot"; import { Button } from "@/shared/ui/button"; /** @@ -61,3 +64,36 @@ export function ThreadRepliesEmptyCard() { ); } + +/** + * The single paint decision for the thread reply region, keyed off the surface + * `selectThreadRepliesSurface` resolved. Owning the branching here — rather than + * inline in `MessageThreadPanel` — keeps the load-bearing invariant inside a + * cheap-to-mount unit: a terminal fetch "error" renders the retry card and NEVER + * the "empty" "No replies" state (the false-empty guard this change exists to + * hold), while "pending" paints nothing (rows stream in on the deferred commit). + * + * The two heavy branches take render callbacks so the panel keeps ownership of + * its skeleton and list construction (Tiptap/React-Query bound, not mountable in + * node:test) without dragging them into this component. The mount test drives + * the real surface→content mapping through this exported unit, so an unwire in + * the panel drops the whole region — a louder regression than a silently dead + * card, and one a source scan could not prove reachable. + */ +export function ThreadReplyRegion({ + surface, + onRetry, + renderSkeleton, + renderList, +}: { + surface: ThreadRepliesSurface; + onRetry?: () => void; + renderSkeleton: () => ReactNode; + renderList: () => ReactNode; +}) { + if (surface === "skeleton") return <>{renderSkeleton()}; + if (surface === "list") return <>{renderList()}; + if (surface === "error") return ; + if (surface === "empty") return ; + return null; +} From f9419ccc502935c649c2dda25ba282d7f3641060 Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 21 Aug 2026 12:09:02 -0400 Subject: [PATCH 05/10] fix(desktop): fuse thread reply surface selection into the mounted region The panel previously computed a ThreadRepliesSurface and passed it to ThreadReplyRegion, leaving a falsifiable seam: a static/wrong surface prop at the panel boundary could silently restore the false-empty bug on every fetch failure while unit tests stayed green (they mounted the region directly and never observed the handoff). Move both the surface selection (selectThreadRepliesSurface + selectDeferredListRenderState) and the surface->content dispatch into ThreadReplyRegion, which now takes only raw query/render state (pending/error flags, deferred vs. live reply counts, huddle flag). The panel has no precomputed surface prop left to mis-set, so the seam class is gone by construction. The mount test drives raw state through the unit so both selection and dispatch are covered. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../messages/ui/MessageThreadPanel.tsx | 18 +++---- .../ui/MessageThreadReplyState.test.mjs | 48 +++++++++++-------- .../messages/ui/MessageThreadReplyState.tsx | 45 ++++++++++++----- 3 files changed, 67 insertions(+), 44 deletions(-) diff --git a/desktop/src/features/messages/ui/MessageThreadPanel.tsx b/desktop/src/features/messages/ui/MessageThreadPanel.tsx index 77898683b4f..01fbcda7ffb 100644 --- a/desktop/src/features/messages/ui/MessageThreadPanel.tsx +++ b/desktop/src/features/messages/ui/MessageThreadPanel.tsx @@ -48,10 +48,7 @@ import { UnreadDivider } from "./UnreadDivider"; import { useComposerHeightPadding } from "./useComposerHeightPadding"; import { useStableSendToChannel } from "./useStableSendToChannel"; import { useAnchoredScroll } from "./useAnchoredScroll"; -import { - selectDeferredListRenderState, - selectThreadRepliesSurface, -} from "@/features/messages/lib/timelineSnapshot"; +import { selectDeferredListRenderState } from "@/features/messages/lib/timelineSnapshot"; import { selectThreadRowHighlight } from "@/features/messages/lib/threadReplyHighlight"; type MessageThreadPanelProps = ThreadPanelLayoutProps & { @@ -317,13 +314,6 @@ export function MessageThreadPanel({ deferredThreadReplies.length, threadReplies.length, ); - // One paint decision: a terminal error never falls through to "No replies". - const repliesSurface = selectThreadRepliesSurface({ - isPending: threadRepliesPending, - isError: threadRepliesError, - renderState: repliesRenderState, - isHuddleTranscript, - }); const threadHeadSummary = React.useMemo(() => { if (!threadHeadId) { return null; @@ -598,7 +588,11 @@ export function MessageThreadPanel({ data-testid="message-thread-replies" > (
createElement("div", null, SKELETON_MARK), renderList: () => createElement("div", null, LIST_MARK), ...props, @@ -75,7 +79,8 @@ async function renderRegion(props) { test("terminal error renders the retry card, never the empty card", async () => { const { screen } = await import("@testing-library/react"); - await renderRegion({ surface: "error", onRetry: () => {} }); + // Raw terminal-failure state: not pending, load errored, nothing to show. + await renderRegion({ isError: true, onRetry: () => {} }); assert.ok( screen.getByTestId("message-thread-replies-error"), @@ -92,7 +97,7 @@ test("Retry button invokes the supplied refetch callback", async () => { const { fireEvent, screen } = await import("@testing-library/react"); let retryCount = 0; await renderRegion({ - surface: "error", + isError: true, onRetry: () => { retryCount += 1; }, @@ -105,7 +110,8 @@ test("Retry button invokes the supplied refetch callback", async () => { test("genuine empty surface renders the empty card, not the error card", async () => { const { screen } = await import("@testing-library/react"); - await renderRegion({ surface: "empty" }); + // Load succeeded (no error), branch is genuinely empty. + await renderRegion({}); assert.ok( document.body.textContent.includes("No replies in this branch yet"), @@ -119,7 +125,9 @@ test("genuine empty surface renders the empty card, not the error card", async ( }); test("pending surface paints nothing", async () => { - const { container } = await renderRegion({ surface: "pending" }); + // Deferred snapshot is empty but the live list has content: rows are + // streaming in on the deferred commit, so paint nothing yet. + const { container } = await renderRegion({ deferredCount: 0, liveCount: 1 }); assert.equal( container.textContent, @@ -129,7 +137,7 @@ test("pending surface paints nothing", async () => { }); test("skeleton surface renders the panel's skeleton branch", async () => { - const { container } = await renderRegion({ surface: "skeleton" }); + const { container } = await renderRegion({ isPending: true }); assert.equal( container.textContent, @@ -139,7 +147,7 @@ test("skeleton surface renders the panel's skeleton branch", async () => { }); test("list surface renders the panel's list branch", async () => { - const { container } = await renderRegion({ surface: "list" }); + const { container } = await renderRegion({ deferredCount: 1, liveCount: 1 }); assert.equal( container.textContent, diff --git a/desktop/src/features/messages/ui/MessageThreadReplyState.tsx b/desktop/src/features/messages/ui/MessageThreadReplyState.tsx index 1af71cf7b29..d7d21aae5df 100644 --- a/desktop/src/features/messages/ui/MessageThreadReplyState.tsx +++ b/desktop/src/features/messages/ui/MessageThreadReplyState.tsx @@ -1,6 +1,9 @@ import type { ReactNode } from "react"; -import type { ThreadRepliesSurface } from "@/features/messages/lib/timelineSnapshot"; +import { + selectDeferredListRenderState, + selectThreadRepliesSurface, +} from "@/features/messages/lib/timelineSnapshot"; import { Button } from "@/shared/ui/button"; /** @@ -66,31 +69,49 @@ export function ThreadRepliesEmptyCard() { } /** - * The single paint decision for the thread reply region, keyed off the surface - * `selectThreadRepliesSurface` resolved. Owning the branching here — rather than - * inline in `MessageThreadPanel` — keeps the load-bearing invariant inside a - * cheap-to-mount unit: a terminal fetch "error" renders the retry card and NEVER - * the "empty" "No replies" state (the false-empty guard this change exists to - * hold), while "pending" paints nothing (rows stream in on the deferred commit). + * The single paint decision for the thread reply region. This unit owns BOTH + * the surface selection (`selectThreadRepliesSurface`, keyed off the same raw + * query/render state the panel already holds) AND the surface→content dispatch. + * Fusing them here removes the last falsifiable seam: the panel passes only its + * raw state — pending/error flags, the deferred vs. live reply counts, and the + * huddle-transcript flag — so there is no precomputed `surface` prop at the + * panel boundary to statically mis-set (e.g. a stray `surface="empty"` that would + * silently restore the false-empty bug on every fetch failure). The load-bearing + * invariant holds by construction: a terminal fetch "error" renders the retry + * card and NEVER the "empty" "No replies" state, while "pending" paints nothing + * (rows stream in on the deferred commit). * * The two heavy branches take render callbacks so the panel keeps ownership of * its skeleton and list construction (Tiptap/React-Query bound, not mountable in * node:test) without dragging them into this component. The mount test drives - * the real surface→content mapping through this exported unit, so an unwire in - * the panel drops the whole region — a louder regression than a silently dead - * card, and one a source scan could not prove reachable. + * the real raw-state→surface→content mapping through this exported unit, so both + * the selection and the dispatch are covered under a cheap mount. */ export function ThreadReplyRegion({ - surface, + isPending, + isError, + deferredCount, + liveCount, + isHuddleTranscript = false, onRetry, renderSkeleton, renderList, }: { - surface: ThreadRepliesSurface; + isPending: boolean; + isError: boolean; + deferredCount: number; + liveCount: number; + isHuddleTranscript?: boolean; onRetry?: () => void; renderSkeleton: () => ReactNode; renderList: () => ReactNode; }) { + const surface = selectThreadRepliesSurface({ + isPending, + isError, + renderState: selectDeferredListRenderState(deferredCount, liveCount), + isHuddleTranscript, + }); if (surface === "skeleton") return <>{renderSkeleton()}; if (surface === "list") return <>{renderList()}; if (surface === "error") return ; From 9d710250ab1c919ba84c8f76c2f2ec9f401e80ba Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 21 Aug 2026 12:57:21 -0400 Subject: [PATCH 06/10] test(desktop): add E2E guard for false-empty thread-load bug Unit tests mount ThreadReplyRegion in isolation and cannot mount the full panel (Tiptap/React-Query hang the runner), so they never observe the production panel->region handoff. A one-token call-site edit (isError={false}) could ship the original false-empty regression with all unit armor green. This smoke E2E drives the real panel wiring through the mock bridge: it forces a terminal get_thread_replies failure at the IPC boundary, opens the thread, and asserts the error/Retry card renders and never the false-empty empty card, then recovers on Retry. Immune to source restructuring because it observes what users see. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/playwright.config.ts | 1 + desktop/tests/e2e/thread-load-failure.spec.ts | 161 ++++++++++++++++++ 2 files changed, 162 insertions(+) create mode 100644 desktop/tests/e2e/thread-load-failure.spec.ts diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 9099beff69e..c1f3bbda303 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -86,6 +86,7 @@ export default defineConfig({ "**/home-collapsed-top-chrome.spec.ts", "**/top-chrome-zoom-clearance.spec.ts", "**/thread-unread.spec.ts", + "**/thread-load-failure.spec.ts", "**/workspace-rail.spec.ts", "**/community-rail.spec.ts", "**/boot-splash.spec.ts", diff --git a/desktop/tests/e2e/thread-load-failure.spec.ts b/desktop/tests/e2e/thread-load-failure.spec.ts new file mode 100644 index 00000000000..291600e736b --- /dev/null +++ b/desktop/tests/e2e/thread-load-failure.spec.ts @@ -0,0 +1,161 @@ +import { expect, test, type Page } from "@playwright/test"; + +import { TEST_IDENTITIES, installMockBridge } from "../helpers/bridge"; + +/** + * End-to-end guard for the false-empty thread-load bug (PR #6447). A terminal + * thread-replies fetch failure must paint the error/Retry card and NEVER the + * "No replies in this branch yet" empty card — the two look similar but a failed + * load presented as an authoritative empty is the user-visible defect. + * + * Unit tests cover `ThreadReplyRegion` in isolation, but they cannot mount the + * full panel (Tiptap/React-Query), so they never observe the production + * panel→region handoff. This spec drives the REAL panel wiring through the mock + * bridge, so any regression at that seam (e.g. a static `isError={false}` at the + * call site) ships a visible false-empty and turns this case red. + */ + +// Fail every get_thread_replies fetch at the IPC boundary while the flag is on, +// then let the real mock handler answer once it is cleared. Wrapping +// __TAURI_INTERNALS__.invoke (installed by the mock bridge) exercises the whole +// thread-load path — query hook, retry, panel, region — exactly as production +// does, with no source seam to bypass. A boolean gate (rather than a failure +// countdown) is deterministic: stray thread-reply prefetches during channel +// open can't drain it, so the panel's own load reliably reaches the terminal +// error state, and clearing the flag makes the Retry fetch reliably succeed. +async function installThreadFailureSwitch(page: Page) { + await page.evaluate(() => { + const w = window as typeof window & { + __FAIL_THREAD_REPLIES__?: boolean; + __TAURI_INTERNALS__: { + invoke: ( + command: string, + payload: unknown, + options: unknown, + ) => Promise; + }; + }; + w.__FAIL_THREAD_REPLIES__ = false; + const original = w.__TAURI_INTERNALS__.invoke.bind(w.__TAURI_INTERNALS__); + w.__TAURI_INTERNALS__.invoke = async (command, payload, options) => { + if (command === "get_thread_replies" && w.__FAIL_THREAD_REPLIES__) { + throw new Error("relay unreachable: request timed out"); + } + return original(command, payload, options); + }; + }); +} + +async function setThreadRepliesFailing(page: Page, failing: boolean) { + await page.evaluate((failing) => { + ( + window as typeof window & { __FAIL_THREAD_REPLIES__?: boolean } + ).__FAIL_THREAD_REPLIES__ = failing; + }, failing); +} + +// Open the welcome thread deterministically: prefer its summary row, but fall +// back to hovering the root message and clicking Reply. The summary row depends +// on the channel-window query having materialized the seeded reply, which can +// lag; the root Reply affordance opens the same thread panel without that race, +// so the panel is reliably open before the error-card assertions run. +async function openWelcomeThread(page: Page) { + const summary = page.locator( + '[data-testid="message-thread-summary"][data-thread-head-id="mock-general-welcome"]', + ); + if (await summary.count()) { + await summary.first().click(); + } else { + const root = page.locator( + '[data-testid="message-row"][data-message-id="mock-general-welcome"]', + ); + await root.hover(); + await root.getByRole("button", { name: "Reply" }).click(); + } + await expect(page.getByTestId("message-thread-panel")).toBeVisible(); +} + +test.describe("thread load failure", () => { + test("terminal fetch failure shows error card, never false-empty; Retry recovers", async ({ + page, + }) => { + await installMockBridge(page); + await page.goto("/"); + + // Seed a reply into the mock store BEFORE opening general, i.e. before its + // live subscription exists. The reply lands in the channel window (so the + // "1 reply" thread summary renders) but is never live-pushed into the + // thread-replies cache — so the thread opens with an empty reply cache and + // the failed get_thread_replies has nothing to fall back to. If it were + // emitted while general was open, the live handler would seed the thread + // cache and the panel would render the list branch, masking the error card. + // + // Wait for the emitter to be installed first: emitting before the app has + // booted is a silent no-op (the helper is undefined), which would leave the + // store empty and the recovery assertion with nothing to render. + await expect + .poll(() => + page.evaluate( + () => + typeof ( + window as typeof window & { + __BUZZ_E2E_EMIT_MOCK_MESSAGE__?: unknown; + } + ).__BUZZ_E2E_EMIT_MOCK_MESSAGE__ === "function", + ), + ) + .toBe(true); + await page.evaluate((pubkey) => { + ( + window as typeof window & { + __BUZZ_E2E_EMIT_MOCK_MESSAGE__?: (input: { + channelName: string; + content: string; + parentEventId?: string; + pubkey?: string; + createdAt?: number; + }) => unknown; + } + ).__BUZZ_E2E_EMIT_MOCK_MESSAGE__?.({ + channelName: "general", + content: "First reply to welcome", + parentEventId: "mock-general-welcome", + pubkey, + createdAt: Math.floor(Date.now() / 1000) - 10, + }); + }, TEST_IDENTITIES.alice.pubkey); + + await page.getByTestId("channel-general").click(); + await expect(page.getByTestId("chat-title")).toHaveText("general"); + await installThreadFailureSwitch(page); + + // Fail every thread-replies fetch, then open the thread: the panel query and + // its retry both fail, driving the terminal error state. + await setThreadRepliesFailing(page, true); + await openWelcomeThread(page); + + // The load-bearing assertion: a terminal failure paints the error/Retry + // card and NEVER the false-empty "No replies in this branch yet" state. + await expect( + page.getByTestId("message-thread-replies-error"), + ).toContainText("Couldn't load replies", { timeout: 15_000 }); + await expect(page.getByTestId("message-thread-replies-retry")).toHaveText( + "Retry", + ); + await expect(page.getByText("No replies in this branch yet")).toHaveCount( + 0, + ); + + // Retry with fetches succeeding again: the reply loads and renders — the + // error card is gone and no false-empty appears. + await setThreadRepliesFailing(page, false); + await page.getByTestId("message-thread-replies-retry").click(); + await expect(page.getByTestId("message-thread-replies-error")).toHaveCount( + 0, + ); + await expect(page.getByText("First reply to welcome")).toBeVisible(); + await expect(page.getByText("No replies in this branch yet")).toHaveCount( + 0, + ); + }); +}); From 6f852dd96ab4b92519c98e1e8e9d46733d8e9cd2 Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 21 Aug 2026 14:26:49 -0400 Subject: [PATCH 07/10] fix(desktop): close thread-load false-empty on Projects surface, timeout label, and a11y Address three review findings on the thread-load reliability fix: - ProjectConversationPanel, a second producer of the shared thread panel, hard-coded threadRepliesPending={false} and passed no error/retry, so a terminal /query failure in a Projects conversation still painted "No replies in this branch yet" with no recovery. Propagate isPending/ isError/refetch; add an E2E regression that drives the real Projects wiring through failure -> error card -> Retry -> recovery. - parse_json_response routed a body-consumption timeout (send() resolves on headers, body stalls past the deadline) into the malformed-response bucket, discarding is_timeout(). Route timeouts through classify_request_error for the stable "relay unreachable: request timed out" label; add a loopback regression that stalls the body after valid 2xx headers. - ThreadRepliesErrorCard appeared asynchronously with no live-region semantics; add role="alert" and assert it in the mounted test. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/playwright.config.ts | 1 + desktop/src-tauri/src/relay.rs | 17 +- desktop/src-tauri/src/relay/tests.rs | 61 ++++++ .../ui/MessageThreadReplyState.test.mjs | 11 +- .../messages/ui/MessageThreadReplyState.tsx | 7 + .../projects/ui/ProjectConversationPanel.tsx | 6 +- .../project-conversation-load-failure.spec.ts | 192 ++++++++++++++++++ 7 files changed, 287 insertions(+), 8 deletions(-) create mode 100644 desktop/tests/e2e/project-conversation-load-failure.spec.ts diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index c1f3bbda303..351eff01c8d 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -87,6 +87,7 @@ export default defineConfig({ "**/top-chrome-zoom-clearance.spec.ts", "**/thread-unread.spec.ts", "**/thread-load-failure.spec.ts", + "**/project-conversation-load-failure.spec.ts", "**/workspace-rail.spec.ts", "**/community-rail.spec.ts", "**/boot-splash.spec.ts", diff --git a/desktop/src-tauri/src/relay.rs b/desktop/src-tauri/src/relay.rs index adf401d8dca..d4ba013e5fb 100644 --- a/desktop/src-tauri/src/relay.rs +++ b/desktop/src-tauri/src/relay.rs @@ -243,10 +243,19 @@ pub(crate) async fn parse_json_response( // "relay unreachable:" bucket so it surfaces loudly instead of being treated // as a transient unreachable-relay condition. The reqwest error detail is // dropped because it contains the raw URL. - response - .json::() - .await - .map_err(|_| MALFORMED_RESPONSE_MESSAGE.to_string()) + // + // A body-consumption timeout is the exception: `send()` resolves once headers + // arrive, so a body that stalls past the request deadline trips the timeout + // HERE rather than at send(). That is a connectivity failure, not a malformed + // body, so route it through the same classifier as a pre-header stall to + // preserve the stable "relay unreachable: request timed out" label. + response.json::().await.map_err(|e| { + if e.is_timeout() { + classify_request_error(&e) + } else { + MALFORMED_RESPONSE_MESSAGE.to_string() + } + }) } /// Extract the `retry in Ns` hint from a rate-limit error string. diff --git a/desktop/src-tauri/src/relay/tests.rs b/desktop/src-tauri/src/relay/tests.rs index 6a46fc1ebce..a99f92d11ba 100644 --- a/desktop/src-tauri/src/relay/tests.rs +++ b/desktop/src-tauri/src/relay/tests.rs @@ -313,6 +313,67 @@ async fn stalled_query_request_times_out_with_classified_error() { let _ = handle.join(); } +// ── /query body-stall timeout → classified error (not malformed) ───────── +// +// `send()` resolves once response headers arrive, so a relay that returns a +// valid 2xx JSON header block and then stalls the body trips the request +// deadline inside `response.json()` — the branch the pre-header stall above +// cannot reach. That is a connectivity failure, not a malformed body, so it +// must surface the stable "relay unreachable: request timed out" string rather +// than the malformed-response bucket. This drives `send_query_request` against +// a loopback that writes headers promising a body it never sends. +#[tokio::test] +async fn stalled_response_body_times_out_with_classified_error() { + use std::io::{Read as _, Write as _}; + use std::time::Duration; + + // Accept, drain the request, write a complete 2xx JSON header block that + // promises a body (Content-Length), then send nothing and hold the socket + // — the "headers arrive, body stalls" half-open case. + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + let handle = std::thread::spawn(move || { + if let Ok((mut stream, _)) = listener.accept() { + let mut buf = [0u8; 4096]; + let _ = stream.read(&mut buf); + let _ = stream.write_all( + b"HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: 64\r\n\r\n", + ); + let _ = stream.flush(); + // Never write the promised body; hold past the client deadline. + std::thread::sleep(Duration::from_secs(2)); + } + }); + + let client = reqwest::Client::new(); + let url = format!("http://{addr}/query"); + let result = tokio::time::timeout( + Duration::from_secs(5), + super::send_query_request( + &client, + &url, + "Nostr test-auth", + None, + b"[]".to_vec(), + Duration::from_millis(200), + ), + ) + .await + .expect( + "send_query_request must honor its per-request timeout through body \ + consumption and resolve within 5s", + ); + + let err = result.expect_err("a stalled response body must surface an error, not succeed"); + assert_eq!( + err, "relay unreachable: request timed out", + "a body-stall timeout must surface the classified timeout string, not the \ + malformed-response bucket" + ); + + let _ = handle.join(); +} + // ── parse_json_response malformed-body contract ────────────────────────── #[test] diff --git a/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs b/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs index 58e05f6bdee..f70143f87b1 100644 --- a/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs +++ b/desktop/src/features/messages/ui/MessageThreadReplyState.test.mjs @@ -82,9 +82,14 @@ test("terminal error renders the retry card, never the empty card", async () => // Raw terminal-failure state: not pending, load errored, nothing to show. await renderRegion({ isError: true, onRetry: () => {} }); - assert.ok( - screen.getByTestId("message-thread-replies-error"), - "a terminal error must render the error card", + const card = screen.getByTestId("message-thread-replies-error"); + assert.ok(card, "a terminal error must render the error card"); + // The card appears asynchronously (after the query/retry lifecycle), so it + // must be an alert live region or a screen-reader user never hears it. + assert.equal( + card.getAttribute("role"), + "alert", + "the async error card must be an alert live region for assistive tech", ); assert.equal( document.body.textContent.includes("No replies in this branch yet"), diff --git a/desktop/src/features/messages/ui/MessageThreadReplyState.tsx b/desktop/src/features/messages/ui/MessageThreadReplyState.tsx index d7d21aae5df..a13d46286ef 100644 --- a/desktop/src/features/messages/ui/MessageThreadReplyState.tsx +++ b/desktop/src/features/messages/ui/MessageThreadReplyState.tsx @@ -20,12 +20,19 @@ import { Button } from "@/shared/ui/button"; * silently presents a broken fetch as an authoritative "no replies" and offers * no recovery. Any cached replies still render via the panel's "list" branch, so * this only surfaces when the failed load left nothing to show. + * + * `role="alert"` (implicit `aria-live="assertive"`, `aria-atomic="true"`) makes + * the asynchronous failure audible to assistive tech: the card appears only after + * the query/retry lifecycle reaches a terminal error, so without a live region a + * screen-reader user parked in the composer never learns the load failed or that + * Retry became available. */ export function ThreadRepliesErrorCard({ onRetry }: { onRetry?: () => void }) { return (

Couldn't load replies diff --git a/desktop/src/features/projects/ui/ProjectConversationPanel.tsx b/desktop/src/features/projects/ui/ProjectConversationPanel.tsx index 59f3264b025..11344ea574d 100644 --- a/desktop/src/features/projects/ui/ProjectConversationPanel.tsx +++ b/desktop/src/features/projects/ui/ProjectConversationPanel.tsx @@ -279,7 +279,11 @@ export function ProjectConversationPanel({ scrollTargetId={scrollTargetId} threadHead={panelData.threadHead} threadReplies={panelData.visibleReplies} - threadRepliesPending={false} + threadRepliesPending={threadRepliesQuery.isPending} + threadRepliesError={threadRepliesQuery.isError} + onRetryThreadReplies={() => { + void threadRepliesQuery.refetch(); + }} threadTypingPubkeys={[]} widthPx={widthPx} />, diff --git a/desktop/tests/e2e/project-conversation-load-failure.spec.ts b/desktop/tests/e2e/project-conversation-load-failure.spec.ts new file mode 100644 index 00000000000..d76bb709ec3 --- /dev/null +++ b/desktop/tests/e2e/project-conversation-load-failure.spec.ts @@ -0,0 +1,192 @@ +import { expect, test, type Page } from "@playwright/test"; + +import { installMockBridge, TEST_IDENTITIES } from "../helpers/bridge"; + +/** + * End-to-end guard for the false-empty thread-load bug on the SECOND producer + * of the shared thread panel: the Projects "channel conversation" panel + * (`ProjectConversationPanel`). PR #6447 wired the query failure state through + * `ChannelScreen`, but the Projects surface calls the same `useThreadReplies` + * and used to hard-code `threadRepliesPending={false}` with no error/retry — so + * a terminal `/query` failure there painted "No replies in this branch yet" + * with no recovery, the exact defect the PR fixed one surface over. + * + * This drives the REAL Projects panel wiring through the mock bridge: open the + * project's Channels tab, open a conversation, fail every `get_thread_replies` + * fetch at the IPC boundary, and assert the error/Retry card renders (never the + * false-empty). Any regression at the Projects call site (e.g. dropping + * `threadRepliesError` again, or a static `isError={false}`) ships a visible + * false-empty and turns this case red. + */ + +const DEFAULT_MOCK_PUBKEY = "deadbeef".repeat(8); +const ROOT_CONTENT = `Projects conversation root ${DEFAULT_MOCK_PUBKEY} buzz`; +const REPLY_CONTENT = "Projects conversation reply body"; + +// The projects surface is a preview feature — opt in before the app mounts. +async function enableProjectsFeature(page: Page) { + await page.addInitScript(() => { + window.localStorage.setItem( + "buzz-feature-overrides-v1", + JSON.stringify({ projects: true }), + ); + }); +} + +// Fail every get_thread_replies fetch at the IPC boundary while the flag is on, +// then let the real mock handler answer once it is cleared. Wrapping +// __TAURI_INTERNALS__.invoke exercises the whole Projects thread-load path — +// the panel's useThreadReplies query, its retry, and the shared region — with +// no source seam to bypass. +async function installThreadFailureSwitch(page: Page) { + await page.evaluate(() => { + const w = window as typeof window & { + __FAIL_THREAD_REPLIES__?: boolean; + __TAURI_INTERNALS__: { + invoke: ( + command: string, + payload: unknown, + options: unknown, + ) => Promise; + }; + }; + w.__FAIL_THREAD_REPLIES__ = false; + const original = w.__TAURI_INTERNALS__.invoke.bind(w.__TAURI_INTERNALS__); + w.__TAURI_INTERNALS__.invoke = async (command, payload, options) => { + if (command === "get_thread_replies" && w.__FAIL_THREAD_REPLIES__) { + throw new Error("relay unreachable: request timed out"); + } + return original(command, payload, options); + }; + }); +} + +async function setThreadRepliesFailing(page: Page, failing: boolean) { + await page.evaluate((failing) => { + ( + window as typeof window & { __FAIL_THREAD_REPLIES__?: boolean } + ).__FAIL_THREAD_REPLIES__ = failing; + }, failing); +} + +test.describe("project conversation load failure", () => { + test("terminal fetch failure shows error card, never false-empty; Retry recovers", async ({ + page, + }) => { + await enableProjectsFeature(page); + await installMockBridge(page); + await page.goto("/", { waitUntil: "domcontentloaded" }); + + // Seed the conversation BEFORE opening general — i.e. before its live + // subscription exists — so the reply lands in the mock store (searchable, + // and returnable by a successful get_thread_replies) but is never live + // pushed into the thread-replies cache. If general were open first, the + // live handler would seed that cache and the panel would render the list + // branch, masking the error card. The root carries the repository discovery + // token so it surfaces as the channel's latest discussion hit (what the + // Channels-tab row opens); its reply omits the token so it never competes + // to be the opened hit. + // + // Wait for the emitter first: emitting before the app boots is a silent + // no-op (the helper is undefined), which would leave the store empty. + await expect + .poll(() => + page.evaluate( + () => + typeof ( + window as typeof window & { + __BUZZ_E2E_EMIT_MOCK_MESSAGE__?: unknown; + } + ).__BUZZ_E2E_EMIT_MOCK_MESSAGE__ === "function", + ), + ) + .toBe(true); + const rootId = await page.evaluate( + ({ author, rootContent, replyContent }) => { + const now = Math.floor(Date.now() / 1000); + const emit = ( + window as typeof window & { + __BUZZ_E2E_EMIT_MOCK_MESSAGE__: (input: { + channelName: string; + content: string; + parentEventId?: string; + pubkey?: string; + createdAt?: number; + }) => { id: string }; + } + ).__BUZZ_E2E_EMIT_MOCK_MESSAGE__; + const root = emit({ + channelName: "general", + content: rootContent, + pubkey: author, + createdAt: now, + }); + emit({ + channelName: "general", + content: replyContent, + parentEventId: root.id, + pubkey: author, + createdAt: now + 1, + }); + return root.id; + }, + { + author: TEST_IDENTITIES.alice.pubkey, + rootContent: ROOT_CONTENT, + replyContent: REPLY_CONTENT, + }, + ); + expect(rootId).toBeTruthy(); + + // Navigate to the project's Channels tab and open the general conversation. + await page.getByTestId("open-projects-view").click(); + await page.getByTestId("projects-section-projects").click(); + const projectEntry = page + .locator( + '[data-testid="project-card-buzz"], [data-testid="project-row-buzz"]', + ) + .first(); + await expect(projectEntry).toBeVisible({ timeout: 10_000 }); + await projectEntry.click(); + await page.getByRole("tab", { name: "Channels", exact: true }).click(); + const channelRow = page + .getByTestId("project-channel-row") + .filter({ hasText: "#general" }) + .first(); + await expect(channelRow).toBeVisible({ timeout: 10_000 }); + + // Fail every thread-replies fetch, THEN open the conversation: the panel's + // query and its retry both fail, driving the terminal error state with an + // empty reply cache and nothing to fall back to. + await installThreadFailureSwitch(page); + await setThreadRepliesFailing(page, true); + await channelRow.click(); + + const panel = page.getByTestId("project-conversation-panel"); + await expect(panel).toBeVisible(); + + // The load-bearing assertion: a terminal failure paints the error/Retry + // card and NEVER the false-empty "No replies in this branch yet" state. + await expect( + page.getByTestId("message-thread-replies-error"), + ).toContainText("Couldn't load replies", { timeout: 15_000 }); + await expect(page.getByTestId("message-thread-replies-retry")).toHaveText( + "Retry", + ); + await expect(page.getByText("No replies in this branch yet")).toHaveCount( + 0, + ); + + // Retry with fetches succeeding: the reply loads and renders — the error + // card is gone and no false-empty appears. + await setThreadRepliesFailing(page, false); + await page.getByTestId("message-thread-replies-retry").click(); + await expect(page.getByTestId("message-thread-replies-error")).toHaveCount( + 0, + ); + await expect(page.getByText(REPLY_CONTENT)).toBeVisible(); + await expect(page.getByText("No replies in this branch yet")).toHaveCount( + 0, + ); + }); +}); From 880a42091750d60f1de0cc1c7021696441655d07 Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 21 Aug 2026 17:54:31 -0400 Subject: [PATCH 08/10] fix(desktop): preserve timeout classification on stalled error-response bodies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A non-success relay response (500/429) routes through relay_error_message, which consumed the body with response.text().await.unwrap_or_default() — silently discarding a body-consumption timeout and surfacing a bare status label instead of the stable "relay unreachable: request timed out" classification the frontend connectivity classifier keys on. Only the 2xx path preserved is_timeout(). Extract that decision into a shared classify_body_timeout helper both body-consuming paths route through so they cannot drift, and preserve the timeout on the error-body path. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/relay.rs | 49 +++++++++--- desktop/src-tauri/src/relay/tests.rs | 114 +++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 11 deletions(-) diff --git a/desktop/src-tauri/src/relay.rs b/desktop/src-tauri/src/relay.rs index d4ba013e5fb..f408ef2afda 100644 --- a/desktop/src-tauri/src/relay.rs +++ b/desktop/src-tauri/src/relay.rs @@ -180,6 +180,22 @@ pub(crate) fn classify_request_error(e: &reqwest::Error) -> String { } } +/// Preserve a body-consumption timeout as the stable connectivity classification. +/// +/// `send()` resolves once response headers arrive, so a body that stalls past +/// the request deadline trips the timeout during body consumption rather than +/// at `send()`. That is a connectivity failure, not a malformed body or a plain +/// status error. Both body-consumption paths — the 2xx `parse_json_response` +/// and the non-2xx `relay_error_message` — route their consumption error +/// through this one helper so a stalled body can never be classified as +/// "request timed out" on one path while the other buries it under a malformed +/// or status label. Returns `Some("relay unreachable: request timed out")` for +/// a timeout; `None` otherwise, leaving the caller to apply its own non-timeout +/// label. +fn classify_body_timeout(e: &reqwest::Error) -> Option { + e.is_timeout().then(|| classify_request_error(e)) +} + /// Detect responses that were intercepted by a captive portal or auth proxy. /// /// Returns `Some(msg)` when the response clearly did not come from the relay: @@ -244,17 +260,14 @@ pub(crate) async fn parse_json_response( // as a transient unreachable-relay condition. The reqwest error detail is // dropped because it contains the raw URL. // - // A body-consumption timeout is the exception: `send()` resolves once headers - // arrive, so a body that stalls past the request deadline trips the timeout - // HERE rather than at send(). That is a connectivity failure, not a malformed - // body, so route it through the same classifier as a pre-header stall to - // preserve the stable "relay unreachable: request timed out" label. + // A body-consumption timeout is the exception: `send()` resolves once + // headers arrive, so a body that stalls past the request deadline trips the + // timeout HERE rather than at send(). That is a connectivity failure, not a + // malformed body, so route it through `classify_body_timeout` — the same + // helper the non-2xx error-body path uses — to preserve the stable + // "relay unreachable: request timed out" label. response.json::().await.map_err(|e| { - if e.is_timeout() { - classify_request_error(&e) - } else { - MALFORMED_RESPONSE_MESSAGE.to_string() - } + classify_body_timeout(&e).unwrap_or_else(|| MALFORMED_RESPONSE_MESSAGE.to_string()) }) } @@ -286,7 +299,21 @@ pub async fn relay_error_message(response: reqwest::Response) -> String { } // Real relay error: extract the structured message field if available. - let body = response.text().await.unwrap_or_default(); + // `text()` consumes the body, which — like the 2xx path — can trip the + // request deadline if the relay sends status headers then stalls the body. + // Preserve that timeout as the stable connectivity classification via the + // shared helper instead of letting `unwrap_or_default` swallow it into a + // bare status label. A non-timeout body error still degrades to an empty + // body → status-only message, exactly as before. + let body = match response.text().await { + Ok(body) => body, + Err(e) => { + if let Some(timeout) = classify_body_timeout(&e) { + return timeout; + } + String::new() + } + }; // 429 Too Many Requests → typed `relay rate-limited:` prefix so the TS // client can activate the rate-limit gate without confusing it with a diff --git a/desktop/src-tauri/src/relay/tests.rs b/desktop/src-tauri/src/relay/tests.rs index a99f92d11ba..4ae39249328 100644 --- a/desktop/src-tauri/src/relay/tests.rs +++ b/desktop/src-tauri/src/relay/tests.rs @@ -374,6 +374,120 @@ async fn stalled_response_body_times_out_with_classified_error() { let _ = handle.join(); } +// ── /query non-2xx body-stall timeout → classified error (not status) ──── +// +// The 2xx path is not the only body-consuming path. A relay that returns a +// non-success status (500, 429, …) routes through `relay_error_message`, which +// consumes the body via `text()` to extract the structured error field. If the +// relay sends the status headers and then stalls the promised body, that +// consumption trips the same request deadline — and it must surface the stable +// "relay unreachable: request timed out" classification, not a bare +// "relay returned 500" that hides the connectivity failure. This drives +// `send_query_request` against a loopback that writes 500 headers promising a +// body it never sends. +#[tokio::test] +async fn stalled_error_response_body_times_out_with_classified_error() { + use std::io::{Read as _, Write as _}; + use std::time::Duration; + + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + let handle = std::thread::spawn(move || { + if let Ok((mut stream, _)) = listener.accept() { + let mut buf = [0u8; 4096]; + let _ = stream.read(&mut buf); + // 500 status headers promising a body (Content-Length) that never + // arrives — the "error headers arrive, body stalls" half-open case. + let _ = stream.write_all( + b"HTTP/1.1 500 Internal Server Error\r\nContent-Type: application/json\r\nContent-Length: 64\r\n\r\n", + ); + let _ = stream.flush(); + std::thread::sleep(Duration::from_secs(2)); + } + }); + + let client = reqwest::Client::new(); + let url = format!("http://{addr}/query"); + let result = tokio::time::timeout( + Duration::from_secs(5), + super::send_query_request( + &client, + &url, + "Nostr test-auth", + None, + b"[]".to_vec(), + Duration::from_millis(200), + ), + ) + .await + .expect( + "send_query_request must honor its per-request timeout through error-body \ + consumption and resolve within 5s", + ); + + let err = result.expect_err("a stalled error-response body must surface an error, not succeed"); + assert_eq!( + err, "relay unreachable: request timed out", + "a non-2xx body-stall timeout must surface the classified timeout string, not the \ + status bucket" + ); + + let _ = handle.join(); +} + +// ── /query non-stalled 500 → status message (timeout preservation is scoped) ─ +// +// The timeout preservation above must not swallow genuine relay errors: a 500 +// whose body arrives promptly still surfaces as "relay returned 500". This +// pins that `classify_body_timeout` only fires on an actual timeout, so the +// error-classification path stays intact for live relay failures. +#[tokio::test] +async fn non_stalled_error_response_yields_status_message() { + use std::io::{Read as _, Write as _}; + use std::time::Duration; + + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + let handle = std::thread::spawn(move || { + if let Ok((mut stream, _)) = listener.accept() { + let mut buf = [0u8; 4096]; + let _ = stream.read(&mut buf); + // A complete 500 with a non-JSON body delivered immediately. + let body = "internal error"; + let response = format!( + "HTTP/1.1 500 Internal Server Error\r\nContent-Type: text/plain\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ); + let _ = stream.write_all(response.as_bytes()); + let _ = stream.flush(); + } + }); + + let client = reqwest::Client::new(); + let url = format!("http://{addr}/query"); + let result = tokio::time::timeout( + Duration::from_secs(5), + super::send_query_request( + &client, + &url, + "Nostr test-auth", + None, + b"[]".to_vec(), + Duration::from_millis(200), + ), + ) + .await + .expect("a promptly-served 500 must resolve well within 5s"); + + let err = result.expect_err("a 500 must surface an error, not succeed"); + assert_eq!( + err, "relay returned 500 Internal Server Error", + "a non-stalled 500 must keep its status classification, not be reclassified as a timeout" + ); + + let _ = handle.join(); +} + // ── parse_json_response malformed-body contract ────────────────────────── #[test] From 42f83d0694b7324199648c1cbe2aaac1e99ea22a Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 21 Aug 2026 18:21:34 -0400 Subject: [PATCH 09/10] fix(desktop): surface aggregate thread-reply load failures, not false-empty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit useThreadRepliesForRoots' combine returned only { events, isPending }, so a failed reply subtree contributed zero rows and vanished silently — the same false-empty class the single-root thread panel guards against, on the multi-root Huddle/Projects surfaces. Expose aggregate isError/error plus a refetch that re-runs only the failed subtrees, extracted into a pure combineThreadRepliesResults so the contract is unit-testable. The Projects agent conversation now renders the shared Couldn't-load-replies + Retry card when a subtree fails. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../combineThreadRepliesResults.test.mjs | 104 ++++++++++++++++++ .../src/features/messages/useThreadReplies.ts | 38 ++++++- .../projects/ui/ProjectsAgentPromptPage.tsx | 4 + 3 files changed, 142 insertions(+), 4 deletions(-) create mode 100644 desktop/src/features/messages/combineThreadRepliesResults.test.mjs diff --git a/desktop/src/features/messages/combineThreadRepliesResults.test.mjs b/desktop/src/features/messages/combineThreadRepliesResults.test.mjs new file mode 100644 index 00000000000..47beb1c7a83 --- /dev/null +++ b/desktop/src/features/messages/combineThreadRepliesResults.test.mjs @@ -0,0 +1,104 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { combineThreadRepliesResults } from "./useThreadReplies.ts"; + +const CHANNEL_A = "a".repeat(64); +const CHANNEL_B = "b".repeat(64); + +function event(id, createdAt) { + return { + id, + pubkey: "c".repeat(64), + kind: 9, + created_at: createdAt, + content: "reply", + tags: [], + sig: "sig", + }; +} + +function ok(data) { + return { + data, + isPending: false, + isError: false, + error: null, + refetch: () => { + throw new Error("a successful subtree must not be refetched"); + }, + }; +} + +function failed(refetch) { + return { + data: undefined, + isPending: false, + isError: true, + error: new Error("subtree load failed"), + refetch, + }; +} + +function pending() { + return { + data: undefined, + isPending: true, + isError: false, + error: null, + refetch: () => {}, + }; +} + +test("aggregates events across roots in chronological order", () => { + const combined = combineThreadRepliesResults([ + ok([event(CHANNEL_A, 200)]), + ok([event(CHANNEL_B, 100)]), + ]); + assert.deepEqual( + combined.events.map((e) => e.created_at), + [100, 200], + ); + assert.equal(combined.isPending, false); + assert.equal(combined.isError, false); + assert.equal(combined.error, null); +}); + +test("a failed subtree surfaces aggregate error and never silently drops", () => { + // The load-bearing contract: one failed root among successful roots must make + // the aggregate report isError so the consumer can surface a failure instead + // of presenting a partial transcript as complete. + const combined = combineThreadRepliesResults([ + ok([event(CHANNEL_A, 100)]), + failed(() => {}), + ]); + assert.equal(combined.isError, true); + assert.ok(combined.error instanceof Error); + // Successful rows still contribute their events (non-destructive). + assert.equal(combined.events.length, 1); +}); + +test("isPending reflects any still-loading root", () => { + const combined = combineThreadRepliesResults([ok([]), pending()]); + assert.equal(combined.isPending, true); +}); + +test("refetch re-runs only the failed subtrees, not the successful ones", () => { + let failedRefetched = 0; + const combined = combineThreadRepliesResults([ + ok([event(CHANNEL_A, 100)]), + failed(() => { + failedRefetched += 1; + }), + ]); + // ok().refetch throws if called, so a partial-success refetch that touched + // every query would throw here; it must only touch the failed one. + combined.refetch(); + assert.equal(failedRefetched, 1); +}); + +test("all-success aggregate reports no error", () => { + const combined = combineThreadRepliesResults([ok([]), ok([])]); + assert.equal(combined.isError, false); + assert.equal(combined.error, null); +}); diff --git a/desktop/src/features/messages/useThreadReplies.ts b/desktop/src/features/messages/useThreadReplies.ts index 25a6b68986b..4d602348f95 100644 --- a/desktop/src/features/messages/useThreadReplies.ts +++ b/desktop/src/features/messages/useThreadReplies.ts @@ -67,6 +67,39 @@ export function useThreadReplies( }); } +/** + * Aggregate a set of per-root thread-reply query results into one view for a + * multi-root consumer. Pure over the results array so the load-bearing + * error-surfacing contract is unit-testable without a live QueryClient. + * + * `isError`/`error` expose aggregate terminal failure so a consumer never + * silently drops a failed reply subtree — the same false-empty class the + * single-root panel guards against. `error` carries the first failed subtree's + * error; `refetch` re-runs only the failed queries so a partial success is not + * needlessly re-fetched. + */ +export function combineThreadRepliesResults( + results: readonly { + data?: RelayEvent[]; + isPending: boolean; + isError: boolean; + error: unknown; + refetch: () => unknown; + }[], +) { + return { + events: sortMessages(results.flatMap((result) => result.data ?? [])), + isPending: results.some((result) => result.isPending), + isError: results.some((result) => result.isError), + error: results.find((result) => result.isError)?.error ?? null, + refetch: () => { + for (const result of results) { + if (result.isError) void result.refetch(); + } + }, + }; +} + /** * Load every summarized reply subtree for a channel-style Huddle transcript. * Ordinary channels keep replies in their thread panels; Huddles flatten those @@ -87,9 +120,6 @@ export function useThreadRepliesForRoots( staleTime: 0, gcTime: 60 * 60 * 1_000, })), - combine: (results) => ({ - events: sortMessages(results.flatMap((result) => result.data ?? [])), - isPending: results.some((result) => result.isPending), - }), + combine: combineThreadRepliesResults, }); } diff --git a/desktop/src/features/projects/ui/ProjectsAgentPromptPage.tsx b/desktop/src/features/projects/ui/ProjectsAgentPromptPage.tsx index 9a465d3da0e..f667fcd2a21 100644 --- a/desktop/src/features/projects/ui/ProjectsAgentPromptPage.tsx +++ b/desktop/src/features/projects/ui/ProjectsAgentPromptPage.tsx @@ -37,6 +37,7 @@ import { } from "@/features/messages/lib/useRichTextEditor"; import { FormattingToolbar } from "@/features/messages/ui/FormattingToolbar"; import { MessageThreadTranscript } from "@/features/messages/ui/MessageThreadTranscript"; +import { ThreadRepliesErrorCard } from "@/features/messages/ui/MessageThreadReplyState"; import type { TimelineMessage } from "@/features/messages/types"; import { useThreadRepliesForRoots } from "@/features/messages/useThreadReplies"; import { useProfileQuery, useUsersBatchQuery } from "@/features/profile/hooks"; @@ -363,6 +364,9 @@ export function ConversationThread({ profiles={profiles} renderAfterMessage={renderSubmittedContext} /> + {threadReplies.isError ? ( + + ) : null} {agentWorking.working ? (

From 6911a6e66e7fcd94162224604c24dcc9235f1946 Mon Sep 17 00:00:00 2001 From: Duncan Date: Sat, 22 Aug 2026 10:58:50 -0400 Subject: [PATCH 10/10] fix(desktop): surface failed reply subtrees on the Huddle transcript The Huddle transcript's useThreadRepliesForRoots fan-out reported an aggregate isError/refetch, but useHuddleChannelMessages consumed only .events and discarded it. One summarized root failing left the partial transcript presenting as complete with no warning or recovery, violating the aggregate-error contract this PR established for its other consumer. Propagate the aggregate failure through ChannelScreen into ChannelPane and render the shared ThreadRepliesErrorCard as a non-destructive banner above the timeline: successful rows stay visible and onRetry re-runs only the failed subtrees. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/playwright.config.ts | 1 + .../src/features/channels/ui/ChannelPane.tsx | 8 + .../features/channels/ui/ChannelPane.types.ts | 8 + .../features/channels/ui/ChannelScreen.tsx | 9 +- .../channels/ui/useHuddleChannelMessages.ts | 10 +- .../e2e/huddle-thread-load-failure.spec.ts | 182 ++++++++++++++++++ 6 files changed, 216 insertions(+), 2 deletions(-) create mode 100644 desktop/tests/e2e/huddle-thread-load-failure.spec.ts diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 351eff01c8d..69250c4b537 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -88,6 +88,7 @@ export default defineConfig({ "**/thread-unread.spec.ts", "**/thread-load-failure.spec.ts", "**/project-conversation-load-failure.spec.ts", + "**/huddle-thread-load-failure.spec.ts", "**/workspace-rail.spec.ts", "**/community-rail.spec.ts", "**/boot-splash.spec.ts", diff --git a/desktop/src/features/channels/ui/ChannelPane.tsx b/desktop/src/features/channels/ui/ChannelPane.tsx index 958b0ba738d..93eb3803f17 100644 --- a/desktop/src/features/channels/ui/ChannelPane.tsx +++ b/desktop/src/features/channels/ui/ChannelPane.tsx @@ -13,6 +13,7 @@ import { useRelaySelfQuery } from "@/features/moderation/hooks"; import { DropZoneOverlay } from "@/features/messages/ui/ComposerAttachments"; import { MessageThreadPanel } from "@/features/messages/ui/MessageThreadPanel"; import { MessageThreadPanelSkeleton } from "@/features/messages/ui/MessageThreadPanelSkeleton"; +import { ThreadRepliesErrorCard } from "@/features/messages/ui/MessageThreadReplyState"; import { MessageTimeline, type MessageTimelineHandle, @@ -95,6 +96,8 @@ export const ChannelPane = React.memo(function ChannelPane({ welcomeKickoffSettingUp = false, messages, threadSummaries, + huddleThreadRepliesError = false, + onRetryHuddleThreadReplies, firstUnreadMessageId = null, unreadCount = 0, canResetThreadPanelWidth, @@ -569,6 +572,11 @@ export const ChannelPane = React.memo(function ChannelPane({ } > {isHuddleTranscript ? null : header} + {isHuddleTranscript && huddleThreadRepliesError ? ( +
+ +
+ ) : null}
; + /** + * A Huddle transcript flattens summarized reply subtrees into the chat + * timeline. When one of those subtree loads fails, this reports the aggregate + * failure so the transcript can surface a non-destructive retry alert instead + * of silently presenting a partial conversation as complete. + */ + huddleThreadRepliesError?: boolean; + onRetryHuddleThreadReplies?: () => void; firstUnreadMessageId?: string | null; unreadCount?: number; canResetThreadPanelWidth: boolean; diff --git a/desktop/src/features/channels/ui/ChannelScreen.tsx b/desktop/src/features/channels/ui/ChannelScreen.tsx index c6803604720..06788de56f0 100644 --- a/desktop/src/features/channels/ui/ChannelScreen.tsx +++ b/desktop/src/features/channels/ui/ChannelScreen.tsx @@ -245,7 +245,12 @@ export function ChannelScreen({ const deleteMessageMutation = useDeleteMessageMutation(activeChannel); const editMessageMutation = useEditMessageMutation(activeChannel); const joinChannelMutation = useJoinChannelMutation(activeChannelId); - const { resolvedMessages, threadSummaries } = useHuddleChannelMessages({ + const { + resolvedMessages, + threadSummaries, + threadRepliesError: huddleThreadRepliesError, + onRetryThreadReplies: onRetryHuddleThreadReplies, + } = useHuddleChannelMessages({ activeChannel, isHuddleTranscript, messages: messagesQuery.data ?? EMPTY_RELAY_EVENTS, @@ -886,6 +891,8 @@ export function ChannelScreen({ isTimelineLoading={isTimelineLoading} messages={timelineMessages} threadSummaries={threadSummaries} + huddleThreadRepliesError={huddleThreadRepliesError} + onRetryHuddleThreadReplies={onRetryHuddleThreadReplies} onCancelEdit={handleCancelEdit} onCancelThreadReply={handleCancelThreadReply} onChannelManagementDeleted={handleChannelManagementDeleted} diff --git a/desktop/src/features/channels/ui/useHuddleChannelMessages.ts b/desktop/src/features/channels/ui/useHuddleChannelMessages.ts index 2a90971ddec..5a3a7c40419 100644 --- a/desktop/src/features/channels/ui/useHuddleChannelMessages.ts +++ b/desktop/src/features/channels/ui/useHuddleChannelMessages.ts @@ -66,5 +66,13 @@ export function useHuddleChannelMessages({ [huddleThreadReplies.events, isHuddleTranscript, resolvedChannelMessages], ); - return { resolvedMessages, threadSummaries }; + return { + resolvedMessages, + threadSummaries, + // A summarized reply subtree failing must not leave the transcript reading + // as complete: surface the aggregate failure so the consumer can show a + // non-destructive retry alert alongside the rows that did load. + threadRepliesError: isHuddleTranscript && huddleThreadReplies.isError, + onRetryThreadReplies: huddleThreadReplies.refetch, + }; } diff --git a/desktop/tests/e2e/huddle-thread-load-failure.spec.ts b/desktop/tests/e2e/huddle-thread-load-failure.spec.ts new file mode 100644 index 00000000000..8d3777201a9 --- /dev/null +++ b/desktop/tests/e2e/huddle-thread-load-failure.spec.ts @@ -0,0 +1,182 @@ +import { expect, test, type Page } from "@playwright/test"; + +import { installMockBridge, TEST_IDENTITIES } from "../helpers/bridge"; + +/** + * Consumer-level guard for the false-empty thread-load bug on the Huddle + * transcript (PR #6447, Carl r3). The transcript flattens summarized reply + * subtrees into the chat timeline via `useThreadRepliesForRoots`, whose combine + * now reports an aggregate `isError`/`refetch`. `useHuddleChannelMessages` used + * to read only `.events` and drop that state, so one failed subtree left the + * partial transcript presenting as complete with no warning or recovery. + * + * The combine unit test proves the hook REPORTS failure; it cannot catch a + * consumer discarding it. This drives the REAL Huddle wiring + * (useHuddleChannelMessages -> ChannelScreen -> ChannelPane) through the mock + * bridge: two summarized roots, fail only ONE subtree's fetch at the IPC + * boundary, assert the surviving root's reply still renders AND the retry alert + * appears, then Retry recovers the failed subtree. Dropping the propagation + * turns this red. + */ + +const HUDDLE_CHANNEL_ID = "11111111-1111-4111-8111-111111111111"; +const HUDDLE_PARENT_ID = "9a1657ac-f7aa-5db0-b632-d8bbeb6dfb50"; + +const ROOT_A_CONTENT = "Huddle root A"; +const REPLY_A_CONTENT = "Huddle reply A survives"; +const ROOT_B_CONTENT = "Huddle root B"; +const REPLY_B_CONTENT = "Huddle reply B recovered"; + +async function waitForMockLiveSubscription(page: Page, channelName: string) { + await expect + .poll(() => + page.evaluate( + (name) => + window.__BUZZ_E2E_HAS_MOCK_LIVE_SUBSCRIPTION__?.({ + channelName: name, + }) ?? false, + channelName, + ), + ) + .toBe(true); +} + +// Fail get_thread_replies for exactly one root while the flag names it, letting +// the other subtree and the eventual retry succeed. Wrapping the real +// __TAURI_INTERNALS__.invoke exercises the whole per-root query path with no +// source seam to bypass — the aggregate error and the failed-only refetch are +// production behavior, not a test stub. +async function installPerRootThreadFailureSwitch(page: Page) { + await page.evaluate(() => { + const w = window as typeof window & { + __FAIL_THREAD_ROOT__?: string | null; + __TAURI_INTERNALS__: { + invoke: ( + command: string, + payload: unknown, + options: unknown, + ) => Promise; + }; + }; + w.__FAIL_THREAD_ROOT__ = null; + const original = w.__TAURI_INTERNALS__.invoke.bind(w.__TAURI_INTERNALS__); + w.__TAURI_INTERNALS__.invoke = async (command, payload, options) => { + if ( + command === "get_thread_replies" && + w.__FAIL_THREAD_ROOT__ != null && + (payload as { rootEventId?: string })?.rootEventId === + w.__FAIL_THREAD_ROOT__ + ) { + throw new Error("relay unreachable: request timed out"); + } + return original(command, payload, options); + }; + }); +} + +async function setFailingThreadRoot(page: Page, rootId: string | null) { + await page.evaluate((rootId) => { + ( + window as typeof window & { __FAIL_THREAD_ROOT__?: string | null } + ).__FAIL_THREAD_ROOT__ = rootId; + }, rootId); +} + +test.describe("huddle thread load failure", () => { + test("a failed reply subtree shows the retry alert beside surviving rows; Retry recovers", async ({ + page, + }) => { + await installMockBridge(page, { + windowLabel: `huddle-${HUDDLE_CHANNEL_ID}`, + huddle: { + parentChannelId: HUDDLE_PARENT_ID, + ephemeralChannelId: HUDDLE_CHANNEL_ID, + members: [ + { pubkey: TEST_IDENTITIES.tyler.pubkey, role: "member" }, + { pubkey: TEST_IDENTITIES.alice.pubkey, role: "bot" }, + ], + transcriptionEnabled: true, + }, + }); + await page.goto("/"); + + await expect(page.getByTestId("huddle-transcript-intro")).toBeVisible(); + await installPerRootThreadFailureSwitch(page); + await waitForMockLiveSubscription(page, "huddle"); + + // Seed two summarized roots. Each threaded reply emits a live thread + // summary (descendant_count > 0), so both roots enter the transcript's + // useThreadRepliesForRoots fan-out and each gets its own subtree fetch. Fail + // root B's fetch BEFORE seeding so its first fan-out fetch reaches the + // terminal error while root A resolves — the aggregate reports failure with + // A's reply already merged, exactly the partial-transcript case. + const rootB = "b".repeat(64); + await setFailingThreadRoot(page, rootB); + const seeded = await page.evaluate( + ({ + agentPubkey, + rootAContent, + replyAContent, + rootBContent, + replyBContent, + rootBId, + }) => { + const emit = window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__; + if (!emit) throw new Error("Mock message emitter is not installed."); + const rootA = emit({ channelName: "huddle", content: rootAContent }); + emit({ + channelName: "huddle", + content: replyAContent, + parentEventId: rootA.id, + pubkey: agentPubkey, + }); + emit({ channelName: "huddle", content: rootBContent, id: rootBId }); + emit({ + channelName: "huddle", + content: replyBContent, + parentEventId: rootBId, + pubkey: agentPubkey, + }); + return { rootA: rootA.id }; + }, + { + agentPubkey: TEST_IDENTITIES.alice.pubkey, + rootAContent: ROOT_A_CONTENT, + replyAContent: REPLY_A_CONTENT, + rootBContent: ROOT_B_CONTENT, + replyBContent: REPLY_B_CONTENT, + rootBId: rootB, + }, + ); + expect(seeded.rootA).toBeTruthy(); + + // The load-bearing assertion: root A's reply still renders (non-destructive + // — a failed subtree does not blank the surviving rows) AND root B's failed + // fan-out surfaces the retry alert. That alert is rendered ONLY by the + // Huddle transcript's `huddleThreadRepliesError` propagation; without it the + // partial transcript would present as complete. (Reply rows themselves flow + // through the live channel window, so their presence is not the signal — the + // alert is.) + await expect( + page.getByTestId("message-row").filter({ hasText: REPLY_A_CONTENT }), + ).toBeVisible({ timeout: 15_000 }); + await expect( + page.getByTestId("message-thread-replies-error"), + ).toContainText("Couldn't load replies", { timeout: 15_000 }); + + // Retry with fetches succeeding: the failed-only refetch recovers root B's + // subtree, the aggregate error clears, and the alert is dismissed while both + // replies stay visible. + await setFailingThreadRoot(page, null); + await page.getByTestId("message-thread-replies-retry").click(); + await expect(page.getByTestId("message-thread-replies-error")).toHaveCount( + 0, + ); + await expect( + page.getByTestId("message-row").filter({ hasText: REPLY_B_CONTENT }), + ).toBeVisible(); + await expect( + page.getByTestId("message-row").filter({ hasText: REPLY_A_CONTENT }), + ).toBeVisible(); + }); +});