From 9660213afac4704f6d51039ebb64188e0fe90f97 Mon Sep 17 00:00:00 2001 From: Quinton Jason Date: Mon, 10 Aug 2026 08:38:05 -0500 Subject: [PATCH 1/6] docs(skills): add pine-visual-test-pr visual regression skill Self-iterating Playwright-MCP visual test harness for Pine PRs, modeled on kajabi-products' kp-visual-test-pr but re-pointed at Pine's Storybook. Builds Stencil, serves Storybook, maps changed pds-* components to their stories via /index.json, and captures screenshots across light/dark themes, viewports, and interactive states. Self-evaluates each shot against embedded rules (dark-mode/theme, focus-ring, console errors, token discipline), retries harness failures, records genuine visual/a11y regressions as findings, and posts a markdown report as a PR comment. Optional before/after baseline diff for modified components. Complements Chromatic (local, fast, pre-CI); does not replace it. --- .claude/skills/pine-visual-test-pr/SKILL.md | 417 ++++++++++++++++++++ 1 file changed, 417 insertions(+) create mode 100644 .claude/skills/pine-visual-test-pr/SKILL.md diff --git a/.claude/skills/pine-visual-test-pr/SKILL.md b/.claude/skills/pine-visual-test-pr/SKILL.md new file mode 100644 index 000000000..8cce4003a --- /dev/null +++ b/.claude/skills/pine-visual-test-pr/SKILL.md @@ -0,0 +1,417 @@ +--- +name: pine-visual-test-pr +description: Self-iterating visual test harness for Pine PRs. Builds Stencil + serves Storybook, maps changed pds-* components to their stories, captures Playwright screenshots across light/dark themes, viewports, and interactive states, self-evaluates each against the embedded rules, rejects and retries failures, and posts a markdown report as a PR comment. Optional before/after baseline diff for modified components. +argument-hint: "[PR number — auto-detects from current branch if omitted]" +--- + +# Visual Test PR (Pine) + +Automated visual testing for Pine design-system pull requests. Pine ships +leaf-level web components consumed by every Kajabi app, so a visual +regression here propagates everywhere — and the most common one is a +dark-mode / token break that never shows in a light-theme review. + +This skill builds Stencil, serves Storybook, discovers the stories for the +components the PR touched, captures Playwright screenshots across **theme +(light/dark)**, viewport, and interactive state, evaluates each against the +rules below, and loops until every shot passes or a structural blocker +requires human input. It complements — does not replace — Chromatic: this is +the fast, local, agent-evaluated pass you run before pushing, and before +Chromatic runs in CI. + +## When to Use + +- After changing any `libs/core/src/components/pds-*/**` file (`.tsx`, `.scss`/`.tokens.scss`, `.stories.*`) +- As a complement to `pine-design-review` (token/a11y code review) and `pine-run-gauntlet` +- Before requesting human review, to pre-validate rendered output in both themes +- To sanity-check a change locally without waiting for the Chromatic CI job + +**Not the right tool when:** the PR only touches docs (`*.mdx`), build config, or non-component TypeScript with no rendered surface — there is nothing to screenshot. Say so and stop. + +## Prerequisites + +- Playwright MCP configured in this Claude Code session +- Node deps installed at the Pine repo root (`node_modules/` present) +- `gh` CLI authenticated +- Able to run `libs/core` scripts (`build.stencil`, `start.storybook`) + +## Progress Tracking + +At the start of Phase 0, create tasks for all phases: + +``` +TaskCreate: "Phase 0: Environment preflight" +TaskCreate: "Phase 1: PR resolution + component scope" +TaskCreate: "Phase 2: Storybook up + story index" +TaskCreate: "Phase 3: Capture plan" +TaskCreate: "Phase 4: Baseline decision" +TaskCreate: "Phase 5: Screenshot capture + evaluation loop" +TaskCreate: "Phase 6: Report" +``` + +Mark each `in_progress` when started, `completed` when done. + +### Pivoting Away (Early Exit) + +If the skill is abandoned mid-run (user redirects, structural blocker, or the PR has no visual surface), clean up the task list before stopping: + +1. Mark the current `in_progress` task `completed` (or `deleted` if no meaningful work was done) +2. Mark all remaining `pending` tasks `deleted` + +Do this immediately when a pivot is decided — before switching to any other work — so stale `in_progress` tasks don't persist across sessions. + +--- + +## Phase 0 — Environment Preflight + +Run all checks from the Pine repo root before touching anything. If **any** check fails, print the exact fix command and stop — do not continue to Phase 1. + +```bash +# 1. gh CLI authenticated +gh auth status + +# 2. Repo root has deps installed +ls node_modules >/dev/null && echo "root deps OK" + +# 3. libs/core scripts present +node -e "const s=require('./libs/core/package.json').scripts; ['build.stencil','start.storybook'].forEach(k=>{if(!s[k]){console.error('missing script '+k);process.exit(1)}}); console.log('core scripts OK')" + +# 4. Node version matches +cat .nvmrc +``` + +Also verify Playwright MCP is reachable by calling `mcp__playwright__browser_snapshot` — if it throws a tool-not-found or connection error, the MCP is down. + +| Check | Failure Symptom | Fix | +|---|---|---| +| `gh auth status` | "not logged in" | `gh auth login` | +| `node_modules` | "No such file or directory" | `npm install` at repo root | +| core scripts | "missing script" | Confirm you're in the Pine repo; scripts live in `libs/core/package.json` | +| Playwright MCP | Tool error / connection refused | Re-open Claude Code — MCP connection dropped | + +**Do not proceed until all checks pass.** + +--- + +## Phase 1 — PR Resolution + Component Scope + +1. **If PR number argument provided:** use it. +2. **Otherwise:** auto-detect from current branch: + ```bash + gh pr view --json number,url,title,headRefName,baseRefName,body + ``` +3. **Get changed files:** + ```bash + gh pr diff --name-only + ``` +4. **Determine component scope.** Reduce the diff to the set of changed components: + ```bash + gh pr diff --name-only | grep -oE 'libs/core/src/components/[^/]+' | sort -u + ``` + Each entry is a component directory (e.g. `pds-button`, `pds-alert`). This is the scope — the skill only screenshots stories for these components (plus their child components, e.g. `pds-tabs` → `pds-tab`, `pds-tabpanel`). +5. **Classify each changed file** to predict the visual risk (drives the capture plan): + | Changed file | Visual risk | Emphasis | + |---|---|---| + | `*.tokens.scss` / `*.scss` | color, spacing, dark-mode | **both themes**, all variants | + | `*.tsx` | markup/DOM, slots, states | interactive states, variant coverage | + | `*.stories.*` | the story itself changed | capture the new/changed stories | + | shared token/global SCSS | broad | widen scope — may affect many components | + +**If no PR found:** stop and tell the user to create one first. +**If the diff touches no `libs/core/src/components/**` files** and no shared token/global styles: there is no component visual surface — say so and stop (this PR is out of scope for the skill). + +--- + +## Phase 2 — Storybook Up + Story Index + +Pine's visual surface is Storybook. Stories render compiled components from `dist/`, so Stencil must be built **before** Storybook starts. **Never guess story IDs — always read them from `/index.json`.** + +1. **Build Stencil** (produces/refreshes `libs/core/dist/`): + ```bash + ( cd libs/core && npm run build.stencil ) + ``` +2. **Start Storybook** on port 6006 as a background process (it stays up for the whole run): + ```bash + ( cd libs/core && npm run start.storybook ) # run_in_background: true + ``` +3. **Health check** — poll until the story index is served (up to ~60s; Storybook's first Vite optimize pass is slow): + ```bash + curl -s -o /dev/null -w "%{http_code}" http://localhost:6006/index.json + ``` + Retry until `200`. If it never comes up, read the background process output for the failure (common: port 6006 already taken → the existing server is fine to reuse; or a Stencil build error → fix and rebuild). +4. **Fetch the story index** and keep it — this is the authoritative story list (the Pine analog of `worktree-server list`): + ```bash + curl -s http://localhost:6006/index.json > /tmp/pine-stories.json + # entries is a map keyed by story id; keep only real stories (not docs) + node -e "const e=require('/tmp/pine-stories.json').entries; Object.values(e).filter(x=>x.type==='story').forEach(x=>console.log(x.id+'\t'+x.title+'\t'+x.name))" + ``` + +Store `http://localhost:6006` as `$SB` for all subsequent phases. + +### Resolving changed components → story IDs + +Story IDs are `slug(title)` + `--` + `slug(exportName)` (e.g. title `components/Button`, story `Primary` → `components-button--primary`). The directory name (`pds-button`) is **not** the title — so map through the story file, don't assume: + +1. For each changed component dir, read its story file's `title:` field: + ```bash + grep -h "title:" libs/core/src/components//**/stories/*.stories.* + ``` +2. Match that title against the `title` column of the index dump from step 4. Every matching entry's `id` is a story to capture. + +--- + +## Phase 3 — Capture Plan + +Build the explicit shot list **before** opening a browser, and output it so the user can see coverage. + +### Axes + +For every in-scope story, the capture matrix is: + +- **Theme (required, both):** `light` and `dark`. This is Pine's highest-value axis — dark-mode token regressions are the most common Pine visual bug. Never skip dark. +- **Viewport:** `desktop` (1280×800) for every story. Add `mobile` (375×812) only for components with responsive layout behavior (e.g. `pds-nav`, `pds-modal`, `pds-tabs` overflow, `pds-filters`, `pds-table`). Document which components you flagged responsive and why. +- **Interactive states** (desktop only, only for the states the component actually has, and only when the changed files could affect them): + - `hover`, `focus` (keyboard focus ring), `active`/`pressed` + - `disabled` (usually already a dedicated story — prefer the story over synthesizing) + - `error`/`invalid` for form components (`pds-input`, `pds-select`, `pds-textarea`, `pds-radio`, `pds-checkbox`, `pds-combobox`, `pds-multiselect`) + - `open`/expanded for overlay components (`pds-modal`, `pds-dropdown-menu`, `pds-tooltip`, `pds-accordion`, `pds-combobox`) +- **Direction (optional):** `rtl` via `globals=direction:rtl`, only if the PR touches logical-property / layout SCSS or the component is directional. + +### Variant coverage + +Pine encodes variants as separate stories (e.g. Button → Primary, Secondary, Destructive, IconOnly, Loading, Sizes…). Capture **every** story for a changed component rather than one representative — variant-specific token regressions are exactly what this catches. If a component has many stories and the change is narrowly scoped (single `.tsx` branch), you may capture the directly-affected variants plus the default; state the reduction explicitly in the plan. + +### Story render URL + +Capture from the chrome-free iframe endpoint (no Storybook toolbar/sidebar in frame): + +``` +$SB/iframe.html?id=&viewMode=story&globals=theme:dark +$SB/iframe.html?id=&viewMode=story&globals=theme:light +# with direction: &globals=theme:dark;direction:rtl (semicolon-separated) +``` + +### Naming convention + +``` +______[__].png +# e.g. button__primary__desktop__dark.png +# button__primary__desktop__light__hover.png +# modal__default__mobile__dark__open.png +``` + +**Output the full capture plan as a checklist.** Do not proceed without a complete plan. + +--- + +## Phase 4 — Baseline Decision (before/after) + +Decide, per component, whether to also capture a **baseline** (the component as it renders on the PR's base branch) for a before/after comparison. + +- **Modified existing component** (component existed on base) → baseline recommended. A screenshot alone can't tell an intended restyle from a regression; the before/after can. +- **Brand-new component / new story** → no baseline (nothing to compare); validate against the rules only. +- **Token-only or SCSS-only diffs** → baseline strongly recommended (this is the highest-risk, easiest-to-miss category). + +If capturing baselines, note that it requires rebuilding Stencil on the base branch — do it in one batch after the PR-branch capture (Phase 5b), not interleaved. If the working tree is dirty or the user is mid-task, ask before switching branches. + +Skipping baselines is always acceptable — say so in the report and rely on the rules-based evaluation. Chromatic remains the authoritative pixel-diff in CI; this baseline mode is a local convenience. + +--- + +## Phase 5 — Screenshot Capture + Evaluation Loop + +Core loop. Run until every item in the capture plan has a passing screenshot or is classified as a structural blocker. + +### For each item (storyId × theme × viewport × state): + +**Step 1 — Navigate** to the chrome-free iframe: +``` +mcp__playwright__browser_navigate({ url: "$SB/iframe.html?id=&viewMode=story&globals=theme:" }) +``` + +**Step 2 — Set viewport, then wait for render:** +``` +mcp__playwright__browser_resize({ width: 1280, height: 800 }) # or 375×812 for mobile +mcp__playwright__browser_wait_for({ selector: "" }) # the custom element tag +mcp__playwright__browser_wait_for({ timeout: 500 }) # let fonts/tokens settle +``` +Confirm the custom element actually upgraded (has a shadow root / rendered content) — a bare unstyled tag means `dist/` wasn't loaded (see Rule 2 retry). + +**Step 3 — Capture default, both themes** (repeat Steps 1–2 for `theme:light` and `theme:dark`): +``` +mcp__playwright__browser_take_screenshot({ filename: "____desktop__.png" }) +``` + +**Step 4 — Capture interactive states** (desktop only, per the plan): +``` +# hover +mcp__playwright__browser_hover({ selector: "" }) +mcp__playwright__browser_take_screenshot({ filename: "____desktop____hover.png" }) + +# keyboard focus (focus ring is a frequent dark-mode regression) +mcp__playwright__browser_press_key({ key: "Tab" }) +mcp__playwright__browser_take_screenshot({ filename: "____desktop____focus.png" }) + +# open/expanded overlays +mcp__playwright__browser_click({ selector: "" }) +mcp__playwright__browser_take_screenshot({ filename: "____desktop____open.png" }) +``` + +**Step 5 — Console check.** Read console messages for the story: +``` +mcp__playwright__browser_console_messages() +``` +Any `error`-level message (Stencil hydration error, missing token, thrown listener) fails Rule 4. + +**Step 6 — Evaluate every screenshot against the rules below.** + +### Visual Test Rules (Pine) + +| # | Rule | Check | +|---|---|---| +| 1 | Component renders | Not blank; the custom element upgraded and shows its expected content/shape | +| 2 | Assets loaded | Styles applied (not raw unstyled DOM), icons/fonts present — a flash-of-unstyled tag means `dist/` didn't load | +| 3 | Both themes correct | Light **and** dark both render; no invisible text, no unthemed white box on dark, contrast holds | +| 4 | No console errors | No `error`-level console output for the story | +| 5 | Focus visible | Keyboard focus produces a visible focus ring in both themes (Pine a11y requirement) | +| 6 | Correct viewport | Frame matches 1280×800 (or 375×812 for flagged responsive components) | +| 7 | No clipping | Overlays (modal/tooltip/dropdown) fully visible, not cut by the iframe; no truncated content | +| 8 | Token discipline | No obviously hard-coded/off-palette color vs the rest of the set (pairs with `pine-design-review`) | +| 9 | Naming convention | `______[__].png` | +| 10 | Variant coverage | Every in-scope story captured (or documented reduction) | + +**If all applicable rules pass:** add to the approved set; move on. + +**If a rule fails — retry by rule:** + +| Rule failed | Retry action | +|---|---| +| 1/2 — blank or unstyled | Verify `libs/core/dist/` exists and is fresh; rebuild `npm run build.stencil`; hard-reload the iframe; re-capture | +| 3 — theme broken | Re-check the `globals=theme:` value in the URL; confirm `data-theme` on ``; if genuinely broken in one theme, that's a **real finding** — record it, don't retry away | +| 4 — console error | Capture the message text into the report; if it's a real runtime error it's a finding, not a flake | +| 5 — no focus ring | Ensure you Tabbed into the element (not just navigated); re-capture; if still absent it's a real a11y finding | +| 6 — wrong viewport | Resize to the correct dimensions, re-capture | +| 7 — clipped overlay | Grow the viewport height or scroll the overlay into view, re-capture | +| 9 — naming | Rename the file — no re-capture needed | + +**Max 3 retries per item.** After 3 failures on the same item, classify it as a **structural blocker**, record the reason, and continue. Never stop the loop. Distinguish a *harness* blocker (couldn't capture) from a *real finding* (captured fine, but the render is wrong) — real findings are the point of the skill and belong in the report as issues, not blockers. + +### Phase 5b — Baseline capture (if elected in Phase 4) + +After all PR-branch shots are approved: + +1. Stash/confirm a clean tree; record current branch. +2. `git checkout ` → `( cd libs/core && npm run build.stencil )` → reuse the running Storybook (or restart it). +3. Re-capture the **same** story×theme×viewport matrix into `*__baseline.png` filenames. +4. `git checkout `, rebuild Stencil to restore the PR state. +5. For each pair, compare before/after and note: **intended** (matches the PR's stated change) vs **regression** (unexpected delta, esp. in a theme/variant the PR didn't claim to touch). + +### Loop termination + +- **Continue** while unchecked plan items remain. +- **Exit** when every item is approved, a real finding, or a structural blocker. +- **Never** loop indefinitely — track attempts per item. + +--- + +## Phase 6 — Report + +Generate a markdown report and post it as a PR comment. + +```bash +mkdir -p .claude/visual-test-reports +``` + +### Report template + +```markdown +# Visual Test Report — Pine PR # + +**PR:** [](<url>) +**Branch:** <headRef> → <baseRef> +**Storybook:** http://localhost:6006 (local) +**Tested:** <date> +**Result:** PASS (<N>/<N>) | PARTIAL (<N>/<M> — see findings/blockers) | BLOCKED + +## Components in scope +- <component> — <n stories> — <light+dark | +mobile | +rtl> + +## Approved Screenshots + +### <Component> — <Story> +**Light** [<file>__light.png] **Dark** [<file>__dark.png] +**States** [<file>__hover.png] [<file>__focus.png] [<file>__open.png] +<!-- if baseline captured --> +**Before/After (dark):** [<file>__dark__baseline.png] → [<file>__dark.png] — <intended|regression> + +## Findings (real visual/a11y issues) +### <Component> — <Story> — <theme/state> +- **Rule:** <e.g. 3 both-themes / 5 focus-visible / 4 console-error> +- **Observed:** <what's wrong> +- **Screenshot:** [<file>] + +## Structural Blockers +### <Item> +- **Rule:** <n> — **Attempts:** 3 — **Observed:** <…> — **Needs:** <human action> + +## Rules Applied +- [x] Component renders - [x] Assets loaded - [x] Both themes - [x] No console errors +- [x] Focus visible - [x] Viewport - [x] No clipping - [x] Token discipline +- [x] Naming - [x] Variant coverage +``` + +Post it: +```bash +gh pr comment <number> --body "$(cat .claude/visual-test-reports/pr-<number>-visual-test.md)" +``` + +--- + +## Completion + +```markdown +## Visual Test Complete: Pine PR #<number> + +### Result +- Approved: N screenshots across <k> components (light + dark) +- Real findings: F (see PR comment) +- Structural blockers: M + +### PR Comment +<url> +``` + +If there are real findings, tell the user which components/themes regressed before they request review. If clean, note it's pre-validated locally and Chromatic in CI is the authoritative pixel-diff gate. + +--- + +## Error Handling + +| Phase | Failure | Recovery | +|---|---|---| +| 0 | Playwright MCP unreachable | Print fix, stop — no browser ops | +| 0 | `gh` not authenticated | `gh auth login`, stop | +| 0 | root deps missing | `npm install` at root, stop | +| 2 | Stencil build fails | Surface the Stencil error, fix/rebuild — Storybook renders nothing without `dist/` | +| 2 | Storybook never serves `/index.json` | Read background output; port 6006 taken → reuse; else stop with the error | +| 2 | Title→id mapping empty | Component may have no stories — note it; nothing to capture for that component | +| 5 | Blank/unstyled render | Rebuild Stencil, hard-reload; 3x → structural blocker | +| 5b | Dirty tree at branch switch | Ask before `git checkout`; never discard the user's work | + +## Anti-Patterns + +- Do NOT guess story IDs — always read them from `/index.json`. +- Do NOT skip dark theme — it's the single highest-value axis for Pine. +- Do NOT `start.storybook` without a fresh `build.stencil` — you'll screenshot stale/absent components. +- Do NOT retry away a genuine theme/focus/console problem — that's a **finding**, the whole point; record it. +- Do NOT loop indefinitely — 3 retries per item, then blocker. +- Do NOT include blank/unstyled or error screenshots in the approved set. +- Do NOT switch branches for a baseline without confirming a clean tree. +- Do NOT treat this as a replacement for Chromatic — it's the fast local pass that runs before it. + +## Related Skills + +- `pine-design-review` — token / SCSS / a11y / Figma code review (pair with this; it reads code, this reads pixels) +- `pine-run-gauntlet` — parallel multi-reviewer pass before a PR (code/security/design/existence) +- `pine-existence-review` — checks whether a component/API already exists before adding new +- Chromatic (`@chromatic-com/storybook`, in CI) — authoritative cloud pixel-diff; this skill is the local complement From 1e58d3595dd6dce5495ea50ba617a087192119e3 Mon Sep 17 00:00:00 2001 From: Quinton Jason <quinton.jason@gmail.com> Date: Mon, 10 Aug 2026 08:46:52 -0500 Subject: [PATCH 2/6] docs(skills): harden pine-visual-test-pr from dry run against #785 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dry run against the pds-tabs PR surfaced three fixes: - Rule 4: whitelist harness noise (favicon/static 404s, Vite HMR); only component-origin errors (pine-core/*.js stack) count as findings. - Screenshots: require an absolute output path into a per-run dir — the Playwright MCP browser cwd differs from the shell cwd, so relative filenames land outside the repo. - Story mapping: match story titles by exact/nested-path equality, not substring (includes('tab') wrongly matched Table and Sortable). --- .claude/skills/pine-visual-test-pr/SKILL.md | 31 +++++++++++++++++---- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/.claude/skills/pine-visual-test-pr/SKILL.md b/.claude/skills/pine-visual-test-pr/SKILL.md index 8cce4003a..46865b961 100644 --- a/.claude/skills/pine-visual-test-pr/SKILL.md +++ b/.claude/skills/pine-visual-test-pr/SKILL.md @@ -156,7 +156,14 @@ Story IDs are `slug(title)` + `--` + `slug(exportName)` (e.g. title `components/ ```bash grep -h "title:" libs/core/src/components/<component>/**/stories/*.stories.* ``` -2. Match that title against the `title` column of the index dump from step 4. Every matching entry's `id` is a story to capture. +2. Match that title against the index by **exact equality** on the title (or an exact nested-path prefix like `components/Tabs` → also `components/Tabs/Tab`, `components/Tabs/Tabpanel`). Do **not** use a loose substring match — `includes('tab')` wrongly pulls in `components/Table` and `components/Sortable`. Every entry whose title equals (or is nested under) a scoped title is a story to capture. + +```bash +# exact + nested-path match for a set of scoped titles +node -e "const e=require('/tmp/pine-stories.json').entries; const t=process.argv.slice(1); +Object.values(e).filter(x=>x.type==='story' && t.some(p=>x.title===p||x.title.startsWith(p+'/'))) + .forEach(x=>console.log(x.id))" 'components/Tabs' +``` --- @@ -191,6 +198,20 @@ $SB/iframe.html?id=<storyId>&viewMode=story&globals=theme:light # with direction: &globals=theme:dark;direction:rtl (semicolon-separated) ``` +### Output location (absolute path — important) + +The Playwright MCP browser runs in its **own** working directory, which is often **not** your shell's cwd (in practice it may be a parent folder). A relative `filename` silently lands the PNG outside the repo. Always pass an **absolute** path into a per-run directory, and create it first: + +```bash +mkdir -p "$(pwd)/.claude/visual-test-reports/pr-<number>" +``` +``` +mcp__playwright__browser_take_screenshot({ + filename: "<repo-abs-path>/.claude/visual-test-reports/pr-<number>/<name>.png" +}) +``` +After the first capture, confirm the file exists at that absolute path before continuing — if it's missing, the MCP wrote it relative to its own cwd; fix the path, don't keep shooting into the void. + ### Naming convention ``` @@ -257,11 +278,11 @@ mcp__playwright__browser_click({ selector: "<trigger>" }) mcp__playwright__browser_take_screenshot({ filename: "<component>__<story>__desktop__<theme>__open.png" }) ``` -**Step 5 — Console check.** Read console messages for the story: +**Step 5 — Console check.** Read `error`-level console messages for the story: ``` -mcp__playwright__browser_console_messages() +mcp__playwright__browser_console_messages({ level: "error", all: true }) ``` -Any `error`-level message (Stencil hydration error, missing token, thrown listener) fails Rule 4. +**Filter harness noise before judging** — a `favicon.ico` (or other static-asset) 404 and Vite HMR chatter are expected and are **not** findings. A message fails Rule 4 only when it originates from the component bundle (stack/URL includes `pine-core/*.js`) or the story itself — e.g. a Stencil render/hydration error, a missing-token throw, or a thrown event listener. Those are real findings: capture the full message text into the report. **Step 6 — Evaluate every screenshot against the rules below.** @@ -272,7 +293,7 @@ Any `error`-level message (Stencil hydration error, missing token, thrown listen | 1 | Component renders | Not blank; the custom element upgraded and shows its expected content/shape | | 2 | Assets loaded | Styles applied (not raw unstyled DOM), icons/fonts present — a flash-of-unstyled tag means `dist/` didn't load | | 3 | Both themes correct | Light **and** dark both render; no invisible text, no unthemed white box on dark, contrast holds | -| 4 | No console errors | No `error`-level console output for the story | +| 4 | No console errors | No **component-origin** `error`-level output (stack/URL points at `pine-core/*.js` or the story). Ignore harness noise: `favicon.ico` and other static-asset 404s, and Vite HMR chatter — these are not findings | | 5 | Focus visible | Keyboard focus produces a visible focus ring in both themes (Pine a11y requirement) | | 6 | Correct viewport | Frame matches 1280×800 (or 375×812 for flagged responsive components) | | 7 | No clipping | Overlays (modal/tooltip/dropdown) fully visible, not cut by the iframe; no truncated content | From 8cfc6c72b0a28e544d94ce2af429256b1c2513d2 Mon Sep 17 00:00:00 2001 From: Quinton Jason <quinton.jason@gmail.com> Date: Mon, 10 Aug 2026 09:01:16 -0500 Subject: [PATCH 3/6] docs(skills): fold baseline-mode lessons into pine-visual-test-pr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running baseline mode against #785 surfaced two more fixes: - Console read must be per-navigation, not all:true — session history floods with stale ERR_CONNECTION_REFUSED / NoStoryMatchError after any Storybook restart, none of which are findings. - Phase 5b: build.stencil dirties tracked generated files (components.d.ts, react createComponent.tsx), aborting the branch switch; reset them before each checkout, restart Storybook on the base (stale index → NoStoryMatchError), and compare only stories shared with base. --- .claude/skills/pine-visual-test-pr/SKILL.md | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/.claude/skills/pine-visual-test-pr/SKILL.md b/.claude/skills/pine-visual-test-pr/SKILL.md index 46865b961..229e23d98 100644 --- a/.claude/skills/pine-visual-test-pr/SKILL.md +++ b/.claude/skills/pine-visual-test-pr/SKILL.md @@ -278,11 +278,11 @@ mcp__playwright__browser_click({ selector: "<trigger>" }) mcp__playwright__browser_take_screenshot({ filename: "<component>__<story>__desktop__<theme>__open.png" }) ``` -**Step 5 — Console check.** Read `error`-level console messages for the story: +**Step 5 — Console check.** Read `error`-level console messages **for this navigation only** — do not pass `all: true`: ``` -mcp__playwright__browser_console_messages({ level: "error", all: true }) +mcp__playwright__browser_console_messages({ level: "error" }) ``` -**Filter harness noise before judging** — a `favicon.ico` (or other static-asset) 404 and Vite HMR chatter are expected and are **not** findings. A message fails Rule 4 only when it originates from the component bundle (stack/URL includes `pine-core/*.js`) or the story itself — e.g. a Stencil render/hydration error, a missing-token throw, or a thrown event listener. Those are real findings: capture the full message text into the report. +`all: true` returns the whole session's history, which after any Storybook restart floods with stale `ERR_CONNECTION_REFUSED` (the dead server) and `NoStoryMatchError` (a tab pointed at a story the new index lacks) — none of which are findings. Reading per-navigation gives the clean, current-story truth (and it matches the per-page `Console: N errors` counter the navigate call reports). **Filter harness noise before judging** — a `favicon.ico` (or other static-asset) 404 and Vite HMR chatter are expected and are **not** findings. A message fails Rule 4 only when it originates from the component bundle (stack/URL includes `pine-core/*.js`) or the story itself — e.g. a Stencil render/hydration error, a missing-token throw, or a thrown event listener. Those are real findings: capture the full message text into the report. **Step 6 — Evaluate every screenshot against the rules below.** @@ -321,11 +321,12 @@ mcp__playwright__browser_console_messages({ level: "error", all: true }) After all PR-branch shots are approved: -1. Stash/confirm a clean tree; record current branch. -2. `git checkout <baseRef>` → `( cd libs/core && npm run build.stencil )` → reuse the running Storybook (or restart it). -3. Re-capture the **same** story×theme×viewport matrix into `*__baseline.png` filenames. -4. `git checkout <headRef>`, rebuild Stencil to restore the PR state. -5. For each pair, compare before/after and note: **intended** (matches the PR's stated change) vs **regression** (unexpected delta, esp. in a theme/variant the PR didn't claim to touch). +1. Confirm a clean tree; record the current branch. +2. **`build.stencil` dirties tracked generated files** — `libs/core/src/components.d.ts` and `libs/react/src/components/react-component-lib/createComponent.tsx` are checked in but regenerated on every build, so a branch switch **aborts** ("would be overwritten") after you've built. Reset them before each `git checkout`: `git checkout -- libs/core/src/components.d.ts libs/react/src/components/react-component-lib/createComponent.tsx`. (These are generated, not authored — safe to discard. If a genuine unrelated WIP file is also dirty, `git stash` it and restore after.) +3. `git checkout <baseRef>` → `( cd libs/core && npm run build.stencil )` → **restart** Storybook (don't reuse the head-branch server — its story index is stale and every open tab will throw `NoStoryMatchError` against the new bundle). +4. Re-capture the **same** story×theme×viewport matrix into `*__baseline.png` filenames. Note that **stories new in the PR won't exist on base** — compare only shared stories (e.g. `--default`); a new story with no baseline is validated against the rules alone. +5. Reset generated files again (step 2), `git checkout <headRef>`, rebuild Stencil to restore the PR state. +6. For each pair, compare before/after and note: **intended** (matches the PR's stated change) vs **regression** (unexpected delta, esp. in a theme/variant the PR didn't claim to touch). A console error present on a new story but absent from the shared `--default` on both branches is **PR-introduced and scoped to that story** — the highest-signal verdict this mode produces. ### Loop termination From e83a33dd5128fe01b594ed85cb939987bef40bf6 Mon Sep 17 00:00:00 2001 From: Quinton Jason <quinton.jason@gmail.com> Date: Mon, 10 Aug 2026 14:17:01 -0500 Subject: [PATCH 4/6] docs(skills): address PR #788 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves Cursor + pixelflips findings on pine-visual-test-pr: - Chromatic hedged everywhere — the CI job isn't merged (only the local Storybook panel is); this skill is currently Pine's only pixel-level check. - Phase 1 threads an explicit <number> through gh pr diff/view (bare calls resolve against the current branch, mis-scoping an explicitly-passed PR). - Dropped the half-wired RTL axis (planned but never looped/graded). - All Phase 5/6 capture commands use the absolute <REPORT_DIR> path (the L201-213 rule was only in prose, not the example commands). - Console check is per-navigation AND per-theme (a single end-of-story read missed the earlier theme); never all:true. - Phase 5b: createComponent.tsx carries an authored double-registration guard that build.stencil strips — restore via git checkout -- after the final rebuild; added an explicit Storybook stop (lsof kill) before restart; Phase 2 'reuse the server' now carves out the post-branch-switch case. - Phase 6: write the report file before cat-ing it; post via --body-file with --repo (not double-quoted $(cat)); added a once-per-PR dedup guard and the 'Posted by pine-visual-test-pr' marker. --- .claude/skills/pine-visual-test-pr/SKILL.md | 125 +++++++++++++------- 1 file changed, 79 insertions(+), 46 deletions(-) diff --git a/.claude/skills/pine-visual-test-pr/SKILL.md b/.claude/skills/pine-visual-test-pr/SKILL.md index 229e23d98..d51af944b 100644 --- a/.claude/skills/pine-visual-test-pr/SKILL.md +++ b/.claude/skills/pine-visual-test-pr/SKILL.md @@ -15,16 +15,19 @@ This skill builds Stencil, serves Storybook, discovers the stories for the components the PR touched, captures Playwright screenshots across **theme (light/dark)**, viewport, and interactive state, evaluates each against the rules below, and loops until every shot passes or a structural blocker -requires human input. It complements — does not replace — Chromatic: this is -the fast, local, agent-evaluated pass you run before pushing, and before -Chromatic runs in CI. +requires human input. As of today this is the **only** pixel-level regression +check Pine has: `@chromatic-com/storybook` is installed as a local Storybook +panel, but the Chromatic **CI** job is not merged (it lives on an open PR). So +a clean result here is currently the whole pixel-level signal — weigh it +accordingly. Once Chromatic CI lands, this stays useful as the fast, local, +agent-evaluated pass you run before pushing and before that job. ## When to Use - After changing any `libs/core/src/components/pds-*/**` file (`.tsx`, `.scss`/`.tokens.scss`, `.stories.*`) - As a complement to `pine-design-review` (token/a11y code review) and `pine-run-gauntlet` - Before requesting human review, to pre-validate rendered output in both themes -- To sanity-check a change locally without waiting for the Chromatic CI job +- To sanity-check a change locally before pushing (and, once the Chromatic CI job lands, without waiting for it) **Not the right tool when:** the PR only touches docs (`*.mdx`), build config, or non-component TypeScript with no rendered surface — there is nothing to screenshot. Say so and stop. @@ -95,21 +98,22 @@ Also verify Playwright MCP is reachable by calling `mcp__playwright__browser_sna ## Phase 1 — PR Resolution + Component Scope -1. **If PR number argument provided:** use it. -2. **Otherwise:** auto-detect from current branch: +1. **Resolve the PR number into `<number>` and thread it through every `gh` call below.** A bare `gh pr diff` / `gh pr view` resolves against the *current branch's* PR — so if the skill is invoked with an explicit number while checked out on a different branch (exactly what the `argument-hint` promotes), an un-numbered call silently scopes the wrong PR. Always pass `<number>` explicitly. + - **If a PR number argument was provided:** use it as `<number>`. + - **Otherwise:** auto-detect from the current branch and read the number back out: + ```bash + gh pr view --json number,url,title,headRefName,baseRefName,body # <number> = .number + ``` +2. **Get changed files** (always numbered): ```bash - gh pr view --json number,url,title,headRefName,baseRefName,body + gh pr diff <number> --name-only ``` -3. **Get changed files:** +3. **Determine component scope.** Reduce the diff to the set of changed components: ```bash - gh pr diff --name-only - ``` -4. **Determine component scope.** Reduce the diff to the set of changed components: - ```bash - gh pr diff --name-only | grep -oE 'libs/core/src/components/[^/]+' | sort -u + gh pr diff <number> --name-only | grep -oE 'libs/core/src/components/[^/]+' | sort -u ``` Each entry is a component directory (e.g. `pds-button`, `pds-alert`). This is the scope — the skill only screenshots stories for these components (plus their child components, e.g. `pds-tabs` → `pds-tab`, `pds-tabpanel`). -5. **Classify each changed file** to predict the visual risk (drives the capture plan): +4. **Classify each changed file** to predict the visual risk (drives the capture plan): | Changed file | Visual risk | Emphasis | |---|---|---| | `*.tokens.scss` / `*.scss` | color, spacing, dark-mode | **both themes**, all variants | @@ -138,7 +142,7 @@ Pine's visual surface is Storybook. Stories render compiled components from `dis ```bash curl -s -o /dev/null -w "%{http_code}" http://localhost:6006/index.json ``` - Retry until `200`. If it never comes up, read the background process output for the failure (common: port 6006 already taken → the existing server is fine to reuse; or a Stencil build error → fix and rebuild). + Retry until `200`. If it never comes up, read the background process output for the failure (common: port 6006 already taken → a server **for this same branch** is fine to reuse, but one left over from another branch is **not** — stop it with `lsof -ti:6006 | xargs kill` and start fresh, or you'll screenshot the wrong bundle; or a Stencil build error → fix and rebuild). 4. **Fetch the story index** and keep it — this is the authoritative story list (the Pine analog of `worktree-server list`): ```bash curl -s http://localhost:6006/index.json > /tmp/pine-stories.json @@ -182,7 +186,8 @@ For every in-scope story, the capture matrix is: - `disabled` (usually already a dedicated story — prefer the story over synthesizing) - `error`/`invalid` for form components (`pds-input`, `pds-select`, `pds-textarea`, `pds-radio`, `pds-checkbox`, `pds-combobox`, `pds-multiselect`) - `open`/expanded for overlay components (`pds-modal`, `pds-dropdown-menu`, `pds-tooltip`, `pds-accordion`, `pds-combobox`) -- **Direction (optional):** `rtl` via `globals=direction:rtl`, only if the PR touches logical-property / layout SCSS or the component is directional. + +> **RTL is intentionally out of scope for now.** Storybook does expose a `direction` global (`.storybook/preview.js` defines the `withDirection` decorator), but this skill does not yet plumb it through the Phase 5 loop or add a mirroring rule to evaluate it against. Rather than ship a half-wired axis, RTL is omitted; add it as a full axis (loop + URL param + rule) in a later iteration if needed. ### Variant coverage @@ -195,7 +200,6 @@ Capture from the chrome-free iframe endpoint (no Storybook toolbar/sidebar in fr ``` $SB/iframe.html?id=<storyId>&viewMode=story&globals=theme:dark $SB/iframe.html?id=<storyId>&viewMode=story&globals=theme:light -# with direction: &globals=theme:dark;direction:rtl (semicolon-separated) ``` ### Output location (absolute path — important) @@ -235,7 +239,7 @@ Decide, per component, whether to also capture a **baseline** (the component as If capturing baselines, note that it requires rebuilding Stencil on the base branch — do it in one batch after the PR-branch capture (Phase 5b), not interleaved. If the working tree is dirty or the user is mid-task, ask before switching branches. -Skipping baselines is always acceptable — say so in the report and rely on the rules-based evaluation. Chromatic remains the authoritative pixel-diff in CI; this baseline mode is a local convenience. +Skipping baselines is always acceptable — say so in the report and rely on the rules-based evaluation. (Once Chromatic CI lands it will be the authoritative pixel-diff; until then this baseline mode is the closest thing to one, so prefer running it on token/SCSS diffs.) --- @@ -258,31 +262,35 @@ mcp__playwright__browser_wait_for({ timeout: 500 }) # let fon ``` Confirm the custom element actually upgraded (has a shadow root / rendered content) — a bare unstyled tag means `dist/` wasn't loaded (see Rule 2 retry). -**Step 3 — Capture default, both themes** (repeat Steps 1–2 for `theme:light` and `theme:dark`): +**Step 3 — Capture both themes, reading the console after *each* navigation.** Run this block for `theme:light`, then `theme:dark`. Read the console **immediately after each navigation** — Playwright MCP's per-navigation read (`all` omitted) only reports errors since the last `navigate`, so a single read at the end of the story would miss the earlier theme entirely: ``` -mcp__playwright__browser_take_screenshot({ filename: "<component>__<story>__desktop__<theme>.png" }) +# for THEME in light, dark: +mcp__playwright__browser_navigate({ url: "$SB/iframe.html?id=<storyId>&viewMode=story&globals=theme:<THEME>" }) +mcp__playwright__browser_resize({ width: 1280, height: 800 }) # 375×812 for flagged responsive +mcp__playwright__browser_wait_for({ selector: "<pds-component>" }) +mcp__playwright__browser_wait_for({ timeout: 500 }) +mcp__playwright__browser_take_screenshot({ filename: "<REPORT_DIR>/<component>__<story>__desktop__<THEME>.png" }) +mcp__playwright__browser_console_messages({ level: "error" }) # per-navigation; judge per Step 5 ``` +`<REPORT_DIR>` is the **absolute** per-run directory from Phase 3 (`<repo-abs>/.claude/visual-test-reports/pr-<number>`). Never pass a bare relative filename — the MCP browser's cwd is not your shell's, so a relative name lands the PNG outside the repo (see "Output location" in Phase 3). -**Step 4 — Capture interactive states** (desktop only, per the plan): +**Step 4 — Capture interactive states** (desktop, per the plan; capture per theme where the state is theme-sensitive, e.g. the focus ring). Read the console again after interacting, still per-navigation: ``` -# hover mcp__playwright__browser_hover({ selector: "<interactive part>" }) -mcp__playwright__browser_take_screenshot({ filename: "<component>__<story>__desktop__<theme>__hover.png" }) +mcp__playwright__browser_take_screenshot({ filename: "<REPORT_DIR>/<component>__<story>__desktop__<theme>__hover.png" }) # keyboard focus (focus ring is a frequent dark-mode regression) mcp__playwright__browser_press_key({ key: "Tab" }) -mcp__playwright__browser_take_screenshot({ filename: "<component>__<story>__desktop__<theme>__focus.png" }) +mcp__playwright__browser_take_screenshot({ filename: "<REPORT_DIR>/<component>__<story>__desktop__<theme>__focus.png" }) # open/expanded overlays mcp__playwright__browser_click({ selector: "<trigger>" }) -mcp__playwright__browser_take_screenshot({ filename: "<component>__<story>__desktop__<theme>__open.png" }) -``` +mcp__playwright__browser_take_screenshot({ filename: "<REPORT_DIR>/<component>__<story>__desktop__<theme>__open.png" }) -**Step 5 — Console check.** Read `error`-level console messages **for this navigation only** — do not pass `all: true`: -``` -mcp__playwright__browser_console_messages({ level: "error" }) +mcp__playwright__browser_console_messages({ level: "error" }) # after interaction, still per-navigation ``` -`all: true` returns the whole session's history, which after any Storybook restart floods with stale `ERR_CONNECTION_REFUSED` (the dead server) and `NoStoryMatchError` (a tab pointed at a story the new index lacks) — none of which are findings. Reading per-navigation gives the clean, current-story truth (and it matches the per-page `Console: N errors` counter the navigate call reports). **Filter harness noise before judging** — a `favicon.ico` (or other static-asset) 404 and Vite HMR chatter are expected and are **not** findings. A message fails Rule 4 only when it originates from the component bundle (stack/URL includes `pine-core/*.js`) or the story itself — e.g. a Stencil render/hydration error, a missing-token throw, or a thrown event listener. Those are real findings: capture the full message text into the report. + +**Step 5 — Judge the console reads.** The per-navigation reads from Steps 3–4 are what you judge — **never pass `all: true`**: it returns the whole session and, after any Storybook restart, floods with stale `ERR_CONNECTION_REFUSED` (the dead server) and `NoStoryMatchError` (a tab pointed at a story the new index lacks), none of which are findings. **Filter harness noise before judging** — a `favicon.ico` (or other static-asset) 404 and Vite HMR chatter are expected and are **not** findings. A message fails Rule 4 only when it originates from the component bundle (stack/URL includes `pine-core/*.js`) or the story itself — e.g. a Stencil render/hydration error, a missing-token throw, or a thrown event listener. Those are real findings: capture the full message text into the report. **Step 6 — Evaluate every screenshot against the rules below.** @@ -321,11 +329,18 @@ mcp__playwright__browser_console_messages({ level: "error" }) After all PR-branch shots are approved: -1. Confirm a clean tree; record the current branch. -2. **`build.stencil` dirties tracked generated files** — `libs/core/src/components.d.ts` and `libs/react/src/components/react-component-lib/createComponent.tsx` are checked in but regenerated on every build, so a branch switch **aborts** ("would be overwritten") after you've built. Reset them before each `git checkout`: `git checkout -- libs/core/src/components.d.ts libs/react/src/components/react-component-lib/createComponent.tsx`. (These are generated, not authored — safe to discard. If a genuine unrelated WIP file is also dirty, `git stash` it and restore after.) -3. `git checkout <baseRef>` → `( cd libs/core && npm run build.stencil )` → **restart** Storybook (don't reuse the head-branch server — its story index is stale and every open tab will throw `NoStoryMatchError` against the new bundle). -4. Re-capture the **same** story×theme×viewport matrix into `*__baseline.png` filenames. Note that **stories new in the PR won't exist on base** — compare only shared stories (e.g. `--default`); a new story with no baseline is validated against the rules alone. -5. Reset generated files again (step 2), `git checkout <headRef>`, rebuild Stencil to restore the PR state. +1. Confirm a clean tree (`git status`); record the current branch. If it's dirty with genuine WIP, `git stash` it and restore after — never discard the user's work (see the error-handling row). +2. **`build.stencil` dirties two tracked files** — `libs/core/src/components.d.ts` and `libs/react/src/components/react-component-lib/createComponent.tsx` — so a branch switch **aborts** ("would be overwritten") after you've built. Restore them to their committed state before each `git checkout`: + ```bash + git checkout -- libs/core/src/components.d.ts libs/react/src/components/react-component-lib/createComponent.tsx + ``` + ⚠️ `createComponent.tsx` is **not** purely generated: it carries a hand-authored double-registration guard the stock `@stencil/react-output-target` template lacks, and `build.stencil` strips it back to the vendor version. `git checkout --` is the right move *because* it restores the **committed** (guarded) copy — but never resolve this by deleting the file, keeping the built version, or `git add`-ing the stripped one. +3. **Stop the running Storybook first, then** `git checkout <baseRef>` → `( cd libs/core && npm run build.stencil )` → start Storybook again. You must stop first — nothing else frees port 6006, and a second `start.storybook` either errors (address in use) or the health check falls through to Phase 2's "reuse the existing server", which would screenshot the **wrong branch's** bundle (the `NoStoryMatchError` trap): + ```bash + lsof -ti:6006 | xargs kill 2>/dev/null # or KillShell the Phase 2 background task + ``` +4. Re-capture the **same** story×theme×viewport matrix into `*__baseline.png` filenames (same absolute `<REPORT_DIR>`). Note that **stories new in the PR won't exist on base** — compare only shared stories (e.g. `--default`); a new story with no baseline is validated against the rules alone. +5. Stop Storybook again, reset the generated files again (step 2), `git checkout <headRef>`, rebuild Stencil, **then run the step-2 `git checkout --` once more** — the rebuild re-strips `createComponent.tsx`'s authored guard, so without this final reset you'd leave the tree with a real fix silently reverted. Confirm `git status` is clean before finishing. 6. For each pair, compare before/after and note: **intended** (matches the PR's stated change) vs **regression** (unexpected delta, esp. in a theme/variant the PR didn't claim to touch). A console error present on a new story but absent from the shared `--default` on both branches is **PR-introduced and scoped to that story** — the highest-signal verdict this mode produces. ### Loop termination @@ -338,11 +353,23 @@ After all PR-branch shots are approved: ## Phase 6 — Report -Generate a markdown report and post it as a PR comment. +Write the filled-in report to a file, then post it **once**. -```bash -mkdir -p .claude/visual-test-reports -``` +**Screenshots can't be auto-attached.** `gh` cannot upload images or video to a PR comment — GitHub only accepts them via drag-drop in the browser. So the posted comment is a **text** report (scope, findings, verdict) that references the local screenshot filenames; the PNGs stay in `<REPORT_DIR>` for the user to drag in if they want them inline. Do **not** write `![](local-path)` — local files won't render on GitHub. + +1. **Write the report to a file** — the post step reads it back, so it must exist on disk; don't just print the template: + ```bash + REPORT=".claude/visual-test-reports/pr-<number>-visual-test.md" + mkdir -p "$(dirname "$REPORT")" + ``` + Fill the template below and write it to `$REPORT` with the **Write tool**. + +2. **Dedup guard — do not double-post.** This skill is built to be re-run (Phase 5 retry loop, Phase 5b baseline), so re-posting is the normal case, not the edge case. Before posting, check for a prior run's comment and **skip unless the user explicitly asks to post again** (mirrors the guard `pine-run-gauntlet` added after #746): + ```bash + gh pr view <number> --repo Kajabi/pine --json comments \ + --jq '.comments[].body' | grep -q "Posted by pine-visual-test-pr" && echo "already posted — skip or edit the existing comment" + ``` + If found, tell the user in-chat and stop (or edit the existing comment) rather than adding another. ### Report template @@ -356,7 +383,7 @@ mkdir -p .claude/visual-test-reports **Result:** PASS (<N>/<N>) | PARTIAL (<N>/<M> — see findings/blockers) | BLOCKED ## Components in scope -- <component> — <n stories> — <light+dark | +mobile | +rtl> +- <component> — <n stories> — <light+dark | +mobile> ## Approved Screenshots @@ -380,11 +407,16 @@ mkdir -p .claude/visual-test-reports - [x] Component renders - [x] Assets loaded - [x] Both themes - [x] No console errors - [x] Focus visible - [x] Viewport - [x] No clipping - [x] Token discipline - [x] Naming - [x] Variant coverage + +--- +_Posted by pine-visual-test-pr_ ``` -Post it: +The `_Posted by pine-visual-test-pr_` footer is the marker the Step 2 dedup guard greps for — keep it in the template. + +3. **Post from the file** — never a double-quoted `--body "$(cat …)"` (report bodies contain backticks, quotes, and `$`-sequences the shell would re-evaluate); always pass `--repo` so it targets Pine even when run from another checkout: ```bash -gh pr comment <number> --body "$(cat .claude/visual-test-reports/pr-<number>-visual-test.md)" +gh pr comment <number> --repo Kajabi/pine --body-file "$REPORT" ``` --- @@ -403,7 +435,7 @@ gh pr comment <number> --body "$(cat .claude/visual-test-reports/pr-<number>-vis <url> ``` -If there are real findings, tell the user which components/themes regressed before they request review. If clean, note it's pre-validated locally and Chromatic in CI is the authoritative pixel-diff gate. +If there are real findings, tell the user which components/themes regressed before they request review. If clean, note it's pre-validated locally — and that, until Chromatic CI lands, this is currently Pine's only pixel-level check, so treat it as the main signal rather than a secondary one. --- @@ -415,7 +447,7 @@ If there are real findings, tell the user which components/themes regressed befo | 0 | `gh` not authenticated | `gh auth login`, stop | | 0 | root deps missing | `npm install` at root, stop | | 2 | Stencil build fails | Surface the Stencil error, fix/rebuild — Storybook renders nothing without `dist/` | -| 2 | Storybook never serves `/index.json` | Read background output; port 6006 taken → reuse; else stop with the error | +| 2 | Storybook never serves `/index.json` | Read background output; port 6006 taken by a server for **this same branch** → reuse; after a branch switch (5b) never reuse — `lsof -ti:6006 \| xargs kill` first, then start fresh; else stop with the error | | 2 | Title→id mapping empty | Component may have no stories — note it; nothing to capture for that component | | 5 | Blank/unstyled render | Rebuild Stencil, hard-reload; 3x → structural blocker | | 5b | Dirty tree at branch switch | Ask before `git checkout`; never discard the user's work | @@ -429,11 +461,12 @@ If there are real findings, tell the user which components/themes regressed befo - Do NOT loop indefinitely — 3 retries per item, then blocker. - Do NOT include blank/unstyled or error screenshots in the approved set. - Do NOT switch branches for a baseline without confirming a clean tree. -- Do NOT treat this as a replacement for Chromatic — it's the fast local pass that runs before it. +- Do NOT `git add` the vendor-stripped `createComponent.tsx` after a build — restore the committed (guarded) copy with `git checkout --`. +- Do NOT describe Chromatic CI as an existing gate — only the local Storybook panel is merged today; hedge to "once Chromatic CI lands". ## Related Skills - `pine-design-review` — token / SCSS / a11y / Figma code review (pair with this; it reads code, this reads pixels) - `pine-run-gauntlet` — parallel multi-reviewer pass before a PR (code/security/design/existence) - `pine-existence-review` — checks whether a component/API already exists before adding new -- Chromatic (`@chromatic-com/storybook`, in CI) — authoritative cloud pixel-diff; this skill is the local complement +- Chromatic — `@chromatic-com/storybook` is installed as a local Storybook panel; the Chromatic **CI** pixel-diff job is not merged yet. Until it lands, this skill is Pine's only pixel-level regression check; after it lands, this is the fast local complement. From e857714fab826f1cbb6afbacc1c3f3fc729c6569 Mon Sep 17 00:00:00 2001 From: Quinton Jason <quinton.jason@gmail.com> Date: Mon, 10 Aug 2026 16:13:10 -0500 Subject: [PATCH 5/6] docs(skills): fix two follow-on findings from #788 re-review Cursor Bugbot re-review flagged two issues introduced by the prior fix: - Cross-branch capture (High): threading the PR number scoped the right files but Phase 2 still builds/screenshots the checked-out tree. Added a Phase 1 branch-match guard that stops on mismatch (no auto-switch). - Stash ordering (Medium): Phase 5b stashed WIP before resetting the generated files, so the stash could snapshot the vendor-stripped createComponent.tsx and reintroduce it on pop. Reordered: reset generated files first, then stash only genuine WIP. --- .claude/skills/pine-visual-test-pr/SKILL.md | 27 +++++++++++++-------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/.claude/skills/pine-visual-test-pr/SKILL.md b/.claude/skills/pine-visual-test-pr/SKILL.md index d51af944b..acfdca63d 100644 --- a/.claude/skills/pine-visual-test-pr/SKILL.md +++ b/.claude/skills/pine-visual-test-pr/SKILL.md @@ -104,16 +104,22 @@ Also verify Playwright MCP is reachable by calling `mcp__playwright__browser_sna ```bash gh pr view --json number,url,title,headRefName,baseRefName,body # <number> = .number ``` -2. **Get changed files** (always numbered): +2. **Verify you're on the PR's head branch — capture renders the checked-out tree, not the number.** Phase 2 builds `dist/` from whatever is currently checked out, so threading `<number>` only fixes *scope* (step 1); an explicit number from a **different** branch would compute the right file list but screenshot the wrong code. Compare, and stop on mismatch — do not auto-switch (branch changes are the user's call): + ```bash + test "$(git branch --show-current)" = "$(gh pr view <number> --repo Kajabi/pine --json headRefName --jq .headRefName)" \ + && echo 'on PR branch' || echo 'MISMATCH — check out the PR head branch first' + ``` + On mismatch, stop and tell the user to `git checkout <headRef>` (and pull) before re-running — do **not** proceed to capture. +3. **Get changed files** (always numbered): ```bash gh pr diff <number> --name-only ``` -3. **Determine component scope.** Reduce the diff to the set of changed components: +4. **Determine component scope.** Reduce the diff to the set of changed components: ```bash gh pr diff <number> --name-only | grep -oE 'libs/core/src/components/[^/]+' | sort -u ``` Each entry is a component directory (e.g. `pds-button`, `pds-alert`). This is the scope — the skill only screenshots stories for these components (plus their child components, e.g. `pds-tabs` → `pds-tab`, `pds-tabpanel`). -4. **Classify each changed file** to predict the visual risk (drives the capture plan): +5. **Classify each changed file** to predict the visual risk (drives the capture plan): | Changed file | Visual risk | Emphasis | |---|---|---| | `*.tokens.scss` / `*.scss` | color, spacing, dark-mode | **both themes**, all variants | @@ -329,19 +335,20 @@ mcp__playwright__browser_console_messages({ level: "error" }) # after inte After all PR-branch shots are approved: -1. Confirm a clean tree (`git status`); record the current branch. If it's dirty with genuine WIP, `git stash` it and restore after — never discard the user's work (see the error-handling row). -2. **`build.stencil` dirties two tracked files** — `libs/core/src/components.d.ts` and `libs/react/src/components/react-component-lib/createComponent.tsx` — so a branch switch **aborts** ("would be overwritten") after you've built. Restore them to their committed state before each `git checkout`: +1. Record the current branch (you'll return to it in step 6). +2. **Reset the two build-generated files first** — a prior `build.stencil` leaves `libs/core/src/components.d.ts` and `libs/react/src/components/react-component-lib/createComponent.tsx` dirty, and a branch switch **aborts** ("would be overwritten") while they are: ```bash git checkout -- libs/core/src/components.d.ts libs/react/src/components/react-component-lib/createComponent.tsx ``` - ⚠️ `createComponent.tsx` is **not** purely generated: it carries a hand-authored double-registration guard the stock `@stencil/react-output-target` template lacks, and `build.stencil` strips it back to the vendor version. `git checkout --` is the right move *because* it restores the **committed** (guarded) copy — but never resolve this by deleting the file, keeping the built version, or `git add`-ing the stripped one. -3. **Stop the running Storybook first, then** `git checkout <baseRef>` → `( cd libs/core && npm run build.stencil )` → start Storybook again. You must stop first — nothing else frees port 6006, and a second `start.storybook` either errors (address in use) or the health check falls through to Phase 2's "reuse the existing server", which would screenshot the **wrong branch's** bundle (the `NoStoryMatchError` trap): + ⚠️ `createComponent.tsx` is **not** purely generated: it carries a hand-authored double-registration guard the stock `@stencil/react-output-target` template lacks, and `build.stencil` strips it back to the vendor version. `git checkout --` is the right move *because* it restores the **committed** (guarded) copy — never resolve this by deleting the file, keeping the built version, or `git add`-ing the stripped one. +3. **Only now check for genuine WIP.** If `git status` still shows unrelated changes, `git stash` them and restore after — never discard the user's work. Doing the step-2 reset **first** is what stops the stash from snapshotting the vendor-stripped `createComponent.tsx` — otherwise a later `git stash pop` would silently reintroduce the guard-less version. +4. **Stop the running Storybook first, then** `git checkout <baseRef>` → `( cd libs/core && npm run build.stencil )` → start Storybook again. You must stop first — nothing else frees port 6006, and a second `start.storybook` either errors (address in use) or the health check falls through to Phase 2's "reuse the existing server", which would screenshot the **wrong branch's** bundle (the `NoStoryMatchError` trap): ```bash lsof -ti:6006 | xargs kill 2>/dev/null # or KillShell the Phase 2 background task ``` -4. Re-capture the **same** story×theme×viewport matrix into `*__baseline.png` filenames (same absolute `<REPORT_DIR>`). Note that **stories new in the PR won't exist on base** — compare only shared stories (e.g. `--default`); a new story with no baseline is validated against the rules alone. -5. Stop Storybook again, reset the generated files again (step 2), `git checkout <headRef>`, rebuild Stencil, **then run the step-2 `git checkout --` once more** — the rebuild re-strips `createComponent.tsx`'s authored guard, so without this final reset you'd leave the tree with a real fix silently reverted. Confirm `git status` is clean before finishing. -6. For each pair, compare before/after and note: **intended** (matches the PR's stated change) vs **regression** (unexpected delta, esp. in a theme/variant the PR didn't claim to touch). A console error present on a new story but absent from the shared `--default` on both branches is **PR-introduced and scoped to that story** — the highest-signal verdict this mode produces. +5. Re-capture the **same** story×theme×viewport matrix into `*__baseline.png` filenames (same absolute `<REPORT_DIR>`). Note that **stories new in the PR won't exist on base** — compare only shared stories (e.g. `--default`); a new story with no baseline is validated against the rules alone. +6. Stop Storybook again, re-run step 2's reset, `git checkout <headRef>`, rebuild Stencil, **then run step 2's reset once more** — the rebuild re-strips `createComponent.tsx`'s authored guard, so without this final reset you'd leave the tree with a real fix silently reverted. Restore any step-3 stash, then confirm `git status` is clean before finishing. +7. For each pair, compare before/after and note: **intended** (matches the PR's stated change) vs **regression** (unexpected delta, esp. in a theme/variant the PR didn't claim to touch). A console error present on a new story but absent from the shared `--default` on both branches is **PR-introduced and scoped to that story** — the highest-signal verdict this mode produces. ### Loop termination From ddf7b5da5970ae4390db53b2a012a46d1f5ce116 Mon Sep 17 00:00:00 2001 From: Quinton Jason <quinton.jason@gmail.com> Date: Mon, 10 Aug 2026 16:17:06 -0500 Subject: [PATCH 6/6] docs(skills): fix stash-vs-clean-check contradiction in Phase 5b Re-review flagged that restoring the step-3 WIP stash and then requiring a clean git status contradict each other. The clean check now applies to the skill's own artifacts (the two generated files match committed) and runs before restoring the stash; the stash restore comes last and legitimately re-dirties the tree with the user's own WIP. --- .claude/skills/pine-visual-test-pr/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/skills/pine-visual-test-pr/SKILL.md b/.claude/skills/pine-visual-test-pr/SKILL.md index acfdca63d..c87a7d38f 100644 --- a/.claude/skills/pine-visual-test-pr/SKILL.md +++ b/.claude/skills/pine-visual-test-pr/SKILL.md @@ -347,7 +347,7 @@ After all PR-branch shots are approved: lsof -ti:6006 | xargs kill 2>/dev/null # or KillShell the Phase 2 background task ``` 5. Re-capture the **same** story×theme×viewport matrix into `*__baseline.png` filenames (same absolute `<REPORT_DIR>`). Note that **stories new in the PR won't exist on base** — compare only shared stories (e.g. `--default`); a new story with no baseline is validated against the rules alone. -6. Stop Storybook again, re-run step 2's reset, `git checkout <headRef>`, rebuild Stencil, **then run step 2's reset once more** — the rebuild re-strips `createComponent.tsx`'s authored guard, so without this final reset you'd leave the tree with a real fix silently reverted. Restore any step-3 stash, then confirm `git status` is clean before finishing. +6. Stop Storybook again, re-run step 2's reset, `git checkout <headRef>`, rebuild Stencil, **then run step 2's reset once more** — the rebuild re-strips `createComponent.tsx`'s authored guard, so without this final reset you'd leave the tree with a real fix silently reverted. Now confirm the tree is clean of **skill-induced** changes (the two generated files match committed). **Restore any step-3 stash last** — that re-dirties the tree with the user's own WIP, which is expected and is **not** a failure of this clean check. 7. For each pair, compare before/after and note: **intended** (matches the PR's stated change) vs **regression** (unexpected delta, esp. in a theme/variant the PR didn't claim to touch). A console error present on a new story but absent from the shared `--default` on both branches is **PR-introduced and scoped to that story** — the highest-signal verdict this mode produces. ### Loop termination