Reviewing pull requests

Most weeks, the bulk of website team work is reviewing pull requests. Some are blog posts. Some are typo fixes. Some are a translation team member adding a Spanish version of a page that was written in English last month. Some are a contributor adding their first chapter. A few are us — fixing something we broke.

Reviewing well is mostly about reading what GitHub Actions has already told you, then doing the parts CI cannot — actually loading the preview, checking the words make sense, deciding whether the change belongs. This page walks through how to do that without missing things.

The assignee is the first owner

Every PR is auto-assigned to a website team member through CODEOWNERS. If your name is on it, you are responsible for getting it reviewed — which does not mean you have to be the one reviewing. If the change is in code you are not comfortable with, or you are unavailable for the next few days, reassign or ask in #team-website for someone to take it. The worst outcome is a contributor’s PR sitting open for three weeks because nobody noticed they were waiting.

If you are the reviewer, you are the one approving, requesting changes, or merging. Your second pair of eyes does not need to also approve — one approving review is enough — but your judgement is what unblocks the merge.

Read the PR comments first

By the time you open a PR, GitHub Actions has usually already commented several times. The comments are the cheapest signal you have, and most of them update in place rather than spamming the PR. The full lineup is documented at GitHub Actions and CI — this section is the reviewer’s eye view of the same workflows.

Here is what each comment is telling you, in roughly the order you should read them.

check-build — does the site even build?

This is the gate. If it is failing, nothing else matters yet — the production build will fail too. Click into the workflow run from the comment and read the last few hundred lines of the log. The most common causes are bad YAML in front matter, a broken shortcode, or a markdown image with an unresolvable path.

A green check-build means the contributor’s change does not break the site under a production-style build with the full directory data merged in. A red check-build from a fork is occasionally a missing-secret false alarm — verify whether the build step actually ran, or whether the job stopped before it.

check-jsons — is the structured data valid?

For PRs touching data/chapters/, data/directory/, or data/sponsors/, this validates the JSON against the schemas in scripts/json_shema/. If it is red, the comment names the file and the field. Almost always, this is a contributor mistake — paste the relevant schema rule back to them so they know what to satisfy.

blog-lint — does the post have what it needs?

For blog and news PRs, the lint comment splits into hard failures (block the merge) and soft failures (suggestions). Hard failures: missing title, author, or date; non-YYYY-MM-DD dates; body images without alt text. Soft failures: missing description, categories, or image; unstructured author lists.

Hard failures must be fixed before merge. Soft failures are conversation starters — sometimes the contributor wants the suggestion applied, sometimes the post is intentionally minimal.

lighthouse — accessibility and performance

The comparison table is per-page, against the same page on production. A drop in scores is informational; a hard failure means an image on an audited page has no alt text. The comment lists the specific images. Send the contributor back with a request: alt: "..." describing what the image shows for screen readers, not just what file it is.

The same workflow runs the Lychee link checker. Broken external links are very common — Meetup pages get deleted, Twitter handles change. Skip the noisy ones; for substantive broken links inside the new content, ask the contributor to fix.

i18n-check — translation coverage

This never fails the build — placeholders are auto-generated by missing_translations.R at build time. The comment is for the translation team’s awareness, and for reviewers of translation PRs. If the PR is a translation PR, the comment shows you what coverage will look like after merge.

checklist-blogpost — editorial cues

Pure informational. The blog editorial team uses this checklist to walk through publication readiness with the author. If you are reviewing a blog post and you are not on the editorial team, leave the checklist conversation to them.

Click the Netlify preview

Every PR — fork or not — gets a Netlify preview URL commented on the PR by the preview action. Open it.

What CI cannot tell you:

  • Does the page actually look right in light and dark mode?
  • Do the contribution superscripts (ᵃ ᵇ ᶜ) line up with the legend at the bottom?
  • Does the directory entry the contributor added show their photo?
  • Are the headings in a sensible hierarchy, or did they skip from ## to ####?
  • Does the page render the way the contributor expected, or did a shortcode silently fail?

Spend two minutes on the preview before approving. For blog posts, also load the post on a phone-width window — the gallery and pull-quote layouts behave differently below the md: breakpoint.

What to look for, by change type

Different PRs need different attention.

Content-only PRs (blog, news, chapter info, page text):

  • Read the prose on the preview, like a reader.
  • Check the front matter on the diff — YAML is where breakage hides.
  • For blog posts, confirm the directory_id resolves on the byline in the preview.

Translation PRs:

  • Compare the English version side by side; structural changes in English need the same change here.
  • The translation team owns the linguistic side; you are checking that the build is intact and the structure matches.

Layout, shortcode, or theme changes:

  • Read the diff carefully.
  • Pull the branch locally if it touches a partial used in many places.
  • Verify dark mode on at least three different page types — homepage, blog post, directory.
  • Confirm hugo --minify is clean locally; check-build does not catch every theme regression yet.

R-script or workflow changes:

  • These are rare and high-stakes.
  • Get a second reviewer if you have any doubt — the data pipeline is brittle and these workflows run on cron with no human in the loop.
  • Look at the most recent build-production run to confirm the change does not break a moving target.

When CI is red but it is not the contributor’s fault

Sometimes a check fails for reasons unrelated to the diff.

Common flakes:

  • Lychee timing out on a slow remote site — re-run the workflow.
  • The directory clone failing because RLADIES_DEPLOY_KEY rotated — flag in #team-website.
  • A meetup-archive fetch hitting Meetup’s API rate limit — re-run, or wait fifteen minutes.

If you suspect a flake, re-run the failed job once. If it fails again with the same error, treat it as real. Do not approve a PR with red required checks even if you “know” it is flaky — branch protection enforces the green tick anyway, so get it green first.

Approve, request changes, or comment

The three review verbs map to three different intents.

Approve is your sign-off — the change is good as-is and you would be comfortable merging it. Request changes is a hard block — you do not want this merged until something specific changes. Comment is everything else — questions, suggestions you would like but are not blocking on, encouragement.

Use request changes sparingly. For first-time contributors especially, a comment with a kind ask works better than a hard block. Save request changes for when the PR genuinely cannot merge — broken build, missing required field, breaking change in shared layouts.

Inline suggestion blocks let the contributor accept your edit with one click. This is the right tool for typo fixes and small front-matter corrections. For bigger asks, write a regular comment so the contributor decides how to apply it.

Merging

Whoever approves typically merges, but anyone with merge rights can. The default is Rebase merge — it keeps history linear and matches what merge-pending does for scheduled posts. Squash is fine when the contributor specifically wants a single clean commit, or when the branch has many WIP commits that nobody needs in the history.

Avoid Create a merge commit — the protected main history stays cleaner without merge commits.

After merging, watch the production build run from the Actions tab. If it goes red on a change you just merged, the GitHub Actions and CI page covers the failure-mode triage.

When something goes wrong after merge

Production deploys from main on every push. If a merged PR breaks the live site, the fastest fix is usually a follow-up PR that reverts or patches the offending change.

For genuine emergencies — broken homepage, broken Code of Conduct — you can gh pr merge --admin over branch protection, but only do this if you know exactly what the fix is and you are willing to own the consequence. Almost every “emergency” is better handled as a fast-tracked PR with one quick review from another team member.