gh-dhofheinz-open-plugins-plugins-10x-fullstack-engineer/commands/review/pr.md

Pull Request Review

Performs comprehensive pull request review with git integration, analyzing changes, assessing impact, and providing structured feedback for code review collaboration.

Parameters

Received from router: $ARGUMENTS (after removing 'pr' operation)

Expected format: scope:"PR #123" [depth:"quick|standard|deep"]

Workflow

1. Parse Parameters

Extract from $ARGUMENTS:

• scope: PR number or description (required) - "PR #123", "pull request 456", "current branch"
• depth: Review thoroughness (default: "standard")

2. Gather PR Context

Extract PR Information:

# Get PR details if PR number provided
# Example: scope:"PR #123" → extract "123"
PR_NUMBER=$(echo "$SCOPE" | grep -oP '#?\K\d+' || echo "")

if [ -n "$PR_NUMBER" ]; then
  # Get PR info via GitHub CLI
  gh pr view $PR_NUMBER --json title,body,author,labels,assignees,reviewDecision

  # Get PR changes
  gh pr diff $PR_NUMBER
else
  # Review current branch changes
  git diff $(git merge-base HEAD main)..HEAD
fi

# Get commit history
git log main..HEAD --oneline

# Get changed files
git diff --name-only $(git merge-base HEAD main)..HEAD

# Get file change stats
git diff --stat $(git merge-base HEAD main)..HEAD

3. Analyze Change Context

Understand the Changes:

# Categorize changed files
git diff --name-only $(git merge-base HEAD main)..HEAD | grep -E '\.(ts|tsx|js|jsx)$' | head -20

# Check for new files
git diff --name-status $(git merge-base HEAD main)..HEAD | grep "^A"

# Check for deleted files
git diff --name-status $(git merge-base HEAD main)..HEAD | grep "^D"

# Check for renamed/moved files
git diff --name-status $(git merge-base HEAD main)..HEAD | grep "^R"

# Look for test changes
git diff --name-only $(git merge-base HEAD main)..HEAD | grep -E '\.(test|spec)\.'

# Check for documentation changes
git diff --name-only $(git merge-base HEAD main)..HEAD | grep -E '\.(md|txt)$'

4. PR-Specific Review Checklist

PR Metadata Review:

• PR title is clear and descriptive
• PR description explains what and why
• PR size is manageable (< 400 lines changed)
• PR has appropriate labels
• PR is linked to issue/ticket
• PR has reviewers assigned
• PR targets correct branch

Change Scope Assessment:

• Changes align with PR description
• No unrelated changes included
• Scope creep avoided
• Breaking changes clearly documented
• Migration path provided for breaking changes

Commit Quality:

• Commit messages are meaningful
• Commits are logically organized
• No "fix typo" or "wip" commits in history
• Commits could be atomic (optional: suggest squash)
• No merge commits (if using rebase workflow)

Branch Strategy:

• Branch name follows conventions
• Branch is up to date with base branch
• No conflicts with base branch
• Branch focused on single feature/fix

5. Impact Analysis

Risk Assessment:

# Check if changes affect critical files
git diff --name-only $(git merge-base HEAD main)..HEAD | grep -E '(auth|payment|security|config)'

# Check for database changes
git diff $(git merge-base HEAD main)..HEAD | grep -E '(migration|schema|ALTER|CREATE TABLE)'

# Check for dependency changes
git diff $(git merge-base HEAD main)..HEAD -- package.json requirements.txt go.mod

# Check for configuration changes
git diff $(git merge-base HEAD main)..HEAD -- '*.config.*' '.env.example'

Impact Categories:

• Critical: Auth, payment, data integrity, security
• High: Core business logic, public APIs, database schema
• Medium: Feature additions, UI changes, internal APIs
• Low: Documentation, tests, minor refactors

Backward Compatibility:

• API changes are backward compatible
• Database migrations are reversible
• Feature flags used for risky changes
• Deprecation warnings for removed features

6. Code Review Categories

Security Review (Critical for all PRs):

• No hardcoded secrets or credentials
• Input validation for user data
• Authentication/authorization checks
• SQL injection prevention
• XSS prevention
• CSRF protection where needed
• Dependency vulnerabilities checked

Performance Review:

• No obvious performance regressions
• Database queries optimized
• No N+1 query problems introduced
• Caching strategy appropriate
• Bundle size impact considered

Code Quality Review:

