fix: improve error handling in InputHandler to prevent stuck keys#241
Closed
kunal-bunkar wants to merge 3 commits into
Closed
fix: improve error handling in InputHandler to prevent stuck keys#241kunal-bunkar wants to merge 3 commits into
kunal-bunkar wants to merge 3 commits into
Conversation
Contributor
WalkthroughAdded localized try/catch/finally error handling across input processing in InputHandler to catch and log nut-js failures, ensure pressed keys and mouse buttons are released, and prevent uncaught exceptions or stuck input states. Also adjusted concurrent operation error handling and updated package manifest. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ 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 |
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 (2)
src/server/InputHandler.ts (2)
368-373:⚠️ Potential issue | 🟡 MinorAvoid success logging after a caught combo failure.
Line 372 logs
Combo completeeven when Line 368 catches an error, which makes operational logs misleading.🔧 Proposed fix
- try { + let comboSucceeded = false + try { const nutKeys: (Key | string)[] = [] @@ - } catch (err) { + comboSucceeded = true + } catch (err) { console.error("Error handling combo input:", err, msg.keys) } - console.log(`Combo complete: ${msg.keys.join("+")}`) + if (comboSucceeded) { + console.log(`Combo complete: ${msg.keys.join("+")}`) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 368 - 373, The success log "Combo complete: ${msg.keys.join("+")}" is executed even when the preceding try/catch caught an error; move the success logging so it only runs on successful completion (e.g., place the console.log inside the try block that wraps the combo handling logic, or use a local success flag set to true at the end of the try and log only if the flag is true) and keep the catch block only for console.error("Error handling combo input:", err, msg.keys) to avoid misleading logs; update the code around the try/catch that surrounds the combo handling in InputHandler (referencing msg.keys) accordingly.
241-259:⚠️ Potential issue | 🔴 CriticalMove zoom modifier press inside the guarded block to prevent uncaught failures.
Line 241 can throw before entering the
try/catch, which still allowshandleMessage()to reject on zoom failures.🔧 Proposed fix
- if (amount !== 0) { - await keyboard.pressKey(Key.LeftControl) - try { - if (amount > 0) { - await mouse.scrollDown(amount) - } else { - await mouse.scrollUp(-amount) - } - } catch (err) { - console.error("Error handling zoom input:", err) - } finally { - await keyboard - .releaseKey(Key.LeftControl) - .catch((releaseErr) => { - console.error( - "Error releasing control key after zoom:", - releaseErr, - ) - }) - } - } + if (amount !== 0) { + let ctrlPressed = false + try { + await keyboard.pressKey(Key.LeftControl) + ctrlPressed = true + if (amount > 0) { + await mouse.scrollDown(amount) + } else { + await mouse.scrollUp(-amount) + } + } catch (err) { + console.error("Error handling zoom input:", err) + } finally { + if (ctrlPressed) { + await keyboard + .releaseKey(Key.LeftControl) + .catch((releaseErr) => { + console.error( + "Error releasing control key after zoom:", + releaseErr, + ) + }) + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 241 - 259, The zoom modifier key press (keyboard.pressKey(Key.LeftControl)) must be moved inside the try so errors from pressing don't escape the guarded block; update the block around mouse.scrollDown/scrollUp to call keyboard.pressKey(Key.LeftControl) at the start of the try, maintain a local flag (e.g., controlPressed) set only if pressKey succeeds, and in finally call keyboard.releaseKey(Key.LeftControl) only when controlPressed is true (still catching release errors via .catch). Ensure mouse.scrollDown/scrollUp and keyboard.releaseKey remain in the try/finally structure so handleMessage won't reject on zoom failures.
🤖 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 249-259: The code formatting in the finally block around the zoom
handling (the await keyboard.releaseKey(Key.LeftControl).catch(...) call and its
console.error callback) is incorrect and causing a Biome lint failure; reformat
that block to match project style (consistent indentation, spacing, trailing
commas, and bracket placement) so the finally block looks clean and
single-indented under the try/finally, keeping the same identifiers (keyboard,
releaseKey, Key.LeftControl, releaseErr, console.error) and the existing
.catch(...) error handler intact.
---
Outside diff comments:
In `@src/server/InputHandler.ts`:
- Around line 368-373: The success log "Combo complete: ${msg.keys.join("+")}"
is executed even when the preceding try/catch caught an error; move the success
logging so it only runs on successful completion (e.g., place the console.log
inside the try block that wraps the combo handling logic, or use a local success
flag set to true at the end of the try and log only if the flag is true) and
keep the catch block only for console.error("Error handling combo input:", err,
msg.keys) to avoid misleading logs; update the code around the try/catch that
surrounds the combo handling in InputHandler (referencing msg.keys) accordingly.
- Around line 241-259: The zoom modifier key press
(keyboard.pressKey(Key.LeftControl)) must be moved inside the try so errors from
pressing don't escape the guarded block; update the block around
mouse.scrollDown/scrollUp to call keyboard.pressKey(Key.LeftControl) at the
start of the try, maintain a local flag (e.g., controlPressed) set only if
pressKey succeeds, and in finally call keyboard.releaseKey(Key.LeftControl) only
when controlPressed is true (still catching release errors via .catch). Ensure
mouse.scrollDown/scrollUp and keyboard.releaseKey remain in the try/finally
structure so handleMessage won't reject on zoom failures.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/InputHandler.ts (1)
241-257:⚠️ Potential issue | 🔴 CriticalWrap
pressKeyinside the zoomtry/finallyblock.At line 241,
await keyboard.pressKey(Key.LeftControl)executes before the try block. If that call rejects, the error propagates without being caught by this zoom block's catch/finally path, and the control key is never released. Move the press into the try block so error handling and key release stay in one containment scope.🔧 Proposed fix
if (amount !== 0) { - await keyboard.pressKey(Key.LeftControl) try { + await keyboard.pressKey(Key.LeftControl) if (amount > 0) { await mouse.scrollDown(amount) } else { await mouse.scrollUp(-amount) } } catch (err) { console.error("Error handling zoom input:", err) } finally { await keyboard.releaseKey(Key.LeftControl).catch((releaseErr) => { console.error( "Error releasing control key after zoom:", releaseErr, ) }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 241 - 257, The call to keyboard.pressKey(Key.LeftControl) is outside the try/finally so if pressKey rejects the catch/finally won't run and the control key may never be released; move the await keyboard.pressKey(Key.LeftControl) inside the same try block that calls mouse.scrollDown/mouse.scrollUp and ensure keyboard.releaseKey(Key.LeftControl) remains in the finally (handling release errors as before) so press, scroll (mouse.scrollDown/mouse.scrollUp), and release are all within one try/finally scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/server/InputHandler.ts`:
- Around line 241-257: The call to keyboard.pressKey(Key.LeftControl) is outside
the try/finally so if pressKey rejects the catch/finally won't run and the
control key may never be released; move the await
keyboard.pressKey(Key.LeftControl) inside the same try block that calls
mouse.scrollDown/mouse.scrollUp and ensure keyboard.releaseKey(Key.LeftControl)
remains in the finally (handling release errors as before) so press, scroll
(mouse.scrollDown/mouse.scrollUp), and release are all within one try/finally
scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb923340-b78a-4c7a-860c-cdc47f70de4d
📒 Files selected for processing (1)
src/server/InputHandler.ts
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
2 participants
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.
Addressed Issues:
Fixes #202
Description
This PR improves error handling in
src/server/InputHandler.tsto prevent stuck keys or mouse state when@nut-tree-fork/nut-jsoperations fail (e.g., screen lock, permission/accessibility changes, OS-level failures).Changes made:
try/catchblocks around nut-js keyboard/mouse operations inmove,click,scroll,zoom,key,combo, andtexthandlers so errors don’t propagate out ofhandleMessage().pressKey()calls are paired withreleaseKey()infinally, and release failures are logged (using.catch) instead of throwing.Screen Mirror
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
Summary by CodeRabbit