eyadsibai Code Review
Both sides of the code review process: requesting and receiving feedback.
When to Request Review
Mandatory
| Situation | Why |
|---|---|
| After completing major feature | Catch issues before they spread |
| Before merge to main | Gate quality |
| After each task in subagent workflow | Fix issues before compounding |
Valuable
| Situation | Why |
|---|---|
| When stuck | Fresh perspective |
| Before refactoring | Baseline check |
| After fixing complex bug | Verify fix doesn't introduce new issues |
Requesting Review
What to Provide
| Element | Purpose |
|---|---|
| What was implemented | Context for reviewer |
| Requirements/plan reference | What it should do |
| Base SHA | Starting point |
| Head SHA | Ending point |
| Brief summary | Quick orientation |
Acting on Feedback
| Severity | Action |
|---|---|
| Critical | Fix immediately |
| Important | Fix before proceeding |
| Minor | Note for later |
| Disagree | Push back with reasoning |
Receiving Review
Core Principle
Verify before implementing. Ask before assuming. Technical correctness over social comfort.
The Response Pattern
- Read - Complete feedback without reacting
- Understand - Restate requirement (or ask)
- Verify - Check against codebase reality
- Evaluate - Technically sound for THIS codebase?
- Respond - Technical acknowledgment or reasoned pushback
- Implement - One item at a time, test each
Handling Unclear Feedback
| Situation | Action |
|---|---|
| Some items unclear | STOP - ask before implementing any |
| Partially understood | Don't implement partial - items may be related |
| Scope unclear | Ask for clarification |
Key concept: Partial understanding leads to wrong implementation. Clarify everything first.
When to Push Back
| Situation | Push Back |
|---|---|
| Suggestion breaks existing functionality | Yes |
| Reviewer lacks full context | Yes |
| Unused feature (YAGNI violation) | Yes |
| Technically incorrect for this stack | Yes |
| Legacy/compatibility reasons exist | Yes |
| Conflicts with architectural decisions | Yes |
How to Push Back
| Do | Don't |
|---|---|
| Use technical reasoning | Be defensive |
| Ask specific questions | Argue emotionally |
| Reference working tests/code | Ignore valid feedback |
| Show evidence | Just say "no" |
Implementation Order
When fixing multiple items:
- Clarify anything unclear FIRST
- Then implement in order:
- Blocking issues (breaks, security)
- Simple fixes (typos, imports)
- Complex fixes (refactoring, logic)
- Test each fix individually
- Verify no regressions
Response Patterns
Forbidden (Performative)
| Don't Say | Why |
|---|---|
| "You're absolutely right!" | Performative, not technical |
| "Great point!" | Empty agreement |
| "Let me implement that now" | Before verification |
Correct Responses
| Situation | Response |
|---|---|
| Feedback is correct | "Fixed. [Brief description]" |
| Need clarification | "Need clarification on X before proceeding" |
| Disagree | "[Technical reasoning why current approach is better]" |
| Can't verify | "Can't verify without [X]. Should I investigate?" |
Key concept: Actions speak. Just fix it. The code shows you heard the feedback.
Red Flags
| Never Do | Why |
|---|---|
| Skip review because "it's simple" | Simple changes cause bugs too |
| Ignore Critical issues | They're critical for a reason |
| Proceed with unfixed Important issues | They'll compound |
| Argue with valid technical feedback | Ego over quality |
| Implement without understanding | Wrong fixes waste time |
Source-Specific Handling
| Source | Approach |
|---|---|
| User | Trusted - implement after understanding, skip to action |
| External reviewer | Evaluate technically before implementing |
| Automated tool | Verify relevance to your context |