diff --git a/packages/engine/src/services/screenshotService.test.ts b/packages/engine/src/services/screenshotService.test.ts
index e6f96d4c6..4332c359d 100644
--- a/packages/engine/src/services/screenshotService.test.ts
+++ b/packages/engine/src/services/screenshotService.test.ts
@@ -453,4 +453,58 @@ describe("video-frame injection respects ancestor visibility", () => {
//
to `visibility: visible` so it overrides the ancestor.
expect(seededImg.style.visibility).toBe("visible");
});
+
+ // Regression for the layered/HDR mask path: `applyDomLayerMask` writes an
+ // `!important` stylesheet rule `#${showId} *{visibility:visible !important}`
+ // which, if a sub-comp host id appears in the show set, would revive a
+ // plain (non-important) inline `visibility: hidden` on a descendant
+ // `__render_frame__` — the cascade rule is "important stylesheet author
+ // beats non-important inline author". To stay safe regardless of which
+ // layer ends up in `show`, the ancestor-hidden hide must be written with
+ // `!important` so inline `!important` beats stylesheet `!important`.
+ //
+ // linkedom strips `!important` from `cssText`/`getPropertyPriority`, so we
+ // pin the contract on the API call site instead: a `setProperty(name,
+ // value, "important")` invocation on the live `
`'s style.
+ it("injectVideoFramesBatch hides a stale
with !important so the layer mask cannot revive it", async () => {
+ const { teardown, setup } = withGlobals(setupHostHiddenScenario({ visibility: "hidden" }));
+ const seededImg = setup.document.createElement("img");
+ seededImg.classList.add("__render_frame__");
+ seededImg.style.visibility = "visible";
+ setup.video.parentNode?.insertBefore(seededImg, setup.video.nextSibling);
+ const setPropertySpy = vi.spyOn(seededImg.style, "setProperty");
+
+ try {
+ await injectVideoFramesBatch(passthroughPage(), [
+ {
+ videoId: "pip",
+ dataUri:
+ "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkAAIAAAoAAv/lxKUAAAAASUVORK5CYII=",
+ },
+ ]);
+ } finally {
+ teardown();
+ }
+
+ expect(seededImg.style.visibility).toBe("hidden");
+ expect(setPropertySpy).toHaveBeenCalledWith("visibility", "hidden", "important");
+ });
+
+ it("syncVideoFrameVisibility hides an existing
with !important so the layer mask cannot revive it", async () => {
+ const { teardown, setup } = withGlobals(setupHostHiddenScenario({ visibility: "hidden" }));
+ const seededImg = setup.document.createElement("img");
+ seededImg.classList.add("__render_frame__");
+ seededImg.style.visibility = "visible";
+ setup.video.parentNode?.insertBefore(seededImg, setup.video.nextSibling);
+ const setPropertySpy = vi.spyOn(seededImg.style, "setProperty");
+
+ try {
+ await syncVideoFrameVisibility(passthroughPage(), ["pip"]);
+ } finally {
+ teardown();
+ }
+
+ expect(seededImg.style.visibility).toBe("hidden");
+ expect(setPropertySpy).toHaveBeenCalledWith("visibility", "hidden", "important");
+ });
});
diff --git a/packages/engine/src/services/screenshotService.ts b/packages/engine/src/services/screenshotService.ts
index a73423f87..7af0481ad 100644
--- a/packages/engine/src/services/screenshotService.ts
+++ b/packages/engine/src/services/screenshotService.ts
@@ -369,13 +369,22 @@ export async function removeDomLayerMask(page: Page, extraHideIds: string[]): Pr
);
}
+/**
+ * Returns the subset of `updates.videoId`s that were actually painted in
+ * this call. Videos skipped because of a hidden visual ancestor are NOT
+ * included — the caller relies on this to avoid recording a `lastInjected`
+ * cache entry for a frame that never reached the page, which would otherwise
+ * short-circuit the next inject at the same frameIndex and leave the host's
+ * first visible frame blank.
+ */
export async function injectVideoFramesBatch(
page: Page,
updates: Array<{ videoId: string; dataUri: string }>,
-): Promise {
- if (updates.length === 0) return;
- await page.evaluate(
+): Promise {
+ if (updates.length === 0) return [];
+ return await page.evaluate(
async (items: Array<{ videoId: string; dataUri: string }>, visualProperties: string[]) => {
+ const injectedIds: string[] = [];
const pendingDecodes: Array> = [];
const replacementLayoutProperties = new Set([
"width",
@@ -435,7 +444,13 @@ export async function injectVideoFramesBatch(
// Don't paint a frame over a hidden host — if an existing replacement
//
is still around from when the host was visible, hide it so it
// doesn't bleed through a sibling host that *is* visible on this seek.
- if (hasImg && img) img.style.visibility = "hidden";
+ //
+ // Use `!important` so the inline hide survives `applyDomLayerMask`'s
+ // stylesheet `#${showId} *{visibility:visible !important}` when the
+ // sub-comp host happens to land in the active layer's `show` set —
+ // important stylesheet beats non-important inline, but important
+ // inline beats important stylesheet.
+ if (hasImg && img) img.style.setProperty("visibility", "hidden", "important");
continue;
}
@@ -522,10 +537,12 @@ export async function injectVideoFramesBatch(
// GSAP-controlled value.
video.style.setProperty("visibility", "hidden", "important");
video.style.setProperty("pointer-events", "none", "important");
+ injectedIds.push(item.videoId);
}
if (pendingDecodes.length > 0) {
await Promise.all(pendingDecodes);
}
+ return injectedIds;
},
updates,
[...MEDIA_VISUAL_STYLE_PROPERTIES],
@@ -577,11 +594,16 @@ export async function syncVideoFrameVisibility(
} else {
// Inactive (or ancestor-hidden) video: hide both. Use visibility only
// (never opacity) so we never clobber GSAP-controlled inline opacity.
+ // Use `!important` on the
hide so `applyDomLayerMask`'s
+ // important stylesheet rule (`#${showId} *{visibility:visible !important}`)
+ // cannot revive a stale frame when the sub-comp host lands in the
+ // active layer's `show` set — same mask-defense reasoning as the
+ // `isVisualAncestorHidden` branch in `injectVideoFramesBatch`.
video.style.removeProperty("display");
video.style.setProperty("visibility", "hidden", "important");
video.style.setProperty("pointer-events", "none", "important");
if (hasImg) {
- img.style.visibility = "hidden";
+ img.style.setProperty("visibility", "hidden", "important");
}
}
}
diff --git a/packages/engine/src/services/videoFrameInjector.test.ts b/packages/engine/src/services/videoFrameInjector.test.ts
index 28c813641..5a1c793b1 100644
--- a/packages/engine/src/services/videoFrameInjector.test.ts
+++ b/packages/engine/src/services/videoFrameInjector.test.ts
@@ -1,9 +1,30 @@
// @vitest-environment node
-import { describe, it, expect, beforeEach, afterEach } from "vitest";
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
-import { __testing } from "./videoFrameInjector.js";
+import { type Page } from "puppeteer-core";
+
+// Hoist mocks before importing the module under test so the mock factory wins.
+// The cache-hygiene block exercises createVideoFrameInjector against stubbed
+// page-side primitives so we can assert on Node-side state (cache poisoning)
+// without standing up a real browser.
+const { injectVideoFramesBatchMock, syncVideoFrameVisibilityMock } = vi.hoisted(() => ({
+ injectVideoFramesBatchMock: vi.fn<
+ (page: Page, updates: Array<{ videoId: string; dataUri: string }>) => Promise
+ >(async (_page, updates) => updates.map((u) => u.videoId)),
+ syncVideoFrameVisibilityMock: vi.fn<(page: Page, ids: string[]) => Promise>(
+ async () => undefined,
+ ),
+}));
+
+vi.mock("./screenshotService.js", () => ({
+ injectVideoFramesBatch: injectVideoFramesBatchMock,
+ syncVideoFrameVisibility: syncVideoFrameVisibilityMock,
+}));
+
+import { __testing, createVideoFrameInjector } from "./videoFrameInjector.js";
+import { type FrameLookupTable } from "./videoFrameExtractor.js";
import { DEFAULT_CONFIG } from "../config.js";
const { createFrameSourceCache } = __testing;
@@ -143,3 +164,91 @@ describe("frame source cache eviction", () => {
expect(cache.stats()).toMatchObject({ ...SHARED_STATS, entries: 0, bytes: 0 });
});
});
+
+describe("createVideoFrameInjector cache hygiene against page-side skips", () => {
+ // Build a minimal FrameLookupTable stand-in that returns one fixed payload
+ // for every time so we can drive the hook deterministically. The real
+ // table is exercised exhaustively in videoFrameExtractor.test.ts.
+ function fakeTable(payload: { videoId: string; framePath: string; frameIndex: number }) {
+ return {
+ getActiveFramePayloads: () =>
+ new Map([
+ [payload.videoId, { framePath: payload.framePath, frameIndex: payload.frameIndex }],
+ ]),
+ } as unknown as FrameLookupTable;
+ }
+
+ // Bypass the on-disk frame cache by handing back a synthetic data URI.
+ function inlineResolver(framePath: string): string {
+ return `data:image/png;base64,fake-${framePath}`;
+ }
+
+ beforeEach(() => {
+ injectVideoFramesBatchMock.mockReset();
+ syncVideoFrameVisibilityMock.mockReset();
+ syncVideoFrameVisibilityMock.mockResolvedValue(undefined);
+ });
+
+ it("does not poison the lastInjected cache when the page reports zero ids injected", async () => {
+ // Regression for the agentic-finecut scenario after PR #1028's ancestor
+ // skip: when injectVideoFramesBatch silently drops a video (its sub-comp
+ // host is hidden), the caller used to record `lastInjectedFrame[v] = N`
+ // anyway. On the next frame, if the source frameIndex is unchanged
+ // (low-fps source, multiple output frames per source frame, or
+ // non-frame-aligned host start), the cache short-circuits the second
+ // call and the host's first visible frame paints blank because the
+ // replacement
was never created.
+ //
+ // Pin the contract: when the page returns `[]` (no ids actually
+ // injected), the cache must not record those frameIndexes, so a follow-
+ // up call at the same frameIndex still issues an inject.
+ const fakePage = {} as Page;
+ const hook = createVideoFrameInjector(
+ fakeTable({ videoId: "pip", framePath: "/p", frameIndex: 5 }),
+ {
+ frameSrcResolver: inlineResolver,
+ },
+ );
+ expect(hook).not.toBeNull();
+
+ // First call: simulate the ancestor-hidden skip — page-side reports it
+ // injected nothing.
+ injectVideoFramesBatchMock.mockResolvedValueOnce([]);
+ await hook!(fakePage, 0);
+ expect(injectVideoFramesBatchMock).toHaveBeenCalledTimes(1);
+ expect(injectVideoFramesBatchMock).toHaveBeenLastCalledWith(fakePage, [
+ { videoId: "pip", dataUri: "data:image/png;base64,fake-/p" },
+ ]);
+
+ // Second call: same frameIndex, but the previous call did not really
+ // paint. The cache must NOT short-circuit; the inject must run again.
+ injectVideoFramesBatchMock.mockResolvedValueOnce(["pip"]);
+ await hook!(fakePage, 0);
+ expect(injectVideoFramesBatchMock).toHaveBeenCalledTimes(2);
+ expect(injectVideoFramesBatchMock).toHaveBeenLastCalledWith(fakePage, [
+ { videoId: "pip", dataUri: "data:image/png;base64,fake-/p" },
+ ]);
+ });
+
+ it("does cache normally when the page reports the id as injected", async () => {
+ // Counter-test: when injection succeeds for a videoId, the cache must
+ // record it and a second call at the same frameIndex must short-circuit.
+ // This pins the happy path so a future refactor can't trade the skip
+ // bug for a never-cache regression.
+ const fakePage = {} as Page;
+ const hook = createVideoFrameInjector(
+ fakeTable({ videoId: "pip", framePath: "/p", frameIndex: 5 }),
+ {
+ frameSrcResolver: inlineResolver,
+ },
+ );
+
+ injectVideoFramesBatchMock.mockResolvedValueOnce(["pip"]);
+ await hook!(fakePage, 0);
+ expect(injectVideoFramesBatchMock).toHaveBeenCalledTimes(1);
+
+ await hook!(fakePage, 0);
+ // Cache hit — no second inject for the same frameIndex.
+ expect(injectVideoFramesBatchMock).toHaveBeenCalledTimes(1);
+ });
+});
diff --git a/packages/engine/src/services/videoFrameInjector.ts b/packages/engine/src/services/videoFrameInjector.ts
index ddc79722a..d8b9e0e56 100644
--- a/packages/engine/src/services/videoFrameInjector.ts
+++ b/packages/engine/src/services/videoFrameInjector.ts
@@ -197,12 +197,22 @@ export function createVideoFrameInjector(
await syncVideoFrameVisibility(page, Array.from(activeIds));
if (updates.length > 0) {
- await injectVideoFramesBatch(
- page,
- updates.map((u) => ({ videoId: u.videoId, dataUri: u.dataUri })),
+ // Only record cache entries for videos the page actually painted.
+ // injectVideoFramesBatch skips any video whose visual ancestor is
+ // hidden (sub-comp host out-of-window) and returns the subset of ids
+ // it really wrote — recording the rest would short-circuit the next
+ // call at the same frameIndex and leave the host's first visible
+ // frame blank.
+ const injectedIds = new Set(
+ await injectVideoFramesBatch(
+ page,
+ updates.map((u) => ({ videoId: u.videoId, dataUri: u.dataUri })),
+ ),
);
for (const update of updates) {
- lastInjectedFrameByVideo.set(update.videoId, update.frameIndex);
+ if (injectedIds.has(update.videoId)) {
+ lastInjectedFrameByVideo.set(update.videoId, update.frameIndex);
+ }
}
}
};