Peer Code Review | Phase 3: Is the Pull Request Ready for Review?
- erinvh620
- Jun 2
- 7 min read
Intended Audience: software developers
TL;DR Apply a tailored “ready for review” checklist to ensure the PR is in a state where detailed code review will be productive.
In Phase 1, I ask whether the pull request (PR) is reviewable—is it scoped and structured in a way I can review without burning out or getting lost? I spend 1–2 minutes checking the PR’s size and structure to make sure a review would be time well spent.
In Phase 2, I read the ticket carefully to understand the problem the code is solving, then go through the PR description to gather context about the author’s chosen solution. I resolve any points of confusion along the way—until I have a solid grasp of both the problem space and the design decisions behind the code.
Now, in Phase 3, I ask: Is this PR actually ready to be reviewed? Not just can it be reviewed, but should it? I take 5 minutes to check whether all critical elements are in place and confirm that the code works as expected. If everything checks out, I move on to Phase 4. If not, I return the PR with a clear explanation of what’s missing—without reviewing the code itself.
Reviewable vs Ready for Review
You may have noticed I use both “reviewable” and “ready for review”—they aren’t the same thing.
A PR is reviewable when it minimises cognitive load for the reviewer. It’s scoped clearly, structured well, and easy to follow.
But even the most reviewable PR might not be ready for review. A PR that’s ready for review meets quality standards: it includes tests, analytics (where needed), metrics, and logging—and it reflects a thoughtful, complete implementation, not just a rough pass.
In earlier posts, I talked about applying a “ready for review” checklist before submitting my own work. In Phase 3, I’m on the other side of the fence: applying a similar checklist to code that’s been submitted to me for review.
Why Phase 3 Exists
Reviewing code that isn’t ready wastes everyone’s time. Code that’s not ready is likely to change—and there’s no point reviewing something that’s going to change.
It also leads to excessive back-and-forth, which means more interruptions and more context switching. That’s expensive for developers.
Phase 3 exists to prevent that.
Big Picture: What’s Missing?
In my experience, the biggest problems in a PR usually live in the changes that were left out—the tests the author forgot to write, the errors they forgot to log, the metrics they forgot to collect. These are the things that tend to cause the most trouble later on (if they get through review).
It’s easy to dive into the diff and get lost in the details—tweaking variable names, fixing formatting, suggesting shorter methods. But if I only look what was changed, I’m missing half the story.
That’s why, before I go line by line, I take a step back and ask: What’s missing from this PR that absolutely should be here?
This is where a checklist comes in handy.
How I Use My Checklist
I start with a generic checklist, which I’ve developed over more than a decade in iOS development:
Happy path
Tests
User analytics
Metrics / Telemetry
Error Handling
Documentation
Knowledge sharing
(I’ll walk through each item in detail below.)
Next, I tailor the checklist to the specific PR I’m reviewing, based on what I learned in Phase 2. For example, not every change will require documentation updates or knowledge sharing.
Then, I evaluate the PR against this tailored checklist. This helps me decide whether the PR is ready for deeper review.
I go through the full checklist—rather than stopping at the first item that’s missing from the PR—so I can give the author complete, consolidated feedback. This minimizes back-and-forth and saves time for everyone involved.
If the PR satisfies every checklist item, I move on to a detailed review (Phase 4).
If something’s missing, I don’t leave inline comments. I reach out directly to the PR author—via Slack, email, or a quick conversation—and then follow up with a written summary. That follow-up in writing is essential. I reserve PR comment threads for the actual code review.
This process ensures that when I do dive into the code, I’m reviewing something that’s complete and genuinely ready for thoughtful feedback.
My Checklist (as a Mobile/Frontend Developer)
In this section, I’ll walk through each item on the checklist I shared above.
I’ve developed this list over years of working in iOS development, so it reflects the kinds of things that typically matter in mobile and frontend work. It won’t necessarily apply to other types of development—like backend, embedded, or web—and I’m not suggesting anyone use it verbatim. Think of it instead as a jumping-off point for creating a checklist that fits your context.
Let’s dive in.
✅ Does the happy path work?
(If this section feels familiar, it’s because I originally included this step in Phase 2. But after further reflection, I realized it belongs here—as part of checking whether the PR is ready for review.)
I run the code to confirm that the primary user flow introduced or modified by the PR works as intended. If it doesn’t, the PR isn’t ready for review.
This isn’t full QA—I’m not testing edge cases or failure states. I’m just confirming that the basic, expected path works. If it doesn’t, I still go through the rest of the checklist before reaching out to the author, so I can provide complete feedback.
When I report the issue back to the PR author, I include:
The conditions under which I ran the code
What I expected to happen
What actually happened
That way, the author has everything they need to reproduce the problem. It’s not my job to solve it—they’re closer to the code and better positioned to fix it. And if they need my help, they can always ask.
✅ Are there tests?
I start by asking myself: what kinds of tests should be here? Unit tests? Integration tests? UI tests? Then I look for them in the PR.
If the tests are present but incomplete, I use judgment. A few missing edge cases in unit tests? Fine—that can be flagged with a review comment later. But a UI change without UI tests? Not okay.
Note: Some developers prefer to add tests in a separate PR. That doesn’t work. Here’s why.
✅ Are user analytics present?
If the PR introduces new user interactions, I expect to see user analytics being logged.
Yes, analytics can technically be added in a later PR—but that’s risky. This one might get deployed without them, leaving us flying blind.
If analytics should be present but aren’t, the PR isn’t ready for review. I make a note and continue down the checklist.
If the app doesn’t already have an analytics framework in place, this PR isn’t the place to add one. That’s a broader team decision. (The team might choose to block this PR until a framework is chosen and integrated—but that’s a separate conversation.)
✅ Are metrics (telemetry) being collected?
If the PR introduces changes that could affect performance—like launch time or memory usage—I look for metrics that confirm the impact. If no data is being collected, the PR isn’t ready for review.
If the change warrants a dashboard, I ask: Has it been created? Is anyone monitoring it? What questions is it meant to answer? It’s not enough to collect the data—there needs to be a plan for using it.
If metrics are warranted but missing, the PR is not ready for review. I make a note and continue through the checklist.
Again, if the app doesn’t already have a telemetry framework in place, this PR isn’t the place to add one. That’s a broader team decision. The team might choose to block the PR until a framework is chosen and integrated—but that’s a separate conversation.
✅ Are all errors handled?
Sometimes developers will leave a TODO in the code, instead of handling or logging an error. That’s not enough. Errors must be handled—and logged. If they’re not, the PR isn’t ready for review.
As with user analytics and telemetry, if a logging framework hasn’t yet been integrated, that’s a separate team-level conversation. This PR isn’t the place to introduce an entire logging solution from scratch. But if a logging system is already in place and the code introduces failure points, those failures should be accounted for.
✅ Has documentation been updated?
If the PR changes public APIs, onboarding workflows, shared processes, or high-level architecture, some form of documentation probably needs updating. That might be a README, a wiki, or even just inline comments. If the relevant docs haven’t been updated, the PR isn’t ready for review.
This is one reason I love keeping docs under version control—documentation changes are included directly in the PR! I, as the reviewer, don’t have to go hunting down other sources in order to complete my review.
If documentation should be updated and hasn’t been, this PR isn’t ready for review. I make a note and continue down the checklist.
If no documentation system exists yet, that’s a broader team-level discussion. This PR isn’t necessarily the place to introduce one from scratch (though adding inline comments or bootstrapping a basic wiki under version control is usually simple enough). It depends on the context.
✅ Is there a knowledge-sharing plan in place?
If the PR introduces something novel—a new tool, library, or architectural pattern—someone needs to explain it to the team, or at least let the team know. (In this case, “the team” includes everyone who works on the affected codebase.)
That might mean scheduling a big presentation. It might mean an announcement at the next few team meetings. Sometimes a Slack thread, short demo, or quick talk can work just fine. But the knowledge-sharing plan needs to exist before the PR is merged.
If the change requires knowledge-sharing but nothing is planned, the PR isn’t ready for review.
What to do if the PR is not ready for review
Don’t stop at the first item on the checklist that the PR is missing. Go through the entire checklist and consolidate your findings. Back-and-forth review cycles are expensive—in time, energy, and morale.
Once you’ve completed your checklist:
Contact the author directly. Face-to-face or video chat is best.
Follow up the face-to-face discussion in writing with a list of what the PR is missing.
Avoid leaving comments directly on the PR at this stage.
Sending a PR back without a detailed review reinforces an important message: incomplete work won’t be reviewed.
This Isn’t Slowing You Down—It’s Making You Faster
Spending just 5-10 minutes on a tailored checklist saves hours of redundant review effort later. That small investment saves me from reviewing code that isn’t ready—code that hasn’t been fully thought through and is likely to change. It also cuts down on review back-and-forths, which saves time and, more importantly, preserves morale.
When code review feels like a drag, it stops being worth it. The benefits—better developer experience and better user experience—only materialize if we keep the process efficient and respectful of everyone’s time.
That’s why I don’t review code that isn’t ready. And that’s why Phase 3 exists.
Comments