feat(cua-driver): add drag tool for press-drag-release gestures - #1402
Merged
Conversation
Pixel-addressed drag primitive between two window-local endpoints. macOS AX has no semantic drag action, so this is pixel-only by design — covers marquee selection, drag-and-drop, slider scrub, resize handles, panel repositioning. Routes via .cghidEventTap when frontmost (real cursor traces the path; required for AppKit drag sources / canvas viewports) and via the auth-signed pid-routed path when backgrounded. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR introduces a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/content/docs/cua-driver/reference/mcp-tools.mdx (1)
437-437:⚠️ Potential issue | 🟡 MinorRecording section omits
dragfrom the action-tool list.Line 437 enumerates the action-tool calls captured by the trajectory recorder — but
dragis now inToolRegistry.actionToolNames(line 37 ofToolRegistry.swift) so it is recorded. The list here should be updated for consistency, otherwise users may not know drags participate in replay.📝 Proposed fix
-The trajectory recorder captures every action-tool call (`click`, `right_click`, `scroll`, `type_text`, `type_text_chars`, `press_key`, `hotkey`, `set_value`) into numbered turn folders. Recordings can be replayed turn-by-turn. +The trajectory recorder captures every action-tool call (`click`, `right_click`, `drag`, `scroll`, `type_text`, `type_text_chars`, `press_key`, `hotkey`, `set_value`) into numbered turn folders. Recordings can be replayed turn-by-turn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/cua-driver/reference/mcp-tools.mdx` at line 437, The documentation’s list of action-tool calls is missing the "drag" action even though ToolRegistry.actionToolNames (and the trajectory recorder) includes it; update the sentence that enumerates recorded actions to include `drag` alongside `click`, `right_click`, `scroll`, `type_text`, `type_text_chars`, `press_key`, `hotkey`, and `set_value` so the docs match the actual behavior of the trajectory recorder and ToolRegistry.actionToolNames.
🧹 Nitpick comments (3)
libs/cua-driver/Sources/CuaDriverServer/Tools/DragTool.swift (1)
296-304: Summary string mislabelsfromX/fromY/toX/toYas window-pixel coords when zoom/ratio is in effect.When
fromZoom == true,fromX/fromY/toX/toYare in the zoom image's pixel space, and when anImageResizeRegistry.ratiois in play the inputs are in resized-image pixels. Calling them "window-pixel" in the summary is technically wrong for those cases — the actual window-local pixels areactualFromX/actualFromY/actualToX/actualToY.Either log the post-translation values (
actualFromX, etc.) or qualify the label so a reader debugging from logs isn't misled:♻️ Proposed nit fix
- let summary = - "Posted drag\(buttonSuffix)\(modSuffix) to pid \(pid) " - + "from window-pixel (\(Int(fromX)), \(Int(fromY))) " - + "→ (\(Int(toX)), \(Int(toY))), " - + "screen (\(Int(startScreen.x)), \(Int(startScreen.y))) " - + "→ (\(Int(endScreen.x)), \(Int(endScreen.y))) " - + "in \(durationMs)ms / \(steps) steps." + let summary = + "Posted drag\(buttonSuffix)\(modSuffix) to pid \(pid) " + + "from input (\(Int(fromX)), \(Int(fromY))) " + + "→ (\(Int(toX)), \(Int(toY))), " + + "window-pixel (\(Int(actualFromX)), \(Int(actualFromY))) " + + "→ (\(Int(actualToX)), \(Int(actualToY))), " + + "screen (\(Int(startScreen.x)), \(Int(startScreen.y))) " + + "→ (\(Int(endScreen.x)), \(Int(endScreen.y))) " + + "in \(durationMs)ms / \(steps) steps."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cua-driver/Sources/CuaDriverServer/Tools/DragTool.swift` around lines 296 - 304, The summary string incorrectly labels fromX/fromY/toX/toY as "window-pixel" even when translations (fromZoom or ImageResizeRegistry.ratio) mean those coordinates are in zoom/resized-image pixel space; update the message built in the summary (the block that composes modSuffix, buttonSuffix and summary) to either use the post-translation window-local coordinates actualFromX/actualFromY/actualToX/actualToY or change the label to an accurate qualifier (e.g. "image-pixel" or "window-local pixel (post-translation)") so the logged coordinates reflect their true space; adjust the summary construction where summary is defined to reference the correct variables or updated label.libs/cua-driver/Tests/integration/test_drag_slider_delivery.py (1)
240-243:next(...)will raiseStopIterationifwindow_idisn't in the list — a confusing failure mode in tests.The window resolved on line 228 should always appear in
list_windows, but if Safari closes/refocuses between the two driver calls,nexthere raisesStopIterationrather than a useful test failure.♻️ Proposed defensive nit
- win = next( - w for w in windows - if w["window_id"] == window_id - ) + win = next( + (w for w in windows if w["window_id"] == window_id), + None, + ) + self.assertIsNotNone( + win, f"window_id {window_id} disappeared from list_windows" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cua-driver/Tests/integration/test_drag_slider_delivery.py` around lines 240 - 243, The current use of next(...) to find the window may raise StopIteration if the window_id is missing; change the lookup to use next((w for w in windows if w["window_id"] == window_id), None) and then assert that the result (win) is not None with a clear failure message (e.g., f"Expected window_id {window_id} in list_windows but not found; windows={windows}") so the test fails with a helpful assertion instead of StopIteration; update the code around variables windows, window_id and win accordingly.libs/cua-driver/Sources/CuaDriverCore/Input/MouseInput.swift (1)
468-473: Consider usingnanosleepinstead ofusleepfor portability.POSIX forbids
usleepwithuseconds_t ≥ 1,000,000(undefined behavior), and with schema extremes (duration_ms=10000, steps=1),perStepUsreaches 10,000,000 µs. While modern Darwin tolerates this (delegating internally tonanosleep), it's a portability issue if this code runs on stricter POSIX systems. Usingnanosleepdirectly or chunking the sleep would be cleaner. This only affects corner cases—defaults (duration_ms=500, steps=20) produce 25 ms/step, well within bounds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cua-driver/Sources/CuaDriverCore/Input/MouseInput.swift` around lines 468 - 473, The calculation for per-step sleep uses perStepUs which can exceed 1,000,000 µs and later calls usleep (undefined per POSIX); change the sleep strategy in the_mouse-drag path that uses clampedSteps / clampedDuration (symbols: clampedSteps, clampedDuration, perStepUs) to either call nanosleep with a timespec computed from perStepUs (seconds + nanoseconds) or break the sleep into safe chunks ≤ 1_000_000 µs and loop, replacing any usleep(perStepUs) call; ensure the new code handles interruptions (EINTR) by retrying the remaining time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/cua-driver/Sources/CuaDriverCore/Input/MouseInput.swift`:
- Around line 468-473: The per-step pacing calculation in MouseInput (the
perStepUs computation) is off-by-one because the sequence does an extra sleep
before mouseUp, so the wall-clock budget should be split across clampedSteps+1
gaps, not clampedSteps; update the perStepUs formula to use (clampedSteps + 1)
when computing per-step microseconds, and apply the same change in
dragFrontmostViaHIDTap and its caller to ensure total duration_ms is honored (or
alternatively remove the trailing sleep before mouseUp in both places if you
prefer that approach).
---
Outside diff comments:
In `@docs/content/docs/cua-driver/reference/mcp-tools.mdx`:
- Line 437: The documentation’s list of action-tool calls is missing the "drag"
action even though ToolRegistry.actionToolNames (and the trajectory recorder)
includes it; update the sentence that enumerates recorded actions to include
`drag` alongside `click`, `right_click`, `scroll`, `type_text`,
`type_text_chars`, `press_key`, `hotkey`, and `set_value` so the docs match the
actual behavior of the trajectory recorder and ToolRegistry.actionToolNames.
---
Nitpick comments:
In `@libs/cua-driver/Sources/CuaDriverCore/Input/MouseInput.swift`:
- Around line 468-473: The calculation for per-step sleep uses perStepUs which
can exceed 1,000,000 µs and later calls usleep (undefined per POSIX); change the
sleep strategy in the_mouse-drag path that uses clampedSteps / clampedDuration
(symbols: clampedSteps, clampedDuration, perStepUs) to either call nanosleep
with a timespec computed from perStepUs (seconds + nanoseconds) or break the
sleep into safe chunks ≤ 1_000_000 µs and loop, replacing any usleep(perStepUs)
call; ensure the new code handles interruptions (EINTR) by retrying the
remaining time.
In `@libs/cua-driver/Sources/CuaDriverServer/Tools/DragTool.swift`:
- Around line 296-304: The summary string incorrectly labels fromX/fromY/toX/toY
as "window-pixel" even when translations (fromZoom or ImageResizeRegistry.ratio)
mean those coordinates are in zoom/resized-image pixel space; update the message
built in the summary (the block that composes modSuffix, buttonSuffix and
summary) to either use the post-translation window-local coordinates
actualFromX/actualFromY/actualToX/actualToY or change the label to an accurate
qualifier (e.g. "image-pixel" or "window-local pixel (post-translation)") so the
logged coordinates reflect their true space; adjust the summary construction
where summary is defined to reference the correct variables or updated label.
In `@libs/cua-driver/Tests/integration/test_drag_slider_delivery.py`:
- Around line 240-243: The current use of next(...) to find the window may raise
StopIteration if the window_id is missing; change the lookup to use next((w for
w in windows if w["window_id"] == window_id), None) and then assert that the
result (win) is not None with a clear failure message (e.g., f"Expected
window_id {window_id} in list_windows but not found; windows={windows}") so the
test fails with a helpful assertion instead of StopIteration; update the code
around variables windows, window_id and win accordingly.
🪄 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: 2f655d10-507f-4afc-8c6a-5a67252af4ce
📒 Files selected for processing (7)
docs/content/docs/cua-driver/reference/cli-reference.mdxdocs/content/docs/cua-driver/reference/mcp-tools.mdxlibs/cua-driver/Skills/cua-driver/SKILL.mdlibs/cua-driver/Sources/CuaDriverCore/Input/MouseInput.swiftlibs/cua-driver/Sources/CuaDriverServer/ToolRegistry.swiftlibs/cua-driver/Sources/CuaDriverServer/Tools/DragTool.swiftlibs/cua-driver/Tests/integration/test_drag_slider_delivery.py
Comment on lines
+468
to
+473
| let clampedSteps = max(1, min(200, steps)) | ||
| let clampedDuration = max(0, min(10_000, durationMs)) | ||
| // Split the wall-clock budget across the dragged-step gaps. | ||
| // `clampedSteps` intermediate points produce `clampedSteps` | ||
| // gaps between down → first-drag → … → last-drag → up. | ||
| let perStepUs = useconds_t((clampedDuration * 1_000) / clampedSteps) |
Contributor
There was a problem hiding this comment.
Per-step pacing has an off-by-one vs the documented contract.
The comment on lines 470–472 says clampedSteps gaps, but the actual posting sequence is:
down → [sleep, drag]×N → sleep, up
That's N+1 sleeps of perStepUs each, so total wall time ≈ (N+1)/N × duration_ms. At the default N=20 it's a benign ~5% overshoot, but at the schema-allowed minimum steps=1 it's a full 2× overshoot — duration_ms=500, steps=1 lands as a 1 s drag.
Either drop the trailing sleep before mouseUp, or split the budget across N+1 gaps. The latter matches the comment more closely:
♻️ Proposed fix
- let clampedSteps = max(1, min(200, steps))
- let clampedDuration = max(0, min(10_000, durationMs))
- // Split the wall-clock budget across the dragged-step gaps.
- // `clampedSteps` intermediate points produce `clampedSteps`
- // gaps between down → first-drag → … → last-drag → up.
- let perStepUs = useconds_t((clampedDuration * 1_000) / clampedSteps)
+ let clampedSteps = max(1, min(200, steps))
+ let clampedDuration = max(0, min(10_000, durationMs))
+ // Split the wall-clock budget across all gaps:
+ // down → drag₁ → … → dragₙ → up
+ // is `clampedSteps + 1` gaps. Without the +1 we overshoot
+ // `duration_ms` by one extra `perStepUs` (significant at low
+ // step counts).
+ let perStepUs = useconds_t((clampedDuration * 1_000) / (clampedSteps + 1))Apply the same change in dragFrontmostViaHIDTap (line 549 / its caller).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/cua-driver/Sources/CuaDriverCore/Input/MouseInput.swift` around lines
468 - 473, The per-step pacing calculation in MouseInput (the perStepUs
computation) is off-by-one because the sequence does an extra sleep before
mouseUp, so the wall-clock budget should be split across clampedSteps+1 gaps,
not clampedSteps; update the perStepUs formula to use (clampedSteps + 1) when
computing per-step microseconds, and apply the same change in
dragFrontmostViaHIDTap and its caller to ensure total duration_ms is honored (or
alternatively remove the trailing sleep before mouseUp in both places if you
prefer that approach).
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
dragMCP tool to cua-driver: pixel-addressed press-drag-release gesture between two window-local endpoints.element_indexmode (unlikeclick/double_click). Address an element viaget_window_state, read itsbounds, pass pixel coordinates.cmd/shift/option/ctrl) held across the gesture..cghidEventTapwhen target is frontmost (real cursor traces the path — required for AppKit drag sources / canvas viewports); auth-signed pid-routed path when backgrounded (cursor-neutral).duration_ms(default 500),steps(default 20),button(left/right/middle),from_zoomfor zoom-image coordinates.mcp-tools.mdx,cli-reference.mdx,SKILL.md. Registered inToolRegistryaction list.Test plan
swift buildclean inlibs/cua-driverpytest libs/cua-driver/Tests/integration/test_drag_slider_delivery.py— slider-scrub delivery test passes🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
dragtool enabling mouse drag interactions from point A to point B with configurable timing, gesture steps, button type, and modifier keys.Documentation
Tests