Pull Requests and Code Reviews

Overview

Pull requests represent a formal mechanism for proposing, reviewing, and integrating code changes into a shared codebase. A developer creates a branch, makes changes, and submits those changes for review before merging into the main branch. The pull request serves as both a technical gate and a collaboration point where team members examine code quality, correctness, security, and maintainability.

Code reviews occur within the pull request process. Reviewers examine the proposed changes line by line, assess architectural decisions, identify bugs, suggest improvements, and verify that changes meet project standards. The review process creates a documented history of technical decisions and serves as a knowledge transfer mechanism across the team.

The pull request model emerged from distributed version control systems, particularly Git, where contributors without direct write access needed a way to propose changes. Modern platforms like GitHub, GitLab, and Bitbucket formalized pull requests into structured workflows with commenting, approval systems, and integration with continuous integration pipelines.

# Example: Checking out a pull request branch locally for review
system("git fetch origin pull/#{pr_number}/head:pr-#{pr_number}")
system("git checkout pr-#{pr_number}")

# Running tests on the PR branch
system("bundle exec rspec")
system("bundle exec rubocop")

Pull requests function as quality gates. No code reaches production without passing through review, automated tests, and explicit approval. This gate prevents bugs, security vulnerabilities, and technical debt from accumulating in the codebase.

Key Principles

Single Responsibility Per Pull Request

Each pull request addresses one logical change. A pull request that fixes a bug, refactors unrelated code, and adds a feature creates review complexity and increases merge conflict risk. Reviewers must understand three different contexts simultaneously, slowing review velocity and reducing review quality.

# Bad: Multiple unrelated changes in one PR
class UserController
  def create
    user = User.new(user_params)
    # New feature: Email verification
    UserMailer.verification_email(user).deliver_later
    
    # Bug fix: Password validation
    if user.password.length < 8
      render json: { error: "Password too short" }
    end
    
    # Refactor: Extract user creation logic
    UserCreator.create_with_defaults(user)
  end
end

# Good: Separate PRs for each concern
# PR #1: Add email verification to user signup
# PR #2: Fix password length validation bug
# PR #3: Extract user creation into service object

Self-Contained Changes

A pull request includes all necessary changes for the feature or fix to function. Incomplete pull requests that depend on future changes create review ambiguity and deployment risk. Reviewers cannot assess whether the change works correctly without seeing the complete picture.

Atomic Commits

Commits within a pull request follow logical progression. Each commit compiles and passes tests. This structure allows reviewers to understand the development process and enables selective reversion if problems appear. Commits like "fix typo", "actually fix it", and "please work" indicate poor commit discipline.

# Example: Structured commit history
# Commit 1: Add database migration for user preferences
# Commit 2: Create UserPreference model with validations
# Commit 3: Add controller actions for preference management
# Commit 4: Implement view templates for preference UI
# Commit 5: Add integration tests for preference workflow

Reviewer Responsibility Distribution

Different review aspects require different expertise. Security-focused reviewers examine authentication and authorization logic. Performance specialists assess query patterns and algorithmic complexity. Domain experts verify business logic correctness. The pull request author requests specific reviewers based on change type.

Automated Checks Before Human Review

Continuous integration runs automated tests, linters, security scanners, and build verification before human reviewers see the pull request. Humans should not spend time identifying issues machines detect reliably. The CI pipeline acts as the first reviewer.

# .github/workflows/pr_checks.yml example
name: Pull Request Checks
on: [pull_request]
jobs:
  test:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - name: Run tests
        run: bundle exec rspec
      - name: Run linter
        run: bundle exec rubocop
      - name: Security scan
        run: bundle exec brakeman -q

Documentation Accompanies Code

Pull requests include updated documentation, README changes, API documentation, and migration guides when applicable. Code changes that alter public interfaces without updating documentation create confusion and increase support burden.

Timely Reviews

Reviews occur within defined SLAs, typically 24 hours for standard changes and 4 hours for critical fixes. Delayed reviews block other work, create merge conflicts, and reduce developer productivity. Teams track review velocity metrics to identify bottlenecks.

