Pull request, issue và code review theo thói quen team

Mục tiêu bài học

• Viết issue đủ rõ để người khác làm được mà không phải hỏi lại ba lần.
• Viết PR description theo cấu trúc What / Why / How và gắn issue đúng cách.
• Phân loại comment review (blocking, nit, question) và phản hồi đúng trọng tâm.
• Chạy self-review trước khi bấm "Ready for review".
• Áp dụng nhịp review của team: thời hạn phản hồi, số approve tối thiểu, xử lý conversation.

Ôn nhanh

Bài trước chốt rằng main được bảo vệ và mọi thay đổi đi qua nhánh feature ngắn ngày, ví dụ feat/post-pagination. Vậy câu hỏi còn lại là: làm sao đưa nhánh đó vào main mà không làm phiền người khác? Câu trả lời nằm ở issue, PR và review.

Lý thuyết

Issue: bản giao kèo nhỏ trước khi viết code

Issue không phải chỗ ghi cảm xúc, nó là bản mô tả công việc. Một issue tốt có bốn phần:

1. Bối cảnh: vì sao cần việc này.
2. Việc cần làm: mô tả ngắn, không lan sang việc khác.
3. Acceptance criteria: danh sách checkbox kiểm chứng được.
4. Ghi chú kỹ thuật: file liên quan, ràng buộc, ảnh thiết kế.

Với project blog, issue #21 "Thêm phân trang cho trang danh sách bài viết" có acceptance criteria:

- [ ] Trang 1 hiển thị đúng 5 bài mới nhất
- [ ] Nút Trước bị disable khi ở trang đầu
- [ ] URL đổi thành /posts?page=2 khi chuyển trang

Ba dòng này quan trọng hơn bạn tưởng: chúng là thứ người review dùng để nói "đủ rồi" thay vì tranh luận vô tận.

Bên cạnh đó là label (type: feature, priority: high), assignee (người làm) và milestone (v1.0).

Pull request: một thay đổi, một câu chuyện

Nguyên tắc số một: PR nhỏ, khoảng dưới 400 dòng thay đổi, một mục đích duy nhất. PR 1.200 dòng thường bị review kiểu "LGTM" cho xong, tức là review vô nghĩa.

Tiêu đề PR viết theo Conventional Commits để lịch sử main đọc được:

feat(posts): thêm phân trang cho danh sách bài viết

Body PR dùng template cố định của team (tạo một lần trong .github/pull_request_template.md):

## What
Thêm phân trang 5 bài mỗi trang cho danh sách bài viết.

## Why
Danh sách hiện tại dài vô tận, tải chậm và khó tìm bài cũ.

## How
Đọc `?page` từ query string, cắt mảng bài viết theo offset.

## Test
- `npm test` xanh
- Kiểm tra tay trang 1, 2, 3 và trường hợp page không hợp lệ

Closes #21

Từ khóa liên kết issue là Closes #21, Fixes #21 hoặc Resolves #21. Khi PR merge vào nhánh mặc định, GitHub tự đóng issue. Đừng đóng tay rồi bỏ quên bước này, lịch sử sẽ mất dấu vết.

Hai thói quen nữa: mở draft PR khi còn làm dở (vừa chạy được CI, vừa cho người khác thấy hướng đi), và self-review bằng cách đọc lại tab "Files changed" trước khi request review.

Code review: chia sẻ hiểu biết, không phải chấm điểm

Người review chia comment thành ba loại rõ ràng:

Người review nên: phản hồi trong vòng 24 giờ, tập trung vào logic và test, không bắt bẻ style nếu đã có formatter, và luôn kèm gợi ý cụ thể thay vì "chỗ này sai".

Người viết PR nên: trả lời từng comment (kể cả "đã sửa ở commit abc123"), không im lặng rồi push tiếp, resolve conversation sau khi xử lý xong, và dùng nút "Add suggestion to batch" khi gợi ý của reviewer đúng.

Thấy vấn đề nằm ngoài phạm vi PR? Đó là lúc tạo issue mới. Mở rộng PR để "tiện tay sửa luôn" là cách nhanh nhất biến PR 300 dòng thành 900 dòng.

flowchart LR
    I["📝<br/>Issue #21"] --> B["🌿<br/>feat/post-pagination"]
    B --> P["🔀<br/>Pull request + Closes #21"]
    P --> R["👀<br/>Code review"]
    R --> M["✅<br/>Squash merge"]
    M --> C["🔒<br/>Issue tự đóng"]

Điều kiện merge điển hình trên main bảo vệ: tối thiểu 1 approve, CI xanh, không còn conversation chưa resolve. Merge bằng squash để main giữ một commit gọn cho mỗi feature, sau đó xóa branch.

Ví dụ minh họa

Ví dụ 1: từ issue xấu sang issue tốt

Issue xấu: "Sửa trang bài viết cho đẹp hơn." Không ai biết "đẹp hơn" là gì, và review sẽ thành cuộc tranh cãi gu thẩm mỹ.

Issue tốt:

## Bối cảnh
Trang chi tiết bài viết đang thiếu ngày đăng và tên tác giả.

## Việc cần làm
Hiển thị metadata dưới tiêu đề bài viết.

## Acceptance criteria
- [ ] Ngày đăng định dạng DD/MM/YYYY
- [ ] Tên tác giả lấy từ front matter
- [ ] Ẩn dòng metadata nếu bài không có front matter

## Ghi chú
File: `src/pages/posts/[slug].tsx`

Ví dụ 2: một vòng review thật

PR #22 có ba comment. Tác giả phản hồi như sau:

