From acd951cb65aa65ce6d5aa8da232c43eb29a3a493 Mon Sep 17 00:00:00 2001 From: webadderall <131426131+webadderall@users.noreply.github.com> Date: Sun, 19 Apr 2026 17:23:03 +1000 Subject: [PATCH] fix: address second round of CodeRabbit review feedback - Audio regions now play at timeline rate (basePlaybackRate: 1), not clip speed - Persist whisper executable/model paths to editor preferences on change - Don't reset whisper model UI state when deletion fails - Disable auto-captioning when captions are cleared - Restore preview playback after GIF exports (was only restored for MP4) - Don't auto-create full clip after loading a project with empty clip set - Use source coordinates for thumbnail-derived speed regions --- .../video-editor/hooks/useEditorAudioSync.ts | 8 ++------ .../video-editor/hooks/useEditorCaptions.ts | 14 ++++++++------ .../video-editor/hooks/useEditorExport.ts | 10 +++++----- .../video-editor/hooks/useEditorRegions.ts | 2 +- src/components/video-editor/videoEditorUtils.ts | 4 ++-- 5 files changed, 18 insertions(+), 20 deletions(-) diff --git a/src/components/video-editor/hooks/useEditorAudioSync.ts b/src/components/video-editor/hooks/useEditorAudioSync.ts index 66821983..2c2bf1a4 100644 --- a/src/components/video-editor/hooks/useEditorAudioSync.ts +++ b/src/components/video-editor/hooks/useEditorAudioSync.ts @@ -189,10 +189,6 @@ export function useEditorAudioSync({ useEffect(() => { const currentTimeMs = currentTime * 1000; const timelineMs = mapSourceTimeToTimelineTime(currentTimeMs); - const activeSpeedRegion = speedRegions.find( - (r) => currentTimeMs >= r.startMs && currentTimeMs < r.endMs, - ); - const targetPlaybackRate = activeSpeedRegion ? activeSpeedRegion.speed : 1; for (const region of audioRegions) { const audio = audioElementsRef.current.get(region.id); @@ -203,7 +199,7 @@ export function useEditorAudioSync({ if (Math.abs(audio.currentTime - audioOffset) > 0.2) audio.currentTime = audioOffset; const syncedRate = getMediaSyncPlaybackRate({ - basePlaybackRate: targetPlaybackRate, + basePlaybackRate: 1, currentTime: audio.currentTime, targetTime: audioOffset, }); @@ -214,7 +210,7 @@ export function useEditorAudioSync({ if (!audio.paused) audio.pause(); } } - }, [isPlaying, currentTime, audioRegions, speedRegions, mapSourceTimeToTimelineTime]); + }, [isPlaying, currentTime, audioRegions, mapSourceTimeToTimelineTime]); // Sync source-audio fallback elements with video playback useEffect(() => { diff --git a/src/components/video-editor/hooks/useEditorCaptions.ts b/src/components/video-editor/hooks/useEditorCaptions.ts index 19048175..fffe5d01 100644 --- a/src/components/video-editor/hooks/useEditorCaptions.ts +++ b/src/components/video-editor/hooks/useEditorCaptions.ts @@ -4,7 +4,7 @@ import { useCallback, useEffect, useState } from "react"; import { toast } from "sonner"; import { resolveAutoCaptionSourcePath } from "../autoCaptionSource"; -import { loadEditorPreferences } from "../editorPreferences"; +import { loadEditorPreferences, saveEditorPreferences } from "../editorPreferences"; import { resolveVideoUrl } from "../projectPersistence"; import { type AutoCaptionSettings, type CaptionCue, DEFAULT_AUTO_CAPTION_SETTINGS } from "../types"; @@ -48,9 +48,12 @@ export function useEditorCaptions({ // Persist whisper paths to preferences useEffect(() => { - // whisperExecutablePath and whisperModelPath are already persisted via - // useEditorPreferences effect if they were passed to it. Here we only need - // to persist after they change within this hook. + const current = loadEditorPreferences(); + saveEditorPreferences({ + ...current, + whisperExecutablePath, + whisperModelPath, + }); }, [whisperExecutablePath, whisperModelPath]); // Load whisper model status on mount @@ -118,8 +121,6 @@ export function useEditorCaptions({ const result = await window.electronAPI.deleteWhisperSmallModel(); if (!result.success) { toast.error(result.error || "Failed to delete Whisper small model"); - setWhisperModelDownloadStatus("idle"); - setWhisperModelDownloadProgress(0); return; } setWhisperModelPath((cur) => (cur === downloadedWhisperModelPath ? null : cur)); @@ -202,6 +203,7 @@ export function useEditorCaptions({ const handleClearAutoCaptions = useCallback(() => { setAutoCaptions([]); + setAutoCaptionSettings((prev) => ({ ...prev, enabled: false })); }, []); /** Reset captions from a freshly loaded project. */ diff --git a/src/components/video-editor/hooks/useEditorExport.ts b/src/components/video-editor/hooks/useEditorExport.ts index 68e73a66..81580bf8 100644 --- a/src/components/video-editor/hooks/useEditorExport.ts +++ b/src/components/video-editor/hooks/useEditorExport.ts @@ -559,12 +559,12 @@ export function useEditorExport({ return; } } + } - if (wasPlaying) { - videoPlaybackRef.current?.play(); - } else { - video.currentTime = restoreTime; - } + if (wasPlaying) { + videoPlaybackRef.current?.play(); + } else { + video.currentTime = restoreTime; } } catch (error) { console.error("Export error:", error); diff --git a/src/components/video-editor/hooks/useEditorRegions.ts b/src/components/video-editor/hooks/useEditorRegions.ts index fde29f44..d8fc1220 100644 --- a/src/components/video-editor/hooks/useEditorRegions.ts +++ b/src/components/video-editor/hooks/useEditorRegions.ts @@ -633,7 +633,7 @@ export function useEditorRegions({ }) => { setZoomRegions(editor.zoomRegions); setClipRegions(editor.clipRegions); - clipInitializedRef.current = editor.clipRegions.length > 0; + clipInitializedRef.current = true; setAnnotationRegions(editor.annotationRegions); setAudioRegions(editor.audioRegions); setSelectedZoomId(null); diff --git a/src/components/video-editor/videoEditorUtils.ts b/src/components/video-editor/videoEditorUtils.ts index 8d1243bf..3845267b 100644 --- a/src/components/video-editor/videoEditorUtils.ts +++ b/src/components/video-editor/videoEditorUtils.ts @@ -21,7 +21,7 @@ import type { ZoomRegion, ZoomTransitionEasing, } from "./types"; -import { getClipSourceEndMs } from "./types"; +import { getClipSourceEndMs, getClipSourceStartMs } from "./types"; import type { VideoPlaybackRef } from "./VideoPlayback"; export const DEFAULT_MP4_EXPORT_FRAME_RATE: ExportMp4FrameRate = 30; @@ -248,7 +248,7 @@ export async function captureVideoThumbnail(config: ThumbnailRenderConfig): Prom .filter((clip) => clip.speed !== 1) .map((clip) => ({ id: `clip-speed-${clip.id}`, - startMs: clip.startMs, + startMs: getClipSourceStartMs(clip), endMs: getClipSourceEndMs(clip), speed: clip.speed as SpeedRegion["speed"], }));