We've been reviewing each other's code for years now, and we've developed a pretty strong opinion: most code review advice is about the wrong things. People talk about formatting, naming conventions, and whether you used a ternary or an if-statement. Your linter handles all of that. What actually matters in a review is the stuff that no tool will ever catch.
The review that saved us the most pain wasn't about a typo or a missing semicolon. It was Ștefan pointing out that Robert had written a database query inside a loop that would run on every user request. It looked clean. It passed lint. It would have melted our server at any real scale. That's the kind of thing code review is actually for.
The Stuff Your Linter Can't See
There's a category of problem we call 'polite disasters' — code that is syntactically perfect and logically correct but will destroy you in production. These are the things reviewers need to be actively hunting for.
- N+1 queries hiding inside clean-looking service functions
- Missing error handling on async operations that will silently swallow exceptions
- Race conditions in code that seems sequential but isn't
- Secrets or sensitive data being logged 'just for debugging'
- Assumptions about data shape that will break the moment real users show up
- Missing indexes on columns that will be filtered or sorted in production
These don't look like bugs at first glance. They look like fine code. That's what makes them dangerous, and that's why human review matters. You're not checking grammar — you're thinking about what happens when this runs at 3am with real data.
Ask 'What Happens When This Fails?'
This is the single most useful question to ask during a review. Not 'does this work?' — it works, that's why they opened the PR. The question is: what happens when it doesn't?
Here's a real pattern we've caught multiple times:
// Looks fine at first glance
export async function processPayment(userId: string, amount: number) {
const user = await db.user.findUnique({ where: { id: userId } });
const charge = await stripe.charges.create({
amount,
currency: 'usd',
customer: user.stripeCustomerId, // 💥 what if user is null?
});
await db.payment.create({
data: { userId, chargeId: charge.id, amount }
});
return charge;
}
// What the reviewer should ask for:
export async function processPayment(userId: string, amount: number) {
const user = await db.user.findUnique({ where: { id: userId } });
if (!user) {
throw new Error(`User ${userId} not found`);
}
if (!user.stripeCustomerId) {
throw new Error(`User ${userId} has no payment method on file`);
}
let charge: Stripe.Charge;
try {
charge = await stripe.charges.create({
amount,
currency: 'usd',
customer: user.stripeCustomerId,
});
} catch (err) {
// Stripe failed — don't create a payment record
throw new Error(`Payment failed: ${err instanceof Error ? err.message : 'unknown error'}`);
}
await db.payment.create({
data: { userId, chargeId: charge.id, amount }
});
return charge;
}The first version will throw a cryptic runtime error the first time a user exists but hasn't set up billing. Or worse — it silently creates a payment record for a charge that never went through. The reviewer's job is to think through these paths before they hit production.
Review the Intent, Not Just the Implementation
This is where most reviews go wrong. Someone submits a 200-line PR and the reviewer spends their energy suggesting variable renames and pointing out that the function could be written more concisely. Meanwhile, the entire feature is solving the wrong problem.
Before you look at a single line of code, ask yourself: do I understand what this PR is supposed to do? If there's no description, ask for one. Not as bureaucracy — as a sanity check. We've caught PRs where the author solved a problem that didn't exist anymore, or where two people were working on overlapping things and one of them was about to get their work deleted.
A code review that approves a well-implemented solution to the wrong problem has done active harm. Make sure you understand the 'why' before you dive into the 'how'.
We have a two-question rule before opening any diff: What is this supposed to do? How will I know it's working? If you can't answer both from the PR description and the tests, that's your first comment.
Security Is Everyone's Job
We don't have a dedicated security person. Most small teams don't. That means security review has to happen inside normal code review, and it means everyone needs to have a baseline threat model in their head while reading PRs.
The patterns to watch for aren't exotic. They're the same ones that show up in every breach writeup:
// Authorization check missing — very common in fast-moving codebases
export async function updateUserProfile(
request: Request,
userId: string,
data: ProfileUpdateInput
) {
// Missing: does the authenticated user have permission to update THIS userId?
// An attacker can just change the userId parameter
return db.user.update({
where: { id: userId },
data,
});
}
// Fixed version
export async function updateUserProfile(
request: Request,
userId: string,
data: ProfileUpdateInput
) {
const session = await getSession(request);
if (!session?.userId) {
throw new AuthError('Not authenticated');
}
// Only allow users to update their own profile (or admins)
if (session.userId !== userId && session.role !== 'admin') {
throw new AuthError('Not authorized to update this profile');
}
return db.user.update({
where: { id: userId },
data,
});
}The things to check for on every PR that touches data: Is there an auth check? Is the auth check actually enforced, or can it be bypassed by changing a URL parameter? Is any user-provided input being passed to a database query without validation? Are we exposing more data than we need to in API responses?
None of this requires being a security expert. It just requires slowing down and thinking like someone who wants to break the thing.
The 'Future Maintainer' Test
When you're reviewing code, you're also reviewing it for the person who will have to debug this at 2am six months from now. That person might be you. It's probably going to be you. Code review is how you protect future-you.
There are specific patterns that make future maintenance a nightmare:
- Magic numbers with no explanation ('why is this 86400? oh, seconds in a day — please use a named constant')
- Complex conditionals with no comment explaining the business rule they encode
- Workarounds for external API quirks that aren't documented inline
- Functions that do three things with a name that describes one of them
- TODOs that have no associated ticket or any indication they'll ever be addressed
// Makes the reviewer say 'what is 1209600 and where did it come from?'
const tokenExpiry = Date.now() + 1209600000;
// Makes the future maintainer's life easier
const TWO_WEEKS_MS = 14 * 24 * 60 * 60 * 1000;
// Trial tokens stay valid for 2 weeks to match our trial period length.
// See: https://linear.app/peal/issue/PEAL-142
const tokenExpiry = Date.now() + TWO_WEEKS_MS;
// Also terrible — no context on why this weird condition exists
if (user.createdAt < new Date('2024-03-15') && user.plan !== 'legacy') {
applyDiscount(user);
}
// At least tell me why
// Users who signed up before our pricing change on 2024-03-15 were
// grandfathered into a 20% discount unless they voluntarily upgraded.
// Remove this after all grandfathered users have churned or upgraded.
if (user.createdAt < PRICING_CHANGE_DATE && user.plan !== 'legacy') {
applyGrandfatheredDiscount(user);
}You're not just reviewing for today. You're writing the documentation that will explain this code to someone who wasn't in the room when the decision was made.
How to Give Feedback That Doesn't Start Arguments
We've had reviews that turned into hour-long debates about things that genuinely didn't matter. We've also had reviews where real problems almost got glossed over because neither of us wanted to be the one to say 'this approach is wrong.' Both failure modes come from the same problem: unclear feedback.
The thing we started doing that made the biggest difference: label your comments. There's a big difference between these three types:
- Blocking: 'This will cause a bug / security issue / data loss. Needs to be fixed before merge.'
- Suggestion: 'I'd probably do this differently, but your approach works too. Up to you.'
- Question: 'I'm not sure I understand what this does — can you explain or add a comment?'
Without that label, the author doesn't know if they're being asked to rethink their entire approach or if you're just sharing a preference. Ambiguity turns reviews into negotiations. Clarity makes them fast.
One more thing: when something is genuinely a preference and not a correctness issue, the reviewer should say so and then let it go. Reviewing shouldn't be about making the code look like you would have written it.
What a Good Review Actually Looks Like
We've landed on a rough checklist that we run through before approving anything non-trivial. It's not formal, it's just the set of questions we ask:
- Do I understand what this is supposed to do and why?
- What happens when external dependencies (database, APIs) fail?
- Is there a path where this causes data corruption or incorrect state?
- Is every database query that touches user data properly authorized?
- Will I understand why this code exists in six months without the Slack context?
- Are there tests? Do the tests actually test the important paths, or just the happy path?
- Is there anything here that will get significantly worse under load?
That last one is easy to miss when you're working on a product with a few hundred users. But the time to think about it is before you have a few thousand. A query that takes 50ms against your dev database will take 5 seconds against a table with 10 million rows and no index. A review comment costs nothing. An unplanned migration at 2am costs a lot.
We bake this same philosophy into the templates we ship at peal.dev — the code is meant to be something you can actually maintain and understand, not just something that worked once on a clean machine. Reviewable code is a design goal, not an afterthought.
The best code reviews aren't the most thorough ones — they're the ones that catch the one thing the author couldn't see because they were too close to it. That's the whole value of having another pair of eyes.
If your team's reviews are mostly style comments, your linter isn't strict enough and your review culture hasn't grown into what it could be. Push the style stuff to automation — Prettier, ESLint, whatever — and free up the human review time for the things only humans can catch: intent, edge cases, security, and the general sense that this is going to hold up when the real world shows up.