Code Reviews: Giving and Receiving Feedback

Colin: Welcome to Build and Learn.

My name is Colin.

CJ: And I'm CJ, and today we're
talking about reviewing poll requests.

If you recall, a couple episodes ago we
talked about authoring prs, but today

we're talking about the other side of
the puzzle, which is reviewing them.

Before we get into that, we wanted to
just talk about some upcoming conferences

and I'm, I'm really jealous because there
is this really epic conference that's

coming up that I really wanted to go to.

And Colin is , Colin is headed there.

That is Rails SaaS.

So yeah, like what is Rails
SaaS and when is it happening?

Colin: Yeah, so let me
just pull it up here.

So it's October six and seven.

I think this is gonna be a
little interesting to see

when this episode comes out.

Rails SaaS may have already happened
when this episode comes out.

But we've got a lot of really cool
speakers that are talking about the

intersection of rails and business.

So you've got some folks who are building
SaaS businesses on top of rails, like

literally, like things like GoRails.

Hatch box hammer stone.dev, or they're
building and releasing gems and things

as, as a, you know, as a business.

And then I imagine that there's
gonna be a little bit of you know,

kind of like the stuff that you
see with that Saastr conference.

There's a lot of just, you
know, the business side of

rails, which I'm excited to.

CJ: Cool.

Yeah.

So SaaS, if you're not familiar,
is software as a service.

This is typically like a
subscription based thing.

We all know these and love them.

If.

own one and like loathe them if you have
a bunch of subscription payments that

are hitting your wallet every month.

But yeah, so I, yeah, SaaS in
general is something obviously

that I'm like pretty excited about.

And yeah, I've been working on some
content for Stripe teaching people how

to collect recurring payments and how to.

Build a SaaS basically with Rails.

And that's why I'm like super bummed.

I'm gonna get, I'm gonna miss this,
but we'll be on a family vacation that

we planned a super long time ago, even
before the conference was announced.

So yeah, but I'll be missing all,
all my all my homies that are gonna

be in, it's in, is it in Hollywood
or it's just in LA somewhere.

Colin: It's in Hollywood and
it's it's organized by Andrew

Culver from Bullet Train.

So they have like a, they kind of bill
it as the Ruby on Rails SaaS template.

CJ: Hmm.

Colin: so they give you kind of an
out of the box framework for building

SaaS companies on top of Rails.

So very cool to see like a thematic
conference like this instead of just

focused on, you know, the language
and the tooling and the processes.

It's gonna be talking about like,
how do you use this to be super

productive in building a business?

Right.

Cause building and building something
that people want and are willing

to pull out their credit card.

And, you know I guess Stripe would
be very relevant here because of how

many subscriptions and, and things
are built on top of Stripe and Rails.

CJ: Yeah, and I think, yeah, in
Andrew's bullet train part of the I

think it's like the pro version or
something you can install like a.

A few Stripe tools that will
set up routes for you and a

few other a few other things.

I haven't actually bought the paid version
of Bullet Train to test it out, but it's

my understanding that you get a lot of
features outta the box with that sort of

like plug into the bullet train starter.

So

Colin: Cool.

And what other conferences are
coming up that you are able to make?

CJ: Yeah, so I I submitted to
speak at Ruby Comp this year.

I put in two talks.

I have not heard back yet.

We'll see by, definitely by the time
this episode airs you, I will know

whether or not those were accepted . But
yeah, Ruby Comp is in end of November.

It's in Houston, Texas.

Excited about that.

Excited to see all my Ruby friends.

Are you going to Ruby Conf this?

Colin: I was not planning on it.

I have not really looked
at November yet, but.

CJ: Okay.

Yeah.

And then another one that's happening
in November is Chirp, which is

the Twitter Developer Conference.

I think this is the first time
they're having it in many, many years.

So this is gonna be in San Francisco.

It's a one day conference.

So yeah, that

should be, it should be pretty fun.

Colin: That one.

I'm definitely gonna make it down for.

I, they claimed it was the first chirp
in 10 years, but I did see that there was

like a conference in 2015 that they held.

