Merge pull request #384 from Coffey-Labs/perf/contacts-calendar-changes

Sync contacts by what changed, and hold fewer calendar windows

server/src/mock/events.ts

@@ -18,6 +18,18 @@ export function recordEmailChange(change: { created?: string[]; updated?: string
   if (emailChanges.length > 200) emailChanges.splice(0, emailChanges.length - 200);
 }
 
+/** The same for contact cards, so `ContactCard/changes` can answer too. */
+export const cardChanges: Array<{ state: number; created: string[]; updated: string[]; destroyed: string[] }> = [];
+/** Changes at or below this state have been dropped from the log, so a client that far behind cannot be answered. */
+export const cardLog = { floor: 0 };
+export function recordCardChange(change: { created?: string[]; updated?: string[]; destroyed?: string[] }) {
+  cardChanges.push({ state: state.n, created: change.created ?? [], updated: change.updated ?? [], destroyed: change.destroyed ?? [] });
+  if (cardChanges.length > 200) {
+    const dropped = cardChanges.splice(0, cardChanges.length - 200);
+    cardLog.floor = dropped[dropped.length - 1]!.state;
+  }
+}
+
 export function broadcast(types: string[]) {
   const payload = `event: state\ndata: ${JSON.stringify({ "@type": "StateChange", changed: { [ACCOUNT]: Object.fromEntries(types.map((t) => [t, String(state.n)])) } })}\n\n`;
   for (const c of sseClients) c.write(payload);

server/src/mock/handlers.ts

@@ -1,5 +1,5 @@
 import { checkOtp } from "./auth.js";
-import { emailChanges, recordEmailChange, broadcast } from "./events.js";
+import { cardChanges, cardLog, emailChanges, recordCardChange, recordEmailChange, broadcast } from "./events.js";
 import { randomUUID } from "node:crypto";
 import { eventGetView, expandOccurrences, occurrenceAt, occurrenceView, parseSyntheticId, splitOccurrencePatch, syntheticId, type Occurrence } from "./recurrence.js";
 import { holdUntilOf, undoStatusOf } from "./futurerelease.js";
@@ -428,7 +428,29 @@ export const handlers: Record<string, Handler> = {
   // An empty `properties` list returns `id` alone, which `pick` already does.
   // 0.16.22 made Stalwart agree; through 0.16.21 it returned every property.
   "ContactCard/get": (a) => genericGet(a.accountId === SHARED_ACCOUNT ? sharedCards : cards)(a),
-  "ContactCard/set": genericSet(cards, "cc"),
+  /*
+   * Recorded and announced like Email/set, so the client's incremental sync
+   * (`ContactCard/changes`, then fetching what it names) runs here too. A
+   * state older than the log's window cannot be answered, as on a real server.
+   */
+  "ContactCard/set": (a) => {
+    const r = genericSet(cards, "cc")(a);
+    nextState();
+    recordCardChange({
+      created: Object.values((r.created ?? {}) as Record<string, { id: string }>).map((x) => x.id),
+      updated: Object.keys((r.updated ?? {}) as Obj),
+      destroyed: (r.destroyed as string[] | undefined) ?? [],
+    });
+    broadcast(["ContactCard"]);
+    return r;
+  },
+  "ContactCard/changes": (a) => {
+    const since = Number(a.sinceState ?? 0);
+    if (since < cardLog.floor) throw new MethodError("cannotCalculateChanges", "That state is too old to answer from.");
+    const relevant = cardChanges.filter((c) => c.state > since);
+    const pick = (k: "created" | "updated" | "destroyed") => [...new Set(relevant.flatMap((c) => c[k]))];
+    return { accountId: a.accountId ?? ACCOUNT, oldState: String(a.sinceState ?? "1"), newState: String(state.n), hasMoreChanges: false, created: pick("created"), updated: pick("updated"), destroyed: pick("destroyed") };
+  },
   "ContactCard/parse": (a) => { const parsed: Obj = {}; for (const b of a.blobIds as string[]) { const t = blobs.get(b)?.data.toString() ?? ""; const fn = /^FN:(.*)$/m.exec(t)?.[1]?.trim() ?? "Imported"; const em = /^EMAIL[^:]*:(.*)$/m.exec(t)?.[1]?.trim(); parsed[b] = [{ "@type": "Card", version: "1.0", uid: randomUUID(), kind: "individual", name: { full: fn }, emails: em ? { e1: { address: em } } : undefined }]; } return { accountId: ACCOUNT, parsed, notParsable: [] }; },
   "FileNode/query": (a) => {
     const f = (a.filter as Obj) ?? {};

web/src/store/__tests__/contacts-calendar-sync.test.ts

@@ -0,0 +1,150 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import { CAP, client } from "@/jmap/client";
+import type { JmapSession } from "@/jmap/types";
+import { useContacts } from "@/store/contacts";
+import { keepRecent, RANGES_KEPT, useCalendar } from "@/store/calendar";
+
+/**
+ * What a change to one contact or one event costs.
+ *
+ * A pushed ContactCard change used to reload the whole address book, and a
+ * CalendarEvent change queried every window the reader had ever visited again.
+ */
+
+type Call = [string, Record<string, unknown>, string];
+let calls: Call[];
+let reply: (name: string, args: Record<string, unknown>) => unknown;
+
+beforeEach(() => {
+  calls = [];
+  client.session = {
+    capabilities: { [CAP.core]: { maxCallsInRequest: 16, maxObjectsInGet: 2 }, [CAP.contacts]: {}, [CAP.calendars]: {} },
+    accounts: {},
+    primaryAccounts: {},
+    state: "s",
+  } as unknown as JmapSession;
+  vi.stubGlobal("fetch", vi.fn(async (_url: string, init: RequestInit) => {
+    const { methodCalls } = JSON.parse(init.body as string) as { methodCalls: Call[] };
+    const methodResponses = methodCalls.map(([name, args, id]) => {
+      calls.push([name, args, id]);
+      const out = reply(name, args);
+      return out instanceof Error ? ["error", { type: out.message }, id] : [name, out, id];
+    });
+    return { ok: true, status: 200, json: async () => ({ methodResponses, sessionState: "s" }) } as Response;
+  }));
+});
+
+afterEach(() => {
+  vi.unstubAllGlobals();
+});
+
+const card = (id: string, name: string) => ({ id, addressBookIds: { b1: true }, name: { full: name } });
+
+describe("syncCards", () => {
+  beforeEach(() => {
+    useContacts.setState({
+      accountId: "a1",
+      available: true,
+      loaded: true,
+      loading: false,
+      cardState: "10",
+      cards: { c1: card("c1", "Ann"), c2: card("c2", "Bob"), c3: card("c3", "Cy") } as never,
+    });
+  });
+
+  it("fetches only what changed, in batches the server takes", async () => {
+    reply = (name, args) => {
+      if (name === "ContactCard/changes") return { oldState: "10", newState: "12", hasMoreChanges: false, created: ["c4", "c5", "c6"], updated: ["c1"], destroyed: ["c2"] };
+      if (name === "ContactCard/get") return { state: "12", list: (args.ids as string[]).map((id) => card(id, `new ${id}`)), notFound: [] };
+      throw new Error(`unexpected ${name}`);
+    };
+    await useContacts.getState().syncCards();
+    const names = calls.map(([n]) => n);
+    expect(names).not.toContain("ContactCard/query");
+    const gets = calls.filter(([n]) => n === "ContactCard/get").map(([, a]) => a.ids as string[]);
+    expect(gets.every((ids) => ids.length <= 2)).toBe(true);
+    expect(gets.flat().sort()).toEqual(["c1", "c4", "c5", "c6"]);
+    const s = useContacts.getState();
+    expect(Object.keys(s.cards).sort()).toEqual(["c1", "c3", "c4", "c5", "c6"]);
+    expect(s.cards.c1!.name!.full).toBe("new c1");
+    expect(s.cards.c3!.name!.full).toBe("Cy");
+    expect(s.cardState).toBe("12");
+  });
+
+  it("follows changes across pages", async () => {
+    reply = (name, args) => {
+      if (name === "ContactCard/changes") {
+        return args.sinceState === "10"
+          ? { oldState: "10", newState: "11", hasMoreChanges: true, created: [], updated: ["c1"], destroyed: [] }
+          : { oldState: "11", newState: "13", hasMoreChanges: false, created: [], updated: [], destroyed: ["c1"] };
+      }
+      return { state: "13", list: [], notFound: [] };
+    };
+    await useContacts.getState().syncCards();
+    // Updated on the first page and destroyed on the second: gone, and not fetched.
+    expect(useContacts.getState().cards.c1).toBeUndefined();
+    expect(calls.filter(([n]) => n === "ContactCard/get")).toHaveLength(0);
+    expect(useContacts.getState().cardState).toBe("13");
+  });
+
+  it("reloads everything when the server cannot say what changed", async () => {
+    reply = (name) => {
+      if (name === "ContactCard/changes") return new Error("cannotCalculateChanges");
+      if (name === "ContactCard/query") return { ids: ["c9"], total: 1, position: 0, queryState: "q" };
+      if (name === "ContactCard/get") return { state: "20", list: [card("c9", "Zed")], notFound: [] };
+      throw new Error(`unexpected ${name}`);
+    };
+    await useContacts.getState().syncCards();
+    const s = useContacts.getState();
+    expect(Object.keys(s.cards)).toEqual(["c9"]);
+    expect(s.cardState).toBe("20");
+  });
+
+  it("records the state a full load was read at", async () => {
+    useContacts.setState({ cardState: null, loaded: false });
+    reply = (name) => {
+      if (name === "ContactCard/query") return { ids: ["c1"], total: 1, position: 0, queryState: "q" };
+      return { state: "30", list: [card("c1", "Ann")], notFound: [] };
+    };
+    await useContacts.getState().loadAll();
+    expect(useContacts.getState().cardState).toBe("30");
+  });
+});
+
+describe("calendar windows", () => {
+  const day = 86_400_000;
+  const windowAt = (n: number) => [new Date(n * 7 * day), new Date((n + 1) * 7 * day)] as const;
+
+  beforeEach(() => {
+    useCalendar.setState({ accountId: "a1", available: true, ranges: {}, sharedRanges: {}, events: {}, sharedCalendars: [] });
+    reply = (name) => (name === "CalendarEvent/query" ? { ids: [], total: 0, position: 0, queryState: "q" } : { state: "1", list: [], notFound: [] });
+  });
+
+  it("keeps only the most recent few", () => {
+    let ranges: Record<string, string[]> = {};
+    for (let i = 0; i < RANGES_KEPT + 3; i++) ranges = keepRecent(ranges, `k${i}`, []);
+    expect(Object.keys(ranges)).toEqual(Array.from({ length: RANGES_KEPT }, (_, i) => `k${i + 3}`));
+    // Seeing one again moves it to the back of the queue.
+    ranges = keepRecent(ranges, "k3", ["e"]);
+    expect(Object.keys(ranges).at(-1)).toBe("k3");
+    expect(ranges.k3).toEqual(["e"]);
+  });
+
+  it("queries only the windows it holds when an event changes", async () => {
+    const store = useCalendar.getState();
+    for (let i = 0; i < 10; i++) {
+      const [a, b] = windowAt(i);
+      await store.loadRange(a, b);
+    }
+    expect(Object.keys(useCalendar.getState().ranges)).toHaveLength(RANGES_KEPT);
+    calls = [];
+    useCalendar.getState().applyChanges(new Set(["CalendarEvent"]));
+    await vi.waitFor(() => expect(calls.filter(([n]) => n === "CalendarEvent/query")).toHaveLength(RANGES_KEPT));
+  });
+
+  it("does not empty the windows while they reload", () => {
+    useCalendar.setState({ ranges: { [`${7 * day}|${14 * day}`]: ["e1"] } });
+    useCalendar.getState().invalidate();
+    expect(Object.values(useCalendar.getState().ranges)).toEqual([["e1"]]);
+  });
+});

web/src/store/calendar.ts

@@ -498,7 +498,8 @@ export const useCalendar = create<CalendarState>((set, get) => ({
     const accounts = [...new Set(shared.map((c) => c.accountId))];
     const ids: string[] = [];
     const events: Record<string, CalendarEvent> = {};
-    for (const accountId of accounts) {
+    // Every account at once, rather than one waiting on the last.
+    await Promise.all(accounts.map(async (accountId) => {
       try {
         const res = await client.chain([
           ["CalendarEvent/query", { accountId, filter: { after: toLocalDateTime(start), before: toLocalDateTime(end) }, timeZone: tz, sort: [{ property: "start", isAscending: true }], expandRecurrences: true, limit: 2000 }, "q"],
@@ -512,10 +513,9 @@ export const useCalendar = create<CalendarState>((set, get) => ({
         }
       } catch {
         // One account refusing must not empty the calendar of the others.
-        continue;
       }
-    }
+    }));
-    set((s) => ({ sharedEvents: { ...s.sharedEvents, ...events }, sharedRanges: { ...s.sharedRanges, [key]: ids } }));
+    set((s) => ({ sharedEvents: { ...s.sharedEvents, ...events }, sharedRanges: key in s.ranges ? { ...s.sharedRanges, [key]: ids } : s.sharedRanges }));
   },
 
   async loadCalendars() {
@@ -535,7 +535,12 @@ export const useCalendar = create<CalendarState>((set, get) => ({
     const accountId = get().accountId;
     if (!accountId) return;
     const key = `${start.getTime()}|${end.getTime()}`;
-    if (!force && get().ranges[key]) return;
+    const held = get().ranges[key];
+    if (!force && held) {
+      // Shown again, so the last to be dropped.
+      set((s) => ({ ranges: keepRecent(s.ranges, key, held) }));
+      return;
+    }
     set({ loading: true });
     const tz = settings().timeZone ?? browserTimeZone;
     try {
@@ -560,7 +565,8 @@ export const useCalendar = create<CalendarState>((set, get) => ({
       set((s) => {
         const events = { ...s.events };
         for (const e of g.list) events[e.id] = e;
-        return { events, ranges: { ...s.ranges, [key]: q.ids }, loading: false, error: null };
+        const ranges = keepRecent(s.ranges, key, q.ids);
+        return { events, ranges, sharedRanges: onlyKeys(s.sharedRanges, ranges), loading: false, error: null };
       });
       void get().loadSharedRange(start, end);
     } catch (err) {
@@ -665,6 +671,7 @@ export const useCalendar = create<CalendarState>((set, get) => ({
        hiding one is remembered under the same account-qualified key. */
     const sharedKeys = new Set<string>();
     for (const list of Object.values(sharedRanges)) for (const k of list) sharedKeys.add(k);
+    const added = new Set(settings().addedShares);
     for (const k of sharedKeys) {
       const e = sharedEvents[k];
       if (!e) continue;
@@ -676,7 +683,6 @@ export const useCalendar = create<CalendarState>((set, get) => ({
          an account linked for its files offered its calendar too. `isSubscribed`
          is the only thing separating "shared with me" from "reachable", so
          nothing unsubscribed is drawn. */
-      const added = new Set(settings().addedShares);
       const theirs: Record<Id, Calendar> = {};
       for (const c of sharedCalendars) {
         if (c.accountId !== accountId) continue;
@@ -1000,11 +1006,16 @@ export const useCalendar = create<CalendarState>((set, get) => ({
     if (types.has("CalendarEvent")) get().invalidate();
   },
 
+  /*
+   * Load the windows held again, after something changed.
+   *
+   * Only the few most recently shown are held (see `keepRecent`), so this is a
+   * handful of queries rather than one for every month ever looked at. They
+   * are replaced where they are, not emptied first: clearing them made the
+   * calendar go blank until the answers came back.
+   */
   invalidate() {
-    // Force reload of all ranges currently cached.
+    for (const k of Object.keys(get().ranges)) {
-    const keys = Object.keys(get().ranges);
-    set({ ranges: {} });
-    for (const k of keys) {
       const [s, e] = k.split("|").map(Number) as [number, number];
       void get().loadRange(new Date(s), new Date(e), true);
     }
@@ -1015,6 +1026,27 @@ export const useCalendar = create<CalendarState>((set, get) => ({
   },
 }));
 
+/**
+ * How many loaded windows are held.
+ *
+ * Every week or month the reader visited used to stay, and each was queried
+ * again whenever any event changed, and walked by every render. A window
+ * dropped here is simply loaded again if the reader goes back to it.
+ */
+export const RANGES_KEPT = 4;
+
+/** `ranges` with `key` set and moved to the end, trimmed to the most recent `RANGES_KEPT`. */
+export function keepRecent(ranges: Record<string, Id[]>, key: string, ids: Id[]): Record<string, Id[]> {
+  const { [key]: _old, ...rest } = ranges;
+  const entries = [...Object.entries(rest), [key, ids] as [string, Id[]]];
+  return Object.fromEntries(entries.slice(-RANGES_KEPT));
+}
+
+/** `map` restricted to the windows still held. */
+function onlyKeys<T>(map: Record<string, T>, held: Record<string, unknown>): Record<string, T> {
+  return Object.fromEntries(Object.entries(map).filter(([k]) => k in held));
+}
+
 /**
  * Whether an event is part of a series.
  *

web/src/store/contacts.ts

@@ -1,7 +1,7 @@
 import { create } from "zustand";
 import { accountKey, loadRaw, saveJson } from "@/lib/storage";
-import { CAP, chunk, client, setErrorMessage } from "@/jmap/client";
+import { CAP, chunk, client, JmapMethodError, setErrorMessage } from "@/jmap/client";
-import type { AddressBook, ContactCard, EmailAddress, GetResponse, Id, Principal, QueryResponse, SetError, SetResponse } from "@/jmap/types";
+import type { AddressBook, ChangesResponse, ContactCard, EmailAddress, GetResponse, Id, Principal, QueryResponse, SetError, SetResponse } from "@/jmap/types";
 import { contactDisplayName, contactEmails, sortKey } from "@/lib/contacts";
 import { parseLdif, uidFromDn } from "@/lib/ldif";
 import { cardFromLdif } from "@/lib/mozillaAb";
@@ -173,6 +173,8 @@ interface ContactsState {
   available: boolean;
   books: Record<Id, AddressBook>;
   cards: Record<Id, ContactCard>;
+  /** The server's ContactCard state `cards` was read at, for asking what changed since. */
+  cardState: string | null;
   loaded: boolean;
   loading: boolean;
   error: string | null;
@@ -189,6 +191,11 @@ interface ContactsState {
   init(): Promise<void>;
   loadBooks(): Promise<void>;
   loadAll(): Promise<void>;
+  /**
+   * Bring `cards` up to date with what changed on the server, or load them all
+   * when that cannot be worked out.
+   */
+  syncCards(): Promise<void>;
   /** Books and cards from accounts that shared with the reader. */
   loadShared(): Promise<void>;
   select(selection: BookSelection): void;
@@ -247,6 +254,7 @@ export const useContacts = create<ContactsState>((set, get) => ({
   available: false,
   books: {},
   cards: {},
+  cardState: null,
   loaded: false,
   loading: false,
   error: null,
@@ -264,7 +272,7 @@ export const useContacts = create<ContactsState>((set, get) => ({
     // should move when the switcher does.
     const accountId = useSession.getState().ownAccountFor(CAP.contacts);
     const available = Boolean(accountId && client.hasCapability(CAP.contacts));
-    if (accountId !== get().accountId) set({ accountId, books: {}, cards: {}, loaded: false, selection: { accountId: null, bookId: "all" } });
+    if (accountId !== get().accountId) set({ accountId, books: {}, cards: {}, cardState: null, loaded: false, selection: { accountId: null, bookId: "all" } });
     set({ available });
     if (!available) return;
     await get().loadBooks();
@@ -408,6 +416,7 @@ export const useContacts = create<ContactsState>((set, get) => ({
       const cards: Record<Id, ContactCard> = {};
       let position = 0;
       const limit = 500;
+      let cardState: string | null = null;
       for (let guard = 0; guard < 50; guard++) {
         const res = await client.chain([
           ["ContactCard/query", { accountId, position, limit, calculateTotal: true }, "q"],
@@ -416,15 +425,63 @@ export const useContacts = create<ContactsState>((set, get) => ({
         const q = res.get("q")?.[0] as unknown as QueryResponse;
         const g = res.get("g")?.[0] as unknown as GetResponse<ContactCard>;
         for (const c of g.list) cards[c.id] = c;
+        // The first page's: a change made while the rest were being read is
+        // then reported again by the next sync, rather than missed.
+        cardState ??= g.state;
         position += q.ids.length;
         if (q.ids.length < limit || (q.total != null && position >= q.total)) break;
       }
-      set({ cards, loaded: true, loading: false, error: null });
+      set({ cards, cardState, loaded: true, loading: false, error: null });
     } catch (err) {
       set({ loading: false, error: (err as Error).message });
     }
   },
 
+  /*
+   * What changed, rather than everything again.
+   *
+   * Every push that touched a card, and every edit made here, used to reload
+   * the whole address book -- up to fifty pages of five hundred cards with all
+   * their properties -- to pick up one change. ContactCard/changes names what
+   * changed since the state the cards were read at, and only those are fetched.
+   * A server that cannot say (`cannotCalculateChanges`), or any other failure,
+   * falls back to the full load, which is what happened before.
+   */
+  async syncCards() {
+    const { accountId, cardState, loaded } = get();
+    if (!accountId || !loaded || !cardState) return get().loadAll();
+    try {
+      const changed = new Set<Id>();
+      const destroyed = new Set<Id>();
+      let since = cardState;
+      for (let guard = 0; guard < 50; guard++) {
+        const ch = await client.call<ChangesResponse>("ContactCard/changes", { accountId, sinceState: since, maxChanges: 500 });
+        for (const id of [...ch.created, ...ch.updated]) { changed.add(id); destroyed.delete(id); }
+        for (const id of ch.destroyed) { destroyed.add(id); changed.delete(id); }
+        since = ch.newState;
+        if (!ch.hasMoreChanges) break;
+      }
+      const fetched = await Promise.all(
+        chunk([...changed], client.maxObjectsInGet).map((part) => client.call<GetResponse<ContactCard>>("ContactCard/get", { accountId, ids: part })),
+      );
+      if (get().accountId !== accountId) return;
+      set((s) => {
+        const cards = { ...s.cards };
+        for (const id of destroyed) delete cards[id];
+        for (const r of fetched) {
+          for (const c of r.list) cards[c.id] = c;
+          // An id listed as changed but gone by the time it was asked for.
+          for (const id of r.notFound ?? []) delete cards[id];
+        }
+        return { cards, cardState: since, error: null };
+      });
+    } catch (err) {
+      if (!(err instanceof JmapMethodError) || err.type !== "cannotCalculateChanges") console.warn("[ihasmail] contact sync failed, reloading:", err);
+      set({ cardState: null });
+      await get().loadAll();
+    }
+  },
+
   async getCard(id) {
     const accountId = get().accountId;
     if (!accountId) return null;
@@ -546,7 +603,7 @@ export const useContacts = create<ContactsState>((set, get) => ({
         refused ??= Object.values(res.notDestroyed ?? {})[0] ?? Object.values(res.notUpdated ?? {})[0];
       }
     } finally {
-      await get().loadAll();
+      await get().syncCards();
     }
     return { destroyed: gone.length, unfiled, refused };
   },
@@ -574,7 +631,7 @@ export const useContacts = create<ContactsState>((set, get) => ({
     const err = res.notDestroyed?.[id];
     if (err) throw new Error(setErrorMessage(err));
     await get().loadBooks();
-    await get().loadAll();
+    await get().syncCards();
   },
 
   async importVCard(text, addressBookId) {
@@ -623,7 +680,7 @@ export const useContacts = create<ContactsState>((set, get) => ({
          matched on it rather than guessed at. */
       return { created, updated, alike: 0 };
     } finally {
-      await get().loadAll();
+      await get().syncCards();
     }
   },
 
@@ -691,7 +748,7 @@ export const useContacts = create<ContactsState>((set, get) => ({
       if (!created && !updated) throw new Error(refused ? setErrorMessage(refused) : "the server did not accept any of its contacts");
       return { created, updated, alike };
     } finally {
-      await get().loadAll();
+      await get().syncCards();
     }
   },
 
@@ -786,7 +843,7 @@ export const useContacts = create<ContactsState>((set, get) => ({
 
   applyChanges(types) {
     if (types.has("AddressBook")) { void get().loadBooks(); void get().loadShared(); }
-    if (types.has("ContactCard") && get().loaded) void get().loadAll();
+    if (types.has("ContactCard") && get().loaded) void get().syncCards();
   },
 }));