From 9c2f819eaccf3eae3d1b91487ed601496e45ecba Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 24 Sep 2026 20:06:58 +0200 Subject: [PATCH] fix: load the folder list once, and right away, when signing in Signing in sent two full `Mailbox/get`s before the inbox loaded, and the first only went out once the main screen had mounted and painted. There is no deliberate delay behind the gap: the mail screen's mount effect started the folder load, and the push setup read the same folders again with a `Mailbox/get` of its own (plus the shared accounts') just to find the Junk folders for its filter. The latter also happened on every start with push notifications on. - Sign-in (password and OAuth/pairing) starts the folder load as soon as the session is up, like a restored session already did. - The mail screen's mount now joins a load on its way (`ensureMailboxes`) instead of queueing another behind it. - The push filter takes the folders from the mail store (joining its load) and only fetches them itself when the store has none for the account. --- .../__tests__/push-config-mailboxes.test.ts | 67 +++++++++++++++++++ src/lib/mailbox-source.ts | 29 ++++++++ src/lib/push-notifications.ts | 14 +++- src/screens/EmailListScreen.tsx | 8 ++- ...ame.test.ts => auth-store-sign-in.test.ts} | 26 ++++++- .../__tests__/email-store-own-writes.test.ts | 30 +++++++++ src/stores/auth-store.ts | 6 ++ src/stores/email-store.ts | 31 +++++++++ 8 files changed, 202 insertions(+), 9 deletions(-) create mode 100644 src/lib/__tests__/push-config-mailboxes.test.ts create mode 100644 src/lib/mailbox-source.ts rename src/stores/__tests__/{auth-store-display-name.test.ts => auth-store-sign-in.test.ts} (63%) diff --git a/src/lib/__tests__/push-config-mailboxes.test.ts b/src/lib/__tests__/push-config-mailboxes.test.ts new file mode 100644 index 00000000..93082ce6 --- /dev/null +++ b/src/lib/__tests__/push-config-mailboxes.test.ts @@ -0,0 +1,67 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +// The push filter needs the Junk folders. It used to send its own full +// Mailbox/get (and the shared accounts') on every sign-in and start, next to +// the mail store's identical load (PF7). + +vi.mock('../../api/jmap-client', () => ({ + jmapClient: { + username: 'user@example.com', + serverUrl: 'https://mail.example.com', + accountId: 'jmap-primary', + currentSession: { capabilities: {} }, + }, +})); + +vi.mock('../../api/email', () => ({ + getMailboxes: vi.fn(async () => [{ id: 'junk-own', role: 'junk', accountId: 'jmap-primary' }]), + getSharedMailboxes: vi.fn(async () => []), +})); + +import { getMailboxes, getSharedMailboxes } from '../../api/email'; +import { buildEmailPushConfig } from '../push-notifications'; +import { provideLoadedMailboxes } from '../mailbox-source'; +import { generateAccountId } from '../account-utils'; + +const ACCOUNT_ID = generateAccountId('user@example.com', 'https://mail.example.com'); + +beforeEach(() => { + vi.clearAllMocks(); + provideLoadedMailboxes(null); +}); + +describe('push filter folders', () => { + it('takes the Junk folders from the mail store instead of fetching them', async () => { + const provider = vi.fn(async () => [ + { id: 'junk', role: 'junk', accountId: 'jmap-primary' }, + { id: 'team:j', originalId: 'j', role: 'junk', accountId: 'team', isShared: true }, + ] as never); + provideLoadedMailboxes(provider); + + const config = await buildEmailPushConfig(); + + expect(provider).toHaveBeenCalledWith(ACCOUNT_ID); + expect(getMailboxes).not.toHaveBeenCalled(); + expect(getSharedMailboxes).not.toHaveBeenCalled(); + expect(config['jmap-primary'].filter).toEqual({ + operator: 'AND', + conditions: [{ notKeyword: '$junk' }, { inMailboxOtherThan: ['junk'] }], + }); + expect(config.team.filter).toEqual({ + operator: 'AND', + conditions: [{ notKeyword: '$junk' }, { inMailboxOtherThan: ['j'] }], + }); + }); + + it('fetches them itself when the store has none for this account', async () => { + provideLoadedMailboxes(async () => null); + + const config = await buildEmailPushConfig(); + + expect(getMailboxes).toHaveBeenCalledTimes(1); + expect(config['jmap-primary'].filter).toEqual({ + operator: 'AND', + conditions: [{ notKeyword: '$junk' }, { inMailboxOtherThan: ['junk-own'] }], + }); + }); +}); diff --git a/src/lib/mailbox-source.ts b/src/lib/mailbox-source.ts new file mode 100644 index 00000000..5237c74b --- /dev/null +++ b/src/lib/mailbox-source.ts @@ -0,0 +1,29 @@ +// The folder list the mail store holds (or is loading) for the signed-in +// account, for code below the stores that needs it too: the push filter's +// Junk folders. It used to send a full `Mailbox/get` of its own right next to +// the store's on every sign-in and start. The mail store registers the +// provider, so nothing here imports it. + +import type { Mailbox } from '../api/types'; + +type Provider = (accountId: string) => Promise; + +let provider: Provider | null = null; + +export function provideLoadedMailboxes(next: Provider | null): void { + provider = next; +} + +/** + * The own and shared folders of `accountId` (the account registry id) once + * the store has them, joining a sync that is on its way. Null when the store + * serves another account or could not load them. + */ +export async function loadedMailboxes(accountId: string): Promise { + if (!provider) return null; + try { + return await provider(accountId); + } catch { + return null; + } +} diff --git a/src/lib/push-notifications.ts b/src/lib/push-notifications.ts index 1a4a695d..93a713f5 100644 --- a/src/lib/push-notifications.ts +++ b/src/lib/push-notifications.ts @@ -8,6 +8,7 @@ import { verifyPushSubscription, } from '../api/push'; import { getMailboxes, getSharedMailboxes } from '../api/email'; +import { loadedMailboxes } from './mailbox-source'; import { jmapClient } from '../api/jmap-client'; import { JMAPMethodError } from '../api/jmap-result'; import { CAPABILITIES } from '../api/types'; @@ -259,9 +260,16 @@ export function serverSupportsEmailPush(): boolean { export async function buildEmailPushConfig(): Promise> { const primary = jmapClient.accountId; const junkByAccount = new Map([[primary, []]]); - const own = await getMailboxes().catch(() => [] as Mailbox[]); - const shared = await getSharedMailboxes().catch(() => [] as Mailbox[]); - for (const m of [...own, ...shared]) { + // The mail store loads the same folders at the same moment (sign-in, + // start): reuse its list rather than a second Mailbox/get of our own. + const username = jmapClient.username; + const serverUrl = jmapClient.serverUrl; + const loaded = username && serverUrl ? await loadedMailboxes(generateAccountId(username, serverUrl)) : null; + const all = loaded ?? [ + ...(await getMailboxes().catch(() => [] as Mailbox[])), + ...(await getSharedMailboxes().catch(() => [] as Mailbox[])), + ]; + for (const m of all) { const accountId = m.accountId || primary; const junk = junkByAccount.get(accountId) ?? []; // Shared-account mailboxes carry a client-side ":" id; diff --git a/src/screens/EmailListScreen.tsx b/src/screens/EmailListScreen.tsx index 5d65d9bd..9276bee8 100644 --- a/src/screens/EmailListScreen.tsx +++ b/src/screens/EmailListScreen.tsx @@ -331,6 +331,7 @@ export default function EmailListScreen({ onEmailPress, onComposePress }: EmailL const filters = useEmailStore((s) => s.filters); const accountErrors = useEmailStore((s) => s.accountErrors); const fetchMailboxes = useEmailStore((s) => s.fetchMailboxes); + const ensureMailboxes = useEmailStore((s) => s.ensureMailboxes); const selectMailbox = useEmailStore((s) => s.selectMailbox); const loadMoreEmails = useEmailStore((s) => s.loadMoreEmails); const refreshEmails = useEmailStore((s) => s.refreshEmails); @@ -937,12 +938,13 @@ export default function EmailListScreen({ onEmailPress, onComposePress }: EmailL ); }; - // Load mailboxes and select inbox on mount + // Load mailboxes and select inbox on mount (joining the load sign-in + // started rather than queueing another) React.useEffect(() => { if (mailboxes.length === 0) { - void fetchMailboxes(); + void ensureMailboxes(); } - }, [fetchMailboxes, mailboxes.length]); + }, [ensureMailboxes, mailboxes.length]); React.useEffect(() => { if (mailboxes.length > 0 && !currentMailboxId) { diff --git a/src/stores/__tests__/auth-store-display-name.test.ts b/src/stores/__tests__/auth-store-sign-in.test.ts similarity index 63% rename from src/stores/__tests__/auth-store-display-name.test.ts rename to src/stores/__tests__/auth-store-sign-in.test.ts index 37fa186b..da842d10 100644 --- a/src/stores/__tests__/auth-store-display-name.test.ts +++ b/src/stores/__tests__/auth-store-sign-in.test.ts @@ -1,13 +1,15 @@ import { describe, it, expect, vi } from 'vitest'; -// The display-name sync on every sign-in and start read the Stalwart -// principal with x:Account/get, which is refused to everyone but admins -// (PF6). The full name lives in x:AccountSettings, which every user reads. +// What a sign-in sends (PF6, PF7): the display-name sync read the Stalwart +// principal with x:Account/get, which is refused to everyone but admins (the +// full name lives in x:AccountSettings, which every user reads), and the +// folder list only started loading once the mail screen had mounted. const request = vi.fn(async (calls: Array<[string, Record, string]>) => ({ methodResponses: calls.map(([name, , id]) => { if (name === 'Identity/get') return [name, { list: [{ id: 'i1', name: 'Old Name', email: 'user@example.com' }] }, id]; if (name === 'x:AccountSettings/get') return [name, { list: [{ description: 'Ada Lovelace' }] }, id]; + if (name === 'Mailbox/get') return [name, { list: [{ id: 'mb-in', name: 'Inbox', role: 'inbox' }], state: 'mbs-1' }, id]; return ['error', { type: 'forbidden' }, id]; }), })); @@ -20,6 +22,8 @@ vi.mock('../../api/jmap-client', () => ({ onAuthFailure: vi.fn(() => () => undefined), onTokenRefresh: vi.fn(() => () => undefined), hasAccountCapability: vi.fn(() => true), + getAccountName: () => undefined, + getSharedMailAccounts: () => [], request: (calls: Array<[string, Record, string]>) => request(calls), accountId: 'acc-1', isConnected: true, @@ -38,6 +42,7 @@ vi.mock('../../lib/push-notifications', () => ({ import { useAuthStore } from '../auth-store'; import { useAccountStore } from '../account-store'; +import { useEmailStore } from '../email-store'; describe('display name sync on sign-in', () => { it('reads the full name without asking for the principal', async () => { @@ -50,4 +55,19 @@ describe('display name sync on sign-in', () => { expect(methods).toContain('x:AccountSettings/get'); expect(methods).not.toContain('x:Account/get'); }); + + it('starts on the folder list at once, and the mail screen joins that load', async () => { + request.mockClear(); + useEmailStore.getState().clearAllAccounts(); + await useAuthStore.getState().login('https://mail.example.com', 'user@example.com', 'pass'); + const mailboxGets = () => request.mock.calls.filter(([calls]) => calls.some(([name]) => name.startsWith('Mailbox/'))); + expect(mailboxGets()).toHaveLength(1); + + // The mail screen mounts while that load is still on its way. + await useEmailStore.getState().ensureMailboxes(); + expect(mailboxGets()).toHaveLength(1); + expect(useEmailStore.getState().mailboxes.map((m) => m.id)).toEqual(['mb-in']); + await useEmailStore.getState().ensureMailboxes(); + expect(mailboxGets()).toHaveLength(1); + }); }); diff --git a/src/stores/__tests__/email-store-own-writes.test.ts b/src/stores/__tests__/email-store-own-writes.test.ts index 2edc9805..3e7f1afc 100644 --- a/src/stores/__tests__/email-store-own-writes.test.ts +++ b/src/stores/__tests__/email-store-own-writes.test.ts @@ -85,6 +85,7 @@ import * as emailApi from '../../api/email'; import { beginOwnWrite, recordOwnEmailWrites, resetOwnWrites } from '../../api/own-writes'; import type { JMAPMethodCall } from '../../api/types'; import { useEmailStore } from '../email-store'; +import { loadedMailboxes } from '../../lib/mailbox-source'; const SERVER = 'https://mail.example.com'; const mock = (fn: unknown) => fn as ReturnType; @@ -291,3 +292,32 @@ describe('folder pushes', () => { expect(emailApi.getSharedMailboxes).toHaveBeenCalledTimes(1); }); }); + +describe('the folder list for the push filter', () => { + const ACCOUNT = generateAccountId('test@example.com', SERVER); + + it('joins the load on its way instead of fetching again', async () => { + useEmailStore.setState({ activeAccountId: ACCOUNT, mailboxes: [], mailboxState: undefined }); + mock(emailApi.getMailboxesWithState).mockResolvedValue({ list: [inboxFolder(0)], state: 'mbs-1' }); + const loading = useEmailStore.getState().fetchMailboxes(); + + const list = await loadedMailboxes(ACCOUNT); + await loading; + + expect(list?.map((m) => m.id)).toEqual(['mb-1']); + expect(emailApi.getMailboxesWithState).toHaveBeenCalledTimes(1); + expect(emailApi.getMailboxChanges).not.toHaveBeenCalled(); + }); + + it('hands out a list it already holds without a request', async () => { + openInbox([row('e1')]); + expect((await loadedMailboxes(ACCOUNT))?.map((m) => m.id)).toEqual(['mb-1']); + expect(emailApi.getMailboxesWithState).not.toHaveBeenCalled(); + expect(emailApi.getMailboxChanges).not.toHaveBeenCalled(); + }); + + it('has nothing for another account', async () => { + openInbox([row('e1')]); + expect(await loadedMailboxes('someone-else@mail.example.com')).toBeNull(); + }); +}); diff --git a/src/stores/auth-store.ts b/src/stores/auth-store.ts index 8c157802..cffbfbdf 100644 --- a/src/stores/auth-store.ts +++ b/src/stores/auth-store.ts @@ -224,6 +224,9 @@ async function completeOAuthHandoff( useEmailStore.getState().setActiveAccount(accountId); applyConnectedState(set, session, result.serverUrl.replace(/\/+$/, ''), username, accountId); + // Start on the folder list now rather than once the mail screen has + // mounted; the screen joins this load. + void useEmailStore.getState().fetchMailboxes(); void syncAccountDisplayName(accountId); } @@ -299,6 +302,9 @@ export const useAuthStore = create((set, get) => ({ applyConnectedState(set, session, serverUrl.replace(/\/+$/, ''), username, accountId); set({ pendingTotpLogin: null }); + // Start on the folder list now rather than once the mail screen has + // mounted; the screen joins this load. + void useEmailStore.getState().fetchMailboxes(); void syncAccountDisplayName(accountId); } catch (err) { if (err instanceof Error && err.name === 'TotpRequiredError') { diff --git a/src/stores/email-store.ts b/src/stores/email-store.ts index 788ae5e0..4a843fe6 100644 --- a/src/stores/email-store.ts +++ b/src/stores/email-store.ts @@ -34,6 +34,7 @@ import { unprefixMailboxId, } from '../api/email'; import { applyOwnWritesToList, ownEmailWritesBetween, whenOwnWritesSettled } from '../api/own-writes'; +import { provideLoadedMailboxes } from '../lib/mailbox-source'; import { useNetworkStore } from './network-store'; import { JMAPMethodError } from '../api/jmap-result'; import { @@ -372,6 +373,12 @@ export interface EmailState { removeAccount: (accountId: string) => void; clearAllAccounts: () => void; fetchMailboxes: () => Promise; + /** + * Load the folder list unless it is loaded or already loading: for + * mount-time callers, which would otherwise queue a second sync behind the + * one sign-in started. + */ + ensureMailboxes: () => Promise; selectMailbox: (mailboxId: string) => Promise; loadMoreEmails: () => Promise; refreshEmails: () => Promise; @@ -931,6 +938,13 @@ export const useEmailStore = create()( return syncMailboxes(activeAccountId!, { own: true, shared: true }); }, + ensureMailboxes: () => { + const { mailboxes, activeAccountId } = get(); + if (mailboxes.length > 0 || !activeAccountId) return Promise.resolve(); + const running = mailboxSyncsRunning(activeAccountId); + return running ?? get().fetchMailboxes(); + }, + selectMailbox: async (mailboxId) => { const state = get(); // Tuck the previously-visible mailbox into its snapshot so a return-trip @@ -2096,6 +2110,23 @@ export const useEmailStore = create()( ), ); +// The folder syncs of an account on their way, joined (not queued behind). +function mailboxSyncsRunning(activeAccountId: string): Promise | null { + const running = [`${activeAccountId}:mailboxes`, `${activeAccountId}:shared-mailboxes`] + .map((key) => inflightRefresh.get(key)) + .filter((p): p is Promise => !!p); + return running.length > 0 ? Promise.all(running).then(() => undefined) : null; +} + +// The push filter needs the Junk folders of every account: hand it this list +// instead of letting it fetch its own (see lib/mailbox-source). +provideLoadedMailboxes(async (accountId) => { + if (useEmailStore.getState().activeAccountId !== accountId) return null; + await (mailboxSyncsRunning(accountId) ?? useEmailStore.getState().ensureMailboxes()); + const { mailboxes, activeAccountId } = useEmailStore.getState(); + return activeAccountId === accountId && mailboxes.length > 0 ? mailboxes : null; +}); + // Copy-then-delete across accounts (webmail `crossAccountMoveEmails`, 1.7.2): // download each message's blob from the source account, upload it to the // target account, Email/import it into the target folder with its keywords,