WestScript Editors@build-notesPosted Nov 3, 2026

Writing a code review comment someone can act on

A review comment is a small piece of technical writing with a demanding reader: someone who knows the change better than you do, is reading your note out of context, and has to decide what to do next. "This looks wrong" gives them nothing to decide with.

Here is what we try to put into a comment, using two small examples. The code is made up for this post, but we ran every snippet on Node 24, and the output shown is what it printed.

Two comments on the same line

Imagine a pull request that adds pagination. Its description says page numbers come from the URL and start at 1. This function is in the diff:

function pageItems(items, page, perPage) {
  const start = page * perPage;
  return items.slice(start, start + perPage);
}

A reviewer who spots the problem could write this:

This seems off. Can you double-check the pagination?

Or this:

Blocking: pages start at 1 in this PR, but this calculation treats them as starting at 0, so the first page is skipped. pageItems(['a', 'b', 'c', 'd', 'e'], 1, 2) returns ['c', 'd'], and page 3 returns [] instead of ['e']. I think const start = (page - 1) * perPage; fixes it.

Both comments point at the same bug. The first starts a conversation: the author has to work out what you noticed, then guess whether you meant the offset, the page size or something else. The second can be checked in a minute and either fixed or answered with "actually, the caller subtracts one".

Also notice that this bug would not announce itself. Array slice returns an empty array when the start is past the end, so nothing throws. A concrete input and output carries that point better than any adjective could.

The four things a reader needs

The longer comment above is not long because it is polite. It is long because it answers four questions:

What did you see? Name the line or the behaviour, not a general impression.
Why does it matter? Give the consequence: skipped results, a slower query, a confusing name for the next reader.
What could be done? Offer a fix if you have one. If you do not, say so; pointing at a problem without a solution is still useful.
How much does it matter? Say whether this has to change before merging.

The small input in that comment does the same job as the small example in our notes on explaining a tricky concept. The author can run it, see what you saw, and skip the part where the two of you describe the same bug differently.

Say how much it matters

Twelve comments on a pull request look like twelve demands unless you say otherwise. The author cannot tell which ones you would hold the merge for, so they either fix everything or start guessing. A one-word label at the start clears that up:

Blocking: a bug, a security problem or a broken contract. It should change before this merges.
Suggestion: you think this is better, and the author can reasonably disagree.
Nit: a small preference. Fine to ignore.
Question: you want to understand something, and you are not asking for a change yet.

The exact words matter less than using them honestly. Calling something a nit and then refusing to approve until it changes teaches people to ignore the label. And if you find yourself leaving the same nit on every pull request, it probably belongs in a formatter or linter rather than in a person's review.

Run your suggestion before you post it

A suggested fix carries extra weight, because a busy author may apply it without much thought. So test it. Here is a line where the obvious suggestion turns out to be wrong:

const timeoutMs = Number(process.env.TIMEOUT_MS) || 5000;

Because || treats 0 as missing, TIMEOUT_MS=0 quietly becomes 5000. The tempting comment is "use ?? here". Trying that takes a few seconds:

bash

$ TIMEOUT_MS=0 node -p "Number(process.env.TIMEOUT_MS) ?? 5000"
0
$ node -p "Number(process.env.TIMEOUT_MS) ?? 5000"
NaN

The zero survives, but the default is gone. An unset variable gives Number(undefined), which is NaN, and ?? only falls back on null or undefined. Moving the operator inside the call works for both cases:

bash

$ TIMEOUT_MS=0 node -p "Number(process.env.TIMEOUT_MS ?? 5000)"
0
$ node -p "Number(process.env.TIMEOUT_MS ?? 5000)"
5000
$ TIMEOUT_MS=abc node -p "Number(process.env.TIMEOUT_MS ?? 5000)"
NaN

That last line is the catch. The original turned TIMEOUT_MS=abc into 5000, and this version gives NaN, so the fix trades one edge case for another. We also do not know from the diff whether 0 is even a value anyone wants. That makes it a question, and the comment should read like one:

Question: should TIMEOUT_MS=0 be allowed? || turns it into 5000. If 0 means something here, Number(process.env.TIMEOUT_MS ?? 5000) keeps it, though TIMEOUT_MS=abc then gives NaN, so it may want a check as well. I tried both in Node 24.

If you could not test something, say that too. "Untested, but something like this" is a perfectly good phrase, and it tells the author how far to trust the snippet.

Aim at the code, and ask real questions

Our guidelines ask people to argue with ideas and code, never with people, and review threads are where that gets tested. A few small habits help:

Describe the code instead of the author. "This path does not handle an empty list" lands better than "you forgot the empty list", and it is more precise.
Give the reason behind a question. "Why a map here?" can read as an accusation. "What made a map better than an array here? I am wondering whether order matters" tells the author what you are actually thinking about.
Do not phrase a demand as a question. If you mean "this must change", label it blocking and say why.
Praise specifically. "Pulling the retry logic into one function made this much easier to review" tells the author what to keep doing. "LGTM" does not.

Written threads also have limits. If a comment has gone back and forth twice without getting closer, talk it through, then write the outcome in the thread so the next reader is not left guessing. If the decision shaped the code, the reason belongs in the history as well, as we described in commit messages your future self can read.

A check before you press send

Most of this fits into a short pass over your own comment:

Could the author act on this without asking what you meant?
Did you give the consequence, not only your preference?
Is the label honest about how much it matters?
If you included code, did you run it, or did you say that you had not?
Reading it back, would you be comfortable receiving this on your own pull request?

Comments like these take a little longer to write, and the time usually comes back in fewer rounds of "what did you mean?". If your team has found review habits that work, that makes a good post. You can ask for a writer invite on the contact page.