From efdc6a1dc3a0ccac0863d0db2e4bc077df260466 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 26 Sep 2026 16:06:28 +0200 Subject: [PATCH] fix: keep a reply draft in its thread when it is re-opened or undone Drafts were saved without In-Reply-To and References, and a draft re-opens in compose mode, where the composer has no original message to rebuild them from. A reply that was saved and re-opened, or sent and brought back with Undo, therefore went out as a new conversation. `createDraft` now stores the threading headers, the composer saves them with every draft of a reply, and re-opening a draft carries them into the composer for later saves and the send. --- .../composer-reply-draft-threading.test.tsx | 231 ++++++++++++++++++ components/email/email-composer.tsx | 28 ++- components/mail/mail-app.tsx | 4 + lib/__tests__/jmap-draft-lifecycle.test.ts | 17 ++ lib/jmap/client-interface.ts | 2 + lib/jmap/client.ts | 12 +- 6 files changed, 288 insertions(+), 6 deletions(-) create mode 100644 components/email/__tests__/composer-reply-draft-threading.test.tsx diff --git a/components/email/__tests__/composer-reply-draft-threading.test.tsx b/components/email/__tests__/composer-reply-draft-threading.test.tsx new file mode 100644 index 000000000..a7b0c951e --- /dev/null +++ b/components/email/__tests__/composer-reply-draft-threading.test.tsx @@ -0,0 +1,231 @@ +import { render, screen, fireEvent, waitFor, act, within } from '@testing-library/react'; +import { describe, it, expect, vi } from 'vitest'; +import React from 'react'; +import { EmailComposer } from '../email-composer'; +import { useAuthStore } from '@/stores/auth-store'; + +// A reply draft re-opens in compose mode, where the composer used to have +// nothing to rebuild In-Reply-To / References from: every later save (and the +// send) dropped the reply out of its thread. + +// ─── Heavy component mocks (mirrors composer-close-guard.test.tsx) ────────── + +vi.mock('@/components/email/rich-text-editor', () => ({ + RichTextEditor: () => React.createElement('div', { 'data-testid': 'rich-text-editor' }), +})); + +vi.mock('@/components/plugins/plugin-slot', () => ({ PluginSlot: () => null })); +vi.mock('@/components/identity/sub-address-helper', () => ({ SubAddressHelper: () => null })); +vi.mock('@/components/templates/template-picker', () => ({ TemplatePicker: () => null })); +vi.mock('@/components/templates/template-form', () => ({ TemplateForm: () => null })); +vi.mock('@/components/files/file-preview-modal', () => ({ FilePreviewModal: () => null })); +vi.mock('@/hooks/use-focus-trap', () => ({ + useFocusTrap: () => ({ current: null }), +})); +vi.mock('@/hooks/use-pro-multi-account-identities', () => ({ + useProMultiAccountIdentities: () => ({ enabled: false, groups: [], allIdentities: [] }), + stripCrossAccountIdentityPrefix: (id: string) => ({ localAccountId: null, rawId: id }), +})); + +// ─── Store mocks ────────────────────────────────────────────────────────────── + +vi.mock('@/stores/auth-store', () => { + const state = { + client: null, + identities: [], + primaryIdentity: null, + isAuthenticated: false, + isDemoMode: false, + activeAccountId: null, + connectionLost: false, + getClientForAccount: () => undefined, + getAllConnectedClients: () => new Map(), + syncIdentities: () => {}, + refreshIdentities: async () => {}, + }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = (p: Partial) => Object.assign(state, p); + return { useAuthStore: hook }; +}); + +vi.mock('@/stores/identity-store', () => { + const state = { + identities: [{ id: 'id-me', email: 'me@example.com', name: 'Me' }], + defaultIdentityId: 'id-me', + }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = (p: Partial) => Object.assign(state, p); + return { useIdentityStore: hook }; +}); + +vi.mock('@/stores/account-store', () => { + const state = { accounts: [], getAccountById: () => undefined }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = (p: Partial) => Object.assign(state, p); + return { useAccountStore: hook }; +}); + +vi.mock('@/stores/email-store', () => { + const state = { + draftSaveEnabled: false, + sendRawEmail: async () => ({ sent: true }), + }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = (p: Partial) => Object.assign(state, p); + return { useEmailStore: hook }; +}); + +const updateSetting = vi.fn(); + +vi.mock('@/stores/settings-store', () => { + const state = { + timeFormat: '24h', + plainTextMode: false, + subAddressDelimiter: '+', + autoSelectReplyIdentity: true, + attachmentReminderEnabled: false, + attachmentReminderKeywords: [], + emptySubjectWarningEnabled: true, + sendDelaySeconds: 0, + signaturePosition: 'above_quote', + signatureSeparatorEnabled: false, + requestReadReceiptDefault: false, + addTrustedSender: () => {}, + trustedSendersAddressBook: null, + updateSetting: (...args: unknown[]) => updateSetting(...args), + }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = (p: Partial) => Object.assign(state, p); + return { useSettingsStore: hook }; +}); + +vi.mock('@/stores/contact-store', () => { + const state = { + contacts: [], + getAutocomplete: async () => [], + addToTrustedSendersBook: async () => {}, + }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = (p: Partial) => Object.assign(state, p); + return { useContactStore: hook }; +}); + +vi.mock('@/stores/template-store', () => { + const state = { templates: [], addTemplate: async () => {} }; + const hook = (sel?: (s: typeof state) => unknown) => + typeof sel === 'function' ? sel(state) : state; + hook.getState = () => state; + hook.setState = (p: Partial) => Object.assign(state, p); + return { useTemplateStore: hook }; +}); + +// ─── Misc dependency mocks ──────────────────────────────────────────────────── + +vi.mock('@/stores/toast-store', () => ({ + toast: { info: () => {}, error: () => {}, success: () => {} }, +})); + +vi.mock('@/lib/plugin-hooks', () => ({ + emailHooks: { + onComposerOpen: { call: async () => [] }, + onRecipientChange: { call: async () => [] }, + getRecipientSuggestions: { call: async () => [] }, + onRecipientChipsChange: { transform: async (chips: unknown) => chips }, + onDraftChange: { emit: () => {} }, + onBeforeDraftAutoSave: { transform: async (draft: unknown) => draft }, + onBeforeEmailSend: { intercept: async () => true }, + onComposeSend: { intercept: async () => true }, + onTransformOutgoingEmail: { transform: async (email: unknown) => email }, + }, + contactHooks: { + search: { call: async () => [] }, + onProvideRecipientSuggestions: { transform: async (initial: unknown) => initial }, + }, +})); + +vi.mock('@/lib/email-sanitization', () => ({ + sanitizeSignatureHtml: (v: string) => v, + sanitizeEmailHtml: (v: string) => v, + parseHtmlSafely: (html: string) => new DOMParser().parseFromString(html, 'text/html'), +})); + +vi.mock('@/lib/email-threading', () => ({ + computeReplyThreadingHeaders: () => ({ inReplyTo: [], references: [] }), +})); +vi.mock('@/lib/signature-utils', () => ({ + appendPlainTextSignature: (body: string) => body, + getPlainTextSignature: () => '', +})); +vi.mock('@/lib/sub-addressing', () => ({ generateSubAddress: () => '' })); +vi.mock('@/lib/debug', () => ({ debug: { log: () => {}, warn: () => {}, error: () => {} } })); +vi.mock('@/components/email/quoted-html', () => ({ + buildQuotedHtmlBlock: () => '', + serializeEditorContent: () => '', +})); +vi.mock('@/lib/template-utils', () => ({ substitutePlaceholders: (s: string) => s })); + +// ─── Tests ──────────────────────────────────────────────────────────────────── + +/** + * A host that replaces the compose session - a mailto: click, say - must not + * drop text the user has not sent. It hands a follow-up to requestCloseRef and + * the composer runs it only once it has really closed, so "Save" and "Discard" + * continue into the new session while "Cancel" leaves the draft standing. + */ + +const REOPENED_REPLY = { + to: 'bob@example.com', + cc: '', + bcc: '', + subject: 'Re: Plans', + body: '

