From 6d44548c24ad3056ad0850bdab57ea886e880a45 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 20 Jul 2026 00:34:43 -0700 Subject: [PATCH] fix(review): bound mobile visual-capture thumbnails and give up on a missing preview build Mobile screenshots rendered at full-page native size in PR comments because the display-thumbnail downscale only bounded width, and thumbnail generation was gated to desktop only. Also, when no preview-build check-run is ever found for a repo (no Workers Builds CI), the "after" cell spun on the loading placeholder forever instead of eventually giving up honestly, since that state was never fed into the existing poll-budget mechanism. Closes #7405 --- src/review/unified-comment-bridge.ts | 5 +- src/review/visual/capture.ts | 70 +++++++++--- src/selfhost/stubs/image-downscale.ts | 24 ++-- .../selfhost-image-downscale-stub.test.ts | 14 ++- test/unit/visual-capture.test.ts | 105 ++++++++++++++++-- test/unit/visual-collapsible.test.ts | 19 +++- 6 files changed, 199 insertions(+), 38 deletions(-) diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 69e47c628a..e1e6cc98a3 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -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 loads; `url` is ALWAYS what the 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 ? `${attr(label)}
${attr(label)}` : "—"; 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; diff --git a/src/review/visual/capture.ts b/src/review/visual/capture.ts index 44cc2f0007..2fa301167f 100644 --- a/src/review/visual/capture.ts +++ b/src/review/visual/capture.ts @@ -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 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 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,6 +700,25 @@ 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; + } } } } @@ -694,13 +726,13 @@ export async function buildCapture( } // 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 } : {}), diff --git a/src/selfhost/stubs/image-downscale.ts b/src/selfhost/stubs/image-downscale.ts index 6ef8579979..84d44de9d9 100644 --- a/src/selfhost/stubs/image-downscale.ts +++ b/src/selfhost/stubs/image-downscale.ts @@ -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 { 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); diff --git a/test/unit/selfhost-image-downscale-stub.test.ts b/test/unit/selfhost-image-downscale-stub.test.ts index a275f68a31..b13274e9fb 100644 --- a/test/unit/selfhost-image-downscale-stub.test.ts +++ b/test/unit/selfhost-image-downscale-stub.test.ts @@ -64,7 +64,7 @@ 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); @@ -72,6 +72,18 @@ describe("selfhost image-downscale stub, display copy (#6324)", () => { 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); diff --git a/test/unit/visual-capture.test.ts b/test/unit/visual-capture.test.ts index ae2ef81e79..26d3892c3d 100644 --- a/test/unit/visual-capture.test.ts +++ b/test/unit/visual-capture.test.ts @@ -338,7 +338,7 @@ describe("visual capture preview discovery", () => { await expect(previewPollAttemptCount(env, "budget-head-2")).resolves.toBe(MAX_PREVIEW_POLL_ATTEMPTS); }); - it("leaves the capture non-pending when no matching preview check run exists at all (buildState 'absent')", async () => { + it("eternal-loading-placeholder fix: marks the capture pending (not silently ignored) when no matching preview check run exists at all (buildState 'absent') and no actions_fallback is configured", async () => { vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { const url = input.toString(); if (url.includes("/deployments?")) return Response.json([]); @@ -355,7 +355,86 @@ describe("visual capture preview discovery", () => { ["apps/loopover-ui/src/routes/app.index.tsx"], ); + // Pre-fix, this silently stayed non-pending forever with no terminal state either -- the "Rendering + // preview…" spinner in the comment table never resolved. Now it's fed into the SAME poll-budget + // mechanism as a 'building' state, so it eventually gives up honestly (see the MAX_PREVIEW_POLL_ATTEMPTS + // test below) instead of spinning forever. + expect(result.previewPending).toBe(true); + expect(result.routes[0]?.afterUrl).toContain("placeholder=loading"); + }); + + it("eternal-loading-placeholder fix: a buildState 'absent' (no actions_fallback) records ONE preview-poll attempt for this head SHA, same budget as 'building'", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/deployments?")) return Response.json([]); + if (url.includes("/status")) return Response.json({ statuses: [] }); + if (url.includes("/check-runs")) return Response.json({ check_runs: [] }); + if (url.includes("/comments")) return Response.json([]); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() }); + + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 16, headSha: "absent-budget-head-1", previewFromChecks: true }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + + expect(result.previewPending).toBe(true); + await expect(previewPollAttemptCount(env, "absent-budget-head-1")).resolves.toBe(1); + }); + + it("eternal-loading-placeholder fix: past MAX_PREVIEW_POLL_ATTEMPTS for a buildState 'absent' head SHA, gives up honestly instead of spinning forever", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/deployments?")) return Response.json([]); + if (url.includes("/status")) return Response.json({ statuses: [] }); + if (url.includes("/check-runs")) return Response.json({ check_runs: [] }); + if (url.includes("/comments")) return Response.json([]); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() }); + for (let i = 0; i < MAX_PREVIEW_POLL_ATTEMPTS; i += 1) { + await recordPreviewPollAttempt(env, "absent-budget-head-2"); + } + + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 17, headSha: "absent-budget-head-2", previewFromChecks: true }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + expect(result.previewPending).toBe(false); + expect(result.routes[0]?.afterUrl).toContain("placeholder=failed"); + await expect(previewPollAttemptCount(env, "absent-budget-head-2")).resolves.toBe(MAX_PREVIEW_POLL_ATTEMPTS); + }); + + it("does NOT apply the buildState 'absent' poll-budget treatment when actions_fallback is enabled -- that feature's own dispatch already owns the 'found nothing' case and needs the previewPending gate free", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/dispatches")) return new Response(null, { status: 204 }); + if (url.includes("/deployments?")) return Response.json([]); + if (url.includes("/status")) return Response.json({ statuses: [] }); + if (url.includes("/check-runs")) return Response.json({ check_runs: [] }); + if (url.includes("/comments")) return Response.json([]); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() }); + + await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 18, headSha: "absent-with-fallback-head", previewFromChecks: true, defaultBranchRef: "main" }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + undefined, + { actionsFallback: true }, + ); + + // The buildState-absent branch itself never ran (it's gated on !actionsFallbackEnabled) -- the dispatch + // path below is what marks pending, and it never records a poll-budget attempt. + await expect(previewPollAttemptCount(env, "absent-with-fallback-head")).resolves.toBe(0); }); it("an explicit routes.paths list replaces file-based route inference end to end", async () => { @@ -814,21 +893,27 @@ describe("buildCapture display-thumbnail wiring (#6324)", () => { } }); - it("never generates a thumbnail for the mobile viewport, even when downscaling is available — 390px is already close to the table's 360px display width", async () => { + it("also generates a thumbnail for the mobile viewport when downscaling is available (bug fix — a full-page mobile capture is height-unbounded even though 390px width is already narrow)", async () => { const availableSpy = vi.spyOn(imageDownscaleModule, "isDisplayDownscaleAvailable").mockReturnValue(true); const downscaleSpy = vi.spyOn(imageDownscaleModule, "downscaleForDisplay").mockResolvedValue(new Uint8Array([1])); const captureShotSpy = vi.spyOn(shotModule, "captureShot").mockResolvedValue({ png: new Uint8Array([9, 9, 9]), authWalled: false }); try { + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com", REVIEW_AUDIT: memoryReviewAudit() }); const result = await buildCapture( - createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com", REVIEW_AUDIT: memoryReviewAudit() }), + env, "installation-token", { repoFullName: "owner/repo", prNumber: 42, previewUrl: "https://preview.example.com" }, ["apps/loopover-ui/src/routes/app.index.tsx"], ); - // downscaleForDisplay was called for the desktop slots only -- CaptureRoute has no mobile thumb field - // at all (by design), so there's nothing to assert false on the route itself; the real assertion is - // that the call count matches "desktop before + desktop after" (2), not 4 (every slot). - expect(downscaleSpy).toHaveBeenCalledTimes(2); + // downscaleForDisplay is now called for every slot (desktop before/after + mobile before/after) — 4, + // not just the desktop 2 from before this fix. + expect(downscaleSpy).toHaveBeenCalledTimes(4); + expect(result.routes[0]?.beforeThumbUrlMobile).toContain("-thumb.png"); + expect(result.routes[0]?.afterThumbUrlMobile).toContain("-thumb.png"); + // The full-resolution mobile URL is UNCHANGED -- still what "click to open full-size" resolves to. + expect(result.routes[0]?.beforeUrlMobile).not.toContain("-thumb.png"); + const storedMobileThumb = await env.REVIEW_AUDIT!.get(await thumbKey(42, "before", "mobile", "https://prod.example.com/app")); + expect(storedMobileThumb).not.toBeNull(); } finally { availableSpy.mockRestore(); downscaleSpy.mockRestore(); @@ -2647,7 +2732,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f expect(result.previewPending).toBe(false); }); - it("never dispatches when actions_fallback is not configured (byte-identical to pre-#4112)", async () => { + it("never dispatches when actions_fallback is not configured (dispatch behavior unchanged; previewPending now true — see the eternal-loading-placeholder fix's own tests above)", async () => { let dispatchCalled = false; vi.stubGlobal( "fetch", @@ -2665,7 +2750,9 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f ); expect(dispatchCalled).toBe(false); - expect(result.previewPending).toBe(false); + // Not false anymore: buildState 'absent' + no actions_fallback now goes through the same poll-budget + // give-up logic as 'building', instead of silently staying non-pending forever. + expect(result.previewPending).toBe(true); }); it("never dispatches without a headSha to pin the build to", async () => { diff --git a/test/unit/visual-collapsible.test.ts b/test/unit/visual-collapsible.test.ts index 0e0c50925c..ed817b733b 100644 --- a/test/unit/visual-collapsible.test.ts +++ b/test/unit/visual-collapsible.test.ts @@ -81,13 +81,30 @@ describe("buildBeforeAfterCollapsible", () => { expect(c?.body).not.toContain('src="https://api.example.dev/loopover/shot?key=loopover/shots/full-after.png"'); }); - it("#6324: falls back to the full-resolution URL for the img src when no thumb URL is present (hosted mode, or mobile rows, which never get one)", () => { + it("#6324: falls back to the full-resolution URL for the img src when no thumb URL is present (hosted mode, or self-host with a route that has none)", () => { const c = buildBeforeAfterCollapsible(routes); // routes (the shared fixture below) has no beforeThumbUrl/afterThumbUrl -- src and href must be identical. expect(c?.body).toContain('before /app/analytics'); expect(c?.body).toContain('after /app/analytics'); }); + it("mobile-thumbnail fix: the mobile row's ALSO prefers beforeThumbUrlMobile/afterThumbUrlMobile over the full-resolution beforeUrlMobile/afterUrlMobile, with still pointing at the full-resolution original", () => { + const c = buildBeforeAfterCollapsible([ + { + path: "/app/analytics", + beforeUrlMobile: "https://api.example.dev/loopover/shot?key=loopover/shots/full-before-m.png", + beforeThumbUrlMobile: "https://api.example.dev/loopover/shot?key=loopover/shots/thumb-before-m.png", + afterUrlMobile: "https://api.example.dev/loopover/shot?key=loopover/shots/full-after-m.png", + afterThumbUrlMobile: "https://api.example.dev/loopover/shot?key=loopover/shots/thumb-after-m.png", + }, + ]); + expect(c?.body).toContain("| `/app/analytics` | mobile |"); + expect(c?.body).toContain('before /app/analytics (mobile)'); + expect(c?.body).toContain('after /app/analytics (mobile)'); + expect(c?.body).not.toContain('src="https://api.example.dev/loopover/shot?key=loopover/shots/full-before-m.png"'); + expect(c?.body).not.toContain('src="https://api.example.dev/loopover/shot?key=loopover/shots/full-after-m.png"'); + }); + it("returns null when no route has any shot URL (no empty table)", () => { expect(buildBeforeAfterCollapsible([])).toBeNull(); expect(buildBeforeAfterCollapsible([{ path: "/" }])).toBeNull();