feat(lint): dense motion re-sampling for content_overlap#2746
Conversation
e443861 to
1dc7b36
Compare
|
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.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
7bb8b4f to
399196e
Compare
1dc7b36 to
938c2cf
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
The narrow implementation is good: this reuses the existing content_overlap detector and its persistence/collapse semantics instead of creating a second overlap rule, and it avoids double-collecting base-grid timestamps.
The animation gate currently defeats the exact transient-motion class this PR is intended to catch:
collectMotionOverlapSamplesreturns unless the sparse base-grid geometry signatures differ (packages/cli/src/utils/checkPipeline.ts:457-468). If an animation starts and ends entirely between sparse samples, every sparse fingerprint can be identical even though the dense grid contains a real overlap. In that case the dense pass never runs, so the new detector misses the motivating defect by construction. The existing static negative test only proves that an actually static mocked fingerprint skips work; it does not cover identical sparse fingerprints with motion/collision between them.
Please gate on an authoritative animation/timeline signal, or run the cheap dense overlap sampling independently of the sparse fingerprints, and add a regression where all sparse signatures are equal but a mid-grid overlap is detected.
Requesting changes for this false-negative path.
— Magi
|
Thanks Magi — you're right, the sparse-fingerprint gate defeats exactly the transient-motion class this PR targets: if an animation starts and ends between two sparse samples, every sparse fingerprint is identical and the dense pass never runs. Fix in progress:
|
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 938c2cf37a7e69288403280c2820fb69043c00bb.
Genuinely tidy small-footprint PR — the "zero new detection logic" claim holds up (grep confirms __hyperframesOverlapAudit at layout-audit.browser.js:1433-1440 calls contentOverlapIssues(root, time) directly, same function invoked by the main layout audit path at :1420; no reimplementation of collectSolidTextBlocks, overlapIssue, or the 0.2 threshold). The static-composition gate is intact (the wiring test at check.test.ts:1364-1373 correctly asserts collectOverlap is never called for collectLayoutGeometry: () => 'static'), root-element discovery matches the main audit's 3-tier chain, baseTimes.has(time) de-dup works despite float risk (both grids round to 3 decimals via roundTime), the geometry fingerprint captures a broad set of animation modes (getBoundingClientRect() + opacity per element + canvas/video pixel hash), and there's no leaky dependency between this PR and #2744's rotation scaffolding (collectMotionOverlapSamples uses only driver.seek, driver.collectOverlap, and pre-existing collected.geometrySignatures).
Reuse claim: verified. Boundary with sparse pass: verified. Not-a-leaky-stack: verified.
Where the review lands hardest is on persistence-tier semantics under the new grid density — your PR-body reviewer note flags the sensitivity honestly, but the comment at layoutAudit.ts:186-198 documents an explicit design contract (≥500ms → error, ~250ms → ignore) that the code silently violates in dense-pass mode. Inline for the specific ask. Not a blocker — but the design choice is worth making explicit either in code or in that comment before merge.
Individual findings — one 🟠 with the design ask, five smaller 🟠 concerns on cliff-effects, one 🟡 doc-string nit — all inline.
Cross-cutting themes
1. Persistence-tier math wasn't updated for the new grid density. The occurrences >= 2 shortcut in isContentOverlapHeldLongEnough (layoutAudit.ts:324) was designed assuming a grid where 2 occurrences ≥ ~1s wall-clock. At 8fps dense sampling, 2 occurrences can span ~125ms — contradicting the design comment (~500ms) and the stated ignore floor (~250ms). Your reviewer note acknowledges this in prose; the code doesn't reflect it. Two lines of code make the choice explicit either way; that's the primary ask.
2. Silent cliff-effects at cap + duration boundaries. The 120-sample cap silently degrades effective fps for 15s+ compositions (~5.94fps at 20s, ~3.95fps at 30s — the "8fps" contract is only guaranteed under ~14.8s). The fingerprint gate has an aliasing failure mode on periodic motion whose period aligns with the base-grid step. The dense loop has no early-terminate — once occurrences clears the threshold there's no signal upside from more samples. All fail-silent — the design's stated invariants get erased by boundary conditions without the code announcing it.
3. Tests validate the mechanism, not the target defect. The new "surfaces a held content_overlap" test at check.test.ts:1345-1362 has collectOverlap return a warning at EVERY dense sample time (~72 samples for the 9s fake duration). That trivializes the persistence promotion — the test would pass equally well against a broken dense pass that promoted every finding. The scenario the PR was built for — a real 125ms crossing seen only by 2 adjacent dense samples — is not reproduced anywhere. Also, buildOverlapSampleTimes(duration) is a pure function (checkPipeline.ts:441) and would take five lines to unit-test for cap/bounds/quantization; that catches the density-math regressions the corpus can't.
🟢 Verified-safe
- "Zero new detection logic" reuse claim verified (
contentOverlapIssuesshared between both paths). - Root-element discovery matches the main layout audit (3-tier
[data-composition-id][data-width][data-height] → [data-composition-id] → document.body). baseTimes.has(time)de-dup works despite float risk — both grids round to 3 decimals viauniqueSortedTimes/roundTime.- Static-composition gate is intact — test 2 at check.test.ts:1364-1373 correctly drives
collectLayoutGeometry: () => 'static'and assertscollectOverlapis not called. - Fingerprint (
getBoundingClientRect() + opacityper element + canvas/video pixel hash) captures CSS keyframes, SVG child anims, GSAP tweens, canvas/video content changes. - No leaky dependency on #2744's rotation collectors —
collectMotionOverlapSamplesonly callsdriver.seek,driver.collectOverlap(new), and reads pre-existingcollected.geometrySignatures. collectSolidTextBlockshas no other callers in the codebase — no state that could break other consumers.- Perf claim "text-only so it's cheap" defensible per-sample (dense pass runs on text elements filtered pre-loop).
Small, focused PR; the ask is mainly to close the persistence-tier contract question before the dense pass ships as a default.
| ): Promise<void> { | ||
| if (new Set(collected.geometrySignatures).size <= 1) return; | ||
| const baseTimes = new Set(grid.layoutSamples); | ||
| for (const time of buildOverlapSampleTimes(grid.duration)) { |
There was a problem hiding this comment.
🟠 Design-contract question — these dense-pass findings enter the persistence tier via a shortcut that assumed sparse-grid timing.
for (const time of buildOverlapSampleTimes(grid.duration)) {
if (baseTimes.has(time)) continue;
await driver.seek(time);
collected.layoutIssues.push(...(await driver.collectOverlap(time)));
}The findings pushed here flow into collapseStaticLayoutIssues → isContentOverlapHeldLongEnough (layoutAudit.ts:323-330, unmodified by this PR):
function isContentOverlapHeldLongEnough(issue: LayoutIssue, occurrences: number): boolean {
if (occurrences >= HELD_ACROSS_SAMPLES_MIN_OCCURRENCES) return true; // ← short-circuits
const firstSeen = issue.firstSeen ?? issue.time;
const lastSeen = issue.lastSeen ?? issue.time;
const heldMs = (lastSeen - firstSeen) * 1000;
return heldMs >= CONTENT_OVERLAP_HELD_ERROR_MS;
}Design comment at layoutAudit.ts:186-198 frames the mapping: "At the default 9-sample grid over a multi-second composition ... two collapsed occurrences are already >= one sample-to-sample gap, which is well past 500ms — so 'held under 250ms' reduces to occurrences <= 1 and 'held >= 500ms' reduces to occurrences >= 2." That mapping holds for the sparse grid (~1s between adjacent samples).
At the new 8fps dense grid, two adjacent samples span ~125ms. So occurrences === 2 → promoted straight to error — well under the 500ms design floor and even under the 250ms ignore floor.
The design comment DOES anticipate this — it says "with the literal ms span (CONTENT_OVERLAP_HELD_ERROR_MS) kept as a fallback for callers whose samples really are spaced close enough together for the ms floor to matter on its own (dense --at/--at-transitions runs)". But the code short-circuits on occurrences FIRST — the ms-fallback branch only runs when occurrences < 2, so it never gates the dense-pass 2-occurrence case.
Your PR-body reviewer note acknowledges this: "At 8fps the occurrences >= 2 persistence shortcut spans ~125ms (vs ~500ms on the sparse grid), so brief-but-real collisions now gate at error. Every inspected instance was real, but if softer severity is preferred, the heldMs >= 500 path is the alternative knob."
The request is: make the choice explicit either in code or in the comment. Three options:
- (a) Tighten
isContentOverlapHeldLongEnoughto AND —return occurrences >= 2 && (lastSeen - firstSeen) * 1000 >= CONTENT_OVERLAP_HELD_ERROR_MS. Still passes the sparse-grid case (samples ≥1s apart → heldMs ≥1000 ≥ 500) and correctly gates the dense case. Cleanest — one source of truth. - (b) Tag dense-pass findings with a
source: 'dense'marker HERE and skip theoccurrencesshortcut only for that source in the persistence-tier code. - (c) Update the design comment at
layoutAudit.ts:186-198to say the ms floor is intentionally relaxed to ~125ms for dense-pass findings, with a rationale from the corpus ("81/81 samples of the corpus showed 125ms crossings were real defects").
Any of the three closes the contract question. Right now the comment and the code disagree; the disagreement widens by 375ms every time the dense pass fires.
— Review by Rames D Jusso
| const OVERLAP_SAMPLE_FPS = 8; | ||
| const OVERLAP_MAX_SAMPLES = 120; | ||
|
|
||
| function buildOverlapSampleTimes(duration: number): number[] { |
There was a problem hiding this comment.
🟠 20s+ compositions silently degrade to sub-8fps sampling under the 120-cap.
const count = Math.min(
OVERLAP_MAX_SAMPLES,
Math.max(2, Math.ceil(duration * OVERLAP_SAMPLE_FPS) + 1),
);
const step = duration / (count - 1);For duration = 20s, Math.ceil(20*8)+1 = 161 → capped to 120 → step = 20/119 ≈ 0.168s → effective ~5.94fps. For duration = 30s, step ≈ 0.253s → ~3.95fps. The comment at :422-430 sells the design as "8fps (~0.125s spacing) lands >= 2 samples inside a window that narrow", but that guarantee holds only for duration <= 14.875s.
Failure mode: long-form compositions (explainer clips, marketing walk-throughs) fall back to a sparser grid than the design claims. Density guarantee erodes silently — the occurrences >= 2 promotion machinery loses its sub-250ms transient coverage past ~15s.
HF's fuzz corpus is short so this may not bite in CI, but production comp durations trend longer.
Fix options: (a) preserve density and drop the tail past 15s: const step = 1 / OVERLAP_SAMPLE_FPS; const count = Math.min(OVERLAP_MAX_SAMPLES, Math.floor(duration * OVERLAP_SAMPLE_FPS) + 1); (b) accept the degradation but return a truncatedAtSeconds signal so downstream consumers can flag long comps. Minimum: document the effective-fps table in the const block so future authors don't assume 8fps everywhere.
— Review by Rames D Jusso
| grid: SampleGrid, | ||
| collected: GridSamples, | ||
| ): Promise<void> { | ||
| if (new Set(collected.geometrySignatures).size <= 1) return; |
There was a problem hiding this comment.
🟠 The fingerprint gate can be fooled by animations whose period aligns with the base-grid step.
async function collectMotionOverlapSamples(
driver: CheckAuditDriver,
grid: SampleGrid,
collected: GridSamples,
): Promise<void> {
if (new Set(collected.geometrySignatures).size <= 1) return;geometrySignatures is populated once per base-grid sample. A composition whose animation period aligns with the base-grid step (e.g. a 1s-period rotation sampled at 1s spacings) can land on the same rotational configuration at every base sample → same bounding rects → same fingerprint → Set.size === 1 → dense pass skipped. This is EXACTLY when it's most needed (fast periodic motion).
Rare in practice because mergeSampleTimes breaks alignment for most real comps (transition/caption/frame-check offsets), but it's asymmetric — false negatives on the gate skip the dense pass ENTIRELY, giving zero coverage where the design promises the most.
Fix (cheap): OR the current signal with motion.times.length > 0 (a comp that had detected motion in the first place trivially animates). Or use a two-fingerprint check across the base grid AND a mid-point sample. Minimum: document the aliasing limitation in the gate comment at :446-455.
— Review by Rames D Jusso
| expect(source).not.toMatch(/prepared\.map\(\(entry\) => entry\.candidate\)/); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🟠 The "surfaces a held content_overlap" test doesn't exercise the dense-vs-sparse distinction it claims to demonstrate.
collectOverlap: vi.fn(async (time: number) => [
layoutIssue("warning", { time, code: "content_overlap" }),
]),This returns a content_overlap warning at EVERY dense sample time (~72 samples for a 9s fake duration at 8fps). The collapse step sees ~72 occurrences → persistence tier trivially promotes to error via the occurrences >= 2 shortcut (see the sibling comment on layoutAudit.ts:324). The test would pass equally well against a broken dense pass that simply promoted every finding — it never asserts occurrences count or firstSeen/lastSeen span.
More importantly: the test doesn't reproduce the target defect (transient crossing seen only by 2 adjacent dense samples that the sparse grid missed). Test 2 (:1364-1373) correctly validates the static-composition gate — that one is fine.
Fix: add a third test where collectOverlap returns a finding at exactly two adjacent dense sample times and nothing anywhere else. Assert (a) driver.collectOverlap.mock.calls.length matches the dense grid size, (b) the surviving finding has occurrences === 2, (c) severity resolves to what the intended semantic is (once the sibling comment's design question is decided).
Also: buildOverlapSampleTimes(duration) at checkPipeline.ts:441 is a pure function and would take ~5 lines to unit-test for the count/bounds/cap-at-120/quantization behavior. Catches density-math regressions the corpus can't.
— Review by Rames D Jusso
| ): Promise<void> { | ||
| if (new Set(collected.geometrySignatures).size <= 1) return; | ||
| const baseTimes = new Set(grid.layoutSamples); | ||
| for (const time of buildOverlapSampleTimes(grid.duration)) { |
There was a problem hiding this comment.
🟠 Dense loop has neither early-terminate nor try/catch — cost + flake risk.
for (const time of buildOverlapSampleTimes(grid.duration)) {
if (baseTimes.has(time)) continue;
await driver.seek(time);
collected.layoutIssues.push(...(await driver.collectOverlap(time)));
}Cost: serial loop of up to ~120 seek + evaluate roundtrips per composition. Each seek waits for GSAP/CSS timeline settle. Once persistence tier is going to promote a finding at occurrences >= 2, additional samples of the same key add zero signal but still pay the cost. The perf claim text-only so it's cheap (comment at :428-430) is defensible per-sample but not per-run.
Flake: neither driver.seek(time) nor driver.collectOverlap(time) is guarded. If either throws mid-loop (transient page-evaluate error, browser blip, timeout on a specific frame), the entire dense pass — and by inheritance the surrounding collectGridSamples await — rejects, aborting the whole check run. Under the sparse grid the same shape existed with 9 chances; now there are ~120, so the throw budget multiplies with the sample count.
Fix: (a) wrap the loop body in try/catch, record the throw as a soft dense_overlap_probe_failed info-level finding rather than aborting. (b) early-terminate when every observed key has already crossed a threshold (say 5 occurrences). (c) at minimum, add a per-sample timeout so a hung seek doesn't stall the whole pass. Neither is a merge blocker — but the const block should carry a worst-case cost note.
— Review by Rames D Jusso
| return issues; | ||
| }; | ||
|
|
||
| // content_overlap only, for the dense motion re-sampling grid (checkPipeline |
There was a problem hiding this comment.
🟡 Comment references a function name that doesn't exist in the codebase.
// content_overlap only, for the dense motion re-sampling grid (checkPipeline
// detectMotionTextOverlap). Two free-positioned text blocks crossing mid-orbitdetectMotionTextOverlap doesn't exist — the actual function is collectMotionOverlapSamples at checkPipeline.ts:457. Reader searching for the reference finds nothing.
Fix: rename to collectMotionOverlapSamples in the comment.
— Review by Rames D Jusso
938c2cf to
3e0c480
Compare
|
@miguel-heygen pushed the fix in False-negative path (animation gate):
Round-2 items folded into the same commit:
CI: the stack's |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 3e0c480d76eed6d4f2dd55e6d59dc72c8a7ac821. The prior correctness blocker is fixed: packages/cli/src/utils/checkPipeline.ts:451-477 no longer gates the dense pass on a sparse fingerprint, and the new regression reproduces an overlap entirely between sparse samples. The 500 ms persistence floor is also restored.
One blocker remains in the corrective delta: packages/cli/src/utils/checkPipeline.ts:432-477 now runs the 8 fps pass unconditionally and raises the cap to 600 seeks, but the production driver maps every one of those calls through packages/cli/src/utils/checkBrowser.ts:347-350 to AUDIT_SEEK_OPTIONS. Those options impose a hard settleMs: 120 at packages/cli/src/capture/captureCompositionFrame.ts:14-19, applied by the timer at :273-275, in addition to double-rAF/font/browser work. That means the pass adds a deterministic sleep floor of ~7.8 s for an 8 s composition and 72 s at the 600-sample cap, before any DOM collection. Calling the overlap query “text-only” does not make the seek cheap. Please use a measured cheaper settle path for this dense probe, or otherwise bound/prove the end-to-end check latency; the current unconditional path is too expensive to ship.
Verdict: REQUEST CHANGES
Reasoning: Correctness is repaired, but the repair introduces a deterministic, user-visible latency regression that scales to at least 72 seconds of sleep per check.
— Magi
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-reviewed at 3e0c480d76eed6d4f2dd55e6d59dc72c8a7ac821 — delta from R1's 938c2cf37.
R1's F1 (dense-pass promotion at ~125ms violating the design's 500ms wall-clock floor) is fixed by isContentOverlapHeldLongEnough now requiring BOTH occurrences >= 2 AND literal ms-span >= 500ms (layoutAudit.ts:321-330). The fingerprint gate is removed cleanly (collectMotionOverlapSamples runs unconditionally at checkPipeline.ts:470-479); the R1 aliasing FN is closed. The between-grid regression at check.test.ts:1349-1367 genuinely reproduces the target defect (collisions living only inside (3.5, 4.5), a gap the sparse grid seeks past). Cap raised 120→600 to hold 8fps up to ~75s.
Deferring to Magi's blocker on the settle-cost multiplier at checkBrowser.ts:347-350 + captureCompositionFrame.ts:14-19 — their line-anchored analysis of the ~72s deterministic sleep floor at the 600-cap is correct and exactly the class of regression that needs a bounded-cost path before merge. My R2 concerns are additive.
Cross-cutting theme: the AND-tighten is a SEMANTICS MIGRATION for external sparse-grid callers, not called out
The AND change fixes the dense-pass problem, but changes semantics for existing external callers who were relying on the old OR:
--samples 20on a 3s composition → 150ms adjacent gap → 2 occurrences that WOULD have promoted toerrorunder OR now staywarning.--at/--at-transitionsdense runs — the pre-R2 design comment (now rewritten) explicitly called these out as ms-floor callers. Same regression.- Short compositions (1.5s at 9 samples → 167ms gap) — same.
The design comment at layoutAudit.ts:186-198 is updated to reflect the new intent ("honored regardless of sampling density"). If that's the intended semantics migration — fine. But the change isn't called out in the PR title/body, and there's no boundary test at the 499/500ms crossing, so a future >/>= typo lands green.
Inline at layoutAudit.ts:321 with three fix options (explicit boundary test, source-tagged branching, or deprecation-log path).
R1 items — carried disposition
- ✅ F1 dense-pass promotion violating 500ms floor — FIXED via AND at persistence tier. Correct architectural level.
- ✅ Fingerprint aliasing gate — REMOVED; pass runs unconditionally.
- ✅ Test doesn't exercise dense-vs-sparse — REPLACED with genuine between-grid test at
:1349-1367. - ✅ Cap concern — MITIGATED; 600 cap holds 8fps to ~75s. (Still silently degrades at >75s — a longer-threshold version of the same failure class.)
- ➖ Dense loop no try/catch + no early-terminate — UNCHANGED at
checkPipeline.ts:474-478. Now WORSE in one specific way: the fingerprint-gate previously short-circuited pathological static comps to zero seeks, so there was implicit budget bounding. Post-R2 the loop runs unconditionally, so a singledriver.seek()failure mid-loop takes down the entire check run for ANY composition. See Magi's blocker on the settle-cost class — try/catch would compose well with whatever bounded-cost fix Magi's finding lands.\n- ➖ Comment refers to non-existentdetectMotionTextOverlapatlayout-audit.browser.js:1429— UNCHANGED. Trivial one-line fix (rename tocollectMotionOverlapSamples).
🟢 Verified
- AND-tighten is at the right architectural level:
isContentOverlapHeldLongEnoughis called only from the promoter branch (layoutAudit.ts:288), NOT fromapplyPersistenceTier's upstream ladder — so the change doesn't leak into other codes' tiering logic. - The design comment at
:186-198matches the new code semantics. HELD_ACROSS_SAMPLES_MIN_OCCURRENCES = 2andCONTENT_OVERLAP_HELD_ERROR_MS = 500co-located at :198-199.buildOverlapSampleTimescap math verified:min(600, ceil(75 * 8) + 1)=min(600, 601)=600— 8fps exactly at the boundary; at 76s it starts degrading.- New between-grid test uses
inBetweenGridWindow = (3.6, 4.4)which sits strictly between default sparse samples 3.5 and 4.5 — so the sparse grid genuinely seeks past it. Real defect reproduction. - Tiering regression test at
layoutAudit.test.ts:230-242verifies 125ms-span stays warning (not error) — but no test at the 499/500ms boundary.
Not blocking on top of Magi's blocker — but if the cost fix ends up bounding dense-pass count differently (e.g. gated on motion presence via a cheaper signal), that may naturally close the try/catch concern too. Would be worth resolving as one round.
| // Split out of applyPersistenceTier so the compound "held long enough" test | ||
| // (>= 2 samples AND wall-clock span >= the ms floor) reads as one boolean | ||
| // question instead of adding a compound branch to the tiering ladder above. | ||
| function isContentOverlapHeldLongEnough(issue: LayoutIssue, occurrences: number): boolean { |
There was a problem hiding this comment.
🟠 The AND-tighten fixes the dense-pass over-promotion, but SILENTLY REGRESSES external sparse-grid callers whose intent was promote-on-any-2-occurrences.
function isContentOverlapHeldLongEnough(issue: LayoutIssue, occurrences: number): boolean {
if (occurrences < HELD_ACROSS_SAMPLES_MIN_OCCURRENCES) return false;
const firstSeen = issue.firstSeen ?? issue.time;
const lastSeen = issue.lastSeen ?? issue.time;
const heldMs = (lastSeen - firstSeen) * 1000;
return heldMs >= CONTENT_OVERLAP_HELD_ERROR_MS;
}The R1 concern was that the old shortcut occurrences >= 2 => promote fires at ~125ms under the new 8fps dense pass, violating the design's 500ms wall-clock floor. The R2 AND-tighten fixes that — good. But it changes semantics for callers who were RELYING on the old OR:
- User-specified
--samples 20on a 3s composition: adjacent-sample gap = 150ms. A genuine held collision hitting 2 sparse occurrences at 150ms apart previously promoted toerror; now stayswarning. --at/--at-transitionsdense runs: the OLD design comment atlayoutAudit.ts:186-198explicitly called these out — quoting the pre-feat(lint): dense motion re-sampling for content_overlap #2746 version: "with the literal ms span kept as a fallback for callers whose samples really are spaced close enough together for the ms floor to matter on its own (dense--at/--at-transitionsruns)". So the R1 design ALREADY anticipated dense sparse-caller sampling. The AND-tighten regresses those callers.- Short compositions: 1.5s composition at 9 samples → 167ms adjacent-gap → same regression.
The R2 commit updates the design comment at :186-198 to say "honored regardless of sampling density" — so the intent IS that these external callers no longer promote at <500ms. That may be the right call! But the change from OR to AND is a semantics migration that:
- Is not called out in the PR title ("dense motion re-sampling" reads as an additive change).
- Is not called out in the commit body's user-facing summary (only mentioned as "honor 500ms floor").
- Has no boundary test at the 499ms/500ms crossing (a future
>/>=typo lands green). - Has no explicit note in the changelog / migration path for the
--at/--samplescallers who now silently see fewer error promotions.
Fix options:
- (a) Explicit design decision + test: add a case at
layoutAudit.test.tswhereoccurrences=2, heldMs=499stays warning ANDheldMs=500promotes — proves the boundary contract and locks it in. Update the PR title/body to call out the semantics change explicitly. - (b) Preserve OR for external callers, apply AND only to the dense-pass source: tag dense-pass findings with an internal
source: 'motion-dense'marker atcheckPipeline.ts:474and branch here on that marker. Keeps the sparse-grid caller behavior unchanged. - (c) Deprecation path: log a warning when a
content_overlapfinding hitsoccurrences >= 2but not the ms floor, mentioning the semantics change and pointing to the ms floor constant.
(a) is probably right — the design change looks intentional and (b) adds an internal-vs-external branch that's harder to reason about. But regardless of which, please add the boundary test.
3e0c480 to
c4d94f2
Compare
Transient text-on-text collisions during continuous motion (e.g. an orbiting label card crossing the center card) overlap for a fraction of a second that the sparse 9-point layout grid seeks straight past. The content_overlap detector is correct; it just never gets a sample at the crossing moment. Rerun ONLY content_overlap on an 8fps grid (text-only, cheap) when the composition animates; findings feed the existing persistence tiering unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round-1 blocker: the dense motion-overlap re-pass was gated on sparse-grid geometry fingerprints changing, so an animation aliased to the sparse grid (identical fingerprints, yet colliding between samples) bypassed the pass — exactly the transient false-negative it was built to catch. Remove the gate: the dense pass now runs unconditionally (bounded, text-only), driven by the composition timeline rather than a fingerprint heuristic. Round-2 follow-ups: - Persistence-tier drift: at 8fps, occurrences>=2 spans only ~125ms, not the ~500ms the design intends, and it short-circuited before the ms floor. content_overlap promotion now requires BOTH occurrences>=2 AND a literal firstSeen..lastSeen span >= 500ms, so the wall-clock floor is honored at any sampling density. Comment block updated to match. - Sample cap scales to hold a true 8fps grid up to ~75s (raised 120 -> 600) with an explicit note that longer comps degrade below 8fps to stay bounded. Tests: - Replaced the trivial "warning at every sample" test with a real between-grid regression: a collision living only inside (3.5,4.5) — a gap the sparse grid seeks past — is detected and, held ~750ms, promoted to error. - Replaced the now-invalid "skips when static" test with one asserting the dense pass runs even when sparse fingerprints are identical (aliased motion). - Added a tiering regression: two dense occurrences spanning ~125ms stay a warning (not error). Both new guards verified red before the fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The dense overlap re-pass did up to OVERLAP_MAX_SAMPLES full-settle seeks (120ms paint settle each, ~72s of pure sleep at the ceiling) even though collectOverlap only reads getBoundingClientRect geometry, valid synchronously after the timeline setTime. Add a settle-free DENSE_GEOMETRY_SEEK_OPTIONS + driver.seekGeometry used only by the dense loop; the base grid keeps full-settle driver.seek. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e callers The occurrences>=2 AND heldMs>=500 promotion rule is a semantics change for sparse callers (--samples 20, --at, short comps) whose two samples can land <500ms apart. Document the change in the tiering comment and add boundary tests: 499ms span stays warning, 500ms span promotes to error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
c4d94f2 to
111ef6e
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Strengths: The 499/500ms semantics boundary is now explicit and pinned at packages/cli/src/utils/layoutAudit.ts:184-203 and packages/cli/src/utils/layoutAudit.test.ts:235-264. The dense loop also moved off the explicit 120ms timer path at packages/cli/src/utils/checkPipeline.ts:468-484.
Blocker: The replacement seek is not actually settle-free. DENSE_GEOMETRY_SEEK_OPTIONS at packages/cli/src/capture/captureCompositionFrame.ts:21-31 spreads AUDIT_SEEK_OPTIONS and overrides only settleMs; it still preserves animationFrameSettle: "double" and waitForFontsMs: 500. seekCompositionTimeline then awaits two rAFs at packages/cli/src/capture/captureCompositionFrame.ts:260-275 and waitForCompositionFonts at :277-284, whose no-load path itself waits another rAF. At the 600-seek ceiling this leaves roughly 1,800 frame waits (about 30s at 60Hz) before overlap collection, plus up to 500ms per seek while fonts report loading. That is materially better than the prior 72s timer floor, but it does not satisfy the new contract/comment that geometry is valid synchronously after setTime.
Please make the dense options genuinely geometry-only (animationFrameSettle: "none", waitForFontsMs: 0, settleMs: 0) and add an options-level regression, or retain the waits only with measured/bounded end-to-end latency that demonstrates the 600-sample path is acceptable.
Verdict: REQUEST CHANGES
Reasoning: Correctness and the explicit timer cost are repaired, but the dense path still retains three frame-settle waits per sample and therefore remains a deterministic large latency multiplier at the advertised cap.
— Magi

What it catches
Transient text-on-text collisions during continuous motion — e.g. an orbiting label card crossing the center card — that overlap for a fraction of a second. The
content_overlapdetector is already correct; the sparse 9-point layout grid simply seeks straight past the crossing moment and never samples it.Root cause of the miss is sampling density, not detection logic: the baseline grid seeks past
t ~ 3.5s, so the fuzz016 collision (a ~0.4s window) is never observed.--samples 30 --at-transitionsalready catches it;motion-fps(20fps) only runs with a.motion.json, which fuzzed comps don't have.How it works
content_overlapdetector verbatim — samecollectSolidTextBlocks, same0.2threshold, zero new detection logic.Corpus evidence (autonomous geometry-fuzz run, 81 fuzzed diagrams)
t = 3.53s, the target defect, previously unseen by the sparse grid.Validation
check.test.ts);bun run buildgreen.Reviewer note — sensitivity
At 8fps the
occurrences >= 2persistence shortcut spans ~125ms (vs ~500ms on the sparse ~500ms grid), so brief-but-real collisions now gate aterror. Every inspected instance was real, but if softer severity is preferred, theheldMs >= 500path is the alternative knob.🤖 Generated with Claude Code