fix: removed client-side sensitivity double-application#54
fix: removed client-side sensitivity double-application#54Rozerxshashank wants to merge 3 commits intoAOSSIE-Org:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughClient-side trackpad no longer applies sensitivity/inversion/axis-threshold; it sends raw gesture deltas. Server InputHandler now reads CONFIG (sensitivity/invert), applies scaling/sign, rounds values, and guards zero amounts before performing scroll/zoom/move actions. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server
participant Config
rect rgba(100,150,240,0.5)
Client->>Client: capture gesture (sumX, sumY, delta)
end
Client->>Server: send raw gesture message (sumX, sumY, delta)
Server->>Config: read CONFIG (MOUSE_SENSITIVITY, MOUSE_INVERT)
Config-->>Server: sensitivity, invert flag
Server->>Server: compute invertMultiplier, scaled = raw * sensitivity * invertMultiplier
Server->>Server: round/scalarize values
alt amount != 0
Server->>Server: perform move / scroll / zoom action
end
Server-->>Client: (optional) ack/state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/server/InputHandler.ts`:
- Around line 47-52: The switch cases for 'scroll' (where invertMultiplier and
scrollSensitivity are declared and mouse.scrollDown/mouse.scrollRight are
called) and the 'zoom' case must be wrapped in their own block braces to prevent
const-scope leakage across cases; edit the case 'scroll' and case 'zoom' bodies
to enclose their existing statements in { ... } so invertMultiplier,
scrollSensitivity and any local vars in the 'zoom' case are block-scoped to that
case.
- Around line 50-51: The scroll amounts passed to mouse.scrollDown and
mouse.scrollRight (in InputHandler.ts) can be non-integers because
scrollSensitivity may be a float, causing native truncation; wrap the computed
values (msg.dy * invertMultiplier * scrollSensitivity and msg.dx * -1 *
invertMultiplier * scrollSensitivity) with Math.round() before awaiting
mouse.scrollDown and mouse.scrollRight so the nut-js functions receive integer
ticks (use msg.dy/msg.dx checks as-is).
🧹 Nitpick comments (1)
src/server/InputHandler.ts (1)
27-27:CONFIG.MOUSE_SENSITIVITY ?? 1.0is repeated three times; the fallback is already inconfig.tsx.
CONFIG.MOUSE_SENSITIVITYalready defaults to1.0via the nullish coalescing insrc/config.tsx(line 20). The?? 1.0here is redundant. Consider reading it once at the top ofhandleMessage(or as a class field) to keep it DRY:async handleMessage(msg: InputMessage) { + const sensitivity = CONFIG.MOUSE_SENSITIVITY; switch (msg.type) {Then reuse
sensitivityin themove,scroll, andzoombranches.Also applies to: 49-49, 57-57
|
Hey @imxade |
Rozerxshashank
force-pushed
the
fix/sensitivity-bug
branch
from
765cbd8 to
c0b424b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/trackpad.tsx (1)
40-66:⚠️ Potential issue | 🔴 CriticalStale closures:
handleKeyDown,handleInput, andhandleCompositionEndwill read stalemodifierstate.
handleKeyDown(line 40) capturesmodifierand callshandleModifier, but its dependency array is[send]. When the user toggles modifier state, this callback won't see the update — key presses will be routed incorrectly (e.g., keys sent as normal input when modifier is "Active", or vice versa). The same issue affectshandleInput(line 104) andhandleCompositionEnd(line 121).The root fix: wrap
handleModifierinuseCallback(with its own correct deps), then include bothmodifierandhandleModifierin the dependency arrays of all three hooks.Proposed fix (sketch)
-const handleModifier = (key: string) => { +const handleModifier = useCallback((key: string) => { if (modifier === "Hold") { const comboKeys = [...buffer, key]; sendCombo(comboKeys); return; } else if (modifier === "Active") { setBuffer(prev => [...prev, key]); return; } -}; +}, [modifier, buffer, sendCombo]); - const handleKeyDown = useCallback((e: React.KeyboardEvent<HTMLInputElement>) => { + const handleKeyDown = useCallback((e: React.KeyboardEvent<HTMLInputElement>) => { // ... same body ... - }, [send]); + }, [send, modifier, handleModifier]); - const handleInput = useCallback((e: React.ChangeEvent<HTMLInputElement>) => { + const handleInput = useCallback((e: React.ChangeEvent<HTMLInputElement>) => { // ... same body ... - }, [sendText]); + }, [sendText, modifier, handleModifier]); - const handleCompositionEnd = useCallback((e: React.CompositionEvent<HTMLInputElement>) => { + const handleCompositionEnd = useCallback((e: React.CompositionEvent<HTMLInputElement>) => { // ... same body ... - }, [sendText]); + }, [sendText, modifier, handleModifier]);
🤖 Fix all issues with AI agents
In `@src/routes/trackpad.tsx`:
- Around line 185-187: The onBlur handler currently always refocuses
hiddenInputRef after INPUT_REFOCUS_DELAY_MS which can steal focus from other
controls; change the setTimeout callback to check document.activeElement and
only call hiddenInputRef.current?.focus() when the new activeElement is the
document.body or not inside the trackpad container (use a trackpad container ref
such as trackpadContainerRef.current) — ensure you guard against null refs and
only refocus when the activeElement is the body or outside the container (or
equals the container) to avoid trapping focus.
- Around line 27-28: The inline comment above the useTrackpadGesture call is
stale: it still reads "Pass sensitivity and invertScroll to the gesture hook"
even though the call now is const { isTracking, handlers } =
useTrackpadGesture(send, scrollMode); Update or remove that comment to reflect
the current usage (e.g., "Pass send and scrollMode to the gesture hook" or
remove the line entirely), locating the string near the useTrackpadGesture
invocation to keep docs accurate.
In `@src/server/InputHandler.ts`:
- Line 27: The file references CONFIG (e.g., the sensitivity assignment in
InputHandler.ts and handlers for move/scroll/zoom) but CONFIG isn't imported and
the existing config export doesn't include MOUSE_SENSITIVITY/MOUSE_INVERT; add
or import the correct configuration values and provide safe defaults: either (A)
update the config module to export MOUSE_SENSITIVITY and MOUSE_INVERT and import
CONFIG (or the named exports) into InputHandler, or (B) replace
CONFIG.MOUSE_SENSITIVITY/CONFIG.MOUSE_INVERT uses with locally read values that
fallback to 1.0 and false respectively (e.g., const sensitivity =
process.env.MOUSE_SENSITIVITY ? Number(...) : 1.0). Ensure all occurrences in
move/scroll/zoom handlers reference the defined variables and include proper
type/coercion to avoid runtime ReferenceError and NaN.
- Around line 29-31: The mouse move handler in InputHandler (the call to
mouse.setPosition with new Point) currently passes floating-point coordinates,
causing inconsistency with scroll/zoom which round values; update the code that
computes the new Point for mouse.setPosition to wrap both X and Y in Math.round
(i.e., Math.round(currentPos.x + msg.dx * sensitivity) and
Math.round(currentPos.y + msg.dy * sensitivity)) so the move path matches
scroll/zoom rounding behavior and avoid subtle coordinate differences.
🧹 Nitpick comments (2)
src/server/InputHandler.ts (1)
73-74:breakis outside the block scope for thezoomcase.The
breakon line 74 sits after the closing}of the block on line 73. While syntactically valid, this is inconsistent with thescrollcase (wherebreakis inside the block) and easy to misread. Move it inside the block for consistency.Proposed fix
case 'zoom': { if (msg.delta !== undefined && msg.delta !== 0) { ... } - } break; + break; + }src/routes/trackpad.tsx (1)
68-86:handleModifierStateis not memoized — will create a new reference every render, causingControlBarto re-render unnecessarily.Since it's passed to
ControlBarviaonModifierToggle, wrapping it inuseCallbackwould be consistent with the other handlers in this file.
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/routes/trackpad.tsx (1)
39-65:⚠️ Potential issue | 🔴 CriticalStale closures:
handleKeyDown,handleInput, andhandleCompositionEndwill read outdatedmodifierstate.
handleKeyDowncapturesmodifierand callshandleModifier, but the dependency array only lists[send]. Whenmodifierchanges (e.g., user activates "Hold" mode), subsequent keystrokes will still see the oldmodifiervalue. The same problem affectshandleInput(Line 103) andhandleCompositionEnd(Line 120). This is confirmed by the Biome linter.The root fix is to either:
- Add
modifierand a memoizedhandleModifierto each dependency array, or- Convert
modifierreads to use a ref so callbacks don't need to re-create.Option 1 (minimal fix) — wrap
handleModifierinuseCallbackand fix dep arrays:Proposed fix
-const handleModifier = (key: string) => { +const handleModifier = useCallback((key: string) => { if (modifier === "Hold") { const comboKeys = [...buffer, key]; sendCombo(comboKeys); return; } else if (modifier === "Active") { setBuffer(prev => [...prev, key]); return; } -}; +}, [modifier, buffer, sendCombo]); ... const handleKeyDown = useCallback((e: React.KeyboardEvent<HTMLInputElement>) => { ... - }, [send]); + }, [send, modifier, handleModifier]); const handleInput = useCallback((e: React.ChangeEvent<HTMLInputElement>) => { ... - }, [sendText]); + }, [sendText, modifier, handleModifier]); const handleCompositionEnd = useCallback((e: React.CompositionEvent<HTMLInputElement>) => { ... - }, [sendText]); + }, [sendText, modifier, handleModifier]);
🧹 Nitpick comments (2)
src/routes/trackpad.tsx (2)
67-96:handleModifierStateandhandleModifierare plain functions — inconsistent with theuseCallbackpattern used elsewhere.These two functions close over
modifier,buffer,sendCombo, and state setters but are not memoized. They are passed to child components (e.g.,onModifierToggle={handleModifierState}) and called from memoized callbacks, which undermines the benefit ofuseCallbackon the callers and causes referential instability for child props.Wrapping both in
useCallbackwith correct deps would complete the memoization story and also make the stale-closure fix above cleaner.
167-174: Inline arrow insendKeyprop defeats memoization ofExtraKeys.A new closure is created every render. If
ExtraKeysis wrapped inReact.memo, this negates it. Consider extracting to auseCallback.
Rozerxshashank
force-pushed
the
fix/sensitivity-bug
branch
from
258be5d to
2ee918d
Compare
…e sensitivity to server-side config
1 participant
Fixes: #52
This PR fixes a bug where mouse sensitivity was being applied twice for cursor movement (client hardcoded + server config), while being ignored completely for scroll and zoom events.
Changes:
Removed the sensitivity parameter and its hardcoded 1.5 default. The client now sends raw touch deltas.
Updated
InputHandler.tsto apply CONFIG.MOUSE_SENSITIVITY to Scroll and Zoom events. Previously, it only applied sensitivity to mouse movement.How Has This Been Tested?
Manual Verification: Verified that removing the client-side multiplier correctly delegates sensitivity logic to the server.
Build Check: Ran npm run build to ensure no type errors or build regressions.
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
Refactor
Bug Fix
Improvement