[Feat]: WebSocket movement throttling and packet handling#101
[Feat]: WebSocket movement throttling and packet handling#101PinJinx wants to merge 3 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR implements client-side backpressure handling and movement throttling: it drops "move" WebSocket messages when the socket's Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Touch Input)
participant TG as useTrackpadGesture
participant RAF as RequestAnimationFrame
participant RC as useRemoteConnection
participant WS as WebSocket
User->>TG: touchmove(dx,dy)
activate TG
TG->>TG: queueMovement(dx,dy)\naccumulate pendingMove\nschedule RAF if needed
deactivate TG
RAF->>TG: animation frame callback
activate TG
TG->>TG: compute consolidated dx/dy\nreset pendingMove
TG->>RC: emit consolidated "move" message
deactivate TG
activate RC
RC->>RC: if WS.bufferedAmount > 10000
alt congested
RC->>RC: drop "move" message
else healthy
RC->>WS: send "move" message
end
deactivate RC
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
🧹 Nitpick comments (1)
src/hooks/useRemoteConnection.ts (1)
70-70: Consider extracting the magic number10000into a named constant.A named constant (e.g.,
const WS_BUFFER_THRESHOLD = 10000) at module scope would clarify intent and make tuning easier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useRemoteConnection.ts` at line 70, Extract the magic number 10000 into a module-level named constant (e.g., WS_BUFFER_THRESHOLD) and replace the literal in useRemoteConnection where you check wsRef.current.bufferedAmount; update the condition in the handler that checks if msg.type == "move" to use WS_BUFFER_THRESHOLD instead of 10000, so the intent is clear and the threshold is configurable/testable.
🤖 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/hooks/useRemoteConnection.ts`:
- Around line 69-70: Replace the non-strict equality in the movement drop check:
in the message handler where msg.type is inspected (the conditional using
msg.type == "move" that also checks wsRef.current.bufferedAmount), change to
strict equality (msg.type === "move") to avoid implicit coercion; update the
conditional that returns early for bufferedAmount > 10000 accordingly.
In `@src/hooks/useTrackpadGesture.ts`:
- Around line 43-60: The batched move rAF can fire after handleTouchEnd and
cause a stale "move" after a "release"; modify queueMovement to store the
requestAnimationFrame id (e.g., rafId.current) when scheduling and make
handleTouchEnd check if a frame is scheduled (frameScheduled.current) and either
cancelAnimationFrame(rafId.current) or synchronously flush/send the
pendingMove.current dx/dy before sending the click press:false event; ensure you
reset pendingMove.current.dx/dy and frameScheduled.current (and clear
rafId.current) so no stale moves are sent after release.
---
Nitpick comments:
In `@src/hooks/useRemoteConnection.ts`:
- Line 70: Extract the magic number 10000 into a module-level named constant
(e.g., WS_BUFFER_THRESHOLD) and replace the literal in useRemoteConnection where
you check wsRef.current.bufferedAmount; update the condition in the handler that
checks if msg.type == "move" to use WS_BUFFER_THRESHOLD instead of 10000, so the
intent is clear and the threshold is configurable/testable.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/hooks/useRemoteConnection.ts (1)
69-70: Consider extracting the backpressure threshold to a named constant.The logic is sound — dropping only
movepackets while letting critical events through is the right approach. The magic number10000would be clearer as a named constant (e.g.,const WS_BACKPRESSURE_THRESHOLD = 10000), but this is a minor readability nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useRemoteConnection.ts` around lines 69 - 70, Extract the magic number 10000 into a named constant (e.g., WS_BACKPRESSURE_THRESHOLD) and replace the inline literal in the backpressure check inside useRemoteConnection where you inspect wsRef.current.bufferedAmount and msg.type === "move"; declare the constant at module scope or near the top of the hook so it’s easy to adjust and document the rationale for the threshold while keeping the existing logic that drops only "move" packets when bufferedAmount exceeds the threshold.src/hooks/useTrackpadGesture.ts (1)
62-104:dx/dyare pre-computed but only used in some branches.Lines 63–64 compute
dxanddy(with sensitivity and rounding) unconditionally, but they're only consumed whendragging(line 66) or single-finger move (line 103). The scroll/pinch branches usesumX/sumYdirectly with their own scaling. Not a bug, but the early computation is slightly misleading — a reader might expect all branches to use the pre-computed values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useTrackpadGesture.ts` around lines 62 - 104, The function processMovement precomputes dx/dy but only uses them in the dragging and single-touch branches, which is misleading; update processMovement to compute dx and dy only where needed (inside the dragging branch and the ongoingTouches.current.length === 1 branch) or rename the precomputed vars to make intent clear, leaving scroll/pinch logic to continue using sumX/sumY with their own scaling; reference processMovement, dx, dy, dragging, queueMovement, and the scroll/pinch branches (pinching, lastPinchDist, send) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/hooks/useTrackpadGesture.ts`:
- Around line 259-268: The flush of any pending batched move (the block that
checks frameScheduled.current, calls cancelAnimationFrame(rafId.current), sends
send({type: 'move', dx: ..., dy: ...}) and resets pendingMove.current) must be
moved to execute before the drag-release / click-handling logic so the server
receives the final move before any release/click events; relocate that entire
flush block to run prior to the code that emits the drag-release ("click
press:false") and tap/click handling so pendingMove.current is flushed and
cleared before sending release/click events.
---
Nitpick comments:
In `@src/hooks/useRemoteConnection.ts`:
- Around line 69-70: Extract the magic number 10000 into a named constant (e.g.,
WS_BACKPRESSURE_THRESHOLD) and replace the inline literal in the backpressure
check inside useRemoteConnection where you inspect wsRef.current.bufferedAmount
and msg.type === "move"; declare the constant at module scope or near the top of
the hook so it’s easy to adjust and document the rationale for the threshold
while keeping the existing logic that drops only "move" packets when
bufferedAmount exceeds the threshold.
In `@src/hooks/useTrackpadGesture.ts`:
- Around line 62-104: The function processMovement precomputes dx/dy but only
uses them in the dragging and single-touch branches, which is misleading; update
processMovement to compute dx and dy only where needed (inside the dragging
branch and the ongoingTouches.current.length === 1 branch) or rename the
precomputed vars to make intent clear, leaving scroll/pinch logic to continue
using sumX/sumY with their own scaling; reference processMovement, dx, dy,
dragging, queueMovement, and the scroll/pinch branches (pinching, lastPinchDist,
send) when making the change.
| const send = useCallback((msg: any) => { | ||
| if (wsRef.current?.readyState === WebSocket.OPEN) { | ||
| //Drop movement when there is a lot of packets left to be processed | ||
| if(msg.type === "move" && wsRef.current.bufferedAmount > 10000) return; |
There was a problem hiding this comment.
10,000 bytes (~10KB) may be arbitrary.
Better: const MAX_WS_BUFFER = 10 * 1024;
more better to understand in future
| const findTouchIndex = (id: number) => ongoingTouches.current.findIndex(t => t.identifier === id); | ||
|
|
||
| //Queue the movement and send it only once an Animationframe | ||
| const queueMovement = (dx:number , dy:number) => { |
There was a problem hiding this comment.
good usage
Before
Touch event → send → WebSocket → network
No throttling
No congestion control
After:
Touch event → accumulate → rAF → send (max 60Hz)
Plus congestion guard
This is:
Client-side rate control + congestion avoidance
That is proper network-aware design.
|
@imxade LGTM If the network is busy, it skips extra movement updates instead of piling them up. |
3 participants
Addressed Issues:
Fixes #86
Screenshots/Recordings:
Nil
Additional Notes:
Checklist
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