Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Angular entity list component by converting from traditional lifecycle hooks (ngOnInit) to signal-based reactive patterns. The changes align with Angular's modern reactive programming model, using signals and effects instead of manual subscription management.
Changes:
- Replaced
ngOnInitlifecycle hook with signal-basedeffect()functions in the constructor - Converted route parameter and filter observables to signals using
toSignal() - Removed manual subscription management (no more
ngOnDestroyneeded)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
generators/angular/templates/src/main/webapp/app/entities/_entityFolder_/list/_entityFile_.ts.ejs |
Converted component from ngOnInit pattern to signal-based effects; added toSignal for route state and filter changes; removed OnInit interface and manual subscription |
generators/angular/templates/src/main/webapp/app/entities/_entityFolder_/list/_entityFile_.spec.ts.ejs |
Updated test to use component's isLoading property; added test for request cancellation behavior |
Comments suppressed due to low confidence (2)
generators/angular/templates/src/main/webapp/app/entities/entityFolder/list/entityFile.ts.ejs:173
- The
toSignalcall forfilterChangesshould include aninitialValueoption or userequireSync: trueto ensure it always has a value. Without this, the effect at line 209 could receiveundefinedon its first run if thefilterChangesobservable doesn't emit immediately. Alternatively, add a null check before using the value:const filterOptions = this.filterChanges(); if (filterOptions) { ... }
protected readonly filterChanges = toSignal(this.filters.filterChanges);
generators/angular/templates/src/main/webapp/app/entities/entityFolder/list/entityFile.ts.ejs:38
- The
Subscription,Observable, andfinalizeimports are no longer used after converting from ngOnInit to signal-based effects. These imports should be removed from the rxjs import statement to keep the code clean and avoid unused imports.
import { combineLatest<%_ if (!readOnly) { _%>, filter<%_ } _%>, finalize, Observable, Subscription, map, tap } from 'rxjs';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ators/angular/templates/src/main/webapp/app/entities/_entityFolder_/list/_entityFile_.ts.ejs
Outdated
Show resolved
Hide resolved
7470d35 to
f3a626c
Compare
.../angular/templates/src/main/webapp/app/entities/_entityFolder_/list/_entityFile_.spec.ts.ejs
Show resolved
Hide resolved
...ators/angular/templates/src/main/webapp/app/entities/_entityFolder_/list/_entityFile_.ts.ejs
Show resolved
Hide resolved
| // if (this.<%= entityInstancePlural %>().length === 0) { | ||
| <%_ } _%> | ||
| this.load(); | ||
| <%_ if (paginationNo && searchEngineNo && !jpaMetamodelFiltering) { _%> | ||
| // } |
There was a problem hiding this comment.
Sonar complains about the commented code:
| // if (this.<%= entityInstancePlural %>().length === 0) { | |
| <%_ } _%> | |
| this.load(); | |
| <%_ if (paginationNo && searchEngineNo && !jpaMetamodelFiltering) { _%> | |
| // } | |
| <%_ } _%> | |
| this.load(); | |
| <%_ if (paginationNo && searchEngineNo && !jpaMetamodelFiltering) { _%> |
|
I don’t know why the microfrontends failures, I may revisit this PR in the future. |
Please make sure the below checklist is followed for Pull Requests.
When you are still working on the PR, consider converting it to Draft (below reviewers) and adding
skip-cilabel, you can still see CI build result at your branch.