How to review Angular pull requests: the 9 defect classes with examples
Angular gives a reviewer a lot of structure to lean on, and its bugs hide in the lifecycle: an input read before it is set, a signal read outside the effect that should track it, or a sanitizer switched off for convenience. This guide walks DiffDojo's nine defect classes as they appear in Angular components and services.
1. Security
Angular escapes bindings by default, so the risk is code that opts out: bypassSecurityTrustHtml on content a user wrote, or secrets placed in environment.ts, which is compiled into the browser bundle. Treat every bypassSecurityTrust* call as a question about who wrote the content, and every key in environment.ts as public.
2. Requirement fidelity
Compare the template with the ticket: the columns shown, the sort order, the disabled states. A component that renders cleanly but leaves out a required control is still the wrong change.
3. Edge cases & error handling
Look at bounds and empty states: an index clamped against the wrong array, a list template with no empty case, an HTTP call with no error handling in the subscription or the signal. These pass a demo with tidy data and fail on the first empty list or failed request, so check the template's @empty block and the error path.
4. Hallucinated APIs & dependencies
A misspelled lifecycle hook such as ngOnInIt is a plain method Angular never calls; the compiler only notices if the class declares implements OnInit. Check hook names, decorator options and RxJS operators you do not recognise.
5. State & data flow
Inputs are not set when the constructor runs, so values computed there use defaults; ngOnInit sees only the first value, so derive anything that must follow the input with a signal input() (Angular 17.1+) and computed, or ngOnChanges. With signals, a value read outside the effect or computed that should track it does not trigger an update.
- Inputs aren't set in the constructor: url is computed from empty values
- body-only edits don't reliably autosave: body() is read outside effect's tracking
6. Performance
Watch for a function called in a template binding that recomputes on every change detection, an @for whose track expression is not stable (a freshly built object, say), and subscriptions created in a loop or never torn down. The cost grows with list size or with how often components are created and destroyed, so it often stays hidden in a small demo.
7. Data correctness
Pipes and formatters hide arithmetic: rounding prices with floats, formatting dates in the browser's time zone when the spec needs UTC, or truncating instead of rounding. Read the spec's rounding rule and time zone, then check which ones the pipe or formatter actually applies.
8. Deceptive tests
A TestBed test that mocks the service under test, or never calls detectChanges() before asserting on the template, can pass while the component is broken. Ask whether the test would fail if the component's logic were deleted; if not, it is testing the mock.
9. Over-engineering
A state store, facades and selectors for one component's local state is a review comment. Signals or a component field are often enough.
The checklist
- Does any code bypass the sanitizer, or ship a secret in the bundle?
- Does the template show exactly what the ticket asks?
- Are bounds, empty lists and HTTP errors handled?
- Are hook names, decorators and operators real?
- Are inputs read after they are set, and signals read inside their effect?
- Any function calls in bindings, unstable track expressions, or leaked subscriptions?
- Is arithmetic and date formatting done as the spec says?
- Does each test run change detection and test the real service?
- Is the state as simple as the feature?
As of October 2026, the DiffDojo library has 47 live Angular PRs to practise on, each graded on whether you catch the planted bug or approve the clean one.
Practise
Review an Angular PR with the answer hidden.
Leave inline comments and a verdict, then see what you caught. Free, no signup.
Open the Angular library →