Skip to content

test: added (currently failing) unit test to reproduce regression introduced in 1.123.2 #4726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

test: added (currently failing) unit test to reproduce regression introduced in 1.123.2 #4726

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
936b8ae
added a (failing) unit test
Dakkers Jul 20, 2025
2fcd4dc
formatting
Dakkers Jul 20, 2025
1da38b1
changes from PR review
Dakkers Jul 20, 2025
82df257
back to act
Dakkers Jul 20, 2025
2667339
ci: apply automated fixes
autofix-ci[bot] Jul 21, 2025
6631a00
Merge branch 'main' into dakkers/issue-4696
Dakkers Aug 7, 2025
e7b3dd3
Merge branch 'main' into dakkers/issue-4696
Dakkers Aug 8, 2025
35c6f86
Merge branch 'main' into dakkers/issue-4696
Dakkers Aug 8, 2025
15dcce3
Merge branch 'main' into dakkers/issue-4696
Dakkers Aug 11, 2025
d9b8213
Merge branch 'main' into dakkers/issue-4696
Dakkers Sep 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions packages/react-router/tests/loaders.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
Outlet,
RouterProvider,
createBrowserHistory,
createMemoryHistory,
createRootRoute,
createRoute,
createRouter,
Expand Down Expand Up @@ -729,3 +730,87 @@ test('clears pendingTimeout when match resolves', async () => {
expect(nestedPendingComponentOnMountMock).not.toHaveBeenCalled()
expect(fooPendingComponentOnMountMock).not.toHaveBeenCalled()
})

test('reproducer for #4696', async () => {
const rootRoute = createRootRoute({
beforeLoad: async ({ context }) => {
return {
...context,
isAuthenticated: true,
isAdmin: false,
}
},
loader: ({ context }) => context,
pendingComponent: () => 'Loading...',
wrapInSuspense: true,
component: RootRouteContent,
})

function RootRouteContent() {
const routeData = rootRoute.useLoaderData()
const isAuthenticated = routeData.isAuthenticated
return (
<>
{!!isAuthenticated && (
<nav data-testid="nav" style={{ display: 'flex', gap: 8 }}>
<Link data-testid="link-index" to="/">
Index
</Link>
<Link data-testid="link-dashboard" to="/dashboard">
Dashboard
</Link>
</nav>
)}
<hr />
<Outlet />
</>
)
}

const indexRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/',
component: () => <h1 data-testid="heading-index">Index</h1>,
})

const dashboardRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/dashboard',
pendingComponent: () => 'Loading dashboard...',
validateSearch: () => {
return {
page: 0,
}
},
component: () => <h1 data-testid="heading-dashboard">Dashboard</h1>,
})

const routeTree = rootRoute.addChildren([indexRoute, dashboardRoute])
const router = createRouter({ routeTree })

render(<RouterProvider router={router} />)
await act(async () => {})
expect(screen.getByTestId('heading-index')).toBeInTheDocument()
expect(screen.getByTestId('nav')).toBeInTheDocument()

cleanup()

const historyWithSearchParam = createMemoryHistory({
initialEntries: ['/dashboard?page=0'],
})
render(<RouterProvider history={historyWithSearchParam} router={router} />)
await act(async () => {})
expect(screen.getByTestId('heading-dashboard')).toBeInTheDocument()
expect(screen.getByTestId('nav')).toBeInTheDocument()

Comment on lines +798 to +805
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Don’t reuse the same router while swapping histories via RouterProvider; create a new router per history.

Passing history into RouterProvider while keeping the same router instance can desynchronize the router’s internal history from the Provider and cause loaders/Suspense not to resolve as expected. Build a fresh router with the target memory history for each scenario and await latestLoadPromise.

