Code review hiệu quả: checklist cho reviewer, cách viết PR dễ review và quy trình cho team nhỏ

Một kịch bản quen thuộc ở nhiều team nhỏ: có hai kiểu PR. Kiểu thứ nhất: 40 dòng đổi tên biến, nhận 15 comment về khoảng trắng và thứ tự import. Kiểu thứ hai: 1.800 dòng gồm migration, API mới và cả màn hình, treo ba ngày, rồi được approve bằng đúng một chữ "LGTM". Bug nghiêm trọng gần như luôn nằm ở kiểu thứ hai, chẳng hạn một route cho phép user sửa hóa đơn của user khác.

Vấn đề không phải team lười review. Vấn đề là sức chú ý của reviewer bị dồn sai chỗ: vào những thứ máy làm được, trong khi những PR quá lớn để đọc kỹ thì được cho qua. Code review hiệu quả là thiết kế lại quy trình để con người dành sức cho thứ chỉ con người làm được: logic, bảo mật, dữ liệu và thiết kế.

Bài này là quy trình mình dùng cho team nhỏ (3-8 người) trên GitHub: review để làm gì, cách chia PR nhỏ và stacked PR, template mô tả PR, self-review, checklist cho reviewer theo thứ tự ưu tiên kèm ví dụ code cụ thể, cách viết comment, phần tự động hóa (CI, CODEOWNERS, branch protection), kỳ vọng về thời gian phản hồi, và cách review code được viết với trợ giúp của AI.

TL;DR

Review để tìm lỗi, chia sẻ kiến thức và giữ thiết kế nhất quán. Style thì giao cho Prettier và ESLint.
PR nhỏ, một mục đích; việc lớn thì chia thành stacked PR. Mô tả PR theo template: bối cảnh, thay đổi, cách test, rủi ro, cách rollback.
Tác giả tự review diff trước khi gán reviewer.
Reviewer đọc theo thứ tự: đúng đắn, bảo mật, dữ liệu/migration, xử lý lỗi, test, dễ đọc, hiệu năng.
Gắn nhãn comment (issue (blocking):, nitpick:, question:) để tác giả biết cái gì phải sửa.
Phản hồi lần đầu trong một ngày làm việc. Code có AI hỗ trợ được review với tiêu chuẩn cao hơn, không thấp hơn.

Code review để làm gì (và không để làm gì)

Ba mục tiêu đáng tốn thời gian của con người:

Tìm lỗi trước production. Lỗi logic, thiếu kiểm tra quyền, race condition, migration khóa bảng. Đây là những thứ CI không bắt được vì test chỉ kiểm tra điều tác giả nghĩ tới.
Chia sẻ kiến thức. Sau mỗi PR, ít nhất hai người hiểu đoạn code đó. Khi tác giả nghỉ phép, đi on-call, hay nghỉ việc, hệ thống không bị phụ thuộc vào một người. Reviewer junior cũng học được cách người khác giải quyết vấn đề.
Giữ thiết kế nhất quán. Code mới đi theo pattern đã có (xử lý lỗi ở boundary, gọi DB qua repository, format tiền qua lib/money.ts) thay vì mỗi người một kiểu.

Những thứ code review không dành cho:

Style và format. Dấu nháy đơn hay kép, thứ tự import, độ dài dòng: Prettier và ESLint quyết định, chạy trong pre-commit và CI. Một comment về style là một dấu hiệu cần thêm rule, không phải thêm comment.
Thiết kế lại từ đầu ở bước cuối. Nếu reviewer nghĩ cả hướng tiếp cận sai, đó là thất bại của bước trước: thay đổi lớn cần một design doc ngắn hoặc một cuộc trao đổi 15 phút trước khi viết code.
Thay thế test. Reviewer không chạy được mọi nhánh trong đầu. Nếu một hành vi quan trọng, nó cần test, và reviewer yêu cầu test đó.
Gác cổng hay thể hiện. Approve không phải là "mình sẽ viết y hệt". Nó nghĩa là "code này đúng, an toàn, và team bảo trì được".

PR nhỏ và stacked PR

PR nhỏ được review kỹ hơn, nhanh hơn, và khi có lỗi thì revert gọn hơn. Hướng dẫn code review công khai của Google cũng đặt "change nhỏ" là một nguyên tắc cốt lõi. Mình dùng mốc ước lượng: dưới khoảng 400 dòng thay đổi (không tính lockfile và file sinh tự động), một mục đích duy nhất, mô tả được bằng một câu.

