code-review-checklist

Code Review Checklist

Pre-review (always start here)

• [ ] Read PR description and linked issue — understand the why
• [ ] Check CI passes before spending time on review
• [ ] Pull branch locally if logic is complex

Functionality

• [ ] Solves stated problem and meets acceptance criteria
• [ ] Edge cases: null/empty inputs, concurrent calls, network failure
• [ ] Error handling: errors caught, message doesn't expose internals
• [ ] No off-by-one, loop termination, or race conditions

Security (block if any fail)

• [ ] No SQL injection — use parameterized queries, not string concat
• [ ] No XSS — escape all user-controlled output in DOM
• [ ] No hardcoded secrets — use environment variables
• [ ] Authentication required on all protected routes
• [ ] Authorization checks presence AND ownership (not just auth)
• [ ] File uploads validated: type, size, content

// ❌ SQL injection
const q = `SELECT * FROM users WHERE email = '${email}'`;

// ✅ Parameterized
db.query("SELECT * FROM users WHERE email = $1", [email]);

// ❌ Hardcoded secret
const KEY = "sk_live_abc123";

// ✅ Env variable
const KEY = process.env.API_KEY;
if (!KEY) throw new Error("API_KEY is required");

Performance

• [ ] No N+1 queries — check ORM calls inside loops
• [ ] Database queries use indexes for filter/sort columns
• [ ] No unbounded queries — always paginate or limit
• [ ] No blocking main thread with sync I/O (Node.js)
• [ ] Caching used for repeated expensive operations

Code quality

• [ ] Names describe intent (calculateTotalPrice not calc)
• [ ] Functions have single responsibility (< ~30 lines is a signal)
• [ ] No dead code or commented-out blocks
• [ ] DRY — no copy-paste of more than 3 lines
• [ ] Follows existing project conventions and patterns
• [ ] Abstractions are deep enough to justify themselves; thin pass-through wrappers fail the deletion test

Tests

• [ ] New behavior has test coverage
• [ ] Happy path + at least 1 failure/edge case tested
• [ ] Tests use real assertions, not just "doesn't throw"
• [ ] No brittle tests that break on unrelated changes

Documentation

• [ ] Complex logic has // why comment (not // what)
• [ ] Public API changes documented
• [ ] Breaking changes documented in CHANGELOG or PR body

Review comment format

**Issue:** [What's wrong]
**Current:** `problematic code`
**Suggested:** `improved code`
**Why:** [reason]

Verdict

• APPROVED — all sections pass
• APPROVED WITH CONDITIONS — minor items, non-blocking
• CHANGES REQUIRED — blocking security, correctness, or test coverage issues

Output: checklist score (X/Y passing) + blocking items with file:line refs + verdict