refactor(author): sort authors using sort_name field - #1955
Open
zachyale wants to merge 5 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📜 Recent review details⏰ Context from checks skipped due to timeout. (5)
🧰 Additional context used📓 Path-based instructions (9)frontend/src/**/*.{ts,tsx,html,scss}📄 CodeRabbit inference engine (AGENTS.md)
Files:
frontend/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
frontend/src/**/*.{ts,tsx,html}📄 CodeRabbit inference engine (AGENTS.md)
Files:
frontend/src/**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*⚙️ CodeRabbit configuration file
Files:
**/*.spec.ts⚙️ CodeRabbit configuration file
Files:
backend/src/**/*.java📄 CodeRabbit inference engine (AGENTS.md)
Files:
backend/src/test/**/*.java📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*Test.java⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (12)📚 Learning: 2026-04-05T21:16:01.715ZApplied to files:
📚 Learning: 2026-04-07T09:28:09.587ZApplied to files:
📚 Learning: 2026-04-11T03:55:57.229ZApplied to files:
📚 Learning: 2026-05-18T14:54:39.422ZApplied to files:
📚 Learning: 2026-06-30T01:30:43.644ZApplied to files:
📚 Learning: 2026-04-10T08:15:37.436ZApplied to files:
📚 Learning: 2026-04-14T12:43:08.698ZApplied to files:
📚 Learning: 2026-05-07T21:21:55.233ZApplied to files:
📚 Learning: 2026-05-08T06:19:20.621ZApplied to files:
📚 Learning: 2026-04-27T15:25:55.042ZApplied to files:
📚 Learning: 2026-05-22T03:20:45.559ZApplied to files:
📚 Learning: 2026-05-04T05:01:33.919ZApplied to files:
🔇 Additional comments (2)
WalkthroughThis PR adds ChangesBackend sortName propagation
Frontend sortName model and sorting
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.java`:
- Around line 8-12: The AuthorUpdateRequest DTO already exposes sortName and
sortNameLocked, but AuthorMetadataService.updateAuthor is not applying them, so
add handling there to copy those request values onto the author metadata during
updates. Make sure the update flow treats sortName and sortNameLocked the same
way as the other editable fields in updateAuthor, using the existing author
update logic and identifiers in AuthorMetadataService and AuthorUpdateRequest to
locate the change.
In `@backend/src/main/java/org/booklore/repository/AuthorRepository.java`:
- Around line 25-28: Both AuthorRepository queries now sort by
COALESCE(a.sortName, a.name), so a regular index on sortName will not help this
ordering. Add a function-based/expression index for the same sort expression in
the database schema or migration used by AuthorEntity so the ordering in
findAllWithBookCount and findAllWithBookCountByLibraryIds can use an index. Keep
the index expression aligned exactly with the query’s COALESCE(a.sortName,
a.name) ordering, adjusting syntax to the target DB engine.
In `@backend/src/test/java/org/booklore/mapper/BookMetadataMapperTest.java`:
- Around line 69-81: Add a new test in BookMetadataMapperTest to cover the
`@AfterMapping` fallback when an AuthorEntity has a null sortName. Build a
BookMetadataEntity with authors where one author’s sortName is null, call
bookMetadataMapper.toBookMetadata, and assert that dto.getAuthorSortNames() uses
the author’s name for that entry while preserving the author order; keep the
existing mapsAuthorSortNamesInAuthorOrder test as the happy path and add this
edge-case coverage alongside it.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: ccd1354e-8b3c-4b4e-8f73-651ddb1df9d4
📒 Files selected for processing (18)
backend/src/main/java/org/booklore/app/dto/AppAuthorDetail.javabackend/src/main/java/org/booklore/app/dto/AppAuthorSummary.javabackend/src/main/java/org/booklore/app/service/AppAuthorService.javabackend/src/main/java/org/booklore/mapper/BookMetadataMapper.javabackend/src/main/java/org/booklore/model/dto/AuthorDetails.javabackend/src/main/java/org/booklore/model/dto/AuthorSummary.javabackend/src/main/java/org/booklore/model/dto/BookMetadata.javabackend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.javabackend/src/main/java/org/booklore/repository/AuthorRepository.javabackend/src/test/java/org/booklore/mapper/BookMetadataMapperTest.javafrontend/src/app/features/author-browser/components/author-card/author-card.component.spec.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.spec.tsfrontend/src/app/features/author-browser/components/author-match/author-match.component.spec.tsfrontend/src/app/features/author-browser/model/author.model.spec.tsfrontend/src/app/features/author-browser/model/author.model.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/book/service/sort.service.spec.tsfrontend/src/app/features/book/service/sort.service.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
grimmory-tools/grimmory-docs(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
⚠️ CI failures not shown inline (2)
GitHub Actions: CI - Frontend Quality Thresholds / 0_Frontend Lint Threshold Check.txt: refactor(author): sort authors using sort_name field
Conclusion: failure
Current runner version: '2.335.1'
##[group]Runner Image Provisioner
Hosted Compute Agent
Version: 20260624.560
Commit: 925d229a51159bc391ae97e54a2dd1fe20af789d
Build Date:
Worker ID: {af3bef76-1922-497c-bf47-c521b87ea100}
Azure Region: northcentralus
##[endgroup]
##[group]Operating System
Ubuntu
24.04.4
LTS
##[endgroup]
##[group]Runner Image
Image: ubuntu-24.04
Version: 20260628.225.1
Included Software: https://github.com/actions/runner-images/blob/ubuntu24/20260628.225/images/ubuntu/Ubuntu2404-Readme.md
Image Release: https://github.com/actions/runner-images/releases/tag/ubuntu24%2F20260628.225
##[endgroup]
##[group]GITHUB_TOKEN Permissions
Contents: read
Metadata: read
##[endgroup]
Secret source: Actions
Prepare workflow directory
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0' (SHA:9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0)
Download action repository 'pnpm/action-setup@0ebf47130e4866e96fce0953f49152a61190b271' (SHA:0ebf47130e4866e96fce0953f49152a61190b271)
Download action repository 'actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e' (SHA:48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e)
Complete job name: Frontend Lint Threshold Check
##[group]Run actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0
with:
repository: grimmory-tools/grimmory
***REDACTED***
ssh-strict: true
ssh-user: git
persist-credentials: true
clean: true
sparse-checkout-cone-mode: true
fetch-depth: 1
fetch-tags: false
show-progress: true
lfs: false
submodules: false
set-safe-directory: true
allow-unsafe-pr-checkout: false
env:
LINT_WARNING_THRESHOLD: 0
LINT_ERROR_THRESHOLD: 0
BUILD_WARNING_THRESHOLD: 0
##[endgroup]
Syncing repository: grimmory-tools/grimmory
##[group]Getting Git version info
Working directory is '/home/runner/work/grimmory/grimmory'
[command]/usr/bin/git version
git version 2.54.0
#...
GitHub Actions: CI - Frontend Quality Thresholds / Frontend Lint Threshold Check: refactor(author): sort authors using sort_name field
Conclusion: failure
##[group]Run set +e
�[36;1mset +e�[0m
�[36;1mCI=1 NG_CLI_ANALYTICS=false pnpm run build:prod 2>&1 | tee build-output.log�[0m
�[36;1mBUILD_EXIT_CODE=${PIPESTATUS[0]}�[0m
�[36;1mset -e�[0m
�[36;1m�[0m
�[36;1mif [ "${BUILD_EXIT_CODE}" -ne 0 ]; then�[0m
�[36;1m echo "ERROR: Angular build failed with exit code ${BUILD_EXIT_CODE}"�[0m
�[36;1m exit ${BUILD_EXIT_CODE}�[0m
�[36;1mfi�[0m
shell: /usr/bin/bash -e {0}
env:
LINT_WARNING_THRESHOLD: 0
LINT_ERROR_THRESHOLD: 0
BUILD_WARNING_THRESHOLD: 0
PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
##[endgroup]
$ ng build --configuration production
�[33m❯�[39m Building...
�[32m✔�[39m Building...
�[1mInitial chunk files�[22m�[2m | �[22m�[1mNames�[22m �[2m | �[22m �[1mRaw size�[22m�[2m | �[22m�[1mEstimated transfer size�[22m
�[32mchunk-B26R76ML.js�[39m �[2m | �[22m�[2m-�[22m �[2m | �[22m�[36m402.42 kB�[39m�[2m | �[22m �[36m112.43 kB�[39m
�[32mchunk-QDGHJ627.js�[39m �[2m | �[22m�[2m-�[22m �[2m | �[22m�[36m363.23 kB�[39m�[2m | �[22m �[36m73.55 kB�[39m
�[32mmain-HI2LBWXC.js�[39m �[2m | �[22m�[2mmain�[22m �[2m | �[22m�[36m209.33 kB�[39m�[2m | �[22m �[36m39.65 kB�[39m
�[32mstyles-IMX2ZJHM.css�[39m�[2m | �[22m�[2mstyles�[22m �[2m | �[22m�[36m176.57 kB�[39m�[2m | �[22m �[36m22.72 kB�[39m
�[32mchunk-HMQJOBVF.js�[39m �[2m | �[22m�[2m-�[22m �[2m | �[22m �[36m96.57 kB�[39m�[2m | �[22m �[36m24.32 kB�[39m
�[32mchunk-5KLJIJHQ.js�[39m �[2m | �[22m�[2m-�[22m �[2m | �[22m �[36m50.31 kB�[39m�[2m | �[22m �[36m9.80 kB�[39m
�[32mchunk-3WGVLDJM.js�[39m �[2m | �[22m�[2m-�[22m �[2m | �[22m �[36m49.57 kB�[39m�[2m | �[22m �[36m11.49 kB�[39m
�[32mchunk-EEDR3NAB.js�[39m �[2m | �[22m�[2m-�[22m �[2m | �[22m �[36m45.43 kB�[39m�[2m | �[22m ...
🧰 Additional context used
📓 Path-based instructions (13)
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/main/java/org/booklore/app/dto/AppAuthorSummary.javabackend/src/main/java/org/booklore/app/dto/AppAuthorDetail.javabackend/src/main/java/org/booklore/model/dto/BookMetadata.javabackend/src/main/java/org/booklore/repository/AuthorRepository.javabackend/src/main/java/org/booklore/mapper/BookMetadataMapper.javabackend/src/main/java/org/booklore/model/dto/AuthorDetails.javabackend/src/test/java/org/booklore/mapper/BookMetadataMapperTest.javabackend/src/main/java/org/booklore/model/dto/AuthorSummary.javabackend/src/main/java/org/booklore/app/service/AppAuthorService.javabackend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.java
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
backend/src/main/java/org/booklore/app/dto/AppAuthorSummary.javafrontend/src/app/features/author-browser/model/author.model.spec.tsfrontend/src/app/features/book/model/book.model.tsbackend/src/main/java/org/booklore/app/dto/AppAuthorDetail.javabackend/src/main/java/org/booklore/model/dto/BookMetadata.javafrontend/src/app/features/book/service/sort.service.spec.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.spec.tsfrontend/src/app/features/author-browser/components/author-match/author-match.component.spec.tsbackend/src/main/java/org/booklore/repository/AuthorRepository.javabackend/src/main/java/org/booklore/mapper/BookMetadataMapper.javafrontend/src/app/features/book/service/sort.service.tsbackend/src/main/java/org/booklore/model/dto/AuthorDetails.javafrontend/src/app/features/author-browser/components/author-card/author-card.component.spec.tsbackend/src/test/java/org/booklore/mapper/BookMetadataMapperTest.javabackend/src/main/java/org/booklore/model/dto/AuthorSummary.javabackend/src/main/java/org/booklore/app/service/AppAuthorService.javabackend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.javafrontend/src/app/features/author-browser/model/author.model.ts
**/dto/**/*.java
⚙️ CodeRabbit configuration file
**/dto/**/*.java: DTO review; prefer records:
- Prefer Java records over classes for DTOs.
- Jackson 3: use
@JsonProperty,@JsonAlias, and@JsonIgnoreProperties(ignoreUnknown = true).- Flag missing validation annotations (
@NotNull,@NotBlank,@Size) on input DTOs.- Flag ObjectMapper instantiation inside a DTO; must never happen.
- New Jackson 3 package is tools.jackson.; flag any com.fasterxml.jackson. in new files.
Files:
backend/src/main/java/org/booklore/app/dto/AppAuthorSummary.javabackend/src/main/java/org/booklore/app/dto/AppAuthorDetail.javabackend/src/main/java/org/booklore/model/dto/BookMetadata.javabackend/src/main/java/org/booklore/model/dto/AuthorDetails.javabackend/src/main/java/org/booklore/model/dto/AuthorSummary.javabackend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.java
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS in frontend code
Files:
frontend/src/app/features/author-browser/model/author.model.spec.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/book/service/sort.service.spec.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.spec.tsfrontend/src/app/features/author-browser/components/author-match/author-match.component.spec.tsfrontend/src/app/features/book/service/sort.service.tsfrontend/src/app/features/author-browser/components/author-card/author-card.component.spec.tsfrontend/src/app/features/author-browser/model/author.model.ts
frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer inject() over constructor injection in frontend code
Files:
frontend/src/app/features/author-browser/model/author.model.spec.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/book/service/sort.service.spec.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.spec.tsfrontend/src/app/features/author-browser/components/author-match/author-match.component.spec.tsfrontend/src/app/features/book/service/sort.service.tsfrontend/src/app/features/author-browser/components/author-card/author-card.component.spec.tsfrontend/src/app/features/author-browser/model/author.model.ts
frontend/src/**/*.{ts,tsx,html}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/src/**/*.{ts,tsx,html}: Follow frontend/eslint.config.js: component selectors use app-, directive selectors use app, and any is disallowed in frontend code
Put user-facing strings in Transloco files under frontend/src/i18n/
Files:
frontend/src/app/features/author-browser/model/author.model.spec.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/book/service/sort.service.spec.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.spec.tsfrontend/src/app/features/author-browser/components/author-match/author-match.component.spec.tsfrontend/src/app/features/book/service/sort.service.tsfrontend/src/app/features/author-browser/components/author-card/author-card.component.spec.tsfrontend/src/app/features/author-browser/model/author.model.ts
frontend/src/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for tests in frontend code
Files:
frontend/src/app/features/author-browser/model/author.model.spec.tsfrontend/src/app/features/book/service/sort.service.spec.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.spec.tsfrontend/src/app/features/author-browser/components/author-match/author-match.component.spec.tsfrontend/src/app/features/author-browser/components/author-card/author-card.component.spec.ts
**/*.spec.ts
⚙️ CodeRabbit configuration file
**/*.spec.ts: Angular 21 test review:
- Flag tests with no expect() calls.
- Flag hardcoded async timeouts; prefer fakeAsync/tick or signal-based testing.
- Flag missing fixture.detectChanges() after state mutations.
Files:
frontend/src/app/features/author-browser/model/author.model.spec.tsfrontend/src/app/features/book/service/sort.service.spec.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.spec.tsfrontend/src/app/features/author-browser/components/author-match/author-match.component.spec.tsfrontend/src/app/features/author-browser/components/author-card/author-card.component.spec.ts
**/repository/**/*.java
⚙️ CodeRabbit configuration file
**/repository/**/*.java: Spring Data / Hibernate repository review:
- Prefer Spring Data JPA repository interfaces with typed methods.
- Flag N+1 risks: use
@EntityGraphor JOIN FETCH in JPQL for collection associations.- Flag missing
@Transactional(readOnly = true) on query-only methods.- Flag any native query with string concatenation; use named parameters only.
- Prefer
@Querywith JPQL over native SQL unless performance-critical.
Files:
backend/src/main/java/org/booklore/repository/AuthorRepository.java
**/*.service.ts
⚙️ CodeRabbit configuration file
**/*.service.ts: Angular 21 service review:
- Prefer providedIn: 'root' unless scope is intentionally limited.
- Prefer inject() over constructor DI.
- Prefer Signals or lightweight RxJS (takeUntilDestroyed) over manual subscription management.
- Flag unsubscribed Observables (missing takeUntilDestroyed or explicit unsubscribe).
- Prefer typed HttpClient responses with explicit error handling (catchError).
- Flag any state mutation outside a defined signal or BehaviorSubject.
Files:
frontend/src/app/features/book/service/sort.service.ts
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/mapper/BookMetadataMapperTest.java
**/*Test.java
⚙️ CodeRabbit configuration file
**/*Test.java: Java test review:
- Prefer
@ExtendWith(SpringExtension.class) or@SpringBootTestfor integration tests.- Flag tests with no assertions.
- Flag Thread.sleep() in tests; use Awaitility or virtual-thread-friendly alternatives.
- Flag hardcoded ports or file paths.
- Flag missing edge case coverage: null, empty, boundary values.
- Prefer AssertJ over JUnit's built-in assertions for readability.
- Prefer
@Sqlor Testcontainers for database state; not hand-rolled setup/teardown.
Files:
backend/src/test/java/org/booklore/mapper/BookMetadataMapperTest.java
**/service/**/*.java
⚙️ CodeRabbit configuration file
**/service/**/*.java: Spring Framework 7 service layer review:
- Flag missing
@Transactionalon methods that perform multiple writes.- Prefer constructor injection over
@Autowiredfield injection. Use@AllArgsConstructor.- Use ApiError enum for throwing exceptions.
- Flag checked exceptions swallowed silently; must log or rethrow.
- Flag Thread.sleep(); prefer Duration-based overloads or ScheduledExecutorService.
- Prefer virtual threads (Thread.ofVirtual()) for I/O-bound operations.
- Flag mutable shared state in singleton beans.
Files:
backend/src/main/java/org/booklore/app/service/AppAuthorService.java
🧠 Learnings (19)
📚 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/main/java/org/booklore/app/dto/AppAuthorSummary.javabackend/src/main/java/org/booklore/app/dto/AppAuthorDetail.javabackend/src/main/java/org/booklore/model/dto/BookMetadata.javabackend/src/main/java/org/booklore/repository/AuthorRepository.javabackend/src/main/java/org/booklore/mapper/BookMetadataMapper.javabackend/src/main/java/org/booklore/model/dto/AuthorDetails.javabackend/src/test/java/org/booklore/mapper/BookMetadataMapperTest.javabackend/src/main/java/org/booklore/model/dto/AuthorSummary.javabackend/src/main/java/org/booklore/app/service/AppAuthorService.javabackend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.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/main/java/org/booklore/app/dto/AppAuthorSummary.javabackend/src/main/java/org/booklore/app/dto/AppAuthorDetail.javabackend/src/main/java/org/booklore/model/dto/BookMetadata.javabackend/src/main/java/org/booklore/repository/AuthorRepository.javabackend/src/main/java/org/booklore/mapper/BookMetadataMapper.javabackend/src/main/java/org/booklore/model/dto/AuthorDetails.javabackend/src/test/java/org/booklore/mapper/BookMetadataMapperTest.javabackend/src/main/java/org/booklore/model/dto/AuthorSummary.javabackend/src/main/java/org/booklore/app/service/AppAuthorService.javabackend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.java
📚 Learning: 2026-05-07T21:21:55.233Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1194
File: backend/src/main/java/org/booklore/service/ReadingSessionService.java:0-0
Timestamp: 2026-05-07T21:21:55.233Z
Learning: When reviewing Java 23+ code, treat `java.time.Instant#until(Instant endExclusive)` as a valid API/method call (it returns a `Duration`, equivalent to `Duration.between(this, endExclusive)`). Do not flag `instant.until(otherInstant)` as a compile error or API misuse when the project targets Java 25+ (as in grimmory-tools/grimmory); the call should be considered correct and returns a `Duration`.
Applied to files:
backend/src/main/java/org/booklore/app/dto/AppAuthorSummary.javabackend/src/main/java/org/booklore/app/dto/AppAuthorDetail.javabackend/src/main/java/org/booklore/model/dto/BookMetadata.javabackend/src/main/java/org/booklore/repository/AuthorRepository.javabackend/src/main/java/org/booklore/mapper/BookMetadataMapper.javabackend/src/main/java/org/booklore/model/dto/AuthorDetails.javabackend/src/test/java/org/booklore/mapper/BookMetadataMapperTest.javabackend/src/main/java/org/booklore/model/dto/AuthorSummary.javabackend/src/main/java/org/booklore/app/service/AppAuthorService.javabackend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.java
📚 Learning: 2026-05-08T06:19:20.621Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1201
File: backend/src/main/java/org/booklore/model/dto/AccessTokenDto.java:3-10
Timestamp: 2026-05-08T06:19:20.621Z
Learning: For Jackson 3 codebases, do not treat imports from `com.fasterxml.jackson.annotation.*` (e.g., `JsonInclude`, `JsonProperty`, `JsonView`) as incorrect. In Jackson 3, `jackson-annotations` intentionally remains under `com.fasterxml.jackson.annotation.*` for backward compatibility, while only the core processing packages (e.g., `jackson-core`, `jackson-databind`) move to the `tools.jackson.*` namespace.
Applied to files:
backend/src/main/java/org/booklore/app/dto/AppAuthorSummary.javabackend/src/main/java/org/booklore/app/dto/AppAuthorDetail.javabackend/src/main/java/org/booklore/model/dto/BookMetadata.javabackend/src/main/java/org/booklore/repository/AuthorRepository.javabackend/src/main/java/org/booklore/mapper/BookMetadataMapper.javabackend/src/main/java/org/booklore/model/dto/AuthorDetails.javabackend/src/test/java/org/booklore/mapper/BookMetadataMapperTest.javabackend/src/main/java/org/booklore/model/dto/AuthorSummary.javabackend/src/main/java/org/booklore/app/service/AppAuthorService.javabackend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.java
📚 Learning: 2026-05-04T20:31:11.075Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1086
File: backend/src/main/java/org/booklore/service/metadata/BookReviewUpdateService.java:63-66
Timestamp: 2026-05-04T20:31:11.075Z
Learning: For this repository, reviewers should treat string truncation done via `String.length()` and `String.substring(0, maxLength)` (UTF-16 code units) as an accepted, consistent convention. Do not flag individual occurrences of this pattern as bugs, even though it is not code-point-aware for surrogate pairs. A separate global effort is already tracked to move toward code-point-aware truncation, so per-site fixes should be avoided during code review.
Applied to files:
backend/src/main/java/org/booklore/app/dto/AppAuthorSummary.javabackend/src/main/java/org/booklore/app/dto/AppAuthorDetail.javabackend/src/main/java/org/booklore/model/dto/BookMetadata.javabackend/src/main/java/org/booklore/repository/AuthorRepository.javabackend/src/main/java/org/booklore/mapper/BookMetadataMapper.javabackend/src/main/java/org/booklore/model/dto/AuthorDetails.javabackend/src/main/java/org/booklore/model/dto/AuthorSummary.javabackend/src/main/java/org/booklore/app/service/AppAuthorService.javabackend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.java
📚 Learning: 2026-05-13T12:34:49.607Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 1293
File: backend/src/main/java/org/booklore/service/metadata/DuckDuckGoCoverService.java:246-252
Timestamp: 2026-05-13T12:34:49.607Z
Learning: In this repo’s Java code, when catching Jsoup `org.jsoup.HttpStatusException` (and similar exceptions originating from external libraries) and wrapping/rethrowing them, do not require preserving the original exception stack trace (e.g., as flagged by PMD `PreserveStackTrace`) as long as the application already captures the actionable diagnostics in logs or the thrown exception message (such as HTTP status code and the requested URL). Reviewers should still ensure the log/message contains those details; the intent is to avoid noisy stack traces that only reflect external-library internals rather than application code.
Applied to files:
backend/src/main/java/org/booklore/app/dto/AppAuthorSummary.javabackend/src/main/java/org/booklore/app/dto/AppAuthorDetail.javabackend/src/main/java/org/booklore/model/dto/BookMetadata.javabackend/src/main/java/org/booklore/repository/AuthorRepository.javabackend/src/main/java/org/booklore/mapper/BookMetadataMapper.javabackend/src/main/java/org/booklore/model/dto/AuthorDetails.javabackend/src/main/java/org/booklore/model/dto/AuthorSummary.javabackend/src/main/java/org/booklore/app/service/AppAuthorService.javabackend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.java
📚 Learning: 2026-05-17T13:38:16.462Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 1366
File: backend/src/main/java/org/booklore/service/FileStreamingService.java:65-66
Timestamp: 2026-05-17T13:38:16.462Z
Learning: In the grimmory-tools/grimmory repo, it’s an accepted pattern to pass the raw AccessDeniedException.getMessage() (even if it may include filesystem path details) into ApiError.PERMISSION_DENIED.createException(...). During code review, do not raise a security/information-disclosure issue solely based on that exception message being propagated to the API when using ApiError.PERMISSION_DENIED.createException with the AccessDeniedException message.
Applied to files:
backend/src/main/java/org/booklore/app/dto/AppAuthorSummary.javabackend/src/main/java/org/booklore/app/dto/AppAuthorDetail.javabackend/src/main/java/org/booklore/model/dto/BookMetadata.javabackend/src/main/java/org/booklore/repository/AuthorRepository.javabackend/src/main/java/org/booklore/mapper/BookMetadataMapper.javabackend/src/main/java/org/booklore/model/dto/AuthorDetails.javabackend/src/main/java/org/booklore/model/dto/AuthorSummary.javabackend/src/main/java/org/booklore/app/service/AppAuthorService.javabackend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.java
📚 Learning: 2026-05-23T23:01:25.769Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1456
File: backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java:618-630
Timestamp: 2026-05-23T23:01:25.769Z
Learning: In this codebase (grimmory-tools/grimmory), it’s intentional to omit per-request timeouts on individual Java HttpRequest.Builder instances (e.g., GoodReadsParser.fetchJson). During reviews, do not flag missing builder-level timeouts as a best-practice violation; rely on framework-level and/or HttpClient-level timeouts configured elsewhere for consistent behavior. Only raise an issue if you can verify that no effective timeout is configured at the HttpClient/framework level.
Applied to files:
backend/src/main/java/org/booklore/app/dto/AppAuthorSummary.javabackend/src/main/java/org/booklore/app/dto/AppAuthorDetail.javabackend/src/main/java/org/booklore/model/dto/BookMetadata.javabackend/src/main/java/org/booklore/repository/AuthorRepository.javabackend/src/main/java/org/booklore/mapper/BookMetadataMapper.javabackend/src/main/java/org/booklore/model/dto/AuthorDetails.javabackend/src/main/java/org/booklore/model/dto/AuthorSummary.javabackend/src/main/java/org/booklore/app/service/AppAuthorService.javabackend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.java
📚 Learning: 2026-06-12T01:10:31.416Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1724
File: backend/src/main/java/org/booklore/repository/BookRepository.java:58-59
Timestamp: 2026-06-12T01:10:31.416Z
Learning: In this codebase (grimmory-tools/grimmory), reviews should not treat inline `LIMIT`/`OFFSET` clauses inside `Query` JPQL/HQL strings as a JPA compliance risk. This is intentional: `hibernate.jpa.compliance.query=true` is intentionally not set, and Hibernate 7.3+ supports `LIMIT`/`OFFSET` as valid HQL extensions. Therefore, do not flag or require changes to `Query` annotations solely due to `LIMIT`/`OFFSET` usage.
Applied to files:
backend/src/main/java/org/booklore/app/dto/AppAuthorSummary.javabackend/src/main/java/org/booklore/app/dto/AppAuthorDetail.javabackend/src/main/java/org/booklore/model/dto/BookMetadata.javabackend/src/main/java/org/booklore/repository/AuthorRepository.javabackend/src/main/java/org/booklore/mapper/BookMetadataMapper.javabackend/src/main/java/org/booklore/model/dto/AuthorDetails.javabackend/src/main/java/org/booklore/model/dto/AuthorSummary.javabackend/src/main/java/org/booklore/app/service/AppAuthorService.javabackend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.java
📚 Learning: 2026-04-05T21:16:01.715Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 385
File: frontend/src/app/app.component.ts:55-56
Timestamp: 2026-04-05T21:16:01.715Z
Learning: When reviewing code in the Grimmory frontend (Angular), prefer modern Angular patterns. Specifically: (1) Prefer `DestroyRef` with `takeUntilDestroyed(destroyRef)` for teardown in Angular v16+ instead of manually tracking `Subscription` arrays and calling `unsubscribe()` in `ngOnDestroy()`. (2) Prefer `inject()` for dependency injection over constructor injection where appropriate. (3) Prefer Angular signals (e.g., `signal`, `computed`) over `BehaviorSubject`/`Observable` for state where signals/computed values fit the use case. Flag older patterns when they can be replaced with these modern equivalents without changing behavior.
Applied to files:
frontend/src/app/features/author-browser/model/author.model.spec.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/book/service/sort.service.spec.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.spec.tsfrontend/src/app/features/author-browser/components/author-match/author-match.component.spec.tsfrontend/src/app/features/book/service/sort.service.tsfrontend/src/app/features/author-browser/components/author-card/author-card.component.spec.tsfrontend/src/app/features/author-browser/model/author.model.ts
📚 Learning: 2026-04-07T09:28:09.587Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 393
File: frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts:255-263
Timestamp: 2026-04-07T09:28:09.587Z
Learning: In this Angular frontend (under frontend/src/app/), flag manual resource management/cleanup patterns when there is an Angular v16+ automatic alternative. Examples to prefer: (1) Instead of manually pairing document/window event listeners with stored cleanup functions (e.g., add/removeEventListener with mouseMoveCleanup/documentClickCleanup/keydownCleanup/touchCleanup fields), register teardown via DestroyRef.onDestroy(cleanupFn) (or equivalent Angular v16+ teardown mechanism). (2) Instead of storing Subscriptions in fields and explicitly unsubscribing in ngOnDestroy (e.g., annotationSaveSubscription/annotationCacheSubscription), use takeUntilDestroyed(destroyRef) (piped into the observable) or other Angular v16+ primitives. (3) If teardown is lifecycle-coupled and can be automated via DestroyRef/takeUntilDestroyed/signals (or other Angular v16+ mechanisms), prefer the automated approach over manual ngOnDestroy cleanup. Raise a review finding for the manual pattern and recommend the aut...
Applied to files:
frontend/src/app/features/author-browser/model/author.model.spec.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/book/service/sort.service.spec.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.spec.tsfrontend/src/app/features/author-browser/components/author-match/author-match.component.spec.tsfrontend/src/app/features/book/service/sort.service.tsfrontend/src/app/features/author-browser/components/author-card/author-card.component.spec.tsfrontend/src/app/features/author-browser/model/author.model.ts
📚 Learning: 2026-04-11T03:55:57.229Z
Learnt from: zachyale
Repo: grimmory-tools/grimmory PR: 439
File: frontend/src/app/features/series-browser/components/series-browser/series-browser.component.ts:178-196
Timestamp: 2026-04-11T03:55:57.229Z
Learning: In this Angular frontend (frontend/src/app/), prefer the team’s reactive i18n “signal/computed” pattern: (1) For individual reactive translated strings, use `translateSignal()` from `jsverse/transloco`. (2) For option/label arrays that must update on language switch, create a single `activeLang` signal with `toSignal(t.langChanges$, { initialValue: t.getActiveLang() })`, then derive the arrays as `computed()` signals that read `activeLang()`. This should avoid manual `langChanges$` subscriptions and any `ngOnDestroy` subscription cleanup; prefer this over subscribing in `ngOnInit` when implementing reactive localization.
Applied to files:
frontend/src/app/features/author-browser/model/author.model.spec.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/book/service/sort.service.spec.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.spec.tsfrontend/src/app/features/author-browser/components/author-match/author-match.component.spec.tsfrontend/src/app/features/book/service/sort.service.tsfrontend/src/app/features/author-browser/components/author-card/author-card.component.spec.tsfrontend/src/app/features/author-browser/model/author.model.ts
📚 Learning: 2026-05-18T14:54:39.422Z
Learnt from: alexhb1
Repo: grimmory-tools/grimmory PR: 1379
File: frontend/src/assets/styles/tailwind.css:3-4
Timestamp: 2026-05-18T14:54:39.422Z
Learning: In the grimmory-tools/grimmory repository, Biome is not used for linting/formatting (no `biome.json` and no Biome dependency in `package.json`). During code reviews, do not raise Biome-related issues or recommend adding/changing `biome.json`/Biome dependencies for formatting or linting in this project.
Applied to files:
frontend/src/app/features/author-browser/model/author.model.spec.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/book/service/sort.service.spec.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.spec.tsfrontend/src/app/features/author-browser/components/author-match/author-match.component.spec.tsfrontend/src/app/features/book/service/sort.service.tsfrontend/src/app/features/author-browser/components/author-card/author-card.component.spec.tsfrontend/src/app/features/author-browser/model/author.model.ts
📚 Learning: 2026-06-30T01:30:43.644Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1881
File: frontend/src/app/shared/components/icon-picker/icon-picker-component.ts:116-119
Timestamp: 2026-06-30T01:30:43.644Z
Learning: In the Grimmory Angular frontend (Angular 21), do not flag `[(ngModel)]` two-way bindings that are bound directly to a `WritableSignal`. Angular supports two-way binding to writable signals, so usages like `[(ngModel)]="svgSearchText"` should be treated as valid and should not be considered an incorrect overwrite of the signal with a plain value. Only flag if the binding target is not a `WritableSignal`.
Applied to files:
frontend/src/app/features/author-browser/model/author.model.spec.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/book/service/sort.service.spec.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.spec.tsfrontend/src/app/features/author-browser/components/author-match/author-match.component.spec.tsfrontend/src/app/features/book/service/sort.service.tsfrontend/src/app/features/author-browser/components/author-card/author-card.component.spec.tsfrontend/src/app/features/author-browser/model/author.model.ts
📚 Learning: 2026-05-02T18:47:09.753Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1052
File: backend/src/main/java/org/booklore/model/dto/kobo/KoboDeals.java:14-33
Timestamp: 2026-05-02T18:47:09.753Z
Learning: In this codebase, do not raise review findings suggesting conversion of class-based DTOs to Java `record`s (e.g., DTOs under `org.booklore.model.dto.*` such as `org.booklore.model.dto.kobo`). Although records may be a preferred guideline in general, the team is intentionally avoiding new `record` DTOs right now due to low existing adoption—so class-based DTOs should not be flagged solely for not being records.
Applied to files:
backend/src/main/java/org/booklore/model/dto/BookMetadata.javabackend/src/main/java/org/booklore/model/dto/AuthorDetails.javabackend/src/main/java/org/booklore/model/dto/AuthorSummary.javabackend/src/main/java/org/booklore/model/dto/request/AuthorUpdateRequest.java
📚 Learning: 2026-04-27T15:25:55.042Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 930
File: backend/src/test/java/org/booklore/service/metadata/parser/ComicvineBookParserTest.java:68-75
Timestamp: 2026-04-27T15:25:55.042Z
Learning: In this repository’s JUnit 5 test sources (e.g., under backend/src/test/java/), do not flag “bare” `assert` statements as a bug. The project test runner is configured to always execute tests with assertions enabled (e.g., `-ea`), so `assert` behavior is consistent. Continue to review for correctness, but don’t treat unguarded `assert` usage in test classes as a static-analysis issue.
Applied to files:
backend/src/test/java/org/booklore/mapper/BookMetadataMapperTest.java
📚 Learning: 2026-05-22T03:20:45.559Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1446
File: backend/src/test/java/org/booklore/service/metadata/MetadataManagementServiceTest.java:465-470
Timestamp: 2026-05-22T03:20:45.559Z
Learning: In backend unit tests (Java files under backend/src/test/java), do not flag hardcoded placeholder filesystem paths (e.g., setting LibraryPathEntity.path = "/example") as violations when the value is intentionally unused for filesystem access. Only suppress the "no hardcoded paths" concern if the test does not perform any filesystem/network IO using that path (no reads/writes/Files.* calls or code paths that access the filesystem with that value); if the placeholder is actually used to touch the filesystem, flag it.
Applied to files:
backend/src/test/java/org/booklore/mapper/BookMetadataMapperTest.java
📚 Learning: 2026-05-04T05:01:33.919Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1081
File: backend/src/test/java/org/booklore/service/metadata/parser/AudibleParserTest.java:132-135
Timestamp: 2026-05-04T05:01:33.919Z
Learning: In grimmory-tools/grimmory unit/integration test classes (e.g., files matching **/*Test.java under backend/src/test/java/**), it is acceptable to directly instantiate Jackson mappers (e.g., `new ObjectMapper()` / `new JsonMapper(...)`) and this should NOT be flagged. The Jackson guidance to use Spring bean injection or `JsonMapper.shared()` applies only to production code; tests may construct dependencies directly as standard practice. Production-code mapper instantiation rules should still be enforced outside test sources.
Applied to files:
backend/src/test/java/org/booklore/mapper/BookMetadataMapperTest.java
📚 Learning: 2026-05-07T21:37:46.988Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1194
File: backend/src/main/java/org/booklore/service/ReadingSessionService.java:121-123
Timestamp: 2026-05-07T21:37:46.988Z
Learning: In grimmory-tools/grimmory service-layer code, if arithmetic overflow occurs inside inference/business-logic (e.g., when deriving inferred fields like durationSeconds from start/end timestamps), treat it as a server-side anomaly. Prefer letting the global exception handler translate it into a generic 5xx response rather than throwing an explicit ApiError 4xx (e.g., do not convert overflow into a client error).
Applied to files:
backend/src/main/java/org/booklore/app/service/AppAuthorService.java
🔇 Additional comments (16)
frontend/src/app/features/author-browser/model/author.model.ts (1)
4-4: LGTM!Also applies to: 44-48, 77-81
frontend/src/app/features/book/model/book.model.ts (1)
202-202: LGTM!frontend/src/app/features/author-browser/components/author-card/author-card.component.spec.ts (1)
238-241: LGTM!frontend/src/app/features/author-browser/components/author-detail/author-detail.component.spec.ts (1)
37-41: LGTM!frontend/src/app/features/author-browser/components/author-match/author-match.component.spec.ts (1)
143-146: LGTM!frontend/src/app/features/author-browser/model/author.model.spec.ts (1)
62-62: LGTM!frontend/src/app/features/book/service/sort.service.ts (1)
66-67: LGTM!frontend/src/app/features/book/service/sort.service.spec.ts (1)
58-60: LGTM!Also applies to: 72-82
backend/src/main/java/org/booklore/app/dto/AppAuthorDetail.java (1)
17-17: LGTM!backend/src/main/java/org/booklore/app/dto/AppAuthorSummary.java (1)
17-17: LGTM!backend/src/main/java/org/booklore/model/dto/AuthorSummary.java (1)
17-17: LGTM!backend/src/main/java/org/booklore/model/dto/AuthorDetails.java (1)
13-17: LGTM!backend/src/main/java/org/booklore/app/service/AppAuthorService.java (2)
89-89: LGTM!Also applies to: 132-132
191-196: LGTM!backend/src/main/java/org/booklore/model/dto/BookMetadata.java (1)
64-64: LGTM!backend/src/main/java/org/booklore/mapper/BookMetadataMapper.java (1)
19-27: LGTM!
Comment on lines
+25
to
+28
| @Query("SELECT a, COUNT(bm) FROM AuthorEntity a LEFT JOIN a.bookMetadataEntityList bm GROUP BY a ORDER BY COALESCE(a.sortName, a.name)") | ||
| List<Object[]> findAllWithBookCount(); | ||
|
|
||
| @Query("SELECT a, COUNT(DISTINCT bm) FROM AuthorEntity a LEFT JOIN a.bookMetadataEntityList bm JOIN bm.book b WHERE b.library.id IN :libraryIds GROUP BY a ORDER BY a.name") | ||
| @Query("SELECT a, COUNT(DISTINCT bm) FROM AuthorEntity a LEFT JOIN a.bookMetadataEntityList bm JOIN bm.book b WHERE b.library.id IN :libraryIds GROUP BY a ORDER BY COALESCE(a.sortName, a.name)") |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
LGTM!
Consider a function-based index for the COALESCE sort expression.
Both findAllWithBookCount and findAllWithBookCountByLibraryIds now order by COALESCE(a.sortName, a.name). A plain index on sortName won't be used by the database for this expression. If author tables grow large, evaluate a function-based/expression index (e.g., CREATE INDEX ... ON author (COALESCE(sort_name, name)) — syntax varies by DB engine) to keep these queries efficient.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/main/java/org/booklore/repository/AuthorRepository.java` around
lines 25 - 28, Both AuthorRepository queries now sort by COALESCE(a.sortName,
a.name), so a regular index on sortName will not help this ordering. Add a
function-based/expression index for the same sort expression in the database
schema or migration used by AuthorEntity so the ordering in findAllWithBookCount
and findAllWithBookCountByLibraryIds can use an index. Keep the index expression
aligned exactly with the query’s COALESCE(a.sortName, a.name) ordering,
adjusting syntax to the target DB engine.
Contributor
|
This feels more like a feature at this point. It's far outside of refactor territory, and the previous sort name code was pushing the definition pretty far. |
imnotjames
reviewed
Jul 11, 2026
Contributor
|
Per feedback in Discord I am waiting for this to be renamed to |
imnotjames
approved these changes
Jul 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR is the second part of the #1892 epic. It begins using the
sort_namewithin the frontend-heavy sorting and filtering logic.Linked Issue
Completes #1894
Changes
sort_namefield on author responsesauthorSortNameson book metadata as another string array (same order as author array, just using the sort values instead of proper names)Manual Testing Steps
Screenshots (Optional)
N/A
Additional Context (Optional)
Still non-user facing
AI Disclosure
N/A
Checklist
just ui checkandjust api check.Summary by CodeRabbit
sortName(andsortNameLocked) support for author details, summaries, and update requests.authorSortNamesso author sorting can use the preferred values.authorSortNames, with fallback to existing author data.