Review Checklist

Use a consistent checklist so every review covers the same ground: correctness, security, performance, readability, tests.

Why

Code review without a checklist is inconsistent — one reviewer focuses on style, another on performance, and correctness is missed because nobody checked it. A checklist ensures every review covers the same critical areas: does the code do what it claims (correctness), does it handle edge cases (robustness), is it secure (no injection, no secrets), is it tested (coverage of the new logic), is it readable (will the next developer understand it). The checklist is not a rigid form to fill out; it is a reminder of the areas that are easy to forget. The most experienced reviewers benefit the most, because experience creates blind spots — the senior reviewer who has never been bitten by a SQL injection assumes it is handled, and the checklist reminds them to check. A shared checklist also makes reviews consistent across the team, so the quality does not depend on who is available to review.

When

On every pull request, as the shared standard for the team. Review the checklist itself periodically to add items the team keeps missing.

How

1. Correctness: does the code do what it claims?
   - Does the implementation match the issue/PR description?
   - Are the edge cases handled (null, empty, max, min, error)?
   - Are the assumptions documented and valid?
   - Trace through the logic with a concrete example

2. Security: is the code safe?
   - Input validation: is all external input validated and sanitized?
   - Authentication/authorization: are the checks correct and on every path?
   - Secrets: are there hardcoded keys, passwords, or tokens?
   - Injection: are queries parameterized, is HTML escaped?
   - Dependencies: are new packages vetted for known vulnerabilities?

3. Performance: is the code efficient?
   - Algorithm: is the complexity appropriate (O(n) vs O(n²))?
   - Database: are there N+1 queries, missing indexes, unbounded results?
   - Memory: are large objects held longer than needed?
   - Concurrency: are there race conditions, deadlocks?

4. Readability: will the next developer understand it?
   - Naming: do names describe what, not how?
   - Structure: is the function short, single-purpose, at the right abstraction?
   - Comments: do they explain why, not what?
   - Consistency: does it follow the codebase's conventions?

5. Tests: is the code tested?
   - Coverage: are the new lines of logic tested?
   - Edge cases: are the boundary conditions tested?
   - Failure paths: are the error cases tested, not just the happy path?
   - Test quality: do the tests assert the right thing, or just that no error was thrown?

Example

// Review checklist (paste in the PR template)
/*
## Code Review Checklist

### Correctness
- [ ] Implementation matches the issue/PR description
- [ ] Edge cases handled (null, empty, max, min, error)
- [ ] Assumptions documented and valid
- [ ] Traced through the logic with a concrete example

### Security
- [ ] All external input validated and sanitized
- [ ] Auth checks correct and on every path
- [ ] No hardcoded secrets (keys, passwords, tokens)
- [ ] Queries parameterized, HTML escaped
- [ ] New dependencies vetted for vulnerabilities

### Performance
- [ ] Algorithm complexity appropriate
- [ ] No N+1 queries, missing indexes, unbounded results
- [ ] No large objects held longer than needed
- [ ] No race conditions or deadlocks

### Readability
- [ ] Names describe what, not how
- [ ] Functions short, single-purpose, right abstraction
- [ ] Comments explain why, not what
- [ ] Follows codebase conventions

### Tests
- [ ] New logic lines tested
- [ ] Edge cases tested
- [ ] Error paths tested (not just happy path)
- [ ] Tests assert the right thing
*/

// Example review comment using the checklist:
// "Security: the userId comes from req.query but is not validated.
//  An attacker could pass userId=admin. Add:
//  const userId = z.string().uuid().parse(req.query.userId);"
//
// "Tests: the happy path is tested, but the error case
//  (user not found) is not. Add a test for the 404 path."

Common Problems

⚠Reviewing only the diff and missing the context — the new function looks correct in isolation, but it is called from a path that does not check authorization, and the reviewer did not see the call site.
⚠Checking every box but not actually tracing the logic — the checklist is marked complete, but the reviewer did not run the code or trace through the edge cases, and a null pointer exception ships.
⚠Using the checklist as a gate to fill out rather than a tool to think — the reviewer marks every box "checked" in 30 seconds, and the review provides no value beyond the checklist.
⚠Adding items to the checklist but never removing obsolete ones — the checklist grows to 50 items, nobody reads it, and it becomes a ceremony rather than a useful tool.

Best Practices

✓Use a shared checklist as a reminder of areas to cover, not a form to fill — the value is in the thinking it prompts, not the boxes it checks.
✓Read the surrounding code, not just the diff — the new function may be correct in isolation but wrong in the context of how it is called.
✓Trace through the logic with a concrete example, especially the edge cases — this catches bugs that reading the code does not, because the code looks right but does not handle the null case.
✓Keep the checklist short (5-10 items per area) and remove items the team consistently gets right — a 50-item checklist is ignored; a 10-item checklist is used.