From d20def9f6690e35438d18479930a4e46b09fde69 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Mar 2026 20:38:58 +0000 Subject: [PATCH] fix: base screenshots + switch to artifact-based image hosting - Fix blank base screenshots by aggressively killing the preview server between PR and base steps (fuser -k on port 4173) and clearing the SvelteKit build cache before rebuilding the base version - Replace git branch push with GitHub Actions artifact upload: - Upload full screenshots as a zipped artifact for download - Upload an HTML report with embedded base64 images as a non-zipped artifact (archive: false) for direct browser viewing - Update compare.ts to generate both a text-only markdown summary (for the PR comment) and a self-contained HTML visual comparison - Downgrade permissions from contents:write to contents:read since we no longer push to the repository https://claude.ai/code/session_01XSTqDJXuR4jaLN7SGm3uES --- .github/workflows/visual-review.yml | 64 ++++++------ e2e/src/screenshots/compare.ts | 155 ++++++++++++++++++++++------ 2 files changed, 159 insertions(+), 60 deletions(-) diff --git a/.github/workflows/visual-review.yml b/.github/workflows/visual-review.yml index 0d1bf99dff..dc333cd317 100644 --- a/.github/workflows/visual-review.yml +++ b/.github/workflows/visual-review.yml @@ -14,7 +14,7 @@ jobs: (github.event.action == 'labeled' && github.event.label.name == 'visual-review') || (github.event.action == 'synchronize' && contains(github.event.pull_request.labels.*.name, 'visual-review')) permissions: - contents: write + contents: read pull-requests: write defaults: run: @@ -188,8 +188,12 @@ jobs: # Run screenshot tests pnpm exec playwright test --config playwright.screenshot.config.ts || true - # Stop the preview server + # Stop the preview server and all children (pnpm spawns vite as child) kill $SERVER_PID 2>/dev/null || true + sleep 1 + # Ensure port is fully released — kill any lingering vite process + fuser -k 4173/tcp 2>/dev/null || true + sleep 1 # === Screenshot base version === # Disable pnpm's verifyDepsBeforeRun for all base steps since the base @@ -201,6 +205,8 @@ jobs: run: | # Restore web directory from base branch git checkout "$BASE_SHA" -- web/ open-api/typescript-sdk/ i18n/ || true + # Clear SvelteKit build cache to avoid stale artifacts from the PR build + rm -rf web/.svelte-kit web/build working-directory: . - name: Build SDK (base) @@ -230,6 +236,10 @@ jobs: SCREENSHOT_BASE_URL: http://127.0.0.1:4173 PNPM_VERIFY_DEPS_BEFORE_RUN: 'false' run: | + # Kill any process still on port 4173 from the PR step + fuser -k 4173/tcp 2>/dev/null || true + sleep 1 + # Start the preview server in background cd ../web && pnpm preview --port 4173 --host 127.0.0.1 & SERVER_PID=$! @@ -248,6 +258,7 @@ jobs: # Stop the preview server kill $SERVER_PID 2>/dev/null || true + fuser -k 4173/tcp 2>/dev/null || true - name: Restore PR source if: steps.routes.outputs.has_routes == 'true' @@ -260,60 +271,46 @@ jobs: # === Compare and report === - name: Compare screenshots if: steps.routes.outputs.has_routes == 'true' - id: compare env: WORKSPACE_DIR: ${{ github.workspace }} - IMAGE_BASE_URL: https://raw.githubusercontent.com/${{ github.repository }}/visual-review/pr-${{ github.event.pull_request.number }} run: | # Ensure directories exist even if base screenshots were skipped mkdir -p "$WORKSPACE_DIR/screenshots/base" "$WORKSPACE_DIR/screenshots/pr" "$WORKSPACE_DIR/screenshots/diff" pnpm exec tsx src/screenshots/compare.ts \ "$WORKSPACE_DIR/screenshots/base" \ "$WORKSPACE_DIR/screenshots/pr" \ - "$WORKSPACE_DIR/screenshots/diff" \ - "$IMAGE_BASE_URL" + "$WORKSPACE_DIR/screenshots/diff" - name: Upload screenshot artifacts if: steps.routes.outputs.has_routes == 'true' - uses: actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f # v6.0.0 + uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0 with: name: visual-review-screenshots path: screenshots/ retention-days: 14 - - name: Push screenshots to review branch + - name: Upload HTML report if: steps.routes.outputs.has_routes == 'true' - env: - REVIEW_BRANCH: visual-review/pr-${{ github.event.pull_request.number }} - GH_TOKEN: ${{ steps.token.outputs.token }} - run: | - cd "$GITHUB_WORKSPACE" - # Save screenshots before git operations - cp -r screenshots /tmp/vr-screenshots - git config user.name "github-actions[bot]" - git config user.email "41898282+github-actions[bot]@users.noreply.github.com" - git checkout --orphan "$REVIEW_BRANCH" - git rm -rf . 2>/dev/null || true - cp -r /tmp/vr-screenshots/base . 2>/dev/null || true - cp -r /tmp/vr-screenshots/pr . 2>/dev/null || true - cp -r /tmp/vr-screenshots/diff . 2>/dev/null || true - git add base/ pr/ diff/ 2>/dev/null || true - git commit -m "Visual review screenshots for PR #${{ github.event.pull_request.number }}" --allow-empty - git remote set-url origin "https://x-access-token:${GH_TOKEN}@github.com/${{ github.repository }}.git" - git push -f origin "$REVIEW_BRANCH" - working-directory: . + id: html-report + uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0 + with: + path: screenshots/diff/visual-review.html + archive: false + retention-days: 14 - name: Post comparison results if: steps.routes.outputs.has_routes == 'true' uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 + env: + REPORT_URL: ${{ steps.html-report.outputs.artifact-url }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} with: github-token: ${{ steps.token.outputs.token }} script: | const fs = require('fs'); const path = require('path'); - // Read from /tmp copy since git push step may have cleared the workspace - const reportPath = path.join('/tmp', 'vr-screenshots', 'diff', 'report.md'); + const reportPath = path.join(process.env.GITHUB_WORKSPACE, 'screenshots', 'diff', 'report.md'); let body; try { @@ -322,6 +319,15 @@ jobs: body = '## Visual Review\n\nScreenshot comparison failed. Check the workflow artifacts for details.'; } + // Append links to the HTML report artifact and workflow run + const reportUrl = process.env.REPORT_URL; + const runUrl = process.env.RUN_URL; + body += '\n---\n'; + if (reportUrl) { + body += `[View full visual comparison](${reportUrl}) | `; + } + body += `[Download all screenshots](${runUrl}#artifacts)\n`; + // Find and update existing comment or create new one const comments = await github.rest.issues.listComments({ owner: context.repo.owner, diff --git a/e2e/src/screenshots/compare.ts b/e2e/src/screenshots/compare.ts index 4cda983b5a..6987d02793 100644 --- a/e2e/src/screenshots/compare.ts +++ b/e2e/src/screenshots/compare.ts @@ -174,8 +174,8 @@ function normalizeImage(png: PNG, targetWidth: number, targetHeight: number): Ui return data; } -/** Generate a markdown report for PR comment. */ -export function generateMarkdownReport(results: ComparisonResult[], artifactUrl: string): string { +/** Generate a text-only markdown summary for the PR comment. */ +export function generateMarkdownReport(results: ComparisonResult[]): string { const changed = results.filter((r) => r.changePercent > 0.1); const unchanged = results.filter((r) => r.changePercent <= 0.1); @@ -190,28 +190,21 @@ export function generateMarkdownReport(results: ComparisonResult[], artifactUrl: } md += '.\n\n'; + md += '| Page | Status | Change |\n'; + md += '|------|--------|--------|\n'; + for (const result of changed) { - md += `### ${result.name}\n\n`; - if (!result.baseExists) { - md += '**New page** (no base screenshot to compare)\n\n'; - md += `| PR |\n|---|\n| ![${result.name} PR](${artifactUrl}/${result.name}.png) |\n\n`; - continue; + md += `| ${result.name} | New | - |\n`; + } else if (!result.prExists) { + md += `| ${result.name} | Removed | - |\n`; + } else { + md += `| ${result.name} | Changed | ${result.changePercent.toFixed(1)}% |\n`; } - - if (!result.prExists) { - md += '**Removed page** (no PR screenshot)\n\n'; - continue; - } - - md += `Change: **${result.changePercent.toFixed(1)}%** (${result.diffPixels.toLocaleString()} pixels)\n\n`; - md += '| Base | PR | Diff |\n'; - md += '|------|-------|------|\n'; - md += `| ![base](${artifactUrl}/base/${result.name}.png) `; - md += `| ![pr](${artifactUrl}/pr/${result.name}.png) `; - md += `| ![diff](${artifactUrl}/diff/${result.name}-diff.png) |\n\n`; } + md += '\n'; + if (unchanged.length > 0) { md += '
\nUnchanged pages\n\n'; for (const result of unchanged) { @@ -223,20 +216,116 @@ export function generateMarkdownReport(results: ComparisonResult[], artifactUrl: return md; } +/** Generate an HTML report with embedded base64 images for the artifact. */ +export function generateHtmlReport(results: ComparisonResult[]): string { + const changed = results.filter((r) => r.changePercent > 0.1); + const unchanged = results.filter((r) => r.changePercent <= 0.1); + + function imgTag(filePath: string | null, alt: string): string { + if (!filePath || !existsSync(filePath)) { + return `
${alt} not available
`; + } + const data = readFileSync(filePath); + return `${alt}`; + } + + let html = ` + + + + +Visual Review + + + +
+

