feat: Misc performance and UX fixes #1390
Conversation
WalkthroughRefactors project-config saves to an explicit, stateful debounce/schedule+flush flow; adds macOS ScreenCaptureKit handling for excluded window IDs with pruning and retry logic; and replaces per-frame canvas waveform drawing with Path2D-based, memoized waveform rendering and precomputed segment offsets. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Editor Component
participant State as Save State
participant Timer as Debounce Timer
participant Commands as setProjectConfig
Component->>State: change detected
alt saveInFlight == true
State->>State: set shouldResave
else
State->>Timer: schedule (250ms)
end
Timer->>State: timer fires
State->>State: saveInFlight = true
State->>Commands: execute setProjectConfig
Commands-->>State: returns
State->>State: saveInFlight = false
alt shouldResave
State->>Timer: schedule next save
end
Component->>State: onCleanup (unmount)
State->>Timer: clear timer
State->>Commands: flush if pending
sequenceDiagram
participant Flow as start_recording
participant Settings as General Settings
participant Acquire as acquire_shareable_content_for_target
participant Prune as prune_excluded_windows
participant Builders as macOS Builders
Flow->>Settings: load excluded_windows
Flow->>Acquire: acquire(content, excluded_windows)
Acquire->>Acquire: check missing target display?
Acquire->>Acquire: check missing excluded windows?
alt missing display or windows
Acquire-->>Flow: request refresh / retry
else
Acquire-->>Flow: return content
end
Flow->>Prune: prune excluded_windows by content
Prune-->>Flow: updated excluded_windows
Flow->>Builders: pass shareable_content + excluded_windows
alt builder error (shareable content)
Flow->>Acquire: re-acquire with updated exclusions
Flow->>Prune: re-prune
Flow->>Builders: retry
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 0
🧹 Nitpick comments (3)
apps/desktop/src-tauri/src/recording.rs (3)
34-35: Pruning helper is sound; consider documenting semantics
prune_excluded_windows_without_shareable_contentis implemented safely: usingmem::takeavoids in-place mutation pitfalls, and dropping any IDs that can’t be resolved or don’t appear in the currentShareableContent(with aremovedcounter) is exactly what you want for keeping the exclusion list in sync.As this will be subtle to reason about later (it removes both “never-resolvable” and “no longer present” windows), a short doc comment explaining that behavior would help future maintainers.
Also applies to: 68-69, 191-230
695-715: macOS excluded-window initialization and pruning fit well into the actor setupInitializing
excluded_windowsfrom general settings, resolving them viaresolve_window_ids, and immediately callingprune_excluded_windows_without_shareable_contentagainst the freshly acquiredshareable_contentensures the pipeline starts with a minimal, valid exclusion set. That should reduce the chance of later ScreenCaptureKit filter errors and keep the occlusion behavior aligned with the UI settings.One thing to keep in mind for future performance work: all of this currently runs while the state write lock is held inside the actor-spawn path. If lock contention around recording startup ever shows up in profiling, this block (resolving/pruning exclusions and acquiring shareable content) would be a good candidate to move outside the critical section.
Also applies to: 717-721
840-851: Guard against unbounded retries on shareable-content errorsOn macOS,
is_shareable_content_errortriggers a re-acquire ofshareable_contentand re-pruning ofexcluded_windows, thencontinue-s the loop. IfSourceError::AsContentFilterkeeps occurring even after refresh (for example due to a persistent OS-level issue), this loop can theoretically retry forever.Consider adding a small attempt counter or time-based cutoff for this specific retry path, and after exceeding it, surface a more explicit error to the user instead of looping indefinitely. This keeps the behavior robust even when ScreenCaptureKit misbehaves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src-tauri/src/recording.rs(8 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
🧠 Learnings (1)
📚 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
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/recording.rs (3)
crates/recording/src/sources/screen_capture/mod.rs (1)
window(74-79)crates/scap-targets/src/platform/macos.rs (1)
from_id(48-51)crates/scap-targets/src/lib.rs (2)
from_id(32-34)from_id(122-124)
⏰ 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). (2)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
apps/desktop/src-tauri/src/recording.rs (1)
111-147: Shareable content acquisition and missing-window handling look correctThe new
acquire_shareable_content_for_targetlogic plusshareable_content_missing_windowsgives a clear, bounded retry: one refresh on stale ScreenCaptureKit content, withDisplayMissingsurfaced cleanly and excluded-window mismatches tolerated after a refresh. The distinction between display-missing and window-missing cases, and only treating known-but-unresolved windows as “missing”, matches the later pruning behavior and should avoid spurious failures.Also applies to: 164-189
richiemcilroy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (1)
46-94: Consider defensive bounds checking and add documentation.The waveform path generation logic is sophisticated but could be clearer:
Line 82:
amplitudeAt(index - 1)on the first iteration may accesswaveform[-1]whensegment.startis 0. While safely handled by theamplitudeAtfunction's type checks, consider clamping:Math.max(index - 1, 0).The bezier curve construction with control points and the coordinate normalization could benefit from inline comments explaining the approach.
Apply this diff to add bounds checking:
const prevY = 1 - amplitudeAt(index - 1); + const prevY = 1 - amplitudeAt(Math.max(index - 1, 0));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/routes/editor/Timeline/ClipTrack.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/routes/editor/Timeline/ClipTrack.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (3)
crates/editor/src/audio.rs (1)
gain(63-65)crates/project/src/configuration.rs (2)
duration(526-528)duration(591-593)crates/editor/src/segments.rs (1)
segments(7-36)
⏰ 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). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (5)
33-44: LGTM! Good constants and scaling logic.The waveform constants are well-defined and the
gainToScalefunction correctly converts decibel gain to a linear scale factor for visualization purposes.
110-171: Excellent Path2D-based rendering optimization!The refactor to use memoized Path2D objects with transform-based scaling is a solid performance improvement over per-frame waveform calculations. The reactive dependencies are correctly tracked.
Minor note: Line 136's
canvas.width = canvasWidthalready clears the canvas, making line 138'sclearRectredundant (though harmless).
196-205: LGTM! Excellent performance optimization.Precomputing segment offsets eliminates redundant calculations across multiple segments. The cumulative offset logic correctly accounts for timescale and provides O(1) lookups.
233-233: LGTM! Clean integration with segmentOffsets.Using the precomputed offsets is more efficient and the nullish coalescing provides good safety.
489-495: LGTM! Appropriate conditional rendering.Limiting waveform rendering to segments with
timescale === 1is sensible, as time-stretched segments would require different waveform handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/recording/src/sources/camera.rs (1)
53-66: Consider adding logging when reattachment occurs.The reattachment logic provides good resilience when the camera sender disconnects, but there's no info-level log when reattachment succeeds. This makes it difficult to diagnose intermittent disconnection issues in production.
Consider adding an info log after successful reattachment:
Err(_) => { let (new_tx, new_rx) = flume::bounded(8); if let Err(err) = feed_lock.ask(camera::AddSender(new_tx)).await { warn!( ?err, "Camera sender disconnected and could not be reattached" ); break Err(err); } + info!( + frame_count, + "Camera sender disconnected; successfully reattached" + ); receiver = new_rx; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.claude/settings.local.json(1 hunks)crates/recording/src/sources/camera.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
crates/recording/src/sources/camera.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/camera.rs
🧠 Learnings (1)
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/recording/src/sources/camera.rs
🧬 Code graph analysis (1)
crates/recording/src/sources/camera.rs (1)
crates/recording/src/feeds/camera.rs (1)
drop(175-179)
⏰ 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: Analyze (rust)
🔇 Additional comments (3)
.claude/settings.local.json (1)
7-8: Approve: Configuration update is consistent.The addition of
"Bash(cargo build:*)"follows the established pattern of wildcard permissions for build tool operations and complements the existing"Bash(cargo check:*)"entry.crates/recording/src/sources/camera.rs (2)
37-37: LGTM! Good observability improvement.The frame counting and enhanced error logging provide valuable diagnostics for tracking camera forwarding behavior. Including the frame count in error logs will help identify whether issues occur at startup or during sustained operation.
Also applies to: 42-50
68-76: LGTM! Explicit cleanup with observable logging.The explicit drop statements and final info log provide clear lifecycle boundaries and telemetry. This makes it easier to diagnose camera forwarding issues and ensures resources are cleaned up promptly.
|
|
||
| feed_lock | ||
| .ask(camera::AddSender(tx)) | ||
| .ask(camera::AddSender(tx.clone())) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the inconsistent sender ownership pattern between initial setup and reattachment.
There's an inconsistency in how senders are passed to AddSender:
- Line 29:
tx.clone()is passed, keeping the original for explicit cleanup at line 69 - Line 55:
new_txis passed directly, transferring ownership to the camera feed
After reattachment, the forwarding task no longer holds a reference to the active sender (new_tx), only to the original tx which may already be disconnected. When the task exits:
- The original
txis explicitly dropped (line 69), but it may no longer be connected - The
new_txis owned by the camera feed and will only be implicitly disconnected whenreceiver(nownew_rx) is dropped at line 70
This creates an asymmetry in cleanup behavior depending on whether reattachment occurred.
Run the following script to check how AddSender is used elsewhere and whether cloning is the expected pattern:
Consider whether line 55 should use new_tx.clone() to maintain consistency, and track the current sender in a mutable variable that gets updated during reattachment and dropped at line 69.
Also applies to: 55-55, 69-69
🏁 Script executed:
#!/bin/bash
# Search for other AddSender usages to verify the expected ownership pattern
rg -nP 'AddSender\(' --type rust -A 2 -B 2Length of output: 4000
Remove the .clone() at line 29 for consistency with standard AddSender pattern.
Across the codebase, 7 of 9 AddSender usages pass senders directly without cloning, establishing a clear ownership transfer pattern. Line 29's tx.clone() breaks this convention. The cloned sender is never used to send frames; keeping the original tx alive until line 69 merely prevents premature channel closure—a role the clone already fulfills.
Change line 29 to:
.ask(camera::AddSender(tx))This ensures consistency and eliminates the unnecessary local reference.
🤖 Prompt for AI Agents
In crates/recording/src/sources/camera.rs around line 29, the call currently
uses .ask(camera::AddSender(tx.clone())), which unnecessarily clones the sender
and breaks the established ownership-transfer pattern used elsewhere; replace it
with .ask(camera::AddSender(tx)) so the original tx is moved into AddSender
(remove the .clone()), aligning with the other 7/9 usages and eliminating the
redundant local reference.
Summary by CodeRabbit
Bug Fixes
Performance Improvements