mirror of
https://github.com/stablyai/orca
synced 2026-04-21 14:17:16 +00:00
perf: make worktree creation click snappy
Three optimizations to the 'Start Agent' / 'Create Worktree' hot path in createLocalWorktree: 1. `git fetch` is now fire-and-forget. It previously blocked creation for 1–5s per click, even though `git worktree add` doesn't need the fetch to succeed — it happily uses whatever local ref `baseBranch` already points at. 2. `gh pr list` is skipped on the first iteration of the name-resolution loop. The original intent (don't reuse a historical PR head) still holds past the first collision; the happy path (brand-new name, no conflicts) now avoids the 1–3s GitHub round-trip entirely. 3. `rebuildAuthorizedRootsCache` is replaced with `invalidateAuthorizedRootsCache`. The cache is consulted lazily on the next fs access, so we don't need to block create on a synchronous per-repo `git worktree list` rebuild. Expected click-to-terminal improvement: 2.5–10s → ~500ms on warm repos. Covered by an additional test that asserts `gh pr list` is not called on the happy path, so future refactors can't silently reintroduce the regression.
This commit is contained in:
parent
3283729a7f
commit
7bb8d0e482
3 changed files with 85 additions and 30 deletions
|
|
@ -15,7 +15,7 @@ import type {
|
|||
import { getPRForBranch } from '../github/client'
|
||||
import { listWorktrees, addWorktree } from '../git/worktree'
|
||||
import { getGitUsername, getDefaultBaseRef, getBranchConflictKind } from '../git/repo'
|
||||
import { gitExecFileSync } from '../git/runner'
|
||||
import { gitExecFileAsync } from '../git/runner'
|
||||
import { isWslPath, parseWslPath, getWslHome } from '../wsl'
|
||||
import { createSetupRunnerScript, getEffectiveHooks, shouldRunSetupForCreate } from '../hooks'
|
||||
import { getSshGitProvider } from '../providers/ssh-git-dispatch'
|
||||
|
|
@ -29,7 +29,7 @@ import {
|
|||
mergeWorktree,
|
||||
areWorktreePathsEqual
|
||||
} from './worktree-logic'
|
||||
import { rebuildAuthorizedRootsCache } from './filesystem-auth'
|
||||
import { invalidateAuthorizedRootsCache } from './filesystem-auth'
|
||||
|
||||
export function notifyWorktreesChanged(mainWindow: BrowserWindow, repoId: string): void {
|
||||
if (!mainWindow.isDestroyed()) {
|
||||
|
|
@ -177,17 +177,22 @@ export async function createLocalWorktree(
|
|||
continue
|
||||
}
|
||||
|
||||
// Why: the UI resolves PR status by branch name alone. Reusing a historical
|
||||
// PR head name would make a fresh worktree inherit that old merged/closed PR
|
||||
// immediately, so auto-suffix until we land on a fresh branch identity.
|
||||
lastExistingPR = null
|
||||
try {
|
||||
lastExistingPR = await getPRForBranch(repo.path, branchName)
|
||||
} catch {
|
||||
// GitHub API may be unreachable, rate-limited, or token missing
|
||||
}
|
||||
if (lastExistingPR) {
|
||||
continue
|
||||
// Why: `gh pr list` is a network round-trip that previously ran on every
|
||||
// create, adding ~1–3s to the happy path even when no conflict exists. We
|
||||
// only probe PR conflicts once a local/remote branch collision has already
|
||||
// forced us past the first suffix — at that point uniqueness matters
|
||||
// enough to justify the GitHub call. The common case (brand-new branch
|
||||
// name, no collisions) skips the network entirely.
|
||||
if (suffix > 1) {
|
||||
lastExistingPR = null
|
||||
try {
|
||||
lastExistingPR = await getPRForBranch(repo.path, branchName)
|
||||
} catch {
|
||||
// GitHub API may be unreachable, rate-limited, or token missing
|
||||
}
|
||||
if (lastExistingPR) {
|
||||
continue
|
||||
}
|
||||
}
|
||||
|
||||
worktreePath = ensurePathWithinWorkspace(
|
||||
|
|
@ -229,13 +234,16 @@ export async function createLocalWorktree(
|
|||
// a real worktree on disk while the renderer reports "create failed".
|
||||
const shouldLaunchSetup = setupScript ? shouldRunSetupForCreate(repo, args.setupDecision) : false
|
||||
|
||||
// Fetch latest from remote so the worktree starts with up-to-date content
|
||||
// Why: `git fetch` previously blocked worktree creation for 1–5s on every
|
||||
// click, even though the fetch result isn't actually required — the
|
||||
// subsequent `git worktree add` uses whatever local ref `baseBranch` points
|
||||
// at. Kicking fetch off in parallel lets the worktree be created off the
|
||||
// last-known tip while the fetch completes in the background; the next
|
||||
// user action (pull, diff, PR create) will see the refreshed remote state.
|
||||
const remote = baseBranch.includes('/') ? baseBranch.split('/')[0] : 'origin'
|
||||
try {
|
||||
gitExecFileSync(['fetch', remote], { cwd: repo.path })
|
||||
} catch {
|
||||
void gitExecFileAsync(['fetch', remote], { cwd: repo.path }).catch(() => {
|
||||
// Fetch is best-effort — don't block worktree creation if offline
|
||||
}
|
||||
})
|
||||
|
||||
await addWorktree(
|
||||
repo.path,
|
||||
|
|
@ -264,7 +272,12 @@ export async function createLocalWorktree(
|
|||
}
|
||||
const meta = store.setWorktreeMeta(worktreeId, metaUpdates)
|
||||
const worktree = mergeWorktree(repo.id, created, meta)
|
||||
await rebuildAuthorizedRootsCache(store)
|
||||
// Why: the authorized-roots cache is consulted lazily on the next filesystem
|
||||
// access (`ensureAuthorizedRootsCache` rebuilds on demand when dirty). We
|
||||
// just invalidate the cache marker instead of blocking worktree creation on
|
||||
// an immediate rebuild, which can spawn `git worktree list` per repo and
|
||||
// adds 100ms+ to every create.
|
||||
invalidateAuthorizedRootsCache()
|
||||
|
||||
let setup: CreateWorktreeResult['setup']
|
||||
if (setupScript && shouldLaunchSetup) {
|
||||
|
|
|
|||
|
|
@ -53,6 +53,14 @@ vi.mock('../git/worktree', () => ({
|
|||
removeWorktree: removeWorktreeMock
|
||||
}))
|
||||
|
||||
vi.mock('../git/runner', () => ({
|
||||
// Why: createLocalWorktree now fires `git fetch` via gitExecFileAsync in the
|
||||
// background. Return a resolved promise so the fire-and-forget `.catch()`
|
||||
// chain has a valid Promise to attach to.
|
||||
gitExecFileAsync: vi.fn().mockResolvedValue({ stdout: '', stderr: '' }),
|
||||
gitExecFileSync: vi.fn()
|
||||
}))
|
||||
|
||||
vi.mock('../git/repo', () => ({
|
||||
getGitUsername: getGitUsernameMock,
|
||||
getDefaultBaseRef: getDefaultBaseRefMock,
|
||||
|
|
|
|||
|
|
@ -180,6 +180,10 @@ describe('registerWorktreeHandlers', () => {
|
|||
getDefaultBaseRefMock.mockReturnValue('origin/main')
|
||||
getBranchConflictKindMock.mockResolvedValue(null)
|
||||
getPRForBranchMock.mockResolvedValue(null)
|
||||
// Why: createLocalWorktree now fires `git fetch` in the background via
|
||||
// gitExecFileAsync. The default mock must return a resolved promise so
|
||||
// the fire-and-forget `.catch()` chain doesn't trip on undefined.
|
||||
gitExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' })
|
||||
getEffectiveHooksMock.mockReturnValue(null)
|
||||
shouldRunSetupForCreateMock.mockReturnValue(false)
|
||||
createSetupRunnerScriptMock.mockReturnValue({
|
||||
|
|
@ -312,12 +316,19 @@ describe('registerWorktreeHandlers', () => {
|
|||
expect(listWorktreesMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('auto-suffixes the branch name when the first choice already belongs to a PR', async () => {
|
||||
// Why: reusing a historical PR head would make a fresh worktree inherit
|
||||
// that old PR, so the loop suffixes past the name until it finds one that
|
||||
// is not associated with any PR.
|
||||
it('skips past a suffix that already belongs to a PR after an initial branch conflict', async () => {
|
||||
// Why: `gh pr list` is network-bound and previously fired on every single
|
||||
// create, adding 1–3s to the happy path. We now only probe PR conflicts
|
||||
// from suffix=2 onward — once a local/remote branch collision has already
|
||||
// forced us past the first candidate and uniqueness matters enough to
|
||||
// justify the GitHub round-trip. This test covers that delayed path:
|
||||
// suffix=1 is a branch conflict, suffix=2 is owned by an old PR, so the
|
||||
// loop lands on suffix=3.
|
||||
getBranchConflictKindMock.mockImplementation(async (_repoPath: string, branch: string) =>
|
||||
branch === 'improve-dashboard' ? 'remote' : null
|
||||
)
|
||||
getPRForBranchMock.mockImplementation(async (_repoPath: string, branch: string) =>
|
||||
branch === 'improve-dashboard'
|
||||
branch === 'improve-dashboard-2'
|
||||
? {
|
||||
number: 3127,
|
||||
title: 'Existing PR',
|
||||
|
|
@ -331,9 +342,9 @@ describe('registerWorktreeHandlers', () => {
|
|||
)
|
||||
listWorktreesMock.mockResolvedValue([
|
||||
{
|
||||
path: '/workspace/improve-dashboard-2',
|
||||
path: '/workspace/improve-dashboard-3',
|
||||
head: 'abc123',
|
||||
branch: 'improve-dashboard-2',
|
||||
branch: 'improve-dashboard-3',
|
||||
isBare: false,
|
||||
isMainWorktree: false
|
||||
}
|
||||
|
|
@ -346,19 +357,42 @@ describe('registerWorktreeHandlers', () => {
|
|||
|
||||
expect(addWorktreeMock).toHaveBeenCalledWith(
|
||||
'/workspace/repo',
|
||||
'/workspace/improve-dashboard-2',
|
||||
'improve-dashboard-2',
|
||||
'/workspace/improve-dashboard-3',
|
||||
'improve-dashboard-3',
|
||||
'origin/main',
|
||||
false
|
||||
)
|
||||
expect(result).toEqual({
|
||||
worktree: expect.objectContaining({
|
||||
path: '/workspace/improve-dashboard-2',
|
||||
branch: 'improve-dashboard-2'
|
||||
path: '/workspace/improve-dashboard-3',
|
||||
branch: 'improve-dashboard-3'
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
it('does not call `gh pr list` on the happy path (no branch conflict)', async () => {
|
||||
// Why: guards the speed optimization. If a future refactor accidentally
|
||||
// reintroduces the PR probe on the first iteration, the happy path will
|
||||
// silently regain a 1–3s GitHub round-trip per click; this test fails
|
||||
// loudly instead.
|
||||
listWorktreesMock.mockResolvedValue([
|
||||
{
|
||||
path: '/workspace/improve-dashboard',
|
||||
head: 'abc123',
|
||||
branch: 'improve-dashboard',
|
||||
isBare: false,
|
||||
isMainWorktree: false
|
||||
}
|
||||
])
|
||||
|
||||
await handlers['worktrees:create'](null, {
|
||||
repoId: 'repo-1',
|
||||
name: 'improve-dashboard'
|
||||
})
|
||||
|
||||
expect(getPRForBranchMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
const createdWorktreeList = [
|
||||
{
|
||||
path: '/workspace/improve-dashboard',
|
||||
|
|
|
|||
Loading…
Reference in a new issue