Crafting Quality Code with Conventional Comments

October 9, 2023

Most code review friction is not technical. It is ambiguity. “Maybe rename this?” leaves the author guessing whether it blocks the merge.

Conventional Comments 🔗 is a prefix that says what to do with the feedback.

Copy<label> [decorations]: <subject>

[discussion]

Label and subject are required. Decorations and discussion are not. That is the whole spec.

What do the labels mean?

The spec’s list, condensed:

The spec also offers typo, polish, and quibble. You do not need them. Nine is already a lot.

Feel free to invent a label. I would not. If it is not on this list, pick the closest one.

Why decorate?

Decorations sit in parentheses after the label. They are comma-separated. The spec’s own example is issue (ux,non-blocking).

The ones that matter:

• (blocking): resolve this before merge.
• (non-blocking): do not hold the PR for this.
• (if-minor): fix it only if the fix stays small.

Blocking and non-blocking both exist because teams do not agree on the default. Some orgs treat every comment as blocking. Some treat every comment as optional. The decoration overrides whichever default you inherited.

A few labels already answer the question. nitpick, thought, and note are non-blocking in the spec. praise is not a request.

suggestion is the one that stalls PRs. The spec tells you to decorate it. Do that.

Copysuggestion (non-blocking): Pull the retry logic into `withRetry()` so
`syncProjects.ts` can use it. Fine as a follow-up.

suggestion (blocking): Use `AbortController` here instead of a boolean
flag. The fetch keeps running after unmount and writes to state.

Same label. Opposite merge rules. The author does not have to guess.

What does a good comment look like?

An issue with a reproduction:

Copyissue (blocking): `parseAmount("1,000.50")` returns `1`. The regex
strips commas before the decimal check.

  expect(parseAmount("1,000.50")).toBe(1000.5);

A failing test in the comment is worth ten paragraphs.

A question that might be an issue:

Copyquestion: Is `userId` set when this runs? `onSubmit` reads it from
`session`, which can be `null` during sign-out.

If the answer is no, edit the label to issue. If the answer is yes because of X, X probably belongs in the code.

A nitpick the author can ignore:

Copynitpick: `useUserData` reads better than `useUserDataHook` to me.
Take it or leave it.

Without “take it or leave it,” every nitpick becomes a negotiation.

A thought that is not a demand:

Copythought: If we get a third permission check like this,
`usePermission(action, resource)` might be worth it. Not for this PR.

Generic praise (“nice!”) is noise. Specific praise teaches the rest of the team what to copy.

Copypraise: The `Result<T, E>` wrapper cleaned up four call sites that
used to swallow errors.

The spec asks you to leave at least one sincere praise per review. One. Not one per hunk.

What should you not do?

Leave suggestion undecorated. That is the comment authors stall on.

Treat issue as automatically blocking. The spec’s own example is issue (ux,non-blocking). If it blocks, say (blocking).

Call a blocker a nitpick. The spec says nitpicks are non-blocking by nature. Mislabeling them trains people to ignore the prefix.

Stack labels. suggestion/issue: means you have not decided. The spec is one label. Decide.

Review fifty lines of an approach that should not exist. One top-level issue (blocking) on the design. Then stop.

Hand-wave. issue: this is broken is not a comment. Say what is broken, and what to do.

Put the strongest blocker first, on its own. A wall of nitpicks buries the bug.

Attach a patch to a suggestion when you can. Suggestions with code get applied. Suggestions without code get debated.

Do you need a rollout?

No.

Use the labels yourself for two weeks. Authors will ask what (non-blocking) means. Other reviewers will copy it because it is faster to read.

If you want less typing later, save the prefixes you actually use. GitHub saved replies 🔗 are personal, up to 100, and work in any comment box. GitLab comment templates 🔗 can be personal, or shared with a group or a project.

I would start with three: suggestion (blocking), suggestion (non-blocking), and nitpick. Decorate issue when you write it. The prefix is the feature. The wiki page can wait.

Written by Zubin Khavarian