Add the build link bisect tool to the website#3902
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the developer experience by integrating web-based tools directly into the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 a new cargo run explore command and a web-based build bisect tool, which is a great addition for debugging. The implementation looks solid, with new Rust logic to open URLs, and a comprehensive JavaScript-powered interactive tool on the website. My review includes a couple of suggestions to improve the robustness of the new functionality: one for the URL opening logic in Rust to handle special characters correctly, and another for the JavaScript to perform more complete HTML escaping.
| pub fn open_url(url: &str) -> Result<(), Error> { | ||
| #[cfg(target_os = "windows")] | ||
| let command = format!("cmd /c start {url}"); | ||
| #[cfg(target_os = "macos")] | ||
| let command = format!("open {url}"); | ||
| #[cfg(not(any(target_os = "windows", target_os = "macos")))] | ||
| let command = format!("xdg-open {url}"); | ||
| run(&command) | ||
| } |
There was a problem hiding this comment.
The current implementation of open_url uses the run function, which splits the command string by whitespace. This is not robust for URLs, as they can contain special characters (like &) that are misinterpreted by the shell, or could in the future contain characters that cause incorrect splitting. A better approach is to construct the std::process::Command directly, passing the URL as a distinct argument. This ensures it's handled correctly across different platforms.
pub fn open_url(url: &str) -> Result<(), Error> {
let status = {
#[cfg(target_os = "windows")]
{
// The empty string argument is a workaround for `start` to correctly handle URLs with ampersands.
process::Command::new("cmd").args(["/C", "start", "", url]).status()
}
#[cfg(target_os = "macos")]
{
process::Command::new("open").arg(url).status()
}
#[cfg(not(any(target_os = "windows", target_os = "macos")))]
{
process::Command::new("xdg-open").arg(url).status()
}
}
.map_err(|e| Error::Io(e, format!("failed to run command to open url: {url}")))?;
if !status.success() {
return Err(Error::Command(format!("open url command for '{url}'"), status));
}
Ok(())
}|
|
||
| function messageToHtml(/** @type {string} */ message) { | ||
| if (!message) return ""; | ||
| const escaped = message.replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">"); |
There was a problem hiding this comment.
The HTML escaping is incomplete. It handles &, <, and > but misses other characters like single and double quotes. While this may not be a security issue here since the output is not used within an HTML attribute, it's less robust. A more complete escaping would prevent potential rendering issues if commit messages contain these characters.
| const escaped = message.replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">"); | |
| const escaped = message.replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">").replace(/"/g, """).replace(/'/g, "'"); |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="website/static/js/page/contributor-guide/bisect-tool.js">
<violation number="1" location="website/static/js/page/contributor-guide/bisect-tool.js:192">
P1: Paginate the initial `/commits` window. With only page 1 loaded here, dense date ranges can omit the requested hash or nearest commit and start the bisect from the wrong point.</violation>
<violation number="2" location="website/static/js/page/contributor-guide/bisect-tool.js:357">
P2: Handle boundary-search exhaustion explicitly. If the issue is still present on the oldest available commit, this path keeps re-testing commit 0 forever.</violation>
</file>
<file name="tools/cargo-run/src/lib.rs">
<violation number="1" location="tools/cargo-run/src/lib.rs:79">
P2: Pass the URL to `Command` as an argument instead of formatting a shell command string. The current helper loses argument boundaries via `split_whitespace()`, and `cmd /c start` will misparse common Windows URLs containing `&`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="website/static/js/page/contributor-guide/bisect-tool.js">
<violation number="1" location="website/static/js/page/contributor-guide/bisect-tool.js:378">
P1: This stop condition misreports the oldest available commit as the introducing commit when no known-good commit exists yet.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
1 participant
No description provided.