diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 7a5eff241e6..00739f36350 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -17,6 +17,10 @@ on: description: JSON array of changed specs; empty runs the full suite required: false type: string + ssh_source_changed: + description: '"true" when the PR touches SSH execution source; gates the Docker-SSH lane' + required: false + type: string workflow_dispatch: inputs: ref: @@ -221,14 +225,21 @@ jobs: ssh-docker-watcher-isolation: name: ssh docker watcher isolation needs: build + # Why ssh_source_changed first: this lane used to trigger on SSH source only as a side + # effect of one route listing a startup-readiness spec — pruning that spec would have + # silently retired the whole lane. The signal is now derived from the SSH routes directly. + # The two spec clauses stay for their honest purpose: changed-e2e hands these specs to this + # lane, so editing one must still run it here. if: >- inputs.test_files == '' || + inputs.ssh_source_changed == 'true' || contains(inputs.test_files, 'tests/e2e/ssh-startup-exec-readiness.spec.ts') || contains(inputs.test_files, 'tests/e2e/paired-startup-exec-readiness.spec.ts') runs-on: ubuntu-latest - # Why 35: the parking, retention, startup-exec, and paired parity specs run - # serially on isolated Electron/SSH fixtures after watcher isolation. - timeout-minutes: 35 + # Why 60: this lane now also runs the remaining Docker-SSH specs serially. They average + # ~18s but several budget 4-10 minutes per test, so a slow run lands far above the old 35 + # — and the sharded lanes already show that a lane which times out is a lane nobody trusts. + timeout-minutes: 60 steps: - name: Checkout @@ -260,6 +271,14 @@ jobs: if: always() run: xvfb-run --auto-servernum env SKIP_BUILD=1 ORCA_E2E_FORWARD_APP_LOGS=1 pnpm run test:e2e:ssh-docker-terminal-parking + # Why here rather than the sharded lanes: the shards set no ORCA_E2E_SSH_DOCKER, so every + # spec below skipped itself while the shard still reported green. Running them on this one + # VM pays the fixture image build once instead of ten times, and keeps an SSH regression + # legible as an SSH-named failure. + - name: Run remaining Docker SSH E2E + if: always() + run: xvfb-run --auto-servernum env SKIP_BUILD=1 ORCA_E2E_FORWARD_APP_LOGS=1 pnpm run test:e2e:ssh-docker + - name: Upload watcher isolation traces if: failure() uses: actions/upload-artifact@v7 diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 9fbe35a173f..9f3b14f5903 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -804,6 +804,7 @@ jobs: outputs: should_run: ${{ steps.filter.outputs.should_run }} test_files: ${{ steps.filter.outputs.test_files }} + ssh_source_changed: ${{ steps.filter.outputs.ssh_source_changed }} steps: - name: Checkout uses: actions/checkout@v6 @@ -826,6 +827,11 @@ jobs: # authorities, exclusions, and sentinels without evaluating workflow shell. TEST_FILES_JSON="$(printf '%s\n' "$CHANGED" | node config/scripts/pr-e2e-source-routing.mjs)" echo "test_files=$TEST_FILES_JSON" >> "$GITHUB_OUTPUT" + # Why a separate signal: the Docker-SSH lane must trigger on SSH source, not on a + # spec name surviving in a route's list. Same routes, so the two cannot drift. + SSH_SOURCE_CHANGED="$(printf '%s\n' "$CHANGED" | node config/scripts/pr-e2e-source-routing.mjs --ssh-source)" + echo "ssh_source_changed=$SSH_SOURCE_CHANGED" >> "$GITHUB_OUTPUT" + echo "SSH source changed: $SSH_SOURCE_CHANGED" if [ "$TEST_FILES_JSON" != '[]' ]; then echo "should_run=true" >> "$GITHUB_OUTPUT" echo "Changed E2E specs: $TEST_FILES_JSON" @@ -844,6 +850,7 @@ jobs: uses: ./.github/workflows/e2e.yml with: test_files: ${{ needs.e2e-paths.outputs.test_files }} + ssh_source_changed: ${{ needs.e2e-paths.outputs.ssh_source_changed }} verify: if: always() diff --git a/config/scripts/pr-e2e-gate-contract.test.mjs b/config/scripts/pr-e2e-gate-contract.test.mjs index 665edd01ec0..8cd622b300c 100644 --- a/config/scripts/pr-e2e-gate-contract.test.mjs +++ b/config/scripts/pr-e2e-gate-contract.test.mjs @@ -1,9 +1,14 @@ -import { existsSync, readFileSync } from 'node:fs' +import { existsSync, readdirSync, readFileSync } from 'node:fs' import { join, resolve } from 'node:path' import { parse as parseJsonc } from 'jsonc-parser' import { describe, expect, it } from 'vitest' import { parse as parseYaml } from 'yaml' -import { PR_E2E_SOURCE_ROUTES, selectPrE2eSpecs } from './pr-e2e-source-routing.mjs' +import { + hasSshSourceChange, + PR_E2E_SOURCE_ROUTES, + selectPrE2eSpecs, + SSH_SOURCE_ROUTE_IDS +} from './pr-e2e-source-routing.mjs' const projectDir = resolve(import.meta.dirname, '../..') const prWorkflow = parseYaml(readFileSync(join(projectDir, '.github/workflows/pr.yml'), 'utf8')) @@ -209,6 +214,8 @@ describe('PR E2E gate contract', () => { 'src/main/providers/ssh-', 'src/main/ipc/pty', 'src/relay/', + 'src/shared/ssh-', + 'src/renderer/src/store/slices/direct-ssh-', 'src/renderer/src/components/terminal-pane/remote-runtime-' ] for (const authority of sshSourceAuthorities) { @@ -217,10 +224,24 @@ describe('PR E2E gate contract', () => { ) } + // Why named files rather than prefixes: these seams are single modules, and a prefix + // here would route their unrelated neighbours. + for (const file of [ + 'src/main/runtime/public-ssh-state.ts', + 'src/renderer/src/startup/ssh-startup-reconnect.ts', + 'src/renderer/src/store/slices/ssh.ts' + ]) { + expect(selectPrE2eSpecs([file]), file).toContain( + 'tests/e2e/ssh-docker-reconnect-pane-restore.spec.ts' + ) + } + const mappedSpecs = [ 'tests/e2e/pty-input-write-queue-ssh.spec.ts', 'tests/e2e/ssh-cold-activation-restore.spec.ts', 'tests/e2e/ssh-docker-reconnect-pane-restore.spec.ts', + 'tests/e2e/ssh-port-forward-lifecycle.spec.ts', + 'tests/e2e/ssh-reconnect-tab-destruction.spec.ts', 'tests/e2e/ssh-startup-exec-readiness.spec.ts', 'tests/e2e/ssh-terminal-window-wake-stale-grid-repro.spec.ts' ] @@ -249,6 +270,140 @@ describe('PR E2E gate contract', () => { expect(changedInstall.run).toContain('openssh-client') }) + it('routes direct-SSH workspace and tab restore from its unnamed source seams', () => { + // Why by name: none of these carry "ssh", so the SSH authorities above never reach them + // — a closed-tab tombstone and a dropped default-tabs marker both shipped through it. + for (const file of [ + 'src/renderer/src/hooks/remote-workspace-session-merge.ts', + 'src/main/ipc/remote-workspace-snapshot-normalization.ts', + 'src/renderer/src/lib/worktree-initial-terminal-seeding.ts', + 'src/renderer/src/lib/worktree-default-terminal-tabs.ts', + 'src/shared/remote-workspace-session-projection.ts', + 'src/renderer/src/components/terminal/initial-terminal.ts' + ]) { + const specs = selectPrE2eSpecs([file]) + expect(specs, file).toContain('tests/e2e/ssh-cold-activation-restore.spec.ts') + expect(specs, file).toContain('tests/e2e/ssh-reconnect-tab-destruction.spec.ts') + } + + expect( + selectPrE2eSpecs(['src/renderer/src/hooks/remote-workspace-session-merge.test.ts']) + ).toEqual([]) + }) + + it('triggers the Docker-SSH lane from SSH source, not from a spec name', () => { + // The behavioural half of the invariant, and the part that actually matters: an SSH source + // edit is recognised as one, through the same routes that select the specs. + for (const file of [ + 'src/main/ssh/connection.ts', + 'src/relay/pty-handler.ts', + 'src/renderer/src/store/slices/direct-ssh-pane-retry-ledger.ts', + 'src/renderer/src/hooks/remote-workspace-session-merge.ts', + 'src/main/ipc/remote-workspace-snapshot-normalization.ts' + ]) { + expect(hasSshSourceChange([file]), file).toBe(true) + } + for (const file of [ + 'src/main/git/git-status.ts', + 'src/renderer/src/components/tab-bar/BrowserTab.tsx', + 'src/main/ssh/connection.test.ts' + ]) { + expect(hasSshSourceChange([file]), file).toBe(false) + } + + // Why: the signal must stay derived from the routes. A route id that no longer exists would + // silently narrow it to nothing. + for (const id of SSH_SOURCE_ROUTE_IDS) { + expect( + PR_E2E_SOURCE_ROUTES.map((route) => route.id), + id + ).toContain(id) + } + + // Why text and not structure: a job `if:` is only ever available as a string. The strongest + // available assertion is that the source signal is its own disjunct, so the lane no longer + // depends on a spec name surviving in a route's spec list. + const sshLaneCondition = e2eWorkflow.jobs['ssh-docker-watcher-isolation'].if + expect(sshLaneCondition).toContain("inputs.ssh_source_changed == 'true' ||") + + expect(e2eWorkflow.on.workflow_call.inputs.ssh_source_changed.type).toBe('string') + expect(prWorkflow.jobs['e2e-paths'].outputs.ssh_source_changed).toBe( + '${{ steps.filter.outputs.ssh_source_changed }}' + ) + expect(prWorkflow.jobs.e2e.with.ssh_source_changed).toBe( + '${{ needs.e2e-paths.outputs.ssh_source_changed }}' + ) + expect(filterStep.run).toContain('pr-e2e-source-routing.mjs --ssh-source') + expect(filterStep.run).toContain('ssh_source_changed=$SSH_SOURCE_CHANGED') + }) + + it('gives every Docker-gated SSH spec a lane that runs it', () => { + // Why this shape: the sharded lanes set no ORCA_E2E_SSH_DOCKER, so a Docker-gated spec + // that no runner names runs nowhere and still reports green — the silent skip this file + // exists to prevent. Asserting reachability rather than a literal keeps that true when + // the lanes move. + // Why these two are exempt: each needs something CI cannot give it, recorded in + // run-ssh-docker-e2e.mjs so the gap stays legible rather than looking like coverage. + const unreachableSpecs = new Set([ + 'tests/e2e/ssh-docker-relay-perf.spec.ts', + 'tests/e2e/ssh-codex-display-artifacts-repro.spec.ts', + 'tests/e2e/ssh-docker-bulk-open-freeze-repro.spec.ts' + ]) + // Why comments are stripped: this file's own runner lists the two exempt specs by name in a + // prose comment. A substring scan over raw text would count any spec merely *discussed* in a + // runner as claimed by it -- the silent skip this assertion exists to catch, re-entering + // through the documentation. + const stripComments = (text) => + text.replace(/\/\*[\s\S]*?\*\//g, '').replace(/^\s*\/\/.*$/gm, '') + const laneRunners = [ + 'run-ssh-docker-e2e.mjs', + 'run-ssh-docker-watcher-isolation-e2e.mjs', + 'run-ssh-docker-terminal-parking-e2e.mjs' + ].map((file) => stripComments(readFileSync(join(projectDir, 'config/scripts', file), 'utf8'))) + + // Why a comparison and not the bare name: preview and demo specs cite the flag in a + // "how to run me" comment without gating on it. Why a regex rather than one literal: an + // equally-valid spelling (double quotes, or a `!==` guard) would escape a fixed-string scan + // and the spec would silently leave the contract. + const dockerGateExpression = /ORCA_E2E_SSH_DOCKER\s*[!=]==\s*['"]1['"]/ + const dockerGatedSpecs = readdirSync(join(projectDir, 'tests/e2e')) + .filter((file) => file.endsWith('.spec.ts')) + .map((file) => `tests/e2e/${file}`) + .filter((spec) => dockerGateExpression.test(readFileSync(join(projectDir, spec), 'utf8'))) + expect(dockerGatedSpecs.length).toBeGreaterThan(0) + + const unclaimed = dockerGatedSpecs.filter( + (spec) => !unreachableSpecs.has(spec) && !laneRunners.some((runner) => runner.includes(spec)) + ) + expect( + unclaimed, + `Docker-gated specs claimed by no lane runner: ${unclaimed.join(', ')}` + ).toEqual([]) + + // Why: an exemption that outlives its spec would quietly excuse a real gap. + for (const spec of unreachableSpecs) { + expect(dockerGatedSpecs, spec).toContain(spec) + // Why also assert absence from every runner: `unreachableSpecs` short-circuits the + // unclaimed check above, so a spec could be documented as exempt while a runner still + // invokes it -- an exemption that reads as coverage removal but changes nothing, and a + // lane that stays red for a reason the file says it excluded. + for (const runner of laneRunners) { + expect(runner.includes(spec), `${spec} is exempt but still invoked by a lane runner`).toBe( + false + ) + } + } + + const laneStep = e2eWorkflow.jobs['ssh-docker-watcher-isolation'].steps.find( + (step) => step.name === 'Run remaining Docker SSH E2E' + ) + expect(laneStep.run).toContain('test:e2e:ssh-docker') + // Why: the added serial tests, several budgeting 4-10 minutes each, do not fit the old 35. + expect( + e2eWorkflow.jobs['ssh-docker-watcher-isolation']['timeout-minutes'] + ).toBeGreaterThanOrEqual(60) + }) + it('scopes the VM rollback oracle to the PR range and recipe schema authorities', () => { expect(rollbackStep.run).toContain('--merge-base "$BASE_SHA" "$HEAD_SHA"') expect(rollbackStep.run).toContain('src/shared/ephemeral-vm-recipes.ts') diff --git a/config/scripts/pr-e2e-source-routing.mjs b/config/scripts/pr-e2e-source-routing.mjs index 04f570f0094..108a4d5ab10 100644 --- a/config/scripts/pr-e2e-source-routing.mjs +++ b/config/scripts/pr-e2e-source-routing.mjs @@ -18,12 +18,32 @@ export const PR_E2E_SOURCE_ROUTES = [ 'tests/e2e/pty-input-write-queue-ssh.spec.ts', 'tests/e2e/ssh-cold-activation-restore.spec.ts', 'tests/e2e/ssh-docker-reconnect-pane-restore.spec.ts', + 'tests/e2e/ssh-port-forward-lifecycle.spec.ts', + 'tests/e2e/ssh-reconnect-tab-destruction.spec.ts', 'tests/e2e/ssh-startup-exec-readiness.spec.ts', 'tests/e2e/ssh-terminal-window-wake-stale-grid-repro.spec.ts' ], + // Why the store/startup/shared additions: the SSH-named authorities stop at the main + // process and the pane component, but the reconnect ledgers and retained-payload + // admission that decide whether a pane rebinds live in the renderer store. matches: (file) => isProductSource(file) && - /^(?:src\/main\/ssh\/|src\/main\/providers\/ssh-|src\/main\/ipc\/(?:ssh-|pty)|src\/relay\/|src\/renderer\/src\/components\/terminal-pane\/(?:pty-|ssh-|remote-runtime-|terminal-parked-pty))/.test( + /^(?:src\/main\/ssh\/|src\/main\/providers\/ssh-|src\/main\/ipc\/(?:ssh-|pty)|src\/main\/runtime\/(?:public-ssh-state|ssh-file-explorer-chunk-read)\.ts|src\/relay\/|src\/shared\/(?:ssh-|skill-ssh-relay-contract)|src\/renderer\/src\/startup\/(?:ssh-startup-reconnect|startup-ssh-connection-restore)\.ts|src\/renderer\/src\/store\/slices\/(?:ssh|direct-ssh-)|src\/renderer\/src\/components\/terminal-pane\/(?:pty-|ssh-|remote-runtime-|terminal-parked-pty))/.test( + file + ) + }, + { + // Why a sibling route rather than more paths on ssh-terminal-source: these modules carry + // no "ssh" in their names, and only the two restore specs gate them. Folding them in + // would run the whole SSH terminal list for a tab-tombstone edit. + id: 'ssh-workspace-session-restore', + specs: [ + 'tests/e2e/ssh-cold-activation-restore.spec.ts', + 'tests/e2e/ssh-reconnect-tab-destruction.spec.ts' + ], + matches: (file) => + isProductSource(file) && + /^(?:src\/main\/ipc\/remote-workspace|src\/shared\/remote-workspace-|src\/renderer\/src\/hooks\/remote-workspace-|src\/renderer\/src\/lib\/worktree-(?:initial-terminal-seeding|default-terminal-tabs)\.ts|src\/renderer\/src\/components\/terminal\/initial-terminal)/.test( file ) }, @@ -131,6 +151,18 @@ export function selectPrE2eSpecs(changedPaths, reportRoute = () => undefined) { return [...specs].sort((left, right) => left.localeCompare(right)) } +/** Routes whose authorities are SSH execution source, and so require the Docker-SSH lane. */ +export const SSH_SOURCE_ROUTE_IDS = ['ssh-terminal-source', 'ssh-workspace-session-restore'] + +// Why derive this from the routes instead of a second path list: the Docker-SSH lane used to +// trigger only because one route happened to list a startup-readiness spec, so pruning that +// spec would have silently retired the lane. Two lists that must agree is how that drifted. +export function hasSshSourceChange(changedPaths) { + return PR_E2E_SOURCE_ROUTES.filter((route) => SSH_SOURCE_ROUTE_IDS.includes(route.id)).some( + (route) => changedPaths.some(route.matches) + ) +} + if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { let input = '' process.stdin.setEncoding('utf8') @@ -138,6 +170,10 @@ if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) input += chunk } const changedPaths = input.split(/\r?\n/).filter(Boolean) - const specs = selectPrE2eSpecs(changedPaths, (message) => console.error(message)) - process.stdout.write(`${JSON.stringify(specs)}\n`) + if (process.argv.includes('--ssh-source')) { + process.stdout.write(`${hasSshSourceChange(changedPaths)}\n`) + } else { + const specs = selectPrE2eSpecs(changedPaths, (message) => console.error(message)) + process.stdout.write(`${JSON.stringify(specs)}\n`) + } } diff --git a/config/scripts/run-ssh-docker-e2e.mjs b/config/scripts/run-ssh-docker-e2e.mjs new file mode 100644 index 00000000000..8000c8a0c26 --- /dev/null +++ b/config/scripts/run-ssh-docker-e2e.mjs @@ -0,0 +1,93 @@ +import { spawnSync } from 'node:child_process' + +const rawExtraArgs = process.argv.slice(2) +const extraArgs = rawExtraArgs[0] === '--' ? rawExtraArgs.slice(1) : rawExtraArgs +const pnpm = process.platform === 'win32' ? 'pnpm.cmd' : 'pnpm' +const env = { + ...process.env, + ORCA_E2E_SSH_DOCKER: '1', + ORCA_E2E_WEB_CLIENT: '1' +} + +// Why: Node's CVE-2024-27980 hardening rejects .cmd spawns without shell on Windows. +const spawnOptions = { + stdio: 'inherit', + env, + shell: process.platform === 'win32' +} + +const runtime = spawnSync(pnpm, ['run', 'ensure:electron-runtime'], spawnOptions) + +if (runtime.status !== 0) { + process.exit(runtime.status ?? 1) +} + +// Why one explicit list: these specs self-skip without ORCA_E2E_SSH_DOCKER and no sharded lane +// sets it, so a spec in no runner runs nowhere. The gate contract proves every flag-reading +// spec is claimed here, by the watcher-isolation or parking runner, or by a listed exclusion. +// +// Deliberately absent, and therefore still covered by no CI trigger: +// ssh-docker-relay-perf.spec.ts — wall-clock latency thresholds; flaky budgets here would +// cost the lane its credibility. NOTE: a runner script test:e2e:ssh-docker-perf exists in +// package.json but NO workflow invokes it, so this spec currently runs in no CI lane at +// all. Recorded as a real gap, not as coverage living somewhere else. +// ssh-codex-display-artifacts-repro.spec.ts — installs a real remote codex binary that CI +// runners do not have (observed as `spawn codex ENOENT`). Runs in no CI lane at all. +// ssh-docker-bulk-open-freeze-repro.spec.ts — two reasons, both disqualifying: +// (a) it is a perf oracle, not a correctness one: SOFT_FREEZE_LAG_MS=2500 / +// HARD_FREEZE_LAG_MS=5000 measured by a renderer lag probe under a deliberate +// 5-pane output flood on a 420s budget. Same rule as ssh-docker-relay-perf above. +// (b) it is ROTTED: four call sites are out of date against terminal.ts's current +// helpers — execInTerminal gained a ptyId parameter and splitActiveTerminalPane +// gained a direction, so it cannot compile, let alone pass. Repairing it needs two +// semantic decisions (which ptyId to capture, which split direction) that change +// what the repro measures. Tracked in stablyai/orca#16764. +// +// Why both projects: ssh-port-forward-lifecycle is @headful, which the headless project +// grep-inverts away. +// +// Known gaps in SSH e2e coverage, recorded here because nothing else names them: +// - The job that runs this is still called `ssh-docker-watcher-isolation`, though watcher +// isolation is now one spec of many. Renaming it changes the GitHub check name and can +// break required-check config, so the name understates the job on purpose. +// - E2E does not gate merges: `verify.needs` in pr.yml omits `e2e` while the suite is red on +// main. Nothing in this lane blocks a PR yet. pr.yml's Require-successful-checks comment +// has the exact wiring to flip it, and the gate contract asserts the current state. +// - Five specs and one unit test are gated on env vars no workflow sets, so they run nowhere +// and are not Docker-gated, which puts them outside this file's contract: +// local-ssh-browser-routing (ORCA_E2E_LOCAL_SSH_BROWSER) +// ssh-client-hosted-browser-drop-reconnect (ORCA_E2E_SSH_CLIENT_HOSTED_BROWSER) +// nested-runtime-ssh-lifecycle, nested-runtime-ssh-routing (ORCA_E2E_NESTED_RUNTIME_SSH) +// ssh-localhost (ORCA_E2E_SSH_LOCALHOST) +// ssh-browser-network-execution-route.docker.unit.test.ts (ORCA_RUN_DOCKER_SSH_BROWSER_E2E) +// Runner scripts for the first four sit unused in package.json; no workflow calls them. +const result = spawnSync( + pnpm, + [ + 'exec', + 'playwright', + 'test', + 'tests/e2e/pty-input-write-queue-ssh.spec.ts', + 'tests/e2e/ssh-ai-vault-session-history.spec.ts', + 'tests/e2e/ssh-cold-activation-restore.spec.ts', + 'tests/e2e/ssh-docker-reconnect-pane-restore.spec.ts', + 'tests/e2e/ssh-external-image-preview.spec.ts', + 'tests/e2e/ssh-pi-compatible-agent-title.spec.ts', + 'tests/e2e/ssh-port-forward-lifecycle.spec.ts', + 'tests/e2e/ssh-reconnect-tab-destruction.spec.ts', + 'tests/e2e/ssh-restart-tab-accumulation.spec.ts', + 'tests/e2e/ssh-skill-installation.spec.ts', + 'tests/e2e/ssh-terminal-window-wake-stale-grid-repro.spec.ts', + '--config', + 'tests/playwright.config.ts', + '--project', + 'electron-headless', + '--project', + 'electron-headful', + '--workers=1', + ...extraArgs + ], + spawnOptions +) + +process.exit(result.status ?? 1) diff --git a/package.json b/package.json index 7b46b369406..c8e59299c70 100644 --- a/package.json +++ b/package.json @@ -111,6 +111,7 @@ "test:e2e:terminal-perf:html-report": "node config/scripts/generate-terminal-perf-html-report.mjs", "test:e2e:ssh-docker-perf": "node config/scripts/run-ssh-docker-perf-e2e.mjs", "test:e2e:ssh-docker-watcher-isolation": "node config/scripts/run-ssh-docker-watcher-isolation-e2e.mjs", + "test:e2e:ssh-docker": "node config/scripts/run-ssh-docker-e2e.mjs", "test:e2e:ssh-client-hosted-browser": "node config/scripts/run-ssh-client-hosted-browser-drop-reconnect-e2e.mjs", "test:e2e:local-ssh-browser": "node config/scripts/run-local-ssh-browser-routing-e2e.mjs", "test:e2e:ssh-docker-terminal-parking": "node config/scripts/run-ssh-docker-terminal-parking-e2e.mjs", diff --git a/src/main/ipc/remote-workspace-snapshot-normalization.test.ts b/src/main/ipc/remote-workspace-snapshot-normalization.test.ts new file mode 100644 index 00000000000..7f57b432813 --- /dev/null +++ b/src/main/ipc/remote-workspace-snapshot-normalization.test.ts @@ -0,0 +1,88 @@ +import { describe, expect, it } from 'vitest' + +import { + normalizeSnapshot, + remoteWorkspaceSessionMatchesSnapshot +} from './remote-workspace-snapshot-normalization' +import type { + RemoteWorkspaceSession, + RemoteWorkspaceSnapshot +} from '../../shared/remote-workspace-types' + +function session(overrides: Partial = {}): RemoteWorkspaceSession { + return { + activeWorktreePath: null, + activeTabId: null, + tabsByWorktreePath: {}, + terminalLayoutsByTabId: {}, + ...overrides + } +} + +function snapshotOf(session: RemoteWorkspaceSession): RemoteWorkspaceSnapshot { + return { namespace: 'ns', revision: 3, updatedAt: 1, schemaVersion: 1, session } +} + +/** + * Every declared field, each holding a value the normalizer must neither drop nor flatten. + * + * `Required` is the ratchet: normalizeRemoteSession rebuilds the session as an object literal, so a + * field added to the wire type and forgotten there is invisible — it round-trips through the + * projection, reaches the relay, and is stripped on the way back. Declaring the fixture Required + * makes the next such field a compile error here before it can become a silent data loss. + */ +const everyDeclaredField: Required = { + activeWorktreePath: '/repo', + activeTabId: 'tab-1', + tabsByWorktreePath: { '/repo': [{ id: 'tab-1', type: 'terminal', title: 'Shell' } as never] }, + terminalLayoutsByTabId: { 'tab-1': { kind: 'leaf', ptyId: 'pty-1' } as never }, + activeWorktreePathsOnShutdown: ['/repo'], + activeTabIdByWorktreePath: { '/repo': 'tab-1' }, + remoteSessionIdsByTabId: { 'tab-1': 'session-1' }, + lastVisitedAtByWorktreePath: { '/repo': 42 }, + defaultTerminalTabsAppliedByWorktreePath: { '/repo': true } +} + +describe('normalizeSnapshot: field coverage', () => { + it('copies every declared field of the wire session', () => { + const normalized = normalizeSnapshot(snapshotOf(everyDeclaredField), 'ns').session + + expect(Object.keys(normalized).sort()).toEqual(Object.keys(everyDeclaredField).sort()) + expect(normalized).toEqual(everyDeclaredField) + }) +}) + +describe('normalizeSnapshot: defaultTerminalTabsAppliedByWorktreePath', () => { + it('carries the marker through instead of dropping it', () => { + // The renderer exports this field and its importer reads it, but the normalizer every read + // passes through never copied it — so the flag reached the relay and could never come back. + const raw = snapshotOf(session({ defaultTerminalTabsAppliedByWorktreePath: { '/repo': true } })) + + expect(normalizeSnapshot(raw, 'ns').session.defaultTerminalTabsAppliedByWorktreePath).toEqual({ + '/repo': true + }) + }) + + it('treats an empty marker record as absent, like every other optional record', () => { + // The export always emits an object, so {} and absent both mean "no worktree has applied + // them". Collapsing them keeps a no-op export from looking like a change to the patch guard. + const raw = snapshotOf(session({ defaultTerminalTabsAppliedByWorktreePath: {} })) + + expect( + normalizeSnapshot(raw, 'ns').session.defaultTerminalTabsAppliedByWorktreePath + ).toBeUndefined() + }) +}) + +describe('remoteWorkspaceSessionMatchesSnapshot', () => { + it('sees a change that only sets the default-terminal-tabs marker', () => { + // patchRemoteWorkspaceSession short-circuits on this comparison, so a field it cannot see is a + // field that can never be written at all. + expect( + remoteWorkspaceSessionMatchesSnapshot( + snapshotOf(session()), + session({ defaultTerminalTabsAppliedByWorktreePath: { '/repo': true } }) + ) + ).toBe(false) + }) +}) diff --git a/src/main/ipc/remote-workspace-snapshot-normalization.ts b/src/main/ipc/remote-workspace-snapshot-normalization.ts index 1d24063ec4d..749d83cc066 100644 --- a/src/main/ipc/remote-workspace-snapshot-normalization.ts +++ b/src/main/ipc/remote-workspace-snapshot-normalization.ts @@ -62,6 +62,11 @@ function normalizeRemoteSession(raw: unknown): RemoteWorkspaceSession { ), lastVisitedAtByWorktreePath: normalizeOptionalRecord>( input.lastVisitedAtByWorktreePath + ), + // Dropping this made the field write-only: every read stripped it, and the patch guard below + // compares normalized sessions, so a change that only sets it looked like no change at all. + defaultTerminalTabsAppliedByWorktreePath: normalizeOptionalRecord>( + input.defaultTerminalTabsAppliedByWorktreePath ) } } diff --git a/src/main/ipc/worktree-create-lineage.test.ts b/src/main/ipc/worktree-create-lineage.test.ts new file mode 100644 index 00000000000..fa7e101ac3f --- /dev/null +++ b/src/main/ipc/worktree-create-lineage.test.ts @@ -0,0 +1,265 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { CreateWorktreeArgs } from '../../shared/worktree/create-types' +import type { WorktreeMeta } from '../../shared/worktree/meta-types' +import type { Worktree } from '../../shared/worktree/types' +import { folderWorkspaceKey, worktreeWorkspaceKey } from '../../shared/workspace-scope' +import { + assertAttachableParentWorkspace, + recordWorkspaceLineageForCreatedWorktree +} from './worktree-remote' + +const CHILD_ID = 'repo-1::/repos/child' +const PARENT_ID = 'repo-1::/repos/parent' +const CREATED_AT = 1_700_000_000_000 + +function createdWorktree(overrides: Partial = {}): Worktree { + return { + id: CHILD_ID, + instanceId: 'child-instance', + repoId: 'repo-1', + hostId: 'local', + projectId: 'project-1', + path: '/repos/child', + head: 'abc123', + branch: 'refs/heads/child', + isBare: false, + isMainWorktree: false, + displayName: 'child', + comment: '', + linkedIssue: null, + linkedPR: null, + linkedLinearIssue: null, + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 0, + lastActivityAt: 0, + ...overrides + } +} + +function parentMeta(overrides: Partial = {}): WorktreeMeta { + return { + instanceId: 'parent-instance', + hostId: 'local', + projectId: 'project-1', + displayName: 'parent', + comment: '', + linkedIssue: null, + linkedPR: null, + linkedLinearIssue: null, + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 0, + lastActivityAt: 0, + ...overrides + } +} + +function createStore(options: { + metaById?: Record + folderWorkspaceIds?: string[] +}) { + const metaById = options.metaById ?? {} + const folderWorkspaceIds = new Set(options.folderWorkspaceIds ?? []) + return { + getWorktreeMeta: vi.fn((id: string) => metaById[id]), + getFolderWorkspace: vi.fn((id: string) => + folderWorkspaceIds.has(id) ? { id, path: `/folders/${id}` } : undefined + ), + setWorktreeLineage: vi.fn((_id: string, lineage: unknown) => lineage), + setWorkspaceLineage: vi.fn((lineage: unknown) => lineage) + } +} + +function record( + store: ReturnType, + args: Partial, + worktree = createdWorktree() +) { + return recordWorkspaceLineageForCreatedWorktree( + store as never, + args as CreateWorktreeArgs, + worktree, + CREATED_AT + ) +} + +describe('recordWorkspaceLineageForCreatedWorktree', () => { + beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + it('writes both lineage rows for a worktree parent inside the same repo/host/project', () => { + const store = createStore({ metaById: { [PARENT_ID]: parentMeta() } }) + + const result = record(store, { parentWorkspace: worktreeWorkspaceKey(PARENT_ID) }) + + expect(store.setWorktreeLineage).toHaveBeenCalledWith(CHILD_ID, { + worktreeId: CHILD_ID, + worktreeInstanceId: 'child-instance', + parentWorktreeId: PARENT_ID, + parentWorktreeInstanceId: 'parent-instance', + origin: 'manual', + capture: { source: 'manual-action', confidence: 'explicit' }, + createdAt: CREATED_AT + }) + expect(result.lineage).toMatchObject({ + parentWorktreeId: PARENT_ID, + parentWorktreeInstanceId: 'parent-instance', + origin: 'manual', + capture: { source: 'manual-action', confidence: 'explicit' } + }) + expect(result.workspaceLineage).toEqual({ + childWorkspaceKey: worktreeWorkspaceKey(CHILD_ID), + childInstanceId: 'child-instance', + parentWorkspaceKey: worktreeWorkspaceKey(PARENT_ID), + parentInstanceId: 'parent-instance', + origin: 'manual', + capture: { source: 'manual-action', confidence: 'explicit' }, + createdAt: CREATED_AT + }) + }) + + it('skips both lineage records when the parent belongs to a different repo', () => { + const foreignParentId = 'repo-2::/repos/parent' + const store = createStore({ metaById: { [foreignParentId]: parentMeta() } }) + + const result = record(store, { parentWorkspace: worktreeWorkspaceKey(foreignParentId) }) + + expect(store.setWorktreeLineage).not.toHaveBeenCalled() + expect(result.lineage).toBeNull() + expect(store.setWorkspaceLineage).not.toHaveBeenCalled() + expect(result.workspaceLineage).toBeNull() + }) + + it.each([ + ['host', parentMeta({ hostId: 'ssh:other-host' })], + ['project', parentMeta({ projectId: 'project-2' })] + ])( + 'skips both lineage records when the parent %s conflicts, so no cross-host row is persisted', + (_label, meta) => { + const store = createStore({ metaById: { [PARENT_ID]: meta } }) + + const result = record(store, { parentWorkspace: worktreeWorkspaceKey(PARENT_ID) }) + + expect(store.setWorktreeLineage).not.toHaveBeenCalled() + expect(result.lineage).toBeNull() + // A persisted cross-host row makes filterLineageForHost return null for the entire host. + expect(store.setWorkspaceLineage).not.toHaveBeenCalled() + expect(result.workspaceLineage).toBeNull() + } + ) + + it('skips worktree lineage when the parent has no instance identity', () => { + const store = createStore({ metaById: { [PARENT_ID]: parentMeta({ instanceId: undefined }) } }) + + const result = record(store, { parentWorkspace: worktreeWorkspaceKey(PARENT_ID) }) + + expect(store.setWorktreeLineage).not.toHaveBeenCalled() + expect(result.lineage).toBeNull() + expect(result.workspaceLineage).toMatchObject({ + parentWorkspaceKey: worktreeWorkspaceKey(PARENT_ID), + parentInstanceId: null + }) + }) + + it('records only workspace lineage for a folder-workspace parent', () => { + const store = createStore({ folderWorkspaceIds: ['folder-1'] }) + + const result = record(store, { parentWorkspace: folderWorkspaceKey('folder-1') }) + + expect(store.setWorktreeLineage).not.toHaveBeenCalled() + expect(result.lineage).toBeNull() + expect(result.workspaceLineage).toMatchObject({ + parentWorkspaceKey: folderWorkspaceKey('folder-1'), + parentInstanceId: null, + capture: { source: 'active-workspace', confidence: 'explicit' } + }) + }) + + it('records nothing without a parent workspace', () => { + const store = createStore({}) + + const result = record(store, {}) + + expect(store.setWorktreeLineage).not.toHaveBeenCalled() + expect(store.setWorkspaceLineage).not.toHaveBeenCalled() + expect(result).toEqual({ lineage: null, workspaceLineage: null }) + }) + + it('records nothing when the parent workspace is the created worktree itself', () => { + const store = createStore({ metaById: { [CHILD_ID]: parentMeta() } }) + + const result = record(store, { parentWorkspace: worktreeWorkspaceKey(CHILD_ID) }) + + expect(store.setWorktreeLineage).not.toHaveBeenCalled() + expect(store.setWorkspaceLineage).not.toHaveBeenCalled() + expect(result).toEqual({ lineage: null, workspaceLineage: null }) + }) + + it('records nothing when the parent worktree disappeared before the write', () => { + const store = createStore({}) + + const result = record(store, { parentWorkspace: worktreeWorkspaceKey(PARENT_ID) }) + + expect(store.setWorkspaceLineage).not.toHaveBeenCalled() + expect(result).toEqual({ lineage: null, workspaceLineage: null }) + }) +}) + +describe('assertAttachableParentWorkspace', () => { + const childKey = worktreeWorkspaceKey(CHILD_ID) + + beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + function assertParent( + store: ReturnType, + parentWorkspace: CreateWorktreeArgs['parentWorkspace'] + ) { + assertAttachableParentWorkspace(store as never, parentWorkspace, childKey) + } + + // Why: the pick can go stale between the composer and the create; nesting is decoration, not the job. + it.each([ + ['worktree', () => worktreeWorkspaceKey(PARENT_ID)], + ['folder', () => folderWorkspaceKey('folder-gone')] + ])('degrades instead of failing when the %s parent disappeared', (_label, key) => { + const store = createStore({}) + + expect(() => assertParent(store, key())).not.toThrow() + expect(console.warn).toHaveBeenCalled() + }) + + it('accepts a parent that still exists', () => { + const store = createStore({ metaById: { [PARENT_ID]: parentMeta() } }) + + expect(() => assertParent(store, worktreeWorkspaceKey(PARENT_ID))).not.toThrow() + expect(console.warn).not.toHaveBeenCalled() + }) + + it('rejects a self-referential parent', () => { + const store = createStore({ metaById: { [CHILD_ID]: parentMeta() } }) + + expect(() => assertParent(store, childKey)).toThrow(/cannot be attached to itself/) + }) + + it('rejects a malformed parent key', () => { + const store = createStore({}) + + expect(() => assertParent(store, 'not-a-workspace-key' as never)).toThrow( + /Invalid parent workspace/ + ) + }) +}) diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index c20899ea9cf..977c29d8792 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -91,6 +91,7 @@ import { findCreatedWorktree } from './created-worktree-reconciliation' import type { BranchPrefixSettings } from '../../shared/branch-prefix' import { getRepoIdFromWorktreeId } from '../../shared/worktree/id' import { parseWorkspaceKey, worktreeWorkspaceKey } from '../../shared/workspace-scope' +import { sharesWorktreeLineageBoundary } from '../../shared/resolved-worktree-lineage' import { cleanupUnusedWorktreePushTargetRemoteWithExec, sameGitHubRemoteUrl, @@ -197,7 +198,11 @@ function getSetupRunnerCommandPlatformForLaunch( return getSetupRunnerCommandPlatformForPath(setup?.runnerScriptPath ?? '', fallbackPlatform) } -function validateWorkspaceLineageParentBeforeCreate( +/** Why not throw on a missing parent: nesting is optional decoration, and the pick can go stale + * between the composer and the create. A malformed or self-referential key is a bad request and + * still fails; a parent that simply disappeared degrades to an unattached workspace, matching the + * runtime path and `recordWorkspaceLineageForCreatedWorktree`. */ +export function assertAttachableParentWorkspace( store: Store, parentWorkspace: CreateWorktreeArgs['parentWorkspace'], childWorkspaceKey: ReturnType @@ -213,51 +218,120 @@ function validateWorkspaceLineageParentBeforeCreate( throw new Error(`Invalid parent workspace: ${parentWorkspace}`) } if (parentScope.type === 'folder' && !store.getFolderWorkspace(parentScope.folderWorkspaceId)) { - throw new Error(`Parent folder workspace not found: ${parentWorkspace}`) + console.warn(`[worktree-create] parent folder workspace not found: ${parentWorkspace}`) + return } if (parentScope.type === 'worktree' && !store.getWorktreeMeta(parentScope.worktreeId)) { - throw new Error(`Parent worktree workspace not found: ${parentWorkspace}`) + console.warn(`[worktree-create] parent worktree workspace not found: ${parentWorkspace}`) } } -function recordWorkspaceLineageForCreatedWorktree( +type CreatedWorktreeLineageRecords = { + lineage: CreateWorktreeResult['lineage'] + workspaceLineage: CreateWorktreeResult['workspaceLineage'] +} + +const NO_CREATED_WORKTREE_LINEAGE: CreatedWorktreeLineageRecords = { + lineage: null, + workspaceLineage: null +} + +/** Mirrors the projection's edge rule so we never persist a row the sidebar would silently drop. + * WorktreeMeta has no repoId, so the parent's comes from its `::` id. */ +function createdWorktreeSharesParentLineageBoundary( + worktree: Worktree, + parentWorktreeId: string, + parentMeta: WorktreeMeta +): boolean { + return sharesWorktreeLineageBoundary(worktree, { + repoId: getRepoIdFromWorktreeId(parentWorktreeId), + hostId: parentMeta.hostId, + projectId: parentMeta.projectId + }) +} + +export function recordWorkspaceLineageForCreatedWorktree( store: Store, args: CreateWorktreeArgs, worktree: Worktree, createdAt: number -): CreateWorktreeResult['workspaceLineage'] { +): CreatedWorktreeLineageRecords { if (!args.parentWorkspace || !worktree.instanceId) { - return null + return NO_CREATED_WORKTREE_LINEAGE } const childWorkspaceKey = worktreeWorkspaceKey(worktree.id) if (args.parentWorkspace === childWorkspaceKey) { console.warn(`[worktree-create] refusing to attach ${worktree.id} to itself`) - return null + return NO_CREATED_WORKTREE_LINEAGE } const parentScope = parseWorkspaceKey(args.parentWorkspace) if (!parentScope) { console.warn(`[worktree-create] ignoring invalid parent workspace ${args.parentWorkspace}`) - return null + return NO_CREATED_WORKTREE_LINEAGE } if (parentScope.type === 'folder' && !store.getFolderWorkspace(parentScope.folderWorkspaceId)) { console.warn(`[worktree-create] parent folder workspace disappeared: ${args.parentWorkspace}`) - return null + return NO_CREATED_WORKTREE_LINEAGE } const parentWorktreeMeta = parentScope.type === 'worktree' ? store.getWorktreeMeta(parentScope.worktreeId) : null if (parentScope.type === 'worktree' && !parentWorktreeMeta) { console.warn(`[worktree-create] parent worktree workspace disappeared: ${args.parentWorkspace}`) - return null + return NO_CREATED_WORKTREE_LINEAGE } - return store.setWorkspaceLineage({ + + // Why: only a worktree parent produces sidebar nesting; a folder parent has no WorktreeLineage row. + let lineage: CreateWorktreeResult['lineage'] = null + let parentOutsideLineageBoundary = false + if (parentScope.type === 'worktree' && parentWorktreeMeta) { + if (!parentWorktreeMeta.instanceId) { + console.warn( + `[worktree-create] parent ${parentScope.worktreeId} has no instance identity; skipping lineage` + ) + } else if ( + !createdWorktreeSharesParentLineageBoundary( + worktree, + parentScope.worktreeId, + parentWorktreeMeta + ) + ) { + parentOutsideLineageBoundary = true + console.warn( + `[worktree-create] parent ${parentScope.worktreeId} is outside ${worktree.id}'s repo/host/project boundary; skipping lineage` + ) + } else { + lineage = store.setWorktreeLineage(worktree.id, { + worktreeId: worktree.id, + worktreeInstanceId: worktree.instanceId, + parentWorktreeId: parentScope.worktreeId, + parentWorktreeInstanceId: parentWorktreeMeta.instanceId, + origin: 'manual', + capture: { source: 'manual-action', confidence: 'explicit' }, + createdAt + }) + } + } + + // Why: persisting a workspace row for an out-of-boundary worktree parent poisons the whole host — + // `filterLineageForHost` returns null for any owned row whose child and parent hosts differ, so + // every nesting on that host stops hydrating, and the row survives restarts. + if (parentOutsideLineageBoundary) { + return NO_CREATED_WORKTREE_LINEAGE + } + + const workspaceLineage = store.setWorkspaceLineage({ childWorkspaceKey, childInstanceId: worktree.instanceId, parentWorkspaceKey: args.parentWorkspace, parentInstanceId: parentWorktreeMeta?.instanceId ?? null, origin: 'manual', - capture: { source: 'active-workspace', confidence: 'explicit' }, + capture: { + source: parentScope.type === 'worktree' ? 'manual-action' : 'active-workspace', + confidence: 'explicit' + }, createdAt }) + return { lineage, workspaceLineage } } function countNonEmptyGitOutputLines(output: string): number { @@ -1677,7 +1751,7 @@ export async function createRemoteWorktree( ) } - validateWorkspaceLineageParentBeforeCreate( + assertAttachableParentWorkspace( store, args.parentWorkspace, worktreeWorkspaceKey(`${repo.id}::${remotePath}`) @@ -1915,7 +1989,12 @@ export async function createRemoteWorktree( const meta = store.setWorktreeMeta(worktreeId, metaUpdates) return { worktree: mergeWorktree(repo.id, created, meta) } }) - const workspaceLineage = recordWorkspaceLineageForCreatedWorktree(store, args, worktree, now) + const { lineage: worktreeLineage, workspaceLineage } = recordWorkspaceLineageForCreatedWorktree( + store, + args, + worktree, + now + ) // Why: shared/symlink paths, `orca.yaml` shared directories, and `.worktreeinclude` copies are local-only; remote (SSH) support needs a new relay method + auth surface, so all are skipped here. @@ -1962,7 +2041,14 @@ export async function createRemoteWorktree( notifyWorktreesChanged(mainWindow, repo.id) return { - worktree: { ...worktree, workspaceLineage }, + worktree: { + ...worktree, + workspaceLineage, + ...(worktreeLineage + ? { lineage: worktreeLineage, parentWorktreeId: worktreeLineage.parentWorktreeId } + : {}) + }, + ...(worktreeLineage ? { lineage: worktreeLineage } : {}), ...(workspaceLineage ? { workspaceLineage } : {}), ...(setup ? { setup } : {}), ...(defaultTabs ? { defaultTabs } : {}), @@ -2308,7 +2394,7 @@ export async function createLocalWorktree( ) } - validateWorkspaceLineageParentBeforeCreate( + assertAttachableParentWorkspace( store, args.parentWorkspace, worktreeWorkspaceKey(`${repo.id}::${worktreePath}`) @@ -2555,7 +2641,12 @@ export async function createLocalWorktree( const meta = store.setWorktreeMeta(worktreeId, metaUpdates) return { worktree: mergeWorktree(repo.id, created, meta) } }) - const workspaceLineage = recordWorkspaceLineageForCreatedWorktree(store, args, worktree, now) + const { lineage: worktreeLineage, workspaceLineage } = recordWorkspaceLineageForCreatedWorktree( + store, + args, + worktree, + now + ) // Why: reuse the roots creation already paid for via `git worktree list` so later IPC doesn't lazily rescan and trip macOS privacy prompts. registerWorktreeRootsForRepo(store, repo.id, [ repo.path, @@ -2658,7 +2749,14 @@ export async function createLocalWorktree( notifyWorktreesChanged(mainWindow, repo.id) return { - worktree: { ...worktree, workspaceLineage }, + worktree: { + ...worktree, + workspaceLineage, + ...(worktreeLineage + ? { lineage: worktreeLineage, parentWorktreeId: worktreeLineage.parentWorktreeId } + : {}) + }, + ...(worktreeLineage ? { lineage: worktreeLineage } : {}), ...(workspaceLineage ? { workspaceLineage } : {}), ...(stagedStartup.activationSetup ? { setup: stagedStartup.activationSetup } diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 7747797de4f..4f02816f1c5 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -6469,6 +6469,52 @@ describe('OrcaRuntimeService', () => { } }) + // Why: the desktop composer sends `parentWorkspace` too, and a bare selector defaults to CLI + // provenance — the same user action must not carry different cleanup semantics per host. + it('records an app-selected parent workspace as a manual action', async () => { + vi.mocked(addWorktree).mockClear() + const created = { + path: '/tmp/workspaces/manual-child', + head: 'def', + branch: 'refs/heads/manual-child', + isBare: false, + isMainWorktree: false + } + const childId = `${TEST_REPO_ID}::${created.path}` + const metaById: Record = {} + const runtimeStore = { + ...createFolderWorkspaceRuntimeStore(), + getAllWorktreeMeta: () => metaById, + getWorktreeMeta: (worktreeId: string) => metaById[worktreeId], + setWorktreeMeta: (worktreeId: string, meta: Partial) => { + metaById[worktreeId] = { ...(metaById[worktreeId] ?? makeWorktreeMeta()), ...meta } + return metaById[worktreeId] + }, + setWorkspaceLineage: vi.fn((lineage: WorkspaceLineage) => lineage) + } + computeWorktreePathMock.mockReturnValue(created.path) + ensurePathWithinWorkspaceMock.mockImplementation((pathValue: string) => pathValue) + vi.mocked(listWorktrees).mockResolvedValueOnce([created]) + const runtime = new OrcaRuntimeService(runtimeStore as never) + + const result = await runtime.createManagedWorktree({ + repoSelector: TEST_REPO_ID, + name: 'manual-child', + baseBranch: 'origin/main', + lineage: { + parentWorkspace: TEST_FOLDER_WORKSPACE_KEY, + parentWorkspaceOrigin: 'manual' + } + }) + + expect(result.workspaceLineage).toMatchObject({ + childWorkspaceKey: `worktree:${childId}`, + parentWorkspaceKey: TEST_FOLDER_WORKSPACE_KEY, + origin: 'manual', + capture: { source: 'active-workspace', confidence: 'explicit' } + }) + }) + it('records folder workspace lineage inferred from environment context', async () => { vi.mocked(addWorktree).mockClear() const created = { diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index c81ca03ed04..c800b861a42 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -2902,6 +2902,8 @@ type ResolvedTerminalWorkspaceLaunchTarget = { type WorktreeLineageInput = { parentWorkspace?: string + /** Set by in-app parent pickers so the row is not recorded as a CLI flag. */ + parentWorkspaceOrigin?: 'manual' envParentWorkspace?: string parentWorktree?: string cwdParentWorktree?: string @@ -32581,11 +32583,20 @@ export class OrcaRuntimeService { if (input.parentWorkspace) { try { + const parent = await this.resolveWorkspaceParentSelector(input.parentWorkspace) + // Why: a picker in the app must record the same provenance as a local create, or the same + // user action would carry different cleanup semantics depending on where the repo lives. return { kind: 'lineage', - parent: await this.resolveWorkspaceParentSelector(input.parentWorkspace), - origin: 'cli', - capture: { source: 'explicit-cli-flag', confidence: 'explicit' } + parent, + origin: input.parentWorkspaceOrigin === 'manual' ? 'manual' : 'cli', + capture: + input.parentWorkspaceOrigin === 'manual' + ? { + source: parent.type === 'worktree' ? 'manual-action' : 'active-workspace', + confidence: 'explicit' + } + : { source: 'explicit-cli-flag', confidence: 'explicit' } } } catch (err) { throw new RuntimeLineageError( diff --git a/src/main/runtime/rpc/methods/worktree-create-args.test.ts b/src/main/runtime/rpc/methods/worktree-create-args.test.ts index fa2b773560b..5a55d54460f 100644 --- a/src/main/runtime/rpc/methods/worktree-create-args.test.ts +++ b/src/main/runtime/rpc/methods/worktree-create-args.test.ts @@ -26,4 +26,19 @@ describe('buildManagedWorktreeCreateArgs', () => { nameWasGenerated: true }) }) + + it('carries the parent-pick provenance only when the client marked it manual', () => { + // Why: older clients never send it, and those creates really are CLI-flag equivalents. + expect( + build({ repo: 'id:repo-1', name: 'child', parentWorkspace: 'folder:f1' }).lineage + ).not.toHaveProperty('parentWorkspaceOrigin') + expect( + build({ + repo: 'id:repo-1', + name: 'child', + parentWorkspace: 'folder:f1', + parentWorkspaceOrigin: 'manual' + }).lineage + ).toMatchObject({ parentWorkspaceOrigin: 'manual' }) + }) }) diff --git a/src/main/runtime/rpc/methods/worktree-create-args.ts b/src/main/runtime/rpc/methods/worktree-create-args.ts index 2b86b12ee05..ac674f08d61 100644 --- a/src/main/runtime/rpc/methods/worktree-create-args.ts +++ b/src/main/runtime/rpc/methods/worktree-create-args.ts @@ -80,6 +80,7 @@ export function buildManagedWorktreeCreateArgs( startupDraft: params.startupDraft, lineage: { parentWorkspace: params.parentWorkspace, + ...(params.parentWorkspaceOrigin ? { parentWorkspaceOrigin: 'manual' as const } : {}), envParentWorkspace: params.envParentWorkspace, parentWorktree: params.parentWorktree, ...(params.cwdParentWorktree ? { cwdParentWorktree: params.cwdParentWorktree } : {}), diff --git a/src/main/runtime/rpc/methods/worktree-create-schemas.ts b/src/main/runtime/rpc/methods/worktree-create-schemas.ts index 507f44b325b..e296b9c3ace 100644 --- a/src/main/runtime/rpc/methods/worktree-create-schemas.ts +++ b/src/main/runtime/rpc/methods/worktree-create-schemas.ts @@ -75,6 +75,9 @@ export const WorktreeCreate = z // startup and defaultTabs. Pass an explicit target to opt into an all-surface reveal. navigation: z.enum(RUNTIME_NAVIGATION_TARGETS).optional(), parentWorkspace: OptionalString, + // Why: an app-selected parent is a manual action, not the CLI's `--parent-workspace` flag. + // Absent keeps the CLI provenance older clients rely on. + parentWorkspaceOrigin: z.literal('manual').optional(), envParentWorkspace: OptionalString, parentWorktree: OptionalString, cwdParentWorktree: OptionalString, diff --git a/src/renderer/src/assets/main.css b/src/renderer/src/assets/main.css index 9d75836cd11..cd556d6f486 100644 --- a/src/renderer/src/assets/main.css +++ b/src/renderer/src/assets/main.css @@ -1145,9 +1145,11 @@ html.native-shell .app-layout { /* ── Jump palette (Cmd+J) ─────────────────────────────── */ -/* Why the data-slot qualifier: CommandItem also sets data-[selected=true]:bg-accent; - this selector must beat that utility so light-mode selection is actually visible. */ -[data-slot='command-item'].jump-palette-item[data-selected='true'] { +/* Why the extra qualifier: CommandItem also sets data-[selected=true]:bg-accent; + this selector must beat that utility so light-mode selection is actually visible. + The role='option' arm covers virtualized rows that render outside cmdk. */ +[data-slot='command-item'].jump-palette-item[data-selected='true'], +[role='option'].jump-palette-item[data-selected='true'] { /* Why: flat --accent (#f5f5f5) on the palette surface (#fff) is nearly invisible in light mode — the keyboard cursor has to read as a solid selected row. Mix foreground into background and draw an inset ring; expose the surface @@ -1159,7 +1161,8 @@ html.native-shell .app-layout { color: var(--foreground); } -.dark [data-slot='command-item'].jump-palette-item[data-selected='true'] { +.dark [data-slot='command-item'].jump-palette-item[data-selected='true'], +.dark [role='option'].jump-palette-item[data-selected='true'] { /* Why accent, not the light-mode mix: --accent (#404040) already reads clearly on dark surfaces; a low-%% foreground mix was a visible step down from the old row. */ --jump-palette-selection-surface: var(--accent); diff --git a/src/renderer/src/components/NewWorkspaceComposerCard.set-location.test.tsx b/src/renderer/src/components/NewWorkspaceComposerCard.set-location.test.tsx index fe68dc4f53c..9cdea1d7d54 100644 --- a/src/renderer/src/components/NewWorkspaceComposerCard.set-location.test.tsx +++ b/src/renderer/src/components/NewWorkspaceComposerCard.set-location.test.tsx @@ -171,6 +171,8 @@ function renderCard( onOpenAgentSettings={() => {}} advancedOpen={false} onToggleAdvanced={() => {}} + parentWorktreeId={null} + onParentWorktreeIdChange={() => {}} createDisabled={false} projectError={null} creating={false} diff --git a/src/renderer/src/components/NewWorkspaceComposerCard.test.tsx b/src/renderer/src/components/NewWorkspaceComposerCard.test.tsx index 9775cd06dae..8206354f916 100644 --- a/src/renderer/src/components/NewWorkspaceComposerCard.test.tsx +++ b/src/renderer/src/components/NewWorkspaceComposerCard.test.tsx @@ -230,6 +230,8 @@ function renderCard( onReuseSelectedBranchChange={() => {}} branchNameOverride="" onBranchNameOverrideChange={() => {}} + parentWorktreeId={null} + onParentWorktreeIdChange={() => {}} forkPushWarning={null} detectedAgentIds={null} onOpenAgentSettings={() => {}} @@ -306,6 +308,11 @@ function findRunTargetItem(label: string): HTMLElement | undefined { let current: { container: HTMLDivElement; root: Root } | null = null +function unmountCurrent(): void { + act(() => current?.root.unmount()) + current?.container.remove() +} + describe('NewWorkspaceComposerCard folder task source mode', () => { beforeEach(() => { ;(window as unknown as { api: unknown }).api = { @@ -333,8 +340,7 @@ describe('NewWorkspaceComposerCard folder task source mode', () => { }) afterEach(() => { - act(() => current?.root.unmount()) - current?.container.remove() + unmountCurrent() current = null vi.clearAllMocks() }) @@ -395,8 +401,7 @@ describe('NewWorkspaceComposerCard folder task source mode', () => { ) expect(collapsedReuse).toBeTruthy() - act(() => current?.root.unmount()) - current?.container.remove() + unmountCurrent() current = renderCard({ canReuseSelectedBranch: true, reuseSelectedBranch: true }) const reuseLabel = [...current.container.querySelectorAll('label')].find((label) => @@ -430,8 +435,7 @@ describe('NewWorkspaceComposerCard folder task source mode', () => { clickReuseCheckbox() expect(offChanges).toEqual([false]) - act(() => current?.root.unmount()) - current?.container.remove() + unmountCurrent() // Unchecked -> checked (opting into reuse — the action that pins the branch). const onChanges: boolean[] = [] @@ -458,8 +462,7 @@ describe('NewWorkspaceComposerCard folder task source mode', () => { 'Wait for setup to complete before starting agent' ) - act(() => current?.root.unmount()) - current?.container.remove() + unmountCurrent() current = renderCard({ advancedOpen: true, @@ -579,6 +582,19 @@ describe('NewWorkspaceComposerCard folder task source mode', () => { expect(findInputByLabel(current.container, 'Branch name')).toBeTruthy() }) + it('places the parent workspace picker immediately after the branch name field', () => { + current = renderCard({ + advancedOpen: true, + branchesEnabled: true + }) + + const nextField = findInputByLabel(current.container, 'Branch name')?.closest( + 'div.space-y-1' + )?.nextElementSibling + expect(nextField?.textContent).toMatch(/Parent worktree(?!.*Note)/) + expect(nextField?.nextElementSibling?.textContent).toContain('Note') + }) + it('does not disable folder workspace creation when only source lookup needs SSH', () => { current = renderCard({ eligibleRepos: [ @@ -886,8 +902,7 @@ describe('NewWorkspaceComposerCard note sizing', () => { // Sizing is layout-driven (field-sizing) rather than a JS measure pass, and happy-dom // has no layout engine, so these assert the class contract that produces the growth. afterEach(() => { - act(() => current?.root.unmount()) - current?.container.remove() + unmountCurrent() current = null }) diff --git a/src/renderer/src/components/NewWorkspaceComposerCard.tsx b/src/renderer/src/components/NewWorkspaceComposerCard.tsx index b0a711cb0eb..c6cca5192ea 100644 --- a/src/renderer/src/components/NewWorkspaceComposerCard.tsx +++ b/src/renderer/src/components/NewWorkspaceComposerCard.tsx @@ -55,6 +55,11 @@ export default function NewWorkspaceComposerCard( setupConfig, setupControlsEnabled = true, selectedProjectId = null, + parentWorktreeId, + onParentWorktreeIdChange, + selectedRepoExecutionHostId, + selectedRepoProjectId, + activeFolderWorkspaceId, onAddProjectOverride, onNestedDialogOpenChange } = props @@ -302,6 +307,11 @@ export default function NewWorkspaceComposerCard( setupRunButtonLabel={setupRunButtonLabel} setupSkipButtonLabel={setupSkipButtonLabel} showSetupAgentStartupPolicy={showSetupAgentStartupPolicy} + parentWorktreeId={parentWorktreeId} + onParentWorktreeIdChange={onParentWorktreeIdChange} + selectedRepoExecutionHostId={selectedRepoExecutionHostId} + selectedRepoProjectId={selectedRepoProjectId} + activeFolderWorkspaceId={activeFolderWorkspaceId} /> + repos: Repo[] + projectHostSetups: never[] + worktreeLineageById: Record + workspaceLineageByChildKey: Record +} + +const storeState = vi.hoisted( + () => + ({ + worktreesByRepo: {}, + repos: [], + projectHostSetups: [], + worktreeLineageById: {}, + workspaceLineageByChildKey: {} + }) as FakeStoreState +) + +const popoverMock = vi.hoisted(() => ({ + open: false, + onOpenChange: undefined as ((open: boolean) => void) | undefined +})) + +const indexSpies = vi.hoisted(() => ({ + getIndexedAllWorktrees: vi.fn(), + getIndexedWorktreeMap: vi.fn(), + getIndexedWorktreeById: vi.fn() +})) + +vi.mock('@/i18n/i18n', () => ({ + translate: (_key: string, fallback: string) => fallback +})) + +vi.mock('react-i18next', () => ({ + useTranslation: () => ({ t: (_key: string, fallback: string) => fallback }) +})) + +vi.mock('@/store', () => ({ + useAppStore: (selector: (state: FakeStoreState) => unknown) => selector(storeState) +})) + +// Spies wrap the real projections, so exclusion rules stay under test while call +// counts prove when enumeration happens. +vi.mock('@/store/worktree-repo-index', async (importOriginal) => { + const actual = await importOriginal() + indexSpies.getIndexedAllWorktrees.mockImplementation(actual.getIndexedAllWorktrees) + indexSpies.getIndexedWorktreeMap.mockImplementation(actual.getIndexedWorktreeMap) + indexSpies.getIndexedWorktreeById.mockImplementation(actual.getIndexedWorktreeById) + return { ...actual, ...indexSpies } +}) + +vi.mock('@/components/ui/popover', async () => { + const { cloneElement } = await import('react') + return { + Popover: ({ + children, + open, + onOpenChange + }: { + children: React.ReactNode + open?: boolean + onOpenChange?: (open: boolean) => void + }) => { + popoverMock.open = open ?? false + popoverMock.onOpenChange = onOpenChange + return
{children}
+ }, + // Radix `asChild` renders the child itself and owns its click; mirror that. + PopoverTrigger: ({ children }: { children: React.ReactElement }) => + cloneElement(children as React.ReactElement<{ onClick?: () => void }>, { + onClick: () => popoverMock.onOpenChange?.(!popoverMock.open) + }), + // Radix mounts content only while open — the property the lazy list relies on. + PopoverContent: ({ children }: { children: React.ReactNode }) => + popoverMock.open ?
{children}
: null + } +}) + +vi.mock('@/components/ui/command', () => ({ + Command: ({ children, className }: { children: React.ReactNode; className?: string }) => ( +
{children}
+ ), + CommandInput: ({ + ref, + value, + placeholder, + onValueChange, + onKeyDown, + onKeyUp, + onCompositionStart, + onCompositionEnd + }: { + ref?: React.Ref + value?: string + placeholder?: string + onValueChange?: (value: string) => void + onKeyDown?: React.KeyboardEventHandler + onKeyUp?: React.KeyboardEventHandler + onCompositionStart?: React.CompositionEventHandler + onCompositionEnd?: React.CompositionEventHandler + }) => ( + onValueChange?.(event.target.value)} + onKeyDown={onKeyDown} + onKeyUp={onKeyUp} + onCompositionStart={onCompositionStart} + onCompositionEnd={onCompositionEnd} + /> + ), + CommandList: ({ + ref, + children, + className + }: { + ref?: React.Ref + children: React.ReactNode + className?: string + }) => ( +
+ {children} +
+ ) +})) + +// The virtual window needs real layout the test DOM has none of; render every row. +vi.mock('@tanstack/react-virtual', () => ({ + useVirtualizer: ({ count }: { count: number }) => ({ + getTotalSize: () => count * 44, + getVirtualItems: () => + Array.from({ length: count }, (_, index) => ({ + index, + key: index, + start: index * 44, + size: 44 + })), + scrollToIndex: vi.fn(), + scrollToOffset: vi.fn() + }) +})) + +const { ComposerParentWorktreePicker } = await import('./ComposerParentWorktreePicker') + +const REPO_ID = 'repo1' + +function makeRepo(id: string): Repo { + return { + id, + path: `/src/${id}`, + displayName: id, + badgeColor: '#111111', + addedAt: 0 + } as Repo +} + +function makeWorktree(overrides: Partial & { id: string }): Worktree { + return { + repoId: REPO_ID, + path: `/work/${overrides.id}`, + head: 'abc123', + branch: 'refs/heads/feature', + isBare: false, + isMainWorktree: false, + displayName: overrides.id, + comment: '', + linkedIssue: null, + linkedPR: null, + linkedLinearIssue: null, + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 0, + lastActivityAt: 0, + ...overrides + } +} + +function seed(worktrees: Worktree[], repoIds: string[] = [REPO_ID]): void { + const byRepo: Record = {} + for (const worktree of worktrees) { + byRepo[worktree.repoId] = [...(byRepo[worktree.repoId] ?? []), worktree] + } + storeState.worktreesByRepo = byRepo + storeState.repos = repoIds.map(makeRepo) +} + +function trigger(): HTMLButtonElement { + const node = document.querySelector('button[role="combobox"]') + if (!node) { + throw new Error('parent picker trigger not found') + } + return node +} + +function rows(): HTMLElement[] { + return Array.from(document.querySelectorAll('[role="option"]')) +} + +function candidateLabels(): string[] { + return rows() + .slice(1) + .map((row) => row.textContent ?? '') +} + +function searchInput(): HTMLInputElement { + const node = document.querySelector('[data-slot="command-input"]') + if (!node) { + throw new Error('parent picker search input not found') + } + return node +} + +function rowFor(label: string): HTMLElement { + const row = rows().find((node) => node.textContent?.includes(label)) + if (!row) { + throw new Error(`row not found for ${label}`) + } + return row +} + +beforeEach(() => { + popoverMock.open = false + popoverMock.onOpenChange = undefined + storeState.worktreesByRepo = {} + storeState.repos = [] + storeState.worktreeLineageById = {} + storeState.workspaceLineageByChildKey = {} + indexSpies.getIndexedAllWorktrees.mockClear() + indexSpies.getIndexedWorktreeMap.mockClear() + indexSpies.getIndexedWorktreeById.mockClear() +}) + +afterEach(cleanup) + +describe('ComposerParentWorktreePicker', () => { + it('enumerates candidate worktrees only once the popover opens', () => { + seed([makeWorktree({ id: 'alpha' }), makeWorktree({ id: 'beta' })]) + + render() + + // The closed trigger resolves one name from the shared index; it never enumerates. + expect(indexSpies.getIndexedWorktreeById).toHaveBeenCalled() + expect(indexSpies.getIndexedAllWorktrees).not.toHaveBeenCalled() + expect(indexSpies.getIndexedWorktreeMap).not.toHaveBeenCalled() + + fireEvent.click(trigger()) + + expect(indexSpies.getIndexedAllWorktrees).toHaveBeenCalled() + expect(indexSpies.getIndexedWorktreeMap).toHaveBeenCalled() + }) + + it('shows "No parent" on the closed trigger when nothing is picked', () => { + seed([makeWorktree({ id: 'alpha' })]) + + render() + + expect(trigger().textContent).toContain('No parent') + }) + + it('shows the picked worktree display name on the closed trigger', () => { + seed([makeWorktree({ id: 'alpha', displayName: 'Alpha workspace' })]) + + render() + + expect(trigger().textContent).toContain('Alpha workspace') + expect(trigger().textContent).not.toContain('No parent') + }) + + it('reports the picked worktree id and closes when a candidate row is clicked', () => { + const onChange = vi.fn() + seed([makeWorktree({ id: 'alpha', displayName: 'Alpha' })]) + + render() + fireEvent.click(trigger()) + fireEvent.click(rowFor('Alpha')) + + expect(onChange).toHaveBeenCalledWith('alpha') + expect(rows()).toHaveLength(0) + }) + + it('clears the pick when the pinned "No parent" row is clicked', () => { + const onChange = vi.fn() + seed([makeWorktree({ id: 'alpha', displayName: 'Alpha' })]) + + render() + fireEvent.click(trigger()) + fireEvent.click(rows()[0]) + + expect(onChange).toHaveBeenCalledWith(null) + expect(rows()).toHaveLength(0) + }) + + it('excludes archived worktrees and worktrees from another repo', () => { + seed( + [ + makeWorktree({ id: 'alpha', displayName: 'Alpha' }), + makeWorktree({ id: 'archived', displayName: 'Archived', isArchived: true }), + makeWorktree({ id: 'other', displayName: 'Other repo', repoId: 'repo2' }) + ], + [REPO_ID, 'repo2'] + ) + + render() + fireEvent.click(trigger()) + + expect(candidateLabels().join(' ')).toContain('Alpha') + expect(candidateLabels().join(' ')).not.toContain('Archived') + expect(candidateLabels().join(' ')).not.toContain('Other repo') + }) + + it('excludes candidates on another execution host or project', () => { + seed([ + makeWorktree({ id: 'same', displayName: 'Same host', hostId: 'local', projectId: 'proj1' }), + makeWorktree({ + id: 'otherHost', + displayName: 'Other host', + hostId: 'ssh:box', + projectId: 'proj1' + }), + makeWorktree({ + id: 'otherProject', + displayName: 'Other project', + hostId: 'local', + projectId: 'proj2' + }) + ]) + + render( + + ) + fireEvent.click(trigger()) + + expect(candidateLabels().join(' ')).toContain('Same host') + expect(candidateLabels().join(' ')).not.toContain('Other host') + expect(candidateLabels().join(' ')).not.toContain('Other project') + }) + + // A worktree with no recorded hostId inherits its repo's host, which is the child's host too. + it('keeps candidates whose host or project is unrecorded', () => { + seed([makeWorktree({ id: 'unscoped', displayName: 'Unscoped' })]) + + render( + + ) + fireEvent.click(trigger()) + + expect(candidateLabels().join(' ')).toContain('Unscoped') + }) + + it('restricts candidates to the active folder workspace subtree', () => { + const attached = makeWorktree({ + id: 'attached', + displayName: 'Attached', + instanceId: 'attached-instance' + }) + const nested = makeWorktree({ + id: 'nested', + displayName: 'Nested', + instanceId: 'nested-instance' + }) + const outside = makeWorktree({ id: 'outside', displayName: 'Outside' }) + seed([attached, nested, outside]) + storeState.workspaceLineageByChildKey = { + [worktreeWorkspaceKey(attached.id)]: { + childWorkspaceKey: worktreeWorkspaceKey(attached.id), + childInstanceId: attached.instanceId ?? null, + parentWorkspaceKey: folderWorkspaceKey('folder-1'), + parentInstanceId: null, + origin: 'manual', + capture: { source: 'active-workspace', confidence: 'explicit' }, + createdAt: 1 + } + } + storeState.worktreeLineageById = { + [nested.id]: { + worktreeId: nested.id, + worktreeInstanceId: 'nested-instance', + parentWorktreeId: attached.id, + parentWorktreeInstanceId: 'attached-instance', + origin: 'manual', + capture: { source: 'manual-action', confidence: 'explicit' }, + createdAt: 2 + } + } + + render( + + ) + fireEvent.click(trigger()) + + const labels = candidateLabels().join(' ') + expect(labels).toContain('Attached') + expect(labels).toContain('Nested') + expect(labels).not.toContain('Outside') + }) + + it('keeps the trigger inert while disabled', () => { + seed([makeWorktree({ id: 'alpha', displayName: 'Alpha' })]) + + render( + + ) + fireEvent.click(trigger()) + + expect(trigger().disabled).toBe(true) + expect(rows()).toHaveLength(0) + }) + + it('picks the top match when Enter follows a search query', () => { + const onChange = vi.fn() + seed([ + makeWorktree({ id: 'alpha', displayName: 'Alpha' }), + makeWorktree({ id: 'beta', displayName: 'Beta' }) + ]) + + render() + fireEvent.click(trigger()) + fireEvent.change(searchInput(), { target: { value: 'Beta' } }) + fireEvent.keyDown(searchInput(), { key: 'Enter', keyCode: 13 }) + + // Regression: the highlight used to reset onto the pinned "No parent" row, so the + // standard type-then-Enter flow cleared the parent instead of picking the match. + expect(onChange).toHaveBeenCalledWith('beta') + }) + + it('clears the parent when Enter is pressed with no search query', () => { + const onChange = vi.fn() + seed([makeWorktree({ id: 'alpha', displayName: 'Alpha' })]) + + render() + fireEvent.click(trigger()) + fireEvent.keyDown(searchInput(), { key: 'Enter', keyCode: 13 }) + + expect(onChange).toHaveBeenCalledWith(null) + }) + + it('keeps the parent when Enter only confirms a CJK composition', () => { + const onChange = vi.fn() + seed([makeWorktree({ id: 'alpha', displayName: 'Alpha' })]) + + render() + fireEvent.click(trigger()) + fireEvent.compositionStart(searchInput()) + // The confirming Enter arrives twice: the marked keydown, then an unmarked redispatch. + fireEvent.keyDown(searchInput(), { key: 'Enter', keyCode: 229 }) + fireEvent.keyDown(searchInput(), { key: 'Enter', keyCode: 13 }) + + expect(onChange).not.toHaveBeenCalled() + expect(rows().length).toBeGreaterThan(0) + }) + + it('falls back to "No parent" on the trigger when the picked parent is archived', () => { + seed([makeWorktree({ id: 'alpha', displayName: 'Alpha workspace', isArchived: true })]) + + render() + + expect(trigger().textContent).not.toContain('Alpha workspace') + expect(trigger().textContent).toContain('No parent') + }) + + it('excludes a folder root whose lineage row is instance-stale', () => { + const attached = makeWorktree({ + id: 'attached', + displayName: 'Attached', + instanceId: 'current-instance' + }) + seed([attached]) + storeState.workspaceLineageByChildKey = { + [worktreeWorkspaceKey(attached.id)]: { + childWorkspaceKey: worktreeWorkspaceKey(attached.id), + childInstanceId: 'deleted-and-recreated', + parentWorkspaceKey: folderWorkspaceKey('folder-1'), + parentInstanceId: null, + origin: 'manual', + capture: { source: 'active-workspace', confidence: 'explicit' }, + createdAt: 1 + } + } + + render( + + ) + fireEvent.click(trigger()) + + expect(candidateLabels().join(' ')).not.toContain('Attached') + }) + + it('closes an open popover when the drawer collapses the trigger', () => { + seed([makeWorktree({ id: 'alpha', displayName: 'Alpha' })]) + + const view = render( + + ) + fireEvent.click(trigger()) + expect(rows().length).toBeGreaterThan(0) + + view.rerender( + + ) + + expect(rows()).toHaveLength(0) + }) +}) diff --git a/src/renderer/src/components/new-workspace/ComposerParentWorktreePicker.tsx b/src/renderer/src/components/new-workspace/ComposerParentWorktreePicker.tsx new file mode 100644 index 00000000000..41c22978f14 --- /dev/null +++ b/src/renderer/src/components/new-workspace/ComposerParentWorktreePicker.tsx @@ -0,0 +1,429 @@ +import React, { useCallback, useEffect, useId, useMemo, useRef, useState } from 'react' +import { useTranslation } from 'react-i18next' +import { Check, ChevronsUpDown, GitBranch } from 'lucide-react' +import { useVirtualizer } from '@tanstack/react-virtual' +import { Button } from '@/components/ui/button' +import { Command, CommandInput, CommandList } from '@/components/ui/command' +import { Popover, PopoverContent, PopoverTrigger } from '@/components/ui/popover' +import { cn } from '@/lib/utils' +import { + isImeCompositionKeyDown, + useImeEnterGestureOwnership +} from '@/lib/ime-composition-keyboard-event' +import { translate } from '@/i18n/i18n' +import { useAppStore } from '@/store' +import { + getIndexedAllWorktrees, + getIndexedWorktreeById, + getIndexedWorktreeMap +} from '@/store/worktree-repo-index' +import { compareWorktreeDisplayName } from '@/lib/worktree-display-name-order' +import { branchDisplayName } from '@/components/sidebar/WorktreeCardHelpers' +import { getCyclicProjectedWorktreeLineageIds } from '@/components/sidebar/worktree-lineage-projection' +import { + clampWorktreeParentPickerIndex, + filterWorktreeParentCandidates +} from '@/components/sidebar/worktree-parent-picker-filtering' +import { + PICKER_LIST_MAX_HEIGHT, + PICKER_ROW_HEIGHT, + PICKER_ROW_OVERSCAN +} from '@/components/sidebar/worktree-parent-picker-placement' +import { + getLineageChildrenByParentId, + getLineageChildWorktree +} from '@/components/right-sidebar/folder-workspace-attached-worktrees' +import { COMBOBOX_POPOVER_SURFACE } from './type-ahead-combobox-styles' +import { + sharesWorktreeLineageBoundary, + type WorktreeLineageBoundary +} from '../../../../shared/resolved-worktree-lineage' +import { folderWorkspaceKey } from '../../../../shared/workspace-scope' +import type { WorkspaceLineage, WorktreeLineage } from '../../../../shared/worktree/lineage-types' +import type { Worktree } from '../../../../shared/worktree/types' +import type { ExecutionHostId } from '../../../../shared/execution-host' + +/** Single-line row: text-sm leading (20px) over py-2. Set as the row's explicit height. */ +const NO_PARENT_ROW_HEIGHT = 36 +const NO_PARENT_ROW_KEY = 'no-parent' + +type ComposerParentWorktreePickerProps = { + repoId: string + /** Parent must belong to the same execution host that will create the child. */ + executionHostId?: ExecutionHostId | null + projectId?: string | null + value: string | null + onChange: (id: string | null) => void + disabled?: boolean + /** When the active workspace is a folder, restrict candidates to that folder's subtree. */ + activeFolderWorkspaceId?: string | null +} + +type ParentWorktreeCandidateListProps = { + repoId: string + executionHostId?: ExecutionHostId | null + projectId?: string | null + value: string | null + activeFolderWorkspaceId: string | null + onSelect: (id: string | null) => void +} + +/** + * Parent picker for the composer's Advanced drawer. Nesting only — the base + * branch is unaffected. + * + * Everything that enumerates worktrees lives in the inner list, which Radix + * mounts on open, so a closed picker costs one string selector and nothing else. + */ +function ComposerParentWorktreePickerImpl({ + repoId, + executionHostId, + projectId, + value, + onChange, + disabled = false, + activeFolderWorkspaceId = null +}: ComposerParentWorktreePickerProps): React.JSX.Element { + // Why: subscribe so translate() copy refreshes on language change without a remount. + useTranslation() + const [open, setOpen] = useState(false) + const labelId = `${useId()}label` + + // Why: an archived parent is already gone from the candidate list and the sidebar would + // drop the edge, so showing its name would promise nesting the create can't deliver. + const parentName = useAppStore((s) => { + if (!value) { + return null + } + const parent = getIndexedWorktreeById(s.worktreesByRepo, value) + return parent && !parent.isArchived ? parent.displayName : null + }) + + // Why: the trigger goes inert when the Advanced drawer collapses, but the portaled + // popover does not — it would linger detached from a control the user can't reach. + // Adjusted during render (not in an effect) so the stale-open frame never paints. + if (open && disabled) { + setOpen(false) + } + + const handleSelect = useCallback( + (nextId: string | null) => { + onChange(nextId) + setOpen(false) + }, + [onChange] + ) + + return ( +
+ + {translate('auto.components.ComposerParentWorktreePicker.label', 'Parent worktree')} + + + + + + { + event.preventDefault() + const content = event.currentTarget + if (content instanceof HTMLElement) { + content.querySelector('[data-slot="command-input"]')?.focus() + } + }} + > + + + +

