Open
Conversation
Contributor
📝 WalkthroughWalkthroughInvert-scroll parsing on the client was simplified, a hardcoded address was removed from server config, and server input handling was adjusted to round movement/scroll/zoom deltas and only enqueue non-zero actions with safer promise handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (3 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 |
![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)
src/server/InputHandler.ts (1)
50-76:⚠️ Potential issue | 🟡 MinorWrap
scrollcase body in a block to avoid switch-clause declaration leak.The
const promisesdeclaration on line 51 is accessible to subsequentcaseclauses due to JavaScript's switch fall-through scoping rules. This is flagged by Biome'snoSwitchDeclarationslint rule.Also, prefer
Promise<void>[]overany[]to retain type safety.Proposed fix
- case 'scroll': - const promises: any[] = []; + case 'scroll': { + const promises: Promise<void>[] = []; // ... existing scroll logic ... - break; + break; + }🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/server/InputHandler.ts` around lines 50 - 76, Wrap the entire `case 'scroll':` body in its own block to avoid leaking the `const promises` declaration into other switch cases and change its type from `any[]` to `Promise<void>[]`; specifically, enclose the current logic that declares `const promises` and calls `mouse.scrollDown/scrollUp/scrollRight/scrollLeft` in `{ ... }`, update `const promises: any[]` to `const promises: Promise<void>[]`, and keep the `await Promise.all(promises)` call inside that block so the variable scope is limited to the `'scroll'` case.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/routes/trackpad.tsx`:
- Around line 24-28: processMovement in useTrackpadGesture.ts is still
multiplying the client-side sensitivity into all deltas (move/zoom/scroll),
causing double-application; remove any multiplication by the sensitivity
variable inside processMovement and from the move/zoom/scroll handlers so the
raw delta values are sent to the server, while leaving the sensitivity state
(the sensitivity useState in trackpad.tsx) intact for server-side
use/round-trip; ensure processMovement(), and the move/zoom/scroll branches in
useTrackpadGesture.ts no longer modify deltas by sensitivity so only the server
applies the sensitivity value.
In `@src/server/InputHandler.ts`:
- Around line 78-101: Extract the hardcoded sensitivityFactor in the 'zoom' case
into a named constant (e.g., ZOOM_SENSITIVITY) alongside MAX_ZOOM_STEP to avoid
the magic number; update the code in InputHandler's switch 'zoom' block to use
this constant instead of the inline 0.5 so it's discoverable and configurable,
and ensure any related comments or tests reference ZOOM_SENSITIVITY where
appropriate.
- Around line 50-76: Wrap the entire `case 'scroll':` body in its own block to
avoid leaking the `const promises` declaration into other switch cases and
change its type from `any[]` to `Promise<void>[]`; specifically, enclose the
current logic that declares `const promises` and calls
`mouse.scrollDown/scrollUp/scrollRight/scrollLeft` in `{ ... }`, update `const
promises: any[]` to `const promises: Promise<void>[]`, and keep the `await
Promise.all(promises)` call inside that block so the variable scope is limited
to the `'scroll'` case.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/server/InputHandler.ts`: - Around line 78-101: Extract the hardcoded sensitivityFactor in the 'zoom' case into a named constant (e.g., ZOOM_SENSITIVITY) alongside MAX_ZOOM_STEP to avoid the magic number; update the code in InputHandler's switch 'zoom' block to use this constant instead of the inline 0.5 so it's discoverable and configurable, and ensure any related comments or tests reference ZOOM_SENSITIVITY where appropriate.src/server/InputHandler.ts (1)
78-101: Zoom implementation looks solid — good use oftry/finallyfor key release.The
try/finallyensuresLeftControlis always released even if the scroll action throws. Rounding and clamping (MAX_ZOOM_STEP) protect against erratic input. Theamount !== 0guard avoids unnecessary key presses.One minor note: the hardcoded
sensitivityFactor = 0.5on line 80 is a magic number. Consider extracting it as a named constant alongsideMAX_ZOOM_STEPfor discoverability.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/server/InputHandler.ts` around lines 78 - 101, Extract the hardcoded sensitivityFactor in the 'zoom' case into a named constant (e.g., ZOOM_SENSITIVITY) alongside MAX_ZOOM_STEP to avoid the magic number; update the code in InputHandler's switch 'zoom' block to use this constant instead of the inline 0.5 so it's discoverable and configurable, and ensure any related comments or tests reference ZOOM_SENSITIVITY where appropriate.
Contributor
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/InputHandler.ts`:
- Around line 57-59: Replace the empty `.then(() => { })` handlers used when
pushing the promises in InputHandler (the calls to mouse.scrollDown and
mouse.scrollUp) with a clearer discard of the resolved value; either use
`.then(() => undefined)` or cast the returned Promise to `Promise<void>` (e.g.,
`as unknown as Promise<void>`) so the intent to coerce to Promise<void> is
explicit and the code reads clearly.
- Around line 54-61: Compute and reuse the rounded delta values instead of
calling Math.round twice: in InputHandler where you currently check typeof
msg.dy and Math.round(msg.dy) !== 0, first compute const amountY =
Math.round(msg.dy) and use that in the guard and subsequent logic to push
mouse.scrollDown(amountY) / mouse.scrollUp(-amountY); do the same for msg.dx
(compute const amountX = Math.round(msg.dx) and reuse it) so both the dx and dy
branches avoid duplicate Math.round calls and reuse the precomputed amount
variables.
- Line 81: The hardcoded "const sensitivityFactor = 0.5" in InputHandler
silently halves zoom input; replace it with a configurable value by either
deriving it from the client's existing sensitivity setting or moving it to a
named config/constant (e.g., ZOOM_SENSITIVITY_FACTOR) and exposing it in the
client settings. Update the code in the InputHandler method that computes zoom
(the place where sensitivityFactor is used) to read the value from the client's
settings or the new config constant, and add validation/fallback (e.g., default
to 0.5) so behavior stays safe if the config is missing.
🧹 Nitpick comments (3)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts`: - Around line 57-59: Replace the empty `.then(() => { })` handlers used when pushing the promises in InputHandler (the calls to mouse.scrollDown and mouse.scrollUp) with a clearer discard of the resolved value; either use `.then(() => undefined)` or cast the returned Promise to `Promise<void>` (e.g., `as unknown as Promise<void>`) so the intent to coerce to Promise<void> is explicit and the code reads clearly. - Around line 54-61: Compute and reuse the rounded delta values instead of calling Math.round twice: in InputHandler where you currently check typeof msg.dy and Math.round(msg.dy) !== 0, first compute const amountY = Math.round(msg.dy) and use that in the guard and subsequent logic to push mouse.scrollDown(amountY) / mouse.scrollUp(-amountY); do the same for msg.dx (compute const amountX = Math.round(msg.dx) and reuse it) so both the dx and dy branches avoid duplicate Math.round calls and reuse the precomputed amount variables. - Line 81: The hardcoded "const sensitivityFactor = 0.5" in InputHandler silently halves zoom input; replace it with a configurable value by either deriving it from the client's existing sensitivity setting or moving it to a named config/constant (e.g., ZOOM_SENSITIVITY_FACTOR) and exposing it in the client settings. Update the code in the InputHandler method that computes zoom (the place where sensitivityFactor is used) to read the value from the client's settings or the new config constant, and add validation/fallback (e.g., default to 0.5) so behavior stays safe if the config is missing.src/server/InputHandler.ts (3)
57-59: The.then(() => { })calls are unnecessary if the only goal isPromise<void>coercion.If
mouse.scrollDownreturnsPromise<Mouse>, a cleaner way to discard the resolved value is.then(() => undefined)or casting viaas unknown as Promise<void>. The empty-block.then(() => { })also silently converts the resolve value but reads like a forgotten handler.Not a bug — just a readability nit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 57 - 59, Replace the empty `.then(() => { })` handlers used when pushing the promises in InputHandler (the calls to mouse.scrollDown and mouse.scrollUp) with a clearer discard of the resolved value; either use `.then(() => undefined)` or cast the returned Promise to `Promise<void>` (e.g., `as unknown as Promise<void>`) so the intent to coerce to Promise<void> is explicit and the code reads clearly.
54-61:Math.round(msg.dy)is computed twice — once in the guard and once foramount.Minor: compute
amountfirst and reuse it in the guard.♻️ Suggested refactor
- if (typeof msg.dy === 'number' && Math.round(msg.dy) !== 0) { - const amount = Math.round(msg.dy); + const vAmount = typeof msg.dy === 'number' ? Math.round(msg.dy) : 0; + if (vAmount !== 0) { + const amount = vAmount;Same pattern applies to
msg.dxon lines 64-65.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 54 - 61, Compute and reuse the rounded delta values instead of calling Math.round twice: in InputHandler where you currently check typeof msg.dy and Math.round(msg.dy) !== 0, first compute const amountY = Math.round(msg.dy) and use that in the guard and subsequent logic to push mouse.scrollDown(amountY) / mouse.scrollUp(-amountY); do the same for msg.dx (compute const amountX = Math.round(msg.dx) and reuse it) so both the dx and dy branches avoid duplicate Math.round calls and reuse the precomputed amount variables.
81-81: HardcodedsensitivityFactor = 0.5for zoom — consider making it configurable.Issue
#52's objective is to eliminate hidden multipliers so the user's sensitivity setting directly controls speed. While this is zoom-specific dampening (not cursor sensitivity), it's still a magic number that silently halves the user's gesture input. Consider either deriving it from the client's sensitivity setting or extracting it to a named constant/config so it's discoverable and tunable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` at line 81, The hardcoded "const sensitivityFactor = 0.5" in InputHandler silently halves zoom input; replace it with a configurable value by either deriving it from the client's existing sensitivity setting or moving it to a named config/constant (e.g., ZOOM_SENSITIVITY_FACTOR) and exposing it in the client settings. Update the code in the InputHandler method that computes zoom (the place where sensitivityFactor is used) to read the value from the client's settings or the new config constant, and add validation/fallback (e.g., default to 0.5) so behavior stays safe if the config is missing.
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.
Fixes: #52
This PR resolves the "double-sensitivity" bug by consolidating sensitivity and inversion logic entirely on the client-side. It also fixes a server-side precision issue where fractional movements from the client were being ignored or incorrectly scaled on Windows.
Changes:
Client-Side Move: Moved mouseSensitivity and mouseInvert from server-config.json to client-side localStorage.
This prevents the server from interfering with user-specific gesture preferences.
Input Precision: Updated InputHandler.ts to use Math.round() for all physical movements, scrolls, and zooms.
This ensures that scaled deltas from the client are correctly translated into discrete integer steps required by the OS.
Zoom Direction Fix: Fixed a bug where zooming was only working in one direction; it now correctly uses both scrollUp and scrollDown .
Upstream Alignment: Built from a fresh sync with upstream/main to ensure full compatibility with the latest processMovement architecture.
How Has This Been Tested?
**Manual Verification: ** Verified on Windows that mouse movement, scrolling, and zooming are smooth and respect both high and very low sensitivity settings.
Build Check:
Ran npm run build to verify that all TypeScript type errors (especially in InputHandler.ts ) are resolved.
Checklist
My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
Summary by CodeRabbit