diff --git a/.agents/skills/pr-visual-writeup/SKILL.md b/.agents/skills/pr-visual-writeup/SKILL.md index 0a43cfc7f3..8c12f1b396 100644 --- a/.agents/skills/pr-visual-writeup/SKILL.md +++ b/.agents/skills/pr-visual-writeup/SKILL.md @@ -18,13 +18,14 @@ If the user only wants a text-only PR description, don't use this skill — it's ## The shape of the work -Five phases. Phases 2 and 3 are the parallel-heavy ones — lean on subagents there. +Six phases. Phases 2 and 3 are the parallel-heavy ones — lean on subagents there. Phase 6 is the cleanup that puts the user's working tree back. -1. **Scope** — figure out which PR, which routes, which dev server, which auth -2. **Capture (parallel)** — matrix of {page × theme × viewport}, plus scroll animations +1. **Scope** — figure out which PR, which routes, which dev server, which auth, **which selectors are new UI** +2. **Capture** — "after" pass first (parallel: page × theme × viewport, red borders on new UI), then stash + checkout base + capture "before" pass against the same dev server. 3. **Process (parallel)** — convert scroll videos → GIFs (inline-playable), prep gist 4. **Upload** — one gist, one commit, all files; get raw URLs -5. **Compose + set** — markdown body with tables, then `gh pr edit --body-file` +5. **Compose + set** — markdown body with before/after tables, then `gh pr edit --body-file` +6. **Restore** — `git checkout ` + `git stash pop` so the user's working tree is exactly where they left it ## Phase 1 — Scope @@ -34,25 +35,69 @@ Before you capture anything, know: - **Changed UI routes** — `gh pr diff --name-only` and filter for page/route files. For Next.js look for `**/page*.tsx` / `**/*page-client.tsx`. Map route files to URL paths based on the app router convention. Ignore changes purely in backend / shared components unless they have an obvious UI surface. - **Dev server port** — `lsof -iTCP -sTCP:LISTEN -P -n | grep node` and `curl -s http://localhost:/ | grep -oE '[^<]+'` to identify which port is the dashboard vs. API vs. docs vs. mock-OAuth. - **Auth flow** — if the app requires login, inspect the sign-in page for the OAuth provider to use, and ask the user (or infer from context) which dev account to sign in as. Mock OAuth servers typically accept any email. +- **New-UI selectors** — for each changed route, derive a list of CSS selectors that point to the UI elements added or visibly modified in this PR. Read the diff for that route's component file(s), pick stable selectors (`data-testid`, semantic role, unique class, or a `:has()` text match) for each new/changed block. These selectors drive the red-border highlight pass on "after" captures. If a route has no visually changed elements (only behavior/copy refactor), record `[]` and skip highlighting for that route. +- **Base ref for "before"** — `gh pr view --json baseRefName` gives you the base branch (usually `dev` or `main`). The "before" pass swaps the working tree to that ref under the same dev server — see Phase 2. Skip the before pass entirely if the user says "after only" or if every route is greenfield. +- **Working-tree state** — `git status --porcelain`. If there are uncommitted changes, they need to be stashed before the base checkout. Record whether a stash will be needed and the current branch name so you can restore exactly. -Record these facts somewhere (a scratchpad file under `/tmp//scope.md` is fine) — the parallel subagents in Phase 2 need them. +Record all of these in `/tmp/pr--visuals/scope.md` — the Phase 2 subagents need them. -## Phase 2 — Parallel capture +## Phase 2 — Capture -This is the skill's core trick. You have N pages × M themes × K viewports of screenshots to take. If you do this in one browser, you navigate sequentially — 9 × 2 × 2 = 36 navigations at ~5s each = 3 minutes. If you fan out, you do it in ~45s. +Two ordered passes against a **single** dev server. The "after" pass runs first while the head branch is checked out; then we swap the working tree to the base ref (same server, same port, HMR rebuilds) and run the "before" pass. -### Fan-out plan +### Pass 1 — "after" (parallel) -Spawn one subagent per **(theme, viewport)** combination. Each subagent owns a named `agent-browser` session, authenticates once, captures every page in its assigned theme/viewport, and returns the output directory. Typical combinations: +The head branch is already checked out and the dev server is already running, so this is just the parallel matrix you started with. -- `light-standard` (1920×1200, theme=light) -- `dark-standard` (1920×1200, theme=dark) -- `light-wide` (2560×1440, theme=light, a subset of pages) -- `dark-wide` (2560×1440, theme=dark, a subset of pages) +Spawn one subagent per **(theme, viewport)** combination. Each owns a named `agent-browser` session, authenticates once, captures every page in its slice with **red borders injected on new-UI selectors**, and returns the output directory. -Widescreen captures are usually only worth taking for the "flagship" pages (the 3-5 most important ones). Full matrix on every page is overkill. +- `after-light-standard` (1920×1200, theme=light, red borders on) +- `after-dark-standard` (1920×1200, theme=dark, red borders on) +- `after-light-wide` / `after-dark-wide` (2560×1440, flagship pages only) -**Important:** issue all Agent tool calls for capture subagents **in a single assistant message** so they run concurrently. If you spawn them one at a time across turns, you've lost the parallelism. +Issue all Agent calls in **one assistant message** so they run concurrently. Unique `--session-name` per agent. + +**Red borders.** The "after" subagents inject a stylesheet that outlines the new-UI selectors recorded in Phase 1 — see the `pr-visual-highlight` block in `references/capture-patterns.md`. Inject after navigation + settle, before the screenshot. Use `outline` (not `border` — `border` shifts layout). Skip the inject for routes whose selector list is empty. + +**Wait for the page to be ready before every shot.** Next.js dev compiles routes on demand and the dashboard renders skeletons during data fetch. Networkidle alone is not enough — see the `wait-for-ready` recipe in `references/capture-patterns.md`. Each subagent should also do a one-time *warm-up pass* over its assigned routes before the real capture loop, so the on-demand compile cost is paid against a throwaway hit rather than the screenshot. + +### Pass 1.5 — swap working tree to base + +After Pass 1 finishes, **before** spawning Pass 2: + +```bash +# 1. Record state so Phase 6 can restore it +ORIG_BRANCH=$(git rev-parse --abbrev-ref HEAD) +echo "$ORIG_BRANCH" > /tmp/pr--visuals/orig-branch.txt + +# 2. Stash anything dirty (including untracked) — even if scope said clean, do this defensively +git stash push --include-untracked -m "pr-visual-writeup pr- after-pass" || true +git rev-parse stash@{0} 2>/dev/null > /tmp/pr--visuals/stash-ref.txt || rm -f /tmp/pr--visuals/stash-ref.txt + +# 3. Move to the base ref +git checkout # usually `dev` or `main`, from Phase 1 scope +git pull --ff-only + +# 4. Give the dev server time to HMR-rebuild against the new tree +sleep 5 && curl -fs http://localhost:/ >/dev/null +``` + +Stash the changes **even if `git status` reported clean.** Defensive: the user might have edited a file mid-skill, and a failed checkout halfway through is much worse than a no-op `git stash pop` at the end. + +If the dev server doesn't recover from HMR after the checkout (some Next configs choke on large simultaneous file changes), the fallback is to restart it manually — flag this to the user rather than papering over it. + +### Pass 2 — "before" (parallel) + +Same fan-out as Pass 1, but: +- Filename suffix is `-before-` instead of `-after-` +- **No red-border injection.** "Before" is the unmodified baseline. +- Skip routes that didn't exist on the base ref (greenfield) — the subagent will hit a 404; have it log and move on instead of failing the whole pass. +- Long-tail pages are usually not worth capturing in "before" — restrict Pass 2 to the flagship set unless the user asks otherwise. + +Filename convention so Phase 5 can pair shots up: + +- `/shots/-before-[-wide].png` +- `/shots/-after-[-wide].png` The exact subagent prompt pattern lives in `references/capture-patterns.md` — read it before spawning. @@ -92,9 +137,10 @@ Full recipe is in `references/gist-upload.md`. Summary: create a public gist via Markdown structure template is in `references/pr-body-template.md`. The load-bearing patterns: - **Summary** paragraph + `Base: → Head:` + scope line (files, +lines) -- **Screenshots** section with one subsection per "flagship" page, each using a 2-col light/dark table, then a widescreen table -- **Other migrated surfaces** compact table for the long tail +- **Screenshots** section with one subsection per "flagship" page, each using a 2×2 before/after × light/dark table, then a widescreen table +- **Other migrated surfaces** compact table for the long tail (after-only) - **Scroll behaviour** section with a light/dark GIF table +- A short legend noting that the red outlines on "after" shots mark the new/changed UI - Everything after the visual section is the usual PR body: What's new, Notes for reviewers, Test plan Set it with: @@ -104,6 +150,25 @@ gh pr edit --body-file Confirm with the user before pushing if the PR is on a public repo — this is a shared-state action. On a personal fork or draft PR, go ahead. +## Phase 6 — Restore the working tree + +The "before" pass left the repo on the base ref with the user's original changes stashed. Put it back: + +```bash +ORIG=$(cat /tmp/pr--visuals/orig-branch.txt) +git checkout "$ORIG" +if [ -f /tmp/pr--visuals/stash-ref.txt ]; then + git stash pop "$(cat /tmp/pr--visuals/stash-ref.txt)" || { + echo "stash pop hit conflicts — leaving stash in place" + git stash list | head -5 + } +fi +``` + +Do this **even if Phase 5 failed**, or the user is stuck on the wrong branch with their work hidden in a stash. If `git stash pop` conflicts, leave the stash alone and surface the conflict to the user explicitly — never auto-resolve. + +After restore: `git status` and `git rev-parse --abbrev-ref HEAD` for sanity, paste the output back to the user so they can verify before continuing other work. + ## A note on trust boundaries Three distinct credentials touch this workflow. Keep them straight: @@ -133,5 +198,7 @@ Create a `/tmp/pr--visuals/` workspace to hold everything. After the PR body ├── clips/ # webm + gif scroll animations ├── body.md # composed PR description ├── gist-id.txt # for re-pushing if you add more shots later -└── urls.txt # raw URL per file, for copy-paste +├── urls.txt # raw URL per file, for copy-paste +├── orig-branch.txt # phase 1.5: branch to return to in phase 6 +└── stash-ref.txt # phase 1.5: stash ref to pop in phase 6 (absent if nothing was stashed) ``` diff --git a/.agents/skills/pr-visual-writeup/references/capture-patterns.md b/.agents/skills/pr-visual-writeup/references/capture-patterns.md index 4692a21f07..bca9963c8d 100644 --- a/.agents/skills/pr-visual-writeup/references/capture-patterns.md +++ b/.agents/skills/pr-visual-writeup/references/capture-patterns.md @@ -1,73 +1,223 @@ -# Capture patterns +# Capture Patterns -Reconstructed from SKILL.md hints — edit freely to match your own `agent-browser` or Playwright setup. +Recipes for `agent-browser` capture work. Read this when you're about to spawn Phase 2 subagents or run scroll animations yourself. -## Subagent prompt pattern (per theme/viewport) +## Prerequisite: load the agent-browser skill -Each subagent owns a named browser session and handles the full capture pass for one `(theme, viewport)` combination. Name the session so it's distinct from the main conversation's session and from sibling subagents — e.g. `pr-light-standard`. +Before any browser command, the subagent must run: + +```bash +agent-browser skills get agent-browser +``` + +This gives it the CLI's current syntax. Don't guess at flags — they change between versions. + +## The capture-subagent prompt pattern + +When you spawn a subagent to capture a (theme, viewport) slice, give it a self-contained prompt. It has no memory of the Phase 1 scope; everything must be spelled out. Template: ``` -You are capturing screenshots for PR # in the theme at viewport. - -Scope (from /tmp/pr--visuals/scope.md): -- Dev server: http://localhost: -- Login: via mock-OAuth at -- Pages (relative URLs): - - /projects// - - /projects// - - ... - -Do this: -1. Start (or reuse) an agent-browser session named "pr--". -2. Set viewport to . -3. Navigate to the sign-in page and complete the mock-OAuth flow once. -4. Switch the app theme to "" (usually a dropdown in user settings, or - the `prefers-color-scheme` override in devtools — inspect the app first). -5. For each page in the list: navigate, wait for network-idle + any late-mount - skeletons to settle (~1s extra), then take a full-page screenshot. - Save as /tmp/pr--visuals/shots/____.png - where route-slug replaces slashes with dashes. -6. Return the list of PNG paths you produced. - -Do NOT: -- Close the browser session at the end (other subagents may be using it, and - re-login is wasteful). -- Use agent-browser's `record` action — it spins up a fresh context that - drops dev-mode auth state. For scroll clips see the frame-stitch recipe. +You are capturing screenshots for PR # of . Context: + +- Dashboard dev URL: http://localhost: # head port for "after", base-worktree port for "before" +- Branch slice: +- Login: click the "" OAuth button, fill the mock OAuth form with "", submit. The local dashboard creates a session automatically. +- Project to navigate into: (click it on /projects after login) +- Routes to capture (relative to the project URL): + /users highlight selectors: ["[data-testid=users-table-toolbar]", "section:has(> h2:text('Bulk actions'))"] + /teams highlight selectors: [] + ... +- Theme: +- Viewport: x +- Output directory: /tmp/pr--visuals/shots/ +- Highlight new UI: # true ONLY for "after" subagents + +For each route: +1. Navigate +2. **Wait for the page to be ready** — run the `wait-for-ready` recipe (below). Networkidle alone is not enough in Next dev mode: the on-demand compiler and skeleton placeholders both finish *after* networkidle. +3. If Highlight new UI is true AND this route has highlight selectors: + run the pr-visual-highlight injector with this route's selector list +4. Screenshot → /tmp/pr--visuals/shots/--.png + (widescreen variant: ...---wide.png) +5. If you injected highlights, remove them before navigating to the next route (so they don't bleed via cached styles): `agent-browser eval "document.getElementById('pr-visual-highlight')?.remove()"` + +Before the per-route loop, **warm the routes**: navigate to each one in sequence with a generous `wait-for-ready` budget, then start the real capture loop. The first hit pays the compile cost; the second hit is fast and produces the clean screenshot. + +Steps to run first in your session: +- `agent-browser skills get agent-browser` +- `agent-browser --session-name open http://localhost:/` +- Log in (instructions above) +- `agent-browser set viewport ` +- Set theme via the "Toggle theme" button (check the root element's class to confirm) + +Use a unique --session-name per agent so browser sessions don't collide. +Return the list of screenshots you saved. +``` + +The `--session-name` must be unique per agent — otherwise multiple agents fight over the same browser profile and cookies. + +## Waiting for the page to be ready (`wait-for-ready`) + +Screenshots taken too early capture skeletons, the Next.js dev "Compiling..." HUD, or a half-hydrated tree. Don't rely on `networkidle` alone. Run this gate after every navigation: + +```bash +agent-browser eval " + (async () => { + const deadline = Date.now() + 30000; // hard ceiling: 30s + const stable = { count: 0, lastHTML: '' }; + + while (Date.now() < deadline) { + const ready = (() => { + if (document.readyState !== 'complete') return false; + + // Next.js dev compile indicator — id varies by version; cover both + if (document.querySelector('#__next-build-watcher, [data-nextjs-dialog]')) return false; + if (document.body.innerText.match(/^Compiling\b|building\.\.\./i)) return false; + + // Common skeleton / loading affordances + if (document.querySelector('[data-loading=\"true\"], [aria-busy=\"true\"], .skeleton, [data-state=\"loading\"]')) return false; + + // Tailwind animate-pulse is the de-facto skeleton signal in this codebase + if (document.querySelector('.animate-pulse')) return false; + + // Pending fetches the app exposes (optional — harmless if undefined) + if (window.__pendingFetches > 0) return false; + + return true; + })(); + + // Two consecutive stable reads of the rendered HTML body = settled + const html = document.body.innerHTML.length; + if (ready && html === stable.lastHTML) stable.count++; + else { stable.count = 0; stable.lastHTML = html; } + if (stable.count >= 2) return { ok: true, elapsed: Date.now() - (deadline - 30000) }; + + await new Promise(r => setTimeout(r, 250)); + } + return { ok: false, reason: 'timeout', html: document.body.innerHTML.slice(0, 200) }; + })() +" +``` + +Return contract: + +- `{ ok: true, elapsed }` — proceed to screenshot. +- `{ ok: false, reason: 'timeout', html }` — log it, then *still screenshot* (a 30s-stuck page is worth capturing as-is rather than failing the whole pass), but mark the route in the agent's return value so the main agent can decide whether to retry. + +Why the two-consecutive-stable-reads check: a single `ready` flicker can land between a skeleton disappearing and the real content swapping in. Two consecutive 250ms ticks with identical body HTML length means the DOM has actually settled. + +Tune the skeleton selector list for your codebase. The Stack Auth dashboard uses Tailwind `.animate-pulse` extensively for loading rows — that's the highest-signal one. Inspect the diff or the running app once and add any project-specific loading markers (`Spinner` component class, etc.) to the selector list in the subagent prompt rather than guessing. + +After `wait-for-ready` returns `ok`, still sleep ~300ms before screenshotting so any final animations (slide-in, fade, etc.) land on their resting frame. + +## Red-border highlight injector (`pr-visual-highlight`) + +Use this on "after" captures to outline the new UI introduced by the PR. The Phase 1 scope file should list selectors per route — pass them as a JS array. + +```bash +agent-browser eval "(() => { + const selectors = $SELECTORS_JSON; // e.g. ['[data-testid=foo]', 'section:has(> h2)'] + document.getElementById('pr-visual-highlight')?.remove(); + const style = document.createElement('style'); + style.id = 'pr-visual-highlight'; + style.textContent = selectors.map(s => \`\${s} { outline: 3px solid #ef4444 !important; outline-offset: 2px !important; border-radius: 6px; box-shadow: 0 0 0 1px rgba(239,68,68,0.25) !important; }\`).join('\n'); + document.head.appendChild(style); + return Array.from(document.querySelectorAll(selectors.join(','))).length; +})()" +``` + +Notes: + +- Use `outline`, not `border`. `border` shifts layout and breaks visual parity with the "before" shot. `outline` paints outside the box and changes nothing else. +- Bright red `#ef4444` (Tailwind `red-500`) reads on both light and dark themes. Don't theme-switch the color — consistency matters more than contrast finesse. +- `!important` on the outline overrides any element-level outline that the app sets on focus/hover. +- The injector returns a count of matched elements. If it returns `0`, your selectors are stale — log a warning and skip highlight for that route rather than ship an "after" shot that looks identical to "before". +- Always remove the `