feat(pdf-reader): PDF reader for vertical/infinite scroll layouts and improved responsiveness#841
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds persistent vertical/horizontal PDF scroll-layout support across backend DTOs, frontend models, reader component, embed service/frame, and settings UI; includes UI toggles, iframe postMessage plumbing for scroll/pan, rendering/tiling/DPR adjustments, accessibility tweaks, and concurrency-safe close flow. Changes
Sequence DiagramsequenceDiagram
participant User
participant SettingsUI
participant UserService
participant PdfReaderComponent
participant EmbedPdfBookService
participant EmbedFrame
User->>SettingsUI: choose scroll layout
SettingsUI->>UserService: updatePreference(scrollLayout)
UserService-->>SettingsUI: persisted
User->>PdfReaderComponent: toggle/cycle scroll layout
PdfReaderComponent->>EmbedPdfBookService: setScrollLayout(layout)
EmbedPdfBookService->>EmbedFrame: postMessage({type: 'setScrollLayout', layout})
EmbedFrame->>EmbedFrame: apply plugin layout and recalc
EmbedFrame-->>EmbedPdfBookService: layout applied (ack)
EmbedPdfBookService-->>PdfReaderComponent: confirm applied
PdfReaderComponent->>User: update UI (class/signal)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.scss (1)
560-592: Minor: disabled opacity inconsistent with.icon-btn.
.icon-btn:disabledusesopacity: 0.65(Line 166) while.embed-close-btn:disabledusesopacity: 0.8. Consider aligning the two if the same visual disabled treatment is intended across close buttons.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.scss` around lines 560 - 592, The disabled opacity for .embed-close-btn is inconsistent with .icon-btn (:disabled uses opacity: 0.65 at .icon-btn) — make them consistent by updating the .embed-close-btn:disabled rule to use the same opacity value as .icon-btn:disabled (or vice versa if you prefer the 0.8 value), ensuring both the .embed-close-btn and .icon-btn disabled styles match for a consistent visual treatment.frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html (1)
165-169: Statictitlevs. state-dependentaria-label.
[title]resolves to a single "Scroll layout" string while[attr.aria-label]announces the current mode (horizontal/vertical). Sighted users and screen-reader users will get different descriptions on the same control. Consider binding[title]to the same state-dependent expression used foraria-labelfor consistency.♻️ Suggested alignment
- <button class="icon-btn" - [title]="'readerPdf.toolbar.scrollLayout' | transloco" - [attr.aria-label]="scrollLayout() === 'horizontal' ? ('readerPdf.toolbar.scrollLayoutHorizontal' | transloco) : ('readerPdf.toolbar.scrollLayoutVertical' | transloco)" + <button class="icon-btn" + [title]="scrollLayout() === 'horizontal' ? ('readerPdf.toolbar.scrollLayoutHorizontal' | transloco) : ('readerPdf.toolbar.scrollLayoutVertical' | transloco)" + [attr.aria-label]="scrollLayout() === 'horizontal' ? ('readerPdf.toolbar.scrollLayoutHorizontal' | transloco) : ('readerPdf.toolbar.scrollLayoutVertical' | transloco)" [attr.aria-pressed]="scrollLayout() === 'horizontal'" (click)="cycleScrollLayout()">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html` around lines 165 - 169, The button's visual tooltip ([title]) is static while the accessible label ([attr.aria-label]) reflects the current state; update the title binding to use the same state-dependent expression as aria-label so sighted and screen-reader users see the same description. Locate the button that calls cycleScrollLayout() and uses scrollLayout(), and replace the literal 'readerPdf.toolbar.scrollLayout' title binding with the ternary/transloco expression currently used in [attr.aria-label] (the same logic that checks scrollLayout() === 'horizontal' and selects 'readerPdf.toolbar.scrollLayoutHorizontal' or 'readerPdf.toolbar.scrollLayoutVertical'). Ensure both [title] and [attr.aria-label] remain in sync when scrollLayout() changes.frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (1)
466-466: Drop leftover dev comment.
// Added thishas no semantic value and looks like a stray note.🧹 Cleanup
- this.suppressProgressSave = true; // Added this - + this.suppressProgressSave = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts` at line 466, Remove the stray developer comment after the assignment to this.suppressProgressSave in the PDF reader component; keep the statement this.suppressProgressSave = true; but delete the inline comment "// Added this" so the code reads plainly and has no leftover dev note in pdf-reader.component.ts (locate the assignment in the PDFReaderComponent where suppressProgressSave is set).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html`:
- Around line 164-181: The horizontal-scroll icon in the PDF toolbar uses the
same two-column SVG as the spread-mode icon, making the scrollLayout() state
indistinguishable; update the template in pdf-reader.component.html where the
button bound to scrollLayout() and (click)="cycleScrollLayout()" renders the
horizontal branch to use a distinct glyph (for example replace the two-rect SVG
with a right-pointing chevron or horizontal-arrow SVG) and make the same change
in the mobile overflow menu branch so the "horizontal" visual differs from the
spread/odd-even icon everywhere it is used.
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 390-400: The per-book branch currently forces scrollLayoutVal to
'vertical' unless the per-book setting is 'horizontal'; change it to fall back
to the user's global setting like zoomVal and spreadVal do: read a rawScroll
value from pdfPrefs.pdfSettings?.scrollLayout ??
myself.userSettings.pdfReaderSetting.scrollLayout || 'vertical' and then set
scrollLayoutVal = rawScroll === 'horizontal' ? 'horizontal' : 'vertical' (leave
the isDarkTheme.set call as-is).
In
`@frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.ts`:
- Around line 127-132: The ternary for defaultImageQuality is redundant
(defaultImageQuality: isSmallViewport ? 0.92 : 0.92); decide whether mobile
should use a different value (like 0.85 used in embedpdf-frame.html) or drop the
conditional. Update the assignment in embedpdf-book.service.ts to either use a
single constant (e.g., defaultImageQuality: 0.92) or change the mobile branch to
the intended mobile quality (e.g., defaultImageQuality: isSmallViewport ? 0.85 :
0.92), and keep getTilingConfig() and surrounding render config untouched.
- Around line 436-477: The applyScrollLayout and readScrollLayoutFromPlugin
methods silently do nothing when the scroll plugin doesn't expose any of the
probed APIs; add a warning log to surface this by detecting the exhausted
fallback chain in both functions (after checking setScrollStrategy,
setScrollMode, setLayoutMode) and calling console.warn with a clear message that
the scroll plugin lacks supported APIs (mentioning setScrollStrategy as the
documented API and fallback methods setScrollMode/setLayoutMode), referencing
the local variable scrollLayoutCap and the methods
setScrollStrategy/setScrollMode/setLayoutMode so reviewers can locate where to
insert the warning; keep the warning consistent with the service's existing
annotation plugin logging style.
In `@frontend/src/assets/embedpdf-frame.html`:
- Around line 17-26: The DPR logic in the iframe (variables isSmallViewport,
currentDpr, targetDpr in embedpdf-frame.html) differs from EmbedPdfBookService
(embedpdf-book.service.ts) and should be aligned; update the targetDpr
calculation in embedpdf-frame.html to match the service's rule (use targetDpr =
isSmallViewport ? 2 : Math.max(currentDpr, 2.5)) so both viewers force DPR=2 on
small viewports and use at least 2.5 on large, or if you intentionally want the
iframe to allow DPR <2 add a clear comment documenting the deliberate asymmetry
and why it differs from the service.
- Line 5: Update the viewport meta tag in embedpdf-frame.html to remove the
scaling restrictions that block pinch/zoom: replace the current <meta
name="viewport" content="width=device-width, initial-scale=1.0,
maximum-scale=1.0, user-scalable=no"> usage with an unrestricted tag such as
<meta name="viewport" content="width=device-width, initial-scale=1"> so
browser/native zoom works and WCAG 1.4.4 is satisfied; if EmbedPDF intentionally
provides alternate zoom controls, instead ensure those controls (in the EmbedPDF
UI) support 200%+ scaling and are documented and accessible rather than keeping
maximum-scale/user-scalable settings.
---
Nitpick comments:
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html`:
- Around line 165-169: The button's visual tooltip ([title]) is static while the
accessible label ([attr.aria-label]) reflects the current state; update the
title binding to use the same state-dependent expression as aria-label so
sighted and screen-reader users see the same description. Locate the button that
calls cycleScrollLayout() and uses scrollLayout(), and replace the literal
'readerPdf.toolbar.scrollLayout' title binding with the ternary/transloco
expression currently used in [attr.aria-label] (the same logic that checks
scrollLayout() === 'horizontal' and selects
'readerPdf.toolbar.scrollLayoutHorizontal' or
'readerPdf.toolbar.scrollLayoutVertical'). Ensure both [title] and
[attr.aria-label] remain in sync when scrollLayout() changes.
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.scss`:
- Around line 560-592: The disabled opacity for .embed-close-btn is inconsistent
with .icon-btn (:disabled uses opacity: 0.65 at .icon-btn) — make them
consistent by updating the .embed-close-btn:disabled rule to use the same
opacity value as .icon-btn:disabled (or vice versa if you prefer the 0.8 value),
ensuring both the .embed-close-btn and .icon-btn disabled styles match for a
consistent visual treatment.
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Line 466: Remove the stray developer comment after the assignment to
this.suppressProgressSave in the PDF reader component; keep the statement
this.suppressProgressSave = true; but delete the inline comment "// Added this"
so the code reads plainly and has no leftover dev note in
pdf-reader.component.ts (locate the assignment in the PDFReaderComponent where
suppressProgressSave is set).
🪄 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: CHILL
Plan: Pro
Run ID: de911865-7f2f-4564-8643-08df2c182ea0
⛔ Files ignored due to path filters (2)
frontend/src/i18n/en/reader-pdf.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/en/settings-reader.jsonis excluded by!frontend/src/i18n/**
📒 Files selected for processing (10)
backend/src/main/java/org/booklore/model/dto/BookLoreUser.javafrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.htmlfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.scssfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/settings/reader-preferences/pdf-reader-preferences/pdf-reader-preferences-component.htmlfrontend/src/app/features/settings/reader-preferences/pdf-reader-preferences/pdf-reader-preferences-component.tsfrontend/src/app/features/settings/user-management/user.service.tsfrontend/src/assets/embedpdf-frame.html
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (5)
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/model/dto/BookLoreUser.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/reader-preferences/pdf-reader-preferences/pdf-reader-preferences-component.htmlfrontend/src/app/features/settings/user-management/user.service.tsfrontend/src/app/features/settings/reader-preferences/pdf-reader-preferences/pdf-reader-preferences-component.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.scssfrontend/src/assets/embedpdf-frame.htmlfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.htmlfrontend/src/app/features/readers/pdf-reader/pdf-reader.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/settings/reader-preferences/pdf-reader-preferences/pdf-reader-preferences-component.htmlfrontend/src/app/features/settings/user-management/user.service.tsfrontend/src/app/features/settings/reader-preferences/pdf-reader-preferences/pdf-reader-preferences-component.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/assets/embedpdf-frame.htmlfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.htmlfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer inject() over constructor injection in frontend code
Files:
frontend/src/app/features/settings/user-management/user.service.tsfrontend/src/app/features/settings/reader-preferences/pdf-reader-preferences/pdf-reader-preferences-component.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
frontend/src/**/*.component.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep Angular code on standalone components. Do not add NgModules in frontend code
Files:
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
🧠 Learnings (9)
📚 Learning: 2026-03-31T19:52:51.386Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 306
File: frontend/src/app/features/settings/user-management/user.service.ts:64-75
Timestamp: 2026-03-31T19:52:51.386Z
Learning: In `grimmory-tools/grimmory`, CBX reader settings (including `CbxPageViewMode`, `CbxPageSplitOption`, `CbxScrollMode`, etc.) are stored as a raw JSON string blob in the `user_settings` table under a setting key (e.g., `CBX_READER_SETTING`). The backend does not deserialize these values into typed Java enum instances, so adding new frontend enum values (e.g., `TWO_PAGE_REVERSED`, `CbxPageSplitOption` members) does NOT cause Jackson `InvalidFormatException` errors. Do not flag frontend-only enum additions as backend serialization issues for these settings.
Applied to files:
backend/src/main/java/org/booklore/model/dto/BookLoreUser.java
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
backend/src/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/model/dto/BookLoreUser.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/user-management/user.service.tsfrontend/src/app/features/settings/reader-preferences/pdf-reader-preferences/pdf-reader-preferences-component.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.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/settings/user-management/user.service.tsfrontend/src/app/features/settings/reader-preferences/pdf-reader-preferences/pdf-reader-preferences-component.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.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/settings/user-management/user.service.tsfrontend/src/app/features/settings/reader-preferences/pdf-reader-preferences/pdf-reader-preferences-component.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
📚 Learning: 2026-03-31T19:52:06.262Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 306
File: frontend/src/app/features/readers/cbx-reader/cbx-reader.component.ts:787-810
Timestamp: 2026-03-31T19:52:06.262Z
Learning: In `frontend/src/app/features/readers/cbx-reader/cbx-reader.component.ts`, the `doublePairs` field (type `DoublePairs = Record<number, number>`) maps right-page indices to their paired left-page indices. By design, `currentPage` always points to the left (lower-indexed) page of any spread pair — backward navigation enforces this via `Math.min(targetPage, pairedWith)` and `goToPage`/`alignCurrentPageToParity` also preserve this invariant. Therefore, looking up `doublePairs[currentPage]` in forward navigation will only ever encounter left-page keys, and there is no scenario where `currentPage` would be a right-page key.
Applied to files:
frontend/src/app/features/settings/reader-preferences/pdf-reader-preferences/pdf-reader-preferences-component.ts
📚 Learning: 2026-04-04T14:03:28.295Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 372
File: frontend/src/app/features/book/service/app-books-api.service.ts:150-178
Timestamp: 2026-04-04T14:03:28.295Z
Learning: In `frontend/src/app/features/book/service/app-books-api.service.ts`, the `summaryToBook` function intentionally uses `as unknown as Book` to cast a partial object to the `Book` type. The returned object omits some required `Book` fields (e.g. `metadata.bookId`, `pdfProgress.page`) and includes an extra `bookFiles: []` property not in the `Book` interface. This is by design — consumers of `AppBooksApiService.books()` (e.g. `BookCardComponent`) only access the subset of properties that are populated from `AppBookSummary`, so no runtime issues occur. Do not flag this as a type safety error.
Applied to files:
frontend/src/app/features/book/model/book.model.ts
📚 Learning: 2026-04-22T01:56:39.495Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-22T01:56:39.495Z
Learning: Applies to frontend/src/**/*.component.ts : Keep Angular code on standalone components. Do not add NgModules in frontend code
Applied to files:
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
🔀 Multi-repo context grimmory-tools/grimmory-docs
Findings
- Documentation references PDF reader and reader preferences; user-facing docs should be reviewed to include the new "Scroll layout" preference and any UX/behavior notes introduced by the PR.
- docs/readers/pdf-reader.md — general PDF reader overview and "Scroll Modes" section (lines shown above include "Horizontal Scrolling", etc.). [::grimmory-tools/grimmory-docs::]
- docs/reader-preferences.md — "PDF Reader: Default Settings" and settings table; mentions "Streaming PDF Reader Settings" and scroll-related settings (matches where the new scrollLayout preference belongs). [::grimmory-tools/grimmory-docs::]
Implication: update docs/readers/pdf-reader.md and docs/reader-preferences.md to document the new Scroll layout toggle (values 'vertical' | 'horizontal'), toolbar/control changes, and any behavior differences (embed/layout notes) so end users see the new option.
🔇 Additional comments (12)
backend/src/main/java/org/booklore/model/dto/BookLoreUser.java (1)
193-193: LGTM!The new
scrollLayoutString field mirrors the existingpageSpread/pageZoompattern inPdfReaderSettingand integrates cleanly with the Lombok-generated accessors for JSON serialization.frontend/src/app/features/settings/user-management/user.service.ts (1)
130-135: LGTM!Optional
scrollLayoutunion literal aligns with the backend DTO String field and the consumer inpdf-reader-preferences-component.ts. Defaults cleanly via the|| 'vertical'fallback at the consumer side.frontend/src/app/features/book/model/book.model.ts (1)
306-311: LGTM!Per-book
PdfViewerSetting.scrollLayoutmirrors the user-levelPdfReaderSetting.scrollLayout, enabling the per-book override that the reader applies on initialization.frontend/src/app/features/settings/reader-preferences/pdf-reader-preferences/pdf-reader-preferences-component.html (1)
53-76: LGTM!The new scroll layout setting block mirrors the existing spread/zoom structure and properly uses Transloco keys for label, tooltip, and description, keeping user-facing strings in i18n as per coding guidelines.
frontend/src/app/features/settings/reader-preferences/pdf-reader-preferences/pdf-reader-preferences-component.ts (1)
36-66: LGTM!
scrollLayoutsoptions and theselectedScrollLayoutaccessor pair mirror the existing spread/zoom patterns. The|| 'vertical'fallback correctly handles the optionalscrollLayoutfromPdfReaderSetting.frontend/src/app/features/readers/pdf-reader/pdf-reader.component.scss (1)
297-303: LGTM!The
.book-viewer-containercleanly handles both vertical default and horizontal scrolling via the.horizontal-scrollmodifier, matching the component’s[class.horizontal-scroll]binding and the scroll-layout signal.frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.ts (2)
230-240: LGTM!
setScrollLayoutcorrectly captures the current page before switching, applies the new layout, and restores the page after letting the engine recalc.getScrollLayoutfalls back to the cached field when the plugin doesn't expose a read API.
144-147: The user preference wiring is correct and will not cause unexpected overrides.At init, the service applies the default
'vertical'layout (line 147 of embedpdf-book.service.ts). However,PdfReaderComponentcorrectly seeds the user preference immediately afterembedPdfBook.init()completes (line 475:this.embedPdfBook.setScrollLayout(this.scrollLayout())), which is synchronous and happens before any further async operations. The user preference is loaded and set in the component signal (line 403) before the viewer initialization is scheduled viaafterNextRender()(line 415), ensuring the correct layout value is applied right after init. No brief override issue will occur.frontend/src/assets/embedpdf-frame.html (2)
464-490: LGTM!The
setScrollLayouthandler mirrors the service-side fallback chain and preserves the current page across the layout switch.setPanModeis a clean pass-through to the pan plugin.
271-278: LGTM!Centering via
[class*="bg-bg-app"]flex + childmargin: autokeeps PDFs visually centered when narrower than the viewport, and matches the corresponding rule added inEmbedPdfBookService.injectBookModeStyles. Note the selector is attribute-substring based and thus tightly coupled to the EmbedPDF internal class naming — future library upgrades may require revisiting this rule.frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (1)
15-15: No action needed —PdfScrollLayouttype andsetScrollLayoutmethod are properly exported.
PdfScrollLayoutis correctly exported fromembedpdf-book.service.ts(line 28) asexport type PdfScrollLayout = 'vertical' | 'horizontal';, andsetScrollLayout(layout: PdfScrollLayout): voidis implemented on the service (line 230). The component's import and method calls are valid.frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html (1)
166-167:⚠️ Potential issue | 🟠 MajorAdd missing Transloco keys for scroll layout toolbar button to all locale bundles.
The three new keys—
readerPdf.toolbar.scrollLayout,readerPdf.toolbar.scrollLayoutHorizontal, andreaderPdf.toolbar.scrollLayoutVertical—are not defined in any of the i18n JSON files underfrontend/src/i18n/. Users will see raw key strings in the button title, aria-label, and button text across all language locales.Add these keys to
frontend/src/i18n/{locale}/reader-pdf.jsonfor all supported locales (da, de, en, es, fr, hr, hu, id, it, ja, nl, etc.) with appropriate translated values.Also applies to: 325
⛔ Skipped due to learnings
Learnt from: alexhb1 Repo: grimmory-tools/grimmory PR: 307 File: frontend/src/i18n/hr/layout.json:3-3 Timestamp: 2026-03-31T14:12:39.521Z Learning: In `frontend/src/i18n/hr/layout.json` (Croatian locale), newly added i18n keys may intentionally use English placeholder values (e.g., `toggleNavigation`, `adjustSidebarWidth`, `closeNavigation`). Do not flag these as missing Croatian translations — this is a deliberate placeholder approach by the team.
There was a problem hiding this comment.
Actionable comments posted: 2
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/readers/pdf-reader/pdf-reader.component.html (1)
276-304:⚠️ Potential issue | 🟡 MinorMobile overflow tool entries lack the
activestate that the desktop counterparts expose.In the desktop toolbar every tool toggle sets
[class.active]+[attr.aria-pressed]based onactiveAnnotationTool()/isSearchOpen()/ etc., but the mobile overflow menu items for search (Line 276), highlight (Line 283), freeText (Line 290) and ink (Line 297) render without any state binding — only the pan entry on Line 304 does. Result: when a user opens the overflow while a tool is already active (e.g. highlight on), the menu shows no indication, and toggling the same entry will turn the tool off with no visual cue of what just happened. The scroll-layout entry (Line 313) has the same issue; the icon updates, but aria-pressed / active-class do not.Minimal fix is to add
[class.active](and ideally[attr.aria-pressed]) on each, mirroring the desktop bindings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html` around lines 276 - 304, The mobile overflow buttons for search and annotation tools lack the active state bindings; update each overflow button that calls toggleSearch() and toggleAnnotationTool('highlight'|'freeText'|'ink') (and the scroll-layout button) to mirror the desktop toolbar by adding [class.active] and [attr.aria-pressed] bindings using the same predicates used on desktop (e.g. use isSearchOpen() for the search button, activeAnnotationTool('highlight') / activeAnnotationTool('freeText') / activeAnnotationTool('ink') for the annotation buttons, and the equivalent predicate for scroll-layout), so the UI shows the current active state and the aria-pressed attribute reflects it.frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (1)
1024-1033:⚠️ Potential issue | 🟠 MajorSwitching book→document mid-read jumps back to the saved initial page instead of the current page.
initialPageis set once inngOnInitfrompdfMeta.pdfProgress?.pageand is never updated as the user navigates. So if a user opens the book on saved page 5, reads to page 50, then toggles to the document viewer,documentOpenedusesthis.initialPage(= 5) and (a) only scrolls wheninitialPage > 1, and (b) scrolls to the wrong page when it does — they land on page 5 instead of page 50.The switch target should be the live
this.page()(and the> 1guard should be dropped — scrolling to page 1 is still correct).🐛 Proposed fix
case 'documentOpened': // Scroll to the page the user was on in book mode and re-enable progress saving - if (this.embedPdfIframe?.contentWindow && this.initialPage > 1) { + const targetPage = this.page(); + if (this.embedPdfIframe?.contentWindow && targetPage > 1) { this.embedPdfIframe.contentWindow.postMessage( - { type: 'scrollToPage', pageNumber: this.initialPage }, + { type: 'scrollToPage', pageNumber: targetPage }, location.origin ); } setTimeout(() => { this.suppressProgressSave = false; }, 500); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts` around lines 1024 - 1033, The handler for the 'documentOpened' case uses the stale this.initialPage and a guard that skips scrolling for page 1; update it to use the live page getter this.page() instead of this.initialPage, remove the "if (… > 1)" check so scrolling to page 1 is allowed, and post the message with pageNumber: this.page(); keep the existing suppressProgressSave setTimeout unchanged.
🧹 Nitpick comments (5)
frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.ts (2)
229-235: PendingsetTimeoutis not cleared ondestroy().The 80ms timeout in
setScrollLayoutisn't tracked and cleared indestroy(). If the user closes the reader betweensetScrollLayoutand timer fire,scrollToPageruns after teardown.this.scrollis nulled so the optional chain makes it benign, but cleanup is still the right pattern — stash the handle andclearTimeouton destroy (similar to how the reader component tracksinitTimeout).♻️ Proposed cleanup
+ private scrollLayoutTimer?: ReturnType<typeof setTimeout>; + setScrollLayout(layout: PdfScrollLayout): void { const page = this.currentPage; this.scrollLayout = layout; this.applyScrollLayout(layout); - // Use a short delay to allow the layout engine to recalculate before restoring position - setTimeout(() => this.scrollToPage(page, 'instant'), 80); + // Use a short delay to allow the layout engine to recalculate before restoring position + if (this.scrollLayoutTimer) clearTimeout(this.scrollLayoutTimer); + this.scrollLayoutTimer = setTimeout(() => { + this.scrollLayoutTimer = undefined; + this.scrollToPage(page, 'instant'); + }, 80); }And in
destroy():+ if (this.scrollLayoutTimer) { + clearTimeout(this.scrollLayoutTimer); + this.scrollLayoutTimer = undefined; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.ts` around lines 229 - 235, The setTimeout in setScrollLayout sets a delayed call to scrollToPage but its timer ID isn't stored or cleared; add a class property (e.g. layoutTimeout or reuse initTimeout pattern) to capture the return value of setTimeout inside setScrollLayout, and then clearTimeout(this.layoutTimeout) and null it inside destroy() so the pending callback cannot run after teardown; update references to setScrollLayout, scrollToPage and destroy to use the stored timer handle.
557-562: Small-viewport tiling is inconsistent between book service and iframe.
EmbedPdfBookService.getTilingConfig()uses{tileSize: 640, ..., extraRings: 1}on small viewports, whileembedpdf-frame.htmluses{tileSize: 512, ..., extraRings: 0}on the same breakpoint. Combined with the DPR-2 force, the book viewer renders materially more pixels per tile (~1.56× the tile area) and keeps an extra ring — i.e. the book viewer will noticeably stress mobile image-memory budgets while the doc iframe deliberately avoids that. If the asymmetry is intentional (richer annotation layer in book mode), a code comment documenting it would prevent future "let's align them" regressions; otherwise consider matching the iframe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.ts` around lines 557 - 562, EmbedPdfBookService.getTilingConfig currently returns {tileSize: 640, overlapPx: 2, extraRings: 1} for small viewports while embedpdf-frame.html uses {tileSize: 512, overlapPx: 2, extraRings: 0}, causing inconsistent tile area under DPR=2; either align EmbedPdfBookService.getTilingConfig to match the iframe values (tileSize: 512, overlapPx: 2, extraRings: 0) to avoid extra memory/tiles on mobile, or if the larger tile and extra ring are intentional for annotation richness, add a concise code comment in EmbedPdfBookService.getTilingConfig referencing embedpdf-frame.html and DPR-2 to document the deliberate asymmetry so future maintainers don’t try to "align them".frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (2)
398-405: NarrowscrollLayoutValto a validPdfScrollLayoutbefore assigning.
myself.userSettings.pdfReaderSetting.scrollLayoutandpdfPrefs.pdfSettings?.scrollLayoutarrive from the backend/stored settings and are typed asPdfScrollLayout | undefined, but nothing validates the value at runtime. If stale data or a future backend rename ever surfaces a different string, the||chain will propagate it intothis.scrollLayoutand onward tosetScrollLayout— the API probing inEmbedPdfBookServicewill then hand a bogus string tosetScrollStrategy. Consider a small coercion (matching thespreadVal/zoomValpattern a few lines above):- scrollLayoutVal = pdfPrefs.pdfSettings?.scrollLayout - || myself.userSettings.pdfReaderSetting.scrollLayout - || 'vertical'; + const rawScroll = pdfPrefs.pdfSettings?.scrollLayout + || myself.userSettings.pdfReaderSetting.scrollLayout + || 'vertical'; + scrollLayoutVal = rawScroll === 'horizontal' ? 'horizontal' : 'vertical';(And the analogous coercion on Line 393.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts` around lines 398 - 405, Narrow the incoming scroll layout to a valid PdfScrollLayout before assigning: validate/coerce the values coming from pdfPrefs.pdfSettings?.scrollLayout and myself.userSettings.pdfReaderSetting.scrollLayout (which are PdfScrollLayout | undefined) into an allowed PdfScrollLayout enum/string set (similar to the spreadVal/zoomVal pattern used nearby) and then assign the sanitized value to scrollLayoutVal and call this.scrollLayout.set(sanitizedValue); also apply the same coercion for the earlier occurrence (the analogous assignment mentioned around the other block) so that downstream callers like EmbedPdfBookService.setScrollStrategy only receive validated PdfScrollLayout values.
1413-1451:closeReader/performCloseReaderflow looks good — two small notes.The
closeReaderPromisesingleton +isClosingReadersignal cleanly prevent the double-close race, and the pointer-vs-click split correctly avoids duplicate invocations while keeping touch/pen responsiveness. Two minor points worth considering:
performCloseReaderawaitspersistAnnotations, butpersistAnnotationsonly initiates afetch().then(...)and returns as soon as the export finishes — the actual HTTP save still happens in the background, afterperformCloseReaderhas already navigated away. If saving-on-close should be best-effort, this is fine; if it must complete before navigation,persistAnnotationsneeds to await the fetch response.- Inside
performCloseReader,this.location.back()/this.router.navigate(['/'])will destroy the component, after whichresetCloseReaderState()writes to a signal on a destroyed instance. It's benign with signals, but you could just skip the reset once navigation is kicked off to avoid the dead-store.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts` around lines 1413 - 1451, performCloseReader currently awaits persistAnnotations but persistAnnotations only starts a fetch and returns before the HTTP save completes; update persistAnnotations so it returns a Promise that awaits the fetch/response (and throws or resolves based on response.ok) so performCloseReader truly waits for the save when required. Also avoid writing to component signals after navigation: in performCloseReader, detect when you call this.location.back() or this.router.navigate(['/']) (i.e., navigation kicked off) and skip calling resetCloseReaderState() afterward (or only call resetCloseReaderState() when navigation was not initiated) to prevent dead-store writes to isClosingReader/closeReaderPromise on a destroyed instance; reference functions: persistAnnotations, performCloseReader, resetCloseReaderState, closeReaderPromise, isClosingReader, location.back, router.navigate.frontend/src/assets/embedpdf-frame.html (1)
271-278: Same[class*="bg-bg-app"]centering rule duplicated from book-mode service.This CSS block is a verbatim duplicate of the one injected by
EmbedPdfBookService.injectBookModeStyles()and carries the same concerns: it targets a library-internal utility class by substring match, forcesflex-direction:columnon any matching node, and thenmargin:autos every direct child. Verify this still behaves correctly in horizontal scroll layout (mixing column-flex parent + auto-margin children is visually contradictory when pages are meant to lay out in a row). If the rule is genuinely shared between book and doc viewers, it would be cleaner to load it from a single stylesheet (or inline-style constant) than to keep two sources of truth in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/assets/embedpdf-frame.html` around lines 271 - 278, The duplicated CSS block targeting [class*="bg-bg-app"] (which forces display:flex; flex-direction:column; and margin:auto on children) is already injected by EmbedPdfBookService.injectBookModeStyles() and can conflict with horizontal-scroll layouts; remove this duplicate from embedpdf-frame.html, or restrict it so it only applies in true book-mode (e.g., under a .book-mode or similar explicit class) instead of a substring class selector, and centralize the rule into a single shared stylesheet or exported inline-style constant used by both EmbedPdfBookService.injectBookModeStyles() and the frame so there is one source of truth and horizontal row layouts aren’t broken by an unguarded column flex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.ts`:
- Around line 706-713: The current global centering CSS targeting
[class*="bg-bg-app"] and its children forces vertical flex layout and
auto-margins unconditionally; scope these rules so they only apply when not in
horizontal scroll/layout mode by wrapping them with a negative class condition
(e.g., :not(.horizontal-scroll) or the component's host selector) so horizontal
scroll set via EmbedPDF's setScrollMode/setLayoutMode/setScrollStrategy remains
unaffected; update the selectors that reference [class*="bg-bg-app"] and
[class*="bg-bg-app"] > * to the conditional variant and ensure the
horizontal-scroll class is added/removed in the same codepath that toggles
EmbedPDF's scroll/layout modes.
In `@frontend/src/assets/embedpdf-frame.html`:
- Around line 464-479: The iframe message handler duplicates
EmbedPdfBookService.applyScrollLayout() by forwarding the raw msg.layout string
to the plugin setters; map/convert msg.layout into the ScrollStrategy enum the
service expects (or otherwise normalize to the same shape) before calling
setScrollStrategy/setScrollMode/setLayoutMode so both viewers stay in sync, and
only call scrollPlugin.scrollToPage(...) if one of those setter methods actually
matched (short-circuit/return when no setter was invoked) to avoid wasted work;
reference the handler's use of msg.layout, setScrollStrategy, setScrollMode,
setLayoutMode, and scrollPlugin.scrollToPage to locate the code to change.
---
Outside diff comments:
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html`:
- Around line 276-304: The mobile overflow buttons for search and annotation
tools lack the active state bindings; update each overflow button that calls
toggleSearch() and toggleAnnotationTool('highlight'|'freeText'|'ink') (and the
scroll-layout button) to mirror the desktop toolbar by adding [class.active] and
[attr.aria-pressed] bindings using the same predicates used on desktop (e.g. use
isSearchOpen() for the search button, activeAnnotationTool('highlight') /
activeAnnotationTool('freeText') / activeAnnotationTool('ink') for the
annotation buttons, and the equivalent predicate for scroll-layout), so the UI
shows the current active state and the aria-pressed attribute reflects it.
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 1024-1033: The handler for the 'documentOpened' case uses the
stale this.initialPage and a guard that skips scrolling for page 1; update it to
use the live page getter this.page() instead of this.initialPage, remove the "if
(… > 1)" check so scrolling to page 1 is allowed, and post the message with
pageNumber: this.page(); keep the existing suppressProgressSave setTimeout
unchanged.
---
Nitpick comments:
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 398-405: Narrow the incoming scroll layout to a valid
PdfScrollLayout before assigning: validate/coerce the values coming from
pdfPrefs.pdfSettings?.scrollLayout and
myself.userSettings.pdfReaderSetting.scrollLayout (which are PdfScrollLayout |
undefined) into an allowed PdfScrollLayout enum/string set (similar to the
spreadVal/zoomVal pattern used nearby) and then assign the sanitized value to
scrollLayoutVal and call this.scrollLayout.set(sanitizedValue); also apply the
same coercion for the earlier occurrence (the analogous assignment mentioned
around the other block) so that downstream callers like
EmbedPdfBookService.setScrollStrategy only receive validated PdfScrollLayout
values.
- Around line 1413-1451: performCloseReader currently awaits persistAnnotations
but persistAnnotations only starts a fetch and returns before the HTTP save
completes; update persistAnnotations so it returns a Promise that awaits the
fetch/response (and throws or resolves based on response.ok) so
performCloseReader truly waits for the save when required. Also avoid writing to
component signals after navigation: in performCloseReader, detect when you call
this.location.back() or this.router.navigate(['/']) (i.e., navigation kicked
off) and skip calling resetCloseReaderState() afterward (or only call
resetCloseReaderState() when navigation was not initiated) to prevent dead-store
writes to isClosingReader/closeReaderPromise on a destroyed instance; reference
functions: persistAnnotations, performCloseReader, resetCloseReaderState,
closeReaderPromise, isClosingReader, location.back, router.navigate.
In
`@frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.ts`:
- Around line 229-235: The setTimeout in setScrollLayout sets a delayed call to
scrollToPage but its timer ID isn't stored or cleared; add a class property
(e.g. layoutTimeout or reuse initTimeout pattern) to capture the return value of
setTimeout inside setScrollLayout, and then clearTimeout(this.layoutTimeout) and
null it inside destroy() so the pending callback cannot run after teardown;
update references to setScrollLayout, scrollToPage and destroy to use the stored
timer handle.
- Around line 557-562: EmbedPdfBookService.getTilingConfig currently returns
{tileSize: 640, overlapPx: 2, extraRings: 1} for small viewports while
embedpdf-frame.html uses {tileSize: 512, overlapPx: 2, extraRings: 0}, causing
inconsistent tile area under DPR=2; either align
EmbedPdfBookService.getTilingConfig to match the iframe values (tileSize: 512,
overlapPx: 2, extraRings: 0) to avoid extra memory/tiles on mobile, or if the
larger tile and extra ring are intentional for annotation richness, add a
concise code comment in EmbedPdfBookService.getTilingConfig referencing
embedpdf-frame.html and DPR-2 to document the deliberate asymmetry so future
maintainers don’t try to "align them".
In `@frontend/src/assets/embedpdf-frame.html`:
- Around line 271-278: The duplicated CSS block targeting [class*="bg-bg-app"]
(which forces display:flex; flex-direction:column; and margin:auto on children)
is already injected by EmbedPdfBookService.injectBookModeStyles() and can
conflict with horizontal-scroll layouts; remove this duplicate from
embedpdf-frame.html, or restrict it so it only applies in true book-mode (e.g.,
under a .book-mode or similar explicit class) instead of a substring class
selector, and centralize the rule into a single shared stylesheet or exported
inline-style constant used by both EmbedPdfBookService.injectBookModeStyles()
and the frame so there is one source of truth and horizontal row layouts aren’t
broken by an unguarded column flex.
🪄 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: CHILL
Plan: Pro
Run ID: b78d7e9f-a0b1-4af3-882b-3a4eb52df6eb
📒 Files selected for processing (4)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.htmlfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/assets/embedpdf-frame.html
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (4)
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS in frontend code
Files:
frontend/src/assets/embedpdf-frame.htmlfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.html
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/assets/embedpdf-frame.htmlfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.html
frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer inject() over constructor injection in frontend code
Files:
frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
frontend/src/**/*.component.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep Angular code on standalone components. Do not add NgModules in frontend code
Files:
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
🧠 Learnings (6)
📚 Learning: 2026-04-04T14:03:28.295Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 372
File: frontend/src/app/features/book/service/app-books-api.service.ts:150-178
Timestamp: 2026-04-04T14:03:28.295Z
Learning: In `frontend/src/app/features/book/service/app-books-api.service.ts`, the `summaryToBook` function intentionally uses `as unknown as Book` to cast a partial object to the `Book` type. The returned object omits some required `Book` fields (e.g. `metadata.bookId`, `pdfProgress.page`) and includes an extra `bookFiles: []` property not in the `Book` interface. This is by design — consumers of `AppBooksApiService.books()` (e.g. `BookCardComponent`) only access the subset of properties that are populated from `AppBookSummary`, so no runtime issues occur. Do not flag this as a type safety error.
Applied to files:
frontend/src/assets/embedpdf-frame.htmlfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.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/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
📚 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/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.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/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
📚 Learning: 2026-04-22T01:56:39.495Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-22T01:56:39.495Z
Learning: Applies to frontend/src/**/*.component.ts : Keep Angular code on standalone components. Do not add NgModules in frontend code
Applied to files:
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
📚 Learning: 2026-04-24T11:20:15.820Z
Learnt from: alexhb1
Repo: grimmory-tools/grimmory PR: 831
File: frontend/src/app/features/book/service/shelf.service.ts:15-16
Timestamp: 2026-04-24T11:20:15.820Z
Learning: Repo grimmory-tools/grimmory — In frontend/src/app/features/book/service/shelf.service.ts, detecting the Kobo system shelf via exact match on name 'Kobo' and icon 'pi pi-tablet' is an intentional exception approved by maintainers; do not flag this as fragile or request a backend systemKey for this case.
Applied to files:
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html
🔀 Multi-repo context grimmory-tools/grimmory-docs
Linked repositories findings
grimmory-tools/grimmory-docs
-
docs/readers/pdf-reader.md — contains a "Scroll Modes" section listing "Horizontal Scrolling" and other modes; the new UI/setting ("Scroll layout" with 'vertical'|'horizontal') should be documented here, including behavior differences and toolbar control availability. [::grimmory-tools/grimmory-docs::docs/readers/pdf-reader.md:55-101]
-
docs/reader-preferences.md — has a "PDF Reader: Default Settings" section and "Streaming PDF Reader Settings" references; add the new persisted preference (pdfReaderSetting.scrollLayout: 'vertical' | 'horizontal') to the PDF Reader settings table and any explanations of Default Override vs Book-Specific scope. [::grimmory-tools/grimmory-docs::docs/reader-preferences.md:1-240]
Conclusion: Documentation repository contains the PDF reader and reader-preferences pages that need updates to describe the new "Scroll layout" option and any UX/behavior notes introduced by the PR (files above are the exact locations).
🔇 Additional comments (3)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html (2)
164-181: Scroll-layout toggle: icons are now distinct from spread — thanks for the earlier fix.The horizontal variant (arrow + narrow rect) and vertical variant (two stacked rects) are clearly distinguishable from the spread icon's side-by-side rects, so the previous icon-collision concern is resolved.
aria-pressedandaria-labelare correctly tied to the signal.One nit: the state is binary, so
aria-pressed="<bool>"fully expresses the toggle — the separate dynamicaria-label/titleswap between "horizontal" / "vertical" strings is somewhat redundant for screen-reader users (they'll hear "Horizontal, pressed" or similar). Not wrong, just worth knowing.
258-261: Pointerdown + click wiring is correct; double-invocation is prevented in the component.Returning early on
pointerType === 'mouse'insideonCloseReaderPointerDownavoids the double-fire on mouse (where bothpointerdownandclickwould otherwise callrequestCloseReader), and touch/pen benefit from firing on pointerdown instead of waiting for the synthesized click. Combined withrequestCloseReader'sisClosingReader()guard and thecloseReaderPromisesingleton in the .ts file, the close-reader path is safely idempotent.frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.ts (1)
435-476: UseScrollStrategyenum from@embedpdf/snippetinstead of raw strings.The code currently passes string literals
'vertical'and'horizontal'tosetScrollStrategy(), but@embedpdf/snippetexports aScrollStrategyenum. Import and useScrollStrategy.Vertical/ScrollStrategy.Horizontalto match the API contract, then remove the fallback checks forsetScrollMode()andgetScrollMode()(these are not part of the current API surface).
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)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (3)
564-603:⚠️ Potential issue | 🟠 MajorPage restoration uses stale
initialPageinstead of the current page on mode switches.
initialPageis captured once inngOnInit(Line 408) frompdfMeta.pdfProgress?.pageand never updated. It is now used in two re-entrant init paths:
- Line 568 (
layoutReady$for book viewer)- Line 1047 / 1049 (
documentOpenedhandler for the doc-viewer iframe)
initBookViewer()andinitDocViewerIframe()both run again whenever the user togglessetViewerMode(...). After a user navigates from page 1 → page 50 in book mode and switches to document mode,documentOpenedwill scroll the iframe back to the originalinitialPage(e.g., 1) instead of page 50. Switching back to book mode then re-runslayoutReady$and snaps the book viewer back toinitialPageagain. Both directions silently lose the user's reading position on every mode toggle.Use the live
page()signal for restoration after the first init (it remains in sync becausesetViewerModedoes not reset it).🐛 Proposed fix
- this.embedPdfBook.layoutReady$.pipe( - takeUntilDestroyed(this.destroyRef), - take(1) - ).subscribe(() => { - const currentPage = this.initialPage; + this.embedPdfBook.layoutReady$.pipe( + takeUntilDestroyed(this.destroyRef), + take(1) + ).subscribe(() => { + const currentPage = this.page();case 'documentOpened': - // Scroll to the page the user was on in book mode and re-enable progress saving - if (this.embedPdfIframe?.contentWindow && this.initialPage > 1) { + // Scroll to the page the user was on in book mode and re-enable progress saving + const restorePage = this.page(); + if (this.embedPdfIframe?.contentWindow && restorePage > 1) { this.embedPdfIframe.contentWindow.postMessage( - { type: 'scrollToPage', pageNumber: this.initialPage }, + { type: 'scrollToPage', pageNumber: restorePage }, location.origin ); }(Alternatively, update
initialPagefromsetViewerMode()before destroying the active viewer.)Also applies to: 1045-1054
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts` around lines 564 - 603, The restore logic is using the stale initialPage captured at ngOnInit; update init paths to read the live page() signal when restoring after the first init: in initBookViewer() (inside the layoutReady$ subscription) and in the documentOpened handler used by initDocViewerIframe(), replace references to the captured initialPage with a currentPage = this.page() (or fallback to initialPage only if page() is undefined) so mode switches preserve the current position; ensure existing isInitialScrollDone/suppressProgressSave logic remains the same and unsubscribe the pageChange$ fallback as before.
1014-1054:⚠️ Potential issue | 🟠 MajorDoc-viewer iframe is not synced with the user's
scrollLayout(or pan) on initial load.The
initpostMessage at Lines 1014–1019 only forwardsthemeandlocale.documentOpened(Lines 1045–1054) does not pushsetScrollLayoutorsetPanModeeither.cycleScrollLayout(Line 860) andapplyPanMode(Line 754) only push to the iframe when the user explicitly toggles.Effect: a user whose persisted preference is
scrollLayout: 'horizontal'opens the document viewer and sees the iframe's built-in vertical default until they manually re-toggle the button — directly defeating the persisted preference this PR introduces. The same gap exists forisPanActive()(notably on phones, where Line 209 auto-enables pan).🐛 Proposed fix — sync state once the document is open
case 'documentOpened': // Scroll to the page the user was on in book mode and re-enable progress saving if (this.embedPdfIframe?.contentWindow && this.page() > 1) { this.embedPdfIframe.contentWindow.postMessage( { type: 'scrollToPage', pageNumber: this.page() }, location.origin ); } + if (this.embedPdfIframe?.contentWindow) { + this.embedPdfIframe.contentWindow.postMessage( + { type: 'setScrollLayout', layout: this.scrollLayout() }, + location.origin + ); + if (this.isPanActive()) { + this.embedPdfIframe.contentWindow.postMessage( + { type: 'setPanMode', enable: true }, + location.origin + ); + } + } setTimeout(() => { this.suppressProgressSave = false; }, 500); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts` around lines 1014 - 1054, The iframe isn't receiving persisted scroll/pan settings on load; update the init/documentOpened messaging in handleEmbedPdfMessage (and the init postMessage block) so after the viewer is ready/documentOpened you send { type: 'setScrollLayout', layout: this.scrollLayout } and { type: 'setPanMode', enabled: this.isPanActive() } to embedPdfIframe.contentWindow (use the same location.origin and transferable buffer pattern where applicable); reference cycleScrollLayout and applyPanMode as the source of truth for values and ensure this happens when docViewerReady is set or inside the 'documentOpened' case right after scrolling and before re-enabling suppressProgressSave.
1263-1316:⚠️ Potential issue | 🟡 MinorHorizontal swipe page-turns can fight horizontal scroll mode on tablets.
handleTouchEndtriggersgoToNextPage()/goToPreviousPage()on horizontal swipes whenever!isPhone() && !isPanActive(). WithscrollLayout() === 'horizontal', the natural gesture for moving across pages is a horizontal pan/swipe, so on a touch-enabled non-phone device the swipe will both scroll the document and trigger a programmatic page jump — racing each other.Consider also disabling swipe page navigation when
scrollLayout() === 'horizontal':♻️ Proposed adjustment
- const skipSwipe = this.isPhone() || this.isPanActive(); + const skipSwipe = this.isPhone() || this.isPanActive() || this.scrollLayout() === 'horizontal';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts` around lines 1263 - 1316, handleTouchEnd triggers page-turns on horizontal swipes even when the reader is in horizontal scroll layout, causing conflict with natural horizontal scrolling; update the swipe-skip logic to also skip swipe-based page navigation when the document is in horizontal scroll layout. In practice, modify the existing skip condition in handleTouchEnd (where it currently sets skipSwipe = this.isPhone() || this.isPanActive()) to include a check for horizontal scroll (e.g., this.scrollLayout() === 'horizontal' or similar property/method used in this component), so that when scrollLayout is 'horizontal' the method will not call goToNextPage() or goToPreviousPage() on swipe. Ensure you reference the same symbols: handleTouchEnd, isPhone(), isPanActive(), scrollLayout(), goToNextPage(), goToPreviousPage().
🧹 Nitpick comments (1)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html (1)
237-408: LGTM — clean consolidation viaactionBtn/btnBase.Routing every toolbar/overflow control through a single dispatcher (
performAction(id)) plus sharedbtnBasetemplate removes a lot of duplication and makes the new aria coverage (aria-label,aria-pressed, conditionaltitle) uniform. The two-handler close flow (pointerdown for touch/pen, click for mouse, both delegating to the idempotentrequestCloseReader) plus[disabled]="isClosingReader()"correctly serializes close attempts.Minor nit: in
btnBase, the overflow branch hard-codes[size]="18"(Line 398) while the icon branch honorssize(Line 405) — thesize/iconSizecontext variable is therefore ignored for overflow items. This is intentional for the menu layout, but consider dropping the unusediconSizefrom overflow call sites or threadingsizethrough both branches for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html` around lines 237 - 408, The overflow branch in the btnBase template ignores the passed icon size (iconSize/size) by hardcoding [size]="18", so either propagate the size into overflow icons or remove iconSize at the actionBtn call sites; update btnBase (template symbol btnBase) so the overflow button uses <app-reader-icon [name]="icon" [size]="size" /> (respecting the let-size passed from actionBtn), or alternatively remove the iconSize context and references in actionBtn so isOverflow items don't pass a redundant iconSize; ensure changes reference actionBtn, btnBase, iconSize, isOverflow and app-reader-icon.
🤖 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 `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 564-603: The restore logic is using the stale initialPage captured
at ngOnInit; update init paths to read the live page() signal when restoring
after the first init: in initBookViewer() (inside the layoutReady$ subscription)
and in the documentOpened handler used by initDocViewerIframe(), replace
references to the captured initialPage with a currentPage = this.page() (or
fallback to initialPage only if page() is undefined) so mode switches preserve
the current position; ensure existing isInitialScrollDone/suppressProgressSave
logic remains the same and unsubscribe the pageChange$ fallback as before.
- Around line 1014-1054: The iframe isn't receiving persisted scroll/pan
settings on load; update the init/documentOpened messaging in
handleEmbedPdfMessage (and the init postMessage block) so after the viewer is
ready/documentOpened you send { type: 'setScrollLayout', layout:
this.scrollLayout } and { type: 'setPanMode', enabled: this.isPanActive() } to
embedPdfIframe.contentWindow (use the same location.origin and transferable
buffer pattern where applicable); reference cycleScrollLayout and applyPanMode
as the source of truth for values and ensure this happens when docViewerReady is
set or inside the 'documentOpened' case right after scrolling and before
re-enabling suppressProgressSave.
- Around line 1263-1316: handleTouchEnd triggers page-turns on horizontal swipes
even when the reader is in horizontal scroll layout, causing conflict with
natural horizontal scrolling; update the swipe-skip logic to also skip
swipe-based page navigation when the document is in horizontal scroll layout. In
practice, modify the existing skip condition in handleTouchEnd (where it
currently sets skipSwipe = this.isPhone() || this.isPanActive()) to include a
check for horizontal scroll (e.g., this.scrollLayout() === 'horizontal' or
similar property/method used in this component), so that when scrollLayout is
'horizontal' the method will not call goToNextPage() or goToPreviousPage() on
swipe. Ensure you reference the same symbols: handleTouchEnd, isPhone(),
isPanActive(), scrollLayout(), goToNextPage(), goToPreviousPage().
---
Nitpick comments:
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html`:
- Around line 237-408: The overflow branch in the btnBase template ignores the
passed icon size (iconSize/size) by hardcoding [size]="18", so either propagate
the size into overflow icons or remove iconSize at the actionBtn call sites;
update btnBase (template symbol btnBase) so the overflow button uses
<app-reader-icon [name]="icon" [size]="size" /> (respecting the let-size passed
from actionBtn), or alternatively remove the iconSize context and references in
actionBtn so isOverflow items don't pass a redundant iconSize; ensure changes
reference actionBtn, btnBase, iconSize, isOverflow and app-reader-icon.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 01ad2132-cba8-4853-9536-020b97abd41b
📒 Files selected for processing (3)
frontend/src/app/features/readers/ebook-reader/shared/icon.component.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.htmlfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (4)
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS in frontend code
Files:
frontend/src/app/features/readers/ebook-reader/shared/icon.component.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.htmlfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
frontend/src/**/*.component.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep Angular code on standalone components. Do not add NgModules in frontend code
Files:
frontend/src/app/features/readers/ebook-reader/shared/icon.component.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer inject() over constructor injection in frontend code
Files:
frontend/src/app/features/readers/ebook-reader/shared/icon.component.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.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/readers/ebook-reader/shared/icon.component.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.htmlfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
🧠 Learnings (5)
📚 Learning: 2026-04-24T11:20:15.820Z
Learnt from: alexhb1
Repo: grimmory-tools/grimmory PR: 831
File: frontend/src/app/features/book/service/shelf.service.ts:15-16
Timestamp: 2026-04-24T11:20:15.820Z
Learning: Repo grimmory-tools/grimmory — In frontend/src/app/features/book/service/shelf.service.ts, detecting the Kobo system shelf via exact match on name 'Kobo' and icon 'pi pi-tablet' is an intentional exception approved by maintainers; do not flag this as fragile or request a backend systemKey for this case.
Applied to files:
frontend/src/app/features/readers/ebook-reader/shared/icon.component.ts
📚 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/readers/ebook-reader/shared/icon.component.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.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/readers/ebook-reader/shared/icon.component.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.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/readers/ebook-reader/shared/icon.component.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
📚 Learning: 2026-04-22T01:56:39.495Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-22T01:56:39.495Z
Learning: Applies to frontend/src/**/*.component.ts : Keep Angular code on standalone components. Do not add NgModules in frontend code
Applied to files:
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
🔀 Multi-repo context grimmory-tools/grimmory-docs
[::grimmory-tools/grimmory-docs::docs/readers/pdf-reader.md:1-240]
- The PDF reader doc includes a "Scroll Modes" table listing "Vertical Scrolling" and "Horizontal Scrolling" and several scroll mode variants (Page Scrolling, Vertical, Horizontal, Wrapped, Infinite, Book Mode). This is the canonical reader documentation that should reflect the new "scrollLayout" toggle and any behavior differences between viewer/embed modes (e.g., how horizontal layout behaves in book vs document mode).
[::grimmory-tools/grimmory-docs::docs/reader-preferences.md:1-240]
- The Reader Preferences doc (Streaming PDF Reader Settings / PDF Reader: Default Settings) documents persisted PDF reader defaults and shows the PDF Reader Settings table and guidance about scope (global vs per-book). It does not currently reference a persisted pdfReaderSetting.scrollLayout property; it should be updated to document the new persisted preference (pdfReaderSetting.scrollLayout: 'vertical' | 'horizontal'), default value, and interaction with per-book vs global preference scope.
Conclusion:
- grimmory-docs contains the two canonical places (docs/readers/pdf-reader.md and docs/reader-preferences.md) that must be updated to describe the new scrollLayout UI, behavior, and persisted setting introduced by the PR.
🔇 Additional comments (1)
frontend/src/app/features/readers/ebook-reader/shared/icon.component.ts (1)
38-292: LGTM — icon set expansion is consistent and type-safe.The expanded
ReaderIconNameunion and matchingICONSrecord cover all glyphs required by the new toolbar (sidebar,highlight,free-text,draw,pan,spread-*,scroll-horizontal/vertical,rotate,sun/moon,dots-horizontal,zoom-in/out). Two things worth noting that are correctly handled:
- The new paths use lowercase relative commands (e.g.,
'm16 8 4 4-4 4'), but every new entry is the defaultpathtype — sogetLineCoords(which only strips uppercaseM/L) is not invoked for them.scroll-horizontal/scroll-verticalare now visually distinct fromspread-double, addressing the earlier collision.
i am not attached to either. I can change to whatever you'd like. |
…f-reader-improvements
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 862-869: The persisted scrollLayout and panMode are only sent on
user toggles (cycleScrollLayout / togglePanMode), so the iframe/book viewers
start with defaults; update the viewer initialization paths to push current
state: when the iframe document viewer mounts/initializes (the code that sets
embedPdfIframe and listens for load/ready) postMessage { type:
'setScrollLayout', layout: this.scrollLayout() } and { type: 'setPanMode', pan:
this.panMode() } to embedPdfIframe.contentWindow with location.origin, and for
the embedded book call embedPdfBook.setScrollLayout(this.scrollLayout()) and
embedPdfBook.setPanMode(this.panMode()) during its init; also apply the same fix
in the equivalent init block referenced around lines 1042-1066 so both viewers
receive persisted state on startup.
In `@frontend/src/app/features/readers/pdf-reader/utils/annotation-converter.ts`:
- Around line 51-53: Change the current all-or-nothing mapping over
parsed.annotations so a single bad item doesn't discard the whole set: iterate
over parsed.annotations (SerializableAnnotationTransferItem) and for each item
call restoreArrayBuffers inside a try/catch, pushing successfully restored items
to the result array and logging or noting failures (but not throwing), then
return the result array; this replaces storedItems.map(restoreArrayBuffers) with
per-item error handling around restoreArrayBuffers so malformed ctx.data only
drops that item instead of returning [] for the entire annotation set.
In `@frontend/src/assets/embedpdf-frame.html`:
- Around line 472-510: The handlers for msg.type 'setScrollLayout' and
'setPanMode' act on the viewer without validating the message source; add an
early guard in the message event handler that verifies event.origin matches the
expected origin (configurable or a known allowed origin) and that event.source
=== parent (or window.parent) before executing any
registry/provides/scrollPlugin or panPlugin logic; apply this check before the
blocks that reference registry/getPlugin('scroll') and registry/getPlugin('pan')
so that untrusted postMessage events are ignored.
- Around line 346-349: The small-viewport tiling settings in the embed PDF frame
are inconsistent with the book-viewer service; update the tiling object used in
the iframe render options (the ternary branch that currently sets tileSize: 512,
extraRings: 0) to match the book-viewer settings used in
embedpdf-book.service.ts (use tileSize: 640 and extraRings: 1 for the small
viewport breakpoint) so both components use the same tileSize and extraRings
values.
🪄 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: d50f15b5-2547-40ac-b0cb-0c3a71d7ad4b
📒 Files selected for processing (5)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/utils/annotation-converter.spec.tsfrontend/src/app/features/readers/pdf-reader/utils/annotation-converter.tsfrontend/src/assets/embedpdf-frame.html
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Frontend Lint Threshold Check
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (9)
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/readers/pdf-reader/utils/annotation-converter.spec.tsfrontend/src/app/features/readers/pdf-reader/utils/annotation-converter.tsfrontend/src/assets/embedpdf-frame.htmlfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer inject() over constructor injection in frontend code
Files:
frontend/src/app/features/readers/pdf-reader/utils/annotation-converter.spec.tsfrontend/src/app/features/readers/pdf-reader/utils/annotation-converter.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.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/readers/pdf-reader/utils/annotation-converter.spec.tsfrontend/src/app/features/readers/pdf-reader/utils/annotation-converter.tsfrontend/src/assets/embedpdf-frame.htmlfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.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/pdf-reader/utils/annotation-converter.spec.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/readers/pdf-reader/utils/annotation-converter.spec.tsfrontend/src/app/features/readers/pdf-reader/utils/annotation-converter.tsfrontend/src/assets/embedpdf-frame.htmlfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.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/pdf-reader/utils/annotation-converter.spec.ts
**/*.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/readers/pdf-reader/services/embedpdf-book.service.ts
frontend/src/**/*.component.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep Angular code on standalone components. Do not add NgModules in frontend code
Files:
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
**/*.component.ts
⚙️ CodeRabbit configuration file
**/*.component.ts: Angular 21 component review:
- Must be standalone (standalone: true); flag NgModule-based declarations.
- Prefer OnPush change detection strategy.
- Prefer inject() over constructor DI.
- Localization: use Transloco for all UI strings.
- Prefer Angular Signals (signal, computed, effect) for local/reactive state.
- Flag missing trackBy / track expression in list rendering.
- Prefer
@if/@for/@switchover *ngIf / *ngFor / *ngSwitch structural directives.
Files:
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:48.330Z
Learning: In grimmory-tools/grimmory, before commenting on any file processing code (epub or pdf), always verify the current state of the relevant upstream grimmory libraries: `grimmory-tools/epub4j` (for epub) and `grimmory-tools/PDFium4j` (for pdf). These custom libraries may have different APIs, capabilities, and limitations compared to the third-party libraries they replace. Issues about streaming, buffering, or API surface may need to be filed/addressed in those upstream repos rather than in grimmory itself.
📚 Learning: 2026-04-22T01:56:39.495Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-22T01:56:39.495Z
Learning: Applies to frontend/src/**/*.{test,spec}.{ts,tsx} : Use Vitest for tests in frontend code
Applied to files:
frontend/src/app/features/readers/pdf-reader/utils/annotation-converter.spec.ts
📚 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/readers/pdf-reader/utils/annotation-converter.spec.tsfrontend/src/app/features/readers/pdf-reader/utils/annotation-converter.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.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/readers/pdf-reader/utils/annotation-converter.spec.tsfrontend/src/app/features/readers/pdf-reader/utils/annotation-converter.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.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/readers/pdf-reader/utils/annotation-converter.spec.tsfrontend/src/app/features/readers/pdf-reader/utils/annotation-converter.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
📚 Learning: 2026-04-04T14:03:28.295Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 372
File: frontend/src/app/features/book/service/app-books-api.service.ts:150-178
Timestamp: 2026-04-04T14:03:28.295Z
Learning: In `frontend/src/app/features/book/service/app-books-api.service.ts`, the `summaryToBook` function intentionally uses `as unknown as Book` to cast a partial object to the `Book` type. The returned object omits some required `Book` fields (e.g. `metadata.bookId`, `pdfProgress.page`) and includes an extra `bookFiles: []` property not in the `Book` interface. This is by design — consumers of `AppBooksApiService.books()` (e.g. `BookCardComponent`) only access the subset of properties that are populated from `AppBookSummary`, so no runtime issues occur. Do not flag this as a type safety error.
Applied to files:
frontend/src/app/features/readers/pdf-reader/utils/annotation-converter.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.ts
📚 Learning: 2026-04-25T02:36:28.491Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 848
File: frontend/src/index.html:9-9
Timestamp: 2026-04-25T02:36:28.491Z
Learning: In the grimmory project (`frontend/src/index.html`), adding broad CSP directives like `default-src 'self'` with targeted overrides breaks the application. The CSP must be kept minimal and targeted to the actual resource needs of the app. Avoid recommending broad defense-in-depth CSP additions for this project.
Applied to files:
frontend/src/assets/embedpdf-frame.html
📚 Learning: 2026-05-01T07:14:45.234Z
Learnt from: zachyale
Repo: grimmory-tools/grimmory PR: 1030
File: frontend/src/app/features/readers/cbx-reader/cbx-reader.component.ts:764-797
Timestamp: 2026-05-01T07:14:45.234Z
Learning: In `frontend/src/app/features/readers/cbx-reader/cbx-reader.component.ts`, the backend page dimension list (`pageDimensions` from `CbxPageDimensionService.getPageDimensions()`) is always expected to be complete for all pages. The `pageDimensionsCache` exists only as a client-side cache for dimensions discovered after image load (e.g., for the magnifier), not as a fallback for missing backend data. Do NOT flag the absence of a `pageDimensionsCache` fallback in `computeCbxSpreads` calls as a bug.
Applied to files:
frontend/src/assets/embedpdf-frame.htmlfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
📚 Learning: 2026-05-01T07:16:38.760Z
Learnt from: zachyale
Repo: grimmory-tools/grimmory PR: 1030
File: frontend/src/app/features/readers/cbx-reader/cbx-reader.component.scss:403-417
Timestamp: 2026-05-01T07:16:38.760Z
Learning: In `frontend/src/app/features/readers/cbx-reader/cbx-reader.component.scss`, the `.fit-actual-size .two-page-view .previous-page-layer` and `.current-page-layer` intentionally use `justify-content: start` (left-aligned) instead of `margin-inline: auto` (centered). This is a deliberate UX decision: in `fit-actual-size` mode, spreads can be wider than the viewport, and left-alignment ensures the reader's view starts at the left page rather than the center fold, preventing spoilers from the spine/center showing before the left page is read.
Applied to files:
frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
📚 Learning: 2026-04-22T01:56:39.495Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-22T01:56:39.495Z
Learning: Applies to frontend/src/**/*.component.ts : Keep Angular code on standalone components. Do not add NgModules in frontend code
Applied to files:
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
🔀 Multi-repo context grimmory-tools/grimmory-docs
Linked repositories findings
grimmory-tools/grimmory-docs
-
docs/readers/pdf-reader.md — contains a "Scroll Modes" section documenting "Vertical Scrolling" and "Horizontal Scrolling" (see "Scroll Modes" table). [::grimmory-tools/grimmory-docs::docs/readers/pdf-reader.md:55-74]
-
docs/reader-preferences.md — documents "Streaming PDF Reader Settings" and the "PDF Reader: Default Settings" section (lists scroll/scroll mode options and general persistence behavior). [::grimmory-tools/grimmory-docs::docs/reader-preferences.md:24-28,78-120]
-
sidebars.ts — includes an entry exposing the PDF Reader doc in the site sidebar (docs entry present). [::grimmory-tools/grimmory-docs::sidebars.ts:81]
Findings summary:
- The docs repository already documents scroll modes and PDF reader default/settings screens. These are the canonical places to update to reflect the new persisted frontend/backend property (pdfReaderSetting.scrollLayout) and the new UI toggle/behavior introduced by the PR. [::grimmory-tools/grimmory-docs::docs/readers/pdf-reader.md][::grimmory-tools/grimmory-docs::docs/reader-preferences.md]
…g settings for small viewports
…ip malformed entries
we can omit probably I think one of them, but plugins are loaded at runtime with no gurantees. Hence why |
Description
Linked Issue: Fixes #838
Changes
This pull request introduces a new scroll layout (vertical/horizontal) option for the PDF reader, enhances accessibility by adding ARIA attributes to toolbar buttons, and expands the toolbar overflow menu with additional controls. There are also corresponding updates to the backend and frontend models to support the new scroll layout setting.
PDF Reader Functionality and UI:
Model Updates:
PdfViewerSettinginterface to include an optionalscrollLayoutproperty, supporting both 'vertical' and 'horizontal' values.EbookReaderSetting.PdfReaderSettingclass to include ascrollLayoutproperty, ensuring the user's preference is saved and loaded.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests