Add connection retry for web lib #1
Conversation
WalkthroughThe changes update two files. In Changes
Sequence Diagram(s)sequenceDiagram
participant TW as TurboWire
participant WS as WebSocket Server
TW->>WS: establishConnection()
Note right of TW: Set onMessage & onError callbacks
WS-->>TW: onopen (connection established)
TW->>TW: Reset retryCount & assign messageCallback
WS-->>TW: onmessage (handle incoming message)
WS-->>TW: onclose (connection closed)
alt Unclean disconnect & retryCount < maxRetries
TW->>TW: Increment retryCount and schedule reconnect
TW->>WS: Re-attempt connection
else Clean disconnect or max retries reached
TW->>TW: Invoke disconnect logic, clear retry timeout
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/web/src/index.ts (3)
28-29: Constructor defaults for retry logic.Providing default values of
maxRetries = 10andretryInterval = 3000is sensible. Consider mentioning this default behavior in the README or doc comments so users know what to expect if they omit these fields.
42-43: Storing callbacks in private properties.This is a clean approach for reusability in different event handlers. However, if the user calls
connect()multiple times without a precedingdisconnect(), it might cause overlapping WebSocket connections. Documenting or guarding against this scenario could make the API more robust.
88-98: Reconnection logic on unclean disconnections.The retry approach is straightforward. However, consider introducing exponential backoff or randomized jitter to avoid tight reconnection loops under severe network instability. This helps reduce server load and collisions from repeated identical intervals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/web/package.json(1 hunks)packages/web/src/index.ts(5 hunks)
🔇 Additional comments (13)
packages/web/src/index.ts (10)
3-4: Optional fields added to TurboWireOptions.These new fields are consistent with the intended retry logic. Consider adding a doc comment explaining the default values to ensure clarity for users reading the type definition.
11-12: Private callbacks for message and error.Defining separate callbacks is a straightforward way to handle different event types. The naming is clear and consistent with the rest of the class.
45-45: Initial WebSocket establishment.Calling
this.establishConnection()at connect time centralizes your connection logic. This is a good pattern for clarity and maintainability.
48-48: Refactored method for connection flow.Extracting the connection flow into
establishConnection()keepsconnect()concise and easy to read.
50-50: Verbose logging of connection attempts.Having a log statement here is useful in debug mode; it helps developers pinpoint connection attempts when analyzing logs.
59-59: Invoking error callback on WebSocket error.Delegating to
this.errorCallbackis a clean way to let higher layers handle errors without polluting core logic. This separation of concerns is good practice.
66-66: Resetting retry count on successful connection.This ensures subsequent disconnections start with a clean slate for retry logic. This is a sensible approach.
73-73: Invoking message callback for incoming messages.Forwarding the raw data to
messageCallbackis a standard pattern for event-driven design. It keeps the receiving mechanism agnostic to any message parsing.
121-124: Clearing the retry timeout and resetting the retry count on disconnect.This ensures no reconnection attempts occur after a deliberate disconnect. The logic neatly prevents lingering timeouts.
Also applies to: 134-134
14-17:❓ Verification inconclusive
New private fields for retry logic.
Defining these fields is essential for reconnection behavior. However, ensure you handle invalid user input (e.g., negative
maxRetriesorretryInterval). An explicit check with a reasonable fallback could prevent edge cases.Would you like to confirm if any external code passes invalid values for these properties? We can do that by scanning the codebase for calls into the constructor:
🏁 Script executed:
#!/bin/bash # Searching all calls of the TurboWire constructor to check for unexpected input values ast-grep --pattern $'new TurboWire($_, {maxRetries: $_, retryInterval: $_})'Length of output: 77
Action Required: Validate External Inputs for TurboWire Constructor
The new private fields for retry logic are correctly introduced for reconnection behavior. However, as noted, it is important to ensure that invalid input (such as negative values for
maxRetriesorretryInterval) is properly handled. Our automated search using the providedast-grepcommand did not yield any conclusive results for calls to theTurboWireconstructor with these parameters. Therefore, please manually verify whether any external code passes invalid values to these properties. If such cases exist, consider adding an explicit check or a reasonable fallback in the constructor to mitigate potential edge cases.
- File:
packages/web/src/index.ts- Concern: Validate inputs for
maxRetriesandretryIntervalpassed toTurboWire.packages/web/package.json (3)
13-13: Consolidating the "files" array into a single entry.This is a minor formatting change. Having the array in one line is acceptable and doesn’t introduce any functional issues.
16-17: Added "prepublishOnly" script.Running
"npm run build"automatically before publishing is a best practice to ensure consumers receive the latest compiled code. Good addition!
19-22:✅ Verification successful
Removed @types/node from devDependencies.
Before removing, confirm that no Node-specific global definitions (e.g.,
process,Buffer, or__dirname) are in use. If they are, the code might break without the Node type definitions.Here’s a script to scan the codebase for references to Node-specific globals or imports:
🏁 Script executed:
#!/bin/bash # Searching references to fs, path, process, or other node built-in modules rg -i "require('fs')|from 'fs'|require('path')|from 'path'|process\\.|Buffer\\.|__dirname|__filename"Length of output: 715
Subject: Verified Safe Removal of @types/node for packages/web
- The grep search confirms that no Node-specific globals (e.g.,
process,Buffer,__dirname) are referenced in thepackages/webdirectory.- Node-specific globals were only found in
packages/serverless(e.g., usage ofprocess.env), which is separate from the web package.- Therefore, removing
@types/nodefrompackages/web/package.jsonis safe as it does not impact the web codebase.
Summary by CodeRabbit
New Features
Chores