[irahopkinson]

Summary

Follow-up to #10 (PT-4313). The review of paranext-core's twin PR, paranext/paranext-core#2827, found things in the derived gutter coverage test that also apply here, plus two stale focus-box comments in the stylesheet. This carries the same changes so the twins stay in step. Test and comments only; no rendering change.

Test (usj-nodes.css.gutter-coverage.test.ts)

• The parser-sanity floors are back (> 40 and > 20 entries). Both sides of the derived comparison come from the same parser, so a parser that started reading less would shrink them in step; > 0 only catches reading nothing.
• The nesting guard walks only rules that name a marker class, and fails a tracked property set anywhere but directly in a top-level marker rule (inside an at-rule, or an at-rule nested in the rule). An unrelated @media print { .note { margin: 0 } } no longer trips it.
• [dir=…] is read in any quoting or case, with or without a case flag, plus :dir(); a direction inside :not() is read as the other direction.
• directionAsymmetries reports a value set in only one direction, for margins and for text-indent.
• No hanging indent may reach past its marker's margin, and the margin must be positive. That is what lets the focus-box comments say the marker's own column wins the min() for every marker in this file, so the claim is pinned rather than left to go stale.
• Each usfm.sty oracle is compared as one object; the gutter maps are compared with toEqual.
• Only standard property names fold to lower case; custom properties keep their case, as CSS does. A marker class inside :not() or :is() cannot be attributed to one marker and is reported. Custom-property values are trimmed. The nesting report names the nested rule once. margin-inline-end leaves the unreadable set: the trailing margin can never change a derived value.
• The header carries a "Known limits" list split into approximations in play today but benign (the blanket .para rule with the 0px defaults; thirteen agnostic margin-right rules, each overwritten by an equal margin-left) and cases the sheet does not reach (descendant form, cascade order, padding, calc(), direction spellings).

Stylesheet (usj-nodes.css)

• The two focus-box comments say what --verse-text-start does today: a fallback start that the marker's own column beats for every marker in this file, rather than "aligning the box with the poetry text start"; without the gutter class poetry alignment is approximate.

Verification

• usj-nodes.css.gutter-coverage.test.ts 4/4 and usj-nodes.css.test.ts 9/9.
• Mutation probes against usj-nodes.css behave as intended: RTL-only margin, at-rule wrapping a marker rule, at-rule nested inside a marker rule, direction-qualified gutter rule, calc() value, hanging indent without margin, real margin in a compensation rule, agnostic margin-right, !important, :not([dir='rtl']) and a hanging indent past its margin all fail; an unrelated nested rule, a hanging indent within its margin and the unchanged sheet pass.
• ESLint and Prettier clean on the changed files.

AI-assisted — Claude Fable 5.1, local Claude Code session (no session URL)

🤖 Generated with Claude Code

---

This change is 

[katherinejensen00]

Review — PR #18: PT-4313: Mirror the core review of the gutter coverage test

Ira — review of pt-4313-coverage-test-review-followup (073bed7a against 2be2fe86). 24 findings, High and Medium only; Low-severity items dropped.

Every finding below was confirmed by mutation probe against a scratch copy of the stylesheet, or by direct call into the module's own helpers. Findings 1–7 are silent false-passes: the suite stays green while the sheet is genuinely broken. Three of them are cases where 2be2fe86 caught the bug and this branch no longer does — verified by running both versions against the same mutated CSS.

Paths: T = packages/platform/src/usj-nodes.css.gutter-coverage.test.ts, C = packages/platform/src/usj-nodes.css.

What checks out: suite 4/4 and sibling 9/9, Prettier and ESLint clean, CSS diff comment-only so "no rendering change" is accurate. The p2 entry is correct (\Marker p2 / \LeftMargin .125 → 2.5vw) and is the only possible witness for that value; all twelve oracle entries verified, and the oracle is now an exact cover of the seven distinct margins and five distinct hanging indents in the sheet. paranext-core#2827 at current head is a genuine twin — helpers identical apart from the two-stylesheet parameterization — so every fix below needs to land in both. The markerOf rewrite is inert on today's sheet (it re-attributes exactly two selectors, neither setting a tracked property).

