Reviewing a diff that spans web, API and schema

A one-layer change is easy to review: read the diff, check the tests. A change that touches the schema, the API and the web app is a different job, because the interesting part is usually not in any one file — it is whether the three agree, and whether they will still agree during the twenty minutes when half of production is running the old version.

Read it in deployment order

Not alphabetically, which is the order your tools will show you. In the order the change reaches production:

1. The migration. It runs first, and it is the only part you cannot undo with a revert.

• Does it drop or rename anything? Then the currently-running code breaks the moment it lands. That needs the expand–migrate–contract sequence, not a reviewer's approval.
• Is a new column NOT NULL without a default on a table that has rows? The migration fails, or worse, succeeds locally on an empty table and fails in production.
• Is there an index for the new query, and will adding it lock the table?
• Can it be rolled back? Ask out loud. Often the answer is no, and that is fine once everyone knows.

2. The schema and shared types. Renamed a field? Both apps must move in the same commit — that is the whole reason the type is shared.

3. The API. The DTO (does it accept anything the server should work out itself?), the service (is the rule here rather than in the controller?), the queries (is ownership in the where?), the error codes.

4. The web app. Does it handle the new error code, and does it still have a default branch? Is the loading state real? Is "use client" where it needs to be and no higher?

5. The tests. Not the count. Is the rule this diff introduces pinned down by a test that would fail if somebody removed the rule?

The questions worth asking every time

Five, and they find more than a line-by-line read does.

Can the old client still work? Somebody has the previous bundle open. If this diff adds a required request field, renames a response field, or changes a type, their next click 500s. Almost every reviewer forgets this on a monorepo, because "we control both sides" feels like an exemption and is not.

Can this be deployed in either order? Web and API deploy at slightly different moments; whichever lands second, the pair must work. If the change only works when both land together, say so in the PR and plan it — do not discover it.

What happens if it runs twice? A double-tap, a retry, a webhook delivered twice.

What happens under two at once? Two customers, two tabs, the same last unit of stock. Look for read-then-write.

What does the customer see when this fails? Not a stack trace, not INTERNAL_ERROR — a sentence in a language they read, and ideally something they can do next.

Things that are specifically full-stack smells

• A price, total or user id in a request DTO.
• An authorisation check after the query rather than in the where.
• findUnique then update on the same row, with a condition in between.
• A "use client" at the top of a page.
• A new environment variable with no .env.example entry — which is how the next person's local setup breaks silently.
• A NEXT_PUBLIC_ variable holding anything you would mind publishing.
• A write with no revalidatePath, when a page shows what was written.
• A date handled with local-time methods.
• A float/NUMERIC column for money.
• A new endpoint with no test, when it changes data.

That list is most of the bugs in the previous lesson, phrased as things to look for in a diff. It is worth keeping.

Let the tools do the mechanical part

{
  "scripts": {
    "check": "npm run typecheck && npm run lint && npm test"
  }
}

Typecheck across the whole monorepo, so a shared-type change fails in both apps at once. Lint. Tests. Run it in CI on every PR so a reviewer never spends attention on formatting or a missing import.

The payoff is not tidiness — it is that a reviewer who cannot comment on formatting will comment on the migration.

The PR description

For a change spanning layers, the description is most of the review. Four parts:

Release a slot when an order is cancelled

A cancelled order was leaving its delivery slot marked full, so a slot could be
permanently lost by one cancellation. Reported by the shop.

- schema: nothing
- api: OrdersService.setStatus now decrements slot bookings on CANCELLED,
  inside the existing transaction
- web: no change

Decisions worth a look:
- Done in setStatus rather than the cancel endpoint, because the payment expiry
  sweep also cancels and would otherwise need its own copy. Two paths to the
  same state now share one function.
- The decrement is guarded with `bookings: { gt: 0 }` so a double cancellation
  cannot take it negative. Second call is a no-op rather than an error, because
  the caller's intent is already satisfied.

Deploy: API only, either order, no migration.

