Conversation
|
Hi @jacob314, thank you so much for your contribution to Gemini CLI! We really appreciate the time and effort you've put into this. We're making some updates to our contribution process to improve how we track and review changes. Please take a moment to review our recent discussion post: Improving Our Contribution Process & Introducing New Guidelines. Key Update: Starting January 26, 2026, the Gemini CLI project will require all pull requests to be associated with an existing issue. Any pull requests not linked to an issue by that date will be automatically closed. Thank you for your understanding and for being a part of our community! |
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 focuses on enhancing the user experience of the application's footer. It addresses issues with how footer items are displayed, particularly in constrained terminal environments, by implementing a more sophisticated truncation algorithm. Additionally, the footer configuration dialog has undergone a substantial refactor, making it more intuitive and flexible for users to customize their footer layout and visibility preferences. Highlights
Changelog
Activity
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 significant and welcome refactoring of the footer and its configuration dialog. The footer's layout logic has been improved to use flexbox, fixing a regression where high-priority items could be truncated on narrow terminals. The footer configuration dialog has been completely rewritten to use a reusable BaseSelectionList component, which greatly improves its structure, maintainability, and responsiveness to terminal height. The tests have also been updated and expanded with SVG snapshots, increasing confidence in the UI changes.
I have one piece of feedback regarding a potentially incorrect magic number used for layout calculations in the FooterConfigDialog.
| : Number.MAX_SAFE_INTEGER; | ||
|
|
||
| const BORDER_HEIGHT = 2; // Outer round border | ||
| const STATIC_ELEMENTS = 13; // Text, margins, preview box, dialog footer |
There was a problem hiding this comment.
The value 13 for STATIC_ELEMENTS is a magic number used for layout calculation. As per repository guidelines, magic numbers, especially those for layout or padding, should be replaced with named constants to improve readability and maintainability. This makes the intent clearer and simplifies future updates.
Furthermore, a manual count of the static lines in the component's layout suggests the value should be 12.
Here's a breakdown of the static lines:
- Title (
<Text bold>with\n): 2 lines - Subtitle (
<Text color...>): 1 line - List
marginTop: 1 line DialogFooter(marginTop+ text): 2 lines- Preview
marginTop: 1 line - Preview
borderStyle: 2 lines - Preview title (
<Text bold>): 1 line - Preview content (
FooterRow): 2 lines
Total: 2 + 1 + 1 + 2 + 1 + 2 + 1 + 2 = 12 lines.
Using an incorrect value can lead to miscalculation of the available space for the list, potentially causing rendering issues on terminals with limited height. While the current value 13 is safer (allocating less space than available), it's still a bug and a maintainability concern.
Please define STATIC_ELEMENTS as a named constant with the correct value (12), or provide a detailed explanation if 13 is indeed correct and my calculation is missing something.
References
- Magic numbers, especially those used for layout or padding, should be replaced with named constants to improve readability and maintainability. This makes the intent clearer and simplifies future updates.
|
Size Change: -724 B (0%) Total Size: 26 MB ℹ️ View Unchanged
|
gemini-cli
bot
added
area/core
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
I've reviewed the PR and found two minor issues:
1. The "Odd Places" for Dots Issue (Footer.tsx)
In Footer.tsx, you map columns to rowItems and apply:
flexGrow: isWorkspace ? 1 : 0,
flexShrink: isWorkspace ? 1 : 0,However, the CwdIndicator uses shortenPath() to manually truncate the text to the estimatedWidth. This means the <Text> inside the workspace column is already the correct, constrained length.
Because the wrapping <Box> has flexGrow: 1, it expands to fill any remaining horizontal space in the terminal. When showLabels is false, Ink renders the workspace text, then leaves a large gap of empty space inside that Box before moving on to the dot separator (which is rendered in the next sibling Box). This causes the dot to float far to the right, right next to the next item. Additionally, if the workspace is focused, the theme.background.focus will awkwardly stretch across that empty gap.
Recommendation: Change flexGrow to 0 for the workspace item. The Box will then fit the text tightly, and the dot separators will render directly adjacent to it as expected.
2. useKeypress Re-subscriptions (FooterConfigDialog.tsx)
The useKeypress hook is passed an inline arrow function:
useKeypress(
(key: Key) => {
// ... uses focusKey, orderedIds, dispatch ...
},
{ isActive: true, priority: true }
);Since this inline function is re-created on every render, and useKeypress uses this handler in its useEffect dependency array, it will unsubscribe and re-subscribe to stdin events on every single render.
Recommendation: Wrap the handler in a useCallback with the appropriate dependencies (focusKey, orderedIds, dispatch) to avoid unnecessary re-subscriptions to stdin events.
|
Review completed. Two comments added regarding flex properties and useKeypress hook. |
jackwotherspoon
left a comment
jackwotherspoon
left a comment
There was a problem hiding this comment.
UX team had agreed on left to right footer items... why are we changing that here?
Also from your image it looks like we are now using headers as the list items instead of item ids...
It doesn't make sense to use headers because /stats is not a good representation in the dialog, it should remain usage in my opinion.
Also now each item is taking up two lines header and description underneath instead of previously each taking 1 with description on same line. Was this intentional to keep things more narrow but long?
Is this dialog now going to get truncated and require expansion with Ctrl+O?
Thanks for the review! Fixed the regression in the titles shown in the footer dialog. Left the items as 2 lines. Offline discussion: a single line doesn't meet the 80 char limit. We can make useSelectionList generically support a mode where summary and details are split across multiple lines if we want but should be a follow up pr. |
jacob314
force-pushed
the
status_line_review
branch
2 times, most recently
from
d26c0fa to
55c48b2
Compare
jacob314
force-pushed
the
status_line_review
branch
from
55c48b2 to
c88a162
Compare
|
Update |
3 participants
Summary
Code review comments as a pr.
Fixes #21208
Updated footer dialog aligned with other recent style changes:
Questionable UI left. The single line footer version puts the dots in slightly odd places. Not sure exactly how we'd want to render this.