Skip to content

Conversation

@AnvayKharb
Copy link
Contributor

@AnvayKharb AnvayKharb commented Nov 21, 2025

Fixed a memory leak in the Holiday components by ensuring that all valueChanges subscriptions are properly unsubscribed. The previous implementation created subscriptions without cleanup, causing them to persist even after the component was destroyed.

This update adds a destroy$ subject and applies takeUntil() to the form control subscriptions, ensuring they are disposed correctly during ngOnDestroy().

#Issue Number

WEB-427

Screenshots, if any

N/A

Summary by CodeRabbit

  • Refactor
    • Improved subscription and resource management in holiday creation and editing workflows for enhanced application stability and performance.

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

…oliday components

- Added OnDestroy lifecycle hook to both Create and Edit Holiday components
- Imported Subject and takeUntil from rxjs
- Created destroy$ Subject to manage subscription cleanup
- Applied takeUntil operator to form valueChanges subscriptions
- Properly unsubscribe on component destruction to prevent memory leaks
- Fixes potential performance issues from accumulated subscriptions
@coderabbitai
Copy link

coderabbitai bot commented Nov 21, 2025

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

Two Angular components now implement the OnDestroy lifecycle hook with a destroy$ Subject to properly manage subscription cleanup. The reschedulingType form control subscription in each component uses takeUntil(destroy$) to unsubscribe automatically on component destruction.

Changes

Cohort / File(s) Summary
Memory leak prevention for form subscriptions
src/app/organization/holidays/create-holiday/create-holiday.component.ts, src/app/organization/holidays/edit-holiday/edit-holiday.component.ts
Added OnDestroy lifecycle hook, imported OnDestroy, Subject, and takeUntil. Created private destroy$ Subject. Wrapped reschedulingType valueChanges subscription with takeUntil(this.destroy$). Implemented ngOnDestroy() to emit and complete destroy$ stream.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Standard Angular memory leak prevention pattern applied consistently across two similar files
  • No complex logic changes, only lifecycle management implementation
  • Repetitive pattern reduces cognitive load per file

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 summarizes the main change: fixing memory leaks by properly unsubscribing from form valueChanges subscriptions.
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

📜 Recent 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 f43695e.

📒 Files selected for processing (2)
  • src/app/organization/holidays/create-holiday/create-holiday.component.ts (6 hunks)
  • src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (6 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/organization/holidays/edit-holiday/edit-holiday.component.ts
  • src/app/organization/holidays/create-holiday/create-holiday.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 (10)
src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (5)

2-2: LGTM! Required imports added correctly.

All necessary imports for implementing the OnDestroy lifecycle pattern are present.

Also applies to: 11-12


31-31: LGTM! OnDestroy interface properly declared.

The class correctly implements the OnDestroy lifecycle hook.


44-45: LGTM! Destroy subject properly initialized.

The destroy$ Subject is correctly declared with appropriate access modifier and clear documentation.


123-136: LGTM! Subscription properly managed with takeUntil.

The form control valueChanges subscription now correctly uses takeUntil(this.destroy$) to ensure cleanup on component destruction, addressing the memory leak.


174-180: LGTM! ngOnDestroy properly implemented.

The lifecycle hook correctly emits and completes the destroy$ Subject, ensuring all subscriptions using takeUntil(this.destroy$) are properly cleaned up.

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

3-3: LGTM! Required imports added correctly.

All necessary imports for implementing the OnDestroy lifecycle pattern are present.

Also applies to: 12-13


57-57: LGTM! OnDestroy interface properly declared.

The class correctly implements the OnDestroy lifecycle hook.


68-69: LGTM! Destroy subject properly initialized.

The destroy$ Subject is correctly declared with appropriate access modifier and clear documentation.


321-330: LGTM! Subscription properly managed with takeUntil.

The form control valueChanges subscription now correctly uses takeUntil(this.destroy$) to ensure cleanup on component destruction, addressing the memory leak.


374-377: LGTM! ngOnDestroy properly implemented.

The lifecycle hook correctly emits and completes the destroy$ Subject, ensuring all subscriptions using takeUntil(this.destroy$) are properly cleaned up.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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

❤️ Share

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

@AnvayKharb
Copy link
Contributor Author

heyy @gkbishnoi07 @alberto-art3ch please take a look

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