From 4afc2a5db09a0e95ae886cb70aeca354330cdf32 Mon Sep 17 00:00:00 2001 From: Linus Rath Date: Mon, 7 Sep 2026 22:34:47 +0200 Subject: [PATCH] fix: surface Email/set failures on delete and move #956 --- lib/__tests__/jmap-email-set-failures.test.ts | 104 ++++++++++++++++++ lib/jmap/client.ts | 37 ++++++- stores/email-store.ts | 4 +- 3 files changed, 139 insertions(+), 6 deletions(-) create mode 100644 lib/__tests__/jmap-email-set-failures.test.ts diff --git a/lib/__tests__/jmap-email-set-failures.test.ts b/lib/__tests__/jmap-email-set-failures.test.ts new file mode 100644 index 000000000..9aa0578fb --- /dev/null +++ b/lib/__tests__/jmap-email-set-failures.test.ts @@ -0,0 +1,104 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { JMAPClient } from '../jmap/client'; + +// #956: Email/set failures were swallowed by the delete/move helpers because +// request() only rejects on HTTP errors. Method-level errors and per-id +// notUpdated/notDestroyed entries must reject so the store keeps the rows. + +function makeSession() { + return { + capabilities: { 'urn:ietf:params:jmap:core': {} }, + accounts: { 'acct-1': { name: 'test', isPersonal: true, accountCapabilities: {} } }, + primaryAccounts: { 'urn:ietf:params:jmap:mail': 'acct-1' }, + apiUrl: 'https://mail.example.com/jmap/api', + downloadUrl: 'https://mail.example.com/jmap/download/{accountId}/{blobId}/{name}', + uploadUrl: 'https://mail.example.com/jmap/upload/{accountId}/', + eventSourceUrl: 'https://mail.example.com/jmap/eventsource', + }; +} + +function jsonResponse(body: unknown): Response { + return new Response(JSON.stringify(body), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }); +} + +const notUpdated = () => + jsonResponse({ + methodResponses: [ + ['Email/set', { updated: null, notUpdated: { e1: { type: 'invalidProperties', description: 'Mailbox does not exist' } } }, '0'], + ], + }); +const notDestroyed = () => + jsonResponse({ + methodResponses: [ + ['Email/set', { destroyed: null, notDestroyed: { e1: { type: 'forbidden', description: 'No permission' } } }, '0'], + ], + }); +const methodError = () => + jsonResponse({ + methodResponses: [['error', { type: 'accountNotFound', description: 'Account not found' }, '0']], + }); +const success = () => + jsonResponse({ + methodResponses: [['Email/set', { updated: { e1: null }, destroyed: ['e1'] }, '0']], + }); + +describe('JMAPClient Email/set failure surfacing (#956)', () => { + let fetchSpy: ReturnType; + + beforeEach(() => { + fetchSpy = vi.spyOn(globalThis, 'fetch'); + }); + + afterEach(() => { + fetchSpy.mockRestore(); + }); + + async function connectedClient(): Promise { + fetchSpy.mockResolvedValueOnce(jsonResponse(makeSession())); + const client = JMAPClient.withBearer('https://mail.example.com', 'token123', 'user@test.com'); + await client.connect(); + fetchSpy.mockReset(); + return client; + } + + it('deleteEmail rejects on notDestroyed', async () => { + const client = await connectedClient(); + fetchSpy.mockResolvedValueOnce(notDestroyed()); + await expect(client.deleteEmail('e1')).rejects.toThrow(/No permission/); + }); + + it('moveToTrash rejects on notUpdated', async () => { + const client = await connectedClient(); + fetchSpy.mockResolvedValueOnce(notUpdated()); + await expect(client.moveToTrash('e1', 'trash')).rejects.toThrow(/Mailbox does not exist/); + }); + + it('batchDeleteEmails rejects on notDestroyed', async () => { + const client = await connectedClient(); + fetchSpy.mockResolvedValueOnce(notDestroyed()); + await expect(client.batchDeleteEmails(['e1', 'e2'])).rejects.toThrow(/No permission/); + }); + + it('batchMoveEmails rejects on notUpdated', async () => { + const client = await connectedClient(); + fetchSpy.mockResolvedValueOnce(notUpdated()); + await expect(client.batchMoveEmails(['e1', 'e2'], 'archive')).rejects.toThrow(/Mailbox does not exist/); + }); + + it('rejects on a method-level error response', async () => { + const client = await connectedClient(); + fetchSpy.mockResolvedValueOnce(methodError()); + await expect(client.batchMoveEmails(['e1'], 'archive')).rejects.toThrow(/Account not found/); + }); + + it('still resolves when the server accepted the change', async () => { + const client = await connectedClient(); + fetchSpy.mockResolvedValueOnce(success()); + await expect(client.deleteEmail('e1')).resolves.toBeUndefined(); + fetchSpy.mockResolvedValueOnce(success()); + await expect(client.batchMoveEmails(['e1'], 'archive')).resolves.toBeUndefined(); + }); +}); diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index 17937d02f..17d028f00 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -2308,37 +2308,65 @@ export class JMAPClient implements IJMAPClient { return { migrated, refused: refusals }; } + /** + * Reject an Email/set response that reports a method-level error or per-id + * `notUpdated`/`notDestroyed` failures. `request()` only throws on HTTP + * errors, so without this the store removes rows the server refused. (#956) + */ + private assertEmailSetSucceeded(response: JMAPResponse, action: string): void { + const [name, result] = response.methodResponses?.[0] ?? []; + if (name === "error") { + throw new Error(`Failed to ${action}: ${result?.description || result?.type || 'unknown error'}`); + } + const failed: Record = { + ...(result?.notUpdated ?? {}), + ...(result?.notDestroyed ?? {}), + }; + const failedIds = Object.keys(failed); + if (failedIds.length === 0) return; + const first = failed[failedIds[0]]; + const reason = first?.description || first?.type || 'unknown error'; + throw new Error( + failedIds.length === 1 + ? `Failed to ${action}: ${reason}` + : `Failed to ${action} ${failedIds.length} email(s); first error: ${reason}`, + ); + } + async deleteEmail(emailId: string, accountId?: string): Promise { - await this.request([ + const response = await this.request([ ["Email/set", { accountId: accountId || this.accountId, destroy: [emailId], }, "0"], ]); + this.assertEmailSetSucceeded(response, 'delete email'); } async moveToTrash(emailId: string, trashMailboxId: string, accountId?: string, markAsRead?: boolean): Promise { const targetAccountId = accountId || this.accountId; const patch: Record = { mailboxIds: { [trashMailboxId]: true } }; if (markAsRead) patch["keywords/$seen"] = true; - await this.request([ + const response = await this.request([ ["Email/set", { accountId: targetAccountId, update: { [emailId]: patch }, }, "0"], ]); + this.assertEmailSetSucceeded(response, 'move email to trash'); } async batchDeleteEmails(emailIds: string[], accountId?: string): Promise { if (emailIds.length === 0) return; for (const batch of batched(emailIds, this.getMaxObjectsInSet())) { - await this.request([ + const response = await this.request([ ["Email/set", { accountId: accountId || this.accountId, destroy: batch, }, "0"], ]); + this.assertEmailSetSucceeded(response, 'delete emails'); } } @@ -2352,9 +2380,10 @@ export class JMAPClient implements IJMAPClient { }; for (const batch of batched(emailIds, this.getMaxObjectsInSet())) { const updates = Object.fromEntries(batch.map(id => [id, buildPatch()])); - await this.request([ + const response = await this.request([ ["Email/set", { accountId: accountId || this.accountId, update: updates }, "0"], ]); + this.assertEmailSetSucceeded(response, 'move emails'); } } diff --git a/stores/email-store.ts b/stores/email-store.ts index 269dbafab..1dada2112 100644 --- a/stores/email-store.ts +++ b/stores/email-store.ts @@ -2020,8 +2020,8 @@ export const useEmailStore = create((set, get) => ({ throw new Error('Trash mailbox not found - cannot move email to trash'); } - // Permanent delete - await effectiveClient.deleteEmail(emailId); + // Permanent delete - in the email's own account, like the batch path. + await effectiveClient.deleteEmail(emailId, accountId); // Remove from local state and update mailbox counters (in the email's own // account list). Unread emails also decrement the unread counters. (#281)