Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/lucky-pears-doubt.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@clerk/shared': patch
'@clerk/ui': patch
---

On the Test step of the self-serve SSO configuration flow, clicking Continue now re-checks for a successful test run before blocking, so a successful run completed in a separate browser tab is recognized without first clicking Refresh logs.
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import { act, renderHook, waitFor } from '@testing-library/react';
import { beforeEach, describe, expect, it, vi } from 'vitest';

import type { GetEnterpriseConnectionTestRunsParams } from '@/types/enterpriseConnectionTestRun';

import { INTERNAL_STABLE_KEYS } from '../../stable-keys';
import { createCacheKeys } from '../createCacheKeys';
import { __internal_useOrganizationEnterpriseConnectionTestRuns } from '../useOrganizationEnterpriseConnectionTestRuns';
import { createMockClerk, createMockQueryClient } from './mocks/clerk';
import { wrapper } from './wrapper';

// The success-filtered, single-row probe the Test step uses to answer
// `hasSuccessfulTestRun`. It is a sibling of the visible list page: both live
// under the same broad org+connection invalidation key, differing only in their
// fetch params (`untracked`).
const PROBE_PARAMS: GetEnterpriseConnectionTestRunsParams = { initialPage: 1, pageSize: 1, status: ['success'] };

const getTestRunsSpy = vi.fn(() => Promise.resolve({ data: [{ id: 'run_success' }], total_count: 1 }));

const defaultQueryClient = createMockQueryClient();

// Only `mock`-prefixed names may be referenced inside the hoisted `vi.mock`
// factory below, hence `mockClerk`.
const mockClerk = createMockClerk({
queryClient: defaultQueryClient,
__internal_lastEmittedResources: {
user: null,
session: null,
organization: { id: 'org_1', getEnterpriseConnectionTestRuns: getTestRunsSpy },
client: null,
},
});

vi.mock('../../contexts', () => ({
useAssertWrappedByClerkProvider: () => {},
useClerkInstanceContext: () => mockClerk,
useInitialStateContext: () => undefined,
}));

// The exact per-query key (includes the fetch params under `untracked`) vs the
// broad org+connection prefix shared by every test-runs query for the
// connection. `invalidateQueries` prefix-matches, so invalidating the broad key
// refetches the probe AND the visible list; the exact key hits only this query.
const { queryKey: exactKey, invalidationKey: broadKey } = createCacheKeys({
stablePrefix: INTERNAL_STABLE_KEYS.ORGANIZATION_ENTERPRISE_CONNECTION_TEST_RUNS_KEY,
authenticated: true,
tracked: { organizationId: 'org_1', enterpriseConnectionId: 'ent_1' },
untracked: { args: PROBE_PARAMS },
});

const renderProbe = () =>
renderHook(
() =>
__internal_useOrganizationEnterpriseConnectionTestRuns({
enterpriseConnectionId: 'ent_1',
params: PROBE_PARAMS,
enabled: true,
}),
{ wrapper },
);

describe('useOrganizationEnterpriseConnectionTestRuns — revalidate invalidation scope', () => {
beforeEach(() => {
vi.clearAllMocks();
defaultQueryClient.client.clear();
mockClerk.loaded = true;
});

it('revalidate({ exact: true }) invalidates ONLY the exact queryKey, never the broad org+connection key — so a sibling list query is left untouched', async () => {
const { result } = renderProbe();
await waitFor(() => expect(result.current.isLoading).toBe(false));

const invalidateSpy = vi.spyOn(defaultQueryClient.client, 'invalidateQueries');
await act(async () => {
await result.current.revalidate({ armPolling: false, exact: true });
});

expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: exactKey, exact: true });
// Never the broad prefix — invalidating it is exactly what would also
// refetch the visible list and spin its `isFetching`-bound "Refresh logs"
// button when the Test step revalidates the probe on Continue.
expect(invalidateSpy).not.toHaveBeenCalledWith({ queryKey: broadKey });

invalidateSpy.mockRestore();
});

it('revalidate() (default) keeps the broad org+connection invalidation so refresh()/"Refresh logs" still refetches the whole connection', async () => {
const { result } = renderProbe();
await waitFor(() => expect(result.current.isLoading).toBe(false));

const invalidateSpy = vi.spyOn(defaultQueryClient.client, 'invalidateQueries');
await act(async () => {
await result.current.revalidate({ armPolling: false });
});

expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: broadKey });
expect(invalidateSpy).not.toHaveBeenCalledWith({ queryKey: exactKey, exact: true });

