From c4181830fed1844876dbb87adf8b6d9a7b7b0ef4 Mon Sep 17 00:00:00 2001 From: Jacek Date: Tue, 13 Jan 2026 10:36:25 -0600 Subject: [PATCH 1/3] refactor(ui): replace flushSync with deferred promise pattern Replace the flushSync call in BaseRouter's baseNavigate with a deferred promise pattern. This achieves the same guarantee (re-render completes before returning control to the caller) without the performance penalty of synchronously flushing React's update queue. The new approach stores the resolve function in state alongside the new route parts, and an effect resolves the promise after React commits the state update. --- packages/ui/src/router/BaseRouter.tsx | 43 ++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/packages/ui/src/router/BaseRouter.tsx b/packages/ui/src/router/BaseRouter.tsx index eff1200e640..7c19a9ac8de 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,17 @@ import { match } from './pathToRegexp'; import { Route } from './Route'; import { RouteContext } from './RouteContext'; +type RouteParts = { + path: string; + queryString: string; +}; + +type PendingNavigation = { + routeParts: RouteParts; + result: unknown; + resolve: (value: unknown) => void; +}; + interface BaseRouterProps { basePath: string; startPath: string; @@ -44,10 +54,22 @@ 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 [pendingNavigation, setPendingNavigation] = React.useState(null); + + // Resolve pending navigation 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. + React.useEffect(() => { + if (pendingNavigation) { + pendingNavigation.resolve(pendingNavigation.result); + setPendingNavigation(null); + } + }, [pendingNavigation]); + const currentPath = routeParts.path; const currentQueryString = routeParts.queryString; const currentQueryParams = getQueryParams(routeParts.queryString); @@ -119,14 +141,19 @@ 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 }); + setPendingNavigation({ + routeParts: { path: toURL.pathname, queryString: toURL.search }, + result: internalNavRes, + resolve, + }); }); - return internalNavRes; }; return ( From 1de758247a2a22eae233cfdf6ff075483bc32964 Mon Sep 17 00:00:00 2001 From: Jacek Date: Wed, 21 Jan 2026 23:14:04 -0600 Subject: [PATCH 2/3] fix(ui): handle concurrent navigations in BaseRouter Use array for pending navigations to ensure all promises resolve when multiple baseNavigate calls occur before the effect runs. Addresses PR feedback about potential hanging promises. --- packages/ui/src/router/BaseRouter.tsx | 20 +- .../src/router/__tests__/BaseRouter.test.tsx | 182 ++++++++++++++++++ 2 files changed, 190 insertions(+), 12 deletions(-) create mode 100644 packages/ui/src/router/__tests__/BaseRouter.test.tsx diff --git a/packages/ui/src/router/BaseRouter.tsx b/packages/ui/src/router/BaseRouter.tsx index 7c19a9ac8de..10ac15a5f7b 100644 --- a/packages/ui/src/router/BaseRouter.tsx +++ b/packages/ui/src/router/BaseRouter.tsx @@ -16,7 +16,6 @@ type RouteParts = { }; type PendingNavigation = { - routeParts: RouteParts; result: unknown; resolve: (value: unknown) => void; }; @@ -58,17 +57,18 @@ export const BaseRouter = ({ path: getPath(), queryString: getQueryString(), }); - const [pendingNavigation, setPendingNavigation] = React.useState(null); + const [pendingNavigations, setPendingNavigations] = React.useState([]); - // Resolve pending navigation after React commits the state update. + // 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 (pendingNavigation) { - pendingNavigation.resolve(pendingNavigation.result); - setPendingNavigation(null); + if (pendingNavigations.length > 0) { + pendingNavigations.forEach(nav => nav.resolve(nav.result)); + setPendingNavigations([]); } - }, [pendingNavigation]); + }, [pendingNavigations]); const currentPath = routeParts.path; const currentQueryString = routeParts.queryString; @@ -148,11 +148,7 @@ export const BaseRouter = ({ // navigation state before setActive might emit. return new Promise(resolve => { setRouteParts({ path: toURL.pathname, queryString: toURL.search }); - setPendingNavigation({ - routeParts: { path: toURL.pathname, queryString: toURL.search }, - result: internalNavRes, - resolve, - }); + setPendingNavigations(prev => [...prev, { result: internalNavRes, resolve }]); }); }; 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..eef933beb2c --- /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.com/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]); + }); + }); +}); From 0c74b6cf40afc717e695ac3c4a13d39b06752d66 Mon Sep 17 00:00:00 2001 From: Jacek Date: Thu, 22 Jan 2026 07:58:18 -0600 Subject: [PATCH 3/3] test(ui): use .test TLD in router tests --- packages/ui/src/router/__tests__/BaseRouter.test.tsx | 2 +- packages/ui/src/router/__tests__/HashRouter.test.tsx | 6 +++--- packages/ui/src/router/__tests__/PathRouter.test.tsx | 6 +++--- packages/ui/src/router/__tests__/Switch.test.tsx | 12 ++++++------ .../ui/src/router/__tests__/VirtualRouter.test.tsx | 4 ++-- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/packages/ui/src/router/__tests__/BaseRouter.test.tsx b/packages/ui/src/router/__tests__/BaseRouter.test.tsx index eef933beb2c..6598aceb093 100644 --- a/packages/ui/src/router/__tests__/BaseRouter.test.tsx +++ b/packages/ui/src/router/__tests__/BaseRouter.test.tsx @@ -38,7 +38,7 @@ describe('BaseRouter', () => { describe('concurrent navigations', () => { beforeEach(() => { // @ts-ignore - window.location = new URL('https://www.example.com/virtual'); + window.location = new URL('https://www.example.test/virtual'); }); it('resolves all promises when multiple navigations occur before effect runs', async () => { 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 () => {