|
| 1 | +--- |
| 2 | +name: code-review |
| 3 | +description: This skill should be used when conducting focused code reviews that emphasize clarity, data flow understanding, and minimal assumptions. Trigger when reviewing pull requests, code changes, or when explicitly asked to review code. Produces structured reviews with priority-based feedback. |
| 4 | +--- |
| 5 | + |
| 6 | +# Code Review |
| 7 | + |
| 8 | +## Overview |
| 9 | + |
| 10 | +Conduct focused, structured code reviews that emphasize understanding over criticism. Reviews prioritize questions about ambiguity and code/data flow rather than nitpicking style. Each review produces a consistent format with summary, assumptions, and priority-based comments. |
| 11 | + |
| 12 | +## When to Use This Skill |
| 13 | + |
| 14 | +Trigger this skill when: |
| 15 | +- Reviewing pull requests or code changes |
| 16 | +- User explicitly requests "review this code" or "conduct a code review" |
| 17 | +- Analyzing a diff or set of file changes |
| 18 | +- Evaluating code before merge or deployment |
| 19 | +- JIRA tickets reference code changes that need review |
| 20 | + |
| 21 | +## Review Format |
| 22 | + |
| 23 | +All code reviews follow this structure: |
| 24 | + |
| 25 | +### 1. What These Changes Do |
| 26 | +3-5 concise bullet points summarizing the intent and scope of changes. |
| 27 | + |
| 28 | +**Example:** |
| 29 | +- Adds GraphQL endpoint for querying user preferences |
| 30 | +- Refactors authentication middleware to support JWT tokens |
| 31 | +- Introduces caching layer for frequently accessed data |
| 32 | + |
| 33 | +### 2. Assumptions |
| 34 | +Brief bullet points documenting assumptions made during the review. |
| 35 | + |
| 36 | +**Example:** |
| 37 | +- Assuming PostgreSQL is the target database (not explicitly stated) |
| 38 | +- Type definitions suggest this is part of a larger refactor |
| 39 | +- Error handling strategy follows existing patterns in the codebase |
| 40 | + |
| 41 | +### 3. Priority Comments |
| 42 | + |
| 43 | +Comments organized into three priority levels. Each comment is 50-150 words maximum. |
| 44 | + |
| 45 | +#### High Priority |
| 46 | +Issues that must be addressed before merge: |
| 47 | +- Security vulnerabilities |
| 48 | +- Data loss risks |
| 49 | +- Breaking changes without migration path |
| 50 | +- Critical logic errors |
| 51 | +- Major performance regressions |
| 52 | + |
| 53 | +#### Medium Priority |
| 54 | +Issues that should be addressed soon: |
| 55 | +- Unclear code/data flow requiring clarification |
| 56 | +- Missing error handling |
| 57 | +- Ambiguous function behavior |
| 58 | +- Type safety concerns |
| 59 | +- Potential edge cases |
| 60 | + |
| 61 | +#### Low Priority |
| 62 | +Suggestions for future improvement: |
| 63 | +- Code organization opportunities |
| 64 | +- Documentation enhancements |
| 65 | +- Minor performance optimizations |
| 66 | +- Consistency improvements |
| 67 | + |
| 68 | +## Review Process |
| 69 | + |
| 70 | +### Step 1: Identify Changes Scope |
| 71 | +Read through all modified files to understand: |
| 72 | +- What functionality is being added/modified/removed |
| 73 | +- Which systems/services are affected |
| 74 | +- Whether this is a feature, bug fix, refactor, or chore |
| 75 | + |
| 76 | +### Step 2: Check for JIRA Reference |
| 77 | +Scan commit messages, PR descriptions, and file paths for JIRA ticket references: |
| 78 | +- Pattern: `FR-###`, `BUG-###`, `TD-###`, `CHORE-###` |
| 79 | +- If found, prepare to save review document in the JIRA folder: `jira/{category}/{ticket-id}/code-review-{date}.md` |
| 80 | + |
| 81 | +**JIRA Folder Structure:** |
| 82 | +``` |
| 83 | +jira/ |
| 84 | +├── feature-requests/ |
| 85 | +│ └── FR-XXX/ |
| 86 | +│ ├── code-review-2025-11-09.md |
| 87 | +│ └── DESCRIPTION.md |
| 88 | +├── bugs/ |
| 89 | +│ └── BUG-XXX/ |
| 90 | +├── tech-debt/ |
| 91 | +│ └── TD-XXX/ |
| 92 | +└── chores/ |
| 93 | + └── CHORE-XXX/ |
| 94 | +``` |
| 95 | + |
| 96 | +### Step 3: Analyze Code Flow |
| 97 | +Focus on understanding: |
| 98 | +- **Data flow**: How data moves through functions/components |
| 99 | +- **Control flow**: Branching logic, error paths, edge cases |
| 100 | +- **Dependencies**: What this code relies on and what relies on it |
| 101 | +- **Assumptions**: What must be true for this code to work correctly |
| 102 | + |
| 103 | +### Step 4: Formulate Questions |
| 104 | +Instead of prescriptive feedback, ask questions about: |
| 105 | +- Ambiguous behavior: "What happens when userId is null here?" |
| 106 | +- Flow clarity: "How does error propagate from this async call?" |
| 107 | +- Missing context: "Is there a reason we're not validating input X?" |
| 108 | +- Edge cases: "Have we considered the case where the array is empty?" |
| 109 | + |
| 110 | +### Step 5: Prioritize Feedback |
| 111 | +Categorize each comment: |
| 112 | +- **High**: Must fix before merge (security, correctness, data integrity) |
| 113 | +- **Medium**: Should address (clarity, robustness, maintainability) |
| 114 | +- **Low**: Nice to have (style, organization, minor optimizations) |
| 115 | + |
| 116 | +### Step 6: Write Review |
| 117 | +Use the review template from `assets/review-template.md` and populate all sections: |
| 118 | +1. **What These Changes Do** (3-5 bullets) |
| 119 | +2. **Assumptions** (as many bullets as needed, keep short) |
| 120 | +3. **High Priority** (only critical issues) |
| 121 | +4. **Medium Priority** (questions and concerns) |
| 122 | +5. **Low Priority** (suggestions for future) |
| 123 | + |
| 124 | +### Step 7: Save Review Document |
| 125 | +If JIRA ticket found: |
| 126 | +- Save to `jira/{category}/{ticket-id}/code-review-{YYYY-MM-DD}.md` |
| 127 | +- Use today's date for the filename |
| 128 | + |
| 129 | +If no JIRA ticket: |
| 130 | +- Present review inline in the conversation |
| 131 | + |
| 132 | +## Review Principles |
| 133 | + |
| 134 | +### DO Focus On |
| 135 | +✅ Understanding code intent and flow |
| 136 | +✅ Asking questions about ambiguity |
| 137 | +✅ Identifying security and correctness issues |
| 138 | +✅ Flagging unclear data flow |
| 139 | +✅ Noting missing error handling |
| 140 | +✅ Highlighting assumptions that need validation |
| 141 | + |
| 142 | +### DON'T Focus On |
| 143 | +❌ Nitpicking style preferences |
| 144 | +❌ Subjective code organization debates |
| 145 | +❌ Minor naming suggestions |
| 146 | +❌ Formatting (linters handle this) |
| 147 | +❌ Personal preference for syntax |
| 148 | +❌ Extensive refactoring suggestions (unless critical) |
| 149 | + |
| 150 | +### Question Framing |
| 151 | +Always frame feedback as questions or observations, not commands: |
| 152 | + |
| 153 | +**Good:** |
| 154 | +- "What happens if the API returns null here?" |
| 155 | +- "Should we validate userId before passing to the database?" |
| 156 | +- "Is there a reason we're not handling the rejected promise?" |
| 157 | + |
| 158 | +**Avoid:** |
| 159 | +- "You need to add validation here." |
| 160 | +- "This should use async/await instead." |
| 161 | +- "Change this to use map() instead of forEach()." |
| 162 | + |
| 163 | +## Word Count Guidelines |
| 164 | + |
| 165 | +Keep reviews concise: |
| 166 | +- **What These Changes Do**: 1-2 sentences per bullet |
| 167 | +- **Assumptions**: 1 sentence per bullet |
| 168 | +- **Comments**: 50-150 words each (strictly enforced) |
| 169 | + |
| 170 | +Short, focused reviews are more actionable than lengthy critiques. |
| 171 | + |
| 172 | +## Resources |
| 173 | + |
| 174 | +### assets/review-template.md |
| 175 | +Pre-formatted template for consistent review structure. Copy and populate all sections. |
| 176 | + |
| 177 | +### references/review-guidelines.md |
| 178 | +Expanded guidance on identifying different priority levels and framing effective review questions. |
| 179 | + |
| 180 | +## Example Review |
| 181 | + |
| 182 | +**What These Changes Do:** |
| 183 | +- Adds real-time WebSocket connection management for live screen updates |
| 184 | +- Introduces connection pooling to handle up to 1000 concurrent sessions |
| 185 | +- Implements automatic reconnection logic with exponential backoff |
| 186 | + |
| 187 | +**Assumptions:** |
| 188 | +- Redis is available and configured for session storage |
| 189 | +- Frontend implements WebSocket protocol correctly |
| 190 | +- Network latency is acceptable for real-time requirements |
| 191 | + |
| 192 | +**High Priority:** |
| 193 | +- Memory leak risk: WebSocket connections appear to be held indefinitely without cleanup when clients disconnect unexpectedly. Should we implement a timeout or heartbeat mechanism? |
| 194 | + |
| 195 | +**Medium Priority:** |
| 196 | +- Error handling: What happens when Redis connection fails mid-session? The code doesn't appear to handle this case, which could leave clients in an inconsistent state. |
| 197 | +- Type safety: The `message` parameter in `handleIncomingMessage()` is typed as `any`. Can we define a discriminated union for the expected message types? |
| 198 | + |
| 199 | +**Low Priority:** |
| 200 | +- Consider extracting the reconnection logic into a separate utility function for reusability across other real-time features. |
0 commit comments