Common code review mistakes: comments, process and bugs that slip through Intermediate

This page collects the most common review mistakes in two groups: reviewer mistakes (comments, process) and bugs in code that reviewers must catch, along with the comment to leave.

Mistakes in writing comments and handling PRs

A comment that only passes judgment without naming the problem

✕ Bad comment

This is wrong.

✓ Good comment

issue (blocking): Two concurrent refund requests both read refunded = 0 and both write
→ this can refund twice. Switch to
UPDATE payments SET refunded = refunded + $1 WHERE id = $2 AND refunded + $1 <= amount
and check rowCount = 1?

Why

• The author reading the bad version doesn’t know what’s wrong and has to ask → one more round (a whole day across time zones).
• The good version has a concrete failure scenario, a fix direction, and a blocking label so nobody mistakes it for optional.

Blocking a PR because it differs from how you'd write it

✕ Bad comment

Rewrite this with the Strategy pattern.   [Request changes]

✓ Good comment

thought: If a third refund method appears later, this switch may be worth extracting
into a Strategy. With 2 branches now, keeping it is fine.   [Approve]

Why

If the PR is correct, readable, follows conventions and improves the code, approve it — Google: “there is only better code”. Personal preferences go with nitpick or thought, never blocking. Blocking on preference pushes the team toward over-engineering and makes authors reluctant to open PRs.

Checking style by hand

✕ Bad comment

L12: missing semicolon
L18: use single quotes
L31: missing semicolon
... (15 comments like this)

✓ Good comment

chore: The repo doesn't run Prettier in CI yet, so style drifts. I'll open a separate PR
adding `prettier --check .` to the pipeline. Not blocking this PR.

Why

• 15 style comments waste two people’s time and hide the real issue comment in the middle.
• Add the rule to ESLint/Prettier once and CI fails automatically; humans only review what machines can’t check.

Agreeing to "clean it up in the next PR"

✕ Reviewer accepts

Author: The isRefund flag is a bit ugly, I'll clean it up in the next PR.
Reviewer: OK, LGTM.

✓ Reviewer holds the line

issue (blocking): processOrder(order, true, false, true) — the third flag is added by this PR.
Please extract processRefund() in this PR. For the older debt around it, I'll create ticket PAY-640
assigned to you.

Why

Google: letting people “clean things up later” is “a common way for codebases to degenerate” — later cleanups usually get buried by other work. New complexity is cleaned up before merging; existing debt gets a ticket and an assignee.

LGTM on a 2,000-line PR

✕ Bad review

LGTM   (approved after 4 minutes, 1,800 lines, 41 files)

✓ Good review

I've read the design: the outbox approach is fine. With 41 files I can't review this thoroughly.
Suggested split: (1) migration + repository, (2) RefundService + tests, (3) endpoint.
I haven't read the controller and DTOs — not approving yet.

Why

“LGTM” on 2,000 lines means nobody reviewed it, yet history says it was reviewed — when a bug reaches production, your name is on the approval. Google advises asking for the PR to be split; if it can’t be, review the design and say clearly which parts you haven’t read.

Settling a disagreement on a call but not recording it

✕ Thread left hanging

(20 comments debating enum vs lookup table)
Reviewer: OK, let me get on a call with you.
[merged]

✓ With a conclusion comment

note: Conclusion of a 15-minute call (An, Binh): use text + a CHECK constraint instead of a PG enum
or a lookup table, because statuses aren't added by operators and need a DB-level constraint.
Added to docs/conventions.md.

Why

Six months later, someone reading the PR only sees 20 comments without a conclusion and reopens the debate from scratch. Google asks for the outcome of discussions to be recorded as a comment on the PR “for future readers”.

Bugs in code that reviewers must catch

All three bugs below pass happy-path tests and run fine locally — that’s why they slip through. Each pair comes with the comment a reviewer should leave.

Missing await inside try/catch

✕ Common

async createOrder(dto: CreateOrderDto) {
  try {
    this.orderRepo.save(dto); // missing await
    return { status: 'ok' };
  } catch (err) {
    throw new ConflictException('Order already exists');
  }
}

✓ Prefer

