From 967d3a695446a7514bdfeeb113df4e33ef676a78 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Fri, 7 Aug 2026 14:15:34 -0700 Subject: [PATCH 1/7] fix(composer): land link preview snapshots sent from a resolved-looking card The composer card flipped to a "done" state on local metadata resolution, but the sendable snapshot tag only exists after the snapshot media finishes uploading to the relay. Sending during that window shipped a bare link with no snapshot tag, so the message rendered without an inline preview even though the composer looked ready. Make the card claim "done" only once the snapshot tag is actually built, and have Send briefly wait (capped) for in-flight snapshot uploads so a fast Enter still lands the preview. The intentional no-wait path for genuinely-pending metadata is preserved. Co-authored-by: Taylor Ho Signed-off-by: Taylor Ho --- .../features/messages/ui/MessageComposer.tsx | 2 +- .../messages/ui/useComposerLinkPreviews.tsx | 57 +++++++++++- desktop/src/testing/e2eBridge.ts | 7 ++ desktop/tests/e2e/messaging.spec.ts | 91 +++++++++++++++---- 4 files changed, 132 insertions(+), 25 deletions(-) diff --git a/desktop/src/features/messages/ui/MessageComposer.tsx b/desktop/src/features/messages/ui/MessageComposer.tsx index 0dc289eefaf..c36be10fa88 100644 --- a/desktop/src/features/messages/ui/MessageComposer.tsx +++ b/desktop/src/features/messages/ui/MessageComposer.tsx @@ -582,7 +582,7 @@ function MessageComposerImpl({ capturedThreadContext, pendingImeta: currentPendingImeta, queuedAttachments: currentQueuedAttachments, - linkPreviewTags: getReadyLinkPreviewTags(), + linkPreviewTags: await getReadyLinkPreviewTags(), sentDraftKey: resolveSentDraftKey( effectiveDraftKeyRef.current, drafts.loadDraft, diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx index 39d5d9db8ee..7e49020ecdf 100644 --- a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx @@ -24,6 +24,12 @@ import { } from "@/shared/ui/attachment"; import { Button } from "@/shared/ui/button"; +// Upper bound on how long Send waits for in-flight snapshot uploads before +// giving up and sending without the not-yet-ready tag. Keeps a fast Enter from +// dropping a preview whose upload is nearly done, without letting a stalled +// relay upload hang the message indefinitely. +const SNAPSHOT_SEND_WAIT_CAP_MS = 2000; + function previewHostname(href: string): string { try { return new URL(href).hostname.replace(/^www\./, ""); @@ -34,8 +40,10 @@ function previewHostname(href: string): string { function ComposerLinkPreviewCard({ preview, + tagReady, }: { preview: ResolvedLinkPreview; + tagReady: boolean; }) { const imageSrc = preview.imageState === "image" ? preview.imageDataUrl : null; const [failedImageSrc, setFailedImageSrc] = React.useState( @@ -43,6 +51,11 @@ function ComposerLinkPreviewCard({ ); const showImage = Boolean(imageSrc && failedImageSrc !== imageSrc); const hostname = previewHostname(preview.href); + // The card only claims "done" once the sendable snapshot tag exists: metadata + // resolution alone does not survive Send (the snapshot media still has to + // finish uploading to the relay). `buzz://` entity links never snapshot, so + // `snapshotReady` stays false for them and they never falsely claim "done". + const sendReady = Boolean(preview.snapshotReady && tagReady); let path = ""; try { const url = new URL(preview.href); @@ -55,7 +68,8 @@ function ComposerLinkPreviewCard({ data-image-state={preview.imageState} data-link-preview={preview.kind} data-link-preview-composer-card="" - state={preview.snapshotReady ? "done" : "processing"} + data-snapshot-tag-ready={sendReady ? "true" : "false"} + state={sendReady ? "done" : "processing"} > ()); + // In-flight snapshot upload promises, keyed by href. Send awaits these (with + // a cap) so a snapshot that is mid-upload still lands its tag on the message. + const pendingUploadsRef = React.useRef(new Map>()); const activeHrefsRef = React.useRef(new Set()); activeHrefsRef.current = new Set(candidates.map((preview) => preview.href)); @@ -184,7 +201,7 @@ export function useComposerLinkPreviews(content: string) { ) continue; uploadsRef.current.add(preview.href); - void Promise.all([ + const uploadPromise = Promise.all([ uploadDataUrl(preview.imageDataUrl, "link-preview-image.png"), uploadDataUrl(preview.faviconDataUrl, "link-preview-favicon.png"), ]) @@ -201,10 +218,22 @@ export function useComposerLinkPreviews(content: string) { faviconSha256: favicon.sha256, }); if (!tag) return; + // Update the ref synchronously as well as state: Send awaits this + // promise and then reads `readyTagsByHrefRef`, which otherwise would + // not reflect the pending `setReadyTags` until the next render. + readyTagsByHrefRef.current = { + ...readyTagsByHrefRef.current, + [preview.href]: tag, + }; setReadyTags((current) => ({ ...current, [preview.href]: tag })); }) .catch(() => {}) - .finally(() => uploadsRef.current.delete(preview.href)); + .finally(() => { + uploadsRef.current.delete(preview.href); + pendingUploadsRef.current.delete(preview.href); + }); + pendingUploadsRef.current.set(preview.href, uploadPromise); + void uploadPromise; } }, [previews, readyTags]); @@ -223,7 +252,11 @@ export function useComposerLinkPreviews(content: string) {
{previews.map((preview) => ( - + ))}
) : null; - const getReadyTags = React.useCallback(() => { + // Send awaits this: give in-flight snapshot uploads a brief, capped chance to + // finish (so a fast Enter still lands the preview) before capturing the tags. + const getReadyTags = React.useCallback(async () => { if (suppressedRef.current) return [["link-preview", "none"]]; + const pending = [...activeHrefsRef.current] + .map((href) => pendingUploadsRef.current.get(href)) + .filter((promise): promise is Promise => Boolean(promise)); + if (pending.length > 0) { + await Promise.race([ + Promise.allSettled(pending), + new Promise((resolve) => { + setTimeout(resolve, SNAPSHOT_SEND_WAIT_CAP_MS); + }), + ]); + if (suppressedRef.current) return [["link-preview", "none"]]; + } return [...activeHrefsRef.current].flatMap((href) => { const tag = readyTagsByHrefRef.current[href]; return tag ? [tag] : []; diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index ed988de259d..a7c32ceec7b 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -365,6 +365,9 @@ type E2eConfig = { linkPreviewMetadataDelayMs?: number; /** Simulates native cold-cache startup work before the async response. */ linkPreviewMetadataStartBlockMs?: number; + /** Delays link-preview snapshot media uploads so specs can exercise the + * Send-time wait for in-flight snapshot uploads. */ + linkPreviewUploadDelayMs?: number; searchProfiles?: MockSearchProfileSeed[]; updateAvailable?: boolean; updateChannelDelayMs?: number; @@ -8930,6 +8933,10 @@ async function resolveMockUploadDescriptorForBytes( args: { data: number[] | Uint8Array; filename?: string | null }, config: E2eConfig | undefined, ): Promise { + const uploadDelayMs = config?.mock?.linkPreviewUploadDelayMs ?? 0; + if (uploadDelayMs > 0 && args.filename?.startsWith("link-preview-")) { + await new Promise((resolve) => setTimeout(resolve, uploadDelayMs)); + } const configured = config?.mock?.uploadDescriptors; if (configured !== undefined) { const descriptors = await resolveMockUploadDescriptors(config); diff --git a/desktop/tests/e2e/messaging.spec.ts b/desktop/tests/e2e/messaging.spec.ts index a0808e4a6af..ef2f3027bb1 100644 --- a/desktop/tests/e2e/messaging.spec.ts +++ b/desktop/tests/e2e/messaging.spec.ts @@ -220,33 +220,47 @@ test.beforeEach(async ({ page }, testInfo) => { imageDomain: null, }, } - : testInfo.title.includes("link preview") || - testInfo.title.includes("supported Compact") + : testInfo.title.includes( + "send waits for an in-flight link preview snapshot upload", + ) ? { linkPreviewMetadata: { title: "Buzz pull request", siteName: "GitHub", description: "A sender-authored preview snapshot.", - imageDataUrl: null, - imageDomain: null, + imageDataUrl: + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", + imageDomain: "opengraph.githubassets.com", }, - linkPreviewMetadataDelayMs: testInfo.title.includes( - "loading card before cold resolver work", - ) - ? 10_000 - : testInfo.title.includes("style defaults") || - testInfo.title.includes("send does not wait") || - testInfo.title.includes("attachment-sized") - ? 1_500 - : undefined, - linkPreviewMetadataStartBlockMs: - testInfo.title.includes( + linkPreviewUploadDelayMs: 400, + } + : testInfo.title.includes("link preview") || + testInfo.title.includes("supported Compact") + ? { + linkPreviewMetadata: { + title: "Buzz pull request", + siteName: "GitHub", + description: "A sender-authored preview snapshot.", + imageDataUrl: null, + imageDomain: null, + }, + linkPreviewMetadataDelayMs: testInfo.title.includes( "loading card before cold resolver work", ) - ? 150 - : undefined, - } - : undefined; + ? 10_000 + : testInfo.title.includes("style defaults") || + testInfo.title.includes("send does not wait") || + testInfo.title.includes("attachment-sized") + ? 1_500 + : undefined, + linkPreviewMetadataStartBlockMs: + testInfo.title.includes( + "loading card before cold resolver work", + ) + ? 150 + : undefined, + } + : undefined; const mock = testInfo.title.includes("unresolvable preview") ? { linkPreviewMetadata: null, linkPreviewMetadataDelayMs: 150 } : baseMock; @@ -721,6 +735,45 @@ test("send does not wait for a pending link preview snapshot", async ({ expect(linkPreviewTags ?? []).toEqual([]); }); +test("send waits for an in-flight link preview snapshot upload", async ({ + page, +}) => { + const previewUrl = "https://github.com/block/buzz/pull/3246"; + await page.goto("/"); + await page.getByTestId("channel-general").click(); + await page.getByTestId("message-input").fill(previewUrl); + + const composerPreviews = page.locator("[data-composer-link-previews]"); + const card = composerPreviews.locator("[data-link-preview-composer-card]"); + await expect(card).toBeVisible(); + // Metadata resolves (image painted) but the sendable tag is not ready yet: + // the snapshot media upload is still in flight, so the card must NOT claim + // "done" and no tag is captured. + await expect(card).toHaveAttribute("data-image-state", "image"); + await expect(card).toHaveAttribute("data-snapshot-tag-ready", "false"); + await expect(composerPreviews).toHaveAttribute( + "data-ready-snapshot-count", + "0", + ); + + // Sending immediately must still land the preview: Send waits for the + // in-flight upload rather than shipping a bare link. + await page.getByTestId("send-message").click(); + const row = page.getByTestId("message-row").last(); + await expect(row).toContainText(previewUrl); + await expect(row.locator("[data-link-preview]")).toBeVisible(); + + const linkPreviewTags = await page.evaluate(() => { + const call = [...(window.__BUZZ_E2E_COMMAND_PAYLOADS__ ?? [])] + .reverse() + .find((entry) => entry.command === "send_channel_message"); + return ( + call?.payload as { linkPreviewTags?: string[][] | null } | undefined + )?.linkPreviewTags; + }); + expect(linkPreviewTags?.map((tag) => tag[3])).toEqual([previewUrl]); +}); + test("hiding composer link previews suppresses the whole draft and emits the blanket marker", async ({ page, }) => { From 23a7d9760a25824e7f08605c726fa3e24dbe5f3c Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Fri, 7 Aug 2026 15:14:55 -0700 Subject: [PATCH 2/7] fix(composer): disable send while a link preview is still settling The send button flickered ready -> not-ready -> ready as a pasted link resolved: it showed ready before metadata came back, disabled once the snapshot upload started, then re-enabled when the tag landed. It never signalled 'not ready yet' during the initial metadata phase. Disable Send for the whole settling window (metadata pending, or snapshot uploading before its tag exists) so the button reads as one continuous not-ready state with no flicker. buzz:// links never snapshot, so they are excluded and never disable Send. A disable cap re-enables Send if a link's metadata or upload stalls, so a dead or slow link never traps the composer. Co-authored-by: Taylor Ho Signed-off-by: Taylor Ho --- .../features/messages/ui/MessageComposer.tsx | 28 +++++------- .../messages/ui/useComposerLinkPreviews.tsx | 39 +++++++++++++++- desktop/tests/e2e/messaging.spec.ts | 44 ++++++++++++++++--- 3 files changed, 87 insertions(+), 24 deletions(-) diff --git a/desktop/src/features/messages/ui/MessageComposer.tsx b/desktop/src/features/messages/ui/MessageComposer.tsx index c36be10fa88..ef95d5fb3a2 100644 --- a/desktop/src/features/messages/ui/MessageComposer.tsx +++ b/desktop/src/features/messages/ui/MessageComposer.tsx @@ -104,6 +104,7 @@ function MessageComposerImpl({ const { previewList: composerLinkPreviews, getReadyTags: getReadyLinkPreviewTags, + hasPendingSnapshots: hasPendingLinkPreviewSnapshots, } = useComposerLinkPreviews(deferredPreviewContent); const [isEmojiPickerOpen, setIsEmojiPickerOpen] = React.useState(false); const [isFormattingOpen, setIsFormattingOpen] = React.useState(false); @@ -802,23 +803,16 @@ function MessageComposerImpl({ }); }, [media.setPendingImeta, richText.editor, scrollComposerToBottom]); // ── Send button state ─────────────────────────────────────────────── - const sendDisabled = React.useMemo( - () => - composerDisabled || - media.isUploading || - mentionSendFlow.isPreparingMentionSend || - (isContentEmpty && - media.pendingImeta.length === 0 && - media.queuedAttachments.length === 0), - [ - composerDisabled, - media.isUploading, - mentionSendFlow.isPreparingMentionSend, - isContentEmpty, - media.pendingImeta.length, - media.queuedAttachments.length, - ], - ); + const hasNothingToSend = + isContentEmpty && + media.pendingImeta.length === 0 && + media.queuedAttachments.length === 0; + const sendDisabled = + composerDisabled || + media.isUploading || + hasPendingLinkPreviewSnapshots || + mentionSendFlow.isPreparingMentionSend || + hasNothingToSend; const handleCaptureSelection = React.useCallback(() => {}, []); const handlePaperclipClick = React.useCallback(() => { diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx index 7e49020ecdf..c14980333f2 100644 --- a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx @@ -30,6 +30,12 @@ import { Button } from "@/shared/ui/button"; // relay upload hang the message indefinitely. const SNAPSHOT_SEND_WAIT_CAP_MS = 2000; +// Upper bound on how long Send stays disabled while a preview is still settling +// (metadata resolving, or snapshot media uploading). Past this the button +// re-enables even if the tag never lands, so a dead or slow link never traps +// the composer — the send-wait cap above still gives the tag a final chance. +const SNAPSHOT_SETTLE_DISABLE_CAP_MS = 2000; + function previewHostname(href: string): string { try { return new URL(href).hostname.replace(/^www\./, ""); @@ -242,11 +248,42 @@ export function useComposerLinkPreviews(content: string) { : candidates.flatMap((candidate) => readyTags[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. + // Send stays disabled across the whole window so the button never flickers + // ready -> not-ready -> ready (buzz:// links never snapshot, so they never + // report settling). `imageState === "none"` is terminal (no snapshot), so it + // does not block. See the disable cap below for the dead/slow-link escape. + const hasSettlingSnapshots = + !suppressed && + previews.some( + (preview) => + !preview.href.startsWith("buzz://") && + (preview.imageState === "pending" || + (preview.snapshotReady && !readyTags[preview.href])), + ); + // Re-enable Send once the disable cap elapses even if a preview is still + // settling, so a link whose metadata or upload stalls never traps the + // composer. Resets whenever settling ends or the draft's previews change. + const [settleDisableExpired, setSettleDisableExpired] = React.useState(false); + React.useEffect(() => { + if (!hasSettlingSnapshots) { + setSettleDisableExpired(false); + return; + } + const timer = window.setTimeout( + () => setSettleDisableExpired(true), + SNAPSHOT_SETTLE_DISABLE_CAP_MS, + ); + return () => window.clearTimeout(timer); + }, [hasSettlingSnapshots]); + const hasPendingSnapshots = hasSettlingSnapshots && !settleDisableExpired; const hideAll = React.useCallback(() => setSuppressed(true), []); const previewList = previews.length ? (
@@ -295,5 +332,5 @@ export function useComposerLinkPreviews(content: string) { return tag ? [tag] : []; }); }, []); - return { previewList, getReadyTags }; + return { previewList, getReadyTags, hasPendingSnapshots }; } diff --git a/desktop/tests/e2e/messaging.spec.ts b/desktop/tests/e2e/messaging.spec.ts index ef2f3027bb1..6ca2c950ad9 100644 --- a/desktop/tests/e2e/messaging.spec.ts +++ b/desktop/tests/e2e/messaging.spec.ts @@ -232,7 +232,8 @@ test.beforeEach(async ({ page }, testInfo) => { "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", imageDomain: "opengraph.githubassets.com", }, - linkPreviewUploadDelayMs: 400, + linkPreviewMetadataDelayMs: 300, + linkPreviewUploadDelayMs: 1_200, } : testInfo.title.includes("link preview") || testInfo.title.includes("supported Compact") @@ -248,11 +249,12 @@ test.beforeEach(async ({ page }, testInfo) => { "loading card before cold resolver work", ) ? 10_000 - : testInfo.title.includes("style defaults") || - testInfo.title.includes("send does not wait") || - testInfo.title.includes("attachment-sized") - ? 1_500 - : undefined, + : testInfo.title.includes("send does not wait") + ? 3_000 + : testInfo.title.includes("style defaults") || + testInfo.title.includes("attachment-sized") + ? 1_500 + : undefined, linkPreviewMetadataStartBlockMs: testInfo.title.includes( "loading card before cold resolver work", @@ -719,6 +721,20 @@ test("send does not wait for a pending link preview snapshot", async ({ composerPreviews.locator('[data-link-preview="github-pull-request"]'), ).toHaveAttribute("data-image-state", "pending"); + // While metadata is still resolving Send is disabled so the button does not + // flicker ready -> not-ready. But a link whose metadata stalls must not trap + // the composer: past the disable cap Send re-enables even though the card is + // still pending, and sending ships a bare link with no snapshot tag. + await expect(page.getByTestId("send-message")).toBeDisabled(); + await expect(composerPreviews).toHaveAttribute( + "data-has-pending-snapshots", + "false", + ); + await expect( + composerPreviews.locator('[data-link-preview="github-pull-request"]'), + ).toHaveAttribute("data-image-state", "pending"); + await expect(page.getByTestId("send-message")).toBeEnabled(); + await page.getByTestId("send-message").click(); const row = page.getByTestId("message-row").last(); await expect(row).toContainText(previewUrl); @@ -746,6 +762,9 @@ test("send waits for an in-flight link preview snapshot upload", async ({ const composerPreviews = page.locator("[data-composer-link-previews]"); const card = composerPreviews.locator("[data-link-preview-composer-card]"); await expect(card).toBeVisible(); + // From the moment the preview appears (metadata still resolving) Send is + // disabled, so the button never flashes ready before the snapshot is built. + await expect(page.getByTestId("send-message")).toBeDisabled(); // Metadata resolves (image painted) but the sendable tag is not ready yet: // the snapshot media upload is still in flight, so the card must NOT claim // "done" and no tag is captured. @@ -755,6 +774,19 @@ test("send waits for an in-flight link preview snapshot upload", async ({ "data-ready-snapshot-count", "0", ); + await expect(composerPreviews).toHaveAttribute( + "data-has-pending-snapshots", + "true", + ); + // Send stays disabled across the whole settle window (pending -> uploading), + // mirroring the attachment-upload guard, so the button visibly reflects "not + // ready yet" without flickering. + await expect(page.getByTestId("send-message")).toBeDisabled(); + + // Once the upload settles the tag is captured, the card claims "done", and + // Send re-enables. + await expect(card).toHaveAttribute("data-snapshot-tag-ready", "true"); + await expect(page.getByTestId("send-message")).toBeEnabled(); // Sending immediately must still land the preview: Send waits for the // in-flight upload rather than shipping a bare link. From 0c6e2828cac79e753919b911808d15cd9c6c11ab Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Fri, 7 Aug 2026 17:19:09 -0700 Subject: [PATCH 3/7] fix(composer): guard link-preview send against races and per-media upload failure A resolved link could fail to render inline in the sent message, and rapid Enter could double-submit or ship a bare link ahead of its snapshot. This hardens the whole composer send path: - Take a synchronous submit lock before any await so a second Enter cannot race in and double-submit. - Enforce disabled-until-ready on every entry point (Enter, form, auto-submit) via a hasPendingSnapshots ref, not just the send button. - Exclude edit mode from preview resolution, upload, and gating (edits never persist snapshots), passing editTarget == null to disable the hook. - Debounce the preview-driving content so typing a URL character by character no longer churns a flickering card per keystroke. - Key the suppression reset on live candidates, not the debounced set, so "hide previews" cannot get stuck across clear-then-retype. - Upload thumbnail and favicon independently: a single failed upload degrades to the survivor instead of dropping both, with one toast naming what failed. Tests: real Enter-during-upload guard, rapid-Enter-exactly-once, paste-and-immediate-Enter, thumbnail-upload-failure degrades to favicon, and edit-mode-excludes-previews. Full messaging spec: 53 passed. Co-authored-by: Taylor Ho Signed-off-by: Taylor Ho --- .../features/messages/ui/MessageComposer.tsx | 22 +- .../messages/ui/useComposerLinkPreviews.tsx | 177 +++++++++--- desktop/src/testing/e2eBridge.ts | 14 +- desktop/tests/e2e/messaging.spec.ts | 268 +++++++++++++++--- desktop/tests/helpers/bridge.ts | 7 + 5 files changed, 390 insertions(+), 98 deletions(-) diff --git a/desktop/src/features/messages/ui/MessageComposer.tsx b/desktop/src/features/messages/ui/MessageComposer.tsx index ef95d5fb3a2..bb0286839c5 100644 --- a/desktop/src/features/messages/ui/MessageComposer.tsx +++ b/desktop/src/features/messages/ui/MessageComposer.tsx @@ -100,12 +100,13 @@ function MessageComposerImpl({ syncContentRefFromEditorRef, } = useComposerContentState(); const [previewContent, setPreviewContent] = React.useState(""); - const deferredPreviewContent = React.useDeferredValue(previewContent); const { previewList: composerLinkPreviews, getReadyTags: getReadyLinkPreviewTags, hasPendingSnapshots: hasPendingLinkPreviewSnapshots, - } = useComposerLinkPreviews(deferredPreviewContent); + // Ref lets the submit guard block Enter/form/auto-submit until snapshots settle. + hasPendingSnapshotsRef: hasPendingLinkPreviewSnapshotsRef, + } = useComposerLinkPreviews(previewContent, editTarget == null); const [isEmojiPickerOpen, setIsEmojiPickerOpen] = React.useState(false); const [isFormattingOpen, setIsFormattingOpen] = React.useState(false); const [spoileredAttachmentUrls, setSpoileredAttachmentUrls] = React.useState< @@ -199,6 +200,8 @@ function MessageComposerImpl({ const disabledRef = React.useRef(disabled); const isSendingRef = React.useRef(isSending); const isUploadingRef = React.useRef(media.isUploading); + // Sync lock: taken before any async send so rapid Enter can't double-submit. + const isSubmitLockedRef = React.useRef(false); const onSendRef = React.useRef(onSend); const onEditSaveRef = React.useRef(onEditSave); const onEditLastOwnMessageRef = React.useRef(onEditLastOwnMessage); @@ -563,7 +566,9 @@ function MessageComposerImpl({ (!trimmed && !hasMedia) || disabledRef.current || isSendingRef.current || + isSubmitLockedRef.current || isUploadingRef.current || + hasPendingLinkPreviewSnapshotsRef.current || mentionSendFlow.isPreparingMentionSend ) { return; @@ -575,6 +580,7 @@ function MessageComposerImpl({ ) { return; } + isSubmitLockedRef.current = true; onPreparingMentionSendChange?.(true); persistentMentionHydration.beginSubmit(); try { @@ -583,7 +589,7 @@ function MessageComposerImpl({ capturedThreadContext, pendingImeta: currentPendingImeta, queuedAttachments: currentQueuedAttachments, - linkPreviewTags: await getReadyLinkPreviewTags(), + linkPreviewTags: getReadyLinkPreviewTags(), sentDraftKey: resolveSentDraftKey( effectiveDraftKeyRef.current, drafts.loadDraft, @@ -595,6 +601,7 @@ function MessageComposerImpl({ audienceRevision: audienceScope ? persistentAudience.revision : null, }); } finally { + isSubmitLockedRef.current = false; persistentMentionHydration.endSubmit(); onPreparingMentionSendChange?.(false); } @@ -605,6 +612,7 @@ function MessageComposerImpl({ drafts.loadDraft, emojiAutocomplete.clearEmojis, getReadyLinkPreviewTags, + hasPendingLinkPreviewSnapshotsRef, media.clearQueuedAttachments, media.pendingImetaRef, media.queuedAttachmentsRef, @@ -803,16 +811,14 @@ function MessageComposerImpl({ }); }, [media.setPendingImeta, richText.editor, scrollComposerToBottom]); // ── Send button state ─────────────────────────────────────────────── - const hasNothingToSend = - isContentEmpty && - media.pendingImeta.length === 0 && - media.queuedAttachments.length === 0; const sendDisabled = composerDisabled || media.isUploading || hasPendingLinkPreviewSnapshots || mentionSendFlow.isPreparingMentionSend || - hasNothingToSend; + (isContentEmpty && + media.pendingImeta.length === 0 && + media.queuedAttachments.length === 0); const handleCaptureSelection = React.useCallback(() => {}, []); const handlePaperclipClick = React.useCallback(() => { diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx index c14980333f2..9f77a250737 100644 --- a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx @@ -1,5 +1,6 @@ import * as React from "react"; import { ImageOff, LoaderCircle, X } from "lucide-react"; +import { toast } from "sonner"; import { getRelayHttpUrl, uploadMediaBytes } from "@/shared/api/tauri"; import { extractSupportedLinkPreviews } from "@/shared/lib/linkPreview"; @@ -24,16 +25,15 @@ import { } from "@/shared/ui/attachment"; import { Button } from "@/shared/ui/button"; -// Upper bound on how long Send waits for in-flight snapshot uploads before -// giving up and sending without the not-yet-ready tag. Keeps a fast Enter from -// dropping a preview whose upload is nearly done, without letting a stalled -// relay upload hang the message indefinitely. -const SNAPSHOT_SEND_WAIT_CAP_MS = 2000; +// Idle time after the last keystroke before link-preview resolution runs, so +// typing a URL does not flicker a card per character (debounce, not throttle: +// throttle would still fire mid-type). +const LINK_PREVIEW_DEBOUNCE_MS = 350; // Upper bound on how long Send stays disabled while a preview is still settling // (metadata resolving, or snapshot media uploading). Past this the button // re-enables even if the tag never lands, so a dead or slow link never traps -// the composer — the send-wait cap above still gives the tag a final chance. +// the composer — the message then sends as a bare link. const SNAPSHOT_SETTLE_DISABLE_CAP_MS = 2000; function previewHostname(href: string): string { @@ -151,21 +151,73 @@ async function uploadDataUrl( return { url: uploaded.url, sha256: uploaded.sha256 }; } -export function useComposerLinkPreviews(content: string) { +// Upload one snapshot media (image or favicon) independently so a single +// failure degrades gracefully instead of dropping the whole preview: on +// failure we return empty url/sha256 (a valid "no media" snapshot field) and +// report which media failed so the caller can toast the user once. +async function uploadSnapshotMedia( + dataUrl: string | null | undefined, + filename: string, + label: "thumbnail" | "favicon", +): Promise<{ url: string; sha256: string; failed: null | typeof label }> { + try { + const { url, sha256 } = await uploadDataUrl(dataUrl, filename); + return { url, sha256, failed: null }; + } catch { + return { url: "", sha256: "", failed: dataUrl ? label : null }; + } +} + +export function useComposerLinkPreviews(content: string, enabled = true) { 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 + // keystroke. `content` is the live editor value; `debounced` is what actually + // resolves. A fast paste-and-Enter before the debounce fires is held by + // `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; + const timer = window.setTimeout( + () => setDebounced(content), + LINK_PREVIEW_DEBOUNCE_MS, + ); + return () => window.clearTimeout(timer); + }, [content]); + const extractCandidates = React.useCallback( + (source: string) => + enabled + ? extractSupportedLinkPreviews(source).filter((preview) => + preview.href.startsWith("buzz://") + ? true + : isValidLinkPreviewSnapshotCanonicalUrl(preview.href), + ) + : [], + [enabled], + ); const candidates = React.useMemo( - () => - extractSupportedLinkPreviews(content).filter((preview) => - preview.href.startsWith("buzz://") - ? true - : isValidLinkPreviewSnapshotCanonicalUrl(preview.href), - ), - [content], + () => extractCandidates(debounced), + [extractCandidates, debounced], + ); + // Supported candidates in the LIVE content. When these differ from what has + // 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 previews = useResolvedLinkPreviews(suppressed ? [] : candidates); + // 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; React.useEffect(() => { - if (candidates.length === 0) setSuppressed(false); - }, [candidates.length]); + if (liveCandidatesEmpty) setSuppressed(false); + }, [liveCandidatesEmpty]); const [readyTags, setReadyTags] = React.useState>( {}, ); @@ -175,9 +227,6 @@ export function useComposerLinkPreviews(content: string) { const suppressedRef = React.useRef(suppressed); suppressedRef.current = suppressed; const uploadsRef = React.useRef(new Set()); - // In-flight snapshot upload promises, keyed by href. Send awaits these (with - // a cap) so a snapshot that is mid-upload still lands its tag on the message. - const pendingUploadsRef = React.useRef(new Map>()); const activeHrefsRef = React.useRef(new Set()); activeHrefsRef.current = new Set(candidates.map((preview) => preview.href)); @@ -207,12 +256,33 @@ export function useComposerLinkPreviews(content: string) { ) continue; uploadsRef.current.add(preview.href); + // 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 + // text-only), so a partial or total media failure still ships a real + // inline preview and the card never spins forever. const uploadPromise = Promise.all([ - uploadDataUrl(preview.imageDataUrl, "link-preview-image.png"), - uploadDataUrl(preview.faviconDataUrl, "link-preview-favicon.png"), + uploadSnapshotMedia( + preview.imageDataUrl, + "link-preview-image.png", + "thumbnail", + ), + uploadSnapshotMedia( + preview.faviconDataUrl, + "link-preview-favicon.png", + "favicon", + ), ]) .then(([image, favicon]) => { if (!activeHrefsRef.current.has(preview.href)) return; + const failedMedia = [image.failed, favicon.failed].filter( + (label): label is "thumbnail" | "favicon" => label !== null, + ); + if (failedMedia.length > 0) { + toast.error( + `Something went wrong with the ${failedMedia.join(" and ")}`, + ); + } const tag = buildLinkPreviewSnapshotTag({ canonicalUrl: preview.href, title: preview.title, @@ -224,21 +294,17 @@ export function useComposerLinkPreviews(content: string) { faviconSha256: favicon.sha256, }); if (!tag) return; - // Update the ref synchronously as well as state: Send awaits this - // promise and then reads `readyTagsByHrefRef`, which otherwise would - // not reflect the pending `setReadyTags` until the next render. + // Update the ref alongside state so a submit reading + // `readyTagsByHrefRef` sees the tag before the next render commits. readyTagsByHrefRef.current = { ...readyTagsByHrefRef.current, [preview.href]: tag, }; setReadyTags((current) => ({ ...current, [preview.href]: tag })); }) - .catch(() => {}) .finally(() => { uploadsRef.current.delete(preview.href); - pendingUploadsRef.current.delete(preview.href); }); - pendingUploadsRef.current.set(preview.href, uploadPromise); void uploadPromise; } }, [previews, readyTags]); @@ -254,7 +320,7 @@ export function useComposerLinkPreviews(content: string) { // ready -> not-ready -> ready (buzz:// links never snapshot, so they never // report settling). `imageState === "none"` is terminal (no snapshot), so it // does not block. See the disable cap below for the dead/slow-link escape. - const hasSettlingSnapshots = + const hasResolvingSnapshots = !suppressed && previews.some( (preview) => @@ -262,22 +328,43 @@ export function useComposerLinkPreviews(content: string) { (preview.imageState === "pending" || (preview.snapshotReady && !readyTags[preview.href])), ); + // A supported link in the LIVE content that resolution has not caught up to + // yet (debounce pending, or resolved for an older revision) also counts as + // settling — otherwise a paste-and-immediate-Enter would ship a bare link + // before resolution even starts. buzz:// links never snapshot, so ignore them. + const hasUnresolvedLiveCandidates = + !suppressed && + liveCandidatesRef.current.some( + (href) => + !href.startsWith("buzz://") && + !readyTags[href] && + !candidates.some((candidate) => candidate.href === href), + ); + const hasSettlingSnapshots = + hasResolvingSnapshots || hasUnresolvedLiveCandidates; // Re-enable Send once the disable cap elapses even if a preview is still // settling, so a link whose metadata or upload stalls never traps the - // composer. Resets whenever settling ends or the draft's previews change. + // 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) { setSettleDisableExpired(false); return; } + setSettleDisableExpired(false); const timer = window.setTimeout( () => setSettleDisableExpired(true), SNAPSHOT_SETTLE_DISABLE_CAP_MS, ); return () => window.clearTimeout(timer); - }, [hasSettlingSnapshots]); + }, [hasSettlingSnapshots, liveCandidatesKey]); const hasPendingSnapshots = hasSettlingSnapshots && !settleDisableExpired; + // Ref mirror so a synchronous submit guard can read the pending state on any + // entry point (Enter, form, auto-submit), not just the reactive button prop. + const hasPendingSnapshotsRef = React.useRef(hasPendingSnapshots); + hasPendingSnapshotsRef.current = hasPendingSnapshots; const hideAll = React.useCallback(() => setSuppressed(true), []); const previewList = previews.length ? (
) : null; - // Send awaits this: give in-flight snapshot uploads a brief, capped chance to - // finish (so a fast Enter still lands the preview) before capturing the tags. - const getReadyTags = React.useCallback(async () => { + // Snapshot tags for a submit, read synchronously at submit start from the href + // set currently backing the previews (activeHrefsRef) — so the tags always + // correspond to the content being sent. 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. A suppressed preview list emits + // the "none" marker; hrefs without a ready tag (dead/slow link past the cap) + // are omitted and the message sends as a bare link. + const getReadyTags = React.useCallback(() => { if (suppressedRef.current) return [["link-preview", "none"]]; - const pending = [...activeHrefsRef.current] - .map((href) => pendingUploadsRef.current.get(href)) - .filter((promise): promise is Promise => Boolean(promise)); - if (pending.length > 0) { - await Promise.race([ - Promise.allSettled(pending), - new Promise((resolve) => { - setTimeout(resolve, SNAPSHOT_SEND_WAIT_CAP_MS); - }), - ]); - if (suppressedRef.current) return [["link-preview", "none"]]; - } return [...activeHrefsRef.current].flatMap((href) => { const tag = readyTagsByHrefRef.current[href]; return tag ? [tag] : []; }); }, []); - return { previewList, getReadyTags, hasPendingSnapshots }; + return { + previewList, + getReadyTags, + hasPendingSnapshots, + hasPendingSnapshotsRef, + }; } diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index a7c32ceec7b..c466985e47d 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -368,6 +368,10 @@ type E2eConfig = { /** Delays link-preview snapshot media uploads so specs can exercise the * Send-time wait for in-flight snapshot uploads. */ linkPreviewUploadDelayMs?: number; + /** Substrings of `link-preview-*` upload filenames whose `upload_media_bytes` + * call should reject, so specs can drive a per-media snapshot upload failure + * (e.g. `["link-preview-image"]` fails only the thumbnail, favicon survives). */ + linkPreviewUploadErrorFilenames?: string[]; searchProfiles?: MockSearchProfileSeed[]; updateAvailable?: boolean; updateChannelDelayMs?: number; @@ -8934,8 +8938,14 @@ async function resolveMockUploadDescriptorForBytes( config: E2eConfig | undefined, ): Promise { const uploadDelayMs = config?.mock?.linkPreviewUploadDelayMs ?? 0; - if (uploadDelayMs > 0 && args.filename?.startsWith("link-preview-")) { - await new Promise((resolve) => setTimeout(resolve, uploadDelayMs)); + if (args.filename?.startsWith("link-preview-")) { + if (uploadDelayMs > 0) { + await new Promise((resolve) => setTimeout(resolve, uploadDelayMs)); + } + const errorFilenames = config?.mock?.linkPreviewUploadErrorFilenames; + if (errorFilenames?.some((needle) => args.filename?.includes(needle))) { + throw new Error(`mock upload failed for ${args.filename}`); + } } const configured = config?.mock?.uploadDescriptors; if (configured !== undefined) { diff --git a/desktop/tests/e2e/messaging.spec.ts b/desktop/tests/e2e/messaging.spec.ts index 6ca2c950ad9..885729f5318 100644 --- a/desktop/tests/e2e/messaging.spec.ts +++ b/desktop/tests/e2e/messaging.spec.ts @@ -221,7 +221,7 @@ test.beforeEach(async ({ page }, testInfo) => { }, } : testInfo.title.includes( - "send waits for an in-flight link preview snapshot upload", + "Enter during an in-flight snapshot upload", ) ? { linkPreviewMetadata: { @@ -235,36 +235,57 @@ test.beforeEach(async ({ page }, testInfo) => { linkPreviewMetadataDelayMs: 300, linkPreviewUploadDelayMs: 1_200, } - : testInfo.title.includes("link preview") || - testInfo.title.includes("supported Compact") + : testInfo.title.includes( + "snapshot thumbnail upload failure", + ) ? { linkPreviewMetadata: { title: "Buzz pull request", siteName: "GitHub", description: "A sender-authored preview snapshot.", - imageDataUrl: null, - imageDomain: null, + imageDataUrl: + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", + imageDomain: "opengraph.githubassets.com", + faviconDataUrl: + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", }, - linkPreviewMetadataDelayMs: testInfo.title.includes( - "loading card before cold resolver work", - ) - ? 10_000 - : testInfo.title.includes("send does not wait") - ? 3_000 - : testInfo.title.includes("style defaults") || - testInfo.title.includes("attachment-sized") - ? 1_500 - : undefined, - linkPreviewMetadataStartBlockMs: - testInfo.title.includes( + // Fail only the thumbnail upload; the favicon survives, + // so the snapshot degrades to a favicon-only preview. + linkPreviewUploadErrorFilenames: [ + "link-preview-image", + ], + } + : testInfo.title.includes("link preview") || + testInfo.title.includes("supported Compact") + ? { + linkPreviewMetadata: { + title: "Buzz pull request", + siteName: "GitHub", + description: + "A sender-authored preview snapshot.", + imageDataUrl: null, + imageDomain: null, + }, + linkPreviewMetadataDelayMs: testInfo.title.includes( "loading card before cold resolver work", ) - ? 150 - : undefined, - } - : undefined; + ? 10_000 + : testInfo.title.includes("send does not wait") + ? 3_000 + : testInfo.title.includes("style defaults") || + testInfo.title.includes("attachment-sized") + ? 1_500 + : undefined, + linkPreviewMetadataStartBlockMs: + testInfo.title.includes( + "loading card before cold resolver work", + ) + ? 150 + : undefined, + } + : undefined; const mock = testInfo.title.includes("unresolvable preview") - ? { linkPreviewMetadata: null, linkPreviewMetadataDelayMs: 150 } + ? { linkPreviewMetadata: null, linkPreviewMetadataDelayMs: 800 } : baseMock; await installMockBridge(page, mock); }); @@ -751,46 +772,126 @@ test("send does not wait for a pending link preview snapshot", async ({ expect(linkPreviewTags ?? []).toEqual([]); }); -test("send waits for an in-flight link preview snapshot upload", async ({ +test("Enter during an in-flight snapshot upload cannot ship a bare link", async ({ page, }) => { const previewUrl = "https://github.com/block/buzz/pull/3246"; await page.goto("/"); await page.getByTestId("channel-general").click(); - await page.getByTestId("message-input").fill(previewUrl); + const input = page.getByTestId("message-input"); + await input.fill(previewUrl); const composerPreviews = page.locator("[data-composer-link-previews]"); const card = composerPreviews.locator("[data-link-preview-composer-card]"); await expect(card).toBeVisible(); - // From the moment the preview appears (metadata still resolving) Send is - // disabled, so the button never flashes ready before the snapshot is built. - await expect(page.getByTestId("send-message")).toBeDisabled(); // Metadata resolves (image painted) but the sendable tag is not ready yet: - // the snapshot media upload is still in flight, so the card must NOT claim - // "done" and no tag is captured. + // the snapshot media upload is still in flight (linkPreviewUploadDelayMs), so + // the composer reports the preview as still pending. await expect(card).toHaveAttribute("data-image-state", "image"); await expect(card).toHaveAttribute("data-snapshot-tag-ready", "false"); - await expect(composerPreviews).toHaveAttribute( - "data-ready-snapshot-count", - "0", - ); await expect(composerPreviews).toHaveAttribute( "data-has-pending-snapshots", "true", ); - // Send stays disabled across the whole settle window (pending -> uploading), - // mirroring the attachment-upload guard, so the button visibly reflects "not - // ready yet" without flickering. - await expect(page.getByTestId("send-message")).toBeDisabled(); - // Once the upload settles the tag is captured, the card claims "done", and - // Send re-enables. + // Drive Enter (not a disabled-button click, which the browser swallows on its + // own) while the upload is deterministically in flight. The synchronous submit + // guard must reject it: no send_channel_message call may occur before the tag + // is ready, or the link would ship bare. This is the core Enter-bypass fix — + // the disabled state is enforced on the keyboard path, not just the button. + await expect(input).toBeFocused(); + await input.press("Enter"); + await input.press("Enter"); + const sendsDuringUpload = await page.evaluate( + () => + (window.__BUZZ_E2E_COMMAND_PAYLOADS__ ?? []).filter( + (entry) => entry.command === "send_channel_message", + ).length, + ); + expect(sendsDuringUpload).toBe(0); + + // Once the upload settles the tag is captured and Send re-enables. Sending + // now lands the preview snapshot matching the body. await expect(card).toHaveAttribute("data-snapshot-tag-ready", "true"); await expect(page.getByTestId("send-message")).toBeEnabled(); + await input.press("Enter"); + const row = page.getByTestId("message-row").last(); + await expect(row).toContainText(previewUrl); + await expect(row.locator("[data-link-preview]")).toBeVisible(); - // Sending immediately must still land the preview: Send waits for the - // in-flight upload rather than shipping a bare link. - await page.getByTestId("send-message").click(); + const linkPreviewTags = await page.evaluate(() => { + const call = [...(window.__BUZZ_E2E_COMMAND_PAYLOADS__ ?? [])] + .reverse() + .find((entry) => entry.command === "send_channel_message"); + return ( + call?.payload as { linkPreviewTags?: string[][] | null } | undefined + )?.linkPreviewTags; + }); + expect(linkPreviewTags?.map((tag) => tag[3])).toEqual([previewUrl]); +}); + +test("rapid Enter presses on a ready link preview send exactly once", async ({ + page, +}) => { + const previewUrl = "https://github.com/block/buzz/pull/3246?rapid=1"; + await page.goto("/"); + await page.getByTestId("channel-general").click(); + const input = page.getByTestId("message-input"); + await input.fill(previewUrl); + + // Wait until the snapshot is fully ready and Send is enabled, so the only + // thing under test is the composer-local send lock — not preview settling. + await waitForReadyComposerSnapshots(page); + await expect(page.getByTestId("send-message")).toBeEnabled(); + + // Mash Enter. The synchronous submit lock (isSubmitLockedRef), acquired before + // any await, must collapse these into exactly one send_channel_message so a + // duplicate cannot clear shared prep/hydration state mid-send. + await input.press("Enter"); + await input.press("Enter"); + await input.press("Enter"); + + const row = page.getByTestId("message-row").last(); + await expect(row).toContainText(previewUrl); + await expect(row.locator("[data-link-preview]")).toBeVisible(); + + await expect + .poll(async () => + page.evaluate( + () => + (window.__BUZZ_E2E_COMMAND_PAYLOADS__ ?? []).filter( + (entry) => entry.command === "send_channel_message", + ).length, + ), + ) + .toBe(1); +}); + +test("pasting a link preview and immediately pressing Enter waits for resolution", async ({ + page, +}) => { + const previewUrl = "https://github.com/block/buzz/pull/3246?fast=send"; + await page.goto("/"); + await page.getByTestId("channel-general").click(); + const input = page.getByTestId("message-input"); + + // Fill the URL and press Enter within the debounce window, before resolution + // has even started. The live-candidate guard must treat the unresolved link + // as pending and reject the Enter, so the message cannot ship bare. + await input.fill(previewUrl); + await input.press("Enter"); + const sendsBeforeResolution = await page.evaluate( + () => + (window.__BUZZ_E2E_COMMAND_PAYLOADS__ ?? []).filter( + (entry) => entry.command === "send_channel_message", + ).length, + ); + expect(sendsBeforeResolution).toBe(0); + + // The debounce fires, resolution + upload complete, and only then does Send + // become available. A press now lands the snapshot. + await waitForReadyComposerSnapshots(page); + await input.press("Enter"); const row = page.getByTestId("message-row").last(); await expect(row).toContainText(previewUrl); await expect(row.locator("[data-link-preview]")).toBeVisible(); @@ -806,6 +907,89 @@ test("send waits for an in-flight link preview snapshot upload", async ({ expect(linkPreviewTags?.map((tag) => tag[3])).toEqual([previewUrl]); }); +test("a snapshot thumbnail upload failure toasts and still sends with the favicon", async ({ + page, +}) => { + const previewUrl = "https://github.com/block/buzz/pull/3246?upload=fail"; + await page.goto("/"); + await page.getByTestId("channel-general").click(); + const input = page.getByTestId("message-input"); + await input.fill(previewUrl); + + // The thumbnail upload is configured to reject while the favicon succeeds. + // The preview must degrade to the surviving favicon rather than dropping the + // whole card or spinning forever: a tag still lands, Send still enables. + await waitForReadyComposerSnapshots(page); + await expect( + page + .locator("[data-sonner-toast]") + .filter({ hasText: "Something went wrong with the thumbnail" }), + ).toBeVisible(); + await expect(page.getByTestId("send-message")).toBeEnabled(); + + await input.press("Enter"); + const row = page.getByTestId("message-row").last(); + await expect(row).toContainText(previewUrl); + await expect(row.locator("[data-link-preview]")).toBeVisible(); + + // The snapshot tag exists (survivor media) but carries no image url — proving + // the graceful per-media degrade rather than a dropped or all-or-nothing tag. + const imageUrl = await page.evaluate(() => { + const call = [...(window.__BUZZ_E2E_COMMAND_PAYLOADS__ ?? [])] + .reverse() + .find((entry) => entry.command === "send_channel_message"); + const tags = ( + call?.payload as { linkPreviewTags?: string[][] | null } | undefined + )?.linkPreviewTags; + const snapshot = tags?.find( + (tag) => tag[0] === "link-preview" && tag[1] === "snapshot", + ); + // Snapshot tag layout: ["link-preview","snapshot",,,...pairs]. + const pairs = snapshot?.slice(4) ?? []; + const imageIndex = pairs.indexOf("image"); + return imageIndex >= 0 ? pairs[imageIndex + 1] : null; + }); + expect(imageUrl).toBeFalsy(); +}); + +test("editing a message excludes link previews entirely", async ({ page }) => { + const message = `Edit-me ${Date.now()}`; + const previewUrl = "https://github.com/block/buzz/pull/3246?edit=1"; + await page.goto("/"); + await page.getByTestId("channel-general").click(); + const input = page.getByTestId("message-input"); + + // Send a plain message with no link, then edit it to add a supported URL. + await input.fill(message); + await input.press("Enter"); + await expect(page.getByTestId("message-timeline")).toContainText(message); + + await expect(input).toBeFocused(); + await page.keyboard.press("ArrowUp"); + await expect(page.getByTestId("edit-target")).toBeVisible(); + + // Adding a link while editing must NOT resolve, upload, gate Save, or render a + // composer preview card — edit mode does not persist snapshots (decision A). + await input.fill(`${message} ${previewUrl}`); + await expect(page.locator("[data-composer-link-previews]")).toHaveCount(0); + // No snapshot upload was attempted for the edited link. + const uploadedPreviewMedia = await page.evaluate( + () => + (window.__BUZZ_E2E_COMMAND_PAYLOADS__ ?? []).filter( + (entry) => + entry.command === "upload_media_bytes" && + typeof (entry.payload as { filename?: string })?.filename === + "string" && + (entry.payload as { filename: string }).filename.startsWith( + "link-preview-", + ), + ).length, + ); + expect(uploadedPreviewMedia).toBe(0); + // Save is not blocked waiting on a snapshot the edit will never emit. + await expect(page.getByTestId("send-message")).toBeEnabled(); +}); + test("hiding composer link previews suppresses the whole draft and emits the blanket marker", async ({ page, }) => { diff --git a/desktop/tests/helpers/bridge.ts b/desktop/tests/helpers/bridge.ts index fe0c1bf2a6e..50f792447c0 100644 --- a/desktop/tests/helpers/bridge.ts +++ b/desktop/tests/helpers/bridge.ts @@ -321,6 +321,13 @@ type MockBridgeOptions = { linkPreviewMetadataDelayMs?: number; /** Simulates native cold-cache startup work before the async response. */ linkPreviewMetadataStartBlockMs?: number; + /** Delays link-preview snapshot media uploads so specs can drive an in-flight + * snapshot upload. See e2eBridge mock.linkPreviewUploadDelayMs. */ + linkPreviewUploadDelayMs?: number; + /** Substrings of `link-preview-*` upload filenames whose upload should reject, + * so specs can drive a per-media snapshot upload failure. See e2eBridge + * mock.linkPreviewUploadErrorFilenames. */ + linkPreviewUploadErrorFilenames?: string[]; searchProfiles?: MockSearchProfileSeed[]; updateAvailable?: boolean; updateChannelDelayMs?: number; From 0352c069df5b9d4cc73302cbf020444d8e6160fd Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Mon, 10 Aug 2026 10:39:45 -0700 Subject: [PATCH 4/7] fix(composer): render a preview card for fragment-bearing links A composer link with a URL fragment (e.g. a GitHub PR review anchor like pull/3767#pullrequestreview-...) rendered no preview card at all, and with multiple links only the last fragment-less link previewed. Two fragment-handling faults compounded here: - The composer candidate filter runs each preview href through isValidLinkPreviewSnapshotCanonicalUrl, which rejects any URL containing '#', so fragment links never entered the candidate set. - Metadata was fetched by the raw fragment-bearing href while the cache and render lookup keyed on the fragment-stripped canonical URL, an identity mismatch. Fix both at the identity source: createPreview now strips the fragment so every downstream consumer -- eligibility filter, dedupe, snapshot canonicalUrl, card key, and fetch input -- shares one fragment-less page identity. Fragment variants of the same page collapse to a single card, and the message body keeps the raw URL so click-through to the anchor still works. Buzz entity/git links are unaffected (built via separate canonical builders that preserve query strings and only clear the hash). Tests: new unit coverage that parseSupportedLinkPreview strips the fragment and extractSupportedLinkPreviews collapses fragment variants of one page while keeping a distinct second page; new e2e asserting two fragments of one page plus a second page render exactly two cards with both original prose links present and clickable; updated the trailing-fragment e2e to expect canonical (fragment-less) snapshot tags. Full unit suite 4537 passed; messaging e2e 54 passed. Co-authored-by: Taylor Ho Signed-off-by: Taylor Ho --- desktop/src/shared/lib/linkPreview.test.mjs | 31 +++ desktop/src/shared/lib/linkPreview.ts | 10 +- desktop/tests/e2e/messaging.spec.ts | 251 +++++++++++++------- 3 files changed, 203 insertions(+), 89 deletions(-) diff --git a/desktop/src/shared/lib/linkPreview.test.mjs b/desktop/src/shared/lib/linkPreview.test.mjs index 4bf3245dbff..b4807f82fd3 100644 --- a/desktop/src/shared/lib/linkPreview.test.mjs +++ b/desktop/src/shared/lib/linkPreview.test.mjs @@ -20,6 +20,37 @@ test("parseSupportedLinkPreview parses GitHub pull request URLs", () => { ); }); +test("parseSupportedLinkPreview strips the fragment from the preview href", () => { + // A `#fragment` is a client-only anchor; the preview and its signed snapshot + // canonical URL are of the page. Keeping it would fail the fragmentless + // snapshot-URL guard and drop the preview entirely. + assert.equal( + parseSupportedLinkPreview( + "https://github.com/block/sprout/pull/1234#pullrequestreview-99", + )?.href, + "https://github.com/block/sprout/pull/1234", + ); +}); + +test("extractSupportedLinkPreviews collapses fragment variants of one page", () => { + const previews = extractSupportedLinkPreviews( + [ + "https://github.com/block/sprout/pull/1234#pullrequestreview-99", + "https://github.com/block/sprout/pull/1234#issuecomment-1", + "https://github.com/block/sprout/pull/5678", + ].join("\n"), + ); + // Two anchors into the same page dedupe to one card at first occurrence; the + // distinct second page keeps its own card. + assert.deepEqual( + previews.map((preview) => preview.href), + [ + "https://github.com/block/sprout/pull/1234", + "https://github.com/block/sprout/pull/5678", + ], + ); +}); + test("parseSupportedLinkPreview parses GitHub repository URLs", () => { assert.deepEqual( parseSupportedLinkPreview("https://github.com/block/sprout"), diff --git a/desktop/src/shared/lib/linkPreview.ts b/desktop/src/shared/lib/linkPreview.ts index 58d8739ef14..4a7772580b1 100644 --- a/desktop/src/shared/lib/linkPreview.ts +++ b/desktop/src/shared/lib/linkPreview.ts @@ -271,9 +271,17 @@ function createPreview( typeLabel: SupportedLinkPreview["typeLabel"], title: string, ): SupportedLinkPreview { + // Strip the `#fragment` from the preview identity. A fragment is a + // client-only anchor into the page — the preview (and the signed snapshot's + // canonicalUrl) is of the page itself. Keeping it would fail the + // fragment-free snapshot-URL guard, so a link like `pull/3767#review-1` + // would silently get no preview at all. The message body keeps the raw URL, + // so click-through to the anchor is preserved. + const canonical = new URL(parsed.href); + canonical.hash = ""; return { kind, - href: parsed.href, + href: canonical.href, provider, title, typeLabel, diff --git a/desktop/tests/e2e/messaging.spec.ts b/desktop/tests/e2e/messaging.spec.ts index 885729f5318..a17b893fe76 100644 --- a/desktop/tests/e2e/messaging.spec.ts +++ b/desktop/tests/e2e/messaging.spec.ts @@ -141,102 +141,111 @@ test.beforeEach(async ({ page }, testInfo) => { imageDomain: "pbs.twimg.com", }, } - : testInfo.title.includes("mixed link preview image outcomes") + : testInfo.title.includes("fragment link previews") ? { + // Metadata is keyed by the canonical, fragment-less URL — the + // shape a real OpenGraph/HTML fetch resolves against. A resolver + // that fetches with the raw `#fragment` attached would miss these + // keys and drop the card, which is exactly the bug under test. linkPreviewMetadataByHref: { - "https://github.com/block/buzz/pull/4001": { - title: "Loaded preview image", + "https://github.com/block/buzz/pull/3767": { + title: "Buzz pull request 3767", siteName: "GitHub", - description: "The image request completed.", - imageDataUrl: - "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", - imageDomain: "opengraph.githubassets.com", - imageFetchState: "image", - imageRetryAfterMs: null, + description: "Fragment-bearing PR link.", + imageDataUrl: null, + imageDomain: null, }, - "https://github.com/block/buzz/pull/4002": { - title: "Rate-limited preview image", + "https://github.com/block/buzz/pull/3867": { + title: "Buzz pull request 3867", siteName: "GitHub", - description: "Metadata remains available during cooldown.", + description: "Plain PR link.", imageDataUrl: null, imageDomain: null, - imageFetchState: "transient_failure", - imageRetryAfterMs: 900_000, }, }, } - : testInfo.title.includes("link preview browser image error") + : testInfo.title.includes("mixed link preview image outcomes") ? { - linkPreviewMetadata: { - title: "Invalid decoded preview image", - siteName: "GitHub", - description: "The browser should replace this image.", - imageDataUrl: null, - imageDomain: null, - imageFetchState: "rejected", - imageRetryAfterMs: null, + linkPreviewMetadataByHref: { + "https://github.com/block/buzz/pull/4001": { + title: "Loaded preview image", + siteName: "GitHub", + description: "The image request completed.", + imageDataUrl: + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", + imageDomain: "opengraph.githubassets.com", + imageFetchState: "image", + imageRetryAfterMs: null, + }, + "https://github.com/block/buzz/pull/4002": { + title: "Rate-limited preview image", + siteName: "GitHub", + description: "Metadata remains available during cooldown.", + imageDataUrl: null, + imageDomain: null, + imageFetchState: "transient_failure", + imageRetryAfterMs: 900_000, + }, }, } - : testInfo.title.includes("link preview image geometry") + : testInfo.title.includes("link preview browser image error") ? { linkPreviewMetadata: { - title: - "Ship a wider horizontal preview with a two-line title that wraps cleanly", + title: "Invalid decoded preview image", siteName: "GitHub", - description: "A polished, stable preview for shared links.", - imageDataUrl: - "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", - imageDomain: "opengraph.githubassets.com", - faviconDataUrl: - "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", + description: "The browser should replace this image.", + imageDataUrl: null, + imageDomain: null, + imageFetchState: "rejected", + imageRetryAfterMs: null, }, - linkPreviewMetadataDelayMs: 800, } - : testInfo.title.includes("link preview no-image layout") || - testInfo.title.includes("composer no-image link embeds") + : testInfo.title.includes("link preview image geometry") ? { linkPreviewMetadata: { - title: "Buzz", + title: + "Ship a wider horizontal preview with a two-line title that wraps cleanly", siteName: "GitHub", description: - "Open-source collaboration for the Buzz app.", - imageDataUrl: null, - imageDomain: null, + "A polished, stable preview for shared links.", + imageDataUrl: + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", + imageDomain: "opengraph.githubassets.com", faviconDataUrl: "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", }, - linkPreviewMetadataDelayMs: 2_000, + linkPreviewMetadataDelayMs: 800, } - : testInfo.title.includes( - "rich link preview preserves description newlines", - ) + : testInfo.title.includes("link preview no-image layout") || + testInfo.title.includes("composer no-image link embeds") ? { linkPreviewMetadata: { - title: "Buzz pull request", + title: "Buzz", siteName: "GitHub", description: - "First paragraph line one.\nFirst paragraph line two.\n\nSecond paragraph.", + "Open-source collaboration for the Buzz app.", imageDataUrl: null, imageDomain: null, + faviconDataUrl: + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", }, + linkPreviewMetadataDelayMs: 2_000, } : testInfo.title.includes( - "Enter during an in-flight snapshot upload", + "rich link preview preserves description newlines", ) ? { linkPreviewMetadata: { title: "Buzz pull request", siteName: "GitHub", - description: "A sender-authored preview snapshot.", - imageDataUrl: - "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", - imageDomain: "opengraph.githubassets.com", + description: + "First paragraph line one.\nFirst paragraph line two.\n\nSecond paragraph.", + imageDataUrl: null, + imageDomain: null, }, - linkPreviewMetadataDelayMs: 300, - linkPreviewUploadDelayMs: 1_200, } : testInfo.title.includes( - "snapshot thumbnail upload failure", + "Enter during an in-flight snapshot upload", ) ? { linkPreviewMetadata: { @@ -246,44 +255,67 @@ test.beforeEach(async ({ page }, testInfo) => { imageDataUrl: "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", imageDomain: "opengraph.githubassets.com", - faviconDataUrl: - "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", }, - // Fail only the thumbnail upload; the favicon survives, - // so the snapshot degrades to a favicon-only preview. - linkPreviewUploadErrorFilenames: [ - "link-preview-image", - ], + linkPreviewMetadataDelayMs: 300, + linkPreviewUploadDelayMs: 1_200, } - : testInfo.title.includes("link preview") || - testInfo.title.includes("supported Compact") + : testInfo.title.includes( + "snapshot thumbnail upload failure", + ) ? { linkPreviewMetadata: { title: "Buzz pull request", siteName: "GitHub", description: "A sender-authored preview snapshot.", - imageDataUrl: null, - imageDomain: null, + imageDataUrl: + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", + imageDomain: "opengraph.githubassets.com", + faviconDataUrl: + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=", }, - linkPreviewMetadataDelayMs: testInfo.title.includes( - "loading card before cold resolver work", - ) - ? 10_000 - : testInfo.title.includes("send does not wait") - ? 3_000 - : testInfo.title.includes("style defaults") || - testInfo.title.includes("attachment-sized") - ? 1_500 - : undefined, - linkPreviewMetadataStartBlockMs: - testInfo.title.includes( - "loading card before cold resolver work", - ) - ? 150 - : undefined, + // Fail only the thumbnail upload; the favicon survives, + // so the snapshot degrades to a favicon-only preview. + linkPreviewUploadErrorFilenames: [ + "link-preview-image", + ], } - : undefined; + : testInfo.title.includes("link preview") || + testInfo.title.includes("supported Compact") + ? { + linkPreviewMetadata: { + title: "Buzz pull request", + siteName: "GitHub", + description: + "A sender-authored preview snapshot.", + imageDataUrl: null, + imageDomain: null, + }, + linkPreviewMetadataDelayMs: + testInfo.title.includes( + "loading card before cold resolver work", + ) + ? 10_000 + : testInfo.title.includes( + "send does not wait", + ) + ? 3_000 + : testInfo.title.includes( + "style defaults", + ) || + testInfo.title.includes( + "attachment-sized", + ) + ? 1_500 + : undefined, + linkPreviewMetadataStartBlockMs: + testInfo.title.includes( + "loading card before cold resolver work", + ) + ? 150 + : undefined, + } + : undefined; const mock = testInfo.title.includes("unresolvable preview") ? { linkPreviewMetadata: null, linkPreviewMetadataDelayMs: 800 } : baseMock; @@ -650,14 +682,22 @@ test("rich link preview preserves description newlines after sending", async ({ ); }); -test("completed link previews send when one URL has an unsnapshotable fragment", async ({ +test("completed link previews normalize a trailing-fragment URL and still send", async ({ page, }) => { + // The third URL carries a trailing `#` (empty fragment). It is normalized to + // its fragmentless canonical form for the preview and snapshot tag, so it now + // gets a card like the others; the message body keeps the original URL. const previewUrls = [ "https://twitter.com/tellaho", "https://github.com/block/buzz/pull/3246", "https://x.com/tellaho/status/1884289176381841506#", ]; + const canonicalUrls = [ + "https://twitter.com/tellaho", + "https://github.com/block/buzz/pull/3246", + "https://x.com/tellaho/status/1884289176381841506", + ]; const pastedText = previewUrls.join("\n"); await page.goto("/"); await page.getByTestId("channel-general").click(); @@ -677,11 +717,8 @@ test("completed link previews send when one URL has an unsnapshotable fragment", const composerPreviewCards = page.locator( "[data-link-preview-composer-card]", ); - await expect(composerPreviewCards).toHaveCount(2); - await expect( - composerPreviewCards.locator(`a[href="${previewUrls[2]}"]`), - ).toHaveCount(0); - await waitForReadyComposerSnapshots(page, 2); + await expect(composerPreviewCards).toHaveCount(3); + await waitForReadyComposerSnapshots(page, 3); const send = page.getByTestId("send-message"); await expect(send).toBeEnabled(); @@ -701,7 +738,7 @@ test("completed link previews send when one URL has an unsnapshotable fragment", ( calls[0]?.payload as { linkPreviewTags?: string[][] | null } | undefined )?.linkPreviewTags?.map((tag) => tag[3]), - ).toEqual(previewUrls.slice(0, 2)); + ).toEqual(canonicalUrls); }); test("unresolvable preview disappears after the terminal miss", async ({ @@ -1151,6 +1188,44 @@ test("mixed link preview image outcomes keep Compact and Rich fallbacks stable", ).toHaveCount(0); }); +test("fragment link previews render a card per canonical URL", async ({ + page, +}) => { + // Two links into the SAME page differing only by `#fragment`, plus a link + // to a second page. The fragment variants collapse to one card (the preview + // is of the page, not the anchor); the second page adds a second card — two + // cards total. A resolver that keys previews on the raw fragment-bearing URL + // drops the fragment cards entirely (the reported bug). + const fragmentUrlA = + "https://github.com/block/buzz/pull/3767#pullrequestreview-4857569498"; + const fragmentUrlB = "https://github.com/block/buzz/pull/3767#issuecomment-1"; + const plainUrl = "https://github.com/block/buzz/pull/3867"; + await page.goto("/"); + await page.getByTestId("channel-general").click(); + await page + .getByTestId("message-input") + .fill(`${fragmentUrlA}\n${fragmentUrlB}\n${plainUrl}`); + + const composerCards = page + .locator("[data-composer-link-previews]") + .locator('[data-link-preview="github-pull-request"]'); + await expect(composerCards).toHaveCount(2); + + await waitForReadyComposerSnapshots(page, 2); + await page.getByTestId("send-message").click(); + + const row = page.getByTestId("message-row").last(); + // Two preview cards: the fragment variants collapsed to the 3767 page, plus + // the 3867 page. + await expect( + row.locator('[data-link-preview="github-pull-request"]'), + ).toHaveCount(2); + // Both original fragment-bearing prose links survive intact and clickable — + // the fragment is a navigation anchor, only the preview is normalized. + await expect(row.locator(`a[href="${fragmentUrlA}"]`)).toBeVisible(); + await expect(row.locator(`a[href="${fragmentUrlB}"]`)).toBeVisible(); +}); + test("link preview browser image errors render a fallback", async ({ page, }) => { From 6ec255a6050fa40a85a380e3d82eda250b5aae46 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Mon, 10 Aug 2026 16:49:06 -0700 Subject: [PATCH 5/7] test(composer): add regression coverage for link-preview auto-send and tag-leak blockers Two reviewer-flagged blockers on the settling-window fix lacked regression tests; both fixes are now backed by deterministic coverage and the fix logic is extracted into pure, testable helpers. Blocker A (auto-send drop): a Drafts-panel Send for a draft with a supported link is normally still inside the ~350 ms settling window when the mount-only auto-submit effect fires. The prior effect cleared ?autoSend then fired submit once at setTimeout(0); submit bailed at the pending-snapshot guard and the one-shot never retried, silently dropping the draft. Extracted scheduleSettleGatedAutoSubmit (poll while pending, then submit exactly once) as a timer-injectable helper. New unit test drives it with a fake timer (waits while pending, sends once on clear, cleanup cancels); new e2e drives the real Drafts-panel Send confirm flow (in-app nav, not page.goto to /channels/... which 404s under the static test server) and asserts exactly one send carrying the resolved snapshot tag. Blocker B (tag leak): resolving link A, deleting it, and sending replacement text within the debounce left A in the debounced active set, so submit rode A's snapshot tag (with media refs) onto a body no longer containing A. Extracted the pure selectSubmitTags selector keyed off the LIVE href set (never the debounced one) and rewired getReadyTags to it. New unit test proves a removed URL's tag is never emitted, a replacement link emits only its own tag, live order is preserved, and suppression emits the none marker. The e2e form of this was flaky (send inside the 350 ms window did not reliably fire headless), so a deterministic unit test replaces it. Verified: leak-guard and drop-guard cases fail on pre-fix behavior. Full unit suite 4595 passed; Blocker A e2e passed; typecheck, biome, and file-size checks clean. Co-authored-by: Taylor Ho Signed-off-by: Taylor Ho --- .../features/messages/ui/MessageComposer.tsx | 14 +-- .../ui/messageComposerAutoSubmit.test.mjs | 103 +++++++++++++++++ .../messages/ui/messageComposerAutoSubmit.ts | 45 ++++++++ .../messages/ui/selectSubmitTags.test.mjs | 78 +++++++++++++ .../messages/ui/useComposerLinkPreviews.tsx | 47 +++++--- desktop/tests/e2e/messaging.spec.ts | 108 ++++++++++++++++-- 6 files changed, 365 insertions(+), 30 deletions(-) create mode 100644 desktop/src/features/messages/ui/messageComposerAutoSubmit.test.mjs create mode 100644 desktop/src/features/messages/ui/messageComposerAutoSubmit.ts create mode 100644 desktop/src/features/messages/ui/selectSubmitTags.test.mjs diff --git a/desktop/src/features/messages/ui/MessageComposer.tsx b/desktop/src/features/messages/ui/MessageComposer.tsx index bb0286839c5..aec94c6f37a 100644 --- a/desktop/src/features/messages/ui/MessageComposer.tsx +++ b/desktop/src/features/messages/ui/MessageComposer.tsx @@ -58,6 +58,7 @@ import { useComposerContentState } from "./useComposerContentState"; import { useDraftPersistLifecycle } from "./useDraftPersistSnapshot"; import { submitMessageEdit } from "./submitMessageEdit"; import { useComposerLinkPreviews } from "./useComposerLinkPreviews"; +import { scheduleSettleGatedAutoSubmit } from "./messageComposerAutoSubmit"; import type { MessageComposerProps } from "./MessageComposer.types"; function MessageComposerImpl({ audienceContext = null, @@ -663,15 +664,10 @@ function MessageComposerImpl({ // Clear the trigger BEFORE firing so any navigation from the send cannot // loop back with the param still present. onAutoSubmitCompleteRef.current?.(); - // Defer by one macrotask so the draft-persist lifecycle effect (which runs - // synchronously after mount) has a chance to load the draft content into - // the Tiptap editor before we try to submit. - const timer = window.setTimeout(() => { - submitMessageRef.current(); - }, 0); - return () => { - window.clearTimeout(timer); - }; + return scheduleSettleGatedAutoSubmit({ + isPending: () => hasPendingLinkPreviewSnapshotsRef.current, + submit: () => submitMessageRef.current(), + }); // eslint-disable-next-line react-hooks/exhaustive-deps }, []); // mount-only const handleSubmit = React.useCallback( diff --git a/desktop/src/features/messages/ui/messageComposerAutoSubmit.test.mjs b/desktop/src/features/messages/ui/messageComposerAutoSubmit.test.mjs new file mode 100644 index 00000000000..e9a62e42dea --- /dev/null +++ b/desktop/src/features/messages/ui/messageComposerAutoSubmit.test.mjs @@ -0,0 +1,103 @@ +/** + * Unit tests for `scheduleSettleGatedAutoSubmit` — the auto-submit scheduler + * that fires a ?autoSend draft submit exactly once, after link-preview settling + * finishes. + * + * Imports and exercises the ACTUAL source helper. Regression guard for the + * auto-send-drop blocker (PR #5245, Blocker A): a confirmed draft with a + * supported link is normally still settling at mount, so an immediate submit + * bails on the pending guard. The prior one-shot `setTimeout(0)` consumed the + * trigger and silently dropped the draft. The scheduler must instead poll while + * pending and submit exactly once when settling clears — never zero, never + * twice. + * + * A controllable fake timer drives the poll deterministically, so there is no + * real-time flakiness (the E2E form could not reliably send inside the ~350 ms + * window headless). + */ + +import assert from "node:assert/strict"; +import test from "node:test"; +import { scheduleSettleGatedAutoSubmit } from "./messageComposerAutoSubmit.ts"; + +// Minimal deterministic timer: records scheduled callbacks so the test can +// advance them one "tick" at a time and assert exact call counts. +function makeFakeTimers() { + const pending = new Map(); + let nextId = 1; + return { + set(fn, _ms) { + const id = nextId++; + pending.set(id, fn); + return id; + }, + clear(id) { + pending.delete(id); + }, + // Fire the earliest-scheduled still-pending callback. + tick() { + const [id, fn] = pending.entries().next().value ?? []; + if (id === undefined) return false; + pending.delete(id); + fn(); + return true; + }, + pendingCount() { + return pending.size; + }, + }; +} + +test("submits once immediately when nothing is pending", () => { + const timers = makeFakeTimers(); + let submits = 0; + scheduleSettleGatedAutoSubmit({ + isPending: () => false, + submit: () => submits++, + timers, + }); + timers.tick(); // fire the initial setTimeout(0) + assert.equal(submits, 1); + assert.equal(timers.pendingCount(), 0, "no retry should be scheduled"); +}); + +test("waits while settling then submits exactly once (the drop-guard)", () => { + const timers = makeFakeTimers(); + let submits = 0; + let pending = true; // still settling at mount + scheduleSettleGatedAutoSubmit({ + isPending: () => pending, + submit: () => submits++, + timers, + }); + timers.tick(); // initial attempt: pending → reschedules, does NOT submit + assert.equal(submits, 0, "must not send while a snapshot is still pending"); + assert.equal(timers.pendingCount(), 1, "a retry must be scheduled"); + + timers.tick(); // retry: still pending + assert.equal(submits, 0); + + pending = false; // settling finished + timers.tick(); // retry: fires the send + assert.equal(submits, 1, "must send exactly once after settling clears"); + assert.equal(timers.pendingCount(), 0); +}); + +test("cleanup before settling finishes cancels the submit (no orphan send)", () => { + const timers = makeFakeTimers(); + let submits = 0; + const cleanup = scheduleSettleGatedAutoSubmit({ + isPending: () => true, + submit: () => submits++, + timers, + }); + timers.tick(); // initial attempt reschedules a retry + assert.equal(timers.pendingCount(), 1); + cleanup(); // unmount + assert.equal( + timers.pendingCount(), + 0, + "cleanup must clear the pending retry", + ); + assert.equal(submits, 0); +}); diff --git a/desktop/src/features/messages/ui/messageComposerAutoSubmit.ts b/desktop/src/features/messages/ui/messageComposerAutoSubmit.ts new file mode 100644 index 00000000000..f15422b93d1 --- /dev/null +++ b/desktop/src/features/messages/ui/messageComposerAutoSubmit.ts @@ -0,0 +1,45 @@ +// Auto-submit scheduler for a confirmed draft that arrived via ?autoSend. A +// draft containing a supported link is normally still settling (350 ms +// debounce + metadata/upload) at mount, so a submit fired immediately bails on +// the pending-snapshot guard. A one-shot `setTimeout(0)` would consume the +// trigger and silently drop the draft; instead poll until settling finishes +// (bounded by the preview hook's own anti-trap cap) then submit exactly once. +// The `didSubmit` guard prevents a double fire, and the initial defer lets the +// draft-persist lifecycle effect load the draft into the editor first. +// +// Extracted from MessageComposer as a pure, timer-injectable helper so the +// retry/one-shot contract is unit-testable without mounting the composer. +export function scheduleSettleGatedAutoSubmit({ + isPending, + submit, + retryDelayMs = 50, + timers = { + set: (fn: () => void, ms: number) => window.setTimeout(fn, ms), + clear: (id: number) => window.clearTimeout(id), + }, +}: { + isPending: () => boolean; + submit: () => void; + retryDelayMs?: number; + timers?: { + set: (fn: () => void, ms: number) => number; + clear: (id: number) => void; + }; +}): () => void { + let didSubmit = false; + let retryTimer = 0; + const attempt = () => { + if (didSubmit) return; + if (isPending()) { + retryTimer = timers.set(attempt, retryDelayMs); + return; + } + didSubmit = true; + submit(); + }; + const initialTimer = timers.set(attempt, 0); + return () => { + timers.clear(initialTimer); + timers.clear(retryTimer); + }; +} diff --git a/desktop/src/features/messages/ui/selectSubmitTags.test.mjs b/desktop/src/features/messages/ui/selectSubmitTags.test.mjs new file mode 100644 index 00000000000..f61f4cbadd9 --- /dev/null +++ b/desktop/src/features/messages/ui/selectSubmitTags.test.mjs @@ -0,0 +1,78 @@ +/** + * Unit tests for `selectSubmitTags` — the pure selector that decides which + * link-preview snapshot tags a composer submit emits. + * + * These import and exercise the ACTUAL source helper (not a mirrored copy), so + * they fail if the submit-tag selection ever regresses. + * + * Regression guard for the "removed-URL tag leak" defect (PR #5245, Blocker B): + * a ready snapshot tag for URL A lingers in the tag map for the 350 ms + * debounce window after A is deleted from the draft. Submit must key off the + * LIVE hrefs in the content being sent — never that debounced set — so deleting + * A and immediately sending replacement text can never attach A's tag (and its + * media refs) to a body that no longer contains A. + * + * The E2E form of this test was flaky: sending inside the 350 ms window from a + * headless browser did not reliably fire a submit, so it could not isolate the + * leak. A pure unit test against the extracted selector is deterministic and + * targets the fix logic directly. + */ + +import assert from "node:assert/strict"; +import test from "node:test"; +import { selectSubmitTags } from "./useComposerLinkPreviews.tsx"; + +const tagA = ["link-preview", "snapshot", "1", "https://a.example/x", "A"]; +const tagB = ["link-preview", "snapshot", "1", "https://b.example/y", "B"]; + +test("emits the tag for a live href that has a ready snapshot", () => { + const tags = selectSubmitTags( + ["https://a.example/x"], + { "https://a.example/x": tagA }, + false, + ); + assert.deepEqual(tags, [tagA]); +}); + +test("LEAK GUARD: a ready tag whose href is no longer live is NOT emitted", () => { + // A resolved (tag still cached), but A was deleted from the draft and the + // live content is now different — the debounced map still holds A's tag. + const tags = selectSubmitTags( + [], // live content no longer contains A + { "https://a.example/x": tagA }, + false, + ); + assert.deepEqual(tags, [], "removed URL A must never leak its snapshot tag"); +}); + +test("LEAK GUARD: replacing A with a live B emits only B's tag, never A's", () => { + const tags = selectSubmitTags( + ["https://b.example/y"], // A deleted, B is what's live now + { "https://a.example/x": tagA, "https://b.example/y": tagB }, + false, + ); + assert.deepEqual(tags, [tagB]); +}); + +test("a live href with no ready tag is omitted (sends as a bare link)", () => { + const tags = selectSubmitTags(["https://a.example/x"], {}, false); + assert.deepEqual(tags, []); +}); + +test("preserves live href order for multiple ready tags", () => { + const tags = selectSubmitTags( + ["https://a.example/x", "https://b.example/y"], + { "https://b.example/y": tagB, "https://a.example/x": tagA }, + false, + ); + assert.deepEqual(tags, [tagA, tagB]); +}); + +test("suppressed emits only the 'none' marker, ignoring any ready tags", () => { + const tags = selectSubmitTags( + ["https://a.example/x"], + { "https://a.example/x": tagA }, + true, + ); + assert.deepEqual(tags, [["link-preview", "none"]]); +}); diff --git a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx index 9f77a250737..9ff0eb790a5 100644 --- a/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx +++ b/desktop/src/features/messages/ui/useComposerLinkPreviews.tsx @@ -44,6 +44,26 @@ function previewHostname(href: string): string { } } +// Pure selector for the snapshot tags emitted on submit. Keyed off `liveHrefs` +// (the hrefs in the content being sent RIGHT NOW), never the debounced active +// set — a ready tag for URL A lingers in `tagsByHref` for the 350 ms until the +// debounce drops A, so keying off live hrefs is what stops "delete A, send +// replacement text within the window" from leaking A's tag (and media refs) +// onto a body that no longer contains A. When `suppressed`, emit only the +// "none" marker. Live hrefs without a ready tag (dead/slow link past the +// anti-trap cap) are omitted and the message sends as a bare link. +export function selectSubmitTags( + liveHrefs: readonly string[], + tagsByHref: Record, + suppressed: boolean, +): string[][] { + if (suppressed) return [["link-preview", "none"]]; + return liveHrefs.flatMap((href) => { + const tag = tagsByHref[href]; + return tag ? [tag] : []; + }); +} + function ComposerLinkPreviewCard({ preview, tagReady, @@ -398,20 +418,21 @@ export function useComposerLinkPreviews(content: string, enabled = true) {
) : null; - // Snapshot tags for a submit, read synchronously at submit start from the href - // set currently backing the previews (activeHrefsRef) — so the tags always - // correspond to the content being sent. No await: Send is disabled until every + // 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. A suppressed preview list emits - // the "none" marker; hrefs without a ready tag (dead/slow link past the cap) - // are omitted and the message sends as a bare link. - const getReadyTags = React.useCallback(() => { - if (suppressedRef.current) return [["link-preview", "none"]]; - return [...activeHrefsRef.current].flatMap((href) => { - const tag = readyTagsByHrefRef.current[href]; - return tag ? [tag] : []; - }); - }, []); + // the tags that will ever exist already exist. + const getReadyTags = React.useCallback( + () => + selectSubmitTags( + liveCandidatesRef.current, + readyTagsByHrefRef.current, + suppressedRef.current, + ), + [], + ); return { previewList, getReadyTags, diff --git a/desktop/tests/e2e/messaging.spec.ts b/desktop/tests/e2e/messaging.spec.ts index a17b893fe76..2d7f3eacda9 100644 --- a/desktop/tests/e2e/messaging.spec.ts +++ b/desktop/tests/e2e/messaging.spec.ts @@ -300,14 +300,16 @@ test.beforeEach(async ({ page }, testInfo) => { "send does not wait", ) ? 3_000 - : testInfo.title.includes( - "style defaults", - ) || - testInfo.title.includes( - "attachment-sized", - ) - ? 1_500 - : undefined, + : testInfo.title.includes("draft auto-send") + ? 500 + : testInfo.title.includes( + "style defaults", + ) || + testInfo.title.includes( + "attachment-sized", + ) + ? 1_500 + : undefined, linkPreviewMetadataStartBlockMs: testInfo.title.includes( "loading card before cold resolver work", @@ -867,6 +869,96 @@ test("Enter during an in-flight snapshot upload cannot ship a bare link", async expect(linkPreviewTags?.map((tag) => tag[3])).toEqual([previewUrl]); }); +test("draft auto-send with a link preview waits for settling and sends exactly once", async ({ + page, +}) => { + // Regression for the one-shot auto-submit blocker: a confirmed Drafts-panel + // "Send message" for a draft containing a supported link is normally still + // inside the preview settling window when the mount-only auto-submit effect + // fires. The old effect cleared the ?autoSend trigger then fired submit once + // at setTimeout(0); submit bailed at the pending-snapshot guard and the + // one-shot never retried, so the confirmed draft was silently never sent. + // The effect must instead wait until settling finishes, then send exactly + // once — with the resolved snapshot tag attached. + const previewUrl = "https://github.com/block/buzz/pull/3246?draft=autosend"; + const channelId = "9a1657ac-f7aa-5db0-b632-d8bbeb6dfb50"; + + // Seed a channel draft under the legacy store key (migrated on startup). The + // main composer keys its draft off the bare channel id, and the Drafts panel + // navigates with ?autoSend=, so seeding under the bare id mirrors + // the real "Send message" target exactly. + await page.addInitScript( + ({ storeKey, draftKey, content, channel }) => { + const timestamp = new Date().toISOString(); + window.localStorage.setItem( + storeKey, + JSON.stringify({ + [draftKey]: { + channelId: channel, + content, + createdAt: timestamp, + pendingImeta: [], + selectionEnd: content.length, + selectionStart: content.length, + spoileredAttachmentUrls: [], + status: "active", + updatedAt: timestamp, + }, + }), + ); + }, + { + storeKey: `buzz-drafts.v1:${"deadbeef".repeat(8)}`, + draftKey: channelId, + content: previewUrl, + channel: channelId, + }, + ); + + // Drive the real Drafts-panel "Send message" confirm flow. This does an + // in-app client navigation to the channel with ?autoSend=, arming + // the main composer's auto-submit effect — the exact production path. + await page.goto("/", { waitUntil: "domcontentloaded" }); + await expect(page.getByTestId("home-inbox")).toBeVisible({ timeout: 10_000 }); + await page.getByTestId("inbox-filter-trigger").click(); + await page.getByRole("menuitemradio", { name: "Drafts" }).click(); + await page.keyboard.press("Escape"); + + const draftRow = page.locator(`[data-testid='home-draft-item-${channelId}']`); + await expect(draftRow).toBeVisible({ timeout: 8_000 }); + await draftRow.hover(); + await draftRow + .getByRole("button", { name: "Send message", exact: true }) + .click(); + const dialog = page.getByRole("alertdialog"); + await expect(dialog).toBeVisible({ timeout: 4_000 }); + await dialog.getByRole("button", { name: "Send", exact: true }).click(); + + // Exactly one send eventually fires (after the ~500 ms metadata settle), and + // it carries the link preview snapshot tag — proving the draft was not + // dropped during the settling window and did not double-send on retry. + await expect + .poll(async () => + page.evaluate( + () => + (window.__BUZZ_E2E_COMMAND_PAYLOADS__ ?? []).filter( + (entry) => entry.command === "send_channel_message", + ).length, + ), + ) + .toBe(1); + + const linkPreviewTags = await page.evaluate(() => { + const call = [...(window.__BUZZ_E2E_COMMAND_PAYLOADS__ ?? [])] + .reverse() + .find((entry) => entry.command === "send_channel_message"); + return ( + call?.payload as { linkPreviewTags?: string[][] | null } | undefined + )?.linkPreviewTags; + }); + expect(linkPreviewTags?.map((tag) => tag[3])).toEqual([previewUrl]); +}); + test("rapid Enter presses on a ready link preview send exactly once", async ({ page, }) => { From 4d55d18c9f7f228aba3b5dbe69cfc809486fbd85 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Mon, 10 Aug 2026 19:29:01 -0700 Subject: [PATCH 6/7] chore(composer): refresh review head Co-authored-by: Taylor Ho Signed-off-by: Taylor Ho From 68ac80faf9c9f9bb80ac92c3923f8eb9ef1c2d08 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Mon, 10 Aug 2026 19:34:33 -0700 Subject: [PATCH 7/7] docs(composer): correct upload-delay test description Co-authored-by: Taylor Ho Signed-off-by: Taylor Ho --- desktop/src/testing/e2eBridge.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index c466985e47d..751b06f484f 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -366,7 +366,7 @@ type E2eConfig = { /** Simulates native cold-cache startup work before the async response. */ linkPreviewMetadataStartBlockMs?: number; /** Delays link-preview snapshot media uploads so specs can exercise the - * Send-time wait for in-flight snapshot uploads. */ + * composer's settle-gated disabled state before the snapshot tag is ready. */ linkPreviewUploadDelayMs?: number; /** Substrings of `link-preview-*` upload filenames whose `upload_media_bytes` * call should reject, so specs can drive a per-media snapshot upload failure