Cách chia một tính năng lớn:

Refactor tách riêng. Đổi tên, di chuyển file, trích hàm thành một PR "không thay đổi hành vi", review rất nhanh vì reviewer chỉ cần xác nhận đúng là không đổi hành vi.
Theo lớp. Migration và model trước, rồi API, rồi UI. Mỗi lớp merge được độc lập nếu lớp sau chưa dùng tới.
Feature flag. Merge code chưa hoàn chỉnh vào main sau một flag tắt sẵn, thay vì giữ một nhánh sống ba tuần.

Stacked PR là chuỗi PR mà mỗi PR dùng nhánh của PR trước làm base. Reviewer xem từng phần nhỏ, còn bạn không phải chờ PR 1 merge mới làm tiếp PR 2:

# Ba nhánh xếp chồng: query -> API -> UI
git switch -c feat/invoice-export-1-query origin/main
# ... code, commit
git switch -c feat/invoice-export-2-api
# ... code, commit
git switch -c feat/invoice-export-3-ui
# ... code, commit

git push -u origin feat/invoice-export-1-query feat/invoice-export-2-api feat/invoice-export-3-ui
gh pr create --base main --head feat/invoice-export-1-query \
  --title "Invoice export (1/3): query hóa đơn theo tháng" --body-file .github/pr-notes/1.md
gh pr create --base feat/invoice-export-1-query --head feat/invoice-export-2-api \
  --title "Invoice export (2/3): API route /api/invoices/export"
gh pr create --base feat/invoice-export-2-api --head feat/invoice-export-3-ui \
  --title "Invoice export (3/3): nút tải CSV"

# Reviewer yêu cầu sửa PR 1: commit lên nhánh 1, rồi rebase cả chồng một lần
git switch feat/invoice-export-1-query
git commit -am "Handle invoices without customer email"
git switch feat/invoice-export-3-ui
git rebase --update-refs feat/invoice-export-1-query     # nhánh 2 được cập nhật theo
git push --force-with-lease origin feat/invoice-export-2-api feat/invoice-export-3-ui

# PR 1 đã squash merge vào main: đưa phần còn lại lên main
git fetch origin
git rebase --update-refs --onto origin/main feat/invoice-export-1-query feat/invoice-export-3-ui
git push --force-with-lease origin feat/invoice-export-2-api feat/invoice-export-3-ui

--update-refs (Git 2.38+) là thứ làm stacked PR dễ chịu: rebase nhánh trên cùng sẽ tự dời các nhánh trung gian theo. Khi bạn xóa nhánh của PR 1 sau khi merge, GitHub tự đổi base của PR 2 sang main. Nếu team làm stacked PR thường xuyên, các công cụ như Graphite hay git-town tự động hóa phần rebase và push này.

Cái giá của stacked PR: nếu reviewer yêu cầu đổi thiết kế ở PR 1, mọi PR phía trên phải làm lại. Vì vậy chỉ xếp chồng khi hướng đi đã được thống nhất, và giữ chồng ngắn (2-4 PR).

Mô tả PR: một template đủ dùng

Reviewer mất phần lớn thời gian để hiểu "PR này nhằm làm gì và vì sao làm thế này". Mô tả tốt cắt bớt phần đó. Đặt template ở .github/pull_request_template.md, GitHub sẽ tự điền vào mọi PR mới:

## Bối cảnh
<!-- Vì sao cần thay đổi này? Link ticket/issue. Một hai câu. -->

## Thay đổi
<!-- Làm gì, theo cách nào. Nếu có lựa chọn thay thế đã cân nhắc, ghi vì sao không chọn. -->

## Cách test
<!-- Các bước reviewer tự kiểm tra được. Test tự động nào đã thêm. -->
- [ ] Unit test
- [ ] Đã chạy thử trên preview deployment

## Ảnh chụp / video
<!-- Bắt buộc nếu thay đổi UI: trước và sau. -->

## Rủi ro
<!-- Phần nào dễ hỏng nhất? Ảnh hưởng tới dữ liệu, hiệu năng, bảo mật? -->

## Rollback
<!-- Revert là đủ? Có migration không đảo ngược được? Có feature flag để tắt? -->

