fix(client): normalize WebSocket URI path to prevent double suffix (#737)#941
Open
Ramreddy2748 wants to merge 1 commit into
Open
fix(client): normalize WebSocket URI path to prevent double suffix (#737)#941Ramreddy2748 wants to merge 1 commit into
Ramreddy2748 wants to merge 1 commit into
Conversation
…ocketride-org#737) _get_websocket_uri previously concatenated /task/service unconditionally, producing double-slash or a duplicated path segment when the caller passed a URI with a trailing slash or an already-correct path. Now strips trailing slashes then appends the suffix only when absent, and extracts the suffix string into a module-level constant.
Ramreddy2748
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
Contributor
📝 WalkthroughWalkthrough
ChangesConnection Lifecycle Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
No description provided. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/client-python/src/rocketride/mixins/connection.py`:
- Around line 386-424: The method set_connection_params can leave
self._manual_disconnect True if _internal_connect() or _schedule_reconnect()
raises; wrap the post-teardown reconnect logic (the block after setting
self._transport = None that calls self._schedule_reconnect() or
self._internal_connect()) in a try/finally so that self._manual_disconnect is
always reset to False in the finally clause; ensure the try covers both the
persist and non-persist branches (i.e., the await self._schedule_reconnect() and
await self._internal_connect() calls) and does not swallow exceptions (re-raise
after finally).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef0540ef-bea0-42da-be2d-5a9ad6db967d
📒 Files selected for processing (1)
packages/client-python/src/rocketride/mixins/connection.py
Comment on lines
+386
to
+424
| async def set_connection_params( | ||
| self, | ||
| uri: Optional[str] = None, | ||
| auth: Optional[str] = None, | ||
| ) -> None: | ||
| """ | ||
| Update server URI and/or auth. If currently connected, disconnects and | ||
| reconnects with the new params. In persist mode, reconnection is scheduled | ||
| only if we were connected (no auto-connect when params are set on a never-connected client). | ||
| In non-persist mode, reconnects only if we were connected. | ||
| """ | ||
| # --- Update params, tear down existing connection/transport, then reconnect (or schedule) only if appropriate --- | ||
| if uri is not None: | ||
| self._set_uri(uri) | ||
| if auth is not None: | ||
| self._set_auth(auth) | ||
|
|
||
| # Remember whether we were connected so we know to disconnect and whether to reconnect at the end | ||
| was_already_connected = self.is_connected() | ||
|
|
||
| # Prevent on_disconnected (from the disconnect below) from scheduling a reconnect during teardown | ||
| self._manual_disconnect = True | ||
| if self._reconnect_task and not self._reconnect_task.done(): | ||
| self._reconnect_task.cancel() | ||
| self._reconnect_task = None | ||
| if was_already_connected: | ||
| await self._internal_disconnect() | ||
|
|
||
| # Drop the transport so the next connect() builds a new one with the new uri/auth | ||
| self._transport = None | ||
| if self._persist and was_already_connected: | ||
| # Schedule a single reconnect attempt (after backoff); only if we were connected (no auto-connect on param set) | ||
| await self._schedule_reconnect() | ||
| elif was_already_connected: | ||
| # Non-persist: only reconnect if we had been connected (same as connect() semantics) | ||
| await self._internal_connect() | ||
|
|
||
| # We're done; clear so future disconnects (e.g. drop) can trigger reconnect if persist | ||
| self._manual_disconnect = False |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Exception handling leaves _manual_disconnect in inconsistent state.
If _internal_connect() on line 421 raises, line 424 never executes and _manual_disconnect remains True. While this only affects non-persist mode (where auto-reconnect doesn't apply), it could cause confusion during debugging or if persist mode is enabled later on the same instance.
Consider wrapping in try/finally:
♻️ Suggested fix
elif was_already_connected:
# Non-persist: only reconnect if we had been connected (same as connect() semantics)
- await self._internal_connect()
-
- # We're done; clear so future disconnects (e.g. drop) can trigger reconnect if persist
- self._manual_disconnect = False
+ try:
+ await self._internal_connect()
+ finally:
+ # We're done; clear so future disconnects (e.g. drop) can trigger reconnect if persist
+ self._manual_disconnect = False
+ else:
+ self._manual_disconnect = False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client-python/src/rocketride/mixins/connection.py` around lines 386
- 424, The method set_connection_params can leave self._manual_disconnect True
if _internal_connect() or _schedule_reconnect() raises; wrap the post-teardown
reconnect logic (the block after setting self._transport = None that calls
self._schedule_reconnect() or self._internal_connect()) in a try/finally so that
self._manual_disconnect is always reset to False in the finally clause; ensure
the try covers both the persist and non-persist branches (i.e., the await
self._schedule_reconnect() and await self._internal_connect() calls) and does
not swallow exceptions (re-raise after finally).
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Problem
_get_websocket_uriconcatenated/task/serviceunconditionally. Two failure modes:https://localhost:5565/producedws://localhost:5565//task/service(double-slash, rejected by the server).https://localhost:5565/task/serviceproducedws://localhost:5565/task/service/task/service(duplicated path, connection hung).Fix
_TASK_SERVICE_SUFFIXto avoid magic strings.Test plan
client.connect("localhost:5565")→ws://localhost:5565/task/service✓client.connect("localhost:5565/")→ws://localhost:5565/task/service✓ (was double-slash)client.connect("localhost:5565/task/service")→ws://localhost:5565/task/service✓ (was duplicated path)client.connect("https://cloud.rocketride.ai")→wss://cloud.rocketride.ai/task/service✓Closes #737
🤖 Generated with Claude Code
Summary by CodeRabbit
Breaking Changes
attach()/detach()/login()/logout()methods removed; useconnect()/disconnect()insteadis_attached(),is_authenticated(),get_account_info()New Features
set_connection_params()to update connection credentials without full reconnection