diff --git a/lib/__tests__/jmap-all-calendars-failures.test.ts b/lib/__tests__/jmap-all-calendars-failures.test.ts new file mode 100644 index 000000000..1cc96a030 --- /dev/null +++ b/lib/__tests__/jmap-all-calendars-failures.test.ts @@ -0,0 +1,114 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { JMAPClient } from '../jmap/client'; + +// The calendar store reconciles its persisted selection against the calendars +// list. getAllCalendars() swallowed every failure into an empty (or partial) +// list, which the store read as "these calendars are gone" and dropped from +// the selection for good. getAllCalendarsWithFailures() lets it tell the two +// apart. + +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': {} } }, + // Shared accounts are probed on suspicion because they are non-personal. + 'team': { name: 'Team', isPersonal: false, accountCapabilities: {} }, + 'ev': { name: 'No access', isPersonal: false, accountCapabilities: {} }, + }, + 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: '', + }; +} + +function jsonResponse(body: unknown): Response { + return new Response(JSON.stringify(body), { status: 200, headers: { 'Content-Type': 'application/json' } }); +} + +type Answer = 'ok' | 'serverFail' | 'network'; + +describe('getAllCalendarsWithFailures', () => { + let fetchSpy: ReturnType; + let consoleErrorSpy: ReturnType; + let client: JMAPClient; + let answers: Record; + + beforeEach(async () => { + fetchSpy = vi.spyOn(globalThis, 'fetch'); + fetchSpy.mockResolvedValueOnce(jsonResponse(makeSession())); + client = new JMAPClient('https://mail.example.com', 'user@test.com', 'pass123'); + await client.connect(); + fetchSpy.mockReset(); + + answers = { 'acct-1': 'ok', team: 'ok' }; + fetchSpy.mockImplementation(async (_url: RequestInfo | URL, init?: RequestInit) => { + const body = JSON.parse(String(init?.body ?? '{}')); + const [method, args] = body.methodCalls?.[0] ?? []; + const accountId = args?.accountId as string; + if (accountId === 'ev') { + return jsonResponse({ + methodResponses: [['error', { type: 'accountNotFound', description: 'You do not have access to account ev' }, '0']], + }); + } + if (method === 'CalendarEvent/query') { + return jsonResponse({ methodResponses: [['CalendarEvent/query', { ids: [] }, '0']] }); + } + const answer = answers[accountId]; + if (answer === 'network') throw new TypeError('Failed to fetch'); + if (answer === 'serverFail') { + return jsonResponse({ methodResponses: [['error', { type: 'serverFail', description: 'Try again' }, '0']] }); + } + return jsonResponse({ methodResponses: [['Calendar/get', { list: [{ id: 'b', name: `${accountId} cal` }] }, '0']] }); + }); + consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + client.disconnect(); + consoleErrorSpy.mockRestore(); + fetchSpy.mockRestore(); + }); + + it('lists every account and reports nothing failed when all load', async () => { + const { calendars, failedAccountIds } = await client.getAllCalendarsWithFailures(); + + expect(calendars.map((c) => c.id)).toEqual(['b', 'team:b']); + expect(failedAccountIds).toEqual([]); + }); + + it('throws when the primary account answers with a method error', async () => { + answers['acct-1'] = 'serverFail'; + + await expect(client.getAllCalendarsWithFailures()).rejects.toThrow('Try again'); + }); + + it('throws when the primary account cannot be reached', async () => { + answers['acct-1'] = 'network'; + + await expect(client.getAllCalendarsWithFailures()).rejects.toThrow(); + }); + + it('names a shared account that failed instead of silently leaving it out', async () => { + answers.team = 'serverFail'; + + const { calendars, failedAccountIds } = await client.getAllCalendarsWithFailures(); + + expect(calendars.map((c) => c.id)).toEqual(['b']); + expect(failedAccountIds).toEqual(['team']); + }); + + it('does not count a shared account without calendar access as failed', async () => { + const { failedAccountIds } = await client.getAllCalendarsWithFailures(); + + expect(failedAccountIds).not.toContain('ev'); + }); + + it('keeps getAllCalendars lenient for callers that only display the list', async () => { + answers['acct-1'] = 'serverFail'; + + await expect(client.getAllCalendars()).resolves.toEqual([]); + }); +}); diff --git a/lib/demo/demo-client.ts b/lib/demo/demo-client.ts index 5d1150a51..502f40180 100644 --- a/lib/demo/demo-client.ts +++ b/lib/demo/demo-client.ts @@ -884,6 +884,9 @@ export class DemoJMAPClient implements IJMAPClient { async getCalendars(): Promise { return [...this.data.calendars]; } async getAllCalendars(): Promise { return [...this.data.calendars]; } + async getAllCalendarsWithFailures(): Promise<{ calendars: Calendar[]; failedAccountIds: string[] }> { + return { calendars: [...this.data.calendars], failedAccountIds: [] }; + } async createCalendar(calendar: Partial): Promise { const full: Calendar = { diff --git a/lib/jmap/client-interface.ts b/lib/jmap/client-interface.ts index 251547426..e6ef7179d 100644 --- a/lib/jmap/client-interface.ts +++ b/lib/jmap/client-interface.ts @@ -430,6 +430,7 @@ export interface IJMAPClient { getCalendarsAccountId(): string; getCalendars(): Promise; getAllCalendars(): Promise; + getAllCalendarsWithFailures(): Promise<{ calendars: Calendar[]; failedAccountIds: string[] }>; createCalendar(calendar: Partial, targetAccountId?: string, options?: CreateCalendarOptions): Promise; updateCalendar(calendarId: string, updates: Partial, targetAccountId?: string): Promise; setDefaultCalendar(calendarId: string, targetAccountId?: string): Promise; diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index abbb93ef5..00cac0b8d 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -434,6 +434,15 @@ function isTaskObject(obj: { '@type'?: string; progress?: unknown; due?: unknown return false; } +/** + * Whether a calendar method error means the account grants this user no + * calendar access at all - the expected answer from a shared account the + * fan-out probed on suspicion (see getCalendarCapableAccountIds). + */ +function isCalendarAccessDenial(type: string | undefined, message: string | undefined): boolean { + return type === 'forbidden' || type === 'accountNotFound' || /not have access/i.test(message ?? ''); +} + const CALENDAR_EVENT_PROPERTIES = [ 'id', '@type', @@ -6379,44 +6388,65 @@ export class JMAPClient implements IJMAPClient { async getAllCalendars(): Promise { try { - const allCalendars: Calendar[] = []; - const primaryId = this.getCalendarsAccountId(); - const accountIds = this.getCalendarCapableAccountIds(); - - for (const accountId of accountIds) { - const isPrimary = accountId === primaryId; - if (!isPrimary && this.calendarAccessDenied.has(accountId)) continue; - const account = this.accounts[accountId]; - - try { - const response = await this.requestCalendars(accountId); - - if (response.methodResponses?.[0]?.[0] === "Calendar/get") { - const rawCalendars = (response.methodResponses[0][1].list || []) as Calendar[]; - const tasksOnly = await this.getTasksOnlyCalendarIds(accountId, rawCalendars.map((c) => c.id)); - const calendars = rawCalendars.map((cal) => ({ - ...cal, - id: isPrimary ? cal.id : `${accountId}:${cal.id}`, - originalId: cal.id, - accountId, - accountName: account?.name || (isPrimary ? this.username : accountId), - isShared: !isPrimary, - isTasksOnly: tasksOnly.has(cal.id), - })); - allCalendars.push(...calendars); - } - } catch (error) { - console.error(`Failed to fetch calendars for account ${accountId}:`, error); - } - } - - return allCalendars; + return (await this.getAllCalendarsWithFailures()).calendars; } catch (error) { console.error('Failed to fetch all calendars:', error); - return this.getCalendars(); + return []; } } + /** + * getAllCalendars() for callers that reconcile state against the list: a + * failure on the primary account throws instead of yielding a list without + * its calendars, and shared accounts that could not be loaded are named in + * `failedAccountIds`. Read as "no calendars", a failed fetch dropped every + * calendar from the persisted selection. + */ + async getAllCalendarsWithFailures(): Promise<{ calendars: Calendar[]; failedAccountIds: string[] }> { + const allCalendars: Calendar[] = []; + const failedAccountIds: string[] = []; + const primaryId = this.getCalendarsAccountId(); + const accountIds = this.getCalendarCapableAccountIds(); + + for (const accountId of accountIds) { + const isPrimary = accountId === primaryId; + if (!isPrimary && this.calendarAccessDenied.has(accountId)) continue; + const account = this.accounts[accountId]; + + try { + const response = await this.requestCalendars(accountId); + const [method, result] = response.methodResponses?.[0] ?? []; + + if (method !== "Calendar/get") { + // Shared accounts are probed on suspicion (see + // getCalendarCapableAccountIds); one that grants no calendar access + // has no calendars to lose. + if (!isPrimary && isCalendarAccessDenial(result?.type, result?.description)) continue; + throw new Error(result?.description || result?.type || "Calendar/get failed"); + } + + const rawCalendars = (result.list || []) as Calendar[]; + const tasksOnly = await this.getTasksOnlyCalendarIds(accountId, rawCalendars.map((c) => c.id)); + const calendars = rawCalendars.map((cal) => ({ + ...cal, + id: isPrimary ? cal.id : `${accountId}:${cal.id}`, + originalId: cal.id, + accountId, + accountName: account?.name || (isPrimary ? this.username : accountId), + isShared: !isPrimary, + isTasksOnly: tasksOnly.has(cal.id), + })); + allCalendars.push(...calendars); + } catch (error) { + if (isPrimary) throw error; + console.error(`Failed to fetch calendars for account ${accountId}:`, error); + failedAccountIds.push(accountId); + } + } + + return { calendars: allCalendars, failedAccountIds }; + } + async createCalendar(calendar: Partial, targetAccountId?: string, options?: CreateCalendarOptions): Promise { const accountId = targetAccountId || this.getCalendarsAccountId(); @@ -6913,8 +6943,7 @@ export class JMAPClient implements IJMAPClient { // calendar access at all. Remember the rejection and go quiet instead // of re-probing - and re-logging - on every range change. const type = (error as { jmapErrorType?: string } | null)?.jmapErrorType; - const denied = type === 'forbidden' || type === 'accountNotFound' || - /not have access/i.test(error instanceof Error ? error.message : ''); + const denied = isCalendarAccessDenial(type, error instanceof Error ? error.message : undefined); if (targetAccountId && denied) { this.calendarAccessDenied.add(targetAccountId); debug.log('calendar', `No calendar access to account ${targetAccountId} - skipping it from now on`); diff --git a/stores/__tests__/calendar-selection-after-fetch.test.ts b/stores/__tests__/calendar-selection-after-fetch.test.ts new file mode 100644 index 000000000..a0afa4654 --- /dev/null +++ b/stores/__tests__/calendar-selection-after-fetch.test.ts @@ -0,0 +1,117 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { useCalendarStore, reconcileSelectedIds } from '../calendar-store'; +import type { IJMAPClient } from '@/lib/jmap/client-interface'; +import type { Calendar } from '@/lib/jmap/types'; +import { BIRTHDAY_CALENDAR_ID } from '@/lib/birthday-calendar'; + +// The calendar selection is persisted and reconciled against every calendars +// fetch. A failed fetch used to come back as an empty list: every real +// calendar dropped out of the selection, the always-kept birthday calendar +// stopped the "nothing left -> show all" fallback, and the calendar stayed on +// "birthdays only" for good. + +function cal(overrides: Partial): Calendar { + return { + id: 'c', name: 'Cal', color: null, sortOrder: 0, isSubscribed: true, isVisible: true, + isDefault: false, includeInAvailability: 'all', timeZone: null, shareWith: null, + myRights: { + mayReadFreeBusy: true, mayReadItems: true, mayWriteAll: true, mayWriteOwn: true, + mayUpdatePrivate: true, mayRSVP: true, mayShare: true, mayDelete: true, + }, + ...overrides, + } as Calendar; +} + +function clientReturning(result: { calendars: Calendar[]; failedAccountIds?: string[] } | Error): IJMAPClient { + return { + getAllCalendarsWithFailures: vi.fn(async () => { + if (result instanceof Error) throw result; + return { calendars: result.calendars, failedAccountIds: result.failedAccountIds ?? [] }; + }), + } as unknown as IJMAPClient; +} + +const own = [cal({ id: 'a', accountId: 'me' }), cal({ id: 'b', accountId: 'me' })]; + +describe('calendar selection after a fetch', () => { + beforeEach(() => { + useCalendarStore.setState({ calendars: [], selectedCalendarIds: [], error: null }); + }); + + it('leaves the list and the selection alone when the fetch fails', async () => { + useCalendarStore.setState({ calendars: own, selectedCalendarIds: ['a', BIRTHDAY_CALENDAR_ID] }); + + await useCalendarStore.getState().fetchCalendars(clientReturning(new Error('Network error'))); + + const state = useCalendarStore.getState(); + expect(state.selectedCalendarIds).toEqual(['a', BIRTHDAY_CALENDAR_ID]); + expect(state.calendars).toBe(own); + expect(state.error).toBe('Failed to load calendars'); + }); + + it('falls back to the default when only the birthday calendar is left', async () => { + // The state the bug left behind: it heals on the next fetch. + useCalendarStore.setState({ selectedCalendarIds: [BIRTHDAY_CALENDAR_ID] }); + + await useCalendarStore.getState().fetchCalendars(clientReturning({ calendars: own })); + + expect(useCalendarStore.getState().selectedCalendarIds).toEqual(['a', 'b', BIRTHDAY_CALENDAR_ID]); + }); + + it('falls back to the default without adding the birthday calendar when it was not selected', async () => { + useCalendarStore.setState({ selectedCalendarIds: ['gone'] }); + + await useCalendarStore.getState().fetchCalendars(clientReturning({ calendars: own })); + + expect(useCalendarStore.getState().selectedCalendarIds).toEqual(['a', 'b']); + }); + + it('keeps a partial selection that still resolves', async () => { + useCalendarStore.setState({ selectedCalendarIds: ['b', BIRTHDAY_CALENDAR_ID] }); + + await useCalendarStore.getState().fetchCalendars(clientReturning({ calendars: own })); + + expect(useCalendarStore.getState().selectedCalendarIds).toEqual(['b', BIRTHDAY_CALENDAR_ID]); + }); + + it('keeps the selected calendars of a shared account that failed to load', async () => { + useCalendarStore.setState({ selectedCalendarIds: ['a', 'team:x', 'gone'] }); + + await useCalendarStore.getState().fetchCalendars(clientReturning({ calendars: own, failedAccountIds: ['team'] })); + + expect(useCalendarStore.getState().selectedCalendarIds).toEqual(['a', 'team:x']); + }); + + it('keeps the selected calendars of an aggregated account that failed to load', async () => { + // Both accounts have a calendar with raw id "b"; B's must not be remapped + // onto A's while B is unavailable. + useCalendarStore.setState({ selectedCalendarIds: ['A@h::b', 'B@h::b', 'B@h::shared:s'] }); + + await useCalendarStore.getState().fetchAllAccountsCalendars([ + { localAccountId: 'A@h', client: clientReturning({ calendars: [cal({ id: 'b', accountId: 'a1' })] }) }, + { localAccountId: 'B@h', client: clientReturning(new Error('Network error')) }, + ]); + + expect(useCalendarStore.getState().selectedCalendarIds).toEqual(['A@h::b', 'B@h::b', 'B@h::shared:s']); + }); + + it('keeps the selected calendars of a shared account behind an aggregated account', async () => { + useCalendarStore.setState({ selectedCalendarIds: ['A@h::b', 'A@h::team:x'] }); + + await useCalendarStore.getState().fetchAllAccountsCalendars([ + { localAccountId: 'A@h', client: clientReturning({ calendars: [cal({ id: 'b', accountId: 'a1' })], failedAccountIds: ['team'] }) }, + ]); + + expect(useCalendarStore.getState().selectedCalendarIds).toEqual(['A@h::b', 'A@h::team:x']); + }); +}); + +describe('reconcileSelectedIds with unloaded accounts', () => { + it('keeps ids under an unloaded prefix verbatim instead of remapping them by raw id', () => { + const calendars = [cal({ id: 'A@h::b', originalId: 'b', localAccountId: 'A@h' })]; + expect(reconcileSelectedIds(['B@h::b'], calendars, ['B@h::'])).toEqual(['B@h::b']); + // Without the prefix the id is remapped onto A's calendar (the raw-> + // namespaced migration path). + expect(reconcileSelectedIds(['B@h::b'], calendars)).toEqual(['A@h::b']); + }); +}); diff --git a/stores/calendar-store.ts b/stores/calendar-store.ts index 6819e8a74..70fb7f79d 100644 --- a/stores/calendar-store.ts +++ b/stores/calendar-store.ts @@ -275,10 +275,15 @@ async function refetchAfterOccurrenceMutation(): Promise { * namespaced transition (aggregation switched on, an account switch, or a * just-created calendar added with its raw id) changes the id string while the * calendar is the same. Remap by raw identity so the selection survives instead - * of silently resetting to "all". Keeps BIRTHDAY_CALENDAR_ID; drops ids that - * resolve to no calendar. + * of silently resetting to "all". Keeps BIRTHDAY_CALENDAR_ID and ids starting + * with one of `unloadedPrefixes` (calendars of an account that failed to load + * are unknown, not deleted); drops ids that resolve to no calendar. */ -export function reconcileSelectedIds(selectedCalendarIds: string[], calendars: Calendar[]): string[] { +export function reconcileSelectedIds( + selectedCalendarIds: string[], + calendars: Calendar[], + unloadedPrefixes: string[] = [], +): string[] { const byStoreId = new Set(calendars.map((c) => c.id)); const rawToStoreId = new Map(); for (const c of calendars) { @@ -290,6 +295,8 @@ export function reconcileSelectedIds(selectedCalendarIds: string[], calendars: C for (const id of selectedCalendarIds) { let mapped: string | undefined; if (id === BIRTHDAY_CALENDAR_ID || byStoreId.has(id)) mapped = id; + // Before the raw remap: another account may hold the same raw id. + else if (unloadedPrefixes.some((prefix) => id.startsWith(prefix))) mapped = id; else mapped = rawToStoreId.get(rawIdentityOf(id)); if (mapped && !seen.has(mapped)) { out.push(mapped); @@ -299,6 +306,23 @@ export function reconcileSelectedIds(selectedCalendarIds: string[], calendars: C return out; } +/** + * The selection to keep after a calendars fetch: the reconciled one, or the + * default when no real calendar is left in it. The virtual birthday calendar + * always survives reconciliation, so it must not count - counting it pinned + * the selection to "birthdays only" once the real calendars had dropped out. + */ +function selectionAfterFetch( + selectedCalendarIds: string[], + calendars: Calendar[], + defaultSelected: string[], + unloadedPrefixes?: string[], +): string[] { + const reconciled = reconcileSelectedIds(selectedCalendarIds, calendars, unloadedPrefixes); + if (reconciled.some((id) => id !== BIRTHDAY_CALENDAR_ID)) return reconciled; + return reconciled.includes(BIRTHDAY_CALENDAR_ID) ? [...defaultSelected, BIRTHDAY_CALENDAR_ID] : defaultSelected; +} + // In-flight refresh dedup. Concurrent callers (auto-interval + // manual refresh, two account-switch reloads, etc.) share the same // promise instead of double-fetching and racing the diff/import phase. @@ -626,16 +650,22 @@ export const useCalendarStore = create()( // alongside without blocking the grid. void get().fetchParticipantIdentities(client); try { - const calendars = await client.getAllCalendars(); - const { selectedCalendarIds } = get(); - const stillValid = reconcileSelectedIds(selectedCalendarIds, calendars); + // Throws when the primary account fails, so a failed fetch leaves the + // list and the selection as they were. + const { calendars, failedAccountIds } = await client.getAllCalendarsWithFailures(); // Default the visible selection to event calendars only - tasks-only // calendars stay out of the event grid. const defaultSelected = calendars.filter(c => !c.isTasksOnly).map(c => c.id); set({ calendars, isLoading: false, - selectedCalendarIds: stillValid.length > 0 ? stillValid : defaultSelected, + selectedCalendarIds: selectionAfterFetch( + get().selectedCalendarIds, + calendars, + defaultSelected, + // Shared calendars' ids are `:` (getAllCalendars). + failedAccountIds.map((accountId) => `${accountId}:`), + ), }); } catch (error) { debug.error('Failed to fetch calendars:', error); @@ -684,24 +714,32 @@ export const useCalendarStore = create()( fetchAllAccountsCalendars: async (accounts) => { set({ isLoading: true, error: null }); try { + // Id prefixes of the calendars that could not be loaded this time. + const unloadedPrefixes: string[] = []; const results = await Promise.all( accounts.map(async ({ client, localAccountId }) => { + const prefix = buildCrossAccountIdPrefix(localAccountId); try { - const list = await client.getAllCalendars(); + const { calendars: list, failedAccountIds } = await client.getAllCalendarsWithFailures(); + unloadedPrefixes.push(...failedAccountIds.map((accountId) => `${prefix}${accountId}:`)); return prefixCalendarsWithLocalAccount(list, localAccountId); } catch (error) { debug.error(`Failed to fetch calendars for account ${localAccountId}:`, error); + unloadedPrefixes.push(prefix); return [] as Calendar[]; } }), ); const calendars = results.flat(); - const { selectedCalendarIds } = get(); - const stillValid = reconcileSelectedIds(selectedCalendarIds, calendars); set({ calendars, isLoading: false, - selectedCalendarIds: stillValid.length > 0 ? stillValid : calendars.map(c => c.id), + selectedCalendarIds: selectionAfterFetch( + get().selectedCalendarIds, + calendars, + calendars.map(c => c.id), + unloadedPrefixes, + ), }); } catch (error) { debug.error('Failed to fetch all-account calendars:', error);