Fix visual failure reporting and unstable captures

.github/workflows/_static-checks.yaml

@@ -34,8 +34,8 @@ jobs:
       - name: Test release PR body renderer
         run: node --test scripts/upstream-release/render-pr-body.test.mjs
 
-      - name: Test screenshot summary updater
-        run: node --test scripts/update-pr-screenshot-summary.test.mjs
+      - name: Test screenshot summary helpers
+        run: node --test scripts/update-pr-screenshot-summary.test.mjs scripts/pr-screenshot-workflow.test.mjs
 
       - name: Test Helm reference generator
         run: node --test scripts/upstream-release/generate-helm-reference.test.mjs

.github/workflows/pr-screenshot-summary.yaml

@@ -95,16 +95,34 @@ jobs:
           JOBS_FILE="$RUNNER_TEMP/jobs.json"
           ARTIFACTS_FILE="$RUNNER_TEMP/artifacts.json"
           gh api "repos/$REPO/actions/runs/$RUN_ID/jobs?per_page=100" > "$JOBS_FILE"
-          gh api "repos/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100" > "$ARTIFACTS_FILE"
           CONCLUSION="$(jq -r '[.jobs[] | select(.name == "Visual regression / Playwright visual snapshots")][0].conclusion // "missing"' "$JOBS_FILE")"
           JOB_URL="$(jq -r '[.jobs[] | select(.name == "Visual regression / Playwright visual snapshots")][0].html_url // ""' "$JOBS_FILE")"
-          jq '.artifacts |= ([.artifacts[] | select(.name == "visual-failure-details" and .expired == false)] + [.artifacts[] | select(.name == "playwright-report" and .expired == false)])' "$ARTIFACTS_FILE" > "$ARTIFACTS_FILE.selected"
+          # Preserve the conclusion so the always() comment step can still
+          # post a fallback if artifact discovery or parsing fails.
+          {
+            echo "conclusion=$CONCLUSION"
+            echo "job_url=$JOB_URL"
+          } >> "$GITHUB_OUTPUT"
+          JOB_STARTED_AT="$(jq -r '[.jobs[] | select(.name == "Visual regression / Playwright visual snapshots")][0].started_at // ""' "$JOBS_FILE")"
+          JOB_COMPLETED_AT="$(jq -r '[.jobs[] | select(.name == "Visual regression / Playwright visual snapshots")][0].completed_at // ""' "$JOBS_FILE")"
+          gh api "repos/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100" > "$ARTIFACTS_FILE"
+          # A run's artifacts include prior attempts. GitHub returns these
+          # timestamps in UTC; accept only files created during this job.
+          jq --arg started_at "$JOB_STARTED_AT" --arg completed_at "$JOB_COMPLETED_AT" '
+            .artifacts = [.artifacts[] | select(
+              .expired == false and
+              $started_at != "" and $completed_at != "" and
+              .created_at >= $started_at and .created_at <= $completed_at
+            )] |
+            .artifacts = (
+              [.artifacts[] | select(.name == "visual-failure-details")] +
+              [.artifacts[] | select(.name == "playwright-report")]
+            )
+          ' "$ARTIFACTS_FILE" > "$ARTIFACTS_FILE.selected"
           mv "$ARTIFACTS_FILE.selected" "$ARTIFACTS_FILE"
           ARTIFACT_ID="$(jq -r '.artifacts[0].id // ""' "$ARTIFACTS_FILE")"
           ARTIFACT_SIZE="$(jq -r '.artifacts[0].size_in_bytes // "0"' "$ARTIFACTS_FILE")"
           {
-            echo "conclusion=$CONCLUSION"
-            echo "job_url=$JOB_URL"
             echo "artifact_id=$ARTIFACT_ID"
             echo "artifact_size=$ARTIFACT_SIZE"
           } >> "$GITHUB_OUTPUT"

scripts/pr-screenshot-workflow.test.mjs

