charon-fan/code-reviewer

Code Reviewer

A comprehensive code review skill that analyzes pull requests and code changes for quality, security, maintainability, and best practices.

When This Skill Activates

This skill activates when you:

• Ask for a code review
• Request a PR review
• Mention reviewing changes
• Say "review this" or "check this code"

Review Process

Phase 1: Context Gathering

1. Get changed files

   git diff main...HEAD --name-only
   git log main...HEAD --oneline
   

2. Get the diff

   git diff main...HEAD
   

3. Understand project context

   • Read relevant documentation
   • Check existing patterns in similar files
   • Identify project-specific conventions

Phase 2: Analysis Categories

1. Correctness

• [ ] Logic is sound and matches requirements
• [ ] Edge cases are handled
• [ ] Error handling is appropriate
• [ ] No obvious bugs or typos

2. Security

• [ ] No hardcoded secrets or credentials
• [ ] Input validation and sanitization
• [ ] SQL injection prevention
• [ ] XSS prevention (for frontend)
• [ ] Authentication/authorization checks
• [ ] Safe handling of user data

3. Performance

• [ ] No N+1 queries
• [ ] Appropriate caching
• [ ] Efficient algorithms
• [ ] No unnecessary computations
• [ ] Memory efficiency

4. Code Quality

• [ ] Follows DRY principle
• [ ] Follows KISS principle
• [ ] Appropriate abstractions
• [ ] Clear naming conventions
• [ ] Proper typing (if TypeScript)
• [ ] No commented-out code

5. Testing

• [ ] Tests cover new functionality
• [ ] Tests cover edge cases
• [ ] Test assertions are meaningful
• [ ] No brittle tests

6. Documentation

• [ ] Complex logic is explained
• [ ] Public APIs have documentation
• [ ] JSDoc/TSDoc for functions
• [ ] README updated if needed

7. Maintainability

• [ ] Code is readable
• [ ] Consistent style
• [ ] Modular design
• [ ] Separation of concerns

Phase 3: Output Format

Use this structured format for review feedback:

# Code Review

## Summary
Brief overview of the changes (2-3 sentences).

## Issues by Severity

### Critical
Must fix before merge.

- [ ] **Issue Title**: Description with file:line reference

### High
Should fix before merge unless there's a good reason.

- [ ] **Issue Title**: Description with file:line reference

### Medium
Consider fixing, can be done in follow-up.

- [ ] **Issue Title**: Description with file:line reference

### Low
Nice to have improvements.

- [ ] **Issue Title**: Description with file:line reference

## Positive Highlights
What was done well in this PR.

## Suggestions
Optional improvements that don't require immediate action.

## Approval Status
- [ ] Approved
- [ ] Approved with suggestions
- [ ] Request changes

Common Issues to Check

Security Issues

| Issue | Pattern | Recommendation | |-------|----------|----------------| | Hardcoded secrets | const API_KEY = "sk-" | Use environment variables | | SQL injection | \"SELECT * FROM...\" + user_input | Use parameterized queries | | XSS vulnerability | innerHTML = user_input | Sanitize or use textContent | | Missing auth check | New endpoint without @RequireAuth | Add authentication middleware |

Performance Issues

| Issue | Pattern | Recommendation | |-------|----------|----------------| | N+1 query | Loop with database call | Use eager loading or batch queries | | Unnecessary re-render | Missing dependencies in useEffect | Fix dependency array | | Memory leak | Event listener not removed | Add cleanup in useEffect return | | Inefficient loop | Nested loops O(n²) | Consider hash map or different algorithm |

Code Quality Issues

| Issue | Pattern | Recommendation | |-------|----------|----------------| | Duplicate code | Similar blocks repeated | Extract to function | | Magic number | if (status === 5) | Use named constant | | Long function | Function >50 lines | Split into smaller functions | | Complex condition | a && b || c && d | Extract to variable with descriptive name |

Testing Issues

| Issue | Pattern | Recommendation | |-------|----------|----------------| | No tests | New feature without test file | Add unit tests | | Untested edge case | Test only covers happy path | Add edge case tests | | Brittle test | Test relies on implementation details | Test behavior, not implementation | | Missing assertion | Test doesn't assert anything | Add proper assertions |

Language-Specific Guidelines

TypeScript

• Use unknown instead of any for untyped values
• Prefer interface for public APIs, type for unions
• Use strict mode settings
• Avoid as assertions when possible

React

• Follow Hooks rules
• Use useCallback/useMemo appropriately (not prematurely)
• Prefer function components
• Use proper key props in lists
• Avoid prop drilling with Context

Python

• Follow PEP 8 style guide
• Use type hints
• Use f-strings for formatting
• Prefer list comprehensions over map/filter
• Use context managers for resources

Go

• Handle errors explicitly
• Use named returns for clarity
• Keep goroutines simple
• Use channels for communication
• Avoid package-level state

Before Approving

Confirm the following:

• [ ] All critical issues are addressed
• [ ] Tests pass locally
• [ ] No merge conflicts
• [ ] Commit messages are clear
• [ ] Documentation is updated
• [ ] Breaking changes are documented

Scripts

Run the review checklist script:

python scripts/review_checklist.py <pr-number>

References

• references/checklist.md - Complete review checklist
• references/security.md - Security review guidelines
• references/patterns.md - Common patterns and anti-patterns