## Gợi ý cho reviewer
<!-- Nên đọc file nào trước. Phần nào cần xem kỹ, phần nào chỉ là đổi tên/di chuyển. -->

Một ví dụ đã điền, để thấy mức chi tiết mình mong đợi:

## Bối cảnh
Kế toán cần tải hóa đơn theo tháng dạng CSV để đối soát (SHOP-482). Hiện họ copy tay từ màn hình.

## Thay đổi
- Thêm `GET /api/invoices/export?month=2026-10`, stream CSV thay vì build cả file trong bộ nhớ
  (tháng cao điểm có khoảng 40k hóa đơn).
- Chỉ role `accountant` và `admin` được gọi.
- Không dùng thư viện CSV mới; tự escape theo RFC 4180 trong `lib/csv.ts` (có test).

## Cách test
1. Đăng nhập bằng `accountant@local.test` trên preview.
2. Vào Hóa đơn > Xuất CSV, chọn tháng 10/2026.
3. Mở file bằng Excel: tên khách hàng có dấu phẩy và tiếng Việt hiển thị đúng.

## Rủi ro
Query quét theo `issued_at`; đã thêm index (migration 0042, tạo CONCURRENTLY).

## Rollback
Revert PR. Migration chỉ thêm index, có thể giữ lại hoặc `DROP INDEX CONCURRENTLY`.

## Gợi ý cho reviewer
Đọc `lib/csv.ts` và test của nó trước. `components/export-button.tsx` chỉ là UI.

Mục "Rollback" là mục mình thấy bị bỏ trống nhiều nhất và hữu ích nhất. Viết nó buộc tác giả nghĩ trước về migration không đảo ngược được, và reviewer biết khi nào cần đặc biệt cẩn thận.

Self-review trước khi nhờ người khác

Trước khi gán reviewer, đọc diff của chính mình như thể người khác viết. Rất nhiều vấn đề lộ ra ngay ở bước này, và mỗi vấn đề tự bắt được là một vòng review tiết kiệm được cho cả hai bên.

git fetch origin
git diff origin/main...HEAD --stat                             # có file nào lạc vào PR?
git diff origin/main...HEAD -- . ':(exclude)pnpm-lock.yaml'    # đọc toàn bộ diff, bỏ lockfile
git log origin/main..HEAD --oneline                            # commit kể một câu chuyện hợp lý?
pnpm lint && pnpm tsc --noEmit && pnpm test

gh pr create --draft --fill     # mở draft, đọc lại diff trên giao diện GitHub
gh pr ready                     # chuyển sang ready khi đã tự review xong

Đọc lại trên giao diện GitHub (ở chế độ draft) khác hẳn đọc trong editor: bạn thấy đúng thứ reviewer sẽ thấy. Những thứ mình tìm khi self-review:

console.log, code comment-out, TODO không có ticket.
File không liên quan: thay đổi format do editor, lockfile đổi vì cài thử package.
Tên hàm và biến có còn đúng sau khi logic đã đổi qua vài lần sửa.
Nhánh lỗi: hàm trả gì khi input rỗng, khi DB trả null, khi API ngoài timeout.

Mẹo nhỏ: tự để comment trên PR của chính mình ở những chỗ quan trọng ("Chỗ này cố ý không dùng transaction vì..."). Reviewer không phải hỏi, còn bạn phát hiện ra những chỗ mà nếu phải giải thích thì có lẽ nên viết lại cho rõ hơn.

Checklist cho reviewer theo thứ tự ưu tiên

Thứ tự đọc PR của mình: mô tả PR, rồi test (để biết tác giả định làm gì), rồi phần lõi, cuối cùng là phần phụ (UI, wiring). Thứ tự ưu tiên khi đánh giá:

Đúng đắn. Code có làm đúng điều mô tả nói không? Input rỗng, null, trùng lặp, đồng thời thì sao?
Bảo mật. Có kiểm tra xác thực và quyền sở hữu tài nguyên không? Input có được validate? Có secret nào bị log hay trả về client?
Dữ liệu và migration. Migration có khóa bảng lớn không? Có tương thích với code cũ đang chạy trong lúc deploy? Đảo ngược được không?
Xử lý lỗi. Lỗi có bị nuốt mất? Promise có được await? Người dùng nhận thông báo gì?
Test. Test có kiểm tra hành vi (không phải chi tiết cài đặt) và có phủ nhánh lỗi không?
Dễ đọc. Người mới vào team có hiểu được không? Tên có nói đúng ý? Có đi theo pattern sẵn có?
Hiệu năng. N+1 query, query không giới hạn, việc nặng chạy đồng bộ trong request, bundle phía client phình to.

