-
Notifications
You must be signed in to change notification settings - Fork 9.5k
feat: support custom ASCII art variants for header logo (TOML) #13706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @jduncan-rva, 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 introduces a significant customization feature for the CLI, allowing users to personalize the header's ASCII art logo. By providing a path to a TOML file, users can define multiple logo variants that adapt to different terminal sizes and environments, ensuring consistent branding and a responsive user experience. The implementation includes a new React hook for asynchronous loading, updates to core UI components for dynamic display, and schema modifications to integrate the new setting. Highlights
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a feature for customizing the CLI header's ASCII art logo using variants from a TOML file. The implementation is well-structured, adding a new setting, a React hook for loading the logo file, and updating relevant UI components. My review identified two high-severity issues. The first is an unsafe type assertion when parsing the user-provided TOML file, which could lead to runtime errors if the file is malformed. The second is a logic flaw in the fallback for IDE-specific logos, which could result in unexpected behavior. I have provided code suggestions to address both of these points to improve the feature's robustness and user experience.
| const longAsciiLogoIde = | ||
| customLogoVariants?.longAsciiLogoIde ?? defaultLongAsciiLogoIde; | ||
| const shortAsciiLogoIde = | ||
| customLogoVariants?.shortAsciiLogoIde ?? defaultShortAsciiLogoIde; | ||
| const tinyAsciiLogoIde = | ||
| customLogoVariants?.tinyAsciiLogoIde ?? defaultTinyAsciiLogoIde; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a potential improvement in the fallback logic for IDE-specific logos. Currently, if a user provides a custom longAsciiLogo but not a longAsciiLogoIde, the header will fall back to the defaultLongAsciiLogoIde when in an IDE environment. This might be unexpected for the user, who might assume their custom non-IDE logo would be used as a fallback.
Consider a more intuitive fallback chain: custom IDE variant -> custom non-IDE variant -> default IDE variant. This would provide a better user experience and more consistent branding for users who don't want to create separate IDE variants.
const longAsciiLogoIde =
customLogoVariants?.longAsciiLogoIde ??
customLogoVariants?.longAsciiLogo ??
defaultLongAsciiLogoIde;
const shortAsciiLogoIde =
customLogoVariants?.shortAsciiLogoIde ??
customLogoVariants?.shortAsciiLogo ??
defaultShortAsciiLogoIde;
const tinyAsciiLogoIde =
customLogoVariants?.tinyAsciiLogoIde ??
customLogoVariants?.tinyAsciiLogo ??
defaultTinyAsciiLogoIde;
| const parsed = toml.parse(content) as unknown as LogoVariants; | ||
| setVariants(parsed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type assertion as unknown as LogoVariants is unsafe and can lead to runtime errors if the TOML file contains non-string values for the logo variants. This can happen if a user provides a malformed file. It's crucial to validate the structure and types of the parsed TOML content before using it. This will make the feature more robust against invalid user input.
const parsed = toml.parse(content);
if (typeof parsed !== 'object' || parsed === null) {
throw new Error('TOML file content is not a valid object.');
}
// Ensure all logo variants are strings.
for (const [key, value] of Object.entries(parsed)) {
if (typeof value !== 'string') {
throw new Error(`Invalid type for logo variant "${key}": expected string, got ${typeof value}.`);
}
}
setVariants(parsed as LogoVariants);
Summary
This PR enables users to customize the ASCII art logo displayed in the CLI header by providing a path to a TOML file containing logo variants. This allows for consistent branding across different terminal sizes and environments (standard vs. IDE) while maintaining the CLI's responsive design.
A helper extension has been created to assist users in generating these TOML files from images: gemini-logos.
Details
ui.customLogoVariantsFile(string). This setting accepts an absolute path to a TOML file.useCustomLogowas added to asynchronously load and parse the TOML file specified in the settings using@iarna/toml.Header.tsxwas updated to accept acustomLogoVariantsprop.customLogoVariants.longAsciiLogo).AppContainer.tsxnow lifts the logo loading state to ensure the logo is ready beforeMainContent(specifically theStaticcomponent) renders it, preventing stale initial renders.settings.schema.jsonandsettingsSchema.tsto include the new setting.LogoVariantsinterface, supporting all 6 standard variants:Related Issues
N/A
How to Validate
Run the following command with a path to an image file:
logo-variants.tomlfile in your current directory.settings.json:/absolute/path/to/...with the actual path to the generated file)npm start.Pre-Merge Checklist