A code review checklist is not a bureaucratic checklist. It’s accumulated team knowledge, compiled into a repeatable process so reviewers don’t have to remember everything from first principles every time.

This is the TechShop Angular 21 review checklist — the patterns that get flagged, why they get flagged, and the ESLint rules that catch them automatically so you don’t have to.

The Forbidden Patterns

1. Structural Directives (use control flow instead)

<!-- ❌ Old structural directives -->
<div *ngIf="isLoggedIn">...</div>
<li *ngFor="let item of items; trackBy: trackById">...</li>
<div [ngSwitch]="status">
  <span *ngSwitchCase="'active'">Active</span>
</div>

<!-- ✅ Angular 17+ control flow -->
@if (isLoggedIn()) {
  <div>...</div>
}
@for (item of items(); track item.id) {
  <li>...</li>
}
@switch (status()) {
  @case ('active') { <span>Active</span> }
}

Why: New control flow is type-narrowing aware, faster to compile, and the track expression in @for is mandatory (prevents silent performance bugs).

2. Constructor Injection (use inject())

// ❌ Constructor injection
@Component({...})
export class ProductListComponent {
  constructor(
    private store: ProductStore,
    private router: Router,
    private analyticsService: AnalyticsService
  ) {}
}

// ✅ inject() function
@Component({...})
export class ProductListComponent {
  private readonly store = inject(ProductStore);
  private readonly router = inject(Router);
  private readonly analytics = inject(AnalyticsService);
}

Why: inject() works with inheritance correctly, is compatible with functional patterns (interceptors, guards), and allows injection context to be shared via utility functions.

3. Missing ChangeDetectionStrategy.OnPush

// ❌ Default change detection — entire component tree checked on every event
@Component({
  selector: 'app-product-card',
  template: `...`,
})
export class ProductCardComponent {}

// ✅ OnPush — only checks when inputs change or signals notify
@Component({
  selector: 'app-product-card',
  changeDetection: ChangeDetectionStrategy.OnPush,
  template: `...`,
})
export class ProductCardComponent {}

Why: In a product catalog with 50 cards, default CD means 50 components fire change detection on every mouse move. OnPush restricts this to actual data changes.

4. any Type

// ❌ any — defeats TypeScript's entire purpose
const handleResponse = (data: any) => {
  this.products.set(data.items); // no type checking, runtime crashes possible
};

// ✅ Explicit type or unknown with narrowing
const handleResponse = (data: ProductListResponse) => {
  this.products.set(data.items);
};

// ✅ When type is truly unknown
const handleEvent = (payload: unknown) => {
  if (isProductUpdate(payload)) {
    this.updateProduct(payload);
  }
};

5. subscribe() Without Cleanup

// ❌ Memory leak — subscription lives until the app dies
ngOnInit() {
  this.priceUpdates$.subscribe(price => this.currentPrice.set(price));
}

// ✅ Option 1: takeUntilDestroyed()
private destroyRef = inject(DestroyRef);

ngOnInit() {
  this.priceUpdates$.pipe(
    takeUntilDestroyed(this.destroyRef)
  ).subscribe(price => this.currentPrice.set(price));
}

// ✅ Option 2: toSignal() (preferred when you just need the value)
currentPrice = toSignal(this.priceUpdates$, { initialValue: 0 });

6. Business Logic in Components

// ❌ Business logic in component
@Component({...})
export class CartComponent {
  calculateDiscount(item: CartItem): number {
    // Discount logic that belongs in a service
    if (item.qty >= 10) return item.price * 0.15;
    if (item.qty >= 5) return item.price * 0.10;
    return 0;
  }
}

// ✅ Logic in service, component just binds
@Component({...})
export class CartComponent {
  private cart = inject(CartService);
  
  // Component only formats for display
  discount = computed(() => this.cart.calculateDiscount(this.item()));
}

7. Decorator-based Inputs/Outputs

