Skip to content

feat(lint): add connector_motion_detached layout check #2745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xuanruli wants to merge 4 commits into
base: xuanru/content-overlap-dense-resampling
Choose a base branch
from
Open

feat(lint): add connector_motion_detached layout check #2745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
46d0847
feat(lint): add connector_motion_detached layout check
xuanruli Jul 23, 2026
c963ede
fix(lint): connector_motion_detached anchors to SVG hub/ring shapes
xuanruli Jul 23, 2026
3b35be0
fix(lint): exclude gauge needles/pointers from connector_motion_detached
xuanruli Jul 23, 2026
70e6026
fix(lint): sustained attachment + path/ring hub nodes for connector_m…
xuanruli Jul 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/cli/src/commands/check.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ function fakeDriver(overrides: Partial<CheckAuditDriver> = {}): CheckAuditDriver
collectLayoutGeometry: vi.fn(async () => `geometry-${geometryCallCount++}`),
collectRotationSample: vi.fn(async (_time: number) => []),
collectOffPivotRotationSample: vi.fn(async (time: number) => ({ time, samples: [] })),
collectConnectorSample: vi.fn(async (time: number) => ({ time, connectors: [], nodes: [] })),
collectGeometryCandidates: vi.fn(async () => []),
collectMotionFrame: vi.fn(async (time: number) => ({ time, data: {}, liveness: {} })),
anchorMotionIssues: vi.fn(async (issues: LayoutIssue[]) =>
Expand Down
192 changes: 188 additions & 4 deletions packages/cli/src/commands/layout-audit.browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1233,10 +1233,18 @@
const mapped = point.matrixTransform(matrix);
return { x: mapped.x, y: mapped.y };
};
return {
start: toScreen(path.getPointAtLength(0)),
end: toScreen(path.getPointAtLength(total)),
};
// getPointAtLength can still throw on a degenerate/malformed path even after
// getTotalLength succeeded. This sampler runs once PER seeked frame, so one
// bad path must degrade to "no endpoints" (skip this path), not abort the
// whole check for every remaining path and frame.
try {
return {
start: toScreen(path.getPointAtLength(0)),
end: toScreen(path.getPointAtLength(total)),
};
} catch {
return null;
}
}

function distanceToRect(point, rect) {
Expand Down Expand Up @@ -1738,4 +1746,180 @@
}
return samples;
};

// connector_motion_detached sampling. Per seeked frame, report every diagram
// connector's two screen-space endpoints AND every plausible node/box bbox.
// Node accumulates these across the grid and flags an endpoint that stays
// anchored to a node while the other endpoint sits in empty space on the held
// frames — a connector whose coordinates were frozen (wrong rotation pivot, or
// measured once at build) while its target kept moving. Icon-sized SVGs and
// short strokes are filtered out so only real diagram connectors count.
const CONNECTOR_MIN_SVG_PX = 100;
const CONNECTOR_MIN_LEN_PX = 60;
// Gauge needles/pointers/ticks are one-end-anchored indicators, not node-to-node
// connectors — a separate (gauge) check owns them. Skip by id/class of the line
// or any group ancestor up to the SVG.
const CONNECTOR_INDICATOR_NAME = /needle|pointer|gauge|tick|indicator/i;

@james-russo-rames-d-jusso james-russo-rames-d-jusso Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 isIndicatorConnector (the name-based prong of gauge exclusion added in the tip commit) has no unit test.

const CONNECTOR_INDICATOR_NAME = /needle|pointer|gauge|tick|indicator/i;

The filter walks the connector element's ancestry (id + className) up to the SVG parent and skips the connector before endpoint extraction. It's ONE of the two prongs the PR body advertises as by name AND geometry, and — per the sibling comment on checkPipeline.ts:1035 — it's the ONLY prong that catches arc-path gauges.

Shipped without unit coverage. The pipeline test file operates on ConnectorLineSample after the browser strip, so the regex ancestry walk is exercisable only in an integration/e2e run.

Regression pathway: someone refactors connectorNameFor (which handles className.baseVal for SVG elements) or edits the ancestor-loop termination (node !== svg.parentElement) — the filter silently regresses and CI stays green because the fuzz corpus doesn't run in CI.

Also: the regex is a small taxonomy. A connector inside <g id="chart-ticker-line"> matches (tick substring) and gets silently skipped even though it's a real dangling connector defect. A legitimate gauge needle authored as class="reading-arrow" doesn't match and slips through to the geometry prong (which has its own gap per :1035 comment) → false-positive.

