chat: preview link unfurls before sending - #29571
Open
chrisnojima wants to merge 16 commits into
Open
Conversation
Paste a link in the composer and its unfurl card now appears above the input before the message is sent. Dismissing a card with the X suppresses that link's unfurl for that send. New chat1.local.unfurlPreviewLocal RPC runs the existing extractor, scraper and packager over unsent text and returns display-ready unfurls, so the preview reuses the same scrape and asset caches the post-send unfurl path already fills via Prefetch. Only whitelisted domains preview, and only generic unfurls; maps are excluded to match the message card. Suppression rides along on postTextNonblock as unfurlSuppress. The service stashes it against the message's outbox ID in a bounded LRU (200 entries, 5 minute TTL) and UnfurlAndSend skips those URLs. The entry is read without consuming so a second UnfurlAndSend, as happens when the user resolves an unfurl prompt on another link in the same message, still honors the dismissal. The composer snapshots the dismissed URLs before clearing the input rather than reading them at send time: clearing the input synchronously fires the draft throttle's leading edge, which drops the dismissed set before a setTimeout-scheduled send would have read it. A stellar payment cancel restores the set along with the text. Desktop only for now; the service side is platform neutral.
The preview was a full-width strip in flow above the composer, so showing one shifted the whole message list up. It is now an absolutely positioned panel anchored to the input's top edge: left aligned, shrink to fit, rounded border and a shadow, with a small gap above the input. Nothing in the thread moves when a preview appears or goes away. A message can contain several links, so the panel shows one card at a time with arrows in its top left to page between them. Every card renders into the same CSS grid cell, which keeps the panel sized to the largest one so paging never resizes it; the inactive cards are only hidden, which also keeps them out of the tab order and unclickable. The grid stacking is desktop only. Mobile still renders nothing here, and when it lands it needs the cross platform equivalent: measure each card with onLayout and hold the container at the max. gridArea joins the desktop style allowlist, which was only missing it.
Mobile rendered nothing at all: the hook was never called and the composer never fed it text. It now renders in flow above the input, inside KeyboardStickyView so it rides the keyboard, the way the reply preview already sits. The card area is a fixed size instead of hugging its content, so paging between cards of different shapes cannot resize the panel. That replaces the css grid that was holding the panel at the largest card: the grid sized the stack to the tallest card, which left a short card overflowing its own box and showing a scrollbar it did not need. It also drops the gridArea style allowlist entry, so css.d.ts is back to what master has. Only the card area scrolls now; the pager sits above it and stays put. Native clips at a fixed height rather than scrolling, so it gets a real ScrollView; desktop scrolls with overflowY. Both axes are set explicitly because the overflow shorthand would also set the y axis and beat the scroll depending on emission order. Drops the test that claimed to cover the mobile branch. Kb.Box2 renders a react-native Pressable when isMobile is set, which produces no DOM under jsdom, so it passed whether or not the guard it was testing existed. The mobile layout is verified on a device.
The input subtree is keyed on the conversation, so returning to a conversation mounts a fresh hook whose first render has empty text, before the draft is restored. The effect read that as "no links" and cleared every dismissed url for the conversation, so dismissing a card, switching away and switching back brought the card straight back and the link unfurled on send. Only prune once real text has been seen. The existing conversation-switch test could not catch this: it re-renders one mounted instance, where the real switch remounts. The new test drives an unmount and remount instead, and fails without the fix. Also from review: postTextNonblock now says so when it is handed unfurlSuppress with no outboxID. Suppression is keyed by outbox ID, and the deliverer generates its own when the caller supplies none, so that combination silently unfurled a link the sender had dismissed. No current caller does this. The pager had no coverage; it gets the counter, the arrows being inert at each end, and re-clamping when the shown card is dismissed away. Editing a message drops unfurlSuppress because postEditNonblock has no such parameter, so the dismiss control does nothing there. Noted at the call site rather than left for someone to rediscover.
A card stayed on screen after the link it described was edited out. The fetch that would have replaced it can fail or still be in flight, and nothing reset the previous result, so the composer could read one url while the card above it described another. Clicking that card's X then suppressed a link that was not in the message, and the link that was about to send never got offered a dismiss at all. Previews are now filtered against the current composer text, so a card survives only as long as its url does. That also makes the failure path honest: a fetch that throws shows nothing rather than leaving the last success on screen. This replaces the conversation masking that used to guard the same memo. Two reviewers independently found it unreachable: the input subtree is keyed on the conversation, so a hook instance never sees a second conversation, and the test defending it drove a rerender where the real app remounts. Filtering on the text covers the case that can actually happen. Adds the coverage those reviews found missing: clicking the close icon dismisses the url of the card on screen rather than the first one, and the generic-only/no-maps rule the frontend relies on is now a named predicate with a test, instead of an inline condition asserted nowhere.
Dismissed urls were held in a five minute cache keyed by outbox id. A message that waited longer than that to send - offline, airplane mode, a flaky connection - lost them, and unfurled links the sender had explicitly dismissed. The bound existed to stop entries from a send that never happened piling up for the life of the process, so it was memory hygiene doubling as a correctness cliff. The urls now ride SenderSendOptions. PostLocalNonblock puts them there, Queue persists them on the outbox record, and the deliverer hands them back to BlockingSender.Send, which is already the caller of UnfurlAndSend. So they live exactly as long as the message they belong to, survive a restart, and need no cleanup: the outbox record going away takes them with it. UnfurlAndSend runs again when the user resolves an unfurl prompt for another url in the same message, and that pass has no outbox record left to read. The first pass writes a marker at the url's existing deterministic task key, and later passes skip on that, so a dismissal holds however long the prompt sits unanswered. Giphy and maps are untouched. They are never previewed, so never dismissed, and the send path still does not filter by unfurl type. Text sends go back to letting the deliverer assign the outbox id, since suppression no longer keys on it. That reverts an unannounced change to every text send, and removes the case where suppression was dropped in silence because no id had been supplied.
… key suppressedKey was copied from statusKey and the prefix never changed, so both produced "s|<outboxID>" in the DBUnfurler namespace. markSuppressed writes a bool there and setStatus writes a task status enum, so whichever ran last for an outbox ID clobbered the other, and reading back the wrong type just fails - isSuppressed swallows that and reports not suppressed, un-dismissing a link. Nothing hits it today only because a suppressed URL never gets a task and so never gets a status, which is an invariant nothing states or enforces. The marker gets its own prefix. Suppression is also checked for every hit again, not only for the unfurl case. A queued message is classified against the whitelist at send time, so a URL dismissed while its domain was whitelisted can arrive as a prompt hit instead, and prompting for a link the sender declined breaks the same promise as unfurling it. Both paths now have a test, and each carries a positive control so a passing run cannot mean the pipeline is simply dead. The comment claiming the marker needs no cleanup was wrong: it is keyed per URL per message, nothing deletes it, and Complete only clears state for URLs that actually unfurled. Says so now, with what it costs. Also corrects the previewText comment, which described a ref it has nothing to do with.
The message helper hardcoded a message id, so the outbox id a caller passed had no effect on the suppression key, which derives from the url, the conversation and the message id. Both suppression tests were leaning on a different url to separate their cases while reading as though the outbox id did it, and the scenario that matters most could not be written at all: dismissing a link in one message must not suppress the same link in a later one. The helper takes a message id now, both tests use it for their positive control, and the message-scoping case is covered. Verified it fails when the key stops depending on the message.
Editing a message shows the preview card with a working looking X. The edit rpc carries no unfurlSuppress, so clicking it hid the card and changed nothing: the link still unfurled on save. A control that removes itself and silently does nothing is worse than no control, and worse than the missing card a failed scrape already produces, because it actively promises something. The card still renders while editing, since it is useful to see what will unfurl. It just has no X there. Closing the gap properly needs unfurlSuppress on postEditNonblock; this only stops lying about it in the meantime.
Prefetch holds prefetchLock for its whole body, so the scrape, package and asset upload it does are serialized. PreviewURLs took no lock at all, and it is a synchronous rpc rather than a goroutine fired and forgotten, so it can be entered again while an earlier call is still running. The composer does exactly that whenever a link is edited before its fetch comes back: the client discards the stale response by request id, but the service had already fetched the page, downloaded the image and uploaded the assets for a card nobody would see. Preview scrapes now go through a singleflight keyed by uid, conversation and url, so overlapping calls for the same link share one fetch. Uses the groupcache singleflight already in the tree rather than adding a second implementation. This does not bound a caller that ignores the client and asks for many distinct urls at once. That would want a semaphore, and it only matters to something that already has the local socket.
chrisnojima
force-pushed
the
nojima/HOTPOT-link-preview
branch
from
August 25, 2026 20:24
9d0aeac to
08aa6ad
Compare
PreviewURLs collapses concurrent scrapes of the same url through a singleflight, but the shared work ran on whichever caller happened to win the group. That caller is the one most likely to go away: the composer issues a new PreviewURLs on every edit and abandons the one in flight, so its cancellation would surface as an error for every other caller waiting on the same url. Run the scrape on a context detached from cancellation and let each caller wait on its own context instead, so a caller that leaves stops waiting rather than taking the result away from everyone else. Bail out of the remaining hits once the caller is gone. Also key the group on hex rather than the raw bytes of the uid and convID, either of which can contain the separator.
Gates the test server mid-scrape so a second caller joins the first one's singleflight, then cancels the first. Asserts the cancelled caller returns nothing, the survivor still gets its preview, and one scrape served both. This pins the collapse and the cancelled caller's own return value. It does not discriminate the detached context on its own: colly takes no context, so cancellation never reaches the scrape stage, and the packaging stage that does honour it is stubbed out here.
PreviewURLs dropped a url it failed to scrape or package, so the composer showed no card for it and the send unfurled it anyway: UnfurlAndSend queues and retries that url for minutes, landing a card the user never saw and had no way to decline. Those urls now come back with a nil unfurl (the avdl field becomes a union with null). The client keeps them in a `failed` set, separate from the dismissed set so a url that starts scraping again returns as a card instead of staying suppressed, and getSuppressedURLs unions both so the message unfurls exactly what the composer offered. Also puts focus back in the composer after a dismiss, since the X takes it.
Review of the previous commit turned up three ways the failed set could misbehave, plus one url class it should never have covered. A canceled stellar send restores the snapshot it took, and that snapshot had already been flattened into a single list, so a scrape failure came back as a user dismissal. A dismissal is never re-derived, so the url stayed suppressed for the rest of the conversation with no card to un-suppress it. The send now carries the two sets apart and restores only the dismissals; the failures come back on their own from the next fetch. The request guard lives in a ref, so it only discriminated against a mount's own older fetches. Leaving a conversation and returning remounts the hook, and the fetch the dead mount left in flight still matched its own id and wrote over what the new mount had since fetched, in either direction. Unmounting now retires the mount's ids. A giphy or a maps url gets no card either way, which is why previewable() excludes them, but a scrape failure on one was still reported and suppressed -- losing an unfurl the send's own retries would have landed. Those are skipped now, matching the success path. Tests: the failed set replacing wholesale rather than accumulating, the set clearing when the link leaves the text, the retired-mount response, the restore path not recording a failure as a dismissal, the union reaching the wire, and the auto-whitelist skip. Each was mutation-checked against the unfixed code. Also comments why sending before the previews land suppresses nothing.
A second review round found the escape hatch from the last commit was cut on the wrong line, and turned up four older mismatches between what the composer shows and what the message posts. The skip for urls that get no card asked the auto-whitelist, which is an exact domain match, while the success path asks ClassifyDomain, which also calls gph.is and any giphy TLD a giphy. A giphy short link whose scrape blipped was therefore suppressed, losing the unfurl the send's own retries would have landed. Both paths now read the same rule, named `carded` for the half of it that is knowable before a scrape. Request ids were per mount and the retirement rewound the counter, so an id could be handed out twice: freezing a screen tears effects down and rebuilds them on the same ref, and a fetch the old one left running would match the new one's id. Ids are now module-wide and never reused. The send's snapshot owned more than it should: it restored dismissals it never took (any send with no snapshot, a coinflip resend, picked up the composer's), and dropped dismissals it never took either, including one the user made while that send was in flight. It carries only what it snapshotted now. The preview no longer renders while editing at all. An edit posts as MessageType_EDIT, which the unfurler does not extract urls from, so the card promised an unfurl the edit could never produce; disabling its X was not enough, and `canDismiss` goes with it. A card also survived the user typing on past its url, since the old url is a prefix of the new one and the check was a substring test. Its X would have suppressed a link the message no longer contained. The composer previews on every debounced edit, and edits of one url are all different singleflight keys, so nothing collapsed them: the detached scrapes are now bounded by a slot limit, a failure is remembered for 30s so a dead link is not re-fetched per keystroke, and Prefetch shares the same singleflight instead of duplicating the scrape the composer is already running. Tests: carded and previewable pinned directly, the failure cache, the keep-list keepOnly honours, the typed-past card, punctuation after a url, the dedup in the suppress list, a snapshot-less send, a dismissal made mid-send, the snapshot preceding the composer clear, and no preview while editing. Each was mutation-checked, including the four that survived the first attempt.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Paste a link in the chat composer and its unfurl card now appears above the input before you send. If you don't want that link unfurled, dismiss the card and it won't be.
Desktop only for the UI polish pass; the service side is platform neutral and mobile renders it too.
How it works
A new
chat.1.local.unfurlPreviewLocalRPC runs the existing extractor, scraper and packager over unsent composer text and returns display-ready unfurls. It reusesdisplay.DisplayUnfurl, the same conversion the post-send path uses, so the preview card and the posted card cannot drift apart. Only whitelisted domains preview, and only generic unfurls — maps are excluded to match what the message card itself refuses to render.The dismissed urls ride the message's outbox record on
SenderSendOptions, soQueuepersists them with the queued message and the deliverer hands them back toBlockingSender.Send, which already callsUnfurlAndSend. A message that waits offline still knows what you dismissed when it finally goes out.UnfurlAndSendruns again when you resolve an unfurl prompt for another link in the same message, and that pass has no outbox record left, so the first pass leaves a marker at the url's deterministic per-url key and later passes skip on that.Suppression never posts and then retracts an unfurl: the unfurl message simply never gets created.
Giphy, maps and youtube links are untouched and unfurl exactly as they do today. They are never previewed, so never dismissable.
What the composer could not preview
A url the service fails to scrape or package comes back with a nil
unfurl(UnfurlPreviewInfo.unfurlis nowunion { null, UnfurlDisplay }) rather than being dropped. Dropping it was not neutral: the send would still queue that url and retry it in the background for minutes, so a card the user never saw could land in the sent message with no way to have declined it. The client suppresses those urls, so the message unfurls exactly the cards the composer offered.The client keeps them apart from the user's dismissals. A failure is re-derived by the next fetch and a dismissal never is, so recording a failure as a dismissal would bury the url for good — it could never come back as a card once it started scraping again. A send carries both sets as one snapshot, sends their union, and restores only the dismissals if it never posts.
The rule for which failures get reported is the one
previewableapplies to a scraped unfurl, read from the domain alone: a giphy or maps link gets no card either way, so a transient scrape failure on one must not suppress an unfurl the send's own retries would have landed. Both sides readClassifyDomain, so a giphy short link is treated as a giphy.Notes for review
Prefetchalready did on every draft update, and both now share one singleflight keyed by uid, conversation and url, so the two cannot duplicate a scrape of the same text.styles/css.d.tsends up identical to master; it was touched mid branch and reverted.Known gaps
www.example.comis decorated as a clickable link but never unfurls, on this branch and on master.DecorateWithLinksusesxurls.Relaxed()while the unfurl extractor usesxurls.Strict(), which requires a scheme, so the url is invisible to the whole unfurl path — no card, no prompt, no unfurl. Unifying them is not a one-liner:Relaxed()also matches things likefoo.pyand1.5.2, which should not be scraped. Left alone here.MessageType_EDIT, which the unfurler does not extract urls from, so a card would promise an unfurl the edit cannot produce.Completeonly clears state for urls that actually unfurled, and a suppressed url never gets that far.Testing
yarn lint:allclean at the repo's 0 bailout baseline, 121 chat jest tests,go build/go vet ./chat/...clean,golangci-lint --new-from-rev masterclean, and the unfurl package tests including preview extraction, suppression across a second pass, suppression of prompt hits, message scoping, a cancelled caller sharing a scrape, the failure cache, and the two domain rules pinned directly.Every test added in the last three commits was mutation checked: the fix was reverted in the production code and the test confirmed to fail, then restored. Four tests did not fail on the first attempt and were rewritten until they did — one of those drove the request id change above.
TestPackagerfails locally on a golden image byte comparison, on this branch and on master alike; unrelated to these changes.🤖 Generated with Claude Code