From 2262b2baf15a6c5aa68cbe7a02386b4abb8d5ea6 Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Mon, 2 Nov 2026 09:06:27 -0800 Subject: [PATCH 1/3] fix: paginate mailbox retrieval without sidebar changes --- lib/__tests__/jmap-mailbox-pagination.test.ts | 157 ++++++++++++++++++ lib/jmap/client.ts | 146 +++++++++++----- lib/offline/__tests__/client-fallback.test.ts | 94 ++++++++++- lib/offline/responder.ts | 20 ++- 4 files changed, 378 insertions(+), 39 deletions(-) create mode 100644 lib/__tests__/jmap-mailbox-pagination.test.ts diff --git a/lib/__tests__/jmap-mailbox-pagination.test.ts b/lib/__tests__/jmap-mailbox-pagination.test.ts new file mode 100644 index 000000000..461a2bee6 --- /dev/null +++ b/lib/__tests__/jmap-mailbox-pagination.test.ts @@ -0,0 +1,157 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { JMAPClient } from '../jmap/client'; + +type RawMailbox = { id: string; name: string; parentId?: string }; +type MethodCall = [string, Record, string]; +type RequestTarget = { request(calls: MethodCall[]): Promise }; + +function createClient( + maxObjectsInGet = 2, + accounts: Record = { primary: { name: 'user@example.com' } }, +): JMAPClient { + const client = new JMAPClient('https://jmap.example.com', 'user@example.com', 'pass'); + Object.assign(client, { + apiUrl: 'https://jmap.example.com/api', + accountId: 'primary', + username: 'user@example.com', + accounts, + capabilities: { 'urn:ietf:params:jmap:core': { maxObjectsInGet } }, + }); + return client; +} + +function serve(client: JMAPClient, mailboxes: Record, clamp = Infinity, withTotal = true): MethodCall[][] { + const requests: MethodCall[][] = []; + vi.spyOn(client as unknown as RequestTarget, 'request').mockImplementation(async (calls) => { + if (calls[0][0] === 'Mailbox/get') { + return { methodResponses: calls.map(([, args, tag]) => [ + 'Mailbox/get', { list: (mailboxes[args.accountId as string] ?? []).slice(0, client.getMaxObjectsInGet()), state: 'm' }, tag, + ]) }; + } + requests.push(calls); + const accountId = calls[0][1].accountId as string; + const position = calls[0][1].position as number; + const limit = calls[0][1].limit as number; + const all = mailboxes[accountId] ?? []; + const page = all.slice(position, position + Math.min(limit, clamp)); + return { methodResponses: [ + ['Mailbox/query', { ids: page.map(({ id }) => id), queryState: 's', ...(withTotal && calls[0][1].calculateTotal ? { total: all.length } : {}) }, 'query'], + ['Mailbox/get', { list: page, state: 'm', notFound: [] }, 'get'], + ] }; + }); + return requests; +} + +afterEach(() => vi.restoreAllMocks()); + +describe('JMAP mailbox pagination', () => { + it('loads a complete hierarchy across server-clamped pages', async () => { + const client = createClient(2); + const requests = serve(client, { primary: [ + { id: 'grandchild', name: 'Grandchild', parentId: 'child' }, + { id: 'other', name: 'Other' }, + { id: 'child', name: 'Child', parentId: 'root' }, + { id: 'root', name: 'Root' }, + ] }, 1); + + const mailboxes = await client.getMailboxes(); + + expect(mailboxes.map(({ id, parentId }) => [id, parentId])).toEqual([ + ['grandchild', 'child'], ['other', undefined], ['child', 'root'], ['root', undefined], + ]); + expect(requests.map(([query]) => query[1].position)).toEqual([0, 1, 2, 3]); + expect(requests.map(([query]) => query[1].calculateTotal)).toEqual([true, false, false, false]); + expect(requests.every((calls) => calls.length === 2 && calls[1][1]['#ids'] && !calls[1][1].ids)).toBe(true); + }); + + it.each([ + ['missing ids', undefined, 's', []], + ['mixed ids', ['third', 4], 's', []], + ['duplicate ids', ['parent'], 's', [{ id: 'parent', name: 'Parent' }]], + ['changed state', ['third'], 's2', [{ id: 'third', name: 'Third' }]], + ['early empty page', [], 's', []], + ['missing object', ['third'], 's', []], + ])('rejects %s instead of publishing a partial hierarchy', async (_label, ids, queryState, list) => { + const client = createClient(); + vi.spyOn(console, 'error').mockImplementation(() => {}); + vi.spyOn(client as unknown as RequestTarget, 'request') + .mockResolvedValueOnce({ methodResponses: [ + ['Mailbox/get', { list: [{ id: 'child' }, { id: 'parent' }] }, '0'], + ] }) + .mockResolvedValueOnce({ methodResponses: [ + ['Mailbox/query', { ids: ['child', 'parent'], queryState: 's', total: 3 }, 'query'], + ['Mailbox/get', { list: [{ id: 'child', name: 'Child' }, { id: 'parent', name: 'Parent' }] }, 'get'], + ] }) + .mockResolvedValueOnce({ methodResponses: [ + ['Mailbox/query', { ids, queryState }, 'query'], + ['Mailbox/get', { list }, 'get'], + ] }); + + await expect(client.getMailboxes()).rejects.toThrow(); + }); + + it('prefixes shared mailbox and parent ids', async () => { + const client = createClient(1, { primary: { name: 'user@example.com' }, shared: { name: 'team@example.com' } }); + serve(client, { + primary: [{ id: 'inbox', name: 'Inbox' }], + shared: [{ id: 'child', name: 'Child', parentId: 'parent' }, { id: 'parent', name: 'Parent' }], + }); + + expect((await client.getAllMailboxes()).find(({ name }) => name === 'Child')).toMatchObject({ + id: 'shared:child', originalId: 'child', parentId: 'shared:parent', accountId: 'shared', isShared: true, + }); + }); + + it('keeps small accounts on the existing single-request fast path', async () => { + const client = createClient(3); + const requests = serve(client, { primary: [{ id: 'inbox', name: 'Inbox' }] }); + expect(await client.getMailboxes()).toHaveLength(1); + expect(requests).toHaveLength(0); + expect(client['request']).toHaveBeenCalledTimes(1); + }); + + it.each([true, false])('loads an exact page multiple (total returned: %s)', async (withTotal) => { + const client = createClient(2); + const list = Array.from({ length: 4 }, (_, i) => ({ id: String(i), name: String(i) })); + const requests = serve(client, { primary: list }, Infinity, withTotal); + expect(await client.getMailboxes()).toHaveLength(4); + expect(requests.map(([query]) => query[1].position)).toEqual(withTotal ? [0, 2] : [0, 2, 4]); + expect(requests.map(([query]) => query[1].calculateTotal)).toEqual(withTotal ? [true, false] : [true, false, false]); + }); + + it('retains Mailbox collection state and packed small-account retrieval', async () => { + const client = createClient(2, { primary: { name: 'Self' }, shared: { name: 'Shared' } }); + serve(client, { + primary: [{ id: 'inbox', name: 'Inbox' }], + shared: [{ id: 'parent', name: 'Parent' }, { id: 'child', name: 'Child', parentId: 'parent' }, { id: 'other', name: 'Other' }], + }); + const result = await client.getAllMailboxesWithState(); + expect(result.mailboxes.map(({ id }) => id)).toEqual(['inbox', 'shared:parent', 'shared:child', 'shared:other']); + expect(result.states).toEqual({ primary: 'm', shared: 'm' }); + 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 () => { + const client = createClient(2, { primary: { name: 'Self' }, shared: { name: 'Shared' } }); + vi.spyOn(console, 'error').mockImplementation(() => {}); + vi.spyOn(client as unknown as RequestTarget, 'request') + .mockResolvedValueOnce({ methodResponses: [ + ['Mailbox/get', { list: [{ id: 'a' }, { id: 'b' }] }, '0'], + ['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' }); + }); + + it('pages when an unbounded get is rejected as tooManyObjects', async () => { + const client = createClient(); + const list = [{ id: 'a', name: 'A' }, { id: 'b', name: 'B' }, { id: 'c', name: 'C' }]; + serve(client, { primary: list }); + vi.mocked(client['request']).mockResolvedValueOnce({ methodResponses: [ + ['error', { type: 'tooManyObjects' }, '0'], + ] }); + expect((await client.getMailboxes()).map(({ id }) => id)).toEqual(['a', 'b', 'c']); + }); +}); diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index 4893af357..eb4ea01b8 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -2159,11 +2159,13 @@ export class JMAPClient implements IJMAPClient { } /** + * `skipOfflineRecord`: mailbox pagination records the complete list once, + * never a capped initial response or an individual page. * `transfer`: a mail import or export's own request. A 429 throws * RateLimitError for the transfer to wait on, without pausing the rest of * the client, and a pause the client is in doesn't hold the transfer up. */ - private async request(methodCalls: JMAPMethodCall[], using?: string[], opts?: { transfer?: boolean }): Promise { + private async request(methodCalls: JMAPMethodCall[], using?: string[], opts?: { transfer?: boolean; skipOfflineRecord?: boolean }): Promise { if (!this.apiUrl) { throw new Error('Not connected. Call connect() first.'); } @@ -2193,7 +2195,7 @@ export class JMAPClient implements IJMAPClient { } if (offline) { offline.noteNetwork(this, true); - offline.record(this, methodCalls, answer); + if (!opts?.skipOfflineRecord) offline.record(this, methodCalls, answer); } return answer; } @@ -2270,37 +2272,98 @@ export class JMAPClient implements IJMAPClient { } } + /** + * Mailbox/get without ids can silently stop at maxObjectsInGet, leaving + * children without their parents. Querying all ids keeps the tree complete. + */ + private async fetchMailboxTree(accountId: string): Promise { + const mailboxes: JMAPMailbox[] = []; + const seenIds = new Set(); + let position = 0; + let total: number | undefined; + let queryState: string | undefined; + let mailboxState: string | undefined; + + let hasMorePages = true; + while (hasMorePages) { + const response = await this.request([ + ["Mailbox/query", { + accountId, + position, + limit: this.getMaxObjectsInGet(), + calculateTotal: position === 0, + }, "query"], + ["Mailbox/get", { + accountId, + properties: MAILBOX_PROPERTIES, + "#ids": { resultOf: "query", name: "Mailbox/query", path: "/ids" }, + }, "get"], + ], undefined, { skipOfflineRecord: true }); + + const queryResponse = response.methodResponses?.[0]; + const getResponse = response.methodResponses?.[1]; + const ids = queryResponse?.[1]?.ids; + const pageQueryState = queryResponse?.[1]?.queryState; + const page = getResponse?.[1]?.list; + const pageMailboxState = getResponse?.[1]?.state; + + if ( + queryResponse?.[0] !== "Mailbox/query" || + !Array.isArray(ids) || + !ids.every((id: unknown) => typeof id === "string") || + typeof pageQueryState !== "string" || + getResponse?.[0] !== "Mailbox/get" || + !Array.isArray(page) || + getResponse[1].notFound?.length || + page.length !== ids.length + ) { + throw new Error("Failed to load the complete mailbox list"); + } + + if (queryState !== undefined && pageQueryState !== queryState) { + throw new Error("Mailbox list changed while it was being loaded"); + } + if (mailboxState !== undefined && pageMailboxState !== undefined && pageMailboxState !== mailboxState) { + throw new Error("Mailbox list changed while it was being loaded"); + } + queryState = pageQueryState; + if (typeof pageMailboxState === "string") mailboxState = pageMailboxState; + for (const id of ids) { + if (seenIds.has(id)) throw new Error("Mailbox/query returned duplicate ids"); + seenIds.add(id); + } + mailboxes.push(...page); + if (typeof queryResponse[1].total === "number") total = queryResponse[1].total; + position += ids.length; + hasMorePages = ids.length > 0 && (total === undefined || position < total); + } + + if (total !== undefined && seenIds.size !== total) { + throw new Error(`Mailbox/query returned ${seenIds.size} of ${total} mailboxes`); + } + + return { methodResponses: [["Mailbox/get", { list: mailboxes, state: mailboxState }, "0"]] }; + } + async getMailboxes(accountId?: string): Promise { const acctId = accountId || this.accountId; try { - const response = await this.request([ - ["Mailbox/get", { accountId: acctId, properties: MAILBOX_PROPERTIES }, "0"] - ]); + const calls: JMAPMethodCall[] = [ + ["Mailbox/get", { accountId: acctId, properties: MAILBOX_PROPERTIES }, "0"], + ]; + let response = await this.request(calls, undefined, { skipOfflineRecord: true }); + const first = response.methodResponses?.[0]; + if ((first?.[0] === "Mailbox/get" && first[1].list?.length >= this.getMaxObjectsInGet()) || + (first?.[0] === "error" && first[1].type === "tooManyObjects")) { + response = await this.fetchMailboxTree(acctId); + } + getOfflineHooks()?.record(this, calls, response); if (response.methodResponses?.[0]?.[0] === "Mailbox/get") { const rawMailboxes = (response.methodResponses[0][1].list || []) as JMAPMailbox[]; debug.log('jmap', `[JMAP Mailbox] getMailboxes returned ${rawMailboxes.length} mailboxes for account ${acctId}`); - // Warn if response might be truncated - const maxObjects = this.getMaxObjectsInGet(); - if (rawMailboxes.length >= maxObjects) { - debug.warn('jmap', - `[JMAP Mailbox] Response contains ${rawMailboxes.length} mailboxes which equals maxObjectsInGet (${maxObjects}). ` + - `Some mailboxes may be missing - nested folders could appear orphaned at root level.` - ); - } - - // Log parentId references to detect potential orphans - const returnedIds = new Set(rawMailboxes.map(mb => mb.id)); - const missingParents = rawMailboxes.filter(mb => mb.parentId && !returnedIds.has(mb.parentId)); - if (missingParents.length > 0) { - debug.warn('jmap', - `[JMAP Mailbox] ${missingParents.length} mailbox(es) reference parentId not in response (will be orphaned):`, - missingParents.map(mb => ({ id: mb.id, name: mb.name, parentId: mb.parentId })) - ); - } - return rawMailboxes.map((mb) => ({ id: mb.id, originalId: undefined, @@ -2370,14 +2433,31 @@ export class JMAPClient implements IJMAPClient { "Mailbox/get", { accountId, properties: MAILBOX_PROPERTIES }, String(i), - ])); + ]), undefined, { skipOfflineRecord: true }); - batch.forEach((accountId, i) => { - const match = response.methodResponses?.find((r) => r[2] === String(i)); + for (const [i, accountId] of batch.entries()) { + let match = response.methodResponses?.find((r) => r[2] === String(i)); + // Keep the packed fast path for small accounts. A full-sized result + // may be capped, so reload that account through query/get pages. + if ((match?.[0] === "Mailbox/get" && Array.isArray(match[1].list) && match[1].list.length >= maxObjects) || + (match?.[0] === "error" && match[1].type === "tooManyObjects")) { + try { + const complete = await this.fetchMailboxTree(accountId); + match = complete.methodResponses[0]; + } catch (error) { + fetchFailed = true; + console.error(`Failed to fetch all mailboxes for account ${accountId}:`, error); + continue; + } + } if (match?.[0] !== "Mailbox/get") { if (match) debug.warn('jmap', `[JMAP Mailbox] getAllMailboxes: account ${accountId} answered ${match[0]}`, match[1]); - return; + continue; } + getOfflineHooks()?.record(this, + [["Mailbox/get", { accountId, properties: MAILBOX_PROPERTIES }, "0"]], + { methodResponses: [["Mailbox/get", match[1], "0"]] }, + ); const rawMailboxes = (match[1].list || []) as JMAPMailbox[]; const mailboxState = (match[1] as { state?: unknown }).state; if (typeof mailboxState === 'string') { @@ -2386,16 +2466,8 @@ export class JMAPClient implements IJMAPClient { debug.log('jmap', `[JMAP Mailbox] getAllMailboxes: account ${accountId} returned ${rawMailboxes.length} mailboxes (isPrimary: ${accountId === this.accountId})`); - // Warn if response might be truncated - if (rawMailboxes.length >= maxObjects) { - debug.warn('jmap', - `[JMAP Mailbox] Account ${accountId}: response contains ${rawMailboxes.length} mailboxes which equals maxObjectsInGet (${maxObjects}). ` + - `Some mailboxes may be missing.` - ); - } - perAccount[offset + i] = rawMailboxes.map((mb) => this.mapRawMailbox(mb, accountId)); - }); + } } catch (error) { fetchFailed = true; console.error(`Failed to fetch mailboxes for accounts ${batch.join(', ')}:`, error); diff --git a/lib/offline/__tests__/client-fallback.test.ts b/lib/offline/__tests__/client-fallback.test.ts index d710d1c77..f7d49e49b 100644 --- a/lib/offline/__tests__/client-fallback.test.ts +++ b/lib/offline/__tests__/client-fallback.test.ts @@ -6,7 +6,7 @@ import { useAccountStore } from '@/stores/account-store'; import { usePolicyStore } from '@/stores/policy-store'; import { DEFAULT_ORGANISATION_POLICY } from '@/lib/admin/organisation-policy'; import { startOfflineMode, wipeEverything } from '../offline-mode'; -import { forgetIndexes, putEmails, putMisc, emailIndex } from '../cache'; +import { forgetIndexes, putEmails, putMisc, getMisc, emailIndex } from '../cache'; import { forgetDeviceKey } from '../crypto'; import { allowOfflineDbCreation, deleteOfflineDb } from '../db'; import { useOfflineStore, writeEnabled } from '../state'; @@ -88,6 +88,98 @@ describe('falling back to the device', () => { }); }); +describe('mailbox paging with the device cache', () => { + const mailboxes = Array.from({ length: 5 }, (_, i) => ({ id: `box-${i}`, name: `Folder ${i}`, parentId: null })); + + function mailboxServer(failLaterPage = false) { + const positions: number[] = []; + vi.stubGlobal('fetch', vi.fn(async (url: string, init?: RequestInit) => { + if (String(url).endsWith('/.well-known/jmap')) { + return new Response(JSON.stringify({ + ...SESSION, + capabilities: { 'urn:ietf:params:jmap:core': { maxObjectsInGet: 2 } }, + }), { status: 200 }); + } + const { methodCalls } = JSON.parse(String(init?.body)); + const query = methodCalls.find(([name]: [string]) => name === 'Mailbox/query'); + const position = query?.[1].position ?? 0; + if (query) positions.push(position); + const page = mailboxes.slice(position, position + 2); + const methodResponses = methodCalls.map(([name, , id]: [string, unknown, string]) => { + if (name === 'Identity/get') return [name, { accountId: 'j1', list: [{ id: 'recording-barrier' }] }, id]; + if (query && failLaterPage && position > 0) return ['error', { type: 'serverFail' }, id]; + if (name === 'Mailbox/query') return [name, { + accountId: 'j1', queryState: 'query-state', position, + ids: page.map((box) => box.id), total: mailboxes.length, + }, id]; + return [name, { accountId: 'j1', state: 'mailbox-state', list: page, notFound: [] }, id]; + }); + return new Response(JSON.stringify({ methodResponses }), { status: 200 }); + })); + return positions; + } + + // Identity recording shares the per-account queue with mailbox recording. + // Waiting for this marker also detects erroneous writes queued by failed pages. + async function recorded(client: JMAPClient) { + await client.getIdentities(); + await vi.waitFor(async () => { + expect(await getMisc('acc', 'identities')).toEqual({ j1: [{ id: 'recording-barrier' }] }); + }); + } + + it('returns a cached list larger than maxObjectsInGet without inventing a collection state', async () => { + mailboxServer(); + const client = await connectedClient(); + writeEnabled(['acc']); + const stop = startOfflineMode(); + try { + await putMisc('acc', 'mailboxes', { j1: mailboxes }); + vi.stubGlobal('navigator', { onLine: false }); + const result = await client.getAllMailboxesWithState(); + expect(result.mailboxes.map((box) => box.id)).toEqual(mailboxes.map((box) => box.id)); + expect(result.states).toEqual({}); + // Only session discovery touched the network; both query/get pages are local. + expect(fetch).toHaveBeenCalledTimes(1); + } finally { + stop(); + } + }); + + it('records the complete live multi-page list rather than its final page', async () => { + const positions = mailboxServer(); + const client = await connectedClient(); + writeEnabled(['acc']); + const stop = startOfflineMode(); + try { + const result = await client.getMailboxes(); + expect(result.map((box) => box.id)).toEqual(mailboxes.map((box) => box.id)); + expect(positions).toEqual([0, 2, 4]); + await recorded(client); + expect(await getMisc('acc', 'mailboxes')).toEqual({ j1: mailboxes }); + } finally { + stop(); + } + }); + + it('retains the previous complete cache when a later live page fails', async () => { + const positions = mailboxServer(true); + const client = await connectedClient(); + writeEnabled(['acc']); + const stop = startOfflineMode(); + const previous = [{ id: 'old', name: 'Previously cached', parentId: null }]; + try { + await putMisc('acc', 'mailboxes', { j1: previous }); + await expect(client.getMailboxes()).rejects.toThrow('Failed to load the complete mailbox list'); + expect(positions).toEqual([0, 2]); + await recorded(client); + expect(await getMisc('acc', 'mailboxes')).toEqual({ j1: previous }); + } finally { + stop(); + } + }); +}); + describe('the sync', () => { function fakeClient(opts: { failQuery?: boolean }) { return { diff --git a/lib/offline/responder.ts b/lib/offline/responder.ts index d0d4ac7e7..9495ee9a2 100644 --- a/lib/offline/responder.ts +++ b/lib/offline/responder.ts @@ -9,7 +9,7 @@ * is refused, as are calls the cache can't answer (search, changes since a * state): the whole request then fails as it would without offline mode. * - * Answers carry no state strings: a made-up one would end up in an + * Answers carry no collection state strings: a made-up one would end up in an * Email/changes once the server is back, which refuses it. */ @@ -267,6 +267,23 @@ async function listGet(ctx: ResponderContext, args: Record, nam }; } +/** Unfiltered mailbox pages used to load a complete folder list offline. */ +async function mailboxQuery(ctx: ResponderContext, args: Record): Promise { + if (args.filter || args.sort || args.anchor || args.sortAsTree) throw new Unanswerable(); + const { list } = await listGet(ctx, { accountId: args.accountId }, 'mailboxes'); + const mailboxes = list as Record[]; + const position = typeof args.position === 'number' ? args.position : 0; + const limit = typeof args.limit === 'number' ? args.limit : mailboxes.length; + return { + accountId: (args.accountId as string) || ctx.defaultJmap, + // A query token only, never a Mailbox collection state for /changes. + queryState: JSON.stringify(mailboxes), + position, + ids: mailboxes.slice(position, position + limit).map((mailbox) => mailbox.id), + ...(args.calculateTotal ? { total: mailboxes.length } : {}), + }; +} + /** Whether a PatchObject only changes what offline mode may queue. */ function queueablePatch(patch: Record): boolean { return Object.keys(patch).length > 0 && Object.keys(patch).every((path) => @@ -361,6 +378,7 @@ export async function answerOffline(ctx: ResponderContext, methodCalls: OfflineM case 'Email/query': result = await emailQuery(ctx, args); break; case 'Email/get': result = await emailGet(ctx, args); break; case 'Thread/get': result = await threadGet(ctx, args); break; + case 'Mailbox/query': result = await mailboxQuery(ctx, args); break; case 'Mailbox/get': result = await listGet(ctx, args, 'mailboxes'); break; case 'Identity/get': result = await listGet(ctx, args, 'identities'); break; case 'Email/set': result = await emailSet(ctx, args); break; From c862909fddc3ec061346dddc767b97d9cd3b2c37 Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Mon, 2 Nov 2026 10:50:50 -0800 Subject: [PATCH 2/3] 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(); + }); +}); From 8ef3ad901ebe8274898ffc99cfb9ceef47a32458 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Mon, 2 Nov 2026 20:18:49 +0100 Subject: [PATCH 3/3] fix: page large folder lists on stalwart 1.0 and retry when mail arrives mid-load #1502 --- lib/__tests__/jmap-mailbox-pagination.test.ts | 56 ++++++++++++++++++- lib/jmap/client.ts | 37 ++++++++++-- 2 files changed, 84 insertions(+), 9 deletions(-) diff --git a/lib/__tests__/jmap-mailbox-pagination.test.ts b/lib/__tests__/jmap-mailbox-pagination.test.ts index 60cf0ea4c..4dea54e23 100644 --- a/lib/__tests__/jmap-mailbox-pagination.test.ts +++ b/lib/__tests__/jmap-mailbox-pagination.test.ts @@ -68,7 +68,6 @@ describe('JMAP mailbox pagination', () => { ['missing ids', undefined, 's', []], ['mixed ids', ['third', 4], 's', []], ['duplicate ids', ['parent'], 's', [{ id: 'parent', name: 'Parent' }]], - ['changed state', ['third'], 's2', [{ id: 'third', name: 'Third' }]], ['early empty page', [], 's', []], ['missing object', ['third'], 's', []], ])('rejects %s instead of publishing a partial hierarchy', async (_label, ids, queryState, list) => { @@ -143,16 +142,67 @@ describe('JMAP mailbox pagination', () => { await expect(client.getAllMailboxesWithState()).rejects.toThrow('complete mailbox list'); }); - it('pages when an unbounded get is rejected as tooManyObjects', async () => { + it('pages when an unbounded get is rejected as requestTooLarge (Stalwart 1.0)', async () => { const client = createClient(); const list = [{ id: 'a', name: 'A' }, { id: 'b', name: 'B' }, { id: 'c', name: 'C' }]; serve(client, { primary: list }); vi.mocked(client['request']).mockResolvedValueOnce({ methodResponses: [ - ['error', { type: 'tooManyObjects' }, '0'], + ['error', { type: 'requestTooLarge' }, '0'], ] }); expect((await client.getMailboxes()).map(({ id }) => id)).toEqual(['a', 'b', 'c']); }); + it('pages a shared account whose packed get is rejected as requestTooLarge', async () => { + const client = createClient(2, { primary: { name: 'Self' }, shared: { name: 'Shared' } }); + const shared = [{ id: 'a', name: 'A' }, { id: 'b', name: 'B', parentId: 'a' }, { id: 'c', name: 'C', parentId: 'b' }]; + serve(client, { primary: [{ id: 'inbox', name: 'Inbox' }], shared }); + vi.mocked(client['request']).mockResolvedValueOnce({ methodResponses: [ + ['Mailbox/get', { list: [{ id: 'inbox', name: 'Inbox' }], state: 'm' }, '0'], + ['error', { type: 'requestTooLarge' }, '1'], + ] }); + const result = await client.getAllMailboxesWithState(); + expect(result.mailboxes.map(({ id }) => id)).toEqual(['inbox', 'shared:a', 'shared:b', 'shared:c']); + expect(result.states).toEqual({ primary: 'm', shared: 'm' }); + }); + + it('starts over when the mailbox state moves between pages', async () => { + const client = createClient(); + const page = (ids: string[], queryState: string, total?: number) => ({ methodResponses: [ + ['Mailbox/query', { ids, queryState, ...(total ? { total } : {}) }, 'query'], + ['Mailbox/get', { list: ids.map((id) => ({ id, name: id })), state: queryState }, 'get'], + ] }); + const request = vi.spyOn(client as unknown as RequestTarget, 'request') + .mockResolvedValueOnce({ methodResponses: [['Mailbox/get', { list: [{ id: 'a' }, { id: 'b' }] }, '0']] }) + .mockResolvedValueOnce(page(['a', 'b'], 's1', 3)) + .mockResolvedValueOnce(page(['c'], 's2')) + .mockResolvedValueOnce(page(['a', 'b'], 's2', 3)) + .mockResolvedValueOnce(page(['c'], 's2')); + + const result = await client.getAllMailboxesWithState(); + expect(result.mailboxes.map(({ id }) => id)).toEqual(['a', 'b', 'c']); + expect(result.states).toEqual({ primary: 's2' }); + expect(request).toHaveBeenCalledTimes(5); + }); + + it('gives up when the mailbox state keeps moving', async () => { + const client = createClient(); + vi.spyOn(console, 'error').mockImplementation(() => {}); + let n = 0; + const request = vi.spyOn(client as unknown as RequestTarget, 'request').mockImplementation(async (calls) => { + if (calls[0][0] === 'Mailbox/get') return { methodResponses: [['Mailbox/get', { list: [{ id: 'a' }, { id: 'b' }] }, '0']] }; + const position = calls[0][1].position as number; + const ids = position === 0 ? ['a', 'b'] : ['c']; + const state = `s${n++}`; + return { methodResponses: [ + ['Mailbox/query', { ids, queryState: state, ...(position === 0 ? { total: 3 } : {}) }, 'query'], + ['Mailbox/get', { list: ids.map((id) => ({ id, name: id })), state }, 'get'], + ] }; + }); + + await expect(client.getMailboxes()).rejects.toThrow('changed while it was being loaded'); + expect(request).toHaveBeenCalledTimes(1 + 3 * 2); + }); + 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 } } }); diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index 1f02ee7df..a5168fc8a 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -603,6 +603,23 @@ const MAILBOX_PROPERTIES = [ "isSubscribed", ] as const; +// A Mailbox/get without ids over maxObjectsInGet: Stalwart 0.16 cuts the list +// at the limit without saying so, 1.0 answers requestTooLarge (RFC 8620 §5.1). +function mailboxListCapped(response: JMAPResponse["methodResponses"][number] | undefined, maxObjects: number): boolean { + if (response?.[0] === "Mailbox/get") return Array.isArray(response[1].list) && response[1].list.length >= maxObjects; + return response?.[0] === "error" && response[1].type === "requestTooLarge"; +} + +// Every new message or flag change moves the Mailbox state (the counts), so a +// paged load is retried a few times before the refresh gives up. +const MAILBOX_TREE_ATTEMPTS = 3; + +class MailboxListChangedError extends Error { + constructor() { + super("Mailbox list changed while it was being loaded"); + } +} + // SieveScript properties (RFC 9661 §2), explicit for the same reason. const SIEVE_SCRIPT_PROPERTIES = ["id", "name", "blobId", "isActive"] as const; @@ -2277,6 +2294,16 @@ export class JMAPClient implements IJMAPClient { * children without their parents. Querying all ids keeps the tree complete. */ private async fetchMailboxTree(accountId: string): Promise { + for (let attempt = 1; ; attempt++) { + try { + return await this.fetchMailboxTreeOnce(accountId); + } catch (error) { + if (!(error instanceof MailboxListChangedError) || attempt >= MAILBOX_TREE_ATTEMPTS) throw error; + } + } + } + + private async fetchMailboxTreeOnce(accountId: string): Promise { const mailboxes: JMAPMailbox[] = []; const seenIds = new Set(); let position = 0; @@ -2335,10 +2362,10 @@ export class JMAPClient implements IJMAPClient { } if (queryState !== undefined && pageQueryState !== queryState) { - throw new Error("Mailbox list changed while it was being loaded"); + throw new MailboxListChangedError(); } if (mailboxState !== undefined && pageMailboxState !== undefined && pageMailboxState !== mailboxState) { - throw new Error("Mailbox list changed while it was being loaded"); + throw new MailboxListChangedError(); } queryState = pageQueryState; if (typeof pageMailboxState === "string") mailboxState = pageMailboxState; @@ -2367,8 +2394,7 @@ export class JMAPClient implements IJMAPClient { ]; let response = await this.request(calls, undefined, { skipOfflineRecord: true }); const first = response.methodResponses?.[0]; - if ((first?.[0] === "Mailbox/get" && first[1].list?.length >= this.getMaxObjectsInGet()) || - (first?.[0] === "error" && first[1].type === "tooManyObjects")) { + if (mailboxListCapped(first, this.getMaxObjectsInGet())) { response = await this.fetchMailboxTree(acctId); } getOfflineHooks()?.record(this, calls, response); @@ -2454,8 +2480,7 @@ export class JMAPClient implements IJMAPClient { let match = response.methodResponses?.find((r) => r[2] === String(i)); // Keep the packed fast path for small accounts. A full-sized result // may be capped, so reload that account through query/get pages. - if ((match?.[0] === "Mailbox/get" && Array.isArray(match[1].list) && match[1].list.length >= maxObjects) || - (match?.[0] === "error" && match[1].type === "tooManyObjects")) { + if (mailboxListCapped(match, maxObjects)) { try { const complete = await this.fetchMailboxTree(accountId); match = complete.methodResponses[0];