Review

Code Review

What to Look For

Review in this priority order:

1. Correctness (highest priority)

• Does the code do what the PR description claims?
• Are there off-by-one errors, null/undefined cases, race conditions?
• Does it handle errors properly?
• Are edge cases covered (empty arrays, zero, negative, very large)?
• Does it break existing behavior?

2. Security

• Is user input validated at trust boundaries?
• Are queries parameterized (no SQL injection)?
• Is output encoded (no XSS)?
• Are secrets absent from code and logs?
• Are authentication/authorization checks in place?
• Are dependencies up to date and trusted?

3. Performance

• Are there N+1 queries?
• Is pagination used for list endpoints?
• Are loops doing synchronous I/O?
• Any unnecessary re-computation or re-renders?
• Are caches invalidated correctly?

4. Maintainability

• Does the code match existing patterns in the codebase?
• Are names clear and specific?
• Is complexity proportional to the problem?
• Are comments explaining WHY (not WHAT)?
• Is there duplicated logic that should be extracted?

5. Testing

• Are there tests for the new behavior?
• Do tests actually test behavior, not implementation details?
• Are both happy path AND error cases covered?
• Do existing tests still pass?

6. Style (lowest priority, automated)

• Let linters/formatters handle this
• Don't nitpick on whitespace or import order in review comments

How to Review Systematically

1. Read the PR description -- what is this trying to do?
2. Check the test diff first -- does the tested behavior match the description?
3. Read the implementation diff
4. Run the code locally if the change is significant
5. Check CI status -- what's passing/failing?
6. Consider: how would this break in production?
7. Write comments grouped by severity

Giving Good Feedback

Categories of Comments

Tag	Meaning	Example
blocking:	Must fix before merge	blocking: SQL injection in line 42
needs-discussion:	Design concern worth debating	needs-discussion: should we use JWT or sessions?
suggestion:	Optional improvement	suggestion: this could use Array.flatMap
nit:	Trivial / stylistic	nit: typo in comment
question:	Asking to understand	question: why did we choose this approach?
praise:	Call out good work	praise: nice error handling here

What Makes a Good Comment

blocking: SQL injection risk in `getUserByEmail`

The query uses string concatenation:

db.query(SELECT * FROM users WHERE email = '${email}')

Use parameterized queries instead:

db.query('SELECT * FROM users WHERE email = $1', [email])

This prevents attackers from injecting SQL via crafted email values.

Good comments:

• State the problem
• Explain the impact
• Suggest a specific fix
• Link to docs or examples if helpful

Bad comments:

• "This is wrong" (no explanation)
• "I don't like this" (not actionable)
• Demanding changes without explaining why
• Restating what the code does

Self-Review Before Submitting

Before marking your own PR ready:

Check	Why
Re-read your own diff	Catches obvious issues
Run the feature end-to-end	Verifies it actually works
Remove debug logs, commented code, TODOs	Clean up
Update or add tests	Behavior without tests is untested
Update docs if behavior changed	Future-you will forget
Check for secrets in diff	Last chance to catch them
Write a clear PR description	Reviewers need context

PR Description Template

## Summary
One sentence: what does this PR do?

## Why
The motivation / context / ticket link.

## Changes
- Bullet list of key changes
- Include new dependencies
- Flag breaking changes

## Testing
How was this tested? Include screenshots/logs for UI/output changes.

## Notes for Reviewer
Anything non-obvious? Trade-offs made? Open questions?

Reviewing Others' PRs

Before leaving comments:

1. Read the comments already left -- don't duplicate
2. Check CI status -- is the author already fixing things?
3. Check how far behind the base branch -- outdated PRs may need rebase first
4. Assume good intent -- ask questions before assuming mistakes

Anti-Patterns

Anti-Pattern	Problem	Do Instead
Rubber-stamping without reading	Bugs ship to production	Actually review the code
Reviewing only lines shown in diff	Miss broken callers	Check related files too
Nit-picking style	Wastes time, demoralizes	Let linters handle style
Blocking on opinion disagreements	Gridlock	Distinguish blocking from suggestion
Vague comments ("this is bad")	Author can't act	State problem + impact + fix
Approving while CI is red	Merges broken code	Wait for CI green
Reviewing while tired	Miss obvious bugs	Batch reviews, review fresh