perf(studio): lightweight preview reload, skip asset overlay#895
Conversation
Elements from the preview iframe are from a different window context, so `el instanceof HTMLElement` always returns false. Use `"outerHTML" in el` instead to correctly detect elements across frame boundaries.
reloadPreview() used location.reload() which bypassed the NLELayout saveSeekPosition effect, causing the playhead to reset to 0:00 after paste. Switch to setRefreshKey which triggers the effect and restores the seek position after the iframe reloads.
DOM element paste was inserting at the composition root, losing the parent context that provides CSS styles and positioning. Now stores the origin selector on copy and inserts the paste as a sibling immediately after the original element, preserving style inheritance. Falls back to root insertion if the selector can't be matched.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
miguel-heygen
changed the title
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
LGTM — net code reduction (-26 LoC) that removes a tricky crossfade in favor of a simpler iframe-src reload. Right architectural direction.
Audited
-
reloadPreviewsimplified from try/catchiframe.contentWindow?.location.reload()to justsetRefreshKey((k) => k + 1). ✓ -
NLELayout.tsxtranslates the bumped refreshKey into a call torefreshPlayer()fromuseTimelinePlayer. The new comment is precise: "refreshPlayer() saves the seek position and appends a cache-busting _t param, avoiding the full web-component teardown + crossfade that the key-based path uses." ✓ -
NLEPreview.tsxremoves the entire retiring-Player crossfade infrastructure:retiringKeystate +retiringTimerRefdeleted- Two-Player render (one retiring + one new) collapsed to one Player
handleNewPlayerLoadwith 160ms timer deletedrefreshKeyremoved fromgetPreviewPlayerKey— Player identity now depends only onprojectId+directUrl
This is structurally simpler AND avoids the ref-clobber race that motivated hf#881's identity-guard fix (the new path doesn't swap iframes, so the ref stays stable for the full session). The hf#881 guard remains a defensive belt for any future code that does swap iframes, but the common-case crossfade lifecycle that triggered the bug is gone. ✓
-
Asset-loading overlay skip on subsequent loads (
Player.tsx):const isContentRefresh = loadCountRef.current > 1; let lastUnloaded = isContentRefresh ? false : hasUnloadedAssets(iframe, false);
First load: probes for unloaded assets and shows overlay (cold cache). Subsequent loads: skips the probe entirely, assumes warm cache. Inline comment captures the intent: "The browser has already cached the assets from the first load, so they resolve near-instantly and the overlay just creates a disruptive flash." ✓
Non-blocking note
Cache-eviction edge case on isContentRefresh: the skip-overlay path assumes the browser cache is warm on subsequent loads. If the cache gets evicted between loads (cache pressure, user clears cache, devtools "disable cache" mode) the user gets a silent slow re-fetch with no feedback. Probably negligible in practice — studio sessions are short and cache pressure is rare during active editing — but worth a one-line note in the comment that the skip is an optimization assuming cache, not a strict invariant. Could fall back to the probe-with-shorter-timeout if you wanted a belt-and-suspenders cache-miss path, but YAGNI seems right here.
CI
mergeable_state: unstable — likely required checks pending on this base-branch (hf#894). Standard stack-of-PRs CI propagation; trust the bottom PR's check matrix.
— Review by Rames Jusso (pr-review)
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Lightweight refresh path is the right call — iframe.src cache-bust + pendingSeekRef preservation beats the dual-Player crossfade for content-only reloads.
Calibrated strengths
useTimelinePlayer.ts:435-445—refreshPlayeris small and correct:saveSeekPosition()first (locks in the time before the load handler resetscurrentTime), then mutatesiframe.srcsoloadCountRef.current++inPlayer.tsx:213fires naturally on the next load. No new state machine.Player.tsx:243— gating the overlay skip onloadCountRef.current > 1rather than a new prop is the right seam; the load counter is already the source of truth for "this is a refresh, not a cold mount."- Diff is small, deletes load-bearing complexity (the retiring-key crossfade), and the strengths-cited paths line up with the PR description's claims.
Findings
important — packages/studio/src/player/components/Player.tsx:243-244: skipping hasUnloadedAssets entirely on isContentRefresh is too aggressive. The premise ("browser has already cached the assets from the first load") doesn't hold when the edit introduces a new asset — e.g. a paste/drop that adds a <video src=\"...\"> not present in the prior load, or a Lottie added via __hfLottie. Those go through the refresh path with the asset overlay suppressed; the runtime's queued play() is resilient, but the user gets no "waiting for media" feedback while the new asset buffers — and on a slow network that can be multiple seconds of black frame with no signal. Suggested fix: keep lastUnloaded = hasUnloadedAssets(iframe, false) unconditionally, and instead add a reveal-delay to the asset overlay (the same 400 ms grace COMPOSITION_LOADING_OVERLAY_DELAY_MS pattern already used for the composition overlay at Player.tsx:39, 132-142). That eliminates the flash on cached-asset refreshes without dropping feedback on uncached-asset refreshes — the failure mode the prompt-text comment promises is rare ("long cold video or a broken asset can legitimately exceed 10s") but the new shortcut silences it on every refresh, not just fast ones.
nit — packages/studio/src/components/nle/NLEPreview.tsx:11, 20-22: refreshKey is now dead — declared on NLEPreviewProps and on getPreviewPlayerKey's param type, but never read in either body. NLELayout.tsx:397 still passes it. Either drop it from both signatures (and update the test name in NLEPreview.test.ts:5 to something like "player identity is independent of refresh-key signalling," since the test no longer exercises a real prop), or leave a one-line comment explaining it's retained for the parent's prop-spreading convenience. Right now the surface is misleading.
nit — packages/studio/src/components/nle/NLEPreview.tsx:26-36: the file-level JSDoc still describes the dual-Player crossfade behavior ("a new Player is mounted alongside the old one..."). Update to reflect the simpler key-swap reality so future readers don't go looking for the retiring path.
nit — NLEPreview.test.ts:5-17: the "keeps the same player identity when only refreshKey changes" test still passes a refreshKey arg even though getPreviewPlayerKey no longer uses it. The assertion is still valid as a regression pin (it locks in that refreshKey is not a key-input), but a one-line comment in the test ("pinned: refreshKey must NOT participate in key identity; content refreshes go through refreshPlayer()") would tell the next reader why we're passing a value we don't use.
Verdict: APPROVE
Reasoning: Approach is sound, code is clean, CI is green. The asset-overlay shortcut is the one real risk and worth a follow-up, but it's a UX-feedback regression on a narrow edge case (refresh that introduces a new uncached asset) — not a correctness bug. Ship and file the follow-up.
Review by Vai
- deduplicateIds regex used \b which matched data-composition-id, data-clip-id, etc. Switch to lookbehind (?<=\s) so only standalone id="..." attributes are rewritten. Add test pinning this. - Ctrl+C no longer calls preventDefault() before confirming there's a selected element. Native browser copy (text selections outside inputs) is preserved when nothing is selected in the Studio. - Add !event.altKey guard on C/V/X to avoid intercepting Cmd+Alt+V (paste-as-plain-text) and similar OS gestures. - Remove no-op .replace(/"/g, '"') flagged by CodeQL.
miguel-heygen
force-pushed
the
perf/studio-preview-reload
branch
from
12c859c to
28b28fa
Compare
…revert drive-by - Cmd+X now pre-checks selection state before preventDefault, mirroring the Cmd+C fix. Native cut preserved when nothing is selected. - handleCut returns Promise<boolean> so the caller can gate on it. - data-start rewrite scoped to the outermost opening tag only, so nested clip timing is preserved on paste. - Removed system clipboard write (cross-tab paste unsupported, in-memory ref is the only read path). - Reverted the reloadPreview drive-by (setRefreshKey→location.reload); the perf branch (#895) handles this properly via refreshPlayer().
…rdown Content refreshes (paste, move, resize, delete, asset drop) previously triggered setRefreshKey which changed the Player's React key, causing full web-component destruction + iframe teardown + crossfade animation + re-initialization of all event listeners and asset polling. Now NLELayout intercepts refreshKey changes and calls refreshPlayer() which just appends a cache-busting _t param to the iframe src. The Player web component stays alive, event listeners persist, and the reload is ~10x faster with no "waiting for media" flash. Key-based teardown is preserved for actual structural changes (project switch, composition drill-down via directUrl change).
The asset-loading overlay ("Preparing preview assets") polled for
video/audio readyState on every iframe load, including content
refreshes from paste/move/resize. On reloads the browser serves
assets from cache so they resolve near-instantly — the overlay
just created a disruptive flash. Now skips the polling on
subsequent loads (loadCountRef > 1), only showing it on the
initial cold load.
miguel-heygen
force-pushed
the
perf/studio-preview-reload
branch
from
28b28fa to
410e2c4
Compare
…ements (#894) * feat(studio): add clipboard payload types and ID deduplication * feat(studio): add Ctrl+C/V/X copy/paste for timeline clips and DOM elements * fix(studio): use duck-typing for cross-frame element access in clipboard Elements from the preview iframe are from a different window context, so `el instanceof HTMLElement` always returns false. Use `"outerHTML" in el` instead to correctly detect elements across frame boundaries. * fix(studio): preserve playhead position after paste reloadPreview() used location.reload() which bypassed the NLELayout saveSeekPosition effect, causing the playhead to reset to 0:00 after paste. Switch to setRefreshKey which triggers the effect and restores the seek position after the iframe reloads. * fix(studio): paste DOM elements as siblings, not at composition root DOM element paste was inserting at the composition root, losing the parent context that provides CSS styles and positioning. Now stores the origin selector on copy and inserts the paste as a sibling immediately after the original element, preserving style inheritance. Falls back to root insertion if the selector can't be matched. * fix(studio): address review — deduplicateIds, native copy, altKey guard - deduplicateIds regex used \b which matched data-composition-id, data-clip-id, etc. Switch to lookbehind (?<=\s) so only standalone id="..." attributes are rewritten. Add test pinning this. - Ctrl+C no longer calls preventDefault() before confirming there's a selected element. Native browser copy (text selections outside inputs) is preserved when nothing is selected in the Studio. - Add !event.altKey guard on C/V/X to avoid intercepting Cmd+Alt+V (paste-as-plain-text) and similar OS gestures. - Remove no-op .replace(/"/g, '"') flagged by CodeQL. * fix(studio): address review round 2 — Cmd+X guard, data-start scope, revert drive-by - Cmd+X now pre-checks selection state before preventDefault, mirroring the Cmd+C fix. Native cut preserved when nothing is selected. - handleCut returns Promise<boolean> so the caller can gate on it. - data-start rewrite scoped to the outermost opening tag only, so nested clip timing is preserved on paste. - Removed system clipboard write (cross-tab paste unsupported, in-memory ref is the only read path). - Reverted the reloadPreview drive-by (setRefreshKey→location.reload); the perf branch (#895) handles this properly via refreshPlayer().
miguel-heygen
dismissed stale reviews from vanceingalls and jrusso1020
The base branch was changed.
* feat(studio): add clipboard payload types and ID deduplication * feat(studio): add Ctrl+C/V/X copy/paste for timeline clips and DOM elements * fix(studio): use duck-typing for cross-frame element access in clipboard Elements from the preview iframe are from a different window context, so `el instanceof HTMLElement` always returns false. Use `"outerHTML" in el` instead to correctly detect elements across frame boundaries. * fix(studio): preserve playhead position after paste reloadPreview() used location.reload() which bypassed the NLELayout saveSeekPosition effect, causing the playhead to reset to 0:00 after paste. Switch to setRefreshKey which triggers the effect and restores the seek position after the iframe reloads. * fix(studio): paste DOM elements as siblings, not at composition root DOM element paste was inserting at the composition root, losing the parent context that provides CSS styles and positioning. Now stores the origin selector on copy and inserts the paste as a sibling immediately after the original element, preserving style inheritance. Falls back to root insertion if the selector can't be matched. * fix(studio): address review — deduplicateIds, native copy, altKey guard - deduplicateIds regex used \b which matched data-composition-id, data-clip-id, etc. Switch to lookbehind (?<=\s) so only standalone id="..." attributes are rewritten. Add test pinning this. - Ctrl+C no longer calls preventDefault() before confirming there's a selected element. Native browser copy (text selections outside inputs) is preserved when nothing is selected in the Studio. - Add !event.altKey guard on C/V/X to avoid intercepting Cmd+Alt+V (paste-as-plain-text) and similar OS gestures. - Remove no-op .replace(/"/g, '"') flagged by CodeQL. * fix(studio): address review round 2 — Cmd+X guard, data-start scope, revert drive-by - Cmd+X now pre-checks selection state before preventDefault, mirroring the Cmd+C fix. Native cut preserved when nothing is selected. - handleCut returns Promise<boolean> so the caller can gate on it. - data-start rewrite scoped to the outermost opening tag only, so nested clip timing is preserved on paste. - Removed system clipboard write (cross-tab paste unsupported, in-memory ref is the only read path). - Reverted the reloadPreview drive-by (setRefreshKey→location.reload); the perf branch (#895) handles this properly via refreshPlayer(). * perf(studio): use lightweight iframe.src reload instead of Player teardown Content refreshes (paste, move, resize, delete, asset drop) previously triggered setRefreshKey which changed the Player's React key, causing full web-component destruction + iframe teardown + crossfade animation + re-initialization of all event listeners and asset polling. Now NLELayout intercepts refreshKey changes and calls refreshPlayer() which just appends a cache-busting _t param to the iframe src. The Player web component stays alive, event listeners persist, and the reload is ~10x faster with no "waiting for media" flash. Key-based teardown is preserved for actual structural changes (project switch, composition drill-down via directUrl change). * perf(studio): skip asset-loading overlay on content refreshes The asset-loading overlay ("Preparing preview assets") polled for video/audio readyState on every iframe load, including content refreshes from paste/move/resize. On reloads the browser serves assets from cache so they resolve near-instantly — the overlay just created a disruptive flash. Now skips the polling on subsequent loads (loadCountRef > 1), only showing it on the initial cold load. * feat(studio): add Timing section to inspector Design panel Adds Start, End, and Duration fields to the Design panel when the selected element has data-start/data-duration attributes. Editing any field commits via the attribute patch pipeline (same as timeline edits) and refreshes the preview. End is computed from start+duration and writing End adjusts duration accordingly. * fix(studio): preserve bare text nodes in mixed-content elements collectDomEditTextFields only captured child HTML elements, ignoring bare text nodes. For elements like: <div class="headline">If you're <span>turning 65</span> soon...</div> only the <span> was collected as a text field. When commitDomTextFields serialized back, "If you're " and " soon..." were lost. Now walks childNodes and creates text-node fields for bare text nodes alongside child element fields. serializeDomEditTextFields emits bare text for text-node fields, preserving the complete mixed content. * fix(studio): address #896 review — remove scrub from timing, add mixed-content test - Remove scrub from Timing fields: 1px = 1 second is too coarse. Scroll-wheel and direct typing still work with sub-second precision. - Add mixed-content text-node serialization test in a separate file (domEditingTextFields.test.ts) to avoid bloating the existing domEditing.test.ts past the filesize limit.
3 participants
Summary
refreshPlayer()(iframe.src cache-bust) instead of full Player web-component teardown + crossfadeTest plan