fix(desktop): restore COLORFGBG env var and mosaic theme for light mode#1235
fix(desktop): restore COLORFGBG env var and mosaic theme for light mode#1235
Conversation
The COLORFGBG environment variable (used by TUI apps like OpenCode and Claude Code to detect light/dark terminal backgrounds) was lost during the terminal hooks refactor. This restores it by passing themeType from the renderer through tRPC to buildTerminalEnv. Also fixes the Mosaic pane layout which had a hardcoded dark theme class by dynamically switching between mosaic-theme-dark and mosaic-theme-light.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThreads an optional Changes
Sequence DiagramsequenceDiagram
participant UI as UI Component
participant Hook as useCreateOrAttachWithTheme
participant Store as Theme Store / utils
participant Router as TRPC Router
participant Daemon as Daemon Manager
participant Env as buildTerminalEnv
participant Session as Terminal Session
UI->>Hook: mutate(params)
Hook->>Store: resolveTerminalThemeType(activeTheme)
Store-->>Hook: themeType
Hook->>Router: terminal.createOrAttach(params + themeType)
Router->>Router: resolveTerminalThemeType(requested, persisted, systemPref)
Router-->>Daemon: doCreateOrAttach(params + resolvedThemeType)
Daemon->>Env: buildTerminalEnv(params + themeType)
Env->>Env: compute COLORFGBG from themeType
Env-->>Daemon: env (includes COLORFGBG)
Daemon->>Session: createSession(params + env)
Session-->>Daemon: session created
Daemon-->>Router: response
Router-->>Hook: success
Hook-->>UI: mutation complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.ts (2)
154-171:⚠️ Potential issue | 🟡 MinorRemove
themeTypeRef.currentfrom dependency array.Adding
ref.currentto a dependency array is incorrect. Refs are stable objects that don't trigger re-renders when their.currentvalue changes. The callback already readsthemeTypeRef.currentat invocation time, which is the correct pattern.Including
.currentin the deps array will:
- Capture a stale value at callback creation time
- Not trigger re-creation when the theme actually changes (since ref mutations don't cause re-renders)
Proposed fix
}, [ paneId, tabId, workspaceId, xtermRef, isStreamReadyRef, isExitedRef, wasKilledByUserRef, isFocusedRef, didFirstRenderRef, pendingInitialStateRef, createOrAttachRef, setConnectionError, setExitStatus, maybeApplyInitialState, flushPendingEvents, - themeTypeRef.current, ]);
238-256:⚠️ Potential issue | 🟡 MinorSame issue: remove
themeTypeRef.currentfrom dependency array.Same concern as
handleRetryConnection— ref values should not be in dependency arrays.Proposed fix
}, [ paneId, tabId, workspaceId, xtermRef, fitAddonRef, isStreamReadyRef, isExitedRef, wasKilledByUserRef, pendingInitialStateRef, pendingEventsRef, createOrAttachRef, setConnectionError, setExitStatus, maybeApplyInitialState, flushPendingEvents, resetModes, - themeTypeRef.current, ]);
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/terminal/env.ts (1)
326-370: Extract COLORFGBG values to named constants.Keeps the ANSI codes discoverable and prevents accidental drift if reused elsewhere.
♻️ Suggested refactor
export const FALLBACK_SHELL = os.platform() === "win32" ? "cmd.exe" : "/bin/sh"; export const SHELL_CRASH_THRESHOLD_MS = 1000; +const COLORFGBG_LIGHT = "0;15"; +const COLORFGBG_DARK = "15;0"; @@ - const colorFgBg = themeType === "light" ? "0;15" : "15;0"; + const colorFgBg = themeType === "light" ? COLORFGBG_LIGHT : COLORFGBG_DARK;As per coding guidelines, extract hardcoded magic numbers, strings, and enums to named constants at module top instead of leaving them inline in logic.
…e-light-mode-displaying
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.ts (1)
154-171:⚠️ Potential issue | 🟡 Minor
themeTypeRef.currentin dependency arrays should bethemeTypeRef.All other refs in these dependency arrays are listed as ref objects (e.g.,
xtermRef,createOrAttachRef), not.current. Using.currentis inconsistent and technically incorrect — React cannot track mutations to a ref's.currentproperty, so it won't reliably trigger callback recreation. Since you're already readingthemeTypeRef.currentinside the callback body (line 98), listing the stable ref object is sufficient and consistent with the existing pattern.Same issue in
handleStartShellat line 255.Proposed fix
], [ paneId, tabId, workspaceId, xtermRef, isStreamReadyRef, isExitedRef, wasKilledByUserRef, isFocusedRef, didFirstRenderRef, pendingInitialStateRef, createOrAttachRef, setConnectionError, setExitStatus, maybeApplyInitialState, flushPendingEvents, - themeTypeRef.current, + themeTypeRef, ]);createOrAttachRef, setConnectionError, setExitStatus, maybeApplyInitialState, flushPendingEvents, resetModes, - themeTypeRef.current, + themeTypeRef, ]);
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/terminal/terminal.ts`:
- Around line 124-127: The fallback path calling resolveTerminalThemeType
currently omits systemPrefersDark, causing persisted "system" theme to assume
dark; update the call that computes resolvedThemeType (where themeType and
appState.data.themeState are passed) to also provide the real OS preference by
reading Electron's nativeTheme.shouldUseDarkColors from the main process (expose
it via your existing IPC/getAPI if needed) and pass that boolean as
systemPrefersDark to resolveTerminalThemeType so "system" resolves correctly.
In `@apps/desktop/src/lib/trpc/routers/terminal/theme-type.ts`:
- Around line 27-28: Replace the hardcoded "system" sentinel with the shared
constant: import and use SYSTEM_THEME_ID in the activeThemeId check (the
comparison involving activeThemeId and the branch that returns systemPrefersDark
? "dark" : "light"). Update the import at the top of theme-type.ts to pull
SYSTEM_THEME_ID from the module that defines it (or move SYSTEM_THEME_ID to the
shared/themes module alongside DEFAULT_THEME_ID and import from there) so the
comparison uses SYSTEM_THEME_ID instead of the literal string.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/react-query/workspaces/useOpenExternalWorktree.ts (1)
21-24: Consider extracting auseTerminalThemeType()hook to reduce duplication.This exact 3-line pattern (
useTheme()→resolveTerminalThemeType(...)) is repeated inWorkspaceInitEffects.tsx,useOpenExternalWorktree.ts, anduseOpenWorktree.ts. A small hook would centralize the logic:export function useTerminalThemeType(): ThemeType { const activeTheme = useTheme(); return resolveTerminalThemeType({ activeThemeType: activeTheme?.type }); }apps/desktop/src/lib/trpc/routers/terminal/theme-type.test.ts (1)
14-72: Good test coverage of the key resolution paths.The tests cover explicit requests, built-in themes, custom themes, system preference, and unknown-theme fallback. One minor gap: there's no test exercising the
persistedThemeState: undefined(withoutrequestedThemeType) path — i.e., callingresolveTerminalThemeType({})and expecting"dark". The first test happens to omitpersistedThemeState, but it short-circuits onrequestedThemeTypebefore reaching that branch. Consider adding a one-liner for completeness:it("falls back to dark when no persisted state exists", () => { expect(resolveTerminalThemeType({})).toBe("dark"); });apps/desktop/src/renderer/stores/theme/utils/terminal-theme-type.ts (1)
1-5:ThemeTypeis defined independently in two files.This same
type ThemeType = "dark" | "light"is also declared inapps/desktop/src/lib/trpc/routers/terminal/theme-type.ts(line 4). Since the project already has ashared/directory (used for themes), consider extractingThemeTypethere so both the renderer and tRPC resolver import the same canonical type — avoiding silent divergence if one is updated without the other.
| const resolvedThemeType = resolveTerminalThemeType({ | ||
| requestedThemeType: themeType, | ||
| persistedThemeState: appState.data.themeState, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the terminal.ts file around the specified lines
find . -name "terminal.ts" -path "*/trpc/routers/terminal/*" | head -5Repository: superset-sh/superset
Length of output: 120
🏁 Script executed:
# Check nativeTheme usage in the codebase
rg -n --type=ts 'nativeTheme' -g '!node_modules/**' | head -20Repository: superset-sh/superset
Length of output: 280
🏁 Script executed:
# Look at the theme-type.ts file to understand the resolveTerminalThemeType function
find . -name "theme-type.ts" -path "*/terminal/*" | head -5Repository: superset-sh/superset
Length of output: 122
🏁 Script executed:
# Read terminal.ts around lines 124-127 to verify the actual code
cat -n apps/desktop/src/lib/trpc/routers/terminal/terminal.ts | sed -n '115,135p'Repository: superset-sh/superset
Length of output: 707
🏁 Script executed:
# Read theme-type.ts to check the function signature and defaults
cat -n apps/desktop/src/lib/trpc/routers/terminal/theme-type.tsRepository: superset-sh/superset
Length of output: 1260
🏁 Script executed:
# Also check the imports at the top of terminal.ts to verify nativeTheme availability
cat -n apps/desktop/src/lib/trpc/routers/terminal/terminal.ts | head -30Repository: superset-sh/superset
Length of output: 1419
🏁 Script executed:
# Check the input schema to see if themeType is optional or required
cat -n apps/desktop/src/lib/trpc/routers/terminal/terminal.ts | sed -n '60,90p'Repository: superset-sh/superset
Length of output: 1051
🏁 Script executed:
# Look for where this tRPC procedure is called from to understand if themeType is always provided
rg -n 'createOrAttach' -A 2 -B 2 --type=ts -g '!node_modules/**' | head -40Repository: superset-sh/superset
Length of output: 2887
🏁 Script executed:
# Search for where the tRPC terminal.createOrAttach is called from the renderer side
rg -n 'terminal\.createOrAttach' --type=ts -g '!node_modules/**' -A 5 -B 5Repository: superset-sh/superset
Length of output: 7746
🏁 Script executed:
# Check if there's a renderer-side theme resolution that determines themeType
rg -n 'resolveTerminalThemeType' --type=ts -g '!node_modules/**' -B 3 -A 3Repository: superset-sh/superset
Length of output: 19732
systemPrefersDark is not passed to resolveTerminalThemeType — "system" theme in the fallback path will always default to dark.
When the renderer doesn't send themeType and the persisted theme is "system", the function defaults systemPrefersDark to true, resolving to "dark". While the normal path sends themeType from the renderer, the fallback (cold restore or missing themeType) needs to detect the actual OS preference. Use Electron's nativeTheme.shouldUseDarkColors in the main process:
Suggested fix
+import { nativeTheme } from "electron";
...
const resolvedThemeType = resolveTerminalThemeType({
requestedThemeType: themeType,
persistedThemeState: appState.data.themeState,
+ systemPrefersDark: nativeTheme.shouldUseDarkColors,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const resolvedThemeType = resolveTerminalThemeType({ | |
| requestedThemeType: themeType, | |
| persistedThemeState: appState.data.themeState, | |
| }); | |
| import { nativeTheme } from "electron"; | |
| // ... other imports ... | |
| import { appState } from "main/lib/app-state"; | |
| import { resolveTerminalThemeType } from "./theme-type"; | |
| // ... rest of file ... | |
| const resolvedThemeType = resolveTerminalThemeType({ | |
| requestedThemeType: themeType, | |
| persistedThemeState: appState.data.themeState, | |
| systemPrefersDark: nativeTheme.shouldUseDarkColors, | |
| }); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/terminal/terminal.ts` around lines 124 -
127, The fallback path calling resolveTerminalThemeType currently omits
systemPrefersDark, causing persisted "system" theme to assume dark; update the
call that computes resolvedThemeType (where themeType and
appState.data.themeState are passed) to also provide the real OS preference by
reading Electron's nativeTheme.shouldUseDarkColors from the main process (expose
it via your existing IPC/getAPI if needed) and pass that boolean as
systemPrefersDark to resolveTerminalThemeType so "system" resolves correctly.
| if (activeThemeId === "system") { | ||
| return systemPrefersDark ? "dark" : "light"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how widely the "system" activeThemeId sentinel is referenced
rg -n --type=ts '"system"' -g '!node_modules/**' -g '*theme*'Repository: superset-sh/superset
Length of output: 430
🏁 Script executed:
#!/bin/bash
# Search for existing theme constants or enums
rg -n --type=ts '(THEME|theme).*=.*["\']system["\']' -g '!node_modules/**'
# Also check for type definitions or constants in the theme file itself
echo "=== Checking theme-type.ts file structure ==="
head -50 apps/desktop/src/lib/trpc/routers/terminal/theme-type.tsRepository: superset-sh/superset
Length of output: 132
🏁 Script executed:
#!/bin/bash
# Search for existing theme constants
rg -n --type=ts 'THEME' -g '!node_modules/**' apps/desktop/src/lib/trpc/routers/terminal/
# Check for type definitions and constants in the theme file
echo "=== Checking theme-type.ts file structure ==="
head -60 apps/desktop/src/lib/trpc/routers/terminal/theme-type.tsRepository: superset-sh/superset
Length of output: 1295
🏁 Script executed:
#!/bin/bash
# Check the shared/themes module to see the pattern of theme constants
cat apps/desktop/src/shared/themes/index.ts || find . -path '*shared/themes*' -name '*.ts' -type f | head -5Repository: superset-sh/superset
Length of output: 452
🏁 Script executed:
#!/bin/bash
# Check the built-in.ts file to see all theme constants
cat apps/desktop/src/shared/themes/built-in.ts | head -100Repository: superset-sh/superset
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Find the actual themes module
find . -path '*shared/themes*' -type f -name '*.ts' | grep -v node_modules
echo "=== Directory structure ==="
find . -path '*shared/themes*' -type d | grep -v node_modulesRepository: superset-sh/superset
Length of output: 649
🏁 Script executed:
#!/bin/bash
# Check the built-in/index.ts to see theme constants
cat apps/desktop/src/shared/themes/built-in/index.ts
echo ""
echo "=== Check if SYSTEM_THEME_ID exists anywhere ==="
rg -n 'SYSTEM_THEME_ID' -g '!node_modules/**'Repository: superset-sh/superset
Length of output: 1785
🏁 Script executed:
#!/bin/bash
# Check the context around SYSTEM_THEME_ID definition in store.ts
sed -n '15,30p' apps/desktop/src/renderer/stores/theme/store.ts
echo ""
echo "=== Check if theme-type.ts imports SYSTEM_THEME_ID ==="
grep -n 'SYSTEM_THEME_ID' apps/desktop/src/lib/trpc/routers/terminal/theme-type.tsRepository: superset-sh/superset
Length of output: 583
🏁 Script executed:
#!/bin/bash
# Check if lib/trpc code typically imports from renderer stores
rg -n 'from.*renderer' apps/desktop/src/lib/trpc/ -g '!node_modules/**'
echo ""
echo "=== Check what shared/themes exports ==="
cat apps/desktop/src/shared/themes/types.ts | head -50Repository: superset-sh/superset
Length of output: 1273
🏁 Script executed:
#!/bin/bash
# Check what SYSTEM_THEME_ID depends on (what does store.ts import)
sed -n '1,25p' apps/desktop/src/renderer/stores/theme/store.ts
echo ""
echo "=== Check theme-type.ts full imports ==="
head -10 apps/desktop/src/lib/trpc/routers/terminal/theme-type.tsRepository: superset-sh/superset
Length of output: 1099
🏁 Script executed:
#!/bin/bash
# Check the ThemeState definition to understand the schema
grep -A 10 'interface ThemeState' apps/desktop/src/main/lib/app-state/schemas.ts || \
grep -A 10 'type ThemeState' apps/desktop/src/main/lib/app-state/schemas.tsRepository: superset-sh/superset
Length of output: 256
Use the existing SYSTEM_THEME_ID constant instead of the hardcoded "system" string.
The sentinel value "system" is already defined as SYSTEM_THEME_ID in apps/desktop/src/renderer/stores/theme/store.ts and is used throughout the renderer code. Import and use this constant in theme-type.ts at line 27 to maintain consistency and prevent typo-related bugs. If SYSTEM_THEME_ID should be shared across the main and renderer processes, consider moving it to shared/themes alongside DEFAULT_THEME_ID.
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/terminal/theme-type.ts` around lines 27 -
28, Replace the hardcoded "system" sentinel with the shared constant: import and
use SYSTEM_THEME_ID in the activeThemeId check (the comparison involving
activeThemeId and the branch that returns systemPrefersDark ? "dark" : "light").
Update the import at the top of theme-type.ts to pull SYSTEM_THEME_ID from the
module that defines it (or move SYSTEM_THEME_ID to the shared/themes module
alongside DEFAULT_THEME_ID and import from there) so the comparison uses
SYSTEM_THEME_ID instead of the literal string.
…e-light-mode-displaying
Pre-existing failure due to missing `projects` export from @superset/local-db.
Kitenite
deleted the
kitenite/fix-opencode-theme-light-mode-displaying
branch
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
COLORFGBGenvironment variable for TUI light mode detection (lost during terminal hooks refactor)What was broken
TUI applications like OpenCode and Claude Code use the
COLORFGBGenv var to detect whether the terminal has a light or dark background. This was previously added in commitda897cfa5but was lost when the terminal component was refactored into separate hooks. Without it, these apps render with dark-mode colors on a light background, making text unreadable.Additionally, the Mosaic pane container always used
mosaic-theme-darkclass regardless of the active theme.Changes
themeTypefrom renderer → tRPC →buildTerminalEnv→ PTY environmentCOLORFGBGto"0;15"(light) or"15;0"(dark) based on active thememosaic-theme-lightormosaic-theme-dark:is()Test plan
bun test apps/desktop/src/main/lib/terminal/env.test.ts— all 70 tests passSummary by CodeRabbit