Socket cleanup#181
Merged
Merged
Conversation
Removed logger.info on every received message touchToken now throttled to once/sec
Contributor
WalkthroughRemoved the exported CONFIG and direct FRONTEND_PORT wiring; added configurable inputThrottleMs in server-config.json and propagated it through websocket initialization into InputHandler to replace hardcoded 8ms throttling, with validation for updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebSocketServer as WebSocket
participant InputHandler
participant TokenStore
Note over WebSocket,InputHandler: Server reads `server-config.json` (inputThrottleMs) at startup
Client->>WebSocket: send input message (move/scroll)
WebSocket->>InputHandler: forward parsed input
InputHandler-->>InputHandler: apply throttle (inputThrottleMs)
alt allowed by throttle
InputHandler->>TokenStore: touchToken (may be rate-limited)
TokenStore-->>InputHandler: ack
InputHandler-->>WebSocket: send response/ack
WebSocket-->>Client: optional response
else throttled
InputHandler-->>WebSocket: drop or defer input
WebSocket-->>Client: optional throttle-notice
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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)
63-75:⚠️ Potential issue | 🟠 MajorThrottle timers should wait the remaining window, not the full window.
Current logic can delay pending events by an extra
throttleMs(especially noticeable wheninputThrottleMsis increased).Proposed fix
if (msg.type === "move") { const now = Date.now() - if (now - this.lastMoveTime < this.throttleMs) { + const elapsed = now - this.lastMoveTime + if (elapsed < this.throttleMs) { this.pendingMove = msg if (!this.moveTimer) { + const waitMs = this.throttleMs - elapsed this.moveTimer = setTimeout(() => { this.moveTimer = null if (this.pendingMove) { const pending = this.pendingMove this.pendingMove = null this.handleMessage(pending).catch((err) => { console.error("Error processing pending move event:", err) }) } - }, this.throttleMs) + }, waitMs) } return } this.lastMoveTime = now } else if (msg.type === "scroll") { const now = Date.now() - if (now - this.lastScrollTime < this.throttleMs) { + const elapsed = now - this.lastScrollTime + if (elapsed < this.throttleMs) { this.pendingScroll = msg if (!this.scrollTimer) { + const waitMs = this.throttleMs - elapsed this.scrollTimer = setTimeout(() => { this.scrollTimer = null if (this.pendingScroll) { const pending = this.pendingScroll this.pendingScroll = null this.handleMessage(pending).catch((err) => { console.error("Error processing pending move event:", err) }) } - }, this.throttleMs) + }, waitMs) } return }Also applies to: 82-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 63 - 75, The throttle timer currently always waits the full this.throttleMs which can add extra delay; change the setTimeout delay in the InputHandler logic that uses lastMoveTime/pendingMove/moveTimer to compute the remaining window as const delay = Math.max(0, this.throttleMs - (Date.now() - this.lastMoveTime)) and use that delay when scheduling the pending handler (and apply the same fix to the other analogous block at 82-94). Ensure you preserve clearing moveTimer, resetting pendingMove, and calling this.handleMessage(pending).catch(...) exactly as before but use the computed remaining delay instead of this.throttleMs.
🤖 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/routes/settings.tsx`:
- Line 5: The route is importing serverConfig from "../server-config.json" which
bundles all server-side keys into client code; remove that import from
src/routes/settings.tsx and replace usage with a frontend-safe surface (e.g.,
create a new module like frontend-config.ts that only exports allowed fields
such as frontendPort or expose the value via a server loader API). Ensure
server-config.json remains read only on the server (e.g., in loader or server
utilities) and reference the new frontend-config export in SettingsRoute (or
whichever component/function in settings.tsx uses serverConfig) so no sensitive
keys are shipped to the browser.
In `@src/server/websocket.ts`:
- Line 50: The startup-only use of inputThrottleMs (used when constructing new
InputHandler) means runtime config updates aren’t applied; change the
config-update handling to propagate new values to the live components instead of
only persisting them. Locate where InputHandler is instantiated (new
InputHandler(inputThrottleMs)) and in your update-config path call a method on
that instance to update the throttle (e.g., add or use an existing
setThrottle/updateThrottle on InputHandler) or replace the instance atomically
with a new one, and likewise update the duplicate-window variables on the
running subsystem(s) (the same places referenced around lines 122-123 and
232-250) so the new duplicate-window and inputThrottleMs values take effect
immediately at runtime.
- Around line 36-42: The JSON parsing of server-config.json is unguarded and
will throw on malformed files; update the code around configPath/serverConfig
(where fs.existsSync, fs.readFileSync and JSON.parse are used) to wrap the
read+parse in a try/catch, log or processLogger.error the parse error, and on
error fall back to an empty object (or safe defaults) so WebSocket
initialization continues; ensure serverConfig remains a Record<string, unknown>
after the fallback.
---
Outside diff comments:
In `@src/server/InputHandler.ts`:
- Around line 63-75: The throttle timer currently always waits the full
this.throttleMs which can add extra delay; change the setTimeout delay in the
InputHandler logic that uses lastMoveTime/pendingMove/moveTimer to compute the
remaining window as const delay = Math.max(0, this.throttleMs - (Date.now() -
this.lastMoveTime)) and use that delay when scheduling the pending handler (and
apply the same fix to the other analogous block at 82-94). Ensure you preserve
clearing moveTimer, resetting pendingMove, and calling
this.handleMessage(pending).catch(...) exactly as before but use the computed
remaining delay instead of this.throttleMs.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/config.tsxsrc/routes/settings.tsxsrc/server-config.jsonsrc/server/InputHandler.tssrc/server/websocket.ts
💤 Files with no reviewable changes (1)
- src/config.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/server/websocket.ts (1)
208-255:⚠️ Potential issue | 🟠 Major
inputThrottleMsupdates are persisted but not applied live.
update-configsaves and validates the new value, but active behavior still uses startup-initialized instances/values (inputHandlerand duplicate-window logic), so the change is not effective until restart.Suggested fix
- const inputHandler = new InputHandler(inputThrottleMs) + let inputHandler = new InputHandler(inputThrottleMs) @@ - const DUPLICATE_WINDOW_MS = inputThrottleMs @@ - if (raw === lastRaw && now - lastTime < DUPLICATE_WINDOW_MS) { + if (raw === lastRaw && now - lastTime < inputThrottleMs) { return } @@ } else if (key === "inputThrottleMs") { const ms = Number(msg.config[key]) @@ filtered[key] = ms + inputThrottleMs = ms + inputHandler = new InputHandler(inputThrottleMs) } else if (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 208 - 255, The update-config branch correctly validates and persists inputThrottleMs but doesn't propagate it to running components; after you persist the new config in the handler that processes the "update-config" message in websocket.ts, apply the value to the live runtime: call the runtime API on the input handler (e.g., inputHandler.setThrottle or inputHandler.updateOptions) or, if the implementation uses a recreated instance, recreate/replace inputHandler with the same constructor/factory using the new ms; also update any duplicate-window logic/timers that rely on the startup value so they read the new inputThrottleMs (e.g., updateDuplicateWindowThreshold or reset related timers) to ensure the change takes effect immediately without restart.
🤖 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/websocket.ts`:
- Around line 48-52: The startup assignment for inputThrottleMs accepts any
positive number but should match the update-time validation (integer between 1
and 1000); change the initialization of inputThrottleMs (using
serverConfig.inputThrottleMs) to validate
Number.isInteger(serverConfig.inputThrottleMs) && serverConfig.inputThrottleMs
>= 1 && serverConfig.inputThrottleMs <= 1000, and fall back to the existing
default (8) if validation fails so runtime behavior matches the update-config
constraints.
---
Duplicate comments:
In `@src/server/websocket.ts`:
- Around line 208-255: The update-config branch correctly validates and persists
inputThrottleMs but doesn't propagate it to running components; after you
persist the new config in the handler that processes the "update-config" message
in websocket.ts, apply the value to the live runtime: call the runtime API on
the input handler (e.g., inputHandler.setThrottle or inputHandler.updateOptions)
or, if the implementation uses a recreated instance, recreate/replace
inputHandler with the same constructor/factory using the new ms; also update any
duplicate-window logic/timers that rely on the startup value so they read the
new inputThrottleMs (e.g., updateDuplicateWindowThreshold or reset related
timers) to ensure the change takes effect immediately without restart.
…m throttle validation
26 tasks
This was referenced Mar 8, 2026
This was referenced Mar 17, 2026
5 tasks
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.
Declaration: Changes are made by claude as per my instructions
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.
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit
New Features
Bug Fixes
Chores