-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5094
PR #5094 · author josephfarina · exact head e27a123 · reviewed 2026-08-27 · loop 1.6.0 · audit rubric 1.13 · request changes — posted
#5094 docs: replace em dashes, en dash, and ellipsis in doc prose by josephfarina (Joey) (bucket: internal)
Bucket evidence: @josephfarina is listed in .github/ENGOWNERS on origin/main; gh api repos/facebook/astryx/collaborators/josephfarina/permission → write. Not a draft. Internal, so the [Full review] wiki link is owed on the posted comment and no Discord clause is used.
Prior review: none. gh api repos/facebook/astryx/pulls/5094/reviews → empty; .../pulls/5094/comments → empty. The only issue comments are vercel[bot] and github-actions[bot] (PR Analysis Report). Nothing to extend, narrow, or contradict, and no named colleague to reconcile with.
Competing work (radius check, R6c): only one other open slop PR exists, #5597 by the same author, and it touches Spinner.doc.mjs and useTableGroupedRows.doc.mjs — no overlap. No open PR claims the two lines this one still has to give, so the remedy is to shrink this PR, not to close it in favour of another.
e27a1235a40a27e1452a61e898c6acd6e136335e <- every claim below was verified at this commit
Base f19156761f8a8a80584bd0a1e5b017eb89db7276; origin/main is d80c7889649b30cb8cecf59f3f65e929491b7c07. Two commits: 4399d643a (2026-08-16), e27a1235a (2026-08-17). Opened 11 days ago, untouched for 10 — the branch did not drift; main moved under it, and nobody reviewed it in between.
R14d: head re-fetched at write time and unmoved (.head.sha still e27a1235a). R14e: every anchor below was opened at that head and its line text is pasted beside it.
LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13
LANE: full
WHY: fast requires "no merge conflict" and no unresolved blocking signal. mergeable: CONFLICTING, mergeStateStatus: DIRTY, and the required test check is red at this head. Two disqualifiers established before deep review, so fast was never available and no promotion was needed.
Clean. 7 files, all *.doc.mjs. No package.json script change, no dependency change, no pnpm-lock.yaml, nothing under .github/workflows/, no new *.sh/*.mjs/*.cjs under scripts/, tools/ or .husky/, no env/.npmrc/.ssh reads. Head repo is facebook/astryx, not a fork. Proceeded normally.
WHY 1: Six doc prose fields used an em dash to hang a trailing clause, and one used a literal …. The house reads that punctuation as a generated-text tell.
WHY 2: A builder reading a component's best practices or astryx docs typography cannot tell hand-written guidance from filler, so they skim the sentence that was written to stop them making a mistake.
WHY 3: These .doc.mjs fields are the teaching surface — they are how someone learns the system without leaving the terminal. Prose that reads as machine output is prose that gets discounted, and the guidance stops doing its job.
USER-FACING PROBLEM: a builder reading Astryx's in-terminal docs meets sentences carrying a house-style tell and discounts advice that was actually written for them. PROBLEM SEVERITY: polish — the docs are accurate and readable today; nothing is unreachable, wrong, or blocked. The PR says as much itself ("No wording or technical content changed; only the punctuation"). Not inflated here, and not inflated in the review.
VERDICT: clear
Doc files hold plain-English sentences that the CLI and the docs site print back to a reader. Some of those sentences hung a trailing clause off a long dash. This rewrites the punctuation — a colon, a semicolon, or parentheses instead — and turns one typographic ellipsis into three dots. What the docs say is untouched; only how the sentences are pointed.
SOLUTION (1 decision · 0 runtime lines of 38)
- restyle typographic punctuation in doc prose strings [traces to the stated problem]
One decision, not seven: every line is the same judgement applied repeatedly, and there is no version of this where you take the SideNav sentences and decline the CLI ones. Commit 2 widened the class from dashes/ellipsis to a curly apostrophe (Thumbnail.doc.mjs); same judgement, so still one decision. Nothing piggybacks — there is no second, unstated problem riding along.
Zero runtime lines. All 19 changed lines are string literals in description / text / label / guidance fields inside *.doc.mjs.
BURDEN: low — no state, no Effects, no listeners or timers, no shared-system coupling, no public surface, no new tests to carry. BURDEN MATCH: proportionate — a polish problem answered with punctuation edits and nothing else.
R1f — the commit type is a risk claim, checked in that direction: docs: asserts nothing consumer-visible changed, and the diff honours it. Seven *.doc.mjs files, zero runtime lines, no changeset owed. The title is honest about scope.
Early 7d hypothesis, recorded before the remaining slots: everything I find will be a child of "this branch has been overtaken by main." Re-tested in JUDGEMENT.
VERDICT: note — the PR body describes six files; the diff carries seven. packages/core/src/Thumbnail/Thumbnail.doc.mjs arrived in commit 2 with its own commit message and was never added to the body or title. Separately, the title claims an en dash the diff never replaces: the only en dash in these files is 20–31px at typography.doc.mjs:126, which is correct usage and which the body explicitly leaves alone.
OWNER: the per-surface *.doc.mjs doc sources.
TIER 1: none — no shared runtime system participates.
TIER 2: none.
SEAMS: one, and it matters. packages/cli/README.md carries generated tables built from response-types.doc.mjs and the command manifest by packages/cli/scripts/generate-cli-readme.mjs, gated in CI by packages/cli/test/readme-gen.test.mjs.
BEHAVIOR UNIT: n/a — no behavior, no Effects, nothing to extract.
| seam | driven result |
|---|---|
CLI generated README (generate-cli-readme.mjs --check) |
fails — test job step 9, run 32014640186, annotation names readme-gen.test.mjs:25
|
| component autodocs / docsite string fields | passes — docsite-test, build-storybook, check-components green at this head |
| CLI dense/docs/help rendering | passes — CLI Smoke Test green at this head |
The change edits a doc source and does not regenerate its published artifact.
packages/cli/README.md:419 (at e27a1235a, opened and read)
| `theme.template` | A write receipt for the annotated theme template: … and the reason it was not — `exists` when a file was already there, which is a success. |
packages/cli/README.md:421 (on d80c7889, opened and read)
| `theme.template` | A write receipt for the annotated theme template: … and the reason it was not. `exists` when a file was already there, …
The em dash this PR deletes from response-types.doc.mjs:189 is still shipping in the README the change exists to clean. main does not have this problem, because the sibling change that landed the same edit regenerated. The repo's own gate names the remedy verbatim: readme-gen.test.mjs:25 throws README tables are out of date. Run `pnpm -F @astryxdesign/cli readme` .
VERDICT: BLOCKS — the doc source and its generated artifact are out of sync; the em dash still ships at packages/cli/README.md:419 and the required test check is red on exactly that.
Reaches builders only. No end user of any app built with Astryx sees any of this: nothing rendered by a component changes.
For a builder the honest delta at this head is two lines, and it is beneath notice. Someone running astryx docs typography sees Google Fonts: above the snippet where they used to see Google Fonts —. Nobody will register the difference, and that is the whole user-facing effect of merging this. The value is house consistency, not anything a person experiences.
Measured, not asserted: of the 19 removed lines, 2 still exist verbatim on origin/main and 17 do not.
typography.doc.mjs:59 'Google Fonts — add to your document <head>' <- still on main
typography.doc.mjs:70 'Self-hosted — one @font-face per family and weight' <- still on main
Thumbnail.doc.mjs diffs to nothing against main — the identical apostrophe edit already landed, character for character, so that file contributes zero.
What landing this newly exposes: nothing. It fixes no member of a pair and leaves no sibling newly asymmetric.
VERDICT: note — the experience delta is two label lines nobody will notice; the other 17 rewritten lines already read their final form on main.
no API change.
No prop, export, type, default, or signature is touched; every changed line is inside a string literal. SideNav.doc.mjs's resizable prop entry differs between this head and main, but the PR's own hunks do not reach it — a trial merge resolves that region cleanly to main's version, so it is an artifact of reading git diff main..head, not a change this PR makes. Verified: the conflicted regions in SideNav.doc.mjs are lines 134, 160 and 186 only, all three the same isSelected best-practice sentence.
OSSIFICATION: nothing ossifies. No prop, no export, no theme target, no accepted value, no default.
VERDICT: clear
no theming change.
grep over the PR's own changed lines: #[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(|stylex\.|xstyle → 0. No theme target added, removed, or renamed. Thumbnail.doc.mjs's edit is inside a vars[].description string; the variable name --_thumbnail-hit-inset, its default: '0px' and its private: true flag are all untouched.
VERDICT: clear
BEHAVIOR: no. Zero runtime lines. No conditional, default, early return, or state setter is in the diff. Walked the six states: empty / loading / error / disabled / controlled-vs-uncontrolled / boundary are all not reachable — the diff adds no state, no default, and no prop. API: no. No call site stops compiling; no meaning changes. VISUAL: yes, trivially. Rendered doc text changes punctuation in two labels; every other sentence already renders main's wording. No geometry, no colour, no element added, removed, or moved, so nothing grows and nothing pushes on a neighbour. THEME: no. No target, token, or var changed.
VERDICT: note — visual text changes, confined to punctuation inside doc strings.
EFFECTS: zero added, changed, kept, or moved. No .ts or .tsx file is in the diff; there is no React code to add one to.
RENDER: no reachable change — nothing in the diff executes at render. LISTENERS/OBSERVERS: none added or removed. LAYOUT: no reads, no writes, no reflow, no compositor work. BUNDLE: no dependency added; the net string-length delta across 19 lines is a few dozen characters.
VERDICT: clear
VISUAL CHECK: not applicable
WHY: a genuinely non-rendering change, established from changed paths rather than from the PR's label. All 7 changed files match *.doc.mjs; every changed line is a string literal in a description / text / label / guidance field. Grep over the diff's own changed lines: 0 style declarations; 0 role or ARIA attribute changes (the 6 aria- grep matches are the characters aria-current="page" appearing inside best-practice prose, not attributes on an element). No component source, no CSS, no element added, removed, or moved. The only rendered delta is the characters of doc text — the brief's stated prose carve-out.
Second, independent reason no frames were captured: this head cannot merge (mergeable: CONFLICTING). The text that will actually render is whatever the conflict resolution produces, not what is at e27a1235a. Frames of this head would picture something nobody can ship.
No sensor receipt is owed because no capture was made, and none is claimed. No sentence in the REVIEW or the inlines asserts an appearance.
VERDICT: clear — not applicable, with the path evidence above.
REMEDY SEARCH: not triggered — no proven visual defect
Nothing here is touched, and here is what was checked.
-
§1, automated half — auto-covered.
pr-a11ysuccess andpr-rtlsuccess at this exact head (17 check runs total)..github/a11y-baseline.jsonis not in the diff, so no baseline entry was added to buy silence. -
A16 / I1 — checked by me, n/a. No AT-facing or user-visible runtime string changed.
.doc.mjsprose is documentation, not a translated UI catalog entry. -
I4 / I5 / I6 / I7 — checked by me, n/a.
packages/core/locales/en.jsonis not in the diff: no key added, renamed, or removed, so no codemod and no[breaking]changeset is owed. -
I8–I12, direction — checked by me, n/a. No style file in the diff; 0 physical properties, 0
translate, no directional glyph.pr-rtlgreen. -
A1–A15 — not reachable. No interactive element, focus path, live region, or painted state exists in a
.doc.mjsstring. Marked not reachable rather than "checked in a browser", because no browser was driven and none was owed.
One thing checked that is worth stating and is not a finding: SideNav.doc.mjs keeps three overlays (docs, docsZh, docsDense) and docsZh's best-practice strings are untranslated English on main today. Pre-existing, not this author's, and not worsened — the PR edits all three overlays identically, which is what keeps them from drifting apart. R13: not charged to this diff.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | note — body describes 6 files, diff carries 7; title claims an en dash never replaced |
| ARCHITECTURE | BLOCKS — generated README out of sync; em dash still ships at packages/cli/README.md:419
|
| IMPACT | note — delta is two label lines nobody will notice; 17 of 19 rewritten lines already settled on main |
| API | clear |
| THEMING | clear |
| BREAKING | note — text-only visual change |
| PERFORMANCE | clear |
| VISUAL | clear — not applicable, prose-only |
| A11Y & I18N | clear |
7d, re-run at the end. The early hypothesis holds and it is the review's real subject: findings 2 and 3 are both children of "this branch has been overtaken by main." Finding 1 is genuinely independent — the README desync would be a defect on a freshly cut branch too. So the review is written at the staleness level, with the README failure carried beside it rather than under it.
GOAL: not met — measured. Of the 19 lines this PR rewrites, 2 still exist verbatim on origin/main and 17 do not; the branch's stated job is already done for everything except typography.doc.mjs:59 and :70. And for the CLI half the goal is actively unmet at this head: the em dash removed from response-types.doc.mjs:189 is still published at packages/cli/README.md:419, because the generated tables were never regenerated. Thumbnail.doc.mjs diffs to zero against main. not met cannot approve; the intended fix is repairable, so this is request-changes rather than a recommendation to close — and #5597, the only other open slop PR, does not cover these files.
DISPOSITION:
- Generated CLI README not regenerated; the em dash still ships at
packages/cli/README.md:419;testred → blocks now - 17 of 19 rewritten lines already settled on main; only
typography.doc.mjs:59and:70remain → blocks now (this is the ask) - The conflict resolution is a trap in two places, both folded into the same ask → blocks now: at
response-types.doc.mjs:189the hunk contains the wholetheme.targetsentry, so taking this branch's side deletes reference docs for a shipped envelope (astryx theme targets,packages/cli/api/theme/targets/targets.mjs, documented atpackages/cli/README.md:422); and atuseContainerReveal.doc.mjs:27main added a serial "or" ("a motion gate, a scroll, or a row whose menu is open") that this branch's side lacks, so the same resolution reverts a landed copy fix - Body describes six files, diff carries seven; title claims an en dash never replaced → follow-up, inside this PR — the correction rides the rebase that finding 2 already requires, so it needs no separate issue and no separate round trip
No orphans: every negative fact stated anywhere above appears exactly once here.
ADVICE: proven existing pattern — the repo's own CI gate states the remedy verbatim: packages/cli/test/readme-gen.test.mjs:25 throws README tables are out of date. Run `pnpm -F @astryxdesign/cli readme` . Everything else is outcome-based; the choice between rebasing this branch and respinning two lines is the author's, and the review does not prescribe it.
AUTHOR CAN PROCEED: yes — acceptance criteria, no implementation prescribed:
- whatever lands carries only what
maindoes not already have; -
packages/cli/README.mdregenerated so thetestjob is green; -
theme.targets,template.cdnandtheme.build.batchall still present inresponse-types.doc.mjsafterwards; - the serial "or" at
useContainerReveal.doc.mjs:27still present; -
typography.doc.mjs:59and:70no longer contain an em dash.
WORST OUTCOME: "whoever takes this branch's side there deletes the reference documentation for a shipped envelope, and because theme.targets's own description still carries an em dash, the slop-cleanup instinct points at exactly the wrong resolution" → request changes. Compatible: the strongest harm anywhere in this review is a builder losing reference documentation for a shipped command's output, reachable through the ordinary act of resolving this PR's conflicts. No slot says a user cannot complete, perceive, or recover from anything, so nothing here reaches past request-changes, and nothing here is soft enough for a note.
JUDGEMENT NEEDED: none — a completeness defect and a staleness call against the existing contract. No new API, no new concept, no design choice, and nothing that is Cindy's to rule on.
request changes
-
The generated CLI README was not regenerated, so the em dash this PR deletes from its source is still published → whoever reads the response-types table on npm still meets
the reason it was not — \exists`, the exact string the PR exists to remove ·packages/cli/README.md:419(source atpackages/cli/foundation/response/response-types.doc.mjs:189`) -
Main has already landed 17 of the 19 lines; two are left → whoever resolves nine conflict hunks, every one of them already dash-free on both sides, spends that work to change two code-block labels ·
packages/cli/assets/docs/typography.doc.mjs:59and:70 -
The
response-types.doc.mjsconflict hunk swallows thetheme.targetsentry → whoever takes this branch's side there deletes the reference documentation for a shipped envelope, and becausetheme.targets's own description still carries an em dash, the slop-cleanup instinct points at exactly the wrong resolution ·packages/cli/foundation/response/response-types.doc.mjs:189
Thanks for keeping at these, and sorry this one sat. Main has overtaken it: of the 19 lines here, 17 already read their final way on main, and both sides of all nine conflict hunks are already dash-free. What's actually left is two labels in typography.doc.mjs, lines 59 and 70.
Two to watch if you rebase rather than respin. The response-types.doc.mjs conflict swallows the whole theme.targets entry, so taking this branch's side there drops the reference docs for a shipped envelope. And test is red because the generated README tables weren't regenerated: packages/cli/README.md:419 still ships the em dash this PR removes from its source.
Given it's down to two lines, is a fresh PR cheaper than the rebase?
[Reviewed by Robohands]
- packages/cli/assets/docs/typography.doc.mjs:59 — This and line 70 are the only two main hasn't already fixed.
- packages/cli/foundation/response/response-types.doc.mjs:189 — Careful merging here; the conflict hunk contains the whole
theme.targetsentry.
- The title claims an en dash. The diff replaces none; the only en dash in these files is
20–31pxattypography.doc.mjs:126, which is correct and deliberately left alone. -
Thumbnail.doc.mjsdiffs to zero against main — the identical apostrophe fix already landed, so that file is dead weight in the branch. -
SideNav.doc.mjs'sdocsZhoverlay carries untranslated English best-practices on main. Pre-existing, unchanged by this PR, and not this author's to fix.
TIME total 19m
setup 2m one worktree at the PR head for the trial merge; warm main reused: yes (read-only — no install, no Storybook, no build, no probe)
reading 6m brief, critic (full), loop, presentation, harness, version file, PR body, full diff
measuring 5m 0 browser probes; 1 trial merge, 9 conflict hunks read, 1 scripted line-by-line settled/live count, CI + check-run + job-annotation reads, anchors reopened at head
writing 5m round-1 draft, critic pass, round-2 rewrite
publishing 1m wiki clone, page, index row, lock, pull --rebase, push, verify
waste 1m one competing-PR sweep looped over 100 open PRs calling gh pr diff per PR, timed out into the background, and returned nothing; a single targeted title filter answered it immediately (counted inside measuring)
CI wait 0m no remote CI triggered or waited on; the head's 17 checks all completed on 2026-08-17
- The raw
testjob log would not download (gh api .../logsrefuses terminal escape sequences). The failure cause is taken from the job's annotations, which name the test, the file and the message exactly —packages/cli/test/readme-gen.test.mjs:25, "README tables are out of date" — so the finding is anchored, but I did not read the surrounding log. - I did not run
pnpm -F @astryxdesign/cli readmelocally to confirm regeneration produces a clean tree. The claim rests on CI's--checkresult plus the observed difference betweenpackages/cli/README.md:419at this head and:421on main, both opened and read.
## #5094
VERDICT CHECK: correct — request changes. Internal author defaults to
approve-with-nits (R1), but R1's carve-out is "only request-changes if
something ships broken": the em dash still published at README:419 plus a red
required check is exactly that, and ARCHITECTURE BLOCKS + GOAL not met leave
JUDGEMENT no downgrade.
LENGTH: 112 words → ok (cap 150). Inlines 12 and 13 words → ok (cap 20).
VIOLATIONS:
R12b — "Main has overtaken this one." No thanks anywhere. Gratitude leads on
every verdict, not just approvals.
WORST OUTCOME gate — paraphrased instead of quoting a slot sentence verbatim.
DISPOSITION — item 5 used `accepted` without naming who pays, why it is
acceptable, or who decided; it was moot, not accepted.
R1f — flagged the en-dash over-claim but never stated the `docs:` type
itself was checked and found honest.
IMPACT — gave the two label strings and never converted them to what a person
notices; the sub-perceptual answer was owed and missing.
R6c — the competing-PR sweep was run but never recorded in the draft.
R14e — anchors cited but not reopened at head, and line text not pasted.
WOULD SHE HAVE POSTED IT: no — the missing gratitude alone gets it rewritten,
and the WORST OUTCOME paraphrase is a gate failure, not a style note.
REWRITE: the round-2 presentation above.
## #5094 — round 2 regrade
VERDICT CHECK: correct — unchanged, request changes.
LENGTH: 121 words → ok (cap 150). Inlines 12 and 13 words → ok (cap 20).
VIOLATIONS: none found.
R12b opens on thanks and names the loop's own 11-day delay rather than
blaming the branch. Findings carry who hits them and what they see.
R11 the conflict gets the space it does because resolving it IS the
remaining work; no lineage is narrated and no sibling PR is named in
the public text.
R15-gate no sentence in REVIEW or the inlines asserts an appearance, so no
frame is owed; VISUAL is `not applicable` on path evidence, not silence.
R12c no rubric ids, no severity headers, no aphorisms, no scorecard talk.
R14e both anchors reopened at e27a1235a and their line text pasted.
R2c `[Full review]` link present and owed (ENGOWNERS); attribution not
graded pre-post.
R13 the untranslated docsZh overlay is named as pre-existing and left with
the author unchanged.
WORST OUTCOME now quotes JUDGEMENT finding 3 verbatim and matches the verdict.
WOULD SHE HAVE POSTED IT: yes.
Two failure modes recurred, and both are the same shape: the work was done and not written down. The competing-PR sweep ran and never reached the draft; the anchors were trusted from the diff rather than reopened; the IMPACT number was measured and never converted into what a person notices. The single change to the reviewer brief that would prevent the most of them: make STEP 7 end with a "record what you checked" pass — every radius question from STEP 1b(c) gets a one-line answer in the presentation even when the answer is "nobody else is on it", the same way R7b already demands the drift sweep be stated when it comes back clean. A check whose clean result is invisible reads exactly like a check nobody ran.
Posted as drafted in review 5043367845. No judgment or wording changed after the clean gate.