Skip to content

Conversation

@cte
Copy link
Collaborator

@cte cte commented Sep 3, 2025

Important

Improves error logging and listener cleanup in SocketTransport class.

  • Error Handling:
    • Improved error logging for auth_error event in SocketTransport to include error message or string representation.
  • Cleanup:
    • Added this.socket.io.removeAllListeners() in disconnect() method of SocketTransport to ensure all listeners are removed.

This description was created by Ellipsis for 1b4831b. You can customize this summary. It will automatically update as commits are pushed.

@cte cte requested review from jr and mrubens as code owners September 3, 2025 07:41
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Sep 3, 2025
@cte cte merged commit aa4144e into main Sep 3, 2025
10 checks passed
@cte cte deleted the cte/more-socket-io-fixes branch September 3, 2025 07:41
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Sep 3, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Sep 3, 2025
@dosubot dosubot bot added the bug Something isn't working label Sep 3, 2025
Copy link

@roomote roomote bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for these improvements to the SocketTransport error handling and cleanup! The changes look good - the improved error logging for auth_error events and the addition of this.socket.io.removeAllListeners() in the disconnect method are solid improvements.

Post-merge suggestions for future enhancements:

  1. Potential error handling issue (line 232): After improving the error logging, there's still a potential issue where error.message might be undefined when creating the Error object. Consider using: reject(new Error(error?.message || String(error) || "Authentication failed")) to be more defensive against various error object shapes.

  2. Missing test coverage: The SocketTransport class currently has no test files. Given that this handles critical connection logic with retries, authentication, and reconnection, it would benefit from comprehensive unit tests to ensure reliability and prevent regressions.

  3. Consistent error handling pattern: The error handling pattern introduced (error instanceof Error ? error.message : String(error)) is excellent! Could we apply this same pattern consistently throughout the file? I noticed similar error logging on lines 147, 180, 188, and 212 that could benefit from the same treatment.

These are suggestions for future improvements - the current changes are a step in the right direction!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants