diff --git a/packages/studio/src/hooks/timelineAudioGroupCreate.ts b/packages/studio/src/hooks/timelineAudioGroupCreate.ts index 939ca1ff02..b98a1903d3 100644 --- a/packages/studio/src/hooks/timelineAudioGroupCreate.ts +++ b/packages/studio/src/hooks/timelineAudioGroupCreate.ts @@ -262,6 +262,30 @@ export async function createAudioGroupAndAssignMembers({ * expanded-rows resolution as element-visibility, for the same reason — a * nested sub-composition child has no entry in the raw store list. */ +/** + * Run the group write; on failure log it, toast it, and rethrow it. + * + * Module scope, not the hook's: the React Compiler cannot lower a `throw` inside + * a `try`/`catch` and declines the whole hook when it finds one. The rethrow is + * the point, not an oversight — the carve's auto-group chains + * `.then(() => ({ ...next, sources: [groupId] }))` off this promise, so + * swallowing here let it persist a carve pointing at a group that was never + * written, the exact silent no-op the throw inside + * `createAudioGroupAndAssignMembers` exists to prevent. + */ +async function reportGroupFailure( + showToast: (message: string, tone?: "error" | "info") => void, + write: () => Promise, +): Promise { + try { + await write(); + } catch (error) { + console.error("[Timeline] Failed to group voice clips", error); + showToast(error instanceof Error ? error.message : "Failed to group voice clips"); + throw error; + } +} + export function useAudioGroupCarveAssignment({ projectIdRef, activeCompPath, @@ -294,7 +318,7 @@ export function useAudioGroupCarveAssignment({ const domId = runtimeAudioId(item); return domId !== null && wanted.has(domId); }); - try { + await reportGroupFailure(showToast, async () => { // Loud, not silent: an unresolved id used to leave `elements` short, // `createAudioGroupAndAssignMembers` returning early with no write, and // the carve still persisting `sources: [groupId]` for a group that was @@ -317,17 +341,7 @@ export function useAudioGroupCarveAssignment({ domEditSaveTimestampRef, pendingTimelineEditPathRef, }); - } catch (error) { - console.error("[Timeline] Failed to group voice clips", error); - const message = error instanceof Error ? error.message : "Failed to group voice clips"; - showToast(message); - // Rethrown, not just reported: the carve's auto-group chains - // `.then(() => ({ ...next, sources: [groupId] }))` off this promise, so - // swallowing here let it persist a carve pointing at a group that was - // never written — the exact silent no-op the throw inside - // `createAudioGroupAndAssignMembers` exists to prevent. - throw error; - } + }); }, [ activeCompPath, diff --git a/packages/studio/src/hooks/useAnimatedPropertyCommit.ts b/packages/studio/src/hooks/useAnimatedPropertyCommit.ts index 177d8c5c5d..75eb722ad5 100644 --- a/packages/studio/src/hooks/useAnimatedPropertyCommit.ts +++ b/packages/studio/src/hooks/useAnimatedPropertyCommit.ts @@ -428,9 +428,8 @@ async function commitKeyframeProps( export function useAnimatedPropertyCommit(deps: CommitAnimatedPropertyDeps) { const { selectedGsapAnimations, gsapCommitMutation, previewIframeRef, bumpGsapCache } = deps; + // The single routing boundary for set, keyframe, whole-tween and first-group writes. const commitAnimatedProperties = useCallback( - // This is the single routing boundary for set, keyframe, whole-tween, and first-group writes. - // fallow-ignore-next-line complexity async (selection: DomEditSelection, props: Record): Promise => { if (!gsapCommitMutation) return; const propEntries = Object.entries(props); @@ -461,10 +460,12 @@ export function useAnimatedPropertyCommit(deps: CommitAnimatedPropertyDeps) { // The picked anim comes from the (possibly stale) panel cache: if keyframes // were just removed or the script changed underneath us, its id is gone - // server-side and the commit 404s. The raw commit already toasts; we catch - // so the rejection doesn't escape as an uncaught promise, and bump the cache - // so selectedGsapAnimations re-syncs and the user's next edit self-heals. - try { + // server-side and the commit 404s. The raw commit already toasts; the + // handler below bumps the cache so selectedGsapAnimations re-syncs and the + // next edit self-heals. A `.catch` and not a `try`: the React Compiler + // cannot lower a `throw` inside a `try`/`catch`, and declines the hook. + // fallow-ignore-next-line complexity + const write = async (): Promise => { // Animated element → keyframe at the playhead, EXACTLY like manual drag / // resize / rotate: if the picked anim is still a static `set`, // commitKeyframeProps converts it to keyframes first, then writes the new @@ -580,10 +581,11 @@ export function useAnimatedPropertyCommit(deps: CommitAnimatedPropertyDeps) { return; } throw new GsapEditBlockedError("no-selector"); - } catch (error) { + }; + await write().catch((error: unknown) => { bumpGsapCache(); throw error; - } + }); }, [selectedGsapAnimations, gsapCommitMutation, previewIframeRef, bumpGsapCache], ); diff --git a/packages/studio/src/hooks/useAppHotkeys.ts b/packages/studio/src/hooks/useAppHotkeys.ts index 505f807904..b7c0e06aba 100644 --- a/packages/studio/src/hooks/useAppHotkeys.ts +++ b/packages/studio/src/hooks/useAppHotkeys.ts @@ -484,25 +484,31 @@ export function useAppHotkeys({ // ── Stable callback ref (one ref replaces fifteen) ── + // Written after commit, not during render: a ref write in the hook body is a + // render side effect and the React Compiler declines the whole hook when it + // sees one. Every reader below is a keydown handler, which cannot fire before + // the render that produced these callbacks has committed. const cbRef = useRef(null!); - cbRef.current = { - handleTimelineElementsDelete, - handleTimelineElementSplit, - handleDomEditElementDelete, - handleUndo, - handleRedo, - handleCopy, - handlePaste, - handleCut, - onResetKeyframes, - onDeleteSelectedKeyframes, - onToggleRecording, - onGroupSelection, - onUngroupSelection, - leftSidebarRef, - domEditSelectionRef, - showToast, - }; + useEffect(() => { + cbRef.current = { + handleTimelineElementsDelete, + handleTimelineElementSplit, + handleDomEditElementDelete, + handleUndo, + handleRedo, + handleCopy, + handlePaste, + handleCut, + onResetKeyframes, + onDeleteSelectedKeyframes, + onToggleRecording, + onGroupSelection, + onUngroupSelection, + leftSidebarRef, + domEditSelectionRef, + showToast, + }; + }); // ── Keydown dispatch ── diff --git a/packages/studio/src/hooks/useBlockCatalog.test.tsx b/packages/studio/src/hooks/useBlockCatalog.test.tsx new file mode 100644 index 0000000000..d03a28fcde --- /dev/null +++ b/packages/studio/src/hooks/useBlockCatalog.test.tsx @@ -0,0 +1,93 @@ +// @vitest-environment happy-dom +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +type Catalog = Awaited>; + +/** + * The catalog is cached in module scope, so each case needs its own module + * instance or the second one reads the first one's answer. + */ +async function loadHook() { + vi.resetModules(); + const { useBlockCatalog } = await import("./useBlockCatalog"); + return useBlockCatalog; +} + +/** One snapshot per render, so the assertions read a list instead of a mutated binding. */ +const states: { loading: boolean; error: string | null; blocks: unknown[] }[] = []; + +async function render(useBlockCatalog: Catalog) { + function Probe() { + const { blocks, loading, error } = useBlockCatalog(); + states.push({ loading, error, blocks }); + return null; + } + const root = createRoot(document.createElement("div")); + await act(async () => { + root.render(); + }); + return () => act(() => root.unmount()); +} + +beforeEach(() => { + states.length = 0; +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe("useBlockCatalog", () => { + it("ends loading with the fetched blocks", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => ({ + ok: true, + json: async () => [{ title: "Fade", description: "", tags: ["transition"] }], + })), + ); + + const unmount = await render(await loadHook()); + + const last = states[states.length - 1]!; + expect(last.loading).toBe(false); + expect(last.error).toBeNull(); + expect(last.blocks).toHaveLength(1); + unmount(); + }); + + it("ends loading with the failure message when the catalog request rejects", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => { + throw new Error("offline"); + }), + ); + + const unmount = await render(await loadHook()); + + const last = states[states.length - 1]!; + expect(last.loading).toBe(false); + expect(last.error).toBe("offline"); + expect(last.blocks).toEqual([]); + unmount(); + }); + + it("ends loading with a message when the catalog responds not-ok", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => ({ ok: false, json: async () => [] })), + ); + + const unmount = await render(await loadHook()); + + const last = states[states.length - 1]!; + expect(last.loading).toBe(false); + expect(last.error).toBe("Failed to load catalog"); + unmount(); + }); +}); diff --git a/packages/studio/src/hooks/useBlockCatalog.ts b/packages/studio/src/hooks/useBlockCatalog.ts index 7efaee4d4e..ce95d62a40 100644 --- a/packages/studio/src/hooks/useBlockCatalog.ts +++ b/packages/studio/src/hooks/useBlockCatalog.ts @@ -36,6 +36,24 @@ function loadCatalog(): Promise { return catalogRequest; } +/** + * The catalog fetch, as a value rather than a throw. + * + * The React Compiler cannot reorder across a `finally`, so a `try`/`finally` + * anywhere in a hook body makes it decline the whole hook and silently drop every + * memo in it. Out here the same control flow is just a function, and the effect + * below is left with one branch instead of three clauses. + */ +type CatalogLoad = { readonly items: CatalogItem[] } | { readonly error: string }; + +async function loadCatalogResult(): Promise { + try { + return { items: await loadCatalog() }; + } catch (err) { + return { error: err instanceof Error ? err.message : "Failed to load catalog" }; + } +} + export function useBlockCatalog() { const [blocks, setBlocks] = useState(() => catalogCache ?? []); const [loading, setLoading] = useState(catalogCache === null); @@ -43,22 +61,15 @@ export function useBlockCatalog() { const [search, setSearch] = useState(""); const [category, setCategory] = useState(null); - // fallow-ignore-next-line complexity useEffect(() => { if (catalogCache) return; let cancelled = false; - (async () => { - try { - const items = await loadCatalog(); - if (cancelled) return; - setBlocks(items); - } catch (err) { - if (cancelled) return; - setError(err instanceof Error ? err.message : "Failed to load catalog"); - } finally { - if (!cancelled) setLoading(false); - } - })(); + void loadCatalogResult().then((result) => { + if (cancelled) return; + if ("items" in result) setBlocks(result.items); + else setError(result.error); + setLoading(false); + }); return () => { cancelled = true; }; diff --git a/packages/studio/src/hooks/useBlockHandlers.test.tsx b/packages/studio/src/hooks/useBlockHandlers.test.tsx new file mode 100644 index 0000000000..47a710adb9 --- /dev/null +++ b/packages/studio/src/hooks/useBlockHandlers.test.tsx @@ -0,0 +1,119 @@ +// @vitest-environment happy-dom +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { useBlockHandlers, type UseBlockHandlersResult } from "./useBlockHandlers"; + +Reflect.set(globalThis, "IS_REACT_ACT_ENVIRONMENT", true); + +const addBlockToProject = vi.hoisted(() => vi.fn()); +vi.mock("../utils/blockInstaller", () => ({ addBlockToProject })); + +/** One handle per render, so the assertions read a list instead of a mutated binding. */ +const rendered: UseBlockHandlersResult[] = []; +const showToast = vi.fn(); + +const PLACEMENT = { start: 0, track: 0, compositionPath: "index.html" }; + +function makeDeps() { + return { + activeCompPath: "index.html", + timelineElements: [], + readProjectFile: vi.fn(async () => ""), + writeProjectFile: vi.fn(async () => {}), + recordEdit: vi.fn(async () => {}), + refreshFileTree: vi.fn(async () => {}), + reloadPreview: vi.fn(), + showToast, + }; +} + +function Probe({ deps }: { deps: ReturnType }) { + rendered.push( + useBlockHandlers({ + projectId: "p1", + blockCtxDeps: deps, + previewIframeRef: { current: null }, + setRightCollapsed: vi.fn(), + setRightPanelTab: vi.fn(), + }), + ); + return null; +} + +function latest(): UseBlockHandlersResult { + const handle = rendered[rendered.length - 1]; + if (!handle) throw new Error("hook did not render"); + return handle; +} + +let unmount: () => void; + +beforeEach(() => { + rendered.length = 0; + vi.clearAllMocks(); + const root = createRoot(document.createElement("div")); + act(() => root.render()); + unmount = () => act(() => root.unmount()); +}); + +afterEach(() => unmount()); + +describe("useBlockHandlers install latch", () => { + it("refuses a second install while the first is still in flight", async () => { + let release = () => {}; + addBlockToProject.mockImplementation( + () => new Promise((resolve) => (release = () => resolve(undefined))), + ); + + const first = latest().handleAddMediaOverlay("fade-in", PLACEMENT); + await act(async () => { + await latest().handleAddMediaOverlay("fade-in", PLACEMENT); + }); + + expect(addBlockToProject).toHaveBeenCalledTimes(1); + expect(showToast).toHaveBeenCalledWith("A block is already installing — one moment…", "info"); + + await act(async () => { + release(); + await first; + }); + }); + + it("lowers the latch when the install rejects, so the next drop still installs", async () => { + addBlockToProject.mockRejectedValueOnce(new Error("install failed")); + + await act(async () => { + await latest().handleAddMediaOverlay("fade-in", PLACEMENT); + }); + + addBlockToProject.mockResolvedValueOnce(undefined); + await act(async () => { + await latest().handleAddMediaOverlay("fade-in", PLACEMENT); + }); + + expect(addBlockToProject).toHaveBeenCalledTimes(2); + }); + + it("reports a failed install instead of leaving the rejection unhandled", async () => { + addBlockToProject.mockRejectedValueOnce(new Error("install failed")); + + // Every caller drops this promise, so a rejection reaches nothing that can + // report it: the user gets "Adding fade-in…" and then silence. + await act(async () => { + await expect(latest().handleAddMediaOverlay("fade-in", PLACEMENT)).resolves.toBeUndefined(); + }); + + expect(showToast).toHaveBeenCalledWith("install failed"); + }); + + it("names the block when the failure carries no message", async () => { + addBlockToProject.mockRejectedValueOnce("nope"); + + await act(async () => { + await latest().handleAddMediaOverlay("fade-in", PLACEMENT); + }); + + expect(showToast).toHaveBeenCalledWith("Failed to add fade-in"); + }); +}); diff --git a/packages/studio/src/hooks/useBlockHandlers.ts b/packages/studio/src/hooks/useBlockHandlers.ts index 368de32027..cdb2be0af5 100644 --- a/packages/studio/src/hooks/useBlockHandlers.ts +++ b/packages/studio/src/hooks/useBlockHandlers.ts @@ -51,6 +51,24 @@ export interface UseBlockHandlersResult { handlePreviewBlockDrop: (blockName: string, position: { left: number; top: number }) => void; } +/** + * Run `install`, and lower `latch` however it ends. + * + * Module scope rather than in the hook that owns the latch: the React Compiler + * cannot reorder across a `finally`, so a `try`/`finally` anywhere inside a hook + * makes it decline the whole hook and silently drop every memo in it. + */ +async function runAndRelease( + latch: React.RefObject, + install: () => Promise, +): Promise { + try { + return await install(); + } finally { + latch.current = false; + } +} + export function useBlockHandlers({ projectId, blockCtxDeps, @@ -61,27 +79,42 @@ export function useBlockHandlers({ const [activeBlockParams, setActiveBlockParams] = useState(null); + // The caller rebuilds `blockCtxDeps` every render, so this repacks it into an + // object keyed on the eight fields that actually matter. Destructured first so + // the dependency list is eight plain names: a list of member expressions had to + // be suppressed to stand, and a suppression is a bail-out the React Compiler + // counts, which cost this hook its memoization anyway. + const { + activeCompPath, + timelineElements, + readProjectFile, + writeProjectFile, + recordEdit, + refreshFileTree, + reloadPreview, + showToast, + } = blockCtxDeps; + const blockCtx = useMemo( () => ({ - activeCompPath: blockCtxDeps.activeCompPath, - timelineElements: blockCtxDeps.timelineElements, - readProjectFile: blockCtxDeps.readProjectFile, - writeProjectFile: blockCtxDeps.writeProjectFile, - recordEdit: blockCtxDeps.recordEdit, - refreshFileTree: blockCtxDeps.refreshFileTree, - reloadPreview: blockCtxDeps.reloadPreview, - showToast: blockCtxDeps.showToast, + activeCompPath, + timelineElements, + readProjectFile, + writeProjectFile, + recordEdit, + refreshFileTree, + reloadPreview, + showToast, }), - // eslint-disable-next-line react-hooks/exhaustive-deps [ - blockCtxDeps.activeCompPath, - blockCtxDeps.timelineElements, - blockCtxDeps.readProjectFile, - blockCtxDeps.writeProjectFile, - blockCtxDeps.recordEdit, - blockCtxDeps.refreshFileTree, - blockCtxDeps.reloadPreview, - blockCtxDeps.showToast, + activeCompPath, + timelineElements, + readProjectFile, + writeProjectFile, + recordEdit, + refreshFileTree, + reloadPreview, + showToast, ], ); @@ -96,11 +129,15 @@ export function useBlockHandlers({ } installingBlockRef.current = true; blockCtx.showToast(`Adding ${blockName}…`, "info"); - try { - return await install(); - } finally { - installingBlockRef.current = false; - } + // Every caller drops this promise: three do `void runBlockInstall(...)` and + // the fourth is awaited from an unawaited JSX handler. A rejection there is + // an unhandled rejection, and the user is left with the "Adding…" toast and + // no second one. Report it on the surface the install itself reports on and + // resolve to null, which every caller already reads as "nothing installed". + return await runAndRelease(installingBlockRef, install).catch((error: unknown) => { + blockCtx.showToast(error instanceof Error ? error.message : `Failed to add ${blockName}`); + return null; + }); }, [blockCtx], ); diff --git a/packages/studio/src/hooks/useClipboard.test.tsx b/packages/studio/src/hooks/useClipboard.test.tsx new file mode 100644 index 0000000000..9b7af3a890 --- /dev/null +++ b/packages/studio/src/hooks/useClipboard.test.tsx @@ -0,0 +1,107 @@ +// @vitest-environment jsdom +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { usePlayerStore } from "../player"; +import { useClipboard } from "./useClipboard"; + +Reflect.set(globalThis, "IS_REACT_ACT_ENVIRONMENT", true); + +const readFileContent = vi.hoisted(() => vi.fn(async () => "")); +vi.mock("./timelineEditingHelpers", async (importOriginal) => ({ + ...(await importOriginal()), + readFileContent, +})); + +type Clipboard = ReturnType; + +/** One handle per render, so the assertions read a list instead of a mutated binding. */ +const rendered: Clipboard[] = []; +const showToast = vi.fn(); + +function Probe({ projectId }: { projectId: string | null }) { + rendered.push( + useClipboard({ + projectId, + activeCompPath: "index.html", + domEditSelectionRef: { current: null }, + showToast, + writeProjectFile: vi.fn(async () => {}), + recordEdit: vi.fn(async () => {}), + domEditSaveTimestampRef: { current: 0 }, + reloadPreview: vi.fn(), + handleTimelineElementDelete: vi.fn(async () => {}), + handleDomEditElementDelete: vi.fn(async () => {}), + previewIframeRef: { current: iframe }, + }), + ); + return null; +} + +function latest(): Clipboard { + const handle = rendered[rendered.length - 1]; + if (!handle) throw new Error("hook did not render"); + return handle; +} + +let iframe: HTMLIFrameElement; +let root: ReturnType; + +beforeEach(() => { + rendered.length = 0; + vi.clearAllMocks(); + iframe = document.createElement("iframe"); + document.body.append(iframe); + const doc = iframe.contentDocument!; + doc.body.innerHTML = `
card
`; + usePlayerStore.setState({ + selectedElementId: "card", + elements: [ + { + id: "card", + domId: "card", + hfId: "hf-card", + selector: "#card", + sourceFile: "index.html", + start: 0, + duration: 1, + }, + ] as never, + }); + root = createRoot(document.createElement("div")); +}); + +afterEach(() => { + act(() => root.unmount()); + document.body.replaceChildren(); + usePlayerStore.getState().reset(); +}); + +describe("useClipboard", () => { + it("pastes into the project the hook was last rendered with", async () => { + act(() => root.render()); + expect(latest().handleCopy()).toBe(true); + + act(() => root.render()); + await act(async () => { + await latest().handlePaste(); + }); + + // The project id has to come from the current render, not from whatever it + // was when the copy happened, or a paste after switching projects reads the + // old project's file. + expect(readFileContent).toHaveBeenCalledWith("second", "index.html"); + }); + + it("does nothing when there is no project to paste into", async () => { + act(() => root.render()); + latest().handleCopy(); + + act(() => root.render()); + await act(async () => { + await latest().handlePaste(); + }); + + expect(readFileContent).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/studio/src/hooks/useClipboard.ts b/packages/studio/src/hooks/useClipboard.ts index e25f983cc5..fc86c00585 100644 --- a/packages/studio/src/hooks/useClipboard.ts +++ b/packages/studio/src/hooks/useClipboard.ts @@ -62,8 +62,6 @@ export function useClipboard({ previewIframeRef, }: UseClipboardOptions) { const clipboardRef = useRef(null); - const projectIdRef = useRef(projectId); - projectIdRef.current = projectId; // The copy-mode branches predate this change; this diff only replaces its // duplicated DOM lookup with the canonical composition-aware resolver. @@ -139,7 +137,7 @@ export function useClipboard({ showToast("Nothing to paste.", "info"); return; } - const pid = projectIdRef.current; + const pid = projectId; if (!pid) return; const targetPath = activeCompPath || "index.html"; @@ -191,6 +189,7 @@ export function useClipboard({ }, [ activeCompPath, domEditSaveTimestampRef, + projectId, recordEdit, reloadPreview, showToast, diff --git a/packages/studio/src/hooks/useConsoleErrorCapture.test.tsx b/packages/studio/src/hooks/useConsoleErrorCapture.test.tsx new file mode 100644 index 0000000000..547aa40c7e --- /dev/null +++ b/packages/studio/src/hooks/useConsoleErrorCapture.test.tsx @@ -0,0 +1,80 @@ +// @vitest-environment jsdom +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import type { LintFinding } from "../components/LintModal"; +import { useConsoleErrorCapture } from "./useConsoleErrorCapture"; + +Reflect.set(globalThis, "IS_REACT_ACT_ENVIRONMENT", true); + +/** One snapshot per render, so the assertions read a list instead of a mutated binding. */ +const captured: (LintFinding[] | null)[] = []; + +function Probe({ iframe }: { iframe: HTMLIFrameElement | null }) { + const { consoleErrors } = useConsoleErrorCapture(iframe); + captured.push(consoleErrors); + return null; +} + +function latest(): LintFinding[] | null { + return captured[captured.length - 1] ?? null; +} + +/** jsdom types `contentWindow` as the bare `Window`, which has no `console`. */ +function previewWindow(frame: HTMLIFrameElement): Window & typeof globalThis { + const win = frame.contentWindow as (Window & typeof globalThis) | null; + if (!win) throw new Error("iframe has no content window"); + return win; +} + +let iframe: HTMLIFrameElement; +let unmount: () => void; + +beforeEach(() => { + captured.length = 0; + iframe = document.createElement("iframe"); + document.body.append(iframe); + const root = createRoot(document.createElement("div")); + act(() => root.render()); + unmount = () => act(() => root.unmount()); +}); + +afterEach(() => { + document.body.replaceChildren(); +}); + +describe("useConsoleErrorCapture", () => { + it("collects a console.error from the preview window", () => { + act(() => previewWindow(iframe).console.error("boom", new Error("detail"))); + + expect(latest()).toEqual([{ severity: "error", message: "boom detail" }]); + unmount(); + }); + + it("drops the favicon noise every preview emits", () => { + act(() => previewWindow(iframe).console.error("GET /favicon.ico 404")); + + expect(latest()).toBeNull(); + unmount(); + }); + + it("collects an uncaught error event from the preview window", () => { + act(() => { + previewWindow(iframe).dispatchEvent(new ErrorEvent("error", { message: "threw" })); + }); + + expect(latest()).toEqual([{ severity: "error", message: "threw" }]); + unmount(); + }); + + it("restores the preview's console.error on unmount", () => { + const win = previewWindow(iframe); + const patched = win.console.error; + + unmount(); + + expect(win.console.error).not.toBe(patched); + act(() => win.console.error("after")); + expect(latest()).toBeNull(); + }); +}); diff --git a/packages/studio/src/hooks/useConsoleErrorCapture.ts b/packages/studio/src/hooks/useConsoleErrorCapture.ts index f49ccf619b..4e72aaf979 100644 --- a/packages/studio/src/hooks/useConsoleErrorCapture.ts +++ b/packages/studio/src/hooks/useConsoleErrorCapture.ts @@ -1,6 +1,57 @@ import { useCallback, useEffect, useRef, useState } from "react"; import type { LintFinding } from "../components/LintModal"; +/** + * Patch a preview window's `console.error` and `error` event, and return the + * undo. + * + * Module scope, not the effect body it used to live in. The React Compiler + * refuses a hook whose nested closures reassign a binding it can see from + * render, and `previewIframe` reached these closures straight from the hook's + * parameters. Out here the parameter is a plain argument, and the patch state + * lives in locals no compiled code observes. + */ +function attachErrorCapture( + win: (Window & typeof globalThis) | null, + onError: (message: string) => void, +): () => void { + if (!win) return () => {}; + if ((win as unknown as Record).__hfErrorCapture) return () => {}; + let origConsoleError: ((...args: unknown[]) => void) | null = null; + let errorHandler: ((e: ErrorEvent) => void) | null = null; + try { + (win as unknown as Record).__hfErrorCapture = true; + origConsoleError = win.console.error.bind(win.console); + win.console.error = function (...args: unknown[]) { + origConsoleError?.(...args); + const text = args.map((a) => (a instanceof Error ? a.message : String(a))).join(" "); + if (text.includes("favicon")) return; + onError(text); + }; + errorHandler = (e: ErrorEvent) => onError(e.message || String(e)); + win.addEventListener("error", errorHandler); + } catch { + /* same-origin only */ + } + return () => { + try { + if (origConsoleError) win.console.error = origConsoleError; + if (errorHandler) win.removeEventListener("error", errorHandler); + delete (win as unknown as Record).__hfErrorCapture; + } catch { + /* cross-origin or destroyed window */ + } + }; +} + +function previewWindow(iframe: HTMLIFrameElement): (Window & typeof globalThis) | null { + try { + return iframe.contentWindow as (Window & typeof globalThis) | null; + } catch { + return null; + } +} + /** * Captures `console.error` and `window.onerror` events from a preview iframe * and exposes them as LintFinding[] for the console errors modal. @@ -14,75 +65,27 @@ export function useConsoleErrorCapture(previewIframe: HTMLIFrameElement | null) setConsoleErrors(null); }, []); - // eslint-disable-next-line no-restricted-syntax + const appendError = useCallback((message: string) => { + consoleErrorsRef.current = [...consoleErrorsRef.current, { severity: "error", message }]; + setConsoleErrors([...consoleErrorsRef.current]); + }, []); + useEffect(() => { if (!previewIframe) return; - - let patchedWin: (Window & typeof globalThis) | null = null; - let origConsoleError: ((...args: unknown[]) => void) | null = null; - let errorHandler: ((e: ErrorEvent) => void) | null = null; - - const detachErrorCapture = () => { - const win = patchedWin; - if (!win) return; - patchedWin = null; - try { - // origConsoleError and errorHandler are always set alongside patchedWin - win.console.error = origConsoleError!; - win.removeEventListener("error", errorHandler!); - delete (win as unknown as Record).__hfErrorCapture; - } catch { - /* cross-origin or destroyed window */ - } - origConsoleError = null; - errorHandler = null; - }; - - const attachErrorCapture = () => { - detachErrorCapture(); - try { - const win = previewIframe.contentWindow as (Window & typeof globalThis) | null; - if (!win) return; - if ((win as unknown as Record).__hfErrorCapture) return; - (win as unknown as Record).__hfErrorCapture = true; - patchedWin = win; - origConsoleError = win.console.error.bind(win.console); - win.console.error = function (...args: unknown[]) { - origConsoleError!(...args); - const text = args.map((a) => (a instanceof Error ? a.message : String(a))).join(" "); - if (text.includes("favicon")) return; - consoleErrorsRef.current = [ - ...consoleErrorsRef.current, - { severity: "error", message: text }, - ]; - setConsoleErrors([...consoleErrorsRef.current]); - }; - errorHandler = (e: ErrorEvent) => { - const text = e.message || String(e); - consoleErrorsRef.current = [ - ...consoleErrorsRef.current, - { severity: "error", message: text }, - ]; - setConsoleErrors([...consoleErrorsRef.current]); - }; - win.addEventListener("error", errorHandler); - } catch { - /* same-origin only */ - } - }; - - attachErrorCapture(); + let detach = attachErrorCapture(previewWindow(previewIframe), appendError); + // Re-attach on every LOAD, not once on mount: a reload keeps the element and + // the WindowProxy while replacing the inner window that holds the listeners. const handleLoad = () => { - consoleErrorsRef.current = []; - setConsoleErrors(null); - attachErrorCapture(); + detach(); + resetErrors(); + detach = attachErrorCapture(previewWindow(previewIframe), appendError); }; previewIframe.addEventListener("load", handleLoad); return () => { previewIframe.removeEventListener("load", handleLoad); - detachErrorCapture(); + detach(); }; - }, [previewIframe]); + }, [previewIframe, appendError, resetErrors]); return { consoleErrors, setConsoleErrors, resetErrors }; } diff --git a/packages/studio/src/hooks/useDomEditPreviewSync.test.tsx b/packages/studio/src/hooks/useDomEditPreviewSync.test.tsx new file mode 100644 index 0000000000..ea79d17706 --- /dev/null +++ b/packages/studio/src/hooks/useDomEditPreviewSync.test.tsx @@ -0,0 +1,88 @@ +// @vitest-environment jsdom +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { DomEditSelection } from "../components/editor/domEditing"; +import { useDomEditPreviewSync } from "./useDomEditPreviewSync"; + +Reflect.set(globalThis, "IS_REACT_ACT_ENVIRONMENT", true); + +function makeSelection(id: string): DomEditSelection { + return { + id, + hfId: `hf-${id}`, + selector: `#${id}`, + selectorIndex: 0, + sourceFile: "index.html", + element: document.createElement("div"), + } as unknown as DomEditSelection; +} + +function Probe({ + domEditSelection, + openSourceForSelection, +}: { + domEditSelection: DomEditSelection | null; + openSourceForSelection: (sourceFile: string, target: unknown) => void; +}) { + useDomEditPreviewSync({ + previewIframe: null, + activeCompPath: "index.html", + captionEditMode: false, + domEditSelectionRef: { current: null }, + domEditGroupSelectionsRef: { current: [] }, + domEditSelection, + applyDomSelection: vi.fn(), + refreshDomEditGroupSelectionsFromPreview: vi.fn(async () => {}), + buildDomSelectionFromTarget: vi.fn(async () => null), + refreshPreviewDocumentVersion: vi.fn(), + syncPreviewHotkeys: vi.fn(), + applyStudioManualEditsToPreviewRef: { current: vi.fn(async () => {}) }, + openSourceForSelection, + getSidebarTab: () => "code", + }); + return null; +} + +let root: ReturnType; + +beforeEach(() => { + root = createRoot(document.createElement("div")); +}); + +afterEach(() => act(() => root.unmount())); + +describe("useDomEditPreviewSync auto-reveal", () => { + it("reveals the newly selected element's source while the Code tab is open", () => { + const openSource = vi.fn(); + + act(() => root.render()); + act(() => + root.render( + , + ), + ); + + expect(openSource).toHaveBeenCalledWith("index.html", { + id: "card", + selector: "#card", + selectorIndex: 0, + }); + }); + + it("calls the newest reveal callback, not the one from the render that selected", () => { + const stale = vi.fn(); + const fresh = vi.fn(); + const selection = makeSelection("card"); + + act(() => root.render()); + // The callback's identity changes on every edit to the open file, which is + // why it is held in a ref at all. It still has to be the CURRENT one when + // the next selection lands, not the one captured beside the ref. + act(() => root.render()); + act(() => root.render()); + + expect(stale).not.toHaveBeenCalled(); + expect(fresh).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/studio/src/hooks/useDomEditPreviewSync.ts b/packages/studio/src/hooks/useDomEditPreviewSync.ts index 383c136b50..6cd9ae5237 100644 --- a/packages/studio/src/hooks/useDomEditPreviewSync.ts +++ b/packages/studio/src/hooks/useDomEditPreviewSync.ts @@ -137,8 +137,15 @@ export function useDomEditPreviewSync({ // Auto-reveal source when an element is selected while the Code tab is active. // Use a ref for the callback so the effect only fires on selection changes, // not when openSourceForSelection is recreated due to editingFile content updates. + // Written after commit rather than during render: a ref write in the hook body + // is a render side effect and the React Compiler declines the whole hook when + // it sees one. This effect is declared above the reader below, and React runs + // a commit's effects in declaration order, so the reader still sees this + // render's callback. const openSourceRef = useRef(openSourceForSelection); - openSourceRef.current = openSourceForSelection; + useEffect(() => { + openSourceRef.current = openSourceForSelection; + }); useEffect( // fallow-ignore-next-line complexity () => { diff --git a/packages/studio/src/hooks/useDomEditSession.ts b/packages/studio/src/hooks/useDomEditSession.ts index d3d650af07..0a2d43bfc3 100644 --- a/packages/studio/src/hooks/useDomEditSession.ts +++ b/packages/studio/src/hooks/useDomEditSession.ts @@ -309,11 +309,11 @@ export function useDomEditSession({ // the SDK resolves each reordered element (the reorderElements op's targets). onReorderShadow: sdkSession ? (targets: string[]) => { - // Single-flight: every target in one reorder batch shares the same file, so - // memoize the read instead of firing one fetch per unresolved target. + // Single-flight: one file per reorder batch, so memoize the read. Assigned + // inside the `??`, not with `??=`, which the React Compiler cannot lower. let reorderSrcPromise: Promise | undefined; const reorderSrc = activeCompPath - ? () => (reorderSrcPromise ??= readProjectFile(activeCompPath)) + ? () => reorderSrcPromise ?? (reorderSrcPromise = readProjectFile(activeCompPath)) : undefined; for (const target of targets) void recordResolverParity(sdkSession, target, "reorderElements", reorderSrc); diff --git a/packages/studio/src/hooks/useDomSelection.ts b/packages/studio/src/hooks/useDomSelection.ts index 76ca4bca94..4d674fb68f 100644 --- a/packages/studio/src/hooks/useDomSelection.ts +++ b/packages/studio/src/hooks/useDomSelection.ts @@ -64,7 +64,6 @@ export function useDomSelection({ // ── Refs ── const rightPanelTabRef = useRef(rightPanelTab); - rightPanelTabRef.current = rightPanelTab; const domEditSelectionRef = useRef(domEditSelection); const domEditGroupSelectionsRef = useRef(domEditGroupSelections); const domEditHoverSelectionRef = useRef(domEditHoverSelection); @@ -74,11 +73,20 @@ export function useDomSelection({ // resolution land after B and restore the wrong selection. const timelineSelectSeqRef = useRef(0); - // Keep refs in sync with state - domEditSelectionRef.current = domEditSelection; - domEditGroupSelectionsRef.current = domEditGroupSelections; - domEditHoverSelectionRef.current = domEditHoverSelection; - activeGroupElementRef.current = activeGroupElement; + // Keep refs in sync with state. + // + // After commit, not during render: a ref write in the hook body is a render + // side effect and the React Compiler declines the whole hook when it sees one. + // The handlers below already write these refs eagerly, ahead of the state they + // set, so this is the catch-up for a state change that did not come through + // them, and every reader is a handler or an effect declared after this one. + useEffect(() => { + rightPanelTabRef.current = rightPanelTab; + domEditSelectionRef.current = domEditSelection; + domEditGroupSelectionsRef.current = domEditGroupSelections; + domEditHoverSelectionRef.current = domEditHoverSelection; + activeGroupElementRef.current = activeGroupElement; + }); // ── Callbacks ── diff --git a/packages/studio/src/hooks/useElementLifecycleOps.ts b/packages/studio/src/hooks/useElementLifecycleOps.ts index 793ebc237c..46a086b377 100644 --- a/packages/studio/src/hooks/useElementLifecycleOps.ts +++ b/packages/studio/src/hooks/useElementLifecycleOps.ts @@ -110,7 +110,10 @@ export function useElementLifecycleOps({ const sameFile = selections.filter( (candidate) => (candidate.sourceFile || activeCompPath || "index.html") === targetPath, ); - try { + // A `.catch` and not a `try`: the React Compiler cannot lower a `throw` + // inside a `try`/`catch`, and declines the whole hook when it finds one. + // fallow-ignore-next-line complexity + const removeSelection = async (): Promise => { const originalContent = await readProjectFileContent(pid, targetPath); const patchTargets = sameFile.map((member) => buildDomEditPatchTarget(member)); @@ -211,13 +214,14 @@ export function useElementLifecycleOps({ "info", ); return { ok: true } as const; - } catch (error) { + }; + return await removeSelection().catch((error: unknown) => { const message = error instanceof Error ? error.message : "Failed to delete element"; showToast(message); // The toast is what tells the human. The returned outcome is what tells // a caller that has no screen to read. return domEditCommitDeclined("persist-failed"); - } + }); }, [ activeCompPath, @@ -259,7 +263,11 @@ export function useElementLifecycleOps({ // fallow-ignore-next-line complexity return (async () => { const releaseZPersists = entries.map((entry) => beginLayerZPersist(entry.element)); - try { + // `.finally` and `.catch` on the promise, not `try`/`finally` and + // `try`/`catch` statements: the React Compiler can lower neither, and + // declines the whole hook the moment it finds one. + // fallow-ignore-next-line complexity + const reorder = async () => { // Resolver shadow (telemetry-only, decoupled from cutover): record whether // the SDK resolves each reordered element — the reorderElements op's targets. onReorderShadow?.( @@ -361,7 +369,7 @@ export function useElementLifecycleOps({ // inline-style-only — a full iframe remount would only blink the preview. // commitDomEditPatchBatches still falls back to reloading whenever the // server reports an unmatched patch target (live DOM ≠ disk). - try { + const persist = async () => { const result = await commitDomEditPatchBatches(batches, { label: "Reorder layers", coalesceKey, @@ -382,13 +390,15 @@ export function useElementLifecycleOps({ completeLayerRevealCommit(element, ownership); } return result; - } catch (error) { + }; + return await persist().catch((error: unknown) => { rollbackOptimisticState(); throw error; - } - } finally { + }); + }; + return await reorder().finally(() => { for (const release of releaseZPersists) release(); - } + }); })(); }, [commitDomEditPatchBatches, onReorderShadow], diff --git a/packages/studio/src/styles/compiler-bailouts.json b/packages/studio/src/styles/compiler-bailouts.json index 012d362c8f..89f96bcaf1 100644 --- a/packages/studio/src/styles/compiler-bailouts.json +++ b/packages/studio/src/styles/compiler-bailouts.json @@ -1,5 +1,5 @@ { - "total": 126, + "total": 115, "files": { "src/App.tsx": 1, "src/captions/components/CaptionOverlay.tsx": 1, @@ -65,17 +65,6 @@ "src/contexts/DomEditContext.tsx": 1, "src/contexts/TimelineEditContext.tsx": 1, "src/contexts/VariablePromoteContext.tsx": 1, - "src/hooks/timelineAudioGroupCreate.ts": 1, - "src/hooks/useAnimatedPropertyCommit.ts": 1, - "src/hooks/useAppHotkeys.ts": 1, - "src/hooks/useBlockCatalog.ts": 1, - "src/hooks/useBlockHandlers.ts": 1, - "src/hooks/useClipboard.ts": 1, - "src/hooks/useConsoleErrorCapture.ts": 1, - "src/hooks/useDomEditPreviewSync.ts": 1, - "src/hooks/useDomEditSession.ts": 1, - "src/hooks/useDomSelection.ts": 1, - "src/hooks/useElementLifecycleOps.ts": 1, "src/hooks/useElementPicker.ts": 1, "src/hooks/useExternalFileChangeCoordinator.ts": 1, "src/hooks/useFileManager.ts": 1,