We had a PR once where the code was perfectly formatted, passed all lints, had green tests, and was approved in under five minutes. Three weeks later it caused a subtle data corruption bug that took us two days to trace. The review caught zero issues. The linter caught zero issues. And yet, the bug was completely obvious in retrospect — a mutable default argument pattern that nobody thought to question.
That's the problem with code reviews that focus on syntax: they give you a false sense of security. You feel like something was reviewed. But formatting and style are the least interesting things about a PR. Your CI pipeline should handle those. A human reviewer's time is too expensive to spend arguing about semicolons.
What You're Actually Reviewing For
When we review each other's code at peal.dev, we're asking roughly four questions: Does this do what the author thinks it does? Does this hold up under weird inputs or concurrent requests? Will the next person who touches this understand it? And — the one people skip most often — does this actually need to exist at all?
The last question is brutal but important. The best code review we ever did resulted in deleting 200 lines of new code because the feature could be solved by a config change. The author wasn't happy for about five minutes, then deeply relieved.
Correctness: The Obvious One Everyone Still Misses
Correctness sounds obvious. But it's surprisingly easy to read code and think 'yeah that looks right' without actually tracing the logic. Here's a pattern we see constantly:
// Looks fine at first glance
async function getUserWithPosts(userId: string) {
const user = await db.query.users.findFirst({
where: eq(users.id, userId)
})
const posts = await db.query.posts.findMany({
where: eq(posts.authorId, userId)
})
return { ...user, posts }
}
// The problem: if userId doesn't exist,
// user is undefined and you're spreading undefined.
// TypeScript won't catch this if your return type is inferred.
// And 'posts' still runs even when user doesn't exist.
// What it should look like:
async function getUserWithPosts(userId: string) {
const user = await db.query.users.findFirst({
where: eq(users.id, userId)
})
if (!user) return null
const posts = await db.query.posts.findMany({
where: eq(posts.authorId, userId)
})
return { ...user, posts }
}When you're reviewing, mentally trace the unhappy path. What happens when the database returns null? What happens when the array is empty? What happens when this runs concurrently for 100 users at once? These scenarios are where bugs live. Not in the happy path, which the author definitely tested.
Security: The One Developers Are Weirdly Embarrassed to Raise
We've noticed a social dynamic in code reviews where people feel awkward flagging security issues, like they're accusing the author of being careless. Get over that. Security issues in PRs are infinitely cheaper to fix than security issues in production.
The things to look for aren't exotic. They're the same five or six issues that appear over and over:
- Authorization checks missing or in the wrong place — 'can this user actually access this resource?' should be answered in the code, not assumed
- User input going into queries, file paths, or shell commands without sanitization
- Sensitive data (tokens, passwords, PII) being logged, returned in API responses, or stored in places it shouldn't be
- Rate limiting missing on endpoints that do expensive operations or accept credentials
- Secrets hardcoded or committed — yes, still happens in 2025
// Common authorization mistake — fetching by ID without ownership check
export async function GET(req: Request, { params }: { params: { id: string } }) {
const document = await db.query.documents.findFirst({
where: eq(documents.id, params.id)
})
return Response.json(document)
}
// Any authenticated user can fetch any document by guessing an ID.
// The fix:
export async function GET(req: Request, { params }: { params: { id: string } }) {
const session = await getSession()
if (!session) return new Response('Unauthorized', { status: 401 })
const document = await db.query.documents.findFirst({
where: and(
eq(documents.id, params.id),
eq(documents.userId, session.user.id) // ownership check
)
})
if (!document) return new Response('Not found', { status: 404 })
return Response.json(document)
}Failing to add an authorization check is not a 'style preference'. It's a bug. Treat it like one in your review comments.
Maintainability: Thinking About the Person Who Reads This in 8 Months
That person is probably you. And you will have no memory of writing any of this. We've learned this the hard way opening files we wrote six months ago and genuinely not understanding what we were thinking.
Maintainability isn't about comments everywhere — over-commented code is its own problem. It's about whether the code communicates its intent clearly. Some things to flag in reviews:
- Functions that do more than one thing — if you can't name it without using 'and', it should probably be two functions
- Magic numbers or strings that appear without explanation — what is 86400 and why? (Seconds in a day. Put it in a named constant.)
- Business logic buried inside utility functions or vice versa
- Boolean parameters that change function behavior — almost always better as two separate functions or an options object
- Deeply nested conditionals that could be flattened with early returns
// Hard to maintain
function process(user: User, type: boolean, flag: boolean) {
if (type) {
if (flag) {
// do thing A
} else {
// do thing B
}
} else {
if (flag) {
// do thing C
} else {
// do thing D
}
}
}
// Better — even if it's more lines
type ProcessOptions = {
sendNotification: boolean
overrideExisting: boolean
}
function processNewUser(user: User, options: ProcessOptions) {
// clear, readable, each branch obvious
}
function processExistingUser(user: User, options: ProcessOptions) {
// separate concerns, separately named
}Performance: Not Premature Optimization, Just Not Stupid Optimization
We're not saying you should profile every PR. But some performance issues are so obvious you'd feel bad not catching them. N+1 queries are the classic one — calling the database inside a loop because it was easiest to write that way.
// Classic N+1 — one query to get posts, then one per post to get the author
const posts = await db.query.posts.findMany()
const postsWithAuthors = await Promise.all(
posts.map(async (post) => {
const author = await db.query.users.findFirst({
where: eq(users.id, post.authorId)
})
return { ...post, author }
})
)
// Better — let the DB do the join
const postsWithAuthors = await db.query.posts.findMany({
with: {
author: true
}
})Beyond N+1, things worth flagging: loading entire tables when you need a count, missing pagination on endpoints that could return thousands of rows, synchronous operations that should be async, and large computations happening on every render instead of being memoized or moved to the server.
How to Actually Give Review Feedback Without Creating Drama
The technical stuff is only half of code review culture. The other half is how you communicate. We've been on both sides of reviews that felt like personal attacks and reviews that somehow managed to be brutally honest without any hard feelings. The difference is almost entirely in phrasing.
A few things that actually work:
- Separate bugs from preferences — 'this will crash when X happens' is different from 'I'd probably extract this into a helper'. Label which is which.
- Ask questions instead of issuing corrections — 'what happens if userId is undefined here?' makes the author think through it rather than just accepting your fix
- Prefix nit-picks with 'nit:' so the author knows it's not blocking — saves time negotiating what's actually important
- Acknowledge good stuff explicitly — 'nice, I didn't know you could do that' isn't wasted time, it's how teams actually learn from each other
- Be explicit about what you want — 'needs changes before merge', 'just thinking out loud, you can ignore', 'please fix or explain why not'
The goal of a code review is a better codebase and a better team, in that order. If your comment improves the code but makes the author not want to show you code again, you've net negative'd the team.
The Stuff Nobody Teaches: Reviewing the PR Description
If the PR has no description, or just 'fixes bug', that's a review comment before you even read the code. A PR description isn't bureaucracy — it's context for the review. What problem does this solve? Why this approach and not another? What did you test? Is there anything you're unsure about?
We've found that authors who write thorough PR descriptions catch their own bugs before review. Explaining what you did in plain language forces you to actually think it through. It's like rubber duck debugging but the duck gives you feedback.
Similarly, if a PR has no tests and the code is doing something non-trivial, that's worth raising. Not 'you must write tests for everything always' — we're pragmatic, not dogmatic — but 'how are you confident this works?' is always a fair question.
Building the Habit, Not Just the Checklist
The checklist approach to code reviews only works for a while. Eventually you need to develop an intuition — that feeling of reading code and having something feel slightly off even before you can articulate why. That intuition comes from reviewing a lot of code and caring about the outcome.
A few practices that helped us build that:
- Review PRs in one sitting when possible — context switching kills the mental model you build while reading
- Actually check out the branch and run it locally for anything non-trivial — reading diffs is not the same as seeing it work
- When you find a bug in review, trace backwards — how did this happen? Is this same pattern elsewhere in the codebase?
- Keep a mental (or literal) log of bug types that come up repeatedly — that's your team's specific checklist
We've built these habits into how we work on peal.dev's templates too — every template goes through review not just for correctness but for the kind of decisions a developer inheriting this code would need to make. Things like: why is auth handled here and not as middleware? Why this caching strategy and not another? Those decisions should be obvious from the code or explained in comments. If they're not, that's a review issue.
The meta-point is that code review culture isn't really about any single PR. It's about what your team's codebase looks like in two years. Every review you do is either raising or lowering that baseline. The syntax stuff is table stakes. The logic, security, maintainability, and communication — that's where it actually matters.