diff --git a/src/renderer/src/components/github/PRAssigneesPanel.metadata.test.tsx b/src/renderer/src/components/github/PRAssigneesPanel.metadata.test.tsx new file mode 100644 index 00000000000..d0d285dea64 --- /dev/null +++ b/src/renderer/src/components/github/PRAssigneesPanel.metadata.test.tsx @@ -0,0 +1,98 @@ +// @vitest-environment happy-dom + +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { GitHubWorkItem } from '../../../../shared/github/work-item-types' +import { clearGitHubSlugMetadataCache } from '@/hooks/useGitHubSlugMetadata' +import { PRAssigneesPanel } from './PRAssigneesPanel' + +type PanelStoreState = { repos: [] } + +vi.mock('@/store', () => ({ + useAppStore: (select: (state: PanelStoreState) => unknown) => select({ repos: [] }) +})) +vi.mock('@/lib/github-source-runtime-context', () => ({ + getGitHubRepoRoutingTarget: () => ({ kind: 'local' }) +})) +vi.mock('@/hooks/useIssueMetadata', async () => { + const { useRepoAssignees } = await import('@/hooks/useGitHubRepoMetadata') + return { useRepoAssignees, useImmediateMutation: () => ({ isPending: () => false }) } +}) +vi.mock('@/i18n/i18n', () => ({ translate: (_key: string, fallback: string) => fallback })) + +const bySlug = vi.fn() +const byPath = vi.fn() +let root: Root + +async function render(repoId: string, url: string, repoPath: string | null): Promise { + const item: GitHubWorkItem = { + id: 'pr:42', + repoId, + type: 'pr', + number: 42, + title: 'Example', + state: 'open', + url, + labels: [], + updatedAt: '', + author: null + } + await act(async () => { + root.render( + {}} + /> + ) + }) +} + +describe('assignee panel metadata source selection', () => { + beforeEach(() => { + vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true) + clearGitHubSlugMetadataCache() + bySlug.mockReset().mockResolvedValue({ ok: true, users: [] }) + byPath.mockReset().mockResolvedValue([]) + Object.defineProperty(window, 'api', { + configurable: true, + value: { gh: { listAssignableUsersBySlug: bySlug, listAssignableUsers: byPath } } + }) + const container = document.createElement('div') + document.body.append(container) + root = createRoot(container) + }) + + afterEach(() => { + act(() => root.unmount()) + document.body.replaceChildren() + vi.unstubAllGlobals() + }) + + it('loads only the slug source even when a registered repo and path are available', async () => { + await render('registered-with-url', 'https://github.com/acme/orca/pull/42', '/repo') + + expect(bySlug).toHaveBeenCalledExactlyOnceWith({ + owner: 'acme', + repo: 'orca', + host: 'github.com' + }) + expect(byPath).not.toHaveBeenCalled() + }) + + it('retains path loading when the item has no repository slug', async () => { + await render('path-only', '', '/folder') + + expect(byPath).toHaveBeenCalledExactlyOnceWith({ repoId: 'path-only', repoPath: '/folder' }) + expect(bySlug).not.toHaveBeenCalled() + }) + + it('retains repo-id loading without a checkout path or slug', async () => { + await render('id-only', '', null) + + expect(byPath).toHaveBeenCalledExactlyOnceWith({ repoId: 'id-only', repoPath: '' }) + expect(bySlug).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/components/github/PRAssigneesPanel.tsx b/src/renderer/src/components/github/PRAssigneesPanel.tsx index eae876ed150..18a20f6682e 100644 --- a/src/renderer/src/components/github/PRAssigneesPanel.tsx +++ b/src/renderer/src/components/github/PRAssigneesPanel.tsx @@ -80,7 +80,12 @@ export function PRAssigneesPanel({ ownerTarget, projectOrigin?.host ?? assigneeSlug?.host ) - const repoAssigneesByPath = useRepoAssignees(repoPath, item.repoId, { target: ownerTarget }) + const usesSlug = Boolean(slugOwner && slugRepo) + const repoAssigneesByPath = useRepoAssignees( + usesSlug ? null : repoPath, + usesSlug ? null : item.repoId, + { target: ownerTarget } + ) const repoAssignees = slugOwner && slugRepo ? repoAssigneesBySlug : repoAssigneesByPath const canEditAssignees = Boolean(projectOrigin || repoPath) const assigneesByLogin = useMemo( diff --git a/src/renderer/src/hooks/useGitHubSlugMetadata.assignee-reuse.test.tsx b/src/renderer/src/hooks/useGitHubSlugMetadata.assignee-reuse.test.tsx new file mode 100644 index 00000000000..3863d765275 --- /dev/null +++ b/src/renderer/src/hooks/useGitHubSlugMetadata.assignee-reuse.test.tsx @@ -0,0 +1,158 @@ +// @vitest-environment happy-dom + +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { GitHubAssignableUser } from '../../../shared/github/pull-request-types' +import type { RuntimeClientTarget } from '../../../shared/runtime-client-target' +import { callRuntimeRpc } from '@/runtime/runtime-rpc-client' +import { clearGitHubSlugMetadataCache, useRepoAssigneesBySlug } from './useGitHubSlugMetadata' +import type { MetadataListState } from './useMetadataListRequest' + +vi.mock('@/runtime/runtime-rpc-client', () => ({ callRuntimeRpc: vi.fn() })) + +const listUsers = vi.fn() +const baseUser = { login: 'member', name: 'Member Name', avatarUrl: 'https://example.test/a' } +const results = new Map>() +let root: Root + +function Probe({ + id = 'row', + seeds, + repo = 'orca', + host, + target +}: { + id?: string + seeds?: string[] + repo?: string + host?: string + target?: RuntimeClientTarget +}): null { + results.set(id, useRepoAssigneesBySlug('stablyai', repo, seeds, target, host)) + return null +} + +async function render(element: React.ReactNode): Promise { + await act(async () => { + root.render(element) + }) +} + +function logins(id = 'row'): string[] | undefined { + return results.get(id)?.data.map((user) => user.login) +} + +describe('repository assignee reuse across row seeds', () => { + beforeEach(() => { + vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true) + clearGitHubSlugMetadataCache() + listUsers + .mockReset() + .mockImplementation(async ({ seedLogins = [] }: { seedLogins?: string[] }) => { + const extras = [...new Set(seedLogins)].filter((login) => login !== baseUser.login) + return { + ok: true, + users: [baseUser, ...extras.map((login) => ({ login, name: null, avatarUrl: '' }))] + } + }) + vi.mocked(callRuntimeRpc) + .mockReset() + .mockResolvedValue({ ok: true, users: [baseUser] }) + Object.defineProperty(window, 'api', { + configurable: true, + value: { gh: { listAssignableUsersBySlug: listUsers } } + }) + results.clear() + const container = document.createElement('div') + document.body.append(container) + root = createRoot(container) + }) + + afterEach(() => { + act(() => root.unmount()) + document.body.replaceChildren() + vi.restoreAllMocks() + vi.unstubAllGlobals() + }) + + it('loads one base list for concurrent rows without sharing their extra users', async () => { + await render( + <> + + + + ) + + expect(listUsers).toHaveBeenCalledExactlyOnceWith({ + owner: 'stablyai', + repo: 'orca', + host: 'github.com' + }) + expect(logins('a')).toEqual(['member', 'alpha', 'zeta']) + expect(logins('b')).toEqual(['member', 'beta']) + expect(results.get('a')?.data[0]).toEqual(baseUser) + expect(results.get('a')?.data[1]).toEqual({ login: 'alpha', name: null, avatarUrl: '' }) + }) + + it('updates seeds in place without a fetch or loading transition', async () => { + await render() + await render() + + expect(logins()).toEqual(['member', 'second']) + expect(results.get('row')).toMatchObject({ loading: false, error: null }) + expect(listUsers).toHaveBeenCalledTimes(1) + }) + + it('adds seeds to a successful empty list but not to loading or failed results', async () => { + let finish: (value: { ok: boolean; users: GitHubAssignableUser[] }) => void = () => {} + listUsers.mockReturnValueOnce(new Promise((resolve) => (finish = resolve))) + await render() + expect(results.get('row')).toMatchObject({ data: [], loading: true, error: null }) + await act(async () => finish({ ok: true, users: [] })) + expect(logins()).toEqual(['existing']) + + listUsers.mockResolvedValueOnce({ ok: false, error: { message: 'Forbidden' } }) + await render() + expect(results.get('row')).toMatchObject({ data: [], loading: false, error: 'Forbidden' }) + }) + + it('retains seed projection after expiry and fetches fresh metadata for a new reader', async () => { + const now = Date.now() + const time = vi.spyOn(Date, 'now').mockReturnValue(now) + await render() + time.mockReturnValue(now + 300_001) + await render() + expect(logins()).toEqual(['member', 'updated']) + expect(listUsers).toHaveBeenCalledTimes(1) + + await render() + expect(logins()).toEqual(['member', 'fresh']) + expect(listUsers).toHaveBeenCalledTimes(2) + }) + + it('isolates repository, host and remote runtime scopes and honors cache clearing', async () => { + await render() + await render() + await render() + await render() + await render() + + expect(listUsers).toHaveBeenCalledTimes(3) + expect(listUsers).toHaveBeenLastCalledWith({ + owner: 'stablyai', + repo: 'orca', + host: 'ghe.example.test' + }) + expect(callRuntimeRpc).toHaveBeenCalledTimes(2) + expect(callRuntimeRpc).toHaveBeenLastCalledWith( + { kind: 'environment', environmentId: 'remote-b' }, + 'github.project.listAssignableUsersBySlug', + { owner: 'stablyai', repo: 'orca', host: 'github.com' }, + { timeoutMs: 30_000 } + ) + clearGitHubSlugMetadataCache() + await render() + expect(listUsers).toHaveBeenCalledTimes(4) + }) +}) diff --git a/src/renderer/src/hooks/useGitHubSlugMetadata.test.tsx b/src/renderer/src/hooks/useGitHubSlugMetadata.test.tsx index 2b3e188d7cc..7ddd1fd22ca 100644 --- a/src/renderer/src/hooks/useGitHubSlugMetadata.test.tsx +++ b/src/renderer/src/hooks/useGitHubSlugMetadata.test.tsx @@ -109,8 +109,7 @@ describe('useGitHubSlugMetadata', () => { expect(apiMocks.listAssignableUsersBySlug).toHaveBeenCalledExactlyOnceWith({ owner: 'stablyai', repo: 'orca', - host: 'github.com', - seedLogins: ['jinwoo'] + host: 'github.com' }) expect(renders).toBeLessThanOrEqual(4) }) diff --git a/src/renderer/src/hooks/useGitHubSlugMetadata.ts b/src/renderer/src/hooks/useGitHubSlugMetadata.ts index 69668e7458d..dbd4ec7ca37 100644 --- a/src/renderer/src/hooks/useGitHubSlugMetadata.ts +++ b/src/renderer/src/hooks/useGitHubSlugMetadata.ts @@ -1,3 +1,4 @@ +import { useMemo } from 'react' import { callRuntimeRpc } from '@/runtime/runtime-rpc-client' import type { RuntimeClientTarget } from '../../../shared/runtime-client-target' import type { GitHubAssignableUser } from '../../../shared/github/pull-request-types' @@ -73,17 +74,16 @@ export function useRepoAssigneesBySlug( const repositoryKey = owner && repo ? githubRepoIdentityKey({ owner, repo, host }) : null const cacheKey = repositoryKey ? target.kind === 'environment' - ? `runtime:${target.environmentId}:${repositoryKey}#${seedKey}` - : `${repositoryKey}#${seedKey}` + ? `runtime:${target.environmentId}:${repositoryKey}` + : repositoryKey : null const args = { owner: selectedOwner, repo: selectedRepo, - host: githubProjectHost(host), - ...(seedKey ? { seedLogins: seedKey.split(',') } : {}) + host: githubProjectHost(host) } - return useMetadataListRequest({ + const base = useMetadataListRequest({ cacheKey, store: slugAssigneeStore, errorFallback: 'Failed to load assignees', @@ -103,4 +103,19 @@ export function useRepoAssigneesBySlug( return res.users }) }) + const data = useMemo(() => { + if (!cacheKey || base.resolvedKey !== cacheKey || !seedKey) { + return base.data + } + const users = [...base.data] + const seen = new Set(users.map((user) => user.login)) + for (const login of seedKey.split(',')) { + if (!seen.has(login)) { + users.push({ login, name: null, avatarUrl: '' }) + seen.add(login) + } + } + return users + }, [base.data, base.resolvedKey, cacheKey, seedKey]) + return { data, loading: base.loading, error: base.error } } diff --git a/src/renderer/src/hooks/useMetadataListRequest.test.tsx b/src/renderer/src/hooks/useMetadataListRequest.test.tsx index 3e0c6a68055..90fc7e8b6ff 100644 --- a/src/renderer/src/hooks/useMetadataListRequest.test.tsx +++ b/src/renderer/src/hooks/useMetadataListRequest.test.tsx @@ -63,6 +63,11 @@ describe('useMetadataListRequest', () => { first.resolve(['first']) await first.promise }) - expect(view.result.current).toEqual({ data: ['second'], loading: false, error: null }) + expect(view.result.current).toEqual({ + data: ['second'], + loading: false, + error: null, + resolvedKey: 'second' + }) }) }) diff --git a/src/renderer/src/hooks/useMetadataListRequest.ts b/src/renderer/src/hooks/useMetadataListRequest.ts index 186b620d6cd..90f44cdd158 100644 --- a/src/renderer/src/hooks/useMetadataListRequest.ts +++ b/src/renderer/src/hooks/useMetadataListRequest.ts @@ -19,11 +19,12 @@ export function useMetadataListRequest({ store, load, errorFallback -}: MetadataListRequest): MetadataListState { - const [state, setState] = useState>({ +}: MetadataListRequest): MetadataListState & { resolvedKey: string | null } { + const [state, setState] = useState & { resolvedKey: string | null }>({ data: [], loading: false, - error: null + error: null, + resolvedKey: null }) const activeKeyRef = useRef(null) const loadLatest = useEffectEvent(load) @@ -36,7 +37,7 @@ export function useMetadataListRequest({ const cached = getFreshMetadata(store, cacheKey) if (cached) { if (activeKeyRef.current !== cacheKey) { - setState({ data: cached.data, loading: false, error: null }) + setState({ data: cached.data, loading: false, error: null, resolvedKey: cacheKey }) } activeKeyRef.current = cacheKey return @@ -47,12 +48,13 @@ export function useMetadataListRequest({ ...current, data: current.data.length ? [] : current.data, loading: true, - error: null + error: null, + resolvedKey: null })) void loadMetadata(store, cacheKey, () => loadLatest()) .then((data) => { if (activeKeyRef.current === requestKey) { - setState({ data, loading: false, error: null }) + setState({ data, loading: false, error: null, resolvedKey: requestKey }) } }) .catch((error: unknown) => {