From feed851f45178cf2dde74a76e265df624103e1cf Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Sun, 6 Sep 2026 19:54:21 -0400 Subject: [PATCH] fix: combine terminal visibility with safe create admission --- ...ca-runtime-create-terminal-desktop.test.ts | 66 ++++++++ .../orca-runtime-create-terminal-desktop.ts | 17 +- .../orca-runtime-mobile-create-reply.test.ts | 74 +++++++++ ...time-run-create-mobile-session-terminal.ts | 8 +- src/main/runtime/rpc/errors.ts | 1 + .../new-external-worktrees-inbox-actions.ts | 4 +- .../terminal-request-ipc-bridge.test.ts | 156 ++++++++++++++++++ .../ipc-events/terminal-request-ipc-bridge.ts | 18 ++ .../terminal-ui-routing-ipc-bridge.ts | 43 +++-- .../terminal-worktree-visibility.test.ts | 71 +++++++- .../terminal-worktree-visibility.ts | 23 +-- src/renderer/src/i18n/locales/en.json | 3 +- .../src/lib/terminal-worktree-route.ts | 45 ++++- .../src/lib/terminal-worktree-surface.test.ts | 82 +++++++++ src/shared/terminal-reveal-identity.ts | 1 + 15 files changed, 556 insertions(+), 56 deletions(-) create mode 100644 src/main/runtime/orca-runtime-create-terminal-desktop.test.ts create mode 100644 src/main/runtime/orca-runtime-mobile-create-reply.test.ts create mode 100644 src/renderer/src/hooks/ipc-events/terminal-request-ipc-bridge.test.ts create mode 100644 src/renderer/src/lib/terminal-worktree-surface.test.ts diff --git a/src/main/runtime/orca-runtime-create-terminal-desktop.test.ts b/src/main/runtime/orca-runtime-create-terminal-desktop.test.ts new file mode 100644 index 00000000000..20ed6543ff9 --- /dev/null +++ b/src/main/runtime/orca-runtime-create-terminal-desktop.test.ts @@ -0,0 +1,66 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { BrowserWindow, IpcMainEvent } from 'electron' +import type { TerminalTabCreateReply } from '../../shared/terminal-reveal-identity' +import type { OrcaRuntimeWithCreateTerminal } from './orca-runtime-create-terminal' +import { createDesktopTerminal } from './orca-runtime-create-terminal-desktop' +import { mapRuntimeError } from './rpc/errors' + +const desktop = vi.hoisted(() => ({ onIpc: vi.fn(), removeIpcListener: vi.fn() })) +vi.mock('./orca-runtime-create-terminal-dependencies', () => ({ + randomUUID: () => 'request-1', + getRuntimeDesktopSurface: () => desktop, + ownerSurfacing: () => ({}) +})) + +afterEach(() => { + vi.useRealTimers() + vi.clearAllMocks() +}) + +describe('desktop terminal creation replies', () => { + it.each([ + { errorCode: 'worktree_not_renderable', rpcCode: 'worktree_not_renderable' }, + { errorCode: undefined, rpcCode: 'runtime_error' } + ])('rejects before waiting for a handle with code $errorCode', async ({ errorCode, rpcCode }) => { + vi.useFakeTimers() + const send = vi.fn((_channel: string, request: { requestId: string }) => { + const handler = desktop.onIpc.mock.calls[0][1] as ( + event: IpcMainEvent, + reply: TerminalTabCreateReply + ) => void + handler({ sender: win.webContents } as IpcMainEvent, { + requestId: request.requestId, + error: 'Show this worktree in the sidebar before creating a terminal.', + ...(errorCode ? { errorCode } : {}) + }) + }) + const win = { webContents: { send } } as unknown as BrowserWindow + const waitForTerminalHandle = vi.fn() + const runtime = { + assertGraphReady: vi.fn(), + resolveTerminalWorkspaceLaunchScope: vi.fn().mockResolvedValue({ id: 'wt-hidden' }), + resolveAgentTerminalCreateOptions: vi.fn().mockResolvedValue({ command: 'claude' }), + resolveWorkspaceTerminalStartupCwd: vi.fn().mockReturnValue('/repo/hidden'), + waitForTerminalHandle + } as unknown as OrcaRuntimeWithCreateTerminal + + const error = await createDesktopTerminal(runtime, 'wt-hidden', {}, 'focused', win).catch( + (error: unknown) => error + ) + + expect(error).toBeInstanceOf(Error) + expect(mapRuntimeError('rpc-1', { runtimeId: 'runtime-1' }, error)).toMatchObject({ + ok: false, + error: { + code: rpcCode, + message: 'Show this worktree in the sidebar before creating a terminal.' + } + }) + expect(waitForTerminalHandle).not.toHaveBeenCalled() + expect(desktop.removeIpcListener).toHaveBeenCalledWith( + 'terminal:tabCreateReply', + desktop.onIpc.mock.calls[0][1] + ) + expect(vi.getTimerCount()).toBe(0) + }) +}) diff --git a/src/main/runtime/orca-runtime-create-terminal-desktop.ts b/src/main/runtime/orca-runtime-create-terminal-desktop.ts index 7e02a749447..bb193667e90 100644 --- a/src/main/runtime/orca-runtime-create-terminal-desktop.ts +++ b/src/main/runtime/orca-runtime-create-terminal-desktop.ts @@ -2,6 +2,7 @@ import * as dependencies from './orca-runtime-create-terminal-dependencies' import type { OrcaRuntimeWithCreateTerminal } from './orca-runtime-create-terminal' import type { RuntimeTerminalPresentation } from '../../shared/runtime-types' +import type { TerminalTabCreateReply } from '../../shared/terminal-reveal-identity' export async function createDesktopTerminal( runtime: OrcaRuntimeWithCreateTerminal, @@ -28,22 +29,18 @@ export async function createDesktopTerminal( dependencies.getRuntimeDesktopSurface().removeIpcListener('terminal:tabCreateReply', handler) reject(new Error('Terminal creation timed out')) }, 10000) - const handler = ( - event: dependencies.IpcMainEvent, - response: { - requestId: string - tabId?: string - title?: string - error?: string - } - ): void => { + const handler = (event: dependencies.IpcMainEvent, response: TerminalTabCreateReply): void => { if (event.sender !== win.webContents || response.requestId !== requestId) { return } clearTimeout(timer) dependencies.getRuntimeDesktopSurface().removeIpcListener('terminal:tabCreateReply', handler) if (response.error) { - reject(new Error(response.error)) + const error = new Error(response.error) + if (response.errorCode) { + Object.assign(error, { code: response.errorCode }) + } + reject(error) } else { resolve({ tabId: response.tabId!, title: response.title ?? launchOpts.title ?? '' }) } diff --git a/src/main/runtime/orca-runtime-mobile-create-reply.test.ts b/src/main/runtime/orca-runtime-mobile-create-reply.test.ts new file mode 100644 index 00000000000..259b6f4edb8 --- /dev/null +++ b/src/main/runtime/orca-runtime-mobile-create-reply.test.ts @@ -0,0 +1,74 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { TerminalTabCreateReply } from '../../shared/terminal-reveal-identity' +import { mapRuntimeError } from './rpc/errors' + +const desktop = vi.hoisted(() => ({ onIpc: vi.fn(), removeIpcListener: vi.fn() })) +vi.mock('./runtime-desktop-surface', () => ({ getRuntimeDesktopSurface: () => desktop })) +vi.mock('./orca-runtime-create-mobile-session-terminal', () => ({ + OrcaRuntimeWithCreateMobileSessionTerminal: class {} +})) +vi.mock('./orca-runtime-core', () => ({ + MOBILE_TERMINAL_READY_FALLBACK_MS: 1000, + MOBILE_TERMINAL_SURFACE_TIMEOUT_MS: 1000, + isClientDisconnectedError: () => false +})) + +import { OrcaRuntimeWithRunCreateMobileSessionTerminal } from './orca-runtime-run-create-mobile-session-terminal' + +afterEach(() => { + vi.clearAllMocks() + vi.useRealTimers() +}) + +describe('mobile terminal create reply errors', () => { + it.each(['worktree_not_renderable', undefined])( + 'preserves code %s before surface waiting', + async (errorCode) => { + vi.useFakeTimers() + const win = { + webContents: { + isDestroyed: () => false, + send: vi.fn((_channel, request) => { + const handler = desktop.onIpc.mock.calls[0][1] + handler({ sender: win.webContents }, { + requestId: request.requestId, + error: 'Show this worktree first', + ...(errorCode ? { errorCode } : {}) + } satisfies TerminalTabCreateReply) + }) + } + } + const release = vi.fn() + const runtime = { + captureReadyGraphEpoch: vi.fn(), + resolveTerminalWorkspaceLaunchScope: vi.fn().mockResolvedValue({ id: 'hidden' }), + resolveWorkspaceTerminalStartupCwd: vi.fn(), + hydrateHeadlessMobileSessionTabsFromWorkspaceSession: vi.fn(), + resolveMobileSessionTerminalCommand: vi.fn().mockResolvedValue({}), + assertStableReadyGraph: vi.fn(), + getAvailableAuthoritativeWindow: () => win, + rendererPublicationThrottle: { acquire: () => release }, + waitForMobileTerminalSurface: vi.fn() + } + const run = ( + OrcaRuntimeWithRunCreateMobileSessionTerminal.prototype as unknown as { + runCreateMobileSessionTerminal: ( + id: string, + opts: { clientNavigationId: string } + ) => Promise + } + ).runCreateMobileSessionTerminal + const error = await run + .call(runtime, 'hidden', { clientNavigationId: 'navigation' }) + .catch((error: unknown) => error) + expect(mapRuntimeError('request', { runtimeId: 'runtime' }, error)).toMatchObject({ + ok: false, + error: { code: errorCode ?? 'runtime_error', message: 'Show this worktree first' } + }) + expect(runtime.waitForMobileTerminalSurface).not.toHaveBeenCalled() + expect(release).toHaveBeenCalledOnce() + expect(desktop.removeIpcListener).toHaveBeenCalledOnce() + expect(vi.getTimerCount()).toBe(0) + } + ) +}) diff --git a/src/main/runtime/orca-runtime-run-create-mobile-session-terminal.ts b/src/main/runtime/orca-runtime-run-create-mobile-session-terminal.ts index 0caedfa1bba..007f274f19e 100644 --- a/src/main/runtime/orca-runtime-run-create-mobile-session-terminal.ts +++ b/src/main/runtime/orca-runtime-run-create-mobile-session-terminal.ts @@ -7,6 +7,7 @@ import type { RuntimeMobileSessionCreateTerminalResult } from '../../shared/runt import { randomUUID } from 'node:crypto' import { getRuntimeDesktopSurface } from './runtime-desktop-surface' import type { IpcMainEvent } from 'electron' +import type { TerminalTabCreateReply } from '../../shared/terminal-reveal-identity' import { MOBILE_TERMINAL_READY_FALLBACK_MS, MOBILE_TERMINAL_SURFACE_TIMEOUT_MS, @@ -97,10 +98,7 @@ export class OrcaRuntimeWithRunCreateMobileSessionTerminal extends OrcaRuntimeWi reject(new Error('client_disconnected')) } - const handler = ( - event: IpcMainEvent, - r: { requestId: string; tabId?: string; title?: string; error?: string } - ): void => { + const handler = (event: IpcMainEvent, r: TerminalTabCreateReply): void => { if (event.sender !== win.webContents || r.requestId !== requestId) { return } @@ -108,7 +106,7 @@ export class OrcaRuntimeWithRunCreateMobileSessionTerminal extends OrcaRuntimeWi getRuntimeDesktopSurface().removeIpcListener('terminal:tabCreateReply', handler) opts.signal?.removeEventListener('abort', onAbort) if (r.error) { - reject(new Error(r.error)) + reject(Object.assign(new Error(r.error), r.errorCode ? { code: r.errorCode } : {})) } else { resolve({ tabId: r.tabId!, title: r.title ?? '' }) } diff --git a/src/main/runtime/rpc/errors.ts b/src/main/runtime/rpc/errors.ts index bbf918f4f55..ba781bf5b17 100644 --- a/src/main/runtime/rpc/errors.ts +++ b/src/main/runtime/rpc/errors.ts @@ -80,6 +80,7 @@ const COMPUTER_PASSTHROUGH_CODES: ReadonlySet = new Set(Object.values(CO const LINEAR_PASSTHROUGH_CODES: ReadonlySet = new Set(LINEAR_ERROR_CODES) const STRUCTURED_RUNTIME_PASSTHROUGH_CODES: ReadonlySet = new Set([ 'worktree_id_requires_full_path', + 'worktree_not_renderable', 'run_not_found', 'run_required', 'stable_pane_required', diff --git a/src/renderer/src/components/sidebar/new-external-worktrees-inbox-actions.ts b/src/renderer/src/components/sidebar/new-external-worktrees-inbox-actions.ts index f9a4534410d..3c8b89ebad1 100644 --- a/src/renderer/src/components/sidebar/new-external-worktrees-inbox-actions.ts +++ b/src/renderer/src/components/sidebar/new-external-worktrees-inbox-actions.ts @@ -103,7 +103,7 @@ export async function keepNewExternalWorktreeInboxHidden( export async function importNewExternalWorktreeInboxPaths( args: NewExternalWorktreesInboxActionDeps -): Promise { +): Promise { const importedExternalWorktreePaths = mergeExternalWorktreeInboxPaths( args.repo.importedExternalWorktreePaths, args.worktreePaths @@ -112,7 +112,7 @@ export async function importNewExternalWorktreeInboxPaths( args.repo.externalWorktreeInboxBaselinePaths, args.worktreePaths ) - await refreshAfterRepoInboxUpdate( + return await refreshAfterRepoInboxUpdate( args, { importedExternalWorktreePaths, externalWorktreeInboxBaselinePaths }, { diff --git a/src/renderer/src/hooks/ipc-events/terminal-request-ipc-bridge.test.ts b/src/renderer/src/hooks/ipc-events/terminal-request-ipc-bridge.test.ts new file mode 100644 index 00000000000..b1ee168ace3 --- /dev/null +++ b/src/renderer/src/hooks/ipc-events/terminal-request-ipc-bridge.test.ts @@ -0,0 +1,156 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { createTestStore, makeWorktree, seedStore } from '../../store/slices/store-test-helpers' +import { buildTerminalCreateWindow } from '../ipc-events-terminal-create-window-test-fixtures' +import type * as TerminalCommandState from './terminal-command-state' +import type { TerminalTabCreateReply } from '../../../../shared/terminal-reveal-identity' + +const WORKTREE_ID = 'repo1::/path/visible' +const HIDDEN_ID = 'repo1::/path/hidden' +const DUPLICATE_ID = 'repo2::/path/visible' +let store: ReturnType +const reply = vi.fn<(data: TerminalTabCreateReply) => void>() +const mount = vi.fn() +const activate = vi.fn() + +vi.mock('../../store', () => ({ useAppStore: { getState: () => store.getState() } })) +vi.mock('@/components/terminal/background-terminal-worktree-mount', () => ({ + requestBackgroundTerminalWorktreeMount: (...args: unknown[]) => mount(...args) +})) +vi.mock('./terminal-command-state', async (importOriginal) => ({ + ...(await importOriginal()), + activateTerminalInitiatedWorktree: (...args: unknown[]) => activate(...args), + focusTerminalInitiatedTab: vi.fn() +})) + +import { registerTerminalRequestIpcBridge } from './terminal-request-ipc-bridge' + +type Request = Parameters[0]>[0] +let request: (data: Request) => void + +beforeEach(() => { + vi.clearAllMocks() + store = createTestStore() + seedStore(store, { + worktreesByRepo: { repo1: [makeWorktree({ id: WORKTREE_ID, repoId: 'repo1' })] }, + detectedWorktreesByRepo: { + repo1: { + repoId: 'repo1', + source: 'git', + authoritative: true, + worktrees: [ + { + ...makeWorktree({ id: HIDDEN_ID, repoId: 'repo1', path: '/path/hidden' }), + ownership: 'external', + selectedCheckout: false, + visible: false + } + ] + } + } + }) + store.setState({ + repos: [...store.getState().repos, { ...store.getState().repos[0], id: 'repo2' }] + }) + const listener = { current: null as unknown } + vi.stubGlobal( + 'window', + buildTerminalCreateWindow({ + dispatchEvent: vi.fn(), + replyTerminalCreate: reply, + requestTerminalCreateListenerRef: listener, + createTerminalListenerRef: { current: null }, + focusTerminalListenerRef: { current: null }, + newTerminalTabListenerRef: { current: null } + }) + ) + registerTerminalRequestIpcBridge([]) + request = listener.current as typeof request +}) + +afterEach(() => vi.unstubAllGlobals()) + +describe('renderer terminal create admission', () => { + it.each([ + [HIDDEN_ID, 'background'], + [DUPLICATE_ID, 'background'] + ] as const)( + 'refuses %s (%s) without creating or queueing anything', + (worktreeId, presentation) => { + const createTab = vi.spyOn(store.getState(), 'createTab') + const queue = vi.spyOn(store.getState(), 'queueTabStartupCommand') + const before = store.getState() + + request({ requestId: 'hidden', worktreeId, presentation, command: 'codex' }) + + expect(reply).toHaveBeenCalledExactlyOnceWith({ + requestId: 'hidden', + errorCode: 'worktree_not_renderable', + error: expect.stringContaining('Show it in Non-Orca worktrees') + }) + expect(createTab).not.toHaveBeenCalled() + expect(queue).not.toHaveBeenCalled() + expect(mount).not.toHaveBeenCalled() + expect(activate).not.toHaveBeenCalled() + expect(store.getState()).toBe(before) + } + ) + + it('creates a visible background tab with its startup command', () => { + request({ + requestId: 'visible', + worktreeId: WORKTREE_ID, + presentation: 'background', + command: 'codex' + }) + const tab = store.getState().tabsByWorktree[WORKTREE_ID][0] + expect(reply).toHaveBeenCalledExactlyOnceWith({ + requestId: 'visible', + tabId: tab.id, + title: tab.title + }) + expect(store.getState().pendingStartupByTabId[tab.id]).toEqual({ command: 'codex' }) + expect(mount).toHaveBeenCalledWith({ worktreeId: WORKTREE_ID, tabIds: [tab.id] }) + }) + + it('retires the new tab and queued command if the success reply throws', () => { + const sibling = store.getState().createTab(WORKTREE_ID) + store.getState().queueTabStartupCommand(sibling.id, { command: 'keep me' }) + const close = vi.spyOn(store.getState(), 'closeTab') + reply.mockImplementationOnce(() => { + throw new Error('Reply transport failed') + }) + + request({ + requestId: 'failed', + worktreeId: WORKTREE_ID, + presentation: 'background', + command: 'codex' + }) + + expect(close).toHaveBeenCalledWith(expect.any(String), { + reason: 'cleanup', + recordInteraction: false + }) + expect(store.getState().tabsByWorktree[WORKTREE_ID]).toEqual([sibling]) + expect(store.getState().pendingStartupByTabId).toEqual({ [sibling.id]: { command: 'keep me' } }) + expect(store.getState().unifiedTabsByWorktree[WORKTREE_ID]).toHaveLength(1) + expect(store.getState().recentlyClosedTerminalTabsByWorktree[WORKTREE_ID] ?? []).toEqual([]) + expect(reply).toHaveBeenLastCalledWith({ requestId: 'failed', error: 'Reply transport failed' }) + }) + + it('cleans up when creation fails after allocating the tab but before queueing', () => { + vi.spyOn(store.getState(), 'setTabCustomTitle').mockImplementationOnce(() => { + throw new Error('Title failed') + }) + request({ + requestId: 'failed', + worktreeId: WORKTREE_ID, + presentation: 'background', + title: 'Agent', + command: 'codex' + }) + expect(store.getState().tabsByWorktree[WORKTREE_ID]).toEqual([]) + expect(store.getState().pendingStartupByTabId).toEqual({}) + expect(reply).toHaveBeenLastCalledWith({ requestId: 'failed', error: 'Title failed' }) + }) +}) diff --git a/src/renderer/src/hooks/ipc-events/terminal-request-ipc-bridge.ts b/src/renderer/src/hooks/ipc-events/terminal-request-ipc-bridge.ts index f9e43cbe4e3..87934c81173 100644 --- a/src/renderer/src/hooks/ipc-events/terminal-request-ipc-bridge.ts +++ b/src/renderer/src/hooks/ipc-events/terminal-request-ipc-bridge.ts @@ -18,6 +18,7 @@ import { export function registerTerminalRequestIpcBridge(unsubs: (() => void)[]): void { unsubs.push( window.api.ui.onRequestTerminalCreate(async (data) => { + let createdTabId: string | undefined try { let store = useAppStore.getState() const worktreeId = data.worktreeId ?? store.activeWorktreeId @@ -58,6 +59,17 @@ export function registerTerminalRequestIpcBridge(unsubs: (() => void)[]): void { await ensureTerminalWorktreeVisible(worktreeId) store = useAppStore.getState() } + if (!hasTerminalWorktreeRow(store, worktreeId)) { + window.api.ui.replyTerminalCreate({ + requestId: data.requestId, + errorCode: 'worktree_not_renderable', + error: translate( + 'auto.hooks.useIpcEvents.worktreeNotRenderable', + 'This window has no terminal surface for the worktree. Show it in Non-Orca worktrees or select a visible workspace, then retry.' + ) + }) + return + } if (shouldActivate) { activateTerminalInitiatedWorktree(store, worktreeId) } @@ -86,6 +98,7 @@ export function registerTerminalRequestIpcBridge(unsubs: (() => void)[]): void { ...(data.cwd ? { startupCwd: data.cwd } : {}) } const tab = store.createTab(worktreeId, data.targetGroupId, undefined, tabOptions) + createdTabId = tab.id if (!shouldActivate) { // Why: renderer-backed Codex startup must mount its new TerminalPane without switching UI or connecting every saved tab. requestBackgroundTerminalWorktreeMount({ worktreeId, tabIds: [tab.id] }) @@ -150,6 +163,11 @@ export function registerTerminalRequestIpcBridge(unsubs: (() => void)[]): void { title: data.title ?? tab.title }) } catch (err) { + if (createdTabId) { + useAppStore + .getState() + .closeTab(createdTabId, { reason: 'cleanup', recordInteraction: false }) + } window.api.ui.replyTerminalCreate({ requestId: data.requestId, error: err instanceof Error ? err.message : 'Terminal creation failed' diff --git a/src/renderer/src/hooks/ipc-events/terminal-ui-routing-ipc-bridge.ts b/src/renderer/src/hooks/ipc-events/terminal-ui-routing-ipc-bridge.ts index f56ba7ab2bf..a9c5993eb84 100644 --- a/src/renderer/src/hooks/ipc-events/terminal-ui-routing-ipc-bridge.ts +++ b/src/renderer/src/hooks/ipc-events/terminal-ui-routing-ipc-bridge.ts @@ -1,3 +1,8 @@ +import { + ensureTerminalWorktreeVisible, + hasTerminalWorktreeRow +} from './terminal-worktree-visibility' +import { toast } from 'sonner' import type { SplitTerminalPaneDetail } from '@/constants/terminal' import { requestBackgroundTerminalWorktreeMount } from '@/components/terminal/background-terminal-worktree-mount' import { @@ -117,7 +122,7 @@ export function registerTerminalUiRoutingIpcBridge(unsubs: (() => void)[]): void unsubs.push( window.api.ui.onFocusTerminal( - ({ + async ({ tabId, worktreeId, leafId, @@ -125,21 +130,29 @@ export function registerTerminalUiRoutingIpcBridge(unsubs: (() => void)[]): void flashFocusedPane, scrollToBottomIfOutputSinceLastView }) => { - const store = useAppStore.getState() - activateTerminalInitiatedWorktree(store, worktreeId) - store.setActiveTab(tabId) - store.revealWorktreeInSidebar(worktreeId) - if (ackPaneKeyOnSuccess || flashFocusedPane || scrollToBottomIfOutputSinceLastView) { - activateTabAndFocusPane(tabId, leafId ?? null, { - ...(ackPaneKeyOnSuccess ? { ackPaneKeyOnSuccess } : {}), - ...(flashFocusedPane ? { flashFocusedPane: true } : {}), - ...(scrollToBottomIfOutputSinceLastView - ? { scrollToBottomIfOutputSinceLastView: true } - : {}) - }) - return + try { + let store = useAppStore.getState() + if (!hasTerminalWorktreeRow(store, worktreeId)) { + await ensureTerminalWorktreeVisible(worktreeId) + store = useAppStore.getState() + } + activateTerminalInitiatedWorktree(store, worktreeId) + store.setActiveTab(tabId) + store.revealWorktreeInSidebar(worktreeId) + if (ackPaneKeyOnSuccess || flashFocusedPane || scrollToBottomIfOutputSinceLastView) { + activateTabAndFocusPane(tabId, leafId ?? null, { + ...(ackPaneKeyOnSuccess ? { ackPaneKeyOnSuccess } : {}), + ...(flashFocusedPane ? { flashFocusedPane: true } : {}), + ...(scrollToBottomIfOutputSinceLastView + ? { scrollToBottomIfOutputSinceLastView: true } + : {}) + }) + return + } + focusTerminalInitiatedTab(tabId, leafId, worktreeId) + } catch (error) { + toast.error(error instanceof Error ? error.message : String(error)) } - focusTerminalInitiatedTab(tabId, leafId, worktreeId) } ) ) diff --git a/src/renderer/src/hooks/ipc-events/terminal-worktree-visibility.test.ts b/src/renderer/src/hooks/ipc-events/terminal-worktree-visibility.test.ts index 75fe793f960..e5360fc267c 100644 --- a/src/renderer/src/hooks/ipc-events/terminal-worktree-visibility.test.ts +++ b/src/renderer/src/hooks/ipc-events/terminal-worktree-visibility.test.ts @@ -66,7 +66,10 @@ describe.each(['reveal', 'create'] as const)('hidden worktree %s bridge', (bridg await new Promise((resolve) => { finishRefresh = resolve }) - s.state.worktreesByRepo['repo-1'].push({ id: s.worktreeId } as never) + s.state.worktreesByRepo = { + ...s.state.worktreesByRepo, + 'repo-1': [...s.state.worktreesByRepo['repo-1'], { id: s.worktreeId, hostId } as never] + } return true }) const pending = invoke(s) @@ -140,7 +143,14 @@ describe.each(['reveal', 'create'] as const)('hidden worktree %s bridge', (bridg await invoke(s, false) expect(s.updateRepo).not.toHaveBeenCalled() expect(s.setActiveWorktree).not.toHaveBeenCalled() - expect(s.createTab).toHaveBeenCalled() + if (bridge === 'reveal') { + expect(s.createTab).toHaveBeenCalled() + } else { + expect(s.createTab).not.toHaveBeenCalled() + expect(s.replyTerminalCreate).toHaveBeenCalledWith( + expect.objectContaining({ errorCode: 'worktree_not_renderable' }) + ) + } }) it('accepts a folder workspace without importing it as a git worktree', async () => { @@ -184,11 +194,11 @@ describe('terminal worktree activation safety', () => { } s.state.detectedWorktreesByRepo['repo-1'].worktrees.push(second) s.fetchWorktrees.mockImplementation(async () => { - s.state.worktreesByRepo['repo-1'] = s.state.detectedWorktreesByRepo[ - 'repo-1' - ].worktrees.filter((row) => - s.state.repos[0].importedExternalWorktreePaths?.includes(row.path) - ) + s.state.worktreesByRepo = { + 'repo-1': s.state.detectedWorktreesByRepo['repo-1'].worktrees.filter((row) => + s.state.repos[0].importedExternalWorktreePaths?.includes(row.path) + ) + } return true }) const { ensureTerminalWorktreeVisible } = await import('./terminal-worktree-visibility') @@ -203,3 +213,50 @@ describe('terminal worktree activation safety', () => { ]) }) }) + +describe('direct terminal focus events', () => { + it('imports before focusing the requested terminal', async () => { + const s = await setup() + await s.focusTerminalListenerRef.current!({ worktreeId: s.worktreeId, tabId: 'existing' }) + expect(s.updateRepo).toHaveBeenCalledTimes(1) + expect(s.setActiveWorktree).toHaveBeenCalledWith(s.worktreeId) + expect(s.setActiveTab).toHaveBeenCalledWith('existing') + }) + + it('handles a failed import without an uncaught rejection or navigation', async () => { + const s = await setup() + s.updateRepo.mockResolvedValue(false) + await expect( + s.focusTerminalListenerRef.current!({ worktreeId: s.worktreeId, tabId: 'existing' }) + ).resolves.toBeUndefined() + expect(s.setActiveWorktree).not.toHaveBeenCalled() + expect(s.setActiveTab).not.toHaveBeenCalled() + }) +}) + +describe.each(['ssh:server', 'runtime:server'] as const)('same-ID %s ownership', (hostId) => { + it('imports the selected host despite a visible local row with the same ID', async () => { + const s = await setup(hostId) + s.state.activeWorktreeId = s.worktreeId + s.state.activeWorkspaceExecutionHostId = hostId + s.state.worktreesByRepo = { + 'repo-1': [{ id: s.worktreeId, repoId: 'repo-1', hostId: 'local' } as never] + } + const { ensureTerminalWorktreeVisible, hasTerminalWorktreeRow } = + await import('./terminal-worktree-visibility') + expect(hasTerminalWorktreeRow(s.state, s.worktreeId)).toBe(false) + await ensureTerminalWorktreeVisible(s.worktreeId) + expect(s.updateRepo).toHaveBeenCalledWith('repo-1', expect.anything(), { hostId }) + expect(hasTerminalWorktreeRow(s.state, s.worktreeId)).toBe(true) + }) + + it('refuses ambiguous ownership instead of accepting the visible local row', async () => { + const s = await setup(hostId) + s.state.worktreesByRepo = { + 'repo-1': [{ id: s.worktreeId, repoId: 'repo-1', hostId: 'local' } as never] + } + const { ensureTerminalWorktreeVisible } = await import('./terminal-worktree-visibility') + await expect(ensureTerminalWorktreeVisible(s.worktreeId)).rejects.toThrow('worktree_hidden') + expect(s.updateRepo).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/hooks/ipc-events/terminal-worktree-visibility.ts b/src/renderer/src/hooks/ipc-events/terminal-worktree-visibility.ts index b46fab8a8db..9f252f7e935 100644 --- a/src/renderer/src/hooks/ipc-events/terminal-worktree-visibility.ts +++ b/src/renderer/src/hooks/ipc-events/terminal-worktree-visibility.ts @@ -1,9 +1,8 @@ -import type { AppState } from '../../store/types' -import { parseWorkspaceKey } from '../../../../shared/workspace-scope' +import { hasRenderableTerminalWorktreeSurface as hasTerminalWorktreeRow } from '@/lib/terminal-worktree-route' import { importNewExternalWorktreeInboxPaths } from '@/components/sidebar/new-external-worktrees-inbox-actions' import { resolveWorktreeOperationRoute } from '@/lib/worktree-operation-route' import { findRepoForHost, getRepoHostIdentity } from '@/store/slices/repo-host-identity' -import { findWorktreeById, getRepoIdFromWorktreeId } from '@/store/slices/worktree-helpers' +import { getRepoIdFromWorktreeId } from '@/store/slices/worktree-helpers' import { worktreeHostMatchOptions, worktreeMatchesHost @@ -12,12 +11,7 @@ import { useAppStore } from '../../store' const pendingImports = new Map>() -export function hasTerminalWorktreeRow(state: AppState, worktreeId: string): boolean { - if (parseWorkspaceKey(worktreeId)?.type === 'folder') { - return Boolean(state.getKnownWorktreeById(worktreeId)) - } - return Boolean(findWorktreeById(state.worktreesByRepo, worktreeId)) -} +export { hasRenderableTerminalWorktreeSurface as hasTerminalWorktreeRow } from '@/lib/terminal-worktree-route' export function hiddenTerminalWorktreeError(): Error { return new Error('worktree_hidden: Terminal workspace could not be shown in the sidebar') @@ -29,7 +23,9 @@ export async function ensureTerminalWorktreeVisible(worktreeId: string): Promise return } const route = resolveWorktreeOperationRoute(state, worktreeId) - const hostId = route?.executionHostId + const hostId = route?.runtimeEnvironmentId + ? (`runtime:${encodeURIComponent(route.runtimeEnvironmentId)}` as const) + : route?.executionHostId if (!hostId) { throw hiddenTerminalWorktreeError() } @@ -57,17 +53,14 @@ export async function ensureTerminalWorktreeVisible(worktreeId: string): Promise if (!targetRepo || detected?.length !== 1 || detected[0].visible) { throw hiddenTerminalWorktreeError() } - let imported = false - await importNewExternalWorktreeInboxPaths({ + const imported = await importNewExternalWorktreeInboxPaths({ projectId: repoId, repo: targetRepo, worktreePaths: [detected[0].path], updateRepo: (id, updates) => current.updateRepo(id, updates, { hostId }), fetchWorktrees: (id, options) => current.fetchWorktrees(id, { ...options, executionHostId: hostId }), - setInboxState: (_id, status) => { - imported = status === null - } + setInboxState: () => {} }) if (!imported || !hasTerminalWorktreeRow(useAppStore.getState(), worktreeId)) { throw hiddenTerminalWorktreeError() diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index e5c4a5fb08e..9f21db0534b 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -1029,7 +1029,8 @@ "ef223fbb6b": "A device tried to connect but is not paired", "11992d0337": "If this was your phone or another Orca client, re-pair it from Settings → Mobile.", "6573cfe955": "Open Mobile Settings", - "unresolvedTerminalWorktreeOwner": "Terminal creation is unavailable because the worktree owner could not be resolved" + "unresolvedTerminalWorktreeOwner": "Terminal creation is unavailable because the worktree owner could not be resolved", + "worktreeNotRenderable": "This window has no terminal surface for the worktree. Show it in Non-Orca worktrees or select a visible workspace, then retry." }, "useSettingsNavigationMetadata": { "4a728cd56b": "New features that are still taking shape. Give them a try.", diff --git a/src/renderer/src/lib/terminal-worktree-route.ts b/src/renderer/src/lib/terminal-worktree-route.ts index 06e43c23075..2c8cab3b269 100644 --- a/src/renderer/src/lib/terminal-worktree-route.ts +++ b/src/renderer/src/lib/terminal-worktree-route.ts @@ -3,18 +3,61 @@ import { isEphemeralSetupTerminalWorktreeId } from '../../../shared/ephemeral-se import { parseExecutionHostId } from '../../../shared/execution-host' import { parseWorkspaceKey } from '../../../shared/workspace-scope' import type { AppState } from '@/store/types' +import { getIndexedWorktreesById } from '@/store/worktree-repo-index' +import { getRepoIdFromWorktreeId } from '@/store/slices/worktree-helpers' +import { + worktreeMatchesHost, + worktreeHostMatchOptions +} from '@/store/slices/worktrees/listing/worktree-host-ownership' +import { + resolveWorktreeOperationRoute, + resolveWorktreeOperationRouteResult +} from './worktree-operation-route' import { getExplicitRuntimeEnvironmentIdForWorktree, getRuntimeEnvironmentIdForWorktree, type WorktreeRuntimeOwnerState } from './worktree-runtime-owner' -import { resolveWorktreeOperationRouteResult } from './worktree-operation-route' import { getSingleFocusedRuntimeEnvironmentId } from './single-runtime-legacy-owner' export type TerminalWorktreeRoute = { runtimeEnvironmentId: string | null } +export function hasRenderableTerminalWorktreeSurface( + state: AppState, + worktreeId: string | null | undefined +): boolean { + if (!worktreeId) { + return false + } + if (worktreeId === FLOATING_TERMINAL_WORKTREE_ID) { + return true + } + const scope = parseWorkspaceKey(worktreeId) + if (scope?.type === 'folder') { + return ( + state.folderWorkspaces?.some((workspace) => workspace.id === scope.folderWorkspaceId) ?? false + ) + } + // Only the workbench's row index can host new tabs; detected rows and inline setup ids cannot. + const route = resolveWorktreeOperationRoute(state, worktreeId) + const hostId = route?.executionHostId + if (!hostId) { + return false + } + const repoId = getRepoIdFromWorktreeId(worktreeId) + const catalogHostId = route.runtimeEnvironmentId + ? (`runtime:${encodeURIComponent(route.runtimeEnvironmentId)}` as const) + : hostId + const matchOptions = worktreeHostMatchOptions(state, repoId, catalogHostId) + return state.worktreesByRepo + ? getIndexedWorktreesById(state.worktreesByRepo, worktreeId).some((row) => + worktreeMatchesHost(row, catalogHostId, matchOptions) + ) + : false +} + /** * Which host owns a terminal surface. Spawn and teardown both resolve it here so they can never * disagree about whether an id is routable — #9994 fixed spawn only, and STA-2639 was the teardown diff --git a/src/renderer/src/lib/terminal-worktree-surface.test.ts b/src/renderer/src/lib/terminal-worktree-surface.test.ts new file mode 100644 index 00000000000..805f0f058aa --- /dev/null +++ b/src/renderer/src/lib/terminal-worktree-surface.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it } from 'vitest' +import type { AppState } from '@/store/types' +import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../shared/constants' +import { brandEphemeralSetupTerminalWorktreeId } from '../../../shared/ephemeral-setup-terminal-worktree-id' +import { folderWorkspaceKey } from '../../../shared/workspace-scope' +import { hasRenderableTerminalWorktreeSurface } from './terminal-worktree-route' + +function state(overrides: Partial = {}): AppState { + return { + repos: [{ id: 'repo-1', executionHostId: 'local' }], + worktreesByRepo: { + 'repo-1': [{ id: 'repo-1::/workspace', repoId: 'repo-1', path: '/workspace' }] + }, + folderWorkspaces: [], + ...overrides + } as unknown as AppState +} + +describe('hasRenderableTerminalWorktreeSurface', () => { + it('accepts an exact workbench row for local, SSH, and paired runtime worktrees', () => { + for (const hostId of ['local', 'ssh:connection', 'runtime:hub']) { + const store = state({ + worktreesByRepo: { + 'repo-1': [{ id: 'repo-1::/workspace', repoId: 'repo-1', hostId }] + } + } as unknown as Partial) + expect(hasRenderableTerminalWorktreeSurface(store, 'repo-1::/workspace')).toBe(true) + } + }) + + it('rejects a hidden detected worktree even when its repo is registered', () => { + const store = state({ + detectedWorktreesByRepo: { + 'repo-1': { worktrees: [{ id: 'repo-1::/external', repoId: 'repo-1' }] } + } + } as unknown as Partial) + expect(hasRenderableTerminalWorktreeSurface(store, 'repo-1::/external')).toBe(false) + }) + + it('rejects a duplicate repo registration id that shares a rendered path', () => { + const store = state({ + repos: [{ id: 'repo-1' }, { id: 'duplicate-repo' }] + } as unknown as Partial) + expect(hasRenderableTerminalWorktreeSurface(store, 'duplicate-repo::/workspace')).toBe(false) + }) + + it('accepts an imported worktree once its exact row reaches the workbench catalog', () => { + const store = state() + const worktreeId = 'repo-1::/external' + expect(hasRenderableTerminalWorktreeSurface(store, worktreeId)).toBe(false) + const imported = state({ + worktreesByRepo: { + ...store.worktreesByRepo, + 'repo-1': [...store.worktreesByRepo['repo-1'], { id: worktreeId, repoId: 'repo-1' }] + } + } as unknown as Partial) + expect(hasRenderableTerminalWorktreeSurface(imported, worktreeId)).toBe(true) + }) + + it('requires a real folder workspace rather than a syntactically valid key', () => { + const store = state({ + folderWorkspaces: [{ id: 'folder-1', folderPath: '/folder' }] + } as unknown as Partial) + expect(hasRenderableTerminalWorktreeSurface(store, folderWorkspaceKey('folder-1'))).toBe(true) + expect(hasRenderableTerminalWorktreeSurface(store, folderWorkspaceKey('missing'))).toBe(false) + }) + + it('preserves the separately hosted floating terminal surface', () => { + expect(hasRenderableTerminalWorktreeSurface(state(), FLOATING_TERMINAL_WORKTREE_ID)).toBe(true) + }) + + it('rejects inline setup ids whose components only render their own created tab', () => { + const id = brandEphemeralSetupTerminalWorktreeId('settings-cli-skill-terminal') + expect(hasRenderableTerminalWorktreeSurface(state(), id)).toBe(false) + }) + + it('fails closed for missing ids or unhydrated catalogs', () => { + for (const id of [null, undefined, '', 'repo-1::/workspace']) { + expect(hasRenderableTerminalWorktreeSurface({} as AppState, id)).toBe(false) + } + }) +}) diff --git a/src/shared/terminal-reveal-identity.ts b/src/shared/terminal-reveal-identity.ts index 514e45e3b6d..856972597de 100644 --- a/src/shared/terminal-reveal-identity.ts +++ b/src/shared/terminal-reveal-identity.ts @@ -11,4 +11,5 @@ export type TerminalTabCreateReply = { title?: string identity?: TerminalRevealIdentity error?: string + errorCode?: string }