feat(torii): update cli to match katana's #2672
Conversation
WalkthroughOhayo, sensei! This pull request introduces modifications to the Changes
Possibly related PRs
🪧 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: 0
🧹 Outside diff range and nitpick comments (4)
bin/torii/src/main.rs (4)
153-174: Clean metrics configuration structure, sensei!The MetricsOptions struct is well-organized with proper field dependencies. However, there's a minor documentation inconsistency.
Consider updating this comment to reflect the current behavior:
- /// For now, metrics will still be collected even if this flag is not set. This only - /// controls whether the metrics server is started or not. + /// Controls whether the metrics server is started and metrics are exposed via HTTP endpoint.
179-196: Consider adding CORS origin validation, sensei!The ServerOptions struct looks good, but the CORS origins could benefit from validation to ensure they are valid URLs or patterns.
Consider adding a custom parser for http_cors_origins:
fn parse_cors_origin(s: &str) -> Result<String, String> { // Allow * for all origins if s == "*" { return Ok(s.to_string()); } // Validate URL-like patterns if !(s.starts_with("http://") || s.starts_with("https://")) { return Err(format!("Invalid CORS origin: {}. Must start with http:// or https://", s)); } Ok(s.to_string()) }
321-328: Consider enhancing error handling for server initialization, sensei!While the server and metrics initialization logic is correct, we could improve error handling and logging.
Consider this approach for metrics initialization:
if args.metrics.metrics { let addr = SocketAddr::new(args.metrics.metrics_addr, args.metrics.metrics_port); - info!(target: LOG_TARGET, %addr, "Starting metrics endpoint."); let prometheus_handle = PrometheusRecorder::install("torii")?; let server = dojo_metrics::Server::new(prometheus_handle).with_process_metrics(); - tokio::spawn(server.start(addr)); + info!(target: LOG_TARGET, %addr, "Starting metrics endpoint..."); + tokio::spawn(async move { + if let Err(e) = server.start(addr).await { + error!(target: LOG_TARGET, error = %e, "Failed to start metrics server"); + } + }); + info!(target: LOG_TARGET, %addr, "Metrics endpoint started successfully."); }Also applies to: 355-360
340-345: Consider using Url type for safer explorer URL construction, sensei!The current string manipulation approach for constructing the explorer URL could be made more robust.
Consider using the
Urltype for safer URL manipulation:let mut base_url = Url::parse("https://worlds.dev/torii").expect("Static URL is valid"); let gql_url = format!("http://{}/graphql", addr.to_string().replace("0.0.0.0", "localhost")); base_url.query_pairs_mut().append_pair("url", &gql_url); let explorer_url = base_url.to_string();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
bin/torii/src/main.rs(8 hunks)
🔇 Additional comments (2)
bin/torii/src/main.rs (2)
148-151: Ohayo sensei! Nice work on the default configuration constants!
The default values are well-chosen for local development:
- HTTP server defaults to localhost:8080
- Metrics server defaults to localhost:9200
Also applies to: 176-177
64-65: Clean integration of the new options structs, sensei!
The use of #[command(flatten)] for both server and metrics options provides a well-organized CLI structure while maintaining a clean Args definition.
Also applies to: 98-99
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2672 +/- ##
==========================================
- Coverage 57.43% 57.42% -0.02%
==========================================
Files 403 403
Lines 50818 50833 +15
==========================================
Hits 29189 29189
- Misses 21629 21644 +15 ☔ View full report in Codecov by Sentry. |
i assume i don't need to update the config file format manually as it's already handled by |
Exactly. |
This reverts commit 2b5e4de.
ref #2663
standardize cli args across katana and torii
Summary by CodeRabbit