fix(link-preview): refetch a link when it re-enters the composer

[tellaho]

Overview

Category: fix
User Impact: When a user re-pastes (or finishes typing) a link that previously failed to load a preview, the composer now refetches it immediately and can never send a snapshot preview built from the old, stale metadata.
Problem: The link-preview cache is shared with passive message-list scroll, so a URL that resolved to a negative result (a hard null miss or a transient fetch failure) stayed cached and re-usable. Re-pasting that exact link into the composer served the stale negative and never refetched. Worse, the stale metadata was still snapshotReady, so a fast clear-then-repaste could attach a stale snapshot preview tag to the sent message — a preview that no longer matched the link.
Solution: A freshly-entering link is forced to refetch, and the composer is fenced against ever shipping a tag built from pre-re-entry metadata. This closes three distinct races surfaced over successive review passes: (1) the shared negative cache being reused on re-entry; (2) the resolver's debounce swallowing a fast clear+re-paste so the re-entry was invisible and the stale tag stayed sendable; and (3) an in-flight media upload started from the stale metadata publishing its tag after fresh metadata had already arrived. Healthy cached hits are never touched (instant card, no redundant fetch), and passive message-list scroll — which never opts in — keeps riding the shared cache exactly as before.

File changes

desktop/src/shared/lib/useResolvedLinkPreviews.ts
Adds a loader invalidateNegative(href) that drops a cached negative result (resolved null or transient fail) while leaving healthy hits and in-flight promises alone, and a refetchNewNegatives option that invalidates each newly-present href's negative entry before the peek/load loop reads the cache. Also adds an optional liveHrefs input so newness is judged against the caller's LIVE (undebounced) content — a debounce-swallowed leave/re-entry of the same URL still counts as new. Because the hook retains its own resolved metadata (the render that scheduled the effect already read the stale negative from it), it also clears its OWN negative key for every re-entered href, so the link renders as pending until the fresh load wins. buzz:// entity links are skipped (they resolve off the relay, not this cache).

desktop/src/features/messages/ui/useComposerLinkPreviews.tsx
Opts the composer into refetchNewNegatives and feeds it the live hrefs. Detects a same-URL re-entry at render time (React batches the empty→repaste renders, so an effect keyed on the live set never observes the transition), then blocks the re-entered href until the resolver's forced refetch visibly cycles through pending: its stale ready tag is dropped from state and excluded from the sendable output until a fresh result re-tags. Only the sendable negative case (fallback) is blocked; a healthy (image) re-entry keeps its instant card. Adds a per-href upload generation token (uploadsRef becomes Map<href, generation>): a live re-entry bumps the generation, the upload effect's dedup guard and completion are generation-aware, so an in-flight upload from stale metadata cannot publish its tag after settling and a fresh upload can start even while the superseded one is still in flight.

desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs
Adds resolver-level regressions: invalidateNegative drops a cached miss (next load refetches) but preserves a healthy hit (no redundant fetch); transient failure → URL removed → re-entered renders pending/not-snapshotReady until a successful retry; and the retained-negative + shared in-flight-fetch + re-entry interleaving clears the local negative regardless of the shared entry's shape.

desktop/src/features/messages/ui/useComposerLinkPreviews.test.mjs
Adds composer-hook regressions driving the REAL hook through the hostile gestures: a fast clear+re-paste inside the debounce window drops the stale tag and holds Send pending until a fresh tag carrying the newly-fetched media lands; and a stale in-flight upload held across the clear+re-paste and fresh-metadata resolution cannot publish its pre-clear tag, while a fresh upload starts and its tag wins.

Reproduction Steps

1. Paste a link whose preview fails to resolve (force a transient fetch failure) so the composer shows a blank/collapsed card.
2. Clear the composer and re-paste the same link (quickly, within the ~350ms debounce window).
3. Observe the preview refetches immediately rather than reusing the stale negative result, and Send stays disabled until a fresh tag lands.
4. Send the message and confirm the attached preview tag reflects the fresh fetch, never the stale pre-clear metadata.
5. Confirm passive message-list scroll of already-resolved links still shows cards instantly with no extra fetches.

Notes

Scope grew across three review passes from the original single resolver opt-in into a full defense against shipping stale snapshot tags on link re-entry — see the scope-adjustment comment on this PR for the detail. Stacked on #5245 (tho/link-preview-snapshot-race), whose rewrite of useComposerLinkPreviews.tsx is the sole overlapping file. The transient-retry work stays in #5502, which touches no composer file and remains based on main.

[wesbillman]

