From f191e32ec66752e3a95e7c94c6880ac51fb31889 Mon Sep 17 00:00:00 2001 From: Manuel Fittko Date: Sat, 11 Jul 2026 01:00:18 +0200 Subject: [PATCH 1/3] feat(loop): thread per-step viewport/interaction into named-state slugs Wire production callers to the Stage 4 slug discriminator. A drive flow step may now declare a viewport (resized before the step) and an interactionState; both are passed to captureNamedUiState so responsive/stateful renders land in distinct slugged directories instead of colliding. The deck-fit harness drops its "(mobile 390)" name hack and passes viewport: MOBILE, letting the slug carry the viewport. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_0177JAWNT67FY6d48qQJeDwo --- docs/ui-review-recipe-contract.md | 8 ++++ packages/core/src/config/config.mjs | 6 +++ packages/core/test/config.test.mjs | 17 +++++++ scripts/loop/ui-review-drive.mjs | 13 +++++ ...eck-fit-harness-viewport-contract.test.mjs | 23 +++++++++ test/docs/ui-review-recipe-doc.test.mjs | 2 +- test/loop/ui-review-drive.test.mjs | 48 +++++++++++++++++++ test/playwright/harness/deck-fit-harness.mjs | 8 +++- 8 files changed, 122 insertions(+), 3 deletions(-) create mode 100644 test/contracts/deck-fit-harness-viewport-contract.test.mjs diff --git a/docs/ui-review-recipe-contract.md b/docs/ui-review-recipe-contract.md index 727f7472..248a8312 100644 --- a/docs/ui-review-recipe-contract.md +++ b/docs/ui-review-recipe-contract.md @@ -178,6 +178,11 @@ allowlisted flow. The selection is capped and any overflow logged. - `uiReview.flows[].steps[].value` — required for `upload` (the file path); also the typed/selected value for `fill`/`select`. - `uiReview.flows[].steps[].event` — optional event name for `dispatch`. +- `uiReview.flows[].steps[].viewport` — optional `{ width, height }`; resizes the + page before the step and slugs the capture so a responsive render lands in its + own reviewable directory. +- `uiReview.flows[].steps[].interactionState` — optional `none`/`focus`/`hover`/`error`; + labels a stateful render the route names, slugged into its own directory. ```yaml uiReview: @@ -270,6 +275,9 @@ from the schema or if the schema gains a key not listed here. - `uiReview.flows[].steps[].path` - `uiReview.flows[].steps[].value` - `uiReview.flows[].steps[].event` +- `uiReview.flows[].steps[].viewport.width` +- `uiReview.flows[].steps[].viewport.height` +- `uiReview.flows[].steps[].interactionState` - `uiReview.caps.maxScreenshots` - `uiReview.caps.maxFlows` - `uiReview.caps.maxStepsPerFlow` diff --git a/packages/core/src/config/config.mjs b/packages/core/src/config/config.mjs index fdc5e113..448c10dd 100644 --- a/packages/core/src/config/config.mjs +++ b/packages/core/src/config/config.mjs @@ -352,6 +352,12 @@ const UiReviewFlowStepConfig = z.strictObject({ path: z.string().trim().min(1).optional(), value: z.string().optional(), event: z.string().trim().min(1).optional(), + // Responsive/stateful captures: a declared viewport resizes the page before the + // step and bakes into the named-state slug, so the mobile vs desktop (or + // default vs error) render lands in a distinct reviewable directory. The route + // NAMES its interaction states — the drive never enumerates them itself. + viewport: z.strictObject({ width: z.number().int().positive(), height: z.number().int().positive() }).optional(), + interactionState: z.enum(["none", "focus", "hover", "error"]).optional(), }).superRefine((step, ctx) => { // Every action but `goto` targets an element, so a missing selector is a // config error, not a runtime step-failure. (`goto` uses `path`/url.) diff --git a/packages/core/test/config.test.mjs b/packages/core/test/config.test.mjs index 903ce125..3fbfa149 100644 --- a/packages/core/test/config.test.mjs +++ b/packages/core/test/config.test.mjs @@ -463,6 +463,23 @@ describe("schema validation", () => { }).success); }); + test("S35d: a step may declare a viewport and an interactionState; both are validated", () => { + assert.ok(DevLoopConfigSchema.safeParse({ + version: 1, + uiReview: { flows: [{ name: "decks", steps: [{ action: "goto", path: "/", viewport: { width: 390, height: 844 }, interactionState: "error" }] }] }, + }).success); + // Non-positive viewport dimensions are rejected (a bad descriptor must fail closed). + assert.ok(!DevLoopConfigSchema.safeParse({ + version: 1, + uiReview: { flows: [{ name: "decks", steps: [{ action: "goto", path: "/", viewport: { width: 0, height: 844 } }] }] }, + }).success); + // Only the route-named interaction states are accepted. + assert.ok(!DevLoopConfigSchema.safeParse({ + version: 1, + uiReview: { flows: [{ name: "decks", steps: [{ action: "goto", path: "/", interactionState: "wiggle" }] }] }, + }).success); + }); + test("S36: uiReview.serverLogExceptionPattern rejects a malformed regex", () => { assert.ok(!DevLoopConfigSchema.safeParse({ version: 1, diff --git a/scripts/loop/ui-review-drive.mjs b/scripts/loop/ui-review-drive.mjs index fe85f340..17d579ea 100644 --- a/scripts/loop/ui-review-drive.mjs +++ b/scripts/loop/ui-review-drive.mjs @@ -282,8 +282,19 @@ async function dismissInterstitials({ page, interstitials, timeoutMs = 2000 }) { * Exported so the per-state attribution + consolePath threading has a regression * test against the REAL production wiring (not a substitute fake runStep). */ export function makeRunStep({ page, outputDir, sliceCapturedEvents }) { + // The Playwright page keeps its size across steps, so a declared viewport stays + // in effect for later undeclared steps too. Carry it forward so the slug always + // names the viewport the page is ACTUALLY at — never a stale `default`. Before + // any explicit resize the page is at the context default, which slugs as `default`. + let effectiveViewport; return async ({ appUrl, flow, step, index }) => { const sel = step.selector; + // Apply a declared viewport BEFORE the action so the render (and its capture) + // reflects it — otherwise a mobile-slugged directory would hold desktop pixels. + if (step.viewport) { + await page.setViewportSize(step.viewport); + effectiveViewport = step.viewport; + } switch (step.action) { case "goto": await page.goto(new URL(step.path ?? "/", appUrl).toString(), { waitUntil: "domcontentloaded" }); @@ -318,6 +329,8 @@ export function makeRunStep({ page, outputDir, sliceCapturedEvents }) { page, sliceId: flow.name, stateName, + viewport: effectiveViewport, + interactionState: step.interactionState, fullPage: false, outputDir, // Hand the already-sliced window to this state's console.json (same attribution diff --git a/test/contracts/deck-fit-harness-viewport-contract.test.mjs b/test/contracts/deck-fit-harness-viewport-contract.test.mjs new file mode 100644 index 00000000..8628471a --- /dev/null +++ b/test/contracts/deck-fit-harness-viewport-contract.test.mjs @@ -0,0 +1,23 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { readFile } from 'node:fs/promises'; + +// Source-level pin for the deck-fit CALLER change: the harness is exercised only +// by live-browser playwright specs (outside `npm run verify`), so this asserts +// the shape here. A revert re-introducing the `(mobile 390)` name-hack (or +// dropping `viewport: MOBILE`) would otherwise pass verify green. +test('deck-fit harness slugs the mobile viewport via `viewport:`, not a name-hacked stateName', async () => { + const source = await readFile(new URL('../../test/playwright/harness/deck-fit-harness.mjs', import.meta.url), 'utf8'); + + // Both mobile captures (deck + article) carry the viewport, so the slug — not + // the state name — distinguishes the responsive render. + const viewportCalls = source.match(/viewport:\s*MOBILE\b/g) ?? []; + assert.ok(viewportCalls.length >= 2, 'deck and article mobile captures must pass `viewport: MOBILE`'); + + // No stateName may embed a viewport in the name string (the dropped hack). + assert.doesNotMatch( + source, + /stateName:\s*`[^`]*\b(?:mobile|390)\b/i, + 'stateName must not encode a viewport — the slug carries it', + ); +}); diff --git a/test/docs/ui-review-recipe-doc.test.mjs b/test/docs/ui-review-recipe-doc.test.mjs index 97b32b61..01747c53 100644 --- a/test/docs/ui-review-recipe-doc.test.mjs +++ b/test/docs/ui-review-recipe-doc.test.mjs @@ -114,7 +114,7 @@ test("recipe doc config keys match the shipped uiReview/worktree schema", async // Anchor the count so a wrapped-then-dropped key (see below) can't pass by // matching a doc that dropped the same key. - assert.equal(schema.size, 31, "expected 31 uiReview/worktree schema leaves"); + assert.equal(schema.size, 34, "expected 34 uiReview/worktree schema leaves"); }); test("collectKeys descends through effects/preprocess wrappers at nested levels", () => { diff --git a/test/loop/ui-review-drive.test.mjs b/test/loop/ui-review-drive.test.mjs index b21d4840..902ae3c6 100644 --- a/test/loop/ui-review-drive.test.mjs +++ b/test/loop/ui-review-drive.test.mjs @@ -434,6 +434,54 @@ test("makeRunStep: threads consolePath into the runStep result and points state. assert.equal(state.artifacts.console.path, outcome.consolePath, "state.json back-references the same console.json"); }); +test("makeRunStep: a declared step viewport resizes the page and slugs the capture into a distinct directory (same state name, different viewport)", async () => { + const outputDir = tempDir(); + const resized = []; + const page = { + on: () => {}, + goto: async () => {}, + setViewportSize: async (v) => resized.push(v), + screenshot: async () => {}, + }; + const runStep = makeRunStep({ page, outputDir }); + + const desktop = await runStep({ appUrl: "http://app", flow: { name: "decks" }, step: { name: "Hero", action: "goto", path: "/" }, index: 0 }); + const mobile = await runStep({ appUrl: "http://app", flow: { name: "decks" }, step: { name: "Hero", action: "goto", path: "/", viewport: { width: 390, height: 844 } }, index: 1 }); + + // The declared viewport is applied to the page (honest pixels, not a mislabeled slug). + assert.deepEqual(resized, [{ width: 390, height: 844 }]); + // Same state name, different viewport → distinct on-disk directories (no collision). + assert.notEqual(desktop.statePath, mobile.statePath); + assert.match(path.dirname(desktop.statePath), /hero-default-none$/); + assert.match(path.dirname(mobile.statePath), /hero-w390h844-none$/); + const mobileState = JSON.parse(readFileSync(mobile.statePath, "utf8")); + assert.equal(mobileState.viewport, "w390h844"); +}); + +test("makeRunStep: a viewport set by an earlier step carries into a later undeclared step's slug (the page stays resized — no stale `default`)", async () => { + const outputDir = tempDir(); + const page = { on: () => {}, goto: async () => {}, setViewportSize: async () => {}, screenshot: async () => {} }; + const runStep = makeRunStep({ page, outputDir }); + + // Step 1 resizes to mobile; step 2 declares NO viewport but the page is STILL at 390. + await runStep({ appUrl: "http://app", flow: { name: "decks" }, step: { name: "Hero", action: "goto", path: "/", viewport: { width: 390, height: 844 } }, index: 0 }); + const later = await runStep({ appUrl: "http://app", flow: { name: "decks" }, step: { name: "Detail", action: "goto", path: "/d" }, index: 1 }); + + // The slug + state.json must name the viewport the page is ACTUALLY at, not `default`. + assert.match(path.dirname(later.statePath), /detail-w390h844-none$/); + assert.equal(JSON.parse(readFileSync(later.statePath, "utf8")).viewport, "w390h844"); +}); + +test("makeRunStep: a declared interactionState slugs the capture (route names it — the drive never enumerates)", async () => { + const outputDir = tempDir(); + const page = { on: () => {}, goto: async () => {}, click: async () => {}, screenshot: async () => {} }; + const runStep = makeRunStep({ page, outputDir }); + + const errored = await runStep({ appUrl: "http://app", flow: { name: "form" }, step: { name: "Email", action: "click", selector: "#submit", interactionState: "error" }, index: 0 }); + assert.match(path.dirname(errored.statePath), /email-default-error$/); + assert.equal(JSON.parse(readFileSync(errored.statePath, "utf8")).interactionState, "error"); +}); + test("makeRunStep: a capture failure still advances the attribution cursor — the next step's console.json does not inherit the prior step's events (no misattribution)", async () => { const outputDir = tempDir(); const handlers = {}; diff --git a/test/playwright/harness/deck-fit-harness.mjs b/test/playwright/harness/deck-fit-harness.mjs index 41e6372c..0c045fbc 100644 --- a/test/playwright/harness/deck-fit-harness.mjs +++ b/test/playwright/harness/deck-fit-harness.mjs @@ -208,7 +208,9 @@ export function defineDeckSuite({ sliceId, deckPath, sectionIds, mobileCapture } page, testInfo, sliceId, - stateName: `${mobileCapture.stateName} (mobile 390)`, + stateName: mobileCapture.stateName, + // The slug carries the viewport — no need to encode it in the state name. + viewport: MOBILE, fullPage: false, metadata: { fixture: deckName, @@ -364,7 +366,9 @@ export function defineArticleSuite({ sliceId, articlePath }) { page, testInfo, sliceId, - stateName: `${sliceId} (mobile 390)`, + stateName: sliceId, + // The slug carries the viewport — no need to encode it in the state name. + viewport: MOBILE, fullPage: false, metadata: { fixture: articleName, From 8ad5566901db7a29b45a1dd4fc4ce81ec3cf134a Mon Sep 17 00:00:00 2001 From: Manuel Fittko Date: Sat, 11 Jul 2026 02:12:28 +0200 Subject: [PATCH 2/3] test(contracts): make the deck-fit name-hack guard quote-agnostic + doc sticky viewport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The source-pin regex only matched backtick template literals, so a name-hack reintroduced via normal quotes or concatenation would evade it; anchor on the stateName: assignment and forbid a viewport word on that line in any string form. Also document that a step viewport is sticky — it persists to later steps and an omitted viewport inherits the last set size rather than resetting. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_0177JAWNT67FY6d48qQJeDwo --- docs/ui-review-recipe-contract.md | 6 +++++- .../deck-fit-harness-viewport-contract.test.mjs | 12 +++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/docs/ui-review-recipe-contract.md b/docs/ui-review-recipe-contract.md index 248a8312..9a491579 100644 --- a/docs/ui-review-recipe-contract.md +++ b/docs/ui-review-recipe-contract.md @@ -180,7 +180,11 @@ allowlisted flow. The selection is capped and any overflow logged. - `uiReview.flows[].steps[].event` — optional event name for `dispatch`. - `uiReview.flows[].steps[].viewport` — optional `{ width, height }`; resizes the page before the step and slugs the capture so a responsive render lands in its - own reviewable directory. + own reviewable directory. The viewport is **sticky**: it persists to later steps + until another step sets one, and an omitted `viewport` inherits the last set size + rather than resetting to default — so a later step's slug faithfully reflects the + size the page is actually at. Set `viewport` explicitly on the first step that + should return to the default dimensions. - `uiReview.flows[].steps[].interactionState` — optional `none`/`focus`/`hover`/`error`; labels a stateful render the route names, slugged into its own directory. diff --git a/test/contracts/deck-fit-harness-viewport-contract.test.mjs b/test/contracts/deck-fit-harness-viewport-contract.test.mjs index 8628471a..01faee29 100644 --- a/test/contracts/deck-fit-harness-viewport-contract.test.mjs +++ b/test/contracts/deck-fit-harness-viewport-contract.test.mjs @@ -14,10 +14,16 @@ test('deck-fit harness slugs the mobile viewport via `viewport:`, not a name-hac const viewportCalls = source.match(/viewport:\s*MOBILE\b/g) ?? []; assert.ok(viewportCalls.length >= 2, 'deck and article mobile captures must pass `viewport: MOBILE`'); - // No stateName may embed a viewport in the name string (the dropped hack). + // No capture may re-encode a viewport into the state NAME (the dropped hack was + // `(mobile 390)`). Anchor on the `stateName:` assignment and forbid a viewport + // word on that line, in ANY string form — backtick, quote, or concatenation all + // put `mobile`/`desktop`/`tablet` on the `stateName:` line, so a regression can't + // slip back under a different quote style. Variable refs like `mobileCapture` + // are unaffected (no word boundary after `mobile`), and the check is scoped to + // `stateName:` lines so unrelated comments/test-names don't trip it. assert.doesNotMatch( source, - /stateName:\s*`[^`]*\b(?:mobile|390)\b/i, - 'stateName must not encode a viewport — the slug carries it', + /stateName:[^\n]*\b(?:mobile|desktop|tablet)\b/i, + 'stateName must not encode a viewport label — the slug carries it', ); }); From 980a74eb8514ba7f896f577f5fcb3c9286fe3c8e Mon Sep 17 00:00:00 2001 From: Manuel Fittko Date: Sat, 11 Jul 2026 02:17:36 +0200 Subject: [PATCH 3/3] docs(ui-review): clarify there is no viewport reset sentinel Returning a later step to a different or the original size requires giving that step an explicit viewport with the exact dimensions; omitting it inherits the last set size. Removes the ambiguous 'return to the default dimensions' wording. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_0177JAWNT67FY6d48qQJeDwo --- docs/ui-review-recipe-contract.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/ui-review-recipe-contract.md b/docs/ui-review-recipe-contract.md index 9a491579..dcb5fcbc 100644 --- a/docs/ui-review-recipe-contract.md +++ b/docs/ui-review-recipe-contract.md @@ -183,8 +183,9 @@ allowlisted flow. The selection is capped and any overflow logged. own reviewable directory. The viewport is **sticky**: it persists to later steps until another step sets one, and an omitted `viewport` inherits the last set size rather than resetting to default — so a later step's slug faithfully reflects the - size the page is actually at. Set `viewport` explicitly on the first step that - should return to the default dimensions. + size the page is actually at. There is no reset sentinel: to render a later step + at a different size (or back at the original one), give that step an explicit + `viewport` with the exact dimensions you want. - `uiReview.flows[].steps[].interactionState` — optional `none`/`focus`/`hover`/`error`; labels a stateful render the route names, slugged into its own directory.