►
From YouTube: Code Review Weekly Workshop - Sep 23, 2022
Description
In this session, we discuss a number of issues pertaining to reviewing code at GitLab.
A
A
This
Mr
was
doing
something
really
interesting
where
we're
trying
to
fix
this
flashing
issue
with
the
drawer
component
on
Safari
was
like
Edge
case
browser
specific,
weird
stuff.
I,
don't
know
if
you
remember
this,
but
the
proposed
fix
was
adding
this,
which
is
also
like
man.
This
is
real
kind
of
we're.
We're
knee-deep
and
I
know
figure
out
how
we
can
get
things
just
working,
and
it's
strange
that
we
would
have
to
add
this.
A
A
Luckily
I
went
through
that
process
because
I
did
run
into
wow.
For
some
reason,
this
is
causing
an
unrelated
like
styling
issue,
with
drawers
where,
when
you
have
that
translate
thing,
you
see
it
adds
this
scroll
bar
to
the
bottom,
and
so
it
creates-
and
this
is
happening
for
all
browsers-
not
just
Safari.
A
What's
the
worst
case
situation
that
can
happen
and
just
testing
it
out,
because
when
we
don't
totally
understand,
what's
the
change,
we're
making,
sometimes
there's
unintended
side
effects
and
those
things
pay
off,
and
even
if
I
didn't
catch
anything
chances
are
you
know
just
getting
in
that
habit
and
that
discipline
is
worth
it
as
well,
because
when
there
is
an
issue,
ideally
we
catch
it
before
users
start
seeing
it
so
I
thought
this
was
interesting
development
in
this
Mr
and
just
wanted
to
encourage
everybody.
It
pays
off
to
smoke
test
things.
C
A
That's
a
really
good
question,
so
I
I
think
I
kind
of
just
passed
it
off
back
to
Simon,
and
so
this
does
fix
the
Safari
issue,
but
it
does
seem
like
it
introduces
a
new
issue
and
now
there's
at
one
level
we
would
decide
is
fixing
the
Safari
issue
worth
introducing
this
new
one
based
on
severity
of
the
new
one.
The
severity
of
the
Safari
one
and
by
severity
is
like
those
are
very
soft
fuzzy
words,
but
then
I
think
now
Simon's
gonna.
A
Like
hacky
thing
we
could
add
on
top
of
translate
Z
to
like
you
know,
we
need
translate
Z
for
some
sort
of
weird
Safari
reason,
but
to
get
it
to
stop
it
from
doing
these,
other
things
we'll
add
more
rules
onto
it.
So
chances
are.
That's.
That's
an
approach
that
could
be
taken,
but
now
it
is,
it
is
kind
of
in
a
balancing
game
of
the
amount
of.
A
Because
these
are
kind
of
maintainability
costs,
the
hacks
that
we're
adding,
and
so
the
amount
of
costs
we
want
to
take
for
this
approach
is,
is
interesting
and
but
yeah
it's
not
great.
It
doesn't
look
great
for
git
lab
when
you
do
have
just
a
by
default.
Safari
user
running
into
issues
like
this
so
I'm
it
is
on
Simon's
plate,
so
I'm
curious
to
see
what
he
thinks.
A
A
E
A
At
the
same
time,
too,
is
like
if,
as
an
author
like
self-testing,
your
own
stuff
is
always
even
for
myself,
like
for
everybody
like
there's
blind
spots
too
I
wrote
the
code
and
now
I'm
testing
it
like
you're,
just
gonna
Overlook.
For
some
reason
you
end
up
just
it
helps
to
have
the
impartial
viewer
run
through
it.
Whoever
that
is-
and
so
QA
is
a
really
great
term
to
throw
around,
because
who
is
responsible
for
that
and
I
think
we
do
expect
a
level
like
you
see
these
screenshots
that
Simon
included.
A
So
there
is
a
level
of
QA
that
happened
here,
but
how
far
does
that
need
to
go?
It's
a
really
great
question,
and
this
touches
drawers
so
I
would
say
anything
that
has
touches
a
drawer.
It's
worth
just
you
know
smoke
testing
it
does
this
look
okay
and
when
I
say
drawer,
that's
just
a
gitlab
UI
component,
the,
and
so
it's
really
on
the
reviewer
side
too,
of
we
review
the
code,
but
also
the
functional
Integrity
of
it
as
well
like.
A
A
A
Thanks
so
much
well,
you're
asking
a
really
great
question,
because
this
is
a
very
specific
task
of
I
want
to
QA
something
that
is
touches
the
front
end
like
I.
Imagine
you're
almost
picturing,
like
a
checklist
of
like
let's
test:
does
it
work
in
Safari?
Does
it
work
in
Firefox
is
work
in
Chrome
and
the
answer
to
that
is:
no.
We
don't
have
like
a
task
list
for
that
activity.
A
Think
putting
human
discretion
in
the
loop
is
important
for
that,
but
it
is
very
likely
that
that
would
be
something
worth
elaborating
of
just
a
guideline
for
how
to
how
things
to
think
about
when
you're
wanting
to
smoke
tests
like
I,
just
like
I
just
did,
if
I'm
wanting
to
smoke
tests
just
from
the
front
end
hey
this
is
this
is
reasonably
okay.
A
guideline
of
steps
to
take
is
actually
a
really
great
idea.
I'm
gonna
I'm
gonna
write
it
down.
I
need
to
write
it
down
or
I'll.
A
D
C
C
F
A
A
And
but
where
things
get
really
challenging
is
the
the
devil
is
in
those
details
of
when
all
the
page
is
interacting
with
each
other,
when
all
the
CSS
from
the
page
like
that
causes.
So
that
was
one
other
really
surprising
thing
with
this.
I
could
have
just
tested
this
just
within
the
gitlab
UI
storybook
I.
Don't
want
to
run
into
this
for
some
reason.
A
We
do
have
like
a
storybook
on
the
gitlab
project,
but
Our
intention
is
to
try
to
create
components
that
we
can
create
these
storybook
snapshots
of
rather
than
like
taking
snapshots
of
like
a
whole
capybara
page,
because
those
end
up
those
those
will
end
up
having
so
many
false
negatives,
because
so
much
changes
with
like
each
incremental
Chrome
version
that
gets
run
and
like
we.
When
we
explored
this
in
the
past,
it
wasn't
the
cost
for
maintain
running
and
maintaining
that
like
page-wide
regression,
testing
was
not
gonna.
A
The
cost
for
that
wasn't
outweighing
the
benefit,
and
we
were
really
interested
in
trying
to
isolate
this
at
the
component
level,
but
there
might
be
more
cost-effective
ways
to
do
this
regression
testing
and
that
we
just
we
need
to
explore.
So
it's
it's
a
good
thing
to
keep
thinking
about
of
like
how
could
we
have
automated
catching
this?
It's
a
really
good
thing
to
think
about.
B
A
A
So,
even
though
we're
like
sliced
to
a
specific
group
bring
up
to
the
manager
and
PM
of
like
hey,
we
should
really
set
aside
time
to
do
this.
That
will
help
our
group
and
all
of
git
lab
and
that
those
things
get
prioritized
and
it's
really
good
to
see
those
initiatives
happen
so
doing
an
investigation
of
using
one
of
those
tools
or
something
like
that
would
be
really
cool.
E
E
E
So
if
I
get
an
MR
which
I
see
the
Lord
backend
changes
and
there
is
a
front-end
label
and
I,
don't
think
that
I'll
ask
questions
I'll
see
that
it
runs,
but
I
don't
think
I
am
good
enough
to
actually
review
it
like
I'm,
not
kid
I
would
say:
I,
don't
know
what
you
were
to
use
yeah,
so
sometimes
I'm,
just
forced
to
like
approve
it,
but
I
know
a
50
percent.
In
my
end
that
I'm
not
100
satisfied
with
the
way
I
have
reviewed
it.
So
what
do
we
do
in
such
cases?.
A
A
A
A
So
in
those
situations,
and
that
will
definitely
come
up,
I
mean
that's.
Why
we
we
do
have
both
the
front
end
and
back
end
review
and
I
am
it's
okay
for
me
to
identify.
This
is
outside
my
area
of
expertise,
and
so
I
want
to
make
sure
that
someone
that
this
is
inside
their
area
of
expertise
has
reviewed
and
approved
it.
A
So
looking
back,
I
was
like
I
guess
that's
what
we
do
I've
seen
this
before,
but
this
is
the
back
end
maintainer
at
Tien
I,
don't
know
how
to
say
that
name
and
he
approved
it
and
I
was
like
okay.
This
is
inside
his
domain.
He
knows
what
he's
doing
so.
I,
don't
I'm
gonna
keep
an
eye
on
it,
but
I
don't
have
that's
not
necessarily
I
will
prove
it.
Knowing
that,
like
almost
delegating,
like
my
trust
to
that
expert,
does
that
make
sense.
D
D
E
E
Yes,
so
like
Emma's
like
such
a
miles,
I
just
put
it
on
the
back
burner
for
late
and
then
I'm,
just
not
like
heavily
doing
it,
because
it's
time
to
do
this,
this
is
an
MR
which
I
see
only
has
like
kind
of
back-end
changes
and
they're.
Very
less
I
would
say,
like
I
saw,
I
was
okay,
so
there
are
no
just
a
hammer
in
it.
So.
C
D
A
And
you
don't
you
don't
have,
and
so
I
would
say
the
way
our
processes
is.
You
are
not
you're
not
being
asked
to
review
the
back
end.
You
should
just
be
concerned
about
the
changes
to
the
HTML
Hamel
side
from
the
even
front-end
perspective,
so
like
Hamill's,
interesting,
where
you
do
have
both
worlds
clashing,
often
where
you
have
not
clashing,
but
you
have
the
back
end
and
the
front
end
at
the
same
time
and
yeah.
A
So
from
that
perspective,
I
would
I
would
if,
if
there
were
like
heavy
rails,
back-end
stuff
happening
on
the
Hamel,
that
I
didn't
feel
like
I
was
comfortable
with
I
would
approve,
saying,
like
hey
from
the
front
in
perspective.
We're
not
changing
anything
here.
So
I'm
approving
this
is
good,
but
can
can
someone
from
the
back
end
just
confirm
just
double
confirm
these
changes
and
and
I
think
that
the
backend
reviewers
are
also
tasked
with
reviewing
the
back
end
change
of
the
handle,
so
I
think
your
responsibility
here
is
almost
like.
A
E
But
I
also
feel
guilty
about
not
like
testing
these,
like
these
kind
of
Mrs,
like
when
I
just
check
out,
for
example,
I
got
a
security
camera
which
I
had
no
clue
about,
and
I
was
like.
I
was
like
what
do
I
do
with
this
I
had
to
mirror
the
the
repo
again
and
I
ended
up
just
approving,
because
I
asked
questions
and
I
just
got
answers
like
correctly
yeah
I
know,
so
is
it
okay
to
prove
it
like
that
I
just
feel
I'm
not
doing
Justice
to
the
new
method,
then
I.
E
D
A
A
Let's
collaborate
and
ask
questions
about
it,
and
it's
really
like
that's
the
most
important
thing
that
can
be
done,
but
then
also,
as
you
start,
to
become
more
and
more
familiar
with
it.
Then
you
can
identify.
You
know
the
issues,
maintainability
issues
or
even
user-facing
issues
that
might
come
up
with
certain
approaches
so
that
that
is
ex
like
when
you
first
start
reviewing
Mrs.
A
That
will
be
probably
difficult
to
to
find,
but
you
may
see
a
get
you'll
see,
you'll,
see
maintainer
comments
after
your
approval,
yeah
and
yeah
it's
over
time.
You
would
want
to
try
to
see
like
oh
the
maintainer
found
this
like
that's
something
I'm
gonna
register
in
the
back
of
my
head
of,
like
oh
I,
gotta,
be
looking
up
for
this
thing
and
there's
a
lot
of
those
things.
So
it's
it's
also
just
about
getting
a
deeper,
deeper
understanding
of
how
we
View
and
look
at
these
things,
but
yeah
from
from
this
Mr
yeah.
A
A
Some
understanding
of
both
sides
of
the
stack,
because
we
do
want
to
understand
hey,
does
the
back.
How
does
the
back
end
work
with
the
scope
of
the
front
and
changing
changes
that
are
happening
and
how
does
the
front
end
working
with
the
scope
of
the
back
end,
changes
that
are
happening
so
keeping
that
context
in
mind
is
really
helpful,
so
I,
even
though
I
was
kind
of
joking
about
oh
this,
isn't
our
problem?
Don't
worry
about
it.
A
D
C
It
basically
says
that
if
you're
not
comfortable
reviewing
a
change
because
it's
a
different
language,
because
it's
like
a
little
bit
over
your
head,
then
basically
don't
be,
don't
feel
afraid
to
ping
someone
for
the
review,
because
you're
not
comfortable
doing
it
yourself
and
I
would
recommend
that
as
well,
because
at
the
end
of
the
day,
if
you
approve
something
and
it
causes
some
drama,
it
will
be
your
name
on
it.
It
was
approved
by
you
and
you
don't
want
that,
because
if
you're
not
sure
that
you're
happy
with
the
code
so.
D
C
And
also
I
think
don't
quote
me
on
this
particular
thing,
but
I
think
I
read
that
the
the
files
that
still
live
in
the
Ruby
on
Rails,
even
the
front-end
stuff,
that
lives
in
Ruby
on
Rails.
It
still
belongs
to
the
back
end
reviewers
and
not
the
front-end
reviewers
I
might
be
wrong,
but
I
feel
like
that's
what
I
read
so.
A
A
That's
an
awesome
thing
to
do,
and
we
do
that
in
front
end,
all
the
time
of,
because
the
front
ends
and
back
into
like
the
Scopes
are
so
large
that
sometimes
there's
like
a
really
detailed
Apollo
change
and
I'm
like
let's
see,
if,
let's
add
this
to
Natalia's
Cube
she's
gonna,
be
able
to
review
this
well,
but
to
what
you're
saying
about
the
I
think
the
haml
files
are
the
ones
that
are
in
question
of
like.
Is
this
a
front
end
or
a
back
end
asset?
Is
that
Cassie?
C
A
B
A
D
A
B
B
Is
this
job,
basically
that
produces
the
danger
output,
and
one
of
that
part
is
the
Denture
bot.
So
you
just
restart
this
part
and
then
it
should
be
fine.
Niche,
okay,
but
I
need
to
double
check,
because
at
least
that
was
totally
mentioned
in
some
summer
in
the
handbook
that
you
can
be
run,
the
the
job
are.
A
Are
you
all
familiar
with
with
this
thing
yep?
So
if
with
without
waiting
for
the
job
or
whatever,
you
can
also
just
get
a?
Let
me
ping
another
sorry,
let
me
ping
another
front.
End
reviewer
and
I'll
just
highlight
all
the
reviewers
front
ends
and
I'll
spin,
the
wheel.
Oh
nice,
I
forgot
about
that
and
I'll
just
get
get
a
new
one
though
nice
yeah
that
helps
that
helps.
A
A
D
A
Find
it
okay
now
there's
an
issue
to
do
that,
and
that
would
be
amazing
and
it
is
worth
for
all
these.
Like
supplemental
automation,
tasks,
we've
done
to
try
to
improve
our
code
review
experience
at
some
point.
We
really
diverged
from
baking
these
features
into
the
product
to
like
creating
these
Bots,
which
is
a
little
counter
to
her
primary
example
of
wanting
to
improve
our
product.
A
So
that's
that's
a
great
idea.
It
would
be
so
amazing
to
have
a
reviewer,
overlet
and
baked
into
gay
lab
I
wish
I
had
the
issue.
I'll,
probably
I'll,
find
it
I'll.
Add
it
asynchronously
to
the
agenda
cool
yeah
thanks
everybody
for
the
interesting
conversations.
Does
anyone
have
anything
else?
They'd
want
to
add.
A
A
E
E
Yeah
but
then
it's
I
I've
been
watching
the
videos
for
the
past
two
weeks,
but
so
I
just
thought
that
I'd
have
to
jump
in
one
day
and
like
it
is
actually
good
conversations
happening,
and
you
know
you
just
want
to
like
listening
and
taking
part
in
it
so
yeah.
So
what
time
is
it
for
both
of
you
right
now.
D
A
A
A
A
This
was
an
MR
that
I
guess
is
improving
the
accessibility
of
some
of
these
work
item
views
I,
look
at
this
and
I
see
it
MRI
with
10
file
changes
I'm,
like
oh
man,
all
right
here
we
go
but
then
I
see
like.
Oh
it's
just
these
kind
of
changes,
like
that's
great,
so
this
is
really
trivial
of
we're.
Just
changing
some
classes,
or
he
mentions
in
the
Mr
description
like
improved
color
contrast,
the
one
thing
that
I
left
a
comment
about
was
previously.
We
had
these
IDs,
these
HTML
IDs.
A
We
had
them
static
for
every
instance
of
these
components
and
we're
really
really
bad
at
managing
HTML
IDs
in
gitlab
of
you're.
Only
supposed
to
ever
have
one
of
them
globally
in
the
page
ever
and
so
I
hate
this.
But
if
you
do
search
for
like
notes,
there's
three
notes
elements
in
our
page
which
is
like,
so
we
really
need
to.
A
Oh,
we
made
the
change
there,
so
he
opted
to
moving
this
to
a
computed
which
is
totally
fine,
and
so
here's
where
that
ID
goes
and
that
idea
is
needed,
because
all
of
these,
like
accessibility,
attributes,
need
to
point
to
ID.
The
need
to
point
C
is
references
by
ID,
but
that
looks
great
so
now
that
we're
having
a
unique
ID
is:
is
nice,
we've
got
just
a
label
update?
That's
this
is
no
problem.
A
Accessibility
component,
the
accessibility
property-
that's
pointing
to
another
element-
has
that
pointer,
but
we're
not
necessarily
asserting
that
this
is
the
ID
of
that
other
element.
It's
supposed
to
be
pointing
to
that
will
be
kind
of
nice
to
test
for,
but
I,
don't
think
that's
a
blocking
thing,
because
it's
like
hey
we're
we're
improving!
You
know:
accessibility
and
user-facing
stuff.
A
lot
here,
I
think
I'm
just
going
to
leave
this
non-blocking
test
suggestion
of
rather
than
just
testing
just
this
prop.
A
B
A
We,
no
so
that's
that's
Cherry
Picked
of
like
or
not
what
do.
They
call
that
they
call
it
tree
shaped
so
here
because
we're
only
importing
the
debounce
export.
That's
the
only
bit
that
will
actually
get
imported
because
we're
only
importing
just
the
unique
ID.
That's
the
only
bit
that
will
get
imported
in
the
bundle
yeah,
so
there's
low
Dash
is
a
tree.
Shakeable
Library.
D
A
Well,
good
question
and
that's
something
to
think
about,
and
we
have
some
of
these
automated
comments
talking
about
automation
outside
the
product
project.
We
have
some
of
these
automation
comments
that
help
give
us
some
perception
on
that,
and
this
bundle
size
analysis
is
helpful
of,
is
anything
changing
significantly
that
we
wouldn't
expect
like,
if
I
add
an
import.
A
D
A
D
E
Yeah
I
just
want
now
that
we
are
like
looking
at
the
Bible
size.
So
when
I
started
like
working
here,
then
I
had
this
one
question
with
my
buddy,
which
happened
to
be
Kong,
so
we
I
asked
you
know
when
I
first,
when
I
had
so
many
pipelines,
this
was
so
how
much
Focus
do
we
get
give
to
this
bundle?
Analysis
I
mean.
Do
we
ever
say
that
she
like?
A
F
D
A
Yeah,
here's
one
we're
really
interested
in
reducing
the
bundle
size,
I
show
this
I
want
to
show
sometimes
I
look
for
comments.
I'm
thinking
about
a
comment
I
want
to
see.
Do
we
have
like
any
quote-unquote
precedence
for
kind
of
what
I'm
thinking
of
saying
and
searching
for
comments
is
really
helpful,
sometimes
matching
10
000
comments
is
probably
not,
hopefully
find
some
more
specific
thing
to
say
there,
but
this
this
search
works
works
really
well,
I
really
like
it.
A
But
the
answer
to
your
question
specifically
is
yes
like
we
are
concerned
about
reducing
it,
and
sometimes
it's
not
that
sometimes
we
will
increase
it
because
we
need
to
because
we
need
to
do
more
on
the
page,
but
sometimes
it's
increasing
and
we're
not
meaning
to,
and
it's
because
something
happened
and
we're
not
expecting
it
to
increase,
and
those
are
things
we
want
to
be
really
observing
about.
With
all
of
that
said,
this
thing
is
flagged
beta
for
a
reason
and
sometimes
can
give
off
false
positives
and
false
negatives.
A
D
A
A
D
A
Cool,
that's
a
good
question,
but
I
think
this
is
I
think
this
is
good
for
me,
though
I
had
it
I
didn't
test
this
and
I
started
this
conversation.
Talking
about
how
important
it
is
to
pass
things
but
I
feel
like
we
don't
have
to
like
I
know
that
Hong
has
clearly
tested
through
this
and
we're
not
doing
anything
out
of
the
norm
here.
A
F
A
Doing
something
like
somewhat
hacky:
that's
where
I
would
want
to
do
a
smoke
test
for
this
since
we're
just
changing
CSS
classes
and
we
have
tests
for
the
ID
stuff
and
we
have
screenshots
here.
This
has
already
gone
through
other
reviews,
I
feel
okay,
I,
don't
I
think
the
worst
case
scenario
has
already
been
mitigated,
and
so
I'm
not
concerned
about
a
worst
case
scenario
here.
So
I,
don't
I
personally,
don't
feel
like
I
need
to
manually
test
this
I'm
using
discretion.
A
A
A
So
as
like
a
maintainer
I,
don't
necessarily
have
to
do
all
the
work
I'm,
just
making
sure
the
work
has
been
done,
which
is
helps
us
be
efficient,
so
I'm
gonna
prove
oh
I
can't
approve,
because
I
already
approved
I
run
into
that
all
the
time.
When
we
use
the
quick
actions,
it
doesn't
update
the
approval
thing,
that's
so
frustrating
all
right.
We
will
use
the
original
commit
I,
always
like
to
include
a
reference
to
the
Mr
URL,
because
sometimes
sometimes
those
get
lost
but
yeah.
A
A
It's
too
late,
oh
well,
all
right!
Well,
you
guys
just
watched
me
circumvent
our
our
process,
but
that's
okay!
I
can
always
revert
it.
It's
something
bad
happens,
but
I
doubt
this
is
going
to
cause
anything
bad
thanks
for
hopping
on
and
helping
out
I
appreciate
you
generating
lots
of
interesting
discussions
and
contributing
to
it
and
maybe
see
you
all
next
week,
yeah
sure.