Pull Request Review

Patterns for effective code review that maintains quality while supporting developers.

Review Focus Areas

• Correctness - Does the code do what it's supposed to?

• Security - Are there any security vulnerabilities?

• Performance - Are there performance concerns?

• Maintainability - Is the code readable and maintainable?

• Testing - Are there adequate tests?

• Style - Does it follow project conventions?

Review Process

• PR Details - Fetch PR diff and description

• Context - Understand the feature/fix being implemented

• Code Analysis - Review each changed file

• Pattern Recognition - Check for known issues with similar patterns

• Feedback - Post review comments

Review Guidelines

Be Constructive

• Focus on the code, not the person

• Explain the "why" behind suggestions

• Offer alternatives, not just criticism

• Acknowledge good patterns when you see them

Categorize Feedback

Category Action Example

Blocking Must fix before merge Security vulnerability, bug

Suggestion Should consider Performance improvement

Nit Nice to have Style preference

Question Need clarification Design decision

Common Review Points

Code Quality:

• Are function names clear and descriptive?

• Is the code DRY (Don't Repeat Yourself)?

• Are there any obvious bugs?

• Is error handling comprehensive?

Security:

• Are inputs validated?

• Are secrets properly managed?

• Are there any injection vulnerabilities?

• Is auth/authz properly enforced?

Performance:

• Are there N+1 query patterns?

• Is there unnecessary computation?

• Are there memory leaks?

• Is caching used appropriately?

Testing:

• Are new features tested?

• Are edge cases covered?

• Are tests readable and maintainable?

• Is test coverage adequate?

Documentation:

• Are public APIs documented?

• Are complex algorithms explained?

• Is the PR description clear?

Approval Criteria

Approve if:

• Code is correct and addresses the requirements

• No security vulnerabilities

• Tests are adequate

• Style follows conventions

• Only nits or minor suggestions remain

Request Changes if:

• There are blocking issues

• Security vulnerabilities exist

• Critical functionality is untested

• Major design concerns

Comment if:

• Have questions but no blocking issues

• Want to discuss alternatives

• Providing information for future consideration

Review Etiquette

• Review promptly (within 24 hours)

• Be respectful and professional

• Assume positive intent

• If it's not clear, ask

• Provide context for your suggestions

• Follow up on your own comments

PR Description Template

Good PRs include:

Summary

Brief description of what this PR does

Changes

• Change 1
• Change 2

Testing

How this was tested

Screenshots (if UI)

Before/after screenshots

Related Issues

Closes #123