feat(studio): seek-restore contract for player reloads#2193
Conversation
bbe1db5 to
90663ec
Compare
68911ca to
1f6c9fe
Compare
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed as part of the 25-PR NLE overhaul stack. Full stack review on #2205 (the keystone). No blockers on this PR. — Miga
vanceingalls
left a comment
There was a problem hiding this comment.
🟢 LGTM — seek-restore priority matches the PR body claim, revealIframe is a defensive read-side landing before the write-side (safe order).
Focus items — verified.
Seek priority contract (resolveReloadSeekTime, useTimelineSyncCallbacks.ts:70-79). The one-liner target = input.pendingSeek ?? input.requestedSeek ?? input.storeCurrentTime produces exactly the priority order the PR body claims: pending → deep-link → store playhead. The ?? operator treats 0 as a valid explicit position (not nullish), so a pendingSeek: 0 correctly wins over a non-null requestedSeek — the test at lines 93-102 pins this. Clamp to [0, duration] via the <= 0 → 0 guard + Math.min(target, duration) is the "one sanctioned move" the PR body calls out (test at line 82-91 pins pendingSeek: 18 + duration: 9 → returns 9). Negative/NaN inputs return 0 unconditionally — the alternative "fall through to storeCurrentTime on invalid pendingSeek" would let a bad ref value silently mask an otherwise-good store playhead, so the current strict-guard behavior is defensible even if slightly counter-intuitive.
revealIframe — safe read-side landing. The comment (line 59) says "Undo the visibility: hidden that refreshPlayer sets across a full reload" — I checked refreshPlayer at this SHA (packages/studio/src/player/hooks/useTimelinePlayer.ts:475-484) and it does NOT currently set iframe.style.visibility = "hidden". That write must land in a later stack PR. The consequence is fine and probably intentional: shipping the reveal side first means every existing reload path continues to work identically (the guard iframe.style.visibility === "hidden" no-ops when never set), and once the hide side lands the pair is already wired — the reverse ordering (hide-before-reveal) is what would introduce a "black player" window. Confirm this reasoning with the follow-up PR: search for the visibility = "hidden" write and make sure every path that sets it either awaits initializeAdapter's success (line 283 reveal) OR the 5s safety-net (line 394 reveal), with no early-return between hide and either resolution.
Guard-seek before real seek (initializeAdapter:276-278). The guardTime = startTime > 0.001 ? Math.max(0, startTime - 0.001) : 0.001 two-step forces a time boundary crossing so adapter.seek(startTime) re-evaluates rather than no-op-ing. Comment (267-275) explains why exhaustively — this is the exact "just-dropped clip stayed invisible until nudge" fix. Correct reasoning, and the 0.001 epsilon is small enough it won't be user-visible on a 30fps timeline (0.033s frame).
Store-request consumption ordering (initializeAdapter:264-265). pendingSeekRef.current = null; if (storeSeek != null) clearSeekRequest(); — both consumers cleared after the resolve pulled them, so a re-entrant initializeAdapter sees empty slots and correctly falls back to storeCurrentTime. Matches the "second overlapping reload" test at line 57-69.
Two reveal paths. initializeAdapter reveals on the SUCCESS path (line 283) after the second seek renders the frame. The 5s safety-net (line 387-395) reveals unconditionally after either a last-try-then-give-up or a genuine timeout. Between these two, the failure-path I looked for is "initializeAdapter throws mid-flight after guard-seeking but before revealIframe" — line 302-338 is wrapped in try {} with catch {} at 338, so post-reveal work is soft-caught, but the reveal itself at 283 executes before that block starts. No hide-never-reveal path within this file.
Cross-PR watch (per R1 focus). #2213's app-shell swap must preserve this contract. Two things to check on that PR: (1) neither of these two reveal call sites gets dropped; (2) if #2213 introduces the visibility = "hidden" write on refreshPlayer, its placement must guarantee onIframeLoad fires afterward — an eager return before iframe.src is re-set would leave the hide with no consumer to reveal.
R1 by Via
1f6c9fe to
c161c77
Compare
90663ec to
b5bf0cf
Compare
|
Confirming the hide/reveal pairing: at this PR's position |
c161c77 to
df6ca19
Compare
b5bf0cf to
9adc2d3
Compare
9adc2d3 to
66a029d
Compare
What: useTimelineSyncCallbacks gains resolveReloadSeekTime and revealIframe — the pure contract for where the playhead lands after a preview reload (pending seek > deep-link seek > store playhead, clamped) and for undoing refreshPlayer's iframe hide. Test suite included. Why: the NLE editor reloads the preview on every committed edit; this contract is the difference between "playhead restores" and "jumps to 0". How: additive exports on an existing hook file; main's consumers unchanged. Test plan: bunx vitest run useTimelineSyncCallbacks.test.ts; tsc --noEmit in packages/studio; fallow audit clean.
66a029d to
89066ef
Compare

What
useTimelineSyncCallbacks gains resolveReloadSeekTime and revealIframe — the pure contract for where the playhead lands after a preview reload (pending seek > deep-link seek > store playhead, clamped) and for undoing refreshPlayer's iframe hide. Test suite included.
Why
the NLE editor reloads the preview on every committed edit; this contract is the difference between "playhead restores" and "jumps to 0".
How
additive exports on an existing hook file; main's consumers unchanged.
Test plan
bunx vitest run useTimelineSyncCallbacks.test.ts; tsc --noEmit in packages/studio; fallow audit clean.
Stack position 2/25 — studio NLE overhaul (CapCut-parity timeline + canvas). Graphite manages bases; merge from #2192 upward. Temporary
TEMP(studio-dnd)fallow entries (unwired-component windows) are all removed by #2213 (app-shell swap), whose tree is byte-identical to the fully-verified integration branch.