fix(library): clear nested-folder groups when deleting from bookshelf (#4226)

* 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) <noreply@anthropic.com> --------- Co-authored-by: Huang Xin <chrox.huang@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-20 12:31:39 +08:00
parent 7230981288
commit d943a1c146
4 changed files with 128 additions and 11 deletions
@@ -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> = {}): 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']);
+  });
+});
@@ -36,6 +36,7 @@ import {
  createWithinGroupSorter,
  ensureLibraryGroupByType,
  ensureLibrarySortByType,
+  expandBookshelfSelection,
  getBookSortValue,
  getGroupSortValue,
  compareSortValues,
@@ -359,16 +360,14 @@ const Bookshelf: React.FC<BookshelfProps> = ({
    }
  };

+  // `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<BookshelfProps> = ({
  };

  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);
  };
@@ -317,7 +317,12 @@ const BookshelfItem: React.FC<BookshelfItemProps> = ({
    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();
@@ -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<string>();
+  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;