Visual Review

+`; + + if (changed.length === 0) { + html += '

No visual changes detected in the affected pages.

'; + } else { + html += `

Found ${changed.length} page(s) with visual changes`; + if (unchanged.length > 0) { + html += ` (${unchanged.length} unchanged)`; + } + html += '.

\n'; + + for (const result of changed) { + html += '
\n
\n'; + html += `

${result.name}

\n`; + + if (!result.baseExists) { + html += 'New\n'; + html += '
\n'; + html += `
${imgTag(result.prImagePath, 'PR')}
\n`; + html += '
\n'; + continue; + } + + if (!result.prExists) { + html += 'Removed\n'; + html += '
\n\n'; + continue; + } + + html += `${result.changePercent.toFixed(1)}% changed\n`; + html += '\n'; + html += '
\n'; + html += `
Base
${imgTag(result.baseImagePath, 'Base')}
\n`; + html += `
PR
${imgTag(result.prImagePath, 'PR')}
\n`; + html += `
Diff
${imgTag(result.diffImagePath, 'Diff')}
\n`; + html += '
\n\n'; + } + } + + if (unchanged.length > 0) { + html += '
\n
\nUnchanged pages\n
    \n'; + for (const result of unchanged) { + html += `
  • ${result.name}
    • \n`; + } + html += '
\n
\n
\n'; + } + + html += '\n\n'; + return html; +} + // CLI usage if (process.argv[1]?.endsWith('compare.ts') || process.argv[1]?.endsWith('compare.js')) { - const [baseDir, prDir, outputDir, imageBaseUrl] = process.argv.slice(2); + const [baseDir, prDir, outputDir] = process.argv.slice(2); if (!baseDir || !prDir || !outputDir) { - console.log('Usage: compare.ts [image-base-url]'); + console.log('Usage: compare.ts '); process.exit(1); } - const results = compareScreenshots( - resolve(baseDir), - resolve(prDir), - resolve(outputDir), - ); + const resolvedOutputDir = resolve(outputDir); + const results = compareScreenshots(resolve(baseDir), resolve(prDir), resolvedOutputDir); console.log('\nComparison Results:'); console.log('=================='); @@ -245,13 +334,17 @@ if (process.argv[1]?.endsWith('compare.ts') || process.argv[1]?.endsWith('compar console.log(` ${r.name}: ${status} (${r.changePercent.toFixed(1)}%)`); } - const report = generateMarkdownReport(results, imageBaseUrl || '.'); - const reportPath = join(resolve(outputDir), 'report.md'); + const report = generateMarkdownReport(results); + const reportPath = join(resolvedOutputDir, 'report.md'); writeFileSync(reportPath, report); - console.log(`\nReport written to: ${reportPath}`); + console.log(`\nMarkdown report written to: ${reportPath}`); - // Also output results as JSON for CI - const jsonPath = join(resolve(outputDir), 'results.json'); + const htmlReport = generateHtmlReport(results); + const htmlPath = join(resolvedOutputDir, 'visual-review.html'); + writeFileSync(htmlPath, htmlReport); + console.log(`HTML report written to: ${htmlPath}`); + + const jsonPath = join(resolvedOutputDir, 'results.json'); writeFileSync(jsonPath, JSON.stringify(results, null, 2)); console.log(`Results JSON written to: ${jsonPath}`); }