fix: Restore footer configuration settings (#8041)#8053
fix: Restore footer configuration settings (#8041)#8053miguelsolorio merged 20 commits intogoogle-gemini:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Lyonk71, 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 addresses a regression introduced after the AppContainer refactor, restoring the functionality of footer configuration settings. Users can now correctly hide specific elements within the CLI's footer, such as the current working directory, sandbox status, and model information, via the settings dialog.
Highlights
- Footer Settings Restoration: Fixed a regression that prevented footer configuration settings from working, allowing users to hide specific footer elements.
- UI Component Updates: The Composer component now passes hideCWD, hideSandboxStatus, and hideModelInfo settings to the Footer component.
- Conditional Rendering: The Footer component has been updated to conditionally render the Current Working Directory (CWD), Sandbox Status, and Model Info based on the new hide props.
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 correctly restores the functionality to hide individual elements in the footer via settings. The changes involve passing the relevant settings as props from the Composer component to the Footer component and then using those props to conditionally render the UI elements. The implementation is straightforward and correct. I have one suggestion to improve maintainability in Composer.tsx by reducing repetitive code.
There was a problem hiding this comment.
These lines repeatedly access the nested settings.merged.ui?.footer object. This violates the DRY (Don't Repeat Yourself) principle and creates a maintenance concern. If the path to the footer settings were to change, it would require updates in multiple places, increasing the risk of introducing inconsistencies or bugs. It would be more robust to retrieve the footer object once into a local variable before defining footerProps.
For example:
const footerSettings = settings.merged.ui?.footer;
const footerProps: Omit<FooterProps, 'vimMode'> = {
// ... other props
hideCWD: footerSettings?.hideCWD || false,
hideSandboxStatus: footerSettings?.hideSandboxStatus || false,
hideModelInfo: footerSettings?.hideModelInfo || false,
};
|
Thanks for taking a look at this! It looks like we're missing the alignment tweaks that allowed the footer items to flex around depending on which ones we're enabled/disabled. Do you mind adding those? |
|
Thanks for pointing this out, @miguelsolorio! You're absolutely right. The previous changes to conditionally hide the CWD and Model Info were only hiding the content within their respective To address this, I've updated the Here are the proposed changes: --- a/packages/cli/src/ui/components/Footer.tsx
+++ b/packages/cli/src/ui/components/Footer.tsx
@@ -76,22 +76,24 @@ export const Footer: React.FC<FooterProps> = ({
</Box>
{/* Left Section: CWD and Debug Info */}
- <Box>
- {debugMode && <DebugProfiler />}
- {vimMode && <Text color={theme.text.secondary}>[{vimMode}] </Text>}
- {!hideCWD && (
- nightly ? (
- <Gradient colors={theme.ui.gradient}>
- <Text>
- {displayPath}
- {branchName && <Text> ({branchName}*)</Text>}
- </Text>
- </Gradient>
- ) : (
- <Text color={theme.text.link}>
- {displayPath}
- {branchName && (
- <Text color={theme.text.secondary}> ({branchName}*)</Text>
- )}
- </Text>
- )
- )}
- {debugMode && (
- <Text color={theme.status.error}>
+ {(debugMode || vimMode || !hideCWD) && (
+ <Box>
+ {debugMode && <DebugProfiler />}
+ {vimMode && <Text color={theme.text.secondary}>[{vimMode}] </Text>}
+ {!hideCWD && (
+ nightly ? (
+ <Gradient colors={theme.ui.gradient}>
+ <Text>
+ {displayPath}
+ {branchName && <Text> ({branchName}*)</Text>}
+ </Text>
+ </Gradient>
+ ) : (
+ <Text color={theme.text.link}>
+ {displayPath}
+ {branchName && (
+ <Text color={theme.text.secondary}> ({branchName}*)</Text>
+ )}
+ </Text>
+ )
+ )}
+ {debugMode && (
+ <Text color={theme.status.error}>
+ )}
+ </Box>
+ )}
{/* Middle Section: Centered Trust/Sandbox Info */}
{!hideSandboxStatus && (
@@ -136,17 +138,22 @@ export const Footer: React.FC<FooterProps> = ({
)}
{/* Right Section: Gemini Label and Console Summary */}
- <Box alignItems="center" paddingTop={isNarrow ? 1 : 0}>
- {!hideModelInfo && (
- <Box alignItems="center">
- <Text color={theme.text.accent}>
- {isNarrow ? '' : ' '}
- {model}{' '}
- <ContextUsageDisplay
- promptTokenCount={promptTokenCount}
- model={model}
- />
- </Text>
- {showMemoryUsage && <MemoryUsageDisplay />}
- </Box>
- )}
- <Box alignItems="center" paddingLeft={2}>
- {corgiMode && (
- <Text>
+ {(!hideModelInfo || corgiMode) && (
+ <Box alignItems="center" paddingTop={isNarrow ? 1 : 0}>
+ {!hideModelInfo && (
+ <Box alignItems="center">
+ <Text color={theme.text.accent}>
+ {isNarrow ? '' : ' '}
+ {model}{' '}
+ <ContextUsageDisplay
+ promptTokenCount={promptTokenCount}
+ model={model}
+ />
+ </Text>
+ {showMemoryUsage && <MemoryUsageDisplay />}
+ </Box>
+ )}
+ <Box alignItems="center" paddingLeft={2}>
+ {corgiMode && (
+ <Text>
+ )}
+ </Box>
+ </Box>
+ )}Let me know if this looks good! |
The footer configuration settings (hideCWD, hideSandboxStatus, hideModelInfo) were lost during the AppContainer refactor. This commit reconnects the existing settings to the Footer component to restore the ability to hide individual footer elements.
Lyonk71
force-pushed
the
fix/8041-footer-config-regression
branch
from
4e5e6e7 to
0a36530
Compare
|
Ahh I see that alignment work now. Originally, I just wired up the missing props from the post refactor. Let me know if you need anything else! |
miguelsolorio
left a comment
miguelsolorio
left a comment
There was a problem hiding this comment.
Thanks for making the updates, this fixes it for me! 🙏
|
One more item to add, there were some alignment tweaks to App.tsx for when context summary is hidden https://github.com/google-gemini/gemini-cli/pull/7419/files#diff-285f405b51ffd9d3a1ebad733da12635608980d6540626a7885fe8aa2fe767d8R1254-R1256 |
The footer configuration settings (hideCWD, hideSandboxStatus, hideModelInfo) were lost during the AppContainer refactor. This commit reconnects the existing settings to the Footer component to restore the ability to hide individual footer elements.
Lyonk71
force-pushed
the
fix/8041-footer-config-regression
branch
from
9162085 to
c52fc84
Compare
|
Ahh yes, I remember moving those alignment tweaks from App.tsx to Composer as part of the App.tsx cleanup effort. Post refactor Composer has a top margin of 1 to maintain spacing above the prompt. The functionality missing from the refactor was the right justification for actions like prompt when context summary is hidden. That's just a 1 line fix in the commit I just pushed. The implementation is a bit different, but I think easier to understand/maintain. Let me know if this works for you! |
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
Approving to unblock. Can we add a test to catch if we break this filtering in the future. Can be a golden snapshot test. That will ensure we don't accidentally regress this functionality again in the future.
Head branch was pushed to by a user without write access
…le-gemini#8053) Co-authored-by: Miguel Solorio <miguelsolorio@google.com> Co-authored-by: Miguel Solorio <miguel.solorio07@gmail.com>
3 participants
Summary
This PR fixes a regression where footer configuration settings stopped working after the AppContainer refactor. Users can now properly hide individual footer elements (CWD, sandbox status, model info) via the settings dialog.
Reviewer Test Plan
Testing Matrix
Linked issues / bugs
Fixes #8041