// ❌ Old decorator-based (still works but deprecated path)
@Input() product!: Product;
@Output() addToCart = new EventEmitter<Product>();

// ✅ Signal-based (Angular 17.1+)
product = input.required<Product>();
addToCart = output<Product>();

// ✅ Two-way with model()
quantity = model(1);

8. Missing track in @for

<!-- ❌ No track — Angular destroys and recreates all DOM on each change -->
@for (product of products()) {
  <app-product-card [product]="product" />
}

<!-- ✅ Track by stable ID -->
@for (product of products(); track product.id) {
  <app-product-card [product]="product" />
}

The Quick-Reference Review Card

Pattern Found	Verdict	Fix
*ngIf, *ngFor, *ngSwitch	❌ Block	Use @if, @for, @switch
Constructor injection	❌ Block	Use inject()
any type	❌ Block	Explicit type or unknown
Missing OnPush on new component	❌ Block	Add changeDetection: OnPush
subscribe() without cleanup	❌ Block	takeUntilDestroyed() or toSignal()
Business logic in component class	❌ Block	Move to service/store
Missing track in @for	❌ Block	Add track item.id
@Input() / @Output() decorators	⚠️ Comment	Suggest input() / output()
BehaviorSubject for state	⚠️ Comment	Suggest Signal service
No NgOptimizedImage on <img>	⚠️ Comment	Suggest migration
Inline styles on component	⚠️ Comment	Move to component CSS
Missing data-testid attributes	⚠️ Comment	Add for Playwright selectors

ESLint Rules That Automate the Checklist

Add these to catch violations before code review:

// eslint.config.mjs
export default [
  {
    plugins: { '@angular-eslint': angularEslint },
    rules: {
      // Forbid old structural directives
      '@angular-eslint/template/no-negated-async': 'error',

      // Require OnPush
      '@angular-eslint/prefer-on-push-component-change-detection': 'error',

      // Forbid @Input/@Output decorators in favor of signal inputs/outputs
      '@angular-eslint/prefer-signals': 'warn',

      // Require standalone components (no NgModules)
      '@angular-eslint/component-max-inline-declarations': 'error',

      // TypeScript strict — catches any leakage
      '@typescript-eslint/no-explicit-any': 'error',
      '@typescript-eslint/no-unsafe-assignment': 'error',
      '@typescript-eslint/no-unsafe-member-access': 'error',
    }
  }
];

How to Give Structured Review Feedback

Don’t do this:

“This is wrong, you need to use OnPush”

Do this:

“This component is missing changeDetection: ChangeDetectionStrategy.OnPush. In a list of 50 product cards, default CD means this component is checked on every user interaction. Even a mouse move triggers it. Adding OnPush restricts checks to when Signal reads change or inputs update. Here’s the one-line fix: [link to convention doc]”

The difference: the second version teaches the pattern, not just corrects the code. After 3–4 of these, the developer never makes the mistake again.

For repeated violations:

1. First occurrence: explain in comments
2. Second occurrence: add an ESLint rule that automates the check
3. There should never be a third — the linter now catches it at save time

The “LGTM” Rule

Never approve a PR with LGTM (Looks Good To Me) without actually reviewing every changed file. The biggest source of degradation in codebases is approvals that were LGTM’d but not actually reviewed.

Tech Lead minimum bar on every PR:

1. Read the description — does the PR match what the ticket asked for?
2. Check nx graph — did this PR introduce any new dependency edges?
3. Grep for any, subscribe(, constructor( — quick scan for common violations
4. Check test coverage delta — did coverage go up or down?
5. Approve only when all blockers are resolved

---

References

• Angular Style Guide — angular.dev
• Angular 21 Control Flow — angular.dev
• @angular-eslint — GitHub
• DestroyRef + takeUntilDestroyed
• Signal Inputs — angular.dev

---

Part of the Angular Tech Lead Series — Back to main series overview