WEB-624 refactor: remove hard-coded back-routing for component reload - #3045
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Collection & reload integrationsrc/app/collections/individual-collection-sheet/individual-collection-sheet.component.ts |
Added OnDestroy, destroy$ and buildDependencies$ Subjects, reloadContext, injected DataReloadService, moved init to ngOnInit, added ngOnDestroy, reload() and refreshData(); route and reload subscriptions use takeUntil and switchMap; dialog made private. |
Groups view lifecycle & reloadsrc/app/groups/groups-view/groups-view.component.ts |
Now implements OnInit, OnDestroy; added destroy$, reloadContext, injected DataReloadService; introduced ngOnInit/ngOnDestroy, reload() and refreshData(); route and reload subscriptions use takeUntil; refactored navigation queryParams and dialog handlers. |
SMS campaign view reloadsrc/app/organization/sms-campaigns/view-campaign/view-campaign.component.ts |
Now implements OnInit, OnDestroy; added destroy$, reloadContext, injected DataReloadService; ngOnInit subscribes to route data and reload observable with takeUntil; added reload() and refreshData() fetching via OrganizationService; MatDialog made private. |
Sequence Diagram(s)
sequenceDiagram
participant UI as Client/UI
participant C as Component
participant D as DataReloadService
participant S as BackendService
UI->>C: navigate / perform action
C->>D: getReloadObservable(reloadContext)
D-->>C: emits reload event
C->>C: refreshData()
C->>S: fetch latest data (service)
S-->>C: return data
C->>UI: update view
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~35 minutes
Possibly related PRs
- WEB-623 refactor: remove hard-coded back-routing for component reload #3044: Implements the same DataReloadService pattern (OnDestroy, destroy$, reloadContext, reload/refreshData) across multiple components.
Suggested reviewers
- alberto-art3ch
- IOhacker
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately describes the main objective of the changeset: refactoring component reload behavior to remove hard-coded back-routing in favor of a shared reload service, which is the central change across all three modified components. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/collections/individual-collection-sheet/individual-collection-sheet.component.ts (1)
138-140: Incorrect@ViewChildoption:read: trueis invalid.The
readoption expects a DI token/type (e.g.,ElementRef,TemplateRef) to specify what to read from the queried element. Usingtrueis incorrect. Since the property types are already specified in the declarations, simply remove the option.🐛 Proposed fix
- `@ViewChild`(MatPaginator, { read: true }) paginator: MatPaginator; - `@ViewChild`(MatSort, { read: true }) sort: MatSort; + `@ViewChild`(MatPaginator) paginator: MatPaginator; + `@ViewChild`(MatSort) sort: MatSort;
🤖 Fix all issues with AI agents
In
`@src/app/collections/individual-collection-sheet/individual-collection-sheet.component.ts`:
- Around line 206-214: The inner HTTP subscription to
organizationService.getStaffs created on each
collectionSheetForm.get('officeId').valueChanges must be replaced with a
flattened stream to avoid leaked inner subscriptions: pipe the valueChanges
through takeUntil(this.destroy$) and switchMap(value =>
this.organizationService.getStaffs(value)) and then subscribe once to set
this.loanOfficerData; ensure switchMap is imported from rxjs/operators and
remove the nested subscribe so the HTTP observable is cancelled when the
component is destroyed.
🧹 Nitpick comments (6)
src/app/organization/sms-campaigns/view-campaign/view-campaign.component.ts (2)
129-141: Potential duplicate subscriptions from nested observable pattern.Each time
route.dataemits, a new subscription togetReloadObservableis created without unsubscribing from the previous one. AlthoughtakeUntil(this.destroy$)will eventually clean up all subscriptions on destroy, during the component's lifetime multiple reload handlers could accumulate if route data changes.Consider using
switchMapto flatten the subscription:♻️ Suggested refactor using switchMap
+import { switchMap } from 'rxjs/operators'; + ngOnInit(): void { - this.route.data.pipe(takeUntil(this.destroy$)).subscribe((data: { smsCampaign: any }) => { - this.smsCampaignData = data.smsCampaign; - this.reloadContext = `sms-campaign-${this.smsCampaignData.id}`; - - // Subscribe to reload events after we have the campaign ID - this.dataReloadService - .getReloadObservable(this.reloadContext) - .pipe(takeUntil(this.destroy$)) - .subscribe(() => { - this.refreshData(); - }); - }); + this.route.data.pipe( + takeUntil(this.destroy$), + switchMap((data: { smsCampaign: any }) => { + this.smsCampaignData = data.smsCampaign; + this.reloadContext = `sms-campaign-${this.smsCampaignData.id}`; + return this.dataReloadService.getReloadObservable(this.reloadContext); + }) + ).subscribe(() => { + this.refreshData(); + }); this.maxDate = this.settingsService.businessDate; this.createSMSForm(); }
199-214: Consider addingtakeUntilto dialog subscriptions for safety.While
afterClosed()completes after one emission, if the component is destroyed before the dialog closes, the callback at Line 210-212 could still execute, potentially causing issues. This applies to all dialog subscriptions in this file (closeCampaign,activateCampaign,reactivateCampaign).♻️ Example fix for closeCampaign
- closeCampaignDialogRef.afterClosed().subscribe((response: any) => { + closeCampaignDialogRef.afterClosed().pipe(takeUntil(this.destroy$)).subscribe((response: any) => {src/app/groups/groups-view/groups-view.component.ts (2)
86-99: Same nested subscription pattern as ViewCampaignComponent.This has the same issue where each
route.dataemission creates a new subscription to the reload observable. Consider usingswitchMapto flatten the subscriptions, as suggested forViewCampaignComponent.
167-169: Inconsistent visibility:reload()is public here but private in other components.In
ViewCampaignComponent(Line 310),reload()is declared asprivate, but here it's public. If this method is intended to be called externally (e.g., from child components or templates), keeping it public is fine. Otherwise, consider aligning visibility across components for consistency.src/app/collections/individual-collection-sheet/individual-collection-sheet.component.ts (2)
445-461:refreshData()doesn't resetloansDataSourceandsavingsDataSource.The method clears
loansDataandsavingsDataarrays but doesn't reset theMatTableDataSourceinstances. If the user had previously loaded data, the tables might display stale data until a new search is performed.♻️ Suggested addition
private refreshData(): void { // Re-create the form and rebuild dependencies this.isCollapsed = false; this.collectionSheetData = null; // Clear bulk transaction arrays to prevent stale data this.bulkRepaymentTransactions = []; this.bulkSavingsDueTransactions = []; this.bulkDisbursementTransactionsData = {}; // Clear table data arrays this.loansData = []; this.savingsData = []; + + // Reset data sources to clear table display + this.loansDataSource = null; + this.savingsDataSource = null; this.createCollectionSheetForm(); this.buildDependencies(); }
177-183: Redundant conditional check forreloadContext.Since
reloadContextis initialized to a constant non-empty string ('individual-collection-sheet') at Line 97, theif (this.reloadContext)check at Line 180 will always be true. This differs from the other components wherereloadContextis dynamically set and could be undefined.♻️ Simplify cleanup
ngOnDestroy(): void { this.destroy$.next(); this.destroy$.complete(); - if (this.reloadContext) { - this.dataReloadService.cleanup(this.reloadContext); - } + this.dataReloadService.cleanup(this.reloadContext); }
53d6605 to
382b0a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/app/collections/individual-collection-sheet/individual-collection-sheet.component.ts`:
- Around line 437-462: The refreshData() -> buildDependencies() flow creates a
new valueChanges subscription for officeId each rebuild without unsubscribing
the previous one; modify the component to keep a Subscription property (e.g.,
officeIdSub) and before creating the new subscription in buildDependencies()
unsubscribe the existing officeIdSub (and set it to the new Subscription), and
also ensure you unsubscribe officeIdSub in ngOnDestroy; update
buildDependencies() to assign the subscription returned by the
form.controls.officeId.valueChanges.subscribe(...) to that property and call
officeIdSub?.unsubscribe() before reassigning.
🧹 Nitpick comments (1)
src/app/groups/groups-view/groups-view.component.ts (1)
165-181: Verify whether datatables should refresh on reload.
refreshData()updates onlygroupViewData. IfgroupDatatablescan change as a result of actions, consider re-fetching them on reload or confirm they’re static.
382b0a2 to
475d190
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/app/organization/sms-campaigns/view-campaign/view-campaign.component.ts (4)
199-214: Dialog response should use optional chaining to prevent potential runtime errors.The
afterClosedsubscription accessesresponse.datawithout first checking ifresponseis defined. If the dialog is dismissed without a response, this could cause a runtime error.🐛 Proposed fix
closeCampaignDialogRef.afterClosed().subscribe((response: any) => { - if (response.data) { + if (response?.data) {
236-251: Same optional chaining issue for activateCampaign dialog response.🐛 Proposed fix
activateCampaignDialogRef.afterClosed().subscribe((response: any) => { - if (response.data) { + if (response?.data) {
273-288: Same optional chaining issue for reactivateCampaign dialog response.🐛 Proposed fix
reactivateCampaignDialogRef.afterClosed().subscribe((response: any) => { - if (response.data) { + if (response?.data) {
326-352: MissingtakeUntilon search() HTTP subscription.The
getMessagebyStatussubscription is not guarded bytakeUntil(this.destroy$). If the component is destroyed while this request is in flight, the subscription will continue and attempt to update component state.🐛 Proposed fix
- this.organizationService.getMessagebyStatus(data).subscribe((response: any) => { - this.dataSource.data = response.pageItems; - this.messageTableRef.renderRows(); - }); + this.organizationService.getMessagebyStatus(data) + .pipe(takeUntil(this.destroy$)) + .subscribe((response: any) => { + this.dataSource.data = response.pageItems; + this.messageTableRef.renderRows(); + });src/app/collections/individual-collection-sheet/individual-collection-sheet.component.ts (3)
408-419: HTTP subscription inpreviewCollectionSheetlackstakeUntil.The
retrieveCollectionSheetDatasubscription is not guarded and could cause issues if the component is destroyed while the request is in flight.🐛 Proposed fix
- this.collectionsService.retrieveCollectionSheetData(collectionSheet).subscribe((response: any) => { + this.collectionsService.retrieveCollectionSheetData(collectionSheet) + .pipe(takeUntil(this.destroy$)) + .subscribe((response: any) => {
437-440: HTTP subscription insubmitlackstakeUntil.The
executeSaveCollectionSheetsubscription should be guarded to prevent state updates after component destruction.🐛 Proposed fix
- this.collectionsService.executeSaveCollectionSheet(finalSubmitData).subscribe(() => { + this.collectionsService.executeSaveCollectionSheet(finalSubmitData) + .pipe(takeUntil(this.destroy$)) + .subscribe(() => {
325-390: DialogafterClosedsubscription should use optional chaining on response.At line 327,
response.datais accessed without checking ifresponseis defined. If the dialog is dismissed (e.g., by clicking outside),responsemay be undefined.🐛 Proposed fix
showPaymentDetailsDialogRef.afterClosed().subscribe((response: any) => { - if (response.data) { + if (response?.data) {
🧹 Nitpick comments (3)
src/app/organization/sms-campaigns/view-campaign/view-campaign.component.ts (1)
129-145: Nested reload subscription creates potential for duplicate subscriptions on route data changes.The reload observable subscription (lines 135-140) is created inside the
route.datasubscription. If route data emits multiple times (e.g., route parameter changes), a new reload subscription is created each time without unsubscribing the previous one.Consider moving the reload subscription outside or using
switchMapto flatten:♻️ Proposed fix using switchMap
ngOnInit(): void { - this.route.data.pipe(takeUntil(this.destroy$)).subscribe((data: { smsCampaign: any }) => { - this.smsCampaignData = data.smsCampaign; - this.reloadContext = `sms-campaign-${this.smsCampaignData.id}`; - - // Subscribe to reload events after we have the campaign ID - this.dataReloadService - .getReloadObservable(this.reloadContext) - .pipe(takeUntil(this.destroy$)) - .subscribe(() => { - this.refreshData(); - }); - }); + this.route.data.pipe( + takeUntil(this.destroy$), + tap((data: { smsCampaign: any }) => { + this.smsCampaignData = data.smsCampaign; + this.reloadContext = `sms-campaign-${this.smsCampaignData.id}`; + }), + switchMap(() => this.dataReloadService.getReloadObservable(this.reloadContext)) + ).subscribe(() => { + this.refreshData(); + }); this.maxDate = this.settingsService.businessDate; this.createSMSForm(); }Add
tapandswitchMapto the imports:-import { takeUntil } from 'rxjs/operators'; +import { takeUntil, tap, switchMap } from 'rxjs/operators';src/app/groups/groups-view/groups-view.component.ts (2)
86-100: Same nested subscription issue as in view-campaign.component.ts.The reload observable subscription is nested inside the
route.datasubscription, which can cause duplicate subscriptions if route data emits multiple times.♻️ Proposed fix using switchMap
+import { takeUntil, tap, switchMap } from 'rxjs/operators'; -import { takeUntil } from 'rxjs/operators';ngOnInit(): void { - this.route.data.pipe(takeUntil(this.destroy$)).subscribe((data: { groupViewData: any; groupDatatables: any }) => { - this.groupViewData = data.groupViewData; - this.groupDatatables = data.groupDatatables; - this.reloadContext = `group-${this.groupViewData.id}`; - - // Subscribe to reload events after we have the group ID - this.dataReloadService - .getReloadObservable(this.reloadContext) - .pipe(takeUntil(this.destroy$)) - .subscribe(() => { - this.refreshData(); - }); - }); + this.route.data.pipe( + takeUntil(this.destroy$), + tap((data: { groupViewData: any; groupDatatables: any }) => { + this.groupViewData = data.groupViewData; + this.groupDatatables = data.groupDatatables; + this.reloadContext = `group-${this.groupViewData.id}`; + }), + switchMap(() => this.dataReloadService.getReloadObservable(this.reloadContext)) + ).subscribe(() => { + this.refreshData(); + }); }
167-169: Inconsistent visibility:reload()is public here but private in other components.In
view-campaign.component.tsandindividual-collection-sheet.component.ts, thereload()method is either private or has different visibility. Consider aligning the access modifier across components for consistency.
2 participants
This PR cleans up the way components reload their data. Earlier, reloading was done by navigating to another route and then coming back using hard-coded paths. This made the code harder to maintain and caused unnecessary routing just to refresh data.
With this change, components reload their data through a shared reload service instead of using route navigation. This keeps routing and data refresh separate, makes the code easier to understand, and provides a consistent approach that can be reused across components.
WEB-624
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
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.