fix(studio): render a missing preview poster in either order and bank the open counts (#4479)

* ci(studio): bank the open-journey counts after #4401 and allow a reasoned raise

* fix(studio): render a missing preview poster whichever of the 404 and the live frame comes first

.github/workflows/ci.yml

@@ -785,7 +785,7 @@ jobs:
           cp -R packages/studio/tests/e2e/fixtures/studio-open \
             packages/studio/data/projects/studio-open
       - name: Read the base branch's work-count ceilings
-        # A ceiling may not rise or disappear against the branch this PR merges into.
+        # A ceiling may not disappear, or rise without a new reason, against the branch this PR merges into.
         if: github.event_name == 'pull_request'
         env:
           BASE_REF: ${{ github.base_ref }}

packages/studio/src/components/nle/NLEPreview.test.ts

@@ -93,23 +93,25 @@ function renderPreview(
   document.body.append(host);
   const root = createRoot(host);
   const iframeRef = createRef<HTMLIFrameElement>();
-
-  act(() => {
-    root.render(
-      React.createElement(NLEPreview, {
-        projectId: "timeline-edit-playground",
-        iframeRef,
-        onIframeLoad: () => {},
-        previewSlots,
-        onShadowIframeLoad: () => {},
-        onShadowReadyChange: () => {},
-        onShadowError: () => {},
-        setShadowIframeNode: () => {},
-        resetPreviewSlots: () => {},
-        fillBox,
-      }),
-    );
-  });
+  const render = (directUrl?: string) =>
+    act(() => {
+      root.render(
+        React.createElement(NLEPreview, {
+          projectId: "timeline-edit-playground",
+          directUrl,
+          iframeRef,
+          onIframeLoad: () => {},
+          previewSlots,
+          onShadowIframeLoad: () => {},
+          onShadowReadyChange: () => {},
+          onShadowError: () => {},
+          setShadowIframeNode: () => {},
+          resetPreviewSlots: () => {},
+          fillBox,
+        }),
+      );
+    });
+  render();
 
   const viewport = host.querySelector('[aria-label="Composition preview"]') as HTMLDivElement;
   const stage = host.querySelector('[data-testid="preview-zoom-stage"]') as HTMLDivElement;
@@ -124,6 +126,7 @@ function renderPreview(
   return {
     host,
     root,
+    render,
     viewport,
     stage,
     cleanup() {
@@ -296,12 +299,67 @@ describe("NLEPreview", () => {
     expect(poster()?.style.zIndex).toBe("2");
 
     act(() => livePlayerProps.onReadyToShowChange?.(false));
-    expect(poster()).not.toBeNull();
+    expect(poster()?.hidden).toBe(false);
     act(() => livePlayerProps.onReadyToShowChange?.(true));
+    expect(poster()?.hidden).toBe(true);
+    act(() => poster()?.dispatchEvent(new Event("load")));
     expect(poster()).toBeNull();
     view.cleanup();
   });
 
+  describe("a missing poster", () => {
+    const renderUrl =
+      "/api/projects/timeline-edit-playground/thumbnail/index.html?t=0&output=source";
+    const fetchSpy = vi.fn(() => Promise.resolve(new Response()));
+    beforeEach(() => {
+      fetchSpy.mockClear();
+      vi.stubGlobal("fetch", fetchSpy);
+    });
+    afterEach(() => vi.unstubAllGlobals());
+
+    const settle = (
+      view: ReturnType<typeof renderPreview>,
+      steps: Array<"ready" | "missing" | "loaded">,
+    ) => {
+      for (const step of steps) {
+        const poster = view.stage.querySelector('[data-testid="preview-poster"]');
+        act(() =>
+          step === "ready"
+            ? livePlayerProps.onReadyToShowChange?.(true)
+            : poster?.dispatchEvent(new Event(step === "missing" ? "error" : "load")),
+        );
+      }
+    };
+
+    it("is rendered for the next open when the live frame is ready first", () => {
+      const view = renderPreview();
+      settle(view, ["ready", "missing"]);
+      expect(fetchSpy.mock.calls).toEqual([[renderUrl]]);
+      expect(view.stage.querySelector('[data-testid="preview-poster"]')).toBeNull();
+      view.cleanup();
+    });
+
+    it("is rendered once the live frame is ready when it is missing first", () => {
+      const view = renderPreview();
+      settle(view, ["missing"]);
+      expect(fetchSpy).not.toHaveBeenCalled();
+      settle(view, ["ready", "ready"]);
+      expect(fetchSpy.mock.calls).toEqual([[renderUrl]]);
+      view.cleanup();
+    });
+
+    it("is rendered on a return from a sub-composition when the live frame is ready first", () => {
+      const view = renderPreview();
+      settle(view, ["loaded", "ready"]);
+      view.render("/api/projects/timeline-edit-playground/preview/comp/compositions/intro.html");
+      settle(view, ["ready"]);
+      view.render();
+      settle(view, ["ready", "missing"]);
+      expect(fetchSpy.mock.calls).toEqual([[renderUrl]]);
+      view.cleanup();
+    });
+  });
+
   it("mounts the live player once when the composition switches", () => {
     playerMounts.length = 0;
     const Harness = ({ projectId }: { projectId: string }) => {

packages/studio/src/components/nle/NLEPreview.tsx

@@ -551,11 +551,13 @@ export const NLEPreview = memo(function NLEPreview({
                 />
               ),
             )}
-            {poster.showPoster && (
+            {poster.mountPoster && (
               <PreviewPoster
                 key={activeKey}
                 projectId={projectId}
+                hidden={poster.hidePoster}
                 onSize={(size) => setCompositionSize((prev) => prev ?? size)}
+                onLoaded={poster.onPosterLoaded}
                 onMissing={poster.onPosterMissing}
               />
             )}

packages/studio/src/components/nle/PreviewPoster.tsx

@@ -21,11 +21,15 @@ const PREVIEW_POSTER_STYLE: React.CSSProperties = {
 /** Covers the live preview with the cached frame 0; reports its size so the stage fits it. */
 export function PreviewPoster({
   projectId,
+  hidden,
   onSize,
+  onLoaded,
   onMissing,
 }: {
   projectId: string;
+  hidden: boolean;
   onSize: (size: PreviewCompositionSize) => void;
+  onLoaded: () => void;
   onMissing: () => void;
 }) {
   return (
@@ -34,42 +38,62 @@ export function PreviewPoster({
       alt=""
       aria-hidden
       data-testid="preview-poster"
+      hidden={hidden}
       style={PREVIEW_POSTER_STYLE}
       onLoad={(event) => {
         const { naturalWidth: width, naturalHeight: height } = event.currentTarget;
-        if (width > 0 && height > 0) onSize({ width, height });
+        if (!hidden && width > 0 && height > 0) onSize({ width, height });
+        onLoaded();
       }}
       onError={onMissing}
     />
   );
 }
 
-/** The poster covers the live slot until its first frame is ready to show; a missing poster is
- * rendered after that, off the open path, for the next open. */
+/** The poster covers the live slot until its first frame is ready; a missing poster is rendered
+ * after both are known, in either order, off the open path, for the next open. */
 export function usePreviewPoster(
   projectId: string,
   activeKey: string,
   directUrl: string | undefined,
 ) {
-  const [posterDoneFor, setPosterDoneFor] = useState<string | null>(null);
-  const posterMissingRef = useRef(false);
+  const visit = useVisitNumber(activeKey);
+  const [coverDoneFor, setCoverDoneFor] = useState<number | null>(null);
+  const [posterSettledFor, setPosterSettledFor] = useState<number | null>(null);
+  const missingForRef = useRef<number | null>(null);
+  const liveReadyForRef = useRef<number | null>(null);
+  const renderMissingPoster = useCallback(() => {
+    if (missingForRef.current !== visit || liveReadyForRef.current !== visit) return;
+    missingForRef.current = null;
+    void fetch(previewPosterUrl(projectId, false)).catch(() => {});
+  }, [visit, projectId]);
   const onLiveReadyToShowChange = useCallback(
     (ready: boolean) => {
       if (!ready) return;
-      setPosterDoneFor(activeKey);
-      if (!posterMissingRef.current || directUrl) return;
-      posterMissingRef.current = false;
-      void fetch(previewPosterUrl(projectId, false)).catch(() => {});
+      setCoverDoneFor(visit);
+      liveReadyForRef.current = visit;
+      renderMissingPoster();
     },
-    [activeKey, directUrl, projectId],
+    [visit, renderMissingPoster],
   );
   return {
-    showPoster: !directUrl && posterDoneFor !== activeKey,
+    mountPoster: !directUrl && (coverDoneFor !== visit || posterSettledFor !== visit),
+    hidePoster: coverDoneFor === visit,
     onLiveReadyToShowChange,
-    onPreviewError: () => setPosterDoneFor(activeKey),
+    onPreviewError: () => setCoverDoneFor(visit),
+    onPosterLoaded: () => setPosterSettledFor(visit),
     onPosterMissing: () => {
-      posterMissingRef.current = true;
-      setPosterDoneFor(activeKey);
+      setCoverDoneFor(visit);
+      setPosterSettledFor(visit);
+      missingForRef.current = visit;
+      renderMissingPoster();
     },
   };
 }
+
+function useVisitNumber(key: string): number {
+  const [seen, setSeen] = useState({ key, visit: 0 });
+  if (seen.key === key) return seen.visit;
+  setSeen({ key, visit: seen.visit + 1 });
+  return seen.visit + 1;
+}

packages/studio/tests/e2e/perf-ceilings.json

@@ -2,10 +2,13 @@
   "studio-open": {
     "browser": "153",
     "counts": {
-      "thumbnailRenders": 20,
+      "thumbnailRenders": 14,
       "total.frameNavigations": 7,
       "total.requests.document": 2,
-      "total.requests.thumbnail": 20
+      "total.requests.thumbnail": 22
+    },
+    "raised": {
+      "total.requests.thumbnail": "#4401 shows a cached frame-0 poster while the preview boots: a cache-only ask that misses on a fresh open, then a render of that poster for the next open (+2 requests)"
     }
   }
 }

packages/studio/tests/e2e/perf-ratchet.mjs

@@ -2,13 +2,15 @@
 /**
  * Work-count ratchet: each gated counter has a ceiling in perf-ceilings.json that may only go
  * down. A journey fails when a counter rises above its ceiling or falls below it (bank the
- * improvement), or when a ceiling was raised or removed against the base branch's file.
+ * improvement), or when a ceiling was raised or removed against the base branch's file. A raise
+ * passes only when the journey's `raised` map gives that counter a reason the base did not have.
  *
  *   node perf-ratchet.mjs check <ceilings.json> <journey> <evidence.json> [<base-ceilings.json>]
  *   node perf-ratchet.mjs lower <ceilings.json> <journey> <evidence.json>
  *   node perf-ratchet.mjs correlate <variant>=<evidence.json> ...
  *
- * Ceilings: `{ <journey>: { browser: "<Chrome major>", counts: { <counter>: n } } }`.
+ * Ceilings: `{ <journey>: { browser: "<Chrome major>", counts: { <counter>: n },
+ *   raised?: { <counter>: "<why this ceiling went up>" } } }`.
  * Evidence is a journey's JSON output: `workCounts`, `wallMs` and the browser it ran in.
  */
 import { existsSync, readFileSync, realpathSync, writeFileSync } from "node:fs";
@@ -27,15 +29,17 @@ function measuredStatus(ceiling, value) {
   return value > ceiling ? "rose" : "below";
 }
 
-function ceilingRow(counter, ceiling, value, base) {
-  if (raisedAgainst(ceiling, base)) return { counter, ceiling, base, status: "raised" };
+function ceilingRow(counter, ceiling, value, base, accepted) {
+  if (raisedAgainst(ceiling, base) && !accepted.has(counter)) {
+    return { counter, ceiling, base, status: "raised" };
+  }
   return { counter, ceiling, value, status: measuredStatus(ceiling, value) };
 }
 
 /** Passes only when every gated counter sits exactly at a ceiling no higher than the base's. */
-export function checkCeilings(ceilings, counts, baseCeilings = {}) {
+export function checkCeilings(ceilings, counts, baseCeilings = {}, accepted = new Set()) {
   const rows = Object.entries(ceilings).map(([counter, ceiling]) =>
-    ceilingRow(counter, ceiling, counts[counter], baseCeilings[counter]),
+    ceilingRow(counter, ceiling, counts[counter], baseCeilings[counter], accepted),
   );
   if (rows.length === 0)
     throw new Error("no gated counters: a ratchet that checks nothing passes nothing");
@@ -57,7 +61,7 @@ const ROW_TEXT = {
   below: ({ counter, ceiling, value }) =>
     `FAIL ${counter} fell ${ceiling} -> ${value}: bank it by setting its ceiling to ${value} (perf-ratchet.mjs lower)`,
   raised: ({ counter, ceiling, base }) =>
-    `FAIL ${counter}: ceiling raised ${base} -> ${ceiling} against the base branch; ceilings only go down`,
+    `FAIL ${counter}: ceiling raised ${base} -> ${ceiling} against the base branch; a raise needs a new reason under "raised" in perf-ceilings.json`,
   removed: ({ counter, base }) =>
     `FAIL ${counter}: ceiling ${base} removed against the base branch; ceilings only go down`,
 };
@@ -153,10 +157,24 @@ export function readBase(base, all, journey) {
   assertBaseShape(base);
   return {
     counts: base[journey]?.counts,
+    raised: base[journey]?.raised,
     removedJourneys: Object.keys(base).filter((name) => !Object.hasOwn(all, name)),
   };
 }
 
+const normalized = (text) => (typeof text === "string" ? text.replace(/\s+/g, " ").trim() : "");
+const isNewReason = (reason, baseReason) =>
+  normalized(reason) !== "" && normalized(reason) !== normalized(baseReason);
+
+/** Counters raised above the base on purpose: a reason in `raised` the base branch did not give. */
+export function acceptedRaises(entry, base) {
+  const reasons = entry.raised ?? {};
+  const onPurpose = (counter) =>
+    raisedAgainst(entry.counts[counter], base.counts?.[counter]) &&
+    isNewReason(reasons[counter], base.raised?.[counter]);
+  return new Set(Object.keys(reasons).filter(onPurpose));
+}
+
 function assertBaseShape(base) {
   for (const [name, entry] of Object.entries(base)) {
     if (!isObject(entry?.counts)) throw new Error(`base ceilings: "${name}" has no counts object`);
@@ -197,10 +215,16 @@ function printResult(journey, ok, rows, removedJourneys) {
 function runCheck(args) {
   const { all, journey, evidence, counts } = journeyInputs(args);
   const base = loadBase(args[3], all, journey);
-  const { passed, rows } = checkCeilings(all[journey].counts, counts, base.counts);
+  const accepted = acceptedRaises(all[journey], base);
+  const { passed, rows } = checkCeilings(all[journey].counts, counts, base.counts, accepted);
   const ok = passed && base.removedJourneys.length === 0;
   printResult(journey, ok, rows, base.removedJourneys);
   printNotes(all[journey], evidence, rows);
+  for (const counter of accepted) {
+    console.log(
+      `[perf-ratchet]   note: ${counter} raised on purpose: ${all[journey].raised[counter]}`,
+    );
+  }
   return ok ? 0 : 1;
 }
 

packages/studio/tests/e2e/perf-ratchet.test.mjs

@@ -5,6 +5,7 @@ import { join } from "node:path";
 import { fileURLToPath } from "node:url";
 import { describe, expect, it } from "vitest";
 import {
+  acceptedRaises,
   browserMajor,
   checkCeilings,
   correlate,
@@ -36,6 +37,60 @@ describe("checkCeilings", () => {
     expect(rows.map((row) => row.status)).toEqual(["raised", "removed"]);
   });
 
+  it("passes a raise only with a reason the base branch did not give", () => {
+    const base = { counts: { a: 5 }, raised: { b: "older raise" } };
+    const withReason = { counts: { a: 6 }, raised: { a: "new work on purpose" } };
+    expect(
+      checkCeilings(withReason.counts, { a: 6 }, base.counts, acceptedRaises(withReason, base))
+        .passed,
+    ).toBe(true);
+    const noReason = { counts: { a: 6 } };
+    expect(
+      checkCeilings(noReason.counts, { a: 6 }, base.counts, acceptedRaises(noReason, base)).passed,
+    ).toBe(false);
+    const blank = { counts: { a: 6 }, raised: { a: " " } };
+    expect(acceptedRaises(blank, base).size).toBe(0);
+    expect(acceptedRaises(blank, { counts: { a: 5 }, raised: { a: "older raise" } }).size).toBe(0);
+    const notText = { counts: { a: 6 }, raised: { a: 1 } };
+    expect(acceptedRaises(notText, base).size).toBe(0);
+  });
+
+  it("does not take a whitespace edit of the base's reason as a new one", () => {
+    const base = { counts: { a: 6 }, raised: { a: "new work  on purpose" } };
+    const respaced = { counts: { a: 7 }, raised: { a: " new work on purpose " } };
+    expect(acceptedRaises(respaced, base).size).toBe(0);
+  });
+
+  it("accepts a reason only for a counter whose ceiling went up", () => {
+    const entry = { counts: { a: 5 }, raised: { a: "why" } };
+    expect(acceptedRaises(entry, { counts: { a: 5 } }).size).toBe(0);
+  });
+
+  it("does not reuse a reason the base already gave for a later raise", () => {
+    const base = { counts: { a: 6 }, raised: { a: "new work on purpose" } };
+    const again = { counts: { a: 7 }, raised: { a: "new work on purpose" } };
+    const { passed, rows } = checkCeilings(
+      again.counts,
+      { a: 7 },
+      base.counts,
+      acceptedRaises(again, base),
+    );
+    expect(passed).toBe(false);
+    expect(rows[0].status).toBe("raised");
+  });
+
+  it("still fails a removed ceiling when other raises carry reasons", () => {
+    const entry = { counts: { a: 6 }, raised: { a: "why", b: "why" } };
+    const { passed, rows } = checkCeilings(
+      entry.counts,
+      { a: 6 },
+      { a: 5, b: 2 },
+      acceptedRaises(entry, { counts: { a: 5, b: 2 } }),
+    );
+    expect(passed).toBe(false);
+    expect(rows.map((row) => row.status)).toEqual(["at", "removed"]);
+  });
+
   it("fails a gated counter the journey did not measure", () => {
     expect(checkCeilings({ reactCommits: 4 }, {}).passed).toBe(false);
     expect(checkCeilings({ reactCommits: 4 }, { reactCommits: Number.NaN }).passed).toBe(false);
@@ -49,6 +104,11 @@ describe("checkCeilings", () => {
 describe("readBase", () => {
   const all = { open: { browser: "153", counts: { a: 1 } } };
 
+  it("returns the base journey's raise reasons", () => {
+    const base = { j: { counts: { a: 1 }, raised: { a: "why" } } };
+    expect(readBase(base, base, "j").raised).toEqual({ a: "why" });
+  });
+
   it("names base journeys this file dropped", () => {
     const base = { open: { counts: { a: 2 } }, scroll: { counts: { b: 1 } } };
     expect(readBase(base, all, "open")).toEqual({ counts: { a: 2 }, removedJourneys: ["scroll"] });
@@ -93,6 +153,46 @@ describe("lower", () => {
   });
 });
 
+describe("check", () => {
+  const cli = fileURLToPath(new URL("./perf-ratchet.mjs", import.meta.url));
+  const run = (reason) => {
+    const dir = mkdtempSync(join(tmpdir(), "perf-ratchet-"));
+    const file = (name, value) => {
+      writeFileSync(join(dir, name), JSON.stringify(value));
+      return join(dir, name);
+    };
+    const base = file("base.json", {
+      j: { browser: "153", counts: { a: 5 }, raised: { a: "why" } },
+    });
+    const ceilings = file("ceilings.json", {
+      j: { browser: "153", counts: { a: 6 }, raised: { a: reason } },
+    });
+    const evidence = file("evidence.json", {
+      browser: "HeadlessChrome/153.0.1.1",
+      workCounts: { a: 6 },
+    });
+    return execFileSync(process.execPath, [cli, "check", ceilings, "j", evidence, base], {
+      stdio: "pipe",
+      encoding: "utf8",
+    });
+  };
+
+  it("passes a raise with a new reason against the base file and prints it", () => {
+    expect(run("new work on purpose")).toMatch(/a raised on purpose: new work on purpose/);
+  });
+
+  it("fails a raise that reuses the base file's reason", () => {
+    let failure;
+    try {
+      run("why");
+    } catch (error) {
+      failure = error;
+    }
+    expect(failure?.status).toBe(1);
+    expect(failure?.stdout).toMatch(/ceiling raised 5 -> 6/);
+  });
+});
+
 describe("lowerCeilings", () => {
   it("only ever lowers", () => {
     expect(lowerCeilings({ a: 10, b: 10, c: 10 }, { a: 7, b: 12 })).toEqual({ a: 7, b: 10, c: 10 });