Code Review
Systematic code review methodology for identifying issues, security vulnerabilities, and optimization opportunities.
Team Review Guidelines
Communication Style
When providing review feedback:
- Be friendly and constructive
- Good: "What do you think about using X here? It might help with Y." - Good: "Nice solution! One small suggestion..." - Avoid: "This is wrong." - Avoid: "You should have done X."
- Ask before assuming
- Good: "I'm curious about this approach - could you explain the reasoning?" - Avoid: "Why didn't you just...?"
- Acknowledge good work
- "Great catch on the edge case!" - "Clean and readable!" - "I learned something from this."
Double Verification Process
Every PR requires TWO passes:
Pass 1 - Security (Blocking)
- All security checklist items MUST pass
- Any security issue = Request Changes
Pass 2 - Quality (Advisory)
- Standards, performance, maintainability
- These are suggestions, not blockers (unless severe)
Review Phases
Phase 1: Preparation
- Understand Context
- Read PR description and linked issues - Understand the feature/fix being implemented - Check if there are related changes in other PRs - Review the commit history for context
- Identify Standards
- Check project's coding standards - Review existing patterns in the codebase - Note any specific requirements (performance, security)
Phase 2: Systematic Analysis
Work through each category methodically:
Security Review
Input Validation
// GOOD: Parameterized + validated app.post('/user', (req, res) => {const schema = z.object({id: z.number().positive()}); const {id} = schema.parse(req.body); db.query('SELECT * FROM users WHERE id =?', [id]);}); ```
### Authentication & Authorization
// GOOD: Verify ownership app.delete('/api/posts/:id', authenticate, async (req, res) => {const post = await Post.findById(req.params.id); if (post.authorId!== req.user.id &&!req.user.isAdmin) {return res.status(403).json({error: 'Forbidden'});} await post.delete();}); ```
Security Checklist
- SQL/NoSQL injection prevented (parameterized queries)
- XSS prevented (output encoding, CSP headers)
- CSRF protection in place
- Sensitive data not logged or exposed
- API keys/secrets not hardcoded
- File uploads validated and sanitized
- Rate limiting on sensitive endpoints
- Proper error messages (no stack traces in production)
Performance Review
N+1 Query Detection
// GOOD: Eager loading const posts = await Post.find().populate('author'); ```
### Performance Checklist
- No N+1 queries (use eager loading)
- Expensive operations cached appropriately
- Database queries use proper indexes
- Large datasets paginated
- Async operations properly awaited
- No unnecessary re-renders (React)
- Memory leaks prevented (cleanup handlers)
- Heavy computations memoized or offloaded
## Code Quality Checklist
- Clear, descriptive naming conventions
- Functions are single-purpose (< 30 lines ideal)
- Cyclomatic complexity reasonable (< 10)
- No code duplication (DRY principle)
- Proper error handling throughout
- Edge cases considered
- Magic numbers/strings extracted to constants
- Comments explain "why", not "what"
## Edge Cases Checklist
Always verify these scenarios are handled:
### Data Edge Cases
- Empty arrays/collections
- Null or undefined values
- Zero values (especially in division)
- Negative numbers where only positive expected
- Empty strings vs null vs undefined
- Unicode and special characters in strings
### User Input Edge Cases
- Missing required fields
- Fields with only whitespace
- Duplicate submissions (double-click)
- Concurrent updates to same resource
- Invalid date formats
- Timezone handling
- File upload with no file selected
### API Edge Cases
- Network timeout handling
- Rate limit exceeded responses
- API returns unexpected data structure
- Partial success in batch operations
- Graceful degradation when service unavailable
### WordPress-Specific Edge Cases
- Post with no featured image
- User with no display name
- Taxonomy with no posts
- Widget in empty sidebar
- Shortcode with no attributes
- Multisite subdirectory vs subdomain
- Translation missing for locale
- Cron job running during high traffic
- Option not yet saved (first install)
- Meta value of 0 vs meta not existing
- get_post() returning null
- WP_Query with no results
- Current user not logged in (ID = 0)
## Language-Specific Checks
### PHP/WordPress
- Escaping output (esc_html, esc_attr, etc.)
- Nonces for form submissions
- Capability checks for actions
- Prepared statements for queries
- WordPress coding standards followed
- Cron hooks: No name collision between cron hook and internal do_action()
- Cron scheduling: Events scheduled on activation, cleared on deactivation
- Hook callbacks: No recursive/self-triggering patterns
- Settings sanitization handles unchecked checkboxes
- Transient expiration set appropriately
- Object cache checked before expensive queries
- REST endpoints have permission_callback
- AJAX handlers verify nonce and capability
### JavaScript/TypeScript
- === used instead of ==
- Proper TypeScript types (no any abuse)
- async/await error handling
- No prototype pollution risks
- ESLint/Prettier rules followed
### React
- Hooks rules followed
- Keys provided for lists
- useEffect dependencies correct
- No state mutations
- Proper component composition
## Review Output Format
Structure feedback as:
### Critical Issues (Must Fix)
- Security vulnerabilities
- Data loss risks
- Breaking changes without migration
- Failing tests
### Warnings (Should Fix)
- Performance concerns
- Code maintainability
- Missing error handling
### Suggestions (Nice to Have)
- Refactoring opportunities
- Better naming
- Additional documentation
### Positive Notes
- Good patterns followed
- Clean code
- Thorough testing