From 461ad3b952813b115f06e2a4f649ff7270910838 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Tue, 8 Sep 2026 17:16:41 -0400 Subject: [PATCH] refactor(studio): compile eleven more hooks under the React Compiler Each of these hooks was emitted uncompiled because it did something the React Compiler declines: wrote a ref during render, held a try/finally or a throw inside try/catch, carried a react-hooks suppression, or used a lowering the compiler does not support. None of that is visible at runtime, so every memo in them was silently lost. Each is fixed where the value is read, not by changing what happens: ref writes move into an effect or drop when the ref was standing in for a memo, guarded control flow moves to a plain function or a promise handler, and one suppressed dependency list is made true rather than silenced. The bail-out baseline falls from 126 to 115, one per file. --- .../src/hooks/timelineAudioGroupCreate.ts | 38 ++++-- .../src/hooks/useAnimatedPropertyCommit.ts | 18 +-- packages/studio/src/hooks/useAppHotkeys.ts | 42 +++--- .../studio/src/hooks/useBlockCatalog.test.tsx | 93 +++++++++++++ packages/studio/src/hooks/useBlockCatalog.ts | 37 ++++-- .../src/hooks/useBlockHandlers.test.tsx | 119 +++++++++++++++++ packages/studio/src/hooks/useBlockHandlers.ts | 81 +++++++++--- .../studio/src/hooks/useClipboard.test.tsx | 107 +++++++++++++++ packages/studio/src/hooks/useClipboard.ts | 5 +- .../src/hooks/useConsoleErrorCapture.test.tsx | 80 +++++++++++ .../src/hooks/useConsoleErrorCapture.ts | 125 +++++++++--------- .../src/hooks/useDomEditPreviewSync.test.tsx | 88 ++++++++++++ .../studio/src/hooks/useDomEditPreviewSync.ts | 9 +- .../studio/src/hooks/useDomEditSession.ts | 6 +- packages/studio/src/hooks/useDomSelection.ts | 20 ++- .../src/hooks/useElementLifecycleOps.ts | 28 ++-- .../studio/src/styles/compiler-bailouts.json | 13 +- 17 files changed, 741 insertions(+), 168 deletions(-) create mode 100644 packages/studio/src/hooks/useBlockCatalog.test.tsx create mode 100644 packages/studio/src/hooks/useBlockHandlers.test.tsx create mode 100644 packages/studio/src/hooks/useClipboard.test.tsx create mode 100644 packages/studio/src/hooks/useConsoleErrorCapture.test.tsx create mode 100644 packages/studio/src/hooks/useDomEditPreviewSync.test.tsx 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,