From cc2ceec2fa3659109b2d7f89ab90edb29ecef91c Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 10 Jul 2026 20:41:00 -0700 Subject: [PATCH] fix(terminal): send mode 2031 replies immediately --- .../terminal-pane/terminal-appearance.test.ts | 29 +++---- .../terminal-pane/terminal-appearance.ts | 31 +------- .../terminal-mode-2031-replies.test.ts | 68 +++++++++++++++++ .../terminal-mode-2031-replies.ts | 75 +++++++++++++++++++ .../use-terminal-pane-lifecycle.ts | 49 +++++------- 5 files changed, 177 insertions(+), 75 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/terminal-mode-2031-replies.test.ts create mode 100644 src/renderer/src/components/terminal-pane/terminal-mode-2031-replies.ts diff --git a/src/renderer/src/components/terminal-pane/terminal-appearance.test.ts b/src/renderer/src/components/terminal-pane/terminal-appearance.test.ts index b7e088ff4aa..d0c060122af 100644 --- a/src/renderer/src/components/terminal-pane/terminal-appearance.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-appearance.test.ts @@ -6,10 +6,10 @@ import { applyTerminalAppearance, hexToRgba, installMode2031Handlers, - maybePushMode2031Flip, - mode2031SequenceFor, publishTerminalViewAttributesAtAppStart } from './terminal-appearance' +import { maybePushMode2031Flip } from './terminal-mode-2031-replies' +import { mode2031SequenceFor } from '../../../../shared/terminal-color-scheme-protocol' import { replayIntoTerminal, type ReplayingPanesRef } from './replay-guard' import { _resetTerminalViewAttributesPublisherForTest } from './terminal-view-attributes-publisher' import type { TerminalViewAttributes } from '../../../../shared/terminal-view-attributes' @@ -17,12 +17,14 @@ import type { TerminalViewAttributes } from '../../../../shared/terminal-view-at function fakeTransport(overrides?: { connected?: boolean; sendOk?: boolean }): { isConnected: () => boolean sendInput: ReturnType boolean>> + sendInputImmediate: ReturnType boolean>> } { const connected = overrides?.connected ?? true const sendOk = overrides?.sendOk ?? true return { isConnected: () => connected, - sendInput: vi.fn<(data: string) => boolean>(() => sendOk) + sendInput: vi.fn<(data: string) => boolean>(() => sendOk), + sendInputImmediate: vi.fn<(data: string) => boolean>(() => sendOk) } } @@ -42,7 +44,7 @@ describe('maybePushMode2031Flip', () => { const pushed = maybePushMode2031Flip(1, 'dark', transport, subs, last) expect(pushed).toBe(false) - expect(transport.sendInput).not.toHaveBeenCalled() + expect(transport.sendInputImmediate).not.toHaveBeenCalled() expect(last.has(1)).toBe(false) }) @@ -54,8 +56,9 @@ describe('maybePushMode2031Flip', () => { const pushed = maybePushMode2031Flip(1, 'dark', transport, subs, last) expect(pushed).toBe(true) - expect(transport.sendInput).toHaveBeenCalledTimes(1) - expect(transport.sendInput).toHaveBeenCalledWith('\x1b[?997;1n') + expect(transport.sendInputImmediate).toHaveBeenCalledTimes(1) + expect(transport.sendInputImmediate).toHaveBeenCalledWith('\x1b[?997;1n') + expect(transport.sendInput).not.toHaveBeenCalled() expect(last.get(1)).toBe('dark') }) @@ -70,7 +73,7 @@ describe('maybePushMode2031Flip', () => { maybePushMode2031Flip(1, 'dark', transport, subs, last) maybePushMode2031Flip(1, 'dark', transport, subs, last) - expect(transport.sendInput).toHaveBeenCalledTimes(1) + expect(transport.sendInputImmediate).toHaveBeenCalledTimes(1) expect(last.get(1)).toBe('dark') }) @@ -83,7 +86,7 @@ describe('maybePushMode2031Flip', () => { maybePushMode2031Flip(1, 'light', transport, subs, last) maybePushMode2031Flip(1, 'dark', transport, subs, last) - expect(transport.sendInput.mock.calls.map((c) => c[0])).toEqual([ + expect(transport.sendInputImmediate.mock.calls.map((c) => c[0])).toEqual([ '\x1b[?997;1n', '\x1b[?997;2n', '\x1b[?997;1n' @@ -99,11 +102,11 @@ describe('maybePushMode2031Flip', () => { const pushed = maybePushMode2031Flip(1, 'dark', transport, subs, last) expect(pushed).toBe(false) - expect(transport.sendInput).not.toHaveBeenCalled() + expect(transport.sendInputImmediate).not.toHaveBeenCalled() expect(last.has(1)).toBe(false) }) - it('leaves last-mode untouched when sendInput reports failure', () => { + it('leaves last-mode untouched when immediate input reports failure', () => { // So a reconnect / retry will re-emit on the next appearance pass. const transport = fakeTransport({ sendOk: false }) const subs = new Map([[1, true]]) @@ -112,7 +115,7 @@ describe('maybePushMode2031Flip', () => { const pushed = maybePushMode2031Flip(1, 'dark', transport, subs, last) expect(pushed).toBe(false) - expect(transport.sendInput).toHaveBeenCalledTimes(1) + expect(transport.sendInputImmediate).toHaveBeenCalledTimes(1) expect(last.has(1)).toBe(false) }) @@ -130,8 +133,8 @@ describe('maybePushMode2031Flip', () => { maybePushMode2031Flip(1, 'dark', transportA, subs, last) // suppressed maybePushMode2031Flip(2, 'dark', transportB, subs, last) // flip - expect(transportA.sendInput).toHaveBeenCalledTimes(1) - expect(transportB.sendInput).toHaveBeenCalledTimes(2) + expect(transportA.sendInputImmediate).toHaveBeenCalledTimes(1) + expect(transportB.sendInputImmediate).toHaveBeenCalledTimes(2) expect(last.get(1)).toBe('dark') expect(last.get(2)).toBe('dark') }) diff --git a/src/renderer/src/components/terminal-pane/terminal-appearance.ts b/src/renderer/src/components/terminal-pane/terminal-appearance.ts index a34ac215e66..d1af2c9f1af 100644 --- a/src/renderer/src/components/terminal-pane/terminal-appearance.ts +++ b/src/renderer/src/components/terminal-pane/terminal-appearance.ts @@ -1,7 +1,6 @@ import type { IDisposable, IParser, ITheme } from '@xterm/xterm' import type { PaneManager } from '@/lib/pane-manager/pane-manager' import type { GlobalSettings } from '../../../../shared/types' -import { mode2031SequenceFor } from '../../../../shared/terminal-color-scheme-protocol' import { resolveTerminalFontWeights } from '../../../../shared/terminal-fonts' import { resolveTerminalLigaturesEnabled } from '../../../../shared/terminal-ligatures' import { @@ -24,8 +23,7 @@ import { HEX_COLOR_RE } from '../../../../shared/color-validation' import type { TerminalViewAttributes } from '../../../../shared/terminal-view-attributes' import { publishTerminalViewAttributes } from './terminal-view-attributes-publisher' import { normalizeTerminalLineHeight } from '../../../../shared/terminal-line-height-settings' - -export { mode2031SequenceFor } +import { maybePushMode2031Flip } from './terminal-mode-2031-replies' // Why Pick over a hand-rolled structural type: keeps the helper // tied to xterm's canonical signature so any upstream tightening (added @@ -112,33 +110,6 @@ export function installMode2031Handlers(deps: Mode2031HandlerDeps): IDisposable[ ] } -// Gate on actual mode flip so font/size/opacity tweaks — which also re-run -// applyTerminalAppearance — don't spam subscribed TUIs with CSI 997. The -// subscribe/last-mode maps are mutated in place so callers share state with -// the lifecycle hook's seed path. -export function maybePushMode2031Flip( - paneId: number, - mode: 'dark' | 'light', - transport: Pick, - paneMode2031: Map, - paneLastThemeMode: Map -): boolean { - if (!transport.isConnected()) { - return false - } - if (!paneMode2031.get(paneId)) { - return false - } - if (paneLastThemeMode.get(paneId) === mode) { - return false - } - if (!transport.sendInput(mode2031SequenceFor(mode))) { - return false - } - paneLastThemeMode.set(paneId, mode) - return true -} - export function hexToRgba(hex: string, alpha: number): string { let clean = hex.replace('#', '') if (clean.length === 3) { diff --git a/src/renderer/src/components/terminal-pane/terminal-mode-2031-replies.test.ts b/src/renderer/src/components/terminal-pane/terminal-mode-2031-replies.test.ts new file mode 100644 index 00000000000..e3da0b861af --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-mode-2031-replies.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it, vi } from 'vitest' +import { pushMode2031SeedReply } from './terminal-mode-2031-replies' + +describe('pushMode2031SeedReply', () => { + function createHarness(connected: boolean): { + connected: { current: boolean } + subscribed: { current: boolean } + sendInput: ReturnType boolean>> + sendInputImmediate: ReturnType boolean>> + scheduled: (() => void)[] + recordMode: ReturnType + push: () => void + } { + const connectedState = { current: connected } + const subscribed = { current: true } + const sendInput = vi.fn<(data: string) => boolean>(() => true) + const sendInputImmediate = vi.fn<(data: string) => boolean>(() => true) + const scheduled: (() => void)[] = [] + const recordMode = vi.fn() + const transport = { + isConnected: () => connectedState.current, + sendInput, + sendInputImmediate + } + return { + connected: connectedState, + subscribed, + sendInput, + sendInputImmediate, + scheduled, + recordMode, + push: () => + pushMode2031SeedReply(1, { + hasPane: () => true, + isSubscribed: () => subscribed.current, + getTransport: () => transport, + getMode: () => 'dark', + recordMode, + schedule: (callback) => scheduled.push(callback) + }) + } + } + + it('routes the DSR response through latency-critical input', () => { + const harness = createHarness(true) + + harness.push() + + expect(harness.sendInputImmediate).toHaveBeenCalledWith('\x1b[?997;1n') + expect(harness.sendInput).not.toHaveBeenCalled() + expect(harness.recordMode).toHaveBeenCalledWith(1, 'dark') + }) + + it('cancels a pre-connect retry after the program unsubscribes', () => { + const harness = createHarness(false) + + harness.push() + expect(harness.scheduled).toHaveLength(1) + + harness.subscribed.current = false + harness.connected.current = true + harness.scheduled.shift()?.() + + expect(harness.sendInputImmediate).not.toHaveBeenCalled() + expect(harness.sendInput).not.toHaveBeenCalled() + expect(harness.scheduled).toHaveLength(0) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/terminal-mode-2031-replies.ts b/src/renderer/src/components/terminal-pane/terminal-mode-2031-replies.ts new file mode 100644 index 00000000000..455a15f528a --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-mode-2031-replies.ts @@ -0,0 +1,75 @@ +import { mode2031SequenceFor } from '../../../../shared/terminal-color-scheme-protocol' +import type { TerminalColorSchemeMode } from '../../../../shared/terminal-color-scheme-protocol' +import type { PtyTransport } from './pty-transport' + +const MODE_2031_CONNECT_RETRY_MS = 25 +const MODE_2031_CONNECT_ATTEMPTS = 8 + +type Mode2031ReplyTransport = Pick + +type Mode2031SeedReplyDeps = { + hasPane: (paneId: number) => boolean + isSubscribed: (paneId: number) => boolean + getTransport: (paneId: number) => Mode2031ReplyTransport | undefined + getMode: () => TerminalColorSchemeMode | null + recordMode: (paneId: number, mode: TerminalColorSchemeMode) => void + schedule: (callback: () => void, delayMs: number) => void +} + +function sendMode2031Reply( + transport: Mode2031ReplyTransport, + mode: TerminalColorSchemeMode +): boolean { + // Why: fish stops reading mode-2031 replies quickly; remote input batching + // can otherwise deliver this terminal response after the shell regains input. + return transport.sendInputImmediate(mode2031SequenceFor(mode)) +} + +export function pushMode2031SeedReply(paneId: number, deps: Mode2031SeedReplyDeps): void { + let attempts = 0 + const send = (): void => { + // Why: a TUI can unsubscribe while the PTY is connecting; every delayed + // attempt must revalidate intent or its DSR reply can land at a shell prompt. + if (!deps.hasPane(paneId) || !deps.isSubscribed(paneId)) { + return + } + const transport = deps.getTransport(paneId) + if (!transport?.isConnected()) { + attempts += 1 + if (attempts < MODE_2031_CONNECT_ATTEMPTS) { + deps.schedule(send, MODE_2031_CONNECT_RETRY_MS) + } + return + } + const mode = deps.getMode() + if (mode && sendMode2031Reply(transport, mode)) { + deps.recordMode(paneId, mode) + } + } + send() +} + +// Appearance updates include font and opacity changes, so only report actual +// color-mode flips to programs that still have mode 2031 enabled. +export function maybePushMode2031Flip( + paneId: number, + mode: TerminalColorSchemeMode, + transport: Mode2031ReplyTransport, + paneMode2031: Map, + paneLastThemeMode: Map +): boolean { + if (!transport.isConnected()) { + return false + } + if (!paneMode2031.get(paneId)) { + return false + } + if (paneLastThemeMode.get(paneId) === mode) { + return false + } + if (!sendMode2031Reply(transport, mode)) { + return false + } + paneLastThemeMode.set(paneId, mode) + return true +} diff --git a/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts b/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts index b0e2cd10ef8..d69f374c668 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts @@ -58,11 +58,8 @@ import { import { resolveTerminalLayoutActiveLeafId } from './terminal-layout-leaf-ids' import { makePaneKey } from '../../../../shared/stable-pane-id' import { applyExpandedLayoutTo, restoreExpandedLayoutFrom } from './expand-collapse' -import { - applyTerminalAppearance, - installMode2031Handlers, - mode2031SequenceFor -} from './terminal-appearance' +import { applyTerminalAppearance, installMode2031Handlers } from './terminal-appearance' +import { pushMode2031SeedReply } from './terminal-mode-2031-replies' import { handleOsc52ClipboardRequest } from './osc52-clipboard' import { showOsc52ClipboardBlockedToast } from './osc52-clipboard-blocked-toast' import { parseOsc7 } from './parse-osc7' @@ -604,34 +601,22 @@ export function useTerminalPaneLifecycle({ } const pushMode2031ForPane = (paneId: number): void => { - let attempts = 0 - const send = (): void => { - if (!managerRef.current?.getPanes().some((pane) => pane.id === paneId)) { - return + pushMode2031SeedReply(paneId, { + hasPane: (candidateId) => + managerRef.current?.getPanes().some((pane) => pane.id === candidateId) === true, + isSubscribed: (candidateId) => paneMode2031Ref.current.get(candidateId) === true, + getTransport: (candidateId) => paneTransportsRef.current.get(candidateId), + getMode: () => { + const currentSettings = settingsRef.current + return currentSettings + ? resolveEffectiveTerminalAppearance(currentSettings, systemPrefersDarkRef.current).mode + : null + }, + recordMode: (candidateId, mode) => paneLastThemeModeRef.current.set(candidateId, mode), + schedule: (callback, delayMs) => { + window.setTimeout(callback, delayMs) } - const transport = paneTransportsRef.current.get(paneId) - if (!transport?.isConnected()) { - // Why: TUIs can subscribe before pty:spawn resolves. Retry briefly so - // the recorded subscription still receives the initial dark/light seed. - attempts += 1 - if (attempts < 8) { - window.setTimeout(send, 25) - } - return - } - const currentSettings = settingsRef.current - if (!currentSettings) { - return - } - const { mode } = resolveEffectiveTerminalAppearance( - currentSettings, - systemPrefersDarkRef.current - ) - if (transport.sendInput(mode2031SequenceFor(mode))) { - paneLastThemeModeRef.current.set(paneId, mode) - } - } - send() + }) } // Initialize PaneManager instance once