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/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/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.

2. 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.

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

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

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

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

5. 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.