Add websocket session support #24
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant TCPServer
Client->>Server: Send request
Server->>TCPServer: Setup TCP server
TCPServer-->>Server: Server ready
Server->>TCPServer: Accept connection
TCPServer->>Server: Connection established
Server->>TCPServer: Process incoming data
TCPServer->>Server: Data received
Server->>Client: Send response
Warning Rate limit exceeded@esterlus has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
esterlus
force-pushed
the
este/session
branch
from
f0189c4 to
55ccdc9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/index.ts (4)
177-192: Ensure Proper Error Handling and Resource Management in TCP ServerThe
setupServerfunction initializes the TCP server and sets up event listeners for'error'and'close'events. Currently, on a server error, the error is logged, but no further action is taken. Consider whether the server should attempt to restart or shut down gracefully upon encountering an error to maintain service availability and prevent resource leaks.
217-219: Adjust Log Level for 'end' Event in Socket ConnectionIn the
conn.on('end', ...)event handler, the unexpected end of an incoming socket connection is logged as an error. If the'end'event is a normal occurrence in the socket lifecycle, consider logging it as a warning or informational message instead of an error.
703-711: Remove Deprecated or Commented-Out CodeThere is a block of commented-out code related to reporting to the discovery platform within the
respondfunction. Removing commented-out code can improve code readability and maintainability. If this code is no longer needed, consider deleting it.Apply this diff to remove the commented-out code:
-// if (ops.discoveryPlatform) { -// reportToDiscoveryPlatform({ -// ops, -// cacheEntry, -// reqPayload, -// segments, -// }); -// }
180-182: Handle Server Errors to Enhance ReliabilityIn the
server.on('error', ...)event handler, the error is logged, but the server is not closed or restarted. Depending on the application's requirements, consider implementing logic to gracefully handle server errors, such as shutting down the server or attempting a restart, to improve reliability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
package.json(2 hunks)src/index.ts(8 hunks)
🔇 Additional comments (3)
package.json (2)
34-34: Consider the risks of using a beta dependency
Moving from a stable version (^3.8.0) to a beta version (^3.9.0-beta.1) of @hoprnet/uhttp-lib in production code carries risks:
- Beta versions may contain bugs or incomplete features
- Breaking changes might be introduced before the final release
- Future updates might be less predictable
Please ensure that:
- This beta version has been thoroughly tested
- There's a compelling reason to use the beta version instead of waiting for a stable release
3-3: Verify breaking changes documentation for v4.0.0
The major version bump (3.x.x → 4.0.0-a.1) correctly indicates breaking changes, which aligns with the new TCP server capabilities being added. However, ensure that all breaking changes are properly documented.
src/index.ts (1)
915-915:
Use Validated 'tcpPort' in Server Configuration
After adding validation for the TCP port, ensure that the validated tcpPort variable is used in the server configuration to prevent potential issues with invalid port numbers.
Apply this diff to use the validated tcpPort:
start({
privateKey: process.env.UHTTP_EA_PRIVATE_KEY,
publicKey: process.env.UHTTP_EA_PUBLIC_KEY,
apiEndpoint: new URL(process.env.UHTTP_EA_HOPRD_ENDPOINT),
accessToken: process.env.UHTTP_EA_HOPRD_ACCESS_TOKEN,
discoveryPlatform,
dbFile: process.env.UHTTP_EA_DATABASE_FILE,
pinnedFetch: globalThis.fetch.bind(globalThis),
- tcpPort: parseInt(process.env.UHTTP_EA_TCPPORT),
+ tcpPort,
});Likely invalid or redundant comment.
| throw new Error("Missing 'UHTTP_EA_TCPPORT' env var."); | ||
| } |
There was a problem hiding this comment.
Validate 'UHTTP_EA_TCPPORT' Environment Variable
When reading the UHTTP_EA_TCPPORT environment variable, ensure that it is not only present but also a valid integer within the acceptable TCP port range (1-65535). Invalid port numbers could cause the server to fail unexpectedly.
Apply this diff to add validation:
if (!process.env.UHTTP_EA_TCPPORT) {
throw new Error("Missing 'UHTTP_EA_TCPPORT' env var.");
}
+const tcpPort = parseInt(process.env.UHTTP_EA_TCPPORT, 10);
+if (isNaN(tcpPort) || tcpPort < 1 || tcpPort > 65535) {
+ throw new Error("Invalid 'UHTTP_EA_TCPPORT' env var. It must be a valid port number (1-65535).");
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw new Error("Missing 'UHTTP_EA_TCPPORT' env var."); | |
| } | |
| if (!process.env.UHTTP_EA_TCPPORT) { | |
| throw new Error("Missing 'UHTTP_EA_TCPPORT' env var."); | |
| } | |
| const tcpPort = parseInt(process.env.UHTTP_EA_TCPPORT, 10); | |
| if (isNaN(tcpPort) || tcpPort < 1 || tcpPort > 65535) { | |
| throw new Error("Invalid 'UHTTP_EA_TCPPORT' env var. It must be a valid port number (1-65535)."); | |
| } |
Summary by CodeRabbit
New Features
Bug Fixes
Version Updates
@hoprnet/uhttp-libupdated to a beta version.