feat(reader): add epub reader tap navigation for mobile#1909
Open
pixelsquared wants to merge 5 commits into
Open
feat(reader): add epub reader tap navigation for mobile#1909pixelsquared wants to merge 5 commits into
pixelsquared wants to merge 5 commits into
Conversation
Enable single-tap page turning on the EPUB reader's side margins so it works on touch devices / PWA, not just desktop. Tapping the left margin goes to the previous page, the right margin advances, and the center keeps toggling the toolbars. Previously the zone-tap navigation was gated behind a desktop-only check, so on touch every tap fell through to the center branch and only toggled the top bar - and sub-threshold swipes registered as taps, which is the "swiping is clunky and regularly triggers the top bar" complaint from the discussion. The behavior is controlled by a new "Tap to Turn Page" preference (default on) in the EPUB reader settings. Disabling it restores the previous touch behavior (taps toggle the toolbars). - Backend: add Boolean tapToTurnPage to EbookReaderSetting with a default of true (JSON-blob storage, no migration; existing users default on). - Reader state: track tapToTurnPage, read it from user settings, expose a setter; view-manager feeds it to the event service via a callback. - Event service: gate side-zone navigation on the setting instead of the isMobile check. - Settings UI + English i18n for the new toggle. Discussion: grimmory-tools#615
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)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (7)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:
**/*⚙️ CodeRabbit configuration file
Files:
**/*.service.ts⚙️ CodeRabbit configuration file
Files:
frontend/src/**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.spec.ts⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (5)📚 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:
🔇 Additional comments (2)
WalkthroughAdds a ChangesTap to Turn Page Feature
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/app/features/settings/user-management/user.service.ts (1)
176-190: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
tapToTurnPagedeclared required despite backend field being nullable and consumer using a fallback.The backend
EbookReaderSetting.tapToTurnPageis a nullableBoolean(Line 189 inBookLoreUser.java), and the reader-preferences component getter falls back with?? true, implying the value can beundefinedat runtime for existing users predating this setting. Declaring it as a requiredbooleanhere misrepresents the actual contract and is also inconsistent with the optionaltapToTurnPage?: booleaninEbookViewerSetting(book.model.ts). Consider marking it optional here too.🔧 Proposed fix
- tapToTurnPage: boolean; + tapToTurnPage?: boolean;🤖 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 `@frontend/src/app/features/settings/user-management/user.service.ts` around lines 176 - 190, The EbookReaderSetting contract in user.service.ts incorrectly marks tapToTurnPage as required even though the backend field is nullable and the reader-preferences consumer already falls back with ?? true. Update the EbookReaderSetting interface to match the actual runtime contract by making tapToTurnPage optional, consistent with EbookViewerSetting in book.model.ts and the existing fallback behavior in the reader-preferences component.frontend/src/app/features/book/model/book.model.ts (1)
325-339: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInconsistent optionality with
EbookReaderSetting.tapToTurnPage.
EbookViewerSetting.tapToTurnPageis declared optional here, but the siblingEbookReaderSettinginterface inuser.service.ts(Line 189) declares the same field as required (tapToTurnPage: boolean). Since both interfaces model the same persisted setting (existing users' stored settings predate this feature and won't have the field), the required declaration is misleading — consumers reading it as required may skip null/undefined guards. Align both interfaces to the same optionality (likely both optional, matching the nullableBooleanbackend field).🤖 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 `@frontend/src/app/features/book/model/book.model.ts` around lines 325 - 339, The tapToTurnPage setting is modeled with inconsistent optionality between the book and user settings types, so align the declarations in EbookViewerSetting and EbookReaderSetting to the same optional shape. Update the persisted settings types in book.model.ts and the matching interface in user.service.ts so tapToTurnPage is treated as optional/undefined-safe everywhere, and adjust any related consumers to handle the absence of the field.
🤖 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.
Outside diff comments:
In `@frontend/src/app/features/book/model/book.model.ts`:
- Around line 325-339: The tapToTurnPage setting is modeled with inconsistent
optionality between the book and user settings types, so align the declarations
in EbookViewerSetting and EbookReaderSetting to the same optional shape. Update
the persisted settings types in book.model.ts and the matching interface in
user.service.ts so tapToTurnPage is treated as optional/undefined-safe
everywhere, and adjust any related consumers to handle the absence of the field.
In `@frontend/src/app/features/settings/user-management/user.service.ts`:
- Around line 176-190: The EbookReaderSetting contract in user.service.ts
incorrectly marks tapToTurnPage as required even though the backend field is
nullable and the reader-preferences consumer already falls back with ?? true.
Update the EbookReaderSetting interface to match the actual runtime contract by
making tapToTurnPage optional, consistent with EbookViewerSetting in
book.model.ts and the existing fallback behavior in the reader-preferences
component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: b905a8ce-e55f-49ff-b3a3-4f1ad08dcd70
⛔ Files ignored due to path filters (1)
frontend/src/i18n/en/settings-reader.jsonis excluded by!frontend/src/i18n/**
📒 Files selected for processing (12)
backend/src/main/java/org/booklore/model/dto/BookLoreUser.javabackend/src/main/java/org/booklore/service/user/DefaultUserSettingsProvider.javabackend/src/test/java/org/booklore/service/user/DefaultUserSettingsProviderTest.javafrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.tsfrontend/src/app/features/readers/ebook-reader/core/view-manager.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.spec.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.tsfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.htmlfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.tsfrontend/src/app/features/settings/user-management/user.service.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (12)
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/book/model/book.model.tsfrontend/src/app/features/settings/user-management/user.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.spec.tsfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.htmlfrontend/src/app/features/readers/ebook-reader/core/event.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.tsfrontend/src/app/features/readers/ebook-reader/core/view-manager.service.tsfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.ts
frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer inject() over constructor injection in frontend code
Files:
frontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/settings/user-management/user.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.tsfrontend/src/app/features/readers/ebook-reader/core/view-manager.service.tsfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.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/book/model/book.model.tsfrontend/src/app/features/settings/user-management/user.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.spec.tsfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.htmlfrontend/src/app/features/readers/ebook-reader/core/event.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.tsfrontend/src/app/features/readers/ebook-reader/core/view-manager.service.tsfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.ts
**/*
⚙️ 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:
frontend/src/app/features/book/model/book.model.tsbackend/src/main/java/org/booklore/service/user/DefaultUserSettingsProvider.javabackend/src/test/java/org/booklore/service/user/DefaultUserSettingsProviderTest.javabackend/src/main/java/org/booklore/model/dto/BookLoreUser.javafrontend/src/app/features/settings/user-management/user.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.spec.tsfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.htmlfrontend/src/app/features/readers/ebook-reader/core/event.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.tsfrontend/src/app/features/readers/ebook-reader/core/view-manager.service.tsfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.ts
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/service/user/DefaultUserSettingsProvider.javabackend/src/test/java/org/booklore/service/user/DefaultUserSettingsProviderTest.javabackend/src/main/java/org/booklore/model/dto/BookLoreUser.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/service/user/DefaultUserSettingsProvider.javabackend/src/test/java/org/booklore/service/user/DefaultUserSettingsProviderTest.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/user/DefaultUserSettingsProviderTest.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/user/DefaultUserSettingsProviderTest.java
**/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/BookLoreUser.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/user-management/user.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.tsfrontend/src/app/features/readers/ebook-reader/core/view-manager.service.ts
frontend/src/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for tests in frontend code
Files:
frontend/src/app/features/readers/ebook-reader/state/reader-state.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.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/readers/ebook-reader/state/reader-state.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.spec.ts
🧠 Learnings (19)
📚 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/book/model/book.model.tsfrontend/src/app/features/settings/user-management/user.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.tsfrontend/src/app/features/readers/ebook-reader/core/view-manager.service.tsfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.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/book/model/book.model.tsfrontend/src/app/features/settings/user-management/user.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.tsfrontend/src/app/features/readers/ebook-reader/core/view-manager.service.tsfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.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/book/model/book.model.tsfrontend/src/app/features/settings/user-management/user.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.tsfrontend/src/app/features/readers/ebook-reader/core/view-manager.service.tsfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.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/book/model/book.model.tsfrontend/src/app/features/settings/user-management/user.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.tsfrontend/src/app/features/readers/ebook-reader/core/view-manager.service.tsfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.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/book/model/book.model.tsfrontend/src/app/features/settings/user-management/user.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.tsfrontend/src/app/features/readers/ebook-reader/state/reader-state.service.spec.tsfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.htmlfrontend/src/app/features/readers/ebook-reader/core/event.service.spec.tsfrontend/src/app/features/readers/ebook-reader/core/event.service.tsfrontend/src/app/features/readers/ebook-reader/core/view-manager.service.tsfrontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.ts
📚 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/service/user/DefaultUserSettingsProvider.javabackend/src/test/java/org/booklore/service/user/DefaultUserSettingsProviderTest.javabackend/src/main/java/org/booklore/model/dto/BookLoreUser.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/service/user/DefaultUserSettingsProvider.javabackend/src/test/java/org/booklore/service/user/DefaultUserSettingsProviderTest.javabackend/src/main/java/org/booklore/model/dto/BookLoreUser.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/service/user/DefaultUserSettingsProvider.javabackend/src/test/java/org/booklore/service/user/DefaultUserSettingsProviderTest.javabackend/src/main/java/org/booklore/model/dto/BookLoreUser.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/service/user/DefaultUserSettingsProvider.javabackend/src/test/java/org/booklore/service/user/DefaultUserSettingsProviderTest.javabackend/src/main/java/org/booklore/model/dto/BookLoreUser.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/service/user/DefaultUserSettingsProvider.javabackend/src/main/java/org/booklore/model/dto/BookLoreUser.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/service/user/DefaultUserSettingsProvider.javabackend/src/main/java/org/booklore/model/dto/BookLoreUser.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/service/user/DefaultUserSettingsProvider.javabackend/src/main/java/org/booklore/model/dto/BookLoreUser.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/service/user/DefaultUserSettingsProvider.javabackend/src/main/java/org/booklore/model/dto/BookLoreUser.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/service/user/DefaultUserSettingsProvider.javabackend/src/main/java/org/booklore/model/dto/BookLoreUser.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/service/user/DefaultUserSettingsProvider.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/user/DefaultUserSettingsProviderTest.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/service/user/DefaultUserSettingsProviderTest.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/user/DefaultUserSettingsProviderTest.java
📚 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/BookLoreUser.java
🔇 Additional comments (11)
backend/src/main/java/org/booklore/model/dto/BookLoreUser.java (1)
176-190: LGTM!backend/src/main/java/org/booklore/service/user/DefaultUserSettingsProvider.java (1)
80-96: LGTM!backend/src/test/java/org/booklore/service/user/DefaultUserSettingsProviderTest.java (1)
1-29: LGTM!frontend/src/app/features/readers/ebook-reader/state/reader-state.service.ts (1)
22-22: LGTM!Also applies to: 63-63, 136-136, 243-246
frontend/src/app/features/readers/ebook-reader/state/reader-state.service.spec.ts (1)
79-93: LGTM!Also applies to: 185-208
frontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.ts (1)
194-202: LGTM!frontend/src/app/features/settings/reader-preferences/epub-reader-preferences/epub-reader-preferences-component.html (1)
291-322: 📐 Maintainability & Code QualityNo translation keys are missing here.
navigation,tapToTurnPage, andtapToTurnPageDescare already present infrontend/src/i18n/en/settings-reader.json, andenis the Transloco fallback lang.> Likely an incorrect or invalid review comment.frontend/src/app/features/readers/ebook-reader/core/event.service.ts (2)
76-82: LGTM!
595-626: 🎯 Functional CorrectnessNo desktop regression The shared iframe-click path now routes both touch and mouse margin clicks through
tapToTurnPage, so disabling the setting consistently falls back tomiddle-single-tap.> Likely an incorrect or invalid review comment.frontend/src/app/features/readers/ebook-reader/core/event.service.spec.ts (1)
67-68: LGTM!Also applies to: 135-136, 187-187, 290-345
frontend/src/app/features/readers/ebook-reader/core/view-manager.service.ts (1)
8-8: LGTM!Also applies to: 112-112, 130-131
zachyale
changed the title
Existing users' stored ebookReaderSetting JSON predates tapToTurnPage, so at runtime the field can be undefined. Type it as optional on the frontend EbookReaderSetting interface (matching EbookViewerSetting) - the settings getter already defaults it to true. Add the field to the ReaderState mocks in style.service.spec.ts so the strict AOT test build type-checks.
Contributor
|
Isn't this fairly standard behavior for an e-reader? Why would we gate this behind an option? |
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
The EPUB reader supports tapping the side margins to turn pages, but the zone-tap handler was gated to desktop only. On touch devices / the PWA every tap fell through to the center "toggle toolbars" branch, so pages could only be turned by swiping and sub-threshold swipes registered as taps and toggled the top bar instead.
This PR enables side-margin tap navigation on touch devices too: tap the left margin for the previous page, the right margin for the next page, and the center to toggle the toolbars. The behavior is controlled by a new "Tap to Turn Page" preference (default on). Disabling the option restores the previous touch behavior where taps toggle the toolbars.
Linked Issue
Fixes #1876
Changes
Boolean tapToTurnPagetoEbookReaderSettingwith a default oftrueinDefaultUserSettingsProvider. Stored in the existing JSON settings blob — no Flyway migration; existing users default to enabled.tapToTurnPageinReaderState, read it from user settings on init (null-safe, defaults on), and expose a setter.isTapToTurnEnabledcallback instead of theisMobilecheck.Manual Testing Steps
Screenshots (Optional)
Additional Context
The middle tap zone (30%–70%) and the 50px swipe threshold are unchanged.
This feature defaults to "on" if we don't want to change default behavior let me know and I can change the default.
AI Disclosure
Claude Code (Anthropic, Opus) assisted with Java code and helping me understanding the codebase.
I reviewed all changes.
Checklist
just ui checkandjust api check.Summary by CodeRabbit