Code Review and Working with Existing Codebases Questions

Direct answer

I track a mix of speed, thoroughness, and outcome metrics, because optimizing for any one alone, especially speed, invites gaming. Below are five metrics with how I'd collect each and its specific gaming risk, plus how a team would use target thresholds operationally instead of just observing raw numbers.

Structured elaboration

Five metrics

Target-behavior and threshold framing

Rather than just reporting raw numbers, a team typically sets a target band per metric, for example "median time-to-first-review under 8 business hours" or "defect-escape rate under a set percentage of merged PRs," and treats the metric as healthy while inside the band, investigating only when it drifts outside. Thresholds should come from the team's own historical baseline, not an industry number, since review load and risk profile vary a lot by codebase. Alert on trend, not a single data point: a metric crossing its threshold for one week is noise, three consecutive weeks is worth a retro.

Using these without them backfiring

• Report at the team level, never rank individual reviewers publicly, individual rankings are exactly what triggers the gaming behaviors above
• Pair a speed metric with a quality metric (time-to-first-review alongside defect-escape rate) so a team can't "win" by optimizing speed alone
• Revisit the metric set periodically; a metric that's been stable for months provides less signal than one that's actively moving

Worked example

A platform team sets these threshold bands: time-to-first-review under 8 business hours (current baseline 6h), rubber-stamp rate under 20% (current baseline 15%), defect-escape rate under 5% of merged PRs (current baseline 3%). In week 3 of a sprint, time-to-first-review spikes to 18 hours for two consecutive weeks while rubber-stamp rate climbs to 35% at the same time. Read together, this isn't "reviewers are slow," it's "reviewers are overloaded and starting to skim": three reviewers are pulled onto a separate incident response for two weeks, leaving review understaffed. The team's response is temporary reviewer reassignment, not a mandate to review faster, because the paired metrics pointed at capacity, not diligence.

Trade-offs and pitfalls

• Any single metric optimized in isolation gets gamed; the value is in reading two or three together, never acting on one alone
• Metrics tied to individual performance reviews reliably produce the gaming behaviors listed above; keep them team-level diagnostics
• Defect-escape rate has a lag (bugs surface weeks later), so it's a trailing indicator, useful for validating whether a change worked, not for fast feedback
• Thresholds set once and never revisited stop being meaningful as the team, codebase, or risk profile changes; review them roughly quarterly

Direct answer

As the engineering manager, act on two tracks at once: repair the specific relationship and any public harm quickly and privately, and separately fix the guidelines or process so this failure mode doesn't repeat. Treat a single conversation as the start of the fix, not the end of it, and check back later.

Structured elaboration

1. Talk to the author first, 1:1. Acknowledge the harm directly without minimizing it, and ask what they need right now, whether the comment should be edited or removed, or whether they'd prefer a different reviewer on this PR (pull request).
2. Talk to the reviewer separately, not in a group setting. Be direct that the language was out of line regardless of whether the underlying technical point was right, and get their perspective, rushed, frustrated, unaware of tone, without treating that as an excuse for the impact.
3. Repair publicly if the harm was public. If the comment was visible to the team, a short, genuine acknowledgment of what happened is worth more than a vague "let's all be kind" message that erases the specifics; the goal is for the team to see it was actually addressed.
4. Close the structural gap. Check whether the team's review guidelines say anything about tone and conduct at all. If they don't, that's a process gap, not solely the reviewer's individual failure, and it's the manager's job to close it, e.g. adding an explicit norm plus example phrasing (this is a nit, this is blocking, avoid absolute language like "this is terrible") to the team's review guide.
5. Coach, don't just discipline, for a first occurrence. Work through concrete feedback practices together: phrasing comments about the code rather than the person, leading with a question instead of a command, labeling severity explicitly so a blocking issue doesn't read as optional, and pairing any criticism with a specific alternative rather than a bare complaint. Consider having the reviewer shadow reviews from someone whose feedback style already does this well. If this turns out to be a repeat pattern, it escalates beyond coaching.
6. Follow up and actually measure it. Check in with the author privately again a couple of weeks later, not just once, to see if the relationship genuinely repaired rather than just went quiet. Watch later review threads for whether tone actually changed; a guideline that's written down but never checked tends to fade.

Worked example

A reviewer writes something like "this is embarrassing, did you even test this" on a junior engineer's PR, visible to the whole team channel. The manager messages both people privately within the day, has the reviewer edit the comment, and posts a short, honest acknowledgment in the channel rather than a vague platitude. The team's review guide gets an explicit tone section with example blocking-versus-nit phrasing. Two weeks later, the manager checks in privately with the junior engineer, not just assuming things are fine because nobody's raised it again.

Trade-offs and pitfalls

Over-correcting into a heavily policed review culture, where people are afraid to say anything critical, is its own failure mode; the goal is honest, direct feedback delivered respectfully, not conflict avoidance. Treating this as purely an individual coaching issue without fixing the guideline gap means the next person makes the same mistake. Treating it as purely a documentation fix without a real conversation with both people leaves the actual relationship unrepaired.

Direct answer

