Code Review & Quality

Code Review & Quality Skill

Philosophy

"Approve a change when it definitely improves overall code health, even if it isn't perfect."

Progress over perfection. Continuous incremental improvement.

Five-Axis Review Framework

Axis 1: Correctness

Questions to ask:

• Does the implementation match the specification?
• Are edge cases handled?
• Are error paths covered?
• Are there potential runtime issues?
  • Off-by-one errors
  • Race conditions
  • Null/undefined access
  • Resource leaks

Test adequacy:

• Do tests verify the claimed behavior?
• Are negative cases tested?
• Would a bug slip through?

Axis 2: Readability & Simplicity

Questions to ask:

• Can another engineer understand this without explanation?
• Are names clear and descriptive?
• Is control flow straightforward?
• Is the code organized logically?

Complexity checks:

• Deep nesting (> 3 levels)?
• Long functions (> 30 lines)?
• Clever tricks that obscure intent?
• Unnecessary abstractions?

Axis 3: Architecture

Questions to ask:

• Does this follow existing patterns in the codebase?
• Are module boundaries respected?
• Is there code duplication that should be extracted?
• Is the abstraction level appropriate?
• Do dependencies flow in the right direction?

Over-engineering checks:

• Is this simpler than it needs to be?
• Are there abstractions "for future use"?
• Would a simpler approach work?

Axis 4: Security

Input validation:

• Is user input validated and sanitized?
• Are queries parameterized?
• Is output properly encoded?

Authentication & Authorization:

• Are auth checks in place?
• Is ownership verified for resources?
• Are permissions enforced?

Secrets management:

• No hardcoded secrets?
• Sensitive data excluded from logs?
• External data treated as untrusted?

Axis 5: Performance

Common issues:

• N+1 query patterns?
• Unbounded operations (loops, queries)?
• Missing pagination?
• Unnecessary re-renders (React)?
• Objects created in hot paths?
• Missing indexes for queries?

Async handling:

• Are async operations properly awaited?
• Could operations be parallelized?

Change Sizing Guidelines

Each PR should be one logical change, not an entire feature.

Comment Severity Labels

Use prefixes to clarify intent:

Examples:

Critical: This allows SQL injection via the `name` parameter.

Nit: Consider renaming `data` to `userData` for clarity.

Optional: You could use `Array.from()` here instead of spread.

FYI: We have a shared utility for this in `src/utils/format.ts`.

Review Process

Step 1: Understand Context

• What is this PR trying to achieve?
• Is there a linked issue or spec?
• What's the scope of changes?

Step 2: Review Tests First

Tests reveal:

• What behavior is expected
• What edge cases are considered
• Whether coverage is adequate

Step 3: Examine Implementation

Walk through changes with the 5 axes in mind.

Step 4: Categorize Findings

• Must fix — Correctness, security, critical bugs
• Should fix — Readability, architecture concerns
• Consider — Suggestions, style preferences

Step 5: Provide Actionable Feedback

❌ "This is confusing"

✅ "This nested conditional is hard to follow. Consider extracting 
    the inner logic to a named function like `isEligibleForDiscount()`"

Review Output Format

## Review: [PR Title]

### Summary
[1-2 sentences on overall assessment]

### Critical Issues
- **[file:line]** [Issue description]

### Important
- **[file:line]** [Issue description]

### Suggestions
- **[file:line]** [Suggestion]

### Verdict
- [ ] Approve
- [ ] Request changes
- [ ] Needs discussion

Rules

• Don't accept "I'll clean it up later" — It won't happen
• Remove dead code — Don't comment it out
• Review within 1 business day — Faster is better
• Be kind, not nice — Honest feedback helps everyone
• One approval minimum — Before any merge