mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 08:03:28 +00:00
fix(worktree): collapse duplicate "Local Mac" run targets in the host picker (#10472)
* fix(worktree): collapse duplicate "Local Mac" run targets in the host picker A linked worktree added as its own project projects a second ready host setup on the same project+host, so the run-target picker rendered N identical "Local Mac" rows differing only by path. Only the first was reachable — resolveWorkspaceCreationTarget takes the first project+host match — so the extras pointed at paths that may no longer exist. - Dedupe ready setup options by host in the picker (display fix for profiles that already hold duplicates). - Canonicalize a stale draft's setup id to the setup the picker shows, so the displayed path is the path the workspace is created in. - Reject a linked worktree at repos:add when its main checkout is already tracked, preventing new duplicates. * fix(worktree): only dedupe a linked worktree against a git main checkout Review follow-up: the repos:add guard matched any tracked repo on the main checkout path, including a folder-kind record. A folder repo does not project onto the same project as the git worktree, so matching it would suppress a legitimate add without deduping anything.
This commit is contained in:
@@ -11,7 +11,12 @@ import {
|
||||
import { tmpdir } from 'node:os'
|
||||
import * as path from 'node:path'
|
||||
import { afterEach, beforeEach, describe, expect, it } from 'vitest'
|
||||
import { getGitRepoRoot, isGitRepo, normalizeGitRepoRootForInputPath } from './repo'
|
||||
import {
|
||||
getGitRepoRoot,
|
||||
getLinkedWorktreeMainRepoRoot,
|
||||
isGitRepo,
|
||||
normalizeGitRepoRootForInputPath
|
||||
} from './repo'
|
||||
|
||||
function git(cwd: string, args: string[]): string {
|
||||
return execFileSync('git', args, { cwd, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] })
|
||||
@@ -329,6 +334,85 @@ describe('isGitRepo', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('getLinkedWorktreeMainRepoRoot', () => {
|
||||
let tmpDir: string
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = mkdtempSync(path.join(tmpdir(), 'orca-linked-worktree-'))
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
rmSync(tmpDir, { recursive: true, force: true })
|
||||
})
|
||||
|
||||
function initRepoWithCommit(repoRoot: string): void {
|
||||
mkdirSync(repoRoot, { recursive: true })
|
||||
git(repoRoot, ['init', '--quiet'])
|
||||
git(repoRoot, ['config', 'user.email', 'test@orca.test'])
|
||||
git(repoRoot, ['config', 'user.name', 'Orca Test'])
|
||||
writeFileSync(path.join(repoRoot, 'README.md'), 'seed\n')
|
||||
git(repoRoot, ['add', 'README.md'])
|
||||
git(repoRoot, ['commit', '--quiet', '-m', 'seed'])
|
||||
}
|
||||
|
||||
it('resolves a linked worktree back to its main checkout', () => {
|
||||
const repoRoot = path.join(tmpDir, 'repo')
|
||||
initRepoWithCommit(repoRoot)
|
||||
const linked = path.join(tmpDir, 'linked')
|
||||
git(repoRoot, ['worktree', 'add', '--quiet', '-b', 'feature', linked])
|
||||
|
||||
const expectedMainRoot = git(repoRoot, ['rev-parse', '--show-toplevel'])
|
||||
.trim()
|
||||
.replace(/\\/g, '/')
|
||||
expect(getLinkedWorktreeMainRepoRoot(linked)).toBe(expectedMainRoot)
|
||||
})
|
||||
|
||||
it('returns null for the main checkout itself', () => {
|
||||
const repoRoot = path.join(tmpDir, 'repo')
|
||||
initRepoWithCommit(repoRoot)
|
||||
|
||||
expect(getLinkedWorktreeMainRepoRoot(repoRoot)).toBeNull()
|
||||
})
|
||||
|
||||
it('returns null for a nested directory inside the main checkout', () => {
|
||||
const repoRoot = path.join(tmpDir, 'repo')
|
||||
initRepoWithCommit(repoRoot)
|
||||
const nested = path.join(repoRoot, 'packages', 'web')
|
||||
mkdirSync(nested, { recursive: true })
|
||||
|
||||
expect(getLinkedWorktreeMainRepoRoot(nested)).toBeNull()
|
||||
})
|
||||
|
||||
it('returns null for a bare repository', () => {
|
||||
const bareRepo = path.join(tmpDir, 'bare.git')
|
||||
git(tmpDir, ['init', '--bare', '--quiet', bareRepo])
|
||||
|
||||
expect(getLinkedWorktreeMainRepoRoot(bareRepo)).toBeNull()
|
||||
})
|
||||
|
||||
it('returns null for a non-repository directory', () => {
|
||||
const plain = path.join(tmpDir, 'plain')
|
||||
mkdirSync(plain)
|
||||
|
||||
expect(getLinkedWorktreeMainRepoRoot(plain)).toBeNull()
|
||||
})
|
||||
|
||||
it('returns null for a missing path', () => {
|
||||
expect(getLinkedWorktreeMainRepoRoot(path.join(tmpDir, 'does-not-exist'))).toBeNull()
|
||||
})
|
||||
|
||||
it('returns null when git cannot be run rather than guessing a main checkout', () => {
|
||||
const repoRoot = path.join(tmpDir, 'repo')
|
||||
initRepoWithCommit(repoRoot)
|
||||
const linked = path.join(tmpDir, 'linked')
|
||||
git(repoRoot, ['worktree', 'add', '--quiet', '-b', 'feature', linked])
|
||||
|
||||
withGitUnavailable(() => {
|
||||
expect(getLinkedWorktreeMainRepoRoot(linked)).toBeNull()
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
/**
|
||||
* Run `fn` with `git` removed from PATH so the in-process git probe fails the
|
||||
* same way a transient spawn failure would, exercising the `.git`-marker
|
||||
|
||||
@@ -155,6 +155,52 @@ export function getGitRepoRoot(path: string): string {
|
||||
return path
|
||||
}
|
||||
|
||||
function canonicalizeGitDirPath(path: string): string {
|
||||
return resolveRealPathSync(path) ?? path
|
||||
}
|
||||
|
||||
/**
|
||||
* Main-checkout path when `path` is a *linked* worktree, else null (main worktree, bare repo,
|
||||
* non-repo, or any git failure). A linked worktree's `--git-dir` is `<common>/worktrees/<name>`
|
||||
* while the main worktree's equals `--git-common-dir`; comparing the two from one invocation is
|
||||
* git's own canonical test and avoids symlink-canonicalization mismatches. Baseline-safe: both
|
||||
* flags long predate Git 2.25, and a relative answer resolves against `path` as old Git reports it.
|
||||
*/
|
||||
export function getLinkedWorktreeMainRepoRoot(path: string): string | null {
|
||||
try {
|
||||
if (!existsSync(path) || !statSync(path).isDirectory()) {
|
||||
return null
|
||||
}
|
||||
if (gitExecFileSync(['rev-parse', '--is-inside-work-tree'], { cwd: path }).trim() !== 'true') {
|
||||
return null
|
||||
}
|
||||
const [gitDir, commonDir] = gitExecFileSync(['rev-parse', '--git-dir', '--git-common-dir'], {
|
||||
cwd: path
|
||||
})
|
||||
.split('\n')
|
||||
.map((line) => line.trim())
|
||||
if (!gitDir || !commonDir) {
|
||||
return null
|
||||
}
|
||||
// Why realpath both: git answers one flag absolutely (already symlink-resolved) and the other
|
||||
// relative to cwd, so a repo under a symlinked root (macOS /var -> /private/var) compares
|
||||
// unequal on raw strings and a main checkout gets misread as a linked worktree.
|
||||
const absoluteCommonDir = canonicalizeGitDirPath(resolve(path, commonDir))
|
||||
if (canonicalizeGitDirPath(resolve(path, gitDir)) === absoluteCommonDir) {
|
||||
return null
|
||||
}
|
||||
// A bare/separate git dir has no adjacent working checkout to point at.
|
||||
if (basename(absoluteCommonDir) !== '.git') {
|
||||
return null
|
||||
}
|
||||
// Re-resolve through getGitRepoRoot so the returned path matches the canonical form
|
||||
// add-project stores for the main checkout (symlinks resolved the way git reports them).
|
||||
return getGitRepoRoot(dirname(absoluteCommonDir))
|
||||
} catch {
|
||||
return null
|
||||
}
|
||||
}
|
||||
|
||||
export function normalizeGitRepoRootForInputPath(inputPath: string, rootPath: string): string {
|
||||
const inputWsl = parseWslUncPath(inputPath)
|
||||
if (inputWsl && rootPath.startsWith('/')) {
|
||||
|
||||
@@ -0,0 +1,193 @@
|
||||
/**
|
||||
* Regression tests for repos:add + git worktrees.
|
||||
*
|
||||
* A linked worktree reports itself as its own `--show-toplevel`, so the path-based dedupe in
|
||||
* addLocalRepoFromPath cannot see that it belongs to an already-tracked repo. Adding it anyway
|
||||
* produced a second ready ProjectHostSetup on the same project and host — a duplicate "Local Mac"
|
||||
* run-target row pointing at a transient worktree path.
|
||||
*/
|
||||
|
||||
import { describe, expect, it, vi, beforeEach } from 'vitest'
|
||||
import type { Repo } from '../../shared/types'
|
||||
|
||||
const {
|
||||
handleMock,
|
||||
removeHandlerMock,
|
||||
mockStore,
|
||||
isGitRepoMock,
|
||||
getGitRepoRootMock,
|
||||
getLinkedWorktreeMainRepoRootMock,
|
||||
invalidateAuthorizedRootsCacheMock,
|
||||
prepareLocalWorktreeRootForRepoMock,
|
||||
detectRepoIconAndUpstreamMock
|
||||
} = vi.hoisted(() => ({
|
||||
handleMock: vi.fn(),
|
||||
removeHandlerMock: vi.fn(),
|
||||
mockStore: {
|
||||
getRepos: vi.fn().mockReturnValue([]),
|
||||
addRepo: vi.fn(),
|
||||
removeProject: vi.fn(),
|
||||
getRepo: vi.fn(),
|
||||
updateRepo: vi.fn()
|
||||
},
|
||||
isGitRepoMock: vi.fn().mockReturnValue(true),
|
||||
getGitRepoRootMock: vi.fn(),
|
||||
getLinkedWorktreeMainRepoRootMock: vi.fn(),
|
||||
invalidateAuthorizedRootsCacheMock: vi.fn(),
|
||||
prepareLocalWorktreeRootForRepoMock: vi.fn(),
|
||||
detectRepoIconAndUpstreamMock: vi.fn()
|
||||
}))
|
||||
|
||||
vi.mock('electron', () => ({
|
||||
dialog: { showOpenDialog: vi.fn() },
|
||||
ipcMain: { handle: handleMock, removeHandler: removeHandlerMock }
|
||||
}))
|
||||
|
||||
vi.mock('../git/repo', () => ({
|
||||
isGitRepo: isGitRepoMock,
|
||||
getGitRepoRoot: getGitRepoRootMock,
|
||||
getLinkedWorktreeMainRepoRoot: getLinkedWorktreeMainRepoRootMock,
|
||||
getRepoName: vi.fn().mockImplementation((path: string) => path.split('/').pop()),
|
||||
getBaseRefDefault: vi.fn().mockResolvedValue('origin/main'),
|
||||
searchBaseRefs: vi.fn().mockResolvedValue([])
|
||||
}))
|
||||
|
||||
vi.mock('../repo-detection', () => ({
|
||||
detectRepoIconAndUpstream: detectRepoIconAndUpstreamMock
|
||||
}))
|
||||
|
||||
vi.mock('./filesystem-auth', () => ({
|
||||
invalidateAuthorizedRootsCache: invalidateAuthorizedRootsCacheMock
|
||||
}))
|
||||
|
||||
vi.mock('../worktree-root-preparation', () => ({
|
||||
prepareLocalWorktreeRootForRepo: prepareLocalWorktreeRootForRepoMock
|
||||
}))
|
||||
|
||||
vi.mock('../providers/ssh-git-dispatch', () => ({ getSshGitProvider: vi.fn() }))
|
||||
vi.mock('./ssh', () => ({ getActiveMultiplexer: vi.fn() }))
|
||||
|
||||
import { registerRepoHandlers } from './repos'
|
||||
|
||||
const MAIN_CHECKOUT = '/Users/dev/projects/orca'
|
||||
const LINKED_WORKTREE = '/Users/dev/orca/workspaces/orca/pr-3235'
|
||||
|
||||
type AddResult = { repo: Repo } | { error: string }
|
||||
|
||||
describe('repos:add with git worktrees', () => {
|
||||
const handlers = new Map<string, (event: unknown, args: unknown) => unknown>()
|
||||
const mockWindow = { isDestroyed: () => false, webContents: { send: vi.fn() } }
|
||||
|
||||
const trackedMainRepo = (): Repo =>
|
||||
({
|
||||
id: 'main-repo-id',
|
||||
path: MAIN_CHECKOUT,
|
||||
displayName: 'orca',
|
||||
badgeColor: '#ef4444',
|
||||
addedAt: 1,
|
||||
kind: 'git'
|
||||
}) as Repo
|
||||
|
||||
const callAdd = (args: { path: string; kind?: 'git' | 'folder' }): Promise<AddResult> => {
|
||||
const handler = handlers.get('repos:add')
|
||||
if (!handler) {
|
||||
throw new Error('repos:add handler was never registered')
|
||||
}
|
||||
return handler(null, args) as Promise<AddResult>
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
handlers.clear()
|
||||
handleMock.mockReset()
|
||||
handleMock.mockImplementation((channel: string, handler: (...a: unknown[]) => unknown) => {
|
||||
handlers.set(channel, handler as (event: unknown, args: unknown) => unknown)
|
||||
})
|
||||
removeHandlerMock.mockReset()
|
||||
mockStore.getRepos.mockReset().mockReturnValue([])
|
||||
mockStore.addRepo.mockReset()
|
||||
isGitRepoMock.mockReset().mockReturnValue(true)
|
||||
// A linked worktree is its own toplevel — this is exactly why path dedupe alone misses it.
|
||||
getGitRepoRootMock.mockReset().mockImplementation((path: string) => path)
|
||||
getLinkedWorktreeMainRepoRootMock.mockReset().mockReturnValue(null)
|
||||
detectRepoIconAndUpstreamMock.mockReset().mockResolvedValue({})
|
||||
invalidateAuthorizedRootsCacheMock.mockReset()
|
||||
prepareLocalWorktreeRootForRepoMock.mockReset().mockResolvedValue(undefined)
|
||||
|
||||
registerRepoHandlers(mockWindow as never, mockStore as never)
|
||||
})
|
||||
|
||||
it('returns the tracked main checkout instead of adding its linked worktree', async () => {
|
||||
mockStore.getRepos.mockReturnValue([trackedMainRepo()])
|
||||
getLinkedWorktreeMainRepoRootMock.mockReturnValue(MAIN_CHECKOUT)
|
||||
|
||||
const result = await callAdd({ path: LINKED_WORKTREE })
|
||||
|
||||
expect(result).toEqual({ repo: expect.objectContaining({ id: 'main-repo-id' }) })
|
||||
expect(mockStore.addRepo).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('still adds a linked worktree whose main checkout is not tracked', async () => {
|
||||
mockStore.getRepos.mockReturnValue([])
|
||||
getLinkedWorktreeMainRepoRootMock.mockReturnValue(MAIN_CHECKOUT)
|
||||
|
||||
const result = await callAdd({ path: LINKED_WORKTREE })
|
||||
|
||||
expect(mockStore.addRepo).toHaveBeenCalledTimes(1)
|
||||
expect(result).toEqual({ repo: expect.objectContaining({ path: LINKED_WORKTREE }) })
|
||||
})
|
||||
|
||||
it('adds a normal repo when git reports it is not a linked worktree', async () => {
|
||||
mockStore.getRepos.mockReturnValue([trackedMainRepo()])
|
||||
getLinkedWorktreeMainRepoRootMock.mockReturnValue(null)
|
||||
|
||||
const result = await callAdd({ path: '/Users/dev/projects/other' })
|
||||
|
||||
expect(mockStore.addRepo).toHaveBeenCalledTimes(1)
|
||||
expect(result).toEqual({ repo: expect.objectContaining({ path: '/Users/dev/projects/other' }) })
|
||||
})
|
||||
|
||||
it('does not consult worktree detection for folder projects', async () => {
|
||||
mockStore.getRepos.mockReturnValue([trackedMainRepo()])
|
||||
|
||||
await callAdd({ path: '/Users/dev/notes', kind: 'folder' })
|
||||
|
||||
expect(getLinkedWorktreeMainRepoRootMock).not.toHaveBeenCalled()
|
||||
expect(mockStore.addRepo).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('matches the tracked main checkout across path separator differences', async () => {
|
||||
mockStore.getRepos.mockReturnValue([
|
||||
{ ...trackedMainRepo(), path: 'C:\\Users\\dev\\projects\\orca' } as Repo
|
||||
])
|
||||
getLinkedWorktreeMainRepoRootMock.mockReturnValue('C:/Users/dev/projects/orca')
|
||||
|
||||
const result = await callAdd({ path: 'C:/Users/dev/worktrees/pr-3235' })
|
||||
|
||||
expect(result).toEqual({ repo: expect.objectContaining({ id: 'main-repo-id' }) })
|
||||
expect(mockStore.addRepo).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does not match a folder record sitting on the main-checkout path', async () => {
|
||||
mockStore.getRepos.mockReturnValue([
|
||||
{ ...trackedMainRepo(), id: 'folder-repo-id', kind: 'folder' } as Repo
|
||||
])
|
||||
getLinkedWorktreeMainRepoRootMock.mockReturnValue(MAIN_CHECKOUT)
|
||||
|
||||
const result = await callAdd({ path: LINKED_WORKTREE })
|
||||
|
||||
expect(mockStore.addRepo).toHaveBeenCalledTimes(1)
|
||||
expect(result).toEqual({ repo: expect.objectContaining({ path: LINKED_WORKTREE }) })
|
||||
})
|
||||
|
||||
it('does not match a tracked SSH repo that shares the local main-checkout path', async () => {
|
||||
mockStore.getRepos.mockReturnValue([
|
||||
{ ...trackedMainRepo(), id: 'ssh-repo-id', connectionId: 'builder' } as Repo
|
||||
])
|
||||
getLinkedWorktreeMainRepoRootMock.mockReturnValue(MAIN_CHECKOUT)
|
||||
|
||||
const result = await callAdd({ path: LINKED_WORKTREE })
|
||||
|
||||
expect(mockStore.addRepo).toHaveBeenCalledTimes(1)
|
||||
expect(result).toEqual({ repo: expect.objectContaining({ path: LINKED_WORKTREE }) })
|
||||
})
|
||||
})
|
||||
@@ -59,6 +59,7 @@ import { createNestedRepoImportTargetResolver } from '../project-groups/nested-r
|
||||
import {
|
||||
isGitRepo,
|
||||
getGitRepoRoot,
|
||||
getLinkedWorktreeMainRepoRoot,
|
||||
getRepoName,
|
||||
getBaseRefDefault,
|
||||
getRemoteCount,
|
||||
@@ -195,6 +196,29 @@ async function addLocalRepoFromPath(
|
||||
}
|
||||
}
|
||||
|
||||
// Why: a linked worktree reports itself as its own toplevel, so the path checks above can't see that
|
||||
// it belongs to an already-tracked repo. Adding it anyway yields a second "ready" host setup on the
|
||||
// same project and host — a duplicate run-target row that resolves to a transient worktree path.
|
||||
if (repoKind === 'git') {
|
||||
const mainRepoRoot = getLinkedWorktreeMainRepoRoot(resolvedPath)
|
||||
if (mainRepoRoot) {
|
||||
const mainRepoKey = normalizeRuntimePathForComparison(mainRepoRoot)
|
||||
// Why !isFolderRepo: only a git-kind main checkout projects onto the same project as its
|
||||
// worktree, so matching a folder record would suppress the add without deduping anything.
|
||||
const trackedMainRepo = store
|
||||
.getRepos()
|
||||
.find(
|
||||
(repo) =>
|
||||
!repo.connectionId &&
|
||||
!isFolderRepo(repo) &&
|
||||
normalizeRuntimePathForComparison(repo.path) === mainRepoKey
|
||||
)
|
||||
if (trackedMainRepo) {
|
||||
return { repo: trackedMainRepo, alreadyExisted: true }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
const detected = await detectRepoIconAndUpstream({ repoPath: resolvedPath, kind: repoKind })
|
||||
const repo: Repo = {
|
||||
id: randomUUID(),
|
||||
|
||||
Reference in New Issue
Block a user