Chuẩn pull request · Project Handbook

PR tồn tại để làm gì

Ba việc, theo thứ tự quan trọng:

Bắt lỗi mà máy không bắt được: hiểu sai yêu cầu, thiết kế sai, thiếu trường hợp biên.
Lan truyền kiến thức: để ít nhất hai người hiểu mỗi vùng code.
Ghi lại lý do: mô tả PR và thảo luận trong PR là tài liệu sống của thay đổi.

PR không tồn tại để bắt lỗi format, lỗi kiểu, hay lỗi test — những thứ đó phải do quality gate tự động chặn trước, ở chặng sớm hơn và rẻ hơn.

Luật về kích thước

Số dòng thay đổi	Đánh giá
Dưới 200	Tốt, review kỹ được
200 – 400	Chấp nhận được
400 – 800	Phải giải thích vì sao không tách
Trên 800	Bị trả lại, trừ khi là thay đổi máy sinh ra

Lý do không phải thẩm mỹ. Khả năng phát hiện lỗi của người review giảm rất nhanh theo kích thước PR: một PR 1000 dòng thường nhận được approve nhanh hơn một PR 100 dòng, vì không ai đọc nổi — và đó chính là điều nguy hiểm.

Cách tách PR lớn:

Tách theo tầng: PR 1 thêm tầng dữ liệu, PR 2 thêm logic, PR 3 thêm giao diện.
Tách refactor ra khỏi thay đổi hành vi. Không bao giờ trộn hai thứ này — người review không thể phân biệt đâu là đổi cấu trúc, đâu là đổi hành vi.
Merge sớm phần chưa hoàn chỉnh sau một feature flag đang tắt.

Mở PR nháp sớm

Không chờ code xong mới mở PR. Mở draft PR ngay khi có khung của giải pháp, kèm một câu hỏi cụ thể:

Draft. Mình đang định lưu hàng đợi tin nhắn offline vào storage cục bộ theo cách này. Trước khi viết tiếp phần đồng bộ, mọi người thấy cách đặt khoá có vấn đề gì không?

Một góp ý về hướng đi nhận được lúc này rẻ hơn nhiều so với cùng góp ý đó nhận được sau khi đã viết xong 600 dòng. Đây là cách dùng PR đúng tinh thần shift left.

Mẫu mô tả PR

## Làm gì
Một đoạn ngắn: thay đổi này làm gì, cho ai.

## Vì sao
Vấn đề đang gặp, hoặc hạng mục / issue liên quan.

## Cách làm
Hướng tiếp cận, và phương án đã cân nhắc rồi loại (nếu có).

## Acceptance criteria
- [ ] Tiêu chí 1 — đã kiểm chứng bằng cách nào
- [ ] Tiêu chí 2 — đã kiểm chứng bằng cách nào

## Đã test thế nào
- Unit test: ...
- Thủ công: thiết bị nào, hệ điều hành nào, luồng nào
- Trường hợp biên đã thử: rỗng, lỗi mạng, quyền bị từ chối, dữ liệu cũ

## Ảnh hưởng
- Module bị chạm: chat / task / sales / dùng chung
- Có đổi schema, cấu hình, biến môi trường không
- Có cần thao tác gì khi release không

## Rủi ro và cách rút lui
Nếu hỏng thì biểu hiện thế nào, rollback bằng cách nào.

## Ảnh / video
Bắt buộc nếu có thay đổi UI. Trước và sau, ở ba bản: cỡ chữ mặc định, cỡ chữ lớn nhất, dark mode.

Ba bản ảnh ở mục cuối là chốt UI rẻ nhất trong quy trình — xem Chuẩn UI trên mobile. Thiếu một bản thì lỗi vỡ layout chỉ lộ ra lúc thử tay ở bước DoD, đắt hơn nhiều.

PR không có mô tả, hoặc mô tả chỉ ghi "fix bug", bị đóng lại mà không cần đọc code.

Trách nhiệm của người mở PR

