From be57f23bf6ff330e30d6affc85f3186037fd51ff Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Wed, 4 Nov 2026 11:36:52 +0100 Subject: [PATCH] fix: say when merging contacts left a card behind, and don't let Undo copy it --- components/contacts/contacts-app.tsx | 3 +- .../contact-store-merge-import.test.ts | 23 +++++++++++++ stores/contact-store.ts | 34 +++++++++++++------ 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/components/contacts/contacts-app.tsx b/components/contacts/contacts-app.tsx index aba03ddfd..00e76d684 100644 --- a/components/contacts/contacts-app.tsx +++ b/components/contacts/contacts-app.tsx @@ -743,7 +743,8 @@ function ContactsAppContent({ linkSegments: routeSegments }: ContactsAppProps = setSelectedContact(plan.keep.id); setView("detail"); } - toast.success(tMerge("merged"), { + // Not every card went: say so, and still offer Undo for what changed. + (undo.incomplete ? toast.error : toast.success)(tMerge(undo.incomplete ? "merge_failed" : "merged"), { action: { label: tMerge("undo"), onClick: () => { diff --git a/stores/__tests__/contact-store-merge-import.test.ts b/stores/__tests__/contact-store-merge-import.test.ts index e1e91eb47..420905c1c 100644 --- a/stores/__tests__/contact-store-merge-import.test.ts +++ b/stores/__tests__/contact-store-merge-import.test.ts @@ -108,6 +108,29 @@ describe('mergeContacts', () => { expect(contacts.find((c) => c.uid === 'urn:uuid:old')).toBeTruthy(); }); + // A delete the server refused left the card in place while the toast said + // merged, and Undo then created it a second time. + it('says when a card could not be deleted, and Undo does not create it again', async () => { + useContactStore.setState({ contacts: [older, newer, team] }); + const client = makeClient(); + client.deleteContact.mockRejectedValueOnce(new Error('server said no')); + const undo = await useContactStore.getState().mergeContacts(client as unknown as IJMAPClient, planMerge([older, newer])); + expect(undo?.incomplete).toBe(true); + expect(undo?.others).toEqual([]); + expect(useContactStore.getState().contacts.some((c) => c.id === 'c-old')).toBe(true); + await useContactStore.getState().undoMergeContacts(client as unknown as IJMAPClient, undo!); + expect(client.createContact).not.toHaveBeenCalled(); + }); + + it('deletes nothing and still hands back Undo when moving a group member fails', async () => { + useContactStore.setState({ contacts: [older, newer, team] }); + const client = makeClient(); + client.updateContact.mockResolvedValueOnce(undefined).mockRejectedValueOnce(new Error('group write failed')); + const undo = await useContactStore.getState().mergeContacts(client as unknown as IJMAPClient, planMerge([older, newer])); + expect(undo).toMatchObject({ keepId: 'c-new', incomplete: true, others: [] }); + expect(client.deleteContact).not.toHaveBeenCalled(); + }); + it('deletes nothing when the kept card cannot be written', async () => { useContactStore.setState({ contacts: [older, newer, team] }); const client = makeClient(); diff --git a/stores/contact-store.ts b/stores/contact-store.ts index 8e55f2153..a73c62328 100644 --- a/stores/contact-store.ts +++ b/stores/contact-store.ts @@ -27,6 +27,8 @@ export interface MergeUndo { others: ContactCard[]; /** The groups' members before the merge. */ groups: Array<{ groupId: string; members: Record }>; + /** Some step after the kept card's update failed or was stopped: not all cards are gone. */ + incomplete?: boolean; } /** A contact that is about to be created, projected for plugins (no id yet). */ @@ -1745,20 +1747,30 @@ export const useContactStore = create()( } const groups: MergeUndo['groups'] = []; const moves = planGroupMoves(get().contacts.filter(c => c.kind === 'group'), keep, others); - for (const { group, members } of moves) { - if (sync) { - const accountId = group.isShared ? group.accountId : undefined; - await resolveAccountClient(client!, group.localAccountId) - .updateContact(group.originalId || group.id, { members }, accountId); + try { + for (const { group, members } of moves) { + if (sync) { + const accountId = group.isShared ? group.accountId : undefined; + await resolveAccountClient(client!, group.localAccountId) + .updateContact(group.originalId || group.id, { members }, accountId); + } + groups.push({ groupId: group.id, members: { ...(group.members ?? {}) } }); + set((state) => ({ + contacts: state.contacts.map(c => c.id === group.id ? { ...c, members } : c), + })); + void contactHooks.onContactGroupMemberChange.emit({ groupId: group.id, added: [keep.id], removed: others.map(o => o.id) }); } - groups.push({ groupId: group.id, members: { ...(group.members ?? {}) } }); - set((state) => ({ - contacts: state.contacts.map(c => c.id === group.id ? { ...c, members } : c), - })); - void contactHooks.onContactGroupMemberChange.emit({ groupId: group.id, added: [keep.id], removed: others.map(o => o.id) }); + } catch (error) { + // The kept card is changed already: hand back what Undo needs, delete nothing. + console.error('Failed to move group members while merging:', error); + return { keepId: keep.id, before, others: [], groups, incomplete: true }; } await get().bulkDeleteContacts(sync ? client : null, others.map(o => o.id)); - return { keepId: keep.id, before, others, groups }; + // A failed delete, or a plugin that stopped it, leaves cards in place: + // Undo must not create those again. + const left = new Set(get().contacts.map(c => c.id)); + const deleted = others.filter(o => !left.has(o.id)); + return { keepId: keep.id, before, others: deleted, groups, ...(deleted.length < others.length ? { incomplete: true } : {}) }; }, undoMergeContacts: async (client, undo) => {