Thứ tự này quan trọng vì sức chú ý có hạn. Một PR có lỗ hổng phân quyền mà reviewer để lại 10 comment về tên biến là một lần review thất bại.

Bài tập: đây là kiểu route rất hay gặp trong PR, bạn thấy bao nhiêu vấn đề?

// app/api/invoices/[id]/route.ts
import { NextResponse } from 'next/server'
import { db } from '@/lib/db'
import { getSession } from '@/lib/auth'
import { sendInvoiceEmail } from '@/lib/email'

export async function PATCH(req: Request, { params }: { params: Promise<{ id: string }> }) {
  const session = await getSession()
  const { id } = await params
  const body = await req.json()

  const invoice = await db.invoice.findUnique({ where: { id } })
  if (invoice.status === 'paid') {
    return NextResponse.json({ error: 'Invoice already paid' }, { status: 400 })
  }

  await db.invoice.update({ where: { id }, data: body })
  sendInvoiceEmail(invoice.customerEmail, id)

  return NextResponse.json({ ok: true })
}

Theo thứ tự ưu tiên:

Bảo mật: session được lấy nhưng không kiểm tra. Người chưa đăng nhập gọi được, và không có kiểm tra hóa đơn thuộc về ai, nên user A sửa được hóa đơn của user B chỉ bằng cách đổi id (IDOR).
Bảo mật: data: body là mass assignment. Client gửi { "status": "paid", "totalAmount": 0 } là xong.
Đúng đắn: invoice có thể null khi id không tồn tại, dòng invoice.status ném TypeError và trả 500.
Đúng đắn: kiểm tra status rồi mới update là hai bước tách rời. Hai request đồng thời (hoặc một request và một webhook thanh toán) có thể cùng vượt qua bước kiểm tra.
Xử lý lỗi: sendInvoiceEmail không được await. Nếu nó reject, đó là unhandled rejection, và email thất bại mà không ai biết.

Phiên bản sau review:

// app/api/invoices/[id]/route.ts
import { NextResponse } from 'next/server'
import { z } from 'zod'
import { db } from '@/lib/db'
import { getSession } from '@/lib/auth'
import { enqueueInvoiceEmail } from '@/lib/jobs'

const UpdateInvoice = z.object({
  dueDate: z.coerce.date().optional(),
  note: z.string().max(500).optional(),
})

export async function PATCH(req: Request, { params }: { params: Promise<{ id: string }> }) {
  const session = await getSession()
  if (!session) {
    return NextResponse.json({ error: 'Unauthorized' }, { status: 401 })
  }

  const { id } = await params
  const parsed = UpdateInvoice.safeParse(await req.json())
  if (!parsed.success) {
    return NextResponse.json({ error: parsed.error.issues }, { status: 422 })
  }

  // Điều kiện sở hữu và trạng thái nằm ngay trong câu UPDATE: một bước, không race
  const result = await db.invoice.updateMany({
    where: { id, ownerId: session.userId, status: { not: 'paid' } },
    data: parsed.data,
  })
  if (result.count === 0) {
    return NextResponse.json({ error: 'Invoice not found or already paid' }, { status: 409 })
  }

  // Đưa vào queue: có retry, và lỗi email không làm hỏng request
  await enqueueInvoiceEmail({ invoiceId: id })
  return NextResponse.json({ ok: true })
}

Phần dữ liệu và migration cần một con mắt riêng, vì lỗi ở đây gây downtime hoặc mất dữ liệu chứ không chỉ một request lỗi. Với PostgreSQL:

-- Cần hỏi lại: CREATE INDEX thường chặn mọi thao tác ghi vào bảng trong suốt quá trình tạo index
CREATE INDEX idx_invoices_issued_at ON invoices (issued_at);

-- An toàn hơn cho bảng lớn đang nhận ghi (không chạy được trong transaction block)
CREATE INDEX CONCURRENTLY idx_invoices_issued_at ON invoices (issued_at);