Reviewing on Wes's behalf. The new behavior does not cover one of the two negative-cache states it explicitly promises to retry. I reproduced this with a focused loader test: after caching imageFetchState: "transient_failure", invalidateNegative() followed by load() returned the same cached transient result and the fetch count stayed at 1. Please make transient failures invalidatable (or narrow the stated behavior if they are intentionally governed only by their retry boundary) and add coverage for that case. The current test also says it preserves an in-flight fetch but never creates one; please exercise that claimed invariant directly. Separately, this stacked PR is currently conflicting/dirty against its advanced base, so its green checks do not establish merge readiness for the eventual rebased head.

[wesbillman]

Re-review by Carl on Wes's behalf: the prior blocker is resolved at this head. invalidateNegative now invalidates both hard null misses and transient failures, while preserving healthy and in-flight entries; focused tests cover all four states. I found no new actionable issues. Per owner policy I am not auto-approving; Wes can approve this head.

[wesbillman]

Reviewing on Wes's behalf. Do not merge this head: invalidating the shared metadata loader does not invalidate useResolvedLinkPreviews' retained resolvedMetadata state.

On URL re-entry, the effect deletes the loader entry at useResolvedLinkPreviews.ts:463-475, but the render that scheduled that effect has already read the old local metadata at :538-545, and the effect never removes that key from local state. For a cached transient_failure, the composer therefore still sees a resolved snapshotReady fallback and can start producing a ready snapshot tag before the retry resolves (useComposerLinkPreviews.tsx:289-349), allowing stale metadata to become sendable. A cached null likewise remains the rendered no-preview state until the retry completes.

Please clear or generation-invalidate the corresponding local resolved state when a newly re-entered negative is invalidated, so that the retry is represented as pending immediately. Add a hook/composer-level regression covering transient failure -> URL removed -> URL re-entered -> pending/no ready tag -> successful retry wins. The loader-only tests do not exercise this second cache layer.

[tellaho]

@wesbillman thanks for the catch — your 14:53Z review was right, no pushback. Fix pushed. Summary:

Root cause: Invalidating the shared loader cache alone wasn't enough. useResolvedLinkPreviews keeps its own resolvedMetadata state, and the render that scheduled the invalidating effect had already read the stale negative out of it. So a re-entered transient_failure stayed a snapshotReady fallback — which the composer could turn into a sendable snapshot tag built from stale metadata before the retry landed.

Fix (useResolvedLinkPreviews.ts):

• invalidateNegative now returns whether it actually dropped a negative entry.
• The re-entry effect clears the matching local resolvedMetadata key in step, so the link renders pending (no snapshotReady) until the fresh load wins. Healthy hits and passive scroll are untouched.

Regression test: hook-level, drives the real hook via renderHook + JSDOM + mocked Tauri IPC — transient failure → URL removed → re-entered → asserts pending / no ready tag → retry succeeds. Proven genuine: it fails with 'fallback' (the stale sendable state) when the fix is temporarily removed, passes with it in.

Verification: tsc clean, biome clean, full desktop suite 4609/4609 pass, pre-push hooks green. Rebased onto latest main.

[tellaho]

Follow-up — also found this during an adversarial re-review, and fixed it in the same commit (new head 24785e3aa). @wesbillman flagging so you see it on your next pass.

Also found: an in-flight shared-cache entry defeated the local-state clear.

The useResolvedLinkPreviews loader is intentionally shared across every hook instance (composer + message list). invalidateNegative deliberately leaves an in-flight Promise entry alone. But my re-entry effect only cleared this hook's retained negative when invalidateNegative returned true — i.e. only when it dropped a settled negative. So this sequence stayed sendably stale:

1. The composer hook retains a transient_failure in local state; its URL leaves.
2. The shared negative expires/is evicted, and another instance (e.g. the message list, or a retry timer) starts a refetch for the same canonical URL — leaving a Promise in the shared cache.
3. The URL re-enters the composer while that fetch is in flight.
4. invalidateNegative returns false (the shared entry is a Promise), so the local clear was skipped — the composer kept returning imageState: "fallback", snapshotReady: true, a sendable tag built from stale metadata, until the shared fetch resolved.

Same defect class as the original bug, just via the in-flight path instead of the settled-negative path.

Fix (useResolvedLinkPreviews.ts): the re-entry effect now clears this hook's own negative local key for every newly-present href, gated on the hook's local value (isNegativeMetadata), independent of whether the shared loader dropped a settled entry. invalidateNegative still runs to drop a settled shared negative; the loop then coalesces onto any in-flight Promise. Healthy local hits are kept, so passive scroll still shows cards instantly.

