From 102ca09a383eada395dbe4930e19d48e33a9329a Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Sun, 17 May 2026 15:43:53 -0400 Subject: [PATCH] Fix Windows dev terminal PATH and shell icons (#2152) --- src/main/ipc/pty.test.ts | 19 +++- src/main/ipc/pty.ts | 7 +- .../src/components/tab-bar/SortableTab.tsx | 14 ++- .../tab-group/useTabGroupWorkspaceModel.ts | 58 ++++++++----- .../src/store/slices/store-cascades.test.ts | 70 +++++++++++++++ src/renderer/src/store/slices/terminals.ts | 47 +++++++++- src/shared/types.ts | 7 +- tests/e2e/windows-terminal-env-icons.spec.ts | 87 +++++++++++++++++++ 8 files changed, 269 insertions(+), 40 deletions(-) create mode 100644 tests/e2e/windows-terminal-env-icons.spec.ts diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 651128c7a15..f4ba75065a9 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -2,6 +2,7 @@ one focused file because the registration helper is stateful and each spawn-path assertion reuses the same mocked IPC and node-pty harness. */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { delimiter, join } from 'node:path' const { handleMock, @@ -780,7 +781,23 @@ describe('registerPtyHandlers', () => { try { const env = await daemonSpawnAndGetEnv({ PATH: '/usr/bin' }) expect(env.ORCA_USER_DATA_PATH).toBe('/tmp/orca-user-data') - expect(env.PATH).toContain('/tmp/orca-user-data/cli/bin') + expect(env.PATH).toContain(join('/tmp/orca-user-data', 'cli', 'bin')) + } finally { + mockedApp.isPackaged = prev + } + }) + + it('preserves the inherited PATH when dev-mode daemon env omits PATH', async () => { + const { app } = await import('electron') + const mockedApp = app as unknown as { isPackaged: boolean } + const prev = mockedApp.isPackaged + mockedApp.isPackaged = false + try { + const env = await daemonSpawnAndGetEnv({}, undefined, undefined, { + PATH: '/system/bin' + }) + expect(env.ORCA_USER_DATA_PATH).toBe('/tmp/orca-user-data') + expect(env.PATH).toBe(`${join('/tmp/orca-user-data', 'cli', 'bin')}${delimiter}/system/bin`) } finally { mockedApp.isPackaged = prev } diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 903bc867e99..224d79c03b1 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -229,6 +229,10 @@ export type BuildPtyHostEnvOptions = { githubAttributionEnabled: boolean } +function readInheritedPath(baseEnv: Record): string { + return baseEnv.PATH ?? process.env.PATH ?? process.env.Path ?? '' +} + /** * Mutates `baseEnv` in place with all host-local PTY env vars and returns it. * @@ -337,11 +341,12 @@ export function buildPtyHostEnv( if (!opts.isPackaged) { baseEnv.ORCA_USER_DATA_PATH ??= opts.userDataPath const devCliBin = join(opts.userDataPath, 'cli', 'bin') + const inheritedPath = readInheritedPath(baseEnv) // Why: avoid a trailing delimiter when PATH is empty — some shells // treat an empty segment as `.`, which would let commands resolve from // the current working directory (a foot-gun we don't want to create // for dev terminals). - baseEnv.PATH = baseEnv.PATH ? `${devCliBin}${delimiter}${baseEnv.PATH}` : devCliBin + baseEnv.PATH = inheritedPath ? `${devCliBin}${delimiter}${inheritedPath}` : devCliBin } // Why: GitHub attribution should only affect commands launched from diff --git a/src/renderer/src/components/tab-bar/SortableTab.tsx b/src/renderer/src/components/tab-bar/SortableTab.tsx index 79c99c22371..33ad80c2368 100644 --- a/src/renderer/src/components/tab-bar/SortableTab.tsx +++ b/src/renderer/src/components/tab-bar/SortableTab.tsx @@ -81,15 +81,10 @@ export default function SortableTab({ // because the slice returns a fresh object reference on each mark/clear. const hasUnreadActivity = useAppStore((s) => s.unreadTerminalTabs[tab.id] === true) - // Why: on Windows, tabs created before the per-tab shell override landed (or - // created via the default Ctrl+T path without picking a specific shell) - // don't carry a shellOverride. We still want the tab-strip icon to reflect - // the shell actually running, so fall back to the user's configured default - // Windows shell. On mac/linux this resolves to undefined and the ShellIcon - // generic-terminal fallback renders. - const defaultWindowsShell = useAppStore((s) => s.settings?.terminalWindowsShell) - const isWindows = navigator.userAgent.includes('Windows') - const shellForIcon = tab.shellOverride ?? (isWindows ? defaultWindowsShell : undefined) + // Why: createTab stamps the shell used at creation time, so changing the + // default shell later does not repaint existing tabs as a different shell. + // Older persisted tabs without this field fall back to the generic icon. + const shellForIcon = tab.shellOverride // Why: intentionally no transform/transition/opacity here. The PR's // design is that tabs stay visually anchored during a drag — only the @@ -276,6 +271,7 @@ export default function SortableTab({ // desaturating the brand colors beyond recognition. diff --git a/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts b/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts index 6059902b579..d76d36f2684 100644 --- a/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts +++ b/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts @@ -5,7 +5,12 @@ import { useCallback, useMemo } from 'react' import { toast } from 'sonner' import { useShallow } from 'zustand/react/shallow' import type { OpenFile } from '@/store/slices/editor' -import type { BrowserTab as BrowserTabState, Tab, TabGroup } from '../../../../shared/types' +import type { + BrowserTab as BrowserTabState, + Tab, + TabGroup, + TerminalTab +} from '../../../../shared/types' import { useAppStore } from '../../store' import { useAllWorktrees } from '../../store/selectors' import { createUntitledMarkdownFile } from '../../lib/create-untitled-markdown' @@ -21,18 +26,9 @@ export type GroupBrowserItem = BrowserTabState & { tabId: string } const EMPTY_GROUPS: readonly TabGroup[] = [] const EMPTY_UNIFIED_TABS: readonly Tab[] = [] const EMPTY_BROWSER_TABS: readonly BrowserTabState[] = [] +const EMPTY_TERMINAL_TABS: readonly TerminalTab[] = [] -type TerminalTabItem = { - id: string - unifiedTabId: string - ptyId: null - worktreeId: string - title: string - customTitle: string | null - color: string | null - sortOrder: number - createdAt: number -} +type TerminalTabItem = TerminalTab & { unifiedTabId: string } export function useTabGroupWorkspaceModel({ groupId, @@ -51,6 +47,7 @@ export function useTabGroupWorkspaceModel({ // and blanks the window as soon as TabGroupPanel mounts. groups: state.groupsByWorktree[worktreeId] ?? EMPTY_GROUPS, unifiedTabs: state.unifiedTabsByWorktree[worktreeId] ?? EMPTY_UNIFIED_TABS, + terminalTabs: state.tabsByWorktree[worktreeId] ?? EMPTY_TERMINAL_TABS, openFiles: state.openFiles, browserTabs: state.browserTabsByWorktree[worktreeId] ?? EMPTY_BROWSER_TABS, expandedPaneByTabId: state.expandedPaneByTabId @@ -92,23 +89,36 @@ export function useTabGroupWorkspaceModel({ ) const activeItemId = group?.activeTabId ?? null const activeTab = groupTabs.find((item) => item.id === activeItemId) ?? null + // Why: split groups render tab labels from unified tabs, but terminal shell + // identity lives on the terminal tab so icons survive default-shell changes. + const terminalTabById = useMemo( + () => new Map(worktreeState.terminalTabs.map((item) => [item.id, item])), + [worktreeState.terminalTabs] + ) const terminalTabs = useMemo( () => groupTabs .filter((item) => item.contentType === 'terminal') - .map((item) => ({ - id: item.entityId, - unifiedTabId: item.id, - ptyId: null, - worktreeId, - title: item.label, - customTitle: item.customLabel ?? null, - color: item.color ?? null, - sortOrder: item.sortOrder, - createdAt: item.createdAt - })), - [groupTabs, worktreeId] + .map((item) => { + const terminalTab = terminalTabById.get(item.entityId) + return { + id: item.entityId, + unifiedTabId: item.id, + ptyId: terminalTab?.ptyId ?? null, + worktreeId, + title: item.label, + defaultTitle: terminalTab?.defaultTitle, + customTitle: item.customLabel ?? terminalTab?.customTitle ?? null, + color: item.color ?? terminalTab?.color ?? null, + sortOrder: item.sortOrder, + createdAt: item.createdAt, + generation: terminalTab?.generation, + shellOverride: terminalTab?.shellOverride, + pendingActivationSpawn: terminalTab?.pendingActivationSpawn + } + }), + [groupTabs, terminalTabById, worktreeId] ) const editorItems = useMemo( diff --git a/src/renderer/src/store/slices/store-cascades.test.ts b/src/renderer/src/store/slices/store-cascades.test.ts index 214d75dc547..00d36b3cbf8 100644 --- a/src/renderer/src/store/slices/store-cascades.test.ts +++ b/src/renderer/src/store/slices/store-cascades.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' import { buildWorktreeComparator } from '@/components/sidebar/smart-sort' import type * as AgentStatusModule from '@/lib/agent-status' +import { getDefaultSettings } from '../../../../shared/constants' // Mock sonner (imported by repos.ts) vi.mock('sonner', () => ({ toast: { info: vi.fn(), success: vi.fn(), error: vi.fn() } })) @@ -628,6 +629,75 @@ describe('setActiveWorktree', () => { expect(groups[0].tabOrder).toEqual([terminal.id]) }) + it('stamps the Windows default shell onto new terminal tabs', () => { + const originalNavigator = globalThis.navigator + Object.defineProperty(globalThis, 'navigator', { + value: { userAgent: 'Mozilla/5.0 (Windows NT 10.0; Win64; x64)' }, + configurable: true + }) + try { + const store = createTestStore() + const wt = 'repo1::/path/wt1' + + seedStore(store, { + settings: { ...getDefaultSettings('/tmp'), terminalWindowsShell: 'wsl.exe' }, + worktreesByRepo: { + repo1: [makeWorktree({ id: wt, repoId: 'repo1', path: '/path/wt1' })] + } + }) + + const terminal = store.getState().createTab(wt) + expect(terminal.shellOverride).toBe('wsl.exe') + + store.setState({ + settings: { ...store.getState().settings!, terminalWindowsShell: 'cmd.exe' } + }) + expect(store.getState().tabsByWorktree[wt][0].shellOverride).toBe('wsl.exe') + } finally { + Object.defineProperty(globalThis, 'navigator', { + value: originalNavigator, + configurable: true + }) + } + }) + + it('does not stamp local Windows shell icons onto SSH terminal tabs', () => { + const originalNavigator = globalThis.navigator + Object.defineProperty(globalThis, 'navigator', { + value: { userAgent: 'Mozilla/5.0 (Windows NT 10.0; Win64; x64)' }, + configurable: true + }) + try { + const store = createTestStore() + const wt = 'remote-repo::/path/wt1' + + seedStore(store, { + repos: [ + { + id: 'remote-repo', + path: '/remote/repo', + displayName: 'Remote Repo', + badgeColor: '#000', + addedAt: 0, + connectionId: 'ssh-1' + } + ], + settings: { ...getDefaultSettings('/tmp'), terminalWindowsShell: 'wsl.exe' }, + worktreesByRepo: { + 'remote-repo': [makeWorktree({ id: wt, repoId: 'remote-repo', path: '/path/wt1' })] + } + }) + + const terminal = store.getState().createTab(wt, undefined, 'cmd.exe') + expect(terminal.shellOverride).toBeUndefined() + } finally { + Object.defineProperty(globalThis, 'navigator', { + value: originalNavigator, + configurable: true + }) + } + }) + it('publishes the first terminal and root tab group atomically', () => { const store = createTestStore() const wt = 'repo1::/path/wt1' diff --git a/src/renderer/src/store/slices/terminals.ts b/src/renderer/src/store/slices/terminals.ts index 5fa80efd927..a368ee00367 100644 --- a/src/renderer/src/store/slices/terminals.ts +++ b/src/renderer/src/store/slices/terminals.ts @@ -59,6 +59,44 @@ function getFallbackTabTitle(tab: TerminalTab, index?: number): string { ) } +function isWindowsRendererRuntime(): boolean { + return typeof navigator !== 'undefined' && navigator.userAgent.includes('Windows') +} + +function resolveCreatedTabShellOverride( + explicitShellOverride: string | undefined, + defaultWindowsShell: string | undefined, + isRemoteWorktree: boolean +): string | undefined { + if (isRemoteWorktree) { + return undefined + } + if (explicitShellOverride !== undefined) { + return explicitShellOverride + } + if (isWindowsRendererRuntime()) { + return defaultWindowsShell + } + return undefined +} + +function worktreeUsesRemoteConnection( + state: Pick, + worktreeId: string +): boolean { + const directRepoId = getRepoIdFromWorktreeId(worktreeId) + const directRepo = state.repos.find((repo) => repo.id === directRepoId) + if (directRepo) { + return Boolean(directRepo.connectionId) + } + + const worktree = Object.values(state.worktreesByRepo) + .flat() + .find((entry) => entry.id === worktreeId) + const repo = worktree ? state.repos.find((entry) => entry.id === worktree.repoId) : null + return Boolean(repo?.connectionId) +} + export type TerminalSlice = { tabsByWorktree: Record activeTabId: string | null @@ -384,6 +422,13 @@ export const createTerminalSlice: StateCreator const shouldActivate = options?.activate !== false const nextOrdinal = getNextTerminalOrdinal(existing) const defaultTitle = `Terminal ${nextOrdinal}` + const createdShellOverride = resolveCreatedTabShellOverride( + shellOverride, + s.settings?.terminalWindowsShell, + // Why: SSH PTYs ignore local Windows shell selection; persisting a + // local shell icon would mislabel a remote terminal. + worktreeUsesRemoteConnection(s, worktreeId) + ) tab = { id, // Why: CLI-created background sessions already own a PTY; revealing @@ -399,7 +444,7 @@ export const createTerminalSlice: StateCreator color: null, sortOrder: existing.length, createdAt: Date.now(), - ...(shellOverride !== undefined ? { shellOverride } : {}), + ...(createdShellOverride !== undefined ? { shellOverride: createdShellOverride } : {}), // Why: when Terminal.tsx's activation fallback auto-creates a tab for a // first-visit worktree, the resulting PTY spawn is caused by the user // clicking the worktree, not by work happening in it. Tagging the tab diff --git a/src/shared/types.ts b/src/shared/types.ts index 832205c8ddc..c537f64806f 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -335,10 +335,9 @@ export type TerminalTab = { createdAt: number /** Bumped on shutdown so TerminalPane remounts with a fresh PTY. */ generation?: number - /** Why: records the shell this tab was explicitly opened with (e.g. 'wsl.exe' - * from the "+" submenu) so the PTY can re-use the same shell on reconnect - * without needing the user to interact with the tab again. Undefined means - * "use the default shell setting". */ + /** Why: records the shell this tab was opened with (e.g. 'wsl.exe') so the + * PTY and tab icon stay stable even if the default shell setting changes + * later. Older persisted tabs may omit this field. */ shellOverride?: string /** Why: when `setActiveWorktree` bumps generation on all-dead tabs to drive a * TerminalPane remount, the fresh PTY that results is caused by navigation, diff --git a/tests/e2e/windows-terminal-env-icons.spec.ts b/tests/e2e/windows-terminal-env-icons.spec.ts new file mode 100644 index 00000000000..85080c3162a --- /dev/null +++ b/tests/e2e/windows-terminal-env-icons.spec.ts @@ -0,0 +1,87 @@ +import { test, expect } from './helpers/orca-app' +import { ensureTerminalVisible, waitForActiveWorktree, waitForSessionReady } from './helpers/store' +import { + execInTerminal, + waitForActivePanePtyId, + waitForActiveTerminalManager, + waitForTerminalOutput +} from './helpers/terminal' + +test.describe('Windows terminal env and shell identity', () => { + test.beforeEach(async ({ orcaPage }) => { + await waitForSessionReady(orcaPage) + await waitForActiveWorktree(orcaPage) + await ensureTerminalVisible(orcaPage) + }) + + test('dev terminal preserves parent PATH so PATH commands resolve', async ({ orcaPage }) => { + await waitForActiveTerminalManager(orcaPage) + + const ptyId = await waitForActivePanePtyId(orcaPage) + const marker = `__ORCA_E2E_NODE_PATH_${Date.now()}__` + + // Why: before the dev PATH fallback, daemon-spawned PTYs could get PATH set + // to only Orca's dev CLI bin. A real terminal command catches that failure. + await execInTerminal(orcaPage, ptyId, `node -e "console.log('${marker}')"`) + + await waitForTerminalOutput(orcaPage, marker, 15_000) + }) + + test('Windows tab icons stay pinned to the shell used at tab creation', async ({ orcaPage }) => { + test.skip(process.platform !== 'win32', 'Windows shell icons only render on Windows') + + const tabIds = await orcaPage.evaluate(() => { + const store = window.__store + if (!store) { + throw new Error('Store unavailable') + } + const state = store.getState() + const worktreeId = state.activeWorktreeId + if (!worktreeId) { + throw new Error('No active worktree') + } + + store.setState({ + settings: { ...state.settings!, terminalWindowsShell: 'wsl.exe' } + }) + const wslTab = store.getState().createTab(worktreeId, undefined, undefined, { + activate: false + }) + + store.setState({ + settings: { ...store.getState().settings!, terminalWindowsShell: 'cmd.exe' } + }) + const cmdTab = store.getState().createTab(worktreeId, undefined, undefined, { + activate: false + }) + + return { wslTabId: wslTab.id, cmdTabId: cmdTab.id } + }) + + const tabSnapshot = await orcaPage.evaluate(({ wslTabId, cmdTabId }) => { + const state = window.__store!.getState() + const tabs = Object.values(state.tabsByWorktree).flat() + return { + wslShell: tabs.find((tab) => tab.id === wslTabId)?.shellOverride, + cmdShell: tabs.find((tab) => tab.id === cmdTabId)?.shellOverride + } + }, tabIds) + + expect(tabSnapshot).toEqual({ + wslShell: 'wsl.exe', + cmdShell: 'cmd.exe' + }) + + const wslTab = orcaPage.locator( + `[data-testid="sortable-tab"][data-tab-id="${tabIds.wslTabId}"]` + ) + const cmdTab = orcaPage.locator( + `[data-testid="sortable-tab"][data-tab-id="${tabIds.cmdTabId}"]` + ) + await expect(wslTab).toBeVisible() + await expect(cmdTab).toBeVisible() + + await expect(wslTab.locator('[data-shell-icon]')).toHaveAttribute('data-shell-icon', 'wsl.exe') + await expect(cmdTab.locator('[data-shell-icon]')).toHaveAttribute('data-shell-icon', 'cmd.exe') + }) +})