Code Review Practices: PR Culture in GitHub

For .NET engineers who know: Azure DevOps pull requests — branch policies, required reviewers, work item linking, build validation You’ll learn: How GitHub’s PR culture differs from Azure DevOps, what a strong PR description and review look like in our team, and the mechanics of GitHub’s review interface Time: 10-15 min read

The .NET Way (What You Already Know)

Azure DevOps pull requests are well-integrated with the broader DevOps ecosystem. Work item linking connects PRs to user stories and bugs. Branch policies enforce reviewer requirements, build validation, and merge strategies. The review UI supports threaded comments and a diff view.

The PR process in many .NET shops is driven by policy: PRs are created because the branch policy requires them, reviewed because the required-reviewer policy mandates it, and merged when all checks pass. The culture tends toward “process compliance” — the PR exists to satisfy the gate.

GitHub’s PR culture grew differently. PRs originated in open-source projects where authors needed to persuade maintainers that a contribution was worth including. The description needed to explain the problem being solved and why this solution was correct. The reviewer needed to understand the change, not just approve it. This culture transferred into GitHub-hosted private repositories, including ours.

The same mechanics exist in both systems. The culture and expectations around them differ.

The GitHub Way

What Makes a Good PR Description

A good PR description does three things:

1. Explains what problem is being solved (and links to the issue or ticket)
2. Describes the approach taken and any alternatives considered
3. Tells the reviewer what to focus on

This matters because reviewers are context-switching from their own work. A description that explains the “why” means the reviewer spends less time reconstructing your reasoning and more time finding actual problems.

Our PR template — save this as .github/PULL_REQUEST_TEMPLATE.md in your repository:

## What and Why

<!-- What does this PR do, and why is it needed?
     Link the issue or ticket: Closes #123 -->

## Approach

<!-- How did you solve it? Why this approach over alternatives?
     Skip if the implementation is straightforward. -->

## Testing

<!-- How was this tested? What should reviewers verify? -->
- [ ] Unit tests added/updated
- [ ] Tested locally with dev server
- [ ] Edge cases considered: <!-- list them -->

## Review Notes

<!-- What should reviewers focus on?
     Flag any areas where you are uncertain. -->

## Screenshots (if UI changes)

<!-- Before / after for any visible changes -->

The Closes #123 syntax automatically closes the linked GitHub issue when the PR is merged. Use Fixes #123 for bugs and Closes #123 for features.

Good PR description example:

## What and Why

Adds invoice PDF generation when an invoice is marked as sent.
Closes #247.

Previously, clicking "Send Invoice" sent the email but attached no document.
Customers were calling in to request PDF copies.

## Approach

Uses Puppeteer to render the invoice HTML template to PDF server-side.
Considered PDFKit (programmatic generation) but the invoice template already
exists as a Handlebars HTML template — rendering it was significantly less
work than recreating the layout with PDFKit primitives.

PDF generation is synchronous in this implementation (acceptable for invoice
sizes we expect). Left a TODO for async/queue approach if generation time
becomes a problem at scale.

## Testing

- Unit tests: `invoice-pdf.service.test.ts` covers the template rendering
  and page size options.
- Manual: Sent a test invoice from the staging environment and verified
  the PDF attachment opened correctly in Gmail and Outlook.
- Edge case: invoices with 0 line items generate a valid (mostly empty) PDF.

## Review Notes

The Puppeteer setup in `src/lib/browser.ts` is the most likely area to need
adjustment — specifically the `executablePath` and launch args, which differ
between local macOS and the Linux CI environment. Flagging in case anyone
has run into this pattern before.

Poor PR description example:

## What and Why

PDF generation.

## Testing

Tested locally.

The second version tells the reviewer nothing. They will spend 10 minutes reading code before they can form an opinion.

PR Size Guidelines

Small, focused PRs are a discipline, not a convenience. They are easier to review, easier to revert, and easier to reason about in git history.

When a PR gets large, the review quality drops. Reviewers spend more time navigating the diff and less time thinking about correctness. The effective approval rate approaches rubber-stamping.

How to keep PRs small:

• Separate refactoring from feature changes. If you need to rename a service before adding a feature, do it in a separate PR (one commit, easy to review, zero risk).
• Separate migrations from the feature that uses them. The migration PR has no logic — it is easy to review. The feature PR does not include schema noise.
• If a PR touches more than three feature areas, ask whether it should be multiple PRs.

