fix: strip local-account prefix from address-book ids on contact update #1043

stores/__tests__/contact-store.test.ts

@@ -806,6 +806,156 @@ describe('contact-store', () => {
     });
   });
 
+  describe('address-book id namespacing on mutations (#133, #1043)', () => {
+    const makeClient = () => ({
+      getContactsAccountId: vi.fn(() => 'acct-primary'),
+      createContact: vi.fn(async (c: Partial<ContactCard>) => ({ ...makeContact(), ...c, id: 'created-id' })),
+      updateContact: vi.fn().mockResolvedValue(undefined),
+      deleteContact: vi.fn().mockResolvedValue(undefined),
+    });
+
+    it('strips the local-account prefix from a personal book on update', async () => {
+      // Multi-account Pro mode namespaces EVERY book, including the active
+      // account's own default one, as `local::bookId`. Updating such a contact
+      // used to send the namespaced id and Stalwart rejected the card with
+      // "Contact has to belong to at least one address book".
+      useContactStore.setState({
+        addressBooks: [
+          { id: 'me@host::ab-1', originalId: 'ab-1', localAccountId: 'me@host', name: 'Personal', isDefault: true },
+        ],
+        contacts: [makeContact({
+          id: 'me@host::c-1',
+          originalId: 'c-1',
+          localAccountId: 'me@host',
+          addressBookIds: { 'me@host::ab-1': true },
+        })],
+      });
+      const client = makeClient();
+
+      await useContactStore.getState().updateContact(
+        client as unknown as IJMAPClient,
+        'me@host::c-1',
+        { name: { components: [{ kind: 'given', value: 'Jane' }], isOrdered: true }, addressBookIds: { 'me@host::ab-1': true } },
+      );
+
+      expect(client.updateContact).toHaveBeenCalledWith(
+        'c-1',
+        expect.objectContaining({ addressBookIds: { 'ab-1': true } }),
+        undefined,
+      );
+      // Local state keeps the namespaced form the sidebar filters by.
+      expect(useContactStore.getState().contacts[0].addressBookIds).toEqual({ 'me@host::ab-1': true });
+    });
+
+    it('strips both local-account and shared-account prefixes on update', async () => {
+      useContactStore.setState({
+        addressBooks: [
+          { id: 'me@host::group-1:ab-7', originalId: 'ab-7', localAccountId: 'me@host', name: 'Team', isShared: true, accountId: 'group-1' },
+        ],
+        contacts: [makeContact({
+          id: 'me@host::group-1:c-9',
+          originalId: 'c-9',
+          localAccountId: 'me@host',
+          isShared: true,
+          accountId: 'group-1',
+          addressBookIds: { 'me@host::group-1:ab-7': true },
+        })],
+      });
+      const client = makeClient();
+
+      await useContactStore.getState().updateContact(
+        client as unknown as IJMAPClient,
+        'me@host::group-1:c-9',
+        { addressBookIds: { 'me@host::group-1:ab-7': true } },
+      );
+
+      expect(client.updateContact).toHaveBeenCalledWith('c-9', { addressBookIds: { 'ab-7': true } }, 'group-1');
+    });
+
+    it('falls back to prefix stripping when the book is not in state', async () => {
+      useContactStore.setState({
+        addressBooks: [],
+        contacts: [makeContact({
+          id: 'me@host::group-1:c-9',
+          originalId: 'c-9',
+          localAccountId: 'me@host',
+          isShared: true,
+          accountId: 'group-1',
+        })],
+      });
+      const client = makeClient();
+
+      await useContactStore.getState().updateContact(
+        client as unknown as IJMAPClient,
+        'me@host::group-1:c-9',
+        { addressBookIds: { 'me@host::group-1:ab-7': true } },
+      );
+
+      expect(client.updateContact).toHaveBeenCalledWith('c-9', { addressBookIds: { 'ab-7': true } }, 'group-1');
+    });
+
+    it('leaves raw ids untouched in single-account mode', async () => {
+      useContactStore.setState({
+        addressBooks: [{ id: 'ab-1', name: 'Personal', isDefault: true }],
+        contacts: [makeContact({ id: 'c-1' })],
+      });
+      const client = makeClient();
+
+      await useContactStore.getState().updateContact(
+        client as unknown as IJMAPClient,
+        'c-1',
+        { addressBookIds: { 'ab-1': true } },
+      );
+
+      expect(client.updateContact).toHaveBeenCalledWith('c-1', { addressBookIds: { 'ab-1': true } }, undefined);
+    });
+
+    it('strips the local-account prefix on create too', async () => {
+      useContactStore.setState({
+        addressBooks: [
+          { id: 'me@host::ab-1', originalId: 'ab-1', localAccountId: 'me@host', name: 'Personal', isDefault: true },
+        ],
+      });
+      const client = makeClient();
+
+      await useContactStore.getState().createContact(
+        client as unknown as IJMAPClient,
+        { addressBookIds: { 'me@host::ab-1': true } },
+      );
+
+      expect(client.createContact).toHaveBeenCalledWith(
+        expect.objectContaining({ addressBookIds: { 'ab-1': true } }),
+        undefined,
+      );
+    });
+
+    it('moves to a namespaced personal book with the raw id and keeps the display id locally', async () => {
+      const target = { id: 'me@host::ab-2', originalId: 'ab-2', localAccountId: 'me@host', name: 'Work' };
+      useContactStore.setState({
+        addressBooks: [
+          { id: 'me@host::ab-1', originalId: 'ab-1', localAccountId: 'me@host', name: 'Personal', isDefault: true },
+          target,
+        ],
+        contacts: [makeContact({
+          id: 'me@host::c-1',
+          originalId: 'c-1',
+          localAccountId: 'me@host',
+          addressBookIds: { 'me@host::ab-1': true },
+        })],
+      });
+      const client = makeClient();
+
+      await useContactStore.getState().moveContactToAddressBook(
+        client as unknown as IJMAPClient,
+        ['me@host::c-1'],
+        target,
+      );
+
+      expect(client.updateContact).toHaveBeenCalledWith('c-1', { addressBookIds: { 'ab-2': true } }, undefined);
+      expect(useContactStore.getState().contacts[0].addressBookIds).toEqual({ 'me@host::ab-2': true });
+    });
+  });
+
   describe('persistence/partialize', () => {
     it('should persist contacts when supportsSync is false', () => {
       const { partialize } = (useContactStore as unknown as { persist: { getOptions: () => { partialize: (state: Record<string, unknown>) => Record<string, unknown> } } }).persist.getOptions();

stores/contact-store.ts

@@ -122,6 +122,41 @@ function stripLocalAccountPrefix(id: string, localAccountId?: string): string {
   return id.startsWith(prefix) ? id.slice(prefix.length) : id;
 }
 
+/**
+ * Map the address-book ids the UI works with (namespaced per shared account
+ * as `accountId:bookId`, and per local account as `local::bookId` in
+ * multi-account Pro mode - possibly both) back to the raw server ids that
+ * `ContactCard/set` accepts. Resolution goes through the loaded address-book
+ * list first, since every book carries its `originalId`; ids that are not in
+ * the list (stale state, tests) fall back to stripping the known prefixes.
+ * Sending a namespaced id makes Stalwart reject the card with "Contact has
+ * to belong to at least one address book" (#133, #1043).
+ */
+function resolveRawAddressBookIds(
+  addressBookIds: Record<string, boolean>,
+  books: AddressBook[],
+  owner?: { localAccountId?: string; accountId?: string; isShared?: boolean },
+): { addressBookIds: Record<string, boolean>; sharedAccountId?: string; localAccountId?: string } {
+  const raw: Record<string, boolean> = {};
+  let sharedAccountId: string | undefined;
+  let localAccountId = owner?.localAccountId;
+  for (const [bookId, value] of Object.entries(addressBookIds)) {
+    const book = books.find(b => b.id === bookId);
+    if (book?.localAccountId) localAccountId = book.localAccountId;
+    if (book?.isShared && book.accountId) sharedAccountId = book.accountId;
+    if (book?.originalId) {
+      raw[book.originalId] = value;
+      continue;
+    }
+    let stripped = stripLocalAccountPrefix(bookId, localAccountId);
+    if (owner?.isShared && owner.accountId && stripped.startsWith(`${owner.accountId}:`)) {
+      stripped = stripped.slice(owner.accountId.length + 1);
+    }
+    raw[stripped] = value;
+  }
+  return { addressBookIds: raw, sharedAccountId, localAccountId };
+}
+
 export function getContactDisplayName(contact: ContactCard): string {
   if (contact.name) {
     // Try given + surname from components first
@@ -508,28 +543,15 @@ export const useContactStore = create<ContactStore>()(
           let cleanedContact = contact;
           let localAccountId = contact.localAccountId;
 
-          // De-namespace addressBookIds if they reference a shared address book
+          // De-namespace addressBookIds (shared and/or local-account prefixes)
           if (contact.addressBookIds) {
-            const books = get().addressBooks;
-            const deNamespaced: Record<string, boolean> = {};
-            let sharedAccountId: string | undefined;
-            for (const [bookId, value] of Object.entries(contact.addressBookIds)) {
-              const book = books.find(b => b.id === bookId);
-              if (book?.localAccountId) localAccountId = book.localAccountId;
-              if (book?.isShared && book.originalId) {
-                deNamespaced[book.originalId] = value;
-                sharedAccountId = book.accountId;
-              } else if (book?.originalId) {
-                deNamespaced[book.originalId] = value;
-              } else {
-                deNamespaced[bookId] = value;
-              }
-            }
-            if (sharedAccountId) {
-              accountId = sharedAccountId;
-              cleanedContact = { ...contact, addressBookIds: deNamespaced, isShared: true, accountId: sharedAccountId };
+            const resolved = resolveRawAddressBookIds(contact.addressBookIds, get().addressBooks, contact);
+            localAccountId = resolved.localAccountId;
+            if (resolved.sharedAccountId) {
+              accountId = resolved.sharedAccountId;
+              cleanedContact = { ...contact, addressBookIds: resolved.addressBookIds, isShared: true, accountId: resolved.sharedAccountId };
             } else {
-              cleanedContact = { ...contact, addressBookIds: deNamespaced };
+              cleanedContact = { ...contact, addressBookIds: resolved.addressBookIds };
             }
           }
 
@@ -562,17 +584,13 @@ export const useContactStore = create<ContactStore>()(
           const accountId = contact?.isShared ? contact.accountId : undefined;
           client = resolveAccountClient(client, contact?.localAccountId);
 
-          // De-namespace addressBookIds for shared contacts before sending to JMAP server
+          // De-namespace addressBookIds before sending to the JMAP server. This
+          // applies to personal books too: in multi-account Pro mode every id
+          // carries the `local::` prefix, and the server rejects it (#1043).
           let cleanedUpdates = updates;
-          if (contact?.isShared && contact?.accountId && updates.addressBookIds) {
-            const prefix = `${contact.accountId}:`;
-            const deNamespaced = Object.fromEntries(
-              Object.entries(updates.addressBookIds).map(([k, v]) => [
-                k.startsWith(prefix) ? k.slice(prefix.length) : k,
-                v
-              ])
-            );
-            cleanedUpdates = { ...updates, addressBookIds: deNamespaced };
+          if (updates.addressBookIds) {
+            const resolved = resolveRawAddressBookIds(updates.addressBookIds, get().addressBooks, contact);
+            cleanedUpdates = { ...updates, addressBookIds: resolved.addressBookIds };
           }
 
           await client.updateContact(originalId, cleanedUpdates, accountId);
@@ -969,12 +987,13 @@ export const useContactStore = create<ContactStore>()(
 
           // Same account: just update the addressBookIds
           if ((sourceAccountId || primaryAccountId) === (targetAccountId || primaryAccountId)) {
-            await client.updateContact(originalId, { addressBookIds: { [targetBookOriginalId]: true } }, sourceAccountId);
-            const isTargetPrimary = !targetAccountId || targetAccountId === primaryAccountId;
-            const localBookId = isTargetPrimary ? targetBookOriginalId : `${targetAccountId}:${targetBookOriginalId}`;
+            const ownerClient = resolveAccountClient(client, contact.localAccountId);
+            await ownerClient.updateContact(originalId, { addressBookIds: { [targetBookOriginalId]: true } }, sourceAccountId);
+            // Keep the id in the same (possibly namespaced) form the sidebar
+            // filters by; `addressBook.id` already is that form.
             set((state) => ({
               contacts: state.contacts.map(c =>
-                c.id === id ? { ...c, addressBookIds: { [localBookId]: true } } : c
+                c.id === id ? { ...c, addressBookIds: { [addressBook.id]: true } } : c
               ),
             }));
           } else {