⚡ Optimizes macOS cursor handling logic. #1427
Conversation
Uses runtime NSCursor information to adapt for custom cursor sizes set in macOS System Preferences.
WalkthroughThis pull request introduces a macOS cursor shape caching mechanism that replaces direct hash-to-shape matching with a cached HashMap lookup approach. It adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cursor-info/examples/cli.rs (1)
37-43:from_hashmethod doesn't exist; useTryFrominstead.The
CursorShapeMacOS::from_hashmethod was replaced withTryFrom<&String>forCursorShape. This code won't compile.- // Try to resolve to CursorShape - if let Some(cursor_shape_macos) = CursorShapeMacOS::from_hash(&hash) { - let cursor_shape = CursorShape::MacOS(cursor_shape_macos); + if let Ok(cursor_shape) = CursorShape::try_from(&hash) { println!("CursorShape: {cursor_shape} | Hash: {hash}"); } else { println!("Unknown cursor | Hash: {hash}"); }Also remove the comment on line 37 per coding guidelines.
🧹 Nitpick comments (5)
crates/cursor-info/src/macos.rs (4)
229-239: Simplify the boolean return logic.The function can be simplified by directly returning the condition result.
#[cfg(target_os = "macos")] pub fn is_tahoe() -> bool { let info = os_info::get(); let version_str = info.version().to_string(); const MACOS_TAHOE_MAJOR_VERSION: &str = "26"; - if version_str.starts_with(MACOS_TAHOE_MAJOR_VERSION) { - return true; - } - false + version_str.starts_with(MACOS_TAHOE_MAJOR_VERSION) }
264-423: Consider reducing duplication between Tahoe and non-Tahoe cursor mappings.The two
vec!blocks share identical structure and cursor type order. A helper macro or array of(NSCursor method, non-Tahoe variant, Tahoe variant)tuples could reduce this ~160 lines to ~40 lines while maintaining clarity.
425-431: Use iteratorcollect()for more idiomatic HashMap construction.- let mut cursors_map = HashMap::new(); - - for (hash, cursor) in cursors { - cursors_map.insert(hash, cursor); - } - - cursors_map + cursors.into_iter().collect()
437-452: Remove doc comments per coding guidelines.The coding guidelines prohibit all comment forms including
///. Consider removing these doc comments or clarifying the guidelines if API documentation should be an exception.#[cfg(target_os = "macos")] mod macos_only { use super::*; - /// Derive the cursor type from a hash - /// macOS doesn't allow comparing `NSCursor` instances directly so we hash the image data. - /// macOS cursor are also resolution-independent so this works. impl TryFrom<&String> for super::CursorShape {crates/recording/src/cursor.rs (1)
214-220: RedundantInto::intoconversion.
shapeis alreadyOption<CursorShape>, so.map(Into::into)is a no-op. This appears to be leftover from previous code where an intermediate type conversion was needed.let shape = CursorShape::try_from(&hex::encode(Sha256::digest(&image))).ok(); Some(CursorData { image, hotspot: XY::new(hotspot.x / size.width, hotspot.y / size.height), - shape: shape.map(Into::into), + shape, })
📜 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 (5)
crates/cursor-info/Cargo.toml(1 hunks)crates/cursor-info/examples/cli.rs(2 hunks)crates/cursor-info/src/macos.rs(2 hunks)crates/recording/src/cursor.rs(1 hunks)crates/recording/src/studio_recording.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
crates/cursor-info/examples/cli.rscrates/recording/src/cursor.rscrates/cursor-info/src/macos.rscrates/recording/src/studio_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:
crates/cursor-info/examples/cli.rscrates/recording/src/cursor.rscrates/cursor-info/src/macos.rscrates/recording/src/studio_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:
crates/cursor-info/examples/cli.rscrates/recording/src/cursor.rscrates/cursor-info/src/macos.rscrates/recording/src/studio_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:
crates/recording/src/studio_recording.rs
🧬 Code graph analysis (3)
crates/cursor-info/examples/cli.rs (1)
crates/cursor-info/src/macos.rs (1)
get_cursor_cache(242-433)
crates/recording/src/cursor.rs (3)
crates/cursor-info/src/macos.rs (1)
try_from(446-451)crates/cursor-info/src/windows.rs (1)
try_from(258-263)crates/camera-avfoundation/src/lib.rs (1)
try_from(66-73)
crates/recording/src/studio_recording.rs (1)
crates/cursor-info/src/macos.rs (1)
get_cursor_cache(242-433)
🔇 Additional comments (1)
crates/recording/src/studio_recording.rs (1)
860-861: LGTM!The cache initialization is correctly placed before cursor recording begins, ensuring the static
OnceLockis populated on the main thread before any cursor processing occurs.
| [target.'cfg(target_os = "macos")'.dev-dependencies] | ||
| objc2 = "0.6" | ||
| objc2-app-kit = { version = "0.3.0", features = ["NSCursor", "NSApplication"] } | ||
| os_info = "3.13.0" |
There was a problem hiding this comment.
os_info should be a regular dependency, not a dev-dependency.
The os_info crate is used by is_tahoe() which is called from get_cursor_cache() in production code (e.g., studio_recording.rs). Dev-dependencies are only available for tests, examples, and benchmarks—not for library code. This will cause compilation failures on macOS builds.
Apply this diff to fix the dependency section:
-[target.'cfg(target_os = "macos")'.dev-dependencies]
-objc2 = "0.6"
-objc2-app-kit = { version = "0.3.0", features = ["NSCursor", "NSApplication"] }
-os_info = "3.13.0"
+[target.'cfg(target_os = "macos")'.dependencies]
+objc2 = "0.6"
+objc2-app-kit = { version = "0.3.0", features = ["NSCursor", "NSApplication"] }
+os_info = "3.13.0"Alternatively, if objc2 and objc2-app-kit should remain dev-dependencies (for the CLI example), move only os_info to a separate [target.'cfg(target_os = "macos")'.dependencies] section.
📝 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.
| [target.'cfg(target_os = "macos")'.dev-dependencies] | |
| objc2 = "0.6" | |
| objc2-app-kit = { version = "0.3.0", features = ["NSCursor", "NSApplication"] } | |
| os_info = "3.13.0" | |
| [target.'cfg(target_os = "macos")'.dependencies] | |
| objc2 = "0.6" | |
| objc2-app-kit = { version = "0.3.0", features = ["NSCursor", "NSApplication"] } | |
| os_info = "3.13.0" |
🤖 Prompt for AI Agents
In crates/cursor-info/Cargo.toml around lines 23 to 26, os_info is mistakenly
listed under target macOS dev-dependencies but is used by production code; move
os_info out of dev-dependencies into the target macOS regular dependencies
section (i.e., create or use [target.'cfg(target_os = "macos")'.dependencies]
and add os_info = "3.13.0") so it is available to library code at build time; if
objc2 and objc2-app-kit must remain dev-dependencies for examples/CLI, keep them
under dev-dependencies and only relocate os_info to the regular dependencies
block.
|
|
||
| use cap_cursor_info::{CursorShape, CursorShapeMacOS}; | ||
| use sha2::{Digest, Sha256}; | ||
| use cap_cursor_info::CursorShape; |
There was a problem hiding this comment.
Missing import: CursorShapeMacOS is used but not imported.
Line 21 and 38 reference CursorShapeMacOS, but only CursorShape is imported. This will cause a compilation error.
-use cap_cursor_info::CursorShape;
+use cap_cursor_info::{CursorShape, CursorShapeMacOS};📝 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.
| use cap_cursor_info::CursorShape; | |
| use cap_cursor_info::{CursorShape, CursorShapeMacOS}; |
🤖 Prompt for AI Agents
In crates/cursor-info/examples/cli.rs around lines 1 to 38, the code uses
CursorShapeMacOS at lines 21 and 38 but only imports CursorShape, causing a
compile error; update the use statement to also import CursorShapeMacOS (for
example add CursorShapeMacOS to the cap_cursor_info import) so both types are in
scope, and re-run cargo check to verify the fix.
| #[inline] | ||
| fn load_cursor(cursor: Retained<NSCursor>) -> String { | ||
| // runtime get a give cursor to hash String | ||
| unsafe { | ||
| hex::encode(Sha256::digest( | ||
| cursor | ||
| .image() | ||
| .TIFFRepresentation() | ||
| .expect("Failed to get TIFF representation of build-in cursor") | ||
| .as_bytes_unchecked(), | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove inline comments per coding guidelines.
The inline comment at line 252 should be removed. Also, there's a typo: "build-in" should be "built-in" in the expect message. As per coding guidelines, code must be self-explanatory through naming, types, and structure.
#[inline]
fn load_cursor(cursor: Retained<NSCursor>) -> String {
- // runtime get a give cursor to hash String
unsafe {
hex::encode(Sha256::digest(
cursor
.image()
.TIFFRepresentation()
- .expect("Failed to get TIFF representation of build-in cursor")
+ .expect("Failed to get TIFF representation of built-in cursor")
.as_bytes_unchecked(),
))
}
}🤖 Prompt for AI Agents
In crates/cursor-info/src/macos.rs around lines 250 to 262, remove the inline
comment on line 252 and correct the typo in the expect message from "build-in"
to "built-in"; update the code so there are no inline comments and the expect
call reads something like "Failed to get TIFF representation of built-in cursor"
to satisfy coding guidelines and improve clarity.
| let cursors: Vec<(String, CursorShapeMacOS)> = if CursorShapeMacOS::is_tahoe() { | ||
| // tahoe cursor | ||
| vec
Uses runtime NSCursor information to adapt for custom cursor sizes set in macOS System Preferences.
Summary by CodeRabbit
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.