fix(ci): don't let one failing scenario sink the whole visual preview

.github/scripts/web-shell-visuals-publish.mjs

@@ -17,7 +17,7 @@
  * `buildComment`) are exported and tested. The file also runs as a CLI for the
  * workflow:
  *   node web-shell-visuals-publish.mjs stage   <screenshotsDir> <gifsDir> <stageDir>
- *   node web-shell-visuals-publish.mjs comment <stageDir> <rawBase> <shortSha> <runUrl> <bodyFile> [changedPathsFile]
+ *   node web-shell-visuals-publish.mjs comment <stageDir> <rawBase> <shortSha> <runUrl> <bodyFile> [changedPathsFile] [renderStatusFile]
  */
 
 import {
@@ -190,15 +190,21 @@ const codePath = (p) => `\`${esc(String(p).replace(/[`\r\n]/g, ''))}\``;
 
 /**
  * Pure comment builder. `files` is the list of staged filenames (png + gif).
- * `ctx` is `{ rawBase, shortSha, runUrl, changedPaths }`, where `changedPaths`
- * is the PR's full changed-file list (used only to triage an empty preview).
- * Returns the markdown body.
+ * `ctx` is `{ rawBase, shortSha, runUrl, changedPaths, renderIncomplete }`:
+ * `changedPaths` is the PR's full changed-file list (used only to triage an
+ * empty preview), and `renderIncomplete` is true when >=1 scenario failed to
+ * render on the PR head — in which case a missing view may be a render failure,
+ * not "no change", and the comment must say so instead of a clean bill of
+ * health. Returns the markdown body.
  */
 export function buildComment(files, ctx = {}) {
   const rawBase = ctx.rawBase ?? '';
   const shortSha = ctx.shortSha ?? '';
   const runUrl = ctx.runUrl ?? '';
+  const renderIncomplete = ctx.renderIncomplete === true;
   const url = (name) => `${rawBase}/${encodeURIComponent(name)}`;
+  // A "see the run" pointer for the render-failure notes (omitted if unknown).
+  const runLink = runUrl ? ` — see the [workflow run](${esc(runUrl)})` : '';
 
   // Screenshots are before/after COMPOSITES (`<view>-<theme>.png`), one per
   // changed view+theme. The compositor already dropped unchanged views, so an
@@ -218,12 +224,30 @@ export function buildComment(files, ctx = {}) {
   out.push('#### Screenshots · before / after');
   out.push('');
   if (shots.length > 0) {
+    // Some views rendered. If others FAILED, say so first: the set below is
+    // partial, and a view a reviewer expects but doesn't see may have crashed
+    // rather than stayed unchanged.
+    if (renderIncomplete) {
+      out.push(
+        `⚠️ _One or more scenarios failed to render_ on this head, so this preview may be missing views${runLink}. The composites below are the scenarios that did render.`,
+      );
+      out.push('');
+    }
     for (const f of shots) {
       out.push(
         `<img src="${url(f)}" width="900" alt="${esc(f.replace(/\.png$/i, ''))} before/after">`,
       );
       out.push('');
     }
+  } else if (renderIncomplete) {
+    // No composites AND the render failed: this is NOT "no visual change". A
+    // scenario that times out or throws produces no image, so treat the empty
+    // set as a render failure — never the reassuring green check or the
+    // coverage-gap prompt, both of which would imply the render actually ran.
+    out.push(
+      `⚠️ _No preview: one or more scenarios failed to render_ on this head${runLink}. This is not "no visual change" — a scenario that times out or throws produces no image. Fix the failing scenario (or a genuine regression it caught) and the preview returns on the next push.`,
+    );
+    out.push('');
   } else {
     // An empty preview is ambiguous, so say WHICH of the two things it means.
     // "No view changed" is only a clean bill of health if nothing that shapes a
@@ -333,7 +357,15 @@ function stageCli(screenshotsDir, gifsDir, stageDir) {
   process.stdout.write(`${accepted.length}\n`);
 }
 
-function commentCli(stageDir, rawBase, shortSha, runUrl, bodyFile, pathsFile) {
+function commentCli(
+  stageDir,
+  rawBase,
+  shortSha,
+  runUrl,
+  bodyFile,
+  pathsFile,
+  renderStatusFile,
+) {
   let files = [];
   try {
     files = readdirSync(stageDir);
@@ -352,7 +384,26 @@ function commentCli(stageDir, rawBase, shortSha, runUrl, bodyFile, pathsFile) {
       // Unreadable → treat as "unknown", not as "nothing changed".
     }
   }
-  const body = buildComment(files, { rawBase, shortSha, runUrl, changedPaths });
+  // Render status written by the capture job. Anything other than the literal
+  // 'success' is treated as incomplete; a MISSING/empty file (older run, no arg)
+  // means "unknown" and defaults to complete so this never fabricates a failure
+  // warning on a preview that actually rendered fine.
+  let renderIncomplete = false;
+  if (renderStatusFile) {
+    try {
+      renderIncomplete =
+        readFileSync(renderStatusFile, 'utf8').trim() !== 'success';
+    } catch {
+      // Unreadable → leave complete (fail open toward the normal preview).
+    }
+  }
+  const body = buildComment(files, {
+    rawBase,
+    shortSha,
+    runUrl,
+    changedPaths,
+    renderIncomplete,
+  });
   writeFileSync(bodyFile, body);
   process.stderr.write(`Comment body: ${body.split('\n').length} lines.\n`);
 }
@@ -362,7 +413,7 @@ if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) {
   if (cmd === 'stage') {
     stageCli(rest[0], rest[1], rest[2]);
   } else if (cmd === 'comment') {
-    commentCli(rest[0], rest[1], rest[2], rest[3], rest[4], rest[5]);
+    commentCli(rest[0], rest[1], rest[2], rest[3], rest[4], rest[5], rest[6]);
   } else {
     process.stderr.write(`unknown command: ${cmd ?? '(none)'}\n`);
     process.exit(2);

.github/scripts/web-shell-visuals-publish.test.mjs

@@ -283,3 +283,59 @@ test('buildComment neutralises a path that tries to break out of its code span',
   assert.equal(bullets.length, 1);
   assert.equal((bullets[0].match(/`/g) ?? []).length, 2);
 });
+
+// --- Render-incomplete honesty (a failed scenario must not read as "no change") ---
+
+test('buildComment: empty preview + renderIncomplete says RENDER FAILED, not no-change or coverage-gap', () => {
+  const body = buildComment([], {
+    shortSha: 'abc1234',
+    runUrl: 'https://run.example/9',
+    renderIncomplete: true,
+    // Even with render-shaping files changed, a failed render must NOT show the
+    // coverage-gap prompt — that would imply the render actually ran.
+    changedPaths: ['packages/web-shell/client/components/WelcomeScreen.tsx'],
+  });
+  assert.match(body, /failed to render/i);
+  assert.doesNotMatch(body, /✅/); // never the clean check
+  assert.doesNotMatch(body, /No screenshot changes against the PR base/);
+  assert.doesNotMatch(body, /coverage gap/); // coverage-gap prompt suppressed
+  assert.doesNotMatch(body, /render-shaping/);
+  assert.match(body, /https:\/\/run\.example\/9/); // links the run
+});
+
+test('buildComment: composites present + renderIncomplete warns the preview is PARTIAL', () => {
+  const body = buildComment(['home-dark.png'], {
+    rawBase: 'r',
+    runUrl: 'https://run.example/9',
+    renderIncomplete: true,
+  });
+  assert.match(body, /<img src="r\/home-dark\.png"/); // the shots that rendered still show
+  assert.match(body, /failed to render/i); // ...prefixed by the partial-preview warning
+  assert.match(body, /may be missing views/i);
+});
+
+test('buildComment: renderIncomplete false keeps the existing no-change / coverage-gap behavior', () => {
+  // Complete render, no shots, no render-shaping files → the plain green check.
+  const clean = buildComment([], {
+    shortSha: 'abc1234',
+    renderIncomplete: false,
+    changedPaths: ['packages/core/src/index.ts'],
+  });
+  assert.match(clean, /✅ _No screenshot changes against the PR base\._/);
+  assert.doesNotMatch(clean, /failed to render/i);
+
+  // Complete render, no shots, render-shaping files → the coverage-gap prompt.
+  const gap = buildComment([], {
+    shortSha: 'abc1234',
+    renderIncomplete: false,
+    changedPaths: ['packages/web-shell/client/components/WelcomeScreen.tsx'],
+  });
+  assert.match(gap, /no scenario renders this UI/);
+  assert.doesNotMatch(gap, /failed to render/i);
+});
+
+test('buildComment: render-failure note omits the run link when runUrl is absent', () => {
+  const body = buildComment([], { renderIncomplete: true });
+  assert.match(body, /failed to render/i);
+  assert.doesNotMatch(body, /\[workflow run\]/); // no dangling empty link
+});

.github/workflows/web-shell-visuals-publish.yml

@@ -241,6 +241,15 @@ jobs:
             --paginate --jq '.[].filename' > "${CHANGED_PATHS_FILE}" 2>/dev/null \
             || echo "::warning::Could not list changed files for PR #${PR}; the preview will not flag coverage gaps."
 
+          # --- Render status, to keep an empty/partial preview honest --------
+          # 'failure' when >=1 scenario failed to render on the PR head (the
+          # render job's continue-on-error keeps the passing shots but the whole
+          # preview would otherwise read as a clean "no changes"). Absent/empty ⇒
+          # treat as complete (older runs, or the file was pruned), so this only
+          # ever ADDS a warning, never suppresses an existing preview.
+          RENDER_STATUS_FILE="${ART}/render-status.txt"
+          [ -f "${RENDER_STATUS_FILE}" ] || RENDER_STATUS_FILE=''
+
           # --- Build the comment body ---------------------------------------
           # Delegated to the same unit-tested script (light/dark pairing, flow
           # labels, and HTML escaping live in web-shell-visuals-publish.mjs).
@@ -249,7 +258,8 @@ jobs:
             "${STAGE}" \
             "${RAW_BASE}" \
             "${SHORT_SHA}" "${RUN_URL}" "${BODY_FILE}" \
-            "${CHANGED_PATHS_FILE}"
+            "${CHANGED_PATHS_FILE}" \
+            "${RENDER_STATUS_FILE}"
 
           # --- Post or update the PR comment --------------------------------
           # Dedup only against OUR OWN prior comment (bot author + marker), and

.github/workflows/web-shell-visuals.yml

@@ -98,7 +98,20 @@ jobs:
           echo "PLAYWRIGHT_PORT=${port}" >> "${GITHUB_ENV}"
           echo "Using web-shell Playwright port ${port}"
 
+      # continue-on-error: a single failing/timing-out scenario must NOT discard
+      # the whole preview. Playwright writes each PNG/webm as its test passes, so
+      # the captures from the scenarios that DID pass are already on disk; the
+      # steps below (compose/upload) then publish those instead of the job dying
+      # here and the passing shots never being uploaded. The real result is read
+      # from `steps.after_capture.outcome` (which continue-on-error does NOT mask,
+      # unlike `.conclusion`) and shipped to the publisher as render-status.txt,
+      # so the comment says "render incomplete" rather than a misleading
+      # "no changes" when a scenario broke. The job stays green because the
+      # publish workflow only runs on a `success` conclusion; the broken scenario
+      # is surfaced in the comment, not by sinking every other PR author's preview.
       - name: 'Capture screenshots and flow recordings (after / PR head)'
+        id: 'after_capture'
+        continue-on-error: true
         env:
           WEB_SHELL_VISUALS_OUTPUT_DIR: '${{ runner.temp }}/web-shell-visuals'
         run: 'npm run test:e2e:visuals --workspace=packages/web-shell'
@@ -227,6 +240,12 @@ jobs:
         env:
           OUT_DIR: '${{ runner.temp }}/web-shell-visuals'
           PR_NUMBER: '${{ github.event.pull_request.number }}'
+          # The TRUE after-render result. `.outcome` is the pre-continue-on-error
+          # value ('failure' when >=1 scenario failed), unlike `.conclusion` which
+          # the continue-on-error masks to 'success'. The publisher reads this to
+          # decide whether an empty/partial preview means "no visual change" or
+          # "a scenario failed to render" — the two must not look alike.
+          AFTER_OUTCOME: '${{ steps.after_capture.outcome }}'
         run: |-
           set -euo pipefail
           # Ensure both dirs exist so the counts below are robust even when an
@@ -257,8 +276,15 @@ jobs:
           # publish job binds to the authenticated github.event.workflow_run
           # .head_sha, and an artifact-sourced SHA would be untrusted.
           printf '%s\n' "${PR_NUMBER}" > "${OUT_DIR}/pr.txt"
+          # Render status for the publisher: 'failure' when >=1 scenario failed
+          # (default to 'failure' if the step somehow reported nothing, so an
+          # unknown state fails safe toward "incomplete" rather than a false
+          # all-clear). Anything other than the literal 'success' is treated as
+          # incomplete on the publish side.
+          printf '%s\n' "${AFTER_OUTCOME:-failure}" > "${OUT_DIR}/render-status.txt"
           echo "Screenshots: $(find "${OUT_DIR}/screenshots" -name '*.png' | wc -l | tr -d ' ')"
           echo "GIFs: $(find "${OUT_DIR}/gifs" -name '*.gif' | wc -l | tr -d ' ')"
+          echo "After-render outcome: ${AFTER_OUTCOME:-unknown}"
 
       # The privileged publish workflow downloads THIS artifact, so keep the raw
       # videos out of it: an untrusted PR could drop a multi-GB file under
@@ -272,6 +298,7 @@ jobs:
             ${{ runner.temp }}/web-shell-visuals/screenshots
             ${{ runner.temp }}/web-shell-visuals/gifs
             ${{ runner.temp }}/web-shell-visuals/pr.txt
+            ${{ runner.temp }}/web-shell-visuals/render-status.txt
           if-no-files-found: 'warn'
           retention-days: 7