refactor(studio): extract live timeline clock - #2711
Conversation
5b22e39 to
816894f
Compare
49e772f to
9f951c8
Compare
816894f to
1a4e7c0
Compare
9f951c8 to
b8035c7
Compare
1a4e7c0 to
7b2f56e
Compare
b8035c7 to
3f43f78
Compare
7b2f56e to
b74d0f7
Compare
3f43f78 to
142a245
Compare
b74d0f7 to
2b50771
Compare
142a245 to
3f2b313
Compare
2b50771 to
d0891eb
Compare
d0891eb to
1be5a1f
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Behavior-preserving extraction — verified.
Behavior parity. New liveTime.ts is byte-equivalent to the old module-scoped block (rename only: _timeListeners → timeListeners, t → time, cb → listener). Set semantics, notify iteration order, subscribe's unsubscribe closure all identical.
Singleton preserved. liveTime is re-exported from playerStore.ts via export { liveTime } from "./liveTime". Re-export shares module identity — one Set<TimeListener> for the whole app. No accidental fork.
Caller audit @ 1be5a1f (grep across repo, checked at HEAD):
- Runtime consumers (
useTimelinePlayerLoop,useTimelinePlayhead,useTimelineActiveClips,useTimelineRangeSelection,useTimelineSyncCallbacks,useTimelinePlayer,useFrameCapture,usePreviewInteraction,useGestureRecording) all import from../store/playerStore— reachliveTimethrough the re-export. ✅ - Public surface (
player/index.ts,studio/src/index.ts) re-exports from./store/playerStore— external API unchanged. ✅ playerStore.test.tsstill exercisesliveTimevia./playerStore— coverage reaches the extracted module through the same re-export.
Import cycle risk. liveTime.ts has zero imports — cycle-free.
Ambient state. None to lose; module is pure pub/sub with no context/hook coupling.
CI. All required checks SUCCESS on 1be5a1f0 (typecheck, runtime contract, studio timeline viewport gate, windows render, preview parity, producer unit+integration).
— Review by Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 1be5a1f0.
Clean behavior-preserving extraction — the liveTime publisher moves from playerStore.ts into a dedicated liveTime.ts module, with the store re-exporting the same public API so existing consumers don't retarget their imports. Verified equivalence: _timeListeners → timeListeners, cb → listener, t → time are pure renames; .notify and .subscribe semantics are byte-identical to the previous inline definition.
Nit (body-only): the migrated comment shortens from "Bypasses React state so the RAF loop can update the playhead/time display without triggering re-renders on every frame" to "Lightweight pub-sub avoids React re-renders on every playback frame." The essential why (RAF loop needs a side channel to avoid per-frame renders) is preserved but the concrete consumer ("playhead/time display") is lost. Consider a ponytail: marker per the HF idiom ([[project_hf_ponytail_comment_idiom]]) to lock in the why so a future contributor doesn't wonder why the RAF loop needs to bypass React state.
Everything else in the diff is mechanical rename. No behavioral surface. Stack sets up cleanly for #2712 / #2713.
Ready from my side, leaving as COMMENTED.
1be5a1f to
fc416c8
Compare
|
Review follow-up folded into exact head |
vanceingalls
left a comment
There was a problem hiding this comment.
Delta re-verify at fc416c8373e82a11b3865e53ea73480903796ced (prior R1 at 1be5a1f0).
Patch-equivalent rebase, with one addressed nit. PR still touches exactly packages/studio/src/player/store/liveTime.ts (+11) and packages/studio/src/player/store/playerStore.ts (+1/-13). playerStore.ts is byte-identical to the R1-approved head. liveTime.ts has a single semantic change from prior head → the file-lead comment is now:
// ponytail: Playback RAF updates the playhead/time display without per-frame React renders.
That's the direct fix for @james-russo-rames-d-jusso's COMMENTED nit at 1be5a1f0 asking for a ponytail: marker locking in the concrete why (playhead/time display + RAF loop). Now preserved verbatim in the specified idiom. Nit → FIXED.
Prior R1 findings state.
- Byte-equivalent extraction + rename discipline (
_timeListeners → timeListeners,t → time,cb → listener) — unchanged, still holds. - Singleton preserved via
export { liveTime } from "./liveTime"inplayerStore.ts— verified at HEAD. - Runtime consumers still reach
liveTimethrough theplayerStorere-export — no import retargets in this PR. - Zero imports in
liveTime.ts→ no cycle risk — unchanged. - Rames's body-only ponytail nit — FIXED (comment updated per HF idiom).
Family F invariants. Preserved — this base PR is a pub-sub extraction and doesn't touch element-identity keying, tie-breaks, deletion fallback chain, same-time keyframe ease, .tsx exports, or useEffect state-syncing.
New-base interactions. None. The 12-commit main advance between 1be5a1f0's base and current base is orthogonal to liveTime.ts / playerStore.ts — no touching commits in the player/store/ subtree.
CI at HEAD. Typecheck / Lint / Fallow / File size / Studio load smoke / Studio timeline viewport gate / Producer unit + integration / Runtime contract / Preview parity / player-perf / windows Render / SDK unit+contract+smoke / CodeQL — all SUCCESS. Tests on windows-latest still IN_PROGRESS at review time (non-blocking, matches prior R1 pattern).
Byte-clean re-stamp.
— Review by Via

What
Extract the live playback-time publisher into a small shared store module without changing playback behavior.
Why
High-frequency timeline consumers need one stable clock contract without coupling to the full player state container or causing React renders on every playback frame.
How
liveTime.tsnow owns the listener set plus thenotifyandsubscribeoperations. The player store re-exports that contract, so existing consumers keep the same public API while the implementation has one owner.This is the first PR in the stack and targets
main.Test plan
Exact-head CI completed without failed conclusions, including typecheck, runtime contract, Studio tests, and the timeline viewport gate.