WEB-954: Migrate Subscription management : loans#3621
Open
alberto-art3ch wants to merge 1 commit into
Open
Conversation
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Core loan account and management components src/app/loans/common-resolvers/loan-action-button.resolver.ts, src/app/loans/create-loans-account/create-loans-account.component.ts, src/app/loans/custom-dialog/loans-account-add-collateral-dialog/loans-account-add-collateral-dialog.component.ts, src/app/loans/edit-loans-account/edit-loans-account.component.ts, src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts, src/app/loans/glim-account/glim-account.component.ts |
Add DestroyRef injection and convert all route data and form control valueChanges subscriptions from direct .subscribe() to .pipe(takeUntilDestroyed(this.destroyRef)).subscribe() for automatic cleanup on component destruction. |
Form stepper and validation components 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 |
Replace OnDestroy lifecycle and _onDestroy subject with DestroyRef + takeUntilDestroyed for managing multiple form control valueChanges subscriptions in validation and term-setting methods; remove ngOnDestroy() and _onDestroy field. |
Loans view components and information tabs src/app/loans/loans-view/account-details/account-details.component.ts, src/app/loans/loans-view/charges-tab/charges-tab.component.ts, src/app/loans/loans-view/datatable-tab/datatable-tab.component.ts, src/app/loans/loans-view/external-asset-owner-tab/external-asset-owner-tab.component.ts, src/app/loans/loans-view/floating-interest-rates/floating-interest-rates.component.ts, src/app/loans/loans-view/general-tab/general-tab.component.ts, src/app/loans/loans-view/loan-collateral-tab/loan-collateral-tab.component.ts, src/app/loans/loans-view/loan-deferred-income-tab/loan-deferred-income-tab.component.ts, src/app/loans/loans-view/loan-delinquency-tags-tab/loan-delinquency-tags-tab.component.ts, src/app/loans/loans-view/loan-documents-tab/loan-documents-tab.component.ts, src/app/loans/loans-view/loan-originators-tab/loan-originators-tab.component.ts, src/app/loans/loans-view/loan-term-variations-tab/loan-term-variations-tab.component.ts, src/app/loans/loans-view/loan-tranche-details/loan-tranche-details.component.ts, src/app/loans/loans-view/loans-view.component.ts, src/app/loans/loans-view/notes-tab/notes-tab.component.ts, src/app/loans/loans-view/original-schedule-tab/original-schedule-tab.component.ts, src/app/loans/loans-view/overdue-charges-tab/overdue-charges-tab.component.ts, src/app/loans/loans-view/reschedule-loan-tab/reschedule-loan-tab.component.ts, src/app/loans/loans-view/standing-instructions-tab/standing-instructions-tab.component.ts |
Add DestroyRef injection and pipe all route data subscriptions (both route.data and route.parent.data) through takeUntilDestroyed(this.destroyRef) to bind subscription lifetime to component destruction. |
Loan action and transaction components src/app/loans/loans-view/loan-account-actions/add-loan-charge/add-loan-charge.component.ts, src/app/loans/loans-view/loan-account-actions/adjust-loan-charge/adjust-loan-charge.component.ts, src/app/loans/loans-view/loan-account-actions/approve-loan/approve-loan.component.ts, src/app/loans/loans-view/loan-account-actions/create-guarantor/create-guarantor.component.ts, src/app/loans/loans-view/loan-account-actions/foreclosure/foreclosure.component.ts, src/app/loans/loans-view/loan-account-actions/loan-account-actions.component.ts, src/app/loans/loans-view/loan-account-actions/prepay-loan/prepay-loan.component.ts, src/app/loans/loans-view/transactions-tab/transactions-tab.component.ts, src/app/loans/loans-view/transactions/edit-transaction/edit-transaction.component.ts, src/app/loans/loans-view/transactions/export-transactions/export-transactions.component.ts, src/app/loans/loans-view/transactions/view-reciept/view-reciept.component.ts, src/app/loans/loans-view/transactions/view-transaction/view-transaction.component.ts, src/app/loans/loans-view/view-charge/view-charge.component.ts |
Inject DestroyRef and apply takeUntilDestroyed to both route data subscriptions and reactive form valueChanges subscriptions across charge handling, guarantor creation, transaction views, and prepayment actions. |
Dashboard and working-capital components src/app/loans/loans-view/loan-account-dashboard/loan-account-dashboard.component.ts, src/app/loans/loans-view/working-capital/loan-balances-tab/loan-balances-tab.component.ts, src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts |
Replace manual Subscription tracking and ngOnDestroy unsubscribe calls with DestroyRef + takeUntilDestroyed, removing import of rxjs Subscription class. |
Make Repayment form typing and template updates
| Layer / File(s) | Summary |
|---|---|
RepaymentForm typed model src/app/loans/models/loan-form.model.ts |
Add RepaymentForm interface defining typed FormControl fields: required fields (transactionDate, externalId, paymentTypeId, note, skipInterestRefund, transactionAmount), optional fields (classificationId, accountNumber, checkNumber, routingCode, receiptNumber, bankNumber). |
Make repayment component form typing and methods src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.ts |
Update repaymentLoanForm from UntypedFormGroup to FormGroup<RepaymentForm> | null; switch formBuilder injection to typed FormBuilder; build form using typed FormControl instances with conditional classificationId field; remove public methods for penalty waiving, selection, and display (toggleWaivePenalties, toggleSelectAllPenalties, togglePenaltySelection, isPenaltySelected, getPenaltyDisplayKey, recalculateTransactionAmount); add null-safety checks in updateTransactionAmountValidators and submit. |
Make repayment template and resolver routing src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.html, src/app/loans/common-resolvers/loan-action-button.resolver.ts |
Replace penalties waiver toggle/list UI with showPaymentDetails toggle that conditionally renders banking/payment fields (account number, cheque/check number, routing code, receipt number, bank number); remove skip-interest-refund checkbox; update LoanActionButtonResolver.resolve to route 'Make Repayment' action to loansService.getLoanActionTemplate for loan products or loansService.getWorkingCapitalLoanActionTemplate otherwise. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
- openMF/web-app#3095: Modifies
LoanActionButtonResolver.resolvefor different action branches; this PR changes the'Make Repayment'routing logic. - openMF/web-app#3380: Also modifies
LoanActionButtonResolveraction-branch logic (for'View Guarantors'action).
Suggested reviewers
- IOhacker
- adamsaghy
- shubhamkumar9199
🚥 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 title 'WEB-954: Migrate Subscription management : loans' directly corresponds to the PR's main objective of migrating subscription management patterns across 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
Comment on lines
+93
to
+99
| this.addCollateralForm.controls.quantity.valueChanges | ||
| .pipe(takeUntilDestroyed(this.destroyRef)) | ||
| .subscribe((quantity: any) => { | ||
| this.addCollateralForm.patchValue({ | ||
| totalValue: this.collateralData.basePrice * quantity, | ||
| totalCollateralValue: (this.collateralData.basePrice * this.collateralData.pctToBase * quantity) / 100 | ||
| }); |
There was a problem hiding this comment.
Guard quantity calculations when no collateral is selected.
At Line 97 and Line 98, this.collateralData is dereferenced without a guard. Entering quantity first can throw at runtime.
Proposed fix
this.addCollateralForm.controls.quantity.valueChanges
.pipe(takeUntilDestroyed(this.destroyRef))
.subscribe((quantity: any) => {
+ if (!this.collateralData || quantity === null || quantity === '') {
+ this.addCollateralForm.patchValue(
+ { totalValue: '', totalCollateralValue: '' },
+ { emitEvent: false }
+ );
+ return;
+ }
+
+ const basePrice = Number(this.collateralData.basePrice) || 0;
+ const pctToBase = Number(this.collateralData.pctToBase) || 0;
+ const qty = Number(quantity) || 0;
this.addCollateralForm.patchValue({
- totalValue: this.collateralData.basePrice * quantity,
- totalCollateralValue: (this.collateralData.basePrice * this.collateralData.pctToBase * quantity) / 100
+ totalValue: basePrice * qty,
+ totalCollateralValue: (basePrice * pctToBase * qty) / 100
});
});🤖 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/custom-dialog/loans-account-add-collateral-dialog/loans-account-add-collateral-dialog.component.ts`
around lines 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.
| this.loanId = this.route.parent?.snapshot.paramMap.get('loanId') || ''; | ||
|
|
||
| this.routeDataSubscription = this.route.parent!.data.subscribe((data: { loanDetailsData: any }) => { | ||
| this.route.parent!.data.pipe(takeUntilDestroyed(this.destroyRef)).subscribe((data: { loanDetailsData: any }) => { |
There was a problem hiding this comment.
Non-null assertion on route.parent may cause runtime error.
Line 74 uses this.route.parent!.data with a non-null assertion, but line 72 uses optional chaining (this.route.parent?.snapshot), suggesting parent may be nullable. If parent is null, this will throw a runtime error.
🛡️ Proposed fix to add null safety
- this.route.parent!.data.pipe(takeUntilDestroyed(this.destroyRef)).subscribe((data: { loanDetailsData: any }) => {
+ this.route.parent?.data.pipe(takeUntilDestroyed(this.destroyRef)).subscribe((data: { loanDetailsData: any }) => {
if (data.loanDetailsData) {🤖 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 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.
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
1 participant
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.
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
New Features
Bug Fixes
UI/UX Updates