Commenting and approving pull requests

Code review has value, but I don't think we are always honest about the costs. At most places I've worked, there has been an informal understanding that code should be reviewed within 24 hours or so, but there is a world of difference between getting a review within 2 hours and 23 hours.

If you have to go back and forth a second time, it's so much more likely that the approval comes much later due to straddling end-of-day in someone's timezone.

Tangentially, if you are designing policies for code review at your org, try to think both about minimum requirements (e.g. PRs should get a first look within 24 hours) and typical requirements (e.g. most PRs should get reviewed within 2-3 hours). What typically happens is what determines overall velocity for your org, so it's much more important than whatever strict SLA policy you have. These are also fixable problems if you have targeted conversations with people. E.g. "I noticed X, Y, Z times, you had unreviewed PRs at the end of the day. Can you set aside some time to review code before EOD? Or try installing PR reminder?"

Things like that I'd much rather leave as comments in the code, rather than dangling as off-hand things in some unrelated PR. Probably no one will see those PR comments unless they go splunking for some reason, but having them right in the code is much more evident to all the people passing through, or even reminding yourself in the future too.

In general I feel like PR reviews are basically reviews happening too late, and if there is a lot of stuff to review and agree upon in the PR, you'll be better off trying to reduce that up front by discussing and noting design decisions before the work even starts. If there is so many unknowns first, do one step to figure out how it can be done, then agree on that, again before starting the actual work.

That leads to PRs basically just being spot-checking, not deep analysis nor even space for nitpicks. If you as a reviewer spots things after the implementation rather in the discussion beforehand, that ends up being on both of you, instead of just the implementer who tried to move along to finish the thing.

The 'auto merge on approval flag' PR authors can flip on GitHub breaks this flow though, as it will just merge as soon as you hit approve.

You're essentially suggesting pre-PRs, but it is circular, since those same pre-PRs would have the same criticism.

PRs are about isolated core changes, not feature or system design. They answer how not why.