Web 428 memory leaks in multiple components due to unmanaged subscriptions

[AnvayKharb]

Resolved memory leaks across multiple components by adding proper RxJS subscription cleanup. Updated each affected component to use the takeUntil(destroy$) pattern and implemented ngOnDestroy() to ensure all active subscriptions are terminated when the component is destroyed. This improves performance, prevents unnecessary background processing, and ensures consistent subscription handling throughout the application.

#Issue Number

WEB-428

Screenshots, if any

N/A

Summary by CodeRabbit

• Refactor
  • Improved resource management across many UI areas (holidays, product steppers, notifications, account steps).
  • Strengthened subscription cleanup to prevent memory leaks, reducing potential UI slowdowns or unexpected behavior over time.
  • Result: more stable performance and predictable component teardown during navigation and interactions.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)

Validation error: Unrecognized key(s) in object: 'pre_merge_checks'

⚙️ Configuration instructions

• Please see the configuration documentation for more information.
• You can also validate your configuration using the online YAML validator.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Walkthrough

Added OnDestroy lifecycle handling across multiple Angular components: each component now defines a private destroy$ Subject, pipes subscriptions through takeUntil(this.destroy$), and emits/completes destroy$ in ngOnDestroy to ensure unsubscription.

Changes

Cohort / File(s)	Summary
Holiday management components src/app/organization/holidays/create-holiday/create-holiday.component.ts, src/app/organization/holidays/edit-holiday/edit-holiday.component.ts	Implements OnDestroy, adds private destroy$ = new Subject<void>(), pipes route/form/valueChanges and submit/update subscriptions through takeUntil(this.destroy$), and adds ngOnDestroy() to emit/complete destroy$.
Product charge step components src/app/products/fixed-deposit-products/.../fixed-deposit-product-charges-step.component.ts, src/app/products/loan-products/.../loan-product-charges-step.component.ts, src/app/products/recurring-deposit-products/.../recurring-deposit-product-charges-step.component.ts, src/app/products/share-products/.../share-product-charges-step.component.ts	Components now implement OnDestroy, introduce destroy$ Subject, pipe currencyCode.valueChanges (and other valueChanges like multiDisburseLoan) through takeUntil(this.destroy$), and add ngOnDestroy() to emit/complete destroy$.
Shared notifications & shares account src/app/shared/notifications-tray/notifications-tray.component.ts, src/app/shares/shares-account-stepper/shares-account-charges-step/shares-account-charges-step.component.ts	Adds destroy$ Subject; wraps forkJoin, API calls, and valueChanges subscriptions with takeUntil(this.destroy$); implements OnDestroy and ngOnDestroy() to emit/complete destroy$.

Sequence Diagram(s)

sequenceDiagram
    participant C as Angular Component
    participant O as Observable (forms, valueChanges, API)
    participant D as destroy$ Subject

    C->>O: subscribe(...).pipe(takeUntil(D))
    O-->>C: emit values
    C->>C: handle emissions

    Note over C: Component destroyed (navigation/unmount)
    C->>D: destroy$.next()
    D->>O: takeUntil triggers unsubscription
    C->>D: destroy$.complete()

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Pay special attention to NotificationsTrayComponent for multiple concurrent subscriptions and forkJoin usage.
• Confirm every subscription site in the changed files uses takeUntil(this.destroy$) (including nested/valueChanges and API calls).
• Verify destroy$ is private and is .next() then .complete() in ngOnDestroy().

Suggested reviewers

• alberto-art3ch

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing memory leaks in multiple components by managing subscriptions, which directly aligns with all file modifications implementing OnDestroy and takeUntil patterns.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (1)

2-3: Apply destroy$ pattern consistently to route.data as well

The new destroy$ subject, OnDestroy implementation, and takeUntil(this.destroy$) on reschedulingType.valueChanges clean up the main reactive form subscription nicely. However, the this.route.data.subscribe(...) in the constructor is still a long‑lived subscription that is not tied to the component lifecycle. To fully align with the leak‑prevention goal, consider piping it through takeUntil(this.destroy$) as well:

