From 5579bb3bf3c0dfad8fbda0620e40e984b23a21e6 Mon Sep 17 00:00:00 2001 From: cbaucom Date: Mon, 22 Jun 2026 22:45:28 -0400 Subject: [PATCH] Fix timeline playhead alignment and scroll-on-select behavior. Position media blocks on the render plan time axis so the red playhead tracks thumbnails when durations change, and only auto-scroll during playback. Co-authored-by: Cursor --- specs/issue-timeline-playhead-alignment.md | 31 ++++++++++++++ src/editor-shell/App.tsx | 5 +++ src/editor-shell/PlayerPane.tsx | 33 ++++++++++++++- src/editor-shell/TimelineMediaBlock.tsx | 2 +- src/editor-shell/TimelinePanel.tsx | 45 ++++++++++++++++++--- src/sequence-planner/timelineLayout.test.ts | 45 +++++++++++++++++++++ src/sequence-planner/timelineLayout.ts | 35 +++++++++------- 7 files changed, 174 insertions(+), 22 deletions(-) create mode 100644 specs/issue-timeline-playhead-alignment.md diff --git a/specs/issue-timeline-playhead-alignment.md b/specs/issue-timeline-playhead-alignment.md new file mode 100644 index 0000000..5eecd64 --- /dev/null +++ b/specs/issue-timeline-playhead-alignment.md @@ -0,0 +1,31 @@ +# Timeline playhead ↔ thumbnail alignment + scroll behavior + +GitHub issue: #57 + +## Problem + +1. **Misalignment**: Media blocks in `buildTimelineLayout` are placed with cumulative packing (`leftPx += width + gap`), but the playhead uses `currentFrame * pixelsPerFrame` (time axis). Changing slide duration widens/narrows packed blocks without moving them on the time axis, so the red line no longer lines up with thumbnails. +2. **Unwanted scroll**: `TimelinePanel` auto-scrolls to the playhead whenever it is outside the viewport, even while paused. Scrolling away to click a distant thumbnail snaps back to the old playhead position before the seek completes. + +## Fix + +### `sequence-planner/timelineLayout.ts` + +- Position **included** media blocks at `entry.startFrame * pixelsPerFrame` (same coordinate system as playhead and audio lane). +- **Excluded** slides (no plan entry) keep compact packed placement after the previous block in storyboard order so reorder UX is unchanged. +- Update unit tests for start-frame positioning and transition overlap cases. + +### `editor-shell/App.tsx` + +- When a slide select triggers a seek, synchronously update `currentFrame` / `currentSlideId` (don't wait for player RAF poll). +- Track `isPlaying` via Remotion player `play` / `pause` events. + +### `editor-shell/TimelinePanel.tsx` + +- Auto-scroll to playhead **only while playing**. +- When paused and the user selects a single slide, scroll that block into view (centre if off-screen). + +## Testing + +- Extend `timelineLayout.test.ts` for start-frame block positions. +- `pnpm test`, `pnpm lint`, `pnpm build`. diff --git a/src/editor-shell/App.tsx b/src/editor-shell/App.tsx index a400b35..bca324e 100644 --- a/src/editor-shell/App.tsx +++ b/src/editor-shell/App.tsx @@ -35,6 +35,7 @@ export function App() { const [currentFrame, setCurrentFrame] = useState(0) const [currentSlideId, setCurrentSlideId] = useState(null) const [exporting, setExporting] = useState(false) + const [isPlaying, setIsPlaying] = useState(false) const [sidebarOpenSections, setSidebarOpenSections] = useState(['settings', 'soundtrack']) const clearSelectionRef = useRef<(() => void) | null>(null) @@ -205,6 +206,8 @@ export function App() { const startFrame = startFrameForSlideId(renderPlan, id) if (startFrame !== null) { playerRef.current?.seekTo(startFrame) + setCurrentFrame(startFrame) + setCurrentSlideId(id) } } }, [renderPlan, selectSlide]) @@ -272,6 +275,7 @@ export function App() { compositionHeight={canvas.height} compositionWidth={canvas.width} onFrameChange={handleFrameChange} + onPlayingChange={setIsPlaying} playerRef={playerRef} renderPlan={renderPlan} totalFrames={totalFrames} @@ -283,6 +287,7 @@ export function App() { audioTracks={audioTracks} currentFrame={currentFrame} currentSlideId={currentSlideId} + isPlaying={isPlaying} loudnessCache={loudnessCache} onClearSelection={clearSelection} onMoveToBeginning={handleMoveToBeginning} diff --git a/src/editor-shell/PlayerPane.tsx b/src/editor-shell/PlayerPane.tsx index 780a1cd..44cb6b7 100644 --- a/src/editor-shell/PlayerPane.tsx +++ b/src/editor-shell/PlayerPane.tsx @@ -13,12 +13,21 @@ type Props = { compositionHeight: number compositionWidth: number onFrameChange: (frame: number) => void + onPlayingChange?: (isPlaying: boolean) => void playerRef?: React.RefObject renderPlan: RenderPlan totalFrames: number } -export function PlayerPane({ compositionHeight, compositionWidth, onFrameChange, playerRef, renderPlan, totalFrames }: Props) { +export function PlayerPane({ + compositionHeight, + compositionWidth, + onFrameChange, + onPlayingChange, + playerRef, + renderPlan, + totalFrames, +}: Props) { const embeddedHostRef = useRef(null) const presentationHostRef = useRef(null) const fallbackPlayerRef = useRef(null) @@ -26,6 +35,28 @@ export function PlayerPane({ compositionHeight, compositionWidth, onFrameChange, const [isPresenting, setIsPresenting] = useState(false) const [presentationFrame, setPresentationFrame] = useState(0) + useEffect(() => { + const player = resolvedPlayerRef.current + if (!player || !onPlayingChange) return + + function handlePlay() { + onPlayingChange?.(true) + } + + function handlePause() { + onPlayingChange?.(false) + } + + player.addEventListener('play', handlePlay) + player.addEventListener('pause', handlePause) + onPlayingChange?.(player.isPlaying()) + + return () => { + player.removeEventListener('play', handlePlay) + player.removeEventListener('pause', handlePause) + } + }, [onPlayingChange, isPresenting, renderPlan, resolvedPlayerRef, totalFrames]) + useEffect(() => { let animationFrameId = 0 let lastReportedFrame = -1 diff --git a/src/editor-shell/TimelineMediaBlock.tsx b/src/editor-shell/TimelineMediaBlock.tsx index ba35f9d..57a1a42 100644 --- a/src/editor-shell/TimelineMediaBlock.tsx +++ b/src/editor-shell/TimelineMediaBlock.tsx @@ -72,7 +72,7 @@ export function TimelineMediaBlock({
  • void onMoveToBeginning: (indices: number[]) => void @@ -38,6 +46,7 @@ export function TimelinePanel({ audioTracks, currentFrame, currentSlideId, + isPlaying, loudnessCache, onClearSelection, onMoveToBeginning, @@ -53,6 +62,7 @@ export function TimelinePanel({ }: Props) { const scrollRef = useRef(null) const mediaDragRef = useRef(null) + const lastScrolledSelectionRef = useRef(null) const { waveformCache } = useWaveformPeaks({ audioClips, audioTracks }) const { pixelsPerFrame, @@ -90,19 +100,42 @@ export function TimelinePanel({ onSeek(clampedFrame) }, [onClearSelection, onSeek, pixelsPerFrame, renderPlan.totalFrames]) - useEffect(() => { + const scrollToCenterPx = useCallback((targetPx: number) => { const scrollElement = scrollRef.current if (!scrollElement) return - const playheadX = playheadLeftPx + const margin = TIMELINE_SCROLL_MARGIN_PX const viewStart = scrollElement.scrollLeft const viewEnd = viewStart + scrollElement.clientWidth - const margin = 48 - if (playheadX < viewStart + margin || playheadX > viewEnd - margin) { - scrollElement.scrollLeft = Math.max(0, playheadX - scrollElement.clientWidth / 2) + if (targetPx < viewStart + margin || targetPx > viewEnd - margin) { + scrollElement.scrollLeft = Math.max(0, targetPx - scrollElement.clientWidth / 2) + } + }, []) + + useEffect(() => { + if (!isPlaying) return + scrollToCenterPx(playheadLeftPx) + }, [isPlaying, playheadLeftPx, scrollToCenterPx]) + + const selectedSlideId = selectedSlideIds.size === 1 ? [...selectedSlideIds][0] : null + + useEffect(() => { + if (!selectedSlideId) { + lastScrolledSelectionRef.current = null } - }, [playheadLeftPx]) + }, [selectedSlideId]) + + useEffect(() => { + if (isPlaying || !selectedSlideId) return + if (lastScrolledSelectionRef.current === selectedSlideId) return + + const block = layout.mediaBlocks.find((entry) => entry.slideId === selectedSlideId) + if (!block) return + + scrollToCenterPx(blockCenterPx(block)) + lastScrolledSelectionRef.current = selectedSlideId + }, [isPlaying, layout.mediaBlocks, scrollToCenterPx, selectedSlideId]) return (
    diff --git a/src/sequence-planner/timelineLayout.test.ts b/src/sequence-planner/timelineLayout.test.ts index f7ac221..a0cc14b 100644 --- a/src/sequence-planner/timelineLayout.test.ts +++ b/src/sequence-planner/timelineLayout.test.ts @@ -71,11 +71,56 @@ describe('buildTimelineLayout', () => { [], ) + expect(layout.mediaBlocks[0].leftPx).toBe(0) expect(layout.mediaBlocks[0].widthPx).toBe(60 * DEFAULT_PIXELS_PER_FRAME) + expect(layout.mediaBlocks[1].leftPx).toBe(45 * DEFAULT_PIXELS_PER_FRAME) expect(layout.mediaBlocks[1].widthPx).toBe(120 * DEFAULT_PIXELS_PER_FRAME) expect(layout.totalWidthPx).toBeGreaterThanOrEqual(165 * DEFAULT_PIXELS_PER_FRAME) }) + it('aligns included slides to render plan start frames', () => { + const renderPlan: RenderPlan = { + entries: [ + imageEntry('a', 0, 90), + imageEntry('b', 75, 90), + ], + totalFrames: 165, + } + + const layout = buildTimelineLayout( + [ + renderPlan.entries[0].slide, + renderPlan.entries[1].slide, + ], + renderPlan, + [], + ) + + expect(layout.mediaBlocks[0].leftPx).toBe(0) + expect(layout.mediaBlocks[1].leftPx).toBe(75 * DEFAULT_PIXELS_PER_FRAME) + }) + + it('packs excluded slides after the previous block in storyboard order', () => { + const included = imageEntry('a', 0, 60) + const excludedSlide = { + ...imageEntry('b', 0, 30).slide, + excluded: true, + } + const renderPlan: RenderPlan = { + entries: [included], + totalFrames: 60, + } + + const layout = buildTimelineLayout( + [included.slide, excludedSlide], + renderPlan, + [], + ) + + expect(layout.mediaBlocks[0].leftPx).toBe(0) + expect(layout.mediaBlocks[1].leftPx).toBe(60 * DEFAULT_PIXELS_PER_FRAME + 4) + }) + it('enforces a minimum block width for very short slides', () => { const renderPlan: RenderPlan = { entries: [imageEntry('a', 0, 5)], diff --git a/src/sequence-planner/timelineLayout.ts b/src/sequence-planner/timelineLayout.ts index 1d7439e..0ec89dd 100644 --- a/src/sequence-planner/timelineLayout.ts +++ b/src/sequence-planner/timelineLayout.ts @@ -54,32 +54,39 @@ function blockWidthPx( return Math.max(minBlockWidthPx, durationInFrames * pixelsPerFrame) } -function durationInFramesForSlide(slide: Slide, renderPlan: RenderPlan): number { - const entry = firstPassEntries(renderPlan).find((planEntry) => planEntry.slide.id === slide.id) - if (entry) return entry.durationInFrames - if (isTitleSlide(slide)) return slide.durationInFrames - return slide.durationInFrames -} - function buildMediaBlocks( slides: Slide[], renderPlan: RenderPlan, pixelsPerFrame: number, minBlockWidthPx: number, ): TimelineMediaBlock[] { - let leftPx = 0 + const entryBySlideId = new Map( + firstPassEntries(renderPlan).map((entry) => [entry.slide.id, entry]), + ) + let packedLeftPx = 0 const blocks: TimelineMediaBlock[] = [] for (const slide of slides) { - const durationInFrames = durationInFramesForSlide(slide, renderPlan) + const entry = slide.excluded ? undefined : entryBySlideId.get(slide.id) + const durationInFrames = entry?.durationInFrames + ?? (isTitleSlide(slide) ? slide.durationInFrames : slide.durationInFrames) const widthPx = blockWidthPx(durationInFrames, pixelsPerFrame, minBlockWidthPx) + const leftPx = entry + ? entry.startFrame * pixelsPerFrame + : packedLeftPx + blocks.push({ durationInFrames, leftPx, slideId: slide.id, widthPx, }) - leftPx += widthPx + TIMELINE_BLOCK_GAP_PX + + if (entry) { + packedLeftPx = Math.max(packedLeftPx, leftPx + widthPx + TIMELINE_BLOCK_GAP_PX) + } else { + packedLeftPx += widthPx + TIMELINE_BLOCK_GAP_PX + } } return blocks @@ -93,10 +100,10 @@ export function buildTimelineLayout( minBlockWidthPx = DEFAULT_MIN_BLOCK_WIDTH_PX, ): TimelineLayout { const mediaBlocks = buildMediaBlocks(slides, renderPlan, pixelsPerFrame, minBlockWidthPx) - const mediaContentWidthPx = mediaBlocks.length > 0 - ? mediaBlocks[mediaBlocks.length - 1].leftPx - + mediaBlocks[mediaBlocks.length - 1].widthPx - : 0 + const mediaContentWidthPx = mediaBlocks.reduce( + (maxEnd, block) => Math.max(maxEnd, block.leftPx + block.widthPx), + 0, + ) const audioEndPx = (renderPlan.audioSegments ?? []).reduce( (maxEnd, segment) => Math.max( maxEnd,