fix(ui): fix brief flash of signin start before successful signin navigation

.changeset/quiet-clouds-flash.md

@@ -0,0 +1,5 @@
+---
+'@clerk/ui': patch
+---
+
+Fix rare condition where the start of the sign-in would briefly flash before the app rendered its signed-in state.

packages/ui/src/components/SignIn/SignInFactorOne.tsx

@@ -1,4 +1,3 @@
-import { useClerk } from '@clerk/shared/react';
 import type { SignInFactor } from '@clerk/shared/types';
 import React from 'react';
 
@@ -24,6 +23,7 @@ import type { PasswordErrorCode } from './SignInFactorOnePasswordCard';
 import { SignInFactorOnePasswordCard } from './SignInFactorOnePasswordCard';
 import { SignInFactorOnePhoneCodeCard } from './SignInFactorOnePhoneCodeCard';
 import { useResetPasswordFactor } from './useResetPasswordFactor';
+import { useSignInStepGuard } from './useSignInStepGuard';
 import { determineStartingSignInFactor, factorHasLocalStrategy } from './utils';
 
 const factorKey = (factor: SignInFactor | null | undefined) => {
@@ -75,7 +75,6 @@ function removeSignInResetPasswordIntentParam(): void {
 }
 
 function SignInFactorOneInternal(): JSX.Element {
-  const { __internal_setActiveInProgress } = useClerk();
   const signIn = useCoreSignIn();
   const { preferredSignInStrategy } = useEnvironment().displayConfig;
   const availableFactors = signIn.supportedFirstFactors;
@@ -123,18 +122,11 @@ function SignInFactorOneInternal(): JSX.Element {
 
   const [passwordErrorCode, setPasswordErrorCode] = React.useState<PasswordErrorCode | null>(null);
 
-  React.useEffect(() => {
-    if (__internal_setActiveInProgress) {
-      return;
-    }
-
-    // Handle the case where a user lands on alternative methods screen,
-    // clicks a social button but then navigates back to sign in.
-    // SignIn status resets to 'needs_identifier'
-    if (signIn.status === 'needs_identifier' || signIn.status === null) {
-      void router.navigate('../');
-    }
-  }, [__internal_setActiveInProgress]);
+  // A social flow returning to the component resets the sign-in to `needs_identifier`.
+  useSignInStepGuard({
+    redirectStatuses: ['needs_identifier'],
+    onLeave: () => void router.navigate('../'),
+  });
 
   if (!currentFactor) {
     return signIn.status ? (

packages/ui/src/components/SignIn/SignInFactorTwo.tsx

@@ -1,5 +1,4 @@
 import { useClerk } from '@clerk/shared/react';
-import React from 'react';
 
 import { withCardStateProvider } from '@/ui/elements/contexts';
 import { LoadingCard } from '@/ui/elements/LoadingCard';
@@ -14,6 +13,7 @@ import { SignInFactorTwoEmailLinkCard } from './SignInFactorTwoEmailLinkCard';
 import { SignInFactorTwoPhoneCodeCard } from './SignInFactorTwoPhoneCodeCard';
 import { SignInFactorTwoTOTPCard } from './SignInFactorTwoTOTPCard';
 import { useSecondFactorSelection } from './useSecondFactorSelection';
+import { useSignInStepGuard } from './useSignInStepGuard';
 
 function SignInFactorTwoInternal(): JSX.Element {
   const clerk = useClerk();
@@ -31,25 +31,22 @@ function SignInFactorTwoInternal(): JSX.Element {
   const onShowAlternativeMethodsClicked =
     signIn.supportedSecondFactors && signIn.supportedSecondFactors.length > 1 ? toggleAllStrategies : undefined;
 
-  React.useEffect(() => {
-    if (clerk.__internal_setActiveInProgress) {
-      return;
+  const leaveFactorTwo = () => {
+    // If the user is already signed in (e.g. multi-session app, page reload after
+    // successful verification), redirect forward to afterSignInUrl instead of
+    // back to sign-in start.
+    if (clerk.isSignedIn) {
+      void router.navigate(afterSignInUrl);
+    } else {
+      void router.navigate('../');
     }
+  };
 
-    // If the sign-in doesn't need second factor verification, redirect away.
-    // Don't redirect for 'complete' status - setActive will handle navigation.
-    if (signIn.status === null || signIn.status === 'needs_identifier' || signIn.status === 'needs_first_factor') {
-      // If the user is already signed in (e.g. multi-session app, page reload after
-      // successful verification), redirect forward to afterSignInUrl instead of
-      // back to sign-in start.
-      if (clerk.isSignedIn) {
-        void router.navigate(afterSignInUrl);
-      } else {
-        void router.navigate('../');
-      }
-    }
-    // eslint-disable-next-line react-hooks/exhaustive-deps -- Only run on mount and when setActiveInProgress changes
-  }, [clerk.__internal_setActiveInProgress]);
+  // Leave if the sign-in no longer needs second-factor verification.
+  useSignInStepGuard({
+    redirectStatuses: ['needs_identifier', 'needs_first_factor'],
+    onLeave: leaveFactorTwo,
+  });
 
   if (!currentFactor) {
     return <LoadingCard />;

packages/ui/src/components/SignIn/__tests__/SignInFactorOne.test.tsx

@@ -70,6 +70,61 @@ describe('SignInFactorOne', () => {
       render(<SignInFactorOne />, { wrapper });
       expect(fixtures.router.navigate).toHaveBeenCalledWith('../');
     });
+
+    it('does not navigate to the start card when it mounts while setActive is in progress', async () => {
+      const { wrapper, fixtures } = await createFixtures();
+      fixtures.clerk.__internal_setActiveInProgress = true;
+      render(<SignInFactorOne />, { wrapper });
+      expect(fixtures.router.navigate).not.toHaveBeenCalledWith('../');
+    });
+
+    it('navigates to the start card when a setActive running on mount finishes without restoring a sign-in', async () => {
+      const { wrapper, fixtures } = await createFixtures();
+      fixtures.clerk.__internal_setActiveInProgress = true;
+      render(<SignInFactorOne />, { wrapper });
+
+      expect(fixtures.router.navigate).not.toHaveBeenCalledWith('../');
+
+      fixtures.clerk.__internal_setActiveInProgress = false;
+
+      await waitFor(() => expect(fixtures.router.navigate).toHaveBeenCalledWith('../'), { timeout: 500 });
+    });
+
+    it('does not navigate to the start card when a setActive running on mount restores a sign-in', async () => {
+      const { wrapper, fixtures } = await createFixtures();
+      fixtures.clerk.__internal_setActiveInProgress = true;
+      render(<SignInFactorOne />, { wrapper });
+
+      fixtures.signIn.status = 'needs_first_factor';
+      fixtures.clerk.__internal_setActiveInProgress = false;
+
+      await act(async () => {
+        await new Promise(resolve => setTimeout(resolve, 100));
+      });
+
+      expect(fixtures.router.navigate).not.toHaveBeenCalledWith('../');
+    });
+
+    it('navigates to the start card when setActive completes and the sign-in was abandoned', async () => {
+      const { wrapper, fixtures } = await createFixtures(f => {
+        f.withMultiSessionMode();
+        f.withEmailAddress();
+        f.withPreferredSignInStrategy({ strategy: 'otp' });
+        f.startSignInWithEmailAddress({ supportEmailCode: true, supportPassword: false });
+      });
+      fixtures.signIn.prepareFirstFactor.mockReturnValueOnce(Promise.resolve({} as SignInResource));
+
+      const { rerender } = render(<SignInFactorOne />, { wrapper });
+
+      fixtures.clerk.__internal_setActiveInProgress = true;
+      rerender(<SignInFactorOne />);
+
+      fixtures.clerk.__internal_setActiveInProgress = false;
+      fixtures.signIn.status = 'needs_identifier';
+      rerender(<SignInFactorOne />);
+
+      expect(fixtures.router.navigate).toHaveBeenCalledWith('../');
+    });
   });
 
   describe('Submitting', () => {

packages/ui/src/components/SignIn/__tests__/SignInFactorTwo.test.tsx

@@ -3,7 +3,7 @@ import type { SignInResource } from '@clerk/shared/types';
 import { describe, expect, it, vi } from 'vitest';
 
 import { bindCreateFixtures } from '@/test/create-fixtures';
-import { render, screen, waitFor } from '@/test/utils';
+import { act, render, screen, waitFor } from '@/test/utils';
 
 import { SignInFactorTwo } from '../SignInFactorTwo';
 
@@ -18,8 +18,74 @@ describe('SignInFactorTwo', () => {
   });
 
   describe('Navigation', () => {
-    //This isn't yet implemented in the component
-    it.todo('navigates to SignInStart component if user lands on SignInFactorTwo page but they should not');
+    it('navigates to SignInStart if the user lands on SignInFactorTwo without a sign-in', async () => {
+      const { wrapper, fixtures } = await createFixtures();
+      render(<SignInFactorTwo />, { wrapper });
+      expect(fixtures.router.navigate).toHaveBeenCalledWith('../');
+    });
+
+    it('does not navigate to the start card when it mounts while setActive is in progress', async () => {
+      const { wrapper, fixtures } = await createFixtures();
+      fixtures.clerk.__internal_setActiveInProgress = true;
+      render(<SignInFactorTwo />, { wrapper });
+      expect(fixtures.router.navigate).not.toHaveBeenCalledWith('../');
+    });
+
+    it('navigates to the start card when a setActive running on mount finishes without restoring a sign-in', async () => {
+      const { wrapper, fixtures } = await createFixtures();
+      fixtures.clerk.__internal_setActiveInProgress = true;
+      render(<SignInFactorTwo />, { wrapper });
+
+      expect(fixtures.router.navigate).not.toHaveBeenCalledWith('../');
+
+      fixtures.clerk.__internal_setActiveInProgress = false;
+
+      await waitFor(() => expect(fixtures.router.navigate).toHaveBeenCalledWith('../'), { timeout: 500 });
+    });
+
+    it('does not navigate to the start card when a setActive running on mount restores a sign-in', async () => {
+      const { wrapper, fixtures } = await createFixtures();
+      fixtures.clerk.__internal_setActiveInProgress = true;
+      render(<SignInFactorTwo />, { wrapper });
+
+      fixtures.signIn.status = 'needs_second_factor';
+      fixtures.clerk.__internal_setActiveInProgress = false;
+
+      await act(async () => {
+        await new Promise(resolve => setTimeout(resolve, 100));
+      });
+
+      expect(fixtures.router.navigate).not.toHaveBeenCalledWith('../');
+    });
+
+    it('navigates to the start card when setActive completes and the sign-in returned to factor one', async () => {
+      const { wrapper, fixtures } = await createFixtures(f => {
+        f.withMultiSessionMode();
+        f.startSignInFactorTwo();
+      });
+      fixtures.signIn.prepareSecondFactor.mockReturnValueOnce(Promise.resolve({} as SignInResource));
+
+      const { rerender } = render(<SignInFactorTwo />, { wrapper });
+
+      fixtures.clerk.__internal_setActiveInProgress = true;
+      rerender(<SignInFactorTwo />);
+
+      fixtures.clerk.__internal_setActiveInProgress = false;
+      fixtures.signIn.status = 'needs_first_factor';
+      rerender(<SignInFactorTwo />);
+
+      expect(fixtures.router.navigate).toHaveBeenCalledWith('../');
+    });
+
+    it('navigates to afterSignInUrl when already signed in with a reset sign-in', async () => {
+      const { wrapper, fixtures, props } = await createFixtures(f => {
+        f.withMultiSessionMode();
+        f.withUser({ email_addresses: ['test@clerk.com'] });
+      });
+      props.setProps({ forceRedirectUrl: 'https://example.com/after-sign-in' });
+      render(<SignInFactorTwo />, { wrapper });
+      expect(fixtures.router.navigate).toHaveBeenCalledWith('https://example.com/after-sign-in');
+    });
   });
 
   describe('Submitting', () => {

packages/ui/src/components/SignIn/useSignInStepGuard.ts

@@ -0,0 +1,89 @@
+import { useClerk, useSafeLayoutEffect } from '@clerk/shared/react';
+import type { SignInResource } from '@clerk/shared/types';
+import React from 'react';
+
+import { useCoreSignIn } from '../../contexts';
+
+const SET_ACTIVE_POLL_INTERVAL_MS = 50;
+
+type UseSignInStepGuardParams = {
+  redirectStatuses: SignInResource['status'][];
+  onLeave: () => void;
+};
+
+/**
+ * This hook calls onLeave when signIn.status is either:
+ *   - null on mount
+ *   - One of redirectStatuses
+ *
+ * Additionally, if the status is null on mount, but __internal_setActiveInProgress
+ * is true, indicating a setActive is running, it does not call onLeave immediately,
+ * instead it starts a timer to poll the status of setActive (since it's not reactive).
+ *
+ * If setActive was called from this signIn process, it's going to result in a navigate
+ * which unmounts this component and cancels the timer. If it was an unrelated setActive,
+ * or the setActive fails, which does not lead to this component unmounting, and the
+ * status is still null after the setActive finishes, this hook calls onLeave.
+ *
+ * We need this complexity only because the logic is brittle to begin with, a better fix
+ * probably lies in rethinking setActive, but that's for another day.
+ */
+export function useSignInStepGuard({ redirectStatuses, onLeave }: UseSignInStepGuardParams): void {
+  const clerk = useClerk();
+  const signIn = useCoreSignIn();
+  const status = signIn.status;
+  const onLeaveRef = React.useRef(onLeave);
+  const redirectStatusesRef = React.useRef(redirectStatuses);
+
+  // We don't want the effects to be reactive to these values, think of this as
+  // a hacky useEffectEvent. The reason we use a layout effect over mutating in
+  // render is for better concurrency support
+  useSafeLayoutEffect(() => {
+    onLeaveRef.current = onLeave;
+    redirectStatusesRef.current = redirectStatuses;
+  });
+
+  React.useEffect(function leaveOnNoSignInStatus() {
+    if (status !== null) {
+      return;
+    }
+
+    // If the component remounts when setActive is in progress, we don't want
+    // to leave since that might bounce to the first page briefly before setActive
+    // navigates us correctly to post-signin, causing a quick flash of content.
+    if (!clerk.__internal_setActiveInProgress) {
+      onLeaveRef.current();
+      return;
+    }
+
+    // If the setActive that was running on mount never finishes in a way that
+    // closes the signIn, for example when it's unrelated to the signIn process,
+    // or fails, the component could get stuck in a loading state forever.
+    // This timer is there to prevent that.
+    const intervalId = setInterval(() => {
+      if (clerk.__internal_setActiveInProgress) {
+        return;
+      }
+
+      clearInterval(intervalId);
+      // Read directly from clerk to avoid reading stale state
+      if (clerk.client.signIn.status === null) {
+        onLeaveRef.current();
+      }
+    }, SET_ACTIVE_POLL_INTERVAL_MS);
+
+    return () => clearInterval(intervalId);
+    // `null` is also the post-setActive consumed state, so only handle it on mount.
+    // This is the reason we have two separate hooks.
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, []);
+
+  React.useEffect(
+    function leaveOnRedirectStatus() {
+      if (redirectStatusesRef.current.includes(status)) {
+        onLeaveRef.current();
+      }
+    },
+    [status],
+  );
+}