From 95b79f2dd39dc9c9aa1c97ae277f8cd06f9d4f00 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Mon, 10 Aug 2026 12:29:44 -0700 Subject: [PATCH 1/6] feat(composer): bust a link's negative cache when it re-enters the composer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A URL freshly entering the composer (paste or finished typing) should get a fresh fetch rather than a stale negative cache hit — the user is actively asking for that card now. useResolvedLinkPreviews gains an opt-in refetchNewNegatives mode that invalidates a newly-present href's NEGATIVE cache entry (null/transient-fail) before reading the cache, then refetches. Healthy cached hits and passive message-list scroll stay on the shared cache untouched. Dropping the shared loader entry alone was not enough: the hook retains its own resolvedMetadata state, and the render that scheduled the invalidating effect had already read the stale negative from it. A re-entered transient_failure therefore stayed a snapshotReady fallback the composer could turn into a sendable snapshot tag from stale metadata before the retry resolved. invalidateNegative now reports whether it dropped an entry, and the effect clears the matching local key in step so the re-entry renders as pending until the fresh load wins. Adds a hook-level regression covering transient failure -> URL removed -> re-entered -> pending/no snapshotReady -> successful retry. The shared loader is shared across every hook instance, so a re-entry can coincide with another instance's in-flight fetch for the same canonical URL. invalidateNegative deliberately leaves an in-flight Promise alone, so gating the local-state clear on its return value missed that case, leaving the retained transient_failure as a sendable tag until the shared fetch resolved. The effect now clears this hook's own negative local key for every re-entered href regardless of the shared entry's shape, then coalesces onto any in-flight fetch. Adds a hook-level regression driving the retained-negative + shared in-flight fetch + re-entry interleaving. The composer feeds useResolvedLinkPreviews from DEBOUNCED content, so a fast clear-then-repaste of the same URL inside the 350ms window never commits an empty candidate set — the resolver's newness tracker never saw the URL leave and never refetched, and the stale snapshot tag stayed sendable. useResolvedLinkPreviews now accepts the caller's LIVE hrefs and judges newness against them, so the debounce-swallowed leave/re-entry still forces the refetch. The composer detects the same re-entry at render time (React batches the empty->repaste renders, so an effect keyed on the live set never observes the transition) and blocks the re-entered href until the resolver's forced refetch visibly cycles through pending: its stale tag is dropped from state and excluded from the sendable output, the upload effect and any in-flight upload will not rebuild a tag from the pre-clear metadata, and only a fresh result re-tags. Only the sendable negative case (fallback) is blocked; a healthy re-entry keeps its instant card. Adds a composer-hook regression driving the real hook through the fast gesture: stale tag gone + Send held pending while the deferred refetch is in flight, then a fresh tag carrying the newly-fetched media once it resolves. The `reenteringHrefsRef` phase marker alone did not fence a media upload that was already in flight when the URL re-entered: the upload effect deletes the marker the moment the forced refetch reaches fresh ready metadata, which can happen before the OLD upload settles, so the stale upload's completion then passed the marker guard and published a snapshot tag built from the pre-clear metadata. A secondary hole left the composer tagless: because the old href still occupied `uploadsRef`, the effect skipped starting a fresh upload, and the stale upload's `.finally` cleared the slot without any state change to re-arm one. Both are fixed with a per-href upload generation: `uploadsRef` becomes a Map and a live re-entry bumps `uploadGenerationRef`. The upload effect captures the current generation, its dedup guard compares against it (so a superseded in-flight upload no longer blocks starting the fresh one), and its completion is fenced by a durable generation check independent of the phase marker (so a stale upload becomes a no-op even after the marker was cleared). `.finally` only clears the slot when it still owns the current generation, so it cannot evict the fresh upload's entry. Adds a composer-hook regression that holds the stale upload unresolved across the clear + re-paste and the fresh-metadata resolution, proves a fresh upload starts and its tag wins, then releases the stale upload and proves it cannot publish its pre-clear tag. Co-authored-by: Taylor Ho Signed-off-by: Taylor Ho --- .../ui/useComposerLinkPreviews.test.mjs | 389 ++++++++++++++++ .../messages/ui/useComposerLinkPreviews.tsx | 171 +++++++- .../lib/useResolvedLinkPreviews.test.mjs | 415 +++++++++++++++++- .../src/shared/lib/useResolvedLinkPreviews.ts | 114 ++++- 4 files changed, 1069 insertions(+), 20 deletions(-) create mode 100644 desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs b/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs new file mode 100644 index 0000000000..e145f6af20 --- /dev/null +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs @@ -0,0 +1,389 @@ +import assert from "node:assert/strict"; +import { after, before, test } from "node:test"; + +import { JSDOM } from "jsdom"; + +// ── Composer-hook regression: fast clear + re-paste of the same URL ─────────── +// +// useComposerLinkPreviews feeds useResolvedLinkPreviews from DEBOUNCED content +// (350ms) but tracks URL-presence newness from the LIVE content. Without that +// live-href signal a fast clear-then-repaste of the same URL inside the debounce +// window never commits an empty debounced set, so the resolver never sees the +// URL leave and never refetches — and the stale snapshot tag built from the +// pre-clear metadata stays sendable. This drives the real composer hook through +// that gesture and asserts the stale tag is not sendable and a fresh fetch is +// forced. (PR #5510, second follow-up.) + +const dom = new JSDOM("", { + url: "http://localhost", +}); + +/** @type {Map Promise>} */ +const ipcHandlers = new Map(); + +before(() => { + Object.assign(globalThis, { + document: dom.window.document, + HTMLElement: dom.window.HTMLElement, + IS_REACT_ACT_ENVIRONMENT: true, + window: dom.window, + }); + dom.window.__TAURI_INTERNALS__ = { + invoke: (cmd, args) => { + const handler = ipcHandlers.get(cmd); + return handler + ? handler(args) + : Promise.reject(new Error(`unmocked Tauri command: ${cmd}`)); + }, + transformCallback: () => Math.random(), + }; +}); + +after(() => dom.window.close()); + +const HREF = "https://example.com/composer-re-entry"; +const DEBOUNCE_WAIT_MS = 400; // > LINK_PREVIEW_DEBOUNCE_MS (350) + +function metadata(overrides = {}) { + return { + title: "A story", + siteName: "Example", + description: "Story description", + imageDataUrl: null, + imageDomain: null, + imageFetchState: "none", + imageRetryAfterMs: null, + faviconDataUrl: null, + ...overrides, + }; +} + +test("composer forces a refetch and drops the stale tag on a fast clear+re-paste of the same URL", async () => { + const { act, cleanup, renderHook } = await import("@testing-library/react"); + const { resetLinkPreviewMetadataCache } = await import( + "@/shared/lib/useResolvedLinkPreviews.ts" + ); + const { useComposerLinkPreviews } = await import( + "./useComposerLinkPreviews.tsx" + ); + + resetLinkPreviewMetadataCache(); + ipcHandlers.clear(); + + // Relay origin for media URLs (composer fetches it once on mount). + ipcHandlers.set("get_relay_http_url", () => + Promise.resolve("https://relay.example.com"), + ); + // Media upload always succeeds instantly so a snapshot tag can be built. + ipcHandlers.set("upload_media_bytes", () => + Promise.resolve({ + url: "https://relay.example.com/media/x", + sha256: "deadbeef", + size: 1, + type: "image/png", + uploaded: 0, + }), + ); + // Call 1 (initial paste) resolves to a transient failure -> sendable fallback, + // instantly. Call 2 (the forced re-entry refetch) resolves to a success but + // only when we release `resolveRefetch`, so the test can observe the + // intermediate window where the stale tag is gone and Send is held pending + // BEFORE the fresh result lands (in the app the refetch is a real network + // round-trip; instant resolution would collapse the window under test). + let fetchCalls = 0; + let resolveRefetch; + ipcHandlers.set("fetch_link_preview_metadata", () => { + fetchCalls += 1; + if (fetchCalls === 1) { + return Promise.resolve( + metadata({ + imageFetchState: "transient_failure", + imageRetryAfterMs: 900_000, + }), + ); + } + return new Promise((resolve) => { + resolveRefetch = () => + resolve( + metadata({ + imageDataUrl: "data:image/png;base64,QQ==", + imageDomain: "images.example.com", + imageFetchState: "image", + }), + ); + }); + }); + + const flushDebounceAndSettle = async () => { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, DEBOUNCE_WAIT_MS)); + }); + }; + + try { + const { result, rerender, unmount } = renderHook( + ({ content }) => useComposerLinkPreviews(content), + { initialProps: { content: `see ${HREF}` } }, + ); + + // 1. Paste settles to a transient-failure fallback with a ready snapshot tag. + await flushDebounceAndSettle(); + assert.equal(fetchCalls, 1); + assert.equal( + result.current.getReadyTags().length, + 1, + "the transient fallback produced a sendable snapshot tag", + ); + assert.equal(result.current.hasPendingSnapshots, false); + + // 2. Fast gesture: clear the URL, then re-paste the SAME URL, both before + // the debounce fires. Two separate ticks (as real keystrokes/paste are), + // so the LIVE content commits empty then re-pasted, but the debounced + // `candidates` never commit the empty intermediate. + await act(async () => { + rerender({ content: "see " }); + }); + await act(async () => { + rerender({ content: `see ${HREF}` }); + }); + + // 3a. The stale tag must be gone AS SOON AS the same URL re-enters — this is + // the core invariant and it holds synchronously (render-time detection + // drops the tag and excludes the href from sendable output), so no timer + // needs to fire first. Send is held pending until a fresh tag lands. + await act(async () => {}); + assert.equal( + result.current.getReadyTags().length, + 0, + "stale snapshot tag must not be sendable the moment the URL re-enters", + ); + assert.equal( + result.current.hasPendingSnapshots, + true, + "Send must be held pending after a clear+re-paste", + ); + + // 3b. Once the debounce settles, the re-entry has forced a fresh fetch. It is + // still in flight (the deferred refetch has NOT resolved), so the stale + // tag stays gone and Send stays pending. Pre-fix the re-entry is + // invisible, so no refetch starts and this fails fast. + await flushDebounceAndSettle(); + assert.equal( + fetchCalls, + 2, + "the re-entry forced a fresh fetch (still in flight)", + ); + assert.equal( + result.current.getReadyTags().length, + 0, + "stale snapshot tag must not be sendable while the re-entry refetches", + ); + assert.equal( + result.current.hasPendingSnapshots, + true, + "Send must be held pending while the re-entry refetches", + ); + + // 4. The forced refetch resolves (success); a fresh tag becomes sendable and + // its media is the freshly-fetched image, not the pre-clear empty + // fallback (proving the tag was rebuilt from new metadata). + await act(async () => { + resolveRefetch(); + }); + await flushDebounceAndSettle(); + const [freshTag] = result.current.getReadyTags(); + assert.equal( + result.current.getReadyTags().length, + 1, + "a fresh sendable tag lands after the refetch", + ); + assert.ok( + freshTag?.includes("https://relay.example.com/media/x"), + "the sendable tag carries the freshly-fetched snapshot media", + ); + assert.equal(result.current.hasPendingSnapshots, false); + + unmount(); + } finally { + cleanup(); + ipcHandlers.clear(); + } +}); + +// ── Composer-hook regression: stale in-flight upload after re-entry ─────────── +// +// A pre-clear transient-fallback snapshot upload (U1) can still be in flight +// when the URL is cleared and re-pasted. The re-entry forces a refetch to fresh +// metadata, and the upload effect starts a fresh upload (U2). When the stale U1 +// finally settles it must NOT publish a snapshot tag built from the pre-clear +// metadata — even though its re-entry phase marker was already cleared once the +// refetch reached fresh ready. The per-href upload generation fence makes the +// stale completion a no-op, and the generation-aware dedup guard lets U2 start +// even while U1's slot is still occupied. Reverting either the generation fence +// in `.then` or the generation-aware dedup guard makes this fail (U1 publishes +// its stale favicon, or U2 never starts). (PR #5510, third follow-up.) +test("a stale in-flight upload cannot publish after the URL re-enters and a fresh upload wins", async () => { + const { act, cleanup, renderHook } = await import("@testing-library/react"); + const { resetLinkPreviewMetadataCache } = await import( + "@/shared/lib/useResolvedLinkPreviews.ts" + ); + const { useComposerLinkPreviews } = await import( + "./useComposerLinkPreviews.tsx" + ); + + resetLinkPreviewMetadataCache(); + ipcHandlers.clear(); + + ipcHandlers.set("get_relay_http_url", () => + Promise.resolve("https://relay.example.com"), + ); + + // Gate the FIRST media upload (U1's favicon — its image is null in the + // transient fallback, so the favicon is U1's only upload) so it stays in + // flight across the clear + re-paste and the fresh-metadata resolution. Every + // later upload resolves instantly with a "fresh" URL. If a stale tag ever + // reaches the sendable set it will carry the STALE favicon URL, which the + // assertions forbid. + let uploadCalls = 0; + let releaseStaleUpload; + ipcHandlers.set("upload_media_bytes", () => { + uploadCalls += 1; + if (uploadCalls === 1) { + return new Promise((resolve) => { + releaseStaleUpload = () => + resolve({ + url: "https://relay.example.com/media/STALE", + sha256: "5741313", + size: 1, + type: "image/png", + uploaded: 0, + }); + }); + } + return Promise.resolve({ + url: "https://relay.example.com/media/FRESH", + sha256: "f8e5", + size: 1, + type: "image/png", + uploaded: 0, + }); + }); + + // Call 1 (initial paste): transient failure WITH a favicon, so it produces a + // sendable fallback whose upload (the favicon) is the gated U1. Call 2 (the + // forced re-entry refetch): a full success that only resolves when released, + // so the intermediate window (U1 in flight, refetch pending) is observable. + let fetchCalls = 0; + let resolveRefetch; + ipcHandlers.set("fetch_link_preview_metadata", () => { + fetchCalls += 1; + if (fetchCalls === 1) { + return Promise.resolve( + metadata({ + faviconDataUrl: "data:image/png;base64,QQ==", + imageFetchState: "transient_failure", + imageRetryAfterMs: 900_000, + }), + ); + } + return new Promise((resolve) => { + resolveRefetch = () => + resolve( + metadata({ + faviconDataUrl: "data:image/png;base64,Qg==", + imageDataUrl: "data:image/png;base64,Qw==", + imageDomain: "images.example.com", + imageFetchState: "image", + }), + ); + }); + }); + + const flushDebounceAndSettle = async () => { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, DEBOUNCE_WAIT_MS)); + }); + }; + + try { + const { result, rerender, unmount } = renderHook( + ({ content }) => useComposerLinkPreviews(content), + { initialProps: { content: `see ${HREF}` } }, + ); + + // 1. Paste settles to a transient-failure fallback. Its favicon upload (U1) + // is gated and stays in flight, so no sendable tag exists yet. + await flushDebounceAndSettle(); + assert.equal(fetchCalls, 1); + assert.equal( + uploadCalls, + 1, + "U1 (the stale fallback favicon) is in flight", + ); + assert.equal( + result.current.getReadyTags().length, + 0, + "U1 has not settled, so no snapshot tag is sendable yet", + ); + + // 2. Fast gesture: clear then re-paste the SAME URL inside the debounce. + await act(async () => { + rerender({ content: "see " }); + }); + await act(async () => { + rerender({ content: `see ${HREF}` }); + }); + await act(async () => {}); + + // 3. The re-entry forces a fresh fetch; resolve it to fresh success. The + // upload effect must start a FRESH upload (U2) even though U1 still holds + // the slot, then produce a fresh sendable tag. + await flushDebounceAndSettle(); + assert.equal(fetchCalls, 2, "the re-entry forced a fresh fetch"); + await act(async () => { + resolveRefetch(); + }); + await flushDebounceAndSettle(); + assert.ok( + uploadCalls >= 2, + "a fresh upload (U2) started despite U1 still holding the slot", + ); + const [freshTag] = result.current.getReadyTags(); + assert.equal( + result.current.getReadyTags().length, + 1, + "the fresh upload produced a sendable tag", + ); + assert.ok( + freshTag?.includes("https://relay.example.com/media/FRESH"), + "the sendable tag carries the freshly-uploaded media", + ); + assert.ok( + !freshTag?.includes("https://relay.example.com/media/STALE"), + "the sendable tag must not carry the stale pre-clear media", + ); + + // 4. Release the stale U1. Its completion must be a no-op: it cannot + // overwrite the fresh tag with one built from pre-clear metadata. + await act(async () => { + releaseStaleUpload(); + }); + await flushDebounceAndSettle(); + const [tagAfterStale] = result.current.getReadyTags(); + assert.equal( + result.current.getReadyTags().length, + 1, + "still exactly one sendable tag after the stale upload settles", + ); + assert.ok( + tagAfterStale?.includes("https://relay.example.com/media/FRESH") && + !tagAfterStale?.includes("https://relay.example.com/media/STALE"), + "the stale upload cannot publish its pre-clear tag after settling", + ); + + unmount(); + } finally { + cleanup(); + ipcHandlers.clear(); + } +}); diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx index 3f251a719d..1b84d95bfb 100644 --- a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx @@ -234,8 +234,18 @@ export function useComposerLinkPreviews(content: string, enabled = true) { liveCandidatesRef.current = extractCandidates(content).map( (preview) => preview.href, ); + // A URL freshly entering the composer (paste, or finishing typing one) should + // get a fresh fetch rather than a stale negative cache hit — the user is + // actively asking for this link's card now. useResolvedLinkPreviews handles + // the timing (invalidate a newly-present href's NEGATIVE cache entry before + // it reads the cache); healthy hits and passive message-list scroll are + // untouched, so the shared cache still does its job everywhere else. Pass the + // LIVE hrefs for newness tracking so a fast clear-then-repaste of the same URL + // within the debounce window (which never commits an empty `candidates`) is + // still seen as a re-entry and refetched — not served the stale negative. const resolvedPreviews = useResolvedLinkPreviews( suppressed ? [] : candidates, + { refetchNewNegatives: true, liveHrefs: liveCandidatesRef.current }, ); // Entity links resolve to null metadata when the relay lookup has nothing // for them; keep their safe fallback cards rather than dropping them. @@ -258,7 +268,16 @@ export function useComposerLinkPreviews(content: string, enabled = true) { readyTagsByHrefRef.current = readyTags; const suppressedRef = React.useRef(suppressed); suppressedRef.current = suppressed; - const uploadsRef = React.useRef(new Set()); + const uploadsRef = React.useRef(new Map()); + // Per-href upload generation. Bumped whenever a stale-negative href re-enters + // (below), so an upload started before a re-entry can be recognized as stale + // when it settles and dropped without publishing its pre-re-entry tag — the + // `reenteringHrefsRef` phase marker alone is not enough, since it is cleared + // the moment fresh metadata arrives, which can be before the OLD upload + // resolves. Keyed uploads also let a fresh upload start while a superseded one + // is still in flight (its generation no longer matches), so the composer is + // never left tagless waiting on a doomed upload. + const uploadGenerationRef = React.useRef(new Map()); const activeHrefsRef = React.useRef(new Set()); activeHrefsRef.current = new Set(candidates.map((preview) => preview.href)); @@ -279,15 +298,110 @@ export function useComposerLinkPreviews(content: string, enabled = true) { ); }, [candidates]); + // Detect live-content presence transitions at render time (not in an effect): + // a fast clear-then-repaste of the same URL commits both the empty and the + // re-pasted render in one batch, so an effect keyed on the live set only ever + // observes the unchanged final value and never fires. Comparing against the + // previous render's live set catches the re-entry synchronously. A re-entered + // href drops its stale ready tag (built from the pre-clear metadata) so it is + // no longer sendable until the forced refetch produces a fresh one; the + // resolver, fed the same live hrefs, refetches it in step. Without this a + // clear+repaste within the 350ms debounce could ship a snapshot tag from the + // stale metadata. + const prevLiveHrefsRef = React.useRef>(new Set()); + // Hrefs that just re-entered while carrying a STALE negative fallback the + // resolver will refetch. The upload effect must not rebuild a snapshot tag + // from that stale metadata (its `previews` entry still carries the pre-clear + // `snapshotReady` fallback until the resolver's refetch commits the pending + // state a render later, then re-resolves). A blocked href is released only + // after the resolver visibly takes over — the preview goes pending ("blocked" + // -> "refetching") and then resolves ready again — so a fresh result (image + // OR a genuinely-new fallback) can tag, but the stale pre-clear fallback that + // merely lingers a render or two cannot. Only the sendable negative case + // (`imageState === "fallback"`) is ever blocked; a healthy (`"image"`) + // re-entry is kept by the resolver, never goes pending, and must not block or + // it would trap Send forever. + const reenteringHrefsRef = React.useRef< + Map + >(new Map()); + const reenteredLiveHrefs = liveCandidatesRef.current.filter( + (href) => !prevLiveHrefsRef.current.has(href), + ); + prevLiveHrefsRef.current = new Set(liveCandidatesRef.current); + if (reenteredLiveHrefs.length > 0) { + const staleReentered = reenteredLiveHrefs.filter( + (href) => + !reenteringHrefsRef.current.has(href) && + previews.some( + (preview) => + preview.href === href && + preview.snapshotReady && + preview.imageState === "fallback", + ), + ); + for (const href of staleReentered) { + reenteringHrefsRef.current.set(href, "blocked"); + // Bump the upload generation so any upload started before this re-entry + // (built from the now-stale pre-clear metadata) is recognized as stale + // when it settles and cannot publish its tag. + uploadGenerationRef.current.set( + href, + (uploadGenerationRef.current.get(href) ?? 0) + 1, + ); + } + if (staleReentered.some((href) => readyTagsByHrefRef.current[href])) { + // Drop the stale ready tag from state so (a) it stops being sendable and + // (b) the upload effect will rebuild it once the block releases with fresh + // metadata (its `readyTags[href]` guard would otherwise keep skipping). + // Functional updater so it survives the ref resync at the top of the next + // render. The read-time filter below also excludes blocked hrefs, so a + // synchronous submit in THIS render cannot ship the stale tag either. + const drop = new Set(staleReentered); + queueMicrotask(() => + setReadyTags((current) => { + let changed = false; + const next = { ...current }; + for (const href of drop) + if (href in next) { + delete next[href]; + changed = true; + } + return changed ? next : current; + }), + ); + } + } + React.useEffect(() => { for (const preview of previews) { + // A re-entering href stays blocked until the resolver's forced refetch has + // visibly cycled through pending: seeing `!snapshotReady` (pending) marks + // "refetching"; only once it is ready AGAIN after that is the block lifted + // and a tag built from the fresh metadata. The stale pre-clear fallback + // (still `snapshotReady` and never pending) can never rebuild the tag. + const phase = reenteringHrefsRef.current.get(preview.href); + if (phase !== undefined) { + if (!preview.snapshotReady) { + reenteringHrefsRef.current.set(preview.href, "refetching"); + continue; + } + if (phase === "blocked") continue; + reenteringHrefsRef.current.delete(preview.href); + } + // The generation captured here fences this upload's completion: a live + // re-entry bumps `uploadGenerationRef` (above), so an in-flight upload + // started from stale pre-clear metadata carries an older generation and + // its `.then` (below) becomes a no-op. The dedup guard is generation-aware + // too, so a superseded in-flight upload does not block starting the fresh + // one at the new generation. + const generation = uploadGenerationRef.current.get(preview.href) ?? 0; if ( !preview.snapshotReady || readyTags[preview.href] || - uploadsRef.current.has(preview.href) + uploadsRef.current.get(preview.href) === generation ) continue; - uploadsRef.current.add(preview.href); + uploadsRef.current.set(preview.href, generation); // Upload image and favicon independently so one failure degrades to the // surviving media instead of dropping the whole preview. A snapshot tag // with empty media fields is valid (renders as text + favicon, or @@ -307,6 +421,20 @@ export function useComposerLinkPreviews(content: string, enabled = true) { ]) .then(([image, favicon]) => { if (!activeHrefsRef.current.has(preview.href)) return; + // If this href re-entered while the upload was in flight, its metadata + // is stale (the resolver is refetching). Drop the result rather than + // writing back a snapshot tag built from the pre-re-entry metadata; + // the forced refetch's own upload will produce the fresh tag. + if (reenteringHrefsRef.current.has(preview.href)) return; + // Durable generation fence, independent of the phase marker: if this + // href re-entered while the upload was in flight, its generation was + // bumped, so this stale completion is dropped even if the marker has + // already been cleared (e.g. the forced refetch reached fresh ready + // and the effect deleted the marker before U1 settled). + if ( + (uploadGenerationRef.current.get(preview.href) ?? 0) !== generation + ) + return; const failedMedia = [image.failed, favicon.failed].filter( (label): label is "thumbnail" | "favicon" => label !== null, ); @@ -335,7 +463,12 @@ export function useComposerLinkPreviews(content: string, enabled = true) { setReadyTags((current) => ({ ...current, [preview.href]: tag })); }) .finally(() => { - uploadsRef.current.delete(preview.href); + // Only clear the slot if this upload is still the current one for the + // href. A superseded upload (older generation) must not delete the + // entry belonging to the fresh upload (U2) that replaced it, or the + // dedup guard would let a third upload start and race again. + if (uploadsRef.current.get(preview.href) === generation) + uploadsRef.current.delete(preview.href); }); void uploadPromise; } @@ -344,7 +477,10 @@ export function useComposerLinkPreviews(content: string, enabled = true) { readyTagsRef.current = suppressed ? [["link-preview", "none"]] : candidates.flatMap((candidate) => - readyTags[candidate.href] ? [readyTags[candidate.href]] : [], + readyTags[candidate.href] && + !reenteringHrefsRef.current.has(candidate.href) + ? [readyTags[candidate.href]] + : [], ); // A preview is "settling" from paste until its sendable tag exists: metadata // is still resolving, or it resolved and the snapshot media is uploading. @@ -433,18 +569,19 @@ export function useComposerLinkPreviews(content: string, enabled = true) { // Snapshot tags for a submit, read synchronously at submit start from the // LIVE candidate set (liveCandidatesRef) via `selectSubmitTags` — so the tags // always correspond to the content actually being sent, never a debounced set - // that still holds a just-removed URL. No await: Send is disabled until every - // settling preview has its tag (or the anti-trap cap fires), so at submit time - // the tags that will ever exist already exist. - const getReadyTags = React.useCallback( - () => - selectSubmitTags( - liveCandidatesRef.current, - readyTagsByHrefRef.current, - suppressedRef.current, - ), - [], - ); + // that still holds a just-removed URL. Re-entering hrefs are excluded: their + // retained tag was built from stale metadata the resolver is refetching, and + // it must not ship until a fresh tag replaces it. No await: Send is disabled + // until every settling preview has its tag (or the anti-trap cap fires), so at + // submit time the tags that will ever exist already exist. + const getReadyTags = React.useCallback(() => { + const reentering = reenteringHrefsRef.current; + return selectSubmitTags( + liveCandidatesRef.current.filter((href) => !reentering.has(href)), + readyTagsByHrefRef.current, + suppressedRef.current, + ); + }, []); return { previewList, getReadyTags, diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs b/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs index c0c91d9f16..a34806ffa0 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs @@ -1,10 +1,13 @@ import assert from "node:assert/strict"; -import test from "node:test"; +import { after, before, test } from "node:test"; + +import { JSDOM } from "jsdom"; import { __linkPreviewMetadataTest, fetchBuzzEntityMetadata, isBuzzEntityPreview, + resetLinkPreviewMetadataCache, resolveLinkPreview, withEntityFallbacks, } from "./useResolvedLinkPreviews.ts"; @@ -432,3 +435,413 @@ test("Buzz repository metadata stays image-less and exposes default branch", asy assert.equal(result?.imageDataUrl, null); assert.equal(result?.imageDomain, null); }); + +test("invalidateNegative drops a cached null miss so the next load refetches", async () => { + // A URL freshly entering the composer clears a stale hard miss (null) so it + // refetches, instead of riding the cached blank. + const now = 1_000; + let calls = 0; + let nextResult = null; + const loader = __linkPreviewMetadataTest.createMetadataLoader({ + fetcher: async () => { + calls += 1; + return nextResult; + }, + now: () => now, + }); + + assert.equal((await loader.load(preview.href)).metadata, null); + assert.equal(calls, 1); + + loader.invalidateNegative(preview.href); + nextResult = metadata(); + assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); + assert.equal(calls, 2); +}); + +test("invalidateNegative drops a cached transient failure so the next load refetches", async () => { + // A transient image failure is a NEGATIVE by contract (option docs + the + // loader's own retry boundary), so re-entering the composer must refetch it + // — not reuse the cached transient entry. Regression for the leak where + // invalidateNegative only cleared hard `null` misses. (PR #5510) + const 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); + + // Bust well before the retry boundary (now is frozen); the cache-bust — not + // the cooldown — is what forces the refetch. + loader.invalidateNegative(preview.href); + assert.equal( + (await loader.load(preview.href)).metadata?.imageFetchState, + "image", + ); + assert.equal(calls, 2); +}); + +test("invalidateNegative leaves a healthy cached hit untouched", async () => { + // A settled positive (instant card, no redundant fetch) must survive a bust + // so passive scroll re-renders keep riding the cache. + const now = 1_000; + let calls = 0; + const loader = __linkPreviewMetadataTest.createMetadataLoader({ + fetcher: async () => { + calls += 1; + return metadata(); + }, + now: () => now, + }); + + assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); + assert.equal(calls, 1); + + loader.invalidateNegative(preview.href); + assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); + assert.equal(calls, 1); +}); + +test("invalidateNegative leaves an in-flight fetch untouched", async () => { + // A fetch still in flight is cached as a Promise, not a resolved entry. + // Busting mid-flight must not cancel or duplicate it: the pending load + // resolves normally and no second fetch is started. + const now = 1_000; + let calls = 0; + let releaseFetch; + const loader = __linkPreviewMetadataTest.createMetadataLoader({ + fetcher: () => { + calls += 1; + return new Promise((resolve) => { + releaseFetch = () => resolve(metadata()); + }); + }, + now: () => now, + }); + + const pending = loader.load(preview.href); + assert.equal(calls, 1); + + // Bust while the fetch is still in flight — the Promise entry is left alone. + loader.invalidateNegative(preview.href); + assert.equal(calls, 1, "no redundant fetch started by the bust"); + + releaseFetch(); + assert.deepEqual((await pending).metadata, metadata()); + assert.equal(calls, 1, "the original in-flight fetch resolved, not a retry"); +}); + +// ── Hook-level regression: retained resolved-state invalidation on re-entry ─── +// +// The loader tests above prove `invalidateNegative` drops the SHARED loader +// cache entry. But `useResolvedLinkPreviews` also retains its OWN +// `resolvedMetadata` React state, and the render that scheduled the +// invalidating effect has already read the stale negative from it. Dropping the +// loader key alone leaves that local key in place, so a re-entered +// `transient_failure` still resolves to a `snapshotReady` fallback the composer +// can turn into a sendable snapshot tag from STALE metadata before the retry +// lands. This drives the REAL hook to prove the local key is cleared too, so +// the re-entry renders as pending (no `snapshotReady`) until the retry wins. +// (PR #5510) Regression: without the local-state clear the re-entry assertion +// below sees `snapshotReady: true` instead of pending. + +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, + }); + // @tauri-apps/api/core reads window.__TAURI_INTERNALS__.invoke at call time. + // A per-test handler map lets each test control the fetch resolution timing. + dom.window.__TAURI_INTERNALS__ = { + invoke: (cmd, args) => { + const handler = ipcHandlers.get(cmd); + return handler + ? handler(args) + : Promise.reject(new Error(`unmocked Tauri command: ${cmd}`)); + }, + transformCallback: () => Math.random(), + }; +}); + +after(() => dom.window.close()); + +/** @type {Map Promise>} */ +const ipcHandlers = new Map(); + +const hookPreview = { + kind: "generic-link", + href: "https://example.com/hook-re-entry", + provider: "example.com", + title: "example.com/hook-re-entry", + typeLabel: "link", +}; + +test("hook clears retained resolved state so a re-entered transient failure renders pending until the retry wins", async () => { + const { act, cleanup, renderHook } = await import("@testing-library/react"); + const { useResolvedLinkPreviews } = await import( + "./useResolvedLinkPreviews.ts" + ); + + // Isolate from any loader state leaked by earlier tests in this process. + resetLinkPreviewMetadataCache(); + ipcHandlers.clear(); + + // Gate every fetch so we can observe the state between renders. Call 1 caches + // a transient failure; call 2 (the re-entry retry) succeeds. + let calls = 0; + const releases = []; + ipcHandlers.set("fetch_link_preview_metadata", () => { + calls += 1; + const attempt = calls; + return new Promise((resolve) => { + releases.push(() => + resolve( + attempt === 1 + ? metadata({ + imageFetchState: "transient_failure", + imageRetryAfterMs: 900_000, + }) + : metadata({ + imageDataUrl: "data:image/jpeg;base64,abc", + imageDomain: "images.example.com", + imageFetchState: "image", + }), + ), + ); + }); + }); + + // scheduleAfterPaint uses requestAnimationFrame -> setTimeout(0); flush both + // plus a microtask turn so the queued load() actually fires. + const flushScheduledLoads = async () => { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + }; + + const settle = async () => { + await act(async () => { + // Release any pending fetch and let its .then() commit setResolvedMetadata. + while (releases.length > 0) releases.shift()(); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + }; + + try { + const { result, rerender, unmount } = renderHook( + ({ previews }) => + useResolvedLinkPreviews(previews, { refetchNewNegatives: true }), + { initialProps: { previews: [hookPreview] } }, + ); + + // 1. Initial paste: pending until the fetch resolves to a transient failure. + assert.equal(result.current[0].imageState, "pending"); + assert.equal(result.current[0].snapshotReady, undefined); + + await flushScheduledLoads(); + await settle(); + + // Transient failure cached: a sendable fallback (this is the state that must + // NOT survive re-entry as ready). + assert.equal(result.current[0].imageState, "fallback"); + assert.equal(result.current[0].snapshotReady, true); + assert.equal(calls, 1); + + // 2. URL removed from the composer. + rerender({ previews: [] }); + assert.deepEqual(result.current, []); + + // 3. URL re-entered. The invalidation effect must drop BOTH the loader entry + // and the retained local key, so this settles to pending with no + // snapshotReady — not the stale fallback. Without the local-state clear + // the hook returns snapshotReady: true here. + rerender({ previews: [hookPreview] }); + await act(async () => {}); + assert.equal( + result.current[0].imageState, + "pending", + "re-entered transient failure must render pending, not a stale fallback", + ); + assert.equal( + result.current[0].snapshotReady, + undefined, + "no snapshotReady before the retry resolves — nothing sendable from stale metadata", + ); + + // 4. Successful retry wins. + await flushScheduledLoads(); + await settle(); + assert.equal(result.current[0].imageState, "image"); + assert.equal(result.current[0].snapshotReady, true); + assert.equal(calls, 2, "the re-entry triggered a fresh fetch"); + + unmount(); + } finally { + cleanup(); + ipcHandlers.clear(); + } +}); + +// ── Hook-level regression: in-flight shared entry defeats the local clear ───── +// +// The shared metadataLoader is intentionally shared across every hook instance +// (composer + message list). `invalidateNegative` deliberately leaves an +// in-flight Promise entry alone — but that means when a URL re-enters the +// composer WHILE another instance has a fetch in flight for the same canonical +// URL, the shared drop is a no-op. Gating the local-state clear on that drop +// (the earlier fix) leaves this hook's retained `transient_failure` in place, +// so it keeps returning `snapshotReady: true` — a sendable tag built from stale +// metadata — until that shared fetch resolves. This drives the real hook +// through that exact interleaving to prove the local negative is cleared on +// re-entry regardless of the shared entry's shape. (PR #5510, follow-up.) + +const otherHookPreview = { + kind: "generic-link", + href: "https://example.com/hook-inflight-re-entry", + provider: "example.com", + title: "example.com/hook-inflight-re-entry", + typeLabel: "link", +}; + +test("hook clears a re-entered negative even when the shared cache holds an in-flight fetch, not a settled entry", async () => { + const { act, cleanup, renderHook } = await import("@testing-library/react"); + const { useResolvedLinkPreviews } = await import( + "./useResolvedLinkPreviews.ts" + ); + + resetLinkPreviewMetadataCache(); + ipcHandlers.clear(); + + // Gate every fetch. Call 1 (composer's initial paste) resolves to a transient + // failure. Call 2 is the SHARED in-flight fetch a message-list instance starts + // after the shared cache is dropped; it is left unreleased so it is still a + // Promise in the shared cache when the composer re-enters. + let calls = 0; + const releases = []; + ipcHandlers.set("fetch_link_preview_metadata", () => { + calls += 1; + const attempt = calls; + return new Promise((resolve) => { + releases.push(() => + resolve( + attempt === 1 + ? metadata({ + imageFetchState: "transient_failure", + imageRetryAfterMs: 900_000, + }) + : metadata({ + imageDataUrl: "data:image/jpeg;base64,abc", + imageDomain: "images.example.com", + imageFetchState: "image", + }), + ), + ); + }); + }); + + const flushScheduledLoads = async () => { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + }; + + const releaseAll = async () => { + await act(async () => { + while (releases.length > 0) releases.shift()(); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + }; + + const composer = renderHook( + ({ previews }) => + useResolvedLinkPreviews(previews, { refetchNewNegatives: true }), + { initialProps: { previews: [otherHookPreview] } }, + ); + // A passive message-list instance sharing the same loader (no refetch). + const messageList = renderHook( + ({ previews }) => useResolvedLinkPreviews(previews, {}), + { initialProps: { previews: [] } }, + ); + + try { + // 1. Composer paste resolves to a transient failure -> sendable fallback. + await flushScheduledLoads(); + await act(async () => { + releases.shift()(); // release call 1 only + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + assert.equal(composer.result.current[0].imageState, "fallback"); + assert.equal(composer.result.current[0].snapshotReady, true); + assert.equal(calls, 1); + + // 2. Composer URL leaves. + composer.rerender({ previews: [] }); + assert.deepEqual(composer.result.current, []); + + // 3. The shared negative is dropped (simulating expiry/eviction), then a + // message-list instance starts a fresh fetch for the SAME canonical URL + // and it is left IN FLIGHT — a Promise, not a settled entry, in the + // shared cache. + act(() => { + resetLinkPreviewMetadataCache(); + }); + messageList.rerender({ previews: [otherHookPreview] }); + await flushScheduledLoads(); + assert.equal(calls, 2, "message-list started the shared in-flight fetch"); + + // 4. Composer re-enters WHILE that fetch is in flight. invalidateNegative is + // a no-op (the shared entry is a Promise), so the earlier drop-gated clear + // would leave the retained transient failure — a sendable tag from stale + // metadata. The local negative must be cleared regardless: pending, no + // snapshotReady. + composer.rerender({ previews: [otherHookPreview] }); + await act(async () => {}); + assert.equal( + composer.result.current[0].imageState, + "pending", + "re-entry during a shared in-flight fetch must render pending, not a stale fallback", + ); + assert.equal( + composer.result.current[0].snapshotReady, + undefined, + "no snapshotReady while the shared fetch is in flight — nothing sendable from stale metadata", + ); + // The composer coalesced onto the in-flight fetch — no third request. + assert.equal(calls, 2, "re-entry coalesced onto the in-flight fetch"); + + // 5. The shared fetch resolves successfully; both instances settle. + await releaseAll(); + await flushScheduledLoads(); + assert.equal(composer.result.current[0].imageState, "image"); + assert.equal(composer.result.current[0].snapshotReady, true); + } finally { + composer.unmount(); + messageList.unmount(); + cleanup(); + ipcHandlers.clear(); + } +}); diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.ts b/desktop/src/shared/lib/useResolvedLinkPreviews.ts index 5f2a5535b5..995777472f 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.ts +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.ts @@ -86,6 +86,13 @@ function metadataCacheKey(href: string): string { } } +function isNegativeMetadata(metadata: LinkPreviewMetadata | null): boolean { + // A cached NEGATIVE is a result that should be retried when the URL freshly + // re-enters the composer: a hard miss (null) or a transient image failure. + // A healthy hit (`image`/`rejected`/no state) is a settled positive. + return metadata === null || metadata.imageFetchState === "transient_failure"; +} + function metadataExpiry( metadata: LinkPreviewMetadata | null, now: number, @@ -186,6 +193,23 @@ function createMetadataLoader({ deleteKey(key: string) { cache.delete(key); }, + /** + * Drop a cached NEGATIVE result (a resolved null or a transient failure) so + * the next load refetches. Used when a URL freshly enters the composer: a + * user pasting a link that previously blanked should get a new attempt now, + * not the stale miss. A healthy cached hit and an in-flight fetch are left + * untouched, so passive scroll re-renders still ride the cache as before. + * Returns whether a negative entry was actually dropped, so callers can + * invalidate their own derived state (e.g. retained React metadata) in step. + */ + invalidateNegative(href: string): boolean { + const key = metadataCacheKey(href); + const cached = cache.get(key); + if (!cached || cached instanceof Promise) return false; + if (!isNegativeMetadata(cached.metadata)) return false; + cache.delete(key); + return true; + }, load, peek, reset() { @@ -417,16 +441,102 @@ export function withEntityFallbacks( export function useResolvedLinkPreviews( previews: SupportedLinkPreview[], + { + refetchNewNegatives = false, + liveHrefs, + }: { + /** + * When a preview href is newly present since the last run, drop any cached + * NEGATIVE (null/transient-fail) metadata for it so it refetches instead of + * resolving to a stale miss. Used by the composer: a freshly pasted link + * should get a new attempt. Off by default so passive renders (the message + * list) keep riding the cache. Healthy cached hits are never invalidated. + */ + refetchNewNegatives?: boolean; + /** + * The hrefs present in the caller's LIVE (undebounced) content. When given, + * newness is judged against this set instead of the resolved `previews`, so + * a URL that leaves and re-enters the live content is treated as re-entered + * even when a debounce swallowed the intermediate empty state (the composer + * debounces resolution, so `previews` may never observe the URL leaving). + * Resolution timing still follows `previews`; only the invalidation decision + * uses this. Omit to track newness against `previews` (the default). + */ + liveHrefs?: readonly string[]; + } = {}, ): ResolvedLinkPreview[] { const [resolvedMetadata, setResolvedMetadata] = React.useState({}); const [retryGeneration, setRetryGeneration] = React.useState(0); - + const seenHrefsRef = React.useRef>(new Set()); + // Newness is tracked against the live href set when the caller supplies one, + // so a debounce-swallowed leave/re-entry of the same URL still counts as new. + // Read through a ref inside the effect and drive re-runs off the stable string + // key, so an unstable per-render array does not have to be an effect dep. + const currentHrefs = liveHrefs ?? previews.map((preview) => preview.href); + const newnessKey = currentHrefs.join("\n"); + const currentHrefsRef = React.useRef(currentHrefs); + currentHrefsRef.current = currentHrefs; + + // biome-ignore lint/correctness/useExhaustiveDependencies: newnessKey is the stable string key for the live href set read via currentHrefsRef; it drives the re-run so the per-render array need not be a dep. React.useEffect(() => { let cancelled = false; let retryAt = Number.POSITIVE_INFINITY; let retryTimer: ReturnType | null = null; + if (refetchNewNegatives) { + // Invalidate first, before the peek/load loop below reads the cache, so a + // newly-present href loads fresh instead of resolving to its stale miss. + // buzz:// entity links resolve off the relay, not this cache — skip them. + // Newness is judged against the live href set when supplied (so a + // debounce-swallowed leave/re-entry still counts), else against previews. + const seen = seenHrefsRef.current; + const liveNow = currentHrefsRef.current; + const next = new Set(liveNow); + const reenteredKeys: string[] = []; + for (const preview of previews) { + if ( + seen.has(preview.href) || + !liveNow.includes(preview.href) || + preview.href.startsWith("buzz://") + ) { + continue; + } + // Drop any settled NEGATIVE from the SHARED loader cache so the load + // below refetches instead of resolving to the stale miss. (A no-op when + // the shared entry is healthy, in-flight, or absent.) + metadataLoader.invalidateNegative(preview.href); + reenteredKeys.push(metadataCacheKey(preview.href)); + } + seenHrefsRef.current = next; + // Dropping the loader entry alone is not enough: this hook retains its own + // resolved metadata, and the render that scheduled this effect already + // read the stale negative from it. Clear this hook's OWN negative key for + // every re-entered href — gating on whether the shared loader dropped a + // settled entry misses the case where another hook left an in-flight + // Promise in the shared cache (invalidateNegative leaves Promises alone + // and the loop below merely coalesces onto it), which would otherwise + // keep this hook's retained `transient_failure` as a `snapshotReady` + // fallback the composer could turn into a sendable snapshot tag from stale + // metadata until that fetch resolves. Clearing the local negative renders + // the re-entered link as pending until the fresh load wins. Healthy local + // hits are kept, so passive re-renders still show their card instantly. + if (reenteredKeys.length > 0) { + setResolvedMetadata((current) => { + let changed = false; + const nextMetadata = { ...current }; + for (const key of reenteredKeys) { + const value = nextMetadata[key]; + if (value !== undefined && isNegativeMetadata(value)) { + delete nextMetadata[key]; + changed = true; + } + } + return changed ? nextMetadata : current; + }); + } + } + const scheduleRetry = ( { expiresAt, key }: Pick, loader: typeof metadataLoader, @@ -485,7 +595,7 @@ export function useResolvedLinkPreviews( for (const cancel of cancelScheduledLoads) cancel(); if (retryTimer !== null) clearTimeout(retryTimer); }; - }, [previews, retryGeneration]); + }, [previews, refetchNewNegatives, retryGeneration, newnessKey]); return React.useMemo( () => From 18e119d2b3c0cd1156e63fb9b6e35ed4f57ef5bd Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Thu, 13 Aug 2026 10:37:56 -0700 Subject: [PATCH 2/6] fix(link-preview): preserve paste newness through debounce Co-authored-by: Taylor Ho Signed-off-by: Taylor Ho --- .../ui/useComposerLinkPreviews.test.mjs | 93 +++++++++++++++++++ .../src/shared/lib/useResolvedLinkPreviews.ts | 25 +++-- 2 files changed, 108 insertions(+), 10 deletions(-) diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs b/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs index e145f6af20..4fa736aebc 100644 --- a/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs @@ -387,3 +387,96 @@ test("a stale in-flight upload cannot publish after the URL re-enters and a fres ipcHandlers.clear(); } }); + +// The ordinary production gesture starts from a mounted empty composer. Live +// href tracking must not mark the pasted href handled before the debounced +// candidate exists, or the eventual resolver pass will reuse a cached negative. +test("composer refetches a cached negative when a link is pasted into an empty draft", async () => { + const { act, cleanup, renderHook } = await import("@testing-library/react"); + const { resetLinkPreviewMetadataCache } = await import( + "@/shared/lib/useResolvedLinkPreviews.ts" + ); + const { useComposerLinkPreviews } = await import( + "./useComposerLinkPreviews.tsx" + ); + + resetLinkPreviewMetadataCache(); + ipcHandlers.clear(); + ipcHandlers.set("get_relay_http_url", () => + Promise.resolve("https://relay.example.com"), + ); + ipcHandlers.set("upload_media_bytes", () => + Promise.resolve({ + url: "https://relay.example.com/media/fresh", + sha256: "f8e5", + size: 1, + type: "image/png", + uploaded: 0, + }), + ); + + let fetchCalls = 0; + ipcHandlers.set("fetch_link_preview_metadata", () => { + fetchCalls += 1; + return Promise.resolve( + fetchCalls === 1 + ? metadata({ + imageFetchState: "transient_failure", + imageRetryAfterMs: 900_000, + }) + : metadata({ + imageDataUrl: "data:image/png;base64,QQ==", + imageDomain: "images.example.com", + imageFetchState: "image", + }), + ); + }); + + try { + // Seed the shared cache with the negative result before the composer sees A. + const { useResolvedLinkPreviews } = await import( + "@/shared/lib/useResolvedLinkPreviews.ts" + ); + const cached = renderHook(() => + useResolvedLinkPreviews([ + { + kind: "generic-link", + href: HREF, + title: HREF, + provider: "example.com", + imageUrl: null, + }, + ]), + ); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 50)); + }); + assert.equal(fetchCalls, 1, "the negative was cached before paste"); + cached.unmount(); + + const { result, rerender, unmount } = renderHook( + ({ content }) => useComposerLinkPreviews(content), + { initialProps: { content: "" } }, + ); + await act(async () => rerender({ content: `see ${HREF}` })); + assert.equal( + fetchCalls, + 1, + "debounce has not resolved the pasted href yet", + ); + assert.equal(result.current.hasPendingSnapshots, true); + + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, DEBOUNCE_WAIT_MS)); + }); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 50)); + }); + assert.equal(fetchCalls, 2, "paste invalidated and refetched the negative"); + assert.equal(result.current.getReadyTags().length, 1); + unmount(); + } finally { + cleanup(); + ipcHandlers.clear(); + } +}); diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.ts b/desktop/src/shared/lib/useResolvedLinkPreviews.ts index 995777472f..acb855f8f0 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.ts +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.ts @@ -469,16 +469,14 @@ export function useResolvedLinkPreviews( React.useState({}); const [retryGeneration, setRetryGeneration] = React.useState(0); const seenHrefsRef = React.useRef>(new Set()); - // Newness is tracked against the live href set when the caller supplies one, - // so a debounce-swallowed leave/re-entry of the same URL still counts as new. - // Read through a ref inside the effect and drive re-runs off the stable string - // key, so an unstable per-render array does not have to be an effect dep. + // Drive newness tracking from a stable string key so an equivalent href list + // does not restart the effect. The effect closes over the committed render's + // hrefs; do not mirror them into a ref during render, because an abandoned + // concurrent render could otherwise leak uncommitted presence into the live + // effect from the previous commit. const currentHrefs = liveHrefs ?? previews.map((preview) => preview.href); const newnessKey = currentHrefs.join("\n"); - const currentHrefsRef = React.useRef(currentHrefs); - currentHrefsRef.current = currentHrefs; - - // biome-ignore lint/correctness/useExhaustiveDependencies: newnessKey is the stable string key for the live href set read via currentHrefsRef; it drives the re-run so the per-render array need not be a dep. + // biome-ignore lint/correctness/useExhaustiveDependencies: newnessKey is the stable identity for currentHrefs; depending on the freshly allocated array would rerun this effect every render. React.useEffect(() => { let cancelled = false; let retryAt = Number.POSITIVE_INFINITY; @@ -491,8 +489,14 @@ export function useResolvedLinkPreviews( // Newness is judged against the live href set when supplied (so a // debounce-swallowed leave/re-entry still counts), else against previews. const seen = seenHrefsRef.current; - const liveNow = currentHrefsRef.current; - const next = new Set(liveNow); + const liveNow = currentHrefs; + // Preserve handled hrefs only while they remain live. A live href is not + // marked handled until its debounced preview exists and invalidation has + // actually been attempted; otherwise blank -> paste would consume + // newness during the 350ms debounce and later reuse the stale negative. + const next = new Set( + [...seen].filter((href) => liveNow.includes(href)), + ); const reenteredKeys: string[] = []; for (const preview of previews) { if ( @@ -507,6 +511,7 @@ export function useResolvedLinkPreviews( // the shared entry is healthy, in-flight, or absent.) metadataLoader.invalidateNegative(preview.href); reenteredKeys.push(metadataCacheKey(preview.href)); + next.add(preview.href); } seenHrefsRef.current = next; // Dropping the loader entry alone is not enough: this hook retains its own From aee51af22728d09cf3d6d4cdeb8d69258494e440 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Thu, 13 Aug 2026 10:37:57 -0700 Subject: [PATCH 3/6] fix(link-preview): commit re-entry bookkeeping safely Co-authored-by: Taylor Ho Signed-off-by: Taylor Ho --- .../ui/useComposerLinkPreviews.test.mjs | 87 +++++++++++ .../messages/ui/useComposerLinkPreviews.tsx | 145 ++++++++---------- 2 files changed, 154 insertions(+), 78 deletions(-) diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs b/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs index 4fa736aebc..554ec52254 100644 --- a/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs @@ -480,3 +480,90 @@ test("composer refetches a cached negative when a link is pasted into an empty d ipcHandlers.clear(); } }); +// A concurrent render may execute the hook and then suspend before commit. No +// href presence, block, generation, or tag mutation from that abandoned render +// may affect the previously committed composer. +test("an abandoned concurrent render cannot invalidate the committed snapshot tag", async () => { + const React = await import("react"); + const { act, cleanup, render } = await import("@testing-library/react"); + const { resetLinkPreviewMetadataCache } = await import( + "@/shared/lib/useResolvedLinkPreviews.ts" + ); + const { useComposerLinkPreviews } = await import( + "./useComposerLinkPreviews.tsx" + ); + + resetLinkPreviewMetadataCache(); + ipcHandlers.clear(); + ipcHandlers.set("get_relay_http_url", () => + Promise.resolve("https://relay.example.com"), + ); + ipcHandlers.set("upload_media_bytes", () => + Promise.resolve({ + url: "https://relay.example.com/media/stable", + sha256: "57ab1e", + size: 1, + type: "image/png", + uploaded: 0, + }), + ); + ipcHandlers.set("fetch_link_preview_metadata", () => + Promise.resolve( + metadata({ + imageFetchState: "transient_failure", + imageRetryAfterMs: 900_000, + }), + ), + ); + + let latest; + const never = new Promise(() => {}); + function Suspender() { + throw never; + } + function Harness({ content, suspend }) { + latest = useComposerLinkPreviews(content); + return suspend ? React.createElement(Suspender) : null; + } + + try { + const view = render( + React.createElement(Harness, { content: `see ${HREF}`, suspend: false }), + ); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, DEBOUNCE_WAIT_MS)); + }); + assert.equal(latest.getReadyTags().length, 1); + + // Render a clear that executes the hook but suspends before commit, then + // supersede it with the unchanged committed content. + await act(async () => { + React.startTransition(() => + view.rerender( + React.createElement(Harness, { content: "", suspend: true }), + ), + ); + await Promise.resolve(); + }); + await act(async () => { + view.rerender( + React.createElement(Harness, { + content: `see ${HREF}`, + suspend: false, + }), + ); + await Promise.resolve(); + }); + + assert.equal( + latest.getReadyTags().length, + 1, + "the committed tag remains sendable after the abandoned render", + ); + assert.equal(latest.hasPendingSnapshots, false); + view.unmount(); + } finally { + cleanup(); + ipcHandlers.clear(); + } +}); diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx index 1b84d95bfb..7f5491a5e8 100644 --- a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx @@ -201,16 +201,14 @@ export function useComposerLinkPreviews(content: string, enabled = true) { // `hasUnresolvedLiveCandidates` below, which keeps Send disabled until the // live candidates resolve — so no synchronous flush is needed at submit. const [debounced, setDebounced] = React.useState(content); - const debouncedRef = React.useRef(debounced); - debouncedRef.current = debounced; React.useEffect(() => { - if (content === debouncedRef.current) return; + if (content === debounced) return; const timer = window.setTimeout( () => setDebounced(content), LINK_PREVIEW_DEBOUNCE_MS, ); return () => window.clearTimeout(timer); - }, [content]); + }, [content, debounced]); const extractCandidates = React.useCallback( (source: string) => enabled @@ -230,10 +228,18 @@ export function useComposerLinkPreviews(content: string, enabled = true) { // resolved (debounce not yet fired after a paste/keystroke), Send must still // treat the preview as pending so a fast Enter cannot ship a bare link ahead // of resolution. - const liveCandidatesRef = React.useRef([]); - liveCandidatesRef.current = extractCandidates(content).map( - (preview) => preview.href, + const liveCandidates = React.useMemo( + () => extractCandidates(content).map((preview) => preview.href), + [extractCandidates, content], ); + const liveCandidatesKey = liveCandidates.join("\n"); + // Submit and async-completion paths read only the last COMMITTED live set. + // Updating this during render would let an abandoned concurrent render leak + // uncommitted editor content into a later submit or upload completion. + const liveCandidatesRef = React.useRef([]); + React.useLayoutEffect(() => { + liveCandidatesRef.current = liveCandidates; + }, [liveCandidates]); // A URL freshly entering the composer (paste, or finishing typing one) should // get a fresh fetch rather than a stale negative cache hit — the user is // actively asking for this link's card now. useResolvedLinkPreviews handles @@ -245,7 +251,7 @@ export function useComposerLinkPreviews(content: string, enabled = true) { // still seen as a re-entry and refetched — not served the stale negative. const resolvedPreviews = useResolvedLinkPreviews( suppressed ? [] : candidates, - { refetchNewNegatives: true, liveHrefs: liveCandidatesRef.current }, + { refetchNewNegatives: true, liveHrefs: liveCandidates }, ); // Entity links resolve to null metadata when the relay lookup has nothing // for them; keep their safe fallback cards rather than dropping them. @@ -256,7 +262,7 @@ export function useComposerLinkPreviews(content: string, enabled = true) { // Clear a "hide previews" suppression as soon as the LIVE draft has no // supported candidates — not the debounced set, whose lag would otherwise let // a clear-then-retype race keep suppression stuck on after the draft changed. - const liveCandidatesEmpty = liveCandidatesRef.current.length === 0; + const liveCandidatesEmpty = liveCandidates.length === 0; React.useEffect(() => { if (liveCandidatesEmpty) setSuppressed(false); }, [liveCandidatesEmpty]); @@ -298,79 +304,62 @@ export function useComposerLinkPreviews(content: string, enabled = true) { ); }, [candidates]); - // Detect live-content presence transitions at render time (not in an effect): - // a fast clear-then-repaste of the same URL commits both the empty and the - // re-pasted render in one batch, so an effect keyed on the live set only ever - // observes the unchanged final value and never fires. Comparing against the - // previous render's live set catches the re-entry synchronously. A re-entered - // href drops its stale ready tag (built from the pre-clear metadata) so it is - // no longer sendable until the forced refetch produces a fresh one; the - // resolver, fed the same live hrefs, refetches it in step. Without this a - // clear+repaste within the 350ms debounce could ship a snapshot tag from the - // stale metadata. - const prevLiveHrefsRef = React.useRef>(new Set()); - // Hrefs that just re-entered while carrying a STALE negative fallback the - // resolver will refetch. The upload effect must not rebuild a snapshot tag - // from that stale metadata (its `previews` entry still carries the pre-clear - // `snapshotReady` fallback until the resolver's refetch commits the pending - // state a render later, then re-resolves). A blocked href is released only - // after the resolver visibly takes over — the preview goes pending ("blocked" - // -> "refetching") and then resolves ready again — so a fresh result (image - // OR a genuinely-new fallback) can tag, but the stale pre-clear fallback that - // merely lingers a render or two cannot. Only the sendable negative case - // (`imageState === "fallback"`) is ever blocked; a healthy (`"image"`) - // re-entry is kept by the resolver, never goes pending, and must not block or - // it would trap Send forever. + // Compare live content with the LAST COMMITTED href set during render, but do + // not mutate anything here. This gives synchronous submit/pending selectors a + // pure fence for a stale fallback on the candidate render. The layout effect + // below commits the block, generation bump, and tag removal only if React + // actually commits this render; an abandoned concurrent render leaks nothing. + const committedLiveHrefsRef = React.useRef>(new Set()); + // Hrefs that re-entered with a STALE negative fallback stay blocked until the + // resolver visibly cycles pending -> ready. Healthy image re-entries are not + // blocked because their cached metadata remains valid and does not refetch. const reenteringHrefsRef = React.useRef< Map >(new Map()); - const reenteredLiveHrefs = liveCandidatesRef.current.filter( - (href) => !prevLiveHrefsRef.current.has(href), + const reenteredLiveHrefs = liveCandidates.filter( + (href) => !committedLiveHrefsRef.current.has(href), ); - prevLiveHrefsRef.current = new Set(liveCandidatesRef.current); - if (reenteredLiveHrefs.length > 0) { - const staleReentered = reenteredLiveHrefs.filter( - (href) => - !reenteringHrefsRef.current.has(href) && - previews.some( - (preview) => - preview.href === href && - preview.snapshotReady && - preview.imageState === "fallback", - ), - ); - for (const href of staleReentered) { + const staleReenteredHrefs = reenteredLiveHrefs.filter( + (href) => + !reenteringHrefsRef.current.has(href) && + previews.some( + (preview) => + preview.href === href && + preview.snapshotReady && + preview.imageState === "fallback", + ), + ); + const staleReenteredKey = staleReenteredHrefs.join("\n"); + + // biome-ignore lint/correctness/useExhaustiveDependencies: stable href keys intentionally represent the live/stale sets; the arrays are rebuilt each render. + React.useLayoutEffect(() => { + committedLiveHrefsRef.current = new Set(liveCandidates); + if (staleReenteredHrefs.length === 0) return; + + for (const href of staleReenteredHrefs) { reenteringHrefsRef.current.set(href, "blocked"); - // Bump the upload generation so any upload started before this re-entry - // (built from the now-stale pre-clear metadata) is recognized as stale - // when it settles and cannot publish its tag. + // Fence any upload built from pre-re-entry metadata. A fresh generation + // can start while the superseded upload is still in flight. uploadGenerationRef.current.set( href, (uploadGenerationRef.current.get(href) ?? 0) + 1, ); } - if (staleReentered.some((href) => readyTagsByHrefRef.current[href])) { - // Drop the stale ready tag from state so (a) it stops being sendable and - // (b) the upload effect will rebuild it once the block releases with fresh - // metadata (its `readyTags[href]` guard would otherwise keep skipping). - // Functional updater so it survives the ref resync at the top of the next - // render. The read-time filter below also excludes blocked hrefs, so a - // synchronous submit in THIS render cannot ship the stale tag either. - const drop = new Set(staleReentered); - queueMicrotask(() => - setReadyTags((current) => { - let changed = false; - const next = { ...current }; - for (const href of drop) - if (href in next) { - delete next[href]; - changed = true; - } - return changed ? next : current; - }), - ); - } - } + const drop = new Set(staleReenteredHrefs); + setReadyTags((current) => { + let changed = false; + const next = { ...current }; + for (const href of drop) + if (href in next) { + delete next[href]; + changed = true; + } + return changed ? next : current; + }); + }, [liveCandidatesKey, staleReenteredKey]); + + const isHrefReentering = (href: string) => + reenteringHrefsRef.current.has(href) || staleReenteredHrefs.includes(href); React.useEffect(() => { for (const preview of previews) { @@ -477,8 +466,7 @@ export function useComposerLinkPreviews(content: string, enabled = true) { readyTagsRef.current = suppressed ? [["link-preview", "none"]] : candidates.flatMap((candidate) => - readyTags[candidate.href] && - !reenteringHrefsRef.current.has(candidate.href) + readyTags[candidate.href] && !isHrefReentering(candidate.href) ? [readyTags[candidate.href]] : [], ); @@ -494,6 +482,7 @@ export function useComposerLinkPreviews(content: string, enabled = true) { (preview) => !preview.href.startsWith("buzz://") && (preview.imageState === "pending" || + isHrefReentering(preview.href) || (preview.snapshotReady && !readyTags[preview.href])), ); // A supported link in the LIVE content that resolution has not caught up to @@ -502,7 +491,7 @@ export function useComposerLinkPreviews(content: string, enabled = true) { // before resolution even starts. buzz:// links never snapshot, so ignore them. const hasUnresolvedLiveCandidates = !suppressed && - liveCandidatesRef.current.some( + liveCandidates.some( (href) => !href.startsWith("buzz://") && !readyTags[href] && @@ -514,7 +503,6 @@ export function useComposerLinkPreviews(content: string, enabled = true) { // settling, so a link whose metadata or upload stalls never traps the // composer. Resets whenever settling ends or the live candidate set changes. const [settleDisableExpired, setSettleDisableExpired] = React.useState(false); - const liveCandidatesKey = liveCandidatesRef.current.join("\n"); // biome-ignore lint/correctness/useExhaustiveDependencies: liveCandidatesKey intentionally restarts the anti-trap cap when the link set changes while still settling, so a replaced/added link gets a fresh disable window rather than inheriting the prior link's near-expired timer. React.useEffect(() => { if (!hasSettlingSnapshots) { @@ -575,9 +563,10 @@ export function useComposerLinkPreviews(content: string, enabled = true) { // until every settling preview has its tag (or the anti-trap cap fires), so at // submit time the tags that will ever exist already exist. const getReadyTags = React.useCallback(() => { - const reentering = reenteringHrefsRef.current; return selectSubmitTags( - liveCandidatesRef.current.filter((href) => !reentering.has(href)), + liveCandidatesRef.current.filter( + (href) => !reenteringHrefsRef.current.has(href), + ), readyTagsByHrefRef.current, suppressedRef.current, ); From da72fc9d9c3ef4ce384aa24a12bcd0cb0b6195ef Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Thu, 13 Aug 2026 14:37:28 -0700 Subject: [PATCH 4/6] fix(link-preview): preserve batched re-entry transitions Co-authored-by: Taylor Ho Signed-off-by: Taylor Ho --- .../features/messages/ui/MessageComposer.tsx | 8 +- .../ui/useComposerLinkPreviews.test.mjs | 59 +++++++------ .../messages/ui/useComposerLinkPreviews.tsx | 82 +++++++++++++++++-- .../src/shared/lib/useResolvedLinkPreviews.ts | 26 +++++- 4 files changed, 140 insertions(+), 35 deletions(-) diff --git a/desktop/src/features/messages/ui/MessageComposer.tsx b/desktop/src/features/messages/ui/MessageComposer.tsx index d13cec95eb..022d60b4d1 100644 --- a/desktop/src/features/messages/ui/MessageComposer.tsx +++ b/desktop/src/features/messages/ui/MessageComposer.tsx @@ -57,7 +57,7 @@ import { usePersistentAgentMentionHydration } from "./usePersistentAgentMentionH import { useComposerContentState } from "./useComposerContentState"; import { useDraftPersistLifecycle } from "./useDraftPersistSnapshot"; import { submitMessageEdit } from "./submitMessageEdit"; -import { useComposerLinkPreviews } from "./useComposerLinkPreviews"; +import { useManagedComposerLinkPreviews } from "./useComposerLinkPreviews"; import { scheduleSettleGatedAutoSubmit } from "./messageComposerAutoSubmit"; import type { MessageComposerProps } from "./MessageComposer.types"; function MessageComposerImpl({ @@ -100,14 +100,14 @@ function MessageComposerImpl({ syncComposerContentFromEditor, syncContentRefFromEditorRef, } = useComposerContentState(); - const [previewContent, setPreviewContent] = React.useState(""); const { previewList: composerLinkPreviews, getReadyTags: getReadyLinkPreviewTags, hasPendingSnapshots: hasPendingLinkPreviewSnapshots, // Ref lets the submit guard block Enter/form/auto-submit until snapshots settle. hasPendingSnapshotsRef: hasPendingLinkPreviewSnapshotsRef, - } = useComposerLinkPreviews(previewContent, editTarget == null); + updateContent: updateLinkPreviewContent, + } = useManagedComposerLinkPreviews(editTarget == null); const [isEmojiPickerOpen, setIsEmojiPickerOpen] = React.useState(false); const [isFormattingOpen, setIsFormattingOpen] = React.useState(false); const [spoileredAttachmentUrls, setSpoileredAttachmentUrls] = React.useState< @@ -269,7 +269,7 @@ function MessageComposerImpl({ onLinkShortcut: () => onLinkShortcutRef.current?.() ?? false, onUpdate: ({ cursor, linkPreviewContent, text }) => { setComposerContentFromText(text); - setPreviewContent(linkPreviewContent); + updateLinkPreviewContent(linkPreviewContent); mentions.updateMentionQuery(text, cursor); channelLinks.updateChannelQuery(text, cursor); emojiAutocomplete.updateEmojiQuery(text, cursor); diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs b/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs index 554ec52254..9e1d2ac9c6 100644 --- a/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs @@ -63,9 +63,8 @@ test("composer forces a refetch and drops the stale tag on a fast clear+re-paste const { resetLinkPreviewMetadataCache } = await import( "@/shared/lib/useResolvedLinkPreviews.ts" ); - const { useComposerLinkPreviews } = await import( - "./useComposerLinkPreviews.tsx" - ); + const { updateComposerLinkPreviewInput, useComposerLinkPreviews } = + await import("./useComposerLinkPreviews.tsx"); resetLinkPreviewMetadataCache(); ipcHandlers.clear(); @@ -121,9 +120,14 @@ test("composer forces a refetch and drops the stale tag on a fast clear+re-paste }; try { + let previewInput = updateComposerLinkPreviewInput( + { content: "", hrefs: new Set(), hrefVersions: new Map() }, + `see ${HREF}`, + ); const { result, rerender, unmount } = renderHook( - ({ content }) => useComposerLinkPreviews(content), - { initialProps: { content: `see ${HREF}` } }, + ({ content, hrefVersions }) => + useComposerLinkPreviews(content, true, hrefVersions), + { initialProps: previewInput }, ); // 1. Paste settles to a transient-failure fallback with a ready snapshot tag. @@ -136,15 +140,19 @@ test("composer forces a refetch and drops the stale tag on a fast clear+re-paste ); assert.equal(result.current.hasPendingSnapshots, false); - // 2. Fast gesture: clear the URL, then re-paste the SAME URL, both before - // the debounce fires. Two separate ticks (as real keystrokes/paste are), - // so the LIVE content commits empty then re-pasted, but the debounced - // `candidates` never commit the empty intermediate. + // 2. Fast gesture: clear the URL, then re-paste the SAME URL, with both + // editor updates folded into one React batch. The debounced candidates + // and the committed live href set therefore never observe empty. + // Model two editor onUpdate calls folded into one React batch. The final + // href set equals the previous commit, but the update-boundary version has + // advanced because the URL left and re-entered between those updates. await act(async () => { - rerender({ content: "see " }); - }); - await act(async () => { - rerender({ content: `see ${HREF}` }); + previewInput = updateComposerLinkPreviewInput(previewInput, "see "); + previewInput = updateComposerLinkPreviewInput( + previewInput, + `see ${HREF}`, + ); + rerender(previewInput); }); // 3a. The stale tag must be gone AS SOON AS the same URL re-enters — this is @@ -227,9 +235,8 @@ test("a stale in-flight upload cannot publish after the URL re-enters and a fres const { resetLinkPreviewMetadataCache } = await import( "@/shared/lib/useResolvedLinkPreviews.ts" ); - const { useComposerLinkPreviews } = await import( - "./useComposerLinkPreviews.tsx" - ); + const { updateComposerLinkPreviewInput, useComposerLinkPreviews } = + await import("./useComposerLinkPreviews.tsx"); resetLinkPreviewMetadataCache(); ipcHandlers.clear(); @@ -306,9 +313,14 @@ test("a stale in-flight upload cannot publish after the URL re-enters and a fres }; try { + let previewInput = updateComposerLinkPreviewInput( + { content: "", hrefs: new Set(), hrefVersions: new Map() }, + `see ${HREF}`, + ); const { result, rerender, unmount } = renderHook( - ({ content }) => useComposerLinkPreviews(content), - { initialProps: { content: `see ${HREF}` } }, + ({ content, hrefVersions }) => + useComposerLinkPreviews(content, true, hrefVersions), + { initialProps: previewInput }, ); // 1. Paste settles to a transient-failure fallback. Its favicon upload (U1) @@ -328,12 +340,13 @@ test("a stale in-flight upload cannot publish after the URL re-enters and a fres // 2. Fast gesture: clear then re-paste the SAME URL inside the debounce. await act(async () => { - rerender({ content: "see " }); - }); - await act(async () => { - rerender({ content: `see ${HREF}` }); + previewInput = updateComposerLinkPreviewInput(previewInput, "see "); + previewInput = updateComposerLinkPreviewInput( + previewInput, + `see ${HREF}`, + ); + rerender(previewInput); }); - await act(async () => {}); // 3. The re-entry forces a fresh fetch; resolve it to fresh success. The // upload effect must start a FRESH upload (U2) even though U1 still holds diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx index 7f5491a5e8..af2af4e18f 100644 --- a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx @@ -192,7 +192,63 @@ async function uploadSnapshotMedia( } } -export function useComposerLinkPreviews(content: string, enabled = true) { +export function extractComposerLinkPreviewHrefs(content: string): string[] { + return extractSupportedLinkPreviews(content) + .filter((preview) => + preview.href.startsWith("buzz://") + ? true + : isValidLinkPreviewSnapshotCanonicalUrl(preview.href), + ) + .map((preview) => preview.href); +} + +export interface ComposerLinkPreviewInput { + content: string; + hrefs: Set; + hrefVersions: Map; +} + +export function updateComposerLinkPreviewInput( + current: ComposerLinkPreviewInput, + content: string, +): ComposerLinkPreviewInput { + const nextHrefs = new Set(extractComposerLinkPreviewHrefs(content)); + const nextVersions = new Map(current.hrefVersions); + for (const href of nextHrefs) { + if (!current.hrefs.has(href)) { + nextVersions.set(href, (nextVersions.get(href) ?? 0) + 1); + } + } + return { content, hrefs: nextHrefs, hrefVersions: nextVersions }; +} + +export function useComposerLinkPreviewInput() { + const [input, setInput] = React.useState(() => ({ + content: "", + hrefs: new Set(), + hrefVersions: new Map(), + })); + const update = React.useCallback( + (content: string) => + setInput((current) => updateComposerLinkPreviewInput(current, content)), + [], + ); + return [input, update] as const; +} + +export function useManagedComposerLinkPreviews(enabled = true) { + const [input, updateContent] = useComposerLinkPreviewInput(); + return { + ...useComposerLinkPreviews(input.content, enabled, input.hrefVersions), + updateContent, + }; +} + +export function useComposerLinkPreviews( + content: string, + enabled = true, + liveHrefVersions?: ReadonlyMap, +) { const [suppressed, setSuppressed] = React.useState(false); // Debounce the content that drives resolution so typing a URL character by // character does not churn a new candidate href (and a flickering card) per @@ -233,6 +289,9 @@ export function useComposerLinkPreviews(content: string, enabled = true) { [extractCandidates, content], ); const liveCandidatesKey = liveCandidates.join("\n"); + const liveHrefVersionsKey = liveCandidates + .map((href) => `${href}\0${liveHrefVersions?.get(href) ?? ""}`) + .join("\n"); // Submit and async-completion paths read only the last COMMITTED live set. // Updating this during render would let an abandoned concurrent render leak // uncommitted editor content into a later submit or upload completion. @@ -251,7 +310,11 @@ export function useComposerLinkPreviews(content: string, enabled = true) { // still seen as a re-entry and refetched — not served the stale negative. const resolvedPreviews = useResolvedLinkPreviews( suppressed ? [] : candidates, - { refetchNewNegatives: true, liveHrefs: liveCandidates }, + { + refetchNewNegatives: true, + liveHrefs: liveCandidates, + liveHrefVersions, + }, ); // Entity links resolve to null metadata when the relay lookup has nothing // for them; keep their safe fallback cards rather than dropping them. @@ -310,15 +373,21 @@ export function useComposerLinkPreviews(content: string, enabled = true) { // below commits the block, generation bump, and tag removal only if React // actually commits this render; an abandoned concurrent render leaks nothing. const committedLiveHrefsRef = React.useRef>(new Set()); + const committedLiveHrefVersionsRef = React.useRef>( + new Map(), + ); // Hrefs that re-entered with a STALE negative fallback stay blocked until the // resolver visibly cycles pending -> ready. Healthy image re-entries are not // blocked because their cached metadata remains valid and does not refetch. const reenteringHrefsRef = React.useRef< Map >(new Map()); - const reenteredLiveHrefs = liveCandidates.filter( - (href) => !committedLiveHrefsRef.current.has(href), - ); + const reenteredLiveHrefs = liveCandidates.filter((href) => { + const version = liveHrefVersions?.get(href); + return version === undefined + ? !committedLiveHrefsRef.current.has(href) + : committedLiveHrefVersionsRef.current.get(href) !== version; + }); const staleReenteredHrefs = reenteredLiveHrefs.filter( (href) => !reenteringHrefsRef.current.has(href) && @@ -334,6 +403,7 @@ export function useComposerLinkPreviews(content: string, enabled = true) { // biome-ignore lint/correctness/useExhaustiveDependencies: stable href keys intentionally represent the live/stale sets; the arrays are rebuilt each render. React.useLayoutEffect(() => { committedLiveHrefsRef.current = new Set(liveCandidates); + committedLiveHrefVersionsRef.current = new Map(liveHrefVersions); if (staleReenteredHrefs.length === 0) return; for (const href of staleReenteredHrefs) { @@ -356,7 +426,7 @@ export function useComposerLinkPreviews(content: string, enabled = true) { } return changed ? next : current; }); - }, [liveCandidatesKey, staleReenteredKey]); + }, [liveCandidatesKey, liveHrefVersionsKey, staleReenteredKey]); const isHrefReentering = (href: string) => reenteringHrefsRef.current.has(href) || staleReenteredHrefs.includes(href); diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.ts b/desktop/src/shared/lib/useResolvedLinkPreviews.ts index acb855f8f0..8ef93e7d13 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.ts +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.ts @@ -444,6 +444,7 @@ export function useResolvedLinkPreviews( { refetchNewNegatives = false, liveHrefs, + liveHrefVersions, }: { /** * When a preview href is newly present since the last run, drop any cached @@ -463,19 +464,29 @@ export function useResolvedLinkPreviews( * uses this. Omit to track newness against `previews` (the default). */ liveHrefs?: readonly string[]; + /** + * Per-href entry versions captured at the live editor-update boundary. + * Unlike committed href-set equality, a bumped version preserves an + * intermediate leave/re-entry even when React batches both updates into one + * commit with the same final href set. + */ + liveHrefVersions?: ReadonlyMap; } = {}, ): ResolvedLinkPreview[] { const [resolvedMetadata, setResolvedMetadata] = React.useState({}); const [retryGeneration, setRetryGeneration] = React.useState(0); const seenHrefsRef = React.useRef>(new Set()); + const handledHrefVersionsRef = React.useRef>(new Map()); // Drive newness tracking from a stable string key so an equivalent href list // does not restart the effect. The effect closes over the committed render's // hrefs; do not mirror them into a ref during render, because an abandoned // concurrent render could otherwise leak uncommitted presence into the live // effect from the previous commit. const currentHrefs = liveHrefs ?? previews.map((preview) => preview.href); - const newnessKey = currentHrefs.join("\n"); + const newnessKey = currentHrefs + .map((href) => `${href}\0${liveHrefVersions?.get(href) ?? ""}`) + .join("\n"); // biome-ignore lint/correctness/useExhaustiveDependencies: newnessKey is the stable identity for currentHrefs; depending on the freshly allocated array would rerun this effect every render. React.useEffect(() => { let cancelled = false; @@ -489,6 +500,7 @@ export function useResolvedLinkPreviews( // Newness is judged against the live href set when supplied (so a // debounce-swallowed leave/re-entry still counts), else against previews. const seen = seenHrefsRef.current; + const handledVersions = handledHrefVersionsRef.current; const liveNow = currentHrefs; // Preserve handled hrefs only while they remain live. A live href is not // marked handled until its debounced preview exists and invalidation has @@ -497,10 +509,18 @@ export function useResolvedLinkPreviews( const next = new Set( [...seen].filter((href) => liveNow.includes(href)), ); + const nextVersions = new Map( + [...handledVersions].filter(([href]) => liveNow.includes(href)), + ); const reenteredKeys: string[] = []; for (const preview of previews) { + const version = liveHrefVersions?.get(preview.href); + const alreadyHandled = + version === undefined + ? seen.has(preview.href) + : handledVersions.get(preview.href) === version; if ( - seen.has(preview.href) || + alreadyHandled || !liveNow.includes(preview.href) || preview.href.startsWith("buzz://") ) { @@ -512,8 +532,10 @@ export function useResolvedLinkPreviews( metadataLoader.invalidateNegative(preview.href); reenteredKeys.push(metadataCacheKey(preview.href)); next.add(preview.href); + if (version !== undefined) nextVersions.set(preview.href, version); } seenHrefsRef.current = next; + handledHrefVersionsRef.current = nextVersions; // Dropping the loader entry alone is not enough: this hook retains its own // resolved metadata, and the render that scheduled this effect already // read the stale negative from it. Clear this hook's OWN negative key for From 431daa6228f31083917a6c6fd1e44cb399079a53 Mon Sep 17 00:00:00 2001 From: Wes Date: Thu, 13 Aug 2026 15:59:00 -0600 Subject: [PATCH 5/6] fix(link-preview): bound composer href versions Retain entry versions only for active hrefs while assigning each new entry from a monotonic counter. This preserves batched leave-and-reentry detection without cloning and retaining every URL seen over the composer lifetime. Add focused coverage for pruning departed hrefs and advancing a reentered href after its previous map entry has been removed. Co-authored-by: Carl Signed-off-by: Wes --- .../ui/useComposerLinkPreviews.test.mjs | 45 ++++++++++++++++++- .../messages/ui/useComposerLinkPreviews.tsx | 20 +++++++-- 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs b/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs index 9e1d2ac9c6..ac9694ef7d 100644 --- a/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs @@ -58,6 +58,37 @@ function metadata(overrides = {}) { }; } +test("composer input versions retain only active hrefs while re-entry advances", async () => { + const { updateComposerLinkPreviewInput } = await import( + "./useComposerLinkPreviews.tsx" + ); + const secondHref = "https://example.com/second"; + let input = { + content: "", + hrefs: new Set(), + hrefVersions: new Map(), + nextHrefVersion: 0, + }; + + input = updateComposerLinkPreviewInput(input, `see ${HREF}`); + const firstVersion = input.hrefVersions.get(HREF); + assert.equal(input.hrefVersions.size, 1); + + input = updateComposerLinkPreviewInput(input, `see ${secondHref}`); + assert.deepEqual( + [...input.hrefVersions.keys()], + [secondHref], + "departed href history is pruned instead of retained for the composer lifetime", + ); + + input = updateComposerLinkPreviewInput(input, `see ${HREF}`); + assert.deepEqual([...input.hrefVersions.keys()], [HREF]); + assert.ok( + input.hrefVersions.get(HREF) > firstVersion, + "a re-entered href receives a new monotonic version after its old entry was pruned", + ); +}); + test("composer forces a refetch and drops the stale tag on a fast clear+re-paste of the same URL", async () => { const { act, cleanup, renderHook } = await import("@testing-library/react"); const { resetLinkPreviewMetadataCache } = await import( @@ -121,7 +152,12 @@ test("composer forces a refetch and drops the stale tag on a fast clear+re-paste try { let previewInput = updateComposerLinkPreviewInput( - { content: "", hrefs: new Set(), hrefVersions: new Map() }, + { + content: "", + hrefs: new Set(), + hrefVersions: new Map(), + nextHrefVersion: 0, + }, `see ${HREF}`, ); const { result, rerender, unmount } = renderHook( @@ -314,7 +350,12 @@ test("a stale in-flight upload cannot publish after the URL re-enters and a fres try { let previewInput = updateComposerLinkPreviewInput( - { content: "", hrefs: new Set(), hrefVersions: new Map() }, + { + content: "", + hrefs: new Set(), + hrefVersions: new Map(), + nextHrefVersion: 0, + }, `see ${HREF}`, ); const { result, rerender, unmount } = renderHook( diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx index af2af4e18f..9f69483689 100644 --- a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx @@ -206,6 +206,7 @@ export interface ComposerLinkPreviewInput { content: string; hrefs: Set; hrefVersions: Map; + nextHrefVersion: number; } export function updateComposerLinkPreviewInput( @@ -213,13 +214,23 @@ export function updateComposerLinkPreviewInput( content: string, ): ComposerLinkPreviewInput { const nextHrefs = new Set(extractComposerLinkPreviewHrefs(content)); - const nextVersions = new Map(current.hrefVersions); + const nextVersions = new Map(); + let nextHrefVersion = current.nextHrefVersion; for (const href of nextHrefs) { - if (!current.hrefs.has(href)) { - nextVersions.set(href, (nextVersions.get(href) ?? 0) + 1); + if (current.hrefs.has(href)) { + const version = current.hrefVersions.get(href); + if (version !== undefined) nextVersions.set(href, version); + continue; } + nextHrefVersion += 1; + nextVersions.set(href, nextHrefVersion); } - return { content, hrefs: nextHrefs, hrefVersions: nextVersions }; + return { + content, + hrefs: nextHrefs, + hrefVersions: nextVersions, + nextHrefVersion, + }; } export function useComposerLinkPreviewInput() { @@ -227,6 +238,7 @@ export function useComposerLinkPreviewInput() { content: "", hrefs: new Set(), hrefVersions: new Map(), + nextHrefVersion: 0, })); const update = React.useCallback( (content: string) => From e88b47c32da56ba195f59271d41cbf67fe77b554 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Fri, 14 Aug 2026 08:45:47 -0700 Subject: [PATCH 6/6] fix(link-preview): clear abandoned re-entry state Co-authored-by: Taylor Ho Signed-off-by: Taylor Ho --- .../ui/useComposerLinkPreviews.test.mjs | 129 ++++++++++++++++++ .../messages/ui/useComposerLinkPreviews.tsx | 20 ++- 2 files changed, 147 insertions(+), 2 deletions(-) diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs b/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs index ac9694ef7d..049c57f70c 100644 --- a/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs @@ -254,6 +254,135 @@ test("composer forces a refetch and drops the stale tag on a fast clear+re-paste } }); +// A blocked re-entry can leave again before its forced refetch settles. The +// abandoned phase must not poison a later paste of the now-healthy cached result. +test("a removed blocked re-entry can later use metadata that resolved while absent", async () => { + const { act, cleanup, renderHook } = await import("@testing-library/react"); + const { resetLinkPreviewMetadataCache } = await import( + "@/shared/lib/useResolvedLinkPreviews.ts" + ); + const { updateComposerLinkPreviewInput, useComposerLinkPreviews } = + await import("./useComposerLinkPreviews.tsx"); + + resetLinkPreviewMetadataCache(); + ipcHandlers.clear(); + ipcHandlers.set("get_relay_http_url", () => + Promise.resolve("https://relay.example.com"), + ); + ipcHandlers.set("upload_media_bytes", () => + Promise.resolve({ + url: "https://relay.example.com/media/fresh-after-absence", + sha256: "f8e5", + size: 1, + type: "image/png", + uploaded: 0, + }), + ); + + let fetchCalls = 0; + let resolveRefetch; + ipcHandlers.set("fetch_link_preview_metadata", () => { + fetchCalls += 1; + if (fetchCalls === 1) { + return Promise.resolve( + metadata({ + imageFetchState: "transient_failure", + imageRetryAfterMs: 900_000, + }), + ); + } + return new Promise((resolve) => { + resolveRefetch = () => + resolve( + metadata({ + imageDataUrl: "data:image/png;base64,QQ==", + imageDomain: "images.example.com", + imageFetchState: "image", + }), + ); + }); + }); + + const settle = async () => { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, DEBOUNCE_WAIT_MS)); + }); + }; + + try { + let previewInput = updateComposerLinkPreviewInput( + { + content: "", + hrefs: new Set(), + hrefVersions: new Map(), + nextHrefVersion: 0, + }, + `see ${HREF}`, + ); + const { result, rerender, unmount } = renderHook( + ({ content, hrefVersions }) => + useComposerLinkPreviews(content, true, hrefVersions), + { initialProps: previewInput }, + ); + + await settle(); + assert.equal(result.current.getReadyTags().length, 1); + + // Re-enter the cached negative and wait until its forced refetch is in flight. + await act(async () => { + previewInput = updateComposerLinkPreviewInput(previewInput, "see "); + previewInput = updateComposerLinkPreviewInput( + previewInput, + `see ${HREF}`, + ); + rerender(previewInput); + }); + await settle(); + assert.equal(fetchCalls, 2); + assert.equal(result.current.getReadyTags().length, 0); + assert.equal(result.current.hasPendingSnapshots, true); + + // Remove the blocked href, then let its refetch populate healthy metadata + // while no candidate is active. + await act(async () => { + previewInput = updateComposerLinkPreviewInput(previewInput, "see "); + rerender(previewInput); + }); + await settle(); + await act(async () => resolveRefetch()); + await settle(); + assert.equal(result.current.getReadyTags().length, 0); + + // A later paste should use the healthy result immediately after debounce; + // the abandoned "blocked" phase must not survive and suppress its tag. + await act(async () => { + previewInput = updateComposerLinkPreviewInput( + previewInput, + `see ${HREF}`, + ); + rerender(previewInput); + }); + await settle(); + const [freshTag] = result.current.getReadyTags(); + assert.equal( + fetchCalls, + 2, + "healthy metadata is preserved without a third fetch", + ); + assert.equal(result.current.getReadyTags().length, 1); + assert.ok( + freshTag?.includes("https://relay.example.com/media/fresh-after-absence"), + "the later paste becomes sendable with metadata resolved while absent", + ); + assert.equal(result.current.hasPendingSnapshots, false); + + unmount(); + } finally { + cleanup(); + ipcHandlers.clear(); + } +}); + // ── Composer-hook regression: stale in-flight upload after re-entry ─────────── // // A pre-clear transient-fallback snapshot upload (U1) can still be in flight diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx index 9f69483689..1ea94821cf 100644 --- a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx @@ -360,7 +360,6 @@ export function useComposerLinkPreviews( // never left tagless waiting on a doomed upload. const uploadGenerationRef = React.useRef(new Map()); const activeHrefsRef = React.useRef(new Set()); - activeHrefsRef.current = new Set(candidates.map((preview) => preview.href)); React.useEffect(() => { if (getCachedRelayOrigin()) return; @@ -414,8 +413,25 @@ export function useComposerLinkPreviews( // biome-ignore lint/correctness/useExhaustiveDependencies: stable href keys intentionally represent the live/stale sets; the arrays are rebuilt each render. React.useLayoutEffect(() => { - committedLiveHrefsRef.current = new Set(liveCandidates); + const previousLiveHrefs = committedLiveHrefsRef.current; + const activeLiveHrefs = new Set(liveCandidates); + activeHrefsRef.current = activeLiveHrefs; + committedLiveHrefsRef.current = activeLiveHrefs; committedLiveHrefVersionsRef.current = new Map(liveHrefVersions); + + // Leaving the live draft ends the current re-entry cycle. Prune its phase so + // a later paste can consume healthy metadata that resolved while absent. + // Also advance the upload generation: an upload started before removal must + // never publish into a later incarnation of the same href. + for (const href of previousLiveHrefs) { + if (activeLiveHrefs.has(href)) continue; + reenteringHrefsRef.current.delete(href); + uploadGenerationRef.current.set( + href, + (uploadGenerationRef.current.get(href) ?? 0) + 1, + ); + } + if (staleReenteredHrefs.length === 0) return; for (const href of staleReenteredHrefs) {