From ab32c2c0c58d30822f03d18b00db79e64eeb17a6 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 20:57:57 -0700 Subject: [PATCH 01/49] perf(startup): stop the persistence milestone from timing its own details closure (#18439) `logPersistenceStartupMilestone` resolved the lazy `details` closure before reading `performance.now()`, so the 1.6 MB `JSON.stringify` that `persistence-load-done` uses to report `workspaceSessionBytes` was billed to the milestone it measures. Snapshot `t` first. Diagnostics output is unchanged; only the recorded timestamp moves. --- ...rsistence-loading-store-extraction.test.ts | 41 +++++++++++++++++++ .../loading-store/loaded-state-parsing.ts | 4 +- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/main/persistence-loading-store-extraction.test.ts b/src/main/persistence-loading-store-extraction.test.ts index e48971d6c5c..a3c5e248909 100644 --- a/src/main/persistence-loading-store-extraction.test.ts +++ b/src/main/persistence-loading-store-extraction.test.ts @@ -116,6 +116,47 @@ describe('loading Store extraction seams', () => { }) }) + it('timestamps persistence-load-done before resolving its details closure', () => { + const sentinel = 'startup-diagnostics-workspace-session-sentinel-ordering' + vi.stubEnv('ORCA_STARTUP_DIAGNOSTICS', '1') + const state = getDefaultPersistedState(testState.dir) + state.workspaceSession = { ...state.workspaceSession, activeTabId: sentinel } + writeDataFile(state) + + // Fake clock only the details closure advances, so a post-closure timestamp is unambiguous. + let clock = 0 + const nowSpy = vi.spyOn(performance, 'now').mockImplementation(() => clock) + const realStringify = JSON.stringify + const stringifySpy = vi.spyOn(JSON, 'stringify').mockImplementation((( + value: unknown, + ...rest: unknown[] + ) => { + if ( + value && + typeof value === 'object' && + (value as { activeTabId?: unknown }).activeTabId === sentinel + ) { + clock += 1000 + } + return (realStringify as (...args: unknown[]) => string)(value, ...rest) + }) as typeof JSON.stringify) + + try { + const store = createStore() + store.freezeWrites() + } finally { + stringifySpy.mockRestore() + nowSpy.mockRestore() + } + + const loadDoneCall = logStartupDiagnosticMock.mock.calls.find( + ([event]) => event === 'persistence-load-done' + ) + const details = loadDoneCall?.[1] as Record | undefined + expect(details?.workspaceSessionBytes).toEqual(expect.any(Number)) + expect(details?.t).toBe(0) + }) + it('accepts the first JSON-parseable backup even when an older backup has richer state', async () => { mkdirSync(testState.dir, { recursive: true }) writeFileSync(dataFile(), '{{corrupt-primary', 'utf-8') diff --git a/src/main/persistence/loading-store/loaded-state-parsing.ts b/src/main/persistence/loading-store/loaded-state-parsing.ts index 5bd6e572ab1..e4034fa2b17 100644 --- a/src/main/persistence/loading-store/loaded-state-parsing.ts +++ b/src/main/persistence/loading-store/loaded-state-parsing.ts @@ -51,8 +51,10 @@ function logPersistenceStartupMilestone( if (!isStartupDiagnosticsEnabled()) { return } + // Why: snapshot `t` before resolving lazy details — otherwise an expensive details closure is billed to the milestone it measures. + const t = Math.round(performance.now()) const resolvedDetails = typeof details === 'function' ? details() : details - logStartupDiagnostic(event, { t: Math.round(performance.now()), ...resolvedDetails }) + logStartupDiagnostic(event, { t, ...resolvedDetails }) } import type { StoreRuntimeState } from './store-runtime-state' From c11c6878c1ee712f893a02a276ca3ec5e52f7aeb Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 20:59:53 -0700 Subject: [PATCH 02/49] perf(persistence): stop dead SSH leases pinning metadata, retire unreachable tombstones (#18430) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two unbounded-growth fixes in the persisted profile, which is re-serialised in full on every save. `collectPersistedWorkspaceOwners` registered every SSH lease's worktreeId as a live persisted owner with no state filter, so a route-retired lease — the operator-close `terminated` tombstone, or an `expired` row already marked `supersededBy`/`relayIdRecycled` — pinned its worktree's metadata row permanently. The prune gate's own doc names that failure: "Rows pinned by a persisted session are never removable, so the repetition cannot even make progress." Reuses `sshRemotePtyLeaseAllowsReattach`, the predicate that already decides which leases still name a route. `sshRemotePtyLeases` had no pruning path at all: removal happens in three explicit places, none age- or state-based, so `terminated` rows accumulated forever (137 rows / 54 KB on the reported profile, ~38/day from one target). Marking a lease `terminated` scrubs its pane bindings in the same write, so once no persisted binding names the id the row routes nothing — reattach, pane recovery, the orphan sweep, `ssh:reset` and `ssh:terminateSessions` all behave identically on an absent row. Delete it then, gated on that reachability check because a lease freezes its tabId and the tab-qualified scrub cannot reach a pane that was detached into a new tab. `expired` rows are deliberately untouched, superseded ones included: `sweepOrphanedRelayPtys` reads those ids as its leave-alone list, so dropping one would authorize stopping a remote shell that supersession left running on purpose (docs/reference/ssh-execution-boundary.md). --- ...istence-ssh-lease-reattach-reclaim.test.ts | 6 +- ...ence-ssh-lease-tombstone-retention.test.ts | 147 ++++++++++++++++++ .../persistence-ssh-remote-pty-leases.test.ts | 24 +-- .../ssh-pty-lease-operations.ts | 17 +- .../ssh-pty-lease-tombstone-retention.ts | 89 +++++++++++ ...ng-local-worktree-metadata-pruning.test.ts | 58 +++++++ ...missing-local-worktree-metadata-pruning.ts | 8 + 7 files changed, 324 insertions(+), 25 deletions(-) create mode 100644 src/main/persistence-ssh-lease-tombstone-retention.test.ts create mode 100644 src/main/persistence/leasing-ssh-ptys/ssh-pty-lease-tombstone-retention.ts diff --git a/src/main/persistence-ssh-lease-reattach-reclaim.test.ts b/src/main/persistence-ssh-lease-reattach-reclaim.test.ts index 9d6c74f710e..b2088fa616b 100644 --- a/src/main/persistence-ssh-lease-reattach-reclaim.test.ts +++ b/src/main/persistence-ssh-lease-reattach-reclaim.test.ts @@ -49,14 +49,16 @@ describe('ssh remote pty lease reclaim after a proven reattach', () => { expect(sshRemotePtyLeaseAllowsReattach(lease)).toBe(true) }) - it('leaves a terminated lease absorbing even when the id appears in a reattach batch', async () => { + it('never lets a reattach batch revive an operator-closed id', async () => { const store = await createStore() store.upsertSshRemotePtyLease({ targetId: 'ssh-1', ptyId: 'pty-1', state: 'attached' }) store.markSshRemotePtyLease('ssh-1', 'pty-1', 'terminated') await store.markSshRemotePtyLeasesAttachedAsync('ssh-1', ['pty-1']) - expect(store.getSshRemotePtyLeases('ssh-1')[0]).toMatchObject({ state: 'terminated' }) + // The unbound tombstone is retired at close, and the batch only ever updates existing rows — + // so the id stays out of the reattach set either way. + expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([]) }) it('does not revive an expired lease from an unqualified bulk attach', async () => { diff --git a/src/main/persistence-ssh-lease-tombstone-retention.test.ts b/src/main/persistence-ssh-lease-tombstone-retention.test.ts new file mode 100644 index 00000000000..feafdd766e4 --- /dev/null +++ b/src/main/persistence-ssh-lease-tombstone-retention.test.ts @@ -0,0 +1,147 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { createStore, testState } from './persistence-test-harness' +import { TEST_LEAF_1 } from './persistence-session-fixtures' + +vi.mock('electron', () => ({ + app: { getPath: () => testState.dir }, + safeStorage: { isEncryptionAvailable: () => false } +})) + +vi.mock('./telemetry/client', () => ({ track: vi.fn() })) +vi.mock('./telemetry/cohort-classifier', () => ({ getCohortAtEmit: () => ({}) })) + +describe('operator-closed SSH lease tombstones', () => { + beforeEach(() => { + testState.dir = mkdtempSync(join(tmpdir(), 'orca-test-')) + }) + + afterEach(() => { + rmSync(testState.dir, { recursive: true, force: true }) + }) + + /** A pane whose lease froze `tab-old` before `detachTerminalPaneToTab` moved it to `tab-new`. + * The binding scrub matches tab-qualified, so it cannot reach this row's binding. */ + async function storeWithDetachedPaneBinding(): Promise>> { + const store = await createStore() + store.upsertSshRemotePtyLease({ + targetId: 'ssh-1', + ptyId: 'remote-pty', + worktreeId: 'wt1', + tabId: 'tab-old', + leafId: TEST_LEAF_1, + state: 'attached' + }) + store.setWorkspaceSession({ + activeRepoId: 'r1', + activeWorktreeId: 'wt1', + activeTabId: 'tab-new', + tabsByWorktree: { + wt1: [ + { + id: 'tab-new', + worktreeId: 'wt1', + title: 'Terminal', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1, + ptyId: null + } + ] + }, + terminalLayoutsByTabId: { + 'tab-new': { + root: { type: 'leaf', leafId: TEST_LEAF_1 }, + activeLeafId: TEST_LEAF_1, + expandedLeafId: null, + ptyIdsByLeafId: { [TEST_LEAF_1]: 'ssh:ssh-1@@remote-pty' } + } + } + }) + return store + } + + it('keeps the tombstone while a binding the scrub could not reach still names the pty', async () => { + const store = await storeWithDetachedPaneBinding() + + store.markSshRemotePtyLease('ssh-1', 'ssh:ssh-1@@remote-pty', 'terminated') + + // `isRestorablePtyBinding` still consults this row to refuse replaying that binding. + expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([ + expect.objectContaining({ ptyId: 'remote-pty', state: 'terminated' }) + ]) + expect(store.getWorkspaceSession().terminalLayoutsByTabId['tab-new'].ptyIdsByLeafId).toEqual({ + [TEST_LEAF_1]: 'ssh:ssh-1@@remote-pty' + }) + }) + + it('keeps an operator-closed lease that still owes an undelivered stop', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ targetId: 'ssh-1', ptyId: 'remote-pty', state: 'attached' }) + store.recordSshRemotePtyKillIntent('ssh-1', 'remote-pty', { + incarnationId: 'inc-1', + requestedAt: 1, + attempts: 0 + }) + + store.markSshRemotePtyLease('ssh-1', 'ssh:ssh-1@@remote-pty', 'terminated') + + expect(store.getSshRemotePtyKillIntents('ssh-1', 2)).toHaveLength(1) + expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([ + expect.objectContaining({ ptyId: 'remote-pty', state: 'terminated' }) + ]) + }) + + // `expired` is never evidence the shell died, and `sweepOrphanedRelayPtys` reads these ids as its + // leave-alone list, so dropping one would authorize stopping a process left running on purpose. + it('keeps a superseded expired lease when a sibling pane is closed', async () => { + const store = await createStore() + store.upsertSshRemotePtyLease({ + targetId: 'ssh-1', + ptyId: 'remote-pty-1', + worktreeId: 'wt1', + tabId: 'tab1', + leafId: TEST_LEAF_1, + state: 'attached' + }) + store.upsertSshRemotePtyLease({ + targetId: 'ssh-1', + ptyId: 'remote-pty-2', + worktreeId: 'wt1', + tabId: 'tab1', + leafId: TEST_LEAF_1, + state: 'attached' + }) + store.upsertSshRemotePtyLease({ targetId: 'ssh-1', ptyId: 'remote-pty-3', state: 'attached' }) + + store.markSshRemotePtyLease('ssh-1', 'ssh:ssh-1@@remote-pty-3', 'terminated') + + expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([ + expect.objectContaining({ + ptyId: 'remote-pty-1', + state: 'expired', + supersededBy: 'remote-pty-2' + }), + expect.objectContaining({ ptyId: 'remote-pty-2', state: 'attached' }) + ]) + }) + + it('retires every unreachable tombstone for the target, not only the one just closed', async () => { + const store = await createStore() + for (const ptyId of ['remote-pty-1', 'remote-pty-2', 'remote-pty-3']) { + store.upsertSshRemotePtyLease({ targetId: 'ssh-1', ptyId, state: 'terminated' }) + } + store.upsertSshRemotePtyLease({ targetId: 'ssh-2', ptyId: 'other-pty', state: 'terminated' }) + expect(store.getSshRemotePtyLeases()).toHaveLength(4) + + store.markSshRemotePtyLease('ssh-1', 'ssh:ssh-1@@remote-pty-1', 'terminated') + + // Other targets are untouched: the pass is scoped to the one whose bindings were just scrubbed. + expect(store.getSshRemotePtyLeases()).toEqual([ + expect.objectContaining({ targetId: 'ssh-2', ptyId: 'other-pty' }) + ]) + }) +}) diff --git a/src/main/persistence-ssh-remote-pty-leases.test.ts b/src/main/persistence-ssh-remote-pty-leases.test.ts index 464138f69d4..d4cdc79285c 100644 --- a/src/main/persistence-ssh-remote-pty-leases.test.ts +++ b/src/main/persistence-ssh-remote-pty-leases.test.ts @@ -526,12 +526,9 @@ describe('Store', () => { store.markSshRemotePtyLeases('ssh-1', 'terminated') const session = store.getWorkspaceSession() - expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([ - expect.objectContaining({ - ptyId: 'remote-pty', - state: 'terminated' - }) - ]) + // The scrub is what retires the row: with no binding left naming the id, the tombstone routes + // nothing and is dropped in the same write. + expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([]) expect(session.tabsByWorktree.wt1[0].ptyId).toBeNull() expect(session.terminalLayoutsByTabId.tab1.ptyIdsByLeafId).toEqual({}) }) @@ -622,12 +619,8 @@ describe('Store', () => { store.markSshRemotePtyLease('ssh-1', 'ssh:ssh-1@@remote-pty', 'terminated') - expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([ - expect.objectContaining({ - ptyId: 'remote-pty', - state: 'terminated' - }) - ]) + // An unresolved id would have left the lease `attached`; this unbound row is retired instead. + expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([]) }) // `expired` never means the shell exited — every writer records that the CLIENT lost its route @@ -658,12 +651,7 @@ describe('Store', () => { store.markSshRemotePtyLease('ssh-1', 'ssh:ssh-1@@remote-pty', 'terminated') const session = store.getWorkspaceSession() - expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([ - expect.objectContaining({ - ptyId: 'remote-pty', - state: 'terminated' - }) - ]) + expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([]) expect(session.tabsByWorktree.wt1[0].ptyId).toBeNull() expect(session.terminalLayoutsByTabId.tab1.ptyIdsByLeafId).toEqual({}) }) diff --git a/src/main/persistence/leasing-ssh-ptys/ssh-pty-lease-operations.ts b/src/main/persistence/leasing-ssh-ptys/ssh-pty-lease-operations.ts index f0628cb25e5..f06e5b0ece7 100644 --- a/src/main/persistence/leasing-ssh-ptys/ssh-pty-lease-operations.ts +++ b/src/main/persistence/leasing-ssh-ptys/ssh-pty-lease-operations.ts @@ -2,6 +2,7 @@ import type { PersistedState } from '../../../shared/persisted-state-types' import type { SshRemotePtyLease } from '../../../shared/ssh-types' import { isTerminalLeafId } from '../../../shared/stable-pane-id' import { invalidateLocalWorktreeMetadataPruneInputs } from '../../local-worktree-metadata-prune-gate' +import { pruneRetiredSshRemotePtyLeaseTombstones } from './ssh-pty-lease-tombstone-retention' import { supersedeSiblingLeasesForPane } from './ssh-pty-pane-supersession' export type SshPtyLeaseOperations = { @@ -145,7 +146,11 @@ function updateSshRemotePtyLeaseStates( const bindingsChanged = shouldClearBindings ? operations.clearBindingsForLeases(targetId, leasesToClear) : false - return changed || bindingsChanged + // Why after the scrub: it is the scrub that makes the tombstones unreachable. + const tombstonesPruned = shouldClearBindings + ? pruneRetiredSshRemotePtyLeaseTombstones(operations, targetId) + : false + return changed || bindingsChanged || tombstonesPruned } export function markSshRemotePtyLeases( @@ -215,10 +220,11 @@ export function markSshRemotePtyLease( } const shouldClearBindings = leaseStateWithdrawsBinding(state) if (lease.state === state) { - if ( - (shouldClearBindings && operations.clearBindingsForLeases(targetId, [lease])) || - recycledChanged - ) { + const bindingsCleared = + shouldClearBindings && operations.clearBindingsForLeases(targetId, [lease]) + const tombstonesPruned = + shouldClearBindings && pruneRetiredSshRemotePtyLeaseTombstones(operations, targetId) + if (bindingsCleared || tombstonesPruned || recycledChanged) { operations.flush() } return @@ -233,6 +239,7 @@ export function markSshRemotePtyLease( } if (shouldClearBindings) { operations.clearBindingsForLeases(targetId, [lease]) + pruneRetiredSshRemotePtyLeaseTombstones(operations, targetId) } operations.flush() } diff --git a/src/main/persistence/leasing-ssh-ptys/ssh-pty-lease-tombstone-retention.ts b/src/main/persistence/leasing-ssh-ptys/ssh-pty-lease-tombstone-retention.ts new file mode 100644 index 00000000000..261162a3825 --- /dev/null +++ b/src/main/persistence/leasing-ssh-ptys/ssh-pty-lease-tombstone-retention.ts @@ -0,0 +1,89 @@ +import type { PersistedState } from '../../../shared/persisted-state-types' +import type { SshRemotePtyLease } from '../../../shared/ssh-types' + +export type SshPtyLeaseTombstoneRetentionOperations = { + state: PersistedState + toComparablePtyId: (targetId: string, ptyId: string) => string +} + +/** A routing tombstone with nothing left to route: the operator closed this PTY and no stop is + * still owed for it. `expired` is deliberately not here — it says only that the CLIENT lost its + * route (docs/reference/ssh-execution-boundary.md), and `sweepOrphanedRelayPtys` reads those ids + * as its leave-alone list, so deleting one would authorize stopping a remote shell that + * supersession left running on purpose. */ +function isRetiredRoutingTombstone(lease: SshRemotePtyLease, targetId: string): boolean { + return ( + lease.targetId === targetId && lease.state === 'terminated' && lease.pendingKill === undefined + ) +} + +/** Every stored-form relay pty id some persisted pane binding still names for this target. + * + * Reads all partitions, not only the two `clearSshRemotePtyBindingsForLeases` scrubs: this answer + * authorizes a delete, so a partition left unscanned would be a binding whose tombstone we dropped. + */ +function boundRelayPtyIds( + operations: SshPtyLeaseTombstoneRetentionOperations, + targetId: string +): Set { + const bound = new Set() + const sessions = [ + operations.state.workspaceSession, + ...Object.values(operations.state.workspaceSessionsByHostId ?? {}) + ] + for (const session of sessions) { + if (!session) { + continue + } + for (const tabs of Object.values(session.tabsByWorktree ?? {})) { + for (const tab of tabs) { + if (tab.ptyId) { + bound.add(operations.toComparablePtyId(targetId, tab.ptyId)) + } + } + } + for (const layout of Object.values(session.terminalLayoutsByTabId ?? {})) { + for (const ptyId of Object.values(layout?.ptyIdsByLeafId ?? {})) { + bound.add(operations.toComparablePtyId(targetId, ptyId)) + } + } + } + return bound +} + +/** + * Deletes the `terminated` rows nothing can reach, bounding an array that otherwise only grew. + * + * `terminated` is written with a binding scrub in the same call, so once no persisted binding names + * the id the row answers no question any reader asks. Reattach refuses it + * (`sshRemotePtyLeaseAllowsReattach`), pane recovery matches on `expired` only, the orphan sweep + * already classes it neither routed nor expired, and `ssh:reset` / `ssh:terminateSessions` skip it + * outright — every one of those behaves identically on an absent row. The one reader that can still + * observe it is `isRestorablePtyBinding`, and only through a binding whose pty id matches, which is + * exactly what the reachability test rules out. A `pendingKill` is an undelivered stop, so those + * rows stay until the replay retires them. + * + * The reachability test is not redundant with the scrub: a lease freezes its `tabId`, so a pane + * broken out into a new tab leaves a binding the scrub's tab-qualified match no longer reaches. + * + * Does not re-arm the local-worktree-metadata prune gate: a `terminated` lease no longer counts as + * a persisted workspace owner, so dropping one cannot make any metadata row more removable. + */ +export function pruneRetiredSshRemotePtyLeaseTombstones( + operations: SshPtyLeaseTombstoneRetentionOperations, + targetId: string +): boolean { + const leases = operations.state.sshRemotePtyLeases ?? [] + if (!leases.some((lease) => isRetiredRoutingTombstone(lease, targetId))) { + return false + } + const bound = boundRelayPtyIds(operations, targetId) + const retained = leases.filter( + (lease) => !isRetiredRoutingTombstone(lease, targetId) || bound.has(lease.ptyId) + ) + if (retained.length === leases.length) { + return false + } + operations.state.sshRemotePtyLeases = retained + return true +} diff --git a/src/main/persistence/tracking-repos/missing-local-worktree-metadata-pruning.test.ts b/src/main/persistence/tracking-repos/missing-local-worktree-metadata-pruning.test.ts index cdad14034e2..0588a2f6dae 100644 --- a/src/main/persistence/tracking-repos/missing-local-worktree-metadata-pruning.test.ts +++ b/src/main/persistence/tracking-repos/missing-local-worktree-metadata-pruning.test.ts @@ -287,6 +287,64 @@ describe('pruneSessionlessMissingLocalWorktreeMetadataForRepo', () => { } }) + // A route-retired lease is a tombstone, not a claim: counting one pinned its worktree's metadata + // row for good, so the prune could never make progress on it (#17775). + it('does not let route-retired SSH leases pin a metadata row', () => { + const state = makeState() + const liveIds = Array.from({ length: 3 }, (_, i) => `${REPO_ID}::/workspace/live-${i}`) + const terminatedIds = Array.from({ length: 5 }, (_, i) => `${REPO_ID}::/workspace/closed-${i}`) + const supersededIds = Array.from({ length: 4 }, (_, i) => `${REPO_ID}::/workspace/lost-${i}`) + const recycledIds = [`${REPO_ID}::/workspace/recycled`] + const allIds = [...liveIds, ...terminatedIds, ...supersededIds, ...recycledIds] + for (const worktreeId of allIds) { + state.worktreeMeta[worktreeId] = makeMeta(worktreeId) + } + const lease = (worktreeId: string, index: number, extra: object) => ({ + targetId: 'builder', + ptyId: `pty-${index}`, + worktreeId, + createdAt: 1, + updatedAt: 1, + ...extra + }) + state.sshRemotePtyLeases = [ + ...liveIds.map((id, i) => lease(id, i, { state: 'detached' })), + ...terminatedIds.map((id, i) => lease(id, 100 + i, { state: 'terminated' })), + ...supersededIds.map((id, i) => + lease(id, 200 + i, { state: 'expired', supersededBy: 'pty-9' }) + ), + ...recycledIds.map((id, i) => lease(id, 300 + i, { state: 'expired', relayIdRecycled: true })) + ] as never + + const scan = capture(state) + + expect(pruneCaptured(state, scan, allIds).sort()).toEqual( + [...terminatedIds, ...supersededIds, ...recycledIds].sort() + ) + expect(Object.keys(state.worktreeMeta).sort()).toEqual([...liveIds].sort()) + }) + + // A plain `expired` lease says only that the CLIENT lost its route, so its pane is still + // recoverable and its metadata row is still owned (docs/reference/ssh-execution-boundary.md). + it('keeps a metadata row pinned by an unmarked expired lease', () => { + const state = makeState() + const worktreeId = `${REPO_ID}::/workspace/orphaned` + state.worktreeMeta[worktreeId] = makeMeta(worktreeId) + const scan = capture(state) + state.sshRemotePtyLeases = [ + { + targetId: 'builder', + ptyId: 'pty', + worktreeId, + state: 'expired', + createdAt: 1, + updatedAt: 1 + } + ] + + expect(pruneCaptured(state, scan, [worktreeId])).toEqual([]) + }) + it('preserves canonically equivalent session and top-level owners', () => { const candidateId = `${REPO_ID}::/workspace/Café`.normalize('NFC') const ownerId = candidateId.normalize('NFD') diff --git a/src/main/persistence/tracking-repos/missing-local-worktree-metadata-pruning.ts b/src/main/persistence/tracking-repos/missing-local-worktree-metadata-pruning.ts index 0e03a560a17..db24c25bcf4 100644 --- a/src/main/persistence/tracking-repos/missing-local-worktree-metadata-pruning.ts +++ b/src/main/persistence/tracking-repos/missing-local-worktree-metadata-pruning.ts @@ -2,6 +2,7 @@ import { isWindowsAbsolutePathLike } from '../../../shared/cross-platform-path' import { getRepoExecutionHostId, LOCAL_EXECUTION_HOST_ID } from '../../../shared/execution-host' import type { PersistedState } from '../../../shared/persisted-state-types' import { getRepoKind } from '../../../shared/repo-kind' +import { sshRemotePtyLeaseAllowsReattach } from '../../../shared/ssh-types' import { worktreeWorkspaceKey } from '../../../shared/workspace-scope' import { FOLDER_WORKSPACE_INSTANCE_SEPARATOR, splitWorktreeId } from '../../../shared/worktree/id' import { isWslUncPath } from '../../../shared/wsl-paths' @@ -40,6 +41,13 @@ function collectPersistedWorkspaceOwners( } } for (const lease of state.sshRemotePtyLeases) { + // A lease that can never be reattached is a routing tombstone, not a claim on a workspace: + // `terminated` is the operator close, and an `expired` row marked `supersededBy` / + // `relayIdRecycled` already lost its pane to a newer lease. Counting them as owners pinned + // their worktree's metadata row permanently, so the prune could never make progress (#17775). + if (!sshRemotePtyLeaseAllowsReattach(lease)) { + continue + } add(lease.worktreeId) } for (const entry of state.migrationUnsupportedPtyEntries) { From 949c9d3353b99ce327e83d344b22f8684b9e8c1a Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 21:02:11 -0700 Subject: [PATCH 03/49] perf(worktrees): classify each worktree once, defer the SSH meta index, drop the conflict-path probe (#18433) * perf(worktrees): classify each worktree once, defer the SSH meta index, unserialise conflict probes Three redundancies on the worktree-catalog and git-status read paths: - buildDetectedGitWorktrees ran mergeWorktree + toDetectedWorktree twice for every visible row. Discovery backfill returns the same meta object when it wrote nothing, and both builders are pure over it, so skip the second pass on identity. - The SSH worktree-meta index parsed every worktree id on the host, then threw it away whenever the provider was connected. Build it lazily, memoised. - Unmerged `u` records were resolved one fs.access at a time. Resolve the prefix the cap can reach with 8-way concurrency, keyed by record index so Git's output order and error precedence are unchanged. * perf(git): read the porcelain worktree mode instead of probing conflicted paths Every porcelain-v2 `u` record already carries `mW`, the working-tree mode Git stat'ed for that row: `000000` means the conflicted path is absent. Reading it replaces the per-conflict `fs.access`, so the bounded-concurrency resolver, its `= 8` cap, and the order/error-precedence invariant are unnecessary rather than cheaper. `access()` stays only as a fallback for a malformed `mW`, so `parseUnmergedEntry` keeps its signature and neither status-read.ts nor the relay loop changes. Also corrects two fixtures that encoded `mW=100644` for a file that does not exist, which real Git never emits. * fix(test): import the conflict parser statically so the CJS cli project compiles --- src/main/git/status.test.ts | 31 ++- ...tected-provider-listing-meta-index.test.ts | 131 ++++++++++ .../listing/detected-provider-listing.ts | 15 +- .../detected-worktree-classification.test.ts | 234 ++++++++++++++++++ .../listing/ssh-worktree-fallback.ts | 15 +- .../listing/worktree-discovery-metadata.ts | 3 +- src/relay/git-porcelain-local-parity.test.ts | 3 +- .../git-status-conflict-entries.test.ts | 87 +++++++ src/shared/git-status-conflict-entries.ts | 25 +- 9 files changed, 514 insertions(+), 30 deletions(-) create mode 100644 src/main/ipc/worktrees/listing/detected-provider-listing-meta-index.test.ts create mode 100644 src/main/ipc/worktrees/listing/detected-worktree-classification.test.ts create mode 100644 src/shared/git-status-conflict-entries.test.ts diff --git a/src/main/git/status.test.ts b/src/main/git/status.test.ts index 4675e3a57a2..7b73739cd3f 100644 --- a/src/main/git/status.test.ts +++ b/src/main/git/status.test.ts @@ -77,6 +77,13 @@ describe('getStatus', () => { gitExecFileAsyncMock.mockResolvedValue({ stdout: '' }) }) + /** `access` targets outside the git dir — i.e. working-tree probes, not conflict-marker reads. */ + function conflictFileProbes(): string[] { + return accessMock.mock.calls + .map(([target]) => String(target).replaceAll('\\', '/')) + .filter((target) => !target.includes('/.git/')) + } + it('parses unmerged porcelain v2 entries into unresolved conflict rows', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') accessMock.mockImplementation(async (target: string) => { @@ -104,11 +111,12 @@ describe('getStatus', () => { ]) }) - it('maps deleted conflicts to deleted when the working tree file is absent', async () => { + // The 7th field of a `u` record is the working-tree mode; `000000` is how Git reports an absent path. + it('maps deleted conflicts to deleted from the porcelain working-tree mode', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: - 'u UD N... 100644 100644 000000 100644 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb cccccccccccccccccccccccccccccccccccccccc src/deleted.ts\n' + 'u UD N... 100644 100644 000000 000000 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb cccccccccccccccccccccccccccccccccccccccc src/deleted.ts\n' }) const result = await getStatus('/repo') @@ -120,10 +128,12 @@ describe('getStatus', () => { conflictKind: 'deleted_by_them', conflictStatus: 'unresolved' }) + expect(conflictFileProbes()).toEqual([]) }) - it('falls back to modified when the working-tree probe fails for a non-absence reason', async () => { + it('never re-probes the working tree for a conflict row, whatever the filesystem would say', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') + // Every probe fails ENOENT (beforeEach) or EIO — neither may reach the row's status. accessMock.mockRejectedValue(Object.assign(new Error('EIO'), { code: 'EIO' })) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: @@ -134,19 +144,14 @@ describe('getStatus', () => { expect(result.entries[0]?.status).toBe('modified') expect(result.entries[0]?.conflictKind).toBe('added_by_us') + expect(conflictFileProbes()).toEqual([]) }) // Why both cases normalize separators: git reports the worktree in the WSL guest namespace, and // the assertion is about which path is probed, not which separator this host's `path` emits. - it('probes the conflict working tree through the distro spelling on Windows', async () => { + it('resolves a WSL conflict row without crossing the 9p share', async () => { const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') readFileMock.mockResolvedValue('gitdir: /home/me/repo/.git/worktrees/feature\n') - accessMock.mockImplementation(async (target: string) => { - if (String(target).endsWith('new.ts')) { - return undefined - } - throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }) - }) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'u DU N... 100644 100644 100644 100644 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb cccccccccccccccccccccccccccccccccccccccc src/new.ts\n' @@ -156,10 +161,12 @@ describe('getStatus', () => { const result = await getStatus('/home/me/repo/feature', { wslDistro: 'Ubuntu' }) const probed = accessMock.mock.calls.map(([target]) => String(target).replaceAll('\\', '/')) - expect(probed).toContain('//wsl.localhost/Ubuntu/home/me/repo/feature/src/new.ts') + // No `\\wsl.localhost` round trip per conflict row: the porcelain `mW` field already answered. + expect(probed).not.toContain('//wsl.localhost/Ubuntu/home/me/repo/feature/src/new.ts') + expect(conflictFileProbes()).toEqual([]) expect(result.entries[0]?.status).toBe('modified') expect(result.entries[0]?.conflictKind).toBe('deleted_by_us') - // The conflict-marker probes travel the same way. + // The conflict-marker probes still travel through the distro spelling. expect( probed.filter((target) => target.startsWith('//wsl.localhost/Ubuntu/home/me/repo/.git/worktrees/feature/') diff --git a/src/main/ipc/worktrees/listing/detected-provider-listing-meta-index.test.ts b/src/main/ipc/worktrees/listing/detected-provider-listing-meta-index.test.ts new file mode 100644 index 00000000000..4bf55e97492 --- /dev/null +++ b/src/main/ipc/worktrees/listing/detected-provider-listing-meta-index.test.ts @@ -0,0 +1,131 @@ +/** + * The SSH worktree-meta index is only ever read via `metaIndex.get(repo.id)` on the disconnected + * fallbacks, so a connected listing must not pay `parseWorktreeId` over the whole host snapshot. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../../../../shared/repo-types' +import type { Store } from '../../../persistence/loading-store/store' +import type * as SshWorktreeFallbackModule from './ssh-worktree-fallback' + +const { getSshGitProviderMock, indexBuildSpy } = vi.hoisted(() => ({ + getSshGitProviderMock: vi.fn(), + indexBuildSpy: vi.fn() +})) + +vi.mock('../../../providers/ssh-git-dispatch', () => ({ + getSshGitProvider: getSshGitProviderMock, + requireSshGitProvider: getSshGitProviderMock, + getSshGitProviderGeneration: () => 1 +})) + +vi.mock('./ssh-worktree-fallback', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + // Both builders are counted: the point is that NO index is built on the connected path. + createSshWorktreeMetaIndex: (...args: Parameters) => { + indexBuildSpy('all-hosts', ...args) + return actual.createSshWorktreeMetaIndex(...args) + }, + createSshWorktreeMetaIndexForRepo: ( + ...args: Parameters + ) => { + indexBuildSpy('repo-scoped', ...args) + return actual.createSshWorktreeMetaIndexForRepo(...args) + } + } +}) + +const { listDetectedWorktreesForCapturedRepo } = await import('./detected-provider-listing') + +const repo = { + id: 'repo-1', + path: '/home/user/repo', + displayName: 'repo', + connectionId: 'conn-1' +} as Repo + +const worktreeId = `${repo.id}::/home/user/feature` + +function createStore(): Store { + const rows: Record = { + [worktreeId]: { instanceId: 'instance-1' }, + // Other repos' rows share the host snapshot; only this repo's bucket is ever read back. + 'repo-2::/home/user/other': { instanceId: 'instance-2' } + } + return { + getRepos: () => [repo], + getRepo: () => repo, + getSettings: () => ({}), + getProjectHostSetups: () => [], + getAllWorktreeLineage: () => ({}), + getAllWorktreeMeta: () => rows, + getWorktreeMeta: (id: string) => rows[id], + setWorktreeMeta: vi.fn() + } as unknown as Store +} + +describe('SSH worktree meta index construction', () => { + beforeEach(() => { + indexBuildSpy.mockClear() + getSshGitProviderMock.mockReset() + }) + + it('does not build the index when the provider answers', async () => { + const provider = { + listWorktrees: vi.fn().mockResolvedValue([ + { path: repo.path, head: 'a', branch: 'main', isBare: false, isMainWorktree: true }, + { + path: '/home/user/feature', + head: 'b', + branch: 'feature', + isBare: false, + isMainWorktree: false + } + ]) + } + + const result = await listDetectedWorktreesForCapturedRepo( + createStore(), + repo, + () => true, + provider as never + ) + + expect(result).toMatchObject({ authoritative: true, source: 'git' }) + expect(indexBuildSpy).not.toHaveBeenCalled() + }) + + it('builds the index once when no provider is available', async () => { + const result = await listDetectedWorktreesForCapturedRepo( + createStore(), + repo, + () => true, + undefined + ) + + expect(result).toMatchObject({ authoritative: false, source: 'metadata-fallback' }) + expect(indexBuildSpy).toHaveBeenCalledTimes(1) + expect(indexBuildSpy).toHaveBeenCalledWith('all-hosts', expect.anything()) + expect( + (result as { worktrees: { id: string }[] }).worktrees.map((worktree) => worktree.id) + ).toEqual([worktreeId]) + }) + + it('builds the index once when the provider listing fails', async () => { + const provider = { listWorktrees: vi.fn().mockRejectedValue(new Error('relay down')) } + + const result = await listDetectedWorktreesForCapturedRepo( + createStore(), + repo, + () => true, + provider as never + ) + + expect(result).toMatchObject({ authoritative: false, source: 'metadata-fallback' }) + expect(indexBuildSpy).toHaveBeenCalledTimes(1) + expect( + (result as { worktrees: { id: string }[] }).worktrees.map((worktree) => worktree.id) + ).toEqual([worktreeId]) + }) +}) diff --git a/src/main/ipc/worktrees/listing/detected-provider-listing.ts b/src/main/ipc/worktrees/listing/detected-provider-listing.ts index 51a0618ba66..25c08d262fb 100644 --- a/src/main/ipc/worktrees/listing/detected-provider-listing.ts +++ b/src/main/ipc/worktrees/listing/detected-provider-listing.ts @@ -10,7 +10,8 @@ import type { ListDesktopLineageForHostArgs } from '../../../../shared/host-line import { buildDetectedGitWorktrees, createSshWorktreeMetaIndex, - listDisconnectedSshWorktrees + listDisconnectedSshWorktrees, + type SshWorktreeMetaIndex } from './ssh-worktree-fallback' import { buildDisconnectedDetectedWorktrees, @@ -42,9 +43,11 @@ export async function listDetectedWorktreesForCapturedRepo( const allMeta = isFolderRepo(repo) ? undefined : readAllWorktreeMetaForHost(store, getRepoExecutionHostId(repo)) - const sshWorktreeMetaIndex = repo.connectionId - ? createSshWorktreeMetaIndex(Object.entries(allMeta ?? {})) - : new Map() + // Why: only the disconnected fallbacks read this, so keep parseWorktreeId over the whole host snapshot + // off the connected path entirely. + let cachedSshWorktreeMetaIndex: SshWorktreeMetaIndex | undefined + const sshWorktreeMetaIndex = (): SshWorktreeMetaIndex => + (cachedSshWorktreeMetaIndex ??= createSshWorktreeMetaIndex(Object.entries(allMeta ?? {}))) try { let gitWorktrees: GitWorktreeInfo[] @@ -86,7 +89,7 @@ export async function listDetectedWorktreesForCapturedRepo( if (!isCurrent()) { return null } - const worktrees = listDisconnectedSshWorktrees(store, repo, sshWorktreeMetaIndex) + const worktrees = listDisconnectedSshWorktrees(store, repo, sshWorktreeMetaIndex()) return { repoId: repo.id, authoritative: false, @@ -158,7 +161,7 @@ export async function listDetectedWorktreesForCapturedRepo( err ) if (repo.connectionId) { - const worktrees = listDisconnectedSshWorktrees(store, repo, sshWorktreeMetaIndex) + const worktrees = listDisconnectedSshWorktrees(store, repo, sshWorktreeMetaIndex()) return { repoId: repo.id, authoritative: false, diff --git a/src/main/ipc/worktrees/listing/detected-worktree-classification.test.ts b/src/main/ipc/worktrees/listing/detected-worktree-classification.test.ts new file mode 100644 index 00000000000..6a25d696c9b --- /dev/null +++ b/src/main/ipc/worktrees/listing/detected-worktree-classification.test.ts @@ -0,0 +1,234 @@ +/** + * Guards the single-classification contract of `buildDetectedGitWorktrees`: every visible worktree + * used to be run through `mergeWorktree` + `toDetectedWorktree` twice per catalog pass. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../../../../shared/repo-types' +import type { Store } from '../../../persistence/loading-store/store' +import type { WorktreeMeta } from '../../../../shared/worktree/meta-types' +import type { GitWorktreeInfo } from '../../../../shared/worktree/types' +import type * as NodeCryptoModule from 'node:crypto' +import type * as OwnershipModule from '../../../../shared/worktree/ownership' + +const { toDetectedWorktreeSpy } = vi.hoisted(() => ({ toDetectedWorktreeSpy: vi.fn() })) + +vi.mock('../../../../shared/worktree/ownership', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + toDetectedWorktree: (args: Parameters[0]) => { + toDetectedWorktreeSpy(args) + return actual.toDetectedWorktree(args) + } + } +}) + +vi.mock('node:crypto', async (importOriginal) => ({ + ...(await importOriginal()), + randomUUID: () => 'fixed-instance-id' +})) + +const { buildDetectedGitWorktrees } = await import('./ssh-worktree-fallback') +const { getProjectHostSetupWorktreeMeta } = + await import('../../../../shared/project-host-setup-lookup') +const { mergeWorktree } = await import('../../worktree-logic') +const { resolveWorktreeMetaWithDiscoveryBackfill } = await import('./worktree-discovery-metadata') +const ownership = await import('../../../../shared/worktree/ownership') +const { projectResolvedWorktreeLineage } = + await import('../../../../shared/resolved-worktree-lineage') +const { createWorktreeVisibilitySourceMatcher, resolveCustomWorktreeVisibilitySources } = + await import('../../../../shared/worktree/visibility-sources') +const { resolveConfiguredWorktreeBasePaths } = + await import('../../../../shared/worktree/configured-worktree-base-path') +const { dedupeWorktreesByPath } = await import('../../worktree-path-comparison') +const { readWorktreeMetaForHost } = + await import('../../../persistence/host-qualified-worktree-meta') +const { getRepoOwnedWorktreeMeta } = await import('../../../worktree-metadata-ownership') +const { getRepoExecutionHostId } = await import('../../../../shared/execution-host') + +const repo: Repo = { + id: 'repo-1', + path: '/workspace/repo', + displayName: 'repo', + badgeColor: '#000', + addedAt: 0 +} as Repo + +const ownershipMeta = getProjectHostSetupWorktreeMeta([], repo) + +function gitWorktree(path: string): GitWorktreeInfo { + return { + path, + head: 'abc123', + branch: 'refs/heads/feature', + isBare: false, + isMainWorktree: false + } +} + +/** Fully settled metadata: discovery backfill has nothing to write, so it hands the same object back. */ +function settledMeta(overrides: Partial = {}): WorktreeMeta { + return { + ...ownershipMeta, + instanceId: 'instance-settled', + orcaCreatedAt: 1, + lastActivityAt: 5, + ...overrides + } as WorktreeMeta +} + +function createStore(meta: Record, repos: Repo[] = [repo]) { + const rows = { ...meta } + return { + getRepos: () => repos, + getSettings: () => ({ workspaceDir: '/workspace', nestWorkspaces: true }), + getProjectHostSetups: () => [], + getAllWorktreeLineage: () => ({}), + getAllWorktreeMeta: () => rows, + getWorktreeMeta: (id: string) => rows[id], + getWorktreeMetaForHost: (id: string, hostId: string) => + rows[id]?.hostId === hostId ? rows[id] : undefined, + getAllWorktreeMetaForHost: () => rows, + setWorktreeMeta: (id: string, patch: Partial) => { + rows[id] = { ...rows[id], ...patch } as WorktreeMeta + return rows[id] + }, + setWorktreeMetaForHost: (id: string, hostId: string, patch: Partial) => { + rows[id] = { ...rows[id], ...patch, hostId } as WorktreeMeta + return rows[id] + } + } as unknown as Store +} + +/** The pre-change implementation, verbatim, as the equivalence oracle. */ +function buildDetectedGitWorktreesTwoPass( + store: Store, + target: Repo, + gitWorktrees: GitWorktreeInfo[], + allMetaOverride?: Record +) { + const settings = store.getSettings() + const knownOrcaLayouts = ownership.buildKnownOrcaWorkspaceLayouts(settings, target) + const isLegacyRepoForVisibility = ownership.isLegacyRepoForExternalWorktreeVisibility(target) + const liveWorktrees = dedupeWorktreesByPath(gitWorktrees.filter((info) => !info.prunable)) + const worktreeVisibilitySourceMatcher = createWorktreeVisibilitySourceMatcher( + [target.path, ...liveWorktrees.map((worktree) => worktree.path)], + resolveCustomWorktreeVisibilitySources(target, settings.worktreeVisibilityDefaults), + resolveConfiguredWorktreeBasePaths(target) + ) + const allMeta = allMetaOverride ?? store.getAllWorktreeMeta?.() + const repoOwnerCount = store.getRepos().filter((candidate) => candidate.id === target.id).length + const detectedRows = liveWorktrees.map((info) => { + const worktreeId = `${target.id}::${info.path}` + const legacyMeta = store.getWorktreeMeta?.(worktreeId) + const metaById = allMeta ?? (legacyMeta ? { [worktreeId]: legacyMeta } : {}) + let meta = + readWorktreeMetaForHost(store, worktreeId, getRepoExecutionHostId(target)) ?? + getRepoOwnedWorktreeMeta(target, worktreeId, metaById, repoOwnerCount) + const worktree = mergeWorktree(target.id, info, meta, target.displayName) + const detected = ownership.toDetectedWorktree({ + repo: target, + worktree, + meta, + settings, + knownOrcaLayouts, + isLegacyRepoForVisibility, + worktreeVisibilitySourceMatcher + }) + if (!detected.visible) { + return detected + } + meta = resolveWorktreeMetaWithDiscoveryBackfill( + store, + target, + worktreeId, + allMeta, + repoOwnerCount + ) + return ownership.toDetectedWorktree({ + repo: target, + worktree: mergeWorktree(target.id, info, meta, target.displayName), + meta, + settings, + knownOrcaLayouts, + isLegacyRepoForVisibility, + worktreeVisibilitySourceMatcher + }) + }) + return projectResolvedWorktreeLineage(detectedRows, store.getAllWorktreeLineage?.() ?? {}) +} + +describe('buildDetectedGitWorktrees classification passes', () => { + beforeEach(() => { + toDetectedWorktreeSpy.mockClear() + // Discovery backfill stamps lastActivityAt from the clock; freeze it so equivalence is deterministic. + vi.spyOn(Date, 'now').mockReturnValue(1_700_000_000_000) + }) + + it('classifies each visible worktree once per catalog pass, not twice', () => { + const paths = ['/workspace/one', '/workspace/two', '/workspace/three'] + const meta = Object.fromEntries( + paths.map((path) => [`${repo.id}::${path}`, settledMeta({ displayName: path })]) + ) + const store = createStore(meta) + + const detected = buildDetectedGitWorktrees(store, repo, paths.map(gitWorktree), meta) + + expect(detected).toHaveLength(3) + expect(detected.every((row) => row.visible)).toBe(true) + expect(toDetectedWorktreeSpy).toHaveBeenCalledTimes(paths.length) + }) + + it('reads the locator-keyed metadata row only when no host snapshot is available', () => { + const worktreeId = `${repo.id}::/workspace/one` + const meta = { [worktreeId]: settledMeta() } + const store = createStore(meta) + const legacyReads = vi.spyOn(store, 'getWorktreeMeta') + + buildDetectedGitWorktrees(store, repo, [gitWorktree('/workspace/one')], meta) + expect(legacyReads).not.toHaveBeenCalled() + + // Partial stores (compatibility shapes) expose no snapshot, so the locator-keyed lookup must still run. + const partialStore = createStore(meta) as Partial + delete partialStore.getAllWorktreeMeta + delete partialStore.getAllWorktreeMetaForHost + delete partialStore.getWorktreeMetaForHost + const partialLegacyReads = vi.spyOn(partialStore as Store, 'getWorktreeMeta') + + const rows = buildDetectedGitWorktrees( + partialStore as Store, + repo, + [gitWorktree('/workspace/one')], + undefined + ) + expect(partialLegacyReads).toHaveBeenCalledWith(worktreeId) + expect(rows[0]).toMatchObject({ id: worktreeId, lastActivityAt: 5 }) + }) + + it.each([ + ['settled metadata', () => settledMeta()], + ['metadata needing discovery backfill', () => ({ orcaCreatedAt: 1 }) as WorktreeMeta], + ['no metadata at all', () => undefined] + ])('emits a catalog deep-equal to the two-pass build for %s', (_label, makeMeta) => { + const worktreeId = `${repo.id}::/workspace/one` + const seed = makeMeta() + const build = (fn: typeof buildDetectedGitWorktrees) => + fn( + createStore(seed ? { [worktreeId]: seed } : {}), + repo, + [gitWorktree('/workspace/one'), gitWorktree('/workspace/hidden-external')], + seed ? { [worktreeId]: seed } : {} + ) + + expect(build(buildDetectedGitWorktrees)).toEqual(build(buildDetectedGitWorktreesTwoPass)) + }) + + it('emits a catalog deep-equal to the two-pass build for a folder-style listing with no host snapshot', () => { + const worktreeId = `${repo.id}::/workspace/one` + const seed = settledMeta() + const build = (fn: typeof buildDetectedGitWorktrees) => + fn(createStore({ [worktreeId]: seed }), repo, [gitWorktree('/workspace/one')], undefined) + + expect(build(buildDetectedGitWorktrees)).toEqual(build(buildDetectedGitWorktreesTwoPass)) + }) +}) diff --git a/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts b/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts index 7ec2cac1fc9..5c734d8bcd9 100644 --- a/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts +++ b/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts @@ -155,9 +155,10 @@ export function buildDetectedGitWorktrees( const repoOwnerCount = store.getRepos().filter((candidate) => candidate.id === repo.id).length const detected = liveWorktrees.map((gitWorktree) => { const worktreeId = `${repo.id}::${gitWorktree.path}` - const legacyMeta = store.getWorktreeMeta?.(worktreeId) + // Why: the locator-keyed row is only a stand-in for a missing host snapshot, so don't read it when we have one. + const legacyMeta = allMeta === undefined ? store.getWorktreeMeta?.(worktreeId) : undefined const metaById = allMeta ?? (legacyMeta ? { [worktreeId]: legacyMeta } : {}) - let meta = + const meta = readWorktreeMetaForHost(store, worktreeId, getRepoExecutionHostId(repo)) ?? getRepoOwnedWorktreeMeta(repo, worktreeId, metaById, repoOwnerCount) const worktree = mergeWorktree(repo.id, gitWorktree, meta, repo.displayName) @@ -174,17 +175,21 @@ export function buildDetectedGitWorktrees( return detected } - meta = resolveWorktreeMetaWithDiscoveryBackfill( + const backfilledMeta = resolveWorktreeMetaWithDiscoveryBackfill( store, repo, worktreeId, allMeta, repoOwnerCount ) + // Why: backfill hands back the same object when it wrote nothing, and both builders are pure over it. + if (backfilledMeta === meta) { + return detected + } return toDetectedWorktree({ repo, - worktree: mergeWorktree(repo.id, gitWorktree, meta, repo.displayName), - meta, + worktree: mergeWorktree(repo.id, gitWorktree, backfilledMeta, repo.displayName), + meta: backfilledMeta, settings, knownOrcaLayouts, isLegacyRepoForVisibility, diff --git a/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts b/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts index 6af4d45c3cd..b17677ddfcc 100644 --- a/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts +++ b/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts @@ -40,8 +40,9 @@ export function resolveWorktreeMetaWithDiscoveryBackfill( repoOwnerCount = store.getRepos().filter((candidate) => candidate.id === repo.id).length ): WorktreeMeta { const executionHostId = getRepoExecutionHostId(repo) - const legacyMeta = store.getWorktreeMeta?.(worktreeId) const allMeta = allMetaOverride ?? store.getAllWorktreeMeta?.() + // Why: the locator-keyed row is only a stand-in for a missing snapshot, so don't read it when we have one. + const legacyMeta = allMeta === undefined ? store.getWorktreeMeta?.(worktreeId) : undefined const existing = readWorktreeMetaForHost(store, worktreeId, executionHostId) ?? getRepoOwnedWorktreeMeta( diff --git a/src/relay/git-porcelain-local-parity.test.ts b/src/relay/git-porcelain-local-parity.test.ts index d4969f6fd0e..9fccc85e7b3 100644 --- a/src/relay/git-porcelain-local-parity.test.ts +++ b/src/relay/git-porcelain-local-parity.test.ts @@ -160,7 +160,8 @@ describe('relay/desktop unmerged-entry porcelain parity', () => { const unmergedLines = [ 'u UU N... 100644 100644 100644 100644 aa bb cc plain.ts', 'u UD N... 100644 100644 000000 100644 aa bb cc "present \\303\\251.ts"', - 'u UD N... 100644 100644 000000 100644 aa bb cc "missing \\303\\251.ts"', + // mW=000000: real Git reports an absent working-tree path this way, and the file is not created below. + 'u UD N... 100644 100644 000000 000000 aa bb cc "missing \\303\\251.ts"', 'u DD N... 100644 100644 000000 000000 aa bb cc both-gone.ts' ] const git = vi.fn(async (args) => { diff --git a/src/shared/git-status-conflict-entries.test.ts b/src/shared/git-status-conflict-entries.test.ts new file mode 100644 index 00000000000..ce6b5ab9e9e --- /dev/null +++ b/src/shared/git-status-conflict-entries.test.ts @@ -0,0 +1,87 @@ +/** + * Asymmetric `u` records used to cost one `fs.access` each — a 9p/network round trip per conflict on + * a WSL or remote worktree. Porcelain v2 already carries the answer in the worktree mode (`mW`), so + * the probe must not come back. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as NodeFsPromisesModule from 'node:fs/promises' + +const { accessMock } = vi.hoisted(() => ({ accessMock: vi.fn() })) + +vi.mock('node:fs/promises', async (importOriginal) => ({ + ...(await importOriginal()), + access: accessMock +})) + +import { parseUnmergedEntry } from './git-status-conflict-entries' + +const WORKTREE = '/repo' + +/** `u

` — `mW` is the working-tree mode. */ +function unmergedLine(xy: string, modeWorktree: string, filePath: string): string { + return `u ${xy} N... 100644 100644 100644 ${modeWorktree} aaa bbb ccc ${filePath}` +} + +const ASYMMETRIC_KINDS = ['AU', 'UA', 'DU', 'UD'] as const + +describe('parseUnmergedEntry', () => { + beforeEach(() => { + accessMock.mockReset() + accessMock.mockRejectedValue(new Error('fs.access must not be reached for well-formed records')) + }) + + it('reads the working-tree mode instead of probing the filesystem', async () => { + for (const xy of ASYMMETRIC_KINDS) { + const absent = await parseUnmergedEntry(WORKTREE, unmergedLine(xy, '000000', 'gone.ts')) + const present = await parseUnmergedEntry(WORKTREE, unmergedLine(xy, '100644', 'here.ts')) + + expect(absent?.status, xy).toBe('deleted') + expect(present?.status, xy).toBe('modified') + } + + // The regression this replaces: one probe per asymmetric row, serialised across the status poll. + expect(accessMock).not.toHaveBeenCalled() + }) + + it('treats a symlink left in place of the conflicted file as present', async () => { + const entry = await parseUnmergedEntry(WORKTREE, unmergedLine('UD', '120000', 'link.ts')) + + expect(entry?.status).toBe('modified') + expect(accessMock).not.toHaveBeenCalled() + }) + + it('resolves the symmetric kinds from XY alone, whatever the working-tree mode says', async () => { + const bothModified = await parseUnmergedEntry(WORKTREE, unmergedLine('UU', '000000', 'a.ts')) + const bothAdded = await parseUnmergedEntry(WORKTREE, unmergedLine('AA', '000000', 'b.ts')) + const bothDeleted = await parseUnmergedEntry(WORKTREE, unmergedLine('DD', '100644', 'c.ts')) + + expect(bothModified?.status).toBe('modified') + expect(bothAdded?.status).toBe('modified') + expect(bothDeleted?.status).toBe('deleted') + expect(accessMock).not.toHaveBeenCalled() + }) + + it('drops submodule conflicts without probing', async () => { + const line = 'u UU S... 160000 160000 160000 160000 aa bb cc vendor/sub' + + expect(await parseUnmergedEntry(WORKTREE, line)).toBeNull() + expect(accessMock).not.toHaveBeenCalled() + }) + + it('keeps the working-tree probe as a fallback for a mode no real Git emits', async () => { + accessMock.mockRejectedValueOnce(Object.assign(new Error('nope'), { code: 'ENOENT' })) + const missing = await parseUnmergedEntry(WORKTREE, unmergedLine('UD', 'zzzzzz', 'weird-a.ts')) + + accessMock.mockResolvedValueOnce(undefined) + const found = await parseUnmergedEntry(WORKTREE, unmergedLine('UD', '12345', 'weird-b.ts')) + + accessMock.mockRejectedValueOnce(Object.assign(new Error('denied'), { code: 'EACCES' })) + const unreadable = await parseUnmergedEntry(WORKTREE, unmergedLine('UD', '', 'weird-c.ts')) + + expect(missing?.status).toBe('deleted') + expect(found?.status).toBe('modified') + // Why: an ambiguous fs failure keeps the row visible rather than falsely reading as 'deleted'. + expect(unreadable?.status).toBe('modified') + expect(accessMock).toHaveBeenCalledTimes(3) + }) +}) diff --git a/src/shared/git-status-conflict-entries.ts b/src/shared/git-status-conflict-entries.ts index a50d389e2bb..980b4960d8b 100644 --- a/src/shared/git-status-conflict-entries.ts +++ b/src/shared/git-status-conflict-entries.ts @@ -3,6 +3,8 @@ import * as path from 'node:path' import type { GitConflictKind, GitFileStatus, GitStatusEntry } from './git-status-types' import { decodeGitCQuotedPath } from './git-cquoted-path' +const OCTAL_FILE_MODE = /^[0-7]{6}$/ + export async function parseUnmergedEntry( worktreePath: string, line: string @@ -13,6 +15,7 @@ export async function parseUnmergedEntry( const modeStage1 = parts[3] const modeStage2 = parts[4] const modeStage3 = parts[5] + const modeWorktree = parts[6] const filePath = decodeGitCQuotedPath(parts.slice(10).join(' ')) if (!filePath) { return null @@ -32,7 +35,12 @@ export async function parseUnmergedEntry( return { path: filePath, area: 'unstaged', - status: await getConflictCompatibilityStatus(worktreePath, filePath, conflictKind), + status: await getConflictCompatibilityStatus( + worktreePath, + filePath, + conflictKind, + modeWorktree + ), conflictKind, conflictStatus: 'unresolved' } @@ -60,11 +68,12 @@ function parseConflictKind(xy: string): GitConflictKind | null { } // Why: `status` here is a rendering-compat choice for icon/color plumbing, not semantic; the conflict badge carries the real meaning. -// Why: for deleted_by_*/added_by_* variants Git's result depends on merge strategy, so check the filesystem. +// Why: for deleted_by_*/added_by_* variants Git's result depends on merge strategy, so ask whether the path is in the working tree. async function getConflictCompatibilityStatus( worktreePath: string, filePath: string, - conflictKind: GitConflictKind + conflictKind: GitConflictKind, + modeWorktree: string ): Promise { if (conflictKind === 'both_modified' || conflictKind === 'both_added') { return 'modified' @@ -74,8 +83,14 @@ async function getConflictCompatibilityStatus( return 'deleted' } - // Why async: on a WSL worktree this path is a `\\wsl.localhost\...` share, and a sync probe - // per asymmetric conflict blocks the Electron main thread for a 9p round trip each. + // Why: `mW` is the worktree mode Git already stat'ed for this row — `000000` means absent. Reading + // it costs nothing and stays consistent with the rest of the snapshot, whereas a re-probe here is a + // 9p/network round trip per asymmetric conflict on a WSL or remote worktree. + if (OCTAL_FILE_MODE.test(modeWorktree)) { + return modeWorktree === '000000' ? 'deleted' : 'modified' + } + + // Why: only reachable on output no real Git emits (truncated/malformed `u` record). try { await access(path.join(worktreePath, filePath)) return 'modified' From d247d6441ba09c23987065631e02c44dd4fba8d6 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 21:04:04 -0700 Subject: [PATCH 04/49] perf(startup): overlap the runtime capability refresh with the session-tabs inventory (#18460) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The startup structured-session restore chained `runtime:getStatus` before `session.tabs.listAll`, but the capability value is discarded at that call site — it only seeds the module cache later launch flows read, and the inventory fetch never reads it. On a profile with 413 worktrees / 801 tabs that serial leg cost a median 109 ms of the did-finish-load -> renderer-startup-hydration-done window. Issue both calls concurrently. `Promise.all` still resolves only after both settle, so the capability cache is populated no later than before. --- ...local-structured-session-tabs-sync.test.ts | 37 +++++++++++++++++++ .../inventory-refresh.ts | 10 +++-- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/renderer/src/runtime/local-structured-session-tabs-sync.test.ts b/src/renderer/src/runtime/local-structured-session-tabs-sync.test.ts index 7a6c713f37f..702c5570c0d 100644 --- a/src/renderer/src/runtime/local-structured-session-tabs-sync.test.ts +++ b/src/renderer/src/runtime/local-structured-session-tabs-sync.test.ts @@ -307,6 +307,43 @@ describe('local structured session tab projection', () => { } }) + it('starts the session-tabs inventory without waiting for the capability refresh', async () => { + const priorApi = window.api + let releaseStatus = (): void => undefined + const statusGate = new Promise((resolve) => { + releaseStatus = resolve + }) + const getStatus = vi.fn(async () => { + await statusGate + return { capabilities: [STRUCTURED_AGENT_SESSION_RUNTIME_CAPABILITY] } + }) + const call = vi.fn().mockResolvedValue({ ok: true, result: { snapshots: [] } }) + Object.defineProperty(window, 'api', { + configurable: true, + value: { runtime: { getStatus, call } } + }) + try { + vi.resetModules() + const { restoreLocalStructuredSessionTabsOnce } = + await import('./local-structured-session-tabs-sync') + let settled = false + const restored = restoreLocalStructuredSessionTabsOnce().finally(() => { + settled = true + }) + expect(getStatus).toHaveBeenCalledOnce() + // The inventory RPC must already be in flight while the capability refresh is pending. + expect(call).toHaveBeenCalledWith({ method: 'session.tabs.listAll', params: {} }) + // ...and overlapping must not let the restore open the gate before capabilities land. + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(settled).toBe(false) + releaseStatus() + await restored + expect(call).toHaveBeenCalledOnce() + } finally { + Object.defineProperty(window, 'api', { configurable: true, value: priorApi }) + } + }) + it('accepts a newer session after merged content returns to the base epoch', () => { const state = createSnapshot() const base = { diff --git a/src/renderer/src/runtime/local-structured-session-tabs-sync/inventory-refresh.ts b/src/renderer/src/runtime/local-structured-session-tabs-sync/inventory-refresh.ts index d0f29ec0cf8..af756458707 100644 --- a/src/renderer/src/runtime/local-structured-session-tabs-sync/inventory-refresh.ts +++ b/src/renderer/src/runtime/local-structured-session-tabs-sync/inventory-refresh.ts @@ -10,10 +10,14 @@ import { applyStructuredSessionTabSnapshots } from './snapshot-apply' export function restoreLocalStructuredSessionTabsOnce( expectedGeneration = localStructuredSessionGeneration() ): Promise { + // Why concurrent: the capability refresh only seeds the module cache that later launch + // flows read; the inventory fetch never reads it, so chaining them only paid a second + // serial IPC round-trip on the startup gate. return latchLocalStructuredSessionRestore(() => - refreshLocalRuntimeCapabilities() - .then(() => refreshLocalStructuredSessionTabs(expectedGeneration)) - .then(() => undefined) + Promise.all([ + refreshLocalRuntimeCapabilities(), + refreshLocalStructuredSessionTabs(expectedGeneration) + ]).then(() => undefined) ) } From 6815fed6d66d2ac0a28123d5e68834b91843564c Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 21:04:54 -0700 Subject: [PATCH 05/49] perf(worktrees): converge the trash sweep instead of re-walking doomed trees (#18429) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(worktrees): converge the trash sweep instead of re-walking doomed trees `transientLockRemovalOptions()` only asked for `maxRetries` on Windows, and `removeHostTree`'s retry ladder was gated on `process.platform === 'win32'`. A concurrent writer is not Windows-specific: Spotlight/`mds`, a scanner, or a live process writing under the tree surface the same EBUSY/ENOTEMPTY/EPERM on macOS and Linux. So on POSIX the startup sweep got exactly one attempt per entry, failed, and re-issued the same guaranteed-to-fail walk on every launch. - Extend the retry policy to every platform. Windows keeps its error set, its message fallback, and its delays; the message fallback stays Windows-only because POSIX always sets a code. - Persist a per-entry failure ledger in the trash root so a repeatedly failing entry is retried on a 15m/1h/6h ladder rather than on every launch. Nothing is abandoned: the ladder clamps, records are pruned when the entry goes, and a torn ledger fails open to a full sweep. - Defer the sweep behind first paint, so its recursive readdir/rm no longer competes with window creation and worktree-catalog hydration. * fix(worktrees): keep Node's per-level rm retries Windows-only Node's rimraf hands every child back to the retrying entry point (`_rmchildren` -> `rimraf`), so `maxRetries` is applied once per directory level and compounds: a permanently-failing leaf at depth d costs roughly `retryDelay * 36 * 9^(d-1)`. Measured on macOS against one `chflags uchg` file at depth 2, `{recursive, force}` rejected in 1 ms while `{maxRetries: 8, retryDelay: 150}` had not settled after 5 minutes. Handing those options to POSIX removals turned every `removeHostTree` on a worktree residue (`node_modules/.pnpm/...`, a dozen levels deep) into a promise that never settles -- wedging the serialized trash-deletion queue, hanging the sweep on its first failing entry so no backoff is ever recorded, and leaving the unregistered-worktree removal IPC pending forever. Keep the cross-platform retry where this PR put it -- the bounded outer ladders that re-issue one whole `rm` against the same already-chosen path -- and restore `transientLockRemovalOptions()` to Windows-only `maxRetries`. Also guard the deferred first-window task: off whenReady's promise chain a synchronous throw is an uncaughtException, which the pipe-error guard re-throws fatally. * fix(worktrees): make host tree removal see through Electron's asar shim The 267 stranded trash entries were not a concurrent-writer race. Electron patches `fs` so a `*.asar` file reports `isDirectory() === true`, so Node's recursive `rm` descends into the archive, `rmdir`s a real file, and fails the parent with ENOTEMPTY — deterministically, on every attempt. Every worktree that has run `pnpm install` carries a `default_app.asar`, which is why every residue stopped at the same path. Route `removeHostTree` through `original-fs` (Electron's unpatched `fs`, with a `node:fs/promises` fallback outside Electron) instead of retrying a failure that can never succeed. `removalPath`, `rmOptions` and the Windows retry ladder are byte-identical to `main`. Reverts the POSIX retry ladder, the `isTransientRemovalError` widening, the sweep backoff ledger and the inverted `does not retry host removal failures outside Windows` ratchet — none of them were fixing the actual failure. * fix(worktrees): drop the stray orchestration test and bundle the asar guard like production `orchestration-statement-compilation.test.ts` belongs to #18420 and was swept into this branch by accident. It imports `./prepared-statement-cache`, which does not exist here, so `tsc -p config/tsconfig.node.json` failed on this branch. Removed; typecheck is clean again. The Electron asar guard pre-externalized `original-fs` in its own Vite build, which is not what the shipped bundle does. Mirror `isExternalMainModule` from electron.vite.config.ts instead, so the guard also proves the production bundler leaves `createRequire(__filename)('original-fs')` as a runtime require — if that ever became a static import or got folded, production would silently degrade to the shimmed `fs` while the old test kept passing. --- src/main/asar-transparent-fs.test.ts | 43 ++++++ src/main/asar-transparent-fs.ts | 35 +++++ .../host-tree-removal-asar.electron.test.ts | 132 ++++++++++++++++++ src/main/host-tree-removal.ts | 8 +- .../host/deferred-secret-protection-report.ts | 20 +-- src/main/startup/first-window-deferral.ts | 37 +++++ .../startup/main-process-ready-runtime.ts | 20 ++- 7 files changed, 268 insertions(+), 27 deletions(-) create mode 100644 src/main/asar-transparent-fs.test.ts create mode 100644 src/main/asar-transparent-fs.ts create mode 100644 src/main/host-tree-removal-asar.electron.test.ts create mode 100644 src/main/startup/first-window-deferral.ts diff --git a/src/main/asar-transparent-fs.test.ts b/src/main/asar-transparent-fs.test.ts new file mode 100644 index 00000000000..f416450599d --- /dev/null +++ b/src/main/asar-transparent-fs.test.ts @@ -0,0 +1,43 @@ +import { mkdir, mkdtemp, writeFile } from 'node:fs/promises' +import { existsSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterAll, describe, expect, it } from 'vitest' +import { rm } from './asar-transparent-fs' + +// Why not an asar fixture here: plain Node has no asar shim to see through, so the archive case can +// only be settled by the real binary — `host-tree-removal-asar.electron.test.ts` does that. What +// this pins is the other half: outside Electron `original-fs` does not resolve, and the helper has +// to degrade to `node:fs/promises` rather than throw at first use. +const roots: string[] = [] + +afterAll(async () => { + for (const root of roots) { + await rm(root, { recursive: true, force: true }).catch(() => {}) + } +}) + +describe('asar-transparent rm', () => { + it('removes a tree recursively where `original-fs` is unresolvable', async () => { + expect(process.versions.electron).toBeUndefined() + const root = await mkdtemp(join(tmpdir(), 'orca-asar-transparent-')) + roots.push(root) + const target = join(root, 'wt-1700000000000-abcdef01') + await mkdir(join(target, 'nested'), { recursive: true }) + await writeFile(join(target, 'nested', 'file.txt'), 'x', 'utf8') + + await expect(rm(target, { recursive: true, force: true })).resolves.toBeUndefined() + + expect(existsSync(target)).toBe(false) + expect(existsSync(root)).toBe(true) + }) + + it('honours `force: false` rather than swallowing a missing path', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-asar-transparent-')) + roots.push(root) + + await expect(rm(join(root, 'absent'), { recursive: true })).rejects.toMatchObject({ + code: 'ENOENT' + }) + }) +}) diff --git a/src/main/asar-transparent-fs.ts b/src/main/asar-transparent-fs.ts new file mode 100644 index 00000000000..cddab843434 --- /dev/null +++ b/src/main/asar-transparent-fs.ts @@ -0,0 +1,35 @@ +// Why: Electron patches `fs` so a `*.asar` file reports `isDirectory() === true`, so Node's +// recursive `rm` descends into the archive, tries to `rmdir` a real file, and fails the parent with +// ENOTEMPTY. Every worktree that has ever run `pnpm install` carries at least one +// (`node_modules/.pnpm/electron@…/…/Electron.app/Contents/Resources/default_app.asar`), so a +// worktree removal aborts there deterministically — the residue is not a concurrent-writer race and +// no amount of retrying clears it. `original-fs` is Electron's unpatched `fs`; unlike +// `process.noAsar` it is scoped to this call rather than to the whole process, which matters because +// a multi-GB removal runs for seconds while the main process may still be loading modules out of +// `app.asar`. See `cli/appimage-payload-removal.ts` for the same bug at a call site short enough to +// use the process-global flag. + +import { rm as nodeRm } from 'node:fs/promises' +import { createRequire } from 'node:module' + +type Rm = typeof nodeRm + +let resolvedRm: Rm | undefined + +function resolveRm(): Rm { + try { + // Why require and not an import: `original-fs` only exists inside Electron, so vitest, the + // `orca` CLI and the plain-node entrypoints must resolve `node:fs/promises` instead — and there + // the shim does not exist either, so plain `fs` is already asar-transparent. + const originalFs = createRequire(__filename)('original-fs') as { promises?: { rm?: Rm } } + return typeof originalFs.promises?.rm === 'function' ? originalFs.promises.rm : nodeRm + } catch { + return nodeRm + } +} + +/** `fs.promises.rm` that sees a `*.asar` as the file it is rather than as a directory. */ +export const rm: Rm = (path, options) => { + resolvedRm ??= resolveRm() + return resolvedRm(path, options) +} diff --git a/src/main/host-tree-removal-asar.electron.test.ts b/src/main/host-tree-removal-asar.electron.test.ts new file mode 100644 index 00000000000..a41a55035ac --- /dev/null +++ b/src/main/host-tree-removal-asar.electron.test.ts @@ -0,0 +1,132 @@ +import { spawnSync } from 'node:child_process' +import { + copyFileSync, + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + writeFileSync +} from 'node:fs' +import { createRequire, isBuiltin } from 'node:module' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' +import { afterAll, describe, expect, it } from 'vitest' +import { removeTreeSync } from '../shared/windows-transient-lock-removal' + +/** + * Why the real binary: Electron patches `fs` so a `*.asar` file reports `isDirectory() === true`, so + * a recursive `rm` descends into the archive, `rmdir`s a real file, and fails the parent with + * ENOTEMPTY. Plain Node has no such shim, so no in-process unit test can reproduce it — and every + * worktree that has run `pnpm install` carries a `default_app.asar`, which is what stranded 267 + * trash entries on the reporting machine. This runs the shipped `removeHostTree` against a real + * archive under the real binary. + */ +const requireFromTest = createRequire(import.meta.url) +const electronBinary = requireFromTest('electron') as string +const electronDist = join(dirname(requireFromTest.resolve('electron/package.json')), 'dist') +const FIXTURE_ASAR = [ + join(electronDist, 'Electron.app/Contents/Resources/default_app.asar'), + join(electronDist, 'resources/default_app.asar') +].find((candidate) => existsSync(candidate)) + +// Mirrors the residue reported on the failing machine, down to the depth of the blocking leaf. +const ENTRY_NAME = 'wt-1700000000000-abcdef01' +const ASAR_PARENT = 'node_modules/.pnpm/electron/node_modules/electron/dist/App/Contents/Resources' + +const roots: string[] = [] + +afterAll(() => { + for (const root of roots) { + try { + removeTreeSync(root) + } catch { + // A fixture the shim strands is exactly what this file is about; never fail teardown on it. + } + } +}) + +type ProbeResult = { failure: string | null; residue: string[] } + +function buildDriver(bundlePath: string, target: string, resultPath: string): string { + return [ + `const fs = require('node:fs')`, + `const { removeHostTree } = require(${JSON.stringify(bundlePath)})`, + // Why noAsar for the read-back: the shim would report the stranded archive as a directory here + // too, so the residue listing has to be taken with real filesystem semantics. + `const withoutAsar = (fn) => { const prev = process.noAsar; process.noAsar = true; try { return fn() } finally { process.noAsar = prev } }`, + `;(async () => {`, + ` let failure = null`, + ` try { await removeHostTree(${JSON.stringify(target)}) } catch (error) { failure = error.code ?? String(error) }`, + ` const residue = withoutAsar(() => fs.existsSync(${JSON.stringify(target)})`, + ` ? fs.readdirSync(${JSON.stringify(target)}, { recursive: true }).map(String)`, + ` : [])`, + ` fs.writeFileSync(${JSON.stringify(resultPath)}, JSON.stringify({ failure, residue }))`, + `})()` + ].join('\n') +} + +async function bundleHostTreeRemoval(outFile: string): Promise { + const { build } = await import('vite') + const result = await build({ + root: process.cwd(), + configFile: false, + logLevel: 'error', + build: { + write: false, + minify: false, + ssr: true, + rollupOptions: { + input: 'src/main/host-tree-removal.ts', + // Why mirror `isExternalMainModule` from electron.vite.config.ts exactly — CJS, and + // `original-fs` deliberately *not* externalized: the shipped bundle does not list it either, + // so if the archive-aware `rm` ever became a static import (or the bundler learned to fold + // `createRequire(...)('original-fs')`) production would silently degrade to the shimmed `fs` + // while a test that pre-externalized it kept passing. + output: { format: 'cjs' }, + external: (id: string) => isBuiltin(id) || id === 'electron' || id.startsWith('electron/') + } + } + }) + const output = (Array.isArray(result) ? result[0] : result) as { output: { code?: string }[] } + const code = output.output[0]?.code + expect(typeof code).toBe('string') + writeFileSync(outFile, code as string, 'utf8') +} + +function buildStrandedTree(root: string): string { + const target = join(root, ENTRY_NAME) + const asarParent = join(target, ...ASAR_PARENT.split('/')) + mkdirSync(asarParent, { recursive: true }) + copyFileSync(FIXTURE_ASAR as string, join(asarParent, 'default_app.asar')) + writeFileSync(join(asarParent, 'plain.txt'), 'x', 'utf8') + return target +} + +describe('removeHostTree against a tree holding an asar archive', () => { + it.runIf(FIXTURE_ASAR)( + 'removes the whole tree under the real Electron binary', + async () => { + const root = mkdtempSync(join(tmpdir(), 'orca-host-tree-asar-')) + roots.push(root) + const bundlePath = join(root, 'host-tree-removal.cjs') + await bundleHostTreeRemoval(bundlePath) + const target = buildStrandedTree(root) + const resultPath = join(root, 'result.json') + const driverPath = join(root, 'driver.cjs') + writeFileSync(driverPath, buildDriver(bundlePath, target, resultPath), 'utf8') + + const run = spawnSync(electronBinary, [driverPath], { + encoding: 'utf8', + env: { ...process.env, ELECTRON_RUN_AS_NODE: '1' }, + timeout: 60_000 + }) + expect(run.status, run.stderr?.slice(-2000)).toBe(0) + + const probe = JSON.parse(readFileSync(resultPath, 'utf8')) as ProbeResult + // Without an asar-transparent `rm` this is `ENOTEMPTY` and the residue stops at the archive, + // on every attempt, forever — it is not a race a retry can win. + expect(probe).toEqual({ failure: null, residue: [] }) + }, + 120_000 + ) +}) diff --git a/src/main/host-tree-removal.ts b/src/main/host-tree-removal.ts index a5d5d447956..f789a9861d0 100644 --- a/src/main/host-tree-removal.ts +++ b/src/main/host-tree-removal.ts @@ -1,10 +1,12 @@ // Why: every recursive host delete Orca performs (worktrees, terminal history, quarantined recovery -// generations) hits the same Windows stickiness — AV/indexers/late handle releases surface transient -// EBUSY/ENOTEMPTY/EPERM on a tree Node just emptied. One helper so no call site forgets the retries. +// generations) hits the same two hazards, so one helper exists so no call site forgets either. +// Windows stickiness — AV/indexers/late handle releases surface transient EBUSY/ENOTEMPTY/EPERM on a +// tree Node just emptied — and Electron's asar shim, which strands any tree holding a `*.asar` +// (see `asar-transparent-fs`). -import { rm } from 'node:fs/promises' import { win32 } from 'node:path' import { setTimeout as delay } from 'node:timers/promises' +import { rm } from './asar-transparent-fs' import { isWindowsAbsolutePathLike } from '../shared/cross-platform-path' import { isWslUncPath } from '../shared/wsl-paths' import { transientLockRemovalOptions } from '../shared/windows-transient-lock-removal' diff --git a/src/main/host/deferred-secret-protection-report.ts b/src/main/host/deferred-secret-protection-report.ts index 8f5be1fb047..7b4ea14367c 100644 --- a/src/main/host/deferred-secret-protection-report.ts +++ b/src/main/host/deferred-secret-protection-report.ts @@ -1,4 +1,4 @@ -import { app, type BrowserWindow } from 'electron' +import { runAfterFirstWindowShown } from '../startup/first-window-deferral' import { reportSecretProtectionGap } from './secret-protection-report' /** @@ -72,21 +72,5 @@ export function scheduleSecretProtectionGapReport({ } } - let ran = false - const run = (): void => { - if (ran) { - return - } - ran = true - clearTimeout(fallback) - // Why setImmediate: keep the blocking keyring probe off the event handler that - // reveals the window, so the reveal paints first. - setImmediate(report) - } - - const fallback = setTimeout(run, REPORT_FALLBACK_MS) - fallback.unref?.() - app.once('browser-window-created', (_event: Electron.Event, window: BrowserWindow) => { - window.once('ready-to-show', run) - }) + runAfterFirstWindowShown(report, REPORT_FALLBACK_MS) } diff --git a/src/main/startup/first-window-deferral.ts b/src/main/startup/first-window-deferral.ts new file mode 100644 index 00000000000..6a144549efe --- /dev/null +++ b/src/main/startup/first-window-deferral.ts @@ -0,0 +1,37 @@ +import { app, type BrowserWindow } from 'electron' + +/** + * Run `task` once the first window can paint, or after `fallbackMs` if it never does. + * + * For startup work nothing on the critical path consumes: a probe or a disk sweep started before the + * window exists competes with window creation for the same main thread and libuv threadpool, and the + * user sees that as the app being slow to open. + * + * Why a fallback as well as the window event: `ready-to-show` can fail to fire at all when the + * GPU/driver cannot present (see main-window-state-lifecycle), and headless serve has no window. + */ +export function runAfterFirstWindowShown(task: () => void, fallbackMs: number): void { + let ran = false + const run = (): void => { + if (ran) { + return + } + ran = true + clearTimeout(fallback) + // Why setImmediate: keep the work off the event handler that reveals the window, so it paints first. + // Why the guard: off whenReady's promise chain a synchronous throw is an uncaughtException, and + // installUncaughtPipeErrorGuard re-throws those fatally — deferred startup chores are never that. + setImmediate(() => { + try { + task() + } catch (error) { + console.warn('[startup] deferred first-window task failed', error) + } + }) + } + const fallback = setTimeout(run, fallbackMs) + fallback.unref?.() + app.once('browser-window-created', (_event: Electron.Event, window: BrowserWindow) => { + window.once('ready-to-show', run) + }) +} diff --git a/src/main/startup/main-process-ready-runtime.ts b/src/main/startup/main-process-ready-runtime.ts index f89f63186a0..75784a4c196 100644 --- a/src/main/startup/main-process-ready-runtime.ts +++ b/src/main/startup/main-process-ready-runtime.ts @@ -32,8 +32,12 @@ import { import { initializeMainProcessAutomations } from './main-process-automations' import { initializeMainProcessPlugins } from './main-process-plugins' import { collectWorktreeTrashSweepRoots, sweepStaleWorktreeTrash } from '../worktree-trash' +import { runAfterFirstWindowShown } from './first-window-deferral' import { logStartupMilestone } from './startup-diagnostics' +// Headless serve never opens a window, so the sweep still has to run off a timer there. +const WORKTREE_TRASH_SWEEP_FALLBACK_MS = 15_000 + export async function initializeReadyRuntimeServices(): Promise { const store = state.store if (!store) { @@ -74,12 +78,16 @@ export async function initializeReadyRuntimeServices(): Promise { state.emulatorBridge = new EmulatorBridge() runtime.setEmulatorBridge(state.emulatorBridge) // Why: worktree deletion renames the checkout aside and deletes it in the background, so a quit or - // crash mid-delete can leave the moved directory on disk. - void sweepStaleWorktreeTrash( - collectWorktreeTrashSweepRoots(store.getRepos(), store.getSettings()) - ).catch((error) => { - console.warn('[worktrees] Failed to sweep leftover worktree directories:', error) - }) + // crash mid-delete can leave the moved directory on disk. Why deferred: the sweep's recursive + // readdir/rm runs on the same libuv threadpool the window's first paint and worktree-catalog + // hydration are reading disk on, and nothing on the startup path consumes its result. + runAfterFirstWindowShown(() => { + void sweepStaleWorktreeTrash( + collectWorktreeTrashSweepRoots(store.getRepos(), store.getSettings()) + ).catch((error) => { + console.warn('[worktrees] Failed to sweep leftover worktree directories:', error) + }) + }, WORKTREE_TRASH_SWEEP_FALLBACK_MS) nativeTheme.themeSource = store.getSettings().theme ?? 'system' // Why (#16441): the real-home grant runs a codex app-server session. It stays // ordered before managed-hook reconciliation — an incapable host must re-arm From 558f57de58958bfaf57dbe9c5df6da5d4f7130b4 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 21:05:55 -0700 Subject: [PATCH 06/49] perf(source-control): sort branch entries before filtering, gate projections by view mode (#18426) * perf(source-control): sort branch entries before filtering, gate projections by view mode Two dead-work fixes in the Source Control file projection. 1. filterAndSortSourceControlPathEntries copied and re-sorted the uncapped branch entry list with Intl.Collator on every keystroke. Sort once on branchEntries, filter after: Array#filter preserves order and compareFileNames is a total order, so filter(sort(x)) === sort(filter(x)). 2. The tree projection was built in list mode and the list projection in tree mode, then discarded. Gate each memo on sourceControlViewMode and return a shared empty projection, matching the combined-diff file tree precedent. * docs(source-control): drop the total-order premise from the projection sort argument The sort-before-filter swap does not need compareFileNames to be a total order. A stable Array#sort places each element by (comparator result, original index) and Array#filter disturbs neither, so filter(sort(x)) === sort(filter(x)) for any self-consistent comparator -- which the previous filter-then-sort already required. Restating that removes a shared-module property (the code-unit tie-break in file-name-sort.ts) from this hook's correctness argument instead of defending it. Also record on the EMPTY_* singletons that the gates and both branching consumers read one sourceControlViewMode prop in one synchronous render, so the off-mode value cannot reach the screen, and warn against deriving the mode from a separate store read. New guard: matches filter-then-sort under a comparator that is not a total order. It ties every path sharing a top-level directory over 300 entries and fails against a correct-but-unstable sort. With the duplicate paths removed from ORDERING_FIXTURE the pre-existing equivalence test passes under that same mutant, so this is the only test that pins stability. No behaviour change: counters over first render + 8 keystrokes at n=2000 are identical before and after (34685 compareFileNames calls, 0 tree builds in list mode). * refactor(source-control): freeze the empty branch-tree singleton Object.freeze([]) matches the other three empty projection singletons and the combined-diff-file-tree precedent; readonly types keep it honest. --- .../source-control-file-filter.test.ts | 21 -- .../right-sidebar/source-control-tree.ts | 2 +- .../source-control/listing/branch-section.tsx | 2 +- .../source-control/listing/file-filter.ts | 10 - .../listing/use-file-projection-work.test.tsx | 323 ++++++++++++++++++ .../listing/use-file-projection.ts | 64 +++- 6 files changed, 379 insertions(+), 43 deletions(-) create mode 100644 src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection-work.test.tsx diff --git a/src/renderer/src/components/right-sidebar/source-control-file-filter.test.ts b/src/renderer/src/components/right-sidebar/source-control-file-filter.test.ts index 6b2419c9195..18d8ad5f3b9 100644 --- a/src/renderer/src/components/right-sidebar/source-control-file-filter.test.ts +++ b/src/renderer/src/components/right-sidebar/source-control-file-filter.test.ts @@ -1,7 +1,6 @@ import { describe, expect, it } from 'vitest' import { SOURCE_CONTROL_FILE_FILTER_QUERY_MAX_BYTES, - filterAndSortSourceControlPathEntries, filterSourceControlGroupedPathEntries, filterSourceControlPathEntries, getSourceControlFileFilterState, @@ -10,26 +9,6 @@ import { } from './source-control/listing/file-filter' describe('source-control-file-filter', () => { - it('naturally orders committed branch rows without mutating store input', () => { - const entries = [ - { path: 'migrations/100.sql' }, - { path: 'migrations/9.sql' }, - { path: 'migrations/99.sql' } - ] - - expect( - filterAndSortSourceControlPathEntries(entries, { - normalizedFilter: '', - tooLarge: false - }).map((entry) => entry.path) - ).toEqual(['migrations/9.sql', 'migrations/99.sql', 'migrations/100.sql']) - expect(entries.map((entry) => entry.path)).toEqual([ - 'migrations/100.sql', - 'migrations/9.sql', - 'migrations/99.sql' - ]) - }) - it('normalizes bounded queries and filters entries by path', () => { const filter = getSourceControlFileFilterState(' SRC/button ') diff --git a/src/renderer/src/components/right-sidebar/source-control-tree.ts b/src/renderer/src/components/right-sidebar/source-control-tree.ts index eb4a01a7ea3..d855201646b 100644 --- a/src/renderer/src/components/right-sidebar/source-control-tree.ts +++ b/src/renderer/src/components/right-sidebar/source-control-tree.ts @@ -165,7 +165,7 @@ export function buildGitStatusSourceControlTree( } export function flattenSourceControlTree( - nodes: SourceControlTreeNode[], + nodes: readonly SourceControlTreeNode[], collapsedDirectoryKeys: ReadonlySet ): SourceControlTreeNode[] { const result: SourceControlTreeNode[] = [] diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/branch-section.tsx b/src/renderer/src/components/right-sidebar/source-control/listing/branch-section.tsx index ab37458adb7..3f2ab4723f7 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/branch-section.tsx +++ b/src/renderer/src/components/right-sidebar/source-control/listing/branch-section.tsx @@ -39,7 +39,7 @@ export function SourceControlBranchSection({ collapsedSections: Set toggleSection: (section: string) => void sourceControlViewMode: SourceControlViewMode - visibleBranchTreeRows: SourceControlTreeNode[] + visibleBranchTreeRows: readonly SourceControlTreeNode[] fileListScrollElement: HTMLDivElement | null collapsedTreeDirs: Set toggleTreeDir: (key: string) => void diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/file-filter.ts b/src/renderer/src/components/right-sidebar/source-control/listing/file-filter.ts index 4bdd676bcf5..ae379b51d4c 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/file-filter.ts +++ b/src/renderer/src/components/right-sidebar/source-control/listing/file-filter.ts @@ -1,5 +1,4 @@ import { isClipboardTextByteLengthOverLimit } from '../../../../../../shared/clipboard-text' -import { compareFileNames } from '../../../../../../shared/file-name-sort' export const SOURCE_CONTROL_FILE_FILTER_QUERY_MAX_BYTES = 2 * 1024 @@ -49,15 +48,6 @@ export function filterSourceControlPathEntries return entries.filter((entry) => entry.path.toLowerCase().includes(filter.normalizedFilter)) } -export function filterAndSortSourceControlPathEntries( - entries: T[], - filter: SourceControlFileFilterState -): T[] { - return [...filterSourceControlPathEntries(entries, filter)].sort((a, b) => - compareFileNames(a.path, b.path) - ) -} - export function filterSourceControlGroupedPathEntries( grouped: SourceControlGroupedPathEntries, filter: SourceControlFileFilterState diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection-work.test.tsx b/src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection-work.test.tsx new file mode 100644 index 00000000000..ed62e05a25a --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection-work.test.tsx @@ -0,0 +1,323 @@ +// @vitest-environment happy-dom + +import { renderHook } from '@testing-library/react' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { GitBranchChangeEntry } from '../../../../../../shared/git-diff-compare-types' +import type { GitStatusEntry } from '../../../../../../shared/git-status-types' +import type { SourceControlViewMode } from '../../../../../../shared/ui-chrome-types' +import type * as FileNameSortModule from '../../../../../../shared/file-name-sort' +import type * as SourceControlTreeModule from '../../source-control-tree' +import type * as SubmoduleExpansionModule from './submodule-expansion' + +const counters = vi.hoisted(() => ({ + compareFileNames: 0, + buildGitStatusSourceControlTree: 0, + buildSourceControlTree: 0, + flattenSourceControlTree: 0, + injectExpandedSubmoduleRows: 0, + injectExpandedSubmoduleEntries: 0 +})) + +/** Lets a test swap in a comparator that is deliberately not a total order. */ +const comparatorOverride = vi.hoisted(() => ({ + current: null as ((a: string, b: string) => number) | null +})) + +vi.mock('../../../../../../shared/file-name-sort', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + compareFileNames: (a: string, b: string) => { + counters.compareFileNames += 1 + return comparatorOverride.current + ? comparatorOverride.current(a, b) + : actual.compareFileNames(a, b) + } + } +}) + +vi.mock('../../source-control-tree', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + buildGitStatusSourceControlTree: ( + ...args: Parameters + ) => { + counters.buildGitStatusSourceControlTree += 1 + return actual.buildGitStatusSourceControlTree(...args) + }, + buildSourceControlTree: ((...args: unknown[]) => { + counters.buildSourceControlTree += 1 + return (actual.buildSourceControlTree as (...a: unknown[]) => unknown)(...args) + }) as typeof actual.buildSourceControlTree, + flattenSourceControlTree: ((...args: unknown[]) => { + counters.flattenSourceControlTree += 1 + return (actual.flattenSourceControlTree as (...a: unknown[]) => unknown)(...args) + }) as typeof actual.flattenSourceControlTree + } +}) + +vi.mock('./submodule-expansion', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + injectExpandedSubmoduleRows: ((...args: unknown[]) => { + counters.injectExpandedSubmoduleRows += 1 + return (actual.injectExpandedSubmoduleRows as (...a: unknown[]) => unknown)(...args) + }) as typeof actual.injectExpandedSubmoduleRows, + injectExpandedSubmoduleEntries: ((...args: unknown[]) => { + counters.injectExpandedSubmoduleEntries += 1 + return (actual.injectExpandedSubmoduleEntries as (...a: unknown[]) => unknown)(...args) + }) as typeof actual.injectExpandedSubmoduleEntries + } +}) + +const { compareFileNames } = await import('../../../../../../shared/file-name-sort') +const { getSourceControlFileFilterState, filterSourceControlPathEntries } = + await import('./file-filter') +const { useSourceControlFileProjection } = await import('./use-file-projection') + +const NO_ENTRIES: GitStatusEntry[] = [] +const NO_COLLAPSED_TREE_DIRS = new Set() +const NO_EXPANDED_SUBMODULES = new Set() +const NO_COLLAPSED_SECTIONS = new Set() +const NO_SUBMODULE_STATUS = {} +const GROUP_ORDER = ['unstaged', 'staged', 'untracked'] as const + +type ProjectionProps = { + entries: GitStatusEntry[] + branchEntries: GitBranchChangeEntry[] + filterQuery: string + sourceControlViewMode: SourceControlViewMode +} + +function renderProjection(initialProps: ProjectionProps) { + return renderHook( + (props: ProjectionProps) => + useSourceControlFileProjection({ + entries: props.entries, + branchEntries: props.branchEntries, + filterQuery: props.filterQuery, + sourceControlGroupOrder: GROUP_ORDER, + activeWorktreeId: 'wt-1', + worktreePath: '/repo', + isFolder: false, + collapsedTreeDirs: NO_COLLAPSED_TREE_DIRS, + expandedSubmoduleKeys: NO_EXPANDED_SUBMODULES, + submoduleStatusByKey: NO_SUBMODULE_STATUS, + sourceControlViewMode: props.sourceControlViewMode, + collapsedSections: NO_COLLAPSED_SECTIONS + }), + { initialProps } + ) +} + +function makeBranchEntries(count: number): GitBranchChangeEntry[] { + return Array.from({ length: count }, (_, index) => ({ + path: `src/area-${index % 7}/nested/deep-${index % 13}/file-${index}.ts`, + status: 'modified' as const + })) +} + +/** Numeric collation, case variants, unicode, collator ties, and an exact duplicate path. */ +const ORDERING_FIXTURE: GitBranchChangeEntry[] = [ + { path: 'migrations/100.sql', status: 'modified' }, + { path: 'migrations/9.sql', status: 'modified' }, + { path: 'migrations/99.sql', status: 'added' }, + { path: 'migrations/02.sql', status: 'modified' }, + { path: 'migrations/2.sql', status: 'deleted' }, + { path: 'src/Button.tsx', status: 'modified' }, + { path: 'src/button.tsx', status: 'added' }, + { path: 'src/éclair.ts', status: 'modified' }, + { path: 'src/eclair.ts', status: 'deleted' }, + { path: 'src/Éclair.ts', status: 'modified' }, + { path: 'src/日本語.ts', status: 'added' }, + { path: 'src/dup.ts', status: 'modified' }, + { path: 'src/dup.ts', status: 'added' } +] + +/** The pre-change implementation: filter, then copy-and-sort. */ +function legacyFilterThenSort( + entries: GitBranchChangeEntry[], + filterQuery: string +): GitBranchChangeEntry[] { + const state = getSourceControlFileFilterState(filterQuery) + return [...filterSourceControlPathEntries(entries, state)].sort((a, b) => + compareFileNames(a.path, b.path) + ) +} + +function makeStatusEntries(count: number): GitStatusEntry[] { + return Array.from({ length: count }, (_, index) => ({ + path: `src/area-${index % 7}/nested/deep-${index % 13}/file-${index}.ts`, + status: 'modified' as const, + area: (['unstaged', 'staged', 'untracked'] as const)[index % 3] + })) +} + +/** + * Deliberately not a total order: every path under the same top-level directory compares equal, so + * distinct paths tie. Sort-before-filter must still match filter-before-sort under it. + */ +function compareTopLevelDirOnly(a: string, b: string): number { + const dirA = a.slice(0, a.indexOf('/')) + const dirB = b.slice(0, b.indexOf('/')) + return dirA < dirB ? -1 : dirA > dirB ? 1 : 0 +} + +/** Big enough that V8 leaves binary insertion sort for TimSort, where instability would show. */ +function makeTieHeavyEntries(count: number): GitBranchChangeEntry[] { + return Array.from({ length: count }, (_, index) => ({ + // Scrambled so the tie order is not already the sorted order. + path: `dir-${(index * 7) % 3}/file-${(index * 31) % count}.ts`, + status: 'modified' as const + })) +} + +beforeEach(() => { + comparatorOverride.current = null + for (const key of Object.keys(counters) as (keyof typeof counters)[]) { + counters[key] = 0 + } +}) + +describe('useSourceControlFileProjection branch entry ordering', () => { + it('sorts committed branch entries once across many filter changes', () => { + const branchEntries = makeBranchEntries(400) + const { rerender } = renderProjection({ + entries: NO_ENTRIES, + branchEntries, + filterQuery: '', + sourceControlViewMode: 'list' + }) + + const comparesForInitialSort = counters.compareFileNames + expect(comparesForInitialSort).toBeGreaterThan(0) + + for (const filterQuery of ['f', 'fi', 'fil', 'file', 'file-', 'file-1']) { + rerender({ entries: NO_ENTRIES, branchEntries, filterQuery, sourceControlViewMode: 'list' }) + } + + expect(counters.compareFileNames).toBe(comparesForInitialSort) + }) + + it('produces the same order as the previous filter-then-sort for every filter', () => { + const { result, rerender } = renderProjection({ + entries: NO_ENTRIES, + branchEntries: ORDERING_FIXTURE, + filterQuery: '', + sourceControlViewMode: 'list' + }) + + for (const filterQuery of ['', 'src', 'MIGRATIONS', 'é', '9', 'dup', 'no-match']) { + rerender({ + entries: NO_ENTRIES, + branchEntries: ORDERING_FIXTURE, + filterQuery, + sourceControlViewMode: 'list' + }) + expect(result.current.filteredBranchEntries).toEqual( + legacyFilterThenSort(ORDERING_FIXTURE, filterQuery) + ) + } + }) + + // Guards the invariant the sort-before-filter swap actually rests on: a stable sort, not a total + // order. If sortedBranchEntries ever stops preserving the original order of tied paths, this + // diverges from filter-then-sort even though every total-order fixture above still passes. + it('matches filter-then-sort under a comparator that is not a total order', () => { + comparatorOverride.current = compareTopLevelDirOnly + const branchEntries = makeTieHeavyEntries(300) + const { result, rerender } = renderProjection({ + entries: NO_ENTRIES, + branchEntries, + filterQuery: '', + sourceControlViewMode: 'list' + }) + + for (const filterQuery of ['', 'dir-1', 'file-1', 'file-12', '7.ts', 'no-match']) { + rerender({ entries: NO_ENTRIES, branchEntries, filterQuery, sourceControlViewMode: 'list' }) + expect(result.current.filteredBranchEntries.map((entry) => entry.path)).toEqual( + legacyFilterThenSort(branchEntries, filterQuery).map((entry) => entry.path) + ) + } + }) + + it('does not mutate the store-owned branch entry array', () => { + const branchEntries = [...ORDERING_FIXTURE] + renderProjection({ + entries: NO_ENTRIES, + branchEntries, + filterQuery: '', + sourceControlViewMode: 'list' + }) + + expect(branchEntries).toEqual(ORDERING_FIXTURE) + }) +}) + +describe('useSourceControlFileProjection view-mode gating', () => { + const entries = makeStatusEntries(120) + const branchEntries = makeBranchEntries(120) + + it('builds no tree projection in list mode', () => { + const { result } = renderProjection({ + entries, + branchEntries, + filterQuery: '', + sourceControlViewMode: 'list' + }) + + expect(counters.buildGitStatusSourceControlTree).toBe(0) + expect(counters.buildSourceControlTree).toBe(0) + expect(counters.flattenSourceControlTree).toBe(0) + expect(counters.injectExpandedSubmoduleRows).toBe(0) + expect(counters.injectExpandedSubmoduleEntries).toBeGreaterThan(0) + expect(result.current.visibleTreeRowsBySection).toEqual({}) + expect(result.current.visibleBranchTreeRows).toEqual([]) + }) + + it('builds no list projection in tree mode', () => { + const { result } = renderProjection({ + entries, + branchEntries, + filterQuery: '', + sourceControlViewMode: 'tree' + }) + + expect(counters.injectExpandedSubmoduleEntries).toBe(0) + expect(counters.buildGitStatusSourceControlTree).toBeGreaterThan(0) + expect(counters.buildSourceControlTree).toBeGreaterThan(0) + expect(result.current.visibleListRowsBySection).toEqual({}) + }) + + it('has the other mode fully projected on the first render after a switch', () => { + const { result, rerender } = renderProjection({ + entries, + branchEntries, + filterQuery: '', + sourceControlViewMode: 'list' + }) + const listRows = result.current.visibleListRowsBySection + const listSelectionCount = result.current.visibleSelectionEntries.length + expect(listSelectionCount).toBe(entries.length) + + rerender({ entries, branchEntries, filterQuery: '', sourceControlViewMode: 'tree' }) + + expect(result.current.visibleListRowsBySection).toEqual({}) + expect(result.current.visibleBranchTreeRows.length).toBeGreaterThan(0) + expect( + Object.values(result.current.visibleTreeRowsBySection).reduce( + (total, rows) => total + rows.length, + 0 + ) + ).toBeGreaterThan(0) + expect(result.current.visibleSelectionEntries.length).toBe(listSelectionCount) + + rerender({ entries, branchEntries, filterQuery: '', sourceControlViewMode: 'list' }) + + expect(result.current.visibleTreeRowsBySection).toEqual({}) + expect(result.current.visibleBranchTreeRows).toEqual([]) + expect(result.current.visibleListRowsBySection).toEqual(listRows) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection.ts b/src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection.ts index c885eaa33dc..66a370dd14d 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection.ts +++ b/src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection.ts @@ -2,10 +2,11 @@ import { useMemo } from 'react' import type { GitBranchChangeEntry } from '../../../../../../shared/git-diff-compare-types' import type { GitStatusEntry } from '../../../../../../shared/git-status-types' import type { SourceControlViewMode } from '../../../../../../shared/ui-chrome-types' +import { compareFileNames } from '../../../../../../shared/file-name-sort' import { compareGitStatusEntries } from '../../source-control-status-sort' import { - filterAndSortSourceControlPathEntries, filterSourceControlGroupedPathEntries, + filterSourceControlPathEntries, getSourceControlFileFilterState, type SourceControlFileFilterState } from './file-filter' @@ -56,10 +57,28 @@ export type SourceControlFileProjection = { visibleListRowsBySection: Partial< Record > - visibleBranchTreeRows: SourceControlTreeNode[] + visibleBranchTreeRows: readonly SourceControlTreeNode[] visibleSelectionEntries: FlatEntry[] } +// Why: only one view mode is ever rendered, so building the other mode's projection is pure dead +// work (precedent: the combined-diff file tree short-circuits the same way while collapsed). +// The gates below and both branching consumers (section-file-list.tsx, branch-section.tsx) read the +// same sourceControlViewMode prop within one synchronous render, so a mode switch can never show +// these. Keep that single source: deriving the mode from a separate store read would let a consumer +// switch a render before the memos do, and only then could one of these reach the screen. +const EMPTY_TREE_ROOTS_BY_SECTION: Readonly< + Partial> +> = Object.freeze({}) +const EMPTY_TREE_ROWS_BY_SECTION: Readonly< + Partial> +> = Object.freeze({}) +const EMPTY_LIST_ROWS_BY_SECTION: Readonly< + Partial> +> = Object.freeze({}) +const EMPTY_BRANCH_TREE_NODES: readonly SourceControlTreeNode[] = + Object.freeze([]) + export function useSourceControlFileProjection({ entries, branchEntries, @@ -127,12 +146,24 @@ export function useSourceControlFileProjection({ [unfilteredDisplaySections] ) + // Why: sorting before filtering keeps the collator off the keystroke path, and is order-identical + // to the old filter-then-sort for any self-consistent comparator (a total order is not required): + // a stable sort fixes each element's position by (comparator result, original index), and + // Array#filter drops elements without disturbing either, so re-sorting the survivors would + // reproduce the same relative order. filter(sort(x)) === sort(filter(x)). + const sortedBranchEntries = useMemo( + () => [...branchEntries].sort((a, b) => compareFileNames(a.path, b.path)), + [branchEntries] + ) const filteredBranchEntries = useMemo( - () => filterAndSortSourceControlPathEntries(branchEntries, fileFilterState), - [branchEntries, fileFilterState] + () => filterSourceControlPathEntries(sortedBranchEntries, fileFilterState), + [fileFilterState, sortedBranchEntries] ) const treeRootsBySection = useMemo(() => { + if (sourceControlViewMode !== 'tree') { + return EMPTY_TREE_ROOTS_BY_SECTION + } const roots: Partial> = {} for (const section of displaySections) { @@ -148,9 +179,12 @@ export function useSourceControlFileProjection({ : sectionRoots } return roots - }, [displaySections]) + }, [displaySections, sourceControlViewMode]) const visibleTreeRowsBySection = useMemo(() => { + if (sourceControlViewMode !== 'tree') { + return EMPTY_TREE_ROWS_BY_SECTION + } const rows: Partial> = {} for (const section of displaySections) { rows[section.id] = injectExpandedSubmoduleRows( @@ -167,11 +201,15 @@ export function useSourceControlFileProjection({ displaySections, treeRootsBySection, expandedSubmoduleKeys, + sourceControlViewMode, submoduleStatusByKey ]) // List view needs the same lazy submodule expansion as tree view, spliced into the flat entry list. const visibleListRowsBySection = useMemo(() => { + if (sourceControlViewMode !== 'list') { + return EMPTY_LIST_ROWS_BY_SECTION + } const rows: Partial> = {} for (const section of displaySections) { rows[section.id] = injectExpandedSubmoduleEntries( @@ -183,15 +221,21 @@ export function useSourceControlFileProjection({ ) } return rows - }, [displaySections, expandedSubmoduleKeys, submoduleStatusByKey]) + }, [displaySections, expandedSubmoduleKeys, sourceControlViewMode, submoduleStatusByKey]) const branchTreeRoots = useMemo( - () => compactSourceControlTree(buildSourceControlTree('branch', filteredBranchEntries)), - [filteredBranchEntries] + () => + sourceControlViewMode === 'tree' + ? compactSourceControlTree(buildSourceControlTree('branch', filteredBranchEntries)) + : EMPTY_BRANCH_TREE_NODES, + [filteredBranchEntries, sourceControlViewMode] ) const visibleBranchTreeRows = useMemo( - () => flattenSourceControlTree(branchTreeRoots, collapsedTreeDirs), - [branchTreeRoots, collapsedTreeDirs] + () => + sourceControlViewMode === 'tree' + ? flattenSourceControlTree(branchTreeRoots, collapsedTreeDirs) + : EMPTY_BRANCH_TREE_NODES, + [branchTreeRoots, collapsedTreeDirs, sourceControlViewMode] ) const visibleSelectionEntries = useMemo(() => { From 6a5aa1904f2f1c0d5e3329ceed05a006b974a318 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 21:11:02 -0700 Subject: [PATCH 07/49] perf(renderer): load the project-location and feedback dialogs on click (#18440) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(renderer): load the project-location and feedback dialogs on click Both are reachable only from an explicit click, but their chunks sat on the renderer boot graph and were fetched and parsed on every launch. Route them through the existing `lazy-with-retry` helper, keeping each trigger eager so the click target still exists, and keep the mount sticky once opened so the dialog's own close animation and repeat opens are unaffected. Renderer boot graph 4,473,242 -> 4,424,142 bytes (-49,100 B / -47.9 KiB). Trade-off: the first open per session now waits on a local chunk fetch — measured at ~0.53 ms (project location) and ~0.26 ms (feedback) of read plus V8 parse/compile, warm page cache. * test(renderer): flush the lazy set-location chunk in the ready-target test Without the flush this case only passed because an earlier test in the file had already resolved the shared lazy chunk; it fails under -t filtering. * perf(renderer): warm the lazy dialog chunks on their precursor Both deferred dialogs have a guaranteed, strictly-earlier precursor: the composer only renders "Set location" for a needs-setup host that can take one, and Send Feedback only exists inside an open help menu. Warm each chunk there with a swallowed `import()` (the `preloadCommentMarkdown` pattern) so the fetch/parse happens while the user is reading the picker or the menu, not on the click. Boot graph is unchanged in kind: `import()` never enters modulepreload, so the win holds at -48,958 B (was -49,100 B before the warm; the 142 B is the warm's own source on an already-preloaded chunk). * test(renderer): make the composer warm guard's no-mount assertion real The mock stubbed SetProjectLocationDialog as `() => null`, so the "warming must not mount the dialog" assertion could never fail — the testid it looked for was not rendered under any condition. Render a marker unconditionally instead, matching the sidebar guard, so the assertion actually pins the behaviour. Verified non-vacuous: forcing the lazy element to mount eagerly now fails with "expected
to be null" rather than passing. * fix(renderer): latch the lazy dialog mounts in state instead of during render React Doctor's ref-mutated-during-render rule failed static analysis on both sticky-mount latches. Use the useState mount-flag idiom already in NewWorkspaceComposerModal (addProjectMounted), set from the open handler. --- ...aceComposerCard.set-location-warm.test.tsx | 123 +++++++++++++++++ ...orkspaceComposerCard.set-location.test.tsx | 130 ++---------------- .../NewWorkspaceComposerCard.test-fixture.tsx | 120 ++++++++++++++++ .../components/NewWorkspaceComposerCard.tsx | 51 +++++-- .../sidebar/SidebarSettingsHelpMenu.test.tsx | 37 ++++- .../sidebar/SidebarSettingsHelpMenu.tsx | 34 ++++- 6 files changed, 359 insertions(+), 136 deletions(-) create mode 100644 src/renderer/src/components/NewWorkspaceComposerCard.set-location-warm.test.tsx create mode 100644 src/renderer/src/components/NewWorkspaceComposerCard.test-fixture.tsx diff --git a/src/renderer/src/components/NewWorkspaceComposerCard.set-location-warm.test.tsx b/src/renderer/src/components/NewWorkspaceComposerCard.set-location-warm.test.tsx new file mode 100644 index 00000000000..fd1a16f7f0a --- /dev/null +++ b/src/renderer/src/components/NewWorkspaceComposerCard.set-location-warm.test.tsx @@ -0,0 +1,123 @@ +// @vitest-environment happy-dom + +import React from 'react' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { hostOptions, renderCard } from './NewWorkspaceComposerCard.test-fixture' +import type { ProjectHostSetupOption } from '@/lib/project-host-setup-options' + +// Counts evaluations of the set-location chunk. A dynamic import evaluates a module once, +// so this only moves when the composer actually reaches for the chunk. +const chunk = vi.hoisted(() => ({ loads: 0 })) + +// Renders a marker unconditionally so the "warming did not mount it" assertion below can +// actually fail; a `() => null` stub would make that check vacuous. +vi.mock('@/components/new-workspace/SetProjectLocationDialog', () => { + chunk.loads += 1 + return { + SetProjectLocationDialog: () =>
+ } +}) + +vi.mock('@/store', () => ({ + useAppStore: Object.assign( + (selector: (state: unknown) => unknown) => + selector({ + closeModal: vi.fn(), + openModal: vi.fn(), + openSettingsPage: vi.fn(), + openSettingsTarget: vi.fn(), + setRuntimeEnvironmentStatus: vi.fn(), + setupProjectExistingFolder: vi.fn(), + setupProjectClone: vi.fn(), + activeModal: 'new-workspace-composer', + settings: { defaultTuiAgent: null, disabledTuiAgents: [] }, + updateSettings: vi.fn(), + projects: [], + repos: [] + }), + { getState: () => ({}) } + ) +})) + +vi.mock('@/components/contextual-tours/use-contextual-tour', () => ({ + useContextualTour: vi.fn() +})) + +vi.mock('@/components/ui/tooltip', () => ({ + Tooltip: ({ children }: { children: React.ReactNode }) => <>{children}, + TooltipContent: ({ children }: { children: React.ReactNode }) => <>{children}, + TooltipTrigger: ({ children }: { children: React.ReactNode }) => <>{children} +})) + +vi.mock('@/components/agent/AgentCombobox', () => ({ + default: () => +})) + +vi.mock('@/components/sidebar/AddRemoteHostDialog', () => ({ + AddRemoteHostDialog: () => null +})) + +vi.mock('@/components/sparse/SparseCheckoutPresetSelect', () => ({ + default: () => null +})) + +vi.mock('@/components/new-workspace/SmartWorkspaceNameField', () => ({ + default: () => +})) + +vi.mock('@/components/new-workspace/ProjectCombobox', () => ({ + default: () =>
+})) + +const readyOnlyHostOptions = hostOptions.filter((option) => option.kind === 'ready') +// A disconnected host is a needs-setup row with no "Set location" action, so it must not warm. +const unavailableHostOptions: ProjectHostSetupOption[] = [ + ...readyOnlyHostOptions, + { + kind: 'needs-setup', + id: 'needs-setup:ssh:offline', + projectId: 'project-group:platform', + hostId: 'ssh:offline', + label: 'Offline box', + detail: 'Not connected', + isAvailable: false, + attention: false, + canSetLocation: false + } +] + +// Declaration order matters here and nowhere else: a module evaluates once, so the +// no-warm cases have to observe the counter before anything warms it. +describe('NewWorkspaceComposerCard set-location chunk warm', () => { + let container: HTMLDivElement | null = null + + afterEach(() => { + container?.remove() + container = null + }) + + it('does not warm the chunk when no host needs its location set', async () => { + container = await renderCard({ projectHostSetupOptions: readyOnlyHostOptions }) + + expect( + [...container.querySelectorAll('button')].some((button) => + button.textContent?.includes('Set project location') + ) + ).toBe(false) + expect(chunk.loads).toBe(0) + }) + + it('does not warm the chunk when the needs-setup host cannot take a location', async () => { + container = await renderCard({ projectHostSetupOptions: unavailableHostOptions }) + + expect(chunk.loads).toBe(0) + }) + + it('warms the chunk on mount for a needs-setup host, before Set project location is clicked', async () => { + container = await renderCard() + + expect(chunk.loads).toBe(1) + // Warming must not mount the dialog; it still waits on an explicit click. + expect(document.body.querySelector('[data-testid="set-project-location-dialog"]')).toBeNull() + }) +}) diff --git a/src/renderer/src/components/NewWorkspaceComposerCard.set-location.test.tsx b/src/renderer/src/components/NewWorkspaceComposerCard.set-location.test.tsx index 9cdea1d7d54..06718fab91b 100644 --- a/src/renderer/src/components/NewWorkspaceComposerCard.set-location.test.tsx +++ b/src/renderer/src/components/NewWorkspaceComposerCard.set-location.test.tsx @@ -1,11 +1,8 @@ // @vitest-environment happy-dom import React, { act } from 'react' -import { createRoot } from 'react-dom/client' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import NewWorkspaceComposerCard from './NewWorkspaceComposerCard' -import type { NewWorkspaceProjectOption } from '@/lib/new-workspace-project-options' -import type { ProjectHostSetupOption } from '@/lib/project-host-setup-options' +import { renderCard } from './NewWorkspaceComposerCard.test-fixture' const storeMocks = vi.hoisted(() => ({ closeModal: vi.fn(), @@ -99,118 +96,6 @@ vi.mock('@/components/new-workspace/SetProjectLocationDialog', () => ({ ) : null })) -const projectOptions: NewWorkspaceProjectOption[] = [ - { - kind: 'project-group', - id: 'project-group:platform', - projectGroupId: 'platform', - displayName: 'Platform', - badgeColor: 'var(--muted-foreground)', - detail: '/workspace/platform', - parentPath: '/workspace/platform', - connectionId: null - } -] - -const hostOptions: ProjectHostSetupOption[] = [ - { - kind: 'ready', - id: 'setup-local', - projectId: 'project-group:platform', - hostId: 'local', - repoId: 'repo-a', - label: 'Local Mac', - detail: 'Orca', - path: '/Users/alice/orca' - }, - { - kind: 'needs-setup', - id: 'needs-setup:ssh:devbox', - projectId: 'project-group:platform', - hostId: 'ssh:devbox', - label: 'Devbox', - detail: 'Project location not set', - isAvailable: true, - attention: false, - canSetLocation: true - } -] - -function renderCard( - overrides: Partial> = {} -): HTMLDivElement { - const container = document.createElement('div') - document.body.appendChild(container) - const root = createRoot(container) - act(() => { - root.render( - {}} - eligibleRepos={[]} - repoId="repo-a" - projectOptions={projectOptions} - selectedProjectId="project-group:platform" - selectedRepoIsGit - onRepoChange={() => {}} - onProjectChange={() => {}} - primaryActionLabel="Create workspace" - name="" - onNameValueChange={() => {}} - onSmartGitHubItemSelect={() => {}} - onSmartGitLabItemSelect={() => {}} - onSmartBranchSelect={() => {}} - onSmartLinearIssueSelect={() => {}} - smartNameSelection={null} - onClearSmartNameSelection={() => {}} - canReuseSelectedBranch={false} - reuseSelectedBranch={false} - onReuseSelectedBranchChange={() => {}} - forkPushWarning={null} - detectedAgentIds={null} - onOpenAgentSettings={() => {}} - advancedOpen={false} - onToggleAdvanced={() => {}} - parentWorktreeId={null} - onParentWorktreeIdChange={() => {}} - createDisabled={false} - projectError={null} - creating={false} - onCreate={() => {}} - note="" - onNoteChange={() => {}} - setupConfig={null} - requiresExplicitSetupChoice={false} - setupDecision={null} - onSetupDecisionChange={() => {}} - setupAgentStartupPolicy="start-immediately" - onSetupAgentStartupPolicyChange={() => {}} - shouldWaitForSetupCheck={false} - resolvedSetupDecision={null} - createError={null} - selectedRepoConnectionId={null} - selectedRepoSshStatus={null} - selectedRepoRequiresConnection={false} - selectedRepoConnectInProgress={false} - onConnectSelectedRepo={async () => {}} - canUseSparseCheckout={false} - sparsePresets={[]} - sparseSelectedPresetId={null} - onSparseSelectPreset={() => {}} - branchNameOverride={undefined} - onBranchNameOverrideChange={() => {}} - branchesEnabled={false} - setupControlsEnabled={false} - sparseControlsEnabled={false} - projectHostSetupOptions={hostOptions} - selectedProjectHostSetupId="setup-local" - {...overrides} - /> - ) - }) - return container -} - describe('NewWorkspaceComposerCard set location', () => { let container: HTMLDivElement | null = null @@ -225,9 +110,10 @@ describe('NewWorkspaceComposerCard set location', () => { container = null }) - it('opens set-location over the composer without leaving the create dialog', () => { + // Async because the dialog is a lazy chunk: the click mounts Suspense, the chunk resolves next tick. + it('opens set-location over the composer without leaving the create dialog', async () => { const nestedOpenChanges: boolean[] = [] - container = renderCard({ + container = await renderCard({ onNestedDialogOpenChange: (open) => nestedOpenChanges.push(open) }) @@ -239,6 +125,7 @@ describe('NewWorkspaceComposerCard set location', () => { ) expect(setLocation).toBeTruthy() act(() => setLocation?.click()) + await act(async () => {}) const dialog = document.body.querySelector('[data-testid="set-project-location-dialog"]') expect(dialog?.getAttribute('data-host')).toBe('Devbox') @@ -249,10 +136,12 @@ describe('NewWorkspaceComposerCard set location', () => { expect(storeMocks.openSettingsPage).not.toHaveBeenCalled() }) - it('closes the nested dialog before publishing the ready run target', () => { + // Async for the same reason: without the flush this only passes when an earlier + // test in this file already resolved the shared lazy chunk. + it('closes the nested dialog before publishing the ready run target', async () => { const nestedOpenChanges: boolean[] = [] const setupChanges: string[] = [] - container = renderCard({ + container = await renderCard({ onNestedDialogOpenChange: (open) => nestedOpenChanges.push(open), onProjectHostSetupChange: (setupId) => setupChanges.push(setupId) }) @@ -264,6 +153,7 @@ describe('NewWorkspaceComposerCard set location', () => { (button) => button.textContent?.includes('Set project location') ) act(() => setLocation?.click()) + await act(async () => {}) const complete = [...document.body.querySelectorAll('button')].find( (button) => button.textContent === 'Complete location' ) diff --git a/src/renderer/src/components/NewWorkspaceComposerCard.test-fixture.tsx b/src/renderer/src/components/NewWorkspaceComposerCard.test-fixture.tsx new file mode 100644 index 00000000000..c8eb2b56f43 --- /dev/null +++ b/src/renderer/src/components/NewWorkspaceComposerCard.test-fixture.tsx @@ -0,0 +1,120 @@ +import React, { act } from 'react' +import { createRoot } from 'react-dom/client' +import NewWorkspaceComposerCard from './NewWorkspaceComposerCard' +import type { NewWorkspaceProjectOption } from '@/lib/new-workspace-project-options' +import type { ProjectHostSetupOption } from '@/lib/project-host-setup-options' + +export const projectOptions: NewWorkspaceProjectOption[] = [ + { + kind: 'project-group', + id: 'project-group:platform', + projectGroupId: 'platform', + displayName: 'Platform', + badgeColor: 'var(--muted-foreground)', + detail: '/workspace/platform', + parentPath: '/workspace/platform', + connectionId: null + } +] + +export const hostOptions: ProjectHostSetupOption[] = [ + { + kind: 'ready', + id: 'setup-local', + projectId: 'project-group:platform', + hostId: 'local', + repoId: 'repo-a', + label: 'Local Mac', + detail: 'Orca', + path: '/Users/alice/orca' + }, + { + kind: 'needs-setup', + id: 'needs-setup:ssh:devbox', + projectId: 'project-group:platform', + hostId: 'ssh:devbox', + label: 'Devbox', + detail: 'Project location not set', + isAvailable: true, + attention: false, + canSetLocation: true + } +] + +export async function renderCard( + overrides: Partial> = {} +): Promise { + const container = document.createElement('div') + document.body.appendChild(container) + const root = createRoot(container) + act(() => { + root.render( + {}} + eligibleRepos={[]} + repoId="repo-a" + projectOptions={projectOptions} + selectedProjectId="project-group:platform" + selectedRepoIsGit + onRepoChange={() => {}} + onProjectChange={() => {}} + primaryActionLabel="Create workspace" + name="" + onNameValueChange={() => {}} + onSmartGitHubItemSelect={() => {}} + onSmartGitLabItemSelect={() => {}} + onSmartBranchSelect={() => {}} + onSmartLinearIssueSelect={() => {}} + smartNameSelection={null} + onClearSmartNameSelection={() => {}} + canReuseSelectedBranch={false} + reuseSelectedBranch={false} + onReuseSelectedBranchChange={() => {}} + forkPushWarning={null} + detectedAgentIds={null} + onOpenAgentSettings={() => {}} + advancedOpen={false} + onToggleAdvanced={() => {}} + parentWorktreeId={null} + onParentWorktreeIdChange={() => {}} + createDisabled={false} + projectError={null} + creating={false} + onCreate={() => {}} + note="" + onNoteChange={() => {}} + setupConfig={null} + requiresExplicitSetupChoice={false} + setupDecision={null} + onSetupDecisionChange={() => {}} + setupAgentStartupPolicy="start-immediately" + onSetupAgentStartupPolicyChange={() => {}} + shouldWaitForSetupCheck={false} + resolvedSetupDecision={null} + createError={null} + selectedRepoConnectionId={null} + selectedRepoSshStatus={null} + selectedRepoRequiresConnection={false} + selectedRepoConnectInProgress={false} + onConnectSelectedRepo={async () => {}} + canUseSparseCheckout={false} + sparsePresets={[]} + sparseSelectedPresetId={null} + onSparseSelectPreset={() => {}} + branchNameOverride={undefined} + onBranchNameOverrideChange={() => {}} + branchesEnabled={false} + setupControlsEnabled={false} + sparseControlsEnabled={false} + projectHostSetupOptions={hostOptions} + selectedProjectHostSetupId="setup-local" + {...overrides} + /> + ) + }) + // Settle the mount-time chunk warm before the click, so the click's import() is not + // overlapping an in-flight one (vitest's module runner serialises those; a browser does not). + await act(async () => {}) + return container +} diff --git a/src/renderer/src/components/NewWorkspaceComposerCard.tsx b/src/renderer/src/components/NewWorkspaceComposerCard.tsx index c6cca5192ea..d17bb8ba56d 100644 --- a/src/renderer/src/components/NewWorkspaceComposerCard.tsx +++ b/src/renderer/src/components/NewWorkspaceComposerCard.tsx @@ -11,7 +11,8 @@ import { AddRemoteHostDialog, type AddRemoteHostMode } from '@/components/sidebar/AddRemoteHostDialog' -import { SetProjectLocationDialog } from '@/components/new-workspace/SetProjectLocationDialog' +import { lazyWithRetry } from '@/lib/lazy-with-retry' +import type * as SetProjectLocationDialogModule from '@/components/new-workspace/SetProjectLocationDialog' import { unwrapRuntimeRpcResult } from '@/runtime/runtime-rpc-client' import { withUiConnectTimeout } from '@/ssh/ssh-connect-ui-timeout' import { isSshConnectInFlight, trackSshConnect } from '@/ssh/ssh-connect-in-flight' @@ -37,6 +38,20 @@ import { import { getSshStatusLabel } from './new-workspace/new-workspace-composer-ssh-status' import { useComposerFileDragOver } from './new-workspace/use-composer-file-drag-over' +// Why lazy: this pulls the ~41 KB project-location browser onto the boot graph, and nothing +// reaches it without an explicit "Set location" click. Shared with the warm below so both hit +// the same module-map entry. +const loadSetProjectLocationDialog = (): Promise => + import('@/components/new-workspace/SetProjectLocationDialog') + +const SetProjectLocationDialog = lazyWithRetry( + () => + loadSetProjectLocationDialog().then((module) => ({ + default: module.SetProjectLocationDialog + })), + { reloadKey: 'set-project-location-dialog' } +) + export default function NewWorkspaceComposerCard( props: NewWorkspaceComposerCardProps ): React.JSX.Element { @@ -83,6 +98,9 @@ export default function NewWorkspaceComposerCard( const [setLocationOption, setSetLocationOption] = React.useState( null ) + // Why sticky: the dialog animates itself closed off its own `option` prop, so unmounting it + // when the option clears would cut that animation short. + const [setLocationDialogMounted, setSetLocationDialogMounted] = React.useState(false) const selectedRepo = eligibleRepos.find((candidate) => candidate.id === repoId) const selectedRepoName = selectedRepo?.displayName ?? selectedRepo?.path ?? 'This project' @@ -96,6 +114,16 @@ export default function NewWorkspaceComposerCard( const needsSetupProjectHostSetupOptions = projectHostSetupOptions.filter( (option) => option.kind === 'needs-setup' ) + // Warm on the precursor: the "Set location" row only renders for a needs-setup host that can + // still take one, so the chunk resolves while the picker is being read rather than on the click. + const hasSetLocationOption = needsSetupProjectHostSetupOptions.some( + (option) => option.canSetLocation + ) + React.useEffect(() => { + if (hasSetLocationOption) { + void loadSetProjectLocationDialog().catch(() => {}) + } + }, [hasSetLocationOption]) const shouldShowRunTargetPicker = readyProjectHostSetupOptions.length > 0 || ephemeralVmRecipes.length > 0 || @@ -177,6 +205,7 @@ export default function NewWorkspaceComposerCard( }, [onAddProjectOverride, openModal]) const handleSetLocation = React.useCallback( (option: NeedsProjectHostOption): void => { + setSetLocationDialogMounted(true) setSetLocationOption(option) onNestedDialogOpenChange?.(true) }, @@ -319,14 +348,18 @@ export default function NewWorkspaceComposerCard( submitShortcutModifierLabel={getScreenSubmitModifierLabel()} /> - + {setLocationDialogMounted ? ( + + + + ) : null}
) } diff --git a/src/renderer/src/components/sidebar/SidebarSettingsHelpMenu.test.tsx b/src/renderer/src/components/sidebar/SidebarSettingsHelpMenu.test.tsx index b88bc1c102e..be0cbe27f97 100644 --- a/src/renderer/src/components/sidebar/SidebarSettingsHelpMenu.test.tsx +++ b/src/renderer/src/components/sidebar/SidebarSettingsHelpMenu.test.tsx @@ -14,6 +14,8 @@ const mocks = vi.hoisted(() => ({ updaterCheck: vi.fn(), shellOpenUrl: vi.fn(), useShortcutKeyDetails: vi.fn(), + /** Counts evaluations of the feedback chunk; a dynamic import evaluates it exactly once. */ + feedbackChunkLoads: 0, setupProgress: { ready: true, coreDoneCount: 2, @@ -56,7 +58,18 @@ vi.mock('../setup-guide/SetupGuideProgressRing', () => ({ })) vi.mock('@/components/ui/dropdown-menu', () => ({ - DropdownMenu: ({ children }: { children: ReactNode }) => <>{children}, + DropdownMenu: ({ + children, + onOpenChange + }: { + children: ReactNode + onOpenChange?: (open: boolean) => void + }) => ( + <> + - {!providerMissing ? ( + {!providerMissing && !reconnectRequired ? (