Lỗi hay gặp khi code review: comment, quy trình và bug lọt Trung cấp

Trang này gom các lỗi review hay gặp nhất, chia hai nhóm: lỗi của người review (comment, quy trình) và bug trong code mà reviewer phải bắt được, kèm comment nên để lại.

Lỗi khi viết comment và xử lý PR

Comment chỉ phán, không nói vấn đề

✕ Comment tồi

Sai rồi.

✓ Comment tốt

issue (blocking): Hai request hoàn tiền song song cùng đọc refunded = 0 rồi cùng ghi
→ có thể hoàn gấp đôi. Đổi sang
UPDATE payments SET refunded = refunded + $1 WHERE id = $2 AND refunded + $1 <= amount
và kiểm tra rowCount = 1?

Vì sao

• Tác giả đọc bản tồi không biết sai ở đâu, phải hỏi lại → mất thêm một vòng (một ngày nếu khác múi giờ).
• Bản tốt có kịch bản lỗi cụ thể, hướng sửa, và nhãn blocking nên không ai nhầm là tuỳ chọn.

Chặn PR vì nó khác cách bạn sẽ viết

✕ Comment tồi

Viết lại cái này theo Strategy pattern đi.   [Request changes]

✓ Comment tốt

thought: Nếu sau này có phương thức hoàn tiền thứ ba, switch này có thể đáng tách
ra Strategy. Hiện có 2 nhánh thì giữ nguyên là ổn.   [Approve]

Vì sao

PR đúng, đọc được, theo quy ước và làm code tốt lên thì phải approve — Google: “there is only better code”. Ý thích cá nhân đi kèm nitpick hoặc thought, không bao giờ blocking. Chặn vì sở thích đẩy team tới over-engineering và làm tác giả ngại mở PR.

Soi style bằng tay

✕ Comment tồi

L12: thiếu dấu chấm phẩy
L18: dùng nháy đơn
L31: thiếu dấu chấm phẩy
... (15 comment như vậy)

✓ Comment tốt

chore: Repo chưa chạy Prettier trong CI nên style lệch. Mình mở PR riêng thêm
`prettier --check .` vào pipeline. Không chặn PR này.

Vì sao

• 15 comment style tốn thời gian hai người và che mất comment issue thật nằm giữa.
• Thêm rule vào ESLint/Prettier một lần, CI fail tự động; con người chỉ review thứ máy không kiểm được.

Đồng ý "dọn ở PR sau"

✕ Reviewer chấp nhận

Tác giả: Flag isRefund hơi xấu, để PR sau mình dọn nhé.
Reviewer: OK, LGTM.

✓ Reviewer giữ lại

issue (blocking): processOrder(order, true, false, true) — flag thứ ba do PR này thêm.
Tách processRefund() riêng trong PR này nhé. Nợ cũ quanh đó thì mình tạo ticket PAY-640
gán cho bạn.

Vì sao

Google: cho người ta “clean things up later” là “a common way for codebases to degenerate” — cleanup sau thường bị việc khác đè mất. Độ phức tạp mới dọn trước khi merge; nợ có sẵn thì có ticket và người nhận.

LGTM trên PR 2.000 dòng

✕ Review tồi

LGTM   (approve sau 4 phút, 1.800 dòng, 41 file)

✓ Review tốt

Mình đã đọc thiết kế: hướng outbox ổn. 41 file thì mình không review kỹ được.
Đề xuất chia: (1) migration + repository, (2) RefundService + test, (3) endpoint.
Mình chưa đọc controller và DTO — chưa approve.

Vì sao

“LGTM” trên 2.000 dòng nghĩa là không ai review, nhưng lịch sử ghi là đã review — bug lọt production có tên bạn ở approval. Google khuyên yêu cầu chia PR; không chia được thì review thiết kế và nói rõ phần nào chưa đọc.

Chốt bất đồng trong call nhưng không ghi lại

✕ Thread bỏ lửng

(20 comment tranh luận enum vs bảng tra)
Reviewer: Ok để mình call với bạn.
[merged]

✓ Có comment kết luận

note: Kết luận call 15 phút (An, Bình): dùng text + CHECK constraint thay vì enum PG
hay bảng tra, vì trạng thái không do vận hành thêm và cần ràng buộc ở DB.
Đã thêm vào docs/conventions.md.

