diff --git a/desktop/src-tauri/src/commands/link_preview.rs b/desktop/src-tauri/src/commands/link_preview.rs index d63d14da08..b6edc5db81 100644 --- a/desktop/src-tauri/src/commands/link_preview.rs +++ b/desktop/src-tauri/src/commands/link_preview.rs @@ -11,6 +11,14 @@ use reqwest::{ use serde::Serialize; use url::Url; +#[path = "link_preview_rate_limit.rs"] +mod rate_limit; + +use rate_limit::{ + image_host_cooldown_remaining, retry_after_duration, set_image_host_cooldown, + MAX_IMAGE_RETRY_AFTER, +}; + const MAX_PREVIEW_FETCH_BYTES: usize = 256 * 1024; const MAX_IMAGE_FETCH_BYTES: usize = 2 * 1024 * 1024; const MAX_IMAGE_DIMENSION: u32 = 4096; @@ -22,6 +30,15 @@ const MAX_REDIRECTS: usize = 3; const MAX_METADATA_CHARS: usize = 180; const MAX_METADATA_DESCRIPTION_CHARS: usize = 280; +#[derive(Clone, Copy, Debug, PartialEq, Serialize)] +#[serde(rename_all = "snake_case")] +enum LinkPreviewImageFetchState { + None, + Image, + TransientFailure, + Rejected, +} + #[derive(Debug, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub struct LinkPreviewMetadata { @@ -30,6 +47,8 @@ pub struct LinkPreviewMetadata { description: Option, image_data_url: Option, image_domain: Option, + image_fetch_state: LinkPreviewImageFetchState, + image_retry_after_ms: Option, favicon_data_url: Option, } @@ -79,16 +98,38 @@ async fn fetch_link_preview_metadata_inner( let Some(mut metadata) = extract_link_preview_metadata(&body) else { return Ok(None); }; - if let Some(image_url) = extract_image_url(&body, &url) { - if let Ok((data_url, domain)) = fetch_sanitized_image(image_url, false).await { - metadata.image_data_url = Some(data_url); - metadata.image_domain = Some(domain); - } - } - if let Some(favicon_url) = extract_favicon_url(&body, &url) { - if let Ok((data_url, _)) = fetch_sanitized_image(favicon_url, true).await { - metadata.favicon_data_url = Some(data_url); + let image_url = extract_image_url(&body, &url); + let favicon_url = extract_favicon_url(&body, &url); + let (image_result, favicon_result) = tokio::join!( + async { + match image_url { + Some(image_url) => Some( + tokio::time::timeout( + PREVIEW_FETCH_TIMEOUT, + fetch_sanitized_image(image_url, false), + ) + .await + .unwrap_or(Err(ImageFetchError::Transient { retry_after: None })), + ), + None => None, + } + }, + async { + match favicon_url { + Some(favicon_url) => tokio::time::timeout( + PREVIEW_FETCH_TIMEOUT, + fetch_sanitized_image(favicon_url, true), + ) + .await + .ok(), + None => None, + } } + ); + + apply_image_result(&mut metadata, image_result); + if let Some(Ok((data_url, _))) = favicon_result { + metadata.favicon_data_url = Some(data_url); } return Ok(Some(metadata)); } @@ -96,6 +137,28 @@ async fn fetch_link_preview_metadata_inner( Ok(None) } +fn apply_image_result( + metadata: &mut LinkPreviewMetadata, + image_result: Option>, +) { + match image_result { + Some(Ok((data_url, domain))) => { + metadata.image_data_url = Some(data_url); + metadata.image_domain = Some(domain); + metadata.image_fetch_state = LinkPreviewImageFetchState::Image; + } + Some(Err(ImageFetchError::Transient { retry_after })) => { + metadata.image_fetch_state = LinkPreviewImageFetchState::TransientFailure; + metadata.image_retry_after_ms = + retry_after.and_then(|duration| u64::try_from(duration.as_millis()).ok()); + } + Some(Err(ImageFetchError::Rejected)) => { + metadata.image_fetch_state = LinkPreviewImageFetchState::Rejected; + } + None => {} + } +} + async fn validate_public_https_url(url: &Url) -> Result<(), String> { if url.scheme() != "https" || url.username() != "" || url.password().is_some() { return Err("link previews require an HTTPS URL without credentials".to_string()); @@ -251,30 +314,57 @@ fn extract_image_url(html: &str, page_url: &Url) -> Option { page_url.join(raw.trim()).ok() } +#[derive(Debug, PartialEq)] +enum ImageFetchError { + Transient { retry_after: Option }, + Rejected, +} + async fn fetch_sanitized_image( mut url: Url, preserve_transparency: bool, -) -> Result<(String, String), String> { - validate_public_https_url(&url).await?; +) -> Result<(String, String), ImageFetchError> { + validate_public_https_url(&url) + .await + .map_err(|_| ImageFetchError::Rejected)?; for redirect_count in 0..=MAX_REDIRECTS { - let response = send_pinned_request(&url, "image/jpeg,image/png,image/webp").await?; + if let Some(retry_after) = image_host_cooldown_remaining(&url) { + return Err(ImageFetchError::Transient { + retry_after: Some(retry_after), + }); + } + let response = send_pinned_request(&url, "image/jpeg,image/png,image/webp") + .await + .map_err(|_| ImageFetchError::Transient { retry_after: None })?; if response.status().is_redirection() { if redirect_count == MAX_REDIRECTS { - return Err("link preview image redirected too many times".to_string()); + return Err(ImageFetchError::Rejected); } let location = response .headers() .get(LOCATION) .and_then(|value| value.to_str().ok()) - .ok_or_else(|| "link preview image redirect has an invalid location".to_string())?; - url = url - .join(location) - .map_err(|error| format!("invalid link preview image redirect: {error}"))?; - validate_public_https_url(&url).await?; + .ok_or(ImageFetchError::Rejected)?; + url = url.join(location).map_err(|_| ImageFetchError::Rejected)?; + validate_public_https_url(&url) + .await + .map_err(|_| ImageFetchError::Rejected)?; continue; } if !response.status().is_success() { - return Err("link preview image request was unsuccessful".to_string()); + let status = response.status(); + if status == reqwest::StatusCode::TOO_MANY_REQUESTS + || status == reqwest::StatusCode::REQUEST_TIMEOUT + || status == reqwest::StatusCode::TOO_EARLY + || status.is_server_error() + { + let retry_after = retry_after_duration(&response); + if let Some(retry_after) = retry_after { + set_image_host_cooldown(&url, retry_after); + } + return Err(ImageFetchError::Transient { retry_after }); + } + return Err(ImageFetchError::Rejected); } let declared_mime = response .headers() @@ -288,29 +378,32 @@ async fn fetch_sanitized_image( .trim() .to_ascii_lowercase() }) - .ok_or_else(|| "link preview image has no content type".to_string())?; + .ok_or(ImageFetchError::Rejected)?; if !matches!( declared_mime.as_str(), "image/jpeg" | "image/png" | "image/webp" ) { - return Err("link preview image type is unsupported".to_string()); + return Err(ImageFetchError::Rejected); } if response .content_length() .is_some_and(|size| size > MAX_IMAGE_FETCH_BYTES as u64) { - return Err("link preview image exceeded the size limit".to_string()); + return Err(ImageFetchError::Rejected); } - let bytes = read_limited_bytes(response, MAX_IMAGE_FETCH_BYTES).await?; + let bytes = read_limited_bytes(response, MAX_IMAGE_FETCH_BYTES) + .await + .map_err(|_| ImageFetchError::Rejected)?; let data_url = tokio::task::spawn_blocking(move || { sanitize_image(&bytes, &declared_mime, preserve_transparency) }) .await - .map_err(|_| "link preview image sanitizer failed".to_string())??; + .map_err(|_| ImageFetchError::Rejected)? + .map_err(|_| ImageFetchError::Rejected)?; let domain = url.host_str().unwrap_or_default().to_string(); return Ok((data_url, domain)); } - Err("link preview image fetch failed".to_string()) + Err(ImageFetchError::Rejected) } fn sanitize_image( @@ -411,6 +504,8 @@ fn extract_link_preview_metadata(html: &str) -> Option { description, image_data_url: None, image_domain: None, + image_fetch_state: LinkPreviewImageFetchState::None, + image_retry_after_ms: None, favicon_data_url: None, }) } @@ -557,9 +652,10 @@ fn decode_html_entities(value: &str) -> String { #[cfg(test)] mod tests { use super::{ - declares_animation, extract_favicon_url, extract_image_url, extract_link_preview_metadata, - is_html_response, read_bytes_prefix, sanitize_image, LinkPreviewMetadata, - MAX_METADATA_DESCRIPTION_CHARS, + apply_image_result, declares_animation, extract_favicon_url, extract_image_url, + extract_link_preview_metadata, is_html_response, read_bytes_prefix, retry_after_duration, + sanitize_image, ImageFetchError, LinkPreviewImageFetchState, LinkPreviewMetadata, + MAX_IMAGE_RETRY_AFTER, MAX_METADATA_DESCRIPTION_CHARS, }; use axum::{body::Body, http::Response, routing::get, Router}; use base64::Engine as _; @@ -594,11 +690,45 @@ mod tests { description: Some("Safe & useful previews".to_string()), image_data_url: None, image_domain: None, + image_fetch_state: LinkPreviewImageFetchState::None, + image_retry_after_ms: None, favicon_data_url: None, }) ); } + #[test] + fn image_results_preserve_absence_and_classify_recovery() { + let mut metadata = extract_link_preview_metadata("Preview result").unwrap(); + apply_image_result(&mut metadata, None); + assert_eq!(metadata.image_fetch_state, LinkPreviewImageFetchState::None); + + apply_image_result( + &mut metadata, + Some(Err(ImageFetchError::Transient { + retry_after: Some(std::time::Duration::from_secs(15)), + })), + ); + assert_eq!( + metadata.image_fetch_state, + LinkPreviewImageFetchState::TransientFailure + ); + assert_eq!(metadata.image_retry_after_ms, Some(15_000)); + + apply_image_result( + &mut metadata, + Some(Ok(( + "data:image/jpeg;base64,abc".to_string(), + "images.example.com".to_string(), + ))), + ); + assert_eq!( + metadata.image_fetch_state, + LinkPreviewImageFetchState::Image + ); + assert_eq!(metadata.image_domain.as_deref(), Some("images.example.com")); + } + #[test] fn metadata_falls_back_to_twitter_then_title() { assert_eq!( @@ -716,6 +846,44 @@ mod tests { assert!(extract_image_url(&html, &Url::parse("https://example.com").unwrap()).is_some()); } + #[tokio::test] + async fn image_retry_after_uses_bounded_delta_seconds() { + let response = test_response( + Router::new().route( + "/rate-limited", + get(|| async { + Response::builder() + .status(429) + .header("retry-after", "900") + .body(Body::empty()) + .unwrap() + }), + ), + "/rate-limited", + ) + .await; + assert_eq!( + retry_after_duration(&response), + Some(std::time::Duration::from_secs(900)) + ); + + let response = test_response( + Router::new().route( + "/excessive", + get(|| async { + Response::builder() + .status(429) + .header("retry-after", "7200") + .body(Body::empty()) + .unwrap() + }), + ), + "/excessive", + ) + .await; + assert_eq!(retry_after_duration(&response), Some(MAX_IMAGE_RETRY_AFTER)); + } + #[tokio::test] async fn oversized_chunked_html_ignores_metadata_beyond_the_bounded_prefix() { const LIMIT: usize = 256; diff --git a/desktop/src-tauri/src/commands/link_preview_rate_limit.rs b/desktop/src-tauri/src/commands/link_preview_rate_limit.rs new file mode 100644 index 0000000000..c3f7ed7188 --- /dev/null +++ b/desktop/src-tauri/src/commands/link_preview_rate_limit.rs @@ -0,0 +1,62 @@ +use std::{ + collections::HashMap, + sync::{Mutex, OnceLock}, + time::{Duration, Instant}, +}; + +use reqwest::header::RETRY_AFTER; +use url::Url; + +pub(super) const MAX_IMAGE_RETRY_AFTER: Duration = Duration::from_secs(60 * 60); +const MAX_IMAGE_HOST_COOLDOWNS: usize = 128; + +static IMAGE_HOST_COOLDOWNS: OnceLock>> = OnceLock::new(); + +pub(super) fn retry_after_duration(response: &reqwest::Response) -> Option { + response + .headers() + .get(RETRY_AFTER) + .and_then(|value| value.to_str().ok()) + .and_then(|value| value.trim().parse::().ok()) + .map(Duration::from_secs) + .map(|duration| duration.min(MAX_IMAGE_RETRY_AFTER)) +} + +pub(super) fn image_host_cooldown_remaining(url: &Url) -> Option { + let host = url.host_str()?; + let cooldowns = IMAGE_HOST_COOLDOWNS.get_or_init(|| Mutex::new(HashMap::new())); + let Ok(mut cooldowns) = cooldowns.lock() else { + return None; + }; + let expires_at = cooldowns.get(host).copied()?; + let now = Instant::now(); + if expires_at <= now { + cooldowns.remove(host); + return None; + } + Some(expires_at.duration_since(now)) +} + +pub(super) fn set_image_host_cooldown(url: &Url, retry_after: Duration) { + let Some(host) = url.host_str() else { + return; + }; + let now = Instant::now(); + let Some(expires_at) = now.checked_add(retry_after) else { + return; + }; + let cooldowns = IMAGE_HOST_COOLDOWNS.get_or_init(|| Mutex::new(HashMap::new())); + if let Ok(mut cooldowns) = cooldowns.lock() { + cooldowns.retain(|_, current_expiry| *current_expiry > now); + if cooldowns.len() >= MAX_IMAGE_HOST_COOLDOWNS && !cooldowns.contains_key(host) { + let oldest_host = cooldowns + .iter() + .min_by_key(|(_, current_expiry)| *current_expiry) + .map(|(current_host, _)| current_host.clone()); + if let Some(oldest_host) = oldest_host { + cooldowns.remove(&oldest_host); + } + } + cooldowns.insert(host.to_string(), expires_at); + } +} diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs b/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs index a594e316b7..bf7e393612 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs @@ -1,7 +1,10 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { resolveLinkPreview } from "./useResolvedLinkPreviews.ts"; +import { + __linkPreviewMetadataTest, + resolveLinkPreview, +} from "./useResolvedLinkPreviews.ts"; const preview = { kind: "generic-link", @@ -11,6 +14,19 @@ const preview = { typeLabel: "link", }; +function metadata(overrides = {}) { + return { + title: "A story", + siteName: "Example", + description: "Story description", + imageDataUrl: null, + imageDomain: null, + imageFetchState: "none", + imageRetryAfterMs: null, + ...overrides, + }; +} + test("pending metadata reserves the image treatment", () => { assert.deepEqual(resolveLinkPreview(preview, undefined), { ...preview, @@ -43,3 +59,124 @@ test("resolved metadata without a complete image collapses to the compact treatm assert.equal(resolved.imageDataUrl, null); assert.equal(resolved.imageDomain, null); }); + +test("transient and rejected image fetches use the stable fallback treatment", () => { + const transient = resolveLinkPreview( + preview, + metadata({ + imageFetchState: "transient_failure", + imageRetryAfterMs: 900_000, + }), + ); + const rejected = resolveLinkPreview( + preview, + metadata({ imageFetchState: "rejected" }), + ); + + assert.equal(transient.imageState, "fallback"); + assert.equal(rejected.imageState, "fallback"); +}); + +test("metadata cache keys deduplicate URL fragments", () => { + assert.equal( + __linkPreviewMetadataTest.metadataCacheKey( + "https://github.com/block/buzz/pull/3834#issuecomment-1", + ), + "https://github.com/block/buzz/pull/3834", + ); +}); + +test("transient metadata expires at the server retry boundary", () => { + assert.equal( + __linkPreviewMetadataTest.metadataExpiry( + metadata({ + imageFetchState: "transient_failure", + imageRetryAfterMs: 900_000, + }), + 1_000, + ), + 901_000, + ); +}); + +test("metadata loader retries transient images after the server cooldown", async () => { + let now = 1_000; + let calls = 0; + const loader = __linkPreviewMetadataTest.createMetadataLoader({ + fetcher: async () => { + calls += 1; + return calls === 1 + ? metadata({ + imageFetchState: "transient_failure", + imageRetryAfterMs: 10_000, + }) + : metadata({ + imageDataUrl: "data:image/jpeg;base64,abc", + imageDomain: "images.example.com", + imageFetchState: "image", + }); + }, + now: () => now, + }); + + assert.equal( + (await loader.load(preview.href)).metadata?.imageFetchState, + "transient_failure", + ); + assert.equal(calls, 1); + + now += 10_000; + assert.equal( + (await loader.load(preview.href)).metadata?.imageFetchState, + "image", + ); + assert.equal(calls, 2); +}); + +test("metadata loader retries rejected requests after the negative-cache TTL", async () => { + let now = 1_000; + let calls = 0; + const loader = __linkPreviewMetadataTest.createMetadataLoader({ + fetcher: async () => { + calls += 1; + if (calls === 1) throw new Error("temporary failure"); + return metadata(); + }, + now: () => now, + }); + + assert.equal((await loader.load(preview.href)).metadata, null); + assert.equal((await loader.load(preview.href)).metadata, null); + assert.equal(calls, 1); + + now += 5 * 60_000; + assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); + assert.equal(calls, 2); +}); + +test("metadata loader coalesces fragment variants and bounds concurrency", async () => { + let active = 0; + let maxActive = 0; + let calls = 0; + const loader = __linkPreviewMetadataTest.createMetadataLoader({ + concurrency: 2, + fetcher: async () => { + calls += 1; + active += 1; + maxActive = Math.max(maxActive, active); + await new Promise((resolve) => setImmediate(resolve)); + active -= 1; + return metadata(); + }, + }); + + await Promise.all([ + loader.load("https://example.com/one#first"), + loader.load("https://example.com/one#second"), + loader.load("https://example.com/two"), + loader.load("https://example.com/three"), + ]); + + assert.equal(calls, 3); + assert.equal(maxActive, 2); +}); diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.ts b/desktop/src/shared/lib/useResolvedLinkPreviews.ts index 71d610ff2b..cd79864c7d 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.ts +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.ts @@ -4,23 +4,153 @@ import { invokeTauri } from "@/shared/api/tauri"; import type { SupportedLinkPreview } from "./linkPreview"; +type LinkPreviewImageFetchState = + | "none" + | "image" + | "transient_failure" + | "rejected"; + type LinkPreviewMetadata = { title: string; siteName: string | null; description: string | null; imageDataUrl: string | null; imageDomain: string | null; + imageFetchState?: LinkPreviewImageFetchState; + imageRetryAfterMs?: number | null; faviconDataUrl?: string | null; }; -const metadataCache = new Map< - string, - Promise | LinkPreviewMetadata | null ->(); +type MetadataCacheEntry = { + expiresAt: number | null; + metadata: LinkPreviewMetadata | null; +}; -/** Clear ephemeral metadata when the active relay/community changes. */ -export function resetLinkPreviewMetadataCache(): void { - metadataCache.clear(); +type MetadataLoadResult = MetadataCacheEntry & { + key: string; +}; + +const DEFAULT_TRANSIENT_RETRY_MS = 30_000; +const NULL_METADATA_RETRY_MS = 5 * 60_000; +const MAX_CONCURRENT_METADATA_FETCHES = 2; + +function metadataCacheKey(href: string): string { + try { + const url = new URL(href); + url.hash = ""; + return url.href; + } catch { + return href.split("#", 1)[0] ?? href; + } +} + +function metadataExpiry( + metadata: LinkPreviewMetadata | null, + now: number, +): number | null { + if (metadata === null) return now + NULL_METADATA_RETRY_MS; + if (metadata.imageFetchState !== "transient_failure") return null; + const retryAfterMs = + typeof metadata.imageRetryAfterMs === "number" && + Number.isFinite(metadata.imageRetryAfterMs) + ? Math.max(1_000, metadata.imageRetryAfterMs) + : DEFAULT_TRANSIENT_RETRY_MS; + return now + retryAfterMs; +} + +function createTaskScheduler(concurrency: number) { + const pending: Array<() => void> = []; + let active = 0; + + const drain = () => { + while (active < concurrency) { + const run = pending.shift(); + if (!run) return; + active += 1; + run(); + } + }; + + return (task: () => Promise): Promise => + new Promise((resolve, reject) => { + pending.push(() => { + void task() + .then(resolve, reject) + .finally(() => { + active -= 1; + drain(); + }); + }); + drain(); + }); +} + +function createMetadataLoader({ + concurrency = MAX_CONCURRENT_METADATA_FETCHES, + fetcher, + now = Date.now, +}: { + concurrency?: number; + fetcher: (href: string) => Promise; + now?: () => number; +}) { + const cache = new Map< + string, + MetadataCacheEntry | Promise + >(); + const schedule = createTaskScheduler(Math.max(1, concurrency)); + let generation = 0; + + const peek = (href: string): MetadataLoadResult | undefined => { + const key = metadataCacheKey(href); + const cached = cache.get(key); + if (!cached || cached instanceof Promise) return undefined; + if (cached.expiresAt !== null && cached.expiresAt <= now()) { + cache.delete(key); + return undefined; + } + return { key, ...cached }; + }; + + const load = (href: string): Promise => { + const key = metadataCacheKey(href); + const cached = cache.get(key); + if (cached instanceof Promise) return cached; + if (cached) { + if (cached.expiresAt === null || cached.expiresAt > now()) { + return Promise.resolve({ key, ...cached }); + } + cache.delete(key); + } + + const requestGeneration = generation; + const promise = schedule(() => fetcher(href)) + .catch(() => null) + .then((metadata) => { + const entry = { + expiresAt: metadataExpiry(metadata, now()), + metadata, + }; + if (requestGeneration === generation) { + cache.set(key, entry); + } + return { key, ...entry }; + }); + cache.set(key, promise); + return promise; + }; + + return { + deleteKey(key: string) { + cache.delete(key); + }, + load, + peek, + reset() { + generation += 1; + cache.clear(); + }, + }; } function fetchLinkPreviewMetadata( @@ -34,25 +164,16 @@ function fetchLinkPreviewMetadata( ); } -function cacheMetadata(href: string): Promise { - const cached = metadataCache.get(href); - if (cached instanceof Promise) return cached; - if (cached !== undefined) return Promise.resolve(cached); +const metadataLoader = createMetadataLoader({ + fetcher: fetchLinkPreviewMetadata, +}); - const promise = fetchLinkPreviewMetadata(href) - .then((metadata) => { - metadataCache.set(href, metadata); - return metadata; - }) - .catch(() => { - metadataCache.set(href, null); - return null; - }); - metadataCache.set(href, promise); - return promise; +/** Clear ephemeral metadata when the active relay/community changes. */ +export function resetLinkPreviewMetadataCache(): void { + metadataLoader.reset(); } -export type LinkPreviewImageState = "pending" | "image" | "none"; +export type LinkPreviewImageState = "pending" | "image" | "fallback" | "none"; export type ResolvedLinkPreview = SupportedLinkPreview & { description?: string | null; @@ -77,6 +198,13 @@ export function resolveLinkPreview( } const hasImage = Boolean(metadata.imageDataUrl && metadata.imageDomain); + const imageState: LinkPreviewImageState = hasImage + ? "image" + : metadata.imageFetchState === "image" || + metadata.imageFetchState === "transient_failure" || + metadata.imageFetchState === "rejected" + ? "fallback" + : "none"; return { ...preview, title: metadata.title, @@ -88,7 +216,7 @@ export function resolveLinkPreview( : preview.provider, imageDataUrl: hasImage ? metadata.imageDataUrl : null, imageDomain: hasImage ? metadata.imageDomain : null, - imageState: hasImage ? "image" : "none", + imageState, }; } @@ -97,41 +225,79 @@ export function useResolvedLinkPreviews( ): ResolvedLinkPreview[] { const [resolvedMetadata, setResolvedMetadata] = React.useState({}); + const [retryGeneration, setRetryGeneration] = React.useState(0); React.useEffect(() => { let cancelled = false; + let retryAt = Number.POSITIVE_INFINITY; + let retryTimer: ReturnType | null = null; + + const scheduleRetry = ({ + expiresAt, + key, + }: Pick) => { + if (expiresAt === null || expiresAt >= retryAt) return; + retryAt = expiresAt; + if (retryTimer !== null) clearTimeout(retryTimer); + retryTimer = setTimeout( + () => { + metadataLoader.deleteKey(key); + setResolvedMetadata((current) => { + if (!(key in current)) return current; + const next = { ...current }; + delete next[key]; + return next; + }); + setRetryGeneration(retryGeneration + 1); + }, + Math.max(0, expiresAt - Date.now()), + ); + }; for (const preview of previews) { - const cached = metadataCache.get(preview.href); - if (cached && !(cached instanceof Promise)) { + const cached = metadataLoader.peek(preview.href); + if (cached !== undefined) { setResolvedMetadata((current) => - current[preview.href] === cached + current[cached.key] === cached.metadata ? current - : { ...current, [preview.href]: cached }, + : { ...current, [cached.key]: cached.metadata }, ); + scheduleRetry(cached); continue; } - void cacheMetadata(preview.href).then((metadata) => { + void metadataLoader.load(preview.href).then((result) => { if (cancelled) return; setResolvedMetadata((current) => - current[preview.href] === metadata + current[result.key] === result.metadata ? current - : { ...current, [preview.href]: metadata }, + : { ...current, [result.key]: result.metadata }, ); + scheduleRetry(result); }); } return () => { cancelled = true; + if (retryTimer !== null) clearTimeout(retryTimer); }; - }, [previews]); + }, [previews, retryGeneration]); return React.useMemo( () => previews.map((preview) => - resolveLinkPreview(preview, resolvedMetadata[preview.href]), + resolveLinkPreview( + preview, + resolvedMetadata[metadataCacheKey(preview.href)], + ), ), [previews, resolvedMetadata], ); } + +export const __linkPreviewMetadataTest = { + createMetadataLoader, + createTaskScheduler, + metadataCacheKey, + metadataExpiry, +}; diff --git a/desktop/src/shared/ui/compact-link-preview-attachment.tsx b/desktop/src/shared/ui/compact-link-preview-attachment.tsx index b108048091..c82e0d4895 100644 --- a/desktop/src/shared/ui/compact-link-preview-attachment.tsx +++ b/desktop/src/shared/ui/compact-link-preview-attachment.tsx @@ -1,3 +1,6 @@ +import { ImageOff } from "lucide-react"; +import { useState } from "react"; + import type { ResolvedLinkPreview } from "@/shared/lib/useResolvedLinkPreviews"; import { cn } from "@/shared/lib/cn"; import { @@ -18,6 +21,30 @@ function getHostname(preview: ResolvedLinkPreview): string { } } +function LinkPreviewImageFallback({ + preview, +}: { + preview: ResolvedLinkPreview; +}) { + return ( + + ); +} + export function CompactLinkPreviewAttachment({ className, onRemove, @@ -30,7 +57,12 @@ export function CompactLinkPreviewAttachment({ showControls?: boolean; }) { const reserveImage = preview.imageState !== "none"; - const showImage = preview.imageState === "image"; + const imageSrc = + preview.imageState === "image" ? (preview.imageDataUrl ?? null) : null; + const [failedImageSrc, setFailedImageSrc] = useState(null); + const showImage = Boolean(imageSrc && failedImageSrc !== imageSrc); + const showFallback = + preview.imageState === "fallback" || Boolean(imageSrc && !showImage); const hostname = getHostname(preview); return ( @@ -57,8 +89,11 @@ export function CompactLinkPreviewAttachment({ {`Preview setFailedImageSrc(imageSrc)} + src={imageSrc ?? undefined} /> + ) : showFallback ? ( + ) : (
(null); + const imageFailed = Boolean(imageSrc && failedImageSrc === imageSrc); + const showFallback = preview.imageState === "fallback" || imageFailed; const alt = `Preview from ${preview.imageDomain}`; - if (!imageSrc) { + if (!imageSrc || imageFailed) { return (
-
+ {showFallback ? ( + + ) : ( +
+ )}
); } @@ -96,6 +120,7 @@ function LinkPreviewImage({ {alt} setFailedImageSrc(imageSrc)} src={imageSrc} />
diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index f78294fa74..d93265fb2f 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -336,8 +336,23 @@ type E2eConfig = { description: string | null; imageDataUrl: string | null; imageDomain: string | null; + imageFetchState?: "none" | "image" | "transient_failure" | "rejected"; + imageRetryAfterMs?: number | null; faviconDataUrl?: string | null; } | null; + linkPreviewMetadataByHref?: Record< + string, + { + title: string; + siteName: string | null; + description: string | null; + imageDataUrl: string | null; + imageDomain: string | null; + imageFetchState?: "none" | "image" | "transient_failure" | "rejected"; + imageRetryAfterMs?: number | null; + faviconDataUrl?: string | null; + } | null + >; linkPreviewMetadataDelayMs?: number; searchProfiles?: MockSearchProfileSeed[]; updateAvailable?: boolean; @@ -10967,6 +10982,11 @@ export function maybeInstallE2eTauriMocks() { if (delayMs > 0) { await new Promise((resolve) => window.setTimeout(resolve, delayMs)); } + const href = (payload as { href?: string }).href; + const metadataByHref = activeConfig?.mock?.linkPreviewMetadataByHref; + if (href && metadataByHref && Object.hasOwn(metadataByHref, href)) { + return metadataByHref[href]; + } return activeConfig?.mock?.linkPreviewMetadata ?? null; } case "apply_workspace": { diff --git a/desktop/tests/e2e/messaging.spec.ts b/desktop/tests/e2e/messaging.spec.ts index bd7a530021..6e5d693c86 100644 --- a/desktop/tests/e2e/messaging.spec.ts +++ b/desktop/tests/e2e/messaging.spec.ts @@ -131,35 +131,72 @@ test.beforeEach(async ({ page }, testInfo) => { imageDomain: "pbs.twimg.com", }, } - : testInfo.title.includes("link preview image geometry") + : testInfo.title.includes("mixed link preview image outcomes") ? { - linkPreviewMetadata: { - title: - "Ship a wider horizontal preview with a two-line title that wraps cleanly", - siteName: "GitHub", - description: "A polished, stable preview for shared links.", - imageDataUrl: - "data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='1600' height='120'%3E%3Crect width='1600' height='120' fill='%237c3aed'/%3E%3Ccircle cx='92' cy='60' r='48' fill='%23fff' fill-opacity='.9'/%3E%3C/svg%3E", - imageDomain: "opengraph.githubassets.com", - faviconDataUrl: - "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", - }, - linkPreviewMetadataDelayMs: 800, - } - : testInfo.title.includes("link preview no-image layout") - ? { - linkPreviewMetadata: { - title: "Buzz", + linkPreviewMetadataByHref: { + "https://github.com/block/buzz/pull/4001": { + title: "Loaded preview image", siteName: "GitHub", - description: "Open-source collaboration for the Buzz app.", + description: "The image request completed.", + imageDataUrl: + "data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='1200' height='630'%3E%3Crect width='1200' height='630' fill='%237c3aed'/%3E%3C/svg%3E", + imageDomain: "opengraph.githubassets.com", + imageFetchState: "image", + imageRetryAfterMs: null, + }, + "https://github.com/block/buzz/pull/4002": { + title: "Rate-limited preview image", + siteName: "GitHub", + description: "Metadata remains available during cooldown.", imageDataUrl: null, imageDomain: null, - faviconDataUrl: - "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", + imageFetchState: "transient_failure", + imageRetryAfterMs: 900_000, + }, + }, + } + : testInfo.title.includes("link preview browser image error") + ? { + linkPreviewMetadata: { + title: "Invalid decoded preview image", + siteName: "GitHub", + description: "The browser should replace this image.", + imageDataUrl: "data:image/png;base64,not-a-valid-image", + imageDomain: "opengraph.githubassets.com", + imageFetchState: "image", + imageRetryAfterMs: null, }, - linkPreviewMetadataDelayMs: 2_000, } - : undefined; + : testInfo.title.includes("link preview image geometry") + ? { + linkPreviewMetadata: { + title: + "Ship a wider horizontal preview with a two-line title that wraps cleanly", + siteName: "GitHub", + description: "A polished, stable preview for shared links.", + imageDataUrl: + "data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='1600' height='120'%3E%3Crect width='1600' height='120' fill='%237c3aed'/%3E%3Ccircle cx='92' cy='60' r='48' fill='%23fff' fill-opacity='.9'/%3E%3C/svg%3E", + imageDomain: "opengraph.githubassets.com", + faviconDataUrl: + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", + }, + linkPreviewMetadataDelayMs: 800, + } + : testInfo.title.includes("link preview no-image layout") + ? { + linkPreviewMetadata: { + title: "Buzz", + siteName: "GitHub", + description: + "Open-source collaboration for the Buzz app.", + imageDataUrl: null, + imageDomain: null, + faviconDataUrl: + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", + }, + linkPreviewMetadataDelayMs: 2_000, + } + : undefined; await installMockBridge(page, mock); }); @@ -508,6 +545,77 @@ test("link preview no-image layout keeps compact height", async ({ page }) => { } }); +test("mixed link preview image outcomes keep Compact and Rich fallbacks stable", async ({ + page, +}) => { + const loadedUrl = "https://github.com/block/buzz/pull/4001"; + const rateLimitedUrl = "https://github.com/block/buzz/pull/4002"; + await page.goto("/"); + await page.getByTestId("channel-general").click(); + await page + .getByTestId("message-input") + .fill(`${loadedUrl}\n${rateLimitedUrl}`); + await page.getByTestId("send-message").click(); + + const row = page.getByTestId("message-row").last(); + const compactCards = row.locator('[data-link-preview="github-pull-request"]'); + await expect(compactCards).toHaveCount(2); + await expect(compactCards.nth(0)).toHaveAttribute( + "data-image-state", + "image", + ); + await expect(compactCards.nth(0).locator("img")).toBeVisible(); + await expect(compactCards.nth(1)).toHaveAttribute( + "data-image-state", + "fallback", + ); + await expect( + compactCards.nth(1).locator("[data-link-preview-image-fallback]"), + ).toBeVisible(); + + await openSettings(page, "appearance"); + await page.getByTestId("link-preview-style-trigger").click(); + await page.getByTestId("link-preview-style-rich").click(); + await page.getByTestId("settings-back-to-app").click(); + + const richCards = row.locator( + '[data-link-preview="github-pull-request"][data-link-preview-inline]', + ); + await expect(richCards).toHaveCount(2); + await expect( + richCards.nth(1).locator("[data-link-preview-image-fallback]"), + ).toBeVisible(); +}); + +test("link preview browser image errors render a fallback", async ({ + page, +}) => { + await page.goto("/"); + await page.getByTestId("channel-general").click(); + await page + .getByTestId("message-input") + .fill("https://github.com/block/buzz/pull/4003"); + await page.getByTestId("send-message").click(); + + const row = page.getByTestId("message-row").last(); + const compactCard = row.locator('[data-link-preview="github-pull-request"]'); + await expect( + compactCard.locator("[data-link-preview-image-fallback]"), + ).toBeVisible(); + + await openSettings(page, "appearance"); + await page.getByTestId("link-preview-style-trigger").click(); + await page.getByTestId("link-preview-style-rich").click(); + await page.getByTestId("settings-back-to-app").click(); + + const richCard = row.locator( + '[data-link-preview="github-pull-request"][data-link-preview-inline]', + ); + await expect( + richCard.locator("[data-link-preview-image-fallback]"), + ).toBeVisible(); +}); + test("supported Compact link previews keep the message link visible with square outer corners", async ({ page, }) => { diff --git a/desktop/tests/helpers/bridge.ts b/desktop/tests/helpers/bridge.ts index ea6fbc3d25..71b41b1ced 100644 --- a/desktop/tests/helpers/bridge.ts +++ b/desktop/tests/helpers/bridge.ts @@ -298,8 +298,23 @@ type MockBridgeOptions = { description: string | null; imageDataUrl: string | null; imageDomain: string | null; + imageFetchState?: "none" | "image" | "transient_failure" | "rejected"; + imageRetryAfterMs?: number | null; faviconDataUrl?: string | null; } | null; + linkPreviewMetadataByHref?: Record< + string, + { + title: string; + siteName: string | null; + description: string | null; + imageDataUrl: string | null; + imageDomain: string | null; + imageFetchState?: "none" | "image" | "transient_failure" | "rejected"; + imageRetryAfterMs?: number | null; + faviconDataUrl?: string | null; + } | null + >; linkPreviewMetadataDelayMs?: number; searchProfiles?: MockSearchProfileSeed[]; updateAvailable?: boolean;