[donatj]

What I find to be maybe the single most important part of code review is knowledge transfer.

Our entire small team thumbs up a PR before it's merged unless there's a big rush on it, and this gives everyone on the team a rough idea of the state of the codebase at any given time. There's no being blindsided like "this whole system I depend on is gone" like I had happen at far more siloed places I've worked.

Beyond that, it gives a forum to ask questions about how things work to further build understanding. On a high functioning team, every developer should have at least a modest understanding of the entire system, including parts they never touch.

Another important feature is just the institutional knowledge check. For instance recently I made a small change to a table and a coworker pointed out that there was a microservice I wasn't considering that wrote to that table that would break (yes, sharing tables is bad design, unrelated). I had no idea this microservice existed let alone had access to this table. The institutional knowledge check here though prevented a larger issue and potential data cleanup situation.

[Aurornis]

> Our entire small team thumbs up a PR before it's merged unless there's a big rush on it

This has been tried by a couple of my past managers.

This feels great with a small team on a slow moving codebase. If you try to force it on a larger team or expect the codebase to move quickly then it turns into a performative game of skimming the code (if that) to click the thumbs up button so you can get back to your work.

The end game was a situation where nobody was really reviewing code because everyone had their own work to do and they didn’t want to be the one person blocking important PRs, so everyone was clicking thumbs up.

[cucumber3732842]

This with a side helping of "when everyone is responsible nobody is"

Then again, if you're dealing with an un-maintainable minefield of a code base then "fire the whole team, I dare you" might be what you're after.

[thomascgalvin]

> Our entire small team thumbs up a PR before it's merged unless there's a big rush on it, and this gives everyone on the team a rough idea of the state of the codebase at any given time. There's no being blindsided like "this whole system I depend on is gone" like I had happen at far more siloed places I've worked.

How large is your team? Because I don't think that would scale beyond maybe five engineers

I'm a huge proponent of automated testing, because that catches things like "this whole system I depend on is gone" even if the guy who depends on it isn't in the room

I'm also a huge proponent of shared ownership of ... everything, really. It's natural for people to kind of own different pieces of a codebase, especially if it's a component they created, but that leads to silos and low bus counts. There shouldn't be one guy who owns one system that depends on one other component

[6LLvveMx2koXfwn]

> For instance recently I made a small change to a table and a coworker pointed out that there was a microservice I wasn't considering that wrote to that table that would break

If code reviews are important, where does testing sit? Presumably if the coworker had not been part of the code review something would have stopped the breaking change making its way to prod?

[chii]

> something would have stopped the breaking change making its way to prod?

or a prod outage causes the knowledge to be experienced.

[marcosdumay]

I'd guess testing is done only for the software being deployed, not for that other microservice.

At least, that's what people do by default.

[skydhash]

> If code reviews are important, where does testing sit?

Testing is for general boundaries. If you have a formalized specs, investing in an harness and writing a lot of tests is worth it.

But more likely in a corporate system, fully documenting everything is a huge hurdle by itself. So your best bet is to gather everyone that is related to a change and let them evaluate its impact.

[nunez]

Before the PR is cut, before the PR is sent to review (so that time isn't wasted on low-valuw bugs and problems --- engineers are expensive!), pre-merge and pre-release.

[rowanseymour]

We even find ourselves creating PRs in situations where the code is going to be merged immediately anyway, and tagging other devs, just so they have a convenient way to see what got merged and why. So people don't lose track of what is in the codebase.

[jonahx]

It's a good practice. Worth mentioning also: the same can be done with ordinary git log, assuming everyone is using git well. A proper git log of yesterday's work can be like your work newspaper with coffee.

[laybak]

this seems like a chain of good practices. though I find it hard to stay disciplined about keeping commits well scoped and well described

[jonahx]

Becoming very comfortable with "rebase --interactive" and other cmds for editing your (local!) history before merging helps a lot. Once you are, it only adds 5m or so of extra work to most PRs. And while acquiring this knowledge used to be difficult, LLMs make it very easy these days.

[duskdozer]

I would also recommend an editor designed for rebases. I use the nodejs rebase-editor TUI (though it looks like the old non-vibecoded releases have been removed from github, so unclear on the current availability of this one) which makes it easier to organize

[morganherlocker]

You can setup most PR systems to squash on merge using the first commit's message, then enforce that the top commit message is prefixed with a ticket ID. This practice has many benefits: readable git log, easier git bisect for tracking down regressions, it becomes easy to find all commits associated with a block of work, more useful git blame.

[jonahx]

I strongly recommend against auto-squashing. It creates large commits without the semantic intent of the author. In a large PR, you want clean, small, semantic commits, which makes it much easier to review and understand.

[duskdozer]

I agree, though it feels like a bit of a lost cause at times to get people to make those commits

[fusslo]

this is a great way to train people to ignore PR emails/notifications

[rowanseymour]

Maybe but those people are also having claude summarize their inbox and get nice little daily reports on things they might want to look into.

[koolba]

> Our entire small team thumbs up a PR before it's merged unless there's a big rush on it, and this gives everyone on the team a rough idea of the state of the codebase at any given time.

For non trivial or chore updates a second pair of eyes is always a good idea. But it’s not possible to scale out “everybody reads everything” to a large N. The problem is that nobody could keep up with that ad the reader when there are some huge number of things to read. That’s why we delegate, create docs, and have overview sessions.

[abirch]

Complexity is caused by Dependencies and Obscurities ~ John Ousterhout

Looks like you had both.

[Scubabear68]

Based on your description, I don't think the process is working.

You describe a shared database, micro services you aren't aware of using the same database, and a desire for everyone to have a rough idea of the state of the codebase. And things breaking unexpectedly being caught by code review.

You have a bigger problem here of a system you can't reason about very well. The code review will help here, but I think you have bigger discoverability problems based on what I'm reading.

As others have said, what is needed is automated testing that would catch this sort of problem automatically without needing a human in the loop saying "Oh, wait...". We still want/need humans in the loop, but they should not be the only safety net.

[chrisra]

Decisions by small groups should be the default. Others only need to be involved if the risk/consequences of failure are high.

I started ignoring all PRs from our large team because we had a similar policy. My teammates can handle, they don't need me to check on each PR.

[stronglikedan]

> "this whole system I depend on is gone"

This would be caught upstream in my org. If something was removed in a PR which was not documented and discussed with the entire team ahead of time, that would not be good for the submitter.