|
| 1 | +--- |
| 2 | +name: code-review-skill |
| 3 | +description: | |
| 4 | + Provides comprehensive code review guidance for React 19, Vue 3, Angular 17+, Svelte 5, |
| 5 | + Rust, TypeScript, Java, PHP, Python, Django, FastAPI, Go, C#/.NET, Kotlin, Swift, |
| 6 | + NestJS, C/C++, Zig, CSS/Less/Sass, Qt, and more. |
| 7 | + Covers architecture review, performance review, security audit, code quality anti-patterns, |
| 8 | + and common bugs across all ecosystems. |
| 9 | + Use when: reviewing pull requests, conducting PR reviews, code review, reviewing code changes, |
| 10 | + establishing review standards, mentoring developers, architecture reviews, security audits, |
| 11 | + performance reviews, checking code quality, finding bugs, giving feedback on code. |
| 12 | +allowed-tools: |
| 13 | + - Read |
| 14 | + - Grep |
| 15 | + - Glob |
| 16 | + - Bash # Run lint/test/build commands to verify code quality |
| 17 | + - WebFetch # Look up the latest documentation and best practices |
| 18 | +--- |
| 19 | + |
| 20 | +# Code Review Skill |
| 21 | + |
| 22 | +Transform code reviews from gatekeeping to knowledge sharing through constructive feedback, systematic analysis, and collaborative improvement. |
| 23 | + |
| 24 | +## When to Use This Skill |
| 25 | + |
| 26 | +- Reviewing pull requests and code changes |
| 27 | +- Establishing code review standards for teams |
| 28 | +- Mentoring junior developers through reviews |
| 29 | +- Conducting architecture reviews |
| 30 | +- Creating review checklists and guidelines |
| 31 | +- Improving team collaboration |
| 32 | +- Reducing code review cycle time |
| 33 | +- Maintaining code quality standards |
| 34 | + |
| 35 | +## Core Principles |
| 36 | + |
| 37 | +### 1. The Review Mindset |
| 38 | + |
| 39 | +**Goals of Code Review:** |
| 40 | +- Catch bugs and edge cases |
| 41 | +- Ensure code maintainability |
| 42 | +- Share knowledge across team |
| 43 | +- Enforce coding standards |
| 44 | +- Improve design and architecture |
| 45 | +- Build team culture |
| 46 | + |
| 47 | +**Not the Goals:** |
| 48 | +- Show off knowledge |
| 49 | +- Nitpick formatting (use linters) |
| 50 | +- Block progress unnecessarily |
| 51 | +- Rewrite to your preference |
| 52 | + |
| 53 | +### 2. Effective Feedback |
| 54 | + |
| 55 | +**Good Feedback is:** |
| 56 | +- Specific and actionable |
| 57 | +- Educational, not judgmental |
| 58 | +- Focused on the code, not the person |
| 59 | +- Balanced (praise good work too) |
| 60 | +- Prioritized (critical vs nice-to-have) |
| 61 | + |
| 62 | +```markdown |
| 63 | +❌ Bad: "This is wrong." |
| 64 | +✅ Good: "This could cause a race condition when multiple users |
| 65 | + access simultaneously. Consider using a mutex here." |
| 66 | + |
| 67 | +❌ Bad: "Why didn't you use X pattern?" |
| 68 | +✅ Good: "Have you considered the Repository pattern? It would |
| 69 | + make this easier to test. Here's an example: [link]" |
| 70 | + |
| 71 | +❌ Bad: "Rename this variable." |
| 72 | +✅ Good: "[nit] Consider `userCount` instead of `uc` for |
| 73 | + clarity. Not blocking if you prefer to keep it." |
| 74 | +``` |
| 75 | + |
| 76 | +### 3. Review Scope |
| 77 | + |
| 78 | +**What to Review:** |
| 79 | +- Logic correctness and edge cases |
| 80 | +- Security vulnerabilities |
| 81 | +- Performance implications |
| 82 | +- Test coverage and quality |
| 83 | +- Error handling |
| 84 | +- Documentation and comments |
| 85 | +- API design and naming |
| 86 | +- Architectural fit |
| 87 | + |
| 88 | +**What Not to Review Manually:** |
| 89 | +- Code formatting (use Prettier, Black, etc.) |
| 90 | +- Import organization |
| 91 | +- Linting violations |
| 92 | +- Simple typos |
| 93 | + |
| 94 | +## Review Process |
| 95 | + |
| 96 | +### Phase 1: Context Gathering (2-3 minutes) |
| 97 | + |
| 98 | +Before diving into code, understand: |
| 99 | +1. Read PR description and linked issue |
| 100 | +2. Check PR size (>400 lines? Ask to split) |
| 101 | +3. Review CI/CD status (tests passing?) |
| 102 | +4. Understand the business requirement |
| 103 | +5. Note any relevant architectural decisions |
| 104 | + |
| 105 | +> For large diffs, pipe the diff through [`scripts/pr-analyzer.py`](scripts/pr-analyzer.py) (`git diff main...HEAD | python scripts/pr-analyzer.py`) to triage complexity and get a suggested review approach before reading. |
| 106 | +
|
| 107 | +### Phase 2: High-Level Review (5-10 minutes) |
| 108 | + |
| 109 | +1. **Architecture & Design** - Does the solution fit the problem? |
| 110 | + - For significant changes, consult [Architecture Review Guide](reference/architecture-review-guide.md) |
| 111 | + - Check: SOLID principles, coupling/cohesion, anti-patterns |
| 112 | +2. **Performance Assessment** - Are there performance concerns? |
| 113 | + - For performance-critical code, consult [Performance Review Guide](reference/performance-review-guide.md) |
| 114 | + - Check: Algorithm complexity, N+1 queries, memory usage |
| 115 | +3. **File Organization** - Are new files in the right places? |
| 116 | +4. **Testing Strategy** - Are there tests covering edge cases? |
| 117 | + |
| 118 | +### Phase 3: Line-by-Line Review (10-20 minutes) |
| 119 | + |
| 120 | +For each file, check: |
| 121 | +- **Logic & Correctness** - Edge cases, off-by-one, null checks, race conditions |
| 122 | +- **Security** - Input validation, injection risks, XSS, sensitive data |
| 123 | +- **Performance** - N+1 queries, unnecessary loops, memory leaks |
| 124 | +- **Maintainability** - Clear names, single responsibility, comments |
| 125 | +- **Reuse** - Before accepting new code, search for existing utilities/helpers that could replace it. Check adjacent files and shared modules for similar patterns. See [Universal Quality Guide](reference/code-quality-universal.md) for anti-patterns like parameter sprawl, leaky abstractions, nested conditionals, stringly-typed code, TOCTOU, and no-op updates. |
| 126 | + |
| 127 | +### Phase 4: Summary & Decision (2-3 minutes) |
| 128 | + |
| 129 | +1. Summarize key concerns |
| 130 | +2. Highlight what you liked |
| 131 | +3. Make clear decision: |
| 132 | + - ✅ Approve |
| 133 | + - 💬 Comment (minor suggestions) |
| 134 | + - 🔄 Request Changes (must address) |
| 135 | +4. Offer to pair if complex |
| 136 | + |
| 137 | +## Review Techniques |
| 138 | + |
| 139 | +### Technique 1: The Checklist Method |
| 140 | + |
| 141 | +Use checklists for consistent reviews. See [Security Review Guide](reference/security-review-guide.md) for comprehensive security checklist. |
| 142 | + |
| 143 | +### Technique 2: The Question Approach |
| 144 | + |
| 145 | +Instead of stating problems, ask questions: |
| 146 | + |
| 147 | +```markdown |
| 148 | +❌ "This will fail if the list is empty." |
| 149 | +✅ "What happens if `items` is an empty array?" |
| 150 | + |
| 151 | +❌ "You need error handling here." |
| 152 | +✅ "How should this behave if the API call fails?" |
| 153 | +``` |
| 154 | + |
| 155 | +### Technique 3: Suggest, Don't Command |
| 156 | + |
| 157 | +Use collaborative language: |
| 158 | + |
| 159 | +```markdown |
| 160 | +❌ "You must change this to use async/await" |
| 161 | +✅ "Suggestion: async/await might make this more readable. What do you think?" |
| 162 | + |
| 163 | +❌ "Extract this into a function" |
| 164 | +✅ "This logic appears in 3 places. Would it make sense to extract it?" |
| 165 | +``` |
| 166 | + |
| 167 | +### Technique 4: Differentiate Severity |
| 168 | + |
| 169 | +Use labels to indicate priority: |
| 170 | + |
| 171 | +- 🔴 `[blocking]` - Must fix before merge |
| 172 | +- 🟡 `[important]` - Should fix, discuss if disagree |
| 173 | +- 🟢 `[nit]` - Nice to have, not blocking |
| 174 | +- 💡 `[suggestion]` - Alternative approach to consider |
| 175 | +- 📚 `[learning]` - Educational comment, no action needed |
| 176 | +- 🎉 `[praise]` - Good work, keep it up! |
| 177 | + |
| 178 | +**Severity levels:** 🔴 / 🟡 / 🟢 are the three severity tiers used as the standard across all guides in this skill — 🔴 blocks the merge, 🟡 should be addressed, 🟢 is optional. The remaining markers (💡 / 📚 / 🎉) are non-blocking annotations. |
| 179 | + |
| 180 | +## Language-Specific Guides |
| 181 | + |
| 182 | +Consult the corresponding detailed guide based on the language being reviewed: |
| 183 | + |
| 184 | +| Language/Framework | Reference File | Key Topics | |
| 185 | +|-------------------|----------------|------------| |
| 186 | +| **React** | [React Guide](reference/react.md) | Hooks, useEffect, React 19 Actions, RSC, Suspense, TanStack Query v5 | |
| 187 | +| **Vue 3** | [Vue Guide](reference/vue.md) | Composition API, Reactivity System, Props/Emits, Watchers, Composables | |
| 188 | +| **Angular 17+** | [Angular Guide](reference/angular.md) | Signals, Standalone, RxJS, Zoneless, Template Optimization, Testing, Route Guards, HttpInterceptor | |
| 189 | +| **Rust** | [Rust Guide](reference/rust.md) | Ownership/Borrowing, Unsafe Review, Async Code, Cancellation Safety, Error Handling | |
| 190 | +| **TypeScript** | [TypeScript Guide](reference/typescript.md) | Type Safety, async/await, Immutability, Testing, Module Resolution, TS 5.x | |
| 191 | +| **Python** | [Python Guide](reference/python.md) | Mutable Default Arguments, Exception Handling, Class Attributes | |
| 192 | +| **Django / DRF** | [Django Guide](reference/django.md) | Security Review, N+1 Queries, Serializer Anti-patterns, ViewSet, Async Views | |
| 193 | +| **FastAPI** | [FastAPI Guide](reference/fastapi.md) | Depends, Pydantic v2 validation, async correctness, sessions/N+1, auth vs authorization, test-driven verification | |
| 194 | +| **Java** | [Java Guide](reference/java.md) | Java 17/21 New Features, Spring Boot 3, Virtual Threads, Stream/Optional | |
| 195 | +| **PHP** | [PHP Guide](reference/php.md) | PHP 8.x type system, PDO, security review, Composer, PHPUnit/PHPStan | |
| 196 | +| **C# / .NET** | [C# Guide](reference/csharp.md) | C# 12 Features, Async Programming, EF Core Performance, ASP.NET Core, LINQ | |
| 197 | +| **Go** | [Go Guide](reference/go.md) | Error Handling, goroutine/channel, context, Interface Design | |
| 198 | +| **Kotlin / Android** | [Kotlin Guide](reference/kotlin.md) | Coroutines, Flow, Jetpack Compose, Null Safety, Memory Leaks, Architecture Patterns | |
| 199 | +| **Swift / SwiftUI** | [Swift Guide](reference/swift.md) | Optionals, Swift Concurrency, Sendable/actors, SwiftUI property wrappers, value vs reference types, API design | |
| 200 | +| **NestJS** | [NestJS Guide](reference/nestjs.md) | Dependency Injection, Layered Architecture, DTO Validation, Guard/Interceptor, Circular Dependencies | |
| 201 | +| **Svelte / SvelteKit** | [Svelte Guide](reference/svelte.md) | Runes, Load Functions, Form Actions, Store Migration, SSR/CSR Boundaries | |
| 202 | +| **C** | [C Guide](reference/c.md) | Pointers/Buffers, Memory Safety, UB, Secure Coding, Portability, Testing | |
| 203 | +| **C++** | [C++ Guide](reference/cpp.md) | RAII, Smart Pointers, C++20/23, constexpr, Testing | |
| 204 | +| **Zig** | [Zig Guide](reference/zig.md) | Allocators, error unions, defer/errdefer, comptime, C interop | |
| 205 | +| **CSS/Less/Sass** | [CSS Guide](reference/css-less-sass.md) | Variable Conventions, !important, Performance Optimization, Responsive Design, Compatibility | |
| 206 | +| **Qt** | [Qt Guide](reference/qt.md) | Object Model, Signals/Slots, Model/View, QML, Qt6 Migration, Testing | |
| 207 | + |
| 208 | +## Cross-Cutting Guides |
| 209 | + |
| 210 | +Language-agnostic patterns applicable to all code reviews: |
| 211 | + |
| 212 | +| Topic | Reference File | Key Topics | |
| 213 | +|-------|----------------|------------| |
| 214 | +| **Architecture Review** | [Architecture Review Guide](reference/architecture-review-guide.md) | SOLID, anti-patterns, coupling/cohesion, dependency direction | |
| 215 | +| **Performance Review** | [Performance Review Guide](reference/performance-review-guide.md) | Web Vitals, N+1, algorithm complexity, memory leaks, caching | |
| 216 | +| **Security Review** | [Security Review Guide](reference/security-review-guide.md) | SQLi, XSS, CSRF, SSRF, IDOR, Command Injection, Cross-language Examples | |
| 217 | +| **Universal Quality** | [Universal Quality Guide](reference/code-quality-universal.md) | Reuse audit, parameter sprawl, leaky abstractions, nested conditionals, stringly-typed code, TOCTOU, no-op updates, redundant state | |
| 218 | +| **Common Bugs** | [Common Bugs Checklist](reference/common-bugs-checklist.md) | Language-specific bug patterns, common pitfalls | |
| 219 | +| **SQL Injection Prevention** | [SQL Injection Guide](reference/cross-cutting/sql-injection-prevention.md) | Parameterized queries, ORM safety, 6 languages, dynamic identifiers, detection | |
| 220 | +| **XSS Prevention** | [XSS Prevention Guide](reference/cross-cutting/xss-prevention.md) | Output encoding, CSP, 5 frameworks, input validation vs encoding, detection | |
| 221 | +| **N+1 Queries** | [N+1 Queries Guide](reference/cross-cutting/n-plus-one-queries.md) | Eager loading, batch fetching, DataLoader, 5 languages, detection | |
| 222 | +| **Error Handling** | [Error Handling Guide](reference/cross-cutting/error-handling-principles.md) | Fail fast, error hierarchy, 7 languages, anti-patterns, logging | |
| 223 | +| **Async & Concurrency** | [Concurrency Guide](reference/cross-cutting/async-concurrency-patterns.md) | Goroutines, async/await, actors, structured concurrency, 7 languages | |
| 224 | +| **Review Best Practices** | [Code Review Best Practices](reference/code-review-best-practices.md) | Communication, reviewer mindset, giving feedback, severity labels | |
| 225 | + |
| 226 | +## Additional Resources |
| 227 | + |
| 228 | +- [PR Review Template](assets/pr-review-template.md) - PR review comment template |
| 229 | +- [Review Checklist](assets/review-checklist.md) - Quick reference checklist |
0 commit comments