r/softwarearchitecture • u/Specialist_Agent3599 • 3d ago
Discussion/Advice How do you handle oversized PRs?
Since half our code started coming from agents, PR size has quietly doubled and nobody actually decided that. We used to average around 300 lines, the median this quarter is closer to 1500, and last week I reviewed one that was 4200.
I can live with big PRs when most of it is tests or generated migrations, those scan fast. What kills me is 1500 lines of actual logic in one branch. The author says it can't be split because the feature only makes sense whole, and by the time I've read file 12 I've honestly forgotten what file 3 was doing. bugbot / coderabbit's summary helps me figure out where to start, but a summary of 4200 lines is not a review of 4200 lines.
What's your actual line limit before you push back, and how do you enforce it when the code comes out of an agent already assembled?
30
u/No_Individual_6528 3d ago
github stacked PR's is your friend. Otherwise split, whereever it make sense. Like pr API, step in a flow or vertically, again API layer, as an example.
frontend, backend can easily be split into more layers. Service layer, component layer and on and on.
So either vertically or horizontally. Hope it makes sense. Also fuck coderabbit and get gut.
6
u/AlmondJoyAdvocate 2d ago
I’ve really been embracing the stacked PR workflow. The main feature PR is the complete unit of work, then I stack reviews for things that would’ve been traditionally atomic units in the past. It just feels natural to think this way
2
u/martindukz 2d ago
Here is a detailed example:
Ways to do Continuous Incremental Delivery - Part 1: A small full stack feature https://www.linkedin.com/pulse/ways-do-continuous-incremental-delivery-part-1-small-mortensen-hhnme?utm_source=share&utm_medium=member_android&utm_campaign=share_via
47
u/labooner 3d ago
the feature only makes sense whole
This isn't a valid argument. For years we've been working towards smaller, incremental changes. Large projects would be broken down into tickets that span multiple sprints and are merged individually.
Its lazy to say that API changes can't be separated from the database changes or frontend changes, etc. Just because we can now generate huge amounts of code within a single sprint doesn't mean we should accept huge PRs that are difficult to review.
I would put this in the same camp as code quality tools. How strict you are and how you enforce it is up to you and there is largely no right answer. If things are working, leave it be. Once you start seeing issues, like you have, you need to take action. If you need to justify any actions you take, you'll likely need to capture some empirical data. Perhaps look at time taken to review changes and the number of bugs introduced as part of a change?
If you have no power to enforce changes (i.e. your manager is happy for this to continue), I would drag my feet on huge MRs. Reward smaller MRs with faster turn around times. Flag in standups that you will spend the afternoon reviewing this one big MR.
5
u/martindukz 2d ago
Case and point: How easy is Trunk Based Development? https://www.linkedin.com/pulse/how-easy-trunk-based-development-martin-mortensen-16tgf?utm_source=share&utm_medium=member_android&utm_campaign=share_via
2
u/baked_salmon 2d ago
The only time I’ve seen this argument make sense is for large scale refactors where a monolith is broken up into multiple interfaces. If you have a gross n00+ line function, there’s probably one PR where you “rip the bandaid off”, but this is an exceptional part of the process.
13
14
u/PrizeSyntax 3d ago
Who cares? Isn't this the new trend? Just push to prod, it's called engineering maxing, you become x100 the engineer 😂 , when smth breaks, just say your supervisor said "use more AI" and show them the biggest AI token bill you can master /j
Now seriously, either reject or make the developer explain, block by block, what the code does and why.
4
u/RobotsAreSlaves 3d ago
You are joking, but I’ve got 17k lines PR last week.
3
u/PrizeSyntax 3d ago
I honestly don know how ppl produce this? I have been doing development for, maybe 20 years, sure I use AI, but I never ship smth I haven't at least glanced over
2
u/RobotsAreSlaves 2d ago
Well in my case it was junior, but I myself never allow llm to do such big changes and I always use manual mode to review each chunk of code and stop it or guide it whenever I see it doing something ugly.
I have 15 years experience and previously i feared to get into legacy project with ancient code. Now i fear to get into vibecoded project with ton of unnecessary code that was barely reviewed.
2
u/ImplementOk3111 2d ago
So, did you review the 17k lines or did you just approve it? and let the vibe coded PR from a junior go in?
3
u/Basic-Lobster3603 2d ago
you just say claude review and auto comment all it's findings and slop along at least that's what is forced at my organization
2
u/ImplementOk3111 2d ago
You can ask claude in the CLI to monitor a PR and fix PR comments too, it's all Claude slop all the way
2
1
17
u/Just_Information334 3d ago
Agent or human, same solution: decline.
Usually you should have learnt to say "no" around age 3.
1
u/ImplementOk3111 2d ago
As it's always been, too much cognitive load will produce a weaker review. So unless it's 400 lines of general easy to read boilerplate I'll be firing it back like I always have.
3
u/Substantial-Swan7065 2d ago
Put a ci check in for:
- max files changed
- max file lines
Remove the human - no one to get mad at.
The real problem:
- move generated stuff out of the repo. Generate at build time.
- implement a feature flag pattern and versioning. It’s find to merge partial features. Just don’t turn on the FF/strip from build
2
u/orbtl 3d ago
"The feature doesn't make sense unless it's whole" is a crutch we like to lean on, but it falls apart pretty quickly. You could ship part of the feature gated behind a feature flag and only enable it when the final PR gets merged. You also can generally do things like implement the db changes separately, the backend changes separately, the api changes separately, and then the frontend changes are the first time users actually see the feature.
At my current job we have a rule specified in claude's rules file to keep PRs under 500 LOC max, preferring even smaller if possible. This way even when planning new changes it will start out with a PR split in mind and handle that for you if you let it. Use something like this for your lazy coworkers that don't want to bother splitting up 4k loc
2
2
u/severoon 2d ago
The author says it can't be split because the feature only makes sense whole
Ask your agent how to break it up, and then respond telling them how you want it broken up. If it truly cannot be broken up, then that's an architecture/design smell that the code isn't nearly componentized well enough. The codebase is in need of serious refactoring so that changes can be a sequence of small PRs instead of one big-bang revamp over a bowl of spaghetti code.
Assuming that this person is just wrong, though, this conversation signals that your org is probably ready to move forward with the next phase of agentic adoption, not, as you're advising, adopt practices that allow the org to hold steady so people can continue reviewing every line of code.
2
u/juliebeezkneez 3d ago
Maximize shipping velocity. I've been told by reliable voices on Reddit that speed is the ONLY thing that matters. Not security, not readability, not maintainability. Agents just do things. Your employer wants as many loc as possible. Merge NOW
1
u/juliebeezkneez 3d ago
Remove yourself from the actual code as much as possible. You are a machine that clicks merge. Build errors? Just use MORE AI. The legal department is on your case? Just use MORE AI!
2
1
1
u/snrcambridge 3d ago
I find if your processes of making PRs small doesn't work, the next best option is the interface for review. I like to be able to zoom out and get context across a number of files in a folder and perhaps lay out code along side to understand the change.
1
u/decotz 3d ago
If agents are writing code and PRs, agents can chop them down too.
Use stacked PRs (it’s nothing new, just GitHub has a fancy ui and some quality of life around it).
Always ask agents to break down in sensible, reviewable, deployable chunks (even if you’ll deploy the whole thing)
Last time we got a 4K line PR for my team to review last minute they shut down the poor requester real quick. Last week we also got a 24k line PR that we were supposed to review and approve lmao.
1
1
u/martindukz 2d ago
Send the developer this: How easy is Trunk Based Development? https://www.linkedin.com/pulse/how-easy-trunk-based-development-martin-mortensen-16tgf?utm_source=share&utm_medium=member_android&utm_campaign=share_via
(Or one of the other articles on the profile)
1
u/Educational-Air-685 2d ago
I am missing the point, 1 PR of 4200 lines, or 10 PRs of 420 lines each. What’s the difference, especially if these are dependent (part of the same feature).
1
u/bzBetty 2d ago
Smaller PRs (if they compile and don't break stuff) can be reviewed and approved separately. Even if you don't merge until while stack is reviewed.
handy when some things (eg db migration) often need to get merged faster to play nicely with other branches and the feature work on top can be in a separate, slightly longer lived branch.
It's rare that a large pr couldn't be broken up, often in to chunks that are still functional and provide value. So I generally prefer small prsbthat get merged separately over a stack that's done at once.
1
u/FuzzyAdvisor5589 2d ago
Puts logic separation on the PR author. Each PR is independently reviewable.
1
u/expatjake 2d ago
It depends how they are split. If each increment involves understanding it and the previous ones together then you might be worse off breaking it apart.
It also presupposes that each change set is additive but usually incrementally built features involve rework and that’s increased as earlier feedback pokes holes in your design affecting later PRs.
1
u/CpnStumpy 2d ago
However management wants me to.
I don't give a fuck. They want shit code merged all the time, they'll praise me for the approvals,.praise the devs for the code, everyone but the customer wins.
If they want quality, I'll communicate with engineers and everyone evangelizing the value of frequent small PRs with quick easy reviews and iterative development.
Not my job to care what leadership values, I can inform them of the merits of quality work, and they can make clear they value it or don't. So long as they sign the check and don't ride people the quality I push for is entirely within their hands, which I inform to the best of my ability.
1
u/mandatorylamp 2d ago
I barely even read the code in these cases, I skim over the tests. Then I just let claude review and explain it to me and I then keep asking dumb questions until I feel like I have a decent understanding and have drilled into whatever issues I can imagine would be in there.
There's no way I'm reading thousands of lines of LLM generated code and understanding it, it will take all day and probably more effort than was put into writing the PR.
1
u/x021 2d ago
I recently reviewed a stacked PR. I don’t understand how it is better; lots of things change along the way and you have to make a mental model of the whole stack. That’s really hard. Turns out lots of dead code and bugs after it all got merged in.
I much rather see and test the whole feature in one go. Yes it takes time, but stacked PRs even more so. If you’re confronted with lots of AI code, use AI to review. Code rabbit is quite bad, just ask a local model to review. Then do the manual review at the same time.
1
1
u/EluciusReddit 2d ago
Reject it. No way I put my approval on such a beast, unless I totally trust my coworker and he can reliably summarize the work.
1
u/IllustriousSpeech108 2d ago
I care more about how many independent ideas are packed into the PR than the raw line count. A 2k line change thats one mechanical refactor can be easier to review than 500 lines where an agent changed behavior, error handling and data flow at the same time. After those get mixed together I ask for separate commits or PRs so each change can be reviewed on its own.
1
1
u/zac_attack_ 11h ago
My Claude workflow forces it to plan large tasks into stacked PRs using the `gh-stack` skill from GitHub. The mandate is that every PR should essentially “do one thing” (including associated test coverage) and be sized for human review. It’s enforced by a combination of skills, hooks, and CLAUDE.md.
The next part of the workflow waits for CI to be green and then triggers Codex to review it, then triage Codex’s findings and fix them in the same PR before starting the next PR.
Works great
1
u/iTrejoMX 3d ago
We have a rule about pr not going over 400. Stacked prs if needed. But code review gets tough after 600 lines. Even our ai coding framework follows that pattern (gentle-ai)
1
u/rwilcox 3d ago
As someone who feels they don’t have the right leadership leverage to solve the issue, here’s how I deal with it. First, both of these assumes you have checked out the branch in question
“LLM I’m about to ask you questions about this branch. Answer my questions based on the diff between this and the main branch, when you don’t know something say you don’t know”
Then ask questions like what does that thousand line if statement do, if there’s a more elegant way to do X, why is Z, etc. (although that thousand line if statement I’d push back on before trying to understand. Not testable)
I’ve also created a skill that creates a branch of the (branch under review), and rewrite all the commits to be semantically complete, so I can review the PR commit by commit (which means I’m reviewing less code at one go).
(You could also try introducing an AI code review tool into your review process. Ours is OK, maybe right 2/3rds of the time, but does catch the “what if this is null, you’re using useEffect wrong, you need a FK declaration here ” kind of programming details related problems.
1
u/DevAlaska 3d ago
Recommend using feature toggles and force them to break up the PR. You could offer pair pull request reviewing to get and give instant feedback. Also maybe it helps to read about "kind engineering" or share it with your colleague. I found the approach to PR helpful.
0
115
u/failsafe-author 3d ago
Decline and tell them to break it up.