code-review

name	code-review
description	Best practices for performing thorough, constructive code reviews. Use when reviewing PRs to ensure quality feedback that improves code and helps developers grow.

Code Review Skill

Guidelines for performing effective code reviews that catch issues, improve code quality, and maintain a positive team dynamic.

Review Mindset

Bias Towards Shipping

The primary goal is to get working code merged, not to achieve perfection.

Before requesting changes, always ask yourself:

"Is this feedback worth an additional review cycle?"

Most feedback should be:

• Approved with suggestions for the author to consider
• Comments for future reference, not blocking
• Follow-up issues for later improvement

Reserve "Request Changes" for actual problems:

• Security vulnerabilities
• Bugs that will cause runtime errors
• Breaking changes to existing functionality
• Missing critical requirements

Do NOT block for:

• Style preferences (unless egregiously inconsistent)
• "I would have done it differently"
• Minor optimizations that don't matter at current scale
• Missing tests for edge cases that are unlikely
• Refactoring opportunities (create a follow-up issue instead)

Goals of Code Review

1. Catch real bugs before they reach production
2. Prevent security issues - the things that actually matter
3. Share knowledge across the team
4. Ship working software - don't let perfect be the enemy of good

The Right Attitude

• You're reviewing the code, not the person
• Assume good intent - the author tried their best
• Be a collaborator, not a gatekeeper
• Your job is to help ship good code, not to find fault
• Velocity matters - every review cycle has a cost

Review Process

1. Understand the Context First

Before looking at code:

1. Read the PR description
2. Read the linked issue
3. Understand WHAT is being done and WHY
4. Consider: Is this the right approach?

2. First Pass: High-Level Review

Ask yourself:

• Does this solve the problem described in the issue?
• Is the approach reasonable?
• Are there obvious architectural concerns?
• Is anything missing?

3. Second Pass: Detailed Review

Look at each file for:

• Correctness (does it work?)
• Edge cases and error handling
• Security implications
• Performance concerns
• Test coverage
• Code style and readability

4. Summarize Your Review

End with an overall assessment:

**[Architect]** Overall this looks good. Clean implementation of the user registration flow.

A few suggestions:
1. Consider adding rate limiting (security)
2. The validation error messages could be more user-friendly
3. Minor: prefer `const` over `let` where possible

Approving with minor suggestions - none are blocking.

What to Look For

Correctness

• Does the code do what it's supposed to do?
• Are there logic errors?
• Are edge cases handled?
• What happens with null/undefined/empty inputs?