The one exception: PRs that are large because they add a lot of tests are fine. Test-heavy PRs are low-risk even when large.

The Review Interface

GitHub’s review UI has several features that Azure DevOps also has, but the interaction patterns differ.

Leaving a comment:

Click any line in the diff to add a single-line comment, or click and drag to select multiple lines. GitHub groups comments into a “review” — you can leave multiple comments and then submit them all at once as “Approve,” “Request changes,” or “Comment.”

This is different from Azure DevOps, where each comment is submitted independently. In GitHub, draft all your comments first, then submit the review with a verdict. This gives the author a single notification instead of one per comment.

Suggesting changes:

The most powerful review feature: click the suggestion icon (looks like a document with a plus) when leaving a comment to propose an exact replacement. The author can accept the suggestion with one click, and GitHub commits it on their behalf.

<!-- In a review comment, click "Add a suggestion" -->
Suggested change:
```suggestion
const result = await this.ordersService.findOne(id);
if (!result) {
  throw new NotFoundException(`Order #${id} not found`);
}
return result;

Suggestions are ideal for small, unambiguous fixes — typos, naming inconsistencies, missing null checks. Use prose comments for anything that requires judgment or discussion.

**Resolving threads:**

When the author addresses a comment (either by making the change or by explaining why they did not), they click "Resolve conversation." This collapses the thread and makes it easy to track which feedback has been addressed. As a reviewer, do not resolve threads yourself unless the author asked you to — it should be the author's action to indicate "I've handled this."

**Review states:**

- **Comment** — feedback only, no verdict on merge readiness
- **Approve** — you are satisfied; the PR is ready to merge (pending other required reviewers and CI)
- **Request changes** — the PR has problems that must be addressed before merge; you will need to re-review and approve afterward

Use "Request changes" sparingly. It is appropriate for correctness problems, security issues, and architectural concerns. For style suggestions or minor improvements, "Comment" is appropriate — you are not blocking the merge, just offering ideas.

### CODEOWNERS

The `.github/CODEOWNERS` file maps file paths to GitHub users or teams who are automatically added as required reviewers when those files change.

.github/CODEOWNERS

Default owners for everything not matched below

•                       @your-org/senior-engineers
  

Database migrations require a senior engineer

prisma/migrations/ @your-org/database-owners

CI/CD configuration changes require DevOps team approval

.github/workflows/ @your-org/devops

Auth-related code requires security review

src/auth/ @your-org/security-reviewers src/middleware/auth*.ts @your-org/security-reviewers

Frontend package changes require frontend lead

apps/web/ @your-org/frontend-lead

CODEOWNERS is enforced via branch protection rules. When a PR changes a file matching a pattern, the associated owner is added as a required reviewer — the PR cannot merge until they approve.

This is the equivalent of Azure DevOps's "Required reviewers by file path" branch policy.

### Branch Protection Rules

Configure branch protection for `main` in GitHub Settings → Branches → Branch protection rules:

Branch name pattern: main

✓ Require a pull request before merging ✓ Require approvals: 1 (or 2 for larger teams) ✓ Dismiss stale pull request approvals when new commits are pushed ✓ Require review from Code Owners

✓ Require status checks to pass before merging Required checks: - ci / lint - ci / type-check - ci / test ✓ Require branches to be up to date before merging

✓ Require conversation resolution before merging

✗ Allow force pushes (leave disabled) ✗ Allow deletions (leave disabled)

The "Require branches to be up to date" rule requires contributors to merge main into their branch before merging. This prevents the scenario where two PRs individually pass CI but conflict when both merge to main.

### Squash vs. Merge Commits

GitHub offers three merge strategies: merge commit, squash merge, and rebase merge. We use **squash merge**.

| Strategy | Result in main's history |
|---|---|
| Merge commit | All individual commits from the branch preserved, plus a merge commit |
| Squash merge | All commits from the branch collapsed into one commit on main |
| Rebase merge | All commits replayed onto main (no merge commit, all individual commits kept) |

**Why squash merge:**

- `main`'s git history stays clean — one commit per PR, each with the PR title and number
- Individual "WIP", "fix typo", "address review feedback" commits don't pollute the log
- `git log --oneline` on `main` reads like a changelog
- `git bisect` and `git blame` are more useful on a clean history

