feat(desktop): make terminal colors optional with xterm defaults fallback#478
feat(desktop): make terminal colors optional with xterm defaults fallback#478
Conversation
WalkthroughTerminal color handling was refactored: Theme.terminal is now optional and terminal palettes are derived via getTerminalColors()/getDefaultTerminalColors() with built-in light/dark defaults; code was updated to use the derived terminal colors across theme store, editor conversion, and UI components. Changes
Sequence Diagram(s)(omitted — changes are refactors of data derivation and constants, not new multi-component control flow) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (9)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/desktop/**/*.{ts,tsx}📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
Files:
apps/desktop/src/renderer/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/{components,features}/**/[!.]*.tsx📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/{components,features}/**/*.{ts,tsx,test.ts,test.tsx,stories.tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{tsx,css}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/{components,features}/**/*.tsx📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)apps/desktop/src/renderer/stores/theme/utils/monaco-theme.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
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 |
AviPeltz
force-pushed
the
feat/optional-terminal-colors
branch
from
c18f9e6 to
322311f
Compare
…back - Add DEFAULT_TERMINAL_COLORS_DARK and DEFAULT_TERMINAL_COLORS_LIGHT constants - Add getDefaultTerminalColors() and getTerminalColors() helper functions - Make terminal property optional in Theme interface - Update light theme to use standard xterm default ANSI colors - Update theme store and terminal helpers to use fallback system Themes can now omit terminal colors and will automatically use xterm defaults based on the theme type (light/dark).
AviPeltz
force-pushed
the
feat/optional-terminal-colors
branch
from
322311f to
3589ff7
Compare
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The main risks are around mutability and robustness: getTerminalColors() can return shared default objects that are easy to accidentally mutate, and the localStorage cached terminal colors aren’t normalized against defaults, which can cause odd rendering if the cache is partial/legacy/corrupted. There’s also duplicated ANSI palette data between dark/light defaults that would be safer and easier to maintain if composed from a shared constant. Addressing these will make the new fallback system more resilient long-term.
Additional notes (3)
- Maintainability |
apps/desktop/src/shared/themes/types.ts:10-67
DEFAULT_TERMINAL_COLORS_DARKandDEFAULT_TERMINAL_COLORS_LIGHTdefine identical ANSI palettes (standard + bright). That duplication increases the chance of divergence or mistakes when updating values.
Also, these are described as “xterm defaults”; the ANSI 16-color palette being identical for light/dark makes sense, but then only the base fields (background, foreground, etc.) differ—those could be layered on top of a single shared palette constant.
- Readability |
...omponents/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts:16-16
getDefaultTerminalTheme()still parsestheme-terminalfrom localStorage and passes it totoXtermTheme(...). Iftheme-terminalwas written by an older app version (or is corrupted), the JSON may be structurally valid but missing required terminal fields, resulting in an incompleteIThemeand potentially weird rendering (e.g., missing ANSI colors).
You already introduced getTerminalColors(theme) for theme objects, but the cached path has no equivalent normalization step.
- Compatibility |
apps/desktop/src/shared/themes/built-in/light.ts:50-75
selectionBackgroundchanged from a semi-transparent black (rgba(0, 0, 0, 0.15)) to an opaque#add6ff. That aligns with xterm defaults, but it can materially impact readability/contrast depending on your UI background and terminal font rendering.
If this project expects theme terminal selection to blend with UI styling, you may want to ensure this value is intentionally accepted (and ideally tested/validated visually).
Summary of changes
What changed
Terminal theming now supports defaults
- Introduced
DEFAULT_TERMINAL_COLORS_DARK/DEFAULT_TERMINAL_COLORS_LIGHTinshared/themes/types.ts. - Added helpers
getDefaultTerminalColors(type)andgetTerminalColors(theme)to centralize fallback logic whentheme.terminalis absent. - Made
Theme.terminaloptional (terminal?: TerminalColors).
App now uses the fallback helper everywhere
- Updated terminal startup flash-prevention logic in
Terminal/helpers.tsto usegetTerminalColors(theme)for built-ins. - Updated theme store persistence and application (
renderer/stores/theme/store.ts) to cache and apply terminal colors viagetTerminalColors(theme).
Light theme terminal palette updated
- Changed
lightTheme.terminalANSI palette and some base values (foreground,cursor,selectionBackground) to xterm defaults.
Public exports
- Re-exported default constants + helper functions from
shared/themes/index.ts.
| export function getTerminalColors(theme: Theme): TerminalColors { | ||
| return theme.terminal ?? getDefaultTerminalColors(theme.type); | ||
| } |
There was a problem hiding this comment.
getTerminalColors() returns the theme’s terminal object as-is, otherwise returns a shared default object. That makes the returned value potentially mutable shared state (especially for the defaults), and it’s easy for downstream code (or xterm integration) to accidentally mutate it and affect all consumers.
Even if the current code path doesn’t mutate, this is a sharp edge that’s hard to debug later.
Suggestion
Consider returning a defensive copy to avoid accidental mutation of the shared defaults (and to keep behavior consistent whether the source is theme.terminal or defaults).
For example:
return { ...(theme.terminal ?? getDefaultTerminalColors(theme.type)) };
If you want to be extra safe against nested mutation (not needed here since it’s flat), a shallow copy is sufficient.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/shared/themes/built-in/light.ts (1)
50-76: Consider removing the terminal property to reduce duplication.The
lightTheme.terminalcolors now exactly matchDEFAULT_TERMINAL_COLORS_LIGHTdefined intypes.ts. Since the Theme interface makes terminal optional with automatic fallback to defaults, you could remove this property to eliminate duplication and reduce maintenance burden.🔎 Proposed refactor to use fallback defaults
}, - - terminal: { - background: "#ffffff", - foreground: "#000000", - cursor: "#000000", - cursorAccent: "#ffffff", - selectionBackground: "#add6ff", - - // Standard ANSI colors (xterm defaults) - black: "#2e3436", - red: "#cc0000", - green: "#4e9a06", - yellow: "#c4a000", - blue: "#3465a4", - magenta: "#75507b", - cyan: "#06989a", - white: "#d3d7cf", - - // Bright ANSI colors (xterm defaults) - brightBlack: "#555753", - brightRed: "#ef2929", - brightGreen: "#8ae234", - brightYellow: "#fce94f", - brightBlue: "#729fcf", - brightMagenta: "#ad7fa8", - brightCyan: "#34e2e2", - brightWhite: "#eeeeec", - }, };This would allow the fallback system to provide the identical colors via
getTerminalColors(lightTheme), eliminating the need to maintain two copies of the same values.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/stores/theme/store.tsapps/desktop/src/shared/themes/built-in/light.tsapps/desktop/src/shared/themes/index.tsapps/desktop/src/shared/themes/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/stores/theme/store.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid using any type in TypeScript - maintain type safety unless absolutely necessary
Files:
apps/desktop/src/shared/themes/types.tsapps/desktop/src/shared/themes/built-in/light.tsapps/desktop/src/shared/themes/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix
Files:
apps/desktop/src/shared/themes/types.tsapps/desktop/src/shared/themes/built-in/light.tsapps/desktop/src/shared/themes/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/shared/themes/types.tsapps/desktop/src/shared/themes/built-in/light.tsapps/desktop/src/shared/themes/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
**/{components,features}/**/*.{ts,tsx,test.ts,test.tsx,stories.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process or shared code - use only in main process (src/main/)
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
🧠 Learnings (3)
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules in renderer process or shared code - use only in main process (src/main/)
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx} : Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (7)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (2)
13-17: LGTM! Clean integration with centralized terminal color helpers.The import and usage of
getTerminalColorsfromshared/themesproperly centralizes terminal color derivation with fallback logic. This maintains type safety while reducing direct access totheme.terminal.
38-38: Fallback logic correctly handles all theme types.The refactoring safely replaces direct
theme.terminalaccess withgetTerminalColors(theme), which properly encapsulates fallback behavior:
- All 5 built-in themes define terminal colors explicitly
- Custom themes without terminal colors fall back via the nullish coalescing operator to
getDefaultTerminalColors(theme.type)- Theme store hydration is protected by a 4-layer fallback chain in
getDefaultTerminalTheme(): cached colors → lookup by theme ID → DEFAULT_THEME_ID → hardcoded xterm defaultstoXtermTheme()correctly mapsTerminalColorsto xterm'sIThemeformat with proper optional property handlingThe refactoring improves maintainability by centralizing color resolution logic.
apps/desktop/src/shared/themes/index.ts (1)
14-19: LGTM! Proper barrel export pattern.The re-exports make the new terminal color utilities available through the index, maintaining clean module boundaries.
apps/desktop/src/shared/themes/types.ts (3)
72-85: LGTM! Clean helper functions with appropriate fallback logic.The helper functions correctly implement the fallback system:
getDefaultTerminalColorsselects the appropriate default palette based on theme typegetTerminalColorsuses nullish coalescing to fall back to defaults whentheme.terminalis undefinedThis maintains type safety while providing sensible defaults.
204-205: Well-documented optional property with clear fallback behavior.Making
terminaloptional in the Theme interface is a good design choice that reduces boilerplate for theme authors. The inline documentation clearly explains the fallback behavior.
10-36: ANSI color values are correct and match xterm.js defaults.The hex values in
DEFAULT_TERMINAL_COLORS_DARKandDEFAULT_TERMINAL_COLORS_LIGHTare verified as the official xterm.js default ANSI color palette. No changes needed.apps/desktop/src/shared/themes/built-in/light.ts (1)
52-55: Clarify the light theme color design rationale or reference a specific design standard.The light theme colors cannot be verified against "xterm.js defaults" because xterm.js does not specify official default colors for light mode—the ITheme properties are customizable and theme-specific. Either document which design standard these colors should align with (e.g., a specific light theme specification or accessibility standard), or verify the color choices match the project's documented design requirements.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 Preview resources have been processed for cleanup |
1 participant
Summary
Makes terminal colors optional in theme definitions, with automatic fallback to xterm.js defaults based on theme type (light/dark).
Changes
DEFAULT_TERMINAL_COLORS_DARKandDEFAULT_TERMINAL_COLORS_LIGHTconstants with standard xterm.js ANSI colorsgetDefaultTerminalColors(type)andgetTerminalColors(theme)helper functionsterminalproperty optional inThemeinterfaceBenefits
Summary by CodeRabbit
New Features
Style
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.