brendendurham PR Review Standards Skill
Review pull requests systematically using team coding standards and best practices.
When to Use
- User asks to "review PR", "review pull request", or "check my changes"
- User wants code review feedback before merging
- User asks about coding standards compliance
Review Process
Step 1: Gather PR Information
- Get the PR diff using
gh pr difforgit diff - Identify changed files and their types
- Determine the scope of changes (feature, bugfix, refactor, etc.)
Step 2: Apply Review Checklist
For each changed file, evaluate against these categories:
Code Quality
- Code is readable and self-documenting
- Functions/methods have single responsibility
- No dead code or commented-out blocks
- Appropriate error handling
- No hardcoded values that should be configurable
Security
- No secrets, API keys, or credentials exposed
- Input validation present where needed
- No SQL injection or XSS vulnerabilities
- Proper authentication/authorization checks
Performance
- No N+1 query patterns
- Appropriate caching strategies
- No memory leaks or resource exhaustion risks
- Efficient algorithms for the use case
Testing
- New code has corresponding tests
- Edge cases are covered
- Tests are meaningful, not just coverage padding
- Integration tests for critical paths
Documentation
- Public APIs are documented
- Complex logic has explanatory comments
- README updated if needed
- Breaking changes documented
Style & Conventions
- Follows project naming conventions
- Consistent formatting (or auto-formatted)
- Import organization matches project style
- File structure follows project patterns
Step 3: Classify Issues by Severity
Use these severity levels for findings:
| Level | Icon | Description | Action Required |
|---|---|---|---|
| Critical | :red_circle: | Security vulnerabilities, data loss risks, crashes | Must fix before merge |
| Major | :orange_circle: | Bugs, significant performance issues, missing error handling | Should fix before merge |
| Minor | :yellow_circle: | Code style issues, minor improvements, suggestions | Nice to have |
| Nitpick | :white_circle: | Personal preferences, optional optimizations | Take or leave |
Step 4: Generate Review Output
Output Format
Structure your review as follows:
## PR Review Summary
**PR:** #{number} - {title}
**Files Changed:** {count}
**Lines:** +{additions} / -{deletions}
**Risk Level:** Low | Medium | High
---
### Overview
{Brief description of what the PR does and overall assessment}
---
### Findings
#### :red_circle: Critical Issues
1. **{File}:{Line}** - {Issue title}
- Problem: {Description}
- Suggestion: {How to fix}
```{language}
{code suggestion if applicable}
:orange_circle: Major Issues
{Same format as above}
:yellow_circle: Minor Issues
{Same format as above}
:white_circle: Nitpicks
{Same format as above}
Checklist Summary
| Category | Status | Notes |
|---|---|---|
| Code Quality | :white_check_mark: / :x: | {brief note} |
| Security | :white_check_mark: / :x: | {brief note} |
| Performance | :white_check_mark: / :x: | {brief note} |
| Testing | :white_check_mark: / :x: | {brief note} |
| Documentation | :white_check_mark: / :x: | {brief note} |
Recommendation
:white_check_mark: Approve - Ready to merge :arrows_counterclockwise: Request Changes - Address critical/major issues :eyes: Comment - Minor suggestions, author discretion
## Example Review
```markdown
## PR Review Summary
**PR:** #142 - Add user authentication middleware
**Files Changed:** 5
**Lines:** +234 / -12
**Risk Level:** High (security-related changes)
---
### Overview
This PR adds JWT-based authentication middleware. The implementation is solid overall, but there are security concerns with token handling that should be addressed before merge.
---
### Findings
#### :red_circle: Critical Issues
1. **src/middleware/auth.js:45** - Token stored in localStorage
- Problem: Storing JWT in localStorage exposes it to XSS attacks
- Suggestion: Use httpOnly cookies for token storage
```javascript
// Instead of:
localStorage.setItem('token', jwt);
// Use httpOnly cookie:
res.cookie('token', jwt, { httpOnly: true, secure: true, sameSite: 'strict' });
:orange_circle: Major Issues
- src/middleware/auth.js:23 - Missing token expiration validation
- Problem: Expired tokens are not being rejected
- Suggestion: Add expiration check in verification logic
Recommendation
:arrows_counterclockwise: Request Changes - Please address the token storage security issue before merge.
## Tips for Effective Reviews
1. **Be constructive** - Suggest solutions, not just problems
2. **Explain the why** - Help authors learn from feedback
3. **Prioritize** - Focus on critical issues first
4. **Acknowledge good work** - Mention well-written code too
5. **Ask questions** - If something is unclear, ask rather than assume