-    this.route.data.subscribe((data: { holiday: any; holidayTemplate: any }) => {
+    this.route.data
+      .pipe(takeUntil(this.destroy$))
+      .subscribe((data: { holiday: any; holidayTemplate: any }) => {
       this.holidayData = data.holiday;
       this.holidayData.repaymentSchedulingTypes = data.holidayTemplate;
       this.reSchedulingType = this.holidayData.reschedulingType;
       if (this.holidayData.status.value === 'Active') {
         this.isActiveHoliday = true;
       } else {
         this.isActiveHoliday = false;
       }
-    });
+      });

This keeps all ongoing subscriptions in this component consistently tied to destroy$. As per coding guidelines.

Also applies to: 11-12, 31-45, 63-72, 119-137, 174-180

src/app/products/fixed-deposit-products/fixed-deposit-product-stepper/fixed-deposit-product-charges-step/fixed-deposit-product-charges-step.component.ts (1)

83-84: valueChanges subscription now safely tied to component lifecycle

Wiring this.currencyCode.valueChanges through .pipe(takeUntil(this.destroy$)) and emitting/completing destroy$ in ngOnDestroy correctly prevents this form control subscription from leaking after the component is destroyed. Consider adding explicit return types (ngOnInit(): void, ngOnDestroy(): void) in a follow‑up pass to tighten type safety, but this is optional.

Also applies to: 86-88

src/app/products/share-products/share-product-stepper/share-product-charges-step/share-product-charges-step.component.ts (1)

84-85: Form control subscription is now properly cleaned up on destroy

Piping this.currencyCode.valueChanges through takeUntil(this.destroy$) and triggering next()/complete() in ngOnDestroy ensures this subscription cannot leak beyond the component’s lifecycle. As an optional improvement, you might later tighten types (e.g., replace any/{}[] with explicit interfaces) to better match the “strict type safety” guideline.

Also applies to: 87-89

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1ef4de5 and 3e32732.

📒 Files selected for processing (8)

