-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5391
cixzhang (ours) · MERGED 535f327 · view on GitHub
Verdict: approve — after a self-gate that requested changes on our own draft.
Head reviewed: 69a0fc45885 (gate 2). Gate 1 ran on the working tree before correction.
Both defects were surfaced by the review of #4847, and both are ours.
- A builder can be shown nothing where a section should be, with every check
green.
component-detail-wiring.test.tsexists to stop a renderer being written and never wired — but it collected importers across all ofsrc/, so a unit test counted as an importer. That is the state Anatomy sat in for months before #4847. - A builder reading
/components/FieldseesOptional/Requ/ired indicator;/components/SideNavshowsCollapse/expa/nd toggle. A solidus is not a break opportunity in CSS the way a hyphen is, so Chromium takes its last resort and breaks inside the word.
1. the wiring guard walks the import graph out from app/ rather than
collecting importers across src/ [problem 1]
2. the Element cell renders a <wbr /> after each solidus [problem 2]
Both trace to a stated problem. The rest is three unit tests and two docblocks.
Decision 1 reaches the next person who writes a renderer and forgets to wire it — a red check instead of a silent nothing. Nobody else; the tree is clean today. Decision 2 reaches readers of the two component pages whose part names contain a solidus. Every other page renders identically.
No API change. anatomyNameSegments is a new export from anatomyHelpers.ts, a
module inside apps/docsite, which is private: true — not reachable from any
package barrel.
No new theme targets.
- API — no.
- Visual — one change, and it is the fix. The Element column measures 140px before and after on Field, SideNav, ChatLayout and Avatar, at 1280px and 375px. The Field anatomy table is 24px taller at 1280px (the long name now takes three lines instead of two-with-a-broken-word) and identical at 375px.
- Theme — no.
One String.split per row over a 4–8 row table, inside a render that already
rebuilds the column array. No effects, no listeners, no layout reads, no
dependency. The guard now reads only files reachable from app/; suite 402
tests in 6.2s.
- Field, the broken name, 1280px before/after Field__before__1280.png · Field__wbr__1280.png
- SideNav, the second one, 1280px before/after SideNav__before__1280.png · SideNav__wbr__1280.png
- Field, 375px before/after Field__before__375.png · Field__wbr__375.png
- Avatar, a short-name page before/after Avatar__before__1280.png · Avatar__after2__1280.png
- ChatLayout, the hyphen case before/after ChatLayout__before__1280.png · ChatLayout__wbr__1280.png
All opened with the read tool; six are published on the fork's assets/pr-5391
branch and embedded in the PR body. Reproduce with
probe-kit/docsite-anatomy-wrap.cjs (PORT=6450, docsite dev server) — it
reports each word's unbroken width in the cell's own font and flags a word whose
line boxes outnumber its break opportunities.
Needs judgement, on the visual axis — the Field table grew 24px at 1280px, so this is not low-risk under the "nothing got bigger" test even though nothing grew horizontally.
approve — gate 1's four findings all discharged.
1. [gate-1 #1, fixed] the probe counted a hyphen break as a defect
→ re-measuring reversed the target list: ChatLayout untouched (it never
had a defect), SideNav added (it did) · probe-kit/docsite-anatomy-wrap.cjs
confirmed a second way: read both frames, not the rect count
2. [gate-1 #2 and #4, dissolved] the px-per-character estimate and its two
drifting constants
→ both deleted; nothing mirrors a font metric or a density token now
3. [gate-1 #3, dissolved] the mobile cost
→ the Field table is 1031px tall at 375px before and after. The 280px
growth the column-widening draft caused is gone
4. [not blocking] a long name with no break opportunity at all would still
break
→ no such name ships: of 275 part names, three have a word of 14+
characters and all three carry a solidus or a hyphen. The unevadable
fix is Table sizing a column to its content — core API, and a decision
Checks run on the new mechanism, because a <wbr /> is a DOM change to a name
people read, copy and search: textContent unchanged · selection copies clean ·
window.find still matches across the break · children are #text, WBR,
#text.
Nothing was posted. This is our own PR and GitHub refuses a self-approval; it merged directly. The gate-2 summary read:
Thanks — this is right. Guard rewrite proven by the scenario table; the
<wbr />keeps every column where it was.
Gate 1 requested changes on our own work, and it was right three times. The
draft it graded widened the Element column from a per-character width estimate.
Gate 1 found that the probe which motivated that fix counted a legitimate hyphen
break as a defect — so the fix was aimed at the wrong pages, widening ChatLayout
which renders fine and missing SideNav which does not. It found the estimate had
zero margin on the one name it existed for (Collapse/expand measures 8.00px
per character against a constant of 8). And it measured what the wider column
cost at 375px: 280px of extra height, invisible at 1280px.
The redirect came from R30's question — the findings shared a parent. The draft
had answered "the name does not fit" with "make the box bigger", when the
browser's own answer for the sibling case was sitting in the same table:
Scroll-to-bottom button is the same length and wraps correctly because a
hyphen is a break opportunity. Offering that break with <wbr /> dissolved
three findings at once — no estimate, no constants, no column growth.
The shipped fix is not the drafted fix. That is the counter-example to
reading "self-review is the weakest form of the loop" as "self-review is
worthless". It is the weakest form because nobody outside checks it and
"it found nothing" is a claim rather than a result — not because it finds
nothing. Here it found three blocking defects and changed what merged.
Cost: two gate passes, 46 minutes total, 16 of them measuring across 11 probe runs and 3 re-measures, each forced by a finding changing the code.