• Code follows project conventions
• Functions are focused and manageable
• Naming is clear and consistent
• No code duplication
• Error handling appropriate
• Type safety maintained

Testing Review:

• New features have tests
• Bug fixes have regression tests
• Tests are meaningful
• Test coverage maintained or improved
• Tests pass locally

Documentation Review:

• Public APIs documented
• Complex logic explained
• README updated if needed
• CHANGELOG updated
• Migration guide for breaking changes

7. Test Coverage Analysis

Check Test Changes:

# Find test files in PR
git diff --name-only $(git merge-base HEAD main)..HEAD | grep -E '\.(test|spec)\.'

# Check test coverage (if available)
npm test -- --coverage --changedSince=main 2>/dev/null || echo "Run tests to check coverage"

# Look for untested code
# (Manual review of changed files vs test files)

Test Quality Questions:

• Are tests testing behavior or implementation?
• Are edge cases covered?
• Are error paths tested?
• Are tests isolated and independent?
• Are test names descriptive?

8. Review Changed Files

For Each Changed File:

1. Understand the purpose of changes
2. Check for security issues
3. Check for performance issues
4. Check for quality issues
5. Verify tests exist
6. Look for potential bugs

Focus Areas by File Type:

• Backend code: Auth, validation, queries, error handling
• Frontend code: Rendering, state management, accessibility
• Database migrations: Reversibility, data safety, indexes
• Configuration: Security, environment-specific values
• Dependencies: Vulnerability check, license compatibility

9. PR Review Template

Structure feedback for GitHub PR review:

## Summary

[High-level assessment of the PR]

**Overall Assessment**: ✅ Approve | 💬 Approve with Comments | 🔄 Request Changes

**Change Type**: [Feature | Bug Fix | Refactor | Documentation | Performance]
**Risk Level**: [Low | Medium | High | Critical]
**Estimated Review Time**: [X minutes]

---

## Critical Issues 🚨

**[Must be fixed before merge]**

### Security Issue: [Title]
**File**: `path/to/file.ts` (line X)
**Issue**: [Description]
**Risk**: [Why this is critical]
**Suggestion**:
```typescript
// Current
[problematic code]

// Suggested
[fixed code]

[Repeat for each critical issue]

---

High Priority Comments ⚠️

[Should be addressed before merge]

[Issue Title]

File: path/to/file.ts (line X) Comment: [Description and suggestion]

[Repeat for each high priority issue]

---

Medium Priority Suggestions 💡

[Consider addressing]

[Suggestion Title]

File: path/to/file.ts (line X) Suggestion: [Description]

[Repeat for each medium priority suggestion]

---

Low Priority / Nits 📝

[Optional improvements]

• [File]: [Minor suggestion]
• [File]: [Style nit]

---

Positive Observations ✅

Things done well in this PR:

• ✅ [Good practice 1]
• ✅ [Good practice 2]
• ✅ [Good practice 3]

---

Testing

Test Coverage: [Adequate | Needs Improvement | Insufficient]

• [✅ | ❌] Unit tests for new/changed logic
• [✅ | ❌] Integration tests for API changes
• [✅ | ❌] Component tests for UI changes
• [✅ | ❌] E2E tests for critical paths

Missing Tests:

• [Area 1 that needs tests]
• [Area 2 that needs tests]

---

Documentation

• [✅ | ❌] Code is well-commented
• [✅ | ❌] Public APIs documented
• [✅ | ❌] README updated
• [✅ | ❌] CHANGELOG updated
• [✅ | ❌] Migration guide (if breaking changes)

---

Questions for Author

1. [Question 1 about design decision]
2. [Question 2 about implementation choice]
3. [Question 3 about future plans]

---

Review Checklist

• [✅ | ❌] PR title and description clear
• [✅ | ❌] Changes align with description
• [✅ | ❌] No unrelated changes
• [✅ | ❌] Tests added/updated
• [✅ | ❌] Documentation updated
• [✅ | ❌] No security issues
• [✅ | ❌] No performance regressions
• [✅ | ❌] Code quality maintained
• [✅ | ❌] Branch up to date with base

---

Next Steps

For Author:

• Address critical issues
• Respond to comments
• Add missing tests
• Update documentation
• Request re-review

For Reviewers:

• Additional reviewers needed?
• Security review needed?
• Performance testing needed?

---

Approval Status

Recommendation: [Approve | Request Changes | Needs More Info]

[Explanation of recommendation]

### 10. PR Size Considerations

**PR Too Large** (>400 lines):
- Suggest breaking into smaller PRs
- Review in phases (architecture first, then details)
- Focus on high-risk areas
- Note areas that need detailed review later

**PR Too Small** (<10 lines):
- Quick review appropriate
- Still check for edge cases
- Verify tests exist

### 11. Special PR Types

**Bug Fix PRs**:
- [ ] Root cause identified and documented
- [ ] Regression test added
- [ ] Fix is minimal and focused
- [ ] No unrelated refactoring
- [ ] Backward compatible

**Refactoring PRs**:
- [ ] Behavior unchanged (tests prove this)
- [ ] Motivation clear
- [ ] Refactoring scope reasonable
- [ ] No functional changes mixed in

**Feature PRs**:
- [ ] Feature complete (no half-done work)
- [ ] Feature flag used if risky
- [ ] Tests comprehensive
- [ ] Documentation complete
- [ ] Backward compatible

**Hotfix PRs**:
- [ ] Minimal changes only
- [ ] High test coverage
- [ ] Rollback plan documented
- [ ] Can be deployed independently

## Review Depth Implementation

**Quick Depth** (5-10 min):
- PR metadata and description review
- Security critical issues only
- Obvious bugs
- Test presence check
- High-level change assessment

**Standard Depth** (20-30 min):
- Complete PR review
- All categories covered (security, performance, quality)
- Test coverage reviewed
- Documentation checked
- Detailed inline comments

**Deep Depth** (45-60+ min):
- Comprehensive PR analysis
- Architecture implications
- Performance impact analysis
- Complete test review
- Documentation thoroughness
- Deployment considerations
- Backward compatibility analysis

## Output Format

Provide a GitHub-compatible PR review following the template above, formatted for posting as PR comments.

## Code Examples - Common PR Issues

```typescript
// ❌ COMMON PR ISSUE: Unrelated changes
// PR says "Fix login bug" but also includes:
function LoginForm() {
  // Login fix
  validateCredentials(email, password);

  // UNRELATED: Random formatting change in different feature
  const userProfile = formatUserProfile(user);
  return <div>{userProfile}</div>;
}

