fix(*): address Protect timeout review feedback

Author: zourzouvillys

.changeset/protect-check-load-timeout.md

@@ -1,5 +1,6 @@
 ---
 '@clerk/clerk-js': patch
+'@clerk/react': patch
 '@clerk/shared': patch
 '@clerk/ui': patch
 ---

packages/clerk-js/src/core/__tests__/protectChallengeLoadTimeout.test.ts

@@ -63,15 +63,29 @@ describe('Protect.challengeLoadTimeoutMs', () => {
     expect(protect.challengeLoadTimeoutMs).toBe(25_000);
   });
 
-  it('ignores a non-positive or non-numeric value rather than passing it on', () => {
+  it('ignores a non-positive, non-finite, or non-numeric value rather than passing it on', () => {
     const protect = new Protect();
     protect.load(
       environment([
         loader({ challenge_load_timeout_ms: 0 }),
+        loader({ challenge_load_timeout_ms: Number.POSITIVE_INFINITY }),
         loader({ challenge_load_timeout_ms: 'soon' as unknown as number }),
       ]),
     );
 
     expect(protect.challengeLoadTimeoutMs).toBeUndefined();
   });
+
+  it('reports the first finite, positive value from the applied loaders', () => {
+    const protect = new Protect();
+    protect.load(
+      environment([
+        loader({ challenge_load_timeout_ms: 0 }),
+        loader({ challenge_load_timeout_ms: 25_000 }),
+        loader({ challenge_load_timeout_ms: 45_000 }),
+      ]),
+    );
+
+    expect(protect.challengeLoadTimeoutMs).toBe(25_000);
+  });
 });

packages/clerk-js/src/core/protect.ts

@@ -55,12 +55,9 @@ export class Protect {
     // loaders we are actually going to apply.
     const loaders = configured.filter(loader => isLoader(loader) && inRollout(loader));
 
-    // Read off the applied set, before any of them run: this only describes config, so a loader
-    // that later fails to be placed has still spoken for the browser it was assigned to.
-    //
-    // First one that asks for it wins, and the instance-wide value applies only when none does —
-    // the precedence is across the applied set rather than per loader, because there is no single
-    // "the" loader once more than one is live. Documented on the field.
+    // Load timeout applies to a single event and we only need one, so we pick the first valid one.
+    // This happens before applying the loaders since we want to use the value even if the loader
+    // later fails to apply.
     this.#challengeLoadTimeoutMs = loaders.find(
       loader =>
         typeof loader.challenge_load_timeout_ms === 'number' &&

packages/react/src/__tests__/isomorphicClerk.test.ts

@@ -73,6 +73,20 @@ describe('isomorphicClerk', () => {
     expect(isomorphicClerk.__internal_moduleManager).toBe(mm);
   });
 
+  it('exposes the inner clerk-js Protect challenge load timeout', () => {
+    const isomorphicClerk = new IsomorphicClerk({ publishableKey: 'pk_test_XXX' });
+
+    expect(isomorphicClerk.__internal_protectChallengeLoadTimeoutMs).toBeUndefined();
+
+    const innerClerk: any = {
+      addListener: vi.fn(),
+      __internal_protectChallengeLoadTimeoutMs: 25_000,
+    };
+    (isomorphicClerk as any).replayInterceptedInvocations(innerClerk);
+
+    expect(isomorphicClerk.__internal_protectChallengeLoadTimeoutMs).toBe(25_000);
+  });
+
   it('updates props asynchronously after clerkjs has loaded', async () => {
     const propsHistory: any[] = [];
     const dummyClerkJS = {

packages/react/src/isomorphicClerk.ts

@@ -302,6 +302,10 @@ export class IsomorphicClerk implements IsomorphicLoadedClerk {
     return this.clerkjs?.__internal_moduleManager;
   }
 
+  public get __internal_protectChallengeLoadTimeoutMs(): number | undefined {
+    return this.clerkjs?.__internal_protectChallengeLoadTimeoutMs;
+  }
+
   constructor(options: IsomorphicClerkOptions) {
     this.#publishableKey = options?.publishableKey;
     this.#proxyUrl = options?.proxyUrl;

packages/shared/src/types/protectConfig.ts

@@ -44,10 +44,11 @@ export interface ProtectLoader {
    * Per loader because loaders roll out gradually: while a new one ramps, two are live for the
    * same instance at once, and the new one may need a different value from the one it replaces.
    *
-   * Precedence is across the APPLIED SET, not per loader: the first applied loader that specifies
-   * one wins, and the instance-wide value applies only when none of them does. An instance running
-   * two loaders at once should therefore either set this on both or on neither — setting it on one
-   * makes it apply to browsers that got the other, which is rarely what is meant.
+   * Precedence is across the APPLIED SET, not per loader: the first applied loader with a finite,
+   * positive value wins, and the instance-wide value applies only when no applied loader specifies
+   * such a value. An instance running two loaders at once should therefore either set this on both
+   * or on neither — setting it on one makes it apply to browsers that got the other, which is rarely
+   * what is meant.
    */
   challenge_load_timeout_ms?: number;
 }

packages/ui/src/hooks/useProtectCheckRunner.ts

@@ -72,6 +72,7 @@ export function useProtectCheckRunner<TResource>(params: ProtectCheckRunnerParam
   // changing anything for browsers still on the loader it replaces. Undefined at both levels
   // leaves the SDK default in force. Read here rather than inside the effect so the effect keeps
   // depending on primitives.
+  // Older clerk-js versions omit this getter; undefined preserves the instance/default fallback.
   const loaderTimeoutMs = useClerk().__internal_protectChallengeLoadTimeoutMs;
   const instanceTimeoutMs = useEnvironment().protectConfig?.challenge_load_timeout_ms;
   const loadTimeoutMs = loaderTimeoutMs ?? instanceTimeoutMs;