-- Cần hỏi lại: đổi tên cột trong một lần deploy làm code cũ (vẫn chạy trong lúc rolling update) lỗi ngay
ALTER TABLE invoices RENAME COLUMN note TO internal_note;
-- An toàn hơn (expand/contract): thêm cột mới, ghi cả hai, backfill, chuyển đọc sang cột mới, xóa cột cũ ở PR sau

Câu hỏi mình luôn đặt cho PR có migration: "Nếu deploy xong rồi phải rollback code, schema mới có làm code cũ chạy sai không?"

Viết comment: Conventional Comments và GitHub suggestions

Cùng một nội dung, cách viết quyết định tác giả sửa ngay hay phòng thủ. Quy tắc của mình: nói rõ vấn đề, nói vì sao nó quan trọng, đề xuất hướng sửa, và gắn nhãn cho biết có bắt buộc hay không.

Nhiều team dùng tiền tố ngắn như nit:, question:, blocking:. Conventional Comments chuẩn hóa thành dạng nhãn (decoration): nội dung, với các nhãn như praise, nitpick, suggestion, issue, todo, question, thought, và decoration (blocking), (non-blocking), (if-minor):

praise: Tách `calculateVat` thành hàm thuần làm test bên dưới rất dễ đọc.

nitpick (non-blocking): `data` → `overdueInvoices` sẽ rõ nghĩa hơn ở đây.

question: Nếu `customer.email` là null (khách vãng lai) thì `enqueueInvoiceEmail` làm gì?

issue (blocking): `data: body` cho phép client sửa cả `status` và `totalAmount`.
Cần whitelist field bằng schema trước khi update, xem `lib/validation/invoice.ts`.

suggestion: Dùng `updateMany` với điều kiện `status: { not: 'paid' }` để kiểm tra và cập nhật
trong một câu, tránh race giữa hai request đồng thời.

thought (non-blocking): Nếu sau này có thêm export PDF, có thể tách `InvoiceFormatter`.
Chưa cần trong PR này.

todo: Thêm test cho hóa đơn đã `paid` (phải trả 409).

Lợi ích lớn nhất của nhãn: tác giả biết ngay comment nào phải xử lý trước khi merge (blocking, issue, todo) và comment nào có thể bỏ qua (nitpick, thought). Không có nhãn, mọi comment trông như yêu cầu bắt buộc, và một PR với 12 nit trông như bị từ chối.

GitHub suggestion. Khi cách sửa ngắn và rõ, đừng mô tả bằng lời, hãy viết luôn code. Trong comment review, chọn một hoặc nhiều dòng (kéo dấu + ở lề), rồi dùng khối suggestion:

issue (blocking): `invoice` là `null` khi id không tồn tại, dòng dưới sẽ ném TypeError và trả 500.

```suggestion
  const invoice = await db.invoice.findUnique({ where: { id } })
  if (!invoice) {
    return NextResponse.json({ error: 'Invoice not found' }, { status: 404 })
  }
```

Tác giả bấm "Commit suggestion" để áp dụng ngay, hoặc "Add suggestion to batch" để gom nhiều suggestion thành một commit. Nội dung trong khối suggestion thay thế đúng các dòng bạn đã chọn, nên chọn sai phạm vi là suggestion sai. Dùng cho thay đổi nhỏ và cục bộ. Thay đổi lớn hơn thì mô tả hướng đi và để tác giả tự viết.

Vài nguyên tắc về giọng điệu:

Comment về code, không về người: "hàm này bỏ qua trường hợp null" thay vì "bạn quên trường hợp null".
Câu hỏi thật sự là câu hỏi. Nếu bạn đã biết câu trả lời và muốn sửa, viết issue hoặc suggestion, đừng giả vờ hỏi.
Khen cụ thể. "Good job" không mang thông tin gì. "Test cho trường hợp tháng có 0 hóa đơn rất đáng giá" cho tác giả biết nên tiếp tục làm gì.
Bất đồng quá hai vòng comment thì gọi nhau 10 phút, rồi ghi kết luận vào PR cho người đọc sau.

Tự động hóa để con người chỉ review logic

Mọi thứ máy kiểm tra được thì để máy kiểm tra, và chặn merge nếu không đạt. Khi đó reviewer không cần nhắc về format, type error hay test fail.

CI chạy trên mọi PR:

# .github/workflows/ci.yml
name: CI
on:
  pull_request:
  push:
    branches: [main]
