WEB-954: Migrate Subscription management : small modules - #3629
Merged
alberto-art3ch merged 1 commit intoJun 3, 2026
Merged
Conversation
Replace manual subscribe/unsubscribe patterns with DestroyRef + takeUntilDestroyed across 43 components and directives. Also replace UntypedFormBuilder/Group/Control/Array with typed Angular equivalents. Modules covered: settings, search, navigation, notifications, collaterals, collections, remittances, templates, login, reports, users, home/dashboard, account-transfers, tasks, core/shell (breadcrumb, shell, toolbar) and configuration-wizard popover directive. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Shell and core components subscription lifecycle src/app/core/shell/breadcrumb/breadcrumb.component.ts, src/app/core/shell/shell.component.ts, src/app/core/shell/toolbar/toolbar.component.ts |
Core shell components (breadcrumb, shell, toolbar) transition from manual Subject-based teardown and stored Subscriptions to DestroyRef with takeUntilDestroyed, eliminating ngOnDestroy methods and improving subscription cleanup automation. |
Account transfers and financial operations src/app/account-transfers/create-standing-instructions/..., src/app/account-transfers/edit-standing-instructions/..., src/app/account-transfers/list-standing-instructions/..., src/app/account-transfers/list-transactions/..., src/app/account-transfers/make-account-transfers/..., src/app/account-transfers/view-account-transfer/..., src/app/account-transfers/view-standing-instructions/... |
Standing instructions, transactions, and account transfer components update to typed FormBuilder/FormGroup and wrap route.data subscriptions with takeUntilDestroyed for automatic cleanup. |
Collections and collateral management src/app/collections/collection-sheet/..., src/app/collections/individual-collection-sheet/..., src/app/collaterals/edit-collateral/..., src/app/collaterals/view-collateral/... |
Collection sheet components migrated from manual Subject-based takeUntil to DestroyRef/takeUntilDestroyed with typed form controls; collateral components updated to typed forms and lifecycle management. |
Dashboard and analytics components src/app/home/dashboard/amount-collected-pie/..., src/app/home/dashboard/amount-disbursed-pie/..., src/app/home/dashboard/client-trends-bar/..., src/app/home/dashboard/dashboard.component.ts, src/app/home/home.component.ts |
Dashboard and pie/bar chart analytics components update filter controls to typed FormControl and wrap route.data and form valueChanges subscriptions with takeUntilDestroyed. |
Authentication and user management src/app/login/login.component.ts, src/app/login/reset-password/..., src/app/login/two-factor-authentication/..., src/app/users/create-user/..., src/app/users/edit-user/..., src/app/users/users.component.ts, src/app/users/view-user/... |
Login, password reset, and two-factor components transition to typed forms and takeUntilDestroyed; user management components (create, edit, list, view) updated with typed forms and automatic subscription cleanup. |
Reports, templates, and data management src/app/reports/run-report/..., src/app/templates/create-edit-template/..., src/app/templates/templates.component.ts, src/app/templates/view-template/... |
RunReportComponent updates reportForm and decimalChoice to typed forms with takeUntilDestroyed for all subscriptions; template components adopt typed forms and lifecycle-aware subscription management. |
Task workflows and approval components src/app/tasks/checker-inbox-and-tasks-tabs/checker-inbox/..., src/app/tasks/checker-inbox-and-tasks-tabs/client-approval/..., src/app/tasks/checker-inbox-and-tasks-tabs/council-approval/..., src/app/tasks/checker-inbox-and-tasks-tabs/loan-approval/..., src/app/tasks/checker-inbox-and-tasks-tabs/loan-disbursal/..., src/app/tasks/checker-inbox-and-tasks-tabs/reschedule-loan/..., src/app/tasks/view-checker-inbox/... |
Approval and task components replace manual Subscription management for paginator/route changes with automatic takeUntilDestroyed cleanup; checker inbox updated with typed forms and lifecycle management. |
Navigation, settings, and miscellaneous components src/app/navigation/navigation.component.ts, src/app/settings/settings.component.ts, src/app/notifications/notifications-page/..., src/app/reports/reports.component.ts, src/app/search/search-page/..., src/app/configuration-wizard/popover/popover-arrow.directive.ts |
Navigation updates selector controls to typed FormControl with takeUntilDestroyed for all valueChanges; settings transitions from Subject-based to DestroyRef/takeUntilDestroyed; remaining components (notifications, reports, search) and popover directive adopt automatic subscription lifecycle management. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- openMF/web-app#3045: Modifies the same
src/app/collections/individual-collection-sheet/individual-collection-sheet.component.tsfile, introducing the manualdestroy$/takeUntilpattern that this PR replaces withDestroyRef/takeUntilDestroyed. - openMF/web-app#3420: Updates
src/app/home/dashboard/dashboard.component.tswith route.data-based office loading and new analytics dashboard component, intersecting with this PR's subscription lifecycle and form typing updates on the same file.
Suggested reviewers
- adamsaghy
- IOhacker
🚥 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 : small modules' clearly summarizes the main change: migrating subscription/lifecycle management patterns across multiple components in smaller modules. |
| 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/app/home/dashboard/amount-disbursed-pie/amount-disbursed-pie.component.ts (1)
95-108:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNested subscription lacks automatic cleanup.
The inner
homeService.getDisbursedAmount()subscription at Line 96 is not wrapped withtakeUntilDestroyed. If the component is destroyed while the HTTP call is pending, this subscription may not be cleaned up, potentially causing memory leaks or unexpected behavior.🛡️ Recommended fix: wrap the inner subscription
getChartData() { this.officeId.valueChanges.pipe(takeUntilDestroyed(this.destroyRef)).subscribe((value: number) => { - this.homeService.getDisbursedAmount(value).subscribe((response: any) => { + this.homeService.getDisbursedAmount(value).pipe(takeUntilDestroyed(this.destroyRef)).subscribe((response: any) => { const data = Object.entries(response[0]).map((entry) => entry[1]);🤖 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/home/dashboard/amount-disbursed-pie/amount-disbursed-pie.component.ts` around lines 95 - 108, Replace the nested subscribe by chaining the observables so the HTTP subscription is automatically torn down: in the officeId.valueChanges pipeline (where you currently call homeService.getDisbursedAmount inside the subscribe), use switchMap (or concatMap) to call homeService.getDisbursedAmount(value) and continue the pipe, then apply takeUntilDestroyed(this.destroyRef) once on the outer pipeline and subscribe to the result to call setChart and set showFallback/hideOutput; reference the officeId.valueChanges stream, homeService.getDisbursedAmount, and setChart/showFallback/hideOutput to locate and update the code.src/app/home/dashboard/client-trends-bar/client-trends-bar.component.ts (1)
110-160:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNested subscriptions lack automatic cleanup.
The inner
forkJoin().subscribe()calls at Lines 122, 136, and 150 are not wrapped withtakeUntilDestroyed. If the component is destroyed while these HTTP calls are pending, the subscriptions may not be cleaned up, potentially causing memory leaks or unexpected behavior.🛡️ Recommended fix: wrap the inner subscriptions
case 'Day': const clientsByDay = this.homeService.getClientTrendsByDay(officeId); const loansByDay = this.homeService.getLoanTrendsByDay(officeId); forkJoin([ clientsByDay, loansByDay - ]).subscribe((data: any[]) => { + ]).pipe(takeUntilDestroyed(this.destroyRef)).subscribe((data: any[]) => { const dayLabels = this.getLabels(timescale);Apply the same pattern to the 'Week' and 'Month' cases.
🤖 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/home/dashboard/client-trends-bar/client-trends-bar.component.ts` around lines 110 - 160, The nested forkJoin subscriptions inside the merge(officeId.valueChanges, timescale.valueChanges) switch (in the 'Day'/'Week'/'Month' cases) are not using takeUntilDestroyed and can leak; fix by piping each forkJoin([...]) with .pipe(takeUntilDestroyed(this.destroyRef)) before subscribe so the inner HTTP subscriptions are torn down when the component is destroyed (apply to the forkJoin calls that produce clientsByDay/Week/Month and loansByDay/Week/Month, keeping the existing logic that computes labels with getLabels, counts with getCounts, then calls setChart and sets hideOutput=false).src/app/home/dashboard/amount-collected-pie/amount-collected-pie.component.ts (1)
95-108:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNested subscription lacks automatic cleanup.
The inner
homeService.getCollectedAmount()subscription at Line 96 is not wrapped withtakeUntilDestroyed. If the component is destroyed while the HTTP call is pending, this subscription may not be cleaned up, potentially causing memory leaks or unexpected behavior.🛡️ Recommended fix: wrap the inner subscription
getChartData() { this.officeId.valueChanges.pipe(takeUntilDestroyed(this.destroyRef)).subscribe((value: number) => { - this.homeService.getCollectedAmount(value).subscribe((response: any) => { + this.homeService.getCollectedAmount(value).pipe(takeUntilDestroyed(this.destroyRef)).subscribe((response: any) => { const data = Object.entries(response[0]).map((entry) => entry[1]);🤖 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/home/dashboard/amount-collected-pie/amount-collected-pie.component.ts` around lines 95 - 108, The nested subscription to homeService.getCollectedAmount inside the officeId.valueChanges handler can leak; replace the inner subscribe with an RXJS operator chain: pipe the valueChanges through takeUntilDestroyed(this.destroyRef) and switchMap (or exhaustMap/mergeMap as appropriate) to call this.homeService.getCollectedAmount(value), then subscribe once to handle the response and call setChart/hideOutput/showFallback; ensure the final observable is also terminated with takeUntilDestroyed(this.destroyRef) so the HTTP observable is cleaned up when the component is destroyed (update the officeId.valueChanges subscription to use switchMap(value => this.homeService.getCollectedAmount(value)) and handle response in that single subscription).src/app/account-transfers/list-transactions/list-transactions.component.ts (1)
82-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix paginator wiring timing in ListTransactionsComponent
route.datasubscription runs in the component constructor, but@ViewChild(MatPaginator, { static: true })is only available after construction;this.dataSource.paginator = this.paginator(lines 84-85) can execute whilethis.paginatorisundefined, breaking pagination (the template has a<mat-paginator>and binds[dataSource]="dataSource").- Set the paginator in
ngOnInit/ngAfterViewInit(after theViewChildis set) or keep a singleMatTableDataSourceinstance and only update its.datain the subscription, then assign.paginatoronce after view init.🤖 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/account-transfers/list-transactions/list-transactions.component.ts` around lines 82 - 86, The subscription to route.data in ListTransactionsComponent currently sets this.dataSource.paginator while the `@ViewChild`(MatPaginator) may be undefined; to fix, keep a single MatTableDataSource instance on the component (initialize dataSource = new MatTableDataSource([])) and in the route.data subscription only update this.dataSource.data = data.listTransactionData.transactions.pageItems, then assign this.dataSource.paginator = this.paginator from ngAfterViewInit (or ngOnInit if using { static: true } correctly) so the paginator wiring happens after the ViewChild is set; update the code references to this.dataSource, the route.data subscription, and implement ngAfterViewInit in ListTransactionsComponent accordingly.
🧹 Nitpick comments (8)
src/app/home/dashboard/amount-collected-pie/amount-collected-pie.component.ts (1)
96-96: ⚡ Quick winReplace
anywith proper response type.Based on learnings, avoid using
anyfor API responses. Introduce a specific interface or type for the collected amount response shape to improve type safety.🤖 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/home/dashboard/amount-collected-pie/amount-collected-pie.component.ts` at line 96, Replace the use of `any` for the API response by defining a concrete interface (e.g., CollectedAmountResponse with the actual fields returned like total, currency, breakdown, etc.) and use that type in the subscribe callback in amount-collected-pie.component (change subscribe((response: any) => ...) to subscribe((response: CollectedAmountResponse) => ...)); also update the HomeService method signature getCollectedAmount to return Observable<CollectedAmountResponse> and adjust any mapping logic accordingly, and import the new interface into amount-collected-pie.component.src/app/home/dashboard/client-trends-bar/client-trends-bar.component.ts (1)
122-122: ⚡ Quick winReplace
any[]with proper response types.Based on learnings, avoid using
anyfor API responses. Introduce specific interfaces or types for the trend data response shapes to improve type safety.Also applies to: 136-136, 150-150
🤖 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/home/dashboard/client-trends-bar/client-trends-bar.component.ts` at line 122, The component is using loose any[] types in subscribe callbacks; define concrete interfaces (e.g., TrendPoint, TrendSeries, TrendResponse) that match the API shapes, update the service method return types to Observable<TrendResponse[]> (or Observable<TrendSeries>) and replace the subscribe signatures in ClientTrendsBarComponent (the subscribe callbacks at the lines using (data: any[])) to use the new types (e.g., (data: TrendResponse[])). Ensure any intermediate mappings (in methods like loadTrends / ngOnInit or the service method called by ClientTrendsBarComponent) are updated to return the typed Observable and update variable declarations accordingly so the component no longer uses any[].src/app/home/dashboard/amount-disbursed-pie/amount-disbursed-pie.component.ts (1)
96-96: ⚡ Quick winReplace
anywith proper response type.Based on learnings, avoid using
anyfor API responses. Introduce a specific interface or type for the disbursed amount response shape to improve type safety.🤖 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/home/dashboard/amount-disbursed-pie/amount-disbursed-pie.component.ts` at line 96, The subscribe callback is typed as `any`; define a concrete interface (e.g., DisbursedAmountResponse) that matches the API shape returned by homeService.getDisbursedAmount and replace `(response: any)` with `(response: DisbursedAmountResponse)` in amount-disbursed-pie.component.ts; also update the homeService.getDisbursedAmount method signature to return Observable<DisbursedAmountResponse> (or import the shared model) so the component and service share the same typed contract and improve type safety.src/app/navigation/navigation.component.ts (1)
68-76: 💤 Low valueOptional: tighten selector typing while migrating off
Untyped*.
new FormControl()infersFormControl<any>, so the typed migration doesn't yet yield real type safety here. Where the value shape is known (selectors hold IDs), considernew FormControl<number | null>(null).🤖 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/navigation/navigation.component.ts` around lines 68 - 76, The selector FormControl instances (officeSelector, employeeSelector, centerSelector, groupSelector, clientSelector) are currently untyped; replace their constructors to specify the expected ID type and initial value (e.g. use new FormControl<number | null>(null) or the appropriate ID type instead of new FormControl()) so each control is strongly typed to number|null; update any callers or bindings if they rely on the previous any-type assumptions.src/app/collections/individual-collection-sheet/individual-collection-sheet.component.ts (1)
208-222: ⚡ Quick winRedundant
takeUntilDestroyedin repeatedly-invokedbuildDependencies().
buildDependencies$is already emitted here (Line 210) to tear down the prior stream and iscomplete()d inngOnDestroy, sotakeUntil(this.buildDependencies$)covers both re-subscription and destroy. The addedtakeUntilDestroyed(this.destroyRef)is redundant, and sincebuildDependencies()is also called fromrefreshData(), each reload registers anotherdestroyRef.onDestroyteardown that only fires at destruction — a slow accumulation across reloads. Drop thetakeUntilDestroyedline here.♻️ Proposed change
.valueChanges.pipe( takeUntil(this.buildDependencies$), - takeUntilDestroyed(this.destroyRef), switchMap((value: any) => this.organizationService.getStaffs(value)) )🤖 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/collections/individual-collection-sheet/individual-collection-sheet.component.ts` around lines 208 - 222, The subscription in buildDependencies() uses both takeUntil(this.buildDependencies$) and takeUntilDestroyed(this.destroyRef), which is redundant and causes accumulation of destroyRef teardowns across repeated calls (e.g., from refreshData()); remove the takeUntilDestroyed(this.destroyRef) operator from the valueChanges pipeline so the stream is only torn down by this.buildDependencies$ when re-subscribed and by ngOnDestroy via buildDependencies$ completion; specifically update the pipeline created from collectionSheetForm.get('officeId').valueChanges in buildDependencies() to drop takeUntilDestroyed while keeping takeUntil(this.buildDependencies$) and the switchMap to organizationService.getStaffs.src/app/collections/collection-sheet/collection-sheet.component.ts (1)
113-129: ⚡ Quick winAvoid nested subscriptions in
buildDependencies().The three
organizationServicecalls are subscribed inside thevalueChangescallback. These inner subscriptions are not bound todestroyRef, so in-flight requests aren't torn down on destroy, and rapidofficeIdchanges can race (a stale response can overwrite fresh data). Flatten withswitchMap+forkJoinso cancellation and ordering follow the outer stream.♻️ Proposed refactor
+import { forkJoin } from 'rxjs'; +import { switchMap, tap } from 'rxjs/operators';buildDependencies() { this.collectionSheetForm .get('officeId') - .valueChanges.pipe(takeUntilDestroyed(this.destroyRef)) - .subscribe((officeId: any) => { - this.officeId = officeId; - this.organizationService.getStaffs(officeId).subscribe((response: any) => { - this.loanOfficerData = response; - }); - this.organizationService.getCenters(officeId).subscribe((response: any) => { - this.centersData = response; - }); - this.organizationService.getGroups(officeId).subscribe((response: any) => { - this.groupsData = response; - }); - }); + .valueChanges.pipe( + tap((officeId: any) => (this.officeId = officeId)), + switchMap((officeId: any) => + forkJoin({ + staffs: this.organizationService.getStaffs(officeId), + centers: this.organizationService.getCenters(officeId), + groups: this.organizationService.getGroups(officeId) + }) + ), + takeUntilDestroyed(this.destroyRef) + ) + .subscribe(({ staffs, centers, groups }) => { + this.loanOfficerData = staffs; + this.centersData = centers; + this.groupsData = groups; + }); }As per coding guidelines for
src/app/**: "clean observable patterns".🤖 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/collections/collection-sheet/collection-sheet.component.ts` around lines 113 - 129, buildDependencies() currently nests three organizationService subscriptions inside the collectionSheetForm.get('officeId').valueChanges stream which prevents cancellation via destroyRef and allows stale responses to overwrite newer data; refactor to flatten the stream by replacing the inner subscriptions with a switchMap from the officeId value to a forkJoin of organizationService.getStaffs(officeId), getCenters(officeId) and getGroups(officeId), then subscribe once (or use tap) and assign loanOfficerData, centersData and groupsData from the forkJoin result, preserving takeUntilDestroyed(this.destroyRef) on the outer stream so in-flight requests are cancelled on destroy and ordering follows the latest officeId.src/app/login/login.component.ts (1)
150-150: ⚡ Quick winType the version service API response.
The parameter uses
anywhen aVersionInfointerface is already defined (lines 17-23). Based on learnings, API responses should use specific interfaces instead ofany.💡 Suggested type improvement
- .subscribe( - (info: any) => { + .subscribe( + (info: VersionInfo) => {Note: The existing VersionInfo interface may need to be extended to cover all properties accessed (mifosX, mifos_x, version, fineractX, fineract_x, git.build.version).
🤖 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/login/login.component.ts` at line 150, The callback parameter currently typed as "any" should be replaced with the specific VersionInfo interface: change the anonymous callback signature from "(info: any) => { ... }" to use "(info: VersionInfo) => { ... }" in LoginComponent where the version service response is handled; if additional properties are accessed (mifosX, mifos_x, version, fineractX, fineract_x, git.build.version) extend the existing VersionInfo interface (lines 17-23) to include those fields so the compiler can type-check usage.src/app/login/two-factor-authentication/two-factor-authentication.component.ts (1)
49-49: ⚡ Quick winType the two-factor authentication API responses.
Multiple properties and callback parameters use
any(delivery methods at lines 49, 51; responses at lines 72, 97). Based on learnings, introduce specific interfaces for these API response shapes instead ofany.💡 Example interface definitions
interface TwoFactorDeliveryMethod { id: number; name: string; // add other fields based on actual API response } interface OTPRequestResponse { tokenLiveTimeInSec: number; // add other fields }Also applies to: 51-51, 72-72, 97-97
🤖 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/login/two-factor-authentication/two-factor-authentication.component.ts` at line 49, Replace the untyped uses of any with concrete interfaces: introduce TwoFactorDeliveryMethod (e.g., id:number, name:string, etc.) and OTPRequestResponse/OTPVerifyResponse (e.g., tokenLiveTimeInSec:number and other fields) and use them to type the twoFactorAuthenticationDeliveryMethods property and the API response/callback parameters referenced at lines 51, 72, and 97 (update the property twoFactorAuthenticationDeliveryMethods and the relevant request/response handlers such as the OTP request and verify functions to return/accept these interfaces 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.
Outside diff comments:
In `@src/app/account-transfers/list-transactions/list-transactions.component.ts`:
- Around line 82-86: The subscription to route.data in ListTransactionsComponent
currently sets this.dataSource.paginator while the `@ViewChild`(MatPaginator) may
be undefined; to fix, keep a single MatTableDataSource instance on the component
(initialize dataSource = new MatTableDataSource([])) and in the route.data
subscription only update this.dataSource.data =
data.listTransactionData.transactions.pageItems, then assign
this.dataSource.paginator = this.paginator from ngAfterViewInit (or ngOnInit if
using { static: true } correctly) so the paginator wiring happens after the
ViewChild is set; update the code references to this.dataSource, the route.data
subscription, and implement ngAfterViewInit in ListTransactionsComponent
accordingly.
In
`@src/app/home/dashboard/amount-collected-pie/amount-collected-pie.component.ts`:
- Around line 95-108: The nested subscription to homeService.getCollectedAmount
inside the officeId.valueChanges handler can leak; replace the inner subscribe
with an RXJS operator chain: pipe the valueChanges through
takeUntilDestroyed(this.destroyRef) and switchMap (or exhaustMap/mergeMap as
appropriate) to call this.homeService.getCollectedAmount(value), then subscribe
once to handle the response and call setChart/hideOutput/showFallback; ensure
the final observable is also terminated with takeUntilDestroyed(this.destroyRef)
so the HTTP observable is cleaned up when the component is destroyed (update the
officeId.valueChanges subscription to use switchMap(value =>
this.homeService.getCollectedAmount(value)) and handle response in that single
subscription).
In
`@src/app/home/dashboard/amount-disbursed-pie/amount-disbursed-pie.component.ts`:
- Around line 95-108: Replace the nested subscribe by chaining the observables
so the HTTP subscription is automatically torn down: in the
officeId.valueChanges pipeline (where you currently call
homeService.getDisbursedAmount inside the subscribe), use switchMap (or
concatMap) to call homeService.getDisbursedAmount(value) and continue the pipe,
then apply takeUntilDestroyed(this.destroyRef) once on the outer pipeline and
subscribe to the result to call setChart and set showFallback/hideOutput;
reference the officeId.valueChanges stream, homeService.getDisbursedAmount, and
setChart/showFallback/hideOutput to locate and update the code.
In `@src/app/home/dashboard/client-trends-bar/client-trends-bar.component.ts`:
- Around line 110-160: The nested forkJoin subscriptions inside the
merge(officeId.valueChanges, timescale.valueChanges) switch (in the
'Day'/'Week'/'Month' cases) are not using takeUntilDestroyed and can leak; fix
by piping each forkJoin([...]) with .pipe(takeUntilDestroyed(this.destroyRef))
before subscribe so the inner HTTP subscriptions are torn down when the
component is destroyed (apply to the forkJoin calls that produce
clientsByDay/Week/Month and loansByDay/Week/Month, keeping the existing logic
that computes labels with getLabels, counts with getCounts, then calls setChart
and sets hideOutput=false).
---
Nitpick comments:
In `@src/app/collections/collection-sheet/collection-sheet.component.ts`:
- Around line 113-129: buildDependencies() currently nests three
organizationService subscriptions inside the
collectionSheetForm.get('officeId').valueChanges stream which prevents
cancellation via destroyRef and allows stale responses to overwrite newer data;
refactor to flatten the stream by replacing the inner subscriptions with a
switchMap from the officeId value to a forkJoin of
organizationService.getStaffs(officeId), getCenters(officeId) and
getGroups(officeId), then subscribe once (or use tap) and assign
loanOfficerData, centersData and groupsData from the forkJoin result, preserving
takeUntilDestroyed(this.destroyRef) on the outer stream so in-flight requests
are cancelled on destroy and ordering follows the latest officeId.
In
`@src/app/collections/individual-collection-sheet/individual-collection-sheet.component.ts`:
- Around line 208-222: The subscription in buildDependencies() uses both
takeUntil(this.buildDependencies$) and takeUntilDestroyed(this.destroyRef),
which is redundant and causes accumulation of destroyRef teardowns across
repeated calls (e.g., from refreshData()); remove the
takeUntilDestroyed(this.destroyRef) operator from the valueChanges pipeline so
the stream is only torn down by this.buildDependencies$ when re-subscribed and
by ngOnDestroy via buildDependencies$ completion; specifically update the
pipeline created from collectionSheetForm.get('officeId').valueChanges in
buildDependencies() to drop takeUntilDestroyed while keeping
takeUntil(this.buildDependencies$) and the switchMap to
organizationService.getStaffs.
In
`@src/app/home/dashboard/amount-collected-pie/amount-collected-pie.component.ts`:
- Line 96: Replace the use of `any` for the API response by defining a concrete
interface (e.g., CollectedAmountResponse with the actual fields returned like
total, currency, breakdown, etc.) and use that type in the subscribe callback in
amount-collected-pie.component (change subscribe((response: any) => ...) to
subscribe((response: CollectedAmountResponse) => ...)); also update the
HomeService method signature getCollectedAmount to return
Observable<CollectedAmountResponse> and adjust any mapping logic accordingly,
and import the new interface into amount-collected-pie.component.
In
`@src/app/home/dashboard/amount-disbursed-pie/amount-disbursed-pie.component.ts`:
- Line 96: The subscribe callback is typed as `any`; define a concrete interface
(e.g., DisbursedAmountResponse) that matches the API shape returned by
homeService.getDisbursedAmount and replace `(response: any)` with `(response:
DisbursedAmountResponse)` in amount-disbursed-pie.component.ts; also update the
homeService.getDisbursedAmount method signature to return
Observable<DisbursedAmountResponse> (or import the shared model) so the
component and service share the same typed contract and improve type safety.
In `@src/app/home/dashboard/client-trends-bar/client-trends-bar.component.ts`:
- Line 122: The component is using loose any[] types in subscribe callbacks;
define concrete interfaces (e.g., TrendPoint, TrendSeries, TrendResponse) that
match the API shapes, update the service method return types to
Observable<TrendResponse[]> (or Observable<TrendSeries>) and replace the
subscribe signatures in ClientTrendsBarComponent (the subscribe callbacks at the
lines using (data: any[])) to use the new types (e.g., (data: TrendResponse[])).
Ensure any intermediate mappings (in methods like loadTrends / ngOnInit or the
service method called by ClientTrendsBarComponent) are updated to return the
typed Observable and update variable declarations accordingly so the component
no longer uses any[].
In `@src/app/login/login.component.ts`:
- Line 150: The callback parameter currently typed as "any" should be replaced
with the specific VersionInfo interface: change the anonymous callback signature
from "(info: any) => { ... }" to use "(info: VersionInfo) => { ... }" in
LoginComponent where the version service response is handled; if additional
properties are accessed (mifosX, mifos_x, version, fineractX, fineract_x,
git.build.version) extend the existing VersionInfo interface (lines 17-23) to
include those fields so the compiler can type-check usage.
In
`@src/app/login/two-factor-authentication/two-factor-authentication.component.ts`:
- Line 49: Replace the untyped uses of any with concrete interfaces: introduce
TwoFactorDeliveryMethod (e.g., id:number, name:string, etc.) and
OTPRequestResponse/OTPVerifyResponse (e.g., tokenLiveTimeInSec:number and other
fields) and use them to type the twoFactorAuthenticationDeliveryMethods property
and the API response/callback parameters referenced at lines 51, 72, and 97
(update the property twoFactorAuthenticationDeliveryMethods and the relevant
request/response handlers such as the OTP request and verify functions to
return/accept these interfaces instead of any).
In `@src/app/navigation/navigation.component.ts`:
- Around line 68-76: The selector FormControl instances (officeSelector,
employeeSelector, centerSelector, groupSelector, clientSelector) are currently
untyped; replace their constructors to specify the expected ID type and initial
value (e.g. use new FormControl<number | null>(null) or the appropriate ID type
instead of new FormControl()) so each control is strongly typed to number|null;
update any callers or bindings if they rely on the previous any-type
assumptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6a4fd78-7c43-4e24-9163-8793826241af
📒 Files selected for processing (43)
src/app/account-transfers/create-standing-instructions/create-standing-instructions.component.tssrc/app/account-transfers/edit-standing-instructions/edit-standing-instructions.component.tssrc/app/account-transfers/list-standing-instructions/list-standing-instructions.component.tssrc/app/account-transfers/list-transactions/list-transactions.component.tssrc/app/account-transfers/make-account-transfers/make-account-transfers.component.tssrc/app/account-transfers/view-account-transfer/view-account-transfer.component.tssrc/app/account-transfers/view-standing-instructions/view-standing-instructions.component.tssrc/app/collaterals/edit-collateral/edit-collateral.component.tssrc/app/collaterals/view-collateral/view-collateral.component.tssrc/app/collections/collection-sheet/collection-sheet.component.tssrc/app/collections/individual-collection-sheet/individual-collection-sheet.component.tssrc/app/configuration-wizard/popover/popover-arrow.directive.tssrc/app/core/shell/breadcrumb/breadcrumb.component.tssrc/app/core/shell/shell.component.tssrc/app/core/shell/toolbar/toolbar.component.tssrc/app/home/dashboard/amount-collected-pie/amount-collected-pie.component.tssrc/app/home/dashboard/amount-disbursed-pie/amount-disbursed-pie.component.tssrc/app/home/dashboard/client-trends-bar/client-trends-bar.component.tssrc/app/home/dashboard/dashboard.component.tssrc/app/home/home.component.tssrc/app/login/login.component.tssrc/app/login/reset-password/reset-password.component.tssrc/app/login/two-factor-authentication/two-factor-authentication.component.tssrc/app/navigation/navigation.component.tssrc/app/notifications/notifications-page/notifications-page.component.tssrc/app/reports/reports.component.tssrc/app/reports/run-report/run-report.component.tssrc/app/search/search-page/search-page.component.tssrc/app/settings/settings.component.tssrc/app/tasks/checker-inbox-and-tasks-tabs/checker-inbox/checker-inbox.component.tssrc/app/tasks/checker-inbox-and-tasks-tabs/client-approval/client-approval.component.tssrc/app/tasks/checker-inbox-and-tasks-tabs/council-approval/council-approval.component.tssrc/app/tasks/checker-inbox-and-tasks-tabs/loan-approval/loan-approval.component.tssrc/app/tasks/checker-inbox-and-tasks-tabs/loan-disbursal/loan-disbursal.component.tssrc/app/tasks/checker-inbox-and-tasks-tabs/reschedule-loan/reschedule-loan.component.tssrc/app/tasks/view-checker-inbox/view-checker-inbox.component.tssrc/app/templates/create-edit-template/create-edit-template.component.tssrc/app/templates/templates.component.tssrc/app/templates/view-template/view-template.component.tssrc/app/users/create-user/create-user.component.tssrc/app/users/edit-user/edit-user.component.tssrc/app/users/users.component.tssrc/app/users/view-user/view-user.component.ts
This was referenced Jun 5, 2026
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
2 participants
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.
Description
Replace manual subscribe/unsubscribe patterns with DestroyRef + takeUntilDestroyed across 43 components and directives. Also replace UntypedFormBuilder/Group/Control/Array with typed Angular equivalents.
Modules covered: settings, search, navigation, notifications, collaterals, collections, remittances, templates, login, reports, users, home/dashboard, account-transfers, tasks, core/shell (breadcrumb, shell, toolbar) and configuration-wizard popover directive.
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