feat: migrate playground #1811
feat: migrate playground #1811
Conversation
amitksingh1490
added
the
type: feature
WalkthroughWalkthroughThe primary change across the codebase involves removing the Changes
Assessment against linked issues
The code changes effectively address the objectives outlined in the linked issue, focusing on migrating away from the GraphiQL interface towards a new playground URL approach. The systematic removal of Recent Review DetailsConfiguration used: CodeRabbit UI Files ignored due to path filters (24)
Files selected for processing (52)
Files skipped from review as they are similar to previous changes (50)
Additional Context UsedGitHub Check Runs (1)
Path-based Instructions (1)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1811 +/- ##
=======================================
Coverage 86.89% 86.90%
=======================================
Files 156 156
Lines 15836 15840 +4
=======================================
+ Hits 13761 13765 +4
Misses 2075 2075 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
src/http/mod.rs
Outdated
| @@ -15,7 +15,7 @@ pub use data_loader::*; | |||
| pub use data_loader_request::*; | |||
| pub use method::Method; | |||
| pub use request_context::RequestContext; | |||
| pub use request_handler::{graphiql, handle_request, API_URL_PREFIX}; | |||
| pub use request_handler::{handle_request, API_URL_PREFIX}; | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Several files in the codebase still use the graphiql function. This contradicts the PR's intention to remove it from the public API. The following files need attention:
src/config/server.rssrc/cli/server/mod.rssrc/cli/server/server_config.rssrc/blueprint/server.rs
Analysis chain
Removal of graphiql from public API aligns with the migration to an external playground.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Ensure that there are no remaining references to the `graphiql` function in the codebase.
# Search for any references to the `graphiql` function across all `.rs` files.
rg --glob '*.rs' 'graphiql' --files-with-matches
Length of output: 147
src/http/request_handler.rs
Outdated
| use async_graphql::ServerError; | ||
| use hyper::header::{self, CONTENT_TYPE}; | ||
| use hyper::header::{self}; |
There was a problem hiding this comment.
Refactoring to focus on direct handling of GraphQL requests is well-implemented.
Would you like me to generate unit tests to ensure the new request handling logic works as expected?
amitksingh1490
force-pushed
the
fix/1790
branch
from
4a54047 to
444a01d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
src/http/request_handler.rs (1)
Line range hint
56-85: Thegraphql_requestfunction is robust, handling errors appropriately and using Arc for shared state in asynchronous tasks. The changes align well with the PR's objectives to streamline GraphQL handling.Would you like me to generate unit tests to ensure the new request handling logic works as expected?
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (3)
README.md (2)
Line range hint
100-100: Consider hyphenating "first time contributor" to "first-time contributor" for grammatical correctness.
Line range hint
114-114: Add alt text to the image for better accessibility.src/http/request_handler.rs (1)
Line range hint
46-46: Handling of REST API requests is well-implemented, integrating seamlessly with the application context. Consider adding more detailed logging for better traceability.
|
| Report | Thu, May 2, 2024 at 16:44:40 UTC |
| Project | tailcall |
| Branch | 1811/merge |
| Testbed | benchmarking-runner |
Click to view all benchmark results
| Benchmark | Latency | Latency Results nanoseconds (ns) | (Δ%) | Latency Upper Boundary nanoseconds (ns) | (%) |
|---|---|---|---|
| group_by | ✅ (view plot) | 600.76 (-1.29%) | 634.74 (94.65%) |
| input/args.missing | ✅ (view plot) | 25.48 (+0.68%) | 26.18 (97.33%) |
| input/args.nested.existing | ✅ (view plot) | 55.90 (-0.62%) | 58.24 (95.98%) |
| input/args.nested.missing | ✅ (view plot) | 38.11 (-2.42%) | 39.95 (95.40%) |
| input/args.root | ✅ (view plot) | 52.79 (+0.02%) | 54.50 (96.86%) |
| input/headers.existing | ✅ (view plot) | 31.67 (+0.42%) | 31.84 (99.45%) |
| input/headers.missing | ✅ (view plot) | 30.05 (-1.25%) | 31.19 (96.34%) |
| input/value.missing | ✅ (view plot) | 23.42 (-0.63%) | 24.99 (93.75%) |
| input/value.nested.existing | ✅ (view plot) | 42.47 (+3.59%) | 42.79 (99.25%) |
| input/value.nested.missing | ✅ (view plot) | 36.71 (+2.61%) | 37.35 (98.30%) |
| input/value.root | ✅ (view plot) | 38.80 (+0.37%) | 39.87 (97.33%) |
| input/vars.existing | ✅ (view plot) | 8.39 (-0.61%) | 8.86 (94.70%) |
| input/vars.missing | ✅ (view plot) | 8.06 (-1.17%) | 8.99 (89.65%) |
| test_batched_body | ✅ (view plot) | 2,820.20 (-99.83%) | 2,375,940.33 (0.12%) |
| test_batched_body #2 | ✅ (view plot) | 1,829,900.00 (+0.32%) | 1,871,779.92 (97.76%) |
| test_data_loader | ✅ (view plot) | 484,320.00 (+2.53%) | 489,812.25 (98.88%) |
| with_mustache_expressions | ✅ (view plot) | 1,173.30 (+0.57%) | 1,223.96 (95.86%) |
| with_mustache_literal | ✅ (view plot) | 699.83 (-3.17%) | 759.95 (92.09%) |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
amitksingh1490
force-pushed
the
fix/1790
branch
2 times, most recently
from
579e656 to
2a8b6b0
Compare
| async fn handle_origin_tailcall<T: DeserializeOwned + GraphQLRequestLike>( | ||
| req: Request<Body>, | ||
| app_ctx: Arc<AppContext>, | ||
| request_counter: &mut RequestCounter, | ||
| ) -> Result<Response<Body>> { | ||
| let method = req.method(); | ||
| if method == Method::OPTIONS { | ||
| let mut res = Response::new(Body::default()); | ||
| res.headers_mut().insert( | ||
| header::ACCESS_CONTROL_ALLOW_ORIGIN, | ||
| HeaderValue::from_static("https://tailcall.run"), | ||
| ); | ||
| res.headers_mut().insert( | ||
| header::ACCESS_CONTROL_ALLOW_METHODS, | ||
| HeaderValue::from_static("GET, POST, OPTIONS"), | ||
| ); | ||
| res.headers_mut().insert( | ||
| header::ACCESS_CONTROL_ALLOW_HEADERS, | ||
| HeaderValue::from_static("*"), | ||
| ); | ||
| Ok(res) | ||
| } else { | ||
| let mut res = handle_request_inner::<T>(req, app_ctx, request_counter).await?; | ||
| res.headers_mut().insert( | ||
| header::ACCESS_CONTROL_ALLOW_ORIGIN, | ||
| HeaderValue::from_static("https://tailcall.run"), | ||
| ); | ||
|
|
||
| Ok(res) | ||
| } |
There was a problem hiding this comment.
Ensure the new CORS handling for tailcall.run is thoroughly tested.
#!/bin/bash
# Description: Verify that the new CORS handling logic in `handle_origin_tailcall` is covered by tests.
# Find test files related to `request_handler.rs` and search for tests covering the new CORS logic.
fd 'request_handler.rs' --exec rg --files-with-matches --type rust $'test.*handle_origin_tailcall'
fixes #1790
Cors enabled for tailcall.run if no cors setting is provided