This one.

I'm really excited to kind of see how
and what they've learned from, because

Twitter has a history of kind of letting
down developers in the past, and so I

think they've realized like they've got a
repair their relationship with developers

and we'll see how that goes with Chirp and
I'm hoping for a lot of new API features.

We are using the Twitter API more
and more at Orbit, and there's some

stuff that's like very lacking in the
API that are obvious things, so I'm

excited to see that they're reinvesting
in in that relationship and the api.

CJ: Yeah, so they shipped
in API V two recently.

And as part of that, they
have like a whole new OAuth 2.

Flow.

So yeah, I've been playing around with
the new APIs quite a bit and yeah,

there's a few things that I would
love to see, like way better webhook

support or webhook support at all.

There's like no way to get the
email address of the person.

There's like, yeah, a bunch of like
little things where I'm like, Oh

gosh.

Like, Huh,

Colin: that we should, we

CJ: I know.

Yeah.

We could dig,

we could dig into.

Colin: API audit.

CJ: Yeah I, I've been using it a lot
lately too, just for like fun little side

project things and yeah, I think they've
definitely turned a corner in terms

of listening to the developer audience
and building, starting to build towards

like what the, what the audience needs.

So yeah, excited to

go check it out and see

how things go.

Colin: to do it.

A little audit of, of various APIs.

Especially like for us, we, we
added a feature that we're working

on an orbit, a little preview here
of like being able to send dms.

CJ: Ooh, Nice.

Colin: us permission to
everything in order to send a dm.

CJ: I bet.

Yeah.

Colin: was like, Why is this not just one?

Like scope.

CJ: Yeah.

Colin: have to have, I could do anything
I want on your Twitter account, which,

you know, we don't, we only send dms
and there's only code to send the dms.

So it's not like we can accidentally
do anything on your account.

But it's, it is that scary oauth
screen of like, you're allowing Orbit

to have access to all this stuff.

And so

CJ: Yeah, I think, yeah, I, I would
love to do episodes about that, just

like look at an API and do like a
breakdown of the DX and so yeah,

that would be, that would be fun.

I think.

Yeah, there's probably a bunch of APIs
too that we're both using just for fun.

Like for fun and profit, right?

Like on the side , like YouTube api,
Twitter api Orbit API or Stripe api.

So yeah, it'd be, it'd be
cool to go through All that.

Colin: So I guess in building those
APIs, you're gonna have a whole lot

of PRs in your team, and hopefully
there's small reviewable prs like we

talked about on build and learn.dev/six,
if you wanna check out that episode.

But today we're gonna dive into
that other side of the fence

where you've submitted your pr.

And you're waiting for someone to
review it, that person's gonna pop

into GitHub or GitLab or Bitbucket.

And now you have this little bit
of science, a little bit of art,

of reviewing someone else's code
and giving constructive feedback.

Making sure that you kind of match
the, the rest of the style of

the code base, things like that,
especially for newer developers.

So yeah, we can kind of dig in.

We've.

A link to a pretty thorough tip
like list of tips from thought Bot,

but that we can kind of review.

But I think it'd be great to kind of hear
like what you first look for when you,

you know, once you get that notification
that it's time to review something.

CJ: Yeah.

So first and foremost, something that I
think is important is responding quickly.

Like trying to like actually.

Look at, if someone opens a pr,
try to unblock them and respond to

their pr because the longer that
you wait, the longer that it's gonna

take them to like ship their thing.

So yeah, jumping in quick and giving
them some feedback or at least telling

them like, Hey, I'm really swamped.

This is gonna take me 24 hours
or something to get back to you.

Yeah, so that's kind of just
like a, a preface, preface thing.

But another thing that I tend
to look for is any instructions

in the description of the pr.

Like, do they have pointers
for what they're looking for?

Do they have requests for
reviewing specific files?

And, you know, oftentimes the description
will say something like, Oh, a lot

of this was auto generated, but can
you please look at file X to you?

You know, make sure that there's a
really good thorough review of security

on, on this like section of whatever.

So yeah, looking at the description and
yeah, being timely in your reply, I guess

is like the first thing I would say.

Colin: Do you guys have like a how to test
section of prs, like in your template?

CJ: We do.

Yeah.

So we, it's actually not how to
test, it's how it was tested.

So we ask people Yeah.

Like to, you know, outline exactly
how you tested it, and that could be,

I wrote a bunch of automated tests,
or if it's a change to like a docs

page, it could be like, here are
the screenshots of the changes that

I made, and also here's a link to.

The staging server, where
the docs are or whatever.

So

Colin: Cause along the lines of
not blocking them, like some of our

PRs require, like as a tester, like
we know that the other person who

wrote it has already gone through
those steps, like you mentioned.

But I do like to like then
run them on my machine, Right.

Or run them on testing, like a
staging or integration server.

But for some of those, you know,
hopefully they're small reviewable

prs, but some of 'em you have to like
do a little bit of world building or.

A script that creates all of
the, the scenario that, that

you need to test this thing in.

And sometimes there's like a bug or
something that's hard to replicate.

And so relying on tests is
sometimes all you can do.

But you know, for us, we do like
to, especially with integrations, we

want to test them on another machine
so that it's like, hey, like some

environment variable didn't make
it into the encrypted credentials.

Something like just about their machine
or their setup is just not quite the same.

With integrations, we end up using
things like ngrok and relying on web

hooks and stuff like that a lot too.

So like making sure that it's like,
Hey, yeah, I caught a web hook

but it didn't hit the right path
on my machine for some reason.

Or just like some sort of
environmental kind of ruling

out environmental differences.

which for some companies, if
you're running in like Docker or

something, those may not even be.

We run pretty bare rails
on, you know, Mac, so,

CJ: Got it.

Yeah.

I was gonna ask, so our, our development
environment is actually like external

boxes that live inside of aws.

And then also once we push
a pr it will spin up staging

environments just with that PR so
that you can link to it from the.

From the PR itself.

And the nice thing about that is
that the environment inside of

AWS when you're building and when
you're in staging and when you're in

production is all the same, right?

Like, it's gonna be on the same boxes,
the same tools, the same like everything.

So that is like, I don't know, it's,
it's, I would say it's a nice experience.

But it's also like super challenging.

. There's a lot of like DevOps overhead
stuff to get that set up on a team.

So

Colin: review apps on Heroku for that.

So we get a little bit of that out
of the box without having to have a

whole DevOps team build that for us.

So we do that when it's necessary.

Some changes, like again, docs changes.

We don't need to spin up
a review app for that,

CJ: Mm.

Colin: like read me updates
and stuff like that.

Those are fine.

We can review them for
accuracy and stuff like that.

But yeah, I think that's a, that's
a pretty good process to get.

CJ: So going, going back to timeliness,
I assume that your team is so globally

distributed, that sometimes your
reviewer will be, you know, 12 hours

difference in terms of time zones.

Is that the case or are you mostly
kind of like working with people

that are closer to your time zone?

Colin: Yeah, this is something
that's kind of interesting for us.

We do tend to have, like when I, my old
team, I was the only person in the US and.

It would be this thing where like by the
end of my day I try to review any open

prs for Europe so that they're waking up
to a reviewed pr, that then they can, you

know, either make changes or merge and
then on their end, you know, I, on my end

I'm trying to get mine, my code done so
that when they also wake up, they have a

PR for me that they can take a look at.

And sometimes there might be this back and
forth conversation around, Maybe it's a

draft PR with questions because it might
be a newer area of the code base for me.

And so I do have to kind
of think about that.

It really forces me that
time box might work too.

It's like if I don't get to the thing
I'm hoping to today, then I'm not

gonna have the PR open and ready for
review by the beginning of their day,

which means now we're gonna have a
whole nother 24 hour cycle of that.

CJ: right?

I is there Is there like a, an
hour or two where you have overlap

and you can kind of like talk live

and.

Colin: Yeah.

We've got.

In my morning.

I have someone in, in Israel
that I, we actually have a lot

of overlap during the day because
she works really late at night.

And so she kind of has a
different setup of her day.

