Repository navigation
Conversation
Signed-off-by: Weiwei Zheng <heymrbox@gmail.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesExplicit URL Scheme Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
Hi @heymrbox, the branch Please use one of the following formats:
Allowed types: To fix, rename your branch locally, push the new branch, and open a new PR: git branch -m <new-branch-name>
git push origin -u <new-branch-name> |
|
Closing this PR because the branch name |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| }; | ||
|
|
||
| fn explicit_connection_mode(url: &str) -> Option<ConnectionMode> { | ||
| let normalized_url = url.to_ascii_lowercase(); |
There was a problem hiding this comment.
🟡 Nit: to_ascii_lowercase() makes detection case-insensitive (e.g. GRPC://host is recognized), but the original mixed-case URL is then passed unchanged to try_grpc_reachable / try_http_reachable, which use case-sensitive starts_with("grpc://") / starts_with("https://") internally. For a URL like GRPC://localhost:30001, detection correctly picks gRPC, but try_grpc_reachable falls into its else branch and produces a malformed grpc://GRPC://localhost:30001.
Two options:
- Drop the
to_ascii_lowercase()so detection is case-sensitive like downstream (simplest, mixed-case schemes are rare). - Normalize the URL itself (lowercase the scheme portion) and pass the normalized URL to the probe functions.
Either way, the detection and the downstream consumers should agree on case handling.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces explicit connection mode detection by checking for URL schemes like grpc://, http://, and https:// before falling back to parallel protocol probing. It also includes unit tests for this new logic. Feedback suggests using a dedicated URL parsing library instead of string prefix matching for better robustness and case-sensitivity handling. Additionally, there is a recommendation to simplify the error message formatting to remove redundancy.
| if let Some(connection_mode) = explicit_connection_mode(&url) { | ||
| let result = match connection_mode { | ||
| ConnectionMode::Http => try_http_reachable(&url, timeout, client).await, | ||
| ConnectionMode::Grpc => try_grpc_reachable(&url, timeout).await, | ||
| }; |
There was a problem hiding this comment.
The current implementation is susceptible to case-sensitivity issues because the underlying reachability probes perform case-sensitive prefix matching on the URL. To reliably handle schemes and distinguish between different URL types, parse the URL using a dedicated library and check its scheme instead of using string prefix matching. This approach avoids misclassifying URLs and ensures robustness against variations like 'GRPC://'.
References
- To reliably distinguish data URIs from other URLs, parse the URL and check its scheme instead of using string prefix matching. This avoids misclassifying relative URLs.
| let protocol = match connection_mode { | ||
| ConnectionMode::Http => "HTTP", | ||
| ConnectionMode::Grpc => "gRPC", | ||
| }; | ||
| return Err(WorkflowError::StepFailed { | ||
| step_id: StepId::new("detect_connection_mode"), | ||
| message: format!( | ||
| "{protocol} health check failed for explicit {} worker URL {}: {}", | ||
| connection_mode, config.url, err | ||
| ), | ||
| }); |
There was a problem hiding this comment.
The error message is redundant, repeating "health check failed" and including both the capitalized protocol string and the connection_mode enum. Additionally, the underlying error err from the probe already includes a "Health check failed" prefix. A more concise message would improve log clarity.
| let protocol = match connection_mode { | |
| ConnectionMode::Http => "HTTP", | |
| ConnectionMode::Grpc => "gRPC", | |
| }; | |
| return Err(WorkflowError::StepFailed { | |
| step_id: StepId::new("detect_connection_mode"), | |
| message: format!( | |
| "{protocol} health check failed for explicit {} worker URL {}: {}", | |
| connection_mode, config.url, err | |
| ), | |
| }); | |
| let protocol = match connection_mode { | |
| ConnectionMode::Http => "HTTP", | |
| ConnectionMode::Grpc => "gRPC", | |
| }; | |
| return Err(WorkflowError::StepFailed { | |
| step_id: StepId::new("detect_connection_mode"), | |
| message: format!( | |
| "Explicit {protocol} worker URL {} is unreachable: {err}", | |
| config.url | |
| ), | |
| }); |
|
Hi @heymrbox, the branch Please use one of the following formats:
Allowed types: To fix, rename your branch locally, push the new branch, and open a new PR: git branch -m <new-branch-name>
git push origin -u <new-branch-name> |
1 participant
Summary by CodeRabbit