(feat) Customize default project name, update project file names from UUID to pretty name #1372
Conversation
…owsCard + Fix button text spilling
WalkthroughThis pull request introduces comprehensive enhancements to the desktop application including project name templating with format customization, mask and text segment editing in the timeline with visual overlays, a new frame conversion pipeline with hardware acceleration support (VideoToolbox on macOS, D3D11 on Windows, and Swscale as fallback), and expanded rendering capabilities for masks, text, and captions. The recording pipeline is restructured to support native camera frames and asynchronous encoding with converter pooling. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Key areas requiring close attention:
Possibly related PRs
Suggested labels
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: 3
🧹 Nitpick comments (4)
crates/recording/src/sources/screen_capture/mod.rs (1)
197-203: LGTM! Clean implementation.The method correctly returns string representations for all
ScreenCaptureTargetvariants. The implementation is straightforward and appropriate for providing human-readable labels.Optional: Consider adding unit tests.
Since this is a new public method, you may want to add a simple test to verify the string representations:
#[cfg(test)] mod tests { use super::*; #[test] fn test_kind_str() { let display = ScreenCaptureTarget::Display { id: DisplayId::default() }; assert_eq!(display.kind_str(), "Display"); let window = ScreenCaptureTarget::Window { id: WindowId::default() }; assert_eq!(window.kind_str(), "Window"); let area = ScreenCaptureTarget::Area { screen: DisplayId::default(), bounds: LogicalBounds::new(LogicalPosition::new(0.0, 0.0), LogicalSize::new(100.0, 100.0)), }; assert_eq!(area.kind_str(), "Area"); } }apps/desktop/src/routes/(window-chrome)/settings/general.tsx (2)
653-660: Consider relaxing the minimum length constraint.The
isSaveDisabledfunction prevents saving templates withlength <= 3, which might be too restrictive. Valid templates like"{D}"(day of month) have only 3 characters but are legitimate use cases.Consider removing or reducing the minimum length check:
const isSaveDisabled = () => { const input = inputValue(); return ( !input || - input === (props.value ?? DEFAULT_PROJECT_NAME_TEMPLATE) || - input.length <= 3 + input === (props.value ?? DEFAULT_PROJECT_NAME_TEMPLATE) ); };Alternatively, validate that the template contains at least one placeholder instead of checking raw length.
735-741: Consider adding blur animation to Collapsible for smoother transitions.The PR description mentions adding blur to the collapsible animation. The current implementation uses opacity transitions, but you may want to add a blur effect for enhanced visual polish, consistent with the PR objectives.
Add blur to the animation if desired:
-<Collapsible.Content class="opacity-0 transition animate-collapsible-up ui-expanded:animate-collapsible-down ui-expanded:opacity-100 text-xs text-gray-12 space-y-3 px-1 pb-2"> +<Collapsible.Content class="opacity-0 blur-sm transition animate-collapsible-up ui-expanded:animate-collapsible-down ui-expanded:opacity-100 ui-expanded:blur-0 text-xs text-gray-12 space-y-3 px-1 pb-2">crates/utils/src/lib.rs (1)
200-222: Consider expanding test coverage for edge cases.The current test validates basic conversion and the borrowed case, but could benefit from additional test cases to ensure robustness.
Add tests for additional scenarios:
#[test] fn test_moment_format_edge_cases() { // Test overlapping patterns assert_eq!(moment_format_to_chrono("MMMMM"), "%BM"); // Test partial matches assert_eq!(moment_format_to_chrono("YYY"), "YY%y"); // Test all placeholder types assert_eq!(moment_format_to_chrono("hh:mm:ss A"), "%I:%M:%S %p"); assert_eq!(moment_format_to_chrono("HH:mm:ss"), "%H:%M:%S"); // Test no-op with special characters assert_eq!(moment_format_to_chrono("---///"), "---///"); }
📜 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 (13)
Cargo.toml(3 hunks)apps/desktop/src-tauri/Cargo.toml(4 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/recording.rs(6 hunks)apps/desktop/src-tauri/src/update_project_names.rs(1 hunks)apps/desktop/src/routes/(window-chrome)/settings/general.tsx(5 hunks)apps/desktop/src/utils/tauri.ts(2 hunks)crates/recording/src/sources/screen_capture/mod.rs(1 hunks)crates/utils/Cargo.toml(2 hunks)crates/utils/src/lib.rs(2 hunks)packages/ui-solid/src/auto-imports.d.ts(1 hunks)packages/ui-solid/tailwind.config.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/desktop/src/utils/tauri.tspackages/ui-solid/src/auto-imports.d.tsapps/desktop/src/routes/(window-chrome)/settings/general.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/utils/tauri.tspackages/ui-solid/src/auto-imports.d.tspackages/ui-solid/tailwind.config.jsapps/desktop/src/routes/(window-chrome)/settings/general.tsx
**/tauri.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit auto-generated files named
tauri.ts.
Files:
apps/desktop/src/utils/tauri.ts
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/general_settings.rscrates/recording/src/sources/screen_capture/mod.rsapps/desktop/src-tauri/src/lib.rscrates/utils/src/lib.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/update_project_names.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/recording/src/sources/screen_capture/mod.rscrates/utils/src/lib.rs
🧠 Learnings (2)
📚 Learning: 2025-09-22T14:19:56.010Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T14:19:56.010Z
Learning: Applies to **/tauri.ts : Do not edit auto-generated files named `tauri.ts`.
Applied to files:
apps/desktop/src/utils/tauri.ts
📚 Learning: 2025-09-22T14:19:56.010Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T14:19:56.010Z
Learning: Applies to packages/ui/**/*.{ts,tsx,js,jsx} : Component files in `packages/ui` should use PascalCase naming if they define React/Solid components.
Applied to files:
packages/ui-solid/src/auto-imports.d.ts
🧬 Code graph analysis (5)
crates/recording/src/sources/screen_capture/mod.rs (1)
apps/desktop/src/utils/tauri.ts (1)
ScreenCaptureTarget(470-470)
apps/desktop/src-tauri/src/lib.rs (2)
apps/desktop/src-tauri/src/recording.rs (2)
format_project_name(305-378)target_name(183-188)apps/desktop/src-tauri/src/update_project_names.rs (1)
migrate_if_needed(12-37)
apps/desktop/src-tauri/src/recording.rs (2)
crates/utils/src/lib.rs (3)
ensure_dir(16-19)moment_format_to_chrono(166-198)ensure_unique_filename(45-91)apps/desktop/src-tauri/src/lib.rs (12)
std(1388-1406)std(1431-1453)format_project_name(2648-2662)new(349-351)new(766-770)new(1326-1356)app(1169-1170)app(2228-2228)app(2259-2259)app(2320-2320)app(2326-2326)app(2550-2551)
apps/desktop/src-tauri/src/update_project_names.rs (3)
apps/desktop/src-tauri/src/lib.rs (12)
std(1388-1406)std(1431-1453)recordings_path(2604-2608)app(1169-1170)app(2228-2228)app(2259-2259)app(2320-2320)app(2326-2326)app(2550-2551)new(349-351)new(766-770)new(1326-1356)crates/project/src/meta.rs (1)
load_for_project(147-153)crates/utils/src/lib.rs (1)
ensure_unique_filename(45-91)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (3)
apps/desktop/src/utils/tauri.ts (1)
commands(7-295)packages/ui-solid/src/Button.tsx (1)
Button(40-50)apps/desktop/src/routes/editor/ui.tsx (1)
Input(149-159)
🔇 Additional comments (16)
packages/ui-solid/src/auto-imports.d.ts (1)
1-11: This is an auto-generated file—verify it should be committed to version control.Line 5 indicates this file is generated by
unplugin-auto-import. Auto-generated files are typically excluded from version control (via.gitignore) and should not be manually edited. The quote style change on Line 11 likely stems from tool regeneration rather than an intentional code change.Before merging, confirm:
- Whether this file should be committed or added to
.gitignore- If committed, whether the changes come from an updated auto-import configuration or tool version
packages/ui-solid/tailwind.config.js (1)
92-97: LGTM! Blur effect added to collapsible animations.The blur transition enhances the visual feedback during collapse/expand animations. The implementation is consistent across both keyframes.
Note that blur filters can be GPU-intensive. If performance issues arise on lower-end devices with multiple collapsibles animating simultaneously, consider reducing the blur amount or making it optional.
crates/utils/Cargo.toml (1)
10-18: LGTM: Formatting and dependency additions are appropriate.The Windows feature list reformatting improves readability without changing functionality, and the aho-corasick workspace dependency aligns with the new string pattern matching utilities introduced in this crate.
apps/desktop/src-tauri/Cargo.toml (1)
22-28: LGTM: Dependency additions support new project naming features.The added dependencies (regex for pattern matching, sanitize-filename for safe filenames, and aho-corasick for efficient string replacement) are appropriate for the project name formatting functionality introduced in this PR.
Also applies to: 63-63, 110-110, 118-118
apps/desktop/src-tauri/src/general_settings.rs (1)
123-124: LGTM: New setting field is properly integrated.The
default_project_name_templatefield is correctly declared with#[serde(default)], ensuring backward compatibility with existing configurations. TheOption<String>type appropriately represents an optional user preference.Cargo.toml (1)
3-8: LGTM: Workspace configuration changes are well-structured.The multiline workspace members format improves maintainability. Adding "chrono" to specta features enables DateTime serialization for the format_project_name command, and the aho-corasick workspace dependency properly centralizes version management for the new string pattern matching functionality.
Also applies to: 30-30, 49-49
apps/desktop/src-tauri/src/lib.rs (4)
28-28: LGTM: Migration module properly integrated.The
update_project_namesmodule is appropriately added to support the one-time project name migration.
1904-1905: LGTM: New commands properly registered.The
format_project_namecommand is correctly registered in the Tauri command set alongside the existing commands.
2646-2662: LGTM: Command implementation correctly delegates to business logic.The
format_project_namecommand properly delegates to therecording::format_project_namefunction, converting the optional template to a string reference as needed.
2054-2054: No issues found — migration implementation is robust.The migration meets all three verification criteria:
Idempotent: Checked via
STORE_KEYon lines 19-25. If already migrated, the function returns immediately without re-running.Error logging: Migration failures are logged with
tracing::error!()on line 28, while store operation failures propagate with?operator on lines 17 and 33-34.Graceful degradation: Migration errors don't block app startup (lines 27-29 catch and log only). The
STORE_KEYis marked true regardless, preventing retry loops on future startups.The only errors that block startup are store access/save failures, which correctly indicate system-level issues that should prevent app initialization.
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (3)
104-105: LGTM: Default template is well-chosen.The default template
"{target_name} ({target_kind}) {date} {time}"provides a good balance of information and readability for project names.
507-512: LGTM: Component integration is clean.The
DefaultProjectNameCardis properly integrated into the settings with appropriate value binding and change handler.
883-883: LGTM: Layout fix prevents button text overflow.The
flex-shrink-0class properly fixes the layout issue where reset button text would spill out at certain viewport sizes, as mentioned in the PR objectives.crates/utils/src/lib.rs (2)
45-91: LGTM: Well-implemented filename uniqueness helper.The
ensure_unique_filenamefunction properly handles:
- Files without extensions
- Incremental numbering with clear formatting
- Early return when no conflict exists
- Reasonable loop limit (1000) to prevent infinite loops
The implementation is clear and well-documented.
166-198: LGTM: Efficient pattern conversion with good performance characteristics.The
moment_format_to_chronofunction demonstrates excellent design:
- Uses
LazyLockfor efficient static initialization of the automatonLeftmostLongestmatching correctly prioritizes longer patterns (e.g., "MMMM" before "MM")- Returns
Cow::Borrowedwhen no replacements are needed, avoiding unnecessary allocations- Comprehensive documentation with examples
apps/desktop/src/utils/tauri.ts (1)
292-294: DateTime serialization is properly configured.The chrono feature has been correctly added to specta in the workspace Cargo.toml (line 30), enabling proper serialization of
Option<chrono::DateTime<chrono::Local>>to TypeScript'sstring | null. The function implementation in tauri.ts passes the datetime parameter directly through TAURI_INVOKE, allowing specta with the chrono feature to handle serialization automatically. No additional verification or testing is required—the configuration is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src-tauri/src/recording.rs (1)
263-324: The{target}placeholder concern appears addressed.The past review comment flagged that
{target}was mentioned in documentation but not implemented. The current documentation and example (line 290) correctly use{target_kind}and{target_name}as separate placeholders, which are implemented in the AhoCorasick patterns (lines 317-320). This appears to resolve the previous concern.
🧹 Nitpick comments (2)
apps/desktop/src-tauri/src/recording.rs (2)
24-24: Consider usingstd::sync::LazyLockfor consistency.The codebase already uses
LazyLockinmoment_format_to_chrono(from the relevant code snippets). For consistency and to use the more modern standard library feature (stable since Rust 1.80), consider replacinglazy_static!withLazyLockin theformat_project_namefunction.
405-406: Consider removing redundant colon replacement.The
sanitize_filename::sanitize()call on line 406 should already handle filesystem-unsafe characters including colons. The explicit.replace(":", ".")on line 405 appears redundant, though it's harmless.If you prefer to keep the explicit replacement for clarity, this is fine. Otherwise, you can simplify to:
- let filename = project_name.replace(":", "."); - let filename = format!("{}.cap", sanitize_filename::sanitize(&filename)); + let filename = format!("{}.cap", sanitize_filename::sanitize(&project_name));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src-tauri/src/recording.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/recording.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/recording.rs (2)
crates/utils/src/lib.rs (3)
ensure_dir(16-19)moment_format_to_chrono(166-198)ensure_unique_filename(45-91)apps/desktop/src-tauri/src/lib.rs (12)
std(1388-1406)std(1431-1453)format_project_name(2648-2662)new(349-351)new(766-770)new(1326-1356)app(1169-1170)app(2228-2228)app(2259-2259)app(2320-2320)app(2326-2326)app(2550-2551)
⏰ 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). (1)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (1)
apps/desktop/src-tauri/src/recording.rs (1)
391-413: LGTM! Clean integration of template-based project naming.The integration of
format_project_namethroughoutstart_recordingis well-executed:
- Retrieves the template from settings
- Generates a sanitized, unique recording directory
- Propagates the project name to metadata and upload info
Note: The
target_namefield inInProgressRecordingCommon(line 590) now holds the computedproject_namerather than just the capture target's name, which extends its semantics while maintaining backward compatibility.Also applies to: 435-435, 476-476, 590-590
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/recording.rs (1)
263-377: Consider usingLazyLockfor consistency and adding unit tests.The function correctly implements template formatting with multiple placeholder types. Two improvements to consider:
- Consistency with utils crate: The
cap_utilscrate usesstd::sync::LazyLock(seemoment_format_to_chrono), but this function useslazy_static!. For consistency, consider migrating toLazyLock:-use lazy_static::lazy_static; +use std::sync::LazyLock; - lazy_static! { - static ref DATE_REGEX: Regex = Regex::new(r"\{date(?::([^}]+))?\}").unwrap(); - static ref TIME_REGEX: Regex = Regex::new(r"\{time(?::([^}]+))?\}").unwrap(); - static ref MOMENT_REGEX: Regex = Regex::new(r"\{moment(?::([^}]+))?\}").unwrap(); - static ref AC: aho_corasick::AhoCorasick = { + static DATE_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\{date(?::([^}]+))?\}").unwrap()); + static TIME_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\{time(?::([^}]+))?\}").unwrap()); + static MOMENT_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\{moment(?::([^}]+))?\}").unwrap()); + static AC: LazyLock<aho_corasick::AhoCorasick> = LazyLock::new(|| { aho_corasick::AhoCorasick::new(&[ "{recording_mode}", "{mode}", "{target_kind}", "{target_name}", ]) .expect("Failed to build AhoCorasick automaton") - }; - } + });
- Unit test coverage: This function has multiple code paths (different placeholders, custom format specifiers, edge cases). Consider adding unit tests in the
testsmodule to verify behavior with various templates, format specifiers, and edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src-tauri/src/recording.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/recording.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/recording.rs (1)
crates/utils/src/lib.rs (3)
ensure_dir(16-19)moment_format_to_chrono(166-198)ensure_unique_filename(45-91)
🔇 Additional comments (2)
apps/desktop/src-tauri/src/recording.rs (2)
22-28: LGTM! Imports are appropriate for the new template formatting functionality.The new imports support the template parsing and formatting infrastructure.
394-416: LGTM! Project name formatting and sanitization flow is well-structured.The integration correctly formats the project name from the template, sanitizes it for filesystem compatibility, and ensures uniqueness. The explicit colon-to-period replacement before sanitization adds clarity even though
sanitize_filenamemight handle it.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/desktop/src-tauri/Cargo.toml (1)
114-114: Clarify the commented feature syntax on line 114.Line 114 has trailing commented-out code that appears to be incomplete feature specifications. This may confuse future maintainers. Consider either removing this comment or clarifying its intent.
-opentelemetry-otlp = "0.31.0" #{ version = , features = ["http-proto", "reqwest-client"] } +opentelemetry-otlp = "0.31.0"apps/desktop/src-tauri/src/recording.rs (3)
24-24: Consider usingstd::sync::LazyLockfor consistency.The
lazy_staticcrate is used here, but thecap_utilscrate usesstd::sync::LazyLock(stable since Rust 1.80). UsingLazyLockwould improve consistency across the codebase and reduce external dependencies.Apply this diff to use
LazyLock:-use lazy_static::lazy_static; +use std::sync::LazyLock;Then update the lazy initialization in
format_project_name(around line 314):- lazy_static! { - static ref DATE_REGEX: Regex = Regex::new(r"\{date(?::([^}]+))?\}").unwrap(); - static ref TIME_REGEX: Regex = Regex::new(r"\{time(?::([^}]+))?\}").unwrap(); - static ref MOMENT_REGEX: Regex = Regex::new(r"\{moment(?::([^}]+))?\}").unwrap(); - static ref AC: aho_corasick::AhoCorasick = { + static DATE_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\{date(?::([^}]+))?\}").unwrap()); + static TIME_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\{time(?::([^}]+))?\}").unwrap()); + static MOMENT_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\{moment(?::([^}]+))?\}").unwrap()); + static AC: LazyLock<aho_corasick::AhoCorasick> = LazyLock::new(|| { aho_corasick::AhoCorasick::new(&[ "{recording_mode}", "{mode}", "{target_kind}", "{target_name}", ]) .expect("Failed to build AhoCorasick automaton") - }; - } + });Based on learnings
263-377: Consider adding unit tests forformat_project_name.This is a core public function exposed as a Tauri command with complex formatting logic involving multiple token replacements. Unit tests would help ensure correctness and prevent regressions, especially for edge cases like:
- Empty or missing template
- Custom date/time formats with moment syntax
- All placeholder combinations
- Special characters in target names
Example test structure:
#[cfg(test)] mod format_project_name_tests { use super::*; use chrono::TimeZone; #[test] fn test_default_template() { let dt = chrono::Local.with_ymd_and_hms(2025, 11, 12, 15, 23, 0).unwrap(); let result = format_project_name( None, "Built-in Retina Display", "Display", RecordingMode::Instant, Some(dt), ); assert!(result.contains("Built-in Retina Display")); assert!(result.contains("Display")); assert!(result.contains("2025-11-12")); } #[test] fn test_custom_moment_format() { let dt = chrono::Local.with_ymd_and_hms(2025, 11, 12, 15, 23, 0).unwrap(); let result = format_project_name( Some("{moment:YYYY-MM-DD HH:mm}"), "Test", "Window", RecordingMode::Studio, Some(dt), ); assert!(result.contains("2025-11-12 15:23")); } #[test] fn test_all_placeholders() { let result = format_project_name( Some("{recording_mode} {mode} {target_kind} {target_name}"), "Chrome", "Window", RecordingMode::Studio, None, ); assert_eq!(result, "Studio studio Window Chrome"); } }
593-593: Consider renamingtarget_namefield toproject_namefor clarity.The
target_namefield inInProgressRecordingCommonis now populated with the formattedproject_namerather than the raw capture target name. This semantic mismatch could confuse future maintainers. While this works functionally, renaming the field toproject_namethroughout the structs (InProgressRecordingCommon,CompletedRecording) would better reflect its actual purpose.Note: This would be a broader refactor touching multiple files and is not urgent.
📜 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 (3)
apps/desktop/src-tauri/Cargo.toml(4 hunks)apps/desktop/src-tauri/src/recording.rs(6 hunks)apps/desktop/src/utils/tauri.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/utils/tauri.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/recording.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/recording.rs (1)
crates/utils/src/lib.rs (3)
ensure_dir(16-19)moment_format_to_chrono(166-198)ensure_unique_filename(45-91)
⏰ 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). (1)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (2)
apps/desktop/src-tauri/Cargo.toml (1)
63-63: All three new dependencies are properly configured and verified.The workspace dependency
aho-corasick = "1.1.4"is correctly defined in the root Cargo.toml (line 49), and the reference at line 118 (aho-corasick.workspace = true) is valid. All three new dependencies are stable, current, and free of known security issues:
regex = "1.10.4"— Well above the CVE-2022-24713 fix threshold (1.5.5)sanitize-filename = "0.6.0"— Latest stable versionaho-corasick— Properly defined in workspace and compatibleThe feature formatting changes are non-functional.
apps/desktop/src-tauri/src/recording.rs (1)
394-416: LGTM! Clean integration of project name formatting.The workflow correctly:
- Formats the project name using the template
- Sanitizes colons and filesystem-unsafe characters
- Ensures uniqueness in the recordings directory
- Creates the directory
The colon replacement (line 408) before sanitization is a good defensive measure for cross-platform compatibility.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)
105-107: Keep TS and Rust default templates in sync
DEFAULT_PROJECT_NAME_TEMPLATEmirrors the RustDEFAULT_FILENAME_TEMPLATEinrecording::format_project_name. To avoid future drift, consider adding a brief comment referencing the Rust constant (or a test that asserts both defaults format the same example) so changes on one side don’t silently desync.apps/desktop/src-tauri/src/recording.rs (1)
720-724: Field nametarget_namenow holds project name
InProgressRecordingCommon { target_name: project_name, .. }repurposes thetarget_namefield to actually carry the formatted project name. That’s fine behaviorally (the field is only used to plumb a display name throughCompletedRecording), but the naming is now misleading.If you touch this area again, consider renaming the field (and
CompletedRecording::target_name) to something likeproject_nameto better reflect its semantics; no urgent change needed.
📜 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 (7)
apps/desktop/src-tauri/Cargo.toml(4 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/recording.rs(7 hunks)apps/desktop/src/routes/(window-chrome)/settings/general.tsx(5 hunks)apps/desktop/src/utils/tauri.ts(2 hunks)packages/ui-solid/src/auto-imports.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src-tauri/src/general_settings.rs
- apps/desktop/src-tauri/Cargo.toml
- apps/desktop/src/utils/tauri.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/desktop/src-tauri/src/recording.rs (2)
crates/utils/src/lib.rs (3)
ensure_dir(16-19)moment_format_to_chrono(166-198)ensure_unique_filename(45-91)apps/desktop/src-tauri/src/lib.rs (12)
std(1785-1803)std(1828-1850)format_project_name(3060-3074)new(730-732)new(1163-1167)new(1723-1753)app(1566-1567)app(2629-2629)app(2660-2660)app(2725-2725)app(2731-2731)app(2962-2963)
apps/desktop/src-tauri/src/lib.rs (2)
apps/desktop/src-tauri/src/recording.rs (2)
format_project_name(396-469)target_name(266-271)apps/desktop/src-tauri/src/update_project_names.rs (1)
migrate_if_needed(12-37)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (3)
apps/desktop/src/utils/tauri.ts (1)
commands(7-298)packages/ui-solid/src/Button.tsx (1)
Button(40-50)apps/desktop/src/routes/editor/ui.tsx (1)
Input(149-159)
⏰ 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). (1)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (6)
packages/ui-solid/src/auto-imports.d.ts (1)
11-11: LGTM! Auto-generated icon import follows the correct pattern.The new
IconCapAutodeclaration is correctly formatted and alphabetically ordered among the other icon imports. Since this file is auto-generated byunplugin-auto-import, the addition appears consistent with the tooling's expected output.apps/desktop/src/routes/(window-chrome)/settings/general.tsx (3)
9-22: New imports and hooks look consistent with usage
Collapsible,createSignal, andonMountare all used inDefaultProjectNameCard; nothing appears unused or mis-imported. No changes needed here.
774-835: Collapsible “How to customize?” section aligns with backend placeholdersThe placeholder descriptions ({recording_mode}, {mode}, {target_kind}, {target_name}, {date}, {time},
{moment:…}) match the Rust implementation offormat_project_nameand themoment_format_to_chronobehavior. This should give users an accurate mental model of what’s supported.
922-944: Excluded Windows header layout fix is correctAdding
flex-shrink-0to the action container prevents the reset/add buttons from compressing or wrapping in narrow viewports, which addresses the earlier text spill without side effects elsewhere in the card.apps/desktop/src-tauri/src/lib.rs (1)
2302-2304:format_project_nameTauri command wiring looks correctThe new
format_project_namecommand is registered in the specta builder and cleanly delegates torecording::format_project_name, convertingOption<String>/Stringarguments into theOption<&str>/&strexpected by the core function. This should interoperate correctly with the generated TS bindings and the frontend calls inutils/tauri.ts.Also applies to: 3058-3074
apps/desktop/src-tauri/src/recording.rs (1)
486-511: New project-name templating and filename derivation look correctThe new
start_recordingflow:
- Derives
project_nameviaformat_project_name, usingdefault_project_name_templatewhen present and falling back to the hard-coded default.- Builds a filesystem-safe, unique directory name by:
- Replacing
:with.,- Passing through
sanitize_filename,- Appending
.cap,- Running
ensure_unique_filenameunderapp_data_dir()/recordings.This cleanly separates human-readable naming from the underlying folder layout and avoids collisions. The subsequent
ensure_dir(&recording_dir)+RecordingMeta { pretty_name: project_name.clone(), .. }wiring is consistent with downstream uses (uploads, editor, listing).
| use cap_utils::{ensure_dir, moment_format_to_chrono, spawn_actor}; | ||
| use futures::{FutureExt, stream}; | ||
| use lazy_static::lazy_static; | ||
| use regex::Regex; | ||
| use serde::{Deserialize, Serialize}; | ||
| use specta::Type; | ||
| use std::borrow::Cow; | ||
| use std::{ |
There was a problem hiding this comment.
format_project_name implementation is solid; fix minor time-format doc mismatch
The new format_project_name helper is well-structured:
- Uses a pre-built Aho-Corasick automaton for
{recording_mode},{mode},{target_kind},{target_name}replacements. - Supports
{date},{time}, and{moment}with optional custom moment-style formats, mapping them viamoment_format_to_chronobefore passing tochrono::DateTime::format. - Keeps a sensible default template (
{target_name} ({target_kind}) {date} {time}) that matches the TSDEFAULT_PROJECT_NAME_TEMPLATE.
One minor nit: the default time format in code is %I:%M %p (Line 451), which produces zero-padded hours (e.g., "03:23 PM"), but the doc comment and UI examples show "3:23 PM". To avoid confusion either:
- Change the default to
%-I:%M %pto match the example, or - Adjust the example/docs to show a zero-padded hour.
Functionally everything else looks correct.
Also applies to: 355-469
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/recording.rs around lines 355-469 (imports are at
top around 26-33), the default time format uses "%I:%M %p" which zero-pads hours
(e.g., "03:23 PM") but the doc comment and UI examples show a non-padded hour
("3:23 PM"); change the default time format string to "%-I:%M %p" to produce
non-padded hours and update the nearby doc comment/example if present so they
match the new default.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/desktop/src-tauri/src/update_project_names.rs (2)
27-34: Critical: Migration flag still set on failure (past issue remains)The past review comment correctly identified that lines 27-34 mark the migration as complete even when
migratereturns an error. This means:
- If migration fails for any reason (permissions, conflicts, etc.), the flag is still set
- Future app launches will never retry the migration
- Users will permanently have UUID-based folders
100-138: Major: Partial failures still swallowed (past issue remains)The past review correctly identified that this function returns
Ok(())even whenfailed > 0. Combined with the flag-setting issue inmigrate_if_needed, a single failure permanently blocks future migration attempts.The detailed performance logging (wall time, averages, top slowest projects) is excellent for observability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src-tauri/src/update_project_names.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/update_project_names.rs (3)
apps/desktop/src-tauri/src/lib.rs (12)
std(1785-1803)std(1828-1850)recordings_path(3016-3020)app(1566-1567)app(2629-2629)app(2660-2660)app(2725-2725)app(2731-2731)app(2962-2963)new(730-732)new(1163-1167)new(1723-1753)crates/project/src/meta.rs (1)
load_for_project(147-153)crates/utils/src/lib.rs (1)
ensure_unique_filename(45-91)
⏰ 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). (1)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (5)
apps/desktop/src-tauri/src/update_project_names.rs (5)
141-167: LGTM: UUID project collection logic is soundThe async directory enumeration correctly filters for UUID-formatted
.capdirectories using the validation helper. Error propagation is appropriate.
175-204: LGTM: Per-project migration logic handles errors appropriatelyThe function properly loads metadata, migrates the filename, and returns appropriate results (Migrated/Skipped) based on whether the path changed. Error logging includes context.
211-211: Verify the colon pre-replacement before sanitizationThe code replaces colons with periods before calling
sanitize_filename::sanitize. This enforces a specific formatting choice (e.g., timestamps like "2024-01-01 12:00:00" become "2024-01-01 12.00.00").Since
sanitize_filenamewould handle colons automatically (possibly differently per platform), verify whether this pre-replacement is:
- Intentional to maintain consistent readability across platforms
- Redundant and could be removed
- Incorrect and should use sanitize's default handling
Consider requesting clarification from the PR author or checking if there's a design document explaining the naming format.
235-251: LGTM: UUID validation is correct and efficientThe function correctly validates:
- Overall length (36 for UUID + 4 for ".cap")
- Hyphen positions at indices 8, 13, 18, 23 (standard UUID format: 8-4-4-4-12)
- All characters are ASCII hex digits or hyphens
The name
fast_is_project_filename_uuidaccurately reflects the optimized approach (byte-level checks before character iteration).
253-279: LGTM: Test coverage validates key scenariosThe tests cover:
- Valid UUID filenames (varied hex, all-zeros edge case)
- Invalid formats (non-UUID string, wrong/missing extension, invalid hex character)
Coverage is adequate for the validation logic. Optional enhancements could include wrong hyphen positions or length variations, but current tests are sufficient.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/utils/src/lib.rs (2)
45-100: Clarify failure behavior and validateattemptsinensure_unique_filename*_with_attempts.The core logic looks solid, but a couple of edge cases are underspecified:
ensure_unique_filename_with_attemptsaccepts anyi32forattempts. Forattempts <= 0, the loop will error immediately (after at most one probe) with"Too many filename conflicts", even when there are no real conflicts.- The doc comment on
ensure_unique_filenamepromises “Returns the unique filename that doesn't conflict with existing files” but doesn’t mention that the function can fail when the suffix limit is exceeded.Consider tightening this by:
- Validating
attemptsat the top (e.g.,debug_assert!(attempts > 0)or returning an explicit error ifattempts <= 0), and- Updating the docs to describe the error case (and that
attemptsbounds the highest numeric suffix tried).Optionally, if
base_filenameis always expected to be a bare file name (no separators), adebug_assert!(!base_filename.contains(std::path::MAIN_SEPARATOR))would encode that assumption.
209-231: Extend tests to cover documented edge cases (e.g., ISO week and invalid/unknown tokens).The current test nicely exercises a mixed format and the borrowed/unchanged path. Given the richer doc examples, you might want to add:
- A test for the ISO week example (once the implementation or docs are corrected) so the mismatch can’t regress silently.
- One or two tests around “near misses” (e.g., strings containing partial tokens or literals adjacent to tokens) to lock in the LeftmostLongest behavior.
This would make the behavior of
moment_format_to_chronomuch clearer and future refactors safer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/update_project_names.rs(1 hunks)crates/utils/src/lib.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src-tauri/src/update_project_names.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). (1)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/utils/src/lib.rs (1)
114-182: Address moment.js compatibility confusion flagged in previous review.The previous review flagged that this function claims to convert "moment template format" but uses incompatible token semantics. This concern was not fully addressed:
- Moment.js convention:
DDD/DDDD= day-of-year (1-365, 001-365);ddd/dddd= weekday names- This implementation:
DDD/DDDD= weekday names; day-of-year tokens are unsupportedThe current documentation still refers to "moment template format" (line 114) without clarifying this is a custom subset with different semantics. Users expecting moment.js compatibility will encounter unexpected behavior.
Based on previous review comments, either:
- Update the function description and docs to explicitly state this is a "moment-style subset" with a clear notice about the divergence:
-/// Converts user-friendly moment template format strings to chrono format strings. +/// Converts moment-style template format strings to chrono format strings. /// -/// This function translates common template format patterns to chrono format specifiers, -/// allowing users to write intuitive date/time formats that get converted to chrono's format. +/// This function translates a custom subset of date/time patterns to chrono format specifiers. +/// +/// **Note**: This is NOT fully compatible with moment.js. Notably, `DDD`/`DDDD` map to +/// weekday names here, whereas in moment.js they represent day-of-year. Day-of-year and +/// ISO week tokens are not supported.
- Or realign the tokens to match moment.js if compatibility is important (add
ddd/ddddfor weekdays, repurposeDDD/DDDDfor day-of-year).apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)
9-9: Default project name card wiring looks solid; consider debouncing preview updatesThe new
DefaultProjectNameCardis well-integrated: theDEFAULT_PROJECT_NAME_TEMPLATEfallback,value/onChangewiring intodefaultProjectNameTemplate, Save/Reset behavior, and the help section with copyable placeholders all read correctly, and the Save handler now correctly awaits persistence and preview updates.One remaining improvement is avoiding a Tauri round-trip on every keystroke in
onInputviaupdatePreview(e.currentTarget.value). A small trailing-edge debounce (e.g., 200–300ms) would reduce redundantformatProjectNamecalls for fast typers and also mitigate the chance of a slower earlier call overwriting a newer preview.For example:
function DefaultProjectNameCard(props: { @@ - const [momentExample, setMomentExample] = createSignal(""); + const [momentExample, setMomentExample] = createSignal(""); + let previewTimeoutId: number | undefined; @@ - async function updatePreview(val = inputValue()) { + async function updatePreview(val = inputValue()) { const formatted = await commands.formatProjectName( val, @@ setPreview(formatted); } + + function schedulePreviewUpdate(val: string) { + if (previewTimeoutId !== undefined) { + clearTimeout(previewTimeoutId); + } + previewTimeoutId = window.setTimeout(() => { + // Best-effort; log if something goes wrong instead of throwing in the UI event loop. + void updatePreview(val).catch((error) => { + console.error("Failed to update project name preview", error); + }); + }, 250); + } @@ - onInput={(e) => { - setInputValue(e.currentTarget.value); - updatePreview(e.currentTarget.value); - }} + onInput={(e) => { + const next = e.currentTarget.value; + setInputValue(next); + schedulePreviewUpdate(next); + }}This keeps the explicit
await updatePreview(...)calls for Reset/Save semantics while making the live preview cheaper and more robust during typing.Also applies to: 105-107, 546-551, 638-838
🧹 Nitpick comments (1)
crates/utils/src/lib.rs (1)
217-239: Consider expanding test coverage.The current tests only cover
moment_format_to_chronowith basic scenarios. Consider adding:
- Tests for
ensure_unique_filenameandensure_unique_filename_with_attemptsto verify:
- Behavior when file doesn't exist (returns base filename)
- Conflict resolution (appending numbers)
- Extension handling (files with/without extensions)
- Max attempts limit enforcement
- Additional
moment_format_to_chronotests for:
- Overlapping pattern handling (e.g., "MMMM" vs "MMM" precedence)
- Mixed token and literal text
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src-tauri/src/update_project_names.rs(1 hunks)apps/desktop/src/routes/(window-chrome)/settings/general.tsx(5 hunks)crates/utils/src/lib.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src-tauri/src/update_project_names.rs
🧰 Additional context used
🧬 Code graph analysis (1)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (3)
apps/desktop/src/utils/tauri.ts (1)
commands(7-298)packages/ui-solid/src/Button.tsx (1)
Button(40-50)apps/desktop/src/routes/editor/ui.tsx (1)
Input(149-159)
⏰ 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). (1)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (2)
crates/utils/src/lib.rs (1)
153-153: No action needed — chrono's%Pspecifier is cross-platform compatible.The web search confirms that Chrono implements the
%Pformat specifier natively (not delegated to platform libc), ensuring consistent lowercase "am"/"pm" output across Windows, macOS, and Linux. The documentation mapping of lowercaseato%Pis correct and safe.apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)
922-944: Excluded windows action row layout tweak is safe and improves resilienceChanging the action container to
class="flex flex-shrink-0 gap-2"is a low-risk layout fix: it keeps the Reset/Add buttons from shrinking and prevents the "Reset to Default" label from wrapping or clipping at narrower widths, without altering any behavior or event handling.
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/recording/src/screenshot.rs (1)
148-188: Remove debug logging statements per coding guidelines.The code contains three
tracing::info!()logging statements (lines 155-168, 175-181) that violate the guideline prohibiting comments and documentation in code. These must be removed to ensure code is self-explanatory through naming, types, and structure rather than relying on logging for understanding.Additionally, verify that the CGRect construction behavior on high-DPI displays is correct. The coordinate space transition from the previous version should be validated across different display scale factors.
apps/desktop/src/routes/editor/CaptionsTab.tsx (1)
305-320: Event listener may leak if download fails.If
commands.downloadWhisperModelthrows,unlisten()is never called, leaving the event listener active. Move the cleanup to thefinallyblock.+ let unlisten: (() => void) | undefined; try { - const unlisten = await events.downloadProgress.listen((event) => { + unlisten = await events.downloadProgress.listen((event) => { setDownloadProgress(event.payload.progress); }); await commands.downloadWhisperModel(modelToDownload, modelPath); - unlisten(); setDownloadedModels((prev) => [...prev, modelToDownload]); toast.success("Transcription model downloaded successfully!"); } catch (error) { console.error("Error downloading model:", error); toast.error("Failed to download transcription model"); } finally { + unlisten?.(); setIsDownloading(false); setDownloadingModel(null); }apps/desktop/src/store/captions.ts (1)
101-118: localStorage overwrites command data rather than serving as fallback.The comment says "Try loading from localStorage as backup" but the code unconditionally overwrites segments/settings loaded from
commands.loadCaptions. If this is intentional (local edits take precedence), the comment should be updated. If localStorage should only apply when the command returns no data, the logic needs adjustment.Consider restructuring to match the "backup" intent:
- // Try loading from localStorage as backup try { + // Use localStorage as backup if no segments loaded from command + if (!state.segments.length) { const localCaptionsData = JSON.parse( localStorage.getItem(`captions-${videoPath}`) || "{}", ); if (localCaptionsData.segments) { setState("segments", localCaptionsData.segments); } if (localCaptionsData.settings) { setState("settings", { ...defaultCaptionSettings, ...localCaptionsData.settings, }); } + } } catch (e) {
♻️ Duplicate comments (1)
apps/desktop/src-tauri/src/lib.rs (1)
2534-2538: Migration no longer blocks app startupCalling
update_project_names::migrate_if_needed(&app)insidesetupnow logs errors instead of propagating them, so project‑name migration failures won’t prevent the UI from starting. This matches the intended behavior for a cosmetic, one‑time migration.
🧹 Nitpick comments (45)
crates/export/src/mp4.rs (2)
209-219: Consider logging warnings for silent failures inside the blocking task.The silent early returns and ignored save result make debugging difficult when screenshot generation fails. While these are non-fatal, adding warning logs would be consistent with the error handling at line 223 and aid troubleshooting.
let Some(rgb_img) = rgb_img else { + warn!("Failed to create RGB image buffer for screenshot"); return; }; let screenshots_dir = project_path.join("screenshots"); if std::fs::create_dir_all(&screenshots_dir).is_err() { + warn!("Failed to create screenshots directory"); return; } let screenshot_path = screenshots_dir.join("display.jpg"); - let _ = rgb_img.save(&screenshot_path); + if let Err(e) = rgb_img.save(&screenshot_path) { + warn!("Failed to save screenshot: {e}"); + }
193-193: Redundant clone ofproject_path.Since
project_pathis not used elsewhere in this async block after this point, the clone is unnecessary—it can be moved directly into the blocking task closure.if let Some(frame) = first_frame { - let project_path = project_path.clone(); let screenshot_task = tokio::task::spawn_blocking(move || {crates/editor/src/editor.rs (1)
90-117: Simplify withif letinstead ofOption::takefollowed bylet Some.The pattern of
take()then matching can be streamlined. Also, lines 115-117 are unreachable sinceframe_to_renderis alwaysSomewhenpending_framewasSomeor a message was received.- let frame_to_render = if let Some(pending) = pending_frame.take() { - Some(pending) - } else { - match self.rx.recv().await { - Some(RendererMessage::RenderFrame { - segment_frames, - uniforms, - finished, - cursor, - frame_number, - }) => Some(PendingFrame { - segment_frames, - uniforms, - finished, - cursor, - frame_number, - }), - Some(RendererMessage::Stop { finished }) => { - let _ = finished.send(()); - return; - } - None => return, - } - }; - - let Some(mut current) = frame_to_render else { - continue; - }; + let mut current = if let Some(pending) = pending_frame.take() { + pending + } else { + match self.rx.recv().await { + Some(RendererMessage::RenderFrame { + segment_frames, + uniforms, + finished, + cursor, + frame_number, + }) => PendingFrame { + segment_frames, + uniforms, + finished, + cursor, + frame_number, + }, + Some(RendererMessage::Stop { finished }) => { + let _ = finished.send(()); + return; + } + None => return, + } + };crates/rendering/src/decoder/avassetreader.rs (1)
262-266: Reset threshold logic looks sound; optionalsaturating_subfor clarityThe reset condition correctly relies on
||short‑circuiting sorequested_frame - last.numberis only evaluated whenrequested_frame >= last.number, avoiding u32 underflow/panics. Behaviorally this matches the intended “large jump or backwards seek → reset” semantics.If you want to make this more obviously overflow‑safe and resilient to future refactors, you could consider:
- .map(|last| { - requested_frame < last.number - || requested_frame - last.number > FRAME_CACHE_SIZE as u32 - }) + .map(|last| { + requested_frame < last.number + || requested_frame + .saturating_sub(last.number) + > FRAME_CACHE_SIZE as u32 + })Not required, but it removes any subtle dependency on evaluation order.
crates/recording/src/screenshot.rs (1)
152-199: Usedebug!ortrace!level instead ofinfo!for detailed capture logging.The added logging statements (lines 155-160, 161-168, 175-181, 195-199) use
tracing::info!, which will produce verbose output in production for every area screenshot. Since these logs appear to be for troubleshooting coordinate calculations, consider usingdebug!ortrace!level to reduce log volume. Line 207 already usesdebug!for similar capture completion logging.Apply this diff to adjust the log levels:
- tracing::info!( + tracing::debug!( "Area screenshot debug: display_id={}, display_logical_bounds={:?}, display_physical={:?}", display_id, display_bounds, display_physical, ); - tracing::info!( + tracing::debug!( "Area screenshot: input logical bounds=({}, {}, {}x{}), scale={}", bounds.position().x(), bounds.position().y(), bounds.size().width(), bounds.size().height(), scale, ); let rect = CGRect::new( &CGPoint::new(bounds.position().x(), bounds.position().y()), &CGSize::new(bounds.size().width(), bounds.size().height()), ); - tracing::info!( + tracing::debug!( "Area screenshot: CGRect for capture (logical/points) = origin({}, {}), size({}x{})", rect.origin.x, rect.origin.y, rect.size.width, rect.size.height, ); let image = unsafe { CGDisplayCreateImageForRect(display_id, rect) }; if image.is_null() { return None; } unsafe { core_graphics::image::CGImage::from_ptr(image) } } }; let width = cg_image.width(); let height = cg_image.height(); let bytes_per_row = cg_image.bytes_per_row(); - tracing::info!( + tracing::debug!( "Fast capture result: image dimensions = {}x{}", width, height, );crates/enc-ffmpeg/src/video/h264.rs (3)
22-28: External conversion flag design is good; watch for silent misuse withqueue_frame
external_conversioncleanly skips internal scaling/conversion, but nothing prevents callers from still usingqueue_framewith unconverted frames when this flag is set, which would silently bypass conversion and send mismatched frames to the encoder. Consider either:
- Restricting
queue_frameusage whenexternal_conversionis true (e.g., via a debug guard), or- At least reviewing call sites to ensure that enabling
external_conversionalways pairs withqueue_preconverted_frameonly.This would make incorrect wiring of the external converter easier to catch.
Also applies to: 52-59, 81-85
272-292:ConversionRequirementsAPI seems reasonable; confirm exposure offfmpeg::format::Pixelis intentional
conversion_requirements()correctly mirrors the internal input/output formats and dimensions and theneeds_conversionflag matches the encoder’s converter decision logic. The struct exposesformat::Pixelrather thancap_media_info::Pixel, which is fine for this ffmpeg‑specific module but slightly tightens coupling to ffmpeg in the public surface. If higher‑level crates previously only dealt withcap_media_info::Pixel, consider whether returning that alias (or a neutral type) would make the external conversion API easier to consume.Also applies to: 309-322
351-375: Consider validating pre‑converted frame format/size before encode
queue_preconverted_framecurrently only logs the incoming and expected format/size and then callssend_frame. If the external converter is misconfigured (wrong pixel format or dimensions), this will only surface as a low‑level encode error, which is harder to diagnose.You could add cheap debug‑time guards here to catch wiring bugs early, for example:
pub fn queue_preconverted_frame( &mut self, mut frame: frame::Video, timestamp: Duration, output: &mut format::context::Output, ) -> Result<(), QueueFrameError> { + debug_assert_eq!(frame.format(), self.output_format); + debug_assert_eq!(frame.width(), self.output_width); + debug_assert_eq!(frame.height(), self.output_height); trace!( "Encoding pre-converted frame: format={:?}, size={}x{}, expected={:?} {}x{}", frame.format(), frame.width(), frame.height(), self.output_format, self.output_width, self.output_height );This keeps release behavior unchanged while making mis‑matched external conversion very obvious in debug builds.
apps/desktop/src/routes/editor/Timeline/Track.tsx (1)
14-18: Inconsistent style merge order between string and object cases.When
props.styleis an object, the spread{ height, ...(props.style ?? {}) }allowsprops.styleto overrideheight. However, whenprops.styleis a string, appending;height:${height}at the end meansheighttakes precedence and cannot be overridden byprops.style.If you want consistent behavior where
props.stylecan override height:const style = typeof props.style === "string" - ? `${props.style};height:${height}` + ? `height:${height};${props.style}` : { height, ...(props.style ?? {}) };Alternatively, if
heightshould always win, swap the object spread order to{ ...(props.style ?? {}), height }.apps/desktop/src/routes/editor/CaptionsTab.tsx (1)
214-227: Consider wrapping localStorage access in try/catch.
localStorageoperations can throw in restricted contexts (e.g., private browsing mode in some browsers, or when storage quota is exceeded). While this may be less of a concern in a Tauri desktop app, defensive error handling would improve robustness.- const savedModel = localStorage.getItem("selectedTranscriptionModel"); - if (savedModel && MODEL_OPTIONS.some((m) => m.name === savedModel)) { - setSelectedModel(savedModel); - } + try { + const savedModel = localStorage.getItem("selectedTranscriptionModel"); + if (savedModel && MODEL_OPTIONS.some((m) => m.name === savedModel)) { + setSelectedModel(savedModel); + } + } catch { + // localStorage may be unavailable + }apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (1)
587-595: Consider removing leftoverconsole.log("NUL")The
console.log("NUL")in the start-handle drag cleanup looks like debug output and may be noisy in production; safe to drop.crates/recording/examples/camera-benchmark.rs (2)
349-373: Consider using a CLI parsing library for consistency.Manual argument parsing works but is verbose and error-prone. The sibling
recording-benchmark.rsexample might use a more robust approach.If other examples use
clapor similar, consider adopting the same pattern for consistency. Otherwise, the current approach is acceptable for an example binary.
391-396: Edge case: p99 index calculation may access wrong element.When
sorted.len()equals the calculated index exactly, this still accesses a valid element. However, for very small arrays (e.g., 1 element),p99_idxcould be 0 which is technically correct but may not represent a meaningful percentile.Consider using
sorted.len().saturating_sub(1).min(p99_idx)to ensure you're accessing the last element for very small samples, or skip percentile reporting if the sample size is too small.crates/recording/examples/recording-benchmark.rs (3)
218-221: Avoidunsafeblock forset_var— prefer handling at startup or document the safety.
std::env::set_varis marked unsafe in Rust 2024 edition because it's not thread-safe. While this is an example binary with a single-threaded initialization path, consider alternatives:- unsafe { std::env::set_var("RUST_LOG", "info") }; - tracing_subscriber::fmt::init(); + tracing_subscriber::fmt() + .with_env_filter( + tracing_subscriber::EnvFilter::from_default_env() + .add_directive(tracing::Level::INFO.into()), + ) + .init();This avoids the unsafe block entirely by configuring the filter directly.
102-102: Intentional leak preserves benchmark outputs but leaves files on disk.The
std::mem::forget(dir)pattern preventsTempDircleanup, preserving output files for inspection. This is acceptable for a benchmark but consider adding a--cleanupflag for automated runs.
271-284: Unknown modes silently default to "full" — consider warning the user.The
"full" | _pattern catches typos silently. Users might not realize their intended mode wasn't recognized."stress" => { // ... } - "full" | _ => { + "full" => { println!("Mode: Full benchmark suite\n"); // ... } + unknown => { + println!("Unknown mode '{}', running full benchmark suite\n", unknown); + // same as full... + }crates/rendering/src/layers/text.rs (1)
101-103: HardcodedAlign::Centermay not suit all text overlays.All text is center-aligned regardless of input. If
PreparedTextshould support different alignments, consider adding an alignment field.+// In PreparedText struct (crates/rendering/src/text.rs): +// pub align: TextAlign, // Left, Center, Right for line in buffer.lines.iter_mut() { - line.set_align(Some(Align::Center)); + line.set_align(Some(match text.align { + TextAlign::Left => Align::Left, + TextAlign::Center => Align::Center, + TextAlign::Right => Align::Right, + })); }crates/rendering/src/mask.rs (2)
10-15: Sorting keyframes on every interpolation call may impact performance.
interpolate_vectorandinterpolate_scalarsort keyframes on each invocation. For frame-by-frame rendering, this could be called many times per second per segment.Consider pre-sorting keyframes when segments are loaded/modified, or cache the sorted order. If keyframes are typically already sorted, add a fast path:
fn interpolate_vector(base: XY<f64>, keys: &[MaskVectorKeyframe], time: f64) -> XY<f64> { if keys.is_empty() { return base; } - let mut sorted = keys.to_vec(); - sorted.sort_by(|a, b| { - a.time - .partial_cmp(&b.time) - .unwrap_or(std::cmp::Ordering::Equal) - }); + // Assume keyframes are pre-sorted by time + let sorted = keys;Or ensure sorting happens once at segment creation time.
86-95: Hardcoded fade duration (0.15s) for Highlight masks.The fade-in/fade-out duration is hardcoded. If this should be configurable per-segment or globally, consider exposing it as a parameter or reading from segment configuration.
crates/rendering/src/text.rs (1)
16-29: Consider supporting 8-character hex colors with alpha channel.The
parse_colorfunction only handles 6-character hex colors (RGB). If users specify colors with alpha (e.g.,#FF000080), the function silently falls back to white, which may be unexpected behavior.fn parse_color(hex: &str) -> [f32; 4] { let color = hex.trim_start_matches('#'); - if color.len() == 6 { - if let (Ok(r), Ok(g), Ok(b)) = ( + if color.len() == 8 { + if let (Ok(r), Ok(g), Ok(b), Ok(a)) = ( + u8::from_str_radix(&color[0..2], 16), + u8::from_str_radix(&color[2..4], 16), + u8::from_str_radix(&color[4..6], 16), + u8::from_str_radix(&color[6..8], 16), + ) { + return [r as f32 / 255.0, g as f32 / 255.0, b as f32 / 255.0, a as f32 / 255.0]; + } + } else if color.len() == 6 { + if let (Ok(r), Ok(g), Ok(b)) = ( u8::from_str_radix(&color[0..2], 16), u8::from_str_radix(&color[2..4], 16), u8::from_str_radix(&color[4..6], 16), ) { return [r as f32 / 255.0, g as f32 / 255.0, b as f32 / 255.0, 1.0]; } } [1.0, 1.0, 1.0, 1.0] }crates/frame-converter/examples/benchmark.rs (1)
71-82: Potential issue: loop may exit early without receiving all converted frames.The condition
stats.frames_converted >= frame_counttriggers an early break, butframes_convertedfrom stats may not equal the number of frames actually received viarecv. This could leave frames in the output channel unconsumed, though thetry_recvloop partially addresses this.The logic is functional but could be slightly clearer by tracking both submitted and received counts more explicitly. Consider if the break condition should be based on
receivedmatching expected rather than internal stats.crates/recording/src/output_pipeline/macos.rs (1)
142-251: Consider extracting shared muxer logic to reduce duplication.
AVFoundationCameraMuxeris nearly identical toAVFoundationMp4Muxer—the only difference is theVideoFrametype (NativeCameraFramevsscreen_capture::VideoFrame). Both share the same:
MAX_QUEUE_RETRIESconstantsetupandfinishimplementationssend_video_frameretry logicsend_audio_frameimplementationBased on learnings, the mutex-holding retry behavior is intentional for sequential processing. However, the duplication could be reduced with a generic base muxer parameterized by the video frame type.
This could be deferred if the two paths are expected to diverge, but currently they are ~95% identical code.
crates/frame-converter/Cargo.toml (1)
20-21: Empty macOS dependencies section can be removed.The
[target.'cfg(target_os = "macos")'.dependencies]section is empty and serves no purpose. Consider removing it unless it's a placeholder for future additions.-[target.'cfg(target_os = "macos")'.dependencies] - [target.'cfg(target_os = "windows")'.dependencies]apps/desktop/src/routes/editor/MaskOverlay.tsx (1)
154-176: Remove JSX comments per coding guidelines.The coding guidelines prohibit comments. Consider removing the
{/* Border/Highlight */},{/* Handles */},{/* Corners */}, and{/* Sides */}comments. The code structure is self-explanatory.apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (3)
404-416: Simplify selection check: use loop index directly.The
findIndexlookup is redundant and could fail if two segments share identical start/end times. Thei()accessor from theForloop already provides the correct index.Apply this diff:
const isSelected = createMemo(() => { const selection = editorState.timeline.selection; if (!selection || selection.type !== "zoom") return false; - - const segmentIndex = project.timeline?.zoomSegments?.findIndex( - (s) => s.start === segment.start && s.end === segment.end, - ); - - // Support both single selection (index) and multi-selection (indices) - if (segmentIndex === undefined || segmentIndex === -1) return false; - - return selection.indices.includes(segmentIndex); + return selection.indices.includes(i()); });This matches the simpler pattern used in
MaskTrack.
290-402: Consider hoistingcreateMouseDownDragoutside the For loop.The drag helper is recreated for each segment. While SolidJS's reactivity mitigates the performance impact, hoisting it outside (like
MaskTrackdoes) and passing the segment index as a parameter would be more consistent and slightly more efficient.
73-74: Remove code comments per coding guidelines.Lines 73-74, 101, 233, 244, 257, 321, 337, 354, and 361 contain comments. Per coding guidelines, code should be self-explanatory through naming and structure.
crates/recording/src/sources/native_camera.rs (1)
53-56: Inconsistent memory ordering between stop signal and check.The
stopmethod usesOrdering::SeqCstwhen storing, but the loop usesOrdering::Relaxedwhen loading. For a simple stop flag,Ordering::Relaxedis typically sufficient on both sides since there's no data dependency. However, the inconsistency is confusing. Consider using the same ordering on both sides for clarity.- if stopped_clone.load(Ordering::Relaxed) { + if stopped_clone.load(Ordering::SeqCst) {Or alternatively, use
Relaxedin both places since you also have the oneshot channel as a backup signal.Also applies to: 138-138
apps/desktop/src/routes/editor/TextOverlay.tsx (1)
300-324: Remove comment and clarify invisible placeholder purpose.Remove the comment on line 301. The div with
opacity: 0serves as a layout placeholder - consider if this could be removed entirely since it's invisible and haspointer-events: none.- {/* Visual placeholder (not used for measurement anymore) */} - <div - style={{ - "white-space": "pre-wrap", - "word-break": "break-word", - "font-family": segment().fontFamily, - "font-size": `${segment().fontSize}px`, - "font-weight": segment().fontWeight, - "font-style": segment().italic ? "italic" : "normal", - color: segment().color, - "line-height": 1.2, - width: "100%", - height: "100%", - display: "flex", - "align-items": "center", - "justify-content": "center", - "text-align": "center", - "pointer-events": "none", - opacity: 0, - }} - class="w-full h-full overflow-hidden" - > - {segment().content} - </div>crates/recording/src/feeds/camera.rs (1)
587-624: Consider extracting common FFmpeg frame handling logic.The FFmpeg frame processing logic (lines 587-624) is nearly identical to the macOS version (lines 485-522). While the platform-specific timestamp construction differs, the remaining logic could potentially be extracted into a shared helper. This is optional given the platform-specific nature.
apps/desktop/src/store/captions.ts (1)
125-155: Consider using spread operator for settings persistence.The explicit field listing works but requires updates whenever
CaptionSettingschanges. Using a spread would be more maintainable:const captionsData = { segments: state.segments, - settings: { - enabled: state.settings.enabled, - font: state.settings.font, - // ... all other fields - }, + settings: { ...state.settings }, };However, if you intentionally want to filter which fields are persisted, the current approach is valid.
apps/desktop/src/routes/editor/ExportDialog.tsx (2)
1104-1132: Button inside anchor has conflicting actions.The "Open Link" button is wrapped in an anchor that navigates to the share link, but the button's
onClickalso copies the URL to clipboard. This dual behavior might be intentional, but:
- The button text says "Open Link" but it also copies (confusing)
- Button inside anchor can have accessibility issues
Consider separating these actions or updating the text to indicate both behaviors (e.g., "Open & Copy Link").
355-366: Inconsistent use of reconcile vs setExportState.The
savemutation's error handler usessetExportState({ type: "idle" })directly, whilecopyanduploadusesetExportState(reconcile({ type: "idle" })). For consistency and to match the existing pattern, consider usingreconcilehere as well.onError: (error) => { if (isCancelled() || isCancellationError(error)) { - setExportState({ type: "idle" }); + setExportState(reconcile({ type: "idle" })); return; } // ... - setExportState({ type: "idle" }); + setExportState(reconcile({ type: "idle" })); },apps/desktop/src/routes/editor/masks.ts (1)
10-14: Consider usingXY<number>for consistency.
MaskVectorKeyframeuses flatxandyfields, whileMaskSegmentusesXY<number>forcenterandsize. UsingXY<number>here would improve consistency:export type MaskVectorKeyframe = { time: number; - x: number; - y: number; + value: XY<number>; };This would require updating
upsertVectorKeyframeto usevalue.xandvalue.y.apps/desktop/src-tauri/src/captions.rs (2)
557-585: Consider dialing back info‑level logging in Whisper processingThe per‑token and per‑word logging in
process_with_whisper(segment, token, RMS, and word summaries) atinfo!level will generate very large logs for longer recordings and may impact performance and log usability. It would be safer to move most of this todebug!/trace!or gate it behind a feature flag or config toggle while keeping only high‑level summaries atinfo!.Also applies to: 609-783
1048-1056: Avoid panicking on invalidfadeDurationwhen serializing settings
serde_json::Number::from_f64(settings.fade_duration as f64).unwrap()will panic iffade_durationever becomes NaN. While unlikely, this is user‑visible data; consider handlingNonefromfrom_f64gracefully (e.g., clamping to a sane default or skipping the field) instead of unwrapping.crates/recording/examples/encoding-benchmark.rs (1)
115-142: Per‑frame metrics use a singlereceive_time, skewing conversion/latency statsWithin the main loop and drain phase, all converted frames in a batch use the same
receive_time(or a fixed 1ms duration during drain), regardless of when each frame was actually submitted. This makesconversion_durationandpipeline_latencyinPipelineMetricsapproximate rather than per‑frame accurate.If you care about the absolute metrics, consider tracking per‑frame submit times keyed by the sequence ID, and propagating that through the pool so
record_frame_converted/record_frame_encodedcan use the correct timestamps.Also applies to: 147-163
apps/desktop/src-tauri/src/lib.rs (1)
653-700: Camera watcher logging may be too chatty at debug level
is_camera_availablenow logs every camera’s IDs and display name on each pass ofspawn_camera_watcher(once per second while recording). Even atdebug!, this can generate a lot of log noise on long recordings or multi‑camera setups. Consider keeping the high‑level availability check log and only emitting per‑camera details when a camera actually transitions between available/unavailable, or behind a dedicated verbose flag.crates/recording/src/output_pipeline/async_camera.rs (1)
54-61: Async camera muxer ignores pause flag / timestamp adjustmentUnlike
WindowsMuxer,AsyncCameraMp4Muxer::setupignores thepause_flagand there is noPauseTrackerequivalent;send_video_framealways uses the rawtimestamp. If this muxer is used in contexts where camera recording can be paused, encoded timestamps will continue advancing through pauses and may no longer align with the rest of the pipeline.If pause/resume is expected for this path, consider threading in a
PauseTracker(or equivalent) and applying it to the incomingtimestampbefore setting PTS or queueing frames, mirroring the behavior in the other muxers.Also applies to: 119-129, 205-283
crates/frame-converter/src/lib.rs (2)
56-81: FrameConverter trait defaults are reasonable but may benefit from being non-exhaustiveThe core methods (
convert,name,backend) plus defaulted helpers (is_hardware_accelerated,conversion_count,verify_hardware_usage) give a clear, minimal contract for backends. Consider marking the trait as the stable public surface and planning for evolution (e.g., via additional helper methods) so future backends don’t need to special‑case behavior outside this trait.
139-237: Backend selection and fallback flow is soundThe platform-specific preference ordering (VideoToolbox on macOS, D3D11 on Windows, then Swscale) with accumulated
fallback_reasonsand a final software fallback is clear and robust. The “no conversion needed” short‑circuit to aPassthroughConverteravoids unnecessary work and matchesConversionConfig::needs_conversion(). Only suggestion is to consider whether you want to expose the aggregated fallback reasons more formally (e.g., as a structured type) if debugging multiple hardware failures becomes common.apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
2767-2827: MaskSegmentConfig correctly normalizes keyframes but has unused reactive helpers
updateSegmentensureskeyframesis always present andcreateEffectclears any existing keyframes once, which matches the intent to reset legacy animated masks when editing.setPosition,setSize, andsetIntensityall clear keyframes on change to avoid conflicting animation. However,currentAbsoluteTimeandmaskStateare computed but never used, which may triggernoUnusedLocals-style lint warnings. Consider either wiringmaskStateinto the UI or removing these helpers to keep the component leaner.crates/rendering/src/layers/captions.rs (1)
707-715: Minor: Redundant scissor rect call.Line 712 sets the same scissor rect that was just set at line 708. This is harmless but unnecessary.
if let Some([x, y, width, height]) = self.background_scissor { pass.set_scissor_rect(x, y, width, height); pass.set_pipeline(&self.background_pipeline); pass.set_bind_group(0, &self.background_bind_group, &[]); pass.draw(0..6, 0..1); - pass.set_scissor_rect(x, y, width, height); } else if self.output_size.0 > 0 && self.output_size.1 > 0 {crates/recording/src/benchmark.rs (2)
71-77:unwrap()on lock operations may panic on poisoned locks.If a thread panics while holding the lock, subsequent operations will panic. For benchmarking code this is usually acceptable, but consider using
unwrap_or_elsewith error logging if silent failures are preferred.
463-466: macOS GPU detection returns a static placeholder.
detect_macos_gpu()returns a hardcoded string rather than actual GPU detection. This may be misleading in benchmark reports.Consider using IOKit or
system_profilerto detect the actual GPU, or update the string to indicate it's a placeholder:#[cfg(target_os = "macos")] fn detect_macos_gpu() -> String { - "Apple Silicon / Intel UHD".to_string() + "VideoToolbox (GPU detection not implemented)".to_string() }
| pub fn format_project_name<'a>( | ||
| template: Option<&str>, | ||
| target_name: &'a str, | ||
| target_kind: &'a str, | ||
| recording_mode: RecordingMode, | ||
| datetime: Option<chrono::DateTime<chrono::Local>>, | ||
| ) -> String { | ||
| const DEFAULT_FILENAME_TEMPLATE: &str = "{target_name} ({target_kind}) {date} {time}"; | ||
| let datetime = datetime.unwrap_or(chrono::Local::now()); | ||
|
|
||
| lazy_static! { | ||
| static ref DATE_REGEX: Regex = Regex::new(r"\{date(?::([^}]+))?\}").unwrap(); | ||
| static ref TIME_REGEX: Regex = Regex::new(r"\{time(?::([^}]+))?\}").unwrap(); | ||
| static ref MOMENT_REGEX: Regex = Regex::new(r"\{moment(?::([^}]+))?\}").unwrap(); | ||
| static ref AC: aho_corasick::AhoCorasick = { | ||
| aho_corasick::AhoCorasick::new([ | ||
| "{recording_mode}", | ||
| "{mode}", | ||
| "{target_kind}", | ||
| "{target_name}", | ||
| ]) | ||
| .expect("Failed to build AhoCorasick automaton") | ||
| }; | ||
| } | ||
| let haystack = template.unwrap_or(DEFAULT_FILENAME_TEMPLATE); | ||
|
|
||
| // Get recording mode information | ||
| let (recording_mode, mode) = match recording_mode { | ||
| RecordingMode::Studio => ("Studio", "studio"), | ||
| RecordingMode::Instant => ("Instant", "instant"), | ||
| RecordingMode::Screenshot => ("Screenshot", "screenshot"), | ||
| }; | ||
|
|
||
| let result = AC | ||
| .try_replace_all(haystack, &[recording_mode, mode, target_kind, target_name]) | ||
| .expect("AhoCorasick replace should never fail with default configuration"); | ||
|
|
||
| let result = DATE_REGEX.replace_all(&result, |caps: ®ex::Captures| { | ||
| datetime | ||
| .format( | ||
| &caps | ||
| .get(1) | ||
| .map(|m| m.as_str()) | ||
| .map(moment_format_to_chrono) | ||
| .unwrap_or(Cow::Borrowed("%Y-%m-%d")), | ||
| ) | ||
| .to_string() | ||
| }); | ||
|
|
||
| let result = TIME_REGEX.replace_all(&result, |caps: ®ex::Captures| { | ||
| datetime | ||
| .format( | ||
| &caps | ||
| .get(1) | ||
| .map(|m| m.as_str()) | ||
| .map(moment_format_to_chrono) | ||
| .unwrap_or(Cow::Borrowed("%I:%M %p")), | ||
| ) | ||
| .to_string() | ||
| }); | ||
|
|
||
| let result = MOMENT_REGEX.replace_all(&result, |caps: ®ex::Captures| { | ||
| datetime | ||
| .format( | ||
| &caps | ||
| .get(1) | ||
| .map(|m| m.as_str()) | ||
| .map(moment_format_to_chrono) | ||
| .unwrap_or(Cow::Borrowed("%Y-%m-%d %H:%M")), | ||
| ) | ||
| .to_string() | ||
| }); | ||
|
|
||
| result.into_owned() | ||
| } |
There was a problem hiding this comment.
format_project_name implementation is solid; remove the new inline comment
The template-based formatter correctly supports {recording_mode}, {mode}, {target_kind}, {target_name}, {date}, {time}, and {moment:…} placeholders. Using a prebuilt Aho‑Corasick automaton plus Regex with moment_format_to_chrono keeps it efficient and flexible while defaulting to {target_name} ({target_kind}) {date} {time} when no template is provided. The only issue is the new // Get recording mode information comment at Line 381, which violates the “no comments in code” rule for Rust files. Please remove that comment line.
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/recording.rs around lines 355 to 429, remove the
single inline comment "// Get recording mode information" at ~line 381 to comply
with the Rust "no comments in code" rule; simply delete that line and leave the
surrounding code (the match assigning (recording_mode, mode)) intact with no
other changes.
| onChange={(e) => | ||
| updateSegment(segment.id, { | ||
| start: parseFloat(e.target.value), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Validate parseFloat result before updating segment.
If the user enters non-numeric text, parseFloat returns NaN, which would corrupt the segment's start or end values.
onChange={(e) =>
- updateSegment(segment.id, {
- start: parseFloat(e.target.value),
- })
+ {
+ const value = parseFloat(e.target.value);
+ if (!Number.isNaN(value)) {
+ updateSegment(segment.id, { start: value });
+ }
+ }
}Apply the same fix to the end time input on lines 853-857.
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/CaptionsTab.tsx around lines 836-840 (and
similarly for the end time input at 853-857): the onChange handlers call
parseFloat(e.target.value) and pass the result straight to updateSegment, which
will set start/end to NaN if the input is non-numeric. Validate the parsed value
before calling updateSegment — parse the input, check Number.isFinite(parsed)
(or !Number.isNaN(parsed)), and only call updateSegment with the numeric value
when valid; otherwise skip the update (or keep the prior value) and optionally
handle invalid input UI state. Apply the same validation for the end time input
handler at lines 853-857.
| return ( | ||
| <Show when={selectedMask() && maskState()}> | ||
| {() => { | ||
| const state = () => maskState()!; |
There was a problem hiding this comment.
Fix TypeScript error: Show callback must accept the resolved value.
The pipeline fails because the Show component's function child must accept the truthy value from when. Currently, the callback takes no arguments, causing TS2769.
Apply this diff to fix the type error:
return (
- <Show when={selectedMask() && maskState()}>
- {() => {
- const state = () => maskState()!;
+ <Show when={maskState()}>
+ {(state) => {
const rect = () => {
- const width = state().size.x * props.size.width;
- const height = state().size.y * props.size.height;
- const left = state().position.x * props.size.width - width / 2;
- const top = state().position.y * props.size.height - height / 2;
+ const width = state().size.x * props.size.width;
+ const height = state().size.y * props.size.height;
+ const left = state().position.x * props.size.width - width / 2;
+ const top = state().position.y * props.size.height - height / 2;
return { width, height, left, top };
};The maskState() memo already depends on selectedMask(), so the outer selectedMask() && check is redundant and causes the type mismatch.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: CI
[error] 90-90: No overload matches this call. (TS2769). Command failed: 'pnpm tsc -b' during typecheck step.
🪛 GitHub Check: Typecheck
[failure] 90-90:
No overload matches this call.
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/MaskOverlay.tsx around lines 88 to 91, the
Show callback currently takes no arguments and the when prop uses
`selectedMask() && maskState()` which causes a TS2769; change the `when` to use
`maskState()` alone (since it already depends on `selectedMask()`) and update
the function child to accept the resolved value (e.g., `(state) => { ... }`) so
the callback signature matches the truthy value provided by Show.
| <KSelect.Value<{ label: string; value: PreviewQuality }> | ||
| class="flex-1 text-left truncate" | ||
| placeholder="Select preview quality" | ||
| > | ||
| {(state) => | ||
| state.selectedOption()?.label ?? "Select preview quality" | ||
| } | ||
| </KSelect.Value> |
There was a problem hiding this comment.
Type error: KSelect.Value children signature mismatch.
The static analysis reports a type error. The children prop signature for KSelect.Value expects a specific render function type. The current implementation returns string | undefined which doesn't match the expected type.
Consider using the component's placeholder prop for the fallback and simplifying the render function:
<KSelect.Value<{ label: string; value: PreviewQuality }>
class="flex-1 text-left truncate"
- placeholder="Select preview quality"
>
- {(state) =>
- state.selectedOption()?.label ?? "Select preview quality"
- }
+ {(state) => state.selectedOption()?.label}
</KSelect.Value>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <KSelect.Value<{ label: string; value: PreviewQuality }> | |
| class="flex-1 text-left truncate" | |
| placeholder="Select preview quality" | |
| > | |
| {(state) => | |
| state.selectedOption()?.label ?? "Select preview quality" | |
| } | |
| </KSelect.Value> | |
| <KSelect.Value<{ label: string; value: PreviewQuality }> | |
| class="flex-1 text-left truncate" | |
| > | |
| {(state) => state.selectedOption()?.label} | |
| </KSelect.Value> |
🧰 Tools
🪛 GitHub Check: Typecheck
[failure] 294-294:
Type '{ children: (state: SelectValueState<{ label: string; value: PreviewQuality; }>) => string; class: string; placeholder: string; }' is not assignable to type 'IntrinsicAttributes & Omit<HTMLAttributes, "id" | "children" | "as"> & SelectValueOptions<{ label: string; value: PreviewQuality; }> & Partial<...> & PolymorphicAttributes<...>'.
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Player.tsx around lines 292-299, the
KSelect.Value children currently return a string or undefined which mismatches
the expected render-function signature; instead, remove the string fallback and
rely on the component's placeholder prop for the fallback, and change the render
callback to always return a valid ReactNode (e.g., return the selected option's
label or null) so the children signature matches the expected type.
| <MenuItem as="div" class="text-gray-11" data-disabled="true"> | ||
| Select preview quality | ||
| </MenuItem> |
There was a problem hiding this comment.
Type error: MenuItem polymorphic as prop type mismatch.
The static analysis indicates that as="div" causes a type conflict since MenuItem defaults to button attributes. Either cast explicitly or use a component that supports the as polymorphism properly.
-<MenuItem as="div" class="text-gray-11" data-disabled="true">
+<MenuItem as={"div" as any} class="text-gray-11" data-disabled="true">
Select preview quality
</MenuItem>Alternatively, consider using a simple <div> with matching styles instead of polymorphing MenuItem.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Typecheck
[failure] 309-309:
Type '{ children: string; as: string; class: string; "data-disabled": string; }' is not assignable to type 'IntrinsicAttributes & ButtonHTMLAttributes'.
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Player.tsx around lines 309-311, the MenuItem
is used with as="div" causing a polymorphic prop type mismatch because MenuItem
defaults to button attributes; replace this with a plain <div> styled the same
way or explicitly cast the MenuItem to the correct polymorphic type (e.g.,
MenuItem as unknown as React.ElementType) and adjust props to match div
attributes so TypeScript stops complaining. Ensure the replacement preserves
className="text-gray-11" and data-disabled="true" behavior and remove any
button-only props when switching to a div.
| fn pixel_to_cv_format(pixel: Pixel) -> Option<u32> { | ||
| match pixel { | ||
| Pixel::YUYV422 => Some(K_CV_PIXEL_FORMAT_TYPE_422_YP_CB_YP_CR8), | ||
| Pixel::UYVY422 => Some(K_CV_PIXEL_FORMAT_TYPE_2VUY), | ||
| Pixel::NV12 => Some(K_CV_PIXEL_FORMAT_TYPE_420_YP_CB_CR8_BI_PLANAR_VIDEO_RANGE), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Apple CoreVideo kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange CVPixelBufferCreateWithBytes baseAddr planar
💡 Result:
Short answer — use the planar API or copy into a CVPixelBuffer you create yourself.
Key facts:
- kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange (NV12) is a 2‑plane Y/CbCr format; when CoreVideo expects a "baseAddr" for this OSType it means a pointer to a big‑endian CVPlanarPixelBufferInfo_YCbCrBiPlanar describing the component(s). [1][2]
- If you already have separate plane pointers, use CVPixelBufferCreateWithPlanarBytes (it takes planeBaseAddress[] and per‑plane strides/widths/heights). [3]
- If you need an IOSurface‑backed CVPixelBuffer (e.g. for texture cache / GPU use) you cannot use CVPixelBufferCreateWithBytes or CVPixelBufferCreateWithPlanarBytes — instead create the pixel buffer with CVPixelBufferCreate (with kCVPixelBufferIOSurfacePropertiesKey) and copy/nomap your plane data into the locked planes via CVPixelBufferGetBaseAddressOfPlane. (Apple QA1781). [3][4]
References:
- kCVPixelFormatType_420YpCbCr8BiPlanar* description (baseAddr → CVPlanarPixelBufferInfo_YCbCrBiPlanar). [1]
- CVPlanar/CVPlanarPixelBufferInfo docs and struct usage. [2]
- CVPixelBufferCreateWithPlanarBytes and Apple QA about IOSurface/backing restrictions. [3][4]
If you want, tell me which form your Y/UV data is in (single buffer with offsets, two separate pointers, or three planes) and whether you need IOSurface/GPU backing — I’ll give the exact API call and a short code example.
🏁 Script executed:
cd crates/frame-converter && find . -name "videotoolbox.rs" -type f && wc -l src/videotoolbox.rsRepository: CapSoftware/Cap
Length of output: 174
🏁 Script executed:
cd crates/frame-converter && sed -n '1,120p' src/videotoolbox.rs | cat -nRepository: CapSoftware/Cap
Length of output: 174
NV12 requires different CoreVideo API than packed formats; verify buffer creation matches format
NV12 is a planar format that requires either CVPixelBufferCreateWithPlanarBytes (for separate Y/UV plane pointers) or manual plane access via CVPixelBufferCreate + CVPixelBufferGetBaseAddressOfPlane. The packed formats YUYV422 and UYVY422 can use CVPixelBufferCreateWithBytes with a single pointer, but NV12 cannot use the same approach without risking buffer corruption. Verify that create_input_pixel_buffer and copy_output_to_frame use the correct API for planar formats.
🤖 Prompt for AI Agents
crates/frame-converter/src/videotoolbox.rs around lines 77 to 84: the code maps
NV12 to a CV pixel format constant but the comment warns NV12 is planar and
cannot be handled like packed YUYV/UYVY; update create_input_pixel_buffer and
copy_output_to_frame to use CoreVideo planar APIs when pixel == Pixel::NV12 —
specifically, for NV12 either call CVPixelBufferCreateWithPlanarBytes (passing
separate Y and UV plane pointers/strides and a release callback) or create a
CVPixelBuffer with kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange and then use
CVPixelBufferLockBaseAddress + CVPixelBufferGetBaseAddressOfPlane to copy Y and
UV data per-plane; ensure stride/rowbytes and plane sizes are computed
correctly, handle buffer locking/unlocking, and do not feed NV12 data into the
single-pointer CVPixelBufferCreateWithBytes path used for packed formats.
| let mut output = | ||
| ffmpeg::format::output_as("/dev/null", "mp4").expect("Failed to create dummy output"); |
There was a problem hiding this comment.
Platform-specific path limits portability.
Using "/dev/null" as the output path only works on Unix-like systems. This example will fail on Windows.
Consider using a cross-platform null device or a temp file:
-let mut output =
- ffmpeg::format::output_as("/dev/null", "mp4").expect("Failed to create dummy output");
+let null_path = if cfg!(windows) { "NUL" } else { "/dev/null" };
+let mut output =
+ ffmpeg::format::output_as(null_path, "mp4").expect("Failed to create dummy output");🤖 Prompt for AI Agents
In crates/recording/examples/camera-benchmark.rs around lines 158-159, the
hardcoded "/dev/null" path is Unix-only and breaks on Windows; replace it with a
cross-platform solution by either choosing the OS-specific null device via
conditional compilation (use "NUL" on Windows, "/dev/null" elsewhere) or create
a temporary file (e.g., use tempfile or std::env::temp_dir to create a
NamedTempFile and pass its path) and use that path for ffmpeg::format::output_as
so the example runs on all platforms.
| let frame_interval = Duration::from_secs_f64(1.0 / config.target_fps as f64); | ||
| let total_frames = config.duration_secs * config.target_fps as u64; | ||
| let warmup_frames = config.warmup_secs * config.target_fps as u64; | ||
|
|
There was a problem hiding this comment.
Guard against target_fps == 0 in benchmark config
frame_interval is computed as Duration::from_secs_f64(1.0 / config.target_fps as f64). If a user passes --fps 0, this becomes 1.0 / 0.0 and will panic inside Duration::from_secs_f64. Consider clamping target_fps to at least 1 or returning an error when parsing invalid values so the benchmark fails gracefully instead of panicking.
Also applies to: 383-399
🤖 Prompt for AI Agents
In crates/recording/examples/encoding-benchmark.rs around lines 82-85 (and
similarly at 383-399), the code computes frame_interval as
Duration::from_secs_f64(1.0 / config.target_fps as f64) which will panic for
target_fps == 0; validate or normalize target_fps before this computation by
either clamping it to a minimum of 1 or returning/propagating a user-facing
error when parsing the config (e.g., check if config.target_fps == 0 then return
Err(...) or set let fps = config.target_fps.max(1) and use fps for subsequent
calculations), and update all other places (lines 383-399) that perform the same
division to use the validated/clamped value so the benchmark fails gracefully
instead of panicking.
| #[derive(Clone)] | ||
| pub struct NativeCameraFrame { | ||
| pub sample_buf: arc::R<cidre::cm::SampleBuf>, | ||
| pub timestamp: Timestamp, | ||
| } | ||
|
|
||
| unsafe impl Send for NativeCameraFrame {} | ||
| unsafe impl Sync for NativeCameraFrame {} | ||
|
|
||
| impl VideoFrame for NativeCameraFrame { | ||
| fn timestamp(&self) -> Timestamp { | ||
| self.timestamp | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd crates && find . -name "*.rs" -type f | xargs grep -l "cidre" | head -5Repository: CapSoftware/Cap
Length of output: 174
🏁 Script executed:
rg "SampleBuf" --type rust -C 5 | head -150Repository: CapSoftware/Cap
Length of output: 174
🏁 Script executed:
rg "impl.*Send|impl.*Sync" --type rust crates/recording/src/output_pipeline/macos.rs -B 2 -A 2Repository: CapSoftware/Cap
Length of output: 174
🏁 Script executed:
rg "arc::R" --type rust -C 3 | head -100Repository: CapSoftware/Cap
Length of output: 174
🌐 Web query:
cidre crate Rust SampleBuf thread safety Send Sync
💡 Result:
The cidre cm::SampleBuf type is Send but not Sync — i.e., it can be moved/owned by another thread but you must not share references to it across threads concurrently. See the type doc showing "Send" under Trait Implementations and "!Sync" under Auto Trait Implementations. [1][2]
Implication: you may move a SampleBuf into a spawned thread (ownership/Move) but you must not access the same SampleBuf from multiple threads via &SampleBuf (no shared references) unless you add your own synchronization or convert to a thread-safe wrapper (Arc/Mutex, or a crate that provides runtime-checked wrappers). [2][3]
Sources:
- cidre cm::SampleBuf docs (shows Send and !Sync). [1]
- Rust Send/Sync explanation (what Send vs Sync mean). [2]
- Common patterns to share non-Sync types across threads (Arc/Mutex or runtime wrappers). [3]
References:
[1] https://docs.rs/cidre/latest/cidre/cm/sample_buffer/struct.SampleBuf.html
[2] https://doc.rust-lang.org/nomicon/send-and-sync.html
[3] https://doc.rust-lang.org/book/ch16-04-extensible-concurrency-sync-and-send.html
🌐 Web query:
Apple Core Media CMSampleBuffer thread safety
💡 Result:
Short answer
- CMSampleBuffer objects are effectively safe to pass between threads for read-only use (CF retains/releases and immutable reads are fine). (developer.apple.com)
- Mutations (e.g., setting attachments with CMSetAttachment / CMSetAttachments) are NOT thread‑safe — Apple documentation and technical Q&As warn against concurrent attachment changes. (docs.rs)
- Many sample buffers can reference shared/pooled memory from device capture; retaining them for long can block reuse — if you need long‑lived ownership or to modify data, copy the buffer (CMSampleBufferCreateCopy / copy the payload) and then release the original. (stackoverflow.com)
Practical rules
- Read from CMSampleBuffer on any thread but avoid concurrent mutation without your own locks. (developer.apple.com)
- Don’t call CMSetAttachment concurrently; instead synthesize separate sample buffers with needed attachments or serialize attachment writes. (docs.rs)
- If you must hold sample buffers across time or threads, make a copy (CMSampleBufferCreateCopy or copy the underlying CMBlockBuffer) to avoid blocking capture memory pools. (stackoverflow.com)
Sources
[1] Apple Thread Safety summary / Core Foundation guidance. (developer.apple.com)
[2] Apple CoreMedia attachment guidance / kCMSampleBufferAttachmentKey_ForceKeyFrame note. (docs.rs)
[3] Apple Q&A on attachments and community guidance on copying CMSampleBuffer for long‑term use (and StackOverflow discussion). (developer-mdn.apple.com)
If you want, I can review specific code and point out where you must add locking or copy buffers.
Citations:
- 1: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/ThreadSafetySummary/ThreadSafetySummary.html?utm_source=openai
- 2: https://docs.rs/objc2-core-media/latest/objc2_core_media/static.kCMSampleBufferAttachmentKey_ForceKeyFrame.html?utm_source=openai
- 3: https://stackoverflow.com/questions/34972377/deep-copy-of-cmsamplebufferref?utm_source=openai
- 4: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/ThreadSafetySummary/ThreadSafetySummary.html?utm_source=openai
- 5: https://docs.rs/objc2-core-media/latest/objc2_core_media/static.kCMSampleBufferAttachmentKey_ForceKeyFrame.html?utm_source=openai
- 6: https://stackoverflow.com/questions/34972377/deep-copy-of-cmsamplebufferref?utm_source=openai
- 7: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/ThreadSafetySummary/ThreadSafetySummary.html?utm_source=openai
- 8: https://docs.rs/objc2-core-media/latest/objc2_core_media/static.kCMSampleBufferAttachmentKey_ForceKeyFrame.html?utm_source=openai
- 9: https://developer-mdn.apple.com/library/archive/qa/qa1957/_index.html?utm_source=openai
🌐 Web query:
cidre cm::SampleBuf arc::R wrapper
💡 Result:
Short answer:
-
arc::R in cidre is the crate's reference-counted Rust wrapper type for Objective‑C/Apple object references (used where an owned/retained Obj‑C id is returned). See examples in the cidre source (e.g., AVAudioConverter methods return Option<arc::R<...>>). [1]
-
cm::sample_buffer in cidre is the CoreMedia bindings around CMSampleBuffer/related APIs (attach keys, errors, creation/copy functions, etc.). The crate exposes the CoreMedia CMSampleBuffer functionality via the cm::sample_buffer module and the many attach_key/err constants. [2]
References:
- cidre source showing arc::R usage (AVAudioConverter returns arc::R). [1]
- cidre API list including cm::sample_buffer items (attach_keys, errors, CMSampleBuffer bindings). [2]
Remove unsafe impl Sync for NativeCameraFrame—cidre::cm::SampleBuf is Send but not Sync.
According to cidre's type documentation, SampleBuf implements Send but not Sync. This means it can be moved across threads but cannot be safely shared via references across threads concurrently. Declaring unsafe impl Sync for NativeCameraFrame is unsound and violates Rust's thread safety guarantees. Keep only unsafe impl Send; use Arc<Mutex<_>> or similar synchronization if shared mutable access across threads is needed.
🤖 Prompt for AI Agents
In crates/recording/src/output_pipeline/macos.rs around lines 16 to 29, remove
the unsafe impl Sync for NativeCameraFrame because cidre::cm::SampleBuf is Send
but not Sync; keep only unsafe impl Send and, if you need shared access across
threads, wrap the SampleBuf (or the whole NativeCameraFrame) in a thread-safe
container such as Arc<Mutex<...>> or Arc<RwLock<...>> (or adjust code to
transfer ownership between threads instead of sharing references) so you no
longer assert Sync unsafely.
| let mut first_timestamp: Option<Duration> = None; | ||
| let mut frame_count = 0u64; | ||
|
|
||
| let process_frame = |frame: NativeCameraFrame, | ||
| timestamp: Duration| | ||
| -> windows::core::Result< | ||
| Option<( | ||
| windows::Win32::Graphics::Direct3D11::ID3D11Texture2D, | ||
| TimeSpan, | ||
| )>, | ||
| > { | ||
| let relative = if let Some(first) = first_timestamp { | ||
| timestamp.checked_sub(first).unwrap_or(Duration::ZERO) | ||
| } else { | ||
| first_timestamp = Some(timestamp); | ||
| Duration::ZERO | ||
| }; | ||
|
|
||
| let texture = upload_mf_buffer_to_texture(&d3d_device, &frame)?; | ||
| Ok(Some((texture, duration_to_timespan(relative)))) | ||
| }; | ||
|
|
||
| if let Ok(Some((texture, frame_time))) = process_frame(first_frame.0, first_frame.1) | ||
| { | ||
| encoder | ||
| .run( | ||
| Arc::new(AtomicBool::default()), | ||
| || { | ||
| if frame_count > 0 { | ||
| let Ok(Some((frame, timestamp))) = video_rx.recv() else { | ||
| trace!("No more camera frames available"); | ||
| return Ok(None); | ||
| }; | ||
| frame_count += 1; | ||
| if frame_count % 30 == 0 { | ||
| debug!( | ||
| "Windows camera encoder: processed {} frames", | ||
| frame_count | ||
| ); | ||
| } | ||
| return process_frame(frame, timestamp); | ||
| } | ||
| frame_count += 1; | ||
| Ok(Some((texture.clone(), frame_time))) | ||
| }, | ||
| |output_sample| { | ||
| let mut output = output.lock().unwrap(); | ||
| let _ = muxer | ||
| .write_sample(&output_sample, &mut *output) | ||
| .map_err(|e| format!("WriteSample: {e}")); | ||
| Ok(()) | ||
| }, | ||
| ) | ||
| .context("run camera encoder")?; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -type f -name "win.rs" -path "*/output_pipeline/*" 2>/dev/null | head -5Repository: CapSoftware/Cap
Length of output: 174
🏁 Script executed:
git ls-files | grep -E "output_pipeline.*win\.rs"Repository: CapSoftware/Cap
Length of output: 174
🏁 Script executed:
cat crates/recording/src/output_pipeline/win.rs | wc -lRepository: CapSoftware/Cap
Length of output: 174
🏁 Script executed:
sed -n '564,618p' crates/recording/src/output_pipeline/win.rsRepository: CapSoftware/Cap
Length of output: 174
Fix closure mutability for process_frame (Rust compile error)
process_frame mutates the captured first_timestamp, so the closure is FnMut. Calling an FnMut closure stored in an immutable binding will not compile; Rust requires the binding itself to be mut so it can be borrowed as &mut for calls.
Update the binding to be mutable:
- let process_frame = |frame: NativeCameraFrame,
+ let mut process_frame = |frame: NativeCameraFrame,
timestamp: Duration|Without this change, this file will fail to compile on Windows targets.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut first_timestamp: Option<Duration> = None; | |
| let mut frame_count = 0u64; | |
| let process_frame = |frame: NativeCameraFrame, | |
| timestamp: Duration| | |
| -> windows::core::Result< | |
| Option<( | |
| windows::Win32::Graphics::Direct3D11::ID3D11Texture2D, | |
| TimeSpan, | |
| )>, | |
| > { | |
| let relative = if let Some(first) = first_timestamp { | |
| timestamp.checked_sub(first).unwrap_or(Duration::ZERO) | |
| } else { | |
| first_timestamp = Some(timestamp); | |
| Duration::ZERO | |
| }; | |
| let texture = upload_mf_buffer_to_texture(&d3d_device, &frame)?; | |
| Ok(Some((texture, duration_to_timespan(relative)))) | |
| }; | |
| if let Ok(Some((texture, frame_time))) = process_frame(first_frame.0, first_frame.1) | |
| { | |
| encoder | |
| .run( | |
| Arc::new(AtomicBool::default()), | |
| || { | |
| if frame_count > 0 { | |
| let Ok(Some((frame, timestamp))) = video_rx.recv() else { | |
| trace!("No more camera frames available"); | |
| return Ok(None); | |
| }; | |
| frame_count += 1; | |
| if frame_count % 30 == 0 { | |
| debug!( | |
| "Windows camera encoder: processed {} frames", | |
| frame_count | |
| ); | |
| } | |
| return process_frame(frame, timestamp); | |
| } | |
| frame_count += 1; | |
| Ok(Some((texture.clone(), frame_time))) | |
| }, | |
| |output_sample| { | |
| let mut output = output.lock().unwrap(); | |
| let _ = muxer | |
| .write_sample(&output_sample, &mut *output) | |
| .map_err(|e| format!("WriteSample: {e}")); | |
| Ok(()) | |
| }, | |
| ) | |
| .context("run camera encoder")?; | |
| } | |
| let mut first_timestamp: Option<Duration> = None; | |
| let mut frame_count = 0u64; | |
| let mut process_frame = |frame: NativeCameraFrame, | |
| timestamp: Duration| | |
| -> windows::core::Result< | |
| Option<( | |
| windows::Win32::Graphics::Direct3D11::ID3D11Texture2D, | |
| TimeSpan, | |
| )>, | |
| > { | |
| let relative = if let Some(first) = first_timestamp { | |
| timestamp.checked_sub(first).unwrap_or(Duration::ZERO) | |
| } else { | |
| first_timestamp = Some(timestamp); | |
| Duration::ZERO | |
| }; | |
| let texture = upload_mf_buffer_to_texture(&d3d_device, &frame)?; | |
| Ok(Some((texture, duration_to_timespan(relative)))) | |
| }; | |
| if let Ok(Some((texture, frame_time))) = process_frame(first_frame.0, first_frame.1) | |
| { | |
| encoder | |
| .run( | |
| Arc::new(AtomicBool::default()), | |
| || { | |
| if frame_count > 0 { | |
| let Ok(Some((frame, timestamp))) = video_rx.recv() else { | |
| trace!("No more camera frames available"); | |
| return Ok(None); | |
| }; | |
| frame_count += 1; | |
| if frame_count % 30 == 0 { | |
| debug!( | |
| "Windows camera encoder: processed {} frames", | |
| frame_count | |
| ); | |
| } | |
| return process_frame(frame, timestamp); | |
| } | |
| frame_count += 1; | |
| Ok(Some((texture.clone(), frame_time))) | |
| }, | |
| |output_sample| { | |
| let mut output = output.lock().unwrap(); | |
| let _ = muxer | |
| .write_sample(&output_sample, &mut *output) | |
| .map_err(|e| format!("WriteSample: {e}")); | |
| Ok(()) | |
| }, | |
| ) | |
| .context("run camera encoder")?; | |
| } |
🤖 Prompt for AI Agents
crates/recording/src/output_pipeline/win.rs around lines 564 to 618: the closure
process_frame mutates the captured first_timestamp so it must be an FnMut;
change its binding from immutable to mutable (declare it with let mut
process_frame = ...), and update any internal call sites where necessary to call
the mutable closure (e.g., use a mutable binding or &mut process_frame inside
the nested closures) so the compiler can borrow it as &mut when invoked.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)
669-870: Refine save gating and preview updates inDefaultProjectNameCardTwo small but meaningful refinements here (both echoing earlier feedback):
isSaveDisabledusesinput.length <= 3as a guard (Line 728). That threshold is arbitrary and may block valid—but short—templates while not guaranteeing correctness. It would be cleaner to:
- Drop the length check and rely on non‑empty + “changed from current/default value”, or
- Replace it with a more semantic check (e.g., ensure at least one
{…}placeholder token is present via a simple pattern like/\{[^}]+\}/).
updatePreviewis invoked on every keystroke (Lines 787‑797) and always awaitscommands.formatProjectName. On slower machines or with transient TAURI latency, this can produce a lot of redundant calls and potential out‑of‑order preview updates. A short trailing debounce (e.g., ~200–300 ms with a cancelable timeout) keyed off the latestinputValuewould reduce work and keep the preview tightly in sync with what’s actually on screen.Neither is blocking, but tightening these up would make the template editor feel more predictable and robust.
apps/desktop/src-tauri/src/recording.rs (1)
355-429: Remove the inline comment; formatter implementation looks solidThe template-based formatter correctly handles mode/target placeholders plus
{date},{time}, and{moment:…}with custom formats, using cached regexes and a single replacement pass. The only issue is the inline comment on Line 381, which violates the no-comments-in-Rust guideline; the code is self-explanatory without it.Apply:
- // Get recording mode information let (recording_mode, mode) = match recording_mode {As per coding guidelines, comments should not be present in Rust code.
🧹 Nitpick comments (3)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)
108-110: Avoid hard‑coding the default template in multiple places
DEFAULT_PROJECT_NAME_TEMPLATEis now a front‑end constant and also drivesDefaultProjectNameCard’svalue/reset logic. Given the backend also defines a default project name template and uses it in migrations, consider centralizing this string (e.g., via a shared config/type surface or deriving the UI default from the stored value) so frontend/TAURI defaults cannot silently diverge over time.Also applies to: 577-582
apps/desktop/src-tauri/src/recording.rs (2)
446-458: Project naming and .cap bundle directory creation look correctUsing
format_project_namewith the optionaldefault_project_name_template, sanitizing the result (including colon replacement), appending.cap, and then callingensure_unique_filenamebeforeensure_dirgives you Windows-safe, non-clobbering project bundle directories. Wiringproject_file_pathintoRecordingMeta.project_path,pretty_name, and the logging handle keeps naming consistent through the recording lifecycle.One small clarity nit:
project_file_pathnow represents a directory rather than a file; consider renaming it toproject_dirorproject_pathin a follow-up for readability.Also applies to: 460-471, 475-476
623-624: Propagation of project_name via InProgressRecordingCommon is consistent but naming is a bit misleadingUsing
recording_dir = project_file_path.clone()for both Studio and Instant builders and storingproject_nameinInProgressRecordingCommon.target_namekeepsCompletedRecording::project_path()andCompletedRecording::target_name()aligned with the new project naming scheme, including in error paths andhandle_recording_finish.However,
target_nameinInProgressRecordingCommonnow effectively means “project name”, which can be confusing when reading the type. A later cleanup to rename this field (and any dependent API) toproject_namewould make the intent clearer.Also applies to: 676-677, 685-688, 749-753
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/recording.rs(16 hunks)apps/desktop/src/routes/(window-chrome)/settings/general.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
apps/desktop/src-tauri/src/recording.rsapps/desktop/src/routes/(window-chrome)/settings/general.tsx
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use Rust 2024 edition and run cargo fmt to format all Rust code with rustfmt
**/*.rs: Format all Rust code usingrustfmtand apply workspace clippy lints
Use snake_case for Rust module names and kebab-case for Rust crate names
Files:
apps/desktop/src-tauri/src/recording.rs
apps/desktop/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Files:
apps/desktop/src-tauri/src/recording.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src-tauri/src/recording.rsapps/desktop/src/routes/(window-chrome)/settings/general.tsx
apps/desktop/src/**/*.{ts,tsx,solid}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx,solid}: Use @tanstack/solid-query for server state in desktop app; call generated commands and events from tauri_specta
Never manually import desktop app icons; use unplugin-icons auto-import instead
Files:
apps/desktop/src/routes/(window-chrome)/settings/general.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use PascalCase for component names; use camelCase starting with 'use' for hook names
Use PascalCase for component names in TypeScript/JavaScript
Files:
apps/desktop/src/routes/(window-chrome)/settings/general.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce strict TypeScript; avoid 'any' type; leverage shared types from packages/utils and other shared packages
Files:
apps/desktop/src/routes/(window-chrome)/settings/general.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Biome for linting and formatting; run pnpm format and pnpm lint regularly and at the end of each coding session
**/*.{ts,tsx,js,jsx}: Use 2-space indentation in TypeScript files
Format all TypeScript and JavaScript code using Biome (viapnpm format)
Files:
apps/desktop/src/routes/(window-chrome)/settings/general.tsx
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.896Z
Learning: PRs must include clear description, linked issues, screenshots/GIFs for UI changes, env/migration notes, and keep scope tight
📚 Learning: 2025-11-29T04:31:05.289Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.289Z
Learning: Applies to **/*.{ts,tsx,js,jsx,rs,py,sh} : Never add any form of comments to code (single-line //, multi-line /* */, documentation ///, //!, /** */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Applied to files:
apps/desktop/src-tauri/src/recording.rs
📚 Learning: 2025-11-29T04:31:23.896Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.896Z
Learning: Applies to **/*.{ts,tsx,js,jsx,rs} : Never add comments to code in any language (no `//`, `/* */`, `///`, `//!`, `#`, etc.). Code must be self-explanatory through naming, types, and structure
Applied to files:
apps/desktop/src-tauri/src/recording.rs
📚 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:
apps/desktop/src-tauri/src/recording.rs
📚 Learning: 2025-11-29T04:31:05.289Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.289Z
Learning: Applies to apps/desktop/**/*.ts : Never edit auto-generated files: tauri.ts, queries.ts, and files under apps/desktop/src-tauri/gen/
Applied to files:
apps/desktop/src/routes/(window-chrome)/settings/general.tsx
📚 Learning: 2025-11-29T04:31:23.896Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.896Z
Learning: Applies to {**/tauri.ts,**/queries.ts,apps/desktop/src-tauri/gen/**} : Never edit auto-generated files including `**/tauri.ts`, `**/queries.ts`, and `apps/desktop/src-tauri/gen/**`
Applied to files:
apps/desktop/src/routes/(window-chrome)/settings/general.tsx
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/recording.rs (1)
crates/utils/src/lib.rs (3)
ensure_dir(22-25)moment_format_to_chrono(186-218)ensure_unique_filename(52-58)
⏰ 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). (1)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (7)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (2)
9-22: Collapsible and Solid imports align with new usageThe added
Collapsible,createSignal, andonMountimports are all exercised inDefaultProjectNameCardand the new help section; import surface stays minimal and consistent with existing patterns.
954-976:flex-shrink-0fix correctly stabilizes Excluded Windows actionsAdding
flex-shrink-0to the action container keeps the “Reset to Default” and “Add” buttons from shrinking or wrapping when the description copy grows, which directly addresses the overflow/ellipsis issue at certain viewport widths.apps/desktop/src-tauri/src/recording.rs (5)
26-32: New utility and regex imports are appropriateThe added imports for directory handling, template/date formatting, regexes, and
Cowalign with the new naming helper and its call sites. No issues here.
76-88: Studio recording now tracks camera_feed consistentlyAdding
camera_feed: Option<Arc<CameraFeedLock>>to theStudiovariant and wiring it through the actor setup keeps parity withInstantrecordings and correctly updatesstate.camera_in_use. The propagation looks coherent and safe.Also applies to: 676-677, 749-753
848-855: Updated handle_spawn_failure usage correctly cleans up on actor spawn errors and panicsPassing
project_file_path.as_path()intohandle_spawn_failureand routing both the error and panic branches through it ensures consistent emission ofRecordingEvent::Failedand proper cleanup viahandle_recording_end, even when the actor never starts successfully. The refactored call sites look correct and preserve the previous behavior with the new project-path naming.Also applies to: 861-867, 907-908
1124-1135: Screenshot flow’s naming and project bundle handling align with recordingsUsing
format_project_namewithRecordingMode::Screenshotplus the same sanitize/.cap/ensure_unique_filenamepattern brings screenshot bundles in line with the recording naming scheme. Creating a dedicated.capdirectory per screenshot, storing the image under it, and saving bothRecordingMetaandProjectConfigurationagainstproject_file_pathkeeps the data model consistent with video projects.The
cap_dir_keyand paths used for pending screenshot tracking and finalpretty_nameall match the new bundle directory.Also applies to: 1145-1156, 1190-1192, 1203-1204, 1158-1161
1802-1808: Initializing new mask_segments and text_segments fields to empty is safeExtending
TimelineConfigurationto includemask_segmentsandtext_segments, initialized toVec::new(), preserves existing behavior while making room for the new editor features. Since you already rebuildtimelinefrom scratch here, explicitly zero-initializing these new vectors is appropriate.
2 participants
This PR adds a new recording setting: Default project name which allows for customizing the default project and file name using a custom template with placeholders.
There are several place holders for different info about a recording, and a custom date placeholder which accepts a moment format.
Renaming the project through the editor still has the same behavior, only the pretty name changes, not the actual name of the file.
There's a new module
update_project_nameswhich updates the name of all previous project files to use their pretty name instead. Once done, it addsuuid_projects_migratedto the data store.Other changes:
Note: I'm not sure how to format the Cargo.toml file, I don't even know why it changed.
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.