Review pull requests for code quality, security, bugs, and best practices
/plugin marketplace add claudeforge/marketplace/plugin install pr-reviewer@claudeforge-marketplaceComprehensive pull request review covering code quality, security, testing, and best practices.
Provide the PR number to review:
/pr-review 789
The command will analyze the PR and provide detailed feedback.
Code Quality
Functionality
Security
Testing
Documentation
PR #789: "Add user search feature"
Code Quality Issues
// Issue: Missing null check
function searchUsers(query) {
return users.filter(u => u.name.includes(query));
// Problem: Crashes if query or u.name is null
}
// Suggestion:
function searchUsers(query) {
if (!query) return [];
return users.filter(u => u.name?.includes(query));
}
Security Concerns
// Issue: SQL injection risk
const query = `SELECT * FROM users WHERE name = '${input}'`;
// Suggestion: Use parameterized queries
const query = 'SELECT * FROM users WHERE name = ?';
db.execute(query, [input]);
Missing Tests
// Needs tests for:
- Empty search query
- Special characters in query
- Case sensitivity
- No results found
- Null/undefined inputs
Functionality
Code Quality
Security
Testing
Performance
Documentation
Missing Error Handling
// Bad
const data = JSON.parse(input);
// Good
try {
const data = JSON.parse(input);
} catch (error) {
console.error('Invalid JSON:', error);
return null;
}
Unsafe Data Access
// Bad
const email = user.profile.email;
// Good
const email = user?.profile?.email || 'unknown';
Inefficient Loops
// Bad
for (let item of items) {
await processItem(item);
}
// Good
await Promise.all(items.map(item => processItem(item)));
Hardcoded Values
// Bad
const timeout = 5000;
// Good
const timeout = config.requestTimeout || 5000;
Structure
**Issue**: Brief description of the problem
**Location**: file.ts:42
**Severity**: Critical | High | Medium | Low
**Suggestion**:
Specific code or approach to fix the issue
**Reason**:
Why this is important and what could go wrong
Example
**Issue**: Potential SQL injection vulnerability
**Location**: api/users.ts:23
**Severity**: Critical
**Suggestion**:
Use parameterized queries instead of string concatenation:
`db.query('SELECT * FROM users WHERE id = ?', [userId])`
**Reason**:
Current code allows attackers to inject malicious SQL,
potentially exposing or deleting all user data.
Approve if:
Request Changes if:
Comment if:
Verify tests are:
Comprehensive
// Good test coverage
describe('searchUsers', () => {
test('returns matching users', () => { ... });
test('handles empty query', () => { ... });
test('is case insensitive', () => { ... });
test('returns empty array for no matches', () => { ... });
});
Meaningful
// Good test
expect(result.length).toBe(2);
expect(result[0].name).toBe('Alice');
// Bad test
expect(result).toBeTruthy(); // Too vague
Check for:
Database Efficiency
Algorithm Efficiency
Resource Usage
Ensure:
Positive Feedback
Great job handling edge cases in the validation logic!
The error messages are clear and helpful to users.
Constructive Feedback
Consider extracting this logic into a separate function
to improve readability and make it easier to test.
Question
How does this handle the case when the user is not
authenticated? Should we add a check here?
Too Many Issues: Focus on critical ones first
Unclear Changes: Ask PR author for clarification
Disagreement: Discuss reasoning, consider team standards
Time Constraints: Do quick pass for critical issues only
A thorough review includes: