From aa112b3397331b38115ff193ef3a90acacd384fb Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 17 Oct 2026 00:52:49 +0200 Subject: [PATCH] fix: sign replies whose From is picked after the composer opens #842 --- .../email/__tests__/compose-format.test.ts | 38 +++ .../composer-reply-signature.test.tsx | 318 ++++++++++++++++++ components/email/compose-format.ts | 37 +- components/email/email-composer.tsx | 45 ++- 4 files changed, 428 insertions(+), 10 deletions(-) create mode 100644 components/email/__tests__/composer-reply-signature.test.tsx diff --git a/components/email/__tests__/compose-format.test.ts b/components/email/__tests__/compose-format.test.ts index 53b34f970..939c856c7 100644 --- a/components/email/__tests__/compose-format.test.ts +++ b/components/email/__tests__/compose-format.test.ts @@ -5,6 +5,7 @@ import { plainComposeBodyToHtml, replaceEmbeddedSignature, replacePlainTextSignature, + replacePlainTextSignatureAboveQuote, stripEmbeddedSignature, } from '../compose-format'; import { SIGNATURE_BLOCK_MARKER, SIGNATURE_RANGE_MARKER, containsEmbeddedSignature } from '../signature-block'; @@ -131,6 +132,43 @@ describe('switching identity swaps the signature', () => { it('adds a signature at the end when there is no quote', () => { expect(replaceEmbeddedSignature('

Hi

', embed(other))).toBe(dom(`

Hi

${embed(other)}`)); }); + + it('adds a signature above the quote header when the body had none (#842)', () => { + const html = '

Hi

On Mon, Bob wrote:

original
'; + expect(replaceEmbeddedSignature(html, embed(other), { quoteHeader: 'On Mon, Bob wrote:' })) + .toBe(dom(`

Hi

${embed(other)}

On Mon, Bob wrote:

original
`)); + }); + + it('adds a signature above a forward header that has no quote element', () => { + const html = '

Hi

---------- Forwarded message ----------
From: Bob

original

'; + expect(replaceEmbeddedSignature(html, embed(other), { quoteHeader: '---------- Forwarded message ----------' })) + .toBe(dom(`

Hi

${embed(other)}

---------- Forwarded message ----------
From: Bob

original

