JSONbored · JSONbored · Jul 20, 2026 · Jul 20, 2026
@@ -470,7 +470,8 @@ export function buildBeforeAfterCollapsible(routes: CaptureRoute[]): UnifiedColl
   // the same de-emphasized styling this table already uses for its own footer legend line below.
   // `imgUrl` (defaults to `url`) is what the <img src> loads; `url` is ALWAYS what the <a href> points at, so
   // "click to open full-size" keeps resolving to the true original even when a smaller downscaled copy
-  // (route.beforeThumbUrl/afterThumbUrl, self-host only) is embedded inline instead.
+  // (route.before/afterThumbUrl[Mobile], self-host only) is embedded inline instead -- both viewports pass
+  // their own thumb field below, not just desktop.
   const cell = (url: string | undefined, label: string, imgUrl: string = url ?? ""): string =>
     url ? `<a href="${attr(url)}" target="_blank" rel="noopener"><img width="360" alt="${attr(label)}" src="${attr(imgUrl)}"></a><br><sub>${attr(label)}</sub>` : "—";
   const rows: string[] = [];
@@ -484,7 +485,7 @@ export function buildBeforeAfterCollapsible(routes: CaptureRoute[]): UnifiedColl
     }
     if (route.beforeUrlMobile || route.afterUrlMobile) {
       if (route.diffUrlMobile) hasAnyDiff = true;
-      rows.push(`| ${path} | mobile${themeSuffix} | ${cell(route.beforeUrlMobile, `before ${route.path} (mobile)${themeSuffix}`)} | ${cell(route.afterUrlMobile, `after ${route.path} (mobile)${themeSuffix}`)} | ${cell(route.diffUrlMobile, `diff ${route.path} (mobile)${themeSuffix}`)} |`);
+      rows.push(`| ${path} | mobile${themeSuffix} | ${cell(route.beforeUrlMobile, `before ${route.path} (mobile)${themeSuffix}`, route.beforeThumbUrlMobile)} | ${cell(route.afterUrlMobile, `after ${route.path} (mobile)${themeSuffix}`, route.afterThumbUrlMobile)} | ${cell(route.diffUrlMobile, `diff ${route.path} (mobile)${themeSuffix}`)} |`);
     }
   }
   if (rows.length === 0) return null;

