From 40b7c2c15e388af6fcb5aee9eea1d7b52219c4a8 Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Sun, 17 May 2026 01:56:56 +0800 Subject: [PATCH] refactor(reader): harden saveConfig updatedAt refresh (#4189) saveConfig refreshed config.updatedAt by mutating the config object in place. That only worked because every reader view shares one config object reference, and it bypassed Zustand change-detection entirely. Refresh updatedAt via an immutable setConfig store update instead, so it no longer depends on callers sharing the same reference, notifies subscribers, and never mutates the caller-provided object. Sync behavior is unchanged. Refs #4184 Co-authored-by: Claude Opus 4.7 (1M context) --- .../__tests__/store/book-data-store.test.ts | 68 +++++++++++++++++++ apps/readest-app/src/store/bookDataStore.ts | 13 ++-- 2 files changed, 77 insertions(+), 4 deletions(-) diff --git a/apps/readest-app/src/__tests__/store/book-data-store.test.ts b/apps/readest-app/src/__tests__/store/book-data-store.test.ts index 0a337366..b81fd2e1 100644 --- a/apps/readest-app/src/__tests__/store/book-data-store.test.ts +++ b/apps/readest-app/src/__tests__/store/book-data-store.test.ts @@ -400,5 +400,73 @@ describe('bookDataStore', () => { expect(saveBookConfig).not.toHaveBeenCalled(); expect(saveLibraryBooks).not.toHaveBeenCalled(); }); + + test('refreshes config.updatedAt in the store with a fresh reference', async () => { + const saveBookConfig = vi.fn().mockResolvedValue(undefined); + const saveLibraryBooks = vi.fn().mockResolvedValue(undefined); + const envConfig = makeEnvConfig({ saveBookConfig, saveLibraryBooks }); + + useLibraryStore.getState().setLibrary([makeLibraryBook({ hash: 'h1' })]); + + const data = makeBookData('h1', { progress: [42, 100] }); + useBookDataStore.setState({ booksData: { h1: data } }); + const before = useBookDataStore.getState().getConfig('h1'); + + await useBookDataStore.getState().saveConfig(envConfig, 'h1', data.config!, FAKE_SETTINGS); + + const after = useBookDataStore.getState().getConfig('h1'); + // New reference so Zustand change-detection fires for selector subscribers. + expect(after).not.toBe(before); + expect(after!.updatedAt).toBeGreaterThan(1000); + }); + + test('refreshes the store config even when the caller passes a separate object', async () => { + const saveBookConfig = vi.fn().mockResolvedValue(undefined); + const saveLibraryBooks = vi.fn().mockResolvedValue(undefined); + const envConfig = makeEnvConfig({ saveBookConfig, saveLibraryBooks }); + + useLibraryStore.getState().setLibrary([makeLibraryBook({ hash: 'h1' })]); + + const data = makeBookData('h1', { progress: [42, 100] }); + useBookDataStore.setState({ booksData: { h1: data } }); + + // Caller passes a freshly-built config, not the shared store object. + const detachedConfig: BookConfig = { ...data.config! }; + await useBookDataStore.getState().saveConfig(envConfig, 'h1', detachedConfig, FAKE_SETTINGS); + + expect(useBookDataStore.getState().getConfig('h1')!.updatedAt).toBeGreaterThan(1000); + }); + + test('does not mutate the caller-provided config object', async () => { + const saveBookConfig = vi.fn().mockResolvedValue(undefined); + const saveLibraryBooks = vi.fn().mockResolvedValue(undefined); + const envConfig = makeEnvConfig({ saveBookConfig, saveLibraryBooks }); + + useLibraryStore.getState().setLibrary([makeLibraryBook({ hash: 'h1' })]); + + const data = makeBookData('h1', { progress: [42, 100] }); + useBookDataStore.setState({ booksData: { h1: data } }); + + const detachedConfig: BookConfig = { ...data.config!, updatedAt: 1000 }; + await useBookDataStore.getState().saveConfig(envConfig, 'h1', detachedConfig, FAKE_SETTINGS); + + expect(detachedConfig.updatedAt).toBe(1000); + }); + + test('persists a config carrying the refreshed updatedAt', async () => { + const saveBookConfig = vi.fn().mockResolvedValue(undefined); + const saveLibraryBooks = vi.fn().mockResolvedValue(undefined); + const envConfig = makeEnvConfig({ saveBookConfig, saveLibraryBooks }); + + useLibraryStore.getState().setLibrary([makeLibraryBook({ hash: 'h1' })]); + + const data = makeBookData('h1', { progress: [42, 100] }); + useBookDataStore.setState({ booksData: { h1: data } }); + + await useBookDataStore.getState().saveConfig(envConfig, 'h1', data.config!, FAKE_SETTINGS); + + const persistedConfig = saveBookConfig.mock.calls[0]![1] as BookConfig; + expect(persistedConfig.updatedAt).toBeGreaterThan(1000); + }); }); }); diff --git a/apps/readest-app/src/store/bookDataStore.ts b/apps/readest-app/src/store/bookDataStore.ts index 06709146..994d38b7 100644 --- a/apps/readest-app/src/store/bookDataStore.ts +++ b/apps/readest-app/src/store/bookDataStore.ts @@ -86,18 +86,23 @@ export const useBookDataStore = create((set, get) => ({ // progress and timestamps. We do NOT mutate the existing book object or // the existing library array — Zustand subscribers see fresh references // and the visibleLibrary cache stays in sync via setLibrary's full update. + const now = Date.now(); const original = library[idx]!; const updatedBook: Book = { ...original, progress: config.progress, - updatedAt: Date.now(), - downloadedAt: original.downloadedAt || Date.now(), + updatedAt: now, + downloadedAt: original.downloadedAt || now, }; const newLibrary = [updatedBook, ...library.slice(0, idx), ...library.slice(idx + 1)]; setLibrary(newLibrary); - config.updatedAt = Date.now(); - await appService.saveBookConfig(updatedBook, config, settings); + // Refresh updatedAt immutably via the store rather than mutating the + // caller-provided object. This notifies Zustand subscribers and works + // regardless of whether the caller passed the shared store config. + get().setConfig(bookKey, { updatedAt: now }); + const configToSave = { ...config, updatedAt: now }; + await appService.saveBookConfig(updatedBook, configToSave, settings); await appService.saveLibraryBooks(useLibraryStore.getState().library); }, updateBooknotes: (key: string, booknotes: BookNote[]) => {