fix: catch up on a folder's changes in one request

Refreshing an open folder that was already loaded took up to four
round trips in a row: `Email/queryChanges`, `Email/changes`, `Email/get`
of the added and changed rows, and a `Thread/get` for their
conversation sizes.

They now go in one request, the `Email/get` of the added messages and
the `Thread/get` of their threads chained by result reference to
`/added/*/id` and `/list/*/threadId`. The list's own query and Email
states still drive it. Each part can fail on its own: a query the
server cannot diff falls back to the full re-query as before, an Email
state it cannot diff is forgotten as before, and added messages the
chained `Email/get` refused (more than one call takes) are fetched
apart. Only further `Email/changes` pages and rows that changed in
place without being re-added cost more requests.
This commit is contained in:
Linus Rath
2026-09-24 20:20:59 +02:00
parent 9c2f819eac
commit 4f3940f1d3
6 changed files with 326 additions and 19 deletions
@@ -0,0 +1,99 @@
import { describe, it, expect, vi, beforeEach } from 'vitest';
// PF7: the incremental refresh was Email/queryChanges, then Email/changes,
// then Email/get, then Thread/get, one round trip each.
vi.mock('../jmap-client', () => ({
jmapClient: { accountId: 'acc-1', request: vi.fn() },
}));
import { jmapClient } from '../jmap-client';
import { getEmailListDelta } from '../email';
const mockRequest = jmapClient.request as ReturnType<typeof vi.fn>;
const sort = [{ property: 'receivedAt', isAscending: false }];
beforeEach(() => {
vi.clearAllMocks();
});
describe('getEmailListDelta', () => {
it('asks for the query diff, the Email changes, the added rows and their threads in one request', async () => {
mockRequest.mockResolvedValue({
methodResponses: [
['Email/queryChanges', {
oldQueryState: 'q-1', newQueryState: 'q-2', total: 7,
removed: ['e2'], added: [{ id: 'n1', index: 0 }],
}, 'qc'],
['Email/changes', {
oldState: 's-1', newState: 's-2', hasMoreChanges: false,
created: ['n1'], updated: ['e2'], destroyed: [],
}, 'ch'],
['Email/get', { state: 's-2', list: [{ id: 'n1', threadId: 't1' }] }, 'get'],
['Thread/get', { list: [{ id: 't1', emailIds: ['x', 'n1'] }] }, 'th'],
],
});
const delta = await getEmailListDelta('mb-1', 'q-1', 's-1', { sort, threads: true });
expect(mockRequest).toHaveBeenCalledTimes(1);
const [calls] = mockRequest.mock.calls[0];
expect(calls).toEqual([
['Email/queryChanges', {
accountId: 'acc-1', filter: { inMailbox: 'mb-1' }, sort, sinceQueryState: 'q-1', calculateTotal: true,
}, 'qc'],
['Email/changes', { accountId: 'acc-1', sinceState: 's-1' }, 'ch'],
['Email/get', expect.objectContaining({
accountId: 'acc-1',
'#ids': { resultOf: 'qc', name: 'Email/queryChanges', path: '/added/*/id' },
}), 'get'],
['Thread/get', {
accountId: 'acc-1',
'#ids': { resultOf: 'get', name: 'Email/get', path: '/list/*/threadId' },
}, 'th'],
]);
expect(delta).toEqual({
queryChanges: { oldQueryState: 'q-1', newQueryState: 'q-2', total: 7, removed: ['e2'], added: [{ id: 'n1', index: 0 }] },
changes: { oldState: 's-1', newState: 's-2', hasMoreChanges: false, created: ['n1'], updated: ['e2'], destroyed: [] },
added: [{ id: 'n1', threadId: 't1' }],
addedFetched: true,
threads: [{ id: 't1', emailIds: ['x', 'n1'] }],
});
});
it('targets a shared folder\'s account and leaves Thread/get out without threading', async () => {
mockRequest.mockResolvedValue({ methodResponses: [] });
await getEmailListDelta('s-inbox', 'q-1', 's-1', { accountId: 'grp-1' });
const [calls] = mockRequest.mock.calls[0];
expect(calls.map((c: [string]) => c[0])).toEqual(['Email/queryChanges', 'Email/changes', 'Email/get']);
expect(calls.every((c: [string, { accountId: string }]) => c[1].accountId === 'grp-1')).toBe(true);
});
it('reports each part that failed on its own', async () => {
mockRequest.mockResolvedValue({
methodResponses: [
['error', { type: 'cannotCalculateChanges' }, 'qc'],
['Email/changes', { oldState: 's-1', newState: 's-2', created: [], updated: [], destroyed: [] }, 'ch'],
['error', { type: 'invalidResultReference' }, 'get'],
],
});
const delta = await getEmailListDelta('mb-1', 'q-1', 's-1');
expect(delta.queryChanges).toBeNull();
expect(delta.changes?.newState).toBe('s-2');
expect(delta.addedFetched).toBe(false);
mockRequest.mockResolvedValue({
methodResponses: [
['Email/queryChanges', { newQueryState: 'q-2', added: [{ id: 'n1', index: 0 }] }, 'qc'],
['error', { type: 'cannotCalculateChanges' }, 'ch'],
['error', { type: 'requestTooLarge' }, 'get'],
],
});
const second = await getEmailListDelta('mb-1', 'q-1', 's-1');
expect(second.queryChanges?.added).toEqual([{ id: 'n1', index: 0 }]);
expect(second.changes).toBeNull();
expect(second.addedFetched).toBe(false);
});
});
+97
View File
@@ -586,6 +586,103 @@ export async function getEmailQueryChanges(
};
}
export interface EmailListDelta {
/** Null when the server can't diff the query (the list is re-queried). */
queryChanges: EmailQueryChangesResult | null;
/** Null when the server can't compute Email changes since the list's state. */
changes: EmailChangesResult | null;
/** The messages the query added, with the list properties. */
added: Email[];
/** False when the chained Email/get failed (too many ids): fetch them apart. */
addedFetched: boolean;
/** Threads of the added messages (with `threads`). */
threads: Thread[];
}
function methodBody(
responses: Array<[string, Record<string, any>, string]>,
callId: string,
name: string,
): Record<string, any> | null {
const match = responses.find((r) => r[2] === callId);
return match && match[0] === name ? match[1] : null;
}
/**
* What changed in a folder list since it was read, in one request instead of
* three or four in a row: Email/queryChanges from the list's queryState,
* Email/changes from its Email state (rows that changed in place), and by
* result reference Email/get of the messages the query added and, with
* `threads`, Thread/get of their threads. Each part fails on its own: a query
* the server can't diff comes back as `queryChanges: null` (the caller
* re-queries), a state it can't diff as `changes: null`.
*/
export async function getEmailListDelta(
mailboxId: string | undefined,
sinceQueryState: string,
sinceState: string,
options?: {
sort?: Array<{ property: string; isAscending: boolean; keyword?: string }>;
/** Owning JMAP account when the mailbox belongs to a shared account. */
accountId?: string;
threads?: boolean;
},
): Promise<EmailListDelta> {
const accountId = options?.accountId ?? jmapClient.accountId;
const calls: JMAPMethodCall[] = [
['Email/queryChanges', {
accountId,
filter: buildMailboxQueryFilter(mailboxId, undefined),
sort: options?.sort ?? [{ property: 'receivedAt', isAscending: false }],
sinceQueryState,
calculateTotal: true,
}, 'qc'],
['Email/changes', { accountId, sinceState }, 'ch'],
['Email/get', {
accountId,
'#ids': { resultOf: 'qc', name: 'Email/queryChanges', path: '/added/*/id' },
properties: EMAIL_LIST_PROPERTIES,
}, 'get'],
];
if (options?.threads) {
calls.push(['Thread/get', {
accountId,
'#ids': { resultOf: 'get', name: 'Email/get', path: '/list/*/threadId' },
}, 'th']);
}
const res = await jmapClient.request(calls);
const responses = res.methodResponses ?? [];
const qc = methodBody(responses, 'qc', 'Email/queryChanges');
const ch = methodBody(responses, 'ch', 'Email/changes');
const got = methodBody(responses, 'get', 'Email/get');
const th = methodBody(responses, 'th', 'Thread/get');
return {
queryChanges: qc
? {
oldQueryState: qc.oldQueryState as string,
newQueryState: qc.newQueryState as string,
total: (qc.total as number) ?? 0,
removed: (qc.removed as string[]) ?? [],
added: (qc.added as Array<{ id: string; index: number }>) ?? [],
}
: null,
changes: ch
? {
oldState: ch.oldState as string,
newState: ch.newState as string,
hasMoreChanges: Boolean(ch.hasMoreChanges),
created: (ch.created as string[]) ?? [],
updated: (ch.updated as string[]) ?? [],
destroyed: (ch.destroyed as string[]) ?? [],
}
: null,
added: (got?.list as Email[] | undefined) ?? [],
addedFetched: !!got,
threads: (th?.list as Thread[] | undefined) ?? [],
};
}
export async function getEmails(ids: string[], accountIdOverride?: string): Promise<Email[]> {
return (await getEmailsWithState(ids, accountIdOverride)).list;
}
@@ -11,6 +11,7 @@ vi.mock('../../api/email', () => ({
getMailboxChanges: vi.fn(async () => null),
queryEmailPage: vi.fn(async () => ({ ids: [], total: 0, list: [], threads: [] })),
getEmailQueryChanges: vi.fn(async () => null),
getEmailListDelta: vi.fn(async () => ({ queryChanges: null, changes: null, added: [], addedFetched: false, threads: [] })),
getEmails: vi.fn(),
getEmailsWithState: vi.fn(async () => ({ list: [], state: 'em-0' })),
getEmailChanges: vi.fn(async () => null),
@@ -124,7 +125,7 @@ function folderCountsBecome(totalEmails: number) {
}
function listWasReRead(): boolean {
return [emailApi.getEmailQueryChanges, emailApi.getEmailChanges, emailApi.queryEmailPage, emailApi.getEmailsWithState]
return [emailApi.getEmailListDelta, emailApi.getEmailChanges, emailApi.queryEmailPage, emailApi.getEmailsWithState]
.some((fn) => mock(fn).mock.calls.length > 0);
}
@@ -207,7 +208,7 @@ describe('echo of our own writes', () => {
'@type': 'StateChange', changed: { 'acc-1': { Email: 'em-3' } },
});
expect(emailApi.getEmailQueryChanges).toHaveBeenCalledTimes(1);
expect(emailApi.getEmailListDelta).toHaveBeenCalledTimes(1);
});
it('refreshes when new mail was delivered', async () => {
@@ -218,7 +219,7 @@ describe('echo of our own writes', () => {
'@type': 'StateChange', changed: { 'acc-1': { Email: 'em-2', EmailDelivery: 'd-1' } },
});
expect(emailApi.getEmailQueryChanges).toHaveBeenCalledTimes(1);
expect(emailApi.getEmailListDelta).toHaveBeenCalledTimes(1);
});
it('refreshes when the folder count shows a message we do not hold left it', async () => {
@@ -230,7 +231,7 @@ describe('echo of our own writes', () => {
'@type': 'StateChange', changed: { 'acc-1': { Email: 'em-2', Mailbox: 'mbs-2' } },
});
expect(emailApi.getEmailQueryChanges).toHaveBeenCalledTimes(1);
expect(emailApi.getEmailListDelta).toHaveBeenCalledTimes(1);
});
it('refreshes when a pinned row moves', async () => {
@@ -241,7 +242,7 @@ describe('echo of our own writes', () => {
'@type': 'StateChange', changed: { 'acc-1': { Email: 'em-2' } },
});
expect(emailApi.getEmailQueryChanges).toHaveBeenCalledTimes(1);
expect(emailApi.getEmailListDelta).toHaveBeenCalledTimes(1);
});
it('waits for a write whose push overtook its response', async () => {
@@ -14,6 +14,7 @@ vi.mock('../../api/email', () => ({
getMailboxChanges: vi.fn(async () => null),
queryEmails: vi.fn(),
getEmailQueryChanges: vi.fn(async () => null),
getEmailListDelta: vi.fn(async () => ({ queryChanges: null, changes: null, added: [], addedFetched: false, threads: [] })),
getEmails: vi.fn(),
getEmailsWithState: vi.fn(async () => ({ list: [], state: 'em-state-0' })),
getEmailChanges: vi.fn(async () => null),
+98
View File
@@ -16,6 +16,9 @@ vi.mock('../../api/email', () => ({
// queryEmailPage.
queryEmailPagesAcrossAccounts: vi.fn(),
getEmailQueryChanges: vi.fn(async () => null),
// Email/queryChanges, Email/changes, Email/get and Thread/get in one
// request; beforeEach scripts it from the separate mocks.
getEmailListDelta: vi.fn(),
getEmails: vi.fn(),
getEmailsWithState: vi.fn(async () => ({ list: [], state: 'em-state-0' })),
getEmailChanges: vi.fn(async () => null),
@@ -194,6 +197,17 @@ beforeEach(() => {
});
return { accountId: target.accountId, ok: true, total: page.total, list: page.list, threads: page.threads };
})));
// The incremental refresh asks for its delta in one request; most tests
// describe it as the queryChanges, changes and Email/get it chains.
(emailApi.getEmailListDelta as ReturnType<typeof vi.fn>).mockImplementation(async (
mailboxId: string | undefined, sinceQueryState: string, sinceState: string, opts?: { accountId?: string },
) => {
const queryChanges = await emailApi.getEmailQueryChanges(mailboxId, sinceQueryState, opts);
const changes = await emailApi.getEmailChanges(sinceState, undefined, opts?.accountId);
const ids = queryChanges?.added.map((a) => a.id) ?? [];
const added = ids.length > 0 ? (await emailApi.getEmailsWithState(ids, opts?.accountId)).list : [];
return { queryChanges, changes, added, addedFetched: !!queryChanges, threads: [] };
});
});
describe('email-store', () => {
@@ -1095,6 +1109,90 @@ describe('email-store', () => {
expect(useEmailStore.getState().emailStates).toEqual({});
});
// PF7: the delta was Email/queryChanges, then Email/changes, then
// Email/get, then Thread/get, one after the other.
describe('in one request', () => {
const mockDelta = emailApi.getEmailListDelta as ReturnType<typeof vi.fn>;
const threaded = (id: string, keywords: Record<string, boolean> = {}) => ({ ...row(id, keywords), threadId: `t-${id}` });
it('needs nothing else when the delta brought every row', async () => {
openInbox();
mockDelta.mockResolvedValueOnce({
queryChanges: {
oldQueryState: 'q-1', newQueryState: 'q-2', total: 4,
removed: ['e2'], added: [{ id: 'n1', index: 0 }, { id: 'e2', index: 2 }],
},
changes: { ...noChanges, created: ['n1'], updated: ['e2'] },
added: [threaded('n1'), threaded('e2', { $seen: true })],
addedFetched: true,
threads: [{ id: 't-n1', emailIds: ['n1', 'x1'] }],
});
await useEmailStore.getState().refreshEmails();
expect(mockDelta).toHaveBeenCalledWith('mb-1', 'q-1', 'em-1', expect.objectContaining({ accountId: undefined }));
expect(mockGetEmailQueryChanges).not.toHaveBeenCalled();
expect(mockGetEmailChanges).not.toHaveBeenCalled();
expect(mockGetEmailsWithState).not.toHaveBeenCalled();
const state = useEmailStore.getState();
expect(state.emails.map((e) => e.id)).toEqual(['n1', 'e1', 'e2', 'e3']);
expect(state.emails[2].keywords).toEqual({ $seen: true });
expect(state.threadCounts['t-n1']).toBe(2);
expect(state.emailStates['mb-1']).toBe('em-2');
expect(state.queryState).toBe('q-2');
});
it('fetches the added rows apart when the chained Email/get was refused', async () => {
openInbox();
mockDelta.mockResolvedValueOnce({
queryChanges: { oldQueryState: 'q-1', newQueryState: 'q-2', total: 4, removed: [], added: [{ id: 'n1', index: 0 }] },
changes: { ...noChanges, created: ['n1'] },
added: [],
addedFetched: false,
threads: [],
});
mockGetEmailsWithState.mockResolvedValue({ list: [row('n1')], state: 'em-2' });
await useEmailStore.getState().refreshEmails();
expect(mockGetEmailsWithState).toHaveBeenCalledWith(['n1'], undefined);
expect(useEmailStore.getState().emails.map((e) => e.id)).toEqual(['n1', 'e1', 'e2', 'e3']);
});
it('re-queries when the server cannot diff the query', async () => {
openInbox();
mockDelta.mockResolvedValueOnce({ queryChanges: null, changes: noChanges, added: [], addedFetched: false, threads: [] });
mockQueryEmails.mockResolvedValue({ ids: ['e1'], total: 1, queryState: 'q-9' });
mockGetEmailsWithState.mockResolvedValue({ list: [row('e1')], state: 'em-9' });
await useEmailStore.getState().refreshEmails();
expect(mockQueryEmailPage).toHaveBeenCalledTimes(1);
expect(useEmailStore.getState().emailStates['mb-1']).toBe('em-9');
expect(useEmailStore.getState().queryState).toBe('q-9');
});
it('drains further Email/changes pages apart', async () => {
openInbox();
mockDelta.mockResolvedValueOnce({
queryChanges: { oldQueryState: 'q-1', newQueryState: 'q-2', total: 3, removed: [], added: [] },
changes: { ...noChanges, newState: 'em-2', hasMoreChanges: true },
added: [],
addedFetched: true,
threads: [],
});
mockGetEmailChanges.mockResolvedValueOnce({ ...noChanges, oldState: 'em-2', newState: 'em-3', updated: ['e3'] });
mockGetEmailsWithState.mockResolvedValue({ list: [row('e3', { $flagged: true })], state: 'em-3' });
await useEmailStore.getState().refreshEmails();
expect(mockGetEmailChanges).toHaveBeenCalledWith('em-2', undefined, undefined);
expect(mockGetEmailsWithState).toHaveBeenCalledWith(['e3'], undefined);
expect(useEmailStore.getState().emails[2].keywords).toEqual({ $flagged: true });
expect(useEmailStore.getState().emailStates['mb-1']).toBe('em-3');
});
});
});
// PF7: a list page or search used to be Email/query, then Email/get, then a
+25 -14
View File
@@ -12,7 +12,7 @@ import {
getMailboxChanges,
queryEmailPage,
queryEmailPagesAcrossAccounts,
getEmailQueryChanges,
getEmailListDelta,
getEmails as fetchEmails,
getEmailsWithState,
getEmailChanges,
@@ -32,6 +32,7 @@ import {
undoSpam as apiUndoSpam,
destroyEmails as apiDestroyEmails,
unprefixMailboxId,
type EmailChangesResult,
} from '../api/email';
import { applyOwnWritesToList, ownEmailWritesBetween, whenOwnWritesSettled } from '../api/own-writes';
import { provideLoadedMailboxes } from '../lib/mailbox-source';
@@ -2620,11 +2621,14 @@ async function refreshEmailsImpl(): Promise<void> {
snap.emails.length >= Math.min(limit, snap.total)
) {
const baseEmails = snap.emails;
const queryChanges = await getEmailQueryChanges(ref.id, snap.queryState, {
// One request: Email/queryChanges, Email/changes, and Email/get of
// the added messages plus their threads by result reference.
const delta = await getEmailListDelta(ref.id, snap.queryState, emailState, {
sort,
filter: undefined,
accountId: ref.accountId,
threads: !useSettingsStore.getState().disableThreading,
});
const queryChanges = delta.queryChanges;
if (queryChanges) {
// What's in the visible window now: drop removed ids, then apply
// added (id, index) entries. Newly added ids need bodies fetched.
@@ -2640,10 +2644,13 @@ async function refreshEmailsImpl(): Promise<void> {
let nextEmailState: string | undefined = emailState;
// Drain `hasMoreChanges`: the server caps one response, so keep
// asking from the returned state until the delta is complete
// (bounded so a runaway server can't loop us forever).
// (bounded so a runaway server can't loop us forever). The first
// round came with the delta.
let since: string | undefined = emailState;
for (let round = 0; since && round < 10; round++) {
const ec = await getEmailChanges(since, undefined, ref.accountId);
const ec: EmailChangesResult | null = round === 0
? delta.changes
: await getEmailChanges(since, undefined, ref.accountId);
if (!ec) {
// cannotCalculateChanges → forget the state so the next
// refresh re-queries the list and records a fresh one.
@@ -2656,19 +2663,22 @@ async function refreshEmailsImpl(): Promise<void> {
since = ec.hasMoreChanges && ec.newState !== since ? ec.newState : undefined;
}
// Fetch every added id, including ones the list already holds:
// `inMailbox` is a mutable filter, so the server reports each
// updated message as removed and re-added, and that row has to
// come back at its new index with fresh keywords. `updatedIds`
// refreshes rows Email/changes saw change in place.
// Every added id is fetched, including ones the list already
// holds: `inMailbox` is a mutable filter, so the server reports
// each updated message as removed and re-added, and that row has
// to come back at its new index with fresh keywords. The delta
// brought them unless there were more than one Email/get takes.
// `updatedIds` refreshes rows Email/changes saw change in place;
// only those the delta didn't bring cost another request.
const existingById = new Map(baseEmails.map((e) => [e.id, e]));
const brought = new Set(delta.added.map((e) => e.id));
const idsToFetch = Array.from(new Set([
...addedIds,
...updatedIds.filter((id) => existingById.has(id)),
...(delta.addedFetched ? [] : addedIds),
...updatedIds.filter((id) => existingById.has(id) && !brought.has(id)),
]));
let fetched: Email[] = [];
let fetched: Email[] = delta.added;
if (idsToFetch.length > 0) {
fetched = (await getEmailsWithState(idsToFetch, ref.accountId)).list;
fetched = [...fetched, ...(await getEmailsWithState(idsToFetch, ref.accountId)).list];
}
// Rebuild the visible window order: start with existing emails,
@@ -2712,6 +2722,7 @@ async function refreshEmailsImpl(): Promise<void> {
currentMailboxId,
nextEmailState,
),
threadCounts: withThreadCounts(get().threadCounts, delta.threads),
loading: false,
mailboxSnapshots: {
...get().mailboxSnapshots,