fix(studio): playback no longer re-renders the whole app every frame - #4529

Review at 7430d5cc.

Verdict: APPROVE. The fix is correct and scoped. Nothing that used to update per frame goes stale, and both pause fixes hold. What I found is a missing test, one overlong line, a short capture-time window while a composition switch loads, and pre-existing sibling pause paths the PR does not touch.

Findings

Blocker

None.

Should-fix

1. The Capture click fix has no test. useFrameCapture.ts:51 now reads playheadTime() at click, which is the fix for "a capture taken during playback used the frame from when Play was pressed". I reverted that one line to usePlayerStore.getState().currentTime and all 13 tests in useFrameCapture.test.tsx + StudioHeader.dom.test.tsx + StudioHeader.test.ts still pass. The three new tests only go through refreshCaptureFrameTime. One test that calls handleCaptureFrameClick while playing and asserts the fetched URL carries the live t would pin it.

2. Pre-existing sibling pause paths still carry the bugs this PR fixes for the hidden tab. Not regressions (main behaves identically, probed at both refs), but the ea23da9d headline and the first hidden-pause test name say "like any other pause", and these three are pauses that do not:

   • End of film (useTimelinePlayerLoop.ts:68-72) sets isPlaying false but leaves shuttleDirectionRef at "forward". Probe: L, L (2x), play to the end, press L. Rate is 4, not 1. The same probe with K instead of end-of-film gives 1.
   • Canvas click (usePreviewInteraction.ts:76-83) pauses the iframe player and sets isPlaying false but never stops the reverse loop. Probe: J at t=20, then do exactly what pausePreviewPlayback does. 200 ms later the adapter has moved 19.9 to 19.7 and isPlaying is back to true, because the reverse tick calls setIsPlaying(true) (useTimelinePlayer.ts:248). This is the "film keeps rewinding" bug from this PR, on a different trigger. It bites when the click selects something; a click on nothing resumes through requestPlayback(true), which does stop the reverse loop but resumes forward.
   • Preview reload during a reverse shuttle (saveSeekPosition, useTimelinePlayer.ts:438-444) stops playback but leaves shuttleDirectionRef at "backward". The next J plays at 2x. It also skips the store sync, so if that reload fails (useShadowPreviewReload.ts:93 clears the pending seek) the store keeps the pre-shuttle time and Capture, Split and Delete use it. Main reads the store there too, so this is the same on both.
     Routing them through pause() (or requestPlayback(false) from outside the player hook), the way the visibility handler now does, would close all three. Fine as a follow-up.

Nits

1. Format is red on one line. useTimelinePlayer.hiddenPause.test.ts:61 is 101 characters. oxfmt --check flags it locally, CI Format and Preflight (lint + format) fail on it, and preview-regression fails only because its parity job was skipped behind Preflight (PREVIEW_PARITY_RESULT: skipped). The body says oxfmt is clean on the changed files; that was true before 7430d5cc. bun run format fixes it.

2. The hidden-pause tests unmount inside the test body. root.unmount() is the last line of each test, not in afterEach, so a failing assertion leaks that test's window keydown and visibilitychange listeners into the next one. I hit this while mutation testing: a variant that only drops the store sync reports all 3 tests failing in one run, but in isolation (-t) the shuttle-speed test passes. The tests still discriminate; they just over-report. Moving the unmount into afterEach fixes it.

3. One narrow window where main's capture time was right and this one is not. Playing inside a sub-composition, click Master in the breadcrumb. handleNavigateComposition writes the saved master time to the store without pausing (useCompositionStack.ts:55-61). Until the new document's adapter commits and sets isPlaying false (useTimelineSyncCallbacks.ts:245-259), the loop has no adapter and stops notifying (useTimelinePlayerLoop.ts:47-52). A Capture click in that window gets the sub-composition's last live time with the Master path (useFrameCapture.ts:32-35). Main read the store and got the master time, when masterSeekRef was above 0. It clears once the load commits, and useLivePlayheadTime shows the same stale value in that window on both refs. I traced this in code and did not reproduce it in a browser. Found by the adversarial pass.

4. One sentence in the body does not match the code. "During a reverse shuttle (J) ... the next L resumed at double the last shuttle speed." After J the direction is "backward", so the next L resets the index to 0 and plays at 1x (usePlaybackKeyboard.ts:63-70). The doubling happens after a forward shuttle (L, L, hide, L gives 4x), which is what test 3 pins. The code is right.

5. The keyboard copy-link limit also applies while paused. The body describes it as "playback runs on". On main a paused seek also notified liveTime, so the href followed arrow-key seeks while the link had focus. Now it holds the time from focus until the next focus or pointer down. Same limit, slightly wider than described. Click, Enter, middle-click and right-click are all fine.

Checked

