Repository navigation
fix(studio): a clip's thumbnail strip stays put while you trim it - #6942
Conversation
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 2 blocker · 1 important · 0 nits · head 196da9b2
- blocker: the body has no
fails without the fix:line naming the test. Addfails without the fix: packages/studio/src/player/components/VideoThumbnail.test.tsx › VideoThumbnail during a trim › keeps its strip through every move of a trim and decodes the new range once at rest. I checked it: with both source files reverted, it fails (decodeVideoThumbnailcalled 6 times). - blocker: two required checks fail on this head. PR captures fails because the body has no
## Before/## Aftersections, each with an image or video. Comments fails because the comment share inVideoThumbnail.tsxrose from 0.5% to 0.9% (see inline).
Scope
I read the full diff, timelineMotion.ts (useValueAtRest, whenTimelineIdle) and every call to publishResizingClip. The held-range logic is correct: the strip holds during a trim and takes the committed range within TIMELINE_REST_MS of release. At head, VideoThumbnail.test.tsx passes 18/18. CodeQL and CI were still running at review time.
Edit accuracy: accurate 271 (base branch 271), smooth 114 of thoseThe gate passes. Quarantined, measured but not gated (0) |
There was a problem hiding this comment.
Verdict: APPROVE · 3 resolved, 0 open, 1 new · 196da9b2 → 6d08b15f
- important: the PR body attributes the
Fallow auditred entirely to existing complexity, and that is only partly true.useTimelineClipDragis 61 cognitive at both the base and this head.VideoThumbnailrose from 25 to 27 (threshold 15). This PR adds the seconduseValueAtRest(useMemo(...)). Moving that into a small hook (useRangeHeldDuringTrim) would bring it back to 25. Recently merged PRs all had Fallow green or skipped. If the job is a required check, the merge will stay blocked until it passes.
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 resolved, 0 open, 1 new · 6d08b15f → e214e8c6
The Fallow finding is fixed: VideoThumbnail is back to 25 and fallow audit --base origin/main is clean. The new docstring fails the Comments ratchet again (↓ inline). Tests pass 1841/1841 and tsc is clean.
Trimming a video clip on the timeline re-requested that clip's thumbnail strip on every pointer move, because the strip's request was keyed on the live trimmed range. A decode ran for frames thrown away a moment later.
Now the strip keeps the range it had while the trim is moving and decodes the new range once when the trim rests. Each trim move marks the timeline as moving, the same signal zoom already uses.
fails without the fix: packages/studio/src/player/components/VideoThumbnail.test.tsx › VideoThumbnail during a trim › keeps its strip through every move of a trim and decodes the new range once at rest
A second test,
useTimelineClipDrag.resize.test.tsx › marks the timeline as moving on every trim move, so thumbnails hold their range, fails if the call that connects a trim to the hold is removed.Measured on a Linux box with the real CLI preview server and a 25-file project (16 videos, 4 audio, 5 images, 9 tracks, 8 minutes). One video clip's end handle dragged 60 steps, fresh project each run, two runs per build:
The request count drops two to three times. Frame time and long tasks did not change in these runs, and the strip stays visible during the drag in both builds (see the captures), so this does not claim a smoother drag, only less decode work per trim.
Before
After
Not claimed: the report also describes the preview taking close to a minute to show a timeline edit and all cores busy with ffmpeg. Neither was reproduced here (the proxy path ran at most one ffmpeg at a time), so this does not claim to fix them.
The
Fallow auditred is the existing cognitive complexity ofuseTimelineClipDrag(61, over the threshold on main; this adds one call).VideoThumbnailstays at its main value of 25: the held request inputs live in one small hook,useRangeHeldDuringTrim, measured withfallow healthon main and on this head.Size
A lone fix: one hook value held at rest plus one call, and its tests.