Review để làm gì, nhìn gì trước và vòng đời một PR Trung cấp

Trang này trả lời: review thật sự mang lại gì, nên nhìn chỗ nào trước, và một PR đi qua những bước nào trước khi merge.

Review để làm gì

Nghiên cứu tại Microsoft (Bacchelli & Bird, ICSE 2013): tìm bug là động lực chính khi review, nhưng kết quả thực tế “less about defects than expected”. Review mang lại nhiều hơn ở chia sẻ kiến thức, nhận biết thay đổi và phương án thay thế.

Mục đích	Ví dụ cụ thể
Chặn lỗi đắt	Race condition khi trừ tồn kho, IDOR, migration khoá bảng 20 phút
Giữ thiết kế lành mạnh	Logic nghiệp vụ lọt vào controller, abstraction thêm flag thứ năm
Chia sẻ kiến thức	Ít nhất hai người hiểu module thanh toán; junior học cách senior nghĩ
Nhất quán	Code mới theo cùng quy ước lỗi, log, đặt tên với phần còn lại

Review không để làm gì

• Tranh luận style. Dấu nháy, thứ tự import, độ dài dòng: đưa vào Prettier/ESLint và cho CI fail. Google: “On matters of style, the style guide is the absolute authority.”
• Thay cho test. Reviewer không chạy được mọi nhánh trong đầu. Logic mới không có test thì comment đầu tiên là xin test.
• Bắt người viết làm theo cách của bạn. Cách của họ đúng, đọc được, hợp quy ước thì việc bạn thích cách khác không phải lý do để chặn.

Thứ tự ưu tiên: nhìn chỗ đắt trước

Google gợi ý đọc PR theo ba bước: nhìn tổng thể (PR này có nên tồn tại không), đọc phần chính, rồi phần còn lại. Thấy vấn đề thiết kế lớn thì gửi comment ngay, đừng đợi đọc hết — tác giả đang tiếp tục xây trên thiết kế đó.

Ưu tiên	Câu hỏi	Mức độ thường gặp
1. Tính đúng và thiết kế	Giải đúng bài toán chưa? Có cách đơn giản hơn? Đặt đúng tầng chưa? Over-engineering?	Blocking
2. Bảo mật và toàn vẹn dữ liệu	Validate ở biên? Query scope theo user? Transaction đúng ranh giới? Migration backward-compatible?	Blocking
3. Test	Logic mới có test? Test có fail khi code sai không?	Thường blocking
4. Dễ đọc	Tên nói đúng ý? Hàm làm một việc? Comment giải thích vì sao?	Thường non-blocking
5. Style	Máy đã kiểm chưa? Chưa thì thêm rule vào linter	nit: hoặc không comment

Bảng “nhìn ở đây trước”

Đừng giữ checklist trong đầu. Mỗi loại thay đổi đã có checklist riêng:

PR chạm vào	Nhìn ở đây trước	Checklist
BEGIN/COMMIT, trừ số dư, tồn kho, FOR UPDATE	Ranh giới transaction, network call trong transaction, rowCount	Transaction trong code
Controller, DTO, query SQL, upload, log	Validate ở biên, parameterized query, secret và log	Secure coding
Endpoint theo id (/orders/:id)	Có lọc theo người dùng hiện tại không	BOLA/IDOR
Migration đổi/xoá cột	Code cũ và mới chạy song song được không	Expand-contract
Tên bảng, cột, endpoint, event mới	Casing, đơn vị, hợp đồng đã public	Đặt tên
Refactor, service logic	Refactor tách khỏi đổi hành vi, lỗi có bị nuốt	Clean code
Lịch sử commit, force push	Commit atomic, message, secret trong diff	Git
Test mới bị retry, sleep	Test có ổn định không	Flaky tests
Vòng lặp gọi DB, findMany()/SELECT không LIMIT, hàm async	N+1, query không giới hạn, thiếu await	Lỗi hay gặp

Vòng đời một PR

Đang tải diagram…

Ba điểm đáng chú ý:

• Draft PR trên GitHub không merge được và không tự request code owner. Dùng nó để chạy CI và tự review trước khi làm phiền ai.
• CI chạy trước người. Reviewer không nên mất thời gian vào PR đang đỏ.
• Mỗi vòng sửa quay lại CI. Approve rồi push thêm commit logic mà không ai xem lại là lỗ hổng quy trình; cấu hình branch protection bỏ approve cũ khi có commit mới nếu team cần.

Tiếp theo: Người viết PR.