• Mechanism, measured. A jsdom probe mounting useFrameCapture in a parent with one child, then 30 separate act(() => liveTime.notify(t)) calls while playing: main re-renders parent and child 30 and 30 times, head 0 and 0. App.tsx:344 calls this hook directly, so on main every playback frame re-rendered App.
• Every reader of the removed state. captureFrameTime is read only by captureFrameHref and captureFrameFilename (useFrameCapture.ts:106-113), consumed only at StudioHeader.tsx:191-192, and refreshed at :201-202 (focus, pointer down) and at click (useFrameCapture.ts:51-52).
• Nothing else rode the per-frame App render. The store's currentTime does not advance during forward playback (useTimelinePlayerLoop.ts:52 notifies liveTime only), so render-time store reads were already frozen on main. Each per-frame UI surface has its own subscription:
  • time display: PlayerControls.tsx:211-224, direct DOM via liveTime
  • timeline playhead and follow-scroll: useTimelinePlayhead.ts:124-140
  • active-clip highlighting: useTimelineActiveClips.ts:125-129
  • layers panel: LayersPanel.tsx:183-190
  • inspector and lane labels: useLivePlayheadTime in PropertyPanel.tsx:140, propertyPanelAudioFxGroup.tsx:149, TimelineGroupLaneLabels.tsx:44
  • captions: their own 66 ms interval while playing (CaptionOverlay.tsx:133-145)
  • selection bounds and motion path: hidden while playing (useStudioContextValue.ts:92-98)
  • I, O and arrow-step read adapter.getTime() at keypress (usePlaybackKeyboard.ts:55,165,171), and in/out and loop are read per tick from the store (useTimelinePlayerLoop.ts:53-59)
    An AST scan of packages/studio/src for useEffect/useLayoutEffect with no dependency array finds two (useFxChainObserved.ts:53, useTimelineSelectionLifecycle.ts:20). Both key off props and state that change on their own, not per frame. No component reads getTime(), __player or __timelines during render.
• Hidden-tab pause. pause() (useTimelinePlayer.ts:266-276) does everything the old partial copy did, plus stopReverseLoop, the store sync, and the shuttle reset. pause is a useCallback over stable deps, so pauseRef (:459-460) cannot hold a stale copy. The listener is removed in the mount-effect cleanup (:489), and the liveTime subscription in useFrameCapture.ts:26-30 returns its unsubscribe through useMountEffect.
• Pause-path audit. Space and the play button (togglePlay), K, playbackRequest (useTimelinePlayer.ts:345), and the hidden tab all go through pause(). Seek while playing resets the shuttle in both branches (:306-313). Reverse reaching the start clears direction (:241), which is enough because shuttle() resets the index on a direction change (usePlaybackKeyboard.ts:63-70). Loop wrap keeps playing by design. There is no blur-driven pause. The three that do not match are in Should-fix 2.
• Fails-first. Main's three source files with the PR's tests: 5 of 6 fail (the paused-capture test passes on main, as the body says). Each hidden-pause test also fails on main when run alone.
• Mutations (one hunk at a time, the relevant files re-run each time):
  • killed: subscriber sets state again; live branch replaced by store; store branch replaced by live; visibility handler reverted to main's partial copy; partial copy plus the reverse and shuttle reset but no store sync (kills the time tests); partial copy plus the store sync only (kills the reverse and speed tests); pause() without the direction reset
  • survived: click reads the store (Should-fix 1); useState initialiser back to 0 and livePlayheadRef initialised to 0 (both overwritten before any user-visible read); dropping the isPlaying guard on the hidden handler (harmless); pause() without the speed-index reset (redundant once direction is null)
• Adversarial pass. A separate agent tried to break it on stale UI, pause paths, capture edge cases and leaks. It found no regression beyond Nit 3, and independently hit the same three sibling pause paths. I re-checked each of its claims in code before including it.
• Suites. Studio tsc --noEmit clean. The body's set (src/player/hooks + useFrameCapture + StudioHeader.dom): 14 files, 136/136, matching the body. Full studio suite: 523 files passed, 1 skipped, 5610 tests. oxlint clean on the five files.

Not verified

• The browser numbers (16 to 43 fps, 42 to 17 ms median, 82 App renders per second). I measured the render count in jsdom, not frame time in Chrome, and did not watch the recordings.
• That the Capture link's server render lands on the right frame end to end. I checked the time the hook hands to buildFrameCaptureUrl, not the server.

— Rames

Re-review at d6e18416. My last review approved (5325165451) at 7430d5cc.

Verdict: APPROVE. The only change is the format fix my review asked for.

• One commit, d6e18416 ("style(studio): format the hidden-tab pause test"). Its only parent is 7430d5cc, so there is no merge.
• It touches one file, useTimelinePlayer.hiddenPause.test.ts, +3/-1. The 101-character act(...) line at :61 is now wrapped over three lines. The tokens are identical, so the test is unchanged.
• No source file changed, so everything in my review at 7430d5cc holds here, including the should-fixes (the Capture-click fix has no test, and three older pause paths still have the bugs this PR fixes for a hidden tab).

— Rames