feat(editor): Cap recording import for timeline stitching#1719
feat(editor): Cap recording import for timeline stitching#1719MinitJain wants to merge 22 commits into
Conversation
Wrap trim handle state updates in batch() so project segment and previewTime update atomically before effects fire. Reorder effects in Editor.tsx so the config-update effect is created before the render-frame effect (SolidJS fires effects in creation order). Add skipRenderFrameForConfigUpdate flag so renderFrameEvent is suppressed when updateConfigAndRender already handles the render, eliminating the race condition where a stale-config frame was emitted before the async config update completed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…artifacts Spacedrive.framework is a pre-built CI artifact not present in local checkouts. Removing it allows the macOS bundle step to complete. Disabling createUpdaterArtifacts avoids the requirement for TAURI_SIGNING_PRIVATE_KEY which is only available in CI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tware#1712) Implements the recording import feature from issue CapSoftware#1712. Users can now import additional Cap recordings into the editor — imported recordings are appended to the timeline and stitched together on export. Changes: - ExternalRecordingReference struct in project config - ProjectRecordingsMeta::new_with_external() loads primary + external segments - create_all_segments() in editor builds unified segment list - import_cap_recording Tauri command: validates resolution match, prevents duplicate imports, computes correct clip index offsets, appends timeline segments - Export and preview pipelines updated to pass external_recordings through - Timeline UI: "Import recording" button with folder picker + reload on success⚠️ Not locally tested — macOS Sequoia TCC blocks ScreenCaptureKit on unsigned dev binaries, preventing the app from recording. Build passes (cargo build + pnpm typecheck clean). Needs testing on a signed build or by a reviewer with a valid dev certificate. Known limitations: - External recording paths stored as absolute strings (breaks if folder is moved) - window.location.reload() on import (loses unsaved editor state) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "bundle": { | ||
| "active": true, | ||
| "createUpdaterArtifacts": true, | ||
| "createUpdaterArtifacts": false, |
There was a problem hiding this comment.
createUpdaterArtifacts set to false
This change disables generation of auto-update artifacts (.sig / .tar.gz / .nsis.zip) at bundle time. If the Cap desktop app ships auto-update, this will silently break it for any release built from this commit — users on older versions won't receive the update. The previous value was true. Please verify this was intentional; if not, revert.
| "createUpdaterArtifacts": false, | |
| "createUpdaterArtifacts": true, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/tauri.conf.json
Line: 39
Comment:
**Likely unintentional: `createUpdaterArtifacts` set to `false`**
This change disables generation of auto-update artifacts (`.sig` / `.tar.gz` / `.nsis.zip`) at bundle time. If the Cap desktop app ships auto-update, this will silently break it for any release built from this commit — users on older versions won't receive the update. The previous value was `true`. Please verify this was intentional; if not, revert.
```suggestion
"createUpdaterArtifacts": true,
```
How can I resolve this? If you propose a fix, please make it concise.| let clip_index_offset = (primary_recordings.segments.len() | ||
| + project_config | ||
| .external_recordings | ||
| .iter() | ||
| .map(|r| { | ||
| let p = std::path::PathBuf::from(&r.path); | ||
| RecordingMeta::load_for_project(&p) | ||
| .ok() | ||
| .and_then(|m| { | ||
| m.studio_meta().map(|s| match s { | ||
| cap_project::StudioRecordingMeta::SingleSegment { .. } => 1usize, | ||
| cap_project::StudioRecordingMeta::MultipleSegments { inner } => { | ||
| inner.segments.len() | ||
| } | ||
| }) | ||
| }) | ||
| .unwrap_or(0) | ||
| }) | ||
| .sum::<usize>()) as u32; |
There was a problem hiding this comment.
unwrap_or(0) corrupts clip indices when a prior external recording is missing
clip_index_offset is computed by summing the segment count of every previously-imported external recording. If any of those paths is missing or has been moved, load_for_project fails and the closure returns 0 for that recording. As a result the new recording's recording_clip indices are computed too low, silently colliding with the indices of an existing external recording in ProjectRecordingsMeta::segments. The timeline would then map multiple TimelineSegments to the wrong clips.
Consider propagating the error instead of swallowing it:
let ext_segment_counts = project_config
.external_recordings
.iter()
.enumerate()
.map(|(i, r)| {
let p = std::path::PathBuf::from(&r.path);
let m = RecordingMeta::load_for_project(&p)
.map_err(|e| format!("existing external recording {i}: {e}"))?;
Ok(m.studio_meta().map(|s| match s {
cap_project::StudioRecordingMeta::SingleSegment { .. } => 1usize,
cap_project::StudioRecordingMeta::MultipleSegments { inner } => inner.segments.len(),
}).unwrap_or(0))
})
.collect::<Result<Vec<_>, String>>()?;
let clip_index_offset = (primary_recordings.segments.len()
+ ext_segment_counts.iter().sum::<usize>()) as u32;Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/lib.rs
Line: 2155-2173
Comment:
**Silent `unwrap_or(0)` corrupts clip indices when a prior external recording is missing**
`clip_index_offset` is computed by summing the segment count of every previously-imported external recording. If any of those paths is missing or has been moved, `load_for_project` fails and the closure returns `0` for that recording. As a result the new recording's `recording_clip` indices are computed too low, silently colliding with the indices of an existing external recording in `ProjectRecordingsMeta::segments`. The timeline would then map multiple `TimelineSegment`s to the wrong clips.
Consider propagating the error instead of swallowing it:
```rust
let ext_segment_counts = project_config
.external_recordings
.iter()
.enumerate()
.map(|(i, r)| {
let p = std::path::PathBuf::from(&r.path);
let m = RecordingMeta::load_for_project(&p)
.map_err(|e| format!("existing external recording {i}: {e}"))?;
Ok(m.studio_meta().map(|s| match s {
cap_project::StudioRecordingMeta::SingleSegment { .. } => 1usize,
cap_project::StudioRecordingMeta::MultipleSegments { inner } => inner.segments.len(),
}).unwrap_or(0))
})
.collect::<Result<Vec<_>, String>>()?;
let clip_index_offset = (primary_recordings.segments.len()
+ ext_segment_counts.iter().sum::<usize>()) as u32;
```
How can I resolve this? If you propose a fix, please make it concise.| </Show> | ||
| <button | ||
| class="flex items-center gap-1.5 px-3 py-2 mt-1 rounded-xl bg-blue-500 text-white text-xs font-medium disabled:opacity-50 self-start" | ||
| onClick={handleImportCapRecording} | ||
| disabled={isImporting()} | ||
| > | ||
| <Show | ||
| when={isImporting()} | ||
| fallback={<IconLucidePlus class="size-3" />} | ||
| > | ||
| <IconLucideLoader2 class="size-3 animate-spin" /> | ||
| </Show> |
There was a problem hiding this comment.
There are now two separate "Import recording" buttons that call the same handler: one added to the timeline toolbar (line ~878) and this one inside the track-rows scroll area. If this second one is intended as an empty-state CTA, it should be wrapped in a <Show> guard; otherwise having two identical call-to-action buttons in the same view is likely to confuse users.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/editor/Timeline/index.tsx
Line: 1045-1056
Comment:
**Duplicate "Import recording" button**
There are now two separate "Import recording" buttons that call the same handler: one added to the timeline toolbar (line ~878) and this one inside the track-rows scroll area. If this second one is intended as an empty-state CTA, it should be wrapped in a `<Show>` guard; otherwise having two identical call-to-action buttons in the same view is likely to confuse users.
How can I resolve this? If you propose a fix, please make it concise.| const handleImportCapRecording = async () => { | ||
| const selected = await openDialog({ | ||
| directory: true, | ||
| title: "Select a Cap Recording to Import", | ||
| filters: [{ name: "Cap Recording", extensions: ["cap"] }], | ||
| }); |
There was a problem hiding this comment.
filters has no effect for directory pickers on most platforms
openDialog is called with directory: true, which opens a folder-selection dialog. The filters: [{ name: "Cap Recording", extensions: ["cap"] }] option is designed for file pickers and is ignored by OS-level folder dialogs on macOS and Windows. The .endsWith(".cap") check below correctly guards against bad input, but the filter gives a false impression of UI-side enforcement.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/editor/Timeline/index.tsx
Line: 728-733
Comment:
**`filters` has no effect for directory pickers on most platforms**
`openDialog` is called with `directory: true`, which opens a folder-selection dialog. The `filters: [{ name: "Cap Recording", extensions: ["cap"] }]` option is designed for file pickers and is ignored by OS-level folder dialogs on macOS and Windows. The `.endsWith(".cap")` check below correctly guards against bad input, but the filter gives a false impression of UI-side enforcement.
How can I resolve this? If you propose a fix, please make it concise.Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Propagate error instead of unwrap_or(0) in clip_index_offset calculation to prevent silent index corruption when a prior external recording path is missing - Remove duplicate Import recording button from track-rows area - Remove no-op filters from directory picker openDialog call Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The |
|
Hey @richiemcilroy just wanted to clarify again. That said, The core logic — config serialization, segment stitching, and index offset calculation — has been reviewed manually. Happy to make any further changes if needed! |
superagent-security
Bot
added
contributor:verified
MinitJain
force-pushed
the
feat/recording-track
branch
from
6fdc529 to
b93db54
Compare
superagent-security
Bot
removed
pr:verified
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| > | ||
| <div class="flex flex-col gap-2 min-h-full"> | ||
| <TrackRow icon={trackIcons.clip} label="Video"> | ||
| <TrackRow icon={trackIcons.clip}> |
There was a problem hiding this comment.
TrackRow has onImport/importing support now, but nothing passes those props, so the overlay button never renders.
If the intent is for the video track to be the import CTA, wiring it up here makes it live (and you can drop the toolbar button if you want a single entry point):
| <TrackRow icon={trackIcons.clip}> | |
| <TrackRow | |
| icon={trackIcons.clip} | |
| onImport={handleImportCapRecording} | |
| importing={isImporting()} | |
| > |
| return Err("Not a valid Cap recording".to_string()); | ||
| } | ||
|
|
||
| let ext_meta = RecordingMeta::load_for_project(&recording_path) |
There was a problem hiding this comment.
Minor but helpful for dedupe/stability: canonicalize recording_path once after the existence check. This avoids treating the same folder as distinct due to symlinks, path separators, or case differences (especially on Windows), and also means you store the normalized absolute path in external_recordings.
| let ext_meta = RecordingMeta::load_for_project(&recording_path) | |
| let recording_path = recording_path | |
| .canonicalize() | |
| .map_err(|e| format!("Failed to resolve recording path: {e}"))?; | |
| let ext_meta = RecordingMeta::load_for_project(&recording_path) |
| const selected = await openDialog({ | ||
| directory: true, | ||
| title: "Select a Cap Recording to Import", | ||
| }); | ||
| if (!selected || typeof selected !== "string") return; | ||
| if (!selected.endsWith(".cap")) { | ||
| toast.error("Please select a .cap recording folder"); | ||
| return; | ||
| } | ||
| setIsImporting(true); | ||
| try { | ||
| await commands.importCapRecording(selected); | ||
| } catch (e) { | ||
| toast.error(String(e)); | ||
| setIsImporting(false); | ||
| } |
There was a problem hiding this comment.
Directory pickers can return a path with a trailing separator (and casing can differ), so endsWith(".cap") can false-negative. Also isImporting only resets in the error path; if the event doesn’t fire for any reason, the UI gets stuck disabled.
| const selected = await openDialog({ | |
| directory: true, | |
| title: "Select a Cap Recording to Import", | |
| }); | |
| if (!selected || typeof selected !== "string") return; | |
| if (!selected.endsWith(".cap")) { | |
| toast.error("Please select a .cap recording folder"); | |
| return; | |
| } | |
| setIsImporting(true); | |
| try { | |
| await commands.importCapRecording(selected); | |
| } catch (e) { | |
| toast.error(String(e)); | |
| setIsImporting(false); | |
| } | |
| const normalized = selected.replace(/[\\/]+$/, ""); | |
| if (!normalized.toLowerCase().endsWith(".cap")) { | |
| toast.error("Please select a .cap recording folder"); | |
| return; | |
| } | |
| setIsImporting(true); | |
| try { | |
| await commands.importCapRecording(normalized); | |
| } catch (e) { | |
| toast.error(String(e)); | |
| } finally { | |
| setIsImporting(false); | |
| } |
| <TrackRow | ||
| icon={trackIcons.clip} | ||
| onImport={handleImportCapRecording} | ||
| importing={isImporting()} | ||
| > |
There was a problem hiding this comment.
Looks like the video track lost its label; all other tracks still pass one, so the UI ends up inconsistent.
| <TrackRow | |
| icon={trackIcons.clip} | |
| onImport={handleImportCapRecording} | |
| importing={isImporting()} | |
| > | |
| <TrackRow | |
| icon={trackIcons.clip} | |
| label="Video" | |
| onImport={handleImportCapRecording} | |
| importing={isImporting()} | |
| > |
| </Show> | ||
| <Show when={props.onImport}> | ||
| <button | ||
| class="absolute inset-0 z-20 flex items-center justify-center rounded-xl border border-blue-400/70 bg-blue-500/90 text-white disabled:opacity-50" |
There was a problem hiding this comment.
Right now the import overlay permanently covers the track icon (unlike delete, which is hover-only). If this is meant as a secondary action, matching the delete hover behavior makes it a lot less visually aggressive.
| class="absolute inset-0 z-20 flex items-center justify-center rounded-xl border border-blue-400/70 bg-blue-500/90 text-white disabled:opacity-50" | |
| class="absolute inset-0 z-20 pointer-events-none flex items-center justify-center rounded-xl border border-blue-400/70 bg-blue-500/90 text-white opacity-0 transition-opacity group-hover/icon:pointer-events-auto group-hover/icon:opacity-100 disabled:opacity-50" |
| if let (Some(primary_first), Some(ext_first)) = ( | ||
| primary_recordings.segments.first(), | ||
| ext_recordings.segments.first(), | ||
| ) && (ext_first.display.width != primary_first.display.width | ||
| || ext_first.display.height != primary_first.display.height) | ||
| { | ||
| return Err(format!( | ||
| "Recording resolution {}x{} does not match project resolution {}x{}", | ||
| ext_first.display.width, | ||
| ext_first.display.height, | ||
| primary_first.display.width, | ||
| primary_first.display.height, | ||
| )); | ||
| } |
There was a problem hiding this comment.
Minor edge case: if either meta produces zero segments, we currently skip the resolution guard and still persist an external_recordings entry (but append no timeline segments). Failing fast here avoids writing a broken config.
| if let (Some(primary_first), Some(ext_first)) = ( | |
| primary_recordings.segments.first(), | |
| ext_recordings.segments.first(), | |
| ) && (ext_first.display.width != primary_first.display.width | |
| || ext_first.display.height != primary_first.display.height) | |
| { | |
| return Err(format!( | |
| "Recording resolution {}x{} does not match project resolution {}x{}", | |
| ext_first.display.width, | |
| ext_first.display.height, | |
| primary_first.display.width, | |
| primary_first.display.height, | |
| )); | |
| } | |
| let Some(primary_first) = primary_recordings.segments.first() else { | |
| return Err("Project has no segments".to_string()); | |
| }; | |
| let Some(ext_first) = ext_recordings.segments.first() else { | |
| return Err("External recording has no segments".to_string()); | |
| }; | |
| if ext_first.display.width != primary_first.display.width | |
| || ext_first.display.height != primary_first.display.height | |
| { | |
| return Err(format!( | |
| "Recording resolution {}x{} does not match project resolution {}x{}", | |
| ext_first.display.width, | |
| ext_first.display.height, | |
| primary_first.display.width, | |
| primary_first.display.height, | |
| )); | |
| } |
| } | ||
| }, |
There was a problem hiding this comment.
Quick sanity check: is dropping Spacedrive.framework intentional? If it’s still required for macOS builds, removing the frameworks entry will break bundling/linking.
| } | |
| }, | |
| }, | |
| "frameworks": ["../../../target/native-deps/Spacedrive.framework"] | |
| }, |
|
|
||
| let recording_path = recording_path | ||
| .canonicalize() | ||
| .map_err(|e| format!("Failed to resolve recording path: {e}"))?; |
There was a problem hiding this comment.
Worth guarding against importing the project into itself (user selects the current .cap folder). Without this, we can append duplicate clips/segments and end up with confusing timeline state.
| .map_err(|e| format!("Failed to resolve recording path: {e}"))?; | |
| let recording_path = recording_path | |
| .canonicalize() | |
| .map_err(|e| format!("Failed to resolve recording path: {e}"))?; | |
| let project_path = project_path | |
| .canonicalize() | |
| .map_err(|e| format!("Failed to resolve project path: {e}"))?; | |
| if recording_path == project_path { | |
| return Err("Cannot import a recording into itself".to_string()); | |
| } |
| inner.segments.len() | ||
| } | ||
| }) | ||
| .unwrap_or(0)) |
There was a problem hiding this comment.
If studio_meta() is None here, using 0 will under-count and can reintroduce recording_clip collisions. Since you already treat non-studio imports as errors, I’d probably fail fast for any existing external recording that isn’t studio too.
| .unwrap_or(0)) | |
| let studio = m | |
| .studio_meta() | |
| .ok_or_else(|| format!("existing external recording {i}: not a studio recording"))?; | |
| Ok(match studio { | |
| cap_project::StudioRecordingMeta::SingleSegment { .. } => 1usize, | |
| cap_project::StudioRecordingMeta::MultipleSegments { inner } => inner.segments.len(), | |
| }) |
| let ext_meta = RecordingMeta::load_for_project(&recording_path) | ||
| .map_err(|e| format!("Failed to load recording meta: {e}"))?; |
There was a problem hiding this comment.
Worth guarding against importing the project into itself (or via a symlink). Right now selecting the current project’s .cap folder will duplicate timeline segments and add a self-reference.
| let ext_meta = RecordingMeta::load_for_project(&recording_path) | |
| .map_err(|e| format!("Failed to load recording meta: {e}"))?; | |
| let project_path_canon = project_path | |
| .canonicalize() | |
| .map_err(|e| format!("Failed to resolve project path: {e}"))?; | |
| if recording_path == project_path_canon { | |
| return Err("Cannot import the current project recording".to_string()); | |
| } | |
| let ext_meta = RecordingMeta::load_for_project(&recording_path) | |
| .map_err(|e| format!("Failed to load recording meta: {e}"))?; |
| Ok(m.studio_meta() | ||
| .map(|s| match s { | ||
| cap_project::StudioRecordingMeta::SingleSegment { .. } => 1usize, | ||
| cap_project::StudioRecordingMeta::MultipleSegments { inner } => { | ||
| inner.segments.len() | ||
| } | ||
| }) | ||
| .unwrap_or(0)) |
There was a problem hiding this comment.
studio_meta() returning None currently becomes 0 segments, which can make clip_index_offset wrong and cause recording_clip collisions on the next import. Seems safer to error here like you do for load_for_project.
| Ok(m.studio_meta() | |
| .map(|s| match s { | |
| cap_project::StudioRecordingMeta::SingleSegment { .. } => 1usize, | |
| cap_project::StudioRecordingMeta::MultipleSegments { inner } => { | |
| inner.segments.len() | |
| } | |
| }) | |
| .unwrap_or(0)) | |
| let studio = m | |
| .studio_meta() | |
| .ok_or_else(|| format!("existing external recording {i}: not a studio recording"))?; | |
| Ok(match studio { | |
| cap_project::StudioRecordingMeta::SingleSegment { .. } => 1usize, | |
| cap_project::StudioRecordingMeta::MultipleSegments { inner } => inner.segments.len(), | |
| }) |
| { | ||
| let _preview_guard = ExportPreviewActiveGuard::try_new(&editor.export_preview_active)?; | ||
| info!("Using isolated FFmpeg renderer for Windows export preview"); | ||
| return generate_export_preview_inner(editor.project_path.clone(), frame_time, settings) |
There was a problem hiding this comment.
Note this path now bypasses the in-memory editor.project_config (and the cached segment_medias) and re-reads from disk via generate_export_preview_inner. If that’s intentional for the Windows release workaround, might be worth a brief comment/log since preview behavior will differ from other platforms and may ignore unsaved edits.
- Restore createUpdaterArtifacts to true (was accidentally set to false, which would break auto-update artifact generation in release builds) - Restore Spacedrive.framework to macOS bundle frameworks list - Move setIsImporting(false) to finally block so the button is always re-enabled regardless of whether the import command succeeds or fails; previously a successful command that lost the capRecordingImported event would leave the button permanently disabled
Absolute paths break project portability — moving either .cap folder silently corrupts the reference. Store paths relative to the primary project directory (consistent with the meta.rs RelativePathBuf anchor), and resolve them at all four read sites with an is_absolute() fallback for any data written before this change. No new dependencies. relative_path_from() implements the same algorithm as pathdiff using only std::path::Path::components().
|
hey @greptileai please review the PR |
Without canonicalization, relative_path_from() produces wrong results when either path contains symlinks (e.g. /var -> /private/var on macOS), causing the stored relative path to silently fail at render/export time. Without the self-import guard, importing a project into itself stores path: "" which resolves back to project_path at load time, doubling all segments and corrupting the timeline. Both canonicalize() calls fail fast with a clear error if the path cannot be resolved, rather than producing a bad stored value.
|
I've addressed the path portability concerns by switching new imports to relative paths while preserving backward compatibility for existing absolute paths. The remaining limitation is that I haven't been able to test this on a signed macOS build locally. If someone with a signed build environment could verify:
I'd be happy to address any issues found. |
I'm just unsure why this is the case... recording locally works fine for me? 🤔 which version of macOS are you using? I'm on 26.2. |
… type studio_meta() returning None was silently counted as 0 segments, producing a wrong clip_index_offset on the next import and causing recording_clip index collisions. Propagate as an error consistent with how the function already handles load_for_project failures.
|
@richiemcilroy Local recording itself works fine for me as well. The issue I ran into wasn't recording, but that the screen recording/accessibility permission checks weren't being recognized correctly for my unsigned local build, which prevented me from fully validating the flow. It's possible this is specific to my setup or a behavior change between 26.2 and 26.5.1. If you're able to reproduce the workflow successfully on 26.2, that would be useful data and may mean the problem isn't universal. |
TimelineSegment gained an optional name field on main (feat: add timeline segment name). The struct initializer in import_cap_recording was missing it, causing a compile error on CI.
Resolves conflicts in: - crates/editor/src/editor_instance.rs: keep create_all_segments alongside new upstream LegacyAudioTimingRepair and layers_rx ordering - crates/editor/src/lib.rs: keep create_all_segments export alongside new upstream Renderer/RendererHandle/playback exports - crates/project/src/configuration.rs: keep ExternalRecordingReference struct, adopt upstream Default removal on ProjectConfiguration
MinitJain
force-pushed
the
feat/recording-track
branch
from
1c2a85e to
a69f066
Compare
| if project_config | ||
| .external_recordings | ||
| .iter() | ||
| .any(|r| resolve_recording_path(&r.path, &project_path) == recording_path) |
There was a problem hiding this comment.
recording_path is canonicalized, but resolve_recording_path(..., project_path) returns a non-normalized path (e.g. contains .. once you store a relative path). PathBuf equality won’t collapse that, so this can miss duplicates.
| .any(|r| resolve_recording_path(&r.path, &project_path) == recording_path) | |
| .any(|r| { | |
| resolve_recording_path(&r.path, &project_path) | |
| .canonicalize() | |
| .map(|p| p == recording_path) | |
| .unwrap_or(false) | |
| }) |
| path: relative_path_from(&project_path, &recording_path) | ||
| .to_string_lossy() | ||
| .to_string(), |
There was a problem hiding this comment.
Minor, but to_string_lossy() can silently mangle non-UTF-8 paths and then you’ll persist something you can’t resolve on next open.
| path: relative_path_from(&project_path, &recording_path) | |
| .to_string_lossy() | |
| .to_string(), | |
| path: relative_path_from(&project_path, &recording_path) | |
| .to_str() | |
| .ok_or("Recording path is not valid UTF-8")? | |
| .to_string(), |
…rackRow import - Dedup check now canonicalizes the resolved relative path before comparing to the canonicalized recording_path, so paths containing .. are correctly normalized and won't miss duplicates - Replace to_string_lossy() with to_str().ok_or_else() so a non-UTF-8 recording path fails fast with a clear error rather than silently storing a mangled path that cannot be resolved on next open - Wire onImport/importing props to the Video TrackRow so the hover overlay actually renders (was dead code — props defined but never passed)
| if project_config | ||
| .external_recordings | ||
| .iter() | ||
| .any(|r| { |
There was a problem hiding this comment.
Since external_recordings can now be stored as relative paths (including ..), resolve_recording_path(... ) == recording_path can miss duplicates when recording_path is canonicalized.
| .any(|r| { | |
| .any(|r| { | |
| resolve_recording_path(&r.path, &project_path) | |
| .canonicalize() | |
| .map(|p| p == recording_path) | |
| .unwrap_or(false) | |
| }) |
|
|
||
| let label = ext_meta.pretty_name.clone(); | ||
|
|
There was a problem hiding this comment.
to_string_lossy() will silently mangle non-UTF-8 paths; failing fast here is safer since you persist this into the config.
| let label = ext_meta.pretty_name.clone(); | |
| path: relative_path_from(&project_path, &recording_path) | |
| .to_str() | |
| .ok_or_else(|| "Recording path is not valid UTF-8".to_string())? | |
| .to_string(), |
| pub project_path: String, | ||
| } | ||
|
|
||
| fn relative_path_from(base: &std::path::Path, target: &std::path::Path) -> PathBuf { |
There was a problem hiding this comment.
relative_path_from assumes base/target share a common prefix. On Windows, importing from a different drive (or UNC vs disk) will produce a nonsense path like ..\..\C:\... and then break subsequent loads.
| fn relative_path_from(base: &std::path::Path, target: &std::path::Path) -> PathBuf { | |
| fn relative_path_from(base: &std::path::Path, target: &std::path::Path) -> PathBuf { | |
| #[cfg(windows)] | |
| { | |
| use std::path::Component; | |
| if let (Some(Component::Prefix(a)), Some(Component::Prefix(b))) = | |
| (base.components().next(), target.components().next()) | |
| { | |
| if a != b { | |
| return target.to_path_buf(); | |
| } | |
| } | |
| } | |
| let base: Vec<_> = base.components().collect(); | |
| let target: Vec<_> = target.components().collect(); | |
| let common = base.iter().zip(&target).take_while(|(a, b)| a == b).count(); | |
| let mut rel = PathBuf::new(); | |
| for _ in 0..(base.len() - common) { | |
| rel.push(".."); | |
| } | |
| for c in &target[common..] { | |
| rel.push(c); | |
| } | |
| rel | |
| } |
| @@ -1047,6 +1099,25 @@ function TrackRow(props: { | |||
| <IconCapTrash class="size-4" /> | |||
There was a problem hiding this comment.
Not seeing any TrackRow call sites passing onImport/importing in this PR, so this overlay looks like dead code right now. If the toolbar button is the intended UX, it might be cleaner to remove the TrackRow props + overlay until it’s actually wired up.
…ive path - Import overlay on TrackRow was always visible (missing opacity-0 and pointer-events-none base classes); now matches delete button hover behavior - Resolution check used if-let so zero-segment recordings silently bypassed the check and still persisted an external_recordings entry with no timeline segments appended; now fails fast with explicit errors for both cases - relative_path_from on Windows with different drive letters (C:\ vs D:\) produced a nonsense path; fall back to storing the absolute path instead
![superagent-security[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
| if p.is_absolute() { | ||
| p.to_path_buf() | ||
| } else { | ||
| recording_meta.project_path.join(p) |
There was a problem hiding this comment.
P2: Unsanitized external recording paths enable directory traversal in editor pipeline
Deserialized config path is joined directly with project_path, allowing .. traversal to arbitrary directories.
Sanitize external recording paths by rejecting .. components and scope-checking resolved paths.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="crates/editor/src/editor_instance.rs">
<violation number="1" location="crates/editor/src/editor_instance.rs:644">
<priority>P2</priority>
<title>Unsanitized external recording paths enable directory traversal in editor pipeline</title>
<evidence>recording_meta.project_path.join(p) directly joins the deserialized ext_ref.path from external_recordings config without sanitizing .. components. A malicious ProjectConfiguration with entries like {"path": "../../../sensitive/recording"} would cause Cap to traverse outside the project directory and attempt to load arbitrary filesystem locations when the project is opened or exported.</evidence>
<recommendation>Validate and sanitize ext_ref.path before joining. Reject paths containing .. components, ensure relative paths stay within the project scope, or canonicalize the resolved path and verify it falls within an allowed base directory.</recommendation>
</violation>
</file>
There was a problem hiding this comment.
Not exploitable in practice. This is a local desktop app — the threat model requires an attacker to deliver a malicious .cap project to the user's machine and have them open it, at which point arbitrary filesystem access is already available.
load_for_project reads only recording-meta.json — structured JSON parsed by Cap, not executed. No shell injection, no credential exposure. Worst case is an error message containing a file-not-found path.
Relative paths containing .. are intentional — that's how project-portable references are stored. Stripping them would break the feature.
There was a problem hiding this comment.
The developer provides a coherent threat-model and functional justification. In a local desktop app, opening a malicious .cap project already presupposes attacker access to the user's machine and user interaction, bounding the practical impact. The use of .. for project-portable external references is intentional and required for correct behavior, and the affected code is limited to structured JSON loading with no shell execution or credential exposure risk. Dismissing as acceptable risk.
| if p.is_absolute() { | ||
| p.to_path_buf() | ||
| } else { | ||
| recording_path.join(p) |
There was a problem hiding this comment.
P2: Unsanitized external recording paths enable directory traversal in rendering pipeline
Deserialized config path is joined directly with recording_path, allowing .. traversal to arbitrary directories.
Sanitize external recording paths by rejecting .. components and scope-checking resolved paths.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="crates/rendering/src/project_recordings.rs">
<violation number="1" location="crates/rendering/src/project_recordings.rs:228">
<priority>P2</priority>
<title>Unsanitized external recording paths enable directory traversal in rendering pipeline</title>
<evidence>recording_path.join(p) directly joins the deserialized ext_ref.path from external_recordings config without sanitizing .. components. A malicious ProjectConfiguration with entries like {"path": "../../../sensitive/recording"} would cause Cap to traverse outside the project directory and attempt to load arbitrary filesystem locations when rendering previews or exporting.</evidence>
<recommendation>Validate and sanitize ext_ref.path before joining. Reject paths containing .. components, ensure relative paths stay within the project scope, or canonicalize the resolved path and verify it falls within an allowed base directory.</recommendation>
</violation>
</file>
There was a problem hiding this comment.
Same as the editor pipeline finding — not exploitable in this context. Local desktop app, no code execution, load_for_project reads only structured JSON. Relative paths with .. are by design for project portability.
There was a problem hiding this comment.
Acknowledged. In the local desktop app context, relative paths with .. are an intentional portability feature for user-trusted project files, and the rendering pipeline does not introduce additional privilege or code execution risk beyond the user's existing filesystem access.
superagent-security
Bot
added
pr:flagged
| EditorInstances::remove(window.clone()).await; | ||
|
|
||
| CapRecordingImported { | ||
| project_path: project_path.to_string_lossy().to_string(), |
There was a problem hiding this comment.
Minor nit: to_string_lossy() here can silently mangle the path. Since you already fail fast for non-UTF-8 recording_path, it’d be more consistent to do the same for project_path.
| project_path: project_path.to_string_lossy().to_string(), | |
| project_path: project_path.to_str().ok_or("Project path contains non-UTF-8 characters")?.to_string(), |
| title: "Select a Cap Recording to Import", | ||
| }); | ||
| if (!selected || typeof selected !== "string") return; | ||
| if (!selected.endsWith(".cap")) { |
There was a problem hiding this comment.
The .cap check is case-sensitive; on some systems users may have FOO.CAP / mixed-case folder names.
| if (!selected.endsWith(".cap")) { | |
| if (!selected.toLowerCase().endsWith(".cap")) { |
| } | ||
| }; | ||
| let ext_meta = cap_project::RecordingMeta::load_for_project(&ext_path) | ||
| .map_err(|e| format!("external recording {i}: {e}"))?; |
There was a problem hiding this comment.
For debugging it might help to include the resolved path in this error (especially since external_recordings can be relative).
| .map_err(|e| format!("external recording {i}: {e}"))?; | |
| let ext_meta = cap_project::RecordingMeta::load_for_project(&ext_path) | |
| .map_err(|e| format!("external recording {i} ({}): {e}", ext_path.display()))?; |
- Add `external_recordings: Default::default()` to ProjectConfiguration manual Default impl (fixes E0063 clippy error on aarch64) - Replace `to_string_lossy()` with `to_str().ok_or_err()` for project_path in CapRecordingImported event (fails fast on non-UTF-8 rather than silently mangling)
| if p.is_absolute() { | ||
| p.to_path_buf() | ||
| } else { | ||
| project_path.join(p) |
There was a problem hiding this comment.
P2: Stored external recording paths are not sanitized, allowing path traversal
Unsanitized external recording path strings from project config are joined against the project directory, allowing traversal outside the project scope.
Validate that resolved external recording paths are contained within an allowed base directory before loading.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="apps/desktop/src-tauri/src/lib.rs">
<violation number="1" location="apps/desktop/src-tauri/src/lib.rs:2883">
<priority>P2</priority>
<title>Stored external recording paths are not sanitized, allowing path traversal</title>
<evidence>The resolve_recording_path function and the corresponding loading logic in ProjectRecordingsMeta::new_with_external and create_all_segments resolve ExternalRecordingReference.path directly via Path::new and Path::join without sanitizing .. components. A malicious project configuration could reference files outside the project directory.</evidence>
<recommendation>Before resolving any stored external recording path, normalize it and ensure the resulting absolute path is within the project directory or the user's explicitly selected import scope. Reject paths that escape the intended sandbox.</recommendation>
</violation>
</file>
|
|
||
| let project_path = { | ||
| let window_ids = EditorWindowIds::get(window.app_handle()); | ||
| let window_ids = window_ids.ids.lock().unwrap(); |
There was a problem hiding this comment.
lock().unwrap() can panic (poisoned mutex) and crash the backend here; might be better to turn that into a normal command error.
| let window_ids = window_ids.ids.lock().unwrap(); | |
| let window_ids = window_ids | |
| .ids | |
| .lock() | |
| .map_err(|_| "Failed to lock editor window ids".to_string())?; |
Poisoned mutex would panic the Tauri backend process. All three EditorWindowIds lock sites in the import command now return a command error instead.
superagent-security
Bot
removed
the
pr:flagged
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
| let ext_meta = cap_project::RecordingMeta::load_for_project(&ext_path) | ||
| .map_err(|e| format!("external recording {i}: {e}"))?; | ||
| let cap_project::RecordingMetaInner::Studio(ext_studio_meta) = &ext_meta.inner else { | ||
| return Err(format!("external recording {i}: not a studio recording")); | ||
| }; | ||
| let ext_segments = create_segments(&ext_meta, ext_studio_meta.as_ref(), force_ffmpeg) | ||
| .await | ||
| .map_err(|e| format!("external recording {i}: {e}"))?; |
There was a problem hiding this comment.
Including the resolved path in these errors makes debugging a lot easier (especially with relative external_recordings).
| let ext_meta = cap_project::RecordingMeta::load_for_project(&ext_path) | |
| .map_err(|e| format!("external recording {i}: {e}"))?; | |
| let cap_project::RecordingMetaInner::Studio(ext_studio_meta) = &ext_meta.inner else { | |
| return Err(format!("external recording {i}: not a studio recording")); | |
| }; | |
| let ext_segments = create_segments(&ext_meta, ext_studio_meta.as_ref(), force_ffmpeg) | |
| .await | |
| .map_err(|e| format!("external recording {i}: {e}"))?; | |
| let ext_meta = cap_project::RecordingMeta::load_for_project(&ext_path) | |
| .map_err(|e| format!("external recording {i} ({}): {e}", ext_path.display()))?; | |
| let cap_project::RecordingMetaInner::Studio(ext_studio_meta) = &ext_meta.inner else { | |
| return Err(format!("external recording {i}: not a studio recording")); | |
| }; | |
| let ext_segments = create_segments(&ext_meta, ext_studio_meta.as_ref(), force_ffmpeg) | |
| .await | |
| .map_err(|e| format!("external recording {i} ({}): {e}", ext_path.display()))?; |
| .canonicalize() | ||
| .map_err(|e| format!("Failed to resolve recording path: {e}"))?; | ||
| let project_path = project_path | ||
| .canonicalize() |
There was a problem hiding this comment.
Minor edge case: since project_path gets canonicalized before relative_path_from(&project_path, &recording_path), the stored relative path is based on the realpath. If the project is later opened via a symlink / different casing path, resolving the stored .. path against that non-canonical base can point somewhere else. Consider keeping the original project_path for relative-path computation (and using canonical paths only for equality/validation), or storing an absolute canonical path.
Merge CapSoftware/Cap upstream/main into feat/recording-track. Four TypeScript errors that appear in the merge state are fixed: - apps/desktop/src/routes/editor/context.ts: explicitly cast config.captions to EditorCaptionsData | null in normalizeProject. CaptionSettings.animation is typed string in the generated bindings but EditorCaptionSettings.animation narrows it to CaptionAnimation; the cast is safe at runtime since the Rust enum always emits one of the three CaptionAnimation values. - apps/desktop/src/store/captions.ts: cast loadedSettings as EditorCaptionSettings for the same reason. - apps/web/__tests__/unit/caption-tracks.test.ts: change createVideo return type from HTMLVideoElement to FakeVideo & HTMLVideoElement so that .dispatch() is visible to TypeScript without a cast at every call site. - apps/web/package.json: add @fortawesome/fontawesome-svg-core as an explicit dependency; Footer.tsx imports from it but it was only an implicit peer dep of @fortawesome/react-fontawesome.
| window.location.reload(); | ||
| }); | ||
| onCleanup(() => { | ||
| importedListenerPromise.then((unlisten) => unlisten()); |
There was a problem hiding this comment.
listen() returns a promise; if it rejects, this cleanup can trigger an unhandled rejection. Might be worth swallowing/logging it here.
| importedListenerPromise.then((unlisten) => unlisten()); | |
| onCleanup(() => { | |
| importedListenerPromise.then((unlisten) => unlisten()).catch(() => {}); | |
| }); |
| } | ||
| }; | ||
| let ext_meta = cap_project::RecordingMeta::load_for_project(&ext_path) | ||
| .map_err(|e| format!("external recording {i}: failed to load meta: {e}"))?; |
There was a problem hiding this comment.
Including the resolved path here makes it much easier to debug (especially since external_recordings can be relative).
| .map_err(|e| format!("external recording {i}: failed to load meta: {e}"))?; | |
| let ext_meta = cap_project::RecordingMeta::load_for_project(&ext_path) | |
| .map_err(|e| { | |
| format!( | |
| "external recording {i} ({}): failed to load meta: {e}", | |
| ext_path.display() | |
| ) | |
| })?; |
2 participants
Summary
ExternalRecordingReferencestruct to project config andexternal_recordingsfield onProjectConfigurationProjectRecordingsMeta::new_with_external()to load primary + external recording segments togethercreate_all_segments()in the editor crate to build a unified segment list across all recordingsimport_cap_recordingTauri command: validates.capfolder, checks resolution match, prevents duplicate imports, computes correct clip index offsets, appends segments to the project timelineexternal_recordingsthroughCloses #1712
macOS Sequoia (Darwin 25) blocks ScreenCaptureKit on unsigned dev binaries —
GetShareableContent: The user declined TCCs for application, window, display capture— preventing the app from recording during development.Build is clean (
cargo build+pnpm typecheckpass). Needs testing on a signed build or by a reviewer with a valid Apple developer certificate.Known limitations
window.location.reload()on successful import loses any unsaved editor stateTest plan
.caprecording into an existing project.capfolder — should show error🤖 Generated with Claude Code
Greptile Summary
This PR introduces
.caprecording import into the timeline editor, stitching a secondary recording's segments onto the end of the primary project by appending entries to a newexternal_recordingsfield onProjectConfigurationand updating the export/preview pipelines to load combined segments viaProjectRecordingsMeta::new_with_externalandcreate_all_segments.import_cap_recording): validates the recording folder, checks resolution compatibility against the primary's first segment, guards against duplicate imports via canonical-path comparison, computes the correctrecording_clipoffset by summing primary + all existing external segment counts (with proper error propagation), then writes the updated config and forces a page reload by emittingCapRecordingImported.external_recordingscorrectly; the Windows release build now routes fast previews through the isolated FFmpeg renderer path.capRecordingImportedevent listener triggerswindow.location.reload()to reinitialise the editor with the new config and segment list.Confidence Score: 5/5
Safe to merge; the import logic, offset calculation, and pipeline integration are all correct. The two nits noted are low-risk and do not affect the happy path on the vast majority of machines.
The clip-index offset arithmetic is sound (primary segments + prior external segment counts, propagating errors via
?),new_with_externalre-validates resolution for every external segment during rendering, and the editor instance is always recreated after import sosegment_mediasstays consistent with the config. The one logic note (to_string_lossypath mangling) only affects systems with non-UTF-8 recording paths, which Cap itself would never produce.apps/desktop/src-tauri/src/lib.rs(import_cap_recording) andapps/desktop/src-tauri/src/export.rs(test reorganisation) are the most significant changed areas and are worth a second look before merging.Important Files Changed
import_cap_recordingTauri command with validation, duplicate detection, and correct clip-index offset computation; error propagation via?is clean.new_with_externalthat appends external recording segments after primary segments; resolution re-validation per external recording is correct.external_recordingstocreate_all_segmentssosegment_mediasalways includes external segments; consistent with the new pipeline.new_with_externalandcreate_all_segments; also adds Windows release FFmpeg workaround, removing a non-Windows test in the process.ExternalRecordingReferencestruct andexternal_recordingsfield (with#[serde(default)]) toProjectConfiguration; backwards-compatible.capRecordingImportedevent fires server-side before the command resolves.importCapRecordingcommand andcapRecordingImportedevent.create_all_segmentsalongside existingcreate_segments; no logic change.external_recordingsthrough the export pipeline.Comments Outside Diff (1)
apps/desktop/src/routes/editor/Timeline/index.tsx, line 1071-1079 (link)TrackRowonImport/importingprops are dead codeonImportandimportingare added to theTrackRowprops interface and the overlay button is wired up insideTrackRow, but no caller in this diff ever passesonImportto aTrackRow. The overlay will never render. Either wire it up at the usage site or remove these props to avoid dead code.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(editor): reset isImporting in finall..." | Re-trigger Greptile