Anatomi #

Imagine a reviewer opening a PR titled only “fix bug” with an empty description and 400 lines of changes scattered across 15 files. What do they do? Most likely: approve with a short “LGTM” comment because they don’t know where to look. Bugs that should have been preventable slip into production, and the reviewer can’t be blamed — they had no context at all. A bad PR doesn’t just waste reviewers’ time; it actively weakens the quality control process that should be the last line of defense before code touches production. This article covers every PR component in depth — what it does, how to write it well, and what the result looks like in practice.

Why PR Anatomy Matters #

A well-structured PR has a measurable impact on the engineering process:

PR without structure:
  Reviewers don't know where to start
  → Read the entire diff top to bottom without context
  → Reviews take 2x longer
  → Comments given aren't on target
  → Bugs slip through because reviewers are exhausted

PR with good structure:
  Reviewers immediately understand context and purpose
  → Focus on the most critical parts
  → Faster, more meaningful reviews
  → More productive technical discussions
  → Bugs are easier to detect because reviewers know what to look for

A good PR also serves as technical decision documentation — six months later, when someone asks “why was this code written this way?”, the answer is in the PR thread, not in the head of an engineer who may no longer work at the team.

PR Anatomy Components #

flowchart TD
    PR[Pull Request] --> T["Title\nDescriptive one-line summary"]
    PR --> D["Description\nContext, solution, and impact"]
    PR --> S["Scope\nClear boundaries of changes"]
    PR --> C["Code Changes\nClean implementation"]
    PR --> TV["Test & Validation\nEvidence the changes are tested"]
    PR --> IR["Impact & Risk\nImpact on other systems"]
    PR --> CL["Checklist\nQuality consistency guardian"]

PR Title #

The title is the first thing seen by reviewers, stakeholders reading changelogs, and engineers searching old PRs in history. A good title is a one-line summary explaining what changed and why it matters.

The most consistent, readable format follows the pattern: [Type] Specific description.

// Formats by change type

feat: Add rate limiting to public API endpoints
fix: Resolve race condition in concurrent order status update
refactor: Extract payment validation logic into dedicated service
chore: Upgrade grpc-go dependency to v1.62
docs: Update API documentation for order status endpoints
perf: Optimize product listing query with composite index

// Without a prefix — still must be specific
Add retry mechanism with exponential backoff to payment callback
Fix null pointer when user has no active subscription
Remove deprecated coupon calculation logic

Good vs Bad Title Comparison #

// ✗ Titles that don't help reviewers or the future

"Update code"

"Fix bug"

"WIP"

"Changes"

"Improve performance"     ← what was improved? by how much?
"Fix issue from QA"       ← what issue? from which QA session?
"Hotfix"                  ← a hotfix for what?

// ✓ Titles giving immediate context

"Fix race condition in concurrent order status update causing stale data"
"Add rate limiting (100 req/min) to public product search endpoint"
"Refactor payment service to use strategy pattern for multi-gateway support"
"Fix null pointer exception when user has no active payment method"
"Migrate session storage from database to Redis for better performance"
Test your PR title with this question: “If someone read this title in a changelog or git log six months from now, would they immediately understand what changed?” If the answer is no, fix the title.

PR Description #

The description is the most important component of a PR. A good description can save 30-60 minutes of reviewer time because they don’t need to guess context from the code they read.

An Effective Description Structure #

## Background

[Explain the current condition and why this change is needed.
 Include data or concrete examples if available.]

## Changes

[What changed technically. Focus on important decisions,
 not implementation details already visible in the code.]

## Alternatives Considered

[Other approaches considered and why they weren't chosen.
 This section is optional but very useful for non-obvious decisions.]

## Testing

[How this change has been tested. What tests were added,
 and how to run manual tests if needed.]

## Impact and Risk

[What's the impact on other systems? Any breaking changes?
 Needs migration? Any risks to monitor after deploy?]

Before vs After Description Example #

// ✗ A useless description
## Description
Fixed the payment issue that was reported.

// ✓ A description giving full context
## Background

The payment callback endpoint (`POST /webhooks/payment`) fails entirely when
a network timeout occurs from the payment gateway. In the last 30 days,
47 callbacks failed (error rate ~3.2%), leaving orders stuck in `pending`
status even though payment actually succeeded.
Users must contact CS for manual confirmation.

## Changes

Added a retry mechanism with exponential backoff for network errors:
- Max 3 retries
- Initial delay: 1 second, multiplier: 2x, maximum: 8 seconds
- Only retry recoverable errors (timeout, 503) — not 400/401

Implemented using the `retryablehttp` library already in dependencies.

## Alternatives Considered

**Dead Letter Queue (DLQ):** A more robust approach, but needs
message queue infrastructure that doesn't exist yet. Keep as an RFC for
evaluation next quarter if retries still aren't enough.

## Testing