`)); + }); + }); + + describe('replacePlainTextSignatureAboveQuote (#842)', () => { + const header = 'On Mon, Bob wrote:'; + const quote = `${header}\n> original`; + + it('adds the signature between the reply and the quote header', () => { + expect(replacePlainTextSignatureAboveQuote(`\n\n${quote}`, header, none, identity, { separator: true })) + .toBe(`\n\n-- \nAlice\nAcme\n\n${quote}`); + expect(replacePlainTextSignatureAboveQuote(`Hi\n\n${quote}`, header, none, identity, { separator: true })) + .toBe(`Hi\n\n-- \nAlice\nAcme\n\n${quote}`); + }); + + it('swaps or removes the signature above the quote', () => { + const signed = `Hi\n\n-- \nAlice\nAcme\n\n${quote}`; + expect(replacePlainTextSignatureAboveQuote(signed, header, identity, other, { separator: true })) + .toBe(`Hi\n\n-- \nTeam\n\n${quote}`); + expect(replacePlainTextSignatureAboveQuote(signed, header, identity, none, { separator: true })) + .toBe(`Hi\n\n${quote}`); + }); + + it('leaves a body alone whose quote header is gone', () => { + expect(replacePlainTextSignatureAboveQuote('Hi\n\n> original', header, none, identity, { separator: true })) + .toBe('Hi\n\n> original'); + }); }); describe('replacePlainTextSignature', () => { diff --git a/components/email/__tests__/composer-reply-signature.test.tsx b/components/email/__tests__/composer-reply-signature.test.tsx new file mode 100644 index 000000000..2cb02c410 --- /dev/null +++ b/components/email/__tests__/composer-reply-signature.test.tsx @@ -0,0 +1,318 @@ +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import React from 'react'; +import { EmailComposer } from '../email-composer'; +import { useSettingsStore } from '@/stores/settings-store'; +import { useIdentityStore } from '@/stores/identity-store'; +import type { Identity } from '@/lib/jmap/types'; + +// ─── Heavy component mocks (mirrors composer-format-toggle.test.tsx) ─ + +// A stand-in for the TipTap editor: it hands the composer an editor object +// whose content is whatever the composer last rendered into it, the way the +// real editor reports itself ready from an effect after mounting. +const editorState = vi.hoisted(() => ({ html: '' })); +vi.mock('@/components/email/rich-text-editor', () => ({ + RichTextEditor: ({ content, onChange, onEditorReady }: { + content: string; + onChange: (html: string) => void; + onEditorReady?: (editor: unknown) => void; + }) => { + editorState.html = content; + const onChangeRef = React.useRef(onChange); + onChangeRef.current = onChange; + React.useEffect(() => { + onEditorReady?.({ + isDestroyed: false, + commands: { + focus: () => {}, + setContent: (html: string) => { + editorState.html = html; + onChangeRef.current(html); + }, + }, + }); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + return React.createElement('div', { 'data-testid': 'rich-text-editor', 'data-html': content }); + }, +})); + +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', htmlSignature: 'Alice', textSignature: 'Alice' }, + { id: 'id-team', email: 'team@example.com', name: 'Team', htmlSignature: '', textSignature: '' }, + ] as Array>, + 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, notifyIdentitySelected: () => {} }; +}); + +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 }; +}); + +vi.mock('@/stores/settings-store', () => { + const state = { + timeFormat: '24h', + plainTextMode: false, + subAddressDelimiter: '+', + autoSelectReplyIdentity: true, + attachmentReminderEnabled: false, + attachmentReminderKeywords: [], + emptySubjectWarningEnabled: true, + sendDelaySeconds: 0, + // The default position: replies/forwards get the signature appended at + // send time rather than embedded above the quote. + signaturePosition: 'below_quote', + signatureSeparatorEnabled: true, + requestReadReceiptDefault: false, + addTrustedSender: () => {}, + externalContentPolicy: 'block', + isSenderTrusted: () => false, + trustedSendersAddressBook: null, + updateSetting: () => {}, + }; + 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', () => ({ + identityHooks: { + onSignatureRender: { size: 0, transform: async (value: unknown) => value }, + onIdentitySelect: { size: 0, emit: async () => {} }, + }, + templateHooks: { + onTemplateRender: { size: 0, transform: async (value: unknown) => value }, + onTemplateApply: { size: 0, emit: async () => {} }, + }, + 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, interceptOutcome: async () => 'allowed' }, + onComposeSend: { takeOver: async () => 'default' }, + 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, + sanitizeSignatureHtmlForDisplay: (v: string) => v, + sanitizeEmailHtml: (v: string) => v, + sanitizePluginBodyHtml: (v: string) => v, + escapeHtml: (v: string) => v, + parseHtmlSafely: (html: string) => new DOMParser().parseFromString(html, 'text/html'), +})); + +vi.mock('@/lib/email-threading', () => ({ + computeReplyThreadingHeaders: () => ({ inReplyTo: [], references: [] }), +})); +vi.mock('@/lib/sub-addressing', () => ({ generateSubAddress: () => '' })); +vi.mock('@/lib/debug', () => ({ debug: { log: () => {}, warn: () => {}, error: () => {} } })); +vi.mock('@/components/email/quoted-html', () => ({ + buildQuotedHtmlBlock: (html: string) => `
${html}
`, + serializeEditorContent: () => editorState.html, +})); +vi.mock('@/lib/template-utils', () => ({ substitutePlaceholders: (s: string) => s })); + +// ─── Tests ──────────────────────────────────────────────────────────────────── + +/** + * #842: replying did not insert the signature. With the signature above the + * quote, a reply's body is built before its From is known - the composer + * picks the identity the mail was sent to in an effect, and the identities + * may load after the composer opened. A plain-text reply never received the + * signature that arrived then, and the send path doesn't append one to an + * above-quote reply, so the mail went out unsigned. An HTML reply got it, but + * between the "On ... wrote:" line and the quote. + */ + +const ME = { id: 'id-me', email: 'me@example.com', name: 'Me', htmlSignature: 'Alice', textSignature: 'Alice' }; +const UNSIGNED_ME = { ...ME, htmlSignature: '', textSignature: '' }; +const SALES = { id: 'id-sales', email: 'sales@example.com', name: 'Sales', htmlSignature: 'Sales team', textSignature: 'Sales team' }; + +const REPLY_TO = { + from: [{ name: 'Bob', email: 'bob@example.com' }], + to: [{ email: 'sales@example.com' }], + subject: 'Question', + body: 'Original text', + htmlBody: '

Original text

', + receivedAt: '2026-08-01T10:00:00Z', +}; +const TO_ME = { ...REPLY_TO, to: [{ email: 'me@example.com' }] }; + +const sendButton = () => screen.getAllByTestId('composer-send')[0] as HTMLButtonElement; + +async function send(onSend: ReturnType) { + fireEvent.click(sendButton()); + await waitFor(() => expect(onSend).toHaveBeenCalledTimes(1)); + return onSend.mock.calls[0][0] as { body: string; htmlBody?: string }; +} + +function setIdentities(list: Array>) { + useIdentityStore.setState({ identities: list as Identity[] }); +} + +describe('reply signature above the quote (#842)', () => { + afterEach(() => { + useSettingsStore.setState({ plainTextMode: false, signaturePosition: 'below_quote' }); + setIdentities([ME]); + vi.clearAllMocks(); + }); + + describe('plain text', () => { + it('signs a reply from an alias the composer picks itself', async () => { + useSettingsStore.setState({ signaturePosition: 'above_quote', plainTextMode: true }); + setIdentities([UNSIGNED_ME, SALES]); + const onSend = vi.fn(); + render(); + + const sent = await send(onSend); + expect(sent.body).toBe('\n\n-- \nSales team\n\nreply_line\n> Original text'); + }); + + it('swaps in the signature of the picked alias instead of the default one', async () => { + useSettingsStore.setState({ signaturePosition: 'above_quote', plainTextMode: true }); + setIdentities([ME, SALES]); + const onSend = vi.fn(); + render(); + + const sent = await send(onSend); + expect(sent.body).toBe('\n\n-- \nSales team\n\nreply_line\n> Original text'); + }); + + it('signs a forward when the identities load after it opened', async () => { + useSettingsStore.setState({ signaturePosition: 'above_quote', plainTextMode: true }); + setIdentities([]); + const onSend = vi.fn(); + const props = { mode: 'forward' as const, replyTo: TO_ME, prefill: { to: ['carol@example.com'] }, onSend }; + const { rerender } = render(); + setIdentities([ME]); + rerender(); + + const sent = await send(onSend); + expect(sent.body.startsWith('\n\n-- \nAlice\n\nforwarded_separator\n')).toBe(true); + }); + }); + + describe('HTML', () => { + it('puts the signature of the picked alias above the "wrote:" line', async () => { + useSettingsStore.setState({ signaturePosition: 'above_quote' }); + setIdentities([UNSIGNED_ME, SALES]); + const onSend = vi.fn(); + render(); + + const sent = await send(onSend); + const html = sent.htmlBody ?? ''; + expect(html).toContain('Sales team'); + expect(html.indexOf('Sales team')).toBeLessThan(html.indexOf('reply_line')); + }); + }); + + it('still signs a reply below the quote at send time by default', async () => { + setIdentities([UNSIGNED_ME, SALES]); + const onSend = vi.fn(); + render(); + + const sent = await send(onSend); + expect(sent.htmlBody).toMatch(/Original text[\s\S]*Sales team<\/i>$/); + expect(sent.body).toMatch(/Original text[\s\S]*Sales team$/); + }); +}); diff --git a/components/email/compose-format.ts b/components/email/compose-format.ts index 4cd9987a0..82e9e9081 100644 --- a/components/email/compose-format.ts +++ b/components/email/compose-format.ts @@ -120,15 +120,25 @@ export function stripEmbeddedSignature(html: string): { html: string; hadSignatu * The HTML body with its embedded signature swapped for `signatureHtml` (from * buildEmbeddedSignatureHtml; empty for an identity without a signature). * The new signature goes where the old one was. A body without one - the - * previous identity had none - gets it before the quote, or at the end when - * there is no quote, which is where getInitialBody would have put it. + * previous identity had none - gets it where getInitialBody would have put + * it: above the quote header of a reply or forward (`quoteHeader`, the + * header's first line, e.g. "On ... wrote:"), else before the quote, else at + * the end. */ -export function replaceEmbeddedSignature(html: string, signatureHtml: string): string { +export function replaceEmbeddedSignature( + html: string, + signatureHtml: string, + options: { quoteHeader?: string } = {}, +): string { const doc = new DOMParser().parseFromString(html, 'text/html'); let removed = removeSignatureRange(doc); if (!removed) { if (!signatureHtml) return html; - const quote = Array.from(doc.body.childNodes).find(isQuoteBoundary) ?? null; + const children = Array.from(doc.body.childNodes); + const header = options.quoteHeader + ? children.find((node) => node.textContent?.includes(options.quoteHeader!)) + : undefined; + const quote = header ?? children.find(isQuoteBoundary) ?? null; removed = { parent: doc.body, insertBefore: quote }; } const host = doc.createElement('div'); @@ -157,6 +167,25 @@ export function replacePlainTextSignature( return appendPlainTextSignature(bare, next, options); } +/** + * replacePlainTextSignature for a plain-text reply or forward with the + * signature above the quote: the signature ends the part before the quote + * header (`quoteHeader`, its first line). A body whose header can't be found + * is returned unchanged. + */ +export function replacePlainTextSignatureAboveQuote( + body: string, + quoteHeader: string, + previous: SignatureIdentityLike, + next: SignatureIdentityLike, + options: { separator: boolean }, +): string { + const at = quoteHeader ? body.indexOf(quoteHeader) : -1; + if (at < 0) return body; + const head = body.slice(0, at).replace(/\n+$/, ''); + return `${replacePlainTextSignature(head, previous, next, options)}\n\n${body.slice(at)}`; +} + /** * Convert the rich-text editor's body to a plain-text body. Formatting is * dropped (that is the point of plain text); an embedded signature comes back diff --git a/components/email/email-composer.tsx b/components/email/email-composer.tsx index 6983aa4b6..434544fd0 100644 --- a/components/email/email-composer.tsx +++ b/components/email/email-composer.tsx @@ -58,6 +58,7 @@ import { plainComposeBodyToHtml, replaceEmbeddedSignature, replacePlainTextSignature, + replacePlainTextSignatureAboveQuote, } from "@/components/email/compose-format"; import { emailHooks, contactHooks, isExternalAttachmentResult } from "@/lib/plugin-hooks"; import { onUploadProgress } from "@/lib/upload-progress"; @@ -554,6 +555,29 @@ export function EmailComposer({ return ""; }; + // The first line of the quote header getInitialBody writes under the reply + // area ("On ... wrote:", the forwarded-message line, or a plugin's header). + // A signature that arrives after the body was built - From resolves to a + // matching identity, or the identities load late - goes above it. + const getQuoteHeaderLine = (): string => { + if (mode !== 'reply' && mode !== 'replyAll' && mode !== 'forward') return ''; + if (!replyTo?.body && !replyTo?.htmlBody) return ''; + const pluginHeader = replyTo.quoteHeaderText + ?? (replyTo.quoteHeaderHtml !== undefined ? htmlToPlainText(replyTo.quoteHeaderHtml) : undefined); + if (pluginHeader !== undefined) { + return pluginHeader.split('\n').map((line) => line.trim()).find(Boolean) ?? ''; + } + if (mode === 'forward') return tQuote('forwarded_separator'); + const date = replyTo.receivedAt ? formatDateTime(replyTo.receivedAt, timeFormat, { weekday: 'short', year: 'numeric', month: 'short', day: 'numeric' }) : ""; + const from = replyTo.from?.[0]; + const fromStrFull = from + ? (from.name && from.email && from.name !== from.email + ? `${from.name} <${from.email}>` + : (from.email || from.name || tCommon('unknown'))) + : tCommon('unknown'); + return tQuote('reply_line', { date, from: fromStrFull }); + }; + const getInitialBody = () => { if (plainTextMode) { // Plain text mode: produce plain text body with no HTML @@ -1062,11 +1086,20 @@ export function EmailComposer({ if (plainTextMode) { // A new plain-text message carries the signature at the end of the - // body; in a reply the quote follows it. - if (!identityChanged || mode !== 'compose') return; - setBody((prev) => replacePlainTextSignature(prev, previousIdentity, signatureIdentity, { - separator: signatureSeparatorEnabled, - })); + // body, a reply or forward above its quote header. + if (!identityChanged) return; + const options = { separator: signatureSeparatorEnabled }; + if (mode === 'compose') { + setBody((prev) => replacePlainTextSignature(prev, previousIdentity, signatureIdentity, options)); + return; + } + // The signature identity of a reply is often only known after the body + // was built: the reply's From resolves to the alias it was sent to, or + // the identities load late. Swap it above the quote header like an + // HTML body does; leaving it out lost it, since an above-quote reply + // is never signed at send time (#842). + const quoteHeader = getQuoteHeaderLine(); + setBody((prev) => replacePlainTextSignatureAboveQuote(prev, quoteHeader, previousIdentity, signatureIdentity, options)); return; } if (!editor) return; @@ -1082,7 +1115,7 @@ export function EmailComposer({ const nextHtml = replaceEmbeddedSignature(currentHtml, buildEmbeddedSignatureHtml(signatureIdentity, { embed: true, separator: signatureSeparatorEnabled, - })); + }), { quoteHeader: getQuoteHeaderLine() }); if (nextHtml !== currentHtml) { editor.commands.setContent(nextHtml, { emitUpdate: true }); }