-
Notifications
You must be signed in to change notification settings - Fork 3.5k
feat(lint): dense motion re-sampling for content_overlap #2746
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.
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
Changes from all commits
783ef64
96bdf45
7b2538d
3da63c3
5f11160
1ba459a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -417,9 +417,42 @@ async function collectGridSamples( | |
| collected.screenshots.push({ time, pngBase64: capture.pngBase64 }); | ||
| } | ||
| } | ||
| await collectMotionOverlapSamples(driver, grid, collected); | ||
| return collected; | ||
| } | ||
|
|
||
| // Dense grid catches mid-motion text collisions the sparse layout grid seeks past; text-only overlap collection is cheap enough to afford it. | ||
| const OVERLAP_SAMPLE_FPS = 8; | ||
| // Ceiling that bounds dense seeks; past ~75s the grid degrades below 8fps rather than growing the seek budget without limit. | ||
| const OVERLAP_MAX_SAMPLES = 600; | ||
|
|
||
| function buildOverlapSampleTimes(duration: number): number[] { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 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 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 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: — Review by Rames D Jusso |
||
| if (!Number.isFinite(duration) || duration <= 0) return []; | ||
| const count = Math.min( | ||
| OVERLAP_MAX_SAMPLES, | ||
| Math.max(2, Math.ceil(duration * OVERLAP_SAMPLE_FPS) + 1), | ||
| ); | ||
| const step = duration / (count - 1); | ||
| return mergeSampleTimes( | ||
| Array.from({ length: count }, (_, index) => Math.round(index * step * 1000) / 1000), | ||
| ); | ||
| } | ||
|
|
||
| /** Reruns content_overlap on a fine grid, unconditionally rather than gated on fingerprint change, since an animation aliased to the sparse grid collides between identical-fingerprint samples. */ | ||
| async function collectMotionOverlapSamples( | ||
| driver: CheckAuditDriver, | ||
| grid: SampleGrid, | ||
| collected: GridSamples, | ||
| ): Promise<void> { | ||
| const baseTimes = new Set(grid.layoutSamples); | ||
| for (const time of buildOverlapSampleTimes(grid.duration)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 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 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 At the new 8fps dense grid, two adjacent samples span ~125ms. So 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 Your PR-body reviewer note acknowledges this: "At 8fps the The request is: make the choice explicit either in code or in the comment. Three options:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 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 Flake: neither Fix: (a) wrap the loop body in try/catch, record the throw as a soft — Review by Rames D Jusso |
||
| if (baseTimes.has(time)) continue; | ||
| // Settle-free seek: collectOverlap reads getBoundingClientRect geometry, valid synchronously after setTime, so the dense pass skips the per-seek paint settle. | ||
| await driver.seekGeometry(time); | ||
| collected.layoutIssues.push(...(await driver.collectOverlap(time))); | ||
| } | ||
| } | ||
|
|
||
| // Frozen-sweep guard (#U10): compositions this short can legitimately hold a | ||
| // single static frame the whole time (a title card) — never flag those. | ||
| const SWEEP_STATIC_MIN_DURATION_SEC = 3; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,21 +181,7 @@ export function dedupeLayoutIssues(issues: LayoutIssue[]): LayoutIssue[] { | |
| return result; | ||
| } | ||
|
|
||
| // Persistence-tier thresholds (#U10, adapted from Adam Rosler's visual-linter | ||
| // design). The approach doc frames these as held-duration floors — ignore | ||
| // under ~250ms, re-promote content_overlap at >= ~500ms — measured against | ||
| // the SAME firstSeen/lastSeen span this collapse step already tracks. At the | ||
| // default 9-sample grid over a multi-second composition, a single collapsed | ||
| // occurrence is held 0ms (one entrance/exit transient sample) and 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`. Tiering below is written in | ||
| // those sample-count terms (the mapping the approach doc asks to document), | ||
| // 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). The | ||
| // ~250ms ignore floor needs no separate constant — see the occurrences <= 1 | ||
| // branch below. | ||
| // Persistence-tier thresholds (#U10): occurrences>=2 alone can't imply the 500ms floor under dense 8fps sampling, so content_overlap promotion requires a literal firstSeen..lastSeen span >= 500ms — stricter for sparse callers too. | ||
| const CONTENT_OVERLAP_HELD_ERROR_MS = 500; | ||
| const HELD_ACROSS_SAMPLES_MIN_OCCURRENCES = 2; | ||
|
|
||
|
|
@@ -317,11 +303,10 @@ function isCanvasBreachHeldLarge(issue: LayoutIssue, occurrences: number): boole | |
| return overlapX > 0 && overlapY > 0; | ||
| } | ||
|
|
||
| // Split out of applyPersistenceTier so the two independent "held long enough" | ||
| // signals (sample count vs. wall-clock span) read as one boolean question | ||
| // instead of adding a third compound branch to the tiering ladder above. | ||
| // Split out of applyPersistenceTier so the compound "held long enough" test reads as one boolean question. | ||
| function isContentOverlapHeldLongEnough(issue: LayoutIssue, occurrences: number): boolean { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 The AND-tighten fixes the dense-pass over-promotion, but SILENTLY REGRESSES external sparse-grid callers whose intent was 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
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:
Fix options:
(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. |
||
| if (occurrences >= HELD_ACROSS_SAMPLES_MIN_OCCURRENCES) return true; | ||
| // Two samples measure a span, but under dense 8fps sampling that span must still clear the wall-clock floor. | ||
| 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; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 The "surfaces a held content_overlap" test doesn't exercise the dense-vs-sparse distinction it claims to demonstrate.
This returns a
content_overlapwarning 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 theoccurrences >= 2shortcut (see the sibling comment onlayoutAudit.ts:324). The test would pass equally well against a broken dense pass that simply promoted every finding — it never assertsoccurrencescount orfirstSeen/lastSeenspan.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
collectOverlapreturns a finding at exactly two adjacent dense sample times and nothing anywhere else. Assert (a)driver.collectOverlap.mock.calls.lengthmatches the dense grid size, (b) the surviving finding hasoccurrences === 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