mirror of
https://github.com/bulwarkmail/webmail.git
synced 2026-11-03 08:15:38 +00:00
fix: respect single-call paging and preserve folders on page failures
This commit is contained in:
@@ -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']]);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
+29
-9
@@ -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);
|
||||
|
||||
|
||||
@@ -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, unknown>, string];
|
||||
type RequestTarget = { request(calls: MethodCall[]): Promise<unknown> };
|
||||
|
||||
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();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user