> Only for a drive-by take-down by someone with none of the context

If that's a regular issue that's a culture problem. Starting with "if you've been talking with colleagues about your problem, why is someone with no context reviewing the result?", people not investing time in reviews, people doing "take-downs" instead of asking questions if they don't understand things, hold up merge for non-urgent concerns ...

> Even helpful review comments might only take a minute to write but a day to incorporate.

Either the feedback is worth the investment of the day of time, then no problem, otherwise it's not that important and doesn't need to be done (or depending on what it is can be done later or ...)

> If that's a regular issue that's a culture problem.

Couldn’t one also argue that if code reviews are routinely catching problems mentioned earlier in the thread (misinterpreted requirements, conflicts with other WIP, etc.) then that’s a cultural problem? It just seems odd to assume that the person assigned the task couldn’t possibly be expected to routinely avoid those problems, but that adding rigorous code review could routinely avoid them.

Yup, definitely. Code review discovers both issues stemming from cultural problems and silly bugs and understandability issues. Peer review in general is a very powerful tool for things that should be designed to satisfy customer needs.

> otherwise it's not that important and doesn't need to be done

Except that this decision is often not up to the code author, because the tooling rejects for example committing changes until all discussions are resolved (see Gitlab for example) and some @holes make their quest for any reason not to resolve the discussions that they started until the code author writes the exact code with the exact words in the exact indentation with the exact architecture, etc that those @holes like, which makes the whole code review like a torture and the REAL productivity killer. Not to mention that this does not provide any improvement to the code base either in most cases.

> drive-by take-down by someone with none of the context.

If this is an issue in your workday, you should bring it up with management or the individual rejecter. If someone has the authority to kill PRs, they should also be required to put in the effort to understand it and/or be available for discussion at an earlier stage of the development process. PRs shouldn't ever be rejected without mutual agreement - neither internally in an organization, or in an open source project.

Is this a common practice? I've never worked under a code review process where the accept / reject decision was being made by "drive-by" reviews from "someone with none of the context". It has always been team members with a lot of context reviewing each others' code.

I've never even worked with a code review process where outright rejecting PRs is a normal part of the process for anything other than external contributions. The normal process is more of that if you think a PR is a bad idea it's up to you to persuade the author of that. You'll certainly sometimes have the author withdraw the PR without being truly convinced, but something like "I don't like this. Rejected." would just be disregarded if you can get other people to approve it.

I think the issue is not that people make drive-by issues, the problem is that people argue about things endlessly which does not bring any value neither to the customers nor the company, that is only for the vanity of the reviewer, and many reviewer get offended when someone does not take blindly their "wisdom" and for retaliation they block code commits endlessly which is enforced by automated tools like Gitlab.

> Only for a drive-by take-down by someone with none of the context.

Then write it down. If the code reviewer can't follow what's going on, what hope is there for the new hire looking at it six months from now?

Because looking through 6 month old prs are what new hires are doing to learn the codebase?

Running through `git blame` and looking at the commit message, the PR, and the PR comments is a very practical way to learn about a codebase! Beyond some high level code organization stuff that exists in a readme, learning a codebase is really about getting a history lesson of how the product evolved, the company pivoted, and the team re-org'd.

It'll be 6-months if you're lucky. Try figuring out why there's a particular "if" clause, one or two or four years later. Software maintenance, especially when the original author has moved onto another team/organization/company/career, is a frustrating art of almost remembered stories and hoping that you can figure out Chesterton's fence, lest it become Chesterton's barbed wire. The worst is when you fix a small bug with code and introduce an even bigger big in the process.

Not necessarily looking through PRs, but presumably they need to work with actual code in your code base that was merged at some point, unless your product is growing so fast that new people just write new code all the time? (Same applies for not-new coworkers that now need to touch that code area anyways)

Sure. If they are good and have a decent grasp of using version control then will look at the history of a certain piece of code if they don't understand why it is the way it is. I do it often, even when I'm no longer a new hire.

That is one of the things I do to learn new codebases.

> Only for a drive-by take-down

Code reviews aren't a "take-down", they're a process of helping each other produce better solutions and better code.

> Even helpful review comments might only take a minute to write but a day to incorporate.

Then like all pieces of work, a decision must be taken whether that day is worth it, no?

That's what YOU think. But in practice they produce sub-par solutions and worse quality in my experience. Code reviews are basically a virtual piss-contest where people argue endlessly, everyone trying to show-off their intellectual superiority. And reviewers (not code authors) often can't handle rejection for some reason.

It sounds like you've worked in some massively toxic teams. That's all I can say.

Take this with a grain of salt, but a good code review should be an exploratory process. Most of the non-trivial comments should be questions[0], and the reviewer should be operating under the assumption they'll have to jump on a call or even fix it themselves.

[0] Not just because the reviewer is trying to be nice, but because they don't know what constraints and problems the author discovered in the process of writing the PR. If the reviewer writes "use a map here, instead of scanning an array" without knowing how many items are handled, then they may be making the PR worse by replacing a cheap linear scan over a maximum of 50 items with a bunch of costly yet constant time hashing. Often the solution is an explanatory comment rather than the "obvious" fix.

Fair, and I've seen this, but usually it's because the reviewers are treating "I would personally prefer to do this another way" as blocking feedback, when it isn't.

Good reviews can take a light touch, in part by differentiating non-blocking ideas and suggestions from "hey I think this is a bug."

Some of my favorite reviews of all time actually don't change a single line of code, but the questions reviews ask zero in on what's going to confuse the next developer so it can be documented appropriately.

Usually I answer unspecific 1-sentence criticisms with a question what alternative solution the reviewer would suggest. Sometimes they look into it more deeply and really come up with a better solution. If they don't, at least the discussion is over.