fix: properly handle single-file audiobooks#540
fix: properly handle single-file audiobooks#540imnotjames wants to merge 1 commit intogrimmory-tools:developfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request refactors LibraryFileHelper's file detection logic by replacing string-key-based matching with Path-equality comparisons and set operations. This includes reworking three core detection methods and simplifying folder-based audiobook handling in directory traversal to fix issues with audiobook files disappearing from the application. Changes
Sequence Diagram(s)No sequence diagram is applicable for this change. The modifications are primarily internal refactoring and bug fixes to file-detection algorithms rather than changes introducing new inter-component interactions or altering control flow in ways that would benefit from visualization. Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Clarifying the problem here as a commit for future review: For example, say I have Author "Foo" who made the audiobook "Bar" and "Baz"
The problem is that when checking for book files during a scan, Grimmory erroneously detects The issue why we can't just add all files is then the individual files in an actual folder-based audiobook will get detected as new books, so we have to handle that case. I also think there are other edge cases that this entire system doesn't handle - it was never written to support audiobook folders in the first place, so cases like an audiobook folder that has a single cover jpeg may break things. A tree or moving the folder detection logic up into the processor makes more sense - but would be a larger rewrite of the feature. Actually, moving in the other direction could work, too? The delete / add detection helpers moved into the library file helper & making them public would allow testing them more directly. |
imnotjames
force-pushed
the
fix/446/mp3-single-file-audiobooks-removed
branch
2 times, most recently
from
1601e61 to
3a18572
Compare
|
agreed on both counts. Some of the weirdness sadly, is rather obvious. For instance, when you select audiobooks in book-view, and generate custom cover nothing happens, you also cannot also regenerate from the file. Whether auto-metadata match works or not, i think so but 🤷♂️ Interestingly, when you do it in edit metadata UI... then it works? Anyways, this is to say, PRs there are very much welcome. For me it has not been priority because it's moreso an "opt-in" features with lot of great alternatives. E.g., Audiobookshelf. Thank you for your work :) |
imnotjames
force-pushed
the
fix/446/mp3-single-file-audiobooks-removed
branch
4 times, most recently
from
de9cc2d to
3d3d3ca
Compare
|
This is a bit much of a PR to review, probably - It's a refactor and then 1631578 is the actual fix. If need be, I can split this into two PRs -- one to refactor with no behavior changes, and one to fix. EDIT: Per DM, splitting would help. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java (1)
299-341:⚠️ Potential issue | 🟠 MajorFolder-based audiobooks not deduplicated in AUTO_DETECT mode — individual files and folder entry create separate books on initial scan.
With AUTO_DETECT,
groupByBaseName()routes folder-based audiobooks through folder-centric logic that handles folder-name matching but does not merge individual chapter files with their folder-based entry. Individual MP3 files (e.g., "Chapter 1.mp3", "Chapter 2.mp3") are detected as series entries and remain separate, while the folder-based entry (e.g.,folderBased=true, fileName="Harry Potter 1-7 Series") groups independently when the folder name matches. Unmatched chapter files are clustered separately, resulting in N+1 distinct books instead of 1.Test coverage exists for ebook + folder-based entry grouping, but not for the scenario where individual audio files coexist with a folder-based entry in the same physical folder. Either skip individual audio files in
LibraryFileHelper.findLibraryFiles()when a folder-based entry is created, or add deduplication logic ingroupByBaseName()to absorb individual chapter files under their parent folder-based entry.🤖 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/LibraryFileHelper.java` around lines 299 - 341, The scan is adding both a folder-based audiobook entry and the individual audio files from that same directory, causing duplicate books; in LibraryFileHelper.findLibraryFiles() (the FileVisitor implementation that builds libraryFiles using dirAudioFiles/dirHasNonAudioBooks and creates a folder-based LibraryFile with BookFileType.AUDIOBOOK and folderBased=true), change the logic so that when you create a folder-based entry for dir you also remove or skip any previously-added individual audio file LibraryFile entries that live under that dir (compare their fileSubPath or FileUtils.getRelativeSubPath(pathEntity.getPath(), file) against the dir path) or mark them ignored so they aren’t returned; alternatively, implement the deduplication in groupByBaseName() to merge individual chapter-file entries into the folder-based LibraryFile by matching the folder-based LibraryFile.folderBased flag and the parent directory, but prefer removing/skipping per-dir individual files immediately when you add the folder-based entry to keep findLibraryFiles() consistent.booklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.java (1)
101-121:⚠️ Potential issue | 🟡 Minor
allAdditionalFileslist remains detached afterentityManager.clear()— accessing lazy associations may fail at runtime.After line 101 loads
allAdditionalFiles, theentityManager.clear()at line 115 detaches all entities. WhendetectNewBookPaths()is called on line 121, it mapsBookFileEntity::getFullFilePathacross the detached list (lines 74, 80, 88, 94 in LibraryFileHelper). This method accessesbook.getLibraryPath().getPath()— both lazy-loaded associations that were never initialized before detach. Accessing uninitialized lazy associations on detached entities will raiseLazyInitializationExceptionor trigger unpredictable behavior depending on Hibernate configuration.Additionally, any additional files deleted via
bookDeletionService.deleteRemovedAdditionalFiles()on line 106 remain in the in-memory list, causingdetectNewBookPaths()to treat their paths as "existing" and suppress legitimate new files.Re-query
allAdditionalFilesafterentityManager.clear()and the deletion step.🤖 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/LibraryProcessingService.java` around lines 101 - 121, The loaded allAdditionalFiles list is detached by entityManager.clear(), so re-query the repository after the clear (and after calling bookDeletionService.deleteRemovedAdditionalFiles) and pass that fresh list into libraryFileHelper.detectNewBookPaths; specifically, replace use of the original allAdditionalFiles variable (populated by bookAdditionalFileRepository.findByLibraryId) with a new call to bookAdditionalFileRepository.findByLibraryId(libraryId) after entityManager.clear() (and after deletion) so detectNewBookPaths (and its BookFileEntity::getFullFilePath / getLibraryPath accesses) operate on managed entities and reflect removed additional files.
🧹 Nitpick comments (2)
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java (1)
108-116: Nit: misleading comment in the folder-based branch.Inside the
file.isFolderBased()branch, the loop checks whether existing files are contained by the current folder (existingFile.startsWith(filePath)), not whether folders contain this file. The comment on Line 111 duplicates the non-folder branch's intent and contradicts the (correct but dangling) comment on Lines 108–109.✏️ Suggested tidy-up
- // Next check if there are any book existing files which are - // contained by this folder. - - // Next check if there are any book folders which contain this file. + // Next, check if any existing (non-folder) book files live inside this folder. for (Path existingFile : existingFilePaths) { if (existingFile.startsWith(filePath)) { return false; } }🤖 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/LibraryFileHelper.java` around lines 108 - 116, The comment in the file.isFolderBased() branch of LibraryFileHelper is misleading: the loop over existingFilePaths checks whether existing files are contained by the current folder (existingFile.startsWith(filePath)), not whether folders contain this file; update the comment near existingFilePaths / filePath (in the file.isFolderBased() branch) to accurately state that we are checking for any existing files inside the folder represented by filePath and return false if any are found, or remove the duplicated incorrect comment altogether.booklore-api/src/test/java/org/booklore/service/library/LibraryFileHelperTest.java (1)
514-538: Consider a test for the initial-scan "folder + member files present together, no existing books" case.The coverage for
detectNewBookPathsis solid, but there's no test where the input contains both a folder-based entry and its member audio files (asfindLibraryFilesnow produces in AUTO_DETECT mode) withbooksandallAdditionalFilesboth empty. That scenario is exactly what happens on initial scan of a real folder-based audiobook, and the current behavior is to return all of them as "new" — relying onBookGroupingService.groupForInitialScanto deduplicate. A pinning test would make that contract explicit.🤖 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/LibraryFileHelperTest.java` around lines 514 - 538, Add a test in LibraryFileHelperTest that covers the initial-scan scenario where a folder-type LibraryFile and its member files are present together and both books and allAdditionalFiles are empty: construct a LibraryPathEntity, create a folder LibraryFile (e.g., "a" folder) plus its member files (e.g., "a/track1.mp3", "a/track2.mp3"), call libraryFileHelper.detectNewBookPaths(...) with List.of(folder, members) and empty lists for books and allAdditionalFiles, and assert the result equals the input list (relying on BookGroupingService.groupForInitialScan to dedupe later); reference detectNewBookPaths, LibraryFile, LibraryPathEntity, and LibraryFileHelperTest when adding the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java`:
- Around line 299-341: The scan is adding both a folder-based audiobook entry
and the individual audio files from that same directory, causing duplicate
books; in LibraryFileHelper.findLibraryFiles() (the FileVisitor implementation
that builds libraryFiles using dirAudioFiles/dirHasNonAudioBooks and creates a
folder-based LibraryFile with BookFileType.AUDIOBOOK and folderBased=true),
change the logic so that when you create a folder-based entry for dir you also
remove or skip any previously-added individual audio file LibraryFile entries
that live under that dir (compare their fileSubPath or
FileUtils.getRelativeSubPath(pathEntity.getPath(), file) against the dir path)
or mark them ignored so they aren’t returned; alternatively, implement the
deduplication in groupByBaseName() to merge individual chapter-file entries into
the folder-based LibraryFile by matching the folder-based
LibraryFile.folderBased flag and the parent directory, but prefer
removing/skipping per-dir individual files immediately when you add the
folder-based entry to keep findLibraryFiles() consistent.
In
`@booklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.java`:
- Around line 101-121: The loaded allAdditionalFiles list is detached by
entityManager.clear(), so re-query the repository after the clear (and after
calling bookDeletionService.deleteRemovedAdditionalFiles) and pass that fresh
list into libraryFileHelper.detectNewBookPaths; specifically, replace use of the
original allAdditionalFiles variable (populated by
bookAdditionalFileRepository.findByLibraryId) with a new call to
bookAdditionalFileRepository.findByLibraryId(libraryId) after
entityManager.clear() (and after deletion) so detectNewBookPaths (and its
BookFileEntity::getFullFilePath / getLibraryPath accesses) operate on managed
entities and reflect removed additional files.
---
Nitpick comments:
In
`@booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java`:
- Around line 108-116: The comment in the file.isFolderBased() branch of
LibraryFileHelper is misleading: the loop over existingFilePaths checks whether
existing files are contained by the current folder
(existingFile.startsWith(filePath)), not whether folders contain this file;
update the comment near existingFilePaths / filePath (in the
file.isFolderBased() branch) to accurately state that we are checking for any
existing files inside the folder represented by filePath and return false if any
are found, or remove the duplicated incorrect comment altogether.
In
`@booklore-api/src/test/java/org/booklore/service/library/LibraryFileHelperTest.java`:
- Around line 514-538: Add a test in LibraryFileHelperTest that covers the
initial-scan scenario where a folder-type LibraryFile and its member files are
present together and both books and allAdditionalFiles are empty: construct a
LibraryPathEntity, create a folder LibraryFile (e.g., "a" folder) plus its
member files (e.g., "a/track1.mp3", "a/track2.mp3"), call
libraryFileHelper.detectNewBookPaths(...) with List.of(folder, members) and
empty lists for books and allAdditionalFiles, and assert the result equals the
input list (relying on BookGroupingService.groupForInitialScan to dedupe later);
reference detectNewBookPaths, LibraryFile, LibraryPathEntity, and
LibraryFileHelperTest when adding the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7056444f-b7fe-46a1-bf53-02171bd189af
📒 Files selected for processing (4)
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/test/java/org/booklore/service/library/LibraryFileHelperTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.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/library/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryFileHelperTest.javabooklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.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/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryFileHelperTest.java
🧠 Learnings (8)
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryFileHelperTest.javabooklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).
Applied to files:
booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryFileHelperTest.javabooklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.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 : 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/LibraryFileHelperTest.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/LibraryFileHelperTest.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/**/*Entity.java : Keep JPA entities on the `*Entity` suffix
Applied to files:
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java
📚 Learning: 2026-04-13T05:02:01.463Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 486
File: booklore-api/src/main/java/org/booklore/service/oidc/OidcGroupMappingService.java:130-159
Timestamp: 2026-04-13T05:02:01.463Z
Learning: In grimmory-tools/grimmory, treat `permissionDemoUser`/`permission_demo_user` as a restricting “limited demo mode” flag: it must never be set to `true` as part of any “grant all permissions” or admin-grant flow. Specifically, in code paths that grant admin permissions (for example the OIDC `isAdmin` branch in `OidcGroupMappingService.applyPermissions`), ensure `permissionDemoUser` is intentionally excluded from the admin-permission block and cannot be turned on; enabling it for admin users is counterproductive.
Applied to files:
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.java
📚 Learning: 2026-04-14T12:46:56.673Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:46:56.673Z
Learning: For this project (grimmory-tools/grimmory / booklore-api), prefer modern Java 25 idioms and language/library features when reviewing or suggesting changes. Favor current constructs such as unnamed variables (e.g., `_`) where applicable, records, pattern matching for `switch`/`instanceof`, sealed classes, sequenced collections, `Stream.toList()`, and text blocks. Avoid proposing regressions to older/legacy equivalents unless there’s a concrete compatibility constraint (e.g., a required older runtime/library behavior).
Applied to files:
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.java
📚 Learning: 2026-03-23T18:36:05.843Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 146
File: booklore-api/src/main/java/org/booklore/util/FileService.java:151-168
Timestamp: 2026-03-23T18:36:05.843Z
Learning: In `booklore-api/src/main/java/org/booklore/util/FileService.java`, the `"bin"` search path in `getSystemSearchPath()` is intentionally left as a relative path (not derived from `appProperties.getPathConfig()`), so binary lookup resolves relative to the process working directory. Do not flag this as an issue.
Applied to files:
booklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.java
🔇 Additional comments (3)
booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.java (1)
116-116: LGTM — Mockito stubs correctly align with the updatedLibraryFileHelperAPI.
detectNewBookPaths(any(), any(), any())matches the three-parameter signature(libraryFiles, books, allAdditionalFiles)anddetectDeletedAdditionalFiles(any(), any())matches(libraryFiles, allAdditionalFiles), as called fromLibraryProcessingService(lines 62 and 103).Also applies to: 212-212, 255-255, 376-376
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java (1)
39-67: LGTM on the two deletion helpers.
detectDeletedBookIdsproperly skips deleted books and fileless books before comparinggetFullFilePath, anddetectDeletedAdditionalFilescorrectly filters byisBookFormatso non-format additional files (covers, etc.) aren't mistakenly scheduled for deletion. Path-based comparison replaces the prior string-key approach, which also removes the/book1vs/book10false-match class of bugs.booklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.java (1)
60-62: LGTM on delegatingprocessLibraryto the helper.Pulling
allAdditionalFilesand passing them intodetectNewBookPathscloses the hole where an additional file's path would be re-detected as a new book on subsequent scans.
imnotjames
force-pushed
the
fix/446/mp3-single-file-audiobooks-removed
branch
3 times, most recently
from
2719a3e to
a219394
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/LibraryFileHelper.java`:
- Around line 102-119: The folder-based candidate check in LibraryFileHelper
currently only rejects exact folder matches and child file paths but misses the
case where the candidate folder is nested under an existing folder-based book;
inside the file.isFolderBased() branch (the block using existingFolderPaths,
existingFilePaths and filePath) add an additional check that returns false if
filePath is located inside any existing folder path (e.g., if
filePath.startsWith(existingFolderPath) for any entry in existingFolderPaths),
and also ensure you still reject candidates when any existing folder is inside
the candidate (existingFolderPath.startsWith(filePath)) to avoid duplicates in
both directions.
- Around line 299-305: findLibraryFiles is currently emitting both individual
audio files and a folder-based audiobook entry, causing duplicate imports on
initial scans; update the pipeline to suppress child-file LibraryFile entries
when a folder-based candidate exists for the same directory. Specifically, in
either findLibraryFiles or detectNewBookPaths (or as a post-step in
groupByFolder/groupForInitialScan) detect folderBased=true entries, reconstruct
their directory path (using FileUtils.getRelativeSubPath/pathEntity.getPath
logic) and filter out any LibraryFile whose parent directory equals that
reconstructed folder path; ensure this check runs during initial scan grouping
so existingFolderPaths being empty doesn't allow child files through.
🪄 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: 4f423eed-4f8c-4a18-b85e-638931950cce
📒 Files selected for processing (1)
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.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/library/LibraryFileHelper.java
🧠 Learnings (5)
📚 Learning: 2026-03-23T18:36:05.843Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 146
File: booklore-api/src/main/java/org/booklore/util/FileService.java:151-168
Timestamp: 2026-03-23T18:36:05.843Z
Learning: In `booklore-api/src/main/java/org/booklore/util/FileService.java`, the `"bin"` search path in `getSystemSearchPath()` is intentionally left as a relative path (not derived from `appProperties.getPathConfig()`), so binary lookup resolves relative to the process working directory. Do not flag this as an issue.
Applied to files:
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).
Applied to files:
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java
📚 Learning: 2026-04-13T05:02:01.463Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 486
File: booklore-api/src/main/java/org/booklore/service/oidc/OidcGroupMappingService.java:130-159
Timestamp: 2026-04-13T05:02:01.463Z
Learning: In grimmory-tools/grimmory, treat `permissionDemoUser`/`permission_demo_user` as a restricting “limited demo mode” flag: it must never be set to `true` as part of any “grant all permissions” or admin-grant flow. Specifically, in code paths that grant admin permissions (for example the OIDC `isAdmin` branch in `OidcGroupMappingService.applyPermissions`), ensure `permissionDemoUser` is intentionally excluded from the admin-permission block and cannot be turned on; enabling it for admin users is counterproductive.
Applied to files:
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java
📚 Learning: 2026-04-14T12:46:56.673Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:46:56.673Z
Learning: For this project (grimmory-tools/grimmory / booklore-api), prefer modern Java 25 idioms and language/library features when reviewing or suggesting changes. Favor current constructs such as unnamed variables (e.g., `_`) where applicable, records, pattern matching for `switch`/`instanceof`, sealed classes, sequenced collections, `Stream.toList()`, and text blocks. Avoid proposing regressions to older/legacy equivalents unless there’s a concrete compatibility constraint (e.g., a required older runtime/library behavior).
Applied to files:
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java
🔇 Additional comments (1)
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java (1)
57-66: Path-based matching looks good.This removes the string-key ambiguity and compares the scanned file path directly against the persisted additional-file path.
| if (file.isFolderBased()) { | ||
| // If there's an exact match for existing folder paths bail early. | ||
| if (existingFolderPaths.contains(filePath)) { | ||
| return false; | ||
| } | ||
|
|
||
| private String generateUniqueKey(BookFileEntity file) { | ||
| return generateKey(file.getBook().getLibraryPath().getId(), file.getFileSubPath(), file.getFileName()); | ||
| } | ||
| // Next check if there are any book existing files which are | ||
| // contained by this folder. | ||
|
|
||
| private String generateUniqueKey(LibraryFile file) { | ||
| return generateKey(file.getLibraryPathEntity().getId(), file.getFileSubPath(), file.getFileName()); | ||
| } | ||
| // Next check if there are any book folders which contain this file. | ||
| for (Path existingFile : existingFilePaths) { | ||
| if (existingFile.startsWith(filePath)) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // If none of that matches then this is a new folder. | ||
| return true; |
There was a problem hiding this comment.
Suppress nested folder candidates under existing folder-based books.
Line 102 only checks exact folder matches and child file paths. If a scanned folder-based candidate is inside an existing folder-based audiobook, it can still be returned as “new” and create a duplicate/nested book entry.
Proposed fix
if (file.isFolderBased()) {
// If there's an exact match for existing folder paths bail early.
if (existingFolderPaths.contains(filePath)) {
return false;
}
+ // If this folder is already covered by an existing folder-based book,
+ // don't surface it as a new book candidate.
+ for (Path existingFolder : existingFolderPaths) {
+ if (filePath.startsWith(existingFolder)) {
+ return false;
+ }
+ }
+
// Next check if there are any book existing files which are
// contained by this folder.
- // Next check if there are any book folders which contain this file.
for (Path existingFile : existingFilePaths) {
if (existingFile.startsWith(filePath)) {
return false;
}
}🤖 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/LibraryFileHelper.java`
around lines 102 - 119, The folder-based candidate check in LibraryFileHelper
currently only rejects exact folder matches and child file paths but misses the
case where the candidate folder is nested under an existing folder-based book;
inside the file.isFolderBased() branch (the block using existingFolderPaths,
existingFilePaths and filePath) add an additional check that returns false if
filePath is located inside any existing folder path (e.g., if
filePath.startsWith(existingFolderPath) for any entry in existingFolderPaths),
and also ensure you still reject candidates when any existing folder is inside
the candidate (existingFolderPath.startsWith(filePath)) to avoid duplicates in
both directions.
| libraryFiles.add(LibraryFile.builder() | ||
| .libraryEntity(libraryEntity) | ||
| .libraryPathEntity(pathEntity) | ||
| .fileSubPath(FileUtils.getRelativeSubPath(pathEntity.getPath(), file)) | ||
| .fileName(fileName) | ||
| .bookFileType(fileType) | ||
| .build()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i '^BookGroupingService\.java$' -x sh -c '
echo "=== $1 ==="
sed -n "1,260p" "$1"
' sh {}
echo
echo "=== Relevant grouping references ==="
rg -nP -C4 '\bgroupForInitialScan\s*\(|\bisFolderBased\s*\(|\bgetFullPath\s*\(|\bstartsWith\s*\(' --type=javaRepository: grimmory-tools/grimmory
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the complete groupByFolder method
echo "=== Complete groupByFolder method ==="
rg -A 60 'private Map<String, List<LibraryFile>> groupByFolder' --type=java | head -80
echo
echo "=== LibraryFileHelper lines 299-336 (full context) ==="
sed -n '290,340p' booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java
echo
echo "=== Tests for grouping with audiobooks ==="
fd -name '*BookGrouping*Test.java' -o -name '*LibraryFileHelper*Test.java' | xargs rg -l 'groupForInitialScan|folder.*audio|folderBased' 2>/dev/null || trueRepository: grimmory-tools/grimmory
Length of output: 14875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for tests that verify the duplicate handling
echo "=== Test cases for grouping initial scans with audio files ==="
rg -B 3 -A 15 'folderBased.*true.*AUDIOBOOK|AUDIOBOOK.*folderBased.*true' booklore-api/src/test --type=java | head -100
echo
echo "=== Check if there's logic to filter child files when folder-based exists ==="
rg 'child|covered|absorb|filter.*folderBased|folderBased.*filter' booklore-api/src/main/java/org/booklore/service/library --type=java -iRepository: grimmory-tools/grimmory
Length of output: 129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== BookGroupingServiceTest cases for folder audiobooks ==="
sed -n '1,500p' booklore-api/src/test/java/org/booklore/service/library/BookGroupingServiceTest.java | grep -A 20 -B 3 'folderBased\|AUDIOBOOK\|folder.*audio'
echo
echo "=== Search for filtering logic that removes individual files ==="
rg 'filter.*folderBased|folderBased.*filter|child.*file.*filter|removeIf.*folder|stream.*filter.*folder' booklore-api/src/main/java --type=java -i -C 3Repository: grimmory-tools/grimmory
Length of output: 16686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== LibraryProcessingService - check processLibraryFiles ==="
rg -B 5 -A 30 'processLibraryFiles|processNewFiles' booklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.java | head -100
echo
echo "=== BookGroupProcessor - check grouping result handling ==="
rg -B 5 -A 20 'newBookGroups|filesToAttach' booklore-api/src/main/java/org/booklore/service/library/BookGroupProcessor.java | head -120Repository: grimmory-tools/grimmory
Length of output: 4596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check detectNewBookPaths - does it filter? ==="
rg -B 5 -A 40 'detectNewBookPaths' booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java | head -80
echo
echo "=== Search for any filtering of child files when folderBased exists ==="
rg 'folderBased.*startsWith|startsWith.*folderBased|child.*folder|parent.*folder' booklore-api/src/main/java --type=java -i -C 2Repository: grimmory-tools/grimmory
Length of output: 7240
🏁 Script executed:
#!/bin/bash
set -euo zipfile pipefail
# Look for a test case that has BOTH individual audiobook files AND folder-based in same dir
echo "=== Searching for test with mixed individual + folder-based audiobooks ==="
grep -r "audiobook.*folderBased\|folderBased.*audiobook" booklore-api/src/test/java --include="*.java" -B 3 -A 10 2>/dev/null | grep -A 15 "files = List.of" | head -60
echo
echo "=== Check if groupByFolder actually filters any files ==="
sed -n '74,150p' booklore-api/src/main/java/org/booklore/service/library/BookGroupingService.javaRepository: grimmory-tools/grimmory
Length of output: 119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Complete detectNewBookPaths method ==="
sed -n '262,330p' booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java
echo
echo "=== Test: look for scenario with both individual and folder-based audiobooks ==="
rg -B 5 -A 25 'audiobook.*audiobook\|mp3.*mp3' booklore-api/src/test/java/org/booklore/service/library/BookGroupingServiceTest.java | head -100Repository: grimmory-tools/grimmory
Length of output: 3727
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full detectNewBookPaths method (complete) ==="
sed -n '262,350p' booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java
echo
echo "=== Check if there's filtering of individual files contained in folder-based ==="
rg -A 50 'contained by this folder' booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.javaRepository: grimmory-tools/grimmory
Length of output: 6833
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check MIN_AUDIO_FILES_FOR_FOLDER_AUDIOBOOK constant ==="
rg 'MIN_AUDIO_FILES_FOR_FOLDER_AUDIOBOOK' booklore-api/src/main/java --type=java
echo
echo "=== Look for any filtering of child files in grouping or processing ==="
rg -B 3 -A 8 'filterChildFiles|removeChild|filterByParent|child.*filter' booklore-api/src/main/java --type=java
echo
echo "=== Test that verifies no duplicates with folder-based + individual files ==="
rg -B 5 -A 20 'folderBasedAudiobookWithChildren|individual.*folder\|folder.*individual' booklore-api/src/test --type=javaRepository: grimmory-tools/grimmory
Length of output: 26474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check BookGroupProcessor and FileAsBookProcessor ==="
rg -B 3 -A 20 'processLibraryFilesGrouped|processLibraryFiles' booklore-api/src/main/java/org/booklore/service/library/BookGroupProcessor.java | head -100
echo
echo "=== Test for mixed individual + folder-based scenario ==="
sed -n '1,800p' booklore-api/src/test/java/org/booklore/service/library/BookGroupingServiceTest.java | tail -400 | grep -B 5 -A 20 'individual\|mp3\|AUDIO' || echo "No test found for mixed scenario"Repository: grimmory-tools/grimmory
Length of output: 123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find FileAsBookProcessor ==="
find booklore-api/src -name '*FileAsBookProcessor*' -o -name '*BookGroupProcessor*'
echo
echo "=== Check if there's any filtering of files by parent folder ==="
rg 'startsWith.*folder\|folder.*startsWith' booklore-api/src/main/java --type=java -C 3 | head -60
echo
echo "=== Look for complete test scenario with both individual and folder audiobooks ==="
sed -n '300,600p' booklore-api/src/test/java/org/booklore/service/library/BookGroupingServiceTest.javaRepository: grimmory-tools/grimmory
Length of output: 411
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== FileAsBookProcessor - main processing logic ==="
sed -n '1,150p' booklore-api/src/main/java/org/booklore/service/library/FileAsBookProcessor.java
echo
echo "=== Check processLibraryFilesGrouped method ==="
rg -B 3 -A 30 'processLibraryFilesGrouped' booklore-api/src/main/java/org/booklore/service/library/FileAsBookProcessor.java
echo
echo "=== Test scenario: look for tests with multiple audio files in folder ==="
grep -n 'structure.*audio\|folderAudiobook.*audiobook\|audiobook.*folderAudiobook' booklore-api/src/test/java/org/booklore/service/library/BookGroupingServiceTest.java || echo "Pattern not found"Repository: grimmory-tools/grimmory
Length of output: 3162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Complete detectNewBookPaths filter logic for folderBased entries ==="
sed -n '262,320p' booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java
echo
echo "=== Check the comment about 'contained by this folder' ==="
sed -n '280,310p' booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java
echo
echo "=== Check if there's test verifying individual files filtered when folder-based exists ==="
rg -B 10 -A 15 'folderBased.*true' booklore-api/src/test/java/org/booklore/service/library/BookGroupingServiceTest.java | grep -A 20 'files = List.of' | head -50Repository: grimmory-tools/grimmory
Length of output: 4394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full detectNewBookPaths with filtering logic ==="
sed -n '262,330p' booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java
echo
echo "=== Search for any downstream deduplication ==="
rg 'deduplicate|collapse.*folder\|suppress.*child\|remove.*duplicate' booklore-api/src/main/java --type=javaRepository: grimmory-tools/grimmory
Length of output: 3879
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Get complete detectNewBookPaths method to verify filtering ==="
rg -A 70 'public List<LibraryFile> detectNewBookPaths' booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java | head -100Repository: grimmory-tools/grimmory
Length of output: 3139
Initial scans will import both individual audio files and their folder-based audiobook entry, creating duplicate books.
When findLibraryFiles detects a folder with sufficient audio files (≥2), it emits both:
- Each individual audio file as a separate
LibraryFile - A folder-based audiobook entry with
folderBased(true)
On fresh library scans, detectNewBookPaths cannot filter these because existingFolderPaths is empty. groupForInitialScan then groups them by path but does not suppress individual child files when a folder-based candidate covers them. Result: 10 individual MP3s + 1 folder audiobook = 11 separate book imports instead of 1.
Filter individual child audio files when a folder-based audiobook entry covers the same directory. Either suppress them in findLibraryFiles or detectNewBookPaths, or add filtering in groupByFolder to exclude child files whose parent directory matches a folder-based entry's reconstructed path.
🤖 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/LibraryFileHelper.java`
around lines 299 - 305, findLibraryFiles is currently emitting both individual
audio files and a folder-based audiobook entry, causing duplicate imports on
initial scans; update the pipeline to suppress child-file LibraryFile entries
when a folder-based candidate exists for the same directory. Specifically, in
either findLibraryFiles or detectNewBookPaths (or as a post-step in
groupByFolder/groupForInitialScan) detect folderBased=true entries, reconstruct
their directory path (using FileUtils.getRelativeSubPath/pathEntity.getPath
logic) and filter out any LibraryFile whose parent directory equals that
reconstructed folder path; ensure this check runs during initial scan grouping
so existingFolderPaths being empty doesn't allow child files through.
this previously failed because multiple mp3 files in a folder would always be detected as a folder-based audiobook and then the mp3 files would counted as "missing"
imnotjames
force-pushed
the
fix/446/mp3-single-file-audiobooks-removed
branch
from
a219394 to
21a659b
Compare
2 participants
Description
in some cases, single file audio books are being removed when the library scan is being done. this is because of ambiguity in our audiobook detection. often, multiple single file audiobooks gets detected as a folder-based audiobook which leads to us not finding those single files
this change updates the algorithm for detecting files so we check for both folder & file based audiobooks
Linked Issue: Fixes #446
Changes
Summary by CodeRabbit
Refactor
Tests