- Unit test: `TestPaymentCallbackWithRetry` in `internal/webhook/payment_test.go`
- Integration test: timeout simulation with a mock server (see `testdata/mock_timeout_server.go`)
- Manual: set `PAYMENT_GATEWAY_URL` to a mock server returning 503

## Impact and Risk

- Latency in failure cases rises by ~9 seconds max (3 retries × 3 seconds average)
- No breaking changes to the API contract
- Idempotency keys already exist at the payment gateway — safe to retry
- Monitor the `payment_callback_retry_count` metric after deploy

Change Scope #

Clear scope helps reviewers understand the PR’s boundaries and avoids review sprawling into out-of-context territory.

// ✗ Unclear scope
PR titled "Update payment service" but contains:
  - Fix callback handler (relevant)
  - Refactor the entire service layer (unrelated)
  - Update 3 dependencies (could be its own PR)
  - Fix typos across various files (noise for reviewers)

// ✓ Limited, consistent scope
PR titled "Fix payment callback retry on network timeout":
  - Only files related to callback handling
  - No changes outside the domain named in the title
  - Unrelated changes moved to a separate PR

How to identify scope that needs splitting:
  Question: "Can this change be rolled back on its own without
             affecting other changes in this PR?"
  If not → split it into a different PR

Code Changes #

This is the heart of the PR, but its quality is determined not just by what changed, but by how understandable the change is.

Review-Friendly Code Guidance #

// Give context through clear naming
// ✗ Hard to understand without reading the whole function
func process(d []byte, n int) ([]byte, error) {
    // ...
}

// ✓ Naming that explains intent
func encryptPaymentData(rawPayload []byte, keyVersion int) ([]byte, error) {
    // ...
}

// Use comments for decisions not obvious from the code
// ✗ Magic numbers without context
time.Sleep(3 * time.Second)

// ✓ Constants with explanations
const paymentGatewayTimeoutBuffer = 3 * time.Second // Buffer beyond the gateway SLA (2 seconds)
time.Sleep(paymentGatewayTimeoutBuffer)

// Split long functions into smaller units
// ✗ A 100-line function doing everything
func handlePaymentCallback(w http.ResponseWriter, r *http.Request) {
    // parse, validate, process, retry, respond — all in one function
}

// ✓ Small functions with single responsibilities
func handlePaymentCallback(w http.ResponseWriter, r *http.Request) {
    payload, err := parseCallbackPayload(r)
    if err != nil { ... }
    
    if err := validateCallbackSignature(payload); err != nil { ... }
    
    if err := processPaymentWithRetry(payload); err != nil { ... }
    
    respondSuccess(w)
}

Point Reviewers to the Most Important Parts #

For large PRs, authors can add PR comments to guide reviewers:

// Author comments in the PR guiding reviewers

Comment on `internal/webhook/handler.go`:
"This is the main change — a new function for retry logic.
 Please focus review on the error classification at lines 45-67."

Comment on `internal/webhook/retry.go`:
"This is a new file. The retry implementation follows the same pattern as
 other services in the payment domain — see `internal/payment/client.go`
 as a reference."

Tests and Validation #

Tests aren’t just about coverage — they’re about giving reviewers (and your future self) confidence that the change works as expected.

What to Include #

UNIT TESTS:
  □ Tests for every new or changed logic path
  □ Happy path tests — the expected normal case
  □ Error handling tests — how the code responds to failures
  □ Relevant edge case tests (empty input, boundary values, concurrency)

INTEGRATION TESTS (if relevant):
  □ Tests involving inter-component interactions
  □ Tests with mocks for external dependencies

HOW TO RUN TESTS (in the PR description):
  # Unit tests
  go test ./internal/webhook/...
  
  # Tests with the race detector (for concurrency)
  go test -race ./internal/webhook/...
  
  # Integration tests
  go test -tags=integration ./internal/webhook/...

MANUAL TEST SCENARIOS (for changes hard to test automatically):
  1. Setup: Set PAYMENT_GATEWAY_URL=http://localhost:9999 (mock server)
  2. Trigger: POST /webhooks/payment with a valid payload
  3. Expected: Request retried 3x, logs show the retry count
  4. Cleanup: Reset PAYMENT_GATEWAY_URL to the production value

Tests as Documentation #

Well-written tests are documentation that can’t lie — code changes but a failing test tells you immediately:

// ✗ An uninformative test
func TestRetry(t *testing.T) {
    // test something
    assert.NoError(t, err)
}

// ✓ A test documenting expected behavior
func TestPaymentCallback_RetriesOnNetworkTimeout_MaxThreeAttempts(t *testing.T) {
    // Given: a payment gateway that always times out
    mockGateway := newMockGateway(t).AlwaysTimeout()
    handler := NewCallbackHandler(mockGateway)
    
    // When: a callback is received
    err := handler.ProcessWithRetry(validPayload)
    
    // Then: 3 attempts happen before finally returning an error
    assert.ErrorIs(t, err, ErrMaxRetriesExceeded)
    assert.Equal(t, 3, mockGateway.CallCount())
}