permissions:
  contents: read
concurrency:
  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
  cancel-in-progress: ${{ github.event_name == 'pull_request' }}
jobs:
  checks:
    runs-on: ubuntu-latest
    timeout-minutes: 15
    steps:
      - uses: actions/checkout@v7
      - uses: pnpm/action-setup@v6
      - uses: actions/setup-node@v7
        with:
          node-version-file: .nvmrc
          cache: pnpm
      - run: pnpm install --frozen-lockfile
      - run: pnpm prettier --check .
      - run: pnpm lint
      - run: pnpm tsc --noEmit
      - run: pnpm vitest run

Một workflow cảnh báo PR quá lớn, dùng số liệu có sẵn trong payload của sự kiện pull_request nên không cần token:

# .github/workflows/pr-size.yml
name: PR size
on:
  pull_request:
    types: [opened, synchronize, reopened]
permissions:
  contents: read
jobs:
  size:
    runs-on: ubuntu-latest
    steps:
      - name: Cảnh báo PR lớn
        env:
          ADDITIONS: ${{ github.event.pull_request.additions }}
          DELETIONS: ${{ github.event.pull_request.deletions }}
          CHANGED_FILES: ${{ github.event.pull_request.changed_files }}
        run: |
          total=$((ADDITIONS + DELETIONS))
          echo "PR thay đổi $total dòng trong $CHANGED_FILES file"
          if [ "$total" -gt 500 ]; then
            echo "::warning title=PR lớn::$total dòng thay đổi. Cân nhắc tách PR, hoặc ghi rõ phần nào cần review kỹ."
          fi

Con số này tính cả lockfile và file sinh tự động. Đánh dấu chúng trong .gitattributes (ví dụ pnpm-lock.yaml linguist-generated=true) để GitHub thu gọn diff của chúng mặc định, reviewer khỏi phải cuộn qua.

CODEOWNERS tự gán reviewer theo đường dẫn file, và kết hợp với branch protection để bắt buộc người phụ trách phải approve:

# .github/CODEOWNERS
# Quy tắc khớp SAU CÙNG trong file sẽ thắng
*                         @acme/web-team
/prisma/                  @acme/backend-leads
/src/lib/auth/            @acme/security-champions
/src/app/(billing)/       @acme/billing
/.github/workflows/       @acme/platform
/.github/CODEOWNERS       @acme/platform

Bẫy thường gặp: vì quy tắc khớp sau cùng thắng, đặt * ở cuối file sẽ ghi đè mọi quy tắc phía trên. Bảo vệ chính file CODEOWNERS và thư mục workflow, nếu không ai cũng có thể tự gỡ mình ra khỏi danh sách review bắt buộc. Team nhỏ đừng giao một thư mục cho đúng một người: khi người đó nghỉ phép, mọi PR đụng tới thư mục đó bị kẹt.

Branch protection hoặc ruleset cho main (Settings > Rules > Rulesets, hoặc Branch protection rules ở repo cũ):

Bắt buộc PR trước khi merge, tối thiểu 1 approve.
Tự hủy approve khi có commit mới (dismiss stale approvals), hoặc yêu cầu approve cho lần push gần nhất, để không ai đẩy thêm code sau khi được approve.
Bắt buộc review từ Code Owners.
Bắt buộc status check (ví dụ checks hoặc job tổng hợp ci-ok) đạt.
Bắt buộc mọi conversation được resolve.
Chặn force push và chặn xóa nhánh main.

Phần còn lại chạy trước khi code rời máy: Prettier và ESLint trong pre-commit hook (Husky hoặc lefthook với lint-staged). Pre-commit chỉ là tiện lợi vì bỏ qua được bằng --no-verify. CI mới là chốt chặn thật.

Thời gian phản hồi và quy trình cho team nhỏ

PR chờ review lâu gây hại theo kiểu ít ai để ý: tác giả chuyển sang việc khác, mất ngữ cảnh, PR bị conflict, và người ta bắt đầu gom nhiều thay đổi vào một PR cho đỡ phải chờ, làm PR lớn hơn và khó review hơn. Hướng dẫn của Google đặt mức tối đa là một ngày làm việc cho lần phản hồi đầu tiên, và mình thấy mốc đó hợp lý cho hầu hết team.

Quy ước mình đề xuất cho team 3-8 người:

