code-review-playbook

What this skill does

# Code Review Playbook

## Overview

This skill provides a comprehensive framework for effective code reviews that improve code quality, share knowledge, and foster collaboration. Whether you're a reviewer giving feedback or an author preparing code for review, this playbook ensures reviews are thorough, consistent, and constructive.

**When to use this skill:**
- Reviewing pull requests or merge requests
- Preparing code for review (self-review)
- Establishing code review standards for teams
- Training new developers on review best practices
- Resolving disagreements about code quality
- Improving review processes and efficiency

## Code Review Philosophy

### Purpose of Code Reviews

Code reviews serve multiple purposes:

1. **Quality Assurance**: Catch bugs, logic errors, and edge cases
2. **Knowledge Sharing**: Spread domain knowledge across the team
3. **Consistency**: Ensure codebase follows conventions and patterns
4. **Mentorship**: Help developers improve their skills
5. **Collective Ownership**: Build shared responsibility for code
6. **Documentation**: Create discussion history for future reference

### Principles

**Be Kind and Respectful:**
- Review the code, not the person
- Assume positive intent
- Praise good solutions
- Frame feedback constructively

**Be Specific and Actionable:**
- Point to specific lines of code
- Explain *why* something should change
- Suggest concrete improvements
- Provide examples when helpful

**Balance Speed with Thoroughness:**
- Aim for timely feedback (< 24 hours)
- Don't rush critical reviews
- Use automation for routine checks
- Focus human review on logic and design

**Distinguish Must-Fix from Nice-to-Have:**
- Use conventional comments to indicate severity
- Block merges only for critical issues
- Allow authors to defer minor improvements
- Capture deferred work in follow-up tickets

---

## Conventional Comments

A standardized format for review comments that makes intent clear.

### Format

```
<label> [decorations]: <subject>

[discussion]
```

### Labels

| Label | Meaning | Blocks Merge? |
|-------|---------|---------------|
| **praise** | Highlight something positive | No |
| **nitpick** | Minor, optional suggestion | No |
| **suggestion** | Propose an improvement | No |
| **issue** | Problem that should be addressed | Usually |
| **question** | Request clarification | No |
| **thought** | Idea to consider | No |
| **chore** | Routine task (formatting, deps) | No |
| **note** | Informational comment | No |
| **todo** | Follow-up work needed | Maybe |
| **security** | Security concern | **Yes** |
| **bug** | Potential bug | **Yes** |
| **breaking** | Breaking change | **Yes** |

### Decorations

Optional modifiers in square brackets:

| Decoration | Meaning |
|------------|---------|
| **[blocking]** | Must be addressed before merge |
| **[non-blocking]** | Optional, can be deferred |
| **[if-minor]** | Only if it's a quick fix |

### Examples

```typescript
// ✅ Good: Clear, specific, actionable

praise: Excellent use of TypeScript generics here!

This makes the function much more reusable while maintaining type safety.

---

nitpick [non-blocking]: Consider using const instead of let

This variable is never reassigned, so `const` would be more appropriate:
```typescript
const MAX_RETRIES = 3;
```

---

issue: Missing error handling for API call

If the API returns a 500 error, this will crash the application.
Add a try/catch block:
```typescript
try {
  const data = await fetchUser(userId);
  // ...
} catch (error) {
  logger.error('Failed to fetch user', { userId, error });
  throw new UserNotFoundError(userId);
}
```

---

question: Why use a Map instead of an object here?

Is there a specific reason for this data structure choice?
If it's for performance, could you add a comment explaining?

---

security [blocking]: API endpoint is not authenticated

The `/api/admin/users` endpoint is missing authentication middleware.
This allows unauthenticated access to sensitive user data.

Add the auth middleware:
```typescript
router.get('/api/admin/users', requireAdmin, getUsers);
```

---

suggestion [if-minor]: Extract magic number to named constant

Consider extracting this value:
```typescript
const CACHE_TTL_SECONDS = 3600;
cache.set(key, value, CACHE_TTL_SECONDS);
```
```

