Skip to content

feat(lint): add connector_motion_detached layout check#2745

Open
xuanruli wants to merge 6 commits into
xuanru/content-overlap-dense-resamplingfrom
xuanru/connector-motion-detached
Open

feat(lint): add connector_motion_detached layout check#2745
xuanruli wants to merge 6 commits into
xuanru/content-overlap-dense-resamplingfrom
xuanru/connector-motion-detached

Conversation

@xuanruli

@xuanruli xuanruli commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

What it catches

A half-attached connector during motion: one endpoint stays anchored to a node (<= 24px) while the other dangles > 80px from every node on >= 80% of held frames (t >= 0.45 * dur). The line/spoke visually detaches from what it should connect while the diagram animates.

The existing connector check only scans named <path> elements, requires both ends far, and judges a single frame — so it misses (a) <line> spokes, (b) one-end detachment, and (c) transient motion detachment. This check samples both <line> and <path> and evaluates persistence across the held-frame window.

How it works

  • Per-frame sampler records both endpoints of each <line>/<path> connector and the node-candidate set.
  • Half-attached signature: one end near a node, other end far from all nodes, sustained across the held window.
  • Node-candidate set includes HTML boxes and sizable SVG <circle>/<rect>/<ellipse>/ring-<path> shapes (so a connector anchored to an SVG-drawn hub/dot reads as attached, not dangling).
  • Excludes gauge/needle/pointer/tick/indicator elements (name + geometry: anchored end near arc-center & dangling end radially outward) — those are the off_pivot_rotation check's domain, keeping the architectural boundary clean.

Corpus evidence (autonomous geometry-fuzz run, 81 fuzzed diagrams)

  • 1 true positive (fuzz011), 0 false positives across all 81 samples after tuning (100% precision).
  • fuzz011 = 4 ring spokes half-attached (~185–190px dangle) — invisible to the existing connector check.
  • Tuning path that reached 0 FP:
    • Initial sweep: 4/81 fires, 3 FP (fuzz033/038/074) — all the same cause: connector anchored to an SVG-drawn hub while the node set was HTML-only.
    • Added sizable SVG shapes to node candidates → 3 FP → 1 FP; new FP fuzz080 = a correct gauge needle mistaken for a half-attached connector.
    • Added the gauge-indicator exclusion → 0 FP / 1 TP.
  • The VLM judge itself produced false positives on this cluster (fuzz051, fuzz014 render fully attached) — deterministic code inspection was the arbiter.

Validation

  • Autonomous Gemini-3.6 video-judge fuzz run to surface candidates, then a deterministic FP sweep across all 81 rendered compositions.
  • 13 unit tests (checkPipeline.connectorMotionDetached.test.ts) + full check suite pass; bun run build green.

Note for reviewers

This branch is the 3 connector commits cleanly rebased onto main — the dense-motion re-sampling change they were originally developed alongside ships separately (its own PR), so this branch contains only the connector detector.

🤖 Generated with Claude Code

@xuanruli
xuanruli changed the base branch from main to graphite-base/2745 July 24, 2026 07:42
@xuanruli
xuanruli force-pushed the xuanru/connector-motion-detached branch from 4678ed1 to de5b697 Compare July 24, 2026 07:42
@xuanruli
xuanruli changed the base branch from graphite-base/2745 to xuanru/content-overlap-dense-resampling July 24, 2026 07:42

Copy link
Copy Markdown
Contributor Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@xuanruli
xuanruli marked this pull request as ready for review July 24, 2026 07:49
@xuanruli
xuanruli force-pushed the xuanru/content-overlap-dense-resampling branch from 1dc7b36 to 938c2cf Compare July 24, 2026 08:04
@xuanruli
xuanruli force-pushed the xuanru/connector-motion-detached branch from de5b697 to f171743 Compare July 24, 2026 08:04
@xuanruli
xuanruli marked this pull request as draft July 24, 2026 08:05
@xuanruli
xuanruli marked this pull request as ready for review July 24, 2026 08:09

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The rule is well-scoped relative to connector_detached: sampling both endpoints across the held window and dropping aliased selectors is a sensible way to target the half-attached motion failure without widening the existing per-frame detector.