Before a human looks at the code, I want checks that are cheap, deterministic, and fast: build/compile, the existing test suite, correctness-oriented linting, formatting, and security or secret scanning. Build, tests, correctness linting, formatting, and security scanning are effectively mandatory since each is cheap and prevents real harm; a coverage-delta threshold is the one I'd keep optional or advisory.

Structured elaboration

Why this ordering matters

Machines are fast, consistent, and never get tired or skip a check under time pressure, exactly the trait humans lose in a rushed review. This reframes the human reviewer's job: not "did the tests pass," but "does this change make sense, is the design right, are there edge cases the tests don't cover."

Worked example

A PR adds a new /export endpoint. Continuous integration (CI, the automated build/test pipeline) runs: the build passes; the existing 340-test suite passes; linting flags an unused import, requiring a one-line manual fix; the secret scanner catches a hardcoded key accidentally left in a test fixture and blocks the merge until the author replaces it with an environment-variable reference; the coverage-delta check shows the new endpoint added 0 new tests against 45 new lines and posts an advisory warning, not a block, prompting the author to add a test before requesting review. Only after the build, existing tests, and the secret-scan block clear does the PR reach a human reviewer, who focuses entirely on whether the endpoint's authorization check is correct, something none of the automated checks could evaluate.

Trade-offs and pitfalls

• Too many mandatory gates slows every PR for marginal benefit; keep the mandatory set to checks that catch real, expensive-to-miss problems, not personal style preferences
• A coverage-delta threshold enforced as a hard block gets gamed with low-value tests written just to hit a number; keep it advisory and let a human interpret it
• Automated checks can produce false confidence, "CI is green" means the mechanical bar was cleared, not that the design is right
• Security or secret scanners produce false positives; if they cry wolf too often, engineers start ignoring or bypassing them

Direct answer

A rollback-safe rename plus backfill never touches the old column or existing readers directly. It adds the new column alongside the old one, writes to both while backfilling the new one in batches, verifies the backfilled data matches, only then switches reads over behind a flag, and keeps the old column around for a retention window so any step can be reversed just by flipping the flag back, not by undoing a destructive change.

Structured elaboration

Each phase below names what code review should specifically confirm before approving it, as a "Review check," plus what to monitor once it ships.

1. Schema change. Add the new column as nullable, with no constraints yet:

ALTER TABLE events ADD COLUMN new_name text NULL;

Review check: confirm this specific statement is additive only and backward-compatible, meaning every existing reader and writer keeps working unmodified the moment this ships, with zero application changes required yet.

2. Dual-write. Deploy an application change, behind a feature flag, that writes both the old and new column on every write to a row. Review check: is the write to both columns transactional or otherwise guaranteed consistent (not "write old, then separately and non-atomically write new"), and is the flag off by default so this ships dormant before anything depends on it?

3. Backfill. A batched, idempotent job fills in the new column for existing rows, only where it's still NULL, ordered by primary key, with a checkpoint so it can resume after an interruption instead of restarting from row one:

UPDATE events
SET new_name = old_name
WHERE id BETWEEN :batch_start AND :batch_end
  AND new_name IS NULL;