async createOrder(dto: CreateOrderDto) {
  try {
    await this.orderRepo.save(dto);
    return { status: 'ok' };
  } catch (err) {
    throw new ConflictException('Order already exists');
  }
}

Why

• The promise rejects after the function has already returned ok → the catch never runs, and the client gets 200 even though the order wasn’t saved.
• Nobody catches the rejection: Node 15+ defaults to --unhandled-rejections=throw, so the process can crash (tested on Node 24: exit code 1). return this.orderRepo.save(dto) inside try isn’t caught by this function’s catch either — it must be return await.
• Comment: issue (blocking): save() isn't awaited, so the unique violation escapes the try/catch. Enable @typescript-eslint/no-floating-promises so CI catches it next time?

N+1 queries in a loop

✕ Common

const { rows: orders } = await pool.query(
  'SELECT * FROM orders WHERE created_at >= $1', [from]);

for (const order of orders) {
  const { rows } = await pool.query(
    'SELECT * FROM customers WHERE id = $1', [order.customer_id]);
  order.customer = rows[0];
}

✓ Prefer

const { rows: orders } = await pool.query(
  'SELECT * FROM orders WHERE created_at >= $1', [from]);

const ids = [...new Set(orders.map((o) => o.customer_id))];
const { rows: customers } = await pool.query(
  'SELECT * FROM customers WHERE id = ANY($1::int[])', [ids]);

const byId = new Map(customers.map((c) => [c.id, c]));
for (const order of orders) order.customer = byId.get(order.customer_id);

Why

• 200 orders = 201 queries (measured on local PostgreSQL 17) versus 2 queries. Locally it’s fast, so nobody notices; in production, with a few ms per round-trip and 5,000 orders, the endpoint takes tens of seconds and hogs the connection pool.
• = ANY($1) with an empty array returns 0 rows, no error. Knex: whereIn('id', ids); Prisma: findMany({ where: { id: { in: ids } } }) or include.
• Comment: issue (blocking): query inside a for loop → 1 + N queries by order count. Collect the ids and query once with ANY($1)?

A query with no row limit

✕ Common

router.get('/orders', async (req, res) => {
  const { status = 'pending' } = req.query;
  const orders = await db('orders')
    .where({ status })
    .orderBy('id');
  res.json(orders);
});

✓ Prefer

router.get('/orders', async (req, res) => {
  const { status = 'pending' } = req.query;
  const limit = Math.min(Math.max(Number(req.query.limit) || 50, 1), 200);
  const after = Number(req.query.after) || 0;

  const orders = await db('orders')
    .where({ status })
    .andWhere('id', '>', after) // keyset pagination
    .orderBy('id')
    .limit(limit);

  res.json({ data: orders, nextAfter: orders.at(-1)?.id ?? null });
});

Why

• Today the table has 2,000 rows and tests pass. Next year it has 8 million: one request pulls the whole table into RAM, OOMs the pod or times out, and anyone can call it.
• Always have a default limit and a hard cap (a client sending limit=1000000 still only gets 200). Prisma: take + cursor; Kysely: .limit().
• Comment: issue (blocking): the endpoint returns all orders by status with no limit. Add keyset pagination with a cap of 200?

Summary

Key takeaways

• A comment must state a specific problem and whether it blocks; personal preference is never blocking.
• Hand style to the linter; accept “clean up later” only for existing debt with a ticket.
• Hand back an oversized PR with a way to split it; don’t LGTM it for show.
• When a disagreement is settled outside the PR, record the conclusion in the PR.
• Three bugs that often slip through review: missing await, N+1 in loops, queries without LIMIT — all pass happy-path tests.

Self-check

1. A reviewer spots a design flaw in the first file of a 15-file PR. What should they do before reading on, and why?
2. What’s the difference for the author between suggestion (non-blocking): and issue (blocking):? When does a comment about a variable name deserve to be blocking?
3. The author says “I’ll clean it up in the next PR”. When do you agree, and when not?
4. Why is “comments per reviewer per week” a harmful KPI?
5. A 1,400-line PR includes renaming a module and adding a new feature. How would you suggest splitting it?

Next: go back to the Code review overview, or see Clean code for what reviewers should look at in service logic.