Skip to content

fix(pdf-viewer): convert component properties to signals for improved reactivity#475

Merged
balazs-szucs merged 2 commits into
grimmory-tools:developfrom
balazs-szucs:pdf-viewer-fixes
Apr 11, 2026
Merged

fix(pdf-viewer): convert component properties to signals for improved reactivity#475
balazs-szucs merged 2 commits into
grimmory-tools:developfrom
balazs-szucs:pdf-viewer-fixes

Conversation

@balazs-szucs

@balazs-szucs balazs-szucs commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Description

Linked Issue: Fixes #460

Changes

This pull request updates the pdf-reader.component.html to 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:

  • Replaced all direct property bindings (e.g., isLoading, isDarkTheme, viewerMode, etc.) with method calls (e.g., isLoading(), isDarkTheme(), viewerMode(), etc.) throughout the pdf-reader.component.html template to ensure state is accessed reactively and consistently.

Improved Data Binding and Input Handling:

  • Updated two-way bindings and input handling for fields like searchQuery, goToPageInput, and slider values to use the new getter/setter patterns, making the component more maintainable and easier to test.

Component Template Enhancements:

  • Changed the book viewer container to use a template reference variable (#bookViewerContainer) for easier access in the component class.

Styling Improvements:

  • Added 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

  • New Features & Improvements
    • Enhanced iOS compatibility with proper safe-area inset handling for devices with notches
    • Improved responsive design with larger touch targets for mobile and tablet users
    • Increased PDF rendering image quality for clearer document display
    • Better support for high-DPI displays

@coderabbitai

coderabbitai Bot commented Apr 11, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This 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 afterNextRender, and applies responsive styling adjustments for mobile safe-area insets and touch targets.

Changes

Cohort / File(s) Summary
Template Signal Bindings
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html
Converted property bindings to signal function calls throughout template (isLoading(), isDarkTheme(), viewerMode(), etc.). Updated interpolations for dynamic text (bookTitle(), zoom(), page(), totalPages()). Reworked search input from two-way [(ngModel)] to one-way binding with explicit (ngModelChange) handler. Changed goToPageInput binding to use signal setter within event handler.
Mobile & Responsive Styling
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.scss
Added env(safe-area-inset-*) offsets to header/footer for iOS notch/home indicator support. Increased touch target sizes for slider thumbs and toolbar controls on mobile/tablet. Adjusted spacing, positioning, and control dimensions at small phone breakpoint. Applied flex: 1 to .page-controls wrapper for responsive layout.
Signals Refactor & Initialization
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
Migrated template-bound properties (isLoading, page, zoom, viewerMode, etc.) from class fields to signal() and computed() declarations. Replaced destroy$ + takeUntil with DestroyRef + takeUntilDestroyed(). Switched @ViewChild to viewChild() signals. Changed initialization sequencing to use afterNextRender() instead of retry-based DOM lookup, fixing timing issue where viewer element wasn't available during early init.
Rendering Configuration
frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.ts
Increased default image quality from 2 to 4 for better PDF tile rendering. Raised high-DPI enforcement threshold from 2 to 2.5 in ensureHighDpiRendering().

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

frontend, enhancement

Suggested reviewers

  • imajes
  • zachyale

Poem

🐰 Signals now flow through the viewer so bright,
No more property dances—just function calls right!
Safe areas and safe insets keep notches at bay,
The PDF loads swiftly—hooray, hooray, hooray! 📄✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title follows conventional commit format with 'fix' type prefix and clear descriptive message about converting properties to signals.
Description check ✅ Passed The PR description includes all required template sections with a linked issue (#460) and detailed explanation of changes made across template, bindings, and styling.
Linked Issues check ✅ Passed The PR addresses issue #460 by fixing PDF viewer initialization through improved state management, converting properties to signals, and rescheduling initialization via afterNextRender to ensure proper element availability.
Out of Scope Changes check ✅ Passed All changes are within scope: template refactoring for signals, styling for safe-area support, embedding service quality/DPI improvements, and component signal conversion directly address the initialization and reactivity issues.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

📋 Issue Planner

Built with CodeRabbit's Coding Plans for faster development and fewer bugs.

View plan used: #460

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟡 Minor

Sync 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

📥 Commits

Reviewing files that changed from the base of the PR and between 6e8f889 and 43e14aa.

📒 Files selected for processing (4)
  • frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html
  • frontend/src/app/features/readers/pdf-reader/pdf-reader.component.scss
  • frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
  • frontend/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.ts
  • 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
frontend/src/app/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

frontend/src/app/**/*.{ts,tsx}: Prefer inject() over constructor injection
Follow frontend/eslint.config.js: component selectors use app-*, directive selectors use app*, and any is disallowed

Files:

  • frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.ts
  • frontend/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.ts
  • frontend/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.ts
  • frontend/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.ts
  • frontend/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 option render.defaultImageQuality: 4 does not exist in the documented @embedpdf/snippet API (v2.14.0). The library's actual imageQuality parameter 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: 1024 and extraRings: 1 to manage memory on constrained devices
  • MIN_DPR = 2.5 is intentionally set (documented in the code comment) to prevent pixelation on low-DPI viewports; reducing it to 2 contradicts this design intent

The suggestions to lower defaultImageQuality and MIN_DPR target a non-existent configuration and would reverse intentional high-fidelity design decisions already reflected in the codebase.

			> Likely an incorrect or invalid review comment.

@balazs-szucs balazs-szucs merged commit f432df3 into grimmory-tools:develop Apr 11, 2026
14 checks passed
zachyale pushed a commit to zachyale/grimmory that referenced this pull request Apr 17, 2026
… 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
zachyale pushed a commit to zachyale/grimmory that referenced this pull request Apr 17, 2026
… 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
zachyale pushed a commit that referenced this pull request Apr 17, 2026
… reactivity (#475)

* fix(pdf-viewer): convert component properties to signals for improved reactivity

* fix(pdf-viewer): improve image quality and improve rendering stability
zachyale pushed a commit that referenced this pull request Apr 22, 2026
… reactivity (#475)

* fix(pdf-viewer): convert component properties to signals for improved reactivity

* fix(pdf-viewer): improve image quality and improve rendering stability
dsmouse pushed a commit to dsmouse/grimmory that referenced this pull request May 11, 2026
… 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Nightly] PDF viewer opens but fails to load PDF

2 participants