Skip to content

Review Presentation

Jump to bottom
Cindy Zhang edited this page Aug 23, 2026 · 30 revisions

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.** The **user-facing failure** — a person, doing something, getting a
worse outcome. *The absence of a mechanism is never a problem*: "we don't expose
the alignment" is a fact about the code; "an RTL reader sees a column the author
left-aligned rendered right" is a problem. If it restates as "the library doesn't
have X", you have written the solution twice. Name the person and what goes wrong
for them.

**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".

**Visual evidence.** The frames, one line each — what it shows, before/after or
after-only, the path. Or one line saying why there are none. Never empty, and
never the author's own screenshots: those are the claim under review.

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

**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. Visual evidence is a slot, not an attachment. Frames, or a stated reason there are none — and the reason has to be real: nothing rendered changes (say what you checked), not renderable at all, or the build failed at this head. "The change is small" and "the tests cover it" are not reasons. Every frame is captured in real Chromium and opened with the read tool; a frame you did not look at is not evidence. Visual not "no" plus an empty evidence slot cannot both be true. This binds our own PRs too. See R15e.

  8. 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?

    On Visual, growth is the asymmetric case. Something getting smaller mostly hides inside the footprint it already had; something getting bigger pushes on everything around it, and the component's own story is the one place that never shows it, because there it is alone on the page. If any outer dimension grew, "Visual: no change" is unproven until it has been rendered with real neighbours in a constrained parent. See R15d — and note it also drops the PR out of the low-risk class.

    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.

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

  10. 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
<the user-facing failure: a person, doing something, getting a worse outcome.
 NOT "the library doesn't expose X" — that is the solution restated>

## 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.

## Visual evidence
| what it shows | frames | path |
|---|---|---|
| the bug, 375px | before/after | `shots/before__375.png` · `shots/after__375.png` |
| the 640/641 snap | after | `shots/after__640.png` · `shots/after__641.png` |
| in a real container | after | `shots/after__docs-column-500.png` |

Or, when there are none: one line saying which reason applies.

## Risk class
<low-risk | needs judgement> — low-risk requires all four: no new API surface,
no behavior regression, no performance regression, and nothing grew (R15d).

## 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.

Clone this wiki locally