And so we do get that and like, we just
restructured our team, so we do have more

people in different, like I have more
US folks on my team now, so you know,

that's gonna change a little bit as well.

So, you know, once we do that,
It's nice to have those comments.

I've noticed that, especially because
it's a little bit async, someone might

open a pr and then I guess this is on
the more of the creating the PR side

of it, but they'll preempt the review
by going into the, like the filed diff

CJ: Mm-hmm.

Colin: add their own comments.

So like calling out like, Hey, could you
specifically look at this thing that I

either had a struggle with or like, I
think this is the best way to do this.

You know, can you just
like do a gut check?

. You know, if there's any sort of things
that need to be specifically tested

from a security or like web hook and
like ingest type of thing, then they

might call that out Especially with
integrations, it's like you have to

do this world building again of, of
having the development integration.

So like, you know, maybe it's
developer Twitter account.

Connected to the developer
app versus the staging app.

Versus the production app.

So there's just a lot
of variables at play.

CJ: Mm-hmm.

. So when you're, when you're looking at
security stuff, In the back of your mind,

do you have or Yeah, I guess like in the
back of your mind, do you have certain

things that you look for when it comes to
security or just like generally thinking?

Colin: so if it's general web application
security things, you know, we trust that

the team, you know, can look at those
things and if, if it's not an area of

expertise, we can tag in somebody else.

If it's like something related to
like SOC2 or something like that,

then it might be someone who's.

Specifically like security engineered type
of person that would look at that stuff.

You know, and that's not just at orbit.

That would be like at past companies
where you have the luxury of having

somebody who can specifically do that.

And then you also hopefully have some
of these like scanners and things too

that are running on your, on your.

PR and CI that are looking for
common vulnerabilities and things

like that in the code base as
well, like strong prams and things

like that come to mind for rails.

You know, CORS, stuff like
that, that that'll pop up.

CJ: Yeah, cross site scripting,
vulnerable, like I'm trying to

remember the name of it, but yeah,
there's a bunch of tools where you

can just say like, point this at
my app and like try to run all the

vulnerability scanning things on it.

Yeah.

Cool.

Colin: like one of one of them
was called like Hound, I think.

CJ: Hound interesting.

Yeah, I think like looking, the one big
one that comes to mind is SQL injection

attacks, which really, if people are not
parameterizing or like not sanitizing user

input before they're passing them down
to sql, then you kind of get in trouble.

But yeah, I feel like the rail strong
params pattern has, you know, if

you kind of like stick to the, the
normal patterns, you're usually okay.

But I have seen a couple times where
maybe it's like a search endpoint and

you're writing a custom where clause
that has a bunch of likes and whatever.

If you're not passing like the right
parameterization, then it's easier to get

User input and pass that right down to SQL

Yeah, so, Okay.

Super cool.

What else we got here, I guess?

Colin: What about the
content of the code itself?

So like, let's say now it's
time to jump into the code.

What sorts of things, you know, when
we've talked about this in past episodes,

but like programming is so opinionated,
so how do you approach this when you

know someone spent a lot of their time.

On the code.

And I think as much as we like to
try to disconnect our ourselves from

it, it's something that we've spent a
lot of time on and, you know, by the

time it comes time to ask for review,
hopefully it's quote unquote done.

So how do you kind of approach those
things that might be trade offs or things

that you might, that might stick out to.

CJ: I think going into every PR was an
open mind and trying to come at it from

a, like a perspective where you're.

About what the original author
intended can be helpful.

And then also after you really
understand what they were going for

then maybe start to ask questions.

Like ask leading questions instead
of like criticizing, Right?

So kind of like, Oh, I see that
you were doing it this way.

That's really interesting.

Did you think about this approach?

Or You know was there a reason why you
know, you're taking this in, taking this

as an argument instead of instantiating
it inside of the class or whatever.

Like there's an opportunity
to talk about design patterns.

There's also opportunity to talk
about, like learning the code base

itself as a, as a result of the pr.

So, yeah, I guess maybe to answer your
question, I think coming at it from a

