From 053b1ef343e73a28336a1b5df23dc9308c2758bc Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 3 Sep 2026 15:40:07 +1000 Subject: [PATCH] fix(desktop): keep agent prefills durable across in-app navigation Address three review findings on the draft entryKind classification: - Keep isValidDraftState a pure structural predicate; move the migration-era defaulting (mentionRefs, status, entryKind) into normalizeStoredDraft, applied in readStore's single read funnel. - Skip the implicit agent-mention prefix strip in the unmount persist cleanup when the entry is an agent-prefill: a pure prefill IS the prefix, so stripping persisted "" and cleared the record on every in-app SPA navigation. Storage-boundary normalization still demotes anything that is not mention-only, and agent-prefill records remain excluded from Inbox draft rows and counts. - Bind the fix in tests: unit coverage now exercises the cleanup with a real getImplicitAgentMentionPrefix getter (durable pure prefill, authored drafts still stripped, no Inbox pollution), and the e2e spec adds an in-app navigation leg so the composer-unmount persist path is exercised and the stored classification is asserted to survive it. Collateral fixes surfaced by verification: - Migrate the two prefill e2e tests to thread scope: since #7144 made automatic mentions thread-only, the channel-composer variants were broken on rebase (they fail on the unmodified base commit too). - Preserve trailing spaces in useRichTextEditor's setContent: the markdown re-parse drops them, but a restored prefill's separator ("@Name ") must survive so the caret lands past it and the next keystroke does not fuse into the mention. Verified with the full desktop unit suite (6067 passing) and the persistent-agent-audience Playwright suite (37 passing); both new guards were falsified against the reverted fix. Co-Authored-By: Claude Fable 5 Signed-off-by: Matt Toohey --- .../src/features/messages/lib/useDrafts.ts | 68 ++++++---- .../messages/lib/useRichTextEditor.ts | 16 +++ .../MessageComposerDraftImagePersist.test.mjs | 116 ++++++++++++++++++ .../messages/ui/useDraftPersistSnapshot.ts | 25 +++- .../e2e/persistent-agent-audience.spec.ts | 69 +++++++++-- 5 files changed, 250 insertions(+), 44 deletions(-) diff --git a/desktop/src/features/messages/lib/useDrafts.ts b/desktop/src/features/messages/lib/useDrafts.ts index f74c7a273c..c20793b95a 100644 --- a/desktop/src/features/messages/lib/useDrafts.ts +++ b/desktop/src/features/messages/lib/useDrafts.ts @@ -76,7 +76,8 @@ export type DraftState = { * The `"sent"` value is not written by any production path; legacy `sent:` * keyed records from older builds are dropped on read by `readStore`. * Entries persisted before this field was added have no status field — - * the read path treats absent status as `"active"` (see `isValidDraftState`). + * the read path treats absent status as `"active"` (see + * `normalizeStoredDraft`). */ status: "active" | "sent"; }; @@ -197,7 +198,7 @@ function readStore(): Map { continue; } if (isValidDraftState(value)) { - map.set(key, value); + map.set(key, normalizeStoredDraft(value)); } } } @@ -214,6 +215,12 @@ function readStore(): Map { return map; } +/** + * Pure structural predicate over a parsed store entry — it never writes to + * its argument. Migration-era absences (`mentionRefs`, `status`, `entryKind`) + * are accepted here and defaulted by `normalizeStoredDraft` at the read + * boundary in `readStore`. + */ function isValidDraftState(v: unknown): v is DraftState { if (typeof v !== "object" || v === null) return false; const d = v as Partial; @@ -229,39 +236,50 @@ function isValidDraftState(v: unknown): v is DraftState { ) { return false; } - // Migration: drafts written before mention routing was persisted have no - // mentionRefs. Preserve them as ordinary drafts with no selected identities. - if (d.mentionRefs === undefined) { - (d as DraftState).mentionRefs = []; - } else if ( - !Array.isArray(d.mentionRefs) || - d.mentionRefs.some( - (ref) => - typeof ref !== "object" || - ref === null || - typeof ref.displayName !== "string" || - ref.displayName.trim().length === 0 || - typeof ref.pubkey !== "string" || - ref.pubkey.trim().length === 0 || - typeof ref.isAgent !== "boolean", - ) + // Drafts written before mention routing was persisted have no mentionRefs — + // accepted, and preserved as ordinary drafts with no selected identities. + if ( + d.mentionRefs !== undefined && + (!Array.isArray(d.mentionRefs) || + d.mentionRefs.some( + (ref) => + typeof ref !== "object" || + ref === null || + typeof ref.displayName !== "string" || + ref.displayName.trim().length === 0 || + typeof ref.pubkey !== "string" || + ref.pubkey.trim().length === 0 || + typeof ref.isAgent !== "boolean", + )) ) { return false; } - // Migration: entries written before the status field was introduced have no - // status field. Treat absent status as "active" to avoid data loss on the - // first run after the upgrade. + // Entries written before the status field was introduced have no status + // field — accepted, to avoid data loss on the first run after the upgrade. // Legacy sent: keys are skipped by readStore before reaching this function; // reject any remaining entry whose status is not "active". - if (d.status === undefined || d.status === null) { - (d as DraftState).status = "active"; - } else if (d.status !== "active") { + if (d.status !== undefined && d.status !== null && d.status !== "active") { return false; } - (d as DraftState).entryKind = normalizeDraftEntryKind(d); return true; } +/** + * Canonicalize a validated store entry at the read boundary: default the + * migration-era optional fields (`mentionRefs`, `status`) and demote any + * stored `entryKind` that no longer satisfies the agent-prefill invariants. + * This lives in the read funnel rather than inside `isValidDraftState` so + * validation stays a pure predicate. + */ +function normalizeStoredDraft(draft: DraftState): DraftState { + return { + ...draft, + mentionRefs: draft.mentionRefs ?? [], + status: draft.status ?? "active", + entryKind: normalizeDraftEntryKind(draft), + }; +} + function normalizeDraftEntryKind(draft: Partial): DraftEntryKind { if (draft.entryKind !== "agent-prefill") return "draft"; const mentionRefs = draft.mentionRefs; diff --git a/desktop/src/features/messages/lib/useRichTextEditor.ts b/desktop/src/features/messages/lib/useRichTextEditor.ts index 40ab8c593a..cc9b7001b3 100644 --- a/desktop/src/features/messages/lib/useRichTextEditor.ts +++ b/desktop/src/features/messages/lib/useRichTextEditor.ts @@ -709,6 +709,22 @@ export function useRichTextEditor({ (markdown: string) => { if (!editor) return; editor.commands.setContent(markdown); + // The markdown re-parse drops trailing spaces, but a restored draft's + // trailing run can be meaningful: an automatic agent prefill is exactly + // "@Name " and its separator must survive restoration so the caret + // lands past it and the next keystroke does not fuse into the mention. + // Re-insert whatever the parse lost, without emitting an update. + const trailingSpaces = / +$/.exec(markdown)?.[0] ?? ""; + if (!trailingSpaces) return; + const doc = editor.state.doc; + const docText = doc.textBetween(0, doc.content.size, "\n", "\n"); + const preserved = / +$/.exec(docText)?.[0] ?? ""; + const missing = trailingSpaces.slice(preserved.length); + if (!missing) return; + const end = TextSelection.atEnd(doc).from; + editor.view.dispatch( + editor.state.tr.insertText(missing, end).setMeta("preventUpdate", true), + ); }, [editor], ); diff --git a/desktop/src/features/messages/ui/MessageComposerDraftImagePersist.test.mjs b/desktop/src/features/messages/ui/MessageComposerDraftImagePersist.test.mjs index 64dc4d4001..fe98deee6d 100644 --- a/desktop/src/features/messages/ui/MessageComposerDraftImagePersist.test.mjs +++ b/desktop/src/features/messages/ui/MessageComposerDraftImagePersist.test.mjs @@ -240,6 +240,7 @@ import { useAgentPrefillDraftBridge } from "./useAgentPrefillDraftBridge.ts"; import { clearAllDrafts, deleteDraftEntry, + getInboxDraftEntries, initDraftStore, loadDraftEntry, persistDraftEntry, @@ -1124,6 +1125,121 @@ test("failed_send_restores_the_captured_entry_kind", async () => { assert.equal(loadDraftEntry(draftKey)?.content, "authored message"); }); +test("unmount_cleanup_keeps_a_pure_prefill_durable_with_a_real_prefix_getter", async () => { + const draftKey = "chan-prefill-unmount-strip"; + setupStore("pubkey-prefill-unmount-strip"); + persistDraftEntry( + draftKey, + "@Jitter ", + draftKey, + [], + [], + AGENT_PREFILL_REFS, + "agent-prefill", + ); + + let editorContent = ""; + const spoileredRef = { current: new Set() }; + function HarnessComposer() { + useDraftPersistLifecycle({ + effectiveDraftKey: draftKey, + channelId: draftKey, + loadDraft: loadDraftEntry, + persistDraft: persistDraftEntry, + getMentionRefs: (content) => + content.includes("@Jitter") ? AGENT_PREFILL_REFS : [], + restoreMentionRefs: () => {}, + livePendingImeta: [], + setPendingImeta: () => {}, + setContent: (content) => { + editorContent = content; + }, + clearContent: () => { + editorContent = ""; + }, + setSpoileredAttachmentUrls: () => {}, + spoileredAttachmentUrlsRef: spoileredRef, + syncComposerContentFromEditor: () => editorContent, + // The production wiring supplies the implicit-prefix getter, so the + // cleanup's persistedContent() would strip a pure prefill to "" — and an + // empty persist clears the stored record. In-app navigation unmounts the + // composer through exactly this path. + getImplicitAgentMentionPrefix: () => "@Jitter ", + }); + return null; + } + + const handle = await mountStrictMode(HarnessComposer); + assert.equal(editorContent, "@Jitter "); + await handle.unmount(); + + assert.equal( + loadDraftEntry(draftKey)?.entryKind, + "agent-prefill", + "the stored agent-prefill classification must survive composer unmount", + ); + assert.equal(loadDraftEntry(draftKey)?.content, "@Jitter "); + assert.equal( + getInboxDraftEntries().some(({ key }) => key === draftKey), + false, + "the surviving prefill record must stay excluded from Inbox drafts", + ); +}); + +test("unmount_cleanup_still_strips_the_implicit_prefix_from_authored_drafts", async () => { + const draftKey = "chan-authored-unmount-strip"; + setupStore("pubkey-authored-unmount-strip"); + persistDraftEntry( + draftKey, + "@Jitter ", + draftKey, + [], + [], + AGENT_PREFILL_REFS, + "agent-prefill", + ); + + let editorContent = ""; + let trackAuthoredContent; + const spoileredRef = { current: new Set() }; + function HarnessComposer() { + ({ trackAuthoredContent } = useDraftPersistLifecycle({ + effectiveDraftKey: draftKey, + channelId: draftKey, + loadDraft: loadDraftEntry, + persistDraft: persistDraftEntry, + getMentionRefs: (content) => + content.includes("@Jitter") ? AGENT_PREFILL_REFS : [], + restoreMentionRefs: () => {}, + livePendingImeta: [], + setPendingImeta: () => {}, + setContent: (content) => { + editorContent = content; + }, + clearContent: () => { + editorContent = ""; + }, + setSpoileredAttachmentUrls: () => {}, + spoileredAttachmentUrlsRef: spoileredRef, + syncComposerContentFromEditor: () => editorContent, + getImplicitAgentMentionPrefix: () => "@Jitter ", + })); + return null; + } + + const handle = await mountStrictMode(HarnessComposer); + editorContent = "@Jitter hello"; + trackAuthoredContent(editorContent); + await handle.unmount(); + + assert.equal( + loadDraftEntry(draftKey)?.content, + "hello", + "authored drafts keep stripping the implicit prefix on unmount", + ); + assert.equal(loadDraftEntry(draftKey)?.entryKind, "draft"); +}); + function mountAgentPrefillBridge(overrides = {}) { let bridge; const contentRef = { current: "@Jitter " }; diff --git a/desktop/src/features/messages/ui/useDraftPersistSnapshot.ts b/desktop/src/features/messages/ui/useDraftPersistSnapshot.ts index abb1d25028..ab4d43afa5 100644 --- a/desktop/src/features/messages/ui/useDraftPersistSnapshot.ts +++ b/desktop/src/features/messages/ui/useDraftPersistSnapshot.ts @@ -221,9 +221,25 @@ export function useDraftPersistLifecycle({ if (queuedAttachments.length > 0) { saveQueuedAttachmentsForDraft?.(effectiveDraftKey, queuedAttachments); } - const content = emptyContentIsAuthoritativeRef.current + const entryKind = + pendingImetaForPersistRef.current.length > 0 || + queuedAttachments.length > 0 + ? "draft" + : entryKindRef.current; + const editorContent = emptyContentIsAuthoritativeRef.current ? "" - : persistedContent(syncComposerContentFromEditor()); + : syncComposerContentFromEditor(); + // A pure automatic prefill IS the implicit prefix: stripping it here + // would persist "" (which clears the record), so the agent-prefill + // classification would survive full reloads but not in-app + // navigation. Any authored edit or retained attachment has already + // demoted entryKind to "draft", and the storage boundary re-demotes + // anything that is not mention-only, so skipping the strip cannot + // leak an implicit prefix into an Inbox-visible draft. + const content = + entryKind === "agent-prefill" + ? editorContent + : persistedContent(editorContent); persistDraft( effectiveDraftKey, content, @@ -231,10 +247,7 @@ export function useDraftPersistLifecycle({ [...pendingImetaForPersistRef.current], [...spoileredAttachmentUrlsRef.current], getMentionRefs(content), - pendingImetaForPersistRef.current.length > 0 || - queuedAttachments.length > 0 - ? "draft" - : entryKindRef.current, + entryKind, ); } }; diff --git a/desktop/tests/e2e/persistent-agent-audience.spec.ts b/desktop/tests/e2e/persistent-agent-audience.spec.ts index 7c354cc8cc..6f5e69b150 100644 --- a/desktop/tests/e2e/persistent-agent-audience.spec.ts +++ b/desktop/tests/e2e/persistent-agent-audience.spec.ts @@ -8,6 +8,8 @@ const CHANNEL_ID = "9a1657ac-f7aa-5db0-b632-d8bbeb6dfb50"; const RANDOM_CHANNEL_ID = "9dae0116-799b-5071-a0a8-fdd30a91a35d"; const AGENT_A = "a".repeat(64); const AGENT_B = "b".repeat(64); +// Active mock identity (DEFAULT_MOCK_IDENTITY.pubkey in e2eBridge.ts). +const OWN_PUBKEY = "deadbeef".repeat(8); const THREAD_ROOT_ID = "mock-general-welcome"; const KEEP_MENTIONED_AGENTS_PINNED_STORAGE_KEY = "buzz.messages.keepMentionedAgentsPinned"; @@ -407,16 +409,22 @@ test("automatic agent prefill survives navigation and reload without becoming an page, }) => { await installAudienceFixtures(page); - await openGeneral(page); + await openThread(page); - const composer = channelComposer(page); + const composer = threadComposer(page); await automaticallyMention(composer, "Morgarita"); await expect(composer.getByTestId("message-input")).toHaveText("@Morgarita "); await expect .poll(() => readStoredDraftEntryKind(page, CHANNEL_ID)) .toBe("agent-prefill"); - await page.goto("/", { waitUntil: "domcontentloaded" }); + // In-app navigation (no full page load) unmounts the composer and runs the + // persist-lifecycle cleanup — the stored classification must survive that + // path too, not just full reloads. + await page + .getByTestId("sidebar-primary-menu") + .getByRole("button", { name: "Inbox", exact: true }) + .click(); await expect(page.getByTestId("home-inbox")).toBeVisible(); expect(await readStoredDraftEntryKind(page, CHANNEL_ID)).toBe( "agent-prefill", @@ -429,13 +437,24 @@ test("automatic agent prefill survives navigation and reload without becoming an ); await expect(page.getByTestId("inbox-draft-badge-option")).toHaveCount(0); - await openGeneral(page); - await expect(channelComposer(page).getByTestId("message-input")).toHaveText( + await openThread(page); + await expect(threadComposer(page).getByTestId("message-input")).toHaveText( + "@Morgarita ", + ); + + await page.goto("/", { waitUntil: "domcontentloaded" }); + await expect(page.getByTestId("home-inbox")).toBeVisible(); + expect(await readStoredDraftEntryKind(page, CHANNEL_ID)).toBe( + "agent-prefill", + ); + + await openThread(page); + await expect(threadComposer(page).getByTestId("message-input")).toHaveText( "@Morgarita ", ); await page.reload({ waitUntil: "domcontentloaded" }); - await expect(page.getByTestId("chat-title")).toHaveText("general"); - await expect(channelComposer(page).getByTestId("message-input")).toHaveText( + await expect(page.getByTestId("message-thread-panel")).toBeVisible(); + await expect(threadComposer(page).getByTestId("message-input")).toHaveText( "@Morgarita ", ); }); @@ -444,19 +463,43 @@ test("cancelling a message edit restores the automatic prefill classification", page, }) => { await installAudienceFixtures(page); - await openGeneral(page); + await openThread(page); + await waitForMockLiveSubscription(page, "general"); - const composer = channelComposer(page); + // Automatic mentions are thread-only, so the edit that displaces the + // prefill must target an own thread reply — routed to the thread composer. + const replyId = "e".repeat(64); + await page.evaluate( + ({ content, eventId, ownPubkey, rootId }) => { + window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__?.({ + channelName: "general", + content, + id: eventId, + parentEventId: rootId, + pubkey: ownPubkey, + }); + }, + { + content: "own reply before editing", + eventId: replyId, + ownPubkey: OWN_PUBKEY, + rootId: THREAD_ROOT_ID, + }, + ); + const threadPanel = page.getByTestId("message-thread-panel"); + const replyRow = threadPanel.locator(`[data-message-id="${replyId}"]`); + await expect(replyRow).toBeVisible(); + + const composer = threadComposer(page); const input = composer.getByTestId("message-input"); await automaticallyMention(composer, "Morgarita"); await expect .poll(() => readStoredDraftEntryKind(page, CHANNEL_ID)) .toBe("agent-prefill"); - const editableRow = page.locator(`[data-message-id="${THREAD_ROOT_ID}"]`); - await editableRow.hover(); - await editableRow.getByRole("button", { name: "More actions" }).click(); - await page.getByTestId(`edit-message-${THREAD_ROOT_ID}`).click(); + await replyRow.hover(); + await replyRow.getByRole("button", { name: "More actions" }).click(); + await page.getByTestId(`edit-message-${replyId}`).click(); await expect(composer.getByTestId("edit-target")).toBeVisible(); await composer.getByRole("button", { name: "Cancel edit" }).click();