invalidateSpy.mockRestore();
});
});
1 change: 1 addition & 0 deletions packages/shared/src/react/hooks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export type { UseOrganizationDomainsParams, UseOrganizationDomainsReturn } from
export { __internal_useOrganizationEnterpriseConnectionTestRuns } from './useOrganizationEnterpriseConnectionTestRuns';
export type {
UseOrganizationEnterpriseConnectionTestRunsParams,
UseOrganizationEnterpriseConnectionTestRunsRevalidateResult,
UseOrganizationEnterpriseConnectionTestRunsReturn,
} from './useOrganizationEnterpriseConnectionTestRuns';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,18 @@ export type UseOrganizationEnterpriseConnectionTestRunsParams = {
keepPreviousData?: boolean;
};

/**
* The freshly-fetched page surfaced by `revalidate`, read straight from the
* cache once the refetch settles. Lets a caller gate on the up-to-date result
* synchronously after `await`, instead of waiting for the hook to re-render with
* the new React state (whose value is still the pre-refetch one inside the
* caller's closure).
*/
export type UseOrganizationEnterpriseConnectionTestRunsRevalidateResult = {
data: EnterpriseConnectionTestRunResource[] | undefined;
totalCount: number | undefined;
};

export type UseOrganizationEnterpriseConnectionTestRunsReturn = {
data: EnterpriseConnectionTestRunResource[] | undefined;
totalCount: number | undefined;
Expand All @@ -55,7 +67,7 @@ export type UseOrganizationEnterpriseConnectionTestRunsReturn = {
*/
isPolling: boolean;
/**
* Force a refetch.
* Force a refetch, resolving with the freshly-fetched page once it settles.
*
* By default this also arms polling when the list is currently empty, so a run
* kicked off elsewhere is picked up as it lands. Pass `{ armPolling: false }`
Expand All @@ -64,7 +76,9 @@ export type UseOrganizationEnterpriseConnectionTestRunsReturn = {
* `revalidate()` (or `revalidate({ armPolling: true })`) after a run is kicked
* off.
*/
revalidate: (options?: RevalidateTestRunsOptions) => Promise<void>;
revalidate: (
options?: RevalidateTestRunsOptions,
) => Promise<UseOrganizationEnterpriseConnectionTestRunsRevalidateResult>;
};

export type RevalidateTestRunsOptions = {
Expand All @@ -75,6 +89,17 @@ export type RevalidateTestRunsOptions = {
* @default true
*/
armPolling?: boolean;
/**
* Invalidate only this query's exact `queryKey` instead of the broad
* org+connection `invalidationKey`. The default broad invalidation
* prefix-matches every test-runs query for the connection, so a sibling query
* (e.g. a success probe sharing the org+connection key with the visible list)
* refetches too. Pass `true` to refetch ONLY this query and leave the siblings
* — and their loading indicators — untouched.
*
* @default false
*/
exact?: boolean;
};

/**
Expand Down Expand Up @@ -149,7 +174,9 @@ function useOrganizationEnterpriseConnectionTestRuns(
}, [shouldPoll, hasRows]);

const revalidate = useCallback(
async (options?: RevalidateTestRunsOptions) => {
async (
options?: RevalidateTestRunsOptions,
): Promise<UseOrganizationEnterpriseConnectionTestRunsRevalidateResult> => {
// Arm polling only when the caller opts in (the default) AND there is
// nothing in the list yet. An entry/pagination refetch passes
// `armPolling: false` so an empty list on entry never arms polling on its
Expand All @@ -159,9 +186,30 @@ function useOrganizationEnterpriseConnectionTestRuns(
if (armPolling && !hasRows) {
setShouldPoll(true);
}
await queryClient.invalidateQueries({ queryKey: invalidationKey });
// `invalidateQueries` awaits the refetch it triggers, so by the time it
// resolves the cache already holds the fresh page. Read it back from the
// cache by the exact `queryKey` (not the broader `invalidationKey`) and
// resolve with it, so a caller can gate on the up-to-date result right
// after `await` — this hook's own `data` state is still the pre-refetch
// value inside the caller's closure until a re-render lands.
//
// `exact` scopes the invalidation: the broad `invalidationKey` (org +
// connection, no fetch params) prefix-matches every test-runs query for
// the connection, so it refetches this query AND its siblings (the success
// probe alongside the visible list). `exact: true` invalidates only this
// query's own `queryKey`, leaving sibling queries untouched.
if (options?.exact) {
await queryClient.invalidateQueries({ queryKey, exact: true });
} else {
await queryClient.invalidateQueries({ queryKey: invalidationKey });
}
const fresh = queryClient.getQueryData<{
data?: EnterpriseConnectionTestRunResource[];
total_count?: number;
}>(queryKey);
return { data: fresh?.data, totalCount: fresh?.total_count };
},
[queryClient, invalidationKey, hasRows],
[queryClient, invalidationKey, queryKey, hasRows],
);

const isPolling = queryEnabled && shouldPoll && !hasRows;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import type { EnterpriseConnectionResource } from '@clerk/shared/types';
import { act, renderHook } from '@testing-library/react';
import { beforeEach, describe, expect, it, vi } from 'vitest';

// The umbrella test-run hook composes the shared internal hook TWICE — once for
// the success probe (success-filtered, pageSize 1) and once for the visible list
// (pageSize 5). We mock the shared module so each gets its own `revalidate` spy,
// letting us assert exactly which query Continue's probe revalidation drives and
// with which options.
const probeRevalidate = vi.fn(() => Promise.resolve({ data: [], totalCount: 0 }));
const listRevalidate = vi.fn(() => Promise.resolve({ data: [], totalCount: 0 }));
Comment on lines +10 to +11

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Test never asserts the actual boolean result of revalidateHasSuccessfulTestRun().

The test only verifies that probeRevalidate was invoked with the right options — it never checks what revalidateHasSuccessfulTestRun() resolves to. Since the mock always returns { data: [], totalCount: 0 }, the boolean-derivation logic (the part that actually decides whether Continue can advance) is untested in both the "no successful run" and "successful run found" branches. Per the downstream snippet, TestConfigurationStep directly gates advance() on this resolved value, so an incorrect derivation here would silently break the revalidation feature.

As per coding guidelines, test files must "Verify proper error handling and edge cases" and tests should "Test component behavior, not implementation details" — this test currently only verifies implementation (spy calls), not the resulting behavior.

✅ Suggested additional coverage
 const probeRevalidate = vi.fn(() => Promise.resolve({ data: [], totalCount: 0 }));
 const listRevalidate = vi.fn(() => Promise.resolve({ data: [], totalCount: 0 }));
...
 describe('useEnterpriseConnectionTestRuns', () => {
   it('revalidateHasSuccessfulTestRun revalidates ONLY the probe, with exact invalidation', async () => {
     ...
   });
+
+  it('resolves true when the revalidated probe contains a successful run', async () => {
+    probeRevalidate.mockResolvedValueOnce({ data: [{ id: 'run_1', status: 'success' }], totalCount: 1 });
+    const { result } = renderHook(() => useEnterpriseConnectionTestRuns(connection, true));
+
+    let resolved: boolean | undefined;
+    await act(async () => {
+      resolved = await result.current.revalidateHasSuccessfulTestRun();
+    });
+
+    expect(resolved).toBe(true);
+  });
+
+  it('resolves false when the revalidated probe has no successful run', async () => {
+    const { result } = renderHook(() => useEnterpriseConnectionTestRuns(connection, true));
+
+    let resolved: boolean | undefined;
+    await act(async () => {
+      resolved = await result.current.revalidateHasSuccessfulTestRun();
+    });
+
+    expect(resolved).toBe(false);
+  });
 });

Also applies to: 40-54

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/ui/src/components/ConfigureSSO/hooks/__tests__/useEnterpriseConnectionTestRuns.test.tsx`
around lines 10 - 11, The test around revalidation is only asserting the spy
calls and never verifies the boolean returned by
revalidateHasSuccessfulTestRun(), so the logic that determines whether a
successful test run exists is untested. Update the
useEnterpriseConnectionTestRuns test to assert the resolved value from
revalidateHasSuccessfulTestRun() for both the no-success and success cases,
using the existing probeRevalidate and listRevalidate mocks to drive each
branch. Focus the assertions on the behavior exposed by
useEnterpriseConnectionTestRuns and TestConfigurationStep rather than only on
implementation details like invocation arguments.

Source: Coding guidelines


vi.mock('@clerk/shared/react', () => ({
__internal_useOrganizationEnterpriseConnectionTestRuns: (params: { params?: { status?: string[] } }) => {
// The probe is the only call that passes a `status` filter; the list omits
// it. Route each render's `revalidate` to the matching spy.
const isProbe = Array.isArray(params.params?.status);
return {
data: [],
totalCount: 0,
error: null,
isLoading: false,
isFetching: false,
isPolling: false,
revalidate: isProbe ? probeRevalidate : listRevalidate,
};
},
}));

import { useEnterpriseConnectionTestRuns } from '../useEnterpriseConnectionTestRuns';

const connection = { id: 'ent_1' } as EnterpriseConnectionResource;

beforeEach(() => {
probeRevalidate.mockClear();
listRevalidate.mockClear();
});

describe('useEnterpriseConnectionTestRuns', () => {
it('revalidateHasSuccessfulTestRun revalidates ONLY the probe, with exact invalidation — so Continue never refetches (or spins) the visible list', async () => {
const { result } = renderHook(() => useEnterpriseConnectionTestRuns(connection, true));

await act(async () => {
await result.current.revalidateHasSuccessfulTestRun();
});

// Continue's probe revalidation scopes invalidation to the probe's own query
// (`exact: true`) and never arms polling — so the list query, and its
// `isFetching`-bound "Refresh logs" spinner, stays idle.
expect(probeRevalidate).toHaveBeenCalledTimes(1);
expect(probeRevalidate).toHaveBeenCalledWith({ armPolling: false, exact: true });
expect(listRevalidate).not.toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@ export interface EnterpriseConnectionTestRuns {
* own. Pass `{ armPolling: true }` after the user kicks off a run.
*/
refresh: (options?: RefreshTestRunsOptions) => Promise<unknown>;
/**
* Revalidates ONLY the success probe and resolves with the freshly-fetched
* answer. Lets the Test step pick up a run completed in another tab the moment
* the user clicks Continue — gating on the fresh result rather than the stale
* `hasSuccessfulTestRun` captured at render — without a manual "Refresh logs".
*/
revalidateHasSuccessfulTestRun: () => Promise<boolean>;
}

/**
Expand Down Expand Up @@ -113,6 +120,17 @@ export const useEnterpriseConnectionTestRuns = (
[revalidateProbe, revalidateList],
);

const revalidateHasSuccessfulTestRun = useCallback(async () => {
// The probe is the success-filtered query, so a non-empty fresh page is the
// up-to-date "has a successful run" answer. `armPolling: false`: this is a
// one-shot check on Continue, never a polling arm. `exact: true`: invalidate
// only the probe's own query, never the broad org+connection key — so
// clicking Continue does not also refetch the visible list (which would spin
// the "Refresh logs" button bound to the list's `isFetching`).
const { data } = await revalidateProbe({ armPolling: false, exact: true });
return (data?.length ?? 0) > 0;
}, [revalidateProbe]);

return {
hasSuccessfulTestRun: (successfulTestRuns?.length ?? 0) > 0,
isLoading: isProbeLoading || isListLoading,
Expand All @@ -123,5 +141,6 @@ export const useEnterpriseConnectionTestRuns = (
page,
setPage,
refresh,
revalidateHasSuccessfulTestRun,
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ import type {
import { useCallback, useMemo, useRef } from 'react';

import {
organizationEnterpriseConnection as buildOrganizationEnterpriseConnection,
isEnterpriseConnectionConfigured,
type OrganizationEnterpriseConnection,
organizationEnterpriseConnection as buildOrganizationEnterpriseConnection,
} from '../domain/organizationEnterpriseConnection';
import type { ProviderType } from '../types';
import { type RefreshTestRunsOptions, useEnterpriseConnectionTestRuns } from './useEnterpriseConnectionTestRuns';
Expand Down Expand Up @@ -109,6 +109,11 @@ export interface TestRunsView {
setPage: (page: number) => void;
/** Pass `{ armPolling: true }` after the user kicks off a run. */
refresh: (options?: RefreshTestRunsOptions) => Promise<unknown>;
/**
* Revalidates the success probe and resolves with the fresh answer, so the
* Test step's Continue gate can pick up a run completed elsewhere on demand.
*/
revalidateHasSuccessfulTestRun: () => Promise<boolean>;
}

/**
Expand Down Expand Up @@ -173,6 +178,7 @@ export const useOrganizationEnterpriseConnection = (): UseOrganizationEnterprise
page: testRunPage,
setPage: setTestRunPage,
refresh: refreshTestRuns,
revalidateHasSuccessfulTestRun,
} = useEnterpriseConnectionTestRuns(enterpriseConnection, testRunsActive);

const { user } = useUser();
Expand Down Expand Up @@ -274,7 +280,6 @@ export const useOrganizationEnterpriseConnection = (): UseOrganizationEnterprise
createTestRun,
};
}, [
user,
organization,
organizationDomains,
enterpriseConnection,
Expand All @@ -293,6 +298,7 @@ export const useOrganizationEnterpriseConnection = (): UseOrganizationEnterprise
page: testRunPage,
setPage: setTestRunPage,
refresh: refreshTestRuns,
revalidateHasSuccessfulTestRun,
}),
[
testRunRows,
Expand All @@ -303,6 +309,7 @@ export const useOrganizationEnterpriseConnection = (): UseOrganizationEnterprise
testRunPage,
setTestRunPage,
refreshTestRuns,
revalidateHasSuccessfulTestRun,
],
);

Expand Down
Loading
Loading