diff --git a/components/calendar/calendar-app.tsx b/components/calendar/calendar-app.tsx index d3994af08..c06c135d2 100644 --- a/components/calendar/calendar-app.tsx +++ b/components/calendar/calendar-app.tsx @@ -1539,8 +1539,9 @@ export function CalendarApp({ linkSegments }: CalendarAppProps = {}) { try { const count = await clearCalendarEvents(client, cal.id); toast.success(tMgmt("events_cleared", { count })); - } catch { - toast.error(tMgmt("error_clear")); + } catch (err) { + // The store rethrows the server's reason (#434). + toast.error(err instanceof Error && err.message ? err.message : tMgmt("error_clear")); } } : undefined} onDeleteCalendar={client ? async (cal: Calendar) => { diff --git a/lib/__tests__/jmap-calendar-batch-failures.test.ts b/lib/__tests__/jmap-calendar-batch-failures.test.ts new file mode 100644 index 000000000..fd0e22e68 --- /dev/null +++ b/lib/__tests__/jmap-calendar-batch-failures.test.ts @@ -0,0 +1,127 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { JMAPClient } from '../jmap/client'; + +// #434: batchDeleteCalendarEvents ignored a method-level error and dropped the +// notDestroyed details, so "Clear events" reported "0 events cleared" with no +// error. The same for batchCreateCalendarEvents' notCreated on import. + +function makeSession() { + return { + capabilities: { + 'urn:ietf:params:jmap:core': {}, + 'urn:ietf:params:jmap:calendars': {}, + }, + accounts: { + 'acct-1': { + name: 'test', + isPersonal: true, + accountCapabilities: { 'urn:ietf:params:jmap:calendars': {} }, + }, + }, + primaryAccounts: { + 'urn:ietf:params:jmap:mail': 'acct-1', + 'urn:ietf:params:jmap:calendars': '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' }, + }); +} + +describe('calendar batch failures surface (#434)', () => { + 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('batchDeleteCalendarEvents rejects with the server reason when nothing was destroyed', async () => { + const client = await connectedClient(); + fetchSpy.mockResolvedValueOnce( + jsonResponse({ + methodResponses: [ + ['CalendarEvent/set', { + destroyed: null, + notDestroyed: { ev1: { type: 'forbidden', description: 'You do not own this calendar' } }, + }, '0'], + ], + }), + ); + + await expect(client.batchDeleteCalendarEvents(['ev1'])).rejects.toThrow(/You do not own this calendar/); + }); + + it('batchDeleteCalendarEvents rejects on a method-level error', async () => { + const client = await connectedClient(); + fetchSpy.mockResolvedValueOnce( + jsonResponse({ + methodResponses: [['error', { type: 'accountNotFound', description: 'Account not found' }, '0']], + }), + ); + + await expect(client.batchDeleteCalendarEvents(['ev1'])).rejects.toThrow(/Account not found/); + }); + + it('batchDeleteCalendarEvents returns the notDestroyed map for partial failures', async () => { + const client = await connectedClient(); + fetchSpy.mockResolvedValueOnce( + jsonResponse({ + methodResponses: [ + ['CalendarEvent/set', { + destroyed: ['ev1'], + notDestroyed: { ev2: { type: 'notFound', description: 'Gone' } }, + }, '0'], + ], + }), + ); + + const result = await client.batchDeleteCalendarEvents(['ev1', 'ev2']); + expect(result.destroyed).toEqual(['ev1']); + expect(result.notDestroyed).toEqual({ ev2: { type: 'notFound', description: 'Gone' } }); + }); + + it('batchCreateCalendarEvents rejects on a method-level error and reports notCreated otherwise', async () => { + const client = await connectedClient(); + fetchSpy.mockResolvedValueOnce( + jsonResponse({ + methodResponses: [['error', { type: 'invalidArguments', description: 'Unknown calendar' }, '0']], + }), + ); + await expect(client.batchCreateCalendarEvents([{ title: 'A' }])).rejects.toThrow(/Unknown calendar/); + + fetchSpy.mockResolvedValueOnce( + jsonResponse({ + methodResponses: [ + ['CalendarEvent/set', { + created: null, + notCreated: { 'new-0': { type: 'invalidProperties', description: 'Missing start' } }, + }, '0'], + ], + }), + ); + const result = await client.batchCreateCalendarEvents([{ title: 'A' }]); + expect(result.created).toEqual([]); + expect(result.failed).toEqual(['new-0']); + expect(result.notCreated['new-0']).toMatchObject({ description: 'Missing start' }); + }); +}); diff --git a/lib/demo/demo-client.ts b/lib/demo/demo-client.ts index 2e23cf326..9858bcee9 100644 --- a/lib/demo/demo-client.ts +++ b/lib/demo/demo-client.ts @@ -915,13 +915,13 @@ export class DemoJMAPClient implements IJMAPClient { return full; } - async batchCreateCalendarEvents(events: Partial[]): Promise<{ created: CalendarEvent[]; failed: string[] }> { + async batchCreateCalendarEvents(events: Partial[]): Promise<{ created: CalendarEvent[]; failed: string[]; notCreated: Record }> { const created: CalendarEvent[] = []; for (const event of events) { const full = await this.createCalendarEvent(event); created.push(full); } - return { created, failed: [] }; + return { created, failed: [], notCreated: {} }; } async updateCalendarEvent(eventId: string, updates: Partial): Promise { @@ -934,10 +934,10 @@ export class DemoJMAPClient implements IJMAPClient { this.data.calendarEvents = this.data.calendarEvents.filter(e => e.id !== eventId); } - async batchDeleteCalendarEvents(eventIds: string[]): Promise<{ destroyed: string[]; notDestroyed: string[] }> { + async batchDeleteCalendarEvents(eventIds: string[]): Promise<{ destroyed: string[]; notDestroyed: Record }> { const idSet = new Set(eventIds); this.data.calendarEvents = this.data.calendarEvents.filter(e => !idSet.has(e.id)); - return { destroyed: eventIds, notDestroyed: [] }; + return { destroyed: eventIds, notDestroyed: {} }; } async queryCalendarEvents(filter: CalendarEventFilter): Promise { diff --git a/lib/jmap/client-interface.ts b/lib/jmap/client-interface.ts index 949670e80..02b225cca 100644 --- a/lib/jmap/client-interface.ts +++ b/lib/jmap/client-interface.ts @@ -405,7 +405,7 @@ export interface IJMAPClient { getCalendarEvents(calendarIds?: string[], targetAccountId?: string): Promise; getCalendarEvent(id: string, targetAccountId?: string): Promise; createCalendarEvent(event: Partial, sendSchedulingMessages?: boolean, targetAccountId?: string): Promise; - batchCreateCalendarEvents(events: Partial[], targetAccountId?: string): Promise<{ created: CalendarEvent[]; failed: string[] }>; + batchCreateCalendarEvents(events: Partial[], targetAccountId?: string): Promise<{ created: CalendarEvent[]; failed: string[]; notCreated: Record }>; updateCalendarEvent( eventId: string, updates: Partial, @@ -413,7 +413,7 @@ export interface IJMAPClient { targetAccountId?: string, ): Promise; deleteCalendarEvent(eventId: string, sendSchedulingMessages?: boolean, targetAccountId?: string): Promise; - batchDeleteCalendarEvents(eventIds: string[], targetAccountId?: string): Promise<{ destroyed: string[]; notDestroyed: string[] }>; + batchDeleteCalendarEvents(eventIds: string[], targetAccountId?: string): Promise<{ destroyed: string[]; notDestroyed: Record }>; queryCalendarEvents(filter: CalendarEventFilter, sort?: Array<{ property: string; isAscending: boolean }>, limit?: number, targetAccountId?: string): Promise; queryAllCalendarEvents(filter: CalendarEventFilter, sort?: Array<{ property: string; isAscending: boolean }>, limit?: number): Promise; parseCalendarEvents(accountId: string, blobId: string): Promise[]>; diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index 17d028f00..3041e0dc0 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -173,6 +173,12 @@ interface JMAPResponse { methodResponses: Array<[string, JMAPResponseResult, string]>; } +/** Per-id failure entry of a CalendarEvent/set (`notCreated` / `notDestroyed`). */ +export interface CalendarSetError { + type?: string; + description?: string; +} + const DEFAULT_MAILBOX_RIGHTS = { mayReadItems: true, mayAddItems: true, @@ -6534,8 +6540,8 @@ export class JMAPClient implements IJMAPClient { async batchCreateCalendarEvents( events: Partial[], targetAccountId?: string, - ): Promise<{ created: CalendarEvent[]; failed: string[] }> { - if (events.length === 0) return { created: [], failed: [] }; + ): Promise<{ created: CalendarEvent[]; failed: string[]; notCreated: Record }> { + if (events.length === 0) return { created: [], failed: [], notCreated: {} }; const accountId = targetAccountId || this.getCalendarsAccountId(); @@ -6543,6 +6549,7 @@ export class JMAPClient implements IJMAPClient { const createdIds: string[] = []; const failed: string[] = []; + const notCreated: Record = {}; const indexed = events.map((event, index) => ({ event, index })); for (const batch of batched(indexed, this.getMaxObjectsInSet())) { @@ -6564,6 +6571,10 @@ export class JMAPClient implements IJMAPClient { ["CalendarEvent/set", { accountId, sendSchedulingMessages: false, create: createMap }, "0"] ], this.calendarUsing()); + // A method-level error (unknown calendar, missing capability, …) is + // not a per-event failure: nothing was created, say so. (#434) + this.throwOnCalendarSetError(response, 'create calendar events'); + if (response.methodResponses?.[0]?.[0] === "CalendarEvent/set") { const result = response.methodResponses[0][1]; for (const { index } of batch) { @@ -6573,13 +6584,14 @@ export class JMAPClient implements IJMAPClient { } else if (result.notCreated?.[key]) { debug.warn('calendar', `CalendarEvent/batchCreate failed for ${key}`, result.notCreated[key]); failed.push(key); + notCreated[key] = result.notCreated[key]; } } } } if (createdIds.length === 0) { - return { created: [], failed }; + return { created: [], failed, notCreated }; } // Fetch the created events back for their server-assigned properties @@ -6608,7 +6620,7 @@ export class JMAPClient implements IJMAPClient { failed: failed.length, }); - return { created: createdEvents, failed }; + return { created: createdEvents, failed, notCreated }; } async updateCalendarEvent( @@ -6746,22 +6758,52 @@ export class JMAPClient implements IJMAPClient { throw new Error("Failed to delete calendar event"); } - async batchDeleteCalendarEvents(eventIds: string[], targetAccountId?: string): Promise<{ destroyed: string[]; notDestroyed: string[] }> { - if (eventIds.length === 0) return { destroyed: [], notDestroyed: [] }; + /** + * Reject a CalendarEvent/set response that came back as a method-level + * error. Without this the callers counted the (empty) results and + * reported "0 events cleared" with no error at all. (#434) + */ + private throwOnCalendarSetError(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'}`); + } + } + + /** + * Destroy events in batches. Per-id failures come back in `notDestroyed` + * (with the server's type/description) so the caller can report them; + * a method-level error, or a batch in which nothing at all could be + * destroyed, rejects with the first description. (#434) + */ + async batchDeleteCalendarEvents(eventIds: string[], targetAccountId?: string): Promise<{ destroyed: string[]; notDestroyed: Record }> { + if (eventIds.length === 0) return { destroyed: [], notDestroyed: {} }; const accountId = targetAccountId || this.getCalendarsAccountId(); const destroyed: string[] = []; - const notDestroyed: string[] = []; + const notDestroyed: Record = {}; for (const batch of batched(eventIds, this.getMaxObjectsInSet())) { const response = await this.request([ ["CalendarEvent/set", { accountId, destroy: batch }, "0"] ], this.calendarUsing()); + this.throwOnCalendarSetError(response, 'delete calendar events'); + + let destroyedInBatch = 0; if (response.methodResponses?.[0]?.[0] === "CalendarEvent/set") { const result = response.methodResponses[0][1]; - if (result.destroyed) destroyed.push(...result.destroyed); - if (result.notDestroyed) notDestroyed.push(...Object.keys(result.notDestroyed)); + if (result.destroyed) { + destroyed.push(...result.destroyed); + destroyedInBatch = result.destroyed.length; + } + if (result.notDestroyed) Object.assign(notDestroyed, result.notDestroyed); + } + if (destroyedInBatch === 0 && batch.length > 0) { + const first = Object.values(notDestroyed)[0]; + throw new Error( + `Failed to delete calendar events: ${first?.description || first?.type || 'server refused the request'}`, + ); } } diff --git a/stores/calendar-store.ts b/stores/calendar-store.ts index babf0a1b2..1c84ddf5c 100644 --- a/stores/calendar-store.ts +++ b/stores/calendar-store.ts @@ -1005,10 +1005,13 @@ export const useCalendarStore = create()( for (let i = 0; i < prepared.length; i += BATCH_SIZE) { const batch = prepared.slice(i, i + BATCH_SIZE); try { - const { created, failed } = await client.batchCreateCalendarEvents(batch, targetAccountId); + const { created, failed, notCreated } = await client.batchCreateCalendarEvents(batch, targetAccountId); imported += created.length; if (failed.length > 0) { - debug.warn('calendar', `Import batch ${i / BATCH_SIZE + 1}: ${failed.length} events failed`); + const first = Object.values(notCreated)[0]; + const reason = first?.description || first?.type || 'unknown error'; + debug.warn('calendar', `Import batch ${i / BATCH_SIZE + 1}: ${failed.length} events failed (${reason})`); + set({ error: `${failed.length} event(s) could not be imported: ${reason}` }); } } catch (error) { debug.error(`Import batch ${i / BATCH_SIZE + 1} failed:`, error); @@ -1185,6 +1188,7 @@ export const useCalendarStore = create()( const targetAccountId = cal?.accountId; client = resolveAccountClient(client, cal?.localAccountId); let totalRemoved = 0; + let firstRefusalMessage = ''; // Loop to handle pagination (getCalendarEvents has a 1000 limit) let hasMore = true; while (hasMore) { @@ -1211,8 +1215,14 @@ export const useCalendarStore = create()( let removedThisPass = 0; if (idsToDelete.length > 0) { - const { destroyed } = await client.batchDeleteCalendarEvents(idsToDelete, targetAccountId); + // Rejects on a method-level error or when nothing could be + // destroyed; partial refusals are reported once below. (#434) + const { destroyed, notDestroyed } = await client.batchDeleteCalendarEvents(idsToDelete, targetAccountId); removedThisPass += destroyed.length; + const firstRefusal = Object.values(notDestroyed)[0]; + if (firstRefusal && !firstRefusalMessage) { + firstRefusalMessage = firstRefusal.description || firstRefusal.type || 'server refused the request'; + } } for (const { id, calendarIds } of eventsToUnlink) { try { @@ -1224,10 +1234,15 @@ export const useCalendarStore = create()( } totalRemoved += removedThisPass; - // If we couldn't remove anything, stop to avoid infinite loop + // If we couldn't remove anything, stop to avoid infinite loop - + // and say why, instead of reporting "0 events cleared". (#434) if (removedThisPass === 0) { debug.warn('calendar', 'Could not clear any events, stopping. Remaining:', calendarEvents.length); - break; + throw new Error( + firstRefusalMessage + ? `Failed to clear calendar events: ${firstRefusalMessage}` + : 'Failed to clear calendar events: the server did not remove any of them', + ); } // If we got fewer than the limit, we've fetched everything @@ -1240,7 +1255,7 @@ export const useCalendarStore = create()( return totalRemoved; } catch (error) { debug.error('Failed to clear calendar events:', error); - set({ error: 'Failed to clear calendar events' }); + set({ error: error instanceof Error ? error.message : 'Failed to clear calendar events' }); throw error; } },