Pull Requests and Code Review

Pull Requests (PRs) are the foundation of modern collaborative software development. While they aren't technically a part of Git itself, they are the most critical feature of platforms like GitHub, GitLab, and Bitbucket.

---

1. Visualizing the Pull Request Lifecycle

A Pull Request is a bridge between your isolated work and the shared codebase.

---

2. Anatomy of a Great Pull Request

Opening a PR is more than just clicking a button. To get your code merged quickly, you must provide context to your reviewers.

A Good PR Description should include:

• The "Why": What problem are you solving?
• The "How": High-level overview of your technical approach.
• Testing: How can a reviewer verify your changes?
• Screenshots/Videos: If you've made UI changes, show them!

Example PR Template:

## Description
Implements the OAuth2 login flow for Google and GitHub.

## Changes
- Added `auth/google` and `auth/github` routes.
- Created `UserSession` model to store tokens.
- Updated login UI with social buttons.

## Verification
- [x] Manual testing on localhost:3000
- [x] Unit tests for token validation passing
- [x] Tested with invalid tokens (proper error handling)

---

3. The Code Review Feedback Loop

Reviewing code is a conversation, not a critique. It is an iterative process where the author and reviewer work together to ensure the code is correct, maintainable, and readable.

Visualizing the Review Loop

---

The Four Phases of Feedback

Phase 1: The Self-Review (Author)

Before notifying anyone, look at your own PR. You will often catch obvious mistakes, console logs, or TODOs you forgot to remove.

• Tip: View the "Files Changed" tab exactly as a reviewer would.

Phase 2: The Initial Pass (Reviewer)

The reviewer looks at the big picture first:

• Does this solve the problem?
• Is the architecture sound?
• Are there any massive performance or security holes?
• Outcome: High-level comments or specific "Requested Changes."

Phase 3: The Dialogue (Both)

This is where the threaded conversations happen.

• Author: "I used this approach because..."
• Reviewer: "Ah, I see. What if we use a Map here instead of an Array for faster lookups?"
• Author: "Great point! Let me update that."

Phase 4: The Resolution (Reviewer)

Once all comments are addressed and the code is updated:

• Approve: The code is ready to merge.
• Comment: General thoughts without blocking the merge.
• Request Changes: Mandatory fixes are required before merging.

---

Etiquette and Tone: The "Golden Rules"

Technical communication can easily be misinterpreted. To maintain a healthy team culture, follow these rules:

For Reviewers:

1. Critique the Code, Not the Person: Your goal is to improve the codebase, not to win an argument. Use neutral language like "This variable name might be misinterpreted" instead of "You named this variable poorly."
2. The "Big Picture" First: Before diving into individual lines, check if the high-level approach is correct. Is the architecture consistent with the rest of the project? Does it introduce any new dependencies that we should avoid?
3. Use the "I" Perspective: Frame feedback as your own experience of reading the code. "I find this logic hard to follow" is less aggressive than "This logic is confusing."
4. Praise and Encourage: Code review isn't just about finding flaws. If you see an elegant solution, a clever optimization, or great test coverage, say so! Positive reinforcement builds a stronger team.
5. Distinguish Between "Must-Fix" and "Optional": Clearly label your comments so the author knows how to prioritize.
   • blocker: or required: - This must be changed to prevent a bug, performance regression, or security vulnerability.
   • question: - I'm curious about why you chose this path; help me understand.
   • nit: (Nitpick) - A small stylistic preference that doesn't affect correctness. The author can choose to ignore these.
   • suggest: or thought: - An alternative approach that might be worth considering.

For Authors:

1. Prepare for Review: Your job isn't done when the code is written. Perform a "Self-Review" first—read your own PR from top to bottom. You'll catch 50% of your own mistakes this way.
2. Don't Be Defensive: Every comment is an opportunity to learn. If a reviewer is confused by your code, it’s an objective sign that the code could be clearer, regardless of how "right" your logic is.
3. Acknowledge and Respond: Don't leave your reviewers hanging. Use emoji reactions (👍, ✅) to show you've seen a comment, and leave a brief reply if you've fixed it or if you have a question.
4. Explain the "Why": If you disagree with a suggestion, don't just say "no." Provide technical reasoning. "I chose approach X over Y because Y has a known memory leak in our specific environment."
5. Batch Your Updates: Instead of pushing a new commit for every single comment, try to address all (or most) feedback in a single push. This makes it much easier for the reviewer to verify your changes in one go.
6. The Goal is the Merge: Remember that both you and the reviewer want the same thing: high-quality code in the main branch. Work together to reach that goal as efficiently as possible.

---

Handling Disagreements

Sometimes, two developers simply don't agree on a solution.

1. Take it Offline: If a thread goes back and forth more than 3 times, jump on a quick call or meet in person. Text is bad for nuance.
2. Seek a Third Opinion: Ask another teammate to weigh in.
3. Follow Team Standards: If the team has an agreed-upon style guide, use that as the tie-breaker.

Key Takeaway: A Pull Request is a shared responsibility. Once merged, the whole team "owns" the code, not just the original author.

---

---

4. PR Merge Strategies

When a PR is ready, there are different ways to bring those changes into main.

Merge Commit (Default)

All commits from your feature branch are added to the main branch, along with a special "Merge Commit."

• Pros: Preserves the full history of your branch.
• Cons: Can make the history look "messy" with many small WIP commits.

Squash and Merge

All commits from your branch are combined into one single, clean commit.

• Pros: Keeps the main history ultra-clean and easy to read.
• Cons: Loses the fine-grained history of how the feature was built.

Rebase and Merge

Replays your commits on top of the latest main.

• Pros: Keeps a perfectly linear history without merge commits.
• Cons: Can be confusing if not managed carefully.

---

5. Summary Table