Việc	Quy ước
Phản hồi lần đầu	Trong một ngày làm việc. PR nhỏ (dưới 100 dòng) nên được xem trong buổi
Khi nào review	Đầu buổi sáng, sau bữa trưa, và khi vừa xong một việc, thay vì cắt ngang lúc đang tập trung
Số approve	1 người. Thêm Code Owner cho auth, billing, migration
Approve kèm comment	Được phép khi chỉ còn `nitpick`. Tác giả tự sửa rồi merge, không cần vòng review mới
Ai merge	Tác giả, sau khi có approve và CI xanh. Tác giả hiểu rõ nhất lúc nào an toàn để deploy
PR treo quá 2 ngày	Nêu ở standup; nếu không ai có thời gian, tác giả tách nhỏ PR
Reviewer không chắc phần nào	Ghi rõ "mình chỉ review phần `billing/`, cần người xem UI" thay vì approve cả PR
Bất đồng	Hai vòng comment chưa xong thì gọi nhau; vẫn chưa xong thì người phụ trách module quyết

Với team rất nhỏ (2-3 người), review mọi PR có thể làm chậm đáng kể. Một số team chọn "ship, show, ask": thay đổi rất nhỏ và an toàn thì merge luôn, thay đổi cần chia sẻ thì merge rồi thông báo, thay đổi rủi ro thì mới mở PR chờ review. Cách này chỉ ổn khi CI đủ mạnh và mọi người thống nhất rõ ràng loại thay đổi nào thuộc nhóm nào.

Review code có AI hỗ trợ

Code được viết với trợ giúp của AI không có nghĩa là xấu, nhưng nó có những kiểu lỗi đặc trưng mà reviewer cần biết. Code trông gọn gàng và tự tin, nên rất dễ được cho qua. Mình áp dụng tiêu chuẩn cao hơn, không thấp hơn:

Tác giả phải giải thích được từng dòng. Quy tắc của team: nếu bạn không giải thích được vì sao dòng này cần thiết, đừng đưa nó vào PR. Reviewer có quyền hỏi "dòng này để làm gì?" và câu trả lời "AI viết vậy" không được chấp nhận.
Package có thật và đúng là package bạn nghĩ không. Mô hình đôi khi gợi ý tên package không tồn tại, và những tên "bịa" lặp đi lặp lại đó có thể bị kẻ tấn công đăng ký trước để chờ người cài (kiểu tấn công này được gọi là slopsquatting). Với mọi dependency mới: kiểm tra trên npm ai publish, repo gốc ở đâu, có ai dùng không.
API có đúng version không. Ví dụ hay gặp với Next.js: code viết theo kiểu cũ, params là object đồng bộ, trong khi từ Next.js 15 nó là Promise. Hoặc option cấu hình đã bị bỏ ở version bạn đang dùng.
Test có thật sự kiểm tra hành vi không. Test sinh ra từ chính implementation thường assert đúng những gì code đang làm, kể cả bug. Hãy hỏi: test này fail khi nào? Nếu câu trả lời là "chỉ khi ai đó sửa code", nó không bảo vệ gì cả.
Lỗi bị nuốt. try { ... } catch { return null } ở khắp nơi làm code chạy "không lỗi" nhưng sai âm thầm.
Trùng lặp thay vì tái sử dụng. Một hàm formatCurrency mới trong khi lib/money.ts đã có. AI không biết codebase của bạn có gì nếu không được chỉ.
Mặc định thiếu an toàn. CORS *, tắt kiểm tra TLS, nối chuỗi SQL, log toàn bộ request body có chứa mật khẩu.
Kích thước. Sinh code thì dễ nên PR phình rất nhanh. Giới hạn kích thước PR áp dụng y nguyên.

Bot review tự động (nhiều công cụ hiện có) hữu ích như một lượt đọc đầu tiên để bắt lỗi hiển nhiên. Coi comment của bot như gợi ý cần kiểm chứng, và đừng để approve của bot thay cho approve của người trong branch protection.

Viết lại comment: từ vô ích sang hữu ích