perspective of like, what can I learn
from this code that I'm about to.

Colin: Definitely.

Yeah.

The, the asking questions is interesting
to me cuz I, I do think it's better

than like, making demands or like
saying like, change this to this cuz

you don't necessarily, like you said,
you don't know where, why they had to

make some of the choices that they made.

The asking questions thing I find is
the art piece because it's sometimes

when I'm writing a question on a pr I'm
trying not to be pa like patronizing.

It's like how do we get
to the topic of this?

Like sometimes there's a more direct
way of saying things, but you do wanna

avoid like judgment or assumptions.

And when I've reviewed like a
lot of prs in a row, it starts

to feel a little bit weird.

And this might just be in my head, but
like, it's like rather than me just

like calling it out, it's like, why
don't, what do you think about this?

Or have you tried this?

And things like that.

Because I'm.

In some ways you wanna, you're not trying
to lead them to the right answer, I guess.

It's like, it's not the goal.

The goal is just like to figure out,
like, did they consider something else?

Was there an issue with like, you
know, is there a reason why this

doesn't match the, the code that's
over here that does the same thing or.

Those kinds of things.

And I would say like, if that's the
case, like always ask for clarification.

Like you could just ask a question.

You don't have to be like, I
need to get this review done.

Maybe it's like, Yeah, I'm gonna
need some more info to, to do this.

But how do you feel about that?

Like in terms of like when you've written
something, are questions helpful or do

they, do they feel like that sometimes to.

CJ: So I think they are helpful
in terms of like the ego part of

receiving a code review, right?

If someone is like, Oh, this is wrong.

You should change this, it.

Definitely chips away at like your
identity that is tied inextricably from

the code that you just published on
GitHub and asked for a kind and gentle

review and someone jumps in there
with a bunch of hyperbole and never do

this, and why didn't you just do that?

And you know, this is
overcomplicated or whatever.

Like those kinds of, those kinds of
comments can really like beat you down.

I will say that I have worked.

Several people who do their prs like
that, and they're, they're like insanely

direct, very hyperbolic and like really,
really explicit and d and I don't

know, very blunt about their feedback.

And it was extremely painful, but,
I think I, I also like got better at

like, building up a shield of like,
Okay, I'm publishing this, it's fine.

Like I've gotta get , like
torn up or whatever.

But like I know that at the end of
the day, like I, my hope is that that

criticism is coming in and that I can
receive it and become a better developer.

So like, I always had to like try to
take that perspective as the author

and because I know that was so painful.

and also it required a lot of
like building up this thick skin.

I always tried to like approach PRS that
I am reviewing with that, that own like my

own personal experience and try to like
be really empathetic and really humble

and really like, Okay well let's be.

Let's use this as a teaching
moment instead of like a, just

make sure the code is fine.

Like you have an opportunity to make
other developers on your team know and

understand the things that you know,
and you can do that through the PR

instead of just kind of like, and you
can do it in like a tactful way instead

of just being kind of like, Yeah.

Attacking.

Colin: that you had to go through that
to then find the compassion, right?

Like you would've probably had that
compassion for your reviews anyway,

but like that experience of having
to build that shield, like did that

also cause you to second guess?

Like that it's ready to be puff.

Submitted or like, I imagine like you're
gonna check triple check everything when

you are like, Oh my God, I don't know
what negative feedback I'm gonna get.

Cuz

to me like, that sounds
like an awful reviewer.

Even if it's coming from a good
place, like, you know, maybe they

just were rushed and they shouldn't
have reviewed it like right before

they left the office or something.

But that's, that's a rough experience,
especially for newer developers who

may not know that it's like, this
isn't about not to take it person.

But it is gonna be personal.

Like there's no way to

CJ: Yeah.

Yeah.

The, a couple things came out of it.

One is when I would publish a
pr, I would read all of the code

myself and review it myself.

For myself, like, like I would say,
yeah, before you push the button that

says, you know, submit pr, whatever.

You can see the diff go
through that diff and Polish.

Take your last like chance to polish it
up and anything that I started to build

up this sense of like, okay, I think.