Fix: extract isIndicatorConnector into a helper that can be exercised without JSDOM — pass in a minimal { id, className } chain and assert the regex disposition. Or add a corpus-e2e test wired to fuzz080 so a regression on the name filter surfaces in the same PR CI that motivated the tip-commit fix.

Stronger fix (couples with the sibling comment on checkPipeline.ts:1035): add a positive data-hf-gauge / data-hf-indicator opt-in data attribute that both checks honor as an explicit author-intent signal — removes reliance on both the fuzzy regex and the arc-path geometry gap.

— Review by Rames D Jusso

const CONNECTOR_NODE_MIN_AREA = 400;
// SVG dots/markers are small; keep the floor low but above sub-pixel decoration.
const CONNECTOR_NODE_MIN_DOT_AREA = 16;
const CONNECTOR_NODE_CAP = 300;
// A stroke-drawn dial/hub ring may be an <path> arc (M...A...), not a <circle>.
// Recognize near-circular stroke paths as ring nodes so connectors attaching
// to them aren't false-flagged as detached, and so the gauge-indicator geometry
// exclusion (which keys on ring nodes) can see arc-drawn dials too.
const CONNECTOR_RING_MIN_LEN = 120;
const CONNECTOR_RING_MIN_RADIUS = 20;
const CONNECTOR_RING_MAX_RESIDUAL_FRAC = 0.15;

function isIndicatorConnector(line, svg) {
for (let node = line; node && node !== svg.parentElement; node = node.parentElement) {
if (CONNECTOR_INDICATOR_NAME.test(connectorNameFor(node))) return true;
}
return false;
}

function lineScreenEndpoints(svg, line) {
if (typeof line.getScreenCTM !== "function" || typeof svg.createSVGPoint !== "function") {
return null;
}
const matrix = line.getScreenCTM();
if (!matrix) return null;
const map = (x, y) => {
const point = svg.createSVGPoint();
point.x = x;
point.y = y;
const mapped = point.matrixTransform(matrix);
return { x: mapped.x, y: mapped.y };
};
return {
start: map(line.x1.baseVal.value, line.y1.baseVal.value),
end: map(line.x2.baseVal.value, line.y2.baseVal.value),
};
}

