From f0bfc945b403b2d1520e909b2cc653b36dc3e1c2 Mon Sep 17 00:00:00 2001 From: Kien Le <122910950+kiendle@users.noreply.github.com> Date: Tue, 8 Sep 2026 01:19:52 -0400 Subject: [PATCH] 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> --- .../project-groups/project-group-mutations.ts | 21 ++- .../repos-project-group-create-race.test.ts | 170 +++++++++++++++++ .../project-group-creation-visibility.spec.ts | 173 ++++++++++++++++++ 3 files changed, 360 insertions(+), 4 deletions(-) create mode 100644 src/renderer/src/store/slices/repos-project-group-create-race.test.ts create mode 100644 tests/e2e/project-group-creation-visibility.spec.ts diff --git a/src/renderer/src/store/project-groups/project-group-mutations.ts b/src/renderer/src/store/project-groups/project-group-mutations.ts index e822fd4bfb3..c0f64fed827 100644 --- a/src/renderer/src/store/project-groups/project-group-mutations.ts +++ b/src/renderer/src/store/project-groups/project-group-mutations.ts @@ -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) diff --git a/src/renderer/src/store/slices/repos-project-group-create-race.test.ts b/src/renderer/src/store/slices/repos-project-group-create-race.test.ts new file mode 100644 index 00000000000..02025001385 --- /dev/null +++ b/src/renderer/src/store/slices/repos-project-group-create-race.test.ts @@ -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() + const createStarted = Promise.withResolvers() + 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' } + ]) +}) diff --git a/tests/e2e/project-group-creation-visibility.spec.ts b/tests/e2e/project-group-creation-visibility.spec.ts new file mode 100644 index 00000000000..d659734570a --- /dev/null +++ b/tests/e2e/project-group-creation-visibility.spec.ts @@ -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() + 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') }) + }) +}