fix: slash command completion menu column width and spacing issues#5797
fix: slash command completion menu column width and spacing issues#5797mattKorwel merged 8 commits intogoogle-gemini:mainfrom
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @naaa760, 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!
I've addressed several layout and spacing issues within the slash command completion menu. The primary goal was to prevent command name truncation and improve the overall readability and efficiency of the menu's display. This was achieved by implementing dynamic width calculation for command names and adding proper spacing between command names and their descriptions, all while ensuring existing non-slash command completions remain unaffected.
Highlights
- Dynamic Command Width: I've implemented a dynamic width calculation for slash command names in the completion menu. This prevents long command names, like
cr:test:gen-browsertests, from being truncated by adapting the column width to fit the longest visible command. - Improved Spacing: I've added
marginLeft={1}between the command name and its description in the completion menu. This ensures clear visual separation and improves readability. - Efficient Terminal Usage: The completion menu now intelligently utilizes available terminal space. The dynamic width calculation considers the terminal's total width, ensuring the menu adapts effectively while maintaining readability.
- Maintained Compatibility: I've ensured that these changes specifically target slash command completions, preserving full compatibility and functionality for all other non-slash command completions (e.g., file paths,
@mentions).
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 or fill out our survey to provide feedback.
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 effectively addresses the layout issues with the slash command completion menu by introducing dynamic column widths. The logic for calculating the width is sound. I've identified a couple of areas for improvement in the implementation. First, there's a potential robustness issue with using Math.max on a possibly empty array, which can be made safer. Second, there's some code duplication in the rendering logic that can be refactored for better maintainability. Addressing these points will make the solution more robust and easier to maintain.
| const maxLabelLength = Math.max( | ||
| ...visibleSuggestions.map((s) => s.label.length), | ||
| ); |
There was a problem hiding this comment.
Using the spread operator on Math.max with a potentially empty visibleSuggestions array can lead to unexpected behavior. If visibleSuggestions is empty, Math.max(...[]) evaluates to -Infinity. While the subsequent logic happens to handle this by falling back to the minimum width, it's safer and clearer to explicitly handle the empty array case to improve code robustness.
const maxLabelLength = visibleSuggestions.length
? Math.max(...visibleSuggestions.map((s) => s.label.length))
: 0;There was a problem hiding this comment.
ignore. the selection array can't be empty
| {isSlashCommandMode ? ( | ||
| <> | ||
| <Box width={commandNameWidth} flexShrink={0}> | ||
| {labelElement} | ||
| </Box> | ||
| {suggestion.description ? ( | ||
| <Box flexGrow={1} marginLeft={1}> | ||
| <Text color={textColor} wrap="wrap"> | ||
| {suggestion.description} | ||
| </Text> | ||
| </Box> | ||
| ) : null} | ||
| </> | ||
| ) : ( | ||
| labelElement | ||
| <> | ||
| {labelElement} | ||
| {suggestion.description ? ( | ||
| <Box flexGrow={1} marginLeft={1}> | ||
| <Text color={textColor} wrap="wrap"> | ||
| {suggestion.description} | ||
| </Text> | ||
| </Box> | ||
| ) : null} | ||
| </> | ||
| )} |
There was a problem hiding this comment.
There's significant code duplication between the two branches of the ternary operator. The logic for displaying the suggestion description is identical in both cases. This can be refactored to remove the duplication, making the code more maintainable. The description rendering can be extracted and placed after the conditional rendering of the label.
{isSlashCommandMode ? (
<Box width={commandNameWidth} flexShrink={0}>
{labelElement}
</Box>
) : (
labelElement
)}
{suggestion.description ? (
<Box flexGrow={1} marginLeft={1}>
<Text color={textColor} wrap="wrap">
{suggestion.description}
</Text>
</Box>
) : null}There was a problem hiding this comment.
| {isSlashCommandMode ? ( | |
| <> | |
| <Box width={commandNameWidth} flexShrink={0}> | |
| {labelElement} | |
| </Box> | |
| {suggestion.description ? ( | |
| <Box flexGrow={1} marginLeft={1}> | |
| <Text color={textColor} wrap="wrap"> | |
| {suggestion.description} | |
| </Text> | |
| </Box> | |
| ) : null} | |
| </> | |
| ) : ( | |
| labelElement | |
| <> | |
| {labelElement} | |
| {suggestion.description ? ( | |
| <Box flexGrow={1} marginLeft={1}> | |
| <Text color={textColor} wrap="wrap"> | |
| {suggestion.description} | |
| </Text> | |
| </Box> | |
| ) : null} | |
| </> | |
| )} | |
| {isSlashCommandMode ? ( | |
| <Box width={commandNameWidth} flexShrink={0}> | |
| {labelElement} | |
| </Box> | |
| ) : ( | |
| labelElement | |
| )} | |
| {suggestion.description ? ( | |
| <Box flexGrow={1} marginLeft={1}> | |
| <Text color={textColor} wrap="wrap"> | |
| {suggestion.description} | |
| </Text> | |
| </Box> | |
| ) : null} | |
| )} |
There was a problem hiding this comment.
Adding in the change from the bot.
|
@jacob314 |
…5797) Co-authored-by: matt korwel <matt.korwel@gmail.com> Co-authored-by: Arya Gummadi <aryagummadi@google.com>
…5797) Co-authored-by: matt korwel <matt.korwel@gmail.com> Co-authored-by: Arya Gummadi <aryagummadi@google.com>
…oogle-gemini#5797) Co-authored-by: matt korwel <matt.korwel@gmail.com> Co-authored-by: Arya Gummadi <aryagummadi@google.com>
…oogle-gemini#5797) Co-authored-by: matt korwel <matt.korwel@gmail.com> Co-authored-by: Arya Gummadi <aryagummadi@google.com>
TLDR
cr:test:gen-browsertests.fixes: #5726
Dive Deeper
Solution implemented:
marginLeft={1}between columnsTechnical details:
SuggestionsDisplay.tsxto calculate optimal command name width based on visible suggestionsMath.max(...visibleSuggestions.map(s => s.label.length))to find longest commandMath.max(15, Math.min(maxLabelLength + 2, maxAllowedWidth))Reviewer Test Plan
npm run devor build and run the CLI/cr:and observe the completion menucr:test:gen-browsertestsshould display fullyGood test commands:
/cr:- Shows the problematic long command names from the original bug report/help- Simple command to verify basic functionality@or file paths - Ensures non-slash completions unchangedTesting Matrix