Merge pull request #378 from Coffey-Labs/fix/push-refresh

Keep list refreshes within the server's limits and stop repeating them

web/src/store/__tests__/list-refresh.test.ts

@@ -0,0 +1,191 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import { CAP, client } from "@/jmap/client";
+import { useMail } from "@/store/mail";
+import type { JmapSession } from "@/jmap/types";
+
+/**
+ * What a list refresh and an open thread cost the server.
+ *
+ * A refresh fetches everything already on screen, and in conversation mode
+ * every message of every listed thread. Both used to go to Email/get in one
+ * call whatever their number, and Stalwart refuses a whole call over
+ * `maxObjectsInGet` -- so past a few pages, or with long threads, the refresh
+ * failed and the list silently stopped updating.
+ */
+
+const MAX = 500;
+const INBOX = "mbInbox";
+
+type Call = [string, Record<string, unknown>, string];
+
+/**
+ * A mailbox of `count` messages. With `threadSize` above 1, each listed
+ * message leads a thread of that many, whose other members are not in the
+ * list. Enforces MAX on every /get, back-referenced ids included, as the mock
+ * server and Stalwart do.
+ */
+function server(count: number, threadSize = 1) {
+  const listed = Array.from({ length: count }, (_, i) => `e${i}`);
+  const members = (lead: string) => [lead, ...Array.from({ length: threadSize - 1 }, (_, j) => `${lead}m${j}`)];
+  const calls: Call[] = [];
+  const fetchMock = vi.fn(async (_url: string, init: RequestInit) => {
+    const body = JSON.parse(init.body as string) as { methodCalls: Call[] };
+    const responses: Call[] = [];
+    const resolve = (args: Record<string, unknown>): Record<string, unknown> => {
+      const ref = args["#ids"] as { resultOf: string; path: string } | undefined;
+      if (!ref) return args;
+      const from = responses.find((r) => r[2] === ref.resultOf)![1];
+      const ids =
+        ref.path === "/ids" ? (from.ids as string[])
+        : ref.path === "/list/*/threadId" ? (from.list as { threadId: string }[]).map((e) => e.threadId)
+        : (from.list as { emailIds: string[] }[]).flatMap((t) => t.emailIds);
+      const { "#ids": _drop, ...rest } = args;
+      return { ...rest, ids };
+    };
+    for (const [name, raw, id] of body.methodCalls) {
+      const args = resolve(raw);
+      calls.push([name, args, id]);
+      const ids = args.ids as string[] | undefined;
+      if (name.endsWith("/get") && ids && ids.length > MAX) {
+        responses.push(["error", { type: "requestTooLarge" }, id]);
+        continue;
+      }
+      if (name === "Email/query") {
+        const position = args.position as number;
+        const limit = args.limit as number;
+        responses.push([name, { accountId: "a1", queryState: "q1", canCalculateChanges: false, position, ids: listed.slice(position, position + limit), total: listed.length }, id]);
+      } else if (name === "Email/get") {
+        const list = ids!.map((e) => ({ id: e, threadId: `t${e.replace(/m\d+$/, "")}`, mailboxIds: { [INBOX]: true }, keywords: {}, receivedAt: "2026-09-16T00:00:00Z" }));
+        responses.push([name, { accountId: "a1", state: "s1", list, notFound: [] }, id]);
+      } else if (name === "Thread/get") {
+        const list = ids!.map((t) => ({ id: t, emailIds: members(t.slice(1)) }));
+        responses.push([name, { accountId: "a1", state: "s1", list, notFound: [] }, id]);
+      } else {
+        responses.push([name, { accountId: "a1", state: "s1", list: [], notFound: [] }, id]);
+      }
+    }
+    return { ok: true, status: 200, json: async () => ({ methodResponses: responses, sessionState: "1" }) } as Response;
+  });
+  vi.stubGlobal("fetch", fetchMock);
+  return { calls, listed };
+}
+
+const getSizes = (calls: Call[]) => calls.filter(([n]) => n === "Email/get").map(([, a]) => (a.ids as string[]).length);
+
+beforeEach(() => {
+  client.session = {
+    capabilities: { [CAP.core]: { maxObjectsInGet: MAX, maxObjectsInSet: MAX }, [CAP.mail]: {} },
+    accounts: {},
+    primaryAccounts: {},
+    state: "s1",
+  } as unknown as JmapSession;
+  useMail.setState({
+    accountId: "a1",
+    mailboxes: { [INBOX]: { id: INBOX, role: "inbox", name: "Inbox" } } as never,
+    list: null,
+    emails: {},
+    fullIds: {},
+    threads: {},
+    emailState: "s1",
+    loadingThreads: {},
+    openThreadId: null,
+  });
+});
+
+afterEach(() => {
+  vi.unstubAllGlobals();
+  vi.restoreAllMocks();
+});
+
+const listOf = (ids: string[], collapseThreads = false) => ({
+  key: "k",
+  filter: { inMailbox: INBOX },
+  sort: [],
+  collapseThreads,
+  mailboxId: INBOX,
+  ids,
+  total: ids.length,
+  queryState: "q0",
+  loading: false,
+  loadingMore: false,
+  error: null,
+  exhausted: false,
+});
+
+describe("refreshList", () => {
+  it("refreshes more rows than one Email/get may carry, in pages the server takes", async () => {
+    const { calls, listed } = server(1300);
+    useMail.setState({ list: listOf(listed.slice(0, 1200)) as never });
+    await useMail.getState().refreshList();
+    const sizes = getSizes(calls);
+    expect(sizes.every((n) => n <= MAX)).toBe(true);
+    expect(calls.some(([n]) => n === "error")).toBe(false);
+    const list = useMail.getState().list!;
+    expect(list.ids).toEqual(listed.slice(0, 1200));
+    expect(list.total).toBe(1300);
+    expect(list.error).toBeNull();
+  });
+
+  it("stops at the end of a folder that shrank", async () => {
+    const { listed } = server(700);
+    useMail.setState({ list: listOf([...listed, ...Array.from({ length: 300 }, (_, i) => `gone${i}`)]) as never });
+    await useMail.getState().refreshList();
+    const list = useMail.getState().list!;
+    expect(list.ids).toEqual(listed);
+    expect(list.exhausted).toBe(true);
+  });
+
+  it("fetches long threads' other messages within the limit", async () => {
+    // 50 listed threads of 20 messages: 950 members that are not in the list.
+    const { calls, listed } = server(50, 20);
+    await useMail.getState().query({ key: "", filter: { inMailbox: INBOX }, sort: [], collapseThreads: true, mailboxId: INBOX });
+    const list = useMail.getState().list!;
+    expect(list.error).toBeNull();
+    expect(list.ids).toEqual(listed);
+    expect(getSizes(calls).every((n) => n <= MAX)).toBe(true);
+    const emails = useMail.getState().emails;
+    expect(Object.keys(emails)).toHaveLength(50 * 20);
+    // A second refresh does not fetch the members it already holds.
+    calls.length = 0;
+    await useMail.getState().refreshList();
+    expect(getSizes(calls)).toEqual([50]);
+  });
+});
+
+describe("loadThread", () => {
+  it("fetches no bodies for messages already held in full", async () => {
+    const { calls } = server(1, 3);
+    useMail.setState({
+      emails: { e0: { id: "e0" }, e0m0: { id: "e0m0" }, e0m1: { id: "e0m1" } } as never,
+      fullIds: { e0: true, e0m0: true, e0m1: true },
+    });
+    const before = useMail.getState().emails.e0;
+    const got = await useMail.getState().loadThread("te0");
+    expect(got.map((e) => e.id)).toEqual(["e0", "e0m0", "e0m1"]);
+    expect(calls.map(([n]) => n)).toEqual(["Thread/get"]);
+    // The same object, so nothing derived from it has to be rebuilt.
+    expect(useMail.getState().emails.e0).toBe(before);
+  });
+
+  it("fetches in full only the message it does not have", async () => {
+    const { calls } = server(1, 3);
+    useMail.setState({
+      emails: { e0: { id: "e0" }, e0m0: { id: "e0m0" }, e0m1: { id: "e0m1" } } as never,
+      fullIds: { e0: true, e0m0: true },
+    });
+    await useMail.getState().loadThread("te0");
+    const gets = calls.filter(([n]) => n === "Email/get");
+    expect(gets).toHaveLength(1);
+    expect(gets[0]![1].ids).toEqual(["e0m1"]);
+    expect(gets[0]![1].fetchHTMLBodyValues).toBe(true);
+    expect(useMail.getState().fullIds.e0m1).toBe(true);
+    expect(useMail.getState().loadingThreads).toEqual({});
+  });
+
+  it("splits a thread longer than one Email/get may carry", async () => {
+    const { calls } = server(1, 1200);
+    const got = await useMail.getState().loadThread("te0");
+    expect(got).toHaveLength(1200);
+    expect(getSizes(calls).every((n) => n <= MAX)).toBe(true);
+  });
+});

