From 2963e75bdd23272716f1f9b0a959756b02480c51 Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Mon, 6 Jul 2026 01:27:17 +0900 Subject: [PATCH] fix(sync): propagate group membership for already-synced books (#4946) Group membership synced only for newly-imported books. Re-grouping a book already present on both devices bumped book.updatedAt and won the library-index LWW race, but mergeBookMetadata dropped groupId/groupName from the overlay, so the change never reached peers. New books instead arrive via addBookToLibrary with the full remote object, which is why their group did travel. Carry groupId/groupName in mergeBookMetadata, matching native cloud sync (transform.ts maps group_id/group_name). Values are assigned raw so a group removal also propagates. Fixes #4942 Co-authored-by: Claude Opus 4.8 (1M context) --- .../sync/file/engine-metadata-sync.test.ts | 27 +++++++++++++++++ .../services/sync/file/merge.test.ts | 30 +++++++++++++++++++ .../src/services/sync/file/merge.ts | 23 +++++++++----- 3 files changed, 73 insertions(+), 7 deletions(-) diff --git a/apps/readest-app/src/__tests__/services/sync/file/engine-metadata-sync.test.ts b/apps/readest-app/src/__tests__/services/sync/file/engine-metadata-sync.test.ts index 1b2cc7f50..078bb8f18 100644 --- a/apps/readest-app/src/__tests__/services/sync/file/engine-metadata-sync.test.ts +++ b/apps/readest-app/src/__tests__/services/sync/file/engine-metadata-sync.test.ts @@ -130,6 +130,33 @@ describe('FileSyncEngine metadata reconciliation (#4756)', () => { expect(indexedBook.title).toBe('New Title'); }); + test('pulls newer remote group membership for a book the device already has (#4942)', async () => { + const local = makeLocalBook({ updatedAt: 100 }); + const remote = makeLocalBook({ groupId: 'g1', groupName: 'Sci-Fi', updatedAt: 200 }); + const capture: { index?: RemoteLibraryIndex | null } = {}; + const provider = makeProvider(makeRemoteIndex(remote, 200), null, capture); + + const updateBookMetadata = vi.fn(async (_book: Book) => {}); + const store = makeStore({ updateBookMetadata }); + + const engine = new FileSyncEngine(provider, store); + const result = await engine.syncLibrary([local], { + strategy: 'silent', + syncBooks: false, + deviceId: 'pc-device', + }); + + expect(updateBookMetadata).toHaveBeenCalledTimes(1); + const merged = updateBookMetadata.mock.calls[0]![0]; + expect(merged.groupId).toBe('g1'); + expect(merged.groupName).toBe('Sci-Fi'); + expect(result.metadataUpdated).toBe(1); + + const indexedBook = capture.index!.books.find((b) => b.hash === 'h1')!; + expect(indexedBook.groupId).toBe('g1'); + expect(indexedBook.groupName).toBe('Sci-Fi'); + }); + test('does not overwrite local metadata when the local copy is newer', async () => { const local = makeLocalBook({ title: 'Local Newer', updatedAt: 300 }); const remote = makeLocalBook({ title: 'Remote Older', updatedAt: 200 }); diff --git a/apps/readest-app/src/__tests__/services/sync/file/merge.test.ts b/apps/readest-app/src/__tests__/services/sync/file/merge.test.ts index fde4fd128..32dcfac8c 100644 --- a/apps/readest-app/src/__tests__/services/sync/file/merge.test.ts +++ b/apps/readest-app/src/__tests__/services/sync/file/merge.test.ts @@ -129,6 +129,36 @@ describe('mergeBookMetadata (LWW field subset)', () => { expect(m.progress).toEqual([1, 2]); expect(m.updatedAt).toBe(9); }); + + test('carries remote group membership (add-to-group) when remote is newer (#4942)', () => { + const local = { hash: 'h', title: 'T', author: 'A', updatedAt: 1 } as Book; + const remote = { + hash: 'h', + title: 'T', + author: 'A', + groupId: 'g1', + groupName: 'Sci-Fi', + updatedAt: 9, + } as Book; + const m = mergeBookMetadata(local, remote); + expect(m.groupId).toBe('g1'); + expect(m.groupName).toBe('Sci-Fi'); + }); + + test('propagates group removal when remote cleared membership (#4942)', () => { + const local = { + hash: 'h', + title: 'T', + author: 'A', + groupId: 'g1', + groupName: 'Sci-Fi', + updatedAt: 1, + } as Book; + const remote = { hash: 'h', title: 'T', author: 'A', updatedAt: 9 } as Book; + const m = mergeBookMetadata(local, remote); + expect(m.groupId).toBeUndefined(); + expect(m.groupName).toBeUndefined(); + }); }); describe('isRemoteBookMetadataNewer', () => { diff --git a/apps/readest-app/src/services/sync/file/merge.ts b/apps/readest-app/src/services/sync/file/merge.ts index 7a88cfe2c..23d0a6aac 100644 --- a/apps/readest-app/src/services/sync/file/merge.ts +++ b/apps/readest-app/src/services/sync/file/merge.ts @@ -87,14 +87,21 @@ export const mergeBookConfig = ( * Overlay the user-facing metadata of `remote` onto `local`, preserving every * device-local / file-system field: `filePath`, `sourceTitle` (which names the * on-disk file), `coverImageUrl` (a device-local blob URL the caller - * regenerates), reading progress, reading status, group membership, `hash`, - * `format`, `createdAt`, etc. + * regenerates), reading progress, reading status, `hash`, `format`, + * `createdAt`, etc. * - * Only the fields a metadata edit actually changes travel — this list mirrors - * `getBookWithUpdatedMetadata` in `utils/book.ts`, which is the local side of - * the same operation. The cover image is replicated separately as cover.png - * bytes (see the reconciliation pass in the engine), so it is intentionally - * absent here. + * Group membership (`groupId` / `groupName`) is user-facing metadata and travels + * too, matching the native cloud sync (`transform.ts` maps `group_id` / + * `group_name`). Without this, re-grouping a book already present on both + * devices bumps `updatedAt` and wins LWW, yet the group change is dropped by the + * overlay — so it never propagated for already-synced books (#4942). Assigning + * the raw remote values (not `?? local`) lets a group removal (undefined) clear + * membership on peers too. + * + * The scalar fields a metadata edit changes mirror `getBookWithUpdatedMetadata` + * in `utils/book.ts`, which is the local side of the same operation. The cover + * image is replicated separately as cover.png bytes (see the reconciliation pass + * in the engine), so it is intentionally absent here. */ export const mergeBookMetadata = (local: Book, remote: Book): Book => ({ ...local, @@ -102,6 +109,8 @@ export const mergeBookMetadata = (local: Book, remote: Book): Book => ({ author: remote.author, metadata: remote.metadata ?? local.metadata, primaryLanguage: remote.primaryLanguage ?? local.primaryLanguage, + groupId: remote.groupId, + groupName: remote.groupName, updatedAt: remote.updatedAt, });