Develop#1526
Conversation
📝 WalkthroughWalkthroughAdds a functional CityCardComponent that fetches and syncs city data via FakeHttpService and CityStore, introduces CardModel and CardService for centralized card add/delete logic, refactors CardComponent to use an image input and delegate additions to CardService, and updates ListItemComponent to support city deletion. Student/teacher card components gain image bindings. ChangesCard feature implementation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CityCardComponent
participant FakeHttpService
participant CityStore
CityCardComponent->>FakeHttpService: fetch cities
FakeHttpService-->>CityCardComponent: emit city data
CityCardComponent->>CityStore: subscribe to cities$
CityStore-->>CityCardComponent: emit updated cities
CityCardComponent->>CityCardComponent: update cities array
sequenceDiagram
participant CardComponent
participant CardService
participant TeacherStore
participant StudentStore
participant CityStore
CardComponent->>CardService: addOne(type)
alt type is TEACHER
CardService->>TeacherStore: add generated teacher
else type is STUDENT
CardService->>StudentStore: add generated student
else type is CITY
CardService->>CityStore: add generated city
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/angular/projection/src/app/component/city-card/city-card.component.tsOops! Something went wrong! :( ESLint: 8.48.0 TypeError: Error while loading rule ' apps/angular/projection/src/app/component/student-card/student-card.component.tsOops! Something went wrong! :( ESLint: 8.48.0 TypeError: Error while loading rule ' apps/angular/projection/src/app/component/teacher-card/teacher-card.component.tsOops! Something went wrong! :( ESLint: 8.48.0 TypeError: Error while loading rule '
Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/angular/projection/src/app/component/city-card/city-card.component.ts (1)
25-34: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSubscriptions in
ngOnInitare never cleaned up.Both
fetchCities$andstore.cities$are subscribed to withouttakeUntilDestroyed()/take(1)/asyncpipe.store.cities$is backed by a singleton store, so if this component is destroyed and recreated (e.g., route navigation), the subscription persists and keeps reassigningthis.citieson a destroyed instance — a memory leak and a source of stale updates.♻️ Suggested fix
+import { DestroyRef, inject } from '`@angular/core`'; +import { takeUntilDestroyed } from '`@angular/core/rxjs-interop`'; ... ngOnInit(): void { - this.http.fetchCities$.subscribe((s) => this.store.addAll(s)); - - this.store.cities$.subscribe((s) => (this.cities = s)); + this.http.fetchCities$ + .pipe(takeUntilDestroyed(this.destroyRef)) + .subscribe((s) => this.store.addAll(s)); + + this.store.cities$ + .pipe(takeUntilDestroyed(this.destroyRef)) + .subscribe((s) => (this.cities = s)); }Alternatively, prefer the
asyncpipe in the template forstore.cities$to avoid manual subscription management entirely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/angular/projection/src/app/component/city-card/city-card.component.ts` around lines 25 - 34, The `CityCardComponent.ngOnInit` subscriptions to `fetchCities$` and `store.cities$` are never torn down, so update them to use Angular-safe cleanup such as `takeUntilDestroyed()` (or replace the `store.cities$` subscription with `async` in the template). Keep the existing logic in `CityCardComponent`, `FakeHttpService.fetchCities$`, and `CityStore`, but ensure `this.cities` is only assigned while the component is alive and the one-time city fetch subscription completes or auto-unsubscribes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/angular/projection/src/app/component/city-card/city-card.component.ts`:
- Around line 11-15: `CityCardComponent` is reusing the `bg-light-red`
customClass from `TeacherCardComponent`, which makes the city card look
identical to the teacher card. Update the `app-card` usage in
`CityCardComponent` to use a city-specific class instead of `bg-light-red`,
following the pattern used by `StudentCardComponent` and the other card
components so each card type remains visually distinct.
In `@apps/angular/projection/src/app/ui/card/card.component.ts`:
- Around line 37-47: The CardComponent input `list` is still declared as
`any[]`, which removes the type safety introduced by `CardModel` and leaves
template accesses like `item.name` and `item.firstName` unchecked. Update the
`CardComponent` `list` input to use `CardModel[]` and, since the binding is
required, switch it to Angular’s `@Input({ required: true })` instead of relying
on the non-null assertion. Keep the existing `CardComponent` API and
`CardService` usage unchanged.
- Around line 13-32: The card template in card.component.ts should be simplified
and made accessible: add an alt attribute to the img element (use the card’s
type to describe it), and remove the unnecessary `#cardContent` ng-template
wrapper since the content is rendered immediately via ngTemplateOutlet. Update
the template structure so the image, list, and Add button live in the direct
card content instead of being wrapped in an extra template layer.
---
Nitpick comments:
In `@apps/angular/projection/src/app/component/city-card/city-card.component.ts`:
- Around line 25-34: The `CityCardComponent.ngOnInit` subscriptions to
`fetchCities$` and `store.cities$` are never torn down, so update them to use
Angular-safe cleanup such as `takeUntilDestroyed()` (or replace the
`store.cities$` subscription with `async` in the template). Keep the existing
logic in `CityCardComponent`, `FakeHttpService.fetchCities$`, and `CityStore`,
but ensure `this.cities` is only assigned while the component is alive and the
one-time city fetch subscription completes or auto-unsubscribes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ac1f4a1-0366-4fe5-b2c2-64fe1d994f25
📒 Files selected for processing (7)
apps/angular/projection/src/app/component/city-card/city-card.component.tsapps/angular/projection/src/app/component/student-card/student-card.component.tsapps/angular/projection/src/app/component/teacher-card/teacher-card.component.tsapps/angular/projection/src/app/model/card.model.tsapps/angular/projection/src/app/services/card.service.tsapps/angular/projection/src/app/ui/card/card.component.tsapps/angular/projection/src/app/ui/list-item/list-item.component.ts
| <app-card | ||
| [list]="cities" | ||
| [type]="cardType" | ||
| [image]="image" | ||
| customClass="bg-light-red"></app-card> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reused bg-light-red class from TeacherCardComponent.
Per the graph context, TeacherCardComponent already uses customClass="bg-light-red". StudentCardComponent uses a distinct bg-light-green. Reusing bg-light-red for CityCardComponent makes city and teacher cards visually indistinguishable — likely a copy/paste oversight rather than an intentional shared style.
🎨 Suggested fix
<app-card
[list]="cities"
[type]="cardType"
[image]="image"
- customClass="bg-light-red"></app-card>
+ customClass="bg-light-blue"></app-card>📝 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.
| <app-card | |
| [list]="cities" | |
| [type]="cardType" | |
| [image]="image" | |
| customClass="bg-light-red"></app-card> | |
| <app-card | |
| [list]="cities" | |
| [type]="cardType" | |
| [image]="image" | |
| customClass="bg-light-blue"></app-card> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/angular/projection/src/app/component/city-card/city-card.component.ts`
around lines 11 - 15, `CityCardComponent` is reusing the `bg-light-red`
customClass from `TeacherCardComponent`, which makes the city card look
identical to the teacher card. Update the `app-card` usage in
`CityCardComponent` to use a city-specific class instead of `bg-light-red`,
following the pattern used by `StudentCardComponent` and the other card
components so each card type remains visually distinct.
| <ng-template #cardContent> | ||
| <img src="assets/img/{{ image }}" width="200px" /> | ||
|
|
||
| <section> | ||
| <app-list-item | ||
| *ngFor="let item of list" | ||
| [name]="item.firstName" | ||
| [id]="item.id" | ||
| [type]="type"></app-list-item> | ||
| </section> | ||
| <section> | ||
| <app-list-item | ||
| *ngFor="let item of list" | ||
| [name]="item.firstName ?? item.name" | ||
| [id]="item.id" | ||
| [type]="type"></app-list-item> | ||
| </section> | ||
|
|
||
| <button | ||
| class="rounded-sm border border-blue-500 bg-blue-300 p-2" | ||
| (click)="addNewItem()"> | ||
| Add | ||
| </button> | ||
| <button | ||
| class="rounded-sm border border-blue-500 bg-blue-300 p-2" | ||
| (click)="addNewItem()"> | ||
| Add | ||
| </button> | ||
| </ng-template> | ||
|
|
||
| <ng-container *ngTemplateOutlet="cardContent"></ng-container> | ||
| </div> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target component and nearby context
git ls-files apps/angular/projection/src/app/ui/card
echo "--- card.component.ts ---"
cat -n apps/angular/projection/src/app/ui/card/card.component.ts
echo "--- search for ng-content / ngTemplateOutlet in projection app ---"
rg -n "ng-content|ngTemplateOutlet|cardContent|CardType|alt=" apps/angular/projection/src/app -SRepository: tomalaforge/angular-challenges
Length of output: 4295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- usages of app-card ---"
rg -n "<app-card|selector: 'app-card'|selector: \"app-card\"" apps/angular/projection/src/app -S
echo "--- card-related component files ---"
git ls-files apps/angular/projection/src/app/component apps/angular/projection/src/app/ui
echo "--- card component and likely callers ---"
sed -n '1,220p' apps/angular/projection/src/app/component/city-card/city-card.component.ts
printf '\n---\n'
sed -n '1,220p' apps/angular/projection/src/app/component/student-card/student-card.component.ts
printf '\n---\n'
sed -n '1,220p' apps/angular/projection/src/app/component/teacher-card/teacher-card.component.tsRepository: tomalaforge/angular-challenges
Length of output: 4184
Add an alt attribute and remove the extra template wrapper
- The card image needs an
altattribute for screen readers, e.g.alt="{{ type }} card". #cardContentis rendered immediately via*ngTemplateOutlet, so the extrang-templatelayer can be inlined unless projected content is coming later.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/angular/projection/src/app/ui/card/card.component.ts` around lines 13 -
32, The card template in card.component.ts should be simplified and made
accessible: add an alt attribute to the img element (use the card’s type to
describe it), and remove the unnecessary `#cardContent` ng-template wrapper since
the content is rendered immediately via ngTemplateOutlet. Update the template
structure so the image, list, and Add button live in the direct card content
instead of being wrapped in an extra template layer.
| export class CardComponent { | ||
| @Input() list: any[] | null = null; | ||
| @Input() list!: any[]; | ||
| @Input() type!: CardType; | ||
| @Input() customClass = ''; | ||
| @Input() image: string = ''; | ||
|
|
||
| CardType = CardType; | ||
|
|
||
| constructor( | ||
| private teacherStore: TeacherStore, | ||
| private studentStore: StudentStore, | ||
| ) {} | ||
| constructor(private cardService: CardService) {} | ||
|
|
||
| addNewItem() { | ||
| if (this.type === CardType.TEACHER) { | ||
| this.teacherStore.addOne(randTeacher()); | ||
| } else if (this.type === CardType.STUDENT) { | ||
| this.studentStore.addOne(randStudent()); | ||
| } | ||
| this.cardService.addOne(this.type); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use CardModel[] instead of any[] for list.
list is typed any[], discarding the type safety CardModel (name, firstName, lastName, subject, id) was introduced for. Accesses like item.firstName, item.name, item.id in the template go unchecked. Since the input is now effectively required (non-null asserted), consider also using Angular's @Input({ required: true }) (supported since v16, confirmed for this project's Angular 17.1.0) so the compiler enforces the binding instead of relying solely on the ! assertion.
♻️ Proposed fix
+import { CardModel } from '../../model/card.model';
...
- `@Input`() list!: any[];
+ `@Input`({ required: true }) list!: CardModel[];📝 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.
| export class CardComponent { | |
| @Input() list: any[] | null = null; | |
| @Input() list!: any[]; | |
| @Input() type!: CardType; | |
| @Input() customClass = ''; | |
| @Input() image: string = ''; | |
| CardType = CardType; | |
| constructor( | |
| private teacherStore: TeacherStore, | |
| private studentStore: StudentStore, | |
| ) {} | |
| constructor(private cardService: CardService) {} | |
| addNewItem() { | |
| if (this.type === CardType.TEACHER) { | |
| this.teacherStore.addOne(randTeacher()); | |
| } else if (this.type === CardType.STUDENT) { | |
| this.studentStore.addOne(randStudent()); | |
| } | |
| this.cardService.addOne(this.type); | |
| } | |
| import { CardModel } from '../../model/card.model'; | |
| export class CardComponent { | |
| `@Input`({ required: true }) list!: CardModel[]; | |
| `@Input`() type!: CardType; | |
| `@Input`() customClass = ''; | |
| `@Input`() image: string = ''; | |
| constructor(private cardService: CardService) {} | |
| addNewItem() { | |
| this.cardService.addOne(this.type); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/angular/projection/src/app/ui/card/card.component.ts` around lines 37 -
47, The CardComponent input `list` is still declared as `any[]`, which removes
the type safety introduced by `CardModel` and leaves template accesses like
`item.name` and `item.firstName` unchecked. Update the `CardComponent` `list`
input to use `CardModel[]` and, since the binding is required, switch it to
Angular’s `@Input({ required: true })` instead of relying on the non-null
assertion. Keep the existing `CardComponent` API and `CardService` usage
unchanged.
1 participant
✅ Challenge Submission Checklist
Start your PR title with: Answer:${challenge_number}
If you would like personal feedback or a detailed review, please support the project on GitHub:
👉 https://github.com/sponsors/tomalaforge
You can also submit a PR without sponsorship to:
Summary by CodeRabbit
New Features
Bug Fixes