Impact and Risk #

This section is the most often skipped but the most critical for ops and on-call teams. A change that doesn’t document its impact is a change not ready for production.

// A complete impact and risk template

## Impact on Other Systems

| Component | Impact | Category |
|---|---|---|
| `payment-service` | Latency rises by max 9 seconds in failure cases | Performance |
| `order-service` | No change | - |
| Monitoring | Add `payment_callback_retry_count` metric | Infrastructure |

## Breaking Change

No breaking changes to the API contract.
Response schema unchanged.

## Database / Migration

No database schema changes.

## Deployment Notes

- Can be deployed directly without special coordination
- No feature flag needed
- Safe rollback — no irreversible data changes

## What to Monitor After Deploy

- `payment_callback_retry_count` — expected present but not too high
- `payment_callback_error_rate` — expected to drop from ~3.2%
- P99 latency of `/webhooks/payment` — may rise slightly, alert if > 15 seconds

Ready-to-Use PR Template #

Here’s a PR template you can use directly or adapt as the PR template in GitHub/GitLab:

## Background

<!-- Explain the current condition and why this change is needed.
     Include data or ticket references if available. -->

Closes: #[issue number]

## Changes

<!-- What changed technically. Focus on important decisions. -->

## Alternatives Considered

<!-- Other approaches considered and why they weren't chosen.
     Remove this section if not relevant. -->

## Testing

<!-- How this change was tested. -->

**How to run tests:**
```bash
# [command to run tests]

Manual test scenarios: (if relevant) 1. 2. 3.

Impact and Risk #

  • No breaking changes to the API contract
  • No non-backward-compatible database schema changes
  • Safe rollback

What to monitor after deploy: #

Author Checklist #

  • Self-review done
  • CI passed (tests, lint, build)
  • No debug code or forgotten TODOs
  • Description provides enough context for reviewers
  • Tests cover the important happy paths and error cases

---

## PR Anatomy Anti-Patterns to Avoid

// ✗ Generic titles giving no context

“Update”, “Fix”, “Changes”, “WIP”, “Hotfix” // ✓ Specific titles answering “what changed and why”

// ✗ Empty or one-sentence descriptions for large PRs 800-line PR: Description: “Added new feature” // ✓ Descriptions explaining background, changes, and impact

// ✗ PRs mixing too much context New feature + unrelated refactor + dependency update + typo fixes → Reviewers are confused about where to focus // ✓ One PR, one purpose — split out the unrelated parts

// ✗ No information about how to test

“I’ve tested it, should be fine” // ✓ Include test commands and manual scenarios if needed

// ✗ Not mentioning breaking changes or impact on other systems A PR changing the API response schema without mentioning it in the description → The frontend suddenly errors because it didn’t know about the change // ✓ Always mention breaking changes and coordinate with affected parties

// ✗ PR checklists left empty or all checked without verification

  • All tests pass (while CI is still red) // ✓ Checklists filled honestly — unfinished items are better left unchecked

---

## Good PR Anatomy Checklist

TITLE: □ Specific — describes what changed and why □ No more than 72 characters □ Uses a team-consistent format (feat:/fix:/refactor: etc.)

DESCRIPTION: □ Background explains the current condition and why the change is needed □ Technical changes explained at the right level (not too detailed, not too abstract) □ Alternatives considered listed if the decision isn’t obvious □ Testing approach explained — including runnable commands □ Impact on other systems stated explicitly

SCOPE: □ One PR, one purpose — no mixing different contexts □ Out-of-scope changes explicitly mentioned if they must exist

CODE: □ Clear, consistent naming □ No debug code or forgotten TODOs □ No commented-out code without a reason □ Comments explain why, not what

TESTS: □ Tests added for every new piece of logic □ Happy paths and error cases covered □ CI passed before the PR was opened for review

IMPACT: □ Breaking changes explicitly stated (or “no breaking changes”) □ Database / migration changes listed □ Post-deploy monitoring needs mentioned


---

## Summary



  
    

  • PR titles must be descriptive — reviewers read dozens of PRs; clear titles save their time before they even open the diff.
    • 

  • The description is the most important component — good context in the description produces faster, more meaningful reviews.
    • 

  • Explain why, not just what — code shows what changed; the description must explain why this change is needed.
    • 

  • One PR, one purpose — PRs mixing too much context can’t be reviewed well.
    • 

  • Include concrete testing information — runnable commands are more useful than a statement of “already tested”.
    • 

  • Always mention breaking changes and impact — changes that don’t document their impact shift the risk onto reviewers and ops teams.
    • 

  • PRs are technical decision documentation — six months from now, a good PR description can answer questions the code can’t.
    • 

  • PR templates remove friction — the team doesn’t need to remember what must exist; templates ensure consistency automatically.
    • 







---





  


  ← Previous: Fundamental


&nbsp;




  


  Next: Small vs Big PR →
About | Author | Content Scope | Editorial Policy | Privacy Policy | Disclaimer | Contact