fargo
1a822493ba
963 markdown files reformatted with the repository's pinned prettier so pnpm format:check covers the folded tree like every other repo file. The formatter's embedded-language pass also normalized code fences (TS semicolons, closed HTML tags in examples, lowercased CSS hex colors, one renumbered list that skipped an index). Alphanumeric token deltas vs the fold commit were audited file-by-file; all are formatter-equivalent markup normalizations plus the four sanitized skills.
3.9 KiB
3.9 KiB
Code Review Best Practices
Comprehensive guidelines for conducting effective code reviews.
Review Philosophy
Goals of Code Review
Primary Goals:
- Catch bugs and edge cases before production
- Ensure code maintainability and readability
- Share knowledge across the team
- Enforce coding standards consistently
- Improve design and architecture decisions
Secondary Goals:
- Mentor junior developers
- Build team culture and trust
- Document design decisions through discussions
What Code Review is NOT
- A gatekeeping mechanism to block progress
- An opportunity to show off knowledge
- A place to nitpick formatting (use linters)
- A way to rewrite code to personal preference
Review Timing
When to Review
| Trigger | Action |
|---|---|
| PR opened | Review within 24 hours, ideally same day |
| Changes requested | Re-review within 4 hours |
| Blocking issue found | Communicate immediately |
Time Allocation
- Small PR (<100 lines): 10-15 minutes
- Medium PR (100-400 lines): 20-40 minutes
- Large PR (>400 lines): Request to split, or 60+ minutes
Review Depth Levels
Level 1: Skim Review (5 minutes)
- Check PR description and linked issues
- Verify CI/CD status
- Look at file changes overview
- Identify if deeper review needed
Level 2: Standard Review (20-30 minutes)
- Full code walkthrough
- Logic verification
- Test coverage check
- Security scan
Level 3: Deep Review (60+ minutes)
- Architecture evaluation
- Performance analysis
- Security audit
- Edge case exploration
Communication Guidelines
Tone and Language
Use collaborative language:
- "What do you think about..." instead of "You should..."
- "Could we consider..." instead of "This is wrong"
- "I'm curious about..." instead of "Why didn't you..."
Be specific and actionable:
- Include code examples when suggesting changes
- Link to documentation or past discussions
- Explain the "why" behind suggestions
Handling Disagreements
- Seek to understand: Ask clarifying questions
- Acknowledge valid points: Show you've considered their perspective
- Provide data: Use benchmarks, docs, or examples
- Escalate if needed: Involve senior dev or architect
- Know when to let go: Not every hill is worth dying on
Review Prioritization
Must Fix (Blocking)
- Security vulnerabilities
- Data corruption risks
- Breaking changes without migration
- Critical performance issues
- Missing error handling for user-facing features
Should Fix (Important)
- Test coverage gaps
- Moderate performance concerns
- Code duplication
- Unclear naming or structure
- Missing documentation for complex logic
Nice to Have (Non-blocking)
- Style preferences beyond linting
- Minor optimizations
- Additional test cases
- Documentation improvements
Anti-Patterns to Avoid
Reviewer Anti-Patterns
- Rubber stamping: Approving without actually reviewing
- Bike shedding: Debating trivial details extensively
- Scope creep: "While you're at it, can you also..."
- Ghosting: Requesting changes then disappearing
- Perfectionism: Blocking for minor style preferences
Author Anti-Patterns
- Mega PRs: Submitting 1000+ line changes
- No context: Missing PR description or linked issues
- Defensive responses: Arguing every suggestion
- Silent updates: Making changes without responding to comments
Metrics and Improvement
Track These Metrics
- Time to first review
- Review cycle time
- Number of review rounds
- Defect escape rate
- Review coverage percentage
Continuous Improvement
- Hold retrospectives on review process
- Share learnings from escaped bugs
- Update checklists based on common issues
- Celebrate good reviews and catches