fix(cli): add missing custom theme properties to settings schema#25729
fix(cli): add missing custom theme properties to settings schema#25729NTaylorMullen wants to merge 1 commit intomainfrom
Conversation
- Add 'text.response', 'ui.active', and 'ui.focus' to CustomTheme schema. - Synchronize 'CustomTheme' interface in core with schema by adding 'border.focused'. - Add validation tests for the new properties. Fixes #25689
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 addresses a validation issue where specific custom theme properties were being flagged as unrecognized by the CLI. By updating the settings schema and synchronizing the internal interface definitions, the changes ensure that users can fully utilize custom theme configurations without encountering validation errors. A new test case was added to confirm these properties are handled correctly. 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. Footnotes
|
|
Size Change: +111 B (0%) Total Size: 33.7 MB
ℹ️ View Unchanged
|
![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 updates the theme configuration schema and interfaces to support additional properties, including a 'response' field for text, 'active' and 'focus' fields for UI elements, and a 'focused' border style. Corresponding validation tests were added to ensure these new fields are correctly handled. I have no feedback to provide.
| }; | ||
| border?: { | ||
| default?: string; | ||
| focused?: string; |
There was a problem hiding this comment.
In packages/cli/src/ui/themes/theme.ts, the createCustomTheme function does not currently read the border.focused property. This means that even if a user defines a focused border color in their settings, it will be ignored and fall back to the default green.
// packages/cli/src/ui/themes/theme.ts
const colors: ColorsTheme = {
...
FocusColor:
customTheme.ui?.focus ??
customTheme.border?.focused ?? // This needs to be added 💚
customTheme.AccentGreen,
...
//
Thanks for improving the custom theme integration 💚
| focus: '#00ff00', | ||
| gradient: ['#000000', '#ffffff'], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Maybe we can add the border field to improve the test 👍 ?
border: {
default: '#313244',
focused: '#89b4fa',
},
nidhishgajjar
left a comment
nidhishgajjar
left a comment
There was a problem hiding this comment.
This is a clean, focused bug fix that addresses a validation error with custom theme properties. The changes are well-structured and properly tested.
What this PR fixes:
- Users were getting validation errors when using
text.response,ui.active, andui.focusproperties in custom theme configurations - There was a mismatch between the settings schema and the CustomTheme interface
Code quality:
- Minimal, focused changes (34 additions, 0 deletions, 3 files)
- Properly synchronizes the settings schema with the CustomTheme interface
- Adds comprehensive validation test case covering all the new properties
- Clean diff that clearly shows the missing properties being added
Testing:
- Added test case that validates a complex nested customThemes configuration
- Test covers all the newly added properties (text.response, ui.active, ui.focus)
- Manual validation steps are clearly documented in the PR description
Documentation:
- Excellent PR description with clear summary, details, and validation steps
- Properly references the related issue (#25689)
- Pre-merge checklist is complete
Approval: This looks good to merge. The fix is targeted, well-implemented, properly tested, and thoroughly documented.
3 participants
Summary
This PR fixes a validation error when using certain properties in a custom theme configuration. Specifically, it adds support for
text.response,ui.active, andui.focusin the settings schema.Details
text.response,ui.active,ui.focus) to theCustomThemedefinition inpackages/cli/src/config/settingsSchema.ts.CustomThemeinterface inpackages/core/src/config/config.tsby addingborder.focused(which was already in the schema but missing from the interface).packages/cli/src/config/settings-validation.test.tsto verify that these properties are now correctly accepted.Related Issues
Fixes #25689
How to Validate
npm test -w @google/gemini-cli -- src/config/settings-validation.test.tsPre-Merge Checklist