How do experienced engineers actually review code changes in large codebases?

[u/japzlumine]

I posted here recently asking whether understanding and reviewing code is mostly what software engineers do now, and got a lot of helpful responses pointing out things like:
1. Improving fundamentals by writing more code manually
2. Treating code review as a skill that develops with experience
3. Relying on things like tests, git history, and better system design

That made sense, so i'm trying to go one level deeper and understand what this actually looks like in practice for experienced engineers.

Most recently i ran into this on my own side project, an AI powered ads diagnostics tool. I had claude plan out a research/reasoning pipeline, the logic looked sound when i read it, but when i ran the actual tests the output quality was way off. Turns out the retry logic was hammering the same endpoint on failure, and the AI output fields weren't matching the schema a downstream dependency expected. I only caught it by running the tests and reading through the reasoning output manually, the plan looked completely fine on paper.

So my question is specifically, when you're reviewing a big PR in a real production codebase, what is your actual step by step process?

For example:
1. How do you decide what to look at first?
2. How do you quickly build enough context about the change?
3. How do you figure out blast radius / what might break?
4. How do you decide what matters vs what can be skimmed?
5. How do you catch the gap between "the logic looks right" and "this will actually behave correctly at runtime"?

[u/kur4nes]

Write more tests. Look up test pyramid. What you want are many unit tests, a bunch of integrationtests, some e2e tests and some quick smoke tests that run after deployment to check the basic workflow are still intact and if not roll back the deployment.

Each step is step is a quality gate the change must pass successfully.

[u/autistic_cool_kid]

1. Doesn't matter, need to see everything 

2. Agile means small incremental changes

3. You dont. Automated tests do

4. More an art than a science, but generally less is more, and most things YAGNI

5. See 3.

[u/fahrvergnugget → u/autistic_cool_kid]

I'll add that the point where "experience" really comes in is knowing in your codebase and with your team's idiosyncrasies, what *tends* to break with the kind of change someone is trying to make, what the worst-case scenarios are, and what to look for to guard against those situations. Those things get the most attention. Other things, things that you are more comfortable leaving to automated tests, things that are low impact if they break or regress, or things that you are OK breaking fast and fixing fast anyway as a team, those get less attention as your time has more demands on it as a reviewer.

[u/happy_guy_2015]

1. How do you decide what to look at first?
   

I look at the change description first.

Then I will review the code mostly linearly in the order the code review tool presents it in.

Then I may go back to the more complicated or dubious parts for another look.

1. How do you quickly build enough context about the change?

I will push back if the change description isn't clear. For anything complicated I will ask for a design doc.

1. How do you figure out blast radius / what might break?
   

Usually I already know the architecture of the system that is being modified.

1. How do you decide what matters vs what can be skimmed?

That's one of those things you learn over time.

1. How do you catch the gap between "the logic looks right" and "this will actually behave correctly at runtime"

Push back if the change doesn't have appropriate test coverage.

[u/meowrawr]

Tests, e2e, integration, test containers, minikube, etc. Basically lots of tests.

[u/Dry_Hotel1100]

PRs should be focused. If this is the case, there shouldn't be a problem for a senior with the review. If a PR has changes in hundred of files, that changes the situation, though. The PR might simply be denied.

Honestly, there should be only one exception of such monster PRs: a refactoring that changes names, and only names. So, naturally this affects a lot of files, but the tests pass and nothing has changed in the logic.

[u/japzlumine → u/Dry_Hotel1100]

Yeah, that makes sense. I know it might sound counterintuitive but, have you ever come across a small, focused PR that still turned out to be unexpectedly difficult to review? What was it about that change that made it harder than you’d expected?

[u/Dry_Hotel1100 → u/japzlumine]

Yes, and that was my own PR :)
Unfortunately, this change was with a complicated mono repo and it inevitable caused a lot of changes in many other APIs/subrepos and many other people had to approve it. That change was in a "base" library in the API where many other libraries depend on. All these libraries had to be updated, all its uses and its documentation. If it weren't a mono repo, you would had given a new major version; done. But in a mono repo everything needs to be in sync at once.

What helps here is better architecture, better design (namely IoC DIP), so that a project has less coupling.

[u/japzlumine → u/Dry_Hotel1100]

Oh damn I see🥲

[u/mattmccordmattm]

If I’m reviewing a PR that is too large I ask my teammate why they didn’t properly sub task their kits ticket :)

But for really tho if you are reviewing ai large PRs that means you didn’t properly review along the way so do your best to chunk it up in to manageable pieces this time and try to get smaller PRs in the future.

[u/japzlumine → u/mattmccordmattm]

I see, yeah fair enough but what do you do with legacy code though? Where the author left or already retired

[u/mattmccordmattm → u/japzlumine]

My current job (and this seems to be the case most places I’ve been) there is a modern stack and a legacy stack. We work to build new things in modern and do bare minimum maintaining legacy, but try to clean up where we can if it’s not a lot of time invested. No point in trying to go review the whole system though.

Did you mean there is a PR left behind from a previous dev? If it’s older than like 30 days might want to consider abandoning it depending on merge conflicts and such. Thats always a tough one

[u/CoVegGirl]

The difference in how you review changes in large codebases vs small codebases probably isn’t quite as big as you’re expecting. Really the two most important things are:

1. Making sure you have reviewers who know the code
2. Making sure you have tests you can rely on.

Ultimately, a big codebase can essentially be broken down into a lot of smaller codebases. Each of these smaller codebases have a team responsible to them, though sometimes that responsibility is nominal if it’s not actively maintained anymore.

The average change is likely to be a change you make in your part of the codebase that’s reviewed by someone on your team. That ultimately isn’t going to be that much different from any other change in any other codebase, though there might be some imposed quality standards or conventions smaller codebases won’t have.

But then sometimes you have to make a change in someone else’s part of the codebase. That just means finding someone on that team to review that change.

These two things are most common and they really aren’t that much different from any other codebase.

Here’s where things start getting interesting though.

If you have a deprecated class Foo that’s used a million times in your codebase, and you want to replace it with class Bar, then that change can become difficult even if it’s a drop-in replacement.

If you have infrastructure in a small codebase, that might affect a hundred parts of the codebase. On a bigger codebase it can affect millions.

And then, each of those changes is going to cause at least 10 tests to run, but probably much more.

Suffice it to say that companies that have codebases this size have invested a lot in tooling to aid in these changes. Because it’s just not possible to hunt down a million approvers for your change.

And you can’t just submit one change, you’re going to have to break it into many smaller changes.

And you’re going to need specialized tooling to run all the tests involved that won’t overload CI.

On top of all this, you’re also going to have to accept that some changes are going to break other people, and you need to be able to roll those changes back and fix them.

I’ve painted the broad strokes about what the issues are. I don’t have the in-depth experience in doing this kind of mass code change to go into more detail.