Review check: is progress persisted somewhere durable (not just in the running process's memory), and does re-running an already-completed batch do nothing (true idempotence), not create incorrect data? Monitoring: track batches completed, rows backfilled, replication lag, and write error rate on the table for the duration of the backfill, and pause automatically if replication lag crosses an agreed threshold.

4. Verification. Before trusting the backfill, sample a random set of rows and confirm new_name matches what old_name implies for each. Review check: is the sample size and comparison method actually specified in the PR, not just asserted as "we verified it"?

5. Read cutover. Only after verification passes, flip the flag so reads prefer the new column, falling back to the old one if the new one is somehow still empty for a given row. Review check: is the fallback logic actually tested, not just written? Monitoring: application error rate and query latency on this table specifically, right after the flag flips, since a regression here is the trigger for the rollback shown in the diagram below.

6. Cleanup. Once reads have run on the new column successfully for a defined retention window, make it NOT NULL, add any index it needs, and only then drop the old column, in a separate, later PR. Review check: is dropping the old column genuinely a separate step from everything above, so it can never accidentally ship bundled with a change that hasn't been verified yet?

flowchart LR
  A[Step 1: add new_name column, nullable] --> B[Step 2: dual write old_name plus new_name]
  B --> C[Step 3: backfill new_name in batches where NULL]
  C --> D{Verification: sampled row values match}
  D -- mismatch found --> C
  D -- fully verified --> E[Step 4: flip reads to new_name behind a flag]
  E --> F{Error rate normal after cutover}
  F -- regression --> G[Rollback: flip flag back to old_name, dual write stays intact]
  F -- healthy --> H[Step 5: make new_name NOT NULL, add index concurrently]
  H --> I[Step 6: drop old_name after a retention window]

Worked example

Renaming user_email to primary_email on a table with 40 million rows, backfilled in batches of 5,000 rows: that's 40,000,000 / 5,000 = 8,000 batches total. With a short pause between batches to keep replication lag bounded, the job runs as a background process over however long it takes to work through all 8,000 batches, checkpointing its position after each one so a restart resumes from the last completed batch instead of row one. Verification samples 10,000 random rows after the backfill reports complete and confirms primary_email equals user_email for every one of them before the flag is ever flipped to prefer reads from the new column.

Trade-offs and pitfalls

Every step here is reversible specifically because the old column and old read path stay intact until the very last, separate cleanup step, which is exactly what makes this slower and more code than a single rename statement; that trade is worth it for a heavily-used column and wrong for a rarely-touched internal table, where a single migration with a maintenance window might be simpler and perfectly safe. The most dangerous version of this pattern to review is one that quietly combines two of these steps, most often shipping the read cutover and the old-column drop in the same change, which collapses the rollback safety the whole design exists to provide.

Direct answer

I'd treat the two symptoms, slower review and more defects, as possibly the same root cause or two different ones, and not assume slower review caused more defects without checking. I'd generate hypotheses across people, process, and tooling, collect data to confirm or rule out each before proposing fixes, then sequence remediation by evidence strength and effort over a 90-day plan with explicit target metrics.

Structured elaboration

Hypotheses by category

People: the team grew or turned over, meaning fewer experienced reviewers are available, data to collect: headcount and tenure over the quarter, reviewer roster size over time. Or reviewer burnout, a few people doing most reviews, now overloaded, data to collect: review-count distribution per person, week over week.

Process: pull request (PR, a proposed code change submitted for review) size grew, bigger diffs take longer to review and hide more defects, data to collect: median and 90th-percentile PR size trend over the quarter. Or review requirements changed, for example a new mandatory second-reviewer rule added friction without adding value, data to collect: a policy change log cross-referenced against the timeline. Or priorities shifted, a launch or incident pulled focus away from review discipline, data to collect: a calendar of major initiatives and incidents in the quarter.

Tooling: continuous integration (CI, the automated build/test pipeline) got slower or flakier, so PRs sit longer waiting for a green build before a human even looks, data to collect: CI run duration and retry rate over the quarter. Or a tooling change silently reduced visibility of pending reviews, for example a broken notification bot, data to collect: a tooling change log and notification-delivery logs.

Data to collect, overall

• PR metadata over the quarter: size, time-to-first-review, time-to-merge, reviewer identity, per week
• Defect data: which PRs the post-release defects trace back to, and whether those PRs had unusually fast or slow review, small or large diffs, few or many comments
• Team roster and calendar: headcount changes, major initiatives, incidents, holidays
• Tooling telemetry: CI duration, notification delivery, any process or tooling changes deployed during the quarter

Prioritized remediation plan

1. Address whichever hypothesis the data most strongly supports first, not the most dramatic-sounding one
2. Prefer high-confidence, low-effort fixes before big structural changes; if the data shows PR size doubled, a size-nudge bot is cheap and directly targets the mechanism, a full process overhaul isn't needed yet
3. Set a measurable target for each fix tied back to the original two symptoms (time-to-first-review, defect-escape rate), not just whether the fix shipped

90-day roadmap (adjust once the data confirms the actual cause)

• Days 1-15: instrument and collect the data above; don't change process yet, since changing multiple things at once makes it impossible to tell what worked
• Days 15-30: analyze, identify the one or two hypotheses the data actually supports, present the findings to the team
• Days 30-60: implement the highest-confidence, lowest-effort fix (for example, a PR-size nudge or reviewer load rebalancing) and hold everything else constant to isolate its effect
• Days 60-90: measure against the target (for example, time-to-first-review back under 24 hours, defect-escape rate back to baseline); if the metric hasn't moved, that hypothesis is disconfirmed and the next-highest-confidence one gets tried next

Worked example

Data collection shows: median PR size grew from 180 to 460 lines over the quarter, coinciding with a migration project that added several large, unavoidably broad refactor PRs; reviewer headcount and roster stayed flat; CI duration was stable. Defect data shows the doubled defects cluster specifically in the migration-related large PRs, not evenly across all PRs. This rules out the "reviewer burnout" and "tooling" hypotheses, since neither metric moved, and supports the "PR size" hypothesis instead. The remediation is scoped narrowly: a required PR-size-and-scope check specifically for the migration project (a splitting rule enforced via a bot comment plus a team norm), with a 60-day target of median PR size back under 250 lines and time-to-first-review back under 24 hours. Reviewer roster and CI tooling are left unchanged since the data didn't implicate them, avoiding a broad reorg that wouldn't have addressed the actual cause.

Trade-offs and pitfalls

• The single biggest mistake here is assuming causation, that slower review caused more defects, without checking whether both are downstream of a third cause like PR size; always check whether the defects actually trace back to the slow-reviewed PRs specifically, not just correlate in time
• Changing multiple things at once (new process AND new tooling AND reviewer reassignment) in the same 90 days makes it impossible to attribute the outcome to any one fix
• A remediation plan built before the data is in is a guess dressed up as a plan; resist the pressure to "do something now" before the first 15-day data-collection window closes
• Metrics chosen for the roadmap need a pre-quarter baseline to compare against; without one, "back to normal" isn't measurable