fix: keep small contact photos on the device and on the server #34

ContactsProvider keeps a photo within its 96 px thumbnail as the
thumbnail alone, without a PHOTO_FILE_ID, and the thumbnail does not
read back. Such a Photo row looked empty to the planner. Every
download of the card inserted the photo again (two, then three
rows), and any edit of the contact on the device deleted the photo on
the server.

A Photo row now always counts as the contact's photo. Photo rows that
earlier downloads added again are deleted by the contact's next clean
write, one row is kept, and nothing reaches the server for them. A
photo set on the device is still uploaded.

Checked with planner tests and end to end through the engine, with a
provider that stores small photos as thumbnails as the device does.

src/device-sync/__tests__/contacts/harness.ts

@@ -4,7 +4,7 @@
  * way the engine applies them (one op group per batch); uploads go through
  * the fake server's patch handling and come back through its normalisation.
  */
-import { GroupMembership, Groups, RawContacts } from '../../android-columns';
+import { Data, GroupMembership, Groups, RawContacts } from '../../android-columns';
 import { makeKeyMinter, parseObjectRef, uuidFrom } from '../../common/ids';
 import type { GroupRights } from '../../contacts/members';
 import { contactsPlanner } from '../../contacts/planner';
@@ -17,7 +17,7 @@ import type {
   UploadAction,
   UploadPlan,
 } from '../../planner';
-import { CONTACTS_AUTHORITY, type BatchResult, type ProviderPort, type Row } from '../../types';
+import { CONTACTS_AUTHORITY, type BatchResult, type OpResult, type ProviderOp, type ProviderPort, type Row } from '../../types';
 import { FakeJmapServer } from '../fakes/fake-jmap-server';
 import { FakeDeviceProviders } from '../fakes/fake-provider';
 
@@ -35,6 +35,40 @@ function rowsOf(res: { columns: string[]; rows: unknown[][] }): Row[] {
   return res.rows.map((cells) => Object.fromEntries(res.columns.map((c, i) => [c, cells[i]])) as Row);
 }
 
