From c862909fddc3ec061346dddc767b97d9cd3b2c37 Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Mon, 2 Nov 2026 10:50:50 -0800 Subject: [PATCH] fix: respect single-call paging and preserve folders on page failures --- lib/__tests__/jmap-mailbox-pagination.test.ts | 31 ++++++++-- lib/jmap/client.ts | 38 +++++++++--- .../email-store-mailbox-pagination.test.ts | 61 +++++++++++++++++++ 3 files changed, 117 insertions(+), 13 deletions(-) create mode 100644 stores/__tests__/email-store-mailbox-pagination.test.ts diff --git a/lib/__tests__/jmap-mailbox-pagination.test.ts b/lib/__tests__/jmap-mailbox-pagination.test.ts index 461a2bee6..60cf0ea4c 100644 --- a/lib/__tests__/jmap-mailbox-pagination.test.ts +++ b/lib/__tests__/jmap-mailbox-pagination.test.ts @@ -131,7 +131,7 @@ describe('JMAP mailbox pagination', () => { expect(vi.mocked(client['request']).mock.calls[0][0].map(([name]) => name)).toEqual(['Mailbox/get', 'Mailbox/get']); }); - it('does not drop later accounts in a packed batch when one paginated account fails', async () => { + it('rejects a partial refresh when one paginated account fails', async () => { const client = createClient(2, { primary: { name: 'Self' }, shared: { name: 'Shared' } }); vi.spyOn(console, 'error').mockImplementation(() => {}); vi.spyOn(client as unknown as RequestTarget, 'request') @@ -140,9 +140,7 @@ describe('JMAP mailbox pagination', () => { ['Mailbox/get', { state: 'shared-state', list: [{ id: 'inbox', name: 'Shared Inbox' }] }, '1'], ] }) .mockRejectedValueOnce(new Error('page failed')); - const result = await client.getAllMailboxesWithState(); - expect(result.mailboxes.map(({ id }) => id)).toEqual(['shared:inbox']); - expect(result.states).toEqual({ shared: 'shared-state' }); + await expect(client.getAllMailboxesWithState()).rejects.toThrow('complete mailbox list'); }); it('pages when an unbounded get is rejected as tooManyObjects', async () => { @@ -154,4 +152,29 @@ describe('JMAP mailbox pagination', () => { ] }); expect((await client.getMailboxes()).map(({ id }) => id)).toEqual(['a', 'b', 'c']); }); + + it.each([false, true])('respects maxCallsInRequest=1 (shared: %s)', async (shared) => { + const client = createClient(2, shared ? { primary: { name: 'Self' }, shared: { name: 'Shared' } } : undefined); + Object.assign(client, { capabilities: { 'urn:ietf:params:jmap:core': { maxObjectsInGet: 2, maxCallsInRequest: 1 } } }); + const boxes = [{ id: 'a', name: 'A' }, { id: 'b', name: 'B' }, { id: 'c', name: 'C' }]; + const spy = vi.spyOn(client as unknown as RequestTarget, 'request').mockImplementation(async (calls) => { + expect(calls).toHaveLength(1); + const [name, args, tag] = calls[0]; + if (name === 'Mailbox/query') { + const ids = boxes.slice(args.position as number, (args.position as number) + 2).map(box => box.id); + return { methodResponses: [[name, { ids, queryState: 'q', total: 3 }, tag]] }; + } + const ids = args.ids as string[] | undefined; + expect(args['#ids']).toBeUndefined(); + const list = ids ? boxes.filter(box => ids.includes(box.id)) : boxes.slice(0, 2); + return { methodResponses: [[name, { list, state: 'm', notFound: [] }, tag]] }; + }); + const result = shared ? await client.getAllMailboxes() : await client.getMailboxes(); + expect(result).toHaveLength(shared ? 6 : 3); + const gets = spy.mock.calls.flatMap(([calls]) => calls).filter(([name, args]) => name === 'Mailbox/get' && args.ids); + for (const accountId of shared ? ['primary', 'shared'] : ['primary']) { + expect(gets.filter(([, args]) => args.accountId === accountId).map(([, args]) => args.ids)) + .toEqual([['a', 'b'], ['c']]); + } + }); }); diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index eb4ea01b8..1f02ee7df 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -2286,22 +2286,36 @@ export class JMAPClient implements IJMAPClient { let hasMorePages = true; while (hasMorePages) { - const response = await this.request([ - ["Mailbox/query", { - accountId, - position, - limit: this.getMaxObjectsInGet(), - calculateTotal: position === 0, - }, "query"], + const queryCall: JMAPMethodCall = ["Mailbox/query", { + accountId, + position, + limit: this.getMaxObjectsInGet(), + calculateTotal: position === 0, + }, "query"]; + const combined = this.getMaxCallsInRequest() > 1; + const response = await this.request(combined ? [queryCall, ["Mailbox/get", { accountId, properties: MAILBOX_PROPERTIES, "#ids": { resultOf: "query", name: "Mailbox/query", path: "/ids" }, }, "get"], - ], undefined, { skipOfflineRecord: true }); + ] : [queryCall], undefined, { skipOfflineRecord: true }); const queryResponse = response.methodResponses?.[0]; - const getResponse = response.methodResponses?.[1]; + let getResponse = response.methodResponses?.[1]; + if (!combined) { + const queriedIds = queryResponse?.[1]?.ids; + if (queryResponse?.[0] !== "Mailbox/query" || !Array.isArray(queriedIds) || + !queriedIds.every((id: unknown) => typeof id === "string")) { + throw new Error("Failed to load the complete mailbox list"); + } + // Result references cannot cross requests. Respect single-call servers + // by passing this page's IDs explicitly, still deferring cache writes. + const fetched = await this.request([["Mailbox/get", { + accountId, properties: MAILBOX_PROPERTIES, ids: queriedIds, + }, "get"]], undefined, { skipOfflineRecord: true }); + getResponse = fetched.methodResponses?.[0]; + } const ids = queryResponse?.[1]?.ids; const pageQueryState = queryResponse?.[1]?.queryState; const page = getResponse?.[1]?.list; @@ -2417,6 +2431,7 @@ export class JMAPClient implements IJMAPClient { } let fetchFailed = false; + let paginationFailed = false; // One Mailbox/get per account, packed maxCallsInRequest to a request and // a few requests in flight at once. One request per account, one after @@ -2446,6 +2461,7 @@ export class JMAPClient implements IJMAPClient { match = complete.methodResponses[0]; } catch (error) { fetchFailed = true; + paginationFailed = true; console.error(`Failed to fetch all mailboxes for account ${accountId}:`, error); continue; } @@ -2486,6 +2502,10 @@ export class JMAPClient implements IJMAPClient { ); await Promise.all(workers); + // The store replaces the whole folder list on a successful refresh. + // Never publish sibling accounts as a complete list after paging fails. + if (paginationFailed) throw new Error('Failed to load the complete mailbox list'); + // Accounts keep the session's order, whichever batch answered first. for (const mailboxes of perAccount) allMailboxes.push(...mailboxes); diff --git a/stores/__tests__/email-store-mailbox-pagination.test.ts b/stores/__tests__/email-store-mailbox-pagination.test.ts new file mode 100644 index 000000000..cb04d9659 --- /dev/null +++ b/stores/__tests__/email-store-mailbox-pagination.test.ts @@ -0,0 +1,61 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { JMAPClient } from '@/lib/jmap/client'; +import type { Mailbox } from '@/lib/jmap/types'; +import { useEmailStore } from '../email-store'; + +const initialState = useEmailStore.getState(); +type MethodCall = [string, Record, string]; +type RequestTarget = { request(calls: MethodCall[]): Promise }; + +beforeEach(() => useEmailStore.setState(initialState, true)); +afterEach(() => { + vi.restoreAllMocks(); + useEmailStore.setState(initialState, true); +}); + +describe('failed mailbox pagination during a store refresh', () => { + it('preserves the entire folder list, collection states, and selection when another account succeeds', async () => { + const client = new JMAPClient('https://jmap.example.com', 'user@example.com', 'pass'); + Object.assign(client, { + apiUrl: 'https://jmap.example.com/api', + accountId: 'primary', + accounts: { primary: { name: 'Self' }, shared: { name: 'Shared' } }, + capabilities: { 'urn:ietf:params:jmap:core': { maxObjectsInGet: 2 } }, + }); + const previousMailboxes = [ + { id: 'inbox', name: 'Inbox', role: 'inbox', accountId: 'primary', isShared: false }, + { id: 'child', name: 'Selected folder', parentId: 'inbox', accountId: 'primary', isShared: false }, + { id: 'shared:inbox', originalId: 'inbox', name: 'Shared Inbox', accountId: 'shared', isShared: true }, + ] as Mailbox[]; + const previousStates = { primary: 'old-primary', shared: 'old-shared' }; + useEmailStore.setState({ + mailboxes: previousMailboxes, + mailboxSyncStates: previousStates, + selectedMailbox: 'child', + isLoading: false, + error: null, + }); + vi.spyOn(console, 'error').mockImplementation(() => {}); + const request = vi.spyOn(client as unknown as RequestTarget, 'request') + .mockResolvedValueOnce({ methodResponses: [ + ['Mailbox/get', { list: [{ id: 'inbox' }, { id: 'child' }], state: 'new-primary' }, '0'], + ['Mailbox/get', { list: [{ id: 'inbox', name: 'Shared Inbox', role: 'inbox' }], state: 'new-shared' }, '1'], + ] }) + .mockResolvedValueOnce({ methodResponses: [ + ['Mailbox/query', { ids: ['inbox', 'child'], queryState: 'q', total: 3 }, 'query'], + ['Mailbox/get', { list: [{ id: 'inbox', name: 'Inbox' }, { id: 'child', name: 'Selected folder' }], state: 'new-primary', notFound: [] }, 'get'], + ] }) + .mockRejectedValueOnce(new Error('Later mailbox page failed')); + + await useEmailStore.getState().fetchMailboxes(client); + + expect(request).toHaveBeenCalledTimes(3); + expect(request.mock.calls[2][0][0][1].position).toBe(2); + const state = useEmailStore.getState(); + expect(state.mailboxes).toBe(previousMailboxes); + expect(state.mailboxSyncStates).toBe(previousStates); + expect(state.selectedMailbox).toBe('child'); + expect(state.isLoading).toBe(false); + expect(state.error).toBeTruthy(); + }); +});