feat: websocket support through /ws #154
Open
+259
−0
Conversation
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
WalkthroughAdds WebSocket support: new Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@server/websocket.go`:
- Around line 36-61: handleWebSocket currently upgrades the HTTP connection but
does not limit inbound frame sizes or idle time; immediately after conn :=
newUpgrader(enableCORS).Upgrade(...) returns, call conn.SetReadLimit with a
sensible maxMessageSize (e.g., 1MB or a configurable constant), set read
deadlines via conn.SetReadDeadline and install a pong handler with
conn.SetPongHandler to extend deadlines on activity; update
newUpgrader/handleWebSocket to use these methods on the returned websocket.Conn
(referencing handleWebSocket, newUpgrader, conn, SetReadLimit, SetReadDeadline,
SetPongHandler, and wsConnection) so oversized frames and idle connections are
bounded.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@server/websocket.go`:
- Around line 108-123: In readMessages, the return value from
wsConnection.sendError is ignored; modify the code to capture the error returned
by sendError (call on wsConn.sendError) and log it (e.g., via utils.Verbose or a
logger) so failures to send error frames are observable; ensure you still
continue the loop after logging so behavior is unchanged but errors are recorded
for wsConnection/sendError failures.
- Around line 194-209: The function handleWSMessage should check and handle the
return values from wsConn.sendError calls (called in the json.Unmarshal error
branch and the validateJSONRPCRequest error branch) instead of ignoring
them—capture the error return and log/handle it appropriately; also avoid
logging potentially sensitive request payloads with utils.Info: change the log
to utils.Verbose (or call a redact function on req.Params before logging) so
JSONRPCRequest params are not emitted at Info level; keep the rest of the flow
(validateJSONRPCRequest, handleWSMethodCall) intact.
- Around line 211-227: The calls to wsConn.sendError and wsConn.sendResponse in
handleWSMethodCall ignore returned errors and static analysis flags these;
update handleWSMethodCall to capture and log any error returned from
wsConn.sendError (both the "Method not found" and server error paths) and from
wsConn.sendResponse after successful execution, e.g., call the methods, check
the error value, and log it with context (mentioning req.Method and req.ID) so
failures in sendError/sendResponse are observable; locate these calls inside
handleWSMethodCall and add error handling/logging around wsConn.sendError and
wsConn.sendResponse.
🧹 Nitpick comments (2)
server/websocket.go (2)
125-142: Consider handlingconn.Close()error for completeness.Static analysis flags the unchecked
conn.Close()error on line 133. While ignoring close errors in defers is common practice, logging can help with debugging connection issues.♻️ Optional fix
- defer conn.Close() + defer func() { + if err := conn.Close(); err != nil { + utils.Verbose("error closing WebSocket connection: %v", err) + } + }()
251-255: Consider setting write deadline insendJSON.Unlike
pingLoop, thesendJSONmethod doesn't set a write deadline before writing. If the peer becomes unresponsive during a response write, this could block indefinitely.♻️ Suggested improvement
func (wsc *wsConnection) sendJSON(v interface{}) error { wsc.writeMu.Lock() defer wsc.writeMu.Unlock() + if err := wsc.conn.SetWriteDeadline(time.Now().Add(wsWriteWait)); err != nil { + return err + } return wsc.conn.WriteJSON(v) }
Comment on lines
+108
to
+123
| func readMessages(wsConn *wsConnection) { | ||
| for { | ||
| messageType, message, err := wsConn.conn.ReadMessage() | ||
| if err != nil { | ||
| utils.Verbose("WebSocket connection closed: %v", err) | ||
| break | ||
| } | ||
|
|
||
| if messageType != websocket.TextMessage { | ||
| wsConn.sendError(nil, ErrCodeInvalidRequest, errTitleInvalidReq, errMsgTextOnly) | ||
| continue | ||
| } | ||
|
|
||
| handleWSMessage(wsConn, message) | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle or acknowledge the error return from sendError.
The static analysis flags that sendError return value is unchecked on line 117. While the loop continuing regardless is reasonable behavior, the error should be logged for observability.
🔧 Suggested fix
if messageType != websocket.TextMessage {
- wsConn.sendError(nil, ErrCodeInvalidRequest, errTitleInvalidReq, errMsgTextOnly)
+ if err := wsConn.sendError(nil, ErrCodeInvalidRequest, errTitleInvalidReq, errMsgTextOnly); err != nil {
+ utils.Verbose("failed to send error response: %v", err)
+ }
continue
}📝 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.
Suggested change
| func readMessages(wsConn *wsConnection) { | |
| for { | |
| messageType, message, err := wsConn.conn.ReadMessage() | |
| if err != nil { | |
| utils.Verbose("WebSocket connection closed: %v", err) | |
| break | |
| } | |
| if messageType != websocket.TextMessage { | |
| wsConn.sendError(nil, ErrCodeInvalidRequest, errTitleInvalidReq, errMsgTextOnly) | |
| continue | |
| } | |
| handleWSMessage(wsConn, message) | |
| } | |
| } | |
| func readMessages(wsConn *wsConnection) { | |
| for { | |
| messageType, message, err := wsConn.conn.ReadMessage() | |
| if err != nil { | |
| utils.Verbose("WebSocket connection closed: %v", err) | |
| break | |
| } | |
| if messageType != websocket.TextMessage { | |
| if err := wsConn.sendError(nil, ErrCodeInvalidRequest, errTitleInvalidReq, errMsgTextOnly); err != nil { | |
| utils.Verbose("failed to send error response: %v", err) | |
| } | |
| continue | |
| } | |
| handleWSMessage(wsConn, message) | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: lint
[failure] 117-117:
Error return value of wsConn.sendError is not checked (errcheck)
🤖 Prompt for AI Agents
In `@server/websocket.go` around lines 108 - 123, In readMessages, the return
value from wsConnection.sendError is ignored; modify the code to capture the
error returned by sendError (call on wsConn.sendError) and log it (e.g., via
utils.Verbose or a logger) so failures to send error frames are observable;
ensure you still continue the loop after logging so behavior is unchanged but
errors are recorded for wsConnection/sendError failures.
Comment on lines
+194
to
+209
| func handleWSMessage(wsConn *wsConnection, message []byte) { | ||
| var req JSONRPCRequest | ||
| if err := json.Unmarshal(message, &req); err != nil { | ||
| wsConn.sendError(nil, ErrCodeParseError, errTitleParseError, errMsgParseError) | ||
| return | ||
| } | ||
|
|
||
| if validationErr := validateJSONRPCRequest(req); validationErr != nil { | ||
| wsConn.sendError(req.ID, validationErr.code, validationErr.message, validationErr.data) | ||
| return | ||
| } | ||
|
|
||
| utils.Info("WebSocket Request ID: %v, Method: %s, Params: %s", req.ID, req.Method, string(req.Params)) | ||
|
|
||
| handleWSMethodCall(wsConn, req) | ||
| } |
There was a problem hiding this comment.
Address unchecked error returns and potential sensitive data logging.
- Static analysis flags unchecked
sendErrorreturns on lines 197 and 202. - Line 206 logs
req.Paramsat Info level, which may contain sensitive user data. Consider usingVerboselevel or redacting params.
🔧 Suggested fix
func handleWSMessage(wsConn *wsConnection, message []byte) {
var req JSONRPCRequest
if err := json.Unmarshal(message, &req); err != nil {
- wsConn.sendError(nil, ErrCodeParseError, errTitleParseError, errMsgParseError)
+ if sendErr := wsConn.sendError(nil, ErrCodeParseError, errTitleParseError, errMsgParseError); sendErr != nil {
+ utils.Verbose("failed to send parse error: %v", sendErr)
+ }
return
}
if validationErr := validateJSONRPCRequest(req); validationErr != nil {
- wsConn.sendError(req.ID, validationErr.code, validationErr.message, validationErr.data)
+ if sendErr := wsConn.sendError(req.ID, validationErr.code, validationErr.message, validationErr.data); sendErr != nil {
+ utils.Verbose("failed to send validation error: %v", sendErr)
+ }
return
}
- utils.Info("WebSocket Request ID: %v, Method: %s, Params: %s", req.ID, req.Method, string(req.Params))
+ utils.Verbose("WebSocket Request ID: %v, Method: %s, Params: %s", req.ID, req.Method, string(req.Params))
handleWSMethodCall(wsConn, req)
}📝 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.
Suggested change
| func handleWSMessage(wsConn *wsConnection, message []byte) { | |
| var req JSONRPCRequest | |
| if err := json.Unmarshal(message, &req); err != nil { | |
| wsConn.sendError(nil, ErrCodeParseError, errTitleParseError, errMsgParseError) | |
| return | |
| } | |
| if validationErr := validateJSONRPCRequest(req); validationErr != nil { | |
| wsConn.sendError(req.ID, validationErr.code, validationErr.message, validationErr.data) | |
| return | |
| } | |
| utils.Info("WebSocket Request ID: %v, Method: %s, Params: %s", req.ID, req.Method, string(req.Params)) | |
| handleWSMethodCall(wsConn, req) | |
| } | |
| func handleWSMessage(wsConn *wsConnection, message []byte) { | |
| var req JSONRPCRequest | |
| if err := json.Unmarshal(message, &req); err != nil { | |
| if sendErr := wsConn.sendError(nil, ErrCodeParseError, errTitleParseError, errMsgParseError); sendErr != nil { | |
| utils.Verbose("failed to send parse error: %v", sendErr) | |
| } | |
| return | |
| } | |
| if validationErr := validateJSONRPCRequest(req); validationErr != nil { | |
| if sendErr := wsConn.sendError(req.ID, validationErr.code, validationErr.message, validationErr.data); sendErr != nil { | |
| utils.Verbose("failed to send validation error: %v", sendErr) | |
| } | |
| return | |
| } | |
| utils.Verbose("WebSocket Request ID: %v, Method: %s, Params: %s", req.ID, req.Method, string(req.Params)) | |
| handleWSMethodCall(wsConn, req) | |
| } |
🧰 Tools
🪛 GitHub Check: lint
[failure] 202-202:
Error return value of wsConn.sendError is not checked (errcheck)
[failure] 197-197:
Error return value of wsConn.sendError is not checked (errcheck)
🤖 Prompt for AI Agents
In `@server/websocket.go` around lines 194 - 209, The function handleWSMessage
should check and handle the return values from wsConn.sendError calls (called in
the json.Unmarshal error branch and the validateJSONRPCRequest error branch)
instead of ignoring them—capture the error return and log/handle it
appropriately; also avoid logging potentially sensitive request payloads with
utils.Info: change the log to utils.Verbose (or call a redact function on
req.Params before logging) so JSONRPCRequest params are not emitted at Info
level; keep the rest of the flow (validateJSONRPCRequest, handleWSMethodCall)
intact.
Comment on lines
+211
to
+227
| func handleWSMethodCall(wsConn *wsConnection, req JSONRPCRequest) { | ||
| registry := GetMethodRegistry() | ||
| handler, exists := registry[req.Method] | ||
| if !exists { | ||
| wsConn.sendError(req.ID, ErrCodeMethodNotFound, "Method not found", req.Method+" not found") | ||
| return | ||
| } | ||
|
|
||
| result, err := handler(req.Params) | ||
| if err != nil { | ||
| log.Printf("Error executing method %s: %v", req.Method, err) | ||
| wsConn.sendError(req.ID, ErrCodeServerError, "Server error", err.Error()) | ||
| return | ||
| } | ||
|
|
||
| wsConn.sendResponse(req.ID, result) | ||
| } |
There was a problem hiding this comment.
Handle error returns from sendError and sendResponse.
Static analysis flags unchecked errors on lines 215, 222, and 226. These should be logged for observability.
🔧 Suggested fix
func handleWSMethodCall(wsConn *wsConnection, req JSONRPCRequest) {
registry := GetMethodRegistry()
handler, exists := registry[req.Method]
if !exists {
- wsConn.sendError(req.ID, ErrCodeMethodNotFound, "Method not found", req.Method+" not found")
+ if err := wsConn.sendError(req.ID, ErrCodeMethodNotFound, "Method not found", req.Method+" not found"); err != nil {
+ utils.Verbose("failed to send method not found error: %v", err)
+ }
return
}
result, err := handler(req.Params)
if err != nil {
log.Printf("Error executing method %s: %v", req.Method, err)
- wsConn.sendError(req.ID, ErrCodeServerError, "Server error", err.Error())
+ if sendErr := wsConn.sendError(req.ID, ErrCodeServerError, "Server error", err.Error()); sendErr != nil {
+ utils.Verbose("failed to send server error: %v", sendErr)
+ }
return
}
- wsConn.sendResponse(req.ID, result)
+ if err := wsConn.sendResponse(req.ID, result); err != nil {
+ utils.Verbose("failed to send response: %v", err)
+ }
}🧰 Tools
🪛 GitHub Check: lint
[failure] 226-226:
Error return value of wsConn.sendResponse is not checked (errcheck)
[failure] 222-222:
Error return value of wsConn.sendError is not checked (errcheck)
[failure] 215-215:
Error return value of wsConn.sendError is not checked (errcheck)
🤖 Prompt for AI Agents
In `@server/websocket.go` around lines 211 - 227, The calls to wsConn.sendError
and wsConn.sendResponse in handleWSMethodCall ignore returned errors and static
analysis flags these; update handleWSMethodCall to capture and log any error
returned from wsConn.sendError (both the "Method not found" and server error
paths) and from wsConn.sendResponse after successful execution, e.g., call the
methods, check the error value, and log it with context (mentioning req.Method
and req.ID) so failures in sendError/sendResponse are observable; locate these
calls inside handleWSMethodCall and add error handling/logging around
wsConn.sendError and wsConn.sendResponse.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.