Người viết PR: PR nhỏ, mô tả rõ, tự review trước Trung cấp

Trang này trả lời: tác giả làm gì để PR được review nhanh và kỹ. Phần lớn chất lượng review được quyết định trước khi reviewer mở PR.

Đang tải diagram…

1. PR nhỏ, một mục đích

Google Small CLs: PR nhỏ được review nhanh hơn, kỹ hơn, ít bug hơn, dễ rollback hơn.

• Mốc tham khảo: “100 lines is usually a reasonable size for a CL, and 1000 lines is usually too large”.
• Số file cũng tính: 200 dòng trong một file có thể ổn, rải trên 50 file thường là quá lớn.

Cách chia một tính năng lớn, ví dụ “hoàn tiền một phần”:

Cách chia	Ví dụ
Stacking	PR 1: bảng refunds + repository. PR 2 (dựa trên PR 1): RefundService. PR 3: endpoint
Theo tầng (horizontal)	Hợp đồng API + stub trước, để frontend làm song song
Theo lát dọc (vertical)	Hoàn tiền thẻ trước, ví sau; mỗi cái đi hết từ API tới DB
Sau feature flag	Merge dần vào main, bật flag khi đủ

Nghiên cứu của SmartBear tại Cisco (nhà cung cấp công cụ review, số liệu từ một team) khuyên review không quá 200–400 dòng mỗi lần. Mật độ lỗi tìm được giảm rõ khi đọc nhanh hơn 500 dòng/giờ.

Coi đó là tín hiệu định hướng, không phải chuẩn. Nhưng nó khớp trải nghiệm: PR 2.000 dòng không được review, nó được lướt.

2. Tách refactor khỏi thay đổi hành vi

Google: “do refactorings in a separate CL from feature changes or bug fixes”; đổi tên biến cục bộ nhỏ thì để chung được.

# Trước — một PR, reviewer phải tự phân biệt 600 dòng di chuyển với 40 dòng logic mới
feat(refund): hỗ trợ hoàn tiền một phần     +640 −410

# Sau — hai PR, mỗi PR một câu hỏi
refactor(payment): tách PaymentGateway khỏi OrderService   +380 −370   # test cũ pass, không đổi test
feat(refund): hỗ trợ hoàn tiền một phần                     +260 −40    # chỉ còn logic mới + test

Với PR refactor, reviewer chỉ hỏi “có đổi hành vi không?” — test cũ pass mà không sửa là bằng chứng. Xem thêm Clean code và Git.

3. Mô tả PR: vì sao, test thế nào, rủi ro, rollback

Google: “Fix bug” là mô tả không đủ — bug gì, sửa bằng cách nào?

• Dòng đầu: câu mệnh lệnh nói PR làm gì.
• Phần thân: vì sao và bối cảnh mà diff không thể hiện.

Mô tả PR chỉ nói 'đã sửa'

✕ Mô tả tồi

Fix bug refund

✓ Mô tả tốt

Chặn hoàn tiền vượt số đã thu khi hai request chạy song song

Hai request hoàn tiền cùng đọc refunded = 0 rồi cùng ghi (INC-231, mất 2,1 triệu).
Đổi sang UPDATE có điều kiện + kiểm tra rowCount, thêm CHECK constraint.
Test: integration test 2 request song song trên PostgreSQL thật.
Rollback: revert PR; constraint chỉ thêm, không cần migration down.

Vì sao

Reviewer đọc bản tồi phải tự đoán bug là gì và đi đọc cả diff mới biết rủi ro. Sáu tháng sau, git log cũng chỉ còn “Fix bug refund” — không ai biết vì sao có CHECK constraint đó.

Đặt template ở .github/pull_request_template.md (GitHub chỉ áp dụng sau khi file có trên default branch):

## Vì sao
<!-- Vấn đề nghiệp vụ hoặc kỹ thuật. Link ticket/incident. -->
Khách hàng hoàn tiền một phần phải chờ CS làm tay (~120 ticket/tuần, PAY-512).

## Thay đổi gì
<!-- Hướng tiếp cận, quyết định thiết kế, phương án đã loại và vì sao. -->
- Thêm `RefundService.refundPartial()`; tổng tiền hoàn bị giới hạn bằng CHECK constraint.
- Chọn không gọi cổng thanh toán trong transaction; dùng outbox.

## Đọc từ đâu
<!-- Gợi ý thứ tự cho reviewer. -->
`refund.service.ts` → `refund.service.spec.ts` → migration.

## Đã test thế nào
- Unit test cho 6 case làm tròn; integration test với PostgreSQL thật.
- Chạy tay trên staging: hoàn 30% đơn 199.000đ, đối soát khớp.

## Rủi ro và rollback
- Rủi ro: hoàn tiền vượt số đã thu khi 2 request song song → chặn bằng `FOR UPDATE` + CHECK.
- Rollback: tắt flag `refund.partial`. Migration chỉ thêm bảng (expand), không cần down.

## Ngoài phạm vi
- Hoàn tiền đa tiền tệ: PR sau.

Mục “Ngoài phạm vi” tiết kiệm nhiều comment nhất: reviewer không phải hỏi “còn trường hợp X thì sao?”.

4. Tự review trước khi request

Mở tab “Files changed” và đọc diff như một reviewer. Lần tự review này thường bắt được:

• console.log sót, file .env.local, test bị .only.
• Đoạn code bị comment, TODO không có ticket.

Đánh dấu sẵn comment vào chỗ khó: “Chỗ này cố ý không retry vì…” — rẻ hơn để reviewer hỏi. Trước khi merge, đọc lại mô tả PR: PR thường đổi trong lúc review, mô tả cũ dễ sai.

Checklist cho tác giả

1. PR một mục đích; refactor và thay đổi hành vi ở PR (hoặc ít nhất commit) riêng.
2. Mô tả có: vì sao, thay đổi gì, đọc từ đâu, test thế nào, rủi ro, rollback, ngoài phạm vi.
3. Đã tự đọc diff: không debug log, .only, file .env, code bị comment, TODO không ticket.
4. CI xanh trước khi request review; PR mở dạng draft khi chưa sẵn sàng.
5. Logic mới có test; test fail khi đảo logic.
6. Migration và thay đổi API theo expand-contract; có cách rollback không cần migration down.
7. Trả lời mọi comment: “Done”, giải thích kèm sửa code cho rõ, hoặc trade-off khi không đồng ý.
8. Đọc lại mô tả trước khi merge để khớp với code cuối cùng.

Tiếp theo: Người review.