Cursor position improvements #845
Conversation
|
Warning Rate limit exceeded@oscartbeaumont has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 33 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 (2)
WalkthroughThe cursor size calculation logic in Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Cursor CLI
participant OS as Operating System
participant CursorShape as CursorShape Enum
CLI->>OS: Request current cursor info
OS-->>CLI: Return cursor handle or hash
CLI->>CursorShape: Convert handle/hash to CursorShape (platform-specific)
CursorShape-->>CLI: Resolved cursor shape or Unknown
CLI->>CLI: Print resolved cursor shape or unknown message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
crates/recording/src/cursor.rs (1)
581-584: Missing cursor shape resolution for WindowsThe Windows implementation doesn't set the
shapefield inCursorData, which is inconsistent with the macOS implementation. The field will be uninitialized or need explicit initialization.Some(CursorData { image: png_data, hotspot: XY::new(hotspot_x, hotspot_y), + shape: None, // TODO: Implement Windows cursor shape resolution })
🧹 Nitpick comments (4)
crates/recording/src/cursor.rs (1)
268-274: Consider handling the shape resolution more explicitlyThe current implementation silently returns
NonewhenCursorShapeMacOS::from_hashfails. Consider logging when an unknown cursor hash is encountered for debugging purposes.- let shape = - cap_cursor_info::CursorShapeMacOS::from_hash(&hex::encode(Sha256::digest(&image))); - + let hash = hex::encode(Sha256::digest(&image)); + let shape = cap_cursor_info::CursorShapeMacOS::from_hash(&hash); + if shape.is_none() { + tracing::debug!("Unknown cursor hash encountered: {}", hash); + } +crates/cursor-info/src/macos.rs (2)
44-44: Fix documentation typo for IBeamVerticalForVerticalLayout.The documentation comment references "ibeamcursorforverticallayout" but should reference "iBeamCursorForVerticalLayout" to match Apple's actual documentation naming.
- /// https://developer.apple.com/documentation/appkit/nscursor/ibeamcursorforverticallayout + /// https://developer.apple.com/documentation/appkit/nscursor/iBeamCursorForVerticalLayout
122-161: Consider documenting the hash generation process.While the implementation is correct, the hardcoded SHA-256 hashes could become outdated if Apple updates their cursor assets. Consider adding a comment explaining how these hashes were generated and potentially create a utility script to regenerate them when needed.
Would you like me to help create a script that can extract and hash macOS cursor images to maintain these values?
crates/cursor-info/src/windows.rs (1)
139-151: Complete implementation for scroll cursor variants.Several scroll cursor variants are currently unimplemented and will return
None. This means these cursors won't be rendered properly.Would you like me to help implement the missing scroll cursor variants or open an issue to track this work?
📜 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 (12)
apps/desktop/src/utils/tauri.ts(1 hunks)crates/cursor-info/Cargo.toml(1 hunks)crates/cursor-info/examples/cli.rs(3 hunks)crates/cursor-info/src/lib.rs(2 hunks)crates/cursor-info/src/macos.rs(1 hunks)crates/cursor-info/src/windows.rs(1 hunks)crates/project/Cargo.toml(1 hunks)crates/project/src/meta.rs(1 hunks)crates/recording/Cargo.toml(1 hunks)crates/recording/src/cursor.rs(5 hunks)crates/recording/src/studio_recording.rs(1 hunks)crates/rendering/src/layers/cursor.rs(4 hunks)
✅ Files skipped from review due to trivial changes (3)
- crates/recording/Cargo.toml
- crates/project/Cargo.toml
- apps/desktop/src/utils/tauri.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/rendering/src/layers/cursor.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: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy
🔇 Additional comments (12)
crates/recording/src/studio_recording.rs (1)
528-528: LGTM!The field update from
hashtoshapecorrectly aligns with the new cursor shape abstraction.crates/project/src/meta.rs (1)
247-247: LGTM!The type change to
Option<cap_cursor_info::CursorShape>properly integrates the new cursor shape abstraction while maintaining backward compatibility with the optional field.crates/cursor-info/Cargo.toml (1)
23-24: Confirm objc2-app-kit version downgrade
- Verified on crates.io: both 0.3.1 and 0.3.0 exist and are not yanked.
- No CHANGELOG or release notes in the repo mention differences between 0.3.1 and 0.3.0.
Please double-check that none of the features or fixes you rely on in 0.3.1 have been removed or altered in 0.3.0.
crates/cursor-info/examples/cli.rs (2)
72-77: Clean implementation of cursor shape resolutionGood use of the new
CursorShapeMacOS::from_hashAPI with proper handling of unknown cursors.
95-111: Excellent simplification of Windows cursor monitoringThe refactored code is much cleaner and more maintainable than the previous manual bitmap extraction approach. Good use of the
TryFromtrait for cursor shape conversion.crates/cursor-info/src/macos.rs (2)
47-120: LGTM!The resolve method implementation correctly maps each cursor variant to its corresponding SVG asset and hotspot coordinates. The explicit
Nonereturn forDisappearingItemis appropriate.
164-168: LGTM!The
Fromtrait implementation correctly wraps the macOS cursor shape in the genericCursorShapeenum.crates/cursor-info/src/windows.rs (1)
214-218: LGTM!The
Fromtrait implementation correctly wraps the Windows cursor shape in the genericCursorShapeenum.crates/cursor-info/src/lib.rs (4)
3-11: LGTM!The module structure and imports are well-organized, properly separating platform-specific implementations.
22-37: LGTM!The
CursorShapeenum design with platform-specific variants and the delegation pattern in theresolvemethod are well-structured.
39-62: LGTM!The
DisplayandSerializeimplementations correctly format the cursor shape as a platform-prefixed string.
97-104: LGTM!The
Typetrait implementation correctly representsCursorShapeas a string type for TypeScript bindings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/cursor-info/src/windows.rs (1)
68-154: Cursor resolution implementation looks solid.The method properly handles supported cursors with embedded SVG assets and normalized hotspot coordinates. The incomplete scroll cursor implementations are handled gracefully by returning
None.Would you like me to help implement the remaining scroll cursor variants or track this as a TODO item?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/cursor-info/src/lib.rs(2 hunks)crates/cursor-info/src/windows.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/cursor-info/src/lib.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: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (3)
crates/cursor-info/src/windows.rs (3)
6-66: Good implementation of Windows cursor shapes enum.The enum is well-structured with comprehensive Windows cursor types and proper documentation. I notice the typo mentioned in previous reviews (
ScrolNS→ScrollNS) has been fixed.
156-266: Excellent implementation of cursor caching optimization.The cursor caching system directly addresses the previous review suggestion about avoiding repeated system calls. The implementation uses
OnceLockfor thread-safe lazy initialization and loads all cursors once into aHashMapfor efficient lookups.
268-272: Clean conversion implementation.Simple and correct implementation that enables ergonomic usage with the generic cursor shape system.
Summary by CodeRabbit