refactor(ui): derive ConfigureSSO test-run fetching from the connection state

Author: iagodahlem

packages/ui/src/components/ConfigureSSO/domain/__tests__/enterpriseConnectionState.test.ts

@@ -5,7 +5,7 @@ import type {
 } from '@clerk/shared/types';
 import { describe, expect, it } from 'vitest';
 
-import { deriveEnterpriseConnectionState } from '../enterpriseConnectionState';
+import { deriveEnterpriseConnectionState, isEnterpriseConnectionConfigured } from '../enterpriseConnectionState';
 
 const makeSamlConnection = (overrides: Partial<SamlAccountConnectionResource> = {}): SamlAccountConnectionResource =>
   ({
@@ -149,3 +149,32 @@ describe('deriveEnterpriseConnectionState', () => {
     expect(typeof state.hasSuccessfulTestRun).toBe('boolean');
   });
 });
+
+describe('isEnterpriseConnectionConfigured', () => {
+  it('undefined connection → false', () => {
+    expect(isEnterpriseConnectionConfigured(undefined)).toBe(false);
+  });
+  it('null connection → false', () => {
+    expect(isEnterpriseConnectionConfigured(null)).toBe(false);
+  });
+  it('no saml config → false', () => {
+    expect(isEnterpriseConnectionConfigured(makeConnection({ samlConnection: null }))).toBe(false);
+  });
+  it('empty saml config → false', () => {
+    expect(isEnterpriseConnectionConfigured(makeConnection({ samlConnection: makeSamlConnection() }))).toBe(false);
+  });
+  it('idpSsoUrl only → false', () => {
+    expect(
+      isEnterpriseConnectionConfigured(
+        makeConnection({ samlConnection: makeSamlConnection({ idpSsoUrl: 'https://idp.example.com/sso' }) }),
+      ),
+    ).toBe(false);
+  });
+  it('idpSsoUrl + idpEntityId present → true', () => {
+    expect(isEnterpriseConnectionConfigured(makeConnection({ samlConnection: fullyConfiguredSaml }))).toBe(true);
+  });
+  it('matches the derived `hasMinimumConfiguration` field', () => {
+    const connection = makeConnection({ samlConnection: fullyConfiguredSaml });
+    expect(isEnterpriseConnectionConfigured(connection)).toBe(derive({ connection }).hasMinimumConfiguration);
+  });
+});

packages/ui/src/components/ConfigureSSO/domain/enterpriseConnectionState.ts

@@ -67,6 +67,19 @@ export interface EnterpriseConnectionState {
   hasSuccessfulTestRun: boolean;
 }
 
