The comment that lands

You once left a comment that was right. The query really did skip tenantDb, you saw it, you flagged it. Three days later the thread has six replies, the author has gotten defensive, and the fix still isn’t in. The last lesson made your judgment good. What failed was the shape of the comment.

That last lesson taught where your eyes go and what holds the merge. This one teaches what you write once you’ve decided to comment: a small, repeatable shape the author acts on in seconds instead of one that burns thirty minutes of thread. You’ll leave with a four-part anatomy every comment fits and a five-label vocabulary that tells the author at a glance which comments hold the merge. Then we flip to the side you’ll spend far more of your career on, receiving the review.

A comment is a message with a severity header

Start from the author’s seat, since that’s whose time the comment spends. You drop eight comments on a PR. The author’s first job isn’t to fix anything; it’s to triage. Which hold the merge? Which are polish they can take or leave? Which are questions waiting on an answer? Until they’ve sorted that, they can’t plan their next twenty minutes.

You already triage a log without reading it: your eye lands on ERROR versus DEBUG and you know how much to care before reading the message. A review comment should work the same way: the author triages it from its first token, the severity label, before reading the body.

blocking: this query goes straight to the table, so it isn't scoped to the org and can read

suggestion: you map then filter in two passes here; a single reduce would do it in one and

question: is there a reason this internal-only path skips the wrapper? if it's reachable from a

praise: good call keeping the helper on the internal path even where it was tempting to skip,

The author triages on the pill, before reading a word of the body.

The four parts of a comment

A comment that lands has four parts, in this order:

1. The severity label. Does this block the merge? One token, triaged before the body. The full set is the next section.
2. The observation. The line or behavior, named concretely. Not “this is wrong” but “this query goes straight to db.select(),” so the author’s eyes land on the exact line.
3. The reason. The principle, pattern, or failure mode it trips, linked to the lesson that owns it. The reason is almost always one map-row away: “this leaks across orgs” is the reason, “see pattern #1” is the link.
4. The action or question. Propose the fix, or ask the question the author answers, then stop. A comment with no ask leaves the author guessing.

These are four slots, not four sentences: a good comment fits all four into one or two. Each part need not be verbose, only present. Here is how they collapse into one real comment.

