feat: fix theming issues and set up new themes#178
Conversation
Kimchi Code Review
Summary📊 Review Score: 76/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: no — No tests were added or modified for the new theme file watcher, hot-reload logic, terminal color extensions, or CLI viewport painting. The new themes are purely data files without accompanying validation tests. 🔒 Security concerns found: Unsanitized 📝 Found 8 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 76/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 4/5 (1 = trivial, 5 = very complex)
🧪 Tests: no — No tests were added or modified for the new theme file watcher, hot-reload logic, terminal color extensions, or CLI viewport painting. The new themes are purely data files without accompanying validation tests.
🔒 Security concerns found: Unsanitized themeName from user-controlled settings.json is interpolated directly into resolve() in both src/cli.ts and src/extensions/terminal-colors.ts, allowing directory traversal (e.g., ../foo) to read arbitrary .json files outside the themes directory. Impact is limited because this is a local CLI tool, but it is still an unsafe path construction.
📝 Found 8 issue(s). See inline comments for details.
| } catch (err) { | ||
| console.warn("[theme-file-watcher] listener error:", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The fire function suppresses events where changedTheme === lastActiveTheme. Because ensureWatcher seeds lastActiveTheme with the currently active theme, editing the active theme file never triggers listeners on the first (or any subsequent) change. The watcher effectively only works once immediately after a theme switch, breaking the hot-reload contract.
💡 Suggestion: Remove the lastActiveTheme guard from fire; the 30 ms debounce timer is already sufficient to coalesce duplicate fs.watch events from a single write.
| // per-process at session_start from the OSC 11 probe. | ||
| const themesDir = resolve(agentDir, "themes") | ||
| const bundledThemes = ["kimchi.json", "kimchi-minimal.json", "kimchi-light.json", "dark.json", "light.json"] | ||
| const bundledThemes = [ |
There was a problem hiding this comment.
theme-file-watcher.ts relies on process.env.KIMCHI_CODING_AGENT_THEMES_DIR to know which directory to watch, but cli.ts computes themesDir locally and never assigns it to that environment variable. Consequently, ensureWatcher exits silently and no file watching is established.
💡 Suggestion: After computing themesDir in cli.ts, add process.env.KIMCHI_CODING_AGENT_THEMES_DIR = themesDir so the theme file watcher can locate the directory.
| // Paint the initial viewport background before pi-mono renders its first frame. | ||
| // This ensures blank areas of the terminal reflect the theme color from the start, | ||
| // on every terminal regardless of OSC 10/11 support. | ||
| if (!acpMode && process.stdout.isTTY) { |
There was a problem hiding this comment.
The themeName value is taken from user-editable settings.json and interpolated into resolve(themesDir, \${themeName}.json`)without sanitization. A crafted value like../package` escapes the themes directory and can read arbitrary files.
💡 Suggestion: Sanitize the name with path.basename(themeName) before resolving, or validate that the resolved path starts with themesDir.
| const raw = readFileSync(path, "utf-8") | ||
| const theme = JSON.parse(raw) | ||
| const vars: Record<string, string> = theme.vars ?? {} | ||
| const resolveVar = (val: string): string => vars[val] ?? val |
There was a problem hiding this comment.
Same path traversal issue as in cli.ts: themeName is passed directly into resolve(dir, "themes", \${themeName}.json`)` without stripping directory traversal segments.
💡 Suggestion: Use path.basename(themeName) before resolving the theme file path, or constrain the lookup to the known themes directory.
| } | ||
| } | ||
|
|
||
| export function onThemeFileChange(listener: ThemeFileChangeListener): () => void { |
There was a problem hiding this comment.
The fs.watch instance is created without an 'error' event listener. If the watched directory is deleted or permissions change while kimchi is running, the emitted error is uncaught and can crash the process.
💡 Suggestion: Add an error handler: watcher.on('error', (err) => { console.warn('[theme-file-watcher] watch error:', err); watcher?.close(); watcher = undefined; }).
| function hexToItermFormat(hex: string): string | null { | ||
| if (!hex || hex === "") return null | ||
| const match = hex.match(/^#?([0-9a-fA-F]{6})$/i) | ||
| if (!match) return null |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
isIterm2() returns true when process.env.TERM === "xterm-256color-italic", which is not iTerm2-specific. Other terminals may use this TERM value and will receive unrecognized OSC 1337 sequences.
💡 Suggestion: Rely only on process.env.TERM_PROGRAM === "iTerm.app" for iTerm2-specific OSC 1337 behavior.
| "cardBg": "#0d1f35", | ||
| "infoBg": "#1a2d42" | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
The file is missing a trailing newline.
💡 Suggestion: Add a newline at the end of the file.
| "oscFg": "", | ||
| "oscBg": "" | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
The file is missing a trailing newline.
💡 Suggestion: Add a newline at the end of the file.
1 participant
Kimchi Summary
What changed
Adds six new community themes (Night Owl, Nord, One Dark, Monokai, Catppuccin Macchiato, Lucent Orng) and a transparent theme. Enables all themes to optionally declare
oscFg/oscBgcolors for terminal-wide foreground/background control via OSC sequences. Fixes background rendering gaps when ANSI reset codes appear inside blocks, and adds auto-contrast text generation for thekimchi-minimaltheme. Theme files can now be hot-reloaded when edited externally.Why
Previously only the hardcoded
kimchitheme could override terminal colors, leaving other themes with inconsistent backgrounds and unreadable text on some terminal emulators. ANSI reset codes inside rendered blocks could also cancel background fills, creating visual gaps. These changes unify terminal-wide theming across all bundled themes and makekimchi-minimaladapt its text color to the probed terminal background for better readability.Key changes
src/extensions/terminal-colors.ts: ReadsoscFg/oscBgdynamically from the active theme JSON instead of using hardcoded values; addsfillViewportto paint backgrounds by writing explicit spaces (works around BCE-disabled terminals like Terminal.app); adds iTerm2-specific OSC 1337 support; restores the original terminal palette when switching to a theme without OSC values.src/cli.ts: Paints the initial viewport with the active theme'soscBgbefore the first render frame; registers new bundled themes (night-owl,nord,one-dark,monokai,catppuccin-macchiato,lucent-orng,transparent).src/extensions/kimchi-minimal-tints.ts: Computes terminal background luminance on startup and setsfgColorsfor thetexttoken to#333333on light backgrounds or#CCCCCCon dark backgrounds; subscribes toonThemeFileChangeto re-apply tints without restarting.src/theme-file-watcher.ts: New debounced file watcher forthemes/*.jsonthat triggers callbacks when the active theme file is modified on disk.patches/@mariozechner__pi-tui.patch: Re-injects background ANSI open codes after inline\x1b[49mand\x1b[0mresets inapplyBackgroundToLine, ensuring block backgrounds cover the full line width including padding.src/terminal-bg-probe.ts: AddshexToFgAnsihelper for truecolor/256-color foreground ANSI generation.oscFg/oscBgtokimchi,dark,light, andkimchi-light; added emptyoscFg/oscBgtokimchi-minimalto opt out of OSC theming.themes/night-owl.json,themes/nord.json,themes/one-dark.json,themes/monokai.json,themes/catppuccin-macchiato.json,themes/lucent-orng.json,themes/transparent.json.Impact
oscBg(including existingkimchi,dark, andlight) now set the terminal emulator's default background on activation and restore the original palette when switching away.kimchi-minimalnow explicitly overrides thetextforeground color based on probed terminal luminance instead of inheriting the terminal's default..jsonwhile kimchi is running triggers an immediate re-application of colors after the internal reload.