It’s hard to write maintainable code without others’ opinions. Code reviews are an easy solution: they’re async, perfect for the busy manager; enforceable and trackable in your code platform; easy to refer back to later.
Unfortunately, it’s easy to give a terrible code review: how many times have you got stuck in the bikeshed iterating on things that were already better than what was there before? On the flipside, how many times have you got a plain “LGTM” when you knew you could do better next time if only you had feedback?
How can we make our code reviews focus on catching bugs before they happen, while still providing pointers for growth, without just being linter 2.0?
If you don’t mind sounding like an assassin droid with a complex past, then I have a solution for you… or at least a set of idiosyncrasies developed over the last few years which have worked well for me and my team(s).
tl;dr: In this scheme, any non-urgent comment in a code review gets a prefix. If there’s no prefix, some response (code or a reply) is needed. If all comments have a prefix, approve the pull request. By leaving nitpick comments but not blocking on them, we accept a slower first review, but kick off a virtuous cycle that should lead to overall better code and faster reviews in the future.
Conventional Code Reviews for Code Authors
To understand the scheme, it’s helpful to imagine you are looking at some comments on a code review you have recently received. For some readers, this may be extremely vivid, as if they have such a review in front of them right now.
The prefixes I commonly use, and their meanings, are as follows:
- No prefix: There is a significant issue which will either prevent the application working now, or mislead developers in the future. I expect a response to this comment before approving. These are therefore blocking comments.
- How to respond: generally either change the code, or respond with explanation. Discussion often helps: I am fallible too, and it’s often a great learning opportunity for me to have my own bad assumptions corrected!
- Nitpick: I think this could be improved, but it’s not going to stop the application working properly. This can be anything from a one character typo up to a refactor of method structure, or improving handling of an edge case.
- There are various other prefixes I use which are functionally equivalent, like “typo”.
- How to respond: as you see fit, with a pinch of salt. Helpful change and you have time? Feel free to make it, even if not blocking. Not useful in your opinion? “No” is a complete sentence.
- Follow-up: I think there is a potential improvement here, but not worth doing now. Typically this is larger refactors, or handling scenarios we’re expecting to encounter at some point in the future.
- How to respond: if you agree it’s worthwhile, leave a comment in the code (if there were blocking comments), and file an issue to fix it (if it’s a big thing), or note it down on a related issue which we expect to touch the same code so we don’t forget (if it’s a small thing). It might even make sense to make the change now if it’s unexpectedly easy, but note that I don’t expect that.
- Aside: I expect no action as a result of this comment, and you can just resolve the thread. For example, I use asides for positive feedback – when the author has done something especially elegant – and for informational content when it doesn’t affect normal operation (for example, if we narrowly avoided making unhelpful assumptions about names, email addresses or IPv4 addresses; or if we added a log line that looks like we might benefit from contextual logging).
- How to respond: Resolve the thread, and be curious why your reviewer thought you’d appreciate the comment.
- Note to reviewer: I typically use this when I do a first-pass self-review of my code, leaving notes for changes which might look non-obvious on the diff but will flow fine once the code has been merged (for example explaining why a change was made if the new option is less elegant). In general these will duplicate comments made in commit descriptions, but it’s unreasonable to expect a reviewer to read those1.
- How to respond: Generally best to leave the thread open until review is completed, then resolve immediately pre-merge.
On blocking comments
No-prefix comments can be tricky. They can indicate clear action:
We’re frobinating2 the
foo, but I think we should instead frobinate thebar. The type system could protect us here.
But if you think it is indeed the foo that should be frobinated, then responding to say so is perfectly acceptable.
Some no-prefix comments are questions, where typically the right thing to do is discuss first, even if you do also make code changes. For example:
Why is this code necessary? It seems to be adding a lot of complication compared to [some concrete alternative].
I’d typically expect you to give some explanation why you thought it was necessary, rather than just removing the code.
I expect some later iteration of this post will include a public, open-source example of a code review following this scheme. Alas, I don’t have one to hand while writing this.
Why this scheme works
Triaging comments this way, assuming you’d leave the comments anyway, has two big wins for both the reviewer and the reviewee:
- Clear expectations avoid iterating on things that waste time.
- As a reviewee, you know exactly which comments are the ones that blocked the merge, so you can focus your time on those.
- As a reviewer, there’s no second guessing whether, on balance, you reject an MR: it’s a simple “did I leave any comments with no prefix? No? Approve. Yes? Do not approve.”
- Skimmability on future passes, for whether there are any important things left unaddressed.
- This is especially helpful for that final-pass-before-merging where all threads must be resolved, if you’re using that functionality in your Git tooling.
The other common path is not to mention the nitpicks at all. While I realise these comments do take time, I believe they still ultimately have a benefit in that the reviewee gets to learn from them, in real-time, when the code is fresh. The codebase can deal with a handful of places where a mistake has been made, but each nitpick is an opportunity not to make the same mistake next time. If you’re never told there’s an issue since the result is “good enough”, how do we improve in the long term?
That is of course to set aside questions of style: the bikeshed is a very real place, and we don’t want to end up there if we don’t need to. Even a nitpick has a non-zero cost, so you should (ideally) even check those over before submitting your review to make sure you’re not wasting the reviewee’s time.
It’s also the case that, under time pressure or when we know changes are temporary, it can sometimes be the right thing to let standards slip… but at least we should consider the repayment plan for our tech debt at review time.
Where did this come from?
I had the privilege to spend several years working with extremely smart junior developers who had a great grasp for the problems being solved, but hadn’t yet encountered edge cases or common patterns (e.g. the Strangler Fig pattern as one I explained multiple times in 2025-10). I found that I could integrate a lot of these lessons into reviews I’d be doing anyway to provide mentorship at relevant times.
This specific regime started after feedback from colleagues ending up in long cycles of stale PRs under the sheer volume of comments with questionable ability to be actioned, but without wanting me to stop leaving these comments. I was looking for a solution which would allow us to clearly stop blocking, while still allowing me to comment on things worth working on.
My particular use evolved naturally over time to settle on this scheme; I’m sure it isn’t fully original. From a quick search while writing this post, although I wasn’t previously aware of it, what I do looks a lot like Conventional Comments without decorators, thus the default assumption is that a comment is non-blocking. My overall style is heavily influenced by Graham Edgecombe’s, although his comments typically had vaguer indications rather than a set rule for prefixes. It’s found success with others too: Archey Barrell has his own code review guide downstream of mine, with which this is compatible.
When I’ve introduced this to colleagues with no prior context, it’s received comparison to Conventional Commits.3 It comes from a similar place of adding some structure – specifically prefixes – to make it easy to skim out what’s important and what’s not.
Closing words
This isn’t quite a free lunch – leaving all those nitpicks definitely does take time – but my base instinct would be to note them down and talk about them later anyway. I’d much rather have that discussion inline on the code while it’s fresh rather than in a 1:1 weeks later: most nitpicks aren’t a big deal, after all.
If you’re a reviewee, I hope you’ve come out of this with a greater understanding of why I do the weird things I do. If you’re a reviewer, I hope you can try this scheme out on those organic meatbags— I mean, give it a go with your colleagues if you’d like to see how it works for you.
I’m sure there are plenty of other good schemes out there – likely even ones which are mutually incompatible with this – but this is something that’s worked for me. In particular, I can imagine people who often work with more senior engineers don’t need a scheme tailored for effective mentorship.
I’d love to hear your experiences with this scheme, or others similar or different, at code-reviews-post@wjbarn.es, which I’ll read and try my best to integrate into this post as FAQs/other suggestions.
-
With apologies to everyone whose commit messages I have nitpicked. ↩︎
-
I misremembered the word frobnicate which I think I first came across in the documentation for the Exporter Perl module. I’ve left this typo as-is as I’ve been using it incorrectly for a long time now. ↩︎
-
Perhaps surprisingly, I don’t often use Conventional Commits myself. It doesn’t fit particularly well with the kind of ultra-granular, self-contained commits I often make. It’s hard to define any particular commit as introducing a feature when deliberately committing (functional) intermediate states to make it as easy as possible to revert and pinpoint where a bad assumption crept in (or to act as a cautionary tale for why we didn’t implement it that way in the first place!). The specification seems to work best with squashed commits, as the documentation itself encourages, but I’m sure there’s a blend that could work… and perhaps it’s worth the tradeoff in the general case. In particular, making semver determination as simple as “what is the largest type of change in this merge request/release cycle” is a great strength. ↩︎