+/**
+ * Whether the connection carries the minimum identity-provider configuration
+ * required to advance past the configure step (currently a SAML IdP SSO URL +
+ * entity ID). A pure predicate over the raw connection — it reads no derived
+ * state, so it is safe to use both inside {@link deriveEnterpriseConnectionState}
+ * and to gate the test-runs source upstream without a circular dependency on
+ * the derived snapshot.
+ */
+// TODO - Update to support OpenID Connect
+export const isEnterpriseConnectionConfigured = (
+  connection: EnterpriseConnectionResource | null | undefined,
+): boolean => Boolean(connection?.samlConnection?.idpSsoUrl && connection?.samlConnection?.idpEntityId);
+
 /**
  * Builds the {@link EnterpriseConnectionState} from the raw inputs gathered in
  * `useOrganizationEnterpriseConnection`. Pure: identical inputs yield identical
@@ -80,8 +93,7 @@ export const deriveEnterpriseConnectionState = ({
   provider: connection?.provider as ProviderType | undefined,
   hasConnection: Boolean(connection),
   isActive: Boolean(connection?.active),
-  // TODO - Update to support OpenID Connect
-  hasMinimumConfiguration: Boolean(connection?.samlConnection?.idpSsoUrl && connection?.samlConnection?.idpEntityId),
+  hasMinimumConfiguration: isEnterpriseConnectionConfigured(connection),
   isPrimaryEmailVerified: primaryEmail?.verification?.status === 'verified',
   hasSuccessfulTestRun,
 });

packages/ui/src/components/ConfigureSSO/hooks/__tests__/useOrganizationEnterpriseConnection.test.tsx

@@ -1,15 +1,31 @@
-import { act, renderHook } from '@testing-library/react';
+import { renderHook } from '@testing-library/react';
 import { beforeEach, describe, expect, it, vi } from 'vitest';
 
 // The umbrella hook composes several `@clerk/shared/react` hooks. We mock the
 // whole module so the test drives exactly what the source query and the
 // underlying test-runs query report, and can assert how the umbrella gates the
 // test-runs query's `enabled` and folds its loading into the global gate.
 
+// A connection shaped enough for the umbrella's gating: the derived
+// "configured" predicate reads `samlConnection.idpSsoUrl` + `idpEntityId`, and
+// the active path reads `active`.
+type MockConnection = {
+  id: string;
+  active?: boolean;
+  samlConnection?: { idpSsoUrl?: string; idpEntityId?: string } | null;
+};
+
+const configuredConnection = (id: string): MockConnection => ({
+  id,
+  samlConnection: { idpSsoUrl: 'https://idp.example.com/sso', idpEntityId: 'https://idp.example.com/entity' },
+});
+
+const unconfiguredConnection = (id: string): MockConnection => ({ id, samlConnection: null });
+
 // Mutable state the connection-source mock reads from, so a test can flip from
 // "no connection at load" to "connection created mid-flow" between renders.
 const connectionsState = vi.hoisted(() => ({
-  data: [] as Array<{ id: string }>,
+  data: [] as MockConnection[],
   isLoading: false,
 }));
 
@@ -63,21 +79,30 @@ beforeEach(() => {
 const lastTwoCalls = () => testRunsState.calls.slice(-2);
 
 describe('useOrganizationEnterpriseConnection — test-runs gating', () => {
-  it('(a) existing connection at load → test-runs active and contribute to the global isLoading', () => {
-    connectionsState.data = [{ id: 'ent_1' }];
+  it('(a) existing configured connection at load → test-runs active and contribute to the global isLoading', () => {
+    connectionsState.data = [configuredConnection('ent_1')];
     testRunsState.isLoading = true;
 
     const { result } = renderHook(() => useOrganizationEnterpriseConnection());
 
-    // Both underlying queries (probe + list) are enabled from the first render.
+    // A configured connection existed at load → both underlying queries (probe +
+    // list) are enabled from the first render.
     expect(lastTwoCalls()).toEqual([{ enabled: true }, { enabled: true }]);
-    // A connection existed at load, so the cold test-runs load gates the full
-    // skeleton.
+    // …so the cold test-runs load gates the full skeleton (hadInitialConnection).
     expect(result.current.isLoading).toBe(true);
     expect(result.current.testRuns.isLoading).toBe(true);
   });
 
-  it('(b) no connection at load → test-runs inactive after a mid-flow create, and never gate the global skeleton', () => {
+  it('(a2) active (but unconfigured) connection at load → test-runs active via the isActive path', () => {
+    connectionsState.data = [{ id: 'ent_active', active: true, samlConnection: null }];
+
+    const { result } = renderHook(() => useOrganizationEnterpriseConnection());
+
+    expect(lastTwoCalls()).toEqual([{ enabled: true }, { enabled: true }]);
+    expect(result.current.isLoading).toBe(false);
+  });
+
+  it('(b) fresh start: no connection → dormant; connection created but NOT yet configured → still dormant, never gating the global skeleton', () => {
     connectionsState.data = [];
     testRunsState.isLoading = true;
 
@@ -87,40 +112,41 @@ describe('useOrganizationEnterpriseConnection — test-runs gating', () => {
     expect(lastTwoCalls()).toEqual([{ enabled: false }, { enabled: false }]);
     expect(result.current.isLoading).toBe(false);
 
-    // A connection appears mid-flow (the create on the fresh-start path).
-    connectionsState.data = [{ id: 'ent_new' }];
+    // A connection appears mid-flow but is not yet configured (the create on the
+    // fresh-start path, before the configure step fills in the SAML fields).
+    connectionsState.data = [unconfiguredConnection('ent_new')];
     rerender();
 
-    // The query stays dormant — creating the connection must NOT fire it…
+    // The query stays dormant — creating an unconfigured connection must NOT
+    // fire it…
     expect(lastTwoCalls()).toEqual([{ enabled: false }, { enabled: false }]);
     // …and the global gate never folds in test-runs on this path, even though
     // the underlying flag would say loading were it enabled.
     expect(result.current.isLoading).toBe(false);
   });
 
-  it('(c) after Test-step activation → test-runs active, with loading at the table level (isFetching), not the global skeleton', () => {
+  it('(c) fresh start: once the connection is configured → test-runs active, with loading at the table level (isFetching), not the global skeleton', () => {
     connectionsState.data = [];
 
     const { result, rerender } = renderHook(() => useOrganizationEnterpriseConnection());
 
-    // Connection created mid-flow; still dormant until the Test step activates.
-    connectionsState.data = [{ id: 'ent_new' }];
+    // Connection created mid-flow but unconfigured → still dormant.
+    connectionsState.data = [unconfiguredConnection('ent_new')];
     rerender();
     expect(lastTwoCalls()).toEqual([{ enabled: false }, { enabled: false }]);
 
-    // The Test step calls `activate()` on entry. A background list fetch is now
+    // The configure step fills in the SAML fields. A background list fetch is now
     // in flight (table-level), but it is not a cold load.
     testRunsState.isFetching = true;
-    act(() => {
-      result.current.testRuns.activate();
-    });
+    connectionsState.data = [configuredConnection('ent_new')];
+    rerender();
 
-    // Now active → both queries fire.
+    // Now configured → both queries fire.
     expect(lastTwoCalls()).toEqual([{ enabled: true }, { enabled: true }]);
     // Loading is table-level only…
     expect(result.current.testRuns.isFetching).toBe(true);
     // …and the global skeleton stays down: no connection at initial load, so
-    // test-runs never gate it.
+    // test-runs never gate it (no flash on configure → test).
     expect(result.current.isLoading).toBe(false);
   });
 

packages/ui/src/components/ConfigureSSO/hooks/useEnterpriseConnectionTestRuns.ts

@@ -101,12 +101,14 @@ export interface EnterpriseConnectionTestRuns {
  * - `isFetching` — any in-flight *list* fetch, with previously-loaded rows kept
  *   visible (`keepPreviousData` semantics) → table-level loading on re-entry.
  *
- * `active` gates *both* underlying queries. The umbrella hook keeps it `true`
- * for the existing-connection-at-load case (so test-runs fetch on first load,
- * cover the full skeleton, and drive the `tested` guard) but `false` on the
- * fresh-start path until the user actually lands on the Test step — so merely
- * *creating* a connection mid-flow does not fire a test-runs fetch and flash
- * the global skeleton for a connection that provably has zero runs.
+ * `active` gates *both* underlying queries. The umbrella hook derives it
+ * straight from the connection — `true` once the connection is configured (or
+ * active), which is also when the Test step becomes reachable. So an existing,
+ * configured connection at load fetches test-runs on first load (covering the
+ * full skeleton and driving the `tested` guard), while on the fresh-start path
+ * a connection that is merely *created* but not yet configured stays `false` —
+ * no test-runs fetch fires to flash the global skeleton for a connection that
+ * provably has zero runs.
  */
 export const useEnterpriseConnectionTestRuns = (
   connection: EnterpriseConnectionResource | undefined,

packages/ui/src/components/ConfigureSSO/hooks/useOrganizationEnterpriseConnection.ts

@@ -12,9 +12,13 @@ import type {
   SignedInSessionResource,
   UserResource,
 } from '@clerk/shared/types';
-import { useCallback, useMemo, useRef, useState } from 'react';
+import { useMemo, useRef } from 'react';
 
-import { deriveEnterpriseConnectionState, type EnterpriseConnectionState } from '../domain/enterpriseConnectionState';
+import {
+  deriveEnterpriseConnectionState,
+  type EnterpriseConnectionState,
+  isEnterpriseConnectionConfigured,
+} from '../domain/enterpriseConnectionState';
 import {
   type EnterpriseConnectionMutations,
   useEnterpriseConnectionMutations,
@@ -28,7 +32,7 @@ export interface UseOrganizationEnterpriseConnectionResult {
    *
    * Test-runs contribute to this only when a connection was present at first
    * load (they're fetched as part of that initial load). On the fresh-start
-   * path they are dormant until the Test step activates them, so creating a
+   * path they stay dormant until the connection is configured, so creating a
    * connection mid-flow never re-flashes the global skeleton.
    */
   isLoading: boolean;
@@ -61,8 +65,8 @@ export interface UseOrganizationEnterpriseConnectionResult {
    * exists at initial load it's fetched as part of that load, so a cold landing
    * on the test step is covered by the full skeleton; re-entering the step later
    * refetches via {@link TestRunsView.refresh}. On the fresh-start path the
-   * queries stay dormant until {@link TestRunsView.activate} is called on Test
-   * step entry, after which loading is table-level only.
+   * queries stay dormant until the connection is configured, after which loading
+   * is table-level only.
    */
   testRuns: TestRunsView;
 }
@@ -109,17 +113,6 @@ export interface TestRunsView {
    * pass `{ armPolling: true }` after the user kicks off a run.
    */
   refresh: (options?: RefreshTestRunsOptions) => Promise<unknown>;
-  /**
-   * Activate the test-runs queries for the fresh-start path. When there was no
-   * connection at initial load the queries stay dormant (so creating a
-   * connection mid-flow doesn't fire a fetch and flash the global skeleton);
-   * the Test step calls this on entry to wake them, after which loading surfaces
-   * at the table level (`isFetching`) rather than as the full skeleton.
-   *
-   * No-op when a connection was present at initial load — the queries are
-   * already active there.
-   */
-  activate: () => void;
 }
 
 /**
@@ -157,17 +150,22 @@ export const useOrganizationEnterpriseConnection = (): UseOrganizationEnterprise
   }
   const hadInitialConnection = hadInitialConnectionRef.current === true;
 
-  // Set once when the user lands on the Test step (event-driven, never an
-  // effect that syncs to props). Used only on the fresh-start path to wake the
-  // test-runs queries that were kept dormant through create/configure.
-  const [testStepActivated, setTestStepActivated] = useState(false);
-  const activateTestStep = useCallback(() => setTestStepActivated(true), []);
-
-  // Existing connection at load → active immediately (fires on load, gates the
-  // initial skeleton, drives the `tested` guard). No connection at load →
-  // dormant until the Test step activates, so a mid-flow create does NOT fire
-  // the test-runs queries and flash the global skeleton.
-  const testRunsActive = hadInitialConnection || testStepActivated;
+  // The test-runs source is relevant exactly when the connection is configured —
+  // the same condition that makes the Test step reachable
+  // (`hasMinimumConfiguration || isActive`). Deriving activation straight from
+  // the connection drops the imperative activate-on-mount ceremony:
+  //   - existing connection at load → configured (or active) → fires on load,
+  //     gates the initial skeleton, drives the `tested` guard;
+  //   - fresh start with no connection, or a connection created but not yet
+  //     configured → dormant, so a mid-flow create does NOT fire the test-runs
+  //     queries and flash the global skeleton;
+  //   - configure completes (`hasMinimumConfiguration`) → fires, surfacing as
+  //     table-level loading because `hadInitialConnection` is false.
+  //
+  // Computed from the raw connection (not `connectionState`, which depends on
+  // the test-run probe below — reading it here would be circular).
+  const testRunsActive =
+    isEnterpriseConnectionConfigured(enterpriseConnection) || Boolean(enterpriseConnection?.active);
 
   const {
     hasSuccessfulTestRun,
@@ -204,7 +202,6 @@ export const useOrganizationEnterpriseConnection = (): UseOrganizationEnterprise
       page: testRunPage,
       setPage: setTestRunPage,
       refresh: refreshTestRuns,
-      activate: activateTestStep,
     }),
     [
       testRunRows,
@@ -215,7 +212,6 @@ export const useOrganizationEnterpriseConnection = (): UseOrganizationEnterprise
       testRunPage,
       setTestRunPage,
       refreshTestRuns,
-      activateTestStep,
     ],
   );
 
@@ -242,8 +238,9 @@ export const useOrganizationEnterpriseConnection = (): UseOrganizationEnterprise
     organization,
     // Test-runs gate the full skeleton only when a connection was present at
     // first load — that case fetches them as part of the initial load. On the
-    // fresh-start path they are dormant until the Test step activates them, and
-    // landing there shows table-level loading, never the global skeleton.
+    // fresh-start path they stay dormant until the connection is configured, and
+    // landing on the test step then shows table-level loading, never the global
+    // skeleton.
     isLoading: isLoadingEnterpriseConnections || (hadInitialConnection && isLoadingTestRuns),
     enterpriseConnection,
     primaryEmailAddress,

packages/ui/src/components/ConfigureSSO/steps/TestConfigurationStep.tsx

@@ -58,15 +58,12 @@ export const TestConfigurationStep = (): JSX.Element => {
     page: currentPage,
     setPage: setCurrentPage,
     refresh: refreshTestRuns,
-    activate: activateTestRuns,
   } = testRuns;
 
-  // Entering the test step wakes the test-runs source on the fresh-start path
-  // (no connection at initial load → the queries were kept dormant through
-  // create/configure so a mid-flow create never flashed the full skeleton).
-  // `activate()` is a no-op when a connection existed at initial load, where the
-  // queries already fetched on load. Once active, loading surfaces at the table
-  // level (`isFetching`), never the global skeleton.
+  // The test-runs source activates itself upstream the moment the connection is
+  // configured (the same condition that makes this step reachable), so the step
+  // no longer has to wake it on entry — by the time we land here it is already
+  // live.
   //
   // The wizard's initial load already fetched the test-runs (covered by the
   // full skeleton) for the existing-connection case, so landing on the test
@@ -77,11 +74,9 @@ export const TestConfigurationStep = (): JSX.Element => {
   //
   // The step only mounts while it is the current step, so this runs once per
   // entry. `isInitialStep` is the wizard's own "no navigation yet" signal, read
-  // at mount: this fires a mount-driven activation + data refresh, it is NOT
-  // syncing state via an effect. Empty deps → mount-only, so re-renders never
-  // re-fire.
+  // at mount: this fires a mount-driven data refresh, it is NOT syncing state
+  // via an effect. Empty deps → mount-only, so re-renders never re-fire.
   useEffect(() => {
-    activateTestRuns();
     if (!isInitialStep) {
       void refreshTestRuns();
     }