+ {translate( + 'auto.components.ComposerParentWorktreePicker.description', + 'Nests this workspace under another in the sidebar. Does not change the base branch.' + )} +

+
+ ) +} + +/** Worktrees reachable from the folder workspace, so a pick can't drop the new one out of it. */ +function getFolderWorkspaceSubtreeIds( + folderWorkspaceId: string, + workspaceLineageByChildKey: Readonly>, + worktreeLineageById: Record, + worktreeById: Map +): Set { + const folderKey = folderWorkspaceKey(folderWorkspaceId) + const rootIds = new Set() + for (const lineage of Object.values(workspaceLineageByChildKey)) { + if (lineage.parentWorkspaceKey !== folderKey) { + continue + } + // Why: the folder view drops archived and instance-stale rows, so accepting them here + // would offer a parent whose whole branch the folder never actually shows. + const rootWorktree = getLineageChildWorktree(lineage, worktreeById) + if (rootWorktree) { + rootIds.add(rootWorktree.id) + } + } + + const subtreeIds = new Set(rootIds) + for (const children of getLineageChildrenByParentId( + worktreeLineageById, + worktreeById, + rootIds + ).values()) { + for (const child of children) { + subtreeIds.add(child.id) + } + } + return subtreeIds +} + +function ParentWorktreeCandidateList({ + repoId, + executionHostId, + projectId, + value, + activeFolderWorkspaceId, + onSelect +}: ParentWorktreeCandidateListProps): React.JSX.Element { + const worktreesByRepo = useAppStore((s) => s.worktreesByRepo) + const worktreeLineageById = useAppStore((s) => s.worktreeLineageById) + const workspaceLineageByChildKey = useAppStore((s) => s.workspaceLineageByChildKey) + const listRef = useRef(null) + const inputRef = useRef(null) + const imeEnter = useImeEnterGestureOwnership() + const optionIdPrefix = `${useId()}option` + const [search, setSearch] = useState('') + const [highlightedIndex, setHighlightedIndex] = useState(0) + + const candidates = useMemo(() => { + // Why `?? undefined`: an unresolved host or project is "not known yet", not "no host", and + // the boundary check reads undefined on either side as a wildcard. A candidate in this repo + // with no recorded hostId inherits that repo's host, so it is on the child's host too. + const childBoundary: WorktreeLineageBoundary = { + repoId, + hostId: executionHostId ?? undefined, + projectId: projectId ?? undefined + } + const worktreeMap = getIndexedWorktreeMap(worktreesByRepo) + const cyclicLineageIds = getCyclicProjectedWorktreeLineageIds(worktreeLineageById, worktreeMap) + const folderSubtreeIds = activeFolderWorkspaceId + ? getFolderWorkspaceSubtreeIds( + activeFolderWorkspaceId, + workspaceLineageByChildKey, + worktreeLineageById, + worktreeMap + ) + : null + return getIndexedAllWorktrees(worktreesByRepo) + .filter( + (candidate) => + candidate.repoId === repoId && + !candidate.isArchived && + sharesWorktreeLineageBoundary(childBoundary, candidate) && + !cyclicLineageIds.has(candidate.id) && + (folderSubtreeIds === null || folderSubtreeIds.has(candidate.id)) + ) + .sort(compareWorktreeDisplayName) + }, [ + activeFolderWorkspaceId, + executionHostId, + projectId, + repoId, + workspaceLineageByChildKey, + worktreeLineageById, + worktreesByRepo + ]) + + const filtered = useMemo( + () => filterWorktreeParentCandidates(candidates, search), + [candidates, search] + ) + // Index 0 is the pinned "No parent" row; candidates start at 1. + const activeIndex = clampWorktreeParentPickerIndex(highlightedIndex, filtered.length + 1) + + // Why: the "No parent" row is virtualized as index 0 rather than pinned above the + // scroller. A row outside the virtual container shifts every item's true position + // without the virtualizer knowing, so scrollToIndex would land short by that height. + const virtualizer = useVirtualizer({ + count: filtered.length + 1, + getScrollElement: () => listRef.current, + estimateSize: (index) => (index === 0 ? NO_PARENT_ROW_HEIGHT : PICKER_ROW_HEIGHT), + overscan: PICKER_ROW_OVERSCAN, + getItemKey: (index) => (index === 0 ? NO_PARENT_ROW_KEY : (filtered[index - 1]?.id ?? index)), + // Why: the list mounts inside a popover that measures on the next frame, so + // seed the viewport to avoid a blank first paint. + initialRect: { width: 0, height: PICKER_LIST_MAX_HEIGHT } + }) + + const moveHighlight = useCallback( + (nextIndex: number) => { + setHighlightedIndex(nextIndex) + virtualizer.scrollToIndex(nextIndex, { align: 'auto' }) + }, + [virtualizer] + ) + + const handleSearchChange = useCallback( + (nextSearch: string) => { + // Why: re-ranking on each keystroke makes any prior highlight meaningless. A query + // highlights the top match so Enter picks it; index 0 clears the parent, which is + // never what typing a name means. The clamp drops it back to 0 when nothing matches. + setSearch(nextSearch) + setHighlightedIndex(nextSearch.trim().length > 0 ? 1 : 0) + virtualizer.scrollToOffset(0) + }, + [virtualizer] + ) + + const handleKeyDown = useCallback( + (event: React.KeyboardEvent) => { + // Why: cmdk's root handler only knows the mounted virtual window, so own navigation here. + // A CJK confirm sends two Enter keydowns and only the first is marked, so the marked-only + // check must be paired with the gesture carry or the unmarked one commits a selection. + if (imeEnter.ownsKeyDown(event) || isImeCompositionKeyDown(event)) { + return + } + const navigate = (nextIndex: number): void => { + event.preventDefault() + event.stopPropagation() + moveHighlight(clampWorktreeParentPickerIndex(nextIndex, filtered.length + 1)) + } + if (event.key === 'ArrowDown') { + navigate(activeIndex + 1) + } else if (event.key === 'ArrowUp') { + navigate(activeIndex - 1) + } else if (event.key === 'Home') { + navigate(0) + } else if (event.key === 'End') { + navigate(filtered.length) + } else if (event.key === 'Enter') { + const candidate = activeIndex === 0 ? null : filtered[activeIndex - 1] + if (activeIndex === 0 || candidate) { + event.preventDefault() + event.stopPropagation() + onSelect(candidate?.id ?? null) + } + } + }, + [activeIndex, filtered, imeEnter, moveHighlight, onSelect] + ) + + // Why: cmdk's Input owns aria-activedescendant and points it at its own empty + // item registry while we drive selection, so re-point it after every commit. + useEffect(() => { + inputRef.current?.setAttribute('aria-activedescendant', `${optionIdPrefix}-${activeIndex}`) + }) + + const virtualRows = virtualizer.getVirtualItems() + + return ( + + imeEnter.setComposing(true)} + onCompositionEnd={() => imeEnter.setComposing(false)} + wrapperClassName="shrink-0" + placeholder={translate( + 'auto.components.ComposerParentWorktreePicker.searchPlaceholder', + 'Search workspaces...' + )} + /> + +
+ {virtualRows.map((virtualRow) => { + const rowIndex = virtualRow.index + const candidate = rowIndex === 0 ? null : filtered[rowIndex - 1] + if (rowIndex !== 0 && !candidate) { + return null + } + const isHighlighted = rowIndex === activeIndex + return ( +
setHighlightedIndex(rowIndex)} + onClick={() => onSelect(candidate?.id ?? null)} + > + + {candidate ? ( +
+
{candidate.displayName}
+
+ + {branchDisplayName(candidate.branch)} +
+
+ ) : ( + + {translate( + 'auto.components.ComposerParentWorktreePicker.noParent', + 'No parent' + )} + + )} +
+ ) + })} +
+ {filtered.length === 0 && search.trim().length > 0 ? ( +
+ {translate('auto.components.ComposerParentWorktreePicker.noMatches', 'No matches.')} +
+ ) : null} +
+
+ ) +} + +/** Why: every prop is a primitive or a stable callback, so the composer's per-keystroke + * renders stop here instead of re-rendering the popover subtree. */ +export const ComposerParentWorktreePicker = React.memo(ComposerParentWorktreePickerImpl) + +export default ComposerParentWorktreePicker diff --git a/src/renderer/src/components/new-workspace/NewWorkspaceComposerAdvancedSection.tsx b/src/renderer/src/components/new-workspace/NewWorkspaceComposerAdvancedSection.tsx index 549370ee029..f7b18f6ebaa 100644 --- a/src/renderer/src/components/new-workspace/NewWorkspaceComposerAdvancedSection.tsx +++ b/src/renderer/src/components/new-workspace/NewWorkspaceComposerAdvancedSection.tsx @@ -11,6 +11,7 @@ import { shouldHandleTextControlPaste } from '@/lib/text-control-paste' import { translate } from '@/i18n/i18n' +import { ComposerParentWorktreePicker } from './ComposerParentWorktreePicker' import type { NewWorkspaceComposerCardProps } from './new-workspace-composer-card-props' function SetupCommandPreview({ @@ -37,6 +38,11 @@ type NewWorkspaceComposerAdvancedSectionProps = Pick< | 'branchesEnabled' | 'branchNameOverride' | 'onBranchNameOverrideChange' + | 'parentWorktreeId' + | 'onParentWorktreeIdChange' + | 'selectedRepoExecutionHostId' + | 'selectedRepoProjectId' + | 'activeFolderWorkspaceId' | 'note' | 'onNoteChange' | 'setupControlsEnabled' @@ -74,6 +80,11 @@ export function NewWorkspaceComposerAdvancedSection({ branchNameInputId, branchNameOverride, onBranchNameOverrideChange, + parentWorktreeId = null, + onParentWorktreeIdChange, + selectedRepoExecutionHostId, + selectedRepoProjectId, + activeFolderWorkspaceId = null, note, onNoteChange, setupControlsEnabled = true, @@ -191,6 +202,18 @@ export function NewWorkspaceComposerAdvancedSection({ ) : null} + {selectedRepoIsGit && branchesEnabled && onParentWorktreeIdChange ? ( + + ) : null} +