This is the area of code that I'm going
to receive the most criticism about.

And so then like I would go
back and maybe make a little,

like clean it up a little bit.

And when I didn't do that, I always
got comments on those parts where I was

like, Okay, like I think maybe this area
might, you know, need some improvement.

Colin: Like negative reinforcement though.

CJ: yes, , it was, it absolutely

was.

No

Colin: it probably made you a much
better developer through fire.

Right?

It's not not through yeah.

Ooh, that's rough.

CJ: yeah,

Colin: yeah, I, I come from a
background of not always having,

Developers to review my code, and so
I would have to do that anyway, right?

I'm like, I'm literally doing
PRS to myself and then going

and looking at the diff and like
reviewing it like the next day.

So like with fresh eyes.

That was like when I was the
only developer at a company.

You know, I wonder if
that's even a service.

It'd be amazing to like, have
like a contract code reviewer

CJ: Mm-hmm.

Colin: Can you just review my PRS
as an external, you know, person?

But I always had to develop that.

And because of that, I never really got a
lot of those, like those negative reviews.

But because of that too, I never
developed a really strong muscle for

like what to look for in other people's.

And I've had to develop that.

You know, at orbit we have plenty of
developers to review each other's code.

And I've learned a lot on both sides
from reviewing other people's code.

I've learned a lot of things where I'm
like, I didn't know Ruby could do that.

know that Rails could do that.

But then on the flip side, just
seeing like what comments people do.

You know, point out you know,
especially if someone knows the

code base in a different area better
and they're like, Hey, we already

have something like this over here.

you considered reusing this?

Things like that.

So and I think, you know, some of
the, the tips from thought bot when

you're reviewing code, where like
communicate things that you feel

strongly about and those that you don't.

So like maybe there's something else,
like, we really shouldn't merge.

Part as is, or like, Hey, over here
maybe we, we, we should clean this up.

But maybe that's like in a future
pr, like, let's not block this one

and ship because it's not gonna
cause any downstream problems.

But like, we probably need to create
a ticket for like coming back in

and, you know, renaming a bunch
of stuff or something like that.

CJ: Yeah.

I think in the past, what I've done
in that experience is like if I,

if I see something that I don't
think meets our quality bar for

polish , but it's almost there.

Then I will still leave comments about the
stuff that is like insanely nitpicky, but

then in like the overall comment for the
entire PR review, I'll say something like,

left a few nit comments, but it's like,
this is, you know, fine to, to move on.

So yeah, I think that's
pretty clear or, yeah.

when, I guess like in GitHub when you're
reviewing, you can also like approve or

comment and so you could say like, Oh, I
left a few comments and then just comment.

And that is kind of you implicitly
saying that you don't approve of like

where it's at right now and that some
things need to be addressed versus

like, I left a few knit comments, but
then you click the approved button and

so like it's technically approved, but

Colin: Right.

And they can still push up some
more commits and stuff after that.

Yeah.

That's interesting.

Like just you pointing that out.

There is no, technically
there's no decline button.

Right.

It's.

Request changes or approve or comment.

And you know, the comment one and the
request changes are kind of the same

at the end of the day, but one's a
little bit gentler than the other one.

CJ: Mm.

Colin: And I would say, I actually found,
I'll put this in the link to, in the

show notes, we found a, like, I, I've
been having this problem of not knowing

when someone commented on a PR or even
to a review, and I finally figured out

how to configure the GitHub settings.

Shout out to Anthony at
Orbit for like sharing this.

There, I tweeted about it a few weeks ago

CJ: Mm.

Colin: I was like, I keep miss,
like, I don't get an email when

someone comments like, Why am I
not, like all my settings are set.

So now I'm getting notifications in
Slack like directly to me when it's like

someone mentions me in a PR or tags my
team in a pr, things like that, which I

found to be more useful cuz I was like
blocking people without realizing it.

Or I'd be like, Hey, why
is no one reviewed my PR?

And I go look at it and there's a
bunch of comments on it already.

CJ: Mm-hmm.

. Mm.

Colin: told me about those things.

And that's really important
when you have like the request

changes, comment, or approve.

It's just nice to know that like,
hey, it's ready to, to be merged,

or we need some changes there.

GitHub has a pretty nice thing where it's
like you can comment everything and then

you get that like final say, the like,
it's either good to go, looks good to me.

You know, we've got all this
language now around ShipIt and

L G, TM and all these things.

I think ultimately are you, are
you in an emoji and animated gif

in your code reviews person or not?

CJ: Definitely emojis.

I, not as much the animated
gifs, but yeah, I think it.

I'm trying to think back if we were
super into it at previous companies.

I think at App Academy we used
animated gifs a lot in prs,

but not at my VR or Stripe.

So yeah, like but I, I do think
they add a little bit of fun and

flavor to the experience, so yeah.

That's a good call.

Colin: Yeah,

I'll use like the eyeball reactions
on certain, like comments and stuff

to let them know that I'm looking
at it or, or reading it if it's

like not, and I don't even know if
you get notifications for those.

I will use the ship it Squirrel and the
ship in the, in the final PR comment

if I, if I think it's ready to go.

CJ: Yeah, I would say that emojis, so
emojis can also be used for a lot of

good positive reinforcement, which I
do think is another really important

part of reviewing someone's PR is like
going through and saying like, Wow,

this is super nicely organized, or, I
really like how you use this pattern.

Or like yeah, like maybe they.

Not necessarily something clever,
but something that might like, do

a performance optimization that
was kind of like something that

they went over and above to do.

Then I'll drop like sunglasses
face or like, Oh, this is cool.

Or even just like those little things
that that you're calling out that

are, that are good can help, I guess.

Yeah, they, they, they help soften the
blow for any critical feedback that

might come later, but also, You're
giving kudos where kudos are due for

someone, you know, doing great work.

So that's a, yeah, I think that's
also like an important part of review.

So at, at orbit, do you have checklists,
like, do you kind of like have a team

organized checklist that's like, okay,
make sure that you're looking for security

vulnerabilities and is it tested and is
there observability and is there logging

and is there, does it need legal review or

Colin: Yeah, we do have a checklist
in Notion we have like a PR.

Checklist especially, we have a
feature that's coming out that

like has a specific QA checklist
that is in the PR template now.

So like that's still more on the PR side.

Because the person opening the PR needs
to go through, I guess both the reviewer

and the PR before it gets merged.

All the check boxes need to be
checked just just for sanity.

Cuz some of them are more
like regression type things.

We hope to catch in tests, but we don't.

There's just some strange
things that could happen.

But we do have a notion like for
onboarding that people read through.

I wouldn't say like, we have explicit
checklists that, you know, has

to get checked off in every pr.

Especially if it's like, you know,
again, a README may not affect security.

So we're gonna skip that section.

Do you guys have something.

CJ: We, I think each team does
it a little bit differently.

Each team kind of like individually
maintains their process and

their own code ownership.

And yeah, I mean, we do use code
owners to like say which files are

owned by which teams and so people
can automatically get pulled in

if there's changes to their stuff.

. But yeah, like I, I've definitely
seen some teams where it's like,

there's a really explicit checklist
and it's, you have to go through

and make sure there's that.

You think about security and then you
think about tests and then you think

about n plus one queries and then you
think about caching and then you think

about whatever, you know, like yeah.

And like another great one is migrations.

I've been bit a million times, This
is more in like Django land, but

yeah, you're trying to roll out a,
my data migration that's gonna add.

Maybe you're gonna add a new column
that needs an index or something

and like you gotta do all the
different steps that in Rails it's

less painful than it is in Django.

But like sometimes in Django you'd
have to do like several prs where

they were coordinated, where like the
first one does something and then the

second one does something else and
you have to like deploy one and then

immediately deploy the other one.

And yeah.

So I think we kind of built a
pretty strong culture around.

Reviewing data migrations
and yeah, I don't know.

I think it's, it's, it's also
tough to like make sure that you're

hitting all of the right things
that are required for a feature.

Like you know, do we have the right
metrics in place to measure the

