The Senior Engineer’s Guide to Code Reviews: What to Look For Beyond Syntax

The pull request sits there with 47 files changed and a description that reads “implement new billing module.” You open it, scroll through the diff, and start doing what most engineers do: scanning for typos, checking variable names, and pointing out that one function should probably be private. Twenty minutes later, you approve it. Two weeks later, the billing module causes an outage because it doesn’t handle partial failures in the payment gateway integration, and nobody caught it in review.

This is the failure mode of most code review processes. Teams treat reviews as a syntax and style check — a glorified linter pass performed by a human. But the bugs that actually matter in production almost never come from naming conventions or missing semicolons. They come from architectural mistakes, unhandled edge cases, security oversights, and subtle performance problems that only show up under load.

Senior engineers review code differently. They spend less time on surface-level issues and more time asking questions like: “What happens when this external service is down?” and “Will this query still perform when the table has 50 million rows?” and “Does this change make the next feature harder to build?”

This guide covers what experienced engineers actually look for during code review, how to give feedback that improves both the code and the author, and how to build a review culture that doesn’t slow your team to a crawl.

The Architectural Review: Does This Belong Here?

Before reading a single line of code, look at which files were changed and how the changes are organized. This tells you more about the quality of a PR than any individual function.

Dependency direction: Does this change introduce a dependency that flows in the wrong direction? If your billing module now imports from notifications, that might be a design problem. Core business logic should not depend on infrastructure or presentation layers. In a well-structured codebase, dependencies point inward — from controllers to services, from services to domain models, never the reverse.

Responsibility boundaries: If a change to the user profile feature requires modifications in the order processing module, the boundaries between these modules are probably wrong. A PR that touches many unrelated modules is often a sign of tight coupling, and approving it without comment means accepting that coupling as the new normal.

Interface design: When a PR introduces a new function, class, or API endpoint, evaluate the interface before the implementation. Is the function signature clear about what it accepts and returns? Could a developer who’s never seen this code use it correctly based on the name and parameters alone? Bad interfaces get calcified quickly — once other code depends on them, changing the signature becomes a much bigger project.

# Bad: What does this function actually do? What's the dict structure?
def process(data: dict, flag: bool = False) -> dict:
    ...

# Better: Clear types, clear purpose
def calculate_order_total(
    line_items: list[LineItem],
    apply_discount: bool = False,
    tax_region: TaxRegion = TaxRegion.US
) -> OrderTotal:
    ...

The “next feature” test: Think about what the team will need to build in the next quarter. Does this PR make that work easier or harder? A quick implementation that works for the current requirement but paints the team into a corner for the next one is not a good trade-off, even if the code itself is clean.

Security Review: The Checks Most Teams Skip

Security bugs are expensive. A SQL injection or an authentication bypass discovered in production costs orders of magnitude more to fix than one caught in review. Yet most code reviews spend zero time on security because reviewers assume “we have security tools for that.”

Automated security scanners catch known vulnerability patterns. They don’t catch logic errors in your authentication flow or authorization gaps in your API. Here’s what to look for manually:

Authentication and authorization: Every new API endpoint should have explicit authorization checks. If you see a new route handler without middleware that verifies the user’s permissions, that’s a red flag. Don’t assume the framework handles it — verify.

// Express.js example -- spot the problem
app.get('/api/admin/users', async (req, res) => {
    // Where is the authorization check?
    // Any authenticated user can access the admin user list
    const users = await UserService.getAllUsers();
    res.json(users);
});

// Fixed
app.get('/api/admin/users', requireRole('admin'), async (req, res) => {
    const users = await UserService.getAllUsers();
    res.json(users);
});

Input validation and sanitization: Any data that comes from outside your trust boundary — user input, webhook payloads, data from partner APIs — must be validated before use. Check that the PR validates types, ranges, and formats. Watch for cases where user input is interpolated into SQL queries, shell commands, or template rendering without sanitization.

Secrets and credentials: Scan the diff for hardcoded API keys, database passwords, or tokens. This seems obvious, but it happens more often than anyone admits. Also check for secrets that might be logged — if a function logs the full request object and that object contains an authorization header, you’ve got a credential leak in your log aggregator.

IDOR (Insecure Direct Object Reference): When an endpoint accepts an ID parameter, verify that the code checks whether the requesting user has access to that specific resource. A common mistake:

// Vulnerable: any authenticated user can view any invoice
app.get('/api/invoices/:id', requireAuth, async (req, res) => {
    const invoice = await Invoice.findById(req.params.id);
    res.json(invoice);
});

// Fixed: verify ownership
app.get('/api/invoices/:id', requireAuth, async (req, res) => {
    const invoice = await Invoice.findById(req.params.id);
    if (!invoice || invoice.userId !== req.user.id) {
        return res.status(404).json({ error: 'Not found' });
    }
    res.json(invoice);
});

Performance Review: What Will This Do Under Load?

Code that works fine in development with 100 rows in the database can fall apart spectacularly in production with 10 million rows. During review, think about the production data volumes and traffic patterns.

Database queries: This is where most performance problems live. Check for:

• N+1 queries: A loop that executes a database query on each iteration. ORMs make this especially easy to introduce accidentally. If you see a loop over a collection where each item triggers a lazy-loaded relationship, flag it.
• Missing indexes: If the PR adds a new query with a WHERE clause on a column that isn’t indexed, it will become a full table scan as data grows. Check the migration files — is there a corresponding index for the new query pattern?
• Unbounded queries: Any query without a LIMIT clause is a potential time bomb. SELECT * FROM events WHERE user_id = ? looks fine until a power user has 500,000 events.

# N+1 problem in Django ORM
# This executes 1 query for orders + N queries for items
orders = Order.objects.filter(status='pending')
for order in orders:
    items = order.items.all()  # Each iteration hits the database
    process_order(order, items)

# Fixed with select_related / prefetch_related
orders = Order.objects.filter(status='pending').prefetch_related('items')
for order in orders:
    items = order.items.all()  # No additional query -- already loaded
    process_order(order, items)

Memory usage: Does the code load entire datasets into memory? Watch for patterns like Model.objects.all() without pagination, reading entire files into a string when line-by-line processing would work, or building large lists when a generator would be more appropriate.

External service calls: If the PR adds a call to an external API in the request path, consider: What’s the timeout? What happens if the service is slow or down? Is there a circuit breaker? An external API call with a 30-second default timeout in your checkout flow will ruin your users’ day when that service has a bad afternoon.

Concurrency: If the code reads a value, makes a decision based on it, and then writes — but other threads or processes could modify that value in between — you have a race condition. This is common in inventory management, account balance updates, and any “check-then-act” pattern.

# Race condition: two concurrent requests could both see quantity=1
# and both succeed, overselling the product
def purchase_item(item_id, quantity):
    item = db.query("SELECT quantity FROM inventory WHERE id = %s", item_id)
    if item.quantity >= quantity:
        db.execute("UPDATE inventory SET quantity = quantity - %s WHERE id = %s",
                   quantity, item_id)
        return True
    return False

# Fixed with atomic operation
def purchase_item(item_id, quantity):
    result = db.execute(
        "UPDATE inventory SET quantity = quantity - %s "
        "WHERE id = %s AND quantity >= %s",
        quantity, item_id, quantity
    )
    return result.rowcount > 0

Error Handling: The Happy Path Is Not Enough

Most code in a PR implements the happy path — what happens when everything works correctly. Senior reviewers spend disproportionate time looking at what happens when things go wrong.

Partial failure: If a function performs three steps (charge the customer, create the order, send the confirmation email) and step two fails, what happens? Is the customer charged without an order? Is there a rollback mechanism? Distributed transactions across services are notoriously hard to get right, and reviewing them requires thinking through every failure combination.

Retry behavior: If the code retries a failed operation, is the operation idempotent? Retrying a non-idempotent operation (like charging a credit card) without a deduplication mechanism will create real problems. Check that retry logic includes exponential backoff and a maximum retry count.

Error messages: Do the error messages contain enough context for debugging? An error log that says “Failed to process request” is useless at 3 AM. An error log that says “Failed to charge customer cus_abc123: Stripe API returned 402 with message ‘card_declined'” is actionable.

Graceful degradation: When a non-critical dependency fails, does the application degrade gracefully or crash entirely? If the recommendation engine is down, the product page should still load — just without recommendations. Review code to ensure that optional features have proper fallback behavior.

The Review Checklist by PR Type

Not all pull requests need the same level of scrutiny. A typo fix in a README doesn’t need a security review. A new payment integration needs everything. Here’s a practical checklist organized by PR type:

Feature PRs

• Does the feature match the specification or ticket requirements?
• Are there adequate tests covering the happy path AND edge cases?
• Is the database migration reversible? Does it lock tables on large datasets?
• Are new API endpoints documented (OpenAPI spec, README, etc.)?
• Is there feature flag support for gradual rollout?
• How does this feature behave for existing users with existing data?
• Are there monitoring/alerting additions for the new functionality?

Bug Fix PRs

• Is there a test that reproduces the bug before the fix?
• Does the fix address the root cause or just the symptom?
• Could this same class of bug exist elsewhere in the codebase?
• Is the fix backward compatible? Could it break existing clients or integrations?
• Is there a risk of regression? Are existing tests still passing?

Refactoring PRs

• Is the refactoring isolated from behavioral changes? (Never mix refactoring and feature changes in the same PR.)
• Do existing tests still pass without modification? If tests needed changes, why?
• Does the refactoring actually improve something measurable — readability, performance, testability — or is it just different?
• Is the refactoring complete, or does it leave the codebase in an inconsistent state where some code uses the old pattern and some uses the new?

Infrastructure / DevOps PRs

• Are there rollback procedures documented for infrastructure changes?
• Have resource limits (CPU, memory, disk) been set appropriately?
• Are secrets managed through a secret manager, not hardcoded or in environment files checked into version control?
• Is there a way to test this change in a staging environment before production?
• What’s the blast radius if this change fails? Does it affect all services or just one?

How to Give Feedback That Teaches

The way you write review comments matters as much as what you write. A code review is a conversation between colleagues, not a judgment handed down by a gatekeeper. Bad review culture is one of the fastest ways to tank team morale and slow down delivery.

Distinguish between blocking and non-blocking comments. Use a clear prefix or label system. At many teams, this looks like:

• blocker: This must be fixed before merge. Security issues, data loss risks, correctness bugs.
• suggestion: I think this would be better, but it’s your call. Style preferences, alternative approaches.
• nit: Trivial issue. Typo, formatting, naming preference. Don’t block a PR on nits alone.
• question: I don’t understand this. Help me learn, or add a comment for the next person.

Without this distinction, every comment feels like a blocker, and authors spend time addressing trivial feedback while real issues get lost in the noise.

Explain the “why,” not just the “what.” Instead of “Use a constant here,” write “This timeout value appears in three places. If we need to change it later, we’d have to find and update all of them. A named constant would make that easier and document the intent.” The first version tells the author what to do. The second teaches them a principle they can apply to future code.

Ask questions instead of making demands. “What happens if this list is empty?” is better than “You need to handle the empty list case.” Questions invite the author to think through the problem. Sometimes they’ll realize there’s a bug. Sometimes they’ll explain why the list can never be empty, and you’ll both learn something.

Acknowledge good work. If a piece of code is well-structured, handles edge cases thoughtfully, or uses a clever technique, say so. Reviews that are 100% criticism create a defensive culture where people dread opening PRs. A quick “Nice approach here — the retry logic with exponential backoff is exactly right” costs nothing and reinforces good practices.

Don’t rewrite their code in comments. If you find yourself writing a 20-line code snippet in a review comment, something has gone wrong. Either the issue is significant enough to warrant a conversation (walk over to their desk, or hop on a call), or you should open your own PR with the suggested changes. Long code blocks in review comments are hard to read, hard to discuss, and often come across as condescending.

Review Velocity: Moving Fast Without Cutting Corners

Slow code reviews kill productivity. If a PR sits unreviewed for two days, the author has context-switched to something else. When review comments finally arrive, they have to reload the mental context of the original change, address feedback, and then wait again. This ping-pong can turn a one-day feature into a one-week feature.

Here are concrete practices that keep reviews fast:

Set a response time SLA. Many high-performing teams target a first-pass review within 4 business hours. This doesn’t mean the review is complete in 4 hours — it means the author gets initial feedback and knows what to work on next. Google’s engineering guidelines suggest that a reviewer should provide feedback within one business day at the absolute latest.

Keep PRs small. The single most effective thing you can do for review quality and speed is to keep pull requests under 400 lines of meaningful changes. Research from Microsoft (published in their “Modern Code Review” paper) found that review effectiveness drops dramatically after about 200-400 lines of diff. Beyond that, reviewers start skimming rather than reading carefully. If a feature requires 2,000 lines of changes, break it into a series of smaller PRs that build on each other.

Use draft PRs for early feedback. If you’re unsure about an architectural approach, open a draft PR with the skeleton and ask for feedback before writing all the tests and error handling. Getting alignment on the approach early prevents the painful scenario of a fully-implemented PR getting rejected for fundamental design reasons.

