fix: route bulk move by authenticated client

2026-11-02 00:14:23 +00:00 · 2026-10-23 13:16:26 +03:00
parent d1f4ccaccf
commit 423c1bb329
2 changed files with 298 additions and 26 deletions
@@ -0,0 +1,251 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import { JMAPClient } from '@/lib/jmap/client';
+import { clearAllHooks } from '@/lib/plugin-hooks';
+import { emailKeyFor, threadKeyFor } from '@/lib/thread-utils';
+import type { Email, Mailbox } from '@/lib/jmap/types';
+import { useAuthStore } from '../auth-store';
+import { useEmailStore } from '../email-store';
+
+const initialStore = useEmailStore.getState();
+const initialAuth = useAuthStore.getState();
+const mailbox = (id: string, owner = 'owner', shared = false): Mailbox => ({
+  id: shared ? `${owner}:${id}` : id, originalId: id, accountId: owner,
+  name: id, isShared: shared, myRights: { mayAddItems: true },
+} as Mailbox);
+const email = (login: string, id: string, owner = 'owner'): Email => ({
+  id, threadId: `thread-${id}`, sourceClientAccountId: login, sourceAccountId: owner,
+  mailboxIds: { inbox: true }, keywords: {}, receivedAt: '2026-10-01T12:00:00Z',
+  size: 100, hasAttachment: false,
+} as Email);
+const unstamped = (id: string): Email => ({
+  ...email('a', id), sourceClientAccountId: undefined, sourceAccountId: undefined,
+});
+
+// Keep the real request serializer, authenticatedFetch and Email/set handling.
+// A synthetic connected session avoids discovery and keepalive traffic.
+function client(login: string, owner = 'owner'): JMAPClient {
+  const server = `https://${login}.example.invalid`;
+  const result = JMAPClient.withBearer(server, `token-${login}`, `${login}@example.invalid`);
+  Object.assign(result, { accountId: owner, apiUrl: `${server}/jmap/api` });
+  return result;
+}
+
+type SetArgs = { accountId: string; update: Record<string, { mailboxIds: Record<string, boolean> }> };
+type Wire = { url: string; auth: string | null; args: SetArgs };
+let a: JMAPClient;
+let b: JMAPClient;
+let clients: Map<string, JMAPClient>;
+let requests: Wire[];
+let serverMailboxes: Map<string, Record<string, boolean>>;
+let refresh: ReturnType<typeof vi.fn<typeof initialStore.refreshCurrentMailbox>>;
+const serverKey = (login: string, owner: string, id: string) => JSON.stringify([login, owner, id]);
+
+function select(rows: Email[], selected = rows): void {
+  useEmailStore.setState({
+    emails: rows, selectedEmail: selected[0] ?? null,
+    selectedEmailKeys: new Set(selected.map(emailKeyFor)),
+  });
+}
+
+function expectRequests(expected: Array<[string, string, string[], string?]>): void {
+  expect(requests).toEqual(expected.map(([login, owner, ids, dest = 'dest']) => ({
+    url: `https://${login}.example.invalid/jmap/api`, auth: `Bearer token-${login}`,
+    args: { accountId: owner, update: Object.fromEntries(ids.map(id => [id, { mailboxIds: { [dest]: true } }])) },
+  })));
+}
+
+async function expectPreflightFailure(): Promise<void> {
+  const before = useEmailStore.getState();
+  expect(await before.batchMoveToMailbox(a, 'dest')).toBe(false);
+  expect(fetch).not.toHaveBeenCalled();
+  expect(requests).toEqual([]);
+  expect(refresh).not.toHaveBeenCalled();
+  const after = useEmailStore.getState();
+  expect(after.error).toMatch(/not connected/i);
+  expect(after.isLoading).toBe(false);
+  expect(after.emails).toBe(before.emails);
+  expect(after.selectedEmailKeys).toBe(before.selectedEmailKeys);
+  expect(after.selectedEmail).toBe(before.selectedEmail);
+  expect(after.threadEmailsCache).toBe(before.threadEmailsCache);
+}
+
+beforeEach(() => {
+  clearAllHooks();
+  a = client('a'); b = client('b'); clients = new Map([['a', a], ['b', b]]);
+  requests = []; serverMailboxes = new Map();
+  refresh = vi.fn<typeof initialStore.refreshCurrentMailbox>().mockResolvedValue(undefined);
+  vi.stubGlobal('fetch', vi.fn(async (url: RequestInfo | URL, init?: RequestInit) => {
+    const body = JSON.parse(String(init?.body)) as { methodCalls: Array<[string, SetArgs, string]> };
+    const responses = body.methodCalls.map(([method, args, tag]) => {
+      expect(method).toBe('Email/set');
+      const auth = new Headers(init?.headers).get('Authorization');
+      const login = new URL(String(url)).hostname.split('.')[0];
+      expect(auth).toBe(`Bearer token-${login}`);
+      requests.push({ url: String(url), auth, args });
+      // These responses explicitly confirm every requested ID. Partial-result
+      // accounting is intentionally outside this routing regression suite.
+      for (const [id, patch] of Object.entries(args.update)) {
+        serverMailboxes.set(serverKey(login, args.accountId, id), patch.mailboxIds);
+      }
+      return [method, { accountId: args.accountId, updated: Object.fromEntries(Object.keys(args.update).map(id => [id, null])) }, tag];
+    });
+    return new Response(JSON.stringify({ methodResponses: responses }), {
+      status: 200, headers: { 'Content-Type': 'application/json' },
+    });
+  }));
+  useAuthStore.setState({
+    activeAccountId: 'a', client: a,
+    getClientForAccount: (id: string) => clients.get(id),
+  });
+  useEmailStore.setState({
+    ...initialStore, isUnifiedView: true, unifiedRole: 'inbox', crossView: null,
+    selectedMailbox: '__unified_inbox__', selectedKeyword: null, viewingAccountId: null,
+    searchQuery: '', emails: [], selectedEmail: null, selectedEmailKeys: new Set(),
+    mailboxes: [mailbox('inbox'), mailbox('dest')], accountMailboxes: {},
+    threadEmailsCache: new Map(), expandedThreadIds: new Set(),
+    error: null, isLoading: false, refreshCurrentMailbox: refresh,
+  });
+});
+
+afterEach(() => {
+  clearAllHooks();
+  vi.unstubAllGlobals();
+  useEmailStore.setState(initialStore);
+  useAuthStore.setState(initialAuth);
+});
+
+describe('bulk Move authenticated-client routing', () => {
+  it('keeps colliding owner IDs on their authenticated clients, leaving an unselected namesake alone', async () => {
+    const selectedA = email('a', 'p'), namesakeA = email('a', 'q'), selectedB = email('b', 'q');
+    select([selectedA, namesakeA, selectedB], [selectedA, selectedB]);
+    serverMailboxes.set(serverKey('a', 'owner', 'q'), { inbox: true });
+
+    expect(await useEmailStore.getState().batchMoveToMailbox(a, 'dest')).toBe(true);
+
+    expectRequests([['a', 'owner', ['p']], ['b', 'owner', ['q']]]);
+    expect(serverMailboxes.get(serverKey('a', 'owner', 'p'))).toEqual({ dest: true });
+    expect(serverMailboxes.get(serverKey('b', 'owner', 'q'))).toEqual({ dest: true });
+    expect(serverMailboxes.get(serverKey('a', 'owner', 'q'))).toEqual({ inbox: true });
+    expect(useEmailStore.getState().emails).toEqual([namesakeA]);
+    expect(useEmailStore.getState().selectedEmailKeys.size).toBe(0);
+  });
+
+  it('sends equal selected email IDs separately through each client', async () => {
+    select([email('a', 'same'), email('b', 'same')]);
+    expect(await useEmailStore.getState().batchMoveToMailbox(a, 'dest')).toBe(true);
+    expectRequests([['a', 'owner', ['same']], ['b', 'owner', ['same']]]);
+    expect(useEmailStore.getState().emails).toEqual([]);
+  });
+
+  it('moves only the selected client copy when another client has the same ID', async () => {
+    const other = email('a', 'same'), selected = email('b', 'same');
+    select([other, selected], [selected]);
+    await useEmailStore.getState().batchMoveToMailbox(a, 'dest');
+    expectRequests([['b', 'owner', ['same']]]);
+    expect(useEmailStore.getState().emails).toEqual([other]);
+  });
+
+  it('keeps delegated owners separate when they share a reaching client', async () => {
+    select([email('a', 'p'), email('a', 'q', 'shared')]);
+    await useEmailStore.getState().batchMoveToMailbox(a, 'dest');
+    expectRequests([['a', 'owner', ['p']], ['a', 'shared', ['q']]]);
+  });
+
+  it('groups aliases of the same effective client and owner into one request', async () => {
+    clients.set('alias-a', a);
+    select([email('a', 'p'), email('alias-a', 'q')]);
+    await useEmailStore.getState().batchMoveToMailbox(a, 'dest');
+    expectRequests([['a', 'owner', ['p', 'q']]]);
+  });
+
+  it('preserves personal-folder routing for unstamped messages', async () => {
+    useEmailStore.setState({ isUnifiedView: false, selectedMailbox: 'inbox' });
+    select([unstamped('p'), unstamped('q')]);
+    await useEmailStore.getState().batchMoveToMailbox(a, 'dest');
+    expectRequests([['a', 'owner', ['p', 'q']]]);
+    expect(refresh).toHaveBeenCalledWith(a);
+  });
+
+  it.each([false, true])('preserves direct shared-folder routing (viewing account=%s)', async viewing => {
+    const login = viewing ? 'b' : 'a';
+    const folders = [mailbox('inbox', 'shared', true), mailbox('dest', 'shared', true)];
+    useEmailStore.setState({
+      isUnifiedView: false, selectedMailbox: 'shared:inbox', viewingAccountId: viewing ? 'b' : null,
+      mailboxes: viewing ? [] : folders, accountMailboxes: viewing ? { b: folders } : {},
+    });
+    select([unstamped('p'), unstamped('q')]);
+    await useEmailStore.getState().batchMoveToMailbox(a, 'shared:dest');
+    expectRequests([[login, 'shared', ['p', 'q']]]);
+  });
+
+  it('uses the viewing client for an unstamped personal folder', async () => {
+    b = client('b', 'owner-b'); clients.set('b', b);
+    useEmailStore.setState({
+      isUnifiedView: false, selectedMailbox: 'inbox', viewingAccountId: 'b',
+      accountMailboxes: { b: [mailbox('inbox', 'owner-b'), mailbox('dest', 'owner-b')] },
+    });
+    select([unstamped('p'), unstamped('q')]);
+    await useEmailStore.getState().batchMoveToMailbox(a, 'dest');
+    expectRequests([['b', 'owner-b', ['p', 'q']]]);
+  });
+
+  it('honors source stamps after the aggregate-view flag has changed', async () => {
+    useEmailStore.setState({ isUnifiedView: false, selectedMailbox: 'inbox' });
+    select([email('b', 'p'), email('b', 'q')]);
+    await useEmailStore.getState().batchMoveToMailbox(a, 'dest');
+    expectRequests([['b', 'owner', ['p', 'q']]]);
+  });
+
+  it('routes selected expanded-thread members without moving an unselected list namesake', async () => {
+    const other = email('a', 'same'), member = email('b', 'same');
+    select([other], [member]);
+    useEmailStore.setState({ threadEmailsCache: new Map([[threadKeyFor(member), [member]]]) });
+    await useEmailStore.getState().batchMoveToMailbox(a, 'dest');
+    expectRequests([['b', 'owner', ['same']]]);
+    expect(useEmailStore.getState().emails).toEqual([other]);
+  });
+
+  it.each(['owner', undefined])('preflights a disconnected explicit source (owner=%s) before sending any group', async owner => {
+    clients.delete('b');
+    select([email('a', 'p'), { ...email('b', 'q'), sourceAccountId: owner }]);
+    await expectPreflightFailure();
+  });
+
+  it('rejects an owner stamp without a reaching login instead of using the active client', async () => {
+    select([email('a', 'p'), { ...email('b', 'q'), sourceClientAccountId: undefined }]);
+    await expectPreflightFailure();
+  });
+
+  it.each([false, true])('preflights an unavailable viewing client (shared=%s)', async shared => {
+    clients.delete('b');
+    useEmailStore.setState({
+      isUnifiedView: false, selectedMailbox: shared ? 'shared:inbox' : 'inbox', viewingAccountId: 'b',
+      accountMailboxes: { b: [mailbox('inbox', 'shared', shared), mailbox('dest', 'shared', shared)] },
+    });
+    select([unstamped('p'), unstamped('q')]);
+    await expectPreflightFailure();
+  });
+
+  it('preflights unavailable source stamps outside aggregate views too', async () => {
+    clients.delete('b');
+    useEmailStore.setState({ isUnifiedView: false, selectedMailbox: 'inbox' });
+    select([email('a', 'p'), email('b', 'q')]);
+    await expectPreflightFailure();
+  });
+
+  it('preflights the viewing client even when a stamped group was resolved first', async () => {
+    clients.delete('b');
+    useEmailStore.setState({ viewingAccountId: 'b' });
+    select([email('a', 'p'), unstamped('q')]);
+    await expectPreflightFailure();
+  });
+
+  it('preserves explicit-target routing without using the unrelated selection', async () => {
+    const selected = email('a', 'selected');
+    select([selected]);
+    await useEmailStore.getState().batchMoveToMailbox(a, 'dest', { client: b, accountId: 'shared', emailIds: ['unloaded'] });
+    expectRequests([['b', 'shared', ['unloaded']]]);
+    expect(useEmailStore.getState().emails).toEqual([selected]);
+    expect([...useEmailStore.getState().selectedEmailKeys]).toEqual([emailKeyFor(selected)]);
+  });
+});
@@ -4810,7 +4810,7 @@ export const useEmailStore = create<EmailStore>((set, get) => ({

    set({ isLoading: true, error: null });
    try {
-      // Ids of an account whose move failed or that no client reaches.
+      // Ids of an account group whose move failed.
      const notMoved = new Set<string>();

      if (target) {
@@ -4818,40 +4818,61 @@ export const useEmailStore = create<EmailStore>((set, get) => ({
        // source stamps cannot route them; the caller names the account and
        // the destination's JMAP id.
        await target.client.batchMoveEmails(emailIdsArray, toMailboxId, target.accountId);
-      } else if (isAggregateListView()) {
-        // Group by owning JMAP account; dispatch through the reaching login client.
+      } else {
        const destMailbox = resolveActionMailboxes().find(mb => mb.id === toMailboxId);
        const jmapDestId = destMailbox?.originalId || toMailboxId;
-        const bySource = new Map<string, { clientAccountId?: string; ids: string[] }>();
+        const aggregate = isAggregateListView();
+        const viewingId = get().viewingAccountId;
+        const sourceClients = new Map<string, IJMAPClient>();
+        const requireClient = (loginId: string): IJMAPClient => {
+          const cached = sourceClients.get(loginId);
+          if (cached) return cached;
+          const connected = useAuthStore.getState().getClientForAccount(loginId);
+          if (!connected) throw new Error('Email account is not connected - cannot move emails');
+          sourceClients.set(loginId, connected);
+          return connected;
+        };
+        const groups: Array<{
+          client: IJMAPClient;
+          accountId: string | undefined;
+          owner: string | undefined;
+          ids: string[];
+        }> = [];
+
+        // Preflight every required login before any group is dispatched. An
+        // explicit source/viewing client must never fall back to another login.
        for (const email of selectedEmails) {
-          const key = email.sourceAccountId || '__default__';
-          if (!bySource.has(key)) bySource.set(key, { clientAccountId: email.sourceClientAccountId, ids: [] });
-          bySource.get(key)!.ids.push(email.id);
+          let actionClient: IJMAPClient;
+          let accountId: string | undefined;
+          if (email.sourceClientAccountId || email.sourceAccountId) {
+            if (!email.sourceClientAccountId) throw new Error('Email account is not connected - cannot move emails');
+            actionClient = requireClient(email.sourceClientAccountId);
+            accountId = email.sourceAccountId;
+          } else {
+            actionClient = viewingId ? requireClient(viewingId) : client;
+            accountId = resolveViewAccountId();
+          }
+          // Owner IDs are server-local; keep the effective authenticated client
+          // in the identity. Unstamped primary rows retain the default accountId.
+          const owner = accountId ?? actionClient.getAccountId?.();
+          let group = groups.find(g => g.client === actionClient && g.owner === owner);
+          if (!group) {
+            group = { client: actionClient, accountId, owner, ids: [] };
+            groups.push(group);
+          }
+          group.ids.push(email.id);
        }

-        const promises = Array.from(bySource.entries()).map(async ([sourceAccountId, { clientAccountId, ids }]) => {
-          const acctClient = sourceAccountId === '__default__'
-            ? resolveActionClient(client)
-            : (clientAccountId ? useAuthStore.getState().getClientForAccount(clientAccountId) : undefined);
-          if (!acctClient) {
-            ids.forEach(id => notMoved.add(id));
-            return;
-          }
-          const jmapAccountId = sourceAccountId === '__default__' ? undefined : sourceAccountId;
-          await acctClient.batchMoveEmails(ids, jmapDestId, jmapAccountId).catch((error) => {
+        const promises = groups.map(async ({ client: actionClient, accountId, ids }) => {
+          await actionClient.batchMoveEmails(ids, jmapDestId, accountId).catch((error) => {
            ids.forEach(id => notMoved.add(id));
            throw error;
          });
        });
-        await Promise.allSettled(promises);
-      } else {
-        // Non-unified: route to the viewed shared/group account (if any), reached
-        // through the active client. Resolve the destination to its bare owner id
-        // (`originalId`) since shared folders use namespaced store ids.
-        const viewAccountId = resolveViewAccountId();
-        const destMailbox = resolveActionMailboxes().find(mb => mb.id === toMailboxId);
-        const jmapDestId = destMailbox?.originalId || toMailboxId;
-        await resolveActionClient(client).batchMoveEmails(emailIdsArray, jmapDestId, viewAccountId);
+        // Preserve the existing aggregate/folder failure behavior. Per-message
+        // confirmation and partial-failure reconciliation are separate concerns.
+        if (aggregate) await Promise.allSettled(promises);
+        else await Promise.all(promises);
      }

      // Update local state - remove from current view since they moved