fix(pdf): improve PDF blob handling and improve viewer destruction logic#506
Merged
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughPrefers cached PDF bytes/blob URLs for viewer init, gates UI until initial book-scroll completes, changes annotation persistence to fire-and-forget with retry-backed backend persistence, adjusts iframe-injected CSS to hide footers/snackbars, and tweaks safe-area/z-index rules and auth-interceptor handling. Changes
Sequence DiagramsequenceDiagram
participant Reader as PDF Reader
participant Cache as In-Memory Cache
participant Auth as Auth Service
participant Network as Remote Fetch
participant Doc as Doc Viewer (iframe)
participant Annot as Annotation Service
rect rgba(120,170,240,0.5)
Reader->>Cache: check `cachedPdfBuffer` / `pdfBlobUrl`
alt cached buffer or blob URL
Reader->>Doc: init with blob URL or bytes (no auth)
Doc->>Doc: load PDF from blob/bytes
else no cache
Reader->>Auth: request token/headers
Auth-->>Reader: return token
Reader->>Network: fetch PDF with auth headers
Network-->>Reader: return ArrayBuffer
Reader->>Cache: store ArrayBuffer / create blob URL
Reader->>Doc: init with fetched bytes/blob
end
end
rect rgba(200,140,120,0.5)
Reader->>Annot: persistAnnotations() (fire-and-forget subscribe)
Annot->>Annot: delegate to PdfAnnotationPersistenceHandler (REQUIRES_NEW)
Annot->>Annot: retry on optimistic lock / integrity errors
Note right of Reader: `suppressProgressSave` blocks progress updates during transitions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
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 (1)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (1)
410-418:⚠️ Potential issue | 🟠 MajorInvalidate or refresh
pdfBlobUrlwhen the underlying PDF changes.
initBookViewer()now prefers the cached blob URL, anddestroyBookViewer(false)keeps that cache alive across mode switches. After document mode saves a modified PDF, switching back to book mode will still reopen the pre-edit blob until the cache is cleared, so the user can immediately see stale content again.Possible fix
} else { // Switching from doc to book — save in background, don't block the switch if (this.embedPdfIframe) { - this.saveEmbedPdfDocument().finally(() => this.destroyDocViewerIframe()); + await this.saveEmbedPdfDocument(); + this.revokePdfBlobUrl(); + await this.destroyDocViewerIframe(); } this.viewerMode.set(mode); this.initTimeout = setTimeout(() => { this.initTimeout = undefined; this.initBookViewer();Also applies to: 549-554, 799-801
🤖 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 410 - 418, The cached pdfBlobUrl can become stale when the underlying PDF changes; update initBookViewer(), places using pdfBlobUrl (lines referencing pdfBlobUrl, fetchAsObjectUrl, revokePdfBlobUrl), and destroyBookViewer(false) to invalidate/refresh the cache whenever currentBookData differs from the blob’s source or after saving edits: detect a change in currentBookData (or a document-version/timestamp flag) and call revokePdfBlobUrl() and clear this.pdfBlobUrl before re-fetching via fetchAsObjectUrl so initBookViewer() never uses an out-of-date blob URL; ensure the same invalidation logic is applied to the other occurrences you noted (around the blocks at ~549-554 and ~799-801).
🤖 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.scss`:
- Around line 755-756: The header block that currently sets "height:
$header-height;" and "padding: 0 8px;" needs to include the top safe-area inset
so controls don't sit under the iPhone/PWA notch: update the rules that contain
those lines to add the safe area to the top (e.g. set height:
calc($header-height + env(safe-area-inset-top, 0px)); and add padding-top:
env(safe-area-inset-top, 0px) while preserving horizontal padding), and add the
legacy fallback constant() if desired; apply the same change to the other
similar blocks noted around the 800–828 region so the header, search bar, and
overflow menu all respect env(safe-area-inset-top).
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 1310-1312: The code currently fire-and-forgets
pdfAnnotationService.saveAnnotations(this.bookId, data).subscribe() but clears
annotationsDirty before the PUT completes; change this so annotationsDirty is
only cleared when the Observable succeeds: replace the blind subscribe with
explicit subscribe handlers on pdfAnnotationService.saveAnnotations(this.bookId,
data) where the next() callback sets this.annotationsDirty = false and logs
success (using this.bookId/items.length), and the error() callback leaves
annotationsDirty true and logs the failure; ensure you do not clear
annotationsDirty outside the success path.
---
Outside diff comments:
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 410-418: The cached pdfBlobUrl can become stale when the
underlying PDF changes; update initBookViewer(), places using pdfBlobUrl (lines
referencing pdfBlobUrl, fetchAsObjectUrl, revokePdfBlobUrl), and
destroyBookViewer(false) to invalidate/refresh the cache whenever
currentBookData differs from the blob’s source or after saving edits: detect a
change in currentBookData (or a document-version/timestamp flag) and call
revokePdfBlobUrl() and clear this.pdfBlobUrl before re-fetching via
fetchAsObjectUrl so initBookViewer() never uses an out-of-date blob URL; ensure
the same invalidation logic is applied to the other occurrences you noted
(around the blocks at ~549-554 and ~799-801).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 590f50d2-7dd6-4448-91db-c0cfff07c69a
📒 Files selected for processing (2)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.scssfrontend/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/pdf-reader.component.scssfrontend/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
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/pdf-reader.component.ts
🧠 Learnings (4)
📓 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-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/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/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/pdf-reader.component.ts
…ove performance and reduce network requests
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (1)
920-928: Consider documenting the 500ms settling delay.The scroll-to-page logic and delayed progress re-enabling are correct. The 500ms delay is a reasonable heuristic to let the viewer settle, but consider extracting it as a named constant with a comment explaining its purpose for future maintainers.
📝 Optional: Extract magic number to constant
// Near line 128 private readonly CHROME_HIDE_DELAY = 3000; + /** Time (ms) to let doc viewer settle before re-enabling progress saves */ + private readonly DOC_VIEWER_SETTLE_DELAY = 500; // At line 928 - setTimeout(() => { this.suppressProgressSave = false; }, 500); + setTimeout(() => { this.suppressProgressSave = false; }, this.DOC_VIEWER_SETTLE_DELAY);🤖 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 920 - 928, In the 'documentOpened' case, extract the magic 500ms timeout into a named constant (e.g., PDF_VIEWER_SETTLE_MS) and add a short comment explaining it is a heuristic delay to allow the embedded viewer to finish scrolling/settling before re-enabling progress saving; update the setTimeout call that toggles this.suppressProgressSave to use that constant and ensure the constant is declared near the top of the component (or with other related config) so maintainers can adjust the embedPdfIframe postMessage/page() scroll timing easily.frontend/src/app/core/security/auth-interceptor.service.ts (1)
8-9: Module-level state is intentional but could benefit from a brief comment.The module-level
isRefreshingandrefreshTokenSubjectcorrectly coordinate token refresh across concurrent requests from different interceptor invocations. Consider adding a short comment explaining this is intentional for cross-request coordination.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/core/security/auth-interceptor.service.ts` around lines 8 - 9, Add a short explanatory comment above the module-level variables isRefreshing and refreshTokenSubject stating that these are intentionally declared at module scope to coordinate token refresh across concurrent HTTP requests handled by multiple interceptor instances, describing their roles (isRefreshing guards a single refresh flow; refreshTokenSubject broadcasts the new token to pending requests) so future readers won't refactor them to instance scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/app/core/security/auth-interceptor.service.ts`:
- Around line 8-9: Add a short explanatory comment above the module-level
variables isRefreshing and refreshTokenSubject stating that these are
intentionally declared at module scope to coordinate token refresh across
concurrent HTTP requests handled by multiple interceptor instances, describing
their roles (isRefreshing guards a single refresh flow; refreshTokenSubject
broadcasts the new token to pending requests) so future readers won't refactor
them to instance scope.
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 920-928: In the 'documentOpened' case, extract the magic 500ms
timeout into a named constant (e.g., PDF_VIEWER_SETTLE_MS) and add a short
comment explaining it is a heuristic delay to allow the embedded viewer to
finish scrolling/settling before re-enabling progress saving; update the
setTimeout call that toggles this.suppressProgressSave to use that constant and
ensure the constant is declared near the top of the component (or with other
related config) so maintainers can adjust the embedPdfIframe postMessage/page()
scroll timing easily.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 396882b3-20f1-4264-822c-935cdae74fb1
📒 Files selected for processing (4)
frontend/src/app/core/security/auth-interceptor.service.tsfrontend/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
✅ Files skipped from review due to trivial changes (1)
- frontend/src/app/features/readers/pdf-reader/services/embedpdf-book.service.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). (1)
- GitHub Check: Packaging Smoke Test
🧰 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/assets/embedpdf-frame.htmlfrontend/src/app/core/security/auth-interceptor.service.tsfrontend/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/core/security/auth-interceptor.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 (5)
📓 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.
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, PDF processing uses `grimmory PDFium4j` from https://github.com/grimmory-tools/PDFium4j. This is a grimmory-maintained library and is fundamentally different from Apache PDFBox. Do NOT generalize API knowledge, behavior, or limitations of PDFBox to grimmory PDFium4j. Before raising review comments on PDF-related file processing code, verify the current API and capabilities in the grimmory-tools/PDFium4j repository.
📚 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/core/security/auth-interceptor.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/core/security/auth-interceptor.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/core/security/auth-interceptor.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/pdf-reader.component.ts
🔇 Additional comments (11)
frontend/src/assets/embedpdf-frame.html (1)
171-183: LGTM - CSS hiding rules for bottom UI elements.The broad selectors appropriately target notification/toast patterns within the shadow DOM scope. This aligns with the PR objective of adjusting UI for PDF presentation on mobile devices.
frontend/src/app/core/security/auth-interceptor.service.ts (2)
16-23: Auth endpoint exclusion logic is correct.The
isAuthRequestcheck correctly excludes all auth endpoints (/api/v1/auth/*) from automatic header injection and 401 refresh handling, preventing infinite refresh loops when the refresh endpoint itself fails.
32-66: Good use ofdefer()to fix race condition in refresh logic.Wrapping the refresh decision in
defer()ensures theisRefreshingcheck happens at subscription time rather than at function invocation time. This correctly handles the race where multiple concurrent 401 responses would otherwise all attempt to refresh before any could setisRefreshing = true.frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (8)
120-121: New state fields for PDF caching and progress suppression are appropriate.These fields correctly support the optimization goals:
cachedPdfBuffereliminates network requests when switching viewer modes, andsuppressProgressSaveprevents spurious progress updates during transitions.
412-443: PDF blob URL reuse and best-effort caching look correct.The logic correctly prioritizes existing resources: existing
pdfBlobUrl→ existing blobbookData→ fetch and create new blob URL. The async caching (lines 435-443) is appropriately fire-and-forget with silent error handling, and the doc viewer initialization (line 844) properly falls back when the cache isn't ready.
561-567: Good parameterization of blob URL revocation.The
revoke = truedefault maintains backward compatibility while allowing the mode switch (line 814) to preserve the blob URL for the doc viewer. Clean approach.
811-814: Viewer mode transition sequence is correct.The sequence properly: (1) suppresses progress saves to prevent spurious updates, (2) awaits annotation persistence before viewer destruction, and (3) preserves the blob URL for the incoming doc viewer. Well-coordinated transition logic.
842-862: PDF fetch logic with caching and auth handling is correct.The three-tier approach (cached buffer → blob URL fetch without auth → remote fetch with auth) is well-structured. Using
slice(0)on the cached buffer creates a defensive copy, and the auth header logic correctly distinguishes between local blob URLs (no auth needed) and remote URLs (auth required via manual header since nativefetchdoesn't go through the Angular interceptor).
1057-1058: Progress save suppression guard is appropriate.Simple early return that correctly prevents spurious progress updates during viewer mode transitions.
1098-1098: Memory cleanup for cached PDF buffer is correct.Setting
cachedPdfBuffer = nullallows the potentially large ArrayBuffer to be garbage collected when the component is destroyed.
1337-1353: Fire-and-forget annotation save with proper error handling addresses prior feedback.The implementation now correctly:
- Only clears
annotationsDirtyon successful save (line 1343)- Preserves the dirty flag on error (line 1347) enabling retry
- Logs both success and failure states for observability
The fire-and-forget pattern with explicit handlers is the right approach here to keep the viewer-mode switch non-blocking while maintaining data integrity.
zachyale
approved these changes
Apr 15, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.java`:
- Around line 57-58: In PdfAnnotationService where
ObjectOptimisticLockingFailureException is currently caught, don't
unconditionally ignore the conflict; instead on catching the exception reload
the current annotation row (e.g., via the repository method used elsewhere in
this class), compare the stored data field to the incoming payload, and if they
are identical treat it as a no-op success, otherwise attempt one retry of the
save with the latest entity (merge/update the entity's version or data as
appropriate) and if the retry still fails propagate the exception; ensure you
reference the same repository/save methods used in this class and update logs to
reflect reload/compare/retry actions.
- Around line 57-64: The current PdfAnnotationService catches
ObjectOptimisticLockingFailureException and DataIntegrityViolationException
after saveAndFlush and then retries or ignores within the same transaction, but
a failed flush makes the transaction rollback-only and the Hibernate Session
unusable; move the retry/recovery logic into a new `@Transactional` boundary in a
separate Service bean (e.g., PdfAnnotationRecoveryService) and have
PdfAnnotationService delegate to it for safe retries: for
DataIntegrityViolationException call a recovery method that does
pdfAnnotationRepository.findByBookIdAndUserId(...), update entity.setData(data)
and save in that new transactional method, and for
ObjectOptimisticLockingFailureException either delegate to a
retry-in-new-transaction that merges/updates the latest state or rethrow to the
caller (do not silently drop edits); ensure PdfAnnotationService no longer
performs repository retries after saveAndFlush and that the recovery bean is a
Spring-managed component with its own `@Transactional` methods to create a fresh
transaction/session for retries.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8c7e68f-635b-45a7-acfe-f43becb38d99
📒 Files selected for processing (4)
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.javafrontend/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.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- 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). (2)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (2)
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/pdf-reader.component.scssfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.html
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.java
🧠 Learnings (5)
📓 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-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:
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.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:
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.java
📚 Learning: 2026-04-13T05:02:01.463Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 486
File: booklore-api/src/main/java/org/booklore/service/oidc/OidcGroupMappingService.java:130-159
Timestamp: 2026-04-13T05:02:01.463Z
Learning: In grimmory-tools/grimmory, treat `permissionDemoUser`/`permission_demo_user` as a restricting “limited demo mode” flag: it must never be set to `true` as part of any “grant all permissions” or admin-grant flow. Specifically, in code paths that grant admin permissions (for example the OIDC `isAdmin` branch in `OidcGroupMappingService.applyPermissions`), ensure `permissionDemoUser` is intentionally excluded from the admin-permission block and cannot be turned on; enabling it for admin users is counterproductive.
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.java
📚 Learning: 2026-04-14T12:46:56.673Z
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:46:56.673Z
Learning: For this project (grimmory-tools/grimmory / booklore-api), prefer modern Java 25 idioms and language/library features when reviewing or suggesting changes. Favor current constructs such as unnamed variables (e.g., `_`) where applicable, records, pattern matching for `switch`/`instanceof`, sealed classes, sequenced collections, `Stream.toList()`, and text blocks. Avoid proposing regressions to older/legacy equivalents unless there’s a concrete compatibility constraint (e.g., a required older runtime/library behavior).
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.java
🪛 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 (3)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.html (1)
1-1: Good fix for the pre-scroll flash window.The combined loader condition and temporary
.v-hiddengating correctly prevent showing partially initialized book-view content beforeisInitialScrollDone()is true.Also applies to: 8-8
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.scss (2)
16-18: Visibility + loading overlay changes look solid.The
.v-hiddenutility and fixed full-screen loader layering are consistent with the new initialization flow and avoid transitional flicker during viewer setup/switching.Also applies to: 23-24, 30-30
818-818: Safe-area top offset update is a good mobile safeguard.Including
env(safe-area-inset-top)for.doc-viewer-infoin responsive mode helps prevent notch/status-bar overlap.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.java (1)
73-75:⚠️ Potential issue | 🟡 MinorPotential NPE if called outside authenticated context.
getAuthenticatedUser()can returnnullwhen there's no authentication (perAuthenticationService.java:84-86). While this service is likely only invoked from authenticated REST endpoints, consider adding a defensive null check or usingOptionalto provide a clearer failure mode.🛡️ Proposed defensive check
private Long getCurrentUserId() { - return authenticationService.getAuthenticatedUser().getId(); + var user = authenticationService.getAuthenticatedUser(); + if (user == null) { + throw new IllegalStateException("No authenticated user"); + } + return user.getId(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.java` around lines 73 - 75, getCurrentUserId currently calls authenticationService.getAuthenticatedUser().getId() which can NPE if getAuthenticatedUser() returns null; update getCurrentUserId to defensively handle a missing authenticated user by checking authenticationService.getAuthenticatedUser() for null (or using Optional) and either throw a clear runtime exception (e.g., IllegalStateException/UnauthorizedException with a descriptive message) or return an Optional<Long> so callers handle absence; reference getCurrentUserId and AuthenticationService.getAuthenticatedUser when making the change.frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (1)
517-531:⚠️ Potential issue | 🟡 MinorConsider adding a timeout to
layoutReady$subscription to prevent indefinite loading overlay.If
layoutReady$never emits (e.g., library error or stall),isInitialScrollDoneremainsfalseand the loading overlay blocks user interaction indefinitely. Adding a timeout fallback would improve resilience.⏱️ Proposed timeout fallback
+import { timeout } from 'rxjs/operators'; + // Use onLayoutReady for initial page scroll (fires when document layout is calculated) this.embedPdfBook.layoutReady$.pipe( takeUntilDestroyed(this.destroyRef), - take(1) + take(1), + timeout(10000) // 10s fallback ).subscribe({ - () => { + next: () => { const currentPage = this.page(); if (currentPage > 1) { this.embedPdfBook.scrollToPage(currentPage, 'instant'); } this.isInitialScrollDone.set(true); // Load outline, bookmarks, and annotations after layout is ready and settled this.loadOutline(); this.loadBookmarks(); this.loadAnnotations(); + }, + error: () => { + console.warn('[PDF Reader] layoutReady$ timed out, proceeding anyway'); + this.isInitialScrollDone.set(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` around lines 517 - 531, The layoutReady$ subscription can hang indefinitely; update the code around embedPdfBook.layoutReady$ (the subscription that currently calls page(), embedPdfBook.scrollToPage(...), sets isInitialScrollDone, and invokes loadOutline/loadBookmarks/loadAnnotations) to apply a timeout fallback (e.g., RxJS timeout or race with timer) so that if layoutReady$ does not emit within a reasonable duration it falls back to the same post-layout actions: set isInitialScrollDone to true, attempt initial scroll using page() and embedPdfBook.scrollToPage if applicable, call loadOutline/loadBookmarks/loadAnnotations, and log or handle the timeout error; ensure the subscription still uses takeUntilDestroyed(this.destroyRef) and cleans up properly.
🤖 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
`@booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.java`:
- Around line 73-75: getCurrentUserId currently calls
authenticationService.getAuthenticatedUser().getId() which can NPE if
getAuthenticatedUser() returns null; update getCurrentUserId to defensively
handle a missing authenticated user by checking
authenticationService.getAuthenticatedUser() for null (or using Optional) and
either throw a clear runtime exception (e.g.,
IllegalStateException/UnauthorizedException with a descriptive message) or
return an Optional<Long> so callers handle absence; reference getCurrentUserId
and AuthenticationService.getAuthenticatedUser when making the change.
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 517-531: The layoutReady$ subscription can hang indefinitely;
update the code around embedPdfBook.layoutReady$ (the subscription that
currently calls page(), embedPdfBook.scrollToPage(...), sets
isInitialScrollDone, and invokes loadOutline/loadBookmarks/loadAnnotations) to
apply a timeout fallback (e.g., RxJS timeout or race with timer) so that if
layoutReady$ does not emit within a reasonable duration it falls back to the
same post-layout actions: set isInitialScrollDone to true, attempt initial
scroll using page() and embedPdfBook.scrollToPage if applicable, call
loadOutline/loadBookmarks/loadAnnotations, and log or handle the timeout error;
ensure the subscription still uses takeUntilDestroyed(this.destroyRef) and
cleans up properly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 750851c9-8571-4a55-bf41-668733b2fafe
📒 Files selected for processing (3)
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationPersistenceHandler.javabooklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.javafrontend/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 (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (4)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.javabooklore-api/src/main/java/org/booklore/service/book/PdfAnnotationPersistenceHandler.java
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/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
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/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-14T22:10:58.270Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 513
File: booklore-api/src/main/java/org/booklore/service/reader/AudiobookReaderService.java:38-39
Timestamp: 2026-04-14T22:10:58.270Z
Learning: In `grimmory-tools/grimmory`, for the `AudiobookReaderService` pattern of wrapping a DB lookup + file I/O under a single `Transactional` boundary, the preferred refactoring approach is **Option B**: extract the `BookFileEntity` lookup into a dedicated `Service` bean (e.g., `AudiobookEntityLoader`) annotated with `Transactional(readOnly = true)`, so that the proxy always narrows the transaction to the DB access only and all filesystem I/O runs outside the persistence context. Do NOT use `TransactionTemplate` (Option A) for this pattern in this codebase.
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.javabooklore-api/src/main/java/org/booklore/service/book/PdfAnnotationPersistenceHandler.java
📚 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 booklore-api/src/**/*.java : Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce `Autowired` field injection
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.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:
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.javabooklore-api/src/main/java/org/booklore/service/book/PdfAnnotationPersistenceHandler.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:
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.javabooklore-api/src/main/java/org/booklore/service/book/PdfAnnotationPersistenceHandler.java
📚 Learning: 2026-04-13T05:02:01.463Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 486
File: booklore-api/src/main/java/org/booklore/service/oidc/OidcGroupMappingService.java:130-159
Timestamp: 2026-04-13T05:02:01.463Z
Learning: In grimmory-tools/grimmory, treat `permissionDemoUser`/`permission_demo_user` as a restricting “limited demo mode” flag: it must never be set to `true` as part of any “grant all permissions” or admin-grant flow. Specifically, in code paths that grant admin permissions (for example the OIDC `isAdmin` branch in `OidcGroupMappingService.applyPermissions`), ensure `permissionDemoUser` is intentionally excluded from the admin-permission block and cannot be turned on; enabling it for admin users is counterproductive.
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.javabooklore-api/src/main/java/org/booklore/service/book/PdfAnnotationPersistenceHandler.java
📚 Learning: 2026-04-14T12:46:56.673Z
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:46:56.673Z
Learning: For this project (grimmory-tools/grimmory / booklore-api), prefer modern Java 25 idioms and language/library features when reviewing or suggesting changes. Favor current constructs such as unnamed variables (e.g., `_`) where applicable, records, pattern matching for `switch`/`instanceof`, sealed classes, sequenced collections, `Stream.toList()`, and text blocks. Avoid proposing regressions to older/legacy equivalents unless there’s a concrete compatibility constraint (e.g., a required older runtime/library behavior).
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.javabooklore-api/src/main/java/org/booklore/service/book/PdfAnnotationPersistenceHandler.java
📚 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/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/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/pdf-reader.component.ts
🔇 Additional comments (8)
booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationPersistenceHandler.java (2)
32-52: LGTM! Well-designed isolated transaction handler.The
REQUIRES_NEWpropagation correctly ensures that flush failures (optimistic locking or constraint violations) don't poison the caller's transaction, allowing clean retries inPdfAnnotationService. The read-then-write pattern with version checking is appropriate here since the retry loop in the caller handles the expected race conditions.
54-62: Helper methods are correct.
EntityNotFoundExceptionappropriately propagates without retry since a missing book or user won't be resolved by retrying. This aligns with the retry loop inPdfAnnotationServicethat only catches concurrency-related exceptions.booklore-api/src/main/java/org/booklore/service/book/PdfAnnotationService.java (1)
37-64: Retry mechanism correctly addresses the previous transaction issues.The refactored design properly delegates to
PdfAnnotationPersistenceHandlerwithREQUIRES_NEW, ensuring each retry attempt runs in a fresh transaction. This correctly resolves the prior concern about retrying within a poisoned transaction.Minor observations:
- The 50ms backoff is reasonable for this use case
InterruptedExceptionhandling correctly re-interrupts the threadfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (5)
438-446: LGTM! Smart PDF buffer caching for network-free viewer switching.The best-effort caching approach is appropriate—failures are silently ignored since it's an optimization. Using
slice(0)correctly creates a copy of theArrayBufferto avoid issues with transferred/neutered buffers.Also applies to: 851-853
1337-1368: Past review concern addressed: dirty flag handling is now correct.The speculative clear of
annotationsDirtywith restoration on error in thesubscribecallback properly handles the fire-and-forget pattern. This addresses the previous review that flagged clearing the flag before confirming success.
817-817: Correct gating of progress saves during viewer transitions.The
suppressProgressSaveflag properly prevents spurious progress updates during viewer mode switches. The 500ms delay afterdocumentOpenedallows the document viewer to stabilize before re-enabling saves.Also applies to: 935-935, 1064-1066
565-571: LGTM! Parameterized blob URL revocation.The
revokeflag correctly allows the blob URL to be preserved when switching to document mode (avoiding a re-fetch) while still revoking on component destruction.
1433-1457: Minor cleanup looks good.The explicit
return falsefor unknown IDs and the namedfilteredvariable improve readability without changing behavior.
zachyale
pushed a commit
to zachyale/grimmory
that referenced
this pull request
Apr 17, 2026
…gic (grimmory-tools#506) * fix(pdf): improve PDF blob handling and improve viewer destruction logic * fix(pdf): PDF annotation saving logic and improve responsive design for mobile * fix(pdf): improve PDF fetching (once again) and caching logic to improve performance and reduce network requests * fix(pdf): hide footer and notification elements in PDF viewer * fix(pdf): improve loading behavior and improve annotation persistence logic * fix(pdf): improve PDF annotation persistence with retry mechanism and logging * fix(pdf): improve PDF annotation saving with raw fetch and improved error handling * fix(pdf): cap highlight opacity and adjust rendering for small viewports
zachyale
pushed a commit
to zachyale/grimmory
that referenced
this pull request
Apr 17, 2026
…gic (grimmory-tools#506) * fix(pdf): improve PDF blob handling and improve viewer destruction logic * fix(pdf): PDF annotation saving logic and improve responsive design for mobile * fix(pdf): improve PDF fetching (once again) and caching logic to improve performance and reduce network requests * fix(pdf): hide footer and notification elements in PDF viewer * fix(pdf): improve loading behavior and improve annotation persistence logic * fix(pdf): improve PDF annotation persistence with retry mechanism and logging * fix(pdf): improve PDF annotation saving with raw fetch and improved error handling * fix(pdf): cap highlight opacity and adjust rendering for small viewports
zachyale
pushed a commit
that referenced
this pull request
Apr 17, 2026
…gic (#506) * fix(pdf): improve PDF blob handling and improve viewer destruction logic * fix(pdf): PDF annotation saving logic and improve responsive design for mobile * fix(pdf): improve PDF fetching (once again) and caching logic to improve performance and reduce network requests * fix(pdf): hide footer and notification elements in PDF viewer * fix(pdf): improve loading behavior and improve annotation persistence logic * fix(pdf): improve PDF annotation persistence with retry mechanism and logging * fix(pdf): improve PDF annotation saving with raw fetch and improved error handling * fix(pdf): cap highlight opacity and adjust rendering for small viewports
zachyale
pushed a commit
that referenced
this pull request
Apr 22, 2026
…gic (#506) * fix(pdf): improve PDF blob handling and improve viewer destruction logic * fix(pdf): PDF annotation saving logic and improve responsive design for mobile * fix(pdf): improve PDF fetching (once again) and caching logic to improve performance and reduce network requests * fix(pdf): hide footer and notification elements in PDF viewer * fix(pdf): improve loading behavior and improve annotation persistence logic * fix(pdf): improve PDF annotation persistence with retry mechanism and logging * fix(pdf): improve PDF annotation saving with raw fetch and improved error handling * fix(pdf): cap highlight opacity and adjust rendering for small viewports
This was referenced Apr 24, 2026
dsmouse
pushed a commit
to dsmouse/grimmory
that referenced
this pull request
May 11, 2026
…gic (grimmory-tools#506) * fix(pdf): improve PDF blob handling and improve viewer destruction logic * fix(pdf): PDF annotation saving logic and improve responsive design for mobile * fix(pdf): improve PDF fetching (once again) and caching logic to improve performance and reduce network requests * fix(pdf): hide footer and notification elements in PDF viewer * fix(pdf): improve loading behavior and improve annotation persistence logic * fix(pdf): improve PDF annotation persistence with retry mechanism and logging * fix(pdf): improve PDF annotation saving with raw fetch and improved error handling * fix(pdf): cap highlight opacity and adjust rendering for small viewports
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Linked Issue: Fixes #
Changes
See: balazsszucs/grimmory:pdf-phone for testing purposes. (arm only)
Fixes problem when switching between the viewers essentially, as well removes "hard-coded" black headers, that were not very on phone. (or smaller devices in general.)
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Reliability
Security