@@ -0,0 +1,184 @@
+// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+import assert from 'node:assert/strict';
+import { spawnSync } from 'node:child_process';
+import fs from 'node:fs';
+import os from 'node:os';
+import path from 'node:path';
+import test from 'node:test';
+import { parse } from 'yaml';
+
+const workflow = parse(
+  fs.readFileSync(
+    new URL('../.github/workflows/pr-screenshot-summary.yaml', import.meta.url),
+    'utf8'
+  )
+);
+const resolveStep = workflow.jobs['update-description'].steps.find(
+  (step) => step.id === 'visual'
+);
+
+function resolveArtifacts(t, artifacts, { unavailable = false } = {}) {
+  const root = fs.mkdtempSync(path.join(os.tmpdir(), 'visual-job-'));
+  t.after(() => fs.rmSync(root, { recursive: true, force: true }));
+  const bin = path.join(root, 'bin');
+  fs.mkdirSync(bin);
+  fs.writeFileSync(
+    path.join(root, 'jobs.json.fixture'),
+    JSON.stringify({
+      jobs: [
+        {
+          name: 'Static checks / Lint and format checks',
+          conclusion: 'success',
+        },
+        {
+          name: 'Visual regression / Playwright visual snapshots',
+          conclusion: 'failure',
+          html_url: 'https://github.com/example/docs/actions/runs/1/job/2',
+          started_at: '2026-10-30T17:00:00Z',
+          completed_at: '2026-10-30T17:05:00Z',
+        },
+      ],
+    })
+  );
+  fs.writeFileSync(
+    path.join(root, 'artifacts.json.fixture'),
+    typeof artifacts === 'string' ? artifacts : JSON.stringify({ artifacts })
+  );
+  fs.writeFileSync(
+    path.join(bin, 'gh'),
+    `#!/bin/sh
+case "$2" in
+  */jobs?*) cat "$RUNNER_TEMP/jobs.json.fixture" ;;
+  */artifacts?*)
+    if [ "$ARTIFACTS_UNAVAILABLE" = true ]; then exit 1; fi
+    cat "$RUNNER_TEMP/artifacts.json.fixture" ;;
+  *) exit 1 ;;
+esac
+`,
+    { mode: 0o755 }
+  );
+  const outputFile = path.join(root, 'outputs');
+  const result = spawnSync('bash', ['-eo', 'pipefail', '-c', resolveStep.run], {
+    encoding: 'utf8',
+    env: {
+      ...process.env,
+      PATH: `${bin}${path.delimiter}${process.env.PATH}`,
+      RUNNER_TEMP: root,
+      GITHUB_OUTPUT: outputFile,
+      REPO: 'example/docs',
+      RUN_ID: '1',
+      ARTIFACTS_UNAVAILABLE: String(unavailable),
+    },
+  });
+  const outputs = Object.fromEntries(
+    fs
+      .readFileSync(outputFile, 'utf8')
+      .trim()
+      .split('\n')
+      .map((line) => {
+        const separator = line.indexOf('=');
+        return [line.slice(0, separator), line.slice(separator + 1)];
+      })
+  );
+  assert.equal(outputs.conclusion, 'failure');
+  assert.equal(
+    outputs.job_url,
+    'https://github.com/example/docs/actions/runs/1/job/2'
+  );
+  return { result, outputs };
+}
+
+const fullReport = {
+  name: 'playwright-report',
+  expired: false,
+  id: 10,
+  size_in_bytes: 55_544_517,
+  created_at: '2026-10-30T17:04:00Z',
+};
+const compactReport = {
+  name: 'visual-failure-details',
+  expired: false,
+  id: 20,
+  size_in_bytes: 21_006,
+  created_at: '2026-10-30T17:03:00Z',
+};
+
+test('selects compact details ahead of a large full report', (t) => {
+  const { result, outputs } = resolveArtifacts(t, [fullReport, compactReport]);
+  assert.equal(result.status, 0, result.stderr);
+  assert.equal(outputs.artifact_id, '20');
+  assert.equal(outputs.artifact_size, '21006');
+});
+
+test('falls back to the full report when compact details are expired', (t) => {
+  const { result, outputs } = resolveArtifacts(t, [
+    { ...compactReport, expired: true },
+    fullReport,
+  ]);
+  assert.equal(result.status, 0, result.stderr);
+  assert.equal(outputs.artifact_id, '10');
+});
+
+test('preserves the failure conclusion when no artifact is available', (t) => {
+  const { result, outputs } = resolveArtifacts(t, []);
+  assert.equal(result.status, 0, result.stderr);
+  assert.equal(outputs.artifact_id, '');
+  assert.equal(outputs.artifact_size, '0');
+});
+
+test('preserves the failure conclusion when artifact discovery fails', (t) => {
+  const { result, outputs } = resolveArtifacts(t, [], { unavailable: true });
+  assert.notEqual(result.status, 0);
+  assert.equal(outputs.artifact_id, undefined);
+});
+
+test('preserves the failure conclusion when artifact parsing fails', (t) => {
+  const { result, outputs } = resolveArtifacts(t, '{malformed');
+  assert.notEqual(result.status, 0);
+  assert.equal(outputs.artifact_id, undefined);
+});
+
+test('uses the current full report ahead of prior-attempt compact details', (t) => {
+  const { result, outputs } = resolveArtifacts(t, [
+    { ...compactReport, created_at: '2026-10-30T16:55:00Z' },
+    fullReport,
+  ]);
+  assert.equal(result.status, 0, result.stderr);
+  assert.equal(outputs.artifact_id, '10');
+});
+
+test('uses the fallback when a rerun produces no new artifacts', (t) => {
+  const { result, outputs } = resolveArtifacts(t, [
+    { ...compactReport, created_at: '2026-10-30T16:55:00Z' },
+    { ...fullReport, created_at: '2026-10-30T16:56:00Z' },
+  ]);
+  assert.equal(result.status, 0, result.stderr);
+  assert.equal(outputs.artifact_id, '');
+});
+
+test('selects current compact details when older compact details appear first', (t) => {
+  const { result, outputs } = resolveArtifacts(t, [
+    { ...compactReport, id: 19, created_at: '2026-10-30T16:55:00Z' },
+    compactReport,
+  ]);
+  assert.equal(result.status, 0, result.stderr);
+  assert.equal(outputs.artifact_id, '20');
+});
+
+test('rejects artifacts uploaded after the current visual job finished', (t) => {
+  const { result, outputs } = resolveArtifacts(t, [
+    { ...compactReport, created_at: '2026-10-30T17:10:00Z' },
+  ]);
+  assert.equal(result.status, 0, result.stderr);
+  assert.equal(outputs.artifact_id, '');
+});
+
+test('rejects artifacts with no creation timestamp', (t) => {
+  const { result, outputs } = resolveArtifacts(t, [
+    { ...compactReport, created_at: undefined },
+  ]);
+  assert.equal(result.status, 0, result.stderr);
+  assert.equal(outputs.artifact_id, '');
+});

