run clippy #1301
run clippy #1301
Conversation
|
Warning Rate limit exceeded@Brendonovich has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughSystematic refactor: consistent string/interpolation style, import narrowing, small API removals/field renames, Option-pattern modernizations, minor control-flow and path-type adjustments, example removal, and several test updates across multiple crates and desktop app code. Changes
Sequence Diagram(s)(omitted — changes are refactors and do not introduce a new or modified high-level control-flow needing a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
Brendonovich
force-pushed
the
run-clippy
branch
from
d903265 to
8399fa5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
crates/enc-ffmpeg/src/audio/buffered_resampler.rs (1)
78-93: Avoid extra flush when delay is zeroUsing delay().is_some() will still iterate when delay == Some(0), causing a redundant flush before breaking on samples == 0. Prefer checking for a positive delay.
- while self.resampler.delay().is_some() { + while matches!(self.resampler.delay(), Some(d) if d > 0) {apps/desktop/src-tauri/src/logging.rs (1)
41-63: Tail large logs without blocking and tolerate non‑UTF8Current path reads the whole file into a String twice and assumes UTF‑8. For ~1MB this is OK, but we can improve UX and reliability:
- Use std::fs::File + seek to last MAX_SIZE to avoid loading entire file.
- Read bytes and convert with from_utf8_lossy to avoid failures on invalid UTF‑8.
- Consider spawn_blocking for the heavy I/O in this async fn.
- let content = - fs::read_to_string(&log_file).map_err(|e| format!("Failed to read log file: {e}"))?; + use std::io::{Read, Seek, SeekFrom}; + let mut f = fs::File::open(&log_file) + .map_err(|e| format!("Failed to read log file: {e}"))?; + let to_read = MAX_SIZE.saturating_sub(1); + let pos = file_size.saturating_sub(to_read); + f.seek(SeekFrom::Start(pos)).ok(); + let mut buf = Vec::with_capacity(to_read as usize); + f.read_to_end(&mut buf) + .map_err(|e| format!("Failed to read log file: {e}"))?; + let content = String::from_utf8_lossy(&buf).into_owned(); ... - format!("{header}{truncated}") + format!("{header}{truncated}") ... - fs::read_to_string(&log_file).map_err(|e| format!("Failed to read log file: {e}"))? + String::from_utf8_lossy(&fs::read(&log_file) + .map_err(|e| format!("Failed to read log file: {e}"))?).into_owned()crates/scap-ffmpeg/examples/cli.rs (3)
50-60: Avoid panics in example pathThese unwraps can panic on misconfigured systems. Consider propagating or logging errors to keep the example robust.
- let config = StreamCfgBuilder::default() + let size = display.physical_size().ok_or("failed to get display size")?; + let config = StreamCfgBuilder::default() .with_fps(60.0) - .with_width(display.physical_size().unwrap().width() as usize) - .with_height(display.physical_size().unwrap().height() as usize) + .with_width(size.width() as usize) + .with_height(size.height() as usize) .build(); - let capturer = Capturer::builder( - display - .raw_handle() - .as_content_filter(sc::ShareableContent::current().await.unwrap()) - .await - .unwrap(), + let shareable = sc::ShareableContent::current().await + .map_err(|_| "failed to get shareable content")?; + let capturer = Capturer::builder( + display.raw_handle().as_content_filter(shareable).await + .map_err(|_| "failed to build content filter")?,
63-74: No-op method calls in output callbackCalling width()/height()/format() and discarding results has no effect. Either log these or remove to keep the example minimal.
- .with_output_sample_buf_cb(|frame| { + .with_output_sample_buf_cb(|frame| { use scap_ffmpeg::AsFFmpeg; let Frame::Screen(video_frame) = frame else { return; }; let ff = video_frame.as_ffmpeg().unwrap(); - ff_frame.width(); - ff_frame.height(); - ff_frame.format(); + println!("frame: {}x{} {:?}", ff.width(), ff.height(), ff.format()); })
75-77: Use underscore args instead of producing an unused tupleThis reads cleaner and avoids constructing a value just to silence warnings.
- .with_stop_with_err_cb(|stream, error| { - (stream, error); - }) + .with_stop_with_err_cb(|_stream, _error| {})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/desktop/src-tauri/src/lib.rs(4 hunks)apps/desktop/src-tauri/src/logging.rs(3 hunks)apps/desktop/src-tauri/src/main.rs(1 hunks)apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs(1 hunks)apps/desktop/src-tauri/src/recording.rs(3 hunks)apps/desktop/src-tauri/src/target_select_overlay.rs(3 hunks)apps/desktop/src-tauri/src/thumbnails/mac.rs(2 hunks)apps/desktop/src-tauri/src/upload.rs(6 hunks)crates/enc-ffmpeg/src/audio/buffered_resampler.rs(3 hunks)crates/enc-ffmpeg/src/video/h264.rs(1 hunks)crates/project/src/configuration.rs(1 hunks)crates/recording/examples/camera.rs(1 hunks)crates/recording/examples/screen_capture.rs(0 hunks)crates/recording/src/sources/audio_mixer.rs(1 hunks)crates/rendering-skia/src/layers/background.rs(2 hunks)crates/rendering/src/decoder/avassetreader.rs(2 hunks)crates/rendering/src/decoder/frame_converter.rs(2 hunks)crates/rendering/src/layers/cursor.rs(1 hunks)crates/rendering/src/lib.rs(2 hunks)crates/scap-ffmpeg/examples/cli.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/recording/examples/screen_capture.rs
🧰 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/rendering/src/layers/cursor.rsapps/desktop/src-tauri/src/thumbnails/mac.rscrates/recording/examples/camera.rscrates/rendering-skia/src/layers/background.rscrates/rendering/src/decoder/avassetreader.rsapps/desktop/src-tauri/src/logging.rscrates/rendering/src/decoder/frame_converter.rscrates/project/src/configuration.rsapps/desktop/src-tauri/src/target_select_overlay.rscrates/scap-ffmpeg/examples/cli.rsapps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rscrates/rendering/src/lib.rscrates/enc-ffmpeg/src/audio/buffered_resampler.rscrates/enc-ffmpeg/src/video/h264.rsapps/desktop/src-tauri/src/recording.rscrates/recording/src/sources/audio_mixer.rsapps/desktop/src-tauri/src/main.rsapps/desktop/src-tauri/src/upload.rsapps/desktop/src-tauri/src/lib.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/rendering/src/layers/cursor.rscrates/rendering-skia/src/layers/background.rscrates/rendering/src/decoder/avassetreader.rscrates/rendering/src/decoder/frame_converter.rscrates/project/src/configuration.rscrates/rendering/src/lib.rscrates/enc-ffmpeg/src/audio/buffered_resampler.rscrates/enc-ffmpeg/src/video/h264.rscrates/recording/src/sources/audio_mixer.rs
🧬 Code graph analysis (12)
crates/recording/examples/camera.rs (1)
apps/desktop/src/utils/tauri.ts (1)
DeviceOrModelID(388-388)
crates/rendering/src/decoder/avassetreader.rs (1)
crates/rendering/src/decoder/mod.rs (2)
width(24-26)height(28-30)
crates/rendering/src/decoder/frame_converter.rs (2)
crates/scap-direct3d/src/lib.rs (1)
stride(518-520)crates/rendering/src/decoder/mod.rs (2)
width(24-26)height(28-30)
crates/project/src/configuration.rs (1)
apps/desktop/src-tauri/src/recording.rs (1)
project_path(173-178)
apps/desktop/src-tauri/src/target_select_overlay.rs (3)
crates/scap-targets/src/platform/macos.rs (5)
get_containing_cursor(78-92)id(49-49)id(302-304)from_id(48-51)get_topmost_at_cursor(285-300)crates/scap-targets/src/platform/win.rs (5)
get_containing_cursor(152-165)id(131-131)id(424-426)from_id(130-133)get_topmost_at_cursor(319-362)crates/scap-targets/src/lib.rs (6)
get_containing_cursor(36-38)id(28-30)id(118-120)from_id(32-34)from_id(122-124)get_topmost_at_cursor(114-116)
crates/scap-ffmpeg/examples/cli.rs (3)
crates/scap-targets/src/platform/macos.rs (2)
display(519-539)primary(28-30)crates/scap-targets/src/lib.rs (2)
display(146-148)primary(20-22)crates/scap-screencapturekit/src/capture.rs (1)
builder(78-87)
crates/rendering/src/lib.rs (1)
crates/rendering/src/cursor_interpolation.rs (1)
interpolate_cursor(16-75)
crates/enc-ffmpeg/src/video/h264.rs (2)
crates/camera-windows/src/lib.rs (2)
formats(145-162)formats(150-152)crates/camera-mediafoundation/src/lib.rs (1)
formats(170-195)
apps/desktop/src-tauri/src/recording.rs (1)
apps/desktop/src/utils/tauri.ts (2)
RecordingMode(447-447)InstantRecordingMeta(416-416)
crates/recording/src/sources/audio_mixer.rs (2)
crates/timestamp/src/lib.rs (1)
duration_since(24-33)crates/media-info/src/lib.rs (1)
rate(125-127)
apps/desktop/src-tauri/src/upload.rs (1)
apps/desktop/src-tauri/src/api.rs (2)
upload_multipart_initiate(12-47)upload_multipart_complete(119-170)
apps/desktop/src-tauri/src/lib.rs (2)
apps/desktop/src/utils/tauri.ts (1)
GeneralSettingsStore(399-399)apps/desktop/src-tauri/src/general_settings.rs (1)
update(222-231)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (37)
crates/project/src/configuration.rs (1)
648-651: LGTM!The change from borrowing to an owned value for the destination path is correct. Both forms work due to
AsRef<Path>, but the owned version is more idiomatic per clippy's recommendations.apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (1)
10-10: LGTM!Correctly removes unused imports (
debug,info) while keepingtrace. The usage oftracing::warn!with a full path qualifier at line 213 is appropriate.apps/desktop/src-tauri/src/main.rs (1)
69-71: LGTM!The change from
format!("{}", e)to direct string interpolation is more idiomatic and aligns with Rust 2021 edition best practices.crates/rendering/src/layers/cursor.rs (1)
620-685: LGTM!Well-structured unit tests covering cursor idle opacity behavior. The use of approximate equality checks at line 676 appropriately handles floating-point comparisons.
crates/recording/examples/camera.rs (1)
3-3: LGTM!The import refactor removes the redundant
CameraFeedimport and uses a more specific path forDeviceOrModelID.crates/rendering-skia/src/layers/background.rs (2)
357-363: LGTM!Explicitly setting
border: Nonemakes the test initialization clearer and more maintainable.
381-387: LGTM!Consistent with the previous test case, explicitly setting
border: Noneimproves test clarity.apps/desktop/src-tauri/src/thumbnails/mac.rs (2)
25-25: LGTM!Removes the unused
std::sliceimport sincestd::slice::from_raw_partsis used with its full path at line 125.
190-195: LGTM!Replacing manual ceiling division
(height + 1) / 2withheight.div_ceil(2)is more idiomatic and makes the intent explicit. Both are mathematically equivalent.apps/desktop/src-tauri/src/target_select_overlay.rs (3)
68-75: LGTM!Removing unnecessary closure wrappers and passing function pointers directly is more efficient and idiomatic. The function signatures correctly match the expected closure types.
129-132: LGTM!Using direct string interpolation is more concise and aligns with modern Rust conventions.
143-146: LGTM!Consistent with the previous change, using direct string interpolation improves readability.
crates/enc-ffmpeg/src/audio/buffered_resampler.rs (1)
51-51: LGTM — clearer trailing expressionReturning remaining_samples as a trailing expression improves readability and satisfies clippy.
crates/enc-ffmpeg/src/video/h264.rs (2)
98-106:is_some_andsimplifies the formats check — OKSemantics match previous logic and read clearer. No issues.
265-269: Minor cleanup in finish() — OKDropping the explicit return is fine; function already returns
().apps/desktop/src-tauri/src/logging.rs (2)
39-47: LGTM — constants and formatting tightenedValue unchanged; formatting consistent with repo style.
75-76: Consistent error interpolation — OK
{e}interpolation matches the PR’s formatting goals.apps/desktop/src-tauri/src/lib.rs (4)
898-899: Error message now includes path — OKUsing
{path:?}gives better context. No behavior change.
1195-1196: Return stringified error — OK
Err(e.to_string())keeps types simple; consistent with nearby code.
2254-2256: Settings store update call — OKClosure mutates only
server_urlas intended; aligns withupdate(&mut Self).
2583-2584: Directory read error mapping — OKConsistent error propagation; no blocking issues added beyond existing sync I/O.
apps/desktop/src-tauri/src/upload.rs (5)
74-74: LGTM: Correct parameter passing.The change from
&apptoappis correct sinceappis already&AppHandleat the call site, so passing it directly provides the reference the function expects.
228-228: LGTM: Improved string interpolation.The inline variable interpolation
{fps}is more idiomatic than the previous format string.
248-252: LGTM: Simplified error handling.The changes improve readability:
- Direct parsing without dereferencing (
bodyinstead of&*body)- Streamlined conditional flow with
&&chaining- Direct error return without nested blocks
496-496: LGTM: Using newerio::Error::otherAPI.This is the idiomatic way to construct "other" category errors, replacing the more verbose
io::Error::new(io::ErrorKind::Other, ...).
682-682: LGTM: More specific error context.Including
{expected_part_number:?}provides clearer debugging information than a generic part reference.apps/desktop/src-tauri/src/recording.rs (4)
12-12: LGTM: Narrowed import scope.Importing only the specific type needed (
RecordingMode) improves clarity and reduces the API surface.
472-472: LGTM: Simplified string construction.Using
.to_string()on a static string is clearer thanformat!()when no interpolation is needed.
831-831: LGTM: Field initialization shorthand.Using
{ error }instead of{ error: error }is the idiomatic Rust field shorthand.
982-984: LGTM: Improved cleanup resilience.The change to log the error instead of returning early ensures that upload completion proceeds even if local file cleanup fails. This is appropriate behavior—the upload succeeded, and file cleanup is a "nice to have" rather than critical.
crates/rendering/src/decoder/avassetreader.rs (2)
82-84: LGTM: Usingis_none_orfor clearer Option handling.The
is_none_ormethod (stabilized in Rust 1.82) is more expressive than themap_or(false, ...)pattern for checking if an Option is None or matches a condition.
101-102: LGTM: Removed unnecessary type casts.The
width()andheight()methods already return the correct types (usize), so the casts are unnecessary.crates/rendering/src/decoder/frame_converter.rs (3)
27-29: LGTM: Removed unnecessary cast.The
stride()method returnsusizedirectly, matching the function parameter type.
51-51: LGTM: Consistent stride handling.Matches the improvement at line 27—no cast needed.
58-60: LGTM: Modern Option handling withis_none_or.This is clearer than the previous
map_orpattern for checking if the scaler needs recreation.crates/rendering/src/lib.rs (2)
1286-1290: LGTM: Clearer buffer size check withis_none_or.The
is_none_ormethod provides a more straightforward way to check if a new buffer is needed.
1441-1518: LGTM: Test module repositioned.The test module has been moved to the end of the file with no changes to its content. This is purely organizational.
| assert_eq!(item.timestamp.duration_since(start), ONE_SECOND / 2); | ||
| assert_eq!(item.inner.samples(), SOURCE_INFO.rate() as usize / 2); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Good move to named fields; update remaining tuple-style usages to avoid breakage
These assertions now target item.timestamp/item.inner, consistent with AudioFrame { inner, timestamp }. Other tests in this file still use .0/.1 on AudioFrame and will fail after this refactor. Please update them for consistency.
Run to find remaining tuple-style usages of AudioFrame:
Example fixes in this file:
- assert_eq!(
- source.buffer[1].1.duration_since(mixer.timestamps),
- ONE_SECOND / 2
- );
- assert_eq!(
- source.buffer[1].0.samples(),
- SOURCE_INFO.rate() as usize / 2
- );
+ assert_eq!(
+ source.buffer[1].timestamp.duration_since(mixer.timestamps),
+ ONE_SECOND / 2
+ );
+ assert_eq!(
+ source.buffer[1].inner.samples(),
+ SOURCE_INFO.rate() as usize / 2
+ );Also applies to: 716-718
🏁 Script executed:
#!/bin/bash
# Find tuple-field access on AudioFrame in this crate
rg -nP '(buffer\[[^\]]+\])\.(0|1)\b|AudioFrame\([^)]' -g 'crates/recording/src/**' -C2Length of output: 1302
🏁 Script executed:
#!/bin/bash
# Check the actual content at lines 712-718 and the AudioFrame struct definition
cat crates/recording/src/sources/audio_mixer.rs | sed -n '712,718p'
echo "---"
# Find the AudioFrame struct definition
rg -nP 'struct\s+AudioFrame|pub\s+struct\s+AudioFrame' crates/recording/src/sources/audio_mixer.rs -A5Length of output: 366
🏁 Script executed:
#!/bin/bash
# Search for AudioFrame struct definition in the crate (may be in different file)
rg -nP 'struct\s+AudioFrame\s*\{' crates/recording/src/ -A10
echo "---"
# Also check if it's defined in a separate types or frames module
fd -e rs . crates/recording/src/ | xargs grep -l 'struct AudioFrame'Length of output: 822
Fix remaining tuple-style usages at lines 637, 641, 670, 672 to complete the refactor
The changes at lines 712-718 are correct. However, four other locations in the same file still use tuple-style access on AudioFrame and will fail to compile:
- Line 637:
source.buffer[1].1→source.buffer[1].timestamp - Line 641:
source.buffer[1].0→source.buffer[1].inner - Line 670:
source.buffer[0].1→source.buffer[0].timestamp - Line 672:
source.buffer[0].0→source.buffer[0].inner
🤖 Prompt for AI Agents
In crates/recording/src/sources/audio_mixer.rs around lines 637, 641, 670, and
672, remaining tuple-style accesses on AudioFrame must be replaced with struct
field names following the refactor: change source.buffer[1].1 to
source.buffer[1].timestamp, source.buffer[1].0 to source.buffer[1].inner,
source.buffer[0].1 to source.buffer[0].timestamp, and source.buffer[0].0 to
source.buffer[0].inner; update those four sites accordingly so compilation
succeeds.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/recording/src/sources/microphone.rs (1)
31-36: Fix misleading error text ("camera" vs "microphone").The error reads “Failed to add camera sender” in the microphone source.
- .map_err(|e| anyhow!("Failed to add camera sender: {e}"))?; + .map_err(|e| anyhow!("Failed to add microphone sender: {e}"))?;crates/recording/src/studio_recording.rs (1)
750-759: Context string copy-paste: “microphone pipeline setup” for system audio.The second context should say “system audio pipeline setup”.
- .context("microphone pipeline setup")?; + .context("system audio pipeline setup")?;
🧹 Nitpick comments (7)
apps/desktop/src-tauri/src/window_exclusion.rs (1)
24-30: Good refactoring to let-chains.The refactoring from nested conditionals to a let-chain pattern is clean and addresses clippy warnings effectively.
Optionally, the comparison logic can be simplified further for improved readability:
if let Some(identifier) = self.bundle_identifier.as_deref() - && bundle_identifier - .map(|candidate| candidate == identifier) - .unwrap_or(false) + && bundle_identifier == Some(identifier) { return true; }This is more idiomatic since both sides are
Option<&str>types.apps/desktop/src-tauri/src/thumbnails/mac.rs (1)
231-231: Remove redundant.take(width).The
.take(width)is unnecessary sincey_rowis already sliced to exactlywidthlength at line 217.Apply this diff:
- for (x, y_val) in y_row.iter().enumerate().take(width) { + for (x, y_val) in y_row.iter().enumerate() {crates/recording/src/feeds/microphone.rs (1)
99-102: API rename default → default_device: consider a deprecation shim.This is a breaking rename for callers. If feasible, add a deprecated
default()wrapper to smooth upgrades and keep clippy happy.Apply:
impl MicrophoneFeed { + #[deprecated(note = "Use default_device()", since = "0.?.?")] + pub fn default() -> Option<(String, Device, SupportedStreamConfig)> { + Self::default_device() + }crates/recording/src/studio_recording.rs (3)
293-301: Watcher deduplication guard: OK, but note race window.Gating on
completion_tx.borrow().is_none()prevents duplicate sends in practice. There’s a tiny race if multiple errors hit simultaneously; acceptable given watch semantics. Add a short comment to capture intent.- if let Err(err) = res - && completion_tx.borrow().is_none() + // Avoid duplicate completion notifications; only send first error. + if let Err(err) = res + && completion_tx.borrow().is_none() {
673-686: Path ergonomics: good shift to &Path.Accepting
&Pathreduces clones. Consider propagating this change toensure_dirfor consistency.Apply:
-async fn create_segment_pipeline( - segments_dir: &Path, +async fn create_segment_pipeline( + segments_dir: &Path, cursors_dir: &Path,And update
ensure_dirbelow (see next comment).
796-800: Prefer &Path for ensure_dir and avoid cloning.Modernize to accept
&Pathand returnPathBufviato_path_buf().-fn ensure_dir(path: &PathBuf) -> Result<PathBuf, MediaError> { - std::fs::create_dir_all(path)?; - Ok(path.clone()) +fn ensure_dir(path: &Path) -> Result<PathBuf, MediaError> { + std::fs::create_dir_all(path)?; + Ok(path.to_path_buf()) }Call sites (e.g., lines 424-428, 711) will continue to work with coercions from
&PathBuf.crates/recording/src/output_pipeline/core.rs (1)
577-581: Minor ergonomics: consider returning &Path.
OutputPipeline::path(&self) -> &PathBufcould return&Pathto reduce PathBuf exposure.- pub fn path(&self) -> &PathBuf { - &self.path + pub fn path(&self) -> &Path { + &self.path }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
Cargo.toml(1 hunks)apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs(2 hunks)apps/desktop/src-tauri/src/recording.rs(4 hunks)apps/desktop/src-tauri/src/recording_settings.rs(1 hunks)apps/desktop/src-tauri/src/thumbnails/mac.rs(7 hunks)apps/desktop/src-tauri/src/upload.rs(9 hunks)apps/desktop/src-tauri/src/window_exclusion.rs(1 hunks)crates/audio/src/latency.rs(4 hunks)crates/audio/src/lib.rs(0 hunks)crates/audio/src/playback.rs(0 hunks)crates/editor/src/playback.rs(2 hunks)crates/enc-avfoundation/src/mp4.rs(3 hunks)crates/enc-ffmpeg/src/audio/buffered_resampler.rs(3 hunks)crates/media-info/src/lib.rs(1 hunks)crates/recording/src/audio_buffer.rs(0 hunks)crates/recording/src/feeds/microphone.rs(2 hunks)crates/recording/src/instant_recording.rs(4 hunks)crates/recording/src/lib.rs(0 hunks)crates/recording/src/output_pipeline/core.rs(8 hunks)crates/recording/src/sources/audio_mixer.rs(4 hunks)crates/recording/src/sources/microphone.rs(2 hunks)crates/recording/src/sources/screen_capture/macos.rs(1 hunks)crates/recording/src/sources/screen_capture/mod.rs(0 hunks)crates/recording/src/studio_recording.rs(2 hunks)
💤 Files with no reviewable changes (5)
- crates/recording/src/lib.rs
- crates/audio/src/playback.rs
- crates/audio/src/lib.rs
- crates/recording/src/audio_buffer.rs
- crates/recording/src/sources/screen_capture/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src-tauri/src/recording.rs
- apps/desktop/src-tauri/src/upload.rs
🧰 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:
apps/desktop/src-tauri/src/window_exclusion.rscrates/enc-ffmpeg/src/audio/buffered_resampler.rscrates/recording/src/feeds/microphone.rscrates/media-info/src/lib.rscrates/recording/src/sources/screen_capture/macos.rscrates/editor/src/playback.rsapps/desktop/src-tauri/src/thumbnails/mac.rsapps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rscrates/audio/src/latency.rscrates/recording/src/instant_recording.rscrates/recording/src/output_pipeline/core.rscrates/enc-avfoundation/src/mp4.rscrates/recording/src/studio_recording.rscrates/recording/src/sources/audio_mixer.rsapps/desktop/src-tauri/src/recording_settings.rscrates/recording/src/sources/microphone.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/enc-ffmpeg/src/audio/buffered_resampler.rscrates/recording/src/feeds/microphone.rscrates/media-info/src/lib.rscrates/recording/src/sources/screen_capture/macos.rscrates/editor/src/playback.rscrates/audio/src/latency.rscrates/recording/src/instant_recording.rscrates/recording/src/output_pipeline/core.rscrates/enc-avfoundation/src/mp4.rscrates/recording/src/studio_recording.rscrates/recording/src/sources/audio_mixer.rscrates/recording/src/sources/microphone.rs
🧠 Learnings (2)
📚 Learning: 2025-09-22T14:19:56.010Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T14:19:56.010Z
Learning: Applies to **/*.rs : Format Rust code using `rustfmt` and ensure all Rust code passes workspace-level clippy lints.
Applied to files:
Cargo.toml
📚 Learning: 2025-10-17T05:58:22.586Z
Learnt from: Brendonovich
PR: CapSoftware/Cap#1219
File: crates/enc-avfoundation/src/mp4.rs:350-373
Timestamp: 2025-10-17T05:58:22.586Z
Learning: In crates/enc-avfoundation/src/mp4.rs, the `finish()` method intentionally skips video extension when `is_paused` is true. This is correct behavior because if recording is paused, the video should not be extended beyond the pause point—the pause is user-initiated, unlike the case where ScreenCaptureKit stops providing frames during static content.
Applied to files:
crates/enc-avfoundation/src/mp4.rs
🧬 Code graph analysis (4)
apps/desktop/src-tauri/src/window_exclusion.rs (1)
crates/scap-targets/src/platform/macos.rs (1)
bundle_identifier(350-378)
crates/recording/src/output_pipeline/core.rs (3)
crates/recording/src/output_pipeline/win.rs (2)
oneshot(67-67)setup(41-269)crates/recording/src/output_pipeline/ffmpeg.rs (2)
setup(36-66)setup(114-132)crates/recording/src/output_pipeline/macos.rs (1)
setup(27-50)
crates/recording/src/sources/audio_mixer.rs (3)
crates/timestamp/src/win.rs (2)
now(49-53)duration_since(30-47)crates/timestamp/src/lib.rs (2)
now(103-112)duration_since(24-33)crates/media-info/src/lib.rs (1)
rate(125-127)
crates/recording/src/sources/microphone.rs (2)
crates/recording/src/feeds/microphone.rs (1)
audio_info(243-245)crates/recording/src/output_pipeline/core.rs (2)
audio_info(682-684)audio_info(760-760)
⏰ 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: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (37)
crates/media-info/src/lib.rs (1)
76-76: LGTM! Idiomatic clamp usage.The refactor from
min().max()toclamp()is more idiomatic and maintains identical behavior, ensuring the channel count stays within [1, 8].crates/editor/src/playback.rs (2)
328-338: LGTM! Cleaner guard clause for buffer adjustment logging.The combined
if letwith&& clamped != desiredis more idiomatic and reduces log noise by only emitting when actual clamping occurs within a known range.
406-423: LGTM! Streamlined latency hint logging.The combined guard
if let Some(hint) && hint.latency_secs > 0.0followed by a match statement is clearer than nested conditionals. The transport-specific messaging (AirPlay vs wireless vs default) improves observability.crates/recording/src/sources/screen_capture/macos.rs (2)
46-52: Good dead code removal.Removing the unused
CreateActorandDidStopWithErrorenum variants simplifies the error type. The remaining variants (NoDisplayandAsContentFilter) cover all the error cases actually used in this module.
110-110: LGTM: Correct clippy optimization.Replacing
ok_or_elsewithok_oris appropriate here sinceSourceError::AsContentFilteris a simple enum variant with no construction cost. This eliminates unnecessary closure overhead.crates/enc-ffmpeg/src/audio/buffered_resampler.rs (4)
51-51: LGTM! Idiomatic trailing expression.Removing the explicit
returnkeyword in favor of a trailing expression is the idiomatic Rust style and resolves the clippy lint.
59-63: LGTM! Modern let-chains pattern.The chained let-pattern is more concise and equally readable compared to nested if-let statements. This is a good use of modern Rust syntax.
78-78: LGTM! More direct condition check.Using
.is_some()is clearer thanwhile let Some(_)when the matched value isn't used, and resolves the clippy lint.
92-92: LGTM! Compound assignment operator.Using
+=is more concise and idiomatic than the expanded form, and resolves the clippy lint.apps/desktop/src-tauri/src/thumbnails/mac.rs (6)
25-25: LGTM: Import narrowing improves clarity.The import is now more specific and makes the dependency explicit.
190-190: LGTM: Usingdiv_ceilis more idiomatic.The
div_ceilmethod is clearer and safer than manual ceiling division with(height + 1) / 2.Also applies to: 195-195
244-244: LGTM: Consistent with iterator change.The dereference is correct and avoids indexing, which is safer.
263-263: LGTM: Consistent with enum variant rename.The reference is correctly updated to match the renamed variant.
274-274: LGTM: Usingclampis more idiomatic.The
clampmethod is clearer and more concise than chainingmaxandmin.
163-166: The_Fullvariant is actively used and should not be removed.The grep search confirms the variant is used in a pattern match at line 263 (
Nv12Range::_Full => (y, 1.0_f32),). The underscore prefix is a valid Rust convention for variants or bindings whose values aren't referenced in the match arm body—not an indicator of dead code. Removing it would break the match expression.Likely an incorrect or invalid review comment.
apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (2)
2-4: LGTM: Clean import consolidation and narrowing.The import changes consolidate
std::syncimports and narrow thetracingimport to onlytrace, which are both standard clippy-recommended improvements that enhance code clarity.
112-114: LGTM: Simplified struct initialization.The constructor simplification to
Self { content }is cleaner and more idiomatic, consistent with the removal of unused cache fields mentioned in the summary.crates/audio/src/latency.rs (4)
260-261: Excellent use ofclamp()for bounds checking.Replacing
max().min()withclamp()is more idiomatic and clearly expresses the intent to constrain a value within bounds.
294-295: Consistent bounds checking withclamp().Good application of the same pattern as in
with_bias().
370-377: Nice refactor using let-chains to reduce nesting.The combined conditional
if let Some(prev_raw) = self.last_raw_latency_secs && self.update_count < ... && clamped > ...effectively flattens the warmup spike detection logic while preserving the same behavior.
587-592: Cleaner conditional flow in stream latency calculation.Combining the output stream check with the latency retrieval in a single conditional reduces nesting and improves readability.
crates/enc-avfoundation/src/mp4.rs (4)
235-240: Improved pause handling with let-chains.The combined conditional elegantly handles the case where both
pause_timestampexists and the gap can be computed, reducing nesting while maintaining the same logic.
246-265: Well-structured monotonic PTS correction.Consolidating the PTS correction logic into a single conditional block with let-chains makes the monotonic guarantee more explicit and easier to follow. The sequence of compute → log → adjust is now clearly contained within the correction path.
295-300: Consistent pause handling for audio frames.Good application of the same let-chain pattern used for video frames, maintaining consistency across the codebase.
344-365: Consistent monotonic PTS correction for audio.The audio PTS correction mirrors the video implementation structure, ensuring both media types handle timing consistency in the same way. The unified conditional block clearly expresses the correction logic.
crates/recording/src/sources/audio_mixer.rs (2)
161-161: LGTM: Underscore prefix follows Rust conventions.The rename to
_resamplersaligns with clippy's unused field lint and signals that the field is retained for its Drop implementation.
345-385: LGTM: Let-chains improve readability.The refactored control flow using let-chains is more idiomatic and easier to follow than nested if-let statements.
crates/recording/src/feeds/microphone.rs (1)
154-163: Nice: removed unnecessary clone in config selection.
with_sample_rateon&SupportedStreamConfigRangealready returns an ownedSupportedStreamConfig; avoidingclone()is correct.crates/recording/src/sources/microphone.rs (4)
10-13: Struct encapsulation improvement LGTM.Moving from tuple to named fields clarifies intent;
_lockkeeps the guard alive without exposing it.
18-23: Manual async fn allow: acceptable here.Given the trait requires returning
impl Future, the lint suppression is appropriate.Ensure other sources follow the same pattern for consistency across the crate.
47-50: Constructor update LGTM.Named struct construction is clear and matches the new layout.
54-56: audio_info accessor remains correct.Returning the stored
AudioInfois unchanged semantically.crates/recording/src/output_pipeline/core.rs (5)
301-305: Error mapping cleanup LGTM.
map_err(anyhow::Error::from)simplifies type conversion before wrapping inPipelineDoneError.
315-320: finish_build signature simplification LGTM.Bounding to
impl AudioMuxeris sufficient (inheritsMuxer), and switching to&Pathis ergonomic.
469-471: Early return for no audio sources: good guard.Prevents spinning up the mixer when unused. Nice.
497-500: Awaiting mixer readiness directly: OK.Direct
awaitwith proper error mapping keeps the flow simple.
383-401: All Muxer implementations correctly satisfy the trait signature.All four
Muxerimplementers (WindowsMuxer, AVFoundationMp4Muxer, Mp4Muxer, OggMuxer) have been verified to adopt the correctsetupmethod signature. Each implementation includes all required parameters (config,output_path,video_config,audio_config,pause_flag,tasks) and usesasync fn, which is the syntactic sugar equivalent offn(...) -> impl Future<Output = Result<Self>> + Sendand fully satisfies the trait requirement.
| // i don't trust anyone to not overwrite the whole store lols | ||
| pub fn update(app: &AppHandle, update: impl FnOnce(&mut Self)) -> Result<(), String> { | ||
| let Ok(store) = app.store("store") else { | ||
| return Err("Store not found".to_string()); | ||
| }; | ||
|
|
||
| let mut settings = Self::get(app)?.unwrap_or_default(); | ||
| update(&mut settings); | ||
| store.set(Self::KEY, json!(settings)); | ||
| store.save().map_err(|e| e.to_string()) | ||
| } | ||
|
|
||
| fn save(&self, app: &AppHandle) -> Result<(), String> { | ||
| let Ok(store) = app.store("store") else { | ||
| return Err("Store not found".to_string()); | ||
| }; | ||
|
|
||
| store.set(Self::KEY, json!(self)); | ||
| store.save().map_err(|e| e.to_string()) | ||
| } | ||
| // pub fn update(app: &AppHandle, update: impl FnOnce(&mut Self)) -> Result<(), String> { | ||
| // let Ok(store) = app.store("store") else { | ||
| // return Err("Store not found".to_string()); | ||
| // }; | ||
|
|
||
| // let mut settings = Self::get(app)?.unwrap_or_default(); | ||
| // update(&mut settings); | ||
| // store.set(Self::KEY, json!(settings)); | ||
| // store.save().map_err(|e| e.to_string()) | ||
| // } | ||
|
|
||
| // fn save(&self, app: &AppHandle) -> Result<(), String> { | ||
| // let Ok(store) = app.store("store") else { | ||
| // return Err("Store not found".to_string()); | ||
| // }; | ||
|
|
||
| // store.set(Self::KEY, json!(self)); | ||
| // store.save().map_err(|e| e.to_string()) | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Remove commented-out code entirely.
Commented-out code should be deleted rather than left in the codebase. Version control preserves the history if these methods need to be recovered later. The informal comment on line 41 doesn't clearly explain why these methods were removed or whether this is temporary.
Apply this diff to remove the commented-out code:
- // i don't trust anyone to not overwrite the whole store lols
- // pub fn update(app: &AppHandle, update: impl FnOnce(&mut Self)) -> Result<(), String> {
- // let Ok(store) = app.store("store") else {
- // return Err("Store not found".to_string());
- // };
-
- // let mut settings = Self::get(app)?.unwrap_or_default();
- // update(&mut settings);
- // store.set(Self::KEY, json!(settings));
- // store.save().map_err(|e| e.to_string())
- // }
-
- // fn save(&self, app: &AppHandle) -> Result<(), String> {
- // let Ok(store) = app.store("store") else {
- // return Err("Store not found".to_string());
- // };
-
- // store.set(Self::KEY, json!(self));
- // store.save().map_err(|e| e.to_string())
- // }Since update was a public method, verify that it's not called elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Search for calls to RecordingSettingsStore::update
# Search for usage of the update method
rg -n 'RecordingSettingsStore::update' --type rust
# Also check for potential method calls on instances
rg -n '\.update\s*\(' apps/desktop/src-tauri/src/recording_settings.rs -A 2 -B 2 --type rustLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check the current state of the recording_settings.rs file around lines 41-60
cat apps/desktop/src-tauri/src/recording_settings.rs | head -70 | tail -35Length of output: 1313
🏁 Script executed:
#!/bin/bash
# Broader search for 'update' calls in the codebase (all types of files)
# Focus on RecordingSettings context
rg -n 'update' apps/desktop/src-tauri/src/recording_settings.rs -B 2 -A 2 --type rustLength of output: 507
🏁 Script executed:
#!/bin/bash
# Check if 'update' is called in JavaScript/TypeScript frontend files
# (Tauri apps often have JS frontend calling Rust backend)
rg -n "RecordingSettings.*update\|recording.*update" apps/desktop/src --type ts --type js -C 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for invocations to this struct/methods
rg -n 'recording_settings\|RecordingSettings' apps/desktop/src-tauri/src/ --type rust -C 1Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if the method might be used via module::method pattern
rg -n '\.update\(' apps/desktop/src-tauri/src/ --type rust --glob '!recording_settings.rs'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any trait implementations that might reference this
rg -n 'impl.*RecordingSettings' apps/desktop/src-tauri/src/ --type rustLength of output: 81
🏁 Script executed:
#!/bin/bash
# Verify if the file compiles by checking it directly for syntax issues
cat apps/desktop/src-tauri/src/recording_settings.rs | sed -n '35,65p'Length of output: 870
🏁 Script executed:
#!/bin/bash
# Try to find all methods defined in this file and check what's actually there
ast-grep --pattern $'impl RecordingSettingsStore {
$$$
}'Length of output: 3319
🏁 Script executed:
#!/bin/bash
# Check for pub fn update pattern more broadly
rg -n 'pub fn update' apps/desktop/src-tauri/src/recording_settings.rs --type rustLength of output: 96
🏁 Script executed:
#!/bin/bash
# Search for commented fn update pattern
rg -n '// pub fn update\|// fn update' apps/desktop/src-tauri/src/recording_settings.rs --type rustLength of output: 0
🏁 Script executed:
#!/bin/bash
# List all function definitions in recording_settings to understand the scope
rg -n 'fn ' apps/desktop/src-tauri/src/recording_settings.rs --type rustLength of output: 236
🏁 Script executed:
#!/bin/bash
# Check Tauri invoke commands if any reference this
rg -n 'invoke.*update\|update.*settings' apps/desktop/src --type ts --type js --type json -C 1Length of output: 0
🏁 Script executed:
#!/bin/bash
# Simple: just show the exact lines requested in the review
sed -n '41,60p' apps/desktop/src-tauri/src/recording_settings.rsLength of output: 792
Remove commented-out code entirely at lines 41-60.
Verification confirms the update method is not called anywhere in the codebase, making its removal safe. Commented-out code should be deleted rather than preserved; git history preserves it if needed later.
Apply this diff:
- // i don't trust anyone to not overwrite the whole store lols
- // pub fn update(app: &AppHandle, update: impl FnOnce(&mut Self)) -> Result<(), String> {
- // let Ok(store) = app.store("store") else {
- // return Err("Store not found".to_string());
- // };
-
- // let mut settings = Self::get(app)?.unwrap_or_default();
- // update(&mut settings);
- // store.set(Self::KEY, json!(settings));
- // store.save().map_err(|e| e.to_string())
- // }
-
- // fn save(&self, app: &AppHandle) -> Result<(), String> {
- // let Ok(store) = app.store("store") else {
- // return Err("Store not found".to_string());
- // };
-
- // store.set(Self::KEY, json!(self));
- // store.save().map_err(|e| e.to_string())
- // }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // i don't trust anyone to not overwrite the whole store lols | |
| pub fn update(app: &AppHandle, update: impl FnOnce(&mut Self)) -> Result<(), String> { | |
| let Ok(store) = app.store("store") else { | |
| return Err("Store not found".to_string()); | |
| }; | |
| let mut settings = Self::get(app)?.unwrap_or_default(); | |
| update(&mut settings); | |
| store.set(Self::KEY, json!(settings)); | |
| store.save().map_err(|e| e.to_string()) | |
| } | |
| fn save(&self, app: &AppHandle) -> Result<(), String> { | |
| let Ok(store) = app.store("store") else { | |
| return Err("Store not found".to_string()); | |
| }; | |
| store.set(Self::KEY, json!(self)); | |
| store.save().map_err(|e| e.to_string()) | |
| } | |
| // pub fn update(app: &AppHandle, update: impl FnOnce(&mut Self)) -> Result<(), String> { | |
| // let Ok(store) = app.store("store") else { | |
| // return Err("Store not found".to_string()); | |
| // }; | |
| // let mut settings = Self::get(app)?.unwrap_or_default(); | |
| // update(&mut settings); | |
| // store.set(Self::KEY, json!(settings)); | |
| // store.save().map_err(|e| e.to_string()) | |
| // } | |
| // fn save(&self, app: &AppHandle) -> Result<(), String> { | |
| // let Ok(store) = app.store("store") else { | |
| // return Err("Store not found".to_string()); | |
| // }; | |
| // store.set(Self::KEY, json!(self)); | |
| // store.save().map_err(|e| e.to_string()) | |
| // } |
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/recording_settings.rs around lines 41 to 60,
remove the entire commented-out block containing the unused update and save
methods (all commented lines shown) so the file no longer contains that
dead/commented code; commit the deletion (rely on git history to recover if
needed) and run a quick build or formatting check to ensure no unrelated
formatting issues were introduced.
| collapsible_if = "deny" | ||
| manual_is_multiple_of = "deny" | ||
| clone_on_copy = "deny" | ||
| redundant_closure = "deny" | ||
| ptr_arg = "deny" | ||
| len_zero = "deny" | ||
| let_unit_value = "deny" | ||
| unnecessary_lazy_evaluations = "deny" | ||
| needless_range_loop = "deny" | ||
| manual_clamp = "deny" |
There was a problem hiding this comment.
🧩 Analysis chain
Clippy lint configuration looks good; verify full workspace compliance.
The added lints are well-established best practices that promote code quality across readability, idiomaticity, optimization, and API design. However, since these are set to "deny", the entire workspace must pass these checks for builds to succeed.
Please verify that all workspace members pass these new lints by running:
🏁 Script executed:
#!/bin/bash
# Description: Verify entire workspace passes the newly added Clippy lints.
# Build and check the entire workspace with the new lints
cargo build --workspace 2>&1 | tee clippy-check.log
# Additionally, run clippy explicitly to catch all warnings
cargo clippy --workspace --all-targets 2>&1 | tee clippy-verbose.log
# Search for any deny-level clippy violations in the output
echo "=== Checking for deny-level violations ==="
grep -E "(error\[E|error:.*clippy)" clippy-check.log || echo "No errors found"Length of output: 192
🏁 Script executed:
#!/bin/bash
# Static analysis to detect potential Clippy lint violations without running cargo
# This searches for common patterns that would trigger the newly added deny lints
echo "=== Searching for Clone-on-Copy violations ==="
# Look for .clone() calls that might be on Copy types
rg '\.clone\(\)' --type=rs -C2 | head -30
echo ""
echo "=== Searching for len_zero violations (checking .len() == 0) ==="
# Look for .len() == 0 patterns
rg -nP '\.len\(\)\s*==\s*0' --type=rs | head -20
echo ""
echo "=== Searching for ptr_arg violations (checking &Vec or &String parameters) ==="
# Look for function parameters with &Vec or &String
rg -nP 'fn\s+\w+\([^)]*&(Vec|String)<' --type=rs | head -20
echo ""
echo "=== Checking for existing clippy config in source ==="
# Check if clippy attributes are in use
rg -nP '#\[allow|#\[deny|#\[warn' --type=rs | head -20
echo ""
echo "=== Searching for let _ patterns (let_unit_value) ==="
rg -nP 'let\s+_\s*=' --type=rs | head -20Length of output: 437
🏁 Script executed:
#!/bin/bash
# Static analysis using glob patterns to find Rust files
echo "=== Searching for Clone-on-Copy violations ==="
rg '\.clone\(\)' -g '*.rs' -C2 | head -40
echo ""
echo "=== Searching for len_zero violations ==="
rg '\.len\(\)\s*==\s*0' -g '*.rs' | head -20
echo ""
echo "=== Searching for ptr_arg violations ==="
rg -P 'fn\s+\w+\([^)]*&(Vec|String)<' -g '*.rs' | head -20
echo ""
echo "=== Summary: List all Rust files in workspace ==="
fd -e rs | head -20Length of output: 4095
🏁 Script executed:
#!/bin/bash
# Comprehensive search for len_zero violations
echo "=== All len_zero violations (comprehensive) ==="
rg '\.len\(\)\s*==\s*0|\.len\(\)\s*!=\s*[1-9]' -g '*.rs' -n
echo ""
echo "=== All let_unit_value violations ==="
rg 'let\s+_\s*=' -g '*.rs' -n | head -30
echo ""
echo "=== Check Cargo.toml for new lints ==="
grep -A 20 'lints.rust.unsafe_code' Cargo.toml | head -30Length of output: 3228
Workspace does not pass new deny lints—multiple violations must be fixed before merge.
The added lints are well-established best practices, but verification found violations that will cause build failures:
len_zero violations (2):
- crates/camera-mediafoundation/examples/cli.rs:24: Replace
.len() == 0with.is_empty() - crates/camera-ffmpeg/src/windows.rs:27: Replace
.len() == 0with.is_empty()
let_unit_value violations (29+):
- crates/scap-targets/src/platform/win.rs: 24 instances (Windows FFI cleanup calls)
- crates/video-decode/src/bin/print_frames.rs:20
- crates/scap-ffmpeg/src/screencapturekit.rs:119
- crates/scap-direct3d/src/lib.rs:389
The let_unit_value pattern is common for intentionally ignoring return values in FFI and cleanup code. Consider using let _ = ...; with #[allow(let_unit_value)] attributes on affected functions instead of removing them entirely.
🤖 Prompt for AI Agents
In Cargo.toml around lines 72–81 you added deny lints that cause build failures;
fix the reported violations by (1) replacing `.len() == 0` with `.is_empty()` in
crates/camera-mediafoundation/examples/cli.rs (line ~24) and
crates/camera-ffmpeg/src/windows.rs (line ~27), and (2) handle the many
let_unit_value violations (≈29+) in FFI/cleanup code by changing bare `let
expr;` return-ignores to `let _ = expr;` where appropriate and adding
`#[allow(let_unit_value)]` to the containing functions (or smallest enclosing
scope) for the Windows FFI cleanup functions in
crates/scap-targets/src/platform/win.rs (24 occurrences), and the other files
listed (crates/video-decode/src/bin/print_frames.rs,
crates/scap-ffmpeg/src/screencapturekit.rs, crates/scap-direct3d/src/lib.rs);
then re-run cargo clippy to confirm no remaining deny failures before merging.
| if width.is_multiple_of(2) { | ||
| width -= 1; | ||
| } |
There was a problem hiding this comment.
Critical: Inverted parity logic makes dimensions odd instead of even.
The conditions are checking if value.is_multiple_of(2) (i.e., "if even") and then decrementing, which converts even dimensions to odd. Video codecs require even dimensions, so this logic must be inverted.
Apply this diff to fix the inverted logic:
- if width.is_multiple_of(2) {
+ if !width.is_multiple_of(2) {
width -= 1;
}- if height.is_multiple_of(2) {
+ if !height.is_multiple_of(2) {
height -= 1;
}Apply the same negation (!) to all eight occurrences of is_multiple_of(2) checks in this function (lines 407, 413, 422, 428, 437, 443, 453, 459).
Also applies to: 413-415, 422-424, 428-430, 437-439, 443-445, 453-455, 459-461
🤖 Prompt for AI Agents
In crates/recording/src/instant_recording.rs around lines 407-461, the parity
checks currently use if value.is_multiple_of(2) and then decrement the
dimension, which turns even values odd; change each check to if
!value.is_multiple_of(2) so we only decrement when the value is odd. Apply this
negation to all eight occurrences at the specified lines (407, 413, 422, 428,
437, 443, 453, 459 and the corresponding +1 lines listed in the comment) so each
dimension is adjusted only when odd to ensure dimensions remain even for video
codecs.
| assert_eq!(item.timestamp.duration_since(start), ONE_SECOND / 2); | ||
| assert_eq!(item.inner.samples(), SOURCE_INFO.rate() as usize / 2); | ||
|
|
||
| let item = &source.buffer[1]; | ||
| assert_eq!(item.1.duration_since(start), ONE_SECOND); | ||
| assert_eq!(item.0.samples(), SOURCE_INFO.rate() as usize / 2); | ||
| assert_eq!(item.timestamp.duration_since(start), ONE_SECOND); | ||
| assert_eq!(item.inner.samples(), SOURCE_INFO.rate() as usize / 2); |
There was a problem hiding this comment.
Incomplete AudioFrame refactor: Lines 591, 595, 624, 626 still use tuple-style access.
While these test assertions correctly use named fields (.timestamp, .inner), the frame_gap test at lines 591, 595 and the start_gap test at lines 624, 626 still use the old tuple-style access (.1, .0). This will cause compilation errors.
Apply this diff to fix the frame_gap test:
assert_eq!(
- source.buffer[1].1.duration_since(mixer.timestamps),
+ source.buffer[1].timestamp.duration_since(mixer.timestamps),
ONE_SECOND / 2
);
assert_eq!(
- source.buffer[1].0.samples(),
+ source.buffer[1].inner.samples(),
SOURCE_INFO.rate() as usize / 2
);Apply this diff to fix the start_gap test:
- assert_eq!(source.buffer[0].1.duration_since(start), ONE_SECOND / 2);
+ assert_eq!(source.buffer[0].timestamp.duration_since(start), ONE_SECOND / 2);
assert_eq!(
- source.buffer[0].0.samples(),
+ source.buffer[0].inner.samples(),
SOURCE_INFO.rate() as usize / 2
);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/recording/src/sources/audio_mixer.rs around lines 591-595 and 624-626,
the tests still use tuple-style field access (.1 and .0) from the old AudioFrame
shape; update those occurrences to use the named fields used elsewhere: replace
.1 with .inner and .0 with .timestamp so the assertions read e.g.
item.inner.samples() and item.timestamp.duration_since(start).
Summary by CodeRabbit
Refactor
Tests
Chores