chore(cua-driver-rs): delete dead screenshot tool code surfaced by PR #1692 warnings - #1694
Merged
Merged
Conversation
…ol after PR #1692 removed their registration PR #1692 stopped registering the standalone `screenshot` tool on all three platforms but left the structs + DEF statics + `Tool` impls in place as a deletion-deferred follow-up. The result was 5 dead-code / unused-import warnings on the Windows daemon build: - `unused import: background_unavailable_error` (from ScrollTool's old import set — the helper's no longer used after the scroll- specific error was added in 9e30d2c) - `struct ScreenshotTool` never constructed - `static SCREENSHOT_DEF` never used - `struct ScreenshotCompatTool` never constructed - `static SCREENSHOT_COMPAT_DEF` never used Cleanup: - platform-windows/src/tools/impl_.rs: drop the dead struct + impl + static for both ScreenshotTool and ScreenshotCompatTool (~225 lines), replace with a short pointer-comment to PR #1692 and GetWindowStateTool as the canonical capture site. - platform-linux/src/tools/impl_.rs: same shape, ~170 lines gone. - platform-macos/src/tools: delete screenshot.rs and screenshot_compat.rs entirely + drop the `mod` lines in tools/mod.rs. - platform-windows/src/tools/impl_.rs ScrollTool import: drop `background_unavailable_error` from the use statement (kept `DispatchMode` and `EventKind` — both still used by the scroll-specific error path). The underlying capture functions (`crate::capture::screenshot_window_bytes*`, `screenshot_display_bytes`, `wgc::screenshot_window_via_wgc`, and the macOS/Linux equivalents) stay — GetWindowStateTool calls them directly. No behavior change. The 4 remaining platform-linux warnings (RenderState::tick / apply_command / etc.) are pre-existing and out of scope. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
📝 WalkthroughWalkthroughThis PR removes the standalone screenshot tool implementations across Linux, macOS, and Windows platforms, consolidating screenshot functionality under the ChangesScreenshot Tool Consolidation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs`:
- Around line 2831-2838: The file now states that ScreenshotTool and
ScreenshotCompatTool were removed and that get_window_state with
capture_mode:"vision" plus the capture backends
(crate::capture::screenshot_window_bytes_with_occlusion,
screenshot_display_bytes, crate::wgc::screenshot_window_via_wgc) and
GetWindowStateTool are the canonical paths; locate the older registry note block
that still mentions keeping ScreenshotTool/ScreenshotCompatTool in this file for
a follow-up and either remove or rewrite that block to match the new state
(i.e., say the two structs were removed and point readers to get_window_state
and GetWindowStateTool and the listed capture functions) so the file no longer
describes two different states.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55bf565a-074e-4057-b730-965d64e1bc01
📒 Files selected for processing (5)
libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rslibs/cua-driver/rust/crates/platform-macos/src/tools/mod.rslibs/cua-driver/rust/crates/platform-macos/src/tools/screenshot.rslibs/cua-driver/rust/crates/platform-macos/src/tools/screenshot_compat.rslibs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs
💤 Files with no reviewable changes (2)
- libs/cua-driver/rust/crates/platform-macos/src/tools/screenshot_compat.rs
- libs/cua-driver/rust/crates/platform-macos/src/tools/screenshot.rs
Comment on lines
+2831
to
+2838
| // `ScreenshotTool` and `ScreenshotCompatTool` were removed in PR #1692 — | ||
| // `get_window_state` with `capture_mode:"vision"` is the single canonical | ||
| // screenshot path. The underlying capture functions | ||
| // (`crate::capture::screenshot_window_bytes_with_occlusion`, | ||
| // `screenshot_display_bytes`, etc.) and the WGC backend | ||
| // (`crate::wgc::screenshot_window_via_wgc`) are still in use by | ||
| // `GetWindowStateTool` — that's where the actual screenshot machinery | ||
| // lives now. |
Contributor
There was a problem hiding this comment.
Update the later registry note to match this deletion.
This new comment says both screenshot tool structs are gone, but Lines 4908-4912 still say they're kept in this file for a follow-up. Please clean up that older block too so the file doesn't describe two different states.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs` around lines
2831 - 2838, The file now states that ScreenshotTool and ScreenshotCompatTool
were removed and that get_window_state with capture_mode:"vision" plus the
capture backends (crate::capture::screenshot_window_bytes_with_occlusion,
screenshot_display_bytes, crate::wgc::screenshot_window_via_wgc) and
GetWindowStateTool are the canonical paths; locate the older registry note block
that still mentions keeping ScreenshotTool/ScreenshotCompatTool in this file for
a follow-up and either remove or rewrite that block to match the new state
(i.e., say the two structs were removed and point readers to get_window_state
and GetWindowStateTool and the listed capture functions) so the file no longer
describes two different states.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR #1692 stopped registering the standalone
screenshottool but left the structs / DEF statics /Toolimpls in place as a deletion-deferred follow-up. The Windows daemon's release build flagged 5 warnings as a result:This PR is the deletion follow-up:
platform-windows/src/tools/impl_.rs: drop deadScreenshotTool+ScreenshotCompatToolstruct/impl/static (~225 lines).platform-linux/src/tools/impl_.rs: same, ~170 lines.platform-macos/src/tools: deletescreenshot.rs+screenshot_compat.rsentirely + drop theirmodlines.platform-windows/src/tools/impl_.rsScrollTool: drop the now-unusedbackground_unavailable_errorfrom the import set (replaced by scroll-specific structured error in 9e30d2c).5 files changed, +23 / −675.
Test plan
cargo build --release -p cua-driver— clean, no warnings on Windowscargo check --workspace— clean except for 4 pre-existingplatform-linux/overlay.rswarnings (RenderState::tick,apply_command, etc. — present onmaintoo, out of scope)get_window_statecapture_mode:"vision" still calls into the same underlyingcapture::*andwgc::*helpers — only the deadToolimpl wrappers were removed.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
get_window_stateapproach withcapture_mode: "vision"instead of separate platform-specific screenshot tools.