Vì sao

Sáu tháng sau, người đọc PR chỉ thấy 20 comment không kết luận và lại mở tranh luận từ đầu. Google yêu cầu ghi kết quả thảo luận thành comment trên PR “for future readers”.

Bug trong code mà reviewer phải bắt

Ba lỗi dưới đây đều pass test happy path và chạy ổn ở local — đó là lý do chúng lọt. Mỗi cặp kèm comment reviewer nên để lại.

Thiếu await trong try/catch

✕ Hay gặp

async createOrder(dto: CreateOrderDto) {
  try {
    this.orderRepo.save(dto); // thiếu await
    return { status: 'ok' };
  } catch (err) {
    throw new ConflictException('Đơn đã tồn tại');
  }
}

✓ Nên viết

async createOrder(dto: CreateOrderDto) {
  try {
    await this.orderRepo.save(dto);
    return { status: 'ok' };
  } catch (err) {
    throw new ConflictException('Đơn đã tồn tại');
  }
}

Vì sao

• Promise bị reject sau khi hàm đã trả ok → catch không bao giờ chạy, client nhận 200 dù đơn không được lưu.
• Rejection không ai bắt: Node 15+ mặc định --unhandled-rejections=throw, process có thể crash (đã chạy thử trên Node 24: exit code 1). return this.orderRepo.save(dto) trong try cũng không được catch của hàm này bắt — phải return await.
• Comment: issue (blocking): save() không await nên lỗi unique violation thoát khỏi try/catch. Bật @typescript-eslint/no-floating-promises để CI bắt lần sau?

N+1 query trong vòng lặp

✕ Hay gặp

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];
}

✓ Nên viết

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);

Vì sao

• 200 đơn = 201 query (đo trên PostgreSQL 17 local) so với 2 query. Local nhanh nên không ai thấy; production với vài ms mỗi round-trip và 5.000 đơn thì endpoint chậm hàng chục giây và giữ connection pool.
• = ANY($1) với mảng rỗng trả 0 dòng, không lỗi. Knex: whereIn('id', ids); Prisma: findMany({ where: { id: { in: ids } } }) hoặc include.
• Comment: issue (blocking): query trong for → 1 + N query theo số đơn. Gom id rồi query một lần với ANY($1)?

Query không giới hạn số dòng

✕ Hay gặp

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

✓ Nên viết

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 });
});

Vì sao

• Hôm nay bảng có 2.000 dòng, test pass. Năm sau 8 triệu dòng: một request kéo cả bảng vào RAM, OOM pod hoặc timeout, và ai cũng gọi được.
• Luôn có limit mặc định và trần cứng (client gửi limit=1000000 vẫn chỉ được 200). Prisma: take + cursor; Kysely: .limit().
• Comment: issue (blocking): endpoint trả toàn bộ orders theo status, không limit. Thêm keyset pagination với trần 200?

Tóm tắt

Tóm tắt

• Comment phải nói vấn đề cụ thể và có chặn hay không; sở thích cá nhân không bao giờ là blocking.
• Style giao cho linter; “dọn sau” chỉ chấp nhận với nợ cũ có ticket.
• PR quá lớn thì trả lại kèm cách chia, không LGTM cho có.
• Bất đồng chốt ngoài PR thì ghi kết luận vào PR.
• Ba bug hay lọt qua review: thiếu await, N+1 trong vòng lặp, query không LIMIT — đều pass test happy path.

Tự kiểm tra

1. Reviewer thấy thiết kế sai ở file đầu tiên trong PR 15 file. Nên làm gì trước khi đọc tiếp, và vì sao?
2. Khác biệt giữa suggestion (non-blocking): và issue (blocking): với tác giả là gì? Khi nào một comment về tên biến xứng đáng là blocking?
3. Tác giả nói “để mình dọn ở PR sau”. Khi nào bạn đồng ý, khi nào không?
4. Vì sao “số comment mỗi reviewer mỗi tuần” là KPI có hại?
5. PR 1.400 dòng gồm đổi tên module và thêm tính năng mới. Bạn đề xuất chia thế nào?

Tiếp theo: quay về Tổng quan code review, hoặc xem Clean code để biết reviewer nên nhìn gì trong service logic.