web/src/store/mail/index.ts

@@ -206,8 +206,24 @@ export const useMail = create<MailState>((set, get) => ({
     const l = get().list;
     if (!accountId || !l) return;
     try {
-      const limit = Math.max(settings().pageSize, l.ids.length);
-      const { ids, total, queryState } = await runQuery(accountId, l, 0, limit);
+      /*
+       * Everything already on screen is fetched again, which past a few pages
+       * is more than one Email/get may carry: Stalwart refuses the whole call
+       * over `maxObjectsInGet`, and a refused refresh left the list silently
+       * stale. So it goes in pages the server will take.
+       */
+      const want = Math.max(settings().pageSize, l.ids.length);
+      const ids: Id[] = [];
+      const seen = new Set<Id>();
+      let total = 0;
+      let queryState = "";
+      while (ids.length < want) {
+        const page = await runQuery(accountId, l, ids.length, want - ids.length);
+        total = page.total;
+        queryState ||= page.queryState;
+        for (const id of page.ids) if (!seen.has(id)) { seen.add(id); ids.push(id); }
+        if (page.ids.length < page.limit || ids.length >= total) break;
+      }
       const cur = get().list;
       if (!cur || cur.key !== l.key) return;
       set({ list: { ...cur, ids, total, queryState, loading: false, error: null, exhausted: ids.length >= total } });
@@ -255,34 +271,31 @@ export const useMail = create<MailState>((set, get) => ({
     if (!accountId) return [];
     set((s) => ({ loadingThreads: { ...s.loadingThreads, [threadId]: true } }));
     try {
-      const res = await client.chain([
-        ["Thread/get", { accountId, ids: [threadId] }, "t"],
-        [
-          "Email/get",
-          {
-            accountId,
-            "#ids": { resultOf: "t", name: "Thread/get", path: "/list/*/emailIds" },
-            properties: FULL_PROPS,
-            fetchHTMLBodyValues: true,
-            fetchTextBodyValues: true,
-            maxBodyValueBytes: 2 * 1024 * 1024,
-            bodyProperties: BODY_PROPS,
-          },
-          "e",
-        ],
-      ]);
-      const thread = (res.get("t")?.[0] as unknown as GetResponse<Thread>).list[0];
-      const emailsRes = res.get("e")?.[0] as unknown as GetResponse<Email>;
-      if (!thread) return [];
+      /*
+       * Bodies are fetched only for messages not already held in full.
+       *
+       * This runs on every push that touches mail, the open thread's own
+       * mark-as-read included, and it used to fetch every message in the
+       * thread in full each time -- up to 2 MB of body apiece, and new
+       * attachment objects that made the reading pane rebuild what it had
+       * already rendered. A body cannot change under an id (RFC 8621), and
+       * keywords and mailboxes come in with the list refresh, so a message
+       * held in full needs nothing more. getEmails also splits the fetch to
+       * `maxObjectsInGet`, which a long thread could exceed.
+       */
+      const res = await client.call<GetResponse<Thread>>("Thread/get", { accountId, ids: [threadId] });
+      const thread = res.list[0];
+      if (!thread) {
+        set((s) => {
+          const { [threadId]: _drop, ...rest } = s.loadingThreads;
+          return { loadingThreads: rest };
+        });
+        return [];
+      }
+      await get().getEmails(thread.emailIds, true);
       set((s) => {
-        const next = { ...s.emails };
-        const nextFull = { ...s.fullIds };
-        for (const e of emailsRes.list) {
-          next[e.id] = { ...next[e.id], ...e };
-          nextFull[e.id] = true;
-        }
         const { [threadId]: _drop, ...rest } = s.loadingThreads;
-        return { emails: next, fullIds: nextFull, threads: { ...s.threads, [threadId]: thread }, loadingThreads: rest };
+        return { threads: { ...s.threads, [threadId]: thread }, loadingThreads: rest };
       });
       return get().threadEmails(threadId);
     } catch (err) {
@@ -1057,29 +1070,54 @@ function isUnsupportedSort(err: unknown): boolean {
   return type === "unsupportedSort" || /unsupportedSort/i.test(message);
 }
 
-async function runQueryOnce(accountId: Id, q: ListQuery, position: number, limit: number) {
+async function runQueryOnce(accountId: Id, q: ListQuery, position: number, requested: number) {
+  // The ids are back-referenced into Email/get, which may carry no more than this.
+  const limit = Math.min(requested, client.maxObjectsInGet);
   const calls: Array<[string, Record<string, unknown>, string]> = [
     ["Email/query", { accountId, filter: q.filter, sort: q.sort, collapseThreads: q.collapseThreads, position, limit, calculateTotal: true }, "q"],
     ["Email/get", { accountId, "#ids": { resultOf: "q", name: "Email/query", path: "/ids" }, properties: LIST_PROPS }, "e"],
   ];
   if (q.collapseThreads) {
     calls.push(["Thread/get", { accountId, "#ids": { resultOf: "e", name: "Email/get", path: "/list/*/threadId" } }, "t"]);
-    calls.push(["Email/get", { accountId, "#ids": { resultOf: "t", name: "Thread/get", path: "/list/*/emailIds" }, properties: LIST_PROPS }, "te"]);
   }
   const res = await client.chain(calls);
   const query = res.get("q")?.[0] as unknown as QueryResponse;
   const emailsRes = res.get("e")?.[0] as unknown as GetResponse<Email>;
   const threadsRes = res.get("t")?.[0] as unknown as GetResponse<Thread> | undefined;
-  const threadEmails = res.get("te")?.[0] as unknown as GetResponse<Email> | undefined;
+  /*
+   * The other messages in each listed thread, for its count and unread state.
+   * These used to come back-referenced from Thread/get in the same request,
+   * with no bound: fifty long conversations could carry more ids than one
+   * Email/get may, and the server refused the whole page. Fetched separately
+   * instead, split to the limit, and only those not already held -- a cached
+   * one is kept current by Email/changes. When there is no state to follow
+   * changes from, every member is fetched, since nothing else will update it.
+   */
+  const following = useMail.getState().emailState !== null;
   useMail.setState((s) => {
     const emails = { ...s.emails };
     for (const e of emailsRes.list) emails[e.id] = { ...emails[e.id], ...e };
-    for (const e of threadEmails?.list ?? []) emails[e.id] = { ...emails[e.id], ...e };
     const threads = { ...s.threads };
     for (const t of threadsRes?.list ?? []) threads[t.id] = t;
     return { emails, threads, emailState: s.emailState ?? emailsRes.state };
   });
-  return { ids: query.ids, total: query.total ?? query.ids.length, queryState: query.queryState };
+  const members = (threadsRes?.list ?? []).flatMap((t) => t.emailIds);
+  if (members.length) await refreshEmails(accountId, following ? members.filter((id) => !useMail.getState().emails[id]) : members);
+  return { ids: query.ids, total: query.total ?? query.ids.length, queryState: query.queryState, limit };
+}
+
+/** Fetch list properties for `ids`, split to `maxObjectsInGet`, and merge them in. */
+async function refreshEmails(accountId: Id, ids: Id[]): Promise<void> {
+  const unique = [...new Set(ids)];
+  if (!unique.length) return;
+  const results = await Promise.all(
+    chunk(unique, client.maxObjectsInGet).map((part) => client.call<GetResponse<Email>>("Email/get", { accountId, ids: part, properties: LIST_PROPS })),
+  );
+  useMail.setState((s) => {
+    const emails = { ...s.emails };
+    for (const r of results) for (const e of r.list) emails[e.id] = { ...emails[e.id], ...e };
+    return { emails };
+  });
 }
 
 /**

web/src/views/mail/MailView.tsx

@@ -79,8 +79,25 @@ export function MailView({ mailboxId, threadId, search }: { mailboxId?: string;
     [settings.listSortScope, settings.listSortPreset, settings.listSortLevels],
   );
 
+  /*
+   * What the list query reads from the folder map: names, roles and places in
+   * the tree. `mailboxes` itself is replaced on every reload -- every push that
+   * touches mail reloads it for the counts -- and depending on it directly made
+   * each reload build a new query, which `query()` answered with a second full
+   * refresh of the list.
+   */
+  const folderShape = useMemo(
+    () =>
+      Object.values(mailboxes)
+        .map((m) => `${m.id}\u0000${m.name}\u0000${m.role ?? ""}\u0000${m.parentId ?? ""}`)
+        .sort()
+        .join("\u0001"),
+    [mailboxes],
+  );
+
   // Build & run the list query
   const listQuery = useMemo<ListQuery | null>(() => {
+    const mailboxes = useMail.getState().mailboxes;
     if (search) {
       if (!q) return null;
       const parsed = parseQuery(q);
@@ -94,7 +111,7 @@ export function MailView({ mailboxId, threadId, search }: { mailboxId?: string;
     // are outgoing, and collapsing them into their threads hides them.
     const isDraftsOrSent = mb?.role === "drafts" || mb?.role === "sent" || mailboxId === scheduledId;
     return { key: "", filter: { inMailbox: mailboxId }, sort: sortForFolder(mailboxId), collapseThreads: settings.conversationMode && !isDraftsOrSent, mailboxId };
-  }, [search, q, mailboxId, mailboxes, settings.conversationMode, scheduledId]);
+  }, [search, q, mailboxId, folderShape, settings.conversationMode, scheduledId]);
 
   useEffect(() => {
     if (listQuery && mailboxesLoaded) void query(listQuery);