Skip to content

fix(components): prevent memory leaks by using takeUntil and managing subscriptions#99

Open
balazs-szucs wants to merge 1 commit intogrimmory-tools:developfrom
balazs-szucs:frontend-memory-leaks
Open

fix(components): prevent memory leaks by using takeUntil and managing subscriptions#99
balazs-szucs wants to merge 1 commit intogrimmory-tools:developfrom
balazs-szucs:frontend-memory-leaks

Conversation

@balazs-szucs
Copy link
Member

@balazs-szucs balazs-szucs commented Mar 21, 2026

Description

Linked Issue: Fixes #

Changes

This PR refactor few component that had potential for memory leaks.

Main changes are the following:

  • Store bound function reference for resize listener so removeEventListener actually works
  • Add takeUntilDestroyed / takeUntil(destroy$) to unmanaged observable subscriptions
  • Clean up iframe document listeners via AbortController in ebook reader event service
  • Clear pending timeouts on service destruction
  • Replace reactive subscription with synchronous read in updateBookmarkIndicator to avoid subscription accumulation

Summary by CodeRabbit

  • Bug Fixes
    • Improved subscription lifecycle management across the application to prevent memory leaks. Components now properly unsubscribe from observables when destroyed.
    • Enhanced event listener cleanup with proper reference binding to ensure deterministic removal of handlers.
    • Added automatic cleanup of timers and DOM listeners to prevent resource leaks in reader components.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 21, 2026

📝 Walkthrough

Walkthrough

This pull request implements systematic subscription lifecycle management across multiple components and services to prevent memory leaks. Changes introduce automatic unsubscription patterns using both Angular's DestroyRef/takeUntilDestroyed and traditional RxJS takeUntil operators, alongside manual subscription tracking and event listener cleanup mechanisms.

Changes

Cohort / File(s) Summary
Angular DestroyRef Pattern
dashboard-settings.component.ts, main-dashboard.component.ts, file-naming-pattern.component.ts, app.menu.component.ts
Injected DestroyRef and piped subscriptions through takeUntilDestroyed(this.destroyRef) to automatically terminate subscriptions on component destruction.
RxJS takeUntil Pattern
cbx-reader.component.ts, pdf-reader.component.ts
Introduced destroy$ Subject and wrapped route/parameter subscriptions with takeUntil(this.destroy$) for lifecycle-bound unsubscription.
Manual Subscription Tracking
app.component.ts
Wrapped AuthInitializationService readiness subscription with this.subscriptions.push(...) for managed cleanup in ngOnDestroy.
Event Listener Management
book-table.component.ts
Pre-bound resize handler to a private field (boundSetScrollHeight) to ensure consistent function references in both addEventListener and removeEventListener calls.
Advanced Event Lifecycle
event.service.ts
Refactored iframe and window message event handling to use AbortController signals and explicit cleanup tracking (windowMessageHandler, attachedDocCleanups). Updated destroy() to properly remove all listeners and clear timers.
State API Modernization
sidebar.service.ts, ebook-reader.component.ts
Added public currentBookmarks getter to expose BehaviorSubject current value; updated updateBookmarkIndicator() to read state synchronously instead of subscribing reactively.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A hop through subscriptions fine,
Each one untangled, clean, aligned,
No memory leaks shall persist here,
With destroys and takeUntils clear,
Our listeners dance, then disappear! 🎭✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: preventing memory leaks through subscription management techniques.
Description check ✅ Passed The description covers the required template structure and provides specific details about the changes made, though it lacks explicit linked issue reference.
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.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.41.1)
booklore-ui/src/app/features/readers/ebook-reader/core/event.service.ts

