Fix editor project save UX and clip cleanup#304
Merged
webadderall merged 6 commits intomainfrom Apr 21, 2026
Merged
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:
📝 WalkthroughWalkthroughAdds a named-save flow: new renderer API + preload bridge, new IPC handler with name normalization and overwrite protection, propagation of Changes
Sequence DiagramsequenceDiagram
actor User
participant VideoEditor
participant Preload as Electron Preload
participant IPCMain as IPC Main
participant FS as File System
participant AppState as App State
User->>VideoEditor: Submit rename (name, projectData, thumbnail?)
VideoEditor->>Preload: saveProjectFileNamed(projectData, name, thumbnail?)
Preload->>IPCMain: "save-project-file-named" (projectData, name, thumbnail?)
IPCMain->>IPCMain: normalizeProjectSaveName(name)
alt invalid name
IPCMain-->>Preload: {success: false, message, canceled?}
else valid name
IPCMain->>IPCMain: ensureProjectDataHasProjectId(projectData)
IPCMain->>IPCMain: ensureNamedProjectSaveDoesNotOverwriteDifferentProject(targetPath, projectData, activePath?)
alt overwrite conflict
IPCMain-->>Preload: {success: false, message}
else ok
IPCMain->>FS: write projects/{normalized}.{ext} (includes projectId)
FS-->>IPCMain: write result
IPCMain->>FS: save thumbnail (if provided)
IPCMain->>AppState: set currentProjectPath, remember project
IPCMain-->>Preload: {success: true, path, projectId}
end
end
Preload-->>VideoEditor: result
VideoEditor->>AppState: update lastSavedSnapshot(projectId), refresh library, remount preview
VideoEditor->>User: exit edit mode / show status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 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 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/ipc/register/project.ts`:
- Around line 219-225: The save currently calls fs.writeFile to write to
path.join(await getProjectsDir(),
`${normalizedProjectName}.${PROJECT_FILE_EXTENSION}`) and will silently
overwrite any existing project; change the logic to detect an existing file
before writing: check if the target path exists (fs.stat or fs.pathExists), and
if it does, read and parse the existing file to compare its project id (or other
immutable identifier) against projectData.id; if ids match allow the write,
otherwise abort and surface an error/result to the caller (or trigger a
confirmation flow) instead of calling fs.writeFile to avoid accidental
overwrites. Ensure this check is performed in the same function that uses
normalizedProjectName and PROJECT_FILE_EXTENSION so the write only proceeds
after the existence/id check.
- Around line 58-62: The regex in the sanitizedName construction includes an
embedded control-character range (\u0000-\u001F) which trips the lint rule;
remove that range from the regex and instead filter out low-control characters
programmatically: first operate on withoutExtension to strip characters using
the existing regex without the control-range (the replace(/[<>:"/\\|?*]/g, "")
part), then run a simple character-code filter on the resulting string (e.g.,
keep only chars with charCodeAt(0) > 31) before applying the remaining
replacements and trim; update the sanitizedName assignment (the expression that
builds sanitizedName from withoutExtension) to reflect these two steps.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2275-2278: The saving state isn't cleared if saveProjectWithName
rejects; wrap the call to saveProjectWithName(trimmedProjectName) inside a
try/finally (or try/catch/finally) so that setIsSavingProjectName(false) is
always executed (e.g., in finally) after setIsSavingProjectName(true) is set;
optionally rethrow or handle the error inside the catch so error behavior
remains unchanged. Ensure you update the block containing setIsSavingProjectName
and saveProjectWithName to use try/finally to guarantee the input is re-enabled
on failure.
- Around line 2922-2938: The current removal logic for deletedClip only drops
regions fully contained inside the clip; update each region cleanup
(setZoomRegions, setAnnotationRegions, setTrimRegions, setSpeedRegions,
setAudioRegions) to remove any region that overlaps the deleted clip by
filtering to keep only regions that do NOT overlap: use prev.filter(region =>
region.endMs <= startMs || region.startMs >= endMs) instead of the existing
containment predicate, referencing deletedClip.startMs and deletedClip.endMs in
the comparison.
🪄 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
Run ID: 7677379e-3a19-4f17-b3be-4f9f99347039
📒 Files selected for processing (6)
electron/electron-env.d.tselectron/ipc/register/project.tselectron/preload.tssrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/timeline/AudioWaveform.tsxsrc/components/video-editor/timeline/Row.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
electron/ipc/register/project.ts (1)
273-315: Named save bypassesshowOverwriteConfirmation— consider a user-facing confirm for same-video overwrites.Unlike
save-project-filewhich uses Electron'sshowSaveDialogwithshowOverwriteConfirmation(Line 242), this path writes directly once the videoPath check passes. Combined with the identity weakness noted above, a rename can overwrite a previous save of the same video without any user confirmation. At minimum, return an explicit result code the renderer can use to prompt the user before overwriting, rather than implicitly trusting the videoPath match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/register/project.ts` around lines 273 - 315, The named-save handler (ipcMain.handle 'save-project-file-named') currently writes directly after ensureNamedProjectSaveDoesNotOverwriteDifferentProject passes; change it to return an explicit overwrite-warning result when the target path already corresponds to an existing project for the same video so the renderer can prompt the user instead of silently overwriting. Specifically, update ensureNamedProjectSaveDoesNotOverwriteDifferentProject (or add a new check) to detect an existing file that would be overwritten and return { success: false, code: 'overwrite-potential', path: targetProjectPath } (or similar) instead of allowing the write, and then have the handler return that result to the renderer rather than performing fs.writeFile; only proceed to writeFile, setCurrentProjectPath, saveProjectThumbnail, and rememberRecentProject after the renderer confirms the overwrite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/ipc/register/project.ts`:
- Around line 85-129: The check in
ensureNamedProjectSaveDoesNotOverwriteDifferentProject wrongly treats videoPath
as a unique identity and conflates read/parse errors with a definite name
collision; update this function to first prefer a stable project id field (e.g.,
projectId) read from projectData and from the existing file (use a helper like
getProjectId), and only fall back to comparing videoPath if both projectIds are
absent and the caller accepts a weaker check; change the fs.readFile/JSON.parse
try/catch to distinguish verification failures (I/O or parse errors) — return {
success: false, message: "Unable to verify project identity for the chosen name"
} for unreadable/malformed files — and only return "A different project already
uses this name" when a definitive mismatch is observed (existingProjectId !==
incomingProjectId or both videoPaths present and different); update references
to getProjectVideoPath, fs.readFile, and JSON.parse accordingly.
---
Nitpick comments:
In `@electron/ipc/register/project.ts`:
- Around line 273-315: The named-save handler (ipcMain.handle
'save-project-file-named') currently writes directly after
ensureNamedProjectSaveDoesNotOverwriteDifferentProject passes; change it to
return an explicit overwrite-warning result when the target path already
corresponds to an existing project for the same video so the renderer can prompt
the user instead of silently overwriting. Specifically, update
ensureNamedProjectSaveDoesNotOverwriteDifferentProject (or add a new check) to
detect an existing file that would be overwritten and return { success: false,
code: 'overwrite-potential', path: targetProjectPath } (or similar) instead of
allowing the write, and then have the handler return that result to the renderer
rather than performing fs.writeFile; only proceed to writeFile,
setCurrentProjectPath, saveProjectThumbnail, and rememberRecentProject after the
renderer confirms the overwrite.
🪄 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
Run ID: 595912cf-8e27-4aaf-8ef2-d4c4d7e35f44
📒 Files selected for processing (2)
electron/ipc/register/project.tssrc/components/video-editor/VideoEditor.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/video-editor/VideoEditor.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/ipc/register/project.ts`:
- Around line 345-374: The handler save-project-file-named writes a new
<name>.recordly and updates setCurrentProjectPath but never removes or migrates
the previous file (currentProjectPath), so implement cleanup after a successful
save: if currentProjectPath exists and is different from targetProjectPath,
delete the old file (fs.unlink) and also migrate/remove associated assets—call
saveProjectThumbnail for the new path (already present) and, if helpers exist,
call removeProjectThumbnail(currentProjectPath) and forget/remove the old recent
entry (e.g., rememberRecentProject/forgetRecentProject or similar); finally
setCurrentProjectPath(targetProjectPath) and ensure
rememberRecentProject(targetProjectPath) is called so the old file is removed
from disk and recent lists when renaming.
🪄 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
Run ID: b60407bf-35a3-4424-b099-4e975db06562
📒 Files selected for processing (4)
electron/electron-env.d.tselectron/ipc/register/project.tssrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/projectPersistence.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/electron-env.d.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/video-editor/VideoEditor.tsx (1)
2318-2335:⚠️ Potential issue | 🟠 MajorMissing catch → IPC rejection becomes an unhandled promise rejection with no user feedback.
saveProjectWithNameawaitswindow.electronAPI.saveProjectFileNamed(...)without its own try/catch, and the submit handler here wraps the call intry/finallywithout acatch. If the IPC rejects (e.g., permission error, main-process throw), the promise bubbles up as an unhandled rejection — the user sees no toast, the editor exitsisSavingProjectNamebut also skips thesetIsEditingProjectName(false)/ refocus paths, leaving the input stuck in edit mode with stale text and no indication of what went wrong. The previous review suggestedtry/catch/finallywith a toast; only thefinallyportion landed.🛠️ Proposed fix
setIsSavingProjectName(true); let saved = false; try { saved = await saveProjectWithName(trimmedProjectName); + } catch (error) { + toast.error(getErrorMessage(error)); } finally { setIsSavingProjectName(false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoEditor.tsx` around lines 2318 - 2335, The submit handler for saving a project name uses try/finally but no catch, so IPC rejections from saveProjectWithName cause unhandled promise rejections and no user feedback; update the handler (the async function that calls saveProjectWithName inside the useCallback) to use try/catch/finally: in catch call the app's toast/notification API with a helpful error message (include the caught error), keep setIsSavingProjectName(false) in finally, and after a failed save ensure the input remains focused and selected via projectNameInputRef.current?.focus()/select() and do NOT call setIsEditingProjectName(false); preserve the existing success path (when saved is true call setIsEditingProjectName(false)). Include references to saveProjectWithName, setIsSavingProjectName, setIsEditingProjectName, and projectNameInputRef so the change is easy to locate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2923-2938: The explicit calls to setTrimRegions inside the
remove-trimmed-segments block and in handleClipDelete are redundant because
trimRegions are derived from clipRegions via the clipsToTrims effect (which runs
when clipRegions or duration change); remove the setTrimRegions((prev) =>
removeTrimmedRegions(prev)) invocations (references: setTrimRegions,
removeTrimmedRegions closure, handleClipDelete, handleClipSpanChange,
clipsToTrims, clipRegions, duration) so trims remain single-sourced and the code
no longer attempts to update trims directly.
---
Duplicate comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2318-2335: The submit handler for saving a project name uses
try/finally but no catch, so IPC rejections from saveProjectWithName cause
unhandled promise rejections and no user feedback; update the handler (the async
function that calls saveProjectWithName inside the useCallback) to use
try/catch/finally: in catch call the app's toast/notification API with a helpful
error message (include the caught error), keep setIsSavingProjectName(false) in
finally, and after a failed save ensure the input remains focused and selected
via projectNameInputRef.current?.focus()/select() and do NOT call
setIsEditingProjectName(false); preserve the existing success path (when saved
is true call setIsEditingProjectName(false)). Include references to
saveProjectWithName, setIsSavingProjectName, setIsEditingProjectName, and
projectNameInputRef so the change is easy to locate.
🪄 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
Run ID: 7df95416-202d-408a-ac9c-f761f1418e77
📒 Files selected for processing (1)
src/components/video-editor/VideoEditor.tsx
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/VideoEditor.tsx (1)
2883-2946:⚠️ Potential issue | 🟠 MajorClip move operations silently delete non-zoom regions from the vacated area.
removedSegmentsis computed unconditionally from(newStart > oldClip.startMs)/(newEnd < oldClip.endMs). For a pure move (e.g. drag right:startDelta === endDelta > 0),newStart > oldClip.startMsis true, so[oldClip.startMs, newStart]gets added toremovedSegments.The
if (isMove)branch only shiftszoomRegionsbydelta, so zooms inside the clip follow it to the new position. ButannotationRegions,speedRegions, andaudioRegionsare not shifted — they then fall through to theremovedSegments.length > 0cleanup and get deleted if they overlap the vacated old-position range.Net effect: dragging a clip sideways preserves zooms but quietly wipes annotations/speed/audio that were authored against the clip's old timeline location. The AI summary describes this logic as kicking in only "when clip time boundaries shrink", which matches trims but not moves.
🛠️ Proposed fix — skip the removed-segments cleanup on moves
const handleClipSpanChange = useCallback( (id: string, span: Span) => { const oldClip = clipRegions.find((c) => c.id === id); const newStart = Math.round(span.start); const newEnd = Math.round(span.end); - const removedSegments = oldClip - ? [ - ...(newStart > oldClip.startMs - ? [{ startMs: oldClip.startMs, endMs: newStart }] - : []), - ...(newEnd < oldClip.endMs - ? [{ startMs: newEnd, endMs: oldClip.endMs }] - : []), - ] - : []; + let removedSegments: { startMs: number; endMs: number }[] = []; if (oldClip) { const startDelta = newStart - oldClip.startMs; const endDelta = newEnd - oldClip.endMs; const isMove = Math.abs(startDelta - endDelta) < 1 && Math.abs(startDelta) > 0; if (isMove) { const delta = startDelta; setZoomRegions((prev) => prev.map((zoom) => { const overlaps = zoom.startMs < oldClip.endMs && zoom.endMs > oldClip.startMs; if (overlaps) { return { ...zoom, startMs: zoom.startMs + delta, endMs: zoom.endMs + delta, }; } return zoom; }), ); + } else { + removedSegments = [ + ...(newStart > oldClip.startMs + ? [{ startMs: oldClip.startMs, endMs: newStart }] + : []), + ...(newEnd < oldClip.endMs + ? [{ startMs: newEnd, endMs: oldClip.endMs }] + : []), + ]; } }If the intent is actually to also carry non-zoom regions along on moves, do that in the
if (isMove)branch instead (apply+deltatoannotationRegions,speedRegions,audioRegions) — but don't let them silently disappear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoEditor.tsx` around lines 2883 - 2946, The bug is in handleClipSpanChange: removedSegments is computed for both trims and pure moves, then used to delete regions (annotationRegions, speedRegions, audioRegions) even on moves; update the logic so that when isMove is true you do NOT run the removedSegments cleanup — instead either shift annotationRegions/speedRegions/audioRegions by the same delta inside the if (isMove) block (use setAnnotationRegions, setSpeedRegions, setAudioRegions to map and add delta to startMs/endMs) or gate the removedSegments removal behind a !isMove check so only trims trigger region deletion; keep the existing zoom shift behavior for setZoomRegions as-is.
🧹 Nitpick comments (1)
src/components/video-editor/VideoEditor.tsx (1)
4520-4576: Inline rename form — focus/disable interplay is sound, small a11y consideration.The form correctly:
- Prevents default on submit and delegates to
handleProjectNameSubmit- Handles Escape to cancel via
closeProjectNameEditor- Guards the blur-to-close path with
!isSavingProjectNameso the async save isn't aborted when React disables the input mid-flight- Mirrors the unsaved-changes indicator in both display and edit states
One optional improvement: when Enter triggers a server-side rejection (duplicate name, unable to verify),
handleProjectNameSubmitrefocuses and reselects the input, but the user gets no inline hint of why the save failed beyond the toast. Consider surfacingresult.messageinline next to the input as well, so screen-reader users and keyboard users don't have to associate a transient toast with the field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoEditor.tsx` around lines 4520 - 4576, The edit flow lacks an inline, accessible error display when handleProjectNameSubmit fails; update the edit branch (the form around projectNameInputRef / projectNameDraft / isSavingProjectName) to store the submit error message from handleProjectNameSubmit (e.g., result.message) in state and render a short inline error node next to the input (with a unique id), mark the input aria-invalid="true" and add aria-describedby pointing to that id when an error exists, and ensure the error node is announced (use role="alert" or aria-live="assertive") so keyboard and screen-reader users see why the save failed while retaining the existing focus/reselect behavior and blur/save guards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2229-2335: saveProjectWithName and handleProjectNameSubmit
currently let exceptions (e.g. window.electronAPI.saveProjectFileNamed IPC
errors) bubble up with no user feedback; wrap the await call to
saveProjectWithName inside handleProjectNameSubmit in a try/catch (keeping the
existing finally that clears setIsSavingProjectName) and on catch call
toast.error(getErrorMessage(error)) so the user sees the failure; reference
handleProjectNameSubmit, saveProjectWithName,
window.electronAPI.saveProjectFileNamed and getErrorMessage when making the
change.
---
Outside diff comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2883-2946: The bug is in handleClipSpanChange: removedSegments is
computed for both trims and pure moves, then used to delete regions
(annotationRegions, speedRegions, audioRegions) even on moves; update the logic
so that when isMove is true you do NOT run the removedSegments cleanup — instead
either shift annotationRegions/speedRegions/audioRegions by the same delta
inside the if (isMove) block (use setAnnotationRegions, setSpeedRegions,
setAudioRegions to map and add delta to startMs/endMs) or gate the
removedSegments removal behind a !isMove check so only trims trigger region
deletion; keep the existing zoom shift behavior for setZoomRegions as-is.
---
Nitpick comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 4520-4576: The edit flow lacks an inline, accessible error display
when handleProjectNameSubmit fails; update the edit branch (the form around
projectNameInputRef / projectNameDraft / isSavingProjectName) to store the
submit error message from handleProjectNameSubmit (e.g., result.message) in
state and render a short inline error node next to the input (with a unique id),
mark the input aria-invalid="true" and add aria-describedby pointing to that id
when an error exists, and ensure the error node is announced (use role="alert"
or aria-live="assertive") so keyboard and screen-reader users see why the save
failed while retaining the existing focus/reselect behavior and blur/save
guards.
🪄 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
Run ID: 2269f034-2a9d-4931-89af-3e3d0287d973
📒 Files selected for processing (2)
electron/ipc/register/project.tssrc/components/video-editor/VideoEditor.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.
Description: Port the editor save UX and clip cleanup fixes onto main without bringing refactor-dependent playback changes.
Motivation: Keep this PR compatible with other open branches against main while still shipping the requested editor UX and cleanup behavior.
Type of Change: bug fix, UX improvement.
Related Issues: none linked.
Screenshots/Video: none included.
Testing Guide: open the editor, click the project title to rename and press Enter, verify the folder icon opens the project browser, delete a clip and confirm overlapping timeline regions are removed, verify waveform stays clipped to its row.
Checklist: manual review completed; local editor diagnostics clean.
Summary by CodeRabbit
New Features
UI Changes
Improvements