fix(torii): graphql playground URL #2707
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2707 +/- ##
=======================================
Coverage ? 56.26%
=======================================
Files ? 410
Lines ? 52848
Branches ? 0
=======================================
Hits ? 29735
Misses ? 23113
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
@Larkooo I guess we also want to use the external URL in the playground?
@steebchen, since there is this external-url, we need to provide it to torii.
WalkthroughOhayo, sensei! The recent changes focus on refining the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
Client->>Server: Request GraphQL Endpoint
Server-->>Client: Return GraphQL URL
Client->>Server: Request WebSocket Endpoint
Server-->>Client: Return WebSocket URL
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/graphql/src/server.rs(1 hunks)
🔇 Additional comments (2)
crates/torii/graphql/src/server.rs (2)
63-65: LGTM! Default paths follow conventions.
The fallback paths "graphql" and "graphql/ws" align with common GraphQL server conventions.
71-72: Ohayo! Verify the playground endpoints are working.
The implementation correctly uses the constructed endpoints for the GraphiQL playground.
Let's verify the GraphQL playground is accessible:
✅ Verification successful
The previous result shows only one occurrence of the WebSocket endpoint, but let's verify the complete endpoint configuration and any WebSocket-related setup to ensure there are no conflicts or misconfigurations.
I need to verify how these endpoints are constructed and ensure they're properly configured in the GraphiQL setup before making a final determination.
I need to see the complete logic for how both endpoints are constructed, including the fallback case when external_url is None, to make a final determination about the playground configuration.
Ohayo! The playground endpoints are properly configured, sensei!
The implementation correctly handles both scenarios:
- For external URLs: Properly transforms HTTP/HTTPS URLs to their WS/WSS counterparts for subscriptions
- For local setup: Uses "graphql" for queries and "graphql/ws" for subscriptions
The endpoint construction is robust and matches the GraphiQL playground configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the GraphQL playground endpoint is properly configured
# Test: Search for any hardcoded GraphQL endpoints that might conflict
rg -g '!target/' -g '*.rs' 'graphql/ws|ws/graphql' .
Length of output: 149
Script:
#!/bin/bash
# Let's check the full context of the GraphQL endpoint configuration
rg -g '!target/' -g '*.rs' -B 5 -A 5 'endpoint\(' .
# Also check for any WebSocket related configurations
rg -g '!target/' -g '*.rs' 'WebSocket|ws_config' .
Length of output: 5821
Script:
#!/bin/bash
# Let's check how the endpoints are constructed
rg -g '!target/' -g '*.rs' -B 10 -A 3 'let.*graphql_endpoint|let.*subscription_endpoint' .
# And verify the GraphiQL source configuration
ast-grep --pattern 'GraphiQLSource::build()$$$'
Length of output: 1113
Script:
#!/bin/bash
# Let's see the complete endpoint construction logic
rg -g '!target/' -g '*.rs' -C 20 'let \(graphql_endpoint, subscription_endpoint\)' .
Length of output: 3009
| let (graphql_endpoint, subscription_endpoint) = if let Some(external_url) = external_url { | ||
| let mut graphql_url = external_url; | ||
| graphql_url.set_path("graphql"); | ||
|
|
||
| let websocket_scheme = match websocket_url.scheme() { | ||
| "http" => "ws", | ||
| let mut websocket_url = graphql_url.clone(); | ||
| websocket_url.set_path("ws"); | ||
| let _ = websocket_url.set_scheme(match websocket_url.scheme() { | ||
| "https" => "wss", | ||
| _ => panic!("Invalid URL scheme"), // URL validated on input so this never hits | ||
| }; | ||
| "http" => "ws", | ||
| _ => panic!("Invalid URL scheme - must be http or https"), | ||
| }); |
There was a problem hiding this comment.
Ohayo sensei! Replace panic with proper error handling.
The URL scheme handling currently uses panic! which could crash the server in production. Consider returning a Result instead.
Here's a suggested improvement:
- let (graphql_endpoint, subscription_endpoint) = if let Some(external_url) = external_url {
- let mut graphql_url = external_url;
- graphql_url.set_path("graphql");
-
- let mut websocket_url = graphql_url.clone();
- websocket_url.set_path("ws");
- let _ = websocket_url.set_scheme(match websocket_url.scheme() {
- "https" => "wss",
- "http" => "ws",
- _ => panic!("Invalid URL scheme - must be http or https"),
- });
+ let (graphql_endpoint, subscription_endpoint) = if let Some(external_url) = external_url {
+ let mut graphql_url = external_url;
+ graphql_url.set_path("graphql");
+
+ let mut websocket_url = graphql_url.clone();
+ websocket_url.set_path("ws");
+ let ws_scheme = match websocket_url.scheme() {
+ "https" => "wss",
+ "http" => "ws",
+ scheme => {
+ log::error!("Invalid URL scheme '{}' - must be http or https", scheme);
+ return warp::reply::with_status(
+ "Invalid URL scheme",
+ warp::http::StatusCode::INTERNAL_SERVER_ERROR,
+ );
+ }
+ };
+ websocket_url.set_scheme(ws_scheme).expect("valid scheme");Committable suggestion skipped: line range outside the PR's diff.
|
@Larkooo if sounds good to you we could move on and integrate with slot. 👍 |
Summary by CodeRabbit
New Features
Bug Fixes