From 89a4d67705ffe8c5f88e68403f1da7a12bcc5ee8 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 30 Aug 2026 09:33:09 -0700 Subject: [PATCH 01/59] Revert waiting for setup before agent startup (#17418) --- orca.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/orca.yaml b/orca.yaml index b05497b3905..6b7ccd6f1ce 100644 --- a/orca.yaml +++ b/orca.yaml @@ -1,4 +1,3 @@ -setupAgentStartupPolicy: wait-for-setup scripts: setup: | node config/scripts/run-internal-dev-setup.mjs From 6677ae4e5ee8d3ea3452d1f0daa7069e4871c9e3 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 30 Aug 2026 11:45:21 -0700 Subject: [PATCH 02/59] test: correct 8 stale specs surfaced by the test-detected-bugs sweep (#17434) --- ...n-tab-browser-placement-projection.test.ts | 22 +- ...tdown-checkpoint-restart-lifecycle.test.ts | 39 ++- .../worktree-manual-order-store-write.test.ts | 115 ++++++++ .../lib/browser-palette-page-entries.test.ts | 34 +++ tests/e2e/alternate-screen-fixture-script.ts | 15 + ...ternate-screen-fixture-script.unit.test.ts | 91 ++++++ ...ificial-opencode-hidden-pressure-script.ts | 7 +- .../artificial-opencode-terminal-load.spec.ts | 2 +- tests/e2e/helpers/alt-screen-frame.ts | 64 ++-- .../e2e/helpers/alt-screen-frame.unit.test.ts | 111 +++++++ .../helpers/renderer-console-forwarding.ts | 26 ++ tests/e2e/helpers/store.ts | 16 + ...d-remote-browser-link-open-routing.spec.ts | 274 +++++++++++++++--- ...inal-duplicate-pty-renderer-reveal.spec.ts | 116 ++++++-- ...terminal-hidden-tui-visual-restore.spec.ts | 104 +++---- .../e2e/terminal-hidden-view-parking.spec.ts | 22 +- .../terminal-osc8-cold-park-restore.spec.ts | 90 +++--- ...stall-renderer-checkpoint-recovery.spec.ts | 31 +- tests/e2e/worktree-card-downward-drag.spec.ts | 12 +- .../e2e/worktree-jump-palette-filter.spec.ts | 58 +++- 20 files changed, 1031 insertions(+), 218 deletions(-) create mode 100644 src/renderer/src/components/sidebar/worktree-manual-order-store-write.test.ts create mode 100644 tests/e2e/alternate-screen-fixture-script.ts create mode 100644 tests/e2e/alternate-screen-fixture-script.unit.test.ts create mode 100644 tests/e2e/helpers/alt-screen-frame.unit.test.ts create mode 100644 tests/e2e/helpers/renderer-console-forwarding.ts diff --git a/src/main/runtime/rpc/methods/session-tab-browser-placement-projection.test.ts b/src/main/runtime/rpc/methods/session-tab-browser-placement-projection.test.ts index 1337ce504d6..e5f309bcd4e 100644 --- a/src/main/runtime/rpc/methods/session-tab-browser-placement-projection.test.ts +++ b/src/main/runtime/rpc/methods/session-tab-browser-placement-projection.test.ts @@ -1,8 +1,13 @@ import { describe, expect, it } from 'vitest' -import { BROWSER_CLIENT_HOST_RUNTIME_CAPABILITY } from '../../../../shared/protocol-version' +import { + BROWSER_CLIENT_HOST_RUNTIME_CAPABILITY, + ELECTRON_REMOTE_RUNTIME_CLIENT_CAPABILITIES, + NATIVE_REMOTE_RUNTIME_CLIENT_CAPABILITIES +} from '../../../../shared/protocol-version' import type { RuntimeMobileSessionTabsResult } from '../../../../shared/runtime-types' import { assertProjectedSessionTabVisible, + clientCanObserveClientHostedBrowserPages, projectSessionTabBrowserPlacements, translateProjectedSessionTabMove } from './session-tab-browser-placement-projection' @@ -36,6 +41,21 @@ describe('projectSessionTabBrowserPlacements', () => { expect(projected.tabGroupLayout).toEqual({ type: 'leaf', groupId: 'group-terminal' }) }) + // Why named here: a CLI socket sends no capabilities at all, so anything asking it for browser + // tabs — an e2e oracle included — is blind to every client-placed page by design. + it('hides client-placed pages from a native peer and from a caller with no capabilities', () => { + expect( + clientCanObserveClientHostedBrowserPages(NATIVE_REMOTE_RUNTIME_CLIENT_CAPABILITIES) + ).toBe(false) + expect( + clientCanObserveClientHostedBrowserPages(ELECTRON_REMOTE_RUNTIME_CLIENT_CAPABILITIES) + ).toBe(true) + expect(clientCanObserveClientHostedBrowserPages(undefined)).toBe(false) + expect(projectSessionTabBrowserPlacements(makeSnapshot(), undefined).tabs).toEqual([ + expect.objectContaining({ id: 'terminal-leaf' }) + ]) + }) + it('preserves hidden raw slots while translating an old-client reorder', () => { const raw = mixedGroupSnapshot() const projected = projectSessionTabBrowserPlacements(raw, []) diff --git a/src/renderer/src/app-shell/shutdown-checkpoint-restart-lifecycle.test.ts b/src/renderer/src/app-shell/shutdown-checkpoint-restart-lifecycle.test.ts index 55f5c0e51bf..8253932f376 100644 --- a/src/renderer/src/app-shell/shutdown-checkpoint-restart-lifecycle.test.ts +++ b/src/renderer/src/app-shell/shutdown-checkpoint-restart-lifecycle.test.ts @@ -19,7 +19,10 @@ import { isIntentionalAppRestartInProgress, registerUpdaterBeforeUnloadBypass } from '../lib/updater-beforeunload' -import { createShutdownCheckpointPersist } from './shutdown-checkpoint-persist' +import { + createShutdownCheckpointPersist, + type ShutdownCheckpointPersistDeps +} from './shutdown-checkpoint-persist' type LifecycleHarness = { cleanup: () => void @@ -27,9 +30,14 @@ type LifecycleHarness = { stageBeforeUnloadSync: ReturnType } +type LifecycleHarnessOverrides = Partial< + Pick +> + function createLifecycleHarness( startedEventName: string, - abortedEventName: string + abortedEventName: string, + overrides: LifecycleHarnessOverrides = {} ): LifecycleHarness { const stageBeforeUnloadSync = vi.fn((args: { sessions: unknown[] }) => { if (args.sessions.length > 0) { @@ -44,7 +52,8 @@ function createLifecycleHarness( buildUiPatch: () => ({ activeView: 'workspace' }) as never, hasDirtyOpenFiles: () => false, isDegradableShutdownInProgress: isIntentionalAppRestartInProgress, - stageBeforeUnloadSync + stageBeforeUnloadSync, + ...overrides }) const guard = createShutdownCheckpointGuard(persist.run, persist.abandonAttempt) const checkpoint = createShutdownCheckpointBeforeUnloadHandler(guard) @@ -130,4 +139,28 @@ describe('shutdown checkpoint restart lifecycle', () => { expect(harness.stageBeforeUnloadSync).toHaveBeenCalledTimes(2) }) + + // Mirrors the e2e fixture in tests/e2e/update-install-renderer-checkpoint-recovery.spec.ts, + // which asserted the pre-STA-5505 bare message long after the cause suffix landed (STA-5668). + it('names the snapshot-build cause when dirty drafts block the checkpoint', async () => { + const snapshotFailure = "Cannot read properties of null (reading 'toLowerCase')" + vi.spyOn(console, 'error').mockImplementation(() => {}) + const harness = createLifecycleHarness( + ORCA_UPDATER_QUIT_AND_INSTALL_STARTED_EVENT, + ORCA_UPDATER_QUIT_AND_INSTALL_ABORTED_EVENT, + { + buildSessionSnapshots: () => { + throw new Error(snapshotFailure) + }, + hasDirtyOpenFiles: () => true + } + ) + cleanupFns.push(harness.cleanup) + + await expect(harness.prepare()).rejects.toThrow( + new Error(`Renderer shutdown checkpoint was not completed: ${snapshotFailure}`) + ) + // Dirty drafts must block before any durable-only degrade stages over them. + expect(harness.stageBeforeUnloadSync).not.toHaveBeenCalled() + }) }) diff --git a/src/renderer/src/components/sidebar/worktree-manual-order-store-write.test.ts b/src/renderer/src/components/sidebar/worktree-manual-order-store-write.test.ts new file mode 100644 index 00000000000..6789c95df1b --- /dev/null +++ b/src/renderer/src/components/sidebar/worktree-manual-order-store-write.test.ts @@ -0,0 +1,115 @@ +// @vitest-environment happy-dom + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { act, cleanup, renderHook } from '@testing-library/react' +import { useAppStore } from '@/store' +import type { Worktree } from '../../../../shared/worktree/types' +import { makeWorktree } from '../worktree-jump-palette-test-fixtures' +import { buildWorktreeManualOrderCatalog } from './worktree-manual-order-catalog' +import { useWorktreeStatusMutations } from './worktree-list/drag/use-status-mutations' + +/** + * The sidebar drop only reorders if the payload `reorderWorktrees` builds is the + * one `updateWorktreesMeta` consumes. The e2e that covered this replaced the store + * action with a fake, so a payload-shape change (#16691, Map -> batch array) showed + * up as a red drag spec instead of a red contract. This runs the real action. + */ + +const initialState = useAppStore.getInitialState() +const REPO_ID = 'repo-manual-order' +const GROUP_KEY = `repo:${REPO_ID}` + +const updateMeta = vi.fn().mockResolvedValue(undefined) + +function seedManualOrderedRows(count: number): Worktree[] { + const worktrees = Array.from({ length: count }, (_, index) => + // The store routes a metadata write by the repo id embedded in the worktree id. + makeWorktree(`${REPO_ID}::manual-${String(index).padStart(2, '0')}`, `Manual ${index}`, { + repoId: REPO_ID, + manualOrder: 100_000 - index + }) + ) + useAppStore.setState({ + sortBy: 'smart', + worktreesByRepo: { [REPO_ID]: worktrees } + }) + return worktrees +} + +function renderReorder(worktrees: readonly Worktree[]) { + return renderHook(() => + useWorktreeStatusMutations({ + worktreeMap: new Map(worktrees.map((worktree) => [worktree.id, worktree])), + manualOrderCatalog: buildWorktreeManualOrderCatalog({ + worktrees, + folderWorkspaces: [] + }), + workspaceStatuses: [], + sortBy: 'smart' + }) + ).result +} + +function manualOrderedIds(): readonly string[] { + return buildWorktreeManualOrderCatalog({ + worktrees: useAppStore.getState().worktreesByRepo[REPO_ID] ?? [], + folderWorkspaces: [] + }).orderedIds +} + +describe('sidebar manual-order drop', () => { + beforeEach(() => { + useAppStore.setState(initialState, true) + updateMeta.mockClear() + Object.assign(window, { api: { worktrees: { updateMeta } } }) + }) + + afterEach(() => { + cleanup() + useAppStore.setState(initialState, true) + }) + + it('moves the dragged row past its neighbor in the store', async () => { + const worktrees = seedManualOrderedRows(3) + const ids = worktrees.map((worktree) => worktree.id) + const reorder = renderReorder(worktrees) + + await act(async () => { + reorder.current.reorderWorktrees({ + groups: [{ key: GROUP_KEY, worktreeIds: ids }], + sourceGroupKey: GROUP_KEY, + draggedIds: [ids[0]!], + dropIndex: 2 + }) + }) + + expect(manualOrderedIds()).toEqual([ids[1], ids[0], ids[2]]) + expect(useAppStore.getState().sortBy).toBe('manual') + expect(updateMeta).toHaveBeenCalledWith({ + worktreeId: ids[0], + executionHostId: 'local', + updates: { manualOrder: expect.any(Number) } + }) + }) + + it('leaves the order alone when the drop lands where the row already is', async () => { + const worktrees = seedManualOrderedRows(3) + const ids = worktrees.map((worktree) => worktree.id) + const reorder = renderReorder(worktrees) + const epochBeforeDrop = useAppStore.getState().sortEpoch + + await act(async () => { + reorder.current.reorderWorktrees({ + groups: [{ key: GROUP_KEY, worktreeIds: ids }], + sourceGroupKey: GROUP_KEY, + draggedIds: [ids[0]!], + dropIndex: 1 + }) + }) + + expect(manualOrderedIds()).toEqual(ids) + expect(useAppStore.getState().sortBy).toBe('smart') + expect(useAppStore.getState().sortEpoch).toBe(epochBeforeDrop) + expect(updateMeta).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/lib/browser-palette-page-entries.test.ts b/src/renderer/src/lib/browser-palette-page-entries.test.ts index 05cb8d45f3f..c05a29a40bd 100644 --- a/src/renderer/src/lib/browser-palette-page-entries.test.ts +++ b/src/renderer/src/lib/browser-palette-page-entries.test.ts @@ -210,6 +210,40 @@ describe('buildSearchableBrowserPages', () => { ]) }) + it('re-hosts a same-id page entry when the sibling row is missing from the catalog', () => { + // Why: host qualification is gated on both same-id rows being present. With one reaped, a + // local-stamped tab still renders but carries the surviving row's host — so a wrong-host + // Cmd-J activation means the catalog lost a row, not that host qualification regressed. + // This characterizes today's fallback, it does not bless it: overriding a tab's own 'local' + // stamp may be the wrong answer, and changing it is tracked as the unified-tab-host-ownership + // follow-up. Update this expectation with that change rather than treating it as a contract. + const sharedId = 'repo-shared::/workspace' + const remote = makeWorktree({ id: sharedId, hostId: 'runtime:host-b' }) + const entries = buildSearchableBrowserPages({ + worktrees: [remote], + repoMap, + worktreeOrder: new Map([[getWorktreeHostIdentity(remote), 0]]), + browserTabsByWorktree: { + [sharedId]: [ + makeWorkspace({ id: 'ws-local', worktreeId: sharedId, activePageId: 'page-local' }) + ] + }, + browserPagesByWorkspace: { + 'ws-local': [makePage({ id: 'page-local', workspaceId: 'ws-local', worktreeId: sharedId })] + }, + unifiedTabsByWorktree: { + [sharedId]: [browserUnifiedTab('tab-local', 'ws-local', sharedId, 'local')] + }, + activeBrowserTabId: null, + activeWorktreeId: null, + activeTabType: 'terminal' + }) + + expect(entries.map((entry) => [entry.page.id, entry.executionHostId])).toEqual([ + ['page-local', 'runtime:host-b'] + ]) + }) + it('does not route one ambiguous legacy browser bucket to both hosts', () => { const sharedId = 'repo-shared::/workspace' const workspace = makeWorkspace({ worktreeId: sharedId }) diff --git a/tests/e2e/alternate-screen-fixture-script.ts b/tests/e2e/alternate-screen-fixture-script.ts new file mode 100644 index 00000000000..f6a62247e07 --- /dev/null +++ b/tests/e2e/alternate-screen-fixture-script.ts @@ -0,0 +1,15 @@ +// Why an alternate-screen fixture must outlive the assertions that read it: +// main's dead-TUI recovery barrier (src/main/daemon/terminal-shell-recovery-barrier.ts) +// injects `\x1b[?1049l` as soon as a shell prompt proves an alternate-screen owner +// exited without its own cleanup, so a fixture that paints and exits has its frames +// discarded before a spec can observe them. Specs Ctrl-C them once done reading. + +/** Node statement that keeps a fixture — and so its alternate screen — the live PTY foreground. */ +export const HOLD_ALTERNATE_SCREEN_OPEN = 'setInterval(() => {}, 1000)' + +/** Fixture program: paint `payload`, then stay the live alternate-screen owner. */ +export function alternateScreenFixtureScript(payload: string, delayMs = 0): string { + const write = `process.stdout.write(${JSON.stringify(payload)})` + const paint = delayMs > 0 ? `setTimeout(() => ${write}, ${delayMs})` : write + return `${paint}\n${HOLD_ALTERNATE_SCREEN_OPEN}\n` +} diff --git a/tests/e2e/alternate-screen-fixture-script.unit.test.ts b/tests/e2e/alternate-screen-fixture-script.unit.test.ts new file mode 100644 index 00000000000..56718af3620 --- /dev/null +++ b/tests/e2e/alternate-screen-fixture-script.unit.test.ts @@ -0,0 +1,91 @@ +import { readFileSync } from 'node:fs' +import { describe, expect, it } from 'vitest' +import { + HOLD_ALTERNATE_SCREEN_OPEN, + alternateScreenFixtureScript +} from './alternate-screen-fixture-script' +import { + type HiddenPressureOutputMode, + pressureOutputScript +} from './artificial-opencode-hidden-pressure-script' + +// The escape as it appears in generated source, where it is still a JS string escape. +const ALTERNATE_SCREEN_ENTER_SOURCE = '\\x1b[?1049h' + +const PRESSURE_MODES: HiddenPressureOutputMode[] = ['tui', 'plain', 'title', 'latin', 'rich-model'] + +describe('alternateScreenFixtureScript', () => { + it('holds the painting process open so the alternate screen survives the assertions', () => { + const source = alternateScreenFixtureScript('\x1b[?1049hFRAME') + + expect(source).toContain(HOLD_ALTERNATE_SCREEN_OPEN) + expect(source).toContain('process.stdout.write("\\u001b[?1049hFRAME")') + expect(source).not.toContain('setTimeout') + }) + + it('defers the paint by the requested delay and still holds open', () => { + const source = alternateScreenFixtureScript('FRAME', 750) + + expect(source).toContain('setTimeout(() => process.stdout.write("FRAME"), 750)') + expect(source).toContain(HOLD_ALTERNATE_SCREEN_OPEN) + }) +}) + +describe('hidden pressure fixture', () => { + // Ratchet: entering the alternate screen and exiting is the dead-TUI shape the + // recovery barrier deliberately dismisses, which silently emptied these panes. + it.each(PRESSURE_MODES)( + 'keeps mode %s alive exactly when it enters the alternate screen', + (mode) => { + const source = pressureOutputScript('run-id', mode) + + expect(source.includes(HOLD_ALTERNATE_SCREEN_OPEN)).toBe( + source.includes(ALTERNATE_SCREEN_ENTER_SOURCE) + ) + } + ) + + it('holds the rich-model alternate screen open past its done marker', () => { + const source = pressureOutputScript('run-id', 'rich-model') + + expect(source).toContain(ALTERNATE_SCREEN_ENTER_SOURCE) + expect(source.indexOf(HOLD_ALTERNATE_SCREEN_OPEN)).toBeGreaterThan( + source.indexOf('OPENCODE_PRESSURE_DONE_') + ) + }) +}) + +// Ratchet: the builder is only worth anything while the specs still route through it, +// and a fixture hand-rolled back to paint-and-exit would surface days later in a +// scheduled Electron run — the detection channel that produced this ticket. +describe('spec alternate-screen fixtures', () => { + const SPEC_FIXTURE_BUILDERS: [spec: string, builder: string][] = [ + ['terminal-hidden-view-parking.spec.ts', 'writeParkedFrameScript'], + ['terminal-hidden-view-parking.spec.ts', 'writeCycleReferenceScript'], + ['terminal-hidden-tui-visual-restore.spec.ts', 'writeHiddenFrameScript'] + ] + + function readSpec(spec: string): string { + return readFileSync(new URL(spec, import.meta.url), 'utf8') + } + + function topLevelFunctionBody(source: string, name: string): string { + const start = source.indexOf(`function ${name}(`) + expect(start, `${name} was renamed or removed; re-point this ratchet`).toBeGreaterThan(-1) + // Why '\n}' and a bound: '\n}\n' misses CRLF checkouts, and an unresolved indexOf slices to EOF, + // letting a later builder call in the same file satisfy the assertion vacuously. + const end = source.indexOf('\n}', start) + expect(end, `${name} has no closing brace; re-point this ratchet`).toBeGreaterThan(start) + return source.slice(start, end) + } + + it.each(SPEC_FIXTURE_BUILDERS)('%s writes %s through the shared builder', (spec, builder) => { + expect(topLevelFunctionBody(readSpec(spec), builder)).toContain('alternateScreenFixtureScript(') + }) + + it('the OSC 8 spec stages its link fixture through the shared builder', () => { + expect(readSpec('terminal-osc8-cold-park-restore.spec.ts')).toContain( + 'stageNodeScriptForTerminal(alternateScreenFixtureScript(' + ) + }) +}) diff --git a/tests/e2e/artificial-opencode-hidden-pressure-script.ts b/tests/e2e/artificial-opencode-hidden-pressure-script.ts index 1eb94b3de89..2fae1387027 100644 --- a/tests/e2e/artificial-opencode-hidden-pressure-script.ts +++ b/tests/e2e/artificial-opencode-hidden-pressure-script.ts @@ -1,5 +1,6 @@ import { mkdirSync, writeFileSync } from 'node:fs' import path from 'node:path' +import { HOLD_ALTERNATE_SCREEN_OPEN } from './alternate-screen-fixture-script' export type HiddenPressureOutputMode = 'tui' | 'plain' | 'title' | 'latin' | 'rich-model' @@ -16,6 +17,10 @@ export function pressureOutputScript(runId: string, mode: HiddenPressureOutputMo : mode === 'rich-model' ? "'\\x1b[?2026h\\x1b[?1049h\\x1b[2J\\x1b[H\\x1b[?25l\\x1b[2;36m╭────────────────────────────────────────╮\\x1b[0m\\r\\n\\x1b[2;36m│ rich model pane=' + paneIndex + ' frame=' + frame + ' 😀 ███░ │\\x1b[0m\\r\\n\\x1b[2;36m│ ' + chunkBody + ' │\\x1b[0m\\r\\n\\x1b[2;36m╰────────────────────────────────────────╯\\x1b[0m\\x1b[6;4H\\x1b[?25h\\x1b[?2026l\\n'" : "'\\x1b[?2026h\\x1b[1;1Hpressure pane=' + paneIndex + ' frame=' + frame + ' ' + chunkBody + '\\x1b[?2026l\\n'" + // Why only rich-model: it is the one mode that enters the alternate screen, and an + // alt-screen owner that exits is exactly the dead TUI main's recovery barrier + // discards — frames and done marker with it. Scenario cleanup Ctrl-Cs the pane. + const holdOpen = mode === 'rich-model' ? `\n${HOLD_ALTERNATE_SCREEN_OPEN}` : '' return ` const paneIndex = process.argv[2] ?? '0' const targetChars = Number(process.argv[3] ?? '0') @@ -38,7 +43,7 @@ function writeMore() { } process.stdout.write('${donePrefix}OPENCODE_PRESSURE_DONE_${runId}_' + paneIndex + '\\n') } -setTimeout(writeMore, Number.isFinite(delayMs) && delayMs > 0 ? delayMs : 0) +setTimeout(writeMore, Number.isFinite(delayMs) && delayMs > 0 ? delayMs : 0)${holdOpen} ` } diff --git a/tests/e2e/artificial-opencode-terminal-load.spec.ts b/tests/e2e/artificial-opencode-terminal-load.spec.ts index 63780c2ae64..bf7b7d2c7db 100644 --- a/tests/e2e/artificial-opencode-terminal-load.spec.ts +++ b/tests/e2e/artificial-opencode-terminal-load.spec.ts @@ -447,7 +447,7 @@ async function measureCrossWorkspaceTypingDuringHiddenLoad({ scheduler, mainPressure ) - expect(scheduler?.rendererDroppedBacklogs ?? 0).toBe(0) + expect(scheduler?.droppedBacklogCount ?? Number.POSITIVE_INFINITY).toBe(0) expect(measurement.medianLatencyMs).toBeLessThan(MAX_MEDIAN_KEY_LATENCY_MS) expect(measurement.worstLatencyMs).toBeLessThan(MAX_WORST_KEY_LATENCY_UNDER_LOAD_MS) expect(measurement.maxTimerDriftMs).toBeLessThan(MAX_TIMER_DRIFT_UNDER_LOAD_MS) diff --git a/tests/e2e/helpers/alt-screen-frame.ts b/tests/e2e/helpers/alt-screen-frame.ts index 6292b0c84e1..7ef3b334652 100644 --- a/tests/e2e/helpers/alt-screen-frame.ts +++ b/tests/e2e/helpers/alt-screen-frame.ts @@ -17,6 +17,47 @@ export function buildAltScreenFrame(marker: string, frame: number): string { ].join('\r\n') } +export type ActiveScreen = { + bufferType: 'normal' | 'alternate' + rows: string[] +} + +// Why: what the pane shows is the active buffer's viewport. `serializeAddon.serialize()` +// dumps the whole normal buffer (scrollback included) before the alt frame, so a stale +// marker there is indistinguishable from the live one (STA-5208). Null on a missing pane +// because callers poll this and `expect.poll` aborts on a generator throw. +export async function readActiveScreen(page: Page, tabId: string): Promise { + return page.evaluate( + ({ tabId }) => { + const manager = window.__paneManagers?.get(tabId) + const pane = manager?.getActivePane?.() ?? manager?.getPanes?.()[0] ?? null + if (!pane) { + return null + } + const buffer = pane.terminal.buffer.active + const rows: string[] = [] + for (let row = 0; row < pane.terminal.rows; row += 1) { + rows.push(buffer.getLine(buffer.viewportY + row)?.translateToString(true) ?? '') + } + return { bufferType: buffer.type, rows } + }, + { tabId } + ) +} + +// Why the highest match rather than the first: a repaint can leave an older marker line +// beside the live one, and only the newest frame says which paint landed last. +export function findMarkerFrame(text: string, marker: string): number | null { + // marker is a literal, so escape it rather than letting `[`/`.`/`+` act as regex syntax. + const pattern = new RegExp(`${marker.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')} frame (\\d+)`, 'g') + let latest: number | null = null + for (const match of text.matchAll(pattern)) { + const frame = Number(match[1]) + latest = latest === null ? frame : Math.max(latest, frame) + } + return latest +} + // Why: the live-write and the reveal restore paint the same layout, so the frame // number is the only thing on screen that says which of the two landed last. export async function readRenderedAltScreenFrame( @@ -24,27 +65,8 @@ export async function readRenderedAltScreenFrame( tabId: string, marker: string ): Promise { - return page.evaluate( - ({ tabId, marker }) => { - const manager = window.__paneManagers?.get(tabId) - const pane = manager?.getActivePane?.() ?? manager?.getPanes?.()[0] - if (!pane) { - throw new Error(`No terminal pane for tab ${tabId}`) - } - // marker is a literal, so escape it rather than letting `[`/`.`/`+` act as regex syntax. - const pattern = new RegExp(`${marker.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')} frame (\\d{3})`) - const buffer = pane.terminal.buffer.active - for (let row = 0; row < pane.terminal.rows; row += 1) { - const line = buffer.getLine(buffer.viewportY + row)?.translateToString(true) ?? '' - const match = pattern.exec(line) - if (match) { - return Number(match[1]) - } - } - return null - }, - { tabId, marker } - ) + const screen = await readActiveScreen(page, tabId) + return screen ? findMarkerFrame(screen.rows.join('\n'), marker) : null } export function describeAltScreenRenderPath( diff --git a/tests/e2e/helpers/alt-screen-frame.unit.test.ts b/tests/e2e/helpers/alt-screen-frame.unit.test.ts new file mode 100644 index 00000000000..1cab97f334c --- /dev/null +++ b/tests/e2e/helpers/alt-screen-frame.unit.test.ts @@ -0,0 +1,111 @@ +// STA-5208: the duplicate-PTY reveal oracle parsed `serializeAddon.serialize()`, which +// concatenates the whole normal buffer (scrollback included) ahead of the alt frame, so a +// stale marker left behind by the pre-hide paint was read as the revealed pane's current +// frame. These pin the replacement oracle: read the frame off the active buffer's viewport. +import '../../../src/main/daemon/xterm-env-polyfill' +import { describe, expect, it } from 'vitest' +import { Terminal } from '@xterm/headless' +import { SerializeAddon } from '@xterm/addon-serialize' +import type { Page } from '@stablyai/playwright-test' +import { findMarkerFrame, readActiveScreen, readRenderedAltScreenFrame } from './alt-screen-frame' + +const MARKER = 'DUPLICATE_PTY_REVEAL_TEST' +const TAB_ID = 'tab-under-test' + +type Harness = { page: Page; terminal: Terminal; serialize: () => string } + +// Matches the streaming TUI fixture in terminal-duplicate-pty-renderer-reveal.spec.ts. +function frameLine(frame: number): string { + return `${MARKER} frame ${String(frame).padStart(6, '0')}` +} + +function write(terminal: Terminal, data: string): Promise { + return new Promise((resolve) => terminal.write(data, () => resolve())) +} + +// Runs the helper's real in-page closure against a headless terminal so the test covers +// the code the browser executes rather than a copy of it. +function createHarness(rows: number): Harness { + const terminal = new Terminal({ cols: 80, rows, scrollback: 100, allowProposedApi: true }) + const serializeAddon = new SerializeAddon() + terminal.loadAddon(serializeAddon) + const pane = { terminal, serializeAddon } + ;(globalThis as Record).__paneManagers = new Map([ + [TAB_ID, { getPanes: () => [pane] }] + ]) + const page = { + evaluate: (fn: (arg: Arg) => Result, arg: Arg): Promise => + Promise.resolve(fn(arg)) + } as unknown as Page + return { page, terminal, serialize: () => serializeAddon.serialize() } +} + +// The oracle this replaces: a positional scan over every buffer serialize() emits. +function parseSerializedFrame(content: string, pick: 'first' | 'last'): number | null { + const prefix = `${MARKER} frame ` + const start = pick === 'first' ? content.indexOf(prefix) : content.lastIndexOf(prefix) + if (start < 0) { + return null + } + const digits = content.slice(start + prefix.length).match(/^\d+/)?.[0] + return digits ? Number(digits) : null +} + +describe('readRenderedAltScreenFrame', () => { + it('reads the live alt frame, not the stale copy left in the normal buffer', async () => { + const harness = createHarness(8) + await write(harness.terminal, `${frameLine(390)}\r\n`) + await write(harness.terminal, `\x1b[?1049h\x1b[H${frameLine(400)}\x1b[J`) + + const serialized = harness.serialize() + expect(parseSerializedFrame(serialized, 'first')).toBe(390) + expect(parseSerializedFrame(serialized, 'last')).toBe(400) + + await expect(readActiveScreen(harness.page, TAB_ID)).resolves.toMatchObject({ + bufferType: 'alternate' + }) + await expect(readRenderedAltScreenFrame(harness.page, TAB_ID, MARKER)).resolves.toBe(400) + }) + + it('reports no frame when the freshest marker scrolled off the visible rows', async () => { + const harness = createHarness(8) + await write(harness.terminal, `${frameLine(400)}\r\n`) + for (let row = 0; row < 12; row += 1) { + await write(harness.terminal, `filler row ${row}\r\n`) + } + + // serialize() still contains the marker from scrollback, so it cannot see that nothing + // correct is on screen; the viewport read can. + expect(parseSerializedFrame(harness.serialize(), 'last')).toBe(400) + await expect(readActiveScreen(harness.page, TAB_ID)).resolves.toMatchObject({ + bufferType: 'normal' + }) + await expect(readRenderedAltScreenFrame(harness.page, TAB_ID, MARKER)).resolves.toBeNull() + }) + + // expect.poll aborts on a generator throw, so a pane mid-remount has to read as + // "not converged yet" rather than ending the poll. + it('returns null when the tab has no pane', async () => { + const harness = createHarness(8) + await expect(readActiveScreen(harness.page, 'tab-without-pane')).resolves.toBeNull() + await expect( + readRenderedAltScreenFrame(harness.page, 'tab-without-pane', MARKER) + ).resolves.toBeNull() + }) +}) + +describe('findMarkerFrame', () => { + it('reads frame numbers of any width', () => { + expect(findMarkerFrame(`${MARKER} frame 000400`, MARKER)).toBe(400) + expect(findMarkerFrame(`| ${MARKER} frame 024 |`, MARKER)).toBe(24) + }) + + it('takes the highest frame when several are on screen', () => { + expect(findMarkerFrame([frameLine(400), frameLine(390)].join('\n'), MARKER)).toBe(400) + }) + + it('treats the marker as a literal', () => { + expect(findMarkerFrame('A[B frame 7', 'A[B')).toBe(7) + expect(findMarkerFrame('AxB frame 7', 'A[B')).toBeNull() + }) +}) diff --git a/tests/e2e/helpers/renderer-console-forwarding.ts b/tests/e2e/helpers/renderer-console-forwarding.ts new file mode 100644 index 00000000000..13277597d6b --- /dev/null +++ b/tests/e2e/helpers/renderer-console-forwarding.ts @@ -0,0 +1,26 @@ +import type { Page, TestInfo } from '@stablyai/playwright-test' + +/** + * Renderer-side counterpart of `forwardElectronProcessLogs`, sharing its + * `ORCA_E2E_FORWARD_APP_LOGS` gate. + * + * Why: a contained render crash only ever reaches the renderer console + * (`RecoverableRenderErrorBoundary` logs the error plus its component stack + * there), so without this a boundary failure leaves nothing but a screenshot of + * the dialog and the stack that would localize the first bad render is lost. + */ +export function forwardRendererConsole(page: Page, testInfo: TestInfo): void { + if (process.env.ORCA_E2E_FORWARD_APP_LOGS !== '1') { + return + } + + const prefix = `[renderer:${testInfo.title}]` + page.on('console', (message) => { + if (message.type() === 'error' || message.type() === 'warning') { + console.error(`${prefix} ${message.type()}: ${message.text()}`) + } + }) + page.on('pageerror', (error) => { + console.error(`${prefix} pageerror: ${error.stack ?? error.message}`) + }) +} diff --git a/tests/e2e/helpers/store.ts b/tests/e2e/helpers/store.ts index e11c3921f82..dad165494d4 100644 --- a/tests/e2e/helpers/store.ts +++ b/tests/e2e/helpers/store.ts @@ -154,6 +154,22 @@ export async function waitForSessionReady(page: Page, timeoutMs = 30_000): Promi .toBe(true) } +/** + * Wait until the deferred startup worktree scan has completed. + * + * Why: hydration fires an unawaited full catalog refresh after + * `workspaceSessionReady`; a fixture seeded before it lands is silently + * overwritten when it does. + */ +export async function waitForStartupWorktreeRefresh(page: Page, timeoutMs = 60_000): Promise { + await expect + .poll(async () => getStoreState(page, 'startupWorktreeRefreshCompleted'), { + timeout: timeoutMs, + message: 'startupWorktreeRefreshCompleted did not become true' + }) + .toBe(true) +} + /** Wait until a worktree is active and return its ID. */ export async function waitForActiveWorktree(page: Page, timeoutMs = 30_000): Promise { let activeWorktreeId: string | null = null diff --git a/tests/e2e/paired-remote-browser-link-open-routing.spec.ts b/tests/e2e/paired-remote-browser-link-open-routing.spec.ts index 6da4965703e..695d39c251b 100644 --- a/tests/e2e/paired-remote-browser-link-open-routing.spec.ts +++ b/tests/e2e/paired-remote-browser-link-open-routing.spec.ts @@ -1,19 +1,26 @@ import { createServer, type IncomingMessage, type Server, type ServerResponse } from 'node:http' import type { AddressInfo } from 'node:net' import type { Page } from '@stablyai/playwright-test' +import { parseBrowserNetworkExecutionHostKey } from '../../src/main/browser/browser-network-execution-route' import { LOCAL_EXECUTION_HOST_ID } from '../../src/shared/execution-host' +import { readOwnedPageUrls } from './helpers/client-hosted-browser-observer' import { launchHeadlessPairedRuntimeHost, type HeadlessPairedRuntimeHost } from './helpers/headless-paired-runtime-host' +import { readHostBrowserPageUrls } from './helpers/host-session-tabs' import { expect, test } from './helpers/orca-app' import { launchPairedElectronClient, type PairedElectronClient } from './helpers/paired-electron-client' -// The link is a dev-server URL, so a client-local fallback would silently load a *different -// machine's* server while looking successful. +// The link is a dev-server URL on the pane runtime's network, so a client-local fallback would +// silently load a *different machine's* server. Which machine renders the pixels no longer answers +// that: under client-hosted placement the guest paints on this desktop while its network is still +// pinned to the host at creation. So each act below pins the placement it was written for and reads +// the host's own record — the page row's placement and executionHostKey — instead of inferring +// routing from where a appeared. const PANE_PATH = '/remote-pane' const LINK_PATH = '/remote-link-target' @@ -77,8 +84,14 @@ async function startLinkFixtureServer(): Promise { } } -/** Asked over the host's own connection, not proxied through the client under test. */ -async function readHostBrowserUrls( +/** + * The host's session-tab view, asked over the host's own CLI socket rather than proxied through the + * client under test. + * + * Server-placed pages only: this socket advertises no `BROWSER_CLIENT_HOST_RUNTIME_CAPABILITY`, so + * the host strips every client-placed page from the snapshot before answering. + */ +async function readHostServerPlacedBrowserUrls( host: HeadlessPairedRuntimeHost, worktreeId: string ): Promise { @@ -90,7 +103,57 @@ async function readHostBrowserUrls( return response.result.tabs.filter((tab) => tab.type === 'browser').map((tab) => tab.url ?? '') } -/** A remote pane is a screencast image; a means the page really loaded on this machine. */ +type HostBrowserRow = { + executionHostKey: string | null + placementKind: string | null + url: string +} + +/** + * The host's rows for one URL, asked through the paired client's connection. + * + * Why through the client: an Electron peer advertises the client-host capability, so the host + * answers it with client-placed pages intact and with the placement and network pin it minted at + * creation. The host still authors every field; the client is only the transport. + */ +async function readHostBrowserRows( + page: Page, + environmentId: string, + worktreeId: string, + urlPrefix: string +): Promise { + return page.evaluate( + async ({ environmentId, urlPrefix, worktreeId }) => { + const response = await window.api.runtimeEnvironments.call({ + selector: environmentId, + method: 'session.tabs.list', + params: { worktree: `id:${worktreeId}` }, + timeoutMs: 15_000 + }) + if (!response.ok) { + throw new Error('host session tab inventory unavailable') + } + const { tabs } = response.result as { + tabs: { + type: string + url?: string + executionHostKey?: string + placement?: { kind: string } + }[] + } + return tabs + .filter((tab) => tab.type === 'browser' && (tab.url ?? '').startsWith(urlPrefix)) + .map((tab) => ({ + executionHostKey: tab.executionHostKey ?? null, + placementKind: tab.placement?.kind ?? null, + url: tab.url ?? '' + })) + }, + { environmentId, urlPrefix, worktreeId } + ) +} + +/** Under server placement the client renders nothing itself, so any is a local fallback. */ async function readLocalBrowserViewUrls(page: Page): Promise { return page.evaluate(() => Array.from(document.querySelectorAll('webview')).map( @@ -117,17 +180,22 @@ async function findMirroredPage( page: Page, worktreeId: string, url: string -): Promise<{ handleEnvironmentId: string | null; pageId: string } | null> { +): Promise<{ + handleEnvironmentId: string | null + pageId: string + placementKind: string | null +} | null> { return page.evaluate( ({ url, worktreeId }) => { const state = window.__store?.getState() for (const workspace of state?.browserTabsByWorktree[worktreeId] ?? []) { for (const browserPage of state?.browserPagesByWorkspace[workspace.id] ?? []) { if (browserPage.url.startsWith(url)) { + const handle = state?.remoteBrowserPageHandlesByPageId[browserPage.id] return { - handleEnvironmentId: - state?.remoteBrowserPageHandlesByPageId[browserPage.id]?.environmentId ?? null, - pageId: browserPage.id + handleEnvironmentId: handle?.environmentId ?? null, + pageId: browserPage.id, + placementKind: handle?.placement?.kind ?? null } } } @@ -149,13 +217,50 @@ async function focusMirroredPage(page: Page, worktreeId: string, pageId: string) ) } +/** Placement is a user setting whose default has already flipped once, so every act pins its own. */ +async function pinClientHostedPlacement(page: Page, enabled: boolean): Promise { + await page.evaluate(async (enabled) => { + await window.__store?.getState().updateSettings({ browserClientHostedRemoteEnabled: enabled }) + }, enabled) + expect( + await page.evaluate( + () => window.__store?.getState().settings?.browserClientHostedRemoteEnabled ?? null + ), + 'the placement setting this act is written for did not take' + ).toBe(enabled) +} + +/** Leaves the workspace holding only the screencast pane the next act right-clicks. */ +async function closeBrowserTabsExceptPane( + page: Page, + worktreeId: string, + paneUrl: string +): Promise { + await page.evaluate( + ({ paneUrl, worktreeId }) => { + const state = window.__store?.getState() + for (const workspace of state?.browserTabsByWorktree[worktreeId] ?? []) { + const pages = state?.browserPagesByWorkspace[workspace.id] ?? [] + if (!pages.some((browserPage) => browserPage.url.startsWith(paneUrl))) { + state?.closeBrowserTab(workspace.id) + } + } + }, + { paneUrl, worktreeId } + ) +} + type LinkOpenOutcome = 'opened on this machine' | 'pending' | 'refused' -async function readLinkOpenOutcome(page: Page, linkUrl: string): Promise { - if ((await readLocalBrowserViewUrls(page)).some((url) => url.startsWith(linkUrl))) { +/** The previous act's guest is settled away before this runs, so any page here is the fallback. */ +async function readLinkOpenOutcome( + client: PairedElectronClient, + linkUrl: string +): Promise { + if ((await readOwnedPageUrls(client.app, linkUrl)).length > 0) { return 'opened on this machine' } - const notice = page.getByTestId('remote-browser-stream-error') + const notice = client.page.getByTestId('remote-browser-stream-error') const text = (await notice.count()) > 0 ? ((await notice.first().textContent()) ?? '') : '' return text.includes('Unable to open URL.') ? 'refused' : 'pending' } @@ -172,7 +277,7 @@ async function openLinkFromRemotePaneContextMenu(page: Page): Promise { await openInOrca.click() } -test('opens a remote pane link on the pane runtime and refuses to fall back to the client', async ({ +test('opens a remote pane link on the pane runtime under either placement and refuses to fall back to the client', async ({ testRepoPath }, testInfo) => { test.setTimeout(300_000) @@ -181,7 +286,8 @@ test('opens a remote pane link on the pane runtime and refuses to fall back to t let client: PairedElectronClient | null = null try { - await host.client.call('repo.add', { path: testRepoPath, kind: 'git' }) + const hostRuntimeId = (await host.client.call('repo.add', { path: testRepoPath, kind: 'git' })) + ._meta.runtimeId client = await launchPairedElectronClient(host.offer, testInfo, 'Remote browser link routing') const page = client.page const environmentId = client.environmentId @@ -199,6 +305,8 @@ test('opens a remote pane link on the pane runtime and refuses to fall back to t throw new Error('paired client did not receive the host worktree') } + const worktreeSelector = `id:${worktreeId}` + // The workspace runs on the paired runtime, the way it does when the user picks that host. await page.evaluate( ({ environmentId, worktreeId }) => { @@ -227,13 +335,15 @@ test('opens a remote pane link on the pane runtime and refuses to fall back to t await focusMirroredPage(page, worktreeId, pane.pageId) const paneCountBeforeOpen = await page.getByTestId('remote-browser-pane').count() - // Act 1: the healthy path must land on the runtime, end to end. + // Act 1: server placement. The user asked for pages to live on the server, so the link must + // land on the runtime and be streamed back — nothing renders here. + await pinClientHostedPlacement(page, false) await openLinkFromRemotePaneContextMenu(page) await expect .poll( async () => - (await readHostBrowserUrls(host, worktreeId)).filter((url) => + (await readHostServerPlacedBrowserUrls(host, worktreeId)).filter((url) => url.startsWith(fixture.linkUrl) ).length, { timeout: 60_000, message: 'the link never opened as a browser tab on the host runtime' } @@ -241,6 +351,12 @@ test('opens a remote pane link on the pane runtime and refuses to fall back to t .toBe(1) // The host's browser really fetched it; a tab record alone would not prove a load. expect(fixture.linkLoadCount()).toBeGreaterThan(0) + await expect + .poll(() => readOwnedPageUrls(host.app, fixture.linkUrl), { + timeout: 60_000, + message: 'the runtime process never held a page for the link' + }) + .toHaveLength(1) // One more remote pane, and still nothing rendered by this machine's own browser. await expect(page.getByTestId('remote-browser-pane')).toHaveCount(paneCountBeforeOpen + 1, { timeout: 60_000 @@ -250,34 +366,118 @@ test('opens a remote pane link on the pane runtime and refuses to fall back to t ) expect(await readLocalBrowserViewUrls(page)).toHaveLength(0) - // Drop every tab except the pane's, so the second act drives the pane it started with. - await page.evaluate( - ({ paneUrl, worktreeId }) => { - const state = window.__store?.getState() - for (const workspace of state?.browserTabsByWorktree[worktreeId] ?? []) { - const pages = state?.browserPagesByWorkspace[workspace.id] ?? [] - if (!pages.some((browserPage) => browserPage.url.startsWith(paneUrl))) { - state?.closeBrowserTab(workspace.id) - } - } - }, - { paneUrl: fixture.paneUrl, worktreeId } - ) + // Drop every tab except the pane's, so the next act drives the pane it started with against a + // host that no longer holds the link. + await closeBrowserTabsExceptPane(page, worktreeId, fixture.paneUrl) await expect(page.getByTestId('remote-browser-pane')).toHaveCount(paneCountBeforeOpen, { timeout: 60_000 }) + await expect + .poll( + async () => + (await readHostServerPlacedBrowserUrls(host, worktreeId)).filter((url) => + url.startsWith(fixture.linkUrl) + ).length, + { timeout: 60_000, message: 'the runtime kept the closed browser tab' } + ) + .toBe(0) + await focusMirroredPage(page, worktreeId, pane.pageId) + const linkLoadsBeforeClientAct = fixture.linkLoadCount() - // Act 2: the user moves this workspace onto their own machine while the runtime's page is + // Act 2: client-hosted placement, the default. The page is hosted by this desktop, so the + // proof of correct routing is the host's record of it, not where it painted. + await pinClientHostedPlacement(page, true) + await openLinkFromRemotePaneContextMenu(page) + + await expect + .poll( + async () => (await findMirroredPage(page, worktreeId, fixture.linkUrl))?.placementKind, + { + timeout: 60_000, + message: 'the link never became a client-hosted browser page on this desktop' + } + ) + .toBe('client') + // The host's own connection, unprojected: browser.tabList reads the page registry, which is + // where a client-hosted page lives. + await expect + .poll( + async () => + (await readHostBrowserPageUrls(host.client, worktreeSelector)).filter((url) => + url.startsWith(fixture.linkUrl) + ).length, + { timeout: 60_000, message: 'the link never became a browser page on the host runtime' } + ) + .toBe(1) + const hostRows = await readHostBrowserRows(page, environmentId, worktreeId, fixture.linkUrl) + expect(hostRows).toHaveLength(1) + expect(hostRows[0]?.placementKind).toBe('client') + // The routing invariant, structurally: the host pinned this page's network to its own runtime + // when it created it, so the dev server was reached through the runtime and not through this + // machine — which CI cannot tell apart by watching the fixture, since both ends are loopback. + const executionHostKey = hostRows[0]?.executionHostKey + if (!executionHostKey) { + // Narrowed before parsing: an unpinned page would otherwise surface as a parse crash rather + // than as the missing network pin it is. + throw new Error('the host minted no network pin for the client-hosted page') + } + expect(parseBrowserNetworkExecutionHostKey(executionHostKey)).toMatchObject({ + runtimeId: hostRuntimeId + }) + expect(fixture.linkLoadCount()).toBeGreaterThan(linkLoadsBeforeClientAct) + // Hosted here, streamed from nowhere: this desktop holds the page and the runtime holds none. + await expect + .poll(() => readOwnedPageUrls(client!.app, fixture.linkUrl), { + timeout: 60_000, + message: 'the client-hosted guest never loaded the link on this desktop' + }) + .toHaveLength(1) + expect(await readOwnedPageUrls(host.app, fixture.linkUrl)).toHaveLength(0) + await expect(page.getByTestId('remote-browser-pane')).toHaveCount(paneCountBeforeOpen) + + // The store drops the tab synchronously and only then fires browser.tabClose, so the mirror + // going empty proves nothing about the host or the guest. Settle both before act 3 baselines + // them, or act 3 reads this teardown landing mid-act as its own doing. + await closeBrowserTabsExceptPane(page, worktreeId, fixture.paneUrl) + await expect + .poll(() => findMirroredPage(page, worktreeId, fixture.linkUrl), { + timeout: 60_000, + message: 'the client kept the closed link tab' + }) + .toBeNull() + await expect + .poll( + async () => + (await readHostBrowserPageUrls(host.client, worktreeSelector)).filter((url) => + url.startsWith(fixture.linkUrl) + ).length, + { timeout: 60_000, message: 'the runtime kept the closed client-hosted page' } + ) + .toBe(0) + await expect + .poll(() => readOwnedPageUrls(client!.app, fixture.linkUrl), { + timeout: 60_000, + message: 'the client-hosted guest outlived the tab that owned it' + }) + .toHaveLength(0) + await focusMirroredPage(page, worktreeId, pane.pageId) + + // Act 3: the user moves this workspace onto their own machine while the runtime's page is // still on screen. Opening the link must fail in the pane, not load the runtime's dev server // here — the client has no business serving a page for a workspace it does not run. await page.evaluate( ({ localHostId, worktreeId }) => { window.__store?.getState().setActiveWorktree(worktreeId, localHostId) }, - { localHostId: LOCAL_EXECUTION_HOST_ID, worktreeId } + // `as const` keeps the host id a literal through serialization; widened to string it stops + // being an ExecutionHostId. + { localHostId: LOCAL_EXECUTION_HOST_ID, worktreeId } as const ) await focusMirroredPage(page, worktreeId, pane.pageId) - const hostUrlsBefore = await readHostBrowserUrls(host, worktreeId) + // The refusal is about who owns the workspace, not where pages render, so it must hold under + // the placement the user most likely has on. + await pinClientHostedPlacement(page, true) + const hostPagesBefore = await readHostBrowserPageUrls(host.client, worktreeSelector) const linkLoadsBefore = fixture.linkLoadCount() await openLinkFromRemotePaneContextMenu(page) @@ -285,20 +485,20 @@ test('opens a remote pane link on the pane runtime and refuses to fall back to t // Wait for the click to produce an outcome — refusal or a local page — so the assertion below // reports which one happened instead of racing past a fallback that lands a moment later. await expect - .poll(() => readLinkOpenOutcome(page, fixture.linkUrl), { + .poll(() => readLinkOpenOutcome(client!, fixture.linkUrl), { timeout: 30_000, message: 'the link open produced neither a refusal nor a page' }) .not.toBe('pending') - expect(await readLinkOpenOutcome(page, fixture.linkUrl)).toBe('refused') + expect(await readLinkOpenOutcome(client, fixture.linkUrl)).toBe('refused') // The workspace must still be the local one, or the refusal above proved nothing. expect( await page.evaluate(() => window.__store?.getState().activeWorkspaceExecutionHostId ?? null) ).toBe(LOCAL_EXECUTION_HOST_ID) - // Nothing rendered here, nothing new on the host, and nobody fetched the link anywhere. - expect(await readLocalBrowserViewUrls(page)).toHaveLength(0) - expect(await readHostBrowserUrls(host, worktreeId)).toEqual(hostUrlsBefore) + // Nothing new rendered here, nothing new on the host, and nobody fetched the link anywhere. + expect(await readOwnedPageUrls(client.app, fixture.linkUrl)).toHaveLength(0) + expect(await readHostBrowserPageUrls(host.client, worktreeSelector)).toEqual(hostPagesBefore) expect(fixture.linkLoadCount()).toBe(linkLoadsBefore) } finally { if (client) { diff --git a/tests/e2e/terminal-duplicate-pty-renderer-reveal.spec.ts b/tests/e2e/terminal-duplicate-pty-renderer-reveal.spec.ts index 1792076de56..2eec7356f65 100644 --- a/tests/e2e/terminal-duplicate-pty-renderer-reveal.spec.ts +++ b/tests/e2e/terminal-duplicate-pty-renderer-reveal.spec.ts @@ -5,6 +5,12 @@ import type { ElectronApplication, Page } from '@stablyai/playwright-test' import type { TerminalLayoutSnapshot } from '../../src/shared/terminal-tab-types' import { DEFAULT_LOCAL_ORCA_PROFILE_ID } from '../../src/shared/orca-profiles' import { test, expect } from './helpers/orca-app' +import { + findMarkerFrame, + readActiveScreen, + readRenderedAltScreenFrame, + type ActiveScreen +} from './helpers/alt-screen-frame' import { attachRepoAndOpenTerminal, createRestartSession } from './helpers/orca-restart' import { stageNodeScriptForTerminal } from './helpers/run-node-script-in-terminal' import { @@ -124,18 +130,7 @@ async function readRendererOwnership( }, tabId) } -async function readStreamingFrame( - page: Page, - tabId: string, - marker: string -): Promise { - const content = await page.evaluate((tabId) => { - const pane = window.__paneManagers?.get(tabId)?.getPanes?.()[0] - return pane?.serializeAddon?.serialize?.() ?? null - }, tabId) - return parseStreamingFrame(content, marker) -} - +// Viewport-only, so it is the same projection the revealed pane is read with. async function readMainStreamingFrame( page: Page, ptyId: string, @@ -145,17 +140,47 @@ async function readMainStreamingFrame( const snapshot = await window.api.pty.getMainBufferSnapshot(ptyId, { scrollbackRows: 0 }) return snapshot?.data ?? null }, ptyId) - return parseStreamingFrame(content, marker) + return findMarkerFrame(content ?? '', marker) } -function parseStreamingFrame(content: string | null, marker: string): number | null { +type RevealFrameDiagnostics = { + bufferType: ActiveScreen['bufferType'] | null + screenFrame: number | null + serializedFrame: number | null + serializedLength: number + markerOffsets: number[] + screenRows: string[] +} + +// Why: if the revealed pane ever fails to converge, whether the freshest marker is on +// screen, only in normal-buffer scrollback, or absent is what separates a stalled +// renderer from stale residue left by the restore replay (STA-5208). +async function readRevealFrameDiagnostics( + page: Page, + tabId: string, + marker: string +): Promise { + const screen = await readActiveScreen(page, tabId) + const serialized = + (await page.evaluate((tabId) => { + // Same pane resolution as readActiveScreen, so both halves describe one pane. + const manager = window.__paneManagers?.get(tabId) + const pane = manager?.getActivePane?.() ?? manager?.getPanes?.()[0] + return pane?.serializeAddon?.serialize?.() ?? null + }, tabId)) ?? '' const prefix = `${marker} frame ` - const start = content?.lastIndexOf(prefix) ?? -1 - if (!content || start < 0) { - return null + const markerOffsets: number[] = [] + for (let at = serialized.indexOf(prefix); at >= 0; at = serialized.indexOf(prefix, at + 1)) { + markerOffsets.push(at) + } + return { + bufferType: screen?.bufferType ?? null, + screenFrame: screen ? findMarkerFrame(screen.rows.join('\n'), marker) : null, + serializedFrame: findMarkerFrame(serialized, marker), + serializedLength: serialized.length, + markerOffsets, + screenRows: screen?.rows ?? [] } - const digits = content.slice(start + prefix.length).match(/^\d+/)?.[0] - return digits ? Number(digits) : null } test('repairs duplicate persisted PTY renderers before streaming tab reveal', async (// oxlint-disable-next-line no-empty-pattern -- this restart test owns its Electron launches. @@ -231,13 +256,8 @@ test('repairs duplicate persisted PTY renderers before streaming tab reveal', as await expect .poll(() => getActiveTabId(secondLaunch.page), { timeout: 10_000 }) .toBe(restoredTabId) - await expect - .poll(() => readStreamingFrame(secondLaunch.page, restoredTabId, marker), { - timeout: 20_000, - message: 'Revealed renderer did not catch up to hidden authoritative output' - }) - .toBeGreaterThanOrEqual(hiddenFrame) - + // Ownership first: the frame is read off the single repaired pane, so the repair has + // to have settled before that read means anything. await expect .poll(() => readRendererOwnership(secondLaunch.page, restoredTabId), { timeout: 10_000 }) .toEqual({ @@ -247,10 +267,46 @@ test('repairs duplicate persisted PTY renderers before streaming tab reveal', as ptyBindingCount: 1, uniquePtyCount: 1 }) - await testInfo.attach('duplicate-pty-renderer-after-reveal.png', { - body: await secondLaunch.page.screenshot(), - contentType: 'image/png' - }) + let revealFailure: unknown = null + try { + await expect + .poll(() => readRenderedAltScreenFrame(secondLaunch.page, restoredTabId, marker), { + timeout: 20_000, + message: 'Revealed renderer did not catch up to hidden authoritative output' + }) + .toBeGreaterThanOrEqual(hiddenFrame) + // The fixture enters the alternate buffer once at startup and repaints in place, so a + // revealed pane parked on the normal buffer is painting the TUI into scrollback. + const revealedScreen = await readActiveScreen(secondLaunch.page, restoredTabId) + expect( + revealedScreen?.bufferType, + 'revealed pane was missing or painting outside the alternate buffer' + ).toBe('alternate') + } catch (error) { + // Diagnostics are more page reads, so a dead page has to degrade to a note in the + // attachment rather than replacing the failure the attachment exists to explain. + revealFailure = error + const diagnostics = await readRevealFrameDiagnostics( + secondLaunch.page, + restoredTabId, + marker + ).catch((diagnosticsError: unknown) => ({ diagnosticsError: String(diagnosticsError) })) + await testInfo.attach('duplicate-pty-reveal-frame-diagnostics.json', { + body: JSON.stringify(diagnostics, null, 2), + contentType: 'application/json' + }) + } + // Evidence for both outcomes; a capture that fails must not become the verdict. + const screenshot = await secondLaunch.page.screenshot().catch(() => null) + if (screenshot) { + await testInfo.attach('duplicate-pty-renderer-after-reveal.png', { + body: screenshot, + contentType: 'image/png' + }) + } + if (revealFailure) { + throw revealFailure + } } finally { tui.cleanup() if (secondApp) { diff --git a/tests/e2e/terminal-hidden-tui-visual-restore.spec.ts b/tests/e2e/terminal-hidden-tui-visual-restore.spec.ts index a78f4e81fa1..2bbba4b9117 100644 --- a/tests/e2e/terminal-hidden-tui-visual-restore.spec.ts +++ b/tests/e2e/terminal-hidden-tui-visual-restore.spec.ts @@ -2,6 +2,7 @@ import type { Page, TestInfo } from '@stablyai/playwright-test' import { randomUUID } from 'node:crypto' import { mkdirSync, rmSync, writeFileSync } from 'node:fs' import path from 'node:path' +import { alternateScreenFixtureScript } from './alternate-screen-fixture-script' import { test, expect } from './helpers/orca-app' import { runNodeScriptInTerminal } from './helpers/run-node-script-in-terminal' import { @@ -87,7 +88,7 @@ function writeHiddenFrameScript(scriptPath: string, runId: string): void { mkdirSync(path.dirname(scriptPath), { recursive: true }) writeFileSync( scriptPath, - `setTimeout(() => process.stdout.write(${JSON.stringify(frames.join(''))}), ${HIDDEN_FRAME_SCRIPT_DELAY_MS})\n` + alternateScreenFixtureScript(frames.join(''), HIDDEN_FRAME_SCRIPT_DELAY_MS) ) } @@ -246,59 +247,63 @@ test.describe('Hidden terminal TUI visual restore', () => { const scriptPath = path.join(testRepoPath, `.orca-hidden-tui-visual-${runId}.mjs`) writeHiddenFrameScript(scriptPath, runId) await resetHiddenDebug(orcaPage) - await writeHiddenFrames(orcaPage, hiddenPane.ptyId, scriptPath) - await resetHiddenDebug(orcaPage) + try { + await writeHiddenFrames(orcaPage, hiddenPane.ptyId, scriptPath) + await resetHiddenDebug(orcaPage) - // Why: hidden-delivery gate contract — the bulk TUI frames must be - // withheld in main (dropped after model ingestion), not delivered and - // skipped renderer-side. - await expect - .poll(() => readMainHiddenDeliveryDroppedChars(orcaPage), { - timeout: 10_000, - message: 'visually rich hidden TUI output was not withheld from the renderer' - }) - .toBeGreaterThan(1024) - await expect - .poll(() => readMainSnapshotSource(orcaPage, hiddenPane.ptyId!), { - timeout: 10_000, - message: 'visually rich hidden TUI source did not come from headless model' - }) - .toBe('headless') + // Why: hidden-delivery gate contract — the bulk TUI frames must be + // withheld in main (dropped after model ingestion), not delivered and + // skipped renderer-side. + await expect + .poll(() => readMainHiddenDeliveryDroppedChars(orcaPage), { + timeout: 10_000, + message: 'visually rich hidden TUI output was not withheld from the renderer' + }) + .toBeGreaterThan(1024) + await expect + .poll(() => readMainSnapshotSource(orcaPage, hiddenPane.ptyId!), { + timeout: 10_000, + message: 'visually rich hidden TUI source did not come from headless model' + }) + .toBe('headless') - await switchToWorktree(orcaPage, secondWorktreeId) - await ensureTerminalVisible(orcaPage) - await waitForActiveTerminalManager(orcaPage, 30_000) + await switchToWorktree(orcaPage, secondWorktreeId) + await ensureTerminalVisible(orcaPage) + await waitForActiveTerminalManager(orcaPage, 30_000) - await expect - .poll(() => getTerminalContent(orcaPage, 12_000), { - timeout: 10_000, - message: 'hidden TUI final frame did not restore when the workspace became visible' - }) - .toContain(finalMarker) + await expect + .poll(() => getTerminalContent(orcaPage, 12_000), { + timeout: 10_000, + message: 'hidden TUI final frame did not restore when the workspace became visible' + }) + .toContain(finalMarker) - const content = await getTerminalContent(orcaPage, 12_000) - expect(content).toContain(`Frame 024`) - expect(content).toContain('╭') - expect(content).toContain('├') - expect(content).toContain('█') - expect(content).not.toContain('Orca skipped hidden terminal output') - await expect - .poll(() => readTuiCursorState(orcaPage), { - timeout: 5_000, - message: 'restored TUI cursor stayed hidden after final frame' - }) - .toMatchObject({ - hidden: false, - initialized: true - }) + const content = await getTerminalContent(orcaPage, 12_000) + expect(content).toContain(`Frame 024`) + expect(content).toContain('╭') + expect(content).toContain('├') + expect(content).toContain('█') + expect(content).not.toContain('Orca skipped hidden terminal output') + await expect + .poll(() => readTuiCursorState(orcaPage), { + timeout: 5_000, + message: 'restored TUI cursor stayed hidden after final frame' + }) + .toMatchObject({ + hidden: false, + initialized: true + }) - const screenshotPath = testInfo.outputPath('hidden-tui-restore-final.png') - await orcaPage.screenshot({ path: screenshotPath, fullPage: true }) - await testInfo.attach('hidden-tui-restore-final.png', { - path: screenshotPath, - contentType: 'image/png' - }) - rmSync(scriptPath, { force: true }) + const screenshotPath = testInfo.outputPath('hidden-tui-restore-final.png') + await orcaPage.screenshot({ path: screenshotPath, fullPage: true }) + await testInfo.attach('hidden-tui-restore-final.png', { + path: screenshotPath, + contentType: 'image/png' + }) + } finally { + await sendToTerminal(orcaPage, hiddenPane.ptyId, '\x03').catch(() => undefined) + rmSync(scriptPath, { force: true }) + } }) test('keeps newer live output correct after plain hidden output restores', async ({ @@ -480,6 +485,7 @@ test.describe('Hidden terminal TUI visual restore', () => { contentType: 'image/png' }) } finally { + await sendToTerminal(orcaPage, hiddenPane.ptyId, '\x03').catch(() => undefined) rmSync(scriptPath, { force: true }) } }) diff --git a/tests/e2e/terminal-hidden-view-parking.spec.ts b/tests/e2e/terminal-hidden-view-parking.spec.ts index 19fc02e8486..9aba68407f5 100644 --- a/tests/e2e/terminal-hidden-view-parking.spec.ts +++ b/tests/e2e/terminal-hidden-view-parking.spec.ts @@ -2,6 +2,7 @@ import type { Page, TestInfo } from '@stablyai/playwright-test' import { randomUUID } from 'node:crypto' import { mkdirSync, rmSync, writeFileSync } from 'node:fs' import path from 'node:path' +import { alternateScreenFixtureScript } from './alternate-screen-fixture-script' import { test, expect } from './helpers/orca-app' import { runNodeScriptInTerminal } from './helpers/run-node-script-in-terminal' import { @@ -18,6 +19,7 @@ import { waitForPaneIdentitySnapshot } from './helpers/terminal' import { parkHiddenTabBehindDecoy, waitForTabParked } from './helpers/terminal-hidden-parking' +import { waitForPtyShellEcho } from './terminal-pty-readiness' import { TERMINAL_TAB_PARK_FLIP_BURST_WINDOW_MS } from '../../src/renderer/src/components/terminal-pane/terminal-park-verdict-flip-telemetry' // Why: the parking wiring registers this handle (dev/exposeStore builds only) @@ -73,7 +75,7 @@ function writeParkedFrameScript(scriptPath: string, runId: string): void { mkdirSync(path.dirname(scriptPath), { recursive: true }) writeFileSync( scriptPath, - `setTimeout(() => process.stdout.write(${JSON.stringify(frames.join(''))}), ${PARKED_FRAME_SCRIPT_DELAY_MS})\n` + alternateScreenFixtureScript(frames.join(''), PARKED_FRAME_SCRIPT_DELAY_MS) ) } @@ -103,12 +105,7 @@ function cycleReferenceFrame(runId: string): string { function writeCycleReferenceScript(scriptPath: string, runId: string): void { mkdirSync(path.dirname(scriptPath), { recursive: true }) - // Paint the frame once, then hold the process open so the alt-screen TUI - // stays on screen (and the parkable PTY session stays alive) across cycles. - writeFileSync( - scriptPath, - `process.stdout.write(${JSON.stringify(cycleReferenceFrame(runId))}); setInterval(() => {}, 1000)\n` - ) + writeFileSync(scriptPath, alternateScreenFixtureScript(cycleReferenceFrame(runId))) } // Why: serialize() re-emits the buffer with cursor-restore trailer sequences @@ -335,6 +332,17 @@ test.describe('Terminal hidden view parking', () => { expect(content).toContain('█') expect(content).not.toContain('Orca skipped hidden terminal output') + // Why: the fixture TUI still owns the PTY foreground after the reveal, so + // interrupt it and wait for the shell to take input back before probing. + await sendToTerminal(orcaPage, tabAPtyId, '\x03') + // Why rethrow: the readiness failure reads as a dead shell, but the only new + // dependency here is Ctrl-C reaching the foreground TUI (ConPTY translates it). + await waitForPtyShellEcho(orcaPage, tabAPtyId, 15_000).catch((error: unknown) => { + throw new Error( + `Ctrl-C did not hand the PTY back from the fixture TUI: ${error instanceof Error ? error.message : String(error)}` + ) + }) + // Why: the typed marker only appears joined in command *output*, so this // proves the revealed terminal accepts input end-to-end, not just echo. const typedMarker = `PARKED_TYPED_OK_${runId}` diff --git a/tests/e2e/terminal-osc8-cold-park-restore.spec.ts b/tests/e2e/terminal-osc8-cold-park-restore.spec.ts index 8a31afe83da..fddcb24b23d 100644 --- a/tests/e2e/terminal-osc8-cold-park-restore.spec.ts +++ b/tests/e2e/terminal-osc8-cold-park-restore.spec.ts @@ -1,6 +1,8 @@ import { randomUUID } from 'node:crypto' import type { Page } from '@stablyai/playwright-test' +import { alternateScreenFixtureScript } from './alternate-screen-fixture-script' import { expect, test } from './helpers/orca-app' +import { stageNodeScriptForTerminal } from './helpers/run-node-script-in-terminal' import { parkHiddenTabBehindDecoy } from './helpers/terminal-hidden-parking' import { ensureTerminalVisible, @@ -136,48 +138,56 @@ test('restores and opens an OSC 8 link after its terminal is cold-parked', async const label = `#${randomUUID().slice(0, 6)}` const url = `https://example.com/orca-osc8-${randomUUID()}` const linkedOutput = `\x1b[?1049h\x1b[2J\x1b[H\x1b]8;id=cold-park;${url}\x1b\\${label}\x1b]8;;\x1b\\\n` - await sendToTerminal( - orcaPage, - ptyId, - `${nodeTerminalCommand(['-e', `process.stdout.write(${JSON.stringify(linkedOutput)})`])}\r` - ) - await expect.poll(() => getTerminalContent(orcaPage, 4_000)).toContain(label) - - const baselineProbe = await locateLink(orcaPage, label) - await orcaPage.mouse.move(baselineProbe.clientX, baselineProbe.clientY) - await expect - .poll(() => readLinkState(orcaPage, tabId, label)) - .toMatchObject({ - bufferType: 'alternate', - serializedUri: true, - underlined: true, - uri: url - }) - - await parkHiddenTabBehindDecoy(orcaPage, worktreeId, tabId, { - parkDelayMs: PARKING_DELAY_MS + // Why staged rather than `node -e`: PowerShell mangles the escapes (#8521), and it + // keeps the label out of the command line so the readiness poll below cannot be + // satisfied by the shell's own echo. `staged.command` is bypassed because it runs a + // bare `node`; nodeTerminalCommand pins process.execPath for Windows CI's PATH. + const staged = stageNodeScriptForTerminal(alternateScreenFixtureScript(linkedOutput), { + prefix: 'orca-osc8-cold-park' }) - await activateTerminalTab(orcaPage, tabId) - await expect.poll(() => getTerminalContent(orcaPage, 4_000)).toContain(label) + try { + await sendToTerminal(orcaPage, ptyId, `${nodeTerminalCommand([staged.scriptPath])}\r`) + await expect.poll(() => getTerminalContent(orcaPage, 4_000)).toContain(label) - const restoredProbe = await locateLink(orcaPage, label) - await orcaPage.mouse.move(restoredProbe.clientX, restoredProbe.clientY) - await expect - .poll(() => readLinkState(orcaPage, tabId, label)) - .toMatchObject({ - bufferType: 'alternate', - serializedUri: true, - underlined: true, - uri: url + const baselineProbe = await locateLink(orcaPage, label) + await orcaPage.mouse.move(baselineProbe.clientX, baselineProbe.clientY) + await expect + .poll(() => readLinkState(orcaPage, tabId, label)) + .toMatchObject({ + bufferType: 'alternate', + serializedUri: true, + underlined: true, + uri: url + }) + + await parkHiddenTabBehindDecoy(orcaPage, worktreeId, tabId, { + parkDelayMs: PARKING_DELAY_MS }) + await activateTerminalTab(orcaPage, tabId) + await expect.poll(() => getTerminalContent(orcaPage, 4_000)).toContain(label) - const isMac = await orcaPage.evaluate(() => navigator.userAgent.includes('Mac')) - const modifier = isMac ? 'Meta' : 'Control' - await orcaPage.keyboard.down(modifier) - await orcaPage.mouse.down() - await orcaPage.mouse.up() - await orcaPage.keyboard.up(modifier) - await expect - .poll(async () => (await getBrowserTabs(orcaPage, worktreeId)).some((tab) => tab.url === url)) - .toBe(true) + const restoredProbe = await locateLink(orcaPage, label) + await orcaPage.mouse.move(restoredProbe.clientX, restoredProbe.clientY) + await expect + .poll(() => readLinkState(orcaPage, tabId, label)) + .toMatchObject({ + bufferType: 'alternate', + serializedUri: true, + underlined: true, + uri: url + }) + + const isMac = await orcaPage.evaluate(() => navigator.userAgent.includes('Mac')) + const modifier = isMac ? 'Meta' : 'Control' + await orcaPage.keyboard.down(modifier) + await orcaPage.mouse.down() + await orcaPage.mouse.up() + await orcaPage.keyboard.up(modifier) + await expect + .poll(async () => (await getBrowserTabs(orcaPage, worktreeId)).some((tab) => tab.url === url)) + .toBe(true) + } finally { + await sendToTerminal(orcaPage, ptyId, '\x03').catch(() => undefined) + staged.cleanup() + } }) diff --git a/tests/e2e/update-install-renderer-checkpoint-recovery.spec.ts b/tests/e2e/update-install-renderer-checkpoint-recovery.spec.ts index 9a521926f19..b1bb0c0f997 100644 --- a/tests/e2e/update-install-renderer-checkpoint-recovery.spec.ts +++ b/tests/e2e/update-install-renderer-checkpoint-recovery.spec.ts @@ -1,7 +1,14 @@ import path from 'node:path' import { test, expect } from './helpers/orca-app' -const CHECKPOINT_ERROR = 'Renderer shutdown checkpoint was not completed.' +// Only the prefix is contract: the checkpoint error appends the swallowed persist +// cause (STA-5505), whose wording belongs to whatever threw. +const CHECKPOINT_ERROR_PREFIX = 'Renderer shutdown checkpoint was not completed: ' + +// The null url is injected, never hydrated: the desktop session schema drops such a row, and +// no other arrival path carries browserUrlHistory at all (paired web reads it unvalidated but +// has no producer — STA-5668 follow-up). It is just a deterministic snapshot-build failure. +const CORRUPT_HISTORY_ENTRY = { url: null, title: 'corrupt persisted history', lastVisitedAt: 0 } test('recovers update install from a corrupt clean session but preserves dirty drafts', async ({ orcaPage, @@ -15,16 +22,14 @@ test('recovers update install from a corrupt clean session but preserves dirty d }) const dirtyResult = await orcaPage.evaluate( - async ({ filePath, worktreeId }) => { + async ({ filePath, worktreeId, corruptEntry }) => { const store = window.__store if (!store) { throw new Error('window.__store is not available') } const state = store.getState() const originalHistory = state.browserUrlHistory - state.browserUrlHistory = [ - { url: null, title: 'corrupt persisted history', lastVisitedAt: 0 } - ] as unknown as typeof state.browserUrlHistory + state.browserUrlHistory = [corruptEntry] as unknown as typeof state.browserUrlHistory const fileId = state.openFile({ filePath, relativePath: 'checkpoint-draft.txt', @@ -48,22 +53,24 @@ test('recovers update install from a corrupt clean session but preserves dirty d }, { filePath: path.join(testRepoPath, 'checkpoint-draft.txt'), - worktreeId: await orcaPage.evaluate(() => window.__store?.getState().activeWorktreeId ?? '') + worktreeId: await orcaPage.evaluate(() => window.__store?.getState().activeWorktreeId ?? ''), + corruptEntry: CORRUPT_HISTORY_ENTRY } ) - expect(dirtyResult).toBe(CHECKPOINT_ERROR) + expect(dirtyResult).toContain(CHECKPOINT_ERROR_PREFIX) + // Pin the cause to the corrupt row, not just any named failure; only the member name + // survives V8 rewording of "Cannot read properties of null". + expect(dirtyResult).toContain('toLowerCase') - const cleanResult = await orcaPage.evaluate(async () => { + const cleanResult = await orcaPage.evaluate(async (corruptEntry) => { const store = window.__store if (!store) { throw new Error('window.__store is not available') } const state = store.getState() const originalHistory = state.browserUrlHistory - state.browserUrlHistory = [ - { url: null, title: 'corrupt persisted history', lastVisitedAt: 0 } - ] as unknown as typeof state.browserUrlHistory + state.browserUrlHistory = [corruptEntry] as unknown as typeof state.browserUrlHistory try { await window.api.updater.quitAndInstall() return 'continued' @@ -72,7 +79,7 @@ test('recovers update install from a corrupt clean session but preserves dirty d } finally { state.browserUrlHistory = originalHistory } - }) + }, CORRUPT_HISTORY_ENTRY) expect(cleanResult).toBe('continued') expect(fallbackLogs).toHaveLength(1) diff --git a/tests/e2e/worktree-card-downward-drag.spec.ts b/tests/e2e/worktree-card-downward-drag.spec.ts index b5cb685a4a6..8ee77548aee 100644 --- a/tests/e2e/worktree-card-downward-drag.spec.ts +++ b/tests/e2e/worktree-card-downward-drag.spec.ts @@ -78,7 +78,17 @@ async function seedVirtualizedManualWorktrees(page: Page): Promise<{ ...state.worktreesByRepo, [repoId]: [...worktrees, ...seededWorktrees] }, - updateWorktreesMeta: async (updatesByWorktreeId) => { + // Stands in for the real action only to skip its IPC persistence tail: these 60 rows + // are store-only, so persisting them would fail and refetch them away mid-drag. + updateWorktreesMeta: async (batchUpdates) => { + if (batchUpdates.length === 0) { + return + } + // executionHostId is ignored on purpose: every seeded row is host-less, which the + // real action treats as local (worktree-meta-host-match.ts). + const updatesByWorktreeId = new Map( + batchUpdates.map((batchUpdate) => [batchUpdate.worktreeId, batchUpdate.updates]) + ) store.setState((current) => ({ sortEpoch: current.sortEpoch + 1, worktreesByRepo: Object.fromEntries( diff --git a/tests/e2e/worktree-jump-palette-filter.spec.ts b/tests/e2e/worktree-jump-palette-filter.spec.ts index b4841cf059e..9c5d6146ba1 100644 --- a/tests/e2e/worktree-jump-palette-filter.spec.ts +++ b/tests/e2e/worktree-jump-palette-filter.spec.ts @@ -1,4 +1,4 @@ -import type { Page } from '@stablyai/playwright-test' +import type { Locator, Page } from '@stablyai/playwright-test' import { expect, test } from './helpers/orca-app' import { waitForActiveWorktree, waitForSessionReady } from './helpers/store' @@ -132,6 +132,23 @@ async function selectRemoteHost(page: Page, useKeyboard = false): Promise await filterTrigger(page).click() } +async function openComposerFromTypedName(page: Page): Promise { + await openPalette(page) + const input = palette(page).getByPlaceholder(SEARCH_PLACEHOLDER) + await input.fill(`cmd-j-enter-${Date.now()}`) + await expect(palette(page).locator('[cmdk-item][data-value="__create_worktree__"]')).toBeVisible() + + await input.press('Enter') + + const createDialog = page.getByRole('dialog', { name: /Create (Workspace|Worktree)/i }) + await expect(createDialog).toBeVisible() + // Why assert focus: the composer auto-focuses the name field, so Escape always + // lands on an input the user never chose. A page-style "blur the field first" + // handler reachable from here would silently cost a second press. + await expect(createDialog.locator('[data-workspace-name-input="true"]')).toBeFocused() + return createDialog +} + test.describe('Worktree jump-palette filters', () => { test.beforeEach(async ({ orcaPage }) => { await waitForSessionReady(orcaPage) @@ -182,22 +199,33 @@ test.describe('Worktree jump-palette filters', () => { }) test('pressing Enter creates a worktree from a typed name', async ({ orcaPage }) => { - await openPalette(orcaPage) - const input = palette(orcaPage).getByPlaceholder(SEARCH_PLACEHOLDER) - await input.fill(`cmd-j-enter-${Date.now()}`) - await expect( - palette(orcaPage).locator('[cmdk-item][data-value="__create_worktree__"]') - ).toBeVisible() + const createDialog = await openComposerFromTypedName(orcaPage) - await input.press('Enter') - - const createDialog = orcaPage.getByRole('dialog', { name: /Create (Workspace|Worktree)/i }) - await expect(createDialog).toBeVisible() - // Why assert focus first: the composer auto-focuses the name field, so Escape - // always lands on an input the user never chose. A page-style "blur the field - // first" handler here would silently cost a second press. - await expect(createDialog.locator('[data-workspace-name-input="true"]')).toBeFocused() await orcaPage.keyboard.press('Escape') + await expect(createDialog).toBeHidden() }) + + test('Escape closes the composer opened over the Automations page', async ({ orcaPage }) => { + // Why this view: Cmd+J has no view guard, and a page mounted under the palette + // keeps its own capture-phase Escape listener registered. Window capture runs + // before Radix's document capture, so a preventDefault there vetoes dismissal. + await orcaPage.evaluate(() => window.__store?.getState().openAutomationsPage()) + const automationsHeading = orcaPage.getByRole('heading', { name: 'Automations', level: 1 }) + await expect(automationsHeading).toBeVisible() + + const createDialog = await openComposerFromTypedName(orcaPage) + + await orcaPage.keyboard.press('Escape') + + await expect(createDialog).toBeHidden() + // The page declined the press rather than consuming it, so it is still open. + await expect(automationsHeading).toBeVisible() + + // Why a second press: with nothing layered above, the real page chrome must not + // trip the overlay check, or Escape would never close Automations again. + await orcaPage.keyboard.press('Escape') + + await expect(automationsHeading).toBeHidden() + }) }) From 5e19c35dc5ce6f1973e0052b1970c06d16f274c3 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 30 Aug 2026 11:49:00 -0700 Subject: [PATCH 03/59] fix(automations): stop vetoing Escape for overlays the page does not own (STA-5207) (#17431) --- ...AutomationsPage.escape-precedence.test.tsx | 77 +++++++++++++++++++ .../automations/AutomationsPage.tsx | 12 ++- .../automations/automations-page-fixtures.ts | 1 + .../settings/use-settings-page-effects.ts | 19 +---- .../use-skills-page-keyboard-navigation.ts | 24 +----- .../workspace-space/WorkspaceSpacePage.tsx | 19 +---- src/renderer/src/lib/visible-overlay.test.ts | 54 +++++++++++++ src/renderer/src/lib/visible-overlay.ts | 31 ++++++++ 8 files changed, 178 insertions(+), 59 deletions(-) create mode 100644 src/renderer/src/components/automations/AutomationsPage.escape-precedence.test.tsx create mode 100644 src/renderer/src/lib/visible-overlay.test.ts create mode 100644 src/renderer/src/lib/visible-overlay.ts diff --git a/src/renderer/src/components/automations/AutomationsPage.escape-precedence.test.tsx b/src/renderer/src/components/automations/AutomationsPage.escape-precedence.test.tsx new file mode 100644 index 00000000000..df0aa39a9aa --- /dev/null +++ b/src/renderer/src/components/automations/AutomationsPage.escape-precedence.test.tsx @@ -0,0 +1,77 @@ +// @vitest-environment happy-dom + +/** + * The Automations page installs a window capture-phase Escape handler. Capture on + * `window` runs before Radix's document-capture DismissableLayer, which dismisses + * only `if (!event.defaultPrevented)` — so any preventDefault here silently vetoes + * dismissal of a dialog layered above the page (STA-5207). + */ + +import { describe, expect, it, vi } from 'vitest' +import { + installAutomationsPageHarness, + mocks, + renderPage, + settleHostQueries +} from './automations-page-test-harness' + +installAutomationsPageHarness() + +function pressEscape(target: EventTarget): KeyboardEvent { + const event = new KeyboardEvent('keydown', { key: 'Escape', bubbles: true, cancelable: true }) + target.dispatchEvent(event) + return event +} + +async function renderWithCloseSpy(): Promise> { + const closeAutomationsPage = vi.fn() + mocks.state.closeAutomationsPage = closeAutomationsPage + await renderPage() + await settleHostQueries() + return closeAutomationsPage +} + +describe('automations page Escape precedence', () => { + it('closes the page when nothing is layered above it', async () => { + const closeAutomationsPage = await renderWithCloseSpy() + + const event = pressEscape(document.body) + + expect(event.defaultPrevented).toBe(true) + expect(closeAutomationsPage).toHaveBeenCalledTimes(1) + }) + + it('leaves Escape to a store modal opened over the page', async () => { + mocks.state.activeModal = 'new-workspace-composer' + const closeAutomationsPage = await renderWithCloseSpy() + const input = document.createElement('input') + document.body.appendChild(input) + + const event = pressEscape(input) + + expect(event.defaultPrevented).toBe(false) + expect(closeAutomationsPage).not.toHaveBeenCalled() + }) + + it('leaves Escape to a store modal even when focus is on page chrome', async () => { + mocks.state.activeModal = 'worktree-palette' + const closeAutomationsPage = await renderWithCloseSpy() + + const event = pressEscape(document.body) + + expect(event.defaultPrevented).toBe(false) + expect(closeAutomationsPage).not.toHaveBeenCalled() + }) + + it('leaves Escape to a visible overlay that the store does not track', async () => { + const closeAutomationsPage = await renderWithCloseSpy() + const overlay = document.createElement('div') + overlay.setAttribute('role', 'dialog') + document.body.appendChild(overlay) + + const event = pressEscape(document.body) + + expect(event.defaultPrevented).toBe(false) + expect(closeAutomationsPage).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/components/automations/AutomationsPage.tsx b/src/renderer/src/components/automations/AutomationsPage.tsx index 1d1c04fe578..f2eabdf346a 100644 --- a/src/renderer/src/components/automations/AutomationsPage.tsx +++ b/src/renderer/src/components/automations/AutomationsPage.tsx @@ -8,6 +8,7 @@ import { useAppStore } from '@/store' import { getAgentCatalog } from '@/lib/agent-catalog' import { useRepoMap, useWorktreeMap } from '@/store/selectors' import { activateAndRevealWorktree } from '@/lib/worktree-activation' +import { hasVisibleOverlay } from '@/lib/visible-overlay' import type { Automation, AutomationCreateInput, @@ -208,6 +209,7 @@ export default function AutomationsPage(): React.JSX.Element { const openSettingsPage = useAppStore((s) => s.openSettingsPage) const openSettingsTarget = useAppStore((s) => s.openSettingsTarget) const closeAutomationsPage = useAppStore((s) => s.closeAutomationsPage) + const activeModal = useAppStore((s) => s.activeModal) const sshConnectionStates = useAppStore((s) => s.sshConnectionStates) const sshTargetLabels = useAppStore((s) => s.sshTargetLabels) const runtimeEnvironments = useAppStore((s) => s.runtimeEnvironments) @@ -2497,7 +2499,9 @@ export default function AutomationsPage(): React.JSX.Element { } useEffect(() => { - if (createOpen || deleteTarget || externalDeleteTarget) { + // Why: a modal layered over the page owns Esc; this listener is capture-phase on + // window, so preventDefault here would veto the modal's own dismissal. + if (createOpen || deleteTarget || externalDeleteTarget || activeModal !== 'none') { return } @@ -2511,6 +2515,11 @@ export default function AutomationsPage(): React.JSX.Element { return } + // Why: popovers and menus live outside the store's modal registry; they own Esc too. + if (hasVisibleOverlay()) { + return + } + // Why: fields that clear their own value on Escape consume this press; // blurring here would drop focus and let the next Escape close the page. if (target.dataset.escapeClearsValue === 'true') { @@ -2554,6 +2563,7 @@ export default function AutomationsPage(): React.JSX.Element { window.addEventListener('keydown', onKeyDown, { capture: true }) return () => window.removeEventListener('keydown', onKeyDown, { capture: true }) }, [ + activeModal, closeAutomationsPage, createOpen, deleteTarget, diff --git a/src/renderer/src/components/automations/automations-page-fixtures.ts b/src/renderer/src/components/automations/automations-page-fixtures.ts index 7dfe4058ad4..16c28c28145 100644 --- a/src/renderer/src/components/automations/automations-page-fixtures.ts +++ b/src/renderer/src/components/automations/automations-page-fixtures.ts @@ -217,6 +217,7 @@ export function makeStoreState(): AutomationsPageStoreFixtures { openSettingsPage: noop, openSettingsTarget: noop, closeAutomationsPage: noop, + activeModal: 'none', sshConnectionStates: new Map(), sshTargetLabels: new Map(), removedSshTargetLabels: new Map(), diff --git a/src/renderer/src/components/settings/use-settings-page-effects.ts b/src/renderer/src/components/settings/use-settings-page-effects.ts index 7a315c6fdc3..a913a181391 100644 --- a/src/renderer/src/components/settings/use-settings-page-effects.ts +++ b/src/renderer/src/components/settings/use-settings-page-effects.ts @@ -6,6 +6,7 @@ import { resolveAppearanceAccordionDeepLink } from './appearance-usage-percentag import { registerWindowCloseGuard } from '../window-close-request-coordinator' import { isIntentionalAppRestartInProgress } from '@/lib/updater-beforeunload' import { getShortcutPlatform } from '@/lib/shortcut-platform' +import { hasVisibleOverlay } from '@/lib/visible-overlay' import { translate } from '@/i18n/i18n' import { getSettingsTargetHostSelection, @@ -80,24 +81,6 @@ export function useSettingsPageEffects( }, [refreshModelStates, setVoiceModelStatesLoading, showDesktopOnlySettings]) useEffect(() => { - const hasVisibleOverlay = (): boolean => - Array.from( - document.querySelectorAll('[role="dialog"], [role="listbox"], [role="menu"]') - ).some((element) => { - if (!(element instanceof HTMLElement)) { - return false - } - if (element.closest('[aria-hidden="true"]')) { - return false - } - const style = window.getComputedStyle(element) - return ( - style.display !== 'none' && - style.visibility !== 'hidden' && - element.getClientRects().length > 0 - ) - }) - const handleKeyDown = (event: KeyboardEvent): void => { if (event.key !== 'Escape' || event.defaultPrevented) { return diff --git a/src/renderer/src/components/skills/use-skills-page-keyboard-navigation.ts b/src/renderer/src/components/skills/use-skills-page-keyboard-navigation.ts index f3518b16375..9f67c5313d8 100644 --- a/src/renderer/src/components/skills/use-skills-page-keyboard-navigation.ts +++ b/src/renderer/src/components/skills/use-skills-page-keyboard-navigation.ts @@ -1,5 +1,6 @@ import { useEffect } from 'react' import type { SkillsPageView } from './skills-page-view' +import { hasVisibleOverlay } from '@/lib/visible-overlay' type UseSkillsPageKeyboardNavigationOptions = { closeSkillsPage: () => void @@ -17,33 +18,12 @@ export function useSkillsPageKeyboardNavigation({ view }: UseSkillsPageKeyboardNavigationOptions): void { useEffect(() => { - const hasVisibleOverlay = (): boolean => - Array.from( - document.querySelectorAll('[role="dialog"], [role="listbox"], [role="menu"]') - ).some((element) => { - if (!(element instanceof HTMLElement)) { - return false - } - if (element.closest('[aria-hidden="true"]')) { - return false - } - if (element.closest('[data-skills-page-list="true"]')) { - return false - } - const style = window.getComputedStyle(element) - return ( - style.display !== 'none' && - style.visibility !== 'hidden' && - element.getClientRects().length > 0 - ) - }) - const handleKeyDown = (event: KeyboardEvent): void => { if (event.key !== 'Escape') { return } // Why: menus and dialogs own Escape before page-level navigation. - if (hasVisibleOverlay()) { + if (hasVisibleOverlay({ ignoreSelector: '[data-skills-page-list="true"]' })) { return } const target = event.target diff --git a/src/renderer/src/components/workspace-space/WorkspaceSpacePage.tsx b/src/renderer/src/components/workspace-space/WorkspaceSpacePage.tsx index ccff6b69572..eb76edc1629 100644 --- a/src/renderer/src/components/workspace-space/WorkspaceSpacePage.tsx +++ b/src/renderer/src/components/workspace-space/WorkspaceSpacePage.tsx @@ -4,30 +4,13 @@ import { Badge } from '../ui/badge' import { Button } from '../ui/button' import { WorkspaceSpaceManagerPanel } from '../status-bar/WorkspaceSpaceManagerPanel' import { useAppStore } from '../../store' +import { hasVisibleOverlay } from '@/lib/visible-overlay' import { translate } from '@/i18n/i18n' export default function WorkspaceSpacePage(): React.JSX.Element { const closeSpacePage = useAppStore((state) => state.closeSpacePage) useEffect(() => { - const hasVisibleOverlay = (): boolean => - Array.from( - document.querySelectorAll('[role="dialog"], [role="listbox"], [role="menu"]') - ).some((element) => { - if (!(element instanceof HTMLElement)) { - return false - } - if (element.closest('[aria-hidden="true"]')) { - return false - } - const style = window.getComputedStyle(element) - return ( - style.display !== 'none' && - style.visibility !== 'hidden' && - element.getClientRects().length > 0 - ) - }) - const handleKeyDown = (event: KeyboardEvent): void => { if (event.key !== 'Escape') { return diff --git a/src/renderer/src/lib/visible-overlay.test.ts b/src/renderer/src/lib/visible-overlay.test.ts new file mode 100644 index 00000000000..b6cf072d2d9 --- /dev/null +++ b/src/renderer/src/lib/visible-overlay.test.ts @@ -0,0 +1,54 @@ +// @vitest-environment happy-dom + +import { afterEach, describe, expect, it } from 'vitest' +import { hasVisibleOverlay } from './visible-overlay' + +afterEach(() => { + document.body.innerHTML = '' +}) + +function mount(html: string): void { + document.body.innerHTML = html +} + +describe('hasVisibleOverlay', () => { + it('is false with no overlay on screen', () => { + mount('
page chrome
') + + expect(hasVisibleOverlay()).toBe(false) + }) + + it.each(['dialog', 'alertdialog', 'listbox', 'menu'])('sees a visible %s', (role) => { + mount(`
`) + + expect(hasVisibleOverlay()).toBe(true) + }) + + it('ignores an overlay inside an aria-hidden subtree', () => { + mount('') + + expect(hasVisibleOverlay()).toBe(false) + }) + + it('ignores a display:none overlay', () => { + mount('
') + + expect(hasVisibleOverlay()).toBe(false) + }) + + it('ignores a visibility:hidden overlay', () => { + mount('
') + + expect(hasVisibleOverlay()).toBe(false) + }) + + it('ignores overlays inside ignoreSelector but not their siblings', () => { + mount('
') + + expect(hasVisibleOverlay({ ignoreSelector: '[data-page-list="true"]' })).toBe(true) + + document.querySelector('[role="menu"]')?.remove() + + expect(hasVisibleOverlay({ ignoreSelector: '[data-page-list="true"]' })).toBe(false) + }) +}) diff --git a/src/renderer/src/lib/visible-overlay.ts b/src/renderer/src/lib/visible-overlay.ts new file mode 100644 index 00000000000..19a8315cccf --- /dev/null +++ b/src/renderer/src/lib/visible-overlay.ts @@ -0,0 +1,31 @@ +const OVERLAY_SELECTOR = '[role="dialog"], [role="alertdialog"], [role="listbox"], [role="menu"]' + +type VisibleOverlayOptions = { + /** Overlays inside a match are treated as page content, not as a layer above it. */ + ignoreSelector?: string +} + +/** + * Whether a dialog, alert dialog, listbox, or menu is on screen. Page-level Escape + * handlers ask this before acting: the overlay owns the first Escape, and a page + * that preventDefaults instead vetoes the overlay's own dismissal. + */ +export function hasVisibleOverlay(options?: VisibleOverlayOptions): boolean { + return Array.from(document.querySelectorAll(OVERLAY_SELECTOR)).some((element) => { + if (!(element instanceof HTMLElement)) { + return false + } + if (element.closest('[aria-hidden="true"]')) { + return false + } + if (options?.ignoreSelector && element.closest(options.ignoreSelector)) { + return false + } + const style = window.getComputedStyle(element) + return ( + style.display !== 'none' && + style.visibility !== 'hidden' && + element.getClientRects().length > 0 + ) + }) +} From fd52e942bdf8888169c802ce22c2786615ba8b07 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 30 Aug 2026 11:49:12 -0700 Subject: [PATCH 04/59] fix(tasks): keep the remembered GitHub scroll offset instead of clobbering it (STA-5949) (#17433) --- src/renderer/src/components/TaskPage.tsx | 74 ++--- ...sk-page-github-list-scroll-restore.test.ts | 267 ++++++++++++++++++ .../github/github-list-scroll-restore.ts | 112 ++++++++ .../github/github-work-item-table.tsx | 23 +- .../hooks/use-task-page-github-list-resume.ts | 204 ------------- .../hooks/use-task-page-github-list-state.ts | 4 + tests/e2e/tasks-page.spec.ts | 23 +- 7 files changed, 435 insertions(+), 272 deletions(-) create mode 100644 src/renderer/src/components/task-page-github-list-scroll-restore.test.ts create mode 100644 src/renderer/src/components/task-page/github/github-list-scroll-restore.ts delete mode 100644 src/renderer/src/components/task-page/hooks/use-task-page-github-list-resume.ts diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index 06cd8e745c9..0ed764d16e8 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -80,6 +80,7 @@ import type { TaskPageJiraFiltersProps } from '@/components/task-page/chrome/tas import type { TaskPageGitlabFiltersProps } from '@/components/task-page/chrome/task-page-gitlab-filters' import type { GithubDetailHostProps } from '@/components/task-page/github/github-detail-host' import type { GithubWorkItemTableProps } from '@/components/task-page/github/github-work-item-table' +import { startGitHubListScrollRestore } from '@/components/task-page/github/github-list-scroll-restore' import type { GitlabWorkItemListProps } from '@/components/task-page/gitlab/gitlab-work-item-list' import type { JiraIssueListHostProps } from '@/components/task-page/jira/jira-issue-list-host' import type { NewGithubIssueDialogProps } from '@/components/task-page/dialogs/new-github-issue-dialog' @@ -407,6 +408,7 @@ export default function TaskPage(): React.JSX.Element { githubListScrollRef, githubListScrollTopRef, pendingGithubScrollRestoreRef, + githubRestoreScrollWriteRef, paginationLoading, setPaginationLoading, loadingTargetPage, @@ -535,64 +537,24 @@ export default function TaskPage(): React.JSX.Element { : null useLayoutEffect(() => { - const scrollTop = pendingGithubScrollRestoreRef.current - const scrollElement = githubListScrollRef.current - if (scrollTop === null || !scrollElement || !pages[currentPage]) { + const target = pendingGithubScrollRestoreRef.current + if (target === null || !pages[currentPage]) { return } - let frame: number | null = null - let timeout: number | null = null - let observer: ResizeObserver | null = null - const clearScheduledRestore = (): void => { - if (frame !== null) { - window.cancelAnimationFrame(frame) - frame = null - } - if (timeout !== null) { - window.clearTimeout(timeout) - timeout = null - } - observer?.disconnect() - } - const restore = (): void => { - const committedScrollElement = githubListScrollRef.current - if (!committedScrollElement || pendingGithubScrollRestoreRef.current !== scrollTop) { - return - } - committedScrollElement.scrollTop = scrollTop - githubListScrollTopRef.current = scrollTop - taskListPositionRef.current = { - contextKey: githubResumeContextKey, - page: currentPage, - scrollTop - } - if (Math.abs(committedScrollElement.scrollTop - scrollTop) < 1) { - pendingGithubScrollRestoreRef.current = null - clearScheduledRestore() - } - } - observer = new ResizeObserver(restore) - for (const child of scrollElement.children) { - observer.observe(child) - } - restore() - if (pendingGithubScrollRestoreRef.current === scrollTop) { - frame = window.requestAnimationFrame(restore) - timeout = window.setTimeout(() => { - if (pendingGithubScrollRestoreRef.current === scrollTop) { - const committedScrollTop = githubListScrollRef.current?.scrollTop ?? 0 - githubListScrollTopRef.current = committedScrollTop - taskListPositionRef.current = { - contextKey: githubResumeContextKey, - page: currentPage, - scrollTop: committedScrollTop - } - pendingGithubScrollRestoreRef.current = null + return startGitHubListScrollRestore({ + target, + scrollElementRef: githubListScrollRef, + pendingRestoreRef: pendingGithubScrollRestoreRef, + restoreWriteRef: githubRestoreScrollWriteRef, + onScrollTopApplied: (scrollTop) => { + githubListScrollTopRef.current = scrollTop + taskListPositionRef.current = { + contextKey: githubResumeContextKey, + page: currentPage, + scrollTop } - clearScheduledRestore() - }, 5_000) - } - return clearScheduledRestore + } + }) }, [ currentPage, dialogWorkItem, @@ -600,6 +562,7 @@ export default function TaskPage(): React.JSX.Element { pages, githubListScrollTopRef, pendingGithubScrollRestoreRef, + githubRestoreScrollWriteRef, githubListScrollRef.current?.scrollTop, githubListScrollRef ]) @@ -2762,6 +2725,7 @@ export default function TaskPage(): React.JSX.Element { githubResumeContextKey, currentPageRef, pendingGithubScrollRestoreRef, + githubRestoreScrollWriteRef, githubListScrollTopRef, taskListPositionRef, githubTaskGridClass, diff --git a/src/renderer/src/components/task-page-github-list-scroll-restore.test.ts b/src/renderer/src/components/task-page-github-list-scroll-restore.test.ts new file mode 100644 index 00000000000..d094973170f --- /dev/null +++ b/src/renderer/src/components/task-page-github-list-scroll-restore.test.ts @@ -0,0 +1,267 @@ +// @vitest-environment happy-dom + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { + startGitHubListScrollRestore, + supersedeGitHubListScrollRestore, + type GitHubListRestoreWrite +} from './task-page/github/github-list-scroll-restore' + +type FakeResizeObserver = { + targets: Set + notify: () => void +} + +const observers: FakeResizeObserver[] = [] + +/** Notifies every observer watching `target`, mirroring a real content/box resize. */ +function resize(target: Element): void { + for (const observer of observers) { + if (observer.targets.has(target)) { + observer.notify() + } + } +} + +/** Scroll container whose scrollTop clamps to `maxScrollTop`, like a real overflow box. */ +function createScrollList(maxScrollTop: number): { + element: HTMLDivElement + rows: HTMLDivElement + setMaxScrollTop: (next: number) => void +} { + const element = document.createElement('div') + const rows = document.createElement('div') + element.append(rows) + let max = maxScrollTop + let value = 0 + Object.defineProperty(element, 'scrollTop', { + configurable: true, + get: () => value, + set: (next: number) => { + value = Math.max(0, Math.min(next, max)) + } + }) + return { + element, + rows, + setMaxScrollTop: (next: number) => { + max = next + value = Math.min(value, next) + } + } +} + +function ref(current: T): { current: T } { + return { current } +} + +describe('GitHub task list scroll restore', () => { + beforeEach(() => { + vi.useFakeTimers() + observers.length = 0 + vi.stubGlobal( + 'ResizeObserver', + class { + private readonly entry: FakeResizeObserver + constructor(callback: () => void) { + this.entry = { targets: new Set(), notify: callback } + observers.push(this.entry) + } + observe(target: Element): void { + this.entry.targets.add(target) + } + disconnect(): void { + this.entry.targets.clear() + } + } + ) + }) + afterEach(() => { + vi.useRealTimers() + vi.unstubAllGlobals() + }) + + it('lands on the remembered offset once the rows finally paint', () => { + const list = createScrollList(0) + const pendingRestoreRef = ref(360) + const restoreWriteRef = ref(null) + const applied: number[] = [] + + startGitHubListScrollRestore({ + target: 360, + scrollElementRef: ref(list.element), + pendingRestoreRef, + restoreWriteRef, + onScrollTopApplied: (scrollTop) => applied.push(scrollTop) + }) + + expect(list.element.scrollTop).toBe(0) + + list.setMaxScrollTop(900) + resize(list.rows) + + expect(list.element.scrollTop).toBe(360) + expect(pendingRestoreRef.current).toBeNull() + expect(applied.at(-1)).toBe(360) + }) + + it('keeps the remembered offset armed when the list never becomes tall enough', () => { + const list = createScrollList(0) + const pendingRestoreRef = ref(360) + const restoreWriteRef = ref(null) + const applied: number[] = [] + + startGitHubListScrollRestore({ + target: 360, + scrollElementRef: ref(list.element), + pendingRestoreRef, + restoreWriteRef, + onScrollTopApplied: (scrollTop) => applied.push(scrollTop) + }) + + vi.advanceTimersByTime(30_000) + + // The remembered offset must survive an arbitrarily slow paint instead of being + // overwritten with the committed 0 — that is what lost the position for good. + expect(applied).not.toContain(0) + expect(pendingRestoreRef.current).toBe(360) + + list.setMaxScrollTop(900) + resize(list.rows) + + expect(list.element.scrollTop).toBe(360) + expect(pendingRestoreRef.current).toBeNull() + }) + + it('retries when only the scroll container itself resizes', () => { + const list = createScrollList(0) + const pendingRestoreRef = ref(360) + const restoreWriteRef = ref(null) + + startGitHubListScrollRestore({ + target: 360, + scrollElementRef: ref(list.element), + pendingRestoreRef, + restoreWriteRef, + onScrollTopApplied: () => {} + }) + + // A pagination bar appearing or the window resizing shrinks only the container, + // which alone can make the remembered offset reachable. + list.setMaxScrollTop(900) + resize(list.element) + + expect(list.element.scrollTop).toBe(360) + expect(pendingRestoreRef.current).toBeNull() + }) + + it('stops observing once the cleanup runs', () => { + const list = createScrollList(0) + const pendingRestoreRef = ref(360) + const restoreWriteRef = ref(null) + + const stop = startGitHubListScrollRestore({ + target: 360, + scrollElementRef: ref(list.element), + pendingRestoreRef, + restoreWriteRef, + onScrollTopApplied: () => {} + }) + stop() + + list.setMaxScrollTop(900) + resize(list.rows) + + expect(list.element.scrollTop).toBe(0) + }) + + it('leaves the pending restore alone when the list is not mounted', () => { + const pendingRestoreRef = ref(360) + + const stop = startGitHubListScrollRestore({ + target: 360, + scrollElementRef: ref(null), + pendingRestoreRef, + restoreWriteRef: ref(null), + onScrollTopApplied: () => { + throw new Error('nothing to apply without a list') + } + }) + stop() + + expect(pendingRestoreRef.current).toBe(360) + }) + + describe('user takeover', () => { + it('ignores the scroll event the restore itself produced', () => { + // Half-painted: the restore commits 200 of the 360 it wants, and that clamped + // write is what the browser echoes back as a scroll event. + const list = createScrollList(200) + const pendingRestoreRef = ref(360) + const restoreWriteRef = ref(null) + + startGitHubListScrollRestore({ + target: 360, + scrollElementRef: ref(list.element), + pendingRestoreRef, + restoreWriteRef, + onScrollTopApplied: () => {} + }) + + expect( + supersedeGitHubListScrollRestore({ + scrollTop: list.element.scrollTop, + pendingRestoreRef, + restoreWriteRef + }) + ).toBe(false) + expect(pendingRestoreRef.current).toBe(360) + }) + + it('ends a restore the list can never satisfy when the user scrolls', () => { + const list = createScrollList(0) + const pendingRestoreRef = ref(360) + const restoreWriteRef = ref(null) + + startGitHubListScrollRestore({ + target: 360, + scrollElementRef: ref(list.element), + pendingRestoreRef, + restoreWriteRef, + onScrollTopApplied: () => {} + }) + + list.setMaxScrollTop(900) + list.element.scrollTop = 120 + expect( + supersedeGitHubListScrollRestore({ scrollTop: 120, pendingRestoreRef, restoreWriteRef }) + ).toBe(true) + expect(pendingRestoreRef.current).toBeNull() + expect(restoreWriteRef.current).toBeNull() + + // Without the takeover the next resize would drag the user back to the old offset. + resize(list.rows) + expect(list.element.scrollTop).toBe(120) + }) + + it('does not read a write left over from an earlier target as an echo', () => { + const pendingRestoreRef = ref(120) + const restoreWriteRef = ref({ target: 360, committed: 200 }) + + expect( + supersedeGitHubListScrollRestore({ scrollTop: 200, pendingRestoreRef, restoreWriteRef }) + ).toBe(true) + expect(pendingRestoreRef.current).toBeNull() + }) + + it('records the offset once no restore is pending', () => { + const pendingRestoreRef = ref(null) + const restoreWriteRef = ref({ target: 360, committed: 200 }) + + expect( + supersedeGitHubListScrollRestore({ scrollTop: 200, pendingRestoreRef, restoreWriteRef }) + ).toBe(true) + }) + }) +}) diff --git a/src/renderer/src/components/task-page/github/github-list-scroll-restore.ts b/src/renderer/src/components/task-page/github/github-list-scroll-restore.ts new file mode 100644 index 00000000000..6596674c588 --- /dev/null +++ b/src/renderer/src/components/task-page/github/github-list-scroll-restore.ts @@ -0,0 +1,112 @@ +import type { RefObject } from 'react' + +// Committed scrollTop is only exact to the sub-pixel; treat sub-pixel drift as a hit. +const SCROLL_MATCH_EPSILON_PX = 1 + +/** What a restore last committed, tagged with its target so a later restore can't inherit it. */ +export type GitHubListRestoreWrite = { + target: number + committed: number +} + +export type GitHubListScrollRestoreOptions = { + /** Remembered offset the reopened list must land on. */ + target: number + scrollElementRef: RefObject + /** Holds `target` until it is reached or a user scroll supersedes it. */ + pendingRestoreRef: RefObject + /** Lets the scroll handler tell the restore's own write apart from a user gesture. */ + restoreWriteRef: RefObject + onScrollTopApplied: (scrollTop: number) => void +} + +/** + * Drives the reopened GitHub task list back to its remembered offset, retrying until the + * list is tall enough to reach it. Why no deadline: rows can paint arbitrarily late on a + * slow host, and abandoning the restore used to also rewrite the remembered offset with + * the committed 0 — turning a late paint into permanent loss (STA-5949). + */ +export function startGitHubListScrollRestore({ + target, + scrollElementRef, + pendingRestoreRef, + restoreWriteRef, + onScrollTopApplied +}: GitHubListScrollRestoreOptions): () => void { + const scrollElement = scrollElementRef.current + if (!scrollElement) { + return () => {} + } + restoreWriteRef.current = null + let frame: number | null = null + let observer: ResizeObserver | null = null + const stop = (): void => { + if (frame !== null) { + window.cancelAnimationFrame(frame) + frame = null + } + observer?.disconnect() + observer = null + } + const restore = (): void => { + const element = scrollElementRef.current + if (!element || pendingRestoreRef.current !== target) { + stop() + return + } + element.scrollTop = target + const committed = element.scrollTop + restoreWriteRef.current = { target, committed } + // Re-assert the target, not the clamped position: a half-painted list must not + // downgrade what gets remembered on unmount. + onScrollTopApplied(target) + if (Math.abs(committed - target) < SCROLL_MATCH_EPSILON_PX) { + pendingRestoreRef.current = null + stop() + } + } + // The container is observed alongside its rows: a pagination bar appearing or a window + // resize changes only the container, and that alone can make the target reachable. + observer = new ResizeObserver(restore) + observer.observe(scrollElement) + for (const child of scrollElement.children) { + observer.observe(child) + } + restore() + if (pendingRestoreRef.current === target) { + frame = window.requestAnimationFrame(restore) + } + return stop +} + +/** + * Classifies a scroll on the GitHub list. A scroll carrying exactly what the pending + * restore last wrote is its own echo and must change nothing; anything else is the user + * taking over and supersedes the restore — otherwise a target the list can never reach + * would suppress position saving forever (STA-5949). + * + * @returns whether the offset is the user's and should become the remembered position. + */ +export function supersedeGitHubListScrollRestore({ + scrollTop, + pendingRestoreRef, + restoreWriteRef +}: { + scrollTop: number + pendingRestoreRef: RefObject + restoreWriteRef: RefObject +}): boolean { + const write = restoreWriteRef.current + // Matching targets is what keeps a write left over from an earlier restore — pending is + // re-armed from passive effects, after this restore's layout effect — out of the echo. + if ( + write !== null && + write.target === pendingRestoreRef.current && + Math.abs(scrollTop - write.committed) < SCROLL_MATCH_EPSILON_PX + ) { + return false + } + pendingRestoreRef.current = null + restoreWriteRef.current = null + return true +} diff --git a/src/renderer/src/components/task-page/github/github-work-item-table.tsx b/src/renderer/src/components/task-page/github/github-work-item-table.tsx index c785a732466..0f6e1ab6e8e 100644 --- a/src/renderer/src/components/task-page/github/github-work-item-table.tsx +++ b/src/renderer/src/components/task-page/github/github-work-item-table.tsx @@ -24,6 +24,10 @@ import { GITHUB_TASK_STICKY_TITLE_HEADER_CLASS } from './github-task-surface-classes' import type { TaskPageGitHubWorkItemMutationRunner } from './github-work-item-mutation-runner' +import { + supersedeGitHubListScrollRestore, + type GitHubListRestoreWrite +} from './github-list-scroll-restore' import { GithubWorkItemRows } from './github-work-item-rows' import { PaginationBar } from '../pagination/pagination-bar' @@ -32,6 +36,7 @@ export type GithubWorkItemTableProps = { githubResumeContextKey: string currentPageRef: React.MutableRefObject pendingGithubScrollRestoreRef: React.MutableRefObject + githubRestoreScrollWriteRef: React.MutableRefObject githubListScrollTopRef: React.MutableRefObject taskListPositionRef: React.MutableRefObject<{ contextKey: string @@ -75,6 +80,7 @@ export function GithubWorkItemTable(props: GithubWorkItemTableProps): React.JSX. githubResumeContextKey, currentPageRef, pendingGithubScrollRestoreRef, + githubRestoreScrollWriteRef, githubListScrollTopRef, taskListPositionRef, githubTaskGridClass, @@ -112,14 +118,21 @@ export function GithubWorkItemTable(props: GithubWorkItemTableProps): React.JSX. style={{ scrollbarGutter: 'stable' }} onScroll={(event) => { const state = useAppStore.getState() - if ( - state.activeView !== 'tasks' || - state.taskPageData.openGitHubWorkItem || - pendingGithubScrollRestoreRef.current !== null - ) { + if (state.activeView !== 'tasks' || state.taskPageData.openGitHubWorkItem) { return } const scrollTop = event.currentTarget.scrollTop + // Why: a scroll the restore did not produce means the user took over, so it + // supersedes a pending restore that may never reach its target. + if ( + !supersedeGitHubListScrollRestore({ + scrollTop, + pendingRestoreRef: pendingGithubScrollRestoreRef, + restoreWriteRef: githubRestoreScrollWriteRef + }) + ) { + return + } githubListScrollTopRef.current = scrollTop taskListPositionRef.current = { contextKey: githubResumeContextKey, diff --git a/src/renderer/src/components/task-page/hooks/use-task-page-github-list-resume.ts b/src/renderer/src/components/task-page/hooks/use-task-page-github-list-resume.ts deleted file mode 100644 index 76fb5b0a2d2..00000000000 --- a/src/renderer/src/components/task-page/hooks/use-task-page-github-list-resume.ts +++ /dev/null @@ -1,204 +0,0 @@ -import { - useEffect, - useLayoutEffect, - useRef, - type Dispatch, - type MutableRefObject, - type RefObject, - type SetStateAction -} from 'react' - -import { useAppStore } from '@/store' -import { taskPageGitHubResumeCache } from '@/components/task-page-github-resume-cache' -import type { GitHubWorkItem } from '../../../../../shared/github/work-item-types' -import type { TaskProvider } from '../../../../../shared/task-providers' - -export function useTaskPageGitHubListResume({ - pages, - currentPage, - githubResumeContextKey, - taskResumeApplied, - taskSource, - githubMode, - openGitHubWorkItem, - githubListScrollRef, - githubListScrollTopRef, - pendingGithubScrollRestoreRef, - paginationGenerationRef, - setPaginationLoading, - setLoadingTargetPage, - selectedReposKey, - appliedTaskSearch, - workItemsInvalidationNonce, - taskRefreshNonce, - dialogWorkItem -}: { - pages: (GitHubWorkItem[] | null)[] - currentPage: number - githubResumeContextKey: string - taskResumeApplied: boolean - taskSource: TaskProvider - githubMode: 'items' | 'project' - openGitHubWorkItem: GitHubWorkItem | undefined - githubListScrollRef: RefObject - githubListScrollTopRef: MutableRefObject - pendingGithubScrollRestoreRef: MutableRefObject - paginationGenerationRef: MutableRefObject - setPaginationLoading: Dispatch> - setLoadingTargetPage: Dispatch> - selectedReposKey: string - appliedTaskSearch: string - workItemsInvalidationNonce: number - taskRefreshNonce: number - dialogWorkItem: GitHubWorkItem | null -}) { - useEffect(() => { - const page = pages[currentPage] - if (!taskResumeApplied || taskSource !== 'github' || githubMode !== 'items' || !page) { - return - } - taskPageGitHubResumeCache.write(githubResumeContextKey, currentPage, page) - }, [currentPage, githubMode, githubResumeContextKey, pages, taskResumeApplied, taskSource]) - - const taskListPositionRef = useRef<{ - contextKey: string - page: number - scrollTop: number - } | null>(null) - - useLayoutEffect(() => { - if ( - taskSource !== 'github' || - githubMode !== 'items' || - openGitHubWorkItem || - pendingGithubScrollRestoreRef.current !== null - ) { - return - } - taskListPositionRef.current = { - contextKey: githubResumeContextKey, - page: currentPage, - scrollTop: githubListScrollTopRef.current - } - }, [ - currentPage, - githubMode, - githubResumeContextKey, - openGitHubWorkItem, - taskSource, - githubListScrollTopRef, - pendingGithubScrollRestoreRef - ]) - - useEffect( - () => () => { - const position = taskListPositionRef.current - const state = useAppStore.getState() - if (position && !state.taskPageData.openGitHubWorkItem) { - state.setTaskListPosition({ - contextKey: position.contextKey, - page: position.page, - scrollTop: position.scrollTop - }) - } - }, - [] - ) - - // Why: keyed on selectedReposKey, not the selectedRepos array — a background - // repos:changed refresh mid-flight would otherwise bump the generation and - // silently discard the user's page navigation (#11485). Mirrors every dep of - // the fetch effect that resets page state, so a reset always invalidates - // in-flight page requests. - useEffect(() => { - paginationGenerationRef.current += 1 - setPaginationLoading(false) - setLoadingTargetPage(null) - }, [ - selectedReposKey, - appliedTaskSearch, - workItemsInvalidationNonce, - taskRefreshNonce, - taskSource, - githubMode, - taskResumeApplied, - - setPaginationLoading, - paginationGenerationRef, - setLoadingTargetPage - ]) - - useLayoutEffect(() => { - const scrollTop = pendingGithubScrollRestoreRef.current - const scrollElement = githubListScrollRef.current - if (scrollTop === null || !scrollElement || !pages[currentPage]) { - return - } - let frame: number | null = null - let timeout: number | null = null - let observer: ResizeObserver | null = null - const clearScheduledRestore = (): void => { - if (frame !== null) { - window.cancelAnimationFrame(frame) - frame = null - } - if (timeout !== null) { - window.clearTimeout(timeout) - timeout = null - } - observer?.disconnect() - } - const restore = (): void => { - const committedScrollElement = githubListScrollRef.current - if (!committedScrollElement || pendingGithubScrollRestoreRef.current !== scrollTop) { - return - } - committedScrollElement.scrollTop = scrollTop - githubListScrollTopRef.current = scrollTop - taskListPositionRef.current = { - contextKey: githubResumeContextKey, - page: currentPage, - scrollTop - } - if (Math.abs(committedScrollElement.scrollTop - scrollTop) < 1) { - pendingGithubScrollRestoreRef.current = null - clearScheduledRestore() - } - } - observer = new ResizeObserver(restore) - for (const child of scrollElement.children) { - observer.observe(child) - } - restore() - if (pendingGithubScrollRestoreRef.current === scrollTop) { - frame = window.requestAnimationFrame(restore) - timeout = window.setTimeout(() => { - if (pendingGithubScrollRestoreRef.current === scrollTop) { - const committedScrollTop = githubListScrollRef.current?.scrollTop ?? 0 - githubListScrollTopRef.current = committedScrollTop - taskListPositionRef.current = { - contextKey: githubResumeContextKey, - page: currentPage, - scrollTop: committedScrollTop - } - pendingGithubScrollRestoreRef.current = null - } - clearScheduledRestore() - }, 5_000) - } - return clearScheduledRestore - }, [ - currentPage, - dialogWorkItem, - githubResumeContextKey, - pages, - githubListScrollRef.current?.scrollTop, - githubListScrollRef, - githubListScrollTopRef, - pendingGithubScrollRestoreRef - ]) - - return { - taskListPositionRef - } -} diff --git a/src/renderer/src/components/task-page/hooks/use-task-page-github-list-state.ts b/src/renderer/src/components/task-page/hooks/use-task-page-github-list-state.ts index b5ad273cc29..d85a7e3d0b4 100644 --- a/src/renderer/src/components/task-page/hooks/use-task-page-github-list-state.ts +++ b/src/renderer/src/components/task-page/hooks/use-task-page-github-list-state.ts @@ -9,6 +9,7 @@ import { sortWorkItemsByNumber } from '../../../../../shared/work-items' import type { GitHubWorkItem } from '../../../../../shared/github/work-item-types' import type { Repo } from '../../../../../shared/repo-types' import type { TaskViewPresetId } from '../../../../../shared/ui-chrome-types' +import type { GitHubListRestoreWrite } from '@/components/task-page/github/github-list-scroll-restore' import type { AppState } from '@/store/types' export function useTaskPageGitHubListState({ @@ -99,6 +100,8 @@ export function useTaskPageGitHubListState({ const githubListScrollRef = useRef(null) const githubListScrollTopRef = useRef(0) const pendingGithubScrollRestoreRef = useRef(null) + // Why: lets the scroll handler tell the restore's own write apart from a user gesture. + const githubRestoreScrollWriteRef = useRef(null) const [paginationLoading, setPaginationLoading] = useState(false) const [loadingTargetPage, setLoadingTargetPage] = useState(null) const [countedTotalPages, setCountedTotalPages] = useState(null) @@ -157,6 +160,7 @@ export function useTaskPageGitHubListState({ githubListScrollRef, githubListScrollTopRef, pendingGithubScrollRestoreRef, + githubRestoreScrollWriteRef, paginationLoading, setPaginationLoading, loadingTargetPage, diff --git a/tests/e2e/tasks-page.spec.ts b/tests/e2e/tasks-page.spec.ts index c9836cfd070..de7ad4b5616 100644 --- a/tests/e2e/tasks-page.spec.ts +++ b/tests/e2e/tasks-page.spec.ts @@ -7,6 +7,12 @@ import { test, expect } from './helpers/orca-app' import { waitForSessionReady, waitForActiveWorktree, getStoreState } from './helpers/store' +import { GITHUB_TASK_SEARCH_IDLE_MS } from '../../src/renderer/src/components/use-github-task-search-commit' + +// Why derived: a fixed 400ms cadence left only ~150ms of margin against the idle window +// on a loaded runner, so one slow keystroke committed a prefix and failed the assertion. +const TASK_SEARCH_TYPING_DELAY_MS = Math.round(GITHUB_TASK_SEARCH_IDLE_MS / 6) +const TASK_SEARCH_SETTLE_MS = GITHUB_TASK_SEARCH_IDLE_MS + 50 type RenderedTaskSource = { source: string @@ -379,6 +385,9 @@ test.describe('Tasks page', () => { 'aria-current', 'page' ) + // Why the wait: outlives the 5s give-up that used to abandon the restore and + // overwrite the remembered offset with the committed 0. A list that never paints + // must defer the restore, never destroy the position. await orcaPage.waitForTimeout(5_500) await orcaPage.getByRole('button', { name: 'Close tasks' }).click() await expect @@ -386,7 +395,7 @@ test.describe('Tasks page', () => { const position = await getStoreState<{ scrollTop: number }>(orcaPage, 'taskListPosition') return position.scrollTop }) - .toBe(0) + .toBeGreaterThan(300) await permanentlyClampedRowsStyle.evaluate((element) => element.remove()) }) @@ -401,17 +410,15 @@ test.describe('Tasks page', () => { await expect(existingIssue).toBeVisible() await input.fill('') - await orcaPage.waitForTimeout(800) + await orcaPage.waitForTimeout(TASK_SEARCH_SETTLE_MS) await resetTaskSearchRequestProbe(orcaPage) - await input.pressSequentially('rate', { delay: 400 }) + await input.pressSequentially('rate', { delay: TASK_SEARCH_TYPING_DELAY_MS }) - expect(await readTaskSearchRequestProbe(orcaPage)).toEqual({ - countQueries: [], - fetchQueries: [] - }) await expect(existingIssue).toBeVisible() + // The contract is that no prefix of the typed query is ever queried, not that the + // probe is empty at one instant: exactly one request per surface, for the final value. await expect .poll(async () => readTaskSearchRequestProbe(orcaPage), { timeout: 2_000 }) .toEqual({ countQueries: ['is:issue rate'], fetchQueries: ['is:issue rate'] }) @@ -423,7 +430,7 @@ test.describe('Tasks page', () => { await expect .poll(async () => readTaskSearchRequestProbe(orcaPage), { timeout: 2_000 }) .toEqual({ countQueries: ['is:issue ratex'], fetchQueries: ['is:issue ratex'] }) - await orcaPage.waitForTimeout(800) + await orcaPage.waitForTimeout(TASK_SEARCH_SETTLE_MS) expect(await readTaskSearchRequestProbe(orcaPage)).toEqual({ countQueries: ['is:issue ratex'], fetchQueries: ['is:issue ratex'] From ac02232015c67cfcb774535a2123e56b50b963b3 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 30 Aug 2026 11:50:29 -0700 Subject: [PATCH 05/59] perf: overlap independent worktree create preflight (#17386) Readiness checklist passed; required CI and review checks are green. --- src/main/ipc/worktree-remote.ts | 65 +++++++++---------- .../ipc/worktrees-local-create-flow.test.ts | 49 ++++++++++++++ src/main/runtime/orca-runtime.ts | 45 ++++++------- 3 files changed, 100 insertions(+), 59 deletions(-) diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index d9908b7c05c..a189f091900 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -1549,14 +1549,16 @@ export async function createRemoteWorktree( await registerRequiredSshWorktreeCreateRoots(repo.connectionId!, [repo.path]) // Why: explicit branches and non-username prefix modes never consume this; skipping the remote probe preserves the exact branch name. - const username = - !args.branchNameOverride && settings.branchPrefix === 'git-username' - ? await getSshGitUsername(provider, repo.path) - : '' - const branchConflictSubject = args.branchNameOverride ? 'branch name' : 'worktree name' // Why: don't fall back to hardcoded 'origin/main'; it may not exist (master/develop) and yields an opaque git error, so fail clearly and let the UI prompt. - const basePlan = await getOrStartRemoteWorktreeCreateBasePlan(provider, repo, args.baseBranch) + // Username and base-plan probes are independent read-only work; overlap them so + // SSH latency is paid once before the conflict loop. + const [username, basePlan] = await Promise.all([ + !args.branchNameOverride && settings.branchPrefix === 'git-username' + ? getSshGitUsername(provider, repo.path) + : Promise.resolve(''), + getOrStartRemoteWorktreeCreateBasePlan(provider, repo, args.baseBranch) + ]) if (!basePlan) { throw new Error( 'Could not resolve a default base ref for this repo. Pick a base branch explicitly and try again.' @@ -1567,12 +1569,10 @@ export async function createRemoteWorktree( let baseFallback: WorktreeCreateBaseFallback | undefined if (remoteTrackingBase) { - const hasRemoteTrackingBaseRef = await hasCommitRefSsh( - provider, - repo.path, - remoteTrackingBase.ref - ) - const hasNamedLocalBaseRef = await hasRemoteWorktreeBaseRef(provider, repo.path, baseBranch) + const [hasRemoteTrackingBaseRef, hasNamedLocalBaseRef] = await Promise.all([ + hasCommitRefSsh(provider, repo.path, remoteTrackingBase.ref), + hasRemoteWorktreeBaseRef(provider, repo.path, baseBranch) + ]) const hasFallbackLocalBaseRef = !hasNamedLocalBaseRef && (await hasRemoteWorktreeBaseRef(provider, repo.path, remoteTrackingBase.branch)) @@ -2016,12 +2016,13 @@ export async function createLocalWorktree( ? sanitizeWorktreeDisplayName(args.displayName) : undefined // Why: explicit branches and non-username prefix modes never consume this; skipping the probe preserves the exact generated branch name. - const username = + // Username and base resolution are independent read-only probes. Starting + // both before awaiting removes one serial git/config round trip from create. + const usernamePromise = !args.branchNameOverride && settings.branchPrefix === 'git-username' - ? await resolveLocalGitUsername(repo.path) - : '' - - let baseBranch = await resolveWorktreeCreateBase({ + ? resolveLocalGitUsername(repo.path) + : Promise.resolve('') + const baseBranchPromise = resolveWorktreeCreateBase({ requestedBaseBranch: args.baseBranch, repoWorktreeBaseRef: repo.worktreeBaseRef, resolveDefaultBaseRef: () => resolveDefaultBaseRefWithLocalGit(localGitExecOptions), @@ -2052,6 +2053,8 @@ export async function createLocalWorktree( return hasLocalWorktreeBaseRefWithOptions(repo.path, baseBranchCandidate, localGitExecOptions) } }) + const [username, resolvedBaseBranch] = await Promise.all([usernamePromise, baseBranchPromise]) + let baseBranch = resolvedBaseBranch if (!baseBranch) { // Why: no default base resolved; fail clearly rather than pass a hardcoded non-existent ref to git worktree add (opaque error) so the UI can prompt. throw new Error( @@ -2075,16 +2078,10 @@ export async function createLocalWorktree( ...localWorktreeGitOptionArgs ) if (remoteTrackingBase) { - const hasRemoteTrackingBaseRef = await runtime.hasRemoteTrackingRef( - repo.path, - remoteTrackingBase, - ...localWorktreeGitOptionArgs - ) - const hasNamedLocalBaseRef = await hasLocalWorktreeBaseRefWithOptions( - repo.path, - baseBranch, - localGitExecOptions - ) + const [hasRemoteTrackingBaseRef, hasNamedLocalBaseRef] = await Promise.all([ + runtime.hasRemoteTrackingRef(repo.path, remoteTrackingBase, ...localWorktreeGitOptionArgs), + hasLocalWorktreeBaseRefWithOptions(repo.path, baseBranch, localGitExecOptions) + ]) const hasFallbackLocalBaseRef = !hasNamedLocalBaseRef && (await hasLocalWorktreeBaseRefWithOptions( @@ -2591,9 +2588,14 @@ export async function createLocalWorktree( // Why: project-level `orca.yaml` shared directories add to (never replace) the per-user // setting, so a repo's shared dirs reach every teammate (issue #10451). - const sharedDirectories = await timing.time('resolve_shared_directories', () => - resolveWorktreeSharedDirectories(repo.path, localWorktreeGitOptions) - ) + const [sharedDirectories, includePaths] = await Promise.all([ + timing.time('resolve_shared_directories', () => + resolveWorktreeSharedDirectories(repo.path, localWorktreeGitOptions) + ), + timing.time('resolve_worktreeinclude', () => + resolveWorktreeIncludePaths(repo.path, localWorktreeGitOptions) + ) + ]) if (sharedDirectories.length > 0) { await timing.time('create_shared_directories', async () => { await createWorktreeSharedPaths(repo.path, created.path, sharedDirectories) @@ -2602,9 +2604,6 @@ export async function createLocalWorktree( // Why: project-level `.worktreeinclude` travels with the repo (issue #7549); copy semantics // (never symlink) so each worktree owns its files. Paths already linked above are skipped. - const includePaths = await timing.time('resolve_worktreeinclude', () => - resolveWorktreeIncludePaths(repo.path, localWorktreeGitOptions) - ) let includeCopyWarning: string | undefined if (includePaths.length > 0) { await timing.time('copy_worktreeinclude', async () => { diff --git a/src/main/ipc/worktrees-local-create-flow.test.ts b/src/main/ipc/worktrees-local-create-flow.test.ts index a030a60de29..4a580f82de0 100644 --- a/src/main/ipc/worktrees-local-create-flow.test.ts +++ b/src/main/ipc/worktrees-local-create-flow.test.ts @@ -7,6 +7,7 @@ import { addWorktreeMock, resolveLocalGitUsernameMock, getBaseRefDefaultMock, + resolveDefaultBaseRefWithLocalGitMock, getBranchConflictKindMock, getEffectiveHooksMock, createSetupRunnerScriptMock, @@ -108,6 +109,54 @@ describe('registerWorktreeHandlers', () => { runtimeStub = setupWorktreeHandlers() }) + it('starts username and base-ref probes concurrently', async () => { + const events: string[] = [] + let resolveUsername!: (value: string) => void + let resolveBase!: (value: string | null) => void + store.getSettings.mockReturnValue({ + branchPrefix: 'git-username', + nestWorkspaces: false, + refreshLocalBaseRefOnWorktreeCreate: false, + workspaceDir: '/workspace' + }) + resolveLocalGitUsernameMock.mockImplementation( + () => + new Promise((resolve) => { + events.push('username-start') + resolveUsername = resolve + }) + ) + resolveDefaultBaseRefWithLocalGitMock.mockImplementation( + () => + new Promise((resolve) => { + events.push('base-start') + resolveBase = resolve + }) + ) + listWorktreesMock.mockResolvedValue([ + { + path: '/workspace/concurrent-probe', + head: 'created-sha', + branch: 'jdoe/concurrent-probe', + isBare: false, + isMainWorktree: false + } + ]) + + const creation = handlers['worktrees:create'](null, { + repoId: 'repo-1', + name: 'concurrent-probe' + }) + await Promise.resolve() + + expect(events).toEqual(['username-start', 'base-start']) + resolveUsername('jdoe') + resolveBase('origin/main') + await expect(creation).resolves.toMatchObject({ + worktree: expect.objectContaining({ branch: 'jdoe/concurrent-probe' }) + }) + }) + it('prefetches the local default create base through the runtime refresh cache', async () => { const repo = { id: 'repo-1', diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index ea852917b94..8b3793581d6 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -26791,14 +26791,13 @@ export class OrcaRuntimeService { const requestedDisplayName = args.displayName?.trim() || undefined const sanitizedName = sanitizeWorktreeName(args.name) let effectiveSanitizedName = sanitizedName - // Why: explicit branches and non-username prefix modes never consume this - // value; skipping the probes preserves the exact generated branch name. - const username = + // Username and base resolution are independent read-only probes. Starting + // both before awaiting removes one serial git/config round trip from create. + const usernamePromise = !args.branchNameOverride && settings.branchPrefix === 'git-username' - ? await resolveLocalGitUsername(repo.path) - : '' - - const baseBranch = await resolveWorktreeCreateBase({ + ? resolveLocalGitUsername(repo.path) + : Promise.resolve('') + const baseBranchPromise = resolveWorktreeCreateBase({ requestedBaseBranch: args.baseBranch, repoWorktreeBaseRef: repo.worktreeBaseRef, resolveDefaultBaseRef: () => @@ -26834,6 +26833,7 @@ export class OrcaRuntimeService { ) } }) + const [username, baseBranch] = await Promise.all([usernamePromise, baseBranchPromise]) if (!baseBranch) { // Why: a null default means no suitable ref exists; fail clearly instead // of handing Git a fabricated origin/main ref. @@ -26990,18 +26990,15 @@ export class OrcaRuntimeService { ...localWorktreeGitOptionArgs ) if (remoteTrackingBase) { - const hadRemoteTrackingBaseRef = await this.hasRemoteTrackingRef( - repo.path, - remoteTrackingBase, - ...localWorktreeGitOptionArgs - ) - const hasLocalBaseRef = - hadRemoteTrackingBaseRef || - (await hasLocalWorktreeBaseRef( + const [hadRemoteTrackingBaseRef, hasNamedLocalBaseRef] = await Promise.all([ + this.hasRemoteTrackingRef(repo.path, remoteTrackingBase, ...localWorktreeGitOptionArgs), + hasLocalWorktreeBaseRef( repo.path, baseBranch, hasLocalWorktreeGitOptions ? localWorktreeGitOptions : {} - )) + ) + ]) + const hasLocalBaseRef = hadRemoteTrackingBaseRef || hasNamedLocalBaseRef if (!hadRemoteTrackingBaseRef && hasLocalBaseRef) { remoteTrackingBase = null } else { @@ -27300,22 +27297,18 @@ export class OrcaRuntimeService { await createWorktreeLinkedPaths(repo.path, created.path, symlinkPaths) } - // Why: project-level `orca.yaml` shared directories add to (never replace) the - // per-user setting, so a repo's shared dirs reach every teammate (issue #10451). - const sharedDirectories = await resolveWorktreeSharedDirectories( - repo.path, - localWorktreeGitOptions - ) + // Why: these discoveries are read-only; overlap them, but keep the + // shared-path mutation ahead of include copies below. + const [sharedDirectories, worktreeIncludePaths] = await Promise.all([ + resolveWorktreeSharedDirectories(repo.path, localWorktreeGitOptions), + resolveWorktreeIncludePaths(repo.path, localWorktreeGitOptions) + ]) if (sharedDirectories.length > 0) { await createWorktreeSharedPaths(repo.path, created.path, sharedDirectories) } // Why: project-level `.worktreeinclude` travels with the repo (issue #7549); copy semantics // (never symlink) so each worktree owns its files. Paths already linked above are skipped. - const worktreeIncludePaths = await resolveWorktreeIncludePaths( - repo.path, - localWorktreeGitOptions - ) let includeCopyWarning: string | undefined if (worktreeIncludePaths.length > 0) { const skippedIncludePaths = await createWorktreeCopiedPaths( From 16e6b103d6e2823a1a86644a625985f9e837d668 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 30 Aug 2026 12:07:41 -0700 Subject: [PATCH 06/59] fix(session): deduplicate editor records during restore (#17370) Deduplicate persisted editor records and repair tab-group references during session hydration. Closes #17185. --- .../editor-session-duplicate-restore.test.ts | 358 ++++++++++++++++++ .../editor/actions/hydrate-editor-session.ts | 12 +- .../slices/tab-group-reference-repair.ts | 49 +++ .../src/store/slices/tab-group-state.ts | 26 ++ .../src/store/slices/tabs-hydration.test.ts | 10 +- .../src/store/slices/tabs-hydration.ts | 34 +- 6 files changed, 477 insertions(+), 12 deletions(-) create mode 100644 src/renderer/src/store/slices/editor-session-duplicate-restore.test.ts diff --git a/src/renderer/src/store/slices/editor-session-duplicate-restore.test.ts b/src/renderer/src/store/slices/editor-session-duplicate-restore.test.ts new file mode 100644 index 00000000000..613ac98f69c --- /dev/null +++ b/src/renderer/src/store/slices/editor-session-duplicate-restore.test.ts @@ -0,0 +1,358 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as AgentStatusModule from '@/lib/agent-status' +import type { WorkspaceSessionState } from '../../../../shared/workspace-session-state-types' +import { folderWorkspaceKey } from '../../../../shared/workspace-scope' +import { buildWorkspaceSessionPayload } from '../../lib/workspace-session' +import { createStoreSessionMockApi } from './store-session-test-harness' +import { createTestStore, makeWorktree } from './store-test-helpers' + +vi.mock('sonner', () => ({ toast: { info: vi.fn(), success: vi.fn(), error: vi.fn() } })) +vi.mock('@/lib/agent-status', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, detectAgentStatusFromTitle: vi.fn().mockReturnValue(null) } +}) + +createStoreSessionMockApi() + +const WORKTREE_ID = 'repo-1::/workspace' +const FILE_PATH = '/workspace/.scratch/preview.png' +const SSH_FOLDER_ID = 'ssh-folder' +const SSH_FOLDER_KEY = folderWorkspaceKey(SSH_FOLDER_ID) +const SSH_FILE_PATH = '/srv/workspace/.scratch/preview.png' + +function prepareStore() { + const store = createTestStore() + store.setState({ + repos: [ + { id: 'repo-1', path: '/workspace', displayName: 'Repo', badgeColor: '#000', addedAt: 0 } + ], + worktreesByRepo: { + 'repo-1': [makeWorktree({ id: WORKTREE_ID, repoId: 'repo-1', path: '/workspace' })] + }, + activeWorktreeId: WORKTREE_ID + }) + return store +} + +function prepareSshFolderStore() { + const store = createTestStore() + store.setState({ + folderWorkspaces: [ + { + id: SSH_FOLDER_ID, + projectGroupId: 'ssh-project', + name: 'SSH folder', + folderPath: '/srv/workspace', + connectionId: 'ssh-target', + linkedTask: null, + comment: '', + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 0, + lastActivityAt: 0, + createdAt: 1, + updatedAt: 1 + } + ], + activeWorktreeId: SSH_FOLDER_KEY + }) + return store +} + +function corruptSession(worktreeId = WORKTREE_ID, filePath = FILE_PATH): WorkspaceSessionState { + const persistedFile = { + filePath, + relativePath: '.scratch/preview.png', + worktreeId, + language: 'image' + } + const editorTab = (id: string, sortOrder: number) => ({ + id, + entityId: filePath, + groupId: 'group-restored', + worktreeId, + contentType: 'editor' as const, + label: 'preview.png', + customLabel: null, + color: null, + sortOrder, + createdAt: 1_788_002_466_152 + }) + return { + activeRepoId: 'repo-1', + activeWorktreeId: worktreeId, + activeTabId: null, + tabsByWorktree: {}, + terminalLayoutsByTabId: {}, + openFilesByWorktree: { + [worktreeId]: [persistedFile, { ...persistedFile }, { ...persistedFile }] + }, + activeFileIdByWorktree: { [worktreeId]: `editor:stale-pane:${filePath}` }, + activeTabTypeByWorktree: { [worktreeId]: 'editor' }, + unifiedTabs: { + [worktreeId]: [editorTab('editor-restored-a', 0), editorTab('editor-restored-b', 1)] + }, + tabGroups: { + [worktreeId]: [ + { + id: 'group-restored', + worktreeId, + activeTabId: `editor:third-pane:${filePath}`, + tabOrder: [`editor:third-pane:${filePath}`] + } + ] + }, + activeGroupIdByWorktree: { [worktreeId]: 'group-restored' } + } +} + +function closeAndRestart( + prepare: typeof prepareStore, + session: WorkspaceSessionState, + workspaceId: string +) { + const store = prepare() + store.getState().hydrateTabsSession(session) + store.getState().hydrateEditorSession(session) + + expect.soft(store.getState().openFiles).toHaveLength(1) + expect.soft(store.getState().unifiedTabsByWorktree[workspaceId]).toHaveLength(1) + + const activeFileId = store.getState().activeFileIdByWorktree[workspaceId] + expect(activeFileId).toBeTruthy() + store.getState().closeFile(activeFileId!) + + const persistedAfterClose = buildWorkspaceSessionPayload(store.getState()) + const restartedStore = prepare() + restartedStore.getState().hydrateTabsSession(persistedAfterClose) + restartedStore.getState().hydrateEditorSession(persistedAfterClose) + return restartedStore.getState() +} + +describe('corrupt editor session restore', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('does not preserve duplicate records that resurrect a closed editor', () => { + const restarted = closeAndRestart(prepareStore, corruptSession(), WORKTREE_ID) + expect(restarted.openFiles).toEqual([]) + expect(restarted.unifiedTabsByWorktree[WORKTREE_ID] ?? []).toEqual([]) + }) + + it('cleans the same corruption for an SSH-hosted folder workspace', () => { + const restarted = closeAndRestart( + prepareSshFolderStore, + corruptSession(SSH_FOLDER_KEY, SSH_FILE_PATH), + SSH_FOLDER_KEY + ) + expect(restarted.openFiles).toEqual([]) + expect(restarted.unifiedTabsByWorktree[SSH_FOLDER_KEY] ?? []).toEqual([]) + }) + + it('rewrites group references from a duplicate editor row to the survivor', () => { + const store = prepareStore() + const session = corruptSession() + session.tabGroups![WORKTREE_ID]![0] = { + ...session.tabGroups![WORKTREE_ID]![0], + activeTabId: 'editor-restored-b', + tabOrder: ['editor-restored-b'], + recentTabIds: ['editor-restored-b'] + } + + store.getState().hydrateTabsSession(session) + + expect(store.getState().unifiedTabsByWorktree[WORKTREE_ID].map((tab) => tab.id)).toEqual([ + 'editor-restored-a' + ]) + expect(store.getState().groupsByWorktree[WORKTREE_ID][0]).toEqual( + expect.objectContaining({ + activeTabId: 'editor-restored-a', + tabOrder: ['editor-restored-a'], + recentTabIds: ['editor-restored-a'] + }) + ) + }) + + it('does not redirect a stale alias reference into another group', () => { + const store = prepareStore() + const session = corruptSession() + const [left, right] = session.unifiedTabs![WORKTREE_ID]! + session.unifiedTabs![WORKTREE_ID] = [ + { ...left, id: 'editor-group-a-left', groupId: 'group-a' }, + { ...right, id: 'editor-group-a-duplicate', groupId: 'group-a' }, + { + ...right, + id: 'editor-group-b', + entityId: '/workspace/.scratch/other.png', + groupId: 'group-b', + label: 'other.png', + sortOrder: 2 + } + ] + session.tabGroups![WORKTREE_ID] = [ + { + id: 'group-a', + worktreeId: WORKTREE_ID, + activeTabId: 'editor-group-a-duplicate', + tabOrder: ['editor-group-a-left', 'editor-group-a-duplicate'] + }, + { + id: 'group-b', + worktreeId: WORKTREE_ID, + activeTabId: 'editor-group-b', + // This stale reference must not be rewritten with group-a's alias. + tabOrder: ['editor-group-a-duplicate', 'editor-group-b'] + } + ] + + store.getState().hydrateTabsSession(session) + + expect(store.getState().groupsByWorktree[WORKTREE_ID]).toEqual([ + expect.objectContaining({ id: 'group-a', tabOrder: ['editor-group-a-left'] }), + expect.objectContaining({ id: 'group-b', tabOrder: ['editor-group-b'] }) + ]) + }) + + it('preserves shared editor entities in separate split groups', () => { + const store = prepareStore() + const session = corruptSession() + const [left, right] = session.unifiedTabs![WORKTREE_ID]! + session.unifiedTabs![WORKTREE_ID] = [ + { ...left, id: 'editor-left', groupId: 'group-left' }, + { ...right, id: 'editor-right', groupId: 'group-right' } + ] + session.tabGroups![WORKTREE_ID] = [ + { + id: 'group-left', + worktreeId: WORKTREE_ID, + activeTabId: 'editor-left', + tabOrder: ['editor-left'] + }, + { + id: 'group-right', + worktreeId: WORKTREE_ID, + activeTabId: 'editor-right', + tabOrder: ['editor-right'] + } + ] + + store.getState().hydrateTabsSession(session) + store.getState().hydrateEditorSession(session) + + expect(store.getState().openFiles).toHaveLength(1) + expect(store.getState().unifiedTabsByWorktree[WORKTREE_ID].map((tab) => tab.id)).toEqual([ + 'editor-left', + 'editor-right' + ]) + }) + + it('does not let a globally duplicated id hydrate into another group', () => { + const store = prepareStore() + const session = corruptSession() + session.unifiedTabs![WORKTREE_ID] = [ + { + ...session.unifiedTabs![WORKTREE_ID]![0], + id: 'shared-id', + entityId: '/workspace/.scratch/group-a.png', + groupId: 'group-a', + label: 'group-a.png', + sortOrder: 0 + }, + { + ...session.unifiedTabs![WORKTREE_ID]![1], + id: 'shared-id', + entityId: '/workspace/.scratch/group-b.png', + groupId: 'group-b', + label: 'group-b.png', + sortOrder: 1 + }, + { + ...session.unifiedTabs![WORKTREE_ID]![1], + id: 'group-b-only', + entityId: '/workspace/.scratch/group-b-only.png', + groupId: 'group-b', + label: 'group-b-only.png', + sortOrder: 2 + } + ] + session.tabGroups![WORKTREE_ID] = [ + { + id: 'group-a', + worktreeId: WORKTREE_ID, + activeTabId: 'shared-id', + tabOrder: ['shared-id'] + }, + { + id: 'group-b', + worktreeId: WORKTREE_ID, + activeTabId: 'shared-id', + tabOrder: ['shared-id', 'group-b-only'] + } + ] + + store.getState().hydrateTabsSession(session) + + expect(store.getState().groupsByWorktree[WORKTREE_ID]).toEqual([ + expect.objectContaining({ id: 'group-a', tabOrder: ['shared-id'] }), + expect.objectContaining({ id: 'group-b', tabOrder: ['group-b-only'], activeTabId: null }) + ]) + }) + + it('keeps a stale cross-group reference away from a surviving declared owner', () => { + const store = prepareStore() + const session = corruptSession() + session.unifiedTabs![WORKTREE_ID] = [ + { + ...session.unifiedTabs![WORKTREE_ID]![0], + id: 'shared-id', + entityId: '/workspace/.scratch/shared.png', + groupId: 'group-a', + label: 'shared.png', + sortOrder: 0 + }, + { + ...session.unifiedTabs![WORKTREE_ID]![0], + id: 'group-a-only', + entityId: '/workspace/.scratch/group-a-only.png', + groupId: 'group-a', + label: 'group-a-only.png', + sortOrder: 1 + }, + { + ...session.unifiedTabs![WORKTREE_ID]![1], + id: 'group-b-only', + entityId: '/workspace/.scratch/group-b-only.png', + groupId: 'group-b', + label: 'group-b-only.png', + sortOrder: 2 + } + ] + session.tabGroups![WORKTREE_ID] = [ + { + id: 'group-b', + worktreeId: WORKTREE_ID, + activeTabId: 'shared-id', + tabOrder: ['shared-id', 'group-b-only'] + }, + { + id: 'group-a', + worktreeId: WORKTREE_ID, + activeTabId: 'group-a-only', + tabOrder: ['group-a-only'] + } + ] + + store.getState().hydrateTabsSession(session) + + expect(store.getState().groupsByWorktree[WORKTREE_ID]).toEqual([ + expect.objectContaining({ id: 'group-b', tabOrder: ['group-b-only'], activeTabId: null }), + expect.objectContaining({ + id: 'group-a', + tabOrder: ['group-a-only', 'shared-id'], + activeTabId: 'group-a-only' + }) + ]) + }) +}) diff --git a/src/renderer/src/store/slices/editor/actions/hydrate-editor-session.ts b/src/renderer/src/store/slices/editor/actions/hydrate-editor-session.ts index 3b76d3df384..9ccd754c970 100644 --- a/src/renderer/src/store/slices/editor/actions/hydrate-editor-session.ts +++ b/src/renderer/src/store/slices/editor/actions/hydrate-editor-session.ts @@ -7,7 +7,7 @@ import { folderWorkspaceKey } from '../../../../../../shared/workspace-scope' import type { WorkspaceVisibleTabType } from '../../../../../../shared/tab-types' import type { OpenFile } from '../types/open-file' import { buildValidWorktreeIdsForSessionHydration } from '../../degraded-repo-worktree-validity' -import { buildOwnedEditorFileId } from '../file-ids/editor-file-ids' +import { buildOwnedEditorFileId, isSameEditorOwner } from '../file-ids/editor-file-ids' import { addEditorFileIdMigration, migrateEditorFileId, @@ -49,6 +49,16 @@ export function createHydrateEditorSession( continue } for (const pf of files) { + // Split tabs share one OpenFile; repeated records for the same owner are corruption. + if ( + legacyHydratedOpenFiles.some( + (file) => + file.filePath === pf.filePath && + isSameEditorOwner(file, worktreeId, pf.runtimeEnvironmentId) + ) + ) { + continue + } const legacyId = resolveLegacyHydratedEditorFileId( legacyHydratedOpenFiles, pf, diff --git a/src/renderer/src/store/slices/tab-group-reference-repair.ts b/src/renderer/src/store/slices/tab-group-reference-repair.ts index e992139cf28..2bf1c468093 100644 --- a/src/renderer/src/store/slices/tab-group-reference-repair.ts +++ b/src/renderer/src/store/slices/tab-group-reference-repair.ts @@ -30,6 +30,55 @@ export function layoutSpanningGroups( ) } +/** Prevent one retained tab row from hydrating into multiple groups. */ +export function resolveTabGroupOwners( + tabs: readonly Tab[], + groups: readonly TabGroup[], + tabIdAliasesByGroup?: Map> +): Map { + const persistedGroupIds = new Set(groups.map((group) => group.id)) + const tabGroupIdById = new Map(tabs.map((tab) => [tab.id, tab.groupId])) + const tabOwners = new Map( + tabs.filter((tab) => persistedGroupIds.has(tab.groupId)).map((tab) => [tab.id, tab.groupId]) + ) + for (const group of groups) { + const tabIdAliases = tabIdAliasesByGroup?.get(group.id) + for (const persistedTabId of group.tabOrder) { + const tabId = tabIdAliases?.get(persistedTabId) ?? persistedTabId + if (!tabGroupIdById.has(tabId)) { + continue + } + if (!tabOwners.has(tabId)) { + tabOwners.set(tabId, group.id) + } + } + } + return tabOwners +} + +/** Restore declared-owner rows omitted from a persisted tab order. */ +export function appendOwnedTabIdsToGroups( + groups: readonly TabGroup[], + tabOwners: ReadonlyMap +): TabGroup[] { + const tabIdsByGroup = new Map() + for (const [tabId, groupId] of tabOwners) { + const tabIds = tabIdsByGroup.get(groupId) ?? [] + tabIds.push(tabId) + tabIdsByGroup.set(groupId, tabIds) + } + return groups.map((group) => { + const ownedTabIds = tabIdsByGroup.get(group.id) + if (!ownedTabIds) { + return group + } + const missingTabIds = ownedTabIds.filter((tabId) => !group.tabOrder.includes(tabId)) + return missingTabIds.length > 0 + ? { ...group, tabOrder: [...group.tabOrder, ...missingTabIds] } + : group + }) +} + /** Re-home tabs stranded by a dropped group so hydration cannot render a blank workspace. */ export function adoptGrouplessTabs( tabsByWorktree: Record, diff --git a/src/renderer/src/store/slices/tab-group-state.ts b/src/renderer/src/store/slices/tab-group-state.ts index 1b3466e9fc7..f67d529652f 100644 --- a/src/renderer/src/store/slices/tab-group-state.ts +++ b/src/renderer/src/store/slices/tab-group-state.ts @@ -201,6 +201,32 @@ export function dedupeTabsById(tabs: T[]): T[] { }) } +export function dedupeEditorTabsWithinGroups(tabs: Tab[]): { + tabs: Tab[] + tabIdAliasesByGroup: Map> +} { + const tabIdAliasesByGroup = new Map>() + const editorTabIdByGroupAndEntity = new Map>() + const dedupedTabs = dedupeTabsById(tabs).filter((tab) => { + if (tab.contentType !== 'editor') { + return true + } + const editorTabIdByEntity = + editorTabIdByGroupAndEntity.get(tab.groupId) ?? new Map() + editorTabIdByGroupAndEntity.set(tab.groupId, editorTabIdByEntity) + const existingTabId = editorTabIdByEntity.get(tab.entityId) + if (existingTabId !== undefined) { + const tabIdAliases = tabIdAliasesByGroup.get(tab.groupId) ?? new Map() + tabIdAliasesByGroup.set(tab.groupId, tabIdAliases) + tabIdAliases.set(tab.id, existingTabId) + return false + } + editorTabIdByEntity.set(tab.entityId, tab.id) + return true + }) + return { tabs: dedupedTabs, tabIdAliasesByGroup } +} + export function dedupeTabOrder(tabIds: string[]): string[] { const seen = new Set() const deduped: string[] = [] diff --git a/src/renderer/src/store/slices/tabs-hydration.test.ts b/src/renderer/src/store/slices/tabs-hydration.test.ts index d5b15293f14..3538b796f54 100644 --- a/src/renderer/src/store/slices/tabs-hydration.test.ts +++ b/src/renderer/src/store/slices/tabs-hydration.test.ts @@ -414,9 +414,9 @@ describe('buildHydratedTabState – legacy format', () => { // Why: editor owner migration re-stamped a tab id a sibling record already // held. Two rows under one id repeat a React key and strand a ghost row. const duplicateId = 'editor:wt%3A%3Alungfish:env-a:FINAL-REPORT.md' - const editorTab = (id: string, sortOrder: number) => ({ + const editorTab = (id: string, entityId: string, sortOrder: number) => ({ id, - entityId: 'editor:wt%3A%3Alungfish:env-b:FINAL-REPORT.md', + entityId, groupId: 'g1', worktreeId: 'w1', contentType: 'editor' as const, @@ -429,7 +429,11 @@ describe('buildHydratedTabState – legacy format', () => { const session: WorkspaceSessionState = { ...makeBaseSession(), unifiedTabs: { - w1: [editorTab('t-unique', 0), editorTab(duplicateId, 1), editorTab(duplicateId, 2)] + w1: [ + editorTab('t-unique', 'editor:wt%3A%3Alungfish:env-c:FINAL-REPORT.md', 0), + editorTab(duplicateId, 'editor:wt%3A%3Alungfish:env-b:FINAL-REPORT.md', 1), + editorTab(duplicateId, 'editor:wt%3A%3Alungfish:env-b:FINAL-REPORT.md', 2) + ] }, tabGroups: { w1: [ diff --git a/src/renderer/src/store/slices/tabs-hydration.ts b/src/renderer/src/store/slices/tabs-hydration.ts index c5372133cd6..7edfc667c07 100644 --- a/src/renderer/src/store/slices/tabs-hydration.ts +++ b/src/renderer/src/store/slices/tabs-hydration.ts @@ -2,10 +2,15 @@ import type { Tab, TabGroup, TabGroupLayoutNode } from '../../../../shared/tab-t import type { WorkspaceSessionState } from '../../../../shared/workspace-session-state-types' import { isValidTerminalTabId } from '../../../../shared/terminal-tab-id' import { createBrowserUuid } from '@/lib/browser-uuid' -import { adoptGrouplessTabs, layoutSpanningGroups } from './tab-group-reference-repair' import { + adoptGrouplessTabs, + appendOwnedTabIdsToGroups, + layoutSpanningGroups, + resolveTabGroupOwners +} from './tab-group-reference-repair' +import { + dedupeEditorTabsWithinGroups, dedupeTabOrder, - dedupeTabsById, getPersistedEditFileIdsByWorktree, isTransientEditorContentType, sanitizeRecentTabIds, @@ -66,6 +71,7 @@ function hydrateUnifiedFormat( const groupsByWorktree: Record = {} const activeGroupIdByWorktree: Record = {} const layoutByWorktree: Record = {} + const tabIdAliasesByWorktree: Record>> = {} const persistedEditFileIdsByWorktree = getPersistedEditFileIdsByWorktree(session) for (const [worktreeId, tabs] of Object.entries(session.unifiedTabs!)) { @@ -137,7 +143,9 @@ function hydrateUnifiedFormat( }) .sort((a, b) => a.sortOrder - b.sortOrder || a.createdAt - b.createdAt) // Why after the sort: the surviving record is the one the strip renders first. - tabsByWorktree[worktreeId] = dedupeTabsById(hydratedTabs) + const deduped = dedupeEditorTabsWithinGroups(hydratedTabs) + tabsByWorktree[worktreeId] = deduped.tabs + tabIdAliasesByWorktree[worktreeId] = deduped.tabIdAliasesByGroup } for (const [worktreeId, groups] of Object.entries(session.tabGroups!)) { @@ -148,18 +156,28 @@ function hydrateUnifiedFormat( continue } - const validTabIds = new Set((tabsByWorktree[worktreeId] ?? []).map((t) => t.id)) - const validatedGroups = groups.map((g) => { + const hydratedTabsForWorktree = tabsByWorktree[worktreeId] ?? [] + const tabIdAliasesByGroup = tabIdAliasesByWorktree[worktreeId] + const tabOwners = resolveTabGroupOwners(hydratedTabsForWorktree, groups, tabIdAliasesByGroup) + const validatedGroups = appendOwnedTabIdsToGroups(groups, tabOwners).map((g) => { + const tabIdAliases = tabIdAliasesByGroup?.get(g.id) + const canonicalTabId = (tabId: string): string => tabIdAliases?.get(tabId) ?? tabId // Why: persisted tabOrder can contain duplicates from older buggy // writes. Deduping during hydration restores the store invariant before // later group operations branch on tab counts or neighbors. - const tabOrder = dedupeTabOrder(g.tabOrder.filter((tid) => validTabIds.has(tid))) - const activeTabId = g.activeTabId && validTabIds.has(g.activeTabId) ? g.activeTabId : null + const tabOrder = dedupeTabOrder( + g.tabOrder.map(canonicalTabId).filter((tid) => tabOwners.get(tid) === g.id) + ) + const canonicalActiveTabId = g.activeTabId ? canonicalTabId(g.activeTabId) : null + const activeTabId = + canonicalActiveTabId && tabOwners.get(canonicalActiveTabId) === g.id + ? canonicalActiveTabId + : null // Why: persisted MRU may reference tabs that no longer exist. Sanitize // against the live tabOrder, then ensure the current active tab sits at // the tail so the first close after restore jumps back to the previous // tab rather than falling through to neighbor selection. - const sanitizedRecent = sanitizeRecentTabIds(g.recentTabIds, tabOrder) + const sanitizedRecent = sanitizeRecentTabIds(g.recentTabIds?.map(canonicalTabId), tabOrder) const recentTabIds = activeTabId && sanitizedRecent.at(-1) !== activeTabId ? [...sanitizedRecent.filter((id) => id !== activeTabId), activeTabId] From b81e578cff00ee725cf39e11ef15b1778f8401b6 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 30 Aug 2026 12:10:11 -0700 Subject: [PATCH 07/59] fix(updater): accept GitHub release asset redirects on Windows Accept manual GitHub release-asset redirects on Windows, preserve non-Windows probing, and cover redirect/error/timeout paths. --- src/main/updater-net-request.fixture.ts | 38 ++++ .../updater-prerelease-feed-readiness.test.ts | 172 +++++++++++++++++- src/main/updater-prerelease-feed.test.ts | 11 +- src/main/updater-prerelease-feed.ts | 66 ++++++- src/main/updater.check-failure.test.ts | 10 +- 5 files changed, 283 insertions(+), 14 deletions(-) create mode 100644 src/main/updater-net-request.fixture.ts diff --git a/src/main/updater-net-request.fixture.ts b/src/main/updater-net-request.fixture.ts new file mode 100644 index 00000000000..0a7ec75395e --- /dev/null +++ b/src/main/updater-net-request.fixture.ts @@ -0,0 +1,38 @@ +import { EventEmitter } from 'node:events' +import { vi, type Mock } from 'vitest' + +type NetFetchResponse = { + ok?: boolean + status?: number +} + +export function installNetRequestFetchAdapter(netRequestMock: Mock, netFetchMock: Mock): void { + netRequestMock.mockImplementation( + (options: { method?: string; redirect?: string; url: string }) => { + const request = new EventEmitter() as EventEmitter & { + abort: Mock + end: Mock + } + request.abort = vi.fn() + request.end = vi.fn(() => { + Promise.resolve( + netFetchMock(options.url, { method: options.method, redirect: options.redirect }) + ).then( + (response: NetFetchResponse) => { + const status = response.status ?? (response.ok ? 200 : 503) + if (options.redirect === 'manual' && status >= 300 && status < 400) { + request.emit('redirect', status, options.method ?? 'GET', 'https://redirect.test', {}) + // Why: Electron cancels an unfollowed manual redirect and then emits 'error'. + request.emit('error', new Error('Redirect was cancelled')) + } else { + request.emit('response', { statusCode: status }) + } + }, + (error) => request.emit('error', error) + ) + return request + }) + return request + } + ) +} diff --git a/src/main/updater-prerelease-feed-readiness.test.ts b/src/main/updater-prerelease-feed-readiness.test.ts index 34340c0717e..305384b3896 100644 --- a/src/main/updater-prerelease-feed-readiness.test.ts +++ b/src/main/updater-prerelease-feed-readiness.test.ts @@ -1,12 +1,16 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { installNetRequestFetchAdapter } from './updater-net-request.fixture' import { publishingIncident } from './updater-prerelease-feed-reproduction.fixture' -const { netFetchMock } = vi.hoisted(() => ({ - netFetchMock: vi.fn() +const ORIGINAL_PLATFORM = process.platform + +const { netFetchMock, netRequestMock } = vi.hoisted(() => ({ + netFetchMock: vi.fn(), + netRequestMock: vi.fn() })) vi.mock('electron', () => ({ - net: { fetch: netFetchMock } + net: { fetch: netFetchMock, request: netRequestMock } })) function buildAtomFeed(tags: string[]): string { @@ -33,6 +37,20 @@ function isPlatformManifestRequest(url: string): boolean { return /\/latest(?:-[a-z]+)?\.yml$/.test(url) } +function setPlatformForTest(platform: NodeJS.Platform): void { + Object.defineProperty(process, 'platform', { value: platform }) +} + +function buildWindowsManifest(version: string): string { + return [ + `version: ${version}`, + 'files:', + ' - url: orca-windows-setup.exe', + ' sha512: test', + 'path: orca-windows-setup.exe' + ].join('\n') +} + function respondWithAtom( tags: string[], missingManifestTags: string[] = [], @@ -86,10 +104,158 @@ describe('fetchNewerReleaseTagsWithReadiness', () => { beforeEach(() => { vi.resetModules() netFetchMock.mockReset() + netRequestMock.mockReset() + installNetRequestFetchAdapter(netRequestMock, netFetchMock) }) afterEach(() => { vi.unstubAllGlobals() + vi.useRealTimers() + setPlatformForTest(ORIGINAL_PLATFORM) + }) + + it("offers a Windows release from GitHub's asset redirect without probing Azure", async () => { + setPlatformForTest('win32') + const assetRequestInits: { method?: string; redirect?: string }[] = [] + + netFetchMock.mockImplementation( + (url: string, init?: { method?: string; redirect?: string }) => { + if (url === 'https://github.com/stablyai/orca/releases.atom') { + return Promise.resolve({ + ok: true, + status: 200, + text: () => Promise.resolve(buildAtomFeed(['v1.4.190'])) + }) + } + if (isPlatformManifestRequest(url)) { + return Promise.resolve({ + ok: true, + status: 200, + text: () => Promise.resolve(buildWindowsManifest('1.4.190')) + }) + } + if (init?.method === 'HEAD') { + assetRequestInits.push(init) + return Promise.resolve({ ok: false, status: 302, text: () => Promise.resolve('') }) + } + return Promise.resolve({ ok: false, status: 503, text: () => Promise.resolve('') }) + } + ) + + const { fetchNewerReleaseTagsWithReadiness } = await import('./updater-prerelease-feed') + + await expect(fetchNewerReleaseTagsWithReadiness('1.4.189', 1)).resolves.toEqual({ + tags: ['v1.4.190'], + state: 'ready' + }) + expect(assetRequestInits).toEqual([expect.objectContaining({ redirect: 'manual' })]) + }) + + it.each([301, 307, 308])('accepts a GitHub %s asset redirect as ready', async (status) => { + setPlatformForTest('win32') + netFetchMock.mockImplementation( + (url: string, init?: { method?: string }) => { + if (url === 'https://github.com/stablyai/orca/releases.atom') { + return Promise.resolve({ + ok: true, + status: 200, + text: () => Promise.resolve(buildAtomFeed(['v1.4.190'])) + }) + } + if (isPlatformManifestRequest(url)) { + return Promise.resolve({ + ok: true, + status: 200, + text: () => Promise.resolve(buildWindowsManifest('1.4.190')) + }) + } + if (init?.method === 'HEAD') { + return Promise.resolve({ ok: false, status, text: () => Promise.resolve('') }) + } + return Promise.resolve({ ok: false, status: 503, text: () => Promise.resolve('') }) + } + ) + + const { fetchNewerReleaseTagsWithReadiness } = await import('./updater-prerelease-feed') + + await expect(fetchNewerReleaseTagsWithReadiness('1.4.189', 1)).resolves.toEqual({ + tags: ['v1.4.190'], + state: 'ready' + }) + }) + + it('reports a GitHub asset request error as unavailable', async () => { + setPlatformForTest('win32') + netFetchMock.mockImplementation((url: string, init?: { method?: string }) => { + if (url === 'https://github.com/stablyai/orca/releases.atom') { + return Promise.resolve({ + ok: true, + status: 200, + text: () => Promise.resolve(buildAtomFeed(['v1.4.190'])) + }) + } + if (isPlatformManifestRequest(url)) { + return Promise.resolve({ + ok: true, + status: 200, + text: () => Promise.resolve(buildWindowsManifest('1.4.190')) + }) + } + if (init?.method === 'HEAD') { + return Promise.reject(new Error('network down')) + } + return Promise.resolve({ ok: false, status: 503, text: () => Promise.resolve('') }) + }) + + const { fetchNewerReleaseTagsWithReadiness } = await import('./updater-prerelease-feed') + + await expect(fetchNewerReleaseTagsWithReadiness('1.4.189', 1)).resolves.toEqual({ + tags: [], + state: 'unavailable', + unavailableReason: 'manifest' + }) + }) + + it('aborts a GitHub asset request that exceeds the timeout', async () => { + vi.useFakeTimers() + setPlatformForTest('win32') + let resolveAsset: (() => void) | undefined + const pendingAsset = new Promise<{ ok: boolean; status: number }>((resolve) => { + resolveAsset = () => resolve({ ok: false, status: 503 }) + }) + netFetchMock.mockImplementation((url: string, init?: { method?: string }) => { + if (url === 'https://github.com/stablyai/orca/releases.atom') { + return Promise.resolve({ + ok: true, + status: 200, + text: () => Promise.resolve(buildAtomFeed(['v1.4.190'])) + }) + } + if (isPlatformManifestRequest(url)) { + return Promise.resolve({ + ok: true, + status: 200, + text: () => Promise.resolve(buildWindowsManifest('1.4.190')) + }) + } + if (init?.method === 'HEAD') { + return pendingAsset + } + return Promise.resolve({ ok: false, status: 503, text: () => Promise.resolve('') }) + }) + + const { fetchNewerReleaseTagsWithReadiness } = await import('./updater-prerelease-feed') + const readiness = fetchNewerReleaseTagsWithReadiness('1.4.189', 1) + await vi.advanceTimersByTimeAsync(5000) + + await expect(readiness).resolves.toEqual({ + tags: [], + state: 'unavailable', + unavailableReason: 'manifest' + }) + const request = netRequestMock.mock.results[0]?.value as { abort: ReturnType } + expect(request.abort).toHaveBeenCalledOnce() + resolveAsset?.() }) it('reports not-ready with a verified last-good tag when the newest assets are unavailable', async () => { diff --git a/src/main/updater-prerelease-feed.test.ts b/src/main/updater-prerelease-feed.test.ts index bef53427e22..12e1f9f357a 100644 --- a/src/main/updater-prerelease-feed.test.ts +++ b/src/main/updater-prerelease-feed.test.ts @@ -1,13 +1,15 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { installNetRequestFetchAdapter } from './updater-net-request.fixture' const ORIGINAL_PLATFORM = process.platform -const { netFetchMock } = vi.hoisted(() => ({ - netFetchMock: vi.fn() +const { netFetchMock, netRequestMock } = vi.hoisted(() => ({ + netFetchMock: vi.fn(), + netRequestMock: vi.fn() })) vi.mock('electron', () => ({ - net: { fetch: netFetchMock } + net: { fetch: netFetchMock, request: netRequestMock } })) function buildAtomFeed(tags: string[]): string { @@ -89,6 +91,8 @@ describe('fetchNewerReleaseTag', () => { beforeEach(() => { vi.resetModules() netFetchMock.mockReset() + netRequestMock.mockReset() + installNetRequestFetchAdapter(netRequestMock, netFetchMock) }) afterEach(() => { @@ -158,6 +162,7 @@ describe('fetchNewerReleaseTag', () => { expect(assetUrls).toEqual([ 'https://github.com/stablyai/orca/releases/download/v1.4.1/Orca-1.4.1-arm64-mac.zip' ]) + expect(netRequestMock).toHaveBeenCalledTimes(platform === 'win32' ? 1 : 0) } ) diff --git a/src/main/updater-prerelease-feed.ts b/src/main/updater-prerelease-feed.ts index 6365ccb653d..f6c2909b2b4 100644 --- a/src/main/updater-prerelease-feed.ts +++ b/src/main/updater-prerelease-feed.ts @@ -105,16 +105,70 @@ function getManifestAssetNames(manifestText: string): string[] { type ReleaseReadiness = 'ready' | 'not-ready' | 'unavailable' -async function isReleaseAssetAvailable(tag: string, assetName: string): Promise { +function getGitHubReleaseAssetReadiness(assetUrl: string): Promise { + return new Promise((resolve) => { + const request = net.request({ method: 'HEAD', url: assetUrl, redirect: 'manual' }) + let settled = false + const settle = (readiness: ReleaseReadiness): void => { + if (settled) { + return + } + settled = true + clearTimeout(timeout) + resolve(readiness) + } + const timeout = setTimeout(() => { + try { + request.abort() + } catch { + // The request may already have been cancelled by Electron. + } + settle('unavailable') + }, FETCH_TIMEOUT_MS) + + request.on('redirect', (statusCode) => { + // Why: GitHub's 302 proves the asset exists without probing its signed storage URL. + settle(statusCode >= 300 && statusCode < 400 ? 'ready' : 'unavailable') + }) + request.on('response', (response) => { + settle( + response.statusCode === 404 + ? 'not-ready' + : response.statusCode >= 200 && response.statusCode < 300 + ? 'ready' + : 'unavailable' + ) + }) + request.on('error', () => settle('unavailable')) + try { + request.end() + } catch { + settle('unavailable') + } + }) +} + +async function getReleaseAssetReadiness(tag: string, assetName: string): Promise { + const isRelativeAsset = !/^https?:\/\//i.test(assetName) + const isGitHubReleaseAsset = + process.platform === 'win32' && + (isRelativeAsset || /^https:\/\/github\.com\/stablyai\/orca\/releases\/download\//i.test(assetName)) + const assetUrl = isRelativeAsset + ? getReleaseAssetUrl(tag, assetName.split('/').findLast(Boolean) ?? assetName) + : assetName + if (isGitHubReleaseAsset) { + return getGitHubReleaseAssetReadiness(assetUrl) + } + try { - const assetUrl = assetName.startsWith('http') - ? assetName - : getReleaseAssetUrl(tag, assetName.split('/').findLast(Boolean) ?? assetName) const res = await net.fetch(assetUrl, { method: 'HEAD', signal: AbortSignal.timeout(FETCH_TIMEOUT_MS) }) - return res.status === 404 ? 'not-ready' : res.ok ? 'ready' : 'unavailable' + if (res.status === 404) { + return 'not-ready' + } + return res.ok ? 'ready' : 'unavailable' } catch { return 'unavailable' } @@ -144,7 +198,7 @@ async function getPlatformManifestReadiness(tag: string): Promise isReleaseAssetAvailable(tag, assetName)) + assetNames.map((assetName) => getReleaseAssetReadiness(tag, assetName)) ) return assetResults.includes('not-ready') ? 'not-ready' diff --git a/src/main/updater.check-failure.test.ts b/src/main/updater.check-failure.test.ts index c17dfef4c24..b0ca31a4556 100644 --- a/src/main/updater.check-failure.test.ts +++ b/src/main/updater.check-failure.test.ts @@ -1,7 +1,11 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import { installNetRequestFetchAdapter } from './updater-net-request.fixture' import { publishingIncident } from './updater-prerelease-feed-reproduction.fixture' -const { netFetchMock } = vi.hoisted(() => ({ netFetchMock: vi.fn() })) +const { netFetchMock, netRequestMock } = vi.hoisted(() => ({ + netFetchMock: vi.fn(), + netRequestMock: vi.fn() +})) const { appMock, browserWindowMock, nativeUpdaterMock, autoUpdaterMock, isMock, killAllPtyMock } = vi.hoisted(() => { @@ -75,7 +79,7 @@ vi.mock('electron', () => ({ BrowserWindow: browserWindowMock, autoUpdater: nativeUpdaterMock, powerMonitor: { on: vi.fn() }, - net: { fetch: netFetchMock } + net: { fetch: netFetchMock, request: netRequestMock } })) vi.mock('electron-updater', () => ({ @@ -173,6 +177,8 @@ describe('updater check failure handling', () => { status: 200, text: () => Promise.resolve('') }) + netRequestMock.mockReset() + installNetRequestFetchAdapter(netRequestMock, netFetchMock) }) it('surfaces GitHub release-transition failures with calmer copy and no short retry', async () => { From a8183884bde33abb88daad391c166289c1bb4252 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 30 Aug 2026 12:11:21 -0700 Subject: [PATCH 08/59] perf(wsl): place worktrees inside the distro when the project runs in WSL Fix-forward for readiness review: align retirement placement with WSL mirrors and preserve Windows-side git-common watchers. --- src/main/ipc/filesystem-allowed-roots.ts | 11 ++- ...ktree-base-directory-watch-targets.test.ts | 14 ++++ .../worktree-base-directory-watch-targets.ts | 65 ++++++++------- src/main/ipc/worktree-logic-wsl.test.ts | 82 +++++++++++++++++++ src/main/ipc/worktree-logic.ts | 55 ++++++++++--- src/main/ipc/worktree-remote.ts | 9 +- src/main/local-project-runtime-resolution.ts | 39 +++++++-- src/main/project-runtime-git-options.test.ts | 41 ++++++++++ src/main/project-runtime-git-options.ts | 23 +++++- src/main/runtime/orca-runtime.ts | 10 ++- src/main/worktree-name-retirement.ts | 51 +++++++++--- src/main/worktree-root-preparation.ts | 16 +++- 12 files changed, 351 insertions(+), 65 deletions(-) diff --git a/src/main/ipc/filesystem-allowed-roots.ts b/src/main/ipc/filesystem-allowed-roots.ts index 5dcf7092fc3..3cb7fe4fa55 100644 --- a/src/main/ipc/filesystem-allowed-roots.ts +++ b/src/main/ipc/filesystem-allowed-roots.ts @@ -1,6 +1,7 @@ import { resolve } from 'node:path' import type { Store } from '../persistence' import { computeWorkspaceRoot, getWorktreePathSettings } from './worktree-logic' +import { getWorktreeMirrorDistro } from '../project-runtime-git-options' import { isPathInsideOrEqual } from '../../shared/cross-platform-path' import { getProjectGroupSubtreeIds } from '../../shared/project-groups' import type { FolderWorkspace } from '../../shared/folder-workspace-types' @@ -97,7 +98,15 @@ export function getAllowedRoots(store: Store): string[] { } else { for (const repo of localRepos) { roots.push( - resolve(computeWorkspaceRoot(repo.path, getWorktreePathSettings(repo, settings))) + resolve( + computeWorkspaceRoot( + repo.path, + // Why enriched here too: placement has to agree with the create + // flow, or renderer file access is denied for a worktree Orca + // just put on the WSL side. + getWorktreePathSettings(repo, settings, getWorktreeMirrorDistro(store, repo)) + ) + ) ) } } diff --git a/src/main/ipc/worktree-base-directory-watch-targets.test.ts b/src/main/ipc/worktree-base-directory-watch-targets.test.ts index 250b3d7625d..490311d4bb2 100644 --- a/src/main/ipc/worktree-base-directory-watch-targets.test.ts +++ b/src/main/ipc/worktree-base-directory-watch-targets.test.ts @@ -215,6 +215,20 @@ describe('worktree base directory watch target resolution', () => { expect(getSshFilesystemProviderMock).toHaveBeenCalledWith('missing') }) + // A mirrored layout puts the working trees inside the distro while the gitdir + // stays on the Windows drive, so the UNC root skip must not take git-common with it. + it('keeps the Windows-side git-common watcher when only the workspace root is a WSL UNC path', async () => { + const repo = makeRepo(0, { + path: 'C:\\Users\\alice\\orca', + worktreeBasePath: '\\\\wsl.localhost\\Ubuntu\\home\\alice\\workspaces' + }) + + const targets = await buildWorktreeBaseDirectoryWatchTargets(makeStore([repo]) as never) + + expect([...targets.values()].map((target) => target.kind)).toEqual(['git-common']) + expect([...targets.values()][0]?.path).toBe('C:/Users/alice/orca/.git') + }) + it('does not publish ordered partial targets when a repo resolver rejects unexpectedly', async () => { const completed = makeRepo(0) const broken = makeRepo(1) diff --git a/src/main/ipc/worktree-base-directory-watch-targets.ts b/src/main/ipc/worktree-base-directory-watch-targets.ts index 5300475593d..3e73194e50f 100644 --- a/src/main/ipc/worktree-base-directory-watch-targets.ts +++ b/src/main/ipc/worktree-base-directory-watch-targets.ts @@ -17,6 +17,7 @@ import { import { isWslUncPath } from '../../shared/wsl-paths' import { mapWithConcurrency } from '../../shared/map-with-concurrency' import { getSshFilesystemProvider } from '../providers/ssh-filesystem-dispatch' +import { getWorktreeMirrorDistro } from '../project-runtime-git-options' import { computeWorkspaceRoot, getWorktreePathSettings, @@ -125,25 +126,21 @@ function getBaseWatchLayout( } } +function warnSkippedWslRoot(repoId: string, workspaceRoot: string): void { + if (shouldEmitBoundedWarning(skippedWslWarnings, `${repoId}:${workspaceRoot}`)) { + console.warn(`[worktree-base-watcher] skipping WSL worktree root watcher for ${workspaceRoot}`) + } +} + async function maybeAddBaseTarget( targets: Map, repo: Repo, settings: GlobalSettings, + mirrorDistro: string | undefined, connectionId?: string ): Promise { - const pathSettings = getWorktreePathSettings(repo, settings) + const pathSettings = getWorktreePathSettings(repo, settings, mirrorDistro) const { workspaceRoot, nestWorkspaces } = getBaseWatchLayout(repo, pathSettings, connectionId) - // Why: WSL UNC roots are unreliable for native watching; avoid project-level polling. - if (isWslUncPath(workspaceRoot) || isWslUncPath(repo.path)) { - const key = `${repo.id}:${workspaceRoot}` - if (shouldEmitBoundedWarning(skippedWslWarnings, key)) { - console.warn( - `[worktree-base-watcher] skipping WSL worktree root watcher for ${workspaceRoot}` - ) - } - return - } - const config = { repoId: repo.id, repoName: getRuntimePathBasename(repo.path).replace(/\.git$/, ''), @@ -153,17 +150,28 @@ async function maybeAddBaseTarget( if (connectionId && !remoteProvider) { return } - try { - const rootStat = remoteProvider - ? await remoteProvider.stat(workspaceRoot) - : await stat(workspaceRoot) - if (isDirectoryStat(rootStat)) { - await addTarget(targets, 'base', workspaceRoot, config, connectionId) - } - } catch { - const key = normalizeWatchKey(workspaceRoot) - if (shouldEmitBoundedWarning(missingRootWarnings, key)) { - console.warn(`[worktree-base-watcher] worktree root unavailable: ${workspaceRoot}`) + // Why: WSL UNC paths are unreliable for native watching. A repo inside the + // distro has nothing watchable at all; a Windows-drive repo whose worktrees + // are mirrored into the distro still has its gitdir on the Windows side. + if (isWslUncPath(repo.path)) { + warnSkippedWslRoot(repo.id, workspaceRoot) + return + } + if (isWslUncPath(workspaceRoot)) { + warnSkippedWslRoot(repo.id, workspaceRoot) + } else { + try { + const rootStat = remoteProvider + ? await remoteProvider.stat(workspaceRoot) + : await stat(workspaceRoot) + if (isDirectoryStat(rootStat)) { + await addTarget(targets, 'base', workspaceRoot, config, connectionId) + } + } catch { + const key = normalizeWatchKey(workspaceRoot) + if (shouldEmitBoundedWarning(missingRootWarnings, key)) { + console.warn(`[worktree-base-watcher] worktree root unavailable: ${workspaceRoot}`) + } } } @@ -176,14 +184,15 @@ async function maybeAddBaseTarget( } : undefined ) - if (commonDir) { + if (commonDir && !isWslUncPath(commonDir)) { await addTarget(targets, 'git-common', commonDir, config, connectionId) } } async function resolveRepoTargets( repo: Repo, - settings: GlobalSettings + settings: GlobalSettings, + mirrorDistro: string | undefined ): Promise> { const targets = new Map() if (isFolderRepo(repo)) { @@ -191,9 +200,9 @@ async function resolveRepoTargets( } const executionHostId = getRepoExecutionHostId(repo) if (executionHostId === LOCAL_EXECUTION_HOST_ID) { - await maybeAddBaseTarget(targets, repo, settings) + await maybeAddBaseTarget(targets, repo, settings, mirrorDistro) } else if (repo.connectionId) { - await maybeAddBaseTarget(targets, repo, settings, repo.connectionId) + await maybeAddBaseTarget(targets, repo, settings, mirrorDistro, repo.connectionId) } return targets } @@ -221,7 +230,7 @@ export async function buildWorktreeBaseDirectoryWatchTargets( const resolvedRepoTargets = await mapWithConcurrency( store.getRepos(), WORKTREE_BASE_TARGET_RESOLUTION_CONCURRENCY, - (repo) => resolveRepoTargets(repo, settings) + (repo) => resolveRepoTargets(repo, settings, getWorktreeMirrorDistro(store, repo)) ) const targets = new Map() for (const repoTargets of resolvedRepoTargets) { diff --git a/src/main/ipc/worktree-logic-wsl.test.ts b/src/main/ipc/worktree-logic-wsl.test.ts index 7921fdd1f93..c30c387263e 100644 --- a/src/main/ipc/worktree-logic-wsl.test.ts +++ b/src/main/ipc/worktree-logic-wsl.test.ts @@ -260,4 +260,86 @@ describe('computeWorktreePath WSL layout', () => { }) ).toBe('\\\\wsl.localhost\\Ubuntu\\home\\jin\\custom-worktrees\\feature') }) + + // The C:\ repo + WSL runtime case: git status stats every working-tree file, + // so a tree on the Windows drive costs ~46s across the 9p mount versus ~0.2s + // native with only the gitdir left behind. + describe('Windows-drive repo whose project runs in WSL', () => { + it('places worktrees inside the distro', () => { + parseWslPathMock.mockReturnValue(null) + getWslHomeMock.mockReturnValue('\\\\wsl.localhost\\Ubuntu\\home\\jin') + + expect( + computeWorktreePath('feature', 'C:\\Users\\jin\\repo', { + nestWorkspaces: false, + workspaceDir: 'C:\\workspaces', + wslMirrorDistro: 'Ubuntu' + }) + ).toBe('\\\\wsl.localhost\\Ubuntu\\home\\jin\\orca\\workspaces\\feature') + }) + + it('keeps Windows placement when the project has no WSL runtime', () => { + parseWslPathMock.mockReturnValue(null) + + expect( + computeWorktreePath('feature', 'C:\\Users\\jin\\repo', { + nestWorkspaces: false, + workspaceDir: 'C:\\workspaces' + }) + ).toBe(win32.join('C:\\workspaces', 'feature')) + expect(getWslHomeMock).not.toHaveBeenCalled() + }) + + it('falls back to Windows placement when the distro home cannot be resolved', () => { + parseWslPathMock.mockReturnValue(null) + getWslHomeMock.mockReturnValue(null) + + expect( + computeWorktreePath('feature', 'C:\\Users\\jin\\repo', { + nestWorkspaces: false, + workspaceDir: 'C:\\workspaces', + wslMirrorDistro: 'Ubuntu' + }) + ).toBe(win32.join('C:\\workspaces', 'feature')) + }) + + it('respects an explicit repo-relative workspace dir', () => { + parseWslPathMock.mockReturnValue(null) + + expect( + computeWorktreePath('feature', 'C:\\Users\\jin\\repo', { + nestWorkspaces: false, + workspaceDir: 'worktrees', + wslMirrorDistro: 'Ubuntu' + }) + ).toBe(win32.join('C:\\Users\\jin\\repo', 'worktrees', 'feature')) + expect(getWslHomeMock).not.toHaveBeenCalled() + }) + + it('ignores a stray mirror distro on a POSIX repo path', () => { + parseWslPathMock.mockReturnValue(null) + + expect( + computeWorktreePath('feature', '/Users/jin/repo', { + nestWorkspaces: false, + workspaceDir: '/Users/jin/workspaces', + wslMirrorDistro: 'Ubuntu' + }) + ).toBe('/Users/jin/workspaces/feature') + expect(getWslHomeMock).not.toHaveBeenCalled() + }) + + it('mirrors on the async path too', async () => { + parseWslPathMock.mockReturnValue(null) + getWslHomeAsyncMock.mockResolvedValue('\\\\wsl.localhost\\Ubuntu\\home\\jin') + + await expect( + computeWorktreePathAsync('feature', 'C:\\Users\\jin\\repo', { + nestWorkspaces: false, + workspaceDir: 'C:\\workspaces', + wslMirrorDistro: 'Ubuntu' + }) + ).resolves.toBe('\\\\wsl.localhost\\Ubuntu\\home\\jin\\orca\\workspaces\\feature') + }) + }) }) diff --git a/src/main/ipc/worktree-logic.ts b/src/main/ipc/worktree-logic.ts index d5ccb345742..c71d132bba5 100644 --- a/src/main/ipc/worktree-logic.ts +++ b/src/main/ipc/worktree-logic.ts @@ -7,7 +7,12 @@ import { splitWorktreeId } from '../../shared/worktree/id' import { replaceKnownEmojiWithShortcodes } from '../../shared/emoji-shortcode-catalog' import { getWslHome, getWslHomeAsync, parseWslPath } from '../wsl' -type WorktreePathSettings = Pick +type WorktreePathSettings = Pick & { + /** Distro to mirror the workspace root into when the repo itself sits on a + * Windows drive but this project's git runs in WSL. Omitted = today's + * placement, so any caller that cannot resolve the runtime is unaffected. */ + wslMirrorDistro?: string +} type WorktreeBasePathRepo = Pick export { @@ -132,11 +137,11 @@ export async function computeWorktreePathAsync( async function computeWorkspaceRootAsync( repoPath: string, - settings: { workspaceDir: string } + settings: { workspaceDir: string; wslMirrorDistro?: string } ): Promise { - const wsl = parseWslPath(repoPath) - if (wsl && shouldMirrorWorkspaceDirInsideWsl(repoPath, settings.workspaceDir)) { - const wslHome = await getWslHomeAsync(wsl.distro) + const distro = resolveMirrorDistro(repoPath, settings) + if (distro && shouldMirrorWorkspaceDirInsideWsl(repoPath, settings.workspaceDir)) { + const wslHome = await getWslHomeAsync(distro) if (wslHome) { return win32.join(wslHome, 'orca', 'workspaces') } @@ -144,10 +149,13 @@ async function computeWorkspaceRootAsync( return resolveWorkspaceDirForRepo(repoPath, settings.workspaceDir) } -export function computeWorkspaceRoot(repoPath: string, settings: { workspaceDir: string }): string { - const wsl = parseWslPath(repoPath) - if (wsl && shouldMirrorWorkspaceDirInsideWsl(repoPath, settings.workspaceDir)) { - const wslHome = getWslHome(wsl.distro) +export function computeWorkspaceRoot( + repoPath: string, + settings: { workspaceDir: string; wslMirrorDistro?: string } +): string { + const distro = resolveMirrorDistro(repoPath, settings) + if (distro && shouldMirrorWorkspaceDirInsideWsl(repoPath, settings.workspaceDir)) { + const wslHome = getWslHome(distro) if (wslHome) { // Why: WSL UNC paths are still Windows paths from Node's perspective. // Mirror absolute local desktop workspace roots inside the distro so @@ -180,11 +188,16 @@ export function computeRemoteWorktreePath( export function getWorktreePathSettings( repo: WorktreeBasePathRepo, - settings: WorktreePathSettings + settings: WorktreePathSettings, + wslMirrorDistro?: string ): WorktreePathSettings { return { nestWorkspaces: settings.nestWorkspaces, - workspaceDir: getEffectiveWorktreeBasePath(repo, settings) + workspaceDir: getEffectiveWorktreeBasePath(repo, settings), + // Why pass it through rather than resolve here: placement has to agree + // across create, allowed-roots and watch-targets, so the distro is + // resolved once by the caller that owns the store and threaded down. + ...(wslMirrorDistro ? { wslMirrorDistro } : {}) } } @@ -238,6 +251,26 @@ function getRepoWorktreeBasePath(repo: Pick): string | return trimmed || undefined } +/** + * Which distro's filesystem this repo's worktrees belong on, if any. + * + * A repo already inside WSL names its own distro. A repo on a Windows drive + * names none — but if this project's git runs in WSL, its worktrees still + * belong on the Linux side: `git status` stats every working-tree file, and + * doing that across the 9p mount is ~20x slower than the same clean tree on + * ext4 (`git worktree add` ~26x), with only the gitdir left on the Windows drive. + */ +function resolveMirrorDistro( + repoPath: string, + settings: { wslMirrorDistro?: string } +): string | undefined { + const wsl = parseWslPath(repoPath) + if (wsl) { + return wsl.distro + } + return isWindowsAbsolutePathLike(repoPath) ? settings.wslMirrorDistro : undefined +} + function shouldMirrorWorkspaceDirInsideWsl(repoPath: string, workspaceDir: string): boolean { if (isWorkspaceDirRelativeToRepo(repoPath, workspaceDir)) { return false diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index a189f091900..88e4942c94f 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -133,7 +133,8 @@ import { } from '../agent-trust-presets' import { getLocalProjectGitExecOptions, - getLocalProjectWorktreeGitOptions + getLocalProjectWorktreeGitOptions, + getWorktreeMirrorDistro } from '../project-runtime-git-options' import { getBranchNameOverrideCandidate, @@ -1996,7 +1997,11 @@ export async function createLocalWorktree( ): Promise { const timing = createWorktreeCreateTimingRecorder() const settings = store.getSettings() - const worktreePathSettings = getWorktreePathSettings(repo, settings) + const worktreePathSettings = getWorktreePathSettings( + repo, + settings, + getWorktreeMirrorDistro(store, repo) + ) const localGitExecOptions = getLocalProjectGitExecOptions(store, repo) const localWorktreeGitOptions = getLocalProjectWorktreeGitOptions(store, repo) const hasLocalWorktreeGitOptions = Object.keys(localWorktreeGitOptions).length > 0 diff --git a/src/main/local-project-runtime-resolution.ts b/src/main/local-project-runtime-resolution.ts index 6f3e7b57f09..b6886dcc339 100644 --- a/src/main/local-project-runtime-resolution.ts +++ b/src/main/local-project-runtime-resolution.ts @@ -1,4 +1,5 @@ import type { Store } from './persistence' +import type { GlobalSettings } from '../shared/global-settings-types' import type { Project } from '../shared/project-types' import type { Repo } from '../shared/repo-types' import { @@ -14,18 +15,42 @@ import { import { getRepoIdFromWorktreeId } from '../shared/worktree/id' import { getRepoExecutionHostId, LOCAL_EXECUTION_HOST_ID } from '../shared/execution-host' -function canResolveProjectRuntimeForRepo(store: Store): boolean { +/** + * The slice of the store runtime resolution actually reads. Structural rather + * than the full `Store` so narrowed stores -- the Orca runtime's `RuntimeStore`, + * worktree root preparation -- resolve the same runtime the create path does + * instead of silently falling back to host placement. + * + * Members stay optional because those stores declare them optional; the + * `typeof` guards below are what actually decide whether resolution can run. + */ +export type ProjectRuntimeResolutionStore = { + getProjects?: Store['getProjects'] + getRepo?: Store['getRepo'] + getSettings?: () => Partial> +} + +type ResolvableStore = ProjectRuntimeResolutionStore & { + getProjects: NonNullable + getSettings: NonNullable +} + +function canResolveProjectRuntimeForRepo( + store: ProjectRuntimeResolutionStore +): store is ResolvableStore { return typeof store.getProjects === 'function' && typeof store.getSettings === 'function' } -function canResolveProjectRuntimeForWorktreeId(store: Store): boolean { +function canResolveProjectRuntimeForWorktreeId( + store: ProjectRuntimeResolutionStore +): store is ResolvableStore & { getRepo: NonNullable } { return canResolveProjectRuntimeForRepo(store) && typeof store.getRepo === 'function' } function resolveLocalProjectRuntime( - store: Store, + store: ResolvableStore, project: Project, - settings: ReturnType = store.getSettings() + settings: ReturnType = store.getSettings() ): ProjectExecutionRuntimeResolution { const wslAvailable = hasCachedWslAvailability() ? (getCachedWslAvailability() ?? undefined) @@ -42,7 +67,7 @@ function resolveLocalProjectRuntime( } export function resolveLocalProjectRuntimeForRepo( - store: Store, + store: ProjectRuntimeResolutionStore, repo: Repo ): ProjectExecutionRuntimeResolution | undefined { if ( @@ -59,7 +84,7 @@ export function resolveLocalProjectRuntimeForRepo( } export function resolveLocalProjectRuntimesForRepos( - store: Store, + store: ProjectRuntimeResolutionStore, repos: readonly Repo[] ): ReadonlyMap { const runtimeByRepoId = new Map() @@ -93,7 +118,7 @@ export function resolveLocalProjectRuntimesForRepos( } export function resolveLocalProjectRuntimeForWorktreeId( - store: Store | undefined, + store: ProjectRuntimeResolutionStore | undefined, worktreeId: string | undefined ): ProjectExecutionRuntimeResolution | undefined { if (!store || !worktreeId) { diff --git a/src/main/project-runtime-git-options.test.ts b/src/main/project-runtime-git-options.test.ts index bff4694acb0..9b247e3bca1 100644 --- a/src/main/project-runtime-git-options.test.ts +++ b/src/main/project-runtime-git-options.test.ts @@ -4,6 +4,7 @@ import type { Project } from '../shared/project-types' import type { Repo } from '../shared/repo-types' import { getLocalProjectGitExecOptions, + getWorktreeMirrorDistro, resolveLocalProjectRuntimeForRepo } from './project-runtime-git-options' import { _resetWslCachesForTests, _setWslCachesForTests } from './wsl' @@ -135,6 +136,46 @@ describe('project runtime git options', () => { expect(runtime).toBeUndefined() }) + describe('getWorktreeMirrorDistro', () => { + it('names the distro a resolved WSL project runs in', () => { + _setWslCachesForTests({ available: true, distros: ['Ubuntu'] }) + const project = makeProject({ + localWindowsRuntimePreference: { kind: 'wsl', distro: 'Ubuntu' } + }) + + expect( + withPlatform('win32', () => getWorktreeMirrorDistro(makeStore(project), makeRepo())) + ).toBe('Ubuntu') + }) + + it('names no distro for a host-runtime project', () => { + const project = makeProject({ localWindowsRuntimePreference: { kind: 'windows-host' } }) + + expect( + withPlatform('win32', () => getWorktreeMirrorDistro(makeStore(project), makeRepo())) + ).toBeUndefined() + }) + + // Placement must not throw where git execution does: a project awaiting + // repair still gets a worktree, on the Windows side as it always has. + it('names no distro instead of throwing when the runtime needs repair', () => { + _setWslCachesForTests({ available: true, distros: ['Debian'] }) + const project = makeProject({ + localWindowsRuntimePreference: { kind: 'wsl', distro: 'Ubuntu' } + }) + + expect( + withPlatform('win32', () => getWorktreeMirrorDistro(makeStore(project), makeRepo())) + ).toBeUndefined() + }) + + it('names no distro when the store cannot resolve projects', () => { + _setWslCachesForTests({ available: true, distros: ['Ubuntu'] }) + + expect(withPlatform('win32', () => getWorktreeMirrorDistro({}, makeRepo()))).toBeUndefined() + }) + }) + it('does not apply local Windows runtime routing to runtime-owned repos', () => { const project = makeProject({ localWindowsRuntimePreference: { kind: 'wsl', distro: 'Ubuntu' } diff --git a/src/main/project-runtime-git-options.ts b/src/main/project-runtime-git-options.ts index 32399aa5976..aafdd38829c 100644 --- a/src/main/project-runtime-git-options.ts +++ b/src/main/project-runtime-git-options.ts @@ -1,6 +1,9 @@ import type { Store } from './persistence' import type { Repo } from '../shared/repo-types' -import { resolveLocalProjectRuntimeForRepo } from './local-project-runtime-resolution' +import { + resolveLocalProjectRuntimeForRepo, + type ProjectRuntimeResolutionStore +} from './local-project-runtime-resolution' import type { ProjectExecutionRuntimeResolution } from '../shared/project-execution-runtime' export { @@ -64,3 +67,21 @@ export function getLocalProjectWorktreeGitOptionsForRuntime( const { wslDistro } = getLocalProjectGitExecOptionsForRuntime(repo, projectRuntime) return wslDistro ? { wslDistro } : {} } + +/** + * Distro whose filesystem this repo's worktrees belong on, or undefined. + * + * Deliberately non-throwing where `getLocalProjectGitExecOptions` throws: a + * runtime that needs repair must not block creating a worktree, it just falls + * back to the Windows-side placement that has always been used. + */ +export function getWorktreeMirrorDistro( + store: ProjectRuntimeResolutionStore, + repo: Repo +): string | undefined { + const projectRuntime = resolveLocalProjectRuntimeForRepo(store, repo) + if (!projectRuntime || projectRuntime.status !== 'resolved') { + return undefined + } + return projectRuntime.runtime.kind === 'wsl' ? projectRuntime.runtime.distro : undefined +} diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 8b3793581d6..790fd3dcf53 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -957,6 +957,7 @@ import { createStackedHostedReview as createStackedHostedReviewFromRepo } from ' import { getLocalProjectGitExecOptions, getLocalProjectWorktreeGitOptions, + getWorktreeMirrorDistro, getLocalProjectWorktreeGitOptionsForRuntime, resolveLocalProjectRuntimeForRepo, resolveLocalProjectRuntimesForRepos @@ -1460,6 +1461,9 @@ type RuntimeStore = { getSettings(): { workspaceDir: string nestWorkspaces: boolean + // Read by worktree placement: decides whether this project's worktrees + // mirror into a WSL distro instead of the Windows drive. + localWindowsRuntimeDefault?: GlobalSettings['localWindowsRuntimeDefault'] refreshLocalBaseRefOnWorktreeCreate: boolean localBaseRefSuggestionDismissed?: boolean branchPrefix: string @@ -26773,7 +26777,11 @@ export class OrcaRuntimeService { } } const settings = createSettings - const worktreePathSettings = getWorktreePathSettings(repo, settings) + const worktreePathSettings = getWorktreePathSettings( + repo, + settings, + getWorktreeMirrorDistro(this.requireStore(), repo) + ) const localGitExecOptions = getLocalProjectGitExecOptions(this.requireStore(), repo) const localWorktreeGitOptions = getLocalProjectWorktreeGitOptions(this.requireStore(), repo) const hasLocalWorktreeGitOptions = hasLocalGitOptions(localWorktreeGitOptions) diff --git a/src/main/worktree-name-retirement.ts b/src/main/worktree-name-retirement.ts index 8839d877a95..58ffc99bbfd 100644 --- a/src/main/worktree-name-retirement.ts +++ b/src/main/worktree-name-retirement.ts @@ -30,23 +30,40 @@ import { import { discoverRetiredWorktreeNames } from './worktree-retirement-discovery' import { runRetirementBackfillScan } from './worktree-retirement-backfill-scan' import { hasCachedWslHome, parseWslPath } from './wsl' +import { getWorktreeMirrorDistro } from './project-runtime-git-options' +import type { ProjectRuntimeResolutionStore } from './local-project-runtime-resolution' const RETIREMENT_PROBE_NAME = 'orca-retirement-probe' -type RetirementReadStore = { +type RetirementRuntimeStore = { + getProjects?: ProjectRuntimeResolutionStore['getProjects'] + getSettings?: ProjectRuntimeResolutionStore['getSettings'] +} +type RetirementReadStore = RetirementRuntimeStore & { getRetiredWorktreeNameRegistry(repoId: string): RetiredNameRegistry getRetiredWorktreeNameRegistryForNamespace?(namespaceKey: string): RetiredNameRegistry getSshTarget?: SshTargetLookup } -type RetirementBackfillStore = { +type RetirementBackfillStore = RetirementRuntimeStore & { mergeRetiredWorktreeNames(repoId: string, names: Iterable): boolean } -type RetirementWriteStore = { +type RetirementWriteStore = RetirementRuntimeStore & { addRetiredWorktreeName(repoId: string, name: string): void mergeRetiredWorktreeNamesForNamespace?(namespaceKey: string, names: Iterable): boolean getSshTarget?: SshTargetLookup } -type RetirementPathSettings = Pick +type RetirementPathSettings = Pick & { + wslMirrorDistro?: string +} + +function withMirrorDistro( + store: RetirementRuntimeStore, + repo: Repo, + settings: RetirementPathSettings +): RetirementPathSettings { + const distro = getWorktreeMirrorDistro(store, repo) + return distro ? { ...settings, wslMirrorDistro: distro } : settings +} /** Only canonical generator output is persisted. Collision retries advance canonical tiers, so a * repeat-suffixed path can never be generated again and needs no permanent registry entry. */ @@ -109,7 +126,8 @@ async function getRetirementCollisionKey( repo.path, repo.worktreeBasePath ?? '', settings.workspaceDir, - settings.nestWorkspaces ? 'nested' : 'flat' + settings.nestWorkspaces ? 'nested' : 'flat', + settings.wslMirrorDistro ?? '' ].join('\u0000') const cached = collisionKeyCache.get(cacheKey) if (cached !== undefined) { @@ -172,15 +190,16 @@ export async function getRetiredNameRegistryForRepo( return EMPTY_RETIRED_NAME_REGISTRY } const lookup = sshTargetLookup(store) + const pathSettings = withMirrorDistro(store, repo, settings) let collisionKey: string | null = null try { - collisionKey = await ensureRetiredWorktreeNamesBackfilled(store, repo, settings) + collisionKey = await ensureRetiredWorktreeNamesBackfilled(store, repo, pathSettings) } catch (error) { console.warn(`[worktrees] retirement backfill failed for repo ${repo.id}:`, error) } let registry = store.getRetiredWorktreeNameRegistry(repo.id) if (store.getRetiredWorktreeNameRegistryForNamespace) { - collisionKey ??= await getRetirementCollisionKey(repo, settings, lookup) + collisionKey ??= await getRetirementCollisionKey(repo, pathSettings, lookup) registry = mergeRetiredNameRegistries( registry, readNamespaceRegistry(store, repo, collisionKey, lookup) @@ -194,8 +213,14 @@ export async function getRetiredNameRegistryForRepo( if (isEmptyRetiredNameRegistry(candidateRegistry)) { continue } - collisionKey ??= await getRetirementCollisionKey(repo, settings, lookup) - if ((await getRetirementCollisionKey(candidate, settings, lookup)) !== collisionKey) { + collisionKey ??= await getRetirementCollisionKey(repo, pathSettings, lookup) + if ( + (await getRetirementCollisionKey( + candidate, + withMirrorDistro(store, candidate, settings), + lookup + )) !== collisionKey + ) { continue } registry = mergeRetiredNameRegistries(registry, candidateRegistry) @@ -226,7 +251,11 @@ export async function retireGeneratedWorktreeName( return } try { - const namespaceKey = await getRetirementCollisionKey(repo, settings, sshTargetLookup(store)) + const namespaceKey = await getRetirementCollisionKey( + repo, + withMirrorDistro(store, repo, settings), + sshTargetLookup(store) + ) store.mergeRetiredWorktreeNamesForNamespace(namespaceKey, [name]) } catch (error) { console.warn(`[worktrees] failed to persist retirement namespace for ${repo.id}:`, error) @@ -260,7 +289,7 @@ export async function ensureRetiredWorktreeNamesBackfilled( const probePath = await computeWorktreePathAsync( RETIREMENT_PROBE_NAME, repo.path, - getWorktreePathSettings(repo, settings) + getWorktreePathSettings(repo, settings, settings.wslMirrorDistro) ) const scanKey = `${getRepoExecutionHostId(repo)}:${worktreePathComparisonKey(probePath)}` const names = await runRetirementBackfillScan(store, scanKey, () => diff --git a/src/main/worktree-root-preparation.ts b/src/main/worktree-root-preparation.ts index d4cf7ead15a..d2583106e93 100644 --- a/src/main/worktree-root-preparation.ts +++ b/src/main/worktree-root-preparation.ts @@ -4,15 +4,22 @@ import type { Repo } from '../shared/repo-types' import { getRepoExecutionHostId, LOCAL_EXECUTION_HOST_ID } from '../shared/execution-host' import { isFolderRepo } from '../shared/repo-kind' import { computeWorkspaceRoot, getWorktreePathSettings } from './ipc/worktree-logic' +import { getWorktreeMirrorDistro } from './project-runtime-git-options' +import type { ProjectRuntimeResolutionStore } from './local-project-runtime-resolution' -type WorktreeRootPreparationSettings = Pick +// `localWindowsRuntimeDefault` is listed because the mirror distro is resolved +// from it plus the project catalog: a store that omits it prepares a different +// root than the create path uses. +type WorktreeRootPreparationSettings = Pick & + Partial> type WorktreeRootPreparationStore = { getSettings: () => WorktreeRootPreparationSettings getRepos: () => Repo[] + getProjects?: ProjectRuntimeResolutionStore['getProjects'] } export async function prepareLocalWorktreeRootForRepo( - store: Pick, + store: Pick, repo: Repo ): Promise { if (getRepoExecutionHostId(repo) !== LOCAL_EXECUTION_HOST_ID || isFolderRepo(repo)) { @@ -20,7 +27,10 @@ export async function prepareLocalWorktreeRootForRepo( } try { - const root = computeWorkspaceRoot(repo.path, getWorktreePathSettings(repo, store.getSettings())) + const root = computeWorkspaceRoot( + repo.path, + getWorktreePathSettings(repo, store.getSettings(), getWorktreeMirrorDistro(store, repo)) + ) // Why: mkdir touches the current root to preflight macOS TCC, while // access remains scoped by recomputed settings instead of a permanent grant. await mkdir(root, { recursive: true }) From 3ab9766e38dc28c1b3c738b2be4b2def7c377c4e Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 30 Aug 2026 12:12:04 -0700 Subject: [PATCH 09/59] perf(worktree): prepare checkouts while the composer is open Squashed merge of PR #17290. --- src/main/git/worktree-add.ts | 122 +++++--- ...rktree-create-preparation-real-git.test.ts | 127 ++++++++ src/main/git/worktree-create-preparation.ts | 270 +++++++++++++++++ src/main/git/worktree-listing.ts | 16 +- src/main/git/worktree-operation-options.ts | 1 + src/main/git/worktree-scan-cache.ts | 2 +- src/main/git/worktree.ts | 6 + src/main/host-tree-removal.ts | 15 +- src/main/ipc/worktree-remote.ts | 18 ++ .../register-worktree-prefetch-handler.ts | 10 +- src/main/local-worktree-filesystem.test.ts | 22 ++ src/main/local-worktree-filesystem.ts | 2 +- src/main/runtime/orca-runtime.ts | 34 ++- src/main/worktree-create-base-prefetch.ts | 21 +- src/main/worktree-create-preparation.test.ts | 214 +++++++++++++ src/main/worktree-create-preparation.ts | 283 ++++++++++++++++++ src/shared/git-binary-compatibility.test.ts | 30 ++ .../worktree/create-preparation.test.ts | 57 ++++ src/shared/worktree/create-preparation.ts | 42 +++ .../worktree-create-speculation-bench.mjs | 155 ++++++++++ 20 files changed, 1384 insertions(+), 63 deletions(-) create mode 100644 src/main/git/worktree-create-preparation-real-git.test.ts create mode 100644 src/main/git/worktree-create-preparation.ts create mode 100644 src/main/worktree-create-preparation.test.ts create mode 100644 src/main/worktree-create-preparation.ts create mode 100644 src/shared/worktree/create-preparation.test.ts create mode 100644 src/shared/worktree/create-preparation.ts create mode 100644 tests/tools/benchmarks/worktree-create-speculation-bench.mjs diff --git a/src/main/git/worktree-add.ts b/src/main/git/worktree-add.ts index 4110a4bda0b..8b43cdcca85 100644 --- a/src/main/git/worktree-add.ts +++ b/src/main/git/worktree-add.ts @@ -19,7 +19,46 @@ import type { import { gitExecOptions, resolveWorktreeAddTimeoutMs } from './worktree-operation-options' import { bumpWorktreeScanGeneration } from './worktree-scan-cache' -async function persistWorktreeCreationBase( +export type WorktreeAddBaseContext = AddWorktreeResult & { + effectiveBase: string +} + +export async function resolveWorktreeAddBaseContext( + repoPath: string, + baseBranch: string, + refreshLocalBaseRef: boolean, + options: AddWorktreeOptions +): Promise { + const effectiveBase = await resolveWorktreeAddBaseRef(baseBranch, (qualifiedRef) => + hasWorktreeBaseCommitRef(repoPath, qualifiedRef, options) + ) + const localBaseRefRefresh = refreshLocalBaseRef + ? await refreshLocalBaseRefForWorktreeCreate( + repoPath, + baseBranch, + effectiveBase, + options.remoteTrackingBase, + options + ) + : undefined + const localBaseRefUpdateSuggestion = + !refreshLocalBaseRef && options.suggestLocalBaseRefUpdate + ? await getLocalBaseRefUpdateSuggestionForWorktreeCreate( + repoPath, + baseBranch, + effectiveBase, + options.remoteTrackingBase, + options + ) + : undefined + return { + effectiveBase, + ...(localBaseRefRefresh ? { localBaseRefRefresh } : {}), + ...(localBaseRefUpdateSuggestion ? { localBaseRefUpdateSuggestion } : {}) + } +} + +export async function persistWorktreeCreationBase( worktreePath: string, branch: string, effectiveBase: string, @@ -46,6 +85,35 @@ async function persistWorktreeCreationBase( } } +export async function configurePushAutoSetupRemote( + worktreePath: string, + options: GitWorktreeExecOptions +): Promise { + try { + // Why: `--get` (not `--local --get`) treats a value at any scope as an explicit user choice. + let alreadySet = false + try { + await gitExecFileAsync(['config', '--get', 'push.autoSetupRemote'], { + ...gitExecOptions(worktreePath, options) + }) + alreadySet = true + } catch (readError) { + // Why: exit 1 means unset; other codes are real read failures and must not overwrite config. + const code = (readError as { code?: unknown })?.code + if (code !== 1) { + throw readError + } + } + if (!alreadySet) { + await gitExecFileAsync(['config', '--local', 'push.autoSetupRemote', 'true'], { + ...gitExecOptions(worktreePath, options) + }) + } + } catch (error) { + console.warn(`addWorktree: failed to set push.autoSetupRemote for ${worktreePath}`, error) + } +} + export async function unsetWorktreeCreationBase( worktreePath: string, branch: string, @@ -120,27 +188,15 @@ async function performAddWorktree( // Why: --no-track avoids inheriting the base's upstream so `git status` won't misreport "behind by N" pre-publish; first push sets it (see push.autoSetupRemote below). args.push('--no-track', '-b', branch, worktreePath) if (baseBranch) { - effectiveBase = await resolveWorktreeAddBaseRef(baseBranch, (qualifiedRef) => - hasWorktreeBaseCommitRef(repoPath, qualifiedRef, options) + const baseContext = await resolveWorktreeAddBaseContext( + repoPath, + baseBranch, + refreshLocalBaseRef, + options ) - // Why: resolve the creation base first to distinguish remote-tracking refs from slash-containing local branches (mutation gated behind the explicit setting). - if (refreshLocalBaseRef) { - localBaseRefRefresh = await refreshLocalBaseRefForWorktreeCreate( - repoPath, - baseBranch, - effectiveBase, - options.remoteTrackingBase, - options - ) - } else if (options.suggestLocalBaseRefUpdate) { - localBaseRefUpdateSuggestion = await getLocalBaseRefUpdateSuggestionForWorktreeCreate( - repoPath, - baseBranch, - effectiveBase, - options.remoteTrackingBase, - options - ) - } + effectiveBase = baseContext.effectiveBase + localBaseRefRefresh = baseContext.localBaseRefRefresh + localBaseRefUpdateSuggestion = baseContext.localBaseRefUpdateSuggestion args.push(effectiveBase) } } @@ -163,29 +219,7 @@ async function performAddWorktree( // `git push` create+set origin/ (git >=2.37; older clients ignore it). `--local` on a // linked worktree writes the shared common-dir config (whole repo) — intentional and idempotent, // so it's warn-only and not rolled back on failure. - try { - // Why: `--get` (not `--local --get`) so a value at any scope counts as "user already chose" and isn't overwritten. - let alreadySet = false - try { - await gitExecFileAsync(['config', '--get', 'push.autoSetupRemote'], { - ...gitExecOptions(worktreePath, options) - }) - alreadySet = true - } catch (readError) { - // Why: `git config --get` exits 1 only when unset at every scope; any other code is a real read failure — rethrow rather than overwrite the user's value. - const code = (readError as { code?: unknown })?.code - if (code !== 1) { - throw readError - } - } - if (!alreadySet) { - await gitExecFileAsync(['config', '--local', 'push.autoSetupRemote', 'true'], { - ...gitExecOptions(worktreePath, options) - }) - } - } catch (error) { - console.warn(`addWorktree: failed to set push.autoSetupRemote for ${worktreePath}`, error) - } + await configurePushAutoSetupRemote(worktreePath, options) return { ...(localBaseRefRefresh ? { localBaseRefRefresh } : {}), ...(localBaseRefUpdateSuggestion ? { localBaseRefUpdateSuggestion } : {}) diff --git a/src/main/git/worktree-create-preparation-real-git.test.ts b/src/main/git/worktree-create-preparation-real-git.test.ts new file mode 100644 index 00000000000..bdb1e9fcc4b --- /dev/null +++ b/src/main/git/worktree-create-preparation-real-git.test.ts @@ -0,0 +1,127 @@ +import { execFileSync } from 'node:child_process' +import { mkdir, mkdtemp, readFile, realpath, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { + createWorktreePreparationLockReason, + isWorktreeCreatePreparation, + WORKTREE_CREATE_PREPARATION_DIRECTORY +} from '../../shared/worktree/create-preparation' +import { listWorktrees } from './worktree' +import { + discardPreparedWorktree, + finalizePreparedWorktree, + prepareWorktreeCreateCheckout +} from './worktree-create-preparation' +import { areWorktreePathsEqual } from './worktree-path-comparison' + +const tempRoots: string[] = [] + +function git(cwd: string, args: string[]): string { + return execFileSync('git', args, { + cwd, + encoding: 'utf8', + stdio: ['pipe', 'pipe', 'pipe'] + }).trim() +} + +async function createRepo(): Promise<{ repoPath: string; root: string }> { + const root = await mkdtemp(join(tmpdir(), 'orca-prepared-worktree-')) + tempRoots.push(root) + const repoPath = join(root, 'repo') + execFileSync('git', ['init', '--quiet', repoPath]) + git(repoPath, ['symbolic-ref', 'HEAD', 'refs/heads/main']) + git(repoPath, ['config', 'user.email', 'test@example.com']) + git(repoPath, ['config', 'user.name', 'Test User']) + git(repoPath, ['config', 'core.autocrlf', 'false']) + await writeFile(join(repoPath, 'version.txt'), 'one\n') + git(repoPath, ['add', 'version.txt']) + git(repoPath, ['commit', '--quiet', '-m', 'initial']) + return { repoPath, root } +} + +afterEach(async () => { + await Promise.all(tempRoots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +describe('prepared worktree creation with real Git', () => { + it('cleans up when the create signal is canceled', async () => { + const { repoPath, root } = await createRepo() + const preparationRoot = join(root, WORKTREE_CREATE_PREPARATION_DIRECTORY) + const preparedPath = join(preparationRoot, `${process.pid}-canceled`) + await mkdir(preparationRoot, { recursive: true }) + + await prepareWorktreeCreateCheckout( + repoPath, + preparedPath, + 'main', + createWorktreePreparationLockReason('canceled-test') + ) + + const controller = new AbortController() + controller.abort() + await expect( + discardPreparedWorktree(repoPath, preparedPath, { signal: controller.signal }) + ).resolves.toBeUndefined() + + expect(await listWorktrees(repoPath, { includeCreatePreparations: true })).toHaveLength(1) + }) + + it('hides the preparation, retargets an advanced base, and attaches the final branch', async () => { + const { repoPath, root } = await createRepo() + const preparationRoot = join(root, WORKTREE_CREATE_PREPARATION_DIRECTORY) + const preparedPath = join(preparationRoot, `${process.pid}-test`) + const finalPath = join(root, 'final-worktree') + await mkdir(preparationRoot, { recursive: true }) + + await prepareWorktreeCreateCheckout( + repoPath, + preparedPath, + 'main', + createWorktreePreparationLockReason('real-git-test') + ) + + const visibleBeforeSubmit = await listWorktrees(repoPath) + const allBeforeSubmit = await listWorktrees(repoPath, { includeCreatePreparations: true }) + expect(visibleBeforeSubmit).toHaveLength(1) + expect(allBeforeSubmit).toHaveLength(2) + expect(allBeforeSubmit.find(isWorktreeCreatePreparation)).toMatchObject({ + locked: true, + lockReason: expect.stringContaining('orca-create-preparation:v1:') + }) + + await writeFile(join(repoPath, 'version.txt'), 'two\n') + git(repoPath, ['add', 'version.txt']) + git(repoPath, ['commit', '--quiet', '-m', 'advance base']) + const latestHead = git(repoPath, ['rev-parse', 'HEAD']) + + await finalizePreparedWorktree( + repoPath, + preparedPath, + finalPath, + 'feature/prepared', + 'main', + false + ) + + expect(git(finalPath, ['rev-parse', 'HEAD'])).toBe(latestHead) + expect(git(finalPath, ['branch', '--show-current'])).toBe('feature/prepared') + expect((await readFile(join(finalPath, 'version.txt'), 'utf8')).replaceAll('\r\n', '\n')).toBe( + 'two\n' + ) + expect(git(finalPath, ['config', '--get', 'branch.feature/prepared.base'])).toBe( + 'refs/heads/main' + ) + expect(git(finalPath, ['config', '--get', 'push.autoSetupRemote'])).toBe('true') + const listedWorktrees = await listWorktrees(repoPath) + const resolvedFinalPath = await realpath(finalPath) + expect( + listedWorktrees.some((worktree) => areWorktreePathsEqual(worktree.path, resolvedFinalPath)) + ).toBe(true) + expect( + listedWorktrees.find((worktree) => areWorktreePathsEqual(worktree.path, resolvedFinalPath)) + ?.locked + ).not.toBe(true) + }) +}) diff --git a/src/main/git/worktree-create-preparation.ts b/src/main/git/worktree-create-preparation.ts new file mode 100644 index 00000000000..626b090a0e0 --- /dev/null +++ b/src/main/git/worktree-create-preparation.ts @@ -0,0 +1,270 @@ +import { windowsLongPathGitArgs } from '../../shared/windows-long-path-git-args' +import { resolveWorktreeAddBaseRef } from '../../shared/worktree/base-ref' +import type { AddWorktreeOptions, AddWorktreeResult, GitWorktreeExecOptions } from './worktree' +import { + configurePushAutoSetupRemote, + notifyPreparedWorktreeMutation, + persistWorktreeCreationBase, + resolveWorktreeAddBaseContext, + resolveWorktreeAddTimeoutMs, + WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS +} from './worktree' +import { hasWorktreeBaseCommitRef } from './worktree-base-ref-probe' +import { gitExecFileAsync } from './runner' +import { runWithGitReadCacheInvalidation } from './status' + +function gitExecOptions( + cwd: string, + options: GitWorktreeExecOptions +): { cwd: string; wslDistro?: string; signal?: AbortSignal; timeout?: number } { + return { + cwd, + ...(options.wslDistro ? { wslDistro: options.wslDistro } : {}), + ...(options.signal ? { signal: options.signal } : {}), + ...(options.timeout ? { timeout: options.timeout } : {}) + } +} + +function gitCleanupOptions( + cwd: string, + options: GitWorktreeExecOptions +): { cwd: string; wslDistro?: string; timeout?: number } { + // Why: cancellation must not strand a partially moved worktree; cleanup is bounded separately. + return gitExecOptions(cwd, { ...options, signal: undefined }) +} + +async function performDiscardPreparedWorktree( + repoPath: string, + worktreePath: string, + options: GitWorktreeExecOptions +): Promise { + const cleanupGitOptions = { + ...gitCleanupOptions(repoPath, options), + timeout: options.timeout ?? WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS + } + try { + await gitExecFileAsync( + [...windowsLongPathGitArgs(repoPath), 'worktree', 'unlock', worktreePath], + cleanupGitOptions + ) + } catch { + // It may be unlocked already or only partially registered. + } + await gitExecFileAsync( + [...windowsLongPathGitArgs(repoPath), 'worktree', 'remove', '--force', worktreePath], + cleanupGitOptions + ) +} + +export async function prepareWorktreeCreateCheckout( + repoPath: string, + worktreePath: string, + baseBranch: string, + lockReason: string, + options: GitWorktreeExecOptions = {} +): Promise { + try { + await runWithGitReadCacheInvalidation(async () => { + const effectiveBase = await resolveWorktreeAddBaseRef(baseBranch, (qualifiedRef) => + hasWorktreeBaseCommitRef(repoPath, qualifiedRef, options) + ) + try { + await gitExecFileAsync( + [ + ...windowsLongPathGitArgs(repoPath), + 'worktree', + 'add', + '--detach', + '--no-checkout', + worktreePath, + effectiveBase + ], + { ...gitExecOptions(repoPath, options), timeout: resolveWorktreeAddTimeoutMs() } + ) + // Why: reset materializes files without running user post-checkout hooks before submit. + await gitExecFileAsync( + [...windowsLongPathGitArgs(worktreePath), 'reset', '--hard', effectiveBase], + { ...gitExecOptions(worktreePath, options), timeout: resolveWorktreeAddTimeoutMs() } + ) + await gitExecFileAsync( + [ + ...windowsLongPathGitArgs(repoPath), + 'worktree', + 'lock', + '--reason', + lockReason, + worktreePath + ], + { ...gitExecOptions(repoPath, options), timeout: resolveWorktreeAddTimeoutMs() } + ) + } catch (error) { + await performDiscardPreparedWorktree(repoPath, worktreePath, options).catch(() => {}) + throw error + } + }) + } finally { + notifyPreparedWorktreeMutation(repoPath) + } +} + +export async function discardPreparedWorktree( + repoPath: string, + worktreePath: string, + options: GitWorktreeExecOptions = {} +): Promise { + try { + await runWithGitReadCacheInvalidation(() => + performDiscardPreparedWorktree(repoPath, worktreePath, options) + ) + } finally { + notifyPreparedWorktreeMutation(repoPath) + } +} + +export async function unlockPreparedWorktree( + repoPath: string, + worktreePath: string, + options: GitWorktreeExecOptions = {} +): Promise { + const cleanupGitOptions = { + ...gitCleanupOptions(repoPath, options), + timeout: options.timeout ?? WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS + } + try { + await runWithGitReadCacheInvalidation(() => + gitExecFileAsync( + [...windowsLongPathGitArgs(repoPath), 'worktree', 'unlock', worktreePath], + cleanupGitOptions + ) + ) + } finally { + notifyPreparedWorktreeMutation(repoPath) + } +} + +async function removeFailedFinalization( + repoPath: string, + cleanupPath: string, + branch: string, + moved: boolean, + options: GitWorktreeExecOptions +): Promise { + let branchAttached = false + if (moved) { + try { + const { stdout } = await gitExecFileAsync( + ['symbolic-ref', '--short', 'HEAD'], + gitCleanupOptions(cleanupPath, options) + ) + branchAttached = stdout.trim() === branch + } catch { + // Detached or no longer readable. + } + } + await performDiscardPreparedWorktree(repoPath, cleanupPath, options).catch(() => {}) + if (branchAttached) { + await gitExecFileAsync( + ['branch', '-D', '--', branch], + gitCleanupOptions(repoPath, options) + ).catch(() => {}) + } +} + +export async function finalizePreparedWorktree( + repoPath: string, + preparedPath: string, + worktreePath: string, + branch: string, + baseBranch: string, + refreshLocalBaseRef = false, + options: AddWorktreeOptions = {} +): Promise { + const finalizeGitOptions: AddWorktreeOptions = { + ...options, + timeout: options.timeout ?? resolveWorktreeAddTimeoutMs() + } + try { + return await runWithGitReadCacheInvalidation(async () => { + const baseContext = await resolveWorktreeAddBaseContext( + repoPath, + baseBranch, + refreshLocalBaseRef, + finalizeGitOptions + ) + const { stdout: targetHeadOutput } = await gitExecFileAsync( + ['rev-parse', '--verify', `${baseContext.effectiveBase}^{commit}`], + gitExecOptions(repoPath, finalizeGitOptions) + ) + const targetHead = targetHeadOutput.trim() + const { stdout: preparedHeadOutput } = await gitExecFileAsync( + ['rev-parse', '--verify', 'HEAD'], + gitExecOptions(preparedPath, finalizeGitOptions) + ) + if (preparedHeadOutput.trim() !== targetHead) { + await gitExecFileAsync( + [...windowsLongPathGitArgs(preparedPath), 'reset', '--hard', targetHead], + gitExecOptions(preparedPath, finalizeGitOptions) + ) + } + + let moved = false + try { + await gitExecFileAsync( + [ + ...windowsLongPathGitArgs(repoPath), + 'worktree', + 'move', + '-f', + '-f', + preparedPath, + worktreePath + ], + gitExecOptions(repoPath, finalizeGitOptions) + ) + moved = true + // Why: `-f -f` moves the locked preparation while preserving its lock reason (Git >=2.25). + await gitExecFileAsync( + [ + ...windowsLongPathGitArgs(worktreePath), + 'checkout', + '--no-track', + '-b', + branch, + targetHead + ], + gitExecOptions(worktreePath, finalizeGitOptions) + ) + await persistWorktreeCreationBase( + worktreePath, + branch, + baseContext.effectiveBase, + finalizeGitOptions + ) + await configurePushAutoSetupRemote(worktreePath, finalizeGitOptions) + await gitExecFileAsync( + [...windowsLongPathGitArgs(repoPath), 'worktree', 'unlock', worktreePath], + gitExecOptions(repoPath, finalizeGitOptions) + ) + } catch (error) { + await removeFailedFinalization( + repoPath, + moved ? worktreePath : preparedPath, + branch, + moved, + finalizeGitOptions + ) + throw error + } + return { + ...(baseContext.localBaseRefRefresh + ? { localBaseRefRefresh: baseContext.localBaseRefRefresh } + : {}), + ...(baseContext.localBaseRefUpdateSuggestion + ? { localBaseRefUpdateSuggestion: baseContext.localBaseRefUpdateSuggestion } + : {}) + } + }) + } finally { + notifyPreparedWorktreeMutation(repoPath) + } +} diff --git a/src/main/git/worktree-listing.ts b/src/main/git/worktree-listing.ts index dd6ee20765f..c2c293678d3 100644 --- a/src/main/git/worktree-listing.ts +++ b/src/main/git/worktree-listing.ts @@ -1,4 +1,5 @@ import { stat } from 'node:fs/promises' +import { isWorktreeCreatePreparation } from '../../shared/worktree/create-preparation' import type { GitWorktreeInfo } from '../../shared/worktree/types' import { readTranslatedWorktreeGraph, readWorktreeList } from './worktree-list-reader' import type { GitWorktreeExecOptions } from './worktree-operation-options' @@ -13,7 +14,10 @@ export async function listWorktreeGraph( options: GitWorktreeExecOptions = {} ): Promise { try { - return await readTranslatedWorktreeGraph(repoPath, options) + const worktrees = await readTranslatedWorktreeGraph(repoPath, options) + return options.includeCreatePreparations + ? worktrees + : worktrees.filter((worktree) => !isWorktreeCreatePreparation(worktree)) } catch (err) { if (getErrorCode(err) === 'ENOENT') { try { @@ -39,7 +43,10 @@ export async function listWorktreesUnshared( ): Promise { try { const worktrees = await readTranslatedWorktreeGraph(repoPath, options) - return annotateSparseCheckoutStatus(worktrees) + const visibleWorktrees = options.includeCreatePreparations + ? worktrees + : worktrees.filter((worktree) => !isWorktreeCreatePreparation(worktree)) + return annotateSparseCheckoutStatus(visibleWorktrees) } catch (err) { if (getErrorCode(err) === 'ENOENT') { try { @@ -68,7 +75,10 @@ export async function listWorktreesStrict( const translatedPath = translateWorktreePath(worktree.path, repoPath, options) return translatedPath === worktree.path ? worktree : { ...worktree, path: translatedPath } }) - return annotateSparseCheckoutStatus(worktrees) + const visibleWorktrees = options.includeCreatePreparations + ? worktrees + : worktrees.filter((worktree) => !isWorktreeCreatePreparation(worktree)) + return annotateSparseCheckoutStatus(visibleWorktrees) } async function annotateSparseCheckoutStatus( diff --git a/src/main/git/worktree-operation-options.ts b/src/main/git/worktree-operation-options.ts index c88c246be9c..9376fe63f85 100644 --- a/src/main/git/worktree-operation-options.ts +++ b/src/main/git/worktree-operation-options.ts @@ -18,6 +18,7 @@ export type GitWorktreeExecOptions = { wslDistro?: string signal?: AbortSignal timeout?: number + includeCreatePreparations?: boolean } export type WorktreeRemovalPreflightOptions = GitWorktreeExecOptions & { diff --git a/src/main/git/worktree-scan-cache.ts b/src/main/git/worktree-scan-cache.ts index 3bb674c2a99..325a8b92d50 100644 --- a/src/main/git/worktree-scan-cache.ts +++ b/src/main/git/worktree-scan-cache.ts @@ -66,7 +66,7 @@ function shareWorktreeScan( const timeout = options.timeout ?? WORKTREE_LIST_TIMEOUT_MS // Why: callers with different deadlines cannot safely share which timeout wins the scan. // Why `run.name`: a strict joiner must never receive a softened `[]` from a lenient scan. - const key = `${repoPath}\0${options.wslDistro ?? ''}\0${timeout}\0${generation}\0${run.name}` + const key = `${repoPath}\0${options.wslDistro ?? ''}\0${timeout}\0${options.includeCreatePreparations === true}\0${generation}\0${run.name}` const inFlight = inFlightWorktreeScans.get(key) if (inFlight) { return inFlight diff --git a/src/main/git/worktree.ts b/src/main/git/worktree.ts index 317585a82e6..78562787a69 100644 --- a/src/main/git/worktree.ts +++ b/src/main/git/worktree.ts @@ -1,4 +1,9 @@ export { addWorktree } from './worktree-add' +export { + configurePushAutoSetupRemote, + persistWorktreeCreationBase, + resolveWorktreeAddBaseContext +} from './worktree-add' export { forceDeleteLocalBranch } from './worktree-branch-removal' export { parseWorktreeList } from './worktree-list-parser' export { listWorktreeGraph, listWorktreesStrict } from './worktree-listing' @@ -25,5 +30,6 @@ export { listWorktrees, listWorktreesSharedStrict } from './worktree-scan-cache' +export { bumpWorktreeScanGeneration as notifyPreparedWorktreeMutation } from './worktree-scan-cache' export { addSparseWorktree } from './worktree-sparse-add' export { parseCoreSparseCheckoutFlag } from './worktree-sparse-state' diff --git a/src/main/host-tree-removal.ts b/src/main/host-tree-removal.ts index dcc8a2f2a43..666d129b27c 100644 --- a/src/main/host-tree-removal.ts +++ b/src/main/host-tree-removal.ts @@ -6,15 +6,28 @@ import type { RmOptions } from 'node:fs' import { rm } from 'node:fs/promises' import { win32 } from 'node:path' import { setTimeout as delay } from 'node:timers/promises' +import { isWindowsAbsolutePathLike } from '../shared/cross-platform-path' +import { isWslUncPath } from '../shared/wsl-paths' const WINDOWS_REMOVE_RETRY_DELAYS_MS = [250, 500, 1_000, 2_000] const WINDOWS_RM_MAX_RETRIES = 8 const WINDOWS_RM_RETRY_DELAY_MS = 150 +/** Convert a native host filesystem path to the Win32 long-path namespace. */ +export function toHostFilesystemPath(targetPath: string): string { + // POSIX paths are used by WSL callers even while the Electron process runs + // on Windows; do not reinterpret those as drive-relative Win32 paths. + return process.platform === 'win32' && + isWindowsAbsolutePathLike(targetPath) && + !isWslUncPath(targetPath) + ? win32.toNamespacedPath(targetPath) + : targetPath +} + export function toHostRemovalPath(targetPath: string): string { // Why: Git for Windows can fail long recursive deletes even after Orca has // proven the worktree target; Node's host deletion should use Win32 long paths. - return process.platform === 'win32' ? win32.toNamespacedPath(targetPath) : targetPath + return toHostFilesystemPath(targetPath) } function getHostRemovalOptions(): RmOptions { diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index 88e4942c94f..66234738128 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -29,6 +29,7 @@ import type { import { getPRForBranch } from '../github/client' import { listWorktrees, addWorktree, addSparseWorktree } from '../git/worktree' import type { AddWorktreeOptions, AddWorktreeResult } from '../git/worktree' +import { consumePreparedWorktreeCreate } from '../worktree-create-preparation' import { getBranchConflictKind, resolveDefaultBaseRefViaExec, @@ -2381,10 +2382,27 @@ export async function createLocalWorktree( ...remoteTrackingBaseOption, ...(suggestLocalBaseRefUpdate ? { suggestLocalBaseRefUpdate } : {}) } + const preparedWorktreeOptions = suggestLocalBaseRefUpdate + ? addProjectGitOptions({ ...remoteTrackingBaseOption, suggestLocalBaseRefUpdate }) + : addProjectGitOptions(remoteTrackingBaseOption) let addResult: AddWorktreeResult try { addResult = (await timing.time('git_worktree_add', async () => { + if (sparseDirectories.length === 0 && !checkoutExistingBranch) { + const preparedResult = await consumePreparedWorktreeCreate({ + repoPath: repo.path, + workspaceRoot, + worktreePath, + branch: branchName, + baseBranch, + refreshLocalBaseRef: settings.refreshLocalBaseRefOnWorktreeCreate, + ...(preparedWorktreeOptions ? { options: preparedWorktreeOptions } : {}) + }) + if (preparedResult) { + return preparedResult + } + } if (sparseDirectories.length > 0) { if (checkoutExistingBranch) { return addSparseWorktree( diff --git a/src/main/ipc/worktrees/create/register-worktree-prefetch-handler.ts b/src/main/ipc/worktrees/create/register-worktree-prefetch-handler.ts index 19b0516f274..849a1ff66d9 100644 --- a/src/main/ipc/worktrees/create/register-worktree-prefetch-handler.ts +++ b/src/main/ipc/worktrees/create/register-worktree-prefetch-handler.ts @@ -1,5 +1,6 @@ import { ipcMain } from 'electron' import { prefetchWorktreeCreateBase } from '../../../worktree-create-base-prefetch' +import { prepareWorktreeCreateForRepo } from '../../../worktree-create-preparation' import type { WorktreeIpcContext } from '../worktree-ipc-context' export function registerWorktreePrefetchHandler(context: WorktreeIpcContext): void { @@ -13,7 +14,14 @@ export function registerWorktreePrefetchHandler(context: WorktreeIpcContext): vo return } try { - await prefetchWorktreeCreateBase({ repo, baseBranch: args.baseBranch, runtime }) + const baseBranch = await prefetchWorktreeCreateBase({ + repo, + baseBranch: args.baseBranch, + runtime + }) + if (baseBranch) { + await prepareWorktreeCreateForRepo(store, repo, baseBranch) + } } catch { // Why: optimistic warm-up; the real create path awaits the same refresh and reports failures there. } diff --git a/src/main/local-worktree-filesystem.test.ts b/src/main/local-worktree-filesystem.test.ts index 9eed498e73d..52b1d16f21a 100644 --- a/src/main/local-worktree-filesystem.test.ts +++ b/src/main/local-worktree-filesystem.test.ts @@ -22,6 +22,7 @@ vi.mock('node:fs/promises', () => ({ import { getLocalWorktreePathAccess, removeLocalWorktreePath, + toHostFilesystemPath, toHostRemovalPath } from './local-worktree-filesystem' @@ -103,6 +104,27 @@ describe('local worktree filesystem runtime access', () => { }) }) + it('uses the same Win32 namespace for host directory creation on Windows', async () => { + await withPlatform('win32', async () => { + const longPath = `C:\\repo\\${'nested\\'.repeat(40)}feature` + + expect(toHostFilesystemPath(longPath)).toBe(`\\\\?\\${longPath}`) + }) + }) + + it('leaves POSIX WSL paths unchanged on a Windows host', async () => { + await withPlatform('win32', async () => { + expect(toHostFilesystemPath('/home/me/worktrees')).toBe('/home/me/worktrees') + }) + }) + + it('leaves WSL UNC paths unchanged on a Windows host', async () => { + await withPlatform('win32', async () => { + const wslPath = String.raw`\\wsl.localhost\Ubuntu\home\me\worktrees` + expect(toHostFilesystemPath(wslPath)).toBe(wslPath) + }) + }) + it('retries transient host removal failures on Windows', async () => { vi.useFakeTimers() await withPlatform('win32', async () => { diff --git a/src/main/local-worktree-filesystem.ts b/src/main/local-worktree-filesystem.ts index 9e4a1dba6a9..f5d8714e196 100644 --- a/src/main/local-worktree-filesystem.ts +++ b/src/main/local-worktree-filesystem.ts @@ -5,7 +5,7 @@ import { removeHostTree } from './host-tree-removal' import { toLinuxPath } from './wsl' import type { ReadPath, StatPath } from './worktree-orphan-gitdir-proof' -export { toHostRemovalPath } from './host-tree-removal' +export { toHostFilesystemPath, toHostRemovalPath } from './host-tree-removal' export type LocalWorktreeFilesystemOptions = { wslDistro?: string diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 790fd3dcf53..77b63f3819e 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -1258,6 +1258,10 @@ import { resolveWorktreeRemovalRepoOwner } from '../worktree-removal-repo-owner' import { prefetchWorktreeCreateBase } from '../worktree-create-base-prefetch' +import { + consumePreparedWorktreeCreate, + prepareWorktreeCreateForRepo +} from '../worktree-create-preparation' import { prepareLocalWorktreeRootForRepo } from '../worktree-root-preparation' import { getWorktreeWatcherRemoval } from '../ipc/worktree-watcher-removal' import { acquireWatcherRemovalGate } from '../ipc/watcher-removal-gate' @@ -26498,11 +26502,18 @@ export class OrcaRuntimeService { } const repo = await this.resolveRepoSelector(args.repoSelector) - await prefetchWorktreeCreateBase({ + const baseBranch = await prefetchWorktreeCreateBase({ repo, baseBranch: args.baseBranch, runtime: this }) + if (baseBranch) { + try { + await prepareWorktreeCreateForRepo(this.requireStore(), repo, baseBranch) + } catch { + // Why: speculative preparation is an optimistic warm-up; the real create path reports failures. + } + } } async createManagedWorktree(args: { @@ -27087,9 +27098,27 @@ export class OrcaRuntimeService { ...(suggestLocalBaseRefUpdate ? { suggestLocalBaseRefUpdate } : {}) } const defaultAddWorktreeOption = addProjectGitOptions() + const preparedWorktreeOptions = suggestLocalBaseRefUpdate + ? addProjectGitOptions({ ...remoteTrackingBaseOption, suggestLocalBaseRefUpdate }) + : remoteTrackingBaseOption + ? addProjectGitOptions(remoteTrackingBaseOption) + : defaultAddWorktreeOption let addResult: AddWorktreeResult try { + const preparedResult = + sparseDirectories.length === 0 && !checkoutExistingBranch + ? await consumePreparedWorktreeCreate({ + repoPath: repo.path, + workspaceRoot, + worktreePath, + branch: branchName, + baseBranch, + refreshLocalBaseRef: settings.refreshLocalBaseRefOnWorktreeCreate, + ...(preparedWorktreeOptions ? { options: preparedWorktreeOptions } : {}) + }) + : null addResult = + preparedResult ?? (await (sparseDirectories.length > 0 ? checkoutExistingBranch ? addSparseWorktree( @@ -27185,7 +27214,8 @@ export class OrcaRuntimeService { branchName, baseBranch, settings.refreshLocalBaseRefOnWorktreeCreate - ))) ?? {} + ))) ?? + {} } catch (error) { if (shouldRetireGeneratedName && failedWorktreeCreationNeedsRetirement(error)) { await retireGeneratedWorktreeName(this.store, repo, settings, effectiveSanitizedName) diff --git a/src/main/worktree-create-base-prefetch.ts b/src/main/worktree-create-base-prefetch.ts index a7665fc4e15..50c81ed1959 100644 --- a/src/main/worktree-create-base-prefetch.ts +++ b/src/main/worktree-create-base-prefetch.ts @@ -44,7 +44,7 @@ async function prefetchLocalWorktreeCreateBase( repo: Repo, baseBranch: string | undefined, runtime: WorktreeCreateBasePrefetchRuntime -): Promise { +): Promise { const resolvedBaseBranch = await resolveWorktreeCreateBase({ requestedBaseBranch: baseBranch, repoWorktreeBaseRef: repo.worktreeBaseRef, @@ -64,13 +64,13 @@ async function prefetchLocalWorktreeCreateBase( } }) if (!resolvedBaseBranch) { - return + return undefined } if ( isFullGitObjectId(resolvedBaseBranch) && (await hasLocalWorktreeBaseRef(repo.path, resolvedBaseBranch)) ) { - return + return resolvedBaseBranch } const remoteTrackingBase = await runtime.resolveRemoteTrackingBase(repo.path, resolvedBaseBranch) if (remoteTrackingBase) { @@ -79,34 +79,35 @@ async function prefetchLocalWorktreeCreateBase( !(await hasLocalWorktreeBaseRef(repo.path, resolvedBaseBranch)) ) { await runtime.getOrStartRemoteTrackingBaseRefresh(repo.path, remoteTrackingBase) - return + return resolvedBaseBranch } } if (await hasLocalWorktreeBaseRef(repo.path, resolvedBaseBranch)) { // Why: hosted-review start points and local branch bases are already local; a broad remote fetch cannot make them fresher. - return + return resolvedBaseBranch } // Why: keep optimistic prefetch on the same best-effort fallback path as // create so the real create can reuse the runtime's remote fetch cache. await runtime.fetchRemoteWithCache(repo.path, 'origin') + return resolvedBaseBranch } export async function prefetchWorktreeCreateBase(args: { repo: Repo baseBranch?: string runtime: WorktreeCreateBasePrefetchRuntime -}): Promise { +}): Promise { if (isFolderRepo(args.repo)) { - return + return undefined } if (args.repo.connectionId) { const provider = getSshGitProvider(args.repo.connectionId) if (!provider) { - return + return undefined } await prefetchRemoteWorktreeCreateBase(provider, args.repo, { baseBranch: args.baseBranch }) - return + return undefined } - await prefetchLocalWorktreeCreateBase(args.repo, args.baseBranch, args.runtime) + return prefetchLocalWorktreeCreateBase(args.repo, args.baseBranch, args.runtime) } diff --git a/src/main/worktree-create-preparation.test.ts b/src/main/worktree-create-preparation.test.ts new file mode 100644 index 00000000000..0f4aa01e78e --- /dev/null +++ b/src/main/worktree-create-preparation.test.ts @@ -0,0 +1,214 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { Store } from './persistence' +import type { Repo } from '../shared/repo-types' +import { WORKTREE_CREATE_PREPARATION_DIRECTORY } from '../shared/worktree/create-preparation' + +const mocks = vi.hoisted(() => ({ + mkdir: vi.fn(), + listWorktreeGraph: vi.fn(), + prepareCheckout: vi.fn(), + finalize: vi.fn(), + discard: vi.fn(), + unlock: vi.fn(), + getWorktreeOptions: vi.fn() +})) + +vi.mock('node:fs/promises', () => ({ mkdir: mocks.mkdir })) +vi.mock('./git/worktree', () => ({ listWorktreeGraph: mocks.listWorktreeGraph })) +vi.mock('./git/worktree-create-preparation', () => ({ + prepareWorktreeCreateCheckout: mocks.prepareCheckout, + finalizePreparedWorktree: mocks.finalize, + discardPreparedWorktree: mocks.discard, + unlockPreparedWorktree: mocks.unlock +})) +vi.mock('./project-runtime-git-options', () => ({ + getLocalProjectWorktreeGitOptions: mocks.getWorktreeOptions +})) +vi.mock('./ipc/worktree-logic', () => ({ + computeWorkspaceRoot: (repoPath: string) => + process.platform === 'win32' && /^[A-Za-z]:[\\/]/.test(repoPath) + ? 'C:\\workspace' + : '/workspace', + getWorktreePathSettings: () => ({ + workspaceDir: process.platform === 'win32' ? 'C:\\workspace' : '/workspace', + nestWorkspaces: false + }) +})) + +import { + _resetWorktreeCreatePreparationsForTests, + consumePreparedWorktreeCreate, + prepareWorktreeCreateForRepo +} from './worktree-create-preparation' + +const repo = { id: 'repo-1', path: '/repo' } as Repo +const store = { getSettings: () => ({}) } as unknown as Store + +beforeEach(() => { + mocks.mkdir.mockReset().mockResolvedValue(undefined) + mocks.listWorktreeGraph.mockReset().mockResolvedValue([]) + mocks.prepareCheckout.mockReset().mockResolvedValue(undefined) + mocks.finalize.mockReset().mockResolvedValue({}) + mocks.discard.mockReset().mockResolvedValue(undefined) + mocks.unlock.mockReset().mockResolvedValue(undefined) + mocks.getWorktreeOptions.mockReset().mockReturnValue({}) +}) + +afterEach(async () => { + await _resetWorktreeCreatePreparationsForTests() +}) + +describe('worktree create preparation registry', () => { + it('namespaces native Windows preparation directories for long paths', async () => { + const originalPlatform = process.platform + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + try { + await prepareWorktreeCreateForRepo(store, { ...repo, path: 'C:\\repo' }, 'origin/main') + + expect(mocks.mkdir).toHaveBeenCalledWith( + expect.stringMatching(/^\\\\\?\\C:\\workspace\\\.orca-preparing/), + { recursive: true } + ) + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + } + }) + + it('deduplicates preparation for the same repo, base, runtime, and workspace root', async () => { + await Promise.all([ + prepareWorktreeCreateForRepo(store, repo, 'origin/main'), + prepareWorktreeCreateForRepo(store, repo, 'origin/main') + ]) + + expect(mocks.prepareCheckout).toHaveBeenCalledTimes(1) + }) + + it('does not claim a preparation after the selected base changes', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + + await expect( + consumePreparedWorktreeCreate({ + repoPath: repo.path, + workspaceRoot: '/workspace', + worktreePath: '/workspace/final', + branch: 'feature/test', + baseBranch: 'origin/release' + }) + ).resolves.toBeNull() + expect(mocks.finalize).not.toHaveBeenCalled() + }) + + it('routes preparation and finalization through the selected WSL runtime', async () => { + const options = { wslDistro: 'Ubuntu' } + mocks.getWorktreeOptions.mockReturnValue(options) + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + + await consumePreparedWorktreeCreate({ + repoPath: repo.path, + workspaceRoot: '/workspace', + worktreePath: '/workspace/final', + branch: 'feature/test', + baseBranch: 'origin/main', + options + }) + + expect(mocks.prepareCheckout).toHaveBeenCalledWith( + repo.path, + expect.any(String), + 'origin/main', + expect.any(String), + options + ) + expect(mocks.finalize).toHaveBeenCalledWith( + repo.path, + expect.any(String), + '/workspace/final', + 'feature/test', + 'origin/main', + undefined, + options + ) + }) + + it('retries stale cleanup after a transient listing failure', async () => { + mocks.listWorktreeGraph.mockRejectedValueOnce(new Error('temporary listing failure')) + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + await prepareWorktreeCreateForRepo(store, repo, 'origin/release') + + expect(mocks.listWorktreeGraph).toHaveBeenCalledTimes(2) + }) + + it('unlocks a stale branch-attached final path instead of deleting user work', async () => { + mocks.listWorktreeGraph.mockResolvedValueOnce([ + { + path: '/workspace/final', + branch: 'refs/heads/feature/test', + lockReason: 'orca-create-preparation:v1:999999999:stale', + head: 'deadbeef', + isBare: false, + isMainWorktree: false + } + ]) + + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + + expect(mocks.unlock).toHaveBeenCalledWith(repo.path, '/workspace/final', {}) + expect(mocks.discard).not.toHaveBeenCalledWith(repo.path, '/workspace/final', {}) + }) + + it('does not classify a user branch worktree under the preparation directory as stale', async () => { + mocks.listWorktreeGraph.mockResolvedValueOnce([ + { + path: '/workspace/.orca-preparing/999999999-user-worktree', + branch: 'refs/heads/user-worktree', + lockReason: undefined, + head: 'deadbeef', + isBare: false, + isMainWorktree: false + } + ]) + + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + + expect(mocks.unlock).not.toHaveBeenCalled() + expect(mocks.discard).not.toHaveBeenCalled() + }) + + it('does not discard a detached worktree with caller-controlled preparation metadata', async () => { + mocks.listWorktreeGraph.mockResolvedValueOnce([ + { + path: `/workspace/${WORKTREE_CREATE_PREPARATION_DIRECTORY}/999-checkout`, + branch: undefined, + lockReason: 'orca-create-preparation:v1:999999999:spoofed', + head: 'deadbeef', + isBare: false, + isMainWorktree: false + } + ]) + + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + + expect(mocks.discard).not.toHaveBeenCalledWith( + repo.path, + `/workspace/${WORKTREE_CREATE_PREPARATION_DIRECTORY}/999-checkout`, + {} + ) + }) + + it('cleans up and returns null so normal add can run when finalization fails', async () => { + await prepareWorktreeCreateForRepo(store, repo, 'origin/main') + mocks.finalize.mockRejectedValueOnce(new Error('submodules prevent worktree move')) + + await expect( + consumePreparedWorktreeCreate({ + repoPath: repo.path, + workspaceRoot: '/workspace', + worktreePath: '/workspace/final', + branch: 'feature/test', + baseBranch: 'origin/main' + }) + ).resolves.toBeNull() + expect(mocks.mkdir).toHaveBeenCalledWith('/workspace', { recursive: true }) + expect(mocks.discard).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/main/worktree-create-preparation.ts b/src/main/worktree-create-preparation.ts new file mode 100644 index 00000000000..c26ca64fcdb --- /dev/null +++ b/src/main/worktree-create-preparation.ts @@ -0,0 +1,283 @@ +import { randomUUID } from 'node:crypto' +import { mkdir } from 'node:fs/promises' +import { posix, win32 } from 'node:path' +import type { Store } from './persistence' +import type { Repo } from '../shared/repo-types' +import { isFolderRepo } from '../shared/repo-kind' +import { isWindowsAbsolutePathLike } from '../shared/cross-platform-path' +import { + WORKTREE_CREATE_PREPARATION_DIRECTORY, + createWorktreePreparationLockReason, + isWorktreeCreatePreparation, + parseWorktreePreparationOwnerPid, + parseWorktreePreparationPathOwnerPid +} from '../shared/worktree/create-preparation' +import type { AddWorktreeOptions, AddWorktreeResult } from './git/worktree' +import { listWorktreeGraph } from './git/worktree' +import { + discardPreparedWorktree, + finalizePreparedWorktree, + unlockPreparedWorktree, + prepareWorktreeCreateCheckout +} from './git/worktree-create-preparation' +import { getLocalProjectWorktreeGitOptions } from './project-runtime-git-options' +import { computeWorkspaceRoot, getWorktreePathSettings } from './ipc/worktree-logic' +import { toHostFilesystemPath } from './host-tree-removal' + +export const WORKTREE_CREATE_PREPARATION_TTL_MS = 5 * 60_000 +export const WORKTREE_CREATE_PREPARATION_LIMIT = 3 +const STALE_PREPARATION_CLEANUP_CONCURRENCY = 4 + +type PreparationEntry = { + key: string + repoPath: string + workspaceRoot: string + preparedPath: string + options: AddWorktreeOptions + createdAt: number + ready: Promise + expiration: NodeJS.Timeout +} + +type ConsumePreparedWorktreeArgs = { + repoPath: string + workspaceRoot: string + worktreePath: string + branch: string + baseBranch: string + refreshLocalBaseRef?: boolean + options?: AddWorktreeOptions +} + +const preparations = new Map() +const staleCleanupInFlight = new Map>() + +function pathOps(path: string): Pick { + return isWindowsAbsolutePathLike(path) ? win32 : posix +} + +function pathKey(path: string): string { + const normalized = pathOps(path).normalize(path) + return isWindowsAbsolutePathLike(path) ? normalized.toLowerCase() : normalized +} + +function preparationKey( + repoPath: string, + workspaceRoot: string, + baseBranch: string, + options: AddWorktreeOptions +): string { + return `${pathKey(repoPath)}\0${pathKey(workspaceRoot)}\0${baseBranch}\0${options.wslDistro ?? ''}` +} + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0) + return true + } catch (error) { + return (error as NodeJS.ErrnoException).code !== 'ESRCH' + } +} + +async function discardEntry(entry: PreparationEntry): Promise { + await entry.ready.catch(() => {}) + await discardPreparedWorktree(entry.repoPath, entry.preparedPath, entry.options).catch(() => {}) +} + +function expireEntry(entry: PreparationEntry): void { + if (preparations.get(entry.key) !== entry) { + return + } + preparations.delete(entry.key) + void discardEntry(entry) +} + +function enforcePreparationLimit(): void { + while (preparations.size >= WORKTREE_CREATE_PREPARATION_LIMIT) { + const oldest = [...preparations.values()].sort( + (left, right) => left.createdAt - right.createdAt + )[0] + if (!oldest) { + return + } + preparations.delete(oldest.key) + clearTimeout(oldest.expiration) + void discardEntry(oldest) + } +} + +async function cleanupStalePreparations( + repoPath: string, + options: AddWorktreeOptions +): Promise { + const cleanupKey = `${pathKey(repoPath)}\0${options.wslDistro ?? ''}` + const existing = staleCleanupInFlight.get(cleanupKey) + if (existing) { + await existing.catch(() => {}) + return + } + const cleanup = (async () => { + const worktrees = await listWorktreeGraph(repoPath, { + ...options, + includeCreatePreparations: true + }) + const staleWorktrees = worktrees.filter(isWorktreeCreatePreparation) + let nextIndex = 0 + async function discardNextStalePreparation(): Promise { + while (nextIndex < staleWorktrees.length) { + const worktree = staleWorktrees[nextIndex] + nextIndex += 1 + const lockOwnerPid = parseWorktreePreparationOwnerPid(worktree.lockReason) + const pathOwnerPid = parseWorktreePreparationPathOwnerPid(worktree.path) + if (!lockOwnerPid || isProcessAlive(lockOwnerPid)) { + continue + } + // Preserve a branch-attached final path after a crash; only detached or + // still-hidden preparations are safe to discard automatically. + if (worktree.branch && pathOwnerPid === null) { + await unlockPreparedWorktree(repoPath, worktree.path, options).catch(() => {}) + } else if (pathOwnerPid === lockOwnerPid) { + await discardPreparedWorktree(repoPath, worktree.path, options).catch(() => {}) + } + } + } + const workerCount = Math.min(STALE_PREPARATION_CLEANUP_CONCURRENCY, staleWorktrees.length) + await Promise.all(Array.from({ length: workerCount }, () => discardNextStalePreparation())) + })() + staleCleanupInFlight.set(cleanupKey, cleanup) + try { + await cleanup.catch(() => {}) + } finally { + if (staleCleanupInFlight.get(cleanupKey) === cleanup) { + staleCleanupInFlight.delete(cleanupKey) + } + } +} + +export function prepareWorktreeCreateForRepo( + store: Store, + repo: Repo, + baseBranch: string +): Promise { + if (repo.connectionId || isFolderRepo(repo)) { + return Promise.resolve() + } + const options = getLocalProjectWorktreeGitOptions(store, repo) + const workspaceRoot = computeWorkspaceRoot( + repo.path, + getWorktreePathSettings(repo, store.getSettings()) + ) + const key = preparationKey(repo.path, workspaceRoot, baseBranch, options) + const existing = preparations.get(key) + if (existing) { + return existing.ready + } + + enforcePreparationLimit() + const preparationId = `${process.pid}-${randomUUID()}` + const lockReason = createWorktreePreparationLockReason(preparationId) + const preparedPath = pathOps(workspaceRoot).join( + workspaceRoot, + WORKTREE_CREATE_PREPARATION_DIRECTORY, + preparationId + ) + const entry = {} as PreparationEntry + const expiration = setTimeout(() => expireEntry(entry), WORKTREE_CREATE_PREPARATION_TTL_MS) + expiration.unref() + Object.assign(entry, { + key, + repoPath: repo.path, + workspaceRoot, + preparedPath, + options, + createdAt: Date.now(), + expiration, + ready: (async () => { + await cleanupStalePreparations(repo.path, options) + await mkdir( + toHostFilesystemPath( + pathOps(workspaceRoot).join(workspaceRoot, WORKTREE_CREATE_PREPARATION_DIRECTORY) + ), + { recursive: true } + ) + await prepareWorktreeCreateCheckout(repo.path, preparedPath, baseBranch, lockReason, options) + })() + } satisfies PreparationEntry) + preparations.set(key, entry) + void entry.ready.catch(() => { + if (preparations.get(key) === entry) { + preparations.delete(key) + clearTimeout(entry.expiration) + } + }) + return entry.ready +} + +async function claimPreparedWorktree( + repoPath: string, + workspaceRoot: string, + baseBranch: string, + options: AddWorktreeOptions +): Promise { + const key = preparationKey(repoPath, workspaceRoot, baseBranch, options) + const entry = preparations.get(key) + if (!entry) { + return null + } + preparations.delete(key) + clearTimeout(entry.expiration) + try { + await entry.ready + return entry + } catch { + return null + } +} + +export async function consumePreparedWorktreeCreate( + args: ConsumePreparedWorktreeArgs +): Promise { + const options = args.options ?? {} + const entry = await claimPreparedWorktree( + args.repoPath, + args.workspaceRoot, + args.baseBranch, + options + ) + if (!entry) { + return null + } + try { + await mkdir(toHostFilesystemPath(pathOps(args.worktreePath).dirname(args.worktreePath)), { + recursive: true + }) + return await finalizePreparedWorktree( + args.repoPath, + entry.preparedPath, + args.worktreePath, + args.branch, + args.baseBranch, + args.refreshLocalBaseRef, + options + ) + } catch (error) { + await discardPreparedWorktree(args.repoPath, entry.preparedPath, options).catch(() => {}) + console.warn( + '[worktree-create] prepared checkout could not be finalized; using normal add', + error + ) + return null + } +} + +export async function _resetWorktreeCreatePreparationsForTests(): Promise { + const entries = [...preparations.values()] + preparations.clear() + staleCleanupInFlight.clear() + await Promise.all( + entries.map(async (entry) => { + clearTimeout(entry.expiration) + await discardEntry(entry) + }) + ) +} diff --git a/src/shared/git-binary-compatibility.test.ts b/src/shared/git-binary-compatibility.test.ts index 712910cef6d..11fede2ae10 100644 --- a/src/shared/git-binary-compatibility.test.ts +++ b/src/shared/git-binary-compatibility.test.ts @@ -151,6 +151,36 @@ describeBinaryCompatibility('real Git binary compatibility', () => { await rm(join(repoPath, 'deferred-trash'), { recursive: true, force: true }) }) + it('supports prepared worktree creation and finalization', async () => { + await runGit(['worktree', 'add', '--detach', '--no-checkout', 'compat-prepared', 'HEAD']) + await runGit(['-C', 'compat-prepared', 'reset', '--hard', 'HEAD']) + await runGit([ + 'worktree', + 'lock', + '--reason', + 'orca-create-preparation:v1:compat', + 'compat-prepared' + ]) + // Why: `-f -f` moves a locked preparation while preserving its lock reason (Git >=2.25). + await runGit(['worktree', 'move', '-f', '-f', 'compat-prepared', 'compat-final']) + await runGit([ + '-C', + 'compat-final', + 'checkout', + '--no-track', + '-b', + 'compat-prepared-final', + 'HEAD' + ]) + + await expect(runGit(['-C', 'compat-final', 'branch', '--show-current'])).resolves.toMatchObject( + { stdout: 'compat-prepared-final\n' } + ) + await runGit(['worktree', 'unlock', 'compat-final']) + await runGit(['worktree', 'remove', '--force', 'compat-final']) + await runGit(['branch', '-D', 'compat-prepared-final']) + }) + it('recognizes ref and merge-tree compatibility boundaries', async () => { const fetchHeadPath = join(repoPath, '.git', 'FETCH_HEAD') await writeFile(fetchHeadPath, 'sentinel\n') diff --git a/src/shared/worktree/create-preparation.test.ts b/src/shared/worktree/create-preparation.test.ts new file mode 100644 index 00000000000..beaca89d186 --- /dev/null +++ b/src/shared/worktree/create-preparation.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from 'vitest' +import { + createWorktreePreparationLockReason, + isWorktreeCreatePreparation, + parseWorktreePreparationPathOwnerPid, + WORKTREE_CREATE_PREPARATION_DIRECTORY +} from './create-preparation' + +describe('worktree create preparation classification', () => { + it('recognizes an explicitly locked preparation regardless of branch state', () => { + expect( + isWorktreeCreatePreparation({ + path: `/workspace/${WORKTREE_CREATE_PREPARATION_DIRECTORY}/123-checkout`, + branch: 'refs/heads/feature', + lockReason: createWorktreePreparationLockReason('test') + }) + ).toBe(true) + }) + + it('does not classify a branch-attached user worktree by path alone', () => { + expect( + isWorktreeCreatePreparation({ + path: `/workspace/${WORKTREE_CREATE_PREPARATION_DIRECTORY}/123-user-worktree`, + branch: 'refs/heads/feature', + lockReason: undefined + }) + ).toBe(false) + }) + + it('does not classify an unlocked detached path without durable ownership', () => { + expect( + isWorktreeCreatePreparation({ + path: `/workspace/${WORKTREE_CREATE_PREPARATION_DIRECTORY}/123-checkout`, + branch: undefined, + lockReason: undefined + }) + ).toBe(false) + }) + + it('does not parse an arbitrary preparation path with a numeric prefix', () => { + expect( + parseWorktreePreparationPathOwnerPid( + `/workspace/${WORKTREE_CREATE_PREPARATION_DIRECTORY}/123-checkout` + ) + ).toBeNull() + }) + + it('does not classify an arbitrary detached user path by directory name alone', () => { + expect( + isWorktreeCreatePreparation({ + path: `/workspace/${WORKTREE_CREATE_PREPARATION_DIRECTORY}/user-worktree`, + branch: undefined, + lockReason: undefined + }) + ).toBe(false) + }) +}) diff --git a/src/shared/worktree/create-preparation.ts b/src/shared/worktree/create-preparation.ts new file mode 100644 index 00000000000..8d98e75d75d --- /dev/null +++ b/src/shared/worktree/create-preparation.ts @@ -0,0 +1,42 @@ +export const WORKTREE_CREATE_PREPARATION_DIRECTORY = '.orca-preparing' +export const WORKTREE_CREATE_PREPARATION_LOCK_PREFIX = 'orca-create-preparation:v1:' +const WORKTREE_CREATE_PREPARATION_ID_PATTERN = + /^(\d+)-[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i + +export function createWorktreePreparationLockReason(sessionId: string): string { + return `${WORKTREE_CREATE_PREPARATION_LOCK_PREFIX}${process.pid}:${sessionId}` +} + +export function parseWorktreePreparationOwnerPid(lockReason?: string): number | null { + if (!lockReason?.startsWith(WORKTREE_CREATE_PREPARATION_LOCK_PREFIX)) { + return null + } + const pid = Number(lockReason.slice(WORKTREE_CREATE_PREPARATION_LOCK_PREFIX.length).split(':')[0]) + return Number.isSafeInteger(pid) && pid > 0 ? pid : null +} + +export function parseWorktreePreparationPathOwnerPid(path: string): number | null { + const pathParts = path.split(/[\\/]+/) + const preparationIndex = pathParts.lastIndexOf(WORKTREE_CREATE_PREPARATION_DIRECTORY) + if (preparationIndex === -1) { + return null + } + const preparationId = pathParts[preparationIndex + 1] + if (!preparationId || !WORKTREE_CREATE_PREPARATION_ID_PATTERN.test(preparationId)) { + return null + } + const pid = Number(preparationId.split('-')[0]) + return Number.isSafeInteger(pid) && pid > 0 ? pid : null +} + +export function isWorktreeCreatePreparation(worktree: { + path: string + lockReason?: string + branch?: string +}): boolean { + // The Git lock reason is the durable ownership proof. A path can be chosen + // by a user (including for an uncommitted detached worktree), so path shape + // alone must never hide or force-remove it. A crash before locking may leave + // an unlocked detached entry for manual cleanup, but cannot delete user data. + return parseWorktreePreparationOwnerPid(worktree.lockReason) !== null +} diff --git a/tests/tools/benchmarks/worktree-create-speculation-bench.mjs b/tests/tools/benchmarks/worktree-create-speculation-bench.mjs new file mode 100644 index 00000000000..b6af96d761f --- /dev/null +++ b/tests/tools/benchmarks/worktree-create-speculation-bench.mjs @@ -0,0 +1,155 @@ +#!/usr/bin/env node +import { spawnSync } from 'node:child_process' +import { mkdtempSync, rmSync } from 'node:fs' +import os from 'node:os' +import path from 'node:path' +import { performance } from 'node:perf_hooks' + +function parseArgs(argv) { + const options = { repo: process.cwd(), base: 'HEAD', iterations: 5 } + const firstFlag = argv.findIndex((value, index) => index > 0 && value.startsWith('--')) + for (let index = firstFlag === -1 ? argv.length : firstFlag; index < argv.length; index += 1) { + const flag = argv[index] + const value = argv[index + 1] + if (flag === '--repo' && value) { + options.repo = path.resolve(value) + } else if (flag === '--base' && value) { + options.base = value + } else if (flag === '--iterations' && value && Number.isInteger(Number(value))) { + options.iterations = Number(value) + } else { + throw new Error(`Unknown or incomplete argument: ${flag}`) + } + index += 1 + } + if (options.iterations < 1) { + throw new Error('--iterations must be positive') + } + return options +} + +function git(repo, args) { + const result = spawnSync('git', ['-C', repo, ...args], { + encoding: 'utf8', + maxBuffer: 64 * 1024 * 1024, + windowsHide: true + }) + if (result.status !== 0) { + throw new Error(`git ${args.join(' ')} failed\n${result.stderr || result.stdout}`) + } + return result.stdout.trim() +} + +function time(operation) { + const startedAt = performance.now() + operation() + return performance.now() - startedAt +} + +function median(values) { + const sorted = [...values].sort((left, right) => left - right) + const middle = Math.floor(sorted.length / 2) + return sorted.length % 2 === 0 ? (sorted[middle - 1] + sorted[middle]) / 2 : sorted[middle] +} + +function summarize(samples) { + return { + medianMs: Number(median(samples).toFixed(1)), + minMs: Number(Math.min(...samples).toFixed(1)), + maxMs: Number(Math.max(...samples).toFixed(1)), + samplesMs: samples.map((sample) => Number(sample.toFixed(1))) + } +} + +function removeWorktree(repo, worktreePath, branch, locked = false) { + if (locked) { + git(repo, ['worktree', 'unlock', worktreePath]) + } + git(repo, ['worktree', 'remove', '--force', worktreePath]) + if (branch) { + git(repo, ['branch', '-D', branch]) + } +} + +function benchmark(options) { + const repo = git(options.repo, ['rev-parse', '--show-toplevel']) + const base = git(repo, ['rev-parse', '--verify', `${options.base}^{commit}`]) + const retargetBase = git(repo, ['rev-parse', '--verify', `${base}~20^{commit}`]) + const scratchRoot = mkdtempSync(path.join(path.dirname(repo), '.orca-create-bench-')) + const samples = { baseline: [], prepare: [], submit: [], retarget: [], cancel: [] } + const prefix = `orca-create-bench-${process.pid}-${Date.now()}` + + try { + for (let index = 0; index < options.iterations; index += 1) { + const baselinePath = path.join(scratchRoot, `baseline-${index}`) + const baselineBranch = `${prefix}-baseline-${index}` + samples.baseline.push( + time(() => + git(repo, ['worktree', 'add', '--no-track', '-b', baselineBranch, baselinePath, base]) + ) + ) + removeWorktree(repo, baselinePath, baselineBranch) + + const preparedPath = path.join(scratchRoot, `prepared-${index}`) + const finalPath = path.join(scratchRoot, `final-${index}`) + const finalBranch = `${prefix}-final-${index}` + samples.prepare.push( + time(() => { + git(repo, ['worktree', 'add', '--detach', '--no-checkout', preparedPath, base]) + git(preparedPath, ['reset', '--hard', base]) + git(repo, [ + 'worktree', + 'lock', + '--reason', + `orca-create-preparation:v1:${process.pid}:${index}`, + preparedPath + ]) + }) + ) + samples.submit.push( + time(() => { + const targetHead = git(repo, ['rev-parse', '--verify', `${base}^{commit}`]) + git(preparedPath, ['rev-parse', '--verify', 'HEAD']) + git(repo, ['worktree', 'move', '-f', '-f', preparedPath, finalPath]) + git(finalPath, ['checkout', '--no-track', '-b', finalBranch, targetHead]) + git(repo, ['worktree', 'unlock', finalPath]) + }) + ) + removeWorktree(repo, finalPath, finalBranch) + + const retargetPath = path.join(scratchRoot, `retarget-${index}`) + git(repo, ['worktree', 'add', '--detach', '--no-checkout', retargetPath, base]) + git(retargetPath, ['reset', '--hard', base]) + git(repo, ['worktree', 'lock', '--reason', 'orca-create-preparation:v1:bench', retargetPath]) + samples.retarget.push(time(() => git(retargetPath, ['reset', '--hard', retargetBase]))) + removeWorktree(repo, retargetPath, undefined, true) + + const cancelledPath = path.join(scratchRoot, `cancel-${index}`) + git(repo, ['worktree', 'add', '--detach', '--no-checkout', cancelledPath, base]) + git(cancelledPath, ['reset', '--hard', base]) + git(repo, ['worktree', 'lock', '--reason', 'orca-create-preparation:v1:bench', cancelledPath]) + samples.cancel.push(time(() => removeWorktree(repo, cancelledPath, undefined, true))) + } + } finally { + rmSync(scratchRoot, { force: true, recursive: true }) + git(repo, ['worktree', 'prune']) + } + + return { + platform: `${process.platform}-${process.arch}`, + os: os.release(), + gitVersion: git(repo, ['--version']), + repo, + trackedFiles: Number(git(repo, ['ls-files']).split('\n').filter(Boolean).length), + iterations: options.iterations, + base, + retargetBase, + baselineSubmit: summarize(samples.baseline), + speculativePrepare: summarize(samples.prepare), + speculativeSubmit: summarize(samples.submit), + changeBaseAfterPrepare: summarize(samples.retarget), + cancelPrepared: summarize(samples.cancel) + } +} + +console.log(JSON.stringify(benchmark(parseArgs(process.argv)), null, 2)) From c539b388569f464c1efe4ef08073bd7e37342b2f Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 30 Aug 2026 12:20:51 -0700 Subject: [PATCH 10/59] Fix select all in native chat composer (#17294) Co-authored-by: Merge Sim --- .../native-chat/NativeChatComposer.tsx | 5 +- ...-chat-composer-app-menu-selection.test.tsx | 147 ++++++++++++++++++ ...native-chat-composer-app-menu-selection.ts | 38 +++++ src/renderer/src/lib/editable-target.test.ts | 45 ++++++ 4 files changed, 233 insertions(+), 2 deletions(-) create mode 100644 src/renderer/src/components/native-chat/use-native-chat-composer-app-menu-selection.test.tsx create mode 100644 src/renderer/src/components/native-chat/use-native-chat-composer-app-menu-selection.ts create mode 100644 src/renderer/src/lib/editable-target.test.ts diff --git a/src/renderer/src/components/native-chat/NativeChatComposer.tsx b/src/renderer/src/components/native-chat/NativeChatComposer.tsx index b11da6676f9..9081f6adf65 100644 --- a/src/renderer/src/components/native-chat/NativeChatComposer.tsx +++ b/src/renderer/src/components/native-chat/NativeChatComposer.tsx @@ -1,4 +1,4 @@ -import { forwardRef, useCallback, useImperativeHandle, useMemo, useRef, useState } from 'react' +import { forwardRef, useCallback, useImperativeHandle, useMemo, useState } from 'react' import { useAppStore } from '../../store' import { sendRuntimePtyInput } from '@/runtime/runtime-terminal-inspection' import { getSettingsForAgentTabRuntimeOwner } from '@/lib/agent-paste-draft' @@ -37,6 +37,7 @@ import type { import { dispatchNativeChatStructuredComposerText } from './native-chat-structured-composer-dispatch' import { useNativeChatPtyComposerSend } from './use-native-chat-pty-composer-send' import { useImeEnterGestureOwnership } from '@/lib/ime-composition-keyboard-event' +import { useNativeChatComposerAppMenuSelection } from './use-native-chat-composer-app-menu-selection' export type { NativeChatComposerHandle, @@ -97,8 +98,8 @@ const NativeChatComposerPane = forwardRef(null) const [dictationPressed, setDictationPressed] = useState(false) - const textareaRef = useRef(null) const imeEnterGesture = useImeEnterGestureOwnership() + const { textareaRef } = useNativeChatComposerAppMenuSelection(imeEnterGesture.isComposing) const { cancelPendingSends, trackPendingSend } = useNativeChatSendLifecycle( terminalTabId, targetPtyId, diff --git a/src/renderer/src/components/native-chat/use-native-chat-composer-app-menu-selection.test.tsx b/src/renderer/src/components/native-chat/use-native-chat-composer-app-menu-selection.test.tsx new file mode 100644 index 00000000000..9fca062e058 --- /dev/null +++ b/src/renderer/src/components/native-chat/use-native-chat-composer-app-menu-selection.test.tsx @@ -0,0 +1,147 @@ +// @vitest-environment happy-dom + +import { act, cleanup, fireEvent, render } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + APP_MENU_SELECTION_ACTION_EVENT, + type AppMenuSelectionAction +} from '@/lib/app-menu-selection-actions' +import { useAppMenuSelectionActions } from '@/hooks/useAppMenuSelectionActions' +import { useNativeChatComposerAppMenuSelection } from './use-native-chat-composer-app-menu-selection' + +let appMenuListener: ((action: AppMenuSelectionAction) => void) | null = null +const performNativeSelectionAction = vi.fn() + +function AppMenuBoundary(): null { + useAppMenuSelectionActions() + return null +} + +function ComposerHarness(): React.JSX.Element { + const { textareaRef, isComposingRef } = useNativeChatComposerAppMenuSelection() + return ( +
+