Code Review That Improves Code and Grows Engineers

// ❌ Unlabeled review comments — ambiguous severity
// "This could be a map instead of a forEach"
// "Missing null check here"
// "Consider extracting this into a helper"
 
// ✅ Labeled review comments — clear expectations
// [blocking] Missing null check on user input — this will throw in production
// [suggestion] A map() would be more idiomatic here, but either works
// [nit] Variable name `d` could be more descriptive — maybe `document`?
// [question] What happens if the queue is empty? I don't see that case handled
 
type ReviewSeverity = "blocking" | "suggestion" | "nit" | "question" | "praise";
 
interface ReviewComment {
  severity: ReviewSeverity;
  line: number;
  file: string;
  comment: string;
  suggestedCode?: string;
}
 
function shouldBlockMerge(comments: ReviewComment[]): boolean {
  return comments.some((c) => c.severity === "blocking");
}
 
function categorizeReview(comments: ReviewComment[]): {
  mustFix: ReviewComment[];
  shouldConsider: ReviewComment[];
  optional: ReviewComment[];
} {
  return {
    mustFix: comments.filter((c) => c.severity === "blocking"),
    shouldConsider: comments.filter(
      (c) => c.severity === "suggestion" || c.severity === "question"
    ),
    optional: comments.filter(
      (c) => c.severity === "nit" || c.severity === "praise"
    ),
  };
}

interface PullRequestMetrics {
  filesChanged: number;
  linesAdded: number;
  linesRemoved: number;
  totalDelta: number;
  reviewTimeEstimate: string;
}
 
function assessReviewability(metrics: PullRequestMetrics): {
  rating: "excellent" | "good" | "risky" | "too-large";
  recommendation: string;
} {
  const { totalDelta, filesChanged } = metrics;
 
  if (totalDelta <= 200 && filesChanged <= 5) {
    return {
      rating: "excellent",
      recommendation: "Quick review — focused and easy to reason about",
    };
  }
 
  if (totalDelta <= 400 && filesChanged <= 10) {
    return {
      rating: "good",
      recommendation: "Standard review — allow 30-60 minutes",
    };
  }
 
  if (totalDelta <= 800) {
    return {
      rating: "risky",
      recommendation:
        "Large PR — consider splitting. Review quality will degrade past 400 lines.",
    };
  }
 
  return {
    rating: "too-large",
    recommendation:
      "Split this PR. Reviewers will skim rather than analyze at this size.",
  };
}

interface ReviewChecklist {
  category: string;
  questions: string[];
}
 
const humanReviewChecklist: ReviewChecklist[] = [
  {
    category: "Design",
    questions: [
      "Does this change belong in this module, or is it a sign of misplaced responsibility?",
      "Will this approach still work when requirements change in the obvious ways?",
      "Are there simpler alternatives the author might not have considered?",
    ],
  },
  {
    category: "Edge Cases",
    questions: [
      "What happens with empty input? Null? Undefined?",
      "What if this is called concurrently?",
      "What if the external service is down or slow?",
    ],
  },
  {
    category: "Naming and Intent",
    questions: [
      "Can I understand what this function does from its name alone?",
      "Do variable names communicate their purpose without reading usage?",
      "Would a new team member understand this code in six months?",
    ],
  },
  {
    category: "Hidden Assumptions",
    questions: [
      "What implicit ordering or state does this code depend on?",
      "Are there environment-specific assumptions baked in?",
      "Does this silently degrade or loudly fail on bad input?",
    ],
  },
];

// ❌ Feedback that corrects without teaching
// "Use Promise.all here instead of sequential awaits"
 
// ✅ Feedback that explains the principle
// [suggestion] These three API calls are independent — they don't depend
// on each other's results. Running them sequentially adds ~600ms of
// unnecessary latency. Promise.all lets them execute concurrently:
//
// const [users, orders, inventory] = await Promise.all([
//   fetchUsers(),
//   fetchOrders(),
//   fetchInventory(),
// ]);
//
// General principle: sequential awaits are correct when each call depends
// on the previous result. For independent calls, always parallelize.
 
interface TeachingComment extends ReviewComment {
  principle: string;
  example?: string;
  resources?: string[];
}
 
const exampleFeedback: TeachingComment = {
  severity: "suggestion",
  line: 42,
  file: "src/services/dashboard.ts",
  comment:
    "These API calls can run in parallel since they're independent.",
  principle:
    "Use sequential await when calls depend on previous results. " +
    "Use Promise.all when calls are independent.",
  suggestedCode: `const [users, orders] = await Promise.all([
  fetchUsers(teamId),
  fetchOrders(teamId),
]);`,
};

interface ReviewAgreement {
  maxResponseTimeHours: number;
  maxPRSizeLines: number;
  requiredApprovals: number;
  commentLabels: ReviewSeverity[];
  selfReviewBeforeSubmit: boolean;
  prDescriptionTemplate: string[];
}
 
const teamAgreement: ReviewAgreement = {
  maxResponseTimeHours: 4,
  maxPRSizeLines: 400,
  requiredApprovals: 1,
  commentLabels: ["blocking", "suggestion", "nit", "question", "praise"],
  selfReviewBeforeSubmit: true,
  prDescriptionTemplate: [
    "## What",
    "Brief description of the change",
    "## Why",
    "Context and motivation",
    "## How to test",
    "Steps for the reviewer to verify",
    "## Risks",
    "What could go wrong and how it's mitigated",
  ],
};