From ac875ffe1d4d208281290ea3a0eafabc13ea04c4 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 18:27:57 +0100 Subject: [PATCH] fix: don't move a queued message to drafts when an earlier attempt already sent it --- locales/rn/en.json | 1 + src/api/__tests__/queued-send.test.ts | 27 ++++++- src/api/queued-send.ts | 78 +++++++++++++------ .../__tests__/outbox-send-queue.test.ts | 34 ++++++++ src/stores/outbox-store.ts | 29 +++++++ 5 files changed, 144 insertions(+), 25 deletions(-) diff --git a/locales/rn/en.json b/locales/rn/en.json index 6f58ab52..37bf230c 100644 --- a/locales/rn/en.json +++ b/locales/rn/en.json @@ -1004,6 +1004,7 @@ "section_personal": "My templates" }, "outbox": { + "already_sent": "This message was already sent", "discard": "Discard", "discard_body": "It will not be sent, and its text is lost.", "discard_title": "Discard this message?", diff --git a/src/api/__tests__/queued-send.test.ts b/src/api/__tests__/queued-send.test.ts index fa93f669..333b2c1f 100644 --- a/src/api/__tests__/queued-send.test.ts +++ b/src/api/__tests__/queued-send.test.ts @@ -18,7 +18,7 @@ vi.mock('../../lib/compose-send-account', () => ({ })); import { jmapClient } from '../jmap-client'; -import { findCopiesByMessageId, runQueuedSend } from '../queued-send'; +import { findCopiesByMessageId, runQueuedSend, wasQueuedSendSent } from '../queued-send'; import { newQueuedSend, type QueuedSendPayload } from '../../lib/send-queue'; const mockRequest = jmapClient.request as ReturnType; @@ -85,6 +85,31 @@ describe('runQueuedSend', () => { }); }); +describe('wasQueuedSendSent', () => { + it('finds a copy an earlier attempt sent, and removes the old draft', async () => { + mockRequest.mockResolvedValueOnce(found([ + { id: 'draft-1', keywords: { $draft: true }, messageId: ['m1@x.de'] }, + { id: 'sent-1', keywords: { $seen: true }, messageId: ['m1@x.de'] }, + ])); + expect(await wasQueuedSendSent(newQueuedSend('q', PAYLOAD, 0, true), [])).toBe(true); + expect(destroyEmails).toHaveBeenCalledWith(['draft-1'], 'acc-1'); + }); + + it('says not sent when only drafts are there, and removes nothing', async () => { + mockRequest.mockResolvedValueOnce(found([ + { id: 'draft-1', keywords: { $draft: true }, messageId: ['m1@x.de'] }, + { id: 'left-1', keywords: { $draft: true, $seen: true }, messageId: ['m1@x.de'] }, + ])); + expect(await wasQueuedSendSent(newQueuedSend('q', PAYLOAD, 0, true), [])).toBe(false); + expect(destroyEmails).not.toHaveBeenCalled(); + }); + + it('does not look for a message no attempt reached the server with', async () => { + expect(await wasQueuedSendSent(newQueuedSend('q', PAYLOAD, 0, false), [])).toBe(false); + expect(mockRequest).not.toHaveBeenCalled(); + }); +}); + describe('findCopiesByMessageId', () => { it('reads the recent messages of Sent and Drafts and keeps exact matches only', async () => { mockRequest.mockResolvedValueOnce(found([ diff --git a/src/api/queued-send.ts b/src/api/queued-send.ts index d9503310..c7641987 100644 --- a/src/api/queued-send.ts +++ b/src/api/queued-send.ts @@ -8,7 +8,7 @@ import type { Mailbox } from './types'; import { sendTargetFor } from '../lib/compose-send-account'; import { supportsSubmissionExtension } from '../lib/submission-options'; import { stripMessageIdBrackets } from '../lib/email-threading'; -import { holdForScheduled, verdictFromCopies, type FoundCopy, type QueuedSend } from '../lib/send-queue'; +import { holdForScheduled, verdictFromCopies, type CopyVerdict, type FoundCopy, type QueuedSend } from '../lib/send-queue'; /** How far before the queueing a copy can be dated (the server's clock may differ). */ const CLOCK_SKEW_MS = 10 * 60_000; @@ -68,34 +68,64 @@ export type QueuedSendOutcome = * went out; copies a refused attempt left in Drafts are removed before the * new attempt. */ -export async function runQueuedSend(send: QueuedSend, mailboxes: Mailbox[], now = Date.now()): Promise { +/** The account and folders a queued message goes through, as the composer works them out. */ +async function queuedSendTarget(send: QueuedSend, mailboxes: Mailbox[]) { const p = send.payload; const target = await sendTargetFor(p.identity, p.fromEmail, mailboxes); - const accountId = target.accountId ?? jmapClient.accountId; - const draftAccountId = p.draftAccountId ?? jmapClient.accountId; + return { + target, + accountId: target.accountId ?? jmapClient.accountId, + draftAccountId: p.draftAccountId ?? jmapClient.accountId, + sentMailboxId: target.sentMailboxId ?? p.sentMailboxId, + draftsMailboxId: target.accountId ? target.draftsMailboxId : p.draftsMailboxId, + }; +} - const sentMailboxId = target.sentMailboxId ?? p.sentMailboxId; - const draftsMailboxId = target.accountId ? target.draftsMailboxId : p.draftsMailboxId; +type Resolved = Awaited>; + +/** + * Whether an earlier, unanswered attempt at `send` went out. When it did, the + * old draft that send would have removed is removed now. + */ +async function earlierAttempt(send: QueuedSend, r: Resolved): Promise { + const p = send.payload; + const copies = await findCopiesByMessageId(p.outgoing.messageId!, { + accountId: r.target.accountId, + mailboxIds: [r.sentMailboxId, r.draftsMailboxId ?? ''], + since: send.createdAt, + }); + // The autosaved draft lives in this account only when the draft did. + const draftHere = p.draftId && r.draftAccountId === r.accountId ? p.draftId : undefined; + const verdict = verdictFromCopies(copies, draftHere); + if (verdict.kind === 'sent' && p.draftId && p.draftId !== verdict.copyId) { + // The send went out but its answer was lost, so the old draft it would + // have removed is still there. + await destroyEmails([p.draftId], r.draftAccountId).catch((err) => { + console.warn('[send-queue] could not remove the draft of a sent message', err); + }); + } + return verdict; +} + +/** + * Before a queued message is taken back (moved to Drafts): true when an + * earlier attempt that may have reached the server did send it. A message no + * attempt reached the server with is never looked up. + */ +export async function wasQueuedSendSent(send: QueuedSend, mailboxes: Mailbox[]): Promise { + if (!send.maybeSent || !send.payload.outgoing.messageId) return false; + const verdict = await earlierAttempt(send, await queuedSendTarget(send, mailboxes)); + return verdict.kind === 'sent'; +} + +export async function runQueuedSend(send: QueuedSend, mailboxes: Mailbox[], now = Date.now()): Promise { + const p = send.payload; + const resolved = await queuedSendTarget(send, mailboxes); + const { target, accountId, sentMailboxId, draftsMailboxId } = resolved; if (send.maybeSent && p.outgoing.messageId) { - const copies = await findCopiesByMessageId(p.outgoing.messageId, { - accountId: target.accountId, - mailboxIds: [sentMailboxId, draftsMailboxId ?? ''], - since: send.createdAt, - }); - // The autosaved draft lives in this account only when the draft did. - const draftHere = p.draftId && draftAccountId === accountId ? p.draftId : undefined; - const verdict = verdictFromCopies(copies, draftHere); - if (verdict.kind === 'sent') { - // The send went out but its answer was lost, so the old draft it would - // have removed is still there. - if (p.draftId && p.draftId !== verdict.copyId) { - await destroyEmails([p.draftId], draftAccountId).catch((err) => { - console.warn('[send-queue] could not remove the draft of a sent message', err); - }); - } - return { kind: 'alreadySent', accountId: target.accountId }; - } + const verdict = await earlierAttempt(send, resolved); + if (verdict.kind === 'sent') return { kind: 'alreadySent', accountId: target.accountId }; if (verdict.staleIds.length > 0) { await destroyEmails(verdict.staleIds, accountId); } diff --git a/src/stores/__tests__/outbox-send-queue.test.ts b/src/stores/__tests__/outbox-send-queue.test.ts index 46bc6715..11655c32 100644 --- a/src/stores/__tests__/outbox-send-queue.test.ts +++ b/src/stores/__tests__/outbox-send-queue.test.ts @@ -25,8 +25,10 @@ vi.mock('../../api/email', () => ({ })); const runQueuedSend = vi.fn(async (..._a: any[]): Promise => ({ kind: 'sent', result: { scheduled: false } })); +const wasQueuedSendSent = vi.fn(async (..._a: any[]) => false); vi.mock('../../api/queued-send', () => ({ runQueuedSend: (...a: any[]) => runQueuedSend(...a), + wasQueuedSendSent: (...a: any[]) => wasQueuedSendSent(...a), })); vi.mock('../email-store', () => ({ useEmailStore: { getState: () => ({ mailboxes: [{ id: 'sent', role: 'sent' }] }) }, @@ -221,6 +223,38 @@ describe('offline send queue', () => { expect(useOutboxStore.getState().sends[0]).toMatchObject({ id: again, status: 'failed' }); }); + it('does not turn a message an earlier attempt sent into a draft, and says it was sent', async () => { + const id = useOutboxStore.getState().enqueueSend(payload('went-out'), { maybeSent: true })!; + wasQueuedSendSent.mockResolvedValueOnce(true); + expect(await useOutboxStore.getState().cancelSend(id, { keepAsDraft: true })).toBe(true); + expect(wasQueuedSendSent).toHaveBeenCalledWith(expect.objectContaining({ id, maybeSent: true }), [{ id: 'sent', role: 'sent' }]); + expect(saveDraftVersion).not.toHaveBeenCalled(); + expect(useOutboxStore.getState().sends).toEqual([]); + await flushed(); + expect(toast.info).toHaveBeenCalledWith('This message was already sent', { message: 'went-out' }); + expect(patchKeywordsForEmails).toHaveBeenCalledWith(['orig-1'], { $answered: true }, undefined); + }); + + it('moves a marked message to Drafts when no earlier attempt sent it, and looks only for marked ones', async () => { + const marked = useOutboxStore.getState().enqueueSend(payload('marked'), { maybeSent: true })!; + expect(await useOutboxStore.getState().cancelSend(marked, { keepAsDraft: true })).toBe(true); + expect(wasQueuedSendSent).toHaveBeenCalledTimes(1); + expect(saveDraftVersion).toHaveBeenCalledTimes(1); + + const plain = useOutboxStore.getState().enqueueSend(payload('plain'))!; + expect(await useOutboxStore.getState().cancelSend(plain, { keepAsDraft: true })).toBe(true); + expect(wasQueuedSendSent).toHaveBeenCalledTimes(1); + expect(saveDraftVersion).toHaveBeenCalledTimes(2); + }); + + it('keeps a marked message in the Outbox when the lookup fails', async () => { + const id = useOutboxStore.getState().enqueueSend(payload('unknown'), { maybeSent: true })!; + wasQueuedSendSent.mockRejectedValueOnce(named('NetworkError', 'Network request failed')); + expect(await useOutboxStore.getState().cancelSend(id, { keepAsDraft: true })).toBe(false); + expect(saveDraftVersion).not.toHaveBeenCalled(); + expect(useOutboxStore.getState().sends[0]).toMatchObject({ id, status: 'failed' }); + }); + it('drops the queued messages with the rest when the server behind the account changed', async () => { useOutboxStore.getState().enqueueSend(payload()); expect(await useOutboxStore.getState().clearAccount(ACCOUNT)).toBe(1); diff --git a/src/stores/outbox-store.ts b/src/stores/outbox-store.ts index d0ea2aef..ab4c7350 100644 --- a/src/stores/outbox-store.ts +++ b/src/stores/outbox-store.ts @@ -562,6 +562,22 @@ export const useOutboxStore = create((set, get) => ({ if (!opts?.keepAsDraft) return true; const p = send.payload; try { + // An attempt that may have reached the server is looked for first, as + // before a resend: a message that went out must not come back as a + // draft. + if (send.maybeSent) { + const [{ wasQueuedSendSent }, { useEmailStore }] = await Promise.all([ + import('../api/queued-send'), + import('./email-store'), + ]); + if (await wasQueuedSendSent(send, useEmailStore.getState().mailboxes)) { + if (p.original) { + patchKeywordsForEmails([p.original.emailId], { [p.original.keyword]: true }, p.original.accountId).catch(() => undefined); + } + void notifyAlreadySent(send); + return true; + } + } if (!p.draftsMailboxId) throw new Error('No Drafts folder'); const { saveDraftVersion } = await import('../api/email'); await saveDraftVersion(p.outgoing, p.draftsMailboxId, p.draftId, undefined, p.draftAccountId); @@ -732,6 +748,19 @@ async function afterQueuedSend(send: QueuedSend, outcome: QueuedSendOutcomeLike) } } +/** Taking back a message found to have gone out already: nothing became a draft. */ +async function notifyAlreadySent(send: QueuedSend): Promise { + try { + const [{ toast }, { useLocaleStore }] = await Promise.all([import('./toast-store'), import('./locale-store')]); + const t = useLocaleStore.getState().t; + toast.info(t('outbox.already_sent', 'This message was already sent'), { + message: send.payload.outgoing.subject || undefined, + }); + } catch (err) { + console.warn('[outbox] could not report an already sent message', err); + } +} + async function notifyQueuedSendFailed(send: QueuedSend, message: string): Promise { try { const [{ toast }, { useLocaleStore }] = await Promise.all([import('./toast-store'), import('./locale-store')]);