@@ -63,13 +63,17 @@ export interface CaptureRoute {
   beforeUrlMobile?: string | undefined;
   afterUrl?: string | undefined;
   afterUrlMobile?: string | undefined;
-  // #6324: a separate, downscaled DISPLAY copy of the desktop shot -- self-host only, desktop-only (see
-  // capturePage's own doc comment for why). beforeUrl/afterUrl above are UNCHANGED in meaning (still the
-  // full-resolution original, still what "click to open full-size" resolves to); these are additive fields
-  // the comment table prefers for the embedded <img> when present, falling back to beforeUrl/afterUrl when
-  // absent (hosted mode, or a resize that didn't actually shrink anything).
+  // #6324: a separate, downscaled DISPLAY copy of each shot -- self-host only (see capturePage's own doc
+  // comment for why). beforeUrl/afterUrl above are UNCHANGED in meaning (still the full-resolution original,
+  // still what "click to open full-size" resolves to); these are additive fields the comment table prefers
+  // for the embedded <img> when present, falling back to beforeUrl/afterUrl when absent (hosted mode, or a
+  // resize that didn't actually shrink anything). desktop/mobile get independent thumb fields, mirroring
+  // beforeUrl/beforeUrlMobile's own split -- both viewports need a bounded thumbnail (see the mobile fix's
+  // own doc comment on capturePage's thumbKey below), not just desktop.
   beforeThumbUrl?: string | undefined;
   afterThumbUrl?: string | undefined;
+  beforeThumbUrlMobile?: string | undefined;
+  afterThumbUrlMobile?: string | undefined;
   diffUrl?: string | undefined;
   diffUrlMobile?: string | undefined;
   beforeGifUrl?: string | undefined;
@@ -367,12 +371,17 @@ async function capturePage(
     );
     const key = `${NAMESPACE}/shots/${fingerprint.slice(0, 40)}.png`;
     const url = resolveShotUrl(env, key) || onDemand;
-    // #6324: a downscaled DISPLAY copy, stored at a SIBLING key so the original at `key` never changes --
-    // diffing (compareCapturedScreenshots, via includeBytes below) always reads the true original on both a
-    // fresh render AND a cache hit, and "click to open full-size" keeps resolving to it unchanged. Self-host
-    // only (isDisplayDownscaleAvailable) and desktop-only: shot.ts's mobile viewport (390px) is already
-    // close enough to the table's own 360px display width that a third resized copy would save little.
-    const thumbKey = viewportName === "desktop" && isDisplayDownscaleAvailable() ? `${NAMESPACE}/shots/${fingerprint.slice(0, 40)}-thumb.png` : undefined;
+    // #6324 / mobile-thumb-fix: a downscaled DISPLAY copy, stored at a SIBLING key so the original at `key`
+    // never changes -- diffing (compareCapturedScreenshots, via includeBytes below) always reads the true
+    // original on both a fresh render AND a cache hit, and "click to open full-size" keeps resolving to it
+    // unchanged. Self-host only (isDisplayDownscaleAvailable). Generated for BOTH viewports, not desktop-only
+    // as originally shipped: that desktop-only gate assumed the mobile viewport's 390px WIDTH being already
+    // close to the table's 360px display width meant a resize would "save little" -- true for width, but
+    // shot.ts captures `fullPage: true`, so a mobile screenshot's HEIGHT is just as unbounded as desktop's,
+    // and a narrow-but-very-tall capture rendered at native size in the comment table (the reported bug).
+    // downscaleForDisplay now bounds height too (see its own doc comment), so a mobile thumb is worth
+    // generating exactly like a desktop one.
+    const thumbKey = isDisplayDownscaleAvailable() ? `${NAMESPACE}/shots/${fingerprint.slice(0, 40)}-thumb.png` : undefined;
     const cached = await env.REVIEW_AUDIT.get(key).catch(() => null);
     if (cached) {
       // Verified via a real read, not assumed from the original's own existence -- the sibling write below
@@ -653,6 +662,10 @@ export async function buildCapture(
   let previewBase = "";
   let previewFailed = target.previewFailed === true;
   let previewPending = false;
+  // Hoisted above the discovery block below (was previously computed after it) so the eternal-"loading"-
+  // placeholder fix's `buildState === "absent"` branch can consult it -- seeing this whole file top to
+  // bottom, its own later use (guarding the actions_fallback dispatch) is unchanged.
+  const actionsFallbackEnabled = visualConfig?.actionsFallback === true;
   const urlTemplate = visualConfig?.preview?.urlTemplate;
   if (urlTemplate) {
     previewBase = resolvePreviewUrlTemplate(urlTemplate, { number: target.prNumber, headSha: target.headSha });
@@ -687,20 +700,39 @@ export async function buildCapture(
               await recordPreviewPollAttempt(env, target.headSha);
               previewPending = true;
             }
+          } else if (buildState === "absent" && !actionsFallbackEnabled) {
+            // Eternal-"loading"-placeholder fix: 'absent' means no Workers-Builds-named check-run was found
+            // AT ALL, not "still building" -- previously this fell through as a silent no-op, leaving
+            // previewPending/previewFailed both false, so the caller's afterPlaceholder always resolved to
+            // the animated "Rendering preview…" spinner and NOTHING ever re-evaluated it to a terminal
+            // state (this state was never fed into the recapture-poll mechanism at all). Confirmed live on a
+            // repo whose UI has no preview-deploy CI configured: every PR's "after" cell spun forever. 'absent'
+            // is genuinely ambiguous on its own (the check-run may just not have started yet), so apply the
+            // SAME poll-budget-then-give-up treatment as 'building' above rather than assuming either
+            // extreme. Skipped when actions_fallback is enabled for this repo: that feature's OWN dispatch
+            // below already treats "found nothing" as its trigger condition, and marking previewPending here
+            // first would starve it of the `!previewPending` gate it needs to ever fire.
+            const attempts = await previewPollAttemptCount(env, target.headSha);
+            if (attempts >= MAX_PREVIEW_POLL_ATTEMPTS) {
+              previewFailed = true;
+            } else {
+              await recordPreviewPollAttempt(env, target.headSha);
+              previewPending = true;
+            }
           }
         }
       }
     }
   }
 
   // Fallback (#4112): the discovery chain above found NOTHING at all for this repo (no preview URL, not
