diff --git a/stores/__tests__/email-store-unified-actions.test.ts b/stores/__tests__/email-store-unified-actions.test.ts index 36ce9a89c..26fd79e23 100644 --- a/stores/__tests__/email-store-unified-actions.test.ts +++ b/stores/__tests__/email-store-unified-actions.test.ts @@ -1,6 +1,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { useEmailStore } from '../email-store'; +import { useEmailStore, buildUnifiedAccountClients } from '../email-store'; import { useAuthStore } from '../auth-store'; +import { useAccountStore } from '../account-store'; import type { Email, Mailbox } from '@/lib/jmap/types'; import type { IJMAPClient } from '@/lib/jmap/client-interface'; import type { UnifiedAccountClient } from '@/lib/unified-mailbox'; @@ -55,6 +56,7 @@ function makeClient() { markAsRead: vi.fn().mockResolvedValue(undefined), toggleStar: vi.fn().mockResolvedValue(undefined), moveEmail: vi.fn().mockResolvedValue(undefined), + moveToTrash: vi.fn().mockResolvedValue(undefined), batchMarkAsRead: vi.fn().mockResolvedValue(undefined), batchDeleteEmails: vi.fn().mockResolvedValue(undefined), batchMoveEmails: vi.fn().mockResolvedValue(undefined), @@ -338,3 +340,132 @@ describe('unified-view single-email action routing (#281)', () => { expect(accountBClient.markAsRead).not.toHaveBeenCalled(); }); }); + +// JMAP account ids are only unique per server. Stalwart numbers accounts the +// same way on every install: userb logged in to the Stalwart 0.16.23 and the +// 1.0 test server is account `e` on both, and the team group is `c` on both +// (message ids repeat too). Batch writes in the unified view used to be +// grouped by account id alone, so both logins' messages went into one group +// and out through whichever login's client came first - to a server where the +// same ids name other messages. +describe('unified-view batch actions with two logins that share account ids', () => { + let client016: IJMAPClient; // userb on 0.16.23, the active login + let client10: IJMAPClient; // userb on 1.0 + + // Two installs need not number their folders alike: here the 1.0 login's + // Deleted Items are `k` (own) and `q` (team), the 0.16.23 login's `b`. + const own016 = [makeMailbox({ id: 'a', role: 'inbox', accountId: 'e' }), makeMailbox({ id: 'b', name: 'Deleted Items', role: 'trash', accountId: 'e' })]; + const own10 = [makeMailbox({ id: 'a', role: 'inbox', accountId: 'e' }), makeMailbox({ id: 'k', name: 'Deleted Items', role: 'trash', accountId: 'e' })]; + const team016 = [makeMailbox({ id: 'c:b', originalId: 'b', name: 'Deleted Items', role: 'trash', isShared: true, accountId: 'c' })]; + const team10 = [makeMailbox({ id: 'c:q', originalId: 'q', name: 'Deleted Items', role: 'trash', isShared: true, accountId: 'c' })]; + + beforeEach(() => { + client016 = makeClient(); + client10 = makeClient(); + useAuthStore.setState({ + activeAccountId: 'userb@v016', + getClientForAccount: (id: string) => + (id === 'userb@v016' ? client016 : id === 'userb@v10' ? client10 : undefined) as never, + } as never); + useEmailStore.setState({ + isUnifiedView: true, + unifiedRole: 'inbox', + viewingAccountId: null, + selectedMailbox: '', + selectedKeyword: null, + mailboxes: own016, + // What buildUnifiedAccountClients leaves behind: the bare account ids + // hold the list of the login fetched last. + accountMailboxes: { + 'userb@v016': own016, + 'userb@v10': own10, + e: own10, + c: team10, + 'userb@v016|e': own016, + 'userb@v10|e': own10, + 'userb@v016|c': team016, + 'userb@v10|c': team10, + }, + emails: [ + makeEmail({ id: 'm016', sourceClientAccountId: 'userb@v016', sourceAccountId: 'e', mailboxIds: { a: true } }), + makeEmail({ id: 'm10', sourceClientAccountId: 'userb@v10', sourceAccountId: 'e', mailboxIds: { a: true } }), + makeEmail({ id: 't016', sourceClientAccountId: 'userb@v016', sourceAccountId: 'c', mailboxIds: { a: true } }), + makeEmail({ id: 't10', sourceClientAccountId: 'userb@v10', sourceAccountId: 'c', mailboxIds: { a: true } }), + ], + selectedEmail: null, + selectedEmailIds: new Set(['m016', 'm10', 't016', 't10']), + processingReadStatus: new Set(), + unifiedScope: [], + unifiedCounts: [], + crossUnreadCount: 0, + }); + }); + + const callsOf = (fn: unknown) => (fn as ReturnType).mock.calls; + + it('marks each login\'s messages read through its own client', async () => { + await useEmailStore.getState().batchMarkAsRead(client016, true); + + expect(callsOf(client016.batchMarkAsRead)).toEqual([[['m016'], true, 'e'], [['t016'], true, 'c']]); + expect(callsOf(client10.batchMarkAsRead)).toEqual([[['m10'], true, 'e'], [['t10'], true, 'c']]); + }); + + it('destroys each login\'s messages through its own client', async () => { + await useEmailStore.getState().batchDelete(client016, true); + + expect(callsOf(client016.batchDeleteEmails)).toEqual([[['m016'], 'e'], [['t016'], 'c']]); + expect(callsOf(client10.batchDeleteEmails)).toEqual([[['m10'], 'e'], [['t10'], 'c']]); + }); + + it('moves each login\'s messages to that login\'s own Deleted Items', async () => { + await useEmailStore.getState().batchDelete(client016, false); + + expect(callsOf(client016.batchMoveEmails)).toEqual([[['m016'], 'b', 'e', false], [['t016'], 'b', 'c', false]]); + expect(callsOf(client10.batchMoveEmails)).toEqual([[['m10'], 'k', 'e', false], [['t10'], 'q', 'c', false]]); + }); + + it('moves a selection through each login\'s own client', async () => { + await useEmailStore.getState().batchMoveToMailbox(client016, 'archive'); + + expect(callsOf(client016.batchMoveEmails)).toEqual([[['m016'], 'archive', 'e'], [['t016'], 'archive', 'c']]); + expect(callsOf(client10.batchMoveEmails)).toEqual([[['m10'], 'archive', 'e'], [['t10'], 'archive', 'c']]); + }); + + it('moves dropped messages through each login\'s own client', async () => { + await useEmailStore.getState().moveEmailsToMailbox(client016, ['m016', 'm10', 't10'], 'archive'); + + expect(callsOf(client016.batchMoveEmails)).toEqual([[['m016'], 'archive', 'e']]); + expect(callsOf(client10.batchMoveEmails)).toEqual([[['m10'], 'archive', 'e'], [['t10'], 'archive', 'c']]); + }); + + it('deletes a single message into its own login\'s Deleted Items', async () => { + // The bare `e` entry holds the 1.0 login's folders; this is the other one's. + await useEmailStore.getState().deleteEmail(client016, 'm016'); + + expect(client016.moveToTrash).toHaveBeenCalledWith('m016', 'b', 'e', false); + expect(client10.moveToTrash).not.toHaveBeenCalled(); + }); + + it('caches each login\'s folder lists apart when building the unified view', async () => { + Object.assign(client016, { getAccountId: () => 'e', getAllMailboxes: vi.fn().mockResolvedValue([...own016, ...team016]) }); + Object.assign(client10, { getAccountId: () => 'e', getAllMailboxes: vi.fn().mockResolvedValue([...own10, ...team10]) }); + useAccountStore.setState({ + accounts: [ + { id: 'userb@v016', label: 'userb (0.16)', email: 'userb@example.org', isConnected: true }, + { id: 'userb@v10', label: 'userb (1.0)', email: 'userb@example.org', isConnected: true }, + ], + } as never); + useAuthStore.setState({ + getAllConnectedClients: () => new Map([['userb@v016', client016], ['userb@v10', client10]]), + } as never); + useEmailStore.setState({ accountMailboxes: {} }); + + await buildUnifiedAccountClients({ includeGroup: true }); + + const cached = useEmailStore.getState().accountMailboxes; + expect(cached['userb@v016|e']).toEqual(own016); + expect(cached['userb@v10|e']).toEqual(own10); + expect(cached['userb@v016|c']).toEqual(team016); + expect(cached['userb@v10|c']).toEqual(team10); + }); +}); diff --git a/stores/email-store.ts b/stores/email-store.ts index c8416adb1..14d68f7c5 100644 --- a/stores/email-store.ts +++ b/stores/email-store.ts @@ -848,8 +848,9 @@ function resolveCrossIncludedMailboxIds(accountId: string, ownMailboxes: Mailbox * is lost on the next reload (issue #281). We always pass `sourceAccountId` as * the JMAP accountId: for personal sources it equals the client's primary (a * no-op, no namespacing), for shared/group sources it targets the owner. The - * owner's mailbox list (cached by `buildUnifiedAccountClients` under that JMAP - * id) resolves role-based destinations like trash/archive. + * owner's mailbox list (cached by `buildUnifiedAccountClients` under the login + * and that JMAP id, see sourceMailboxes) resolves role-based destinations like + * trash/archive. * * For the normal single-account / viewing-account flow this preserves the * existing behavior exactly: the active/viewing client, its mailbox list, and @@ -874,7 +875,7 @@ function resolveEmailActionContext( if (email.sourceClientAccountId && email.sourceAccountId) { return { client: useAuthStore.getState().getClientForAccount(email.sourceClientAccountId) ?? resolveActionClient(passedClient), - mailboxes: state.accountMailboxes[email.sourceAccountId] ?? state.mailboxes, + mailboxes: sourceMailboxes(state, email.sourceClientAccountId, email.sourceAccountId) ?? state.mailboxes, accountId: email.sourceAccountId, }; } @@ -979,8 +980,11 @@ export async function buildUnifiedAccountClients( built.push({ accountId: a.id, accountLabel: a.label || a.email, client: c, mailboxes: ownMailboxes, clientAccountId: a.id, jmapAccountId: primaryJmapId, isShared: false, crossIncludedMailboxIds: resolveCrossIncludedMailboxIds(a.id, ownMailboxes) }); fetchedMailboxes[a.id] = ownMailboxes; // Also cache under the JMAP id so `accountMailboxes[email.sourceAccountId]` - // resolves uniformly for personal and shared sources alike. + // resolves uniformly for personal and shared sources alike, and under + // login plus JMAP id, which stays apart when another login's server uses + // the same account id (sourceMailboxes). fetchedMailboxes[primaryJmapId] = ownMailboxes; + fetchedMailboxes[sourceMailboxesKey(a.id, primaryJmapId)] = ownMailboxes; if (includeGroup) { const sharedByOwner = new Map(); @@ -1005,6 +1009,7 @@ export async function buildUnifiedAccountClients( // and batch actions can resolve role-based destinations (trash/archive) // in the owner account instead of falling back to the active account. fetchedMailboxes[ownerId] = ownerMailboxes; + fetchedMailboxes[sourceMailboxesKey(a.id, ownerId)] = ownerMailboxes; } } } catch { @@ -1101,6 +1106,69 @@ function isAggregateListView(): boolean { return s.isUnifiedView || !!s.selectedKeyword || isUnscopedSearchActive(s); } +/** A batch write's share for one source of an aggregate list (see groupBySource). */ +interface SourceBatch { + /** Login that reaches the messages (`sourceClientAccountId`). */ + clientAccountId?: string; + /** JMAP account that owns them; undefined for undecorated messages. */ + accountId?: string; + ids: string[]; +} + +/** + * Groups the messages of a batch write by the login that reaches each one and + * the JMAP account that owns it. Account ids are only unique per server: two + * logins on different servers often share them (Stalwart numbers accounts b, + * c, d, ... on every install), and a group keyed on the account alone sent one + * login's messages through the other's client - to a server where the same + * message ids name other messages. Messages without source stamps (a plain + * folder view) form one group without an account, for the caller's client. + */ +function groupBySource( + messages: Array<{ id: string; sourceClientAccountId?: string; sourceAccountId?: string }>, +): SourceBatch[] { + const groups = new Map(); + for (const message of messages) { + const accountId = message.sourceAccountId || undefined; + const clientAccountId = accountId ? message.sourceClientAccountId : undefined; + const key = accountId ? `${clientAccountId ?? ''}|${accountId}` : ''; + const group = groups.get(key) ?? { clientAccountId, accountId, ids: [] }; + group.ids.push(message.id); + groups.set(key, group); + } + return [...groups.values()]; +} + +/** + * The client for a SourceBatch: the stamped login's, or the caller's for + * undecorated messages. Undefined when the login is gone - never another + * login's client, which would write to the wrong server. + */ +function sourceBatchClient(batch: SourceBatch, passedClient: IJMAPClient): IJMAPClient | undefined { + if (!batch.accountId) return resolveActionClient(passedClient); + return batch.clientAccountId ? useAuthStore.getState().getClientForAccount(batch.clientAccountId) : undefined; +} + +/** + * Key of the folder list of one source of an aggregate list in + * `accountMailboxes`: the login plus the JMAP account. The list is cached + * under the bare JMAP id as well, but with two logins that share account ids + * that entry holds whichever login's list was fetched last. + */ +function sourceMailboxesKey(clientAccountId: string, jmapAccountId: string): string { + return `${clientAccountId}|${jmapAccountId}`; +} + +/** The cached folder list of a source of an aggregate list, if any (see sourceMailboxesKey). */ +function sourceMailboxes( + state: { accountMailboxes: Record }, + clientAccountId: string | undefined, + jmapAccountId: string, +): Mailbox[] | undefined { + return (clientAccountId ? state.accountMailboxes[sourceMailboxesKey(clientAccountId, jmapAccountId)] : undefined) + ?? state.accountMailboxes[jmapAccountId]; +} + /** * After a mailbox-list mutation (create/rename/delete/etc.), refresh the * cache for whichever account we're operating on. Writes the result to the @@ -2593,23 +2661,14 @@ export const useEmailStore = create((set, get) => ({ const affected = emails.filter(e => idSet.has(e.id)); if (isAggregateListView()) { - // In unified view, emails may span accounts – group by owning JMAP account - // and dispatch through the login client that can reach each one. The login - // client is keyed by `sourceClientAccountId` (a real AccountEntry.id); the - // owning account is `sourceAccountId` (passed for owner-scoped routing). - const bySource = new Map(); - for (const e of affected) { - const key = e.sourceAccountId || '__default__'; - if (!bySource.has(key)) bySource.set(key, { clientAccountId: e.sourceClientAccountId, ids: [] }); - bySource.get(key)!.ids.push(e.id); - } - await Promise.all(Array.from(bySource.entries()).map(async ([sourceAccountId, { clientAccountId, ids }]) => { - const acctClient = sourceAccountId === '__default__' - ? resolveActionClient(client) - : (clientAccountId ? useAuthStore.getState().getClientForAccount(clientAccountId) : undefined); + // In unified view, emails may span accounts – group by login and owning + // JMAP account and dispatch through the login client that can reach each + // one: `sourceClientAccountId` (a real AccountEntry.id) picks the client, + // `sourceAccountId` is passed for owner-scoped routing (groupBySource). + await Promise.all(groupBySource(affected).map(async (batch) => { + const acctClient = sourceBatchClient(batch, client); if (!acctClient) return; - const jmapAccountId = sourceAccountId === '__default__' ? undefined : sourceAccountId; - await acctClient.batchMoveEmails(ids, jmapDestId, jmapAccountId); + await acctClient.batchMoveEmails(batch.ids, jmapDestId, batch.accountId); })); } else { const currentMailbox = mailboxes.find(mb => mb.id === selectedMailbox); @@ -3199,24 +3258,14 @@ export const useEmailStore = create((set, get) => ({ const writes: { ids: string[]; write: () => Promise }[] = []; if (isAggregateListView()) { - // Group by owning JMAP account; dispatch through the reaching login client. - const bySource = new Map(); - for (const emailId of emailIdsArray) { - const email = emails.find(e => e.id === emailId); - const key = email?.sourceAccountId || '__default__'; - if (!bySource.has(key)) bySource.set(key, { clientAccountId: email?.sourceClientAccountId, ids: [] }); - bySource.get(key)!.ids.push(emailId); - } - - for (const [sourceAccountId, { clientAccountId, ids }] of bySource) { - const acctClient = sourceAccountId === '__default__' - ? resolveActionClient(client) - : (clientAccountId ? useAuthStore.getState().getClientForAccount(clientAccountId) : undefined); - const jmapAccountId = sourceAccountId === '__default__' ? undefined : sourceAccountId; + // Group by login and owning JMAP account; dispatch through the reaching + // login client (groupBySource). + for (const batch of groupBySource(emailIdsArray.map(id => emails.find(e => e.id === id) ?? { id }))) { + const acctClient = sourceBatchClient(batch, client); writes.push({ - ids, + ids: batch.ids, write: acctClient - ? () => acctClient.batchMarkAsRead(ids, read, jmapAccountId) + ? () => acctClient.batchMarkAsRead(batch.ids, read, batch.accountId) : () => Promise.reject(new Error('Not connected')), }); } @@ -3321,68 +3370,60 @@ export const useEmailStore = create((set, get) => ({ const forceDestroy = permanent || isInTrash || (isInJunk && permanentlyDeleteJunk); const alsoMarkRead = useSettingsStore.getState().deleteAction === 'trash-and-read'; - // Group emails by owning JMAP account (handles unified view and search results - // spanning accounts). Each group resolves the reaching login client via - // `sourceClientAccountId` and routes JMAP via `sourceAccountId`. Undecorated - // emails (normal single-mailbox view) fall into '__default__' = active client. + // Group emails by login and owning JMAP account (handles unified view and + // search results spanning accounts). Each group resolves the reaching login + // client via `sourceClientAccountId` and routes JMAP via `sourceAccountId`. + // Undecorated emails (normal single-mailbox view) form one group for the + // active client (groupBySource). const accountMailboxes = get().accountMailboxes; - const bySource = new Map(); - for (const emailId of emailIdsArray) { - const email = emails.find(e => e.id === emailId); - const key = email?.sourceAccountId || '__default__'; - if (!bySource.has(key)) bySource.set(key, { clientAccountId: email?.sourceClientAccountId, ids: [] }); - bySource.get(key)!.ids.push(emailId); - } + const batches = groupBySource(emailIdsArray.map(id => emails.find(e => e.id === id) ?? { id })); // Undecorated emails viewed in a shared/group folder directly (non-unified) // are reached through the active client but must carry the owner accountId, // or the move/destroy silently no-ops on the user's own account (see // resolveViewAccountId). undefined for a normal own-account view. const viewAccountId = currentMailbox?.isShared ? currentMailbox.accountId : undefined; - const getClient = (sourceAccountId: string, clientAccountId?: string) => - sourceAccountId === '__default__' - ? resolveActionClient(client) - : (clientAccountId ? useAuthStore.getState().getClientForAccount(clientAccountId) : undefined); - const mailboxesFor = (sourceAccountId: string) => - sourceAccountId === '__default__' + // Each group's trash comes from its own login's folder list: another + // login may use the same account id for an account with other folders. + const mailboxesFor = (batch: SourceBatch) => + !batch.accountId ? (viewAccountId ? (accountMailboxes[viewAccountId] ?? mailboxes) : mailboxes) - : (accountMailboxes[sourceAccountId] ?? mailboxes); - const jmapIdFor = (sourceAccountId: string) => - sourceAccountId === '__default__' ? viewAccountId : sourceAccountId; + : (sourceMailboxes({ accountMailboxes }, batch.clientAccountId, batch.accountId) ?? mailboxes); + const jmapIdFor = (batch: SourceBatch) => batch.accountId ?? viewAccountId; if (forceDestroy) { - const promises = Array.from(bySource.entries()).map(async ([sourceAccountId, { clientAccountId, ids }]) => { - const acctClient = getClient(sourceAccountId, clientAccountId); + const promises = batches.map(async (batch) => { + const acctClient = sourceBatchClient(batch, client); if (!acctClient) return; - await acctClient.batchDeleteEmails(ids, jmapIdFor(sourceAccountId)); + await acctClient.batchDeleteEmails(batch.ids, jmapIdFor(batch)); }); await Promise.allSettled(promises); } else { // Move to trash per account. - const failedAccounts: string[] = []; + let failedAccounts = 0; const movedEmailIds = new Set(); - const promises = Array.from(bySource.entries()).map(async ([sourceAccountId, { clientAccountId, ids }]) => { - const acctClient = getClient(sourceAccountId, clientAccountId); + const promises = batches.map(async (batch) => { + const acctClient = sourceBatchClient(batch, client); if (!acctClient) { - failedAccounts.push(sourceAccountId); + failedAccounts++; return; } - const trashMailbox = findTrashMailbox(mailboxesFor(sourceAccountId), { - accountId: jmapIdFor(sourceAccountId), + const trashMailbox = findTrashMailbox(mailboxesFor(batch), { + accountId: jmapIdFor(batch), }); if (!trashMailbox) { // No trash for this account: skip rather than silently destroying. // The user asked to move to trash, not permanently delete. - failedAccounts.push(sourceAccountId); + failedAccounts++; return; } const trashId = trashMailbox.originalId || trashMailbox.id; - await acctClient.batchMoveEmails(ids, trashId, jmapIdFor(sourceAccountId), alsoMarkRead); - ids.forEach(id => movedEmailIds.add(id)); + await acctClient.batchMoveEmails(batch.ids, trashId, jmapIdFor(batch), alsoMarkRead); + batch.ids.forEach(id => movedEmailIds.add(id)); }); await Promise.allSettled(promises); - if (failedAccounts.length > 0 && movedEmailIds.size === 0) { + if (failedAccounts > 0 && movedEmailIds.size === 0) { // Nothing moved - bail out so the UI doesn't drop the emails from view. throw new Error('Trash mailbox not found - cannot move emails to trash'); } @@ -3435,24 +3476,15 @@ export const useEmailStore = create((set, get) => ({ const emailIdsArray = Array.from(selectedEmailIds); if (isAggregateListView()) { - // Group by owning JMAP account; dispatch through the reaching login client. + // Group by login and owning JMAP account; dispatch through the reaching + // login client (groupBySource). const destMailbox = resolveActionMailboxes().find(mb => mb.id === toMailboxId); const jmapDestId = destMailbox?.originalId || toMailboxId; - const bySource = new Map(); - for (const emailId of emailIdsArray) { - const email = emails.find(e => e.id === emailId); - const key = email?.sourceAccountId || '__default__'; - if (!bySource.has(key)) bySource.set(key, { clientAccountId: email?.sourceClientAccountId, ids: [] }); - bySource.get(key)!.ids.push(emailId); - } - - const promises = Array.from(bySource.entries()).map(async ([sourceAccountId, { clientAccountId, ids }]) => { - const acctClient = sourceAccountId === '__default__' - ? resolveActionClient(client) - : (clientAccountId ? useAuthStore.getState().getClientForAccount(clientAccountId) : undefined); + const batches = groupBySource(emailIdsArray.map(id => emails.find(e => e.id === id) ?? { id })); + const promises = batches.map(async (batch) => { + const acctClient = sourceBatchClient(batch, client); if (!acctClient) return; - const jmapAccountId = sourceAccountId === '__default__' ? undefined : sourceAccountId; - await acctClient.batchMoveEmails(ids, jmapDestId, jmapAccountId); + await acctClient.batchMoveEmails(batch.ids, jmapDestId, batch.accountId); }); await Promise.allSettled(promises); } else { @@ -3646,7 +3678,7 @@ export const useEmailStore = create((set, get) => ({ if (listEmail?.sourceClientAccountId && listEmail?.sourceAccountId) { undoClient = useAuthStore.getState().getClientForAccount(listEmail.sourceClientAccountId) ?? undoClient; accountId = listEmail.sourceAccountId; - inboxMailboxes = get().accountMailboxes[listEmail.sourceAccountId] ?? mailboxes; + inboxMailboxes = sourceMailboxes(get(), listEmail.sourceClientAccountId, listEmail.sourceAccountId) ?? mailboxes; } else { const currentMailbox = mailboxes.find(m => m.id === selectedMailbox); accountId = currentMailbox?.accountId;