Review Presentation

Review presentation format

Every draft review is handed to the maintainer in the same five slots, in this order. She reads dozens of these; the shape has to be identical every time so she never has to hunt for where the verdict went.

"when we do review the important pieces are: what problem, what was solution, did API update and how, any breaking considerations (API, visual, theme), review judgement — please present these to me in a consistent format we remember."

---

The template

**#<number> — <title>** (<author>, <bucket>)

**Problem.** What was broken or missing, and who hit it. The reason, not the diff.

**Solution.** What the PR does about it, and how. The mechanism, not the files.

**Impact.** Who the CHANGE reaches once it lands, and what changes for them.
Everyone on the next release · only apps that opt in · only themes reaching for
a target · nobody yet. "Invisible until someone opts in" is a real answer.

**API.** One line per change, with the signature — never a summary:
`+ Step.indicator?: StepIndicatorPreset | ReactNode = 'auto' (public, core barrel)`.
`+` added · `-` removed · `~` changed. Say whether each is genuinely reachable
from the barrel. Or "no API change".

**Theme targets.** Same treatment, naming the element each points at:
`+ selector-option-row → the <li> wrapping each option`. Or "no new theme targets".

**Ossification.** *(only when either of the two above is not "none")* Which class
of problems does this serve, and who else is in that class?

**Breaking.** API: … · Visual: … · Theme: … — anything not "no" must appear in
Judgement, as a finding or as one line saying why not. Test the far side of any
`max()`/`min()`/`clamp()`/new default: a bound is invisible at the default value.

**Performance.** Render · Listeners · Layout · Bundle — or "nothing".

**Risk class.** low-risk | needs judgement — low-risk means *all three*: no new
API surface, no behavior regression, no performance regression.

**Judgement: <verdict>** — then one numbered entry per finding, each with a `→`
consequence and an anchor. A finding whose arrow you cannot write is not a
finding.

---
<the review comment, quoted, ready to post>
---

<inlines, if any: `file:line` — ≤20 words each>

Rules

1. Two to four sentences per slot, at most. This is the summary, not the brief.

2. Never skip a slot. "No API change" and "nothing breaks" are answers and carry information. Silence does not — it is indistinguishable from not having checked.

3. The API slot is an enumeration, not a summary. "Adds a prop to Step" is unjudgeable and lets the draft skip the work. Writing the signature out IS the check — Step.icon and Step.indicator on adjacent lines is how a duplicate axis becomes obvious, and "type NOT in barrel" beside a public return is a finding in itself. See R16b.

4. New surface must earn itself. Props and theme targets both ossify the system: once shipped, a prop cannot quietly change meaning and a theme target cannot quietly move. The bar is a class of problems, not one problem — name another real member of the class or the answer is "not yet, wait for the second case". A single-case addition becomes the precedent cited for the next one, and the surface ratchets. Theme targets get their own slot precisely because they look free: no runtime behavior, so nobody counts them. See R1g-surface.

5. Performance is a slot, not a footnote. Render, listeners, layout, bundle. "Nothing" is an answer; "I didn't look" is also an answer and must be said — silence reads as "checked and clean", which is the one thing it must never mean. See R18.

6. State the risk class. A PR is low-risk only when it adds no API surface, regresses no behavior, and regresses no performance. Not size, not file count: a 500-line change touching 20 files can be low-risk, and a 6-line change adding one prop is not. This class is the graduation path — the set the loop may eventually post unattended, because the only thing left to get wrong is communication style. A PR wrongly called low-risk is the expensive miss. See R19.

7. Breaking is three axes, always all three:

   axis	the question
   API	does an existing call site stop compiling, or change meaning?
   Visual	does anything already on screen move, resize, recolor?
   Theme	do existing theme targets, tokens and overrides still resolve?

   Visual and theme are the two a reviewer forgets to ask about, and they are exactly the ones that reach a user with no compiler saying anything.

8. The review text comes last, quoted — so the summary is judged before the wording.

9. Everything else comes after, or not at all. Evidence, what was run, what could not be verified: only if asked.

Why this order

Problem before solution, because a solution to the wrong problem is the expensive mistake and it should be visible in the first line. API, theme targets and breaking before judgement, because those are what usually move a verdict — and because new surface is the most expensive thing a PR can contain. Judgement before the review text, because if the verdict is wrong the wording does not matter.

Enforced by R16 in Critic Rules; the reviewer's own output block is in the Reviewer Brief.

---

The wiki page

The same slots, saved. Every reviewed PR gets a Review-<number> page and a row in All reviews — that is R17, not a nicety. Copy this skeleton:

# #<number> — <title>

**<author>** · <state> · [view on GitHub](https://github.com/facebook/astryx/pull/<number>)

**Verdict: <verdict>**

## Problem
<what was broken or missing, and who hit it>

## Solution
<what the PR does about it, and how — the mechanism>

## Impact
<who the change reaches once it lands, and what changes for them. "Nobody yet —
nothing consumes it" is a real answer.>

## API

• Component.prop?: ExactType = default (public | internal, where exported)

• Component.oldProp?: Type (REMOVED) ~ hookName(): + member: Type (note if the type is not in the barrel)

Or "no API change".

## Theme targets

• target-name → the element it points at (public, permanent)

Or "no new theme targets".

## Ossification
<only when either of the two above is not "none": which class, and who else is in it>

## Breaking
- **API** — …
- **Visual** — …
- **Theme** — …

Anything not "no" must reach Judgement. A bound (`max`, `min`, `clamp`, a new
default, a breakpoint) is invisible at the default value — test past it.

## Performance & resources
- **Render** — new effects, subscriptions, work in render, unstable deps. An
  effect earns the question "could a handler have done this?" every time.
- **Value ownership** — direct DOM writes for transient per-element state
  (roving `tabindex`, drag position, `data-` keys CSS reads) are a deliberate
  pattern, not a smell. The finding is two owners, not the write itself.
- **Listeners** — document/window, observers, and whether they are torn down
- **Layout** — forced sync reflow, animation off the compositor
- **Bundle** — new dependency or size delta
- **Perf test** — required when the work is O(N) in something a user can grow
  AND sits on a flow they hit constantly. Convention: `*-perf.test.tsx`,
  colocated, asserting render counts rather than milliseconds.

## Risk class
<low-risk | needs judgement> — low-risk requires all three: no new API surface,
no behavior regression, no performance regression.

## Judgement
<verdict>

1. → · file:line

A finding whose arrow cannot be written is not a finding — cut it.

## The review, as posted
> <the comment text, verbatim>

<inlines, if any>

## What changed before posting
<the maintainer's edits, verbatim — or "posted as drafted">

The last slot is the one that earns the page. The gap between what the loop drafted and what actually went out is the only honest record of where its taste is still wrong; everything else is recoverable from GitHub. A page that says "posted as drafted" is a scored point, and a page that quotes an edit is the next rule waiting to be written.

Rounds stack: a second review appends ## Round 2 rather than replacing round one, because R1e asks whether earlier findings were addressed and that is unanswerable without them.