fix(library): refresh book cover after editing metadata (#4572)
* fix(library): refresh book cover after editing metadata Editing a book's cover in Book Details and saving showed the old cover until a full reload, in two render paths: - Library grid: handleUpdateMetadata mutated the book object in place, so the memoized <BookCover> compared fields off the same (mutated) reference and skipped re-rendering. Build a new book object via the new getBookWithUpdatedMetadata helper instead of mutating. - Book Details view: BookDetailView renders cover/title/author from the modal's `book` prop, which the parent never re-passed after save. BookDetailModal now tracks the saved book locally (displayBook) and renders the view from it. Adds a unit test for the immutable helper, a BookDetailModal regression test (edit cover -> save -> view reflects it), and a sample-alice.txt fixture for TXT import testing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(agent): add cover-refresh stale-render memory Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Huang Xin
@@ -80,6 +80,7 @@
|
||||
## Library Fixes
|
||||
- [Tauri menu append race (#4389)](tauri-menu-append-race-4389.md) — un-awaited `Menu.append()` (async IPC) in `BookshelfItem.tsx` → context-menu items shuffle order every open (native only, invisible in jsdom); fix = single `await Menu.new({ items })` of ordered `MenuItemOptions`; order/inclusion extracted to pure `getBookContextMenuItemIds` for unit testing
|
||||
- [TXT author recognition (#4390)](txt-author-recognition-4390.md) — 【】-titled Chinese web-novels show author missing/garbage; they're TXT→EPUB (title==full filename is the tell, check `txt.ts` not foliate-js); `extractTxtFilenameMetadata` only handled 《》 + greedy header capture grabbed metadata blobs; fix = `parseLabeledAuthor` for any filename + `isPlausibleAuthorName` guard
|
||||
- [Cover stale until refresh (in-place mutation vs React.memo)](cover-stale-inplace-mutation-memo.md) — editing a book cover in details + Save left the library cover stale until reload; `handleUpdateMetadata` mutated `book` IN PLACE so memoized `<BookCover>`'s prev snapshot pointed at the same object → comparator saw no change → skip; fix = pure `getBookWithUpdatedMetadata` returns a NEW book object. Cloning in `updateBook` wouldn't help (original already mutated). Verified live on emulator via CDP fiber-store extraction (A: mutate→stale, B: new obj→updates)
|
||||
|
||||
## Library Architecture
|
||||
- [Book action platform surfaces](book-actions-platform-surfaces.md) — library context menu is **Tauri-desktop-only** (`hasContextMenu` false on web + iOS/Android); cross-platform book actions go in `BookDetailView`'s icon row. #4543 Goodreads search added both surfaces + a built-in web-search provider for highlighted-text lookup
|
||||
|
||||
@@ -0,0 +1,20 @@
|
||||
---
|
||||
name: cover-stale-inplace-mutation-memo
|
||||
description: Library cover (or any memoized child) not updating until refresh — in-place object mutation defeats React.memo
|
||||
metadata:
|
||||
node_type: memory
|
||||
type: project
|
||||
originSessionId: ec78c172-79e7-448c-8671-780dcc115613
|
||||
---
|
||||
|
||||
Symptom: edit a book's cover in Book Details → Save → return to library, the cover shows the OLD image; a full page refresh fixes it. Title/author DO update.
|
||||
|
||||
Root cause: `handleUpdateMetadata` in `src/app/library/page.tsx` mutated the existing `book` object IN PLACE (`book.metadata = …; book.coverImageUrl = …; book.updatedAt = …`) then passed that same reference to `updateBook`. `<BookCover>` (`src/components/BookCover.tsx`) is `React.memo`'d with a custom comparator reading `coverImageUrl`/`metadata.coverImageUrl`/`updatedAt` off the book. Because the previous-render snapshot (`prevProps.book`) is the *same mutated object*, every field compares equal → memo skips → cover never re-renders. Title updates because `BookItem` is NOT memoized. Refresh works because `loadLibraryBooks` (`libraryService.ts`) strips `coverImageUrl` on save and REGENERATES it from `${hash}/cover.png` on load (the file was overwritten by `updateCoverImage`).
|
||||
|
||||
KEY INSIGHT: cloning inside `updateBook` would NOT fix it — once the original object is mutated, `prevProps` already reads the new values. The fix must leave the object React holds as `prevProps` untouched.
|
||||
|
||||
Fix (PR for fix/txt-open-with-conversion): pure helper `getBookWithUpdatedMetadata(book, metadata)` in `src/utils/book.ts` returns a NEW book object (`{...book, metadata, title, author, primaryLanguage, updatedAt, coverImageUrl}`); `handleUpdateMetadata` uses it instead of mutating. Cover URL is set from `metadata.coverImageBlobUrl || metadata.coverImageUrl` (cached/blob URL is a unique path = the new image; `'_blank'` for remove). Unit test asserts immutability of the input + new reference.
|
||||
|
||||
General rule: when a memoized child reads fields off a store object, NEVER mutate that object in place to "update" it — build a new object. Same trap could bite any `React.memo` field comparator in this codebase.
|
||||
|
||||
On-device CDP verification (reusable): the cover SET path uses the native Tauri file picker (`selectFiles` → `openDialog`), NOT automatable via CDP; the installed emulator app is the released bundled build (`http://tauri.localhost/...`), not the dev server. So I verified the MECHANISM directly in the live WebView: extracted the zustand library store from the React fiber tree (the library page calls `useLibraryStore()` with NO selector, so its fiber hook `memoizedState` holds the full state incl. `library`/`setLibrary`/`updateBook`), injected an Alice book, then A) mutated it in place + `setLibrary([sameRef])` → rendered `<img src>` stayed stale (bug), B) `setLibrary([{...book,coverImageUrl:NEW}])` → `<img src>` updated immediately (fix). Restore with `Page.reload` (injected book was in-memory only). See [[cdp-android-webview-profiling]], [[android-cdp-e2e-lane]].
|
||||
@@ -0,0 +1,120 @@
|
||||
import { describe, it, expect, vi, afterEach } from 'vitest';
|
||||
import { render, cleanup, fireEvent, waitFor, screen } from '@testing-library/react';
|
||||
|
||||
import { Book } from '@/types/book';
|
||||
import BookDetailModal from '@/components/metadata/BookDetailModal';
|
||||
|
||||
vi.mock('@/hooks/useTranslation', () => ({
|
||||
useTranslation: () => (s: string) => s,
|
||||
}));
|
||||
|
||||
const appSvc = {
|
||||
getBookFileSize: vi.fn(async () => 1024),
|
||||
fetchBookDetails: vi.fn(async () => null),
|
||||
};
|
||||
|
||||
vi.mock('@/context/EnvContext', () => ({
|
||||
useEnv: () => ({ envConfig: { getAppService: async () => appSvc }, appService: appSvc }),
|
||||
}));
|
||||
|
||||
vi.mock('@/store/themeStore', () => ({
|
||||
useThemeStore: () => ({ safeAreaInsets: { top: 0, bottom: 0, left: 0, right: 0 } }),
|
||||
}));
|
||||
|
||||
vi.mock('@/store/settingsStore', () => ({
|
||||
useSettingsStore: () => ({
|
||||
settings: {
|
||||
metadataSeriesCollapsed: true,
|
||||
metadataOthersCollapsed: true,
|
||||
metadataDescriptionCollapsed: true,
|
||||
},
|
||||
}),
|
||||
}));
|
||||
|
||||
vi.mock('@/hooks/useFileSelector', () => ({
|
||||
useFileSelector: () => ({ selectFiles: vi.fn() }),
|
||||
}));
|
||||
|
||||
vi.mock('@/hooks/useResponsiveSize', () => ({
|
||||
useResponsiveSize: (n: number) => n,
|
||||
useDefaultIconSize: () => 20,
|
||||
}));
|
||||
|
||||
vi.mock('@/helpers/settings', () => ({ saveSysSettings: vi.fn() }));
|
||||
|
||||
vi.mock('@/services/environment', () => ({ isWebAppPlatform: () => false }));
|
||||
|
||||
vi.mock('@/libs/metadata', () => ({ searchMetadata: vi.fn(async () => []) }));
|
||||
|
||||
// Render the cover with the same src-resolution as the real <BookCover> so we
|
||||
// can assert which cover the view actually shows.
|
||||
vi.mock('@/components/BookCover', () => ({
|
||||
__esModule: true,
|
||||
default: ({ book }: { book: Book }) => (
|
||||
// biome-ignore lint/a11y/useAltText: test mock
|
||||
<img data-testid='cover' src={book.metadata?.coverImageUrl || book.coverImageUrl || ''} />
|
||||
),
|
||||
}));
|
||||
|
||||
vi.mock('next/image', () => ({
|
||||
__esModule: true,
|
||||
// biome-ignore lint/a11y/useAltText: test mock
|
||||
default: (props: Record<string, unknown>) => <img {...props} />,
|
||||
}));
|
||||
|
||||
// Chrome we don't exercise — keep imports cheap and side-effect free.
|
||||
vi.mock('@/components/Dialog', () => ({
|
||||
__esModule: true,
|
||||
default: ({ children, isOpen }: { children: React.ReactNode; isOpen: boolean }) =>
|
||||
isOpen ? <div>{children}</div> : null,
|
||||
}));
|
||||
vi.mock('@/components/Alert', () => ({ __esModule: true, default: () => null }));
|
||||
vi.mock('@/components/metadata/SourceSelector', () => ({ __esModule: true, default: () => null }));
|
||||
vi.mock('@/components/Spinner', () => ({ __esModule: true, default: () => null }));
|
||||
|
||||
afterEach(() => cleanup());
|
||||
|
||||
const makeBook = (): Book =>
|
||||
({
|
||||
hash: 'abc123',
|
||||
title: 'Old Title',
|
||||
author: 'Old Author',
|
||||
format: 'EPUB',
|
||||
coverImageUrl: 'old-cover',
|
||||
primaryLanguage: 'en',
|
||||
createdAt: Date.now(),
|
||||
updatedAt: Date.now(),
|
||||
metadata: {
|
||||
title: 'Old Title',
|
||||
author: 'Old Author',
|
||||
language: 'en',
|
||||
coverImageUrl: 'old-cover',
|
||||
},
|
||||
}) as Book;
|
||||
|
||||
describe('BookDetailModal cover refresh after save', () => {
|
||||
it('updates the Book Details view cover when the cover is edited and saved', async () => {
|
||||
const book = makeBook();
|
||||
const onUpdate = vi.fn();
|
||||
render(
|
||||
<BookDetailModal book={book} isOpen onClose={vi.fn()} handleBookMetadataUpdate={onUpdate} />,
|
||||
);
|
||||
|
||||
// The view initially shows the original cover.
|
||||
await waitFor(() => expect(screen.getByTestId('cover').getAttribute('src')).toBe('old-cover'));
|
||||
|
||||
// Edit → remove the cover → save.
|
||||
fireEvent.click(screen.getByTitle('Edit Metadata'));
|
||||
fireEvent.click(await screen.findByTitle('Remove cover image'));
|
||||
fireEvent.click(screen.getByText('Save'));
|
||||
|
||||
expect(onUpdate).toHaveBeenCalledTimes(1);
|
||||
|
||||
// The details view must reflect the saved cover change without needing the
|
||||
// modal to be reopened (the bug: it kept showing the stale `book` prop).
|
||||
await waitFor(() => {
|
||||
expect(screen.getByTestId('cover').getAttribute('src')).not.toBe('old-cover');
|
||||
});
|
||||
expect(screen.getByTestId('cover').getAttribute('src')).toBe('_blank');
|
||||
});
|
||||
});
|
||||
File diff suppressed because it is too large
Load Diff
@@ -0,0 +1,95 @@
|
||||
import { describe, expect, it } from 'vitest';
|
||||
|
||||
import { getBookWithUpdatedMetadata } from '@/utils/book';
|
||||
import { Book } from '@/types/book';
|
||||
import { BookMetadata } from '@/libs/document';
|
||||
|
||||
const makeBook = (): Book =>
|
||||
({
|
||||
hash: 'abc123',
|
||||
format: 'EPUB',
|
||||
title: 'Old Title',
|
||||
author: 'Old Author',
|
||||
coverImageUrl: 'old-cover-url',
|
||||
updatedAt: 1000,
|
||||
primaryLanguage: 'en',
|
||||
metadata: {
|
||||
title: 'Old Title',
|
||||
author: 'Old Author',
|
||||
language: 'en',
|
||||
coverImageUrl: 'old-cover-url',
|
||||
},
|
||||
}) as Book;
|
||||
|
||||
describe('getBookWithUpdatedMetadata', () => {
|
||||
it('returns a new book object without mutating the input', () => {
|
||||
const book = makeBook();
|
||||
const editedMeta: BookMetadata = {
|
||||
title: 'New Title',
|
||||
author: 'New Author',
|
||||
language: 'fr',
|
||||
coverImageUrl: 'new-cover-url',
|
||||
};
|
||||
|
||||
const updated = getBookWithUpdatedMetadata(book, editedMeta);
|
||||
|
||||
// A fresh reference is required so React.memo'd <BookCover> detects the
|
||||
// change. The original cover-refresh bug mutated `book` in place, so the
|
||||
// memo's previous snapshot pointed to the same object and skipped re-render.
|
||||
expect(updated).not.toBe(book);
|
||||
expect(updated.metadata).not.toBe(book.metadata);
|
||||
|
||||
// Input must be left untouched.
|
||||
expect(book.title).toBe('Old Title');
|
||||
expect(book.author).toBe('Old Author');
|
||||
expect(book.coverImageUrl).toBe('old-cover-url');
|
||||
expect(book.updatedAt).toBe(1000);
|
||||
expect(book.metadata?.coverImageUrl).toBe('old-cover-url');
|
||||
});
|
||||
|
||||
it('applies the edited cover, title, author, language and a fresh updatedAt', () => {
|
||||
const book = makeBook();
|
||||
const editedMeta: BookMetadata = {
|
||||
title: 'New Title',
|
||||
author: 'New Author',
|
||||
language: 'fr',
|
||||
coverImageUrl: 'new-cover-url',
|
||||
};
|
||||
|
||||
const updated = getBookWithUpdatedMetadata(book, editedMeta);
|
||||
|
||||
expect(updated.coverImageUrl).toBe('new-cover-url');
|
||||
expect(updated.title).toBe('New Title');
|
||||
expect(updated.author).toBe('New Author');
|
||||
expect(updated.primaryLanguage).toBe('fr');
|
||||
expect(updated.updatedAt).toBeGreaterThan(book.updatedAt);
|
||||
});
|
||||
|
||||
it('prefers a blob cover URL over the plain cover URL', () => {
|
||||
const book = makeBook();
|
||||
const editedMeta: BookMetadata = {
|
||||
title: 'Old Title',
|
||||
author: 'Old Author',
|
||||
language: 'en',
|
||||
coverImageBlobUrl: 'blob:new-cover',
|
||||
coverImageUrl: 'http-cover-url',
|
||||
};
|
||||
|
||||
const updated = getBookWithUpdatedMetadata(book, editedMeta);
|
||||
|
||||
expect(updated.coverImageUrl).toBe('blob:new-cover');
|
||||
});
|
||||
|
||||
it('keeps the existing cover when the edit does not change it', () => {
|
||||
const book = makeBook();
|
||||
const editedMeta: BookMetadata = {
|
||||
title: 'New Title',
|
||||
author: 'Old Author',
|
||||
language: 'en',
|
||||
};
|
||||
|
||||
const updated = getBookWithUpdatedMetadata(book, editedMeta);
|
||||
|
||||
expect(updated.coverImageUrl).toBe('old-cover-url');
|
||||
});
|
||||
});
|
||||
@@ -10,7 +10,7 @@ import { Book } from '@/types/book';
|
||||
import { AppService, DeleteAction } from '@/types/system';
|
||||
import { buildBookLookupIndex } from '@/services/bookService';
|
||||
import { navigateToLibrary, navigateToLogin, navigateToReader } from '@/utils/nav';
|
||||
import { formatAuthors, formatTitle, getPrimaryLanguage, listFormater } from '@/utils/book';
|
||||
import { getBookWithUpdatedMetadata, listFormater } from '@/utils/book';
|
||||
import { getImportErrorMessage } from '@/services/errors';
|
||||
import { ingestFile } from '@/services/ingestService';
|
||||
import { eventDispatcher } from '@/utils/event';
|
||||
@@ -918,16 +918,15 @@ const LibraryPageContent = ({ searchParams }: { searchParams: ReadonlyURLSearchP
|
||||
};
|
||||
|
||||
const handleUpdateMetadata = async (book: Book, metadata: BookMetadata) => {
|
||||
book.metadata = metadata;
|
||||
book.title = formatTitle(metadata.title);
|
||||
book.author = formatAuthors(metadata.author);
|
||||
book.primaryLanguage = getPrimaryLanguage(metadata.language);
|
||||
book.updatedAt = Date.now();
|
||||
// Build a NEW book object instead of mutating `book` in place. <BookCover>
|
||||
// is memoized and compares fields off the book, so mutating the existing
|
||||
// object (which React holds as the previous snapshot) makes the comparator
|
||||
// see no change and the library cover only refreshes after a full reload.
|
||||
const updatedBook = getBookWithUpdatedMetadata(book, metadata);
|
||||
if (metadata.coverImageBlobUrl || metadata.coverImageUrl || metadata.coverImageFile) {
|
||||
book.coverImageUrl = metadata.coverImageBlobUrl || metadata.coverImageUrl;
|
||||
try {
|
||||
await appService?.updateCoverImage(
|
||||
book,
|
||||
updatedBook,
|
||||
metadata.coverImageBlobUrl || metadata.coverImageUrl,
|
||||
metadata.coverImageFile,
|
||||
);
|
||||
@@ -945,7 +944,7 @@ const LibraryPageContent = ({ searchParams }: { searchParams: ReadonlyURLSearchP
|
||||
}
|
||||
metadata.coverImageBlobUrl = undefined;
|
||||
metadata.coverImageFile = undefined;
|
||||
await updateBook(envConfig, book);
|
||||
await updateBook(envConfig, updatedBook);
|
||||
};
|
||||
|
||||
const handleImportBooksFromFiles = async () => {
|
||||
|
||||
@@ -2,6 +2,7 @@ import clsx from 'clsx';
|
||||
import React, { useEffect, useState } from 'react';
|
||||
|
||||
import { Book } from '@/types/book';
|
||||
import { getBookWithUpdatedMetadata } from '@/utils/book';
|
||||
import { BookMetadata } from '@/libs/document';
|
||||
import { useEnv } from '@/context/EnvContext';
|
||||
import { useThemeStore } from '@/store/themeStore';
|
||||
@@ -54,6 +55,10 @@ const BookDetailModal: React.FC<BookDetailModalProps> = ({
|
||||
const [editMode, setEditMode] = useState(false);
|
||||
const [bookMeta, setBookMeta] = useState<BookMetadata | null>(null);
|
||||
const [fileSize, setFileSize] = useState<number | null>(null);
|
||||
// The parent owns the `book` prop and does not re-pass it after a metadata
|
||||
// save, so the details view tracks the saved book locally to refresh its
|
||||
// cover/title/author immediately (otherwise it shows the stale prop).
|
||||
const [displayBook, setDisplayBook] = useState<Book>(book);
|
||||
|
||||
// Initialize metadata edit hook
|
||||
const {
|
||||
@@ -110,6 +115,10 @@ const BookDetailModal: React.FC<BookDetailModalProps> = ({
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, [book]);
|
||||
|
||||
useEffect(() => {
|
||||
setDisplayBook(book);
|
||||
}, [book]);
|
||||
|
||||
const handleClose = () => {
|
||||
setBookMeta(null);
|
||||
setEditMode(false);
|
||||
@@ -129,6 +138,9 @@ const BookDetailModal: React.FC<BookDetailModalProps> = ({
|
||||
const handleSaveMetadata = () => {
|
||||
if (editedMeta && handleBookMetadataUpdate) {
|
||||
setBookMeta({ ...editedMeta });
|
||||
// Capture the updated book before handleBookMetadataUpdate clears the
|
||||
// temporary cover fields on editedMeta, so the view refreshes its cover.
|
||||
setDisplayBook(getBookWithUpdatedMetadata(book, editedMeta));
|
||||
handleBookMetadataUpdate(book, editedMeta);
|
||||
setEditMode(false);
|
||||
}
|
||||
@@ -220,7 +232,7 @@ const BookDetailModal: React.FC<BookDetailModalProps> = ({
|
||||
/>
|
||||
) : (
|
||||
<BookDetailView
|
||||
book={book}
|
||||
book={displayBook}
|
||||
metadata={bookMeta}
|
||||
fileSize={fileSize}
|
||||
onEdit={handleBookMetadataUpdate ? handleEditMetadata : undefined}
|
||||
|
||||
@@ -182,6 +182,26 @@ export const getPrimaryLanguage = (lang: string | string[] | undefined) => {
|
||||
return 'en';
|
||||
};
|
||||
|
||||
// Immutably apply edited metadata to a book, returning a NEW book object.
|
||||
// Callers must not mutate the existing book in place: <BookCover> is memoized
|
||||
// and compares fields off the book, so an in-place mutation makes the memo's
|
||||
// previous snapshot point to the same object and skips re-rendering the cover.
|
||||
export const getBookWithUpdatedMetadata = (book: Book, metadata: BookMetadata): Book => {
|
||||
const updatedBook: Book = {
|
||||
...book,
|
||||
metadata,
|
||||
title: formatTitle(metadata.title),
|
||||
author: formatAuthors(metadata.author),
|
||||
primaryLanguage: getPrimaryLanguage(metadata.language),
|
||||
updatedAt: Date.now(),
|
||||
};
|
||||
const newCoverImageUrl = metadata.coverImageBlobUrl || metadata.coverImageUrl;
|
||||
if (newCoverImageUrl) {
|
||||
updatedBook.coverImageUrl = newCoverImageUrl;
|
||||
}
|
||||
return updatedBook;
|
||||
};
|
||||
|
||||
export const formatDate = (date: string | number | Date | null | undefined, isUTC = false) => {
|
||||
if (!date) return;
|
||||
const userLang = getUserLang();
|
||||
|
||||
Reference in New Issue
Block a user