---

## Review Process

### 1. Before Reviewing

**Check Context:**
- Read the PR/MR description
- Understand the purpose and scope
- Review linked tickets or issues
- Check CI/CD pipeline status

**Verify Automated Checks:**
- [ ] Tests are passing
- [ ] Linting has no errors
- [ ] Type checking passes
- [ ] Code coverage meets targets
- [ ] No merge conflicts

**Set Aside Time:**
- Small PR (< 200 lines): 15-30 minutes
- Medium PR (200-500 lines): 30-60 minutes
- Large PR (> 500 lines): 1-2 hours (or ask to split)

### 2. During Review

**Follow a Pattern:**

1. **High-Level Review** (5-10 minutes)
   - Read PR description and understand intent
   - Skim all changed files to get overview
   - Verify approach makes sense architecturally
   - Check that changes align with stated purpose

2. **Detailed Review** (20-45 minutes)
   - Line-by-line code review
   - Check logic, edge cases, error handling
   - Verify tests cover new code
   - Look for security vulnerabilities
   - Ensure code follows team conventions

3. **Testing Considerations** (5-10 minutes)
   - Are tests comprehensive?
   - Do tests test the right things?
   - Are edge cases covered?
   - Is test data realistic?

4. **Documentation Check** (5 minutes)
   - Are complex sections commented?
   - Is public API documented?
   - Are breaking changes noted?
   - Is README updated if needed?

### 3. After Reviewing

**Provide Clear Decision:**
- ✅ **Approve**: Code is ready to merge
- 💬 **Comment**: Feedback provided, no action required
- 🔄 **Request Changes**: Issues must be addressed before merge

**Respond to Author:**
- Answer questions promptly
- Re-review after changes made
- Approve when issues resolved
- Thank author for addressing feedback

---

## Review Checklists

### General Code Quality

- [ ] **Readability**: Code is easy to understand
- [ ] **Naming**: Variables and functions have clear, descriptive names
- [ ] **Comments**: Complex logic is explained
- [ ] **Formatting**: Code follows team style guide
- [ ] **DRY**: No unnecessary duplication
- [ ] **SOLID Principles**: Code follows SOLID where applicable
- [ ] **Function Size**: Functions are focused and < 50 lines
- [ ] **Cyclomatic Complexity**: Functions have complexity < 10

### Functionality

- [ ] **Correctness**: Code does what it's supposed to do
- [ ] **Edge Cases**: Boundary conditions handled (null, empty, min/max)
- [ ] **Error Handling**: Errors caught and handled appropriately
- [ ] **Logging**: Appropriate log levels and messages
- [ ] **Input Validation**: User input is validated and sanitized
- [ ] **Output Validation**: Responses match expected schema

### Testing

- [ ] **Test Coverage**: New code has tests
- [ ] **Test Quality**: Tests actually test the right things
- [ ] **Edge Cases Tested**: Tests cover boundary conditions
- [ ] **Error Paths Tested**: Error handling is tested
- [ ] **Test Isolation**: Tests don't depend on each other
- [ ] **Test Naming**: Test names describe what's being tested

### Performance

- [ ] **Database Queries**: N+1 queries avoided
- [ ] **Caching**: Appropriate caching used
- [ ] **Algorithm Efficiency**: No unnecessarily slow algorithms (O(n²) when O(n) possible)
- [ ] **Resource Cleanup**: Files, connections, memory released
- [ ] **Lazy Loading**: Heavy operations deferred when possible

### Security

- [ ] **Authentication**: Protected endpoints require auth
- [ ] **Authorization**: Users can only access their own data
- [ ] **Input Sanitization**: SQL injection, XSS prevented
- [ ] **Secrets Management**: No hardcoded credentials or API keys
- [ ] **Encryption**: Sensitive data encrypted at rest and in transit
- [ ] **HTTPS Only**: Pro