Skip to content

feat(core): add baseline/rhythm rule - #207

Merged
aram-devdocs merged 5 commits into
mainfrom
issue-73-baseline-rhythm-textboxes
May 2, 2026
Merged

feat(core): add baseline/rhythm rule#207
aram-devdocs merged 5 commits into
mainfrom
issue-73-baseline-rhythm-textboxes

Conversation

@aram-devdocs

Copy link
Copy Markdown
Owner

Summary

  • Adds the baseline/rhythm rule that flags text elements whose typographic baselines miss the configured vertical-rhythm grid (rhythm.base_line_px).
  • Computes approximate baseline position from font-size, line-height, and bounding rect; fires when distance to nearest grid line exceeds rhythm.tolerance_px.
  • Includes golden snapshot test, determinism test, rule docs page, SUMMARY.md and CHANGELOG entries.

Closes #73

Validations

  • Rule module + registration in register_builtin (alphabetical order)
  • Golden snapshot test (golden_baseline_rhythm_golden) with on-grid, off-grid, and non-text nodes
  • Determinism test (baseline_rhythm_run_is_deterministic) — 3-run byte-identical check
  • Rule docs at docs/src/rules/baseline-rhythm.md with all required sections
  • docs/src/SUMMARY.md updated with alphabetically placed entry
  • CHANGELOG [Unreleased] entry added
  • git diff --check — no whitespace issues
  • No unwrap/expect/todo!/dbg!/println! in library code
  • Pure function of (snapshot, config) — no I/O, no wall-clock, no HashMap
  • Uses IndexMap for metadata, parse_px from rules::util

Test plan

  • cargo nextest run -p plumb-core --test golden_baseline_rhythm passes both golden and determinism tests
  • just validate passes (fmt + clippy + full test suite)
  • CI green

🤖 Generated with Claude Code

Add the `baseline/rhythm` rule that flags text elements whose
typographic baselines miss the configured vertical-rhythm grid.

The rule computes an approximate baseline position from font-size,
line-height, and the bounding rect, then checks distance to the
nearest multiple of `rhythm.base_line_px`. Violations fire when
the distance exceeds `rhythm.tolerance_px`.

Includes golden snapshot test, determinism test, rule docs, SUMMARY
and CHANGELOG entries.

Closes #73

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented May 2, 2026

Copy link
Copy Markdown
Contributor

I have all the information needed. Let me write the review.


PR #207 Review — feat(core): add baseline/rhythm rule

File buckets touched: plumb-core (rule, snapshot, tests), plumb-cdp (text-box extraction), plumb-cli (selector rewrite), plumb-mcp (explain), plumb-format (SARIF snapshot), docs, ci/schema.


1. Determinism

  • IndexMap used for metadata
  • No HashMap/HashSet in observable output ✓
  • text_boxes sorted (dom_order, start) before return in extract_text_boxes
  • Engine sort key (rule_id, viewport, selector, dom_order) is stable because Rust's sort is stable and insertion order within identical keys follows sorted text_boxes
  • No wall-clock, no env reads in plumb-core

2. Blocker — multiple violations per node per viewport

crates/plumb-core/src/rules/baseline/rhythm.rs:342-393

The loop over y_origins emits one violation per off-grid text-box line. When a 4-line paragraph has 2 off-grid lines, the rule fires twice with identical (rule_id, viewport, selector, dom_order). This directly violates the documented invariant:

.agents/rules/rule-engine-patterns.md:70: "Don't emit more than one violation per offending node per viewport. Duplicate detection is caller-side; upstream fan-out burns token budget."

The existing tests don't catch this because baseline_rhythm_multiline_text_boxes is constructed with exactly one on-grid and one off-grid line — the double-off-grid case is untested.

Required fix: aggregate all off-grid lines into a single violation per node. The off-grid line details (baseline_y, distance, nearest grid) can go in metadata (e.g., "offgrid_lines": [...]) or the message can reference the worst/first offender.

3. Silent precondition in text_box_index

crates/plumb-core/src/snapshot.rs:505-518

text_box_index requires text_boxes to be sorted by (dom_order, start). The field is pub and serde-deserialized, so any caller loading a JSON snapshot with unsorted text boxes gets silently wrong text_boxes_for results — wrong index slices, potentially no violations where violations should fire. CDP output is always sorted, but test-fixture snapshots built from JSON could easily violate the invariant.

Required fix: either sort defensively in SnapshotCtx::new, or validate + return an error, or at minimum add a debug_assert that the slice is sorted so failures surface in test runs.

4. Missing double-off-grid test

Follows from the blocker above: the baseline_rhythm_multiline_text_boxes test cannot demonstrate the invariant violation because it only puts one line off-grid. The test suite needs a case where two text-box lines are off-grid for the same node, and asserts violations.len() == 1 after the fix lands.


