feat: Add fragmented mp4 (.fMP4) for Studio Mode #1441
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds fragmentation and crash-recovery support across recording, encoding, remuxing, and desktop UI: new segmented/fragmented encoders, a RecoveryManager with Tauri commands, manifest/fragment management, pipeline plumbing for fragmented recording, remuxing utilities, and frontend components to detect and recover incomplete recordings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Tauri
participant RecoveryMgr
participant FileSystem
participant Remuxer
User->>Frontend: App starts or opens recovery UI
Frontend->>Tauri: find_incomplete_recordings()
Tauri->>RecoveryMgr: find_incomplete(recordings_dir)
RecoveryMgr->>FileSystem: scan recordings dir, read manifests
RecoveryMgr-->>Tauri: Vec<IncompleteRecordingInfo>
Tauri-->>Frontend: show RecoveryToast
User->>Frontend: Click "Recover"
Frontend->>Tauri: recover_recording(project_path)
Tauri->>RecoveryMgr: recover(&recording)
RecoveryMgr->>FileSystem: analyze fragments & manifests
RecoveryMgr->>Remuxer: concatenate_video_fragments / audio
Remuxer-->>RecoveryMgr: produced recovered media files
RecoveryMgr->>FileSystem: write recovered meta & project config
RecoveryMgr-->>Tauri: recovered project path
Tauri-->>Frontend: open editor with recovered project
sequenceDiagram
participant Studio
participant Pipeline
participant SegEncoder
participant FS
Studio->>Pipeline: start(recording, fragmented=true)
Pipeline->>SegEncoder: init(base_path, segment_duration)
SegEncoder->>FS: create fragment_000.mp4 + manifest.json
loop frames
Studio->>Pipeline: queue_video_frame(ts)
Pipeline->>SegEncoder: queue_video_frame(ts)
alt segment time exceeded
SegEncoder->>FS: finalize current fragment, rotate, write manifest
end
end
Studio->>SegEncoder: finish(final_ts)
SegEncoder->>FS: finalize last fragment, write manifest (complete)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/recording/src/studio_recording.rs (1)
884-894: Windows camera pipeline doesn't respect the fragmentation setting.Unlike macOS, the Windows camera pipeline doesn't conditionally apply fragmentation based on the
fragmentedflag. It always usesWindowsCameraMuxerConfig::default()(which setsfragmented: false). If crash recovery is needed on Windows, the camera setup should branch based on thefragmentedflag, similar to the macOS implementation at lines 860-876.
🧹 Nitpick comments (23)
apps/desktop/src-tauri/src/platform/macos/delegates.rs (1)
78-81: Good defensive change, but inner callbacks still panic.The guarded pattern here is a solid improvement. However,
on_window_did_resize(line 122) andon_window_did_exit_full_screen(line 256) still call.expect()onns_window(), which can panic insideextern "C"callbacks. Consider applying the same guarded pattern there for consistency.apps/desktop/src-tauri/src/platform/macos/mod.rs (1)
21-24: Consider logging on failure for debuggability.Unlike the similar pattern in
delegates.rs(line 79), this silently returns on failure. Adding atracing::warn!would help diagnose issues when window level setting fails unexpectedly.let Ok(ns_win) = c_window.ns_window() else { + tracing::warn!("Failed to get window handle for set_window_level"); return; };apps/desktop/src/routes/editor/context.ts (2)
743-744: Unused signal - consider removing or usingwsConnectedinstead.The
isConnectedsignal is set but never read in the visible code. Additionally,createImageDataWSalready returns awsConnectedfunction (line 750) that tracks connection state. Consider either usingwsConnecteddirectly or removing the duplicate signal if it's not needed elsewhere.
746-748: Remove debug console.log statements before merge.These logging statements appear to be debug artifacts. Per coding guidelines, code should be self-explanatory. Consider removing them or replacing with structured logging if persistent diagnostics are needed.
const [editorInstance] = createResource(async () => { - console.log("[Editor] Creating editor instance..."); const instance = await commands.createEditorInstance(); - console.log("[Editor] Editor instance created, setting up WebSocket");.claude/settings.local.json (1)
10-33: Expanded Bash/Web permissions — confirm intended scopeThe allow‑list now includes powerful commands (
cargo run,python3,ffprobe,git log, multipleWebFetchdomains, etc.). If this config is ever used beyond strictly local/sandboxed development, it may be worth tightening to just the commands required for this workflow to minimize surface area.crates/mediafoundation-ffmpeg/src/h264.rs (1)
174-214: Clarify error semantics ofset_fragmented_mp4_optionsThis helper logs failures from
av_opt_setbut still returnsOk(())unconditionally, which makes theResult<(), ffmpeg::Error>signature misleading for callers. Consider either changing the return type to()if configuration is best‑effort, or mapping the first negativeretinto a concrete error so callers can react when fragmented‑MP4 options cannot be applied.crates/enc-ffmpeg/src/mux/fragmented_audio.rs (1)
75-92: Simplifyfinish’s nestedResultand make error handling more explicit
finishreturningResult<Result<(), ffmpeg::Error>, FinishError>is awkward to consume and duplicates error channels (flush errors as innerResult, trailer errors asFinishError::WriteTrailerFailed), while the else branch andDropsilently ignore trailer/finish errors. Flattening this toResult<(), FinishError>and choosing a single strategy for flush/trailer failures (e.g., log and treat as best‑effort, or add explicitFinishErrorvariants) would make the API easier to use and aligns better with the “always handle Result/Option” guideline, instead of swallowing them withlet _ = ....As per coding guidelines, Results should be handled rather than silently ignored.
Also applies to: 95-98
crates/enc-ffmpeg/src/mux/segmented_audio.rs (1)
145-152: Avoid silently discarding flush/trailer and manifest write errorsIn
rotate_segment,finish,finish_with_timestamp, and the manifest helpers, errors fromencoder.encoder.flush,encoder.output.write_trailer, andstd::fs::writeare either ignored withlet _ = ...or only logged as warnings. That may be acceptable if segment/manifest writing is explicitly best‑effort, but it makes it hard to detect real failures and diverges from the guideline of always handlingResult/Option. Consider at least logging failures in the rotation path and treating manifest write failures as an error (or bubbling them up) when they would make recovery/remux logic unreliable.As per coding guidelines, Results should generally be handled explicitly rather than dropped.
Also applies to: 176-201, 273-301
crates/enc-avfoundation/src/segmented.rs (2)
46-53: Init error mapping hides underlying IO failure details
std::fs::create_dir_all(&base_path)mapping all errors toInitError::NoSettingsAssistantloses the actual IO error context (permission issues, disk full, invalid path, etc.). IfNoSettingsAssistantis meant to be a specific condition rather than “any init failure,” consider introducing a dedicated IO variant (or bubbling thestd::io::Error) so callers and logs can distinguish real filesystem problems from the “no settings assistant” case.
106-137: Do not silently ignorefinishand manifest write failuresIn
rotate_segment, the result ofencoder.finish(Some(timestamp))is discarded, and bothwrite_manifestandfinalize_manifestignorestd::fs::writeerrors. For a feature that underpins crash recovery and remuxing, silently dropping these failures can make issues hard to diagnose and can leave manifests inconsistent with on‑disk fragments. Propagating or at least logging these errors (similar to how other modules log flush/trailer warnings) would make this more robust and better aligned with the “always handle Result/Option” guideline.As per coding guidelines, Results should be handled explicitly rather than dropped.
Also applies to: 144-170, 206-234
apps/desktop/src-tauri/src/windows.rs (1)
592-594: Consider adding logging for failed window handle retrieval.The guarded error handling pattern correctly prevents panics. However, adding a warning log would aid debugging and align with the defensive error handling approach emphasized throughout this PR.
Apply this diff to add diagnostic logging:
let Ok(win) = window.ns_window() else { + tracing::warn!("Failed to get window handle for camera delegate setup"); return; };apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (1)
1128-1144: Consider enhancing recovery failure feedback.The recovery flow correctly implements graceful degradation by falling back to the original path if recovery fails. However, users might benefit from explicit feedback when recovery attempts fail, helping them understand why a recording might not open as expected.
This pattern works well as-is since the
RecoveryToastcomponent likely handles global recovery notifications. If individual recording recovery failures need specific user feedback, consider using a toast notification or dialog to inform the user.apps/desktop/src/components/RecoveryToast.tsx (1)
43-48: Missing error feedback for discard operation.The
discardMutationdoesn't surface errors to the user. If deletion fails, the user won't know why the recording is still showing. Consider adding error handling similar torecoverMutation.const discardMutation = createMutation(() => ({ mutationFn: async (projectPath: string) => { await commands.discardIncompleteRecording(projectPath); await incompleteRecordings.refetch(); }, }));Add a
<Show when={discardMutation.error}>block similar to lines 77-89 to display discard errors.crates/recording/src/output_pipeline/fragmented.rs (1)
157-274: Significant duplication betweenFragmentedAVFoundationMp4MuxerandFragmentedAVFoundationCameraMuxer.These two muxers are nearly identical—only differing in the
VideoFrametype (screen_capture::VideoFramevsNativeCameraFrame). Consider extracting common logic into a generic implementation or shared helper to reduce maintenance burden.apps/desktop/src-tauri/src/recovery.rs (1)
59-66: Double scan of recordings directory is inefficient.
find_incompleteis called infind_incomplete_recordingsto list all incomplete recordings, then called again inrecover_recordingjust to find the same recording by path. Consider caching or passing the recording data from the frontend.crates/recording/src/fragmentation/manifest.rs (1)
31-35: Accept&Pathinstead of&PathBuffor flexibility.Per coding guidelines, function parameters should accept
&Pathinstead of&PathBufto allow callers to pass either type without conversion.- pub fn load_from_file(path: &PathBuf) -> std::io::Result<Self> { + pub fn load_from_file(path: &Path) -> std::io::Result<Self> {Add the import at the top:
-use std::{path::PathBuf, time::Duration}; +use std::{path::{Path, PathBuf}, time::Duration};crates/enc-ffmpeg/src/remux.rs (2)
276-295: Reopening the input file is inefficient but necessary.The file is opened twice: once to get stream parameters for the decoder, and again to iterate packets. This is due to the borrow of
input_streampreventing use ofinputfor packet iteration.Consider refactoring to avoid the double-open by extracting only the necessary data before dropping
input_stream:let input_stream = input .streams() .best(ffmpeg::media::Type::Video) .ok_or_else(|| "No video stream found".to_string())?; - let decoder_ctx = avcodec::Context::from_parameters(input_stream.parameters()) + let parameters = input_stream.parameters(); + let stream_index = input_stream.index(); + drop(input_stream); + + let decoder_ctx = avcodec::Context::from_parameters(parameters) .map_err(|e| format!("Failed to create decoder context: {e}"))?; let mut decoder = decoder_ctx .decoder() .video() .map_err(|e| format!("Failed to create video decoder: {e}"))?; - let stream_index = input_stream.index(); - drop(input_stream); - - let mut input = avformat::input(path).map_err(|e| format!("Failed to reopen file: {e}"))?;This avoids reopening the file entirely.
268-270:stream_copy_fragmentsis a thin wrapper.This function simply delegates to
concatenate_video_fragments. Consider whether this indirection is necessary or if callers should useconcatenate_video_fragmentsdirectly.crates/recording/src/fragmentation/mod.rs (1)
70-94: Consider extracting shared manifest creation logic.
write_manifestandfinalize_manifestshare nearly identical code, differing only in theis_completefield. A private helper could reduce duplication.+ fn create_manifest(&self, is_complete: bool) -> FragmentManifest { + FragmentManifest { + fragments: self.fragments.clone(), + total_duration: self.total_duration(), + is_complete, + } + } + pub fn write_manifest(&self) -> std::io::Result<()> { - let manifest = FragmentManifest { - fragments: self.fragments.clone(), - total_duration: self.total_duration(), - is_complete: false, - }; - + let manifest = self.create_manifest(false); let manifest_path = self.base_path.join("manifest.json"); let json = serde_json::to_string_pretty(&manifest)?; std::fs::write(manifest_path, json)?; Ok(()) } pub fn finalize_manifest(&self) -> std::io::Result<()> { - let manifest = FragmentManifest { - fragments: self.fragments.clone(), - total_duration: self.total_duration(), - is_complete: true, - }; - + let manifest = self.create_manifest(true); let manifest_path = self.base_path.join("manifest.json"); let json = serde_json::to_string_pretty(&manifest)?; std::fs::write(manifest_path, json)?; Ok(()) }apps/desktop/src-tauri/src/recording.rs (1)
1900-1906: Simplify theor_elseclosure.The closure can be simplified since it just returns an Option directly.
let incomplete_recording = incomplete .into_iter() .find(|r| r.project_path == recording_dir) - .or_else(|| { - if let Some(analyzed) = analyze_recording_for_remux(recording_dir, &meta) { - Some(analyzed) - } else { - None - } - }); + .or_else(|| analyze_recording_for_remux(recording_dir, &meta));crates/recording/src/recovery.rs (3)
117-121: Consider using.flatten()for cleaner iteration.The pattern
.filter_map(|e| e.ok())can be simplified to.flatten()which is the idiomatic approach for filtering outErrvariants from an iterator ofResults.- let mut segment_dirs: Vec<_> = std::fs::read_dir(&segments_dir) - .ok()? - .filter_map(|e| e.ok()) - .filter(|e| e.path().is_dir()) - .collect(); + let mut segment_dirs: Vec<_> = std::fs::read_dir(&segments_dir) + .ok()? + .flatten() + .filter(|e| e.path().is_dir()) + .collect();
311-332: Cleanup errors are silently ignored.Multiple
let _ = std::fs::remove_file/remove_dir_all(...)calls suppress errors without logging. While cleanup failures may be non-critical, logging them aids debugging if recovery behaves unexpectedly.As per coding guidelines,
#[must_use]types should be handled.Consider logging cleanup failures:
- let _ = std::fs::remove_dir_all(&display_dir); + if let Err(e) = std::fs::remove_dir_all(&display_dir) { + warn!("Failed to remove display fragments directory: {}", e); + }This pattern should be applied to similar cleanup operations throughout this method (lines 314, 327, 331, 344, 357, 361, 379, 383, 396, 400, 421, 425, 438, 442, 488, 495).
546-554: Camera FPS is hardcoded instead of detected.The camera FPS is hardcoded to 30, while display FPS is correctly detected via
get_video_fps. This could cause playback sync issues if the camera was recorded at a different frame rate.camera: if camera_path.exists() { + let camera_fps = get_video_fps(&camera_path).unwrap_or(30); Some(VideoMeta { path: RelativePathBuf::from(format!("{}/camera.mp4", segment_base)), - fps: 30, + fps: camera_fps, start_time: None, }) } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
.claude/settings.local.json(1 hunks)Cargo.toml(1 hunks)apps/desktop/src-tauri/src/general_settings.rs(2 hunks)apps/desktop/src-tauri/src/lib.rs(4 hunks)apps/desktop/src-tauri/src/platform/macos/delegates.rs(1 hunks)apps/desktop/src-tauri/src/platform/macos/mod.rs(1 hunks)apps/desktop/src-tauri/src/recording.rs(6 hunks)apps/desktop/src-tauri/src/recovery.rs(1 hunks)apps/desktop/src-tauri/src/windows.rs(1 hunks)apps/desktop/src/components/RecoveryToast.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/new-main/index.tsx(3 hunks)apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx(2 hunks)apps/desktop/src/routes/editor/context.ts(1 hunks)apps/desktop/src/utils/tauri.ts(6 hunks)crates/editor/src/editor_instance.rs(7 hunks)crates/enc-avfoundation/Cargo.toml(1 hunks)crates/enc-avfoundation/src/lib.rs(1 hunks)crates/enc-avfoundation/src/segmented.rs(1 hunks)crates/enc-ffmpeg/Cargo.toml(1 hunks)crates/enc-ffmpeg/src/lib.rs(1 hunks)crates/enc-ffmpeg/src/mux/fragmented_audio.rs(1 hunks)crates/enc-ffmpeg/src/mux/mod.rs(1 hunks)crates/enc-ffmpeg/src/mux/segmented_audio.rs(1 hunks)crates/enc-ffmpeg/src/remux.rs(1 hunks)crates/mediafoundation-ffmpeg/src/h264.rs(3 hunks)crates/mediafoundation-ffmpeg/src/lib.rs(1 hunks)crates/project/src/meta.rs(4 hunks)crates/recording/src/capture_pipeline.rs(6 hunks)crates/recording/src/cursor.rs(5 hunks)crates/recording/src/fragmentation/manifest.rs(1 hunks)crates/recording/src/fragmentation/mod.rs(1 hunks)crates/recording/src/lib.rs(1 hunks)crates/recording/src/output_pipeline/ffmpeg.rs(2 hunks)crates/recording/src/output_pipeline/fragmented.rs(1 hunks)crates/recording/src/output_pipeline/mod.rs(1 hunks)crates/recording/src/output_pipeline/win.rs(6 hunks)crates/recording/src/recovery.rs(1 hunks)crates/recording/src/studio_recording.rs(17 hunks)crates/rendering/src/decoder/avassetreader.rs(2 hunks)crates/rendering/src/decoder/ffmpeg.rs(2 hunks)crates/rendering/src/decoder/mod.rs(2 hunks)crates/rendering/src/lib.rs(3 hunks)crates/video-decode/src/avassetreader.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
crates/recording/src/lib.rscrates/enc-ffmpeg/src/mux/mod.rscrates/enc-ffmpeg/src/lib.rscrates/recording/src/output_pipeline/mod.rscrates/mediafoundation-ffmpeg/src/lib.rscrates/enc-avfoundation/src/lib.rsapps/desktop/src-tauri/src/windows.rscrates/rendering/src/decoder/avassetreader.rsapps/desktop/src-tauri/src/platform/macos/mod.rscrates/recording/src/output_pipeline/ffmpeg.rsapps/desktop/src-tauri/src/recovery.rsapps/desktop/src-tauri/src/general_settings.rscrates/rendering/src/lib.rscrates/recording/src/cursor.rscrates/enc-ffmpeg/src/mux/fragmented_audio.rscrates/rendering/src/decoder/ffmpeg.rscrates/video-decode/src/avassetreader.rscrates/enc-ffmpeg/src/mux/segmented_audio.rscrates/recording/src/capture_pipeline.rscrates/recording/src/output_pipeline/win.rscrates/mediafoundation-ffmpeg/src/h264.rsapps/desktop/src-tauri/src/platform/macos/delegates.rsapps/desktop/src-tauri/src/recording.rscrates/editor/src/editor_instance.rscrates/recording/src/output_pipeline/fragmented.rscrates/enc-ffmpeg/src/remux.rscrates/enc-avfoundation/src/segmented.rscrates/recording/src/fragmentation/mod.rscrates/recording/src/fragmentation/manifest.rscrates/rendering/src/decoder/mod.rscrates/recording/src/recovery.rscrates/recording/src/studio_recording.rsapps/desktop/src-tauri/src/lib.rscrates/project/src/meta.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
crates/recording/src/lib.rscrates/enc-ffmpeg/src/mux/mod.rscrates/enc-ffmpeg/src/lib.rsapps/desktop/src/components/RecoveryToast.tsxcrates/recording/src/output_pipeline/mod.rsapps/desktop/src/routes/(window-chrome)/settings/experimental.tsxcrates/mediafoundation-ffmpeg/src/lib.rscrates/enc-avfoundation/src/lib.rsapps/desktop/src-tauri/src/windows.rscrates/rendering/src/decoder/avassetreader.rsapps/desktop/src-tauri/src/platform/macos/mod.rscrates/recording/src/output_pipeline/ffmpeg.rsapps/desktop/src/routes/(window-chrome)/new-main/index.tsxapps/desktop/src-tauri/src/recovery.rsapps/desktop/src-tauri/src/general_settings.rscrates/rendering/src/lib.rscrates/recording/src/cursor.rscrates/enc-ffmpeg/src/mux/fragmented_audio.rscrates/rendering/src/decoder/ffmpeg.rscrates/video-decode/src/avassetreader.rscrates/enc-ffmpeg/src/mux/segmented_audio.rscrates/recording/src/capture_pipeline.rscrates/recording/src/output_pipeline/win.rscrates/mediafoundation-ffmpeg/src/h264.rsapps/desktop/src-tauri/src/platform/macos/delegates.rsapps/desktop/src/utils/tauri.tsapps/desktop/src-tauri/src/recording.rscrates/editor/src/editor_instance.rscrates/recording/src/output_pipeline/fragmented.rscrates/enc-ffmpeg/src/remux.rscrates/enc-avfoundation/src/segmented.rscrates/recording/src/fragmentation/mod.rscrates/recording/src/fragmentation/manifest.rscrates/rendering/src/decoder/mod.rscrates/recording/src/recovery.rsapps/desktop/src/routes/editor/context.tscrates/recording/src/studio_recording.rsapps/desktop/src-tauri/src/lib.rscrates/project/src/meta.rs
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Use React Query hooks with Server Actions for mutations and perform precise cache updates using setQueryData/setQueriesData instead of broad invalidations
Files:
apps/desktop/src/components/RecoveryToast.tsxapps/desktop/src/routes/(window-chrome)/settings/experimental.tsxapps/desktop/src/routes/(window-chrome)/new-main/index.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use strict TypeScript; avoid any type; leverage shared types from @cap/* packages
Follow camelCase naming for variables and functions; PascalCase for components; hooks must start with 'use' prefix
Use Biome for linting and formatting; match existing formatting conventions in the codebase
Use Tailwind CSS for styling in web components; stay consistent with spacing and tokens
Use static skeletons for loading states that mirror content; avoid bouncing animations
Memoize expensive work, code-split naturally, and use Next/Image for remote assets
**/*.{ts,tsx,js,jsx}: Use 2-space indent for TypeScript files; format with Biome usingpnpm format
Use Biome for code formatting and linting; runpnpm formatregularly
Use kebab-case for file names (e.g.,user-menu.tsx); use PascalCase for components
Files:
apps/desktop/src/components/RecoveryToast.tsxapps/desktop/src/routes/(window-chrome)/settings/experimental.tsxapps/desktop/src/routes/(window-chrome)/new-main/index.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/editor/context.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never add any form of comments to code (single-line //, multi-line /* /, JSDoc /* */, or any other comment syntax); code must be self-explanatory through naming, types, and structure
Directory naming must use lowercase-dashed convention
Files:
apps/desktop/src/components/RecoveryToast.tsxapps/desktop/src/routes/(window-chrome)/settings/experimental.tsxapps/desktop/src/routes/(window-chrome)/new-main/index.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/editor/context.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use TanStack Query v5 for all client-side server state and data fetching in TypeScript files
Files:
apps/desktop/src/utils/tauri.tsapps/desktop/src/routes/editor/context.ts
apps/desktop/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.ts: Use @tanstack/solid-query for server state management in SolidJS components
Use generated commands and events from tauri_specta for IPC; never manually construct IPC calls
Listen directly to generated events from tauri_specta and use typed event interfaces
Files:
apps/desktop/src/utils/tauri.tsapps/desktop/src/routes/editor/context.ts
🧠 Learnings (4)
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/recording/src/output_pipeline/mod.rscrates/mediafoundation-ffmpeg/src/lib.rscrates/enc-avfoundation/src/lib.rscrates/rendering/src/decoder/avassetreader.rscrates/recording/src/output_pipeline/ffmpeg.rscrates/enc-ffmpeg/src/mux/fragmented_audio.rscrates/rendering/src/decoder/ffmpeg.rscrates/enc-ffmpeg/src/mux/segmented_audio.rscrates/recording/src/capture_pipeline.rscrates/recording/src/output_pipeline/win.rscrates/mediafoundation-ffmpeg/src/h264.rscrates/recording/src/output_pipeline/fragmented.rscrates/enc-ffmpeg/src/remux.rscrates/enc-avfoundation/src/segmented.rscrates/rendering/src/decoder/mod.rscrates/recording/src/studio_recording.rs
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.rs : Use `rustfmt` and workspace clippy lints for Rust code formatting and linting
Applied to files:
crates/enc-avfoundation/Cargo.toml
📚 Learning: 2025-10-17T05:58:22.586Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1219
File: crates/enc-avfoundation/src/mp4.rs:350-373
Timestamp: 2025-10-17T05:58:22.586Z
Learning: In crates/enc-avfoundation/src/mp4.rs, the `finish()` method intentionally skips video extension when `is_paused` is true. This is correct behavior because if recording is paused, the video should not be extended beyond the pause point—the pause is user-initiated, unlike the case where ScreenCaptureKit stops providing frames during static content.
Applied to files:
crates/rendering/src/decoder/avassetreader.rscrates/enc-ffmpeg/src/mux/fragmented_audio.rscrates/rendering/src/decoder/ffmpeg.rscrates/enc-ffmpeg/src/mux/segmented_audio.rscrates/mediafoundation-ffmpeg/src/h264.rscrates/recording/src/output_pipeline/fragmented.rscrates/enc-avfoundation/src/segmented.rs
📚 Learning: 2025-12-07T14:29:19.165Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.165Z
Learning: Applies to apps/desktop/**/*.ts : Use tanstack/solid-query for server state management in SolidJS components
Applied to files:
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx
🧬 Code graph analysis (19)
apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx (1)
apps/desktop/src/routes/(window-chrome)/settings/Setting.tsx (1)
ToggleSettingItem(24-40)
crates/mediafoundation-ffmpeg/src/lib.rs (1)
crates/mediafoundation-ffmpeg/src/h264.rs (1)
set_fragmented_mp4_options(174-214)
crates/recording/src/output_pipeline/ffmpeg.rs (2)
crates/enc-ffmpeg/src/mux/fragmented_audio.rs (2)
init(33-60)finish(75-92)crates/enc-ffmpeg/src/mux/segmented_audio.rs (2)
init(73-92)finish(204-238)
apps/desktop/src-tauri/src/recovery.rs (1)
crates/recording/src/recovery.rs (2)
find_incomplete(63-100)recover(295-520)
crates/recording/src/cursor.rs (1)
crates/project/src/cursor.rs (1)
moves(209-213)
crates/rendering/src/decoder/ffmpeg.rs (1)
crates/rendering/src/decoder/mod.rs (1)
new(20-26)
crates/video-decode/src/avassetreader.rs (1)
crates/rendering/src/decoder/mod.rs (2)
width(32-34)height(36-38)
crates/enc-ffmpeg/src/mux/segmented_audio.rs (1)
crates/enc-ffmpeg/src/mux/fragmented_audio.rs (4)
init(33-60)encoder(62-64)queue_frame(66-73)finish(75-92)
crates/recording/src/capture_pipeline.rs (3)
crates/recording/src/studio_recording.rs (8)
OutputPipeline(863-866)OutputPipeline(872-875)OutputPipeline(886-889)OutputPipeline(899-902)OutputPipeline(906-909)OutputPipeline(921-924)OutputPipeline(928-931)builder(436-441)crates/recording/src/output_pipeline/core.rs (1)
builder(39-46)crates/recording/src/output_pipeline/win.rs (1)
default(440-446)
apps/desktop/src-tauri/src/recording.rs (2)
crates/project/src/meta.rs (2)
load_for_project(131-137)output_path(170-175)crates/recording/src/recovery.rs (2)
find_incomplete(63-100)recover(295-520)
crates/editor/src/editor_instance.rs (3)
crates/rendering/src/project_recordings.rs (10)
inner(21-45)inner(65-85)inner(120-172)duration(178-180)duration(196-207)new(20-48)new(64-88)new(97-176)Some(197-204)fps(50-52)crates/editor/src/segments.rs (1)
segments(7-36)crates/editor/src/editor.rs (1)
spawn(37-71)
crates/recording/src/output_pipeline/fragmented.rs (1)
crates/enc-avfoundation/src/segmented.rs (1)
init(46-69)
crates/enc-ffmpeg/src/remux.rs (2)
crates/media-info/src/lib.rs (2)
from_decoder(89-101)rate(125-127)crates/enc-ffmpeg/src/mux/fragmented_audio.rs (1)
init(33-60)
crates/enc-avfoundation/src/segmented.rs (2)
crates/enc-ffmpeg/src/mux/segmented_audio.rs (6)
init(73-92)rotate_segment(145-169)current_segment_path(171-174)write_manifest(176-202)finalize_manifest(273-301)completed_segments(303-305)crates/recording/src/fragmentation/mod.rs (5)
new(14-21)current_index(49-51)write_manifest(70-81)finalize_manifest(83-94)total_duration(96-102)
crates/recording/src/fragmentation/mod.rs (1)
crates/recording/src/fragmentation/manifest.rs (1)
complete_fragments(37-39)
apps/desktop/src/routes/editor/context.ts (2)
apps/desktop/src/utils/tauri.ts (1)
commands(7-298)apps/desktop/src/utils/socket.ts (1)
createImageDataWS(4-86)
crates/recording/src/studio_recording.rs (3)
crates/recording/src/output_pipeline/core.rs (6)
builder(39-46)new(307-325)new(732-734)new(775-777)new(808-810)new(852-869)apps/desktop/src-tauri/src/lib.rs (3)
new(797-799)new(1249-1253)new(1820-1850)crates/recording/src/cursor.rs (1)
spawn_cursor_recorder(57-215)
apps/desktop/src-tauri/src/lib.rs (1)
apps/desktop/src-tauri/src/recovery.rs (3)
find_incomplete_recordings(22-48)recover_recording(52-106)discard_incomplete_recording(110-122)
crates/project/src/meta.rs (1)
apps/desktop/src/utils/tauri.ts (2)
Cursors(397-397)CursorMeta(395-395)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (60)
crates/video-decode/src/avassetreader.rs (1)
140-147: LGTM!Simple accessor methods that follow the same pattern used in
crates/rendering/src/decoder/mod.rs. These cleanly expose the dimensions needed for black-frame fallback construction in the rendering decoders.crates/rendering/src/decoder/ffmpeg.rs (2)
84-85: LGTM!Capturing video dimensions upfront enables proper sizing of the black-frame fallback later.
246-263: LGTM!Good fallback behavior for crash recovery scenarios. When no frames can be decoded (e.g., from a corrupted/incomplete recording), the black frame ensures the caller always receives a response rather than hanging indefinitely. The
debug!logging aligns with guidelines.crates/rendering/src/decoder/avassetreader.rs (2)
219-221: LGTM!Correctly uses the new accessor methods from the inner decoder to capture dimensions for the black-frame fallback.
381-398: LGTM!Mirrors the fallback logic in
ffmpeg.rsfor consistency. Usestracing::debug!appropriately per guidelines. This ensures the AVAssetReader decoder path also handles incomplete/corrupted recordings gracefully during crash recovery.crates/recording/src/cursor.rs (1)
95-96: LGTM! Flush logic correctly implements crash recovery.The periodic flush mechanism is well-implemented:
- Flushes accumulated cursor data every 5 seconds during recording
- Performs a final flush when the recorder stops to prevent data loss
- Correctly checks for
output_pathpresence before attempting writesThis enables recovery of incomplete recordings if the app crashes between flush intervals.
Also applies to: 194-199, 204-206
crates/editor/src/editor_instance.rs (5)
5-12: LGTM!The new imports properly support the timeline auto-generation and segment duration features added in this PR.
41-66: LGTM!Good improvements to error handling - returning structured errors instead of panicking, and the zero-segment guard provides a clear error message for recordings that need recovery.
124-147: LGTM!Proper error propagation with descriptive error messages and appropriate trace logging for initialization steps.
277-359: LGTM!The enhanced logging throughout the preview renderer provides good observability for debugging frame rendering issues. The
unwrap()on line 286 is acceptable since the sender is held byselfand won't be dropped while this task runs.
407-491: No changes in this segment.This function appears unchanged from the previous version based on the line annotations.
crates/rendering/src/lib.rs (3)
40-40: Re‑exportingVideofromproject_recordingslooks goodPulling
Videointo the rendering crate’s public surface alongsideProjectRecordingsMetaandSegmentRecordingskeeps the API coherent and should make downstream usage simpler, assuming there are no conflictingVideotypes elsewhere in the crate graph.
210-228:RenderingError::NoSegmentsis a clean, explicit edge‑case for empty recordingsAdding
NoSegmentsmakes the “no segment data available” situation explicit instead of relying on indexing errors or opaque failures. This pairs well with the new guard inRenderVideoConstants::newand should be straightforward for callers to pattern‑match on and surface a user‑friendly error.
371-379:RenderVideoConstants::newnow fails fast on empty segments and derives sizes from the first segmentUsing
segments.first().ok_or(RenderingError::NoSegments)?avoids panics on empty input and clearly signals misconfigured/empty recordings. Derivingscreen_sizeand optionalcamera_sizefromfirst_segmentmatches how the rest of the rendering pipeline consumesoptions, and integrates cleanly with the existingoptions.camera_sizechecks inProjectUniformsandRendererLayers.crates/enc-ffmpeg/Cargo.toml (2)
10-11: LGTM!Standard workspace dependency additions for serde serialization support.
4-4: No action required regardingedition = "2024".The Rust 2024 edition is stable as of February 20, 2025 (Rust 1.85.0) and is fully supported. Using
edition = "2024"is appropriate and recommended for new projects.apps/desktop/src/routes/editor/context.ts (1)
755-763: LGTM - explicit lifecycle handling for initial frame emission.Emitting the initial
renderFrameEventon WebSocket open ensures the frame is sent only when the connection is ready, which is more reliable than a reactive effect.crates/rendering/src/decoder/mod.rs (2)
83-84: LGTM!Good practice capturing the path display string before
pathis moved into the decoder spawn call.
93-100: Good reliability improvement with timeout.Adding a 30-second timeout prevents indefinite hangs during decoder initialization. The error messages include helpful context (decoder name and path). This aligns well with the broader recovery/resilience improvements in this PR.
crates/enc-avfoundation/Cargo.toml (1)
14-17: macOS serde/serde_json workspace deps look correctUsing
serdeandserde_jsonfrom the workspace for the macOS target is consistent with the new segmented/manifest code and the workspace dependency block; no issues here.crates/mediafoundation-ffmpeg/src/h264.rs (1)
13-27: MuxerConfig fragmentation fields and logging look goodAdding
fragmented/frag_duration_uswith a sane default (disabled, 2s) and includingfragmentedin the stream log keeps existing behavior while making fMP4 configuration explicit and observable.Also applies to: 72-79
crates/enc-avfoundation/src/lib.rs (1)
3-7: macOS segmented module wiring looks correctRegistering
mod segmented;and re‑exporting it alongsidemp4under the macOS cfg cleanly exposes the new segmented MP4 encoder without affecting non‑macOS targets.Cargo.toml (1)
32-34: Workspaceserde_jsondependency matches new serialization usageAdding
serde_json = "1"at the workspace level aligns with the new manifest/fragment serialization in segmented/fragmented modules and avoids per‑crate duplicate version specs.crates/recording/src/lib.rs (1)
5-5: LGTM - Clean module additions.The new public modules
fragmentationandrecoveryare properly exposed and integrate well with the broader crash recovery feature.Also applies to: 8-8
apps/desktop/src-tauri/src/general_settings.rs (1)
127-128: LGTM - Consistent field addition.The
crash_recovery_recordingfield is properly declared with#[serde(default)]and correctly initialized tofalsein theDefaultimplementation, following the established pattern for experimental features in this struct.Also applies to: 195-195
crates/enc-ffmpeg/src/mux/mod.rs (1)
1-1: LGTM - Module additions for fragmentation support.The new
fragmented_audioandsegmented_audiomodules are properly exposed to support the fragmented MP4 recording feature.Also applies to: 4-4
crates/enc-ffmpeg/src/lib.rs (1)
12-15: LGTM - Well-structured API expansion.The
remuxmodule andsegmented_audiore-export are properly structured. The re-export pattern provides a cleaner public API by exposingsegmented_audiofunctionality at the crate root level.crates/recording/src/output_pipeline/mod.rs (1)
4-4: LGTM - Proper module integration.The
fragmentedmodule is correctly integrated with appropriate visibility and re-exports for the output pipeline.Also applies to: 9-9
crates/mediafoundation-ffmpeg/src/lib.rs (1)
7-7: LGTM - API expansion for fragmented MP4 support.The
set_fragmented_mp4_optionsfunction is properly exposed to enable configuration of fragmented MP4 recording on Windows.apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (1)
30-30: LGTM - Recovery UI integration.The
RecoveryToastcomponent is properly imported and rendered to provide users with recovery status feedback.Also applies to: 1200-1200
apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx (1)
90-97: LGTM!The crash-recoverable recording toggle follows the established pattern for experimental feature toggles. The description clearly communicates the trade-off (storage usage) to users.
crates/project/src/meta.rs (2)
274-274: LGTM!The
NeedsRemuxvariant is appropriately added toStudioRecordingStatusto support the recovery workflow for fragmented recordings.
121-121: The review comment cannot be substantiated. The current code showsInProgress { recording: bool }as a named struct variant, and git history shows only a formatting commit ("fmt") with no evidence of a structural change from tuple-style to named struct-style. No parent commit exists to verify a format change, and no serialization tests or examples exist in the codebase. The backwards compatibility concern appears to be based on a misreading of the diff.Likely an incorrect or invalid review comment.
crates/recording/src/output_pipeline/win.rs (2)
89-91: LGTM!The fragmentation configuration fields are properly added to
WindowsMuxerConfigand correctly propagated through the encoding pipeline. The implementation is consistent with the camera muxer configuration below.
439-446: LGTM!The
Defaultimplementation provides sensible defaults with fragmentation disabled and a 2-second fragment duration, matching the pattern used elsewhere in the codebase.apps/desktop/src-tauri/src/lib.rs (3)
27-27: LGTM!The recovery module is properly declared and its commands are correctly registered in the specta builder below.
2167-2169: Good improvement: Error propagation forshow_window.Previously errors from
window.show(&app)may have been silently ignored. Now they're properly propagated to the caller, improving debuggability.
3129-3133: LGTM!The debug logging for
RenderFrameEventis appropriate for troubleshooting frame rendering issues without impacting production performance.crates/recording/src/capture_pipeline.rs (2)
75-94: LGTM - Fragmented pipeline wiring for macOS looks correct.The conditional branching cleanly separates fragmented vs non-fragmented paths. The fragments directory is derived from the parent path with a
displaysubdirectory, which aligns with the multi-segment structure expected by the recovery system.
134-154: This asymmetry is intentional and properly handled.macOS uses a directory (
display) becauseSegmentedMP4Encodercreates multiple segment files, while Windows uses a file (display.mp4) because it applies fragmentation within a single MP4 container viaset_fragmented_mp4_options. The recovery code explicitly handles both cases (checking for fragments in thedisplaydirectory first, then falling back todisplay.mp4), indicating this design difference is anticipated and managed appropriately.crates/recording/src/output_pipeline/ffmpeg.rs (2)
160-198: LGTM - FragmentedAudioMuxer implementation follows established patterns.The implementation mirrors the existing
OggMuxerstructure correctly. Error handling is consistent, and the finish/send_audio_frame methods properly delegate to the underlyingFragmentedAudioFile.
200-251: LGTM - SegmentedAudioMuxer with configurable segment duration.The implementation is clean with a reasonable 3-second default segment duration. The
finish_with_timestampcall properly propagates the final timestamp for accurate segment finalization.apps/desktop/src/utils/tauri.ts (2)
288-297: LGTM - New recovery commands properly typed.The auto-generated Tauri commands for
findIncompleteRecordings,recoverRecording, anddiscardIncompleteRecordingcorrectly match the Rust backend signatures. Return types are appropriately typed.
426-426: LGTM -IncompleteRecordingInfotype correctly mirrors Rust definition.The TypeScript type matches the Rust struct with proper camelCase field names as specified by
#[serde(rename_all = "camelCase")].apps/desktop/src-tauri/src/recovery.rs (2)
110-117: Verify path is within expected recordings directory before deletion.
discard_incomplete_recordingaccepts a user-provided path and deletes it withremove_dir_all. While the path originates fromfind_incomplete_recordings, a malicious or buggy frontend could pass arbitrary paths.Consider adding validation:
pub async fn discard_incomplete_recording(project_path: String) -> Result<(), String> { let path = PathBuf::from(&project_path); if !path.exists() { return Err("Recording path does not exist".to_string()); } + let canonical_path = path.canonicalize().map_err(|e| e.to_string())?; + if !canonical_path.components().any(|c| c.as_os_str() == "recordings") { + return Err("Path is not within recordings directory".to_string()); + } + std::fs::remove_dir_all(&path).map_err(|e| e.to_string())?;
84-92: LGTM - Display path extraction handles both single and multiple segment cases.The pattern match correctly handles
SingleSegmentandMultipleSegmentsvariants, and the previous check on line 68 ensuresrecoverable_segmentsis non-empty before reaching this point.crates/recording/src/output_pipeline/fragmented.rs (2)
260-273: Same unbounded retry issue inFragmentedAVFoundationCameraMuxer::send_audio_frame.This has the identical unbounded loop problem. Apply the same bounded retry fix here as well.
⛔ Skipped due to learnings
Learnt from: Brendonovich Repo: CapSoftware/Cap PR: 1305 File: crates/recording/src/output_pipeline/macos.rs:80-90 Timestamp: 2025-10-28T08:39:42.230Z Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
53-56: Theunsafe impl Send + SyncforFragmentedNativeCameraFrameis sound. The wrappedcidre::arc::R<cidre::cm::SampleBuf>is a reference-counted wrapper that safely shares the underlyingCMSampleBufferacross threads—consistent with the identical pattern inNativeCameraFrame(macos.rs:22–23) and Apple's Core Media design, which explicitly supports multi-threaded sample buffer access.crates/recording/src/fragmentation/manifest.rs (1)
77-82: Path serialization uses lossy conversion.
to_string_lossy()replaces invalid UTF-8 sequences with the replacement character. This is typically fine for manifest files where paths are expected to be valid UTF-8, but worth noting for cross-platform compatibility.crates/enc-ffmpeg/src/remux.rs (1)
153-169: DTS adjustment handles timestamp discontinuities correctly.The logic ensures monotonically increasing DTS values across concatenated fragments by tracking the last DTS per stream and applying an offset when a discontinuity is detected. This is essential for proper playback of concatenated fragments.
apps/desktop/src-tauri/src/recording.rs (2)
1381-1396: Fragment remux runs synchronously and may block.The
remux_fragmented_recordingcall happens synchronously during recording finish. For recordings with many fragments, this could cause noticeable delay before the UI responds. Consider running this in a background task if user experience is impacted.
2008-2021: Fragment discovery implementation looks good.The function correctly handles directory read errors, filters by relevant extensions, and sorts for deterministic ordering. The sorting ensures fragments are processed in the expected order.
crates/recording/src/studio_recording.rs (2)
995-1017: In-progress metadata creation for crash recovery.This function creates a placeholder recording metadata file at the start of fragmented recordings. This enables the recovery system to identify incomplete recordings after a crash. The implementation correctly sets the status to
InProgress.
940-968: Cursor output path handling in fragmented mode.The cursor pipeline correctly uses incremental output when fragmented, writing cursor events as they occur rather than only at the end. This ensures cursor data is available for recovery.
crates/recording/src/fragmentation/mod.rs (1)
61-68: No duplicate fragment concern; the API design correctly separates concerns.
rotate()incrementscurrent_indexafter pushing a fragment, transitioning to a new fragment file.mark_current_complete()marks the current fragment complete without incrementing the index. These methods have complementary purposes—rotate()is for rotating to a new file, whilemark_current_complete()is for marking the current file as complete without rotation. The design is intentional and correct.crates/recording/src/recovery.rs (5)
1-60: LGTM!Type definitions and error handling are well-structured. The
RecoveryErrorenum properly usesthiserrorwith appropriate#[from]derives for automatic conversion.
63-100: LGTM!The
find_incompletemethod correctly scans for recordings withInProgress,NeedsRemux, orFailedstatus. The use of pattern matching and early returns keeps the logic clean.
197-245: LGTM!Fragment discovery logic is robust with proper fallback from manifest to probing. The methods correctly filter for valid media files.
600-666: LGTM!Project configuration creation handles edge cases appropriately, including fallback duration estimation and graceful handling when no valid timeline segments exist.
728-750: LGTM!The
discardandmark_needs_remuxmethods properly handle errors and maintain consistent logging. The status update flow inmark_needs_remuxcorrectly mutates and persists the metadata.
| } | ||
|
|
||
| export function RecoveryToast() { | ||
| const [dismissed] = createSignal(false); |
There was a problem hiding this comment.
Unused dismissed signal - dismiss functionality appears incomplete.
The dismissed signal is created but no setter is destructured or used, meaning users cannot dismiss the toast. Either implement dismiss functionality or remove the signal if the toast should always be visible until actioned.
- const [dismissed] = createSignal(false);
+ const [dismissed, setDismissed] = createSignal(false);Then add a dismiss button or auto-dismiss behavior as needed.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/recording/src/cursor.rs (1)
42-54: Address the issues flagged in the previous review.As noted in the previous review comment, this function has two issues:
- Accepts
&PathBufinstead of&Path(violates Clippyptr_argguideline)- Ignores the
Resultfromfs::write, which could silently lose cursor data (violatesmust_useguideline)Please apply the fix suggested in the earlier review.
crates/recording/src/recovery.rs (1)
692-693: Potential panic from chained.unwrap()calls.
path.file_name().unwrap().to_str().unwrap()can panic if the filename contains invalid UTF-8. This was already flagged in a previous review.if path.extension().map(|e| e == "png").unwrap_or(false) && let Some(file_name) = path.file_stem().and_then(|s| s.to_str()) && let Some(id_str) = file_name.strip_prefix("cursor_") { - let relative_path = RelativePathBuf::from("content/cursors") - .join(path.file_name().unwrap().to_str().unwrap()); + let Some(filename_str) = path.file_name().and_then(|f| f.to_str()) else { + continue; + }; + let relative_path = RelativePathBuf::from("content/cursors") + .join(filename_str);
🧹 Nitpick comments (1)
crates/recording/src/recovery.rs (1)
541-546: Hardcoded camera FPS may cause sync issues.Camera FPS is hardcoded to 30, while display FPS is dynamically retrieved. If the camera recorded at a different frame rate (e.g., 24, 25, or 60 fps), this could cause timing/sync issues in the recovered recording.
camera: if camera_path.exists() { + let camera_fps = get_video_fps(&camera_path).unwrap_or(30); Some(VideoMeta { path: RelativePathBuf::from(format!("{}/camera.mp4", segment_base)), - fps: 30, + fps: camera_fps, start_time: None, }) } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/recording/src/cursor.rs(5 hunks)crates/recording/src/recovery.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
crates/recording/src/cursor.rscrates/recording/src/recovery.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
crates/recording/src/cursor.rscrates/recording/src/recovery.rs
🧠 Learnings (4)
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.rs : Accept `&[T]` or `&str` instead of `&Vec<T>` or `&String` in function parameters (Clippy: `ptr_arg` = deny)
Applied to files:
crates/recording/src/cursor.rs
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.rs : Never write `let _ = async_fn()` which silently drops futures; await or explicitly handle them (Clippy: `let_underscore_future` = deny)
Applied to files:
crates/recording/src/cursor.rscrates/recording/src/recovery.rs
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.rs : Use `.unwrap_or(val)` instead of `.unwrap_or_else(|| val)` for cheap values (Clippy: `unnecessary_lazy_evaluations` = deny)
Applied to files:
crates/recording/src/recovery.rs
📚 Learning: 2025-12-07T14:29:19.165Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.165Z
Learning: Applies to **/*.rs : Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Applied to files:
crates/recording/src/recovery.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
crates/recording/src/cursor.rs (4)
64-64: LGTM: output_path parameter addition.The
Option<PathBuf>parameter is appropriate for capturing and moving into the async block.
95-96: LGTM: flush tracking initialization.Proper use of
InstantandDurationfor periodic flushing logic.
194-199: LGTM: periodic flush logic.The let-chain pattern and elapsed time checking are implemented correctly. The periodic flush will work as intended once the issues in
flush_cursor_data(lines 42-54) are resolved.
204-206: LGTM: final flush on completion.Correctly ensures cursor data is flushed when the recording stops. This will work as intended once the issues in
flush_cursor_data(lines 42-54) are resolved.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Bug: Fragmented recording sets Complete status before remux
When a fragmented recording finishes, stop_recording sets status to Complete even though paths point to directories that still need remuxing. This meta is saved in handle_recording_finish at lines 1372-1378 before remux occurs. If the app crashes during remux, the recording has Complete status with directory paths that either don't exist or can't be opened by the editor. The mark_needs_remux function was created but never integrated into the flow. Fragmented recordings need status NeedsRemux until remux completes successfully.
crates/recording/src/studio_recording.rs#L673-L674
Cap/crates/recording/src/studio_recording.rs
Lines 673 to 674 in b953e25
apps/desktop/src-tauri/src/recording.rs#L1371-L1378
Cap/apps/desktop/src-tauri/src/recording.rs
Lines 1371 to 1378 in b953e25
|
|
||
| await commands.showWindow({ | ||
| Editor: { project_path: recording.path }, | ||
| Editor: { project_path: projectPath }, |
There was a problem hiding this comment.
Bug: Recovery failure doesn't prevent opening corrupted recording
When recovering a recording that needs remux fails (lines 1138-1140), the error is logged but execution continues to line 1143 which opens the editor with the original corrupted/incomplete recording path. This could lead to the editor trying to open a malformed recording, causing errors or crashes. The code should either abort or properly handle the recovery failure instead of proceeding with the unrecovered path.
| } | ||
|
|
||
| export function RecoveryToast() { | ||
| const [dismissed] = createSignal(false); |
There was a problem hiding this comment.
Bug: RecoveryToast dismissed signal setter never destructured
The dismissed signal is created with const [dismissed] = createSignal(false) but only the getter is destructured, not the setter. Since there's no setDismissed function available, the toast can never be dismissed—it will always remain visible whenever there are incomplete recordings. The signal should be destructured as const [dismissed, setDismissed] = createSignal(false) with appropriate dismiss handling added.
| info!("Discarded incomplete recording: {}", project_path); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Bug: Discard recording lacks path validation security check
The discard_incomplete_recording function accepts any path string from the frontend and deletes the entire directory after only checking that it exists. Unlike recover_recording which validates the path against the incomplete recordings list, this function performs no validation that the path is within the recordings directory or is actually an incomplete recording. A malicious or buggy frontend could potentially delete any directory the app has write access to.
fmt
| index: self.current_index, | ||
| duration: final_duration, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: Segmented encoder records zero duration for final segment
In the finish method, when timestamp is None, the calculation timestamp.unwrap_or(segment_start).saturating_sub(segment_start) evaluates to segment_start - segment_start = Duration::ZERO. This records an incorrect zero duration for the final segment even when it contains actual content. Unlike SegmentedAudioEncoder which tracks last_frame_timestamp, this encoder has no way to determine the actual final segment duration when the caller doesn't provide a timestamp.
Note
Adds fragmented (fMP4/segmented) studio recording with automatic crash recovery, remuxing, and UI/commands to recover or discard incomplete recordings.
SegmentedMP4Encoder(AVFoundation), FFmpeg segmented/fragmented audio, and Windows muxers with fMP4 options.display/,camera/, audio fragments) and writes in‑progress meta; marks statusNeedsRemuxwhen fragments exist.RecoveryManagerscans.capprojects, concatenates/remuxes fragments, validates media, rebuilds meta/timeline, and cleans up.cap-enc-ffmpeg(concat video/audio, probing helpers).find_incomplete_recordings,recover_recording,discard_incomplete_recording.RecoveryToastsurfaces incomplete recordings with Recover/Discard; auto‑recover when opening items needing remux.crashRecoveryRecordingto enable fragmented recording.show_window).Written by Cursor Bugbot for commit 36ee402. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.