Tự review thay đổi của mình trước, và để lại comment ở chỗ bạn biết người khác sẽ thắc mắc. Việc này tiết kiệm một vòng qua lại.
Chỉ định người review cụ thể. "Ai rảnh thì xem" nghĩa là không ai xem.
Trả lời mọi comment, kể cả khi không đồng ý — và khi không đồng ý thì nói lý do, đừng im lặng rồi bỏ qua.
Không tự merge khi còn comment chưa xử lý.
Không sửa thêm thứ ngoài phạm vi giữa chừng — nó làm review đã duyệt thành vô hiệu.

Trách nhiệm của người review

Thời hạn

Việc	Thời hạn
Phản hồi đầu tiên	Trong 4 giờ làm việc
Review lại sau khi tác giả sửa	Trong 2 giờ làm việc
PR chặn release hoặc hotfix	Ngay lập tức

PR nằm chờ là tồn kho: code đã viết xong nhưng chưa tạo ra giá trị, lại đang lạc hậu dần so với main. Review là việc ưu tiên cao hơn viết code mới của chính mình.

Nhìn cái gì

Theo thứ tự:

Đúng yêu cầu chưa — đối chiếu acceptance criteria, không chỉ đọc code.
Trường hợp biên — rỗng, null, lỗi mạng, trùng lặp, người dùng thao tác hai lần, dữ liệu cũ từ phiên bản trước.
Bảo mật và dữ liệu — phân quyền kiểm ở đâu, có log dữ liệu cá nhân không, đầu vào có được kiểm tra không.
Thiết kế — đặt đúng tầng chưa, có tạo phụ thuộc vòng không, có nhân bản logic không.
Khả năng đọc — tên, cấu trúc, chỗ sáu tháng sau sẽ khó hiểu.
Test — test có thật sự khẳng định điều gì không, hay chỉ chạy cho có.

Cách viết comment

Phân loại rõ mức độ, để tác giả biết cái gì chặn merge:

Nhãn	Nghĩa	Chặn merge
`[chặn]`	Phải sửa mới merge được	Có
`[nên]`	Nên sửa, tác giả quyết	Không
`[hỏi]`	Tôi chưa hiểu, giải thích giúp	Không
`[vặt]`	Ý kiến nhỏ, bỏ qua cũng được	Không

Nguyên tắc viết: nhắm vào code, không nhắm vào người; nói rõ vấn đề và đề xuất hướng sửa; khen khi thấy chỗ làm tốt — review chỉ toàn chê sẽ làm người ta sợ mở PR nhỏ.

Xấu:  "Chỗ này viết ẩu quá."
Tốt:  "[chặn] Nếu danh sách rỗng thì dòng 42 sẽ ném lỗi. Thêm early return
       hoặc dùng optional chaining giúp mình nhé."

Comment dạng "tôi không thích cách này" mà không nêu được vấn đề cụ thể thì không phải góp ý, và không được dùng để chặn PR.

Điều kiện merge

Mọi quality gate tự động xanh.
Ít nhất 1 approve; 2 approve nếu PR chạm tới thanh toán, phân quyền, dữ liệu cá nhân, hoặc code dùng chung giữa các module.
Chủ module bị ảnh hưởng đã approve nếu PR sửa vào phần của module đó.
Mọi comment [chặn] đã được xử lý.
Đã rebase lên main mới nhất.

Người merge là tác giả, không phải người review — vì tác giả là người theo dõi pipeline sau merge và chịu trách nhiệm nếu main đỏ.

Những cách phá chuẩn thường gặp

Hành vi	Vì sao bị cấm
Approve mà không đọc, để "cho nhanh"	Biến review thành thủ tục hình thức, mất luôn tác dụng
Mở PR khổng lồ sát ngày release	Ép người review approve mù
Trộn refactor với sửa lỗi	Không ai tách nổi cái gì làm đổi hành vi
Sửa thêm sau khi đã được approve	Vô hiệu hoá kết quả review
Tự approve PR của mình	Không có chặng người-nhìn-người nào cả
Merge khi gate đang đỏ	Phá luật cứng, xử lý như sự cố