Repository navigation
feat(setup): display ASCII art banner during onboarding - #851
Conversation
[skip-regression-check]
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 enhances the initial user experience of 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
|
There was a problem hiding this comment.
Code Review
This pull request adds a nice ASCII art banner to the onboarding wizard, which improves the first-run experience. The implementation is straightforward. I've added one comment regarding panic safety in the new print_banner function to ensure the terminal color is always reset, even in case of an unexpected panic during printing.
Note: Security Review did not run due to the size of the PR.
| /// Print the IronClaw ASCII art banner in blue. | ||
| pub fn print_banner() { | ||
| let mut stdout = io::stdout(); | ||
| let _ = execute!(stdout, SetForegroundColor(Color::Cyan)); | ||
| println!(); | ||
| println!(r" ██╗██████╗ ██████╗ ███╗ ██╗ ██████╗██╗ █████╗ ██╗ ██╗"); | ||
| println!(r" ██║██╔══██╗██╔═══██╗████╗ ██║██╔════╝██║ ██╔══██╗██║ ██║"); | ||
| println!(r" ██║██████╔╝██║ ██║██╔██╗ ██║██║ ██║ ███████║██║ █╗ ██║"); | ||
| println!(r" ██║██╔══██╗██║ ██║██║╚██╗██║██║ ██║ ██╔══██║██║███╗██║"); | ||
| println!(r" ██║██║ ██║╚██████╔╝██║ ╚████║╚██████╗███████╗██║ ██║╚███╔███╔╝"); | ||
| println!(r" ╚═╝╚═╝ ╚═╝ ╚═════╝ ╚═╝ ╚═══╝ ╚═════╝╚══════╝╚═╝ ╚═╝ ╚══╝╚══╝ "); | ||
| let _ = execute!(stdout, ResetColor); | ||
| } |
There was a problem hiding this comment.
This implementation isn't fully panic-safe. If any of the println! macros were to panic (e.g., due to a broken pipe when writing to stdout), the ResetColor command at the end would not be executed, leaving the user's terminal in a colored state.
Additionally, the doc comment says the banner is 'blue', but the code uses Color::Cyan.
To improve robustness and fix the documentation, I suggest using a simple RAII guard to ensure the color is always reset. This is a common pattern in Rust for cleanup logic.
/// Print the IronClaw ASCII art banner in cyan.
pub fn print_banner() {
struct ResetGuard;
impl Drop for ResetGuard {
fn drop(&mut self) {
// Best-effort attempt to reset terminal color.
let _ = execute!(io::stdout(), ResetColor);
}
}
let mut stdout = io::stdout();
// Setting color can fail, but we'll try to reset anyway.
let _ = execute!(stdout, SetForegroundColor(Color::Cyan));
let _guard = ResetGuard;
println!();
println!(r" ██╗██████╗ ██████╗ ███╗ ██╗ ██████╗██╗ █████╗ ██╗ ██╗");
println!(r" ██║██╔══██╗██╔═══██╗████╗ ██║██╔════╝██║ ██╔══██╗██║ ██║");
println!(r" ██║██████╔╝██║ ██║██╔██╗ ██║██║ ██║ ███████║██║ █╗ ██║");
println!(r" ██║██╔══██╗██║ ██║██║╚██╗██║██║ ██║ ██╔══██║██║███╗██║");
println!(r" ██║██║ ██║╚██████╔╝██║ ╚██████║╚██████╗███████╗██║ ██║╚███╔███╔╝");
println!(r" ╚═╝╚═╝ ╚═╝ ╚═════╝ ╚═╝ ╚═══╝ ╚═════╝╚══════╝╚═╝ ╚═╝ ╚══╝╚══╝ ");
}
zmanian
left a comment
There was a problem hiding this comment.
Looks good -- small, focused change that adds visual polish to the onboarding flow.
What I checked:
SetForegroundColor,Color::Cyan,ResetColor, andexecute!are already imported inprompts.rs, so no new dependencies or imports needed in that file.- The
let _ =pattern for ignoring crossterm errors is consistent withprint_success,print_error,print_info, and other styled output helpers in the same file. - Banner placement (
print_banner()right beforeprint_header("IronClaw Setup Wizard")) reads well sinceprint_bannerstarts withprintln!()(blank line above) andprint_headeralso starts withprintln!()(blank line between banner and header box). - The
[skip-regression-check]tag is appropriate -- this is a cosmetic-only change with no behavioral logic to regress.
Minor nits (non-blocking):
- The last line of the ASCII art has a trailing space after the closing
");. Not a functional issue, but worth trimming for tidiness. - There is no
println!()afterResetColor, so the color reset and the subsequentprint_headerblank line run together. This works fine visually today becauseprint_headeropens with its ownprintln!(), but adding one after the reset would makeprint_bannerself-contained (no dependency on what follows it).
Neither nit warrants blocking. Approved.
|
Is this ready to merge, or does the non-blocking issue need to be resolved first? |
[skip-regression-check]
[skip-regression-check]
Adds an IronClaw ASCII art banner at the start of
ironclaw onboard,giving the setup wizard a more polished and recognizable first impression.
A strong visual identity on first run helps new users feel confident
they are in the right place and sets IronClaw apart from generic CLI tools.