fix: paginate mailbox retrieval without sidebar changes

This commit is contained in:
Nick Sweeting
2026-11-02 09:06:27 -08:00
parent 3f3dfebb84
commit 2262b2baf1
4 changed files with 378 additions and 39 deletions
@@ -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, unknown>, string];
type RequestTarget = { request(calls: MethodCall[]): Promise<unknown> };
function createClient(
maxObjectsInGet = 2,
accounts: Record<string, { name: string }> = { 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<string, RawMailbox[]>, 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']);
});
});
+109 -37
View File
@@ -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<JMAPResponse> {
private async request(methodCalls: JMAPMethodCall[], using?: string[], opts?: { transfer?: boolean; skipOfflineRecord?: boolean }): Promise<JMAPResponse> {
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<JMAPResponse> {
const mailboxes: JMAPMailbox[] = [];
const seenIds = new Set<string>();
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<Mailbox[]> {
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);
+93 -1
View File
@@ -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 {
+19 -1
View File
@@ -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<string, unknown>, nam
};
}
/** Unfiltered mailbox pages used to load a complete folder list offline. */
async function mailboxQuery(ctx: ResponderContext, args: Record<string, unknown>): Promise<MethodResult> {
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<string, unknown>[];
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<string, unknown>): 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;