# Review Pr

> Review a pull request against project conventions and Angular 22+ best practices

- Skill: `sap/review-pr` (Agent Skill)
- Install (CLI): `npx skillmds@latest add sap/review-pr`
- Raw SKILL.md: https://api.skillmd.com/api/skills/sap/review-pr/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools, Web & Frontend
- Author: sap (https://skillmd.com/u/sap)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/sap/review-pr

---


# PR Review: #$0

If `$0` is empty or not a number, ask the user for a PR number before proceeding.

## Context

Fetch the PR details:

- Diff: !`gh pr diff $0`
- PR info: !`gh pr view $0`
- Changed files: !`gh pr diff $0 --name-only`

## Review Checklist

For each changed file, check the applicable sections below. Report findings grouped by severity: **Blocking** (must fix), **Suggestion** (should fix), **Nit** (optional).

### 1. Angular 22+ Patterns

- [ ] **New code** uses `input()` / `output()` / `model()` / `linkedSignal()`. Existing `@Input()` / `@Output()` decorators being modified should prefer migration to signal functions, but it's not blocking.
- [ ] `host: {}` in decorator — no `@HostBinding()` / `@HostListener()`
- [ ] `@if` / `@for` / `@switch` — no `*ngIf` / `*ngFor` / `*ngSwitch`
- [ ] No `standalone: true` in `@Component` (default since Angular 19)
- [ ] No `allowSignalWrites` option in `effect()` (the option no longer exists)
- [ ] `DestroyRef` + `takeUntilDestroyed()` — no custom `DestroyedService`
- [ ] `computed()` + `host: { '[class]': }` — no `CssClassBuilder` / `@applyCssClass`
- [ ] Member ordering: decorated props → signal inputs/outputs → public → protected → private → constructor → methods

### 2. State Management

- [ ] `signal()` only used when a reactive consumer exists (template, computed, effect, host binding)
- [ ] Plain properties for internal bookkeeping, one-time flags, cached values
- [ ] No redundant `markForCheck()` after signal updates
- [ ] `BehaviorSubject` → `signal()` where there are no async consumers
- [ ] `effect()` for signal reactions; RxJS only for async operations (HTTP, WebSocket, timers)
- [ ] No `effect()` used for state derivation — use `computed()` or `linkedSignal` instead
- [ ] `linkedSignal` used where mutable derived state is needed (not `effect()` + `signal.set()`)
- [ ] No object/array mutation in place then `signal.set()` with same reference — always new references
- [ ] No conditional signal reads creating invisible dependency gaps in `effect()` / `computed()`

### 3. Dependency Injection

- [ ] `InjectionToken` for contextual defaults (not `@ContentChild` assigning to signal inputs)
- [ ] Token defined near child component, injected with `{ optional: true }`
- [ ] `FD_` prefix for component identity tokens
- [ ] `contentChild()` / `contentChildren()` query by token, not concrete class

### 4. Unit Tests

- [ ] **New/changed inputs, outputs, or behavior have corresponding test updates**
- [ ] Tests cover user interactions and realistic scenarios, not implementation details
- [ ] No tests for TypeScript-prevented scenarios (e.g. passing undefined to required input)
- [ ] No tests for Angular DI-prevented scenarios (e.g. missing required dependency)
- [ ] `fixture.componentRef.setInput()` used for signal inputs in tests
- [ ] Individual component imports in test `imports` arrays — no deprecated `*Module` classes
- [ ] Unique test component names (no generic `TestComponent`)

### 5. Documentation Examples

- [ ] **If public API changed (new input, changed behavior, removed feature), docs examples are updated**
- [ ] Examples consolidated into existing examples — no unnecessary standalone example files
- [ ] Every demonstrated feature is user-observable (can the user see it working?)
- [ ] `@fundamental-styles/common-css` utility classes (`sap-flex`, `sap-margin-*`, `sap-padding-*`) — no inline styles
- [ ] Individual component imports in examples — no deprecated `*Module` classes

### 6. Breaking Changes

- [ ] **If exports removed, inputs/outputs renamed, or defaults changed:**
    - [ ] Commit message has `!` after scope: `fix(core)!: description`
    - [ ] Commit message has `BREAKING CHANGE:` footer with migration instructions
    - [ ] PR description has a "Breaking Changes" section with before/after examples
- [ ] Widely-used APIs marked `@deprecated` before removal (not removed in same PR)
- [ ] All usages searched with grep before removing exports

### 7. Commit & PR Format

- [ ] Commit format: `<type>(<scope>): <subject>`
- [ ] Valid type: `feat` | `fix` | `docs` | `style` | `refactor` | `test` | `build` | `ci` | `chore`
- [ ] Valid scope: `core` | `platform` | `cdk` | `btp` | `cx` | `i18n` | `datetime-adapter` | `ui5` | `docs` | `e2e` | `ci`
- [ ] PR title follows same format
- [ ] No `WIP` prefix (unless intentionally draft)

## Output Format

Summarize findings as:

```
## Review Summary

**PR:** #$0
**Overall:** APPROVE / REQUEST CHANGES / COMMENT

### Blocking
- [file:line] Issue description

### Suggestions
- [file:line] Issue description

### Nits
- [file:line] Issue description

### Missing
- Tests: list any untested new behavior
- Docs: list any undocumented API changes
- Breaking changes: list any unannounced breaking changes
```