Verified: 3 new tests; cancelled an order twice and confirmed the slot count
went 1 → 0 → 0; ran the expiry sweep and confirmed the same path.

What changed, which layers, the decisions worth arguing about, the deploy constraint, and what was actually run. A reviewer who reads that knows where to spend their attention — and the deploy line is the one people leave out and then need at four o'clock.

Keep it small. A 200-line diff gets read; a 2,000-line diff gets approved without being read, which is worse than no review because now two names are on it. Split a mechanical rename from a behaviour change: one is skimmable and the other is not, and mixed together neither gets the right attention.

Comments that land

Say the consequence, not the verdict, and label the severity:

blocking: this migration drops `total`, so every pod still running the old
build 500s until the rollout finishes. Add the new column, backfill, then drop
in a follow-up?

question: if the webhook arrives twice, does the second one decrement stock
again? I could not see a guard.

nit: `amt` → `amountPaise`, to match the rest. Not blocking.

blocking, question, nit — three words that remove most review friction, because a reviewer who does not label severity leaves the author guessing which of eight comments must be fixed.

Ask rather than assert when you have not read everything. "If the webhook arrives twice…" is a better comment than "this is not idempotent", and half the time the answer teaches you something about the system.

Say what is good, specifically. Not "LGTM". "Putting this in setStatus so the expiry sweep gets it for free is the right call" tells the author which instinct to repeat — and that is the only part of a review that makes the next PR better rather than this one.

Receiving one

Every comment is about the code. If a reviewer misread it, that is usually a fact about the code, not about the reviewer — the fix is a clearer name or a comment saying why, not a reply explaining it. Answer the ones you disagree with; "I chose this because…" is legitimate, and sometimes the reviewer changes their mind.

Push fixes as new commits so the reviewer can see what changed, rather than force-pushing a rewritten branch mid-review.

Review your own diff first

The cheapest review there is, and the one people skip. Open your own PR on GitHub before requesting anybody — not in your editor, where you have been looking at it for three hours.

You will reliably find a console.log, a commented-out block, a file you did not mean to include, a migration you generated twice, an .env value you hard-coded while debugging, and something you would be embarrassed to have someone else point out. Ten minutes, and it makes the human review about the design instead.

Read the diff, not the files. git diff main is what the reviewer sees.

Check your work

The order to read a full-stack diff: deployment order — migration, shared types, API, web, tests.

Why the migration first: it lands first and a revert does not undo it.

The five questions: can the old client work, can it deploy in either order, what if it runs twice, what under two at once, what does the customer see on failure.

Why "we control both sides" is not an exemption: somebody has the old bundle open.

Why CI matters to review quality: a reviewer who cannot mention formatting will mention the migration.

The four parts of a description: what and why, which layers, the debatable decisions, deploy constraint and verification.

Why split a rename from a behaviour change: one is skimmable, the other is not.

Three labels: blocking, question, nit.

Why ask rather than assert: you have not read everything, and the answer often teaches you something.

What it means when a reviewer misreads your code: the code needs to be clearer.

Practice

1. Open a PR for your last Kirana Store change and read it in deployment order. Note where you were tempted to start.
2. Take the five questions to that diff and answer each in writing.
3. Find a migration in your history that drops or renames something. Write out the expand–migrate–contract version.
4. Write a migration adding a NOT NULL column with no default to a table with rows. Read the error.
5. Add npm run check and wire it into CI. Confirm a broken shared type fails both apps.
6. Write a description with all four parts, including the deploy line.
7. Go through the full-stack smells list against your own repo. Count the hits.
8. Deploy the API without the web app, or the reverse, and see what breaks.
9. Rewrite a blunt review comment as a consequence plus a question, with a label.
10. Review a friend's PR — or an open one in a public repo — in deployment order.

That is the course. The Kirana Store works, it is deployed, it has tests, and you now have the habits that let somebody else change it without being afraid — which is the part of the job nobody interviews for and everybody needs.