success of this thing as it goes out?

Yeah.

Colin: Yeah.

I think other than that checklist,
I'd say to kind of put a bow on this

one, like the big thing is to remember
that the, the person, the the person

on the other end of the review is
human and like they're gonna mix their

identity with the code a little bit.

. And so just keeping that in mind
and, and having compassion and pa

compassionate code reviews is important.

And with that, have you used any of the
tools on like, I think one of the things

that we've kind of talked about here is
like to, for both the PR reviewer and

the person authoring it I've been seeing
a lot more like tooling and processes

for like, Prs that make it easier.

And the one that comes
to mind is graphite.dev.

Have you seen this before?

CJ: I have not seen graphite.

We used a tool called, I think
it was called Reviewable.

Yeah.

We used reviewable.

Colin: The idea behind graphite, and I
think this is similar to like fabricator

at Facebook and some of these other tools
is That like we ran into this issue when

we were building some of our integrations,
we would usually build them as one pr.

They were the most insanely
large PRS you've ever seen.

If people just wouldn't wanna review it
because they were like, I don't like,

Unless you were on the integrations
team, you didn't know how to review it.

what we've moved to right
now is just small prs.

Each PR being kind of like what you just
said, that they're like coordinated.

This is PR one.

Once that lands, we can
then merge two and three.

The problem with that is that you
end up with dependencies on branches

and prs and so graph I aims to solve
that and it essentially has these

like stacks of prs that all end up
rolling up into like one major change.

I've been playing with it.

I'm still not sure how I feel about it.

It feels like something like your
whole team kind of has to use if

you're gonna really dedicate to.

Or maybe you can use it for your
own prs, but it has a really cool

integration with GitHub, whereas
each PR gets added and merged.

Like there's a little running
list that gets added to the GitHub

PR of like, this is one of four.

This one's been merged,
this one's been merged.

And then it auto rebates
all of the upstream onto it.

And this is

CJ: Ooh.

Colin: doing all the get like get does
not translate well to audio at all.

CJ: Yeah,

Colin: when you start talking
about merges and branches.

And Rebasing, but definitely check out
graphite.dev if you have this issue.

There are other tools that are you know,
available if you, if you search for like,

merging and, and code review strategies.

I'm still trying to find that, that
thing that's like lightweight on

the team where you actually can have
branches and prs that are dependent

on previous ones, but not block you
as a developer from keeping, you know,

development, you know, working on it.

You know, doing it where you have
to constantly rebase upstream or you

know, if the thing you need is not in
the branch that you're in right now,

then that, that becomes an issue.

And so little bit off topic for code
reviews, but Graphite's own website claims

that the biggest reason to do this is
that it makes code review even easier

because maybe the migration is in a PR.

The model and the controller are
and there on prs, and then you

have the business logic in its own.

And that way you can also start
getting things into main faster

feature flag things, right?

You're starting to develop around
feature flag development instead

of like, you know, having that big
launch day where you're afraid to

see if everything's gonna land.

CJ: Mm-hmm.

. Mm-hmm.

. Yeah, I, I'm trying to remember
the features of reviewable

that were killer features.

I think many of the ones that we
were using are now rolled into just

like the default GitHub interface.

So yeah, I'm sure there's features,
new features that we, I haven't

seen added, but yeah, tools
can definitely help your flow.

I think we also talked about having
like an automated system for.

Adding comments to the PR that like
you kind of have a bot do a first pass

of a review and make comments about
things like style or, Hey, you know,

at this company we put parentheses when
we make our method calls or whatever.

Like you can kind of apply
those automatically, but.

Colin: Let the, let the bot
do the nitpicky stuff so you

CJ: Yeah.

. Exactly.

Yep.

Cool.

Well let's wrap it up.

We hope you enjoyed this deep dive into
how to review PRS and doing code reviews.

Next time we're gonna start talking
about some developer content creation

and a lot of the workflows that
we use for dev content creation.

Colin: As always, you can head over to
build and learn.dev to check out all the

links and resources in the show notes.

That's all for this episode.

We'll see you next time.

CJ: Bye friends.