-  const historyWithSearchParam = createMemoryHistory({
-    initialEntries: ['/dashboard?page=0'],
-  })
-  render(<RouterProvider history={historyWithSearchParam} router={router} />)
-  await act(async () => {})
-  expect(screen.getByTestId('heading-dashboard')).toBeInTheDocument()
-  expect(screen.getByTestId('nav')).toBeInTheDocument()
+  const router2 = createRouter({
+    routeTree,
+    history: createMemoryHistory({ initialEntries: ['/dashboard?page=0'] }),
+  })
+  render(<RouterProvider router={router2} />)
+  await act(() => router2.latestLoadPromise)
+  expect(await screen.findByTestId('heading-dashboard')).toBeInTheDocument()
+  expect(await screen.findByTestId('nav')).toBeInTheDocument()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const historyWithSearchParam = createMemoryHistory({
initialEntries: ['/dashboard?page=0'],
})
render(<RouterProvider history={historyWithSearchParam} router={router} />)
await act(async () => {})
expect(screen.getByTestId('heading-dashboard')).toBeInTheDocument()
expect(screen.getByTestId('nav')).toBeInTheDocument()
const router2 = createRouter({
routeTree,
history: createMemoryHistory({ initialEntries: ['/dashboard?page=0'] }),
})
render(<RouterProvider router={router2} />)
await act(() => router2.latestLoadPromise)
expect(await screen.findByTestId('heading-dashboard')).toBeInTheDocument()
expect(await screen.findByTestId('nav')).toBeInTheDocument()
🤖 Prompt for AI Agents 
In packages/react-router/tests/loaders.test.tsx around lines 798 to 805, the
test reuses the same router instance while swapping in a new memory history via
RouterProvider which can desynchronize the router's internal history and cause
loaders/Suspense to not resolve; instead construct a fresh router for each
history scenario (createMemoryHistory with the desired initialEntries, build a
new router using that history), render RouterProvider with that new router, and
after rendering await the router's latestLoadPromise (or equivalent promise that
indicates loading finished) before asserting that elements like
heading-dashboard and nav are in the document.

cleanup()

const historyWithoutSearchParam = createMemoryHistory({
initialEntries: ['/dashboard'],
})
render(<RouterProvider history={historyWithoutSearchParam} router={router} />)
await act(async () => {})
expect(screen.getByTestId('heading-dashboard')).toBeInTheDocument()
// Fails here!
expect(screen.getByTestId('nav')).toBeInTheDocument()
})
Comment on lines +806 to +816
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Apply the same fix for the no-search-param case.

Create a third router with /dashboard as the initial entry and await load before asserting. This addresses the current failure.

-  const historyWithoutSearchParam = createMemoryHistory({
-    initialEntries: ['/dashboard'],
-  })
-  render(<RouterProvider history={historyWithoutSearchParam} router={router} />)
-  await act(async () => {})
-  expect(screen.getByTestId('heading-dashboard')).toBeInTheDocument()
-  // Fails here!
-  expect(screen.getByTestId('nav')).toBeInTheDocument()
+  const router3 = createRouter({
+    routeTree,
+    history: createMemoryHistory({ initialEntries: ['/dashboard'] }),
+  })
+  render(<RouterProvider router={router3} />)
+  await act(() => router3.latestLoadPromise)
+  expect(await screen.findByTestId('heading-dashboard')).toBeInTheDocument()
+  expect(await screen.findByTestId('nav')).toBeInTheDocument()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
cleanup()
const historyWithoutSearchParam = createMemoryHistory({
initialEntries: ['/dashboard'],
})
render(<RouterProvider history={historyWithoutSearchParam} router={router} />)
await act(async () => {})
expect(screen.getByTestId('heading-dashboard')).toBeInTheDocument()
// Fails here!
expect(screen.getByTestId('nav')).toBeInTheDocument()
})
cleanup()
const router3 = createRouter({
routeTree,
history: createMemoryHistory({ initialEntries: ['/dashboard'] }),
})
render(<RouterProvider router={router3} />)
await act(() => router3.latestLoadPromise)
expect(await screen.findByTestId('heading-dashboard')).toBeInTheDocument()
expect(await screen.findByTestId('nav')).toBeInTheDocument()
})
🤖 Prompt for AI Agents 
In packages/react-router/tests/loaders.test.tsx around lines 806 to 816, the
"no-search-param" test renders RouterProvider with a history whose initial entry
is '/dashboard' but does not await the router load, causing the nav assertion to
fail; fix by creating a dedicated router instance for the no-search-param case
using '/dashboard' as the initial entry (i.e., a third router), render
RouterProvider with that router, then await the router/load to finish (use act
or await the navigation promise) before asserting that the heading and nav
elements are in the document.

Loading