feat(themes): add themeable bgTitleBar token for the window title bar#1102
Conversation
The draggable title bar (App.tsx) rendered transparent and showed bgMain behind it, so themes had no way to color that top strip independently. Add an optional bgTitleBar color token: - New optional field on ThemeColors (falls back to bgMain when unset, so pre-existing saved/imported custom themes are unchanged). - Wire the title bar to bgTitleBar ?? bgMain. - Set bgTitleBar on all 20 built-in themes, each mirroring its own bgMain, so every existing theme looks pixel-identical. - Expose it as an editable field in the Custom Theme Builder, with a title-bar strip added to the live preview. - Add bgTitleBar to the REQUIRED_COLORS theme test.
📝 WalkthroughWalkthroughAdds an optional ChangesbgTitleBar theme color
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Companion RC PR: #1103 (same single commit cherry-picked onto |
Greptile SummaryAdds a
Confidence Score: 4/5Safe to merge — all built-in themes are updated, the runtime fallback is sound, and no existing theme changes appearance. The change is well-scoped and the fallback logic is correct. Two minor observations:
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Theme loaded] --> B{bgTitleBar set?}
B -- Yes --> C[Use bgTitleBar]
B -- No --> D[Fallback: bgMain]
C --> E[Apply to title bar div in App.tsx]
D --> E
E --> F[Render draggable title bar]
G[CustomThemeBuilder open] --> H{bgTitleBar in customThemeColors?}
H -- Yes --> I[Show stored value in ColorInput]
H -- No --> J[Display bgMain as visual placeholder]
J -.->|not persisted until user edits| K[(customThemeColors store)]
I --> K
K --> L[Export / save theme JSON]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Theme loaded] --> B{bgTitleBar set?}
B -- Yes --> C[Use bgTitleBar]
B -- No --> D[Fallback: bgMain]
C --> E[Apply to title bar div in App.tsx]
D --> E
E --> F[Render draggable title bar]
G[CustomThemeBuilder open] --> H{bgTitleBar in customThemeColors?}
H -- Yes --> I[Show stored value in ColorInput]
H -- No --> J[Display bgMain as visual placeholder]
J -.->|not persisted until user edits| K[(customThemeColors store)]
I --> K
K --> L[Export / save theme JSON]
Reviews (1): Last reviewed commit: "feat(themes): add themeable bgTitleBar t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 'bgMain', | ||
| 'bgSidebar', | ||
| 'bgActivity', | ||
| 'bgTitleBar', |
There was a problem hiding this comment.
bgTitleBar is declared bgTitleBar?: string in ThemeColors, but it is listed in REQUIRED_COLORS which the test asserts must be defined on every theme. The test passes today because all 20 built-in themes set the key explicitly, but a future contributor adding an optional UI-surface color may follow the same pattern and be surprised that their optional key must also appear in REQUIRED_COLORS. Consider either renaming the array (e.g. REQUIRED_BUILTIN_COLORS) to signal that this is a stricter contract than the TypeScript type, or adding a short comment explaining the intentional stronger invariant for built-in themes.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| value={ | ||
| customThemeColors[key] ?? (key === 'bgTitleBar' ? customThemeColors.bgMain : '') | ||
| } |
There was a problem hiding this comment.
customThemeColors.bgTitleBar is undefined (e.g. an imported pre-existing custom theme), the ColorInput displays bgMain's value as if the field is already populated. If the user inspects the color, reads the hex, and then exports the theme without touching bgTitleBar, the exported JSON will not contain bgTitleBar at all — the pre-filled value is purely cosmetic. This is silent but unlikely to cause data loss since the runtime fallback ?? bgMain recovers it correctly. A placeholder/hint approach (grey text, placeholder attribute) rather than a real color value would more honestly signal "not yet set".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/components/CustomThemeBuilder.tsx (1)
382-390:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImport validation breaks backward compatibility for old custom themes.
The validation requires all
COLOR_CONFIGkeys to be present, including the newly addedbgTitleBar. Old exported custom themes that don't havebgTitleBarwill fail import with a "missing color keys" error, contradicting the PR's stated goal of "ensuring backward compatibility with existing custom themes."🛡️ Proposed fix: Auto-fill missing bgTitleBar before validation
try { const data = JSON.parse(e.target?.result as string); if (data.colors && typeof data.colors === 'object') { + // Backward compatibility: auto-fill optional bgTitleBar with bgMain if missing + if (!data.colors.bgTitleBar && data.colors.bgMain) { + data.colors.bgTitleBar = data.colors.bgMain; + } + // Validate all required color keys exist const requiredKeys = COLOR_CONFIG.map((c) => c.key);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/components/CustomThemeBuilder.tsx` around lines 382 - 390, The validation logic in the theme import handler breaks backward compatibility by requiring all COLOR_CONFIG keys to be present, including newly added keys like bgTitleBar. Old exported themes missing this key will fail validation. Before performing the hasAllKeys validation check, add logic to auto-fill missing color keys from data.colors with their corresponding default values from COLOR_CONFIG. This way, old themes will have missing keys populated automatically before the validation occurs, allowing them to import successfully while maintaining the integrity check for genuinely invalid themes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/renderer/components/CustomThemeBuilder.tsx`:
- Around line 382-390: The validation logic in the theme import handler breaks
backward compatibility by requiring all COLOR_CONFIG keys to be present,
including newly added keys like bgTitleBar. Old exported themes missing this key
will fail validation. Before performing the hasAllKeys validation check, add
logic to auto-fill missing color keys from data.colors with their corresponding
default values from COLOR_CONFIG. This way, old themes will have missing keys
populated automatically before the validation occurs, allowing them to import
successfully while maintaining the integrity check for genuinely invalid themes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f5ce690-e5fa-4218-9ca2-d4b94e1023e3
📒 Files selected for processing (5)
src/__tests__/renderer/constants/themes.test.tssrc/renderer/App.tsxsrc/renderer/components/CustomThemeBuilder.tsxsrc/shared/theme-types.tssrc/shared/themes.ts
1 participant
Summary
The draggable window title bar (the top strip with the traffic-light buttons and the centered agent title) rendered transparent and showed
bgMainbehind it. Themes had no way to color that strip independently, so on light themes it always read as the lightest color in the palette.This adds a themeable
bgTitleBartoken so the title bar can be colored per-theme.What changed
src/shared/theme-types.ts- new optionalbgTitleBar?field onThemeColors. Optional (like the ANSI keys) so pre-existing saved/imported custom themes degrade gracefully.src/shared/themes.ts- setbgTitleBaron all 20 built-in themes, each mirroring its ownbgMain, so every existing theme looks pixel-identical after this change.src/renderer/App.tsx- wire the title bar background tobgTitleBar ?? bgMain(the fallback reproduces today's behavior for any theme lacking the key).src/renderer/components/CustomThemeBuilder.tsx- new editable "Title Bar Background" field + a title-bar strip in the live preview.src/__tests__/renderer/constants/themes.test.ts- addbgTitleBartoREQUIRED_COLORS.Backward compatibility
bgTitleBardefaults tobgMaineverywhere it is unset (built-in themes set it explicitly; the runtime falls back via?? bgMain). No theme changes appearance unless itsbgTitleBaris deliberately set to something different.Validation
Notes
App.tsxwiring.rcwith the same single commit cherry-picked.Summary by CodeRabbit