fix(phone/mobile): enable pan mode and disable swipe-to-navigate and tap-to-turn-page zones on mobile devices#742
Merged
Conversation
…tap-to-turn-page zones on mobile devices
Contributor
📝 WalkthroughWalkthroughAdded an Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User (touch/resize)"
participant Window as "Window (resize)"
participant PDFComp as "PDFReaderComponent"
participant Embed as "EmbedPDF Viewer"
User->>PDFComp: touch (tap/swipe)
alt isPhone() or isPanActive()
PDFComp-->>User: handle tap -> toggle chrome (no page turn)
PDFComp--x Embed: suppress swipe/page-change
else
PDFComp->>Embed: forward swipe -> page turn
end
Window->>PDFComp: resize event
PDFComp->>PDFComp: update isPhone signal
alt transitioned to phone
PDFComp->>PDFComp: applyPanMode(true)
PDFComp->>Embed: set pan mode = true (after init)
else transitioned to non-phone
PDFComp->>PDFComp: applyPanMode(userPreference)
PDFComp->>Embed: set pan mode accordingly
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
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)
1202-1217:⚠️ Potential issue | 🟡 MinorAdd a movement threshold before toggling chrome on phone taps.
The comment says “minimal movement,” but the code only checks duration and
touchMoveCount. A quick pan/selection drag with fewer than 3 move events can now toggle chrome anywhere on phones. Gate the tap path on actualdeltaX/deltaYdistance too.Suggested fix
// Tap detection: short duration, minimal movement - if (elapsed < this.TAP_MAX_DURATION && this.touchMoveCount < 3) { + const tapMaxMovement = 10; + const isTap = + elapsed < this.TAP_MAX_DURATION && + this.touchMoveCount < 3 && + Math.abs(deltaX) < tapMaxMovement && + Math.abs(deltaY) < tapMaxMovement; + + if (isTap) { const screenWidth = window.innerWidth; const tapX = this.touchStartX;🤖 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 1202 - 1217, The tap detection currently only checks elapsed time (TAP_MAX_DURATION) and touchMoveCount but not actual movement, so on phones a short pan with few move events can still trigger the chrome toggle; add a movement threshold constant (e.g., TAP_MOVE_THRESHOLD) and compute deltaX/deltaY from touchStartX/touchStartY and the current touch end position, then require Math.abs(deltaX) and Math.abs(deltaY) to be below that threshold before taking the tap path. Update the existing tap branch (the block using TAP_MAX_DURATION, touchMoveCount, touchStartX and the phone/zone checks) to include this movement check so that the center/phone toggle chrome action only runs when movement is within the threshold.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 193-195: The component computes isPhone once and sets pan state
via this.isPanActive.set, but viewport changes can desync it from later
touch-time checks (and this.isMobile); add a shared reactive flag (e.g., a
component property like isPhoneSignal or getIsPhone()) that is updated on window
'resize' and 'orientationchange', call setPanMode() whenever that computed
phone-state changes to keep EmbedPDF pan mode and touch-time logic in sync, and
register the listeners with DestroyRef.onDestroy(...) for cleanup instead of
manual teardown fields; update all references that read the one-time isPhone or
this.isMobile to use the shared signal/helper so both isPanActive.set and touch
handlers see the same current value.
---
Outside diff comments:
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 1202-1217: The tap detection currently only checks elapsed time
(TAP_MAX_DURATION) and touchMoveCount but not actual movement, so on phones a
short pan with few move events can still trigger the chrome toggle; add a
movement threshold constant (e.g., TAP_MOVE_THRESHOLD) and compute deltaX/deltaY
from touchStartX/touchStartY and the current touch end position, then require
Math.abs(deltaX) and Math.abs(deltaY) to be below that threshold before taking
the tap path. Update the existing tap branch (the block using TAP_MAX_DURATION,
touchMoveCount, touchStartX and the phone/zone checks) to include this movement
check so that the center/phone toggle chrome action only runs when movement is
within the threshold.
🪄 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: 1c946552-447e-4211-9701-8af8028d18ef
📒 Files selected for processing (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). (5)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Frontend Lint Threshold Check
🧰 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.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 (3)
📚 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
🔇 Additional comments (1)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (1)
440-442: Good placement for applying the pan-mode default.This runs after the embedded book viewer is initialized, so the pan plugin is available before
setPanMode(true)is called.
…one mode state and pan settings
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 195-205: syncPhoneMode currently overwrites user pan state on
every resize; change it to only act on breakpoint transitions and route all pan
changes through the existing togglePanMode/shared helper so manual tool state
isn't clobbered. Specifically: replace the direct writes to isPanActive inside
syncPhoneMode with logic that detects a change in isPhone (compare previous
value) and, on transitions, apply embedPdfBook.setPanMode only via the shared
togglePanMode (or a new helper) so active-tool clearing is preserved; introduce
a separate userPanActive vs phoneForcedPan flag so entering phone mode sets
phoneForcedPan=true and enables pan, and leaving phone mode clears
phoneForcedPan and restores userPanActive instead of overriding it; keep the
same event listener and destroyRef.onDestroy remove call.
🪄 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: a6fcfaed-4965-4ae6-b091-cb2acfcc218a
📒 Files selected for processing (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). (3)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend 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.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 (3)
📚 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 (4)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (4)
73-73: LGTM — phone mode is modelled reactively.This fits the component’s existing signal-based state and keeps template/touch logic using one source of truth.
452-454: LGTM — pan mode is applied after viewer initialization.This covers the case where phone mode set
isPanActivebefore EmbedPDF’s pan capability was ready.
1198-1202: LGTM — swipe navigation is suppressed in the right modes.Using the shared
isPhone()/isPanActive()state here keeps gesture behavior aligned with pan/mobile mode.
1219-1228: LGTM — phone taps no longer trigger page-zone navigation.The fallback to chrome toggling preserves the expected tap behavior without accidental page turns.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (2)
461-464: Consider routing the deferred pan throughapplyPanModefor consistency.Not a functional issue —
isPanActiveandactiveAnnotationToolwere already reconciled bysyncPhoneMode()duringngOnInit, so onlysetPanModeneeds to flush here. However, callingapplyPanMode(this.isPanActive())would keep all pan side-effect logic in one place and defensively clear any annotation tool that may have been selected betweenngOnInitandinitBookViewer(e.g. via a toolbar click before the viewer finished initializing).♻️ Suggested change
- if (this.isPanActive()) { - this.embedPdfBook.setPanMode(true); - } + if (this.isPanActive()) { + this.applyPanMode(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 461 - 464, Replace the direct call to embedPdfBook.setPanMode(true) in initBookViewer with a call to applyPanMode(this.isPanActive()) so all pan-related side effects (including clearing activeAnnotationTool) are centralized; locate the block using isPanActive(), embedPdfBook.setPanMode, and initBookViewer and change it to call applyPanMode instead, ensuring syncPhoneMode/ngOnInit reconciliation is preserved and any annotation tool selected after ngOnInit gets cleared by applyPanMode.
1238-1256: Optional: hoist the phone check above the tap-zone branches.Functionally correct, but
this.isPhone()is evaluated twice and the "center zone (or any zone on phone)" comment is doing heavy lifting. A short-circuit keeps intent obvious and avoids re-reading the signal.♻️ Suggested change
- this.ngZone.run(() => { - // On phones, disable tap-to-change-page zones to prevent accidental navigation. - // Taps will only toggle the chrome. - if (!this.isPhone() && tapX < screenWidth * this.TAP_ZONE_RATIO) { - // Left zone: previous page - this.goToPreviousPage(); - } else if (!this.isPhone() && tapX > screenWidth * (1 - this.TAP_ZONE_RATIO)) { - // Right zone: next page - this.goToNextPage(); - } else { - // Center zone (or any zone on phone): toggle chrome - if (this.headerVisible() || this.footerVisible()) { + this.ngZone.run(() => { + const toggleChrome = () => { + if (this.headerVisible() || this.footerVisible()) { this.hideChrome(); } else { this.showChrome(); this.startChromeAutoHide(); } + }; + + // On phones, tap zones are disabled — taps only toggle chrome. + if (this.isPhone()) { + toggleChrome(); + } else if (tapX < screenWidth * this.TAP_ZONE_RATIO) { + this.goToPreviousPage(); + } else if (tapX > screenWidth * (1 - this.TAP_ZONE_RATIO)) { + this.goToNextPage(); + } else { + toggleChrome(); } });🤖 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 1238 - 1256, The tap handler currently calls this.isPhone() twice and mixes phone logic into the tap-zone branches; refactor the ngZone.run block to check this.isPhone() once at the top and short-circuit to the "phone" behavior (always toggle chrome) if true, otherwise evaluate tapX against screenWidth * this.TAP_ZONE_RATIO to call goToPreviousPage() or goToNextPage() or toggle/show/hide chrome and call startChromeAutoHide() as appropriate; update the block containing ngZone.run, isPhone(), TAP_ZONE_RATIO, goToPreviousPage, goToNextPage, headerVisible, footerVisible, hideChrome, showChrome, and startChromeAutoHide so the intent is explicit and this.isPhone() is not re-evaluated.
🤖 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/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 461-464: Replace the direct call to embedPdfBook.setPanMode(true)
in initBookViewer with a call to applyPanMode(this.isPanActive()) so all
pan-related side effects (including clearing activeAnnotationTool) are
centralized; locate the block using isPanActive(), embedPdfBook.setPanMode, and
initBookViewer and change it to call applyPanMode instead, ensuring
syncPhoneMode/ngOnInit reconciliation is preserved and any annotation tool
selected after ngOnInit gets cleared by applyPanMode.
- Around line 1238-1256: The tap handler currently calls this.isPhone() twice
and mixes phone logic into the tap-zone branches; refactor the ngZone.run block
to check this.isPhone() once at the top and short-circuit to the "phone"
behavior (always toggle chrome) if true, otherwise evaluate tapX against
screenWidth * this.TAP_ZONE_RATIO to call goToPreviousPage() or goToNextPage()
or toggle/show/hide chrome and call startChromeAutoHide() as appropriate; update
the block containing ngZone.run, isPhone(), TAP_ZONE_RATIO, goToPreviousPage,
goToNextPage, headerVisible, footerVisible, hideChrome, showChrome, and
startChromeAutoHide so the intent is explicit and this.isPhone() is not
re-evaluated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbf3e924-c936-4bf6-9f56-f01be4ef4bfe
📒 Files selected for processing (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). (3)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend 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.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 (3)
📚 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 (2)
frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (2)
196-214: LGTM — resize-driven phone sync is clean.The
lastIsPhonetransition guard prevents clobbering user pan state on incidental resizes,syncPhoneMode()runs once on init to set initial state, and teardown is wired viadestroyRef.onDestroyrather than a manual cleanup field. This correctly addresses the prior review feedback.
717-745: LGTM —applyPanModecentralizes pan side effects well.
applyPanModeconsolidates signal updates, annotation-tool clearing, and EmbedPDF sync in one place, and bothtoggleAnnotationToolandtogglePanModenow correctly trackuserPanPreferredso leaving phone width restores the user's explicit choice rather than deriving it fromisPhone.
zachyale
approved these changes
Apr 22, 2026
zachyale
pushed a commit
that referenced
this pull request
Apr 22, 2026
…-turn-page zones on mobile devices (#742)
This was referenced Apr 23, 2026
dsmouse
pushed a commit
to dsmouse/grimmory
that referenced
this pull request
May 11, 2026
…-turn-page zones on mobile devices (grimmory-tools#742)
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
This pull request improves the mobile usability of the
PdfReaderComponentby adjusting gesture handling and navigation to prevent accidental page turns and interactions on phones. The main changes ensure that swipe and tap gestures behave differently on phones, prioritizing document selection and navigation over page turning.Mobile gesture and navigation improvements:
ngOnInit, the component now detects if the device is a phone (screen width < 768px) and automatically enables pan mode for a better touch experience.Gesture handling changes:
Summary by CodeRabbit
New Features
Bug Fixes