diff --git a/src/api/__tests__/email-list-delta.test.ts b/src/api/__tests__/email-list-delta.test.ts new file mode 100644 index 00000000..77f4224c --- /dev/null +++ b/src/api/__tests__/email-list-delta.test.ts @@ -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; +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); + }); +}); diff --git a/src/api/email.ts b/src/api/email.ts index f0da145c..37eb0347 100644 --- a/src/api/email.ts +++ b/src/api/email.ts @@ -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]>, + callId: string, + name: string, +): Record | 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 { + 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 { return (await getEmailsWithState(ids, accountIdOverride)).list; } diff --git a/src/stores/__tests__/email-store-own-writes.test.ts b/src/stores/__tests__/email-store-own-writes.test.ts index 3e7f1afc..979f5c69 100644 --- a/src/stores/__tests__/email-store-own-writes.test.ts +++ b/src/stores/__tests__/email-store-own-writes.test.ts @@ -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 () => { diff --git a/src/stores/__tests__/email-store-viewer-actions.test.ts b/src/stores/__tests__/email-store-viewer-actions.test.ts index ee6ec8ea..682f115a 100644 --- a/src/stores/__tests__/email-store-viewer-actions.test.ts +++ b/src/stores/__tests__/email-store-viewer-actions.test.ts @@ -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), diff --git a/src/stores/__tests__/email-store.test.ts b/src/stores/__tests__/email-store.test.ts index a28a20c1..762d1e0e 100644 --- a/src/stores/__tests__/email-store.test.ts +++ b/src/stores/__tests__/email-store.test.ts @@ -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).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; + const threaded = (id: string, keywords: Record = {}) => ({ ...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 diff --git a/src/stores/email-store.ts b/src/stores/email-store.ts index 4a843fe6..19489c5f 100644 --- a/src/stores/email-store.ts +++ b/src/stores/email-store.ts @@ -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 { 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 { 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 { 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 { currentMailboxId, nextEmailState, ), + threadCounts: withThreadCounts(get().threadCounts, delta.threads), loading: false, mailboxSnapshots: { ...get().mailboxSnapshots,