fix(studio): preserve playhead position on composition refresh#1000
Conversation
Stop NLEPreview from including refreshKey in the Player's React key. Previously, a refreshKey change caused a full Player teardown + remount, which destroyed the playback adapter before refreshPlayer() could save the seek position — so the playhead always reset to 0:00. Now refreshKey changes only trigger refreshPlayer()'s lightweight iframe.src reload path, which correctly captures the current time via saveSeekPosition() before reloading the iframe content. Closes #996
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Verdict: COMMENT (would-be APPROVE)
🎉 1000th PR for hyperframes — solid choice of fix to land on this number. Clean one-line surgical fix at exactly the right scope.
Root-cause trace confirmed
The fix matches the failure mode described in #996:
- Pre-fix:
refreshKeychange →activeKeychanges (${baseKey}:${refreshKey}) → React sees a new key on<Player>→ mounts a NEW Player instance at the new key while the OLD one moves toretiringKeyand is unmounted 160ms later. The OLD Player holds the user's playhead state; the NEW Player initializes from 0. The crossfade was visually masking the playhead reset, not preventing it. - Post-fix:
activeKey = baseKey(no refreshKey suffix) → same Player instance is retained across refreshes → NLELayout'suseEffecton[refreshKey](NLELayout.tsx:122-126) callsrefreshPlayer(), which is the iframe.src-reload path thatsaveSeekPosition()s (useTimelinePlayer.ts:464-470) intopendingSeekRefand restores oninitializeAdapter(). ✓
Sub-composition drilling and project switch still remount correctly (those go through directUrl / projectId, both of which still feed into baseKey).
Non-blocking observations — dead code left behind
The PR is correctly scoped to the one-line behavior fix, but it leaves several symbols unreachable that the original crossfade scaffolding required. Worth a follow-up cleanup PR (or scoped down into this one if you'd like — I'm fine either way):
-
retiringKeystate,retiringTimerRef, andhandleNewPlayerLoadare unreachable.setRetiringKey(oldKey)was the only path to a non-null value, and that block was removed.- The remaining
setRetiringKey(null)(NLEPreview.tsx:228) lives insidehandleNewPlayerLoad, which is only called whenretiringKeyis truthy — circular dead state. - The first
<Player key={retiringKey}>rendering block (NLEPreview.tsx:415-424) and thestyle={retiringKey ? {...} : undefined}(line 440) become permanently inert. - ~30 lines. Knip/lint probably won't flag it (the hooks are technically "used"), but it's real cruft.
-
getPreviewPlayerKeystill acceptsrefreshKeyin its signature (NLEPreview.tsx:27-36) but ignores it. The associated test (NLEPreview.test.ts:115-128— "keeps the same player identity when only refreshKey changes") documents the deliberate ignore. Post-fix, the signature could simplify to({ projectId, directUrl }) => directUrl ?? projectIdand the test reduces to redundancy (getPreviewPlayerKey({...}) === activeKey). Optional. -
Stale comment in NLELayout.tsx:117-120: "avoiding the full web-component teardown + crossfade that the key-based path uses." After this PR, the key-based path (directUrl / projectId changes) doesn't crossfade either — the crossfade trigger was only the now-removed refreshKey block. Worth a one-line update in the cleanup pass.
Testing
Test plan in the PR body is comprehensive (file touch, content edit, build, sub-comp drill, undo/redo — 5 scenarios). No new automated test added, which is acceptable since the public surface is the React component lifecycle (hard to unit-test the seek-preservation chain without a real iframe). The existing getPreviewPlayerKey test still passes — and now exercises the actual activeKey (it didn't before, since activeKey included refreshKey while getPreviewPlayerKey didn't; the test was passing only by coincidence).
CI
All required green. 2 Windows checks still in_progress (pattern matches recent PRs).
— Rames Jusso
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approved per James's go-ahead. 🎉 The 1000th hyperframes PR — fitting that it's a clean one-line surgical fix.
Findings from the prior COMMENT review stand as non-blocking follow-ups (dead crossfade scaffolding cleanup, getPreviewPlayerKey signature simplification, stale comment in NLELayout.tsx:117-120). None of them gate this merge.
Follow-up to the playhead-preservation fix: clean up the now-unreachable crossfade infrastructure that was only triggered by refreshKey changes. - Remove retiringKey state, retiringTimerRef, handleNewPlayerLoad - Remove the retiring Player render block and conditional onLoad/style - Drop refreshKey from getPreviewPlayerKey signature and NLEPreviewProps - Stop passing refreshKey from NLELayout to NLEPreview - Update NLELayout comment to reflect current iframe.src reload model - Simplify getPreviewPlayerKey test
Fallow audit reportFound 24 findings. Duplication (16)
Health (8)
Generated by fallow. |
miguel-heygen
force-pushed
the
feat/studio-preserve-playhead
branch
3 times, most recently
from
916f267 to
d3cf649
Compare
When the Code tab is active and a user clicks an element in the preview, the code editor now auto-scrolls to the corresponding HTML source. This removes the need for Alt+click — the existing click-to-source mechanism fires automatically when the Code tab is already open. - Add getTab() to LeftSidebarHandle for reading the active tab - Add getSidebarTab getter to useDomEditSession - Add effect that calls openSourceForSelection on selection change when Code tab is active
miguel-heygen
force-pushed
the
feat/studio-preserve-playhead
branch
from
d3cf649 to
e2de547
Compare
2 participants
Summary
refreshKeywas included in the Player's React key, causing a full teardown + remount that destroyed the adapter beforerefreshPlayer()could save the seek positionrefreshKeyfrom the Player key so refreshes use the lightweightiframe.srcreload path, which correctly saves and restores the playhead viasaveSeekPosition()→pendingSeekRef→initializeAdapter()Implementation
Single-file change in
NLEPreview.tsx(-8 lines, +1 line):refreshKey-triggered crossfade block that setretiringKeyactiveKeyfrom`${baseKey}:${refreshKey}`to justbaseKeyThe existing
refreshPlayer()mechanism inNLELayoutalready handles seek preservation correctly — the bug was that the Player remount was racing ahead of it.Test plan
directUrl)Closes #996