One Year, and the Lessons Hiding in My PR Comments
shayraOne year, and the lessons hiding in my PR comments
Two commit messages from my git history:
May 2025 — PROJ-15142: fix: optimize an item register screen — Fixed the data table distortion as long as it is not too zoomed in. Also this commit includes a slight change in the proportion of the grid rows.
May 2026 — PROJ-22424: refactor: widen Params constraint to object; drop Record cast in useResource.
Same engineer. One year apart. I don't think anyone explicitly taught me to write commits like the second one — but somewhere between those two messages, a lot of other things changed too.
This post is about those changes. Most of what I learned came from PR review comments I kept getting from senior engineers — the same handful of patterns, repeated patiently across tickets, until they became how I think. I'm writing them down for two reasons: so I remember them, and so the next person joining a frontend team has a head start.
Where I started
I joined my team on May 1st, 2025. I had React experience, but it came from solo projects — codebases where I was the only voice, the only reviewer, and frankly, the only person who'd ever read the code again.
My very first ticket was an ESLint upgrade. I remember thinking, at the time, that it was an oddly humble start — but it turned out to be exactly the right one, because a linter forces you to walk every corner of a codebase you don't yet understand. By the time I shipped it, I'd at least seen the shape of where I was working, even if I didn't yet understand most of what I'd looked at.
The work that followed was more typical — features, fixes, refactors, in a frontend I was still learning the taste of. I look at those early PRs now and I can see exactly what I was: an engineer who knew the syntax of React but hadn't yet learned that a codebase has its own way of doing things, and that working with that way is most of the job. I made things work but I didn't yet know how to make them belong.
Here's what a year of patient reviewers taught me — and what I think will save the next person months if they take it seriously from day one.
Lesson 1: Subtraction is the work
The instinct I came in with was additive. A new field? New component. A new mode in a form? New folder. A new piece of state? New hook.
The feedback that recurred most often, across reviewers and across tickets, was the opposite. Sample commit subjects from my own log, all written in response to review:
- drop confirm form wrapper in ItemSearchModal
- flatten FilterRow folder
- inline Item/Variant mode fields and drop UI component unit tests
- drop redundant container tests
- trim useSearchModalForm to coordinated fields only
Each one of these started as something I had added, that a reviewer asked me to take away. At first I treated each as a one-off correction. After the fifth or sixth, I started to see the pattern: a wrapper that does nothing is worse than no wrapper, because it makes the reader stop and ask why it exists. A folder containing one file is a promise of growth that may never come — and until it does, it's just an extra click. A test that exercises plumbing instead of behavior gives you no signal when it passes, and false alarms when it fails.
The shift in my head was learning to ask, of every new file or abstraction: does this earn its keep? If the only answer is "it might, someday" — that's not enough.
Lesson 2: Derive, don't duplicate
A close cousin of subtraction. When I needed a list of status options for one of the search modals, I wrote a new hook to fetch them. Reviewer comment:
drop useStatusOptions, derive statusOptions from constants
The options already existed, statically, in a constants file. I had built an entire data-fetching layer to produce data I could have computed in one line:
// Before — a hook whose only job was to surface options that already lived in a constants file
const useStatusOptions = () => {
const { data, error, isLoading } = useSWR('/api/options', fetcher)
return {
statusOptions: data ?? [],
isLoading,
isError: error != null,
}
}
// ...plus its tests, its mock fixtures, and a snapshot.
// After — derived from the existing source of truth
import { statuses } from '@/constants/items'
const statusOptions = [statuses.active.label, statuses.archived.label] as const
Two lines changed in the constants file. But because the entire useStatusOptions hook had only ever existed to fetch that one piece of data, the same PR also deleted the hook itself, its tests, and a snapshot — net −231 lines, +7 lines. The biggest cleanups often hide behind the smallest review comments.
Another version of the same lesson came up on a piece of theming work. I had branched on a variant name in a handful of components to pick the right accent color. A reviewer suggested I move the variant-to-theme mapping into a single utility, so the components didn't have to know any variant names at all. The components got simpler. Adding a new variant became a one-line change.
A year in, when I reach for a new hook or a new constant, the first question I ask myself is whether the answer already lives somewhere in the codebase, possibly under a different name. More often than I'd have guessed, it does.
Lesson 3: Separate concerns at the right seam
This one took the longest to internalize, because it requires you to first build the wrong thing well enough to see the seam.
My useSearchModalForm hook started life doing two things: coordinating form fields, and fetching item data when the user typed. It worked. The tests passed. But the tests were also enormous — every form interaction had to set up a mock for the data layer it pulled along behind it.
A reviewer suggested I split data-fetching out of the form-event hook entirely. The shape of the change:
// Before — one hook, two concerns
const useSearchModalForm = () => {
const form = useForm(...);
const [submittedValues, setSubmittedValues] = useState(null);
const { data: items, isValidating, isError } = useItemSearch(
submittedValues != null ? toSearchParams(submittedValues) : undefined,
{ enabled: submittedValues != null }
);
// ...
return { getInputProps, handleSubmit, items, isValidating, isError, isSearched };
};
// After — data-fetching injected from the outside
const useSearchModalForm = ({ searchItems }) => {
const form = useForm(...);
// No useItemSearch. No submittedValues snapshot. No isValidating to surface.
return { getInputProps, handleSubmit, categories, tags };
};
The data-fetching now lives at the call site, where it can be composed however that page needs. The hook returns to being one thing.
The resulting refactor deleted around 400 lines net across the hook and its tests, and the test files visibly shrank — because each one was now testing one thing.
The lesson wasn't "always separate data from UI." It was something more subtle: when a piece of code has two concerns, you can see it by how its tests behave. If the tests need to know about both, the code does too — and that's the seam.
Lesson 4: Don't fight the type system — widen the constraint
This is the most technical of the five, and probably my favorite, because it captures a moment I felt myself stop being a newer TypeScript user.
I had a generic hook with a narrow constraint, and at the call site I was casting to make my types satisfy it. The cast worked. The cast made me uncomfortable. I rationalized it as "TypeScript being annoying":
// The hook
const useResource = <
Params extends Record<string, unknown> = Record<string, unknown>,
>(
basePath: string,
initialConfig?: ResourceConfig<Params>,
) => { /* ... */ };
// The call site
const { activate } = useResource<Record<string, unknown>>(endpoint, ...);
const searchItems = useCallback(
(params: ItemSearchParams) => {
activate(params as Record<string, unknown>); // <-- the confession
},
[activate],
);
ItemSearchParams was a perfectly well-typed object — it just didn't index as Record<string, unknown> because some of its fields were optional. So I'd written a useCallback wrapper whose only job was to launder the type. Then exported the wrapper. Then used the wrapper.
The review comment was simple: widen the constraint.
// The hook — one character of meaningful change
const useResource = <Params extends object = Record<string, unknown>>(
basePath: string,
initialConfig?: ResourceConfig<Params>,
) => { /* ... */ };
// The call site
const { activate: searchItems } =
useResource<ItemSearchParams>(endpoint, ...);
// The wrapper, the cast, and the useCallback are all gone.
Five lines added, seventeen removed.
The lesson — and I think this generalizes well beyond TypeScript — is that a workaround at the use-site is usually the type telling you the definition is wrong, not that the type is wrong. A cast is a confession. Most of the time, the honest fix is one level up.
Lesson 5: Learn the dialect before you write your own
Every codebase has idioms — patterns the team has settled on for problems that come up often. They aren't documented in any one place; you absorb them by reading code and getting your own code reviewed.
Early on I was working on a CSV upload feature that needed to count and display validation errors as the file was processed. I wrote what felt natural to me: nested ifs, intermediate variables, a counter I incremented as errors accumulated. My commit log from that week is honestly embarrassing — "trying hard to fix the error count issue", "debugging error count issue", "go-like error handling" — a string of attempts to wrestle the same problem with the same shape of tool. Conceptually, what I was writing looked like this:
// What I wrote — counting errors imperatively
let errorCount = 0
const validName = validateName(input)
if (!validName.ok) errorCount++
const validEmail = validName.ok ? validateEmail(validName.value) : null
if (validEmail && !validEmail.ok) errorCount++
// ...and so on, with errorCount as the source of truth I had to maintain
A reviewer pointed me at flatMapR, a Result-style combinator that already existed in our utilities folder. Its signature is roughly:
const flatMapR = <Prev, Post, PrevErr, PostErr>(
prev: Result<Prev, PrevErr>,
fn: (value: Prev) => Result<Post, PostErr>,
): Result<Post, PrevErr | PostErr>;
It chains a sequence of Result-returning operations, short-circuiting on the first failure and threading the error through automatically. I rewrote the flow to use it:
// What the codebase already had — Result + flatMapR
const result = flatMapR(validateName(input), (v) => validateEmail(v))
// Errors don't need counting; they're the absence of `ok`.
The intermediate variables disappeared. The error counter disappeared. The bug disappeared too — because the thing I had been trying to "count" was no longer something I was tracking by hand.
The lesson wasn't about Result monads specifically. It was that every codebase has a dialect, and the polite — and faster — thing is to read enough of it to write in it. New patterns are expensive: every future reader has to learn yours. Old patterns are free.
One habit that's worth more than any single lesson
Somewhere around month four or five, I started doing something simple before I requested review on any PR: I'd go through the diff myself, slowly, as if I were one of the senior engineers about to look at it. I'd ask the questions I'd already been asked five times — does this folder need to exist? does this hook do two things? is this constant duplicated somewhere? — and I'd fix what I found before anyone else had to.
The first time a PR came back with no review comments, I was prouder than I should have been. But what really mattered is what happened gradually: the review cycles got shorter, the feedback I did receive got more useful (because the obvious things were already fixed), and the lessons started compounding instead of repeating.
If you're new, this is the single highest-leverage habit I can recommend. You don't have to be a senior engineer to review your own code like one — you just have to be willing to look at the diff for a few extra minutes before clicking "Ready for review."
If you're the next person joining
Here's the short version of everything above, the way I wish someone had told me on day one:
- The reviewers aren't testing you. They're showing you the shape of a codebase you can't see yet. Treat every recurring comment as a free lesson, not a criticism.
- Every new abstraction has to earn its keep. The default answer is no.
- When two things are tangled, look at the tests — they'll tell you exactly where to cut.
- A cast is a confession. Fix the definition.
- Read enough of the codebase to write in its dialect. Inventing is expensive; matching is free.
- Review your own draft PR before requesting review. Five minutes there saves an hour of back-and-forth.
- Write the commit message as if a stranger will read it in a year. They will. It's you.
Closing
I'm grateful to the engineers who took the time to write thoughtful comments on PRs that, in retrospect, deserved much sharper edits. They didn't just point out what was wrong — they kept doing it consistently enough that the patterns became internal.
If you're early in your career, or new to a serious codebase, the highest-bandwidth way to learn is the comments on your own PRs, written by people who already know the territory. Treat them as the curriculum. Most of mine were.
Year two starts now.
Belong is hiring engineers who like working in a codebase where review is the curriculum. If that sounds like the kind of team you'd grow in, take a look at our Engineering Careers page.