The five-layer review stack

A pull request is open in front of you: thirty files changed, three other reviews waiting, and the author wants it merged today. Where do your eyes go first, and what are they looking for?

For most people the honest answer is the top of the first file, line by line, flagging anything that looks off. It feels thorough, but it surfaces the cheap stuff first, a stray import the formatter already owns, and spends your attention on the way down. By the time you reach the query on line 200 that quietly dropped its tenant filter, you’re skimming, so the most expensive bug gets the least attention.

This lesson rests on one reframe: a reviewer doesn’t read the file top-down, they read the diff’s concerns top-down. Every change touches something the codebase already decided, like how to scope a query to a tenant, a wrapper that handles auth, or the shape a mutation returns. So the question for each touched concern isn’t “is this line correct?” but “did this change defend the invariant that decision enforces, or punch a hole through it?” You scan the diff once, map each change onto principles and patterns you already know, and the comments that matter write themselves.

This lesson gives you two tools for that: a five-layer stack that tells you where your finite attention goes first, and a map of the principles and patterns to scan each change against.

Review defends invariants; the linter hunts mistakes

Every architectural principle and SaaS pattern this course taught exists to make some failure mode either impossible to represent or guaranteed to be caught. A discriminated union makes a contradictory state unrepresentable, a tenant-scoped query helper makes a cross-tenant read impossible to write by accident, a webhook dedup key makes a double-charge a non-event. These are load-bearing decisions the codebase already paid for.

A review is where those decisions get defended or quietly eroded, and the erosion comes from well-meaning diffs: an author hand-writes a type the schema already defines, reaches past the tenant helper to a raw query, or re-invents a wrapper, because they didn’t know the established surface was there. Catching that is your job.

Mistake-hunting, scanning for typos or a missing semicolon, is the linter’s work, and it is faster and more consistent at it than you will ever be. Noticing that a diff re-implemented something the codebase already guarantees is the human’s work, and nothing else can do it. Spend a review hunting mistakes and you do the machine’s job while leaving your own undone.

The five layers, top-down

Five layers, highest-stakes at the top, run in order on every diff.

