Skip to content

Code Reviewer Checklist

Jump to bottom
Jonathan Sharpe edited this page May 7, 2025 · 10 revisions

This is a guide to help you through code reviews and how to comment on them.

📋 Description

As GitHub's documentation says:

A pull request is a proposal to merge a set of changes from one branch into another. In a pull request, collaborators can review and discuss the proposed set of changes before they integrate the changes into the main codebase. Pull requests display the differences, or diffs, between the content in the source branch and the content in the target branch.

In the CYF ecosystem, we have two general types of PR:

  • Feature PR: this is a pull request from a feature branch into a deployed branch (... -> staging or qa). This (probably) hasn't been deployed anywhere yet, so the job is to review the implementation details (code) and the behaviour (features) by reading the code and running it on your local machine. Approval of a feature PR is generally given by one peer developer and the team's tech lead.

  • Promotion PR: this is a pull request between two deployed branches (staging -> production/main/master, depending on which repo). This is taking functionality that's already deployed in a live environment, where we can test it by interacting with it as a real user would, and . At this point we've already reviewed the details of the implementation in the feature PR - by this point the code should already be well-written and have appropriate tests, documentation, etc., so now we should only be checking the functionality from an end-user perspective. Approval of a promotion PR is generally given by one developer and a non-developer member of the product team (e.g. Product Owner).

☑️ Feature PR checklist

When you approve a PR, you're offering your personal guarantee that:

  1. The PR does what it's supposed to (prerequisite: you know what it's supposed to do!)

  2. You understand how it does that (remember at some point in the future you may be asked to explain or maintain that code)

  3. You can't think of any problems with the way it does that, or any better alternatives (some of those alternatives may have already been considered and dismissed by the author, and if so it's useful for them to include that in the PR description - e.g. see https://github.com/typicode/json-server/pull/1174 where the author explicitly spells out some of the things they didn't implement)

You should be going through the following:

  • Pull request - read the content of the user story/task and of the PR itself (Conversation tab) and consider the following questions:

    • Is the user story/ticket linked?

    • Does the description match the linked item?

    • Does the description tell you how to validate the work?

    • What approach would you take to meet this goal?

    • (If there are previous rounds of feedback or other comments)

      • Do you agree or disagree with the previous feedback?
      • Has that feedback been adequately dealt with by the author?

  • Build - review the results of the CI pipeline (Checks tab) and consider the following question:

    • Does the build still pass?

  • Code changes - read through the changeset (Files changed tab, or Commits tab to see the changes step-by-step) and consider the following questions:

    • Do the code changes match the description?

    • Are there any changes that should not be part of this pull request (e.g. changes in files not related to the functionality)?

    • Do you understand what the changes are doing, how they meet the goal of the PR?

    • Does the approach to meeting those goals seem sensible? If it's not the one you thought of, is it significantly better or worse?

    • Is this well-structured code (consistent layout, comprehensible variable names, relatively small files)?

  • Behaviour changes - check out the PR branch, run the software locally and consider the following questions:

    • Does the product still start and run correctly?

    • Is the goal of the pull request met (i.e. new behaviour for a new feature, changed behaviour for a bug fix, identical behaviour for a refactor)?

    • In the parts of the product this PR touched:

      • Is the spelling, punctuation and grammar for user-facing text correct?

      • Does the layout/UI match the designs?

  • Project progress:

    • Have new tickets been created to cover any additional actions that have appeared during this review?

🏁 Creating a review

✅ Do

  • Provide comments on the code and PR so your work is visible

  • Prefer Approve or Request changes, to be clear about whether you think the code is ready to merge or not, over Comment (or leaving a regular comment in the Conversation tab)

  • Explain what you did to validate the work, e.g.

    I checked the build and I can see the new linting step passing. Running the linting locally on the PR branch also works. If I change the rule back to the old version, I see a lint failure which would be caught in CI.

🤔 Consider

  • Classifying your comments so the author understands what action to take, e.g.:

    • ✅ looks good, no change needed before approval

    • ❓ question, needs answer before approval

    • ❌ problem, needs fixing before approval

    • 💡 suggestion, no change needed before approval

❌ Don't

  • Leave trivial comments like 👍, LGTM ("looks good to me") or nothing at all

📚 Resources

Clone this wiki locally