[
{
"text": "window.postMessage({\n type: 'iframe-click',\n clientX: viewportX,\n clientY: viewportY,\n iframeLeft: iframeRect.left,\n iframeWidth: iframeRect.width,\n eventClientX: event.clientX,\n target: (event.target as HTMLElement)?.tagName\n }, '')",
"range": {
"byteOffset": {
"start": 7960,
"end": 8248
},
"start": {
"line": 228,
"column": 6
},
"end": {
"line": 236,
"column": 13
}
},
"file": "booklore-ui/src/app/features/readers/ebook-reader/core/event.service.ts",
"lines": " window.postMessage({\n type: 'iframe-click',\n clientX: viewportX,\n clientY: viewportY,\n iframeLeft: iframeRect.left,\n iframeWidth: iframeRect.width,\n eventClientX: event.clientX,\n target: (event.target as HTMLElement)?.tagName\n }, '
');",
"charCount": {
"leading": 6,
"trailing": 1
},
"language": "TypeScript

... [truncated 16836 characters] ...

14
}
},
"style": "secondary"
},
{
"text": "({\n type: 'iframe-click',\n clientX: viewportX,\n clientY: iframeRect.top + touch.clientY,\n iframeLeft: iframeRect.left,\n iframeWidth: iframeRect.width,\n eventClientX: touch.clientX,\n target: (event.target as HTMLElement)?.tagName\n }, '*')",
"range": {
"byteOffset": {
"start": 12548,
"end": 12855
},
"start": {
"line": 372,
"column": 26
},
"end": {
"line": 380,
"column": 15
}
},
"style": "secondary"
}
]
}
]
Error: 2 error(s) found in code.
Help: Scan succeeded and found error level diagnostics in the codebase.


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

Tip

CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.

OpenGrep is compatible with Semgrep configurations. Add an opengrep.yml or semgrep.yml configuration file to your project to enable OpenGrep analysis.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
booklore-ui/src/app/features/readers/pdf-reader/pdf-reader.component.ts (1)

68-70: Consider using takeUntil for consistency.

The annotationSaveSubscription is manually unsubscribed while other subscriptions use takeUntil(this.destroy$). For consistency, you could apply the same pattern here.

♻️ Optional: Use takeUntil for consistency
-    this.annotationSaveSubscription = this.annotationSaveSubject
+    this.annotationSaveSubject
       .pipe(debounceTime(1500))
+      .pipe(takeUntil(this.destroy$))
       .subscribe(() => this.persistAnnotations());

Then remove the manual unsubscribe at line 184:

-    this.annotationSaveSubscription?.unsubscribe();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@booklore-ui/src/app/features/readers/pdf-reader/pdf-reader.component.ts`
around lines 68 - 70, The subscription created from annotationSaveSubject should
use takeUntil(this.destroy$) for consistency: change the chain to
this.annotationSaveSubject.pipe(debounceTime(1500),
takeUntil(this.destroy$)).subscribe(() => this.persistAnnotations()); then
remove the manual unsubscribe of annotationSaveSubscription in the component
teardown (and you can remove the annotationSaveSubscription field if it’s no
longer used). This keeps behavior identical while matching other subscriptions
that use destroy$.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@booklore-ui/src/app/features/readers/pdf-reader/pdf-reader.component.ts`:
- Around line 68-70: The subscription created from annotationSaveSubject should
use takeUntil(this.destroy$) for consistency: change the chain to
this.annotationSaveSubject.pipe(debounceTime(1500),
takeUntil(this.destroy$)).subscribe(() => this.persistAnnotations()); then
remove the manual unsubscribe of annotationSaveSubscription in the component
teardown (and you can remove the annotationSaveSubscription field if it’s no
longer used). This keeps behavior identical while matching other subscriptions
that use destroy$.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0db722f1-c47c-4aa4-9c9e-4f3195ed2186

📥 Commits

Reviewing files that changed from the base of the PR and between 722ef97 and ee06c39.

📒 Files selected for processing (11)
  • booklore-ui/src/app/app.component.ts
  • booklore-ui/src/app/features/book/components/book-browser/book-table/book-table.component.ts
  • booklore-ui/src/app/features/dashboard/components/dashboard-settings/dashboard-settings.component.ts
  • booklore-ui/src/app/features/dashboard/components/main-dashboard/main-dashboard.component.ts
  • booklore-ui/src/app/features/readers/cbx-reader/cbx-reader.component.ts
  • booklore-ui/src/app/features/readers/ebook-reader/core/event.service.ts
  • booklore-ui/src/app/features/readers/ebook-reader/ebook-reader.component.ts
  • booklore-ui/src/app/features/readers/ebook-reader/layout/sidebar/sidebar.service.ts
  • booklore-ui/src/app/features/readers/pdf-reader/pdf-reader.component.ts
  • booklore-ui/src/app/features/settings/file-naming-pattern/file-naming-pattern.component.ts
  • booklore-ui/src/app/shared/layout/component/layout-menu/app.menu.component.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant