Skip to content

Commit c86f5a3

Browse files
brkalowclaude
andauthored
fix(ui): Guard router refresh against out-of-basePath navigations (#7965)
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 3c35688 commit c86f5a3

5 files changed

Lines changed: 223 additions & 7 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@clerk/ui': patch
3+
---
4+
5+
Fix router updating state when navigating outside of the specified basePath, which caused components like SignIn to re-render and trigger catch-all redirects.

packages/ui/src/router/BaseRouter.tsx

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { getQueryParams, stringifyQueryParams } from '@clerk/shared/internal/clerk-js/querystring';
22
import { trimTrailingSlash } from '@clerk/shared/internal/clerk-js/url';
33
import { useClerk } from '@clerk/shared/react';
4+
import { withLeadingSlash } from '@clerk/shared/url';
45
import type { NavigateOptions } from '@clerk/shared/types';
56
import React from 'react';
67
import { flushSync } from 'react-dom';
@@ -13,7 +14,7 @@ import { RouteContext } from './RouteContext';
1314
// Custom events that don't exist on WindowEventMap but are handled
1415
// by wrapping history.pushState/replaceState in the fallback path.
1516
type HistoryEvent = 'pushstate' | 'replacestate';
16-
type RefreshEvent = keyof WindowEventMap | HistoryEvent;
17+
export type RefreshEvent = keyof WindowEventMap | HistoryEvent;
1718
type NavigationType = 'push' | 'replace' | 'traverse';
1819

1920
const isWindowRefreshEvent = (event: RefreshEvent): event is keyof WindowEventMap => {
@@ -160,6 +161,8 @@ export const BaseRouter = ({
160161
// eslint-disable-next-line custom-rules/no-navigate-useClerk
161162
const { navigate: clerkNavigate } = useClerk();
162163

164+
const normalizedBasePath = withLeadingSlash(basePath);
165+
163166
const [routeParts, setRouteParts] = React.useState({
164167
path: getPath(),
165168
queryString: getQueryString(),
@@ -196,13 +199,17 @@ export const BaseRouter = ({
196199
const newPath = getPath();
197200
const newQueryString = getQueryString();
198201

202+
if (basePath && !newPath.startsWith(normalizedBasePath)) {
203+
return;
204+
}
205+
199206
if (newPath !== currentPath || newQueryString !== currentQueryString) {
200207
setRouteParts({
201208
path: newPath,
202209
queryString: newQueryString,
203210
});
204211
}
205-
}, [currentPath, currentQueryString, getPath, getQueryString]);
212+
}, [basePath, normalizedBasePath, currentPath, currentQueryString, getPath, getQueryString]);
206213

207214
// Suppresses the history observer during baseNavigate's internal navigation.
208215
// Without this, the observer's microtask triggers a render before setActive's
@@ -214,11 +221,11 @@ export const BaseRouter = ({
214221
return;
215222
}
216223
const newPath = getPath();
217-
if (basePath && !newPath.startsWith('/' + basePath)) {
224+
if (basePath && !newPath.startsWith(normalizedBasePath)) {
218225
return;
219226
}
220227
refresh();
221-
}, [basePath, getPath, refresh]);
228+
}, [basePath, normalizedBasePath, getPath, refresh]);
222229

223230
useHistoryChangeObserver(refreshEvents, observerRefresh);
224231

@@ -229,7 +236,7 @@ export const BaseRouter = ({
229236
}
230237

231238
const isCrossOrigin = toURL.origin !== window.location.origin;
232-
const isOutsideOfUIComponent = !toURL.pathname.startsWith('/' + basePath);
239+
const isOutsideOfUIComponent = !toURL.pathname.startsWith(normalizedBasePath);
233240

234241
if (isOutsideOfUIComponent || isCrossOrigin) {
235242
isNavigatingRef.current = true;

packages/ui/src/router/HashRouter.tsx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
import { hasUrlInFragment, stripOrigin } from '@clerk/shared/internal/clerk-js/url';
22
import React from 'react';
33

4+
import type { RefreshEvent } from './BaseRouter';
45
import { BaseRouter } from './BaseRouter';
56

67
export const hashRouterBase = 'CLERK-ROUTER/HASH';
78

9+
const HASH_REFRESH_EVENTS: RefreshEvent[] = ['pushstate', 'replacestate', 'popstate', 'hashchange'];
10+
811
interface HashRouterProps {
912
preservedParams?: string[];
1013
children: React.ReactNode;
@@ -44,7 +47,7 @@ export const HashRouter = ({ preservedParams, children }: HashRouterProps): JSX.
4447
startPath={''}
4548
getQueryString={getQueryString}
4649
internalNavigate={internalNavigate}
47-
refreshEvents={['pushstate', 'replacestate', 'popstate', 'hashchange']}
50+
refreshEvents={HASH_REFRESH_EVENTS}
4851
preservedParams={preservedParams}
4952
>
5053
{children}

packages/ui/src/router/PathRouter.tsx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,11 @@ import { useClerk } from '@clerk/shared/react';
33
import type { NavigateOptions } from '@clerk/shared/types';
44
import React from 'react';
55

6+
import type { RefreshEvent } from './BaseRouter';
67
import { BaseRouter } from './BaseRouter';
78

9+
const PATH_REFRESH_EVENTS: RefreshEvent[] = ['pushstate', 'replacestate', 'popstate'];
10+
811
interface PathRouterProps {
912
basePath: string;
1013
preservedParams?: string[];
@@ -59,7 +62,7 @@ export const PathRouter = ({ basePath, preservedParams, children }: PathRouterPr
5962
getPath={getPath}
6063
getQueryString={getQueryString}
6164
internalNavigate={internalNavigate}
62-
refreshEvents={['pushstate', 'replacestate', 'popstate']}
65+
refreshEvents={PATH_REFRESH_EVENTS}
6366
preservedParams={preservedParams}
6467
>
6568
{children}
Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
import type { Clerk } from '@clerk/shared/types';
2+
import { act, render, screen } from '@testing-library/react';
3+
import React from 'react';
4+
import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest';
5+
6+
import { PathRouter, Route, Switch, useRouter } from '..';
7+
8+
const mockNavigate = vi.fn();
9+
10+
vi.mock('@clerk/shared/react', () => {
11+
return {
12+
useClerk: () => {
13+
return {
14+
navigate: (to: string) => {
15+
mockNavigate(to);
16+
if (to) {
17+
// @ts-ignore
18+
window.location = new URL(to, window.location.origin);
19+
}
20+
return Promise.resolve();
21+
},
22+
} as Clerk;
23+
},
24+
};
25+
});
26+
27+
/**
28+
* Renders router.currentPath so tests can assert against it.
29+
*/
30+
const CurrentPath = () => {
31+
const router = useRouter();
32+
return <div data-testid='current-path'>{router.currentPath}</div>;
33+
};
34+
35+
const oldWindowLocation = window.location;
36+
37+
const setWindowLocation = (url: string) => {
38+
// @ts-ignore
39+
window.location = new URL(url);
40+
};
41+
42+
describe('BaseRouter basePath guard', () => {
43+
beforeAll(() => {
44+
// @ts-ignore
45+
delete window.location;
46+
});
47+
48+
afterAll(() => {
49+
window.location = oldWindowLocation;
50+
});
51+
52+
afterEach(() => {
53+
vi.clearAllMocks();
54+
});
55+
56+
describe('does not update state for out-of-basePath navigations', () => {
57+
it('keeps currentPath when window.location changes outside basePath and refresh is called', () => {
58+
setWindowLocation('https://www.example.com/sign-in');
59+
60+
const RefreshTrigger = () => {
61+
const router = useRouter();
62+
return (
63+
<button
64+
data-testid='refresh'
65+
onClick={() => router.refresh()}
66+
>
67+
Refresh
68+
</button>
69+
);
70+
};
71+
72+
render(
73+
<PathRouter basePath='/sign-in'>
74+
<Route index>
75+
<CurrentPath />
76+
<RefreshTrigger />
77+
</Route>
78+
</PathRouter>,
79+
);
80+
81+
// Verify initial state
82+
expect(screen.getByTestId('current-path').textContent).toBe('/sign-in');
83+
84+
// Simulate URL changing outside basePath (e.g. framework navigation)
85+
setWindowLocation('https://www.example.com/');
86+
87+
// Call refresh — this should bail out because '/' doesn't start with '/sign-in'
88+
act(() => {
89+
screen.getByTestId('refresh').click();
90+
});
91+
92+
// currentPath should still be /sign-in, not /
93+
expect(screen.getByTestId('current-path').textContent).toBe('/sign-in');
94+
});
95+
96+
it('does not trigger catch-all route when URL changes outside basePath', () => {
97+
setWindowLocation('https://www.example.com/sign-in');
98+
99+
const redirectFn = vi.fn();
100+
101+
const CatchAll = () => {
102+
React.useEffect(() => {
103+
redirectFn();
104+
}, []);
105+
return <div data-testid='catch-all'>Redirecting...</div>;
106+
};
107+
108+
const RefreshTrigger = () => {
109+
const router = useRouter();
110+
return (
111+
<button
112+
data-testid='refresh'
113+
onClick={() => router.refresh()}
114+
>
115+
Refresh
116+
</button>
117+
);
118+
};
119+
120+
render(
121+
<PathRouter basePath='/sign-in'>
122+
<Switch>
123+
<Route index>
124+
<CurrentPath />
125+
<RefreshTrigger />
126+
</Route>
127+
<Route>
128+
<CatchAll />
129+
</Route>
130+
</Switch>
131+
</PathRouter>,
132+
);
133+
134+
// Verify initial render — index route should match
135+
expect(screen.getByTestId('current-path').textContent).toBe('/sign-in');
136+
expect(screen.queryByTestId('catch-all')).toBeNull();
137+
expect(redirectFn).not.toHaveBeenCalled();
138+
139+
// Simulate URL changing outside basePath
140+
setWindowLocation('https://www.example.com/');
141+
142+
// Call refresh — should be guarded by basePath check
143+
act(() => {
144+
screen.getByTestId('refresh').click();
145+
});
146+
147+
// The catch-all should NOT have rendered
148+
expect(screen.queryByTestId('catch-all')).toBeNull();
149+
expect(redirectFn).not.toHaveBeenCalled();
150+
// Index route should still be visible
151+
expect(screen.getByTestId('current-path').textContent).toBe('/sign-in');
152+
});
153+
});
154+
155+
describe('still updates state for within-basePath navigations', () => {
156+
it('updates currentPath when URL changes within basePath', () => {
157+
setWindowLocation('https://www.example.com/sign-in');
158+
159+
const RefreshTrigger = () => {
160+
const router = useRouter();
161+
return (
162+
<button
163+
data-testid='refresh'
164+
onClick={() => router.refresh()}
165+
>
166+
Refresh
167+
</button>
168+
);
169+
};
170+
171+
render(
172+
<PathRouter basePath='/sign-in'>
173+
<Route path='factor-one'>
174+
<div data-testid='factor-one'>Factor One</div>
175+
</Route>
176+
<Route index>
177+
<CurrentPath />
178+
<RefreshTrigger />
179+
</Route>
180+
</PathRouter>,
181+
);
182+
183+
// Verify initial state
184+
expect(screen.getByTestId('current-path').textContent).toBe('/sign-in');
185+
186+
// Simulate URL changing within basePath
187+
setWindowLocation('https://www.example.com/sign-in/factor-one');
188+
189+
// Call refresh — this should update because '/sign-in/factor-one' starts with '/sign-in'
190+
act(() => {
191+
screen.getByTestId('refresh').click();
192+
});
193+
194+
// currentPath should have updated
195+
expect(screen.getByTestId('factor-one')).toBeInTheDocument();
196+
});
197+
});
198+
});

0 commit comments

Comments
 (0)