I use the GitHub Pull Request extension in VSCode to do the same thing (reviewing code locally in my editor). It works pretty well, and you can add/review comments directly in the editor.
Same! Its much nicer now especially since Github seems to be pretty arbitrary/rigid about when it hides files that have "too many changes". Its so much nicer to see/navigate around such changes quickly in VSCode vs trying to do the same in the web interface.
I suspect that since this is possible with VSCode/Github, its probably extensible to other providers editors.
I find the idea of using git for code reviews directly quite compelling. Working with the change locally as you were the one who made it is very convenient, considering the clunky read-only web UI.
I didn't get why stick with the requirement that review is a single commit? To keep git-review implementation simple?
I wonder if approach where every reviewer commits their comments/fixes to the PR branch directly would work as well as I think it would. One might not even need any additional tools to make it convenient to work with. This idea seems like a hybrid of traditional github flow and a way Linux development is organized via mailing lists and patches.
Agree with your pain points. One thing id add is GitHub makes you reapprove every PR after each push. As an OSS contributor it’s exhausting to chase re-approvals for minor tweaks.
> When I review code, I like to pull the source branch locally. Then I soft-reset the code to mere base, so that the code looks as if it was written by me.
This is eerily similar to how I review large changes that do not have a clear set of commits. The real problem is working with people that don’t realize that if you don’t break work down into small self contained units, everybody else is going to have to do it individually. Nobody can honestly say they can review tons of diffs to a ton of files and truly understand what they’ve reviewed.
Graphite seems cool, it’s just unfortunately quite expensive and sometimes hard to convince procurement/etc to invest in when it has a noticeable cost involved.
So I’m really hoping something like Graphite becomes open-source, or integrated into GitHub.
It's so cool that Git is considering first class change IDs!! That's huge! This sounds similar to what we had at Facebook to track revisions in Phabricator diffs. Curious if anyone knows the best place to read about this?
While I like the post and agree with everything the author talked about I find that this is not my problem. Despite having a similar workflow (classic vim user). The problem I have and I think a lot of others have too is that review just doesn't actually exist. LGTMs are not reviews, yet so common.
I'm not sure there's even a tech solution to this class of problems and it is down to culture. LGTMs exist because it satisfies the "letter of the law" but not the spirit. Classic bureaucracy problem combined with classic engineer problems. It feels like there are simple solutions but LGTMs are a hack. You try to solve this by requiring reviews but LGTMs are just a hack to that. Fundamentally you just can't measure the quality of a review[0]. Us techie types and bureaucrats have a similar failure mode: we like measurements. But a measurement of any kind is meaningless without context. Part of the problem is that businesses treat reviewing as a second class citizen. It's not "actual work" so shouldn't be given preference, which excuses the LGTM style reviews. Us engineers are used to looking at metrics without context and get lulled into a false sense of security, or convince ourselves that we can find a tech solution to this stuff. I'm sure someone's going to propose a LLM reviewer and hey, it might help, but it won't address the root problems. The only way to get good code reviews is for them to be done by someone capable of writing the code in the first place. Until the LLMs can do all the coding they won't make this problem go away, even if they can improve upon the LGTM bar. But that's barely a bar, it's sitting on the floor.
The problem is cultural. The problem is that code reviews are just as essential to the process as writing the code itself. You'll notice that companies that do good code review already do this. Then it is about making this easier to do! Reducing friction is something that should happen and we should work on, but you could make it all trivial and it wouldn't make code reviews better if they aren't treated as first class citizens.
So while I like the post and think the tech here is cool, you can't engineer your way out of a social problem. I'm not saying "don't solve engineering problems that exist in the same space" but I'm making the comment because I think it is easy to ignore the social problem by focusing on the engineering problem(s). I mean the engineering problems are magnitudes easier lol. But let's be real, avoiding addressing this, and similar, problems only adds debt. I don't know what the solution is[1], but I think we need to talk about it.
[0] Then there's the dual to LGTM! Code reviews exist and are detailed but petty and overly nitpicky. This is also hacky, but in a very different way. It is a misunderstanding of what review (or quality control) is. There's always room for criticism as nothing you do, ever, will be perfect. But finding problems is the easy part. The hard part is figuring out what problems are important and how to properly triage them. It doesn't take a genius to complain, but it does take an expert to critique. That's why the dual can even be more harmful as it slows progress needlessly and encourages the classic nerdy petty bickering over inconsequential nuances or over unknowns (as opposed to important nuances and known unknowns). If QC sees their jobs as finding problems and/or their bosses measure their performance based on how many problems they find then there's a steady state solution as the devs write code with the intentional errors that QC can pick up on, so they fulfill their metric of finding issues, and can also easily be fixed. This also matches the letter but not the spirit. This is why AI won't be able to step in without having the capacity of writing the code in the first place, which solves the entire problem by making it go away (even if agents are doing this ...
If you want to remain relevant in the AI-enabled software engineering future, you MUST get very good at reviewing code that you did not write.
AI can already write very good code. I have led teams of senior+ software engineers for many years. AI can write better code than most of them can at this point.
Educational establishments MUST prioritize teaching code review skills, and other high-level leadership skills.
Totally agree with this. Code review is quickly becoming the most important skill for engineers in the AI era. Tools can generate solid code, but judgment, context, and maintainability come from humans. That’s exactly why we built LiveReview(https://hexmos.com/livereview/) — to help teams get better at reviewing and learning from code they didn’t write.
Gitpatch attempts to solve this. Supports versioned patches and patch stacks (aka stacked PRs). Also handles force-pushes in stacks correctly even without Change-IDs using heuristics based on title, author date etc. It should also be unusually fast. Disclosure: I'm the author.
I'm not convinced that review comments as commits make thing easier, but I think storing them in git in some way is a good idea (i.e. git annotations or in commit messages after merge etc)
putting the review into git notes might have worked better. It's not attached to tje lines directly, but the commit and it can stay as part of the repo
Recently, I've been wondering about the point of code review as a whole.
When I started my career, no one did code review. I'm old.
At some point, my first company grew; we hired new people and started to offshore. Suddenly, you couldn't rely on developers having good judgement... or at least being responsible for fixing their own mess.
Code review was a tool I discovered and made mandatory.
A few years later, everyone converged on GitHub, PRs, and code review. What we were already doing now became the default.
Many, many years layer, I work with a 100% remote team that is mostly experienced and 75% or more of our work is writing code that looks like code we've already written. Most code review is low value. Yes, we do catch issues in review, especially with newer hires, but it's not obviously worth the delay of a review cycle.
Our current policy is to trust the author to opt-in for review. So far, this approach works, but I doubt it will scale.
My point? We have a lot of posts about code review and related tools and not enough about whether to review and how to make reviews useful.
My rule of thumb is that if you have an OnCall rotation for a codebase, you should require reviews. Besides all the benefits you've mentioned, its important to spread know-how of the code so that people on the rotation don't need to be pulled in e.g. over the weekends/on vacation because they're the only ones familiar with the code.
(There should be breakglass mechanisms to bypass code reviews, sure. Just the default should always be to require reviews)
I never did proper code review, other than when being lucky that we got a team of top devs in specific projects.
More often than not, it either doesn't exist, or turns out in a kind of architecture fetishism that the lead devs/architects have from conferences or space ship enterprise architecture.
Already without this garbage it feels so much better, than arguing about SOLID, clean code, hexagonal architecture, member functions being with an underscore, explicit types or not,...
me and my team have been doing code reviews purely within IntelliJ, for something like 6 years. We started doing it "by hand", by checking out the branch and comparing with master, then using Github for comments.
Now there's official support and tooling for reviews (at least in IDEA, but probably in the others too), where you also get in-line highlighting of changed lines, comments, status checks, etc...
I feel sorry for anyone still using GitHub itself (or GitLab or whatever). It's horrible for anything more than a few lines of changes here and there.
What bothered me for a long time with code reviews is that almost all useful things they catch (i.e. not nit-picking about subjective minor things that doesn't really matter) are much too late in the process. Not rarely the only (if any) useful outcome of a review is that everything has to be done from scratch in a different ways (completely new design) or that it is abandoned since it turns out it should never have been done at all.
It always seems as if the code review is the only time when all stakeholders really gets involved and starts thinking about a change. There may be some discussion earlier on in a jira ticket or meeting, and with some luck someone even wrote a design spec, but there will still often be someone from a different team or distant part of the organization that only hears about the change when they see the code review. This includes me. I often only notice that some other team implemented something stupid because I suddenly get a notification that someone posted a code review for some part of the code that I watch for changes.
Not that I know how to fix that. You can't have everyone in the entire company spend time looking at every possible thing that might be developed in the near future. Or can you? I don't know. That doesn't seem to ever happen anyway. At university in the 1990's in a course about development processes there wasn't only code reviews but also design reviews, and that isn't something I ever encountered in the wild (in any formal sense) but I don't know if even a design review process would be able to catch all the things you would want to catch BEFORE starting to implement something.
This can be used in any process where the result is only judged at the end.
The solution here may be to add a midterm check. I think this is what you mean by a "design review."
In my experience, there are some rules that need to be followed for it to work.
- Keep the number of stakeholders involved in all decisions, including PR, as small as possible.
- Everyone involved should take part in this check. That way, no one will be surprised by the results.
- This check should have been documented, like in the ticket.
This can be used in any process where the result is only judged at the end.
The solution here may be to add a midterm check. I think this is what you mean by a "design review."
In my experience, there are some rules that need to be followed for it to work.
We should keep the number of stakeholders involved in all decisions, including PR, as small as possible.
Everyone involved should take part in this mid-term check. That way, no one will be surprised by the results.
This check should have been documented, like in the ticket.
When and how to do this check and how to handle disagreements depend on the task, culture, and personalities.
With AI based coding (no, i won't use "Vibe coding", thank you) this workflow improves a lot.
Instead of jumping straight into code, I have my engineers write a Notion doc that describes what needs to be built. Think of it like an LLD, but really it’s a prompt for Claude-code.
This forces them to think through the problem at a low level, and they share the doc with me before sending it to Claude — so I get to review early in the process.
Once we finalize this "LLD" or "low-level-prompt", they hand it to Claude. The next time I see the work is in a GitHub PR. At that point, we rarely have to throw everything away and start from scratch.
Tangential, long ago I wanted to use a repo-backed (IIRC tied to Mercurial) backend for issues. They were also flat files. We put too many plugins into Redmine and it died frequently.
59 comments
[ 156 ms ] story [ 400 ms ] threadI suspect that since this is possible with VSCode/Github, its probably extensible to other providers editors.
I didn't get why stick with the requirement that review is a single commit? To keep git-review implementation simple?
I wonder if approach where every reviewer commits their comments/fixes to the PR branch directly would work as well as I think it would. One might not even need any additional tools to make it convenient to work with. This idea seems like a hybrid of traditional github flow and a way Linux development is organized via mailing lists and patches.
I was on a lookout for best "precommit" review tool and zeroed on Magit, gitui, Sublime Merge.
I am not an emac user, so i'll have to learn this.
This is eerily similar to how I review large changes that do not have a clear set of commits. The real problem is working with people that don’t realize that if you don’t break work down into small self contained units, everybody else is going to have to do it individually. Nobody can honestly say they can review tons of diffs to a ton of files and truly understand what they’ve reviewed.
The whole is more than just the sum of the parts.
When we started graphite.dev years ago that was a workflow most developers had never heard of unless they had previously been at FB / Google.
Fun to see how fast code review can change over 3-4yrs :)
So I’m really hoping something like Graphite becomes open-source, or integrated into GitHub.
https://youtu.be/Qscq3l0g0B8
This is a pretty cool tool for it: https://github.com/sindrets/diffview.nvim
On the branch that you are reviewing, you can do something like this:
:DiffviewOpen origin/HEAD...HEAD
I'm not sure there's even a tech solution to this class of problems and it is down to culture. LGTMs exist because it satisfies the "letter of the law" but not the spirit. Classic bureaucracy problem combined with classic engineer problems. It feels like there are simple solutions but LGTMs are a hack. You try to solve this by requiring reviews but LGTMs are just a hack to that. Fundamentally you just can't measure the quality of a review[0]. Us techie types and bureaucrats have a similar failure mode: we like measurements. But a measurement of any kind is meaningless without context. Part of the problem is that businesses treat reviewing as a second class citizen. It's not "actual work" so shouldn't be given preference, which excuses the LGTM style reviews. Us engineers are used to looking at metrics without context and get lulled into a false sense of security, or convince ourselves that we can find a tech solution to this stuff. I'm sure someone's going to propose a LLM reviewer and hey, it might help, but it won't address the root problems. The only way to get good code reviews is for them to be done by someone capable of writing the code in the first place. Until the LLMs can do all the coding they won't make this problem go away, even if they can improve upon the LGTM bar. But that's barely a bar, it's sitting on the floor.
The problem is cultural. The problem is that code reviews are just as essential to the process as writing the code itself. You'll notice that companies that do good code review already do this. Then it is about making this easier to do! Reducing friction is something that should happen and we should work on, but you could make it all trivial and it wouldn't make code reviews better if they aren't treated as first class citizens.
So while I like the post and think the tech here is cool, you can't engineer your way out of a social problem. I'm not saying "don't solve engineering problems that exist in the same space" but I'm making the comment because I think it is easy to ignore the social problem by focusing on the engineering problem(s). I mean the engineering problems are magnitudes easier lol. But let's be real, avoiding addressing this, and similar, problems only adds debt. I don't know what the solution is[1], but I think we need to talk about it.
[0] Then there's the dual to LGTM! Code reviews exist and are detailed but petty and overly nitpicky. This is also hacky, but in a very different way. It is a misunderstanding of what review (or quality control) is. There's always room for criticism as nothing you do, ever, will be perfect. But finding problems is the easy part. The hard part is figuring out what problems are important and how to properly triage them. It doesn't take a genius to complain, but it does take an expert to critique. That's why the dual can even be more harmful as it slows progress needlessly and encourages the classic nerdy petty bickering over inconsequential nuances or over unknowns (as opposed to important nuances and known unknowns). If QC sees their jobs as finding problems and/or their bosses measure their performance based on how many problems they find then there's a steady state solution as the devs write code with the intentional errors that QC can pick up on, so they fulfill their metric of finding issues, and can also easily be fixed. This also matches the letter but not the spirit. This is why AI won't be able to step in without having the capacity of writing the code in the first place, which solves the entire problem by making it go away (even if agents are doing this ...
AI can already write very good code. I have led teams of senior+ software engineers for many years. AI can write better code than most of them can at this point.
Educational establishments MUST prioritize teaching code review skills, and other high-level leadership skills.
Shockingly, the best code review tool I've ever used was Azure DevOps.
I'm not convinced that review comments as commits make thing easier, but I think storing them in git in some way is a good idea (i.e. git annotations or in commit messages after merge etc)
When I started my career, no one did code review. I'm old.
At some point, my first company grew; we hired new people and started to offshore. Suddenly, you couldn't rely on developers having good judgement... or at least being responsible for fixing their own mess.
Code review was a tool I discovered and made mandatory.
A few years later, everyone converged on GitHub, PRs, and code review. What we were already doing now became the default.
Many, many years layer, I work with a 100% remote team that is mostly experienced and 75% or more of our work is writing code that looks like code we've already written. Most code review is low value. Yes, we do catch issues in review, especially with newer hires, but it's not obviously worth the delay of a review cycle.
Our current policy is to trust the author to opt-in for review. So far, this approach works, but I doubt it will scale.
My point? We have a lot of posts about code review and related tools and not enough about whether to review and how to make reviews useful.
(There should be breakglass mechanisms to bypass code reviews, sure. Just the default should always be to require reviews)
GitLab enables this - make the suggestion in-line which the original dev can either accept or decline.
More often than not, it either doesn't exist, or turns out in a kind of architecture fetishism that the lead devs/architects have from conferences or space ship enterprise architecture.
Already without this garbage it feels so much better, than arguing about SOLID, clean code, hexagonal architecture, member functions being with an underscore, explicit types or not,...
Now there's official support and tooling for reviews (at least in IDEA, but probably in the others too), where you also get in-line highlighting of changed lines, comments, status checks, etc...
I feel sorry for anyone still using GitHub itself (or GitLab or whatever). It's horrible for anything more than a few lines of changes here and there.
It always seems as if the code review is the only time when all stakeholders really gets involved and starts thinking about a change. There may be some discussion earlier on in a jira ticket or meeting, and with some luck someone even wrote a design spec, but there will still often be someone from a different team or distant part of the organization that only hears about the change when they see the code review. This includes me. I often only notice that some other team implemented something stupid because I suddenly get a notification that someone posted a code review for some part of the code that I watch for changes.
Not that I know how to fix that. You can't have everyone in the entire company spend time looking at every possible thing that might be developed in the near future. Or can you? I don't know. That doesn't seem to ever happen anyway. At university in the 1990's in a course about development processes there wasn't only code reviews but also design reviews, and that isn't something I ever encountered in the wild (in any formal sense) but I don't know if even a design review process would be able to catch all the things you would want to catch BEFORE starting to implement something.
The solution here may be to add a midterm check. I think this is what you mean by a "design review."
In my experience, there are some rules that need to be followed for it to work.
- Keep the number of stakeholders involved in all decisions, including PR, as small as possible.
- Everyone involved should take part in this check. That way, no one will be surprised by the results.
- This check should have been documented, like in the ticket.
This can be used in any process where the result is only judged at the end. The solution here may be to add a midterm check. I think this is what you mean by a "design review." In my experience, there are some rules that need to be followed for it to work. We should keep the number of stakeholders involved in all decisions, including PR, as small as possible. Everyone involved should take part in this mid-term check. That way, no one will be surprised by the results. This check should have been documented, like in the ticket.
When and how to do this check and how to handle disagreements depend on the task, culture, and personalities.
The patchsets get stacked up and you know where you left off if there are different changes and that is very cool.