-
Notifications
You must be signed in to change notification settings - Fork 35
fix computer controls hang #92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Mesa Descriptionusing --sync was ill-advised, it doesn't seem to detect when no mouse movements are necessary to achieve a mousemove, so getting rid of it for now Description generated by Mesa. Update settings |
|
tested with this, which randomly hangs for 15s prior to the change #!/bin/bash
# Test script for scroll down endpoint - runs 10 times with timing
BASE_URL="http://localhost:444"
echo "=== Testing /computer/scroll (vertical down) - 10 iterations ==="
echo ""
total_time=0
times=()
for i in {1..10}; do
start_time=$(date +%s.%N)
curl -X POST "${BASE_URL}/computer/scroll" \
-H "Content-Type: application/json" \
-d '{
"x": 500,
"y": 500,
"delta_y": 3
}' \
-s -o /dev/null
end_time=$(date +%s.%N)
elapsed=$(awk "BEGIN {printf \"%.4f\", $end_time - $start_time}")
times+=($elapsed)
total_time=$(awk "BEGIN {printf \"%.4f\", $total_time + $elapsed}")
echo "Iteration $i: ${elapsed}s"
done
echo ""
echo "=== Results ==="
echo "Total time: ${total_time}s"
avg_time=$(awk "BEGIN {printf \"%.4f\", $total_time / 10}")
echo "Average time: ${avg_time}s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performed full review of ce46982...429cec9
Analysis
-
Removing the
--syncflag from xdotool mousemove commands eliminates hanging issues but introduces potential race conditions where subsequent mouse actions (clicks, drags) may occur before the cursor reaches its intended position. -
The implementation lacks any alternative synchronization mechanism to ensure cursor positioning completes before dependent operations execute, which could lead to clicks, drags, and scrolls happening at incorrect coordinates.
-
No specific testing has been implemented to verify operations work correctly at boundary conditions, such as when the cursor needs to move significant distances versus when it's already at the target position.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
1 files reviewed | 0 comments | Edit Agent Settings • Read Docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performed full review of ce46982...429cec9
Analysis
-
Removing
--synctrades synchronization guarantees for performance, potentially introducing race conditions if any calling code assumes mouse operations complete before the function returns. -
No tests were added to prevent regression or verify that mouse operations no longer hang, leaving the fix vulnerable to future changes.
-
The PR lacks documentation updates explaining the new asynchronous behavior of mouse operations, which could lead to incorrect usage by other developers.
-
No consideration of adding alternative verification logic or retry mechanisms for critical operations where position accuracy is essential.
-
Inconsistent approach to synchronization across the codebase - some mouse operations already didn't use
--sync, suggesting this problem was partially understood before.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
1 files reviewed | 0 comments | Edit Agent Settings • Read Docs
using --sync was ill-advised, it doesn't seem to detect when no mouse movements are necessary to achieve a mousemove, so getting rid of it for now
Note
Removes the
--syncflag from allxdotool mousemovecalls to prevent hanging during mouse operations.server/cmd/api/api/computer.go:--syncfromxdotool mousemovein:MoveMouseClickMouseScrollDragMouse(initial move to start point)Written by Cursor Bugbot for commit 429cec9. This will update automatically on new commits. Configure here.