Fix ThemeRuntime review findings for intent scope and theme sync.
Keep follow-app intent until the side panel closes, scope manual picks to the focused host, and cancel stale hidden-pane scheduler updates when a visible terminal applies its theme synchronously. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
1 parent
eee7a8846c
commit
f726c6b4f6
9 files changed
+142
-15
No files matched your search
@@ -73,9 +73,11 @@ export function useThemeRuntime(settings: ThemeRuntimeSettings) {
|
||||
return true;
|
||||
}, [settings]);
|
||||
|
||||
const pickTheme = useCallback((themeId: string, options?: { followApp?: boolean }) => {
|
||||
const pickTheme = useCallback((themeId: string, options?: { followApp?: boolean; scopeHostId?: string | null }) => {
|
||||
const followApp = options?.followApp ?? settings.followAppTerminalTheme;
|
||||
setUserIntent(pickingThemeUserIntent(themeId));
|
||||
setUserIntent(pickingThemeUserIntent(themeId, {
|
||||
scopeHostId: followApp ? undefined : options?.scopeHostId,
|
||||
}));
|
||||
if (followApp) {
|
||||
applyFollowAppSettingsForPick(themeId);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,65 @@
|
||||
import assert from 'node:assert/strict';
|
||||
import test from 'node:test';
|
||||
|
||||
import {
|
||||
cancelTerminalThemeUpdate,
|
||||
resetTerminalThemeSchedulerForTests,
|
||||
scheduleTerminalThemeUpdate,
|
||||
} from './terminalThemeScheduler';
|
||||
import type { TerminalTheme } from '../../domain/models';
|
||||
|
||||
const theme = (id: string): TerminalTheme => ({
|
||||
id,
|
||||
name: id,
|
||||
type: 'dark',
|
||||
colors: {
|
||||
background: '#111111',
|
||||
foreground: '#eeeeee',
|
||||
cursor: '#22aaff',
|
||||
selection: '#22aaff44',
|
||||
black: '#000',
|
||||
red: '#000',
|
||||
green: '#000',
|
||||
yellow: '#000',
|
||||
blue: '#000',
|
||||
magenta: '#000',
|
||||
cyan: '#000',
|
||||
white: '#fff',
|
||||
brightBlack: '#000',
|
||||
brightRed: '#000',
|
||||
brightGreen: '#000',
|
||||
brightYellow: '#000',
|
||||
brightBlue: '#000',
|
||||
brightMagenta: '#000',
|
||||
brightCyan: '#000',
|
||||
brightWhite: '#fff',
|
||||
},
|
||||
});
|
||||
|
||||
test('cancelTerminalThemeUpdate drops a pending hidden-pane theme flush', async () => {
|
||||
resetTerminalThemeSchedulerForTests();
|
||||
let appliedThemeId: string | null = null;
|
||||
const fakeTerm = {
|
||||
options: { theme: {} as Record<string, string> },
|
||||
};
|
||||
|
||||
scheduleTerminalThemeUpdate(
|
||||
'session-1',
|
||||
theme('old-theme'),
|
||||
{ visible: false, focused: false },
|
||||
() => fakeTerm as never,
|
||||
);
|
||||
|
||||
cancelTerminalThemeUpdate('session-1');
|
||||
|
||||
await new Promise<void>((resolve) => {
|
||||
if (typeof requestIdleCallback === 'function') {
|
||||
requestIdleCallback(() => resolve());
|
||||
return;
|
||||
}
|
||||
setTimeout(resolve, 0);
|
||||
});
|
||||
|
||||
appliedThemeId = fakeTerm.options.theme.background ?? null;
|
||||
assert.equal(appliedThemeId, null);
|
||||
});
|
||||
@@ -61,6 +61,10 @@ function flushPendingUpdates(): void {
|
||||
}
|
||||
}
|
||||
|
||||
export function cancelTerminalThemeUpdate(sessionId: string): void {
|
||||
pendingBySession.delete(sessionId);
|
||||
}
|
||||
|
||||
export function scheduleTerminalThemeUpdate(
|
||||
sessionId: string,
|
||||
theme: TerminalTheme,
|
||||
|
||||
@@ -16,7 +16,7 @@ import {
|
||||
prioritizeTerminalInput,
|
||||
shouldArmTerminalInterruptDisplayGateForProtocol,
|
||||
} from './runtime/terminalOutputPipeline';
|
||||
import { scheduleTerminalThemeUpdate, applyTerminalThemeSync } from './terminalThemeScheduler';
|
||||
import { scheduleTerminalThemeUpdate, applyTerminalThemeSync, cancelTerminalThemeUpdate } from './terminalThemeScheduler';
|
||||
import { injectTerminalPaneAppearanceVars } from '../../infrastructure/theme/terminalAppearanceVars';
|
||||
import {
|
||||
isTerminalCloseGenerationCurrent,
|
||||
@@ -544,6 +544,7 @@ export function useTerminalEffects(ctx: TerminalEffectsContext) {
|
||||
if (!term) return;
|
||||
|
||||
if (isVisibleRef.current || isFocused) {
|
||||
cancelTerminalThemeUpdate(sessionId);
|
||||
applyTerminalThemeSync(term, effectiveTheme);
|
||||
injectTerminalPaneAppearanceVars(sessionId, effectiveTheme);
|
||||
return;
|
||||
|
||||
@@ -184,6 +184,7 @@ export function TerminalLayerTabBridge({ stableRef }: { stableRef: StableRef })
|
||||
focusedSessionId,
|
||||
fontSize: s.fontSize,
|
||||
hostMap: s.hostMap,
|
||||
isSidePanelOpenForCurrentTab,
|
||||
isVisible,
|
||||
onUpdateHost: s.onUpdateHost,
|
||||
onUpdateTerminalFontFamilyId: s.onUpdateTerminalFontFamilyId,
|
||||
|
||||
@@ -13,11 +13,18 @@ test("follow-app side panel theme changes delegate to ThemeRuntime pickTheme", (
|
||||
});
|
||||
|
||||
test("manual side panel theme changes persist host overrides and use runtime pick intent", () => {
|
||||
assert.match(source, /pickTheme\(themeId, \{ followApp: false \}\)/);
|
||||
assert.match(source, /pickTheme\(themeId, \{/);
|
||||
assert.match(source, /scopeHostId:/);
|
||||
assert.match(source, /onUpdateHost\(\{ \.\.\.rawFocusedHost, theme: themeId, themeOverride: true \}\)/);
|
||||
assert.doesNotMatch(source, /startTransition\(\(\) => \{[\s\S]*onUpdateHost\(\{ \.\.\.rawFocusedHost, theme: themeId/);
|
||||
});
|
||||
|
||||
test("follow-app keeps runtime intent until the side panel closes", () => {
|
||||
assert.match(source, /isSidePanelOpenForCurrentTab/);
|
||||
assert.match(source, /if \(!followAppTerminalTheme && activeSidePanelTab !== 'theme'\)/);
|
||||
assert.match(source, /clearIntent\(\)/);
|
||||
});
|
||||
|
||||
test("follow-app theme list selection tracks global runtime theme id", () => {
|
||||
assert.match(source, /listSelectedThemeId = followAppTerminalTheme/);
|
||||
assert.match(source, /terminalTheme\.id/);
|
||||
@@ -30,6 +37,6 @@ test("manual theme list selection reads focused appearance from runtime", () =>
|
||||
});
|
||||
|
||||
test("closing the theme tab clears runtime user intent", () => {
|
||||
assert.match(source, /if \(activeSidePanelTab !== 'theme'\) \{/);
|
||||
assert.match(source, /if \(isSidePanelOpenForCurrentTab\)/);
|
||||
assert.match(source, /clearIntent\(\)/);
|
||||
});
|
||||
@@ -33,6 +33,7 @@ interface UseTerminalThemePanelStateOptions {
|
||||
focusedSessionId: string | undefined;
|
||||
fontSize: number;
|
||||
hostMap: Map<string, Host>;
|
||||
isSidePanelOpenForCurrentTab: boolean;
|
||||
isVisible: boolean;
|
||||
onUpdateHost: (host: Host) => void;
|
||||
onUpdateTerminalFontFamilyId?: (fontFamilyId: string) => void;
|
||||
@@ -58,6 +59,7 @@ export function useTerminalThemePanelState({
|
||||
focusedSessionId,
|
||||
fontSize,
|
||||
hostMap,
|
||||
isSidePanelOpenForCurrentTab,
|
||||
isVisible,
|
||||
onUpdateHost,
|
||||
onUpdateTerminalFontFamilyId,
|
||||
@@ -74,10 +76,14 @@ export function useTerminalThemePanelState({
|
||||
terminalTheme,
|
||||
}: UseTerminalThemePanelStateOptions) {
|
||||
useEffect(() => {
|
||||
if (activeSidePanelTab !== 'theme') {
|
||||
clearIntent();
|
||||
if (isSidePanelOpenForCurrentTab) {
|
||||
if (!followAppTerminalTheme && activeSidePanelTab !== 'theme') {
|
||||
clearIntent();
|
||||
}
|
||||
return;
|
||||
}
|
||||
}, [activeSidePanelTab, clearIntent]);
|
||||
clearIntent();
|
||||
}, [activeSidePanelTab, clearIntent, followAppTerminalTheme, isSidePanelOpenForCurrentTab]);
|
||||
|
||||
const focusedHost = useMemo((): Host | null => {
|
||||
if (activeWorkspace && focusedSessionId) {
|
||||
@@ -168,7 +174,10 @@ export function useTerminalThemePanelState({
|
||||
return;
|
||||
}
|
||||
|
||||
pickTheme(themeId, { followApp: false });
|
||||
pickTheme(themeId, {
|
||||
followApp: false,
|
||||
scopeHostId: rawFocusedHost?.id ?? focusedHost?.id ?? null,
|
||||
});
|
||||
if (isFocusedHostEphemeral) {
|
||||
onUpdateTerminalThemeId?.(themeId);
|
||||
} else if (rawFocusedHost) {
|
||||
|
||||
@@ -130,7 +130,7 @@ test('manual mode intent overrides host theme while picking', () => {
|
||||
} as Host;
|
||||
|
||||
const appearance = resolveTerminalAppearance({
|
||||
userIntent: pickingThemeUserIntent('solarized-light'),
|
||||
userIntent: pickingThemeUserIntent('solarized-light', { scopeHostId: 'host-1' }),
|
||||
settings: {
|
||||
...baseSettings,
|
||||
followAppTerminalTheme: false,
|
||||
@@ -143,6 +143,35 @@ test('manual mode intent overrides host theme while picking', () => {
|
||||
assert.equal(appearance.source, 'intent');
|
||||
});
|
||||
|
||||
test('manual mode intent stays scoped to the picked host', () => {
|
||||
const focusedHost = {
|
||||
id: 'host-1',
|
||||
theme: 'dracula',
|
||||
themeOverride: true,
|
||||
} as Host;
|
||||
const otherHost = {
|
||||
id: 'host-2',
|
||||
theme: 'netcatty-dark',
|
||||
themeOverride: true,
|
||||
} as Host;
|
||||
|
||||
const intent = pickingThemeUserIntent('solarized-light', { scopeHostId: 'host-1' });
|
||||
|
||||
assert.equal(resolveTerminalAppearance({
|
||||
userIntent: intent,
|
||||
settings: { ...baseSettings, followAppTerminalTheme: false },
|
||||
hostScope: { host: focusedHost, isEphemeral: false },
|
||||
customThemes: [],
|
||||
}).themeId, 'solarized-light');
|
||||
|
||||
assert.equal(resolveTerminalAppearance({
|
||||
userIntent: intent,
|
||||
settings: { ...baseSettings, followAppTerminalTheme: false },
|
||||
hostScope: { host: otherHost, isEphemeral: false },
|
||||
customThemes: [],
|
||||
}).themeId, 'netcatty-dark');
|
||||
});
|
||||
|
||||
test('follow-app session appearance ignores host override', () => {
|
||||
const host = {
|
||||
id: 'host-1',
|
||||
|
||||
@@ -12,7 +12,7 @@ import { TERMINAL_THEMES, getBuiltinTerminalThemeById } from '../infrastructure/
|
||||
|
||||
export type ThemeUserIntent =
|
||||
| { kind: 'idle' }
|
||||
| { kind: 'picking'; themeId: string; startedAt: number };
|
||||
| { kind: 'picking'; themeId: string; startedAt: number; scopeHostId?: string | null };
|
||||
|
||||
export type AppearanceSource = 'intent' | 'follow-app' | 'host-override' | 'manual-global';
|
||||
|
||||
@@ -49,10 +49,14 @@ export type ResolveTerminalAppearanceInput = {
|
||||
|
||||
export const idleThemeUserIntent = (): ThemeUserIntent => ({ kind: 'idle' });
|
||||
|
||||
export const pickingThemeUserIntent = (themeId: string, startedAt = Date.now()): ThemeUserIntent => ({
|
||||
export const pickingThemeUserIntent = (
|
||||
themeId: string,
|
||||
options?: { startedAt?: number; scopeHostId?: string | null },
|
||||
): ThemeUserIntent => ({
|
||||
kind: 'picking',
|
||||
themeId,
|
||||
startedAt,
|
||||
startedAt: options?.startedAt ?? Date.now(),
|
||||
...(options?.scopeHostId !== undefined ? { scopeHostId: options.scopeHostId } : {}),
|
||||
});
|
||||
|
||||
export function isFollowAppIntentSettled(
|
||||
@@ -99,10 +103,15 @@ function resolveManualThemeId(
|
||||
hostScope: TerminalAppearanceHostScope,
|
||||
): { themeId: string; source: AppearanceSource } {
|
||||
if (userIntent.kind === 'picking') {
|
||||
if (hostScope.isEphemeral || !hostScope.host) {
|
||||
const scopeHostId = userIntent.scopeHostId ?? null;
|
||||
const hostId = hostScope.host?.id ?? null;
|
||||
if (scopeHostId === null) {
|
||||
if (hostScope.isEphemeral || !hostScope.host) {
|
||||
return { themeId: userIntent.themeId, source: 'intent' };
|
||||
}
|
||||
} else if (hostId === scopeHostId) {
|
||||
return { themeId: userIntent.themeId, source: 'intent' };
|
||||
}
|
||||
return { themeId: userIntent.themeId, source: 'intent' };
|
||||
}
|
||||
|
||||
const globalThemeId = resolveManualTerminalThemeId({
|
||||
|
||||
Reference in new issue
Block a user