Skip to content

Conversation

@cte
Copy link
Collaborator

@cte cte commented Sep 3, 2025

Important

Improved logging in SocketTransport.ts with method-specific references and context, renamed a variable for clarity, and simplified the onReconnect callback signature.

  • Logging Improvements:
    • Updated log messages in SocketTransport.ts to include method references, e.g., [SocketTransport#connect].
    • Added more context to log messages, such as connection attempts and delays.
  • Variable Renaming:
    • Renamed hasConnectedOnce to isPreviouslyConnected in SocketTransport.ts for clarity.
  • Callback Signature Change:
    • Simplified onReconnect callback in SocketTransportOptions by removing attemptNumber parameter.

This description was created by Ellipsis for 6a32376. 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:59
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. Enhancement New feature or request labels Sep 3, 2025
@cte cte merged commit bcb71db into main Sep 3, 2025
16 of 17 checks passed
@cte cte deleted the cte/socket-io-logging branch September 3, 2025 07:59
@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
Copy link
Contributor

@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 your contribution! I've reviewed the changes and found that the logging improvements are excellent. The method-specific prefixes make debugging much easier, and the variable rename to isPreviouslyConnected is more descriptive.

Suggestions for Future Improvements

While this PR has already been merged, here are some observations for consideration in future updates:

  1. Breaking API change: The removal of the attemptNumber parameter from the onReconnect callback (line 10) is a breaking change that could affect consumers who depend on knowing the reconnection attempt number.

  2. Debug visibility: The removed "Waiting Xms before retry..." log message previously provided useful visibility into retry behavior during debugging. Consider adding it back with a debug flag.

  3. Connection state timing: Setting connectionState = CONNECTED before calling the callbacks could lead to race conditions if the callbacks fail. Consider setting it after successful callback execution.

  4. Logging consistency: Line 145 still uses [SocketTransport] instead of [SocketTransport#_connect] for consistency with other log messages in that method.

Overall, great improvements to the logging system!

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

Labels

Enhancement New feature or request size:M This PR changes 30-99 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants