WEB-38: Fix guarantors page data display and update breadcrumb to Loans - #3380
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Breadcrumb URL Adjustment src/app/core/shell/breadcrumb/breadcrumb.component.ts |
Rewrites module-root breadcrumb URLs (e.g., /loans-accounts/) to target a specific entity path derived from the current router URL or disables the URL when no entity segment exists. |
Edit Guarantor Dialog src/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.ts, .../edit-guarantor-dialog.component.html, .../edit-guarantor-dialog.component.scss |
Introduces a new standalone Material dialog component with a reactive form to edit guarantor details and returns form data on submit. |
View Guarantor Details Dialog src/app/loans/custom-dialog/loans-account-view-guarantor-details-dialog/loans-account-view-guarantor-details-dialog.component.ts, ...component.html |
Layout adjustments (row→column), label/value width changes, optional chaining for nested data access, and inclusion of FormatNumberPipe for template usage. |
View & Edit Guarantors Workflow src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.ts, .../view-guarantors.component.html, src/app/loans/loans-view/loan-account-actions/create-guarantor/create-guarantor.component.ts |
Replaces dataObject with loanData, adds editGuarantor flow opening the new dialog, submits cleaned payloads (stripping empty/null/undefined), introduces refreshGuarantors() to re-fetch list, and simplifies template rendering/permission guards. |
Loans Service & Resolver src/app/loans/loans.service.ts, src/app/loans/common-resolvers/loan-action-button.resolver.ts |
Adds public methods getGuarantors() and updateGuarantor() to LoansService; updates resolver to safely fetch guarantors with catchError returning an empty array on failure. |
Loans View Refactor & Null-safety src/app/loans/loans-view/loans-view.component.ts, src/app/loans/loans-view/loans-view.component.html |
Extensive null-safety guards and conditional rendering: defer initialization and UI setup until loanDetailsData exists, add optional chaining across helpers and template, and reorganize large template blocks to render only when data is present. |
Sequence Diagram
sequenceDiagram
participant User
participant ViewGuarantorsComponent
participant LoansService
participant API
participant EditGuarantorDialog
User->>ViewGuarantorsComponent: Click "Edit" on guarantor
ViewGuarantorsComponent->>LoansService: GET /loans/{id}/template (relationTypes)
LoansService->>API: GET /loans/{id}/template
API-->>LoansService: template (relationTypes)
LoansService-->>ViewGuarantorsComponent: template data
ViewGuarantorsComponent->>EditGuarantorDialog: Open (guarantorData, relationTypes)
User->>EditGuarantorDialog: Edit form → Submit
EditGuarantorDialog-->>ViewGuarantorsComponent: Form result (cleaned)
ViewGuarantorsComponent->>LoansService: PUT /loans/{id}/guarantors/{guarantorId} (payload)
LoansService->>API: PUT /loans/{id}/guarantors/{guarantorId}
API-->>LoansService: updated guarantor
LoansService-->>ViewGuarantorsComponent: success
ViewGuarantorsComponent->>LoansService: GET /loans/{id}/guarantors (refresh)
LoansService->>API: GET /loans/{id}/guarantors
API-->>LoansService: guarantor list
LoansService-->>ViewGuarantorsComponent: updated list
ViewGuarantorsComponent->>User: Render refreshed guarantors
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- WEB-398 fix(breadcrumb):update breadcrumb headings on language change #2954 — Modifies the same breadcrumb component and breadcrumb generation flow.
- WEB-795 Fix the Loan Information section #3236 — Overlaps with large loans-view template/component reorganizations and null-safety changes.
- WEB-642: Add Loan Originators management #3095 — Related resolver changes for guarantor/template fetching and resolve flow.
Suggested reviewers
- IOhacker
- adamsaghy
🚥 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 clearly summarizes the main changes: fixing guarantors page data display and updating breadcrumb navigation to Loans, which are core focus areas of this PR. |
| 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 unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
📝 Coding Plan
- Generate coding plan for human review comments
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/core/shell/breadcrumb/breadcrumb.component.ts (1)
197-218:⚠️ Potential issue | 🟡 MinorSavings breadcrumbs still stay plain-text.
Line 198 can set
urltofalse, and the new rewrite on Lines 205-218 only runs for string URLs.src/app/savings/savings-routing.module.ts:56-58setsaddBreadcrumbLink: falseon the module root, so savings account pages never hit this branch and the"Savings"breadcrumb remains non-clickable. If this change is meant to cover savings too, derive the entity URL fromcurrentUrlbefore applying theaddBreadcrumbLinkoverride, or let this account-root rewrite opt back into linking when it finds a concrete account URL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/shell/breadcrumb/breadcrumb.component.ts` around lines 197 - 218, The breadcrumb URL rewrite can set url = false when module routes set addBreadcrumbLink: false (routeAddBreadcrumbLink), leaving "Savings" unclickable; to fix, compute the entity URL from currentUrl (or from this.router.url) before applying the addBreadcrumbLink override and/or, when the account-root rewrite (the block using accountPathMatch and entityUrlMatch) finds a concrete account URL, restore url to that string even if route.snapshot.data[routeAddBreadcrumbLink] was false; update the logic around the routeAddBreadcrumbLink override and the account-path rewrite in breadcrumb.component.ts so the accountPathMatch/entityUrlMatch extraction runs on currentUrl/this.router.url and can set url to the entity path when found.
🧹 Nitpick comments (2)
src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.ts (1)
70-75: Tighten typing for guarantor data flow (anyis too broad here).This edit path now handles key UI/API contracts, so broad
anyweakens checks on payload shape and makes regressions easier to miss.As per coding guidelines
src/app/**/*.ts: “Use TypeScript for all application code with strict typing conventions”, and based on learnings: “avoid using Observable/any response shapes in TypeScript files; introduce specific interfaces/types.”Also applies to: 128-133, 159-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.ts` around lines 70 - 75, The component uses broad any types for key properties (dataObject, loanData, guarantorDetails, loanId) which weakens type safety; define and use explicit interfaces (e.g., Guarantor, LoanData, LoanId) and replace any with those types on the `@Input`() dataObject, loanData: LoanData, guarantorDetails: Guarantor[] (or Guarantor | Guarantor[] as appropriate), and loanId: string|number; update method signatures and any Observables that consume these values to return/emit the new interfaces and adjust usages in the template; also apply the same typing fixes to the other referenced spots (around lines 128-133 and 159-160) so the component and its data flow conform to strict TypeScript typing.src/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.ts (1)
37-43: Type the dialog contract instead ofanyto preserve compile-time safety.
dataandrelationTypesare currently untyped, so payload shape drift between caller and dialog won’t be caught early. Please introduce explicit interfaces for dialog input/form output.♻️ Proposed typing refinement
+interface EditGuarantorDialogData { + guarantorData?: { + firstname?: string; + lastname?: string; + clientRelationshipType?: { id?: number }; + addressLine1?: string; + addressLine2?: string; + city?: string; + zip?: string; + mobileNumber?: string; + housePhoneNumber?: string; + }; + relationTypes?: Array<{ id: number; value: string }>; +} + export class EditGuarantorDialogComponent implements OnInit { dialogRef = inject<MatDialogRef<EditGuarantorDialogComponent>>(MatDialogRef); - data = inject(MAT_DIALOG_DATA); + data = inject<EditGuarantorDialogData>(MAT_DIALOG_DATA); private formBuilder = inject(UntypedFormBuilder); editGuarantorForm: UntypedFormGroup; - relationTypes: any[] = []; + relationTypes: Array<{ id: number; value: string }> = [];As per coding guidelines
src/app/**/*.ts: “Use TypeScript for all application code with strict typing conventions”, and based on learnings: “avoid using Observable/any response shapes in TypeScript files; introduce specific interfaces/types.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.ts` around lines 37 - 43, Declare explicit interfaces for the dialog input/output and relation items and apply them to the injected values and form: create e.g. EditGuarantorDialogData (shape of MAT_DIALOG_DATA), GuarantorRelationType (shape of items in relationTypes) and EditGuarantorResult (shape returned via MatDialogRef.close), then change the injections/types in EditGuarantorDialogComponent so data: EditGuarantorDialogData = inject(MAT_DIALOG_DATA), relationTypes: GuarantorRelationType[] = [], and make dialogRef a MatDialogRef<EditGuarantorDialogComponent, EditGuarantorResult>; also tighten editGuarantorForm typing to a typed FormGroup whose controls match the guarantor model so form.value is strongly typed when used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/loans/common-resolvers/loan-action-button.resolver.ts`:
- Around line 68-69: The resolver currently swallows all errors with
catchError(() => of([])) when calling loansService.getGuarantors(loanId); change
this to only convert a definitive “no data” response (e.g., 404 or a specific
API signal) to an empty array and rethrow other errors so the component can
react (e.g., use catchError(err => err.status === 404 ? of([]) :
throwError(err))). Then add explicit error handling in the component's
refreshGuarantors() subscription to catch errors from getGuarantors(), set an
error state (e.g., guarantorError or similar) and surface a user-facing message
(toast/snackbar/template alert) rather than silently doing nothing; reference
getGuarantors, refreshGuarantors, and guarantorDetails when locating places to
update.
In
`@src/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.html`:
- Around line 47-53: The inputs bound to formControlName "mobileNumber" and
"housePhoneNumber" are using type="number" which can strip leading zeros and
disallow "+"; change both input elements in the edit-guarantor-dialog template
from type="number" to type="tel" so phone values are treated as text while
keeping matInput and the existing formControlName bindings intact.
In
`@src/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.ts`:
- Around line 44-57: In ngOnInit (edit-guarantor-dialog.component.ts) guard
this.data.guarantorData before dereferencing when building editGuarantorForm:
ensure you assign a safe default object (e.g., const guarantor =
this.data?.guarantorData || {}) and use that in the formBuilder.group call so
fields like firstname, lastname, clientRelationshipType?.id, addressLine1, etc.
read from a defined object; also ensure relationTypes is initialized with
this.data?.relationTypes || [] to avoid undefined access.
In
`@src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.html`:
- Around line 79-82: The template currently renders both the formatted
loanData.summary?.totalOverdue and the "Not Provided" fallback together; change
the view in view-guarantors.component.html to conditionally render only one
branch so that if loanData.summary?.totalOverdue is negative you display the
translated 'labels.inputs.Not Provided' text, otherwise display the formatted
totalOverdue value (use an *ngIf/*ngIfElse, ternary pipe usage, or equivalent
template branching around loanData.summary?.totalOverdue to ensure mutual
exclusivity).
In
`@src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.ts`:
- Around line 129-134: When opening the EditGuarantorDialogComponent in the
callback from loansService.getGuarantorTemplate(this.loanId), guard against
templateData.allowedClientRelationshipTypes being undefined by passing a safe
fallback (e.g., an empty array) into the dialog data; update the data object
passed to EditGuarantorDialogComponent to use
templateData.allowedClientRelationshipTypes ?? [] (or || []) for the
relationTypes property so the dialog always receives a defined array while
keeping guarantorData as-is.
In `@src/app/loans/loans-view/loans-view.component.html`:
- Around line 165-169: The button hardcodes aria-label="Loan account actions" in
loans-view.component.html; replace that hardcoded string with a translated value
using `@ngx-translate` core so screen readers get localized text (target the
button with matMenuTriggerFor="accountMenu" / the menu identified by
accountMenu). Bind aria-label to a translate key (e.g. loans.account.actions)
via the translate pipe or TranslateService ([attr.aria-label] or interpolation
with | translate) and add the corresponding key to your translation JSON files.
In `@src/app/loans/loans-view/loans-view.component.ts`:
- Line 156: The assignment to this.recalculateInterest currently uses logical OR
which converts an explicit false from loanDetailsData?.recalculateInterest into
true; change the fallback to use nullish coalescing instead so that explicit
false is preserved (i.e., replace the use of || with ?? in the initialization of
this.recalculateInterest where loanDetailsData?.recalculateInterest is read).
---
Outside diff comments:
In `@src/app/core/shell/breadcrumb/breadcrumb.component.ts`:
- Around line 197-218: The breadcrumb URL rewrite can set url = false when
module routes set addBreadcrumbLink: false (routeAddBreadcrumbLink), leaving
"Savings" unclickable; to fix, compute the entity URL from currentUrl (or from
this.router.url) before applying the addBreadcrumbLink override and/or, when the
account-root rewrite (the block using accountPathMatch and entityUrlMatch) finds
a concrete account URL, restore url to that string even if
route.snapshot.data[routeAddBreadcrumbLink] was false; update the logic around
the routeAddBreadcrumbLink override and the account-path rewrite in
breadcrumb.component.ts so the accountPathMatch/entityUrlMatch extraction runs
on currentUrl/this.router.url and can set url to the entity path when found.
---
Nitpick comments:
In
`@src/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.ts`:
- Around line 37-43: Declare explicit interfaces for the dialog input/output and
relation items and apply them to the injected values and form: create e.g.
EditGuarantorDialogData (shape of MAT_DIALOG_DATA), GuarantorRelationType (shape
of items in relationTypes) and EditGuarantorResult (shape returned via
MatDialogRef.close), then change the injections/types in
EditGuarantorDialogComponent so data: EditGuarantorDialogData =
inject(MAT_DIALOG_DATA), relationTypes: GuarantorRelationType[] = [], and make
dialogRef a MatDialogRef<EditGuarantorDialogComponent, EditGuarantorResult>;
also tighten editGuarantorForm typing to a typed FormGroup whose controls match
the guarantor model so form.value is strongly typed when used.
In
`@src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.ts`:
- Around line 70-75: The component uses broad any types for key properties
(dataObject, loanData, guarantorDetails, loanId) which weakens type safety;
define and use explicit interfaces (e.g., Guarantor, LoanData, LoanId) and
replace any with those types on the `@Input`() dataObject, loanData: LoanData,
guarantorDetails: Guarantor[] (or Guarantor | Guarantor[] as appropriate), and
loanId: string|number; update method signatures and any Observables that consume
these values to return/emit the new interfaces and adjust usages in the
template; also apply the same typing fixes to the other referenced spots (around
lines 128-133 and 159-160) so the component and its data flow conform to strict
TypeScript typing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f332414-1995-4f23-85ed-b6a7bd788b79
📒 Files selected for processing (13)
src/app/core/shell/breadcrumb/breadcrumb.component.tssrc/app/loans/common-resolvers/loan-action-button.resolver.tssrc/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.htmlsrc/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.scsssrc/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.tssrc/app/loans/custom-dialog/loans-account-view-guarantor-details-dialog/loans-account-view-guarantor-details-dialog.component.htmlsrc/app/loans/custom-dialog/loans-account-view-guarantor-details-dialog/loans-account-view-guarantor-details-dialog.component.tssrc/app/loans/loans-view/loan-account-actions/create-guarantor/create-guarantor.component.tssrc/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.htmlsrc/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.tssrc/app/loans/loans-view/loans-view.component.htmlsrc/app/loans/loans-view/loans-view.component.tssrc/app/loans/loans.service.ts
| } else if (loanActionButton === 'View Guarantors') { | ||
| return this.loansService.getLoanAccountResource(loanId, 'guarantors'); | ||
| return this.loansService.getGuarantors(loanId).pipe(catchError(() => of([]))); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== resolver branch =="
sed -n '64,71p' src/app/loans/common-resolvers/loan-action-button.resolver.ts
echo
echo "== view-guarantors component consumption =="
rg -n -C2 'actionButtonData|guarantorDetails|refreshGuarantors' \
src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.ts
echo
echo "== view-guarantors template states =="
rg -n -C2 'guarantorDetails|mat-table|error|alert|snack|message' \
src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.htmlRepository: openMF/web-app
Length of output: 2066
Don't silence guarantor fetch failures as "no guarantors."
catchError(() => of([])) makes 403/500/network failures indistinguishable from a legitimate empty result. The component directly assigns the empty array to guarantorDetails and the template renders the same empty table whether the fetch succeeded with no results or failed entirely. Users cannot tell if guarantors truly don't exist or if a request failed.
Additionally, the refreshGuarantors() method (lines 159–161) also lacks error handling—if getGuarantors() fails, the component silently does nothing.
Either resolve the specific expected no-data case only, or surface an explicit error state (toast, snack bar, or template alert) to inform users when a fetch fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/loans/common-resolvers/loan-action-button.resolver.ts` around lines
68 - 69, The resolver currently swallows all errors with catchError(() =>
of([])) when calling loansService.getGuarantors(loanId); change this to only
convert a definitive “no data” response (e.g., 404 or a specific API signal) to
an empty array and rethrow other errors so the component can react (e.g., use
catchError(err => err.status === 404 ? of([]) : throwError(err))). Then add
explicit error handling in the component's refreshGuarantors() subscription to
catch errors from getGuarantors(), set an error state (e.g., guarantorError or
similar) and surface a user-facing message (toast/snackbar/template alert)
rather than silently doing nothing; reference getGuarantors, refreshGuarantors,
and guarantorDetails when locating places to update.
| ngOnInit() { | ||
| this.relationTypes = this.data.relationTypes || []; | ||
| const guarantor = this.data.guarantorData; | ||
| this.editGuarantorForm = this.formBuilder.group({ | ||
| firstname: [guarantor.firstname || ''], | ||
| lastname: [guarantor.lastname || ''], | ||
| clientRelationshipTypeId: [guarantor.clientRelationshipType?.id || ''], | ||
| addressLine1: [guarantor.addressLine1 || ''], | ||
| addressLine2: [guarantor.addressLine2 || ''], | ||
| city: [guarantor.city || ''], | ||
| zip: [guarantor.zip || ''], | ||
| mobileNumber: [guarantor.mobileNumber || ''], | ||
| housePhoneNumber: [guarantor.housePhoneNumber || ''] | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.tsRepository: openMF/web-app
Length of output: 2552
🏁 Script executed:
rg "EditGuarantorDialogComponent" --type ts -B 5 -A 5Repository: openMF/web-app
Length of output: 4588
🏁 Script executed:
rg "editGuarantor\(" --type ts -B 3 -A 1Repository: openMF/web-app
Length of output: 715
🏁 Script executed:
rg "\.editGuarantor\(" --type ts -B 2 -A 2Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
cat -n src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.ts | head -150Repository: openMF/web-app
Length of output: 6054
🏁 Script executed:
rg "editGuarantor" --type html -B 2 -A 2Repository: openMF/web-app
Length of output: 1384
🏁 Script executed:
cat -n src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.html | grep -A 20 -B 5 "editGuarantor"Repository: openMF/web-app
Length of output: 1218
Guard guarantorData before dereferencing to prevent form initialization crashes.
The guarantor object is accessed directly without null/undefined checks; if the data payload is missing or incomplete, this will throw at form initialization (lines 48-56 access properties like guarantor.firstname on a potentially undefined value).
🛡️ Proposed null-safe init
ngOnInit() {
- this.relationTypes = this.data.relationTypes || [];
- const guarantor = this.data.guarantorData;
+ this.relationTypes = this.data?.relationTypes ?? [];
+ const guarantor = this.data?.guarantorData ?? {};
this.editGuarantorForm = this.formBuilder.group({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ngOnInit() { | |
| this.relationTypes = this.data.relationTypes || []; | |
| const guarantor = this.data.guarantorData; | |
| this.editGuarantorForm = this.formBuilder.group({ | |
| firstname: [guarantor.firstname || ''], | |
| lastname: [guarantor.lastname || ''], | |
| clientRelationshipTypeId: [guarantor.clientRelationshipType?.id || ''], | |
| addressLine1: [guarantor.addressLine1 || ''], | |
| addressLine2: [guarantor.addressLine2 || ''], | |
| city: [guarantor.city || ''], | |
| zip: [guarantor.zip || ''], | |
| mobileNumber: [guarantor.mobileNumber || ''], | |
| housePhoneNumber: [guarantor.housePhoneNumber || ''] | |
| }); | |
| ngOnInit() { | |
| this.relationTypes = this.data?.relationTypes ?? []; | |
| const guarantor = this.data?.guarantorData ?? {}; | |
| this.editGuarantorForm = this.formBuilder.group({ | |
| firstname: [guarantor.firstname || ''], | |
| lastname: [guarantor.lastname || ''], | |
| clientRelationshipTypeId: [guarantor.clientRelationshipType?.id || ''], | |
| addressLine1: [guarantor.addressLine1 || ''], | |
| addressLine2: [guarantor.addressLine2 || ''], | |
| city: [guarantor.city || ''], | |
| zip: [guarantor.zip || ''], | |
| mobileNumber: [guarantor.mobileNumber || ''], | |
| housePhoneNumber: [guarantor.housePhoneNumber || ''] | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.ts`
around lines 44 - 57, In ngOnInit (edit-guarantor-dialog.component.ts) guard
this.data.guarantorData before dereferencing when building editGuarantorForm:
ensure you assign a safe default object (e.g., const guarantor =
this.data?.guarantorData || {}) and use that in the formBuilder.group call so
fields like firstname, lastname, clientRelationshipType?.id, addressLine1, etc.
read from a defined object; also ensure relationTypes is initialized with
this.data?.relationTypes || [] to avoid undefined access.
| {{ loanData.summary?.totalOverdue | formatNumber }} | ||
| @if (loanData.summary?.totalOverdue < 0) { | ||
| <span>{{ 'labels.inputs.Not Provided' | translate }}</span> | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "view-guarantors.component.html" -type fRepository: openMF/web-app
Length of output: 152
🏁 Script executed:
cat -n ./src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.html | sed -n '70,90p'Repository: openMF/web-app
Length of output: 875
🏁 Script executed:
cat -n ./src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.html | sed -n '1,20p'Repository: openMF/web-app
Length of output: 957
Render only one branch for "Arrears By"—either the formatted value or "Not Provided".
The current code outputs both the formatted totalOverdue value and the "Not Provided" fallback simultaneously when totalOverdue is negative, creating duplicate and confusing output. Use conditional branching to display only one at a time.
🧭 Proposed conditional rendering fix
- {{ loanData.summary?.totalOverdue | formatNumber }}
- `@if` (loanData.summary?.totalOverdue < 0) {
- <span>{{ 'labels.inputs.Not Provided' | translate }}</span>
- }
+ `@if` ((loanData.summary?.totalOverdue ?? -1) >= 0) {
+ <span>{{ loanData.summary?.totalOverdue | formatNumber }}</span>
+ } `@else` {
+ <span>{{ 'labels.inputs.Not Provided' | translate }}</span>
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{ loanData.summary?.totalOverdue | formatNumber }} | |
| @if (loanData.summary?.totalOverdue < 0) { | |
| <span>{{ 'labels.inputs.Not Provided' | translate }}</span> | |
| } | |
| `@if` ((loanData.summary?.totalOverdue ?? -1) >= 0) { | |
| <span>{{ loanData.summary?.totalOverdue | formatNumber }}</span> | |
| } `@else` { | |
| <span>{{ 'labels.inputs.Not Provided' | translate }}</span> | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.html`
around lines 79 - 82, The template currently renders both the formatted
loanData.summary?.totalOverdue and the "Not Provided" fallback together; change
the view in view-guarantors.component.html to conditionally render only one
branch so that if loanData.summary?.totalOverdue is negative you display the
translated 'labels.inputs.Not Provided' text, otherwise display the formatted
totalOverdue value (use an *ngIf/*ngIfElse, ternary pipe usage, or equivalent
template branching around loanData.summary?.totalOverdue to ensure mutual
exclusivity).
| this.loansService.getGuarantorTemplate(this.loanId).subscribe((templateData: any) => { | ||
| const editDialogRef = this.dialog.open(EditGuarantorDialogComponent, { | ||
| data: { | ||
| guarantorData: guarantorData, | ||
| relationTypes: templateData.allowedClientRelationshipTypes | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.ts | sed -n '120,145p'Repository: openMF/web-app
Length of output: 1300
🏁 Script executed:
find . -type f -name "*loans.service.ts" | head -5Repository: openMF/web-app
Length of output: 90
🏁 Script executed:
rg -A 10 "getGuarantorTemplate" src/app/loans/loans.service.tsRepository: openMF/web-app
Length of output: 469
🏁 Script executed:
find . -type f -name "*.ts" | xargs rg -l "allowedClientRelationshipTypes" | head -5Repository: openMF/web-app
Length of output: 245
🏁 Script executed:
rg -B 5 -A 5 "allowedClientRelationshipTypes" src/app/loans/loans-view/loan-account-actions/create-guarantor/create-guarantor.component.tsRepository: openMF/web-app
Length of output: 292
🏁 Script executed:
rg -B 15 -A 2 "setNewGuarantorDetailsForm" src/app/loans/loans-view/loan-account-actions/create-guarantor/create-guarantor.component.ts | head -40Repository: openMF/web-app
Length of output: 1087
🏁 Script executed:
rg -B 10 "this.dataObject" src/app/loans/loans-view/loan-account-actions/create-guarantor/create-guarantor.component.ts | head -50Repository: openMF/web-app
Length of output: 814
🏁 Script executed:
rg -B 20 "dataObject\s*=" src/app/loans/loans-view/loan-account-actions/create-guarantor/create-guarantor.component.ts | head -60Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
rg "dataObject" src/app/loans/loans-view/loan-account-actions/create-guarantor/create-guarantor.component.tsRepository: openMF/web-app
Length of output: 261
🏁 Script executed:
rg -B 5 -A 10 "allowedClientRelationshipTypes" src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.tsRepository: openMF/web-app
Length of output: 761
🏁 Script executed:
find . -type f -name "edit-guarantor-dialog.component.ts" -o -name "*edit*guarantor*dialog*"Repository: openMF/web-app
Length of output: 374
🏁 Script executed:
cat -n src/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.tsRepository: openMF/web-app
Length of output: 2552
🏁 Script executed:
rg -B 2 -A 15 "getGuarantorTemplate" src/app/loans/loans.service.tsRepository: openMF/web-app
Length of output: 640
🏁 Script executed:
rg -B 5 -A 5 "templateData\?" src/app/loans/Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
rg "subscribe\(\(.*\).*=>" src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.ts -A 5 | head -50Repository: openMF/web-app
Length of output: 1407
Use a fallback when reading relationship types from template response.
templateData.allowedClientRelationshipTypes can be undefined if the API returns an unexpected or incomplete payload. Although the dialog component has a fallback (this.data.relationTypes || []), it's better to handle this defensively at the source.
🧩 Proposed safe mapping
const editDialogRef = this.dialog.open(EditGuarantorDialogComponent, {
data: {
guarantorData: guarantorData,
- relationTypes: templateData.allowedClientRelationshipTypes
+ relationTypes: templateData?.allowedClientRelationshipTypes ?? []
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.ts`
around lines 129 - 134, When opening the EditGuarantorDialogComponent in the
callback from loansService.getGuarantorTemplate(this.loanId), guard against
templateData.allowedClientRelationshipTypes being undefined by passing a safe
fallback (e.g., an empty array) into the dialog data; update the data object
passed to EditGuarantorDialogComponent to use
templateData.allowedClientRelationshipTypes ?? [] (or || []) for the
relationTypes property so the dialog always receives a defined array while
keeping guarantorData as-is.
| <button | ||
| mat-icon-button | ||
| [matMenuTriggerFor]="accountMenu" | ||
| aria-label="Loan account actions" | ||
| yPosition="below" |
There was a problem hiding this comment.
Localize the menu button’s aria-label.
aria-label="Loan account actions" is still user-facing text for screen readers, so it will stay English-only even when the rest of the page is translated. Please move it behind a translate key like the surrounding labels.
As per coding guidelines, "Use proper i18n variables from @ngx-translate/core for all user-facing strings instead of hardcoded text".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/loans/loans-view/loans-view.component.html` around lines 165 - 169,
The button hardcodes aria-label="Loan account actions" in
loans-view.component.html; replace that hardcoded string with a translated value
using `@ngx-translate` core so screen readers get localized text (target the
button with matMenuTriggerFor="accountMenu" / the menu identified by
accountMenu). Bind aria-label to a translate key (e.g. loans.account.actions)
via the translate pipe or TranslateService ([attr.aria-label] or interpolation
with | translate) and add the corresponding key to your translation JSON files.
| this.loanStatus = this.loanDetailsData.status; | ||
| this.loanSubStatus = this.loanDetailsData.subStatus === undefined ? null : this.loanDetailsData.subStatus; | ||
| if (this.loanStatus.active && this.loanDetailsData.multiDisburseLoan) { | ||
| this.recalculateInterest = this.loanDetailsData?.recalculateInterest || true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant section
if [ -f "src/app/loans/loans-view/loans-view.component.ts" ]; then
echo "=== File exists, reading around line 156 ==="
wc -l src/app/loans/loans-view/loans-view.component.ts
sed -n '150,165p' src/app/loans/loans-view/loans-view.component.ts | cat -n
else
echo "File not found at specified path"
# Try to find it
find . -name "loans-view.component.ts" -type f
fiRepository: openMF/web-app
Length of output: 1063
🏁 Script executed:
# Search for other usages of recalculateInterest to understand its purpose and type
rg "recalculateInterest" --type ts -B 2 -A 2Repository: openMF/web-app
Length of output: 1674
Use nullish coalescing (??) instead of logical OR (||) to respect explicit false from the API.
The current code this.loanDetailsData?.recalculateInterest || true masks explicit false values and converts them to true. Since recalculateInterest is a feature flag controlling button visibility ("Add Interest Pause", "Prepay Loan"), an explicit false from the backend must be respected. Use ?? to only default to true when the value is null or undefined.
🐛 Proposed fix
- this.recalculateInterest = this.loanDetailsData?.recalculateInterest || true;
+ this.recalculateInterest = this.loanDetailsData?.recalculateInterest ?? true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.recalculateInterest = this.loanDetailsData?.recalculateInterest || true; | |
| this.recalculateInterest = this.loanDetailsData?.recalculateInterest ?? true; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/loans/loans-view/loans-view.component.ts` at line 156, The assignment
to this.recalculateInterest currently uses logical OR which converts an explicit
false from loanDetailsData?.recalculateInterest into true; change the fallback
to use nullish coalescing instead so that explicit false is preserved (i.e.,
replace the use of || with ?? in the initialization of this.recalculateInterest
where loanDetailsData?.recalculateInterest is read).
|
Hi @IOhacker ! |
…crumb navigation to Loans
f4db03f to
ae22cbc
Compare
|
Hey @IOhacker! |
There was a problem hiding this comment.
Actionable comments posted: 3
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/loan-account-actions/view-guarantors/view-guarantors.component.html (1)
101-103:⚠️ Potential issue | 🟡 MinorLocalize the deleted-guarantors toggle.
These labels are hardcoded, so this control stays English-only. Move both branches behind translate keys and extract them with the normal translations workflow. As per coding guidelines, "Use proper i18n variables from
@ngx-translate/corefor all user-facing strings instead of hardcoded text".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.html` around lines 101 - 103, Replace the hardcoded button labels with translation keys using `@ngx-translate/core`: change the template button text to use translate keys (e.g. 'LOANS.SHOW_DELETED_GUARANTORS' and 'LOANS.HIDE_DELETED_GUARANTORS') chosen based on the existing showDeletedGuarantorsAccounts boolean and keep the (click)="toggleGuarantorsDetailsOverview()"; then add those keys and their localized strings to the translations files so they are extracted by the translations workflow; ensure you reference the template's showDeletedGuarantorsAccounts and toggleGuarantorsDetailsOverview() when updating the template.
♻️ Duplicate comments (5)
src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.html (1)
79-82:⚠️ Potential issue | 🟡 MinorRender only one "Arrears By" branch.
When
totalOverdueis negative, this cell still shows both the formatted value and the "Not Provided" fallback. Make the value and fallback mutually exclusive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.html` around lines 79 - 82, The template currently renders both the formatted totalOverdue and the "Not Provided" fallback; change the markup so the two branches are mutually exclusive by moving the formatted value and the fallback into separate conditional blocks using the loanData.summary?.totalOverdue check (e.g., one block with *ngIf="loanData.summary?.totalOverdue >= 0" that renders {{ loanData.summary?.totalOverdue | formatNumber }} and another with *ngIf="loanData.summary?.totalOverdue < 0" that renders the translated 'labels.inputs.Not Provided'). Ensure you reference loanData.summary?.totalOverdue in both conditions so only one branch displays.src/app/loans/loans-view/loans-view.component.html (1)
165-169:⚠️ Potential issue | 🟡 MinorLocalize the actions menu's accessible name.
The
aria-labelis still hardcoded, so screen readers will always announce this button in English. Bind[attr.aria-label]to a translate key like the surrounding UI text. As per coding guidelines, "Use proper i18n variables from@ngx-translate/corefor all user-facing strings instead of hardcoded text".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/loans-view.component.html` around lines 165 - 169, Replace the hardcoded aria-label on the menu trigger button with a bound translated value using the translate pipe (e.g., change aria-label="Loan account actions" to [attr.aria-label]=" 'loans.actionsMenuAria' | translate") so the button that references accountMenu uses `@ngx-translate/core` for accessibility; ensure the translation key (e.g., loans.actionsMenuAria) is added to the i18n JSON files and used wherever similar menu buttons appear.src/app/loans/loans-view/loans-view.component.ts (1)
156-156:⚠️ Potential issue | 🟠 MajorPreserve explicit
falseforrecalculateInterest.Line 156 still uses
|| true, so a backendfalsebecomestrueand re-enables actions that should stay hidden. Use?? truehere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/loans-view.component.ts` at line 156, Replace the fallback operator so explicit false values from the backend are preserved: change the assignment of this.recalculateInterest (currently using this.loanDetailsData?.recalculateInterest || true) to use the nullish coalescing operator (this.loanDetailsData?.recalculateInterest ?? true) in the loans-view.component.ts so only undefined/null will default to true and explicit false remains false.src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.ts (1)
113-116:⚠️ Potential issue | 🟠 MajorHandle delete/edit refresh failures explicitly.
These flows only implement the success path. If the delete/update request or the follow-up
getGuarantors()call fails, the table stays stale and the user gets no feedback.Also applies to: 129-149, 158-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.ts` around lines 113 - 116, The delete/edit flows currently only handle the success path and can leave the UI stale if either loansService.deleteGuarantor or the follow-up refreshGuarantors (which calls loansService.getGuarantors) fails; add error handling to the Observable subscriptions in view-guarantors.component.ts by providing error callbacks for deleteGuarantor and for the internal getGuarantors call inside refreshGuarantors, surface failures to the user (e.g., this.snackBar.error / this.toast.error or processLogger.error) and restore UI state (enable buttons/spinners) on error so the table isn’t left stale; specifically update the subscribe(...) calls in deleteGuarantor invocations and inside refreshGuarantors/getGuarantors to handle next and error, and ensure refreshGuarantors resolves both success and failure paths.src/app/loans/common-resolvers/loan-action-button.resolver.ts (1)
69-69:⚠️ Potential issue | 🟠 MajorDon't mask guarantor fetch failures as "no guarantors".
catchError(() => of([]))still makes 403/500/network failures look like a legitimate empty result. That leaves the view unable to distinguish "no guarantors" from "load failed" and renders the same empty state for both.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/common-resolvers/loan-action-button.resolver.ts` at line 69, The current catchError(() => of([])) on this.loansService.getGuarantors(loanId) masks real errors as an empty guarantor list; remove or change that swallow to propagate the error so the caller/view can distinguish failure from an empty result. Replace catchError(() => of([])) with either remove the catchError entirely or rethrow the original error (e.g., catchError(err => throwError(() => err))) or return a typed envelope (e.g., of({ data: [], error: err })) consistent with your app's error handling, and update the consuming code to handle the propagated error/envelope instead of treating it as "no guarantors".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/core/shell/breadcrumb/breadcrumb.component.ts`:
- Around line 205-217: The regex that identifies module-root account paths
requires a trailing slash, so URLs like "/clients/:id/loans-accounts" don't
match; update the match in the breadcrumb logic (the url
variable/accountPathMatch check in breadcrumb.component.ts) to allow an optional
trailing slash (e.g., change the pattern from
/\/(loans-accounts|savings-accounts|shares-accounts)\/$/ to one that accepts an
optional "/" before the end). Keep the rest of the logic (building fullUrl from
this.router.url and the entityUrlMatch RegExp using accountPathMatch[1]) the
same so that when the module-root is matched without a trailing slash the code
still locates the entity URL and rewrites url appropriately.
In
`@src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.html`:
- Around line 159-167: The template currently gates the eye button with the
wrong permission ('READ_SAVINGSACCOUNT'), preventing users with guarantor access
from calling viewGuarantorDetails(guarantor); change the permission token in the
*mifosxHasPermission directive on that button to the guarantor-read permission
(e.g. 'READ_GUARANTOR' or the project's canonical guarantor read permission) so
the button is shown to users allowed to view guarantor details.
In `@src/app/loans/loans-view/loans-view.component.html`:
- Around line 367-377: The template guard currently reads
loanDetailsData.overdueCharges.length and will throw if overdueCharges is
undefined; update the condition in the loans-view template to safely handle
missing data by using the safe-navigation operator or a nullish fallback (e.g.,
check loanDetailsData.overdueCharges?.length > 0 or
(loanDetailsData.overdueCharges ?? []).length > 0) so the tab link rendering
(the <a mat-tab-link ...> block) only runs when the array exists and has items.
---
Outside diff comments:
In
`@src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.html`:
- Around line 101-103: Replace the hardcoded button labels with translation keys
using `@ngx-translate/core`: change the template button text to use translate keys
(e.g. 'LOANS.SHOW_DELETED_GUARANTORS' and 'LOANS.HIDE_DELETED_GUARANTORS')
chosen based on the existing showDeletedGuarantorsAccounts boolean and keep the
(click)="toggleGuarantorsDetailsOverview()"; then add those keys and their
localized strings to the translations files so they are extracted by the
translations workflow; ensure you reference the template's
showDeletedGuarantorsAccounts and toggleGuarantorsDetailsOverview() when
updating the template.
---
Duplicate comments:
In `@src/app/loans/common-resolvers/loan-action-button.resolver.ts`:
- Line 69: The current catchError(() => of([])) on
this.loansService.getGuarantors(loanId) masks real errors as an empty guarantor
list; remove or change that swallow to propagate the error so the caller/view
can distinguish failure from an empty result. Replace catchError(() => of([]))
with either remove the catchError entirely or rethrow the original error (e.g.,
catchError(err => throwError(() => err))) or return a typed envelope (e.g., of({
data: [], error: err })) consistent with your app's error handling, and update
the consuming code to handle the propagated error/envelope instead of treating
it as "no guarantors".
In
`@src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.html`:
- Around line 79-82: The template currently renders both the formatted
totalOverdue and the "Not Provided" fallback; change the markup so the two
branches are mutually exclusive by moving the formatted value and the fallback
into separate conditional blocks using the loanData.summary?.totalOverdue check
(e.g., one block with *ngIf="loanData.summary?.totalOverdue >= 0" that renders
{{ loanData.summary?.totalOverdue | formatNumber }} and another with
*ngIf="loanData.summary?.totalOverdue < 0" that renders the translated
'labels.inputs.Not Provided'). Ensure you reference
loanData.summary?.totalOverdue in both conditions so only one branch displays.
In
`@src/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.ts`:
- Around line 113-116: The delete/edit flows currently only handle the success
path and can leave the UI stale if either loansService.deleteGuarantor or the
follow-up refreshGuarantors (which calls loansService.getGuarantors) fails; add
error handling to the Observable subscriptions in view-guarantors.component.ts
by providing error callbacks for deleteGuarantor and for the internal
getGuarantors call inside refreshGuarantors, surface failures to the user (e.g.,
this.snackBar.error / this.toast.error or processLogger.error) and restore UI
state (enable buttons/spinners) on error so the table isn’t left stale;
specifically update the subscribe(...) calls in deleteGuarantor invocations and
inside refreshGuarantors/getGuarantors to handle next and error, and ensure
refreshGuarantors resolves both success and failure paths.
In `@src/app/loans/loans-view/loans-view.component.html`:
- Around line 165-169: Replace the hardcoded aria-label on the menu trigger
button with a bound translated value using the translate pipe (e.g., change
aria-label="Loan account actions" to [attr.aria-label]=" 'loans.actionsMenuAria'
| translate") so the button that references accountMenu uses `@ngx-translate/core`
for accessibility; ensure the translation key (e.g., loans.actionsMenuAria) is
added to the i18n JSON files and used wherever similar menu buttons appear.
In `@src/app/loans/loans-view/loans-view.component.ts`:
- Line 156: Replace the fallback operator so explicit false values from the
backend are preserved: change the assignment of this.recalculateInterest
(currently using this.loanDetailsData?.recalculateInterest || true) to use the
nullish coalescing operator (this.loanDetailsData?.recalculateInterest ?? true)
in the loans-view.component.ts so only undefined/null will default to true and
explicit false remains false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 513b4e66-59dc-48ec-b475-57b97214464f
📒 Files selected for processing (13)
src/app/core/shell/breadcrumb/breadcrumb.component.tssrc/app/loans/common-resolvers/loan-action-button.resolver.tssrc/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.htmlsrc/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.scsssrc/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.tssrc/app/loans/custom-dialog/loans-account-view-guarantor-details-dialog/loans-account-view-guarantor-details-dialog.component.htmlsrc/app/loans/custom-dialog/loans-account-view-guarantor-details-dialog/loans-account-view-guarantor-details-dialog.component.tssrc/app/loans/loans-view/loan-account-actions/create-guarantor/create-guarantor.component.tssrc/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.htmlsrc/app/loans/loans-view/loan-account-actions/view-guarantors/view-guarantors.component.tssrc/app/loans/loans-view/loans-view.component.htmlsrc/app/loans/loans-view/loans-view.component.tssrc/app/loans/loans.service.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.html
- src/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.scss
- src/app/loans/custom-dialog/edit-guarantor-dialog/edit-guarantor-dialog.component.ts
- src/app/loans/loans.service.ts
- src/app/loans/loans-view/loan-account-actions/create-guarantor/create-guarantor.component.ts
- src/app/loans/custom-dialog/loans-account-view-guarantor-details-dialog/loans-account-view-guarantor-details-dialog.component.html
- src/app/loans/custom-dialog/loans-account-view-guarantor-details-dialog/loans-account-view-guarantor-details-dialog.component.ts
| if (url && typeof url === 'string') { | ||
| const accountPathMatch = url | ||
| .replace(/\/+/g, '/') | ||
| .match(/\/(loans-accounts|savings-accounts|shares-accounts)\/$/); | ||
| if (accountPathMatch) { | ||
| const fullUrl = this.router.url.replace(/\/+/g, '/'); | ||
| const entityUrlMatch = fullUrl.match(new RegExp(`(.*/${accountPathMatch[1]}/\\d+)`)); | ||
| if (entityUrlMatch) { | ||
| url = entityUrlMatch[1]; | ||
| } else { | ||
| url = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Allow module-root matches without a trailing slash.
currentUrl is built as /clients/:id/loans-accounts, but the regex at Lines 206-208 only matches /clients/:id/loans-accounts/. In the common case this rewrite never runs, so the breadcrumb still points at the module root instead of the entity page.
🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 210-210: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((.*/${accountPathMatch[1]}/\\d+))
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/core/shell/breadcrumb/breadcrumb.component.ts` around lines 205 -
217, The regex that identifies module-root account paths requires a trailing
slash, so URLs like "/clients/:id/loans-accounts" don't match; update the match
in the breadcrumb logic (the url variable/accountPathMatch check in
breadcrumb.component.ts) to allow an optional trailing slash (e.g., change the
pattern from /\/(loans-accounts|savings-accounts|shares-accounts)\/$/ to one
that accepts an optional "/" before the end). Keep the rest of the logic
(building fullUrl from this.router.url and the entityUrlMatch RegExp using
accountPathMatch[1]) the same so that when the module-root is matched without a
trailing slash the code still locates the entity URL and rewrites url
appropriately.
There was a problem hiding this comment.
The trailing slash is present in currentUrl.
Angular's empty-path child route (path: '') produces an empty routeURL, and currentUrl += /${routeURL}at line 121 appends a/. So at the Loans breadcrumb, currentUrlis/clients/175/general/loans-accounts/ - the regex matches as intended. This was verified during development
There was a problem hiding this comment.
@YousufFFFF, thanks for the clear explanation! You're right — Angular's empty-path child route appends an empty segment via currentUrl += \/${routeURL}`, which reliably produces the trailing slash (e.g. /clients/175/general/loans-accounts/`), so the regex matches correctly. My concern was unfounded.
Regarding the ast-grep ReDoS warning on new RegExp(...): that's also a false positive here. accountPathMatch[1] is already validated by the preceding .match(/(loans-accounts|savings-accounts|shares-accounts)\/$/), so it can only ever be one of three known safe strings — there's no untrusted input reaching the RegExp constructor.
✏️ Learnings added
Learnt from: YousufFFFF
Repo: openMF/web-app PR: 3380
File: src/app/core/shell/breadcrumb/breadcrumb.component.ts:205-217
Timestamp: 2026-03-15T16:47:41.827Z
Learning: In `src/app/core/shell/breadcrumb/breadcrumb.component.ts` (BreadcrumbComponent), Angular's empty-path child route (path: '') causes `currentUrl += \`/${routeURL}\`` (line ~121) to append a trailing `/` to `currentUrl`. So at the Loans/Savings module-root breadcrumb, `currentUrl` is already e.g. `/clients/175/general/loans-accounts/` — with a trailing slash — making the regex `/\/(loans-accounts|savings-accounts|shares-accounts)\/$/` match correctly. This was verified during development.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: DeathGun44
Repo: openMF/web-app PR: 3179
File: src/app/savings/common-resolvers/savings-account-actions.resolver.ts:44-57
Timestamp: 2026-02-17T12:26:35.042Z
Learning: In TypeScript files (e.g., src/app/.../*.ts), avoid using Observable<any> as a project-wide pattern for API responses. Treat typing the full API response layer as a cross-cutting refactor and track it as a separate enhancement backlog item rather than addressing it in individual bug fixes. When you encounter API responses, introduce specific interfaces/types for the response shapes and use proper typing instead of any, updating services and resolvers accordingly.
| @if (loanDetailsData.overdueCharges.length > 0) { | ||
| <a | ||
| mat-tab-link | ||
| [routerLink]="['./overdue-charges']" | ||
| routerLinkActive | ||
| #overduecharges="routerLinkActive" | ||
| [active]="overduecharges.isActive" | ||
| > | ||
| {{ 'labels.inputs.Overdue Charges' | translate }} | ||
| </a> | ||
| } |
There was a problem hiding this comment.
Guard overdueCharges before reading .length.
This condition assumes the backend always sends an array. If overdueCharges is missing, the tab bar throws before rendering; use loanDetailsData.overdueCharges?.length or a nullish fallback in the guard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/loans/loans-view/loans-view.component.html` around lines 367 - 377,
The template guard currently reads loanDetailsData.overdueCharges.length and
will throw if overdueCharges is undefined; update the condition in the
loans-view template to safely handle missing data by using the safe-navigation
operator or a nullish fallback (e.g., check
loanDetailsData.overdueCharges?.length > 0 or (loanDetailsData.overdueCharges ??
[]).length > 0) so the tab link rendering (the <a mat-tab-link ...> block) only
runs when the array exists and has items.
There was a problem hiding this comment.
This is a pre-existing code, already exists on dev branch (line 356).
Adding optional chaining here would be a separate improvement outside the scope of this fix.
There was a problem hiding this comment.
@YousufFFFF, understood — since this guard was already present on the dev branch before this PR, the risk isn't introduced by these changes. That's a fair scoping call.
Would you like me to open a follow-up GitHub issue to track adding the ?.length safe-navigation guard to loanDetailsData.overdueCharges in loans-view.component.html? That way it can be addressed in a dedicated PR without blocking this one.
🧠 Learnings used
Learnt from: shubhamkumar9199
Repo: openMF/web-app PR: 3367
File: src/app/savings/gsim-account/gsim-account.component.html:17-17
Timestamp: 2026-03-13T09:33:25.634Z
Learning: In `src/app/savings/gsim-account/gsim-account.component.html`, the `mat-card-header` / `mat-card-title` elements were intentionally removed from the Group Summary card because `mat-card-header` introduces flex layout behavior that caused horizontal overflow and alignment issues with the `layout-row-wrap` container. A plain `<h3>` heading inside `mat-card-content` is used instead as a deliberate workaround. Do not flag this as a violation of the Angular Material usage guideline.
|
@YousufFFFF could you please review the latest Code Rabbit comments? |
Sure! |
|
Hey @IOhacker! |
2 participants
Description
This pull request introduces enhancements and fixes related to loan guarantor management, including improved error handling, UI consistency, and the addition of an edit dialog for guarantors. The most important changes are grouped below.
Guarantor Management Improvements
EditGuarantorDialogComponentwith corresponding HTML and SCSS files, enabling users to edit guarantor details via a dialog. The dialog is initialized with existing data and relationship types, and submits the updated form back to the parent component. [1] [2] [3]LoanActionButtonResolverby returning an empty array if fetching guarantors fails, preventing potential runtime errors. [1] [2]CreateGuarantorComponentto remove empty optional fields before submitting data, reducing API validation errors.UI and Data Consistency
view-guarantors.component.htmltable to consistently useloanDatainstead ofdataObject, improved null handling, and streamlined the display of guarantor details, including relationship and funding information. Also added edit action for eligible guarantors. [1] [2] [3] [4] [5]loans-account-view-guarantor-details-dialog.component.htmland its component to improve layout, null safety, and formatting, including the addition of theFormatNumberPipefor numeric values. [1] [2] [3] [4]Navigation Fixes
Fixes Issue:
#WEB-38
Before Video:
General.-.Google.Chrome.2026-03-15.02-46-34.mp4
After Video:
General.-.Google.Chrome.2026-03-15.02-13-02.mp4
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
Improvements
UX