How I review a Pull Request

NOTE: This was originally intended for my co-workers at Skuid, but I figured it might nice to share with the world. Hope you enjoy.

I want to say, first and foremost, this is just how I do things. It's not everything I do, it's not necessarily complete, it's not prescriptive, and it's not a codified "Skuid way" to do PR reviews. The following is roughly common threads in my thought process when going through a PR. If you, the reader, learn something then that would make me really happy. But if you don't this has been a good exercise for me as well.

The second thing I want to say, is that if you like the way I've reviewed PRs in the past, I want to say that I don't do anything that you can't do too. We all have unique ideas from time to time, but really all I'm doing is asking questions.

And if you don't like how I've reviewed a PR of yours, then I am very open to feedback. Send me a message and lets talk.

Here are the sorts of questions I ask. These are roughly in order too.

1. Are the builds passing? If they aren't I usually let the submitter know, unless they've specifically asked for feedback on an incomplete PR, or if it's a draft PR.
2. What is the functionality like? It's usually helpful to know ahead of time if something isn't working when I'm going through the code.
   • Does it do what the ticket describes?
   • Do the non-happy paths work? I usually try to modify the test page to try things a little out of the ordinary.
   • Does the ticket describe enough information for QA to do their job without reaching out to an engineer?
     • Does it have a test page?
   • Will it cause regressions? Will it cause anything that will feel like a regression?
3. How is the UI Design aspect? We're not the designers, but we can save them from a lot of trouble if we handle as many aspects as we can before it circles back to them.
   • Are there any borders, spacing, colors, border radii, etc. that do not fit within our design system guidelines?
   • How are the contrast ratios? Does it look like it would pas WCAG AA guidelines?
   • If anything feels "off" verify that this aspect of the experience has made it through design. Inevitably there will be gaps in the designs we sometimes need to talk through a little more.
   • Is there any "jank"?
4. How is the approach, from an "architectural" standpoint?
   • Is it sustainable? Is it extensible? I try to think what happens if we need to do something similar 3 or 4 more times. Sometimes it's too early to create an abstract solution, but it's nice to not dig ourselves into a hole either.
   • Is it too sustainable? Is it too extensible? It's not all the time, but I'm always on the guard for making systems that we don't need yet. Usually this presents itself in the form of making a function that's only used once. And yes, I know this contradicts the earlier point; it's always a balancing act
   • Is it "fixing" the wrong part of the code? This is always a tough one, but sometimes I see us (myself included) "fixing" something to satisfy Acceptance Criteria on a ticket, but there's actually a deeper issue going on that can be adjusted instead to prevent other similar issues.
5. What is the code quality like?
   • Are there unnecessary event handlers? Subscriptions? Is it stylistically odd?
   • Are we using a library where when we don't need to?
   • Is the code introducing any new libraries that aren't strictly necessary?
   • Are we adding any functions to our public API that we don't need to?
   • Are there areas where a comment might be helpful?
   • Are there any "magic" numbers or strings that should be named constants?
   • I there the potential for performance pitfalls? Will it work as well in a complex page as it will in a simple one?
6. Is the solution accessible? If accessibility is too much to ask for in the initial PR, I usually will ask the submitter to create a follow up accessibility ticket.
   • Does the focus end up in the right place?
   • Can a screen reader correctly parse it?
   • Does it use the right roles and aria attributes?
   • etc.
7. What are the tests like?
   • Is it testable with our current tools?
   • Is there a test? If not, and it is testable, I'll ask for a test.
   • Does the test actually cover the situation described in the ticket? Although not super common, sometimes we write tests that don't end up covering the initial problem. I've caught myself a couple times doing this.

If you've stumbled upon this article, and read the "Skuid" and don't know what that is, it is the name of the company I work for and the product I work on daily, a no code UI development toolkit. I love to make the company I work for look good, but it should be noted that this is not an official Skuid piece of content. The views, thoughts, and opinions expressed in the text belong solely to the author, and not necessarily to the author's employer, organization, committee or other group or individual.

Comments

[Aftab Syed]

Nice article 👍

[Andrew Duensing]

Thanks Aftab! Glad you liked it.