I found two blocking gaps in the rule’s stated contract:

  1. The “anchored” endpoint does not have to stay anchored. endpointHeldGaps records only minGap, and isAnchored returns true if that minimum is within 24px (packages/cli/src/utils/checkPipeline.ts:990-1025). One momentary contact on any held frame therefore marks the endpoint anchored for the entire window. I reproduced a finding where endpoint A is detached on every held frame except one, while B remains dangling. Please track an attached-frame fraction and require sustained attachment (consistent with the existing ~80% held criterion), with a regression for one-frame transient contact.

  2. The PR promises SVG circle/rect/ellipse/ring-path node candidates, but connectorNodeBoxes excludes every element inside SVG from the HTML pass and the SVG pass queries only circle, ellipse, rect (packages/cli/src/commands/layout-audit.browser.js:1781-1817). A hub/ring drawn as <path> is invisible to nearest-node matching, creating false detach findings for a common shape this PR explicitly claims to support. Please include validated ring paths, ideally by reusing the existing arc/hub path geometry rather than creating another path classifier, and add a path-ring fixture.

Requesting changes for these false-positive paths.

— Magi

@xuanruli

Copy link
Copy Markdown
Contributor Author

Thanks Magi — both are real. Fix in progress:

  1. Sustained attachment: replacing the minGap/isAnchored single-frame contact with an attached-frame fraction requiring sustained attachment (consistent with the existing ~80% held criterion). One momentary contact no longer marks an endpoint anchored for the whole window; your repro (A detached on every held frame but one, B dangling) will fire. Adding the one-frame-transient-contact regression.
  2. Ring <path> nodes: extending the SVG node pass (currently circle, ellipse, rect only) to include validated ring/hub paths, reusing the existing arc/hub path geometry rather than a new classifier, so path-drawn hubs are visible to nearest-node matching and don't produce false detach findings. Adding a path-ring fixture.

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed at f17174307080ef7f57eae3b7b15f4d52eba40320.

Well-executed extension of the connector-detection surface. The additive shape reads coherently: <line> and <path> both sampled per frame (previously only static <path>); half-attached signature computed against a broader node set (HTML boxes + sizable SVG shapes vs HTML-only before); persistence across the held-frame window (previously single-frame); gauge/needle exclusion drawing an architectural boundary with #2744's off_pivot_rotation. SVG API usage checks out (SVGLineElement.x1.baseVal.value, SVGPathElement.getPointAtLength(0), getScreenCTM, createSVGPoint/matrixTransform, getTotalLength all real and matching W3C SVG DOM), root discovery matches the pre-existing sibling __hyperframesRotationSample (3-tier [data-composition-id][data-width][data-height] chain — good), pointToNodeGap bbox-perimeter approximation errs toward reporting anchor (safer FP direction), and this new cross-sample check doesn't double-report against the pre-existing connector_detached per-sample check (the pre-existing one requires BOTH endpoints detached; this one requires exactly one dangling — non-overlapping predicates).

Blockers: none.

The two 🟠 concerns are about the gauge-exclusion boundary being narrower than the PR body advertises and a defensive gap in pathScreenEndpoints that this PR amplifies by sample count. Neither prevents merge but both are worth folding in.

Cross-cutting themes

