WEB-954: Migrate Subscription management : loans - #3621
Conversation
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Core account creation and GLIM account components src/app/loans/create-loans-account/create-loans-account.component.ts, src/app/loans/edit-loans-account/edit-loans-account.component.ts, src/app/loans/glim-account/... |
CreateLoansAccountComponent, EditLoansAccountComponent, and GLIM account components inject DestroyRef and pipe ActivatedRoute.data through takeUntilDestroyed for automatic unsubscription on destroy. |
Collateral dialog and form value change handlers src/app/loans/custom-dialog/loans-account-add-collateral-dialog/loans-account-add-collateral-dialog.component.ts |
LoansAccountAddCollateralDialogComponent pipes form control valueChanges (collateral and quantity) through takeUntilDestroyed, adding guards that clear computed totals when collateralData is missing or quantity is invalid. |
Loans account stepper form controls src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.ts, src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts |
LoansAccountDetailsStepComponent removes OnDestroy lifecycle and _onDestroy Subject, replacing with DestroyRef. LoansAccountTermsStepComponent applies takeUntilDestroyed to repaymentFrequencyType, numberOfRepayments, loanTermFrequencyType, amortizationType, and interestRatePerPeriod subscriptions while preserving validator and numeric clamping logic. |
General loans-view information and asset tabs src/app/loans/loans-view/account-details/..., src/app/loans/loans-view/charges-tab/..., src/app/loans/loans-view/external-asset-owner-tab/..., src/app/loans/loans-view/floating-interest-rates/..., src/app/loans/loans-view/general-tab/... |
Account-details, charges, external-asset-owner, floating-interest-rates, and general-tab components use takeUntilDestroyed for route.parent.data or route.data subscriptions. |
Loans-view datatable, collateral, and deferred-income tabs src/app/loans/loans-view/datatable-tab/..., src/app/loans/loans-view/loan-collateral-tab/..., src/app/loans/loans-view/loan-deferred-income-tab/... |
Datatable-tab moves route.data subscription to constructor with takeUntilDestroyed for entityDatatable and multiRowDatatableFlag setup. Collateral-tab and deferred-income-tab apply lifecycle-aware cleanup to route data subscriptions. |
Loans-view delinquency, documents, originators, and term-variation tabs src/app/loans/loans-view/loan-delinquency-tags-tab/..., src/app/loans/loans-view/loan-documents-tab/..., src/app/loans/loans-view/loan-originators-tab/..., src/app/loans/loans-view/loan-term-variations-tab/... |
Delinquency-tags-tab, documents-tab, originators-tab, and term-variations-tab use takeUntilDestroyed for route parent data subscriptions while maintaining existing callback logic. |
Loans-view tranche, schedule, overdue, and reschedule tabs src/app/loans/loans-view/loan-tranche-details/..., src/app/loans/loans-view/original-schedule-tab/..., src/app/loans/loans-view/overdue-charges-tab/..., src/app/loans/loans-view/reschedule-loan-tab/... |
Tranche-details, original-schedule-tab, overdue-charges-tab, and reschedule-loan-tab pipe route parent data subscriptions through takeUntilDestroyed. |
Main loans view and notes tab src/app/loans/loans-view/loans-view.component.ts, src/app/loans/loans-view/notes-tab/notes-tab.component.ts |
LoansViewComponent manages both ActivatedRoute.data and params subscriptions with takeUntilDestroyed. NotesTabComponent applies the same pattern to route.data and route.parent.params. |
Standing instructions and working-capital tabs src/app/loans/loans-view/standing-instructions-tab/..., src/app/loans/loans-view/working-capital/loan-period-payment-rates/... |
Standing-instructions-tab and loan-period-payment-rates use takeUntilDestroyed for automatic route data subscription cleanup. |
Loan action components (charges, approval, foreclosure) src/app/loans/loans-view/loan-account-actions/add-loan-charge/..., src/app/loans/loans-view/loan-account-actions/adjust-loan-charge/..., src/app/loans/loans-view/loan-account-actions/approve-loan/..., src/app/loans/loans-view/loan-account-actions/foreclosure/... |
Add-loan-charge, adjust-loan-charge, approve-loan, and foreclosure components use takeUntilDestroyed for route.data and form control valueChanges subscriptions, preserving charge selection and dueDate control toggling logic. |
Guarantor creation and loan account actions src/app/loans/loans-view/loan-account-actions/create-guarantor/..., src/app/loans/loans-view/loan-account-actions/loan-account-actions.component.ts |
CreateGuarantorComponent applies takeUntilDestroyed to existingClient toggle and client-name search subscriptions. LoanAccountActionsComponent manages both route.data and route.params with lifecycle-aware cleanup. |
Prepay-loan and transaction components src/app/loans/loans-view/loan-account-actions/prepay-loan/..., src/app/loans/loans-view/transactions/edit-transaction/..., src/app/loans/loans-view/transactions/export-transactions/... |
Prepay-loan uses takeUntilDestroyed for transactionDate valueChanges while preserving date formatting and template retrieval. Edit-transaction and export-transactions apply the pattern to route data subscriptions. |
Transaction view/receipt and charge view components src/app/loans/loans-view/transactions/view-reciept/..., src/app/loans/loans-view/view-charge/... |
View-receipt and view-charge components use takeUntilDestroyed for ActivatedRoute.data subscriptions while maintaining pentahoUrl generation and charge data assignments. |
Loan dashboard and subscription cleanup src/app/loans/loans-view/loan-account-dashboard/loan-account-dashboard.component.ts |
LoanAccountDashboardComponent removes manual Subscription tracking and ngOnDestroy cleanup, using takeUntilDestroyed for route-data and language-change subscriptions while preserving chart and timeout logic. |
Repayment form model and component cleanup
| Layer / File(s) | Summary |
|---|---|
Repayment form model definition src/app/loans/models/loan-form.model.ts |
New RepaymentForm interface defines typed FormControl fields for repayment inputs: required (transactionDate, externalId, paymentTypeId, note, skipInterestRefund, transactionAmount) and optional (classificationId, accountNumber, checkNumber, routingCode, receiptNumber, bankNumber). |
Make repayment component cleanup src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.ts |
Remove unused standalone component imports (MatCheckbox, CdkTextareaAutosize) and debug console.log statements from submit() working-capital and loan-action handling flows. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- openMF/web-app#3257: Updates CreateLoansAccountComponent initialization to pass loan and product data from route resolver, complementing this PR's subscription lifecycle refactoring.
Suggested reviewers
- IOhacker
- adamsaghy
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The PR title 'WEB-954: Migrate Subscription management : loans' clearly and specifically describes the main change—migrating subscription management patterns in loan components. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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 @coderabbitai help to get the list of available commands and usage tips.
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)
src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts (1)
111-123:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftNested subscriptions lack lifecycle management.
The
addPaymentRate()method contains three nested subscriptions withouttakeUntilDestroyed(). If the component is destroyed while the dialog is open or while API calls are pending, these subscriptions will leak—exactly the issue this PR aims to fix.🔧 Proposed fix to add lifecycle management
- dialogRef.afterClosed().subscribe((response: any) => { + dialogRef.afterClosed() + .pipe(takeUntilDestroyed(this.destroyRef)) + .subscribe((response: any) => { if (response?.data) { const { periodPaymentRate, note } = response.data.value; const payload = { periodPaymentRate: periodPaymentRate, note: note || '', locale: this.settingsService.language.code }; - this.loanService.addWorkingCapitalPeriodPaymentRate(this.loanId, payload).subscribe((response: any) => { - this.loanService.getWorkingCapitalPeriodPaymentRates(this.loanId).subscribe((data: any) => { + this.loanService.addWorkingCapitalPeriodPaymentRate(this.loanId, payload) + .pipe(takeUntilDestroyed(this.destroyRef)) + .subscribe((response: any) => { + this.loanService.getWorkingCapitalPeriodPaymentRates(this.loanId) + .pipe(takeUntilDestroyed(this.destroyRef)) + .subscribe((data: any) => { this.loanPaymentRatesData = data; }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts` around lines 111 - 123, The nested subscriptions in dialogRef.afterClosed() (response handling), loanService.addWorkingCapitalPeriodPaymentRate and loanService.getWorkingCapitalPeriodPaymentRates are not tied to component lifecycle and can leak; refactor to a single observable chain by piping dialogRef.afterClosed() through RxJS operators (e.g., filter/map and switchMap/concatMap) to call addWorkingCapitalPeriodPaymentRate and then getWorkingCapitalPeriodPaymentRates, and apply lifecycle teardown (e.g., takeUntilDestroyed(this) or untilDestroyed(this) in the pipe) so the subscription is automatically cleaned up when the component is destroyed.
🧹 Nitpick comments (3)
src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.ts (1)
118-124: 💤 Low valueAdd null guard for consistency with other methods.
repaymentLoanFormis typed asFormGroup<RepaymentForm> | null, butsetRepaymentLoanDetails()accesses it without a null check. While the call order inngOnInitensures the form exists, adding a guard maintains consistency withupdateTransactionAmountValidatorsandsubmit().♻️ Suggested fix
setRepaymentLoanDetails() { + if (!this.repaymentLoanForm) { + return; + } this.paymentTypes = this.dataObject.paymentTypeOptions; this.classificationOptions = this.dataObject.classificationOptions; this.repaymentLoanForm.patchValue({ transactionAmount: this.dataObject.amount }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.ts` around lines 118 - 124, Add a null guard in setRepaymentLoanDetails so it checks repaymentLoanForm before accessing it; specifically, inside the setRepaymentLoanDetails method, return early if this.repaymentLoanForm is null (mirroring patterns used in updateTransactionAmountValidators and submit) and only call this.repaymentLoanForm.patchValue(...) when the form exists, leaving assignment of this.paymentTypes and this.classificationOptions unchanged.src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts (1)
111-111: ⚖️ Poor tradeoffConsider typing Observable responses instead of
any.The subscriptions use
anyfor their response types. Introducing specific interfaces for dialog responses and service method return types would improve type safety and catch potential errors at compile time.Based on learnings, avoid using
Observable<any>as a pattern. When you encounter API responses, introduce specific interfaces/types for the response shapes and use proper typing instead ofany.Also applies to: 119-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts` at line 111, The subscription to dialogRef.afterClosed().subscribe((response: any) => ...) (and the other subscribe at lines 119-120) uses any; replace it with a properly named interface (e.g., LoanPeriodDialogResult) describing the dialog payload and use that type in the Observable and subscribe callback signature; update the dialog open call to use MatDialogRef<LoanPeriodDialogComponent, LoanPeriodDialogResult> (or equivalent) and change any service return types from Observable<any> to Observable<ConcreteResponseType> so the callbacks and service methods are strongly typed throughout.src/app/loans/loans-view/loan-account-dashboard/loan-account-dashboard.component.ts (1)
60-60: ⚖️ Poor tradeoffConsider typing
loanDataandloanDetailsDatainstead ofany.The component uses
anytype for loan data throughout. Introducing a specific interface for the loan details response would improve type safety and enable better IDE support.Based on learnings, avoid using
Observable<any>as a pattern. When you encounter API responses, introduce specific interfaces/types for the response shapes and use proper typing instead ofany.Also applies to: 74-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/loans/loans-view/loan-account-dashboard/loan-account-dashboard.component.ts` at line 60, Define a concrete TypeScript interface (e.g., LoanDetails or LoanAccountData) that models the API shape returned to this component and replace the two any usages (loanData and loanDetailsData) with that interface; update any Observable<any> types to Observable<LoanDetails> (or Observable<LoanDetails[]>, as appropriate) and adjust method signatures in LoanAccountDashboardComponent (and any service method return types it consumes) to return the strongly typed response so the component properties and subscriptions use the new type instead of any.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/app/loans/custom-dialog/loans-account-add-collateral-dialog/loans-account-add-collateral-dialog.component.ts`:
- Around line 93-99: The subscription in
addCollateralForm.controls.quantity.valueChanges is dereferencing
this.collateralData (used to compute totalValue and totalCollateralValue)
without guarding for null/undefined; update the callback in
loans-account-add-collateral-dialog.component.ts to first check that
this.collateralData exists (or provide safe defaults) before using its
properties — e.g., use a truthy check or optional chaining on
this.collateralData and fallback 0 for basePrice and pctToBase so patchValue
only writes computed numbers and never throws when quantity is entered before a
collateral is selected.
In
`@src/app/loans/loans-view/loan-account-dashboard/loan-account-dashboard.component.ts`:
- Line 74: The code uses a non-null assertion on this.route.parent!.data which
can throw if parent is null; change to a null-safe guard: capture const parent =
this.route.parent (or use this.route.parent?) and only call
parent.data.pipe(...).subscribe(...) when parent is truthy (or use optional
chaining like this.route.parent?.data?.pipe(...)). Update the subscription to
use the existing takeUntilDestroyed(this.destroyRef) and the same callback
(data: { loanDetailsData: any }) so behavior stays identical but without the
unsafe non-null assertion.
---
Outside diff comments:
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts`:
- Around line 111-123: The nested subscriptions in dialogRef.afterClosed()
(response handling), loanService.addWorkingCapitalPeriodPaymentRate and
loanService.getWorkingCapitalPeriodPaymentRates are not tied to component
lifecycle and can leak; refactor to a single observable chain by piping
dialogRef.afterClosed() through RxJS operators (e.g., filter/map and
switchMap/concatMap) to call addWorkingCapitalPeriodPaymentRate and then
getWorkingCapitalPeriodPaymentRates, and apply lifecycle teardown (e.g.,
takeUntilDestroyed(this) or untilDestroyed(this) in the pipe) so the
subscription is automatically cleaned up when the component is destroyed.
---
Nitpick comments:
In
`@src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.ts`:
- Around line 118-124: Add a null guard in setRepaymentLoanDetails so it checks
repaymentLoanForm before accessing it; specifically, inside the
setRepaymentLoanDetails method, return early if this.repaymentLoanForm is null
(mirroring patterns used in updateTransactionAmountValidators and submit) and
only call this.repaymentLoanForm.patchValue(...) when the form exists, leaving
assignment of this.paymentTypes and this.classificationOptions unchanged.
In
`@src/app/loans/loans-view/loan-account-dashboard/loan-account-dashboard.component.ts`:
- Line 60: Define a concrete TypeScript interface (e.g., LoanDetails or
LoanAccountData) that models the API shape returned to this component and
replace the two any usages (loanData and loanDetailsData) with that interface;
update any Observable<any> types to Observable<LoanDetails> (or
Observable<LoanDetails[]>, as appropriate) and adjust method signatures in
LoanAccountDashboardComponent (and any service method return types it consumes)
to return the strongly typed response so the component properties and
subscriptions use the new type instead of any.
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts`:
- Line 111: The subscription to dialogRef.afterClosed().subscribe((response:
any) => ...) (and the other subscribe at lines 119-120) uses any; replace it
with a properly named interface (e.g., LoanPeriodDialogResult) describing the
dialog payload and use that type in the Observable and subscribe callback
signature; update the dialog open call to use
MatDialogRef<LoanPeriodDialogComponent, LoanPeriodDialogResult> (or equivalent)
and change any service return types from Observable<any> to
Observable<ConcreteResponseType> so the callbacks and service methods are
strongly typed throughout.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0912a1b-5e19-47f0-90bf-6ae30af02b8d
📒 Files selected for processing (46)
src/app/loans/common-resolvers/loan-action-button.resolver.tssrc/app/loans/create-loans-account/create-loans-account.component.tssrc/app/loans/custom-dialog/loans-account-add-collateral-dialog/loans-account-add-collateral-dialog.component.tssrc/app/loans/edit-loans-account/edit-loans-account.component.tssrc/app/loans/glim-account/create-glim-account/create-glim-account.component.tssrc/app/loans/glim-account/glim-account.component.tssrc/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.tssrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.tssrc/app/loans/loans-view/account-details/account-details.component.tssrc/app/loans/loans-view/charges-tab/charges-tab.component.tssrc/app/loans/loans-view/datatable-tab/datatable-tab.component.tssrc/app/loans/loans-view/external-asset-owner-tab/external-asset-owner-tab.component.tssrc/app/loans/loans-view/floating-interest-rates/floating-interest-rates.component.tssrc/app/loans/loans-view/general-tab/general-tab.component.tssrc/app/loans/loans-view/loan-account-actions/add-loan-charge/add-loan-charge.component.tssrc/app/loans/loans-view/loan-account-actions/adjust-loan-charge/adjust-loan-charge.component.tssrc/app/loans/loans-view/loan-account-actions/approve-loan/approve-loan.component.tssrc/app/loans/loans-view/loan-account-actions/create-guarantor/create-guarantor.component.tssrc/app/loans/loans-view/loan-account-actions/foreclosure/foreclosure.component.tssrc/app/loans/loans-view/loan-account-actions/loan-account-actions.component.tssrc/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.htmlsrc/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.tssrc/app/loans/loans-view/loan-account-actions/prepay-loan/prepay-loan.component.tssrc/app/loans/loans-view/loan-account-dashboard/loan-account-dashboard.component.tssrc/app/loans/loans-view/loan-collateral-tab/loan-collateral-tab.component.tssrc/app/loans/loans-view/loan-deferred-income-tab/loan-deferred-income-tab.component.tssrc/app/loans/loans-view/loan-delinquency-tags-tab/loan-delinquency-tags-tab.component.tssrc/app/loans/loans-view/loan-documents-tab/loan-documents-tab.component.tssrc/app/loans/loans-view/loan-originators-tab/loan-originators-tab.component.tssrc/app/loans/loans-view/loan-term-variations-tab/loan-term-variations-tab.component.tssrc/app/loans/loans-view/loan-tranche-details/loan-tranche-details.component.tssrc/app/loans/loans-view/loans-view.component.tssrc/app/loans/loans-view/notes-tab/notes-tab.component.tssrc/app/loans/loans-view/original-schedule-tab/original-schedule-tab.component.tssrc/app/loans/loans-view/overdue-charges-tab/overdue-charges-tab.component.tssrc/app/loans/loans-view/reschedule-loan-tab/reschedule-loan-tab.component.tssrc/app/loans/loans-view/standing-instructions-tab/standing-instructions-tab.component.tssrc/app/loans/loans-view/transactions-tab/transactions-tab.component.tssrc/app/loans/loans-view/transactions/edit-transaction/edit-transaction.component.tssrc/app/loans/loans-view/transactions/export-transactions/export-transactions.component.tssrc/app/loans/loans-view/transactions/view-reciept/view-reciept.component.tssrc/app/loans/loans-view/transactions/view-transaction/view-transaction.component.tssrc/app/loans/loans-view/view-charge/view-charge.component.tssrc/app/loans/loans-view/working-capital/loan-balances-tab/loan-balances-tab.component.tssrc/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.tssrc/app/loans/models/loan-form.model.ts
💤 Files with no reviewable changes (1)
- src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.html
9dfe33c to
8fce37c
Compare
Migrate subscription management from Subject / takeUntil to DestroyRef / takeUntilDestroyed across the application
Refactor to replace the manual subscription cleanup pattern (Subject + takeUntil(this.destroy$) + ngOnDestroy) with Angular's built-in DestroyRef token and the takeUntilDestroyed() operator.
Context
We were having valueChanges.subscribe() calls were piped. Now we are ensuring with takeUntilDestroyed(this.destroyRef), on every form subscription automatically cancels when its component is destroyed. Before this change, navigating away from a form left active listeners running in the background, leaking memory on every route visit
Related issues and discussion
WEB-954
Screenshots, if any
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit