Code Review in Pull Requests: What to Look For

1. What code review is actually for

A pull request that is only checked for indentation, naming conventions and missing whitespace has wasted its potential. Formatting questions like that belong in a linter or automatic formatter, not in a human reviewer's attention. Code review earns its value exactly where tools stop: at the question of whether the change actually does what it claims, whether it hooks into the right place in the system, and whether it will still be understandable in six months.

A good reviewer asks, for every line, why it was written this way and not another. That requires understanding the context of the change, not just reading the diff lines in isolation. In Magento projects that means additionally: does the change fit the existing module boundaries, does it use Service Contracts instead of direct repository access, does it respect the plugin architecture instead of overriding core classes. These questions cannot be automated, they require review with intent.

2. Logic correctness and edge cases: reading the diff with intent

The core of every review is whether the logic stays correct for all relevant inputs, not just the happy path the author tested. Empty arrays, null values, negative numbers, duplicate IDs, concurrent requests: these edge cases are rarely visible explicitly in the diff, they have to be actively kept in mind. Anyone reading a method that filters a collection should ask what happens when the collection is empty or when an expected attribute is missing.

Before evaluating individual lines, it is worth looking at the full context of the change, not just the isolated diff snippet shown in the PR interface. With git diff against the base branch and git log for the commit history, you understand why a file was changed and whether earlier commits in the same PR already raise questions.

# Inspect a PR branch before reviewing line by line
git fetch origin pull/482/head:pr-482
git checkout pr-482

# Full diff against the target branch, not just the last commit
git diff main...pr-482

# See every commit in the PR, not just the squashed result
git log main..pr-482 --oneline --stat

# Function-context diff: see which method a change belongs to
git diff main...pr-482 -- src/Model/Cart.php --function-context

These commands show the PR the way it will actually merge into the base branch, including intermediate steps that a PR tool's web interface often collapses. Anyone who only reads the final diff view in the browser can easily miss that a variable was introduced in an earlier commit and renamed again in a later one, which obscures the actual intent of the change.

3. Test coverage: does the PR test the claimed behavior

A change without an accompanying test is a claim, not a guarantee. The reviewer should check whether the new or changed tests actually cover the behavior the PR description promises, rather than just exercising the code path without a sharp enough assertion. A test that only calls assertNotNull() where a specific value is actually expected fakes a confidence that does not exist.

For bug fixes, the most important question is whether a regression test exists that would fail without the fix. Without that test, the same bug can silently return during the next refactor. For Magento modules this specifically means: unit tests for ViewModels and service classes, integration tests for repository interactions with the database, and for critical payment or inventory logic, an additional look at API tests covering the full request-response cycle.

Test coverage does not mean code coverage in percent. A module can reach 90% line coverage and still leave the critical edge cases untested, because the percentage only measures which lines were executed, not whether the assertions ask the right questions. Reviewers who actively ask about missing test cases, for example "What happens here if the cart is empty?", find gaps no coverage report shows.

4. Scope creep: keeping a PR focused on one concern

A pull request should contain a single, clearly nameable change. As soon as a PR incidentally reformats an entire folder, renames an unrelated method, or introduces a second feature that has nothing to do with the actual ticket, it becomes unreadable for the reviewer. This mixing is called scope creep, and it is one of the most common reasons reviews either get rubber-stamped superficially or sit unreviewed for weeks.

A PULL_REQUEST_TEMPLATE.md with a clear checkbox structure forces authors to explicitly state the scope of the change before the reviewer even looks at the code.

## What does this PR do?
<!-- One sentence. If you need more, the PR is probably too big. -->

## Scope
- [ ] This PR touches exactly one concern
- [ ] No unrelated formatting or renaming changes included
- [ ] Follow-up work is tracked in a separate ticket, not bundled here

## Testing
- [ ] Unit tests added or updated
- [ ] Manually verified against staging

## Backward compatibility
- [ ] No breaking changes to public interfaces
- [ ] If breaking, deprecation path documented below

When a reviewer spots scope creep, the right response is not to reject the PR outright, but to name concretely which part should be carved out: "Renaming getPrice() to getFinalPrice() makes sense, but belongs in its own PR so this one stays focused on the discount logic." That keeps the change history in the Git log traceable and makes later git bisect runs far more meaningful.

5. Security and backward compatibility in a Magento context

Security-relevant questions belong in every review, regardless of whether the PR is explicitly flagged as security-related. For Magento modules this specifically means: is user input treated with escapeHtml() or escapeHtmlAttr() before output, are SQL queries built exclusively through the query builder or with bound parameters instead of string concatenation, and does a new admin controller check the correct ACL resource in acl.xml before exposing sensitive data.

Backward compatibility is the second dimension that is often overlooked in pure functional testing. A change to a public interface, for example adding a required parameter to a method of a Service Contract, breaks every third-party module that implements or calls that interface. The reviewer should specifically check whether a method is public, protected, or part of an Api/Interfaces namespace, because only there does Magento's strict backward compatibility policy apply.

For preference-based changes to core classes, it is also worth checking whether a plugin should be used instead, since preferences can collide with every core update, while plugins are far more robust against Magento version changes. A reviewer who asks this question prevents silent breaking changes that only surface in production during the next Composer update.

6. Concrete vs. vague feedback: phrasing review comments with suggestions

"This is wrong" or "I don't like this" are comments that force the author to ask the reviewer what is actually meant, instead of continuing directly. A review comment should contain three things: what is problematic about the code, why it is problematic, and ideally a concrete alternative suggestion that makes the point actionable without further back and forth.

# Vague, forces a round trip
# "this looks wrong, please fix"

# Actionable: names the problem, explains the risk, suggests a fix
# "getCustomerGroup() can return null for guest checkout (see
#  CustomerSession::getGroupId()). This will throw a TypeError
#  on line 42 since getGroupId() is typed as int. Suggest:
#  $groupId = $this->customerSession->getGroupId() ?? GroupManagement::NOT_LOGGED_IN_ID;"

Concrete feedback saves more time overall than it costs to write, because it reduces the number of review rounds. A reviewer who provides code examples instead of pure descriptions makes it easy for the author to adopt the suggestion directly instead of guessing at the solution themselves. Many PR tools also allow suggestion comments the author can accept with one click, which speeds up the workflow especially for small, unambiguous fixes.

7. Review etiquette: nit vs. blocking, tone and comment prefixes

Not every comment in a review carries the same weight, and that distinction must be immediately clear to the author. Conventional prefixes like nit: for small, optional style points, question: for genuine understanding questions without a change request, and blocking: or must-fix: for points that absolutely must be resolved before merging, create this clarity without extra discussion.

The tone of a review comment should always address the code, never the person. "This method is duplicated in three places" reads completely differently from "you copied this code three times," even though both name the same problem. Questions instead of statements ("Why is a try-catch used here instead of an early return?") open a dialogue, while categorical statements often shut one down from the start.

Etiquette matters in the other direction too: an author who treats every nit comment as a personal attack makes future reviews more uncomfortable for the whole team. Teams that document these conventions in a short CONTRIBUTING.md save themselves recurring misunderstandings and speed up onboarding new team members into the review culture.

8. Keeping PRs small: splitting, stacked PRs and draft PRs

A PR with over 800 changed lines statistically gets reviewed less thoroughly than a PR with 100 lines, because the cognitive load for the reviewer grows sharply with diff size. The usual consequence is a superficial "LGTM" after a quick skim, instead of a real check of logic and edge cases. The most effective countermeasure is planning PRs to be small from the start: one ticket, one change, one manageable diff.

When a feature inevitably needs several steps that build on each other, for example first a new interface, then the implementation, then integration into the controller, the pattern of stacked PRs fits well: several small, dependent PRs, each based on the previous one instead of bundling everything into one mega PR.

# Stacked PRs: branch B builds on branch A, not on main
git checkout -b feature/cart-api-interface main
# ... implement the interface, open PR #1 against main

git checkout -b feature/cart-api-implementation feature/cart-api-interface
# ... implement it, open PR #2 against feature/cart-api-interface

# After PR #1 merges into main, rebase the stack
git checkout feature/cart-api-implementation
git rebase --onto main feature/cart-api-interface feature/cart-api-implementation
git push --force-with-lease

Draft PRs are the third tool against unnecessarily large or premature reviews. A PR marked as draft signals to the team that it is not review-ready yet, but still enables CI feedback and early direction feedback. Once the author has addressed changes after a review, they should explicitly re-request review, instead of silently expecting someone to notice the new commits on their own.

9. Using Git tools to understand context before reviewing

Before commenting on a single line, it is worth looking at the history of the affected file. git blame shows who last changed a line and in which commit, which often explains why a seemingly odd solution actually had a good reason, for example an earlier bug fix that forced exactly that behavior.

# Understand why a line looks the way it does before questioning it
git blame -L 40,55 src/Model/Cart/TotalsCalculator.php

# Follow the line back through renames and refactors
git log --follow -p -- src/Model/Cart/TotalsCalculator.php

# Find the commit that introduced a specific line, with message
git log -S "roundingMode" --oneline -- src/Model/Cart/TotalsCalculator.php

These tools prevent a common review trap: writing a comment like "Why is this rounding up instead of down?" when git blame shows in seconds that exactly this was decided three months ago in a linked ticket fixing a tax rounding bug. Checking context first leads to better questions and avoids relitigating decisions already discussed.

The overview below summarizes the typical differences between ineffective and effective review behavior, from the phrasing of individual comments to the size of the entire pull request.

In practice these points reinforce each other: small PRs make it easier to give concrete instead of vague feedback, because the reviewer can actually take in the full context of a manageable change. Anyone who also consults git blame and git log before the first comment asks better questions and speeds up the whole review cycle for the team.

10. Summary

Code review in pull requests only pays off once it goes beyond formatting. Logic correctness and edge cases are caught by reading the diff with intent and understanding the full context through git diff and git log, instead of relying on the isolated view in the PR tool. Test coverage means the assertions actually verify the claimed behavior, not just execute a line of code. Scope creep, security holes and broken backward compatibility are the points that most often go unnoticed when a review stays too superficial.

The second lever is how feedback is delivered: concrete instead of vague, with code examples, clear prefixes like nit: and blocking: to weight comments, and a factual tone that always addresses the code. Small, focused PRs, split into stacked PRs if necessary, and draft PRs for early feedback are what make thorough reviews realistic in the first place. Combining these points consistently turns code review from a formality into a real quality lever.

11. FAQ: Code Review in Pull Requests