Sec/inputhandle

[imxade]

Fixes #202

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability of input operations with expanded error handling and clearer error logging for moves, clicks, scrolls, key presses, combos, and text input
  • Added guaranteed release/fallback paths to prevent input states from becoming stuck when failures occur
  • Enhanced batch operation handling so individual failures are logged without blocking other operations

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 81e66ac0-5253-4ca1-a255-741e70d4ae9c

📥 Commits

Reviewing files that changed from the base of the PR and between d664346 and f9ac743.

📒 Files selected for processing (1)

• src/server/InputHandler.ts

---

Walkthrough

Enhanced error handling in src/server/InputHandler.ts: added localized try/catch blocks, per-operation error logging, Promise.allSettled for batch cleanup, and guaranteed release/cleanup paths for move, click, scroll, key, combo, and text handlers to avoid stuck input states.

Changes

Cohort / File(s)	Summary
Input Error Handling & Resilience src/server/InputHandler.ts	Wrapped move, click, scroll, key, combo, and text handling in try/catch; added explicit error logging; used Promise.allSettled for batch operations (scroll/combo cleanup); ensured release/cleanup paths (key releases, mouse release) and preserved fallback behavior for failed relative moves.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Sec/inputhandle #261: Applies the same try/catch, Promise.allSettled, and guaranteed-release changes to src/server/InputHandler.ts — directly overlaps these edits.
• Harden InputHandler Against Event Flooding and Oversized Payload Attacks (Add Rate Limiting & Input Safeguards) #87: Changes input handlers (move/scroll) with overlapping logic; may conflict or complement rate-limiting/sanitation around the same handlers.

Suggested labels

Typescript Lang

Poem

🐇 I hopped through code with careful paws,
Wrapped every press in gentle laws.
If keys should stumble, fear no fright,
Try/catch and releases set things right.
A tidy hop, the input's light ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The pull request description only contains 'Fixes #202' and is missing most required sections from the template including detailed description, screenshots, and functional verification.	Complete the description template by adding detailed explanation of changes, functional verification checklist, and additional notes for reviewers.
Title check	❓ Inconclusive	The title 'Sec/inputhandle' is vague and does not clearly convey the main change of improving error handling in InputHandler.	Use a more descriptive title such as 'Improve error handling in InputHandler to prevent stuck keys' that clearly summarizes the change.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The code changes in InputHandler.ts successfully implement all primary objectives from issue #202: added try/catch blocks, ensured key cleanup in finally blocks, and implemented Promise.allSettled for batch error handling.
Out of Scope Changes check	✅ Passed	All changes are scoped to InputHandler.ts error handling improvements as required by issue #202; the minor interface field reordering is directly related to the error handling refactoring.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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)

234-258: ⚠️ Potential issue | 🟠 Major

Zoom handler missing outer try/catch - LeftControl can get stuck.

The keyboard.pressKey(Key.LeftControl) on line 246 is outside the try block. If this call throws, the finally block won't execute and LeftControl will remain pressed. This contradicts the PR objective of ensuring guaranteed key release.

🐛 Proposed fix to wrap the entire zoom operation

 			case "zoom":
 				if (this.isFiniteNumber(msg.delta) && msg.delta !== 0) {
 					const sensitivityFactor = 0.5
 					const MAX_ZOOM_STEP = 5

 					const scaledDelta =
 						Math.sign(msg.delta) *
 						Math.min(Math.abs(msg.delta) * sensitivityFactor, MAX_ZOOM_STEP)

 					const amount = Math.round(-scaledDelta)

 					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("Zoom event failed:", err)
 						} finally {
-							await keyboard.releaseKey(Key.LeftControl)
+							await keyboard.releaseKey(Key.LeftControl).catch(() => {})
 						}
 					}
 				}
 				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 234 - 258, In the "zoom" case of
InputHandler.ts the keyboard.pressKey(Key.LeftControl) is called before the
try/finally, so if pressKey throws LeftControl can stay stuck; move the pressKey
call inside a try block (so the try begins before or immediately after calling
keyboard.pressKey) and ensure keyboard.releaseKey(Key.LeftControl) remains in
the finally to always run; in other words, wrap the entire zoom operation
(including keyboard.pressKey, the mouse.scrollDown/scrollUp calls, and any
conditional logic that computes amount) in a try/finally that calls
keyboard.releaseKey(Key.LeftControl) in finally, keeping the existing amount
checks and mouse.scrollDown/scrollUp behavior.

---

6-25: ⚠️ Potential issue | 🟡 Minor

Address formatting check failure before merge.

The CI pipeline reports a formatting mismatch. Run the formatter to fix code style issues:

npx biome format --write src/server/InputHandler.ts

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/server/InputHandler.ts` around lines 6 - 25, The CI failed due to
formatting mismatches in the InputMessage interface; run the formatter to fix
style (e.g. run `npx biome format --write src/server/InputHandler.ts`), then
stage and commit the updated file so the InputMessage interface definition and
surrounding whitespace/indentation match project formatting rules before
re-pushing the branch.

🤖 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 234-258: In the "zoom" case of InputHandler.ts the
keyboard.pressKey(Key.LeftControl) is called before the try/finally, so if
pressKey throws LeftControl can stay stuck; move the pressKey call inside a try
block (so the try begins before or immediately after calling keyboard.pressKey)
and ensure keyboard.releaseKey(Key.LeftControl) remains in the finally to always
run; in other words, wrap the entire zoom operation (including
keyboard.pressKey, the mouse.scrollDown/scrollUp calls, and any conditional
logic that computes amount) in a try/finally that calls
keyboard.releaseKey(Key.LeftControl) in finally, keeping the existing amount
checks and mouse.scrollDown/scrollUp behavior.
- Around line 6-25: The CI failed due to formatting mismatches in the
InputMessage interface; run the formatter to fix style (e.g. run `npx biome
format --write src/server/InputHandler.ts`), then stage and commit the updated
file so the InputMessage interface definition and surrounding
whitespace/indentation match project formatting rules before re-pushing the
branch.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f59b7801-64b4-4750-98b8-b52c3fd6cec1

📥 Commits

Reviewing files that changed from the base of the PR and between 200822b and d664346.

📒 Files selected for processing (1)

• src/server/InputHandler.ts