LNK-4673: update admin UI to angular21 - #1384
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughUpdated Angular framework and dependencies from version 20.x to 21.x, including Angular Material and Font Awesome 4. Migrated template syntax across multiple components from structural directives ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 7
🤖 Fix all issues with AI agents
In
`@Web/Admin.UI/src/app/components/data-acquisition/data-acquisition-fhir-query-config-form/data-acquisition-fhir-query-config-form.component.html`:
- Around line 236-251: The password input currently shows plain text; update the
<input> element bound to formControlName "password" in the
DataAcquisitionFhirQueryConfigForm component so it uses a password mask by
adding type="password" (while preserving [readonly]="viewOnly",
formControlName="password", placeholder and existing clearPassword() behavior);
ensure the matSuffix clear button logic (related to passwordControl and
clearPassword()) and the mat-error condition using authTypeControl remain
unchanged so validation and read-only behavior continue to work.
- Around line 160-165: The mat-error blocks are shown whenever
authTypeControl.value != 'None' && authTypeControl.value != 'Basic' regardless
of the individual control's validity; update each error's *ngIf to require both
the authType condition AND that the specific control is invalid (and optionally
touched/dirty) so errors only show on actual validation failures. For example,
change the Auth Key mat-error to check authTypeControl.value != 'None' &&
authTypeControl.value != 'Basic' && authKeyControl.invalid &&
(authKeyControl.touched || authKeyControl.dirty), and apply the same pattern for
tokenUrlControl, audienceControl, clientIdControl, userNameControl and
passwordControl.
In
`@Web/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.html`:
- Around line 130-151: The table elements in the template are invalidly placed
inside a <div>; wrap the table structure properly by replacing the surrounding
<div class="input-container"> with a <table class="input-container"> (or add a
<table> inside the div) so the <thead>, <tr>, <th>, and <td> are children of a
<table>; keep the existing iteration and bindings (the `@if`(`@for`) blocks,
patients array usage and the removePatient(i) click handler) unchanged but
ensure the markup is a valid table parent for those elements.
In
`@Web/Admin.UI/src/app/components/reports/reports-dashboard/reports-dashboard.component.html`:
- Around line 106-114: The orphaned ng-template named `#noResultsFoundTemplate`
isn't rendered because the Angular 21 control-flow `@if` block using
reportListSummary closes without an `@else`; update the `@if` that checks
reportListSummary to include an `@else` branch that displays the
noResultsFoundTemplate (reference the existing template id
`#noResultsFoundTemplate`) so that when reportListSummary is empty/undefined the
fallback message is shown.
In
`@Web/Admin.UI/src/app/components/tenant/facility-view/view-report/view-report.component.html`:
- Around line 299-302: Remove the duplicated word in the loading message inside
the view-report component template: locate the span in
view-report.component.html that currently reads "Loading report summary
summary." and change it to "Loading report summary." so the loading state
displays a single correct phrase.
- Around line 1-29: The template adds two control-flow branches around
reportSummary and reportSummary.submitted; add tests to cover both branches: for
the Angular side, add/extend tests for ViewReportComponent to render with
reportSummary = null/undefined (assert nav links and action buttons are
absent/loader state) and with reportSummary present where submitted = false
(assert Acquisition Log button visible, Download button absent) and submitted =
true (assert both Acquisition Log and Download buttons present), using
fixture.componentInstance.reportSummary assignment, fixture.detectChanges(), and
query selectors for the buttons and links; for server/view-model logic covered
by XUnit, add tests that exercise the code paths which set the view model's
reportSummary and submitted flags (asserting the expected values emitted to the
view), so both loading vs content and submitted vs not-submitted branches are
covered.
In
`@Web/Admin.UI/src/app/components/testing/integration-test/integration-test.component.html`:
- Around line 104-126: The table headers render Trace ID and Error Message
conditionally when any error exists, but rows skip those <td> cells for items
without item.ErrorMessage, causing column misalignment; update the row rendering
in integration-test.component.html so that whenever the header condition
(presence of any error) is true you always emit the two <td> cells for TraceId
and ErrorMessage: for TraceId render either {{ item.TraceId }} or an empty cell
when item.ErrorMessage is falsy, and for ErrorMessage render the
<mat-expansion-panel> with content when item.ErrorMessage exists or render an
empty <td> (or a hidden/disabled panel) when it does not; keep the existing
bindings/isPanelOpen and togglePanel(CorrelationId) logic and the slice + length
checks only inside the panel content so structural alignment is preserved.
🧹 Nitpick comments (9)
Web/Admin.UI/src/app/components/reports/reports-dashboard/reports-dashboard.component.html (2)
37-37: Track by unique identifier for better change detection.Tracking by object reference (
track report) can cause unnecessary DOM re-renders when the list is refreshed from an API, as new object instances are created even if the data is unchanged. Sincereport.idis available and unique, use it for tracking.♻️ Proposed fix
- `@for` (report of reportListSummary; track report) { + `@for` (report of reportListSummary; track report.id) {
72-75: Consider using strict equality.The conditions use loose equality (
==) for boolean comparisons. While this works, strict equality (===) is preferred for clarity and to avoid unexpected type coercion.♻️ Suggested change
<span [ngClass]="{ - 'status-pending': report.submitted == false, - 'status-submitted': report.submitted == true + 'status-pending': report.submitted === false, + 'status-submitted': report.submitted === true }">{{ report.submitted ? 'Submitted' : 'Pending' }}</span>Web/Admin.UI/src/app/components/data-acquisition/data-acquisition-fhir-query-config-form/data-acquisition-fhir-query-config-form.component.html (1)
151-166: Consider consistent disabled/readonly usage.The template mixes
[disabled](lines 12, 35, 61, etc.) and[readonly](lines 154, 171, 188, etc.) for view-only mode. While both prevent editing:
disabled: Excludes the field from form submission and valuereadonly: Field is still submitted and included in form valueIf this is intentional (auth fields should be included in form value even in view-only mode), this is fine. Otherwise, consider using a consistent approach across all fields.
Web/Admin.UI/src/app/components/kafka/kafka-dashboard/kafka-dashboard.component.html (1)
3-12: Consider using@elsefor mutually exclusive conditions.The current implementation works, but Angular 21's control flow supports
@elsefor cleaner mutual exclusion:♻️ Optional: Use `@else` syntax
- `@if` (kafkaUrl) { - <p> - <a href="{{kafkaUrl}}" target="_blank">Kafka UI</a> - </p> - } - `@if` (!kafkaUrl) { - <p> - <span>Default Kafka UI is not available.</span> - </p> - } + `@if` (kafkaUrl) { + <p> + <a href="{{kafkaUrl}}" target="_blank">Kafka UI</a> + </p> + } `@else` { + <p> + <span>Default Kafka UI is not available.</span> + </p> + }Web/Admin.UI/src/app/components/reports/generate-report/generate-report-form.component.html (1)
143-147: Redundant condition inside@forloop.The
@if (patients.length > 0)check is unnecessary here. If thepatientsarray were empty, the@forloop wouldn't iterate and this code would never execute.♻️ Remove redundant check
<td class="header-cell"> - `@if` (patients.length > 0) { <button mat-icon-button color="warn" (click)="removePatient(i)"> <mat-icon>remove_circle</mat-icon> </button> - } </td>Web/Admin.UI/src/main.ts (1)
1-12: Correct Angular 21 bootstrap configuration.Adding
provideZoneChangeDetection()toapplicationProvidersis the recommended approach for configuring zone-based change detection in Angular 21.Consider enabling
eventCoalescingfor potential performance improvements in change detection:💡 Optional: Enable event coalescing for performance
-platformBrowserDynamic().bootstrapModule(AppModule, { applicationProviders: [provideZoneChangeDetection()], }) +platformBrowserDynamic().bootstrapModule(AppModule, { applicationProviders: [provideZoneChangeDetection({ eventCoalescing: true })], })Event coalescing batches multiple change detection cycles triggered by rapid events into fewer cycles, which can improve performance.
Web/Admin.UI/src/app/components/testing/integration-test/integration-test.component.html (3)
28-32: Use stable tracking keys for@forloops to avoid DOM churn.
track fac,track entry, andtrack itemrely on object identity. With async streams or rebuilt arrays, Angular will recreate DOM nodes and can reset expansion-panel state. Prefer stable identifiers (e.g.,facilityId, topic,CorrelationId) if they’re unique.♻️ Proposed change
- `@for` (fac of filteredFacilities | async; track fac) { + `@for` (fac of filteredFacilities | async; track fac.facilityId) { ... - `@for` (entry of displayedEntries; track entry) { + `@for` (entry of displayedEntries; track entry[0]) { ... - `@for` (item of entry[1]; track item; let i = $index) { + `@for` (item of entry[1]; track item.CorrelationId; let i = $index) {Also applies to: 89-92
80-85: CachehasAnyError(displayedEntries)instead of recomputing in the template.The method is invoked repeatedly (including inside loops). If it scans entries, this adds avoidable work each change detection. Consider precomputing a boolean in the component (e.g.,
hasDisplayedErrors) and binding to it.💡 Example template update
- `@if` (hasAnyError(displayedEntries)) { + `@if` (hasDisplayedErrors) { ... - `@if` (hasAnyError(displayedEntries)) { + `@if` (hasDisplayedErrors) {Also applies to: 134-138
51-66: Add XUnit coverage for the new UI branches.The new
@if/@else branches (loading vs. button, and conditional forms) should have tests covering each branch (e.g.,isLoadingtrue/false,showReportScheduledForm/showPatientsAcquiredForm). Mock any external calls. As per coding guidelines.Also applies to: 152-166
arianamihailescu
changed the title
2 participants
🛠️ Description of Changes
Upgrade to Angular 21
🧪 Testing Performed
Tested locally
🧑🔬 Unit Testing
📓 Documentation Updated
Please update any relevant sections in the project documentation that were impacted by the changes in the PR.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.