Effective Code Reviews: Beyond Nitpicking and Rubber Stamps

interface ReviewChecklist {
  // High priority — these create production incidents
  correctness: [
    "Does the code do what the PR description says?",
    "Are edge cases handled (null, empty, max values)?",
    "Are error paths handled, not just the happy path?",
  ];
  security: [
    "Is user input validated and sanitized?",
    "Are authorization checks in place?",
    "Is sensitive data handled properly (no logging PII)?",
  ];
  // Medium priority — these create long-term pain
  design: [
    "Is this the right abstraction level?",
    "Will this be maintainable by someone who didn't write it?",
    "Does it follow existing patterns in the codebase?",
  ];
  // Low priority — automate these away
  style: [
    "Let the linter handle this.",
    "Seriously, configure the linter.",
  ];
}

// ❌ Unhelpful — what should they do instead?
// "This is wrong."
// "Don't do it this way."
// "Nit: use const here."
 
// ✅ Helpful — explains the concern and suggests an alternative
// "This query runs inside the loop, which will cause N+1
// queries at scale. Consider using a JOIN or batch query
// to load all related records in one call."
 
// ✅ Questions work better than commands for design decisions
// "What happens if this promise rejects? I don't see error
// handling — is it intentional to let it bubble up?"

**blocker**: This will cause data loss in production. The DELETE
query has no WHERE clause when `userId` is undefined.
 
**suggestion**: Consider extracting this into a utility function —
I've seen this pattern in three other files.
 
**question**: Is the timeout of 30s intentional? Our SLA is 5s
for this endpoint.
 
**nit**: Minor style preference, not blocking. Take it or leave it.

<!-- ❌ PR description that wastes reviewer time -->
## Changes
Updated the user service.
 
<!-- ✅ PR description that enables quality review -->
## Context
Users reported intermittent 500 errors during checkout.
Root cause: race condition in inventory reservation.
 
## Changes
- Added optimistic locking to inventory updates
- Added retry logic for concurrent modification errors
- Added integration test reproducing the race condition
 
## Testing
- [x] Reproduced the race condition with parallel requests
- [x] Verified fix under concurrent load (k6 script attached)
- [x] Existing tests pass
 
## Risks
- Retry logic adds ~50ms latency in the contention case
- Optimistic locking may surface errors in other flows
  that were silently succeeding with stale data

const reviewEffectiveness = {
  "1-100 lines": { defectRate: "high", reviewTime: "15 min" },
  "100-400 lines": { defectRate: "medium", reviewTime: "30-60 min" },
  "400-1000 lines": { defectRate: "low", reviewTime: "60+ min" },
  "1000+ lines": { defectRate: "near-zero", reviewTime: "rubber stamp" },
};
// Research shows defect detection drops dramatically above 400 lines

// ❌ Gatekeeper review — imposes personal preferences as requirements
// "I would have done this differently. Please rewrite using
// the visitor pattern instead of the switch statement."
 
// ✅ Collaborative review — explains trade-offs
// "A switch statement works here. If we expect more than 5-6
// cases, a strategy pattern might be easier to extend. For now,
// this is fine — just flagging for future reference."

// ❌ Blocking on subjective preferences
// "Please rename `processData` to `transformUserRecords`."
// (4 rounds of review later, still debating the name)
 
// ✅ Approve and suggest
// "Approving — the logic is correct and well-tested.
// Optional: `transformUserRecords` might be more descriptive
// than `processData`, but not blocking on this."

{
  "scripts": {
    "lint": "eslint . --max-warnings 0",
    "format:check": "prettier --check .",
    "typecheck": "tsc --noEmit"
  }
}

# .github/workflows/pr-checks.yml
name: PR Checks
on: [pull_request]
jobs:
  quality:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - run: npm ci
      - run: npm run lint
      - run: npm run format:check
      - run: npm run typecheck
      - run: npm test