tests/visual/pages.spec.ts

@@ -2,7 +2,11 @@
 // SPDX-License-Identifier: Apache-2.0
 
 import { expect, test, type Page } from '@playwright/test';
-import { captureBothThemes, captureElementBothThemes } from './fixtures';
+import {
+  captureBothThemes,
+  captureColorScheme,
+  captureElementBothThemes,
+} from './fixtures';
 
 /**
  * The documented representative-page matrix for issue #1163. Keep this
@@ -32,16 +36,20 @@ test('home page', async ({ page }, testInfo) => {
   await captureBothThemes(page, testInfo, 'Home page');
 });
 
-test('theme preview page', async ({ page }, testInfo) => {
-  await gotoSuccessful(page, '/theme-preview');
-  // Mermaid diagrams render client-side and asynchronously — wait for
-  // both of the page's diagrams to finish before capturing, or the
-  // snapshot flakes between "still rendering" and "done".
-  await expect(page.locator('.docusaurus-mermaid-container svg')).toHaveCount(
-    2
-  );
-  await captureBothThemes(page, testInfo, 'Theme preview page');
-});
+for (const scheme of ['light', 'dark'] as const) {
+  test(`theme preview page - ${scheme}`, async ({ page }, testInfo) => {
+    // Mermaid rerenders asynchronously when the theme changes. Start each
+    // page in its final theme so full-page capture cannot measure the page
+    // while a theme switch temporarily removes a diagram.
+    await page.emulateMedia({ colorScheme: scheme });
+    await gotoSuccessful(page, '/theme-preview');
+    await expect(page.locator('html')).toHaveAttribute('data-theme', scheme);
+    await expect(page.locator('.docusaurus-mermaid-container svg')).toHaveCount(
+      2
+    );
+    await captureColorScheme(page, testInfo, 'Theme preview page', scheme);
+  });
+}
 
 for (const { section, path } of NAV_PAGES) {
   test(`navigation loads - ${section}`, async ({ page }) => {

tests/visual/pages.spec.ts-snapshots/theme-preview-page-dark-desktop-linux.json

@@ -1,12 +1,12 @@
 {
   "titlePath": [
     "pages.spec.ts",
-    "theme preview page"
+    "theme preview page - dark"
   ],
   "testFile": "tests/visual/pages.spec.ts",
   "url": {
     "origin": "http://localhost:3005",
     "path": "/theme-preview"
   },
-  "reproCommand": "npx playwright test tests/visual/pages.spec.ts -g \"theme preview page\""
+  "reproCommand": "npx playwright test tests/visual/pages.spec.ts -g \"theme preview page - dark\""
 }

tests/visual/pages.spec.ts-snapshots/theme-preview-page-light-desktop-linux.json

@@ -1,12 +1,12 @@
 {
   "titlePath": [
     "pages.spec.ts",
-    "theme preview page"
+    "theme preview page - light"
   ],
   "testFile": "tests/visual/pages.spec.ts",
   "url": {
     "origin": "http://localhost:3005",
     "path": "/theme-preview"
   },
-  "reproCommand": "npx playwright test tests/visual/pages.spec.ts -g \"theme preview page\""
+  "reproCommand": "npx playwright test tests/visual/pages.spec.ts -g \"theme preview page - light\""
 }