1. Correctness and security. Does the code actually do what the PR claims it does? And does it leak a secret, drop an auth check, expose the tenant boundary, log PII, or trust client input the server is supposed to validate? This is the layer where a miss becomes a breach.
2. Architectural principles (#1–#7). Does the diff respect the seven structural rules the codebase is built on: co-location, schema-as-truth, the side-effect boundary, naming for intent, framework conventions over invention, explicit over magic, and impossible states unrepresentable?
3. SaaS patterns (#1–#15). Wherever the diff touches an established surface, does it use it: the tenant helper, the authed wrapper, the billing carve-out, webhook idempotency, the Result shape, URL state, cache decisions, soft delete, the notification layer, the migration cadence, the security baseline, the time and i18n primitives, the performance reach?
4. Tests and contract integrity. Are the new code paths covered? Do the tests assert behavior the user cares about, or just the implementation? Does the TSDoc still match the signature? Did .env.example move when env.ts did?
5. Style and naming polish. The linter already caught the formatting. What’s left is reading names against intent and flagging the genuinely confusing one. This is the last layer. If your review only ever reached here, it missed.

The order is the whole point: the layers are sorted by what a miss costs in production. A dropped tenant boundary at layer 1 is a customer reading another customer’s data, a breach you report to regulators; a confusing name at layer 5 is a five-minute rename someone does next week. Your attention drains as you read, so a reviewer who starts at file one, line one sorts by file position and runs out before the bug, while one who starts at “does this leak a secret or drop a tenant filter” sorts by severity and reaches the bikeshed last, or never.

Most of the work lives in layers 2 and 3, the subjects of the next two sections.

↑ Attention first · severity high

1 Correctness & security

Cross-tenant leak, double-charge, breach

2 Architectural principles (#1–#7)

Structure rots; the next change costs double

3 SaaS patterns (#1–#15)

An invariant the codebase bought, re-broken

4 Tests & contract integrity

A test that passes while the behavior is broken

5 Style & naming polish

A confusing name, fixed later

↓ Attention last · severity low

The review stack, severity high to low, top to bottom. Spend attention from the top.

A coding agent’s PR lands in this same stack: the principles and patterns it checks don’t change based on who typed the keys, so machine-written code gets neither softer layers nor harsher ones.

Each principle’s diff signature and review comment

The codebase rests on seven structural rules, each taught in its own lesson. The table maps every rule to its diff signature, the pattern that signals a change skipped the established way, and the comment you leave to point the author back at the rule. The signature is what your eye learns to catch on a scan; once you spot it, the comment is almost automatic.

Principle	Diff signature	The comment
#1 Co-locate by feature, not layer (Ch29, File-system routing)	A new file landing in /utils/, /services/, or /types/ instead of its feature module	”Belongs in lib/<feature>/.”
#2 The schema is the source of truth (Drizzle chapters, Unit 5)	A hand-written type duplicating a Drizzle/Zod shape; a hardcoded list the schema already enumerates	”Derive it from the schema.”
#3 Pure functions in /lib, effects at named boundaries (Server Actions, Unit 6)	A db.insert or fetch inside a lib/<feature>/ helper; business logic in a route handler	”Move the effect to the action seam.”
#4 Name for intent, not implementation (naming convention)	handleClick, processData, helper, utils, manager	”Name what it means, not what it does.”
#5 Use framework conventions; don’t invent (App Router & Server Actions)	A custom router, a hand-rolled action-call wrapper, a homemade form lib where native forms + Server Actions cover it	”The framework already has this.” (authedAction/authedRoute are the sanctioned carve-outs)
#6 Prefer explicit over magic (boundary directives)	A global side-effect import mutating module state; a magic string driving control flow; a missing 'use client'/'use server' directive	”Name the boundary.”
#7 Make impossible states unrepresentable (TypeScript, Unit 1)	A boolean pair that admits a contradiction; a should-be-required-together optional pair; a string that wants to be a discriminated union	”Tighten the type.”

Principle #7 is worth seeing in code, because its diff signature is a shape rather than a keyword. Here the author models one piece of async state as separate fields, and the type now permits combinations the UI can never render.

The flagged shape

type RequestState = {
  isLoading: boolean;
  data: Invoice[] | null;
  error: string | null;
};

The violation. Three flags give eight combinations, most of them nonsense. What does isLoading: true alongside both data and an error mean? The type allows it; the UI can’t render it.

Tightened

type RequestState =
  | { status: 'loading' }
  | { status: 'success'; data: Invoice[] }
  | { status: 'error'; error: string };

Tightened. One discriminant, three legal states, zero contradictions. The impossible combinations are now compile errors, not bugs to remember.

Two terms recur across the table. Drizzle names the data layer, and a Server Action names the seam for any mutation.

Each pattern’s diff signature and review comment

The fifteen patterns sit in the same three columns: pattern, diff signature, and comment. Each row pins one SaaS concern, so you scan and read it exactly as you did the principles. It’s a reference you return to at a real diff, not prose to read once.

Pattern	Diff signature	The comment
#1 Tenant-aware query (Ch56, Organizations)	db.select().from(table) with no org filter	”Scope it with tenantDb(orgId) in the where.”
#2 Authed action wrapper (Server Actions / RBAC)	A Server Action opening with manual auth() + if (!user) throw	”Wrap it in authedAction(role, schema, fn).”
#3 Seat management (Ch56, Organizations)	Adding/removing members around the seat & role primitives	”Go through the seat primitive.”
#4 Billing carve-out (Stripe billing, Unit 11)	import Stripe outside lib/billing/	”Go through the billing interface.”
#5 Webhook idempotency (Ch63, Webhook ingestion)	A webhook handler that doesn’t dedup on the provider’s event id	”Add the processed_events dedup key.”
#6 Result return shape (Server Actions, Unit 6)	A Server Action that throws on a validation failure	”Return { ok: false, error }; throw only at the framework edge.”
#7 URL-state list view (Ch60, URL-state list views)	Filter/sort/page state in useState instead of searchParams	”Lift the state to the URL (nuqs).”
#8 Cache decisions (caching & invalidation)	A fetch with no explicit cache directive on mutable data; a missing revalidateTag/revalidatePath after a mutation	”Name the cache decision.”
#9 Soft delete / archive (Ch61, Soft delete, archive, concurrency)	A DELETE FROM that should set deletedAt; a unique constraint blind to soft-deleted rows	”Soft-delete, or make the constraint partial.”
#10 Notification layer (notifications, Unit 13)	A direct Resend/Twilio call inside a feature module	”Route it through the notification dispatcher.”
#11 Expand-migrate-contract (migrations, Unit 20)	A NOT NULL on an existing column, a rename, or a drop landing in one PR	”Split it into the three-step cadence.”
#12 Security baseline (the security baseline, Unit 16)	Untyped JSON.parse of a request body; a secret in a client component; a missing mutating-route guard	”Name the baseline being violated.”
#13/#14 Time and i18n (time & internationalization, Unit 17)	new Date() for user-visible formatting in a server component; a hardcoded English string in a translatable surface	”Use the Temporal / Intl / next-intl primitives.”
#15 Performance vigilance (performance, Unit 19)	A list query with no pagination; an N+1 on a relation that has a join helper	”Paginate, or use the join.”

The map tells you what to flag, not which concern wins when a diff trips several at once. That’s the stack’s job, and it’s the skill worth drilling. Sort these diff signatures onto the layer they belong to.

Each item is something you spotted in a diff. Sort it onto the layer whose severity it earns — when a defect could sit in two layers, the higher layer wins. Drag each item into the bucket it belongs to, then press Check.

Correctness & security Layer 1 — a miss here is a breach or a bug

Principle / pattern violation Layers 2–3 — skipped the established surface

Tests & contract Layer 4

Style & naming Layer 5 — last

A process.env.STRIPE_SECRET_KEY read inside a 'use client' file

db.select().from(invoices) with no org filter

A request body passed to JSON.parse with no validation before it hits the DB

A boolean pair that admits a contradictory state

A direct Resend call inside a feature module

A new helper file dropped in /utils/ instead of its feature module

A new Server Action with no test covering its failure path

processData as a function name

Check Reset

Sit with the tenant-filter item. On the map it’s pattern #1, “use tenantDb,” but a missing tenant filter doesn’t just skip a convention: it lets one customer read another’s rows. That cross-tenant breach makes it a layer-1 security finding, not a layer-3 style nudge, because when one defect could sit in two layers the higher layer’s severity wins.

What does not get a comment

Knowing what to flag is half the skill. The other half, the one that separates a reviewer people want to work with from one they dread, is knowing what to stay silent on.

Here’s what you deliberately leave alone:

• Formatting. Biome owns it. A comment about indentation or quote style spends your attention and the author’s on something a tool already settled.
• Naming or casing the linter accepts. If the rule passed, arguing camelCase versus your preference is noise. The codebase has a convention and the linter enforces it.
• Personal style. “I’d have used a ternary.” “I prefer an early return.” These aren’t defects; they’re your hands projected onto someone else’s code.
• Off-topic rewrites that aren’t load-bearing for this PR. “You could’ve used the visitor pattern.” “While you’re here, refactor the export logic.” A real architecture proposal belongs in a design doc, not a line comment on this PR.

One principle sits under all four: the review defends the established patterns, it doesn’t redesign the system or impose your taste. Each item above sits below layer 5 or outside the stack, so the senior move is to read a diff full of things you’d have done differently and comment only on the ones that break an invariant.

Tip

Before you submit a comment, ask which layer it belongs to. If you can’t place it on the stack, you’re leaving a preference, not a finding. Delete it.

When CI already caught it, you don’t

One reviewer reads every PR before you do, never tires, and never disagrees with itself: the toolchain. tsc, Biome, and the test suite run on every push, and anything they catch deterministically is not your beat. A red CI isn’t a review comment; it’s a precondition the author hasn’t met.

A mature codebase keeps promoting checks into CI. You added several while building the app: a type-coverage gate, a migration linter that catches an unsafe NOT NULL, an env-parity check that fails when env.ts and .env.example drift. Each one started as something a human caught by hand.

That is the senior’s long game: when the same comment recurs across PRs, you stop writing it by hand and promote it to a lint rule or CI gate. As the structural net widens your human beat narrows, so the map isn’t a permanent burden; every quarter the machine takes a few more rows, leaving you the judgment calls only a human can make.

Three preconditions for review: size, description, and tempo

Before any of this works, the review has to be possible: the diff readable, its claim understood, your response arriving while the work is still warm. Three disciplines gate that, none of them about the diff’s content.

PR size, and the “split this” comment. Reviewer accuracy doesn’t degrade as a diff grows; it falls off a cliff, and both research and team experience put the edge around 400 lines of meaningful change. Past that you either rubber-stamp the PR or read it and miss the load-bearing issue anyway. Neither is a review, so the right comment is “split this,” and it holds the merge as firmly as a security finding does: an unreviewable PR can’t be reviewed.

The description scopes the review. Before reading a line of the diff, read the description and ask: do I understand what this claims to do, and can I verify it? A one-line description on a 600-line diff asks you to verify a claim you were never told, so your first comment isn’t on line 47, it’s “what does this change, and why?”

Tempo, and the asymmetry. A review that lands a week late is a review that didn’t happen: the author has moved on, the context has evaporated, and the merge goes through under deadline with no review input. So respond within a business day, ideally within hours, and turn small PRs around same-day. A slow review costs more than a fast imperfect one, because the alternative to slow isn’t thorough, it’s nothing.

Watch video

Trisha Gee (Modern Software Engineering) lays out how to decide what to look for: keep the diff small, let tools own what’s automatable, and spend finite attention on what matters (~20 min).

Reading tests with the same eyes as code

Layer 4 has a failure mode of its own: a junior reviewer treats tests as the author’s private business and ships the PR as long as they’re green. But tests are part of the diff, so read them with the same posture as production code.

Ask one question of every assertion: does it check behavior the user cares about, or only the implementation the test was written next to? A test that asserts a mock was called or a spy fired is a snapshot of today’s wiring; it keeps passing no matter how badly the behavior breaks tomorrow. The worst case is a test that mocks the very thing it claims to be testing, proving only that the mock does what you told it to.

The same test, written two ways, makes the point.

Tautological

test('createInvoice saves the invoice', async () => {
  const save = vi.spyOn(invoiceRepo, 'save').mockResolvedValue(undefined);
  await createInvoice(validInput);
  expect(save).toHaveBeenCalledOnce();
});

The violation. This proves only that the action called the function we mocked. It would pass even if createInvoice saved garbage, or saved nothing the user could read back.

Defends behavior

test('createInvoice persists a readable invoice', async () => {
  const result = await createInvoice(validInput);
  expect(result.ok).toBe(true);
  const saved = await listInvoices();
  expect(saved).toContainEqual(expect.objectContaining({ number: validInput.number }));
});

Defends behavior. This asserts what the user cares about: the call succeeded, and the invoice is really there to read back through listInvoices. Break the implementation any way you like; if the outcome breaks, this test fails.

When you see the first shape in a diff, the comment is one line: this asserts the mock, not the behavior, so what would a user notice if it broke?

The five-second layer-1 security scan

Layer 1 sits at the top of the stack, so give it a fixed checklist: five things you scan for on every review, in about five seconds, before anything else. This is a scan, not an audit; the depth lives in the security unit you’ve already worked through.

The five:

1. Secrets on client-bundle paths. A server secret like process.env.STRIPE_SECRET_KEY read from a 'use client' file. That doesn’t keep it on the server; it ships it to every browser that loads the page.
2. Missing tenant filters. A query without the org scope. It overlaps the pattern map, but here it’s a security concern: the consequence is one tenant reading another’s data.
3. Unvalidated user input crossing a boundary. Client input flowing into SQL or HTML with no parse or escape step in between.
4. Auth checks living in components instead of the action seam. A guard in JSX hides a button; it doesn’t stop a request. The real check belongs at authedAction or requireOrgUser, on the server, where it can’t be bypassed.
5. PII in logs. Emails, tokens, and raw request payloads logged where they’ll sit in plaintext in your log drain forever.

The tenant filter and the input validation also sit on the pattern map. Caught here, they’re layer-1 findings that hold the merge: the same defect in two layers takes the higher severity.

Same bar for every author, including the agent

In 2026, a meaningful share of the PRs in your queue were opened by a coding agent, not typed by a person. An agent’s diff goes through the same stack and the same map as anyone’s, no softer and no harsher.

Agent-written code does have a characteristic failure shape, and it maps cleanly onto things you already check. It re-invents a wrapper the codebase already has (principle #5, and half the pattern map). It adds tests that look plausible but don’t exercise the contract (layer 4, the tautological-test problem at scale). It drifts from the conventions written down in AGENTS.md (the in-source discipline from the last chapter).

The trap cuts both ways. Don’t over-tighten on agent code because “the machine should’ve gotten it right”; that’s nitpicking. And don’t loosen on a trusted teammate’s PR because “they know what they’re doing”; that’s how a tired senior ships the bug. The principles and patterns don’t bifurcate by who wrote the code, so neither does the review.

Run the stack on a diff

The skill isn’t any single row of the stack; it’s the order you ask the questions in. The tree below forces severity-first: at each step you choose where your eyes go, and it won’t let you jump to the variable name on line three before you’ve checked whether the change leaks a secret.

Where do the eyes go first?

The eyes don’t go to the top of the file. They go to the top of the stack.

Where to read next

Two references go deeper on the priorities themselves.

Google Engineering Practices — What to look for in a code review

google.github.io

A large org's reviewer guide: design, functionality, and tests before style, the same severity-first ordering as the stack.

SmartBear — 11 Best Practices for Peer Code Review

smartbear.com

The Cisco-study data behind the ~400-line reviewability threshold: defect detection falls off a cliff past it.

The model to leave with is one sentence: a diff is not lines to read, it’s a set of invariants to verify, spent top-down on the stack, never top-down on the file. Next is phrasing the finding so it lands as collaboration.