Implementation Approaches

Trunk-Based Development

Developers commit directly to the main branch or create short-lived feature branches lasting hours to days. Pull requests remain small, typically under 200 lines. Teams practicing trunk-based development merge changes multiple times daily. This approach requires strong automated testing and feature flags to hide incomplete features.

# Feature flag example for trunk-based development
class PaymentController
  def process
    if FeatureFlag.enabled?(:new_payment_processor, current_user)
      NewPaymentProcessor.process(payment_params)
    else
      LegacyPaymentProcessor.process(payment_params)
    end
  end
end

Trunk-based development reduces merge conflicts by minimizing branch lifetimes. Developers synchronize with the main branch frequently, encountering conflicts when changes remain small and context fresh. The approach demands discipline around breaking large features into independently mergeable increments.

GitHub Flow

Developers create feature branches from main, make changes, open a pull request, receive reviews, and merge to main. Every commit to main deploys to production. This model assumes strong test coverage and fast deployment pipelines.

The main branch remains production-ready at all times. Teams practicing GitHub Flow maintain comprehensive automated testing and implement automated rollback mechanisms for failed deployments.

GitFlow

This approach uses multiple long-lived branches: main for production releases, develop for integration, feature branches for new work, release branches for deployment preparation, and hotfix branches for production bugs. Pull requests merge features into develop, then develop merges into main during releases.

# Branching structure example
# main (production)
# develop (integration)
# feature/user-authentication (PR to develop)
# feature/payment-integration (PR to develop)
# release/v2.1.0 (PR from develop to main)
# hotfix/security-patch (PR to main)

GitFlow suits teams with scheduled releases and complex deployment processes. The multiple branches create coordination overhead and increase merge conflict frequency. Teams using GitFlow often maintain separate QA environments mapped to different branches.

Stacked Pull Requests

For large features requiring multiple changes, developers create a chain of dependent pull requests. Each PR builds on the previous one, allowing parallel review of different aspects while maintaining logical separation.

# PR Chain Example:
# PR #1: Database schema changes (base: main)
# PR #2: Model layer implementation (base: PR #1 branch)
# PR #3: Controller and API changes (base: PR #2 branch)
# PR #4: Frontend integration (base: PR #3 branch)

# Script to update stacked PRs after base PR merges
def update_stacked_prs(merged_pr)
  dependent_prs = find_dependent_prs(merged_pr)
  dependent_prs.each do |pr|
    system("git checkout #{pr.branch}")
    system("git rebase #{merged_pr.target_branch}")
    system("git push --force-with-lease")
  end
end

Reviewers can approve lower-level changes while higher-level work continues. When the base PR merges, the author rebases dependent PRs onto the updated base branch. This approach requires careful coordination and clear documentation of dependencies.

Pair Programming Integration

Some teams combine pair programming with pull requests. Pairs create pull requests together, but reviews still occur from developers outside the pair. The pair programming session provides real-time review, while the pull request review catches issues the pair missed.

Tools & Ecosystem

GitHub Pull Requests

GitHub provides the most widely used pull request implementation. The platform includes inline commenting, approval workflows, review requests, draft pull requests, and integration with GitHub Actions for CI/CD. GitHub's API enables automation of pull request workflows.

require 'octokit'

client = Octokit::Client.new(access_token: ENV['GITHUB_TOKEN'])

# Create a pull request
pr = client.create_pull_request(
  'organization/repository',
  'main',
  'feature/new-authentication',
  'Add OAuth 2.0 authentication',
  'Implements OAuth 2.0 authentication flow with Google and GitHub providers'
)

# Request reviewers
client.request_pull_request_review(
  'organization/repository',
  pr.number,
  reviewers: ['senior-dev-1', 'security-team'],
  team_reviewers: ['backend-team']
)

# Add labels
client.add_labels_to_an_issue(
  'organization/repository',
  pr.number,
  ['needs-review', 'backend', 'security']
)

