(feat) Customize default project name, update project file names from UUID to pretty name #1372
Conversation
…owsCard + Fix button text spilling
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a customizable project-name template (formatting + chrono token conversion), a migration that renames UUID-based project directories to pretty sanitized names, utilities for unique filename generation and token replacement (Aho-Corasick), Tauri command exposure, frontend settings UI for templates, and related dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend (Settings / Editor)
participant Tauri as Tauri Backend
participant Recording as Recording Module
participant Migration as Migration Module
participant FS as File System
rect rgb(240,248,255)
Note over User,Frontend: Configure default project-name template
User->>Frontend: Edit & preview template
Frontend->>Tauri: Save default_project_name_template
Tauri->>Tauri: Persist in GeneralSettingsStore
end
rect rgb(240,255,240)
Note over Tauri,Migration: App startup migration
Tauri->>Migration: migrate_if_needed()
Migration->>FS: Check migration flag / list recordings
alt UUID projects found
loop each UUID project
Migration->>FS: Load RecordingMeta
Migration->>Migration: Compute sanitized pretty name
Migration->>Migration: ensure_unique_filename()
Migration->>FS: Rename project directory
end
Migration->>Tauri: Mark migration complete
end
end
rect rgb(255,250,240)
Note over User,Recording: Start recording flow
User->>Tauri: Start recording request
Tauri->>Recording: start_recording()
Recording->>Recording: format_project_name(template?, target_name, kind, mode, datetime?)
Recording->>Recording: moment_format_to_chrono() -> date tokens
Recording->>Recording: ensure_unique_filename() for project dir
Recording->>FS: Create project directory / files (sanitized name)
Recording->>Tauri: Return project_name / metadata
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 0
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/lib.rs (1)
3062-3078: LGTM! Command implementation is correct.The command properly delegates to
recording::format_project_namewith correct parameter type conversions. The async marker is unnecessary since no async operations are performed, but this is a minor inefficiency and may be intentional for consistency with other commands.If desired, you can remove the
asyncmarker for a slight performance improvement:-#[tauri::command] -#[specta::specta] -async fn format_project_name( +#[tauri::command(async)] +#[specta::specta] +fn format_project_name( template: Option<String>, target_name: String, target_kind: String, recording_mode: RecordingMode, datetime: Option<chrono::DateTime<chrono::Local>>, ) -> String {Note: The
#[tauri::command(async)]attribute makes the command callable asynchronously from the frontend while keeping the Rust implementation synchronous.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src-tauri/src/lib.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
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(16-41)
⏰ 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 (3)
apps/desktop/src-tauri/src/lib.rs (3)
28-28: LGTM! Module declaration is appropriate.The new
update_project_namesmodule is correctly declared and used in the setup function to perform one-time project folder migration.
2447-2450: Past review concern has been addressed.The migration error handling has been updated since the previous review. The code now correctly catches and logs migration errors without aborting app startup, which is appropriate for this cosmetic one-time rename operation.
2303-2303: LGTM! Command registration is correct.The
format_project_namecommand is properly registered in the specta builder and will be available to the frontend via TypeScript bindings.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/desktop/src-tauri/src/recording.rs (1)
451-451: Consider non-padded hour format for consistency.The default time format uses
%I:%M %p, which produces zero-padded hours (e.g., "03:23 PM"). If the intent is to display non-padded hours (e.g., "3:23 PM"), use%-I:%M %pinstead.Based on past review comment that noted this discrepancy.
Apply this diff if non-padded hours are preferred:
- .unwrap_or(Cow::Borrowed("%I:%M %p")), + .unwrap_or(Cow::Borrowed("%-I:%M %p")),
🧹 Nitpick comments (1)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)
794-796: Consider debouncing the preview updatesThe
onInputhandler triggersupdatePreviewon every keystroke, which invokes a Tauri command. As noted in the previous review, adding a trailing-edge debounce (200-300ms) would reduce redundant round-trips during fast typing without changing the perceived behavior.Apply this approach to add debouncing:
+const [debounceTimer, setDebounceTimer] = createSignal<number | undefined>(); + async function updatePreview(val = inputValue()) { const formatted = await commands.formatProjectName( val, macos ? "Safari" : "Chrome", "Window", "instant", datetime, ); setPreview(formatted); }Then update the onInput handler:
onInput={(e) => { setInputValue(e.currentTarget.value); - updatePreview(e.currentTarget.value); + const timer = debounceTimer(); + if (timer !== undefined) clearTimeout(timer); + setDebounceTimer(setTimeout(() => { + updatePreview(e.currentTarget.value); + }, 250) as unknown as number); }}
📜 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 (6)
apps/desktop/src-tauri/Cargo.toml(4 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/Cargo.toml
- apps/desktop/src/utils/tauri.ts
- packages/ui-solid/src/auto-imports.d.ts
🧰 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/lib.rsapps/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/lib.rsapps/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/lib.rsapps/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/lib.rsapps/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 (2)
📚 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/**/*.rs : Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Applied to files:
apps/desktop/src-tauri/src/lib.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
🧬 Code graph analysis (2)
apps/desktop/src-tauri/src/lib.rs (4)
apps/desktop/src-tauri/src/recording.rs (3)
format_project_name(396-469)app(1179-1179)target_name(266-271)apps/desktop/src-tauri/src/update_project_names.rs (1)
migrate_if_needed(16-41)apps/desktop/src-tauri/src/upload.rs (2)
app(712-713)app(794-795)apps/desktop/src/utils/tauri.ts (1)
RecordingMode(475-475)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (3)
apps/desktop/src/utils/tauri.ts (1)
commands(7-310)packages/ui-solid/src/Button.tsx (1)
Button(40-50)apps/desktop/src/routes/editor/ui.tsx (1)
Input(151-161)
⏰ 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 (11)
apps/desktop/src-tauri/src/recording.rs (2)
26-33: LGTM!The new imports support the project name formatting functionality and are appropriately scoped.
486-508: LGTM!The integration of
format_project_nameinto the recording flow is well-structured:
- Properly retrieves template from settings
- Generates project name from template
- Sanitizes and ensures filename uniqueness
- Consistently uses the generated name throughout the recording setup
Also applies to: 530-530, 572-572, 725-725
apps/desktop/src-tauri/src/lib.rs (4)
29-29: LGTM!Module declaration is correct and necessary for the project name migration functionality.
2355-2356: LGTM!The
format_project_namecommand is correctly registered in the tauri_specta command list, following the established pattern.
2502-2504: LGTM!The migration error handling is correct. By logging the error rather than propagating it, the app can start even if the one-time project name migration fails. This prevents the migration from blocking users.
3126-3142: LGTM!The tauri command implementation is well-structured:
- Properly annotated for async tauri command and specta type generation
- Correctly converts owned string parameters to references when delegating to
recording::format_project_name- No coding guideline violations (no comments)
- Appropriate signature for IPC boundary
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (5)
9-9: LGTM: Imports and constant declarationThe new imports and default template constant are appropriate for the DefaultProjectNameCard component functionality.
Also applies to: 17-17, 19-19, 108-109
577-582: LGTM: Component integrationThe DefaultProjectNameCard is properly integrated with the settings flow, correctly wiring the persisted value and change handler.
777-780: Good: Save handler properly awaits async operationsThe Save button handler correctly awaits both
props.onChangeandupdatePreview, addressing the concern from the previous review. This ensures the UI preview reflects the persisted state.
953-953: Good fix: Prevent button container from shrinkingAdding
flex-shrink-0prevents the button text from overflowing at certain viewport sizes, as noted in the PR objectives.
669-869: Well-structured component with comprehensive documentationThe DefaultProjectNameCard component is well-implemented with:
- Clear state management using signals
- Live preview functionality via the formatProjectName command
- Helpful collapsible documentation with interactive code examples
- Proper async handling in button callbacks
The component provides a good user experience for customizing project name templates.
| const isSaveDisabled = () => { | ||
| const input = inputValue(); | ||
| return ( | ||
| !input || | ||
| input === (props.value ?? DEFAULT_PROJECT_NAME_TEMPLATE) || | ||
| input.length <= 3 | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Arbitrary length threshold in save validation
The isSaveDisabled check uses input.length <= 3 as one of its conditions. This threshold appears arbitrary—it would block "Hi" but allow "Test", neither of which is necessarily a valid template format. Consider either removing this check entirely (relying on the empty check and template validation in the backend) or adding a more meaningful validation (e.g., checking for at least one placeholder token).
🤖 Prompt for AI Agents
In apps/desktop/src/routes/(window-chrome)/settings/general.tsx around lines 723
to 730, the save-disable logic uses an arbitrary length check (`input.length <=
3`) which should be replaced with a meaningful validation: remove the length
threshold and instead ensure the template is non-empty, not equal to the
current/default value, and contains at least one placeholder token (e.g. match a
pattern like `{token}`) before allowing save; keep backend validation but update
this frontend check to look for a placeholder token (regex `\{[^}]+\}`) rather
than relying on string length.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src-tauri/src/recording.rs (2)
355-429: Consider validating template output for empty/whitespace-only results.The function doesn't validate whether the processed template produces a non-empty string. If a user provides an unusual template (e.g., only whitespace or placeholders that resolve to empty strings), the downstream sanitization on Line 461 might produce an empty or minimal filename, potentially causing issues with directory creation.
Consider adding validation after Line 428:
result.into_owned() + .trim() + .to_string() + .or_else(|s| if s.is_empty() { + "{target_name} ({target_kind}) {date} {time}".to_string() + } else { + s + }) }Actually, a simpler approach:
- result.into_owned() + let result = result.into_owned(); + if result.trim().is_empty() { + format_project_name( + Some(DEFAULT_FILENAME_TEMPLATE), + target_name, + target_kind, + recording_mode, + Some(datetime), + ) + } else { + result + } }
683-687: Project name correctly assigned to recording common structure.Setting
common.target_nametoproject_name(Line 684) propagates the formatted name throughout the recording flow. While the field nametarget_namemight suggest it holds the capture target's name, it effectively serves as the recording's identifier, so this usage is functionally correct.For future clarity, consider renaming
InProgressRecordingCommon.target_nametoInProgressRecordingCommon.recording_nameorproject_nameto better reflect its purpose. This is a low-priority refactor since the current usage is correct.
📜 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 (4)
**/*.{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.rs
**/*.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.rs
🧠 Learnings (2)
📚 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
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/recording.rs (1)
crates/utils/src/lib.rs (2)
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 (4)
apps/desktop/src-tauri/src/recording.rs (4)
26-32: LGTM on new imports.The imports appropriately support the template-based project naming feature:
lazy_staticfor compiled regex/Aho-Corasick patterns,moment_format_to_chronofor date format conversion, andCowfor efficient string handling.
446-468: Well-structured project name generation and path handling.The implementation correctly:
- Retrieves the custom template from settings with a sensible fallback
- Handles missing target titles with "Unknown"
- Sanitizes the filename to prevent path issues
- Ensures uniqueness to avoid directory collisions
- Properly constructs the recording directory path
The explicit colon-to-period replacement on Line 460 before sanitization ensures consistent Windows compatibility, though
sanitize_filenameshould also handle this.
486-507: Good alignment of video upload name with project name.Passing the formatted
project_nametocreate_or_get_video(Line 490) ensures the backend video record uses a meaningful, user-customizable name instead of a timestamp or UUID, improving consistency between local and remote naming.
529-555: Correct propagation of project name through metadata.Setting
pretty_nametoproject_name(Line 532) ensures the recording metadata reflects the formatted, customizable project name. This properly integrates the new naming system with the project metadata structure.
| 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() | ||
| }); |
There was a problem hiding this comment.
Time format produces zero-padded hours on Windows.
The default time format %I:%M %p on Line 411 produces zero-padded hours like "03:23 PM" on all platforms. If documentation or UI examples show non-padded hours like "3:23 PM", there's a mismatch. The Unix-specific %-I flag for non-padded hours doesn't work on Windows, creating a cross-platform inconsistency.
Options to resolve:
- Accept zero-padding (simplest): Update any documentation/examples to show "03:23 PM"
- Platform-specific formats:
+#[cfg(not(windows))]
+const DEFAULT_TIME_FORMAT: &str = "%-I:%M %p";
+#[cfg(windows)]
+const DEFAULT_TIME_FORMAT: &str = "%I:%M %p";
+
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")),
+ .unwrap_or(Cow::Borrowed(DEFAULT_TIME_FORMAT)),
)
.to_string()
});- Post-process to remove leading zeros (more complex, affects all platforms)
Would you like me to implement one of these solutions?
There was a problem hiding this comment.
Chrono's docs don't mention anything about it being platform specific. I'm pretty sure it's locale based. The frontend calls the same format function for showing examples so it's always showing the true result.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
rust chrono strftime format flags platform specific
💡 Result:
Short answer: Chrono provides its own strftime-like implementation with a stable, cross‑platform set of format specifiers (so you generally do not get the platform-dependent oddities of C's strftime). A few notes/compatibility points:
- Chrono documents the complete set of supported specifiers and padding modifiers (%, %- , %_ , %0 , etc.) and implements them consistently across platforms. [1][2]
- Some semantics intentionally differ from C/OS implementations: e.g. chrono’s %s (seconds since epoch) and fractional‑seconds (%f / %.3f / %.6f / %.9f) have chrono‑defined padding/behavior; %+ is given a stable ISO‑like form in chrono (unlike platform/locale C implementations) but is advised to be avoided if you need exact control. [1]
- Locale‑dependent specifiers (like %x, %X, %c, %r) are only localized when you opt into chrono’s locale feature (unstable‑locales); otherwise chrono uses its own defaults. [1][3]
- If you need exact parity with a C library’s strftime (glibc, musl, Windows CRT), don’t assume full correspondence — consult chrono’s specifiers in chrono::format::strftime or the source. [1][4]
Sources:
- Chrono strftime docs and source (specifiers, modifiers, notes about %+, %s, %f and locales). [1][3][4]
- C/C++ strftime platform differences reference (for contrast). [2]
@ItsEeleeya You're absolutely right, and thank you for the correction! I confused chrono with C's strftime, which does have platform-specific limitations.
Chrono provides its own cross-platform implementation with stable format specifiers that work consistently across all platforms, including padding modifiers like %-. The behavior is locale-based (when the unstable-locales feature is enabled), not platform-specific.
Since the frontend uses the same format_project_name function for examples, there's complete consistency between what users see in the UI and the actual generated filenames. The current implementation is correct as-is.
My apologies for the confusion!
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.
| 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.
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.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/desktop/src-tauri/src/recording.rs (1)
382-382: Remove inline comment per coding guidelines.Line 382 contains
// Get recording mode informationwhich violates the no-comments rule for Rust files.As per coding guidelines, remove all comments from Rust code:
- // Get recording mode information let (recording_mode, mode) = match recording_mode {
🧹 Nitpick comments (1)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (1)
405-417: RedundantfindIndexlookup—iis already the segment index.Since
Indexprovides the indexidirectly, thefindIndexcall on lines 409-411 is unnecessary. You can simplify by usingidirectly.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, - ); - - if (segmentIndex === undefined || segmentIndex === -1) - return false; - - return selection.indices.includes(segmentIndex); + return selection.indices.includes(i); });
📜 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 (4)
apps/desktop/src-tauri/src/lib.rs(4 hunks)apps/desktop/src-tauri/src/recording.rs(13 hunks)apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx(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 (5)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
apps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/lib.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src-tauri/src/recording.rsapps/desktop/src/routes/editor/Timeline/ZoomTrack.tsxapps/desktop/src-tauri/src/lib.rs
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Use React Query hooks with Server Actions for mutations and perform precise cache updates using setQueryData/setQueriesData instead of broad invalidations
Files:
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use strict TypeScript; avoid any type; leverage shared types from @cap/* packages
Follow camelCase naming for variables and functions; PascalCase for components; hooks must start with 'use' prefix
Use Biome for linting and formatting; match existing formatting conventions in the codebase
Use Tailwind CSS for styling in web components; stay consistent with spacing and tokens
Use static skeletons for loading states that mirror content; avoid bouncing animations
Memoize expensive work, code-split naturally, and use Next/Image for remote assets
**/*.{ts,tsx,js,jsx}: Use 2-space indent for TypeScript files; format with Biome usingpnpm format
Use Biome for code formatting and linting; runpnpm formatregularly
Use kebab-case for file names (e.g.,user-menu.tsx); use PascalCase for components
Files:
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never add any form of comments to code (single-line //, multi-line /* /, JSDoc /* */, or any other comment syntax); code must be self-explanatory through naming, types, and structure
Directory naming must use lowercase-dashed convention
Files:
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.{ts,tsx,js,jsx,rs} : Never add comments to code (`//`, `/* */`, `///`, `//!`, `#`, etc.); code must be self-explanatory through naming, types, and structure
Applied to files:
apps/desktop/src-tauri/src/recording.rs
📚 Learning: 2025-12-07T14:29:19.165Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.165Z
Learning: Applies to apps/web/**/*.tsx : Minimize useEffect usage in React components; compute during render, handle logic in event handlers, and ensure cleanups for subscriptions/timers
Applied to files:
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx
🧬 Code graph analysis (2)
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)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (1)
apps/desktop/src/routes/editor/Timeline/Track.tsx (3)
SegmentRoot(55-96)SegmentHandle(112-134)SegmentContent(98-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
apps/desktop/src-tauri/src/lib.rs (2)
2538-2541: Migration error handling looks good.The migration now logs errors without blocking app startup, which is the correct approach for a one-time cosmetic migration. This addresses the concern from the previous review.
3163-3179: Tauri command wrapper is clean and correct.The
format_project_namecommand properly converts ownedStringparameters to&strreferences using.as_deref()and.as_str()before delegating to the underlying implementation. This provides a clean API boundary for the frontend.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 8
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| > | ||
| <Show | ||
| when={(project.timeline?.zoomSegments ?? []).length > 0} | ||
| when={project.timeline?.zoomSegments} |
There was a problem hiding this comment.
Bug: Empty zoomSegments array hides fallback UI message
The Show component's when condition was changed from (project.timeline?.zoomSegments ?? []).length > 0 to just project.timeline?.zoomSegments. In JavaScript, an empty array [] is truthy, so when zoomSegments is an empty array (no segments yet but the array exists), the fallback message "Click to add zoom segment" won't display. Previously, the message would show for both undefined/null AND empty arrays, guiding users on how to add their first segment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src-tauri/src/update_project_names.rs (1)
31-41: Consider not flippinguuid_projects_migratedwhenmigratereturnsErrRight now, even if
migrate(&app_handle).awaitreturnsErr(e.g., a fatalread_dir/try_existsfailure), the code still setsuuid_projects_migrated = trueand saves the store. That permanently disables retries even though no projects were actually processed in that failure mode. Given the key name implies “migration completed”, you may want to gate the flag update onmigratereturningOk(())so catastrophic failures can be retried on next startup.
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/update_project_names.rs (1)
300-325: Tests cover the UUID filename helper well, but comments conflict with guidelinesThe unit test exercises both valid and several invalid cases for
fast_is_project_filename_uuid, which is sufficient coverage for this helper.However, the in-test
//comments (and other///comments elsewhere in this file) conflict with the repo guideline for*.rsfiles to avoid comments entirely and rely on naming and structure instead. Consider dropping these comments and letting the test function and assertion names speak for themselves to stay aligned with that convention.
📜 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
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
apps/desktop/src-tauri/src/update_project_names.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src-tauri/src/update_project_names.rs
🧠 Learnings (3)
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.rs : Use snake_case for Rust module names and kebab-case for crate names
Applied to files:
apps/desktop/src-tauri/src/update_project_names.rs
📚 Learning: 2025-12-07T14:29:19.165Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.165Z
Learning: Applies to **/*.rs : Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Applied to files:
apps/desktop/src-tauri/src/update_project_names.rs
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.rs : Never write `let _ = async_fn()` which silently drops futures; await or explicitly handle them (Clippy: `let_underscore_future` = deny)
Applied to files:
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). (3)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (5)
apps/desktop/src-tauri/src/update_project_names.rs (5)
50-151: Async migration pipeline, concurrency limit, and metrics look solidThe overall
migrateflow is well-structured: existence check viatry_exists, UUID project collection, adaptive concurrency limit viaavailable_parallelism().clamp(2, 16).min(total_found), and aggregation of migrated/skipped/failed counts with wall-clock and per-project timings. Usingbuffer_unorderedhere is appropriate and the instrumentation (including slowest-N logging) should make diagnosing migration behavior straightforward. No correctness issues stand out in this block.
153-179: UUID project collection logic is straightforward and correctDirectory scanning with
tokio::fs::read_dir, filtering only directories, and then requiring both.capsuffix andfast_is_project_filename_uuidgives a clear and tight definition of migration candidates. Error handling is consistent (String-based with context), and the function returns an ownedVec<PathBuf>suitable for the later concurrent processing.
187-251: Per-project migration and base-name locking approach look goodThe per-project flow (
migrate_single_project) correctly:
- Derives the user-facing name from
RecordingMeta::load_for_project.- Sanitizes to a filesystem-safe base and uses an
Arc<Mutex<HashSet<String>>>to serialize migrations sharing the same base, reducing uniqueness/rename races.- Ensures the lock is always released (via the
removecall afterawait) and maps outcomes to Migrated/Skipped/Failed with appropriate tracing.This is a reasonable balance between concurrency and avoiding conflicting renames; no functional issues stand out.
253-280: Filename sanitization and uniqueness handling are correct
migrate_project_filename_asynccleanly:
- Reuses the sanitized pretty name, normalizing “.cap” extension handling.
- Uses the parent directory of the project as the uniqueness scope.
- Delegates uniqueness to
cap_utils::ensure_unique_filenameand then performs a singlefs::renamewith proper async error handling.The logic should behave well even when multiple projects have similar names, and the error messages are informative.
282-298: UUID filename detection is tight and efficient
fast_is_project_filename_uuidcorrectly enforces:
- Fixed length of 40 bytes and
.capsuffix.- Hyphens at positions 8, 13, 18, and 23 within the 36-byte UUID portion.
- All characters in the UUID section are ASCII hex digits or hyphens.
This is a nice, allocation-free check that aligns with the intended UUID format and is well-covered by the tests below.
| pub fn migrate_if_needed(app: &AppHandle) -> Result<(), String> { | ||
| let store = app | ||
| .store("store") | ||
| .map_err(|e| format!("Failed to access store: {}", e))?; | ||
|
|
||
| if store | ||
| .get(STORE_KEY) | ||
| .and_then(|v| v.as_bool()) | ||
| .unwrap_or(false) | ||
| { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let app_handle = app.clone(); | ||
| tokio::spawn(async move { | ||
| if let Err(err) = migrate(&app_handle).await { | ||
| tracing::error!("Updating project names failed: {err}"); | ||
| } | ||
|
|
||
| if let Ok(store) = app_handle.store("store") { | ||
| store.set(STORE_KEY, true); | ||
| if let Err(e) = store.save() { | ||
| tracing::error!("Failed to save store after migration: {}", e); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src-tauri/src/update_project_names.rs | sed -n '15,50p'Repository: CapSoftware/Cap
Length of output: 174
🏁 Script executed:
rg "tokio::spawn" apps/desktop/src-tauri/src/update_project_names.rs -A5 -B2Repository: CapSoftware/Cap
Length of output: 174
🏁 Script executed:
# Check Cargo.toml for tokio and clippy lint config
cat apps/desktop/src-tauri/Cargo.toml | grep -A 20 "\[dependencies\]" | grep tokioRepository: CapSoftware/Cap
Length of output: 174
🏁 Script executed:
# Check workspace root for clippy deny config
find . -maxdepth 2 -name "Cargo.toml" -exec grep -l "deny\|warn.*unused_must_use" {} \;Repository: CapSoftware/Cap
Length of output: 174
Bind the tokio::spawn JoinHandle to avoid ignoring a #[must_use] type
tokio::spawn returns a JoinHandle<T> marked #[must_use]. Calling it as a bare statement at line 32 discards this handle, which violates the coding guideline requiring all #[must_use] types to be handled explicitly. Bind the handle to an underscore-prefixed variable to acknowledge the fire-and-forget intent:
- tokio::spawn(async move {
+ let _migration_task = tokio::spawn(async move {
if let Err(err) = migrate(&app_handle).await {
tracing::error!("Updating project names failed: {err}");
}
if let Ok(store) = app_handle.store("store") {
store.set(STORE_KEY, true);
if let Err(e) = store.save() {
tracing::error!("Failed to save store after migration: {}", e);
}
}
- });
+ });🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/update_project_names.rs around lines 17 to 45, the
tokio::spawn JoinHandle is currently discarded which violates the #[must_use]
requirement; bind the returned handle to an underscore-prefixed variable (e.g.,
let _handle = tokio::spawn(...);) to explicitly acknowledge the fire-and-forget
intent, keeping the spawned task behavior unchanged and avoiding the ignored
#[must_use] warning.
| > | ||
| <Show | ||
| when={(project.timeline?.zoomSegments ?? []).length > 0} | ||
| when={project.timeline?.zoomSegments} |
There was a problem hiding this comment.
Bug: Empty zoom segments array no longer shows fallback UI
The Show component's when condition changed from (project.timeline?.zoomSegments ?? []).length > 0 to project.timeline?.zoomSegments. In JavaScript, an empty array [] is truthy, so when zoomSegments exists but is empty, the fallback UI ("Click to add zoom segment") will no longer be displayed. Users will see an empty component instead of the helpful instruction text. The condition likely needs to check for array length to maintain the original behavior.
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
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Adds a customizable project-name template (with date/time placeholders) and migrates existing UUID-named projects to sanitized, human-readable names.
format_project_name(Tauri command) and use it to generatepretty_nameand project directory (.cap) names for recordings and screenshots.{recording_mode|mode|target_kind|target_name}and date/time{date},{time},{moment:...}with moment-like formats; sanitize and ensure unique filenames.ScreenCaptureTarget::kind_str.general_settings.default_project_name_templatewith UI (live preview, helper docs) in General Settings.update_project_namesmodule: on startup, one-time async migration renames UUID-named project folders to sanitizedpretty_name(tracked viauuid_projects_migrated).cap_utils::ensure_unique_filenameandmoment_format_to_chrono(+ tests).tauri.ts; updateGeneralSettingsStoreschema.aho-corasick,regex,sanitize-filename; enablespectachronofeature.Written by Cursor Bugbot for commit ead1b32. This will update automatically on new commits. Configure here.