fix(onboarding): Handle repo selection race with background link_all_repos (#111716)

Fix repo selection in the SCM onboarding connect step to handle the race
with the background `link_all_repos` task.

After a GitHub integration is installed, `link_all_repos` runs async and
registers all repos. The old approach cached existing repos on mount and
checked that cache before deciding to POST. This broke because the cache
was often stale (task still running) and the repos endpoint is paginated
(specific repo might not be on page 1).

New approach:
- GET first with a targeted query filtered by repo name (sidesteps
pagination)
- POST only if the repo doesn't exist yet

The old code also hid (soft-deleted) repo records when the user
deselected or switched repos. This made sense when repos were manually
registered one at a time, but with `link_all_repos` auto-registering
everything, hiding repos on deselect was counterproductive -- it removed
records the background task created, causing them to disappear from the
integration settings page. Repo selection is now just a context pointer
with no side effects on the repo records themselves.

Also switches to `queryClient.fetchQuery` + `fetchDataQuery` for the
imperative GET instead of deprecated `Client.requestPromise`.

Replaces #111699 (closed due to CODEOWNERS auto-review noise from base
change).

Author: jaydgoss

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 (
-      <OnboardingContextProvider initialValue={initialState}>
-        {children}
-      </OnboardingContextProvider>
-    );
-  };
-}
-
 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,171 +110,104 @@ 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'},
     });
 
     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);
   });
 });