From 8ea1eef2ff91f75dd447a9f47b574d3c83c6dbdc Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Wed, 14 Oct 2026 23:49:34 +0200 Subject: [PATCH] fix: swap the signature when you pick a From address without one --- .../email/__tests__/compose-format.test.ts | 51 ++++ .../composer-identity-signature.test.tsx | 259 ++++++++++++++++++ components/email/compose-format.ts | 41 +++ components/email/email-composer.tsx | 53 ++-- 4 files changed, 382 insertions(+), 22 deletions(-) create mode 100644 components/email/__tests__/composer-identity-signature.test.tsx diff --git a/components/email/__tests__/compose-format.test.ts b/components/email/__tests__/compose-format.test.ts index b53a558c4..322c4e771 100644 --- a/components/email/__tests__/compose-format.test.ts +++ b/components/email/__tests__/compose-format.test.ts @@ -3,6 +3,8 @@ import { buildEmbeddedSignatureHtml, htmlComposeBodyToPlainText, plainComposeBodyToHtml, + replaceEmbeddedSignature, + replacePlainTextSignature, stripEmbeddedSignature, } from '../compose-format'; import { SIGNATURE_BLOCK_MARKER, SIGNATURE_RANGE_MARKER, containsEmbeddedSignature } from '../signature-block'; @@ -90,3 +92,52 @@ describe('compose-format (#1022): HTML <-> plain text switch', () => { expect(count(html, '-- ')).toBe(1); }); }); + +describe('switching identity swaps the signature', () => { + const other = { htmlSignature: 'Team', textSignature: 'Team' }; + const none = { htmlSignature: '', textSignature: '' }; + const embed = (sig: typeof identity) => buildEmbeddedSignatureHtml(sig, { embed: true, separator: true }); + // The DOM writes valueless attributes as =""; compare in its spelling. + const dom = (html: string) => new DOMParser().parseFromString(html, 'text/html').body.innerHTML; + + describe('replaceEmbeddedSignature', () => { + it('puts the new signature where the old one was', () => { + const html = `

Hi

${embed(identity)}
original
`; + expect(replaceEmbeddedSignature(html, embed(other))) + .toBe(dom(`

Hi

${embed(other)}
original
`)); + }); + + it('removes the signature for an identity without one', () => { + const html = `

Hi

${embed(identity)}
original
`; + expect(replaceEmbeddedSignature(html, embed(none))).toBe('

Hi

original
'); + }); + + it('adds a signature above the quote when the body had none', () => { + expect(replaceEmbeddedSignature('

Hi

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

Hi

${embed(other)}
original
`)); + }); + + it('adds a signature at the end when there is no quote', () => { + expect(replaceEmbeddedSignature('

Hi

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

Hi

${embed(other)}`)); + }); + }); + + describe('replacePlainTextSignature', () => { + const signed = 'Hi\n\n-- \nAlice\nAcme'; + + it('swaps a trailing signature', () => { + expect(replacePlainTextSignature(signed, identity, other, { separator: true })).toBe('Hi\n\n-- \nTeam'); + }); + + it('removes it for an identity without one and adds it back', () => { + const bare = replacePlainTextSignature(signed, identity, none, { separator: true }); + expect(bare).toBe('Hi'); + expect(replacePlainTextSignature(bare, none, identity, { separator: true })).toBe(signed); + }); + + it('leaves a body alone whose signature the user edited', () => { + const edited = 'Hi\n\n-- \nAlice (edited)'; + expect(replacePlainTextSignature(edited, identity, other, { separator: true })).toBe(edited); + }); + }); +}); diff --git a/components/email/__tests__/composer-identity-signature.test.tsx b/components/email/__tests__/composer-identity-signature.test.tsx new file mode 100644 index 000000000..84a6c3a98 --- /dev/null +++ b/components/email/__tests__/composer-identity-signature.test.tsx @@ -0,0 +1,259 @@ +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'; + +// ─── Heavy component mocks (mirrors composer-format-toggle.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', 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: () => {}, + 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: () => '', + serializeEditorContent: () => '', +})); +vi.mock('@/lib/template-utils', () => ({ substitutePlaceholders: (s: string) => s })); + +// ─── Tests ──────────────────────────────────────────────────────────────────── + +/** + * Switching From to an identity without a signature kept the previous + * identity's signature, so a mail from a team address went out with the + * personal signature. An identity the composer picks itself (a reply to an + * alias) still borrows the primary signature. + */ + +const DRAFT = { + to: 'bob@example.com', + cc: '', + bcc: '', + subject: 'Hello', + body: '

Draft text

', + showCc: false, + showBcc: false, + selectedIdentityId: 'id-me', + subAddressTag: '', + mode: 'compose' as const, + draftId: null, +}; + +const fromSelect = () => screen.getByTestId('composer-from') as HTMLSelectElement; +const sendButton = () => screen.getAllByTestId('composer-send')[0] as HTMLButtonElement; +const textarea = () => document.querySelector('textarea') as HTMLTextAreaElement; + +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 }; +} + +describe('composer signature follows the picked identity', () => { + afterEach(() => { + useSettingsStore.setState({ plainTextMode: false }); + vi.clearAllMocks(); + }); + + it('sends a reply from a hand-picked identity without the previous signature', async () => { + const onSend = vi.fn(); + render(); + fireEvent.change(fromSelect(), { target: { value: 'id-team' } }); + + const sent = await send(onSend); + expect(sent.htmlBody).not.toContain('Alice'); + expect(sent.body).not.toContain('Alice'); + }); + + it('still borrows the primary signature for an identity it did not let the user pick', async () => { + const onSend = vi.fn(); + render(); + + const sent = await send(onSend); + expect(sent.htmlBody).toContain('Alice'); + }); + + it('swaps the signature in a plain-text message and brings it back', async () => { + const SIGNED = 'Plain draft\n\n-- \nAlice'; + useSettingsStore.setState({ plainTextMode: true }); + const onSend = vi.fn(); + render(); + + fireEvent.change(fromSelect(), { target: { value: 'id-team' } }); + await waitFor(() => expect(textarea().value).toBe('Plain draft')); + + fireEvent.change(fromSelect(), { target: { value: 'id-me' } }); + await waitFor(() => expect(textarea().value).toBe(SIGNED)); + + fireEvent.change(fromSelect(), { target: { value: 'id-team' } }); + await waitFor(() => expect(textarea().value).toBe('Plain draft')); + const sent = await send(onSend); + expect(sent.body).toBe('Plain draft'); + }); +}); diff --git a/components/email/compose-format.ts b/components/email/compose-format.ts index 0cc0a9f98..7a0190dad 100644 --- a/components/email/compose-format.ts +++ b/components/email/compose-format.ts @@ -107,6 +107,47 @@ export function stripEmbeddedSignature(html: string): { html: string; hadSignatu return { html: doc.body.innerHTML, hadSignature: true }; } +/** + * 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. + */ +export function replaceEmbeddedSignature(html: string, signatureHtml: 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; + removed = { parent: doc.body, insertBefore: quote }; + } + const host = doc.createElement('div'); + host.innerHTML = signatureHtml; + Array.from(host.childNodes).forEach((node) => + removed.parent.insertBefore(node, removed.insertBefore) + ); + return doc.body.innerHTML; +} + +/** + * The plain-text body with the previous identity's trailing signature swapped + * for the next one's. A body that doesn't end with the previous signature was + * edited there (or never had one) and is only given the new signature when + * the previous identity had none. + */ +export function replacePlainTextSignature( + body: string, + previous: SignatureIdentityLike, + next: SignatureIdentityLike, + options: { separator: boolean }, +): string { + const hadSignature = plainTextBodyHasSignature(body, previous); + if (!hadSignature && (previous?.htmlSignature || previous?.textSignature)) return body; + const bare = hadSignature ? plainTextBodyWithoutSignature(body, previous) : body; + return appendPlainTextSignature(bare, next, options); +} + /** * 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 5cff52511..e1a89e293 100644 --- a/components/email/email-composer.tsx +++ b/components/email/email-composer.tsx @@ -55,7 +55,8 @@ import { buildEmbeddedSignatureHtml, htmlComposeBodyToPlainText, plainComposeBodyToHtml, - removeSignatureRange, + replaceEmbeddedSignature, + replacePlainTextSignature, } from "@/components/email/compose-format"; import { emailHooks, contactHooks, isExternalAttachmentResult } from "@/lib/plugin-hooks"; import { onUploadProgress } from "@/lib/upload-progress"; @@ -794,6 +795,9 @@ export function EmailComposer({ const [validationErrors, setValidationErrors] = useState<{ to?: boolean; body?: boolean }>({}); const [shakeField, setShakeField] = useState(null); const [selectedIdentityId, setSelectedIdentityId] = useState(initialData?.selectedIdentityId ?? null); + // Set once the user picks From by hand. Their pick then brings its own + // signature, also when it has none (see ownSignatureIdentity). + const [identityPickedByUser, setIdentityPickedByUser] = useState(false); const [subAddressTag, setSubAddressTag] = useState(initialData?.subAddressTag ?? ''); const [fromOverrideEnabled, setFromOverrideEnabled] = useState(initialData?.fromOverrideEnabled ?? false); const [fromOverrideEmail, setFromOverrideEmail] = useState(initialData?.fromOverrideEmail ?? ''); @@ -944,8 +948,10 @@ export function EmailComposer({ : null; // Alias identities often lack a configured signature - fall back to the primary // identity's signature so replies (which auto-select a matching alias) still - // populate the user's signature. - const ownSignatureIdentity = (currentIdentity?.htmlSignature || currentIdentity?.textSignature) + // populate the user's signature. An identity the user picked by hand keeps + // its own, also an empty one: switching to a team address without a + // signature sent the team mail with the personal signature. + const ownSignatureIdentity = identityPickedByUser || currentIdentity?.htmlSignature || currentIdentity?.textSignature ? currentIdentity : primaryIdentity; // Plugins may rewrite the signature (onSignatureRender). @@ -961,46 +967,48 @@ export function EmailComposer({ const prevSignatureIdentityIdRef = useRef(signatureIdentity?.id); const prevSignatureSeparatorRef = useRef(signatureSeparatorEnabled); const prevPluginSignatureRevisionRef = useRef(pluginSignatureRevision); + const prevSignatureIdentityRef = useRef(signatureIdentity); useEffect(() => { const editor = editorRef.current; const identityChanged = prevSignatureIdentityIdRef.current !== signatureIdentity?.id; const separatorChanged = prevSignatureSeparatorRef.current !== signatureSeparatorEnabled; const pluginSignatureChanged = prevPluginSignatureRevisionRef.current !== pluginSignatureRevision; + const previousIdentity = prevSignatureIdentityRef.current; prevSignatureIdentityIdRef.current = signatureIdentity?.id; prevSignatureSeparatorRef.current = signatureSeparatorEnabled; prevPluginSignatureRevisionRef.current = pluginSignatureRevision; - if (!editor) return; + prevSignatureIdentityRef.current = signatureIdentity; if (!identityChanged && !separatorChanged && !pluginSignatureChanged) return; - if (plainTextMode) return; // Replies/forwards only embed when configured for "above quote". Compose // always embeds (see getInitialBody), so swap on identity change there too. const isReplyLike = mode === 'reply' || mode === 'replyAll' || mode === 'forward'; if (!isReplyLike && mode !== 'compose') return; if (isReplyLike && signaturePosition !== 'above_quote') return; + 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, + })); + return; + } + if (!editor) return; + // serializeEditorContent (not getHTML) so a QuotedHtml island's verbatim // body isn't lost during the signature splice + setContent round-trip. + // The old range is replaced in place (bounded so it never eats into the + // quoted body - see removeSignatureRange), or removed when the new + // identity has no signature. const currentHtml = serializeEditorContent(editor); - const doc = new DOMParser().parseFromString(currentHtml, 'text/html'); - const newSignature = buildEmbeddedSignatureHtml(signatureIdentity, { + // Only a new identity brings a signature back into a body without one. + if (!identityChanged && !containsEmbeddedSignature(currentHtml)) return; + const nextHtml = replaceEmbeddedSignature(currentHtml, buildEmbeddedSignatureHtml(signatureIdentity, { embed: true, separator: signatureSeparatorEnabled, - }); - if (!newSignature) return; - - // Remove the existing signature range (bounded so it never eats into the - // quoted body - see removeSignatureRange) and splice the new one in at the - // same position, without re-serializing/parsing twice. - const removed = removeSignatureRange(doc); - if (!removed) return; - const replacementHost = doc.createElement('div'); - replacementHost.innerHTML = newSignature; - Array.from(replacementHost.childNodes).forEach((node) => - removed.parent.insertBefore(node, removed.insertBefore) - ); - - const nextHtml = doc.body.innerHTML; + })); if (nextHtml !== currentHtml) { editor.commands.setContent(nextHtml, { emitUpdate: true }); } @@ -3543,6 +3551,7 @@ export function EmailComposer({ value={selectedIdentityId || primaryIdentity?.id || ''} onChange={(e) => { setSelectedIdentityId(e.target.value); + setIdentityPickedByUser(true); notifyIdentitySelected(identities, e.target.value); }} className="flex-1 bg-transparent text-sm text-foreground outline-none cursor-pointer hover:text-muted-foreground transition-colors min-w-0 truncate"