Skip to content

fix(solid-start): navigation transitions #5792

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

Merged
merged 5 commits into from
Nov 9, 2025
Merged

fix(solid-start): navigation transitions #5792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d74d533
fix(solid-start): navigation transitions
birkskyum Nov 8, 2025
9d2fe25
ci: apply automated fixes
autofix-ci[bot] Nov 9, 2025
18b7f7b
add return type for getMatch
birkskyum Nov 9, 2025
c8d15f0
reduce diff
birkskyum Nov 9, 2025
705b952
remove comment
birkskyum Nov 9, 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
95 changes: 95 additions & 0 deletions e2e/solid-start/basic/tests/transition.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import { expect, test } from '@playwright/test'

test('transitions/count/create-resource should keep old values visible during navigation', async ({
page,
}) => {
await page.goto('/transition/count/create-resource')

await expect(page.getByTestId('n-value')).toContainText('n: 1')
await expect(page.getByTestId('double-value')).toContainText('double: 2')

const bodyTexts: Array<string> = []

const pollInterval = setInterval(async () => {
const text = await page
.locator('body')
.textContent()
.catch(() => '')
if (text) bodyTexts.push(text)
}, 50)

// 1 click

page.getByTestId('increase-button').click()
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 8, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Await Playwright click actions to prevent race conditions.

The click operation is missing await, which can cause race conditions where the test proceeds before the click is fully processed. This is particularly problematic when testing transition behavior with precise timing requirements.

Apply this diff:

-  page.getByTestId('increase-button').click()
+  await page.getByTestId('increase-button').click()

Also applies to: 45-46, 66-68

🤖 Prompt for AI Agents 
In e2e/solid-start/basic/tests/transition.spec.ts around lines 23 (and also at
45-46 and 66-68), the Playwright click calls are not awaited which can cause
race conditions; update each occurrence to await the click promise (i.e., prefix
each page.getByTestId(...).click() with await) so the test waits for the click
to complete before proceeding, keeping the existing selectors and test flow
intact.


await expect(page.getByTestId('n-value')).toContainText('n: 1', {
timeout: 2_000,
})
await expect(page.getByTestId('double-value')).toContainText('double: 2', {
timeout: 2_000,
})

await page.waitForTimeout(200)

await expect(page.getByTestId('n-value')).toContainText('n: 2', {
timeout: 2000,
})
await expect(page.getByTestId('double-value')).toContainText('double: 4', {
timeout: 2000,
})

// 2 clicks

page.getByTestId('increase-button').click()
page.getByTestId('increase-button').click()

await expect(page.getByTestId('n-value')).toContainText('n: 2', {
timeout: 2000,
})
await expect(page.getByTestId('double-value')).toContainText('double: 4', {
timeout: 2000,
})

await page.waitForTimeout(200)

await expect(page.getByTestId('n-value')).toContainText('n: 4', {
timeout: 2000,
})
await expect(page.getByTestId('double-value')).toContainText('double: 8', {
timeout: 2000,
})

// 3 clicks

page.getByTestId('increase-button').click()
page.getByTestId('increase-button').click()
page.getByTestId('increase-button').click()

await expect(page.getByTestId('n-value')).toContainText('n: 4', {
timeout: 2000,
})
await expect(page.getByTestId('double-value')).toContainText('double: 8', {
timeout: 2000,
})

await page.waitForTimeout(200)

await expect(page.getByTestId('n-value')).toContainText('n: 7', {
timeout: 2000,
})
await expect(page.getByTestId('double-value')).toContainText('double: 14', {
timeout: 2000,
})

clearInterval(pollInterval)

// With proper transitions, old values should remain visible until new ones arrive
const hasLoadingText = bodyTexts.some((text) => text.includes('Loading...'))

if (hasLoadingText) {
throw new Error(
'FAILED: "Loading..." appeared during navigation. ' +
'Solid Router should use transitions to keep old values visible.',
)
}
})
2 changes: 1 addition & 1 deletion packages/react-router/tests/store-updates-during-navigation.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,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(14)
expect(updates).toBe(16)
})

