fix(projects): streamline import and save flow#679
Open
webadderall wants to merge 7 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR extends the Electron video file picker to support project file selection alongside media with early validation to reject malformed projects before media approval, introduces a Promise-based "Save Project" dialog to VideoEditor, adds an Import button to ProjectBrowserDialog, adds unified media/project import handling in the editor, enables inline auto-caption editing in VideoPlayback with improved caption word normalization for blank entries and editor wiring, refactors AnnotationOverlay to use a scene-transform-based coordinate system and threads scene transformations through the annotation export rendering pipeline, fixes transient webcam fields being counted as unsaved changes, registers the Recordly project file type on macOS, and removes the connected zoom transition path from zoomRegionUtils. ChangesProject Import, Validation, and Save As Dialog
Inline Auto-Caption Editing in Playback
Annotation Scene Transform Support in Export Rendering
Transient Webcam Dirty State Fix
macOS Project File Type Registration
Zoom Region Transition Removal
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 149, 237, 0.5)
Note over Renderer,loadProjectFromPath: Project/Media Import Flow with Validation
end
participant Renderer as VideoEditor / LaunchWindow
participant PreloadBridge as electronAPI.openVideoFilePicker
participant IPCHandler as open-video-file-picker
participant loadProjectFromPath as loadProjectFromPath + validation
Renderer->>PreloadBridge: openVideoFilePicker({ includeProjects: true })
PreloadBridge->>IPCHandler: ipcRenderer.invoke("open-video-file-picker", options)
IPCHandler->>IPCHandler: show OS dialog (video + project filters)
alt project file selected
IPCHandler->>loadProjectFromPath: loadProjectFromPath(filePath)
alt valid project
loadProjectFromPath-->>IPCHandler: projectResult
IPCHandler-->>PreloadBridge: { success, kind: "project", project, ... }
else invalid project
loadProjectFromPath-->>IPCHandler: { success: false, message: "Invalid project file format" }
IPCHandler-->>PreloadBridge: error result
end
PreloadBridge-->>Renderer: result
Renderer->>Renderer: applyLoadedProjectState / switchToEditor
else media file selected
IPCHandler-->>PreloadBridge: { success, kind: "media", path, extension }
PreloadBridge-->>Renderer: result
Renderer->>Renderer: setCurrentVideoPath / reset editor state
end
sequenceDiagram
rect rgba(144, 238, 144, 0.5)
Note over saveProject,saveProjectWithName: Save Project Dialog Flow
end
participant saveProject
participant openProjectSaveDialog
participant SaveDialog as Save Project Dialog UI
participant handleProjectSaveDialogSubmit
participant saveProjectWithName
saveProject->>openProjectSaveDialog: forceSaveAs or no project path
openProjectSaveDialog->>SaveDialog: open dialog, set draft name
SaveDialog-->>saveProject: Promise<boolean> (pending)
Note over SaveDialog: user types name, clicks Save
SaveDialog->>handleProjectSaveDialogSubmit: submit
handleProjectSaveDialogSubmit->>saveProjectWithName: trimmed name
saveProjectWithName-->>handleProjectSaveDialogSubmit: result
handleProjectSaveDialogSubmit->>openProjectSaveDialog: resolve(true)
openProjectSaveDialog-->>saveProject: saved = true
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/projectDirtyState.ts`:
- Around line 56-68: After stripping transient webcam fields (enabled,
sourcePath, timeOffsetMs) in the destructuring assignment, add a check to
determine whether the remaining persistentWebcamFields object is empty. When
constructing the return object, conditionally include the webcam key only if
persistentWebcamFields has at least one property; if it is empty, omit the
webcam key entirely from the editor object. This ensures both snapshots are
normalized consistently—either both have an empty webcam object or neither has
the webcam key—preventing false dirty state comparisons when only transient
webcam properties have changed.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2877-2883: The Save As functionality (triggered by forceSaveAs)
incorrectly uses saveProjectWithName() which calls the IPC path
save-project-file-named that deletes the active project when the target differs,
converting Save As into a move operation instead of creating a copy. Either
create a new distinct IPC handler path for saving a copy with a new name (that
preserves the active project) or modify the existing save-project-file-named
handler to accept an explicit mode parameter that prevents deletion of the
original file. Apply this fix at both the location where openProjectSaveDialog
is called (around line 2877-2883) and where its result is processed (around line
3069-3099).
- Around line 3168-3227: The handleImportMediaOrProject callback applies a
loaded project or imports media and resets editor state without checking for
unsaved changes, which can discard work. Add an unsaved changes guard at the
start of this handler after the initial error checks: if hasUnsavedChanges is
true, either show a confirmation dialog asking the user to confirm they want to
discard changes before proceeding, or require them to save first. This guard
should run before applyLoadedProject is called and before the media import
branch executes the state reset operations.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eb8c6c33-9f68-40cb-badc-9545cca57237
📒 Files selected for processing (8)
electron/electron-env.d.tselectron/ipc/register/captions.tselectron/preload.tssrc/components/launch/hooks/useLaunchWindowActions.tssrc/components/video-editor/ProjectBrowserDialog.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/projectDirtyState.tssrc/components/video-editor/videoPlayback/zoomRegionUtils.ts
Comment on lines
+56
to
+68
| const { | ||
| enabled: _enabled, | ||
| sourcePath: _sourcePath, | ||
| timeOffsetMs: _timeOffsetMs, | ||
| ...persistentWebcamFields | ||
| } = webcam; | ||
|
|
||
| return { | ||
| ...project, | ||
| editor: { | ||
| ...editor, | ||
| webcam: persistentWebcamFields, | ||
| }, |
Contributor
There was a problem hiding this comment.
Normalize away empty webcam objects after stripping transient fields.
Stripping transient keys can leave webcam: {} on one snapshot while the other snapshot has no webcam key at all; that still compares as dirty even when only transient webcam state changed.
Suggested fix
const {
enabled: _enabled,
sourcePath: _sourcePath,
timeOffsetMs: _timeOffsetMs,
...persistentWebcamFields
} = webcam;
+
+ const { webcam: _webcam, ...editorWithoutWebcam } = editor;
+ const hasPersistentWebcamFields = Object.keys(persistentWebcamFields).length > 0;
return {
...project,
editor: {
- ...editor,
- webcam: persistentWebcamFields,
+ ...editorWithoutWebcam,
+ ...(hasPersistentWebcamFields ? { webcam: persistentWebcamFields } : {}),
},
};
}🤖 Prompt for 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.
In `@src/components/video-editor/projectDirtyState.ts` around lines 56 - 68, After
stripping transient webcam fields (enabled, sourcePath, timeOffsetMs) in the
destructuring assignment, add a check to determine whether the remaining
persistentWebcamFields object is empty. When constructing the return object,
conditionally include the webcam key only if persistentWebcamFields has at least
one property; if it is empty, omit the webcam key entirely from the editor
object. This ensures both snapshots are normalized consistently—either both have
an empty webcam object or neither has the webcam key—preventing false dirty
state comparisons when only transient webcam properties have changed.
Comment on lines
+2877
to
+2883
| if (forceSaveAs || !targetProjectPath) { | ||
| if (options?.silent) { | ||
| return false; | ||
| } | ||
|
|
||
| return openProjectSaveDialog(projectDisplayName || fileNameBase); | ||
| } |
Contributor
There was a problem hiding this comment.
Keep Save As separate from the rename-style named-save path.
forceSaveAs now opens this dialog, but submit calls saveProjectWithName(), whose IPC path save-project-file-named deletes the active project when the new named target differs. That makes Save As remove the original project instead of creating a copy; use a distinct “save copy as named project” path or pass an explicit mode that preserves the active file. See electron/ipc/register/project.ts:354-429 for the delete-on-rename behavior.
Also applies to: 3069-3099
🤖 Prompt for 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.
In `@src/components/video-editor/VideoEditor.tsx` around lines 2877 - 2883, The
Save As functionality (triggered by forceSaveAs) incorrectly uses
saveProjectWithName() which calls the IPC path save-project-file-named that
deletes the active project when the target differs, converting Save As into a
move operation instead of creating a copy. Either create a new distinct IPC
handler path for saving a copy with a new name (that preserves the active
project) or modify the existing save-project-file-named handler to accept an
explicit mode parameter that prevents deletion of the original file. Apply this
fix at both the location where openProjectSaveDialog is called (around line
2877-2883) and where its result is processed (around line 3069-3099).
Comment on lines
+3168
to
+3227
| const handleImportMediaOrProject = useCallback(async () => { | ||
| const result = await window.electronAPI.openVideoFilePicker({ includeProjects: true }); | ||
|
|
||
| if (result.canceled) { | ||
| return; | ||
| } | ||
|
|
||
| if (!result.success) { | ||
| toast.error(result.message || "Failed to import file"); | ||
| return; | ||
| } | ||
|
|
||
| if (result.kind === "project" || result.project) { | ||
| const restored = await applyLoadedProject(result.project, result.path ?? null); | ||
| if (!restored) { | ||
| toast.error("Invalid project file format"); | ||
| return; | ||
| } | ||
|
|
||
| setProjectBrowserOpen(false); | ||
| await refreshProjectLibrary(); | ||
| toast.success(result.path ? `Project loaded from ${result.path}` : "Project loaded"); | ||
| return; | ||
| } | ||
|
|
||
| if (!result.path) { | ||
| toast.error("No media file selected"); | ||
| return; | ||
| } | ||
|
|
||
| const sourcePath = fromFileUrl(result.path); | ||
| const sourceVideoUrl = await resolveVideoUrl(sourcePath); | ||
| try { | ||
| videoPlaybackRef.current?.pause(); | ||
| } catch { | ||
| // no-op | ||
| } | ||
|
|
||
| setIsPlaying(false); | ||
| setCurrentTime(0); | ||
| setDuration(0); | ||
| setVideoSourcePath(sourcePath); | ||
| setVideoPath(sourceVideoUrl); | ||
| setCurrentProjectPath(null); | ||
| setLastSavedSnapshot(null); | ||
| resetSourceScopedEditorState(); | ||
| pendingFreshRecordingAutoZoomPathRef.current = autoApplyFreshRecordingAutoZooms | ||
| ? sourceVideoUrl | ||
| : null; | ||
| setWebcam((prev) => ({ | ||
| ...prev, | ||
| enabled: false, | ||
| sourcePath: null, | ||
| timeOffsetMs: DEFAULT_WEBCAM_TIME_OFFSET_MS, | ||
| })); | ||
| applySessionPresentation(null); | ||
| await window.electronAPI.setCurrentVideoPath(sourcePath, { preserveProjectPath: false }); | ||
| setProjectBrowserOpen(false); | ||
| await refreshProjectLibrary(); | ||
| toast.success("Media imported"); |
Contributor
There was a problem hiding this comment.
Gate imports on unsaved-change handling before replacing editor state.
This handler applies a loaded project or media file and resets the current editor state without checking hasUnsavedChanges, so importing from the project browser can discard an unsaved/first-time project. Save first or require an explicit discard confirmation before applyLoadedProject or the media reset branch runs.
Possible localized guard
if (!result.success) {
toast.error(result.message || "Failed to import file");
return;
}
+ if (hasUnsavedChanges) {
+ const saved = await saveProject(false);
+ if (!saved) {
+ return;
+ }
+ }
+
if (result.kind === "project" || result.project) { }, [
applyLoadedProject,
applySessionPresentation,
autoApplyFreshRecordingAutoZooms,
+ hasUnsavedChanges,
refreshProjectLibrary,
resetSourceScopedEditorState,
+ saveProject,
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleImportMediaOrProject = useCallback(async () => { | |
| const result = await window.electronAPI.openVideoFilePicker({ includeProjects: true }); | |
| if (result.canceled) { | |
| return; | |
| } | |
| if (!result.success) { | |
| toast.error(result.message || "Failed to import file"); | |
| return; | |
| } | |
| if (result.kind === "project" || result.project) { | |
| const restored = await applyLoadedProject(result.project, result.path ?? null); | |
| if (!restored) { | |
| toast.error("Invalid project file format"); | |
| return; | |
| } | |
| setProjectBrowserOpen(false); | |
| await refreshProjectLibrary(); | |
| toast.success(result.path ? `Project loaded from ${result.path}` : "Project loaded"); | |
| return; | |
| } | |
| if (!result.path) { | |
| toast.error("No media file selected"); | |
| return; | |
| } | |
| const sourcePath = fromFileUrl(result.path); | |
| const sourceVideoUrl = await resolveVideoUrl(sourcePath); | |
| try { | |
| videoPlaybackRef.current?.pause(); | |
| } catch { | |
| // no-op | |
| } | |
| setIsPlaying(false); | |
| setCurrentTime(0); | |
| setDuration(0); | |
| setVideoSourcePath(sourcePath); | |
| setVideoPath(sourceVideoUrl); | |
| setCurrentProjectPath(null); | |
| setLastSavedSnapshot(null); | |
| resetSourceScopedEditorState(); | |
| pendingFreshRecordingAutoZoomPathRef.current = autoApplyFreshRecordingAutoZooms | |
| ? sourceVideoUrl | |
| : null; | |
| setWebcam((prev) => ({ | |
| ...prev, | |
| enabled: false, | |
| sourcePath: null, | |
| timeOffsetMs: DEFAULT_WEBCAM_TIME_OFFSET_MS, | |
| })); | |
| applySessionPresentation(null); | |
| await window.electronAPI.setCurrentVideoPath(sourcePath, { preserveProjectPath: false }); | |
| setProjectBrowserOpen(false); | |
| await refreshProjectLibrary(); | |
| toast.success("Media imported"); | |
| const handleImportMediaOrProject = useCallback(async () => { | |
| const result = await window.electronAPI.openVideoFilePicker({ includeProjects: true }); | |
| if (result.canceled) { | |
| return; | |
| } | |
| if (!result.success) { | |
| toast.error(result.message || "Failed to import file"); | |
| return; | |
| } | |
| if (hasUnsavedChanges) { | |
| const saved = await saveProject(false); | |
| if (!saved) { | |
| return; | |
| } | |
| } | |
| if (result.kind === "project" || result.project) { | |
| const restored = await applyLoadedProject(result.project, result.path ?? null); | |
| if (!restored) { | |
| toast.error("Invalid project file format"); | |
| return; | |
| } | |
| setProjectBrowserOpen(false); | |
| await refreshProjectLibrary(); | |
| toast.success(result.path ? `Project loaded from ${result.path}` : "Project loaded"); | |
| return; | |
| } | |
| if (!result.path) { | |
| toast.error("No media file selected"); | |
| return; | |
| } | |
| const sourcePath = fromFileUrl(result.path); | |
| const sourceVideoUrl = await resolveVideoUrl(sourcePath); | |
| try { | |
| videoPlaybackRef.current?.pause(); | |
| } catch { | |
| // no-op | |
| } | |
| setIsPlaying(false); | |
| setCurrentTime(0); | |
| setDuration(0); | |
| setVideoSourcePath(sourcePath); | |
| setVideoPath(sourceVideoUrl); | |
| setCurrentProjectPath(null); | |
| setLastSavedSnapshot(null); | |
| resetSourceScopedEditorState(); | |
| pendingFreshRecordingAutoZoomPathRef.current = autoApplyFreshRecordingAutoZooms | |
| ? sourceVideoUrl | |
| : null; | |
| setWebcam((prev) => ({ | |
| ...prev, | |
| enabled: false, | |
| sourcePath: null, | |
| timeOffsetMs: DEFAULT_WEBCAM_TIME_OFFSET_MS, | |
| })); | |
| applySessionPresentation(null); | |
| await window.electronAPI.setCurrentVideoPath(sourcePath, { preserveProjectPath: false }); | |
| setProjectBrowserOpen(false); | |
| await refreshProjectLibrary(); | |
| toast.success("Media imported"); | |
| }, [ | |
| applyLoadedProject, | |
| applySessionPresentation, | |
| autoApplyFreshRecordingAutoZooms, | |
| hasUnsavedChanges, | |
| refreshProjectLibrary, | |
| resetSourceScopedEditorState, | |
| saveProject, | |
| ]); |
🤖 Prompt for 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.
In `@src/components/video-editor/VideoEditor.tsx` around lines 3168 - 3227, The
handleImportMediaOrProject callback applies a loaded project or imports media
and resets editor state without checking for unsaved changes, which can discard
work. Add an unsaved changes guard at the start of this handler after the
initial error checks: if hasUnsavedChanges is true, either show a confirmation
dialog asking the user to confirm they want to discard changes before
proceeding, or require them to save first. This guard should run before
applyLoadedProject is called and before the media import branch executes the
state reset operations.
Contributor
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/VideoEditor.tsx`:
- Around line 2835-2839: The handleSaveAutoCaptionEdit function always displays
a success toast message regardless of whether the caption edit actually modified
the state. Since updateCaptionCuesForEditedTarget returns the original cues
unchanged for empty normalized text or empty targets, you should add a check to
verify that the caption was actually updated before calling the success toast.
Compare the result of updateCaptionCuesForEditedTarget with the previous
captions state to determine if a real change occurred, and only emit the success
toast when the captions have genuinely been modified.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 228577cd-a431-462d-b50b-0f511fe8f5d2
📒 Files selected for processing (2)
src/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsx
Comment on lines
+2835
to
+2839
| const handleSaveAutoCaptionEdit = useCallback( | ||
| (target: CaptionEditTarget, text: string) => { | ||
| setAutoCaptions((captions) => updateCaptionCuesForEditedTarget(captions, target, text)); | ||
| toast.success(t("settings.captions.editSaved", "Caption updated")); | ||
| }, |
Contributor
There was a problem hiding this comment.
Avoid showing “Caption updated” when the edit is rejected as a no-op.
updateCaptionCuesForEditedTarget() returns the original cues for empty normalized text or empty targets, but this handler always emits success. Clearing a caption can therefore appear saved while no state changes.
Possible localized guard
const handleSaveAutoCaptionEdit = useCallback(
(target: CaptionEditTarget, text: string) => {
+ if (target.words.length === 0 || text.trim().length === 0) {
+ toast.error(
+ t("settings.captions.editTextRequired", "Enter caption text to save"),
+ );
+ return;
+ }
setAutoCaptions((captions) => updateCaptionCuesForEditedTarget(captions, target, text));
toast.success(t("settings.captions.editSaved", "Caption updated"));
},🤖 Prompt for 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.
In `@src/components/video-editor/VideoEditor.tsx` around lines 2835 - 2839, The
handleSaveAutoCaptionEdit function always displays a success toast message
regardless of whether the caption edit actually modified the state. Since
updateCaptionCuesForEditedTarget returns the original cues unchanged for empty
normalized text or empty targets, you should add a check to verify that the
caption was actually updated before calling the success toast. Compare the
result of updateCaptionCuesForEditedTarget with the previous captions state to
determine if a real change occurred, and only emit the success toast when the
captions have genuinely been modified.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/video-editor/captionLayout.ts (1)
98-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReindex
cueWordIndexafter removing blank words.Line 102 assigns
cueWordIndexbefore Line 111 removes trimmed-empty words. With mixed blank/non-blankcue.words, this leaves holey indices and can cause edit replacement to append instead of replace inupdateCaptionCuesForEditedTarget.Proposed fix
const words = cue.words .filter((word): word is CaptionCueWord => Boolean(word && typeof word.text === "string"), ) - .map((word, cueWordIndex) => ({ + .map((word) => ({ cueId: cue.id, - cueWordIndex, text: word.text.trim(), forcedBreakBefore: false, leadingSpace: Boolean(word.leadingSpace), startMs: word.startMs, endMs: word.endMs, })) - .filter((word) => word.text.length > 0); + .filter((word) => word.text.length > 0) + .map((word, cueWordIndex) => ({ + ...word, + cueWordIndex, + }));🤖 Prompt for 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. In `@src/components/video-editor/captionLayout.ts` around lines 98 - 112, The cueWordIndex is being assigned based on the original word array indices before the empty words filter is applied, creating holey indices when blank words are removed. This causes issues in updateCaptionCuesForEditedTarget where edit replacements append instead of replace. Reorder the operations so that cueWordIndex is assigned after filtering out blank words: first filter the words for valid text and trim them, then in a separate map operation assign sequential cueWordIndex values (0, 1, 2, etc.) to the filtered results. This ensures the indices are contiguous and reflect the actual position in the final words array.
🧹 Nitpick comments (1)
src/lib/exporter/annotationRenderer.ts (1)
11-15: 💤 Low valueConsider exporting
AnnotationSceneTransformfor caller type safety.The interface is used in the exported
renderAnnotationsfunction signature but isn't exported itself. While TypeScript's structural typing allows callers to pass inline objects, exporting the type would let callers reference it for better type safety and documentation.-interface AnnotationSceneTransform { +export interface AnnotationSceneTransform { scale: number; x: number; y: number; }🤖 Prompt for 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. In `@src/lib/exporter/annotationRenderer.ts` around lines 11 - 15, The AnnotationSceneTransform interface is currently not exported but is used in the signature of the exported renderAnnotations function, which prevents callers from directly referencing the type. Add the export keyword to the AnnotationSceneTransform interface definition to allow callers to import and use the type directly for better type safety and clearer API documentation.
🤖 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/lib/exporter/modernFrameRenderer.ts`:
- Around line 1589-1593: The composeBlurAnnotationFrame call is using mutable
this.animationState for the transform properties (scale, x, y) instead of the
temporal snapshot transform. Replace the object passed to
composeBlurAnnotationFrame that reads from this.animationState with
resolvedSnapshot.sceneTransform to ensure blur annotations are rendered at the
correct transform that matches the selected snapshot, preventing misalignment
when temporal zoom blur is enabled.
---
Outside diff comments:
In `@src/components/video-editor/captionLayout.ts`:
- Around line 98-112: The cueWordIndex is being assigned based on the original
word array indices before the empty words filter is applied, creating holey
indices when blank words are removed. This causes issues in
updateCaptionCuesForEditedTarget where edit replacements append instead of
replace. Reorder the operations so that cueWordIndex is assigned after filtering
out blank words: first filter the words for valid text and trim them, then in a
separate map operation assign sequential cueWordIndex values (0, 1, 2, etc.) to
the filtered results. This ensures the indices are contiguous and reflect the
actual position in the final words array.
---
Nitpick comments:
In `@src/lib/exporter/annotationRenderer.ts`:
- Around line 11-15: The AnnotationSceneTransform interface is currently not
exported but is used in the signature of the exported renderAnnotations
function, which prevents callers from directly referencing the type. Add the
export keyword to the AnnotationSceneTransform interface definition to allow
callers to import and use the type directly for better type safety and clearer
API documentation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 788a8c0b-788a-477c-ab5b-ce59a456ee45
📒 Files selected for processing (8)
src/components/video-editor/AnnotationOverlay.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/captionEditing.test.tssrc/components/video-editor/captionEditing.tssrc/components/video-editor/captionLayout.tssrc/lib/exporter/annotationRenderer.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/modernFrameRenderer.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/video-editor/AnnotationOverlay.tsx
Comment on lines
+1589
to
+1593
| { | ||
| scale: this.animationState.appliedScale, | ||
| x: this.animationState.x, | ||
| y: this.animationState.y, | ||
| }, |
Contributor
There was a problem hiding this comment.
Use the temporal snapshot transform when compositing blur annotations.
composeBlurAnnotationFrame reads mutable this.animationState, but the temporal path calls it with resolvedSnapshot.timeMs after sampling has advanced state to the last sample. If temporal zoom blur is enabled, annotations can be rendered at a different transform than the selected snapshot; pass resolvedSnapshot.sceneTransform through instead.
Proposed fix
private async composeBlurAnnotationFrame(
timeMs: number,
sourceCanvas?: CanvasImageSource,
+ sceneTransform: { scale: number; x: number; y: number } = {
+ scale: this.animationState.appliedScale,
+ x: this.animationState.x,
+ y: this.animationState.y,
+ },
): Promise<void> {
@@
await renderAnnotations(
context,
this.config.annotationRegions ?? [],
this.config.width,
this.config.height,
timeMs,
this.annotationScaleFactor,
this.annotationAssets ?? undefined,
- {
- scale: this.animationState.appliedScale,
- x: this.animationState.x,
- y: this.animationState.y,
- },
+ sceneTransform,
);- await this.composeBlurAnnotationFrame(resolvedSnapshot.timeMs, compositeState.canvas);
+ await this.composeBlurAnnotationFrame(
+ resolvedSnapshot.timeMs,
+ compositeState.canvas,
+ resolvedSnapshot.sceneTransform,
+ );🤖 Prompt for 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.
In `@src/lib/exporter/modernFrameRenderer.ts` around lines 1589 - 1593, The
composeBlurAnnotationFrame call is using mutable this.animationState for the
transform properties (scale, x, y) instead of the temporal snapshot transform.
Replace the object passed to composeBlurAnnotationFrame that reads from
this.animationState with resolvedSnapshot.sceneTransform to ensure blur
annotations are rendered at the correct transform that matches the selected
snapshot, preventing misalignment when temporal zoom blur is enabled.
Contributor
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/lib/exporter/modernFrameRenderer.ts`:
- Around line 1617-1627: The `setupAnnotationLayer()` method is being called in
the `initialize()` method before `layoutCache` is populated by `updateLayout()`,
which causes the annotation positioning logic to always use the fallback
full-canvas dimensions instead of the actual masked rectangle. To fix this,
either reorder the calls in `initialize()` so that `updateLayout()` is invoked
before `setupAnnotationLayer()`, or alternatively defer the annotation sprite
positioning calculations from `setupAnnotationLayer()` to occur later during the
first `renderFrame()` call after `layoutCache` has been properly initialized,
similar to how blur annotations correctly use the actual `maskRect` at render
time.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7680a615-b778-429f-ba0a-1b77eed14589
📒 Files selected for processing (6)
src/components/video-editor/AnnotationOverlay.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/lib/exporter/annotationRenderer.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/modernFrameRenderer.ts
💤 Files with no reviewable changes (1)
- src/components/video-editor/VideoEditor.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/exporter/frameRenderer.ts
- src/lib/exporter/annotationRenderer.ts
- src/components/video-editor/VideoPlayback.tsx
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
.recordlyas a macOS document type using the Recordly app iconValidation
Summary by CodeRabbit