// Node/box candidates a connector could anchor to: sized, opaque or text-
// bearing HTML elements plus SVG hub/ring/dot shapes. A shape drawn stroke-only
// (fill:none) is a ring — the connector attaches to its stroke, so it is marked
// `ring` and matched by perimeter, not hollow interior (see pointToNodeGap).
// A near-circular stroke <path> read as a ring/hub node. Returns a node box
// (ring flag from fill) or null when the path is not a resolvable circular hub.
// getPointAtLength is guarded — one malformed path must not abort the sampler.
function ringPathBox(path, rootRect) {
if (typeof path.getTotalLength !== "function" || typeof path.getPointAtLength !== "function") {
return null;
}
let total;
try {
total = path.getTotalLength();
} catch {
return null;
}
if (!Number.isFinite(total) || total < CONNECTOR_RING_MIN_LEN) return null;
const points = [];
try {
for (let i = 0; i < 16; i++) {
const local = path.getPointAtLength((total * i) / 16);
points.push({ x: local.x, y: local.y });
}
} catch {
return null;
}
const fit = fitCirclePoints(points);
if (!fit || fit.radius < CONNECTOR_RING_MIN_RADIUS) return null;
if (fit.residual > CONNECTOR_RING_MAX_RESIDUAL_FRAC * fit.radius) return null;

@james-russo-rames-d-jusso james-russo-rames-d-jusso Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 The ringPathBox residual gate is scale-relative and can be vacuous for shallow-curvature paths — a decorative flourish then registers as a ring node and silently suppresses real connector findings that end near it.

const fit = fitCirclePoints(points);
if (!fit || fit.radius < CONNECTOR_RING_MIN_RADIUS) return null;
if (fit.residual > CONNECTOR_RING_MAX_RESIDUAL_FRAC * fit.radius) return null;

Kåsa on a shallow-curvature path (say a 100px × 5px slightly-curved decorative stroke) produces a huge phantom-radius fit: as the determinant shrinks toward the degenerate-line case, uc/vc grow — the returned radius can be enormous even though the true curvature is small. Meanwhile the absolute residual stays ~1px because the points do lie on SOMETHING circular. Ratio residual / radius ≈ 0.001 easily passes the 0.15 gate. CONNECTOR_RING_MIN_RADIUS doesn't help because the phantom radius is LARGE.

Consequence: ringPathBox returns a node box with ring: true for that decorative flourish. Downstream pointToNodeGap treats it as a ring perimeter with the flourish's bbox as the outline. Any dangling connector endpoint near the flourish's bbox reads as anchored → the true connector_motion_detached finding is silently suppressed.

Fix: add a bbox-vs-radius sanity gate. A genuine ring's bounding box spans its diameter within a small factor; a shallow-curvature path's bbox is much smaller than the phantom-radius circle it fits to. Something like:

const rect = path.getBoundingClientRect();
const bboxDiag = Math.hypot(rect.width, rect.height);
if (fit.radius > bboxDiag) return null;  // phantom-radius rejector

Or require the fit centre (fit.cx, fit.cy) to sit within the bounding box (a real closed ring's centre is inside its own bbox; a shallow-arc phantom-fit's centre is far away).

Background: this is the same lens Magi caught on #2744's Kåsa fit — different failure mode (n=3 exact-fit there, degenerate-scale phantom-fit here), same underlying issue: residual-normalized-by-radius doesn't degrade gracefully when the fit is bad. Fitting >= CONNECTOR_RING_MIN_LEN-point curves (n = 16 vs 3 params here) makes it overdetermined, but doesn't rule out the shallow-curvature phantom.

— Review by Rames D Jusso

const rect = toRect(path.getBoundingClientRect());
const area = rectArea(rect);
if (area < CONNECTOR_NODE_MIN_DOT_AREA || area >= rectArea(rootRect) * 0.5) return null;
const fill = getComputedStyle(path).fill;
return {
selector: selectorFor(path),
left: rect.left,

@james-russo-rames-d-jusso james-russo-rames-d-jusso Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 ringPathBox returns path.getBoundingClientRect() as the node box but discards the actual fitted (fit.cx, fit.cy, fit.radius) — for a partial-arc dial (e.g. a 60°-of-a-circle gauge arc), the arc's bbox is much smaller than the true ring geometry.

const rect = toRect(path.getBoundingClientRect());
// ...
return {
  selector: selectorFor(path),
  left: rect.left,
  top: rect.top,
  right: rect.right,
  bottom: rect.bottom,
  ring: ...,
};

Downstream pointToNodeGap treats ring: true nodes as "distance to bbox perimeter". So for a 60°-arc gauge whose actual ring would span 200×200px, the arc's bbox might be only 100×60px. An endpoint that sits AT (bbox.right + 10) reads as 10px from the perimeter → anchored, even though it's nowhere near the drawn arc.

Consequence: over-anchoring on partial-arc gauges. The real fix is to keep the fit result and use distance_to_circle_perimeter(x, y, fit.cx, fit.cy, fit.radius) for ring: true path nodes. That would require plumbing the fit result through the NodeBox shape — bigger change — but the current bbox approximation is silently wrong for anything less than a full circle.

Cheap intermediate: reject ringPathBox when the arc covers less than ~270° of the circle (total < 0.75 * 2π * fit.radius). Full-ring dials still detect; partial-arc gauges bail to the name-based isIndicatorConnector path.

— Review by Rames D Jusso

top: rect.top,
right: rect.right,
bottom: rect.bottom,
ring: fill === "none" || fill === "transparent" || path.getAttribute("fill") === "none",
};
}

