refactor(api): rename KOReader sync references from booklore to grimmory#1085
Open
Michael-Tanzer wants to merge 2 commits intogrimmory-tools:developfrom
Open
refactor(api): rename KOReader sync references from booklore to grimmory#1085Michael-Tanzer wants to merge 2 commits intogrimmory-tools:developfrom
Michael-Tanzer wants to merge 2 commits intogrimmory-tools:developfrom
Conversation
Rename the sync-progress endpoint, database column, and all Java/TypeScript field and method references from "booklore" to "grimmory". Remove the legacy frontend endpoint fallback and DTO field. Add Flyway migration V136 to rename the sync_with_booklore_reader column. Fixes grimmory-tools#191
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughThis PR consolidates KOReader sync terminology by renaming the sync-progress flag from ChangesSync Terminology Consolidation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
🚥 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
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Author
|
@imnotjames rebased! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (3)
backend/src/test/java/org/booklore/service/KoreaderUserServiceTest.java (1)
122-134: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMissing unit tests for
toggleSyncProgressWithGrimmory.
toggleSyncis covered by two tests (toggleSync_setsFlag_andSaves,toggleSync_throws_whenEntityMissing), but the newtoggleSyncProgressWithGrimmorymethod has no analogous tests here. The controller test only verifies delegation to the service; service-layer behavior (correct field set,savecalled, exception on missing entity) is untested.✅ Suggested tests to add
`@Test` void toggleSyncProgressWithGrimmory_setsFlag_andSaves() { when(koreaderUserRepository.findByBookLoreUserId(123L)).thenReturn(Optional.of(entity)); service.toggleSyncProgressWithGrimmory(true); assertTrue(entity.isSyncWithGrimmoryReader()); verify(koreaderUserRepository).save(entity); } `@Test` void toggleSyncProgressWithGrimmory_throws_whenEntityMissing() { when(koreaderUserRepository.findByBookLoreUserId(123L)).thenReturn(Optional.empty()); assertThrows(APIException.class, () -> service.toggleSyncProgressWithGrimmory(true)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/org/booklore/service/KoreaderUserServiceTest.java` around lines 122 - 134, Add two unit tests in KoreaderUserServiceTest mirroring the existing toggleSync tests but targeting toggleSyncProgressWithGrimmory: mock koreaderUserRepository.findByBookLoreUserId(123L) to return Optional.of(entity), call service.toggleSyncProgressWithGrimmory(true), assert entity.isSyncWithGrimmoryReader() is true and verify koreaderUserRepository.save(entity) was called; and a second test where the repository returns Optional.empty() and you assertThrows(APIException.class, () -> service.toggleSyncProgressWithGrimmory(true)).backend/src/main/java/org/booklore/service/koreader/KoreaderService.java (1)
131-155:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale log message inside the renamed method.
Line 151 still says
"BookLore reader sync"after the parameter was renamedsyncWithGrimmoryReaderon line 131. This is a missed rename within this method's own changed block:✏️ Proposed fix
- log.info("Converted xpointer to CFI for BookLore reader sync: {}", cfi); + log.info("Converted xpointer to CFI for Grimmory reader sync: {}", cfi);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/service/koreader/KoreaderService.java` around lines 131 - 155, The log message in updateProgressData still references "BookLore reader sync" after the parameter was renamed to syncWithGrimmoryReader; update the log statement inside updateProgressData (the line using log.info("Converted xpointer to CFI for BookLore reader sync: {}", cfi)) to reflect the new name/intent (e.g., mention Grimmory or use a neutral message about reader sync) so it matches the syncWithGrimmoryReader parameter and current functionality.backend/src/main/java/org/booklore/config/security/filter/KoreaderAuthFilter.java (1)
48-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTiming attack: replace
equalsIgnoreCasewith a constant-time comparison for the MD5 key.
String.equalsIgnoreCaseshort-circuits on the first mismatched character, leaking timing information about how many characters of the MD5 key matched. An attacker measuring response latency can incrementally reconstruct the secret key. UseMessageDigest.isEqualfor constant-time byte comparison:🔒 Proposed fix
- if (!user.getPasswordMD5().equalsIgnoreCase(key)) { + if (!MessageDigest.isEqual( + user.getPasswordMD5().toLowerCase(Locale.ROOT).getBytes(StandardCharsets.UTF_8), + key.toLowerCase(Locale.ROOT).getBytes(StandardCharsets.UTF_8))) {Add to imports:
import java.security.MessageDigest; import java.nio.charset.StandardCharsets; import java.util.Locale;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/config/security/filter/KoreaderAuthFilter.java` around lines 48 - 52, The code in KoreaderAuthFilter currently uses user.getPasswordMD5().equalsIgnoreCase(key) which leaks timing information; replace this with a constant-time comparison using MessageDigest.isEqual on byte arrays: normalize both strings to the same case (e.g., toLowerCase(Locale.ROOT)), convert to bytes with StandardCharsets.UTF_8, and call MessageDigest.isEqual(expectedBytes, providedBytes) to decide mismatch instead of equalsIgnoreCase; update the conditional around user.getPasswordMD5(), key, and the block that calls chain.doFilter(request, response) accordingly and add imports for java.security.MessageDigest, java.nio.charset.StandardCharsets, and java.util.Locale.
🤖 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
`@backend/src/main/java/org/booklore/config/security/filter/KoreaderAuthFilter.java`:
- Around line 48-52: The code in KoreaderAuthFilter currently uses
user.getPasswordMD5().equalsIgnoreCase(key) which leaks timing information;
replace this with a constant-time comparison using MessageDigest.isEqual on byte
arrays: normalize both strings to the same case (e.g.,
toLowerCase(Locale.ROOT)), convert to bytes with StandardCharsets.UTF_8, and
call MessageDigest.isEqual(expectedBytes, providedBytes) to decide mismatch
instead of equalsIgnoreCase; update the conditional around
user.getPasswordMD5(), key, and the block that calls chain.doFilter(request,
response) accordingly and add imports for java.security.MessageDigest,
java.nio.charset.StandardCharsets, and java.util.Locale.
In `@backend/src/main/java/org/booklore/service/koreader/KoreaderService.java`:
- Around line 131-155: The log message in updateProgressData still references
"BookLore reader sync" after the parameter was renamed to
syncWithGrimmoryReader; update the log statement inside updateProgressData (the
line using log.info("Converted xpointer to CFI for BookLore reader sync: {}",
cfi)) to reflect the new name/intent (e.g., mention Grimmory or use a neutral
message about reader sync) so it matches the syncWithGrimmoryReader parameter
and current functionality.
In `@backend/src/test/java/org/booklore/service/KoreaderUserServiceTest.java`:
- Around line 122-134: Add two unit tests in KoreaderUserServiceTest mirroring
the existing toggleSync tests but targeting toggleSyncProgressWithGrimmory: mock
koreaderUserRepository.findByBookLoreUserId(123L) to return Optional.of(entity),
call service.toggleSyncProgressWithGrimmory(true), assert
entity.isSyncWithGrimmoryReader() is true and verify
koreaderUserRepository.save(entity) was called; and a second test where the
repository returns Optional.empty() and you assertThrows(APIException.class, ()
-> service.toggleSyncProgressWithGrimmory(true)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: c51b5ae0-2874-41a8-a31a-a40601332b0a
📒 Files selected for processing (13)
backend/src/main/java/org/booklore/config/security/filter/KoreaderAuthFilter.javabackend/src/main/java/org/booklore/config/security/userdetails/KoreaderUserDetails.javabackend/src/main/java/org/booklore/controller/KoreaderUserController.javabackend/src/main/java/org/booklore/model/dto/KoreaderUser.javabackend/src/main/java/org/booklore/model/entity/KoreaderUserEntity.javabackend/src/main/java/org/booklore/service/koreader/KoreaderService.javabackend/src/main/java/org/booklore/service/koreader/KoreaderUserService.javabackend/src/main/resources/db/migration/V141__Rename_sync_with_booklore_to_grimmory.sqlbackend/src/test/java/org/booklore/controller/KoreaderUserControllerTest.javabackend/src/test/java/org/booklore/service/KoreaderUserServiceTest.javafrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader-settings-component.tsfrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.spec.tsfrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (17)
backend/src/main/resources/db/migration/*.sql
📄 CodeRabbit inference engine (AGENTS.md)
Add Flyway migrations as new files named V__.sql
Files:
backend/src/main/resources/db/migration/V141__Rename_sync_with_booklore_to_grimmory.sql
**/*
⚙️ 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/resources/db/migration/V141__Rename_sync_with_booklore_to_grimmory.sqlbackend/src/test/java/org/booklore/service/KoreaderUserServiceTest.javabackend/src/main/java/org/booklore/controller/KoreaderUserController.javabackend/src/main/java/org/booklore/service/koreader/KoreaderUserService.javafrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader-settings-component.tsbackend/src/main/java/org/booklore/model/entity/KoreaderUserEntity.javabackend/src/test/java/org/booklore/controller/KoreaderUserControllerTest.javabackend/src/main/java/org/booklore/config/security/userdetails/KoreaderUserDetails.javafrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.spec.tsbackend/src/main/java/org/booklore/model/dto/KoreaderUser.javafrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.tsbackend/src/main/java/org/booklore/service/koreader/KoreaderService.javabackend/src/main/java/org/booklore/config/security/filter/KoreaderAuthFilter.java
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/service/KoreaderUserServiceTest.javabackend/src/main/java/org/booklore/controller/KoreaderUserController.javabackend/src/main/java/org/booklore/service/koreader/KoreaderUserService.javabackend/src/main/java/org/booklore/model/entity/KoreaderUserEntity.javabackend/src/test/java/org/booklore/controller/KoreaderUserControllerTest.javabackend/src/main/java/org/booklore/config/security/userdetails/KoreaderUserDetails.javabackend/src/main/java/org/booklore/model/dto/KoreaderUser.javabackend/src/main/java/org/booklore/service/koreader/KoreaderService.javabackend/src/main/java/org/booklore/config/security/filter/KoreaderAuthFilter.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/service/KoreaderUserServiceTest.javabackend/src/test/java/org/booklore/controller/KoreaderUserControllerTest.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/test/java/org/booklore/service/KoreaderUserServiceTest.javabackend/src/main/java/org/booklore/service/koreader/KoreaderUserService.javabackend/src/main/java/org/booklore/service/koreader/KoreaderService.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/service/KoreaderUserServiceTest.javabackend/src/test/java/org/booklore/controller/KoreaderUserControllerTest.java
**/controller/**/*.java
⚙️ CodeRabbit configuration file
**/controller/**/*.java: Spring Framework 7 REST controller review:
- Prefer
@RestController+@RequestMappingwith ResponseEntity.- Flag missing input validation (
@Valid/@Validatedon request body/params).- Prefer ProblemDetail (RFC 9457) for error responses.
- Flag
@RequestMappingwithout explicit HTTP method constraint.- Check for missing authorization checks on sensitive endpoints.
- Flag any logging of request body that may contain PII or credentials.
Files:
backend/src/main/java/org/booklore/controller/KoreaderUserController.javabackend/src/test/java/org/booklore/controller/KoreaderUserControllerTest.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/settings/device-settings/component/koreader-settings/koreader-settings-component.tsfrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.spec.tsfrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.ts
frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer inject() over constructor injection in frontend code
Files:
frontend/src/app/features/settings/device-settings/component/koreader-settings/koreader-settings-component.tsfrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.spec.tsfrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.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/settings/device-settings/component/koreader-settings/koreader-settings-component.tsfrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.spec.tsfrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.ts
**/entity/**/*.java
⚙️ CodeRabbit configuration file
**/entity/**/*.java: Hibernate 7.3 entity review:
- Jakarta Persistence (jakarta.persistence.*) only; never javax.
- Every
@Entitymust override equals() and hashCode() using business key or@NaturalId.- Flag EAGER fetch on collections; must always be LAZY.
- Flag
@OneToManywithout mappedBy.- Flag missing
@Indexannotations on foreign key columns.- Flag
@Whereusage; use@SQLRestrictioninstead (Hibernate 7.x).- Flag any mutable collection field that is not initialized (e.g., new ArrayList<>()).
Files:
backend/src/main/java/org/booklore/model/entity/KoreaderUserEntity.java
**/security/**/*.java
⚙️ CodeRabbit configuration file
**/security/**/*.java: Security-critical code; maximum scrutiny:
- Check token validation, expiration, and signature verification.
- Flag password hashing not using BCrypt or Argon2.
- Flag hardcoded secrets, tokens, or API keys.
- Verify CSRF protection on state-mutating endpoints.
- Check for timing attack vulnerabilities in comparison logic (use MessageDigest.isEqual or similar constant-time methods).
- Flag missing
@PreAuthorize/@Securedon protected endpoints.
Files:
backend/src/main/java/org/booklore/config/security/userdetails/KoreaderUserDetails.javabackend/src/main/java/org/booklore/config/security/filter/KoreaderAuthFilter.java
**/config/**/*.java
⚙️ CodeRabbit configuration file
**/config/**/*.java: Spring Framework 7 configuration review:
- Flag hardcoded values that should be
@Valueor@ConfigurationProperties.- Flag ObjectMapper bean missing essential modules (JavaTimeModule, etc.).
- Flag RestTemplate bean for new code; prefer RestClient.
- Flag missing
@Primaryor@Qualifieron ambiguous beans.
Files:
backend/src/main/java/org/booklore/config/security/userdetails/KoreaderUserDetails.javabackend/src/main/java/org/booklore/config/security/filter/KoreaderAuthFilter.java
frontend/src/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for tests in frontend code
Files:
frontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.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/settings/device-settings/component/koreader-settings/koreader.service.spec.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/model/dto/KoreaderUser.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/settings/device-settings/component/koreader-settings/koreader.service.ts
🧠 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:
backend/src/test/java/org/booklore/service/KoreaderUserServiceTest.javabackend/src/main/java/org/booklore/controller/KoreaderUserController.javabackend/src/main/java/org/booklore/service/koreader/KoreaderUserService.javabackend/src/main/java/org/booklore/model/entity/KoreaderUserEntity.javabackend/src/test/java/org/booklore/controller/KoreaderUserControllerTest.javabackend/src/main/java/org/booklore/config/security/userdetails/KoreaderUserDetails.javabackend/src/main/java/org/booklore/model/dto/KoreaderUser.javabackend/src/main/java/org/booklore/service/koreader/KoreaderService.javabackend/src/main/java/org/booklore/config/security/filter/KoreaderAuthFilter.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/service/KoreaderUserServiceTest.javabackend/src/main/java/org/booklore/controller/KoreaderUserController.javabackend/src/main/java/org/booklore/service/koreader/KoreaderUserService.javabackend/src/main/java/org/booklore/model/entity/KoreaderUserEntity.javabackend/src/test/java/org/booklore/controller/KoreaderUserControllerTest.javabackend/src/main/java/org/booklore/config/security/userdetails/KoreaderUserDetails.javabackend/src/main/java/org/booklore/model/dto/KoreaderUser.javabackend/src/main/java/org/booklore/service/koreader/KoreaderService.javabackend/src/main/java/org/booklore/config/security/filter/KoreaderAuthFilter.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/service/KoreaderUserServiceTest.javabackend/src/test/java/org/booklore/controller/KoreaderUserControllerTest.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/service/KoreaderUserServiceTest.javabackend/src/test/java/org/booklore/controller/KoreaderUserControllerTest.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/settings/device-settings/component/koreader-settings/koreader-settings-component.tsfrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.spec.tsfrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.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/settings/device-settings/component/koreader-settings/koreader-settings-component.tsfrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.spec.tsfrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.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/settings/device-settings/component/koreader-settings/koreader-settings-component.tsfrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.spec.tsfrontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.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/KoreaderUser.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
[::grimmory-tools/grimmory-docs::]
- docs/integration/koreader.md: contains KOReader integration docs and mentions progress sync and API path (
/api/koreader) used by devices; no references to "syncWithBooklore" or the specific toggle endpoint name. - docs/tools/devices.md: references KOReader Sync configuration and "KOReader API Path".
- docs/integration/opds.md: mentions KOReader progress sync in feature table.
- sidebars.ts: includes integration/koreader doc entry.
Evaluation: repository documents KOReader integration and the API path used by devices, but I found no occurrences of the old "sync_with_booklore" names or the specific controller endpoint paths renamed in the PR. No direct consumers of the renamed DTO fields or controller endpoint were found in this docs repo.
🔇 Additional comments (12)
backend/src/main/java/org/booklore/model/entity/KoreaderUserEntity.java (1)
41-43: LGTM. The column name,nullable = false, and@Builder.Default = falseare consistent with the Flyway migration (V141) and downstream usages across the service and auth filter.backend/src/main/resources/db/migration/V141__Rename_sync_with_booklore_to_grimmory.sql (1)
1-2: LGTM.CHANGE COLUMNwith the repeated type definition is the correct MySQL/MariaDB syntax for a rename-with-type-preservation, andTINYINT(1) NOT NULL DEFAULT 0matches both the original column and the entity's@Builder.Default = false.backend/src/main/java/org/booklore/model/dto/KoreaderUser.java (1)
15-15: LGTM. Renamed field matches the entity and MapStruct will auto-map it without requiring explicit@Mapping.backend/src/main/java/org/booklore/service/koreader/KoreaderUserService.java (1)
74-81: LGTM.@Transactional,ApiError, and constructor injection are all consistent with the codebase conventions, and the logic mirrorstoggleSynccorrectly.backend/src/test/java/org/booklore/service/KoreaderUserServiceTest.java (1)
74-74: LGTM. The mapper lambda correctly usesu.isSyncWithGrimmoryReader(), matching the renamed field.backend/src/main/java/org/booklore/controller/KoreaderUserController.java (1)
54-62: LGTM.@PatchMappingresolves theHttpRequestMethodNotSupportedExceptionfrom issue#191,@PreAuthorizeis consistent with the other endpoints, and the endpoint path matches what the frontend service sends.backend/src/test/java/org/booklore/controller/KoreaderUserControllerTest.java (1)
64-70: LGTM. The new test correctly stubs the void service method, exercises the controller directly, and asserts 204/null body, matching the pattern of the existingupdateSyncEnabled_returnsNoContenttest.frontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.ts (1)
38-42: LGTM. Removing thecatchErrorfallback is correct — the legacy Booklore endpoint is gone, and the component'serror:handler (inonToggleSyncWithGrimmoryReader) covers HTTP errors. The direct PATCH matches the backend path and the frontend test expectation.frontend/src/app/features/settings/device-settings/component/koreader-settings/koreader-settings-component.ts (1)
73-73: LGTM — legacy fallback correctly removed.The
?? falseguard is harmless even though the backend DTO declaressyncWithGrimmoryReaderas a non-nullableboolean, and removing thesyncWithBookloreReaderfallback is the right move now that the backend no longer emits that field.frontend/src/app/features/settings/device-settings/component/koreader-settings/koreader.service.spec.ts (1)
94-109: LGTM — test accurately reflects the single-endpoint contract.The removal of the legacy fallback leg and the single-PATCH-to-grimmory assertion are correct. The
httpTestingController.verify()inafterEachprovides a safety net against unintended extra requests.backend/src/main/java/org/booklore/config/security/filter/KoreaderAuthFilter.java (1)
63-63: LGTM — rename is consistent with entity andKoreaderUserDetailsconstructor.backend/src/main/java/org/booklore/config/security/userdetails/KoreaderUserDetails.java (1)
16-28: LGTM — rename is clean and the full call chain is consistent.
@Getteron abooleanfield generatesisSyncWithGrimmoryReader(), which matches all call sites inKoreaderAuthFilterandKoreaderService.
imnotjames
reviewed
May 4, 2026
…p DB column, deprecate booklore endpoint Apply review feedback from grimmory-tools#410 reopen: - Rename Java field `syncWithGrimmoryReader` → `syncWithWebReader` (clearer long-term naming, decoupled from the product brand). Keep the DB column as `sync_with_booklore_reader` so no migration is required — drops V141__Rename_sync_with_booklore_to_grimmory.sql. - Restore the legacy `PATCH /me/sync-progress-with-booklore` endpoint as a deprecated alias delegating to the new method, so the API change has a deprecation period rather than being cut. Annotated with `@Deprecated(forRemoval = true)` and Swagger `deprecated = true`. - Service method, DTO field, and frontend identifiers renamed to `syncWithWebReader` / `toggleSyncProgressWithWebReader` to match. Frontend continues to call the new `/me/sync-progress-with-grimmory` endpoint URL (unchanged from the previous round).
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
Renames the KOReader sync-progress endpoint, database column, and all corresponding Java/TypeScript field, method, and DTO references from booklore to grimmory. Removes the legacy frontend endpoint fallback and the deprecated syncWithBookloreReader DTO field.
The KOReader endpoint was named /sync-progress-with-booklore and broke with a 405 after the project rename to Grimmory. The frontend already had a partial rename in place, syncWithGrimmoryReader with a catchError fallback to the legacy endpoint and TODO(grimmory-cleanup) comments anticipating this PR. This PR completes that cleanup.
This is a re-submission of #410 after rebase onto current develop, per the closing comment. Descopes #305 to just the rename, per the discussion in #305 (comment), the bidirectional sync (CFI ↔ XPointer conversion, writing web reader progress back to KOReader) is not included.
Linked Issue
Fixes #191
Changes
Backend (backend/)
Frontend (frontend/)
Manual Testing Steps
sync_with_grimmory_reader.
behaviour change, just the rename).
Additional Context
overflow longhand). These reproduce on a clean origin/develop checkout and are unrelated to this change.
AI Disclosure
Claude Code assisted with the rebase onto current develop, resolving the conflict in KoreaderAuthFilter.java, renumbering the Flyway migration from V136 → V141, and drafting the PR description. I reviewed all changes; the logic in this PR is the same rename and cleanup originally authored in #410.
Checklist
Tests
Summary by CodeRabbit
Release Notes
New Features
Deprecations
UI/UX