• src/app/organization/holidays/create-holiday/create-holiday.component.ts (6 hunks)
• src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (6 hunks)
• src/app/products/fixed-deposit-products/fixed-deposit-product-stepper/fixed-deposit-product-charges-step/fixed-deposit-product-charges-step.component.ts (4 hunks)
• src/app/products/loan-products/loan-product-stepper/loan-product-charges-step/loan-product-charges-step.component.ts (4 hunks)
• src/app/products/recurring-deposit-products/recurring-deposit-product-stepper/recurring-deposit-product-charges-step/recurring-deposit-product-charges-step.component.ts (4 hunks)
• src/app/products/share-products/share-product-stepper/share-product-charges-step/share-product-charges-step.component.ts (4 hunks)
• src/app/shared/notifications-tray/notifications-tray.component.ts (7 hunks)
• src/app/shares/shares-account-stepper/shares-account-charges-step/shares-account-charges-step.component.ts (5 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

src/app/**

⚙️ CodeRabbit configuration file

src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.

Files:

• src/app/shared/notifications-tray/notifications-tray.component.ts
• src/app/organization/holidays/create-holiday/create-holiday.component.ts
• src/app/products/share-products/share-product-stepper/share-product-charges-step/share-product-charges-step.component.ts
• src/app/shares/shares-account-stepper/shares-account-charges-step/shares-account-charges-step.component.ts
• src/app/products/fixed-deposit-products/fixed-deposit-product-stepper/fixed-deposit-product-charges-step/fixed-deposit-product-charges-step.component.ts
• src/app/products/loan-products/loan-product-stepper/loan-product-charges-step/loan-product-charges-step.component.ts
• src/app/organization/holidays/edit-holiday/edit-holiday.component.ts
• src/app/products/recurring-deposit-products/recurring-deposit-product-stepper/recurring-deposit-product-charges-step/recurring-deposit-product-charges-step.component.ts

⏰ 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: Run Lint, Build and Deploy

🔇 Additional comments (12)

src/app/shares/shares-account-stepper/shares-account-charges-step/shares-account-charges-step.component.ts (1)

2-8: Solid destroy$ + takeUntil integration; subscriptions look well-scoped

The added OnDestroy implementation, destroy$ subject, and takeUntil(this.destroy$) on currencyCode.valueChanges are wired correctly and align with Angular/RxJS best practices for avoiding leaks. The remaining dialog afterClosed() subscriptions are short-lived and complete on close, so leaving them as-is is fine.

Also applies to: 62-88, 97-105, 115-118

src/app/products/loan-products/loan-product-stepper/loan-product-charges-step/loan-product-charges-step.component.ts (1)

1-8: Charges-step subscriptions correctly bound to component lifecycle

Using destroy$ with takeUntil on both currencyCode.valueChanges and multiDisburseLoan.valueChanges, plus the new ngOnDestroy, provides proper cleanup for the two long‑lived streams in this component. If there are code paths where either input control might be undefined at ngOnInit, consider adding a simple null guard before subscribing; otherwise this implementation looks good.

Also applies to: 58-77, 84-97, 99-101

src/app/products/recurring-deposit-products/recurring-deposit-product-stepper/recurring-deposit-product-charges-step/recurring-deposit-product-charges-step.component.ts (1)

1-8: Recurring-deposit charges subscription cleanup looks correct

The addition of destroy$, OnDestroy, and takeUntil(this.destroy$) on currencyCode.valueChanges correctly scopes the only ongoing subscription in this component to its lifecycle. This matches the cleanup pattern used elsewhere in the PR and should eliminate the previous leak.

Also applies to: 52-67, 74-82, 84-86

src/app/products/fixed-deposit-products/fixed-deposit-product-stepper/fixed-deposit-product-charges-step/fixed-deposit-product-charges-step.component.ts (2)

1-1: Subscription‑cleanup imports and OnDestroy integration look correct

Importing OnDestroy, Subject, and takeUntil, and updating the class to implement OnInit, OnDestroy cleanly supports the new takeUntil(destroy$) pattern; no issues here.

Also applies to: 5-7, 54-54

---

68-69: destroy$ subject is correctly scoped and typed

private destroy$ = new Subject<void>(); is a good fit for centralizing subscription teardown and is properly kept private to the component.

src/app/products/share-products/share-product-stepper/share-product-charges-step/share-product-charges-step.component.ts (2)

1-1: Consistent lifecycle‑aware imports and interface usage

Bringing in OnDestroy, Subject, and takeUntil and updating the class to implement OnInit, OnDestroy aligns this component with the shared subscription‑cleanup pattern used elsewhere in the PR.

Also applies to: 5-7, 54-54

---

69-71: destroy$ setup is appropriate for managing subscriptions

The private destroy$ = new Subject<void>(); field is correctly declared for use as a teardown notifier and kept private to avoid external misuse.

src/app/shared/notifications-tray/notifications-tray.component.ts (5)

5-6: RxJS imports and destroy$ field correctly prepared for teardown

Importing Subject and takeUntil, and adding private destroy$ = new Subject<void>();, is an appropriate foundation for centralizing subscription cleanup in this component.

Also applies to: 50-51

---

73-83: Constructor subscription and ngOnDestroy are now lifecycle‑safe

Wrapping the initial forkJoin([...getNotifications(...)]) subscription with .pipe(takeUntil(this.destroy$)) and then emitting/completing destroy$ in ngOnDestroy, while still calling destroy() to clear the timer, correctly prevents notification subscriptions from surviving past component destruction.

Also applies to: 89-93

---

110-117: Polling subscription tied to destroy$ while timer is still cleared

The fetchUnreadNotifications HTTP call is now guarded by takeUntil(this.destroy$), and with the existing clearTimeout(this.timer) in destroy(), both the polling subscription and its scheduling are safely bounded by the component lifecycle.

---

127-133: updateNotifications subscription no longer risks lingering

Wrapping updateNotifications() in takeUntil(this.destroy$) ensures this side‑effecting call cannot outlive the component if the menu closes near teardown; the local state updates remain unchanged.

---

142-149: Mock notifications subscription is also cleaned up

Guarding getMockUnreadNotification() with takeUntil(this.destroy$) brings the test/mock path in line with the production paths, preventing accidental leaks when using the mock helper.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

src/app/products/loan-products/loan-product-stepper/loan-product-charges-step/loan-product-charges-step.component.ts (1)

1-1: Loan product charges: all long‑lived subscriptions now properly scoped to lifecycle

• Both currencyCode.valueChanges and multiDisburseLoan.valueChanges are now piped through takeUntil(this.destroy$), and ngOnDestroy correctly signals and completes destroy$, so these form‑control streams will no longer outlive the component.
• The pattern matches the other charges‑step components in this PR, giving you consistent subscription handling across the product steppers.

You might later consider a small shared base class or helper for the repeated destroy$ + ngOnDestroy pattern across the various *ChargesStepComponents, but that’s purely a maintainability win, not required here.

Also applies to: 5-7, 58-58, 76-78, 84-97, 99-101

src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (1)

2-2: EditHoliday: lifecycle‑aware subscriptions implemented correctly

• destroy$ and OnDestroy are wired correctly, and the key subscriptions (route.data, reschedulingType.valueChanges, and the updateHoliday call in submit()) are now all scoped with takeUntil(this.destroy$), eliminating the previous risk of lingering subscriptions when the component is destroyed.
• Given getReschedulingType() is only called after the reschedulingType control is added for inactive holidays, accessing .get('reschedulingType') is safe in practice; if you ever change that flow, a small defensive check for a missing control would be the only refinement to consider.

Also applies to: 11-12, 31-31, 44-45, 63-73, 123-136, 168-175, 177-183

src/app/products/share-products/share-product-stepper/share-product-charges-step/share-product-charges-step.component.ts (1)

55-61: Optional: tighten typings for inputs and charge data

shareProductsTemplate: any, chargeData: any, and chargesDataSource: {}[] make this area loosely typed; defining an interface for the share product template and charge shape would help catch errors earlier and improve IDE support. This can be a follow-up and is not required for the subscription-leak fix.

src/app/shared/notifications-tray/notifications-tray.component.ts (1)

110-121: Optional: consider RxJS-based polling instead of setTimeout recursion

For a future cleanup, you could replace the manual setTimeout recursion in fetchUnreadNotifications with an RxJS interval/timer + switchMap chain also governed by takeUntil(this.destroy$). That would unify the polling logic with your observable-based teardown and further simplify destroy(). Not required for this PR.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3e32732 and 43854ce.

📒 Files selected for processing (8)

• src/app/organization/holidays/create-holiday/create-holiday.component.ts (8 hunks)
• src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (7 hunks)
• src/app/products/fixed-deposit-products/fixed-deposit-product-stepper/fixed-deposit-product-charges-step/fixed-deposit-product-charges-step.component.ts (4 hunks)
• src/app/products/loan-products/loan-product-stepper/loan-product-charges-step/loan-product-charges-step.component.ts (4 hunks)
• src/app/products/recurring-deposit-products/recurring-deposit-product-stepper/recurring-deposit-product-charges-step/recurring-deposit-product-charges-step.component.ts (4 hunks)
• src/app/products/share-products/share-product-stepper/share-product-charges-step/share-product-charges-step.component.ts (4 hunks)
• src/app/shared/notifications-tray/notifications-tray.component.ts (7 hunks)
• src/app/shares/shares-account-stepper/shares-account-charges-step/shares-account-charges-step.component.ts (5 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

src/app/**

⚙️ CodeRabbit configuration file

src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.

Files:

• src/app/products/recurring-deposit-products/recurring-deposit-product-stepper/recurring-deposit-product-charges-step/recurring-deposit-product-charges-step.component.ts
• src/app/organization/holidays/edit-holiday/edit-holiday.component.ts
• src/app/organization/holidays/create-holiday/create-holiday.component.ts
• src/app/products/loan-products/loan-product-stepper/loan-product-charges-step/loan-product-charges-step.component.ts
• src/app/shares/shares-account-stepper/shares-account-charges-step/shares-account-charges-step.component.ts
• src/app/products/share-products/share-product-stepper/share-product-charges-step/share-product-charges-step.component.ts
• src/app/shared/notifications-tray/notifications-tray.component.ts
• src/app/products/fixed-deposit-products/fixed-deposit-product-stepper/fixed-deposit-product-charges-step/fixed-deposit-product-charges-step.component.ts

🧬 Code graph analysis (1)

src/app/organization/holidays/create-holiday/create-holiday.component.ts (1)

src/app/organization/holidays/create-holiday/checklist-db.class.ts (1)

• data (11-13)

⏰ 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: Run Lint, Build and Deploy

🔇 Additional comments (8)

src/app/shares/shares-account-stepper/shares-account-charges-step/shares-account-charges-step.component.ts (1)

2-8: Subscription cleanup pattern is correctly applied here

• Importing OnDestroy, Subject, and takeUntil, adding destroy$, and wiring currencyCode.valueChanges through takeUntil(this.destroy$) with a matching ngOnDestroy is correct and will prevent the previous long‑lived subscription leak.
• Dialog afterClosed() subscriptions remain fine as those observables complete on close, so focusing takeUntil on the FormControl stream is appropriate.

Also applies to: 62-62, 86-88, 97-106, 115-118

src/app/products/recurring-deposit-products/recurring-deposit-product-stepper/recurring-deposit-product-charges-step/recurring-deposit-product-charges-step.component.ts (1)

1-1: Recurring deposit charges: RxJS teardown looks good

The newly added destroy$ subject, OnDestroy implementation, and the currencyCode.valueChanges.pipe(takeUntil(this.destroy$)) subscription form a clean teardown pattern and should address the leak in this component without changing behavior.

Also applies to: 5-7, 52-52, 66-68, 74-82, 84-87

src/app/products/fixed-deposit-products/fixed-deposit-product-stepper/fixed-deposit-product-charges-step/fixed-deposit-product-charges-step.component.ts (1)

1-1: Fixed deposit charges: correct use of destroy$ for FormControl stream

The added destroy$ subject, OnDestroy implementation, and wrapping of currencyCode.valueChanges with takeUntil(this.destroy$) correctly bound the only long‑lived subscription in this component to its lifecycle, aligning it with the rest of the PR’s cleanup work.

Also applies to: 5-7, 54-54, 68-70, 76-84, 86-88

src/app/organization/holidays/create-holiday/create-holiday.component.ts (3)

3-13: destroy$ lifecycle wiring and imports look correct

Importing OnDestroy, Subject, and takeUntil, having the class implement OnDestroy, and declaring private destroy$ = new Subject<void>() is idiomatic and sets up a clean lifecycle boundary for all observable chains. No issues here.
As per coding guidelines, please just re-run the holiday-related unit/integration tests to confirm everything still compiles and behaves as expected.

Also applies to: 57-69

---

118-133: All relevant subscriptions now consistently use takeUntil(this.destroy$)

The subscriptions on route.data, this._database.dataChange, reschedulingType.valueChanges, and organizationService.createHoliday(data) are all correctly piped through takeUntil(this.destroy$), which addresses the earlier unmanaged-subscription leaks and keeps the component’s observable usage consistent. This aligns well with the PR’s objective.
As per coding guidelines, please verify at runtime that holiday creation, office tree rendering, and rescheduling-type toggling still behave as before.

Also applies to: 321-330, 360-371

---

374-379: ngOnDestroy correctly tears down destroy$

Calling this.destroy$.next() followed by this.destroy$.complete() is the expected pattern to terminate all takeUntil(this.destroy$) chains when the component is destroyed; no extra cleanup is needed here.
Consider adding/maintaining a simple component-destroy test (if feasible) to validate no subscriptions remain active after navigating away.

src/app/products/share-products/share-product-stepper/share-product-charges-step/share-product-charges-step.component.ts (1)

1-7: destroy$ pattern for currencyCode.valueChanges is implemented correctly

Adding OnDestroy, importing Subject/takeUntil, introducing private destroy$ = new Subject<void>(), and piping this.currencyCode.valueChanges through takeUntil(this.destroy$) with ngOnDestroy completing the subject gives this component a clean subscription lifecycle. The dialog’s afterClosed() subscription remains a one-shot stream, so leaving it as-is is fine.
As per coding guidelines, please confirm manually that changing the currency still resets chargesDataSource and that no console warnings about subscriptions appear on repeated navigation.

Also applies to: 54-71, 84-89

src/app/shared/notifications-tray/notifications-tray.component.ts (1)

5-7: Subscription cleanup for notifications is comprehensive and consistent

Using private destroy$ = new Subject<void>() plus takeUntil(this.destroy$) on the initial forkJoin, getNotifications(false, 9), updateNotifications(), and getMockUnreadNotification() calls—combined with destroy() clearing the timer and ngOnDestroy nexting/completing destroy$—gives this component a clean teardown path for both HTTP streams and the polling loop. This should effectively address the previously leaking subscriptions.
As per coding guidelines, please verify manually (e.g., by opening/closing the notifications tray and navigating around) that notifications still load and update correctly with no lingering network activity after component destruction.

Also applies to: 50-52, 73-83, 89-93, 111-117, 129-132, 143-149