Skip to content

Commit ca4ee36

Browse files
xuanruliclaude
andcommitted
fix(check): uniquely select both ends before keying by pair
Review found the previous commit's premise false: `overlapIssue` used `selectorFor`, which returns `tag.class1.class2` with no uniqueness check, so dropping `text` left nothing to separate two aliased siblings. Two unrelated 125ms transients on sibling stat cards merged into one group whose span bridged the gap between them, promoting past the 500ms floor to `error` — a false failure worse than the false pass being fixed. Both builders now use the file's own `uniqueSelectorFor`, so the pair really is the identity the collapse key assumes. Measured on an aliased-sibling fixture: `error occ=2 div.num` / `ok=false` before, two separate warnings / `ok=true` after. `text_occluded` had the identical defect and is included: it carries both selectors plus the animating element's text, and is persistence-tiered, so a count-up held under an opaque scrim for 6s — a hard `error` at every sample — collapsed into 12 single-occurrence `info` rows and the run passed. Now `error occ=12` / `ok=false`. Per-code policy moved next to `PERSISTENCE_TIERED_CODES` as a named set, and `dedupeLayoutIssues` keeps raw text deliberately (it pins an exact time, where text still separates concurrent findings) with a note so the divergence is not "fixed" back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 7b28267 commit ca4ee36

3 files changed

Lines changed: 44 additions & 11 deletions

File tree

packages/cli/src/commands/layout-audit.browser.js

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -656,8 +656,9 @@
656656
code: "content_overlap",
657657
severity: "warning",
658658
time,
659-
selector: selectorFor(a.element),
660-
containerSelector: selectorFor(b.element),
659+
// Unique both ends: the collapse key names this pair by its selectors alone, so aliased siblings must not share one.
660+
selector: uniqueSelectorFor(a.element),
661+
containerSelector: uniqueSelectorFor(b.element),
661662
text: textContentFor(a.element),
662663
message: "Two text blocks overlap and may render unreadable.",
663664
rect: a.rect,
@@ -1017,8 +1018,9 @@
10171018
code: "text_occluded",
10181019
severity: "error",
10191020
time,
1020-
selector: selectorFor(element),
1021-
containerSelector: selectorFor(occluder),
1021+
// Unique both ends: the collapse key names this pair by its selectors alone, so aliased siblings must not share one.
1022+
selector: uniqueSelectorFor(element),
1023+
containerSelector: uniqueSelectorFor(occluder),
10221024
text,
10231025
message: "Text is hidden beneath an opaque element.",
10241026
rect: textRect,

packages/cli/src/utils/layoutAudit.test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,33 @@ describe("persistence-tiered severity (#U10)", () => {
279279
expect(collapsed[0]).toMatchObject({ severity: "error", occurrences: 2 });
280280
});
281281

282+
it("keeps two content_overlap pairs on different containers in separate groups", () => {
283+
const collapsed = collapseStaticLayoutIssues(
284+
[
285+
{ ...issue("content_overlap", "warning"), time: 4.0, containerSelector: ".num" },
286+
{ ...issue("content_overlap", "warning"), time: 4.5, containerSelector: ".pct" },
287+
],
288+
73,
289+
);
290+
291+
expect(collapsed).toHaveLength(2);
292+
});
293+
294+
it("does not bridge two separate transients on one pair into a held collision", () => {
295+
// Both blips sit under the 500ms floor; spanning them would fabricate a 4.1s collision that never happened.
296+
const blip = { ...issue("content_overlap", "warning"), containerSelector: ".label" };
297+
const collapsed = collapseStaticLayoutIssues(
298+
[
299+
{ ...blip, time: 1.0 },
300+
{ ...blip, time: 1.125 },
301+
],
302+
73,
303+
);
304+
305+
expect(collapsed).toHaveLength(1);
306+
expect(collapsed[0]).toMatchObject({ severity: "warning", occurrences: 2 });
307+
});
308+
282309
it("still separates two distinct text_box_overflow findings that differ only by text", () => {
283310
const collapsed = collapseStaticLayoutIssues(
284311
[

packages/cli/src/utils/layoutAudit.ts

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ export function formatLayoutIssue(issue: LayoutIssue): string {
158158
return issue.fixHint ? `${line}\n Fix: ${issue.fixHint}` : line;
159159
}
160160

161+
// Keeps raw `text` where `staticIssueKey` drops it: this key pins an exact time, so text still separates concurrent findings rather than spanning samples.
161162
export function dedupeLayoutIssues(issues: LayoutIssue[]): LayoutIssue[] {
162163
const seen = new Set<string>();
163164
const result: LayoutIssue[] = [];
@@ -204,6 +205,15 @@ const PERSISTENCE_TIERED_CODES: ReadonlySet<LayoutIssueCode> = new Set([
204205
"connector_detached",
205206
]);
206207

208+
// Codes whose collapse key is the selector pair alone. Both builders uniquely
209+
// select their two ends, so the pair IS the identity — and keeping per-sample
210+
// text would split one held finding into transient groups whenever the subject's
211+
// text animates (count-up, typewriter, rotating word).
212+
const TEXT_AGNOSTIC_KEY_CODES: ReadonlySet<LayoutIssueCode> = new Set([
213+
"content_overlap",
214+
"text_occluded",
215+
]);
216+
207217
export function collapseStaticLayoutIssues(
208218
issues: LayoutIssue[],
209219
totalSampleCount?: number,
@@ -342,18 +352,12 @@ function staticIssueKey(issue: LayoutIssue): string {
342352
issue.severity,
343353
issue.selector,
344354
issue.containerSelector ?? "",
345-
collapseTextKey(issue),
355+
TEXT_AGNOSTIC_KEY_CODES.has(issue.code) ? "" : (issue.text ?? ""),
346356
issue.overflow ? formatOverflow(issue.overflow) : "",
347357
framePositionKey(issue),
348358
].join("|");
349359
}
350360

351-
/** Text identity for the collapse key, omitted where the code identifies itself by geometry instead. */
352-
function collapseTextKey(issue: LayoutIssue): string {
353-
// content_overlap names its pair by both selectors, so animated text would split one held collision into per-sample groups.
354-
return issue.code === "content_overlap" ? "" : (issue.text ?? "");
355-
}
356-
357361
function framePositionKey(issue: LayoutIssue): string {
358362
// connector_detached shares it: id-less paths collapse to one selector, so distinct lines need geometry in the key.
359363
return issue.code === "frame_out_of_frame" || issue.code === "connector_detached"

0 commit comments

Comments
 (0)