diff --git a/web/src/store/__tests__/shared-accounts-batched.test.ts b/web/src/store/__tests__/shared-accounts-batched.test.ts new file mode 100644 index 0000000..2001a03 --- /dev/null +++ b/web/src/store/__tests__/shared-accounts-batched.test.ts @@ -0,0 +1,81 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { CAP, client } from "@/jmap/client"; +import type { JmapSession } from "@/jmap/types"; +import { useSession } from "@/store/session"; +import { useFiles } from "@/store/files"; +import { useContacts } from "@/store/contacts"; +import { useCalendar } from "@/store/calendar"; + +/** + * What signing in costs for each account somebody has shared with the reader. + * + * The files, contacts and calendar stores each asked every shared account a + * question at sign-in, one account after another -- a request apiece, before + * the reader had opened any of those views. The questions now go out together, + * and Files does not ask at all until it is opened. + */ + +const SHARED = ["s1", "s2", "s3"]; + +const session = { + capabilities: { [CAP.core]: { maxCallsInRequest: 16, maxObjectsInGet: 500 }, [CAP.filenode]: {}, [CAP.contacts]: {}, [CAP.calendars]: {} }, + accounts: { + own: { name: "me@example.com", isPersonal: true, accountCapabilities: { [CAP.filenode]: {}, [CAP.contacts]: {}, [CAP.calendars]: {} } }, + ...Object.fromEntries(SHARED.map((id) => [id, { name: `${id}@example.com`, isPersonal: false, accountCapabilities: {} }])), + }, + primaryAccounts: { [CAP.filenode]: "own", [CAP.contacts]: "own", [CAP.calendars]: "own" }, + state: "s", +} as unknown as JmapSession; + +type Call = [string, Record, string]; + +let requests: Call[][]; + +beforeEach(() => { + requests = []; + vi.stubGlobal("fetch", vi.fn(async (_url: string, init: RequestInit) => { + const { methodCalls } = JSON.parse(init.body as string) as { methodCalls: Call[] }; + requests.push(methodCalls); + const methodResponses = methodCalls.map(([name, args, id]) => { + const accountId = args.accountId as string; + if (name === "FileNode/query") return [name, { accountId, ids: accountId === "s2" ? ["f1"] : [], total: 0, position: 0, queryState: "q" }, id]; + if (name === "Calendar/get") return [name, { accountId, state: "1", list: [{ id: `cal-${accountId}`, name: `Calendar of ${accountId}` }], notFound: [] }, id]; + return [name, { accountId, state: "1", list: [], notFound: [] }, id]; + }); + return { ok: true, status: 200, json: async () => ({ methodResponses, sessionState: "s" }) } as Response; + })); + client.session = session; + useSession.setState({ status: "authenticated", session, accountId: "own" }); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe("shared accounts", () => { + it("are not asked about files at sign-in", async () => { + await useFiles.getState().init(); + expect(requests).toHaveLength(0); + expect(useFiles.getState().available).toBe(true); + expect(useFiles.getState().ownAccountId).toBe("own"); + }); + + it("are asked about files together when Files wants to know", async () => { + await useFiles.getState().discoverShared(); + expect(requests).toHaveLength(1); + expect(requests[0]!.map(([n, a]) => `${n} ${a.accountId}`)).toEqual(SHARED.map((id) => `FileNode/query ${id}`)); + expect(useFiles.getState().sharedAccounts).toEqual([{ id: "s2", name: "s2@example.com" }]); + }); + + it("are asked about address books in one request", async () => { + await useContacts.getState().loadShared(); + expect(requests).toHaveLength(1); + expect(requests[0]!.filter(([n]) => n === "AddressBook/get")).toHaveLength(3); + }); + + it("are asked about calendars in one request, and listed in the session's order", async () => { + await useCalendar.getState().loadSharedCalendars(); + expect(requests).toHaveLength(1); + expect(useCalendar.getState().sharedCalendars.map((c) => c.accountId)).toEqual(SHARED); + }); +}); diff --git a/web/src/store/calendar.ts b/web/src/store/calendar.ts index 1cbb5ec..b2e1052 100644 --- a/web/src/store/calendar.ts +++ b/web/src/store/calendar.ts @@ -409,14 +409,13 @@ export const useCalendar = create((set, get) => ({ if (accountId !== get().accountId) set({ accountId, calendars: {}, events: {}, ranges: {} }); set({ available }); if (!available) return; - await get().loadCalendars(); + // Side by side: none of the three waits on another, and together they share a request. + const identities = client.call>("ParticipantIdentity/get", { accountId, ids: null }).then( + (res) => set({ identities: res.list }), + () => set({ identities: [] }), + ); void get().loadSharedCalendars(); - try { - const res = await client.call>("ParticipantIdentity/get", { accountId, ids: null }); - set({ identities: res.list }); - } catch { - set({ identities: [] }); - } + await Promise.all([get().loadCalendars(), identities]); }, /* @@ -434,15 +433,16 @@ export const useCalendar = create((set, get) => ({ const session = useSession.getState(); const own = session.ownAccountFor(CAP.calendars); const accounts = Object.entries(session.session?.accounts ?? {}).filter(([id, a]) => a.isPersonal === false && id !== own); - const found: SharedCalendar[] = []; - for (const [accountId, account] of accounts) { - try { - const res = await client.call>("Calendar/get", { accountId, ids: null, properties: CALENDAR_PROPS }); - for (const calendar of res.list) found.push({ accountId, accountName: account.name, calendar }); - } catch { - continue; - } - } + // Every account at once, in one request, and listed in the session's order. + const answers = await Promise.all( + accounts.map(([accountId, account]) => + client.call>("Calendar/get", { accountId, ids: null, properties: CALENDAR_PROPS }).then( + (res) => res.list.map((calendar): SharedCalendar => ({ accountId, accountName: account.name, calendar })), + (): SharedCalendar[] => [], + ), + ), + ); + const found = answers.flat(); set({ sharedCalendars: found }); // Fill in whatever windows are already on screen. for (const key of Object.keys(get().ranges)) { diff --git a/web/src/store/contacts.ts b/web/src/store/contacts.ts index 1c3d75d..ecc6d62 100644 --- a/web/src/store/contacts.ts +++ b/web/src/store/contacts.ts @@ -294,7 +294,9 @@ export const useContacts = create((set, get) => ({ } const books: SharedBook[] = []; const cards: Record = {}; - for (const [accountId, account] of accounts) { + // Every account at once: calls made in one tick share a request, where a + // loop sent one after another for each account shared with the reader. + await Promise.all(accounts.map(async ([accountId, account]) => { try { const res = await client.call>("AddressBook/get", { accountId, ids: null, properties: ADDRESS_BOOK_PROPS }); for (const book of res.list) books.push({ accountId, accountName: account.name, book }); @@ -310,7 +312,7 @@ export const useContacts = create((set, get) => ({ */ const added = new Set(useSettings.getState().settings.addedShares); const wanted = new Set(res.list.filter((b) => b.isSubscribed || added.has(sharedKey(accountId, b.id))).map((b) => b.id)); - if (!wanted.size) continue; + if (!wanted.size) return; // One page. A shared book is a colleague's contacts, not an archive, // and the alternative is holding the reader's own list hostage to it. const cardsRes = await client.chain([ @@ -325,9 +327,11 @@ export const useContacts = create((set, get) => ({ } catch { // An account that refuses is one that shared nothing here. Not an // error to show: the reader did not ask for it and cannot act on it. - continue; } - } + })); + // Answers arrive in any order; list the books in the session's. + const order = new Map(accounts.map(([id], i) => [id, i])); + books.sort((a, b) => (order.get(a.accountId) ?? 0) - (order.get(b.accountId) ?? 0)); set({ sharedBooks: books, sharedCards: cards, sharedLoaded: true }); }, diff --git a/web/src/store/files.ts b/web/src/store/files.ts index 9290806..1bd9256 100644 --- a/web/src/store/files.ts +++ b/web/src/store/files.ts @@ -50,7 +50,10 @@ interface FilesState { */ draggingIds: Id[]; + /** Whether Files is available and which account is the reader's. No round trip. */ init(): Promise; + /** Ask each shared account whether it holds files; see the note on it. */ + discoverShared(): Promise; /** Browse an account: the reader's own, or one shared with them. */ openAccount(accountId: Id | null): void; loadChildren(parentId: Id | null): Promise; @@ -138,6 +141,17 @@ export const useFiles = create((set, get) => ({ const session = useSession.getState(); const ownAccountId = session.ownAccountFor(CAP.filenode); const available = Boolean(ownAccountId && client.hasCapability(CAP.filenode)); + // Stay where the reader is if the session still offers that account; + // whether it still holds files is `discoverShared`'s to say. + const browsing = get().accountId; + const offered = Object.entries(session.session?.accounts ?? {}).some(([id, a]) => id === browsing && a.isPersonal === false); + if (!(browsing && (browsing === ownAccountId || offered))) set(emptyForAccount(ownAccountId)); + set({ available, ownAccountId }); + }, + + async discoverShared() { + const session = useSession.getState(); + const ownAccountId = get().ownAccountId; /* * Which accounts hold shared files cannot be worked out from capabilities: * Stalwart advertises the whole set on a shared account -- mail, calendars, @@ -151,23 +165,29 @@ export const useFiles = create((set, get) => ({ * account whose calendar or contacts were the thing actually shared. An * account that shares no files does not belong in a list of shared files. */ + /* + * Not at sign-in: only the Files view and the file picker list shared + * accounts, and each opening asks afresh. The questions go out together -- + * calls made in one tick share a request -- rather than one account after + * another. + */ const s = session.session; const candidates = Object.entries(s?.accounts ?? {}).filter(([, a]) => a.isPersonal === false); - const sharedAccounts: SharedAccount[] = []; - for (const [id, a] of candidates) { - try { - const res = await client.call("FileNode/query", { accountId: id, limit: 1 }); - if (res.ids.length) sharedAccounts.push({ id, name: a.name }); - } catch { - // Refused means nothing here is ours to see, which is the same answer. - continue; - } - } + const answers = await Promise.all( + candidates.map(([id, a]) => + client.call("FileNode/query", { accountId: id, limit: 1 }).then( + (res): SharedAccount | null => (res.ids.length ? { id, name: a.name } : null), + // Refused means nothing here is ours to see, which is the same answer. + () => null, + ), + ), + ); + const sharedAccounts = answers.filter((a): a is SharedAccount => a !== null); // Stay where the reader is if they are reading a share that still exists. const browsing = get().accountId; const keep = browsing && (browsing === ownAccountId || sharedAccounts.some((a) => a.id === browsing)); if (!keep) set(emptyForAccount(ownAccountId)); - set({ available, ownAccountId, sharedAccounts }); + set({ sharedAccounts }); }, openAccount(accountId) { diff --git a/web/src/views/compose/FilePicker.tsx b/web/src/views/compose/FilePicker.tsx index 663ce7f..8d243a3 100644 --- a/web/src/views/compose/FilePicker.tsx +++ b/web/src/views/compose/FilePicker.tsx @@ -27,6 +27,11 @@ export function FilePicker({ onPick, onClose }: { onPick: (files: AttachableFile const [picked, setPicked] = useState>({}); const [returnTo] = useState(() => files.accountId); + // Shared accounts are not looked for at sign-in; the picker lists them, so it asks. + useEffect(() => { + void useFiles.getState().discoverShared(); + }, []); + useEffect(() => { void files.loadChildren(cur); // eslint-disable-next-line react-hooks/exhaustive-deps diff --git a/web/src/views/files/FilesTree.tsx b/web/src/views/files/FilesTree.tsx index 0c491ae..23d58bb 100644 --- a/web/src/views/files/FilesTree.tsx +++ b/web/src/views/files/FilesTree.tsx @@ -32,6 +32,7 @@ async function refreshShares(force = false): Promise { return; } await useFiles.getState().init(); + await useFiles.getState().discoverShared(); }