diff --git a/lib/__tests__/jmap-contact-client.test.ts b/lib/__tests__/jmap-contact-client.test.ts index 8fb5a6147..d3e234405 100644 --- a/lib/__tests__/jmap-contact-client.test.ts +++ b/lib/__tests__/jmap-contact-client.test.ts @@ -134,6 +134,31 @@ describe('JMAPClient contact methods', () => { const result = await client.getAddressBooks(); expect(result).toEqual([]); }); + + it('should throw on network error when throwOnError is set', async () => { + const client = createClient(); + vi.spyOn(globalThis, 'fetch').mockRejectedValue(new Error('Network error')); + + await expect(client.getAddressBooks({ throwOnError: true })).rejects.toThrow('Network error'); + }); + + it('should throw on a JMAP method error when throwOnError is set', async () => { + const client = createClient(); + mockFetch({ + methodResponses: [['error', { type: 'serverFail', description: 'Too many requests' }, '0']], + }); + + await expect(client.getAddressBooks({ throwOnError: true })).rejects.toThrow('Too many requests'); + }); + + it('should still resolve to an empty list for a genuinely empty account', async () => { + const client = createClient(); + mockFetch({ + methodResponses: [['AddressBook/get', { list: [] }, '0']], + }); + + await expect(client.getAddressBooks({ throwOnError: true })).resolves.toEqual([]); + }); }); describe('getContacts', () => { diff --git a/lib/jmap/client-interface.ts b/lib/jmap/client-interface.ts index 2693f3d86..27655105c 100644 --- a/lib/jmap/client-interface.ts +++ b/lib/jmap/client-interface.ts @@ -265,12 +265,12 @@ export interface IJMAPClient { // ── Contacts ────────────────────────────────────────────────── getContactsAccountId(): string; - getAddressBooks(): Promise; + getAddressBooks(options?: { throwOnError?: boolean }): Promise; getAllAddressBooks(): Promise; createAddressBook(name: string): Promise; updateAddressBook(addressBookId: string, updates: Partial, targetAccountId?: string): Promise; deleteAddressBook(addressBookId: string, targetAccountId?: string): Promise; - getContacts(addressBookId?: string): Promise; + getContacts(addressBookId?: string, options?: { throwOnError?: boolean }): Promise; getAllContacts(): Promise; getContact(contactId: string, accountId?: string): Promise; createContact(contact: Partial, targetAccountId?: string): Promise; diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index d2655a774..d5c9e0e67 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -4241,7 +4241,7 @@ export class JMAPClient implements IJMAPClient { return [primaryId, ...accountIds]; } - async getAddressBooks(): Promise { + async getAddressBooks(options?: { throwOnError?: boolean }): Promise { try { const accountId = this.getContactsAccountId(); const response = await this.request([ @@ -4251,9 +4251,13 @@ export class JMAPClient implements IJMAPClient { if (response.methodResponses?.[0]?.[0] === "AddressBook/get") { return (response.methodResponses[0][1].list || []) as AddressBook[]; } - return []; + const methodError = response.methodResponses?.[0]?.[1]; + throw new Error(methodError?.description || methodError?.type || "AddressBook/get failed"); } catch (error) { console.error('Failed to get address books:', error); + // Callers that would treat an empty list as "nothing exists yet" must be + // able to tell a real empty account from a failed fetch (#730). + if (options?.throwOnError) throw error; return []; } } @@ -4500,13 +4504,14 @@ export class JMAPClient implements IJMAPClient { return allContacts; } - async getContacts(addressBookId?: string): Promise { + async getContacts(addressBookId?: string, options?: { throwOnError?: boolean }): Promise { try { const accountId = this.getContactsAccountId(); const filter = addressBookId ? { inAddressBook: addressBookId } : undefined; return await this.fetchPaginatedContacts(accountId, filter); } catch (error) { console.error('Failed to get contacts:', error); + if (options?.throwOnError) throw error; return []; } } diff --git a/stores/__tests__/contact-store.test.ts b/stores/__tests__/contact-store.test.ts index df6c32d52..d2c9020cf 100644 --- a/stores/__tests__/contact-store.test.ts +++ b/stores/__tests__/contact-store.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { useContactStore, getContactPhotoUri, normalizeContactPhotoUri } from '../contact-store'; +import { useContactStore, getContactPhotoUri, normalizeContactPhotoUri, TRUSTED_SENDERS_BOOK_NAME } from '../contact-store'; import type { ContactCard } from '@/lib/jmap/types'; vi.stubGlobal('crypto', { randomUUID: () => '00000000-0000-0000-0000-000000000000' }); @@ -560,6 +560,136 @@ describe('contact-store', () => { }); }); + describe('loadTrustedSendersBook (#730)', () => { + const trustedBook = (id: string) => ({ id, name: TRUSTED_SENDERS_BOOK_NAME }); + + const makeClient = (overrides: Record = {}) => ({ + getAddressBooks: vi.fn().mockResolvedValue([]), + createAddressBook: vi.fn(async (name: string) => ({ id: 'created-book', name })), + getContacts: vi.fn().mockResolvedValue([]), + createContact: vi.fn(), + updateContact: vi.fn().mockResolvedValue(undefined), + deleteContact: vi.fn().mockResolvedValue(undefined), + deleteAddressBook: vi.fn().mockResolvedValue(undefined), + ...overrides, + }); + + beforeEach(() => { + useContactStore.setState({ + trustedSendersBookId: null, + trustedSendersLoaded: false, + trustedSendersLoading: false, + trustedSenderEmails: [], + }); + }); + + it('does not create a book when the address book fetch fails', async () => { + const client = makeClient({ getAddressBooks: vi.fn().mockRejectedValue(new Error('429 rate limited')) }); + + await useContactStore.getState().loadTrustedSendersBook(client as never); + + expect(client.createAddressBook).not.toHaveBeenCalled(); + expect(useContactStore.getState().trustedSendersBookId).toBeNull(); + }); + + it('creates the book exactly once for a genuinely empty account', async () => { + const client = makeClient(); + + await useContactStore.getState().loadTrustedSendersBook(client as never); + + expect(client.createAddressBook).toHaveBeenCalledTimes(1); + expect(useContactStore.getState().trustedSendersBookId).toBe('created-book'); + }); + + it('shares one in-flight load between concurrent callers', async () => { + const client = makeClient(); + + await Promise.all([ + useContactStore.getState().loadTrustedSendersBook(client as never), + useContactStore.getState().loadTrustedSendersBook(client as never), + useContactStore.getState().loadTrustedSendersBook(client as never), + ]); + + expect(client.createAddressBook).toHaveBeenCalledTimes(1); + }); + + it('picks the same book on every client when duplicates already exist', async () => { + const client = makeClient({ + getAddressBooks: vi.fn().mockResolvedValue([trustedBook('book-c'), trustedBook('book-a'), trustedBook('book-b')]), + }); + + await useContactStore.getState().loadTrustedSendersBook(client as never); + + expect(client.createAddressBook).not.toHaveBeenCalled(); + expect(useContactStore.getState().trustedSendersBookId).toBe('book-a'); + }); + + it('merges duplicate books into the canonical one and deletes the leftovers', async () => { + const contactsByBook: Record = { + 'book-a': [makeContact({ id: 'c1', addressBookIds: { 'book-a': true }, emails: { e0: { address: 'kept@example.com' } } })], + 'book-b': [makeContact({ id: 'c2', addressBookIds: { 'book-b': true }, emails: { e0: { address: 'moved@example.com' } } })], + }; + const client = makeClient({ + getAddressBooks: vi.fn().mockResolvedValue([trustedBook('book-a'), trustedBook('book-b')]), + getContacts: vi.fn(async (bookId: string) => contactsByBook[bookId] ?? []), + }); + + await useContactStore.getState().loadTrustedSendersBook(client as never); + + expect(client.updateContact).toHaveBeenCalledWith('c2', { addressBookIds: { 'book-a': true } }); + expect(client.deleteAddressBook).toHaveBeenCalledWith('book-b'); + expect(useContactStore.getState().trustedSenderEmails.sort()).toEqual(['kept@example.com', 'moved@example.com']); + }); + + it('drops a duplicate card instead of copying it when the address is already trusted', async () => { + const duplicate = makeContact({ id: 'c2', addressBookIds: { 'book-b': true }, emails: { e0: { address: 'kept@example.com' } } }); + const contactsByBook: Record = { + 'book-a': [makeContact({ id: 'c1', addressBookIds: { 'book-a': true }, emails: { e0: { address: 'kept@example.com' } } })], + 'book-b': [duplicate], + }; + const client = makeClient({ + getAddressBooks: vi.fn().mockResolvedValue([trustedBook('book-a'), trustedBook('book-b')]), + getContacts: vi.fn(async (bookId: string) => contactsByBook[bookId] ?? []), + }); + + await useContactStore.getState().loadTrustedSendersBook(client as never); + + expect(client.deleteContact).toHaveBeenCalledWith('c2'); + expect(useContactStore.getState().trustedSenderEmails).toEqual(['kept@example.com']); + }); + + it('unfiles rather than destroys a card that also lives in another book', async () => { + const contactsByBook: Record = { + 'book-a': [makeContact({ id: 'c1', addressBookIds: { 'book-a': true }, emails: { e0: { address: 'kept@example.com' } } })], + 'book-b': [makeContact({ id: 'c2', addressBookIds: { 'book-b': true, personal: true }, emails: { e0: { address: 'kept@example.com' } } })], + }; + const client = makeClient({ + getAddressBooks: vi.fn().mockResolvedValue([trustedBook('book-a'), trustedBook('book-b')]), + getContacts: vi.fn(async (bookId: string) => contactsByBook[bookId] ?? []), + }); + + await useContactStore.getState().loadTrustedSendersBook(client as never); + + expect(client.deleteContact).not.toHaveBeenCalled(); + expect(client.updateContact).toHaveBeenCalledWith('c2', { addressBookIds: { personal: true } }); + }); + + it('keeps a duplicate book when its contacts cannot be read', async () => { + const client = makeClient({ + getAddressBooks: vi.fn().mockResolvedValue([trustedBook('book-a'), trustedBook('book-b')]), + getContacts: vi.fn(async (bookId: string) => { + if (bookId === 'book-b') throw new Error('Network error'); + return []; + }), + }); + + await useContactStore.getState().loadTrustedSendersBook(client as never); + + expect(client.deleteAddressBook).not.toHaveBeenCalled(); + expect(useContactStore.getState().trustedSendersBookId).toBe('book-a'); + }); + }); + describe('persistence/partialize', () => { it('should persist contacts when supportsSync is false', () => { const { partialize } = (useContactStore as unknown as { persist: { getOptions: () => { partialize: (state: Record) => Record } } }).persist.getOptions(); diff --git a/stores/contact-store.ts b/stores/contact-store.ts index df2ab54c3..f5c92f48e 100644 --- a/stores/contact-store.ts +++ b/stores/contact-store.ts @@ -140,6 +140,69 @@ export function getContactPhotoUri(contact: ContactCard): string | undefined { export const TRUSTED_SENDERS_BOOK_NAME = 'Trusted Senders'; +/** In-flight trusted senders load, shared by all callers to keep it single-flight. */ +let trustedSendersLoadPromise: Promise | null = null; + +/** Every e-mail address held by the given contacts, lowercased and de-duplicated. */ +export function collectContactEmails(contacts: ContactCard[]): string[] { + const seen = new Set(); + for (const contact of contacts) { + if (!contact.emails) continue; + for (const entry of Object.values(contact.emails)) { + const address = entry.address?.toLowerCase().trim(); + if (address) seen.add(address); + } + } + return Array.from(seen); +} + +/** + * Fold duplicate "Trusted Senders" books into the canonical one and delete the + * emptied leftovers (#730). Contacts that already exist in the canonical book + * are dropped rather than copied, so merging never produces two cards for the + * same address. A duplicate is only destroyed once every one of its contacts + * has been moved or removed - if any step fails the book is left untouched so + * no trusted sender is lost. + */ +export async function consolidateTrustedSenderBooks( + client: IJMAPClient, + canonicalBookId: string, + duplicates: AddressBook[], + knownEmails: string[] +): Promise { + const emails = new Set(knownEmails); + + for (const duplicate of duplicates) { + try { + const contacts = await client.getContacts(duplicate.id, { throwOnError: true }); + for (const contact of contacts) { + const addresses = collectContactEmails([contact]); + // Cards filed in other books too must never be destroyed - only unfiled + // from the duplicate. + const otherBookIds = Object.keys(contact.addressBookIds || {}).filter(id => id !== duplicate.id); + const addressBookIds: Record = {}; + for (const id of otherBookIds) addressBookIds[id] = true; + + if (addresses.some(address => !emails.has(address))) { + addressBookIds[canonicalBookId] = true; + await client.updateContact(contact.id, { addressBookIds }); + addresses.forEach(address => emails.add(address)); + } else if (otherBookIds.length > 0) { + await client.updateContact(contact.id, { addressBookIds }); + } else { + await client.deleteContact(contact.id); + } + } + await client.deleteAddressBook(duplicate.id); + debug.log('contacts', 'Merged duplicate trusted senders book:', duplicate.id); + } catch (error) { + debug.error('Failed to merge duplicate trusted senders book:', duplicate.id, error); + } + } + + return Array.from(emails); +} + interface ContactStore { contacts: ContactCard[]; addressBooks: AddressBook[]; @@ -1028,28 +1091,48 @@ export const useContactStore = create()( }, loadTrustedSendersBook: async (client) => { - if (get().trustedSendersLoading) return; + // Share the in-flight load so parallel callers (mail view, settings, + // the modal, addToTrustedSendersBook) can never create the book twice. + if (trustedSendersLoadPromise) return trustedSendersLoadPromise; set({ trustedSendersLoading: true }); - try { - debug.log('contacts', 'Loading trusted senders address book'); - const books = await client.getAddressBooks(); - let book = books.find(b => b.name === TRUSTED_SENDERS_BOOK_NAME); - if (!book) { - debug.log('contacts', 'Creating new trusted senders address book'); - book = await client.createAddressBook(TRUSTED_SENDERS_BOOK_NAME); + + trustedSendersLoadPromise = (async () => { + try { + debug.log('contacts', 'Loading trusted senders address book'); + // Must throw rather than yield [] on failure: treating a failed + // fetch as "no book yet" minted a duplicate on every hiccup (#730). + const books = await client.getAddressBooks({ throwOnError: true }); + // Sort so every client picks the same book when duplicates exist. + const matches = books + .filter(b => b.name === TRUSTED_SENDERS_BOOK_NAME) + .sort((a, b) => a.id.localeCompare(b.id)); + + let book = matches[0]; + if (!book) { + debug.log('contacts', 'Creating new trusted senders address book'); + book = await client.createAddressBook(TRUSTED_SENDERS_BOOK_NAME); + } + const bookId = book.id; + debug.log('contacts', 'Trusted senders book id:', bookId); + const contacts = await client.getContacts(bookId, { throwOnError: true }); + debug.log('contacts', 'Loaded', contacts.length, 'trusted sender contacts'); + let emails = collectContactEmails(contacts); + + if (matches.length > 1) { + debug.log('contacts', 'Found', matches.length, 'trusted senders books, merging duplicates'); + emails = await consolidateTrustedSenderBooks(client, bookId, matches.slice(1), emails); + } + + set({ trustedSendersBookId: bookId, trustedSenderEmails: emails, trustedSendersLoaded: true, trustedSendersLoading: false }); + } catch (error) { + debug.error('Failed to load trusted senders address book:', error); + set({ trustedSendersLoaded: true, trustedSendersLoading: false }); + } finally { + trustedSendersLoadPromise = null; } - const bookId = book.id; - debug.log('contacts', 'Trusted senders book id:', bookId); - const contacts = await client.getContacts(bookId); - debug.log('contacts', 'Loaded', contacts.length, 'trusted sender contacts'); - const emails = contacts.flatMap(c => - c.emails ? Object.values(c.emails).map(e => e.address.toLowerCase().trim()) : [] - ).filter(Boolean); - set({ trustedSendersBookId: bookId, trustedSenderEmails: emails, trustedSendersLoaded: true, trustedSendersLoading: false }); - } catch (error) { - debug.error('Failed to load trusted senders address book:', error); - set({ trustedSendersLoaded: true, trustedSendersLoading: false }); - } + })(); + + return trustedSendersLoadPromise; }, addToTrustedSendersBook: async (client, email) => {