Review kém hiếm khi vì “thiếu IQ”, mà thường vì thiếu tiêu chí và PR quá lớn — reviewer không còn “working memory” để giữ mô hình mental của thay đổi. Middle+ reviewer cần rubric và ngân sách: biết cái gì bắt buộc kiểm, cái gì nhường cho test/monitor.

Phạm vi: async review trên GitHub/GitLab tương tự. Tham chiếu tác giả PR: Viết PR để reviewer muốn merge.

1. Vì sao cần rubric (dù chỉ trong đầu)

Rubric giúp tránh hai thái cực:

  • LGTM vô điều kiện vì deadline.
  • Nitpick vô tận vì style cá nhân.

2. Bốn tầng kiểm (thứ tự ưu tiên mặc định)

  1. Correctness & safety — logic, null, race, migration an toàn.
  2. Security — authz, input validation, secret, SSRF surface nhỏ.
  3. Performance / reliability — N+1, unbounded loop, retry storm.
  4. Maintainability — naming, boundary module, test đọc được.

Nếu (1) hoặc (2) chưa rõ, đừng dành nhiều pixel cho (4).

3. PR sizing và batching

Heuristic thực dụng:

  • > ~400 dòng diff (tuỳ ngôn ngữ): xác suất lỗ sót tăng nhanh.
  • Tách: refactor thuần túy vs behavior change; hoặc feature flag.

“Một PR một ý” không phải dogma — nhưng một PR một mục tiêu review là bắt buộc.

4. Template comment (ví dụ ngắn)

[blocking] ... — lý do + đề xuất fix
[question] ... — cần tác giả làm rõ
[nit] ... — không chặn merge nếu team chấp nhận

Design note bắt buộc khi: thay đổi contract API, schema, authz model, hoặc chi phí infra lớn.

5. Async vs pair review

AsyncPair (live)
Ưu điểmGhi lại được, scale timezoneGiải quyết hiểu nhầm nhanh
Nhược điểmDễ kéo dài vòng lặpKhó scheduling

Gợi ý: PR rủi ro cao → 15 phút pair trước khi merge thay vì 30 comment ping-pong.

6. Checklist theo mức rủi ro PR

Low (copy, typo, docs nhỏ): CI xanh + một reviewer.

Medium (feature nội bộ): rubric đủ 4 tầng; test tự động liên quan.

High (auth, billing, migration dữ liệu):

  • Hai reviewer hoặc 1 senior theo policy team
  • Rollback / feature flag / backup (tuỳ loại) được nhắc trong mô tả PR
  • Kiểm tra authz matrix (ít nhất mentally) cho path mới

Khi không nên yêu cầu “perfect abstraction”

  • Hotfix incident: minimize diff, document debt ticket.
  • Spike thử nghiệm: label rõ experimental và giới hạn merge window.

7. Vai trò reviewer vs “gatekeeper”

Reviewer tốt tăng throughput của team: phản hồi nhanh, phân loại rõ, đề xuất diff nhỏ. Reviewer xấu trở thành gatekeeper: mọi PR phải theo gu cá nhân không viết thành văn.

Giải thích thêm: rubric nên là tài sản team (ADR ngắn, checklist), không phải truyền miệng.

8. Security review nhẹ trong PR (không chờ audit riêng)

Với PR đụng auth/payment:

  • Có thay đổi trust boundary không? (xem OWASP theo luồng)
  • Có log secret/PII không?
  • Có test deny cho authz không?

Chỉ cần 3 câu hỏi này đã bắt được nhiều lỗ hổng “ngớ ngẩn nhưng đắt”.

9. “Comment culture” và tâm lý an toàn

  • Viết comment về code, không về con người.
  • Dùng “chúng ta”: “mình lo case null ở đây” thay vì “sao anh không nghĩ”.
  • Khi author push fix, reviewer resolve thread hoặc giải thích vì sao chưa đủ — tránh thread treo vô hạn.

10. Thời gian phản hồi SLA nội bộ (gợi ý)

Loại PRSLA phản hồi đầu tiên (gợi ý)
Hotfix< 30 phút (the-ca)
Nhỏ< 1 ngày làm việc
Lớnack trong 4h + lịch pair nếu cần

SLA không cần công cụ — cần thỏa thuận và người backup khi reviewer bận.

11. AI-assisted review: giới hạn

LLM có thể tóm tắt diff, nhưng không thay thế trách nhiệm authz/security. Policy team: AI là bật thêm một reviewer giả — output vẫn cần người chịu trách nhiệm merge.

12. Rubric mở rộng (checklist nhanh)

Correctness: edge case, null, concurrency, migration backward compatible?

Security: input validation, authz, secret, SSRF/injection surface?

Perf: N+1, unbounded query, retry storm?

Maintainability: naming, module boundary, test name mô tả hành vi?

Bài tập ngắn

Lấy một PR gần đây của team: phân loại comment theo [blocking]/[question]/[nit] — tỉ lệ nit có quá cao không?

Mở rộng: Viết một ADR một trang “Review policy” từ kết quả bài tập.

13. “Approve with comment” — khi nào chấp nhận được?

Chấp nhận được khi comment còn lại là nit hoặc follow-up đã ticket hoá (link Jira/GitHub issue). Không chấp nhận khi còn [blocking] chưa có cam kết thời điểm fix — trừ khi policy team cho phép merge + hotfix branch (ghi rõ trong PR).

14. Review PR của người senior: cùng rubric

Đôi khi team ngại challenge senior. Rubric chữ thành văn giúp junior hỏi đúng chỗ (“em không thấy test deny cho role X theo matrix”). Đó là văn hoá kỹ thuật, không phải hierarchy.

15. Đo chất lượng review (định tính)

Mỗi quý, hội ý nhanh:

  • PR lớn nhất merge được là bao nhiêu dòng?
  • median thời gian first response?
  • Có PR nào revert trong 48h không — vì sao?

Không cần dashboard phức tạp; spreadsheet 10 dòng vẫn đủ học.

16. Tài liệu kèm PR: khi nào cần diagram

Khi thay đổi luồng cross-service hoặc state machine — một sequence ASCII (như các bài khác trong blog) đáng giá hơn mô tả dài bằng lời.

Tóm tắt cho người vội

  • Rubric + sizing PR là lever lớn nhất.
  • Security/perf là tầng ưu tiên trước style.
  • SLA phản hồi và văn hoá comment quyết định throughput.

Đọc thêm