Non-blocking observations

  • Cargo.lock shows subtle re-entering the plumb-mcp section, but it's already in crates/plumb-mcp/Cargo.toml:31 and lib.rs:71 for constant-time token comparison. This is a pre-existing dep; the lock-file churn is just regeneration. No concern.
  • #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] in plumb-cdp/src/lib.rs:118-121 is narrowly scoped per-line and inside the unsafe-permitted crate with .max(0) guards. Acceptable.
  • TEXT_TAGS.contains(&node.tag.as_str()) is O(31) linear scan in the hot loop. Fine under the "no new deps for a single rule" policy, but could become const PHFS if this ever shows in profiles.
  • Violation rect is the element bounding box, not the off-grid text-box line rect. Intentional or not, it means the fix hint can point to the right grid line but the rect highlights the whole element. Worth noting in follow-up.

Punch list:

# File:line Issue Class
1 crates/plumb-core/src/rules/baseline/rhythm.rs:341-393 Loop emits N violations per node; one-per-node invariant violated BLOCKER
2 crates/plumb-core/src/snapshot.rs:505-518 text_box_index silently wrong if text_boxes unsorted at deserialize time BLOCKER
3 crates/plumb-core/tests/golden_baseline_rhythm.rs Missing double-off-grid multiline test Warning

Verdict: REQUEST_CHANGES

aram-devdocs and others added 4 commits May 2, 2026 20:31
Remove philosophical phrasing ("consistent cadence that readers perceive
as orderly even without consciously noticing") and tighten the rationale
to plain, direct language.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add TextBox type to PlumbSnapshot and extract per-line text fragments
from CDP's document.textBoxes. The baseline/rhythm rule now checks each
rendered line independently instead of approximating from the element
bounding rect, giving accurate results for multi-line and mixed-font
text. Includes golden tests for multi-line and multi-font scenarios.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses three review items on PR #207:

1. Collect all off-grid lines per node and emit a single aggregated
   violation using the worst-distance line as primary metadata.
   Multi-line nodes get a summary message format.

2. Add debug_assert in text_box_index to catch unsorted text_boxes
   in test builds.

3. Add baseline_rhythm_multiline_both_off_grid test asserting exactly
   one violation when both lines are off-grid.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@aram-devdocs
aram-devdocs merged commit a1b32eb into main May 2, 2026
15 of 16 checks passed
aram-devdocs added a commit that referenced this pull request May 12, 2026
…292)

* fix(ci): unstick Preflight + tolerate Dogfood warnings/nextjs flake

PR #287 deleted docs/src/ci/release-prep.md and removed the matching
guard in tests/install-smoke-validate.sh, but missed the parallel guard
in tests/release-security-validate.sh. The validator has been failing
on every CI run since 2026-05-07, skipping every downstream job (MSRV,
deny, Test, Size, Coverage, Determinism, Docs).

Drop the stale RELEASE_PREP_DOC declaration and the corresponding
sentinel-grep block. The Homebrew + npm contracts are still validated
authoritatively against dist-workspace.toml and install-smoke.yml in
sections 5 and 6 of the same script.

Two follow-on dogfood fixes since the daily run has been red since
2026-05-02 (after the Phase 7 rule additions in #202/#204/#207):

- lint-canonical-docs now tolerates exit 3 (warnings/info only)
  alongside exit 0, mirroring the test-sites leg in the same workflow.
  Errors still fail. The 318-violation steady state on the live docs
  site is a separate content/theme cleanup, not a CI bug.
- nextjs leg gets continue-on-error on both plumb invocations to mirror
  the e2e-sites stance on the upstream chromiumoxide WebSocket flake
  tracked in #233.

Also drops the dead docs/src/ci/release-prep.md reference from the
install-smoke.yml header comment.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(cdp): gate macos_candidate ordering test to unix

The `macos_candidate_order_lists_system_apps_then_user_apps` test in
crates/plumb-cdp/src/chrome_path.rs asserts that paths returned by
`macos_candidates` start with `/Users/example/Applications/...` —
forward slashes against `Path::display()`. On Windows, `Path::join`
uses `\`, so display[3] becomes
`/Users/example\Applications\Google Chrome.app\...` and the
starts_with check fails.

`detect_with` short-circuits to `None` on non-macOS targets, so the
function is never reached in production on Windows. Gate the test to
`#[cfg(unix)]` to keep the existing Linux coverage and drop the
unreachable Windows leg.

This test was previously masked because Preflight has been failing on
every CI run since 2026-05-07, which skipped Test (windows-latest)
entirely. Surfacing now as a side effect of the validator fix in this
same PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(cdp): gate macos_candidates import to unix in tests

Follow-up to 890b14c. Gating the test to `#[cfg(unix)]` left
`macos_candidates` as an unused import on Windows, breaking the
`Test (windows-latest)` build with `error: unused import:
\`macos_candidates\``. Mirror the gate on the import.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(core): rule baseline/rhythm (font metrics + CDP textBoxes)

1 participant