fix(file): allow physical books to be moved between libraries#296
fix(file): allow physical books to be moved between libraries#296dsmouse wants to merge 7 commits intogrimmory-tools:developfrom
Conversation
## [2.3.0](grimmory-tools/grimmory@v2.2.6...v2.3.0) (2026-03-21) ### Features * **release:** document develop-based stable release previews ([930e526](grimmory-tools@930e526)) ### Bug Fixes * **api:** fix potential memory leaks in file processing ([031e8ae](grimmory-tools@031e8ae)) * **ci:** correct artifact download action pin ([37ca101](grimmory-tools@37ca101)) * **ci:** publish PR test results from workflow_run ([11a76bf](grimmory-tools@11a76bf)) * **ci:** repair release preview and test result publishing ([afa5b81](grimmory-tools@afa5b81)) * drop telemetry from app ([grimmory-tools#52](grimmory-tools#52)) ([4d82cb7](grimmory-tools@4d82cb7)) * **ui:** repair frontend compile after rebrand ([fea1ec6](grimmory-tools@fea1ec6)) ### Refactors * **build:** rename frontend dist output to grimmory ([ecf388f](grimmory-tools@ecf388f)) * **i18n:** rename booklore translation keys to grimmory ([eb94afa](grimmory-tools@eb94afa)) * **metadata:** move default parser from Amazon to Goodreads ([e252122](grimmory-tools@e252122)) * pull kepubify & ffprobe during build ([grimmory-tools#50](grimmory-tools#50)) ([1c15629](grimmory-tools@1c15629)) * **ui:** rebrand frontend surfaces to grimmory ([d786dd8](grimmory-tools@d786dd8)) ### Chores * **api:** remove the custom startup banner ([98c9b1a](grimmory-tools@98c9b1a)) * **deps:** bump flatted from 3.4.1 to 3.4.2 in /booklore-ui ([grimmory-tools#73](grimmory-tools#73)) ([c4bd0c7](grimmory-tools@c4bd0c7)) * **funding:** point support links at opencollective ([55c0ac0](grimmory-tools@55c0ac0)) * **release:** 2.2.7 [skip ci] ([0b5e24c](grimmory-tools@0b5e24c)) * remove old verbose PR template, replace with temporary more low-key one. ([grimmory-tools#84](grimmory-tools#84)) ([b868526](grimmory-tools@b868526)) * **ui:** drop financial support dialog ([grimmory-tools#21](grimmory-tools#21)) ([62be6b1](grimmory-tools@62be6b1)) ### Documentation * updated supported file formats in README.md ([grimmory-tools#68](grimmory-tools#68)) ([f912e80](grimmory-tools@f912e80)) ### Style * **i18n:** normalize translation json formatting ([grimmory-tools#89](grimmory-tools#89)) ([857290d](grimmory-tools@857290d)) * **ui:** simplify the topbar logo branding ([0416d48](grimmory-tools@0416d48))
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralized shelf-filtering into BookUtils.filterShelvesByUserId (handles nulls and public shelves); updated services to delegate to that utility; deduplicated book-file processing and fixed physical-book library moves in FileMoveService; updated unit tests for the new shelf-filtering semantics. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FileMoveService
participant BookRepository
participant EntityManager
participant TopicPublisher
Client->>FileMoveService: requestMove(bookId, targetLibrary)
FileMoveService->>BookRepository: findBookWithFiles(bookId)
alt no files && isPhysical == true
FileMoveService->>BookRepository: updateLibrary(bookId, targetLibrary)
FileMoveService->>EntityManager: clear()
FileMoveService->>BookRepository: findBookWithFiles(bookId)
FileMoveService->>TopicPublisher: publish(BOOK_UPDATE, book)
FileMoveService->>Client: return success
else has files
FileMoveService->>FileMoveService: deduplicate bookFiles by id
FileMoveService->>FileMoveService: validate source paths and plan staging/commit
FileMoveService->>BookRepository: update filenames/subpaths as needed
FileMoveService->>Client: return success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
booklore-api/src/main/java/org/booklore/service/file/FileMoveService.java (1)
125-130:⚠️ Potential issue | 🔴 CriticalThe physical-book move path is still missing.
processSingleMove()still returns as soon asbookFilesis empty, so a physical book is skipped exactly as before: no library/path update happens and noBOOK_UPDATEis sent. That is the original issue#295behavior. This needs a DB-only branch before the early return that updates the target library/path, skips file I/O, refetches, and emits the normal notification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/file/FileMoveService.java` around lines 125 - 130, processSingleMove currently returns early when bookEntity.getBookFiles() is empty, skipping the DB-only move path; modify processSingleMove so that if bookFiles is null or empty it performs a database-only update: set the BookEntity's libraryId and path to the target values, persist the change (via the repository/save used elsewhere), refetch the updated BookEntity (to ensure relations are loaded), and emit the normal BOOK_UPDATE notification before returning; keep the existing file-I/O code path for non-empty bookFiles (refer to processSingleMove and the local variable bookFiles / bookEntity.getBookFiles()) so physical files are skipped but library/path and event behavior remain identical for empty-file (physical-book) moves.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-api/src/main/java/org/booklore/service/book/BookService.java`:
- Around line 524-528: The change in BookService.filterShelvesByUserId now
allows other users' public shelves, but BookUpdateService.buildBook and
MetadataRefreshService.updateBookMetadata still use the old owner-only predicate
causing inconsistent serialization; centralize the shelf-visibility predicate
and apply it everywhere: update BookUpdateService.buildBook and
MetadataRefreshService.updateBookMetadata (and any builders used by getBook(),
getBookDTOs(), BookFileAttachmentService.getUpdatedBook(),
BookFileDetachmentService.getUpdatedBook()) to call the centralized method
(e.g., BookService.filterShelvesByUserId or a new util like
ShelfVisibility.shouldInclude(shelf, userId)) so all code paths use the same
logic for owner-or-public shelves. Ensure you replace inline predicates with the
shared method and run tests for these service methods.
In `@booklore-api/src/main/java/org/booklore/service/file/FileMoveService.java`:
- Line 130: The current use of Stream.distinct() on bookEntity.getBookFiles()
only removes identical object references; change both processSingleMove() (where
List<BookFileEntity> bookFiles =
bookEntity.getBookFiles().stream().distinct().toList();) and moveSingleFile() to
deduplicate by the stable identifier instead: group or map by
BookFileEntity.getId() and collect a single instance per id (e.g., use
.collect(Collectors.toMap(BookFileEntity::getId, Function.identity(),
(a,b)->a)).values() or a groupingBy then pickFirst) so subsequent staging/moving
logic operates on unique database rows rather than object instances.
---
Outside diff comments:
In `@booklore-api/src/main/java/org/booklore/service/file/FileMoveService.java`:
- Around line 125-130: processSingleMove currently returns early when
bookEntity.getBookFiles() is empty, skipping the DB-only move path; modify
processSingleMove so that if bookFiles is null or empty it performs a
database-only update: set the BookEntity's libraryId and path to the target
values, persist the change (via the repository/save used elsewhere), refetch the
updated BookEntity (to ensure relations are loaded), and emit the normal
BOOK_UPDATE notification before returning; keep the existing file-I/O code path
for non-empty bookFiles (refer to processSingleMove and the local variable
bookFiles / bookEntity.getBookFiles()) so physical files are skipped but
library/path and event behavior remain identical for empty-file (physical-book)
moves.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2433342c-570a-4273-be1f-66e02afed094
📒 Files selected for processing (3)
booklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/file/FileMoveService.javabooklore-api/src/test/java/org/booklore/service/book/BookServiceTest.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/test/java/org/booklore/service/book/BookServiceTest.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/file/FileMoveService.java
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/service/book/BookServiceTest.java
🧠 Learnings (3)
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/test/**/*.java : Prefer focused unit tests; use `SpringBootTest` only when the Spring context is required
Applied to files:
booklore-api/src/test/java/org/booklore/service/book/BookServiceTest.java
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/BookService.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/main/java/org/booklore/service/file/FileMoveService.java
booklore-api/src/main/java/org/booklore/service/book/BookService.java
Outdated
Show resolved
Hide resolved
booklore-api/src/main/java/org/booklore/service/file/FileMoveService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-api/src/main/java/org/booklore/util/BookUtils.java`:
- Around line 16-21: The filterShelvesByUserId method in BookUtils can NPE when
userId is null because the predicate calls userId.equals(...); change the
predicate to avoid calling methods on userId — e.g. replace the filter with
Objects.equals(userId, shelf.getUserId()) || shelf.isPublicShelf() (or check
shelf.isPublicShelf() first) so public shelves are always included and null
userId is handled safely; update imports if using java.util.Objects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0836e7a1-92e7-458a-a233-c59982cb6bcc
📒 Files selected for processing (5)
booklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/book/BookUpdateService.javabooklore-api/src/main/java/org/booklore/service/file/FileMoveService.javabooklore-api/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabooklore-api/src/main/java/org/booklore/util/BookUtils.java
🚧 Files skipped from review as they are similar to previous changes (2)
- booklore-api/src/main/java/org/booklore/service/book/BookService.java
- booklore-api/src/main/java/org/booklore/service/file/FileMoveService.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/book/BookUpdateService.javabooklore-api/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabooklore-api/src/main/java/org/booklore/util/BookUtils.java
🧠 Learnings (2)
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/BookUpdateService.javabooklore-api/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabooklore-api/src/main/java/org/booklore/util/BookUtils.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/BookUpdateService.java
🔇 Additional comments (2)
booklore-api/src/main/java/org/booklore/service/metadata/MetadataRefreshService.java (1)
850-852: Good consolidation to shared shelf-filter utility.The delegation keeps filtering semantics centralized and avoids drift across services.
booklore-api/src/main/java/org/booklore/service/book/BookUpdateService.java (1)
363-365: Nice refactor to remove duplicated shelf-filtering logic.This keeps behavior consistent with other services and simplifies maintenance.
|
gonna put in draft to run full backend unit test |
|
Rar5FallbackIntegrationTest > readerService_streamsImageFromRar5(Path) FAILED Rar5FallbackIntegrationTest > readerService_listsImagePagesFromRar5(Path) FAILED I didn't touch rar5 handling; unrelated failure. |
agreed, that should be fine. Thanks for this I'll try to review it, later. |
|
Those RAR test failures are most likely caused by a missing unarchiving package in your PATH that supports the RAR format. The Docker container includes the RAR binary, which is why the tests pass there but fail locally. There's an open PR that, among other things, will make those tests conditional on whether the user has RAR tooling available in their PATH. This way, the tests simply won't run instead of failing, avoiding further confusion. Anyways, this is to say, in you want to fix it, download unrar, otherwise if you are not interested in development of CBX specific code you can ignore that. Thanks again for your PRs again! |
imajes
changed the title
2 participants
Fixes #295
What went wrong
FileMoveService.processSingleMove bails out early when a book has no associated files. Physical books have no BookFileEntity records by design, so they were silently dropped at this guard with no feedback to the user.
Fix
Before returning, check isPhysical. If true, skip file I/O entirely and update only the library/path association in the database, then send the usual BOOK_UPDATE websocket notification so the UI reflects the change.
Summary by CodeRabbit
New Features
Bug Fixes
Tests