feat(lint): add rotation_pivot_drift layout check#2741
Conversation
Catches a rotating element that should spin in place but pivots about the wrong point (e.g. a wheel whose spokes use a hardcoded/incorrect transformOrigin so they swing off-center while every existing check still passes). Cross-sample detection: __hyperframesRotationSample() reports each transformed element's bbox center + decoded rotation angle per layout sample; the driver accumulates them and detectRotationPivotDrift() flags any element that (a) is actually spinning (angle spread > 20deg across >=3 samples), (b) is size-stable (bbox width ratio <= 1.6, excluding scale/entrance growth), and (c) whose bbox center drifts > max(10% of its size, 2% of the min viewport dim). Emits a warning; exempt via data-layout-allow-orbit. Not persistence-tiered, so the warning is preserved (not demoted to info). Validated: fires on an off-transformOrigin spoked wheel (109px drift), clean on non-spinning compositions and on a correctly-centered spinner (drift 0). tsc/oxlint clean, 112/112 existing tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
miga-heygen
left a comment
There was a problem hiding this comment.
SSOT Review: rotation_pivot_drift lint check
PR: feat(lint): add rotation_pivot_drift layout check — 6 files, +239/-1
Architecture
Clean addition that follows the established cross-sample pattern (detectSweepStatic precedent):
-
Browser-side (
layout-audit.browser.js):__hyperframesRotationSample()collects every rotatable candidate's bbox center, size, and decoded rotation angle per seeked sample. Caps at 200 candidates, skips[data-layout-allow-orbit], filters by visibility and minimum area. -
Driver bridge (
checkBrowser.ts):collectRotationSample()evaluates the browser function, validates each field viaparseRotationSample(using existingisRecord/stringValue/numberValuehelpers — no new parsing invented). ReturnsRotationSample[]. -
Pipeline (
checkPipeline.ts): samples accumulated inGridSamples.rotationSamplesinside the mainmergeSampleTimesloop, grouped by selector post-run, thendetectRotationPivotDrift()runs the five-stage filter chain. -
Types (
checkTypes.ts):RotationSampleinterface, driver method signature. Issue code (layoutAudit.ts+checkBrowser.tsLAYOUT_ISSUE_CODES): both updated.
SSOT checks
-
RotationSampledefined once incheckTypes.ts, imported by pipeline and browser. No duplication. ✓ -
Issue code
rotation_pivot_driftregistered in both places — the type union (LayoutIssueCode) and the runtime code list (LAYOUT_ISSUE_CODES). ✓ -
rotationAngleDegextracts rotation frommatrix(a,b,...)viaatan2(b, a). Correct for bothmatrix()andmatrix3d()(first two values are the 2D rotation cosine/sine in both). Returnsnullfor non-transformed elements — browser-side skips them. ✓ -
maxAngleSpreadis wrap-aware.Math.min(raw, 360 - raw)picks the shorter arc. Edge case:atan2returns [-180°, 180°], so two angles -170° and 170° yieldabs(-170-170) % 360 = 340,min(340, 20) = 20°. Correct. ✓ -
No new helpers that shadow existing ones.
median()andmaxCenterDrift()are local to the pipeline file.medianis small and used only indetectRotationPivotDrift— not worth extracting to a shared util. ✓ -
Division-by-zero guard.
minWidth <= 0check beforeMath.max(...widths) / minWidth. ✓
FP guard chain (the real engineering here)
| # | Guard | Purpose |
|---|---|---|
| 1 | data-layout-allow-orbit |
Intentional orbits opt out (browser-side) |
| 2 | ≥3 samples |
Cross-sample confidence |
| 3 | angle spread > 20° |
Filters static tilts from real rotation |
| 4 | size ratio ≤ 1.6 |
Excludes scale/entrance animations |
| 5 | median area ≥ 2500px² |
Skips decorative spinners |
| 6 | drift > max(10% size, 2% viewport) |
Relative threshold, not absolute |
The 1.6 size ratio is well-motivated — a square's AABB naturally oscillates 1.41x between flat and 45°, and 8-spoke stars hit ~1.32x. The ceiling needs to sit above that. Documented in both code comments and PR body. ✓
Observations (non-blocking)
-
Unit tests for
detectRotationPivotDriftare deferred. The PR body calls this out as a follow-up ("via the fake driver'scollectRotationSample"). The fake driver stub is already wired (collectRotationSample: vi.fn(async (_time: number) => [])), so the follow-up has a clean landing spot. The five-stage filter chain and the tuning constants would benefit from direct unit coverage — I'd recommend prioritizing that follow-up. -
CANDIDATE_CAP = 200in the browser function is a reasonable safety valve. Worth noting: if a composition has >200 transformed elements, the cap could suppress legitimate findings on later-DOM-order elements. Unlikely in practice, but the cap is undocumented in the issue message — a composition hitting it would see silence, not a warning about being capped. Minor.
Blocking SSOT issues
None found.
Verdict
Approve. This follows the established cross-sample lint pattern precisely, the FP guards are thorough and well-motivated, the type surface is clean, and the browser→pipeline→issue code chain has no gaps. The deferred unit tests are called out and have a ready landing spot. Ship it.
— Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
Found one blocking false-positive path not covered by the existing review.
packages/cli/src/utils/checkPipeline.ts:580: the size-stability guard checks onlyw, even though the comment and PR contract say it excludes scale/entrance animations. A rotating element whose width stays at 100px while height scales 50→100→150 passes the ratio guard (1.0), has enough area and angle spread, and its AABB center can move 50px from a top-anchored scale. On a 1000px canvas that exceeds the 20px threshold, so this emitsrotation_pivot_driftand recommends a pivot rewrite even though the movement is caused by height-only scaling. Since the code explicitly notes that false positives feed destructive downstream auto-fixes, check stability in both dimensions (or use a scale-invariant rigid-size test) and pin this height-only scale case as clean.
The same complex five-stage filter currently has no direct positive/negative unit coverage, and the Fallow audit is failing on its uncovered CRAP score. Please add focused tests for a real bad pivot, a centered spinner, wrap-around angles, width/height scaling, and the threshold boundaries rather than deferring them.
— Magi
Verdict: Request changes.
Reasoning: The width-only stability guard admits height-only scale/entrance motion as pivot drift, risking a destructive false-positive auto-fix; both dimensions and direct regression coverage are required before approval.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 341a5f3ec1b0172d9b69b406f31238a50b31180d.
Clean cross-sample finder following the detectSweepStatic precedent. Miga's SSOT / matrix-decode / wrap-arithmetic audit covers the algorithmic verification well; concurring on those. Adding four deltas — one hitting current CI, three sub-blockers.
1. Fallow CI is failing on this PR's new function's CRAP score.
detectRotationPivotDrift hits CRAP 37.1 (threshold 30, cyclomatic 11) — see the sticky Fallow comment. Since fallow audit runs with --fail-on-issues and --gate new-only, this is a genuinely new-code finding and the job is exiting 1. Two ways out (inline):
- Factor the five-stage guard into named predicates (
isSpinning(group),hasStableSize(group),hasSizableMedian(group),driftExceedsFloor(group, canvas)) — drops cyclomatic below the threshold and reads a lot closer to the FP-guard table you already have in the doc-comment. - Or bump the CRAP threshold if the team has decided this class of guard-chain function is fine as-is.
2. Concurring with Miga on the two she surfaced: unit tests deferred (the fake driver stub is already wired, so the follow-up landing spot is clean — but 6 tunable thresholds without regression coverage is a real gap for future tweaks), and CANDIDATE_CAP=200 is a silent-drop pattern that could suppress legitimate findings on late-DOM-order rotating elements without any signal to the operator. Small inline notes on each.
3. Selector stability across seeks. selectorFor() falls back to parent > tag:nth-of-type(N) when an element has no id, no data-layout-*, and no classes. N shifts as siblings render/unrender across the seek grid, so two different rotating elements at different times can alias to the same selector and get grouped. Fringe (author-crafted rotating figures almost always have ids/classes) and the FP guards can't detect it. Worth documenting the invariant "grouped selectors are assumed stable across seeks" at the top of detectRotationPivotDrift.
Verified-safe on my pass (no finding):
matrix3d(...)values[0]/[1] correctly recover the in-plane rotation regardless of composed rotateY/rotateX (the leading two entries of the 4x4 are stillcos θ, sin θfor the Z-axis rotation component).element.closest("[data-layout-allow-orbit]")correctly includeselementitself — self-attributed opt-outs work.- Wrap-aware
maxAngleSpreadhandles atan2's ±180° discontinuity (-179°vs+179°reads as ~2°, not 358°). dataAttributes: {}on the finding matches the established pattern for cross-sample findings (sweep_static, motion_*, connector_detached all pass{}); not a regression.- Fake driver stub (
collectRotationSample: vi.fn(async () => [])) correctly satisfies the extendedCheckAuditDriverinterface. LayoutIssueCodeunion and runtimeLAYOUT_ISSUE_CODESarray both updated in the same PR — no serialization mismatch.
Nits (non-anchored):
- Cross-repo severity comment (
checkPipeline.ts:538, "EF promotes this to error separately") — worth a link/issue reference so future readers can trace the severity contract without hunting in EF. - The two size floors — browser
<= 400px²and pipeline< 2500px² median— are intentional (network-noise cutoff vs the real filter) but not tied together in prose; a brief pointer in the browser comment would make it obvious the "real" cutoff is downstream. fixHintprescribes "use svgOrigin/percent, not hardcoded px" but the DOM sample only sees the composed transform — the check flags the symptom (bbox drift), not the specific authoring cause. Softening to "check transformOrigin/svgOrigin" (matching themessagefield's phrasing) would be more accurate.
LGTM from my side once Fallow is settled and the deferred unit-test follow-up has a tracker. Leaving as a comment — you can stamp when ready.
| * (excludes scale/entrance animations), a sizable element, and honors | ||
| * `[data-layout-allow-orbit]` opt-outs (applied browser-side). | ||
| */ | ||
| function detectRotationPivotDrift( |
There was a problem hiding this comment.
🟠 Fallow CI is failing here — CRAP 37.1 vs threshold 30 (cyclomatic 11). The --gate new-only + --fail-on-issues combo means this is genuinely new-code complexity that blocks CI, not tool noise.
Two ways out:
- Factor the five-stage guard into named predicates that mirror your doc-comment's FP-guard table — something like:
That drops cyclomatic below the threshold and reads closer to the guard-table.
for (const group of bySelector.values()) { if (!hasEnoughSamples(group)) continue; if (!isSpinning(group)) continue; if (!hasStableSize(group)) continue; if (!hasSizableMedian(group)) continue; const drift = maxCenterDrift(group); if (drift <= driftFloor(group, canvas)) continue; const finding = rotationDriftFinding(group, drift); if (finding) findings.push(finding); }
- Or bump the CRAP threshold if guard-chain finders are considered fine as-is.
Not a design blocker — it's just CI arithmetic — but the current SHA won't merge until it's resolved.
— Rames D Jusso
| const widths = group.map((s) => s.w); | ||
| const minWidth = Math.min(...widths); | ||
| if (minWidth <= 0 || Math.max(...widths) / minWidth > ROTATION_MAX_SIZE_RATIO) continue; | ||
| if (median(group.map((s) => s.w * s.h)) < ROTATION_MIN_MEDIAN_AREA_PX) continue; |
There was a problem hiding this comment.
🟡 Deferred unit tests for the detector — concurring with Miga, adding rationale.
The function has 6 tunable thresholds (ROTATION_MIN_SAMPLES, ROTATION_MIN_ANGLE_SPREAD_DEG, ROTATION_MAX_SIZE_RATIO, ROTATION_MIN_MEDIAN_AREA_PX, ROTATION_DRIFT_SIZE_FRACTION, ROTATION_DRIFT_VIEWPORT_FRACTION) whose values are load-bearing for FP suppression. Without direct unit coverage, any future tweak (e.g. tightening 1.6→1.4) lands silent — CI can't tell whether legitimate targets still fire or whether the FP-guard corners still hold.
A table-driven test covering the guard-chain corners (below-min-samples, static tilt, size-not-stable, tiny-median-area, drift-below-threshold, positive-fire) would pin the current tuning as the SSOT. The fake driver stub (check.test.ts:152) already gives you the entry point; consider prioritizing this before the check ships to enough compositions that a threshold drift becomes hard to bisect.
— Rames D Jusso
| const samples = []; | ||
| // Cap the candidate set so a pathological composition can't blow up the | ||
| // per-sample payload; transformed elements above a minimum area only. | ||
| const CANDIDATE_CAP = 200; |
There was a problem hiding this comment.
🟡 CANDIDATE_CAP = 200 is a silent-drop — concurring with Miga.
If a composition has >200 transformed visible elements, DOM order determines who survives; anyone past the cap gets no rotation sample, no warning, no telemetry. Unlikely for author-crafted comps, but the mode of failure is silent (a legitimate rotating spoked-wheel late in the DOM would just… not fire).
A couple of cheap mitigations if you want to close it in this PR:
- Sort candidates by bbox area desc before capping — largest rotating figures survive first, which is where the real prod-case lives.
- Or surface a one-line info-level finding when
samples.length === CANDIDATE_CAPso the operator at least sees "rotation sampling was capped, some elements were skipped." - Or raise the cap — 200 is arbitrary; if the whole payload is
{selector, cx, cy, w, h, angle}(~50 bytes/entry) even 1000 candidates is <50KB per sample.
Nit-shaped, but leaving here so it doesn't get lost.
— Rames D Jusso
| const box = element.getBoundingClientRect(); | ||
| if (box.width * box.height <= 400) continue; | ||
| samples.push({ | ||
| selector: selectorFor(element), |
There was a problem hiding this comment.
🟡 selectorFor() nth-of-type fallback isn't guaranteed stable across seeks.
When an element has no id, no data-layout-name/data-composition-id/data-start, AND no classes, selectorFor falls back to parent > tag:nth-of-type(N) (see layout-audit.browser.js:80-84). N counts live siblings at the current seek — so if a rotating element's sibling renders/unrenders across the seek grid (very common with entrance/exit animations), N shifts, and the same rotating element can collect samples under two different selectors OR two different rotating elements can alias to the same selector.
Cross-contamination case: Element A rotates from t=0-2 (nth=3). Element B rotates from t=1-3 (nth=3 after A exits). They'd be grouped together under identical parent > tag:nth-of-type(3), and the maxCenterDrift across A and B combined can trip the finding falsely.
Fringe (author-crafted rotating figures almost always have ids/classes/data-layout-name), and the FP guards can't detect it. Worth a top-of-function invariant note like:
// Assumes selector stability across seeks — rotating elements without stable ids/classes are on their own here.
Or, if you want to close it structurally, gate the browser-side sampler to only report elements where selectorFor didn't hit the nth-of-type fallback.
— Rames D Jusso
…pivot_drift Review fixes for the size-stability false positive and Fallow CRAP score. - isRotationSizeStable now bounds the bbox ratio in BOTH width and height. The width-only guard let a top-anchored height-scaling element (fixed width, growing height) through: its AABB center moves on its own, so it emitted rotation_pivot_drift and recommended a destructive pivot rewrite. - Factor the guard chain into named single-decision predicates (hasEnoughRotationSamples / isActuallySpinning / isRotationSizeStable / isSizableRotation) so detectRotationPivotDrift reads as a candidate filter + drift test. Drops the CRAP score (complexity + coverage). - Export detectRotationPivotDrift and add direct positive/negative coverage for all five filter stages, including the height-scaling regression. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed 1. Size-stability FP (width-only guard) — fixed. 2. Fallow CRAP (37.1 → under threshold) — refactored + covered. Split the guard chain into named single-decision predicates ( Non-blocking, acknowledged as follow-ups:
|
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at 6aff573 — my earlier comment was at 341a5f3. Prior blockers verified resolved; one new CI failure now gates merge.
Prior blockers — resolved
- Miguel's width-only size-stability FP (the
CHANGES_REQUESTEDblocker): fixed.isRotationSizeStable(checkPipeline.ts:591) now bounds the bbox ratio on both axes (maxW/minW ≤ 1.6 && maxH/minH ≤ 1.6) with aminWidth <= 0 || minHeight <= 0degenerate guard. The exact path he called out — fixedw=100, top-anchoredh=50→100→150— now fails the height ratio (3.0 > 1.6) and is dropped before the drift test. Pinned by thedoes not fire when height scales (top-anchored)…regression test. ✓ - Fallow CRAP (my CI blocker at
341a5f3): resolved —Fallow auditis green on this SHA. The guard chain is split intohasEnoughRotationSamples/isActuallySpinning/isRotationSizeStable/isSizableRotationbehindisRotationDriftCandidate, sodetectRotationPivotDriftnow reads as candidate-filter → drift-test and the cyclomatic score drops under threshold. ✓ - Deferred unit tests (raised by Miga, Miguel, and me): resolved — new
checkPipeline.rotationPivotDrift.test.ts(11 cases) exercises all five guard stages, both size axes, the degenerate zero-dim case, the viewport-floor path, multi-selector independence, and empty input.detectRotationPivotDriftis exported for direct testing. ✓
New blocker — gating merge
Preflight (lint + format)is RED on this SHA:oxfmt --checkfails onpackages/cli/src/utils/checkPipeline.ts("Format issues found in above 1 files", exit 1 — job log).oxlintitself is clean (0 warnings / 0 errors), so the PR comment's "oxlint clean" is accurate butformat:checkwas missed. Runbun run format(oxfmt) oncheckPipeline.tsand push — this is the only code-side thing blocking merge.
Still pending / notes
- The
CIworkflow is stillin_progress:Lint,Fallow audit,SDK, andFile size checkare green, butTest/Typecheckhaven't finished, so the "11 new + 40 existing pass" claim isn't yet confirmed by CI. Worth a glance once it lands. preview-regressionis red becausepreview-parityreturnedskipped(PREVIEW_PARITY_RESULT: skipped) and the gate fails closed — the parity job didn't execute (infra/approval), not a code defect. Should re-run green after the reformat push.- Minor coverage gap: of Miguel's explicit test list, wrap-around angles is the one case not pinned by the new unit tests (angles used are 0/90/180 etc., no ±180° boundary). The wrap arithmetic in
maxAngleSpreadis verified correct by inspection (Miga's audit + my earlier pass), so this is nice-to-have — one extrait()with e.g.angle: -170 → 170would close it. Not blocking.
Verdict: COMMENT
Reasoning: Every prior design blocker (Miguel's dual-axis FP guard, my Fallow CRAP, and the deferred unit tests) is resolved and verified at 6aff573. Not approving because a required check is red — merge is gated on oxfmt/Preflight going green (plus Miguel clearing his standing CHANGES_REQUESTED and CI Test finishing). On the code merits this is ready once formatting is fixed.
— Jerrai
- Run oxfmt on checkPipeline.ts (fixes the red Preflight format:check; oxlint was already clean). - Soften message/fixHint to phrase the finding as the symptom (bbox-center drift, "check transformOrigin/svgOrigin") rather than prescribing a specific authoring cause the DOM sample can't confirm. - Move the detector doc-comment above detectRotationPivotDrift and document the cross-seek selector-stability invariant (nth-of-type fallback can mis-group a classless rotating element). - Add wrap-around angle-spread tests: a ±180° boundary wobble is not spinning; a real spin crossing the seam still fires. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed Merge blocker — In-file nits (@jrusso1020 / @james-russo-rames-d-jusso) — done in this commit:
Still deferred (non-blocking, tracked):
Verified locally at |
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Delta from 341a5f3ec → e53e1a40886.
Every R1 open item closed:
- Fallow CRAP. Guard chain factored into 5 named predicates (
hasEnoughRotationSamples/isActuallySpinning/isRotationSizeStable/isSizableRotation/isRotationDriftCandidate);Fallow auditis now green. - Unit tests. 12-case
checkPipeline.rotationPivotDrift.test.ts(137 lines) covering all five guard stages plus the degenerate zero-dim path, the viewport-floor branch, multi-selector independence, and empty input. The tuning constants are now pinned by regression coverage. - Selector-stability invariant. Documented in the
detectRotationPivotDriftdoc-comment with the exact fringe (nth-of-type shift on classless elements) and the structural fix (stable-anchor gate on the browser sampler) called out for a future follow-up. - Magi's dual-axis FP.
isRotationSizeStable(checkPipeline.ts:575) now bounds bothwandhratios plus aminWidth <= 0 || minHeight <= 0degenerate guard; the top-anchored height-scale regression is pinned bydoes not fire when height scales (top-anchored)…. - James's
oxfmtgate.Preflight (lint + format)now green. - James's wrap-around coverage nice-to-have. Xuanru went further and added BOTH the
±180° boundary wobble(should NOT fire) andreal spin that crosses the ±180° boundary(SHOULD fire) tests.
CANDIDATE_CAP = 200 and the fixHint / EF-severity / two-tier-floor nits weren't taken — all author-choice territory; not blocking.
CI at e53e1a40886 is fully green (37 SUCCESS, 8 SKIPPED, 0 in-progress, 0 failed). mergeStateStatus is BLOCKED only because Magi's R1 CHANGES_REQUESTED is still standing — she'll want to re-review or dismiss to unblock the merge.
LGTM from my side.
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed the exact head e53e1a40886faafdda6f28f7382db9469c05b0e6 against my R1 blocker and the full follow-up delta.
The false-positive path is closed: size stability now checks both axes, so the height-only top-anchored scaling case that previously looked like pivot drift is rejected, and that exact regression is pinned in the new direct suite. The factored guard chain is substantially easier to audit, and the focused coverage now exercises the real drifting case, centered rotation, insufficient/low-angle samples, width and height scaling, zero/tiny geometry, viewport-floor behavior, selector isolation, and both near-seam wobble and a real rotation crossing ±180°.
Local exact-head verification: 125 focused tests passed (checkPipeline.rotationPivotDrift, check, and browser layout-audit suites), CLI typecheck passed, changed-file oxlint/oxfmt checks passed, and git diff --check passed. GitHub is also fully green at this head: 37 successful checks, 8 intentionally skipped, 0 failed or in progress. No new findings.
— Magi
Verdict: APPROVE
Reasoning: The dual-axis guard and direct boundary-focused tests close the prior correctness blocker, while the refactor clears the complexity gate without changing the intended detection pipeline; exact-head local and CI verification are green.

What
New cross-sample layout check
rotation_pivot_drift— flags a rotating element that should spin in place but pivots about the wrong point (e.g. a wheel whose spokes use a hardcoded pxtransformOrigininstead ofsvgOrigin/%, so they swing off-center while every existing check still passes).Motivating prod case: a portrait ad's spoked-wheel whose
#spokesrotated abouttransformOrigin:"250px 250px"in a resized 460px container — spokes detached from the hub, shipped clean because no rule inspects rotation.How
layout-audit.browser.js:window.__hyperframesRotationSample()reports each visible transformed element's bbox center + decoded rotation angle per layout sample. Skips[data-layout-allow-orbit].checkPipeline.ts: accumulates samples across the seek grid;detectRotationPivotDrift()(modeled ondetectSweepStatic) flags an element that (a) actually spins (angle spread > 20° over ≥3 samples), (b) is size-stable (bbox width ratio ≤ 1.6), and (c) whose bbox center drifts >max(10% of its size, 2% of min viewport dim). Emitswarning; not persistence-tiered (not demoted to info).FP guards
Real rotation required, ≥3 samples, size stability,
data-layout-allow-orbitexemption, min area ~2500px². Center-drift (not bbox size) is the discriminator, so a correctly-centered spinner reads drift ≈ 0.Validation (
check --json)#spokessvgOrigin)data-layout-allow-orbitoff-origin spinnersvgOrigincontrol, no opt-outNo false positives.
tsc --noEmitclean,oxlintclean,check.test.ts+layout-audit.browser.test.ts= 112/112 pass.Note
ROTATION_MAX_SIZE_RATIOis 1.6 (not 1.3): a rotating anisotropic shape's axis-aligned bbox inherently oscillates (8-spoke star ~1.32×, square 1.41×), so a tighter ratio rejects legitimate targets. Center-drift stays the real discriminator; thin swinging bars are excluded.Follow-up: a
detectRotationPivotDriftunit test via the fake driver'scollectRotationSample(mirroring the sweep_static tests).🤖 Generated with Claude Code