```bash
# What main's history looks like with squash merges:
# a1b2c3d feat: add invoice PDF generation (#247)
# d4e5f6g fix: correct webhook retry backoff logic (#231)
# g7h8i9j feat: add customer notification preferences (#219)

# Vs. with merge commits:
# a1b2c3d Merge pull request #247 from feat/invoice-pdf
# b2c3d4e Address PR feedback - fix undefined case
# c3d4e5f WIP: pdf generation working locally
# d4e5f6g Merge pull request #231 from fix/webhook-retry
# ...

Configure squash merge as the only option in GitHub Settings → General → Pull Requests:

✓ Allow squash merging — Default commit message: "Pull request title and description"
✗ Allow merge commits
✗ Allow rebase merging

Handling Review Feedback

When you receive “Request changes” feedback:

1. Read all comments before making any changes. Some comments may be related, and understanding the full picture avoids making changes that conflict.
2. Address each comment — either make the change, or reply with a clear explanation of why you did not.
3. Mark each addressed thread as resolved (click “Resolve conversation”).
4. Push the updated commits. GitHub will notify the reviewers.
5. Do not re-request review until all threads are resolved — reviewers should not have to re-check whether you addressed earlier feedback.

When you are the reviewer and the author pushes changes in response to your feedback:

1. Review the new commits, not the entire diff again. GitHub’s “Files changed since your last review” view helps.
2. Resolve threads that were addressed satisfactorily. Leave comments if changes raise new issues.
3. If all “Request changes” issues are resolved, change your review to “Approve.”

Team Norms for Turnaround Time

Our expectations:

• First review response: within one business day of a PR being opened.
• Follow-up reviews (after author pushes changes): same day if the PR was already in review.
• Blocking reviews (“Request changes”): the reviewer is responsible for staying engaged until the PR merges or is closed. A review that blocks a PR and then goes silent is worse than no review.

If a PR is open and you cannot review it within one business day, leave a “Comment” acknowledging it and give an estimated timeline. This is basic courtesy that significantly reduces context-switching costs for the author.

Review checklist — what to look for:

## Correctness
- [ ] Does the code do what the PR description says it does?
- [ ] Are there missing null checks or unhandled edge cases?
- [ ] Is error handling consistent with the rest of the codebase?
- [ ] Are async operations awaited everywhere they should be?

## Security
- [ ] No secrets or credentials in code
- [ ] Input validated before use in queries or commands
- [ ] Authorization checks present on new endpoints

## Design
- [ ] Does this fit the existing architecture patterns?
- [ ] Is the code in the right layer (controller vs. service vs. repository)?
- [ ] Is there anything that will be hard to change later?

## Tests
- [ ] Are tests present for the new logic?
- [ ] Do tests cover the error cases, not just the happy path?
- [ ] Are test assertions specific enough to catch regressions?

## Operational
- [ ] Are new environment variables documented?
- [ ] Does this require a migration? Is it included?
- [ ] Are there logging statements for significant operations?

Key Differences

Gotchas for .NET Engineers

Gotcha 1: “Approve” in GitHub Does Not Always Allow Merge

In Azure DevOps, an approval from a required reviewer is typically sufficient to unlock the merge button (combined with a passing build). In GitHub, several independent checks must all pass simultaneously:

• Required number of approvals
• No “Request changes” reviews outstanding (even from non-required reviewers by default)
• All required status checks passing
• Branch up to date with base branch (if configured)
• All conversations resolved (if configured)

The most common confusion: a reviewer who submitted “Request changes” previously must either re-approve or dismiss their own review before the PR can merge — even if the author addressed all the feedback. The author cannot dismiss another reviewer’s “Request changes” review by themselves (unless they have admin access).

If a PR is stuck despite looking like it should be mergeable, check the PR’s “Checks” section and the review status carefully. GitHub shows exactly which conditions are blocking the merge.

Gotcha 2: Force-Pushing to a PR Branch Discards Review History

Azure DevOps handles amended commits more gracefully. In GitHub, if you force-push to a branch (rewriting commits), any review comments attached to specific lines in the old commits become “outdated” and may lose their thread context. This makes it difficult for reviewers to see whether their feedback was addressed.

The rule: after opening a PR, only add new commits. Do not git commit --amend and git push --force to clean up your commit history. The PR branch is a shared record of the review process. Clean up the history happens automatically during the squash merge.

If you absolutely must force-push (e.g., to remove a accidentally committed secret), communicate it to the reviewer so they can re-review the changed commits.

Gotcha 3: GitHub Actions CI Status Must Be Configured — It Is Not Automatic

Azure DevOps build validation automatically runs the pipeline when a PR is opened. In GitHub, the CI workflow must be configured to run on pull_request events, and it must be added to the branch protection rules as a required status check.

If CI is not required, reviewers cannot rely on “all checks pass” as a signal that the code is correct. The setup required in your GitHub Actions workflow:

# .github/workflows/ci.yml
on:
  push:
    branches: [main]
  pull_request:               # This line is required for PR checks
    branches: [main]

And in branch protection:

Require status checks to pass:
  Required checks: ci / lint, ci / type-check, ci / test

If a check does not appear in the required checks dropdown, it has never run in a PR against that branch. Merge a small PR with the CI workflow configured first, then add the checks to branch protection.

Gotcha 4: Small PRs Feel Slower But Are Faster

.NET engineers accustomed to large feature branches (“I’ll do the whole story in one PR”) often resist breaking work into smaller PRs because it feels like more overhead. In practice, the opposite is true:

• A 150-line PR gets reviewed same day. A 800-line PR waits days for someone to find time.
• A small PR’s review takes 15 minutes. A large PR’s review takes an hour and still misses things.
• When a small PR has a bug, the cause is obvious. When a large PR has a bug, the investigation is significant.

The discipline is in the decomposition: learn to identify the smallest shippable unit of a feature and ship that first. The database migration, then the API endpoint, then the UI — not all three together.

Hands-On Exercise

Practice the complete PR workflow with a non-trivial change.

Step 1: Set up the repository infrastructure

In a repository you have admin access to, configure:

1. Create .github/PULL_REQUEST_TEMPLATE.md using the template from this article
2. Create .github/CODEOWNERS with at least one path-based rule
3. Configure branch protection on main with required reviews and required CI checks

Step 2: Open a PR using the full workflow

Create a feature branch, make a meaningful change (not a typo fix — something with at least 50 lines of new code), and open a PR:

1. Fill out the PR template completely
2. Make sure the description answers: what, why, approach, and what to review
3. Add yourself as a reviewer (or ask a team member)

Step 3: Practice the review workflow

As the reviewer (or ask a team member to review), practice:

1. Leave at least one inline suggestion using the suggestion feature
2. Leave at least one comment asking for clarification or raising a concern
3. Submit the review as “Request changes” (even if the changes are minor)

As the author, respond:

1. Accept the suggestion with one click
2. Address the comment with code changes or a reply explaining why not
3. Resolve the threads
4. Push the updated commits

Step 4: Verify the merge requirements

Confirm that all required checks pass and the PR meets all branch protection conditions before merging. Merge using squash merge.

After merging, run:

git log --oneline main -5

Confirm the squash merge produced a single clean commit with the PR title.

Quick Reference

PR Template (save as .github/PULL_REQUEST_TEMPLATE.md)

## What and Why
<!-- What does this PR do, and why? Closes #[issue] -->

## Approach
<!-- How? Why this over alternatives? -->

## Testing
- [ ] Unit tests added/updated
- [ ] Tested locally
- [ ] Edge cases: <!-- list -->

## Review Notes
<!-- What should reviewers focus on? Where are you uncertain? -->

Review Checklist

GitHub vs. Azure DevOps Quick Map

Key Branch Protection Settings for main

✓ Require pull request before merging
  ✓ Require approvals: 1
  ✓ Dismiss stale approvals on new commits
  ✓ Require review from Code Owners
✓ Require status checks to pass
  ✓ Require branches to be up to date
  (add your CI job names here)
✓ Require conversation resolution before merging
✗ Allow force pushes
✗ Allow deletions

Further Reading

• GitHub Documentation — About Pull Requests — Complete reference for GitHub’s PR features
• GitHub Documentation — About Code Owners — CODEOWNERS syntax and configuration
• GitHub Documentation — Branch Protection Rules — Branch protection configuration reference
• Conventional Commits — The commit message convention used by our team and compatible with automated changelog generation