code-review-playbook

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

// ✅ 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:

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:

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:

router.get('/api/admin/users', requireAdmin, getUsers);

---

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

Consider extracting this value:

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)
2. Read PR description and understand intent
3. Skim all changed files to get overview
4. Verify approach makes sense architecturally
5. Check that changes align with stated purpose

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

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

1. Documentation Check (5 minutes)
2. Are complex sections commented?
3. Is public API documented?
4. Are breaking changes noted?
5. 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: Prod