From 348c9fc6076d9702fbdcdcc5e4806bb2a721548f Mon Sep 17 00:00:00 2001 From: CasbaL <26603379+CasbaL@users.noreply.github.com> Date: Thu, 17 Sep 2026 16:41:54 +0800 Subject: [PATCH] =?UTF-8?q?fix(studio):=20stop=20VideoFrameThumbnail=20err?= =?UTF-8?q?or=E2=86=92cleanup=20infinite=20loop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The error handler called cleanup(), which sets video.src = "" and calls load(). Clearing src itself fires another error event, re-entering the handler at event-loop speed — every pass allocating a React update via setFailed(true). The seeked path triggers the same loop: its cleanup provokes the synthetic error right after a successful frame extract. Detach the error listener inside cleanup() and ignore the synthetic empty-src error. Instrumentation on a real session measured ~475k src assignments/second and ~8.6 MB/s of update allocations before the fix. Fixes #4036 --- .../ui/VideoFrameThumbnail.test.tsx | 170 ++++++++++++++++++ .../src/components/ui/VideoFrameThumbnail.tsx | 15 +- 2 files changed, 180 insertions(+), 5 deletions(-) create mode 100644 packages/studio/src/components/ui/VideoFrameThumbnail.test.tsx diff --git a/packages/studio/src/components/ui/VideoFrameThumbnail.test.tsx b/packages/studio/src/components/ui/VideoFrameThumbnail.test.tsx new file mode 100644 index 0000000000..a5e919e4d0 --- /dev/null +++ b/packages/studio/src/components/ui/VideoFrameThumbnail.test.tsx @@ -0,0 +1,170 @@ +// @vitest-environment happy-dom +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { VideoFrameThumbnail } from "./VideoFrameThumbnail"; + +Object.assign(globalThis, { IS_REACT_ACT_ENVIRONMENT: true }); + +// The component builds its thumbnail from document.createElement("video"/"canvas"), +// so both are faked here. The video fake records load() calls and keeps listener +// sets, letting a test re-fire an event handler after cleanup removed it — the +// exact shape of the error→cleanup→error loop this suite guards against. +interface FakeVideo { + crossOrigin: string; + muted: boolean; + preload: string; + duration: number; + videoWidth: number; + videoHeight: number; + currentTime: number; + loadCalls: number; + _src: string; + src: string; + addEventListener(type: string, fn: () => void): void; + removeEventListener(type: string, fn: () => void): void; + getAttribute(name: string): string | null; + load(): void; + dispatch(type: string): void; +} + +function makeVideo(): FakeVideo { + const listeners = new Map void>>(); + const video: FakeVideo = { + crossOrigin: "", + muted: false, + preload: "", + duration: 10, + videoWidth: 640, + videoHeight: 360, + currentTime: 0, + loadCalls: 0, + _src: "", + src: "", + addEventListener(type, fn) { + if (!listeners.has(type)) listeners.set(type, new Set()); + listeners.get(type)!.add(fn); + }, + removeEventListener(type, fn) { + listeners.get(type)?.delete(fn); + }, + getAttribute(name) { + return name === "src" ? video.src : null; + }, + load() { + video.loadCalls++; + }, + dispatch(type) { + for (const fn of [...(listeners.get(type) ?? [])]) fn(); + }, + }; + Object.defineProperty(video, "src", { + get: () => video._src, + set: (v: string) => { + video._src = v; + }, + }); + return video; +} + +const canvas = { + width: 0, + height: 0, + getContext: () => ({ drawImage: () => {} }), + toDataURL: () => "data:image/jpeg;base64,AAAA", +}; + +describe("VideoFrameThumbnail", () => { + let videos: FakeVideo[]; + let root: Root; + let container: HTMLDivElement; + + beforeEach(() => { + videos = []; + const original = document.createElement.bind(document); + vi.spyOn(document, "createElement").mockImplementation(((tag: string) => { + if (tag === "video") { + const v = makeVideo(); + videos.push(v); + return v as unknown as HTMLVideoElement; + } + if (tag === "canvas") return canvas as unknown as HTMLCanvasElement; + return original(tag); + }) as typeof document.createElement); + container = document.createElement("div"); + document.body.append(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + vi.restoreAllMocks(); + }); + + const render = (props: { src: string; fallbackLabel?: string }) => { + act(() => root.render()); + return videos[videos.length - 1]; + }; + + it("renders the fallback label when the video errors", () => { + render({ src: "missing.mp4", fallbackLabel: "VIDEO" }); + const video = videos[0]; + expect(video.src).toBe("missing.mp4"); + expect(video.loadCalls).toBe(1); + + act(() => video.dispatch("error")); + + expect(container.textContent).toContain("VIDEO"); + // cleanup ran once: detached the error listener and reset the media element + expect(video.src).toBe(""); + expect(video.loadCalls).toBe(2); + }); + + it("does not loop when the cleared src fires a synthetic error", () => { + render({ src: "missing.mp4", fallbackLabel: "VIDEO" }); + const video = videos[0]; + act(() => video.dispatch("error")); + expect(video.loadCalls).toBe(2); + + // The empty src makes the browser fire `error` again; the detached handler + // must stay detached — repeated dispatches must not touch load() anymore. + act(() => { + video.dispatch("error"); + video.dispatch("error"); + video.dispatch("error"); + }); + + expect(video.loadCalls).toBe(2); + expect(container.textContent).toContain("VIDEO"); + }); + + it("keeps the extracted frame and stays inert after a post-seek synthetic error", () => { + render({ src: "clip.mp4" }); + const video = videos[0]; + + act(() => video.dispatch("loadedmetadata")); + expect(video.currentTime).toBe(1); // 10% of a 10s clip + + act(() => video.dispatch("seeked")); + const img = container.querySelector("img"); + expect(img?.getAttribute("src")).toBe("data:image/jpeg;base64,AAAA"); + expect(video.src).toBe(""); + expect(video.loadCalls).toBe(2); + + act(() => { + video.dispatch("error"); + video.dispatch("error"); + }); + + expect(container.querySelector("img")?.getAttribute("src")).toBe("data:image/jpeg;base64,AAAA"); + expect(video.loadCalls).toBe(2); + }); + + it("retries with a fresh video element when src changes", () => { + render({ src: "a.mp4" }); + act(() => root.render()); + expect(videos.length).toBe(2); + expect(videos[1].src).toBe("b.mp4"); + }); +}); diff --git a/packages/studio/src/components/ui/VideoFrameThumbnail.tsx b/packages/studio/src/components/ui/VideoFrameThumbnail.tsx index 5204e6a793..92dbcc57c5 100644 --- a/packages/studio/src/components/ui/VideoFrameThumbnail.tsx +++ b/packages/studio/src/components/ui/VideoFrameThumbnail.tsx @@ -26,10 +26,19 @@ export function VideoFrameThumbnail({ const canvas = document.createElement("canvas"); const ctx = canvas.getContext("2d"); + // Clearing src fires one more `error` on the video, so the error handler + // must be detached first — otherwise error → cleanup → error spins forever. const cleanup = () => { + video.removeEventListener("error", onError); video.src = ""; video.load(); }; + const onError = () => { + // Ignore the synthetic error cleanup itself just triggered. + if (!video.getAttribute("src")) return; + setFailed(true); + cleanup(); + }; video.addEventListener("loadedmetadata", () => { video.currentTime = Math.min(2, video.duration * 0.1 || 2); @@ -44,11 +53,7 @@ export function VideoFrameThumbnail({ cleanup(); }); - video.addEventListener("error", () => { - // Resolve the loading state — a permanent shimmer reads as "still loading". - setFailed(true); - cleanup(); - }); + video.addEventListener("error", onError); video.src = src; video.load();