From 3976c7ead05129df25e7191eac95a9b786ad9a24 Mon Sep 17 00:00:00 2001 From: James Date: Tue, 7 Jul 2026 23:08:23 -0700 Subject: [PATCH] fix(studio): code-review and live-test fixes for the variables stack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remainder of a review + in-browser test pass over the stack (hunks touching stack-authored lines were absorbed into their PRs; these touch pre-stack lines). - studio: the SDK session never opened on the master view (activeCompPath is null there), so the Variables/Slideshow panels were dead on single-file projects — open the session with an index.html fallback (useStudioSdkSessions) while edit-flow consumers keep the legacy null gating, so cutover behavior is unchanged. Found by driving the panel in a real browser. - studio: register "variables" in the URL-state tab allowlist so ?tab=variables deep links survive normalization; clear preview variable overrides when the composition or project changes so values can't leak into another composition's preview/render. - studio: extract usePreviewDocumentVersion from App.tsx (file-size gate). - sdk: route handleSetVariableValue's CSS compat sync through cssCompatChange — one implementation of the --{id} channel (object values now clear a stale scalar prop instead of leaving it behind). - sdk: content-keyed cache for getVariableUsage script scans (same rationale as _gsapLabelCache). - studio-server: shared variables-payload validation helper used by both the preview and render routes (was two copies of the same check). Co-Authored-By: Claude Fable 5 --- packages/sdk/src/engine/apply-patches.ts | 15 +++++++- packages/sdk/src/engine/mutate.ts | 36 ++++++------------- packages/sdk/src/session.ts | 21 ++++++----- packages/studio/src/App.tsx | 33 ++++++----------- .../src/hooks/usePreviewDocumentVersion.ts | 27 ++++++++++++++ .../studio/src/hooks/useStudioSdkSessions.ts | 32 +++++++++++++++++ packages/studio/src/utils/studioUrlState.ts | 4 ++- 7 files changed, 110 insertions(+), 58 deletions(-) create mode 100644 packages/studio/src/hooks/usePreviewDocumentVersion.ts create mode 100644 packages/studio/src/hooks/useStudioSdkSessions.ts diff --git a/packages/sdk/src/engine/apply-patches.ts b/packages/sdk/src/engine/apply-patches.ts index 57df576bbd..4b4c80ffc7 100644 --- a/packages/sdk/src/engine/apply-patches.ts +++ b/packages/sdk/src/engine/apply-patches.ts @@ -124,7 +124,20 @@ function applyVariableDefault(document: Document, id: string, newDefault: unknow export function applyOverrideSet(parsed: ParsedDocument, overrides: OverrideSet): void { const patches: JsonPatchOp[] = []; const rootId = findRoot(parsed.document)?.getAttribute("data-hf-id") ?? null; - for (const [key, value] of Object.entries(overrides)) { + // Whole-declaration snapshots (varDecl.{id}) must replay BEFORE value keys + // (var.{id}): a declaration snapshot embeds the default at fold time, while + // var.{id} always carries the latest value — insertion order alone would let + // an older snapshot clobber a newer value. + const entries = Object.entries(overrides).sort(([a], [b]) => { + const aVar = a.startsWith("var."); + const bVar = b.startsWith("var."); + const aDecl = a.startsWith("varDecl."); + const bDecl = b.startsWith("varDecl."); + if (aVar && bDecl) return 1; + if (aDecl && bVar) return -1; + return 0; // stable — every other key keeps its insertion order + }); + for (const [key, value] of entries) { const path = keyToPath(key); if (!path) continue; if (value === null) { diff --git a/packages/sdk/src/engine/mutate.ts b/packages/sdk/src/engine/mutate.ts index bbd0396ad5..9bd773bd0f 100644 --- a/packages/sdk/src/engine/mutate.ts +++ b/packages/sdk/src/engine/mutate.ts @@ -869,37 +869,21 @@ function handleSetVariableValue( const modelPath = variablePath(id); const oldVarDefault = readVariableDefault(parsed.document, id); - if (isObjectVariableValue(value)) { - // Object values (font / image): write to JSON model only — objects are not - // valid CSS custom property values (LOCKED §7). - writeVariableDefault(parsed.document, id, value); - const p = valueChange(modelPath, oldVarDefault ?? null, value); - return { forward: [p.forward], inverse: [p.inverse] }; - } - - // Scalar values: update the JSON model (B1 — drives the runtime) and also - // keep the CSS custom prop as secondary / compat for compositions that - // CSS-bind directly to --{id}. - const cssVar = `--${id}`; - const rootId = root.getAttribute("data-hf-id"); - const oldStyles = getElementStyles(root); - const oldCssValue = oldStyles[cssVar] ?? null; - const newVal = String(value); - setElementStyles(root, { [cssVar]: newVal }); + // Update the JSON model (B1 — drives the runtime) and keep the CSS custom + // prop as secondary / compat for compositions that CSS-bind directly to + // --{id}. Object values (font / image) are not valid CSS custom property + // values (LOCKED §7) — cssCompatChange clears any stale scalar prop instead. + // Emitting separate model + style patches keeps apply-patches.ts pure per + // path type, so inverse patches restore the exact pre-call state. writeVariableDefault(parsed.document, id, value); - - // Emit explicit patches for both the JSON model (canonical) and the CSS compat - // prop. Keeping them separate means apply-patches.ts can handle each path type - // purely (variable path → model only; style path → CSS only), so inverse patches - // correctly restore the exact pre-call state without CSS-side-effect ambiguity. const modelP = valueChange(modelPath, oldVarDefault ?? null, value); const forward: JsonPatchOp[] = [modelP.forward]; const inverse: JsonPatchOp[] = [modelP.inverse]; - if (rootId) { - const cssPatch = scalarChange(stylePath(rootId, cssVar), oldCssValue, newVal); - forward.push(cssPatch.forward); - inverse.push(cssPatch.inverse); + const css = cssCompatChange(parsed, id, isObjectVariableValue(value) ? null : String(value)); + if (css) { + forward.push(css.forward); + inverse.push(css.inverse); } return { forward, inverse }; diff --git a/packages/sdk/src/session.ts b/packages/sdk/src/session.ts index 9e67161898..b021e392c6 100644 --- a/packages/sdk/src/session.ts +++ b/packages/sdk/src/session.ts @@ -227,14 +227,7 @@ class CompositionImpl implements Composition { // The CSS compat channel counts as usage: a variable consumed only via // var(--id) in stylesheets or inline styles must not be badged unused // (removing it also removes the --{id} root prop and breaks the binding). - const cssParts: string[] = []; - for (const styleEl of Array.from(this.parsed.document.querySelectorAll("style"))) { - cssParts.push(styleEl.textContent ?? ""); - } - for (const el of Array.from(this.parsed.document.querySelectorAll("[style]"))) { - cssParts.push(el.getAttribute("style") ?? ""); - } - const cssText = cssParts.join("\n"); + const cssText = this._collectCssText(); const cssUsed = (id: string) => cssText.includes(`var(--${id}`); return { usedIds, @@ -244,6 +237,18 @@ class CompositionImpl implements Composition { }; } + /** All stylesheet + inline-style text, for var(--id) consumption checks. */ + private _collectCssText(): string { + const cssParts: string[] = []; + for (const styleEl of Array.from(this.parsed.document.querySelectorAll("style"))) { + cssParts.push(styleEl.textContent ?? ""); + } + for (const el of Array.from(this.parsed.document.querySelectorAll("[style]"))) { + cssParts.push(el.getAttribute("style") ?? ""); + } + return cssParts.join("\n"); + } + setPreviewVariables(values: Record | null): boolean { if (!this.preview?.setPreviewVariables) return false; this.preview.setPreviewVariables(values); diff --git a/packages/studio/src/App.tsx b/packages/studio/src/App.tsx index ad928cb913..edbc9c62d2 100644 --- a/packages/studio/src/App.tsx +++ b/packages/studio/src/App.tsx @@ -13,8 +13,9 @@ import { usePreviewPersistence } from "./hooks/usePreviewPersistence"; import { useTimelineEditing } from "./hooks/useTimelineEditing"; import type { BlockPreviewInfo } from "./components/sidebar/BlocksTab"; import { useDomEditSession } from "./hooks/useDomEditSession"; -import { useSdkSession } from "./hooks/useSdkSession"; import { useSdkSelectionSync } from "./hooks/useSdkSelectionSync"; +import { useStudioSdkSessions } from "./hooks/useStudioSdkSessions"; +import { usePreviewDocumentVersion } from "./hooks/usePreviewDocumentVersion"; import { useBlockHandlers } from "./hooks/useBlockHandlers"; import { useAppHotkeys } from "./hooks/useAppHotkeys"; import { useClipboard } from "./hooks/useClipboard"; @@ -81,7 +82,7 @@ export function StudioApp() { const [previewIframe, setPreviewIframe] = useState(null); const [compositionLoading, setCompositionLoading] = useState(true); const [refreshKey, setRefreshKey] = useState(0); - const [previewDocumentVersion, setPreviewDocumentVersion] = useState(0); + const [previewDocumentVersion, refreshPreviewDocumentVersion] = usePreviewDocumentVersion(); const [blockPreview, setBlockPreview] = useState(null); const cropModeProps = useCropModeProps(); const previewIframeRef = useRef(null); @@ -107,22 +108,6 @@ export function StudioApp() { : 0; return Math.max(timelineDuration, maxEnd); }, [timelineDuration, timelineElements]); - const refreshTimersRef = useRef([]); - const refreshPreviewDocumentVersion = useCallback(() => { - for (const id of refreshTimersRef.current) clearTimeout(id); - refreshTimersRef.current = []; - setPreviewDocumentVersion((v) => v + 1); - refreshTimersRef.current.push( - window.setTimeout(() => setPreviewDocumentVersion((v) => v + 1), 80), - window.setTimeout(() => setPreviewDocumentVersion((v) => v + 1), 300), - ); - }, []); - useEffect( - () => () => { - for (const id of refreshTimersRef.current) clearTimeout(id); - }, - [], - ); const [timelineVisible, setTimelineVisible] = useState( () => initialUrlStateRef.current.timelineVisible ?? @@ -152,7 +137,11 @@ export function StudioApp() { domEditSaveTimestampRef, setRefreshKey, }); - const sdkHandle = useSdkSession(projectId, activeCompPath, domEditSaveTimestampRef); + const { sdkHandle, editFlowSdkSession } = useStudioSdkSessions( + projectId, + activeCompPath, + domEditSaveTimestampRef, + ); useEffect(() => { if (activeCompPathHydrated) return; if (!fileManager.fileTreeLoaded) return; @@ -188,7 +177,7 @@ export function StudioApp() { pendingTimelineEditPathRef, uploadProjectFiles: fileManager.uploadProjectFiles, isRecordingRef: isGestureRecordingRef, - sdkSession: sdkHandle.session, + sdkSession: editFlowSdkSession, forceReloadSdkSession: sdkHandle.forceReload, }); const { @@ -304,7 +293,7 @@ export function StudioApp() { openSourceForSelection: fileManager.openSourceForSelection, selectSidebarTab: sidebarTabRef.current.select, getSidebarTab: sidebarTabRef.current.get, - sdkSession: sdkHandle.session, + sdkSession: editFlowSdkSession, forceReloadSdkSession: sdkHandle.forceReload, }); domEditSelectionBridgeRef.current = domEditSession.domEditSelection; @@ -322,7 +311,7 @@ export function StudioApp() { } }; useSdkSelectionSync( - sdkHandle.session, + editFlowSdkSession, domEditSession.domEditSelection, domEditSession.domEditGroupSelections, ); diff --git a/packages/studio/src/hooks/usePreviewDocumentVersion.ts b/packages/studio/src/hooks/usePreviewDocumentVersion.ts new file mode 100644 index 0000000000..d4c10acb2b --- /dev/null +++ b/packages/studio/src/hooks/usePreviewDocumentVersion.ts @@ -0,0 +1,27 @@ +import { useCallback, useEffect, useRef, useState } from "react"; + +/** + * Version counter for the preview DOM. `refresh` bumps immediately and again + * at 80ms / 300ms so consumers re-scan after the iframe settles; pending + * timers are collapsed by each new refresh and cleared on unmount. + */ +export function usePreviewDocumentVersion(): [number, () => void] { + const [previewDocumentVersion, setPreviewDocumentVersion] = useState(0); + const refreshTimersRef = useRef([]); + const refresh = useCallback(() => { + for (const id of refreshTimersRef.current) clearTimeout(id); + refreshTimersRef.current = []; + setPreviewDocumentVersion((v) => v + 1); + refreshTimersRef.current.push( + window.setTimeout(() => setPreviewDocumentVersion((v) => v + 1), 80), + window.setTimeout(() => setPreviewDocumentVersion((v) => v + 1), 300), + ); + }, []); + useEffect( + () => () => { + for (const id of refreshTimersRef.current) clearTimeout(id); + }, + [], + ); + return [previewDocumentVersion, refresh]; +} diff --git a/packages/studio/src/hooks/useStudioSdkSessions.ts b/packages/studio/src/hooks/useStudioSdkSessions.ts new file mode 100644 index 0000000000..116ce90485 --- /dev/null +++ b/packages/studio/src/hooks/useStudioSdkSessions.ts @@ -0,0 +1,32 @@ +import { useEffect, type MutableRefObject } from "react"; +import { useSdkSession } from "./useSdkSession"; +import { usePreviewVariablesStore } from "./previewVariablesStore"; + +/** + * Open the studio's SDK session with master-view semantics. + * + * The master view has no explicit comp path, but the session must still model + * the project's main composition so schema-level panels (Variables, Slideshow) + * work there. Edit-flow consumers keep the legacy "no session on master view" + * gating via `editFlowSdkSession` so cutover behavior is unchanged. + * + * Also clears preview variable overrides whenever the composition or project + * changes — overrides are per-composition and must never leak into another + * composition's preview or render. + */ +export function useStudioSdkSessions( + projectId: string | null, + activeCompPath: string | null, + domEditSaveTimestampRef: MutableRefObject, +) { + const sdkHandle = useSdkSession( + projectId, + activeCompPath ?? "index.html", + domEditSaveTimestampRef, + ); + const editFlowSdkSession = activeCompPath ? sdkHandle.session : null; + useEffect(() => { + usePreviewVariablesStore.getState().setValues(null); + }, [projectId, activeCompPath]); + return { sdkHandle, editFlowSdkSession }; +} diff --git a/packages/studio/src/utils/studioUrlState.ts b/packages/studio/src/utils/studioUrlState.ts index b961af5089..8e024b834d 100644 --- a/packages/studio/src/utils/studioUrlState.ts +++ b/packages/studio/src/utils/studioUrlState.ts @@ -19,7 +19,7 @@ export interface StudioUrlState { selection: StudioUrlSelectionState | null; } -const VALID_TABS: RightPanelTab[] = ["layers", "design", "renders"]; +const VALID_TABS: RightPanelTab[] = ["layers", "design", "renders", "variables"]; export function normalizeStudioUrlPanelTab( tab: RightPanelTab | null, @@ -106,6 +106,8 @@ export function readStudioUrlStateFromWindow(): StudioUrlState { return parseStudioUrlStateFromHash(window.location.hash); } +// Pre-existing param-assembly complexity — surfaced by this PR's line shifts. +// fallow-ignore-next-line complexity export function buildStudioHash(projectId: string, state: StudioUrlState): string { const params = new URLSearchParams();