Mistakes I See Engineers Making in Their Code Reviews
Posted3 months agoActive2 months ago
seangoedecke.comTechstoryHigh profile
calmpositive
Debate
60/100
Code ReviewSoftware DevelopmentBest Practices
Key topics
Code Review
Software Development
Best Practices
The article discusses common mistakes engineers make during code reviews, sparking a discussion on best practices and the role of code review in software development.
Snapshot generated from the HN discussion
Discussion Activity
Very active discussionFirst comment
5h
Peak period
57
12-24h
Avg / period
17.8
Comment distribution160 data points
Loading chart...
Based on 160 loaded comments
Key moments
- 01Story posted
Oct 25, 2025 at 12:42 AM EDT
3 months ago
Step 01 - 02First comment
Oct 25, 2025 at 5:53 AM EDT
5h after posting
Step 02 - 03Peak activity
57 comments in 12-24h
Hottest window of the conversation
Step 03 - 04Latest activity
Oct 30, 2025 at 4:49 PM EDT
2 months ago
Step 04
Generating AI Summary...
Analyzing up to 500 comments to identify key contributors and discussion patterns
ID: 45701404Type: storyLast synced: 11/20/2025, 8:00:11 PM
Want the full context?
Jump to the original sources
Read the primary article or dive into the live Hacker News thread when you're ready.
> Many engineers seem to think it’s rude to leave a blocking review even if they see big problems, so they instead just leave comments describing the problems. Don’t do this. [...] Just leaving comments should mean “I’m happy for you to merge if someone else approves, even if you ignore my comments.”
Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ?
It's like ignoring someone telling you you're stepping into a hole because they're not grabbing you by the neck. Reviews shouldn't be that adversarial nor hand holding.
I'm also realizing, everywhere I work a comment is basically a blocking, except if it's explicitely flagged or discussed as optional. Trying to find some other person to review and ignore the comment is just a big NO.
Yes, yes they do.
And some devs will go the other way - not matter how minor a comment you leave, even just those that are tagged nitpick/FYI, they must address them.
I'm in agreement with most of the points, but I still find that most of the PRs I review, I block and request changes. Maybe that says more about me and the devs I've worked with...
Approval is approval. If you complete your review and don't block, you have concretely indicated to the author that in fact nothing is wrong with the diff and it is ok to be merged as is no matter what else you said. Many authors won't even look at comments if their change is approved.
Ignoring the comments is a tactic of a careless coworker. The diff may get merged and they can move on, sure. But come review time, they may find something else in their work environment is being rejected and removed.
By leaving a non-blocking review, the coworker has said that they have no strong objection to merging the code as is.
If it makes you feel better, just think of ignoring non-blocking comments as equivalent to reading them and disagreeing.
That's why you are at work and get compensated. I don't need to motivate you to do your work, that's something you should discuss with your boss.
> it is for the author to take the reviewer at their word about whether the change needs additional work or not.
How can the reviewer know that? The reviewer can only point at issues that tangent their part of the code, or what problems they think this will cause. Whether this is intentional or accident can only be known by the author.
> By leaving a non-blocking review, the coworker has said that they have no strong objection to merging the code as is.
No they say e.g. they would be know they need to work around the issue on their side, but prefer no to, because this comes at a costs for the company in work-hours and whether that is something you want to cause is your choice.
It seems like you think code reviews are a way to put someone else in charge of your change and deflecting the blame to him. You are still responsible for your work. Code reviews prevent you from accidentally breaking things, reducing the error rate by having more eyes and knowledge looking at your code. They won't prevent you from willfully breaking things.
> think of ignoring non-blocking comments as equivalent to reading them and disagreeing.
This is fine and expected, but we already have that discussion elsewhere.
Their additional work on updating the code also comes at a cost for the company in work-hours. If you suspect that the cost of your work-around is greater than the cost to update their code and you don't block, you're the one being negligent.
My non-blocking comment is exactly a symptom of me being unable to decide whether I would block or accept it. I just ask for the author to improve the PR so this can be decided on.
Non-blocking could cover everything from “wow this is kind of jank, but it’s good enough for now and let’s figure out how we fix it later” through to very minor thing like “you have a typo”.
If we blocked on everything being perfect, we’d never get anything finished. Plus, PR’s are free.
Writing a non-blocking review feels a bit like writing to /dev/null. Good to get it out of my system but not much else.
If it's important enough to be tracked then I'd want to make sure that it's been filed properly. It's too easy to forget about such a trivial task, especially in a time crunch.
I trust that it will get done. It is not important to me, whether it happens before the PR goes in or afterwards.
non blocking may be resolved if you have a strong argument for why you did it that way (most people don't, so it ends up being a code change as well).
If they just push anyways, then post-push there are gonna be some awkward conversations. If it becomes common, other people would know to just not approve CRs for that person until they are pristine. It will basically just slow development speed down in the long run.
People should be made aware that that's a bad and dangerous practice, because by doing that they are directly requesting unreviewed changes, because it means that now nobody needs to review the follow-up to their review. People need to learn to not sign off on code that they believe needs to be changed. Saying "I believe this code is wrong and needs to be fixed but am confident that the next version you produce will be completely correct without further review" is lazy reviewing and leads to inevitable disaster.
That's where I see little need for a "blocked" state, that's already what not approved should mean, short of explicitely saying otherwise.
The PR's author will always have ways around dealing with someone's feedback, and I think they should, otherwise there would be too many deadlocks (especially during incident respons etc.)
I general I prefer teams where devs give a shit and seek feedback as a positive. In my current job we allow deployment of self approved MRs in that spirit.
Only when that is you social agreement. To others this has the meaning of "objection, but not rejection".
If you care about whether your concerns are addressed by further discussion, block. If you don't actually care about whether something happens as indicated by not blocking, it's passive aggressive to expect more work.
People forget that you can just as easily block pending responses to comments and then approve after.
Some people leave flood of comments that cosplay as explanation, but they are not one. Or I simply disgree.
I am just getting to the point of ignoring them, because the alternative is to write an essay over each of those comments just for it to be ignored and the person passive aggressively ignoring the patch forever.
Blocking means "I don't approve and no one else should either."
That just sucks... because with that mindset typically nobody approves and leaves the submitter begging for approvals.
Having non-blocking comments leaves room for the discussion you want.
It's your job as the PR submitter to advocate for your code and shepherd it through.
Either you, indeed, work with the reviewer who made the comments to resolve them, or you have the option to seek out another if you think the feedback isn't valid enough to address.
Edit: TBH I don't get why you'd see a non-blocking comment differently, eg not meaning "let's get this done".
Someone just leaving comments and not signing off on reviews isn't helping unblock anyone and should put in more effort to be willing to sign off and move the work forward. If the most people in the org thought this way nothing would be committed and everyone would have 'non-blocking' comments to deal with.
Another way to look at this is in absence of another code reviewer, not signing off after commenting is equivalent to passively blocking the PR and can be a bit toxic depending on the circumstance.
I'm probably missing a scenario (maybe there's a bunch of people you know will review the code for instance) that this makes sense so happy to learn where/when specifically it makes sense :)
Blocking a PR can also be toxic "depending on the circumstance".
I see zero toxicity in leaving comments without blocking. It's never prevented the people I've worked with from getting work done.
I've worked at three large tech companies and none of them had this "block PRs" mentality but they all got stuff done. The reviewers understand their roles: they leave feedback, if there are questions, they answer them. If the feedback's handled, they re-review.
It works exactly the way you say it should, minus the "blocked/changes requested" status on a PR. Maybe precisely because we understand that a PR is blocked until it's approved anyway, and the green check is the goal.
All the opportunities for dysfunction are the same: people can still bikeshed, they can not review, they can not come back and re-review, etc. None of that is affected by the "changes requested" vs "comment" dichotomy.
Frankly, the "we can't collaborate without blocking PRs" take seems strangely dysfunctional to me.
As for leaving comments without blocking I do not mean it is always or even commonly toxic but that I've seen instances where it could be argued to be so or potentially unhelpful.
I think the misunderstanding might be when you or your team leave comments without blocking are you going to sign off after they are addressed or are you leaving them on a review you ultimately don't feel comfortable signing off on even if they're addressed?
How often does someone leave comments on a review they would never feel comfortable signing off on either way because they don't know the area? I think I'm in agreement with you - leaving comments without blocking and signing off after they're addressed or if someone else signs off and mine aren't addressed that's fine. I'd block the review if it was something I was that concerned with.
For instance sometimes the translation isn't consistent with other screens, but that's not an issue I'm willing to follow to the bitter end. If that's the only thing I have concerns about, leaving a comment to check with the copy writing team and let that team approve or not is a decent course of action.
Same with security issues, queries that will cost decent money at each execution, design inconsistencies, etc.
In these cases, not approving is also less ambiguous than approving but requesting extra action that don't require additional review from us (assuming we're very explicit in the comment about being ok with the rest of the code and not needing a re-review)
I totally agree with you on being explicit about why approval isn't given.
I'll say that there are lots of things that make any/some of us suck at PR reviews that I don't think are made worse or better by this "always approve or request changes" vs "comment without approval or requesting changes is okay" difference.
If someone blocks your PR, no matter what time you believe you've finished addressing the PR comments, your PR will not go anywhere while that person is out of the office, in a meeting, or working on their own story. They've just linearized the development process.
And given the previously mentioned phenomenon, they have to be available, read your changes, unblock your PR, and then your other coworkers have to check their PR inbox, do their reviews, and then you have to make more rounds of changes.
So if a block is involved early in a review, your code basically goes through two full rounds of PR. It can lead to whiplash if the reviewers need to argue amongst themselves.
And if this is just a PR to refactor code so you can get on to the meat of your ticket, then you're blocked, not just your PR. And god help you if someone "doesn't get the point" of a PR that doesn't exhibit clear forward motion on your story. So they make it difficult to make the change easy, and then do a second PR to make the easy change.
Although at this point, I think something that's missing from all of these discussions (spawned from whatever ancestor comment in this thread) is what the actual policies/culture/expectations are in our respective projects.
We're all going on about things like there's one absolute truth that should be followed and that is clearly not the case.
If you want me to write an essay in the response comment, then freaking call or something.
This somewhat violates the articles “approval or not is the only bit that matters” argument, but it at least forces participants to at least acknowledge an issue, and either provide a satisfactory reason to resolve on the spot, or open a new ticket for that work.
You want to block me, but also want it to look like my decision instead of yours.
Doing "something", that solves the issue that is described in the comment?
That can range from actually changing the code, putting a comment in, over updating the documentation, moving the code somewhere else, to merging the PR unchanged and opening a new PR, to not doing anything.
> You want to block me, but also want it to look like my decision instead of yours.
Because it IS your decision. What the comment means for the effect on your PR depends on how you address it. "changing the code" -> "new code review"; "putting a comment in", "updating the documentation" -> don't need approval, but expect questions; "not doing anything" -> get your approval from someone else, you won't get it from me, but if someone else is fine with it, maybe I am wrong.
I DO NOT want to block you, that's why I want to be able to only leave a comment.
Examples:
"This conflicts with POSIX and SUSv4 ..." -> (but the old code also did) "document that diversion and leave a point in todo.txt"
"This is highly surprising for users" -> "add a warning or a confirmation"
"This breaks the workflow for X people (including me), but this was undocumented behaviour" -> "Consider whether the feature is worth it and we care about this"
"This conflicts with my understanding of RFC1234 in combination with 'random blog post from The Old New Thing' and documentation in 'different vendor's codebase'" -> "Yes, I know, I did it on purpose, because of footnote in RFC5678 and random comment on HN proving why Raymond Chen is wrong; will document that".
The answer to all these comments could also be "Oops, didn't thought of that, now I need to change the code / throw everything away.", but this entirely depends on your state of mind, which I don't have introspection to. If I would start researching that problem in depth, then I could also do the PR myself, that is work, that you already did. I only want to make sure, that you are aware. If you are, consider it approved, if you are not, consider it blocked.
I kinda hate this.
It will feel efficient for the two people involved, but I'd actually want that exchange to be public and in writing, as we're talking about code that will go to production and will be read, debugged and maintained by dozen of other people from there.
Especially if it was complex enough to be discussed to these lengths. I don't care about code comments and don't trust people to properly maintain them, as long as MRs have the original context and discussion of the decisions. Shorting these discussions is a net loss to the org IMHO.
And a call where we react to each other takes 3 minutes max while the exchange through comments can take a week. Or, 2 days but both of you need to interrupt what you are doing a lot to answer quickly.
It is just absurd to treat code review as a discussion forum.
My biggest realization was that the code comments were meaningless and borderline deceptive compared to the actual discussions and the target specs in the MRs. There's such a gap between what people want to explain or think will be useful, and the info actually needed. And I don't blame them, I'm not sure I'd do better.
My favorite is threads like "This name makes no sense" -> "Sure, give me a better name for X doing Y to Z" -> "Nevermind", which completely explains some of the insane naming, what they actually meant by it, and would never be left in any half official documentation.
"This name makes no sense" -> "Sure, give me a better name for X doing Y to Z" -> "Nevermind"
Should really result in a comment "/* X doing Y to Z */" before the declaration. I try to address these issues by treating PRs the same as phone calls, they can be gone tomorrow, so I put all the things in the code or the commit message. But of course I am not perfect, the people after me will determine whether I was successful.
Maybe you’re not the best person to approve the code, or you just don’t have time to commit to the reviewer role at this second, but you’ve still spotted something that you think will help the person writing the code; so, leave a comment.
If the reviewer didn't accept your PR you already know they're expecting something more, and what it is will be in the comments.
I don't know if GitHub comments can be individually marked as optional or not, but even then the reviewer might not know the actual impact of what they're pointing at.
For instance if you're deleting a bit of feature that has been already disabled at the step before, a reviewer pointing it out as blocking might as well be lacking context. I find it more natural for a comment to be mostly factual and let you check if it needs a fix or not.
It's more work on your part, but it's also your PR (all of that within reason, and not being jerks)
I point out things that aren't functional blockers all the time. If I see there's a gap from the code itself, but maybe weren't considered (is there a ticket to deal with this? I'm not going to guess what it's named or where it is), I'll point it out. If it's not blocking, it's an approval from me, but someone else may disagree when they read it as well.
Not sorry that you think it's indecisive. This is technical feedback, which is separate from product feedback. You should still read your review comments and other people's review comments. If I was in charge of things to the point only my approval matters AND I was sure the author would respond, it would be an approval. Generally, this is not the case.
I have worked with individuals who would assign NULL to a pointer, then immediately dereference it. I would bring up that this obviously would not do anything useful. The response I would get was that they already had the code working in their dev. environment, so there could not be any bugs.
I've worked in teams where I'm the only one who actually checks out the branch to poke around
A great idea, though; when we finally get automated testing in CI, I'll certainly suggest it :)
This can help you identify additional pieces of feedback that you might have missed by just reading the code in a web interface.
The CI pipeline may not wholly be in your team's control. I've worked at places where a 'devops' team had too much control, liked to tinker all the time and often broke stuff. Nothing we could do - devops were gods and didn't take input from anyone.
Should I not do check it out locally and instead spend months and political capital arguing for more in the CI (which the PM would argue slows down velocity - and will win)?
Pragmatism and workarounds are often employed in non-bigtech enterprise companies...
edit: I also meant 'poke around' as in do a bit of informal QA. Again, your test and QA team might not be perfect...
The worst part is the false sense of security that you get that will bite your team the second you miss reviewing a change or go on vacation
Things that would be personal taste in a greenfield project become important for maintenance in a mature product.
That is why you should either make it a rule or let it be.
There is not. There are a variety of personal tastes and they all conflict with each other.
A small example adding an item to a list, it might be preference whether you use “push” or “append”
Don’t care which, but it is maddening to have a unit of code that mixes them.
There are all these choices that are numerous that should be consistent, but too numerous to add to a standards document ahead of time.
What is taken by one as taste may be for another a hard won lesson in neglect or negligence.
I feel that just giving out what to change without context does little to teach the hard won lessons, and is everything that would annoy people who are trying to get a change in.
It also depends where you draw the line. For example, is dependency injection a matter of taste or a fundamental design decision for a project? What about "defensive" coding practices, like `0 == x` over `x == 0` in C?
This is one of the biggest traps I’ve seen in code review. Generally, everyone is coming from a good place of “I’m reviewing this code to maintain codebase quality. This technically could cause problems. Thus I’m obligated to mention it”. Since the line of “could cause problems (important enough to mention)” is subjective, you can (and will, in my experience) get good natured pedants. They’ll block a 100LOC patch for weeks because “well if we name this variable x that COULD cause someone to think of it like y so we can’t name it x” or “this pattern you used has <insert textbook downsides that generally aren’t relevant for the problem>. I would do it with this other pattern (which has its own downsides but i wont say them)”.
The GMail outage of 2010, where Google had to restore 10% of GMail accounts from tape backup, was precisely because of this. A C++ migration script had used the "x = 0" construct, which set the variable to false and made the script go haywire. So not a theoretical concern. The other major takeaway from this was to always treat your migration scripts like production code and use the same warnings, tests, and defensive programming that you would use everywhere.
Some traits can only be seen clearly in the rear view window.
You don't get to have your own personal taste of the moment when you are reviewing; you have to broadly refer to the documented collection of tastes.
I wonder sometimes if these arguments in software are less of an issue of lack of care for craft, and more just a fundamental misunderstanding that the sorts of arguments and compromises one makes with a roommate over what state the apartment is allowed to be in are simply not analogs for a professional setting.
I get the very distinct feeling sometimes that my mis en place rules are being received as if I'm whining about whether coasters get put away when not in use, and how long old magazines get left on the couch.
Nearly all of the fastidiousness I can manage in a day I save for work, so I'm very familiar with the other side of this conversation outside of the office.
That said, it's also important the reviewer feels equally responsible for getting the work done and is willing to make pragmatic tradeoffs taking into account timeline pressure, how contained something is, how likely it is to actually cause trouble, how hard it is to change later. Cross-team and cross-org reviews are more likely to involve pure gatekeeping, which is death.
The reasons to block merging code should be explicitly and clearly stated in your Coding Standards documentation.
https://conventionalcomments.org/
While the goals that it is designed to get people to follow is something that I've preferred in my own commits in the past, this gives a structure to them that helps identify when there's too much in a commit for it to be reasonably reviewed in isolation.
"Don't leave too many comments" i think can really be rethought of, don't review style and syntax. Leave that to the robots. If you're relying on other engineers to flag style problems and linting, you're just wasting everybody's time. Set up linting and style checkers and be done with it.
You know you've progressed in their eyes every time they start bugging you about something new. You didn't suddenly become worse at something. Rather you got good enough on some higher priority thing that they knocked it off the list and replaced it with the next item in the backlog.
You should treat code reviews similarly. It's a journey, and we are in the middle. If you keep making the same comments on reviews, eventually they'll get addressed beforehand and you can point out something else.
I’ve always found the cognitive overhead of going through a pull request challenging, seems like the paradigm can shift now with all of the new tooling available to all of us.
Sure, and PR descriptions and comments are a way to do that (async).
But IME it helps to start with a good description of what the changes are rather than just having a pile of code changes dumped on you where you then have to reverse engineer the intent.
It seems draining to me to have to have to start from nothing and interrogate someone for every PR that comes in.
> You can also talk to people without scheduling a meeting
I didn't say anything about scheduling.
That's what I intended that to mean. You can talk to your colleagues even if you have PRs.
Most of the time, I imagine that people on a team know what each other is generally doing, and that everyone is broadly familiar with the codebase. So, if you really need a ground up walkthrough of a pull request, then that is a time to talk to your colleague, because they’ve either done something brilliant or something weird. That colleague has asked you to engage with their code by tagging you in the review. If you can’t ask a question like “Alice, what am I looking at here?”, then there’s a problem.
It also seems weird to me that you seem to need to set a meeting with someone to talk about a pull review. Do you not just regularly talk to people on your team about what you guys are doing? Like, you just randomly see pull request review notifications pop up and that’s your only interaction with your team? It’s not like we talk about every detail of everything, but if something came up in a pull request that I wasn’t sure about, it would just come up in conversation.
But that's what we're talking about. This is what you criticized:
automated walk through of a pull request, where it steps a reviewer through initially the overview and they why of the changes, then step by step through each change, with commentary along the way generated by the LLM (and hopefully reviewed by the pull requester for accuracy)
This is effectively what a PR description should do. It should explain what changes are being proposed and why. And having comments alongside the code changes only enhances that description IMO.
> a ground up walkthrough
Nobody said anything about a ground up walkthrough. It's walking through the changes, not the entire codebase.
> It also seems weird to me that you seem to need to set a meeting with someone to talk about a pull review
Again, I never said anything about scheduling a meeting. An ad-hoc discussion is a meeting too.
And sure, if I have a question after reading the PR then I'll ask the author. But it's certainly not the first thing I want to do.
If a dev has to take time even to review and edit an LLM generated version of this for every pull review (and it will require time to do this so that it doesn’t waste the reviewer’s time with wild goose chases due to faulty interpretation), and you are then going to have to wade through that doc in addition to reading the code, you could save everyone a lot of time and just talk to each other when you have questions.
I'm not looking at it like a line by line explanation of the code. Think of it like commit messages, but better. Better because:
1. commit messages are usually not very informative and the context is implicit.
2. commit messages are coupled to time rather than to the final PR changes. The PR changes are what really matters to the reviewer, not a log of what the author did to get there (especially if things are changing back and forth).
In terms of formatting etc I rely on tooling which break the CI. That way they don't even surface. Equally if something slips through the CI needs to be updated.
A piece of code should match the general style of the code it is being integrated into. When you create an addition to a wood-framed house the addition won't be steel construction, unless you have an explicit reason (weight, speed, foundation etc). And if I'm the main maintainer of a codebase, it is something I can call out on. If we use Tokio and you use raw threads I'll ask about this choice.
In an ideal world you (the PR author) includes this in its description, because it is a reflection of your understanding of the codebase.
Reviewing code with 'will this work' is also debatable. Someone might propose a change that fixes an issue they are having. But maybe it's not something we want to solve in our codebase, or maybe it breaks other things the maintainers are thinking off.
I also do something similar with lint rules. For Rust, this means turning on every single reasonable clippy lint (there are many), and making all warnings into errors. Some would say that some rules are overzealous or annoying, but it's a price I'm willing to pay to keep the codebase uniform.
If I’m trying to get the code to work don’t cockblock me on Make it Work with your Make it Right enforcements.
One of them uses tabs. Fucking Richard.
And then there’s the duplication and differences between the runscripts and the GitHub actions…
Like, they are reasonable ideas, but open up a new issue. If every reviewer wants to tackle large topics in the PR Review that have nothing to do with what specifically is happening, then it explodes and gets even harder for others to review now that we are changing things that have nothing to do with the change.
I had to tell one guy to knock it off because I was the only person brave enough to touch certain files and he was quickly making me regret trying by marking blocking comments on things I already planned to address in the subsequent or following PR. But I have to keep the old and new stuff working at the same time, boyo, so tap the brakes.
- you review and if to the best of your knowledge you think something can be done better you comment about it and leave a suggestion on how to do it better
- then you approve the PR. Because your job is not to gatekeep the code
Maybe this is because I’m working somewhere where we don’t use stacked reviews though? So it’s a major pain for someone to have a PR open for a long time going through lots of cycles of review, because it’s tricky to build more work on top of the in-review work
the places i work at expect trunk to always be clean, and ready for production (continuous delivery). if you work someplace with a slower release cycle, then getting a not perfect change in may be more acceptable.
there's also responsibility, which is traced during incidents back to author and reviewers. i won't approve until i'm confident in the code, and that will mean the author needs to answer questions etc.
To some people, "I take full responsibility," means, "I will publicly admit to being wrong," instead of, "I will do everything in my power to clean up this mess that is my creation."
I can see this working when the person who wrote the code is responsible for making sure the product works for the client and that their code does not interfere with everyone else's work.
If the person reviewing though is responsible for the above it makes sense to gatekeep the code. I have been in this position before and off loading as much as possible to automated processes helps, but has never been enough or at least there is never enough time to make those automated process that would be enough.
> Review with a “will this work” filter, not with a “is this exactly how I would have done it” filter
I suppose there are edge cases where you could say "technically this will work, but when the system is close to OOM it will fail silently", but I would consider that to be a negative response for "will this work" rather than a case where you rubber stamp it.
You adjust for the sort of code the rest of the team wants to see, to an extent. And you adjust for the sort of time it's reasonable to spend on a story. Even in open source I'm adjusting for that.
Occasionally, if I'm the SME on a particular section of code, it will eventually, by increments, end up being nearly exactly the sort of code I would write. And it's usually the sort of thing I do right before I leave a project. If I'm handing it over to someone else instead, I'm still going to be making it a bit more like what the new maintainer will want. If you give someone a project they suspect a trick if you don't sweeten the pot.
If the PR is broken, you clearly denote where is it broken. I usually start comments to lines requiring changes with a red circle (U+1f354), because GitHub code review UI lacks an explicit way to mark it. You explain what needs changing, crucially, why can't it remain as is, and ideally suggest ways to fix it. Then you demand changes to the PR.
Because yes, your job is to gatekeep the codebase, protecting it from code that will definitely cause trouble. Hopefully such cases are few, but they do occur, even to best engineers at best engineering orgs.
So look at the code and decide if you're willing to defend it if someone says, "Who approved this for production?" If you did your due diligence, thought the tests and the code were reasonable but some obscure interaction caused problems, you didn't have a way to know that.
If the code is just full of bad code smells and that's what blew up, then your defense is flimsy.
Production issues will happen. But they should always be the confluence of two or more errors resulting in a bad situation. Single cause failures are inexcusable.
Wow, that seems crazy. I can only hope I never have to work with somebody who thinks it is productive to leave that many comments on a change -- I genuinely cannot imagine any change that could ever require that.
Great article, fully agree with all the points.
If the PR went in a completely different direction and missed the goal by a lot, I take ownership of it (with a brief explanation) and re-implement it. I then use the new PR for a pairing session, where I walk through both PRs with the original author for learning purposes.
If it’s mostly smaller issues, I schedule a half-hour pairing session with the author and review everything together, after preparing a list of issues.
Doing it any other way puts too much burden on the author to guess what the reviewer wants, and it slows down velocity significantly.
I want to nitpick on
> Don’t review with a “how would I write it?” filter
_Thinking_ about "how would I write it?" when reviewing code catches a lot of stuff for me - I want to be able to come up with how I'd do it, see how the PR does it, and then check if the approach matches. If not, there's something to be learned and explained here. Although, all of that is in my head! If I come to the conclusion that the presented approach is wrong or inferior in an objective way (say, doesn't handle corner cases neatly or at all), I'll comment. Otherwise, let people do their thing!
So, I agree with the point of not using this to generate a lot of comments, but I do believe in it as a useful technique to engage the brain, instead of just loosely scrolling over the diff.
If you can't find it, you can't fix bugs. If you don't find it, then you end up splitting related functionality across several places in the code. Which will introduce bugs, and make them a pain in the ass to diagnose.
Clearly this is not true for everyone, but I don't think I've ever felt good about gatekeeping, and a number of people I respect generally don't either. Blocking is a weapon of last resort. That someone has had to use it means either that the system isn't working, or they're abusing the tool.
Someone shouldn't arrive at the final phase of their ticket and be told to start it over. That's a retrospective item, and if it's happened multiple times then this is an agenda item at the retro meeting.
This is exactly how I feel. The only times I would block a PR is if I know it will either:
a- actually break something existing
b- not deliver what the PR says it should deliver (eg new feature but the feature doesn't actually work)
It's basically saying "I'm certain that if we merge this, something bad will happen and we'll either cause an incident or need a hotfix".
I'll say this is directly proportionate to the size of the changes. If a code review is a conversation between n parties, it seems reasonable to me that more code leads to more comments.
I frankly don't even know they do it so consistently.
Something to keep in mind in your reviews as well I guess, lol.
If you are leaving a comment it’s because someone didn’t learn how things should be from the tools in your codebase.
Naming style problem? Linting isn’t catching it.
Architecture violation? Your import linting (checkers to prevent route handlers importing the DB and skipping the service layer, etc.) isn’t catching it.
Duplicate method? What tooling can you create in the LLM era - remembering you can ask agents to build scripts now?
Code reviews are also an opportunity for learning, and provide real-life scenarios to inform decisions that are otherwise not grounded and mostly opinion. Of course, not all comments and discussions should block approval, but there is a balance. No one is going to die if this feature is delayed by a few days if you get a better result _and_ are investing in team growth.
I would expand this and say if too many of the blocking statuses are coming from one reviewer, too much gatekeeping may be going on, even if the overall rate is low. Either they're going too far, or nobody else is going far enough. If you're shouting into the void, discretion is the better part of valor. You can't save people who don't want to be saved. Find people who will appreciate your efforts instead of resenting them.
It’s no small responsibility.
This hits home.
14 more comments available on Hacker News