GitHub's protected branch features enforce review requirements. Administrators configure rules requiring specific numbers of approvals, successful CI runs, and up-to-date branches before merging. These rules prevent accidental or unauthorized merges.

GitLab Merge Requests

GitLab's merge requests offer similar functionality with additional enterprise features. The platform includes built-in CI/CD, security scanning, code quality analysis, and deployment tracking. GitLab merge requests support multiple assignees and more granular approval rules.

require 'gitlab'

Gitlab.configure do |config|
  config.endpoint = 'https://gitlab.com/api/v4'
  config.private_token = ENV['GITLAB_TOKEN']
end

# Create merge request
mr = Gitlab.create_merge_request(
  12345, # project ID
  'Add user preference system',
  source_branch: 'feature/user-preferences',
  target_branch: 'develop',
  assignee_ids: [101, 102],
  labels: 'enhancement,backend'
)

# Approve merge request
Gitlab.approve_merge_request(12345, mr.iid)

GitLab provides merge request templates stored in the repository, standardizing pull request descriptions across the team. Templates prompt authors to include testing steps, security considerations, and deployment notes.

Bitbucket Pull Requests

Atlassian's Bitbucket integrates with Jira for issue tracking and Confluence for documentation. Pull requests link to Jira issues automatically, and merging pull requests updates issue status. Bitbucket supports both cloud and self-hosted deployments.

Review Automation Tools

Danger automates common review checks. The tool runs during CI builds and comments on pull requests when detecting issues. Danger enforces conventions like pull request size limits, changelog updates, and test coverage requirements.

# Dangerfile
# Warn when PR is too large
warn "Large PR (#{git.lines_of_code} lines)" if git.lines_of_code > 500

# Fail if no tests added
fail "Please include tests" if !git.modified_files.any? { |f| f =~ /spec/ }

# Check for changelog update
has_app_changes = !git.modified_files.grep(/^app/).empty?
has_changelog_changes = git.modified_files.include?("CHANGELOG.md")
warn "Please update CHANGELOG.md" if has_app_changes && !has_changelog_changes

# Check for migration safety
danger_migration = false
git.added_files.grep(/db\/migrate/).each do |migration|
  content = File.read(migration)
  danger_migration = true if content.include?("remove_column")
end
fail "Migration contains dangerous operations" if danger_migration

CodeClimate analyzes code quality during pull requests, highlighting complexity issues, duplication, and maintainability problems. The platform provides historical trend analysis showing code quality evolution over time.

SonarQube scans for bugs, vulnerabilities, and code smells. The tool integrates with pull request workflows, blocking merges when critical issues appear. SonarQube tracks technical debt and provides remediation guidance.

Reviewable enhances GitHub's review interface with advanced features like file-level completion tracking, reviewer workload balancing, and discussion threading improvements.

Practical Examples

Feature Addition with Comprehensive Review

A developer adds a new API endpoint for user profile updates. The pull request includes the controller action, model validations, authorization checks, tests, and API documentation updates.

# PR Title: Add PATCH /api/v1/users/:id endpoint for profile updates
# PR Description:
# Implements user profile update endpoint with the following:
# - Field-level authorization (users can only update their own profiles)
# - Validation for email format and username uniqueness
# - Avatar upload with size and type restrictions
# - Audit logging for profile changes
# - Rate limiting (5 updates per hour)
# 
# Testing: Includes 15 new tests covering success cases, validation failures,
# authorization failures, and edge cases
# 
# Security: All inputs sanitized, authorization enforced, audit trail created

# app/controllers/api/v1/users_controller.rb
class Api::V1::UsersController < ApplicationController
  before_action :authenticate_user!
  before_action :authorize_user_update, only: [:update]
  
  def update
    user = User.find(params[:id])
    
    if user.update(user_update_params)
      AuditLog.create(
        user: current_user,
        action: 'profile_update',
        changes: user.previous_changes
      )
      render json: UserSerializer.new(user), status: :ok
    else
      render json: { errors: user.errors }, status: :unprocessable_entity
    end
  end
  
  private
  
  def user_update_params
    params.require(:user).permit(:email, :username, :bio, :avatar)
  end
  
  def authorize_user_update
    user = User.find(params[:id])
    render json: { error: 'Unauthorized' }, status: :forbidden unless current_user == user
  end
