From 7bb8447743db33c21b59d6cda97806a2a2930bbb Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Tue, 3 Nov 2026 05:47:00 +0100 Subject: [PATCH] fix: keep the contact list when the address book can't be read --- lib/jmap/client.ts | 27 ++++- lib/store/__tests__/client-contacts.test.ts | 113 ++++++++++++++++++++ stores/__tests__/contact-store.test.ts | 34 +++++- stores/contact-store.ts | 5 +- 4 files changed, 172 insertions(+), 7 deletions(-) create mode 100644 lib/store/__tests__/client-contacts.test.ts diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index 6d35dcdfa..56258dd86 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -7327,7 +7327,10 @@ export class JMAPClient implements IJMAPClient { } } + /** Throws when no account could answer, so a failure doesn't read as having no address books. */ async getAllAddressBooks(): Promise { + let answered = 0; + let failure: unknown; try { const allBooks: AddressBook[] = []; const primaryId = this.getContactsAccountId(); @@ -7353,17 +7356,23 @@ export class JMAPClient implements IJMAPClient { isShared: !isPrimary, })); allBooks.push(...books); + answered += 1; + } else if (response.methodResponses?.[0]?.[0] === "error") { + const refused = response.methodResponses[0][1]; + throw new Error(refused?.description || refused?.type || "AddressBook/get failed"); } } catch (error) { console.error(`Failed to fetch address books for account ${accountId}:`, error); + failure ??= error; } } - return allBooks; + if (answered > 0 || !failure) return allBooks; } catch (error) { console.error('Failed to fetch all address books:', error); return this.getAddressBooks(); } + throw failure; } async createAddressBook(name: string): Promise { @@ -8230,6 +8239,8 @@ export class JMAPClient implements IJMAPClient { ], this.contactUsing()); const queryResult = response.methodResponses?.[0]; + // A refused call isn't an empty address book: the caller keeps what it has. + if (queryResult?.[0] === "error") throw new Error(queryResult[1]?.description || queryResult[1]?.type || "ContactCard/query failed"); if (queryResult?.[0] !== "ContactCard/query") break; const ids: string[] = queryResult[1].ids || []; @@ -8252,8 +8263,10 @@ export class JMAPClient implements IJMAPClient { ["ContactCard/get", { accountId, ids: chunk }, "g"], ], this.contactUsing()); - if (response.methodResponses?.[0]?.[0] === "ContactCard/get") { - const list = (response.methodResponses[0][1].list || []) as ContactCard[]; + const getResult = response.methodResponses?.[0]; + if (getResult?.[0] === "error") throw new Error(getResult[1]?.description || getResult[1]?.type || "ContactCard/get failed"); + if (getResult?.[0] === "ContactCard/get") { + const list = (getResult[1].list || []) as ContactCard[]; allContacts.push(...list.map(contactFromWire)); } } @@ -8273,7 +8286,10 @@ export class JMAPClient implements IJMAPClient { } } + /** Throws when no account could answer, so a failure doesn't read as an empty address book. */ async getAllContacts(): Promise { + let answered = 0; + let failure: unknown; try { const allContacts: ContactCard[] = []; const primaryId = this.getContactsAccountId(); @@ -8297,16 +8313,19 @@ export class JMAPClient implements IJMAPClient { isShared: !isPrimary, })); allContacts.push(...contacts); + answered += 1; } catch (error) { console.error(`Failed to fetch contacts for account ${accountId}:`, error); + failure ??= error; } } - return allContacts; + if (answered > 0 || !failure) return allContacts; } catch (error) { console.error('Failed to fetch all contacts:', error); return this.getContacts(); } + throw failure; } async getContact(contactId: string, accountId?: string): Promise { diff --git a/lib/store/__tests__/client-contacts.test.ts b/lib/store/__tests__/client-contacts.test.ts new file mode 100644 index 000000000..87d516a34 --- /dev/null +++ b/lib/store/__tests__/client-contacts.test.ts @@ -0,0 +1,113 @@ +// @vitest-environment node +/** + * The real JMAPClient reading the store's answers: what the client's own code + * makes of them is the contract, not the store's idea of the shape. + */ +import { afterEach, describe, expect, it } from 'vitest'; +import { JMAPClient } from '@/lib/jmap/client'; +import { setOfflineHooks, type OfflineHooks } from '@/lib/jmap/offline-hooks'; +import { DEFAULT_SCOPE } from '../area'; +import { contactsArea } from '../contacts'; +import { AccountEngine } from '../engine/account-engine'; +import { JMAP_ACCOUNT } from './helpers/fake-jmap'; +import { CONTACTS_SESSION, FakeContacts, SHARED_ACCOUNT } from './helpers/fake-contacts'; +import { openMemoryStore } from './helpers/node-adapter'; + +afterEach(() => setOfflineHooks(null)); + +async function offlineClient(server: FakeContacts): Promise<{ client: JMAPClient; engine: AccountEngine }> { + const engine = new AccountEngine({ + account: 'a@fake.test', + db: openMemoryStore(), + session: CONTACTS_SESSION, + transport: server, + areas: [contactsArea], + scope: DEFAULT_SCOPE, + sleep: async () => {}, + }); + await engine.requestSync(); + engine.setOnline(false); + const hooks: Partial = { + skipNetwork: () => true, + answer: (_client, calls) => engine.answer(calls), + record: () => {}, + noteNetwork: () => {}, + }; + setOfflineHooks(hooks as OfflineHooks); + const client = JMAPClient.fromOfflineSession('https://fake.test', 'a@example.org', { + ...CONTACTS_SESSION, + capabilities: { ...CONTACTS_SESSION.capabilities, 'urn:ietf:params:jmap:core': { maxObjectsInGet: 2 } }, + }); + return { client, engine }; +} + +function seeded(): FakeContacts { + const server = new FakeContacts(); + for (const full of ['Anna Bauer', 'Bernd Graf', 'Carla Müller']) server.addCard({ name: { full } }); + server.addBook({ id: 'teambook', name: 'Team' }, SHARED_ACCOUNT); + server.addCard({ id: 'c1', name: { full: 'Dora Team' }, addressBookIds: { teambook: true } }, SHARED_ACCOUNT); + return server; +} + +describe('the client over the store', () => { + it('reports a store that has nothing yet as a failure, not as no contacts', async () => { + const engine = new AccountEngine({ + account: 'a@fake.test', db: openMemoryStore(), session: CONTACTS_SESSION, transport: new FakeContacts(), + areas: [contactsArea], scope: DEFAULT_SCOPE, sleep: async () => {}, + }); + // Answered per call, with one more call the store can answer: the error isn't hidden by a null answer. + const perCall: Partial = { + skipNetwork: () => true, + answer: async (_client, calls) => (await engine.answer(calls)) ?? { + methodResponses: calls.map(([, , id]) => ['error', { type: 'serverUnavailable' }, id]), + }, + record: () => {}, + noteNetwork: () => {}, + }; + setOfflineHooks(perCall as OfflineHooks); + const client = JMAPClient.fromOfflineSession('https://fake.test', 'a@example.org', CONTACTS_SESSION); + await expect(client.getAllContacts()).rejects.toThrow(); + await expect(client.getAllAddressBooks()).rejects.toThrow(); + }); + + it('lists all contacts and books across accounts, paging in its own batch size', async () => { + const { client } = await offlineClient(seeded()); + const contacts = await client.getAllContacts(); + expect(contacts.map((c) => c.name?.full).sort()).toEqual(['Anna Bauer', 'Bernd Graf', 'Carla Müller', 'Dora Team']); + const shared = contacts.find((c) => c.name?.full === 'Dora Team'); + expect(shared).toMatchObject({ id: `${SHARED_ACCOUNT}:c1`, originalId: 'c1', isShared: true }); + const books = await client.getAllAddressBooks(); + expect(books.map((b) => b.id).sort()).toEqual(['book1', `${SHARED_ACCOUNT}:teambook`]); + }); + + it('searches with its own filter', async () => { + const { client } = await offlineClient(seeded()); + expect((await client.searchContacts('müll')).map((c) => c.name?.full)).toEqual(['Carla Müller']); + expect((await client.searchContacts('team')).map((c) => c.originalId)).toEqual(['c1']); + }); + + it('creates, edits and deletes through its own methods with the stand-in id', async () => { + const { client, engine } = await offlineClient(seeded()); + const created = await client.createContact({ name: { full: 'Neue Person' }, emails: { e: { address: 'neu@example.org' } } }); + expect(created.id).toBeTruthy(); + expect(created.addressBookIds).toEqual({ book1: true }); + await client.updateContact(created.id, { name: { full: 'Neue Person B' } }); + expect((await client.getContact(created.id))?.name?.full).toBe('Neue Person B'); + expect((await client.searchContacts('person b')).map((c) => c.id)).toEqual([created.id]); + await client.deleteContact(created.id); + expect(await client.getContact(created.id)).toBeNull(); + expect(await engine.areaCall('contacts.pending', undefined)).toBe(0); + }); + + it('throws the server-style error for an edit to a card it does not have', async () => { + const { client } = await offlineClient(seeded()); + await expect(client.updateContact('nope', { name: { full: 'X' } })).rejects.toThrow(); + await expect(client.deleteContact('nope')).rejects.toThrow(); + }); + + it('reads an account the store has as the primary contacts account', async () => { + const { client } = await offlineClient(seeded()); + expect(client.getContactsAccountId()).toBe(JMAP_ACCOUNT); + expect((await client.getContacts('book1')).length).toBe(3); + }); +}); diff --git a/stores/__tests__/contact-store.test.ts b/stores/__tests__/contact-store.test.ts index 1a6fb072d..f9dfabcca 100644 --- a/stores/__tests__/contact-store.test.ts +++ b/stores/__tests__/contact-store.test.ts @@ -9,7 +9,7 @@ import { normalizeContactPhotoUri, TRUSTED_SENDERS_BOOK_NAME, } from '../contact-store'; -import type { ContactCard, ContactMedia } from '@/lib/jmap/types'; +import type { AddressBook, ContactCard, ContactMedia } from '@/lib/jmap/types'; import type { IJMAPClient } from '@/lib/jmap/client-interface'; import { clearContactPhotoCache } from '@/lib/contact-photo-cache'; import { PrincipalsRequestError } from '@/lib/jmap/principals-error'; @@ -1364,6 +1364,38 @@ describe('contact-store', () => { }); }); + describe('an account that cannot answer', () => { + it('keeps its contacts and books while the other accounts refresh', async () => { + const failing = { + getAllContacts: vi.fn().mockRejectedValue(new Error('offline')), + getAllAddressBooks: vi.fn().mockRejectedValue(new Error('offline')), + } as unknown as IJMAPClient; + const working = { + getAllContacts: vi.fn().mockResolvedValue([makeContact({ id: 'b1' })]), + getAllAddressBooks: vi.fn().mockResolvedValue([{ id: 'bookb', name: 'B' }]), + } as unknown as IJMAPClient; + const kept = makeContact({ id: 'acc-a::a1', localAccountId: 'acc-a' }); + const keptBook = { id: 'acc-a::booka', name: 'A', localAccountId: 'acc-a' } as AddressBook; + useContactStore.setState({ contacts: [kept], addressBooks: [keptBook] }); + const accounts = [{ client: failing, localAccountId: 'acc-a' }, { client: working, localAccountId: 'acc-b' }]; + + await useContactStore.getState().fetchAllAccountsContacts(accounts); + await useContactStore.getState().fetchAllAccountsAddressBooks(accounts); + + const state = useContactStore.getState(); + expect(state.contacts.map((c) => c.id)).toEqual(['acc-a::a1', 'acc-b::b1']); + expect(state.addressBooks.map((b) => b.id)).toEqual(['acc-a::booka', 'acc-b::bookb']); + }); + + it('keeps the list when the only account fails', async () => { + const failing = { getAllContacts: vi.fn().mockRejectedValue(new Error('offline')) } as unknown as IJMAPClient; + useContactStore.setState({ contacts: [makeContact()] }); + await useContactStore.getState().fetchContacts(failing); + expect(useContactStore.getState().contacts).toHaveLength(1); + expect(useContactStore.getState().error).toBe('Failed to fetch contacts'); + }); + }); + 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 cd10a91f7..3403088b2 100644 --- a/stores/contact-store.ts +++ b/stores/contact-store.ts @@ -671,7 +671,8 @@ export const useContactStore = create()( return prefixContactsWithLocalAccount(list, localAccountId); } catch (error) { debug.error(`Failed to fetch contacts for account ${localAccountId}:`, error); - return [] as ContactCard[]; + // Keep what this account's cards were: it just couldn't answer. + return get().contacts.filter((contact) => contact.localAccountId === localAccountId); } }), ); @@ -694,7 +695,7 @@ export const useContactStore = create()( return prefixAddressBooksWithLocalAccount(list, localAccountId); } catch (error) { debug.error(`Failed to fetch address books for account ${localAccountId}:`, error); - return [] as AddressBook[]; + return get().addressBooks.filter((book) => book.localAccountId === localAccountId); } }), );