---

High

1. Lowercasing declaration names makes the test accept a custom property the browser ignores

T:206 — if (node.type === "decl" && node.prop.toLowerCase() === property)

Custom property names are case-sensitive; only standard properties aren't. The new .toLowerCase() (here, T:237, T:258) erases the distinction. Probe: spelling a real gutter declaration --PARA-INDENT: 15vw leaves all four tests green while the browser ignores it, var(--para-indent, 0px) falls back to 0px, and the glyph lands in the text. The parent failed this correctly. Fix: decl.prop.startsWith("--") ? decl.prop : decl.prop.toLowerCase().

2. Taking the last .usfm_ token misreads functional pseudo-classes, where the subject is leftmost

T:164 — const last = tokens[tokens.length - 1];

First→last is right for descendant chains (.usfm_c .usfm_ca) and wrong for :not(), :is(), :has(). Probe: .text-spacing .usfm_q1:not(.usfm_qr) { margin-left: 0 } passes all four tests, but the browser really does zero q1's margin — the override is filed under a nonexistent marker qr, q1 keeps its derived 15vw, and the glyph sits 15vw out. The old first-match rule got this right, and .marker:not(.chapter) is already an idiom here (C:2739, C:2752). Take the last token outside any functional pseudo-class.

3. Gating the nesting guard on a marker class dropped true positives with the false one

T:234 — if (!rule.selectors.some((selector) => markerOf(selector))) return;

The false positive was real, but so is what went with it. A/B against the parent: @media print { .text-spacing .para { margin-left: 0 } } and @media print { .psc-gutter-markers .para { --para-indent: 99vw } } both fail on 2be2fe86 and pass here — the second is a direct at-rule override of the variable this suite exists to police. Gating on scope rather than marker class keeps both properties; verified this restores the true positive, keeps your false positive fixed, and leaves the baseline green:

if (!rule.selectors.some((s) => markerOf(s) || s.includes("text-spacing"))) return;

4. isBase/isGutter are per-rule but used as if per-selector, so the new guard never fires where it was needed

T:255 — const isBase = markers.some(({ selector }) => isBaseSelector(selector));

baseValues classifies per selector; these flags collapse a selector list into one verdict. Probe: .formatted-font .usfm_zz, .text-spacing .usfm_q1 { margin-left: 15vw } passes — isBase is true via the q1 half, so !isBase && !isGutter never fires and zz's real 15vw margin is never derived or compensated. The guard doesn't close the hole it was added for; it needs to run per selector. The mirror arm at T:266 has the same defect (a base-half margin in a mixed selector list is rejected as "a compensation rule may set only…"), though that shape is contrived enough that I'd fix it only as a side effect of fixing this one.

