feat(katana) : add explorer flag and build folder #2997
Conversation
ayushtom
changed the title
ayushtom
changed the title
|
Ohayo, sensei! Below is the updated breakdown of the changes: WalkthroughThe changes integrate a new Explorer feature into the Katana project. A new dependency and workspace member for the Explorer crate are added, along with updates to CLI argument parsing to enable Explorer options. The Explorer frontend is implemented with its own Cargo configuration, documentation, and server logic to serve embedded UI assets. Additionally, the CI workflow has been updated to build the Explorer UI and incorporate its output, and Git configurations now account for the Explorer-specific build output and submodule. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Parser
participant Args as NodeArgs
participant Explorer as Explorer Server
participant RPC as RPC Configurator
CLI->>Args: Parse command line arguments
Args->>Args: Check if 'explorer' flag is enabled
alt Explorer enabled
Args->>Explorer: Instantiate Explorer (new with RPC URL)
Explorer->>Explorer: Start server at specified SocketAddr
Args->>RPC: Update RPC config with explorer URL (CORS origins)
end
Possibly related PRs
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 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
🧹 Nitpick comments (5)
crates/katana/cli/src/args.rs (2)
37-37: Consider feature-gating the explorer usage.
Directly importingExplorerServerincreases code size even if the explorer feature is unused. Consider wrapping this import with an optional feature to keep builds lean.
269-278: Add flexibility for custom explorer domains.
Currently, the code unconditionally adds127.0.0.1:<port>to the CORS list. If your explorer is hosted elsewhere or utilizes HTTPS, you may want to allow specifying an alternative domain or protocol.crates/katana/cli/src/lib.rs (1)
7-7: Ohayo sensei, the new explorer module is appreciated.
If you want to reduce compilation overhead when the explorer is not used, consider guarding it behind the same feature flag as other server functionality.crates/katana/cli/src/options.rs (1)
413-428: Ohayo! Nice implementation of ExplorerOptions, sensei! Consider adding port validation.The struct is well-documented and follows the codebase's patterns. However, consider validating that the explorer port doesn't conflict with common service ports (80, 443, etc.) or the RPC port.
#[derive(Debug, Args, Clone, Default,Serialize, Deserialize)] #[command(next_help_heading = "Explorer options")] pub struct ExplorerOptions { /// Enable and launch the explorer frontend #[arg(long)] #[serde(default)] pub explorer: bool, /// The port to run the explorer frontend on - #[arg(long = "explorer-port", default_value_t = 3001)] + #[arg(long = "explorer-port", default_value_t = 3001, value_parser = port_validator)] pub explorer_port: u16, /// Path to the explorer's build directory #[arg(long = "explorer-build-dir")] pub explorer_build_dir: Option<PathBuf>, } +fn port_validator(s: &str) -> Result<u16, String> { + let port: u16 = s.parse().map_err(|_| "Invalid port number")?; + if port == 80 || port == 443 || port == DEFAULT_RPC_PORT { + Err(format!("Port {} is reserved", port)) + } else { + Ok(port) + } +}crates/katana/cli/Cargo.toml (1)
32-32: Ohayo sensei! Let's manage tiny_http in the workspace.For consistency with other dependencies, consider moving tiny_http to workspace management in the root Cargo.toml.
-tiny_http = "0.12" +tiny_http.workspace = trueThen add to the root Cargo.toml's
[workspace.dependencies]section:[workspace.dependencies] tiny_http = "0.12"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.gitmodules(1 hunks)crates/katana/cli/Cargo.toml(1 hunks)crates/katana/cli/src/args.rs(4 hunks)crates/katana/cli/src/explorer.rs(1 hunks)crates/katana/cli/src/lib.rs(1 hunks)crates/katana/cli/src/options.rs(2 hunks)
🔇 Additional comments (8)
crates/katana/cli/src/args.rs (4)
32-32: Ohayo sensei, this import for CORS headers looks fine.
It aligns well with the upcoming explorer functionality.
120-122: Flatten usage of explorer options is solid.
This nicely integrates the CLI flags for the explorer intoNodeArgs.
144-174: Support additional bind addresses for the explorer.
Right now, the server strictly binds to127.0.0.1with the specified port. If you foresee remote usage or containerization, consider making the explorer address configurable or verifying open ports.
[refactor_suggestion_good_to_have, verify]
284-284: No issues here; looks good to go.
This line simply references the updated CORS origins variable.crates/katana/cli/src/explorer.rs (4)
1-12: Struct fields are well-chosen.
port,build_dir, andrpc_urlcover the essentials. If you plan to support more advanced usage, consider adding an address field for external binding.
[approved, refactor_suggestion_good_to_have]
14-21: Validate server initialization parameters.
Ensure the providedportis valid and non-zero to prevent runtime issues.
23-95: Consider handling 404s and large file requests.
The current approach defaults to servingindex.htmlif a file isn't found, which may mask missing files. Also, synchronous file I/O might block under heavy load; switching to an asynchronous model could improve performance.
[refactor_suggestion_good_to_have, nitpick]
98-112: Take care when injecting HTML.
Although settingVITE_RPC_URLis convenient, a maliciously formedrpc_urlmight open up XSS vectors. Consider sanitizing or validating the string if you expect untrusted input.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/katana/cli/src/args.rs(4 hunks)crates/katana/cli/src/explorer.rs(1 hunks)crates/katana/cli/src/options.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/katana/cli/src/explorer.rs
- crates/katana/cli/src/args.rs
🔇 Additional comments (3)
crates/katana/cli/src/options.rs (3)
34-34: Ohayo! New import for PathBuf.The import of
std::path::PathBufis correctly added to support the newexplorer_build_dirfield in theExplorerOptionsstruct.
422-423: Explorer port configured properly, sensei!The default port is set to 3001 which is a good choice as it avoids common conflicts with other services. The field is correctly typed as
u16which is appropriate for a port number.
426-427: Consider adding validation for the build directory path, sensei.The
explorer_build_dirfield is correctly typed asOption<PathBuf>, but the code doesn't validate whether the path exists or is a valid directory. Consider adding validation logic when this option is used.The consumer of this option should verify that the directory exists and contains the expected explorer build files before attempting to serve them.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/katana/cli/src/options.rs (1)
413-428: 🛠️ Refactor suggestionOhayo! Implement a merge method for consistency, sensei.
The
ExplorerOptionsstruct looks excellent and follows the project's patterns. I see you've already addressed the previous feedback by adding proper spacing and thePartialEqtrait. However, other option structs in this file have amergemethod to handle configuration merging from different sources, which is missing here.Here's an implementation to maintain consistency:
#[derive(Debug, Args, Clone, Default, Serialize, Deserialize, PartialEq)] #[command(next_help_heading = "Explorer options")] pub struct ExplorerOptions { /// Enable and launch the explorer frontend #[arg(long)] #[serde(default)] pub explorer: bool, /// The port to run the explorer frontend on #[arg(long = "explorer.port", default_value_t = 3001)] pub explorer_port: u16, /// Path to the explorer's build directory #[arg(long = "explorer.build-dir")] pub explorer_build_dir: Option<PathBuf>, } +impl ExplorerOptions { + pub fn merge(&mut self, other: Option<&Self>) { + if let Some(other) = other { + if !self.explorer { + self.explorer = other.explorer; + } + + if self.explorer_port == 3001 { + self.explorer_port = other.explorer_port; + } + + if self.explorer_build_dir.is_none() { + self.explorer_build_dir = other.explorer_build_dir.clone(); + } + } + } +}
🧹 Nitpick comments (1)
crates/katana/cli/src/options.rs (1)
422-423: Consider defining a constant for the default explorer port, sensei.Following the pattern used elsewhere in this file, it would be more maintainable to define a constant for the default explorer port (3001) and reference it in both the argument default and potentially in the merge method.
+const DEFAULT_EXPLORER_PORT: u16 = 3001; #[derive(Debug, Args, Clone, Default, Serialize, Deserialize, PartialEq)] #[command(next_help_heading = "Explorer options")] pub struct ExplorerOptions { // ... /// The port to run the explorer frontend on - #[arg(long = "explorer.port", default_value_t = 3001)] + #[arg(long = "explorer.port", default_value_t = DEFAULT_EXPLORER_PORT)] pub explorer_port: u16, // ... } +fn default_explorer_port() -> u16 { + DEFAULT_EXPLORER_PORT +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/katana/cli/src/explorer.rs(1 hunks)crates/katana/cli/src/options.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/cli/src/explorer.rs
There was a problem hiding this comment.
Sorry for late review. I think we should just embed the explorer's build files directly into the binary with something like rust-embed. Making users build the explorer themselves feels kinda clunky. We could just build it in our CI pipeline instead.
Also would prefer if this could be moved into its own crate (/crates/katana/explorer), but more of a nitpick for now.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
crates/katana/explorer/build.rs (1)
1-14: Ohayo! Build script for explorer assets looks good.This build script correctly checks for the existence of the
distdirectory and provides clear warnings when it's missing. Thecargo:rerun-if-changed=distdirective ensures the build script reruns when the directory contents change.A few suggestions to enhance this:
- Consider providing more specific instructions on how to generate the build files (e.g., "Run
npm run buildin the explorer directory").- Maybe add a check for specific expected files within the dist directory to ensure completeness.
fn main() { println!("cargo:rerun-if-changed=dist"); // Check if the dist directory exists within the explorer crate let dist_dir = Path::new("dist"); if !dist_dir.exists() { println!("cargo:warning=Explorer dist directory not found at {:?}. The embedded explorer will not be available.", dist_dir); - println!("cargo:warning=Please ensure the explorer build files are copied to the 'dist' directory in the explorer crate."); + println!("cargo:warning=Please ensure the explorer build files are copied to the 'dist' directory in the explorer crate."); + println!("cargo:warning=This can typically be done by running `npm run build` in the explorer UI project directory."); } else { println!("Using explorer build files from {:?}", dist_dir); + // Check for a key file to ensure the build is complete + let index_html = dist_dir.join("index.html"); + if !index_html.exists() { + println!("cargo:warning=Explorer build files may be incomplete. Could not find index.html in the dist directory."); + } } }crates/katana/explorer/README.md (1)
1-14: Ohayo! README for the new explorer crate.The README provides a good overview of the crate's purpose and structure. There are a couple of small improvements we could make:
- The line "The explorer build files" could use more context about what these files are (React/UI files).
- The last line is missing a period at the end.
- Consider adding more details about how to build the explorer files or where they come from.
# Katana Explorer This crate embeds the Katana Explorer UI files directly into the binary, eliminating the need for users to build the explorer themselves. ## Structure -The explorer build files are expected to be in the `dist` directory within this crate. These files are embedded at compile time using the `rust-embed` crate. +The explorer build files (React/UI assets) are expected to be in the `dist` directory within this crate. These files are embedded at compile time using the `rust-embed` crate. ## Utilities This crate also provides some utility functions for working with the embedded files: - -- `inject_rpc_url`: Injects the RPC URL into an HTML file +- `inject_rpc_url`: Injects the RPC URL into an HTML file. + +## Development + +To update the explorer UI, you'll need to build the UI project and copy the generated files to the `dist` directory in this crate.🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: The verb form ‘build’ does not appear to fit in this context.
Context: ...explorer themselves. ## Structure The explorer build files are expected to be in thedist...(SINGULAR_NOUN_VERB_AGREEMENT)
[uncategorized] ~13-~13: Loose punctuation mark.
Context: ... the embedded files: -inject_rpc_url: Injects the RPC URL into an HTML file(UNLIKELY_OPENING_PUNCTUATION)
crates/katana/explorer/src/lib.rs (1)
30-45: Consider adding more common file types and handling query parameters, senseiThe content type mapping is missing some common image formats like jpg/jpeg and gif. Also, file paths might include query parameters that should be ignored when determining the content type.
pub fn get_content_type(path: &str) -> &'static str { + // Strip query parameters if present + let path = path.split('?').next().unwrap_or(path); + match path.rsplit('.').next() { Some("html") => "text/html", Some("js") => "application/javascript", Some("css") => "text/css", Some("png") => "image/png", + Some("jpg") | Some("jpeg") => "image/jpeg", + Some("gif") => "image/gif", Some("svg") => "image/svg+xml", Some("json") => "application/json", Some("ico") => "image/x-icon", Some("woff") => "font/woff", Some("woff2") => "font/woff2", Some("ttf") => "font/ttf", Some("eot") => "application/vnd.ms-fontobject", _ => "application/octet-stream", } }crates/katana/cli/src/explorer.rs (1)
75-89: Consider conditional CORS headers based on response typeCurrently, CORS headers are added to all responses including error responses. Consider adding them conditionally based on the response type or status code for better clarity.
- // Add CORS headers - let response = content - .with_header(tiny_http::Header { - field: "Access-Control-Allow-Origin".parse().unwrap(), - value: "*".parse().unwrap(), - }) - .with_header(tiny_http::Header { - field: "Access-Control-Allow-Methods".parse().unwrap(), - value: "GET, POST, OPTIONS".parse().unwrap(), - }) - .with_header(tiny_http::Header { - field: "Access-Control-Allow-Headers".parse().unwrap(), - value: "Content-Type".parse().unwrap(), - }); + // Add CORS headers + let mut response = content; + + // Only add CORS headers for successful responses + if response.status_code() < 400 { + response = response + .with_header(tiny_http::Header { + field: "Access-Control-Allow-Origin".parse().unwrap(), + value: "*".parse().unwrap(), + }) + .with_header(tiny_http::Header { + field: "Access-Control-Allow-Methods".parse().unwrap(), + value: "GET, POST, OPTIONS".parse().unwrap(), + }) + .with_header(tiny_http::Header { + field: "Access-Control-Allow-Headers".parse().unwrap(), + value: "Content-Type".parse().unwrap(), + }); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.toml(2 hunks)crates/katana/cli/Cargo.toml(2 hunks)crates/katana/cli/src/args.rs(4 hunks)crates/katana/cli/src/explorer.rs(1 hunks)crates/katana/cli/src/options.rs(1 hunks)crates/katana/explorer/.gitignore(1 hunks)crates/katana/explorer/Cargo.toml(1 hunks)crates/katana/explorer/README.md(1 hunks)crates/katana/explorer/build.rs(1 hunks)crates/katana/explorer/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- crates/katana/explorer/.gitignore
- crates/katana/explorer/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/katana/cli/src/args.rs
- crates/katana/cli/src/options.rs
🧰 Additional context used
🪛 LanguageTool
crates/katana/explorer/README.md
[grammar] ~7-~7: The verb form ‘build’ does not appear to fit in this context.
Context: ...explorer themselves. ## Structure The explorer build files are expected to be in the dist ...
(SINGULAR_NOUN_VERB_AGREEMENT)
[uncategorized] ~13-~13: Loose punctuation mark.
Context: ... the embedded files: - inject_rpc_url: Injects the RPC URL into an HTML file
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (4)
crates/katana/cli/Cargo.toml (2)
11-11: Ohayo! New dependency added for explorer feature.Good addition of the workspace dependency
katana-explorer. This aligns perfectly with the PR objective of adding an explorer feature to Katana.
33-33:✅ Verification successful
Using tiny_http for serving the explorer UI, nice choice sensei!
The
tiny_httpcrate is a lightweight HTTP server that's perfect for this use case. Version 0.12 is current as of this review.
🏁 Script executed:
#!/bin/bash # Check if there are newer versions of tiny_http available cargo search tiny_http --limit 1Length of output: 130
Ohayo sensei! It looks like our initial cargo command didn’t work because the tool wasn’t available in our sandbox environment. To double-check that version 0.12 is still the most current release for the tiny_http crate, please verify using the following shell script (or confirm manually via crates.io):
Once you’ve confirmed that 0.12 is indeed up-to-date, your usage of tiny_http for serving the explorer UI looks great—nice choice, sensei!
🏁 Script executed:
[run_scripts] #!/bin/bash # Fetch the latest version of tiny_http from crates.io curl -s "https://crates.io/api/v1/crates/tiny_http" | jq '.crate.max_version'Length of output: 206
Ohayo sensei! After verifying with the latest crates.io data, it's confirmed that
tiny_httpis at version 0.12.0, which aligns perfectly with the code snippet incrates/katana/cli/Cargo.toml. Your choice to usetiny_httpfor serving the explorer UI is solid and up-to-date—nice call, sensei!Cargo.toml (2)
21-21: Ohayo! New explorer crate added to workspace members.Good addition of the explorer crate to the workspace members list. This is necessary for the new explorer feature.
102-102: New workspace dependency for the explorer crate, nice work sensei!The addition of the
katana-explorerworkspace dependency with the correct path looks good. This allows other crates in the workspace to use the explorer functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/katana/cli/src/options.rs (1)
416-452: Consider adding amergemethod for consistency with other option structs.Other option structs in this file implement a
mergemethod to handle configuration merging from different sources. For consistency, consider adding a similar implementation forExplorerOptions.impl ExplorerOptions { /// Returns the socket address for the explorer frontend. pub fn addr(&self) -> SocketAddr { SocketAddr::new(self.explorer_addr, self.explorer_port) } + + pub fn merge(&mut self, other: Option<&Self>) { + if let Some(other) = other { + if !self.explorer { + self.explorer = other.explorer; + } + + if self.explorer_addr == DEFAULT_EXPLORER_ADDR { + self.explorer_addr = other.explorer_addr; + } + + if self.explorer_port == DEFAULT_EXPLORER_PORT { + self.explorer_port = other.explorer_port; + } + } + } }crates/katana/explorer/src/lib.rs (1)
31-125: Consider adding a way to gracefully stop the explorer server.The
startmethod spawns a thread but doesn't provide a way to stop it or join it later. This could lead to resource leaks if the main application needs to shut down the explorer.Consider returning the thread handle or implementing a stop method:
pub fn start(&self, addr: SocketAddr) -> Result<()> { let server = Server::http(addr).map_err(|e| anyhow!("Failed to start explorer server: {}", e))?; let rpc_url = self.rpc_url.clone(); - let _handle = thread::spawn(move || { + let handle = thread::spawn(move || { for request in server.incoming_requests() { // ... existing code ... } }); info!(target: "katana", %addr, "Explorer started."); - Ok(()) + Ok(handle) }Then the caller could join the thread when needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/katana/cli/Cargo.toml(1 hunks)crates/katana/cli/src/args.rs(5 hunks)crates/katana/cli/src/options.rs(2 hunks)crates/katana/explorer/Cargo.toml(1 hunks)crates/katana/explorer/build.rs(1 hunks)crates/katana/explorer/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/katana/cli/Cargo.toml
- crates/katana/explorer/build.rs
- crates/katana/cli/src/args.rs
🔇 Additional comments (7)
crates/katana/cli/src/options.rs (2)
416-435: Ohayo, sensei! New explorer options struct looks well-structured.The new
ExplorerOptionsstruct is designed well, following the project patterns with proper derive attributes and documentation. The configuration options for enabling the explorer and specifying its address and port are sensible.
437-442: Theaddr()helper method is a nice touch!Good design choice to provide the
addr()helper method that combines the address and port into aSocketAddr. This simplifies usage elsewhere in the codebase.crates/katana/explorer/Cargo.toml (1)
1-18: Ohayo! Clean package configuration for the new explorer crate.The package metadata and dependencies are well-structured and appropriate for the explorer functionality. The use of
rust-embedfor embedding static files andtiny_httpfor serving is a good choice for this use case.crates/katana/explorer/src/lib.rs (4)
16-28: Smart approach to validate embedded assets on initialization, sensei!Good practice to check if the embedded assets are available during initialization and log a warning if they're not. This helps users identify potential issues early.
63-75: Good path sanitization to prevent directory traversal attacks!The code properly sanitizes paths to prevent directory traversal attacks by checking for ".." patterns and normalizing empty paths to "index.html". This is an important security practice.
135-153: Good job implementing the XSS protection, sensei!The URL escaping implementation properly handles special characters to prevent XSS vulnerabilities. This addresses the previous review comment about securing the RPC URL injection.
156-171: Comprehensive MIME type mapping.The
get_content_typefunction handles a wide range of file types commonly used in web applications. This ensures that files are served with the correct Content-Type headers for proper browser handling.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
crates/katana/explorer/src/lib.rs (5)
31-125: Ohayo sensei! Consider improving thread management for graceful shutdownThe current implementation spawns a thread but doesn't provide a way to gracefully shut down the explorer server. This could lead to orphaned threads if the main application terminates.
Consider adding a shutdown mechanism:
pub struct Explorer { /// The JSON-RPC url of the chain that the explorer will connect to. rpc_url: Url, + /// Handle to the server thread for graceful shutdown + server_handle: Option<thread::JoinHandle<()>>, } impl Explorer { // ... pub fn start(&self, addr: SocketAddr) -> Result<()> { let server = Server::http(addr).map_err(|e| anyhow!("Failed to start explorer server: {}", e))?; let rpc_url = self.rpc_url.clone(); - let _handle = thread::spawn(move || { + let handle = thread::spawn(move || { // Existing implementation }); + // Store the handle for later use + self.server_handle = Some(handle); info!(target: "katana", %addr, "Explorer started."); Ok(()) } + /// Stop the explorer server gracefully + pub fn stop(self) -> Result<()> { + if let Some(handle) = self.server_handle { + // Signal thread to stop (implementation depends on how you want to signal) + handle.join().map_err(|_| anyhow!("Failed to join explorer server thread"))?; + } + Ok(()) + } }
40-60: Extract CORS headers to reduce code duplicationOhayo sensei! There's duplication in the CORS header logic between the OPTIONS request handler and the regular response handler.
Create a helper function to add CORS headers:
+ /// Adds CORS headers to a response + fn add_cors_headers(response: Response<impl std::io::Read>) -> Response<impl std::io::Read> { + response + .with_header(tiny_http::Header { + field: "Access-Control-Allow-Origin".parse().unwrap(), + value: "*".parse().unwrap(), + }) + .with_header(tiny_http::Header { + field: "Access-Control-Allow-Methods".parse().unwrap(), + value: "GET, POST, OPTIONS".parse().unwrap(), + }) + .with_header(tiny_http::Header { + field: "Access-Control-Allow-Headers".parse().unwrap(), + value: "Content-Type, Authorization".parse().unwrap(), + }) + } // Then in the code: if request.method() == &tiny_http::Method::Options { - let response = Response::empty(204) - .with_header(tiny_http::Header { - field: "Access-Control-Allow-Origin".parse().unwrap(), - value: "*".parse().unwrap(), - }) - // ... other headers + let response = add_cors_headers(Response::empty(204)) .with_header(tiny_http::Header { field: "Access-Control-Max-Age".parse().unwrap(), value: "86400".parse().unwrap(), }); // ... } // And later: - let response = content - .with_header(tiny_http::Header { - field: "Access-Control-Allow-Origin".parse().unwrap(), - value: "*".parse().unwrap(), - }) - // ... other headers + let response = add_cors_headers(content);Also applies to: 101-115
78-99: Consider adding caching headers for better performanceOhayo! Adding proper caching headers would improve performance for static assets.
Add Cache-Control headers based on file type:
// Try to serve from embedded assets let content = if let Some(asset) = ExplorerAssets::get(&path[1..]) { let content_type = get_content_type(&path); let content = asset.data; + + // Create a base response with common headers + let mut response = if content_type == "text/html" { + let html = String::from_utf8_lossy(&content).to_string(); + let html = inject_rpc_url(&html, &rpc_url); + Response::from_string(html) + } else { + Response::from_data(content.to_vec()) + }; + + // Add Content-Type header + response = response.with_header(tiny_http::Header { + field: "Content-Type".parse().unwrap(), + value: content_type.parse().unwrap(), + }); + + // Add caching headers based on file type + let cache_control = if content_type.starts_with("image/") || + content_type.contains("font") || + path.ends_with(".js") || + path.ends_with(".css") { + // Static assets can be cached longer + "public, max-age=86400" + } else { + // HTML and other files should be checked more frequently + "no-cache" + }; + + response = response.with_header(tiny_http::Header { + field: "Cache-Control".parse().unwrap(), + value: cache_control.parse().unwrap(), + }); + + response - // If it's HTML, inject the RPC URL - if content_type == "text/html" { - let html = String::from_utf8_lossy(&content).to_string(); - let html = inject_rpc_url(&html, &rpc_url); - Response::from_string(html).with_header(tiny_http::Header { - field: "Content-Type".parse().unwrap(), - value: content_type.parse().unwrap(), - }) - } else { - Response::from_data(content.to_vec()).with_header(tiny_http::Header { - field: "Content-Type".parse().unwrap(), - value: content_type.parse().unwrap(), - }) - } } else { // Not found Response::from_string("Not found").with_status_code(404) };
116-118: Provide more detailed error loggingOhayo sensei! Error logging could be more informative by including the requested path.
- if let Err(e) = request.respond(response) { - warn!(target: "katana", "Error sending response: {}", e); - } + if let Err(e) = request.respond(response) { + warn!( + target: "katana", + "Error sending response for path {}: {}", + request.url(), + e + ); + }
128-131: Consider implementing a default fallback for the embedded assetsOhayo! It would be helpful to have a fallback mechanism when embedded assets are missing.
Consider adding a default asset implementation:
/// Embedded explorer UI files. #[derive(RustEmbed)] #[folder = "dist"] struct ExplorerAssets; +impl ExplorerAssets { + /// Gets an asset, or returns a default minimal HTML if "index.html" is requested + /// and embedded assets are not available + fn get_with_fallback(path: &str) -> Option<rust_embed::EmbeddedFile> { + Self::get(path).or_else(|| { + if path == "index.html" { + // Create a minimal HTML file as fallback + warn!( + target: "katana", + "Using fallback HTML for explorer as embedded assets were not found" + ); + Some(rust_embed::EmbeddedFile { + data: std::borrow::Cow::Owned( + r#"<!DOCTYPE html> +<html> +<head> + <meta charset="UTF-8"> + <title>Katana Explorer</title> + <style> + body { font-family: system-ui; max-width: 800px; margin: 0 auto; padding: 2rem; } + pre { background: #f1f1f1; padding: 1rem; border-radius: 4px; } + </style> +</head> +<body> + <h1>Katana Explorer</h1> + <p>The explorer assets were not found. Please make sure to build the explorer:</p> + <pre>cd crates/katana/explorer && cargo build</pre> +</body> +</html>"#.as_bytes().to_vec(), + ), + metadata: Default::default(), + }) + } else { + None + } + }) + } +}And then update the code in the
startmethod to use this:- if let Some(asset) = ExplorerAssets::get(&path[1..]) { + if let Some(asset) = ExplorerAssets::get_with_fallback(&path[1..]) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/katana/cli/src/args.rs(5 hunks)crates/katana/explorer/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/cli/src/args.rs
🔇 Additional comments (2)
crates/katana/explorer/src/lib.rs (2)
135-153: LGTM! Well implemented URL injection with XSS protectionGreat job implementing the XSS protection that was suggested in the previous review. The code properly escapes special characters in the URL.
156-171: LGTM! Comprehensive MIME type mappingOhayo sensei! The MIME type mapping is thorough and covers all the common web file types.
|
I took the liberty to change some things, mostly around styling (read: nitpicks) and conventions that |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/katana/explorer/src/lib.rs (2)
61-73: 🛠️ Refactor suggestionEnhance path sanitization for better security
Ohayo sensei! The path sanitization logic could be improved to be more robust against path traversal attempts.
Consider using a more comprehensive approach:
// Decode URL and sanitize path to prevent directory traversal let path = { let url_path = request.url().to_string(); let decoded_path = urlencoding::decode(&url_path) .map(|s| s.into_owned()) .unwrap_or_else(|_| url_path); - let p = decoded_path.trim_start_matches('/'); - if p.is_empty() || p.contains("..") || p.starts_with('/') { - "/index.html".to_string() - } else { - format!("/{}", p) - } + // More thoroughly sanitize the path + let p = decoded_path.trim_start_matches('/'); + let components: Vec<&str> = p.split('/').filter(|s| { + !s.is_empty() && *s != "." && *s != ".." && !s.contains('\\') + }).collect(); + + if components.is_empty() { + "/index.html".to_string() + } else { + format!("/{}", components.join("/")) + } }
109-112: 🛠️ Refactor suggestionUpdate CORS Allow-Headers value to match preflight response
Ohayo sensei! The Access-Control-Allow-Headers value in regular responses differs from the preflight response.
Make the header values consistent:
.with_header(tiny_http::Header { field: "Access-Control-Allow-Headers".parse().unwrap(), - value: "Content-Type".parse().unwrap(), + value: "Content-Type, Authorization".parse().unwrap(), })
🧹 Nitpick comments (7)
crates/katana/explorer/README.md (2)
7-7: Small grammar fix neededOhayo sensei! There's a small grammar issue in this line.
-The explorer UI code is located in the `ui` directory as a git submodule. The built files must be present in `ui/dist` directory for the explorer to work. +The explorer UI code is located in the `ui` directory as a git submodule. The built files must be present in the `ui/dist` directory for the explorer to work.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ...ule. The built files must be present inui/distdirectory for the explorer to w...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
53-53: Add a period to the end of the sentenceOhayo! For consistency with other bullet points, let's add a period at the end.
-- `inject_rpc_url`: Injects the RPC URL into an HTML file +- `inject_rpc_url`: Injects the RPC URL into an HTML file.🧰 Tools
🪛 LanguageTool
[uncategorized] ~53-~53: Loose punctuation mark.
Context: ... the embedded files: -inject_rpc_url: Injects the RPC URL into an HTML file(UNLIKELY_OPENING_PUNCTUATION)
.github/workflows/ci.yml (2)
14-32: Update checkout action versionOhayo sensei! The GitHub action
actions/checkout@v3is outdated. Update to the latest version for better compatibility and security.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
36-46: Update checkout action version in build jobOhayo sensei! Similar to the previous comment, update the checkout action in the build job as well.
- - uses: actions/checkout@v3 + - uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
40-40: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
crates/katana/explorer/src/lib.rs (3)
96-96: Consider returning index.html for 404sOhayo sensei! For single-page applications, a common pattern is to return the index.html file for 404s to support client-side routing.
- // Not found - Response::from_string("Not found").with_status_code(404) + // For single-page apps, return index.html on 404 to support client-side routing + if let Some(index_asset) = ExplorerAssets::get("index.html") { + let html = String::from_utf8_lossy(&index_asset.data).to_string(); + let html = inject_rpc_url(&html, &rpc_url); + Response::from_string(html).with_header(tiny_http::Header { + field: "Content-Type".parse().unwrap(), + value: "text/html".parse().unwrap(), + }) + } else { + Response::from_string("Not found").with_status_code(404) + }
19-23: Consider enhancing the error with troubleshooting infoOhayo sensei! The error message could be more helpful by including instructions for resolving the issue.
if ExplorerAssets::get("index.html").is_none() { return Err(anyhow!( - "Explorer assets not found. Make sure the explorer UI is built in CI and the ui/dist directory is available." + "Explorer assets not found. Make sure the explorer UI is built and the ui/dist directory is available. Run 'git submodule update --init --recursive' followed by 'cd crates/katana/explorer/ui && bun install && bun run build'." )); }
29-29: Consider alternatives to tiny_http for future scalabilityOhayo sensei! While
tiny_httpworks for this simple use case, more feature-rich alternatives likeaxumcould provide better scalability and maintainability as the explorer functionality grows.For future enhancements, consider migrating to a more robust framework like
axum, which offers:
- Better ergonomics through async/await
- Built-in middleware support
- Type-safe routing
- Better integration with the Tokio ecosystem
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/ci.yml(1 hunks).gitignore(1 hunks).gitmodules(1 hunks)crates/katana/cli/src/args.rs(5 hunks)crates/katana/explorer/Cargo.toml(1 hunks)crates/katana/explorer/README.md(1 hunks)crates/katana/explorer/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/cli/src/args.rs
🧰 Additional context used
🪛 LanguageTool
crates/katana/explorer/README.md
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ...ule. The built files must be present in ui/dist directory for the explorer to w...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~53-~53: Loose punctuation mark.
Context: ... the embedded files: - inject_rpc_url: Injects the RPC URL into an HTML file
(UNLIKELY_OPENING_PUNCTUATION)
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
40-40: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (8)
.gitmodules (1)
7-10: Ohayo! Let's pin that submodule to a commit, sensei!Using the branch directly (
branch = dist) risks unexpected updates since its history can change. It's safer to pin the submodule to a specific commit hash instead.#!/bin/bash # Get the latest commit hash from the dist branch of the repository git ls-remote https://github.com/ayushtom/dojo-explorer dist | cut -f1crates/katana/explorer/Cargo.toml (1)
1-15: LGTM! Clean Cargo.toml setupOhayo sensei! The package definition and dependencies look well-structured. All required dependencies for the explorer server implementation are properly specified with workspace inheritance.
crates/katana/explorer/README.md (2)
19-31: LGTM! Clear build instructionsOhayo sensei! The build instructions are clear and provide all necessary steps for both CI and local development. This will be helpful for contributors.
36-37: LGTM! Good fail-fast approachOhayo! This is a good design choice to fail fast when UI files are missing rather than serving a broken explorer.
.github/workflows/ci.yml (1)
14-32: LGTM! Well-structured Explorer build jobOhayo sensei! The Explorer build job is well-structured, with proper dependency ordering, submodule checkout, and artifact upload steps.
🧰 Tools
🪛 actionlint (1.7.4)
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
crates/katana/explorer/src/lib.rs (3)
17-26: LGTM! Good validation of embedded assetsOhayo sensei! The validation of embedded assets at initialization time is a good practice to fail fast if resources are missing, rather than discovering issues at runtime.
114-116: Use error! log level instead of warnOhayo sensei! This should be an error instead of a warning since it indicates a failure to respond to a request.
-warn!(target: "katana", "Error sending response: {}", e); +error!(target: "katana", "Error sending response: {}", e);
133-151: LGTM! Good XSS prevention in the inject_rpc_url functionOhayo sensei! The implementation properly escapes special characters to prevent XSS vulnerabilities. This is important security practice when injecting dynamic content into HTML.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/katana/explorer/src/lib.rs (1)
35-36: Ohayo sensei! Consider implementing graceful shutdown.Spawning a thread for the server is convenient, but it currently lacks a clean way to stop. This can complicate maintenance, testing, or controlled restarts. A graceful shutdown mechanism (e.g., using a signal or an atomic flag) would improve stability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/katana/explorer/Cargo.toml(1 hunks)crates/katana/explorer/src/lib.rs(1 hunks)
🔇 Additional comments (5)
crates/katana/explorer/Cargo.toml (2)
1-7: Ohayo, Sensei! Package metadata looks spot-on.
The workspace fields for version, edition, repository, and license ensure uniformity across your monorepo. Nice work setting these up for the new explorer crate.
8-15: Ohayo, Sensei! Dependencies section is well-crafted.
The dependencies are clearly defined with appropriate version constraints and workspace settings. This will help maintain consistency and simplifies dependency management across the workspace.crates/katana/explorer/src/lib.rs (3)
49-50: Ohayo sensei! Make CORS headers consistent across preflight and regular responses.The preflight path (lines 49–50) allows
Content-Type, Authorization, while the regular response (lines 114–116) only allowsContent-Type. This mismatch can cause client-side CORS issues ifAuthorizationis required.Also applies to: 114-116
60-78: Ohayo sensei! Path sanitization looks good.Filtering out empty strings, ".", "..", and backslashes mitigates directory traversal attempts. Nice job ensuring we default to "/index.html" when needed.
135-155: Ohayo sensei! Great work on preventing XSS by escaping special characters.The
inject_rpc_urlfunction properly replaces<,>, and quotes. This reduces the risk of malicious script injection.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/katana/cli/src/options.rs (1)
416-442: Ohayo sensei! Consider adding a merge method for consistency with other option structs.Your other structs (e.g.,
ServerOptions,StarknetOptions) implement amergemethod to unify configuration from multiple sources. Doing the same forExplorerOptionscan keep the codebase consistent and extensible in scenarios where partial overrides might be needed.crates/katana/explorer/src/lib.rs (2)
152-172: Ohayo sensei! Consider escaping additional characters in the RPC URL.You already escape
<,>, and". For extra safety, you may also consider escaping'or backticks in case the RPC URL contains them.
174-190: Ohayo sensei! Expand content type coverage.If the Explorer UI uses files like
.map(source maps) or.wasm(WebAssembly), consider adding mappings for these extensions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/katana/cli/src/args.rs(6 hunks)crates/katana/cli/src/options.rs(2 hunks)crates/katana/explorer/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/cli/src/args.rs
🔇 Additional comments (4)
crates/katana/cli/src/options.rs (1)
444-452: Ohayo sensei! The default implementation looks neat.This aligns with the pattern used across other option structs. Reviewed and looking good.
crates/katana/explorer/src/lib.rs (3)
10-28: Ohayo sensei! TheExplorerstruct and constructor are thoughtfully designed.Performing an early check for embedded assets prevents runtime surprises later. Nicely done.
29-62: Ohayo sensei! Consider more robust concurrency or a framework approach.You’ve spawned a single thread for incoming requests via
tiny_http, which is fine for simpler use cases. For larger workloads or advanced routing, frameworks like Axum or Warp may be beneficial.
107-120: Ohayo sensei! Align the CORS headers with preflight responses.Currently, the preflight
Access-Control-Allow-HeadersincludesAuthorization, but the main response headers allow onlyContent-Type. This mismatch can cause CORS issues for requests needingAuthorization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
18-22: Update Checkout Action Version in Explorer BuildOhayo sensei, a quick note here: the static analysis tool flagged the usage of
actions/checkout@v3(line 18) as potentially outdated. It might be beneficial to upgrade to a newer version (for example,actions/checkout@v4if available) to leverage the latest improvements and bug fixes. Here’s a suggested diff for improvement:- - uses: actions/checkout@v3 + - uses: actions/checkout@v4This update could help eliminate any compatibility or performance issues flagged by actionlint.
[static_analysis]🧰 Tools
🪛 actionlint (1.7.4)
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
44-46: Refresh Checkout Action in Build JobOhayo sensei, similar to the Explorer build step, the checkout step in the
buildjob (lines 44–46) is still usingactions/checkout@v3, which the static analysis tool recommends updating. Consider the following diff:- - uses: actions/checkout@v3 - with: - submodules: true + - uses: actions/checkout@v4 + with: + submodules: trueUpgrading the checkout action here should smooth out any potential issues and align practices across the workflow.
[static_analysis]🧰 Tools
🪛 actionlint (1.7.4)
44-44: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
44-44: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
14-37: Explorer Build Job IntegrationOhayo sensei, fantastic work integrating the new Explorer UI build job! The configuration for the
build-explorerjob (lines 14–37) is clear and well-structured: it checks out the repository (with submodules), initializes them, sets up Bun, builds the Explorer UI, and finally uploads the resulting distribution artifact. One minor consideration: since you’re both using the checkout action with submodule handling and explicitly running agit submodule updatecommand, please confirm that this redundancy is intentional—for example, if additional submodule initialization steps are required beyond what the checkout provides.🧰 Tools
🪛 actionlint (1.7.4)
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
38-50: Build Job Dependency and Artifact IntegrationOhayo sensei, the update to the
buildjob (lines 38–50) now correctly includesbuild-explorerin itsneedsarray (line 40), ensuring that the Explorer UI is built before proceeding. The subsequent download of theexplorer-distartifact is consistent with the upload step in the Explorer job. Please double-check that the artifact’s path (crates/katana/explorer/ui/dist) aligns with the consuming tasks later in the build. Overall, this alignment should help maintain a smooth and integrated CI flow.🧰 Tools
🪛 actionlint (1.7.4)
44-44: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
14-38: Ohayo Sensei, fantastic work on integrating the new build-explorer job!
This new job is well structured to build the Explorer UI and upload its artifact. However, as noted by static analysis, please consider updating the checkout action version fromactions/checkout@v3toactions/checkout@v4for improved reliability and compatibility with the latest GitHub Actions features.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
45-47: Ohayo Sensei, a quick nitpick on the build job checkout step!
For consistency across jobs and in line with the static analysis recommendations, please update the checkout action in the build job (lines 45-47) fromactions/checkout@v3toactions/checkout@v4.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
45-45: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
45-45: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
48-51: Ohayo Sensei, nice integration of the Explorer UI artifact!
The steps to download the Explorer artifact in the build job look solid. Just ensure that the path (crates/katana/explorer/ui/dist) remains accurate if future changes alter the Explorer UI build output directory.
40-41: Good dependency management, Sensei!
Addingbuild-exploreras a dependency in the build job’sneedslist ensures proper ordering and integration of the Explorer build. Keep an eye on this dependency chain for any future modifications.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
14-39: Ohayo, Sensei! The new "build-explorer" job looks like a solid addition to our CI pipeline.
This job is clearly structured to build the Explorer UI and upload its artifact. A couple of pointers:
- The checkout step (lines 18–21) is using
actions/checkout@v3, which our static analysis flags as outdated. Consider updating it (e.g., toactions/checkout@v4) to benefit from the latest improvements and compatibility fixes.- The
git submodule addcommand (line 24) is a neat way to initialize the submodule; however, please ensure this command is idempotent or add a check to prevent failures if the submodule already exists.🧰 Tools
🪛 actionlint (1.7.4)
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
47-49: Ohayo, Sensei! Please Update the Checkout Action Version in the Build Job.
The checkout step here (lines 47–49) also usesactions/checkout@v3, which is indicated by our static analysis as being too old. Upgrading to a more recent version (such asactions/checkout@v4) will help avoid potential issues and leverage improved performance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
46-46: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
42-43: Nice Job on Chaining Dependencies in the Build Job!
Updating theneedsclause to includebuild-explorer(line 42) ensures that the main build job waits until the Explorer UI build is complete. This sequencing is crucial for maintaining consistency across our builds.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/ci.yml (3)
14-39: Ohayo sensei, great job introducing the build-explorer job!
The new job for building the Explorer UI is well structured and integrates the submodule initialization, dependency setups, and artifact uploads seamlessly. One note: in step at line 18, the checkout action (actions/checkout@v3) is used—static analysis suggests updating it to a newer version if available for improved reliability.🧰 Tools
🪛 actionlint (1.7.4)
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
22-26: Ohayo sensei, please double-check the submodule initialization logic.
The submodule is force-added usinggit submodule add -f -b main ...which works if the submodule isn’t already present, but in CI environments where caching might occur, consider if usinggit submodule update --init --recursiveor ensuring idempotency would be more robust.
42-49: Ohayo sensei, nice integration updating the build job dependency!
The update to includebuild-explorerin theneedsclause ensures proper job sequencing. However, note that the checkout step in this job (line 46) still relies onactions/checkout@v3; updating it to a newer version is recommended per static analysis hints.🧰 Tools
🪛 actionlint (1.7.4)
46-46: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
46-46: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
14-39: Explorer Build Job – Validate Submodule Initialization & Checkout Version
Ohayo, sensei! The newbuild-explorerjob is a neat addition and its steps are clearly laid out. A couple of suggestions:
- Submodule Initialization: Using
git submodule add -f -b main ...(line 24) may cause issues when the submodule already exists, leading to non-idempotent behavior. Consider replacing it with a command like:This change would make the submodule initialization more robust in repeated CI runs.- git submodule add -f -b main https://github.com/cartridge-gg/explorer crates/katana/explorer/ui + git submodule update --init --recursive crates/katana/explorer/ui- Checkout Action Version: The checkout step on line 18 uses
actions/checkout@v3. Static analysis hints suggest this version may be outdated. Please consider upgrading to a newer version if available.🧰 Tools
🪛 actionlint (1.7.4)
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
47-49: Modernize Checkout Action in Build Job
Ohayo, sensehi! In thebuildjob, the checkout step (lines 47–49) properly includes submodules and a full fetch depth. However, similar to the earlier note, the use ofactions/checkout@v3has been flagged by static analysis. Consider updating to the latest stable version to benefit from performance improvements and enhanced security.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
46-46: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
42-42: Build Job Dependency Update
Ohayo, sensehi! The update to includebuild-explorerin theneedslist for thebuildjob (line 42) ensures that the Explorer UI is built prior to starting the main build process. This dependency ordering is well thought out.
67-67: Test Job Dependency Extended
Ohayo, sensehi! Updating thetestjob’s dependencies to include[ensure-docker, build-explorer](line 67) is a smart move. This ensures that the tests only run once the Explorer UI has been built and other necessary components are ready.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
.github/workflows/ci.yml (5)
14-39: Ohayo sensei! New Build-Explorer Job Integration:
The newbuild-explorerjob is a solid addition that encapsulates the Explorer UI build process nicely. The steps for checking out the repository (with recursive submodules), initializing submodules, setting up Bun, and building/uploading the Explorer UI assets are well structured.Action Item: Consider updating the checkout action on line 18 from
actions/checkout@v3to a more recent version (e.g., v4) as suggested by static analysis for improved reliability.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
42-49: Ohayo sensei! Build Job Dependency Update:
Thebuildjob now properly waits for the Explorer build by includingbuild-explorerin itsneedslist (line 42). This helps ensure that the Explorer assets are available for the build process. Also, note the checkout step on line 46—updating it to a newer version would maintain consistency across jobs.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
46-46: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
67-84: Ohayo sensei! Test Job Enhancements:
Thetestjob now includesbuild-explorerin its dependency list (line 67) and also downloads theexplorer-distartifact (lines 78–84). This ensures that tests relying on the Explorer UI assets get executed only after a successful Explorer build. Great job keeping the CI dependencies robust!🧰 Tools
🪛 actionlint (1.7.4)
68-68: label "ubuntu-latest-32-cores" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
72-72: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
101-113: Ohayo sensei! Ensure-WASM Job Update:
Integrating the Explorer build dependency in theensure-wasmjob (line 101) and downloading the Explorer UI artifact (lines 107–113) improves the consistency of your WASM build pipeline.Action Item: Updating the checkout step on line 105 from
actions/checkout@v3to a newer version would align this job with the other jobs.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
105-105: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
119-137: Ohayo sensei! Ensure-Windows Job Adjustments:
Theensure-windowsjob now depends on bothensure-dockerandbuild-explorer(line 119) and properly downloads the Explorer UI assets (lines 131–137), which is a great step for Windows compatibility.Action Items:
- Update the checkout action at line 122 from
actions/checkout@v3to a newer version for consistency.- Consider upgrading the toolchain setup at line 126 from
actions-rs/toolchain@v1to a later version (e.g., v2) if available to keep pace with the latest improvements.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - - uses: actions-rs/toolchain@v1 + - uses: actions-rs/toolchain@v2🧰 Tools
🪛 actionlint (1.7.4)
122-122: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
126-126: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(6 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
46-46: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
105-105: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
122-122: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
126-126: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build
- GitHub Check: clippy
- GitHub Check: ensure-wasm
- GitHub Check: docs
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
221-229: Ohayo sensei! Clippy Job Dependency Update:
The update to includebuild-explorerin theneedslist for theclippyjob (line 222) ensures that linting doesn’t start until after the Explorer build has completed. This enhancement strengthens the CI dependency chain.🧰 Tools
🪛 actionlint (1.7.4)
226-226: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
239-249: Ohayo sensei! Docs Job Dependency Enhancement:
The modification in thedocsjob to wait forbuild-explorer(line 241) further ensures that the documentation reflects the most recent Explorer build outputs. This is a thoughtful integration.🧰 Tools
🪛 actionlint (1.7.4)
245-245: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2997 +/- ##
==========================================
- Coverage 57.54% 57.24% -0.31%
==========================================
Files 439 441 +2
Lines 59776 60135 +359
==========================================
+ Hits 34399 34422 +23
- Misses 25377 25713 +336 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
14-39: Ohayo, sensei! Build-Explorer Job Integration.
The newbuild-explorerjob is thoughtfully constructed. It checks out the repository with recursive submodules, initializes the explorer submodule, sets up Bun, builds the Explorer UI, and uploads the resulting distribution as an artifact. This effectively integrates the explorer feature into the CI process.Suggestions:
- Consider updating
actions/checkout@v3toactions/checkout@v4(or a later stable version) to improve reliability and stay current with GitHub Actions best practices.- Verify that the
git submodule add -f -b main ...command is idempotent if the job might run multiple times, to avoid re-adding an already initialized submodule.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
42-49: Ohayo, sensei! Build Job Update.
Thebuildjob now properly depends onbuild-explorer, ensuring that the explorer UI is built before the main build proceeds. The checkout step is correctly modified to include recursive submodules, ensuring all required code (including submodules) is available.Suggestion:
- As with the previous step, update
actions/checkout@v3to a newer version to address static analysis recommendations.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
46-46: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(7 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
18-18: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
46-46: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
105-105: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
223-223: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
249-249: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: build
🔇 Additional comments (5)
.github/workflows/ci.yml (5)
67-82: Ohayo, sensei! Test Job Enhancement.
The test job now includesbuild-explorerin its dependencies and downloads theexplorer-distartifact prior to execution. This ensures that the explorer UI assets are available during testing, which should improve the reliability of tests that depend on integrated explorer functionality.🧰 Tools
🪛 actionlint (1.7.4)
68-68: label "ubuntu-latest-32-cores" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
72-72: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
100-116: Ohayo, sensei! Ensure-WASM Job Improvement.
In theensure-wasmjob, addingbuild-exploreras a dependency and downloading theexplorer-distartifact ensures that the explorer UI assets are integrated into the WASM build workflow. The verification step that lists thedistdirectory helps catch missing assets early.🧰 Tools
🪛 actionlint (1.7.4)
105-105: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
118-135: Ohayo, sensei! Ensure-Windows Job Update.
Theensure-windowsjob now incorporatesbuild-explorerin its dependency chain and downloads the explorer UI artifact using Windows directory commands. This integration appears seamless and should ensure that Windows builds have access to the updated explorer assets.🧰 Tools
🪛 actionlint (1.7.4)
122-122: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
123-123: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
218-232: Ohayo, sensei! Clippy Job Integration.
Theclippyjob has been updated to depend onbuild-explorerand now downloads theexplorer-distartifact. This guarantees that linting is performed on a build that includes all necessary explorer UI assets.Suggestion:
- Update the checkout action from
actions/checkout@v3toactions/checkout@v4for consistency with current best practices and to satisfy static analysis hints.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
223-223: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
243-259: Ohayo, sensei! Docs Job Enhancement.
Thedocsjob now awaits the completion ofbuild-explorerand downloads the explorer UI artifact to verify its presence before building the docs. This ensures that documentation accurately reflects the latest explorer UI changes.Suggestion:
- As with other jobs, update
actions/checkout@v3to a more recent version to improve stability and align with best practices.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
249-249: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
Description
This PR aims to add a explorer in the dojo toolchain by spawning a vite react project on spinning up a katana instance
Related issue
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
Documentation
Chores
.gitignoreto exclude the explorer UI directory from version control.