feat(studio): instant, flicker-free manual editing#1605
Conversation
b2b2e98 to
decff5d
Compare
a60f8c8 to
1ea09b1
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 1ea09b1d. The four commits compose well and each has a clear story (drop escalation, harden verify + bootstrap plugin, break re-save loop, inject plugin server-side). Race-fix classification for the headline "flicker-free" claim: MOVE — the visible flash from reloadPreview() is structurally eliminated for the value-only path (the iframe never remounts), but a new failure mode is introduced: a silently stale preview when both the instant patch AND the soft reload report failure. The defense (live gsap.set already shows the value) is correct for the current playhead and shaky for any seek that follows. Details below.
Concerns
-
Silent staleness when
applySoftReloadreturnsfalsefor a non-transient reason. The PR justifies dropping the→ reloadPreview()escalation by citing transientverifyTimelinesPopulatedflakes (now hardened by scoping to expectedtargetKeys). ButapplySoftReloadreturnsfalsefor several non-transient reasons too:!win.gsap,!win.__hfForceTimelineRebind,targetKeys.length === 0(script has no parseable__timelines["…"]key),gsapScripts.length === 0(live DOM has no GSAP<script>). These are now silently swallowed — the source file is current, the panel cache is invalidated, but the iframe's runtime is stale until the user happens to trigger a full reload some other way. At minimum, log a warning + emit a one-shot telemetry event fromapplySoftReloadreturningfalseso we'd see this regress in the field. Even better: keep the escalation for the permanent-failure signals (those fourfalsepaths above) and only drop it for the verify-failed path that motivates the change. Today's binarybooleancollapses transient vs permanent into one signal — worth differentiating. -
Observability gap on the fast-path-failed case (carries over from #1613). When
patchRuntimeTweenInPlacereturnsfalse, the code silently falls through toapplySoftReload. Combined with the above, apatch:false → soft:falsesequence now produces zero visible signal and zero telemetry. Add a counter (PostHog or log) forinstantPatch_failedandsoftReload_failedso we can verify the U4 invariant ("live state is already correct") holds in production, not just in the unit test fixture. -
ensureMotionPathPluginLoadedrace window. The bootstrap runs ononIframeLoad. Between__hfMotionPathPluginLoading = trueandpluginScript.onload, a user-triggered soft reload that needs the plugin will see!win.MotionPathPluginANDwin.__hfMotionPathPluginLoading === true→ the soft-reload's own async-load fallback (needsMotionPath && !win.MotionPathPlugin && win.gsap) fires, queuing a second<script>tag with the same URL. Both eventually load (browser caches the second), bothexecuteScript()callbacks run — the second one tears down + re-executes the script the first one just registered. Idempotent but wasteful and visually a re-flash. The bootstrap's__hfMotionPathPluginLoadingflag should be honored byapplySoftReload's async path too (e.g.if (win.__hfMotionPathPluginLoading) wait-for-itinstead of queue-another). -
SourceEditorExternalSyncsemantics. The annotation correctly suppresses theonChangecallback on programmatic syncs — good. But theuseEffectonly dispatches whencurrent !== content(the textual compare). After a manual-edit commit, the source file gets written server-side,onFileContentChanged?.(targetPath, after)fires, the editor'scontentprop updates, the effect re-runs and dispatches the new doc taggedExternalSync. Fine. However: if the user is mid-typing when the commit lands (rare but possible — they kicked off a drag, then started typing in the editor before the commit returned), thecurrentis the user's in-flight typed state,contentis the server-write — they differ, so the effect replaces the user's typing wholesale, taggedExternalSync, AND noonChangefires (because of the annotation). The user's in-flight keystrokes are silently lost without a save. Edge case but recoverable — worth either docstring + tracking, or a quick "did the editor have focus?" check before the replace.
Nits
injectMotionPathPluginIfNeededuses regexgsap@([\d.]+)to extract the composition's gsap version. If the comp ships gsap from a non-@-versioned URL (e.g. self-hosted/static/gsap.min.js),versionfalls back toGSAP_CDN_VERSION(3.15.0) — which works, but mismatches the comp's actual gsap if it's a different version. Acceptable for now; just a known soft skew. (nit)- The 87-line growth of
gsapSoftReload.tsputs it past the file's prior surface area; might be worth splittingensureMotionPathPluginLoaded+ the CDN constant into a sibling module before the next addition pushes against the 600-line cap. (nit) applyPreviewSyncdoc comment in #1613 says "fall back to existing soft/full reload"; this PR drops the→ full reloadhalf. Update the comment in this PR's file too. (nit)
What I didn't verify
- The actual cross-stack diff against main vs the stack tip —
gh pr diffshows the per-PR slice and I trusted the PR body's claim that the combined stack diff is byte-identical to the originally-reviewed work. - That
__hfForceTimelineRebindis guaranteed to be present on the preview iframe by the time a studio edit commits (the!win.__hfForceTimelineRebind → return falsepath is one of the silent-failure modes I flagged above). - The full keyboard interaction matrix for the SourceEditor mid-typing race — I traced the effect's textual compare path but didn't reproduce.
— Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 1ea09b1d. Concur with @james-russo-rames-d-jusso — the four commits compose well and each has a clear story, but the escalation drop's defense is scoped to the instantPatch-ran case while the drop applies universally. That's the load-bearing silent-staleness blocker.
Verified at HEAD:
ExternalSyncannotation inSourceEditor.tsx: only theview.dispatchat:140carriesExternalSync.of(true); the other dispatch at:152is selection/scroll-only (nodocChanged, listener doesn't run). No spurious suppression of unrelated dispatches.verifyTimelinesPopulatedregex/__timelines\s*\[\s*["']([^"'`]+)["'`]\s*]/gmatches string-literal keys including whitespace variants. **Cannot match dynamic-key registrations** (window.__timelines[k] = tl). When zero keys parse →applySoftReloadshort-circuits toreturn false` early.injectMotionPathPluginIfNeeded:/motionPath\s*[:{]/regex matches sub-composition usage. Version-matching viamatch[0].match(/gsap@([\d.]+)/)?.[1]falls back toGSAP_CDN_VERSION(3.15.0) for non-@-versioned (self-hosted) URLs — no major-version compatibility check.ensureMotionPathPluginLoaded:finalizeclosure setswin.__hfMotionPathPluginLoading = falseon BOTHonloadandonerror. Test atgsapSoftReload.test.ts:510confirms retry works after a CDN load error.- "Byte-identical combined stack diff" body claim: spot-check
main...1ea09b1dshows ~+6212/-699 across 84 files vs body's ~+6378/-849. Variance is rebase-adjustment-shaped. Roughly matches; not anomalous.
Concur with @james-russo-rames-d-jusso's BLOCKERs:
-
Silent staleness when
applySoftReloadreturnsfalsefor a non-transient reason. The PR justifies dropping→ reloadPreview()by citing transientverifyTimelinesPopulatedflakes (now hardened by scoping to expectedtargetKeys). ButapplySoftReloadreturnsfalsefor several non-transient reasons too:!win.gsap,!win.__hfForceTimelineRebind,targetKeys.length === 0(dynamic-key registration),gsapScripts.length === 0(live DOM has no GSAP<script>). Each is now silently swallowed — source updated, panel cache invalidated, iframe runtime stale until a full reload triggers some other way.The body's defense ("live
gsap.setalready shows the value") is only correct for the value-only-drag path whereinstantPatchran successfully BEFORE the soft reload would have fired. The escalation drop applies to ALLsoftReload: truecommits. Concretely:useGsapPropertyDebounce.flushPendingPropertyEditshipssoftReload: truewith NOinstantPatchcommitKeyframedPosition,extendTweenAndAddKeyframe,commitFlatViaKeyframesadd-keyframe, etc. all shipsoftReload: truewith noinstantPatch
Maps to band-aid pattern #4 (defensive code that catches its own throw and treats failure as success) + #7 (silent failure fallback). Fix shape: differentiate transient (
verify-failed) from permanent (!gsap,!__hfForceTimelineRebind,targetKeys empty,gsapScripts empty); drop the escalation only on the verify-failed signal that motivated the change. Strong concur with Rames on BLOCK. -
Observability gap (
patch:false → soft:falsecarries from #1613). Combined with the above, zero visible signal AND zero telemetry on the worst case. The U4 invariant ("live state is already correct") needs production evidence, not just unit fixture. Counter forinstantPatch_failed+softReload_failedis cheap. NIT-leaning-BLOCK depending on how often this surfaces in practice. -
ensureMotionPathPluginLoadedrace window. Between bootstrap setting__hfMotionPathPluginLoading = trueandonload/onerror, a soft reload that needs the plugin sees!win.MotionPathPlugin && win.gsapAND ignores the bootstrap flag → queues a SECOND<script>tag. Both eventually load, second tears down + re-executes the first's work. Idempotent but visible re-flash. Fix: honor__hfMotionPathPluginLoadinginapplySoftReload's async-load branch (poll/wait instead of queue-another). NIT-leaning-BLOCK depending on the practical hit rate. -
SourceEditorExternalSyncmid-typing race. If user is mid-typing when commit lands:current !== contenttriggers the effect, dispatches withExternalSync.of(true), OVERWRITES user's keystrokes, AND noonChangefires (annotation suppresses it). User keystrokes lost silently. Edge case but real. Fix: docstring +view.hasFocus()guard before the replace.
Concur with @james-russo-rames-d-jusso on nits:
injectMotionPathPluginIfNeededfalls back toGSAP_CDN_VERSIONfor self-hosted gsap → mismatches comp's actual version. Soft skew, low practical risk (HF ships 3.x).- 87-line growth of
gsapSoftReload.tspast the file's prior surface area — worth a split before next addition pushes the 600-line cap. applyPreviewSyncdoc comment in #1613 says "fall back to existing soft/full reload"; update in this PR's diff too.
Verdict: BLOCK. The silent-staleness escalation drop is the hard one; the mid-typing race + plugin race are smaller-but-real follow-ons. Once the escalation differentiates transient vs permanent failures (Rames's framing is exactly right), the rest is comment-level.
Review by Via
A softReload edit (and the SDK single-script refresh) no longer escalates to a full reloadPreview() iframe remount when applySoftReload returns false — the live gsap.set already shows the value, and a remount is the worst flash + re-inlines subcomps (reverting their keyframes). verifyTimelinesPopulated now checks the expected target keys the re-run registers, so a correct scoped re-run doesn't spuriously report empty. Full reload stays only for the structural (no-softReload) and ambiguous-script paths.
…ync-flash ensureMotionPathPluginLoaded() runs once at the preview iframe-load seam (NLELayout onIframeLoad), eagerly loading + registering MotionPathPlugin without killing the timeline. So when a user adds a motion path to a composition that didn't originally use one, the soft reload runs synchronously instead of taking the kill-then-await-CDN async path (the flash). Idempotent + defensive; the existing async fallback stays for genuine cold-start/CDN-failure.
The SourceEditor's CodeMirror update listener fired onChange on ANY docChanged — including the programmatic dispatch that syncs external content (e.g. a manual-edit commit writing the source back into the open editor). That made the editor re-save the file and bump refreshKey, fully reloading the preview iframe on every drag/keyframe edit — defeating the in-place instant patch and causing the flash. Annotate the programmatic sync (ExternalSync) and skip onChange for it, so only real keystrokes save.
…es motionPath A studio-created motion path writes a gsap motionPath tween into the single-source timeline, but the preview HTML only loaded gsap core — so the first render threw "Invalid property motionPath ... Missing plugin?". Detect motionPath usage and inject MotionPathPlugin right after the composition's gsap script, version-matched to it.
…odes + observability, SourceEditor focus guard
BLOCKER: applySoftReload now returns SoftReloadResult ('applied' | 'verify-failed' |
'cannot-soft-reload') instead of a bare bool. applyPreviewSync + sdkRefresh escalate to a full
reloadPreview() on the PERMANENT 'cannot-soft-reload' (no gsap/rebind hook/scopable key/script,
or sync re-run threw) — fixing the silent-stale-preview U4 dropped — but still suppress the
TRANSIENT 'verify-failed' (live gsap.set is correct). Telemetry: gsap_soft_reload_outcome
(origin/result/escalated) + gsap_instant_patch_fallback, so the U4 invariant is enforced, not asserted.
- SourceEditor: skip the programmatic external-sync replace while the editor is focused, so an
in-flight commit doesn't clobber the user's uncommitted keystrokes (ExternalSync kept for unfocused).
- Verified ensureMotionPathPluginLoaded already guards __hfMotionPathPluginLoading (no double-append).
decff5d to
ff2be9d
Compare
1ea09b1 to
ca5d144
Compare

Summary
This is the final polish slice of the instant manual-editing work. It closes out the last of the visible flicker when committing a manual edit in the studio: a value/keyframe commit now updates the preview in place with no full iframe remount, the
SourceEditorno longer re-saves (and re-reloads) on a programmatic content sync, and a composition that uses GSAPmotionPathgetsMotionPathPluginreliably present — both in the served preview HTML and eagerly in the live iframe — so adding a motion path never takes the async-load flash path.(Previously #1605 was the whole instant-editing feature. After the split it carries only this closing polish slice; everything below describes solely what is in this diff.)
What's changed
useGsapScriptCommits.ts— no full remount for soft-reloadable editsapplyPreviewSync: dropped theif (!applySoftReload(...)) reloadPreview()escalation. AsoftReload: truecommit now callsapplySoftReloadand never escalates to a full iframe remount, even if the soft reload reports failure — the remount is the worst flash and re-inlines sub-compositions (reverting a subcomp's keyframes), while the livegsap.setalready shows the committed value, so a transiently-failing soft reload still leaves a correct screen.sdkRefresh: same invariant.extractGsapScriptTextreturningnull(ambiguous/structural — zero/multiple scripts) still does a fullreloadPreview(), but a single-script soft-reloadable edit only soft-reloads. Full reload stays reserved for the structural path.gsapSoftReload.ts—verifyTimelinesPopulatedhardening + shared plugin URL + bootstrapverifyTimelinesPopulated(win, targetKeys)checks the expected keys the script re-registers (parsed from__timelines["..."]in the script text) rather than "any non-empty key". A scoped soft reload re-runs exactly one composition, so the right success signal is "my target keys are back" — avoiding the transient false (global map momentarily empty right after the re-run) that used to escalate to the full remount. Falls back to "any key present" with no target keys.MOTION_PATH_PLUGIN_CDNconstant, shared by the soft-reload async fallback and the new bootstrap.ensureMotionPathPluginLoaded(iframe): pre-loads + registersMotionPathPluginonce per iframe load. Idempotent (no-ops if present or a load is in flight, guarded by__hfMotionPathPluginLoading); registers an already-present plugin without appending; no-ops withoutgsap.registerPlugin; clears the loading flag on bothonloadandonerror(CDN failure can be retried; the async fallback still covers it).NLELayout.tsx— eager plugin preload at iframe loadonIframeLoadcallsensureMotionPathPluginLoaded(iframeRef.current)afterbaseOnIframeLoad(), sowin.MotionPathPluginis set before any studio edit. The first soft reload after a user adds a motion path to a composition that never used one runs synchronously instead of the async<script src>load path (which kills/clears the timeline mid-load — a visible flash).SourceEditor.tsx—ExternalSyncannotation breaks the re-save loopExternalSyncCodeMirrorAnnotation. The content-sync effect that pushes new source into the editor tags its dispatch withExternalSync.of(true), and theupdateListenerignores transactions carrying it. Programmatic content syncs (a manual-edit commit writing the source back into the editor) are no longer mistaken for user keystrokes, so they don't fireonChange→ re-save → preview reload.preview.ts— version-matched MotionPathPlugin in served preview HTMLinjectMotionPathPluginIfNeeded, wired intoinjectStudioPreviewAugmentations. When the served HTML usesmotionPath(/motionPath\s*[:{]/, anywhere in the bundle — the plugin registers globally, so sub-composition usage counts) and doesn't already ship the plugin, it injects theMotionPathPluginscript directly after the composition's owngsap(.min).jstag (the plugin must register onto an already-loaded gsap, often at body-end not<head>), version-matched to that gsap (gsap@<version>) to avoid GSAP's minor-version warning. Falls back to<head>only when no gsap tag is found.Why
Earlier slices removed most of the flash, but two paths still flickered on a manual edit. First, a soft-reloadable commit could still escalate to a full iframe remount whenever
applySoftReloadreported failure — and the common "failure" was a transient empty-__timelinesread right after a correct re-run, so an edit that worked still flashed and re-inlined sub-compositions (reverting their keyframes). Hardening the verify + removing the escalation kills that. Second — the real remaining flash — theSourceEditorre-save loop: a commit writes new source into the editor, the update listener treated that as a user edit, re-saved, and triggered a reload; theExternalSyncannotation severs that loop. The MotionPath changes ensure adding a motion path never falls into the async plugin-load path that clears the timeline mid-load.Testing
gsapSoftReload.test.ts—applySoftReloadreturnstruewhen the re-run re-registers the script's expected key (hardened verify); editing composition A leaves B's timeline intact (scoped kill regression); runs synchronously (no async load, no CDN<script>) whenMotionPathPluginis already present (timeline repopulated inline,__hfForceTimelineRebind+__player.seekcalled); falls back to async load when the plugin is genuinely absent (onerrorstill runs the script). NewensureMotionPathPluginLoadedsuite: null-iframe / missing-gsap no-ops; appends + registers once (sets/clears the loading flag); idempotent while loading; registers an already-present plugin without appending; clears the flag + allows retry on error.useGsapScriptCommits.test.tsx— asoftReload: truecommit whoseapplySoftReloadreturnsfalseno longer escalates toreloadPreview()(with/without instantPatch, with/without a prior failing patch); the structural case still full-reloads; the pre-change characterization test is retained with its assertion flipped so the change reads as intentional.preview.test.ts— injectsMotionPathPluginwhen the composition usesmotionPath, version-matched to its own gsap and ordered after the core gsap script; does not inject when there's nomotionPath.Stack
Part of the GSAP keyframe/motion-path stack:
#1553 → #1554 → #1555 → #1607 → #1608 → #1609 → #1610 → #1611 → #1567 → #1612 → #1613 → #1605. Top of the stack, on #1613. (Was the whole instant-editing PR; now scoped to the final polish slice after the split.) Builds independently; combined diff byte-identical to the originally-reviewed work.