fix(tui): re-resolve transient status colors on theme swap
Status presenters (showWarning/showError/showStatus and extension/tool error presenters) baked theme.fg() into Text at construction, so a warning shown while the auto-theme default guess was dark kept the dark-mode color after async appearance detection switched the active theme to light — dark-catppuccin Mocha yellow on the light Latte background (1.12:1 contrast, effectively invisible). Add Text.setStyleFn(), a foreground styler evaluated at render time, and route the transient status presenters through it. Because the coding agent invalidates status components on onThemeChange, a theme swap now re-shapes them against the live theme instead of replaying the palette active when they were constructed. Fixes #6337
This commit is contained in:
@@ -29,6 +29,7 @@
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed startup status messages (warnings, errors, extension/tool errors, status lines) keeping the dark-mode color after auto-theme detection later switched the active theme to light — e.g. `dark-catppuccin`/`light-catppuccin` warnings rendered in Mocha yellow on the Latte background. Transient status presenters now resolve their color lazily at render time so a theme swap re-shapes them ([#6337](https://github.com/can1357/oh-my-pi/issues/6337)).
|
||||
- Fixed `error.notify` raising a "Stopped with error" toast for provider failures while an auto-retry or async-delivery continuation was pending; the toast now waits for the true terminal settle.
|
||||
- Fixed terminal `yield` results racing post-turn maintenance, which could trigger an unnecessary automatic handoff or compaction.
|
||||
- Fixed credential-shaped tokens (GitHub/GitLab/OpenAI/Anthropic key patterns) being redacted from outbound provider requests even with `secrets.enabled` off; the pattern redaction now follows the `secrets.enabled` ("Hide Secrets") setting like the secret obfuscator.
|
||||
|
||||
@@ -532,7 +532,7 @@ export class ExtensionUiController {
|
||||
* Show a tool error in the chat.
|
||||
*/
|
||||
showToolError(toolName: string, error: string): void {
|
||||
const errorText = new Text(theme.fg("error", `Tool "${toolName}" error: ${error}`), 1, 0);
|
||||
const errorText = new Text(`Tool "${toolName}" error: ${error}`, 1, 0).setStyleFn(t => theme.fg("error", t));
|
||||
this.ctx.present(errorText);
|
||||
}
|
||||
|
||||
@@ -1092,7 +1092,9 @@ export class ExtensionUiController {
|
||||
}
|
||||
|
||||
showExtensionError(extensionPath: string, error: string): void {
|
||||
const errorText = new Text(theme.fg("error", `Extension "${extensionPath}" error: ${error}`), 1, 0);
|
||||
const errorText = new Text(`Extension "${extensionPath}" error: ${error}`, 1, 0).setStyleFn(t =>
|
||||
theme.fg("error", t),
|
||||
);
|
||||
this.ctx.present(errorText);
|
||||
}
|
||||
async #handleInteractiveCompact(instructionsOrOptions: string | CompactOptions | undefined): Promise<void> {
|
||||
|
||||
@@ -114,16 +114,19 @@ export class UiHelpers {
|
||||
const last = children.length > 0 ? children[children.length - 1] : undefined;
|
||||
const secondLast = children.length > 1 ? children[children.length - 2] : undefined;
|
||||
const useDim = options?.dim ?? true;
|
||||
const rendered = useDim ? theme.fg("dim", message) : message;
|
||||
// Resolve the dim color lazily so a later theme change re-shapes the line
|
||||
// instead of leaving the palette that was active when it was presented.
|
||||
const styleFn = useDim ? (t: string) => theme.fg("dim", t) : undefined;
|
||||
|
||||
if (last && secondLast && last === this.ctx.lastStatusText && secondLast === this.ctx.lastStatusSpacer) {
|
||||
this.ctx.lastStatusText.setText(rendered);
|
||||
this.ctx.lastStatusText.setStyleFn(styleFn);
|
||||
this.ctx.lastStatusText.setText(message);
|
||||
this.ctx.ui.requestRender();
|
||||
return;
|
||||
}
|
||||
|
||||
const spacer = new Spacer(1);
|
||||
const text = new Text(rendered, 1, 0);
|
||||
const text = new Text(message, 1, 0).setStyleFn(styleFn);
|
||||
this.ctx.present([spacer, text]);
|
||||
this.ctx.lastStatusSpacer = spacer;
|
||||
this.ctx.lastStatusText = text;
|
||||
@@ -714,11 +717,13 @@ export class UiHelpers {
|
||||
}
|
||||
|
||||
showError(errorMessage: string): void {
|
||||
this.ctx.present([new Spacer(1), new Text(theme.fg("error", `Error: ${errorMessage}`), 1, 0)]);
|
||||
const text = new Text(`Error: ${errorMessage}`, 1, 0).setStyleFn(t => theme.fg("error", t));
|
||||
this.ctx.present([new Spacer(1), text]);
|
||||
}
|
||||
|
||||
showWarning(warningMessage: string): void {
|
||||
this.ctx.present([new Spacer(1), new Text(theme.fg("warning", `Warning: ${warningMessage}`), 1, 0)]);
|
||||
const text = new Text(`Warning: ${warningMessage}`, 1, 0).setStyleFn(t => theme.fg("warning", t));
|
||||
this.ctx.present([new Spacer(1), text]);
|
||||
}
|
||||
|
||||
showNewVersionNotification(newVersion: string): void {
|
||||
|
||||
@@ -0,0 +1,66 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test";
|
||||
import * as themeModule from "@oh-my-pi/pi-coding-agent/modes/theme/theme";
|
||||
import { Text } from "@oh-my-pi/pi-tui";
|
||||
|
||||
/**
|
||||
* Regression for issue #6337: a status message presented while the auto-theme
|
||||
* default guess (dark) was active must re-resolve its color when the terminal's
|
||||
* appearance reply later switches the active theme to light. The transient
|
||||
* status presenters supply the color via `Text.setStyleFn` (evaluated at render
|
||||
* time against the live `theme` binding) instead of baking `theme.fg()` into the
|
||||
* component, so invalidating on `onThemeChange` re-shapes it.
|
||||
*/
|
||||
|
||||
/** Opening SGR sequence `theme.fg(color, ...)` emits, independent of color mode. */
|
||||
function fgPrefix(color: "warning"): string {
|
||||
const styled = themeModule.theme.fg(color, "\u0001");
|
||||
return styled.slice(0, styled.indexOf("\u0001"));
|
||||
}
|
||||
|
||||
describe("lazy status color re-resolves on theme switch", () => {
|
||||
beforeEach(async () => {
|
||||
themeModule.stopThemeWatcher();
|
||||
const dark = await themeModule.getThemeByName("dark");
|
||||
if (!dark) throw new Error("Failed to load dark theme for tests");
|
||||
themeModule.setThemeInstance(dark);
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
themeModule.stopThemeWatcher();
|
||||
const dark = await themeModule.getThemeByName("dark");
|
||||
if (dark) themeModule.setThemeInstance(dark);
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("swaps a presented warning from dark-catppuccin to light-catppuccin color", async () => {
|
||||
// Auto-theme resolves dark before the appearance reply arrives.
|
||||
themeModule.onTerminalAppearanceChange("dark");
|
||||
await themeModule.initTheme(false, undefined, undefined, "dark-catppuccin", "light-catppuccin");
|
||||
expect(themeModule.getCurrentThemeName()).toBe("dark-catppuccin");
|
||||
|
||||
const darkPrefix = fgPrefix("warning");
|
||||
const warning = new Text("Warning: Failed to load extension", 1, 0).setStyleFn(t =>
|
||||
themeModule.theme.fg("warning", t),
|
||||
);
|
||||
expect(warning.render(80).join("")).toContain(darkPrefix);
|
||||
|
||||
// The OSC 11 reply arrives → auto-theme switches to light-catppuccin.
|
||||
const switched = Promise.withResolvers<void>();
|
||||
const off = themeModule.onThemeChange(() => switched.resolve());
|
||||
themeModule.onTerminalAppearanceChange("light");
|
||||
await switched.promise;
|
||||
off();
|
||||
expect(themeModule.getCurrentThemeName()).toBe("light-catppuccin");
|
||||
|
||||
const lightPrefix = fgPrefix("warning");
|
||||
expect(lightPrefix).not.toBe(darkPrefix);
|
||||
|
||||
// The onThemeChange handler invalidates + repaints; the warning must now
|
||||
// render the light-mode color, not the baked dark one.
|
||||
warning.invalidate();
|
||||
const out = warning.render(80).join("");
|
||||
expect(out).toContain(lightPrefix);
|
||||
expect(out).not.toContain(darkPrefix);
|
||||
});
|
||||
});
|
||||
@@ -2,6 +2,10 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Added
|
||||
|
||||
- `Text.setStyleFn()` applies a foreground styler at render time, so a component re-resolves its color after `invalidate()` instead of baking the palette that was active when it was constructed.
|
||||
|
||||
## [17.0.9] - 2026-07-23
|
||||
|
||||
### Added
|
||||
|
||||
@@ -11,13 +11,20 @@ import {
|
||||
} from "../utils";
|
||||
|
||||
/**
|
||||
* Text component - displays multi-line text with word wrapping
|
||||
* Text component - displays multi-line text with word wrapping.
|
||||
*
|
||||
* Foreground colors may be supplied lazily via {@link setStyleFn} instead of
|
||||
* baked into `text`: the styler runs at render time, so a caller that
|
||||
* invalidates the component on a theme change (see the coding-agent's
|
||||
* `onThemeChange` handler) re-resolves the color against the now-active theme
|
||||
* rather than replaying the palette active when the component was constructed.
|
||||
*/
|
||||
export class Text implements Component {
|
||||
#text: string;
|
||||
#paddingX: number; // Left/right padding
|
||||
#paddingY: number; // Top/bottom padding
|
||||
#customBgFn?: (text: string) => string;
|
||||
#styleFn?: (text: string) => string;
|
||||
|
||||
#ignoreTight = false;
|
||||
|
||||
@@ -64,6 +71,21 @@ export class Text implements Component {
|
||||
this.#cachedLines = undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
* Supply a foreground styler applied to the text at render time (e.g. a
|
||||
* theme color resolver). Unlike baking the color into `text`, the styler
|
||||
* re-runs on every render, so invalidating the component after a theme
|
||||
* change re-resolves the color against the active theme.
|
||||
*/
|
||||
setStyleFn(styleFn?: (text: string) => string): this {
|
||||
this.#styleFn = styleFn;
|
||||
this.#cachedText = undefined;
|
||||
this.#cachedWidth = undefined;
|
||||
this.#cachedWidthEpoch = undefined;
|
||||
this.#cachedLines = undefined;
|
||||
return this;
|
||||
}
|
||||
|
||||
invalidate(): void {
|
||||
this.#cachedText = undefined;
|
||||
this.#cachedWidth = undefined;
|
||||
@@ -93,7 +115,7 @@ export class Text implements Component {
|
||||
}
|
||||
|
||||
// Replace tabs with 3 spaces
|
||||
const normalizedText = replaceTabs(this.#text);
|
||||
const normalizedText = replaceTabs(this.#styleFn ? this.#styleFn(this.#text) : this.#text);
|
||||
|
||||
// Calculate content width (subtract left/right margins)
|
||||
const paddingX = this.#ignoreTight ? this.#paddingX : getPaddingX(this.#paddingX);
|
||||
|
||||
@@ -9,4 +9,28 @@ describe("Text component", () => {
|
||||
expect(text.setText("b")).toBe(true);
|
||||
expect(text.getText()).toBe("b");
|
||||
});
|
||||
|
||||
it("applies the style fn at render time, not construction time", () => {
|
||||
const text = new Text("hello", 0, 0).setStyleFn(t => `<c>${t}</c>`);
|
||||
expect(text.render(40).join("\n")).toContain("<c>hello</c>");
|
||||
});
|
||||
|
||||
it("re-resolves the style fn after invalidate so a theme change re-shapes", () => {
|
||||
// The styler reads a mutable `color`, standing in for the active theme.
|
||||
// The coding-agent invalidates status components on `onThemeChange`, so a
|
||||
// lazily-styled Text must pick up the new color on the next render —
|
||||
// something a baked ANSI string can never do (issue #6337).
|
||||
let color = "RED";
|
||||
const text = new Text("hello", 0, 0).setStyleFn(t => `[${color}]${t}`);
|
||||
expect(text.render(40).join("\n")).toContain("[RED]hello");
|
||||
|
||||
// Without invalidation the cached render is returned unchanged.
|
||||
color = "BLUE";
|
||||
expect(text.render(40).join("\n")).toContain("[RED]hello");
|
||||
|
||||
text.invalidate();
|
||||
const out = text.render(40).join("\n");
|
||||
expect(out).toContain("[BLUE]hello");
|
||||
expect(out).not.toContain("[RED]hello");
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user