+/**
+ * ContactsProvider keeps a photo no larger than its 96 px thumbnail as the
+ * thumbnail only: PHOTO_FILE_ID (DATA14) stays NULL, and the thumbnail
+ * (DATA15) does not read back. The fake stores every photo with a display
+ * photo; this makes the photo rows a batch wrote thumbnail-only, as a device
+ * does with the test fixtures' 1×1 JPEGs.
+ */
+export function keepThumbnailsOnly(device: FakeDeviceProviders, ops: ProviderOp[], results: OpResult[]): void {
+  ops.forEach((op, i) => {
+    if ((op.op !== 'insert' && op.op !== 'update') || op.table !== 'data') return;
+    const bytes = op.values[Data.DATA15];
+    if (!bytes || typeof bytes !== 'object') return;
+    const id = op.op === 'insert' ? results[i]?.id : op.id;
+    const row = id === undefined ? undefined : device.table('data').get(id);
+    if (row) row[Data.DATA14] = null;
+  });
+}
+
+/** A provider port whose photo writes come out thumbnail-only (see `keepThumbnailsOnly`). */
+export function thumbnailOnlyPort(device: FakeDeviceProviders, port: ProviderPort): ProviderPort {
+  return {
+    accountName: port.accountName,
+    authority: port.authority,
+    query: (q) => port.query(q),
+    readSyncState: () => port.readSyncState(),
+    readPhoto: (id, px) => port.readPhoto(id, px),
+    applyBatch: async (ops) => {
+      const res = await port.applyBatch(ops);
+      if (res.ok) keepThumbnailsOnly(device, ops, res.results);
+      return res;
+    },
+  };
+}
+
 export class Harness {
   readonly device = new FakeDeviceProviders();
   readonly port: ProviderPort = this.device.port(ACCOUNT, CONTACTS_AUTHORITY);
@@ -49,6 +83,8 @@ export class Harness {
   readonly unselected = new Set<string>();
   /** When false the context has no `groupsOf` (memberships not synced). */
   groupIndex = true;
+  /** When true the photos the planner writes come out thumbnail-only (see `keepThumbnailsOnly`). */
+  thumbnailPhotos = false;
   readonly ctx: ContactsContext & GroupRights;
 
   constructor() {
@@ -98,7 +134,9 @@ export class Harness {
 
   async apply(group: OpGroup): Promise<BatchResult> {
     if (!group.ops.length) return { ok: true, results: [] };
-    return this.port.applyBatch(group.ops);
+    const res = await this.port.applyBatch(group.ops);
+    if (res.ok && this.thumbnailPhotos) keepThumbnailsOnly(this.device, group.ops, res.results);
+    return res;
   }
 
   async applyOk(group: OpGroup): Promise<void> {

src/device-sync/__tests__/contacts/lifecycle.test.ts

@@ -181,6 +181,67 @@ describe('contacts lifecycle', () => {
     });
   });
 
+  describe('a small photo the provider keeps as its thumbnail only (no PHOTO_FILE_ID)', () => {
+    const photo = { ph: { kind: 'photo', uri: `data:image/jpeg;base64,${JPEG}`, mediaType: 'image/jpeg' } };
+    const small = () => ({ name: { full: 'Photo Smalltest' }, emails: { e1: { address: 'small@example.org' } }, notes: { n1: { note: 'v1' } }, media: photo });
+    const photos = (h: Harness, rawId: number) => h.device.rows('data', 'raw_contact_id = ? AND mimetype = ?', [rawId, MimeType.PHOTO]);
+
+    it('keeps one row through downloads and merges, and never reads it as deleted', async () => {
+      const h = new Harness();
+      h.thumbnailPhotos = true;
+      const { id, rawId } = await h.seed(small());
+      expect(photos(h, rawId)).toMatchObject([{ data14: null, data_sync1: 'media:ph' }]);
+      await expectSettled(h, id, rawId);
+      for (const note of ['v2', 'v3']) {
+        h.server.serverUpdate('ContactCard', JMAP, id, { 'notes/n1/note': note });
+        await h.download(id);
+        expect(photos(h, rawId)).toHaveLength(1);
+      }
+      // A merge into an edited contact.
+      h.device.user.updateData(h.rowsByKey(rawId)['emails:e1']._id as number, { data1: 'small2@example.org' });
+      h.server.serverUpdate('ContactCard', JMAP, id, { 'notes/n1/note': 'v4' });
+      await h.applyOk(h.planner.planDownload(h.card(id), await h.contact(rawId), h.ctx).ops);
+      expect(photos(h, rawId)).toHaveLength(1);
+      // An edit of another field uploads alone.
+      expect(h.planner.planUpload(await h.contact(rawId), h.ctx)).toEqual({
+        kind: 'upload', actions: [{ kind: 'update', id, patch: { 'emails/e1/address': 'small2@example.org' } }],
+      });
+      await h.upload(rawId);
+      expect(h.card(id).media).toEqual(photo);
+      expect(photos(h, rawId)).toHaveLength(1);
+      await expectSettled(h, id, rawId);
+      // A photo set on the device (it gets a display photo) replaces it.
+      h.device.user.setPhoto(rawId, JPEG2);
+      h.devicePhotos.set(rawId, JPEG2);
+      expect(h.planner.planUpload(await h.contact(rawId), h.ctx)).toEqual({
+        kind: 'upload', actions: [{ kind: 'update', id, patch: { 'media/ph/uri': `data:image/jpeg;base64,${JPEG2}`, 'media/ph/mediaType': 'image/jpeg' } }],
+      });
+    });
+
+    it('heals the photo rows the same photo was inserted as again, keeping one and the server photo', async () => {
+      const h = new Harness();
+      h.thumbnailPhotos = true;
+      const { id, rawId } = await h.seed(small());
+      const [row] = photos(h, rawId);
+      const again = { [Data.MIMETYPE]: MimeType.PHOTO, [Data.DATA15]: { b64: JPEG }, data_sync1: row.data_sync1, data_sync2: row.data_sync2, data_sync3: row.data_sync3 };
+      for (let i = 0; i < 2; i++) await h.applyOk({ ref: 'again', ops: [{ op: 'insert', table: 'data', values: { ...again, [Data.RAW_CONTACT_ID]: rawId } }] });
+      expect(photos(h, rawId)).toHaveLength(3);
+      // An edit on the device uploads without touching the photo, and its accepted write keeps one row.
+      h.device.user.updateData(h.rowsByKey(rawId)['notes:n1']._id as number, { data1: 'v2 edited on device' });
+      expect(h.planner.planUpload(await h.contact(rawId), h.ctx)).toEqual({
+        kind: 'upload', actions: [{ kind: 'update', id, patch: { 'notes/n1/note': 'v2 edited on device' } }],
+      });
+      await h.upload(rawId);
+      expect(photos(h, rawId)).toHaveLength(1);
+      expect(h.card(id).media).toEqual(photo);
+      await expectSettled(h, id, rawId);
+      // An unchanged card downloaded again (a full reconcile) heals too.
+      for (let i = 0; i < 2; i++) await h.applyOk({ ref: 'again', ops: [{ op: 'insert', table: 'data', values: { ...again, [Data.RAW_CONTACT_ID]: rawId } }] });
+      await h.applyOk(h.planner.planDownload(h.card(id), await h.contact(rawId), h.ctx).ops);
+      expect(photos(h, rawId)).toHaveLength(1);
+    });
+  });
+
   it('heals a baseline the provider stored differently from the prediction', async () => {
     const h = new Harness();
     const { id, rawId } = await h.seed({ name: { full: 'Dr. Jane Q. Public' } });

src/device-sync/__tests__/contacts/real-engine.test.ts

@@ -3,11 +3,13 @@
 // themselves are tested in the other files of this folder.
 
 import { describe, expect, it } from 'vitest';
-import { Groups, MimeType } from '../../android-columns';
+import { Data, Groups, MimeType } from '../../android-columns';
 import { calendarPlanner } from '../../calendar/planner';
 import { contactsPlanner } from '../../contacts/planner';
 import { addServerCards, CONTACTS_AUTHORITY, createHarness, rowWrites, type Harness } from '../engine/harness';
 import type { SetTarget } from '../fakes/fake-jmap-server';
+import { JPEG } from './fixtures';
+import { thumbnailOnlyPort } from './harness';
 
 function real(options: Parameters<typeof createHarness>[0] = {}): Harness {
   const h = createHarness(options);
@@ -78,6 +80,28 @@ describe('contacts through the engine', () => {
     expect(await h.teardown(CONTACTS_AUTHORITY)).toEqual({ pending: 0 });
   });
 
+  it('keeps one row for a small photo the provider stores as its thumbnail, and never deletes it on the server', async () => {
+    const h = real();
+    const base = h.deps.provider;
+    h.deps.provider = (name, authority) => thumbnailOnlyPort(h.device, base(name, authority));
+    const media = { ph: { kind: 'photo', uri: `data:image/jpeg;base64,${JPEG}`, mediaType: 'image/jpeg' } };
+    const card = h.server.addCard('a', { uid: 'u-small', name: { full: 'Photo Smalltest' }, notes: { n1: { note: 'v1' } }, addressBookIds: { [h.book]: true }, media });
+    expect((await h.run()).outcome).toBe('ok');
+    const id = h.contactNamed('Photo Smalltest')!.id;
+    const rowsOf = (mimetype: string) => h.device.rows('data').filter((d) => Number(d.raw_contact_id) === id && d.mimetype === mimetype);
+    expect(rowsOf(MimeType.PHOTO)).toMatchObject([{ data14: null }]);
+
+    for (const note of ['v2', 'v3']) {
+      h.server.serverUpdate('ContactCard', 'a', card, { 'notes/n1/note': note });
+      expect((await h.run()).outcome).toBe('ok');
+      expect(rowsOf(MimeType.PHOTO)).toHaveLength(1);
+    }
+    h.device.user.updateData(Number(rowsOf(MimeType.NOTE)[0]._id), { [Data.DATA1]: 'v4 edited on device' });
+    expect(await h.run()).toMatchObject({ outcome: 'ok', itemErrors: [], stats: { uploaded: { updated: 1 } } });
+    expect(h.server.get('ContactCard', 'a', card)).toMatchObject({ notes: { n1: { note: 'v4 edited on device' } }, media });
+    expect(rowsOf(MimeType.PHOTO)).toHaveLength(1);
+  });
+
   it('syncs a contact whose photo is too large for a provider batch, without the photo', async () => {
     const h = real();
     // About 900 KB of JPEG as a data: URI (a camera photo set over CardDAV).

src/device-sync/contacts/project.ts

@@ -318,11 +318,16 @@ export function rowMatchesUnit(u: Pick<Unit, 'mimetype' | 'cells'>, stored: Row,
   return spec.columns.every((c) => same(u.cells[c], stored[c], c));
 }
 
-/** Whether a row has nothing in it (an editor's blank nickname or note). */
+/**
+ * Whether a row has nothing in it (an editor's blank nickname or note). A
+ * photo row always holds a photo: ContactsProvider keeps one within its 96 px
+ * thumbnail as the thumbnail alone, without PHOTO_FILE_ID, and the thumbnail
+ * (DATA15, a blob) does not read back.
+ */
 export function isEmptyRow(mimetype: string, cells: Row): boolean {
   const spec = SPECS[mimetype];
   if (!spec) return true;
-  if (mimetype === MimeType.PHOTO) return cellText(cells[Data.DATA14]) === null;
+  if (mimetype === MimeType.PHOTO) return false;
   if (mimetype === MimeType.GROUP_MEMBERSHIP) return cellText(cells[GroupMembership.GROUP_SOURCE_ID]) === null && cellText(cells[Data.DATA1]) === null;
   return spec.valueColumns.every((c) => text(cells[c]) === null);
 }