// ✅ GOOD: Focused changes only
// PR says "Fix login bug" and only includes:
function LoginForm() {
  // Only the login validation fix
  validateCredentials(email, password);
  return <div>...</div>;
}

// ❌ COMMON PR ISSUE: No tests for new feature
// PR adds new feature without tests
export function calculateDiscount(price: number, code: string): number {
  if (code === 'SAVE10') return price * 0.9;
  if (code === 'SAVE20') return price * 0.8;
  return price;
}

// ✅ GOOD: New feature with tests
export function calculateDiscount(price: number, code: string): number {
  if (code === 'SAVE10') return price * 0.9;
  if (code === 'SAVE20') return price * 0.8;
  return price;
}

// In test file:
describe('calculateDiscount', () => {
  it('should apply 10% discount for SAVE10 code', () => {
    expect(calculateDiscount(100, 'SAVE10')).toBe(90);
  });

  it('should apply 20% discount for SAVE20 code', () => {
    expect(calculateDiscount(100, 'SAVE20')).toBe(80);
  });

  it('should return original price for invalid code', () => {
    expect(calculateDiscount(100, 'INVALID')).toBe(100);
  });
});

// ❌ COMMON PR ISSUE: Breaking change without migration
// Removes field without deprecation period
export interface User {
  id: string;
  name: string;
  // 'email' field removed - BREAKING!
}

// ✅ GOOD: Deprecation with migration guide
export interface User {
  id: string;
  name: string;
  /**
   * @deprecated Use primaryEmail instead. Will be removed in v2.0.0
   */
  email?: string;
  primaryEmail: string; // New field
}

Agent Invocation

This operation MUST leverage the 10x-fullstack-engineer agent for comprehensive PR review expertise.

Best Practices

1. Be Timely: Review PRs promptly to unblock teammates
2. Be Thorough: But don't let perfect be enemy of good
3. Be Constructive: Suggest solutions, not just problems
4. Ask Questions: Understand intent before criticizing
5. Acknowledge Good Work: Positive feedback matters
6. Focus on Impact: Prioritize issues by importance
7. Be Specific: Reference exact files and lines
8. Consider Context: Understand constraints and trade-offs