mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 00:02:31 +00:00
fix: avoid duplicate repository groups during catalog refresh (#19170)
* fix: keep grouped repositories visible after creation race * test: strengthen project group creation race verification --------- Co-authored-by: Kien Le <122910950+kien-ship-it@users.noreply.github.com> Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com>
This commit is contained in:
Kien Le
Kien Le
Neil
@@ -5,6 +5,7 @@ import type { Repo } from '../../../../shared/repo-types'
|
||||
import { selectProjectGroupRemovalTargets } from '../slices/project-group-removal-targets'
|
||||
import {
|
||||
catalogOwnsHost,
|
||||
getProjectGroupHostId,
|
||||
projectGroupMatchesOwnerHost,
|
||||
resolveProjectGroupOwnerHostId,
|
||||
settingsForProjectGroupOwner
|
||||
@@ -48,10 +49,22 @@ export function createProjectGroupMutationActions(
|
||||
)
|
||||
).group
|
||||
const ownedGroup = projectGroupWithFetchedOwner(group, target)
|
||||
set((s) => ({
|
||||
projectGroups: [...s.projectGroups, ownedGroup],
|
||||
folderWorkspacePathStatuses: {}
|
||||
}))
|
||||
const ownerHostId = getProjectGroupHostId(ownedGroup)
|
||||
set((s) => {
|
||||
// An overlapping catalog refresh may have already inserted a newer copy.
|
||||
if (
|
||||
s.projectGroups.some(
|
||||
(existing) =>
|
||||
existing.id === ownedGroup.id && getProjectGroupHostId(existing) === ownerHostId
|
||||
)
|
||||
) {
|
||||
return s
|
||||
}
|
||||
return {
|
||||
projectGroups: [...s.projectGroups, ownedGroup],
|
||||
folderWorkspacePathStatuses: {}
|
||||
}
|
||||
})
|
||||
return ownedGroup
|
||||
} catch (err) {
|
||||
console.error('Failed to create project group:', err)
|
||||
|
||||
@@ -0,0 +1,170 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { getDefaultSettings } from '../../../../shared/constants'
|
||||
import type { ProjectGroup } from '../../../../shared/project-group-types'
|
||||
import { clearRuntimeCompatibilityCacheForTests } from '../../runtime/runtime-rpc-client'
|
||||
import {
|
||||
createCompatibleRuntimeStatusResponseIfNeeded,
|
||||
type RuntimeEnvironmentCallRequest
|
||||
} from '../../runtime/runtime-compatibility-test-fixture'
|
||||
import { createTestStore } from './store-test-helpers'
|
||||
|
||||
const projectGroup: ProjectGroup = {
|
||||
id: 'group-1',
|
||||
name: 'Platform',
|
||||
parentPath: null,
|
||||
parentGroupId: null,
|
||||
createdFrom: 'manual',
|
||||
tabOrder: 0,
|
||||
isCollapsed: false,
|
||||
color: null,
|
||||
createdAt: 1,
|
||||
updatedAt: 1
|
||||
}
|
||||
const refreshedGroup = { ...projectGroup, name: 'Renamed after creation', updatedAt: 2 }
|
||||
const otherHostGroup = { ...projectGroup, executionHostId: 'runtime:other' }
|
||||
|
||||
beforeEach(() => {
|
||||
clearRuntimeCompatibilityCacheForTests()
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.unstubAllGlobals()
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
|
||||
function setup(runtimeEnvironmentId: string | null) {
|
||||
const created = Promise.withResolvers<ProjectGroup>()
|
||||
const createStarted = Promise.withResolvers<void>()
|
||||
const create = vi.fn(() => {
|
||||
createStarted.resolve()
|
||||
return created.promise
|
||||
})
|
||||
const list = vi.fn(async () => [refreshedGroup])
|
||||
vi.stubGlobal('window', {
|
||||
api: {
|
||||
projectGroups: { create, list },
|
||||
runtimeEnvironments: {
|
||||
call: async (request: RuntimeEnvironmentCallRequest) => {
|
||||
const compatibility = createCompatibleRuntimeStatusResponseIfNeeded(request)
|
||||
if (compatibility) {
|
||||
return compatibility
|
||||
}
|
||||
expect(request).toMatchObject({ selector: runtimeEnvironmentId })
|
||||
switch (request.method) {
|
||||
case 'projectGroup.create':
|
||||
return { id: 'create', ok: true, result: { group: await create() } }
|
||||
case 'projectGroup.list':
|
||||
return { id: 'list', ok: true, result: { groups: await list() } }
|
||||
default:
|
||||
throw new Error(`Unexpected RPC: ${request.method}`)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
const store = createTestStore()
|
||||
store.setState({
|
||||
settings: { ...getDefaultSettings('/test'), activeRuntimeEnvironmentId: runtimeEnvironmentId },
|
||||
projectGroups: [otherHostGroup]
|
||||
})
|
||||
const ownerHostId = runtimeEnvironmentId ? `runtime:${runtimeEnvironmentId}` : 'local'
|
||||
return { store, created, createStarted, ownerHostId }
|
||||
}
|
||||
|
||||
describe.each([null, 'env-1'])('project group creation on host %s', (runtimeEnvironmentId) => {
|
||||
it('keeps the refreshed row without notifying subscribers when refresh finishes first', async () => {
|
||||
const { store, created, createStarted, ownerHostId } = setup(runtimeEnvironmentId)
|
||||
const pendingCreate = store.getState().createProjectGroup('Platform')
|
||||
await createStarted.promise
|
||||
await store.getState().fetchProjectGroups()
|
||||
const refreshedState = store.getState()
|
||||
const listener = vi.fn()
|
||||
const unsubscribe = store.subscribe(listener)
|
||||
try {
|
||||
created.resolve(projectGroup)
|
||||
await expect(pendingCreate).resolves.toEqual({
|
||||
...projectGroup,
|
||||
executionHostId: ownerHostId
|
||||
})
|
||||
expect(store.getState()).toBe(refreshedState)
|
||||
expect(listener).not.toHaveBeenCalled()
|
||||
expect(store.getState().projectGroups).toEqual([
|
||||
otherHostGroup,
|
||||
{ ...refreshedGroup, executionHostId: ownerHostId }
|
||||
])
|
||||
} finally {
|
||||
unsubscribe()
|
||||
}
|
||||
})
|
||||
|
||||
it('inserts beside another host with the same ID, then accepts the later refresh', async () => {
|
||||
const { store, created, ownerHostId } = setup(runtimeEnvironmentId)
|
||||
const pendingCreate = store.getState().createProjectGroup('Platform')
|
||||
created.resolve(projectGroup)
|
||||
await pendingCreate
|
||||
expect(store.getState().projectGroups).toEqual([
|
||||
otherHostGroup,
|
||||
{ ...projectGroup, executionHostId: ownerHostId }
|
||||
])
|
||||
await store.getState().fetchProjectGroups()
|
||||
expect(store.getState().projectGroups).toEqual([
|
||||
otherHostGroup,
|
||||
{ ...refreshedGroup, executionHostId: ownerHostId }
|
||||
])
|
||||
const groups = store.getState().projectGroups
|
||||
await store.getState().fetchProjectGroups()
|
||||
expect(store.getState().projectGroups).toBe(groups)
|
||||
})
|
||||
|
||||
it('keeps the original owner when the focused host changes during creation', async () => {
|
||||
const { store, created, createStarted, ownerHostId } = setup(runtimeEnvironmentId)
|
||||
const pendingCreate = store.getState().createProjectGroup('Platform')
|
||||
await createStarted.promise
|
||||
store.setState({
|
||||
settings: { ...getDefaultSettings('/test'), activeRuntimeEnvironmentId: 'other' }
|
||||
})
|
||||
await store.getState().fetchProjectGroups({ runtimeEnvironmentId })
|
||||
created.resolve(projectGroup)
|
||||
await pendingCreate
|
||||
expect(store.getState().projectGroups).toEqual([
|
||||
otherHostGroup,
|
||||
{ ...refreshedGroup, executionHostId: ownerHostId }
|
||||
])
|
||||
})
|
||||
|
||||
it('does not roll back a successful refresh if the create response fails', async () => {
|
||||
const { store, created, createStarted } = setup(runtimeEnvironmentId)
|
||||
vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
const pendingCreate = store.getState().createProjectGroup('Platform')
|
||||
await createStarted.promise
|
||||
await store.getState().fetchProjectGroups()
|
||||
const refreshedState = store.getState()
|
||||
created.reject(new Error('Create response lost'))
|
||||
await expect(pendingCreate).resolves.toBeNull()
|
||||
expect(store.getState()).toBe(refreshedState)
|
||||
})
|
||||
})
|
||||
|
||||
it('recognizes an unstamped local group without conflating an SSH catalog row', async () => {
|
||||
const { store, created } = setup(null)
|
||||
const sshGroup = { ...projectGroup, connectionId: 'server' }
|
||||
store.setState({ projectGroups: [sshGroup, refreshedGroup] })
|
||||
const state = store.getState()
|
||||
const pendingCreate = state.createProjectGroup('Platform')
|
||||
created.resolve(projectGroup)
|
||||
await pendingCreate
|
||||
expect(store.getState()).toBe(state)
|
||||
})
|
||||
|
||||
it('does not suppress a local group whose ID matches a direct SSH group', async () => {
|
||||
const { store, created } = setup(null)
|
||||
const sshGroup = { ...projectGroup, connectionId: 'server' }
|
||||
store.setState({ projectGroups: [sshGroup] })
|
||||
const pendingCreate = store.getState().createProjectGroup('Platform')
|
||||
created.resolve(projectGroup)
|
||||
await pendingCreate
|
||||
expect(store.getState().projectGroups).toEqual([
|
||||
sshGroup,
|
||||
{ ...projectGroup, executionHostId: 'local' }
|
||||
])
|
||||
})
|
||||
@@ -0,0 +1,173 @@
|
||||
import { mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from 'node:fs'
|
||||
import os from 'node:os'
|
||||
import path from 'node:path'
|
||||
import { test, expect } from './helpers/orca-app'
|
||||
import { waitForSessionReady } from './helpers/store'
|
||||
import { runProcess } from '../../src/shared/child-process/run-process'
|
||||
|
||||
test.use({ seedTestRepo: false })
|
||||
|
||||
for (const delayCreateResponse of [false, true]) {
|
||||
test(`created groups survive sidebar expansion (${delayCreateResponse ? 'refresh first' : 'ordinary timing'})`, async ({
|
||||
orcaPage,
|
||||
electronApp,
|
||||
registerPostElectronShutdownCleanup
|
||||
}, testInfo) => {
|
||||
await waitForSessionReady(orcaPage)
|
||||
const root = realpathSync(mkdtempSync(path.join(os.tmpdir(), 'orca-group-visibility-')))
|
||||
registerPostElectronShutdownCleanup(async () => {
|
||||
rmSync(root, { recursive: true, force: true })
|
||||
})
|
||||
const paths = Array.from({ length: 30 }, (_, index) =>
|
||||
path.join(root, `repo-${String(index).padStart(2, '0')}`)
|
||||
)
|
||||
for (const repoPath of paths) {
|
||||
mkdirSync(repoPath)
|
||||
writeFileSync(path.join(repoPath, 'seed.txt'), 'seed\n')
|
||||
for (const args of [
|
||||
['init'],
|
||||
['add', '.'],
|
||||
[
|
||||
'-c',
|
||||
'user.name=Test',
|
||||
'-c',
|
||||
'user.email=test@example.com',
|
||||
'-c',
|
||||
'commit.gpgsign=false',
|
||||
'commit',
|
||||
'-m',
|
||||
'seed'
|
||||
]
|
||||
]) {
|
||||
const result = await runProcess({ program: 'git', args, cwd: repoPath, timeoutMs: 10_000 })
|
||||
expect(result.code, result.stderr).toBe(0)
|
||||
}
|
||||
}
|
||||
const repoIds = await orcaPage.evaluate(async (paths) => {
|
||||
const store = window.__store!
|
||||
for (const repoPath of paths) {
|
||||
await window.api.repos.add({ path: repoPath })
|
||||
}
|
||||
await store.getState().awaitLocalRepoCatalogSettlement()
|
||||
const repos = store.getState().repos.filter((repo) => paths.includes(repo.path))
|
||||
for (const repo of repos) {
|
||||
await store.getState().fetchWorktrees(repo.id)
|
||||
}
|
||||
store.getState().setGroupBy('repo')
|
||||
store.getState().setProjectOrderBy('manual')
|
||||
return repos.map((repo) => repo.id)
|
||||
}, paths)
|
||||
expect(repoIds).toHaveLength(paths.length)
|
||||
|
||||
// Force the adverse ordering separately from the ordinary IPC path.
|
||||
if (delayCreateResponse) {
|
||||
await electronApp.evaluate(({ ipcMain }) => {
|
||||
if (!('_invokeHandlers' in ipcMain) || !(ipcMain._invokeHandlers instanceof Map)) {
|
||||
throw new Error('Electron invoke handlers unavailable')
|
||||
}
|
||||
const create = ipcMain._invokeHandlers.get('projectGroups:create')
|
||||
if (typeof create !== 'function') {
|
||||
throw new Error('Group create handler unavailable')
|
||||
}
|
||||
const gate = Promise.withResolvers<void>()
|
||||
Reflect.set(globalThis, '__releaseGroupCreateResponse', gate.resolve)
|
||||
ipcMain.removeHandler('projectGroups:create')
|
||||
ipcMain.handle('projectGroups:create', async (...args) => {
|
||||
ipcMain.removeHandler('projectGroups:create')
|
||||
ipcMain.handle('projectGroups:create', create)
|
||||
const group = await create(...args)
|
||||
await gate.promise
|
||||
return group
|
||||
})
|
||||
})
|
||||
}
|
||||
const creation = orcaPage.evaluate(() =>
|
||||
window.__store!.getState().createProjectGroup('Crowded group')
|
||||
)
|
||||
if (delayCreateResponse) {
|
||||
try {
|
||||
await expect
|
||||
.poll(() =>
|
||||
orcaPage.evaluate(() =>
|
||||
window
|
||||
.__store!.getState()
|
||||
.projectGroups.some((group) => group.name === 'Crowded group')
|
||||
)
|
||||
)
|
||||
.toBe(true)
|
||||
} finally {
|
||||
await electronApp.evaluate(() => {
|
||||
const release = Reflect.get(globalThis, '__releaseGroupCreateResponse')
|
||||
if (typeof release !== 'function') {
|
||||
throw new Error('Group create response gate unavailable')
|
||||
}
|
||||
release()
|
||||
Reflect.deleteProperty(globalThis, '__releaseGroupCreateResponse')
|
||||
})
|
||||
}
|
||||
}
|
||||
const createdGroup = await creation
|
||||
if (!createdGroup) {
|
||||
throw new Error('Group creation failed')
|
||||
}
|
||||
await orcaPage.evaluate(
|
||||
async ({ repoIds, groupId }) => {
|
||||
const store = window.__store!
|
||||
for (const repoId of repoIds.slice(0, 2)) {
|
||||
await store.getState().moveProjectToGroup(repoId, groupId)
|
||||
}
|
||||
const collapsedGroups = store
|
||||
.getState()
|
||||
.projectHostSetups.map((setup) => `project:${setup.projectId}`)
|
||||
await window.api.ui.set({ groupBy: 'repo', collapsedGroups })
|
||||
store.setState({ collapsedGroups: new Set(collapsedGroups) })
|
||||
},
|
||||
{ repoIds, groupId: createdGroup.id }
|
||||
)
|
||||
|
||||
const scroller = orcaPage.locator('[data-worktree-sidebar]')
|
||||
const group = scroller.locator(`[data-project-group-header-id="${createdGroup.id}"]`)
|
||||
const groupedRepos = repoIds
|
||||
.slice(0, 2)
|
||||
.map((id) => scroller.locator(`[data-repo-header-id="${id}"]`))
|
||||
for (const repo of groupedRepos) {
|
||||
await expect(repo).toBeVisible()
|
||||
}
|
||||
await orcaPage.screenshot({ path: testInfo.outputPath('before-expansion.png') })
|
||||
for (const repoId of repoIds.slice(2, 12)) {
|
||||
const repo = scroller.locator(`[data-repo-header-id="${repoId}"]`)
|
||||
await expect
|
||||
.poll(async () => {
|
||||
if (await repo.count()) {
|
||||
return true
|
||||
}
|
||||
await scroller.evaluate((element) => {
|
||||
element.scrollTop += element.clientHeight / 2
|
||||
})
|
||||
return false
|
||||
})
|
||||
.toBe(true)
|
||||
await repo.scrollIntoViewIfNeeded()
|
||||
await expect(repo).toHaveAttribute('aria-expanded', 'false')
|
||||
await repo.click()
|
||||
await scroller.evaluate((element) => {
|
||||
element.scrollTop = 0
|
||||
})
|
||||
for (const groupedRepo of groupedRepos) {
|
||||
await expect(groupedRepo).toBeVisible()
|
||||
}
|
||||
}
|
||||
await expect(group).toHaveCount(1)
|
||||
await orcaPage.evaluate(() => window.__store!.getState().fetchProjectGroups())
|
||||
await expect(group).toHaveCount(1)
|
||||
await group.click()
|
||||
for (const repo of groupedRepos) {
|
||||
await expect(repo).toHaveCount(0)
|
||||
}
|
||||
await group.click()
|
||||
for (const repo of groupedRepos) {
|
||||
await expect(repo).toBeVisible()
|
||||
}
|
||||
await orcaPage.screenshot({ path: testInfo.outputPath('after-expansion.png') })
|
||||
})
|
||||
}
|
||||
Reference in New Issue
Block a user