1. "By name AND geometry" gauge exclusion is technically two independent OR filters, and both prongs have holes. The geometry branch (isGaugeIndicator at checkPipeline.ts:1035) requires node.ring === true on a ConnectorNodeBox, but connectorNodeBoxes (browser.js:1802) only registers circle, ellipse, rect — the very common arc-path dial pattern (<path d="M... A..." fill="none"/>) never becomes a ConnectorNodeBox at all, so the geometry prong cannot fire for it. That leaves only the name-based isIndicatorConnector regex added in the tip commit — a /needle|pointer|gauge|tick|indicator/i walk up the ancestor chain, with no unit-test coverage. Both prongs individually have gaps; the safety net has holes on both sides. Two levers to close the gap: (a) extend connectorNodeBoxes to register <path fill="none"> when the path is closed / near-circular (mirror the arc-hub fit that #2744 already implements), and (b) add a positive data-hf-gauge / data-hf-indicator opt-in data attribute that both checks honor as an explicit-author-intent signal. Inline anchors below.

2. Defensive gaps in pathScreenEndpoints (browser.js:1237-1238) amplify by sample count. getTotalLength() is try/catch-guarded at :1221-1225 but the two getPointAtLength() calls that use its return value are not. In Chrome the pair is usually safe once total > 0, but the API is documented to throw for malformed paths. If it does throw, the exception propagates out of __hyperframesConnectorSamplepage.evaluate rejects → driver.collectConnectorSample(time) rejects → the whole runBrowserCheck rejects → every layout/contrast/motion finding for the run is dropped over one malformed path. Pre-existing shape, but this PR moves the sampler from being called once (connector_detached) to being called per-sample — the throw budget multiplies by sample count.

3. Framework consistency with #2744 (which this stacks over).

  • Wire shape: collectConnectorSample(time) returns ConnectorFrame = { time, connectors, nodes } — the frame-envelope shape with time hoisted. #2744's collectOffPivotRotationSample(time) returns OffPivotRotationSample[] with time on each sample. Same "one seek's data" concept, two different shapes. The frame envelope here is objectively better (per-frame nodes shared across all connectors, no duplication) — this PR is on the right side of the seam. Worth mentioning here so the eventual reconciliation lands as "align #2744 to #2745's shape", not the other way.

4. Doc drift: PR body cites a sibling check that doesn't exist under that name.

Your PR body says: "those are the needle_pivot_offset check's domain, keeping the architectural boundary clean". Grep of packages/cli/: needle_pivot_offset → 0 hits; off_pivot_rotation → 9 hits (canonical name from #2744). The stack branch is also named xuanru/needle-pivot-offset-check but the actual check is off_pivot_rotation. Doc-only; please update the PR body when you touch it next. A future reader searching for needle_pivot_offset (e.g. to --exclude-check needle_pivot_offset) finds nothing — the CLI would silently ignore the unknown code and the check keeps firing.

🟢 Verified-safe

  • All SVG DOM APIs are real and match W3C signatures — no hallucinated APIs.
  • Root discovery matches the pre-existing sibling __hyperframesRotationSample (3-tier chain). #2744's __hyperframesOffPivotRotationSample skips the primary selector — flagged separately on that PR.
  • groupConnectorsBySelector aliasing behavior — a selector observed twice in one frame is dropped globally across all frames — matches the test at test:166. Correct defensive posture (false-association risk outweighs recall loss).
  • pointToNodeGap ring perimeter math is correct — bbox-perimeter approximation for <circle>/<ellipse> matches the outer stroke edge; for corner-of-bbox points the approximation errs toward reporting anchor (safer FP direction).
  • No duplication of the earlier connectorDetachmentIssues — the pre-existing per-sample check requires both endpoints detached; this new cross-sample check requires exactly one dangling. Non-overlapping predicates; cannot double-report.
  • connector_motion_detached correctly excluded from PERSISTENCE_TIERED_CODES (layoutAudit.ts:212). Held-frame semantics already baked in via CONNECTOR_HELD_DETACH_FRAC = 0.8; adding to the tier set would double-apply the persistence gate.
  • Runtime path layoutSet.has(time) gate at :386-394 ensures collectConnectorSample runs only on layout samples, not motion/transition times.
  • fakeDriver in check.test.ts stubs the new collectConnectorSample returning { time, connectors: [], nodes: [] } — existing integration tests won't crash.
  • CheckAuditDriver.collectConnectorSample typing round-trips cleanly through parseConnectorLine / parseConnectorNode in checkBrowser.ts:586-606.
  • isVisibleElement(svg, 0.05) opacity gate at :1829 correctly excludes hidden / entrance-pre-reveal SVGs.
  • Tip commit's isIndicatorConnector ancestor walk terminates at svg.parentElement (:1752) — bounded, can't escape composition root.
  • ConnectorFrame.nodes populated once per frame and shared across every connector for that frame — no O(N_conn × N_node × frames) recomputation.
  • 11 unit tests present as claimed; line-endpoint detection, ring anchoring, gauge exclusion (geometry branch), inward-vs-outward radial distinction, held-frame gating, min-frame threshold, and selector aliasing all covered.
  • Cross-PR boundary with #2744 verifiable via fuzz080 cross-reference: PR body says "TP there, FP-cleared here via gauge exclusion" — matches #2744's PR body saying the same about fuzz080 from the other side. Coherent architectural partition.

Solid extension — the mid-development gauge-exclusion pivot (visible in the tip commit fix(lint): exclude gauge needles/pointers) reads as tightening rather than papering over, which is good. Please close the two prongs of the exclusion gap or acknowledge them explicitly as scoped-known blind spots before merge.

Review by Rames D Jusso

// drifting toward the centre — so this only excludes true centre-pivot pointers.
const GAUGE_HUB_CENTRE_FRACTION = 0.25;

function isGaugeIndicator(

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 geometry prong of isGaugeIndicator silently skips arc-path dials — a very common gauge pattern.

function isGaugeIndicator(
  anchored: { x: number; y: number },
  dangling: { x: number; y: number },
  nodes: ConnectorNodeBox[],
): boolean {
  for (const node of nodes) {
    if (!node.ring) continue;
    // ...
    if (dAnchored <= GAUGE_HUB_CENTRE_FRACTION * size && dDangling > dAnchored) return true;
  }
  return false;
}

The PR body promises the gauge exclusion works by name AND geometry. This function is the geometry prong. It iterates nodes and skips anything with !node.ring, so the only exclusion carriers are <circle> / <ellipse> / <rect> shapes registered by connectorNodeBoxes (layout-audit.browser.js:1802: for (const shape of Array.from(root.querySelectorAll("circle, ellipse, rect")))).

A very common gauge/dial pattern draws the arc as <path d="M... A..." fill="none"/> — matches the fallback arcHubForSvg at browser.js:1627 that #2744 already implements for its own hub-recovery. Those <path> arcs are never registered as ConnectorNodeBox at all, so isGaugeIndicator cannot fire for them.

The remaining safety net is the name-based isIndicatorConnector regex added in the tip commit. But an anonymous unlabeled arc-path gauge → no name hit + no ring hit → false positive connector_motion_detached on the needle.

Not exercised by any test — the gauge test at test:108 hardcodes an <rect>-shaped arc with ring: true; no test constructs a needle over an arc-path gauge.

Fix options:

  • (a) Extend connectorNodeBoxes to also register path[fill="none"] as ring: true when the path is closed / near-circular. Mirror the arcHubForSvg fit that already lives in the same file — you get parity with feat(lint): add off_pivot_rotation hub-referenced layout check #2744's hub-recovery for free.
  • (b) Piggy-back on any OffPivotRotationSample.hx/hy the sampler already recovers for the same frame — if feat(lint): add off_pivot_rotation hub-referenced layout check #2744's off-pivot check found a hub for this SVG at this time, treat it as a ring-equivalent for this frame.
  • (c) Softer: acknowledge the arc-path miss as a known blind spot in the pipeline comment at :1028-1032 so future PRs don't inherit the wrong invariant.

Review by Rames D Jusso

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

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 __hyperframesConnectorSamplepage.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


// connector_motion_detached thresholds. Corpus timings: entrances settle by
// ~2s of a 7-8s composition, so held/steady state starts well before half.
const CONNECTOR_MIN_FRAMES = 4;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 CONNECTOR_ATTACH_PX = 24 is a flat pixel constant but its detach counterpart scales to viewport.

const CONNECTOR_ATTACH_PX = 24;
// ...
const CONNECTOR_DETACH_FLOOR_PX = 80;
const CONNECTOR_DETACH_VIEWPORT_FRACTION = 0.06;

The detach side scales up on 4K (Math.max(80, 0.06 * min(w, h)) = 129px on 3840×2160) but the attach side stays at a flat 24px on every canvas.

Failure mode (safer direction): on very-high-DPR compositions a genuinely-anchored endpoint (say 32px away from a node — visually adjacent, well within the node's own painted size) reads as !isAnchored, danglingEndpoint returns null (neither endpoint anchored), and the finding never fires. This is a false-NEGATIVE (safer than the FP direction) but breaks the symmetry the comment at :927-929 asserts ("measured against a dense node-candidate set").

Fix: either mirror the detach ladder (Math.max(CONNECTOR_ATTACH_PX, CONNECTOR_ATTACH_VIEWPORT_FRACTION * min(w, h)) with fraction ~0.018 to preserve the 24/1000×1000 anchor point), or leave the constant flat and document at :922 that anchor tolerance intentionally does NOT scale — trading recall on 4K for FP suppression on 1080p.

Review by Rames D Jusso

canvas: Canvas,
): AnchoredLayoutIssue[] {
const findings: AnchoredLayoutIssue[] = [];
const duration = frames.reduce((max, frame) => Math.max(max, frame.time), 0);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Effective holdStart is ~0.425·D under default sampling, not the 0.45·D the comment / PR body promise.

const duration = frames.reduce((max, frame) => Math.max(max, frame.time), 0);
if (duration <= 0) return findings;
const holdStart = CONNECTOR_HELD_START_FRAC * duration;

duration is derived from max(frame.time), but buildLayoutSampleTimes (layoutAudit.ts:82-95) places samples at ((index + 0.5) / count) * D — so with the default 9 samples the max sampled time is 8.5/9 · D ≈ 0.944 · D, and holdStart = 0.45 · 0.944 · D ≈ 0.425 · D.

So the effective held-window starts ~5% of duration earlier than the design comment at :919-920 states. Purely cosmetic on the current corpus (test at test:74 doesn't exhibit the drift because its frames array happens to include time=0 and time=duration exactly), but a future author reading CONNECTOR_HELD_START_FRAC = 0.45 and inferring contract intent gets the wrong number.

Fix: pass the true composition duration in from the caller (grid.duration is available at the call site in runAuditGrid) rather than reducing over sample times. Or update the comment at :919-920 to note that the effective threshold is 0.45 · max_sample_time, not 0.45 · duration.

Review by Rames D Jusso

// 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;

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

@xuanruli
xuanruli force-pushed the xuanru/connector-motion-detached branch from f171743 to 59ae513 Compare July 24, 2026 10:23
@xuanruli
xuanruli force-pushed the xuanru/content-overlap-dense-resampling branch from 938c2cf to 3e0c480 Compare July 24, 2026 10:23
@xuanruli
xuanruli marked this pull request as draft July 24, 2026 10:24
@xuanruli

Copy link
Copy Markdown
Contributor Author

@miguel-heygen pushed the fix in 59ae513ec — ready for another look.

FP path 1 — sustained attachment:

  • Replaced the single-frame minGap/isAnchored contact with an attached-frame fraction requiring sustained attachment (consistent with the existing ~80% held criterion). One momentary contact no longer anchors an endpoint for the whole window; the repro (A detached every held frame but one, B dangling) now fires. Added the one-frame-transient-contact regression.

FP path 2 — ring <path> nodes:

  • The SVG node pass now registers validated ring/arc <path> hubs (reusing the existing arc/hub path geometry), so path-drawn hubs are visible to nearest-node matching. This also flows into isGaugeIndicator (node.ring), closing the geometry prong of the gauge exclusion for arc-path dials. Added a path-ring fixture, and a unit test for the name-based isIndicatorConnector regex.
  • Guarded the getPointAtLength pair in pathScreenEndpoints so a single malformed path degrades gracefully instead of aborting the whole check (matters now that the sampler runs once-per-sample).

PR body updated: off_pivot_rotation (was the stale needle_pivot_offset).

CI: green on the stack's #2744 SHA run; Windows re-run finishing.

@xuanruli
xuanruli requested a review from miguel-heygen July 24, 2026 10:54
@xuanruli
xuanruli marked this pull request as ready for review July 24, 2026 21:29

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed exact head 59ae513ec327e0c4e1e7a0d20aea3567c53b3ab9 against my prior requested changes. Both blockers are resolved: packages/cli/src/utils/checkPipeline.ts:1014-1021 and :1082-1114 require attachment/detachment to persist across held frames, so a one-frame graze cannot anchor a connector; the browser sampler now includes ring-like <path> geometry and the regressions cover both the transient-touch case and gauge-ring suppression. The isolated #2745 delta is sound. This approval is for #2745 itself; the stack remains blocked by the separate #2746 finding below.

Verdict: APPROVE

Reasoning: Sustained attachment replaces the single-frame shortcut, path/ring hubs are represented, and the new tests exercise the exact prior failures.

— Magi

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-reviewed at 59ae513ec327e0c4e1e7a0d20aea3567c53b3ab9 — delta from R1's f17174307.

Substantial R2 pass. Both R1 blockers cleanly closed: sustained-anchoring at checkPipeline.ts:1099-1114 (CONNECTOR_HELD_ATTACH_FRAC = 0.8) replaces the single-frame graze; and the arc-path ring gap I flagged at R1 is closed by the new ringPathBox at layout-audit.browser.js:1808 — near-circular <path fill="none"> elements now register as ring nodes via a Kåsa fit + residual gate, flowing into both anchoring and isGaugeIndicator. pathScreenEndpoints's getPointAtLength pair is now try/catch-guarded (:1240-1247). Coverage adds touch-once-then-detached (fires) + single-graze-off-node (stays quiet). Magi APPROVED at this SHA and I concur on direction.

Not blocking. The ringPathBox fix does exactly what I asked for at R1 — but it inherits the same residual-normalized-by-radius shape Magi flagged as vacuous on #2744's Kåsa. Different failure mode (n=3 exact-fit there, scale-degenerate phantom-fit here — n=16 overdetermined so the residual isn't exactly zero, but the ratio can still be tiny for shallow-curvature paths). Worth folding in a bbox-vs-fit sanity gate before merge.

Cross-cutting theme: ringPathBox residual gate is vacuous for shallow-curvature paths

Kåsa on a slightly-curved decorative stroke produces a huge phantom radius as the determinant shrinks toward the degenerate-line case, while absolute residual stays small — so residual / radius can pass 0.15 for something that isn't a ring at all. Result: decorative flourish registers as a ring node, dangling connector endpoints near it read as anchored, real findings suppressed.

Inline anchor with fix at layout-audit.browser.js:1830 — a bbox-vs-radius sanity gate (or require fit.cx, fit.cy inside the bbox) rejects the phantom-radius case. Same underlying lens as Magi's #2744 catch, just a different degenerate.

Related: ringPathBox returns getBoundingClientRect() as the node box but discards the fit's (cx, cy, radius). For a partial-arc gauge (60°-of-a-circle dial), the arc's bbox is much smaller than the true ring — downstream anchor tests use bbox-perimeter distance instead of true-ring-perimeter distance → over-anchoring on partial-arc dials. Inline at :1837.

R1 items — carried disposition

  • isGaugeIndicator geometry prong for arc-path dials — closed via ringPathBox.
  • pathScreenEndpoints.getPointAtLength defensive gap — try/catch added at :1240-1247.
  • CONNECTOR_ATTACH_PX = 24 still flat pixel at checkPipeline.ts:1004; asymmetry vs viewport-scaled detach unchanged. Low-signal on 1080p, may bite on 4K.
  • holdStart = 0.45·D still uses max(frame.time) at :1237; effective threshold ~0.425·D. Cosmetic drift; the sampling-dependent semantics survive.
  • isIndicatorConnector name-regex still has no unit test — but the R2 landing of ringPathBox closes the primary gauge-exclusion gap, so the reliance on the name regex is lower now. Deferrable.
  • data-hf-gauge / data-hf-indicator opt-in — I suggested this at R1 as an escape hatch; silently deferred. That's fine; the geometry-based path now covers most real cases.

Small R2 seams (inline)

  1. Shallow-curvature phantom-radius passes gatelayout-audit.browser.js:1830. The one worth folding in.
  2. Partial-arc dial bbox mismatchlayout-audit.browser.js:1837. Bbox approximation is silently wrong for anything less than a closed ring.
  3. Self-reference risklayout-audit.browser.js:1886. A near-circular open-arc <path> can register as BOTH a ring node AND a connector; its own endpoints self-anchor → check never fires. Rare shape, one-line fix (skip ring-registered paths in the connector loop).

🟢 Verified

  • Sustained-anchoring math correct at boundaries: heldFrames=2 requires unanimous 2/2 for both anchored and dangling; the 1/1 mixed case yields neither state, silently no-oping — safe direction.
  • ringPathBox is overdetermined (n=16 vs 3 params) — Magi's n=3 vacuity does not apply directly. Only the shallow-curvature phantom-fit case bites.
  • getPointAtLength guard returns null from ringPathBox on throw — skip-this-path semantics, doesn't abort the sampler.
  • New touch-once-then-detached test at .connectorMotionDetached.test.ts:92-104 uses 7 held frames with 1/7 attached ≈ 0.143 (well below 0.8) → exercises the sustained-anchor branch correctly.
  • Mirror single-graze-off-node test at :107-114 uses 6/7 attached ≈ 0.857 (above 0.8) → correctly stays quiet.
  • CI green on required checks; canceled state on parallel-preflight is due to Graphite's stack-rebase pattern (in-progress reruns), not real failures.

Solid R2. The ringPathBox fix is exactly the right architectural move; folding in a bbox-vs-radius sanity gate closes the last-mile residual-vacuity hole and would leave the check meaningfully harder to fool than R1.

Review by Rames D Jusso

}
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;

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 fill = getComputedStyle(path).fill;
return {
selector: selectorFor(path),
left: rect.left,

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

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);

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

@xuanruli
xuanruli force-pushed the xuanru/content-overlap-dense-resampling branch from 3e0c480 to c4d94f2 Compare July 24, 2026 22:06
@xuanruli
xuanruli force-pushed the xuanru/connector-motion-detached branch from 59ae513 to 70e6026 Compare July 24, 2026 22:06
@xuanruli
xuanruli force-pushed the xuanru/content-overlap-dense-resampling branch from c4d94f2 to 111ef6e Compare July 24, 2026 22:28
@xuanruli
xuanruli force-pushed the xuanru/connector-motion-detached branch from 70e6026 to 14a1bcd Compare July 24, 2026 22:28

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Strengths: The new bbox-vs-radius sanity gate at packages/cli/src/commands/layout-audit.browser.js:1834-1845 closes the shallow-curvature phantom-circle failure without weakening the residual guard. The focused browser regression at packages/cli/src/commands/layout-audit.browser.test.ts:889-926 proves a genuine ring remains eligible while the huge-radius shallow arc is rejected.

I also ran the exact-head browser/layout suites locally: 103 tests passed, including the new ring and 499/500ms persistence cases. No new blocker in this delta.

Verdict: APPROVE
Reasoning: The last scale-degenerate Kåsa-fit false-positive is now rejected by an independent geometric invariant with direct coverage.

— Magi

@xuanruli
xuanruli force-pushed the xuanru/connector-motion-detached branch 2 times, most recently from 2a09c42 to 3569c76 Compare July 25, 2026 00:46
@xuanruli
xuanruli force-pushed the xuanru/content-overlap-dense-resampling branch from 4e8c3d3 to 5f11160 Compare July 25, 2026 00:46
xuanruli and others added 6 commits July 25, 2026 08:15
Detects a connector (SVG <line>/<path>) that stays anchored to a node at
one endpoint while its other endpoint sits in empty space across the held
frames — the half-attached dangle the per-frame connector_detached check
deliberately ignores. Catches spokes/edges frozen at build or rotated
about a wrong pivot while their target kept moving.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds SVG circle/ellipse/rect to the node-candidate set so a connector
terminating on an SVG-drawn hub, ring, or dot marker counts as attached.
Stroke-only shapes (fill:none rings) match by bbox perimeter, not hollow
interior. Clears the leader-line-to-ring false positives (fuzz033/038/074)
while the drifted-ring detachment (fuzz011) still fires.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A gauge needle/pointer/tick is a one-end-anchored indicator, not a
node-to-node connector; a separate gauge check owns it. Skip a connector
whose element or group ancestor id/class matches needle|pointer|gauge|tick|
indicator, and skip gauge-indicator geometry (endpoint pivots near an SVG
arc/hub centre with the loose end radially outward). A defective radial
connector points the other way so it still fires. Clears the fuzz080 gauge
false positive; fuzz011 detach still fires (0 FP across the 81-sample corpus).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…otion_detached

Round-1 blocker: "anchored" was defined as minGap (the single closest frame),
so an endpoint that grazed a node once and then detached for the rest of the
held window still read as anchored — letting a genuinely dangling partner
escape the finding. Anchoring is now SUSTAINED: an endpoint counts as anchored
only if it stays within attach tolerance on >= 80% of held frames
(CONNECTOR_HELD_ATTACH_FRAC). A connector that touches once then detaches is
flagged; a connector that stays attached but for a single graze off-node is not.

Blocker-3 part 2 + round-2 gauge-exclusion gap: the node sampler only registered
circle/ellipse/rect, so arc-drawn dials (<path d="M..A.." fill="none"/>) never
became ring nodes — a connector attached to a path ring false-fired, and the
gauge-indicator geometry exclusion (which keys on ring nodes) couldn't see them.
connectorNodeBoxes now recognizes near-circular stroke <path> elements as ring
nodes (Kåsa fit, residual-gated), flowing into both anchoring and isGaugeIndicator.

Round-2: pathScreenEndpoints' getPointAtLength pair was unguarded; since this
sampler now runs once per seeked frame, one malformed path could abort the whole
check. Wrapped in try/catch so a bad path degrades to "skip this path".

Tests: touch-once-then-detached now fires (verified red before the fix); the
mirror single-graze-off-node case stays quiet. Existing ring-node/gauge pipeline
coverage unchanged and still green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…udit

ringPathBox gated only on Kåsa residual normalized by radius, which a
shallow-curvature arc passes by fitting an enormous phantom circle. Add a
bbox-vs-radius sanity gate: the fitted diameter must stay within 3x the
path's drawn bounding box, so a nearly-straight arc no longer registers as
a false ring/hub node (and no longer creates phantom gauge-exclusions).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@xuanruli
xuanruli force-pushed the xuanru/connector-motion-detached branch from 3569c76 to d114f47 Compare July 25, 2026 08:26
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.

3 participants