From 3c156673d50ca09560c5cf1ac6f172dc4475013f Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 04:34:17 +0100 Subject: [PATCH] fix: ask before a contact import replaces a card with the same UID --- locales/rn/de.json | 4 +- locales/rn/en.json | 2 + .../contacts/ContactImportSheet.tsx | 47 ++++++++++++++-- src/stores/__tests__/contacts-store.test.ts | 44 +++++++++++++++ src/stores/contacts-store.ts | 53 +++++++++++++++---- 5 files changed, 135 insertions(+), 15 deletions(-) diff --git a/locales/rn/de.json b/locales/rn/de.json index 409969f4..4b860f2f 100644 --- a/locales/rn/de.json +++ b/locales/rn/de.json @@ -253,7 +253,9 @@ "into_address_book": "In Adressbuch importieren", "into_named": "In {name} importieren", "no_address_book": "Erstellen Sie ein Adressbuch, bevor Sie Kontakte importieren.", - "picker_failed": "Die Dateiauswahl konnte nicht geöffnet werden" + "picker_failed": "Die Dateiauswahl konnte nicht geöffnet werden", + "replaces": "Ersetzt {name}", + "update_existing": "Vorhandenen aktualisieren" }, "no_tags_hint": "Noch keine Kategorien. Geben Sie oben eine ein.", "picker": { diff --git a/locales/rn/en.json b/locales/rn/en.json index 7c59befb..bf768f30 100644 --- a/locales/rn/en.json +++ b/locales/rn/en.json @@ -261,7 +261,9 @@ "into_named": "Import into {name}", "no_address_book": "Create an address book before importing contacts.", "picker_failed": "Could not open the file picker", + "replaces": "Replaces {name}", "unnamed": "Unnamed contact", + "update_existing": "Update existing", "updated_count": "{count, plural, one {1 contact was already there and has been updated.} other {# contacts were already there and have been updated.}}" }, "no_tags_hint": "No tags yet. Type one above.", diff --git a/src/components/contacts/ContactImportSheet.tsx b/src/components/contacts/ContactImportSheet.tsx index f87a409a..1d88d1d1 100644 --- a/src/components/contacts/ContactImportSheet.tsx +++ b/src/components/contacts/ContactImportSheet.tsx @@ -8,7 +8,7 @@ import { Upload, X, Check, FileText, AlertTriangle, BookUser } from '../icons'; import type { ContactCard } from '../../api/types'; import { parseVCard, detectDuplicates } from '../../lib/vcard'; import { getContactDisplayName, getContactPrimaryEmail } from '../../lib/contact-utils'; -import { useContactsStore, type ContactImportResult } from '../../stores/contacts-store'; +import { selectImportUidMatches, useContactsStore, type ContactImportResult } from '../../stores/contacts-store'; import Button from '../Button'; import { spacing, radius, typography, type ThemePalette } from '../../theme/tokens'; import { useColors } from '../../theme/colors'; @@ -49,6 +49,7 @@ export default function ContactImportSheet({ const insets = useSafeAreaInsets(); const t = useLocaleStore((s) => s.t); const existing = useContactsStore((s) => s.contacts); + const addressBooks = useContactsStore((s) => s.addressBooks); const importContacts = useContactsStore((s) => s.importContacts); const [parsed, setParsed] = React.useState([]); @@ -57,6 +58,28 @@ export default function ContactImportSheet({ const [error, setError] = React.useState(null); const [importing, setImporting] = React.useState(false); const [result, setResult] = React.useState(null); + // Cards whose UID the target's account already has: importing one replaces + // that card, which may be another contact (an older export, another app's + // "1", "2" UIDs). Left unselected, like an e-mail duplicate, until the user + // picks them. + const uidMatches = React.useMemo( + () => selectImportUidMatches({ contacts: existing, addressBooks }, parsed, targetBookId), + [existing, addressBooks, parsed, targetBookId], + ); + // Another target book (another account) or a refreshed list can match + // cards the user has not seen marked yet: those start unselected too. + const shownMatches = React.useRef>(new Map()); + React.useEffect(() => { + const before = shownMatches.current; + shownMatches.current = uidMatches; + const fresh = [...uidMatches.keys()].filter((idx) => !before.has(idx)); + if (fresh.length === 0) return; + setSelected((prev) => { + const next = new Set(prev); + for (const idx of fresh) next.delete(idx); + return next; + }); + }, [uidMatches]); const slideY = React.useRef(new Animated.Value(900)).current; // Centred on a wide window, clear of side insets in landscape. @@ -120,8 +143,9 @@ export default function ContactImportSheet({ return; } const dupes = detectDuplicates(existing, contacts); + const replaces = selectImportUidMatches({ contacts: existing, addressBooks }, contacts, targetBookId); const initial = new Set(); - contacts.forEach((_, idx) => { if (!dupes.has(idx)) initial.add(idx); }); + contacts.forEach((_, idx) => { if (!dupes.has(idx) && !replaces.has(idx)) initial.add(idx); }); setParsed(contacts); setDuplicates(dupes); setSelected(initial); @@ -152,9 +176,15 @@ export default function ContactImportSheet({ } const toImport = parsed.filter((_, i) => selected.has(i)); if (toImport.length === 0) return; + // Only the existing cards the user left selected are replaced. + const updateUids = new Set(); + for (const idx of selected) { + const uid = parsed[idx]?.uid; + if (uid && uidMatches.has(idx)) updateUids.add(uid); + } setImporting(true); try { - const outcome = await importContacts(toImport, targetBookId); + const outcome = await importContacts(toImport, targetBookId, updateUids); setResult(outcome); onImported?.(outcome.imported + outcome.updated); } catch (e) { @@ -276,6 +306,8 @@ export default function ContactImportSheet({ const name = getContactDisplayName(contact); const email = getContactPrimaryEmail(contact); const isDupe = duplicates.has(idx); + const replaces = uidMatches.get(idx); + const replacesName = replaces ? getContactDisplayName(replaces) || getContactPrimaryEmail(replaces) : ''; const isSelected = selected.has(idx); return ( {email} )} + {!!replaces && !!replacesName && replacesName !== (name || email) && ( + + {t('contacts.import.replaces', 'Replaces {name}', { name: replacesName })} + + )} - {isDupe && ( + {replaces ? ( + {t('contacts.import.update_existing', 'Update existing')} + ) : isDupe && ( {t('contacts.import.duplicate', 'Duplicate')} )} diff --git a/src/stores/__tests__/contacts-store.test.ts b/src/stores/__tests__/contacts-store.test.ts index 6664be61..c4b78a27 100644 --- a/src/stores/__tests__/contacts-store.test.ts +++ b/src/stores/__tests__/contacts-store.test.ts @@ -71,6 +71,7 @@ import { selectGroupMembers, sortContactsByName, selectCreateTargetBookId, + selectImportUidMatches, CONTACTS_STALE_MS, } from '../contacts-store'; import type { AddressBook, ContactCard } from '../../api/types'; @@ -429,6 +430,7 @@ describe('contacts-store', () => { const result = await useContactsStore.getState().importContacts( [{ id: 'import-1', uid: 'urn:uuid:aaa', addressBookIds: { x: true }, name: { full: 'New' } }], 'ab-1', + new Set(['urn:uuid:aaa']), ); expect(mockCreateContact).not.toHaveBeenCalled(); expect(mockUpdateContact).toHaveBeenCalledWith('c1', { name: { full: 'New' } }, undefined); @@ -445,10 +447,52 @@ describe('contacts-store', () => { const result = await useContactsStore.getState().importContacts( [{ id: 'import-1', uid: 'urn:uuid:bbb', name: { full: 'B' } }], 'ab-1', + new Set(['urn:uuid:bbb']), ); expect(mockUpdateContact).toHaveBeenCalledWith('m', { name: { full: 'B' } }, undefined); expect(result).toMatchObject({ imported: 0, updated: 1, failed: 0, failures: [] }); }); + + // Another app's export numbers its cards "1", "2": the same UID can be + // another contact, which the import replaced wholesale without asking. + it('replaces a card with the same UID only when the user chose to', async () => { + useContactsStore.setState({ + addressBooks: [{ id: 'ab-1', name: 'P' }], + contacts: [card('c1', { uid: '1', name: { full: 'Anna' } }), card('c2', { uid: '2', name: { full: 'Ben' } })], + }); + mockCreateContact.mockRejectedValueOnce(new Error('Contact with UID 3 already exists with id c9. (uid)')); + const result = await useContactsStore.getState().importContacts( + [ + { id: 'import-1', uid: '1', name: { full: 'Carla' } }, + { id: 'import-2', uid: '2', name: { full: 'Ben B.' } }, + { id: 'import-3', uid: '3', name: { full: 'Dora' } }, + ], + 'ab-1', + new Set(['2']), + ); + expect(mockUpdateContact).toHaveBeenCalledTimes(1); + expect(mockUpdateContact).toHaveBeenCalledWith('c2', { name: { full: 'Ben B.' } }, undefined); + expect(result).toMatchObject({ imported: 0, updated: 1, failed: 2 }); + expect(result.failures.map((f) => f.name)).toEqual(['Carla', 'Dora']); + expect(useContactsStore.getState().contacts.map((c) => c.name?.full)).toEqual(['Anna', 'Ben B.']); + }); + + it('tells the import sheet which cards an import would replace, in the target book\'s account', () => { + const state = { + addressBooks: [ + { id: 'ab-1', name: 'P' }, + { id: 'acc-team:ab-9', originalId: 'ab-9', name: 'Team', isShared: true, accountId: 'acc-team' }, + ] as AddressBook[], + contacts: [ + card('c1', { uid: '1' }), + card('acc-team:t1', { uid: '2', isShared: true, accountId: 'acc-team' }), + ], + }; + const incoming = [{ uid: '1' }, { uid: '2' }, { uid: '3' }, {}]; + expect([...selectImportUidMatches(state, incoming, 'ab-1')].map(([i, c]) => [i, c.id])).toEqual([[0, 'c1']]); + expect([...selectImportUidMatches(state, incoming, 'acc-team:ab-9')].map(([i, c]) => [i, c.id])) + .toEqual([[1, 'acc-team:t1']]); + }); }); describe('address books', () => { diff --git a/src/stores/contacts-store.ts b/src/stores/contacts-store.ts index 0afeef89..23d086d7 100644 --- a/src/stores/contacts-store.ts +++ b/src/stores/contacts-store.ts @@ -117,12 +117,15 @@ export interface ContactsState { deleteContact: (id: string) => Promise; bulkDelete: (ids: string[]) => Promise; /** - * Creates the cards in the book; one whose UID the account already has is - * updated in place instead (a re-import of an export). + * Creates the cards in the book. One whose UID the account already has is + * updated in place instead (a re-import of an export), but only when its + * UID is in `updateUids`: the user chose to replace that card. Any other + * card whose UID is taken is not written and counts as failed. */ importContacts: ( contacts: Partial[], addressBookId: string, + updateUids?: ReadonlySet, ) => Promise; addContactKeyword: (id: string, keyword: string) => Promise; @@ -194,6 +197,37 @@ export function existingUidCardId(err: unknown): string | null { return m ? m[1] : null; } +/** The account's cards by UID; `accountId` undefined is the own account. */ +function cardsByUid(contacts: ContactCard[], accountId: string | undefined): Map { + const byUid = new Map(); + for (const c of contacts) { + // As the server compares them: "x" and "urn:uuid:x" are two UIDs. + if (c.uid && (c.isShared ? c.accountId : undefined) === accountId) byUid.set(c.uid, c); + } + return byUid; +} + +/** + * The cards an import into `addressBookId` would replace: its account holds + * one with the incoming card's UID, in whatever book. That may be another + * contact altogether (an older export, another app's "1", "2" UIDs), so the + * import sheet asks first. Keyed by the incoming card's index. + */ +export function selectImportUidMatches( + state: Pick, + incoming: Partial[], + addressBookId: string | null, +): Map { + const book = state.addressBooks.find((b) => b.id === addressBookId); + const byUid = cardsByUid(state.contacts, book?.isShared ? book.accountId : undefined); + const matches = new Map(); + incoming.forEach((card, idx) => { + const known = card.uid ? byUid.get(card.uid) : undefined; + if (known) matches.set(idx, known); + }); + return matches; +} + function persistCategory(category: ContactCategory): void { void AsyncStorage.setItem(SELECTED_CATEGORY_STORAGE_KEY, JSON.stringify(category)).catch( (err) => console.warn('[contacts-store] persist category failed', err), @@ -603,7 +637,7 @@ export const useContactsStore = create()( if (removed.size > 0) requestDeviceSync(CONTACTS_AUTHORITY); }, - importContacts: async (incoming, addressBookId) => { + importContacts: async (incoming, addressBookId, updateUids) => { let imported = 0; let updated = 0; let refetch = false; @@ -612,12 +646,10 @@ export const useContactsStore = create()( const { check } = generationGuard(); // Stalwart keeps one card per UID in an account and refuses a second // ("Contact with UID ... already exists"), so re-importing an export - // failed card by card. A card the account already has is updated. - const byUid = new Map(); - for (const c of get().contacts) { - // As the server compares them: "x" and "urn:uuid:x" are two UIDs. - if (c.uid && (c.isShared ? c.accountId : undefined) === accountId) byUid.set(c.uid, c); - } + // failed card by card. A card the account already has is updated, + // when the user chose to: the same UID may be another contact. + const byUid = cardsByUid(get().contacts, accountId); + const mayUpdate = (uid: string | undefined) => !!uid && !!updateUids?.has(uid); const updateExisting = async (serverId: string, data: Partial) => { // The card stays in its own book and keeps its UID. const { uid: _uid, ...patch } = data; @@ -637,6 +669,7 @@ export const useContactsStore = create()( const known = data.uid ? byUid.get(data.uid) : undefined; try { if (known) { + if (!mayUpdate(data.uid)) throw new Error(`A contact with UID ${data.uid} already exists`); await updateExisting(known.originalId || known.id, data); updated++; continue; @@ -648,7 +681,7 @@ export const useContactsStore = create()( // A card with this UID the list does not hold (not loaded yet, // or in another book): the server names it. const existingId = existingUidCardId(err); - if (!existingId) throw err; + if (!existingId || !mayUpdate(data.uid)) throw err; await updateExisting(existingId, data); updated++; continue;