Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @miguelsolorio, 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 refines the default text styling within the CLI's user interface. It addresses specific text elements that were not fully covered in a previous semantic tokens refactor, primarily impacting the appearance of markdown text and foreground colors in syntax highlighting. The goal is to ensure a more consistent and visually appealing presentation across the application.
Highlights
- Consistent Text Styling: Ensures that various text elements, particularly within markdown displays and tool messages, correctly inherit or apply the primary theme text color.
- Improved Code Highlighting Fallback: Modifies the code colorizer to use a default theme color when no specific inherited color is provided, preventing unstyled text.
- Markdown List Item Styling: Applies the primary text color to list item prefixes and content within markdown rendering, enhancing readability and consistency.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to fix text styling by applying semantic theme colors. The changes are generally in the right direction. However, I've identified a recurring potential issue across multiple files where the theme color being applied can be an empty string under certain configurations. This could lead to runtime errors or rendering bugs in the UI. My review includes suggestions to make these color applications more robust by handling this edge case.
| <Text wrap="wrap" color={theme.text.primary}> | ||
| {resultDisplay} | ||
| </Text> |
There was a problem hiding this comment.
This change correctly applies a theme color, but theme.text.primary could be an empty string ('') depending on the theme configuration. Passing an empty string as a color to Ink's <Text> component can cause runtime errors or rendering issues.
To make this safer, we should fall back to undefined if theme.text.primary is an empty string, which lets Ink use its default text color.
| <Text wrap="wrap" color={theme.text.primary}> | |
| {resultDisplay} | |
| </Text> | |
| <Text wrap="wrap" color={theme.text.primary || undefined}> | |
| {resultDisplay} | |
| </Text> |
| const color = inheritedColor || theme.defaultColor; | ||
| return <Text color={color}>{node.value}</Text>; |
There was a problem hiding this comment.
This change correctly adds a fallback to the theme's default color. However, theme.defaultColor can be an empty string ('') for certain theme configurations (e.g., if a custom theme doesn't specify a foreground color). Passing an empty string to the color prop of Ink's <Text> component might cause a runtime error or lead to unexpected rendering behavior.
To make this more robust, we should ensure we don't pass an empty string by converting any falsy color value to undefined. This allows Ink to use its default color, which was the previous behavior for uncolored text.
| const color = inheritedColor || theme.defaultColor; | |
| return <Text color={color}>{node.value}</Text>; | |
| const color = inheritedColor || theme.defaultColor; | |
| return <Text color={color || undefined}>{node.value}</Text>; |
| > | ||
| <Box width={prefixWidth}> | ||
| <Text>{prefix}</Text> | ||
| <Text color={theme.text.primary}>{prefix}</Text> |
There was a problem hiding this comment.
Using theme.text.primary directly as a color can be problematic. If the active theme doesn't define a primary text color, this value could be an empty string, which may cause rendering issues or a crash in Ink. It's safer to fall back to undefined if the color is an empty string, allowing Ink to use its default.
| <Text color={theme.text.primary}>{prefix}</Text> | |
| <Text color={theme.text.primary || undefined}>{prefix}</Text> |
| </Box> | ||
| <Box flexGrow={LIST_ITEM_TEXT_FLEX_GROW}> | ||
| <Text wrap="wrap"> | ||
| <Text wrap="wrap" color={theme.text.primary}> |
There was a problem hiding this comment.
Similar to the list prefix, using theme.text.primary here could be an empty string for some theme configurations, which can lead to errors when used as a color value. To avoid this, we should ensure that an empty string is converted to undefined before being passed to the <Text> component.
| <Text wrap="wrap" color={theme.text.primary}> | |
| <Text wrap="wrap" color={theme.text.primary || undefined}> |
|
Size Change: +255 B (0%) Total Size: 13.2 MB ℹ️ View Unchanged
|
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |

TLDR
This is a minor text style updates to some elements that didn't get caught in the original semantic tokens refactor. Mostly happens in markdown text and foreground colors in syntax highlighting.
Dive Deeper
Before
After
Reviewer Test Plan
Testing Matrix
Linked issues / bugs