Uh oh!
There was an error while loading. Please reload this page.
fix(ui): revalidate test runs before advancing from the Test step - #9046
Conversation
Continue on the Test step gated on a locally-cached success probe. A test run completed in a different browser tab left that probe stale, so Continue showed the "needs a successful test run" error and blocked until the user clicked "Refresh logs" first. Continue now revalidates the success probe and gates on the freshly fetched result, advancing as soon as a run that succeeded elsewhere is picked up. The button shows a loading state while revalidating and still surfaces the inline error when there is genuinely no successful run.
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe shared test-run hook now returns fresh page data from ChangesSSO test-run revalidation
Sequence Diagram(s)sequenceDiagram
participant User
participant ContinueTestSsoStepButton
participant useOrganizationEnterpriseConnection
participant useEnterpriseConnectionTestRuns
participant QueryClient
participant Wizard
User->>ContinueTestSsoStepButton: click Continue
ContinueTestSsoStepButton->>useOrganizationEnterpriseConnection: read testRuns.revalidateHasSuccessfulTestRun
useOrganizationEnterpriseConnection->>useEnterpriseConnectionTestRuns: call revalidateHasSuccessfulTestRun()
useEnterpriseConnectionTestRuns->>QueryClient: revalidateProbe({ armPolling: false, exact: true })
QueryClient-->>useEnterpriseConnectionTestRuns: fresh probe data
useEnterpriseConnectionTestRuns-->>ContinueTestSsoStepButton: boolean result
alt result is true
ContinueTestSsoStepButton->>Wizard: goNext()
else
ContinueTestSsoStepButton->>ContinueTestSsoStepButton: set no-success error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
@clerk/astro@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/electron@clerk/electron-passkeys@clerk/eslint-plugin@clerk/expo@clerk/expo-passkeys@clerk/express@clerk/fastify@clerk/hono@clerk/localizations@clerk/nextjs@clerk/nuxt@clerk/react@clerk/react-router@clerk/shared@clerk/tanstack-react-start@clerk/testing@clerk/ui@clerk/upgrade@clerk/vuecommit: |
API Changes Report
Summary
🔴 Breaking changes index (25)Every breaking change, up front. Full diffs are in the package sections below.
@clerk/sharedCurrent version: 4.22.1 Subpath |
a31554a to
da3f2fcCompare🦋 Changeset detectedLatest commit: 584c4df The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…nnection-hook imports/deps
The shared test-runs hook's revalidate() invalidated the broad org+connection key, which prefix-matches every test-runs query for the connection. On the SSO Test step that meant revalidating the success probe also refetched the visible test-runs list, spinning the "Refresh logs" button (bound to the list's isFetching) whenever the user clicked Continue. Add an `exact` option to revalidate() that invalidates only the query's own key, and use it for the Test step's probe-only revalidation on Continue. The default broad behavior is unchanged, so "Refresh logs" still refetches both the probe and the list.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In
`@packages/ui/src/components/ConfigureSSO/hooks/__tests__/useEnterpriseConnectionTestRuns.test.tsx`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e79adfb9-1707-4186-8b5e-82f102f91f01
📒 Files selected for processing (4)
packages/shared/src/react/hooks/__tests__/useOrganizationEnterpriseConnectionTestRuns.spec.tsxpackages/shared/src/react/hooks/useOrganizationEnterpriseConnectionTestRuns.tsxpackages/ui/src/components/ConfigureSSO/hooks/__tests__/useEnterpriseConnectionTestRuns.test.tsxpackages/ui/src/components/ConfigureSSO/hooks/useEnterpriseConnectionTestRuns.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/src/components/ConfigureSSO/hooks/useEnterpriseConnectionTestRuns.ts
- packages/shared/src/react/hooks/useOrganizationEnterpriseConnectionTestRuns.tsx
| const probeRevalidate = vi.fn(() => Promise.resolve({ data: [], totalCount: 0 })); | ||
| const listRevalidate = vi.fn(() => Promise.resolve({ data: [], totalCount: 0 })); |
There was a problem hiding this comment.
🎯 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
Uh oh!
There was an error while loading. Please reload this page.
What
On the Test step of the self-serve SSO configuration flow (
<ConfigureSSO />), Continue gated on the locally-cached success probe. If a successful test run completed in a different browser tab, the local probe was stale, so Continue showed "requires a successful test run" and blocked until the user clicked Refresh logs. Continue now revalidates the success probe and gates on the fresh result, so a run completed elsewhere is recognized without a manual refresh.If the local probe already shows success, Continue still advances immediately (no extra round-trip); the revalidate path only runs when the local state says "no success yet".
Notes
__internal_useOrganizationEnterpriseConnectionTestRunsrevalidatenow returns the freshly-fetched page (wasvoid). Internal (__internal_) API; the only consumer is@clerk/ui.Tests
Continue revalidates and advances when a successful run exists only server-side; a loading state on the button while the probe revalidates; the inline error when there is genuinely no successful run.
ORGS-1701
Summary by CodeRabbit
New Features
Bug Fixes