fix(file): update file type and size retrieval to use primaryFile structure#383
Conversation
📝 WalkthroughWalkthroughThe changes refactor how file-related rule fields ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
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 |
|
I think there may be more to this, though. |
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 (1)
booklore-api/src/main/java/org/booklore/service/BookRuleEvaluatorService.java (1)
774-817:⚠️ Potential issue | 🟠 MajorConstrain
bookFilesto a single primary row before evaluating file rules.
Line 774andLine 817join the full one-to-manybookFilescollection, so rule evaluation can match a non-primary file when a book has multiple files. That can make backend filtering diverge from frontend behavior (primaryFile) and return incorrectfileType/fileSizematches.🤖 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/BookRuleEvaluatorService.java` around lines 774 - 817, The switch is joining the whole one-to-many bookFiles collection (root.join("bookFiles", JoinType.LEFT)) so file-based rules can match non-primary files; change to join only the primary file by creating a single Join<?,?> primaryBookFileJoin = root.join("bookFiles", JoinType.LEFT) and apply an ON predicate for the primary flag (e.g., primaryBookFileJoin.on(cb.isTrue(primaryBookFileJoin.get("primaryFile"))) or equivalent CriteriaBuilder predicate/subquery), then replace all occurrences that reference root.join("bookFiles", JoinType.LEFT) (used for FILE_SIZE, AUDIOBOOK_DURATION, AUDIOBOOK_CODEC, AUDIOBOOK_CHAPTER_COUNT, AUDIOBOOK_BITRATE, FILE_TYPE, etc.) to use primaryBookFileJoin so fileType/fileSize/duration/codec lookups use the constrained primary file; keep READING_PROGRESS and other non-file joins unchanged.
🧹 Nitpick comments (1)
frontend/src/app/features/magic-shelf/service/book-rule-evaluator.service.spec.ts (1)
17-17: Consider removing defaultprimaryFile.fileSizeKbfrom fixture to avoid masked assertions.A default size of
1024can let file-size tests pass even when a test accidentally sets deprecated top-levelfileSizeKbinstead ofprimaryFile.fileSizeKb. Prefer settingfileSizeKbexplicitly per file-size test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/magic-shelf/service/book-rule-evaluator.service.spec.ts` at line 17, The fixture currently sets primaryFile.fileSizeKb = 1024 which can mask bugs; remove the default fileSizeKb from the primaryFile object in the test fixture (the object with key primaryFile) and update any tests that rely on a size to set primaryFile.fileSizeKb explicitly inside those test cases; also audit assertions to ensure they reference primaryFile.fileSizeKb (not any deprecated top-level fileSizeKb) in BookRuleEvaluatorService specs.
🤖 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/BookRuleEvaluatorService.java`:
- Around line 774-817: The switch is joining the whole one-to-many bookFiles
collection (root.join("bookFiles", JoinType.LEFT)) so file-based rules can match
non-primary files; change to join only the primary file by creating a single
Join<?,?> primaryBookFileJoin = root.join("bookFiles", JoinType.LEFT) and apply
an ON predicate for the primary flag (e.g.,
primaryBookFileJoin.on(cb.isTrue(primaryBookFileJoin.get("primaryFile"))) or
equivalent CriteriaBuilder predicate/subquery), then replace all occurrences
that reference root.join("bookFiles", JoinType.LEFT) (used for FILE_SIZE,
AUDIOBOOK_DURATION, AUDIOBOOK_CODEC, AUDIOBOOK_CHAPTER_COUNT, AUDIOBOOK_BITRATE,
FILE_TYPE, etc.) to use primaryBookFileJoin so fileType/fileSize/duration/codec
lookups use the constrained primary file; keep READING_PROGRESS and other
non-file joins unchanged.
---
Nitpick comments:
In
`@frontend/src/app/features/magic-shelf/service/book-rule-evaluator.service.spec.ts`:
- Line 17: The fixture currently sets primaryFile.fileSizeKb = 1024 which can
mask bugs; remove the default fileSizeKb from the primaryFile object in the test
fixture (the object with key primaryFile) and update any tests that rely on a
size to set primaryFile.fileSizeKb explicitly inside those test cases; also
audit assertions to ensure they reference primaryFile.fileSizeKb (not any
deprecated top-level fileSizeKb) in BookRuleEvaluatorService specs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36fc821b-5a6a-458e-a5c1-3cb3016b2ac8
📒 Files selected for processing (3)
booklore-api/src/main/java/org/booklore/service/BookRuleEvaluatorService.javafrontend/src/app/features/magic-shelf/service/book-rule-evaluator.service.spec.tsfrontend/src/app/features/magic-shelf/service/book-rule-evaluator.service.ts
📜 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 (4)
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS files
Files:
frontend/src/app/features/magic-shelf/service/book-rule-evaluator.service.tsfrontend/src/app/features/magic-shelf/service/book-rule-evaluator.service.spec.ts
frontend/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/src/app/**/*.{ts,tsx}: Preferinject()over constructor injection
Followfrontend/eslint.config.js: component selectors useapp-*, directive selectors useapp*, andanyis disallowed
Files:
frontend/src/app/features/magic-shelf/service/book-rule-evaluator.service.tsfrontend/src/app/features/magic-shelf/service/book-rule-evaluator.service.spec.ts
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/BookRuleEvaluatorService.java
frontend/src/**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for tests in the frontend
Files:
frontend/src/app/features/magic-shelf/service/book-rule-evaluator.service.spec.ts
🧠 Learnings (1)
📚 Learning: 2026-04-04T14:03:24.311Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 372
File: frontend/src/app/features/book/service/app-books-api.service.ts:150-178
Timestamp: 2026-04-04T14:03:24.311Z
Learning: In `frontend/src/app/features/book/service/app-books-api.service.ts`, the `summaryToBook` function intentionally uses `as unknown as Book` to cast a partial object to the `Book` type. The returned object omits some required `Book` fields (e.g. `metadata.bookId`, `pdfProgress.page`) and includes an extra `bookFiles: []` property not in the `Book` interface. This is by design — consumers of `AppBooksApiService.books()` (e.g. `BookCardComponent`) only access the subset of properties that are populated from `AppBookSummary`, so no runtime issues occur. Do not flag this as a type safety error.
Applied to files:
frontend/src/app/features/magic-shelf/service/book-rule-evaluator.service.tsfrontend/src/app/features/magic-shelf/service/book-rule-evaluator.service.spec.ts
🔀 Multi-repo context grimmory-tools/grimmory-docs
Linked repositories findings
grimmory-tools/grimmory-docs
-
docs/magic-shelf.md — Field reference documents
File Type(fileType) andFile Size(fileSize) as first-class Magic Shelf fields and includes example rules that rely on them (e.g. EPUB filter, includes_any for comic formats, oversized-files > 102400 KB). These docs describe evaluation happening in the browser and show JSON rule examples usingfileType/fileSize. [::grimmory-tools/grimmory-docs::docs/magic-shelf.md: "File & Identifiers" table, examples around lines ~120–140 and ~1290–1335] -
docs/magic-shelf.md — Several example Magic Shelf JSON snippets (EPUB filter, comics filter, oversized files) explicitly use
fileTypeandfileSizekeys; these will depend on the frontend evaluator reading those fields from book.primaryFile rather than top-level book properties after this PR. [::grimmory-tools/grimmory-docs::docs/magic-shelf.md: examples at lines ~1298, ~1310, ~1320]
Relevance summary:
- The documentation confirms the frontend/browser-side rule evaluator is expected to expose
fileTypeandfileSizefields for rules. The PR’s change (frontend: source fileType/fileSize from book.primaryFile; backend: join bookFiles) aligns code with the documented rule keys but changes which books match when primaryFile/bookFiles are absent. Reviewers should ensure docs and example rules remain accurate and that consumers/readers of these docs/test fixtures reflect the primaryFile-centric model. [::grimmory-tools/grimmory-docs::docs/magic-shelf.md]
🔇 Additional comments (1)
frontend/src/app/features/magic-shelf/service/book-rule-evaluator.service.ts (1)
74-75: Primary-file field extraction looks correct and consistent.These updates consistently source
fileType/fileSizefrombook.primaryFilewith null-safe access, matching the updated data model.Also applies to: 278-280
Description
Linked Issue: Fixes #
Changes
This pull request updates how file-related attributes are accessed for books, moving from top-level fields like
bookTypeandfileSizeKbto retrieving these values from theprimaryFileproperty on the frontend and from thebookFilesrelationship on the backend. This change improves consistency with the data model and prepares the codebase for handling books with multiple files. The test suite has also been updated to reflect these changes.Essentially fixes filetype filters.
Summary by CodeRabbit