function connectorNodeBoxes(root, rootRect) {
const boxes = [];
const rootArea = rectArea(rootRect);
for (const element of Array.from(root.querySelectorAll("*"))) {
if (boxes.length >= CONNECTOR_NODE_CAP) break;
if (element.closest("svg") || !isVisibleElement(element, 0.05)) continue;
const opaque =
RASTER_TAGS.has(element.tagName) || hasOpaqueBackground(getComputedStyle(element));
if (!opaque && !textContentFor(element)) continue;
const rect = toRect(element.getBoundingClientRect());
const area = rectArea(rect);
if (area < CONNECTOR_NODE_MIN_AREA || area >= rootArea * 0.5) continue;
boxes.push({
selector: selectorFor(element),
left: rect.left,
top: rect.top,
right: rect.right,
bottom: rect.bottom,
ring: false,
});
}
for (const shape of Array.from(root.querySelectorAll("circle, ellipse, rect"))) {
if (boxes.length >= CONNECTOR_NODE_CAP) break;
if (!isVisibleElement(shape, 0.05)) continue;
const rect = toRect(shape.getBoundingClientRect());
const area = rectArea(rect);
if (area < CONNECTOR_NODE_MIN_DOT_AREA || area >= rootArea * 0.5) continue;
const fill = getComputedStyle(shape).fill;
boxes.push({
selector: selectorFor(shape),
left: rect.left,
top: rect.top,
right: rect.right,
bottom: rect.bottom,
ring: fill === "none" || fill === "transparent" || shape.getAttribute("fill") === "none",
});
}
for (const path of Array.from(root.querySelectorAll("path"))) {
if (boxes.length >= CONNECTOR_NODE_CAP) break;
if (path.closest(CONNECTOR_SKIP_CONTAINERS)) continue;
if (!isVisibleElement(path, 0.05)) continue;
const box = ringPathBox(path, rootRect);

@james-russo-rames-d-jusso james-russo-rames-d-jusso Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 A near-circular <path> can be registered as BOTH a ring node AND sampled as a connector — its endpoints then self-anchor and no finding fires.

const box = ringPathBox(path, rootRect);
if (box) boxes.push(box);

connectorNodeBoxes iterates <path> at :1877 to build node boxes. connectorSample (:1930+) iterates the same <path> set with the same visibility filters to build connectors. There's no cross-check that a path used as a ring node isn't also being sampled as a connector.

Failure scenario: a near-circular open-arc <path> (say a 240° arc) whose endpoint separation ≥ CONNECTOR_MIN_LEN_PX becomes:

  • A ring node via ringPathBox → registered as NodeBox with the arc's bbox
  • A connector via the connector loop → its two endpoints lie on its own bbox perimeter, so pointToNodeGap returns 0 for both (both endpoints are within the arc's own bbox) → both endpoints self-anchor → check never fires

Rare shape (most connectors are <line> or short polyline; most rings are closed shapes), but hides real bugs when it occurs.

Fix: track ring-registered paths in a Set in connectorNodeBoxes and skip them in the connector loop, or vice versa. One-line change.

— Review by Rames D Jusso

if (box) boxes.push(box);
}
return boxes;
}

window.__hyperframesConnectorSample = function collectConnectorSample() {
const root =
document.querySelector("[data-composition-id][data-width][data-height]") ||
document.querySelector("[data-composition-id]") ||
document.body;
const rootRect = rootRectFor(root);
const connectors = [];
for (const svg of Array.from(root.querySelectorAll("svg"))) {
if (!isVisibleElement(svg, 0.05) || hasAllowOverflowFlag(svg)) continue;
const svgRect = svg.getBoundingClientRect();
if (svgRect.width < CONNECTOR_MIN_SVG_PX || svgRect.height < CONNECTOR_MIN_SVG_PX) continue;
for (const line of Array.from(svg.querySelectorAll("line, path"))) {
if (line.closest(CONNECTOR_SKIP_CONTAINERS)) continue;
if (!isVisibleElement(line, 0.05)) continue;
if (isIndicatorConnector(line, svg)) continue;
const ends =
line.tagName.toLowerCase() === "line"
? lineScreenEndpoints(svg, line)
: pathScreenEndpoints(svg, line);

@james-russo-rames-d-jusso james-russo-rames-d-jusso Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 This new call site amplifies a defensive gap in the pre-existing pathScreenEndpoints helper — one bad diagram path can now abort the whole check.

const ends =
  line.tagName.toLowerCase() === "line"
    ? lineScreenEndpoints(svg, line)
    : pathScreenEndpoints(svg, line);

Look at pathScreenEndpoints at :1211-1239: getTotalLength() is defensively try/catch-guarded at :1221-1225 but the two getPointAtLength(0) / getPointAtLength(total) calls at :1237-1238 are NOT:

return {
  start: toScreen(path.getPointAtLength(0)),
  end: toScreen(path.getPointAtLength(total)),
};

In Chrome the pair is usually safe once total > 0, but the API is documented to throw for malformed paths (arcs with degenerate radii, invalid path data that survives getTotalLength numerically).

Failure mode: if either getPointAtLength throws, the exception propagates out of __hyperframesConnectorSample → page.evaluate rejects → driver.collectConnectorSample(time) rejects → collectGridSamples/runAuditGrid have no per-sample guard → the whole runBrowserCheck rejects. runCheckPipeline catches and replaces the entire browser result with emptyBrowserResult() + a single runtime finding — every layout/contrast/motion finding for the run is dropped over one malformed path.

The pre-existing per-sample connector_detached code path has the same shape (called once via connectorDetachmentIssues at :1287), so this isn't a net-new hazard. But this PR extends the surface — the sampler now runs on EVERY layout sample instead of once — multiplying the throw budget by the sample count.

Fix: wrap the two getPointAtLength calls at :1237-1238 in the same try block that already guards getTotalLength, returning null on throw. Two-line defensive change; matches the surrounding style. Or wrap the pathScreenEndpoints(svg, line) call HERE in a try/catch returning null, which is more localized to your PR.

— Review by Rames D Jusso

if (!ends) continue;
const len = Math.hypot(ends.end.x - ends.start.x, ends.end.y - ends.start.y);
if (len < CONNECTOR_MIN_LEN_PX) continue;
connectors.push({
selector: selectorFor(line),
ax: round(ends.start.x),
ay: round(ends.start.y),
bx: round(ends.end.x),
by: round(ends.end.y),
});
}
}
return { connectors, nodes: connectorNodeBoxes(root, rootRect) };
};
})();
43 changes: 43 additions & 0 deletions packages/cli/src/utils/checkBrowser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ import type {
CheckGeometryCandidate,
CheckOptions,
CheckSeverity,
ConnectorFrame,
ConnectorLineSample,
ConnectorNodeBox,
ContrastAuditEntry,
ContrastCapture,
GeometryCandidateRequest,
Expand Down Expand Up @@ -353,6 +356,7 @@ function createPageDriver(page: Page, setTime: (time: number) => void): CheckAud
collectLayoutGeometry: () => collectLayoutGeometry(page),
collectRotationSample: (time) => collectRotationSample(page, time),
collectOffPivotRotationSample: (time) => collectOffPivotRotationSample(page, time),
collectConnectorSample: (time) => collectConnectorSample(page, time),
collectGeometryCandidates: (time, request) => collectGeometryCandidates(page, time, request),
collectMotionFrame: (time, selectors, scopes) =>
collectMotionFrame(page, time, selectors, scopes),
Expand Down Expand Up @@ -563,6 +567,44 @@ function parseOffPivotRotationSample(value: unknown): OffPivotRotationSample[] {
];
}

