From 2262b2baf15a6c5aa68cbe7a02386b4abb8d5ea6 Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Mon, 2 Nov 2026 09:06:27 -0800 Subject: [PATCH] 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;