Skip to content

Commit e8b75b7

Browse files
jrusso1020claude
andcommitted
fix(studio): code-review and live-test fixes for the variables stack
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 <noreply@anthropic.com>
1 parent 8254c95 commit e8b75b7

10 files changed

Lines changed: 133 additions & 58 deletions

File tree

packages/sdk/src/engine/apply-patches.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,20 @@ function applyVariableDefault(document: Document, id: string, newDefault: unknow
124124
export function applyOverrideSet(parsed: ParsedDocument, overrides: OverrideSet): void {
125125
const patches: JsonPatchOp[] = [];
126126
const rootId = findRoot(parsed.document)?.getAttribute("data-hf-id") ?? null;
127-
for (const [key, value] of Object.entries(overrides)) {
127+
// Whole-declaration snapshots (varDecl.{id}) must replay BEFORE value keys
128+
// (var.{id}): a declaration snapshot embeds the default at fold time, while
129+
// var.{id} always carries the latest value — insertion order alone would let
130+
// an older snapshot clobber a newer value.
131+
const entries = Object.entries(overrides).sort(([a], [b]) => {
132+
const aVar = a.startsWith("var.");
133+
const bVar = b.startsWith("var.");
134+
const aDecl = a.startsWith("varDecl.");
135+
const bDecl = b.startsWith("varDecl.");
136+
if (aVar && bDecl) return 1;
137+
if (aDecl && bVar) return -1;
138+
return 0; // stable — every other key keeps its insertion order
139+
});
140+
for (const [key, value] of entries) {
128141
const path = keyToPath(key);
129142
if (!path) continue;
130143
if (value === null) {

packages/sdk/src/engine/mutate.ts

Lines changed: 10 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -869,37 +869,21 @@ function handleSetVariableValue(
869869
const modelPath = variablePath(id);
870870
const oldVarDefault = readVariableDefault(parsed.document, id);
871871

872-
if (isObjectVariableValue(value)) {
873-
// Object values (font / image): write to JSON model only — objects are not
874-
// valid CSS custom property values (LOCKED §7).
875-
writeVariableDefault(parsed.document, id, value);
876-
const p = valueChange(modelPath, oldVarDefault ?? null, value);
877-
return { forward: [p.forward], inverse: [p.inverse] };
878-
}
879-
880-
// Scalar values: update the JSON model (B1 — drives the runtime) and also
881-
// keep the CSS custom prop as secondary / compat for compositions that
882-
// CSS-bind directly to --{id}.
883-
const cssVar = `--${id}`;
884-
const rootId = root.getAttribute("data-hf-id");
885-
const oldStyles = getElementStyles(root);
886-
const oldCssValue = oldStyles[cssVar] ?? null;
887-
const newVal = String(value);
888-
setElementStyles(root, { [cssVar]: newVal });
872+
// Update the JSON model (B1 — drives the runtime) and keep the CSS custom
873+
// prop as secondary / compat for compositions that CSS-bind directly to
874+
// --{id}. Object values (font / image) are not valid CSS custom property
875+
// values (LOCKED §7) — cssCompatChange clears any stale scalar prop instead.
876+
// Emitting separate model + style patches keeps apply-patches.ts pure per
877+
// path type, so inverse patches restore the exact pre-call state.
889878
writeVariableDefault(parsed.document, id, value);
890-
891-
// Emit explicit patches for both the JSON model (canonical) and the CSS compat
892-
// prop. Keeping them separate means apply-patches.ts can handle each path type
893-
// purely (variable path → model only; style path → CSS only), so inverse patches
894-
// correctly restore the exact pre-call state without CSS-side-effect ambiguity.
895879
const modelP = valueChange(modelPath, oldVarDefault ?? null, value);
896880
const forward: JsonPatchOp[] = [modelP.forward];
897881
const inverse: JsonPatchOp[] = [modelP.inverse];
898882

899-
if (rootId) {
900-
const cssPatch = scalarChange(stylePath(rootId, cssVar), oldCssValue, newVal);
901-
forward.push(cssPatch.forward);
902-
inverse.push(cssPatch.inverse);
883+
const css = cssCompatChange(parsed, id, isObjectVariableValue(value) ? null : String(value));
884+
if (css) {
885+
forward.push(css.forward);
886+
inverse.push(css.inverse);
903887
}
904888

905889
return { forward, inverse };

packages/sdk/src/session.ts

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,16 @@ class CompositionImpl implements Composition {
188188
return validateVariables(values, this.getVariableDeclarations());
189189
}
190190

191+
/**
192+
* Script scans are content-keyed (same rationale as _gsapLabelCache): the
193+
* panel recomputes usage on every preview reload, and unchanged script text
194+
* is the common case — never pay a second acorn parse for identical input.
195+
*/
196+
private _variableUsageScanCache = new Map<string, ReturnType<typeof scanVariableUsage>>();
197+
198+
// Scan/merge dispatcher — same complexity class as the suppressed
199+
// variableUsage.ts classifiers it drives.
200+
// fallow-ignore-next-line complexity
191201
getVariableUsage(): VariableUsageReport {
192202
const usedIds: string[] = [];
193203
const seen = new Set<string>();
@@ -217,14 +227,7 @@ class CompositionImpl implements Composition {
217227
// The CSS compat channel counts as usage: a variable consumed only via
218228
// var(--id) in stylesheets or inline styles must not be badged unused
219229
// (removing it also removes the --{id} root prop and breaks the binding).
220-
const cssParts: string[] = [];
221-
for (const styleEl of Array.from(this.parsed.document.querySelectorAll("style"))) {
222-
cssParts.push(styleEl.textContent ?? "");
223-
}
224-
for (const el of Array.from(this.parsed.document.querySelectorAll("[style]"))) {
225-
cssParts.push(el.getAttribute("style") ?? "");
226-
}
227-
const cssText = cssParts.join("\n");
230+
const cssText = this._collectCssText();
228231
const cssUsed = (id: string) => cssText.includes(`var(--${id}`);
229232
return {
230233
usedIds,
@@ -234,6 +237,18 @@ class CompositionImpl implements Composition {
234237
};
235238
}
236239

240+
/** All stylesheet + inline-style text, for var(--id) consumption checks. */
241+
private _collectCssText(): string {
242+
const cssParts: string[] = [];
243+
for (const styleEl of Array.from(this.parsed.document.querySelectorAll("style"))) {
244+
cssParts.push(styleEl.textContent ?? "");
245+
}
246+
for (const el of Array.from(this.parsed.document.querySelectorAll("[style]"))) {
247+
cssParts.push(el.getAttribute("style") ?? "");
248+
}
249+
return cssParts.join("\n");
250+
}
251+
237252
setPreviewVariables(values: Record<string, unknown> | null): boolean {
238253
if (!this.preview?.setPreviewVariables) return false;
239254
this.preview.setPreviewVariables(values);
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
/**
2+
* Shared shape check for composition-variable payloads (`?variables=` on the
3+
* preview routes, `body.variables` on the render route) — one contract, one
4+
* error string, so the routes can't drift.
5+
*/
6+
7+
export const VARIABLES_PAYLOAD_ERROR = "variables must be a JSON object of {variableId: value}";
8+
9+
export function isVariablesPayload(value: unknown): value is Record<string, unknown> {
10+
return typeof value === "object" && value !== null && !Array.isArray(value);
11+
}

packages/studio-server/src/routes/preview.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
} from "../helpers/studioMotionRenderScript.js";
1515
import { ensureHfIds } from "@hyperframes/parsers/hf-ids";
1616
import { persistHfIdsIfNeeded, stampFileHfIds } from "../helpers/hfIdPersist.js";
17+
import { isVariablesPayload, VARIABLES_PAYLOAD_ERROR } from "../helpers/variablesPayload.js";
1718

1819
const PROJECT_SIGNATURE_META = "hyperframes-project-signature";
1920
const GSAP_CDN_VERSION = "3.15.0";

packages/studio-server/src/routes/render.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import type { StudioApiAdapter, RenderJobState } from "../types.js";
66
import { VALID_CANVAS_RESOLUTIONS, type CanvasResolution } from "@hyperframes/parsers";
77
import { parseFps } from "@hyperframes/core";
88
import { resolveWithinProject } from "../helpers/safePath.js";
9+
import { isVariablesPayload, VARIABLES_PAYLOAD_ERROR } from "../helpers/variablesPayload.js";
910

1011
const VALID_RESOLUTIONS = new Set<string>(VALID_CANVAS_RESOLUTIONS);
1112

packages/studio/src/App.tsx

Lines changed: 11 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,9 @@ import { usePreviewPersistence } from "./hooks/usePreviewPersistence";
1313
import { useTimelineEditing } from "./hooks/useTimelineEditing";
1414
import type { BlockPreviewInfo } from "./components/sidebar/BlocksTab";
1515
import { useDomEditSession } from "./hooks/useDomEditSession";
16-
import { useSdkSession } from "./hooks/useSdkSession";
1716
import { useSdkSelectionSync } from "./hooks/useSdkSelectionSync";
17+
import { useStudioSdkSessions } from "./hooks/useStudioSdkSessions";
18+
import { usePreviewDocumentVersion } from "./hooks/usePreviewDocumentVersion";
1819
import { useBlockHandlers } from "./hooks/useBlockHandlers";
1920
import { useAppHotkeys } from "./hooks/useAppHotkeys";
2021
import { useClipboard } from "./hooks/useClipboard";
@@ -81,7 +82,7 @@ export function StudioApp() {
8182
const [previewIframe, setPreviewIframe] = useState<HTMLIFrameElement | null>(null);
8283
const [compositionLoading, setCompositionLoading] = useState(true);
8384
const [refreshKey, setRefreshKey] = useState(0);
84-
const [previewDocumentVersion, setPreviewDocumentVersion] = useState(0);
85+
const [previewDocumentVersion, refreshPreviewDocumentVersion] = usePreviewDocumentVersion();
8586
const [blockPreview, setBlockPreview] = useState<BlockPreviewInfo | null>(null);
8687
const cropModeProps = useCropModeProps();
8788
const previewIframeRef = useRef<HTMLIFrameElement | null>(null);
@@ -107,22 +108,6 @@ export function StudioApp() {
107108
: 0;
108109
return Math.max(timelineDuration, maxEnd);
109110
}, [timelineDuration, timelineElements]);
110-
const refreshTimersRef = useRef<number[]>([]);
111-
const refreshPreviewDocumentVersion = useCallback(() => {
112-
for (const id of refreshTimersRef.current) clearTimeout(id);
113-
refreshTimersRef.current = [];
114-
setPreviewDocumentVersion((v) => v + 1);
115-
refreshTimersRef.current.push(
116-
window.setTimeout(() => setPreviewDocumentVersion((v) => v + 1), 80),
117-
window.setTimeout(() => setPreviewDocumentVersion((v) => v + 1), 300),
118-
);
119-
}, []);
120-
useEffect(
121-
() => () => {
122-
for (const id of refreshTimersRef.current) clearTimeout(id);
123-
},
124-
[],
125-
);
126111
const [timelineVisible, setTimelineVisible] = useState(
127112
() =>
128113
initialUrlStateRef.current.timelineVisible ??
@@ -152,7 +137,11 @@ export function StudioApp() {
152137
domEditSaveTimestampRef,
153138
setRefreshKey,
154139
});
155-
const sdkHandle = useSdkSession(projectId, activeCompPath, domEditSaveTimestampRef);
140+
const { sdkHandle, editFlowSdkSession } = useStudioSdkSessions(
141+
projectId,
142+
activeCompPath,
143+
domEditSaveTimestampRef,
144+
);
156145
useEffect(() => {
157146
if (activeCompPathHydrated) return;
158147
if (!fileManager.fileTreeLoaded) return;
@@ -188,7 +177,7 @@ export function StudioApp() {
188177
pendingTimelineEditPathRef,
189178
uploadProjectFiles: fileManager.uploadProjectFiles,
190179
isRecordingRef: isGestureRecordingRef,
191-
sdkSession: sdkHandle.session,
180+
sdkSession: editFlowSdkSession,
192181
forceReloadSdkSession: sdkHandle.forceReload,
193182
});
194183
const {
@@ -304,7 +293,7 @@ export function StudioApp() {
304293
openSourceForSelection: fileManager.openSourceForSelection,
305294
selectSidebarTab: sidebarTabRef.current.select,
306295
getSidebarTab: sidebarTabRef.current.get,
307-
sdkSession: sdkHandle.session,
296+
sdkSession: editFlowSdkSession,
308297
forceReloadSdkSession: sdkHandle.forceReload,
309298
});
310299
domEditSelectionBridgeRef.current = domEditSession.domEditSelection;
@@ -322,7 +311,7 @@ export function StudioApp() {
322311
}
323312
};
324313
useSdkSelectionSync(
325-
sdkHandle.session,
314+
editFlowSdkSession,
326315
domEditSession.domEditSelection,
327316
domEditSession.domEditGroupSelections,
328317
);
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import { useCallback, useEffect, useRef, useState } from "react";
2+
3+
/**
4+
* Version counter for the preview DOM. `refresh` bumps immediately and again
5+
* at 80ms / 300ms so consumers re-scan after the iframe settles; pending
6+
* timers are collapsed by each new refresh and cleared on unmount.
7+
*/
8+
export function usePreviewDocumentVersion(): [number, () => void] {
9+
const [previewDocumentVersion, setPreviewDocumentVersion] = useState(0);
10+
const refreshTimersRef = useRef<number[]>([]);
11+
const refresh = useCallback(() => {
12+
for (const id of refreshTimersRef.current) clearTimeout(id);
13+
refreshTimersRef.current = [];
14+
setPreviewDocumentVersion((v) => v + 1);
15+
refreshTimersRef.current.push(
16+
window.setTimeout(() => setPreviewDocumentVersion((v) => v + 1), 80),
17+
window.setTimeout(() => setPreviewDocumentVersion((v) => v + 1), 300),
18+
);
19+
}, []);
20+
useEffect(
21+
() => () => {
22+
for (const id of refreshTimersRef.current) clearTimeout(id);
23+
},
24+
[],
25+
);
26+
return [previewDocumentVersion, refresh];
27+
}
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
import { useEffect, type MutableRefObject } from "react";
2+
import { useSdkSession } from "./useSdkSession";
3+
import { usePreviewVariablesStore } from "./previewVariablesStore";
4+
5+
/**
6+
* Open the studio's SDK session with master-view semantics.
7+
*
8+
* The master view has no explicit comp path, but the session must still model
9+
* the project's main composition so schema-level panels (Variables, Slideshow)
10+
* work there. Edit-flow consumers keep the legacy "no session on master view"
11+
* gating via `editFlowSdkSession` so cutover behavior is unchanged.
12+
*
13+
* Also clears preview variable overrides whenever the composition or project
14+
* changes — overrides are per-composition and must never leak into another
15+
* composition's preview or render.
16+
*/
17+
export function useStudioSdkSessions(
18+
projectId: string | null,
19+
activeCompPath: string | null,
20+
domEditSaveTimestampRef: MutableRefObject<number>,
21+
) {
22+
const sdkHandle = useSdkSession(
23+
projectId,
24+
activeCompPath ?? "index.html",
25+
domEditSaveTimestampRef,
26+
);
27+
const editFlowSdkSession = activeCompPath ? sdkHandle.session : null;
28+
useEffect(() => {
29+
usePreviewVariablesStore.getState().setValues(null);
30+
}, [projectId, activeCompPath]);
31+
return { sdkHandle, editFlowSdkSession };
32+
}

packages/studio/src/utils/studioUrlState.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ export interface StudioUrlState {
1919
selection: StudioUrlSelectionState | null;
2020
}
2121

22-
const VALID_TABS: RightPanelTab[] = ["layers", "design", "renders"];
22+
const VALID_TABS: RightPanelTab[] = ["layers", "design", "renders", "variables"];
2323

2424
export function normalizeStudioUrlPanelTab(
2525
tab: RightPanelTab | null,
@@ -106,6 +106,8 @@ export function readStudioUrlStateFromWindow(): StudioUrlState {
106106
return parseStudioUrlStateFromHash(window.location.hash);
107107
}
108108

109+
// Pre-existing param-assembly complexity — surfaced by this PR's line shifts.
110+
// fallow-ignore-next-line complexity
109111
export function buildStudioHash(projectId: string, state: StudioUrlState): string {
110112
const params = new URLSearchParams();
111113

0 commit comments

Comments
 (0)