Regression test: a second hook-level test drives the exact interleaving — retained transient local state → URL leaves → shared cache dropped → a message-list instance starts a shared in-flight fetch → composer re-enters → asserts pending / no snapshotReady while in flight, coalesces (no third fetch), then settles on the successful resolve. Proven genuine: it fails on the old drop-gated logic (re-entry during a shared in-flight fetch must render pending, not a stale fallback) while the original re-entry test still passes — so it pins the new in-flight gap specifically.

Verification: tsc clean, biome clean, full hook test file 23/23, pre-push hooks green (desktop-check + typecheck + full desktop-test at head 24785e3aa).

Credit to an adversarial second-reviewer pass for catching the in-flight interleaving.

[tellaho]

Also found + fixed a second call-site gap (thanks @wesbillman for the earlier round; this one surfaced in a follow-up review).

The bug: the composer feeds useResolvedLinkPreviews from debounced candidates (350ms), but a URL's leave/re-entry has to be observed from live content. On a fast clear-then-repaste of the same URL A inside the debounce window, each keystroke resets the debounce timer so the debounced set never commits the empty intermediate — candidates stays [A] throughout. The newness tracker never sees A leave, never invalidates, and the stale transient_failure snapshot tag (keyed by A) stays sendable. The two hook-level tests couldn't catch it because they bypass useComposerLinkPreviews.

The fix (2 layers):