• Reviewer: "blocking: hàm getPage() không xử lý page=0, sẽ trả về toàn bộ bài viết." Tác giả: "Đã sửa, clamp về 1 trong commit a1b2c3d, thêm test getPage(0)."
• Reviewer: "nit: đổi postsArr thành posts." Tác giả: "Đã đổi."
• Reviewer: "question: sao không dùng thư viện pagination có sẵn?" Tác giả: "Vì chỉ cần 20 dòng, thêm dependency không đáng. Mình ghi chú vào issue #23 để xem lại nếu số bài tăng."

Cả ba phản hồi đều ngắn, đúng trọng tâm, có dẫn chứng. Đó chính là "phản hồi review đúng trọng tâm".

Luyện tập có hướng dẫn

Bài tập gắn với capstone blog của bạn.

Bài 1. Viết issue cho tính năng "trang chi tiết bài viết" với đủ bốn phần và ít nhất ba acceptance criteria kiểm chứng được.

Bài 2. Viết PR description cho nhánh feat/post-detail, nhớ gắn Closes #<số issue> ở cuối.

Bài 3. Phân loại bốn comment sau và viết phản hồi cho mỗi cái: (a) "Thiếu test cho trường hợp bài viết không tồn tại." (b) "Đổi Title thành PostTitle." (c) "Sao lại dùng any ở đây?" (d) "PR này có 12 file, tách được không?"

Góc chuyên gia

Lỗi mình thấy nhiều nhất ở intern:

• PR description rỗng hoặc chỉ ghi "fix bug". Người review phải đọc diff để đoán ý, mất gấp ba thời gian.
• Trả lời review bằng "ok" rồi push, không nói sửa ở đâu. Reviewer phải lục lại diff.
• Comment kiểu "sai rồi" mà không nói sai ở đâu. Viết "chỗ này trả về null khi mảng rỗng" tốt hơn nhiều.
• Rebase rồi force-push giữa lúc đang review mà không báo trước, làm comment cũ trỏ sai dòng.

Quy tắc nhanh:

1. PR quá 500 dòng thì tách, gần như luôn tách được.
2. Review xong trong 24 giờ, kể cả chỉ để nói "mai mình xem".
3. Một PR một mục đích. Việc phát sinh thì mở issue mới.
4. Approve phải kèm câu cụ thể, "LGTM" một mình là vô nghĩa.
5. Trước khi request review, tự đọc lại diff một lượt.

[!TIP] Nếu bạn phải giải thích một đoạn code cho reviewer hiểu, đó là dấu hiệu đoạn code nên được viết lại, không phải dấu hiệu cần viết mô tả dài hơn.

Tóm tắt và bước tiếp theo

Issue rõ ràng là hợp đồng, PR nhỏ là cách tôn trọng thời gian người review, và comment review có nhãn (blocking, nit, question) là cách nói chuyện chuyên nghiệp. Gắn Closes #<issue>, self-review, phản hồi từng comment, và không mở rộng phạm vi PR.

Bài học tiếp theo là lúc áp dụng toàn bộ vào capstone: hoàn tất blog, mở PR cuối, merge vào main và cắt release v1.0 với tag.

Từ khóa {{c1::Closes}} trong PR description giúp tự động đóng issue khi PR được merge.
Một PR tốt chỉ giải quyết {{c2::một}} mục đích và nên giữ dưới khoảng {{c3::400}} dòng thay đổi.
Comment {{c4::nit}} là góp ý không bắt buộc, không chặn merge.
Điều kiện merge điển hình trên main bảo vệ là tối thiểu {{c5::1}} approve và CI xanh.

[
  { "front": "Từ khóa nào trong PR description tự đóng issue khi merge?", "back": "Closes (hoặc Fixes, Resolves) kèm số issue, ví dụ Closes #21." },
  { "front": "Một issue tốt gồm bốn phần nào?", "back": "Bối cảnh, việc cần làm, acceptance criteria, ghi chú kỹ thuật." },
  { "front": "Acceptance criteria nên viết dạng gì?", "back": "Checkbox kiểm chứng được, ví dụ '- [ ] Trang 1 hiển thị đúng 5 bài'." },
  { "front": "Kích thước PR được khuyến nghị?", "back": "Khoảng dưới 400 dòng thay đổi, chỉ một mục đích." },
  { "front": "Ba phần chính trong PR description?", "back": "What (làm gì), Why (vì sao), How/Test (cách làm và cách kiểm)." },
  { "front": "Draft PR dùng khi nào?", "back": "Khi muốn chia sẻ sớm hoặc chạy CI nhưng chưa sẵn sàng review." },
  { "front": "Comment 'nit' nghĩa là gì?", "back": "Góp ý nhỏ, không bắt buộc, không chặn merge." },
  { "front": "Comment 'blocking' nghĩa là gì?", "back": "Phải sửa trước khi merge, thường là lỗi logic, thiếu test hoặc lỗi bảo mật." },
  { "front": "Thấy vấn đề ngoài phạm vi PR thì làm gì?", "back": "Tạo issue mới, không mở rộng PR, tránh scope creep." },
  { "front": "Ai nên self-review và khi nào?", "back": "Tác giả PR, đọc lại tab Files changed trước khi request review." },
  { "front": "Điều kiện merge điển hình trên main bảo vệ?", "back": "Tối thiểu 1 approve, CI xanh, không còn conversation chưa resolve." },
  { "front": "Cách merge nào giữ lịch sử main gọn với PR nhỏ?", "back": "Squash merge, gộp cả PR thành một commit." },
  { "front": "Nhịp phản hồi review phổ biến của team?", "back": "Trong vòng 24 giờ, kể cả khi chỉ để hẹn thời điểm xem." },
  { "front": "Sau khi merge thì làm gì với branch feature?", "back": "Xóa cả trên remote lẫn local để tránh nhầm lẫn." }
]