Ideas for better reviews of larger PRs

[chrisdavidmills]

What follows is a write-up of a discussion had in the Open Web Docs steering committee meeting, October 18, 2023.

Parties involved in the discussion were myself, @Elchi3, @estelle, @wbamberg, and @robnyman. Also mentioning @rachelandrew and @Rumyra here, to make sure Google and Mozilla docs leadership have awareness and can provide input.

The problem

Smaller MDN content PRs are not problematic as such. The MDN codeowners system automatically assigns reviewers, and the reviewers do the reviews as part of their day-to-day tasks.

However, larger PRs can be problematic — they can take hours or days to review, and everyone is super busy and finds it tough to commit to spending the time in their schedules doing these reviews. As a result, sometimes large PRs can sit unreviewed for weeks at a time.

Solutions

The following are some ideas we came up with on how to improve the situation.

Clear separation of technical and editorial reviews

Often, a large PR goes through two phases — a technical review and an editorial review, at least in the case of my PRs. I get a Google engineer to comment on the technical accuracy of my work, and then once they are happy I ask one of my MDN writer colleagues to do an editorial review.

There was a comment made that it is often difficult to tell when to start the editorial review, plus the large volume of technical review comments is distracting and hard to navigate. To remedy this, we are proposing starting up a system whereby we do the technical review in one PR, and the editorial review in another PR. This will hopefully serve to provide a) a clear signal that the technical review has been completed, and b) a separate place to do it where you are not distracted by all of the technical review comments.

The process would go something like this:

1. Create the first PR as normal. Call it something like "Technical review of docs"
2. Get the technical reviewer to do the review, then make updates.
3. Once the technical reviewer has approved the work (get them to provide an LGTM comment):
   a. Write a comment informing involved parties that the PR will now be closed and a new one will be opened to contain the subsequent editorial review.
   b. Close the PR. IMPORTANT: DO NOT delete the branch!
4. Open a new PR comparing the same branch against the mdn content main branch. Use Pull requests > New pull request to achieve this
5. Title the new PR something like "Technical review of docs". In the description, make it clear that this is an editorial review of work that has already had a technical review done. Link to the technical review PR.
6. Inform the MDN content team that it is ready for editorial review.
7. Add a note to the top of the old technical review PR along the lines of "Note: This technical review is now completed and approved. For the follow-on editorial review, see "
8. Optionally, write "DO NOT MERGE" at the beginning of the technical review (although it should be obvious enough from the fact that the PR is now closed.)

I have tested this process out on my new content for the Window Management API. See:

• Technical review PR: DO NOT MERGE: Technical review for Window Management API docs content#28851
• Editorial review PR: Editorial review for Window Management API docs content#29719

Splitting up PRs into smaller chunks

There was a mixed reaction to this idea. While this makes each individual PR easier to manage, the overall amount of work remains the same, and it can be a pain to figure out how to split the work.

One necessity would be to include a prominent note on each PR informing reviews not to merge each PR until all are complete.

Create a plan first, and discuss it

One thing I've not been very good at is discussing my work before I do it. I'm not sure how Mozilla or the OWD have been handling this, but going forward I propose that I will document the plan for each intended bit of work in an issue on the mdn content repo first, then bring it to a forum such as the OWD SC meeting to get eyes on it before starting the work.

The plan should include:

• Information on when the feature will go live, and in what browser versions
• A link to the latest spec
• Links to supporting information such as Chromstatus, blog posts, explainers, demos
• A list of what new MDN content pages should be created
• A list of what BCD work has been done already (if none, just assume that all the listed features need BCD content)
• A list of what MDN pages already exist that might need updating

This will help facilitate reviews by surfacing high-level structural issues early (the kinds of things that could be expensive to fix once the documentation has already been written) and giving editorial teams a heads up so they can think to set aside review cycles for this work ahead of time.

Generally discuss review needs in forums more

Rather than just pinging people on PRs and hoping for the best, it will always bring better results to turn up to meetings and discuss review needs face-to-face with people. This builds up better relationships and helps to make the ongoing review pipeline more transparent.

In terms of making a transparent review pipeline available, I'm not sure of the best place to publish such a thing, but I'm open to ideas.

[robnyman]

Thanks for writing this up, @chrisdavidmills! I think that sounds good overall.
A few things that I hope might help this discussion and PR reviews:

