-
Notifications
You must be signed in to change notification settings - Fork 0
Review Loop 2026 08 23
Five runs, seven new rules, and then a second half of the night — §9 and §10 carry that. Every rule came from Cindy correcting a review, and the corrections have one shape, so this page is mostly about that shape rather than about the seven rules individually.
| rule | in one line | came from |
|---|---|---|
| R1j | an unjustified change is a finding even when nothing measurably breaks | #5353 |
| R2g | an inline names the problem and points at the pattern | #5314 |
| R6e | a superseded PR's leftovers need a user, not just a difference | #4839 |
| R18e | an effect that sets state costs a render pass; price it | #5353 |
| R18f | a listener is fine when scoped to the moment it is needed | #5353 |
| R29 | split by decision count, not line count | #5353 · #5352 |
| R29 § the boundary | every decision traces to a stated PROBLEM | #5353 |
what it cannot
This is the single biggest gap between a loop review and hers, and #5353 is the case study.
All three of Cindy's blocking objections were changes the PR made and never justified. The review found all three — every one of them is written down somewhere in the draft, accurately — and drew no conclusion from any of them:
| what changed | what the review did with it |
|---|---|
isAutoHide default flips to false when endContent is set |
listed in API and IMPACT, never asked why |
'swipe' joins ToastDismissReason
|
OSSIFICATION argued for it |
the imperative announce became a useEffect
|
PERFORMANCE called it "the smell", did not block |
Her reading of the same diff: "a swipe is arguably also manual, so why the third classification — what does it gain", and "this needs a stronger rationale". Neither is a measurement. Both are the same question: the PR changed something and never said why.
The 'swipe' row is worse than a miss. The review reasoned its way to a
justification — "a genuine third member of an existing enumeration" — that the PR
itself never offered. An unargued change with a reviewer-supplied rationale is
strictly worse than an unargued change, because the reviewer has now laundered
it. Never build the author's case for them.
So R1j: for every entry in API, THEME TARGETS and every changed default, find the sentence in the PR that justifies it — not the one that describes it, the one that says why the old behaviour was wrong. Quote it, or record that there is none.
Why the loop is built this way is worth naming, because it is not carelessness: a measurement is checkable and a judgement is not, so a run that only says what it measured is never wrong. It is just not a review.
The first #5353 draft said "six decisions". Read as a complaint about size, which invites "it's a cohesive change" and goes nowhere.
The posted review says the same thing as four bullets, each with a file:line:
one toast at a time below 640px ToastViewport.tsx:390, isAutoHide :520,
swipe Toast.tsx:100 + types.ts:15, the announce :395. Now it is four things
to write up, and the author can do the first one this afternoon.
Same content, different instrument. A count describes the PR; a list is work someone can start. R29 carries this as "then name the split, concretely" — say which changes go in which PR and which one lands first, never "please split this up".
The first #5353 draft named the split mid-review and then closed on a design question about swipe direction. Whatever the middle says, the author answers the last thing you asked — they would have answered the design question, the split would have evaporated, and round three would have been the same review again.
The posted version ends on the ask that matches the verdict: write up the problem each of those four solves, and split out any that want their own discussion.
This one is Cindy's correction to the first version of R29, which had landed as "one PR, one problem":
"I actually don't mind more than one problem per PR, but all problems and solutions should be recorded, not piggybacked."
So the boundary is not arithmetic, it is the PROBLEM statement:
Every decision in SOLUTION traces to a problem written down in PROBLEM. Anything else is piggybacking, and it needs its own PR.
Which resolves honestly in both directions: five decisions that all trace to stated problems are one legitimate PR, however long; two decisions where the second traces to nothing are two PRs, even though two is under any threshold. The failure is silence, not plurality.
On #5353 the written problem is a toast against a notched phone's raw viewport
edge. The safe-area gutters answer it. The compact queue, the isAutoHide
default, swipe and the announce rewrite answer nothing written down anywhere in
2,777 lines. Four of six piggybacked — and that is what "six decisions" had been
pointing at all along.
The remedy has two forms and the cheaper one is usually right: either the author writes the missing problem down, often a two-line edit that ends the finding, or the change moves to its own PR. Ask for the record first. Do not demand a split from someone who simply under-described what they did.
Same author, same day, same quality of work:
| PR | runtime lines | decisions | outcome |
|---|---|---|---|
| #5352 Dialog | ~50, in one file | 2 | merged within the day |
| #5353 Toast | ~480, under ~2,300 of stories/tests/docs | 6 | draft, round two |
Cindy: "The dialog stuff seems fine compared to toast. Most of it is stuff like documentation and visual tuning." #5352 also touches 9 files and adds 604 lines — size did not decide it. Its bulk is genuinely evidence, and a big diff whose bulk is evidence should be called fine.
Size is still a separate, real signal — "2777 new LOC is a smell for sure" — and the two fail differently. Many decisions in a small diff means the reviewer holds four unrelated judgements at once and the settled one waits on the unsettled one. Few decisions in a huge diff means nobody can actually read it. #5353 is the second enabling the first: the ~5:1 ratio of evidence to runtime is what let four unargued behaviour changes arrive looking like housekeeping.
Report both numbers and say which one is the problem. Count judgements about runtime behaviour — stories, docs, tests and changesets are how a decision is evidenced, not more decisions, and counting them makes the rule fire on well-shaped PRs.
#5314, on the first draft: "The inline comments need better voice", then "Basically tell them the problem concisely. If there's an existing pattern, ask them to use it."
| drafted | posted |
|---|---|
| "an iPhone in landscape is 852px, so this bound swaps the component under someone mid-filter" | "Hmm, 768 is an iPhone in landscape. Swaps the component mid-filter" |
The drafted line is the JUDGEMENT → consequence copied down to the line number.
The summary carries consequence; the inline carries location — and, when one
exists, the pattern we already have, named with a file and a line. The
duplication between the two is exactly what makes a review feel machine-made.
The hedges — "Hmm", "Might need", "Probably can" — are not politeness. An inline is written by someone who has read one part of one file, and the hedge is honest about that, which is what leaves the author room to answer "no, because…".
Both from #5353, and both fill the same hole as R1j inside the PERFORMANCE slot: the slot described what was there and charged nothing for it.
-
R18e — an effect that sets state renders, commits, runs, sets,
and renders again. Report it as a number (
+1 render pass per visible-toast change), not as an observation. Watch for the tell that the effect created its own problem: aSetor ref of already-handled ids, existing only to suppress re-runs — on #5353,announcedIdsRef, unbounded. And replacing a handler with an effect is a regression unless argued: the question is why was the handler not enough?, not is the effect correct?. Often the deleted comment on the handler already said why it lived there. - R18f — "Listeners understandable if we aren't incurring them globally all the time. Should be added when needed." Two questions per listener, not one: where is it attached, and when. A drag listener that exists only during a drag is free the rest of the time; the same listener attached at mount is not.
#4839 had two genuine leftovers the winner lacked. The instinct is to hand them over so nothing is lost, which feels like diligence and is R28 committed from the other side — a mechanism with no established need, now proposed by us, in a close comment, to someone whose PR we are shutting.
Two minutes of counting settled it: 150 /template-assets/ references, zero
multi-dot, zero .m4v. Neither leftover reaches anyone today. A leftover earns a
handover only when you can name who it reaches — the same → line a JUDGEMENT
finding needs.
And the measurement goes in the close. A contributor who found a real edge case is owed the difference between "we checked, and it reaches nobody today" and silence.
Two instances tonight, same shape: the loop reasoned its way to a convention from first principles instead of looking up where the system had already written it down.
| PR | what happened | cost |
|---|---|---|
| #4707 | prescribed a remedy the code already called | caught by STEP 7c2 before posting |
| #4743 | requested changes against the wiki's own theming principle, which 20 landed components and all three siblings already follow | posted; 16 days of a contributor's time |
7c2 greps the DIFF for the fix it is about to ask for. Nothing greps the SIBLINGS for the convention — which is the check that would have killed both. That is a kit-shaped gap, not a wiki one; it is proposed as a rule and not yet written.
The tell on #4743 was there to read: the author's changeset mirrored the precedent's wording down to the prefix. He read the precedent better than the review did.
- 17 posted reviews audited · 1 false block — #4743, the row above.
- The NEED test over 23 aged PRs: 1 death in 22 as a kill mechanism, and 13 of 23 sorted as "waiting on a decision, not a review". It is a sorter, not a killer — see Aged PRs.
- Self-review is the weakest form of the loop — nobody outside checks it, so "it found nothing" is a claim rather than a result. Weakest is not worthless: on #5391 the self-gate blocked our own draft three times and the shipped fix is not the drafted fix.
| PR | verdict | posted? |
|---|---|---|
| #5261 DateInput native touch picker | hold for a ruling | no |
#5255 Banner collapsible
|
request-changes, round two | no |
| #5314 PowerSearchMobile | comment | yes + follow-up |
| #5353 Toast | comment | yes |
| #4839 cli video sources | close as superseded | yes |
-
A ruling on #5261 — whether a
nativePickerescape hatch survives now that DateInput decides touch from the pointer type. Tier question, so it is hers. - #5255 round two is drafted and held until the maintainer's own round-one thread moves.
- An issue against the CLI's extension guard (#4839) — the silent fall-through to the image branch lost its home when the PR closed.