refactor(library): implement asynchronous book addition and cover generation with event handling#329
Conversation
…eration with event handling
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRelocated async enablement to config and switched executor to virtual threads; introduced event types and async transactional listeners for book-added and library-scan workflows; extracted group/cover processing into new components; updated callers and tests to publish/listen for events and to delegate processing. Changes
Sequence Diagram(s)sequenceDiagram
participant GroupProcessor as BookGroupProcessor
participant Publisher as ApplicationEventPublisher
participant EventListener as BookAddedEventListener
participant Broadcaster as BookEventBroadcaster
participant Kobo as KoboAutoShelfService
GroupProcessor->>Publisher: publish(BookAddedEvent(book))
note right of Publisher: TransactionPhase.AFTER_COMMIT\nDelivered asynchronously
Publisher->>EventListener: deliver BookAddedEvent (async)
EventListener->>Broadcaster: broadcastBookAddEvent(book)
EventListener->>Kobo: autoAddBookToKoboShelves(book.id)
sequenceDiagram
participant LibraryService
participant Publisher as ApplicationEventPublisher
participant ScanListener as LibraryScanListener
participant Processing as LibraryProcessingService
LibraryService->>Publisher: publish(LibraryScanRequestedEvent(libraryId, fullRescan))
note right of Publisher: TransactionPhase.AFTER_COMMIT\nDelivered asynchronously
Publisher->>ScanListener: deliver LibraryScanRequestedEvent (async)
ScanListener-->>ScanListener: dedupe check (scanningLibraries)
alt not already scanning
ScanListener->>Processing: rescanLibrary(...) or processLibrary(...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This will conflict bunch of PRs so merge last if possible. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
booklore-api/src/test/java/org/booklore/service/watcher/LibraryFileEventProcessorTest.java (1)
38-39: Add a focused test for the “scan already running” branch.Since
LibraryScanListeneris now a hard dependency, a dedicated test withwhen(libraryScanListener.isScanning(1L)).thenReturn(true)should assert that incoming file events are skipped.As per coding guidelines "Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required".Also applies to: 54-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/test/java/org/booklore/service/watcher/LibraryFileEventProcessorTest.java` around lines 38 - 39, Add a focused unit test in LibraryFileEventProcessorTest that mocks LibraryScanListener and stubs libraryScanListener.isScanning(1L) to return true, then sends a file event into the LibraryFileEventProcessor and asserts that the event is skipped (e.g., verify no interactions with PendingDeletionPool and no processing methods called); ensure the test is a plain unit test (do not use `@SpringBootTest`) and reference the existing mocks libraryScanListener and pendingDeletionPool and the isScanning(long) method when implementing the test.booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.java (1)
60-71: Consider adding one focused assertion for cover-generation delegation.Now that
BookCoverGeneratoris injected, add a targeted rescan/attach test that verifiesgenerateCoverFromAdditionalFile(...)is invoked for an auto-attached file.As per coding guidelines "Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.java` around lines 60 - 71, Add a focused unit test in LibraryProcessingServiceTest that verifies LibraryProcessingService delegates cover generation to the injected BookCoverGenerator: mock BookCoverGenerator and any necessary collaborators, create a test scenario that simulates an auto-attached additional file (the same setup used elsewhere in this test class), call the LibraryProcessingService method that handles rescan/attach (the method under test that currently triggers attachment behavior), and assert (verify) that BookCoverGenerator.generateCoverFromAdditionalFile(...) was invoked with the expected Book and AdditionalFile arguments; keep this as a plain unit test (do not use `@SpringBootTest`) and use the existing libraryProcessingService, bookAdditionalFileRepository, and related test fixtures to construct the scenario.booklore-api/src/test/java/org/booklore/service/bookdrop/BookDropServiceTest.java (1)
21-21: Test mocking updated correctly, but event publication is not verified.The test correctly replaces
KoboAutoShelfServicemock withApplicationEventPublishermock. However, none of the existing tests verify thateventPublisher.publishEvent(any(BookAddedEvent.class))is called on successful import. Consider adding verification in tests likefinalizeImport_WhenProcessingSucceeds_ShouldDeleteSourceFileFromBookdropto ensure the event is published.💡 Suggested verification addition
In the
finalizeImport_WhenProcessingSucceeds_ShouldDeleteSourceFileFromBookdroptest (around line 509), add:verify(eventPublisher).publishEvent(any(BookAddedEvent.class));Also applies to: 28-28, 89-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/test/java/org/booklore/service/bookdrop/BookDropServiceTest.java` at line 21, The test suite replaced KoboAutoShelfService with an ApplicationEventPublisher mock but never asserts that a BookAddedEvent is published; update tests such as finalizeImport_WhenProcessingSucceeds_ShouldDeleteSourceFileFromBookdrop to verify the event publication by adding a verification against the eventPublisher mock (e.g., verify(eventPublisher).publishEvent(any(BookAddedEvent.class))); locate usages of ApplicationEventPublisher named eventPublisher and add this verify in tests that assert successful import behavior.booklore-api/src/main/java/org/booklore/service/library/BookGroupProcessor.java (1)
140-147: Exception handling for additional file creation appropriately logs and continues.Swallowing exceptions for additional file creation is reasonable since these are supplementary files that shouldn't fail the primary book import. However, consider whether cover generation failures specifically should be logged at a different level or tracked for retry.
💭 Optional: Distinguish cover generation failures
try { bookAdditionalFileRepository.save(additionalFile); String primaryFileName = bookEntity.getPrimaryBookFile() != null ? bookEntity.getPrimaryBookFile().getFileName() : "unknown"; log.info("Attached additional format {} to book: {}", file.getFileName(), primaryFileName); bookCoverGenerator.generateCoverFromAdditionalFile(bookEntity, file); } catch (Exception e) { - log.error("Error creating additional file {}: {}", file.getFileName(), e.getMessage()); + log.error("Error creating additional file {}: {}", file.getFileName(), e.getMessage(), e); }Including the full stack trace with
ehelps debugging, especially for cover generation issues that may have deeper root causes.🤖 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/library/BookGroupProcessor.java` around lines 140 - 147, The catch currently logs only e.getMessage() when any error occurs while saving/processing additional files; update exception handling so the save and the cover generation are treated separately: keep the surrounding try for bookAdditionalFileRepository.save(additionalFile) and include a second try/catch around bookCoverGenerator.generateCoverFromAdditionalFile(bookEntity, file) that logs failures with the full throwable (use log.error("Error generating cover from additional file {} for book {}.", file.getFileName(), primaryFileName, e)) and optionally a different log level/message, and change the original catch for the save block to log the full exception (log.error("Error creating additional file {}: {}", file.getFileName(), e)) so stack traces are preserved for debugging while still allowing import to continue.booklore-api/src/main/java/org/booklore/service/library/LibraryScanListener.java (1)
52-53: Reconsider debug-level logging for data access exception.Logging
InvalidDataAccessApiUsageExceptionatdebuglevel may obscure actual issues. If this exception is expected in specific scenarios (e.g., library deleted mid-scan), consider adding a comment explaining why, or logging atwarnwith context about when it's benign.🤖 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/library/LibraryScanListener.java` around lines 52 - 53, The catch block in LibraryScanListener that currently logs InvalidDataAccessApiUsageException at debug level should be changed to log at warn (or retain debug but add a clarifying comment) so real issues aren't obscured; update the catch in the method handling library scans to call log.warn("InvalidDataAccessApiUsageException during library scan - Library id: {}. This can occur if the library was deleted mid-scan; treating as benign.", libraryId, e) (or add a brief comment explaining the benign scenario if you prefer debug), ensuring the exception and contextual libraryId are preserved in the log.
🤖 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/library/BookCoverGenerator.java`:
- Around line 97-99: The code assumes bookEntity.getMetadata() is non-null
before updating cover fields, which can NPE; guard both places (the block
updating audiobook cover around
bookEntity.getMetadata().setAudiobookCoverUpdatedOn(...) and the similar block
in generateEbookCoverFromFile that updates ebook cover) by checking if
bookEntity.getMetadata() == null and, if so, instantiating and setting a new
metadata object (e.g., new BookMetadata()) via bookEntity.setMetadata(...)
before calling setAudiobookCoverUpdatedOn / setEbookCoverUpdatedOn, then proceed
to set the cover hash and save the bookEntity so the newly created metadata is
persisted.
In
`@booklore-api/src/main/java/org/booklore/service/library/FileAsBookProcessor.java`:
- Around line 34-37: The catch block in FileAsBookProcessor currently logs only
e.getMessage(), losing the stack trace; update the log.error call in the catch
for the file-group processing (inside class FileAsBookProcessor) to pass the
exception object so the logger records the full stack trace (e.g., include e as
the throwable parameter or use the logger overload that accepts a Throwable)
while keeping the same message and fileNames variable.
In
`@booklore-api/src/main/java/org/booklore/service/library/LibraryScanRequestedEvent.java`:
- Around line 7-10: The record LibraryScanRequestedEvent documents only the
libraryId parameter; add documentation for the fullRescan component in the
public JavaDoc by describing its purpose and expected values (e.g., whether true
forces a complete rescan and false means incremental), updating the Javadoc
block above LibraryScanRequestedEvent to include a `@param` fullRescan line that
clearly explains the flag's semantics.
---
Nitpick comments:
In
`@booklore-api/src/main/java/org/booklore/service/library/BookGroupProcessor.java`:
- Around line 140-147: The catch currently logs only e.getMessage() when any
error occurs while saving/processing additional files; update exception handling
so the save and the cover generation are treated separately: keep the
surrounding try for bookAdditionalFileRepository.save(additionalFile) and
include a second try/catch around
bookCoverGenerator.generateCoverFromAdditionalFile(bookEntity, file) that logs
failures with the full throwable (use log.error("Error generating cover from
additional file {} for book {}.", file.getFileName(), primaryFileName, e)) and
optionally a different log level/message, and change the original catch for the
save block to log the full exception (log.error("Error creating additional file
{}: {}", file.getFileName(), e)) so stack traces are preserved for debugging
while still allowing import to continue.
In
`@booklore-api/src/main/java/org/booklore/service/library/LibraryScanListener.java`:
- Around line 52-53: The catch block in LibraryScanListener that currently logs
InvalidDataAccessApiUsageException at debug level should be changed to log at
warn (or retain debug but add a clarifying comment) so real issues aren't
obscured; update the catch in the method handling library scans to call
log.warn("InvalidDataAccessApiUsageException during library scan - Library id:
{}. This can occur if the library was deleted mid-scan; treating as benign.",
libraryId, e) (or add a brief comment explaining the benign scenario if you
prefer debug), ensuring the exception and contextual libraryId are preserved in
the log.
In
`@booklore-api/src/test/java/org/booklore/service/bookdrop/BookDropServiceTest.java`:
- Line 21: The test suite replaced KoboAutoShelfService with an
ApplicationEventPublisher mock but never asserts that a BookAddedEvent is
published; update tests such as
finalizeImport_WhenProcessingSucceeds_ShouldDeleteSourceFileFromBookdrop to
verify the event publication by adding a verification against the eventPublisher
mock (e.g., verify(eventPublisher).publishEvent(any(BookAddedEvent.class)));
locate usages of ApplicationEventPublisher named eventPublisher and add this
verify in tests that assert successful import behavior.
In
`@booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.java`:
- Around line 60-71: Add a focused unit test in LibraryProcessingServiceTest
that verifies LibraryProcessingService delegates cover generation to the
injected BookCoverGenerator: mock BookCoverGenerator and any necessary
collaborators, create a test scenario that simulates an auto-attached additional
file (the same setup used elsewhere in this test class), call the
LibraryProcessingService method that handles rescan/attach (the method under
test that currently triggers attachment behavior), and assert (verify) that
BookCoverGenerator.generateCoverFromAdditionalFile(...) was invoked with the
expected Book and AdditionalFile arguments; keep this as a plain unit test (do
not use `@SpringBootTest`) and use the existing libraryProcessingService,
bookAdditionalFileRepository, and related test fixtures to construct the
scenario.
In
`@booklore-api/src/test/java/org/booklore/service/watcher/LibraryFileEventProcessorTest.java`:
- Around line 38-39: Add a focused unit test in LibraryFileEventProcessorTest
that mocks LibraryScanListener and stubs libraryScanListener.isScanning(1L) to
return true, then sends a file event into the LibraryFileEventProcessor and
asserts that the event is skipped (e.g., verify no interactions with
PendingDeletionPool and no processing methods called); ensure the test is a
plain unit test (do not use `@SpringBootTest`) and reference the existing mocks
libraryScanListener and pendingDeletionPool and the isScanning(long) method when
implementing the test.
🪄 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: 896828d4-685a-40a0-b2a0-c0ec4bcaced6
📒 Files selected for processing (19)
booklore-api/src/main/java/org/booklore/BookloreApplication.javabooklore-api/src/main/java/org/booklore/config/TaskExecutorConfig.javabooklore-api/src/main/java/org/booklore/service/bookdrop/BookDropService.javabooklore-api/src/main/java/org/booklore/service/event/BookAddedEvent.javabooklore-api/src/main/java/org/booklore/service/event/BookAddedEventListener.javabooklore-api/src/main/java/org/booklore/service/library/BookCoverGenerator.javabooklore-api/src/main/java/org/booklore/service/library/BookGroupProcessor.javabooklore-api/src/main/java/org/booklore/service/library/FileAsBookProcessor.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/main/java/org/booklore/service/library/LibraryScanListener.javabooklore-api/src/main/java/org/booklore/service/library/LibraryScanRequestedEvent.javabooklore-api/src/main/java/org/booklore/service/library/LibraryService.javabooklore-api/src/main/java/org/booklore/service/watcher/LibraryFileEventProcessor.javabooklore-api/src/test/java/org/booklore/service/bookdrop/BookDropServiceTest.javabooklore-api/src/test/java/org/booklore/service/library/FileAsBookProcessorTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceRegressionTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryServiceIconTest.javabooklore-api/src/test/java/org/booklore/service/watcher/LibraryFileEventProcessorTest.java
💤 Files with no reviewable changes (1)
- booklore-api/src/main/java/org/booklore/BookloreApplication.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 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/main/java/org/booklore/service/event/BookAddedEvent.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceRegressionTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/watcher/LibraryFileEventProcessorTest.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/test/java/org/booklore/service/library/LibraryServiceIconTest.javabooklore-api/src/main/java/org/booklore/service/library/LibraryScanRequestedEvent.javabooklore-api/src/main/java/org/booklore/config/TaskExecutorConfig.javabooklore-api/src/main/java/org/booklore/service/event/BookAddedEventListener.javabooklore-api/src/main/java/org/booklore/service/watcher/LibraryFileEventProcessor.javabooklore-api/src/test/java/org/booklore/service/library/FileAsBookProcessorTest.javabooklore-api/src/main/java/org/booklore/service/bookdrop/BookDropService.javabooklore-api/src/main/java/org/booklore/service/library/LibraryService.javabooklore-api/src/main/java/org/booklore/service/library/BookGroupProcessor.javabooklore-api/src/main/java/org/booklore/service/library/LibraryScanListener.javabooklore-api/src/main/java/org/booklore/service/library/BookCoverGenerator.javabooklore-api/src/test/java/org/booklore/service/bookdrop/BookDropServiceTest.javabooklore-api/src/main/java/org/booklore/service/library/FileAsBookProcessor.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/library/LibraryProcessingServiceRegressionTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/watcher/LibraryFileEventProcessorTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryServiceIconTest.javabooklore-api/src/test/java/org/booklore/service/library/FileAsBookProcessorTest.javabooklore-api/src/test/java/org/booklore/service/bookdrop/BookDropServiceTest.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/**/*.java : Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce `Autowired` field injection
Applied to files:
booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceRegressionTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/watcher/LibraryFileEventProcessorTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryServiceIconTest.javabooklore-api/src/main/java/org/booklore/service/watcher/LibraryFileEventProcessor.javabooklore-api/src/main/java/org/booklore/service/library/BookCoverGenerator.javabooklore-api/src/test/java/org/booklore/service/bookdrop/BookDropServiceTest.javabooklore-api/src/main/java/org/booklore/service/library/FileAsBookProcessor.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/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/library/LibraryProcessingServiceRegressionTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/watcher/LibraryFileEventProcessorTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryServiceIconTest.javabooklore-api/src/test/java/org/booklore/service/library/FileAsBookProcessorTest.javabooklore-api/src/test/java/org/booklore/service/bookdrop/BookDropServiceTest.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/library/FileAsBookProcessor.java
🔇 Additional comments (14)
booklore-api/src/main/java/org/booklore/service/event/BookAddedEvent.java (1)
11-12: Clean event contract for post-commit processing.The record-based event payload is concise and appropriate for the new event-driven flow.
booklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.java (1)
47-47: Good extraction point for cover handling.Delegating additional-file cover generation to
BookCoverGeneratorkeepsLibraryProcessingServicefocused on scan/attach orchestration.Also applies to: 218-218
booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceRegressionTest.java (1)
49-50: Test wiring update is correct.The regression test constructor setup now matches the updated
LibraryProcessingServicedependency list.Also applies to: 57-68
booklore-api/src/test/java/org/booklore/service/library/LibraryServiceIconTest.java (1)
29-30: Good fixture alignment with event-publisher dependency.Adding the
ApplicationEventPublishermock keepsLibraryServiceconstructor injection complete in this test suite.Also applies to: 67-68
booklore-api/src/main/java/org/booklore/service/bookdrop/BookDropService.java (1)
30-31: Refactoring from direct service call to event publishing looks correct.The transition from
KoboAutoShelfServicetoApplicationEventPublisherproperly decouples the side effects (notification broadcasting and Kobo shelf updates) from the main transaction. TheBookAddedEventis published with theBookDTO which contains the necessaryidfield for downstream processing.The event will be handled asynchronously after the transaction commits via
BookAddedEventListener, which aligns well with the PR objectives.Also applies to: 37-38, 78-78, 488-488
booklore-api/src/main/java/org/booklore/service/watcher/LibraryFileEventProcessor.java (1)
74-78: Scan guard correctly prevents event handling during active library scans.The early exit when
libraryScanListener.isScanning(libraryId)returnstrueis a sensible optimization to prevent redundant processing while a full scan is in progress. The DEBUG log level is appropriate for this skip condition.Note: There's a potential narrow race window where an event could be enqueued just before a scan starts and then processed during the scan. However, this is acceptable since the scan will handle the file anyway, and this guard primarily prevents a burst of events from being queued when a scan is already running.
booklore-api/src/main/java/org/booklore/config/TaskExecutorConfig.java (1)
17-22: Security context propagation and virtual thread executor configuration looks correct.The
DelegatingSecurityContextExecutorwrapper ensures that theSecurityContextis properly propagated to async tasks, which is essential for maintaining authentication state in event handlers likeBookAddedEventListener. TheExecutors.newVirtualThreadPerTaskExecutor()is well-suited for I/O-bound async operations.The return type change from
AsyncTaskExecutortoExecutoris compatible with existing usages:
TaskServiceinjects asExecutortype@Async("taskExecutor")annotations resolve by bean name, not typebooklore-api/src/main/java/org/booklore/service/event/BookAddedEventListener.java (1)
25-37: Event listener implementation is well-structured with proper error handling.The combination of
@Async("taskExecutor")with@TransactionalEventListener(phase = AFTER_COMMIT, fallbackExecution = true)correctly ensures:
- Execution happens after the publishing transaction commits
fallbackExecution = truehandles cases where no transaction exists- Processing runs on a separate thread with security context propagation
The try-catch block appropriately swallows exceptions since these are side effects that shouldn't fail the main operation. Using the
BookDTO (not entity) avoidsLazyInitializationExceptionrisks in the async context.booklore-api/src/test/java/org/booklore/service/library/FileAsBookProcessorTest.java (1)
23-146: Test refactoring aligns well with the new delegation architecture.The tests are correctly refactored to focus on
FileAsBookProcessor's responsibility as a thin delegator that groups files and delegates toBookGroupProcessor. Key coverage includes:
- Different base names → separate groups
- Empty input → no processing
- Different directories → separate groups
- Pre-grouped input → direct delegation
This follows the guideline to prefer focused unit tests without
@SpringBootTest.booklore-api/src/main/java/org/booklore/service/library/LibraryService.java (1)
185-191: Event-driven scan request simplifies the control flow.The refactoring from direct
TransactionSynchronizationManagerhooks to publishingLibraryScanRequestedEventis cleaner. The booleanfullRescanflag correctly distinguishes between:
true: Full rescan triggered by user actionfalse: Background scan after library creation/path additionNote that
rescanLibrary()is not annotated with@Transactional, so the event is published immediately andfallbackExecution = trueon the listener ensures it still fires. This behavior appears intentional for user-triggered rescans.booklore-api/src/main/java/org/booklore/service/library/BookGroupProcessor.java (1)
47-91: Transaction isolation withREQUIRES_NEWprovides good fault tolerance for batch processing.Each book group is processed in its own transaction, so failures in one group don't affect others. The
BookAddedEventis published within this transaction and will be delivered after the inner transaction commits (due to@TransactionalEventListener(phase = AFTER_COMMIT)), which is correct.booklore-api/src/main/java/org/booklore/service/library/LibraryScanListener.java (2)
39-42: Correct atomic deduplication pattern.Using
add()onConcurrentHashMap.newKeySet()is the right approach—it atomically checks and adds in one operation, returningfalseif already present. This avoids TOCTOU race conditions that would occur with separatecontains()/add()calls.
34-35:fallbackExecution = trueis the correct choice for the actual usage pattern.The event is published from both transactional (
createLibrary()) and non-transactional (rescanLibrary()) contexts. SettingfallbackExecution = trueensures the handler executes in both cases—synchronously when no transaction is active. Removing this would silently drop events published fromrescanLibrary().booklore-api/src/main/java/org/booklore/service/library/FileAsBookProcessor.java (1)
17-22: Clean delegator pattern.Good refactoring—the class now focuses on orchestration (grouping and iteration) while delegating transactional work to
BookGroupProcessor. Single dependency with constructor injection follows project conventions.
…ook cover information
# Conflicts: # booklore-api/src/main/java/org/booklore/BookloreApplication.java
# Conflicts: # booklore-api/src/main/java/org/booklore/service/bookdrop/BookDropService.java # booklore-api/src/test/java/org/booklore/service/bookdrop/BookDropServiceTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
booklore-api/src/main/java/org/booklore/service/library/LibraryService.java (1)
323-324: Rename this helper to match its actual guarantee.This method publishes immediately; the after-commit behavior now lives in
LibraryScanListener.handle.scheduleBackgroundScanAfterCommitreads stronger than what the method itself enforces, which makes future non-transactional call sites easy to misread.🤖 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/library/LibraryService.java` around lines 323 - 324, Rename the helper method scheduleBackgroundScanAfterCommit to reflect that it publishes the event immediately (e.g., publishLibraryScanRequest or publishImmediateLibraryScan), update all callers to use the new name, and keep the body that does eventPublisher.publishEvent(new LibraryScanRequestedEvent(libraryId, false)); also add a javadoc/comment on the renamed method clarifying that after-commit semantics are enforced elsewhere (LibraryScanListener.handle) rather than by this method.
🤖 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/library/LibraryService.java`:
- Around line 195-197: The current code audits LIBRARY_SCANNED immediately
before queuing the async work (auditService.log(...,
AuditAction.LIBRARY_SCANNED)), which over-reports successes because
eventPublisher.publishEvent(new LibraryScanRequestedEvent(libraryId, true)) may
be skipped or fail later; change this to audit a "scan requested" action (e.g.,
AuditAction.LIBRARY_SCAN_REQUESTED or a new enum value) here, or remove the
success audit and move the LIBRARY_SCANNED audit into LibraryScanListener.handle
where the scan actually completes successfully (update the listener to call
auditService.log(AuditAction.LIBRARY_SCANNED, ... ) upon confirmed completion).
---
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/service/library/LibraryService.java`:
- Around line 323-324: Rename the helper method
scheduleBackgroundScanAfterCommit to reflect that it publishes the event
immediately (e.g., publishLibraryScanRequest or publishImmediateLibraryScan),
update all callers to use the new name, and keep the body that does
eventPublisher.publishEvent(new LibraryScanRequestedEvent(libraryId, false));
also add a javadoc/comment on the renamed method clarifying that after-commit
semantics are enforced elsewhere (LibraryScanListener.handle) rather than by
this method.
🪄 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: cd8903df-0865-4513-b6a3-4901a801650e
📒 Files selected for processing (8)
booklore-api/src/main/java/org/booklore/service/bookdrop/BookDropService.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/main/java/org/booklore/service/library/LibraryService.javabooklore-api/src/main/java/org/booklore/service/watcher/LibraryFileEventProcessor.javabooklore-api/src/test/java/org/booklore/service/bookdrop/BookDropServiceTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceRegressionTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/watcher/LibraryFileEventProcessorTest.java
🚧 Files skipped from review as they are similar to previous changes (5)
- booklore-api/src/main/java/org/booklore/service/watcher/LibraryFileEventProcessor.java
- booklore-api/src/test/java/org/booklore/service/bookdrop/BookDropServiceTest.java
- booklore-api/src/test/java/org/booklore/service/watcher/LibraryFileEventProcessorTest.java
- booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.java
- booklore-api/src/main/java/org/booklore/service/bookdrop/BookDropService.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 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/library/LibraryProcessingServiceRegressionTest.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/main/java/org/booklore/service/library/LibraryService.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/library/LibraryProcessingServiceRegressionTest.java
🧠 Learnings (1)
📚 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 : Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce `Autowired` field injection
Applied to files:
booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceRegressionTest.java
🔇 Additional comments (4)
booklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.java (2)
49-49: Constructor-injected dependency addition is aligned with project DI style.
BookCoverGeneratoris added as afinalcollaborator and fits the existing Lombok constructor injection pattern cleanly.As per coding guidelines, "Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce
@Autowiredfield injection".
226-226: Cover generation delegation is a clean responsibility split.Switching to
bookCoverGenerator.generateCoverFromAdditionalFile(...)is consistent with the refactor and keeps cover logic in the dedicated service.booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceRegressionTest.java (1)
52-53: Test constructor wiring update looks correct.The new
BookCoverGeneratormock is properly introduced and passed toLibraryProcessingService, keeping the regression test setup in sync with production dependencies.Also applies to: 70-70
booklore-api/src/main/java/org/booklore/service/library/LibraryService.java (1)
29-29: Good decoupling point.Injecting
ApplicationEventPublisherhere keeps the scan trigger separated from the actual scan execution path instead of pulling that work back intoLibraryService.Also applies to: 63-63
zachyale
left a comment
zachyale
left a comment
There was a problem hiding this comment.
On a surface level nothing crazy, but let's hold off until we get the 3rd party contributor stuff in so we don't mess with their PRs
Right now, scanning in a library or any event that requires these book events to work well... Simply dont. They are blocking each other, and you have hard refresh for anything... What 3rd contributor stuff is related to this? |
# Conflicts: # booklore-api/src/main/java/org/booklore/config/TaskExecutorConfig.java # booklore-api/src/main/java/org/booklore/service/library/LibraryService.java # booklore-api/src/test/java/org/booklore/service/library/LibraryServiceIconTest.java
zachyale
left a comment
zachyale
left a comment
There was a problem hiding this comment.
There may be value in making follow up issues re: the two points but they aren't blockers
2 participants
Description
Linked Issue: Fixes #
Changes
This pull request introduces improvements to the book processing workflow, focusing on event-driven architecture, asynchronous processing, and enhanced cover image handling. The main changes include refactoring how book addition side effects are managed, introducing new services for cover generation and book group processing, and updating the asynchronous execution configuration.
Event-driven book addition and asynchronous processing:
KoboAutoShelfServiceand notification broadcasting inBookDropServicewith the publishing of a newBookAddedEvent. This decouples side effects from the main transaction and enables more modular handling of post-book-creation actionsBookAddedEventrecord and theBookAddedEventListenercomponent, which asynchronously handles book addition events after transaction commit. The listener broadcasts notifications and updates Kobo shelves, ensuring these actions are performed outside the main transaction for better reliability and scalability.Book file and cover management enhancements:
BookCoverGeneratorservice, which generates and attaches cover images to books from additional files, supporting both audiobooks and ebooks. This service ensures that cover images are extracted and saved appropriately when new formats are added to a book.BookGroupProcessorservice, which processes groups of book files in their own transactions. It selects the primary file, processes additional formats, attaches them to the book, and triggers theBookAddedEvent. This improves robustness and maintainability of the book import workflow.Asynchronous execution configuration updates:
@EnableAsyncfrom the main application class to a dedicated configuration class. The task executor is now aDelegatingSecurityContextExecutorwrapping a virtual thread executor, ensuring proper security context propagation for async tasks.Summary by CodeRabbit
New Features
Improvements
Refactor
Tests