fix(filters): fix app file size filtering for books + make the f/e aware of the filter type#817
fix(filters): fix app file size filtering for books + make the f/e aware of the filter type#817
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
backend/src/test/java/org/booklore/app/specification/AppBookSpecificationTest.java (3)
183-210: Test name is slightly misleading — the fixture isn't "only supplementary".
createBook("Only Supplementary Match", 70_000L)still creates one book‑format file (70,000 KB). The scenario you're actually covering is "a supplementary file matches bucket 0 but the book-format file does not, so the book must be excluded from the OR result." Consider renaming the fixture variable (and possibly the test) to something likesupplementaryMatchesButBookFormatDoesNotto avoid confusion.Same note applies to the twin test at lines 212-239.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/org/booklore/app/specification/AppBookSpecificationTest.java` around lines 183 - 210, Rename the misleading fixture and test identifiers so they reflect that the supplementary file matches but the book-format file does not: change the BookEntity created by createBook("Only Supplementary Match", 70_000L) and the test method name to something like supplementaryMatchesButBookFormatDoesNot (or rename the local variable onlySupplementaryMatch to supplementaryMatchesButBookFormatDoesNot) and update the twin test similarly; keep the setup using BookFileEntity (isBookFormat=false) and the assertion against AppBookSpecification.withFileSizes unchanged.
280-293: Minor:bookTypevsfileNameextension mismatch in the helper.When
index >= 1you setbookType = PDFbut the generated filename still ends in.epub. It doesn't affectwithFileSizes(which only inspectsisBookFormatandfileSizeKb), but it makes the fixtures a bit confusing to read and would trip up anyone who later writes a test usingwithFileType/hasAudiobookFileon top ofcreateBook.♻️ Proposed tweak
- BookFileEntity file = BookFileEntity.builder() - .book(book) - .fileName(title.toLowerCase().replace(' ', '-') + "-" + index + ".epub") - .fileSubPath("") - .isBookFormat(true) - .bookType(index == 0 ? BookFileType.EPUB : BookFileType.PDF) + BookFileType type = index == 0 ? BookFileType.EPUB : BookFileType.PDF; + String ext = type == BookFileType.EPUB ? ".epub" : ".pdf"; + BookFileEntity file = BookFileEntity.builder() + .book(book) + .fileName(title.toLowerCase().replace(' ', '-') + "-" + index + ext) + .fileSubPath("") + .isBookFormat(true) + .bookType(type)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/org/booklore/app/specification/AppBookSpecificationTest.java` around lines 280 - 293, The generated test fixture filenames currently always use ".epub" even when bookType is set to PDF; update the helper loop that builds BookFileEntity (in AppBookSpecificationTest) so the fileName extension is derived from the chosen BookFileType (e.g., use ".epub" when bookType == BookFileType.EPUB and ".pdf" when BookFileType.PDF) rather than hardcoding ".epub", ensuring the filename generation logic in the BookFileEntity.builder() block matches the bookType selection.
34-54: Consider@DataJpaTestover full@SpringBootTestfor these specification tests.These tests only need JPA/EntityManager + a
BookRepository, but loadingBookloreApplication.classforces the whole application context to come up, which is why you need to disable Flyway, the bookdrop folder, scheduling, OIDC, mockTaskCronService, etc. Switching to@DataJpaTest(with the H2 +create-dropconfig) would give a much slimmer, faster slice and drop most of these@TestPropertySourceentries and theTestConfig. This would also make the tests more focused per the backend test guideline.As per coding guidelines: "Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required in backend code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/org/booklore/app/specification/AppBookSpecificationTest.java` around lines 34 - 54, The test class AppBookSpecificationTest currently uses `@SpringBootTest`(classes = BookloreApplication.class) and a large TestPropertySource/TestConfig to bring up the whole app; change it to use `@DataJpaTest` to load only JPA slice (EntityManager, BookRepository) so you can remove most TestPropertySource entries and the TestConfig; update imports/annotations on AppBookSpecificationTest (replace `@SpringBootTest` with `@DataJpaTest` and keep transactional behavior if needed), ensure H2 create-drop settings remain (or provide them via `@AutoConfigureTestDatabase` or test properties limited to datasource/h2), and remove mocks/configs that were only necessary for full context (e.g., TestConfig and any TaskCronService/OIDC/scheduling overrides).backend/src/main/java/org/booklore/app/specification/AppBookSpecification.java (1)
675-675: Nit:>= 0check on bucket 0 is redundant.
fileSizeKbcan't be negative in practice, so the lower bound on bucket 0 could be dropped to mirror e.g.withPageCountsbucket 0 (cb.lessThan(count, 50)). Not a blocker.♻️ Optional simplification
- case 0 -> cb.and(cb.greaterThanOrEqualTo(size, 0L), cb.lessThan(size, 1024L)); + case 0 -> cb.lessThan(size, 1024L);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/app/specification/AppBookSpecification.java` at line 675, The case 0 bucket in AppBookSpecification.java uses an unnecessary lower-bound check (cb.greaterThanOrEqualTo(size, 0L)) for fileSizeKb; remove that redundant condition and simplify the predicate returned in the switch arm for case 0 (the expression using variable size) to only enforce the upper bound (cb.lessThan(size, 1024L)), mirroring the simpler pattern used by withPageCounts's bucket 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend/src/main/java/org/booklore/app/specification/AppBookSpecification.java`:
- Line 675: The case 0 bucket in AppBookSpecification.java uses an unnecessary
lower-bound check (cb.greaterThanOrEqualTo(size, 0L)) for fileSizeKb; remove
that redundant condition and simplify the predicate returned in the switch arm
for case 0 (the expression using variable size) to only enforce the upper bound
(cb.lessThan(size, 1024L)), mirroring the simpler pattern used by
withPageCounts's bucket 0.
In
`@backend/src/test/java/org/booklore/app/specification/AppBookSpecificationTest.java`:
- Around line 183-210: Rename the misleading fixture and test identifiers so
they reflect that the supplementary file matches but the book-format file does
not: change the BookEntity created by createBook("Only Supplementary Match",
70_000L) and the test method name to something like
supplementaryMatchesButBookFormatDoesNot (or rename the local variable
onlySupplementaryMatch to supplementaryMatchesButBookFormatDoesNot) and update
the twin test similarly; keep the setup using BookFileEntity
(isBookFormat=false) and the assertion against
AppBookSpecification.withFileSizes unchanged.
- Around line 280-293: The generated test fixture filenames currently always use
".epub" even when bookType is set to PDF; update the helper loop that builds
BookFileEntity (in AppBookSpecificationTest) so the fileName extension is
derived from the chosen BookFileType (e.g., use ".epub" when bookType ==
BookFileType.EPUB and ".pdf" when BookFileType.PDF) rather than hardcoding
".epub", ensuring the filename generation logic in the BookFileEntity.builder()
block matches the bookType selection.
- Around line 34-54: The test class AppBookSpecificationTest currently uses
`@SpringBootTest`(classes = BookloreApplication.class) and a large
TestPropertySource/TestConfig to bring up the whole app; change it to use
`@DataJpaTest` to load only JPA slice (EntityManager, BookRepository) so you can
remove most TestPropertySource entries and the TestConfig; update
imports/annotations on AppBookSpecificationTest (replace `@SpringBootTest` with
`@DataJpaTest` and keep transactional behavior if needed), ensure H2 create-drop
settings remain (or provide them via `@AutoConfigureTestDatabase` or test
properties limited to datasource/h2), and remove mocks/configs that were only
necessary for full context (e.g., TestConfig and any
TaskCronService/OIDC/scheduling overrides).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17eb8994-effb-465b-8e31-4bbe65e74f31
📒 Files selected for processing (2)
backend/src/main/java/org/booklore/app/specification/AppBookSpecification.javabackend/src/test/java/org/booklore/app/specification/AppBookSpecificationTest.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). (5)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (2)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/test/java/org/booklore/app/specification/AppBookSpecificationTest.javabackend/src/main/java/org/booklore/app/specification/AppBookSpecification.java
backend/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required in backend code
Files:
backend/src/test/java/org/booklore/app/specification/AppBookSpecificationTest.java
🧠 Learnings (3)
📚 Learning: 2026-04-22T01:56:39.495Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-22T01:56:39.495Z
Learning: Applies to backend/src/test/**/*.java : Prefer focused unit tests; use SpringBootTest only when the Spring context is required in backend code
Applied to files:
backend/src/test/java/org/booklore/app/specification/AppBookSpecificationTest.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:
backend/src/test/java/org/booklore/app/specification/AppBookSpecificationTest.javabackend/src/main/java/org/booklore/app/specification/AppBookSpecification.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:
backend/src/test/java/org/booklore/app/specification/AppBookSpecificationTest.javabackend/src/main/java/org/booklore/app/specification/AppBookSpecification.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
Findings for grimmory-tools/grimmory-docs [::grimmory-tools/grimmory-docs::]
- docs/view-preferences.md references "File Size" filter (label/UX) — may need frontend wording or behavior alignment with backend change. [::grimmory-tools/grimmory-docs::docs/view-preferences.md:1]
- docs/book-browser/grid.md includes the sidebar-filters image (visual for filters UI) — frontend UI that uses file-size filter; visuals may become stale if filter behavior/labels change. [::grimmory-tools/grimmory-docs::docs/book-browser/grid.md:1]
- docs/magic-shelf.md documents a "fileSize" field and shows an example JSON filter using fileSize (KB) — a contract example that should match backend bucket semantics. [::grimmory-tools/grimmory-docs::docs/magic-shelf.md:~ lines]
- DOCS_TODO.md and various docs mention file size limits and related settings (informational only). [::grimmory-tools/grimmory-docs::DOCS_TODO.md, docs/tools/global-preferences.md, docs/tools/custom-fonts.md]
Assessment
- No code-level consumers of AppBookSpecification or BookFileEntity found in this docs repo. The repo contains user-facing docs and images referencing the file-size filter; those may require doc/UX updates to reflect the backend's corrected AND/OR/NOT semantics or bucket behavior, but there are no direct code changes to coordinate here.
🔇 Additional comments (2)
backend/src/main/java/org/booklore/app/specification/AppBookSpecification.java (1)
638-671: Refactor looks correct and addresses both bugs cleanly.
- Replacing the MIN(id) correlated subquery with
book.id IN (subquery filtered by isBookFormat=true)sidesteps the Hibernate-generated SQL MariaDB rejected, and it now evaluates every book-format file rather than only the lowest-id one."and"mode builds one subquery per bucket and requires membership in each, which correctly allows different files to satisfy different buckets (perandMode_requiresFilesAcrossAllSelectedRanges)."not"mode now negates the IN-subquery, so books with only non-book-format files (or no files) are included — this is consistent with the new testnotMode_includesBooksWithOnlySupplementaryMatches.One thing worth double-checking:
"not"over books that have no book-format files at all (e.g. audiobook-only) will now include them in the result set. If that matches the product intent (as suggested by the tests), no action needed.backend/src/test/java/org/booklore/app/specification/AppBookSpecificationTest.java (1)
98-260: Nice regression coverage.Good spread across
or,and,not, multi-bucket, and the supplementary-vs-book-format distinction, andandMode_noLongerBehavesLikeOrForMutuallyExclusiveRangesclearly documents the semantic change. As called out in the PR description, backfilling similar coverage for the other specification filters in a follow-up would be worthwhile.
1 participant
Description
Fixes app book file size filtering so it works on MariaDB and evaluates all book format files instead of just the file w/ the lowest file ID.
Linked Issue: Fixes #808
Changes
The previous file size filter was broken in 2 ways:
MIN(id)subquery comparison to identify a single file row, which was rendered into an SQL shape that Maria rejected, causing a 500 when filtering by file size.The changes also add regression coverage in a new
AppBookSpecificationTestfor the following cases:notbehaviororandNotes:
sidebar-filteris no longer used anywhere since the move to backend-based filtering. This work made it clear we should should kill it in a subsequent PR/do a scan to ensure all filters previously available on the F/E logic are fully available via the new approach. Follow up issue: Fix lubimyczytac + ranobedb + audible app book rating filters #815Summary by CodeRabbit
Bug Fixes
Tests