From 4168b8a737ded186caac31ee65bc8d8e55d4c062 Mon Sep 17 00:00:00 2001 From: Jay Goss Date: Thu, 26 Mar 2026 18:51:10 -0500 Subject: [PATCH 01/14] fix(onboarding): Handle repo selection race with background link_all_repos After a GitHub integration is installed, link_all_repos runs async and may still be populating repos when the user reaches the repo selector. The previous approach cached existing repos on mount and looked them up before deciding whether to POST, but the cache was often stale or paginated, causing selection to fail silently. Now always POST first. If the repo already exists (background task or previous session created it), the POST fails and we fall back to a targeted GET filtered by repo name to find the existing record. This avoids both the stale cache and pagination issues. --- .../components/useScmRepoSelection.ts | 71 ++++++++++--------- 1 file changed, 36 insertions(+), 35 deletions(-) diff --git a/static/app/views/onboarding/components/useScmRepoSelection.ts b/static/app/views/onboarding/components/useScmRepoSelection.ts index 807ddf295deee7..a61ad9ae7fd176 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.ts +++ b/static/app/views/onboarding/components/useScmRepoSelection.ts @@ -1,5 +1,6 @@ -import {useMemo, useRef, useState} from 'react'; +import {useRef, useState} from 'react'; +import {Client} from 'sentry/api'; import {useOnboardingContext} from 'sentry/components/onboarding/onboardingContext'; import type { Integration, @@ -7,8 +8,7 @@ import type { Repository, } 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 {fetchMutation} from 'sentry/utils/queryClient'; import {useOrganization} from 'sentry/utils/useOrganization'; interface UseScmRepoSelectionOptions { @@ -46,25 +46,6 @@ export function useScmRepoSelection({ const {selectedRepository} = useOnboardingContext(); 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); @@ -93,15 +74,10 @@ export function useScmRepoSelection({ const optimistic = buildOptimisticRepo(repo, integration); onSelect(optimistic); - if (repo.isInstalled) { - const existing = existingReposBySlug.get(repo.identifier); - if (existing) { - onSelect({...optimistic, ...existing}); - return; - } - // Lookup missed (e.g., repo was hidden). Fall through to re-add it. - } - + // Always try POST first. If the background link_all_repos task already + // created this repo, the POST will fail and we fall back to fetching + // the existing record. This avoids relying on a potentially stale cache + // that may not have caught up with the background task yet. setBusy(true); try { const created = await fetchMutation({ @@ -116,7 +92,32 @@ export function useScmRepoSelection({ onSelect({...optimistic, ...created}); addedRepoIdRef.current = created.id; } catch { - onSelect(undefined); + // POST failed — likely because the repo already exists (created by + // link_all_repos or a previous session). Query for it by name to + // avoid pagination issues with the full repo list. + try { + const api = new Client(); + const matches = await api.requestPromise( + `/organizations/${organization.slug}/repos/`, + { + query: { + status: 'active', + integration_id: integration.id, + query: repo.identifier, + }, + } + ); + const existing = (matches as Repository[])?.find( + r => r.externalSlug === repo.identifier + ); + if (existing) { + onSelect({...optimistic, ...existing}); + } else { + onSelect(undefined); + } + } catch { + onSelect(undefined); + } } finally { setBusy(false); } @@ -148,9 +149,9 @@ export function useScmRepoSelection({ }; 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 while adding/removing a repo. + // The UI disables the Select and remove button when true. + busy, handleSelect, handleRemove, }; From c9bef349ebf2438b4d9985e7b04c8ff872dcdf3c Mon Sep 17 00:00:00 2001 From: Jay Goss Date: Thu, 26 Mar 2026 18:53:44 -0500 Subject: [PATCH 02/14] fix(onboarding): Check for existing repo before POST to avoid noise Flip the order: do a targeted GET filtered by repo name first, and only POST if the repo doesn't exist yet. This avoids expected 400s showing up in devtools and Sentry when the background link_all_repos task has already created the repo. --- .../components/useScmRepoSelection.ts | 55 +++++++++---------- 1 file changed, 25 insertions(+), 30 deletions(-) diff --git a/static/app/views/onboarding/components/useScmRepoSelection.ts b/static/app/views/onboarding/components/useScmRepoSelection.ts index a61ad9ae7fd176..29481d05b6a545 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.ts +++ b/static/app/views/onboarding/components/useScmRepoSelection.ts @@ -74,12 +74,32 @@ export function useScmRepoSelection({ const optimistic = buildOptimisticRepo(repo, integration); onSelect(optimistic); - // Always try POST first. If the background link_all_repos task already - // created this repo, the POST will fail and we fall back to fetching - // the existing record. This avoids relying on a potentially stale cache - // that may not have caught up with the background task yet. + // Check if the repo already exists in Sentry (e.g. created by the + // background link_all_repos task or a previous session). Use a targeted + // query filtered by name to avoid pagination issues with the full list. setBusy(true); try { + const api = new Client(); + const matches = await api.requestPromise( + `/organizations/${organization.slug}/repos/`, + { + query: { + status: 'active', + integration_id: integration.id, + query: repo.identifier, + }, + } + ); + const existing = (matches as Repository[])?.find( + r => r.externalSlug === repo.identifier + ); + + if (existing) { + onSelect({...optimistic, ...existing}); + return; + } + + // Repo not found — create it. const created = await fetchMutation({ url: `/organizations/${organization.slug}/repos/`, method: 'POST', @@ -92,32 +112,7 @@ export function useScmRepoSelection({ onSelect({...optimistic, ...created}); addedRepoIdRef.current = created.id; } catch { - // POST failed — likely because the repo already exists (created by - // link_all_repos or a previous session). Query for it by name to - // avoid pagination issues with the full repo list. - try { - const api = new Client(); - const matches = await api.requestPromise( - `/organizations/${organization.slug}/repos/`, - { - query: { - status: 'active', - integration_id: integration.id, - query: repo.identifier, - }, - } - ); - const existing = (matches as Repository[])?.find( - r => r.externalSlug === repo.identifier - ); - if (existing) { - onSelect({...optimistic, ...existing}); - } else { - onSelect(undefined); - } - } catch { - onSelect(undefined); - } + onSelect(undefined); } finally { setBusy(false); } From 18a823e57a38be883d214a6183916a256e75ede0 Mon Sep 17 00:00:00 2001 From: Jay Goss Date: Thu, 26 Mar 2026 18:55:51 -0500 Subject: [PATCH 03/14] ref(onboarding): Use fetchMutation for GET instead of deprecated Client Replace direct Client.requestPromise usage with fetchMutation which wraps the same call but is the recommended API. --- .../components/useScmRepoSelection.ts | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/static/app/views/onboarding/components/useScmRepoSelection.ts b/static/app/views/onboarding/components/useScmRepoSelection.ts index 29481d05b6a545..af66de8d347c6b 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.ts +++ b/static/app/views/onboarding/components/useScmRepoSelection.ts @@ -1,6 +1,5 @@ import {useRef, useState} from 'react'; -import {Client} from 'sentry/api'; import {useOnboardingContext} from 'sentry/components/onboarding/onboardingContext'; import type { Integration, @@ -79,20 +78,18 @@ export function useScmRepoSelection({ // query filtered by name to avoid pagination issues with the full list. setBusy(true); try { - const api = new Client(); - const matches = await api.requestPromise( - `/organizations/${organization.slug}/repos/`, - { + const matches = await fetchMutation({ + url: `/organizations/${organization.slug}/repos/`, + method: 'GET', + options: { query: { status: 'active', integration_id: integration.id, query: repo.identifier, }, - } - ); - const existing = (matches as Repository[])?.find( - r => r.externalSlug === repo.identifier - ); + }, + }); + const existing = matches?.find(r => r.externalSlug === repo.identifier); if (existing) { onSelect({...optimistic, ...existing}); From fd253156fc0193b114fd4e173f8f1bfcb649b4f5 Mon Sep 17 00:00:00 2001 From: Jay Goss Date: Thu, 26 Mar 2026 18:56:55 -0500 Subject: [PATCH 04/14] fix(onboarding): Use QUERY_API_CLIENT for GET request fetchMutation only accepts PUT/POST/DELETE methods. Use the shared QUERY_API_CLIENT instance directly for the imperative GET call. --- .../onboarding/components/useScmRepoSelection.ts | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/static/app/views/onboarding/components/useScmRepoSelection.ts b/static/app/views/onboarding/components/useScmRepoSelection.ts index af66de8d347c6b..408e2f7a4a95cf 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.ts +++ b/static/app/views/onboarding/components/useScmRepoSelection.ts @@ -7,7 +7,7 @@ import type { Repository, } from 'sentry/types/integrations'; import {RepositoryStatus} from 'sentry/types/integrations'; -import {fetchMutation} from 'sentry/utils/queryClient'; +import {fetchMutation, QUERY_API_CLIENT} from 'sentry/utils/queryClient'; import {useOrganization} from 'sentry/utils/useOrganization'; interface UseScmRepoSelectionOptions { @@ -78,17 +78,16 @@ export function useScmRepoSelection({ // query filtered by name to avoid pagination issues with the full list. setBusy(true); try { - const matches = await fetchMutation({ - url: `/organizations/${organization.slug}/repos/`, - method: 'GET', - options: { + const matches: Repository[] = await QUERY_API_CLIENT.requestPromise( + `/organizations/${organization.slug}/repos/`, + { query: { status: 'active', integration_id: integration.id, query: repo.identifier, }, - }, - }); + } + ); const existing = matches?.find(r => r.externalSlug === repo.identifier); if (existing) { From 1fe539b010de6d970c5b8ead2dc5b0cc2c8b922d Mon Sep 17 00:00:00 2001 From: Jay Goss Date: Thu, 26 Mar 2026 19:03:42 -0500 Subject: [PATCH 05/14] ref(onboarding): Use queryClient.fetchQuery for imperative GET Replace deprecated QUERY_API_CLIENT.requestPromise with the sanctioned queryClient.fetchQuery + fetchDataQuery pattern for imperative GET requests outside of React Query hooks. --- .../components/useScmRepoSelection.ts | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/static/app/views/onboarding/components/useScmRepoSelection.ts b/static/app/views/onboarding/components/useScmRepoSelection.ts index 408e2f7a4a95cf..415d32869172e9 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.ts +++ b/static/app/views/onboarding/components/useScmRepoSelection.ts @@ -7,7 +7,7 @@ import type { Repository, } from 'sentry/types/integrations'; import {RepositoryStatus} from 'sentry/types/integrations'; -import {fetchMutation, QUERY_API_CLIENT} from 'sentry/utils/queryClient'; +import {fetchDataQuery, fetchMutation, useQueryClient} from 'sentry/utils/queryClient'; import {useOrganization} from 'sentry/utils/useOrganization'; interface UseScmRepoSelectionOptions { @@ -42,6 +42,7 @@ export function useScmRepoSelection({ reposByIdentifier, }: UseScmRepoSelectionOptions) { const organization = useOrganization(); + const queryClient = useQueryClient(); const {selectedRepository} = useOnboardingContext(); const [busy, setBusy] = useState(false); @@ -78,16 +79,20 @@ export function useScmRepoSelection({ // query filtered by name to avoid pagination issues with the full list. setBusy(true); try { - const matches: Repository[] = await QUERY_API_CLIENT.requestPromise( - `/organizations/${organization.slug}/repos/`, - { - query: { - status: 'active', - integration_id: integration.id, - query: repo.identifier, + const [matches] = await queryClient.fetchQuery({ + queryKey: [ + `/organizations/${organization.slug}/repos/`, + { + query: { + status: 'active', + integration_id: integration.id, + query: repo.identifier, + }, }, - } - ); + ], + queryFn: fetchDataQuery, + staleTime: 0, + }); const existing = matches?.find(r => r.externalSlug === repo.identifier); if (existing) { From e6a9c800040a36dc1ffcadbf27750f136fb34377 Mon Sep 17 00:00:00 2001 From: Jay Goss Date: Thu, 26 Mar 2026 19:07:09 -0500 Subject: [PATCH 06/14] fix(onboarding): Fix TypeScript errors in fetchQuery usage Use ApiQueryKey type with getApiUrl for proper URL typing, satisfying the queryKey constraint on queryClient.fetchQuery. --- .../components/useScmRepoSelection.ts | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/static/app/views/onboarding/components/useScmRepoSelection.ts b/static/app/views/onboarding/components/useScmRepoSelection.ts index 415d32869172e9..74dfac4ac1da79 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.ts +++ b/static/app/views/onboarding/components/useScmRepoSelection.ts @@ -7,6 +7,8 @@ import type { Repository, } from 'sentry/types/integrations'; import {RepositoryStatus} from 'sentry/types/integrations'; +import {getApiUrl} from 'sentry/utils/api/getApiUrl'; +import type {ApiQueryKey} from 'sentry/utils/queryClient'; import {fetchDataQuery, fetchMutation, useQueryClient} from 'sentry/utils/queryClient'; import {useOrganization} from 'sentry/utils/useOrganization'; @@ -79,17 +81,20 @@ export function useScmRepoSelection({ // query filtered by name to avoid pagination issues with the full list. setBusy(true); try { - const [matches] = await queryClient.fetchQuery({ - queryKey: [ - `/organizations/${organization.slug}/repos/`, - { - query: { - status: 'active', - integration_id: integration.id, - query: repo.identifier, - }, + 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, }); From 88bc7fa27a6d8270b3610aa18f3a0f4e41a561bc Mon Sep 17 00:00:00 2001 From: Jay Goss Date: Thu, 26 Mar 2026 19:09:36 -0500 Subject: [PATCH 07/14] ref(onboarding): Remove repo hide/cleanup logic from selection hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With link_all_repos running in the background after integration install, all repos are automatically registered in Sentry. The hide-on-deselect and cleanup-on-switch logic was designed for manual repo registration and is now counterproductive — it removes repo records that the background task created, causing them to disappear from settings. Selection is now just a context pointer. --- .../components/useScmRepoSelection.ts | 58 +++---------------- 1 file changed, 7 insertions(+), 51 deletions(-) diff --git a/static/app/views/onboarding/components/useScmRepoSelection.ts b/static/app/views/onboarding/components/useScmRepoSelection.ts index 74dfac4ac1da79..fff0cdde4d706e 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.ts +++ b/static/app/views/onboarding/components/useScmRepoSelection.ts @@ -1,6 +1,5 @@ -import {useRef, useState} from 'react'; +import {useState} from 'react'; -import {useOnboardingContext} from 'sentry/components/onboarding/onboardingContext'; import type { Integration, IntegrationRepository, @@ -45,40 +44,21 @@ export function useScmRepoSelection({ }: UseScmRepoSelectionOptions) { const organization = useOrganization(); const queryClient = useQueryClient(); - const {selectedRepository} = useOnboardingContext(); const [busy, setBusy] = useState(false); - // 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); - // Check if the repo already exists in Sentry (e.g. created by the - // background link_all_repos task or a previous session). Use a targeted - // query filtered by name to avoid pagination issues with the full list. + // 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 = [ @@ -105,7 +85,7 @@ export function useScmRepoSelection({ return; } - // Repo not found — create it. + // Repo not yet registered (link_all_repos may still be running). const created = await fetchMutation({ url: `/organizations/${organization.slug}/repos/`, method: 'POST', @@ -116,7 +96,6 @@ export function useScmRepoSelection({ }, }); onSelect({...optimistic, ...created}); - addedRepoIdRef.current = created.id; } catch { onSelect(undefined); } finally { @@ -124,34 +103,11 @@ export function useScmRepoSelection({ } }; - 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. - // The UI disables the Select and remove button when true. busy, handleSelect, handleRemove, From 9a20d697de886bbc7bd782f4810d6b6159495942 Mon Sep 17 00:00:00 2001 From: Jay Goss Date: Thu, 26 Mar 2026 19:19:12 -0500 Subject: [PATCH 08/14] test(onboarding): Update useScmRepoSelection tests for new behavior Rewrite tests to match the GET-first-then-POST approach and removal of repo hide/cleanup logic. Remove tests for cleanup-on-switch and hide-on-remove since those behaviors no longer exist. --- .../components/useScmRepoSelection.spec.tsx | 207 ++++++------------ 1 file changed, 66 insertions(+), 141 deletions(-) diff --git a/static/app/views/onboarding/components/useScmRepoSelection.spec.tsx b/static/app/views/onboarding/components/useScmRepoSelection.spec.tsx index edd6aa20cfba24..46d5c989e64a8a 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.spec.tsx +++ b/static/app/views/onboarding/components/useScmRepoSelection.spec.tsx @@ -3,25 +3,10 @@ import {OrganizationIntegrationsFixture} from 'sentry-fixture/organizationIntegr import {act, renderHookWithProviders, waitFor} 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,69 +34,16 @@ 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 - MockApiClient.addMockResponse({ - url: `/organizations/${organization.slug}/repos/`, - body: [], - }); }); afterEach(() => { MockApiClient.clearMockResponses(); }); - it('calls POST to add new repo and updates onSelect with server ID', async () => { - const addRequest = MockApiClient.addMockResponse({ - url: `/organizations/${organization.slug}/repos/`, - method: 'POST', - body: { - id: '42', - name: 'getsentry/sentry', - externalSlug: 'getsentry/sentry', - status: 'active', - }, - }); - - const {result} = renderHookWithProviders( - () => - useScmRepoSelection({integration: mockIntegration, onSelect, reposByIdentifier}), - { - organization, - additionalWrapper: makeOnboardingWrapper({}), - } - ); - - await act(async () => { - await result.current.handleSelect({value: 'getsentry/sentry'}); - }); - - expect(addRequest).toHaveBeenCalled(); - // Optimistic call with empty id - expect(onSelect).toHaveBeenCalledWith( - expect.objectContaining({id: '', name: 'sentry'}) - ); - // Then real call with server response spread over optimistic - 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('uses existing repo when GET finds it, skips POST', async () => { MockApiClient.addMockResponse({ url: `/organizations/${organization.slug}/repos/`, body: [ @@ -133,15 +65,9 @@ describe('useScmRepoSelection', () => { 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'}); }); @@ -152,120 +78,119 @@ describe('useScmRepoSelection', () => { ); }); - it('reverts onSelect on addRepository failure', 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', - statusCode: 500, - body: {detail: 'Internal Error'}, + body: { + id: '42', + name: 'getsentry/sentry', + externalSlug: 'getsentry/sentry', + status: 'active', + }, }); 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(addRequest).toHaveBeenCalled(); + // Optimistic call with empty id expect(onSelect).toHaveBeenCalledWith( expect.objectContaining({id: '', name: 'sentry'}) ); - expect(onSelect).toHaveBeenCalledWith(undefined); + // Then real call with server response + expect(onSelect).toHaveBeenCalledWith( + expect.objectContaining({id: '42', name: 'getsentry/sentry'}) + ); }); - it('cleans up previously added repo when selecting a new one', async () => { + it('reverts onSelect when both GET and POST fail', async () => { MockApiClient.addMockResponse({ url: `/organizations/${organization.slug}/repos/`, - method: 'POST', - body: { - id: '42', - name: 'getsentry/sentry', - externalSlug: 'getsentry/sentry', - status: 'active', - }, + body: [], }); - const hideRequest = MockApiClient.addMockResponse({ - url: `/organizations/${organization.slug}/repos/42/`, - method: 'PUT', - body: {}, + MockApiClient.addMockResponse({ + url: `/organizations/${organization.slug}/repos/`, + method: 'POST', + statusCode: 500, + body: {detail: 'Internal Error'}, }); const {result} = renderHookWithProviders( () => useScmRepoSelection({integration: mockIntegration, onSelect, reposByIdentifier}), - { - organization, - additionalWrapper: makeOnboardingWrapper({}), - } + {organization} ); - // First selection await act(async () => { await result.current.handleSelect({value: 'getsentry/sentry'}); }); - // Add a second repo - const secondRepo: IntegrationRepository = { - identifier: 'getsentry/relay', - name: 'relay', - isInstalled: false, - }; - reposByIdentifier.set('getsentry/relay', secondRepo); + // Optimistic, then revert + expect(onSelect).toHaveBeenCalledWith( + expect.objectContaining({id: '', name: 'sentry'}) + ); + expect(onSelect).toHaveBeenCalledWith(undefined); + }); - MockApiClient.addMockResponse({ - url: `/organizations/${organization.slug}/repos/`, - method: 'POST', - body: { - id: '43', - name: 'getsentry/relay', - externalSlug: 'getsentry/relay', - status: 'active', - }, - }); + it('handleRemove calls onSelect with undefined', async () => { + const {result} = renderHookWithProviders( + () => + useScmRepoSelection({integration: mockIntegration, onSelect, reposByIdentifier}), + {organization} + ); - // Second selection should clean up the first - await act(async () => { - await result.current.handleSelect({value: 'getsentry/relay'}); + act(() => { + result.current.handleRemove(); }); - expect(hideRequest).toHaveBeenCalled(); + expect(onSelect).toHaveBeenCalledWith(undefined); }); - 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: {}, + it('sets busy during selection and clears after', async () => { + MockApiClient.addMockResponse({ + url: `/organizations/${organization.slug}/repos/`, + 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); + + let selectPromise: Promise; + act(() => { + selectPromise = result.current.handleSelect({value: 'getsentry/sentry'}); + }); + + await waitFor(() => expect(result.current.busy).toBe(false)); await act(async () => { - await result.current.handleRemove(); + await selectPromise!; }); - expect(onSelect).toHaveBeenCalledWith(undefined); - expect(hideRequest).not.toHaveBeenCalled(); + expect(result.current.busy).toBe(false); }); }); From 44c7d1a29dfa4ac101ed2ef5d88da086558e2397 Mon Sep 17 00:00:00 2001 From: Jay Goss Date: Thu, 26 Mar 2026 19:23:52 -0500 Subject: [PATCH 09/14] fix(onboarding): Match repos by name for cross-provider compatibility Match on r.name === repo.identifier instead of externalSlug because externalSlug varies by provider (GitLab uses a numeric project ID, VSTS uses external_id). Repository.name is consistently the full repo path across all providers. --- .../app/views/onboarding/components/useScmRepoSelection.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/static/app/views/onboarding/components/useScmRepoSelection.ts b/static/app/views/onboarding/components/useScmRepoSelection.ts index fff0cdde4d706e..5a0955dd73fe6d 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.ts +++ b/static/app/views/onboarding/components/useScmRepoSelection.ts @@ -78,7 +78,11 @@ export function useScmRepoSelection({ queryFn: fetchDataQuery, staleTime: 0, }); - const existing = matches?.find(r => r.externalSlug === repo.identifier); + // Match on name rather than externalSlug because externalSlug varies + // by provider (e.g. GitLab uses a numeric project ID) while name is + // consistently the full repo path (e.g. "getsentry/sentry") across + // all providers — matching repo.identifier from the search results. + const existing = matches?.find(r => r.name === repo.identifier); if (existing) { onSelect({...optimistic, ...existing}); From c9ae31ef04abf13f8a7e0e47062b9b9a357bba09 Mon Sep 17 00:00:00 2001 From: Jay Goss Date: Thu, 26 Mar 2026 19:31:33 -0500 Subject: [PATCH 10/14] docs(onboarding): Clarify repo matching comment --- .../views/onboarding/components/useScmRepoSelection.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/static/app/views/onboarding/components/useScmRepoSelection.ts b/static/app/views/onboarding/components/useScmRepoSelection.ts index 5a0955dd73fe6d..03505947802182 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.ts +++ b/static/app/views/onboarding/components/useScmRepoSelection.ts @@ -78,10 +78,11 @@ export function useScmRepoSelection({ queryFn: fetchDataQuery, staleTime: 0, }); - // Match on name rather than externalSlug because externalSlug varies - // by provider (e.g. GitLab uses a numeric project ID) while name is - // consistently the full repo path (e.g. "getsentry/sentry") across - // all providers — matching repo.identifier from the search results. + // Match on Repository.name === IntegrationRepository.identifier. + // This is the same comparison the backend uses in the search endpoint + // (organization_integration_repos.py) 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) { From cc9c3d0a4a609071bff64a7ba8b06b19835627cd Mon Sep 17 00:00:00 2001 From: Jay Goss Date: Thu, 26 Mar 2026 19:35:16 -0500 Subject: [PATCH 11/14] fix(onboarding): Show error message on repo selection failure --- static/app/views/onboarding/components/useScmRepoSelection.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/static/app/views/onboarding/components/useScmRepoSelection.ts b/static/app/views/onboarding/components/useScmRepoSelection.ts index 03505947802182..9704a80988445b 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.ts +++ b/static/app/views/onboarding/components/useScmRepoSelection.ts @@ -1,5 +1,7 @@ import {useState} from 'react'; +import {addErrorMessage} from 'sentry/actionCreators/indicator'; +import {t} from 'sentry/locale'; import type { Integration, IntegrationRepository, @@ -102,6 +104,7 @@ export function useScmRepoSelection({ }); onSelect({...optimistic, ...created}); } catch { + addErrorMessage(t('Failed to select repository')); onSelect(undefined); } finally { setBusy(false); From 3c25c5a9c14a4cd2ceb1238a473198778824b732 Mon Sep 17 00:00:00 2001 From: Jay Goss Date: Thu, 26 Mar 2026 19:42:44 -0500 Subject: [PATCH 12/14] docs(onboarding): Clarify query vs exact match in repo lookup comment --- .../onboarding/components/useScmRepoSelection.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/static/app/views/onboarding/components/useScmRepoSelection.ts b/static/app/views/onboarding/components/useScmRepoSelection.ts index 9704a80988445b..42274b637e99f4 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.ts +++ b/static/app/views/onboarding/components/useScmRepoSelection.ts @@ -80,11 +80,12 @@ export function useScmRepoSelection({ queryFn: fetchDataQuery, staleTime: 0, }); - // Match on Repository.name === IntegrationRepository.identifier. - // This is the same comparison the backend uses in the search endpoint - // (organization_integration_repos.py) to determine isInstalled. - // Can't use externalSlug because it varies by provider (e.g. GitLab - // returns a numeric project ID). + // 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) { From 0f97ab1fe6fad99a3faf071ec3e150b42c313bfb Mon Sep 17 00:00:00 2001 From: Jay Goss Date: Thu, 26 Mar 2026 19:45:54 -0500 Subject: [PATCH 13/14] test(onboarding): Fix lint error and improve repo selection test coverage Remove unnecessary async from handleRemove test to fix require-await lint error. Rename misleading test name to accurately reflect that only POST fails (GET succeeds with empty results). Add dedicated test for GET failure scenario. Simplify busy state test by removing waitFor dance that couldn't observe the intermediate state. --- .../components/useScmRepoSelection.spec.tsx | 39 +++++++++++++------ 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/static/app/views/onboarding/components/useScmRepoSelection.spec.tsx b/static/app/views/onboarding/components/useScmRepoSelection.spec.tsx index 46d5c989e64a8a..f8e827339c66a0 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.spec.tsx +++ b/static/app/views/onboarding/components/useScmRepoSelection.spec.tsx @@ -1,7 +1,7 @@ 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 type {IntegrationRepository} from 'sentry/types/integrations'; @@ -116,7 +116,7 @@ describe('useScmRepoSelection', () => { ); }); - it('reverts onSelect when both GET and POST fail', async () => { + it('reverts onSelect on POST failure', async () => { MockApiClient.addMockResponse({ url: `/organizations/${organization.slug}/repos/`, body: [], @@ -146,7 +146,30 @@ describe('useScmRepoSelection', () => { expect(onSelect).toHaveBeenCalledWith(undefined); }); - it('handleRemove calls onSelect with undefined', async () => { + it('reverts onSelect on GET failure', async () => { + MockApiClient.addMockResponse({ + url: `/organizations/${organization.slug}/repos/`, + statusCode: 500, + body: {detail: 'Internal Error'}, + }); + + const {result} = renderHookWithProviders( + () => + useScmRepoSelection({integration: mockIntegration, onSelect, reposByIdentifier}), + {organization} + ); + + await act(async () => { + await result.current.handleSelect({value: 'getsentry/sentry'}); + }); + + expect(onSelect).toHaveBeenCalledWith( + expect.objectContaining({id: '', name: 'sentry'}) + ); + expect(onSelect).toHaveBeenCalledWith(undefined); + }); + + it('handleRemove calls onSelect with undefined', () => { const {result} = renderHookWithProviders( () => useScmRepoSelection({integration: mockIntegration, onSelect, reposByIdentifier}), @@ -160,7 +183,7 @@ describe('useScmRepoSelection', () => { expect(onSelect).toHaveBeenCalledWith(undefined); }); - it('sets busy during selection and clears after', async () => { + it('clears busy state after selection completes', async () => { MockApiClient.addMockResponse({ url: `/organizations/${organization.slug}/repos/`, body: [ @@ -181,14 +204,8 @@ describe('useScmRepoSelection', () => { expect(result.current.busy).toBe(false); - let selectPromise: Promise; - act(() => { - selectPromise = result.current.handleSelect({value: 'getsentry/sentry'}); - }); - - await waitFor(() => expect(result.current.busy).toBe(false)); await act(async () => { - await selectPromise!; + await result.current.handleSelect({value: 'getsentry/sentry'}); }); expect(result.current.busy).toBe(false); From b289d7b44347cac37bc95a3db014d1bd299ff9f5 Mon Sep 17 00:00:00 2001 From: Jay Goss Date: Fri, 27 Mar 2026 12:58:54 -0500 Subject: [PATCH 14/14] fix(onboarding): Report repo selection errors to Sentry --- static/app/views/onboarding/components/useScmRepoSelection.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/static/app/views/onboarding/components/useScmRepoSelection.ts b/static/app/views/onboarding/components/useScmRepoSelection.ts index 42274b637e99f4..080661b063d958 100644 --- a/static/app/views/onboarding/components/useScmRepoSelection.ts +++ b/static/app/views/onboarding/components/useScmRepoSelection.ts @@ -1,4 +1,5 @@ import {useState} from 'react'; +import * as Sentry from '@sentry/react'; import {addErrorMessage} from 'sentry/actionCreators/indicator'; import {t} from 'sentry/locale'; @@ -104,7 +105,8 @@ export function useScmRepoSelection({ }, }); onSelect({...optimistic, ...created}); - } catch { + } catch (error) { + Sentry.captureException(error); addErrorMessage(t('Failed to select repository')); onSelect(undefined); } finally {