fix(pdf-viewer): convert component properties to signals for improved reactivity#475
Conversation
📝 WalkthroughWalkthroughThis PR converts the PDF reader component from plain properties to Angular Signals, updates all template bindings to function call syntax, reorganizes viewer initialization to run after template render using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.ts (1)
446-454:⚠️ Potential issue | 🟡 MinorSync the high-DPI docblock with the new threshold.
The comment says “at least 2”, but Line [454] now enforces
2.5. Please update the docstring to avoid confusion during future performance tuning.🤖 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 446 - 454, Update the docblock for ensureHighDpiRendering to reflect the actual minimum DPR now enforced (MIN_DPR = 2.5) — change the phrase "at least 2" to "at least 2.5" and adjust any surrounding wording that references DPI threshold so the comment accurately matches the enforced value in the ensureHighDpiRendering method.
🤖 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/services/embedpdf-book.service.ts`:
- Around line 446-454: Update the docblock for ensureHighDpiRendering to reflect
the actual minimum DPR now enforced (MIN_DPR = 2.5) — change the phrase "at
least 2" to "at least 2.5" and adjust any surrounding wording that references
DPI threshold so the comment accurately matches the enforced value in the
ensureHighDpiRendering method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8c8541b-6b54-4279-94f4-6f399e85aa4b
📒 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.scssfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.tsfrontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.ts
👮 Files not reviewed due to content moderation or server errors (3)
- frontend/src/app/features/readers/pdf-reader/pdf-reader.component.scss
- frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html
- frontend/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). (3)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (3)
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS files
Files:
frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.scssfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.htmlfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
frontend/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/src/app/**/*.{ts,tsx}: Preferinject()over constructor injection
Followfrontend/eslint.config.js: component selectors useapp-*, directive selectors useapp*, andanyis disallowed
Files:
frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
frontend/src/app/**/*.component.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep Angular code on standalone components. Do not add NgModules
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/app/features/readers/pdf-reader/services/embedpdf-book.service.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-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-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-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to frontend/src/app/**/*.component.ts : Keep Angular code on standalone components. Do not add NgModules
Applied to files:
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
📚 Learning: 2026-03-24T18:58:08.199Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:291-294
Timestamp: 2026-03-24T18:58:08.199Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a Hardcover edition has no page count, falling back to the book-level page count (book.pages) is the preferred behavior. Skipping the sync entirely in this case is considered worse than using a slightly approximate total, because the page count is only a proxy for converting progress % to page numbers for the API. The Hardcover API allows setting page-based progress even when the edition lacks a page count, and progress displays in at least some UI views (e.g., the book detail dropdown).
Applied to files:
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
🪛 HTMLHint (1.9.2)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (1)
frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.ts (1)
110-110: The configuration optionrender.defaultImageQuality: 4does not exist in the documented@embedpdf/snippetAPI (v2.14.0). The library's actualimageQualityparameter accepts a 0–1 range in per-page render options, not a global default. Additionally, the codebase already implements performance-conscious design:
- Tiling is enabled with
tileSize: 1024andextraRings: 1to manage memory on constrained devicesMIN_DPR = 2.5is intentionally set (documented in the code comment) to prevent pixelation on low-DPI viewports; reducing it to 2 contradicts this design intentThe suggestions to lower
defaultImageQualityandMIN_DPRtarget a non-existent configuration and would reverse intentional high-fidelity design decisions already reflected in the codebase.> Likely an incorrect or invalid review comment.
… reactivity (grimmory-tools#475) * fix(pdf-viewer): convert component properties to signals for improved reactivity * fix(pdf-viewer): improve image quality and improve rendering stability
… reactivity (grimmory-tools#475) * fix(pdf-viewer): convert component properties to signals for improved reactivity * fix(pdf-viewer): improve image quality and improve rendering stability
… reactivity (#475) * fix(pdf-viewer): convert component properties to signals for improved reactivity * fix(pdf-viewer): improve image quality and improve rendering stability
… reactivity (#475) * fix(pdf-viewer): convert component properties to signals for improved reactivity * fix(pdf-viewer): improve image quality and improve rendering stability
… reactivity (grimmory-tools#475) * fix(pdf-viewer): convert component properties to signals for improved reactivity * fix(pdf-viewer): improve image quality and improve rendering stability
Description
Linked Issue: Fixes #460
Changes
This pull request updates the
pdf-reader.component.htmlto use getter methods for all stateful properties instead of directly accessing component variables, improving consistency and preparing the codebase for more reactive state management. Additionally, a small style improvement is made to the header for better support of devices with safe areas.Key changes include:
Template Refactoring for State Management:
isLoading,isDarkTheme,viewerMode, etc.) with method calls (e.g.,isLoading(),isDarkTheme(),viewerMode(), etc.) throughout thepdf-reader.component.htmltemplate to ensure state is accessed reactively and consistently.Improved Data Binding and Input Handling:
searchQuery,goToPageInput, and slider values to use the new getter/setter patterns, making the component more maintainable and easier to test.Component Template Enhancements:
#bookViewerContainer) for easier access in the component class.Styling Improvements:
padding-top: env(safe-area-inset-top);to the PDF header toolbar for better layout on devices with notches or safe areas.Summary by CodeRabbit