There’s SO much to be done around Code Review, I’m very surprised GitHub isn’t doing more.
They have the reach already, they have years of real review data. And yet, the PR itself is not using any of the newly available methods for more efficient review.
Agents are hammering PRs out left and right, and yet here I am still staring at the raw diff of 35 PRs and end up being called the bottleneck.
[–] pksunkara 4 hours ago
1. Hunk checkpoints - If it's a moderately sized PR (which most of the agentic PRs nowadays are), I as a developer find myself like to think in terms of hunks whether I finished reviewing that part or not. GitHub allows you to mark only files as reviewed and nothing else.
2. I am trying to add semantic diffs to code reviews. That way a developer can actually understand the changes in the structure of the code instead of relying on reading plain text changes only. For ex: a function rename can simply compress 20 lines of changes into a single line.
3. Any agentic reviews and/or automated deterministic reviews can use both of the above to mark some simple things as reviewed in order to help the reviewer more.
4. This is in addition to existing review features from GitHub and the interdiff feature from other review tools.
A small mockup is available on the marketing website at https://juju.bi
I appreciate any comments on this approach. What do you guys think?
[–] gruez 4 hours ago
How long are your files that you can't easily "finish" a file?
[–] pksunkara 3 hours ago
[–] echelon 3 hours ago
(Semi serious here.)
[–] RyJones 3 hours ago
[–] Olscore 5 hours ago
Working on autonomous development with agents and no human code review. The future is human oversight, without humans in the loop. A PR centric cycle is like being in a factory and standing next to a conveyor belt of PRs. Humans can't possibly review properly. They have to take a few steps back and start looking at the entire machine, evaluating the system as a whole.
[–] skydhash 4 hours ago
Then something is either wrong with your conception of PRs or whatever process you have been submitted to. PR is asking for feedback from people that already understand the software. It's also a learning place for anyone that is not yet familiar with the part being modified. So you have the author, the reviewers, and the learners. Which is three separate sets of people.
The PR review itself shouldn't take a lot of time (10mn is already a lot), because it's either a small change which is easily understood[0] or it's a more complex one [1] that should have a design discussion thread attached. This discussion is orthogonal to the code itself. Both should have a description good enough for the reviewer to have an idea why the change is necessary before looking at the code.
So a PR is an exchange of ideas around a real artifact, the code. If no idea is being exchanged, then it's merely a gatekeeping activity.
[0] https://cgit.freebsd.org/src/commit/?id=73b1453e6d87f9af4c47...
[1] https://cgit.freebsd.org/src/commit/?id=49a3cbc47f8e721a787e...
[–] ivanjermakov 5 hours ago
[–] Buttons840 5 hours ago
At least for me. Am I missing something?
[–] jeffbaumes 1 hour ago
[–] Buttons840 1 hour ago
[–] NewJazz 2 hours ago
Use CI results to control quality, no nitpicking style means easier review
Review tests one by one, to make sure I understand what the submitter is trying to test and if they actually did so
Check files off one-by-one, compartmentalizing understanding as I go
Complain loudly when PRs try to change too many subsystems or different functionality, especially if less related changes are not separated into standalone commits
[–] jayknight 4 hours ago
[–] johannes1234321 4 hours ago
Also mind: one doesn't only have to review changes code, but also review what hasn't been changed (are preconditions of other code still met?)
[–] karakrakow 3 hours ago
[–] dirtbag__dad 5 hours ago
For me, what works is highly templated directory structure and rules that linters can quickly check and enforce. Every PR is a cookie cutter design filled in. There are only a handful of things you need and they’re all just CRUD at the end of the day. You can employ the same very few design patterns and primitives across a lot of your stack. Review is so fast, if you trust your CI to do its job and the developer to actually QA their own work.
What isn’t working is the sheer volume of PRs. Even with CODEOWNERS directing attention to only high impact changes, it is a constant distraction. I’m not sure how to handle SOC2 where we need a human reviewer on all code. Lastly, agents shit the bed when they touch spaghetti, esp when it’s sitting next to structured patterns. IME, the best way to get around this is to just rewrite features from scratch. But in this current world of instant completion and reward, it’s been challenging to convince people to commit to more ambitious lifts.
Finally, I want review more than ever. A team can’t keep up with the pace of change even if it was all perfect. I like the idea of more up front planning, but the code is ultimately what matters.
[–] NewJazz 2 hours ago
Idk why everyone says this. Yeah crud is tedious and common. No it is not the majority of my time spent, implementing, fixing, or reviewing.
[–] eddythompson80 2 hours ago
This all has changed. I’ve only seen gaslighting (in the form of “of course we expect you to read the code LLMs is generating”), lying along the same lines, or the realistic people who acknowledge they don’t read any of it. The former camp is mostly traditional engineers, and the later is mostly new or traditional non-engineering roles like recent product management coding converts. I think it would help everyone if we just normalize the fact that no one if reading code much anymore. It’s just not feasible. Write a bunch of creative reviewing bots or SKILLs and just leave it at that. It’s not a great answer, but like I said I haven’t seen any alternatives.
> if you trust your CI to do its job and the developer to actually QA their own work.
I trust the CI, but the CI is doing what the CI is written to do. I don’t know how you can achieve the second part in a large team/corporate setting or an open source setting.
[–] criley2 5 hours ago
Code review is great if the code isn't a surprise, the architecture was already agreed on, the AI reviewers are already happy, and you just need a final pass.
Code review was already terrible before AI if you were slopping down weeks of work on a poor teammate to sift through.
I tell the younger engineers at my company the same thing today I said 5 and 10 years ago. Make the review easy for another engineer to do it, and they'll do it. Make it hard on them, and you won't ship shit.
[–] gertlex 4 hours ago
But the "make review easy" part is one where I'm failing (a bit) to keep up, I feel.
Also, what does "a final pass" look like to you? E.g. if a PR is 3000 lines... I'd guess one approach is enforce smaller PRs. I've opened a few, looked at the line count, and closed them, to be redone/broken into stacked PRs.
If I'm reviewing a PR that I'm familiar with the plan/work on, my "a final pass" is often just "read and suggest touch-ups of the docs". But that's really just the first step of a "full review" too, so it skips other stuff, and I know that isn't a proper review in the back of my head, but on to the next thing! :|
(Because of several factors including joining a much less mature org git-wise this year, I'm full of internal questions/dilemmas around PRs that I'm working on feasible solves for)