blocking: this query goes straight to db.select(), so it
isn't scoped to the org — it'll read rows across tenants.
Route it through tenantDb(orgId) (pattern #1).

The severity label. The header the author triages on, before reading a word of the body.

blocking: this query goes straight to db.select(), so it
isn't scoped to the org — it'll read rows across tenants.
Route it through tenantDb(orgId) (pattern #1).

The observation. The concrete thing, named, so the author’s eyes land on the exact line.

blocking: this query goes straight to db.select(), so it
isn't scoped to the org — it'll read rows across tenants.
Route it through tenantDb(orgId) (pattern #1).

The reason and the link. The failure mode plus the map-row that owns the full context.

blocking: this query goes straight to db.select(), so it
isn't scoped to the org — it'll read rows across tenants.
Route it through tenantDb(orgId) (pattern #1).

The action. Propose the fix, then stop. Two sentences, all four slots filled.

← Previous 1 / 1 Next →

Notice what the comment does not do: it doesn’t apologize, pile on context, or ask a rhetorical question. Here are three more in the same shape, with varied severity.

blocking: this throws on a validation miss, but actions
return the Result shape — callers expect { ok: false, error },
not an exception (pattern #6). Return the error instead.

suggestion: this maps then filters in two passes; a single
reduce would do it in one. Not blocking — the current version
is correct, just doing extra work on large lists.

question: is there a reason this internal-only path skips
authedAction? If it's reachable from a request I'd want the
wrapper; if it truly can't be, a one-line note would help
the next reader.

The five severity labels

The vocabulary for that first token is five labels, lowercase, each with a trailing colon, paired here with the question that decides everything else: does it hold the merge?

Label	When to use	Blocks the merge?
blocking:	Must change before merge. Correctness, security, a principle or pattern violation the codebase already paid to establish.	Yes
suggestion:	A strong recommendation: the alternative is genuinely better, but the current code isn’t broken.	No (unless the author agrees in discussion)
question:	You don’t know if it’s right and want the author to explain.	No, on its own
nit:	Non-blocking polish the author may ignore.	No
praise:	Genuine, specific praise for a non-trivial good call.	No

Three of these have a sharp edge worth naming.

question: resolves into an answer, not a fix, and the answer decides what happens next: either the explanation settles it, or your question upgrades to a blocking: or suggestion: now that you understand. It’s a fork in the thread, not a verdict.

nit: is the one to ration. Sometimes a name really is slightly off, but nits drown signal: a PR with twelve nit: comments and one blocking: buries the blocker, and the author can’t tell “this leaks tenant data” from “I’d rename this variable.” Aim for one or two a PR. If you reach for a third, the formatter or linter should probably own that rule instead.

praise: is the most misused. Reflexive praise like “nice work!” is noise, a participation trophy stapled to the top of the review. Real praise names the specific good call: “good call keeping authedAction on the internal-only path, which keeps the pattern uniform even where it’s tempting to skip.” That comment teaches every future reader that the uniform thing was the deliberate thing. Aim for the three a quarter someone remembers, not the thirty rote ones nobody reads.

Where these labels come from

The five labels are a trimmed subset of Conventional Comments , a public standard for putting a parseable label on the front of every review comment.

One difference is worth knowing: the full spec has no blocking label. There, “blocking” is a parenthetical decoration, as in issue (blocking):. This course promotes it to a label of its own, because the merge-hold signal is what the author needs most and a beginner reads a first-class label faster than a parenthetical. So on a team using the full spec you’ll write issue (blocking): where this course writes blocking: — the skill is identical.

Conventional Comments

conventionalcomments.org

The public standard the course's five labels are a trimmed subset of — the full label set plus the blocking / non-blocking decoration.

Drill the label-to-situation mapping until it’s reflexive. Sort each review situation into the label you’d reach for.

Sort each review situation into the label you'd reach for. Drag each item into the bucket it belongs to, then press Check.

blocking: Must change before merge

suggestion: Better, but not broken

question: You need an explanation

nit: Optional polish

praise: A specific good call

A list query reads the table with no org filter at all

An as User cast stands in where a runtime parse should validate the shape

A Server Action throws on bad input instead of returning the Result shape

This loop builds an array that a single .map would express more directly

Two state fields could collapse into one discriminated union, but the current code works

You can’t tell why this path does a manual auth check instead of the wrapper

It’s unclear whether this endpoint is reachable from a request or strictly internal

A variable named data2 would read better as pendingInvoices

A tricky error path still returns the Result shape cleanly instead of throwing

The migration was split into expand and contract steps exactly as the pattern calls for

Check Reset

Blocking or suggesting: decide before you type

The single most useful reviewer habit is deciding before you type the comment whether the line must change.

Blocking is an objective failure. The code is wrong against a decision the codebase already made: the principle violation, the security gap, the contract drift, the test that doesn’t actually test. You’re not stating a preference; you’re showing where the diff broke something that was settled.

Suggesting is a subjective preference. The code works. You’d have factored it differently, reached for another pattern, picked a nicer name. It’s a real opinion, often a good one, but no established decision says the current code is wrong.

When you don’t make this cut, the author has to, and fails one of two ways. They treat everything as blocking, churn the whole PR to satisfy your stylistic asides, and start resenting your reviews. Or they treat everything as optional and merge straight past the one comment that was a real security hole. Either way the signal is gone: you can be right about every comment and still run a useless review, because the author can’t tell your “must” from your “maybe.”

So the discipline shows in the comments that aren’t blocking. Anyone can mark the scary bug blocking:; the skill is marking the suggestion a suggestion and the nit a nit, because that restraint is what makes your blocking: trustworthy. An author who has learned you only write it when you mean it drops everything on sight; one who has watched you slap it on a variable name learns to ignore the label.

The stack from the last lesson gives you a prior. Layer-1 findings — correctness, security, clear principle or pattern violations — are almost always blocking:; pure style and polish are nit: or suggestion: at most. But don’t apply it mechanically: a principle question that’s genuinely ambiguous in this diff might be a question: first, and become blocking: only once the author’s answer confirms the violation.

Tip

Before you type blocking:, try to finish the sentence “this is wrong because the codebase already decided ___.” If you can name the decision, it’s blocking. If the honest ending is “…because I’d have done it differently,” it’s a suggestion.

Try the cut on a few scenarios. Pick the severity that fits, and notice why the tempting answer is wrong.

A working function does its job correctly, but the reviewer is convinced it would read better split into two smaller functions, and they hold that view strongly. What severity fits?

suggestion:

blocking:

nit:

question:

The diff includes a list query against a tenant-scoped table with no org filter at all, so running it can hand back rows belonging to a different organization. What severity fits?

blocking:

suggestion:

nit:

question:

A reviewer genuinely can’t tell whether an endpoint is reachable from an outside request. If it is, it’s missing an auth check; if it’s strictly internal, the current code is fine. What severity fits first?

question:

blocking:

nit:

suggestion:

The language of disagreement

Now come the words below the label. The craft is three adjectives: opinionated, evidence-led, short. State your position, name the reason it’s right, make the ask, all in one to three sentences. The voice is one peer talking to another who shares the same codebase: not a gatekeeper guarding the merge button, not a supplicant apologizing for an opinion.

Beginners’ comments go wrong three recurring ways. Here is each, with the rewrite that fixes it.

Hedged into uselessness

maybe we could perhaps consider possibly scoping this to
the org at some point? not sure, just a thought, feel free
to ignore

No position, so no signal. The author can’t tell a real concern from thinking out loud, so they ignore it, and a tenant leak ships.

Blame-laden

this is wrong. did you even read the docs on tenantDb?
we literally have a helper for exactly this.

Aims at the person, not the code. It puts the author on the defensive, and read a year later it sounds like an accusation. The defensiveness, not the bug, becomes the topic.

Essay-length

So historically the reason we have tenantDb is that back
when we first built multi-tenancy we had an incident where
[three paragraphs of history] ... anyway, so the upshot of
all that is you probably want to use tenantDb here.

The ask is buried. A one-line ask lost under context the author has to dig for is the same as no ask at all.

Same finding, mangled three ways. Here it is said well.

blocking: this query isn't org-scoped, so it can read
across tenants. Route it through tenantDb(orgId) — see
pattern #1 for why we never query the table directly.

Position, reason, ask. The history the essay dumped inline lives behind the pattern #1 link, where the author can read it or skip it.

Talk about the code, not the author

The argument for this reflex is not politeness; it’s durability. Aim the comment at the code, not the person: “This bypasses tenantDb, let’s route it through the helper” instead of “You bypassed tenantDb.”

PR threads get read months later, often by people who weren’t in the room. Read cold a year on, “you didn’t validate this” lands like an accusation in the permanent record, even though it was a neutral note at the time. “The code does X” stays neutral forever, because it never pointed at a person. You’re not being nice; you’re writing for an audience that includes strangers reading the archive.

You forgot to scope this to the org.

This isn't scoped to the org — let's route it through tenantDb.

Two exceptions earn a “you”: praise (“nice call keeping the pattern uniform here”) and direct questions (“can you explain why this path skips the wrapper?”), since both are genuinely about the person.

Ask when you’re unsure, assert when you’re sure

The question: label has one honest use and one dishonest one, and the difference is credibility.

The honest use is when you genuinely don’t know. “Is there a reason this doesn’t go through authedAction?” is a real question when the author might hold context you lack — maybe this path can’t be reached from a request and the wrapper would be dead weight. It signals the uncertainty, invites the explanation, and resolves fast.

The dishonest use dresses a position you’re certain about as a question to dodge committing to it: “Can we use authedAction here?” when you mean “this must use authedAction.” That’s epistemic cowardice , and it costs you three ways. It adds a round-trip, since the author answers the literal question (“yeah, we can”) instead of acting on the position. It softens the signal, so a real requirement reads as idle musing. And it erodes your standing: a reviewer whose “questions” are secretly demands trains the team to distrust both.

So ask when you’re uncertain, assert when you’re certain, and never use a question to dodge a position. A question: that’s really a blocking: in disguise hides the merge-hold signal exactly the way an unlabeled comment does.

Watch video

Code-review researcher Dr. Michaela Greiler walks through ten phrasing tips (11 min): make it about the code, state it as your perspective, drop condescending words, give the reason.

One concern per comment

Put one concern in each comment.

A comment that bundles three observations forces the author to take or reject all three as a unit; they can’t agree with the first, push back on the second, and ask about the third, because it’s one thread with nothing clean to converge on. Split them, one concern per comment, each separately ack-able, fixable, or push-back-able.

Before: one bundled comment.

This function is doing a lot — the query isn't org-scoped,
the error path throws instead of returning Result, and the
variable names are pretty unclear. Might be worth a look.

After: three separate comments, each independently ack-able, fixable, or push-back-able:

blocking: this query isn't org-scoped — route it through
tenantDb(orgId) (pattern #1).

blocking: the error path throws on bad input; actions
return the Result shape (pattern #6).

nit: data2 would read better as pendingInvoices.

A corollary: if three genuine concerns really do cluster on one line, that’s a signal the line is doing too much. The honest response isn’t three comments but one, a level up: “this function carries too many responsibilities to address cleanly; it wants to split first.” That, itself, is exactly one concern.

The suggestion block: when the fix is one click away

Most review platforms — GitHub, GitLab, Graphite — support a ```suggestion block inside a review comment, and beginners reliably under-use it. Whatever you put in the block renders as a proposed diff the author applies with a single click and can batch with others into one commit. You’re not describing the fix; you’re handing it over, ready to merge.

The syntax is a fenced block with suggestion as the language, on the line you’re replacing.

suggestion: scope this to the org so it can't read across tenants.

```suggestion
const invoices = await tenantDb(orgId).select().from(invoicesTable);
```

Put several lines inside the fence and the author applies them all as a single commit.

Reach for it when the edit is under roughly five lines and obviously correct — a renamed identifier, a missing await, a one-line wrapper swap, anything mechanical with exactly one right answer. There it removes a manual step at no cost. Above that bar — a forty-line rewrite, or any fix that involves judgment — write prose: a suggestion block jams an architecture conversation into a one-click diff the author can’t engage with as a decision. A mechanical fix becomes a suggestion, a design change becomes prose, wherever you review.

Receiving the review

Now flip to the author’s seat, where you’ll spend far more career-hours than writing comments. Three reflexes carry the receiving side.

Respond to every comment. Even a 👍, even “done in a3f9c2.” A comment with no response leaves the reviewer guessing whether you saw it, agreed, or missed it — the same latency the lesson fights, now running the other way. This is the payoff of one-concern-per-comment: each comment is a single thing, so each gets a cheap acknowledgment.

Push back with evidence; don’t just defer. A reviewer can be wrong, and an incorrect comment shouldn’t reach the codebase just because a reviewer wrote it. Where you have evidence the design is right, defend it neutrally, with the same code-not-person discipline you’d want from the other side, because you own the code’s correctness as much as the reviewer does. Deferring to a wrong comment fails exactly as much as ignoring a right one; both put bad code in main.

Resolve threads honestly. “I’ll get to it next PR” is a small lie the archive remembers: six months on the thread reads “resolved” and the fix never came. “Fixed in a3f9c2” is the honest resolution. Quietly closing a thread without addressing the comment is the author-side twin of the drive-by approval , making the record say a conversation happened that didn’t.

One thing is non-negotiable: a blocking: comment is structural. You don’t 👍 it and merge anyway; the label means the merge waits. If you think the blocker is wrong, you don’t route around it, you argue it down to a non-blocker in the thread, where the reviewer can agree or hold the line. Disagreeing with a blocker is fine. Pretending it wasn’t there is not.

You’re the author. A reviewer left a blocking: comment, but after a careful re-read you’re confident the current code is correct and the blocker is mistaken. What’s the right move?

Reply in the thread with the evidence for why the code is right, and argue the label down to a non-blocker in the open — merging only once that’s resolved.

Apply the requested change anyway; conceding is faster than a thread, and the reviewer’s call carries more weight on their own comment.

Resolve the thread yourself and merge, since you’ve already satisfied yourself the code is correct.

Take it to a private DM so the disagreement stays off the PR, then merge once you’ve talked it through.

Match the review action to the comments

Most platforms make you pick an action when you submit a review: approve, comment, or request changes. The action should mirror the severities you wrote, not your mood or your relationship with the author.

Action	When	What must be true of the comments
request changes	At least one blocking: comment exists	The state says, out loud, that the merge waits
approve (with comments)	Only suggestion: / nit: / resolved question: remain	The author is trusted to address them or not, and can merge
comment	The default during back-and-forth	No final verdict is warranted yet

The rule: the review state tracks the comment severities. Write a blocking: and you click request changes; anything else lies about what you found. The drive-by failures break this both ways. A drive-by approve, clicking through unread because you trust the author, skips the content entirely; a drive-by request changes, blocking over a style preference, inflates it. Don’t wave the senior teammate through unread, and don’t over-block the junior. The action is a claim about the comments, so make it a true one.

Habits that keep reviews honest

Three habits compound over a career.

The “what would I want to see” mirror. Before you submit, re-read your comments as the author about to receive them. Are the severities right? Is every ask actionable, every reason named and, where one exists, linked? Sixty seconds, every review. It catches the comment that came out as “I don’t like this” when you meant “this conflicts with pattern #7.” If you build one habit from this lesson, build this one.

Escalate at three round-trips. When a thread crosses roughly three exchanges without converging, the disagreement is structural, and the comment box is the wrong surface: you’re litigating a design decision one paragraph at a time. Pull it into a fifteen-minute call, decide, and then capture the decision back in the PR — a call that isn’t written down didn’t happen, as far as the archive is concerned. If the decision is architectural, it’s a candidate for an ADR, which the next chapter covers.

Mentorship is a side effect of linking. When a comment explains a principle or pattern violation, link the lesson or doc that owns it: “see The tenantDb helper for why we never query the table directly.” It costs a minute and saves the author an hour of context-hunting. Feedback on your own code, at the moment you got it wrong, teaches faster than any doc read cold; the junior who gets three linked comments a week is up to speed in a quarter.

Reviewing agent-authored PRs and comments

Coding agents change two things about your queue, and neither moves the bar.

An agent-authored PR gets the same bar. The diff gets the same five-layer pass, the same checklist, the same severities as any other. The bar doesn’t soften because “the AI should have gotten it right,” and it doesn’t tighten into nitpicking because “it’s only a bot.” Agents trip the exact failure modes the last lesson named: re-inventing a wrapper the codebase already has, adding plausible-looking tests that don’t exercise the contract, drifting from the conventions in AGENTS.md.

An agent-authored comment gets calibrated, not obeyed. Review tools like Copilot, CodeRabbit, and Qodo now leave their own comments. Read them like a brand-new junior reviewer’s: sometimes load-bearing, often noise, and your job is to tell which is which. A bot’s blocking: is a claim to verify, not a gate to obey; the tool flagging it doesn’t make it true.

A review, end to end

The diff below is a small two-file PR built on canon you already know, so the findings are familiar and the work now is purely the comment. Click any line to leave one. For each, label it with one of the five severities and keep it to the four-part shape: label, observation, reason, ask. The grader scores the substance, not just which line you clicked, so a comment that spots the bug but skips the severity or the ask is half the job.

Label every comment with one of the five severities (`blocking:` / `suggestion:` / `question:` / `nit:` / `praise:`) and keep each to the four-part shape: label, observation, reason, ask. Click any line to leave a review comment, then press Submit review.

src/lib/invoices/list-invoices.ts

src/lib/invoices/list-invoices.ts

import { eq } from 'drizzle-orm';
import { db, tenantDb } from '@/lib/db';
import { invoicesTable } from '@/lib/schema';

export async function listInvoices(orgId: string, limit = 50) {
  return tenantDb(orgId).select().from(invoicesTable).limit(limit);
}

export async function getInvoiceByNumber(orgId: string, number: string) {
  return db.select().from(invoicesTable).where(eq(invoicesTable.number, number));
}

src/app/invoices/actions.ts

src/app/invoices/actions.ts

import { authedAction } from '@/lib/safe-action';
import { invoiceSchema } from '@/lib/invoices/schema';
import { createInvoice } from '@/lib/invoices/create';
import { getInvoiceByNumber } from '@/lib/invoices/list-invoices';

export const createInvoiceAction = authedAction('member', invoiceSchema, async (input, ctx) => {
  const existing = await getInvoiceByNumber(ctx.orgId, input.number);
  if (existing.length > 0) {
    throw new Error(`Invoice ${input.number} already exists`);
  }
  const invoice = await createInvoice(ctx.orgId, input);
  return { ok: true, data: invoice };
});

Submit review

The next chapter turns this rehearsal into the real thing: a full PR reviewed start to finish, with the architecture-level decision written up as an ADR.

External resources

Google Engineering Practices — How to write code review comments

google.github.io

Google's vendor-neutral guide: be courteous, comment on the code not the developer, explain the why, and label severity so authors can prioritize — the same craft, from a different shop.