From c4e55a4e2121c38eed03a031145f8ca9ea3e14bf Mon Sep 17 00:00:00 2001 From: Logan Johnson Date: Mon, 31 Aug 2026 15:32:50 -0400 Subject: [PATCH] fix(desktop): retain outgoing draft deletion authority after navigation Bind authored revisions to captured lifecycle visits instead of the visible editor. Apply source authority to cancellation and preflight recovery and sent-draft cleanup, while keeping editor ownership separate. Cover reviewer deletion reproduction, queued files, supersession, return visits, and actual thread-switch persistence. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson (cherry picked from commit 13e1245bbd042fb6d1afa6cf54152e839d9fe400) Signed-off-by: Logan Johnson (cherry picked from commit e5e77d88c3eb87360aac8771d17389abee831d3f) Signed-off-by: Logan Johnson --- .../messages/ui/useDraftPersistSnapshot.ts | 22 ++- .../useMentionSendFlow.cancellation.test.mjs | 178 ++++++++++++++++++ .../messages/ui/useMentionSendFlow.helpers.ts | 2 + .../messages/ui/useMentionSendFlow.ts | 46 ++--- .../messages/ui/useMentionSendFlow.types.ts | 1 + .../tests/e2e/remote-owned-mentions.spec.ts | 101 ++++++++++ docs/remote-mention-routing.md | 9 +- 7 files changed, 328 insertions(+), 31 deletions(-) diff --git a/desktop/src/features/messages/ui/useDraftPersistSnapshot.ts b/desktop/src/features/messages/ui/useDraftPersistSnapshot.ts index 18b7327bdb..1f04d09c55 100644 --- a/desktop/src/features/messages/ui/useDraftPersistSnapshot.ts +++ b/desktop/src/features/messages/ui/useDraftPersistSnapshot.ts @@ -66,7 +66,7 @@ type UseDraftPersistLifecycleParams = { }; type UseDraftPersistLifecycleResult = { - /** Monotonic editor revision, including same-text edit/clear cycles. */ + /** Captured visit revision; remains readable after exit, excluding later visits. */ getComposerRevision: () => number; /** Optimistic send/recovery is not an authored edit or authoritative deletion. */ runComposerUpdate: (update: () => void) => void; @@ -139,11 +139,13 @@ export function useDraftPersistLifecycle({ [getImplicitAgentMentionPrefix], ); - const composerRevision = React.useRef(0); - const getComposerRevision = React.useCallback( - () => composerRevision.current, - [], + // Pending sends retain this visit's record after navigation/unmount. A live + // editor ref would lose A's deletion authority as soon as B became visible. + const visit = React.useMemo( + () => ({ channelId, draftKey: effectiveDraftKey, revision: 0 }), + [channelId, effectiveDraftKey], ); + const getComposerRevision = React.useCallback(() => visit.revision, [visit]); const pendingImetaForPersistRef = React.useRef([]); const emptyContentIsAuthoritativeRef = React.useRef(false); const isRestoringContentRef = React.useRef(false); @@ -240,7 +242,7 @@ export function useDraftPersistLifecycle({ const trackAuthoredContent = React.useCallback( (content: string) => { if (isRestoringContentRef.current) return; - composerRevision.current += 1; + visit.revision += 1; if (!effectiveDraftKey) return; const authoritativeDraftKey = scopedDraftKey(effectiveDraftKey); if (content.length > 0) { @@ -259,7 +261,13 @@ export function useDraftPersistLifecycle({ [], ); }, - [channelId, effectiveDraftKey, persistDraft, spoileredAttachmentUrlsRef], + [ + channelId, + effectiveDraftKey, + persistDraft, + spoileredAttachmentUrlsRef, + visit, + ], ); return { trackAuthoredContent, getComposerRevision, runComposerUpdate }; diff --git a/desktop/src/features/messages/ui/useMentionSendFlow.cancellation.test.mjs b/desktop/src/features/messages/ui/useMentionSendFlow.cancellation.test.mjs index 17b578cd05..38f8c5c074 100644 --- a/desktop/src/features/messages/ui/useMentionSendFlow.cancellation.test.mjs +++ b/desktop/src/features/messages/ui/useMentionSendFlow.cancellation.test.mjs @@ -633,3 +633,181 @@ for (const action of ["unchanged", "navigate", "edit"]) { } }); } + +// Independent reviewer reproduction, plus storage/queued-file boundaries. +for (const media of [false, true]) { + for (const action of ["dismiss", "navigate", "unmount"]) { + test(`authored edit-clear before ${action} stays deleted (queued media: ${media})`, async () => { + const s = await setup({ lifecycle: true }); + const gate = deferred(); + s.control.publish = gate; + if (media) { + s.dismiss(); + s.control.attachments = [{ id: "file", file: {}, spoilered: false }]; + await s.prompt(); + } + await s.invite(); + let upload; + if (media) { + await s.act(async () => { + upload = s.control.uploadCallbacks.onComplete( + [], + new AbortController().signal, + ); + }); + } + assert.equal(s.events("publish").length, 1); + s.edit("new authored text"); + s.edit("", []); + assert.equal(s.store.has("thread:a"), false); + const recoveryWrites = s.events("save-queue").length; + if (action === "navigate") s.navigate("thread:b"); + else if (action === "unmount") s.unmount(); + else s.dismiss(); + assert.equal(s.store.has("thread:a"), false); + if (action === "navigate") { + s.edit(TEXT, [{ ...s.refs[0], pubkey: "c".repeat(64) }]); + s.navigate("thread:a"); + assert.equal(s.options.contentRef.current, ""); + assert.equal(s.control.currentRefs.length, 0); + s.edit("new A after return", []); + s.navigate("thread:b"); + } + await s.finish(gate); + if (upload) await upload; + assert.equal(s.events("SEND").length, 0); + assert.equal(s.events("save-queue").length, recoveryWrites); + if (action === "navigate") { + assert.equal(s.options.contentRef.current, TEXT); + assert.equal(s.control.currentRefs[0].pubkey, "c".repeat(64)); + assert.equal(s.store.get("thread:a").content, "new A after return"); + assert.deepEqual(s.store.get("thread:a").mentionRefs, []); + } else assert.equal(s.store.has("thread:a"), false); + }); + } +} +for (const incoming of [TEXT, "different B text"]) { + for (const edit of ["untouched", "same-text-new-refs", "new-text"]) { + test(`source authority survives navigation: ${edit}, ${incoming}`, async () => { + const s = await setup({ lifecycle: true }); + const otherRefs = [{ ...s.refs[0], pubkey: "c".repeat(64) }]; + s.store.set("thread:b", { + content: incoming, + channelId: "general", + pendingImeta: [], + spoileredAttachmentUrls: [], + mentionRefs: otherRefs, + }); + const gate = deferred(); + s.control.publish = gate; + await s.invite(); + if (edit !== "untouched") + s.edit(edit === "new-text" ? "new A text" : TEXT, otherRefs); + s.navigate("thread:b"); + const saved = s.store.get("thread:a"); + assert.equal(saved.content, edit === "new-text" ? "new A text" : TEXT); + assert.deepEqual( + saved.mentionRefs, + edit === "untouched" ? s.refs : otherRefs, + ); + s.edit("B edit during old await", []); + await s.finish(gate); + assert.equal(s.store.get("thread:a"), saved); + assert.equal(s.options.contentRef.current, "B edit during old await"); + assert.equal(s.control.currentRefs.length, 0); + assert.equal(s.events("SEND").length, 0); + }); + } +} + +for (const action of ["untouched", "deleted", "superseded"]) { + test(`ordinary send preflight recovery respects source authority on unmount: ${action}`, async () => { + const s = await setup({ lifecycle: true }); + s.dismiss(); + s.options.mentions.memberPubkeys = new Set([KEY]); + const gate = deferred(); + s.control.prepare = gate; + s.rerender(); + let send; + await s.act(async () => { + send = s.result.current.sendMessageWithMentionFlow({ + capturedChannelId: "general", + pendingImeta: [], + trimmed: TEXT, + recoveryDraftKey: "thread:a", + }); + }); + assert.equal(s.options.contentRef.current, ""); + if (action === "deleted") { + s.edit("new text"); + s.edit("", []); + } + s.unmount(); + const other = { + content: "new saved draft", + channelId: "general", + pendingImeta: [], + spoileredAttachmentUrls: [], + mentionRefs: [], + }; + if (action === "superseded") s.store.set("thread:a", other); + await s.finish(gate); + await send; + assert.equal(s.events("SEND").length, 0); + if (action === "deleted") assert.equal(s.store.has("thread:a"), false); + else if (action === "superseded") + assert.equal(s.store.get("thread:a"), other); + else { + assert.equal(s.store.get("thread:a").content, TEXT); + assert.deepEqual(s.store.get("thread:a").mentionRefs, s.refs); + } + }); +} +for (const authored of [false, true]) { + test(`sent-draft cleanup consults exited source visit, not B revision (authored: ${authored})`, async () => { + const s = await setup({ lifecycle: true }); + s.dismiss(); + s.options.mentions.memberPubkeys = new Set([KEY]); + s.options.drafts.markDraftSent = (...args) => + s.calls.push(["mark-sent", ...args]); + const gate = deferred(); + s.control.publish = gate; + s.rerender(); + let send; + await s.act(async () => { + send = s.result.current.sendMessageWithMentionFlow({ + capturedChannelId: "general", + pendingImeta: [], + trimmed: TEXT, + recoveryDraftKey: "thread:a", + sentDraftKey: "thread:a", + }); + }); + if (authored) s.edit(TEXT); // same snapshot, but a genuinely newer draft + const sourceRevision = s.options.getComposerRevision; + const revision = sourceRevision(); + s.navigate("thread:b"); + s.edit("B edit", []); + assert.equal( + sourceRevision(), + revision, + "B must not change the captured A revision", + ); + // An untouched optimistic empty has no stored refs. Seed the unchanged + // submitted snapshot to exercise markDraftSent's bounded exact-ref guard. + if (!authored) + s.store.set("thread:a", { + content: TEXT, + channelId: "general", + pendingImeta: [], + spoileredAttachmentUrls: [], + mentionRefs: s.refs, + }); + await s.finish(gate); + await send; + assert.equal(s.events("SEND").length, 1); // ordinary destination-bound send + assert.equal(s.events("mark-sent").length, authored ? 0 : 1); + assert.equal(s.options.contentRef.current, "B edit"); + assert.equal(s.control.currentRefs.length, 0); + }); +} diff --git a/desktop/src/features/messages/ui/useMentionSendFlow.helpers.ts b/desktop/src/features/messages/ui/useMentionSendFlow.helpers.ts index 2c15cd1b83..da43b0f385 100644 --- a/desktop/src/features/messages/ui/useMentionSendFlow.helpers.ts +++ b/desktop/src/features/messages/ui/useMentionSendFlow.helpers.ts @@ -16,6 +16,8 @@ export { MENTION_REFERENCE_TAG }; export type ComposerDraftOwner = { channelId: string | null; draftKey: string | null | undefined; + /** Read this source visit, never the currently visible editor visit. */ + getComposerRevision: () => number; }; export type PendingNonMemberMentionSend = { diff --git a/desktop/src/features/messages/ui/useMentionSendFlow.ts b/desktop/src/features/messages/ui/useMentionSendFlow.ts index a83d0ce14f..c5ce1aac17 100644 --- a/desktop/src/features/messages/ui/useMentionSendFlow.ts +++ b/desktop/src/features/messages/ui/useMentionSendFlow.ts @@ -81,8 +81,8 @@ export function useMentionSendFlow({ // Persistence identity is independent of the host component and destination // channel. A -> B -> A must not revive A's previous invitation or recovery. const sourceOwner = React.useMemo( - () => ({ channelId, draftKey: effectiveDraftKey }), - [channelId, effectiveDraftKey], + () => ({ channelId, draftKey: effectiveDraftKey, getComposerRevision }), + [channelId, effectiveDraftKey, getComposerRevision], ); const sourceOwnerRef = React.useRef(sourceOwner); sourceOwnerRef.current = sourceOwner; @@ -357,25 +357,15 @@ export function useMentionSendFlow({ draft.queuedAttachments.length > 0 ? prepareBackgroundMediaUpload(draft.queuedAttachments) : null; - const persistPreflightDraft = () => { - if (isSendCancelled() || !draft.recoveryDraftKey) return; - drafts.persistDraft( - draft.recoveryDraftKey, - draft.savedContent, - draft.capturedChannelId ?? draft.recoveryDraftKey, - draft.savedImeta, - [...draft.savedSpoileredAttachmentUrls], - draft.savedMentionRefs, - ); - saveQueuedAttachmentsForDraft( - draft.recoveryDraftKey, - draft.queuedAttachments, - ); - }; - const persistCanceledDraft = () => { + const persistRecoverableDraft = () => { // Invitation cancellation still owes the captured draft recovery. Link // preview cancellation retains its existing independent recovery owner. - if (sendSignal?.aborted || !draft.recoveryDraftKey) return false; + if ( + sendSignal?.aborted || + !draft.recoveryDraftKey || + draft.sourceOwner.getComposerRevision() !== draft.composerRevision + ) + return false; const existing = drafts.loadDraft(draft.recoveryDraftKey); if ( existing && @@ -401,6 +391,18 @@ export function useMentionSendFlow({ ); return true; }; + const persistPreflightDraft = () => { + if ( + !draft.recoveryDraftKey || + isSendCancelled() || + !persistRecoverableDraft() + ) + return; + saveQueuedAttachmentsForDraft( + draft.recoveryDraftKey, + draft.queuedAttachments, + ); + }; let composerCleared = false; let optimisticComposerContent = ""; let clearedRevision = -1; @@ -409,8 +411,8 @@ export function useMentionSendFlow({ composerCleared = false; // An authored edit (even edit -> clear) ends optimistic recovery's // authority over this visit, including its persisted record and files. - if (ownsComposer() && getComposerRevision() !== clearedRevision) return; - const persisted = persistCanceledDraft(); + if (draft.sourceOwner.getComposerRevision() !== clearedRevision) return; + const persisted = persistRecoverableDraft(); const canAnimateCurrentComposer = isMountedRef.current && ownsComposer(); if ( @@ -635,7 +637,7 @@ export function useMentionSendFlow({ } if ( draft.sentDraftKey && - (!ownsComposer() || getComposerRevision() === clearedRevision) && + draft.sourceOwner.getComposerRevision() === clearedRevision && JSON.stringify( drafts.loadDraft(draft.sentDraftKey)?.mentionRefs ?? [], ) === JSON.stringify(draft.savedMentionRefs) diff --git a/desktop/src/features/messages/ui/useMentionSendFlow.types.ts b/desktop/src/features/messages/ui/useMentionSendFlow.types.ts index 05bc6beb3f..ac82b74eca 100644 --- a/desktop/src/features/messages/ui/useMentionSendFlow.types.ts +++ b/desktop/src/features/messages/ui/useMentionSendFlow.types.ts @@ -10,6 +10,7 @@ import type { UseRichTextEditorResult } from "@/features/messages/lib/useRichTex import type { UseDraftsResult } from "@/features/messages/lib/useDrafts"; export type UseMentionSendFlowOptions = { + /** Lifecycle accessor bound to this visit, still readable after it exits. */ getComposerRevision: () => number; runComposerUpdate: (update: () => void) => void; channelId: string | null; diff --git a/desktop/tests/e2e/remote-owned-mentions.spec.ts b/desktop/tests/e2e/remote-owned-mentions.spec.ts index 3d0e36ca40..c8c75cd725 100644 --- a/desktop/tests/e2e/remote-owned-mentions.spec.ts +++ b/desktop/tests/e2e/remote-owned-mentions.spec.ts @@ -458,3 +458,104 @@ for (const stage of ["add", "publish"] as const) { }); } } + +for (const incoming of ["unrelated thread B draft", "@RemoteScout hello"]) { + test(`B1 authored deletion before thread switch preserves storage and ${incoming}`, async ({ + page, + }) => { + await install(page); + await expect + .poll(() => + page.evaluate( + () => + window.__BUZZ_E2E_HAS_MOCK_LIVE_SUBSCRIPTION__?.({ + channelName: "general", + }) ?? false, + ), + ) + .toBe(true); + const roots = await page.evaluate(() => + ["Lifecycle thread A", "Lifecycle thread B"].map( + (content) => + window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__?.({ + channelName: "general", + content, + })?.id, + ), + ); + expect(roots.every(Boolean)).toBe(true); + const navigate = async (id: string | undefined) => { + // Drive the real reply route handler, including while the modal has + // focus (the equivalent external history/navigation intent). + await page + .getByTestId(`reply-message-${id}`) + .first() + .evaluate((button) => (button as HTMLButtonElement).click()); + await expect(page.getByTestId("message-thread-panel")).toBeVisible(); + await expect(page.getByTestId("message-thread-head")).toContainText( + id === roots[0] ? "Lifecycle thread A" : "Lifecycle thread B", + ); + }; + const input = page + .getByTestId("message-thread-panel") + .getByTestId("message-input"); + // Visit both first: the transition under test must reuse the available + // panel/composer, not get a fortuitous unmount via a loading skeleton. + await navigate(roots[1]); + await input.fill(incoming); + await navigate(roots[0]); + await expect(input).toHaveText(""); + await input.fill("@Remote"); + await page + .getByTestId(`mention-suggestion-${REMOTE}`) + .locator("button") + .first() + .click(); + await page.keyboard.type("hello"); + await page + .getByTestId("message-thread-panel") + .getByTestId("send-message") + .click(); + await holdInviteCommand(page, "revalidate_relay_agents", 2); + await page.getByRole("button", { name: "Invite", exact: true }).click(); + await waitForInviteGate(page); + await expect(input).toHaveText(""); + await input.fill("new authored text"); + await input.fill(""); + const sourceRecord = () => + page.evaluate(([root, otherRoot]) => { + const key = Object.keys(localStorage).find((key) => + key.startsWith("buzz-drafts.v2"), + ); + if (!key) throw new Error("draft storage scope missing"); + const drafts = JSON.parse(localStorage.getItem(key) ?? "{}"); + if (!drafts[`thread:${otherRoot}`]) + throw new Error("control B draft missing"); + return drafts[`thread:${root}`] ?? null; + }, roots); + expect(await sourceRecord()).toBeNull(); + // Expando proves the actual editor DOM host survived A -> B. + await input.evaluate((el) => + el.setAttribute("data-lifecycle-host", "retained"), + ); + await navigate(roots[1]); + await expect(input).toHaveAttribute("data-lifecycle-host", "retained"); + await expect(input).toHaveText(incoming); + await expect(page.getByRole("alertdialog")).toHaveCount(0); + // Read actual persistence, not the editor (whose tombstone could mask a + // resurrected record until reload). Neither text nor exact refs may return. + expect(await sourceRecord()).toBeNull(); + await navigate(roots[0]); + await expect(input).toHaveText(""); + + await navigate(roots[1]); + await releaseInviteCommand(page); + await page.waitForTimeout(300); + expect(await sent(page)).toEqual([]); + await expect(input).toHaveText(incoming); + await navigate(roots[0]); + await expect(input).toHaveText(""); + expect(await sourceRecord()).toBeNull(); + await assertNoLocalLifecycle(page); + }); +} diff --git a/docs/remote-mention-routing.md b/docs/remote-mention-routing.md index 0a985edd7f..5d4ea14ce7 100644 --- a/docs/remote-mention-routing.md +++ b/docs/remote-mention-routing.md @@ -52,8 +52,13 @@ visible pending, Escape/retry and route navigation at deferred IPC boundaries. Invitation intent belongs to one visit of the effective persistence key and channel, not just the mounted composer or destination channel. Same-channel -thread changes and A → B → A invalidate the original visit. The same owner gates -optimistic clear, recovery and pending state. Invalidation makes recovery durable +thread changes and A → B → A invalidate the original visit. The visible owner gates +optimistic clear, editor recovery and pending state. Storage recovery instead +consults the captured source visit's authored revision even after that visit +exits; losing visible ownership never revokes an authored deletion. The lifecycle +retains a per-visit revision record through its captured accessor, so B's edits +cannot authorize or suppress A's recovery. Preflight recovery and sent-draft +cleanup use that same source authority. Invalidation makes recovery durable synchronously, before a later visit can load or edit the source draft; late async completion cannot revive that recovery or release a newer attempt's latch.