From feacf9b1349763afed824ce66b6386759cd02170 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 26 Feb 2026 15:14:43 +0000 Subject: [PATCH] fix: improve screenshot waiting, add inline images to PR comments - Increase waitForSelector timeout from 5s to 15s for slower page loads - Add explicit wait for loading spinners to disappear before screenshot - Push screenshot images to a temporary branch for inline display - Read report.md from compare.ts with raw.githubusercontent.com URLs - Accept optional image base URL in compare.ts CLI - Upgrade contents permission to write for pushing screenshot branch https://claude.ai/code/session_01XSTqDJXuR4jaLN7SGm3uES --- .github/workflows/visual-review.yml | 87 ++++++++++------------------ e2e/src/screenshots/compare.ts | 6 +- e2e/src/screenshots/run-scenarios.ts | 16 +++-- 3 files changed, 45 insertions(+), 64 deletions(-) diff --git a/.github/workflows/visual-review.yml b/.github/workflows/visual-review.yml index abfc9d360e..c340604224 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: read + contents: write pull-requests: write defaults: run: @@ -244,11 +244,13 @@ jobs: 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: | pnpm exec tsx src/screenshots/compare.ts \ "$WORKSPACE_DIR/screenshots/base" \ "$WORKSPACE_DIR/screenshots/pr" \ - "$WORKSPACE_DIR/screenshots/diff" + "$WORKSPACE_DIR/screenshots/diff" \ + "$IMAGE_BASE_URL" - name: Upload screenshot artifacts if: steps.routes.outputs.has_routes == 'true' @@ -258,72 +260,45 @@ jobs: path: screenshots/ retention-days: 14 + - name: Push screenshots to review branch + 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: . + - name: Post comparison results if: steps.routes.outputs.has_routes == 'true' uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 - env: - WORKSPACE_DIR: ${{ github.workspace }} with: github-token: ${{ steps.token.outputs.token }} script: | const fs = require('fs'); const path = require('path'); - const workspaceDir = process.env.WORKSPACE_DIR; - const diffDir = path.join(workspaceDir, 'screenshots', 'diff'); + // Read from /tmp copy since git push step may have cleared the workspace + const reportPath = path.join('/tmp', 'vr-screenshots', 'diff', 'report.md'); - // Read comparison results - let results; + let body; try { - results = JSON.parse(fs.readFileSync(path.join(diffDir, 'results.json'), 'utf8')); + body = fs.readFileSync(reportPath, 'utf8'); } catch { - await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - body: '## Visual Review\n\nScreenshot comparison failed. Check the workflow artifacts for details.' - }); - return; - } - - const changed = results.filter(r => r.changePercent > 0.1); - const unchanged = results.filter(r => r.changePercent <= 0.1); - - let body = '## Visual Review\n\n'; - - if (changed.length === 0) { - body += 'No visual changes detected in the affected pages.\n'; - } else { - body += `Found **${changed.length}** page(s) with visual changes`; - if (unchanged.length > 0) { - body += ` (${unchanged.length} unchanged)`; - } - body += '.\n\n'; - body += '> **Note:** Download the `visual-review-screenshots` artifact to view full-resolution before/after/diff images.\n\n'; - - for (const result of changed) { - body += `### ${result.name}\n\n`; - - if (!result.baseExists) { - body += '**New page** — no base screenshot to compare against.\n\n'; - continue; - } - - if (!result.prExists) { - body += '**Removed page** — no longer exists in the PR.\n\n'; - continue; - } - - body += `Change: **${result.changePercent.toFixed(1)}%** of pixels (${result.diffPixels.toLocaleString()} pixels differ)\n\n`; - } - } - - if (unchanged.length > 0) { - body += '
\nUnchanged pages (' + unchanged.length + ')\n\n'; - for (const result of unchanged) { - body += `- ${result.name}\n`; - } - body += '\n
\n'; + body = '## Visual Review\n\nScreenshot comparison failed. Check the workflow artifacts for details.'; } // Find and update existing comment or create new one diff --git a/e2e/src/screenshots/compare.ts b/e2e/src/screenshots/compare.ts index 2819b26964..4cda983b5a 100644 --- a/e2e/src/screenshots/compare.ts +++ b/e2e/src/screenshots/compare.ts @@ -225,10 +225,10 @@ export function generateMarkdownReport(results: ComparisonResult[], artifactUrl: // CLI usage if (process.argv[1]?.endsWith('compare.ts') || process.argv[1]?.endsWith('compare.js')) { - const [baseDir, prDir, outputDir] = process.argv.slice(2); + const [baseDir, prDir, outputDir, imageBaseUrl] = process.argv.slice(2); if (!baseDir || !prDir || !outputDir) { - console.log('Usage: compare.ts '); + console.log('Usage: compare.ts [image-base-url]'); process.exit(1); } @@ -245,7 +245,7 @@ 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, '.'); + const report = generateMarkdownReport(results, imageBaseUrl || '.'); const reportPath = join(resolve(outputDir), 'report.md'); writeFileSync(reportPath, report); console.log(`\nReport written to: ${reportPath}`); diff --git a/e2e/src/screenshots/run-scenarios.ts b/e2e/src/screenshots/run-scenarios.ts index 08989d4673..d951309335 100644 --- a/e2e/src/screenshots/run-scenarios.ts +++ b/e2e/src/screenshots/run-scenarios.ts @@ -103,16 +103,22 @@ for (const scenario of allScenarios) { // Wait for specific selector if specified if (scenario.waitForSelector) { try { - await page.waitForSelector(scenario.waitForSelector, { timeout: 5000 }); + await page.waitForSelector(scenario.waitForSelector, { timeout: 15_000 }); } catch { - // Continue with screenshot even if selector doesn't appear + console.warn(`Selector ${scenario.waitForSelector} not found for ${scenario.name}, continuing...`); } } + // Wait for loading spinners to disappear + await page + .waitForFunction( + () => document.querySelectorAll('[data-testid="loading-spinner"]').length === 0, + { timeout: 10_000 }, + ) + .catch(() => {}); + // Wait for animations/transitions to settle - if (scenario.settleTime) { - await page.waitForTimeout(scenario.settleTime); - } + await page.waitForTimeout(scenario.settleTime ?? 500); // Take the screenshot await page.screenshot({