From 893f40a21ddfa75d5e68c330f8ab6936ad44d6d9 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 04:40:19 +0100 Subject: [PATCH 1/7] fix: send group mail through the group's identity on Send now and quick reply --- src/api/__tests__/email-submission.test.ts | 33 +++++++++++- src/api/email.ts | 10 +++- src/components/email/QuickReplyBox.tsx | 51 +++++++++++-------- .../compose-send-target-wiring.test.ts | 29 +++++++++++ src/screens/ComposeScreen.tsx | 4 +- 5 files changed, 101 insertions(+), 26 deletions(-) create mode 100644 src/lib/__tests__/compose-send-target-wiring.test.ts diff --git a/src/api/__tests__/email-submission.test.ts b/src/api/__tests__/email-submission.test.ts index f0f55ded..e148012a 100644 --- a/src/api/__tests__/email-submission.test.ts +++ b/src/api/__tests__/email-submission.test.ts @@ -107,7 +107,7 @@ describe('rescheduleScheduledSend', () => { expect(result).toEqual({ emailSubmissionId: 'sub-2', sendAt: '2026-09-25T08:00:00Z' }); const [lookup] = mockRequest.mock.calls[0][0]; expect(lookup).toEqual([ - 'EmailSubmission/get', { accountId: 'acc-1', ids: ['sub-1'], properties: ['envelope', 'sendAt'] }, '0', + 'EmailSubmission/get', { accountId: 'acc-1', ids: ['sub-1'], properties: ['envelope', 'sendAt', 'identityId'] }, '0', ]); expect(createdArgs().create.replacement.envelope).toEqual({ mailFrom: { email: 'me@example.com', parameters: { HOLDFOR: '7200' } }, @@ -218,6 +218,37 @@ describe('rescheduleScheduledSend', () => { vi.mocked(Date.now).mockRestore(); }); + // A group send's undo entry carried the login's own identity id, which + // names nothing in the group account: both creates were refused and the + // message went back to Drafts unsent. + it("submits the replacement and the put-back through the original's identity, not the caller's", async () => { + const lookup = structuredClone(LOOKUP); + (lookup.methodResponses[0][1] as { list: Array> }).list[0].identityId = 'grp-identity'; + mockRequest + .mockResolvedValueOnce(lookup) + .mockResolvedValueOnce(CANCELLED) + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockResolvedValueOnce(REFUSED('Quota exceeded')) + .mockResolvedValueOnce(set({ created: { replacement: { id: 'sub-3' } } })); + + await rescheduleScheduledSend({ ...SCHEDULED, identityId: 'own-identity', accountId: 'grp-1' }, 0).catch(() => undefined); + + expect(createdArgs(0).create.replacement.identityId).toBe('grp-identity'); + expect(createdArgs(1).create.replacement.identityId).toBe('grp-identity'); + }); + + it("keeps the caller's identity when the server does not name one", async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce(CANCELLED) + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockResolvedValueOnce({ methodResponses: REPLACED }); + + await rescheduleScheduledSend(SCHEDULED, 0); + + expect(createdArgs().create.replacement.identityId).toBe('id-1'); + }); + it('moves the message back to Drafts when neither the replacement nor the original can be made', async () => { vi.spyOn(console, 'warn').mockImplementation(() => undefined); mockRequest diff --git a/src/api/email.ts b/src/api/email.ts index 75fedc13..f4c4ecce 100644 --- a/src/api/email.ts +++ b/src/api/email.ts @@ -2230,7 +2230,7 @@ export async function rescheduleScheduledSend( const lookup = await jmapClient.request( [ ['EmailSubmission/get', { - accountId, ids: [scheduled.emailSubmissionId], properties: ['envelope', 'sendAt'], + accountId, ids: [scheduled.emailSubmissionId], properties: ['envelope', 'sendAt', 'identityId'], }, '0'], ['Email/get', { accountId, ids: [scheduled.emailId], properties: ['to', 'cc', 'bcc'] }, '1'], ['Mailbox/get', { accountId, ids: null, properties: ['id', 'role'] }, '2'], @@ -2240,8 +2240,14 @@ export async function rescheduleScheduledSend( const original = (requireMethodResult(lookup, '0', 'EmailSubmission/get').list as Array<{ envelope?: { mailFrom?: { email?: string }; rcptTo?: Array<{ email?: string }> } | null; sendAt?: string; + identityId?: string; }> | undefined)?.[0]; const envelope = original?.envelope; + // The original's identity is one of the submission account's. The caller's + // may be the composer's, from the login's own account: for a group + // identity's message that id names nothing in the group account, and the + // replacement and the put-back were both refused (webmail parity). + const identityId = original?.identityId || scheduled.identityId; const held = requireMethodResult(lookup, '1', 'Email/get').list as Email[] | undefined; const headerRecipients = [...(held?.[0]?.to ?? []), ...(held?.[0]?.cc ?? []), ...(held?.[0]?.bcc ?? [])]; const source = recipients ?? (envelope?.rcptTo?.length ? envelope.rcptTo : headerRecipients); @@ -2271,7 +2277,7 @@ export async function rescheduleScheduledSend( create: { replacement: { emailId: scheduled.emailId, - identityId: scheduled.identityId, + identityId, envelope: { mailFrom: seconds === undefined ? { email: mailFrom } diff --git a/src/components/email/QuickReplyBox.tsx b/src/components/email/QuickReplyBox.tsx index 3d3f7068..420c8f77 100644 --- a/src/components/email/QuickReplyBox.tsx +++ b/src/components/email/QuickReplyBox.tsx @@ -22,7 +22,7 @@ import { resolveReplyIdentity } from '../../lib/reply-identity'; import { signatureIdentityFor, signPlainTextReply } from '../../lib/signature-utils'; import { pickEmailBody, plainTextBody } from '../../lib/email-body'; import { htmlToPlainText } from '../../lib/compose-html'; -import { mailboxesOfAccount } from '../../lib/mailbox-tree'; +import { sendTargetFor } from '../../lib/compose-send-account'; import { emailDisplayDate } from '../../lib/email-date'; import { buildQuoteHeader, quoteHeaderLabels } from '../../lib/quote-header'; @@ -66,16 +66,19 @@ export function QuickReplyBox({ email, jmapAccountId, onMoreOptions, onSent, onF const from = email.from?.[0]; if (!from?.email || email.keywords?.$draft) return null; + const refuseAfterSwitch = () => { + if (!loginMovedFrom(ownerLoginRef.current)) return false; + Alert.alert( + t('email_composer.login_switched_title', 'Account switched'), + t('email_viewer.quick_reply_login_switched', 'This message is in another account. Switch back to it to reply.'), + ); + return true; + }; + const send = async () => { const body = text.trim(); if (!body || sending) return; - if (loginMovedFrom(ownerLoginRef.current)) { - Alert.alert( - t('email_composer.login_switched_title', 'Account switched'), - t('email_viewer.quick_reply_login_switched', 'This message is in another account. Switch back to it to reply.'), - ); - return; - } + if (refuseAfterSwitch()) return; const ownEmails = identities.map((i) => i.email).filter(Boolean); // The own identity the message was delivered to (or, for our own message, // the one that sent it). Never the catch-all From rewrite: this box has no @@ -88,16 +91,20 @@ export function QuickReplyBox({ email, jmapAccountId, onMoreOptions, onSent, onF Alert.alert(t('common.error', 'Error'), t('email_viewer.unsubscribe_banner.no_identity', 'No sending identity available')); return; } - // Sent/Drafts of the account the reply is submitted from: the message's. - const scoped = mailboxesOfAccount(mailboxes, jmapAccountId); - const sent = scoped.find((m) => m.role === 'sent'); - const drafts = scoped.find((m) => m.role === 'drafts'); - if (!sent) { - Alert.alert(t('common.error', 'Error'), t('email_composer.no_sent_folder', 'No Sent folder found')); - return; - } setSending(true); try { + // The account, identity and folders the reply goes out through, as the + // composer picks them (#1090): a group address sends from the group's + // account with that account's identity id. The message's own account + // with the login's identity id named nothing there, or another + // identity. + const target = await sendTargetFor(identity, identity.email, mailboxes); + if (!target.sentMailboxId) { + Alert.alert(t('common.error', 'Error'), t('email_composer.no_sent_folder', 'No Sent folder found')); + return; + } + // The lookup may have outlasted a switch of logins. + if (refuseAfterSwitch()) return; const recipients = buildReplyRecipients( { from: email.from, replyToAddresses: email.replyTo, to: email.to, cc: email.cc }, 'reply', @@ -116,10 +123,10 @@ export function QuickReplyBox({ email, jmapAccountId, onMoreOptions, onSent, onF labels: quoteHeaderLabels(t), }); const threading = computeReplyThreadingHeaders(email); - const holdFor = jmapClient.undoSendHold(sendDelaySeconds, jmapAccountId); + const holdFor = jmapClient.undoSendHold(sendDelaySeconds, target.accountId); // Where the reply goes, read before it goes: Undo must reach that login // and account whatever the app switches to meanwhile. - const sentThrough = { login: currentSendLogin(), accountId: jmapAccountId ?? jmapClient.connectedAccountId ?? undefined }; + const sentThrough = { login: currentSendLogin(), accountId: target.accountId ?? jmapClient.connectedAccountId ?? undefined }; const result = await sendEmail( { from: [{ name: identity.name, email: identity.email }], @@ -135,15 +142,15 @@ export function QuickReplyBox({ email, jmapAccountId, onMoreOptions, onSent, onF inReplyTo: threading?.inReplyTo, references: threading?.references, }, - identity.id, - sent.originalId ?? sent.id, + target.identityId, + target.sentMailboxId, holdFor, - { draftsMailboxId: drafts ? (drafts.originalId ?? drafts.id) : undefined, accountId: jmapAccountId }, + { draftsMailboxId: target.draftsMailboxId, accountId: target.accountId }, ); // Held for the undo-send delay: the undo bar offers Undo / Send now. // Recorded before the flag below so its round trip doesn't eat the window. useSendUndoStore.getState().recordHeldSend(result, holdFor, { - identityId: identity.id, + identityId: target.identityId, ...sentThrough, from: [{ name: identity.name, email: identity.email }], }); diff --git a/src/lib/__tests__/compose-send-target-wiring.test.ts b/src/lib/__tests__/compose-send-target-wiring.test.ts new file mode 100644 index 00000000..1495b893 --- /dev/null +++ b/src/lib/__tests__/compose-send-target-wiring.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, it } from 'vitest'; +import fs from 'node:fs'; +import path from 'node:path'; + +// A group or shared identity sends from the group's account with that +// account's identity id (lib/compose-send-account). Both places below used +// the login's own identity id, which names nothing in the group account: +// "Send now" from the undo bar lost the message, and a quick reply was +// refused or went out through another identity. +const read = (rel: string) => fs.readFileSync(path.resolve(__dirname, rel), 'utf8'); + +describe('send target wiring', () => { + it("records the target's identity for the undo bar of a composer send", () => { + const source = read('../../screens/ComposeScreen.tsx'); + const held = source.slice(source.indexOf('recordHeldSend(result, holdForSeconds')); + expect(held).toMatch(/^recordHeldSend\(result, holdForSeconds, \{[\s\S]*?identityId: target\.identityId,/); + expect(held.slice(0, 300)).not.toContain('primaryIdentity.id'); + }); + + it('sends a quick reply through the target the composer would pick', () => { + const source = read('../../components/email/QuickReplyBox.tsx'); + expect(source).toMatch(/const target = await sendTargetFor\(identity, identity\.email, mailboxes\)/); + const call = source.slice(source.indexOf('await sendEmail(')); + expect(call).toMatch(/\},\s+target\.identityId,\s+target\.sentMailboxId,/); + expect(call).toMatch(/accountId: target\.accountId \}/); + expect(source).toMatch(/recordHeldSend\(result, holdFor, \{\s*identityId: target\.identityId,/); + expect(source).not.toMatch(/identity\.id,\s+sent\./); + }); +}); diff --git a/src/screens/ComposeScreen.tsx b/src/screens/ComposeScreen.tsx index ea5f054e..ab038b05 100644 --- a/src/screens/ComposeScreen.tsx +++ b/src/screens/ComposeScreen.tsx @@ -2407,7 +2407,9 @@ export default function ComposeScreen({ route, navigation }: Props) { } else if (result.scheduled) { // Undo-send window: the undo bar offers Undo / Send now. useSendUndoStore.getState().recordHeldSend(result, holdForSeconds, { - identityId: primaryIdentity.id, + // The identity the message went out through, as the submission + // account names it: "Send now" submits again with it. + identityId: target.identityId, accountId: submissionAccountId, login: ownerLoginRef.current, from: outgoing.from, From fdf8eece6256118fdae7210fc687625d8d442801 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 04:47:20 +0100 Subject: [PATCH 2/7] fix: keep a rescheduled send from going out twice or not at all when the server's answer is lost --- src/api/__tests__/email-submission.test.ts | 90 +++++++++++++++++++ .../__tests__/live-stalwart-compose.test.ts | 63 ++++++++++++- src/api/email.ts | 71 +++++++++++++-- 3 files changed, 214 insertions(+), 10 deletions(-) diff --git a/src/api/__tests__/email-submission.test.ts b/src/api/__tests__/email-submission.test.ts index e148012a..f7caea65 100644 --- a/src/api/__tests__/email-submission.test.ts +++ b/src/api/__tests__/email-submission.test.ts @@ -218,6 +218,96 @@ describe('rescheduleScheduledSend', () => { vi.mocked(Date.now).mockRestore(); }); + describe('answers that never came', () => { + const status = (undoStatus: string) => ({ + methodResponses: [['EmailSubmission/get', { list: [{ id: 'sub-1', undoStatus }] }, '0']], + }); + const records = (list: Array>) => ({ + methodResponses: [ + ['EmailSubmission/query', { ids: list.map((s) => s.id) }, '0'], + ['EmailSubmission/get', { list }, '1'], + ], + }); + const creates = () => setCalls().filter(([created]) => created.length > 0).length; + + it('goes on to the replacement when the cancel went through but its answer was lost', async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockRejectedValueOnce(new Error('Request timed out')) + .mockResolvedValueOnce(status('canceled')) + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockResolvedValueOnce({ methodResponses: REPLACED }); + + const result = await rescheduleScheduledSend(SCHEDULED, 0); + + expect(result.emailSubmissionId).toBe('sub-2'); + // The canceled record goes, as after a confirmed cancel. + expect(setCalls()).toEqual([[[], ['sub-1'], []], [[], [], ['sub-1']], [['replacement'], [], []]]); + }); + + it('still makes no replacement when the server refused the cancel and the original was cancelled elsewhere', async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce(set({ notUpdated: { 'sub-1': { type: 'invalidProperties' } } })) + .mockResolvedValueOnce(status('canceled')); + + const err = await rescheduleScheduledSend(SCHEDULED, 0).catch((e: unknown) => e); + + expect(err).toBeInstanceOf(NoLongerScheduledError); + expect(creates()).toBe(0); + }); + + it('takes a replacement the server made for the answer that was lost, and does not put the original back', async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce(CANCELLED) + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockRejectedValueOnce(new Error('Request timed out')) + .mockResolvedValueOnce(records([ + { id: 'sub-1', emailId: 'e-1', undoStatus: 'canceled' }, + { id: 'sub-2', emailId: 'e-1', undoStatus: 'final', sendAt: '2026-09-25T08:00:00Z' }, + ])); + + const result = await rescheduleScheduledSend(SCHEDULED, 0); + + expect(result).toEqual({ emailSubmissionId: 'sub-2', sendAt: '2026-09-25T08:00:00Z' }); + expect(creates()).toBe(1); + const [query] = mockRequest.mock.calls.at(-1)![0]; + expect(query).toEqual(['EmailSubmission/query', { accountId: 'acc-1', filter: { emailIds: ['e-1'] } }, '0']); + }); + + it('puts the original back when the lost create left nothing on the server', async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce(CANCELLED) + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockRejectedValueOnce(new Error('Request timed out')) + .mockResolvedValueOnce(records([{ id: 'sub-1', emailId: 'e-1', undoStatus: 'canceled' }])) + .mockResolvedValueOnce(set({ created: { replacement: { id: 'sub-3' } } })); + + const err = await rescheduleScheduledSend(SCHEDULED, 7200).catch((e: unknown) => e); + + expect(err).toBeInstanceOf(RescheduleRestoredError); + expect(err).toMatchObject({ restoredSubmissionId: 'sub-3' }); + }); + + it('counts a put-back the server made for the answer that was lost as restored, and leaves the message in Sent', async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce(CANCELLED) + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockResolvedValueOnce(REFUSED('Quota exceeded')) + .mockRejectedValueOnce(new Error('Network request failed')) + .mockResolvedValueOnce(records([{ id: 'sub-3', emailId: 'e-1', undoStatus: 'pending', sendAt: ORIGINAL_AT }])); + + const err = await rescheduleScheduledSend(SCHEDULED, 7200).catch((e: unknown) => e); + + expect(err).toBeInstanceOf(RescheduleRestoredError); + expect(err).toMatchObject({ restoredSubmissionId: 'sub-3', restoredSendAt: ORIGINAL_AT }); + expect(mockRequest.mock.calls.some((c) => c[0][0][0] === 'Email/set')).toBe(false); + }); + }); + // A group send's undo entry carried the login's own identity id, which // names nothing in the group account: both creates were refused and the // message went back to Drafts unsent. diff --git a/src/api/__tests__/live-stalwart-compose.test.ts b/src/api/__tests__/live-stalwart-compose.test.ts index a8ba70d3..5b2b922e 100644 --- a/src/api/__tests__/live-stalwart-compose.test.ts +++ b/src/api/__tests__/live-stalwart-compose.test.ts @@ -22,7 +22,7 @@ import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; import { JMAPClient } from '../jmap-client'; import { - destroyEmails, getFullEmails, getMailboxes, listScheduledEmails, rescheduleScheduledSend, saveDraftVersion, sendEmail, + cancelScheduledSend, destroyEmails, getFullEmails, getMailboxes, listScheduledEmails, rescheduleScheduledSend, saveDraftVersion, sendEmail, } from '../email'; import { uploadBytes } from '../blob'; import { deleteFileNodes } from '../files'; @@ -69,6 +69,11 @@ live('live Stalwart: composer and sending (0.16 and 1.0)', () => { }, 60_000); afterAll(async () => { + // A failed test may leave a send scheduled; it must not outlive its message. + const held = await listScheduledEmails().catch(() => []); + for (const s of held.filter((h) => createdEmails.includes(h.emailId))) { + await cancelScheduledSend(s.emailSubmissionId).catch(() => undefined); + } const res = await singleton.request([['Email/query', { accountId: singleton.accountId, filter: { subject: tag } }, '0']]); const leftovers = [...new Set([...(res.methodResponses[0][1].ids as string[] ?? []), ...createdEmails])]; if (leftovers.length) await destroyEmails(leftovers).catch(() => undefined); @@ -115,6 +120,62 @@ live('live Stalwart: composer and sending (0.16 and 1.0)', () => { }, 90_000); }); + describe('scheduled sends whose answers get lost', () => { + /** Let the first call matching `pick` reach the server, then lose its answer. */ + const loseAnswerOf = (pick: (method: string, args: Record) => boolean) => { + const real = singleton.request.bind(singleton); + let lost = false; + return vi.spyOn(singleton, 'request').mockImplementation(async (calls, using) => { + const res = await real(calls, using); + const [method, args] = calls[0] as unknown as [string, Record]; + if (!lost && pick(method, args)) { + lost = true; + throw new Error('Request timed out'); + } + return res; + }); + }; + const scheduleOne = async (label: string) => { + const [identity] = await getIdentities(); + const result = await sendEmail( + { from: [{ email: USER }], to: [{ email: USER }], subject: `${label} ${tag}`, textBody: 'live' }, + identity.id, sent, 3600, { draftsMailboxId: drafts }, + ); + createdEmails.push(result.emailId!); + return (await listScheduledEmails()).find((s) => s.emailSubmissionId === result.emailSubmissionId)!; + }; + const pendingOf = async (emailId: string) => (await listScheduledEmails()).filter((s) => s.emailId === emailId); + + it('makes the replacement when the cancel went through and its answer was lost', async () => { + const first = await scheduleOne('lost cancel'); + const spy = loseAnswerOf((method, args) => method === 'EmailSubmission/set' && !!args.update); + let moved: Awaited>; + try { + moved = await rescheduleScheduledSend(first, 7200); + } finally { + spy.mockRestore(); + } + const pending = await pendingOf(first.emailId); + expect(pending.map((s) => s.emailSubmissionId)).toEqual([moved.emailSubmissionId]); + await cancelScheduledToDrafts(pending[0], await getMailboxes(), singleton.accountId); + }, 90_000); + + it('takes the replacement the server made when the answer to its create was lost, and schedules it once', async () => { + const first = await scheduleOne('lost create'); + const spy = loseAnswerOf((method, args) => method === 'EmailSubmission/set' && !!args.create); + let moved: Awaited>; + try { + moved = await rescheduleScheduledSend(first, 7200); + } finally { + spy.mockRestore(); + } + const pending = await pendingOf(first.emailId); + expect(pending.map((s) => s.emailSubmissionId)).toEqual([moved.emailSubmissionId]); + expect(new Date(pending[0].sendAt).getTime()).toBeGreaterThan(new Date(first.sendAt).getTime() + 3_000_000); + await cancelScheduledToDrafts(pending[0], await getMailboxes(), singleton.accountId); + }, 90_000); + }); + describe('Files and mail share blobs', () => { it('saves an attachment to Files and attaches a file from Files, by blob', async () => { const upload = await uploadBytes(new TextEncoder().encode(`hello ${RUN}`), 'text/plain'); diff --git a/src/api/email.ts b/src/api/email.ts index f4c4ecce..48947f51 100644 --- a/src/api/email.ts +++ b/src/api/email.ts @@ -1,6 +1,6 @@ import { jmapClient } from './jmap-client'; import { - assertSetResult, batched, failedIdsOf, JMAPSetError, NoLongerScheduledError, parseHoldLimit, RecipientsRefusedError, + assertSetResult, batched, failedIdsOf, JMAPMethodError, JMAPSetError, NoLongerScheduledError, parseHoldLimit, RecipientsRefusedError, requireMethodResult, ScheduleTooLateError, type RefusedRecipient, } from './jmap-result'; import { keywordPointer, mailboxPointer } from './patch-pointer'; @@ -2146,7 +2146,8 @@ export async function cancelScheduledSend(emailSubmissionId: string, accountIdOv | { type?: string; description?: string } | undefined; if (failure) { - throw new Error(failure.description ?? failure.type ?? 'Failed to cancel scheduled send'); + // A SetError: the server answered and kept the submission as it was. + throw new JMAPSetError(failure.type ?? 'setError', failure.description ?? failure.type ?? 'Failed to cancel scheduled send'); } // The canceled record is dead weight against Stalwart 1.0's submission // limit. It goes in a request of its own, and only once the cancel is @@ -2201,9 +2202,10 @@ export class RescheduleLostError extends Error { * out already (NoLongerScheduledError). A replacement the server refuses, or * takes for no recipient, puts the original back at its old time under a new * id (RescheduleRestoredError); when that fails too the message goes back to - * Drafts (RescheduleLostError). Returns the new submission id and send time, - * and the recipients the server refused for the replacement when it took - * the others. + * Drafts (RescheduleLostError). A cancel or create whose answer was lost is + * checked against the server's records first. Returns the new submission id + * and send time, and the recipients the server refused for the replacement + * when it took the others. */ export async function rescheduleScheduledSend( scheduled: { @@ -2263,6 +2265,9 @@ export async function rescheduleScheduledSend( const sentId = roles.find((m) => m.role === 'sent')?.id; type Created = { id: string; sendAt?: string; delivery: { recipients: RefusedRecipient[]; all: boolean } }; + // Creates that got no answer (timeout, dropped connection): the server + // may have taken them all the same. + const unanswered = new Set(); // `until`: when the submission goes out (ms); undefined, at once. const createSubmission = async (until: number | undefined): Promise => { // HOLDFOR counts from when the server takes the request, so a retry @@ -2295,7 +2300,10 @@ export async function rescheduleScheduledSend( deliveryStatusCall(accountId, 'replacement', '1'), ], SUBMISSION_USING, - ); + ).catch((err: unknown) => { + unanswered.add(err); + throw err; + }); }; let res = await send(); // A full submission quota (Stalwart 1.0): make room and try once more. @@ -2336,8 +2344,17 @@ export async function rescheduleScheduledSend( await cancelScheduledSend(scheduled.emailSubmissionId, accountId); } catch (err) { const status = await submissionUndoStatus(scheduled.emailSubmissionId, accountId); - if (status === 'final' || status === 'canceled') throw new NoLongerScheduledError(status === 'final'); - throw err; + // A cancel the server answered and refused changed nothing, so a + // canceled original was cancelled elsewhere. One whose answer was lost + // (timeout, dropped connection) may have gone through: then the + // original is cancelled by this call and the replacement still has to + // be made, or the message sits in Sent unsent. + const refusedCancel = err instanceof JMAPMethodError; + if (status === 'final' || (status === 'canceled' && refusedCancel)) { + throw new NoLongerScheduledError(status === 'final'); + } + if (status !== 'canceled') throw err; + await destroyEmailSubmissions([scheduled.emailSubmissionId], accountId); } let replacement: Created; @@ -2350,6 +2367,13 @@ export async function rescheduleScheduledSend( const refused = await withdrawIfDead(replacement); if (refused) throw new RecipientsRefusedError(refused); } catch (err) { + // A replacement whose answer was lost may have been taken: putting the + // original back as well would send the message twice. The server's + // record of it stands in for the answer. + if (unanswered.has(err)) { + const taken = await liveSubmissionOf(scheduled.emailId, accountId); + if (taken) return { emailSubmissionId: taken.id, sendAt: taken.sendAt }; + } // Put the original back at its old time: HOLDFOR with the seconds left, // at once when that time has passed meanwhile. const originalTime = original?.sendAt ? Date.parse(original.sendAt) : NaN; @@ -2357,7 +2381,9 @@ export async function rescheduleScheduledSend( try { restored = await createSubmission(Number.isFinite(originalTime) && originalTime > Date.now() ? originalTime : undefined); } catch (restoreErr) { - console.warn('[email] could not put the original scheduled send back:', restoreErr); + const taken = unanswered.has(restoreErr) ? await liveSubmissionOf(scheduled.emailId, accountId) : undefined; + if (taken) restored = { id: taken.id, sendAt: taken.sendAt, delivery: { recipients: [], all: false } }; + else console.warn('[email] could not put the original scheduled send back:', restoreErr); } if (restored && !(await withdrawIfDead(restored).catch(() => null))) { throw new RescheduleRestoredError(err, restored.id, restored.sendAt ?? original?.sendAt); @@ -2396,6 +2422,33 @@ async function readSubmissionUndoStatus(emailSubmissionId: string, accountId: st return list?.find((s) => s.id === emailSubmissionId)?.undoStatus; } +/** + * A submission of `emailId` that is still held or went out, for a create + * whose answer was lost. Undefined when there is none or it can't be read. + */ +async function liveSubmissionOf(emailId: string, accountId: string): Promise<{ id: string; sendAt?: string } | undefined> { + try { + const res = await jmapClient.request( + [ + ['EmailSubmission/query', { accountId, filter: { emailIds: [emailId] } }, '0'], + ['EmailSubmission/get', { + accountId, + '#ids': { resultOf: '0', name: 'EmailSubmission/query', path: '/ids' }, + properties: ['id', 'emailId', 'undoStatus', 'sendAt'], + }, '1'], + ], + SUBMISSION_ONLY_USING, + ); + const list = requireMethodResult(res, '1', 'EmailSubmission/get').list as + Array<{ id?: string; emailId?: string; undoStatus?: string; sendAt?: string }> | undefined; + // "Send now" goes out a second later: a final one is the replacement too. + const live = list?.find((s) => s.id && s.emailId === emailId && (s.undoStatus === 'pending' || s.undoStatus === 'final')); + return live?.id ? { id: live.id, sendAt: live.sendAt } : undefined; + } catch { + return undefined; + } +} + /** A submission's `undoStatus`, or undefined when it can't be read. */ async function submissionUndoStatus(emailSubmissionId: string, accountId: string): Promise { try { From c8600e859a2569ed24e21ab2d6fd811a25d8bdbe Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 04:49:31 +0100 Subject: [PATCH 3/7] fix: attach a quoted inline image under a plain image type, not the part's raw header --- src/lib/__tests__/compose-inline-type.test.ts | 31 +++++++++++++++++++ src/lib/compose-inline-type.ts | 17 ++++++++++ src/screens/ComposeScreen.tsx | 5 ++- 3 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 src/lib/__tests__/compose-inline-type.test.ts create mode 100644 src/lib/compose-inline-type.ts diff --git a/src/lib/__tests__/compose-inline-type.test.ts b/src/lib/__tests__/compose-inline-type.test.ts new file mode 100644 index 00000000..e4a510a7 --- /dev/null +++ b/src/lib/__tests__/compose-inline-type.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from 'vitest'; +import fs from 'node:fs'; +import path from 'node:path'; +import { inlineImageType } from '../compose-inline-type'; + +describe('inlineImageType', () => { + it('keeps a plain image type, without its parameters', () => { + expect(inlineImageType('image/jpeg', null)).toBe('image/jpeg'); + expect(inlineImageType('Image/JPEG; name="a.jpg"', null)).toBe('image/jpeg'); + expect(inlineImageType('image/svg+xml', 'image/png')).toBe('image/svg+xml'); + }); + + it('drops a hostile declared type for the sniffed one', () => { + expect(inlineImageType('image/x">', 'image/gif')).toBe('image/gif'); + expect(inlineImageType('image/png"onload="x', null)).toBe('image/png'); + expect(inlineImageType('image/', 'image/webp')).toBe('image/webp'); + }); + + it('falls back to the sniffed type, then PNG, for a part that is not labelled an image', () => { + expect(inlineImageType('application/octet-stream', 'image/jpeg')).toBe('image/jpeg'); + expect(inlineImageType(undefined, null)).toBe('image/png'); + }); + + // The composer took the part's type as it came for the data URI, the + // re-upload and the part sent with the reply. + it('is what the composer uses for a hydrated inline image', () => { + const source = fs.readFileSync(path.resolve(__dirname, '../../screens/ComposeScreen.tsx'), 'utf8'); + expect(source).toMatch(/const mime = inlineImageType\(att\.type, sniffImageMime\(bytes\)\);/); + expect(source).not.toMatch(/startsWith\('image\/'\) \? att\.type/); + }); +}); diff --git a/src/lib/compose-inline-type.ts b/src/lib/compose-inline-type.ts new file mode 100644 index 00000000..0af54c49 --- /dev/null +++ b/src/lib/compose-inline-type.ts @@ -0,0 +1,17 @@ +// The media type of a quoted original's inline image, as the composer shows +// it (a data: URI) and attaches it again on send. The server hands the part's +// Content-Type over verbatim, parameters, quotes and `>` included, so only a +// plain `image/` is taken from it (the rule of lib/email-html's +// inline image check); otherwise the bytes decide. + +const PLAIN_IMAGE_TYPE = /^image\/[a-z0-9.+-]+$/; + +/** + * `declared` without parameters when it is a plain image type ("image/jpeg; + * name=x" is "image/jpeg"), else `sniffed`, else image/png. + */ +export function inlineImageType(declared: string | null | undefined, sniffed: string | null | undefined): string { + const essence = (declared ?? '').toLowerCase().split(';')[0].trim(); + if (PLAIN_IMAGE_TYPE.test(essence)) return essence; + return sniffed ?? 'image/png'; +} diff --git a/src/screens/ComposeScreen.tsx b/src/screens/ComposeScreen.tsx index ab038b05..ceb25ef1 100644 --- a/src/screens/ComposeScreen.tsx +++ b/src/screens/ComposeScreen.tsx @@ -68,6 +68,7 @@ import { import { shouldBlockEditorRemoteImages } from '../lib/editor-html'; import { placeholderWordsFor, unfilledPlaceholdersInMessage } from '../lib/placeholder-check'; import { sendTargetFor } from '../lib/compose-send-account'; +import { inlineImageType } from '../lib/compose-inline-type'; import { attachmentFromFileNode } from '../lib/compose-files'; import { supportsFiles } from '../api/files'; import { FilesPickerSheet } from '../components/compose/FilesPickerSheet'; @@ -862,7 +863,9 @@ export default function ComposeScreen({ route, navigation }: Props) { try { const buf = await jmapClient.fetchBlobArrayBuffer(att.blobId, att.name, att.type, seedOwnerAccountId); const bytes = new Uint8Array(buf); - const mime = att.type?.toLowerCase().startsWith('image/') ? att.type : (sniffImageMime(bytes) ?? 'image/png'); + // Never the part's type as it came: it goes into the data URI and + // into the part sent with the reply. + const mime = inlineImageType(att.type, sniffImageMime(bytes)); map.set(cid, `data:${mime};base64,${bytesToBase64(bytes)}`); let blobId = att.blobId; if (seedOwnerAccountId) blobId = (await uploadBytesForLogin(ownerLoginRef.current, bytes, mime)).blobId; From 39e82f99e4c8e98a920be842a8f93b7a026e6e01 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 04:54:11 +0100 Subject: [PATCH 4/7] fix: save the attachment itself to Files on Stalwart 0.16, under a name the folder has free --- .../__tests__/live-stalwart-compose.test.ts | 45 +++++++++++++++++ src/api/files.ts | 11 +++++ src/lib/__tests__/compose-files.test.ts | 49 ++++++++++++++++++- src/lib/compose-files.ts | 44 +++++++++++++---- 4 files changed, 137 insertions(+), 12 deletions(-) diff --git a/src/api/__tests__/live-stalwart-compose.test.ts b/src/api/__tests__/live-stalwart-compose.test.ts index 5b2b922e..123f26e3 100644 --- a/src/api/__tests__/live-stalwart-compose.test.ts +++ b/src/api/__tests__/live-stalwart-compose.test.ts @@ -199,6 +199,51 @@ live('live Stalwart: composer and sending (0.16 and 1.0)', () => { expect(saved.attachments.map((a) => a.name)).toContain('from-files.txt'); }, 60_000); + const text = (buf: ArrayBuffer) => new TextDecoder().decode(new Uint8Array(buf)); + const nodeBlob = async (id: string) => { + const res = await singleton.request( + [['FileNode/get', { accountId: singleton.accountId, ids: [id] }, '0']], + [CAPABILITIES.CORE, CAPABILITIES.FILES], + ); + return res.methodResponses[0][1].list[0] as { blobId: string; name: string }; + }; + + // 0.16 made a FileNode from a part's blobId hold the whole raw message. + it("files a message's attachment part as the attachment's bytes, under a free name, and attaches it back", async () => { + const content = `part ${RUN}`; + const upload = await uploadBytes(new TextEncoder().encode(content), 'text/plain'); + const draft = await saveDraftVersion({ + from: [{ email: USER }], to: [{ email: USER }], subject: `part ${tag}`, textBody: 'see attached', + attachments: [{ blobId: upload.blobId, name: 'part.txt', type: 'text/plain', disposition: 'attachment' }], + }, drafts); + createdEmails.push(draft.id); + const [full] = await getFullEmails([draft.id]); + const part = full.attachments!.find((a) => a.name === 'part.txt')!; + expect(part.blobId).toBeTruthy(); + + const name = `${tag}-part.txt`; + const first = await saveAttachmentToFiles({ blobId: part.blobId!, name, type: 'text/plain' }, null); + createdNodes.push(first.id); + const stored = await nodeBlob(first.id); + expect(text(await singleton.fetchBlobArrayBuffer(stored.blobId, name, 'text/plain'))).toBe(content); + + // The same name again: the server would refuse it (alreadyExists). + const second = await saveAttachmentToFiles({ blobId: part.blobId!, name, type: 'text/plain' }, null); + createdNodes.push(second.id); + expect(second.name).toBe(`${tag}-part (1).txt`); + + // And back: the file attached to a new draft carries the same bytes. + const back = await attachmentFromFileNode({ ...stored, id: first.id, nodeType: 'file', type: 'text/plain' }); + const reply = await saveDraftVersion({ + from: [{ email: USER }], to: [{ email: USER }], subject: `back ${tag}`, textBody: 'see attached', + attachments: [{ blobId: back.blobId, name: 'back.txt', type: 'text/plain', disposition: 'attachment' }], + }, drafts); + createdEmails.push(reply.id); + const [again] = await getFullEmails([reply.id]); + const attached = again.attachments!.find((a) => a.name === 'back.txt')!; + expect(text(await singleton.fetchBlobArrayBuffer(attached.blobId!, 'back.txt', 'text/plain'))).toBe(content); + }, 60_000); + it('copies blobs with Blob/copy only when the blob capability is in `using`', async () => { const upload = await uploadBytes(new TextEncoder().encode(`copy ${RUN}`), 'text/plain'); const call = ['Blob/copy', { fromAccountId: singleton.accountId, accountId: singleton.accountId, blobIds: [upload.blobId] }, '0']; diff --git a/src/api/files.ts b/src/api/files.ts index d618f601..8907d2e2 100644 --- a/src/api/files.ts +++ b/src/api/files.ts @@ -426,6 +426,17 @@ function usesDraftFileNodeCreate(accountId: string): boolean { return !!cap && typeof cap === 'object' && 'supportedNodeTypes' in cap; } +/** + * Whether a FileNode made from a message part's blobId holds that part. + * Stalwart 1.0.0 stores the part; 0.16 stores the whole raw message the part + * is in under the part's name (seen on 0.16.27), so there the part's bytes + * have to be uploaded as a blob of their own first. Told apart as for the + * create shape: only 1.0.0 implements File Storage draft 18. + */ +export function fileNodeTakesPartBlobs(accountId: string = filesAccountId()): boolean { + return usesDraftFileNodeCreate(accountId); +} + /** One FileNode/set create in `accountId`, in the shape that server takes. */ async function createFileNodeIn( accountId: string, diff --git a/src/lib/__tests__/compose-files.test.ts b/src/lib/__tests__/compose-files.test.ts index 07010c62..11c060ca 100644 --- a/src/lib/__tests__/compose-files.test.ts +++ b/src/lib/__tests__/compose-files.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -const { files, email } = vi.hoisted(() => ({ +const { files, email, blob, client } = vi.hoisted(() => ({ files: { filesAccountId: vi.fn(() => 'me'), copyFileNode: vi.fn(async (node: { blobId: string }, parentId: string | null, name: string) => ({ @@ -8,16 +8,30 @@ const { files, email } = vi.hoisted(() => ({ })), fileNodeKind: vi.fn((node: { nodeType?: string; blobId?: string | null }) => node.nodeType ?? (node.blobId == null ? 'directory' : 'file')), + // Stalwart 1.0.0 unless a test says otherwise. + fileNodeTakesPartBlobs: vi.fn(() => true), + getAllFileNodes: vi.fn(async (): Promise>> => []), + getFileNameRules: vi.fn(() => null), }, email: { // As the real one: the ids as they are within one account. copyBlobs: vi.fn(async (from: string, ids: string[], to: string) => Object.fromEntries(ids.map((id) => [id, from === to ? id : `copy-of-${id}`]))), }, + blob: { + uploadBytes: vi.fn(async (_bytes: Uint8Array, type: string, accountId?: string) => ({ + blobId: `uploaded-to-${accountId}`, size: 3, type, + })), + }, + client: { + accountId: 'me', + fetchBlobArrayBuffer: vi.fn(async () => new Uint8Array([1, 2, 3]).buffer), + }, })); vi.mock('../../api/files', () => files); vi.mock('../../api/email', () => email); -vi.mock('../../api/jmap-client', () => ({ jmapClient: { accountId: 'me' } })); +vi.mock('../../api/blob', () => blob); +vi.mock('../../api/jmap-client', () => ({ jmapClient: client })); import type { FileNode } from '../../api/types'; import { @@ -92,4 +106,35 @@ describe('Save to Files', () => { expect.objectContaining({ blobId: 'copy-of-att-1' }), null, 'invoice.pdf', ); }); + + // 0.16 filed the whole raw message under the attachment's name. + it("uploads the attachment's bytes into the Files account on Stalwart 0.16", async () => { + files.fileNodeTakesPartBlobs.mockReturnValue(false); + try { + await saveAttachmentToFiles({ blobId: 'att-1', name: 'invoice.pdf', type: 'application/pdf' }, 'd1', 'team'); + } finally { + files.fileNodeTakesPartBlobs.mockReturnValue(true); + } + expect(client.fetchBlobArrayBuffer).toHaveBeenCalledWith('att-1', 'invoice.pdf', 'application/pdf', 'team'); + expect(blob.uploadBytes).toHaveBeenCalledWith(new Uint8Array([1, 2, 3]), 'application/pdf', 'me'); + expect(email.copyBlobs).not.toHaveBeenCalled(); + expect(files.copyFileNode).toHaveBeenCalledWith( + expect.objectContaining({ blobId: 'uploaded-to-me' }), 'd1', 'invoice.pdf', + ); + }); + + it('numbers the name when the folder holds one like it', async () => { + files.getAllFileNodes.mockResolvedValueOnce([ + { id: 'x', name: 'invoice.pdf', parentId: 'd1' }, + { id: 'y', name: 'invoice (1).pdf', parentId: 'd1' }, + { id: 'z', name: 'other.pdf', parentId: null }, + { id: 's', name: 'other (1).pdf', parentId: null, isShared: true }, + ]); + await saveAttachmentToFiles({ blobId: 'att-1', name: 'invoice.pdf', type: 'application/pdf' }, 'd1'); + expect(files.copyFileNode).toHaveBeenLastCalledWith(expect.anything(), 'd1', 'invoice (2).pdf'); + + files.getAllFileNodes.mockResolvedValueOnce([{ id: 'x', name: 'invoice.pdf', parentId: 'd1' }]); + await saveAttachmentToFiles({ blobId: 'att-1', name: 'invoice.pdf', type: 'application/pdf' }, null); + expect(files.copyFileNode).toHaveBeenLastCalledWith(expect.anything(), null, 'invoice.pdf'); + }); }); diff --git a/src/lib/compose-files.ts b/src/lib/compose-files.ts index bd56ad95..52649d98 100644 --- a/src/lib/compose-files.ts +++ b/src/lib/compose-files.ts @@ -1,14 +1,21 @@ // Mail and Files meet here: a file from Files attached to a message (#1179) -// and an attachment filed in Files (webmail save-to-files-dialog). Neither -// downloads or uploads anything: both refer to a blob the server already -// has. Stalwart 0.16.27 and 1.0.0 take a FileNode's blob as an attachment -// and an attachment's blob for a FileNode within one account; across -// accounts the blob is copied first (Blob/copy), as the webmail does. +// and an attachment filed in Files (webmail save-to-files-dialog). Both +// refer to a blob the server already has where they can. Stalwart 0.16.27 +// and 1.0.0 take a FileNode's blob as an attachment, and 1.0.0 an +// attachment's blob for a FileNode; across accounts the blob is copied first +// (Blob/copy), as the webmail does. On 0.16 a FileNode made from an +// attachment's blob holds the whole message, so there the attachment is +// downloaded and uploaded again. +import { uploadBytes } from '../api/blob'; import { copyBlobs } from '../api/email'; -import { copyFileNode, fileNodeKind, filesAccountId } from '../api/files'; +import { + copyFileNode, fileNodeKind, fileNodeTakesPartBlobs, filesAccountId, getAllFileNodes, getFileNameRules, +} from '../api/files'; import { jmapClient } from '../api/jmap-client'; import type { FileNode } from '../api/types'; +import { acceptedFileName } from './file-name-rules'; +import { getUniqueName } from './filenode-name'; /** The Files trash the webmail keeps (lib/files-trash): hidden like any dot folder. */ const TRASH_FOLDER_NAME = '.Trash'; @@ -73,7 +80,9 @@ export async function attachmentFromFileNode( * File an attachment in the user's Files, in `parentId` (null: My files). * `blobAccountId` is the account the message is in when it is not the * login's own: a shared or group mailbox's blob is copied into the Files - * account first. + * account first. On Stalwart 0.16 the attachment's bytes are uploaded into + * the Files account instead (see fileNodeTakesPartBlobs). A name the folder + * has already gets a number. */ export async function saveAttachmentToFiles( attachment: { blobId: string; name?: string | null; type?: string | null }, @@ -81,8 +90,23 @@ export async function saveAttachmentToFiles( blobAccountId?: string, ): Promise { const from = blobAccountId ?? jmapClient.accountId; - const blobId = (await copyBlobs(from, [attachment.blobId], filesAccountId()))[attachment.blobId]; - const name = attachment.name || 'attachment'; - const node: FileNode = { id: '', name, type: attachment.type || 'application/octet-stream', blobId, nodeType: 'file' }; + const to = filesAccountId(); + const type = attachment.type || 'application/octet-stream'; + // A name the folder holds already is refused (alreadyExists): the copy + // gets " (1)", " (2)", ... as an upload does. + const taken = new Set( + (await getAllFileNodes()) + .filter((n) => !n.isShared && (n.parentId ?? null) === parentId) + .map((n) => n.name), + ); + const name = getUniqueName(acceptedFileName(attachment.name || 'attachment', getFileNameRules()), taken); + let blobId: string; + if (fileNodeTakesPartBlobs(to)) { + blobId = (await copyBlobs(from, [attachment.blobId], to))[attachment.blobId]; + } else { + const bytes = await jmapClient.fetchBlobArrayBuffer(attachment.blobId, name, type, from); + blobId = (await uploadBytes(new Uint8Array(bytes), type, to)).blobId; + } + const node: FileNode = { id: '', name, type, blobId, nodeType: 'file' }; return copyFileNode(node, parentId, name); } From 7594e45937f23f0f8264231239a466fa11a857f7 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 04:55:40 +0100 Subject: [PATCH 5/7] fix: stop the placeholder check from flagging text inside link addresses --- src/lib/__tests__/placeholder-check.test.ts | 10 ++++++++++ src/lib/placeholder-check.ts | 8 +++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/lib/__tests__/placeholder-check.test.ts b/src/lib/__tests__/placeholder-check.test.ts index 32ff0515..ae2d4175 100644 --- a/src/lib/__tests__/placeholder-check.test.ts +++ b/src/lib/__tests__/placeholder-check.test.ts @@ -181,4 +181,14 @@ describe('unfilledPlaceholdersInMessage', () => { subject: '{{subject}}', body, plainTextMode: true, signature, words: EN, })).toEqual(['{{subject}}', '[TIME]']); }); + + // The plain text of a link is "text ": a URL's {{id}} or [TODO] + // stopped the send though the sender never sees it. + it('leaves link targets out and keeps the link text', () => { + const body = '

See the [DRAFT] page' + + " and this.

"; + expect(unfilledPlaceholdersInMessage({ + subject: 'Hello', body, plainTextMode: false, signature, words: EN, + })).toEqual(['[DRAFT]']); + }); }); diff --git a/src/lib/placeholder-check.ts b/src/lib/placeholder-check.ts index 33e19c67..9dec23e2 100644 --- a/src/lib/placeholder-check.ts +++ b/src/lib/placeholder-check.ts @@ -143,6 +143,9 @@ export function placeholderWordsFor(locale: string): PlaceholderWords[] { return lists; } +/** The href of an `` tag, with the tag up to it in $1. */ +const LINK_HREF = /(]*?)\shref\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>]+)/gi; + /** * The unfilled placeholders of a message as the composer holds it: the body * without its signature (a placeholder there would stop every message) and @@ -161,7 +164,10 @@ export function unfilledPlaceholdersInMessage(message: { const signature = getPlainTextSignature(message.signature ?? undefined); authored = signature ? message.body.split(signature).join('') : message.body; } else { - authored = stripEmbeddedSignature(message.body); + // Without the link targets: the plain text writes a link as + // "text ", and a URL is no fill-in the sender left ("{{id}}" in a + // tracking link, "[TODO]" in a wiki path). + authored = stripEmbeddedSignature(message.body).replace(LINK_HREF, '$1'); } const bodyText = extractUserAuthoredText(authored, { plainTextMode: message.plainTextMode, From 51b3208b3bf5f5a97b095a3e1baf7ed901705693 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 04:59:32 +0100 Subject: [PATCH 6/7] fix: drop a cancelled scheduled send from the list when it can't go back to Drafts, and say it stayed in Sent --- locales/rn/en.json | 2 + .../__tests__/live-stalwart-compose.test.ts | 4 +- src/lib/__tests__/compose-scheduled.test.ts | 13 +++++- .../__tests__/scheduled-screen-wiring.test.ts | 26 ++++++++++++ src/lib/compose-scheduled.ts | 40 +++++++++++++------ src/screens/ScheduledScreen.tsx | 21 +++++++++- 6 files changed, 88 insertions(+), 18 deletions(-) create mode 100644 src/lib/__tests__/scheduled-screen-wiring.test.ts diff --git a/locales/rn/en.json b/locales/rn/en.json index 679fe44d..b5ad94de 100644 --- a/locales/rn/en.json +++ b/locales/rn/en.json @@ -758,6 +758,8 @@ "cancel_body": "The message will not be delivered. A copy stays in your Sent folder.", "cancel_failed": "Cancel failed", "cancel_to_drafts_body": "The message will not be sent. It goes back to your Drafts.", + "cancelled_not_moved": "Send cancelled, but not moved to Drafts", + "cancelled_not_moved_body": "The message will not be sent. It is still in Sent.", "edit_failed": "Could not open the message for editing", "keep_scheduled": "Keep scheduled", "no_longer_scheduled": "This message is no longer scheduled.", diff --git a/src/api/__tests__/live-stalwart-compose.test.ts b/src/api/__tests__/live-stalwart-compose.test.ts index 123f26e3..e916e217 100644 --- a/src/api/__tests__/live-stalwart-compose.test.ts +++ b/src/api/__tests__/live-stalwart-compose.test.ts @@ -110,8 +110,8 @@ live('live Stalwart: composer and sending (0.16 and 1.0)', () => { expect(filed.mailboxIds[sent]).toBe(true); // Cancel send: nothing goes out, and the message is a draft again. - const inDrafts = await cancelScheduledToDrafts(second, await getMailboxes(), singleton.accountId); - expect(inDrafts).toBe(true); + const cancelled = await cancelScheduledToDrafts(second, await getMailboxes(), singleton.accountId); + expect(cancelled).toEqual({ inDrafts: true }); expect((await listScheduledEmails()).some((s) => s.emailId === result.emailId)).toBe(false); const [restored] = await getFullEmails([result.emailId!]); expect(restored.mailboxIds[drafts]).toBe(true); diff --git a/src/lib/__tests__/compose-scheduled.test.ts b/src/lib/__tests__/compose-scheduled.test.ts index 99b0d206..28cc24ee 100644 --- a/src/lib/__tests__/compose-scheduled.test.ts +++ b/src/lib/__tests__/compose-scheduled.test.ts @@ -36,7 +36,7 @@ beforeEach(() => vi.clearAllMocks()); describe('Cancel send of a scheduled message', () => { it('cancels the submission and moves an own message back into the own Drafts', async () => { const item = { accountId: 'own', emailSubmissionId: 'sub-1', emailId: 'mail-1' }; - await expect(cancelScheduledToDrafts(item, MAILBOXES, 'own')).resolves.toBe(true); + await expect(cancelScheduledToDrafts(item, MAILBOXES, 'own')).resolves.toEqual({ inDrafts: true }); expect(api.cancelScheduledSend).toHaveBeenCalledWith('sub-1', 'own'); expect(api.restoreEmailToDraft).toHaveBeenCalledWith('mail-1', 'own-drafts', 'own-sent', 'own'); }); @@ -57,10 +57,19 @@ describe('Cancel send of a scheduled message', () => { it('says so when the account has no Drafts folder', async () => { const item = { accountId: 'own', emailSubmissionId: 'sub-1', emailId: 'mail-1' }; - await expect(cancelScheduledToDrafts(item, [mailbox('sent', 'own-sent')], 'own')).resolves.toBe(false); + await expect(cancelScheduledToDrafts(item, [mailbox('sent', 'own-sent')], 'own')).resolves.toEqual({ inDrafts: false }); expect(api.restoreEmailToDraft).not.toHaveBeenCalled(); }); + // The cancel went through, so it must not read as "Cancel failed" with the + // row still listed as scheduled. + it('reports a cancelled send it could not move back to Drafts, without failing', async () => { + const moveError = new Error('Network request failed'); + api.restoreEmailToDraft.mockRejectedValueOnce(moveError); + const item = { accountId: 'own', emailSubmissionId: 'sub-1', emailId: 'mail-1' }; + await expect(cancelScheduledToDrafts(item, MAILBOXES, 'own')).resolves.toEqual({ inDrafts: false, moveError }); + }); + it('names the own account as no override', () => { expect(scheduledAccountOverride({ accountId: 'own' }, 'own')).toBeUndefined(); expect(scheduledAccountOverride({ accountId: 'team' }, 'own')).toBe('team'); diff --git a/src/lib/__tests__/scheduled-screen-wiring.test.ts b/src/lib/__tests__/scheduled-screen-wiring.test.ts new file mode 100644 index 00000000..9b1c8887 --- /dev/null +++ b/src/lib/__tests__/scheduled-screen-wiring.test.ts @@ -0,0 +1,26 @@ +import { describe, expect, it } from 'vitest'; +import fs from 'node:fs'; +import path from 'node:path'; + +// The Scheduled screen is a component the node test environment can't +// render; these check its wiring to the helpers tested elsewhere. +const source = fs.readFileSync(path.resolve(__dirname, '../../screens/ScheduledScreen.tsx'), 'utf8'); +const body = (name: string) => { + const start = source.indexOf(`const ${name} = `); + expect(start).toBeGreaterThan(-1); + return source.slice(start, source.indexOf('\n };', start)); +}; + +describe('Scheduled screen', () => { + // A cancel that went through, with the move to Drafts failing after it, + // read "Cancel failed" and left the row listed as scheduled. + it('drops the row of a send cancelled but not moved to Drafts, and says so', () => { + for (const name of ['onCancel', 'onEdit']) { + const handler = body(name); + expect(handler).toMatch(/\{ (inDrafts, )?moveError \} = await cancelScheduledToDrafts\(/); + const handled = handler.slice(handler.indexOf('cancelScheduledToDrafts('), handler.indexOf('scheduled.cancelled_not_moved\'')); + expect(handled).toMatch(/removeItem\(item\);/); + expect(handled).toMatch(/if \(moveError\) \{/); + } + }); +}); diff --git a/src/lib/compose-scheduled.ts b/src/lib/compose-scheduled.ts index 3d815f06..2dada447 100644 --- a/src/lib/compose-scheduled.ts +++ b/src/lib/compose-scheduled.ts @@ -12,26 +12,42 @@ export function scheduledAccountOverride(item: Pick return item.accountId !== ownAccountId ? item.accountId : undefined; } +/** Where a cancelled scheduled message is now. */ +export interface CancelledToDrafts { + /** Back in Drafts. False: still in Sent, but no longer scheduled. */ + inDrafts: boolean; + /** + * Why the move to Drafts failed after the cancel went through. The send is + * cancelled all the same: the row goes, and the user hears it stayed in + * Sent rather than "Cancel failed". + */ + moveError?: unknown; +} + /** - * Cancel the scheduled send and move the message back into Drafts. Resolves - * with whether it is in Drafts now: false when the account has no Drafts - * folder, where it stays in Sent. A failed cancel throws and moves nothing. + * Cancel the scheduled send and move the message back into Drafts. It stays + * in Sent when the account has no Drafts folder or the move fails (see + * CancelledToDrafts). A failed cancel throws and moves nothing. */ export async function cancelScheduledToDrafts( item: Pick, mailboxes: Mailbox[], ownAccountId: string, -): Promise { +): Promise { await cancelScheduledSend(item.emailSubmissionId, item.accountId); const scope = mailboxesOfAccount(mailboxes, scheduledAccountOverride(item, ownAccountId)); const drafts = scope.find((m) => m.role === 'drafts'); const sent = scope.find((m) => m.role === 'sent'); - if (!drafts) return false; - await restoreEmailToDraft( - item.emailId, - drafts.originalId ?? drafts.id, - sent ? (sent.originalId ?? sent.id) : undefined, - item.accountId, - ); - return true; + if (!drafts) return { inDrafts: false }; + try { + await restoreEmailToDraft( + item.emailId, + drafts.originalId ?? drafts.id, + sent ? (sent.originalId ?? sent.id) : undefined, + item.accountId, + ); + } catch (moveError) { + return { inDrafts: false, moveError }; + } + return { inDrafts: true }; } diff --git a/src/screens/ScheduledScreen.tsx b/src/screens/ScheduledScreen.tsx index a3f6c770..62c9bedf 100644 --- a/src/screens/ScheduledScreen.tsx +++ b/src/screens/ScheduledScreen.tsx @@ -194,9 +194,17 @@ export default function ScheduledScreen({ navigation }: Props) { style: 'destructive', onPress: () => { void runAction(item, async () => { - const inDrafts = await cancelScheduledToDrafts(item, mailboxes, jmapClient.accountId); + const { inDrafts, moveError } = await cancelScheduledToDrafts(item, mailboxes, jmapClient.accountId); clearUndoFor(item); removeItem(item); + if (moveError) { + // Not scheduled any more either way: it just stayed in Sent. + toast.warning( + t('scheduled.cancelled_not_moved', 'Send cancelled, but not moved to Drafts'), + { message: t('scheduled.cancelled_not_moved_body', 'The message will not be sent. It is still in Sent.') }, + ); + return; + } toast.success( t('scheduled.send_cancelled', 'Send cancelled'), inDrafts @@ -252,8 +260,17 @@ export default function ScheduledScreen({ navigation }: Props) { const onEdit = (item: ScheduledEmail) => { void runAction(item, async () => { // A message held in a shared account goes back to that account's Drafts. - await cancelScheduledToDrafts(item, mailboxes, jmapClient.accountId); + const { moveError } = await cancelScheduledToDrafts(item, mailboxes, jmapClient.accountId); clearUndoFor(item); + if (moveError) { + // Opening it in the composer would edit the copy in Sent. + removeItem(item); + toast.warning( + t('scheduled.cancelled_not_moved', 'Send cancelled, but not moved to Drafts'), + { message: t('scheduled.cancelled_not_moved_body', 'The message will not be sent. It is still in Sent.') }, + ); + return; + } const email = await getFullEmail(item.emailId, item.accountId); removeItem(item); navigation.replace('Compose', { draft: draftContextFromEmail(email, scheduledAccountOverride(item, jmapClient.accountId)) }); From c817f978a4d7d7bae34e3bbbc22062a12bdeddcf Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 05:01:12 +0100 Subject: [PATCH 7/7] fix: reload Scheduled after a switch of accounts and refuse actions on the other account's rows --- locales/rn/en.json | 1 + .../__tests__/scheduled-screen-wiring.test.ts | 19 +++++++ src/screens/ScheduledScreen.tsx | 55 +++++++++++++++++-- 3 files changed, 70 insertions(+), 5 deletions(-) diff --git a/locales/rn/en.json b/locales/rn/en.json index b5ad94de..3b00ff73 100644 --- a/locales/rn/en.json +++ b/locales/rn/en.json @@ -762,6 +762,7 @@ "cancelled_not_moved_body": "The message will not be sent. It is still in Sent.", "edit_failed": "Could not open the message for editing", "keep_scheduled": "Keep scheduled", + "login_switched": "These scheduled messages belong to another account. Switch back to it to change them.", "no_longer_scheduled": "This message is no longer scheduled.", "reschedule_failed": "Reschedule failed", "reschedule_lost": "The message is no longer scheduled. Check Drafts and Sent before sending it again.", diff --git a/src/lib/__tests__/scheduled-screen-wiring.test.ts b/src/lib/__tests__/scheduled-screen-wiring.test.ts index 9b1c8887..eabe86b8 100644 --- a/src/lib/__tests__/scheduled-screen-wiring.test.ts +++ b/src/lib/__tests__/scheduled-screen-wiring.test.ts @@ -23,4 +23,23 @@ describe('Scheduled screen', () => { expect(handled).toMatch(/if \(moveError\) \{/); } }); + + // The list loaded once, on mount: after a deep-link switch of logins it + // still showed the first login's rows, and Cancel or Send now acted on the + // new login's submissions with the same ids. + it('remembers the login the rows were read for and acts on them only while it is active', () => { + const load = source.slice(source.indexOf('const load = React.useCallback(')); + expect(load).toMatch(/^const load = React\.useCallback\(async \(\) => \{[\s\S]*?const owner = servedLoginOf\(jmapClient\);[\s\S]*?await listScheduledEmails\(\);[\s\S]*?if \(loginMovedFrom\(owner\)\) return;\s+ownerRef\.current = owner;\s+setItems\(list\);/); + expect(body('runAction')).toMatch(/^const runAction = async [^\n]*\{\s+if \(refuseOtherLogin\(\)\) return;/); + expect(source).toMatch(/const openView = React\.useCallback\(\(item: ScheduledEmail\) => \{\s+if \(refuseOtherLogin\(\)\) return;/); + expect(source).toMatch(/if \(!loginMovedFrom\(ownerRef\.current\)\) return false;/); + }); + + it('reads the list again for every session and drops the rows of another login', () => { + expect(source).toMatch(/if \(ownerRef\.current !== null && loginMovedFrom\(ownerRef\.current\)\) \{[\s\S]*?setItems\(\[\]\);[\s\S]*?void load\(\);\s+\}, \[activeAccountId, session, load\]\);/); + }); + + it('closes the view of a message that is no longer listed', () => { + expect(source).toMatch(/if \(viewing && !items\.some\(\(i\) => sameScheduledSend\(i, viewing\)\)\) closeView\(\);\s+\}, \[items, viewing, closeView\]\);/); + }); }); diff --git a/src/screens/ScheduledScreen.tsx b/src/screens/ScheduledScreen.tsx index 62c9bedf..4a406f98 100644 --- a/src/screens/ScheduledScreen.tsx +++ b/src/screens/ScheduledScreen.tsx @@ -30,6 +30,9 @@ import { isPendingSend, sameScheduledSend, scheduledSendKey } from '../lib/sched import { rescheduleErrorNotice, rescheduleLostMessage, restoredSubmission } from '../lib/compose-reschedule'; import { cancelScheduledToDrafts, scheduledAccountOverride } from '../lib/compose-scheduled'; import { useBackWhileFocused } from '../lib/use-back-while-focused'; +import { loginMovedFrom } from '../lib/compose-login'; +import { servedLoginOf } from '../lib/draft-login'; +import { useAuthStore } from '../stores/auth-store'; import { ScheduledMessageView } from '../components/compose/ScheduledMessageView'; import type { Email } from '../api/types'; @@ -56,7 +59,23 @@ export default function ScheduledScreen({ navigation }: Props) { const [viewEmail, setViewEmail] = React.useState(null); const [viewError, setViewError] = React.useState(null); const viewSeq = React.useRef(0); + // The login the rows were read for. Submission and email ids repeat across + // logins: after a switch (a notification or link for another account) the + // rows on screen are the old login's, and acting on them would cancel or + // send whatever the new one has under the same ids. + const ownerRef = React.useRef(null); + const activeAccountId = useAuthStore((s) => s.activeAccountId); + const session = useAuthStore((s) => s.session); + const refuseOtherLogin = React.useCallback(() => { + if (!loginMovedFrom(ownerRef.current)) return false; + Alert.alert( + t('email_composer.login_switched_title', 'Account switched'), + t('scheduled.login_switched', 'These scheduled messages belong to another account. Switch back to it to change them.'), + ); + return true; + }, [t]); const openView = React.useCallback((item: ScheduledEmail) => { + if (refuseOtherLogin()) return; const seq = ++viewSeq.current; setViewing(item); setViewEmail(null); @@ -65,7 +84,7 @@ export default function ScheduledScreen({ navigation }: Props) { (email) => { if (viewSeq.current === seq) setViewEmail(email); }, (e: unknown) => { if (viewSeq.current === seq) setViewError(e instanceof Error ? e.message : String(e)); }, ); - }, []); + }, [refuseOtherLogin]); const closeView = React.useCallback(() => { viewSeq.current += 1; setViewing(null); @@ -87,23 +106,49 @@ export default function ScheduledScreen({ navigation }: Props) { return extra > 0 ? `${name} +${extra}` : name; }; + // Only the latest read is shown: one started before a switch of logins may + // finish after the next one. + const loadSeq = React.useRef(0); const load = React.useCallback(async () => { + const seq = ++loadSeq.current; + const owner = servedLoginOf(jmapClient); setLoading(true); setError(null); try { - setItems(await listScheduledEmails()); + const list = await listScheduledEmails(); + if (seq !== loadSeq.current) return; + // Switched during the read: the next session reads again. + if (loginMovedFrom(owner)) return; + ownerRef.current = owner; + setItems(list); } catch (e) { - setError(e instanceof Error ? e.message : t('email_list.error', 'Error')); + if (seq === loadSeq.current) setError(e instanceof Error ? e.message : t('email_list.error', 'Error')); } finally { - setLoading(false); + if (seq === loadSeq.current) setLoading(false); } }, [t]); + // Read on mount and again for every session; rows, the open view and the + // menus of another login go at once. React.useEffect(() => { + if (ownerRef.current !== null && loginMovedFrom(ownerRef.current)) { + ownerRef.current = null; + setItems([]); + setActionsFor(null); + setRescheduleFor(null); + setCustomStage(null); + } void load(); - }, [load]); + }, [activeAccountId, session, load]); + + // A message that is gone from the list (sent, cancelled, read again after + // a switch) closes its view. + React.useEffect(() => { + if (viewing && !items.some((i) => sameScheduledSend(i, viewing))) closeView(); + }, [items, viewing, closeView]); const runAction = async (item: ScheduledEmail, action: () => Promise, failTitle: string) => { + if (refuseOtherLogin()) return; setBusyId(scheduledSendKey(item)); try { await action();