diff --git a/static/app/views/onboarding/components/useScmRepoSelection.spec.tsx b/static/app/views/onboarding/components/useScmRepoSelection.spec.tsx index edd6aa20cfba24..f8e827339c66a0 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.spec.tsx +++ b/static/app/views/onboarding/components/useScmRepoSelection.spec.tsx @@ -1,27 +1,12 @@ import {OrganizationFixture} from 'sentry-fixture/organization'; import {OrganizationIntegrationsFixture} from 'sentry-fixture/organizationIntegrations'; -import {act, renderHookWithProviders, waitFor} from 'sentry-test/reactTestingLibrary'; +import {act, renderHookWithProviders} from 'sentry-test/reactTestingLibrary'; -import { - OnboardingContextProvider, - type OnboardingSessionState, -} from 'sentry/components/onboarding/onboardingContext'; import type {IntegrationRepository} from 'sentry/types/integrations'; -import {sessionStorageWrapper} from 'sentry/utils/sessionStorage'; import {useScmRepoSelection} from './useScmRepoSelection'; -function makeOnboardingWrapper(initialState?: OnboardingSessionState) { - return function OnboardingWrapper({children}: {children?: React.ReactNode}) { - return ( - - {children} - - ); - }; -} - describe('useScmRepoSelection', () => { const organization = OrganizationFixture(); @@ -49,29 +34,56 @@ describe('useScmRepoSelection', () => { isInstalled: false, }; - const mockInstalledRepo: IntegrationRepository = { - identifier: 'getsentry/sentry', - name: 'sentry', - isInstalled: true, - }; - beforeEach(() => { - sessionStorageWrapper.clear(); onSelect = jest.fn(); reposByIdentifier = new Map([['getsentry/sentry', mockRepo]]); + }); - // Default: no existing repos + afterEach(() => { + MockApiClient.clearMockResponses(); + }); + + it('uses existing repo when GET finds it, skips POST', async () => { MockApiClient.addMockResponse({ url: `/organizations/${organization.slug}/repos/`, - body: [], + body: [ + { + id: '99', + name: 'getsentry/sentry', + externalSlug: 'getsentry/sentry', + status: 'active', + }, + ], }); - }); - afterEach(() => { - MockApiClient.clearMockResponses(); + const addRequest = MockApiClient.addMockResponse({ + url: `/organizations/${organization.slug}/repos/`, + method: 'POST', + body: {}, + }); + + const {result} = renderHookWithProviders( + () => + useScmRepoSelection({integration: mockIntegration, onSelect, reposByIdentifier}), + {organization} + ); + + await act(async () => { + await result.current.handleSelect({value: 'getsentry/sentry'}); + }); + + expect(addRequest).not.toHaveBeenCalled(); + expect(onSelect).toHaveBeenCalledWith( + expect.objectContaining({id: '99', name: 'getsentry/sentry'}) + ); }); - it('calls POST to add new repo and updates onSelect with server ID', async () => { + it('creates repo via POST when GET finds nothing', async () => { + MockApiClient.addMockResponse({ + url: `/organizations/${organization.slug}/repos/`, + body: [], + }); + const addRequest = MockApiClient.addMockResponse({ url: `/organizations/${organization.slug}/repos/`, method: 'POST', @@ -86,10 +98,7 @@ describe('useScmRepoSelection', () => { const {result} = renderHookWithProviders( () => useScmRepoSelection({integration: mockIntegration, onSelect, reposByIdentifier}), - { - organization, - additionalWrapper: makeOnboardingWrapper({}), - } + {organization} ); await act(async () => { @@ -101,61 +110,45 @@ describe('useScmRepoSelection', () => { expect(onSelect).toHaveBeenCalledWith( expect.objectContaining({id: '', name: 'sentry'}) ); - // Then real call with server response spread over optimistic + // Then real call with server response expect(onSelect).toHaveBeenCalledWith( expect.objectContaining({id: '42', name: 'getsentry/sentry'}) ); }); - it('does not POST for already-installed repos, uses existing repo ID', async () => { - reposByIdentifier = new Map([['getsentry/sentry', mockInstalledRepo]]); - - // Override repos response with an existing repo - MockApiClient.clearMockResponses(); + it('reverts onSelect on POST failure', async () => { MockApiClient.addMockResponse({ url: `/organizations/${organization.slug}/repos/`, - body: [ - { - id: '99', - name: 'getsentry/sentry', - externalSlug: 'getsentry/sentry', - status: 'active', - }, - ], + body: [], }); - const addRequest = MockApiClient.addMockResponse({ + MockApiClient.addMockResponse({ url: `/organizations/${organization.slug}/repos/`, method: 'POST', - body: {}, + statusCode: 500, + body: {detail: 'Internal Error'}, }); const {result} = renderHookWithProviders( () => useScmRepoSelection({integration: mockIntegration, onSelect, reposByIdentifier}), - { - organization, - additionalWrapper: makeOnboardingWrapper({}), - } + {organization} ); - // Wait for existing repos query to resolve - await waitFor(() => expect(result.current.busy).toBe(false)); - await act(async () => { await result.current.handleSelect({value: 'getsentry/sentry'}); }); - expect(addRequest).not.toHaveBeenCalled(); + // Optimistic, then revert expect(onSelect).toHaveBeenCalledWith( - expect.objectContaining({id: '99', name: 'getsentry/sentry'}) + expect.objectContaining({id: '', name: 'sentry'}) ); + expect(onSelect).toHaveBeenCalledWith(undefined); }); - it('reverts onSelect on addRepository failure', async () => { + it('reverts onSelect on GET failure', async () => { MockApiClient.addMockResponse({ url: `/organizations/${organization.slug}/repos/`, - method: 'POST', statusCode: 500, body: {detail: 'Internal Error'}, }); @@ -163,109 +156,58 @@ describe('useScmRepoSelection', () => { const {result} = renderHookWithProviders( () => useScmRepoSelection({integration: mockIntegration, onSelect, reposByIdentifier}), - { - organization, - additionalWrapper: makeOnboardingWrapper({}), - } + {organization} ); await act(async () => { await result.current.handleSelect({value: 'getsentry/sentry'}); }); - // Optimistic, then revert expect(onSelect).toHaveBeenCalledWith( expect.objectContaining({id: '', name: 'sentry'}) ); expect(onSelect).toHaveBeenCalledWith(undefined); }); - it('cleans up previously added repo when selecting a new one', async () => { - MockApiClient.addMockResponse({ - url: `/organizations/${organization.slug}/repos/`, - method: 'POST', - body: { - id: '42', - name: 'getsentry/sentry', - externalSlug: 'getsentry/sentry', - status: 'active', - }, - }); - - const hideRequest = MockApiClient.addMockResponse({ - url: `/organizations/${organization.slug}/repos/42/`, - method: 'PUT', - body: {}, - }); - + it('handleRemove calls onSelect with undefined', () => { const {result} = renderHookWithProviders( () => useScmRepoSelection({integration: mockIntegration, onSelect, reposByIdentifier}), - { - organization, - additionalWrapper: makeOnboardingWrapper({}), - } + {organization} ); - // First selection - await act(async () => { - await result.current.handleSelect({value: 'getsentry/sentry'}); + act(() => { + result.current.handleRemove(); }); - // Add a second repo - const secondRepo: IntegrationRepository = { - identifier: 'getsentry/relay', - name: 'relay', - isInstalled: false, - }; - reposByIdentifier.set('getsentry/relay', secondRepo); + expect(onSelect).toHaveBeenCalledWith(undefined); + }); + it('clears busy state after selection completes', async () => { MockApiClient.addMockResponse({ url: `/organizations/${organization.slug}/repos/`, - method: 'POST', - body: { - id: '43', - name: 'getsentry/relay', - externalSlug: 'getsentry/relay', - status: 'active', - }, - }); - - // Second selection should clean up the first - await act(async () => { - await result.current.handleSelect({value: 'getsentry/relay'}); - }); - - expect(hideRequest).toHaveBeenCalled(); - }); - - it('does not call hideRepository on remove if repo was pre-existing', async () => { - const hideRequest = MockApiClient.addMockResponse({ - url: `/organizations/${organization.slug}/repos/99/`, - method: 'PUT', - body: {}, + body: [ + { + id: '99', + name: 'getsentry/sentry', + externalSlug: 'getsentry/sentry', + status: 'active', + }, + ], }); const {result} = renderHookWithProviders( () => useScmRepoSelection({integration: mockIntegration, onSelect, reposByIdentifier}), - { - organization, - additionalWrapper: makeOnboardingWrapper({ - selectedRepository: { - id: '99', - name: 'sentry', - externalSlug: 'getsentry/sentry', - } as any, - }), - } + {organization} ); + expect(result.current.busy).toBe(false); + await act(async () => { - await result.current.handleRemove(); + await result.current.handleSelect({value: 'getsentry/sentry'}); }); - expect(onSelect).toHaveBeenCalledWith(undefined); - expect(hideRequest).not.toHaveBeenCalled(); + expect(result.current.busy).toBe(false); }); }); diff --git a/static/app/views/onboarding/components/useScmRepoSelection.ts b/static/app/views/onboarding/components/useScmRepoSelection.ts index fa0ae9424693f6..42274b637e99f4 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.ts +++ b/static/app/views/onboarding/components/useScmRepoSelection.ts @@ -1,6 +1,7 @@ -import {useMemo, useRef, useState} from 'react'; +import {useState} from 'react'; -import {useOnboardingContext} from 'sentry/components/onboarding/onboardingContext'; +import {addErrorMessage} from 'sentry/actionCreators/indicator'; +import {t} from 'sentry/locale'; import type { Integration, IntegrationRepository, @@ -8,7 +9,8 @@ import type { } from 'sentry/types/integrations'; import {RepositoryStatus} from 'sentry/types/integrations'; import {getApiUrl} from 'sentry/utils/api/getApiUrl'; -import {fetchMutation, useApiQuery} from 'sentry/utils/queryClient'; +import type {ApiQueryKey} from 'sentry/utils/queryClient'; +import {fetchDataQuery, fetchMutation, useQueryClient} from 'sentry/utils/queryClient'; import {useOrganization} from 'sentry/utils/useOrganization'; interface UseScmRepoSelectionOptions { @@ -43,69 +45,55 @@ export function useScmRepoSelection({ reposByIdentifier, }: UseScmRepoSelectionOptions) { const organization = useOrganization(); - const {selectedRepository} = useOnboardingContext(); + const queryClient = useQueryClient(); const [busy, setBusy] = useState(false); - // Fetch repos already registered in Sentry for this integration, so we - // can look up the real Repository (with Sentry ID) for "Already Added" repos. - const {data: existingRepos, isPending: existingReposPending} = useApiQuery< - Repository[] - >( - [ - getApiUrl('/organizations/$organizationIdOrSlug/repos/', { - path: {organizationIdOrSlug: organization.slug}, - }), - {query: {status: 'active', integration_id: integration.id}}, - ], - {staleTime: 0} - ); - - const existingReposBySlug = useMemo( - () => new Map((existingRepos ?? []).map(r => [r.externalSlug, r])), - [existingRepos] - ); - - // Track the ID of a repo we added during this session so we can clean - // it up if the user switches to a different repo. - const addedRepoIdRef = useRef(null); - - // Best-effort cleanup: fire-and-forget the hide request. If it fails the - // repo stays registered in Sentry but is non-critical for the onboarding flow. - const cleanupPreviousAdd = () => { - if (addedRepoIdRef.current) { - fetchMutation({ - url: `/organizations/${organization.slug}/repos/${addedRepoIdRef.current}/`, - method: 'PUT', - data: {status: 'hidden'}, - }).catch(() => {}); - addedRepoIdRef.current = null; - } - }; - const handleSelect = async (selection: {value: string}) => { const repo = reposByIdentifier.get(selection.value); if (!repo) { return; } - cleanupPreviousAdd(); - const optimistic = buildOptimisticRepo(repo, integration); onSelect(optimistic); - if (repo.isInstalled) { - const existing = existingReposBySlug.get(repo.identifier); + // Look up the repo in Sentry. The background link_all_repos task + // registers all repos after integration install, so most repos will + // already exist. Use a targeted query filtered by name to avoid + // pagination issues with the full list. + setBusy(true); + try { + const queryKey: ApiQueryKey = [ + getApiUrl('/organizations/$organizationIdOrSlug/repos/', { + path: {organizationIdOrSlug: organization.slug}, + }), + { + query: { + status: 'active', + integration_id: integration.id, + query: repo.identifier, + }, + }, + ]; + const [matches] = await queryClient.fetchQuery({ + queryKey, + queryFn: fetchDataQuery, + staleTime: 0, + }); + // The query param above is an icontains filter to narrow results + // and avoid pagination. The exact match here uses Repository.name + // against IntegrationRepository.identifier — the same comparison the + // backend uses in organization_integration_repos.py:61,80 to determine + // isInstalled. Can't use externalSlug because it varies by provider + // (e.g. GitLab returns a numeric project ID). + const existing = matches?.find(r => r.name === repo.identifier); + if (existing) { onSelect({...optimistic, ...existing}); return; } - // Lookup missed (e.g., repo was hidden). Fall through to re-add it. - } - // Note: for project creation (non-onboarding), we'll also need to handle - // migrateRepository for repos previously connected via legacy plugins. - setBusy(true); - try { + // Repo not yet registered (link_all_repos may still be running). const created = await fetchMutation({ url: `/organizations/${organization.slug}/repos/`, method: 'POST', @@ -116,43 +104,20 @@ export function useScmRepoSelection({ }, }); onSelect({...optimistic, ...created}); - addedRepoIdRef.current = created.id; } catch { + addErrorMessage(t('Failed to select repository')); onSelect(undefined); } finally { setBusy(false); } }; - const handleRemove = async () => { - if (!selectedRepository) { - return; - } - - const previous = selectedRepository; + const handleRemove = () => { onSelect(undefined); - - if (addedRepoIdRef.current && addedRepoIdRef.current === previous.id) { - setBusy(true); - try { - await fetchMutation({ - url: `/organizations/${organization.slug}/repos/${previous.id}/`, - method: 'PUT', - data: {status: 'hidden'}, - }); - addedRepoIdRef.current = null; - } catch { - onSelect(previous); - } finally { - setBusy(false); - } - } }; return { - // Busy while adding/removing a repo or while existing repos are still - // loading. The UI disables the Select and remove button when true. - busy: busy || existingReposPending, + busy, handleSelect, handleRemove, };