From 996aa66ef0528b8d21a8a74536e19bb5f9205d38 Mon Sep 17 00:00:00 2001 From: jcoffey <51408202+jcoffey-dev@users.noreply.github.com> Date: Sat, 19 Sep 2026 14:43:38 -0700 Subject: [PATCH 1/7] Offer the message's own format when replying (#407) (#408) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A reply opened in the format the settings ask for, whatever the message being answered was written in, and the per-draft switch was buried in the composer's ⋮ menu. Replying in plain text to a rich text message throws away the formatting; replying in rich text to a plain-text one overrides what the sender chose to write in. When the two disagree the composer now says so above the editor -- "This message is rich text", with a Switch button and a dismiss -- and the draft still opens in the format the settings ask for. Switching converts that draft only and leaves the setting alone; switching from the ⋮ menu answers the offer too. Forwards get it as well, where the formatting being passed on is somebody else's. What counts as rich text is hasHtmlAlternative(), which reads the body part's own type: `htmlBody` is derived (RFC 8621 4.1.4), so a plain-text message has one too and its presence proves nothing. The mock said otherwise -- it returned an empty `htmlBody` for a plain-text message, where Stalwart 0.16.21 returns the text/plain part in both lists. Both builders now answer as the server does, so the path this feature depends on is exercised in development rather than only against a real mailbox. Two new strings, translated in all nine catalogs; the buttons reuse the menu's existing "Switch to plain text" / "Switch to rich text". The count falling back to English stays at 16 in every language. Fixes #407 (cherry picked from commit d992442b8194be5e9c48204332c7243d9587b4ca) --- server/src/mock/data.ts | 9 +- web/src/locales/de.ts | 2 + web/src/locales/es.ts | 2 + web/src/locales/fr.ts | 2 + web/src/locales/ja.ts | 2 + web/src/locales/nl.ts | 2 + web/src/locales/pt-BR.ts | 2 + web/src/locales/ru.ts | 2 + web/src/locales/uk.ts | 2 + web/src/locales/zh-Hans.ts | 2 + web/src/store/__tests__/compose-email.test.ts | 2 +- .../__tests__/reply-format-offer.test.ts | 106 ++++++++++++++++++ web/src/store/compose.ts | 17 ++- web/src/styles/app.css | 3 + web/src/views/compose/Composer.tsx | 21 +++- .../__tests__/format-offer-bar.test.tsx | 63 +++++++++++ 16 files changed, 233 insertions(+), 6 deletions(-) create mode 100644 web/src/store/__tests__/reply-format-offer.test.ts create mode 100644 web/src/views/compose/__tests__/format-offer-bar.test.tsx diff --git a/server/src/mock/data.ts b/server/src/mock/data.ts index a9bd8b6..162210a 100644 --- a/server/src/mock/data.ts +++ b/server/src/mock/data.ts @@ -122,7 +122,11 @@ export function addSignedEmail(o: { which: keyof typeof SIGNED_MESSAGES; from: [ hasAttachment: false, preview: body.slice(0, 120), textBody: [{ partId: "1", blobId: textBlob, size: body.length, name: null, type: "text/plain", charset: "utf-8", disposition: null, cid: null }], - htmlBody: [], + // `htmlBody` is derived (RFC 8621 4.1.4): a message with no HTML + // alternative still gets one, holding the text/plain part. Checked against + // Stalwart 0.16.21 on 2026-09-10 -- see hasHtmlAlternative() in the client, + // which reads the part's type rather than trusting this list to be empty. + htmlBody: [{ partId: "1", blobId: textBlob, size: body.length, name: null, type: "text/plain", charset: "utf-8", disposition: null, cid: null }], attachments: [], bodyValues: { "1": { value: body, isEncodingProblem: false, isTruncated: false } }, bodyStructure: { @@ -192,7 +196,8 @@ export function addEmail(o: { from: [string, string]; to?: string; subject: stri from: [{ name: o.from[0], email: o.from[1] }], to: [{ name: "Demo User", email: o.to ?? USER }], cc: null, bcc: null, replyTo: null, sender: null, subject: o.subject, hasAttachment: Boolean(o.attach), preview: text.slice(0, 120).replace(/\n/g, " "), textBody: [{ partId: "1", blobId: textBlob, size: text.length, name: null, type: "text/plain", charset: "utf-8", disposition: null, cid: null }], - htmlBody: o.html ? [{ partId: "2", blobId: htmlBlob, size: (o.styled ? STYLED_MARKETING_HTML : html).length, name: null, type: "text/html", charset: "utf-8", disposition: null, cid: null }] : [], + // No HTML alternative means `htmlBody` names the text part, not nothing. See addSignedEmail. + htmlBody: o.html ? [{ partId: "2", blobId: htmlBlob, size: (o.styled ? STYLED_MARKETING_HTML : html).length, name: null, type: "text/html", charset: "utf-8", disposition: null, cid: null }] : [{ partId: "1", blobId: textBlob, size: text.length, name: null, type: "text/plain", charset: "utf-8", disposition: null, cid: null }], attachments, bodyValues: { "1": { value: text, isEncodingProblem: false, isTruncated: false }, ...(o.html ? { "2": { value: o.styled ? STYLED_MARKETING_HTML : html, isEncodingProblem: false, isTruncated: false } } : {}) }, bodyStructure: { partId: null, blobId: null, size: 0, type: "multipart/mixed", name: null, charset: null, disposition: null, cid: null, subParts: [{ partId: "1", blobId: textBlob, size: text.length, type: "text/plain", name: null, charset: "utf-8", disposition: null, cid: null }, ...(o.html ? [{ partId: "2", blobId: htmlBlob, size: (o.styled ? STYLED_MARKETING_HTML : html).length, type: "text/html", name: null, charset: "utf-8", disposition: null, cid: null }] : []), ...attachments] }, diff --git a/web/src/locales/de.ts b/web/src/locales/de.ts index db9fd98..3ada0e7 100644 --- a/web/src/locales/de.ts +++ b/web/src/locales/de.ts @@ -1406,6 +1406,8 @@ export const catalog: Catalog = { "Collapse all": "Alle einklappen", "Expand all": "Alle ausklappen", "Send now instead": "Stattdessen jetzt senden", + "This message is rich text": "Diese Nachricht ist formatierter Text", + "This message is plain text": "Diese Nachricht ist Nur-Text", "Switch to plain text": "Zu Nur-Text wechseln", "Switch to rich text": "Zu formatiertem Text wechseln", "{used} of {total} used": "{used} von {total} belegt", diff --git a/web/src/locales/es.ts b/web/src/locales/es.ts index 42b05f6..19b32ce 100644 --- a/web/src/locales/es.ts +++ b/web/src/locales/es.ts @@ -1379,6 +1379,8 @@ export const catalog: Catalog = { "Collapse all": "Contraer todo", "Expand all": "Expandir todo", "Send now instead": "Enviar ahora, sin programar", + "This message is rich text": "Este mensaje es texto enriquecido", + "This message is plain text": "Este mensaje es texto sin formato", "Switch to plain text": "Cambiar a texto sin formato", "Switch to rich text": "Cambiar a texto enriquecido", "{used} of {total} used": "{used} de {total} usados", diff --git a/web/src/locales/fr.ts b/web/src/locales/fr.ts index b8f8b13..3a37a94 100644 --- a/web/src/locales/fr.ts +++ b/web/src/locales/fr.ts @@ -1384,6 +1384,8 @@ export const catalog: Catalog = { "Collapse all": "Tout réduire", "Expand all": "Tout développer", "Send now instead": "Envoyer tout de suite", + "This message is rich text": "Ce message est en texte enrichi", + "This message is plain text": "Ce message est en texte brut", "Switch to plain text": "Passer en texte brut", "Switch to rich text": "Passer en texte enrichi", "{used} of {total} used": "{used} sur {total} utilisés", diff --git a/web/src/locales/ja.ts b/web/src/locales/ja.ts index 1c59378..517af6d 100644 --- a/web/src/locales/ja.ts +++ b/web/src/locales/ja.ts @@ -1387,6 +1387,8 @@ export const catalog: Catalog = { "Collapse all": "すべて折りたたむ", "Expand all": "すべて展開", "Send now instead": "予約をやめて今すぐ送信", + "This message is rich text": "このメールはリッチテキストです", + "This message is plain text": "このメールはプレーンテキストです", "Switch to plain text": "プレーンテキストに切り替え", "Switch to rich text": "リッチテキストに切り替え", "{used} of {total} used": "{total} 中 {used} を使用", diff --git a/web/src/locales/nl.ts b/web/src/locales/nl.ts index f619711..789f5b4 100644 --- a/web/src/locales/nl.ts +++ b/web/src/locales/nl.ts @@ -1379,6 +1379,8 @@ export const catalog: Catalog = { "Collapse all": "Alles samenvouwen", "Expand all": "Alles uitvouwen", "Send now instead": "Toch nu verzenden", + "This message is rich text": "Dit bericht is opgemaakte tekst", + "This message is plain text": "Dit bericht is platte tekst", "Switch to plain text": "Overschakelen naar platte tekst", "Switch to rich text": "Overschakelen naar opgemaakte tekst", "{used} of {total} used": "{used} van {total} gebruikt", diff --git a/web/src/locales/pt-BR.ts b/web/src/locales/pt-BR.ts index d9de982..c396e48 100644 --- a/web/src/locales/pt-BR.ts +++ b/web/src/locales/pt-BR.ts @@ -1382,6 +1382,8 @@ export const catalog: Catalog = { "Collapse all": "Recolher tudo", "Expand all": "Expandir tudo", "Send now instead": "Enviar agora mesmo", + "This message is rich text": "Esta mensagem está em texto formatado", + "This message is plain text": "Esta mensagem está em texto simples", "Switch to plain text": "Mudar para texto simples", "Switch to rich text": "Mudar para texto formatado", "{used} of {total} used": "{used} de {total} usados", diff --git a/web/src/locales/ru.ts b/web/src/locales/ru.ts index f645f1b..f4bbc00 100644 --- a/web/src/locales/ru.ts +++ b/web/src/locales/ru.ts @@ -1381,6 +1381,8 @@ export const catalog: Catalog = { "Collapse all": "Свернуть все", "Expand all": "Развернуть все", "Send now instead": "Отправить сейчас", + "This message is rich text": "Это письмо в формате HTML", + "This message is plain text": "Это письмо в виде простого текста", "Switch to plain text": "Переключиться на обычный текст", "Switch to rich text": "Переключиться на форматированный текст", "{used} of {total} used": "Использовано {used} из {total}", diff --git a/web/src/locales/uk.ts b/web/src/locales/uk.ts index 4b335c3..e1b615c 100644 --- a/web/src/locales/uk.ts +++ b/web/src/locales/uk.ts @@ -1375,6 +1375,8 @@ export const catalog: Catalog = { "Collapse all": "Згорнути все", "Expand all": "Розгорнути все", "Send now instead": "Надіслати зараз", + "This message is rich text": "Цей лист у форматі HTML", + "This message is plain text": "Цей лист у вигляді простого тексту", "Switch to plain text": "Перейти на звичайний текст", "Switch to rich text": "Перейти на форматований текст", "{used} of {total} used": "Використано {used} з {total}", diff --git a/web/src/locales/zh-Hans.ts b/web/src/locales/zh-Hans.ts index 628971c..85aada8 100644 --- a/web/src/locales/zh-Hans.ts +++ b/web/src/locales/zh-Hans.ts @@ -1386,6 +1386,8 @@ export const catalog: Catalog = { "Collapse all": "全部折叠", "Expand all": "全部展开", "Send now instead": "改为立即发送", + "This message is rich text": "这封邮件是富文本", + "This message is plain text": "这封邮件是纯文本", "Switch to plain text": "切换为纯文本", "Switch to rich text": "切换为富文本", "{used} of {total} used": "已使用 {used}，共 {total}", diff --git a/web/src/store/__tests__/compose-email.test.ts b/web/src/store/__tests__/compose-email.test.ts index 44a46fa..395ae97 100644 --- a/web/src/store/__tests__/compose-email.test.ts +++ b/web/src/store/__tests__/compose-email.test.ts @@ -18,7 +18,7 @@ function draft(over: Partial = {}): Draft { requestReceipt: false, priority: "normal", showCc: false, showBcc: false, showReplyTo: false, minimized: false, maximized: false, dirty: false, savedAt: null, - saving: false, sending: false, error: null, signatureHtml: "", replyMode: null, sendAt: null, + saving: false, sending: false, error: null, signatureHtml: "", replyMode: null, formatOffer: null, sendAt: null, ...over, }; } diff --git a/web/src/store/__tests__/reply-format-offer.test.ts b/web/src/store/__tests__/reply-format-offer.test.ts new file mode 100644 index 0000000..6f8847c --- /dev/null +++ b/web/src/store/__tests__/reply-format-offer.test.ts @@ -0,0 +1,106 @@ +import { beforeEach, describe, expect, it } from "vitest"; +import { useCompose } from "@/store/compose"; +import { useMail } from "@/store/mail"; +import { DEFAULT_SETTINGS, useSettings } from "@/store/settings"; +import type { Email, Identity } from "@/jmap/types"; + +/* + * Offering to answer a message in the format it was written in (#407). + * + * The trap is `htmlBody`: RFC 8621 derives it, so a plain-text message has one + * too, holding its text/plain part. Reading that as "there is HTML" would + * offer a switch to rich text on every plain-text message, and never offer the + * switch to plain text where it is actually wanted. + */ + +const base = { + messageId: [""], subject: "Numbers", references: [], inReplyTo: [], + keywords: {}, attachments: [], receivedAt: "2026-09-04T10:00:00Z", mailboxIds: {}, + from: [{ name: "Ann", email: "ann@example.com" }], to: [{ name: "John", email: "john@example.org" }], cc: [], +}; + +/** A real multipart/alternative: two parts, one of them text/html. */ +const RICH = { + ...base, id: "m1", + htmlBody: [{ partId: "2", type: "text/html" }], + textBody: [{ partId: "1", type: "text/plain" }], + bodyValues: { "1": { value: "hi", isEncodingProblem: false, isTruncated: false }, "2": { value: "

hi

", isEncodingProblem: false, isTruncated: false } }, +} as unknown as Email; + +/** Plain text, as Stalwart returns it: both lists name the same text/plain part. */ +const PLAIN = { + ...base, id: "m2", + htmlBody: [{ partId: "1", type: "text/plain" }], + textBody: [{ partId: "1", type: "text/plain" }], + bodyValues: { "1": { value: "hi", isEncodingProblem: false, isTruncated: false } }, +} as unknown as Email; + +const IDENTITIES = [{ id: "i1", name: "John", email: "john@example.org", replyTo: null }] as unknown as Identity[]; + +function draftFor(email: Email, mode: "reply" | "replyAll" | "forward") { + useMail.setState({ + accountId: "a1", + identities: IDENTITIES as never, + getEmails: (async () => [email]) as never, + defaultIdentity: (() => IDENTITIES[0]) as never, + loadIdentities: (async () => IDENTITIES) as never, + roleId: (() => null) as never, + }); + return useCompose.getState().reply(email, mode).then((key) => useCompose.getState().drafts.find((d) => d.key === key)!); +} + +const composeIn = (format: "html" | "text") => useSettings.setState({ settings: { ...DEFAULT_SETTINGS, composeFormat: format } }); + +beforeEach(() => { + useCompose.setState({ drafts: [], activeKey: null }); + useSettings.setState({ settings: { ...DEFAULT_SETTINGS } }); +}); + +describe("answering a message written in the other format", () => { + it("offers rich text when a plain-text reply answers a rich message", async () => { + composeIn("text"); + const d = await draftFor(RICH, "reply"); + expect(d.format).toBe("text"); + expect(d.formatOffer).toBe("html"); + }); + + it("offers plain text when a rich reply answers a plain-text message", async () => { + composeIn("html"); + const d = await draftFor(PLAIN, "reply"); + expect(d.format).toBe("html"); + expect(d.formatOffer).toBe("text"); + }); + + it("offers nothing when the formats already agree", async () => { + composeIn("html"); + expect((await draftFor(RICH, "reply")).formatOffer).toBeNull(); + composeIn("text"); + expect((await draftFor(PLAIN, "reply")).formatOffer).toBeNull(); + }); + + it("reads the part's own type, not the derived htmlBody list", async () => { + // PLAIN has an htmlBody; it names the text/plain part. Offering a switch + // to rich text here would fire on every plain-text message there is. + composeIn("text"); + expect((await draftFor(PLAIN, "reply")).formatOffer).toBeNull(); + }); + + it("offers on a reply all and on a forward, where the same formatting is lost", async () => { + composeIn("text"); + expect((await draftFor(RICH, "replyAll")).formatOffer).toBe("html"); + expect((await draftFor(RICH, "forward")).formatOffer).toBe("html"); + }); + + it("carries both bodies either way, so switching has something to switch to", async () => { + composeIn("text"); + const d = await draftFor(RICH, "reply"); + expect(d.text).toContain("hi"); + expect(d.html).toContain("hi"); + }); + + it("makes no offer on a message started from scratch", () => { + composeIn("text"); + const key = useCompose.getState().open(); + expect(useCompose.getState().drafts.find((x) => x.key === key)!.formatOffer).toBeNull(); + }); +}); diff --git a/web/src/store/compose.ts b/web/src/store/compose.ts index 91a5a8b..0a02669 100644 --- a/web/src/store/compose.ts +++ b/web/src/store/compose.ts @@ -4,7 +4,7 @@ import type { Email, EmailAddress, EmailBodyPart, Id, Identity, SetResponse } fr import { formatFullDate, uid } from "@/lib/format"; import { formatAddress, parseMailto, sameAddress, uniqueAddresses } from "@/lib/address"; import { escapeHtml, htmlToText, quoteText, replySubject, textToHtml } from "@/lib/text/text"; -import { sanitizeEmailHtml, sanitizeEditorHtml } from "@/lib/text/html"; +import { hasHtmlAlternative, sanitizeEmailHtml, sanitizeEditorHtml } from "@/lib/text/html"; import { toast } from "@/ui/toast"; import { useMail, FULL_PROPS, BODY_PROPS } from "./mail"; import { useSession } from "./session"; @@ -76,6 +76,12 @@ export interface Draft { /** Original identity signature HTML currently embedded, to replace on identity switch. */ signatureHtml: string; replyMode: "reply" | "replyAll" | "forward" | null; + /** + * The format the message being answered was written in, when it is not the + * one this draft opened in (#407). The composer offers the switch; answering + * it either way, or dismissing it, clears this. + */ + formatOffer: "html" | "text" | null; mailboxIdOnSend?: Id | null; /** When set, hand the message to the server held until this instant. */ sendAt: number | null; @@ -146,6 +152,7 @@ function blankDraft(init: Partial = {}): Draft { error: null, signatureHtml: "", replyMode: null, + formatOffer: null, sendAt: null, ...init, }; @@ -405,6 +412,13 @@ export const useCompose = create((set, get) => ({ const textPart = full.textBody?.[0]; const origHtml = htmlPart?.partId ? (full.bodyValues?.[htmlPart.partId]?.value ?? "") : ""; const origText = textPart?.partId ? (full.bodyValues?.[textPart.partId]?.value ?? "") : ""; + /* + * What the message being answered was really written in. `htmlBody` is + * derived, so its presence proves nothing -- hasHtmlAlternative() reads the + * part's own type. Getting this wrong would offer every plain-text message + * a switch to rich text it does not need. + */ + const origFormat = hasHtmlAlternative(htmlPart, origHtml) ? "html" : "text"; const accountId = mail.accountId!; const attachments: ComposeAttachment[] = []; const cidMap: Record = {}; @@ -460,6 +474,7 @@ export const useCompose = create((set, get) => ({ relatedKeyword: mode === "forward" ? "$forwarded" : "$answered", signatureHtml: sigHtml, replyMode: mode, + formatOffer: origFormat === s.composeFormat ? null : origFormat, }); set((st) => ({ drafts: [...st.drafts, d], activeKey: d.key })); return d.key; diff --git a/web/src/styles/app.css b/web/src/styles/app.css index 8681f41..55a5eef 100644 --- a/web/src/styles/app.css +++ b/web/src/styles/app.css @@ -1584,6 +1584,9 @@ a.menu-item:hover { color: var(--fg); } .composer-head .title { flex: 1; font-weight: 600; white-space: nowrap; overflow: hidden; text-overflow: ellipsis; } .composer-head .status { color: var(--fg-faint); font-size: .8em; margin-right: 6px; white-space: nowrap; } .composer-body { display: flex; flex-direction: column; flex: 1; min-height: 0; } +/* An offer the draft makes about itself, above the editor: quiet, one line, and dismissible. */ +.composer-notice { display: flex; align-items: center; gap: 8px; padding: 6px 10px 6px 14px; background: var(--accent-soft); color: var(--accent-soft-fg); border-bottom: 1px solid var(--border); font-size: .85em; flex: 0 0 auto; } +.composer-notice span { flex: 1; min-width: 0; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } .composer-fields { flex: 0 0 auto; padding: 0 12px; } .composer-field { display: flex; align-items: center; gap: 8px; min-height: 40px; border-bottom: 1px solid var(--border); padding: 4px 0; } .composer-field > label { color: var(--fg-muted); width: 42px; flex: 0 0 auto; font-size: .92em; } diff --git a/web/src/views/compose/Composer.tsx b/web/src/views/compose/Composer.tsx index f81b5e4..644a032 100644 --- a/web/src/views/compose/Composer.tsx +++ b/web/src/views/compose/Composer.tsx @@ -148,10 +148,11 @@ export function Composer({ draft }: { draft: Draft }) { }; const toggleFormat = () => { + // Whichever way the format is changed, the offer has been answered. if (d.format === "html") { - patch({ format: "text", text: htmlToText(d.html) }); + patch({ format: "text", text: htmlToText(d.html), formatOffer: null }); } else { - patch({ format: "html", html: textToHtml(d.text, { linkify: false, quoteColors: false }).replace(/\n/g, "
") }); + patch({ format: "html", html: textToHtml(d.text, { linkify: false, quoteColors: false }).replace(/\n/g, "
"), formatOffer: null }); } }; @@ -261,6 +262,22 @@ export function Composer({ draft }: { draft: Draft }) { )} + {/* + Replying in one format to a message written in the other loses + something either way: the formatting of a rich reply, or the plain + text somebody chose to write in. The draft opens in the format the + settings ask for, and this offers the other one for this message + only, rather than quietly overriding the setting (#407). + */} + {d.formatOffer && ( +
+ {d.formatOffer === "html" ? translate("This message is rich text") : translate("This message is plain text")} + + +
+ )} {d.format === "html" ? ( addFiles(key, files)} showToolbar={showToolbar} autoFocus={initialFocus === "body"} /> ) : ( diff --git a/web/src/views/compose/__tests__/format-offer-bar.test.tsx b/web/src/views/compose/__tests__/format-offer-bar.test.tsx new file mode 100644 index 0000000..5d1cc34 --- /dev/null +++ b/web/src/views/compose/__tests__/format-offer-bar.test.tsx @@ -0,0 +1,63 @@ +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { Composer } from "../Composer"; +import { useCompose, type Draft } from "@/store/compose"; +import { useMail } from "@/store/mail"; + +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +/** + * The bar the composer shows when the draft's format doesn't match the message + * it is answering (#407). A store test can say the offer was made; only the + * component can say that pressing it converts the body and puts the bar away. + */ + +window.matchMedia = ((q: string) => ({ matches: false, media: q, addEventListener() {}, removeEventListener() {} })) as unknown as typeof window.matchMedia; + +const REPLY: Partial = { + key: "d1", replyMode: "reply", subject: "Re: Numbers", + format: "text", text: "\n\nOn Friday, Ann wrote:\n> hi", html: "

hi
", + formatOffer: "html", +}; + +describe("the format offer in the composer", () => { + let host: HTMLDivElement; + let root: Root; + const bar = () => document.querySelector(".composer-notice"); + const draft = () => useCompose.getState().drafts[0]!; + const button = (label: string) => Array.from(document.querySelectorAll(".composer-notice button")).find((b) => b.textContent === label || b.getAttribute("aria-label") === label)!; + + beforeEach(() => { + useMail.setState({ accountId: "a1", identities: [] as never }); + useCompose.setState({ drafts: [], activeKey: null, pendingSends: {} }); + const key = useCompose.getState().open(); + useCompose.getState().update(key, REPLY); + host = document.createElement("div"); + document.body.appendChild(host); + root = createRoot(host); + act(() => root.render()); + }); + afterEach(() => { act(() => root.unmount()); host.remove(); }); + + it("offers the message's own format, and says which it is", () => { + expect(bar()?.textContent).toContain("This message is rich text"); + expect(button("Switch to rich text")).toBeTruthy(); + }); + + it("switches this draft and puts the bar away", () => { + act(() => button("Switch to rich text").click()); + act(() => root.render()); + expect(draft().format).toBe("html"); + // The quoted reply came across, rather than the editor opening empty. + expect(draft().html).toContain("Ann wrote"); + expect(bar()).toBeNull(); + }); + + it("dismisses without changing the format", () => { + act(() => button("Dismiss").click()); + act(() => root.render()); + expect(draft().format).toBe("text"); + expect(bar()).toBeNull(); + }); +}); From 9ba2c6e290bf27ec7055bec26049c6c3dccdfc27 Mon Sep 17 00:00:00 2001 From: jcoffey <51408202+jcoffey-dev@users.noreply.github.com> Date: Sat, 19 Sep 2026 15:17:12 -0700 Subject: [PATCH 2/7] Switching format keeps the original quote, not a flattened copy (#409) (#409) Switching a reply between plain text and rich text converted whatever body the draft was showing. Going from plain text to rich, that meant the quoted message came back as the "> " text quote run through a converter -- the sender's formatting, images and links gone, even though the original markup was sitting on the draft untouched. Both forms of the quote are prepared when the reply opens, so keep them on the draft and re-attach the right one when the format changes. Only what the author typed above the quote is converted. Where the quote can't be found any more -- edited by hand, or a draft that quotes nothing -- the whole body is converted as before, which is what every non-reply draft does. No new strings. (cherry picked from commit 88f9e6c50a04f8ffc4702d1d1e3cffa6a93e7690) --- web/src/store/__tests__/compose-email.test.ts | 2 +- .../__tests__/reply-format-offer.test.ts | 18 ++++++++ web/src/store/compose.ts | 12 +++++ web/src/views/compose/Composer.tsx | 18 +++++++- .../__tests__/format-offer-bar.test.tsx | 46 ++++++++++++++++++- 5 files changed, 92 insertions(+), 4 deletions(-) diff --git a/web/src/store/__tests__/compose-email.test.ts b/web/src/store/__tests__/compose-email.test.ts index 395ae97..1b574a4 100644 --- a/web/src/store/__tests__/compose-email.test.ts +++ b/web/src/store/__tests__/compose-email.test.ts @@ -18,7 +18,7 @@ function draft(over: Partial = {}): Draft { requestReceipt: false, priority: "normal", showCc: false, showBcc: false, showReplyTo: false, minimized: false, maximized: false, dirty: false, savedAt: null, - saving: false, sending: false, error: null, signatureHtml: "", replyMode: null, formatOffer: null, sendAt: null, + saving: false, sending: false, error: null, signatureHtml: "", replyMode: null, quoteHtml: "", quoteText: "", formatOffer: null, sendAt: null, ...over, }; } diff --git a/web/src/store/__tests__/reply-format-offer.test.ts b/web/src/store/__tests__/reply-format-offer.test.ts index 6f8847c..608d5d6 100644 --- a/web/src/store/__tests__/reply-format-offer.test.ts +++ b/web/src/store/__tests__/reply-format-offer.test.ts @@ -98,6 +98,24 @@ describe("answering a message written in the other format", () => { expect(d.html).toContain("hi"); }); + it("keeps the quoted message in both formats, so a switch can restore it", async () => { + composeIn("text"); + const d = await draftFor(RICH, "reply"); + // The HTML quote is the original's markup, not the text one converted. + expect(d.quoteHtml).toContain("

hi

"); + expect(d.quoteHtml).toContain("ihm-quote"); + expect(d.quoteText).toContain("Ann"); + expect(d.text.endsWith(d.quoteText)).toBe(true); + }); + + it("quotes nothing on a message started from scratch", () => { + composeIn("text"); + const key = useCompose.getState().open(); + const d = useCompose.getState().drafts.find((x) => x.key === key)!; + expect(d.quoteHtml).toBe(""); + expect(d.quoteText).toBe(""); + }); + it("makes no offer on a message started from scratch", () => { composeIn("text"); const key = useCompose.getState().open(); diff --git a/web/src/store/compose.ts b/web/src/store/compose.ts index 0a02669..6ec2906 100644 --- a/web/src/store/compose.ts +++ b/web/src/store/compose.ts @@ -76,6 +76,14 @@ export interface Draft { /** Original identity signature HTML currently embedded, to replace on identity switch. */ signatureHtml: string; replyMode: "reply" | "replyAll" | "forward" | null; + /** + * The quoted message as it was prepared in each format, kept so that + * switching format re-attaches the original rather than a conversion of + * whatever the other format flattened it into. Empty on a draft that quotes + * nothing. + */ + quoteHtml: string; + quoteText: string; /** * The format the message being answered was written in, when it is not the * one this draft opened in (#407). The composer offers the switch; answering @@ -152,6 +160,8 @@ function blankDraft(init: Partial = {}): Draft { error: null, signatureHtml: "", replyMode: null, + quoteHtml: "", + quoteText: "", formatOffer: null, sendAt: null, ...init, @@ -474,6 +484,8 @@ export const useCompose = create((set, get) => ({ relatedKeyword: mode === "forward" ? "$forwarded" : "$answered", signatureHtml: sigHtml, replyMode: mode, + quoteHtml, + quoteText: quoteTxt, formatOffer: origFormat === s.composeFormat ? null : origFormat, }); set((st) => ({ drafts: [...st.drafts, d], activeKey: d.key })); diff --git a/web/src/views/compose/Composer.tsx b/web/src/views/compose/Composer.tsx index 644a032..a08f4f7 100644 --- a/web/src/views/compose/Composer.tsx +++ b/web/src/views/compose/Composer.tsx @@ -147,12 +147,26 @@ export function Composer({ draft }: { draft: Draft }) { patch({ sendAt: at.getTime() }); }; + /* + * Switching format converts what has been written, but the quoted message + * is not something this draft wrote: it was prepared in both formats when + * the reply opened. Converting the plain-text quote into HTML would hand + * back a flattened copy of a message that still exists in its original + * markup, so re-attach that instead, and keep only what the author typed + * above it. Where the quote can no longer be found -- edited, or a draft + * that quotes nothing -- convert the whole body as before. + */ const toggleFormat = () => { // Whichever way the format is changed, the offer has been answered. if (d.format === "html") { - patch({ format: "text", text: htmlToText(d.html), formatOffer: null }); + const at = d.quoteHtml ? d.html.indexOf('
') : -1; + const written = at >= 0 ? htmlToText(d.html.slice(0, at)) : htmlToText(d.html); + patch({ format: "text", text: at >= 0 ? written.replace(/\s+$/, "") + d.quoteText : written, formatOffer: null }); } else { - patch({ format: "html", html: textToHtml(d.text, { linkify: false, quoteColors: false }).replace(/\n/g, "
"), formatOffer: null }); + const keeps = Boolean(d.quoteText) && d.text.endsWith(d.quoteText); + const written = keeps ? d.text.slice(0, d.text.length - d.quoteText.length) : d.text; + const asHtml = textToHtml(written, { linkify: false, quoteColors: false }).replace(/\n/g, "
"); + patch({ format: "html", html: keeps ? asHtml + d.quoteHtml : asHtml, formatOffer: null }); } }; diff --git a/web/src/views/compose/__tests__/format-offer-bar.test.tsx b/web/src/views/compose/__tests__/format-offer-bar.test.tsx index 5d1cc34..8d8626e 100644 --- a/web/src/views/compose/__tests__/format-offer-bar.test.tsx +++ b/web/src/views/compose/__tests__/format-offer-bar.test.tsx @@ -15,9 +15,13 @@ import { useMail } from "@/store/mail"; window.matchMedia = ((q: string) => ({ matches: false, media: q, addEventListener() {}, removeEventListener() {} })) as unknown as typeof window.matchMedia; +const QUOTE_HTML = '

On Friday, Ann wrote:

Look at this

'; +const QUOTE_TEXT = "\n\nOn Friday, Ann wrote:\n> Look at this"; + const REPLY: Partial = { key: "d1", replyMode: "reply", subject: "Re: Numbers", - format: "text", text: "\n\nOn Friday, Ann wrote:\n> hi", html: "

hi
", + format: "text", text: QUOTE_TEXT, html: `

${QUOTE_HTML}`, + quoteHtml: QUOTE_HTML, quoteText: QUOTE_TEXT, formatOffer: "html", }; @@ -54,6 +58,46 @@ describe("the format offer in the composer", () => { expect(bar()).toBeNull(); }); + /* + * The message being quoted was prepared in both formats when the reply + * opened. Switching used to convert the plain-text body it had, handing + * back a flattened copy -- "> Look at this" -- of markup that still + * existed untouched on the draft. + */ + it("restores the original message, rather than converting the flattened quote", () => { + act(() => button("Switch to rich text").click()); + act(() => root.render()); + expect(draft().html).toContain("this"); + expect(draft().html).toContain("
"); + expect(draft().html).not.toContain("> Look at this"); + }); + + it("keeps what the author typed above the quote", () => { + useCompose.getState().update("d1", { text: `Thanks, that helps.${QUOTE_TEXT}` }); + act(() => root.render()); + act(() => button("Switch to rich text").click()); + act(() => root.render()); + expect(draft().html).toContain("Thanks, that helps."); + expect(draft().html).toContain("this"); + // Once only: the typed reply must not arrive with the quote doubled. + expect(draft().html.match(/On Friday, Ann wrote:/g)).toHaveLength(1); + }); + + it("goes back to plain text with the prepared quote, not a re-flattened one", () => { + // A rich draft answering a plain-text message: the offer runs the other way. + act(() => { + useCompose.getState().update("d1", { format: "html", html: `
Thanks.
${QUOTE_HTML}`, formatOffer: "text" }); + }); + act(() => root.render()); + act(() => button("Switch to plain text").click()); + act(() => root.render()); + expect(draft().format).toBe("text"); + expect(draft().text).toContain("Thanks."); + // The prepared plain-text quote, not HTML run through a converter. + expect(draft().text.endsWith(QUOTE_TEXT)).toBe(true); + expect(draft().text).not.toContain("
"); + }); + it("dismisses without changing the format", () => { act(() => button("Dismiss").click()); act(() => root.render()); From a94fd9cce33e8ef559b0993e403d74995be03a86 Mon Sep 17 00:00:00 2001 From: jcoffey <51408202+jcoffey-dev@users.noreply.github.com> Date: Sat, 19 Sep 2026 15:48:13 -0700 Subject: [PATCH 3/7] Quoting follows the message's own image decision (#410) (#411) Replying sanitized the quoted body with allowRemote: true, so quoting fetched every remote image in the message whatever the reader had decided about it. A tracking pixel in the quote then reported the message read, and the address live, to whoever was counting -- the thing leaving the images blocked was meant to prevent. Edit as new and opening a draft that quotes a message did the same. The decision now lives in one place, remoteImagesAllowed(), asked with the same inputs the reader's answer used: the image policy, the trusted senders, whether the sender is a contact, and whether Show images was pressed on that message. The last of those was component state, so it moves to the mail store, where the composer can see it. Blocked images already keep their address in data-ihm-remote, so nothing is lost by not fetching: it goes back on the way out, and the sent quote is what its sender wrote. The recipient's client decides for itself, as it would with any other client's reply. Before pr408 this needed a rich-text default to reach; the format offer made it reachable from plain text, which is how it was found. No new strings. (cherry picked from commit d329b33912921a851c548bffef5085e5fbf72bed) --- web/src/lib/mail/remoteImages.ts | 45 +++++++++ .../__tests__/quote-image-policy.test.ts | 91 +++++++++++++++++++ web/src/store/compose.ts | 40 +++++++- web/src/store/mail/index.ts | 5 + web/src/store/mail/types.ts | 8 ++ web/src/views/mail/MessageView.tsx | 10 +- 6 files changed, 193 insertions(+), 6 deletions(-) create mode 100644 web/src/lib/mail/remoteImages.ts create mode 100644 web/src/store/__tests__/quote-image-policy.test.ts diff --git a/web/src/lib/mail/remoteImages.ts b/web/src/lib/mail/remoteImages.ts new file mode 100644 index 0000000..b31b773 --- /dev/null +++ b/web/src/lib/mail/remoteImages.ts @@ -0,0 +1,45 @@ +import type { ImagePolicy } from "@/store/settings"; + +/** + * Whether a message's remote images may be fetched. + * + * The reader's decision, in one place, because the composer has to make the + * same one. Quoting a message into a reply renders it again — and a quote that + * fetched what the reader had declined would report the message read, and the + * address live, to whoever was counting. The tracking pixel does not care + * which window it loaded in. + */ +export function remoteImagesAllowed(opts: { + from: string | null | undefined; + policy: ImagePolicy; + trusted: string[]; + inContacts: boolean; + /** The reader pressed "Show images" on this message. */ + shown: boolean; +}): boolean { + if (opts.shown || opts.policy === "always") return true; + if (opts.trusted.includes((opts.from ?? "").toLowerCase())) return true; + return opts.policy === "contacts" && opts.inContacts; +} + +/** + * Put back the addresses of images that were blocked when the message was + * quoted, on the way out. + * + * Blocking keeps the original URL on the element (`data-ihm-remote`), so + * nothing was lost by not fetching it. The copy that leaves here should be the + * quote as its sender wrote it: the recipient's client decides for itself + * whether to load those images, the same as it would have with any other + * client's reply. + */ +export function restoreBlockedImages(html: string): string { + if (!html.includes("data-ihm-blocked")) return html; + const doc = new DOMParser().parseFromString(html, "text/html"); + for (const img of Array.from(doc.querySelectorAll("img[data-ihm-blocked]"))) { + const url = img.getAttribute("data-ihm-remote"); + if (url) img.setAttribute("src", url); + img.removeAttribute("data-ihm-blocked"); + img.removeAttribute("data-ihm-remote"); + } + return doc.body.innerHTML; +} diff --git a/web/src/store/__tests__/quote-image-policy.test.ts b/web/src/store/__tests__/quote-image-policy.test.ts new file mode 100644 index 0000000..38b00f9 --- /dev/null +++ b/web/src/store/__tests__/quote-image-policy.test.ts @@ -0,0 +1,91 @@ +import { beforeEach, describe, expect, it } from "vitest"; +import { buildEmailObject, useCompose } from "@/store/compose"; +import { useMail } from "@/store/mail"; +import { useContacts } from "@/store/contacts"; +import { DEFAULT_SETTINGS, useSettings } from "@/store/settings"; +import type { Email, EmailAddress, Identity } from "@/jmap/types"; + +/* + * Remote images in a quoted message (#410). + * + * Quoting renders the message a second time. The reply was fetching every + * remote image in it, whatever the reader had decided — so replying to a + * message whose images had been left blocked told the tracker the mail was + * read and the address live. The composer is a window like any other. + */ + +const PIXEL = "https://tracker.example/open.gif?id=42"; + +const MESSAGE = { + id: "m1", messageId: [""], subject: "Sale", references: [], inReplyTo: [], keywords: {}, + attachments: [], receivedAt: "2026-09-04T10:00:00Z", mailboxIds: {}, + from: [{ name: "Shop", email: "shop@example.com" }], to: [{ name: "John", email: "john@example.org" }], cc: [], + htmlBody: [{ partId: "2", type: "text/html" }], + textBody: [{ partId: "1", type: "text/plain" }], + bodyValues: { + "1": { value: "Sale on now", isEncodingProblem: false, isTruncated: false }, + "2": { value: `

Sale on now

`, isEncodingProblem: false, isTruncated: false }, + }, +} as unknown as Email; + +const IDENTITIES = [{ id: "i1", name: "John", email: "john@example.org", replyTo: null }] as unknown as Identity[]; + +function replyDraft() { + useMail.setState({ + accountId: "a1", + identities: IDENTITIES as never, + getEmails: (async () => [MESSAGE]) as never, + defaultIdentity: (() => IDENTITIES[0]) as never, + loadIdentities: (async () => IDENTITIES) as never, + roleId: (() => null) as never, + }); + return useCompose.getState().reply(MESSAGE, "reply").then((key) => useCompose.getState().drafts.find((d) => d.key === key)!); +} + +beforeEach(() => { + useCompose.setState({ drafts: [], activeKey: null, pendingSends: {} }); + useMail.setState({ imagesShown: {} }); + useContacts.setState({ loaded: false } as never); + useSettings.setState({ settings: { ...DEFAULT_SETTINGS, imagePolicy: "ask", composeFormat: "html" } }); +}); + +describe("quoting a message whose images were not allowed", () => { + it("does not put a fetchable address in the draft", async () => { + const d = await replyDraft(); + expect(d.html).not.toContain(PIXEL.split("?")[0]! + '"'); + expect(d.html).toContain("data-ihm-blocked"); + // The src is what the browser would fetch; nothing else in the draft is. + expect(/]+src="https:/.test(d.html)).toBe(false); + }); + + it("keeps the address, so the sent copy is the quote as it was written", async () => { + const d = await replyDraft(); + expect(d.html).toContain(PIXEL); + const email = await buildEmailObject({ ...d, to: [{ name: null, email: "shop@example.com" }] as EmailAddress[] }, { forSend: true }); + const sent = JSON.stringify(email); + expect(sent).toContain(PIXEL); + expect(sent).not.toContain("data-ihm-blocked"); + }); + + it("fetches them once the reader has shown images on that message", async () => { + useMail.setState({ imagesShown: { m1: true } }); + const d = await replyDraft(); + expect(d.html).toContain(`src="${PIXEL}"`); + expect(d.html).not.toContain("data-ihm-blocked"); + }); + + it("fetches them when the policy is to show images always", async () => { + useSettings.setState((s) => ({ settings: { ...s.settings, imagePolicy: "always" } })); + expect((await replyDraft()).html).toContain(`src="${PIXEL}"`); + }); + + it("fetches them from a sender the reader trusts", async () => { + useSettings.setState((s) => ({ settings: { ...s.settings, trustedImageSenders: ["shop@example.com"] } })); + expect((await replyDraft()).html).toContain(`src="${PIXEL}"`); + }); + + it("leaves them blocked for a stranger when the policy is contacts only", async () => { + useSettings.setState((s) => ({ settings: { ...s.settings, imagePolicy: "contacts" } })); + expect((await replyDraft()).html).toContain("data-ihm-blocked"); + }); +}); diff --git a/web/src/store/compose.ts b/web/src/store/compose.ts index 6ec2906..6ee6afe 100644 --- a/web/src/store/compose.ts +++ b/web/src/store/compose.ts @@ -5,6 +5,7 @@ import { formatFullDate, uid } from "@/lib/format"; import { formatAddress, parseMailto, sameAddress, uniqueAddresses } from "@/lib/address"; import { escapeHtml, htmlToText, quoteText, replySubject, textToHtml } from "@/lib/text/text"; import { hasHtmlAlternative, sanitizeEmailHtml, sanitizeEditorHtml } from "@/lib/text/html"; +import { remoteImagesAllowed, restoreBlockedImages } from "@/lib/mail/remoteImages"; import { toast } from "@/ui/toast"; import { useMail, FULL_PROPS, BODY_PROPS } from "./mail"; import { useSession } from "./session"; @@ -13,6 +14,7 @@ import { formatScheduleTime, holdUntil } from "@/lib/schedule"; import { t as translate } from "@/lib/i18n"; import { BASE_PATH } from "@/lib/basePath"; import { settings } from "./settings"; +import { useContacts } from "./contacts"; import { emlFilename } from "@/lib/text/emlName"; import { fillPlaceholders, type PlaceholderContext } from "@/lib/templatePlaceholders"; import { shareBody, type SharedContent } from "@/lib/shareTarget"; @@ -168,6 +170,26 @@ function blankDraft(init: Partial = {}): Draft { }; } +/** + * Whether this message's remote images may be fetched into a composer. + * + * The same question the reader answered, asked with the same inputs: the + * policy, the trusted senders, whether the sender is a contact, and whether + * the reader pressed "Show images" on this message. + */ +function remoteImagesForMessage(email: Email): boolean { + const s = settings(); + const from = email.from?.[0]?.email; + const contacts = useContacts.getState(); + return remoteImagesAllowed({ + from, + policy: s.imagePolicy, + trusted: s.trustedImageSenders, + inContacts: Boolean(from && contacts.loaded && contacts.lookupByEmail(from)), + shown: Boolean(useMail.getState().imagesShown[email.id]), + }); +} + export function signatureBlock(identity: Identity | undefined, format: "html" | "text"): string { if (!identity) return ""; if (format === "text") return identity.textSignature ? `\n\n-- \n${identity.textSignature}` : ""; @@ -284,7 +306,7 @@ export const useCompose = create((set, get) => ({ showCc: Boolean(full.cc?.length), showBcc: Boolean(full.bcc?.length), subject: full.subject ?? "", - html: html ? sanitizeEmailHtml(html, { cidMap, allowRemote: true, dropStyleBlocks: true }).html : textToHtml(text).replace(/\n/g, "
"), + html: html ? sanitizeEmailHtml(html, { cidMap, allowRemote: remoteImagesForMessage(full), dropStyleBlocks: true }).html : textToHtml(text).replace(/\n/g, "
"), text: text || (html ? htmlToText(html) : ""), format: html ? "html" : settings().composeFormat, attachments, @@ -351,7 +373,7 @@ export const useCompose = create((set, get) => ({ showCc: Boolean(full.cc?.length), showBcc: Boolean(full.bcc?.length), subject: full.subject ?? "", - html: html ? sanitizeEmailHtml(html, { cidMap, allowRemote: true, dropStyleBlocks: true }).html : textToHtml(text).replace(/\n/g, "
"), + html: html ? sanitizeEmailHtml(html, { cidMap, allowRemote: remoteImagesForMessage(full), dropStyleBlocks: true }).html : textToHtml(text).replace(/\n/g, "
"), text: text || (html ? htmlToText(html) : ""), format: html ? "html" : settings().composeFormat, attachments, @@ -439,9 +461,17 @@ export const useCompose = create((set, get) => ({ attachments.push({ id: uid("a"), name: a.name ?? "attachment", type: a.type, size: a.size, blobId: a.blobId, progress: 100, error: null, cid: a.cid ?? undefined, inline }); } } + /* + * Quoting renders the message a second time, so the reader's decision + * about its remote images applies here too: a quote that fetched what + * they declined would report the message read to whoever was counting + * (#410). Blocked images keep their address and get it back on the way + * out, so the recipient's copy is the quote as its sender wrote it. + */ + const allowRemote = remoteImagesForMessage(full); // Inline images are shown via their blob URLs in the editor and converted back to cid: at send time. const quotedHtmlBody = origHtml - ? sanitizeEmailHtml(origHtml, { cidMap, allowRemote: true, proxyRemote: false, dropStyleBlocks: true }).html + ? sanitizeEmailHtml(origHtml, { cidMap, allowRemote, proxyRemote: false, dropStyleBlocks: true }).html : textToHtml(origText).replace(/\n/g, "
"); const fromStr = escapeHtml((full.from ?? []).map(formatAddress).join(", ")); const date = formatFullDate(full.receivedAt); @@ -808,7 +838,9 @@ export async function buildEmailObject(d: Draft, opts: { forSend: boolean; mailb if (!ident) throw new Error(translate("No sending identity available")); const from: EmailAddress = { name: ident.name || null, email: ident.email }; - let html = d.format === "html" ? d.html : ""; + // Images blocked when the message was quoted keep their address; the copy + // that leaves carries it, and the recipient's client decides for itself. + let html = d.format === "html" ? restoreBlockedImages(d.html) : ""; const text = d.format === "html" ? htmlToText(d.html) : d.text; // Inline attachments shown via blob URLs in the editor → back to cid: references. diff --git a/web/src/store/mail/index.ts b/web/src/store/mail/index.ts index 7e71131..6a71dcb 100644 --- a/web/src/store/mail/index.ts +++ b/web/src/store/mail/index.ts @@ -78,6 +78,7 @@ function offerArchiveFolder(retry: () => Promise): void { export const useMail = create((set, get) => ({ accountId: null, + imagesShown: {}, mailboxes: {}, mailboxState: null, mailboxesLoaded: false, @@ -875,6 +876,10 @@ export const useMail = create((set, get) => ({ } }, + showImages(id) { + set((s) => ({ imagesShown: { ...s.imagesShown, [id]: true } })); + }, + select(ids, on) { set((s) => { const next = { ...s.selected }; diff --git a/web/src/store/mail/types.ts b/web/src/store/mail/types.ts index 751a677..c8a65aa 100644 --- a/web/src/store/mail/types.ts +++ b/web/src/store/mail/types.ts @@ -33,6 +33,12 @@ export interface ListState extends ListQuery { export interface MailState { accountId: Id | null; + /** + * Messages the reader pressed "Show images" on, this session. Kept here + * rather than in the message view because replying quotes the message into + * a second window, which has to honour the same decision. + */ + imagesShown: Record; mailboxes: Record; mailboxState: string | null; mailboxesLoaded: boolean; @@ -113,6 +119,8 @@ export interface MailState { saveVacation(patch: Partial): Promise; loadQuota(): Promise; + /** Remember that this message's remote images were allowed by hand. */ + showImages(id: Id): void; select(ids: Id[], on: boolean): void; clearSelection(): void; /** Refresh the per-label unread counts, in one request. */ diff --git a/web/src/views/mail/MessageView.tsx b/web/src/views/mail/MessageView.tsx index d9133df..438a246 100644 --- a/web/src/views/mail/MessageView.tsx +++ b/web/src/views/mail/MessageView.tsx @@ -19,6 +19,7 @@ import { llmOpinion } from "@/lib/llmOpinion"; import { LlmOpinionBanner, LlmOpinionDetail, llmBannerOpinion } from "./LlmOpinion"; import { formatFullDate, formatListDate, formatSize } from "@/lib/format"; import { displayName, domainOf, formatAddress } from "@/lib/address"; +import { remoteImagesAllowed } from "@/lib/mail/remoteImages"; import { EMAIL_BASE_CSS, TEXT_EMAIL_CSS, hasHtmlAlternative, htmlDeclaresColors, markKeptSurfaces, sanitizeEmailHtml } from "@/lib/text/html"; import { openableInTab, previewKind } from "@/lib/preview"; // Loaded when first opened: it is not needed to show mail, and it is not small. @@ -120,7 +121,12 @@ export const MessageView = memo(function MessageView({ email: e, expanded, wasUn /* Stable, so the body's click handler keeps its identity between renders. Passing an inline arrow here is what made the handler change on every render in the first place. */ - const showImages = useCallback(() => setAllowRemote(true), []); + const showImages = useCallback(() => { + setAllowRemote(true); + // Recorded for the composer: a reply quotes this message and must not + // fetch what the reader has not agreed to (#410). + useMail.getState().showImages(e.id); + }, [e.id]); const [filterOpen, setFilterOpen] = useState(false); const moreMenu = useMenu(); const [, navigate] = useLocation(); @@ -130,7 +136,7 @@ export const MessageView = memo(function MessageView({ email: e, expanded, wasUn const from = e.from?.[0]; const senderTrusted = settings.trustedImageSenders.includes((from?.email ?? "").toLowerCase()); const inContacts = useContacts((s) => Boolean(from && s.loaded && s.lookupByEmail(from.email))); - const remoteAllowed = allowRemote || settings.imagePolicy === "always" || senderTrusted || (settings.imagePolicy === "contacts" && inContacts); + const remoteAllowed = remoteImagesAllowed({ from: from?.email, policy: settings.imagePolicy, trusted: settings.trustedImageSenders, inContacts, shown: allowRemote }); const imageProxy = useSession((s) => s.session?.ihasmail?.imageProxy ?? true); const scheduled = useScheduled((s) => s.pending[e.id]); const receipt = useMemo(() => mdnDecision(e), [e]); From 2fffc9043db25c793eb067f801ea817a22cdd2a3 Mon Sep 17 00:00:00 2001 From: jcoffey <51408202+jcoffey-dev@users.noreply.github.com> Date: Sat, 19 Sep 2026 16:08:52 -0700 Subject: [PATCH 4/7] Quote images through the proxy, and unproxy them on the way out (#412) (#413) Reading a message fetches its remote images through this server, so the sender learns nothing about the reader. Quoting the same message into a reply fetched them directly: same pixel, same reader, but the request carried their IP and user agent -- exactly what the proxy withholds. A quote now proxies them the way the message view does. That alone would be wrong, because a proxied URL belongs to this deployment: sent unchanged it would reach the recipient as images only this server can serve, broken for them and a beacon back here. So buildEmailObject turns them back into the addresses they came from, beside the pass that restores images blocked under pr411 and the one that turns editor blob URLs into cid: references. Deployments with the proxy off are unaffected: the quote fetches directly, as reading does there. Three tests from pr411 asserted the address sat in src when images were allowed, which was the old behaviour; they now ask whether the draft fetches it at all, proxied or not. No new strings. (cherry picked from commit 23557a72a2a72088081792f8ea0cabedea4e7bcb) --- web/src/lib/mail/remoteImages.ts | 20 ++++ web/src/lib/text/html.ts | 19 +++- .../__tests__/quote-image-policy.test.ts | 13 ++- .../store/__tests__/quote-image-proxy.test.ts | 99 +++++++++++++++++++ web/src/store/compose.ts | 19 +++- 5 files changed, 161 insertions(+), 9 deletions(-) create mode 100644 web/src/store/__tests__/quote-image-proxy.test.ts diff --git a/web/src/lib/mail/remoteImages.ts b/web/src/lib/mail/remoteImages.ts index b31b773..aac0181 100644 --- a/web/src/lib/mail/remoteImages.ts +++ b/web/src/lib/mail/remoteImages.ts @@ -1,3 +1,4 @@ +import { unproxiedImageUrl } from "@/lib/text/html"; import type { ImagePolicy } from "@/store/settings"; /** @@ -22,6 +23,25 @@ export function remoteImagesAllowed(opts: { return opts.policy === "contacts" && opts.inContacts; } +/** + * Point proxied images back at their own addresses, on the way out. + * + * Reading a message fetches its remote images through this server, so the + * sender learns nothing about the reader. Those URLs belong to this + * deployment, so a quote that kept them would reach the recipient as images + * only this server can serve -- broken for them, and a beacon back here for + * anyone who could load them (#412). + */ +export function unproxyImages(html: string): string { + if (!html.includes("/api/image?url=")) return html; + const doc = new DOMParser().parseFromString(html, "text/html"); + for (const img of Array.from(doc.querySelectorAll("img[src]"))) { + const real = unproxiedImageUrl(img.getAttribute("src") ?? ""); + if (real) img.setAttribute("src", real); + } + return doc.body.innerHTML; +} + /** * Put back the addresses of images that were blocked when the message was * quoted, on the way out. diff --git a/web/src/lib/text/html.ts b/web/src/lib/text/html.ts index bb0d9a4..a2eb1ed 100644 --- a/web/src/lib/text/html.ts +++ b/web/src/lib/text/html.ts @@ -1,5 +1,5 @@ import DOMPurify from "dompurify"; -import { withBase } from "@/lib/basePath"; +import { BASE_PATH, withBase } from "@/lib/basePath"; export interface SanitizeOptions { /** Map of Content-ID (without angle brackets) → URL for inline images. */ @@ -158,6 +158,23 @@ export function proxiedImageUrl(url: string): string { return withBase(`/api/image?url=${encodeURIComponent(url)}`); } +/** + * The address a proxied image really points at, or null if this is not one. + * + * A proxied URL is this server's, so it is right for reading a message and + * wrong for sending one: a quote left this way would hand the recipient + * images that only load from inside this deployment (#412). + */ +export function unproxiedImageUrl(src: string): string | null { + const path = `${BASE_PATH}/api/image?url=`; + if (!src.startsWith(path)) return null; + try { + return decodeURIComponent(src.slice(path.length)) || null; + } catch { + return null; // Malformed escape: leave it alone rather than mangle it. + } +} + export function sanitizeEmailHtml(input: string, opts: SanitizeOptions = {}): SanitizeResult { ensureHooks(); let bodyStyle = ""; diff --git a/web/src/store/__tests__/quote-image-policy.test.ts b/web/src/store/__tests__/quote-image-policy.test.ts index 38b00f9..fb92444 100644 --- a/web/src/store/__tests__/quote-image-policy.test.ts +++ b/web/src/store/__tests__/quote-image-policy.test.ts @@ -30,6 +30,13 @@ const MESSAGE = { const IDENTITIES = [{ id: "i1", name: "John", email: "john@example.org", replyTo: null }] as unknown as Identity[]; +/** + * Whether the draft will actually load the image. Allowed images go through + * the server's proxy where the deployment has one (#412), so the address is + * escaped inside an `/api/image` URL rather than sitting in `src` as it is. + */ +const fetched = (html: string) => html.includes(`/api/image?url=${encodeURIComponent(PIXEL)}`) || html.includes(`src="${PIXEL}"`); + function replyDraft() { useMail.setState({ accountId: "a1", @@ -70,18 +77,18 @@ describe("quoting a message whose images were not allowed", () => { it("fetches them once the reader has shown images on that message", async () => { useMail.setState({ imagesShown: { m1: true } }); const d = await replyDraft(); - expect(d.html).toContain(`src="${PIXEL}"`); + expect(fetched(d.html)).toBe(true); expect(d.html).not.toContain("data-ihm-blocked"); }); it("fetches them when the policy is to show images always", async () => { useSettings.setState((s) => ({ settings: { ...s.settings, imagePolicy: "always" } })); - expect((await replyDraft()).html).toContain(`src="${PIXEL}"`); + expect(fetched((await replyDraft()).html)).toBe(true); }); it("fetches them from a sender the reader trusts", async () => { useSettings.setState((s) => ({ settings: { ...s.settings, trustedImageSenders: ["shop@example.com"] } })); - expect((await replyDraft()).html).toContain(`src="${PIXEL}"`); + expect(fetched((await replyDraft()).html)).toBe(true); }); it("leaves them blocked for a stranger when the policy is contacts only", async () => { diff --git a/web/src/store/__tests__/quote-image-proxy.test.ts b/web/src/store/__tests__/quote-image-proxy.test.ts new file mode 100644 index 0000000..0c21079 --- /dev/null +++ b/web/src/store/__tests__/quote-image-proxy.test.ts @@ -0,0 +1,99 @@ +import { beforeEach, describe, expect, it } from "vitest"; +import { buildEmailObject, useCompose } from "@/store/compose"; +import { useMail } from "@/store/mail"; +import { useContacts } from "@/store/contacts"; +import { useSession } from "@/store/session"; +import { DEFAULT_SETTINGS, useSettings } from "@/store/settings"; +import { unproxyImages } from "@/lib/mail/remoteImages"; +import type { Email, EmailAddress, Identity } from "@/jmap/types"; + +/* + * Remote images in a quote go through this server, and come back out pointing + * at their own addresses (#412). + * + * Reading a message proxies its images so the sender learns nothing about the + * reader. Quoting fetched them directly, which handed the same pixel the + * reader's IP and user agent. Proxying the quote is only half of it: those + * URLs belong to this deployment, so the copy that is sent has to carry the + * originals or the recipient gets images only this server can serve. + */ + +const IMAGE = "https://cdn.example/banner.png?id=7"; + +const MESSAGE = { + id: "m1", messageId: [""], subject: "Sale", references: [], inReplyTo: [], keywords: {}, + attachments: [], receivedAt: "2026-09-04T10:00:00Z", mailboxIds: {}, + from: [{ name: "Shop", email: "shop@example.com" }], to: [{ name: "John", email: "john@example.org" }], cc: [], + htmlBody: [{ partId: "2", type: "text/html" }], + textBody: [{ partId: "1", type: "text/plain" }], + bodyValues: { + "1": { value: "Sale on now", isEncodingProblem: false, isTruncated: false }, + "2": { value: `

Sale

`, isEncodingProblem: false, isTruncated: false }, + }, +} as unknown as Email; + +const IDENTITIES = [{ id: "i1", name: "John", email: "john@example.org", replyTo: null }] as unknown as Identity[]; + +function draftFor(mode: "reply" | "forward") { + useMail.setState({ + accountId: "a1", + identities: IDENTITIES as never, + getEmails: (async () => [MESSAGE]) as never, + defaultIdentity: (() => IDENTITIES[0]) as never, + loadIdentities: (async () => IDENTITIES) as never, + roleId: (() => null) as never, + }); + return useCompose.getState().reply(MESSAGE, mode).then((key) => useCompose.getState().drafts.find((d) => d.key === key)!); +} + +const proxy = (on: boolean) => useSession.setState({ session: { ihasmail: { imageProxy: on } } } as never); + +beforeEach(() => { + useCompose.setState({ drafts: [], activeKey: null, pendingSends: {} }); + useMail.setState({ imagesShown: {} }); + useContacts.setState({ loaded: false } as never); + // Images allowed, so the question is only how they are fetched. + useSettings.setState({ settings: { ...DEFAULT_SETTINGS, imagePolicy: "always", composeFormat: "html" } }); + proxy(true); +}); + +describe("images in a quote, while the reply is being written", () => { + it("are fetched through this server, as reading the message does", async () => { + const d = await draftFor("reply"); + expect(d.html).toContain("/api/image?url="); + expect(d.html).not.toContain(`src="${IMAGE}"`); + }); + + it("are fetched directly where the deployment has no proxy", async () => { + proxy(false); + const d = await draftFor("reply"); + expect(d.html).toContain(`src="${IMAGE}"`); + expect(d.html).not.toContain("/api/image?url="); + }); + + it("go through it on a forward too", async () => { + expect((await draftFor("forward")).html).toContain("/api/image?url="); + }); +}); + +describe("the copy that is sent", () => { + it("points at the image's own address, not at this server", async () => { + const d = await draftFor("reply"); + const sent = JSON.stringify(await buildEmailObject({ ...d, to: [{ name: null, email: "shop@example.com" }] as EmailAddress[] }, { forSend: true })); + expect(sent).toContain(IMAGE.replace(/&/g, "&")); + expect(sent).not.toContain("/api/image?url="); + }); + + it("restores a signature or template image that used the proxy as well", () => { + const logo = "https://cdn.example/logo.png"; + const html = `

Regards

`; + const out = unproxyImages(html); + expect(out).toContain(`src="${logo}"`); + expect(out).toContain('src="cid:x@1"'); + }); + + it("leaves everything else alone", () => { + const html = 'link'; + expect(unproxyImages(html)).toBe(html); + }); +}); diff --git a/web/src/store/compose.ts b/web/src/store/compose.ts index 6ee6afe..1833f34 100644 --- a/web/src/store/compose.ts +++ b/web/src/store/compose.ts @@ -5,7 +5,7 @@ import { formatFullDate, uid } from "@/lib/format"; import { formatAddress, parseMailto, sameAddress, uniqueAddresses } from "@/lib/address"; import { escapeHtml, htmlToText, quoteText, replySubject, textToHtml } from "@/lib/text/text"; import { hasHtmlAlternative, sanitizeEmailHtml, sanitizeEditorHtml } from "@/lib/text/html"; -import { remoteImagesAllowed, restoreBlockedImages } from "@/lib/mail/remoteImages"; +import { remoteImagesAllowed, restoreBlockedImages, unproxyImages } from "@/lib/mail/remoteImages"; import { toast } from "@/ui/toast"; import { useMail, FULL_PROPS, BODY_PROPS } from "./mail"; import { useSession } from "./session"; @@ -177,6 +177,11 @@ function blankDraft(init: Partial = {}): Draft { * policy, the trusted senders, whether the sender is a contact, and whether * the reader pressed "Show images" on this message. */ +/** Whether this deployment fetches remote images through its own server. */ +function imageProxyOn(): boolean { + return useSession.getState().session?.ihasmail?.imageProxy ?? true; +} + function remoteImagesForMessage(email: Email): boolean { const s = settings(); const from = email.from?.[0]?.email; @@ -306,7 +311,7 @@ export const useCompose = create((set, get) => ({ showCc: Boolean(full.cc?.length), showBcc: Boolean(full.bcc?.length), subject: full.subject ?? "", - html: html ? sanitizeEmailHtml(html, { cidMap, allowRemote: remoteImagesForMessage(full), dropStyleBlocks: true }).html : textToHtml(text).replace(/\n/g, "
"), + html: html ? sanitizeEmailHtml(html, { cidMap, allowRemote: remoteImagesForMessage(full), proxyRemote: imageProxyOn(), dropStyleBlocks: true }).html : textToHtml(text).replace(/\n/g, "
"), text: text || (html ? htmlToText(html) : ""), format: html ? "html" : settings().composeFormat, attachments, @@ -373,7 +378,7 @@ export const useCompose = create((set, get) => ({ showCc: Boolean(full.cc?.length), showBcc: Boolean(full.bcc?.length), subject: full.subject ?? "", - html: html ? sanitizeEmailHtml(html, { cidMap, allowRemote: remoteImagesForMessage(full), dropStyleBlocks: true }).html : textToHtml(text).replace(/\n/g, "
"), + html: html ? sanitizeEmailHtml(html, { cidMap, allowRemote: remoteImagesForMessage(full), proxyRemote: imageProxyOn(), dropStyleBlocks: true }).html : textToHtml(text).replace(/\n/g, "
"), text: text || (html ? htmlToText(html) : ""), format: html ? "html" : settings().composeFormat, attachments, @@ -469,9 +474,13 @@ export const useCompose = create((set, get) => ({ * out, so the recipient's copy is the quote as its sender wrote it. */ const allowRemote = remoteImagesForMessage(full); + // Fetched through this server while the reply is written, as reading the + // message does, and pointed back at their own addresses on the way out + // (#412). + const proxyRemote = imageProxyOn(); // Inline images are shown via their blob URLs in the editor and converted back to cid: at send time. const quotedHtmlBody = origHtml - ? sanitizeEmailHtml(origHtml, { cidMap, allowRemote, proxyRemote: false, dropStyleBlocks: true }).html + ? sanitizeEmailHtml(origHtml, { cidMap, allowRemote, proxyRemote, dropStyleBlocks: true }).html : textToHtml(origText).replace(/\n/g, "
"); const fromStr = escapeHtml((full.from ?? []).map(formatAddress).join(", ")); const date = formatFullDate(full.receivedAt); @@ -840,7 +849,7 @@ export async function buildEmailObject(d: Draft, opts: { forSend: boolean; mailb // Images blocked when the message was quoted keep their address; the copy // that leaves carries it, and the recipient's client decides for itself. - let html = d.format === "html" ? restoreBlockedImages(d.html) : ""; + let html = d.format === "html" ? unproxyImages(restoreBlockedImages(d.html)) : ""; const text = d.format === "html" ? htmlToText(d.html) : d.text; // Inline attachments shown via blob URLs in the editor → back to cid: references. From 8bfc7a85a9e7372f9d7b08a1a41af0ff822851ce Mon Sep 17 00:00:00 2001 From: jcoffey <51408202+jcoffey-dev@users.noreply.github.com> Date: Sun, 20 Sep 2026 14:50:48 -0700 Subject: [PATCH 5/7] A reply to a self-addressed message follows its Reply-To (#415) (#416) A website contact form mails the site's own address: From and To are both info@thesite, and the person who filled the form in is in Reply-To. Replying addressed the draft to info@thesite -- the site's own desk -- instead of to them. The reply already knows two shapes. A message somebody sent me is answered to its Reply-To, which is what that header is for. A message *I* sent is answered to the people I wrote to, and deliberately not to my own Reply-To, which is where answers to me belong and would send my reply to myself. A contact form passes the test for the second: every address in From is mine. So it fell down the chain the second shape keeps for a message with nobody obvious to answer -- To without me, then Cc, then, having run out, every address on the message, which here was mine alone. The Reply-To now goes in that chain, one step before the last: when no recipient but me is left and the message names a Reply-To that is not mine either, that address is who it is really from. Keeping it after the Cc is what leaves a message I did send alone -- somebody I actually wrote to still beats my own Reply-To, which is the case the existing guard was built for and its test still holds. No new strings. (cherry picked from commit 01dc322aebcff0e8075c54f81eb7d171a32fb9e7) --- .../store/__tests__/reply-addressing.test.ts | 24 +++++++++++++++++++ web/src/store/compose.ts | 13 ++++++++++ 2 files changed, 37 insertions(+) diff --git a/web/src/store/__tests__/reply-addressing.test.ts b/web/src/store/__tests__/reply-addressing.test.ts index 437aac8..d3ee2bb 100644 --- a/web/src/store/__tests__/reply-addressing.test.ts +++ b/web/src/store/__tests__/reply-addressing.test.ts @@ -153,6 +153,30 @@ describe("a message of mine with nobody obvious to reply to", () => { const d = await draftFor({ ...MINE, to: [ME], cc: [] } as Email, "reply"); expect(addrs(d.to)).toEqual([ME.email]); }); + + it("answers the Reply-To rather than my own desk when nobody else is on it", async () => { + /* + * A contact form: the site mails itself, From and To both its own address, + * and the person who filled the form in is in Reply-To. From alone makes + * this look like mine, and the fallback used to reply to me (#415). + */ + const form = { ...MINE, to: [ME], cc: [], replyTo: [{ name: "Michael", email: "michael@example.com" }] } as Email; + const d = await draftFor(form, "reply"); + expect(addrs(d.to)).toEqual(["michael@example.com"]); + }); + + it("does the same on a reply all, without cc-ing myself", async () => { + const form = { ...MINE, to: [ME], cc: [], replyTo: [{ name: "Michael", email: "michael@example.com" }] } as Email; + const d = await draftFor(form, "replyAll"); + expect(addrs(d.to)).toEqual(["michael@example.com"]); + expect(d.cc).toEqual([]); + }); + + it("still prefers somebody I actually wrote to over my own Reply-To", async () => { + // The Cc is a person; the Reply-To is where answers to me belong. + const d = await draftFor({ ...MINE, to: [ME], replyTo: [{ name: null, email: "desk@example.org" }] } as Email, "reply"); + expect(addrs(d.to)).toEqual([BOB.email]); + }); }); describe("forwarding", () => { diff --git a/web/src/store/compose.ts b/web/src/store/compose.ts index 1833f34..d48776d 100644 --- a/web/src/store/compose.ts +++ b/web/src/store/compose.ts @@ -436,6 +436,19 @@ export const useCompose = create((set, get) => ({ // Addressed only to myself, or only in Cc: there is still somebody this // is a reply to, and an empty To is not it. if (!to.length) { to = cc.length ? cc : withoutOwn(full.cc ?? []); cc = []; } + /* + * Nobody but me on the message, and a Reply-To pointing somewhere that + * is not mine: that address is who this is really from. + * + * A contact form is the shape of it -- From and To are both the site's + * own mailbox, and the person who filled the form in is in Reply-To. + * The address test above calls that mine, correctly as far as it goes, + * and the fallback then addressed the reply to my own desk (#415). + * + * After the Cc, not before it: a message I really did send carries my + * own Reply-To, and somebody I actually wrote to beats it. + */ + if (!to.length) to = withoutOwn(full.replyTo ?? []); if (!to.length) to = uniqueAddresses([...(full.to ?? []), ...(full.cc ?? [])]); } else { to = uniqueAddresses(full.replyTo?.length ? full.replyTo : (full.from ?? [])); From fedc34698e4cb418181239f3134dd60907df1279 Mon Sep 17 00:00:00 2001 From: jcoffey <51408202+jcoffey-dev@users.noreply.github.com> Date: Sun, 20 Sep 2026 14:53:45 -0700 Subject: [PATCH 6/7] The toolbar above an open message acts on that message (#414) (#417) With conversation view off, marking a message unread from the list -- the hover button, the right-click menu -- marked that message. Opening it and pressing Mark as unread in the toolbar above it marked every message in its thread, and so did Move to, Report spam and Delete. The setting already reaches all the way into the reading pane: the list draws one row per message, and `visibleMessages` narrows the pane to the one opened. The toolbar was half converted. Its labels were right -- Mark as unread against Mark as read, the star, the labels shown -- all of those read `messages`, which is the narrowed set. Only `rowIds`, the one thing actually handed to the action, still read `thread.emailIds`. So the button said one message and did the whole conversation. `rowIds` is now the same question `visibleMessages` answers for the pane, asked of the same ids, with the same fallback: an id that names nothing in the thread -- a link from somebody with conversation view on, a stale `m` in the URL -- shows the conversation, so the toolbar takes the conversation. Conversation view on is unchanged: nothing is singled out, so the whole thread comes back as before. No new strings. (cherry picked from commit f627bfc1237d6e8bbc728147022f624c3834d648) --- web/src/views/mail/ThreadView.tsx | 18 ++- .../__tests__/thread-toolbar-scope.test.tsx | 128 ++++++++++++++++++ 2 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 web/src/views/mail/__tests__/thread-toolbar-scope.test.tsx diff --git a/web/src/views/mail/ThreadView.tsx b/web/src/views/mail/ThreadView.tsx index 585e3dc..46db4f6 100644 --- a/web/src/views/mail/ThreadView.tsx +++ b/web/src/views/mail/ThreadView.tsx @@ -228,7 +228,23 @@ export function ThreadView({ threadId, mailboxId, onBack, actions, onNavigate, h }, [messages, reply]); const subject = messages[0]?.subject || emails[thread?.emailIds[0] ?? ""]?.subject || "(no subject)"; - const rowIds = thread ? thread.emailIds.filter((id) => emails[id]) : []; + /* + * What the toolbar acts on: the messages the pane is showing, not the thread + * they belong to. + * + * With conversation view off, opening a message opens that message -- the + * list shows it alone, the pane renders it alone, and the buttons above it + * said so, because `anyUnread` and the rest already read `messages`. Only the + * ids handed to the action still named the whole thread, so Mark as unread, + * Move to, Report spam and Delete quietly took every message in it (#414). + * + * Same fallback as the pane's: an id naming nothing in this thread means the + * whole conversation, so the buttons keep matching what is on screen. + */ + const rowIds = useMemo(() => { + const loaded = thread ? thread.emailIds.filter((id) => emails[id]).map((id) => ({ id })) : []; + return visibleMessages(loaded, messageId).map((m) => m.id); + }, [thread, emails, messageId]); const anyUnread = messages.some((e) => !e.keywords.$seen); const anyStarred = messages.some((e) => e.keywords.$flagged); const inJunk = Boolean(mailboxId && mailboxes[mailboxId]?.role === "junk"); diff --git a/web/src/views/mail/__tests__/thread-toolbar-scope.test.tsx b/web/src/views/mail/__tests__/thread-toolbar-scope.test.tsx new file mode 100644 index 0000000..24cf3c0 --- /dev/null +++ b/web/src/views/mail/__tests__/thread-toolbar-scope.test.tsx @@ -0,0 +1,128 @@ +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { ThreadView } from "../ThreadView"; +import { useMail } from "@/store/mail"; +import type { ListActions } from "../MessageList"; +import type { Email, Id } from "@/jmap/types"; + +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +/* jsdom has neither of these, and the opening scroll uses both. */ +Element.prototype.scrollIntoView = () => {}; +globalThis.ResizeObserver ??= class { observe() {} unobserve() {} disconnect() {} } as unknown as typeof ResizeObserver; + +/* jsdom has no matchMedia, and the toolbar asks whether this is a phone. */ +window.matchMedia = ((q: string) => ({ matches: false, media: q, addEventListener() {}, removeEventListener() {} })) as unknown as typeof window.matchMedia; + +/* + * Reported from the inbox with conversation view off: marking a message unread + * from the list -- hover button, right-click menu -- touched that message, but + * the same action from the toolbar above the *opened* message marked every + * message in its thread. Move to, Report spam and Delete did it too (#414). + * + * The toolbar's labels were already right: "Mark as unread" read the messages + * on screen. Only the ids it handed the action named the whole thread. + */ + +const msg = (id: Id, subject: string): Email => + ({ + id, threadId: "t1", subject, mailboxIds: { inbox: true }, keywords: { $seen: true }, + from: [{ name: "Ann", email: "ann@example.com" }], to: [{ name: "Me", email: "me@example.org" }], + receivedAt: "2026-09-20T10:00:00Z", size: 10, blobId: "b1", preview: "hi", + htmlBody: [], textBody: [{ partId: "1", type: "text/plain" }], + bodyValues: { "1": { value: "hi", isEncodingProblem: false, isTruncated: false } }, + attachments: [], + }) as unknown as Email; + +const FIRST = msg("m1", "The question"); +const SECOND = msg("m2", "Re: The question"); + +function stubStore() { + useMail.setState({ + accountId: "a1", + threads: { t1: { id: "t1", emailIds: ["m1", "m2"] } } as never, + emails: { m1: FIRST, m2: SECOND } as never, + fullIds: { m1: true, m2: true } as never, + loadingThreads: {} as never, + mailboxes: { inbox: { id: "inbox", name: "Inbox", role: "inbox" } } as never, + loadThread: (async () => undefined) as never, + setOpenThread: (() => undefined) as never, + markRead: (async () => undefined) as never, + roleId: (() => null) as never, + }); +} + +describe("what the toolbar above an opened message acts on", () => { + let host: HTMLDivElement; + let root: Root; + let actions: ListActions; + + const show = async (messageId: Id | null) => { + await act(async () => { + root.render( + undefined} onNavigate={() => undefined} hasPrev={false} hasNext={false} + />, + ); + }); + }; + + /** The toolbar buttons carry their shortcut in the title, as the tooltips show. */ + const press = async (title: string) => { + const btn = [...host.querySelectorAll("button")].find((b) => b.title === title); + expect(btn, `no toolbar button titled ${title}`).toBeTruthy(); + await act(async () => btn!.click()); + }; + + beforeEach(() => { + stubStore(); + actions = { + archive: vi.fn(async () => undefined), trash: vi.fn(async () => undefined), + spam: vi.fn(async () => undefined), read: vi.fn(async () => undefined), + star: vi.fn(async () => undefined), move: vi.fn(async () => undefined), + label: vi.fn(async () => undefined), + } as unknown as ListActions; + host = document.createElement("div"); + document.body.appendChild(host); + root = createRoot(host); + }); + + afterEach(async () => { + await act(async () => root.unmount()); + host.remove(); + }); + + it("marks only the message that is open, not its thread", async () => { + await show("m1"); + await press("Mark as unread"); + expect(actions.read).toHaveBeenCalledWith(false, ["m1"]); + }); + + it("moves, reports and deletes only that message too", async () => { + await show("m1"); + await press("Move to (v)"); + await press("Report spam (!)"); + await press("Delete (#)"); + expect(actions.move).toHaveBeenCalledWith(["m1"]); + expect(actions.spam).toHaveBeenCalledWith(["m1"]); + expect(actions.trash).toHaveBeenCalledWith(["m1"]); + }); + + it("takes the whole thread when the pane is showing the whole thread", async () => { + // Conversation view on: no message singled out, and the toolbar is the + // conversation's toolbar. That is the behaviour this must not disturb. + await show(null); + await press("Mark as unread"); + expect(actions.read).toHaveBeenCalledWith(false, ["m1", "m2"]); + }); + + it("falls back to the thread when the open id names nothing in it", async () => { + // A link from somebody with conversation view on, or a stale `m` in the + // URL. The pane shows the conversation, so the toolbar acts on it. + await show("gone"); + await press("Mark as unread"); + expect(actions.read).toHaveBeenCalledWith(false, ["m1", "m2"]); + }); +}); From c64a23f9d96d5de2d589da2ca5a9a548110dbd11 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Mon, 21 Sep 2026 08:25:58 -0700 Subject: [PATCH 7/7] List folders in sidebar order in the move-to picker The picker sorted folders A-Z by path, with Inbox first, so a folder dragged into place in the sidebar turned up somewhere else when moving mail. It now walks the tree in compareFolders order, the sidebar's order with every folder expanded: Inbox, then the saved order, then the special folders, then A-Z, with subfolders under their parent. treeOrder lives beside compareFolders. A folder the walk from the top cannot reach is appended rather than dropped, so it stays pickable as it was before. Closes #1 (cherry picked from commit ea03406646062359f74e16ad8a8aed074b4dc409) --- .../lib/mailbox/__tests__/folderOrder.test.ts | 22 ++++++- web/src/lib/mailbox/folderOrder.ts | 31 ++++++++++ web/src/views/mail/MailboxPicker.tsx | 8 ++- .../mail/__tests__/move-picker-order.test.tsx | 62 +++++++++++++++++++ 4 files changed, 119 insertions(+), 4 deletions(-) create mode 100644 web/src/views/mail/__tests__/move-picker-order.test.tsx diff --git a/web/src/lib/mailbox/__tests__/folderOrder.test.ts b/web/src/lib/mailbox/__tests__/folderOrder.test.ts index c21cf0b..faee3bf 100644 --- a/web/src/lib/mailbox/__tests__/folderOrder.test.ts +++ b/web/src/lib/mailbox/__tests__/folderOrder.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { canPlaceFolder, compareFolders, neighbour, placeFolder, siblingsOf } from "../folderOrder"; +import { canPlaceFolder, compareFolders, neighbour, placeFolder, siblingsOf, treeOrder } from "../folderOrder"; import type { Id, Mailbox } from "@/jmap/types"; const RIGHTS = { mayRename: true, mayCreateChild: true } as Mailbox["myRights"]; @@ -119,3 +119,23 @@ describe("neighbour", () => { expect(neighbour(hidden, "alpha", "up", (m) => m.isSubscribed)).toEqual({ targetId: "junk", placement: "before" }); }); }); + +describe("treeOrder", () => { + const ids = (all: Record) => treeOrder(all).map((m) => m.id); + + it("lists the tree the way the sidebar does, each folder followed by its subfolders", () => { + expect(ids(fresh)).toEqual(["inbox", "drafts", "sent", "junk", "trash", "alpha", "work", "clients", "zeta"]); + }); + + it("follows a saved order rather than A–Z", () => { + // #1 on GitLab: the move-to picker kept the old order after the sidebar changed. + const ordered = apply(fresh, { zeta: { sortOrder: 10 }, sent: { sortOrder: 20 }, alpha: { sortOrder: 30 }, drafts: { sortOrder: 40 }, junk: { sortOrder: 50 }, trash: { sortOrder: 60 }, work: { sortOrder: 70 } }); + expect(ids(ordered)).toEqual(["inbox", "zeta", "sent", "alpha", "drafts", "junk", "trash", "work", "clients"]); + }); + + it("still lists a folder the walk from the top can't reach", () => { + const looped = apply(fresh, { work: { parentId: "clients" } }); + expect(ids(looped)).toHaveLength(Object.keys(looped).length); + expect(ids(looped)).toEqual(expect.arrayContaining(["work", "clients"])); + }); +}); diff --git a/web/src/lib/mailbox/folderOrder.ts b/web/src/lib/mailbox/folderOrder.ts index b70e05a..46b9b3a 100644 --- a/web/src/lib/mailbox/folderOrder.ts +++ b/web/src/lib/mailbox/folderOrder.ts @@ -25,6 +25,37 @@ function roleRank(m: Mailbox): number { return m.role && m.role in ROLE_ORDER ? ROLE_ORDER[m.role]! : Number.MAX_SAFE_INTEGER; } +/** + * Every folder, parents before their children and siblings in + * `compareFolders` order: the sidebar's order with every folder expanded. + * Lists that show all folders at once, like the move-to picker, use this so a + * folder sits where the user dragged it rather than where A–Z would put it. + * + * A folder the walk from the top never reaches (a parent loop the server + * should not allow) is appended rather than dropped, so it can still be + * picked. + */ +export function treeOrder(mailboxes: Record): Mailbox[] { + const byParent = new Map(); + for (const m of Object.values(mailboxes)) { + const p = m.parentId && mailboxes[m.parentId] ? m.parentId : null; + byParent.set(p, [...(byParent.get(p) ?? []), m]); + } + for (const list of byParent.values()) list.sort(compareFolders); + const out: Mailbox[] = []; + const seen = new Set(); + const walk = (parent: Id | null) => { + for (const m of byParent.get(parent) ?? []) { + if (seen.has(m.id)) continue; + seen.add(m.id); + out.push(m); + walk(m.id); + } + }; + walk(null); + return out.concat(Object.values(mailboxes).filter((m) => !seen.has(m.id)).sort(compareFolders)); +} + /** Every folder under `parentId` (null: the top level), in list order. */ export function siblingsOf(mailboxes: Record, parentId: Id | null): Mailbox[] { return Object.values(mailboxes) diff --git a/web/src/views/mail/MailboxPicker.tsx b/web/src/views/mail/MailboxPicker.tsx index 43eece7..41ffeda 100644 --- a/web/src/views/mail/MailboxPicker.tsx +++ b/web/src/views/mail/MailboxPicker.tsx @@ -5,6 +5,7 @@ import { Dialog } from "@/ui/dialog"; import type { Id, Mailbox } from "@/jmap/types"; import { t } from "@/lib/i18n"; import { mailboxDisplayPath } from "@/lib/mailbox/mailboxName"; +import { treeOrder } from "@/lib/mailbox/folderOrder"; /** * @param need which right a folder has to grant to be worth offering. @@ -24,10 +25,11 @@ export function MailboxPicker({ title, onClose, onPick, exclude, need = "mayAddI const [q, setQ] = useState(""); const [active, setActive] = useState(0); const list = useMemo(() => { - const all = Object.values(mailboxes) + // The sidebar's order, not A–Z by path: a folder dragged into place has to + // be found in the same place here. + const all = treeOrder(mailboxes) .filter((m) => !exclude?.includes(m.id) && m.myRights[need] && (!allow || allow(m.id))) - .map((m) => ({ m, path: mailboxDisplayPath(m, mailboxes), pick: () => onPick(m.id) })) - .sort((a, b) => (a.m.role === "inbox" ? -1 : b.m.role === "inbox" ? 1 : a.path.localeCompare(b.path))); + .map((m) => ({ m, path: mailboxDisplayPath(m, mailboxes), pick: () => onPick(m.id) })); const rows: { m: Mailbox | null; path: string; pick: () => void }[] = root ? [{ m: null, path: root.label, pick: root.onPick }, ...all] : all; const ql = q.trim().toLowerCase(); return ql ? rows.filter((x) => x.path.toLowerCase().includes(ql)) : rows; diff --git a/web/src/views/mail/__tests__/move-picker-order.test.tsx b/web/src/views/mail/__tests__/move-picker-order.test.tsx new file mode 100644 index 0000000..6d00d78 --- /dev/null +++ b/web/src/views/mail/__tests__/move-picker-order.test.tsx @@ -0,0 +1,62 @@ +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { MailboxPicker } from "../MailboxPicker"; +import { useMail } from "@/store/mail"; +import type { Mailbox, MailboxRole } from "@/jmap/types"; + +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +/** + * The move-to picker (v) lists folders in the sidebar's order (#1 on GitLab). + * + * It used to sort A–Z by path, so a folder dragged into place in the sidebar + * turned up somewhere else here. The ordering has its own tests in + * lib/mailbox; these check what the dialog actually shows. + */ + +window.matchMedia = ((q: string) => ({ matches: false, media: q, addEventListener() {}, removeEventListener() {} })) as unknown as typeof window.matchMedia; + +const rights = { mayReadItems: true, mayAddItems: true, mayRemoveItems: true, maySetSeen: true, maySetKeywords: true, mayCreateChild: true, mayRename: true, mayDelete: true, maySubmit: true }; +const box = (id: string, name: string, parentId: string | null, role: MailboxRole = null, sortOrder = 0): Mailbox => ({ + id, name, parentId, role, sortOrder, totalEmails: 0, unreadEmails: 0, totalThreads: 0, unreadThreads: 0, myRights: rights, isSubscribed: true, +}); + +/** Ordered by hand in the sidebar: Zeta dragged to the top, Alpha to the bottom. */ +const MAILBOXES = { + inbox: box("inbox", "Inbox", null, "inbox", 10), + zeta: box("zeta", "Zeta", null, null, 20), + sent: box("sent", "Sent", null, "sent", 30), + work: box("work", "Work", null, null, 40), + clients: box("clients", "Clients", "work"), + trash: box("trash", "Deleted Items", null, "trash", 50), + alpha: box("alpha", "Alpha", null, null, 60), +}; + +describe("the move-to picker", () => { + let host: HTMLDivElement; + let root: Root; + const rows = () => Array.from(document.querySelectorAll('[role="option"]')).map((r) => r.querySelector(".grow")?.textContent); + + function open(props: Partial[0]> = {}) { + act(() => root.render( {}} onPick={() => {}} {...props} />)); + } + + beforeEach(() => { + useMail.setState({ mailboxes: MAILBOXES, mailboxesLoaded: true }); + host = document.createElement("div"); + document.body.appendChild(host); + root = createRoot(host); + }); + afterEach(() => { act(() => root.unmount()); host.remove(); }); + + it("lists folders in the order they were dragged into, not A–Z", () => { + open(); + expect(rows()).toEqual(["Inbox", "Zeta", "Sent", "Work", "Work / Clients", "Deleted Items", "Alpha"]); + }); + + it("keeps that order for the folders left after excluding one", () => { + open({ exclude: ["work"] }); + expect(rows()).toEqual(["Inbox", "Zeta", "Sent", "Work / Clients", "Deleted Items", "Alpha"]); + }); +});