Still typing

', + showCc: false, + showBcc: false, + selectedIdentityId: 'id-me', + subAddressTag: '', + mode: 'compose' as const, + draftId: 'draft-7', + inReplyTo: ['parent@example.com'], + references: ['root@example.com', 'parent@example.com'], +}; + +describe('re-opened reply draft', () => { + it('saves with the threading headers it was opened with', async () => { + const createDraft = vi.fn().mockResolvedValue('draft-8'); + useAuthStore.setState({ + client: { + createDraft, + getEmail: vi.fn().mockResolvedValue({ id: 'draft-8', attachments: [] }), + hasDelayedSend: () => false, + getMaxDelayedSend: () => 0, + } as never, + activeAccountId: 'acct-A' as never, + }); + + const requestCloseRef = { current: null } as React.MutableRefObject<((afterClose?: () => void) => void) | null>; + render(); + fireEvent.change(screen.getByDisplayValue('Re: Plans'), { target: { value: 'Re: Plans, edited' } }); + + act(() => requestCloseRef.current!(vi.fn())); + const dialog = await screen.findByRole('alertdialog'); + fireEvent.click(within(dialog).getByText('save')); + + await waitFor(() => expect(createDraft).toHaveBeenCalled()); + const args = createDraft.mock.calls[0]; + expect(args[11]).toEqual(['parent@example.com']); + expect(args[12]).toEqual(['root@example.com', 'parent@example.com']); + useAuthStore.setState({ client: null, activeAccountId: null }); + }); +}); diff --git a/components/email/email-composer.tsx b/components/email/email-composer.tsx index 8c2c18f79..723807d23 100644 --- a/components/email/email-composer.tsx +++ b/components/email/email-composer.tsx @@ -45,7 +45,7 @@ import type { EmailTemplate } from "@/lib/template-types"; import { appendPlainTextSignature, getPlainTextSignature, plainTextBodyHasSignature, plainTextBodyWithoutSignature } from "@/lib/signature-utils"; import { findComposeIdentityId, findDraftIdentityId, findReplyIdentityId, resolveReplyFrom } from "@/lib/reply-identity"; import { buildReplyRecipients, isSelfSent } from "@/lib/reply-recipients"; -import { computeReplyThreadingHeaders } from "@/lib/email-threading"; +import { computeReplyThreadingHeaders, type ReplyThreadingHeaders } from "@/lib/email-threading"; import { RequestTimeoutError, ScheduleTooLateError } from "@/lib/jmap/client"; import { rewriteCidImagesForEditor, @@ -142,6 +142,13 @@ export interface ComposerDraftData { * format it was written in (#1022). */ plainTextMode?: boolean; + /** + * Threading headers of a re-opened reply draft (RFC 5322 In-Reply-To / + * References). A draft re-opens in "compose" mode, where nothing else + * rebuilds them, so they ride along or the reply drops out of its thread. + */ + inReplyTo?: string[]; + references?: string[]; } interface EmailComposerProps { @@ -730,6 +737,17 @@ export function EmailComposer({ const composerClientRef = useRef(composerClient); composerClientRef.current = composerClient; const currentIdentityRawId = currentIdentityParts.rawId ?? currentIdentity?.id; + // RFC 5322 §3.6.4 threading for this message: computed from the original + // on a reply (not a forward), carried over from a re-opened reply draft. + // Drafts store it too, so a reply stays in its thread when re-opened or + // re-sent after Undo. + const replyThreadingHeaders = useMemo(() => { + if (mode === 'reply' || mode === 'replyAll') return computeReplyThreadingHeaders(replyTo); + if (initialData?.inReplyTo?.length) { + return { inReplyTo: initialData.inReplyTo, references: initialData.references ?? [] }; + } + return null; + }, [mode, replyTo, initialData?.inReplyTo, initialData?.references]); // A From override is asked for as the envelope MAIL FROM too, but a server // may only accept the identity's own address there (Stalwart does), which // then shows in the Return-Path. Unless one of the account's identities owns @@ -1894,7 +1912,9 @@ export function EmailComposer({ savedDraft.draftId, savedDraft.attachments, savedDraft.fromName, - savedDraft.htmlBody + savedDraft.htmlBody, + replyThreadingHeaders?.inReplyTo, + replyThreadingHeaders?.references, ); // Update the ref synchronously so a queued save sees the new id and @@ -2285,9 +2305,7 @@ export function EmailComposer({ }; // RFC 5322 §3.6.4 threading - only continues the chain on a reply, not a forward. - const threadingHeaders = (mode === 'reply' || mode === 'replyAll') - ? computeReplyThreadingHeaders(replyTo) - : null; + const threadingHeaders = replyThreadingHeaders; // In plain text mode, send text/plain only (no HTML body) const signatureOpts = { separator: signatureSeparatorEnabled }; diff --git a/components/mail/mail-app.tsx b/components/mail/mail-app.tsx index 0924db809..8c18a2bb8 100644 --- a/components/mail/mail-app.tsx +++ b/components/mail/mail-app.tsx @@ -1959,6 +1959,10 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { subAddressTag: '', mode: 'compose', draftId: draft.id, + // A reply draft re-opens in compose mode; its threading headers must + // come along or the reply leaves its thread. + inReplyTo: draft.inReplyTo ?? undefined, + references: draft.references ?? undefined, // Existing server-side attachments must ride along, or the composer // starts empty and the next save/send silently rebuilds the draft // without them (#849). diff --git a/lib/__tests__/jmap-draft-lifecycle.test.ts b/lib/__tests__/jmap-draft-lifecycle.test.ts index c3e7200e6..a01fc2cae 100644 --- a/lib/__tests__/jmap-draft-lifecycle.test.ts +++ b/lib/__tests__/jmap-draft-lifecycle.test.ts @@ -103,6 +103,23 @@ describe('draft replace lifecycle (#849)', () => { expect(sets[0].destroy).toBeUndefined(); }); + it('createDraft keeps a reply draft in its thread', async () => { + // A reply draft re-opened (or restored by Undo) has nothing else to + // rebuild In-Reply-To / References from, so the draft must carry them. + const client = createClient(); + const captured = mockFlow(); + + await client.createDraft( + ['bob@example.com'], 'Re: Subject', 'body', + undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, + [''], ['root@example.com', 'parent@example.com'], + ); + + const created = Object.values(emailSetCalls(captured)[0].create as Record>)[0]; + expect(created.inReplyTo).toEqual(['parent@example.com']); + expect(created.references).toEqual(['root@example.com', 'parent@example.com']); + }); + it('createDraft destroys the previous version only after the create succeeded', async () => { const client = createClient(); const captured = mockFlow(); diff --git a/lib/jmap/client-interface.ts b/lib/jmap/client-interface.ts index 04038a7c8..709901cc5 100644 --- a/lib/jmap/client-interface.ts +++ b/lib/jmap/client-interface.ts @@ -287,6 +287,8 @@ export interface IJMAPClient { attachments?: Array<{ blobId: string; name: string; type: string; size: number; disposition?: 'attachment' | 'inline'; cid?: string }>, fromName?: string, htmlBody?: string, + inReplyTo?: string[], + references?: string[], ): Promise; sendEmail( diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index a556e4d63..c9602b83e 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -3576,7 +3576,9 @@ export class JMAPClient implements IJMAPClient { draftId?: string, attachments?: Array<{ blobId: string; name: string; type: string; size: number; disposition?: 'attachment' | 'inline'; cid?: string }>, fromName?: string, - htmlBody?: string + htmlBody?: string, + inReplyTo?: string[], + references?: string[], ): Promise { const mailboxes = await this.getMailboxes(); const draftsMailbox = mailboxes.find(mb => mb.role === 'drafts'); @@ -3598,9 +3600,15 @@ export class JMAPClient implements IJMAPClient { textBody: { partId: string; type?: string }[]; htmlBody?: { partId: string; type: string }[]; attachments?: { blobId: string; type: string; name: string; disposition: string; cid?: string }[]; + inReplyTo?: string[]; + references?: string[]; } const sanitizedFromName = sanitizeIdentityDisplayName(fromName); + // A reply draft keeps its thread: re-opened or re-sent after Undo, it has + // nothing else to rebuild In-Reply-To / References from. + const draftInReplyTo = inReplyTo?.map(stripMessageIdBrackets).filter(Boolean); + const draftReferences = references?.map(stripMessageIdBrackets).filter(Boolean); const emailData: EmailDraft = { from: [{ ...(sanitizedFromName ? { name: sanitizedFromName } : {}), email: fromEmail || this.username }], // "Name " must be split into the two JMAP fields: storing the whole @@ -3619,6 +3627,8 @@ export class JMAPClient implements IJMAPClient { ? [{ partId: "text", type: "text/plain" }] : [{ partId: "1" }], ...(htmlBody ? { htmlBody: [{ partId: "html", type: "text/html" }] } : {}), + ...(draftInReplyTo?.length ? { inReplyTo: draftInReplyTo } : {}), + ...(draftReferences?.length ? { references: draftReferences } : {}), }; if (attachments?.length) {