5. The :not() direction flip only fires when :not( is immediately adjacent, and inverts the reading otherwise

T:182 — const negated = match && /:not\(\s*$/i.test(selector.slice(0, match.index));

A regex can't emulate a lookbehind past arbitrary compound content. Verified: :not(.foo[dir='rtl']) is not flipped and reads rtl — the opposite of intent — so an LTR-only margin lands in the RTL map and passes. The bare :not([dir='rtl']) form in your probe list happens to work. Separately :not([dir='rtl']) isn't semantically ltr: dir="auto" and unset both match. No selector in the sheet exercises this branch today, so deleting it is probably cleaner than fixing it.

6. The "descendant form reads as a gutter rule" limit isn't benign — that form is dead CSS

T:22 — * `.psc-gutter-markers .text-spacing` still reads as a gutter rule, and a rule with no marker class

Editor.tsx:1062 puts text-spacing, psc-gutter-markers and psc-active-focus in one className on one element, so the compound is load-bearing and the descendant form matches nothing. Probe: rewriting .psc-gutter-markers.text-spacing .usfm_p2 as .psc-gutter-markers .text-spacing .usfm_p2 leaves the suite green while \p2 silently loses compensation. The limit isn't "reads as a gutter rule" — it's "dead CSS counts as a marker's compensation", which wants a check, not a note.

7. Nothing in the suite reads the focus-box rule the new comments and assertion claim to pin

C:3012 — .psc-active-focus.psc-gutter-markers .psc-active-text::after {

The derivation only parses .text-spacing and gutter rules; it never reads .psc-active-focus. Probes, all green: deleting the block entirely; dropping - var(--para-indent, 0px) from the second operand; changing --psc-gutter-width from 4em to 2em. isWithin hard-codes the formula rather than deriving it from the rule, so the assertion and the stylesheet it describes can drift apart silently. That undercuts the stated reason for adding the check.

---

Medium

8. Dropping the early return turns a gutter-coverage test into a style policeman for 281 unrelated rules

T:253 — BLOCKS.forEach(({ rule, markers }) => {

The parent returned early for rules that were neither base nor gutter. Without it this test now polices 281 of the 507 .usfm_* rules in the sheet, including inline character markers that can never be a .para. Probes, passing on the parent and failing here: .formatted-font .usfm_nd { margin: 0 2px } → "is not derivable; use margin-left/right"; .formatted-font .usfm_nd { margin-left: 2px } → "the derivation is blind to it". Neither is true guidance for \nd. This is the change a maintainer is most likely to hit.

Worth pairing with #3 in the PR description: this commit narrowed one guard and widened another, in opposite directions, with no stated rationale for either.

9. Math.abs on the margin discards a sign the invariant depends on

T:333 — return Math.abs(Number(parsedIndent[1])) <= Math.abs(Number(parsedMargin[1]));

The indent is negative by definition so Math.abs on it is fine; on the margin it silently accepts a negative one. Probe: margin-left: -10vw with text-indent: -10vw and matching gutter values passes the whole suite, yet the second min() operand becomes calc(-1 * 4em - -10vw) = 10vw - 4em, positive above ~640px, so --verse-text-start wins and the box starts outside the editor — exactly what this check exists to rule out. Exotic input, but the Math.abs on the margin is unmotivated regardless.

10. "Known limits, each unreachable in today's stylesheet" is untrue of the limits the same paragraph adds

T:20 — * Known limits, each unreachable in today's stylesheet. Selectors are classified by class-name

Two listed limits are reached now. 20 blanket non-marker rules already set a tracked or banned property, including .psc-gutter-markers .book { --para-indent: 0px; --verse-text-start: 0px } (C:2726) — the very default the derivation assumes — and .text-spacing .verse { margin: 0px 2px 0px 2px; text-indent: 0in } (C:2515). And 13 base rules set a direction-agnostic margin-right (.usfm_ipi, .usfm_pmo, .usfm_pm, …), all symmetric margin-left: 5vw; margin-right: 5vw, which the header's last clause calls unreachable. This isn't just wording: "each unreachable" is the stated premise for the narrowing in #3.

11. usfm.sty is checked into this repo and already generated into a typed export — the oracle could be derived

T:102 — p2: "2.5vw", // 0.125"

The value is right, but the comment cites a remote GitHub URL while the file is checked in at tools/usfm-markers/src/generators/markers-data/data/usfm.sty:3110, and libs/shared/src/utils/usfm/defaultStyleInfo.ts is generated from it (p2.leftMargin: 0.125) and already exported — shared is already a platform devDependency. Deriving the oracle (×20 for vw, as generateUsjCss.ts does) would have covered p2 and every other marker with no hand edit, and would keep covering them across spec bumps. As written, the next re-sync silently invalidates a hand-maintained table whose comment points at the wrong copy of the spec.

12. Fourth hand-rolled number-and-unit parser in one file, and they already disagree

T:328 — const parse = (value: string) => /^(-?\d*\.?\d+)([a-z%]*)$/i.exec(value);

With T:271, T:315 and T:320 that's four. 0.0 is "not a plain length" to one and a valid zero to another; isNonZeroLength is a negated match, so calc(0px) and inherit both read as non-zero lengths. postcss-value-parser and postcss-selector-parser both resolve in this workspace (they'd need adding to platform's devDependencies, as postcss was). The selector parser in particular answers #2, #5, #18 and #19 structurally rather than by a hand-maintained character class.

13. Values are whitespace-collapsed but never trimmed, and postcss preserves trailing space on custom properties only

T:207 — value = node.value.replace(/\s+/g, " ");

Verified directly: --para-indent: 15vw ; → "15vw ", margin-left: 15vw ; → "15vw". One stray space before a semicolon in a gutter rule fails ACTUAL != EXPECTED with a diff that looks identical on screen. unreadableDeclarations calls .trim() at T:271, so the two paths disagree about the same value.

14. isWithin drops the gutter term, so it is stricter than the claim it pins — and reports bare tuples

T:327 — function isWithin(indent: string, margin: string): boolean {

Real threshold is |indent| < gutter + margin; this asserts |indent| <= margin. Probe: margin-left: 15em with text-indent: -10vw fires hangingBeyondMargin though the real min() resolves to -19em and the comment's claim still holds. The failure reads expected [ [ 'zz15', '-10vw' ] ] to deeply equal [] — a bare tuple, where every other check here builds an explanatory sentence. Unit mismatches are also reported as "beyond margin", conflating "cannot evaluate" with "violates". The conservatism may be deliberate (the exact condition mixes vw and em and flips with viewport width) but that reasoning appears nowhere, which is why the next reader will "fix" it.

15. The length gate rejects valid CSS, and the new /i trades a clear message for a 13-row diff

T:271 — if (!/^-?\d*\.?\d+[a-z%]+$|^0$/i.test(node.value.trim()))

Verified rejected: +5vw, -0, 1e3px, 0.0, 00 — all valid lengths, all hard-failing the first test. Separately the /i is correct CSS (units are case-insensitive) but gutterValues/baseValues still compare raw strings, so 15VW now clears the gate and fails toEqual with a 13-row Map diff instead of the parent's one-line "is not a plain length". Fix by case-normalizing in declarationValue, not by reverting the /i.

16. A .usfm_ token inside an attribute selector throws at module scope and takes all four tests with it

T:162 — const tokens = [...selector.matchAll(/\.usfm_[^\s.:#[>+~,)]*/g)].map((match) => match[0]);

The token class excludes ) but not ], quotes, =, * or \. Probe: .text-spacing .usfm_q1[data-x="a.usfm_zz"] { margin-left: 15vw } throws Cannot read the marker class ".usfm_zz"]", and because BLOCKS is built at module scope the file aborts with Test Files 1 failed and no test count rather than one test naming the selector. Other verified throwers: .usfm_q1*, .usfm_q1], .usfm_q1\:hover — the last being the shape generateUsjCss's escapeCssIdentifier emits.

17. Only the last token is strict-validated, so the docstring's promise holds in one position

T:165 — const match = /^\.usfm_([A-Za-z0-9-]+)$/.exec(last);

The docstring says a .usfm_ token that isn't a whole class name throws. Verified: markerOf(".usfm_a_bad .usfm_q1") returns q1 silently; markerOf(".usfm_q1 .usfm_a_bad") throws. Validate every token, then take the last.

18. The case-insensitivity flag requires whitespace the grammar doesn't

T:171 — const DIRECTION_SELECTOR =

(?:\s+[is])? needs a space, but [dir="rtl"i] is valid CSS. Verified: [dir="rtl"i] → agnostic, [dir="rtl" i] → rtl, while the doc comment claims "with or without a case flag". Substring matchers ([dir|=], [dir^=]) are likewise agnostic to directionOf but qualified to isDirectionQualified — two definitions of "mentions direction" now hand-synced.

19. Non-global exec reads the first direction mention, so an ancestor beats the subject

T:179 — const match = DIRECTION_SELECTOR.exec(selector);

Verified: [dir="ltr"] .text-spacing [dir="rtl"] .usfm_q1 → ltr, where the old code preferred rtl. The innermost scope is what applies, so an RTL-only margin filed as LTR leaves the RTL gutter uncompensated with the suite green.

20. The nesting report blames the wrong rule and double-reports the same declaration

T:239 — found.add(`${property} inside a nested rule: ${rule.selector}`);

Probe on .text-spacing .usfm_zz21 { & .usfm_yy21 { margin-left: 9vw } } emits two entries: "margin-left inside a nested rule: .text-spacing .usfm_zz21" — blaming the outer rule, whose own body is fine — and "margin-left inside a nested rule: & .usfm_yy21", a nesting-syntax selector that can't locate anything on its own. Should name decl.parent once. The Set is quietly absorbing the duplicate, and where it does collapse, two genuinely different at-rule violations under one selector become one message locating neither.

21. The docstring's own example passes only by accident; the near-identical one fails

T:229 — * so an unrelated `@media print { .usfm .note { margin: 0 } }` does not trip this.

The gate asks "does any selector contain .usfm_", not "would the derivation read this rule". Your example passes only because .usfm has no underscore. Probe: @media print { .usfm_q1 .note { margin: 0 } } fails, though the subject is .note and the margin is nothing the derivation reads. An example that works for the wrong reason makes the guard look narrower than it is.

22. --verse-text-start is inert in both modes, and this PR freezes that in place

C:2979 — --verse-text-start is a fallback start that no marker in this file wins today.

Verified: all 13 setters are scoped under .psc-gutter-markers, and there are exactly 2 readers. So the non-gutter reader at C:2998 can only ever resolve to 0px; and in gutter mode the min() never picks it — which your new hangingBeyondMargin assertion now pins forever. The variable does nothing for the focus box in either mode, and the PR responds with prose about it plus a test guaranteeing it stays that way, keeping ~60 lines of setters (C:2913–2973) and the machinery across three hand-synced copies. Pick a depth: move the setters off .psc-gutter-markers so the non-gutter rule does what it was written to do, or delete the min(), the setter block and the machinery.

23. The comment rewrite kept the mechanism and dropped the consequence

C:2980 — Without the gutter class that variable defaults to 0px. */

The removed sentence ("so poetry alignment is approximate") carried a consequence; the replacement carries only a mechanism. With the focus box on and the gutter off — independent flags at Editor.tsx:1062 — a \q1 paragraph's first line hangs ~10vw outside a box starting at 0px. Verified: grep -c approximate usj-nodes.css now returns 0, so nothing records that this is known and accepted rather than undiscovered.

24. margin-inline-end can never affect the derivation, so banning it is a pure false positive

T:135 — "margin-inline-end",

It's the trailing margin; the derivation only ever reads inline-start. Probe: .text-spacing .usfm_q1 { margin-inline-end: 3vw } now fails with "is not derivable; use margin-left/right" — a property that cannot change any derived value, and advice pointing at margin-right, which is exactly the spelling BASE_INLINE_START_MARGIN misreads as inline-start per the file's own documented limit. Drop it from the set.

---

Triage

Before merge: 1, 2, 3, 4 — each is a one-to-three-line change restoring a guarantee the parent had or that this PR claims.
Needs a reply even if the code doesn't change: 6, 7, 10, 22 — these are about the PR's own premises.
Will annoy the next person to touch an unrelated character-marker rule: 8, 24.

No files were modified; all probes ran against a scratch copy.

@katherinejensen00 reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on irahopkinson).

---

packages/platform/src/usj-nodes.css.gutter-coverage.test.ts line 206 at r1 (raw file):

  let value: string | undefined;
  rule.each((node) => {
    if (node.type === "decl" && node.prop.toLowerCase() === property)

High

1. Lowercasing declaration names makes the test accept a custom property the browser ignores

T:206 — if (node.type === "decl" && node.prop.toLowerCase() === property)

Custom property names are case-sensitive; only standard properties aren't. The new .toLowerCase() (here, T:237, T:258) erases the distinction. Probe: spelling a real gutter declaration --PARA-INDENT: 15vw leaves all four tests green while the browser ignores it, var(--para-indent, 0px) falls back to 0px, and the glyph lands in the text. The parent failed this correctly. Fix: decl.prop.startsWith("--") ? decl.prop : decl.prop.toLowerCase().

[irahopkinson]

This PR and paranext-core#2827 are at the same head, helper for helper, and your second round on #2827 rated that head "very close" with three items. I have applied the same bar here: taken the three, plus the one-line fixes from this list that are strict improvements, and declined the rest. Eight of 24 taken.

Taken

• 1, 10 as on #2827: custom properties keep their case; the Known-limits header is a list split into "in play today, benign" and "not reached".
• 2. A marker class inside :not()/:is()/:has() is refused rather than attributed to whichever token comes last. .usfm_q1:not(.usfm_qr) and :is(.usfm_q1, .usfm_zzy) both fail. Refusing is louder than attributing to the leftmost subject and neither sheet uses the form.
• 9. The margin must be positive; no Math.abs on it.
• 13. Values are trimmed as well as collapsed.
• 20. The nesting report names the nested rule or at-rule once (& .usfm_yy21 in your probe).
• 23. "so poetry alignment is approximate" is back in all three copies.
• 24. margin-inline-end is out of the unreadable set. You asked for it back in #2827's 7; this finding is the right one, it is the trailing margin in both directions.

Declined, as diminishing returns. Each of these is a hypothetical CSS form that no hand re-sync of this file has produced, and the stylesheets themselves have been correct since #10. 3 (the marker gate; note your suggested predicate misses your own .psc-gutter-markers .para probe, since that selector has no text-spacing), 4 (per-selector isBase/isGutter), 5 (:not() flip on a compound; no selector reaches it), 6 (descendant form as a check rather than a note), 7 and 14 (parse the focus-box rule; isWithin is the sufficient condition for any positive gutter width, which the TSDoc now says), 8 (this is the guard you asked for in #2827's 14; it reports character markers only when they set a tracked property, which none do), 11 (the oracle stays a spot check because the sheet deviates from usfm.sty deliberately in places), 12 and 15 (a shared length parser), 16 and 17 (attribute selectors and per-token validation; module-scope throw is the intended loud failure), 18, 19, 21.

You have twice suggested postcss-selector-parser. If the substring helpers should go, that is one rewrite in both twins as its own PR, and it would close 3, 4, 5, 6, 16, 17, 18, 19 and 21 together. I would take that over another round of point fixes.

22. Real and pre-existing: every --verse-text-start setter is gutter-scoped, so the non-gutter focus box never sees a hanging indent. Filed as PT-4742 with both options you name; not this PR.

@irahopkinson+AI made 2 comments.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on katherinejensen00).

---

packages/platform/src/usj-nodes.css.gutter-coverage.test.ts line 206 at r1 (raw file):

Previously, katherinejensen00 wrote…

High

1. Lowercasing declaration names makes the test accept a custom property the browser ignores

T:206 — if (node.type === "decl" && node.prop.toLowerCase() === property)

Custom property names are case-sensitive; only standard properties aren't. The new .toLowerCase() (here, T:237, T:258) erases the distinction. Probe: spelling a real gutter declaration --PARA-INDENT: 15vw leaves all four tests green while the browser ignores it, var(--para-indent, 0px) falls back to 0px, and the glyph lands in the text. The parent failed this correctly. Fix: decl.prop.startsWith("--") ? decl.prop : decl.prop.toLowerCase().

Taken. propertyName folds only standard property names; a -- prefix keeps its case. Your --PARA-INDENT probe fails the suite again, and an upper-case MARGIN-LEFT still fails as before. Same change in paranext-core#2827.

[katherinejensen00]

@katherinejensen00 reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on irahopkinson).