From d943a1c1467843b467e2186207d6e08acfbf92d7 Mon Sep 17 00:00:00 2001 From: loveheaven Date: Wed, 20 May 2026 12:31:39 +0800 Subject: [PATCH] fix(library): clear nested-folder groups when deleting from bookshelf (#4226) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(library): clear nested-folder groups when deleting from bookshelf Deleting a group from the bookshelf right-click menu used to leave the group on screen whenever the import had any sub-directories. The cause: getBooksToDelete matched only `book.groupId === id`, but the bookshelf renders a top-level group with id = md5("MyDir") while books imported from a sub-folder carry groupId = md5("MyDir/sub"). Sub-folder books never got marked for deletion, refreshGroups re-built the parent group from their groupName on the next render, and the user saw an undeletable folder. Fix: when an id resolves to a known group via getGroupName, also collect every book whose groupName equals that path or starts with `${path}/`. Hash-based dedup keeps a book from being queued twice when both rules match. Single-book deletes and flat-folder group deletes are unaffected. * refactor: expand group selections into book hashes at intake Address review feedback on #4226: instead of re-deriving which books belong to a group inside the deletion path with a path-prefix sweep, resolve group ids into their constituent book hashes upstream where the selection enters the deletion pipeline. * New helper `expandBookshelfSelection(ids, items)` in libraryUtils: group ids resolve to every (non-soft-deleted) book in the rendered rollup; standalone book hashes pass through. Tested in isolation. * `Bookshelf.deleteSelectedBooks` runs select-mode picks through the helper before populating `bookIdsToDelete`. * `BookshelfItem` right-click group delete dispatches the constituent hashes from `group.books` directly, so the receiver is a simple pass-through. * `getBooksToDelete` collapses to a flat hash lookup — no prefix sweep, no `getGroupName` call in the deletion path, no dedup set. The nested-folder fix still holds because `generateBookshelfItems` already rolls "MyDir/sub" books into the top-level "MyDir" group; expanding via the rendered `group.books` picks them up automatically. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Huang Xin Co-authored-by: Claude Opus 4.7 (1M context) --- .../src/__tests__/utils/library-utils.test.ts | 82 +++++++++++++++++++ .../src/app/library/components/Bookshelf.tsx | 24 +++--- .../app/library/components/BookshelfItem.tsx | 7 +- .../src/app/library/utils/libraryUtils.ts | 26 ++++++ 4 files changed, 128 insertions(+), 11 deletions(-) diff --git a/apps/readest-app/src/__tests__/utils/library-utils.test.ts b/apps/readest-app/src/__tests__/utils/library-utils.test.ts index fe326cca..a50185db 100644 --- a/apps/readest-app/src/__tests__/utils/library-utils.test.ts +++ b/apps/readest-app/src/__tests__/utils/library-utils.test.ts @@ -10,6 +10,7 @@ import { ensureLibraryGroupByType, findGroupById, getGroupDisplayName, + expandBookshelfSelection, } from '../../app/library/utils/libraryUtils'; import { Book, BooksGroup } from '../../types/book'; import { LibraryGroupByType, LibrarySortByType } from '../../types/settings'; @@ -983,3 +984,84 @@ describe('getGroupDisplayName', () => { expect(getGroupDisplayName(items, 'non-existent')).toBeUndefined(); }); }); + +describe('expandBookshelfSelection', () => { + const createMockGroup = (overrides: Partial = {}): BooksGroup => ({ + id: 'test-group', + name: 'Test Group', + displayName: 'Test Display Name', + books: [], + updatedAt: Date.now(), + ...overrides, + }); + + it('passes through standalone book hashes unchanged', () => { + const items: (Book | BooksGroup)[] = [ + createMockBook({ hash: 'book-1' }), + createMockBook({ hash: 'book-2' }), + ]; + + expect(expandBookshelfSelection(['book-1', 'book-2'], items)).toEqual(['book-1', 'book-2']); + }); + + it('expands a group id into every book hash in the rendered rollup', () => { + // generateBookshelfItems rolls nested-folder books into the top-level + // group, so the rendered BooksGroup.books already includes them. The + // helper must rely on that rollup so deleting "MyDir" also catches + // books whose own groupName is "MyDir/sub". + const items: (Book | BooksGroup)[] = [ + createMockGroup({ + id: 'group-mydir', + name: 'MyDir', + books: [ + createMockBook({ hash: 'direct-book', groupName: 'MyDir', groupId: 'group-mydir' }), + createMockBook({ hash: 'nested-book', groupName: 'MyDir/sub', groupId: 'group-sub' }), + ], + }), + ]; + + expect(expandBookshelfSelection(['group-mydir'], items).sort()).toEqual([ + 'direct-book', + 'nested-book', + ]); + }); + + it('deduplicates when a book hash and its parent group are both selected', () => { + const items: (Book | BooksGroup)[] = [ + createMockGroup({ + id: 'group-1', + books: [createMockBook({ hash: 'book-a' }), createMockBook({ hash: 'book-b' })], + }), + ]; + + expect(expandBookshelfSelection(['book-a', 'group-1'], items).sort()).toEqual([ + 'book-a', + 'book-b', + ]); + }); + + it('skips soft-deleted books inside a group', () => { + const items: (Book | BooksGroup)[] = [ + createMockGroup({ + id: 'group-1', + books: [ + createMockBook({ hash: 'alive' }), + createMockBook({ hash: 'gone', deletedAt: Date.now() }), + ], + }), + ]; + + expect(expandBookshelfSelection(['group-1'], items)).toEqual(['alive']); + }); + + it('returns an empty array when no ids are given', () => { + expect(expandBookshelfSelection([], [])).toEqual([]); + }); + + it('preserves unknown ids so callers can surface stale selections', () => { + // Defensive: if the rendered items no longer contain a selected id (e.g. + // it was a hash from another view), the helper leaves it alone rather + // than silently dropping it. + expect(expandBookshelfSelection(['ghost-hash'], [])).toEqual(['ghost-hash']); + }); +}); diff --git a/apps/readest-app/src/app/library/components/Bookshelf.tsx b/apps/readest-app/src/app/library/components/Bookshelf.tsx index 9ff07e2b..9edbcf8e 100644 --- a/apps/readest-app/src/app/library/components/Bookshelf.tsx +++ b/apps/readest-app/src/app/library/components/Bookshelf.tsx @@ -36,6 +36,7 @@ import { createWithinGroupSorter, ensureLibraryGroupByType, ensureLibrarySortByType, + expandBookshelfSelection, getBookSortValue, getGroupSortValue, compareSortValues, @@ -359,16 +360,14 @@ const Bookshelf: React.FC = ({ } }; + // `bookIdsToDelete` always holds book hashes by the time we get here — + // group ids are expanded into their constituent hashes at intake (see + // `deleteSelectedBooks` and `handleDeleteBooksIntent`), so a top-level + // folder is now resolved against the rendered group's `books` rollup, + // which already includes nested sub-folder books. const getBooksToDelete = () => { - const booksToDelete: Book[] = []; - bookIdsToDelete.forEach((id) => { - for (const book of filteredBooks.filter((book) => book.hash === id || book.groupId === id)) { - if (book && !book.deletedAt) { - booksToDelete.push(book); - } - } - }); - return booksToDelete; + const wanted = new Set(bookIdsToDelete); + return filteredBooks.filter((book) => wanted.has(book.hash) && !book.deletedAt); }; const confirmDelete = async () => { @@ -390,7 +389,12 @@ const Bookshelf: React.FC = ({ }; const deleteSelectedBooks = () => { - setBookIdsToDelete(getSelectedBooks()); + // Expand any group ids in the selection into the book hashes they + // visually represent — `generateBookshelfItems` rolls nested-folder + // books into the parent group, and we want every one of them queued + // for deletion, not just the books whose own `groupId` happens to + // match the top-level group's id. + setBookIdsToDelete(expandBookshelfSelection(getSelectedBooks(), sortedBookshelfItems)); setShowSelectModeActions(false); setShowDeleteAlert(true); }; diff --git a/apps/readest-app/src/app/library/components/BookshelfItem.tsx b/apps/readest-app/src/app/library/components/BookshelfItem.tsx index a19cf191..423d7da2 100644 --- a/apps/readest-app/src/app/library/components/BookshelfItem.tsx +++ b/apps/readest-app/src/app/library/components/BookshelfItem.tsx @@ -317,7 +317,12 @@ const BookshelfItem: React.FC = ({ const deleteGroupMenuItem = await MenuItem.new({ text: _('Delete'), action: async () => { - eventDispatcher.dispatch('delete-books', { ids: [group.id] }); + // Dispatch the constituent book hashes — `group.books` is the + // rendered rollup and already includes books from nested sub- + // folders, so the deletion path doesn't need to re-derive what + // belongs to the group from the id alone. + const ids = group.books.filter((book) => !book.deletedAt).map((book) => book.hash); + eventDispatcher.dispatch('delete-books', { ids }); }, }); const menu = await Menu.new(); diff --git a/apps/readest-app/src/app/library/utils/libraryUtils.ts b/apps/readest-app/src/app/library/utils/libraryUtils.ts index 91d34012..ded95ee2 100644 --- a/apps/readest-app/src/app/library/utils/libraryUtils.ts +++ b/apps/readest-app/src/app/library/utils/libraryUtils.ts @@ -59,6 +59,32 @@ export const getGroupDisplayName = ( return group?.displayName || group?.name; }; +/** + * Expand a list of selection ids (book hashes or group ids from the rendered + * bookshelf) into the unique book hashes those ids represent. + * + * Group ids resolve to every (non-soft-deleted) book in the group's visible + * rollup — `generateBookshelfItems` already folds nested-folder books into + * their top-level group, so the rendered `BooksGroup.books` is the source of + * truth. Standalone book hashes (and any unknown ids) pass through unchanged, + * letting callers like the bookshelf delete flow collect the right set up + * front instead of re-deriving it later. + */ +export const expandBookshelfSelection = (ids: string[], items: (Book | BooksGroup)[]): string[] => { + const hashes = new Set(); + for (const id of ids) { + const group = findGroupById(items, id); + if (group) { + for (const book of group.books) { + if (!book.deletedAt) hashes.add(book.hash); + } + } else { + hashes.add(id); + } + } + return [...hashes]; +}; + export const createBookFilter = (queryTerm: string | null) => (item: Book) => { if (!queryTerm) return true; if (item.deletedAt) return false;