From bfbeab6204bb4e9da5c2159e6ad013b75d03a977 Mon Sep 17 00:00:00 2001 From: Brennan Benson Date: Sun, 12 Jul 2026 15:34:34 -0700 Subject: [PATCH] fix(terminal): make teardown ownership generation-safe --- config/reliability-gates.jsonc | 69 +++++++- src/main/ipc/pty.test.ts | 10 +- src/main/ipc/pty.ts | 96 ++++++----- src/main/ipc/settings.test.ts | 5 +- src/main/providers/local-pty-provider.ts | 10 +- src/main/providers/ssh-pty-provider.test.ts | 33 ++++ src/main/providers/ssh-pty-provider.ts | 31 +++- src/main/providers/types.ts | 10 +- src/main/runtime/orca-runtime.test.ts | 45 ++++- src/main/runtime/orca-runtime.ts | 62 ++++++- src/main/runtime/rpc/methods/client-ui.ts | 4 +- src/main/ssh/ssh-relay-session.ts | 18 +- src/preload/api-types.ts | 9 +- src/preload/index.ts | 6 +- src/relay/pty-handler.test.ts | 51 +++++- src/relay/pty-handler.ts | 27 ++- src/relay/relay.ts | 5 + src/relay/subprocess.test.ts | 4 + src/renderer/src/assets/main.css | 3 +- .../src/components/editor/DiffViewer.tsx | 14 +- .../src/components/editor/EditorPanel.tsx | 104 ++++++------ .../components/editor/EditorPanelHeader.tsx | 50 +++++- .../src/components/editor/MonacoEditor.tsx | 14 +- .../editor/diff-navigation-context.test.tsx | 156 ++++++++++++++++++ .../editor/diff-navigation-context.tsx | 115 +++++++++++++ .../file-editor-word-wrap-options.test.ts | 13 ++ .../editor/file-editor-word-wrap-options.ts | 8 + .../settings/EditorWordWrapSetting.test.tsx | 85 ++++++++++ .../settings/EditorWordWrapSetting.tsx | 69 ++++++++ .../settings/GeneralEditorSettingsSection.tsx | 3 + .../components/settings/GeneralPane.test.ts | 7 + .../settings/general-editor-search.ts | 11 ++ .../pty-pre-handler-buffer.test.ts | 23 ++- .../terminal-pane/pty-pre-handler-buffer.ts | 8 + .../terminal-pane/pty-transport.test.ts | 16 +- .../components/terminal-pane/pty-transport.ts | 18 +- .../terminal-shortcut-policy.test.ts | 13 +- .../terminal-pane/terminal-shortcut-policy.ts | 6 +- src/renderer/src/i18n/locales/en.json | 12 +- src/renderer/src/i18n/locales/es.json | 12 +- src/renderer/src/i18n/locales/ja.json | 12 +- src/renderer/src/i18n/locales/ko.json | 12 +- src/renderer/src/i18n/locales/zh.json | 12 +- .../lib/ephemeral-vm-runtime-cleanup.test.ts | 11 ++ .../src/lib/ephemeral-vm-runtime-cleanup.ts | 2 +- .../src/lib/pr-bot-author-overrides.test.ts | 4 +- .../src/lib/pty-kill-retry-ownership.test.ts | 37 +++++ .../src/lib/pty-kill-retry-ownership.ts | 45 ++++- src/renderer/src/store/slices/repos.test.ts | 4 +- src/renderer/src/store/slices/repos.ts | 3 +- src/shared/constants.test.ts | 4 + src/shared/constants.ts | 1 + src/shared/ssh-pty-id.test.ts | 15 ++ src/shared/ssh-pty-id.ts | 29 +++- src/shared/types.ts | 2 + tests/e2e/terminal-shortcuts.spec.ts | 39 ++--- 56 files changed, 1274 insertions(+), 213 deletions(-) create mode 100644 src/renderer/src/components/editor/diff-navigation-context.test.tsx create mode 100644 src/renderer/src/components/editor/diff-navigation-context.tsx create mode 100644 src/renderer/src/components/editor/file-editor-word-wrap-options.test.ts create mode 100644 src/renderer/src/components/editor/file-editor-word-wrap-options.ts create mode 100644 src/renderer/src/components/settings/EditorWordWrapSetting.test.tsx create mode 100644 src/renderer/src/components/settings/EditorWordWrapSetting.tsx create mode 100644 src/renderer/src/lib/pty-kill-retry-ownership.test.ts diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index ab752c1eb4e..70da4dd1c86 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -3600,16 +3600,17 @@ "providers": ["ssh", "remote-runtime"], "coveredPlatforms": ["macos"], "coveredProviders": ["ssh"], - "coverageNotes": "A macOS unit harness loads the installed patched node-pty WindowsTerminal prototype without constructing native state, proving pre-ready kill/destroy dispatch immediately. A relay artifact harness reproduces vanilla npm node-pty retaining one deferred kill with zero native close calls, then proves both Windows relay artifacts hash, ship, apply, include the local ConPTY console-list fallback, and idempotently verify the fix. Relay tests simulate win32 shutdown, repeated stale-spawn native failure, one-shot ownership, and retry after native failure. Renderer tests retain exact local/SSH kill identity across rejection and retry, while project removal keeps local or remote ownership state until provider-verified shutdown and VM cleanup succeed. Runtime tests prove terminal.stop awaits stopAndWait plus a fresh post-stop liveness check. The same relay contract serves paired web/mobile clients; no live Windows ConPTY or Windows-over-SSH run is recorded yet.", + "coverageNotes": "A macOS unit harness loads the installed patched node-pty WindowsTerminal prototype without constructing native state, proving pre-ready kill/destroy dispatch immediately. A relay artifact harness reproduces vanilla npm node-pty retaining one deferred kill with zero native close calls, then proves both Windows relay artifacts hash, ship, apply, include the local ConPTY console-list fallback, and idempotently verify the fix. Relay tests simulate win32 shutdown, repeated stale-spawn native failure, one-shot ownership, and retry after native failure without deleting the only handle before close is proven. Relay boot UUIDs scope recycled pty-N counters, while pane identity protects older relay shutdown. Renderer pre-handler exits and retained shutdown retries are capped at 64, and lifecycle retry performs at most one attempt with backoff. Project removal keeps local or remote ownership until provider-verified shutdown and successful VM cleanup; runtime repo removal holds terminal admission closed across fresh liveness, stop, and metadata mutation. Targeted terminal.stop probes skip unrelated sessions. The same relay contract serves paired web/mobile clients; no live Windows ConPTY or Windows-over-SSH run is recorded yet.", "motivatingLinks": [ "https://github.com/stablyai/orca/issues/8260", "https://github.com/stablyai/orca/pull/8279" ], - "invariant": "A Windows relay PTY closed before its first output executes exactly one no-argument node-pty shutdown immediately; Orca does not delete ownership for a native kill failure, and a later retry can still target the same managed PTY.", + "invariant": "A Windows relay PTY closed before its first output executes exactly one no-argument node-pty shutdown immediately; Orca does not delete ownership for a native kill failure, and a later retry can target only the same relay generation and pane while project removal fences new terminal admission.", "oracle": "Load the installed patched WindowsTerminal prototype in a pre-ready state and assert kill and destroy each close the terminal and native agent once with zero deferred work, while signal arguments still throw. Build both Windows SSH relay artifacts, reproduce vanilla npm node-pty with close=0/native-kill=0/deferred=1, apply the shipped patch, and assert close=1/native-kill=1/deferred=0, AttachConsole failure falls back to the shell pid, and the artifact version derives from all patch bytes. Assert remote npm install executes patch enforcement before its runtime probe and never finalizes a failed patch. In the relay, assert immediate, graceful, stale-spawn fallback, and handler-dispose paths preserve ownership through native throw and issue at most one successful no-argument shutdown. In the renderer, reject the first kill, retain exact identity, and assert the next lifecycle/removal retry succeeds before ownership state is purged.", "commands": [ "pnpm exec vitest run --config config/vitest.config.ts config/scripts/build-relay-windows-node-pty-patch.test.mjs src/main/ssh/ssh-relay-native-deps-install.test.ts src/relay/node-pty-windows-pre-ready-shutdown.test.ts src/relay/pty-handler.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts src/renderer/src/store/slices/repos.test.ts src/renderer/src/store/slices/repos-runtime-vm-removal.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts", - "pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/orca-runtime.test.ts" + "pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/orca-runtime.test.ts", + "pnpm exec vitest run --config config/vitest.config.ts src/shared/ssh-pty-id.test.ts src/main/providers/ssh-pty-provider.test.ts src/main/ssh/ssh-relay-session.test.ts src/relay/pty-handler.test.ts src/relay/subprocess.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts src/renderer/src/lib/pty-kill-retry-ownership.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts src/renderer/src/store/slices/repos.test.ts src/main/runtime/orca-runtime.test.ts src/main/ipc/pty.test.ts" ], "testFiles": [ "config/scripts/build-relay-windows-node-pty-patch.test.mjs", @@ -3621,7 +3622,14 @@ "src/renderer/src/store/slices/repos.test.ts", "src/renderer/src/store/slices/repos-runtime-vm-removal.test.ts", "src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts", - "src/main/runtime/orca-runtime.test.ts" + "src/main/runtime/orca-runtime.test.ts", + "src/shared/ssh-pty-id.test.ts", + "src/main/providers/ssh-pty-provider.test.ts", + "src/main/ssh/ssh-relay-session.test.ts", + "src/relay/subprocess.test.ts", + "src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts", + "src/renderer/src/lib/pty-kill-retry-ownership.test.ts", + "src/main/ipc/pty.test.ts" ], "assertionRefs": [ { @@ -3688,7 +3696,45 @@ "file": "src/main/runtime/orca-runtime.test.ts", "assertions": [ "terminal.stop awaits stopAndWait and fresh post-stop liveness before acknowledging provider shutdown", - "provider shutdown rejection fails terminal.stop closed" + "provider shutdown rejection fails terminal.stop closed", + "project removal fences paired terminal admission through provider stop and repo mutation", + "targeted terminal stop omits foreground probes for unrelated sessions" + ] + }, + { + "file": "src/shared/ssh-pty-id.test.ts", + "assertions": [ + "the same relay counter in two boot generations produces distinct app PTY ids" + ] + }, + { + "file": "src/main/providers/ssh-pty-provider.test.ts", + "assertions": [ + "SSH shutdown forwards pane identity and rejects an older relay generation before RPC" + ] + }, + { + "file": "src/relay/subprocess.test.ts", + "assertions": [ + "relay.status publishes one stable UUID for the relay process lifetime" + ] + }, + { + "file": "src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts", + "assertions": [ + "ownerless pre-handler exits evict oldest state at the 64-id bound" + ] + }, + { + "file": "src/renderer/src/lib/pty-kill-retry-ownership.test.ts", + "assertions": [ + "retained shutdown ids are capped at 64 and one lifecycle retries at most one id" + ] + }, + { + "file": "src/main/ipc/pty.test.ts", + "assertions": [ + "renderer pane identity reaches SSH provider shutdown" ] } ], @@ -3710,10 +3756,19 @@ "result": "passed", "durationSeconds": 10, "summary": "662 runtime tests passed, including provider-verified terminal.stop acceptance and post-stop liveness coverage." + }, + { + "date": "2026-07-12", + "runner": "local", + "platform": "macos", + "command": "pnpm exec vitest run --config config/vitest.config.ts src/shared/ssh-pty-id.test.ts src/main/providers/ssh-pty-provider.test.ts src/main/ssh/ssh-relay-session.test.ts src/relay/pty-handler.test.ts src/relay/subprocess.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts src/renderer/src/lib/pty-kill-retry-ownership.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts src/renderer/src/store/slices/repos.test.ts src/main/runtime/orca-runtime.test.ts src/main/ipc/pty.test.ts", + "result": "passed", + "durationSeconds": 12, + "summary": "1236 tests passed with relay-generation identity, stale-spawn retained ownership, bounded renderer maps/retry work, VM cleanup success ordering, atomic runtime removal admission, and targeted liveness probes." } ], "runtimeBudget": { - "p95Seconds": 5, + "p95Seconds": 20, "scope": "dependency and relay unit tests" }, "flakeHistory": { @@ -3726,7 +3781,7 @@ }, "performanceBudget": { "required": true, - "evidence": "Shutdown removes indefinite deferred retention and preserves at most one in-flight native kill plus one existing graceful timer per PTY request. Failed renderer kills retain a bounded exact-id entry and retry only on a later PTY lifecycle event; no polling, new permanent listener, startup await, or steady-state work is added." + "evidence": "Shutdown removes indefinite deferred retention and preserves at most one in-flight native kill plus one existing graceful timer per PTY request. Failed renderer kills retain at most 64 generation-scoped ids; one lifecycle event attempts at most one retry with exponential backoff. Pre-handler exits are capped at 64. Targeted runtime stop still performs one provider verification per target PTY plus two liveness snapshots, while foreground probes are limited to the target worktree." }, "promotionCriteria": [ "Run the deterministic command across the required CI platforms without flakes.", diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 35d386eb473..82f1c06f4da 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -3966,11 +3966,17 @@ describe('registerPtyHandlers', () => { store as never ) - await handlers.get('pty:kill')!(null, { id: 'ssh:ssh-1@@relay-pty' }) + await handlers.get('pty:kill')!(null, { + id: 'ssh:ssh-1@@relay-pty', + expectedPaneKey: 'tab-a:leaf-a', + expectedTabId: 'tab-a' + }) expect(sshShutdown).toHaveBeenCalledWith('ssh:ssh-1@@relay-pty', { immediate: true, - keepHistory: false + keepHistory: false, + expectedPaneKey: 'tab-a:leaf-a', + expectedTabId: 'tab-a' }) expect(localShutdown).not.toHaveBeenCalled() expect(store.markSshRemotePtyLease).toHaveBeenCalledWith('ssh-1', 'relay-pty', 'terminated') diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 305ebabfc14..91f1bc1f77c 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -54,7 +54,12 @@ import { piTitlebarExtensionService } from '../pi/titlebar-extension-service' import { detectPiAgentKindFromCommand, type PiAgentKind } from '../../shared/pi-agent-kind' import { isPwshAvailable } from '../pwsh' import { LocalPtyProvider } from '../providers/local-pty-provider' -import type { IPtyProvider, PtySpawnOptions, PtySpawnResult } from '../providers/types' +import type { + IPtyProvider, + PtyShutdownOptions, + PtySpawnOptions, + PtySpawnResult +} from '../providers/types' import type { StartupCommandDelivery } from '../../shared/codex-startup-delivery' import { SSH_SESSION_EXPIRED_ERROR, @@ -2542,7 +2547,7 @@ export function registerPtyHandlers( async function shutdownProviderAndDetectExit( provider: IPtyProvider, id: string, - opts: { immediate?: boolean; keepHistory?: boolean } + opts: PtyShutdownOptions ): Promise { let providerExitObserved = false const unsubscribe = provider.onExit((payload) => { @@ -4938,46 +4943,59 @@ export function registerPtyHandlers( runtime?.clearHeadlessTerminalBuffer(args.id).catch(() => {}) }) - ipcMain.handle('pty:kill', async (_event, args: { id: string; keepHistory?: boolean }) => { - const ownedConnectionId = ptyOwnership.get(args.id) - const parsedSshId = ownedConnectionId === undefined ? parseAppSshPtyId(args.id) : null - const connectionId = ownedConnectionId ?? parsedSshId?.connectionId - const provider = connectionId ? sshProviders.get(connectionId) : tryGetProviderForPty(args.id) - if (!provider && connectionId) { - // Why: detached SSH PTYs intentionally keep ownership after their - // provider is unregistered; hydrated app-scoped ids can also arrive - // before ownership is rebuilt. Tombstone instead of falling back local. + ipcMain.handle( + 'pty:kill', + async ( + _event, + args: { + id: string + keepHistory?: boolean + expectedPaneKey?: string + expectedTabId?: string + } + ) => { + const ownedConnectionId = ptyOwnership.get(args.id) + const parsedSshId = ownedConnectionId === undefined ? parseAppSshPtyId(args.id) : null + const connectionId = ownedConnectionId ?? parsedSshId?.connectionId + const provider = connectionId ? sshProviders.get(connectionId) : tryGetProviderForPty(args.id) + if (!provider && connectionId) { + // Why: detached SSH PTYs intentionally keep ownership after their + // provider is unregistered; hydrated app-scoped ids can also arrive + // before ownership is rebuilt. Tombstone instead of falling back local. + finishPtyShutdown(args.id, connectionId, store) + runtime?.onPtyExit(args.id, -1) + rememberSyntheticKillExit(args.id) + sendPtyExitToRenderer({ id: args.id, code: -1 }) + return + } + const shutdownProvider = provider ?? getProviderForPty(args.id) + let providerExitObserved = false + try { + providerExitObserved = await shutdownProviderAndDetectExit(shutdownProvider, args.id, { + immediate: true, + keepHistory: args.keepHistory ?? false, + ...(args.expectedPaneKey ? { expectedPaneKey: args.expectedPaneKey } : {}), + ...(args.expectedTabId ? { expectedTabId: args.expectedTabId } : {}) + }) + } catch (err) { + if (!isPtyAlreadyGoneError(err)) { + // Why: a failed SSH shutdown can leave the remote process alive in + // the relay grace window; daemon failures have the same risk locally. + // Keep ownership/lease state so the user can retry. + throw err + } + /* session already dead — cleanup below handles the rest */ + } + // Why: some shutdown paths do not emit onExit through the provider listener. + // Explicit cleanup is idempotent and covers already-dead PTYs. finishPtyShutdown(args.id, connectionId, store) - runtime?.onPtyExit(args.id, -1) - rememberSyntheticKillExit(args.id) - sendPtyExitToRenderer({ id: args.id, code: -1 }) - return - } - const shutdownProvider = provider ?? getProviderForPty(args.id) - let providerExitObserved = false - try { - providerExitObserved = await shutdownProviderAndDetectExit(shutdownProvider, args.id, { - immediate: true, - keepHistory: args.keepHistory ?? false - }) - } catch (err) { - if (!isPtyAlreadyGoneError(err)) { - // Why: a failed SSH shutdown can leave the remote process alive in - // the relay grace window; daemon failures have the same risk locally. - // Keep ownership/lease state so the user can retry. - throw err + if (!providerExitObserved) { + runtime?.onPtyExit(args.id, -1) + rememberSyntheticKillExit(args.id) + sendPtyExitToRenderer({ id: args.id, code: -1 }) } - /* session already dead — cleanup below handles the rest */ } - // Why: some shutdown paths do not emit onExit through the provider listener. - // Explicit cleanup is idempotent and covers already-dead PTYs. - finishPtyShutdown(args.id, connectionId, store) - if (!providerExitObserved) { - runtime?.onPtyExit(args.id, -1) - rememberSyntheticKillExit(args.id) - sendPtyExitToRenderer({ id: args.id, code: -1 }) - } - }) + ) ipcMain.handle( 'pty:listSessions', diff --git a/src/main/ipc/settings.test.ts b/src/main/ipc/settings.test.ts index 08b0abbe98d..0f3ec98535c 100644 --- a/src/main/ipc/settings.test.ts +++ b/src/main/ipc/settings.test.ts @@ -115,10 +115,7 @@ describe('registerSettingsHandlers', () => { registerSettingsHandlers(store as never) const handler = handleMock.mock.calls.find( (call) => call[0] === 'settings:update-pr-bot-author-override' - )?.[1] as ( - event: typeof settingsInvokeEvent, - args: { author: string; isBot: boolean } - ) => unknown + )?.[1] as (event: typeof settingsInvokeEvent, args: { author: string; isBot: boolean }) => unknown const result = handler(settingsInvokeEvent, { author: ' Bob ', isBot: true }) diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index 90ccae11ea6..99f35fee47d 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -21,7 +21,13 @@ import { updateHistFileForFallback, logHistoryInjection } from '../terminal-history' -import type { IPtyProvider, PtyProcessInfo, PtySpawnOptions, PtySpawnResult } from './types' +import type { + IPtyProvider, + PtyProcessInfo, + PtyShutdownOptions, + PtySpawnOptions, + PtySpawnResult +} from './types' import { ensureNodePtySpawnHelperExecutable, validateWorkingDirectory, @@ -882,7 +888,7 @@ export class LocalPtyProvider implements IPtyProvider { return { cols: proc.cols, rows: proc.rows } } - async shutdown(id: string, _opts: { immediate?: boolean; keepHistory?: boolean }): Promise { + async shutdown(id: string, _opts: PtyShutdownOptions): Promise { const proc = ptyProcesses.get(id) if (!proc) { return diff --git a/src/main/providers/ssh-pty-provider.test.ts b/src/main/providers/ssh-pty-provider.test.ts index ca0a9fd8261..7fe38858129 100644 --- a/src/main/providers/ssh-pty-provider.test.ts +++ b/src/main/providers/ssh-pty-provider.test.ts @@ -49,6 +49,15 @@ describe('SshPtyProvider', () => { expect(result).toEqual({ id: scopedPty1 }) }) + it('includes relay boot identity so recycled counters cannot collide', async () => { + provider = new SshPtyProvider('conn-1', mux as never, undefined, 'relay-generation-2') + mux.request.mockResolvedValue({ id: 'pty-1' }) + + const result = await provider.spawn({ cols: 80, rows: 24 }) + + expect(result.id).toBe('ssh:conn-1@@relay-generation-2@@pty-1') + }) + it('passes cwd and env through', async () => { mux.request.mockResolvedValue({ id: 'pty-2' }) @@ -434,6 +443,21 @@ describe('SshPtyProvider', () => { }) }) + it('shutdown forwards pane identity to reject recycled relay ids', async () => { + await provider.shutdown(scopedPty1, { + immediate: true, + expectedPaneKey: 'tab-a:leaf-a', + expectedTabId: 'tab-a' + }) + expect(mux.request).toHaveBeenCalledWith('pty.shutdown', { + id: 'pty-1', + immediate: true, + keepHistory: false, + expectedPaneKey: 'tab-a:leaf-a', + expectedTabId: 'tab-a' + }) + }) + it('sendSignal sends pty.sendSignal request', async () => { await provider.sendSignal(scopedPty1, 'SIGINT') expect(mux.request).toHaveBeenCalledWith('pty.sendSignal', { id: 'pty-1', signal: 'SIGINT' }) @@ -485,6 +509,15 @@ describe('SshPtyProvider', () => { ) }) + it('rejects a recycled id from an older relay generation before shutdown', async () => { + provider = new SshPtyProvider('conn-1', mux as never, undefined, 'relay-new') + + await expect( + provider.shutdown('ssh:conn-1@@relay-old@@pty-1', { immediate: true }) + ).rejects.toThrow('relay generation mismatch') + expect(mux.request).not.toHaveBeenCalled() + }) + it('listProcesses returns process list', async () => { const processes = [{ id: 'pty-1', cwd: '/home', title: 'zsh' }] mux.request.mockResolvedValue(processes) diff --git a/src/main/providers/ssh-pty-provider.ts b/src/main/providers/ssh-pty-provider.ts index e386e89dbba..4b64a7a1cbe 100644 --- a/src/main/providers/ssh-pty-provider.ts +++ b/src/main/providers/ssh-pty-provider.ts @@ -1,6 +1,12 @@ import type { SshChannelMultiplexer } from '../ssh/ssh-channel-multiplexer' -import type { IPtyProvider, PtyProcessInfo, PtySpawnOptions, PtySpawnResult } from './types' -import { toAppSshPtyId, toRelaySshPtyId } from './ssh-pty-id' +import type { + IPtyProvider, + PtyProcessInfo, + PtyShutdownOptions, + PtySpawnOptions, + PtySpawnResult +} from './types' +import { parseAppSshPtyId, toAppSshPtyId, toRelaySshPtyId } from './ssh-pty-id' import { seedPowerlevel10kWizardEnv } from '../pty/powerlevel10k-wizard-env' type DataCallback = (payload: { id: string; data: string }) => void @@ -46,7 +52,8 @@ export class SshPtyProvider implements IPtyProvider { constructor( connectionId: string, mux: SshChannelMultiplexer, - private readonly remoteCliBridgeEnv?: RemoteCliBridgeEnv + private readonly remoteCliBridgeEnv?: RemoteCliBridgeEnv, + private readonly relayInstanceId?: string ) { this.connectionId = connectionId this.mux = mux @@ -90,11 +97,21 @@ export class SshPtyProvider implements IPtyProvider { } private toRelayPtyId(id: string): string { + const parsed = parseAppSshPtyId(id) + if ( + parsed?.relayInstanceId && + this.relayInstanceId && + parsed.relayInstanceId !== this.relayInstanceId + ) { + throw new Error( + `${SSH_PTY_IDENTITY_MISMATCH_ERROR}: PTY "${id}" not found (relay generation mismatch)` + ) + } return toRelaySshPtyId(this.connectionId, id) } private toAppPtyId(id: string): string { - return toAppSshPtyId(this.connectionId, id) + return toAppSshPtyId(this.connectionId, id, this.relayInstanceId) } async spawn(opts: PtySpawnOptions): Promise { @@ -234,11 +251,13 @@ export class SshPtyProvider implements IPtyProvider { this.mux.notify('pty.resize', { id: this.toRelayPtyId(id), cols, rows }) } - async shutdown(id: string, opts: { immediate?: boolean; keepHistory?: boolean }): Promise { + async shutdown(id: string, opts: PtyShutdownOptions): Promise { await this.mux.request('pty.shutdown', { id: this.toRelayPtyId(id), immediate: opts.immediate ?? false, - keepHistory: opts.keepHistory ?? false + keepHistory: opts.keepHistory ?? false, + ...(opts.expectedPaneKey ? { expectedPaneKey: opts.expectedPaneKey } : {}), + ...(opts.expectedTabId ? { expectedTabId: opts.expectedTabId } : {}) }) } diff --git a/src/main/providers/types.ts b/src/main/providers/types.ts index d5cf5c60985..c2cb977220f 100644 --- a/src/main/providers/types.ts +++ b/src/main/providers/types.ts @@ -144,6 +144,14 @@ export type PtySpawnResult = { } } +export type PtyShutdownOptions = { + immediate?: boolean + keepHistory?: boolean + /** Relay generations can recycle pty-N; identity prevents stale shutdown. */ + expectedPaneKey?: string + expectedTabId?: string +} + export type PtyProcessInfo = { id: string cwd: string @@ -201,7 +209,7 @@ export type IPtyProvider = { */ getAppliedSize?: (id: string) => Promise<{ cols: number; rows: number } | null> - shutdown(id: string, opts: { immediate?: boolean; keepHistory?: boolean }): Promise + shutdown(id: string, opts: PtyShutdownOptions): Promise sendSignal(id: string, signal: string): Promise getCwd(id: string): Promise getInitialCwd(id: string): Promise diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 3e53354d764..148c5ebeb9b 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -22205,7 +22205,14 @@ describe('OrcaRuntimeService', () => { it('awaits provider shutdown and verifies liveness before terminal.stop returns', async () => { const runtime = new OrcaRuntimeService(store) const stopped: string[] = [] - const processLists = [[{ id: 'pty-1', cwd: '/tmp/worktree-a', title: 'Shell' }], []] + const processLists = [ + [ + { id: 'pty-1', cwd: '/tmp/worktree-a', title: 'Shell' }, + { id: 'pty-unrelated', cwd: '/tmp/unrelated', title: 'Other' } + ], + [{ id: 'pty-unrelated', cwd: '/tmp/unrelated', title: 'Other' }] + ] + const getForegroundProcess = vi.fn().mockResolvedValue(null) runtime.setPtyController({ write: () => true, kill: () => false, @@ -22214,7 +22221,7 @@ describe('OrcaRuntimeService', () => { runtime.onPtyExit(ptyId, -1) return true }, - getForegroundProcess: async () => null, + getForegroundProcess, listProcesses: async () => processLists.shift() ?? [] }) syncSinglePty(runtime, 'pty-1') @@ -22223,6 +22230,40 @@ describe('OrcaRuntimeService', () => { stopped: 1 }) expect(stopped).toEqual(['pty-1']) + expect(getForegroundProcess).not.toHaveBeenCalledWith('pty-unrelated') + }) + + it('blocks paired terminal admission until project stop and removal are atomic', async () => { + const removeProject = vi.fn() + const runtime = new OrcaRuntimeService({ ...store, removeProject } as never) + const stop = deferred() + const spawn = vi.fn().mockResolvedValue({ id: 'pty-late' }) + const processLists = [[{ id: 'pty-1', cwd: '/tmp/worktree-a', title: 'Shell' }], []] + runtime.setPtyController({ + spawn, + write: () => true, + kill: () => false, + stopAndWait: () => stop.promise, + getForegroundProcess: async () => null, + listProcesses: async () => processLists.shift() ?? [] + }) + syncSinglePty(runtime, 'pty-1') + + const removal = runtime.removeProject(`id:${TEST_REPO_ID}`) + await vi.waitFor(() => expect(stop.promise).toBeDefined()) + await vi.waitFor(() => { + expect(processLists).toHaveLength(1) + }) + + await expect(runtime.createTerminal(`id:${TEST_WORKTREE_ID}`)).rejects.toThrow( + 'terminal_admission_blocked_for_project_removal' + ) + expect(spawn).not.toHaveBeenCalled() + + runtime.onPtyExit('pty-1', -1) + stop.resolve(true) + await expect(removal).resolves.toEqual({ removed: true }) + expect(removeProject).toHaveBeenCalledWith(TEST_REPO_ID) }) it('rejects terminal.stop when provider shutdown is not accepted', async () => { diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 968bce2b394..f94d8547fc5 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -2224,6 +2224,9 @@ export class OrcaRuntimeService { // iterates them all. Listeners are cleaned up via subscriptionCleanups. private notificationListeners = new Set<(event: MobileNotificationEvent) => void>() private ptysById = new Map() + // Why: repo removal must make verified stop + metadata removal atomic with + // respect to paired/mobile terminal creation, or a late spawn is orphaned. + private terminalAdmissionBlockedWorktreeIds = new Set() private titleObservationSequence = 0 private headlessTerminals = new Map() private ptyOutputSequenceById = new Map() @@ -12202,11 +12205,31 @@ export class OrcaRuntimeService { throw new Error('runtime_unavailable') } const repo = await this.resolveRepoSelector(repoSelector) - this.store.removeProject(repo.id) - this.invalidateResolvedWorktreeCache() - invalidateAuthorizedRootsCache() - this.notifyReposChanged() - return { removed: true } + const worktreeIds = (await this.listResolvedWorktrees()) + .filter((worktree) => worktree.repoId === repo.id) + .map((worktree) => worktree.id) + if (worktreeIds.some((id) => this.terminalAdmissionBlockedWorktreeIds.has(id))) { + throw new Error('project_removal_in_progress') + } + for (const id of worktreeIds) { + this.terminalAdmissionBlockedWorktreeIds.add(id) + } + try { + // Why: hold admission closed across both the fresh liveness proof and + // repo mutation. A separate terminal.stop RPC leaves an inter-call gap. + for (const id of worktreeIds) { + await this.stopTerminalsForWorktree(`id:${id}`) + } + this.store.removeProject(repo.id) + this.invalidateResolvedWorktreeCache() + invalidateAuthorizedRootsCache() + this.notifyReposChanged() + return { removed: true } + } finally { + for (const id of worktreeIds) { + this.terminalAdmissionBlockedWorktreeIds.delete(id) + } + } } async inspectTerminalProcess( @@ -17482,6 +17505,9 @@ export class OrcaRuntimeService { throw new Error('runtime_unavailable') } const workspace = await this.resolveTerminalWorkspaceLaunchScope(worktreeSelector) + if (this.terminalAdmissionBlockedWorktreeIds.has(workspace.id)) { + throw new Error('terminal_admission_blocked_for_project_removal') + } const launchOpts = await this.resolveAgentTerminalCreateOptions(workspace, opts) const cwd = this.resolveWorkspaceTerminalStartupCwd(workspace, launchOpts.cwd) ?? workspace.path @@ -17571,6 +17597,9 @@ export class OrcaRuntimeService { tabId, agentTeamsPlan?.env ) + if (this.terminalAdmissionBlockedWorktreeIds.has(workspace.id)) { + throw new Error('terminal_admission_blocked_for_project_removal') + } const result = await this.ptyController.spawn({ cols: 120, rows: 40, @@ -17672,6 +17701,9 @@ export class OrcaRuntimeService { const workspace = worktreeSelector ? await this.resolveTerminalWorkspaceLaunchScope(worktreeSelector) : null + if (workspace && this.terminalAdmissionBlockedWorktreeIds.has(workspace.id)) { + throw new Error('terminal_admission_blocked_for_project_removal') + } const launchOpts = workspace ? await this.resolveAgentTerminalCreateOptions(workspace, opts) : opts @@ -17680,6 +17712,9 @@ export class OrcaRuntimeService { ? this.resolveWorkspaceTerminalStartupCwd(workspace, launchOpts.cwd) : launchOpts.cwd const requestId = randomUUID() + if (workspace && this.terminalAdmissionBlockedWorktreeIds.has(workspace.id)) { + throw new Error('terminal_admission_blocked_for_project_removal') + } // Why: terminal creation is a renderer-side Zustand store operation (like // browser tab creation). The main process sends a request, the renderer @@ -18597,9 +18632,15 @@ export class OrcaRuntimeService { } const direction = opts.direction ?? 'horizontal' const workspace = await this.resolveTerminalWorkspaceLaunchScope(`id:${pty.worktreeId}`) + if (this.terminalAdmissionBlockedWorktreeIds.has(workspace.id)) { + throw new Error('terminal_admission_blocked_for_project_removal') + } const leafId = randomUUID() const preAllocatedHandle = this.createPreAllocatedTerminalHandle() const paneKey = makePaneKey(parentTabId, leafId) + if (this.terminalAdmissionBlockedWorktreeIds.has(workspace.id)) { + throw new Error('terminal_admission_blocked_for_project_removal') + } const result = await this.ptyController.spawn({ cols: 120, rows: 40, @@ -18754,8 +18795,10 @@ export class OrcaRuntimeService { this.assertStableReadyGraph(graphEpoch) const resolvedWorktrees = [...(await this.getResolvedWorktreeMap()).values()] this.assertStableReadyGraph(graphEpoch) - const refreshedPtyLiveness = - await this.refreshPtyWorktreeRecordsFromController(resolvedWorktrees) + const refreshedPtyLiveness = await this.refreshPtyWorktreeRecordsFromController( + resolvedWorktrees, + worktree.id + ) if (!refreshedPtyLiveness) { throw new Error('terminal_liveness_unavailable') } @@ -18774,7 +18817,10 @@ export class OrcaRuntimeService { } stoppedPtyIds.push(ptyId) } - const postStopLiveness = await this.refreshPtyWorktreeRecordsFromController(resolvedWorktrees) + const postStopLiveness = await this.refreshPtyWorktreeRecordsFromController( + resolvedWorktrees, + worktree.id + ) if (!postStopLiveness) { throw new Error('terminal_stop_postcheck_unavailable') } diff --git a/src/main/runtime/rpc/methods/client-ui.ts b/src/main/runtime/rpc/methods/client-ui.ts index 401f09bc0b9..241824e43a5 100644 --- a/src/main/runtime/rpc/methods/client-ui.ts +++ b/src/main/runtime/rpc/methods/client-ui.ts @@ -279,9 +279,7 @@ export const CLIENT_UI_METHODS: RpcMethod[] = [ defineMethod({ name: 'settings.updatePRBotAuthorOverride', params: PRBotAuthorOverrideUpdate, - handler: (params, { runtime }) => ({ - settings: runtime.updateClientPRBotAuthorOverride(params) - }) + handler: (params, { runtime }) => ({ settings: runtime.updateClientPRBotAuthorOverride(params) }) }), defineMethod({ name: 'ui.get', diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 05b0068cade..78fb1d7df66 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -671,7 +671,23 @@ export class SshRelaySession { this.wireUpRemoteOrcaCli(mux) - const ptyProvider = new SshPtyProvider(this.targetId, mux, this.remoteCliBridgeEnv ?? undefined) + let relayInstanceId: string | undefined + try { + const status = (await mux.request('relay.status', {})) as { instanceId?: unknown } + relayInstanceId = + typeof status.instanceId === 'string' && status.instanceId.length > 0 + ? status.instanceId + : undefined + } catch { + // Why: older relays lack boot identity. Attach/shutdown identity checks + // remain the compatibility guard until the versioned deploy upgrades it. + } + const ptyProvider = new SshPtyProvider( + this.targetId, + mux, + this.remoteCliBridgeEnv ?? undefined, + relayInstanceId + ) registerSshPtyProvider(this.targetId, ptyProvider) const fsProvider = new SshFilesystemProvider( diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 0f499751e98..d1a065601d1 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -1263,7 +1263,14 @@ export type PreloadApi = { reportGeometry: (id: string, cols: number, rows: number) => void signal: (id: string, signal: string) => void clearBuffer: (id: string) => void - kill: (id: string, opts?: { keepHistory?: boolean }) => Promise + kill: ( + id: string, + opts?: { + keepHistory?: boolean + expectedPaneKey?: string + expectedTabId?: string + } + ) => Promise ackColdRestore: (id: string) => void ackData: (id: string, charCount: number, processedChars?: number) => void onDeliveryResyncRequest: (callback: (payload: { requestId: number }) => void) => () => void diff --git a/src/preload/index.ts b/src/preload/index.ts index ddc85ada4f4..6a1b13c85c9 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -940,8 +940,10 @@ const api = { ipcRenderer.send('pty:terminalViewAttributes', attributes) }, - kill: (id: string, opts?: { keepHistory?: boolean }): Promise => - ipcRenderer.invoke('pty:kill', { id, keepHistory: opts?.keepHistory ?? false }), + kill: ( + id: string, + opts?: { keepHistory?: boolean; expectedPaneKey?: string; expectedTabId?: string } + ): Promise => ipcRenderer.invoke('pty:kill', { id, ...opts }), listSessions: (): Promise<{ id: string; cwd: string; title: string }[]> => ipcRenderer.invoke('pty:listSessions'), diff --git a/src/relay/pty-handler.test.ts b/src/relay/pty-handler.test.ts index 5696bb0f624..2253e7fb678 100644 --- a/src/relay/pty-handler.test.ts +++ b/src/relay/pty-handler.test.ts @@ -712,10 +712,15 @@ describe('PtyHandler', () => { expect(handler.activePtyCount).toBe(0) }) - it('contains repeated stale-spawn native kill throws and still releases ownership', async () => { - const killSpy = vi.fn(() => { - throw new Error('native ConPTY kill failed') - }) + it('contains repeated stale-spawn throws and retains ownership until a proven retry', async () => { + const killSpy = vi + .fn() + .mockImplementationOnce(() => { + throw new Error('native ConPTY kill failed') + }) + .mockImplementationOnce(() => { + throw new Error('native ConPTY kill failed') + }) const stderr = vi.spyOn(process.stderr, 'write').mockImplementation(() => true) const term = { ...mockPtyInstance, kill: killSpy, onData: vi.fn(), onExit: vi.fn() } mockPtySpawn.mockReturnValue(term) @@ -726,10 +731,13 @@ describe('PtyHandler', () => { ).rejects.toThrow('native ConPTY kill failed') expect(() => vi.advanceTimersByTime(5000)).not.toThrow() expect(killSpy).toHaveBeenCalledTimes(2) - expect(handler.activePtyCount).toBe(0) + expect(handler.activePtyCount).toBe(1) expect(stderr).toHaveBeenCalledWith( expect.stringContaining('stale-spawn fallback kill failed') ) + await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) + expect(killSpy).toHaveBeenNthCalledWith(3, 'SIGKILL') + expect(handler.activePtyCount).toBe(0) } finally { stderr.mockRestore() } @@ -1917,6 +1925,39 @@ describe('PtyHandler', () => { expect(handler.activePtyCount).toBe(0) }) + it('rejects shutdown when a recycled id belongs to another pane generation', async () => { + const mockKill = vi.fn() + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + kill: mockKill, + onData: vi.fn(), + onExit: vi.fn() + }) + await dispatcher.callRequest('pty.spawn', { + paneKey: 'tab-new:leaf-new', + tabId: 'tab-new' + }) + + await expect( + dispatcher.callRequest('pty.shutdown', { + id: 'pty-1', + immediate: true, + expectedPaneKey: 'tab-old:leaf-old', + expectedTabId: 'tab-old' + }) + ).rejects.toThrow('identity mismatch') + expect(mockKill).not.toHaveBeenCalled() + expect(handler.activePtyCount).toBe(1) + + await dispatcher.callRequest('pty.shutdown', { + id: 'pty-1', + immediate: true, + expectedPaneKey: 'tab-new:leaf-new', + expectedTabId: 'tab-new' + }) + expect(handler.activePtyCount).toBe(0) + }) + it('dispose kills all PTYs with SIGKILL and invokes exit listeners', async () => { const mockKill = vi.fn() mockPtySpawn.mockReturnValue({ diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index 2d382e6a4e8..62f1b24da20 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -737,15 +737,16 @@ export class PtyHandler { process.stderr.write( `[pty-handler] stale-spawn fallback kill failed: ${error instanceof Error ? error.message : String(error)}\n` ) - } finally { - // Why: stale-spawn cleanup has no client who will ever attach. Even - // a repeated native throw must release ownership without escaping - // the timer into relay.ts uncaughtException process termination. - this.notifyExitListener(still) - disposeManagedPty(still) - this.ptys.delete(id) - this.clearPtyFlowState(id) + // Why: a thrown native kill is not proof of close. Retain the only + // managed handle so relay disposal can retry instead of orphaning + // an untracked PTY/ConPTY process. + still.killTimer = undefined + return } + this.notifyExitListener(still) + disposeManagedPty(still) + this.ptys.delete(id) + this.clearPtyFlowState(id) } }, 5000) // Why: arm fallback ownership before native shutdown. node-pty can throw @@ -863,6 +864,16 @@ export class PtyHandler { if (!managed) { return } + const mismatch = attachIdentityMismatches( + { + paneKey: typeof params.expectedPaneKey === 'string' ? params.expectedPaneKey : undefined, + tabId: typeof params.expectedTabId === 'string' ? params.expectedTabId : undefined + }, + managed.attachIdentity ?? { paneKey: managed.paneKey, tabId: managed.tabId } + ) + if (mismatch) { + throw new Error(`PTY "${id}" not found (identity mismatch)`) + } if (immediate) { this.releaseStartupCommand(managed) diff --git a/src/relay/relay.ts b/src/relay/relay.ts index d47c2019582..ab12eb75fb7 100644 --- a/src/relay/relay.ts +++ b/src/relay/relay.ts @@ -20,6 +20,7 @@ // SSH channel's stdin/stdout to the existing relay's socket. import { createServer, createConnection, type Socket, type Server } from 'node:net' +import { randomUUID } from 'node:crypto' import { homedir } from 'node:os' import { resolve, join } from 'node:path' import { unlinkSync, existsSync, statSync } from 'node:fs' @@ -669,6 +670,9 @@ async function main(): Promise { let socketServer: Server | null = null const launchVersion = readLaunchVersion() const startedAt = Date.now() + // Why: relay-local pty-N counters reset at process start. Thread this stable + // boot identity into app PTY ids so delayed exits cannot hit a recycled id. + const instanceId = randomUUID() let acceptedSocketConnections = 0 let hasAcceptedSocketClient = false let graceDeadlineAt: number | null = null @@ -676,6 +680,7 @@ async function main(): Promise { dispatcher.onRequest('relay.status', async () => ({ pid: process.pid, + instanceId, uptimeMs: Date.now() - startedAt, detached, stdoutAlive, diff --git a/src/relay/subprocess.test.ts b/src/relay/subprocess.test.ts index 4d19542b91c..e7d7c3c74e6 100644 --- a/src/relay/subprocess.test.ts +++ b/src/relay/subprocess.test.ts @@ -421,14 +421,18 @@ describe('Subprocess: Relay entry point', () => { expect(resp.error).toBeUndefined() const status = resp.result as { pid: number + instanceId: string memory: { rss: number } ptys: { active: number } socket: { owned: boolean; listening: boolean; clients: number } } expect(status.pid).toBeGreaterThan(0) + expect(status.instanceId).toMatch(/^[0-9a-f-]{36}$/) expect(status.memory.rss).toBeGreaterThan(0) expect(status.ptys.active).toBe(0) expect(status.socket).toMatchObject({ owned: true, listening: true, clients: 0 }) + const next = await relay.waitForResponse(relay.send('relay.status')) + expect((next.result as { instanceId: string }).instanceId).toBe(status.instanceId) }, 10_000) it('session.registerRoot request returns ok acknowledgment', async () => { diff --git a/src/renderer/src/assets/main.css b/src/renderer/src/assets/main.css index 25a5a4e751b..1d4cbac0572 100644 --- a/src/renderer/src/assets/main.css +++ b/src/renderer/src/assets/main.css @@ -970,7 +970,8 @@ .editor-header { display: flex; align-items: center; - gap: 12px; + /* Keep adjacent actions aligned with the app's compact toolbar spacing. */ + gap: 4px; min-height: 36px; padding: 6px 14px; background: var(--editor-surface); diff --git a/src/renderer/src/components/editor/DiffViewer.tsx b/src/renderer/src/components/editor/DiffViewer.tsx index c119abee471..dbeb8fca2bf 100644 --- a/src/renderer/src/components/editor/DiffViewer.tsx +++ b/src/renderer/src/components/editor/DiffViewer.tsx @@ -24,6 +24,7 @@ import { useDiffViewerLargeDiffLifecycle } from './useDiffViewerLargeDiffLifecyc import { getDiffViewerLargeDiffSaveAction } from './diff-viewer-large-diff-save-action' import type { DiffViewerProps } from './diff-viewer-props' import { buildDiffEditorWordWrapOptions } from './diff-editor-word-wrap-options' +import { useDiffEditorRegistration } from './diff-navigation-context' export default function DiffViewer({ modelKey, @@ -72,6 +73,7 @@ export default function DiffViewer({ (settings?.theme === 'system' && window.matchMedia('(prefers-color-scheme: dark)').matches) const diffEditorRef = useRef(null) + const { registerDiffEditor, unregisterDiffEditor } = useDiffEditorRegistration() const diffBodyRef = useRef(null) const lineNumberOptionsSubRef = useRef<{ dispose: () => void } | null>(null) const [modifiedEditor, setModifiedEditor] = useState(null) @@ -244,10 +246,16 @@ export default function DiffViewer({ // must not keep comment decorators or save handlers talking to disposed UI. lineNumberOptionsSubRef.current?.dispose() lineNumberOptionsSubRef.current = null + // Why: capture before nulling so we unregister the exact instance the + // navigator may still hold (identity guard no-ops a stale dispose). + const fallenBackEditor = diffEditorRef.current diffEditorRef.current = null + if (fallenBackEditor) { + unregisterDiffEditor(fallenBackEditor) + } setModifiedEditor(null) setPopover(null) - }, []) + }, [unregisterDiffEditor]) const handleSubmitComment = async (body: string): Promise => { if (!popover) { @@ -307,6 +315,7 @@ export default function DiffViewer({ const handleMount: DiffOnMount = useCallback( (diffEditor, monaco) => { diffEditorRef.current = diffEditor + registerDiffEditor(diffEditor) lineNumberOptionsSubRef.current?.dispose() lineNumberOptionsSubRef.current = applyDiffEditorLineNumberOptions(diffEditor, sideBySide) @@ -363,11 +372,12 @@ export default function DiffViewer({ lineNumberOptionsSubRef.current?.dispose() lineNumberOptionsSubRef.current = null diffEditorRef.current = null + unregisterDiffEditor(diffEditor) setModifiedEditor(null) setPopover(null) }) }, - [editable, setupCopy, modelKey, filePath, sideBySide] + [editable, setupCopy, modelKey, filePath, sideBySide, registerDiffEditor, unregisterDiffEditor] ) // Why: VS Code snapshots diff view state on deactivation, not on scroll events. diff --git a/src/renderer/src/components/editor/EditorPanel.tsx b/src/renderer/src/components/editor/EditorPanel.tsx index 2181dfc13eb..21b116ea246 100644 --- a/src/renderer/src/components/editor/EditorPanel.tsx +++ b/src/renderer/src/components/editor/EditorPanel.tsx @@ -10,6 +10,7 @@ import { requestEditorFileSave } from './editor-autosave' import { exportActiveMarkdownToPdf } from './export-active-markdown' import type { EditorToggleValue } from './EditorViewToggle' import { EditorPanelShell } from './EditorPanelShell' +import { DiffNavigationProvider } from './diff-navigation-context' import { canUseChangesModeForFile } from './editor-panel-file-mode' import { getEditorPanelRenderModel } from './editor-panel-render-model' import { useClosedEditorTabCleanup } from './useClosedEditorTabCleanup' @@ -351,56 +352,59 @@ function EditorPanelInner({ markdownTableOfContentsVisible[markdownDocumentStateFileId] ?? false return ( - void handleCopyPath()} - onOpenDiffTargetFile={handleOpenDiffTargetFile} - onOpenPreviewToSide={handleOpenPreviewToSide} - onOpenMarkdownPreview={handleOpenMarkdownPreview} - onOpenContainingFolder={handleOpenContainingFolder} - onToggleSideBySide={() => setSideBySide((prev) => !prev)} - onEditorToggleChange={handleEditorToggleChange} - onToggleMarkdownTableOfContents={() => - setMarkdownTableOfContentsVisible( - markdownDocumentStateFileId, - !isMarkdownTableOfContentsVisible - ) - } - onToggleMarkdownFrontmatter={() => - setMarkdownFrontmatterVisible(markdownDocumentStateFileId, !isMarkdownFrontmatterVisible) - } - onExportMarkdownToPdf={() => - void exportActiveMarkdownToPdf({ fileId: activeFile.id, root: panelRef.current }) - } - onContentChange={handleContentChange} - onContentChangeForFile={handleContentChangeForFile} - onDirtyStateHint={handleDirtyStateHint} - onSave={handleSave} - onSaveForFile={handleSaveForFile} - onReloadContent={reloadContent} - onCloseMarkdownTableOfContents={() => - setMarkdownTableOfContentsVisible(markdownDocumentStateFileId, false) - } - onCloseRenameDialog={closeRenameDialog} - onRenameConfirm={handleRenameConfirm} - markdownAnnotationsEnabled={markdownAnnotationsEnabled} - /> + // Why: each split pane needs an isolated bridge between its diff editor and header controls. + + void handleCopyPath()} + onOpenDiffTargetFile={handleOpenDiffTargetFile} + onOpenPreviewToSide={handleOpenPreviewToSide} + onOpenMarkdownPreview={handleOpenMarkdownPreview} + onOpenContainingFolder={handleOpenContainingFolder} + onToggleSideBySide={() => setSideBySide((prev) => !prev)} + onEditorToggleChange={handleEditorToggleChange} + onToggleMarkdownTableOfContents={() => + setMarkdownTableOfContentsVisible( + markdownDocumentStateFileId, + !isMarkdownTableOfContentsVisible + ) + } + onToggleMarkdownFrontmatter={() => + setMarkdownFrontmatterVisible(markdownDocumentStateFileId, !isMarkdownFrontmatterVisible) + } + onExportMarkdownToPdf={() => + void exportActiveMarkdownToPdf({ fileId: activeFile.id, root: panelRef.current }) + } + onContentChange={handleContentChange} + onContentChangeForFile={handleContentChangeForFile} + onDirtyStateHint={handleDirtyStateHint} + onSave={handleSave} + onSaveForFile={handleSaveForFile} + onReloadContent={reloadContent} + onCloseMarkdownTableOfContents={() => + setMarkdownTableOfContentsVisible(markdownDocumentStateFileId, false) + } + onCloseRenameDialog={closeRenameDialog} + onRenameConfirm={handleRenameConfirm} + markdownAnnotationsEnabled={markdownAnnotationsEnabled} + /> + ) } diff --git a/src/renderer/src/components/editor/EditorPanelHeader.tsx b/src/renderer/src/components/editor/EditorPanelHeader.tsx index 6cbb2b6f69b..44816a40ed0 100644 --- a/src/renderer/src/components/editor/EditorPanelHeader.tsx +++ b/src/renderer/src/components/editor/EditorPanelHeader.tsx @@ -1,5 +1,5 @@ import { useMemo } from 'react' -import { Columns2, Eye, FileText, ListTree, Rows2 } from 'lucide-react' +import { ArrowDown, ArrowUp, Columns2, Eye, FileText, ListTree, Rows2 } from 'lucide-react' import { useAppStore } from '@/store' import { selectWorktreeDiffCommentsOrEmpty } from '@/store/worktree-diff-comments-selector' import type { OpenFile } from '@/store/slices/editor' @@ -14,6 +14,7 @@ import { DiffNotesSendMenu } from './DiffNotesSendMenu' import { EditorPanelMarkdownActionsMenu } from './EditorPanelMarkdownActionsMenu' import { translate } from '@/i18n/i18n' import { EditorPanelHeaderPath } from './EditorPanelHeaderPath' +import { useDiffNavigation } from './diff-navigation-context' type EditorPanelHeaderProps = { activeFile: OpenFile @@ -92,6 +93,7 @@ export function EditorPanelHeader({ () => diffComments.filter((comment) => comment.filePath === activeFile.relativePath), [activeFile.relativePath, diffComments] ) + const { changeCount, goToPreviousDiff, goToNextDiff } = useDiffNavigation() return (
@@ -203,6 +205,52 @@ export function EditorPanelHeader({ )} + {isDiffSurface && ( + + + + + + + {translate('auto.components.editor.EditorPanelHeader.2076ecfc9c', 'Previous change')} + + + + )} + {isDiffSurface && ( + + + + + + + {translate('auto.components.editor.EditorPanelHeader.631dab0df3', 'Next change')} + + + + )} {hasEditorToggle && ( { if (!autoHeight) { return null @@ -707,14 +710,15 @@ export default function MonacoEditor({ // Update editor options when settings change useEffect(() => { - if (!editorRef.current || !settings) { + if (!editorRef.current) { return } editorRef.current.updateOptions({ fontSize: editorFontSize, - fontFamily: settings.terminalFontFamily || 'monospace' + fontFamily: editorFontFamily, + ...buildFileEditorWordWrapOptions(editorWordWrap) }) - }, [editorFontSize, settings]) + }, [editorFontFamily, editorFontSize, editorWordWrap]) useEffect(() => { markdownDocLinkDecorationsRef.current?.refresh() @@ -833,9 +837,9 @@ export default function MonacoEditor({ // setting into DiffViewer/DiffSectionItem would have no effect. minimap: { enabled: settings?.editorMinimapEnabled ?? false }, scrollBeyondLastLine: false, - wordWrap: 'on', + ...buildFileEditorWordWrapOptions(editorWordWrap), fontSize: editorFontSize, - fontFamily: settings?.terminalFontFamily || 'monospace', + fontFamily: editorFontFamily, lineNumbers: 'on', renderLineHighlight: 'line', automaticLayout: true, diff --git a/src/renderer/src/components/editor/diff-navigation-context.test.tsx b/src/renderer/src/components/editor/diff-navigation-context.test.tsx new file mode 100644 index 00000000000..581d091b5e1 --- /dev/null +++ b/src/renderer/src/components/editor/diff-navigation-context.test.tsx @@ -0,0 +1,156 @@ +// @vitest-environment happy-dom +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { editor } from 'monaco-editor' +import { + DiffNavigationProvider, + useDiffEditorRegistration, + useDiffNavigation, + type DiffEditorRegistrationContextValue, + type DiffNavigationContextValue +} from './diff-navigation-context' + +type FakeDiffEditor = editor.IStandaloneDiffEditor & { + setLineChanges: (count: number) => void + fireUpdate: () => void + goToDiff: ReturnType + disposeUpdate: ReturnType +} + +function createFakeEditor(initialCount: number): FakeDiffEditor { + let count = initialCount + let updateCallback: (() => void) | null = null + const disposeUpdate = vi.fn(() => { + updateCallback = null + }) + const editor = { + getLineChanges: () => (count > 0 ? Array.from({ length: count }, () => ({})) : []), + goToDiff: vi.fn(), + onDidUpdateDiff: (cb: () => void) => { + updateCallback = cb + return { + dispose: disposeUpdate + } + }, + setLineChanges: (next: number) => { + count = next + }, + fireUpdate: () => updateCallback?.(), + disposeUpdate + } as unknown as FakeDiffEditor + return editor +} + +let captured: DiffNavigationContextValue | null = null +let registration: DiffEditorRegistrationContextValue | null = null +let registrationRenderCount = 0 + +function Probe(): null { + captured = useDiffNavigation() + return null +} + +function RegistrationProbe(): null { + registration = useDiffEditorRegistration() + registrationRenderCount += 1 + return null +} + +describe('DiffNavigationProvider', () => { + let container: HTMLDivElement | null = null + let root: Root | null = null + + function mount(): void { + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + act(() => { + root?.render( + + + + + ) + }) + } + + afterEach(() => { + if (root) { + act(() => root?.unmount()) + } + container?.remove() + container = null + root = null + captured = null + registration = null + registrationRenderCount = 0 + }) + + it('exposes the change count and routes nav actions to the registered editor', () => { + mount() + const editor = createFakeEditor(3) + act(() => registration?.registerDiffEditor(editor)) + + expect(captured?.changeCount).toBe(3) + + act(() => captured?.goToNextDiff()) + expect(editor.goToDiff).toHaveBeenCalledWith('next') + + act(() => captured?.goToPreviousDiff()) + expect(editor.goToDiff).toHaveBeenCalledWith('previous') + }) + + it('re-renders when onDidUpdateDiff flips the count 0 -> N (count is state)', () => { + mount() + const editor = createFakeEditor(0) + act(() => registration?.registerDiffEditor(editor)) + expect(captured?.changeCount).toBe(0) + + act(() => { + editor.setLineChanges(2) + editor.fireUpdate() + }) + expect(captured?.changeCount).toBe(2) + expect(registrationRenderCount).toBe(1) + }) + + it('ignores a stale unregister for an editor that is no longer current (identity guard)', () => { + mount() + const oldEditor = createFakeEditor(1) + const newEditor = createFakeEditor(4) + + // Fast-swap: new editor registers before the old one's dispose fires. + act(() => registration?.registerDiffEditor(oldEditor)) + act(() => registration?.registerDiffEditor(newEditor)) + expect(captured?.changeCount).toBe(4) + expect(oldEditor.disposeUpdate).toHaveBeenCalledOnce() + + // A stale update from the old editor must not flip the count back: registering + // the new editor disposed the old subscription, so its callback no longer fires. + act(() => { + oldEditor.setLineChanges(9) + oldEditor.fireUpdate() + }) + expect(captured?.changeCount).toBe(4) + + act(() => registration?.unregisterDiffEditor(oldEditor)) + + // New editor's count is intact and nav still routes to it. + expect(captured?.changeCount).toBe(4) + act(() => captured?.goToNextDiff()) + expect(newEditor.goToDiff).toHaveBeenCalledWith('next') + expect(oldEditor.goToDiff).not.toHaveBeenCalled() + }) + + it('disposes the active diff update subscription when the provider unmounts', () => { + mount() + const editor = createFakeEditor(1) + act(() => registration?.registerDiffEditor(editor)) + + act(() => root?.unmount()) + + expect(editor.disposeUpdate).toHaveBeenCalledOnce() + root = null + }) +}) diff --git a/src/renderer/src/components/editor/diff-navigation-context.tsx b/src/renderer/src/components/editor/diff-navigation-context.tsx new file mode 100644 index 00000000000..da1f37c9032 --- /dev/null +++ b/src/renderer/src/components/editor/diff-navigation-context.tsx @@ -0,0 +1,115 @@ +import { createContext, useCallback, useContext, useEffect, useMemo, useRef, useState } from 'react' +import type { editor } from 'monaco-editor' + +export type DiffEditorRegistrationContextValue = { + registerDiffEditor: (editor: editor.IStandaloneDiffEditor) => void + unregisterDiffEditor: (editor: editor.IStandaloneDiffEditor) => void +} + +export type DiffNavigationContextValue = { + goToPreviousDiff: () => void + goToNextDiff: () => void + changeCount: number +} + +const noop = (): void => {} + +// Why: registration stays separate from changeCount so diff recomputation only +// rerenders the header controls, not the heavy Monaco DiffViewer consumer. +const DiffEditorRegistrationContext = createContext({ + registerDiffEditor: noop, + unregisterDiffEditor: noop +}) + +const DiffNavigationContext = createContext({ + goToPreviousDiff: noop, + goToNextDiff: noop, + changeCount: 0 +}) + +function countChanges(diffEditor: editor.IStandaloneDiffEditor): number { + return diffEditor.getLineChanges()?.length ?? 0 +} + +export function DiffNavigationProvider({ + children +}: { + children: React.ReactNode +}): React.JSX.Element { + const editorRef = useRef(null) + const updateSubRef = useRef<{ dispose: () => void } | null>(null) + // Why: changeCount must be state, not a ref — the header is a sibling consumer + // and only re-renders (enabling the buttons) when the value object identity + // changes on the 0 -> N flip once the diff computation lands. + const [changeCount, setChangeCount] = useState(0) + + const registerDiffEditor = useCallback((diffEditor: editor.IStandaloneDiffEditor) => { + editorRef.current = diffEditor + // Hold at most one update subscription; replace any prior editor's. + updateSubRef.current?.dispose() + updateSubRef.current = diffEditor.onDidUpdateDiff(() => { + // Why: ignore updates from an editor that is no longer current so a stale + // subscription in the fast-swap case can't write a wrong count. + if (editorRef.current === diffEditor) { + setChangeCount(countChanges(diffEditor)) + } + }) + setChangeCount(countChanges(diffEditor)) + }, []) + + const unregisterDiffEditor = useCallback((diffEditor: editor.IStandaloneDiffEditor) => { + // Why: identity guard for the fast-swap race — a stale dispose carrying the + // old editor must not wipe a freshly-registered new one. + if (editorRef.current !== diffEditor) { + return + } + updateSubRef.current?.dispose() + updateSubRef.current = null + editorRef.current = null + setChangeCount(0) + }, []) + + const goToPreviousDiff = useCallback(() => { + editorRef.current?.goToDiff('previous') + }, []) + + const goToNextDiff = useCallback(() => { + editorRef.current?.goToDiff('next') + }, []) + + useEffect(() => { + return () => { + updateSubRef.current?.dispose() + updateSubRef.current = null + } + }, []) + + const registrationValue = useMemo( + () => ({ registerDiffEditor, unregisterDiffEditor }), + [registerDiffEditor, unregisterDiffEditor] + ) + const navigationValue = useMemo( + () => ({ + goToPreviousDiff, + goToNextDiff, + changeCount + }), + [goToPreviousDiff, goToNextDiff, changeCount] + ) + + return ( + + + {children} + + + ) +} + +export function useDiffEditorRegistration(): DiffEditorRegistrationContextValue { + return useContext(DiffEditorRegistrationContext) +} + +export function useDiffNavigation(): DiffNavigationContextValue { + return useContext(DiffNavigationContext) +} diff --git a/src/renderer/src/components/editor/file-editor-word-wrap-options.test.ts b/src/renderer/src/components/editor/file-editor-word-wrap-options.test.ts new file mode 100644 index 00000000000..c50d4389ab3 --- /dev/null +++ b/src/renderer/src/components/editor/file-editor-word-wrap-options.test.ts @@ -0,0 +1,13 @@ +import { describe, expect, it } from 'vitest' +import { buildFileEditorWordWrapOptions } from './file-editor-word-wrap-options' + +describe('buildFileEditorWordWrapOptions', () => { + it('keeps wrapping enabled for existing profiles without the preference', () => { + expect(buildFileEditorWordWrapOptions(undefined)).toEqual({ wordWrap: 'on' }) + expect(buildFileEditorWordWrapOptions(true)).toEqual({ wordWrap: 'on' }) + }) + + it('disables wrapping for horizontal file-editor scrolling', () => { + expect(buildFileEditorWordWrapOptions(false)).toEqual({ wordWrap: 'off' }) + }) +}) diff --git a/src/renderer/src/components/editor/file-editor-word-wrap-options.ts b/src/renderer/src/components/editor/file-editor-word-wrap-options.ts new file mode 100644 index 00000000000..af9bedfb2ef --- /dev/null +++ b/src/renderer/src/components/editor/file-editor-word-wrap-options.ts @@ -0,0 +1,8 @@ +import type { editor } from 'monaco-editor' + +export function buildFileEditorWordWrapOptions( + editorWordWrap: boolean | undefined +): Pick { + // Why: profiles saved before this preference existed must retain Orca's previous wrapped default. + return { wordWrap: editorWordWrap === false ? 'off' : 'on' } +} diff --git a/src/renderer/src/components/settings/EditorWordWrapSetting.test.tsx b/src/renderer/src/components/settings/EditorWordWrapSetting.test.tsx new file mode 100644 index 00000000000..63a963c215e --- /dev/null +++ b/src/renderer/src/components/settings/EditorWordWrapSetting.test.tsx @@ -0,0 +1,85 @@ +// @vitest-environment happy-dom + +import { join } from 'node:path' +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { getDefaultSettings } from '../../../../shared/constants' + +vi.mock('../../store', () => ({ + useAppStore: (selector: (state: { settingsSearchQuery: string }) => unknown) => + selector({ settingsSearchQuery: '' }) +})) + +import { EditorWordWrapSetting } from './EditorWordWrapSetting' + +let root: Root | null = null +let container: HTMLDivElement | null = null + +afterEach(() => { + if (root) { + act(() => root?.unmount()) + } + container?.remove() + root = null + container = null +}) + +function renderSetting(editorWordWrap: boolean | undefined, updateSettings = vi.fn()) { + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + act(() => { + root?.render( + + ) + }) + return { container, updateSettings } +} + +describe('EditorWordWrapSetting', () => { + it('shows wrapping as on for profiles saved before the preference existed', () => { + const { container } = renderSetting(undefined) + const on = [...container.querySelectorAll('[role="radio"]')].find( + (button) => button.textContent === 'On' + ) + + expect(on?.getAttribute('aria-checked')).toBe('true') + }) + + it('shows wrapping as off when the preference is disabled', () => { + const { container } = renderSetting(false) + const off = [...container.querySelectorAll('[role="radio"]')].find( + (button) => button.textContent === 'Off' + ) + + expect(off?.getAttribute('aria-checked')).toBe('true') + }) + + it('persists the off choice for horizontal scrolling', () => { + const updateSettings = vi.fn() + const { container } = renderSetting(true, updateSettings) + const off = [...container.querySelectorAll('[role="radio"]')].find( + (button) => button.textContent === 'Off' + ) + + act(() => off?.click()) + + expect(updateSettings).toHaveBeenCalledWith({ editorWordWrap: false }) + }) + + it('persists the on choice without changing the diff preference', () => { + const updateSettings = vi.fn() + const { container } = renderSetting(false, updateSettings) + const on = [...container.querySelectorAll('[role="radio"]')].find( + (button) => button.textContent === 'On' + ) + + act(() => on?.click()) + + expect(updateSettings).toHaveBeenCalledWith({ editorWordWrap: true }) + }) +}) diff --git a/src/renderer/src/components/settings/EditorWordWrapSetting.tsx b/src/renderer/src/components/settings/EditorWordWrapSetting.tsx new file mode 100644 index 00000000000..b3dfbb853c4 --- /dev/null +++ b/src/renderer/src/components/settings/EditorWordWrapSetting.tsx @@ -0,0 +1,69 @@ +import type { GlobalSettings } from '../../../../shared/types' +import { translate } from '@/i18n/i18n' +import { SearchableSetting } from './SearchableSetting' +import { Label } from '../ui/label' +import { SettingsSegmentedControl } from './SettingsFormControls' + +type EditorWordWrapSettingProps = { + settings: GlobalSettings + updateSettings: (updates: Partial) => void +} + +export function EditorWordWrapSetting({ + settings, + updateSettings +}: EditorWordWrapSettingProps): React.JSX.Element { + return ( + +
+ +

+ {translate( + 'auto.components.settings.GeneralEditorSettingsSection.9b18de6eea', + 'Wrap long lines in file editors instead of requiring horizontal scrolling.' + )} +

+
+ updateSettings({ editorWordWrap: option === 'on' })} + options={[ + { + value: 'off', + label: translate( + 'auto.components.settings.GeneralEditorSettingsSection.bf16ef0af2', + 'Off' + ) + }, + { + value: 'on', + label: translate( + 'auto.components.settings.GeneralEditorSettingsSection.3f6892f307', + 'On' + ) + } + ]} + /> + + ) +} diff --git a/src/renderer/src/components/settings/GeneralEditorSettingsSection.tsx b/src/renderer/src/components/settings/GeneralEditorSettingsSection.tsx index a4a33f1102a..9cccdf166ee 100644 --- a/src/renderer/src/components/settings/GeneralEditorSettingsSection.tsx +++ b/src/renderer/src/components/settings/GeneralEditorSettingsSection.tsx @@ -17,6 +17,7 @@ import { } from './SettingsFormControls' import { translate } from '@/i18n/i18n' import { RichMarkdownSpellcheckSetting } from './RichMarkdownSpellcheckSetting' +import { EditorWordWrapSetting } from './EditorWordWrapSetting' export type AutoSaveDelayDraftState = { sourceDelayMs: number @@ -248,6 +249,8 @@ export function GeneralEditorSettingsSection({ /> + + { expect(matchesSettingsSearch('wsl', entries)).toBe(true) }) + it('includes file-editor word-wrap and horizontal-scroll keywords', () => { + const entries = getGeneralPaneSearchEntries() + + expect(matchesSettingsSearch('editor word wrap', entries)).toBe(true) + expect(matchesSettingsSearch('horizontal scroll', entries)).toBe(true) + }) + it('includes rich Markdown spellcheck keywords', () => { const entries = getGeneralPaneSearchEntries() diff --git a/src/renderer/src/components/settings/general-editor-search.ts b/src/renderer/src/components/settings/general-editor-search.ts index 8f93eea9544..c7ba4653d32 100644 --- a/src/renderer/src/components/settings/general-editor-search.ts +++ b/src/renderer/src/components/settings/general-editor-search.ts @@ -29,6 +29,17 @@ export const getGeneralEditorSearchEntries = createLocalizedCatalog(() => [ ) ] }, + { + title: translate('auto.components.settings.general.search.e61157e926', 'Editor Word Wrap'), + description: translate( + 'auto.components.settings.general.search.005be5c699', + 'Wrap long lines in file editors instead of requiring horizontal scrolling.' + ), + keywords: [ + ...translateSearchKeyword('auto.components.settings.general.search.e1ee631696', 'editor'), + ...translateSearchKeyword('auto.components.settings.general.search.3ca5ab78a5', 'code') + ] + }, { title: translate('auto.components.settings.general.search.2760c9933f', 'Default Diff View'), description: translate( diff --git a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts index 7d95304ba90..18bd6de7603 100644 --- a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts @@ -1,8 +1,10 @@ -import { afterEach, describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import { bufferPreHandlerPtyData, + bufferPreHandlerPtyExit, clearPreHandlerPtyState, - drainPreHandlerPtyData + drainPreHandlerPtyData, + drainPreHandlerPtyExit } from './pty-pre-handler-buffer' const RESCAN_PTY_ID = 'pty-pre-handler-rescan' @@ -12,6 +14,9 @@ describe('pre-handler PTY buffer', () => { afterEach(() => { clearPreHandlerPtyState(RESCAN_PTY_ID) clearPreHandlerPtyState(TRIM_PTY_ID) + for (let index = 0; index <= 64; index += 1) { + clearPreHandlerPtyState(`pty-exit-${index}`) + } }) it('does not rescan historical chunks while buffering small startup output', () => { @@ -71,4 +76,18 @@ describe('pre-handler PTY buffer', () => { expect(drained).toHaveLength(512) expect(drained.join('')).toHaveLength(512 * 1_024) }) + + it('bounds exits that arrive after their owner unregistered', () => { + for (let index = 0; index <= 64; index += 1) { + bufferPreHandlerPtyExit(`pty-exit-${index}`, index) + } + const oldest = vi.fn() + const newest = vi.fn() + + drainPreHandlerPtyExit('pty-exit-0', oldest) + drainPreHandlerPtyExit('pty-exit-64', newest) + + expect(oldest).not.toHaveBeenCalled() + expect(newest).toHaveBeenCalledWith(64) + }) }) diff --git a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts index c3d12fed9fb..a58d7a76b27 100644 --- a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts +++ b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts @@ -21,6 +21,7 @@ const preHandlerPtyExit = new Map() // and dropping the first setup-script bytes. const PRE_HANDLER_PTY_DATA_MAX_BYTES = 512 * 1024 const PRE_HANDLER_PTY_DATA_MAX_PTYS = 64 +const PRE_HANDLER_PTY_EXIT_MAX_PTYS = 64 // Why: legit pre-attach windows drain within milliseconds and hold little // data. Sustained accumulation means a pane lost its data handler (the // frozen-pane detach/attach race) — leave a breadcrumb for trace capture. @@ -36,6 +37,7 @@ export function bufferPreHandlerPtyData(ptyId: string, data: string, meta?: PtyD const oldestPtyId = preHandlerPtyData.keys().next().value if (typeof oldestPtyId === 'string') { preHandlerPtyData.delete(oldestPtyId) + warnedLostHandlerPtyIds.delete(oldestPtyId) } } const bufferedMeta = @@ -90,6 +92,12 @@ export function drainPreHandlerPtyData( } export function bufferPreHandlerPtyExit(ptyId: string, code: number): void { + if (!preHandlerPtyExit.has(ptyId) && preHandlerPtyExit.size >= PRE_HANDLER_PTY_EXIT_MAX_PTYS) { + const oldestPtyId = preHandlerPtyExit.keys().next().value + if (typeof oldestPtyId === 'string') { + preHandlerPtyExit.delete(oldestPtyId) + } + } preHandlerPtyExit.set(ptyId, code) } diff --git a/src/renderer/src/components/terminal-pane/pty-transport.test.ts b/src/renderer/src/components/terminal-pane/pty-transport.test.ts index e9d1cdecfc2..830acee090c 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.test.ts @@ -1383,7 +1383,11 @@ describe('createIpcPtyTransport', () => { } as unknown as typeof window try { - const transport = createIpcPtyTransport({ onPtySpawn }) + const transport = createIpcPtyTransport({ + onPtySpawn, + tabId: 'tab-late', + leafId: '11111111-1111-4111-8111-111111111111' + }) const connectPromise = transport.connect({ url: '', callbacks: {} @@ -1397,7 +1401,10 @@ describe('createIpcPtyTransport', () => { await connectPromise await flushPtySideEffects() - expect(killMock).toHaveBeenCalledWith('pty-late') + expect(killMock).toHaveBeenCalledWith('pty-late', { + expectedPaneKey: 'tab-late:11111111-1111-4111-8111-111111111111', + expectedTabId: 'tab-late' + }) expect(warn).toHaveBeenCalledWith( '[pty] Failed to stop PTY spawned after transport teardown', killError @@ -1409,7 +1416,10 @@ describe('createIpcPtyTransport', () => { spawnMock.mockResolvedValueOnce({ id: 'pty-next' }) await createIpcPtyTransport().connect({ url: '', callbacks: {} }) await flushPtySideEffects() - expect(killMock).toHaveBeenNthCalledWith(2, 'pty-late') + expect(killMock).toHaveBeenNthCalledWith(2, 'pty-late', { + expectedPaneKey: 'tab-late:11111111-1111-4111-8111-111111111111', + expectedTabId: 'tab-late' + }) } finally { warn.mockRestore() } diff --git a/src/renderer/src/components/terminal-pane/pty-transport.ts b/src/renderer/src/components/terminal-pane/pty-transport.ts index 7090f2e9334..b2809169539 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.ts @@ -38,6 +38,7 @@ import { import { extractIpcErrorMessage } from '@/lib/ipc-error' import { isTuiAgent } from '../../../../shared/tui-agent-config' import { killPtyRetainingRetryOwnership } from '@/lib/pty-kill-retry-ownership' +import { makePaneKey } from '../../../../shared/stable-pane-id' // Re-export public API so existing consumers keep working. export { @@ -542,6 +543,12 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra onAgentStatus }) let storedCallbacks: Parameters[0]['callbacks'] = {} + const shutdownIdentity = tabId + ? { + ...(leafId ? { expectedPaneKey: makePaneKey(tabId, leafId) } : {}), + expectedTabId: tabId + } + : undefined // Why: pane->tab detach / split-group moves rehome the React subtree, so a // NEW TerminalPane can attach to the same ptyId before the OLD instance's @@ -735,7 +742,8 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra if (destroyed) { void killPtyRetainingRetryOwnership( spawnResult.id, - '[pty] Failed to stop PTY spawned after transport teardown' + '[pty] Failed to stop PTY spawned after transport teardown', + shutdownIdentity ).catch(() => {}) return } @@ -922,9 +930,11 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra inputWriteQueue.clear() if (ptyId) { const id = ptyId - void killPtyRetainingRetryOwnership(id, '[pty] Failed to stop disconnected PTY').catch( - () => {} - ) + void killPtyRetainingRetryOwnership( + id, + '[pty] Failed to stop disconnected PTY', + shutdownIdentity + ).catch(() => {}) connected = false ptyId = null unregisterPtyHandlers(id) diff --git a/src/renderer/src/components/terminal-pane/terminal-shortcut-policy.test.ts b/src/renderer/src/components/terminal-pane/terminal-shortcut-policy.test.ts index a9929951449..927f57effe1 100644 --- a/src/renderer/src/components/terminal-pane/terminal-shortcut-policy.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-shortcut-policy.test.ts @@ -66,12 +66,12 @@ describe('resolveTerminalShortcutAction', () => { ).toEqual({ type: 'focusPane', direction: 'next' }) }) - it('keeps shift-enter and delete helpers explicit', () => { + it('keeps inactive shift-enter and delete helpers explicit', () => { expect( resolveTerminalShortcutAction(event({ key: 'Enter', code: 'Enter', shiftKey: true }), true) ).toEqual({ type: 'sendInput', - data: '\x1b[13;2u' + data: '\x1b\r' }) expect(resolveTerminalShortcutAction(event({ key: 'Backspace', ctrlKey: true }), true)).toEqual( { type: 'sendInput', data: '\x17' } @@ -164,9 +164,9 @@ describe('resolveTerminalShortcutAction', () => { expect(getWindowsShiftEnterEncoding).not.toHaveBeenCalled() }) - it('always uses CSI-u Shift+Enter off Windows regardless of Windows encoding', () => { + it('uses CSI-u Shift+Enter off Windows only while Kitty keyboard is active', () => { for (const encoding of [() => 'csi-u' as const, () => 'alt-enter' as const, undefined]) { - expect( + const resolve = (kittyActive: boolean) => resolveTerminalShortcutAction( event({ key: 'Enter', code: 'Enter', shiftKey: true }), false, @@ -175,11 +175,12 @@ describe('resolveTerminalShortcutAction', () => { false, undefined, undefined, - undefined, + () => kittyActive, undefined, encoding ) - ).toEqual({ type: 'sendInput', data: '\x1b[13;2u' }) + expect(resolve(true)).toEqual({ type: 'sendInput', data: '\x1b[13;2u' }) + expect(resolve(false)).toEqual({ type: 'sendInput', data: '\x1b\r' }) } }) diff --git a/src/renderer/src/components/terminal-pane/terminal-shortcut-policy.ts b/src/renderer/src/components/terminal-pane/terminal-shortcut-policy.ts index f0f284d167f..15a1ec07db8 100644 --- a/src/renderer/src/components/terminal-pane/terminal-shortcut-policy.ts +++ b/src/renderer/src/components/terminal-pane/terminal-shortcut-policy.ts @@ -158,7 +158,11 @@ export function resolveTerminalShortcutAction( : isWindows ? 'alt-enter' : 'csi-u' - return { type: 'sendInput', data: encoding === 'csi-u' ? '\x1b[13;2u' : '\x1b\r' } + // Why: CSI-u is application input, not a universal terminal sequence. Off + // Windows, only send it while the pane's application has KKP active. + const canSendCsiU = + encoding === 'csi-u' && (useLocalWindowsCapability || isKittyKeyboardActivePane?.() === true) + return { type: 'sendInput', data: canSendCsiU ? '\x1b[13;2u' : '\x1b\r' } } if ( diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 007096cb832..6a98f7b81cd 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -5401,7 +5401,9 @@ "bf16ef0af2": "Off", "3f6892f307": "On", "b82f86d7d2": "Rich Markdown Spellcheck", - "5195f0b9ef": "Show browser spelling underlines and suggestions while editing rich Markdown." + "5195f0b9ef": "Show browser spelling underlines and suggestions while editing rich Markdown.", + "7ddd66fede": "Editor Word Wrap", + "9b18de6eea": "Wrap long lines in file editors instead of requiring horizontal scrolling." }, "AdvancedNetworkSettingsSection": { "3e431564b5": "localhost, 127.0.0.1, *.internal", @@ -7716,7 +7718,9 @@ "defaultProjectRuntime": "Default Project Runtime", "defaultProjectRuntimeDescription": "Choose the runtime inherited by local Windows projects.", "d2d2d929c0": "Rich Markdown Spellcheck", - "4497e2e2bb": "Show browser spelling underlines and suggestions while editing rich Markdown." + "4497e2e2bb": "Show browser spelling underlines and suggestions while editing rich Markdown.", + "e61157e926": "Editor Word Wrap", + "005be5c699": "Wrap long lines in file editors instead of requiring horizontal scrolling." } }, "git": { @@ -11436,7 +11440,9 @@ "c98ce191da": "This diff has no modified-side file to open", "9b80bbe1de": "Open file tab", "f0fd4174b5": "Open file tab to use rich markdown editing", - "a10d9b8337": "Open file" + "a10d9b8337": "Open file", + "2076ecfc9c": "Previous change", + "631dab0df3": "Next change" }, "EditorPanelMarkdownActionsMenu": { "3e0ce48c24": "Export as PDF", diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 2053adb00dc..5ea1e331e18 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -5401,7 +5401,9 @@ "bf16ef0af2": "Desactivado", "3f6892f307": "Activado", "b82f86d7d2": "Corrector ortográfico de Rich Markdown", - "5195f0b9ef": "Muestra subrayados y sugerencias ortográficas del navegador al editar Rich Markdown." + "5195f0b9ef": "Muestra subrayados y sugerencias ortográficas del navegador al editar Rich Markdown.", + "7ddd66fede": "Ajuste de línea en el editor", + "9b18de6eea": "Ajusta las líneas largas en editores de archivos en lugar de requerir desplazamiento horizontal." }, "AdvancedNetworkSettingsSection": { "3e431564b5": "localhost, 127.0.0.1, *.internal", @@ -7679,7 +7681,9 @@ "defaultProjectRuntime": "Runtime de proyecto predeterminado", "defaultProjectRuntimeDescription": "Elige el runtime que heredarán los proyectos locales de Windows.", "d2d2d929c0": "Corrector ortográfico de Rich Markdown", - "4497e2e2bb": "Muestra subrayados y sugerencias ortográficas del navegador al editar Rich Markdown." + "4497e2e2bb": "Muestra subrayados y sugerencias ortográficas del navegador al editar Rich Markdown.", + "e61157e926": "Ajuste de línea en el editor", + "005be5c699": "Ajusta las líneas largas en editores de archivos en lugar de requerir desplazamiento horizontal." } }, "git": { @@ -11436,7 +11440,9 @@ "c98ce191da": "Este diff no tiene ningún archivo del lado modificado para abrir", "9b80bbe1de": "Abrir pestaña de archivo", "f0fd4174b5": "Abre la pestaña de archivo para usar la edición enriquecida de Markdown", - "a10d9b8337": "Abrir archivo" + "a10d9b8337": "Abrir archivo", + "2076ecfc9c": "Previous change", + "631dab0df3": "Next change" }, "EditorPanelMarkdownActionsMenu": { "3e0ce48c24": "Exportar como PDF", diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 1c811412623..ea6894df23e 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -5386,7 +5386,9 @@ "bf16ef0af2": "オフ", "3f6892f307": "オン", "b82f86d7d2": "Rich Markdown Spellcheck", - "5195f0b9ef": "Show browser spelling underlines and suggestions while editing rich Markdown." + "5195f0b9ef": "Show browser spelling underlines and suggestions while editing rich Markdown.", + "7ddd66fede": "エディターのワードラップ", + "9b18de6eea": "水平スクロールを使わずに、ファイルエディターで長い行を折り返します。" }, "AdvancedNetworkSettingsSection": { "3e431564b5": "ローカルホスト、127.0.0.1、*.internal", @@ -7701,7 +7703,9 @@ "defaultProjectRuntime": "Default Project Runtime", "defaultProjectRuntimeDescription": "Choose the runtime inherited by local Windows projects.", "d2d2d929c0": "Rich Markdown Spellcheck", - "4497e2e2bb": "Show browser spelling underlines and suggestions while editing rich Markdown." + "4497e2e2bb": "Show browser spelling underlines and suggestions while editing rich Markdown.", + "e61157e926": "エディターのワードラップ", + "005be5c699": "水平スクロールを使わずに、ファイルエディターで長い行を折り返します。" } }, "git": { @@ -11436,7 +11440,9 @@ "c98ce191da": "この差分には開くための変更側ファイルがありません", "9b80bbe1de": "ファイルタブを開く", "f0fd4174b5": "ファイル タブを開いてリッチ markdown 編集を使用する", - "a10d9b8337": "ファイルを開く" + "a10d9b8337": "ファイルを開く", + "2076ecfc9c": "Previous change", + "631dab0df3": "Next change" }, "EditorPanelMarkdownActionsMenu": { "3e0ce48c24": "PDFとしてエクスポート", diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 1851bc4059f..1a5da97b828 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -5386,7 +5386,9 @@ "bf16ef0af2": "끄다", "3f6892f307": "~에", "b82f86d7d2": "Rich Markdown Spellcheck", - "5195f0b9ef": "Show browser spelling underlines and suggestions while editing rich Markdown." + "5195f0b9ef": "Show browser spelling underlines and suggestions while editing rich Markdown.", + "7ddd66fede": "편집기 자동 줄 바꿈", + "9b18de6eea": "가로 스크롤 대신 파일 편집기에서 긴 줄을 자동으로 줄 바꿈합니다." }, "AdvancedNetworkSettingsSection": { "3e431564b5": "로컬호스트, 127.0.0.1, *.internal", @@ -7664,7 +7666,9 @@ "defaultProjectRuntime": "기본 프로젝트 런타임", "defaultProjectRuntimeDescription": "로컬 Windows 프로젝트가 상속할 런타임을 선택합니다.", "d2d2d929c0": "Rich Markdown Spellcheck", - "4497e2e2bb": "Show browser spelling underlines and suggestions while editing rich Markdown." + "4497e2e2bb": "Show browser spelling underlines and suggestions while editing rich Markdown.", + "e61157e926": "편집기 자동 줄 바꿈", + "005be5c699": "가로 스크롤 대신 파일 편집기에서 긴 줄을 자동으로 줄 바꿈합니다." } }, "git": { @@ -11436,7 +11440,9 @@ "c98ce191da": "이 차이점에는 열 수 있는 수정된 측면 파일이 없습니다.", "9b80bbe1de": "파일 탭 열기", "f0fd4174b5": "풍부한 markdown 편집을 사용하려면 파일 탭을 엽니다.", - "a10d9b8337": "파일 열기" + "a10d9b8337": "파일 열기", + "2076ecfc9c": "Previous change", + "631dab0df3": "Next change" }, "EditorPanelMarkdownActionsMenu": { "3e0ce48c24": "PDF로 내보내기", diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 60632816ac6..9fc7fc0cee3 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -5386,7 +5386,9 @@ "bf16ef0af2": "关闭", "3f6892f307": "开启", "b82f86d7d2": "Rich Markdown Spellcheck", - "5195f0b9ef": "在编辑富 Markdown 时显示浏览器拼写下划线和建议。" + "5195f0b9ef": "在编辑富 Markdown 时显示浏览器拼写下划线和建议。", + "7ddd66fede": "编辑器自动换行", + "9b18de6eea": "在文件编辑器中自动换行长行，而无需水平滚动。" }, "AdvancedNetworkSettingsSection": { "3e431564b5": "本地主机，127.0.0.1，*.internal", @@ -7664,7 +7666,9 @@ "defaultProjectRuntime": "默认项目运行时", "defaultProjectRuntimeDescription": "选择本地 Windows 项目继承的运行时。", "d2d2d929c0": "Rich Markdown Spellcheck", - "4497e2e2bb": "在编辑富 Markdown 时显示浏览器拼写下划线和建议。" + "4497e2e2bb": "在编辑富 Markdown 时显示浏览器拼写下划线和建议。", + "e61157e926": "编辑器自动换行", + "005be5c699": "在文件编辑器中自动换行长行，而无需水平滚动。" } }, "git": { @@ -11436,7 +11440,9 @@ "c98ce191da": "此差异没有可打开的修改端文件", "9b80bbe1de": "打开文件选项卡", "f0fd4174b5": "打开文件选项卡以使用丰富的 Markdown 编辑", - "a10d9b8337": "打开文件" + "a10d9b8337": "打开文件", + "2076ecfc9c": "Previous change", + "631dab0df3": "Next change" }, "EditorPanelMarkdownActionsMenu": { "3e0ce48c24": "导出为 PDF", diff --git a/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts b/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts index 933411bf7b3..98d83972ce9 100644 --- a/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts +++ b/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts @@ -76,4 +76,15 @@ describe('cleanupEphemeralVmRuntimesForDeleted', () => { cleanupEphemeralVmRuntimesForDeleted({ workspaceIds: ['wt-1'], throwOnError: true }) ).rejects.toBe(error) }) + + it('does not report a target destroyed when cleanup rejects', async () => { + listRuntimes.mockResolvedValue([ + runtime({ id: 'rt-1', workspaceId: 'wt-1', sshTargetId: 'runtime-ssh-a' }) + ]) + cleanup.mockRejectedValueOnce(new Error('cleanup failed')) + + await expect(cleanupEphemeralVmRuntimesForDeleted({ workspaceIds: ['wt-1'] })).resolves.toEqual( + [] + ) + }) }) diff --git a/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.ts b/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.ts index 6e57febf036..7cd203965a8 100644 --- a/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.ts +++ b/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.ts @@ -35,10 +35,10 @@ export async function cleanupEphemeralVmRuntimesForDeleted(args: { (runtime.sshTargetId !== undefined && sshTargetIdSet.has(runtime.sshTargetId))) ) for (const runtime of matchingRuntimes) { + await window.api.ephemeralVm.cleanup({ runtimeId: runtime.id }) if (runtime.sshTargetId) { destroyedSshTargetIds.push(runtime.sshTargetId) } - await window.api.ephemeralVm.cleanup({ runtimeId: runtime.id }) } } catch (error) { console.error('Failed to clean up ephemeral VM runtime for deleted workspace:', error) diff --git a/src/renderer/src/lib/pr-bot-author-overrides.test.ts b/src/renderer/src/lib/pr-bot-author-overrides.test.ts index 3516cd73341..79706f162a0 100644 --- a/src/renderer/src/lib/pr-bot-author-overrides.test.ts +++ b/src/renderer/src/lib/pr-bot-author-overrides.test.ts @@ -56,7 +56,9 @@ describe('PR bot author override updates', () => { store.pending.shift()?.() await vi.waitFor(() => expect(store.apiUpdate).toHaveBeenCalledTimes(2)) store.pending.shift()?.() - await vi.waitFor(() => expect(store.settings.prBotAuthorOverrides).toEqual(['alice', 'bob'])) + await vi.waitFor(() => + expect(store.settings.prBotAuthorOverrides).toEqual(['alice', 'bob']) + ) }) it('continues processing updates after an atomic settings write fails', async () => { diff --git a/src/renderer/src/lib/pty-kill-retry-ownership.test.ts b/src/renderer/src/lib/pty-kill-retry-ownership.test.ts new file mode 100644 index 00000000000..3b968b70565 --- /dev/null +++ b/src/renderer/src/lib/pty-kill-retry-ownership.test.ts @@ -0,0 +1,37 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + killPtyRetainingRetryOwnership, + releaseRetainedPtyKillOwnership, + retryRetainedPtyKills +} from './pty-kill-retry-ownership' + +const IDS = Array.from({ length: 65 }, (_, index) => `pty-retained-${index}`) + +describe('PTY kill retry ownership', () => { + afterEach(() => { + for (const id of IDS) { + releaseRetainedPtyKillOwnership(id) + } + vi.unstubAllGlobals() + vi.restoreAllMocks() + }) + + it('bounds retained ids and retries at most one per lifecycle event', async () => { + const kill = vi.fn().mockRejectedValue(new Error('provider disconnected')) + vi.stubGlobal('window', { api: { pty: { kill } } }) + vi.spyOn(console, 'warn').mockImplementation(() => {}) + + for (const id of IDS) { + await killPtyRetainingRetryOwnership(id, '[pty] failed').catch(() => {}) + } + kill.mockResolvedValue(undefined) + + retryRetainedPtyKills() + await Promise.resolve() + await Promise.resolve() + + expect(kill).toHaveBeenCalledTimes(66) + expect(kill.mock.calls.filter(([id]) => id === IDS[0])).toHaveLength(1) + expect(kill.mock.calls.filter(([id]) => id === IDS[1])).toHaveLength(2) + }) +}) diff --git a/src/renderer/src/lib/pty-kill-retry-ownership.ts b/src/renderer/src/lib/pty-kill-retry-ownership.ts index b808c894b83..1c4e5c137e9 100644 --- a/src/renderer/src/lib/pty-kill-retry-ownership.ts +++ b/src/renderer/src/lib/pty-kill-retry-ownership.ts @@ -1,28 +1,61 @@ type RetainedPtyKill = { diagnostic: string inFlight: Promise | null + options?: PtyKillIdentity + attempts: number + nextRetryAt: number } +export type PtyKillIdentity = { + expectedPaneKey?: string + expectedTabId?: string +} + +const MAX_RETAINED_PTY_KILLS = 64 +const MAX_RETRY_BACKOFF_MS = 30_000 const retainedPtyKills = new Map() /** * Keep exact PTY identity until the owning provider accepts shutdown. A rejected * local/SSH IPC call can otherwise leave a live process with no renderer owner. */ -export function killPtyRetainingRetryOwnership(id: string, diagnostic: string): Promise { - const retained = retainedPtyKills.get(id) ?? { diagnostic, inFlight: null } +export function killPtyRetainingRetryOwnership( + id: string, + diagnostic: string, + options?: PtyKillIdentity +): Promise { + let retained = retainedPtyKills.get(id) + if (!retained) { + if (retainedPtyKills.size >= MAX_RETAINED_PTY_KILLS) { + const oldestId = retainedPtyKills.keys().next().value + if (typeof oldestId === 'string') { + retainedPtyKills.delete(oldestId) + console.warn('[pty] Dropped oldest retained shutdown retry at bounded capacity') + } + } + retained = { diagnostic, inFlight: null, options, attempts: 0, nextRetryAt: 0 } + } retained.diagnostic = diagnostic + retained.options = options ?? retained.options retainedPtyKills.set(id, retained) if (retained.inFlight) { return retained.inFlight } const attempt = Promise.resolve() - .then(() => window.api.pty.kill(id)) + .then(() => + retained.options ? window.api.pty.kill(id, retained.options) : window.api.pty.kill(id) + ) .then(() => { retainedPtyKills.delete(id) }) .catch((error: unknown) => { + retained.attempts += 1 + retained.nextRetryAt = + retained.attempts === 1 + ? 0 + : Date.now() + + Math.min(MAX_RETRY_BACKOFF_MS, 250 * 2 ** Math.min(retained.attempts - 1, 7)) console.warn(retained.diagnostic, error) throw error }) @@ -37,8 +70,12 @@ export function killPtyRetainingRetryOwnership(id: string, diagnostic: string): /** Retry on the next PTY lifecycle event; no polling or permanent timer is added. */ export function retryRetainedPtyKills(): void { + const now = Date.now() for (const [id, retained] of retainedPtyKills) { - void killPtyRetainingRetryOwnership(id, retained.diagnostic).catch(() => {}) + if (!retained.inFlight && retained.nextRetryAt <= now) { + void killPtyRetainingRetryOwnership(id, retained.diagnostic, retained.options).catch(() => {}) + return + } } } diff --git a/src/renderer/src/store/slices/repos.test.ts b/src/renderer/src/store/slices/repos.test.ts index e3a69686e33..2676e67b303 100644 --- a/src/renderer/src/store/slices/repos.test.ts +++ b/src/renderer/src/store/slices/repos.test.ts @@ -744,7 +744,7 @@ describe('repo slice runtime routing', () => { params: { worktree: `id:${worktreeId}` }, timeoutMs: 15_000 }) - expect(ptyKill).toHaveBeenCalledWith('pty-local-stale') + expect(ptyKill).toHaveBeenCalledWith('pty-local-stale', { expectedTabId: 'tab-1' }) expect(ptyKill).not.toHaveBeenCalledWith('remote:term-1') }) @@ -834,7 +834,7 @@ describe('repo slice runtime routing', () => { expect(store.getState().detectedWorktreesByRepo[localRepo.id]).toBeUndefined() expect(store.getState().tabsByWorktree[hiddenWorktree.id]).toBeUndefined() expect(store.getState().activeWorktreeId).toBeNull() - expect(ptyKill).toHaveBeenCalledWith('pty-hidden') + expect(ptyKill).toHaveBeenCalledWith('pty-hidden', { expectedTabId: 'tab-hidden' }) }) it('reorders repos through the active remote runtime environment', async () => { diff --git a/src/renderer/src/store/slices/repos.ts b/src/renderer/src/store/slices/repos.ts index 4c4a7cba417..fffa2ff051f 100644 --- a/src/renderer/src/store/slices/repos.ts +++ b/src/renderer/src/store/slices/repos.ts @@ -2793,7 +2793,8 @@ export const createRepoSlice: StateCreator = (set, localPtyKills.push( killPtyRetainingRetryOwnership( ptyId, - '[pty] Failed to stop PTY while removing project' + '[pty] Failed to stop PTY while removing project', + { expectedTabId: tab.id } ) ) } diff --git a/src/shared/constants.test.ts b/src/shared/constants.test.ts index e185c4d073f..343e0ffe173 100644 --- a/src/shared/constants.test.ts +++ b/src/shared/constants.test.ts @@ -52,6 +52,10 @@ describe('getDefaultSettings', () => { expect(getDefaultSettings('/tmp').confirmClosePinnedTab).toBe(true) }) + it('keeps file-editor word wrapping enabled by default', () => { + expect(getDefaultSettings('/tmp').editorWordWrap).toBe(true) + }) + it('keeps rich Markdown spellcheck enabled by default', () => { expect(getDefaultSettings('/tmp').richMarkdownSpellcheckEnabled).toBe(true) }) diff --git a/src/shared/constants.ts b/src/shared/constants.ts index 26c547ad20a..950cc999477 100644 --- a/src/shared/constants.ts +++ b/src/shared/constants.ts @@ -208,6 +208,7 @@ export function getDefaultSettings(homedir: string): GlobalSettings { editorAutoSave: false, editorAutoSaveDelayMs: DEFAULT_EDITOR_AUTO_SAVE_DELAY_MS, editorMinimapEnabled: false, + editorWordWrap: true, richMarkdownSpellcheckEnabled: true, markdownReviewToolsEnabled: true, primarySelectionMiddleClickPaste: getDefaultPrimarySelectionMiddleClickPaste(), diff --git a/src/shared/ssh-pty-id.test.ts b/src/shared/ssh-pty-id.test.ts index b8fcfd8d373..17eecb70053 100644 --- a/src/shared/ssh-pty-id.test.ts +++ b/src/shared/ssh-pty-id.test.ts @@ -17,6 +17,21 @@ describe('ssh pty id routing', () => { expect(toAppSshPtyId(CONNECTION, APP_ID)).toBe(APP_ID) }) + it('scopes recycled relay ids to the relay process generation', () => { + const first = toAppSshPtyId(CONNECTION, 'pty-1', 'relay-a') + const second = toAppSshPtyId(CONNECTION, 'pty-1', 'relay-b') + + expect(first).toBe(`ssh:${CONNECTION}@@relay-a@@pty-1`) + expect(second).not.toBe(first) + expect(parseAppSshPtyId(first)).toEqual({ + connectionId: CONNECTION, + relayInstanceId: 'relay-a', + relayPtyId: 'pty-1' + }) + expect(toRelaySshPtyId(CONNECTION, first)).toBe('pty-1') + expect(toAppSshPtyId(CONNECTION, first, 'relay-b')).toBe(second) + }) + // Why: reconnect/restore callers may pass the execution-host id form // ("ssh:") from a workspace `hostId`; it names the same connection // as the bare id embedded in the app pty id and must not throw or misencode. diff --git a/src/shared/ssh-pty-id.ts b/src/shared/ssh-pty-id.ts index 5f7d2a5663e..dc07f5377dd 100644 --- a/src/shared/ssh-pty-id.ts +++ b/src/shared/ssh-pty-id.ts @@ -17,6 +17,7 @@ function normalizeConnectionId(connectionId: string): string { // needs the target id embedded so two relays cannot collide after restore. export type ParsedSshPtyId = { connectionId: string + relayInstanceId?: string relayPtyId: string } @@ -29,13 +30,23 @@ export function parseAppSshPtyId(ptyId: string): ParsedSshPtyId | null { return null } const encodedConnectionId = ptyId.slice(SSH_PTY_ID_PREFIX.length, separatorIndex) - const relayPtyId = ptyId.slice(separatorIndex + SSH_PTY_ID_SEPARATOR.length) + const relayIdentity = ptyId.slice(separatorIndex + SSH_PTY_ID_SEPARATOR.length) + const generationSeparatorIndex = relayIdentity.indexOf(SSH_PTY_ID_SEPARATOR) + const encodedRelayInstanceId = + generationSeparatorIndex === -1 ? null : relayIdentity.slice(0, generationSeparatorIndex) + const relayPtyId = + generationSeparatorIndex === -1 + ? relayIdentity + : relayIdentity.slice(generationSeparatorIndex + SSH_PTY_ID_SEPARATOR.length) if (!encodedConnectionId || !relayPtyId) { return null } try { return { connectionId: decodeURIComponent(encodedConnectionId), + ...(encodedRelayInstanceId + ? { relayInstanceId: decodeURIComponent(encodedRelayInstanceId) } + : {}), relayPtyId } } catch { @@ -43,16 +54,26 @@ export function parseAppSshPtyId(ptyId: string): ParsedSshPtyId | null { } } -export function toAppSshPtyId(connectionId: string, relayPtyId: string): string { +export function toAppSshPtyId( + connectionId: string, + relayPtyId: string, + relayInstanceId?: string +): string { const normalizedConnectionId = normalizeConnectionId(connectionId) const parsed = parseAppSshPtyId(relayPtyId) if (parsed) { if (parsed.connectionId !== normalizedConnectionId) { throw new Error(`PTY ${relayPtyId} belongs to SSH connection "${parsed.connectionId}"`) } - return relayPtyId + if (!relayInstanceId || parsed.relayInstanceId === relayInstanceId) { + return relayPtyId + } + relayPtyId = parsed.relayPtyId } - return `${SSH_PTY_ID_PREFIX}${encodeURIComponent(normalizedConnectionId)}${SSH_PTY_ID_SEPARATOR}${relayPtyId}` + const generationSegment = relayInstanceId + ? `${encodeURIComponent(relayInstanceId)}${SSH_PTY_ID_SEPARATOR}` + : '' + return `${SSH_PTY_ID_PREFIX}${encodeURIComponent(normalizedConnectionId)}${SSH_PTY_ID_SEPARATOR}${generationSegment}${relayPtyId}` } export function toRelaySshPtyId(connectionId: string, ptyId: string): string { diff --git a/src/shared/types.ts b/src/shared/types.ts index bc6bcfc38a3..7b0c9b5ca84 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -2512,6 +2512,8 @@ export type GlobalSettings = { editorAutoSave: boolean editorAutoSaveDelayMs: number editorMinimapEnabled: boolean + /** Defaults on for profiles saved before file-editor wrapping became configurable. */ + editorWordWrap?: boolean /** Persisted opt-out for browser spellcheck noise in rich Markdown editing surfaces. */ richMarkdownSpellcheckEnabled?: boolean /** Whether local markdown review note controls and the review panel are shown. */ diff --git a/tests/e2e/terminal-shortcuts.spec.ts b/tests/e2e/terminal-shortcuts.spec.ts index c6c40afe6fc..17d837aa1ce 100644 --- a/tests/e2e/terminal-shortcuts.spec.ts +++ b/tests/e2e/terminal-shortcuts.spec.ts @@ -518,19 +518,26 @@ test.describe('Terminal Shortcuts', () => { await waitForPaneCount(orcaPage, 1, 30_000) }) - test('Shift+Enter writes the platform newline chord for terminal TUIs', async ({ - orcaPage, - electronApp - }) => { + test('Shift+Enter follows the pane Kitty keyboard state', async ({ orcaPage, electronApp }) => { await installMainProcessPtyWriteSpy(electronApp) - await waitForActivePanePtyId(orcaPage) + const ptyId = await waitForActivePanePtyId(orcaPage) - await pressAndExpectWrite( - orcaPage, - electronApp, - 'Shift+Enter', - process.platform === 'win32' ? '\x1b\r' : '\x1b[13;2u' - ) + await pressAndExpectWrite(orcaPage, electronApp, 'Shift+Enter', '\x1b\r') + if (process.platform === 'win32') { + return + } + + // Why: exercise the production PTY-output tracker, not xterm's renderer- + // local flag state, so the test covers the bytes the shortcut policy sees. + await execInTerminal(orcaPage, ptyId, "printf '\\033[>1u'") + await expect.poll(() => getKittyKeyboardFlags(orcaPage)).toBe(1) + await pressAndExpectWrite(orcaPage, electronApp, 'Shift+Enter', '\x1b[13;2u') + + // The shell is only standing in for a KKP-aware TUI and does not consume + // the CSI-u input above, so cancel that synthetic line before its reset. + await execInTerminal(orcaPage, ptyId, "\x03printf '\\033[ getKittyKeyboardFlags(orcaPage)).toBe(0) + await pressAndExpectWrite(orcaPage, electronApp, 'Shift+Enter', '\x1b\r') }) test('Droid gets CSI-u Shift+Enter on Windows without changing Antigravity', async ({ @@ -769,14 +776,8 @@ test.describe('Terminal Shortcuts', () => { // Ctrl+Backspace → \x17 (unix-word-rubout). await pressAndExpectWrite(orcaPage, electronApp, 'Control+Backspace', '\x17') - // Shift+Enter stays distinct; Windows keeps Esc+CR unless the active agent - // explicitly requires CSI-u (currently Droid, #7620). - await pressAndExpectWrite( - orcaPage, - electronApp, - 'Shift+Enter', - process.platform === 'win32' ? '\x1b\r' : '\x1b[13;2u' - ) + // The shell has not enabled KKP, so Shift+Enter must not leak CSI-u text. + await pressAndExpectWrite(orcaPage, electronApp, 'Shift+Enter', '\x1b\r') // --- send-input chords (macOS-only) ---