Automate everything automatable. Linting, formatting, type checking, and basic test execution should happen in CI, not in review. Every minute a human spends pointing out a formatting issue is a minute they’re not spending on architectural or logical review. In 2026, there’s no excuse for not having automated formatting. Use Prettier for JavaScript/TypeScript, Black or Ruff for Python, gofmt for Go, and enforce it in CI. Remove style discussions from reviews entirely.

# .github/workflows/ci.yml -- automate the boring stuff
name: CI
on: [pull_request]
jobs:
  checks:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Lint
        run: npx eslint . --max-warnings 0
      - name: Format check
        run: npx prettier --check .
      - name: Type check
        run: npx tsc --noEmit
      - name: Test
        run: npm test -- --coverage --ci

Rotate reviewers. Don’t funnel all reviews through one senior engineer. It creates a bottleneck, burns out that person, and prevents the rest of the team from developing review skills. Assign reviewers round-robin or by area of ownership, and use required reviewers only for high-risk changes (security-sensitive code, database migrations, public API changes).

Building a Healthy Review Culture

Code review culture is ultimately about trust. Do authors trust that reviewers are trying to help, not gatekeep? Do reviewers trust that authors have thought through their changes? Building that trust takes intentional effort.

Review your own code first. Before requesting a review, read through your own diff as if you were the reviewer. You’ll catch obvious issues, leftover debug statements, and TODO comments that should have been addressed. This shows respect for the reviewer’s time and sets a higher baseline for what they need to check.

Write good PR descriptions. A PR with a one-line description that says “fix bug” is asking the reviewer to reverse-engineer the intent from the diff. A good description includes: what the change does, why it’s needed (link to the ticket or issue), how it works at a high level, what you considered but decided against, and how to test it. This context dramatically speeds up review and leads to better feedback.

Don’t take review comments personally. This is advice that’s easy to give and hard to follow, especially for less experienced developers. It helps to remember that comments are about the code, not about you. It also helps when reviewers follow the feedback practices described above — framing comments as questions and suggestions rather than criticisms.

Treat review as a learning opportunity. Every PR is a chance to learn something — a new library, a different approach to a problem, a domain concept you weren’t familiar with. The best engineering teams treat review as a knowledge-sharing mechanism, not just a quality gate. Junior engineers learn from seeing how seniors structure code. Seniors learn from seeing fresh perspectives and new tools that junior engineers bring in.

Measure and improve. Track metrics like time-to-first-review, review cycle time (from first review to merge), and PR size distribution. These numbers tell you whether your review process is healthy or becoming a bottleneck. If the average time-to-merge has crept from 1 day to 5 days over six months, that’s a signal to investigate. Maybe PRs are getting too large, maybe the team needs more reviewers, or maybe the review standards have become unreasonably high.

What Code Review Is Not

Code review is not a replacement for testing. If you find yourself relying on reviewers to catch bugs that tests should catch, your test coverage is insufficient. Reviews are for the things that tests can’t easily verify: design quality, maintainability, security posture, and alignment with the team’s architectural direction.

Code review is not a design review. By the time code is written, the design decisions are largely made. If you disagree with the fundamental approach, the time to raise that is during the design phase — in an RFC, a design doc, or an early draft PR — not after the author has spent a week implementing it.

Code review is not a status report. If your manager is reading PRs to understand what the team is working on, that’s a process problem, not a code review problem.

Final Thoughts

The difference between a team that does code review well and one that does it poorly isn’t the tooling (GitHub, GitLab, Gerrit — they all work fine). It’s the mindset. Good review teams understand that the goal is collective code ownership and continuous improvement, not individual approval or gatekeeping.

If you’re a senior engineer, your review comments shape how junior engineers think about code. Take that responsibility seriously. Don’t just flag problems — explain the reasoning, share the experience that taught you to look for that specific issue, and make it clear that asking questions is not just acceptable but expected.

If you’re earlier in your career, actively seek out review feedback and study how experienced reviewers comment on other people’s PRs. The patterns you learn — checking for race conditions, thinking about failure modes, evaluating interface design — will make you a better engineer faster than almost any other practice.

Code review done right is one of the highest-value activities in software engineering. It catches bugs before they reach production, spreads knowledge across the team, maintains architectural coherence as the codebase grows, and creates a shared standard of quality that makes everyone’s work better. Invest in it.