refactor(clips): render clean video filmstrip - #2558
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Visual recap — skippedThe visual recap job did not run for this pull request. This is informational only and does not block the PR. Recap skipped for |
There was a problem hiding this comment.
Builder reviewed your changes and found 4 potential issues 🟡
Review Details
Code Review Summary
PR #2558 adds server-generated filmstrip sprites with a browser extraction fallback, wires sprite geometry into the timeline, and preserves the canvas overlays for playhead/selection/split controls. The overall approach is sound: media bytes stay in blob storage, server work is bounded through ffmpeg slots/timeouts, ownership is scoped, and the client has a graceful fallback. The existing additive-column safety net also covers pre-existing databases.
Risk assessment: Standard — this spans recording actions, media processing, persistence, and editor state, although it does not change authentication or billing.
Key Findings
- 🟡 MEDIUM —
generate-filmstrip --all --forcestill filters out every recording that already has a filmstrip, so documented batch regeneration cannot work. - 🟡 MEDIUM — browser-extracted frames remain in state while switching recordings, allowing the previous clip's thumbnails to render during a potentially long extraction.
- 🟡 MEDIUM — trim handles disappear during playback when no selection exists, preventing the primary pointer workflow from starting a trim while playing.
- 🟡 MEDIUM — forced regeneration uploads replacement sprites without reclaiming the previous asset, and recording deletion cleanup does not include
filmstripUrl.
Good patterns include cancellation guards around async frame extraction, race protection on the recording update, bounded frame/grid sizes, and explicit ffmpeg timeout handling.
🧪 Browser testing: Will run after this review (PR touches UI code)
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest commit fixes the previously reported filmstrip upload races: the update now guards the originally read filmstripUrl, deletes newly uploaded sprites when the conditional update loses, and removes the replaced asset after a successful update. The long-duration canvas-cap comment was also resolved because the current implementation no longer applies that cap. I resolved those three threads. The trim-handle visibility comment remains open and was not reposted.
The implementation otherwise has good ownership checks, bounded server frame generation, cleanup coverage, and browser fallback state handling. I did not count the migration concern as actionable because the Clips startup ensureAdditiveColumns safety net adds missing schema columns for existing databases.
New Findings
- 🟡 MEDIUM — Pointer events from trim handles/range bubble to the parent waveform scrub handlers, so dragging a trim selection also seeks the video.
- 🟡 MEDIUM — At high zoom, the displayed sprite-cell count is capped at the source sprite frame count, causing each cell to become much wider than the video aspect ratio and stretching thumbnails.
🧪 Browser testing: Will run after this review (PR touches UI code)
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Code Review Summary
The latest code still preserves the previously open trim-visibility, trim-drag bubbling, and high-zoom thumbnail aspect-ratio comments; I did not repost them. The upload-race cleanup and concurrent replacement guards remain present. The new duration cap keeps logical track sizing bounded, but the backing-store clamp introduces a separate rendering problem: canvas drawing still uses full logical coordinates while the bitmap is capped, so overlays and waveform content are clipped/stretched on wide tracks.
I did not report the migration concern or hardcoded constant duplication as actionable: Clips has the additive-column startup safety net, and the duplicated values currently match without a user-facing defect.
New Finding
- 🟡 MEDIUM — The canvas backing-store width is capped without scaling the drawing coordinate system, causing wide-track waveform/selection overlays to clip or blur while the DOM canvas remains full width.
🧪 Browser testing: Will run after this review (PR touches UI code)
Description
Refactor the video editor timeline track visualization in
clipsto align with Canva-style filmstrip presentation standards.What Changed
FRAME_SCRIM) when video filmstrip thumbnails or sprites (hasImagery) are present.!hasImagery).Files Modified
templates/clips/app/components/editor/waveform.tsxVerification