feat: export caption sidecars and merge editor follow-ups#686
feat: export caption sidecars and merge editor follow-ups#686webadderall wants to merge 10 commits into
Conversation
…xport-caption-sidecars # Conflicts: # electron/ipc/register/export.ts # src/components/video-editor/VideoEditor.tsx
…xport-caption-sidecars # Conflicts: # src/components/video-editor/VideoPlayback.tsx
📝 WalkthroughWalkthroughThis PR adds inline caption cue editing in the video preview, independent webcam width/height controls (replacing a single size slider), advanced vertical padding beyond 100%, modal-driven project Save/Copy/Rename flows with unsaved-changes gating, ChangesInline Caption Editing and Sidecar Export
Independent Webcam Dimensions and Advanced Vertical Padding
Modal Project Save / Copy / Rename Flow
Export Pipeline: Bitrates, Annotations, Zoom Transition Removal, Cursor
Sequence Diagram(s)sequenceDiagram
rect rgba(135, 206, 250, 0.5)
Note over VideoEditor,VideoPlayback: Caption Editing Flow
end
participant VideoEditor
participant VideoPlayback
participant captionEditing
participant electronIPC
VideoPlayback->>VideoEditor: onEditAutoCaption(target, draftText)
VideoEditor->>captionEditing: updateCaptionCuesForEditedTarget(target, text)
captionEditing-->>VideoEditor: updated project cues
VideoEditor->>VideoEditor: showSuccessToast()
rect rgba(255, 200, 100, 0.5)
Note over VideoEditor,electronIPC: Project Save / Copy / Rename Flow
end
participant User
User->>VideoEditor: saveProject(saveAs=true)
VideoEditor->>VideoEditor: openProjectSaveDialog()
User->>VideoEditor: handleProjectSaveDialogSubmit(name, mode)
VideoEditor->>electronIPC: saveProjectFileNamed(name, data, mode)
alt mode="copy" and different path
electronIPC->>electronIPC: randomUUID() → new projectId
else mode="rename"
electronIPC->>electronIPC: ensureProjectDataHasProjectId()
electronIPC->>electronIPC: cleanup old project file + thumbnail
end
electronIPC-->>VideoEditor: {path, projectId}
VideoEditor->>VideoEditor: resolveProjectSaveDialog(result)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
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/VideoEditor.tsx (1)
628-653:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate sidecar export on cue data, not burn-in caption visibility.
Line 628 requires
autoCaptionSettings.enabled, but generating captions only updatesautoCaptionsat Lines 2852-2853. In the default generated-caption flow, valid cues can exist while the sidecar option stays hidden and no payload is sent. Use the validated sidecar cue list as the eligibility check instead.🐛 Proposed fix
- const hasCaptionsForSidecar = autoCaptionSettings.enabled && autoCaptions.length > 0; const captionSidecarCues = useMemo( () => autoCaptions @@ })), [autoCaptions], ); + const hasCaptionsForSidecar = captionSidecarCues.length > 0; const captionSidecarPayload = - hasCaptionsForSidecar && captionSidecarCues.length > 0 && includeCaptionSidecar + hasCaptionsForSidecar && includeCaptionSidecar ? { format: "both" as const, cues: captionSidecarCues,Also applies to: 2852-2853, 6081-6083
🤖 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 628 - 653, The hasCaptionsForSidecar variable on line 628 gates the sidecar export based on autoCaptionSettings.enabled, but this doesn't guarantee valid captions exist. Instead of checking autoCaptionSettings.enabled and autoCaptions.length, use the validated captionSidecarCues array as the eligibility check. Replace the hasCaptionsForSidecar definition to check if captionSidecarCues has items, so the sidecar payload is only sent when actual valid cue data is present, not just when the caption setting is enabled.
🧹 Nitpick comments (2)
src/components/video-editor/projectPersistence.test.ts (1)
6-46: ⚡ Quick winAdd coverage for webcam width/height normalization branches.
This suite only asserts padding behavior, but
normalizeProjectEditorin this PR also changedwebcam.width/webcam.heightfallback/clamp logic. Add focused assertions for explicit width/height, legacysizefallback, and clamping.Proposed test additions
describe("normalizeProjectEditor", () => { + it("normalizes webcam width/height from explicit values and legacy size fallback", () => { + const explicit = normalizeProjectEditor({ + webcam: { width: 88, height: 36, size: 50 } as never, + }); + expect(explicit.webcam.width).toBe(88); + expect(explicit.webcam.height).toBe(36); + + const legacy = normalizeProjectEditor({ + webcam: { size: 55 } as never, + }); + expect(legacy.webcam.width).toBe(55); + expect(legacy.webcam.height).toBe(55); + }); + + it("clamps webcam width/height to allowed range", () => { + const editor = normalizeProjectEditor({ + webcam: { width: 2, height: 1000 } as never, + }); + expect(editor.webcam.width).toBe(10); + expect(editor.webcam.height).toBe(100); + }); + it("preserves the extended advanced vertical padding range", () => {🤖 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/projectPersistence.test.ts` around lines 6 - 46, The test suite for normalizeProjectEditor currently only covers padding normalization behavior and is missing test coverage for the webcam width/height normalization logic that was also changed. Add new test cases to the describe block that verify the webcam width/height handling in normalizeProjectEditor, including tests for: explicit width/height values being preserved, legacy size property fallback when width/height are not provided, and proper clamping of webcam dimensions to their maximum values. These tests should follow the same pattern as the existing padding tests using expect and toMatchObject assertions.electron/ipc/register/project.ts (1)
301-305: 💤 Low valueConsider resolving symlinks for defense-in-depth.
The
isPathInsideDirectorycheck uses path normalization but doesn't resolve symlinks. A symlink insideprojectsDirpointing outside would pass this check, andfs.writeFilewould follow the symlink to write outside the trusted directory.In a desktop app where users have full filesystem control, this is low-risk (users would be "attacking themselves"). However, for defense-in-depth, consider using
fs.realpathbefore the directory containment check:🛡️ Optional hardening
+ const resolvedExistingPath = existingProjectPath + ? await fs.realpath(existingProjectPath).catch(() => path.resolve(existingProjectPath)) + : null; const trustedExistingProjectPath = existingProjectPath && path.extname(existingProjectPath).toLowerCase() === `.${PROJECT_FILE_EXTENSION}` && - (isTrustedProjectPath(existingProjectPath) || isPathInsideDirectory(existingProjectPath, projectsDir)) - ? path.resolve(existingProjectPath) + (isTrustedProjectPath(existingProjectPath) || isPathInsideDirectory(resolvedExistingPath!, projectsDir)) + ? resolvedExistingPath : null🤖 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 `@electron/ipc/register/project.ts` around lines 301 - 305, The trustedExistingProjectPath assignment does not resolve symlinks before validating containment with isPathInsideDirectory, which could allow a symlink inside projectsDir to escape the trusted boundary. To fix this, resolve the existingProjectPath using fs.realpathSync before passing it to the isPathInsideDirectory check, ensuring the actual target location of any symlinks is validated as being within projectsDir rather than the symlink itself.
🤖 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/SettingsPanel.tsx`:
- Around line 3925-3927: In the getCropMatchedWebcamHeightPercent function call,
the second argument is incorrectly passing webcamWidth when it should pass
webcamHeight. This causes the height to be recalculated based on width every
time, overwriting user manual height adjustments. Replace the second webcamWidth
argument with webcamHeight to preserve the user's previously set height value
when applying crop updates.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 2390-2404: The AnnotationOverlay component is receiving the
interactionScale prop but not using it, which prevents the Rnd component from
accounting for zoom scale during drag and resize operations. Add
interactionScale as a property to the AnnotationOverlayProps interface, then
destructure interactionScale in the AnnotationOverlay component signature, and
finally pass this interactionScale value to the Rnd component's scale prop to
ensure drag and resize deltas are calculated correctly under zoom conditions.
In `@src/lib/exporter/modernFrameRenderer.ts`:
- Around line 2569-2571: The getCropMatchedWebcamHeightPercent method is
receiving already-cropped dimensions from renderableWebcamSource after
refreshWebcamFrameCache has created a crop-trimmed cache, which double-applies
crop aspect compensation and skews webcam overlay proportions. Instead of
passing renderableWebcamSource.width and renderableWebcamSource.height to
getCropMatchedWebcamHeightPercent, pass the original uncropped source dimensions
from liveSource (liveSourceWidth and liveSourceHeight) to ensure crop-matched
height is calculated correctly without double compensation.
---
Outside diff comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 628-653: The hasCaptionsForSidecar variable on line 628 gates the
sidecar export based on autoCaptionSettings.enabled, but this doesn't guarantee
valid captions exist. Instead of checking autoCaptionSettings.enabled and
autoCaptions.length, use the validated captionSidecarCues array as the
eligibility check. Replace the hasCaptionsForSidecar definition to check if
captionSidecarCues has items, so the sidecar payload is only sent when actual
valid cue data is present, not just when the caption setting is enabled.
---
Nitpick comments:
In `@electron/ipc/register/project.ts`:
- Around line 301-305: The trustedExistingProjectPath assignment does not
resolve symlinks before validating containment with isPathInsideDirectory, which
could allow a symlink inside projectsDir to escape the trusted boundary. To fix
this, resolve the existingProjectPath using fs.realpathSync before passing it to
the isPathInsideDirectory check, ensuring the actual target location of any
symlinks is validated as being within projectsDir rather than the symlink
itself.
In `@src/components/video-editor/projectPersistence.test.ts`:
- Around line 6-46: The test suite for normalizeProjectEditor currently only
covers padding normalization behavior and is missing test coverage for the
webcam width/height normalization logic that was also changed. Add new test
cases to the describe block that verify the webcam width/height handling in
normalizeProjectEditor, including tests for: explicit width/height values being
preserved, legacy size property fallback when width/height are not provided, and
proper clamping of webcam dimensions to their maximum values. These tests should
follow the same pattern as the existing padding tests using expect and
toMatchObject assertions.
🪄 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: bbab6d85-a7c5-48cb-94dd-db98892ba0ba
📒 Files selected for processing (34)
electron/ipc/register/export.tselectron/ipc/register/project.tssrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/WebcamCropControl.tsxsrc/components/video-editor/captionEditing.test.tssrc/components/video-editor/captionEditing.tssrc/components/video-editor/captionLayout.tssrc/components/video-editor/projectPersistence.test.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/types.tssrc/components/video-editor/videoPlayback/cursorRenderer.tssrc/components/video-editor/videoPlayback/layoutUtils.test.tssrc/components/video-editor/videoPlayback/layoutUtils.tssrc/components/video-editor/webcamOverlay.test.tssrc/components/video-editor/webcamOverlay.tssrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/it/settings.jsonsrc/i18n/locales/ko/settings.jsonsrc/i18n/locales/nl/settings.jsonsrc/i18n/locales/pt-BR/settings.jsonsrc/i18n/locales/ru/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/i18n/locales/zh-TW/settings.jsonsrc/lib/exporter/exportBitrate.test.tssrc/lib/exporter/exportBitrate.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/modernFrameRenderer.test.tssrc/lib/exporter/modernFrameRenderer.tssrc/lib/exporter/modernVideoExporter.nativeStaticLayout.test.tssrc/lib/exporter/modernVideoExporter.ts
| ? getCropMatchedWebcamHeightPercent( | ||
| webcamWidth, | ||
| webcamWidth, |
There was a problem hiding this comment.
Preserve user-set webcam height when applying crop updates.
Line 3927 passes webcamWidth as both width and height, which forces auto-height recomputation on every crop edit and overwrites manual height choices. Pass the current webcamHeight as the second argument.
Proposed fix
height: previewFrame
? getCropMatchedWebcamHeightPercent(
webcamWidth,
- webcamWidth,
+ webcamHeight,
previewFrame.width,
previewFrame.height,
cropRegion,📝 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.
| ? getCropMatchedWebcamHeightPercent( | |
| webcamWidth, | |
| webcamWidth, | |
| height: previewFrame | |
| ? getCropMatchedWebcamHeightPercent( | |
| webcamWidth, | |
| webcamHeight, | |
| previewFrame.width, | |
| previewFrame.height, | |
| cropRegion, |
🤖 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/SettingsPanel.tsx` around lines 3925 - 3927, In
the getCropMatchedWebcamHeightPercent function call, the second argument is
incorrectly passing webcamWidth when it should pass webcamHeight. This causes
the height to be recalculated based on width every time, overwriting user manual
height adjustments. Replace the second webcamWidth argument with webcamHeight to
preserve the user's previously set height value when applying crop updates.
| setAnnotationSceneTransform((current) => { | ||
| if ( | ||
| Math.abs(current.scale - appliedTransform.scale) < 0.001 && | ||
| Math.abs(current.x - appliedTransform.x) < 0.1 && | ||
| Math.abs(current.y - appliedTransform.y) < 0.1 | ||
| ) { | ||
| return current; | ||
| } | ||
|
|
||
| return { | ||
| scale: appliedTransform.scale, | ||
| x: appliedTransform.x, | ||
| y: appliedTransform.y, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify AnnotationOverlay consumes interactionScale and forwards it to the Rnd scale prop.
annotation_file="$(fd -i '^AnnotationOverlay\.tsx$' . | head -n 1)"
test -n "$annotation_file" || { echo "AnnotationOverlay.tsx not found"; exit 1; }
# Expect: interactionScale is declared in props/destructuring and <Rnd ... scale={interactionScale}> is present.
rg -n -C 4 'interactionScale|sceneTransform|recordingRect|<Rnd|scale=' "$annotation_file"Repository: webadderallorg/Recordly
Length of output: 238
🏁 Script executed:
annotation_file="$(fd -i '^AnnotationOverlay\.tsx$' . | head -n 1)"
test -n "$annotation_file" || { echo "AnnotationOverlay.tsx not found"; exit 1; }
# Get file stats and show the component signature and full Rnd element
echo "=== File: $annotation_file ==="
wc -l "$annotation_file"
echo ""
echo "=== Component signature (first 100 lines) ==="
head -n 100 "$annotation_file"Repository: webadderallorg/Recordly
Length of output: 3305
🏁 Script executed:
annotation_file="$(fd -i '^AnnotationOverlay\.tsx$' . | head -n 1)"
# Search for component export and interface/type definitions
echo "=== Component export and props type ==="
rg -n 'export|interface|type.*Props|function AnnotationOverlay' "$annotation_file" | head -n 20Repository: webadderallorg/Recordly
Length of output: 183
🏁 Script executed:
annotation_file="$(fd -i '^AnnotationOverlay\.tsx$' . | head -n 1)"
# Get the full Rnd element to see all its props
echo "=== Full Rnd element with props ==="
rg -n '<Rnd' -A 50 "$annotation_file" | head -n 80Repository: webadderallorg/Recordly
Length of output: 2191
🏁 Script executed:
video_file="$(fd -i '^VideoPlayback\.tsx$' . | head -n 1)"
# Search for where AnnotationOverlay is used/imported and see if interactionScale is passed
rg -n 'AnnotationOverlay|interactionScale' "$video_file" | head -n 50Repository: webadderallorg/Recordly
Length of output: 228
🏁 Script executed:
video_file="$(fd -i '^VideoPlayback\.tsx$' . | head -n 1)"
# Get context around where AnnotationOverlay component is rendered
rg -n -B 5 -A 15 '<AnnotationOverlay' "$video_file"Repository: webadderallorg/Recordly
Length of output: 806
🏁 Script executed:
video_file="$(fd -i '^VideoPlayback\.tsx$' . | head -n 1)"
# Get full AnnotationOverlay component with all props
rg -n -A 30 '<AnnotationOverlay' "$video_file" | head -n 40Repository: webadderallorg/Recordly
Length of output: 1353
🏁 Script executed:
# Also check the lines around 2390-2404 in VideoPlayback to see annotationSceneTransform usage
video_file="$(fd -i '^VideoPlayback\.tsx$' . | head -n 1)"
sed -n '2385,2410p' "$video_file"Repository: webadderallorg/Recordly
Length of output: 680
🏁 Script executed:
# Check full AnnotationOverlayProps interface definition
annotation_file="$(fd -i '^AnnotationOverlay\.tsx$' . | head -n 1)"
sed -n '7,20p' "$annotation_file"Repository: webadderallorg/Recordly
Length of output: 537
🏁 Script executed:
# Check if sceneTransform and recordingRect are in the props but I missed them
annotation_file="$(fd -i '^AnnotationOverlay\.tsx$' . | head -n 1)"
rg -n 'sceneTransform|recordingRect|interactionScale' "$annotation_file"Repository: webadderallorg/Recordly
Length of output: 49
🏁 Script executed:
video_file="$(fd -i '^VideoPlayback\.tsx$' . | head -n 1)"
# Search for where annotationSceneTransform is declared/initialized
rg -n 'annotationSceneTransform|setAnnotationSceneTransform' "$video_file" | head -n 20Repository: webadderallorg/Recordly
Length of output: 427
🏁 Script executed:
# Check the <Rnd> props in Rnd documentation or react-rnd to understand what scale prop does
# Also check if there are other components using scale prop with Rnd
rg -n 'scale=' | grep -i rnd | head -n 10Repository: webadderallorg/Recordly
Length of output: 49
🏁 Script executed:
# Check the exact signature/destructuring of AnnotationOverlay function to see if interactionScale is there
annotation_file="$(fd -i '^AnnotationOverlay\.tsx$' . | head -n 1)"
sed -n '19,35p' "$annotation_file"Repository: webadderallorg/Recordly
Length of output: 551
🏁 Script executed:
# Double check the complete props interface and what's actually being passed from VideoPlayback
annotation_file="$(fd -i '^AnnotationOverlay\.tsx$' . | head -n 1)"
cat "$annotation_file" | head -n 50Repository: webadderallorg/Recordly
Length of output: 1667
🏁 Script executed:
# Verify lines 3322-3399 in VideoPlayback to understand "Also applies to"
video_file="$(fd -i '^VideoPlayback\.tsx$' . | head -n 1)"
sed -n '3320,3330p' "$video_file"Repository: webadderallorg/Recordly
Length of output: 425
Add interactionScale prop to AnnotationOverlay and pass it to Rnd's scale prop.
The overlay is CSS-transformed with annotationSceneTransform, but interactionScale is being passed to AnnotationOverlay (line 3398 in VideoPlayback) without being consumed. The AnnotationOverlayProps interface and component destructuring don't include it, and the <Rnd> component doesn't receive it as a scale prop. Without this, drag/resize deltas will be calculated in screen pixels rather than accounting for the zoom scale, causing persisted coordinates to be incorrect under zoom.
🤖 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/VideoPlayback.tsx` around lines 2390 - 2404, The
AnnotationOverlay component is receiving the interactionScale prop but not using
it, which prevents the Rnd component from accounting for zoom scale during drag
and resize operations. Add interactionScale as a property to the
AnnotationOverlayProps interface, then destructure interactionScale in the
AnnotationOverlay component signature, and finally pass this interactionScale
value to the Rnd component's scale prop to ensure drag and resize deltas are
calculated correctly under zoom conditions.
| if (needsCacheBackedSource) { | ||
| this.refreshWebcamFrameCache( | ||
| liveSource, | ||
| liveSourceWidth, | ||
| liveSourceHeight, | ||
| ); | ||
| this.refreshWebcamFrameCache(liveSource, liveSourceWidth, liveSourceHeight); | ||
| const cachedSource = this.getCachedWebcamRenderSource(); |
There was a problem hiding this comment.
Use uncropped source dimensions for crop-matched height calculation.
At Line 2923 and Line 2924, getCropMatchedWebcamHeightPercent receives renderableWebcamSource.width/height, but Line 2570 may already have produced a crop-trimmed cache. That double-applies crop aspect compensation and can skew webcam overlay proportions for cropped webcams.
💡 Suggested fix
const widthPercent = webcam.width ?? webcam.size ?? 50;
+ const aspectSourceWidth =
+ liveSourceDimensions.width > 0
+ ? liveSourceDimensions.width
+ : renderableWebcamSource.width;
+ const aspectSourceHeight =
+ liveSourceDimensions.height > 0
+ ? liveSourceDimensions.height
+ : renderableWebcamSource.height;
const heightPercent = getCropMatchedWebcamHeightPercent(
widthPercent,
webcam.height ?? webcam.size ?? 50,
- renderableWebcamSource.width,
- renderableWebcamSource.height,
+ aspectSourceWidth,
+ aspectSourceHeight,
webcam.cropRegion,
);Also applies to: 2920-2926
🤖 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 2569 - 2571, The
getCropMatchedWebcamHeightPercent method is receiving already-cropped dimensions
from renderableWebcamSource after refreshWebcamFrameCache has created a
crop-trimmed cache, which double-applies crop aspect compensation and skews
webcam overlay proportions. Instead of passing renderableWebcamSource.width and
renderableWebcamSource.height to getCropMatchedWebcamHeightPercent, pass the
original uncropped source dimensions from liveSource (liveSourceWidth and
liveSourceHeight) to ensure crop-matched height is calculated correctly without
double compensation.
Summary
Validation
Summary by CodeRabbit
Release Notes
New Features
Improvements
Localization