refactor(api): robust websocket URL construction#1560
Conversation
📝 WalkthroughWalkthroughThe ChangesWebSocket URL Construction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
🧹 Nitpick comments (1)
src/api/socket.rs (1)
7-9: ⚡ Quick winAdd trace/debug logging on URL parse fallback path.
Line 7 silently returns the raw input on parse failure; add a grep-friendly debug/trace log (e.g.,
[api]) with error context so WS misconfiguration is diagnosable.As per coding guidelines, "Use
log/tracingatdebug/tracelevel; include stable grep-friendly prefixes ([domain],[rpc])..." and "Add copious debug/trace logs while implementing features..."🤖 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 `@src/api/socket.rs` around lines 7 - 9, Replace the silent parse-fallback with a logged fallback: instead of discarding the parse Err inside the current let Ok(...) else { return ... } construct, capture the parse error (via match or if let Err(e) = Url::parse(http_or_https_base)) and emit a debug/trace log using the project logging facade (log or tracing) with a grep-friendly prefix like "[api]" that includes the raw http_or_https_base and the parse error, then return http_or_https_base.to_string(); reference Url::parse and the http_or_https_base variable when making the change.
🤖 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.
Nitpick comments:
In `@src/api/socket.rs`:
- Around line 7-9: Replace the silent parse-fallback with a logged fallback:
instead of discarding the parse Err inside the current let Ok(...) else { return
... } construct, capture the parse error (via match or if let Err(e) =
Url::parse(http_or_https_base)) and emit a debug/trace log using the project
logging facade (log or tracing) with a grep-friendly prefix like "[api]" that
includes the raw http_or_https_base and the parse error, then return
http_or_https_base.to_string(); reference Url::parse and the http_or_https_base
variable when making the change.
|
looks great merging shortly dude... |
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Approve with minor nits. Clean, well-scoped refactor — url::Url parsing is a clear improvement over manual string manipulation. All existing tests pass. A few minor observations inline below. None are blocking.
| } else { | ||
| base.to_string() | ||
| let Ok(mut url) = Url::parse(http_or_https_base) else { | ||
| return http_or_https_base.to_string(); |
There was a problem hiding this comment.
[minor] Silent fallback on parse failure — CodeRabbit already flagged this, so just +1: a debug!("[api] websocket_url parse failed for {}: {}", http_or_https_base, e) before returning would make misconfiguration diagnosable per project logging guidelines.
| "http" => "ws", | ||
| other => other, | ||
| } | ||
| .to_string(); |
There was a problem hiding this comment.
[minor] set_scheme error discarded — Url::set_scheme returns Err(()) if the new scheme is invalid or the URL has an opaque path. In practice "ws"/"wss" are always valid for a parsed HTTP(S) URL, so this is safe — but a trace! on failure would be defensive:
if url.set_scheme(&scheme).is_err() {
trace!("[api] websocket_url: failed to set scheme to {scheme} for {http_or_https_base}");
}| let scheme = match url.scheme() { | ||
| "https" => "wss", | ||
| "http" => "ws", | ||
| other => other, |
There was a problem hiding this comment.
[nit] Unnecessary allocation — .to_string() allocates a String just for set_scheme which takes &str. You can keep it as &str:
let scheme = match url.scheme() {
"https" => "wss",
"http" => "ws",
other => other,
};No functional difference, just avoids a small alloc.
| }; | ||
| format!("{}/socket.io/?EIO=4&transport=websocket", ws_base) | ||
|
|
||
| let scheme = match url.scheme() { |
There was a problem hiding this comment.
[minor] Consider an idempotency test — if someone passes an already-websocket URL (wss://...), the other => other branch keeps it as-is, which is good. Worth adding a test to lock that in:
#[test]
fn handles_wss_input_directly() {
let url = websocket_url("wss://api.tinyhumans.ai");
assert_eq!(url, "wss://api.tinyhumans.ai/socket.io/?EIO=4&transport=websocket");
}3 participants
Refactors the
websocket_urlconstruction logic to use theurlcrate instead of manual string manipulation. Currently, the implementation relies onreplacenandtrim_end_matches, which can lead to fragile URL building if the input base URL has unexpected formatting or multiple trailing slashes.Transitioning to a proper URL parser ensures that protocol switching (HTTP/HTTPS to WS/WSS) is handled robustly and that path segments are normalized correctly. This change enhances the reliability of the background WebSocket loop when connecting to the TinyHumans backend.
Verified the new implementation against existing unit tests. All tests for protocol conversion and trailing slash normalization passed successfully.
Summary by CodeRabbit