end

# spec/requests/api/v1/users_spec.rb
RSpec.describe "PATCH /api/v1/users/:id", type: :request do
  let(:user) { create(:user) }
  let(:headers) { { 'Authorization' => "Bearer #{user.auth_token}" } }
  
  context 'when updating own profile' do
    it 'updates user attributes' do
      patch "/api/v1/users/#{user.id}", 
        params: { user: { bio: 'New bio' } },
        headers: headers
      
      expect(response).to have_http_status(:ok)
      expect(user.reload.bio).to eq('New bio')
    end
    
    it 'creates audit log entry' do
      expect {
        patch "/api/v1/users/#{user.id}",
          params: { user: { email: 'new@example.com' } },
          headers: headers
      }.to change(AuditLog, :count).by(1)
      
      log = AuditLog.last
      expect(log.action).to eq('profile_update')
      expect(log.changes['email']).to eq([user.email, 'new@example.com'])
    end
  end
  
  context 'when updating another user profile' do
    let(:other_user) { create(:user) }
    
    it 'returns forbidden' do
      patch "/api/v1/users/#{other_user.id}",
        params: { user: { bio: 'Hacked' } },
        headers: headers
      
      expect(response).to have_http_status(:forbidden)
      expect(other_user.reload.bio).not_to eq('Hacked')
    end
  end
end

Review Comments and Discussion

Reviewer 1 (Security Focus): "Authorization check passes but consider rate limiting. Add throttling gem to prevent abuse."

Author Response: "Added rack-attack configuration limiting profile updates to 5 per hour per user. Updated tests."

Reviewer 2 (Performance Focus): "The audit log creation adds database write on every update. Consider async job for non-critical logging."

Author Response: "Moved audit logging to background job using Sidekiq. Tests updated to use have_enqueued_job matcher."

Bug Fix with Regression Prevention

A production bug causes nil pointer errors when users without email addresses log in. The fix adds nil checks and includes regression tests preventing similar issues.

# PR Title: Fix NoMethodError when authenticating users without email
# PR Description:
# Production error: NoMethodError: undefined method `downcase' for nil:NilClass
# Root cause: Legacy users migrated from old system lack email addresses
# Fix: Add nil checks and default values
# Prevention: Add validation requiring email for new users
# 
# Impact: Affects ~500 legacy users (0.5% of user base)
# Risk: Low - backwards compatible, only adds safety checks

# app/models/user.rb (Before)
class User < ApplicationRecord
  def email_normalized
    email.downcase.strip
  end
end

# app/models/user.rb (After)
class User < ApplicationRecord
  validates :email, presence: true, on: :create
  
  def email_normalized
    return '' if email.nil?
    email.downcase.strip
  end
  
  def email_domain
    return 'unknown' if email.nil?
    email.split('@').last
  end
end

# spec/models/user_spec.rb
RSpec.describe User do
  describe '#email_normalized' do
    it 'handles nil email for legacy users' do
      user = User.new(email: nil)
      user.save(validate: false) # Bypass validation for legacy user simulation
      
      expect(user.email_normalized).to eq('')
      expect { user.email_normalized }.not_to raise_error
    end
    
    it 'normalizes valid email' do
      user = User.new(email: 'User@Example.COM  ')
      expect(user.email_normalized).to eq('user@example.com')
    end
  end
  
  describe 'validations' do
    it 'requires email for new users' do
      user = User.new(email: nil)
      expect(user).not_to be_valid
      expect(user.errors[:email]).to include("can't be blank")
    end
  end
end

Refactoring with Migration Strategy

A large controller grows to 500 lines with complex business logic. The refactor extracts logic into service objects while maintaining backward compatibility during transition.

# PR Title: Extract payment processing into service objects (Phase 1)
# PR Description:
# Part of larger refactoring effort to decompose PaymentsController
# This PR introduces service objects while keeping controller working
# 
# Changes:
# - Add PaymentProcessor service object
# - Add RefundProcessor service object  
# - Controller delegates to services but keeps old code as fallback
# - Full test coverage for service objects
# 
# Migration: After deployment and monitoring, Phase 2 PR will remove old code
# Rollback: Feature flag allows instant rollback to old implementation

# app/services/payment_processor.rb
class PaymentProcessor
  def initialize(order, payment_method)
    @order = order
    @payment_method = payment_method
  end
  
  def process
    validate_order
    charge_payment
    update_order_status
    send_confirmation
    
    Result.success(order: @order)
  rescue PaymentError => e
    Result.failure(error: e.message)
  end
  
  private
  
  def validate_order
    raise PaymentError, "Invalid order" unless @order.valid?
    raise PaymentError, "Already paid" if @order.paid?
  end
  
  def charge_payment
    @transaction = PaymentGateway.charge(
      amount: @order.total,
      payment_method: @payment_method
    )
  end
  
  def update_order_status
    @order.update!(
      status: 'paid',
      payment_transaction_id: @transaction.id,
      paid_at: Time.current
    )
  end
  
  def send_confirmation
    OrderMailer.payment_confirmation(@order).deliver_later
  end
end

# app/controllers/payments_controller.rb
class PaymentsController < ApplicationController
  def create
    order = Order.find(params[:order_id])
    
    result = if FeatureFlag.enabled?(:payment_service_objects)
      PaymentProcessor.new(order, payment_params).process
    else
      process_payment_legacy(order) # Old implementation kept for rollback
    end
    
    if result.success?
      render json: { order: order }, status: :ok
    else
      render json: { error: result.error }, status: :unprocessable_entity
    end
  end
  
  private
  
  def process_payment_legacy(order)
    # Original 200-line implementation kept for safe rollback
    # Will be removed in Phase 2 after monitoring confirms new code stable
  end
end

Common Patterns

Pre-Merge Checklist Pattern

Teams embed checklists in pull request templates, ensuring authors complete required steps before requesting review. The checklist includes test coverage, documentation updates, database migration safety checks, and security considerations.

## Pull Request Checklist

- [ ] Tests added/updated with 90%+ coverage
- [ ] Documentation updated (README, API docs, inline comments)
- [ ] Database migrations are reversible
- [ ] No sensitive data in logs or error messages
- [ ] Breaking changes documented in CHANGELOG
- [ ] Performance impact assessed (if applicable)
- [ ] Security implications reviewed (if applicable)
- [ ] Accessibility requirements met (if UI changes)

## Testing Performed

- [ ] Unit tests pass locally
- [ ] Integration tests pass locally  
- [ ] Manual testing performed
- [ ] Edge cases tested

## Deployment Notes

<!-- Any special deployment steps, migrations to run, feature flags to enable -->

Two-Reviewer Approval Pattern

Critical code paths require approval from two reviewers with different specializations. Database schema changes need both a senior backend developer and a DBA. Security-sensitive code requires security team approval plus domain expert approval.

