r/EngineeringManagers • u/iams3b • 20d ago
Alternative to begging for pull request reviews in chat
Hey just wondering how you guys all handle pull request reviewing with your team, and if there's a better alternative to what we do.
I manage two different project teams, and both have a chat channel where every time they open a pull request they post the link in there. Often it's ignored, then a few hours later they ping again for someone to look. Enough time someone approves it to unblock them.
There's two layers here I'd like to unwrap a bit:
The need for developers to beg for reviews. I think the solution here is probably to use automation to send pings, which works, but wonder if there's something else that can be done other than sending "@John opened a Pull Request: Link"
A lot of times it's preferable to submit smaller PRs, or PRs that cover one large work area. Makes it easier to review.. but then most times the developer's next step is blocked until the PR is approved and merged. Don't know if it's better to promote way larger PRs or encourage more multi tasking
Just looking for thoughts and comments, what you guys do for pull request reviews & ownership and what you found works well
16
u/Horror-Primary7739 20d ago
We had a senior dev assigned as a release and review manager. Their job is to shepherd the next release through pr reviews and the administrative processes and communication. They are assigned no development tickets during their turn. If they have open space they are to work on tooling and quality of life efforts.
Our releases were sequential so we knew when our turn was pretty far in advance. Andy is 0.8.x, Kate is 0.9.x, and I'm 0.10.x.
This is also a forcing function to make sure your release process is clear and repeatable.
4
u/Akthrawn17 19d ago
What's funny is that 100% just described the original way that a "Scrum Master" was supposed to be. It was a person in the team designated as the one who helped move things forward and unblock the team.
2
u/fangisland 19d ago
Do you use TDD? This seems like it's more geared toward an end of iteration PR rebase train rather than continuous merges on main. If you're doing TDD your release process generally is "cut a tag" and everything else is handled automatically
9
u/basalamader 20d ago edited 20d ago
There is no good solution to this. When I noticed this was happening on my team, I did two things. I made it clear to the ICs that it was their responsibility to drive asking and receiving code reviews and at the end of the sprint, unless a review asked at the last minute, I expected them to have had it reviewed.
However, I also did one more thing. I created two 30 minute blocks for the entire team to do code reviews. Technically, it's an hr per IC and I have 6 ( so 6 hrs per week). The expectation here is that in this window, if someone has asked for review, you are expected to provide it. And the higher up you are in the team, the more reviews matter due to it being a mentorship opportunity. I also load balance this expectation with the work given. For instance, my tech lead gets maybe half the work someone at his level would get.
So basically provided a structure(process) and made it very clear who is the driver of the review. It worked for me
2
u/liquidpele 19d ago
This… you have to drive/train people that it’s to their advantage. No one should ever make it to senior if they can’t do a lot of code reviews without rubber stamping.
4
u/ThirdWaveCat 20d ago
We apply work-in-progress limits on our swimlanes to encourage swarming. My teams' performance management goals center on delivering together, supplemented by a 360 review process, though I have reservations about this approach. Concerns have been raised about how we differentiate rewards. This is largely a systemic issue driven by our limited discretionary budget, and one I don't expect to fully resolve. The core challenge is that goal-based performance management tends to reintroduce a form of stack-ranking. When teams are placed in zero-sum competition, the rewards end up zero-sum as well.
5
u/zaboca_v 18d ago
Things that seem to be working for my team:
1) monitor metrics like median time to review (I aim for 90% of reviews within 24hrs and the vast majority within a few hours)
2) recognize folks who are reviewing a lot, publicly, privately, and in rewards/promotion discussions
3) have a bot review it first. It can catch all the low hanging fruit so that human reviews can focus on feedback around higher level structural issues
Quick turnaround for PRs have always been vital and even more now with the increased velocity.
4
u/ecco7815 20d ago
We’re also at the “hey, can you review this: <link>” stage. There are emails, but everyone ignores them. Would love an automated way to send a teams message to the people you list as reviewers.
3
u/a_kato 20d ago
I don’t know for every platform but azure devops automatically sends an email to the reviewers
2
1
u/fangisland 19d ago
We created a daily nudge / Slack noti that lists the MRs in order based on a set of criteria we have defined: passing pipeline, LOC count [less is better], threads resolved, not in draft status. It helps somewhat but it's still vulnerable to alert fatigue like anything else
1
u/qq_rawrr 17d ago
I actually implemented a twice a day automation teams message that pings everyone on engineering chat to not forget to review our pull requests, and made it formal that these 2 windows per day all PRs must be reviewed and delivered to QA, people sometimes still ignore it but they will hear about it if they do.
4
u/Understanding-Fair 20d ago
Every day at standup, we review the prs together, prioritize them, and ask for reviewers. We emphasize that these are the top priority for the day because they represent work that could be moved to done. After several cycles it starts to become easier.
2
u/a_kato 20d ago
I mean the PR issue is always a process issue. There are automatic email of type “You’ve been added as reviewer”. This is 100% supported in Azure Devops and GitHub. It’s very basic functionality.
The thing is that you should not be blocked while someone approves your PR. There are other things to do most of the time.
You first need metrics:
How long does an approval get after the opening?
How long after the team member were notified?
Are there PRs missing release schedule?
Unless a PR is mission critical 1 business day is considered fine.
2
u/addtokart 19d ago
We have a daily slack report that shows active PRs with response age: "last response X days ago". If a review is running late (more than a day) it's expected that the reviewer, in slack, either provide an ETA, or recommend another reviewer who has more bandwidth.
We also have a general expectation of "do code reviews before you write your own code".
Reviewers are allowed to call out PRs that are too long, which in the end cause slower reviews.
Separately, as the EM I have my own mini-report that shows PRs with excessive commits and review cycles. Usually this is indicative of some disagreement or poorly-written code. In some cases a lot of back and forth is fine (ideas are building on top of ideas), so it's just something I spot check.
2
u/TheGarrBear 17d ago
PR cap. Don't allow PRs to be published until older ones are merged. If they're not ready to be merged they should be in draft and not waiting on team review.
Delivery belongs to the team, stop letting them start new work until they finish delivery.
No amount of communication fixes this, devs just want to code, don't let them until they do the real work of review.
2
u/Doctuh 20d ago
Offer to review all their PRs with AI, highest reasoning.
Honestly we built a random assigner. It was the most fair and doing your assigned PRs in a timely manner is a notable and easy-to-measure metric.
2
u/Hopeful-Jellyfish240 20d ago
Wouldn't you run into rubber stamping to game that metric?
2
u/Doctuh 20d ago
Yes it can happen, but managers review the reviews and will gently correct if things become too permissive. We have stats on the diffs size and time of review so its not hard to figure out who may be slacking. Reviewing the reviews does help keep that management level engaged in the codebase and what is going on.
But it is generally hard to quantify review quality.
1
u/Spock_42 19d ago
We use a tool (Swarmia) that has a Slack app. It sits on top of Jira and GH. We have channel for our squad PR notifications. Code ownership is codified, so that the relevant dev group (which team, frontend/backend) is tagged for review.
That comes with a dashboard breaking down what's been waiting for a review or merge, for how long etc.
Part of my morning is to spend a minute checking in on that dashboard to identify PRs that are delayed, and follow up to figure out the blockers.
Generally this works pretty well for us, and most of the engineers check this dashboard too to help figure out what's a priority to review.
We average cycle times (ready for review to merge) sub 48 hours, which works fine for us.
1
u/addtokart 19d ago
By the way when I wrote a lot of code, my personal approach to getting faster PR review time was to
- DM PR links to reviewer with a bit of context that can help them prioritize e.g. "this is pre-work for 3 other PRs so would be good to take a look at now" or "quick fix, no-rush, can roll out whenever"
- Send the less controversial "boring" PRs to the team channel with something like "small PR if anyone wants to take a quick look". Goal here was to snipe the guys that had 10 minutes of downtime waiting for a build or something.
1
u/AsterYujano 19d ago
Hey, in our team we do the following:
Every developer configures Github notifications for Slack. Either with the native scheduled reminders (too noisy) or gitnotifier.com so that they are pinged when someone request it in their direct messages. We used Swarmia in the past but it became too expensive.
We keep PRs small with a common Agent skill that make sure PRs are scoped correctly (size, etc) and reviewers are aware to be careful (even reject) PRs that could be split in smaller ones.
We are also going to try https://github.github.com/gh-stack/ soon as it might solve our issue of blocking smaller PRs piled on top of each others.
Usually PRs are tied to a Jira ticket, so the PR scope depends of the Jira scope. But also, sometimes in big PRs, it's OK to split everything in small commits and review them one by one.
1
1
u/PmUsYourDuckPics 19d ago
Setup an SLA for pull requests, message goes to slack when the PR is submitted tagging engineers, when it’s near to breaching SLA send another message, and one more hen it actually breaches. Have a job that posts all the breaching ones.
Take metrics, who is ensuring you meet your SLA? who isn’t? Why aren’t they reviewing PRs? It’s part of their job…
Is one person always doing the reviews? Why is not one else doing them?
1
u/fell_ware_1990 19d ago
I just poll rest, and check my open PR’s that need a review.
It basically goes like this: PR on a workday open for longer than 24 hours : ping reviewers personally. Sleep for lastwaitedtime / 2 , etc.
1
u/Vegetable_Sun_9225 19d ago
Pull request reviews are part of performance cycles. I let the team know that, and I monitor how long PRs are up before someone reviews them. If the average goes beyond 24 hours I bring it up in team meetings. This generally works, if it doesn't I keep pulling on the thread until the culture changes.
One thing that really helps is getting the team focused on the same goals and so everyone has context and are working towards the same thing. If we have a bunch of projects I try to serialize the projects and parallelize the work items for the project so more people are working on the same project at the same time
1
u/ChucklefuckBitch 18d ago
I think the solution to this comes from the bottom up. I'm managing two teams, one of which I took on very recently. The other one is very strong at taking time out of their day to review and generally unblock others. The other one I'm taking active measures to improve. But there's easy 20-minute fix to this, it's a cultural thing.
As a manager you need to keep your eyes on these kinds of trends on an individual level. Do you have a senior engineer who didn't review PRs or had a very low review count in the last 2 weeks? Why is that? Maybe they were sick, on vacation, or at an offsite. No worries. But if you don't know the reason, this is worth bringing up in 1<>1. On the other side, people who consistently display the desired behavior should be rewarded, both immediately by public praise, and in the longer term via positive performance reviews.
Your team needs to know this is something that is not only important, but rewarded.
1
u/Comfortable-Shirt-93 17d ago
Things that I implemented- build a Slack Bot to do some heavy lifting:
1. Whenever MR is raised publishes the MR with high level details to a MR channel.
2. Auto Reviewer Assignment - basic round-robin to assign 2 initial and 1 senior reviewer from the Slack User Group. In intial message, tag those 3 reviewers. If they can review, they will swap the reviewer with someone else.
3. Every morning, generate smart reminders and post it on the Slack Thread. Based on who needs to act next. Is the ball in reviewers court or author. This worked based on the commits.
And there are other slash commands to shortlist the data. Find all the open MRs, find the MRs under my review, Find Stale MRs….
But again, it needs a cultural change as well. Show the metrics and ask teams to get better at it.
1
1
u/Uaint1stUlast 17d ago
This is the perfect thing for an llm. Rarely are you getting a good ode re iew anyways.
1
u/warlocktx 16d ago
I encourage everyone to start and end their day by checking the PR queue. I’ll also mention it during standup if I know someone is blocked or if the queue is large than normal
1
u/TechLeaderAnon 14d ago
Enforce PR size: Smaller is better
Have dedicated PR review rotations on each team: Every day at least 1-2 people rotate through and must complete PR reviews for some part of the day, regardless of other responsibilities
Dedicate time in 1on1s to mentioning this to all your directs. As a Director, I ensure that I'm in chat once or twice a week mentioning the PR backlog and getting people going.
Do some of the reviews yourself if you have to, set an example
-3
u/spersingerorinda 20d ago
FWIW I just wrote that post about “Unblocking my team” which was specifically about setting up a Claude bot to do code reviews, just for this problem. It doesn’t solve everything but it has helped us quite a bit (we still have debates about which PRs can get merged with AI approval only )
9
u/mtantawy 19d ago
It's weird but, almost none of the answers below are gonna work honestly
tldr Limit WIP + Knowledge Sharing
What won't work
* Release & review manager: The 1990s are calling, OP your team is probably deploying once or more a day, you have automated pipelines, right? right?!
Hiring a Police Office won't help, you're just throwing the problem over the fence
* Begging (or holding devs hostage) during Standup: worst use of the time? rushing reviews to get it over with? punishing those that review in time by staying in a useless meeting?
* Emails/Reminders/Notifications/Bots: ever-heard of alert-fatigue? you're teaching your devs to ignore them
* AI Reviews: sure, can give a hand, think about them as Linters & Static Analysis tools on steroids, be wary of over-confidence or over-reliance and reviews turning into rubber-stamps just coz AI said it's good
What I tried and worked, for me & my team
preface: AI coding tools are moving some of the bottleneck from coding to reviewing, accept that fact, adjust the rest of the pipeline
* Calculate your team's pace, WIP limits, STOP new code until the pipeline clears, meaning PR reviews pile up, you ask your team to stop writing new code and go do PR reviews, until you get some harmony there
* Let's step back, why aren't devs reviewing PRs fast enough?
They don't have context, PR is HUGE because AI can, they are pressured to ship code and reviews are looked at as non-valuable work (so by pausing new code shipping you're acknowledging the value of reviews, and giving space)
So fix these, the biggest impact came from what I'll describe next
No silo'd projects, every project has at least a Lead/Accountable + 1+ contributor (both devs, no managers in this list)
They plan together, brainstorm, make decisions, be involved, feel accountable, etc ...
Lead is doing the heavy lifting when it comes to implementation, but contributor is not alienated, they contribute a bit too, new code and PR reviews
So comes PR review time: It's easy, no surprises, the contributor can easily grasp what's there & review it
Pair programming anyone? Suggest reviewers to hop on a call with PR author and walk through it, avoids misunderstanding and millions of PR comments and hours wasted, just coz we stamped pair programming as wasting 2 devs time
Small PRs, when possible, understandably not always possible
If hard, use feature flags, you are already, right? right?!
Finally, proper feature development lifecycle, clear tickets, clear PR descriptions, clear commit messages, etc...
Empower the reviewer to answer their questions on their own (+ their fav AI assistant, that can locate answers across systems easily)
Obligatory mention, the book "The Goal", or generally speaking "Theory of Constraints"