test('navigate, w/ preloaded & async loaders', async () => {
Expand Down
2 changes: 2 additions & 0 deletions packages/router-core/src/Matches.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ export interface RouteMatch<
displayPendingPromise?: Promise<void>
minPendingPromise?: ControlledPromise<void>
dehydrated?: boolean
/** @internal */
error?: unknown
}
loaderData?: TLoaderData
/** @internal */
Expand Down
23 changes: 20 additions & 3 deletions packages/router-core/src/load-matches.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ const handleRedirectAndNotFound = (

const status = isRedirect(err) ? 'redirected' : 'notFound'

match._nonReactive.error = err

inner.updateMatch(match.id, (prev) => ({
...prev,
status,
Expand Down Expand Up @@ -560,9 +562,23 @@ const getLoaderContext = (
route: AnyRoute,
): LoaderFnContext => {
const parentMatchPromise = inner.matchPromises[index - 1] as any
const { params, loaderDeps, abortController, context, cause } =
const { params, loaderDeps, abortController, cause } =
inner.router.getMatch(matchId)!

let context = inner.router.options.context ?? {}

for (let i = 0; i <= index; i++) {
const innerMatch = inner.matches[i]
if (!innerMatch) continue
const m = inner.router.getMatch(innerMatch.id)
if (!m) continue
context = {
...context,
...(m.__routeContext ?? {}),
...(m.__beforeLoadContext ?? {}),
}
}

const preload = resolvePreload(inner, matchId)

return {
Expand Down Expand Up @@ -750,8 +766,9 @@ const loadRouteMatch = async (
}
await prevMatch._nonReactive.loaderPromise
const match = inner.router.getMatch(matchId)!
if (match.error) {
handleRedirectAndNotFound(inner, match, match.error)
const error = match._nonReactive.error || match.error
if (error) {
handleRedirectAndNotFound(inner, match, error)
}
} else {
// This is where all of the stale-while-revalidate magic happens
Expand Down
34 changes: 19 additions & 15 deletions packages/router-core/src/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2295,23 +2295,27 @@ export class RouterCore<
}

updateMatch: UpdateMatchFn = (id, updater) => {
const matchesKey = this.state.pendingMatches?.some((d) => d.id === id)
? 'pendingMatches'
: this.state.matches.some((d) => d.id === id)
? 'matches'
: this.state.cachedMatches.some((d) => d.id === id)
? 'cachedMatches'
: ''

if (matchesKey) {
this.__store.setState((s) => ({
...s,
[matchesKey]: s[matchesKey]?.map((d) => (d.id === id ? updater(d) : d)),
}))
}
this.startTransition(() => {
const matchesKey = this.state.pendingMatches?.some((d) => d.id === id)
? 'pendingMatches'
: this.state.matches.some((d) => d.id === id)
? 'matches'
: this.state.cachedMatches.some((d) => d.id === id)
? 'cachedMatches'
: ''

if (matchesKey) {
this.__store.setState((s) => ({
...s,
[matchesKey]: s[matchesKey]?.map((d) =>
d.id === id ? updater(d) : d,
),
}))
}
})
}

getMatch: GetMatchFn = (matchId: string) => {
getMatch: GetMatchFn = (matchId: string): AnyRouteMatch | undefined => {
const findFn = (d: { id: string }) => d.id === matchId
return (
this.state.cachedMatches.find(findFn) ??
Expand Down
12 changes: 6 additions & 6 deletions packages/solid-router/tests/store-updates-during-navigation.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
// that needs to be done during a navigation.
// Any change that increases this number should be investigated.
// Note: Solid has different update counts than React due to different reactivity
expect(updates).toBe(5)
expect(updates).toBe(6)
})

test('sync beforeLoad', async () => {
Expand All @@ -173,7 +173,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
// that needs to be done during a navigation.
// Any change that increases this number should be investigated.
// Note: Solid has different update counts than React due to different reactivity
expect(updates).toBe(9)
expect(updates).toBe(8)
})

test('nothing', async () => {
Expand Down Expand Up @@ -226,7 +226,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(14)
expect(updates).toBe(16)
})

test('navigate, w/ preloaded & async loaders', async () => {
Expand All @@ -242,7 +242,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(8)
expect(updates).toBe(9)
})

test('navigate, w/ preloaded & sync loaders', async () => {
Expand All @@ -259,7 +259,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
// that needs to be done during a navigation.
// Any change that increases this number should be investigated.
// Note: Solid has one fewer update than React due to different reactivity
expect(updates).toBe(6)
expect(updates).toBe(7)
})

test('navigate, w/ previous navigation & async loader', async () => {
Expand Down Expand Up @@ -293,6 +293,6 @@ 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(1)
expect(updates).toBe(2)
})
})
Loading