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
2026-03-22 12:09:23 +03:00 · 2026-03-01 20:38:58 +00:00
parent 13e8a0121f
commit d20def9f66
2 changed files with 159 additions and 60 deletions
--- 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,
--- 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 += '<details>\n<summary>Unchanged pages</summary>\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 `<div class="no-image">${alt} not available</div>`;
+    }
+    const data = readFileSync(filePath);
+    return `<img src="data:image/png;base64,${data.toString('base64')}" alt="${alt}" loading="lazy" />`;
+  }
+
+  let html = `<!DOCTYPE html>
+<html lang="en">
+<head>
+<meta charset="UTF-8">
+<meta name="viewport" content="width=device-width, initial-scale=1.0">
+<title>Visual Review</title>
+<style>
+  * { box-sizing: border-box; margin: 0; padding: 0; }
+  body { font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif;
+         background: #0d1117; color: #e6edf3; padding: 32px; line-height: 1.5; }
+  .container { max-width: 1800px; margin: 0 auto; }
+  h1 { font-size: 24px; border-bottom: 1px solid #30363d; padding-bottom: 12px; margin-bottom: 24px; }
+  .summary { color: #8b949e; margin-bottom: 32px; font-size: 16px; }
+  .scenario { margin-bottom: 40px; border: 1px solid #30363d; border-radius: 8px; overflow: hidden; }
+  .scenario-header { background: #161b22; padding: 12px 16px; display: flex; align-items: center; gap: 12px; }
+  .scenario-header h2 { font-size: 16px; font-weight: 600; }
+  .badge { display: inline-block; padding: 2px 10px; border-radius: 12px; font-size: 12px; font-weight: 500; }
+  .badge-changed { background: #da363380; color: #f85149; }
+  .badge-new { background: #1f6feb80; color: #58a6ff; }
+  .badge-removed { background: #6e767e80; color: #8b949e; }
+  .grid { display: grid; grid-template-columns: 1fr 1fr 1fr; gap: 1px; background: #30363d; }
+  .grid-cell { background: #0d1117; }
+  .grid-label { text-align: center; padding: 8px; font-size: 13px; color: #8b949e; font-weight: 600;
+                background: #161b22; text-transform: uppercase; letter-spacing: 0.5px; }
+  .grid-cell img { width: 100%; display: block; }
+  .no-image { padding: 40px; text-align: center; color: #484f58; font-style: italic; }
+  .unchanged-section { margin-top: 32px; color: #8b949e; }
+  .unchanged-section summary { cursor: pointer; font-size: 14px; }
+  .unchanged-section ul { margin-top: 8px; padding-left: 24px; }
+  .unchanged-section li { font-size: 14px; margin: 4px 0; }
+</style>
+</head>
+<body>
+<div class="container">
+<h1>Visual Review</h1>
+`;
+
+  if (changed.length === 0) {
+    html += '<p class="summary">No visual changes detected in the affected pages.</p>';
+  } else {
+    html += `<p class="summary">Found <strong>${changed.length}</strong> page(s) with visual changes`;
+    if (unchanged.length > 0) {
+      html += ` (${unchanged.length} unchanged)`;
+    }
+    html += '.</p>\n';
+
+    for (const result of changed) {
+      html += '<div class="scenario">\n<div class="scenario-header">\n';
+      html += `<h2>${result.name}</h2>\n`;
+
+      if (!result.baseExists) {
+        html += '<span class="badge badge-new">New</span>\n';
+        html += '</div>\n';
+        html += `<div style="padding: 16px;">${imgTag(result.prImagePath, 'PR')}</div>\n`;
+        html += '</div>\n';
+        continue;
+      }
+
+      if (!result.prExists) {
+        html += '<span class="badge badge-removed">Removed</span>\n';
+        html += '</div>\n</div>\n';
+        continue;
+      }
+
+      html += `<span class="badge badge-changed">${result.changePercent.toFixed(1)}% changed</span>\n`;
+      html += '</div>\n';
+      html += '<div class="grid">\n';
+      html += `<div class="grid-cell"><div class="grid-label">Base</div>${imgTag(result.baseImagePath, 'Base')}</div>\n`;
+      html += `<div class="grid-cell"><div class="grid-label">PR</div>${imgTag(result.prImagePath, 'PR')}</div>\n`;
+      html += `<div class="grid-cell"><div class="grid-label">Diff</div>${imgTag(result.diffImagePath, 'Diff')}</div>\n`;
+      html += '</div>\n</div>\n';
+    }
+  }
+
+  if (unchanged.length > 0) {
+    html += '<div class="unchanged-section">\n<details>\n<summary>Unchanged pages</summary>\n<ul>\n';
+    for (const result of unchanged) {
+      html += `<li>${result.name}</li>\n`;
+    }
+    html += '</ul>\n</details>\n</div>\n';
+  }
+
+  html += '</div>\n</body>\n</html>';
+  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 <base-dir> <pr-dir> <output-dir> [image-base-url]');
+    console.log('Usage: compare.ts <base-dir> <pr-dir> <output-dir>');
    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}`);
 }