gh-claudeforge-marketplace-plugins-commands-pr-reviewer/commands/pr-review.md

description, version

description	version
Review pull requests for code quality, security, bugs, and best practices	1.0.0

PR Reviewer

Comprehensive pull request review covering code quality, security, testing, and best practices.

What It Does

• Analyzes PR changes for code quality issues
• Identifies potential bugs and edge cases
• Checks security vulnerabilities
• Verifies test coverage
• Suggests improvements and optimizations
• Reviews documentation and naming

How to Use

Provide the PR number to review:

/pr-review 789

The command will analyze the PR and provide detailed feedback.

Review Areas

Code Quality

• Code structure and organization
• Naming conventions
• Error handling
• Code duplication

Functionality

• Logic correctness
• Edge case handling
• Error conditions
• Performance implications

Security

• Input validation
• SQL injection risks
• XSS vulnerabilities
• Authentication/authorization

Testing

• Test coverage
• Test quality
• Missing test cases
• Integration tests

Documentation

• Code comments
• API documentation
• README updates
• Inline explanations

Example Review

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

Use Cases

• Pre-Merge Review: Catch issues before merging to main
• Learning Tool: Help team improve code quality
• Security Audit: Identify security vulnerabilities
• Best Practices: Ensure code follows standards
• Knowledge Sharing: Educate on better approaches

Best Practices

• Be Constructive: Suggest improvements, don't just criticize
• Explain Why: Provide reasoning for suggestions
• Prioritize Issues: Mark critical vs nice-to-have changes
• Test Suggestions: Verify suggestions actually work
• Link Resources: Provide docs or examples when helpful
• Acknowledge Good Code: Highlight what's done well

Review Checklist

Functionality

• Code does what PR description claims
• Edge cases are handled
• Error conditions are managed
• No obvious bugs

Code Quality

• Code is readable and maintainable
• Functions are focused and single-purpose
• No code duplication
• Naming is clear and consistent

Security

• Input is validated
• No SQL injection vulnerabilities
• No XSS vulnerabilities
• Authentication is required where needed

Testing

• New code has tests
• Tests cover edge cases
• Tests are meaningful
• All tests pass

Performance

• No obvious performance issues
• Database queries are optimized
• Large data sets are handled efficiently
• No memory leaks

Documentation

• Complex logic is commented
• API changes are documented
• README is updated if needed
• Breaking changes are noted

Common Issues

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;

Review Comments Format

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.

Approval Guidelines

Approve if:

• All critical issues are resolved
• Code meets quality standards
• Tests are comprehensive
• No security concerns

Request Changes if:

• Critical bugs exist
• Security vulnerabilities present
• Missing essential tests
• Code quality issues

Comment if:

• Minor improvements suggested
• Questions need clarification
• Architecture discussion needed

Testing Review

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

Performance Review

Check for:

Database Efficiency

• Are queries optimized?
• Are indexes being used?
• Is there an N+1 query problem?

Algorithm Efficiency

• Could a better algorithm be used?
• Is time complexity acceptable?
• Are there unnecessary iterations?

Resource Usage

• Are large objects being copied unnecessarily?
• Is memory being freed properly?
• Are files/connections closed?

Documentation Review

Ensure:

• Complex logic has comments explaining why
• Public APIs have JSDoc or similar
• Breaking changes are documented
• Migration guides exist if needed

Feedback Examples

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?

Troubleshooting

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

Quality Standards

A thorough review includes:

• All code changes examined
• Security implications considered
• Test coverage verified
• Documentation checked
• Performance assessed
• Clear, actionable feedback
• Specific code suggestions where applicable