fix(check): key content_overlap/text_occluded collapse by pair, and bill a clipped text box once

[xuanruli]

Rebuilt onto today's main (the July branch shared no ancestry after the history rewrite) and consolidated with #2803, which touched the same file.

1. Collapse key (was this PR)
staticIssueKey dropped issue.text for content_overlap and text_occluded. Both builders now emit a unique selector for both ends, so the selector pair alone is the identity. Previously an animating subject — count-up, typewriter, rotating word — changed text every sample and split one held finding into a run of transients.

3. Persistence tiering now measures a contiguous run, not a bounding hull (from review 5127257211)

collapseStaticLayoutIssues grouped with Math.min/Math.max and no adjacency check, so the promotion test read the span between the outermost samples. Dropping text from the key is what brings animating subjects onto that path, so two 125 ms blips 8 s apart merged into one "8-second held collision" and failed the run at error. Tiering now uses the longest contiguous run.

The contiguity break is wall-clock, not grid-derived (from review 5127334595). A first attempt broke the run at 1.5x the sample spacing, which failed both ways: check re-samples content_overlap on its own dense 8fps grid but collapses against browser.layoutSamples, so the tolerance came from a grid the findings were never sampled on; and the spacing was a global minimum, so one tight transition pair anywhere set the tolerance for every finding in the run. A run now breaks on a gap wider than twice the 500 ms promotion floor — a gap that wide cannot evidence a continuous hold — which needs no grid at all. Tests pin both failure directions: two blips 1.5 s apart stay warning under either grid, and a 625 ms contiguous collision still promotes with a 50 ms pair sitting elsewhere in the same run.

The test that named this case (does not bridge two separate transients on one pair into a held collision) sampled t=1.0 and t=1.125 — 125 ms, under the 500 ms floor, so it passed whether or not the bridging happened. It now uses two blips 8 s apart, and a second test pins that one genuinely contiguous run still promotes.

Rollout note: both codes are persistence-tiered, so merging previously-distinct per-frame entries raises occurrences and widens the span. content_overlap can go warning → error; text_occluded stops being demoted to info at a single occurrence and stays error. That is the intended fix, but it will turn some previously-green projects red, and those entries now sort ahead of other findings under maxIssues.

