How to Over-Engineer Pull Request Reviews

The phase "I'm reviewing your pull requests" makes you sound pretentious on a literal level. What do you mean by reviewing other people's PR when most of them are more senior than you? I'm just kidding. To be fair, I think taking away the pressure of "I have to review your PR to decide if your code should go to prod" off your shoulder can make you a lot more productive; it gives you more patience to understand parts of the codebase you haven't touched / seen, it helps you ask better questions rather than nitpicking on syntax, and it helps giving you more assurance to verify "these changes are legit, I can take the blame if shit goes wrong in prod". Before reviewing a PR itself, I usually ask myself: Should I be reviewing this PR? Why is it "me" who should be reviewing this PR? Do I have expertise in the stuff I'm about to review? What value do I provide other than giving them a green tick? Which parts of this PR should I be responsible of reviewing, while I let other experts review stuff that they're good at? Once I start reviewing a PR, I don't just start reviewing line by line. I have a workflow: Open the repository of the PR locally Pull from the latest main branch (or target branch) Spin up local development environment - at least! Check-out the PR branch in your IDE (I use Cursor) using the VsCode Github extension Review the PR diff in your local IDE, not on Github web browser To see the latest comments from other reviewers or the author of the PR, open the Github PR web version as a reference When I first started working in the industry, I couldn't wrap my head around how could senior engineers can just tell something is off in a PR just by reviewing the diff from the web browser. Turns out most of them don't review from the browser. They run the PR branch locally so they have the full file/folder/repo context. Another crucial reason why it's better to review them in your local IDE is that, sometimes you have to review stuff that's not changed in the PR, for lines changed to make sense. This stuff never shows up in the PR diff section. For example, if you're reviewing a PR to do with SDK upgrade, the method name and input type for a function completely changed, you can see the stuff people made changes to, all of it can make sense; but what you're missing out on is the stuff that they forgot to update. If you happen to be in a team where the codebase has no quality CI checks on PRs, you'll never see what's broken until that PR caused an outage that woke you up at 3am. For most PRs making updates to system behaviours, being able to run the PR branch locally is a must-have. You can't approve something you haven't physically verified to be working as expected. If shit happens in prod and people ask why you approved the change that broke the system, you are responsible for outlining why you had the confidence to merge those changes. For other types of PRs, such as documentation updates, CI workflow edits, config changes, most can be reviewed in the web browser, no need to pull them locally; but it varies depending on the PR. Now, let's move onto "actually reviewing the PR" itself. I often ask myself these questions: What problem is being addressed? What's the problem statement? Why does it have to be addressed now? What's the expected wrong behaviour, can I reproduce the issue locally? All of these questions should at least be mentioned in the PR description, or a link to a Jira/Linear ticket somewhere. For new feature related PRs, the "why" is more of a question you should've aligned & agreed on on well-before the sprint (or dev cycle has started), it shouldn't be "new ideas". Going deeper, into the technicals (finally), I tend to ask myself these questions: What's the minimal amount of code changes required to achieve our goal? For changes in line x, what happens if we change it to "y", or "z"? What if we leave it unchanged? What are some of the invisible side-effects of this change? Will shits blow up in those "rare" edge cases? Will I be okay with taking the blamed if I let this stuff go through to prod? Am I nitpicking syntax, coding style, superficial preferences, or am I actually adding value with my comments? What are the changes that I'm not a huge fan of, but am okay with approving, knowing that the core changes of this PR already addresses the main problem? If the goal is to introduce a hard-coded value (infrastructure configs, environment variables, some magic app-level configs, etc), where does the value come from? Why should I trust it's the right value? What's the decision path we went through to land on the conclusion to use this particular value? Will it ever get outdated sometime down the line? What are the consequences if this value gets outdated? Sometimes, a single feedback on PRs can become full-on philosophical discussions about product decisions or design principles. It's important to know when to press the brake, either by moving the thread to another dedicated channel, creating another ticket for a major change in scope, or just commit to the minimal changes in the currenr PR with an interim solution, while you and the team works on a more long-term solution to the flagged issue. Reviewing pull requests goes in both ways. PRs that tell reviewers ways to test the changes is always helpful, though sometimes this knowledge is already shared so it doesn't need to be stated in every PR description. What I do find most helpful is when you state the "wrong past/current behaviours" in PR descriptions, then compare it to the "new/correct behaviours" of your PR, so I know exactly what problem your changes are trying to address. In an ideal world, you should ask yourself all of the questions above so you have 100% confidence approving a PR; but in the real world, no one has absolutely 100% confidence all the time. Sometimes, going with the "vibe" might be the only option, but it's still important to be as exhaustive with reviews as you can.