**Concern:** This will throw if `user` is null:
\`\`\`javascript
const name = user.profile.name;
\`\`\`

Consider:
\`\`\`javascript
const name = user?.profile?.name ?? 'Unknown';
\`\`\`

Error Handling

• Are errors caught appropriately?
• Are error messages helpful?
• Does the error handling match the project patterns?

**Suggestion:** The error is swallowed here - the user won't know what went wrong:
\`\`\`javascript
try {
  await saveUser(data);
} catch (e) {
  console.log(e);
}
\`\`\`

Consider surfacing this to the user or rethrowing.

Security (OWASP Top 10)

• SQL injection (parameterized queries?)
• XSS (output encoding?)
• Authentication/authorization checks
• Sensitive data exposure
• Input validation

**Security Issue:** User input is interpolated directly into the query:
\`\`\`javascript
const query = `SELECT * FROM users WHERE email = '${email}'`;
\`\`\`

Use parameterized queries to prevent SQL injection.

Performance

• Unnecessary loops or iterations
• N+1 query problems
• Missing indexes
• Large payloads
• Memory leaks

**Performance:** This creates an N+1 query problem - one query per user:
\`\`\`javascript
for (const user of users) {
  user.posts = await getPosts(user.id);
}
\`\`\`

Consider using a batch query or join.

Maintainability

• Is the code readable?
• Are names descriptive?
• Is the logic easy to follow?
• Will this be easy to modify later?

**Readability:** This function does a lot - consider splitting:
- Validation logic → `validateUserInput()`
- Transformation → `transformUserData()`
- Persistence → `saveUser()`

This would make each piece easier to test and understand.

Tests

• Are tests included?
• Do tests cover the main paths?
• Do tests cover edge cases?
• Are tests readable and maintainable?

**Testing:** Good coverage of the happy path! Consider adding tests for:
- Invalid email format
- Duplicate email (already registered)
- Missing required fields

Consistency

• Does it follow project conventions?
• Does it match existing patterns?
• Is it consistent with itself?

**Consistency:** The rest of the codebase uses `async/await`, but this uses `.then()`:
\`\`\`javascript
fetchUser(id).then(user => { ... });
\`\`\`

Consider using async/await for consistency.

How to Give Feedback

Be Specific and Actionable

Bad:

This code is confusing.

Good:

The nested callbacks make this hard to follow. Consider using async/await or extracting the inner logic into named functions.

Explain the Why

Bad:

Use const here.

Good:

Use const here since user is never reassigned. This signals intent to readers and catches accidental reassignment.

Offer Solutions

Bad:

This won't scale.

Good:

This loads all users into memory, which won't scale. Consider:

1. Pagination with limit/offset
2. Streaming with cursor-based pagination
3. If the use case allows, a count query instead

Distinguish Severity

Use prefixes to indicate importance:

Prefix	Meaning	Frequency
Blocker:	Must fix before merge - security/bugs only	Rare
Suggestion:	Would improve the code, not blocking	Common
Nit:	Minor style/preference, definitely not blocking	Occasional
Question:	Seeking to understand	As needed
Praise:	Something done well	Often!

Important: Most comments should be Suggestions or Nits, not Blockers. If you find yourself writing many Blockers, reconsider whether they truly block shipping.

**Blocker:** This SQL query is vulnerable to injection. (Security - must fix)

**Suggestion:** Consider extracting this into a helper function for reuse. (Not blocking - can be done later)

**Nit:** Extra blank line here. (Definitely not blocking)

**Question:** Why did you choose this approach over X? (Curious, not blocking)

**Praise:** Really clean error handling here! (Always welcome)

Ask Questions Instead of Demanding

Demanding:

Change this to use a Map instead of an object.

Collaborative:

Have you considered using a Map here? It might give better performance for frequent lookups. What do you think?

Common Review Scenarios

When You'd Do It Differently

Don't block for preference. Ask yourself:

• Is their way wrong, or just different?
• Does it work correctly?
• Is it maintainable?

If it's just different:

**Note:** I might have used X approach here, but this works well too. Not blocking.

When Something Is Missing

**Missing:** I don't see error handling for the case where the API returns 404. What should happen?

When You Don't Understand

**Question:** I'm not following the logic in this section. Could you add a comment explaining the business rule, or walk me through it?

When Praising Good Work

Don't just point out problems:

**Praise:** Nice job extracting this into a reusable hook! This will help with the other forms too.

**Praise:** The test coverage here is excellent.

Review Response Templates

Default to Approve. Most reviews should approve, possibly with suggestions.

Approve (Most Common)

**[Architect]** LGTM! Clean implementation, good test coverage.

Minor suggestions (not blocking):
- Line 42: prefer const
- Consider adding a comment explaining the retry logic

Merging as-is is fine. Address these if you agree, or not - your call.

Approve with Suggestions

**[Architect]** Approving - this is solid work.

A few things to consider (can address now or in follow-up):
1. The validation could be more specific about what's wrong
2. Consider adding logging for debugging

Not blocking merge. Ship it!

Request Changes (Rare - Use Sparingly)

Only use for genuine blockers: security issues, bugs, or missing critical functionality.

**[Architect]** Good progress! Found one issue that needs fixing before merge:

**Blocker:**
1. SQL injection vulnerability in the search query - this is a security risk

Everything else looks good. Happy to re-review once the security fix is in.

Ask yourself before requesting changes:

• Will this cause a production incident if shipped?
• Is this a security vulnerability?
• Does it break existing functionality?

If the answer to all three is "no", consider approving with suggestions instead.

Decline (N/A)

**[UI/UX]** N/A - No UI changes in this PR.

Reviewed: Backend/infrastructure changes only, no user-facing impact.

Review Etiquette

Do

• Review promptly (within 24 hours ideally)
• Be thorough but not pedantic
• Acknowledge good work
• Offer to discuss complex issues in person/call
• Re-review quickly after changes

Don't

• Nitpick excessively
• Bike-shed on minor style issues
• Block for preferences
• Be condescending
• Leave reviews hanging

Self-Review Checklist

Before requesting review, authors should self-review:

• I've re-read my own diff
• Tests pass locally
• No console.logs or debug code left
• No commented-out code
• Variable names are descriptive
• Complex logic has comments
• Error cases are handled
• No obvious security issues