# .github/CODEOWNERS
# Database migrations require DBA review
db/migrate/* @database-team @backend-leads

# Authentication/authorization requires security review
app/controllers/sessions_controller.rb @security-team @backend-leads
app/models/user.rb @security-team @backend-leads
lib/auth/* @security-team

# Payment processing requires security + payments team
app/services/payment_*.rb @security-team @payments-team

# Infrastructure changes require DevOps + senior engineer
.github/workflows/* @devops-team @engineering-leads
docker/* @devops-team @engineering-leads

Draft Pull Request Pattern

Developers open draft pull requests early in development to signal work in progress and receive early feedback. Draft PRs run CI checks but don't trigger review requests. When complete, the author converts the draft to a ready pull request, triggering formal review.

# Script to create draft PR and convert when ready
require 'octokit'

def create_draft_pr(branch, title, body)
  client = Octokit::Client.new(access_token: ENV['GITHUB_TOKEN'])
  
  pr = client.create_pull_request(
    'org/repo',
    'main',
    branch,
    title,
    body,
    draft: true
  )
  
  puts "Draft PR created: #{pr.html_url}"
  pr
end

def mark_ready_for_review(pr_number)
  client = Octokit::Client.new(access_token: ENV['GITHUB_TOKEN'])
  
  # Mark ready and request reviewers
  client.update_pull_request(
    'org/repo',
    pr_number,
    draft: false
  )
  
  client.request_pull_request_review(
    'org/repo',
    pr_number,
    reviewers: ['senior-dev', 'architect']
  )
  
  puts "PR #{pr_number} marked ready for review"
end

Incremental Review Pattern

For large pull requests that cannot be split, reviewers approve sections incrementally. The author marks files or sections as "ready for review" and reviewers comment "LGTM on auth changes" or "Approved for database schema." This pattern prevents reviewer fatigue and provides clear progress tracking.

Automated Merge Pattern

After receiving required approvals and passing all checks, pull requests merge automatically. The automation reduces context switching and ensures merges happen promptly. The pattern requires strong test coverage and automated rollback capability.

# .github/workflows/auto_merge.yml
name: Auto Merge
on:
  pull_request_review:
    types: [submitted]

jobs:
  auto_merge:
    if: github.event.review.state == 'approved'
    runs-on: ubuntu-latest
    steps:
      - name: Check if PR is approved and passing
        run: |
          # Check approval count
          # Check CI status
          # Check required checks passed
          
      - name: Enable auto-merge
        run: gh pr merge --auto --squash ${{ github.event.pull_request.number }}

Revert-Ready Pattern

Teams structure commits and pull requests to enable clean reverts. Each pull request represents a complete feature or fix that can be reverted as a single unit. When deploying problematic changes, teams revert the merge commit rather than attempting forward fixes under pressure.

# Script to create revert PR with full context
def create_revert_pr(original_pr_number, reason)
  client = Octokit::Client.new(access_token: ENV['GITHUB_TOKEN'])
  
  original_pr = client.pull_request('org/repo', original_pr_number)
  
  # Create revert commit
  system("git revert -m 1 #{original_pr.merge_commit_sha}")
  
  # Create PR with context
  revert_pr = client.create_pull_request(
    'org/repo',
    'main',
    'revert/pr-#{original_pr_number}',
    "Revert: #{original_pr.title}",
    <<~BODY
      Reverts ##{original_pr_number}
      
      **Reason for revert:** #{reason}
      
      **Original PR:** #{original_pr.html_url}
      **Deployed at:** #{Time.current}
      **Reverted by:** #{ENV['USER']}
      
      **Impact:**
      - Systems affected: 
      - Rollback duration:
      - Users impacted:
    BODY
  )
  
  # Mark as high priority
  client.add_labels_to_an_issue('org/repo', revert_pr.number, ['urgent', 'revert'])
  
  revert_pr
end

Common Pitfalls

Reviewing Too Much Code at Once

Pull requests exceeding 400 lines reduce review effectiveness. Reviewers miss bugs and provide superficial feedback when overwhelmed with changes. Research shows defect detection rates drop significantly above 400 lines. Large PRs also create merge conflict risk and block other work during review.

# Bad: Massive PR touching multiple systems
# PR: Migrate entire application to new framework (3,500 lines)
# - Update all controllers (1,200 lines)
# - Modify all models (800 lines)  
# - Change database schema (15 migrations)
# - Update all tests (1,500 lines)

# Good: Break into manageable pieces
# PR #1: Add framework alongside existing code (200 lines)
# PR #2: Migrate user authentication controllers (250 lines)
# PR #3: Migrate user models and tests (300 lines)
# PR #4: Migrate admin controllers (350 lines)
# PR #5: Remove old framework dependencies (150 lines)

Rubber Stamp Approvals

Approving pull requests without thorough review undermines the process. Pressure to approve quickly, review fatigue, or assumed competence of the author lead to rubber stamping. These approvals miss bugs, security issues, and maintainability problems.

Teams combat rubber stamping by tracking review depth metrics, rotating reviewers to prevent fatigue, and making specific review requirements explicit. Comments like "LGTM" without substantive feedback indicate insufficient review.

Scope Creep During Review

Reviewers requesting unrelated changes during review expands scope and delays merging. Comments like "While you're here, refactor this other class" turn focused pull requests into sprawling changes. The original change gets buried under additional work.

# Example of scope creep in review comments

# Original PR: Fix login button color
# 
# Reviewer comment: "The login button looks good, but can you also:
# - Refactor the entire authentication flow
# - Update all button styles across the application
# - Migrate from CSS to CSS-in-JS
# - Update the style guide documentation"
# 
# Result: 2-day PR becomes 2-week project, original fix delayed

# Correct approach:
# Reviewer: "Button color fix approved. File separate issues for:
# - Issue #123: Refactor authentication flow
# - Issue #124: Standardize button styles
# These should be separate PRs with appropriate scope"

Ignoring Automated Check Failures

Developers requesting review despite failing CI builds or linter errors wastes reviewer time. Reviewers shouldn't manually verify issues machines detect automatically. The practice signals poor development discipline and creates review backlog.

Force Pushing Over Review Comments

Using git push --force after receiving review comments erases the review context. Reviewers lose track of what changed between review rounds. Comments reference lines that no longer exist. The practice creates confusion and duplicates review work.

# Bad: Force push after review
# 1. Push initial PR
# 2. Receive review comments
# 3. Make changes
# 4. Rebase and force push, destroying history
# 5. Reviewers must re-review entire PR, comments orphaned

# Good: Preserve review history
# 1. Push initial PR
# 2. Receive review comments  
# 3. Add fixup commits: "Address review comments on validation"
# 4. Reviewers can see exactly what changed
# 5. After merge approval, squash commits before merging

# Or use --force-with-lease for safety
git push --force-with-lease origin feature/branch

Missing Context in Descriptions

Pull requests lacking context force reviewers to investigate the motivation, approach, and trade-offs independently. Descriptions should explain the problem, solution, alternatives considered, and testing approach. Screenshot, architecture diagrams, and performance measurements provide valuable context.

Nitpicking Style Without Automation

Reviewers focusing on formatting, naming conventions, or style preferences should codify these preferences in linters and formatters. Manual style enforcement in reviews wastes time, creates inconsistency, and frustrates developers. Tools like RuboCop, Prettier, and EditorConfig enforce style automatically.

Delayed Reviews Blocking Progress

Pull requests sitting unreviewed for days block dependent work, increase merge conflicts, and reduce developer productivity. Teams should track review SLAs and assign clear review ownership. Review capacity must match development throughput.

Approving Without Running Tests Locally

Complex changes benefit from reviewers checking out the branch and running tests locally. The practice catches environment-specific issues, validates test quality, and verifies the change works as described. Critical changes warrant local validation before approval.

# Script for thorough local review
#!/bin/bash

PR_NUMBER=$1

echo "Checking out PR #${PR_NUMBER}..."
git fetch origin pull/${PR_NUMBER}/head:pr-${PR_NUMBER}
git checkout pr-${PR_NUMBER}

echo "Installing dependencies..."
bundle install

echo "Running database migrations..."
bundle exec rake db:migrate RAILS_ENV=test

echo "Running tests..."
bundle exec rspec

echo "Running linters..."
bundle exec rubocop

echo "Running security scan..."
bundle exec brakeman -q

echo "Checking for N+1 queries..."
bundle exec rake test:detect_n_plus_one

echo "Review checklist complete for PR #${PR_NUMBER}"

Reference

Pull Request Size Guidelines

Review Response Times

Required Reviewers by Change Type

Automated Check Requirements

Review Comment Types

Branch Protection Rules

Pull Request Template Variables