-
Notifications
You must be signed in to change notification settings - Fork 134
fix(tui): resolve terminal theme mode from every signal instead of guessing dark #1152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
17196d3
b01d99a
0306bf0
e452112
7969a8c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,13 @@ | ||
| import { expect, test } from "bun:test" | ||
| import { RGBA, type CliRenderer, type TerminalColors } from "@opentui/core" | ||
| import { RUN_THEME_FALLBACK, generateSystem, resolveRunTheme, resolveTheme } from "@/cli/cmd/run/theme" | ||
| import { type ColorInput, RGBA, type CliRenderer, type TerminalColors } from "@opentui/core" | ||
| import { | ||
| RUN_THEME_FALLBACK, | ||
| isRunThemeFallback, | ||
| runThemeFallback, | ||
| generateSystem, | ||
| resolveRunTheme, | ||
| resolveTheme, | ||
| } from "@/cli/cmd/run/theme" | ||
|
|
||
| const palette = ["#15161e", "#f7768e", "#9ece6a", "#e0af68", "#7aa2f7", "#bb9af7", "#7dcfff", "#c0caf5"] as const | ||
|
|
||
|
|
@@ -59,7 +66,40 @@ function spread(color: RGBA) { | |
| } | ||
|
|
||
| test("falls back when palette lookup fails", async () => { | ||
| expect(await resolveRunTheme(renderer({ fail: true }))).toBe(RUN_THEME_FALLBACK) | ||
| // Deliberately not `toBe(RUN_THEME_FALLBACK)`: with no OSC reply and no | ||
| // COLORFGBG the fallback now asks the OS for its appearance, so which | ||
| // per-mode instance comes back depends on the machine running the test. | ||
| // Pinning the dark one would re-encode the #809 behaviour this PR removes | ||
| // and would fail on a light-mode runner. The invariant is that a failed | ||
| // palette lookup yields *a* fallback rather than a resolved theme. | ||
| const theme = await resolveRunTheme(renderer({ fail: true })) | ||
| expect(isRunThemeFallback(theme)).toBe(true) | ||
| }) | ||
|
|
||
| test("a dark terminal still gets the dark fallback", async () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: This new test duplicates an existing test already in this file: both are titled "a dark terminal still gets the dark fallback" and both assert that Prompt for AI agents |
||
| // The mode-aware path must not have inverted anything: given an explicit | ||
| // dark signal the fallback is still the dark instance callers compare by | ||
| // identity. | ||
| expect(await resolveRunTheme(renderer({ fail: true, themeMode: "dark" }))).toBe(RUN_THEME_FALLBACK) | ||
| }) | ||
|
|
||
| test("the fallback follows a light terminal instead of always going dark", async () => { | ||
| // The direct-run fallback used to be unconditionally dark while its `text` | ||
| // preferred the terminal's own default foreground. On a light terminal that | ||
| // foreground resolves to black, so a dark panel produced black-on-black — | ||
| // the class of symptom reported in #809. | ||
| const light = await resolveRunTheme(renderer({ fail: true, themeMode: "light" })) | ||
|
|
||
| expect(light).not.toBe(RUN_THEME_FALLBACK) | ||
| expect(light).toBe(runThemeFallback("light")) | ||
|
|
||
| // The panel must actually be light, or the fix is cosmetic. | ||
| const sum = (color: ColorInput) => (color as RGBA).toInts().slice(0, 3).reduce((a, b) => a + b, 0) | ||
| expect(sum(light.block.diffContextBg)).toBeGreaterThan(sum(RUN_THEME_FALLBACK.block.diffContextBg)) | ||
| }) | ||
|
|
||
| test("a dark terminal still gets the dark fallback", async () => { | ||
| expect(await resolveRunTheme(renderer({ fail: true, themeMode: "dark" }))).toBe(runThemeFallback("dark")) | ||
| }) | ||
|
|
||
| test("returns syntax styles and indexed splash colors", async () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -107,7 +107,7 @@ import { win32DisableProcessedInput, win32FlushInputBuffer } from "./terminal-wi | |||||
| import { destroyRenderer } from "./util/renderer" | ||||||
| import { cliErrorMessage, errorFormat } from "./util/error" | ||||||
| // altimate_change start — fix: pure helper extracted to terminal-detection for test coverage (#704) | ||||||
| import { detectModeFromCOLORFGBG } from "./terminal-detection" | ||||||
| import { detectModeFromCOLORFGBG, detectSystemAppearance, resolveInitialMode } from "./terminal-detection" | ||||||
| // altimate_change end | ||||||
|
|
||||||
| const appGlobalBindingCommands = [ | ||||||
|
|
@@ -265,9 +265,19 @@ export const run = Effect.fn("Tui.run")(function* (input: TuiInput) { | |||||
| yield* Effect.tryPromise(async () => { | ||||||
| // Prewarm palette before ThemeProvider mounts so `system` theme avoids a first-paint fallback flash. | ||||||
| void renderer.getPalette({ size: 16 }).catch(() => undefined) | ||||||
| // altimate_change start — fix: check COLORFGBG eagerly to avoid 1s startup delay on terminals without OSC 11 (#704) | ||||||
| // altimate_change start — fix: resolve the startup mode from every available | ||||||
| // signal instead of falling through to "dark" (#617 → #704 → #736). | ||||||
| // COLORFGBG is free, so it short-circuits the OSC wait in both directions. | ||||||
| // Only when the terminal answers neither do we ask the OS, which is the | ||||||
| // case Apple Terminal users kept hitting. | ||||||
| const envMode = detectModeFromCOLORFGBG(process.env.COLORFGBG) | ||||||
| const mode = envMode === "light" ? "light" : ((await renderer.waitForThemeMode(1000)) ?? "dark") | ||||||
| // Always ask the terminal — it is the only signal describing this window | ||||||
| // now. COLORFGBG only buys a shorter wait: with a usable hint in hand we | ||||||
| // can stop waiting sooner, which keeps #704's startup win without | ||||||
| // letting a stale env var override a live answer. | ||||||
| const oscMode = (await renderer.waitForThemeMode(envMode ? 250 : 1000)) ?? null | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: When a valid but stale Prompt for AI agents
Suggested change
|
||||||
| const appearance = oscMode || envMode ? null : await detectSystemAppearance() | ||||||
| const mode = resolveInitialMode({ colorfgbg: process.env.COLORFGBG, osc: oscMode, appearance }) | ||||||
| // altimate_change end | ||||||
| if (renderer.isDestroyed) return | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SUGGESTION:
detectSystemAppearance()spawnsdefaultson the footer's palette-refresh failure path, where the answer is discarded.resolveRunThemeis shared by the direct-run startup (runtime.lifecycle.ts:198, which consumes the fallback) and the TUI footer'shandlePalette(footer.ts:1009). In the footer,isRunThemeFallback(theme)discards the fallback to keep the last-known-good theme, so the OS probe's result is thrown away. On the exact machine this PR targets — a light macOS Apple Terminal with noCOLORFGBGand no OSC 11 reply — every failed runtime palette refresh now spawns/usr/bin/defaultsand waits up to 400ms for nothing. Only the direct-run path consumes the appearance signal; consider skipping the probe when the fallback will be discarded.Reply with
@kilocode-bot fix itto have Kilo Code address this issue.