From 8c2fa8cfd99e6e2bbeef8eca5c357dcf2e2ac752 Mon Sep 17 00:00:00 2001 From: Xuanru Li <157947275+xuanruli@users.noreply.github.com> Date: Sun, 26 Jul 2026 05:52:21 -0700 Subject: [PATCH] fix(check): keep evaluating a motion spec past an ambiguous selector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One selector matching more than one element used to disable the whole motion audit. `planMotionSampling` set `times = []`, and the evaluate step was skipped a second time by a `motionIssues.length === 0` guard, so every other assertion in the sidecar went unevaluated while the report still read `enabled: true`. Measured on two fixtures with identical HTML: the spec without an ambiguous selector reports `samples=121` and its missing-selector finding; adding one ambiguous selector drops it to `samples=0` and that finding disappears. Only the assertions naming an ambiguous selector are dropped now. The rest are sampled and evaluated, and their findings are appended to the preflight ones rather than replacing them. Filtering a spec down must not be silent either — `samples === 0` was the only signal that part of a spec went unevaluated, and sampling again removes it. The ambiguity finding now names what it cost, so the human report says so too. The selector-to-assertion mapping lives in `assertionTargets` in motionAudit.ts, where `collectSamplingTargets` already folds over it, behind an exhaustive switch: a new assertion kind that forgets to declare its targets fails to compile instead of having its ambiguous selector reported and then evaluated against an arbitrary first match. This is strictly louder and never newly-breaking: reaching this path means a hardcoded-error `motion_selector_ambiguous` was already present, so the run had already failed. All 12 motion fixtures keep their verdicts; 13 registry examples are byte-identical. Co-Authored-By: Claude Opus 5 --- packages/cli/src/commands/check.test.ts | 85 +++++++++++++++++++++++++ packages/cli/src/utils/checkPipeline.ts | 38 ++++++++--- packages/cli/src/utils/motionAudit.ts | 42 ++++++++---- 3 files changed, 143 insertions(+), 22 deletions(-) diff --git a/packages/cli/src/commands/check.test.ts b/packages/cli/src/commands/check.test.ts index 8d9a07f97e..2a9c6f77b1 100644 --- a/packages/cli/src/commands/check.test.ts +++ b/packages/cli/src/commands/check.test.ts @@ -934,6 +934,91 @@ describe("check pipeline", () => { expect(browser).toHaveBeenCalledTimes(1); }); + // An ambiguous selector used to zero the sample grid, so every other assertion in the sidecar + // went unevaluated with no trace — the section still reported `enabled`. + it("keeps evaluating the unambiguous assertions when one selector is ambiguous", async () => { + const motion: MotionSpecResolution = { + kind: "valid", + path: "/project/index.motion.json", + spec: { + assertions: [ + { kind: "appearsBy", selector: ".item", bySec: 1 }, + { kind: "appearsBy", selector: "#hero", bySec: 0.2 }, + ], + }, + }; + const driver = fakeDriver({ + getDuration: vi.fn(async () => 1), + findAmbiguousSelectors: vi.fn(async () => [ + { ...layoutIssue("error", { code: "motion_selector_ambiguous" }), selector: ".item" }, + ]), + collectMotionFrame: vi.fn(async (time: number) => heroMotionFrame(time, (t) => t >= 0.5)), + }); + const { report } = await runScenario(driver, {}, { motion }); + + expect(report.motion.samples).toBeGreaterThan(0); + expect(report.motion.findings.map((finding) => finding.code).sort()).toEqual([ + "motion_appears_late", + "motion_selector_ambiguous", + ]); + expect(report.ok).toBe(false); + // The grid is sampled again, so the report must say on its own that part of the spec was dropped. + expect( + report.motion.findings.find((finding) => finding.code === "motion_selector_ambiguous") + ?.message, + ).toContain("1 assertion(s) naming it were not evaluated"); + // Only the surviving assertion's selector is sampled. + expect(driver.collectMotionFrame).toHaveBeenCalledWith(expect.any(Number), ["#hero"], []); + }); + + it("drops a before assertion when either side is ambiguous, keeping the rest", async () => { + const motion: MotionSpecResolution = { + kind: "valid", + path: "/project/index.motion.json", + spec: { + assertions: [ + { kind: "before", a: "#hero", b: ".item" }, + { kind: "appearsBy", selector: "#hero", bySec: 0.2 }, + ], + }, + }; + const driver = fakeDriver({ + getDuration: vi.fn(async () => 1), + findAmbiguousSelectors: vi.fn(async () => [ + { ...layoutIssue("error", { code: "motion_selector_ambiguous" }), selector: ".item" }, + ]), + collectMotionFrame: vi.fn(async (time: number) => heroMotionFrame(time, (t) => t >= 0.5)), + }); + const { report } = await runScenario(driver, {}, { motion }); + + expect(report.motion.findings.map((finding) => finding.code).sort()).toEqual([ + "motion_appears_late", + "motion_selector_ambiguous", + ]); + // `#hero` survives via the appearsBy assertion even though the before naming it was dropped. + expect(driver.collectMotionFrame).toHaveBeenCalledWith(expect.any(Number), ["#hero"], []); + }); + + it("does not sample when every assertion names an ambiguous selector", async () => { + const motion: MotionSpecResolution = { + kind: "valid", + path: "/project/index.motion.json", + spec: { assertions: [{ kind: "appearsBy", selector: ".item", bySec: 1 }] }, + }; + const driver = fakeDriver({ + getDuration: vi.fn(async () => 1), + findAmbiguousSelectors: vi.fn(async () => [ + { ...layoutIssue("error", { code: "motion_selector_ambiguous" }), selector: ".item" }, + ]), + }); + const { report } = await runScenario(driver, {}, { motion }); + + expect(report.motion.samples).toBe(0); + expect(report.motion.findings.map((finding) => finding.code)).toEqual([ + "motion_selector_ambiguous", + ]); + }); + it("reports a failing appearsBy sidecar as motion_appears_late", async () => { const motion: MotionSpecResolution = { kind: "valid", diff --git a/packages/cli/src/utils/checkPipeline.ts b/packages/cli/src/utils/checkPipeline.ts index a02fb2fa5b..80a7bd6f04 100644 --- a/packages/cli/src/utils/checkPipeline.ts +++ b/packages/cli/src/utils/checkPipeline.ts @@ -15,12 +15,13 @@ import { type LayoutRect, } from "./layoutAudit.js"; import { + assertionTargets, collectSamplingTargets, evaluateMotion, type Canvas, type MotionFrame, } from "./motionAudit.js"; -import { findMotionSpec, readMotionSpec } from "./motionSpec.js"; +import { findMotionSpec, readMotionSpec, type MotionAssertion } from "./motionSpec.js"; import { normalizeErrorMessage } from "./errorMessage.js"; import { parseColorRGBA, @@ -167,6 +168,8 @@ interface MotionPlan { selectors: string[]; livenessScopes: string[]; preflightIssues: AnchoredLayoutIssue[]; + /** Assertions naming no ambiguous selector; a zero-match selector survives and reports as missing. */ + assertions: MotionAssertion[]; } async function planMotionSampling( @@ -175,13 +178,30 @@ async function planMotionSampling( duration: number, ): Promise { if (motion.kind !== "valid") { - return { times: [], selectors: [], livenessScopes: [], preflightIssues: [] }; + return { times: [], selectors: [], livenessScopes: [], preflightIssues: [], assertions: [] }; } - const targets = collectSamplingTargets(motion.spec.assertions); - const preflightIssues = await driver.findAmbiguousSelectors(targets.selectors); + const preflightIssues = await driver.findAmbiguousSelectors( + collectSamplingTargets(motion.spec.assertions).selectors, + ); + // One ambiguous selector used to zero the grid, so every other assertion went unevaluated in + // silence. Drop only the assertions that name it; the rest still run. + const ambiguous = new Set(preflightIssues.map((issue) => issue.selector)); + const assertions = motion.spec.assertions.filter( + (assertion) => + !assertionTargets(assertion).selectors.some((selector) => ambiguous.has(selector)), + ); + noteSkippedAssertions(preflightIssues, motion.spec.assertions.length - assertions.length); + const targets = collectSamplingTargets(assertions); const times = - preflightIssues.length === 0 ? buildMotionSampleTimes(motion.spec.duration ?? duration) : []; - return { times, ...targets, preflightIssues }; + assertions.length > 0 ? buildMotionSampleTimes(motion.spec.duration ?? duration) : []; + return { times, ...targets, preflightIssues, assertions }; +} + +/** Filtering a spec down must not be silent: a sampled grid no longer says on its own that part of the spec went unevaluated. */ +function noteSkippedAssertions(issues: AnchoredLayoutIssue[], skipped: number): void { + if (skipped <= 0) return; + const suffix = ` ${skipped} assertion(s) naming it were not evaluated.`; + for (const issue of issues) issue.message += suffix; } interface GridSamples { @@ -1000,13 +1020,13 @@ export async function runAuditGrid( const seekLoopMs = Date.now() - seekLoopStart; let motionIssues = plan.preflightIssues; - if (motion.kind === "valid" && motionIssues.length === 0 && collected.motionFrames.length > 0) { + if (motion.kind === "valid" && plan.assertions.length > 0 && collected.motionFrames.length > 0) { const evaluated = evaluateMotion( collected.motionFrames, - motion.spec.assertions, + plan.assertions, await driver.getCanvas(), ); - motionIssues = await driver.anchorMotionIssues(evaluated); + motionIssues = [...motionIssues, ...(await driver.anchorMotionIssues(evaluated))]; } const sweepFindings = detectSweepStatic( grid.duration, diff --git a/packages/cli/src/utils/motionAudit.ts b/packages/cli/src/utils/motionAudit.ts index 368de902ee..03d6d09ae0 100644 --- a/packages/cli/src/utils/motionAudit.ts +++ b/packages/cli/src/utils/motionAudit.ts @@ -240,6 +240,32 @@ export function evaluateMotion( * Selectors and liveness scopes the in-page sampler must read for a spec. * Selectors feed the per-element matrix; scopes feed keepsMoving liveness. */ +/** + * What one assertion resolves against: `selectors` are preflighted for ambiguity + * and sampled per element, `scopes` drive liveness. Exhaustive by design — a new + * assertion kind must declare its targets here or fail to compile, because a + * kind missing from this map would have its ambiguous selector reported AND + * still evaluated against an arbitrary first match. + */ +export function assertionTargets(assertion: MotionAssertion): { + selectors: string[]; + scopes: string[]; +} { + switch (assertion.kind) { + case "appearsBy": + case "staysInFrame": + return { selectors: [assertion.selector], scopes: [] }; + case "before": + return { selectors: [assertion.a, assertion.b], scopes: [] }; + case "keepsMoving": + return { selectors: [], scopes: [assertion.withinSelector ?? "*"] }; + default: { + const exhaustive: never = assertion; + return exhaustive; + } + } +} + export function collectSamplingTargets(assertions: MotionAssertion[]): { selectors: string[]; livenessScopes: string[]; @@ -247,19 +273,9 @@ export function collectSamplingTargets(assertions: MotionAssertion[]): { const selectors = new Set(); const scopes = new Set(); for (const assertion of assertions) { - switch (assertion.kind) { - case "appearsBy": - case "staysInFrame": - selectors.add(assertion.selector); - break; - case "before": - selectors.add(assertion.a); - selectors.add(assertion.b); - break; - case "keepsMoving": - scopes.add(assertion.withinSelector ?? "*"); - break; - } + const targets = assertionTargets(assertion); + for (const selector of targets.selectors) selectors.add(selector); + for (const scope of targets.scopes) scopes.add(scope); } return { selectors: [...selectors], livenessScopes: [...scopes] }; }