Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
110 changes: 110 additions & 0 deletions packages/react-router/tests/loaders.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -824,3 +824,113 @@ test('cancelMatches after pending timeout', async () => {
expect(fooPendingComponentOnMountMock).toHaveBeenCalled()
expect(onAbortMock).toHaveBeenCalled()
})

test('reproducer for #6388 - rapid navigation between parameterized routes should not trigger errorComponent', async () => {
const errorComponentRenderCount = vi.fn()
const onAbortMock = vi.fn()
const loaderCompleteMock = vi.fn()

const rootRoute = createRootRoute({
component: () => (
<div>
<Link data-testid="link-to-home" to="/">
Home
</Link>
<Link
data-testid="link-to-param-1"
to="/something/$id"
params={{ id: '1' }}
preload={false}
>
Param 1
</Link>
<Link
data-testid="link-to-param-2"
to="/something/$id"
params={{ id: '2' }}
preload={false}
>
Param 2
</Link>
<Outlet />
</div>
),
})

const indexRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/',
component: () => <div data-testid="home-page">Home page</div>,
})

const paramRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/something/$id',
pendingMs: 0,
loader: async ({ params, abortController }) => {
const result = await new Promise<{ id: string; done: boolean }>(
(resolve, reject) => {
const timer = setTimeout(() => {
loaderCompleteMock(params.id)
resolve({ id: params.id, done: true })
}, WAIT_TIME * 5)

abortController.signal.addEventListener('abort', () => {
clearTimeout(timer)
onAbortMock(params.id)
reject(new DOMException('Aborted', 'AbortError'))
})
},
)

return result
},
component: () => {
const data = paramRoute.useLoaderData()
return (
<div data-testid="param-page">
Param Component {data.id} {data.done ? 'Done' : 'Not done'}
</div>
)
},
errorComponent: ({ error }) => {
errorComponentRenderCount(error)
return (
<div data-testid="error-component">
Error Component: {error.message} | Name: {error.name}
</div>
)
},
pendingComponent: () => <div data-testid="pending-component">Pending</div>,
})

const routeTree = rootRoute.addChildren([indexRoute, paramRoute])
const router = createRouter({
routeTree,
history,
defaultPreload: false,
})

render(<RouterProvider router={router} />)
await act(() => router.latestLoadPromise)

expect(await screen.findByTestId('home-page')).toBeInTheDocument()

const param1Link = await screen.findByTestId('link-to-param-1')
fireEvent.click(param1Link)

const param2Link = await screen.findByTestId('link-to-param-2')
fireEvent.click(param2Link)

fireEvent.click(param1Link)

await act(() => router.latestLoadPromise)

expect(onAbortMock).toHaveBeenCalled()
expect(errorComponentRenderCount).not.toHaveBeenCalled()
expect(screen.queryByTestId('error-component')).not.toBeInTheDocument()

const paramPage = await screen.findByTestId('param-page')
expect(paramPage).toBeInTheDocument()
expect(loaderCompleteMock).toHaveBeenCalled()
})
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
// This number should be as small as possible to minimize the amount of work
// that needs to be done during a navigation.
// Any change that increases this number should be investigated.
expect(updates).toBe(7)
expect(updates).toBe(8)
})

test('navigate, w/ preloaded & sync loaders', async () => {
Expand Down
32 changes: 22 additions & 10 deletions packages/router-core/src/load-matches.ts
Original file line number Diff line number Diff line change
Expand Up @@ -703,12 +703,19 @@ const runLoader = async (
let error = e

if ((error as any)?.name === 'AbortError') {
inner.updateMatch(matchId, (prev) => ({
...prev,
status: prev.status === 'pending' ? 'success' : prev.status,
isFetching: false,
context: buildMatchContext(inner, index),
}))
const wasAbortedByNavigation =
match.abortController?.signal.aborted === true

if (!wasAbortedByNavigation) {
inner.updateMatch(matchId, (prev) => ({
...prev,
status: prev.status === 'pending' ? 'success' : prev.status,
isFetching: false,
}))
return
}
match._nonReactive.loaderPromise?.resolve()
match._nonReactive.loaderPromise = undefined
return
}

Expand Down Expand Up @@ -773,12 +780,17 @@ const loadRouteMatch = async (
return prevMatch
}
await prevMatch._nonReactive.loaderPromise
const match = inner.router.getMatch(matchId)!
const error = match._nonReactive.error || match.error
const matchAfterWait = inner.router.getMatch(matchId)!
const error = matchAfterWait._nonReactive.error || matchAfterWait.error
if (error) {
handleRedirectAndNotFound(inner, match, error)
handleRedirectAndNotFound(inner, matchAfterWait, error)
}
} else {

if (matchAfterWait.status !== 'pending') {
return matchAfterWait
}
}
Comment on lines +783 to +792
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Clear pendingTimeout before the early return.

The new early return skips the cleanup block at the end of loadRouteMatch. If a pending timeout was set in this load cycle, it can remain attached and block future pending scheduling. Consider clearing it before returning (Line 789).

🧹 Suggested cleanup before return
       if (matchAfterWait.status !== 'pending') {
+        clearTimeout(matchAfterWait._nonReactive.pendingTimeout)
+        matchAfterWait._nonReactive.pendingTimeout = undefined
         return matchAfterWait
       }
📝 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 matchAfterWait = inner.router.getMatch(matchId)!
const error = matchAfterWait._nonReactive.error || matchAfterWait.error
if (error) {
handleRedirectAndNotFound(inner, match, error)
handleRedirectAndNotFound(inner, matchAfterWait, error)
}
} else {
if (matchAfterWait.status !== 'pending') {
return matchAfterWait
}
}
const matchAfterWait = inner.router.getMatch(matchId)!
const error = matchAfterWait._nonReactive.error || matchAfterWait.error
if (error) {
handleRedirectAndNotFound(inner, matchAfterWait, error)
}
if (matchAfterWait.status !== 'pending') {
clearTimeout(matchAfterWait._nonReactive.pendingTimeout)
matchAfterWait._nonReactive.pendingTimeout = undefined
return matchAfterWait
}
}
🤖 Prompt for AI Agents
In `@packages/router-core/src/load-matches.ts` around lines 783 - 792, In
loadRouteMatch ensure any pending timeout set during this load cycle is cleared
before the early return: if you created a pendingTimeout (e.g., pendingTimeout
or inner.pendingTimeout) clear it with clearTimeout(...) and reset the variable
(e.g., pendingTimeout = undefined or inner.pendingTimeout = undefined)
immediately before returning when matchAfterWait.status !== 'pending'; do this
near the block that reads matchAfterWait and calls handleRedirectAndNotFound so
pending timers don't persist and block future pending scheduling.

{
// This is where all of the stale-while-revalidate magic happens
const age = Date.now() - prevMatch.updatedAt

Expand Down
Loading