diff --git a/.changeset/quiet-auth-stability.md b/.changeset/quiet-auth-stability.md new file mode 100644 index 00000000..f6b9853d --- /dev/null +++ b/.changeset/quiet-auth-stability.md @@ -0,0 +1,5 @@ +--- +"@tailor-platform/app-shell": patch +--- + +Fixed a bug where placing a guard component between `AuthProvider` and `AppShell` could block `AppShell` from mounting, preventing the authentication flow from ever starting and leaving the app stuck in an unauthenticated state. diff --git a/packages/core/src/contexts/auth-context.test.tsx b/packages/core/src/contexts/auth-context.test.tsx index d558258d..f77e69b5 100644 --- a/packages/core/src/contexts/auth-context.test.tsx +++ b/packages/core/src/contexts/auth-context.test.tsx @@ -8,13 +8,21 @@ vi.mock("@tailor-platform/auth-public-client", () => ({ createAuthClient: vi.fn(), })); -import { AuthProvider, useAuth, useAuthSuspense, type EnhancedAuthClient } from "./auth-context"; -import { useRootRouteContext } from "./root-route-context"; +import { + createAuthClient, + AuthProvider, + useAuth, + useEnsureAuthInitialized, + useAuthSuspense, + type EnhancedAuthClient, +} from "./auth-context"; +import { createAuthClient as createAuthClientMock } from "@tailor-platform/auth-public-client"; afterEach(() => { cleanup(); vi.clearAllMocks(); vi.unstubAllGlobals(); + window.history.replaceState({}, "", "/"); }); const LoadingGuard = () =>
Loading...
; @@ -38,12 +46,27 @@ describe("AuthProvider", () => { isReady: false, }; + const baseHandleCallback = overrides?.handleCallback ?? vi.fn(); + let handleCallbackInFlight: Promise | null = null; + const handleCallback = vi.fn(() => { + if (handleCallbackInFlight) { + return handleCallbackInFlight; + } + + const callbackPromise = Promise.resolve(baseHandleCallback()).finally(() => { + handleCallbackInFlight = null; + }); + handleCallbackInFlight = callbackPromise; + return callbackPromise; + }); + + const { handleCallback: _ignoredHandleCallback, ...otherOverrides } = overrides ?? {}; + return { getState: vi.fn(() => state), login: vi.fn(), logout: vi.fn(), getAuthUrl: vi.fn(), - handleCallback: vi.fn(), checkAuthStatus: vi.fn().mockResolvedValue({ isAuthenticated: false, error: null, @@ -56,10 +79,138 @@ describe("AuthProvider", () => { getAuthHeaders: vi.fn(), fetch: vi.fn(), getAppUri: vi.fn(() => "https://api.test.com"), - ...overrides, + ...otherOverrides, + handleCallback, } as EnhancedAuthClient; }; + describe("useEnsureAuthInitialized", () => { + it("should initialize auth status on mount", async () => { + const state = { + isAuthenticated: false, + error: null, + isReady: false, + }; + const mockCheckAuthStatus = vi.fn().mockResolvedValue({ + isAuthenticated: true, + error: null, + isReady: true, + }); + + const mockClient = createMockAuthClient(state, { + checkAuthStatus: mockCheckAuthStatus, + }); + + const { result } = renderHook(() => useEnsureAuthInitialized(mockClient, "idle")); + + await act(async () => { + await result.current(); + }); + + expect(mockCheckAuthStatus).toHaveBeenCalledTimes(1); + }); + + it("should coalesce overlapping auth initialization checks", async () => { + const state = { + isAuthenticated: false, + error: null, + isReady: false, + }; + + let resolveCheckAuthStatus: (() => void) | undefined; + const mockCheckAuthStatus = vi.fn( + () => + new Promise<{ + isAuthenticated: boolean; + error: null; + isReady: true; + }>((resolve) => { + resolveCheckAuthStatus = () => + resolve({ + isAuthenticated: true, + error: null, + isReady: true, + }); + }), + ); + + const mockClient = createMockAuthClient(state, { + checkAuthStatus: mockCheckAuthStatus, + }); + + const { result } = renderHook(() => useEnsureAuthInitialized(mockClient, "idle")); + + const mountRetry = result.current(); + + await waitFor(() => { + expect(mockCheckAuthStatus).toHaveBeenCalledTimes(1); + }); + + const firstRetry = result.current(); + const secondRetry = result.current(); + + expect(mockCheckAuthStatus).toHaveBeenCalledTimes(1); + + resolveCheckAuthStatus?.(); + await Promise.all([mountRetry, firstRetry, secondRetry]); + }); + + it("should skip auth initialization while handling an OAuth callback", async () => { + window.history.replaceState({}, "", "/?code=auth-code-123&state=abc"); + + const state = { + isAuthenticated: false, + error: null, + isReady: false, + }; + const mockCheckAuthStatus = vi.fn().mockResolvedValue({ + isAuthenticated: false, + error: null, + isReady: true, + }); + + const mockClient = createMockAuthClient(state, { + checkAuthStatus: mockCheckAuthStatus, + }); + + const { result } = renderHook(() => useEnsureAuthInitialized(mockClient, "pending")); + + await act(async () => { + await result.current(); + }); + + expect(mockCheckAuthStatus).not.toHaveBeenCalled(); + }); + + it("should run auth initialization after callback is rejected", async () => { + window.history.replaceState({}, "", "/?code=auth-code-123&state=abc"); + + const state = { + isAuthenticated: false, + error: null, + isReady: false, + }; + const mockCheckAuthStatus = vi.fn().mockResolvedValue({ + isAuthenticated: false, + error: null, + isReady: true, + }); + + const mockClient = createMockAuthClient(state, { + checkAuthStatus: mockCheckAuthStatus, + }); + + // callbackStatus "rejected" means handleCallback failed — initialization should proceed + const { result } = renderHook(() => useEnsureAuthInitialized(mockClient, "rejected")); + + await act(async () => { + await result.current(); + }); + + expect(mockCheckAuthStatus).toHaveBeenCalledTimes(1); + }); + }); + describe("initial state", () => { it("should render children when not using guard component", () => { const mockClient = createMockAuthClient(); @@ -73,7 +224,7 @@ describe("AuthProvider", () => { expect(screen.getByText("Test Content")).toBeDefined(); }); - it("should always render children even with guardComponent when not ready", () => { + it("should show guard component when not ready", () => { const state = { isAuthenticated: false, error: null, @@ -87,11 +238,12 @@ describe("AuthProvider", () => { , ); - // AuthProvider always renders children; guard rendering is handled by the router - expect(screen.getByText("Protected Content")).toBeDefined(); + // Guard is now rendered directly by AuthProvider + expect(screen.getByText("Loading...")).toBeDefined(); + expect(screen.queryByText("Protected Content")).toBeNull(); }); - it("should always render children even with guardComponent when not authenticated", async () => { + it("should show guard component when not authenticated", async () => { const state = { isAuthenticated: false, error: null, @@ -105,8 +257,9 @@ describe("AuthProvider", () => { , ); - // AuthProvider always renders children; guard rendering is handled by the router - expect(screen.getByText("Protected Content")).toBeDefined(); + // Guard is now rendered directly by AuthProvider + expect(screen.getByText("Please log in")).toBeDefined(); + expect(screen.queryByText("Protected Content")).toBeNull(); }); it("should show children when authenticated", async () => { @@ -137,7 +290,7 @@ describe("AuthProvider", () => { }); describe("authentication flow", () => { - it("should check auth status via useRootRouteContext", async () => { + it("should call checkAuthStatus once on mount for non-callback URLs", async () => { const state = { isAuthenticated: false, error: null, @@ -153,38 +306,74 @@ describe("AuthProvider", () => { checkAuthStatus: mockCheckAuthStatus, }); - const { result } = renderHook(() => useRootRouteContext(), { - wrapper: ({ children }) => {children}, - }); + render( + +
Content
+
, + ); - expect(result.current).not.toBeNull(); - const response = await result.current!.loader(new URL("http://localhost/")); - expect(mockCheckAuthStatus).toHaveBeenCalled(); - expect(response).toBeNull(); + await waitFor(() => { + expect(mockCheckAuthStatus).toHaveBeenCalledTimes(1); + }); }); - it("should handle OAuth callback via useRootRouteContext when code is present", async () => { + it("should wait to render children until callback settles when set on client", async () => { + const mockHandleCallback = vi.fn().mockResolvedValue(undefined); + let callbackStatus: "idle" | "pending" | "resolved" | "rejected" = "pending"; + let callbackStatusListener: (() => void) | undefined; const state = { isAuthenticated: true, error: null, isReady: true, }; - const mockHandleCallback = vi.fn().mockResolvedValue(undefined); - const mockClient = createMockAuthClient(state, { - handleCallback: mockHandleCallback, + getCallbackStatusSnapshot: vi.fn(() => callbackStatus), + subscribeCallbackStatus: vi.fn((listener: () => void) => { + callbackStatusListener = listener; + return () => { + callbackStatusListener = undefined; + }; + }), }); - const { result } = renderHook(() => useRootRouteContext(), { - wrapper: ({ children }) => {children}, + render( + +
Content
+
, + ); + + expect(screen.queryByText("Content")).toBeNull(); + + await act(async () => { + await mockHandleCallback(); + callbackStatus = "resolved"; + callbackStatusListener?.(); }); - expect(result.current).not.toBeNull(); - const response = await result.current!.loader( - new URL("http://localhost/?code=auth-code-123"), - ); + expect(screen.getByText("Content")).toBeDefined(); expect(mockHandleCallback).toHaveBeenCalled(); - expect(response).toBeNull(); + }); + + it("should render guarded children while callback is pending", async () => { + const state = { + isAuthenticated: true, + error: null, + isReady: true, + }; + const mockClient = createMockAuthClient(state, { + getCallbackStatusSnapshot: vi.fn(() => "pending" as const), + }); + + render( + +
Protected Content
+
, + ); + + await waitFor(() => { + expect(screen.getByText("Protected Content")).toBeDefined(); + }); + expect(screen.queryByText("Please log in")).toBeNull(); }); it("should be authenticated when logged in", async () => { @@ -461,7 +650,7 @@ describe("AuthProvider", () => { }); describe("autoLogin", () => { - it("should not call login from the loader (autoLogin is handled by provider subscription)", async () => { + it("should not call login on initial render when autoLogin is enabled and auth is not yet ready", async () => { const state = { isAuthenticated: false, error: null, @@ -478,21 +667,17 @@ describe("AuthProvider", () => { checkAuthStatus: mockCheckAuthStatus, }); - const { result } = renderHook(() => useRootRouteContext(), { - wrapper: ({ children }) => ( - - {children} - - ), - }); + render( + +
Content
+
, + ); - expect(result.current).not.toBeNull(); - await result.current!.loader(new URL("http://localhost/")); - expect(mockCheckAuthStatus).toHaveBeenCalled(); - // autoLogin is no longer evaluated in the loader; it is handled - // reactively by the provider subscription so that it also covers - // mid-session expiry while idle on a page. + // login should not be called before the auth check completes expect(mockLogin).not.toHaveBeenCalled(); + await waitFor(() => { + expect(mockCheckAuthStatus).toHaveBeenCalled(); + }); }); it("should login when auth state changes to unauthenticated", async () => { @@ -597,6 +782,59 @@ describe("AuthProvider", () => { { timeout: 1000 }, ); }); + + it("should not login while the current URL is an OAuth callback", async () => { + window.history.replaceState({}, "", "/?code=auth-code-123&state=abc"); + + let authEventListener: ((event: { type: string; data?: unknown }) => void) | undefined; + + const mockAddEventListener = vi.fn( + (listener: (event: { type: string; data?: unknown }) => void) => { + authEventListener = listener; + return () => {}; + }, + ); + + let currentState = { + isAuthenticated: false, + error: null as string | null, + isReady: true, + }; + + const mockLogin = vi.fn().mockResolvedValue(undefined); + const mockClient = createMockAuthClient(undefined, { + login: mockLogin, + addEventListener: mockAddEventListener, + getState: vi.fn(() => currentState), + }); + + render( + +
Content
+
, + ); + + await act(async () => { + await Promise.resolve(); + }); + + expect(mockLogin).not.toHaveBeenCalled(); + + act(() => { + currentState = { + isAuthenticated: false, + error: null, + isReady: true, + }; + authEventListener?.({ type: "auth_state_changed", data: {} }); + }); + + await act(async () => { + await Promise.resolve(); + }); + + expect(mockLogin).not.toHaveBeenCalled(); + }); }); describe("event listeners", () => { @@ -683,3 +921,49 @@ describe("AuthProvider", () => { }); }); }); + +describe("createAuthClient", () => { + const makeBaseClient = (mockHandleCallback: ReturnType) => ({ + handleCallback: mockHandleCallback, + getState: vi.fn(() => ({ + isAuthenticated: false, + error: null, + isReady: false, + })), + login: vi.fn(), + logout: vi.fn(), + getAuthUrl: vi.fn(), + checkAuthStatus: vi.fn(), + refreshTokens: vi.fn(), + ready: vi.fn(() => Promise.resolve()), + configure: vi.fn(), + addEventListener: vi.fn(() => () => {}), + getAuthHeaders: vi.fn(), + fetch: vi.fn(), + getAuthHeadersForQuery: vi.fn(), + }); + + it("calls handleCallback immediately when URL contains OAuth callback parameters", () => { + window.history.replaceState({}, "", "/?code=auth-code-123"); + + const mockHandleCallback = vi.fn().mockResolvedValue(undefined); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + vi.mocked(createAuthClientMock).mockReturnValue(makeBaseClient(mockHandleCallback) as any); + + createAuthClient({ clientId: "test", appUri: "https://test.com" }); + + expect(mockHandleCallback).toHaveBeenCalledTimes(1); + }); + + it("does not call handleCallback when URL has no OAuth parameters", () => { + // URL is already "/" from afterEach reset + + const mockHandleCallback = vi.fn().mockResolvedValue(undefined); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + vi.mocked(createAuthClientMock).mockReturnValue(makeBaseClient(mockHandleCallback) as any); + + createAuthClient({ clientId: "test", appUri: "https://test.com" }); + + expect(mockHandleCallback).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/core/src/contexts/auth-context.tsx b/packages/core/src/contexts/auth-context.tsx index d5719a1f..2cb86b7d 100644 --- a/packages/core/src/contexts/auth-context.tsx +++ b/packages/core/src/contexts/auth-context.tsx @@ -3,6 +3,7 @@ import { useContext, useSyncExternalStore, useCallback, + useEffect, useMemo, useRef, } from "react"; @@ -10,7 +11,6 @@ import { createAuthClient as createAuthClientOriginal, type AuthClient, } from "@tailor-platform/auth-public-client"; -import { RootRouteContext } from "@/contexts/root-route-context"; // ============================================================================ // Auth Client @@ -28,6 +28,11 @@ export interface AuthClientConfig { redirectUri?: string; } +/** + * Internal type for tracking OAuth callback handling status. + */ +type CallbackStatus = "idle" | "pending" | "resolved" | "rejected"; + /** * Enhanced auth client with additional helper methods */ @@ -42,14 +47,79 @@ export interface EnhancedAuthClient extends AuthClient { * Same signature as the standard `fetch` API. */ fetch: AuthClient["fetch"]; + + /** + * Returns the current OAuth callback handling state. + * Provided automatically by clients created with {@link createAuthClient}. + * @internal + */ + getCallbackStatusSnapshot?(): CallbackStatus; + + /** + * Subscribe to callback settlement changes. + * Provided automatically by clients created with {@link createAuthClient}. + * @internal + */ + subscribeCallbackStatus?(listener: () => void): () => void; } +/** + * Small external-store manager dedicated to the OAuth callback lifecycle. + * + * AuthProvider reads this state via useSyncExternalStore so it can delay + * rendering while the login callback is still being processed, without + * coupling that control flow to the auth client's own auth_state_changed + * events. The manager exposes callback-specific transitions instead of a + * generic setter so createAuthClient can describe the callback flow directly. + */ +const createCallbackStatusManager = () => { + let callbackStatus: CallbackStatus = "idle"; + const CALLBACK_STATUS_CHANGE_EVENT = "callbackstatuschange"; + const target = new EventTarget(); + + const updateStatus = (nextStatus: CallbackStatus) => { + callbackStatus = nextStatus; + target.dispatchEvent(new Event(CALLBACK_STATUS_CHANGE_EVENT)); + }; + + return { + getSnapshot: () => callbackStatus, + start: () => { + updateStatus("pending"); + + return { + resolve: () => { + updateStatus("resolved"); + }, + reject: () => { + updateStatus("rejected"); + }, + }; + }, + subscribe: (listener: () => void) => { + const eventListener: EventListener = () => { + listener(); + }; + + target.addEventListener(CALLBACK_STATUS_CHANGE_EVENT, eventListener); + return () => { + target.removeEventListener(CALLBACK_STATUS_CHANGE_EVENT, eventListener); + }; + }, + }; +}; + /** * Create an enhanced authentication client. * * This wrapper around the original createAuthClient adds convenience methods * to reduce duplication of the appUri across your application. * + * **Side effect on call**: If the current URL contains OAuth callback parameters + * (`code` or `error`), this function immediately starts `handleCallback()` to + * exchange the authorization code. This happens at call time — before any React + * render — so it is safe to call at module scope. + * * @example * ```tsx * import { createAuthClient, AuthProvider } from '@tailor-platform/app-shell'; @@ -81,6 +151,26 @@ export interface EnhancedAuthClient extends AuthClient { export function createAuthClient(config: AuthClientConfig): EnhancedAuthClient { const baseClient = createAuthClientOriginal(config); const { appUri } = config; + const callbackManager = createCallbackStatusManager(); + + // Start OAuth callback handling immediately at module load time (before any + // React render). This is intentionally a side effect outside the React + // lifecycle — network I/O should not happen in the render phase. + if (typeof window !== "undefined") { + const currentUrl = new URL(window.location.href); + + if (isOAuthCallbackUrl(currentUrl)) { + const { resolve, reject } = callbackManager.start(); + + baseClient + .handleCallback() + .then(resolve) + .catch((error) => { + reject(); + console.error("Failed to handle OAuth callback:", error); + }); + } + } const enhancedClient: EnhancedAuthClient = { ...baseClient, @@ -88,6 +178,14 @@ export function createAuthClient(config: AuthClientConfig): EnhancedAuthClient { getAppUri(): string { return appUri; }, + + getCallbackStatusSnapshot() { + return callbackManager.getSnapshot(); + }, + + subscribeCallbackStatus(listener) { + return callbackManager.subscribe(listener); + }, }; return enhancedClient; @@ -160,6 +258,17 @@ type AuthContextType = { const AuthContext = createContext(null); +const isOAuthCallbackUrl = (url: URL) => + url.searchParams.has("code") || url.searchParams.has("error"); + +const isCurrentOAuthCallbackUrl = () => { + if (typeof window === "undefined") { + return false; + } + + return isOAuthCallbackUrl(new URL(window.location.href)); +}; + /** * Guard component that shows a fallback UI while auth is not ready or * not authenticated. Defined here so that the router layer does not @@ -173,6 +282,7 @@ const AuthGuard = ({ children: React.ReactNode; }) => { const { isReady, isAuthenticated } = useAuth(); + if (!isReady || !isAuthenticated) { return guardComponent(); } @@ -194,11 +304,11 @@ type AuthProviderProps = { /** * Guard UI component to show when loading or unauthenticated. * - * If not provided, children will be just rendered in any auth state. + * When provided, AuthProvider renders this component directly while auth + * is not ready or the user is not authenticated. Children are hidden until + * auth resolves to an authenticated state. * - * Note: This prop only takes effect when AuthProvider wraps an AppShell - * component. The guard is rendered by the router's root route element, - * so it requires RouterContainer to be present in the component tree. + * If not provided, children are rendered regardless of auth state. */ guardComponent?: () => React.ReactNode; }; @@ -221,6 +331,7 @@ const useAutoLogin = (props: { client: EnhancedAuthClient; enabled?: boolean }) const authState = props.client.getState(); if ( !props.enabled || + isCurrentOAuthCallbackUrl() || !authState.isReady || authState.isAuthenticated || loginInFlightRef.current @@ -230,6 +341,7 @@ const useAutoLogin = (props: { client: EnhancedAuthClient; enabled?: boolean }) loginInFlightRef.current = props.client .login() + .then(() => undefined) .catch((error) => { console.error("Failed to auto-login after session expiry:", error); }) @@ -262,6 +374,64 @@ const useAutoLogin = (props: { client: EnhancedAuthClient; enabled?: boolean }) }; }; +/** + * Builds a stable function that resolves the initial auth state once. + * + * AuthProvider uses the returned function from its own useEffect so the + * initialization flow stays visible at the call site, while overlapping + * checks still collapse into a single request. + */ +export const useEnsureAuthInitialized = ( + client: EnhancedAuthClient, + callbackStatus: CallbackStatus, +) => { + const initInFlightRef = useRef | null>(null); + + const ensureInitialized = useCallback(async (): Promise => { + const authState = client.getState(); + + // Skip initialization while a callback exchange is actively in progress. + // If the callback fails ("rejected"), we fall through so checkAuthStatus + // can still resolve the session instead of leaving isReady permanently false. + if (callbackStatus === "pending" || authState.isReady) { + return; + } + + if (initInFlightRef.current) { + return initInFlightRef.current; + } + + initInFlightRef.current = client + .checkAuthStatus() + .then(() => undefined) + .catch((error) => { + throw error; + }) + .finally(() => { + initInFlightRef.current = null; + }); + + return initInFlightRef.current; + }, [client, callbackStatus]); + + return ensureInitialized; +}; + +/** + * Reads the OAuth callback handling state from the auth client via + * useSyncExternalStore so AuthProvider can coordinate rendering while the + * callback exchange is still in flight. + */ +const useCallbackStatus = (client: EnhancedAuthClient): CallbackStatus => { + const subscribe = useCallback( + (notify: () => void) => client.subscribeCallbackStatus?.(notify) ?? (() => {}), + [client], + ); + const getSnapshot = useCallback(() => client.getCallbackStatusSnapshot?.() ?? "idle", [client]); + + return useSyncExternalStore(subscribe, getSnapshot); +}; + /** * Authentication provider component. * @@ -295,76 +465,52 @@ export const AuthProvider = (props: React.PropsWithChildren) enabled: props.autoLogin, }); - // Get current state snapshot (no cache needed - client returns same reference) + // Use useSyncExternalStore for state management from auth client. const getSnapshot = useCallback(() => client.getState(), [client]); - - // Use useSyncExternalStore for state management const authState = useSyncExternalStore(subscribeAuthState, getSnapshot); - // Build the root loader inside AuthProvider so that the router layer - // never needs to know about EnhancedAuthClient internals. - const authLoader = useCallback( - async (requestUrl: URL): Promise => { - // The "code" query parameter indicates a redirect back from the OAuth provider. - // handleCallback() internally cleans up the OAuth-related query parameters - // from the URL, so no additional URL cleanup is needed here. - if (requestUrl.searchParams.has("code")) { - try { - await client.handleCallback(); - } catch (error) { - console.error("Failed to handle callback:", error); - } - } - - // Only check auth status on first load (when isReady is false). - // Subsequent navigations skip this because the client already holds - // the cached auth state via useSyncExternalStore. - if (!client.getState().isReady) { - try { - await client.checkAuthStatus(); - } catch (error) { - // Intentionally swallow errors to avoid rendering the error boundary - // on transient failures (e.g. network timeouts). The next navigation - // will re-run this loader and retry automatically. - console.error("Failed to check auth status:", error); - } - } - - return null; - }, - [client], - ); - - const guardComponent = props.guardComponent; - const guardComponentWrapper = useMemo( - () => - guardComponent - ? (children: React.ReactNode) => ( - {children} - ) - : undefined, - [guardComponent], - ); - - const rootRouteCtxValue = useMemo( - () => ({ loader: authLoader, wrapComponent: guardComponentWrapper }), - [authLoader, guardComponentWrapper], + // Read callback status first so it can be passed to useEnsureAuthInitialized. + // This lets initialization retry automatically when a pending callback settles + // to rejected — the new ensureInitialized reference triggers the useEffect again. + const callbackStatus = useCallbackStatus(client); + + // Prepare a shared initialization function so AuthProvider can start the + // first auth check itself without depending on router navigation. + const ensureAuthInitialized = useEnsureAuthInitialized(client, callbackStatus); + + // AuthProvider owns the normal startup path: on mount, ask the auth client + // to resolve the current session so consumers can rely on authState even + // before any router loader has run. + useEffect(() => { + ensureAuthInitialized().catch((error) => { + console.error("Failed to check auth status:", error); + }); + }, [client, ensureAuthInitialized]); + + // While handling an OAuth callback, keep unguarded children hidden until + // the callback settles. Guarded trees already wait on auth state instead. + const resolvedChildren = + callbackStatus === "pending" && props.guardComponent == null ? null : props.children; + + const authContextValue = useMemo( + () => ({ + authState, + login: () => client.login(), + logout: () => client.logout(), + checkAuthStatus: () => client.checkAuthStatus(), + ready: () => client.ready(), + }), + [authState, client], ); return ( - - client.login(), - logout: () => client.logout(), - checkAuthStatus: () => client.checkAuthStatus(), - ready: () => client.ready(), - }} - > - {props.children} - - + + {props.guardComponent ? ( + {resolvedChildren} + ) : ( + resolvedChildren + )} + ); }; diff --git a/packages/core/src/contexts/root-route-context.tsx b/packages/core/src/contexts/root-route-context.tsx deleted file mode 100644 index 3e943309..00000000 --- a/packages/core/src/contexts/root-route-context.tsx +++ /dev/null @@ -1,55 +0,0 @@ -import { createContext, useContext } from "react"; - -/** @internal */ -export type RootRouteLoaderFn = (requestUrl: URL) => Promise; - -/** @internal */ -export type RootRouteContextType = { - /** - * Runs in the react-router loader phase (before rendering). - * Used for side effects such as OAuth callback handling and auth - * status checks. - * - * ``` - * loader runs ──▶ rendering starts ──▶ wrapComponent applied - * (pre-render) (React lifecycle) (render phase) - * ``` - */ - loader: RootRouteLoaderFn; - /** - * Wraps the root route element during the React render phase. - * Used for UI concerns such as showing a guard component while - * auth is loading or the user is unauthenticated. - * - * ``` - * loader runs ──▶ rendering starts ──▶ wrapComponent applied - * (pre-render) (React lifecycle) (render phase) - * ``` - */ - wrapComponent?: (children: React.ReactNode) => React.ReactNode; -}; - -/** - * Internal context that allows providers (e.g. AuthProvider) to inject - * a loader function and an optional element wrapper into the router's - * root route. The router does not need to know which provider supplied - * the value — it only consumes this interface. - * - * Currently the only provider is AuthProvider. If additional providers - * need to participate in root loading in the future, a composition - * mechanism (e.g. accepting an array of loaders, or merging multiple - * contexts) would need to be introduced, since a single React context - * only holds one value. - * @internal - */ -export const RootRouteContext = createContext(null); - -/** - * Internal hook consumed by RouterContainer to obtain the root route - * context. Returns null when no provider (e.g. AuthProvider) supplies - * a value, making the integration optional. - * @internal - */ -export const useRootRouteContext = (): RootRouteContextType | null => { - return useContext(RootRouteContext); -}; diff --git a/packages/core/src/routing/router.test.tsx b/packages/core/src/routing/router.test.tsx index 0fb7c3d7..afa313d0 100644 --- a/packages/core/src/routing/router.test.tsx +++ b/packages/core/src/routing/router.test.tsx @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { act, cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; import { RouterContainer } from "./router"; import { AppShellConfigContext, AppShellDataContext } from "@/contexts/appshell-context"; +import * as ReactRouter from "react-router"; import { Link, Outlet, useNavigate } from "react-router"; import { defineModule, @@ -17,11 +18,13 @@ import { AuthProvider, type EnhancedAuthClient } from "@/contexts/auth-context"; afterEach(() => { cleanup(); + window.history.replaceState({}, "", "/"); }); const renderWithConfig = ({ modules = [], basePath, + locale = "en", rootComponent, initialEntries, contextData = {}, @@ -31,6 +34,7 @@ const renderWithConfig = ({ }: { modules?: Array; basePath?: string; + locale?: string; rootComponent?: () => ReactNode; initialEntries: Array; contextData?: ContextData; @@ -43,7 +47,7 @@ const renderWithConfig = ({ settingsResources: [] as Array, basePath, errorBoundary: undefined, - locale: "en", + locale, }; setContextData(contextData); @@ -58,7 +62,7 @@ const renderWithConfig = ({ ); - render( + return render( authClient ? ( {tree} @@ -425,12 +429,14 @@ const createMockAuthClient = ( getAuthHeaders: vi.fn(), getAppUri: vi.fn(() => "https://api.test.com"), getAuthHeadersForQuery: vi.fn(), + getCallbackStatusSnapshot: vi.fn(() => "idle"), + subscribeCallbackStatus: vi.fn(() => () => {}), ...overrides, } as EnhancedAuthClient; }; describe("RouterContainer with AuthProvider", () => { - it("calls checkAuthStatus via loader on initial load", async () => { + it("does not call checkAuthStatus via loader on initial load", async () => { const mockCheckAuthStatus = vi.fn().mockResolvedValue({ isAuthenticated: true, error: null, @@ -455,33 +461,139 @@ describe("RouterContainer with AuthProvider", () => { }); await screen.findByText("Dashboard"); - expect(mockCheckAuthStatus).toHaveBeenCalled(); + expect(mockCheckAuthStatus).toHaveBeenCalledTimes(1); }); - it("calls handleCallback when OAuth code is present in URL", async () => { - const mockHandleCallback = vi.fn().mockResolvedValue(undefined); - const mockCheckAuthStatus = vi.fn().mockResolvedValue({ - isAuthenticated: true, - error: null, - isReady: true, - }); + it("waits to render until callback handling finishes", async () => { + let callbackStatus: "idle" | "pending" | "resolved" | "rejected" = "pending"; + let callbackStatusListener: (() => void) | undefined; const authClient = createMockAuthClient( { isAuthenticated: true, error: null, isReady: true }, { - handleCallback: mockHandleCallback, - checkAuthStatus: mockCheckAuthStatus, + getCallbackStatusSnapshot: vi.fn(() => callbackStatus), + subscribeCallbackStatus: vi.fn((listener: () => void) => { + callbackStatusListener = listener; + return () => { + callbackStatusListener = undefined; + }; + }), }, ); renderWithConfig({ modules: [], rootComponent: () =>
Home
, - initialEntries: ["/?code=auth-code-123&state=abc"], + initialEntries: ["/"], authClient, }); - await screen.findByText("Home"); - expect(mockHandleCallback).toHaveBeenCalled(); + expect(screen.queryByText("Home")).toBeNull(); + + await act(async () => { + callbackStatus = "resolved"; + callbackStatusListener?.(); + }); + + expect(await screen.findByText("Home")).toBeDefined(); + }); + + it("does not recreate the router when auth state changes", async () => { + let snapshot = { + isAuthenticated: false, + error: null as string | null, + isReady: false, + }; + const listeners: Array<(event: { type: string }) => void> = []; + const createMemoryRouterSpy = vi.spyOn(ReactRouter, "createMemoryRouter"); + + const authClient = createMockAuthClient(snapshot, { + getState: vi.fn(() => snapshot), + addEventListener: vi.fn((listener) => { + listeners.push(listener); + return () => { + const idx = listeners.indexOf(listener); + if (idx >= 0) listeners.splice(idx, 1); + }; + }), + }); + + try { + renderWithConfig({ + modules: [], + rootComponent: () =>
Home
, + initialEntries: ["/"], + authClient, + guardComponent: () =>
Loading...
, + }); + + expect(await screen.findByText("Loading...")).toBeDefined(); + // With guardComponent now applied in AuthProvider (not the router), + // RouterContainer is not mounted until auth is ready. + expect(createMemoryRouterSpy).toHaveBeenCalledTimes(0); + + act(() => { + snapshot = { + isAuthenticated: true, + error: null, + isReady: true, + }; + for (const listener of listeners) { + listener({ type: "auth_state_changed" }); + } + }); + + expect(await screen.findByText("Home")).toBeDefined(); + // Router is created once when children first render after auth is ready. + expect(createMemoryRouterSpy).toHaveBeenCalledTimes(1); + } finally { + createMemoryRouterSpy.mockRestore(); + } + }); + + it("recreates the router when route-defining config changes", async () => { + const createMemoryRouterSpy = vi.spyOn(ReactRouter, "createMemoryRouter"); + const module = defineModule({ + path: "dashboard", + component: () =>
Dashboard
, + meta: { title: "Dashboard" }, + resources: [], + }); + + try { + const view = renderWithConfig({ + modules: [module], + locale: "en", + initialEntries: ["/dashboard"], + }); + + expect(await screen.findByText("Dashboard")).toBeDefined(); + expect(createMemoryRouterSpy).toHaveBeenCalledTimes(1); + + view.rerender( + + + + + + + , + ); + + expect(await screen.findByText("Dashboard")).toBeDefined(); + expect(createMemoryRouterSpy).toHaveBeenCalledTimes(2); + } finally { + createMemoryRouterSpy.mockRestore(); + } }); it("calls login automatically when autoLogin is enabled and not authenticated", async () => { @@ -595,8 +707,8 @@ describe("RouterContainer with AuthProvider", () => { it("transitions from guard to children when auth state changes", async () => { // Mutable snapshot; initially not ready, not authenticated. - // The loader calls checkAuthStatus, but the mock does NOT update the - // snapshot during the loader — so the guard is shown on first render. + // Mount-time initialization runs outside the router loader, so the guard + // remains visible until the auth client publishes a ready state. let snapshot = { isAuthenticated: false, error: null as string | null, @@ -708,10 +820,19 @@ describe("RouterContainer with AuthProvider", () => { }); it("renders the correct page after OAuth callback with a specific path", async () => { - const mockHandleCallback = vi.fn().mockResolvedValue(undefined); + let callbackStatus: "idle" | "pending" | "resolved" | "rejected" = "pending"; + let callbackStatusListener: (() => void) | undefined; const authClient = createMockAuthClient( { isAuthenticated: true, error: null, isReady: true }, - { handleCallback: mockHandleCallback }, + { + getCallbackStatusSnapshot: vi.fn(() => callbackStatus), + subscribeCallbackStatus: vi.fn((listener: () => void) => { + callbackStatusListener = listener; + return () => { + callbackStatusListener = undefined; + }; + }), + }, ); renderWithConfig({ @@ -723,12 +844,16 @@ describe("RouterContainer with AuthProvider", () => { resources: [], }), ], - initialEntries: ["/dashboard?code=auth-code-123&state=abc"], + initialEntries: ["/dashboard"], authClient, }); + await act(async () => { + callbackStatus = "resolved"; + callbackStatusListener?.(); + }); + expect(await screen.findByText("Dashboard")).toBeDefined(); - expect(mockHandleCallback).toHaveBeenCalled(); }); it("applies both auth guard and route-level guards correctly", async () => { diff --git a/packages/core/src/routing/router.tsx b/packages/core/src/routing/router.tsx index f8bd875c..e5ee76bb 100644 --- a/packages/core/src/routing/router.tsx +++ b/packages/core/src/routing/router.tsx @@ -1,50 +1,31 @@ -import { type PropsWithChildren, type ReactNode } from "react"; +import { type PropsWithChildren, type ReactNode, useMemo } from "react"; import { Outlet, createMemoryRouter, createBrowserRouter, RouterProvider } from "react-router"; -import type { LoaderFunctionArgs, RouteObject } from "react-router"; +import type { RouteObject } from "react-router"; import { createContentRoutes, RootComponentOption, wrapErrorBoundary } from "./routes"; import { useAppShellConfig, type RootConfiguration } from "@/contexts/appshell-context"; import { createNavItemsLoader } from "@/routing/navigation"; import type { Guard } from "@/resource"; -import { useRootRouteContext, type RootRouteContextType } from "@/contexts/root-route-context"; // ============================================================================ // Root Route // ============================================================================ /** - * Create the root route that combines root loader, navigation loading, - * error boundary, and optional element wrapping into a single RouteObject. - * - * When AuthProvider wraps AppShell, rootRouteCtx provides: - * - loader: runs before rendering (e.g. OAuth callback, auth status check) - * - wrapComponent: wraps the root component (e.g. guard UI while loading) - * When AuthProvider is not used, rootRouteCtx is null and these are skipped. + * Create the root route that combines navigation loading and error boundary + * into a single RouteObject. */ const createRootRoute = (params: { configurations: RootConfiguration; - rootRouteCtx: RootRouteContextType | null; contentRoutes: Array; children: ReactNode; }): RouteObject => { - const { configurations, rootRouteCtx, contentRoutes, children } = params; + const { configurations, contentRoutes, children } = params; - // --- Loader: combine auth callback handling with navigation loading --- - const rootLoader = rootRouteCtx?.loader ?? null; - const { loaderID, loader: navLoader } = createNavItemsLoader({ + // --- Loader: load navigation items --- + const { loaderID, loader } = createNavItemsLoader({ modules: configurations.modules, locale: configurations.locale, }); - const loader = async (args: LoaderFunctionArgs) => { - if (rootLoader) { - const result = await rootLoader(new URL(args.request.url)); - if (result) return result; - } - return navLoader(); - }; - - // --- Element: apply wrapper when provided (e.g. auth guard) --- - const wrapComponent = rootRouteCtx?.wrapComponent; - const element = wrapComponent ? wrapComponent(children) : children; // --- Children: wrap with error boundary when configured --- const globalErrorBoundary = configurations.errorBoundary; @@ -61,7 +42,7 @@ const createRootRoute = (params: { return { id: loaderID, loader, - element, + element: children, children: routeChildren, // Hydration fallback is unused in CSR-only usage of AppShell. // Return null to silence hydration warnings. @@ -90,26 +71,47 @@ export type RouterContainerProps = export const RouterContainer = (props: PropsWithChildren) => { const { rootComponent, children } = props; const { configurations } = useAppShellConfig(); - const rootRouteCtx = useRootRouteContext(); - const contentRoutes = createContentRoutes({ - modules: configurations.modules, - settingsResources: configurations.settingsResources, - rootComponent, - rootGuards: props.rootGuards, - }); - const routes = [ - createRootRoute({ configurations, rootRouteCtx, contentRoutes, children }), - ] satisfies Array; + const contentRoutes = useMemo( + () => + createContentRoutes({ + modules: configurations.modules, + settingsResources: configurations.settingsResources, + rootComponent, + rootGuards: props.rootGuards, + }), + [configurations.modules, configurations.settingsResources, rootComponent, props.rootGuards], + ); + const routes = useMemo( + () => + [ + createRootRoute({ + configurations, + contentRoutes, + children, + }), + ] satisfies Array, + [configurations, contentRoutes, children], + ); const basename = configurations.basePath ? "/" + configurations.basePath : undefined; - const router = props.memory - ? createMemoryRouter(routes, { - basename, - ...(props.initialEntries ? { initialEntries: props.initialEntries } : {}), - }) - : createBrowserRouter(routes, { - basename, - }); + const initialEntries = props.memory ? props.initialEntries : undefined; + + // Keep the router instance stable across auth-driven rerenders. Recreating the + // router would re-run the root loader for the same location, which can cause + // OAuth callback URLs to be processed more than once. Still rebuild when + // route-defining inputs such as routes, basename, or initial entries change. + const router = useMemo( + () => + props.memory + ? createMemoryRouter(routes, { + basename, + ...(initialEntries ? { initialEntries } : {}), + }) + : createBrowserRouter(routes, { + basename, + }), + [basename, initialEntries, props.memory, routes], + ); return ; };