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

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);
   });
 });