• Documentation for how PR reviews should be categorized, e.g. P0 topic (Interop, Baseline etc) with completed specs vs. experimental technologies.
• The expected time for how long time a PR review should take, to be able to have clear expectations (can be dependent on the category of PR)
• Clear visibility and ownership of who will do the PR review, and a reasonable time frame for reconnecting with the PR submitter

[chrisdavidmills]

These are all really good points @robnyman. For the first one, this bleeds into the larger discussion of PR triaging and sorting. We were discussing this in the writers' meeting we had earlier this week. Paging @estelle so she is aware of your suggestions here.

For the second one, I agree, but this is tricky - it is very hard to estimate review times. We should think about what is needed here. In the past, we have gone with something like an old-fashioned t-shirt size type approach.

For the third one, yup, totally. I think this will need to be discussed as set in editorial meetings, and then a timeframe needs to be decided on.

[robnyman]

Thanks @chrisdavidmills!

1. Completely get it, and believe that having something like that will benefit everyone.
2. Yes, I get it. I think there's a challenge when reviewing PRs – especially large ones – is that when you look at the numbers it's just one review, but timewise it can be a massive effort. T-shirt sizing can definitely be a good first step, at least.
3. Yep, completely agree!

[wbamberg]

Clear separation of technical and editorial reviews

...
I have tested this process out on my new content for the Window Management API.

I just merged that PR and thought it was a much more manageable process.

[bsmth]

Clear separation of technical and editorial reviews

How has this been going so far? Just occurred to me; given this is always a two-phase process, I wonder if it's a good idea to do the technical review in your fork, i.e.:

• Technical PR: chrisdavidmills/content@dev -> chrisdavidmills/content@window-management-api
• Editorial PR: chrisdavidmills/content@window-management-api -> mdn/content@main

The same workflow you're currently doing, but without the placeholder PR that's closed each time. Codeowners don't get auto assigned as reviewers, the review queue is easier to navigate, with the downside that you don't have a live preview & some tests (example).

[chrisdavidmills]

Hi Brian!

Thanks for sharing your idea.

So basically you're saying that I should try creating the tech review PR by comparing the changes against my own fork's main branch, then once that's completed I should create the editorial review PR by comparing the changes against upstream/main?

I'd be happy to try it. The main things I'm concerned about are that:

1. I find the tests useful when creating the PR, to make sure nothing untoward has happened with the marking structure, etc.
2. I reckon the tech reviewers find the previews really useful.
3. I don't feel like I'd gain that much from doing it like this. Closing the PR and creating a new one is only about 10 minutes' work, and I'm not convinced that it makes the PR itself difficult to navigate. In terms of the review queue being harder to navigate, do you think we could mitigate this with a clear labeling system for the tech reviews, e.g. always start with "Tech review:", and write a GitHub rule that doesn't label these with "review needed", or some other flag we could use the pull together the PRs that need triage (just thinking out loud)

Number 2 is just a hunch; I'd be happy to ask the Google engineers that I regularly interact with how useful they find the previews, whether they use them a lot, etc.

[bsmth]

Heya 👋🏻

So basically you're saying that I should try creating the tech review PR by comparing the changes against my own fork's main branch, then once that's completed I should create the editorial review PR by comparing the changes against upstream/main?

Something like this, it doesn't have to be a PR against your fork's main if you want to keep that syncd with mdn/content. You could do a PR from cdm/content@branchA -> cdm/content@branchB and have technical discussion / comments and suggestions on that PR.

Re: 2, doing a checkout & yarn start on the branch would be the alternative to the live preview, I suppose.

you think we could mitigate this with a clear labeling system for the tech reviews

In general, I really like labels for things like this because they're easy to create and apply, pretty visible, and have descriptions attached that explain what they're about. I think I added "Do not merge" to some of the technical review PRs just to highlight that at some point. We still have 2x PRs per task/topic, which gives me the impression we can improve something.

write a GitHub rule that doesn't label these with "review needed", or some other flag we could use the pull together the PRs that need triage (just thinking out loud)

Maybe that's possible, yeah, that's something to look into in future.

just thinking out loud

Same here, if it's too much friction for now, stick to what's working! 🙏🏻

[chrisdavidmills]

This is answered; I've been using the technique of splitting up the work into tech review PRs and editorial review PRs, and it has been working fairly well.