Code Review Interview Questions

You are asked to review a PR in a part of the codebase you have never worked in. How do you give a useful review?

Review what you can actually judge and be explicit about what you cannot. You can still assess whether the change matches its stated intent, whether the tests would fail if the code were wrong, whether errors and edge cases are handled, whether naming and structure are followable by the next person, and whether anything looks unsafe. Read the surrounding code and the tests before the diff. Ask questions rather than asserting — "what happens if this is called twice?" is useful even when you do not know the answer. Then say plainly that you cannot vouch for the domain logic and name who can.

The two failure modes are rubber-stamping, which converts a review into a formality, and nitpicking style because it is the only thing you feel qualified to comment on, which wastes the author's time and buries anything real. Saying "I reviewed X and Y, someone needs to check Z" is the honest output and is more valuable than an unqualified approval — it also surfaces the bus-factor problem, because if no second reviewer exists for Z, that is the finding.

Review this in-memory cache added to speed up a hot endpoint

const cache = {};

export async function getUser(id) {
  if (cache[id]) return cache[id];
  const res = await fetch('/api/users/' + id);
  const user = await res.json();
  cache[id] = user;
  return user;
}

Cache the promise rather than the resolved value, only cache successful responses, give entries a TTL or an explicit invalidation hook, bound the size, and use a `Map` instead of an object literal.

Flag, roughly in order of severity. There is no expiry and no invalidation, so a user who changes their name shows the old one until reload — a cache without an eviction story is a correctness change, not a performance change. There is no in-flight deduplication: ten simultaneous calls for the same id issue ten requests, because the entry is only written after the await; caching the promise rather than the value fixes both that and the duplicate work. Failures are not handled — a non-2xx response still parses and caches whatever the error body was, poisoning the entry permanently. The object grows without bound, which is a leak in a long-lived tab. And `cache[id]` on a plain object inherits from `Object.prototype`, so an id of `"constructor"` returns a function; `new Map()` or `Object.create(null)` avoids it. The truthiness check also means a legitimately falsy cached value is never a hit.

What is code review actually for, given that CI already checks correctness?

CI checks what can be automated: does it compile, do the tests pass, is it formatted, does it lint. Review is for the things a machine cannot judge. Is the approach right, or does it solve the wrong problem? Will this be understandable in six months by someone without the author’s context? Does it fit the existing patterns, or silently introduce a third way of doing something? Are the tests actually meaningful, or do they pass regardless? Are there consequences the author could not see — a security boundary, a performance path, another team’s assumption? Review is also how knowledge spreads: the reviewer learns the change exists, which is why the codebase does not end up with one owner per file.

Drawing this line is what keeps review valuable: once formatting, linting and test results are automated, spending review attention on them is pure waste and crowds out the judgement only a human can supply. The questions left are about approach, comprehensibility later, consistency with existing patterns, and whether the tests would actually fail if the code were wrong.

The author disagrees with your blocking comment and you are not convinced. What now?

Separate the disagreement from the blockage. First check you have understood their reasoning — restating it back often resolves it, because many disagreements are actually different assumptions about the requirement. If it stands, make the cost concrete: what specifically breaks, under what input, and how likely is it. If the concern is a real correctness or security issue, holding the block is the right call and worth escalating to a third opinion rather than relitigating in comments. If it is a design preference, say so explicitly and unblock — preferences should not gate a merge. Where the disagreement reveals a missing team convention, the durable fix is to write the convention down so the same argument is not repeated per PR.

Being able to say "this is a preference, I am unblocking" is as important as being able to hold a genuine blocker.

What should a reviewer look at first?

Formatting should be automated so review never spends attention on it. Human review is best used on things tools cannot judge: whether the logic is right, whether it is safe, and whether the design will be workable in six months.

Reviews in your team take two days. What would you change, without lowering the bar?

Attack the queue rather than the standard. Make PRs smaller by default — an author-side norm of one concern per PR does more for latency than any process change. Set an explicit expectation for first response (not full approval) so work is not blocked on someone’s deep-work block. Automate everything mechanical — formatting, lint, types, tests — so human attention is spent only on things tools cannot judge. Distinguish blocking from non-blocking comments so an author is not waiting on preferences. For large or risky changes, prefer a short synchronous walkthrough over a long asynchronous thread. And measure time-to-first-review rather than time-to-merge, because that is the number that tells you whether people are stuck.

The distinction between time-to-first-review and time-to-merge is the practical insight; the latter conflates review latency with rework.

Review this date handling

const due = new Date(task.dueDate);
const isOverdue = due < new Date();
return <span>{due.toLocaleDateString()} {isOverdue && '(overdue)'}</span>;

Several correctness issues. `new Date(string)` parsing is only reliable for full ISO-8601 with an offset — a bare `YYYY-MM-DD` is parsed as UTC while `YYYY-MM-DDTHH:mm` is parsed as local, so the same field can shift a day depending on format and timezone. Comparing against `new Date()` mixes a date-only concept with an instant: a task due today is "overdue" from 00:00 onward in some timezones. `toLocaleDateString()` renders in the viewer’s locale and timezone, which is right for display but makes this non-deterministic under SSR and in tests. And an invalid date fails silently as `Invalid Date` rather than raising. I would parse explicitly with a known format, decide whether the field is an instant or a calendar date and compare like with like, and pass explicit locale/timeZone options for display.

Date bugs are the archetypal "works for me" defect — every issue here is invisible to a developer in the same timezone as the test data.

Why do very large pull requests get worse reviews?

A 2,000-line diff gets skimmed, not read; comments cluster in the first few files and thin out after that. Splitting into reviewable units — refactor separately from behaviour change — gets each part genuine attention.