2. Clipped text billed once (was #2803)
A clipping box that is its own nearest constraint reports clipped_text only, instead of also reporting text_box_overflow for the same defect. A non-clipping self-constraint still reports.

The suppression is now gated on clippedTextIssue having actually produced a finding, not on re-deriving its predicate. The two measure different things — clipped_text uses integer scrollWidth - clientWidth against the padding box, text_box_overflow uses fractional rect geometry against the border box — so a re-derived predicate lets an overflow of 2.01–3.0 px fall through both. It is additionally limited to right/bottom overflow, because scroll metrics by spec never report leftward or upward overflow. Tests pin both cases.

Tests: 190 pass on the four affected files (main: 182).

Known gap: nothing exercises the selectorFor → uniqueSelectorFor swap itself. That swap is the precondition for selector-only identity, and it also carries a risk worth naming — in a composition that recreates nodes per frame (SplitText spans, re-rendered lists), a structural parent > tag:nth-of-type(n) fallback can resolve to a different element at different samples, which text previously kept apart. uniqueSelectorFor is a pre-existing helper already used elsewhere, but the overlap test harness is id-keyed and cannot express aliased siblings without a parallel fixture.

Closes #2803.

[james-russo-rames-d-jusso]

:large_green_circle: 87bbeb09c COMMENTED

Hey Xuanru — clean fix on both fronts. The pair-only key with uniqueSelectorFor on both ends closes the animating-subject split, and the clippedIssue-gated suppression avoids the fractional-vs-integer trap you called out. No blockers. Landing thoughts below.

Verified as OK

• TEXT_AGNOSTIC_KEY_CODES in staticIssueKey — old key concatenated issue.text unconditionally, so an animating count-up over one label split into N transients that each demoted to warning/info under persistence tiers. New key (staticIssueKey in layoutAudit.ts, gated by the set at :207 and the set-membership check at :350) drops text for the two two-element codes only; every other code (text_box_overflow et al.) still keys on text. The still separates two distinct text_box_overflow findings that differ only by text test pins the scope so the change can't drift.
• content_overlap + text_occluded builders swapped to uniqueSelectorFor on both ends — required precondition for a selector-only identity, because selectorFor can class-collide (2-class prefix) and let two distinct pairs alias into one key. uniqueSelectorFor falls back to a structural parent > tag:nth-of-type(n) chain when the preferred selector isn't unique, so identity holds.
• billedAsClippedText gate in textOverflowIssues — verified at layout-audit.browser.js:465-467, gated on the actual clippedIssue object, not on re-deriving clipsOverflow(...) && scrollWidth-clientWidth>tol inline, so the two measurement systems (integer scroll metrics vs padding box, fractional rects vs border box) can't disagree at the boundary. left==null && top==null guard preserves emission for leftward/upward spill, which clipped_text provably can't see (scrollWidth/scrollHeight are one-sided by spec). Non-clipping self-constraint still fires — the clipsOverflow guard in clippedTextIssue makes clippedIssue null on that path, so billedAsClippedText is false. Confirmed by the "painted, NON-clipping box" test.
• Test discriminators actually probe the boundary — the count-up test in layoutAudit.test.ts is exactly the old defect (same pair, changing text → expects one collapsed error, not two warnings). The scroll metrics round below tolerance test in the browser file is the 2.5px fractional-vs-integer case you named. Neither is happy-path.
• Blast radius is local — staticIssueKey is a private function (dedup only); the only external effect is that persisted content_overlap / text_occluded findings now carry longer structural selectors when the class/data selector isn't unique. No downstream schema/DB consumer keys off selector shape from a scan of packages/cli/src.
• Fix is properly scoped — of the 10 layout codes emitted, only content_overlap and text_occluded involve two independently-named elements; the other builders single-element or single-path. No sibling with the same asymmetric-pair defect that this fix should widen to.

Concerns (non-blocking)

• Pair symmetry is implicit, not enforced — content_overlap's key is (uniqueSelectorFor(a), uniqueSelectorFor(b)) with A/B roles asymmetric. It only holds because contentOverlapIssues (layout-audit.browser.js:683) iterates blocks[i], blocks[j] with i<j over document.querySelectorAll("*") (document order, stable per frame). Any future refactor that sorts blocks by area / filters mid-loop / uses a different traversal could silently flip a pair across frames and re-introduce the split. A [a,b].sort().join('|') symmetric key would be robust; not this-PR-scope, but worth a comment near contentOverlapIssues naming the invariant, or a follow-up.
• Rollout ratchet you already flagged — worth spelling out in the release note that projects with previously-transient content_overlap on animating subjects will now surface a promoted error and sort ahead of other findings under maxIssues. Users will read this as a regression unless the note is prominent.
• Coverage gap on the overflow.left/top guard — no test exercises the case where clippedIssue fires (right/bottom spill) AND text_box_overflow additionally sees leftward/upward spill on the same element (e.g. negative-margin transform). The current gate correctly emits both, but nothing pins that behavior. One extra fixture would seal it.
• Known gap you named — uniqueSelectorFor on recreated-per-frame nodes — SplitText spans / re-rendered lists can alias siblings via the structural fallback across samples. You flagged this and noted the harness limitation; agreed it's a follow-up not a blocker, but it's the one shape where this fix could over-collapse.

What I didn't verify

• Real-run behavior against a project fixture that used to split — only source and unit tests. If you have a snapshot-diff run against a project that previously showed the transient split, that'd be the definitive rollout check.
• Whether .check/report.json consumers downstream of this CLI grep selector strings — scanned packages/cli/src only.

State at HEAD 87bbeb09c: isDraft: false, mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED. CI: Tests on windows-latest aggregator went red at 00:26:06Z because studio-engine-cli was still in_progress — that child lane finished success at 00:37:14Z, so the red is a stale race, not diff-caused. A rerun of the aggregator (or any push) should clear it before merge.

Stamp goes through a HyperFrames-authorized reviewer — heygen-com/hyperframes runs dismiss_stale=false / require_last_push_approval=true, so pin the stamp to the exact SHA that lands.

— Review by Rames D Jusso

[jrusso1020]

Requesting changes at 87bbeb09ce9b6382fb9adf8e7e2f64a840184f9c on one item. Everything else here is additive to the existing review at this same SHA -- I deliberately went looking for what that review did not already cover, so the symmetry note and the per-frame uniqueSelectorFor gap are only sharpened below, not restated.

The direction of this PR is right and most of it verified clean. One consequence needs fixing first.

Blocker: the collapse span is a bounding hull, not a held duration

isContentOverlapHeldLongEnough (layoutAudit.ts:312-319) promotes warning -> error on (lastSeen - firstSeen) >= 500ms. collapseStaticLayoutIssues (:226-242) groups purely by staticIssueKey and keeps Math.min(firstSeen) / Math.max(lastSeen) with no adjacency or gap check, so that span is the bounding hull of all occurrences rather than a contiguous run.

Dropping text from the key at :350 is what makes this reachable for animating subjects, which is exactly the class this PR is written to un-split.

I ported collapseStaticLayoutIssues, applyPersistenceTier and isContentOverlapHeldLongEnough verbatim from this SHA and ran them. Input is two separate 125ms crossfade blips 8 seconds apart on a count-up subject -- a label and a counter that cross on entrance and again on exit, with the counter text differing per sample:

input: content_overlap #label/#counter at t = 1.0, 1.125, 9.0, 9.125

BEFORE (text in key, = main):
   info  occurrences=1  t=1-1s
   info  occurrences=1  t=1.125-1.125s
   info  occurrences=1  t=9-9s
   info  occurrences=1  t=9.125-9.125s
   -> errorCount=0   check ok=true

AFTER (text dropped, = this PR):
   error occurrences=4  t=1-9.125s
   -> errorCount=1   check ok=false

So hyperframes check now fails the run on an 8-second "held collision" that never existed. section() sets ok: errorCount === 0, and the dense overlap grid samples at 8fps across the whole composition, so two transients on one pair is ordinary shape rather than a corner case.

Worth noting the code already agrees with me about the intent -- the applyPersistenceTier docstring says the promotion is for "when the collision is sustained rather than a crossfade blip." The hull defeats that for precisely the two-blip case the sentence describes.

In fairness: the hull behaviour pre-dates this PR -- constant-text pairs already merged this way. I am blocking on it because this PR is the thing that brings animating-text subjects into that path, and those are the subjects it exists to serve. A false-positive-reduction change that introduces a new false error is the shape most likely to get the gate switched off.

What makes it blocker-grade rather than a note: the PR ships a test that reads as preventing this and does not.

layoutAudit.test.ts:316 is titled "does not bridge two separate transients on one pair into a held collision" and uses time: 1.0 and time: 1.125. That span is 125ms, which never reaches the 500ms floor, so the test passes because it is one short hold, not because bridging is prevented. Same fixture with the second sample at 9.0:

PR test as written (t=1.0, 1.125): severity=warning   <- what it asserts
Same test, second sample at 9.0  : severity=error     <- what the title claims is prevented

A future reader takes that title as a guarantee.

Fix direction: track the longest contiguous run while grouping -- break the run when the gap between consecutive occurrences exceeds a sample-spacing threshold -- and tier on that instead of on the hull. Then retitle or re-fixture :316 so it actually pins it.

Non-blocking

1. container === element is gated on a different opacity floor than the rest of the path, so the double-bill survives mid-fade. layout-audit.browser.js:1444 admits elements via isVisibleElement(element, 0.05) (any single ancestor below 0.05), while nearestConstraint (:360) uses isVisibleElement(current) with no floor, i.e. opacityChain(element) < 0.2 on the product. So <div style="opacity:.5"><p id="cap" style="overflow:hidden;opacity:.35"> gives a chain of 0.175: the element passes the outer filter and clipped_text fires, but nearestConstraint skips the element itself and returns an ancestor, so container !== element, the gate never arms, and both clipped_text and text_box_overflow are emitted for the same defect. The conjunct is semantically right; the two floors just disagree across the 0.05-0.2 effective-opacity band.

2. The gate drops the whole finding rather than the double-billed axis. layout-audit.browser.js:465-470. With #cap at border box 200x20, clientWidth 200 / scrollWidth 240 gives clipped_text {right: 40}; clientHeight 20 / scrollHeight 20 (scroll metrics do not grow for glyph-ink overshoot) but the Range ink rect is 26px, so containerOverflow = {right: 40, bottom: 6}. The gate arms and suppresses text_box_overflow entirely, while clipped_text's hint -- built from {right: 40} -- says only "widen the text box to at least ~240px". The 6px vertical cut-off is reported by nothing. This bites specifically because :461-463 keeps verticalTolerance tight on a clipping box so that a real cut-off does get reported. A per-axis subtraction is strictly better and subsumes the left/top guard for free, since clippedTextIssue can only ever set right/bottom.

3. Three of the gate's four conjuncts are pinned by nothing. Only three tests in the 2680-line browser suite fabricate scroll metrics (layout-audit.browser.test.ts:274, 313, 345); elsewhere jsdom leaves scrollWidth === clientWidth === 0. Of those, :313 (202 vs 200, diff 2, not > 2) and :345 (100 vs 100) both yield clippedIssue === null, so the gate is inert in both new tests. Exactly one assertion exercises its true branch (:295). Deleting container === element, containerOverflow != null, or the left == null && top == null conjunct breaks no test.

4. layout-audit.browser.test.ts:367 asserts the wrong field for its own title. "still flags a painted, non-clipping box that is its own nearest constraint" asserts found[0]?.selector === "#bubble", but selector is selectorFor(element) regardless of what nearestConstraint returned, so the "its own nearest constraint" premise is never checked. containerSelector would check it. (Same shape as the blocker above -- worth a pass over test titles generally.)

5. Pair symmetry is reachable today, not just under a future refactor. Orientation comes from blocks[i], blocks[j] with i<j over root.querySelectorAll("*") (:683-692), i.e. live document order. parent.appendChild(el) to raise stacking order moves an existing child to the end; a FLIP or leaderboard re-sort does the same. A bar-chart race is the canonical case -- the labels collide while swapping, and the swap is the reorder. (A,B) before and (B,A) after become two groups, each often one occurrence, both demoted to info, and the run stays green. [a,b].sort().join("|") in the key would settle it. Related and unmentioned: for text_occluded the container end is a probe-order artifact, since occlusionCoverage (:967-983) returns the first hit across a fixed 27-point grid, so text drifting under two adjacent overlays flips containerSelector and re-splits the key.

General shape worth naming: a split key demotes to info, ok is errorCount === 0, and nothing counts demotions -- so a silently-split finding is indistinguishable from a clean run.

PR body vs source

Verified true: the TEXT_AGNOSTIC_KEY_CODES scoping; uniqueSelectorFor on both ends of both builders; "selector pair alone is the identity" (overflow and framePositionKey are both provably empty for these two codes); the rollout consequences; errors sorting ahead under maxIssues; the gate keyed on the actual clippedIssue object rather than a re-derived predicate; left/top exclusion being correct; the 182 -> 190 arithmetic; and your own note that nothing exercises the selectorFor -> uniqueSelectorFor swap, which is accurate since every existing overlap test is id-keyed. Your scoping check is also right: of the persistence-tiered codes, content_overlap and text_occluded are the only two naming independent element pairs.

"Tests pin both cases" is half true. The rounding case (:303) genuinely pins that the predicate was not re-derived. The left case (:335) has scrollWidth === clientWidth, so clippedIssue is null and the gate never arms -- it pins that leftward spill reports when nothing is clipped, which was already true on main. The conjunct it is meant to guard is untested (note 3).

Minor: "an overflow of 2.01-3.0 px falls through both" -- at exactly 3.0 the integer diff is 3, which is > 2, so clipped_text does fire. The band is open at the top.

Method

I read the code at the SHA above and executed the ported collapse/tiering functions for the reproduction; I did not run the repo suite. CI is green on all eight required contexts at this head, so this is a code finding, not a CI one. #2805 is approved and #2811 is in progress -- this blocker is specific to #2801 and does not hold up the other two.

Happy to re-review quickly on a push.

-- Rames

[jrusso1020]

Re-verified at b9349e2ee9452d5299bada26782a5f247224b1fc. The case I filed is fixed and the contiguous-run idea is the right one -- but I have to hold, because the tolerance that decides contiguity is derived from the wrong grid, and that reopens the same defect at short range while adding the opposite one. Everything below is from executing the real layoutAudit.ts at this head.

First, what works

two 125ms blips 8.0s apart, uniform grid  -> warning  heldMs=125   check ok=true    (was: error)
contiguous 625ms run,       uniform grid  -> error    heldMs=625   check ok=false   (still promotes)

Your test moved to 9.0 does go red on the old code. longestContiguousRunMs is the right mechanism, heldMs is threaded correctly, and applyPersistenceTier is private and only called from inside the collapse -- so the issue.heldMs ?? (lastSeen - firstSeen) hull fallback at layoutAudit.ts:367 is dead code today rather than a live second path. Good.

The blocker: check collapses content_overlap against a grid it was never sampled on

collectMotionOverlapSamples (checkPipeline.ts:456-467) runs unconditionally (:434) and re-samples content_overlap on its own dense grid -- buildOverlapSampleTimes, uniform 8fps, step=0.125s for a 10s composition -- pushing those findings into collected.layoutIssues (:466). But the collapse is handed browser.layoutSamples (:1413 -> :1432), which is the base/transition/caption/frame grid and excludes every one of those dense times. So smallestPositiveGap measures a grid the issues do not live on:

content_overlap sampled on: step=0.125s, 81 points
collapse handed:            step=1.111s,  9 points   -> tolerance = 1.6665s

Two live consequences on the default check path, both executed:

(a) the false error survives at short range. Tolerance 1.6665s bridges any two isolated transients closer than that:

two isolated 1-sample blips 1.5s apart -> error  heldMs=1500  check ok=false
two isolated 1-sample blips 8.0s apart -> warning heldMs=0    check ok=true

Same defect I filed, now bounded by the tolerance instead of unbounded. Note this class is newly reachable for animating subjects, since dropping text from the key is what brings them into this path.

(b) a genuine held collision is silently disarmed. smallestPositiveGap takes a global minimum, so one tight pair anywhere in the timeline sets the tolerance for every group. With transition boundaries at 2.0/2.05/2.1 -- nowhere near the collision -- tolerance becomes 0.075s, every 0.125s dense-grid gap exceeds it, and the run shatters into singletons:

genuine 625ms contiguous collision, grid = dense (its own)      -> error   heldMs=625
                                    grid = base only            -> error   heldMs=625
                                    grid = base + transitions   -> warning heldMs=0    check ok=true

heldMs=0 with occurrences=6 is the tell. This one also reaches hyperframes layout --at-transitions, where the grid is genuinely merged.

Fix direction

The threshold this feeds is wall-clock (500ms), so deriving its companion from a grid's global minimum couples it to unrelated parts of the timeline. Simplest robust fix is to make the contiguity break wall-clock too -- break when a gap exceeds a constant -- which removes the grid dependency entirely and fixes (a) and (b) together. If you'd rather keep it grid-relative, it needs to be the grid the code was actually sampled on and a robust statistic (median positive gap) rather than the global minimum; passing the merged dense grid alone still leaves (b), since one tight pair still wins a min.

Worth adding a pin for each direction: two transients ~1s apart must stay warning, and a contiguous run must stay error when the grid also contains a tight unrelated pair. Neither is expressible on a uniform fixture, which is why the current tests pass.

To be clear about what this is: the mechanism you built is correct and the fixture problem is genuinely solved. This is the plumbing that feeds it. And I'll own half of it -- I told you the span needed to be a contiguous run and said nothing about where the spacing comes from, which is the part that actually decides it.

-- Rames

[jrusso1020]

Approving at 6784277646a4662eacb27b2f8e17f4341679eab0. Both blockers are closed, and I verified it by running this head and the previous one side by side rather than reading the diff.

Both blockers closed

Removing the grid was the right call, and it resolves the root cause rather than the symptom. spacing is gone, longestContiguousRunMs takes only times, and checkPipeline.ts:1433 is back to collapseStaticLayoutIssues(deduped, totalSampleCount) off browser.layoutSamples.length. The dense-8fps-vs-base-grid mismatch can no longer reach the tolerance at all; totalSampleCount now feeds only multiSampleRun = sampleCount > 1, which is a correct use of a count and insensitive to which grid produced it.

Differential, both files executed against the same fixtures:

case	prev b9349e2	this head
two blips 1.5s apart, grid=9	error heldMs=1500	warning heldMs=0
two blips 1.5s apart, grid=81	error heldMs=1500	warning heldMs=0
625ms contiguous run + 50ms pair elsewhere	warning heldMs=0	error heldMs=625

So the false positive and the false negative both close, and the 625ms run no longer depends on what else is in the timeline. My original 8s case stays warning at both heads, as it should.

Your "each fails against the previous head" claim checks out for the two cases it covers -- I ran the new fixtures against b9349e2 and both flip. Worth stating because a regression test that passes at the previous head documents nothing.

On the 2x choice -- you asked, so: I read it differently, but not enough to hold the PR

The judgement is sound where the grid is sparse. With two samples 600ms apart and nothing in between, you genuinely cannot distinguish a hold from two blips, and promoting is the safe read. My disagreement is that the same assumption gets applied where the grid is dense, and there the gap is not unknown -- it is observed absence.

Three probes at this head:

• 5 transients at exactly 1.0s intervals -> error, heldMs=4000. Under 8fps sampling that is 7 consecutive samples showing no overlap between each pair, and the run is reported as a 4-second continuous hold.
• two blips 900ms apart -> error, heldMs=900.
• the boundary is sharp: gap of exactly 1000ms promotes, 1001ms does not.

The part I would call a defect rather than a taste difference is the last mile of that first line. heldMs reads as a held duration and is surfaced as one, but it measures the span of a run that may contain observed absence. The floor exists to separate transients from holds, so a field that reports 4000ms for something never observed for more than 125ms weakens the distinction the floor is drawing.

It also lands on the subject this PR is about. Text-agnostic keying exists so a count-up ticker's changing label stops splitting the group -- and a 1-second ticker is the canonical instance of that, sitting exactly on the cliff. Your flagship test (two ticks 600ms apart -> error) is the same doctrine at two samples, so I read this as deliberate rather than missed. If it is deliberate, that is a legitimate call and this note is just the dissent on the record.

If you ever want it tighter, the information is available: break a run when an intervening sample observed no overlap, which needs the sampling times the overlap was actually collected on -- the dense grid, not the base one. That is grid-awareness again, but keyed to the right grid, which was the actual bug rather than grid-awareness itself. Not worth doing now; worth a comment at CONTIGUOUS_SAMPLE_GAP_MS naming the 1s rule as a judgement and why, so the next person to widen it sees the constraint at the site.

The other half, and one thing it depends on

The selectorFor -> uniqueSelectorFor swap at the two emit sites is what makes text-agnostic keying safe, and it is easy to miss that the two changes are coupled. Dropping text from the key is only sound if the selector alone separates distinct elements; uniqueSelectorFor earns that by checking querySelectorAll(preferred).length === 1 and otherwise building a structural :nth-of-type path. Without it, two different overlapping pairs sharing a class would have merged into one group and inflated both occurrences and heldMs. Worth calling out in the PR body, since a later reader could revert the selector half thinking it is cosmetic.

One consequence I have not demonstrated, flagging it only so it is on the record: :nth-of-type indices are positional, so if a subject adds or removes siblings mid-render, the same logical element can produce different selectors across samples (splitting a genuine hold) or two different elements can collide on one selector (merging distinct pairs). A count-up ticker that only swaps text is unaffected. I have no repro for this and did not look for one.

billedAsClippedText reads correctly to me: it only suppresses the text_box_overflow when the container is the element itself and the overflow is purely right/bottom, so a left/top breach still bills separately. That is the right asymmetry.

Notes

• heldMs is a new optional field on LayoutIssue, longestContiguousRunMs is module-private, single caller. No downstream reach.
• isContentOverlapHeldLongEnough keeps issue.heldMs ?? (lastSeen - firstSeen) * 1000. The fallback is unreachable from the collapse (which always sets heldMs), so it only matters if the tier is ever applied to an issue built elsewhere. Fine as a guard, just noting it is currently dead.
• CI is green at this head: Test (7m42s), Tests on windows-latest and both its lanes (studio-core, studio-engine-cli), typecheck, build, render, preflight, Fallow audit. The one red check is Tests on windows-latest: ${{ matrix.lane }} in run 34073562919, which is a cancelled duplicate dispatch on this same SHA, superseded by run 34073584474 -- zero steps, started and completed in the same second. Not a code failure; worth knowing so nobody blocks the merge on it.

Approving on the verdict only; the merge call is not mine.

-- Rames