Update Angular from 19 to 20 #2737
Conversation
WalkthroughComprehensive Angular 19-to-20 upgrade with systematic migration of template syntax from structural directives (*ngIf, *ngFor) to new control flow syntax (@if, @for). Dependency updates span framework packages, Material, FontAwesome, tooling, and dev environment requirements. Angular schematics configuration added to angular.json. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (54)
src/app/clients/clients-view/client-actions/client-screen-reports/client-screen-reports.component.html (2)
9-13: Optimize the track expression in @for loop.The
@forloop tracks by the entire object reference (track template) rather than a unique identifier. Per Angular best practices and coding guidelines for trackBy optimization, track by the uniquetemplate.idinstead for better change detection performance.- @for (template of templatesData; track template) { + @for (template of templatesData; track template.id) {
15-20: Consider caching the form control for improved performance.Accessing
clientScreenReportForm.controls.templateIdrepeatedly in the template can trigger unnecessary checks. Consider extracting it as a template variable or component property to avoid redundant lookups on each change detection cycle.- @if (clientScreenReportForm.controls.templateId.hasError('required')) { + @if (templateIdControl.hasError('required')) {Then in the component, expose:
templateIdControl = this.clientScreenReportForm.get('templateId');src/app/clients/clients-view/client-actions/withdraw-client/withdraw-client.component.html (1)
29-33: Optimize track function to use stable identifier.The
@forloop currently tracks the entirereasonobject, but since each reason has a uniqueidproperty (used as the mat-option value), tracking byreason.idwould be more efficient and prevent unnecessary DOM reconciliation.Apply this diff to optimize the track function:
- @for (reason of withdrawalData; track reason) { + @for (reason of withdrawalData; track reason.id) { <mat-option [value]="reason.id"> {{ reason.name }} </mat-option>As per coding guidelines, stable track functions improve performance in loops.
src/app/accounting/financial-activity-mappings/edit-financial-activity-mapping/edit-financial-activity-mapping.component.html (1)
9-14: Track by unique identifier, not the entire object.The
@forloop should track byfinancialActivity.idinstead of the entirefinancialActivityobject. Tracking by a unique, immutable property prevents unnecessary re-renders when the array is re-created or objects are replaced in the change detection cycle.Apply this diff to optimize the track expression:
- @for (financialActivity of financialActivityData; track financialActivity) { + @for (financialActivity of financialActivityData; track financialActivity.id) {As per coding guidelines, trackBy optimizations should be applied to loops within component templates.
src/app/clients/clients-view/identities-tab/identities-tab.component.html (1)
40-49: Use stable unique identifier for @for track function.The track function should reference a unique identifier (e.g.,
document.id) rather than the entire object for optimal change detection performance and best practices.Apply this diff to improve the track function:
- @for (document of identity.documents; track document) { + @for (document of identity.documents; track document.id) {src/app/account-transfers/make-account-interbank-transfers/make-account-interbank-transfers.component.html (1)
46-57: Consider standardizing form control access patterns.Lines 46 and 52 use different approaches to access the form control:
makeAccountTransferForm.controls.transferAmount(line 46) versusmakeAccountTransferForm.get('transferAmount')?(line 52). While both work, the optional chaining approach on line 52 is slightly safer. For consistency, consider using the same pattern across all error checks in this template.Apply this diff to standardize on the safer
.get()approach:- @if (makeAccountTransferForm.controls.transferAmount.hasError('required')) { + @if (makeAccountTransferForm.get('transferAmount')?.hasError('required')) {Optionally, apply the same pattern to lines 15 and 63:
- @if (makeAccountTransferForm.controls.transferDate.hasError('required')) { + @if (makeAccountTransferForm.get('transferDate')?.hasError('required')) {- @if (makeAccountTransferForm.controls.transferDescription.hasError('required')) { + @if (makeAccountTransferForm.get('transferDescription')?.hasError('required')) {src/app/accounting/create-journal-entry/create-journal-entry.component.html (1)
209-216: Address mixed structural directive syntax.Line 213 uses the legacy
*mifosxHasPermissionstructural directive while the entire template has been migrated to Angular 20's control flow syntax (@if/@for). This creates an inconsistency in the migration approach.Consider migrating this custom directive to the new syntax pattern, or document why it must remain on the legacy syntax. If permission checking must wrap the button, consider replacing it with:
@if (hasPermission('CREATE_JOURNALENTRY')) { <button mat-raised-button color="primary" [disabled]="!journalEntryForm.valid" type="submit" > {{ 'labels.buttons.Submit' | translate }} </button> }This assumes the component exposes a
hasPermission()method or similar guard logic.src/app/accounting/financial-activity-mappings/create-financial-activity-mapping/create-financial-activity-mapping.component.html (1)
9-14: Improve trackBy expression for loop performance.The
trackexpression references the entire object. For better change detection performance, track by a unique identifier.- @for (financialActivity of financialActivityData; track financialActivity) { + @for (financialActivity of financialActivityData; track financialActivity.id) {As per coding guidelines, trackBy should reference a unique property rather than the full object.
src/app/clients/client-stepper/client-family-members-step/client-family-members-step.component.html (1)
11-11: Verify track strategy for change detection and reusability.The loop uses
track memberto track the entire object. For optimal change detection and DOM reuse in Angular, consider tracking by a unique identifier (e.g.,track member.idif available). If members don't have stable IDs, the current approach is acceptable but may cause unnecessary DOM re-creation on data mutations. Verify that this aligns with howclientFamilyMembersdata is managed in the component.src/app/clients/client-stepper/client-family-members-step/client-family-member-dialog/client-family-member-dialog.component.html (2)
49-53: Optimize track strategy in option loops for better performance.The
@forloops tracking dropdown options usetrack relation,track gender,track profession, andtrack maritalStatusrespectively. For better change detection and DOM reuse, especially if these option arrays are frequently updated, consider tracking by unique ID:track relation.id,track gender.id, etc. This is particularly important for Material select components where identity tracking affects rendering efficiency.Also applies to: 66-70, 83-87, 94-98
8-13: Error display logic is correct for Angular 20.The @if blocks conditionally render
mat-errorbased onhasError('required')checks. This pattern correctly gates error messages in the new control flow syntax. The form control access viafamilyMemberForm.controls.fieldName.hasError()is verbose but functional; consider extracting getter methods in the component for improved readability.Consider adding getter methods in the component class for cleaner template expressions:
// Example in component.ts get firstNameError() { return this.familyMemberForm.controls.firstName.hasError('required'); }Then simplify the template to:
@if (firstNameError) { <mat-error>...</mat-error> }Also applies to: 24-29, 55-60, 72-77
src/app/centers/centers-view/center-actions/manage-groups/manage-groups.component.html (2)
9-13: Consider tracking by unique identifier instead of object reference.The @for loop uses
track group, which tracks by the entire object reference. For better performance and to follow Angular best practices, consider tracking by a unique, stable property such astrack group.id.- @for (group of groupsData; track group) { + @for (group of groupsData; track group.id) {
43-43: Use unique identifier for tracking in nested loop.The nested @for loop also uses
track groupinstead of a stable unique identifier. Update it to track bygroup.idfor consistency and performance.- @for (group of groupMembers; track group; let i = $index) { + @for (group of groupMembers; track group.id; let i = $index) {src/app/collections/collection-sheet/collection-sheet.component.html (1)
7-11: Use unique identifiers intrackexpressions for optimal change detection performance.The
@forloop usestrack office(the entire object), which requires Angular to compare object references during change detection. It's more efficient to track by a unique identifier.Apply this diff to use unique identifiers:
- @for (office of officesData; track office) { + @for (office of officesData; track office.id) { <mat-option [value]="office.id"> {{ office.name }} </mat-option> }Apply the same pattern to other
@forloops in this file (lines 47, 60, 73):
- Line 47: change
track loanOfficer→track loanOfficer.id- Line 60: change
track group→track group.id- Line 73: change
track center→track center.idsrc/app/collections/individual-collection-sheet/individual-collection-sheet.component.html (1)
16-20: Use unique identifiers intrackexpressions for optimal change detection performance.The
@forloops usetrack officeandtrack loanOfficer(entire objects), which requires Angular to compare object references. Use unique identifiers instead for better change detection efficiency.Apply these diffs:
- @for (office of officesData; track office) { + @for (office of officesData; track office.id) { <mat-option [value]="office.id"> {{ office.name }} </mat-option> }- @for (loanOfficer of loanOfficerData; track loanOfficer) { + @for (loanOfficer of loanOfficerData; track loanOfficer.id) { <mat-option [value]="loanOfficer.id"> {{ loanOfficer.displayName }} </mat-option> }Also applies to: 51-56
src/app/clients/clients-view/client-actions/take-survey/take-survey.component.html (1)
6-10: Track patterns are baseline but could be more specific.The
@forloops use broad object tracking (e.g.,track surveyData,track component,track question). If these data objects have stable unique identifiers (IDs, keys, etc.), consider tracking by those properties for better change detection performance. This optimization becomes important in large lists but is currently acceptable for these use cases.For example:
- @for (surveyData of allSurveyData; track surveyData) { + @for (surveyData of allSurveyData; track surveyData.id) {Also applies to: 17-45
src/app/clients/client-stepper/client-datatable-step/client-datatable-step.component.html (2)
3-3: Optimize trackBy functions to use unique identifiers instead of entire objects.While the current trackBy implementations using the entire object are functional, they lose the performance benefits of trackBy for change detection. Use unique, stable identifiers (e.g., id, index) instead.
For line 3, if datatableInputs are uniquely identified by a property (e.g.,
idorcontrolName), apply:- @for (datatableInput of datatableInputs; track datatableInput) { + @for (datatableInput of datatableInputs; track datatableInput.controlName) {Similarly for line 12, if columnValues have a unique identifier:
- @for (code of datatableInput.columnValues; track code) { + @for (code of datatableInput.columnValues; track code.id) {Verify the appropriate unique property for each data source and update the track function accordingly.
Also applies to: 12-12
39-48: Improve accessibility for clickable span element.The span at line 39 has a click handler but lacks proper ARIA attributes or semantic meaning for keyboard navigation and screen readers.
Consider one of these approaches:
Option 1: Use a button element (preferred for click actions)
- <span (click)="datePicker.open()"> + <button type="button" (click)="datePicker.open()" class="icon-button">Option 2: Add ARIA attributes to span
- <span (click)="datePicker.open()"> + <span (click)="datePicker.open()" role="button" tabindex="0" (keydown.enter)="datePicker.open()">src/app/clients/edit-client/edit-client.component.html (2)
10-14: Improve@fortracking to use unique identifiers instead of object references.All @for loops currently use
track office,track legalForm, etc., which tracks by object reference. For better performance and alignment with Angular best practices, track by a unique identifier (typicallyid).- @for (office of officeOptions; track office) { + @for (office of officeOptions; track office.id) { <mat-option [value]="office.id"> {{ office.name }} </mat-option> }Apply this pattern to all
@forloops in the template:
- Line 10:
track office→track office.id- Line 27:
track legalForm→track legalForm.id- Line 114:
track gender→track gender.id- Line 129:
track staff→track staff.id- Line 156:
track constitution→track constitution.id- Line 166:
track business→track business.id- Line 217:
track clientType→track clientType.id- Line 228:
track clientClassification→track clientClassification.idAlso applies to: 27-31, 114-118, 129-133, 156-160, 166-170, 217-221, 228-234
16-21: Consider extracting form control access into template variables for clarity and safety.Multiple locations directly access deeply nested form controls (e.g.,
editClientForm.controls.officeId.hasError('required')), which can be error-prone. Using template variables with@let(or the template expression directly with safe navigation) improves readability and reduces redundant property access.- @if (editClientForm.controls.officeId.hasError('required')) { - <mat-error> - {{ 'labels.inputs.Office' | translate }} {{ 'labels.commons.is' | translate }} - <strong>{{ 'labels.commons.required' | translate }}</strong> - </mat-error> - } + @let officeControl = editClientForm.get('officeId'); + @if (officeControl?.hasError('required')) { + <mat-error> + {{ 'labels.inputs.Office' | translate }} {{ 'labels.commons.is' | translate }} + <strong>{{ 'labels.commons.required' | translate }}</strong> + </mat-error> + }This pattern improves type safety and readability throughout the template for similar validation patterns.
Also applies to: 54-59, 67-72, 87-92, 253-258
src/app/clients/client-stepper/client-general-step/client-general-step.component.html (2)
6-10: Use track with unique identifiers (e.g.,track item.id) instead of the entire object.Tracking by object identity rather than a stable unique identifier can trigger unnecessary DOM resets and re-renders during change detection, especially if array items are recreated. Use
track office.id,track legalForm.id, etc., for optimal performance.Apply this pattern across all @for loops:
-@for (office of officeOptions; track office) { +@for (office of officeOptions; track office.id) { <mat-option [value]="office.id"> {{ office.name }} </mat-option> -} +}Repeat for all similar @for loops.
Also applies to: 23-27, 138-143, 148-153, 182-187, 194-199, 232-237, 243-250, 306-311
12-17: Adopt consistent defensive checks for form control access.The template mixes patterns: some conditionals use
createClientForm.contains()before accessing controls (lines 36, 57, 130, 273, 302), while others access controls directly viacreateClientForm.controls.officeIdwithout an existence check (line 12, and repeatedly within error blocks). WhileformControlNamebindings ensure controls exist at bind time, for consistency and strict type safety, consider wrapping all direct.controls.*accesses in existence checks or usingget()with optional chaining.Consider this pattern for uniform defensive coding:
-@if (createClientForm.controls.officeId.hasError('required')) { +@if (createClientForm.get('officeId')?.hasError('required')) { <mat-error>Also applies to: 42-47, 48-53, 69-74, 75-80, 87-92, 99-104, 105-110, 289-294, 312-317
src/app/clients/clients-view/custom-dialogs/view-signature-dialog/view-signature-dialog.component.html (2)
3-13: Consider using @else for complementary conditions.Lines 3–7 and 9–13 contain mutually exclusive conditions (
@if (signatureId)and@if (!signatureId)). Using@elsewould be more idiomatic and maintainable in Angular 20:-@if (signatureId) { - <div mat-dialog-content> - <img [src]="signatureImage" alt="Signature" /> - </div> -} - -@if (!signatureId) { - <div mat-dialog-content> - <p>{{ 'labels.text.Client Signature is not provided.' | translate }}</p> - </div> -} +@if (signatureId) { + <div mat-dialog-content> + <img [src]="signatureImage" alt="Signature" /> + </div> +} @else { + <div mat-dialog-content> + <p>{{ 'labels.text.Client Signature is not provided.' | translate }}</p> + </div> +}
17-26: Refactor button conditions with @else.The action buttons (lines 17–26) also use complementary conditions that can be simplified with
@else:- @if (signatureId) { - <button mat-raised-button color="warn" [mat-dialog-close]="{ delete: true }"> - {{ 'labels.buttons.Delete' | translate }} - </button> - } - @if (!signatureId) { - <button mat-raised-button color="primary" [mat-dialog-close]="{ upload: true }"> - {{ 'labels.buttons.Upload' | translate }} - </button> - } + @if (signatureId) { + <button mat-raised-button color="warn" [mat-dialog-close]="{ delete: true }"> + {{ 'labels.buttons.Delete' | translate }} + </button> + } @else { + <button mat-raised-button color="primary" [mat-dialog-close]="{ upload: true }"> + {{ 'labels.buttons.Upload' | translate }} + </button> + }src/app/clients/clients-view/custom-dialogs/delete-signature-dialog/delete-signature-dialog.component.html (2)
3-13: Simplify complementary conditions with @else.Combine the mutually exclusive
@if (signatureId)and@if (!signatureId)blocks using@else:-@if (signatureId) { - <div mat-dialog-content> - <p>{{ 'labels.text.Delete clients signature' | translate }} ?</p> - </div> -} - -@if (!signatureId) { - <div mat-dialog-content> - <p>{{ 'labels.text.Client Signature is not provided.' | translate }}</p> - </div> -} +@if (signatureId) { + <div mat-dialog-content> + <p>{{ 'labels.text.Delete clients signature' | translate }} ?</p> + </div> +} @else { + <div mat-dialog-content> + <p>{{ 'labels.text.Client Signature is not provided.' | translate }}</p> + </div> +}
17-26: Use @else for button conditionals.Apply the same refactoring to lines 17–26:
- @if (signatureId) { - <button mat-raised-button color="warn" [mat-dialog-close]="{ delete: true, id: signatureId }"> - {{ 'labels.buttons.Confirm' | translate }} - </button> - } - @if (!signatureId) { - <button mat-raised-button color="primary" [mat-dialog-close]="{ upload: true }"> - {{ 'labels.buttons.Upload' | translate }} - </button> - } + @if (signatureId) { + <button mat-raised-button color="warn" [mat-dialog-close]="{ delete: true, id: signatureId }"> + {{ 'labels.buttons.Confirm' | translate }} + </button> + } @else { + <button mat-raised-button color="primary" [mat-dialog-close]="{ upload: true }"> + {{ 'labels.buttons.Upload' | translate }} + </button> + }src/app/clients/clients-view/family-members-tab/edit-family-member/edit-family-member.component.html (1)
48-52: Prefer tracking by unique identifier instead of object reference.All
@forloops currently track by the entire object (e.g.,track relation). For better change detection performance and adherence to Angular best practices, track by a unique identifier property instead.Apply these diffs to track by unique identifiers:
- @for (relation of addFamilyMemberTemplate.relationshipIdOptions; track relation) { + @for (relation of addFamilyMemberTemplate.relationshipIdOptions; track relation.id) {- @for (gender of addFamilyMemberTemplate.genderIdOptions; track gender) { + @for (gender of addFamilyMemberTemplate.genderIdOptions; track gender.id) {- @for (profession of addFamilyMemberTemplate.professionIdOptions; track profession) { + @for (profession of addFamilyMemberTemplate.professionIdOptions; track profession.id) {- @for (maritalStatus of addFamilyMemberTemplate.maritalStatusIdOptions; track maritalStatus) { + @for (maritalStatus of addFamilyMemberTemplate.maritalStatusIdOptions; track maritalStatus.id) {As per coding guidelines, proper tracking ensures optimal change detection behavior.
Also applies to: 65-69, 82-86, 93-99
src/app/clients/clients-view/custom-dialogs/capture-image-dialog/capture-image-dialog.component.ts (1)
26-32: Consider adding specific types to ElementRef declarations.For stricter type safety, specify the HTML element types in the ElementRef generics:
- @ViewChild('video', { static: true }) video: ElementRef; + @ViewChild('video', { static: true }) video: ElementRef<HTMLVideoElement>; - @ViewChild('canvas', { static: true }) canvas: ElementRef; + @ViewChild('canvas', { static: true }) canvas: ElementRef<HTMLCanvasElement>; - @ViewChild('fallback', { static: true }) fallback: ElementRef; + @ViewChild('fallback', { static: true }) fallback: ElementRef<HTMLElement>; - @ViewChild('captureButton') captureButton: ElementRef; + @ViewChild('captureButton') captureButton: ElementRef<HTMLButtonElement>;As per coding guidelines.
src/app/accounting/provisioning-entries/view-provisioning-entry/view-provisioning-entry.component.html (1)
62-84: Prefer tracking by unique property instead of object reference.The
@forloops currently track by object reference (track office,track loanProduct,track provisioningCategory). If the observable emits a new array with the same data, Angular will unnecessarily recreate all DOM elements because the object references differ.Consider tracking by a unique property for better performance:
For office autocomplete (lines 62-66):
-@for (office of filteredOfficeData | async; track office) { +@for (office of filteredOfficeData | async; track office.name) { <mat-option [value]="office.name"> {{ office.name }} </mat-option> }For loan product autocomplete (lines 70-74):
-@for (loanProduct of filteredLoanProductData | async; track loanProduct) { +@for (loanProduct of filteredLoanProductData | async; track loanProduct.name) { <mat-option [value]="loanProduct.name"> {{ loanProduct.name }} </mat-option> }For provisioning category autocomplete (lines 78-84):
-@for (provisioningCategory of filteredProvisioningCategoryData | async; track provisioningCategory) { +@for (provisioningCategory of filteredProvisioningCategoryData | async; track provisioningCategory.categoryName) { <mat-option [value]="provisioningCategory.categoryName" > {{ provisioningCategory.categoryName }} </mat-option> }Based on coding guidelines.
src/app/clients/clients-view/identities-tab/identities-tab.component.ts (2)
60-62: Consider adding stricter type definitions.The use of
anytypes reduces type safety and IDE support. Based on usage patterns in the component, these properties have known structures:
clientIdentitiesis an array of identity objects with properties likeid,description,documentType,documentKey,documents, etc.clientIdentifierTemplatehas at least anallowedDocumentTypesarray property.Consider creating interfaces for these types to improve type safety and developer experience.
Based on coding guidelines.
103-107: Consider adding error handling to HTTP subscriptions.The subscribe calls for HTTP operations (download, add, delete, upload) lack error handling. While these are user-initiated one-shot operations that typically succeed, adding error callbacks or catchError operators would improve user experience by providing feedback when operations fail.
Example for the download method:
download(parentEntityId: string, documentId: string) { - this.clientService.downloadClientIdentificationDocument(parentEntityId, documentId).subscribe((res) => { - const url = window.URL.createObjectURL(res); - window.open(url); - }); + this.clientService.downloadClientIdentificationDocument(parentEntityId, documentId).subscribe({ + next: (res) => { + const url = window.URL.createObjectURL(res); + window.open(url); + }, + error: (err) => { + // Handle error - show notification to user + console.error('Failed to download document:', err); + } + }); }Based on coding guidelines.
Also applies to: 166-182, 199-202, 221-230
src/app/clients/clients-view/client-actions/reject-client/reject-client.component.html (1)
29-33: Prefer tracking by unique identifier for better performance.The
@forloop currently tracks by object reference (track reason), which can cause unnecessary re-renders when the data array is refreshed with new object instances, even if the content is the same.Apply this diff to track by the unique identifier:
-@for (reason of rejectionData; track reason) { +@for (reason of rejectionData; track reason.id) { <mat-option [value]="reason.id"> {{ reason.name }} </mat-option> }As per coding guidelines
src/app/clients/clients-view/client-actions/update-client-savings-account/update-client-savings-account.component.html (1)
9-13: Prefer tracking by unique identifier for better performance.The
@forloop currently tracks by object reference (track account), which can cause unnecessary re-renders when the data array is refreshed with new object instances, even if the content is the same.Apply this diff to track by the unique identifier:
-@for (account of savingsAccounts; track account) { +@for (account of savingsAccounts; track account.id) { <mat-option [value]="account.id"> {{ account.accountNo }} </mat-option> }As per coding guidelines
src/app/clients/clients-view/client-actions/client-assign-staff/client-assign-staff.component.html (1)
15-20: Use safe navigation for form control access.Direct property access
clientAssignStaffForm.controls.staffIdcan throw if the control is undefined. Use the saferget()method instead:- @if (clientAssignStaffForm.controls.staffId.hasError('required')) { + @if (clientAssignStaffForm.get('staffId')?.hasError('required')) {src/app/centers/centers-view/center-actions/center-assign-staff/center-assign-staff.component.html (1)
15-20: Use safe navigation for form control access.Direct property access
centerAssignStaffForm.controls.staffIdcan throw if the control is undefined. Use the saferget()method instead:- @if (centerAssignStaffForm.controls.staffId.hasError('required')) { + @if (centerAssignStaffForm.get('staffId')?.hasError('required')) {src/app/clients/clients-view/client-actions/transfer-client/transfer-client.component.html (1)
15-20: Use safe navigation for form control access.Form control access should use the
get()method with safe navigation. Update both validation blocks:- @if (transferClientForm.controls.destinationOfficeId.hasError('required')) { + @if (transferClientForm.get('destinationOfficeId')?.hasError('required')) {- @if (transferClientForm.controls.transferDate.hasError('required')) { + @if (transferClientForm.get('transferDate')?.hasError('required')) {Also applies to: 35-40
src/app/clients/clients-view/client-actions/close-client/close-client.component.html (1)
15-23: Use safe navigation for form control access in both validation blocks.Update the form control access pattern for both the closureDate and closureReasonId validations:
- @if (closeClientForm.controls.closureDate.hasError('required')) { + @if (closeClientForm.get('closureDate')?.hasError('required')) {- @if (closeClientForm.controls.closureReasonId.hasError('required')) { + @if (closeClientForm.get('closureReasonId')?.hasError('required')) {Also applies to: 35-40
src/app/clients/clients-view/address-tab/address-tab.component.html (1)
29-78: LGTM with performance note!The defensive conditional patterns (
@if (isFieldEnabled(...))and@if (member.property)) are well-structured. Note that calls togetSelectedValue()in the template (lines 60, 65) may impact change detection performance if called frequently. Consider memoizing or moving to the component class if the list is large.src/app/centers/centers-view/notes-tab/notes-tab.component.html (1)
29-47: @for loop with track implemented.The conversion from
*ngForto@foris correctly implemented withtrack centerNoteand local index variable$index. However, for optimal performance with large note lists that may be reordered, consider usingtrack centerNote.idif centerNote objects have a stable ID field.- @for (centerNote of centerNotes; track centerNote; let i = $index) { + @for (centerNote of centerNotes; track centerNote.id; let i = $index) {src/app/clients/client-stepper/client-address-step/client-address-step.component.html (1)
10-12: Guard clause and @for loop structure.The outer
@if (clientAddressData.length > 0)guard ensures the accordion only renders when addresses exist. The inner@forloop correctly implements Angular 20 syntax withtrackandlet i = $index. However,track addressuses object identity; if address objects have an ID field, usetrack address.idfor optimal performance and consistency with change detection.- @for (address of clientAddressData; track address; let i = $index) { + @for (address of clientAddressData; track address.id; let i = $index) {src/app/clients/clients-view/general-tab/general-tab.component.html (1)
40-41: *Custom structural directive mifosxHasPermission coexists appropriately with new @if syntax. Mixing is intentional (permission layer on top of conditional logic) and compatible with Angular 20. Consider documenting or scheduling migration of custom directive to new control flow syntax in future refactoring cycles.Also applies to: 90-91, 99-101, 191-191, 207-207, 223-223
src/app/centers/centers-view/general-tab/general-tab.component.html (1)
81-157: Refactor repetitive deposit type conditionals.Lines 81-157 contain four separate
@ifblocks checkingelement.depositType.id === 100andelement.depositType.id === 300for both Deposit and Withdrawal actions. This creates unnecessary duplication and nesting complexity. Consider refactoring to consolidate logic—e.g., checkingelement.status.activeonce, then nesting the deposit type checks, or extracting button generation to a helper method/component.src/app/accounting/closing-entries/edit-closure/edit-closure.component.html (1)
9-13: Consider tracking by unique ID for better performance.The
@forloop currently tracks by object reference. For better change detection performance when the office list updates, track by the uniqueidproperty instead.Apply this diff:
- @for (office of officeData; track office) { + @for (office of officeData; track office.id) {As per coding guidelines requiring trackBy on loops.
src/app/accounting/closing-entries/create-closure/create-closure.component.html (1)
9-13: Track by unique identifier instead of full object for performance.The
track officeshould betrack office.idto avoid unnecessary DOM reconciliation when array items are unchanged. Since the mat-option binds[value]="office.id", the office object has an id property that should be used as the track key.Apply this diff:
- @for (office of officeData; track office) { + @for (office of officeData; track office.id) { <mat-option [value]="office.id"> {{ office.name }} </mat-option> - } + }src/app/accounting/closing-entries/closing-entries.component.html (1)
19-23: Track by unique identifier instead of full object for performance.The
track officeshould betrack office.idto ensure proper change detection. Since mat-option uses [value], the office object has an id that should be the track key.Apply this diff:
- @for (office of filteredOfficeData | async; track office) { + @for (office of filteredOfficeData | async; track office.id) { <mat-option [value]="office.name"> {{ office.name }} </mat-option> - } + }src/app/accounting/chart-of-accounts/edit-gl-account/edit-gl-account.component.html (3)
9-13: Track by unique identifier instead of full object.The
track accountTypeshould betrack accountType.idfor optimal performance. This allows Angular to properly identify items when the array is mutated.Apply this diff:
- @for (accountType of accountTypeData; track accountType) { + @for (accountType of accountTypeData; track accountType.id) { <mat-option [value]="accountType.id"> {{ accountType.value }} </mat-option> - } + }
37-41: Track by unique identifier instead of full object.The
track accountUsageshould betrack accountUsage.idfor consistency and performance.Apply this diff:
- @for (accountUsage of accountUsageData; track accountUsage) { + @for (accountUsage of accountUsageData; track accountUsage.id) { <mat-option [value]="accountUsage.id"> {{ accountUsage.value }} </mat-option> - } + }
74-78: Track by unique identifier instead of full object.The
track tagshould betrack tag.idfor optimal change detection performance.Apply this diff:
- @for (tag of tagData; track tag) { + @for (tag of tagData; track tag.id) { <mat-option [value]="tag.id"> {{ tag.name }} </mat-option> - } + }src/app/centers/centers-view/center-actions/edit-center-meeting/edit-center-meeting.component.html (2)
16-20: Track by unique identifier instead of full object.The
track optionshould betrack option.idfor optimal performance.Apply this diff:
- @for (option of frequencyOptions; track option) { + @for (option of frequencyOptions; track option.id) { <mat-option [value]="option.id"> {{ option.value }} </mat-option> - } + }
51-55: Track by unique identifier instead of full object.The
track dayshould betrack day.idfor consistency and performance.Apply this diff:
- @for (day of repeatsOnDays; track day) { + @for (day of repeatsOnDays; track day.id) { <mat-option [value]="day.id"> {{ day.value }} </mat-option> - } + }src/app/accounting/chart-of-accounts/create-gl-account/create-gl-account.component.html (3)
9-13: Track by unique identifier instead of full object.The
track accountTypeshould betrack accountType.idfor optimal performance.Apply this diff:
- @for (accountType of accountTypeData; track accountType) { + @for (accountType of accountTypeData; track accountType.id) { <mat-option [value]="accountType.id"> {{ 'labels.inputs.accounting.' + accountType.value | translate }} </mat-option> - } + }
37-41: Track by unique identifier instead of full object.The
track accountUsageshould betrack accountUsage.idfor consistency and performance.Apply this diff:
- @for (accountUsage of accountUsageData; track accountUsage) { + @for (accountUsage of accountUsageData; track accountUsage.id) { <mat-option [value]="accountUsage.id"> {{ accountUsage.value }} </mat-option> - } + }
74-78: Track by unique identifier instead of full object.The
track tagshould betrack tag.idfor optimal change detection performance.Apply this diff:
- @for (tag of tagData; track tag) { + @for (tag of tagData; track tag.id) { <mat-option [value]="tag.id"> {{ tag.name }} </mat-option> - } + }src/app/clients/clients-view/clients-view.component.html (1)
105-110: Minor: Consider consolidating @if/@if pattern into single @if/@else.Lines 105-110 use two separate @if blocks to handle staff presence/absence:
@if (clientViewData.staffName) { <td>{{ clientViewData.staffName }}</td> } @if (!clientViewData.staffName) { <td>{{ 'labels.inputs.Unassigned' | translate }}</td> }Consider combining into a single @if/@else for clarity:
@if (clientViewData.staffName) { <td>{{ clientViewData.staffName }}</td> } @else { <td>{{ 'labels.inputs.Unassigned' | translate }}</td> }This is a readability improvement but not a functional issue.
| this.id = this.route.snapshot.queryParams['savingsId']; | ||
| this.interbank = this.route.snapshot.queryParams['interbank'] === 'true'; | ||
| this.balance = this.router.getCurrentNavigation().extras.state.balance; | ||
| this.balance = this.router.currentNavigation().extras.state.balance; |
There was a problem hiding this comment.
Add null safety checks for Router API.
currentNavigation() can return null when called outside an active navigation context. Accessing properties without null checks will cause runtime errors.
Apply this diff to add proper null safety:
- this.balance = this.router.currentNavigation().extras.state.balance;
+ this.balance = this.router.currentNavigation()?.extras?.state?.balance ?? 0;📝 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.balance = this.router.currentNavigation().extras.state.balance; | |
| this.balance = this.router.currentNavigation()?.extras?.state?.balance ?? 0; |
🤖 Prompt for AI Agents
In
src/app/account-transfers/make-account-transfers/make-account-transfers.component.ts
around line 122, the call this.router.currentNavigation().extras.state.balance
assumes currentNavigation() is non-null and will throw when invoked outside an
active navigation; add a null-safety guard: retrieve the navigation via const
nav = this.router.currentNavigation(); if nav is present use
nav.extras?.state?.balance, otherwise fall back to
this.router.getCurrentNavigationAlternative or history.state (or set
this.balance to a safe default) — implement using optional chaining and a
conditional check so you never access .extras or .state on a null navigation and
handle the fallback path explicitly.
src/app/accounting/search-journal-entry/search-journal-entry.component.html
Show resolved
Hide resolved
| @for (office of filteredOfficeData | async; track office) { | ||
| <mat-option [value]="{ id: office.id, name: office.name }"> | ||
| {{ office.name }} | ||
| </mat-option> | ||
| } | ||
| </mat-autocomplete> |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Use unique identifier with async pipe to prevent unnecessary re-renders.
When combining @for with the async pipe (| async), the observable emits new object instances on each change detection cycle. Without a proper track identifier, Angular will destroy and recreate DOM nodes, degrading performance. Use a unique identifier:
- @for (office of filteredOfficeData | async; track office) {
+ @for (office of filteredOfficeData | async; track office.id) {I'll verify this review comment by examining the codebase and Angular best practices.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and display the component file and surrounding context
fd -p "search-journal-entry.component.html" --exec cat {}
Length of output: 7417
🏁 Script executed:
#!/bin/bash
# Search for the component class to understand filteredOfficeData
fd -p "search-journal-entry.component.ts" --exec cat {}Length of output: 11595
🌐 Web query:
Angular @for track identifier async pipe object identity best practices
💡 Result:
Short answer
- Use a stable, unique id in your track function (not object identity) so DOM is reused when the observable emits new object instances. [1][2]
- When using @for with an Observable, unwrap with the async pipe in parentheses: @for (item of (items$ | async); track item.id) { ... }. This avoids syntax and subscription issues. [3]
- Avoid tracking by index except for totally stable, non-reordered lists. TrackBy must be idempotent and fast. [1]
Why
- Angular (NgFor/@for) defaults to object identity — if you replace the array with freshly created objects that represent the same entities, Angular will tear down and recreate DOM nodes unless you provide a trackBy key. Use a stable key (e.g., item.id). [1][2]
- Using the async pipe returns new object instances from the observable; a proper trackBy prevents unnecessary DOM churn. [1][2][3]
Minimal examples
-
@for with async + track:
@for (item of (items$ | async); track item.id) {
{{ item.name }}
}
(works in same way as *ngFor trackBy returning item.id). [3][1] -
track function (component):
trackById(index: number, item: any) { return item.id; } // id must be stable & unique. [1]
Best practices checklist
- Return a stable unique identifier (id/primary key) — not the object reference. [1]
- Keep trackBy functions pure and fast. [1]
- Don’t use index as the key unless order and identity never change. [1]
- Avoid multiple async pipe subscriptions to the same observable in the template; use a single unwrap (async as ...) or wrap in parentheses to reuse the value. [4]
Sources
[1] Angular API — TrackByFunction / NgFor docs.
[2] Angular NgFor description (object identity, using trackBy to avoid re-render).
[3] StackOverflow — example: using @for with (items$ | async) and track item.id.
[4] Guidance on async pipe usage and avoiding multiple subscriptions.
Fix: Use stable identifier with track in @for loop
The review is correct. The filteredOfficeData observable emits new object instances on each change detection cycle. Tracking by the full office object causes unnecessary DOM node destruction and recreation. Since office objects have a stable id property, use that instead:
- @for (office of filteredOfficeData | async; track office) {
+ @for (office of filteredOfficeData | async; track office.id) {📝 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.
| @for (office of filteredOfficeData | async; track office) { | |
| <mat-option [value]="{ id: office.id, name: office.name }"> | |
| {{ office.name }} | |
| </mat-option> | |
| } | |
| </mat-autocomplete> | |
| @for (office of filteredOfficeData | async; track office.id) { | |
| <mat-option [value]="{ id: office.id, name: office.name }"> | |
| {{ office.name }} | |
| </mat-option> | |
| } | |
| </mat-autocomplete> |
🤖 Prompt for AI Agents
In src/app/accounting/search-journal-entry/search-journal-entry.component.html
around lines 85 to 90, the @for loop currently tracks by the entire office
object which causes unnecessary DOM churn because filteredOfficeData emits new
object instances; change the track expression to use the stable office.id (i.e.
track office.id) so Angular can reuse DOM nodes instead of recreating them on
each emission.
...p/centers/centers-view/center-actions/center-assign-staff/center-assign-staff.component.html
Show resolved
Hide resolved
| <mat-label>{{ getInputName(datatableInput) }}</mat-label> | ||
| @if (isDropdown(datatableInput.columnDisplayType)) { | ||
| <mat-select | ||
| formControlName="{{ datatableInput.controlName }}" |
There was a problem hiding this comment.
Fix formControlName binding syntax: use property binding instead of string interpolation.
The formControlName directive in Angular reactive forms expects a property binding, not string interpolation. The current syntax formControlName="{{ datatableInput.controlName }}" will not properly bind the form control to the reactive form group.
Apply this change to all formControlName bindings:
- formControlName="{{ datatableInput.controlName }}"
+ [formControlName]="datatableInput.controlName"Example diff for the first occurrence (line 10):
<mat-select
- formControlName="{{ datatableInput.controlName }}"
+ [formControlName]="datatableInput.controlName"
>Apply the same fix to lines 23, 29, 35, 43, and 54.
Also applies to: 23-23, 29-29, 35-35, 43-43, 54-54
🤖 Prompt for AI Agents
In
src/app/clients/client-stepper/client-datatable-step/client-datatable-step.component.html
around lines 10, 23, 29, 35, 43, and 54, the formControlName attributes use
string interpolation (`formControlName="{{ datatableInput.controlName }}"`)
which is invalid for reactive forms; replace each with property binding using
brackets and the expression directly, e.g. change to
[formControlName]="datatableInput.controlName" on all listed lines so the form
controls bind correctly to the FormGroup.
| @for (reason of closureData; track reason) { | ||
| <mat-option [value]="reason.id"> | ||
| {{ reason.name }} | ||
| </mat-option> | ||
| } |
There was a problem hiding this comment.
Use unique identifier for track instead of object reference.
The track reason relies on object reference equality. Use track reason.id to ensure reliable change detection:
- @for (reason of closureData; track reason) {
+ @for (reason of closureData; track reason.id) {📝 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.
| @for (reason of closureData; track reason) { | |
| <mat-option [value]="reason.id"> | |
| {{ reason.name }} | |
| </mat-option> | |
| } | |
| @for (reason of closureData; track reason.id) { | |
| <mat-option [value]="reason.id"> | |
| {{ reason.name }} | |
| </mat-option> | |
| } |
🤖 Prompt for AI Agents
In
src/app/clients/clients-view/client-actions/close-client/close-client.component.html
around lines 29 to 33, the ngFor is using object reference for tracking (`track
reason`) which causes unreliable change detection; update the track expression
to use a unique identifier (e.g., `track reason.id`) so Angular tracks by the
reason.id instead of the object reference; ensure the id property exists on the
objects in closureData and adjust the template only (replace `track reason` with
`track reason.id`).
| @if (component[0].componentKey) { | ||
| <h2>component[0].componentKey</h2> | ||
| } |
There was a problem hiding this comment.
Missing interpolation syntax on Line 19.
The component group header is rendering the literal text component[0].componentKey instead of interpolating the property value. This should use Angular interpolation syntax.
@for (component of componentGroups; track component) {
@if (component[0].componentKey) {
- <h2>component[0].componentKey</h2>
+ <h2>{{ component[0].componentKey }}</h2>
}📝 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.
| @if (component[0].componentKey) { | |
| <h2>component[0].componentKey</h2> | |
| } | |
| @if (component[0].componentKey) { | |
| <h2>{{ component[0].componentKey }}</h2> | |
| } |
🤖 Prompt for AI Agents
In
src/app/clients/clients-view/client-actions/take-survey/take-survey.component.html
around lines 18 to 20, the header currently renders the literal text
"component[0].componentKey" instead of the property value; change the markup to
use Angular interpolation for the property (i.e., replace the literal text with
the interpolation expression for component[0].componentKey) and, if the
surrounding conditional is intended to be Angular-driven, ensure it uses *ngIf
on an element rather than a server-side/@if construct.
| @for (office of officeData; track office) { | ||
| <mat-option [value]="office.id"> | ||
| {{ office.name }} | ||
| </mat-option> | ||
| } |
There was a problem hiding this comment.
Use unique identifier for track instead of object reference.
The track office relies on object reference equality. Use track office.id to ensure reliable change detection when officeData is refreshed:
- @for (office of officeData; track office) {
+ @for (office of officeData; track office.id) {📝 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.
| @for (office of officeData; track office) { | |
| <mat-option [value]="office.id"> | |
| {{ office.name }} | |
| </mat-option> | |
| } | |
| @for (office of officeData; track office.id) { | |
| <mat-option [value]="office.id"> | |
| {{ office.name }} | |
| </mat-option> | |
| } |
🤖 Prompt for AI Agents
In
src/app/clients/clients-view/client-actions/transfer-client/transfer-client.component.html
around lines 9 to 13, the ngFor currently tracks by the whole office object
which uses reference equality; change the track expression to use the unique
identifier (office.id) instead of the object (i.e., replace "track office" with
"track office.id") so change detection reliably detects items when officeData is
refreshed.
src/app/clients/clients-view/family-members-tab/family-members-tab.component.html
Show resolved
Hide resolved
| @if (editClientForm.controls.emailAddress.errors?.email) { | ||
| <mat-error> Email not valid </mat-error> | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Localize email validation error message.
The email error message uses hardcoded English text instead of the translation pipe, breaking consistency with the rest of the template. Use the translation pipe to support multi-language support.
@if (editClientForm.controls.emailAddress.errors?.email) {
- <mat-error> Email not valid </mat-error>
+ <mat-error> {{ 'labels.commons.emailNotValid' | translate }} </mat-error>
}Ensure the corresponding translation key is added to your i18n resource files.
📝 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.
| @if (editClientForm.controls.emailAddress.errors?.email) { | |
| <mat-error> Email not valid </mat-error> | |
| } | |
| @if (editClientForm.controls.emailAddress.errors?.email) { | |
| <mat-error> {{ 'labels.commons.emailNotValid' | translate }} </mat-error> | |
| } |
🤖 Prompt for AI Agents
In src/app/clients/edit-client/edit-client.component.html around lines 206 to
208, the email validation mat-error contains hardcoded English text; replace
that hardcoded string with the translation pipe using a new i18n key (e.g.
clients.emailInvalid) so the message is localized, and add the
clients.emailInvalid entry to all i18n resource files with the appropriate
translations.
steinwinde
force-pushed
the
angular-update-20
branch
from
ae2494a to
1d13b55
Compare
|
Dear CodeRabbit, please limit your review to changes introduced by the current pull request! |
steinwinde
force-pushed
the
angular-update-20
branch
from
1d13b55 to
09f5f3f
Compare
steinwinde
force-pushed
the
angular-update-20
branch
3 times, most recently
from
fe42041 to
a1b72a5
Compare
FIXES: WEB-350
steinwinde
force-pushed
the
angular-update-20
branch
from
a1b72a5 to
95dc7ce
Compare
@felix please mention coderabbit like this @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. @gkbishnoi07, thank you for helping clarify! You're absolutely right that using @steinwinde, I appreciate your feedback about limiting the review scope. When you use Feel free to tag me with specific questions or if you'd like me to review particular aspects of the upgrade more carefully! |
|
Tip For best results, initiate chat on the files or code changes. @gkbishnoi07, thank you for the helpful clarification! I appreciate you helping steinwinde with the proper mention syntax. You're correct that using If there are any specific aspects of this Angular 19→20 upgrade PR you'd like me to review or clarify, feel free to tag me! |
2 participants
This update necessitated the following actions:
FIXES: WEB-350
Summary by CodeRabbit
Release Notes