async function collectConnectorSample(page: Page, time: number): Promise<ConnectorFrame> {
const raw = await page.evaluate(() => {
const sample = Reflect.get(window, "__hyperframesConnectorSample");
if (typeof sample !== "function") return null;
return Reflect.apply(sample, window, []);
});
if (!isRecord(raw)) return { time, connectors: [], nodes: [] };
const connectors = Array.isArray(raw.connectors) ? raw.connectors : [];
const nodes = Array.isArray(raw.nodes) ? raw.nodes : [];
return {
time,
connectors: connectors.flatMap(parseConnectorLine),
nodes: nodes.flatMap(parseConnectorNode),
};
}

function parseConnectorLine(value: unknown): ConnectorLineSample[] {
if (!isRecord(value)) return [];
const selector = stringValue(value, "selector");
const ax = numberValue(value, "ax");
const ay = numberValue(value, "ay");
const bx = numberValue(value, "bx");
const by = numberValue(value, "by");
if (!selector || ax === null || ay === null || bx === null || by === null) return [];
return [{ selector, ax, ay, bx, by }];
}

function parseConnectorNode(value: unknown): ConnectorNodeBox[] {
if (!isRecord(value)) return [];
const selector = stringValue(value, "selector");
const left = numberValue(value, "left");
const top = numberValue(value, "top");
const right = numberValue(value, "right");
const bottom = numberValue(value, "bottom");
if (!selector || left === null || top === null || right === null || bottom === null) return [];
return [{ selector, left, top, right, bottom, ring: value.ring === true }];
}

async function collectGeometryCandidates(
page: Page,
time: number,
Expand Down Expand Up @@ -1146,6 +1188,7 @@ const LAYOUT_ISSUE_CODES: readonly LayoutIssueCode[] = [
"escaped_container",
"panel_out_of_canvas",
"connector_detached",
"connector_motion_detached",
"rotation_pivot_drift",
"off_pivot_rotation",
"motion_appears_late",
Expand Down
Loading
Loading