-  // failed, and no real build already in flight) -- if review.visual.actions_fallback is enabled, dispatch
-  // .github/workflows/visual-capture-fallback.yml against the repo's own default branch and mark
-  // previewPending so the EXISTING recapture-poll mechanism (processors.ts) retries this same buildCapture
-  // call later, by which point the workflow_run webhook handler (running independently) has stored the
-  // fallback's captured PNGs in R2 for resolveFallbackAfterShot below to find. Requires headSha + a resolved
-  // default branch to pin the dispatch to a trusted ref; either missing ⇒ no dispatch (fail-safe).
-  const actionsFallbackEnabled = visualConfig?.actionsFallback === true;
+  // failed, and no real build already in flight) -- if review.visual.actions_fallback is enabled
+  // (actionsFallbackEnabled, hoisted above), dispatch .github/workflows/visual-capture-fallback.yml against
+  // the repo's own default branch and mark previewPending so the EXISTING recapture-poll mechanism
+  // (processors.ts) retries this same buildCapture call later, by which point the workflow_run webhook
+  // handler (running independently) has stored the fallback's captured PNGs in R2 for
+  // resolveFallbackAfterShot below to find. Requires headSha + a resolved default branch to pin the dispatch
+  // to a trusted ref; either missing ⇒ no dispatch (fail-safe).
   const routes = resolveVisualRoutes(visualFiles, visualConfig?.routes);
   if (!previewBase && !previewFailed && !previewPending && actionsFallbackEnabled && target.headSha && target.defaultBranchRef) {
     // Never re-dispatch onto an already in-flight run (#4112 review fix): the workflow's own `concurrency:
@@ -796,6 +828,8 @@ export async function buildCapture(
         afterUrlMobile: afterMobileShot.url,
         ...(beforeShot.thumbUrl ? { beforeThumbUrl: beforeShot.thumbUrl } : {}),
         ...(afterShot.thumbUrl ? { afterThumbUrl: afterShot.thumbUrl } : {}),
+        ...(beforeMobileShot.thumbUrl ? { beforeThumbUrlMobile: beforeMobileShot.thumbUrl } : {}),
+        ...(afterMobileShot.thumbUrl ? { afterThumbUrlMobile: afterMobileShot.thumbUrl } : {}),
         ...(diffUrl ? { diffUrl } : {}),
         ...(diffUrlMobile ? { diffUrlMobile } : {}),
         ...(beforeGifUrl ? { beforeGifUrl } : {}),

@@ -44,16 +44,26 @@ export function isDisplayDownscaleAvailable(): boolean {
  *  much larger one for a tall full-page capture, since height scales down proportionally too). */
 const DISPLAY_MAX_WIDTH_PX = 720;
 
-/** Downscale `png` so its width is at most {@link DISPLAY_MAX_WIDTH_PX}, preserving aspect ratio and never
- *  enlarging an already-narrow image (a mobile-viewport capture, already close to display width, passes
- *  through unchanged rather than being upscaled). Any decode/resize failure degrades to the ORIGINAL bytes,
- *  matching downscaleForVision's own "a full-size image beats no image" contract -- capturePage's caller
- *  falls back to the original URL entirely when this genuinely can't produce a smaller copy, so a failure
- *  here is never user-visible as a broken image, only as a missed optimization. */
+/** Height cap for the DISPLAY thumbnail, alongside {@link DISPLAY_MAX_WIDTH_PX} -- fixes a real bug (observed
+ *  live on a mobile capture table cell): shot.ts's `fullPage: true` capture means HEIGHT is unbounded by the
+ *  viewport, and a NARROW capture (shot.ts's MOBILE_VIEWPORT is 390px, already under DISPLAY_MAX_WIDTH_PX)
+ *  passed straight through a width-only resize untouched via `withoutEnlargement` -- a several-thousand-pixel-
+ *  tall full-page mobile screenshot rendered at its native size in the comment table instead of a bounded
+ *  thumbnail. Same value as VISION_MAX_DIMENSION_PX -- both are "a reasonable bounded preview", no reason for
+ *  the two budgets to diverge. */
+const DISPLAY_MAX_HEIGHT_PX = 1280;
+
+/** Downscale `png` so its width is at most {@link DISPLAY_MAX_WIDTH_PX} AND its height is at most
+ *  {@link DISPLAY_MAX_HEIGHT_PX} (`fit: "inside"` -- whichever bound is hit first wins, aspect ratio
+ *  preserved), never enlarging an already-small image (a mobile-viewport capture short enough to clear both
+ *  caps passes through unchanged rather than being upscaled). Any decode/resize failure degrades to the
+ *  ORIGINAL bytes, matching downscaleForVision's own "a full-size image beats no image" contract --
+ *  capturePage's caller falls back to the original URL entirely when this genuinely can't produce a smaller
+ *  copy, so a failure here is never user-visible as a broken image, only as a missed optimization. */
 export async function downscaleForDisplay(png: Uint8Array): Promise<Uint8Array> {
   try {
     const resized = await sharp(png)
-      .resize({ width: DISPLAY_MAX_WIDTH_PX, withoutEnlargement: true })
+      .resize({ width: DISPLAY_MAX_WIDTH_PX, height: DISPLAY_MAX_HEIGHT_PX, fit: "inside", withoutEnlargement: true })
       .png()
       .toBuffer();
     return new Uint8Array(resized);

@@ -64,14 +64,26 @@ describe("selfhost image-downscale stub, display copy (#6324)", () => {
     expect(result.byteLength).toBeLessThan(desktopShot.byteLength);
   });
 
-  it("leaves an already-narrow image's dimensions unchanged (withoutEnlargement) -- e.g. a mobile-width capture", async () => {
+  it("leaves an already-narrow, normal-height image's dimensions unchanged (withoutEnlargement) -- e.g. a mobile-viewport capture that isn't full-page", async () => {
     const mobileShot = await solidPng(390, 844);
     const result = await downscaleForDisplay(mobileShot);
     const { width, height } = await dimensionsOf(result);
     expect(width).toBe(390);
     expect(height).toBe(844);
   });
 
+  it("bug fix: downscales a NARROW but very TALL image (a mobile full-page capture) so height is capped too, not just width", async () => {
+    // The real shape that shipped huge, un-downscaled screenshots in a PR comment table: shot.ts's mobile
+    // viewport is 390px wide (already under DISPLAY_MAX_WIDTH_PX), but `fullPage: true` means height is
+    // unbounded by the viewport -- a width-only resize (the original bug) left this completely untouched.
+    const tallMobileShot = await solidPng(390, 5000);
+    const result = await downscaleForDisplay(tallMobileShot);
+    const { width, height } = await dimensionsOf(result);
+    expect(height).toBe(1280);
+    expect(width).toBe(100); // round(390/5000 * 1280) = round(99.84) = 100
+    expect(result.byteLength).toBeLessThan(tallMobileShot.byteLength);
+  });
+
   it("degrades to the ORIGINAL bytes (never drops the image) when the input isn't a valid image", async () => {
     const garbage = new Uint8Array([1, 2, 3, 4, 5]);
     const result = await downscaleForDisplay(garbage);