Comment vô ích	Vì sao không giúp được	Viết lại hữu ích
"Sai rồi."	Không nói sai ở đâu, sửa thế nào	"issue (blocking): Khi `items` rỗng, `reduce` không có giá trị khởi tạo sẽ ném TypeError. Thêm `0` làm giá trị khởi tạo và một test cho giỏ hàng trống."
"Sao không dùng X?"	Nghe như chất vấn, không rõ có bắt buộc	"question: `lib/money.ts` đã có `formatVnd`. Có lý do để viết hàm mới không, hay dùng lại được?"
"Code khó đọc quá."	Cảm nhận chung chung	"suggestion: `processOrder` đang làm ba việc: validate, tính tiền, gửi mail. Tách `calculateTotals` ra thì test riêng được phần tính tiền."
"Thêm test."	Không biết test trường hợp nào	"todo: Cần test cho hóa đơn đã `paid` (trả 409) và id không tồn tại (trả 404)."
"Performance?"	Không chỉ ra vấn đề cụ thể	"issue: Vòng lặp gọi `db.customer.findUnique` cho từng order, trang 50 order là 51 query. Dùng `findMany` với `id: { in: customerIds }` được không?"
"Đặt tên tệ."	Chê mà không đề xuất	"nitpick (non-blocking): `data2` → `overdueInvoices`."
"Mình sẽ không viết thế này."	Sở thích cá nhân trình bày như yêu cầu	"thought (non-blocking): Mình hay dùng early return ở đây để bớt lồng `if`, nhưng cách hiện tại vẫn đúng, tùy bạn."
"LGTM" cho PR 1.500 dòng	Không ai thật sự review	"Mình đã review kỹ `billing/` và migration 0042. Phần `components/` chỉ lướt qua, cần một bạn frontend xem thêm."

Những lỗi thường gặp

1. Bình luận về style thay vì cấu hình tool.

Sai:  "nitpick: dùng nháy đơn" (lần thứ 20 trong tháng)
Đúng: thêm rule vào Prettier/ESLint trong một PR riêng, chạy trong CI, không bao giờ comment về nó nữa

2. PR gộp refactor với thay đổi hành vi. Đổi tên 30 file cùng lúc với sửa logic tính VAT, và thay đổi quan trọng chìm giữa 600 dòng diff vô hại. Tách refactor thành PR riêng, merge trước.

3. Approve khi chưa đọc hết mà không nói. Nếu bạn chỉ review một phần, ghi rõ phần nào. Approve nghĩa là bạn chịu trách nhiệm cùng tác giả.

4. Comment không có nhãn.

Sai:  "Có thể đổi tên biến này." (bắt buộc hay không?)
Đúng: "nitpick (non-blocking): `res` → `invoiceResponse` cho rõ, tùy bạn."

5. Đẩy thêm commit sau khi được approve mà không ai xem lại. Bật "dismiss stale approvals" hoặc yêu cầu approve cho lần push gần nhất.

6. CODEOWNERS có * ở cuối file. Quy tắc cuối cùng thắng, nên mọi quy tắc cụ thể phía trên bị vô hiệu.

7. Review khi chưa đọc mô tả PR. Bạn sẽ comment về những quyết định đã được giải thích sẵn, hoặc bỏ qua đúng phần tác giả nhờ xem kỹ.

8. Để PR treo vì "chưa có thời gian review kỹ". Một phản hồi sớm ("mình đã xem phần API, chiều nay xem tiếp phần UI") tốt hơn im lặng hai ngày.

Checklist

Thêm .github/pull_request_template.md với các mục: bối cảnh, thay đổi, cách test, ảnh chụp, rủi ro, rollback, gợi ý cho reviewer.
Đưa Prettier, ESLint, typecheck và test vào CI, đặt làm required status check cho main.
Tạo .github/CODEOWNERS cho các thư mục nhạy cảm (auth, billing, migration, workflows), mỗi thư mục ít nhất hai người.
Bật ruleset cho main: bắt buộc PR, approve, review của Code Owner, dismiss stale approvals, resolve conversation, chặn force push.
Thống nhất trong team dùng nhãn issue (blocking), nitpick, question, suggestion, praise cho comment.
Self-review mọi PR ở chế độ draft trên GitHub trước khi gán reviewer.
Đặt mục tiêu phản hồi lần đầu trong một ngày làm việc và theo dõi các PR treo quá hai ngày ở standup.
Thêm workflow cảnh báo PR lớn hơn 500 dòng, đánh dấu file sinh tự động bằng linguist-generated.
Viết quy tắc cho code có AI hỗ trợ: tác giả giải thích được từng dòng, kiểm tra mọi dependency mới.