Ydotool linux cursor#184
Conversation
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
|
Warning Rate limit exceeded
⌛ 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 (1)
WalkthroughReplaces direct mouse movement in InputHandler with a CursorController abstraction that uses nut.js and falls back to ydotool on Linux; adds new cursor-controller and ydotool-cursor-handler modules, singleton management, status reporting, and error/cooldown handling. Changes
Sequence DiagramsequenceDiagram
participant InputHandler
participant CursorController
participant NutJS as nut.js
participant YdotoolHandler
participant ydotool
InputHandler->>CursorController: moveToAbsolute(x,y)
CursorController->>CursorController: validate & round coords
CursorController->>NutJS: attempt move (x,y)
alt nut.js succeeds
NutJS-->>CursorController: success
CursorController-->>InputHandler: MoveCursorResult(success=true, method='nut.js')
else nut.js fails
NutJS-->>CursorController: error
CursorController->>CursorController: increment error & check cooldown
CursorController->>YdotoolHandler: tryYdotoolFallback(x,y)
YdotoolHandler->>ydotool: spawn move (x,y)
ydotool-->>YdotoolHandler: result
YdotoolHandler-->>CursorController: fallback result
CursorController-->>InputHandler: MoveCursorResult(success=bool, method='ydotool'|'none')
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/cursor-controller.ts`:
- Around line 185-196: getStatus currently returns nutJsAvailable: true which is
misleading; instead track actual runtime success in a boolean
(this.nutJsWorking) and return that value from getStatus(). Update getStatus()
to use this.nutJsWorking (e.g., nutJsAvailable: this.nutJsWorking) and ensure
any initialization/handler methods that attempt to start or use nut.js set
this.nutJsWorking = false on repeated or fatal failures and true on successful
initialization so runtime availability is accurately reflected.
In `@src/server/InputHandler.ts`:
- Around line 94-98: The snippet in InputHandler.ts has inconsistent indentation
for the block computing targetX/targetY and calling
cursorController.moveToAbsolute; fix it by aligning these statements with the
surrounding method body/indentation level inside the relevant method in the
InputHandler class so that const targetX, const targetY, the await
this.cursorController.moveToAbsolute(...) call and the console.log(...) line
share the same indentation as neighboring statements; verify the variables
targetX/targetY and the call to moveToAbsolute remain unchanged and that no
extra whitespace or tabs are introduced.
- Around line 92-98: The code mixes direct mouse API and the cursor controller:
it calls mouse.getPosition() but uses this.cursorController.moveToAbsolute(...)
for movement; replace the direct call with
this.cursorController.getCurrentPosition() so position reading goes through the
same abstraction and unified error handling, then compute targetX/targetY from
that position and call this.cursorController.moveToAbsolute(...) as before;
ensure you handle or propagate any errors from getCurrentPosition() consistently
with moveToAbsolute().
In `@src/server/ydotool-cursor-handler.ts`:
- Around line 103-108: The current use of spawnSync in ydotool-cursor-handler.ts
(inside the block that spawns 'ydotool') will block the event loop under
high-frequency cursor updates; change this to use the async child_process.spawn
(or spawn wrapped in a Promise) and await its completion instead of spawnSync,
ensuring you capture stdout/stderr, enforce the 5000ms timeout by using a timer
that kills the child on timeout, and propagate errors; update
CursorController.tryYdotoolFallback to be async (returning a Promise<boolean> or
similar) and adjust callers to await it so the fallback runs non-blockingly and
handles success/failure properly.
- Around line 103-108: The ydotool invocation in spawnSync is missing the
--absolute flag so mousemove treats roundedX/roundedY as relative deltas; update
the args passed to spawnSync in the function using spawnSync (the call that
currently uses ['mousemove', String(roundedX), String(roundedY)]) to
['mousemove', '--absolute', String(roundedX), String(roundedY)] and ensure
stdio/timeout/encoding options remain unchanged; after changing, verify ydotool
version handles --absolute correctly (some versions have a bug that moves to
0,0).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/server/InputHandler.tssrc/server/cursor-controller.tssrc/server/ydotool-cursor-handler.ts
| getStatus(): SystemStatus { | ||
| const ydotoolStatus = this.ydotoolHandler.getStatus(); | ||
|
|
||
| return { | ||
| nutJsAvailable: true, // Always available as it's a dependency | ||
| ydotoolFallbackAvailable: ydotoolStatus.available, | ||
| ydotoolMessage: ydotoolStatus.message, | ||
| activeMethod: ydotoolStatus.available | ||
| ? 'nut.js (with ydotool fallback)' | ||
| : 'nut.js only', | ||
| }; | ||
| } |
There was a problem hiding this comment.
nutJsAvailable: true is misleading.
nut.js being a dependency doesn't guarantee runtime availability. It can fail on systems without proper display server access, missing native dependencies, or headless environments. Consider tracking actual nut.js success/failure state.
🛠️ Proposed improvement
export class CursorController {
private ydotoolHandler = getYdotoolHandler();
private lastErrorTime = 0;
private consecutiveErrors = 0;
+ private nutJsWorking = true;
private readonly MAX_CONSECUTIVE_ERRORS = 5;
private readonly ERROR_COOLDOWN_MS = 1000;Then update getStatus():
return {
- nutJsAvailable: true, // Always available as it's a dependency
+ nutJsAvailable: this.nutJsWorking,
ydotoolFallbackAvailable: ydotoolStatus.available,And set this.nutJsWorking = false when nut.js consistently fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/cursor-controller.ts` around lines 185 - 196, getStatus currently
returns nutJsAvailable: true which is misleading; instead track actual runtime
success in a boolean (this.nutJsWorking) and return that value from getStatus().
Update getStatus() to use this.nutJsWorking (e.g., nutJsAvailable:
this.nutJsWorking) and ensure any initialization/handler methods that attempt to
start or use nut.js set this.nutJsWorking = false on repeated or fatal failures
and true on successful initialization so runtime availability is accurately
reflected.
| const currentPos = await mouse.getPosition(); | ||
|
|
||
| await mouse.setPosition(new Point( | ||
| Math.round(currentPos.x + msg.dx), | ||
| Math.round(currentPos.y + msg.dy) | ||
| )); | ||
| const targetX = Math.round(currentPos.x + msg.dx); | ||
| const targetY = Math.round(currentPos.y + msg.dy); | ||
|
|
||
| const result = await this.cursorController.moveToAbsolute(targetX, targetY); | ||
| console.log(`[Input] ${result.message}`); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent cursor abstraction usage.
Line 92 uses mouse.getPosition() directly while line 97 uses the cursor controller for movement. For consistency and to benefit from unified error handling, consider using this.cursorController.getCurrentPosition() for reading position as well.
♻️ Consistent cursor controller usage
- const currentPos = await mouse.getPosition();
+ const currentPos = await this.cursorController.getCurrentPosition();
+ if (!currentPos) {
+ console.warn('[Input] Could not get cursor position');
+ break;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/InputHandler.ts` around lines 92 - 98, The code mixes direct mouse
API and the cursor controller: it calls mouse.getPosition() but uses
this.cursorController.moveToAbsolute(...) for movement; replace the direct call
with this.cursorController.getCurrentPosition() so position reading goes through
the same abstraction and unified error handling, then compute targetX/targetY
from that position and call this.cursorController.moveToAbsolute(...) as before;
ensure you handle or propagate any errors from getCurrentPosition() consistently
with moveToAbsolute().
| const targetX = Math.round(currentPos.x + msg.dx); | ||
| const targetY = Math.round(currentPos.y + msg.dy); | ||
|
|
||
| const result = await this.cursorController.moveToAbsolute(targetX, targetY); | ||
| console.log(`[Input] ${result.message}`); |
There was a problem hiding this comment.
Fix inconsistent indentation.
Lines 94-98 have broken indentation that doesn't match the surrounding code structure.
🔧 Proposed fix
- const targetX = Math.round(currentPos.x + msg.dx);
- const targetY = Math.round(currentPos.y + msg.dy);
-
- const result = await this.cursorController.moveToAbsolute(targetX, targetY);
- console.log(`[Input] ${result.message}`);
+ const targetX = Math.round(currentPos.x + msg.dx);
+ const targetY = Math.round(currentPos.y + msg.dy);
+
+ const result = await this.cursorController.moveToAbsolute(targetX, targetY);
+ console.log(`[Input] ${result.message}`);📝 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.
| const targetX = Math.round(currentPos.x + msg.dx); | |
| const targetY = Math.round(currentPos.y + msg.dy); | |
| const result = await this.cursorController.moveToAbsolute(targetX, targetY); | |
| console.log(`[Input] ${result.message}`); | |
| const targetX = Math.round(currentPos.x + msg.dx); | |
| const targetY = Math.round(currentPos.y + msg.dy); | |
| const result = await this.cursorController.moveToAbsolute(targetX, targetY); | |
| console.log(`[Input] ${result.message}`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/InputHandler.ts` around lines 94 - 98, The snippet in
InputHandler.ts has inconsistent indentation for the block computing
targetX/targetY and calling cursorController.moveToAbsolute; fix it by aligning
these statements with the surrounding method body/indentation level inside the
relevant method in the InputHandler class so that const targetX, const targetY,
the await this.cursorController.moveToAbsolute(...) call and the
console.log(...) line share the same indentation as neighboring statements;
verify the variables targetX/targetY and the call to moveToAbsolute remain
unchanged and that no extra whitespace or tabs are introduced.
| // Spawn ydotool process | ||
| const result = spawnSync('ydotool', ['mousemove', String(roundedX), String(roundedY)], { | ||
| stdio: 'pipe', | ||
| timeout: 5000, | ||
| encoding: 'utf8', | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Synchronous spawnSync may block the event loop on high-frequency cursor movements.
For cursor movements occurring at ~125fps (8ms intervals), synchronous process spawning could degrade responsiveness. Consider using async spawn with promise-based handling, or document this as an acceptable trade-off for the fallback path.
♻️ Async alternative using spawn
+import { execSync, spawnSync, spawn } from 'child_process';
+
+ public moveCursorAbsolute(x: number, y: number): Promise<boolean> {
+ return new Promise((resolve) => {
+ if (!this.available) {
+ resolve(false);
+ return;
+ }
+
+ try {
+ if (!Number.isFinite(x) || !Number.isFinite(y)) {
+ console.warn('[ydotool] Invalid coordinates:', { x, y });
+ resolve(false);
+ return;
+ }
+
+ const roundedX = Math.round(x);
+ const roundedY = Math.round(y);
+
+ const proc = spawn('ydotool', ['mousemove', '--absolute', String(roundedX), String(roundedY)]);
+ const timeout = setTimeout(() => {
+ proc.kill();
+ resolve(false);
+ }, 5000);
+
+ proc.on('close', (code) => {
+ clearTimeout(timeout);
+ resolve(code === 0);
+ });
+
+ proc.on('error', () => {
+ clearTimeout(timeout);
+ resolve(false);
+ });
+ } catch {
+ resolve(false);
+ }
+ });
+ }Note: If you switch to async, you'll need to update CursorController.tryYdotoolFallback accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/ydotool-cursor-handler.ts` around lines 103 - 108, The current use
of spawnSync in ydotool-cursor-handler.ts (inside the block that spawns
'ydotool') will block the event loop under high-frequency cursor updates; change
this to use the async child_process.spawn (or spawn wrapped in a Promise) and
await its completion instead of spawnSync, ensuring you capture stdout/stderr,
enforce the 5000ms timeout by using a timer that kills the child on timeout, and
propagate errors; update CursorController.tryYdotoolFallback to be async
(returning a Promise<boolean> or similar) and adjust callers to await it so the
fallback runs non-blockingly and handles success/failure properly.
🧩 Analysis chain
🌐 Web query:
ydotool mousemove absolute positioning flag syntax documentation
💡 Result:
ydotool mousemove absolute-positioning is enabled with --absolute (short form -a in some builds/manpages). The documentation isn’t fully consistent across versions:
-
Repo README example (flagged axes):
ydotool mousemove --absolute -x 100 -y 100
(README also shows relative movement without
--absolute.)[1] -
openSUSE manpage (positional args):
ydotool mousemove [--absolute] <x> <y> # example: ydotool mousemove --absolute 100 100 ```[2]
Because of this mismatch, the most reliable way to confirm the exact syntax your installed ydotool expects is:
ydotool mousemove --help
```[1]
Note: there are reports that `mousemove --absolute` may be broken in some versions (always moving to 0,0 / top-left).[3]
Sources: [1] [2] [3]
---
</details>
**Add `--absolute` flag to ydotool mousemove command.**
The `ydotool mousemove` command requires the `--absolute` flag to interpret coordinates as absolute screen positions. Without it, coordinates are treated as relative movement from the current cursor position. Update the command to `['mousemove', '--absolute', String(roundedX), String(roundedY)]`.
Note: Some ydotool versions have a bug where `--absolute` moves the cursor to (0,0). Verify the installed ydotool version supports absolute positioning correctly.
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @src/server/ydotool-cursor-handler.ts around lines 103 - 108, The ydotool
invocation in spawnSync is missing the --absolute flag so mousemove treats
roundedX/roundedY as relative deltas; update the args passed to spawnSync in the
function using spawnSync (the call that currently uses ['mousemove',
String(roundedX), String(roundedY)]) to ['mousemove', '--absolute',
String(roundedX), String(roundedY)] and ensure stdio/timeout/encoding options
remain unchanged; after changing, verify ydotool version handles --absolute
correctly (some versions have a bug that moves to 0,0).
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/InputHandler.ts`:
- Around line 73-75: The log message incorrectly says "pending move event" for
the scroll-event handler; update the error message in the catch of the
this.handleMessage(pending) call to reflect that it's processing a pending
scroll event (e.g., "Error processing pending scroll event:"), referencing the
this.handleMessage(pending) invocation and the pending variable so the message
matches the handled event.
- Line 1: Remove the unused imported symbol Point from the import statement in
src/server/InputHandler.ts; update the import line that currently reads "import
{ mouse, Point, Button, keyboard, Key } from '@nut-tree-fork/nut-js';" to
exclude Point so only used symbols (mouse, Button, keyboard, Key) remain.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
#127
I Testing on :
npm run dev
When you move cursor from mobile, console show
[Input] Cursor moved to (500, 300)
[Input] Cursor moved to (505, 310)
[Input] Cursor moved to (510, 320)
Functional Verification
Basic Gestures
One-finger tap: Verified as Left Click.
Two-finger tap: Verified as Right Click.
Click and drag: Verified selection behavior.
Pinch to zoom: Verified zoom functionality (if applicable).
Modes & Settings
Cursor mode: Cursor moves smoothly and accurately.
Scroll mode: Page scrolls as expected.
Sensitivity: Verified changes in cursor speed/sensitivity settings.
Invert Scrolling: Verified scroll direction toggles correctly.
Advanced Input
Key combinations: Verified "hold" behavior for modifiers (e.g., Ctrl+C) and held keys are shown in buffer.
Keyboard input: Verified Space, Backspace, and Enter keys work correctly.
Glide typing: Verified path drawing and text output.
Voice input: Verified speech-to-text functionality.
Backspace doesn't send the previous input.
Any other gesture or input behavior introduced:
Additional Notes:
Checklist
My PR addresses a single issue, fixes a single bug or makes a single improvement.
My code follows the project's code style and conventions
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
If applicable, I have made corresponding changes or additions to the documentation
If applicable, I have made corresponding changes or additions to tests
My changes generate no new warnings or errors
I have joined the and I will share a link to this PR with the project maintainers there
I have read the
Once I submit my PR, CodeRabbit AI will automatically review it and I will address CodeRabbit's comments.
Incase of UI change I've added a demo video.
Summary by CodeRabbit