diff --git a/packages/ui/src/router/BaseRouter.tsx b/packages/ui/src/router/BaseRouter.tsx index eff1200e640..10ac15a5f7b 100644 --- a/packages/ui/src/router/BaseRouter.tsx +++ b/packages/ui/src/router/BaseRouter.tsx @@ -3,7 +3,6 @@ import { trimTrailingSlash } from '@clerk/shared/internal/clerk-js/url'; import { useClerk } from '@clerk/shared/react'; import type { NavigateOptions } from '@clerk/shared/types'; import React from 'react'; -import { flushSync } from 'react-dom'; import { useWindowEventListener } from '../hooks'; import { newPaths } from './newPaths'; @@ -11,6 +10,16 @@ import { match } from './pathToRegexp'; import { Route } from './Route'; import { RouteContext } from './RouteContext'; +type RouteParts = { + path: string; + queryString: string; +}; + +type PendingNavigation = { + result: unknown; + resolve: (value: unknown) => void; +}; + interface BaseRouterProps { basePath: string; startPath: string; @@ -44,10 +53,23 @@ export const BaseRouter = ({ // eslint-disable-next-line custom-rules/no-navigate-useClerk const { navigate: clerkNavigate } = useClerk(); - const [routeParts, setRouteParts] = React.useState({ + const [routeParts, setRouteParts] = React.useState({ path: getPath(), queryString: getQueryString(), }); + const [pendingNavigations, setPendingNavigations] = React.useState([]); + + // Resolve pending navigations after React commits the state update. + // This replaces flushSync by deferring the promise resolution to an effect, + // ensuring re-render completes before returning control to the caller. + // Using an array ensures multiple rapid navigations all resolve correctly. + React.useEffect(() => { + if (pendingNavigations.length > 0) { + pendingNavigations.forEach(nav => nav.resolve(nav.result)); + setPendingNavigations([]); + } + }, [pendingNavigations]); + const currentPath = routeParts.path; const currentQueryString = routeParts.queryString; const currentQueryParams = getQueryParams(routeParts.queryString); @@ -119,14 +141,15 @@ export const BaseRouter = ({ toURL.search = stringifyQueryParams(toQueryParams); } const internalNavRes = await internalNavigate(toURL, { metadata: { navigationType: 'internal' } }); - // We need to flushSync to guarantee the re-render happens before handing things back to the caller, - // otherwise setActive might emit, and children re-render with the old navigation state. - // An alternative solution here could be to return a deferred promise, set that to state together - // with the routeParts and resolve it in an effect. That way we could avoid the flushSync performance penalty. - flushSync(() => { + + // Use a deferred promise pattern instead of flushSync to guarantee the re-render + // happens before handing things back to the caller. This avoids the flushSync + // performance penalty while still ensuring children re-render with the new + // navigation state before setActive might emit. + return new Promise(resolve => { setRouteParts({ path: toURL.pathname, queryString: toURL.search }); + setPendingNavigations(prev => [...prev, { result: internalNavRes, resolve }]); }); - return internalNavRes; }; return ( diff --git a/packages/ui/src/router/__tests__/BaseRouter.test.tsx b/packages/ui/src/router/__tests__/BaseRouter.test.tsx new file mode 100644 index 00000000000..6598aceb093 --- /dev/null +++ b/packages/ui/src/router/__tests__/BaseRouter.test.tsx @@ -0,0 +1,182 @@ +import { act, render, screen, waitFor } from '@testing-library/react'; +import React from 'react'; +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { Route, useRouter, VirtualRouter } from '..'; + +const mockNavigate = vi.fn(); + +vi.mock('@clerk/shared/react', () => ({ + useClerk: () => ({ + navigate: vi.fn(to => { + mockNavigate(to); + if (to) { + // @ts-ignore + window.location = new URL(to, window.location.origin); + } + return Promise.resolve(); + }), + }), +})); + +describe('BaseRouter', () => { + const oldWindowLocation = window.location; + + beforeAll(() => { + // @ts-ignore + delete window.location; + }); + + afterAll(() => { + window.location = oldWindowLocation; + }); + + beforeEach(() => { + mockNavigate.mockReset(); + }); + + describe('concurrent navigations', () => { + beforeEach(() => { + // @ts-ignore + window.location = new URL('https://www.example.test/virtual'); + }); + + it('resolves all promises when multiple navigations occur before effect runs', async () => { + let routerRef: ReturnType | null = null; + + const CaptureRouter = () => { + routerRef = useRouter(); + return null; + }; + + render( + + + +
Index
+
+ +
Page 1
+
+ +
Page 2
+
+ +
Page 3
+
+
, + ); + + expect(screen.queryByText('Index')).toBeInTheDocument(); + expect(routerRef).not.toBeNull(); + + const results: string[] = []; + let promise1: Promise; + let promise2: Promise; + let promise3: Promise; + + // Trigger multiple navigations synchronously before React can run effects + act(() => { + promise1 = routerRef!.navigate('page1').then(() => { + results.push('nav1'); + }); + promise2 = routerRef!.navigate('page2').then(() => { + results.push('nav2'); + }); + promise3 = routerRef!.navigate('page3').then(() => { + results.push('nav3'); + }); + }); + + // All promises should resolve + await Promise.all([promise1!, promise2!, promise3!]); + + // All three navigations should have resolved + expect(results).toHaveLength(3); + expect(results).toContain('nav1'); + expect(results).toContain('nav2'); + expect(results).toContain('nav3'); + }); + + it('does not hang promises when rapid navigations occur', async () => { + let routerRef: ReturnType | null = null; + + const CaptureRouter = () => { + routerRef = useRouter(); + return null; + }; + + render( + + + + + +
Target
+
+
, + ); + + expect(routerRef).not.toBeNull(); + + // Create a race between navigation promise and a timeout + // If navigation hangs, the timeout will win and the test will fail + const navigationPromises: Promise[] = []; + + act(() => { + for (let i = 0; i < 5; i++) { + navigationPromises.push(routerRef!.navigate('target')); + } + }); + + // Use Promise.race to detect hanging promises + const timeoutPromise = new Promise((_, reject) => { + setTimeout(() => reject(new Error('Navigation promise timed out - likely hanging')), 1000); + }); + + // All navigations should resolve before the timeout + await expect(Promise.race([Promise.all(navigationPromises), timeoutPromise])).resolves.toBeDefined(); + }); + + it('resolves promises in order they were called', async () => { + let routerRef: ReturnType | null = null; + const resolveOrder: number[] = []; + + const CaptureRouter = () => { + routerRef = useRouter(); + return null; + }; + + render( + + + + + +
A
+
+ +
B
+
+
, + ); + + let promise1: Promise; + let promise2: Promise; + + act(() => { + promise1 = routerRef!.navigate('a').then(() => { + resolveOrder.push(1); + }); + promise2 = routerRef!.navigate('b').then(() => { + resolveOrder.push(2); + }); + }); + + await Promise.all([promise1!, promise2!]); + + // Promises should resolve in the order they were called + expect(resolveOrder).toEqual([1, 2]); + }); + }); +}); diff --git a/packages/ui/src/router/__tests__/HashRouter.test.tsx b/packages/ui/src/router/__tests__/HashRouter.test.tsx index ab6ee1c17a8..ba75950eaa8 100644 --- a/packages/ui/src/router/__tests__/HashRouter.test.tsx +++ b/packages/ui/src/router/__tests__/HashRouter.test.tsx @@ -69,7 +69,7 @@ describe('HashRouter', () => { describe('when hash has a path included in it', () => { beforeEach(() => { // @ts-ignore - window.location = new URL('https://www.example.com/hash#/foo'); + window.location = new URL('https://www.example.test/hash#/foo'); }); it('loads that path', () => { @@ -84,7 +84,7 @@ describe('HashRouter', () => { describe('when query has a preservedParam', () => { beforeEach(() => { // @ts-ignore - window.location = new URL('https://www.example.com/hash#/?preserved=1'); + window.location = new URL('https://www.example.test/hash#/?preserved=1'); }); it('preserves the param for internal navigation', async () => { @@ -103,7 +103,7 @@ describe('HashRouter', () => { const button = screen.getByRole('button', { name: /External/i }); await userEvent.click(button); - expect(mockNavigate).toHaveBeenNthCalledWith(1, 'https://www.example.com/external'); + expect(mockNavigate).toHaveBeenNthCalledWith(1, 'https://www.example.test/external'); }); }); }); diff --git a/packages/ui/src/router/__tests__/PathRouter.test.tsx b/packages/ui/src/router/__tests__/PathRouter.test.tsx index 5803c23d325..a3658c18695 100644 --- a/packages/ui/src/router/__tests__/PathRouter.test.tsx +++ b/packages/ui/src/router/__tests__/PathRouter.test.tsx @@ -69,7 +69,7 @@ describe('PathRouter', () => { describe('when hash has a path included in it', () => { beforeEach(() => { // @ts-ignore - window.location = new URL('https://www.example.com/foo#/bar'); + window.location = new URL('https://www.example.test/foo#/bar'); }); it('adds the hash path to the primary path', async () => { @@ -84,7 +84,7 @@ describe('PathRouter', () => { describe('when query has a preservedParam', () => { beforeEach(() => { // @ts-ignore - window.location = new URL('https://www.example.com/foo/bar?preserved=1'); + window.location = new URL('https://www.example.test/foo/bar?preserved=1'); }); it('preserves the param for internal navigation', async () => { @@ -102,7 +102,7 @@ describe('PathRouter', () => { const button = screen.getByRole('button', { name: /External/i }); await userEvent.click(button); - expect(mockNavigate).toHaveBeenNthCalledWith(1, 'https://www.example.com/'); + expect(mockNavigate).toHaveBeenNthCalledWith(1, 'https://www.example.test/'); }); }); }); diff --git a/packages/ui/src/router/__tests__/Switch.test.tsx b/packages/ui/src/router/__tests__/Switch.test.tsx index 1ebcdd0a7e3..34a7d2ea722 100644 --- a/packages/ui/src/router/__tests__/Switch.test.tsx +++ b/packages/ui/src/router/__tests__/Switch.test.tsx @@ -38,7 +38,7 @@ describe('', () => { }); it('ignores nodes that are not of type Route', () => { - setWindowOrigin('http://dashboard.example.com/#/first'); + setWindowOrigin('http://dashboard.example.test/#/first'); render( @@ -53,7 +53,7 @@ describe('', () => { }); it('renders only the first Route that matches', () => { - setWindowOrigin('http://dashboard.example.com/#/first'); + setWindowOrigin('http://dashboard.example.test/#/first'); render( @@ -70,7 +70,7 @@ describe('', () => { }); it('renders null if no route matches', () => { - setWindowOrigin('http://dashboard.example.com/#/cat'); + setWindowOrigin('http://dashboard.example.test/#/cat'); render( @@ -87,7 +87,7 @@ describe('', () => { }); it('always matches a Route without path', () => { - setWindowOrigin('http://dashboard.example.com/#/cat'); + setWindowOrigin('http://dashboard.example.test/#/cat'); render( @@ -104,7 +104,7 @@ describe('', () => { }); it('always matches a Route without path even when other routes match down the tree', () => { - setWindowOrigin('http://dashboard.example.com/#/first'); + setWindowOrigin('http://dashboard.example.test/#/first'); render( @@ -119,7 +119,7 @@ describe('', () => { }); it('always matches a Route without path even if its an index route', () => { - setWindowOrigin('http://dashboard.example.com/#/first'); + setWindowOrigin('http://dashboard.example.test/#/first'); render( diff --git a/packages/ui/src/router/__tests__/VirtualRouter.test.tsx b/packages/ui/src/router/__tests__/VirtualRouter.test.tsx index a9ecc9649e2..27af81ccd2f 100644 --- a/packages/ui/src/router/__tests__/VirtualRouter.test.tsx +++ b/packages/ui/src/router/__tests__/VirtualRouter.test.tsx @@ -74,7 +74,7 @@ describe('VirtualRouter', () => { describe('when mounted', () => { beforeEach(() => { // @ts-ignore - window.location = new URL('https://www.example.com/virtual'); + window.location = new URL('https://www.example.test/virtual'); }); it('it loads the start path', async () => { @@ -86,7 +86,7 @@ describe('VirtualRouter', () => { describe('when a preserved query param is created internally', () => { beforeEach(() => { // @ts-ignore - window.location = new URL('https://www.example.com/virtual'); + window.location = new URL('https://www.example.test/virtual'); }); it('preserves the param for internal navigation', async () => {