• useResolvedLinkPreviews now takes an optional liveHrefs and judges newness against the live href set (read via a ref, driven by a stable newnessKey), so a 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. On a stale-fallback re-entry it drops the stale tag from state, excludes the href from sendable output, and blocks the upload effect (and any in-flight upload's completion) from rebuilding a tag from the pre-clear metadata. The href is released only after the resolver's forced refetch visibly cycles through pending → ready, so only genuinely-fresh metadata can re-tag. Healthy (image) re-entries keep their instant card and are never blocked.

Test: new useComposerLinkPreviews.test.mjs drives the real hook through the fast gesture with a deferred re-entry fetch. Asserts the stale tag is not sendable the moment the URL re-enters (synchronous invariant), that the re-entry forces a fresh fetch while Send is held pending, then that a fresh tag carrying the newly-fetched media lands once the refetch resolves. Verified genuine: reverting either fix layer makes it fail fast.

Full desktop suite green (4673 pass); pre-push hook desktop-test green. Author/DCO amended into the feature commit.

[tellaho]

@wesbillman Third blocker addressed (thanks to Princess Donut's third review pass) — head is now 50ead432d.

The race: an old in-flight media upload (U1), started from the pre-clear transient-fallback metadata, could still publish a snapshot tag after the URL was cleared, re-pasted, and freshly resolved. The reenteringHrefsRef phase marker alone didn't fence it: the upload effect deletes that marker the moment the forced refetch reaches fresh-ready metadata, which can happen before U1 settles — so U1's completion then passed the marker guard and wrote a tag built from the stale pre-clear metadata. Secondary hole: because the old href still occupied uploadsRef, the effect skipped starting a fresh upload (U2), and U1's .finally cleared the slot with no state change to re-arm one, so the composer could sit tagless until the anti-trap cap.

Fix — per-href upload generation token:

• uploadsRef is now Map<href, generation>; a live re-entry bumps uploadGenerationRef.
• The upload effect captures the current generation, and its dedup guard compares against it — so a superseded in-flight upload no longer blocks starting the fresh one at the new generation (U2 starts immediately on fresh-ready).
• A durable generation fence in .then, independent of the phase marker, makes a stale completion a no-op even after the marker was cleared.
• .finally only clears the slot when it still owns the current generation, so it can't evict U2's entry.

Regression: new composer-hook test holds U1 unresolved across the clear + re-paste and the fresh-metadata resolution, proves U2 starts and its fresh tag wins, then releases U1 and proves it cannot publish its pre-clear tag. Proven genuine — it fails on the prior head both ways (U2 never starts with the old Set dedup; and, with only the dedup fix, U1 publishes its stale favicon). tsc + biome clean; full desktop suite 4674 pass; pre-push desktop-test green.

[tellaho]

📝 Scope adjustment — description updated to match what actually shipped.

The original description covered only the first fix: an opt-in refetchNewNegatives mode in useResolvedLinkPreviews plus a one-line composer opt-in and a single resolver test. Over three successive review passes (thanks @wesbillman and the review crew) two further races were found and fixed on this branch, so the "File changes" section and the User Impact/Solution had drifted out of date. I've broadened the description to describe all three, with no change to the shipped code:

1. Resolver negative-cache re-entry (original scope): invalidateNegative + refetchNewNegatives, and clearing the hook's OWN retained negative key so a re-entry renders pending until the fresh load wins.
2. Debounce-swallowed re-entry (useResolvedLinkPreviews.ts + useComposerLinkPreviews.tsx): the composer feeds the resolver DEBOUNCED candidates, so a fast clear+re-paste of the same URL inside ~350ms never committed an empty set and the re-entry was invisible — the stale transient_failure tag stayed sendable. Fixed with live-href newness tracking in the resolver and render-time re-entry detection in the composer that drops the stale tag and excludes the href from sendable output until the forced refetch cycles through pending.
3. Stale in-flight upload (useComposerLinkPreviews.tsx): an old media upload started from pre-clear metadata could publish a snapshot tag after fresh metadata arrived. Fixed with a per-href upload generation token — uploadsRef is now Map<href, generation>, a live re-entry bumps the generation, and the dedup guard + .then fence + .finally are generation-aware.

Net diff: useComposerLinkPreviews.tsx +171 / useResolvedLinkPreviews.ts +114, plus deterministic regressions in both test files (+389 / +415). tsc + biome clean; full desktop suite 4674 pass; pre-push desktop-test green at head 50ead432d.

[wesbillman]

Carl reviewing on Wes's behalf.

The stale-upload generation fencing is sound for committed renders, but the re-entry detector introduces render-phase side effects that are unsafe under React concurrent rendering. At useComposerLinkPreviews.tsx:327-373, render mutates prevLiveHrefsRef, reenteringHrefsRef, and uploadGenerationRef, then queues a setReadyTags microtask. React may restart or abandon that render before commit (Suspense, a transition, or another higher-priority update), but none of those mutations are rolled back and the queued state update still runs.

That can make an uncommitted clear/re-paste count as a real re-entry: the committed composer still contains the old URL, yet its valid tag can be dropped, its upload generation invalidated, and the href left in blocked. If the corresponding resolver transition was also abandoned, nothing guarantees the required pending -> ready cycle that releases the block. After the two-second cap, the user can send a bare link despite having a valid snapshot before the abandoned render.

Please keep render pure. Model committed live-presence/generation as state or update it in a layout/effect phase, while retaining a synchronous submit fence derived from current content/state rather than mutating refs and scheduling state during render. Add a regression that starts and abandons/restarts a re-entry render (for example via a suspending sibling) and proves the previously committed tag remains sendable and no block/generation change leaks from the abandoned render.

[wesbillman]

Reviewing on Wes's behalf at exact head 50ead432d13ec0e2b6c97fbcec4fd2b886bbb6c1.

P1 — the ordinary blank-composer → paste path consumes “newness” before the debounced preview exists, so the negative cache is not invalidated.

useComposerLinkPreviews passes debounced candidates as previews, but live hrefs immediately (useComposerLinkPreviews.tsx:225-248). In useResolvedLinkPreviews, every effect run replaces seenHrefsRef with the live set (useResolvedLinkPreviews.ts:493-511), while invalidation can only happen inside for (const preview of previews) (:497-509).

Concrete production sequence:

1. Composer is mounted blank: previews=[], liveHrefs=[], seen={}.
2. User pastes URL A whose shared cache contains a negative. Before the 350ms debounce, previews=[] but liveHrefs=[A].
3. The effect has no preview to invalidate, yet sets seenHrefsRef.current = {A}.
4. After debounce, previews=[A] and liveHrefs=[A]; seen.has(A) is now true, so invalidation is skipped and the stale negative is reused.

That is the headline behavior this PR promises to fix. The composer regressions miss it because they mount the hook with URL A already in initialProps, which initializes debounced to A (useComposerLinkPreviews.test.mjs:123-130); they do not drive the normal mounted-empty → paste transition. The fast clear/re-paste test works only because debounced candidates remains [A] while the live set temporarily empties.

Required fix: do not mark a live href as seen/handled until there is a corresponding resolvable preview and its negative invalidation has been attempted (or track live presence separately from pending invalidation). Add a composer-level regression that mounts with empty content, seeds/caches a negative for A, then pastes A and proves the post-debounce load refetches rather than reusing the negative.

The generation fencing for the already-covered fast re-entry/upload races otherwise looks internally consistent, but this gap leaves the primary user gesture broken. CARL, THE DEBOUNCE ATE THE INVALIDATION. 👑🐈