Import recordings into editor and improve rendering stability#1837
Conversation
superagent-security
Bot
added
contributor:verified
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| pub fn is_warp(&self) -> bool { | ||
| self.description.contains("Microsoft Basic Render Driver") | ||
| || self.description.contains("WARP") | ||
| matches_non_hardware_adapter_marker(&self.description) |
There was a problem hiding this comment.
is_warp() now matches non-WARP adapters, causing semantic drift
After this change, is_warp() delegates to matches_non_hardware_adapter_marker, so it returns true for Parsec, DisplayLink, Splashtop, and other virtual adapters — not just WARP. Any call site that uses is_warp() to distinguish WARP-specific behavior from, say, "Parsec virtual display" will silently conflate them. Consider renaming the method to is_non_hardware_adapter() or is_virtual_display() to match the expanded semantics, or keeping a narrower WARP-only check.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/frame-converter/src/d3d11.rs
Line: 108-111
Comment:
**`is_warp()` now matches non-WARP adapters, causing semantic drift**
After this change, `is_warp()` delegates to `matches_non_hardware_adapter_marker`, so it returns `true` for Parsec, DisplayLink, Splashtop, and other virtual adapters — not just WARP. Any call site that uses `is_warp()` to distinguish WARP-specific behavior from, say, "Parsec virtual display" will silently conflate them. Consider renaming the method to `is_non_hardware_adapter()` or `is_virtual_display()` to match the expanded semantics, or keeping a narrower WARP-only check.
How can I resolve this? If you propose a fix, please make it concise.| const importedCount = await invoke<number>( | ||
| "add_existing_recording_to_editor", | ||
| { sourcePath }, | ||
| ); |
There was a problem hiding this comment.
invoke function instead of the generated typed binding commands.addExistingRecordingToEditor that was added in this same PR to tauri.ts. The rest of this file consistently uses commands.*, and bypassing the generated binding means any future signature change won't be caught by the TypeScript compiler.
| const importedCount = await invoke<number>( | |
| "add_existing_recording_to_editor", | |
| { sourcePath }, | |
| ); | |
| const importedCount = | |
| await commands.addExistingRecordingToEditor(sourcePath); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/editor/Header.tsx
Line: 134-137
Comment:
The command invocation uses the raw `invoke` function instead of the generated typed binding `commands.addExistingRecordingToEditor` that was added in this same PR to `tauri.ts`. The rest of this file consistently uses `commands.*`, and bypassing the generated binding means any future signature change won't be caught by the TypeScript compiler.
```suggestion
const importedCount =
await commands.addExistingRecordingToEditor(sourcePath);
```
How can I resolve this? If you propose a fix, please make it concise.![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| @@ -1,14 +1,20 @@ | |||
| import { Button } from "@cap/ui-solid"; | |||
| import { Dialog as KDialog } from "@kobalte/core/dialog"; | |||
| import { convertFileSrc, invoke } from "@tauri-apps/api/core"; | |||
There was a problem hiding this comment.
You can drop the direct invoke import once the call site uses the typed commands wrapper.
| import { convertFileSrc, invoke } from "@tauri-apps/api/core"; | |
| import { convertFileSrc } from "@tauri-apps/api/core"; |
| await commands.setProjectConfig(serializeProjectConfiguration(project)); | ||
| const importedCount = await invoke<number>( | ||
| "add_existing_recording_to_editor", | ||
| { sourcePath }, | ||
| ); |
There was a problem hiding this comment.
Minor consistency/typing win: use the generated commands binding instead of a raw string command.
| await commands.setProjectConfig(serializeProjectConfiguration(project)); | |
| const importedCount = await invoke<number>( | |
| "add_existing_recording_to_editor", | |
| { sourcePath }, | |
| ); | |
| await commands.setProjectConfig(serializeProjectConfiguration(project)); | |
| const importedCount = await commands.addExistingRecordingToEditor(sourcePath); |
| await fs.writeFile(probePath, ""); | ||
| await execFile(sccachePath, [ | ||
| rustcPath, | ||
| probePath, | ||
| "--crate-name", | ||
| "___", | ||
| "--print=file-names", | ||
| "--crate-type", | ||
| "bin", | ||
| "--crate-type", | ||
| "rlib", | ||
| "--crate-type", | ||
| "dylib", | ||
| "--crate-type", | ||
| "cdylib", | ||
| "--crate-type", | ||
| "staticlib", | ||
| "--crate-type", | ||
| "proc-macro", | ||
| "--print=sysroot", | ||
| "--print=split-debuginfo", | ||
| "--print=crate-name", | ||
| "--print=cfg", | ||
| "-Wwarnings", | ||
| ]); |
There was a problem hiding this comment.
This probe is likely to fail with an empty source file (e.g. --crate-type bin expects main), which would make sccache look unusable even when it is. Also, proc-macro generally needs special source.
| await fs.writeFile(probePath, ""); | |
| await execFile(sccachePath, [ | |
| rustcPath, | |
| probePath, | |
| "--crate-name", | |
| "___", | |
| "--print=file-names", | |
| "--crate-type", | |
| "bin", | |
| "--crate-type", | |
| "rlib", | |
| "--crate-type", | |
| "dylib", | |
| "--crate-type", | |
| "cdylib", | |
| "--crate-type", | |
| "staticlib", | |
| "--crate-type", | |
| "proc-macro", | |
| "--print=sysroot", | |
| "--print=split-debuginfo", | |
| "--print=crate-name", | |
| "--print=cfg", | |
| "-Wwarnings", | |
| ]); | |
| await fs.writeFile(probePath, "fn main() {}\n"); | |
| await execFile(sccachePath, [ | |
| rustcPath, | |
| probePath, | |
| "--crate-name", | |
| "___", | |
| "--print=file-names", | |
| "--crate-type", | |
| "bin", | |
| "--crate-type", | |
| "rlib", | |
| "--crate-type", | |
| "dylib", | |
| "--crate-type", | |
| "cdylib", | |
| "--crate-type", | |
| "staticlib", | |
| "--print=sysroot", | |
| "--print=split-debuginfo", | |
| "--print=crate-name", | |
| "--print=cfg", | |
| "-Wwarnings", | |
| ]); |
| GpuVendor::Intel => ENCODER_PRIORITY_INTEL, | ||
| _ => ENCODER_PRIORITY_DEFAULT, | ||
| }, | ||
| None => &["libx264"], |
There was a problem hiding this comment.
If DXGI enumeration fails (detect_primary_gpu() == None), forcing libx264 can unnecessarily disable HW encoding on systems where encoders work but detection is flaky. I'd keep the old default order in that case and let the encoder-open step decide.
| None => &["libx264"], | |
| None => ENCODER_PRIORITY_DEFAULT, |
|
please re-review the pr @greptileai |
| Ok(1) | ||
| } | ||
|
|
||
| async fn append_cap_project_to_editor_project( |
There was a problem hiding this comment.
Nice import path. One UX thing: append_cap_project_to_editor_project can copy a lot of segment/cursor/event data but doesn't emit any emit_progress updates (unlike append_mp4_to_editor_project). Even a couple of stage + percentage updates in the copy loop would help avoid a "stuck" toast on large projects.
| async fn append_cap_project_to_editor_project( | |
| let project_path_str = target_project_path.to_string_lossy().to_string(); | |
| emit_progress(&app, &project_path_str, ImportStage::Finalizing, 0.0, "Importing Cap project..."); |
superagent-security
Bot
added
pr:verified
|
please re-review the pr @greptile just addressed the security review |
| : `${importedCount} recordings imported`, | ||
| { id: toastId }, | ||
| ); | ||
| window.location.reload(); |
There was a problem hiding this comment.
This reload happens immediately after toast.success(...), so the success state likely never renders. Either drop the success toast or delay the reload slightly.
| window.location.reload(); | |
| setTimeout(() => window.location.reload(), 750); |
|
|
||
| let can_decode = | ||
| probe_video_can_decode(&source_path).map_err(|e| format!("Cannot decode video: {e}"))?; | ||
| if !can_decode { |
There was a problem hiding this comment.
unique_segment_dir(...) creates the segment directory before probing/transcoding. If we return early here, that new directory becomes an unreferenced disk leak.
| if !can_decode { | |
| if !can_decode { | |
| let _ = std::fs::remove_dir_all(target_project_path.join(&target_relative_dir)); | |
| return Err("Video format not supported or file is corrupted".to_string()); | |
| } |
| ) | ||
| }) | ||
| .await | ||
| .map_err(|e| format!("Video import task failed: {e}"))? |
There was a problem hiding this comment.
Similarly, if transcode_video fails after the dir is created, we return early and leave the orphaned segment dir behind. Cleaning up in the error mapping keeps the filesystem tidy.
| .map_err(|e| format!("Video import task failed: {e}"))? | |
| .await | |
| .map_err(|e| { | |
| let _ = std::fs::remove_dir_all(target_project_path.join(&target_relative_dir)); | |
| format!("Video import task failed: {e}") | |
| })? | |
| .map_err(|e| { | |
| let _ = std::fs::remove_dir_all(target_project_path.join(&target_relative_dir)); | |
| e.to_string() | |
| })?; |
1 participant
Summary
Validation
Greptile Summary
This PR adds an editor import flow for existing Cap recordings and MP4 files, improves rendering/export adapter handling, and makes sccache opt-in. The previous security review findings have all been addressed: path-traversal is blocked by
normalized_metadata_relative_path(rejects.., absolute paths, Windows drive letters) plus a canonical-path containment check that also catches symlink escapes; comprehensive unit tests cover these paths; and the frontend now correctly uses the generatedcommands.addExistingRecordingToEditortyped binding.import.rs):add_existing_recording_to_editorcopies Cap project segments or transcodes an MP4 into the target project with full path validation, extension allowlists, and cursor-ID remapping.display.rs,yuv_converter.rs): Frame textures are now sized to the actual source frame rather than the configured frame size, and YUV plane stride is validated before upload.d3d11.rs,rendering/lib.rs,enc-ffmpeg): Non-hardware adapters (Parsec, DisplayLink, Splashtop, WARP, etc.) are now detected consistently and excluded from hardware-encoder selection.Confidence Score: 5/5
Safe to merge; the new import path is well-guarded against path traversal and the rendering fixes are mechanical substitutions of source_size for frame_size.
All changed paths are either additive (new import command with thorough validation and tests) or targeted bug fixes (frame-size mismatches, stride guards, adapter detection). No regressions were identified in the existing export or rendering flows.
No files require special attention; the duplicate marker list in
crates/rendering/src/lib.rsandcrates/frame-converter/src/d3d11.rsis worth a future clean-up but does not affect correctness today.Important Files Changed
add_existing_recording_to_editorcommand with robust path-traversal defenses (normalized relative paths, extension allowlists, canonical path containment checks, symlink escape tests), plus helpers for copying segments/cursors/audio into target projects.NON_HARDWARE_ADAPTER_MARKERSlist to detect virtual adapters (Parsec, DisplayLink, Splashtop, WARP, etc.) and consolidates theis_softwaredetection intois_non_hardware_adapter;is_warp()retains its original narrow semantics.is_software_wgpu_adapterto also flagwgpu::DeviceType::VirtualGpuand matches against the same set of non-hardware adapter names as d3d11.rs.source_size) instead of the configuredframe_sizefor texture creation, upload, and YUV conversion, and scales crop/frame uniforms accordingly.is_exportis true and constrains the AMD export priority override to adapters that actually support hardware encoding.commands.addExistingRecordingToEditortyped binding; filters out the current project and non-complete recordings before display.Comments Outside Diff (1)
apps/desktop/src-tauri/src/import.rs, line 928-1046 (link)append_cap_project_to_editor_projectcan copy many segments, cursor images, and event files without emitting anyemit_progresscalls, whileappend_mp4_to_editor_projectemitsImportStage::Probing,ImportStage::Converting, andImportStage::Complete. For a large multi-segment project the user will see only the "Importing recording…" toast with no progress indication until the operation finishes.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix: validate imported recording asset p..." | Re-trigger Greptile