fix: match loading#7637
Conversation
github-actions
Bot
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves the ChangesCore router lifecycle and framework alignment
New React e2e reproductions for issues 7120 and 7457
Sequence Diagram(s)sequenceDiagram
participant Browser
participant RouterLoad as router.load()
participant LoadMatches as loadMatches
participant HandleBeforeLoad as handleBeforeLoad
participant ExecuteBeforeLoad as executeBeforeLoad
participant RunLoaderPhase as runLoaderPhase
participant Cache as router cache
Browser->>RouterLoad: navigate to route
RouterLoad->>LoadMatches: matches + preloadMatchIds
LoadMatches->>HandleBeforeLoad: queueExecution (checks in-flight promises)
HandleBeforeLoad->>ExecuteBeforeLoad: run if generation current
ExecuteBeforeLoad->>LoadMatches: commitMatch(__beforeLoadContext)
LoadMatches->>RunLoaderPhase: loader with generation tracking
alt background reload
RunLoaderPhase->>LoadMatches: detach (clear invalid, return)
else normal load
RunLoaderPhase->>LoadMatches: await loader, commitMatch success
end
alt redirect or notFound
LoadMatches->>Cache: clearMatchPromises, evict entry
LoadMatches->>RouterLoad: throw redirect/notFound
end
RouterLoad->>Browser: settled load with status code
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 1d394e1
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview1 package(s) bumped directly, 22 bumped as dependents. 🟩 Patch bumps
|
schiller-manuel
force-pushed
the
fix-match-loading
branch
from
63e7df5 to
9698aee
Compare
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
Merging this PR will degrade performance by 23.73%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem loader-data-retention (solid) |
158.4 KB | 2,333 KB | -93.21% |
| ❌ | Memory | mem loader-data-retention (react) |
203.5 KB | 2,335.7 KB | -91.29% |
| ❌ | Memory | mem navigation-churn (react) |
430.2 KB | 1,188.3 KB | -63.8% |
| ❌ | Memory | mem unique-location-churn (react) |
728.8 KB | 1,177.4 KB | -38.1% |
| ❌ | Memory | mem interrupted-navigations (react) |
759.7 KB | 1,210.4 KB | -37.24% |
| ❌ | Memory | mem serialization-payload (react) |
31.2 MB | 34.9 MB | -10.56% |
| ❌ | Memory | mem request-churn (react) |
1.6 MB | 1.7 MB | -7% |
| ❌ | Simulation | ssr redirect (react) |
61.4 ms | 64.8 ms | -5.3% |
| ❌ | Simulation | ssr redirect (solid) |
59.5 ms | 62.1 ms | -4.22% |
| ❌ | Memory | mem request-churn (vue) |
1.3 MB | 1.3 MB | -3.91% |
| ⚡ | Memory | mem preload-churn (solid) |
578.1 KB | 246.4 KB | ×2.3 |
| ⚡ | Memory | mem preload-churn (react) |
610.3 KB | 436.6 KB | +39.78% |
| ⚡ | Memory | mem aborted-requests (solid) |
1.8 MB | 1.6 MB | +12.93% |
| ⚡ | Memory | mem error-paths unmatched (react) |
1.1 MB | 1 MB | +7.83% |
| ⚡ | Simulation | ssr control-flow route headers (solid) |
82.5 ms | 76.8 ms | +7.41% |
| ⚡ | Simulation | ssr selective (solid) |
74.8 ms | 70.3 ms | +6.4% |
| ⚡ | Simulation | ssr server-fn multipart (solid) |
63 ms | 59.9 ms | +5.07% |
| ⚡ | Simulation | ssr server-fn raw-stream (solid) |
111.8 ms | 106.6 ms | +4.88% |
| ⚡ | Memory | mem mount-unmount (solid) |
493 KB | 472.4 KB | +4.36% |
| ⚡ | Simulation | ssr server-fn raw-response (solid) |
57.2 ms | 55.3 ms | +3.54% |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix-match-loading (0d36b6c) with main (f23ed0f)
Footnotes
-
37 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
e2e/react-router/issue-7120/src/main.tsx (1)
22-22: Add typed global declaration for__pendingSeento replace(globalThis as any)casts.Replace the untyped
as anypattern across three files with a global type declaration:
e2e/react-router/issue-7120/src/main.tsxLine 22e2e/react-router/issue-7120/tests/issue-7120.repro.spec.tsLine 23e2e/react-router/issue-7457/src/main.tsxLine 19This violates the TypeScript strict mode guideline for these
.ts/.tsxfiles.Add to a global d.ts file (e.g., vite-env.d.ts)
declare global { var __pendingSeen: boolean | undefined } export {}Then update all occurrences from
(globalThis as any).__pendingSeentoglobalThis.__pendingSeen.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/react-router/issue-7120/src/main.tsx` at line 22, Add a typed global declaration for __pendingSeen to eliminate the untyped `as any` casts. Create or update a global type declaration file (e.g., vite-env.d.ts) with a declare global block that types __pendingSeen as `boolean | undefined`, then export an empty object. Replace all three occurrences of `(globalThis as any).__pendingSeen` with `globalThis.__pendingSeen` in: e2e/react-router/issue-7120/src/main.tsx at line 22, e2e/react-router/issue-7120/tests/issue-7120.repro.spec.ts at line 23, and e2e/react-router/issue-7457/src/main.tsx at line 19. This will satisfy TypeScript strict mode requirements without the unsafe `any` casts.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/react-router/issue-7120/package.json`:
- Around line 14-20: The package.json file uses workspace:^ for internal
workspace packages (`@tanstack/react-router` and `@tanstack/router-e2e-utils`) but
the repository guideline requires workspace:* for all internal dependencies.
Change both `@tanstack/react-router` and `@tanstack/router-e2e-utils` entries from
workspace:^ to workspace:* to align with the established guideline and prevent
dependency drift.
In `@e2e/react-router/issue-7120/tsconfig.json`:
- Line 12: The types configuration in tsconfig.json at the "types" field
currently only includes vite/client, which does not provide Node global type
definitions. Since playwright.config.ts uses process.env, the Node types must be
added to the types array to resolve the process global and satisfy type-checking
when tsc --noEmit is run. Add "node" to the types array alongside the existing
"vite/client" entry.
In `@e2e/react-router/issue-7457/package.json`:
- Around line 14-15: Replace the version specifiers for internal dependencies in
the package.json file from workspace:^ to workspace:* to comply with the coding
guideline for internal dependency versioning. Specifically, change the version
specifier for `@tanstack/react-router` on line 14, `@tanstack/router-plugin` on line
15, and the internal dependency on line 21 from workspace:^ to workspace:*.
In `@packages/solid-router/tests/routeContext.test.tsx`:
- Around line 2479-2487: The `sawUndefinedContext` variable uses a one-time
assignment during component setup, so it will not detect if the context becomes
undefined after the component mounts. Replace the simple boolean assignment with
a reactive pattern: create a signal to store the `sawUndefinedContext` state and
use `createEffect` to reactively update it whenever `number()` changes, ensuring
that any context loss during the component's lifetime (such as during a
background loader reload) is properly detected and reflected in the rendered
output.
---
Nitpick comments:
In `@e2e/react-router/issue-7120/src/main.tsx`:
- Line 22: Add a typed global declaration for __pendingSeen to eliminate the
untyped `as any` casts. Create or update a global type declaration file (e.g.,
vite-env.d.ts) with a declare global block that types __pendingSeen as `boolean
| undefined`, then export an empty object. Replace all three occurrences of
`(globalThis as any).__pendingSeen` with `globalThis.__pendingSeen` in:
e2e/react-router/issue-7120/src/main.tsx at line 22,
e2e/react-router/issue-7120/tests/issue-7120.repro.spec.ts at line 23, and
e2e/react-router/issue-7457/src/main.tsx at line 19. This will satisfy
TypeScript strict mode requirements without the unsafe `any` casts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31051461-bf2e-4485-99b1-ad9e397b0087
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (46)
.changeset/violet-poets-wait.mddocs/router/api/router/RouteMatchType.mde2e/react-router/issue-7120/index.htmle2e/react-router/issue-7120/package.jsone2e/react-router/issue-7120/playwright.config.tse2e/react-router/issue-7120/src/main.tsxe2e/react-router/issue-7120/src/posts.lazy.tsxe2e/react-router/issue-7120/tests/issue-7120.repro.spec.tse2e/react-router/issue-7120/tsconfig.jsone2e/react-router/issue-7120/vite.config.jse2e/react-router/issue-7457/index.htmle2e/react-router/issue-7457/package.jsone2e/react-router/issue-7457/playwright.config.tse2e/react-router/issue-7457/src/main.tsxe2e/react-router/issue-7457/src/routeTree.gen.tse2e/react-router/issue-7457/src/routes/__root.tsxe2e/react-router/issue-7457/src/routes/another.tsxe2e/react-router/issue-7457/src/routes/index.tsxe2e/react-router/issue-7457/src/vite-env.d.tse2e/react-router/issue-7457/tests/issue-7457.repro.spec.tse2e/react-router/issue-7457/tsconfig.jsone2e/react-router/issue-7457/vite.config.jse2e/vue-router/view-transitions/tests/view-transitions.spec.tspackages/react-router/src/Match.tsxpackages/react-router/tests/not-found.test.tsxpackages/react-router/tests/redirect.test.tsxpackages/react-router/tests/routeContext.test.tsxpackages/router-core/src/Matches.tspackages/router-core/src/load-matches.tspackages/router-core/src/redirect.tspackages/router-core/src/router.tspackages/router-core/src/ssr/ssr-client.tspackages/router-core/src/utils.tspackages/router-core/tests/hydrate.test.tspackages/router-core/tests/load.test.tspackages/router-devtools-core/src/useStyles.tsxpackages/router-devtools-core/src/utils.tsxpackages/solid-router/src/Match.tsxpackages/solid-router/tests/redirect.test.tsxpackages/solid-router/tests/routeContext.test.tsxpackages/solid-router/tests/store-updates-during-navigation.test.tsxpackages/vue-router/src/Match.tsxpackages/vue-router/src/Transitioner.tsxpackages/vue-router/tests/redirect.test.tsxpackages/vue-router/tests/store-updates-during-navigation.test.tsxskills/bundle-size-optimization/SKILL.md
💤 Files with no reviewable changes (5)
- packages/react-router/tests/redirect.test.tsx
- packages/solid-router/tests/redirect.test.tsx
- packages/router-devtools-core/src/utils.tsx
- packages/vue-router/tests/redirect.test.tsx
- packages/vue-router/src/Match.tsx
| "@tanstack/react-router": "workspace:^", | ||
| "react": "^19.0.0", | ||
| "react-dom": "^19.0.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@playwright/test": "^1.50.1", | ||
| "@tanstack/router-e2e-utils": "workspace:^", |
There was a problem hiding this comment.
Align internal workspace dependency ranges with repo guideline.
Line 14 and Line 20 use workspace:^ for internal packages, but this PR’s package guideline requires workspace:* in package.json files. Please switch both entries (or explicitly document an e2e exception in the guideline to avoid drift).
Suggested patch
- "`@tanstack/react-router`": "workspace:^",
+ "`@tanstack/react-router`": "workspace:*",
@@
- "`@tanstack/router-e2e-utils`": "workspace:^",
+ "`@tanstack/router-e2e-utils`": "workspace:*",As per coding guidelines, internal dependencies in **/package.json must use the workspace protocol workspace:*.
📝 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.
| "@tanstack/react-router": "workspace:^", | |
| "react": "^19.0.0", | |
| "react-dom": "^19.0.0" | |
| }, | |
| "devDependencies": { | |
| "@playwright/test": "^1.50.1", | |
| "@tanstack/router-e2e-utils": "workspace:^", | |
| "`@tanstack/react-router`": "workspace:*", | |
| "react": "^19.0.0", | |
| "react-dom": "^19.0.0" | |
| }, | |
| "devDependencies": { | |
| "`@playwright/test`": "^1.50.1", | |
| "`@tanstack/router-e2e-utils`": "workspace:*", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/react-router/issue-7120/package.json` around lines 14 - 20, The
package.json file uses workspace:^ for internal workspace packages
(`@tanstack/react-router` and `@tanstack/router-e2e-utils`) but the repository
guideline requires workspace:* for all internal dependencies. Change both
`@tanstack/react-router` and `@tanstack/router-e2e-utils` entries from workspace:^
to workspace:* to align with the established guideline and prevent dependency
drift.
Source: Coding guidelines
| "resolveJsonModule": true, | ||
| "allowJs": true, | ||
| "skipLibCheck": true, | ||
| "types": ["vite/client"] |
There was a problem hiding this comment.
Include Node types for Playwright config type-checking.
Line 12 restricts global types to vite/client, but playwright.config.ts uses process.env (Line 18 there). With tsc --noEmit in Line 8 of package.json, this can fail type-checking due to missing Node globals.
Suggested patch
- "types": ["vite/client"]
+ "types": ["vite/client", "node"]📝 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.
| "types": ["vite/client"] | |
| "types": ["vite/client", "node"] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/react-router/issue-7120/tsconfig.json` at line 12, The types
configuration in tsconfig.json at the "types" field currently only includes
vite/client, which does not provide Node global type definitions. Since
playwright.config.ts uses process.env, the Node types must be added to the types
array to resolve the process global and satisfy type-checking when tsc --noEmit
is run. Add "node" to the types array alongside the existing "vite/client"
entry.
| "@tanstack/react-router": "workspace:^", | ||
| "@tanstack/router-plugin": "workspace:^", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify workspace protocol usage in this package
rg -n '"`@tanstack/`(react-router|router-plugin|router-e2e-utils)"\s*:\s*"workspace:\^"' \
e2e/react-router/issue-7457/package.jsonRepository: TanStack/router
Length of output: 208
Use workspace:* for internal dependencies in this package.
Lines 14–15 and 21 use workspace:^, but the coding guideline for **/package.json requires workspace:* for internal dependencies.
Suggested patch
- "`@tanstack/react-router`": "workspace:^",
- "`@tanstack/router-plugin`": "workspace:^",
+ "`@tanstack/react-router`": "workspace:*",
+ "`@tanstack/router-plugin`": "workspace:*",
@@
- "`@tanstack/router-e2e-utils`": "workspace:^",
+ "`@tanstack/router-e2e-utils`": "workspace:*",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/react-router/issue-7457/package.json` around lines 14 - 15, Replace the
version specifiers for internal dependencies in the package.json file from
workspace:^ to workspace:* to comply with the coding guideline for internal
dependency versioning. Specifically, change the version specifier for
`@tanstack/react-router` on line 14, `@tanstack/router-plugin` on line 15, and the
internal dependency on line 21 from workspace:^ to workspace:*.
Source: Coding guidelines
| component: () => { | ||
| const context = reloadInFlightIndexRoute.useRouteContext() | ||
| const number = () => context().number | ||
| sawUndefinedContext ||= number() === undefined | ||
|
|
||
| return ( | ||
| <div> | ||
| number = {String(number())}, saw undefined ={' '} | ||
| {String(sawUndefinedContext)} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant section
fd -t f "routeContext.test.tsx" 2>/dev/null | head -5Repository: TanStack/router
Length of output: 207
🏁 Script executed:
# Read the specific lines mentioned in the review
cat -n packages/solid-router/tests/routeContext.test.tsx | sed -n '2450,2530p'Repository: TanStack/router
Length of output: 3024
🏁 Script executed:
# Check the file for imports to see what's currently imported
head -50 packages/solid-router/tests/routeContext.test.tsxRepository: TanStack/router
Length of output: 1240
🏁 Script executed:
# Check the imports for Solid.js functions to see what's available
rg "import.*from.*solid-js" packages/solid-router/tests/routeContext.test.tsxRepository: TanStack/router
Length of output: 108
🏁 Script executed:
# Search for other uses of createSignal or createEffect in the test file to understand patterns used
rg "createSignal|createEffect" packages/solid-router/tests/routeContext.test.tsx | head -20Repository: TanStack/router
Length of output: 137
🏁 Script executed:
# Let's understand how useRouteContext behaves - check if it's reactive and can change during component lifetime
rg "useRouteContext" packages/solid-router/tests/routeContext.test.tsx -A 2 | head -30Repository: TanStack/router
Length of output: 981
🏁 Script executed:
# Check if there are other similar patterns in the test file to see how other tests track state changes
rg "createSignal" packages/solid-router/tests/routeContext.test.tsx -B 3 -A 3Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Look for documentation or examples of how context reactivity works in the codebase
fd -t f "*.tsx" packages/solid-router/tests | xargs grep -l "useRouteContext" | head -3Repository: TanStack/router
Length of output: 344
🏁 Script executed:
# Let's check how useRouteContext is implemented to understand if it can return undefined
rg "useRouteContext" packages/solid-router/src -A 5 | head -40Repository: TanStack/router
Length of output: 2582
🏁 Script executed:
# Look for context implementation and whether it becomes undefined during reloads
fd -type f -name "*.ts" -o -name "*.tsx" packages/solid-router/src | head -10Repository: TanStack/router
Length of output: 230
🏁 Script executed:
# Check the beforeLoad and context propagation logic
rg "beforeLoad" packages/solid-router/src -B 2 -A 2 | head -50Repository: TanStack/router
Length of output: 857
🏁 Script executed:
# Read the useRouteContext implementation to understand if it can return undefined
cat -n packages/solid-router/src/useRouteContext.tsRepository: TanStack/router
Length of output: 1248
🏁 Script executed:
# Check if the context can change during component lifetime by looking at route state management
rg "context.*undefined|undefined.*context" packages/solid-router/src -B 2 -A 2 | head -30Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Look at the test more carefully - does the context actually change, or is it always available?
sed -n '2454,2528p' packages/solid-router/tests/routeContext.test.tsxRepository: TanStack/router
Length of output: 2328
🏁 Script executed:
# Let's understand how context is managed during route transitions
# Check the useMatch implementation to see if it can become undefined
rg "useMatch" packages/solid-router/src/useMatch.ts -A 10 | head -50Repository: TanStack/router
Length of output: 137
🏁 Script executed:
# Look at how routes handle context during loaders and transitions
rg "context.*undefined|becoming undefined" packages/solid-router -B 3 -A 3Repository: TanStack/router
Length of output: 2926
🏁 Script executed:
# Check if there's documentation or comments about context stability during reloads
rg "reload.*context|context.*reload" packages/solid-router/tests -B 2 -A 2 | head -40Repository: TanStack/router
Length of output: 1902
🏁 Script executed:
# Let's check the useMatch implementation in solid-router
find packages/solid-router/src -name "*.ts" -o -name "*.tsx" | xargs grep -l "useMatch"Repository: TanStack/router
Length of output: 460
🏁 Script executed:
# Search for how match/context is obtained - look at the core import
rg "from '`@tanstack/router-core`'" packages/solid-router/src/useRouteContext.ts -A 5Repository: TanStack/router
Length of output: 213
🏁 Script executed:
# Let's see what useMatch does by finding its implementation
rg "export.*useMatch" packages/solid-router/src -B 2 -A 10Repository: TanStack/router
Length of output: 3521
🏁 Script executed:
# Check if context can become undefined by looking at route state during navigation
rg "staleTime.*0|background.*load|reload.*in.*flight" packages/solid-router -B 3 -A 3 | head -50Repository: TanStack/router
Length of output: 3814
🏁 Script executed:
# Let's read the useMatch.tsx implementation to understand how context updates work
cat -n packages/solid-router/src/useMatch.tsx | head -80Repository: TanStack/router
Length of output: 2737
🏁 Script executed:
# Check if createEffect is used in other component context tracking scenarios
rg "createEffect.*context|context.*createEffect" packages/solid-router/tests -B 3 -A 3Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Verify the test behavior - check if context is supposed to become undefined
rg "number\(\) === undefined|context.*undefined" packages/solid-router/tests/routeContext.test.tsx -B 3 -A 3Repository: TanStack/router
Length of output: 528
🏁 Script executed:
# Let's check more of the useMatch implementation to confirm how it's reactive
cat -n packages/solid-router/src/useMatch.tsx | sed -n '77,120p'Repository: TanStack/router
Length of output: 1526
🏁 Script executed:
# Verify Solid.js component lifecycle - does component function body re-run on reactivity changes?
web_searchRepository: TanStack/router
Length of output: 110
🌐 Web query:
Solid.js component function body execution once reactive dependencies
💡 Result:
In Solid.js, a component function is executed only once, at the time of its initial creation [1][2][3]. Because the component function is not re-run when its state or props change, the code inside the function body serves as a setup phase for initializing the reactive system [1][4]. To ensure updates occur in response to reactive dependencies, you must place those dependencies within reactive scopes, such as JSX expressions, createEffect, or createMemo [4]. When a signal or reactive value is accessed inside these scopes, Solid.js automatically tracks the dependency and updates only the affected part of the DOM, rather than re-running the component function [1][5][3]. Reading a signal directly in the component body will only capture its initial value and will not cause subsequent updates when that signal changes [4]. For example, to derive a value reactively: - Do not compute it directly in the component body (e.g., const val = signal * 2), as this will not update [5][4]. - Use a function wrapper or createMemo (e.g., const val = createMemo( => signal * 2)) to ensure the calculation is re-evaluated only when the dependency changes [4]. This "run-once" model is fundamental to Solid.js, allowing it to bypass the VDOM and re-rendering overhead typical of other frameworks [1][3]. The component function itself is wrapped in an untracked execution context during setup to prevent it from becoming a dependency of its own reactive contents [2].
Citations:
- 1: https://docs.solidjs.com/concepts/components/basics
- 2: https://deepwiki.com/solidjs/solid/2.2-component-model
- 3: https://app.studyraid.com/en/read/8387/231127/component-lifecycle
- 4: https://brenelz.com/posts/solid-js-best-practices/
- 5: Question: component doesn't update when property changes solidjs/solid#287
🏁 Script executed:
# Let's understand the test flow better - what does the test actually expect?
# Look for similar test patterns or comments
rg "undefined.*context|context.*undefined" packages/solid-router/tests/routeContext.test.tsx -B 5 -A 2 | head -60Repository: TanStack/router
Length of output: 1000
🏁 Script executed:
# Check if there are any issues or PRs related to this test
git log --oneline packages/solid-router/tests/routeContext.test.tsx | head -10Repository: TanStack/router
Length of output: 88
🏁 Script executed:
# Let's check if this test is currently passing or if it's a known issue
git log -p --all -S "sawUndefinedContext" packages/solid-router/tests/routeContext.test.tsx | head -100Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check if context could actually become undefined during the test scenario
rg "beforeLoad.*undefined|context.*undefined.*beforeLoad" packages/solid-router -B 3 -A 3Repository: TanStack/router
Length of output: 1523
🏁 Script executed:
# Let's get more git history to understand when this test was added and why
git log --oneline --all -- packages/solid-router/tests/routeContext.test.tsx | head -20Repository: TanStack/router
Length of output: 232
🏁 Script executed:
# Check the recent commit that added this test
git show 9698aeed7 --stat | head -30Repository: TanStack/router
Length of output: 1532
🏁 Script executed:
# Let's see the actual diff of when this test was added
git show 9698aeed7 -- packages/solid-router/tests/routeContext.test.tsx | head -150Repository: TanStack/router
Length of output: 4400
🏁 Script executed:
# Let's search for the specific issues this commit fixes
rg "7602|7210|7457" . --type txt --type md 2>/dev/null | head -20Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check the broader test to understand the full context lifecycle
sed -n '2454,2528p' packages/solid-router/tests/routeContext.test.tsx | cat -nRepository: TanStack/router
Length of output: 2853
🏁 Script executed:
# Look at how useRouteContext accessor behaves - can it become undefined mid-render?
rg "undefined" packages/solid-router/src/useMatch.tsx -B 2 -A 2 | head -50Repository: TanStack/router
Length of output: 1226
🏁 Script executed:
# Let's verify the component lifecycle more carefully - does the component stay mounted during loader reload?
# Check if re-entering the route causes a remount
rg "staleTime.*0" packages/solid-router/tests/routeContext.test.tsx -B 5 -A 15 | head -60Repository: TanStack/router
Length of output: 674
🏁 Script executed:
# Search for other tests that track post-mount state changes
rg "createEffect.*number\|number.*createEffect" packages/solid-router/tests -B 3 -A 3Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Verify - in the test scenario, does the component get unmounted/remounted, or stay mounted during reload?
# Check if there's a specific behavior for background reloads
rg "background.*reload|reload.*background|in.*flight" packages/solid-router -B 3 -A 3 | head -40Repository: TanStack/router
Length of output: 1778
🏁 Script executed:
# Let's check if createSignal is already imported in the test file
head -20 packages/solid-router/tests/routeContext.test.tsx | grep -E "import|createSignal"Repository: TanStack/router
Length of output: 370
🏁 Script executed:
# Verify the exact issue - when component rerenders due to reactive update, does the body re-run?
# Let's search for similar reactive state tracking patterns in the solid-router tests
rg "createSignal.*=>" packages/solid-router/tests -B 2 -A 5 | head -60Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Understand the actual behavior: does the component body execute multiple times or just once?
web_searchRepository: TanStack/router
Length of output: 110
🌐 Web query:
Solid.js component function body executes once vs JSX expressions reactive
💡 Result:
In Solid.js, the component function body executes exactly once, serving as a setup function that initializes the reactive graph [1][2][3]. Unlike frameworks that re-render entire components when state changes, Solid components do not re-run after their initial mount [1][4][5]. The reactive behavior is achieved through the following mechanisms: 1. Setup Phase: During the component's single execution, Solid initializes signals, stores, and effects [1][6]. The JSX is compiled by the framework into fine-grained DOM operations [7][2][4]. 2. Tracking Scope: When JSX expressions (e.g., {count}) are encountered, they create reactive computations (often internally treated as effects) [6][8][9]. These expressions are subscribed to the signals they access [8][9]. 3. Fine-Grained Updates: When a signal's value changes, Solid does not re-execute the component [1][2]. Instead, it triggers only the specific reactive computations (such as individual text nodes or attribute updates in the JSX) that depend on that signal [2][3][9]. Because the component function only runs once, reading a signal directly in the component body (outside of a tracking scope like a JSX expression, createMemo, or createEffect) will only capture the signal's initial value [4][5]. Subsequent updates to that signal will not be reflected because the code that accessed it is never re-run [4][10]. To maintain reactivity, signal access must occur within a tracking scope, most commonly by placing the signal call directly inside the JSX return or within reactive primitives [4][5].
Citations:
- 1: https://docs.solidjs.com/concepts/components/basics
- 2: https://ganeshjoshi.dev/blogs/solid-js-reactive-primitives
- 3: https://strapi.io/blog/solidjs-explained-fine-grained-reactive-framework
- 4: https://fixdevs.com/blog/solidjs-not-working/
- 5: https://docs.solidjs.com/concepts/intro-to-reactivity
- 6: https://deepwiki.com/solidjs/solid/2.2-component-model
- 7: https://deepwiki.com/solidjs/solid/2.3-jsx-compilation
- 8: https://stackoverflow.com/questions/75657763/in-solidjs-how-do-signals-get-repeatedly-rendered-into-the-dom
- 9: https://docs.solidjs.com/advanced-concepts/fine-grained-reactivity
- 10: https://stackoverflow.com/questions/76272753/solidjs-why-does-destructuring-of-props-cause-a-loss-of-reactivity
🏁 Script executed:
# Let's check the actual component behavior - does number() ever become undefined during the test?
# If it does, the JSX would show "undefined" in the rendered output
rg "number = undefined" packages/solid-router/tests/routeContext.test.tsxRepository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check if there are any other tests or issues that show context becoming undefined during reload
rg "undefined.*context|context.*undefined" packages/solid-router/tests/routeContext.test.tsx | head -20Repository: TanStack/router
Length of output: 462
🏁 Script executed:
# Look at the commit more carefully - what specific issues was "fix: match loading" addressing?
git log --all --grep="7602\|7210\|7457" --onelineRepository: TanStack/router
Length of output: 88
🏁 Script executed:
# Check if there's any code that could cause context to become undefined mid-component-lifecycle
rg "match.*undefined|selectedMatch.*undefined" packages/solid-router/src/useMatch.tsx -B 2 -A 2Repository: TanStack/router
Length of output: 325
Use reactive state tracking to properly detect context changes during component lifetime.
The boolean assignment sawUndefinedContext ||= number() === undefined only executes during component creation because Solid.js component functions run once as a setup phase. If the context becomes undefined after the component mounts (e.g., during the background loader reload), the flag and rendered output will not update. Convert to a reactive pattern using a signal and createEffect to ensure any context loss is properly detected and reflected in the rendered output.
-import { createEffect, onMount } from 'solid-js'
+import { createEffect, createSignal, onMount } from 'solid-js'- let sawUndefinedContext = false
+ const [sawUndefinedContext, setSawUndefinedContext] = createSignal(false) component: () => {
const context = reloadInFlightIndexRoute.useRouteContext()
const number = () => context().number
- sawUndefinedContext ||= number() === undefined
+ createEffect(() => {
+ if (number() === undefined) {
+ setSawUndefinedContext(true)
+ }
+ })
return (
<div>
number = {String(number())}, saw undefined ={' '}
- {String(sawUndefinedContext)}
+ {String(sawUndefinedContext())}
</div>
)
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/solid-router/tests/routeContext.test.tsx` around lines 2479 - 2487,
The `sawUndefinedContext` variable uses a one-time assignment during component
setup, so it will not detect if the context becomes undefined after the
component mounts. Replace the simple boolean assignment with a reactive pattern:
create a signal to store the `sawUndefinedContext` state and use `createEffect`
to reactively update it whenever `number()` changes, ensuring that any context
loss during the component's lifetime (such as during a background loader reload)
is properly detected and reflected in the rendered output.
9698aee to
3cb972f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-core/src/load-matches.ts (1)
599-610:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the resumed beforeLoad wait against evicted or replaced matches.
clearMatchPromisescan resolvependingBeforeLoadwhile cache cleanup removes the cached match. This continuation then dereferencesgetMatch(matchId)!, or can resume against a newer match with the same id. Capture the_nonReactivebucket and bail if the match disappeared or changed.Suggested guard
if (pendingBeforeLoad) { + const pendingBeforeLoadBucket = existingMatch._nonReactive return pendingBeforeLoad.then(() => { - const match = inner.router.getMatch(matchId)! + const match = inner.router.getMatch(matchId) + if (!match || match._nonReactive !== pendingBeforeLoadBucket) { + return + } if (match.preload && match.status === 'notFound') { handleRedirectOrNotFound(inner, match, match.error) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/src/load-matches.ts` around lines 599 - 610, The continuation after pendingBeforeLoad resolves can dereference a match that was evicted or replaced during cache cleanup. Before entering the pendingBeforeLoad.then() block in the if statement checking pendingBeforeLoad, capture a reference to the current match's _nonReactive bucket or state. Then, after pendingBeforeLoad resolves, validate that the match retrieved via getMatch(matchId) is still the same match instance (not a newly created replacement) by comparing against the captured reference. If the match has disappeared or been replaced with a different instance, bail out early before calling handleRedirectOrNotFound, getLoaderMatch, or executeBeforeLoad to prevent operating on stale or incorrect match data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/router-core/src/router.ts`:
- Around line 2882-2886: The current logic in the `beforeLoad` promise check
skips waiting if `_nonReactive.beforeLoadPromise` is undefined, but this causes
a race condition where active pending matches whose beforeLoad phase has not yet
started can be bypassed. For active matches, we need to ensure their beforeLoad
phase has completed before proceeding, even if the promise hasn't been created
yet. Add a mechanism to detect when a match is active and has a pending
beforeLoad phase that hasn't begun, and ensure we wait for it to start and
complete before the code continues with `settledMatch` substitution. This could
involve adding an explicit per-match beforeLoad phase gate when pending matches
are created, or adding additional checks to identify and wait for active matches
with not-yet-started beforeLoad phases.
---
Outside diff comments:
In `@packages/router-core/src/load-matches.ts`:
- Around line 599-610: The continuation after pendingBeforeLoad resolves can
dereference a match that was evicted or replaced during cache cleanup. Before
entering the pendingBeforeLoad.then() block in the if statement checking
pendingBeforeLoad, capture a reference to the current match's _nonReactive
bucket or state. Then, after pendingBeforeLoad resolves, validate that the match
retrieved via getMatch(matchId) is still the same match instance (not a newly
created replacement) by comparing against the captured reference. If the match
has disappeared or been replaced with a different instance, bail out early
before calling handleRedirectOrNotFound, getLoaderMatch, or executeBeforeLoad to
prevent operating on stale or incorrect match data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51059b2b-6851-4b78-993c-6741ca94f043
📒 Files selected for processing (5)
packages/react-router/tests/redirect.test.tsxpackages/router-core/src/load-matches.tspackages/router-core/src/router.tspackages/solid-router/tests/redirect.test.tsxpackages/vue-router/tests/redirect.test.tsx
💤 Files with no reviewable changes (3)
- packages/solid-router/tests/redirect.test.tsx
- packages/vue-router/tests/redirect.test.tsx
- packages/react-router/tests/redirect.test.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/router-core/src/load-matches.ts (2)
607-616:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing null checks after async operations in
queueExecution. Both sites use non-null assertions ongetMatchresults where the match may have been evicted during async operations, with null guards positioned after the problematic accesses.
packages/router-core/src/load-matches.ts#L607-L616: Guardmatchbefore accessingmatch.preloadafter awaitingpendingBeforeLoad.packages/router-core/src/load-matches.ts#L620-L623: Guardmatchbefore accessingmatch.preloadwhenloaderPromiseis pending.🛡️ Combined fix
if (pendingBeforeLoad) { return pendingBeforeLoad.then(() => { - const match = inner.router.getMatch(matchId)! - if (match.preload && match.status === 'notFound') { + const match = inner.router.getMatch(matchId) + if (match?.preload && match.status === 'notFound') { handleRedirectOrNotFound(inner, match, match.error) } if (!getLoaderMatch(inner, matchId)) { return } return executeBeforeLoad(inner, matchId, index, route) }) } - const match = inner.router.getMatch(matchId)! - if (match.preload && match.status === 'notFound') { + const match = inner.router.getMatch(matchId) + if (match?.preload && match.status === 'notFound') { handleRedirectOrNotFound(inner, match, match.error) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/src/load-matches.ts` around lines 607 - 616, The getMatch function in queueExecution may return null or have its result evicted during async operations, but the code accesses properties like match.preload on the non-null asserted result without verifying the match still exists. At packages/router-core/src/load-matches.ts lines 607-616, add a null check on the match variable returned from getMatch before accessing match.preload and match.status. At packages/router-core/src/load-matches.ts lines 620-623, add a similar null check on the match variable before accessing match.preload when handling the loaderPromise pending case. In both locations, ensure the match exists (is not null or undefined) immediately after calling getMatch, before attempting to access any of its properties.
905-921:⚠️ Potential issue | 🟡 MinorHandle
notFounderrors in background stale-while-revalidate reload, or document intentional behavior.The background IIFE catches errors but only handles
isRedirect. WhenrunLoaderthrows anotFounderror (viahandleNotFound), the error falls through the catch block without being re-thrown. This leaves the match in'notFound'status while the error is silently swallowed, creating inconsistent state:
- The match has already been patched to
status: 'notFound'byhandleNotFoundinsiderunLoader- The error is not re-thrown or handled
settleLoadPromisesstill runs, completing as if the load succeededFor proper SWR behavior, non-redirect errors should either be handled explicitly (similar to redirects) or re-thrown. Consider:
- Adding
isNotFoundhandling in the catch block (e.g., revert the status back to the previous state to preserve stale data)- Or re-throwing the error to let finalization handle it
- Or adding a comment explaining why silently swallowing is intentional
Note: There is test coverage for background redirects but none for background
notFounderrors, suggesting this case may not be intentionally supported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/src/load-matches.ts` around lines 905 - 921, The background IIFE in the async wrapper catches errors but only handles redirects via isRedirect check, allowing notFound errors from runLoader to be silently swallowed while the match is already patched to 'notFound' status by handleNotFound. Modify the catch block to explicitly handle notFound errors (check using isNotFound or similar) by either reverting the match status back to preserve stale data similar to how redirects are handled via router.navigate, or re-throw the error to let proper finalization occur. If silently swallowing is intentional, add a clear comment explaining this behavior. Consider whether settleLoadPromises should still execute in the notFound case or only in successful load scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/router-core/src/load-matches.ts`:
- Around line 607-616: The getMatch function in queueExecution may return null
or have its result evicted during async operations, but the code accesses
properties like match.preload on the non-null asserted result without verifying
the match still exists. At packages/router-core/src/load-matches.ts lines
607-616, add a null check on the match variable returned from getMatch before
accessing match.preload and match.status. At
packages/router-core/src/load-matches.ts lines 620-623, add a similar null check
on the match variable before accessing match.preload when handling the
loaderPromise pending case. In both locations, ensure the match exists (is not
null or undefined) immediately after calling getMatch, before attempting to
access any of its properties.
- Around line 905-921: The background IIFE in the async wrapper catches errors
but only handles redirects via isRedirect check, allowing notFound errors from
runLoader to be silently swallowed while the match is already patched to
'notFound' status by handleNotFound. Modify the catch block to explicitly handle
notFound errors (check using isNotFound or similar) by either reverting the
match status back to preserve stale data similar to how redirects are handled
via router.navigate, or re-throw the error to let proper finalization occur. If
silently swallowing is intentional, add a clear comment explaining this
behavior. Consider whether settleLoadPromises should still execute in the
notFound case or only in successful load scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0024442-7f0e-42dc-8215-e0c9b9fe6050
📒 Files selected for processing (1)
packages/router-core/src/load-matches.ts
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We fixed two code-change failures introduced by the PR's match loading refactor. The unnecessary non-null assertion (!) on inner.matches[index] in load-matches.ts was removed to satisfy the @typescript-eslint/no-unnecessary-type-assertion ESLint rule, since standard TypeScript array indexing already returns AnyRouteMatch without needing the assertion. We also updated the redirection in preload store-update count expectation in the Vue router test from 5 to 3, reflecting the PR's more efficient match loading which correctly produces fewer reactive updates for this navigation scenario.
Tip
✅ We verified this fix by re-running @tanstack/router-core:test:eslint, @tanstack/vue-router:test:unit.
diff --git a/packages/router-core/src/load-matches.ts b/packages/router-core/src/load-matches.ts
index 4d2d22b6..9cc9778d 100644
--- a/packages/router-core/src/load-matches.ts
+++ b/packages/router-core/src/load-matches.ts
@@ -86,7 +86,7 @@ const joinActivePreloadMatch = async (
match._nonReactive.loaderPromise || match._nonReactive.loadPromise
if (loaderPromise?.status === 'pending') {
await loaderPromise
- match = inner.router.getMatch(matchId) ?? inner.matches[index]!
+ match = inner.router.getMatch(matchId) ?? inner.matches[index]
}
inner.matches[index] = match
error = match._nonReactive.error || match.error
diff --git a/packages/vue-router/tests/store-updates-during-navigation.test.tsx b/packages/vue-router/tests/store-updates-during-navigation.test.tsx
index 3a51d638..6ce87e8a 100644
--- a/packages/vue-router/tests/store-updates-during-navigation.test.tsx
+++ b/packages/vue-router/tests/store-updates-during-navigation.test.tsx
@@ -157,7 +157,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: Vue has different update counts than React/Solid due to different reactivity
- expect(updates).toBe(5)
+ expect(updates).toBe(3)
expect(
router.stores.cachedMatches
.get()
Or Apply changes locally with:
npx nx-cloud apply-locally B2ee-K2Av
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
schiller-manuel
force-pushed
the
fix-match-loading
branch
from
2bd2ef7 to
1d394e1
Compare
2 participants
fixes #7602
fixes #7120
fixes #7457
fixes #7635
Summary by CodeRabbit
Release Notes
Bug Fixes
beforeLoadcontext values into sub-routes during background reloads, including whenbeforeLoadthrows.Documentation
RouteMatch.statuslifecycle docs to remove theredirectedstate.Tests