fix: wait for async loaders before executing head functions #6222
Conversation
The bug: When loaders run asynchronously (stale-while-revalidate), loadRouteMatch returns immediately while the loader runs in the background. Promise.allSettled(inner.matchPromises) waits for loadRouteMatch promises, not the actual loader completion, causing head() to execute with undefined loaderData. The fix: After loadRouteMatch promises settle, explicitly wait for all loaderPromises to complete before executing head functions. This ensures loaderData is available when head() executes. Reproduction scenario: 1. Navigate to authenticated route (e.g., /article/123) 2. Delete auth cookie, reload (shows 'not found') 3. Login, redirect to dashboard 4. Click back button to /article/123 - Before fix: Article loads but title shows fallback (loaderData undefined) - After fix: Article loads with correct title (loaderData available) Fixes the issue identified in PR TanStack#6093 follow-up investigation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
📝 WalkthroughWalkthroughRun all route head() functions serially after initial loaders finish; detect in-flight loaders and schedule a non-blocking re-run of head() once pending loaders settle (guarded by navigation-locality); ensure loader promises always resolve and preserve per-match error/redirect/notFound semantics. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Router
participant Loaders as "Loader(s)"
participant HeadFns as "head() functions"
participant Scheduler as "AsyncRe-run Scheduler"
Client->>Router: navigate to /match
Router->>Loaders: invoke match loaders (parallel)
note right of Loaders `#f0f7ff`: some loader promises may remain pending
Loaders-->>Router: some loader results resolved/rejected
Router->>HeadFns: call executeAllHeadFns(inner) serially
HeadFns-->>Router: apply per-match head data (errors caught)
alt in-flight async loaders detected
Router->>Scheduler: schedule non-blocking re-run when pending settle
Scheduler->>Loaders: await pending loader promises (allSettled)
Loaders-->>Scheduler: pending loaders settled
alt navigation still current
Scheduler->>HeadFns: re-invoke executeAllHeadFns(inner)
HeadFns-->>Router: refreshed head data applied
else navigation changed
Scheduler-->>Router: abort re-run (stale)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 9m 3s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 41s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-12-26 09:13:28 UTC
| // Wait for async loaders to complete before executing head functions | ||
| // loadRouteMatch may return immediately while loaders run asynchronously in the background | ||
| // We need to wait for the actual loaderPromise, not just the loadRouteMatch promise | ||
| await Promise.all( |
There was a problem hiding this comment.
we definitely don't want to block for async loaders.
There was a problem hiding this comment.
I guess it depends on what we're trying to do in the head function, right? For setting the document title, blocking until the loaderData is ready at least gets us the correct title, while not blocking, under certain scenarios, can get us nothing. Also, for page title, is blocking that bad?
Or do you see a non-blocking way to ensure page title is returned reliably? Thanks.
There was a problem hiding this comment.
we probably need to detect if we have async loaders and then schedule to serially re-run all head() functions
There was a problem hiding this comment.
Are you saying, we let the head functions run a first-pass, non-blocking (current behavior), and then we check if there're any remaining unresolved loader promises, and if yes, then we await them (Promise.all), and then run the head functions for the second time?
There was a problem hiding this comment.
possibly yes. the second run of head() must also run detached like the async loaders itself do
There was a problem hiding this comment.
made an attempt, let me know if that could work?
When loaders run asynchronously (stale-while-revalidate), head() executes immediately but may have stale loaderData. This change detects async loaders and schedules a re-execution of all head() functions after they complete, ensuring fresh metadata without blocking navigation. Benefits: - Non-blocking: navigation completes immediately - Correct metadata: title updates when fresh data arrives - Consistent: all head() functions see complete loader state 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Extracted head() execution loop into a reusable helper function to eliminate code duplication between initial execution and re-execution after async loaders complete. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/router-core/src/load-matches.ts (1)
955-957: Add error handling for the detached promise to avoid unhandled rejections.The re-execution is correctly non-blocking per maintainer feedback. However, the returned promise from
executeAllHeadFnsisn't handled, which could lead to unhandled promise rejections if an unexpected error occurs (e.g., router state corruption).Proposed fix
if (asyncLoaderPromises.length > 0) { // Schedule re-execution after all async loaders complete (non-blocking) - Promise.all(asyncLoaderPromises).then(() => executeAllHeadFns(inner)) + Promise.all(asyncLoaderPromises) + .then(() => executeAllHeadFns(inner)) + .catch((err) => { + console.error('Error re-executing head functions after async loaders:', err) + }) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/load-matches.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/load-matches.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/load-matches.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (2)
packages/router-core/src/load-matches.ts (2)
586-605: LGTM! Clean extraction of head execution logic.The helper correctly implements serial execution with per-match error handling, ensuring one failing
head()doesn't prevent others from running. The error logging approach is appropriate for this non-critical path.
948-953: Logic for detecting async loaders is correct.The promise collection correctly identifies matches with in-flight async loaders by checking
_nonReactive.loaderPromise, which is cleared upon completion (lines 845, 865). The optional chaining and.filter(Boolean)pattern safely handles edge cases.
|
|
||
| if (asyncLoaderPromises.length > 0) { | ||
| // Schedule re-execution after all async loaders complete (non-blocking) | ||
| Promise.all(asyncLoaderPromises).then(() => executeAllHeadFns(inner)) |
There was a problem hiding this comment.
can the detached loaders throw? if yes, should we execute the head chain? probably yes
There was a problem hiding this comment.
we should probably store a reference to this promise so we can override it in case a new round of loaders has already started before the detached head() executed.
what if this happens while the detached head() runs?
There was a problem hiding this comment.
Comparing location before executing the head seems to work? I tested with a slow fetcher, and navigate away before it can finish, and that check prevents the head function from executing.
Now if the head already starts executing, and we navigate to a new route, then the new head will execute next, and override the old value, hopefully? Not sure. Will think about it more after sleep. Insights/advice welcome as always.
There was a problem hiding this comment.
comparing location is not enough IMO as you can stay on the same location and invalidate and trigger loaders.
|
we need tests for this. unit tests and e2e test |
Async loaders can throw errors. When this happens, head() functions should still execute to set page metadata (e.g., error page titles). The fix: 1. Use try-catch-finally to always resolve loaderPromise 2. Change Promise.all() to Promise.allSettled() Why resolve on error: - loaderPromise signals "completion" not "success" - Errors are already stored in match.error by runLoader - head() should execute with whatever state is available (success, error, or stale data) - Without this, the promise hangs forever and head() never re-executes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
…execution Addresses two scenarios: 1. New navigation starts BEFORE scheduled head() executes 2. New navigation starts WHILE head() is executing Solution: - Capture this navigation's location (thisNavigationLocation) - Before executing head(), check if router's current location matches - If location changed (new navigation), skip stale head() execution - Location objects are always unique (parseLocation creates new objects) Both concerns are resolved: - Scenario 1: Location check prevents stale head() from executing - Scenario 2: Stale head() may complete but fresh navigation overwrites with correct data immediately after 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/router-core/src/load-matches.ts (4)
586-605: Consider enhancing error handling and add early exit for efficiency.The current implementation logs errors to
console.error, which may not integrate with the router's error handling or observability strategy. Additionally, the function processes all matches even when they don't have head functions defined.🔎 Suggested improvements
1. Enhanced error handling:
} catch (err) { - console.error(`Error executing head for route ${routeId}:`, err) + // Consider using router's error handling mechanism or a callback + console.error(`Error executing head for route ${routeId}:`, err) + route.options.onError?.(err) }2. Skip matches without head functions:
for (const match of inner.matches) { const { id: matchId, routeId } = match const route = inner.router.looseRoutesById[routeId]! + // Skip if no head-related functions are defined + if (!route.options.head && !route.options.scripts && !route.options.headers) { + continue + } try {
947-970: Consider preventing concurrent head() executions.The location check at line 966 prevents stale executions when a new navigation starts before the scheduled re-execution begins. However, if a new navigation starts during the
executeAllHeadFnscall itself, the currently executing head functions will complete even though they're now stale.Based on past review comments, this was a concern raised by schiller-manuel: "what if this happens while the detached head() runs?"
🔎 Potential solution using an execution token
Store a reference to track the current execution and cancel/ignore stale ones:
+ let currentHeadExecutionToken = Symbol() + inner.router._currentHeadExecutionToken = currentHeadExecutionToken + if (asyncLoaderPromises.length > 0) { const thisNavigationLocation = inner.location Promise.allSettled(asyncLoaderPromises).then(() => { const latestLocation = inner.router.state.location - if (latestLocation === thisNavigationLocation) { + if (latestLocation === thisNavigationLocation && + inner.router._currentHeadExecutionToken === currentHeadExecutionToken) { executeAllHeadFns(inner) } })Then in
executeAllHeadFns, check the token before updating each match. However, this may be over-engineering for the "Chill" review mode.Alternatively, verify whether the existing architecture already handles this scenario gracefully (e.g., if
updateMatchis idempotent or location changes automatically invalidate head updates).
952-970: Optional: Consider selective head() re-execution.Currently, all head functions are re-executed after async loaders complete, even if only a subset of matches had async loaders. While this ensures consistency (and handles cases where one match's head depends on another's loader data), it may result in unnecessary work.
🔎 Alternative: Track which matches have async loaders
- const asyncLoaderPromises = inner.matches + const matchesWithAsyncLoaders = inner.matches .map((match) => { const currentMatch = inner.router.getMatch(match.id) - return currentMatch?._nonReactive.loaderPromise + return currentMatch?._nonReactive.loaderPromise ? match : null }) - .filter(Boolean) + .filter((m): m is AnyRouteMatch => m !== null) - if (asyncLoaderPromises.length > 0) { + if (matchesWithAsyncLoaders.length > 0) { + const asyncLoaderPromises = matchesWithAsyncLoaders.map( + (m) => inner.router.getMatch(m.id)?._nonReactive.loaderPromise! + ) const thisNavigationLocation = inner.location Promise.allSettled(asyncLoaderPromises).then(() => { const latestLocation = inner.router.state.location if (latestLocation === thisNavigationLocation) { - executeAllHeadFns(inner) + // Re-execute only affected matches + // (or keep current behavior if heads depend on each other) + executeAllHeadFns(inner) } })However, keeping the current approach (re-executing all heads) is safer and simpler, as it handles cascading dependencies where one match's head might depend on another's loader data.
586-970: Tests are needed for the new head execution behavior.As requested in the past review comments by schiller-manuel, this change requires both unit tests and e2e tests to cover:
- Head functions executing after synchronous loaders complete
- Head functions re-executing after async (stale-while-revalidate) loaders complete
- Head functions receiving correct loaderData in both executions
- Race condition handling when navigation occurs during async loader execution
- Error handling when head functions or async loaders throw
Based on past review comments requesting tests for this change.
Would you like me to help draft test scenarios or open an issue to track the test implementation?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/load-matches.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/load-matches.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/load-matches.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (1)
packages/router-core/src/load-matches.ts (1)
846-854: Good approach to ensure head() executes regardless of loader outcome.The finally block correctly ensures that loader promises are always resolved (even on error), allowing
head()functions to execute with whatever data is available. This aligns with the PR objective and addresses the concern from past comments about handling errors from detached loaders.
There's one e2e test in a local branch that I'm using, is there any guideline for where to place it, and how to name it? It's currently called |
|
please use an existing e2e project (e2e/react-start/basic for example) and add new routes in there and playwright tests |
Adds test routes demonstrating the head() re-execution fix: - /test-head/article/$id - auth-gated article with dynamic title - /test-head/dashboard - simple dashboard for navigation - /fake-login - simulates login with localStorage Testing flow: 1. Visit /test-head/article/123 (unauthenticated) → Shows "Article Not Found" title & content 2. Click login link → simulate login → redirects to dashboard 3. Press browser BACK button → Article content loads correctly → Page title updates from stale to "Article 123 Title" → Console shows head() executing twice (non-blocking fix) Note: fake-auth.ts uses localStorage for auth state 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Adds comprehensive playwright tests verifying the head() re-execution fix: - Verifies page title updates correctly on back navigation after login - Tests fallback title when loader returns null - Tests logout flow with correct title updates - Verifies race condition handling with rapid navigation Also fixes package.json start script to use 'pnpm dlx' instead of deprecated 'pnpx' command for compatibility with modern pnpm versions. All tests passing, confirming non-blocking head() re-execution works correctly after async loaders complete. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
e2e/solid-start/basic-solid-query/tests/head.spec.ts (1)
94-98: Minor: redundant assertion.The
expect(title).toBe(...)on line 96 already verifies the title matches. The follow-up assertion on line 97 is redundant.🔎 Suggested simplification
// Title should match the current route, not the previous one - const title = await page.title() - expect(title).toBe('Article 222 Title') - expect(title).not.toBe('Article 111 Title') + await expect(page).toHaveTitle('Article 222 Title')e2e/solid-start/basic-solid-query/src/routes/test-head/article.$id.tsx (1)
30-36: Consider removing the debug log before merging.The
console.logon line 32 is useful during development but may add noise to test output.🔎 Suggested change
head: ({ loaderData }) => { const title = loaderData?.title || 'Article Not Found' - console.log('[!] head function: title =', title) return { meta: [{ title }], } },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e/solid-start/basic-solid-query/package.jsone2e/solid-start/basic-solid-query/src/routeTree.gen.tse2e/solid-start/basic-solid-query/src/routes/fake-login.tsxe2e/solid-start/basic-solid-query/src/routes/test-head/article.$id.tsxe2e/solid-start/basic-solid-query/src/routes/test-head/dashboard.tsxe2e/solid-start/basic-solid-query/src/utils/fake-auth.tse2e/solid-start/basic-solid-query/tests/head.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace protocol
workspace:*for internal dependencies in package.json files
Files:
e2e/solid-start/basic-solid-query/package.json
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
e2e/solid-start/basic-solid-query/src/utils/fake-auth.tse2e/solid-start/basic-solid-query/src/routes/test-head/dashboard.tsxe2e/solid-start/basic-solid-query/src/routes/fake-login.tsxe2e/solid-start/basic-solid-query/src/routes/test-head/article.$id.tsxe2e/solid-start/basic-solid-query/tests/head.spec.tse2e/solid-start/basic-solid-query/src/routeTree.gen.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
e2e/solid-start/basic-solid-query/src/utils/fake-auth.tse2e/solid-start/basic-solid-query/src/routes/test-head/dashboard.tsxe2e/solid-start/basic-solid-query/src/routes/fake-login.tsxe2e/solid-start/basic-solid-query/src/routes/test-head/article.$id.tsxe2e/solid-start/basic-solid-query/tests/head.spec.tse2e/solid-start/basic-solid-query/src/routeTree.gen.ts
🧠 Learnings (8)
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/solid-start/basic-solid-query/src/routes/test-head/dashboard.tsxe2e/solid-start/basic-solid-query/src/routes/test-head/article.$id.tsxe2e/solid-start/basic-solid-query/src/routeTree.gen.ts
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
e2e/solid-start/basic-solid-query/src/routes/test-head/dashboard.tsxe2e/solid-start/basic-solid-query/src/routeTree.gen.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Use file-based routing in `src/routes/` directories or code-based routing with route definitions
Applied to files:
e2e/solid-start/basic-solid-query/src/routes/test-head/dashboard.tsxe2e/solid-start/basic-solid-query/src/routeTree.gen.ts
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
e2e/solid-start/basic-solid-query/src/routes/fake-login.tsxe2e/solid-start/basic-solid-query/src/routeTree.gen.ts
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/solid-start/basic-solid-query/tests/head.spec.ts
📚 Learning: 2025-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
e2e/solid-start/basic-solid-query/src/routeTree.gen.ts
📚 Learning: 2025-12-21T12:52:35.231Z
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:35.231Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Applied to files:
e2e/solid-start/basic-solid-query/src/routeTree.gen.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
e2e/solid-start/basic-solid-query/src/routeTree.gen.ts
🧬 Code graph analysis (3)
e2e/solid-start/basic-solid-query/src/routes/fake-login.tsx (1)
e2e/solid-start/basic-solid-query/src/utils/fake-auth.ts (1)
authQy(8-13)
e2e/solid-start/basic-solid-query/src/routes/test-head/article.$id.tsx (2)
e2e/solid-start/basic-solid-query/src/utils/fake-auth.ts (2)
isAuthed(4-6)authQy(8-13)e2e/solid-start/basic-solid-query/src/routes/fake-login.tsx (1)
Route(5-11)
e2e/solid-start/basic-solid-query/src/routeTree.gen.ts (2)
e2e/react-start/basic-react-query/src/routeTree.gen.ts (4)
FileRoutesByTo(138-153)FileRoutesById(154-174)FileRouteTypes(175-231)RootRouteChildren(232-243)e2e/solid-start/query-integration/src/routeTree.gen.ts (4)
FileRoutesByTo(37-41)FileRoutesById(42-47)FileRouteTypes(48-55)RootRouteChildren(56-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (8)
e2e/solid-start/basic-solid-query/package.json (1)
11-11: LGTM!Using
pnpm dlxinstead ofpnpxis the correct and modern approach for executing packages with pnpm.e2e/solid-start/basic-solid-query/src/routes/test-head/dashboard.tsx (1)
1-45: LGTM!Well-structured test route with proper route definition, static head metadata, and appropriate
data-testidattributes for Playwright tests. The instructional content serves as useful documentation for manual testing of the fix.e2e/solid-start/basic-solid-query/src/utils/fake-auth.ts (1)
1-13: LGTM!Clean implementation of client-only auth utilities. Using
createClientOnlyFnensures SSR safety forlocalStorageaccess, and the query configuration with disabledrefetchOnWindowFocusis appropriate for stable e2e testing.e2e/solid-start/basic-solid-query/tests/head.spec.ts (2)
3-8: Good test isolation setup.Clearing
localStoragebefore each test ensures tests don't affect each other. The navigation to/first ensures the page context is established before clearing storage.
10-48: Excellent coverage of the core bug scenario.This test precisely reproduces the issue described in #6221: the back navigation after login should update the page title from the stale "Article Not Found" to the correct article title. The step-by-step comments make the test intention clear.
e2e/solid-start/basic-solid-query/src/routes/fake-login.tsx (1)
1-43: LGTM!Well-implemented login simulation route. Key design choices are sound:
- SSR disabled for client-only auth state
- Query invalidation ensures auth cache is refreshed
replace: trueremoves login from history stack, enabling the back-button test scenario- Consistent use of
authQy.queryKeyfor query invalidatione2e/solid-start/basic-solid-query/src/routes/test-head/article.$id.tsx (2)
6-21: Solid test implementation.The simulated async loader with authentication gating correctly reproduces the stale-while-revalidate scenario. The 200ms delay ensures the async timing behavior is exercised.
41-86: Good component structure for testing both states.The use of
Showfor conditional rendering based on auth query state, combined with separate loader data access, correctly exercises both the head() timing and UI rendering paths. The logout mechanism usingwindow.location.reload()ensures a clean state reset for test verification.
Fixes race condition where async loader's finally block executes after navigation has changed, causing match to be undefined when accessing _nonReactive properties. This resolves 11 unhandled rejection errors in unit tests: - TypeError: Cannot read properties of undefined (reading '_nonReactive') - All errors pointed to load-matches.ts:851 in async callback's finally block The fix adds a null check before accessing match._nonReactive to handle cases where the match has been removed from the router during async execution. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/router-core/src/load-matches.ts (2)
586-605: Consider using router-specific error handling.The function logs errors using
console.error, which may not integrate with the router's error handling or logging mechanisms. While this ensures errors don't halt head execution for other routes, consider whether these errors should be:
- Stored in match state for observability
- Reported through a router-level error handler
- Surfaced differently in development vs production
</comment_end>
960-960: Strengthen type safety of the filter operation.The
.filter(Boolean)removes falsy values but could be more explicit for TypeScript strict mode compliance.🔎 Proposed type-safe filter
- .filter(Boolean) + .filter((p): p is NonNullable<typeof p> => p !== null && p !== undefined)Or more concisely with a type predicate:
- .filter(Boolean) + .filter((p): p is Exclude<typeof p, null | undefined> => Boolean(p))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/load-matches.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/load-matches.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/load-matches.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (2)
packages/router-core/src/load-matches.ts (2)
846-857: Good defensive fix for navigation race condition.The null check properly handles the edge case where a match might be removed during async loader execution due to navigation changes. This prevents the TypeError mentioned in the commit message and ensures promises resolve correctly to allow head() execution.
</comment_end>
955-973: Location guard via reference equality correctly prevents stale head() execution in invalidation scenarios.The concern raised about location comparison being insufficient for invalidation is unfounded. When
invalidate()is called on the same route:
load()→beforeLoad()→updateLatestLocation()creates a new ParsedLocation object reference each time viaparseLocation()- The new reference is stored in
state.location- The previously scheduled head() from the earlier
loadMatchescomparesstate.location === thisNavigationLocationusing reference equality- Since a new ParsedLocation reference was created, the comparison fails (
B !== A), correctly preventing stale execution- A fresh
loadMatchesinvocation runs with the new location referenceThis design correctly handles all invalidation scenarios without requiring additional generation counters. The reference equality check is sufficient because new location objects are always created on each load cycle.
Moving the promise resolution from finally block back to try block. This ensures promises are only resolved on successful loader completion, not on errors or redirects. Resolving on redirect was incorrect because we're navigating away from the route, so head() re-execution for the old route doesn't make sense. The null check is kept as a safety measure since this code still runs in an async callback where navigation could theoretically change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/load-matches.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/load-matches.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/load-matches.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (2)
packages/router-core/src/load-matches.ts (2)
586-605: LGTM: Centralizes head execution with proper error isolation.The new
executeAllHeadFnsfunction correctly serializes head execution across all matches and ensures that errors in one head function don't prevent others from running. The per-match try-catch with logging is a solid pattern.
842-848: LGTM: Good defensive guard against navigation race conditions.This null check correctly handles the case where navigation changes while an async loader is running, preventing errors when attempting to resolve promises on a match that no longer exists. This is consistent with similar guards elsewhere in the codebase (e.g., line 550-554 in
executeHead).
|
can you please move this into e2e/react-start/basic? |
This commit adds comprehensive null checks to prevent crashes when match is undefined due to navigation changes during async operations. Two critical fixes: 1. Finally block for async loaders (lines 846-855): - Ensures promises always settle (success, error, or redirect) - Required for Promise.allSettled() to complete properly - Enables correct store updates during navigation - Location check (line 969) prevents stale head() execution 2. Null check after async callback (lines 860-871): - Protects clearTimeout and other _nonReactive accesses - Hypothesis: The original 11 errors were from line 866, not the async callback - This line was using getMatch(matchId)! without protection Root cause: When navigation changes during async loader execution, getMatch(matchId) returns undefined, causing crashes when accessing _nonReactive properties. Fixes: - 11 unhandled rejection errors (TypeError accessing _nonReactive) - 2 test failures (link test + store updates with redirect) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/router-core/src/load-matches.ts (1)
962-972: Critical issues with scheduled head re-execution remain unaddressed.The scheduled head re-execution has two critical problems that were flagged in previous reviews:
Location comparison is insufficient: The guard
latestLocation === thisNavigationLocationfails to detect new load cycles whenrouter.invalidate()is called on the same route. The location remains unchanged during invalidation, but a new load cycle begins with fresh loaders. This allows stale scheduled executions to overwrite fresher head data.Missing error handler: The
Promise.allSettled().then()chain lacks a.catch()handler, creating potential unhandled promise rejections ifexecuteAllHeadFnsthrows or rejects.Additional concerns:
- No stored reference to the scheduled promise, so if a new
loadMatchescycle starts before this completes, there's no way to cancel the stale execution.- Multiple rapid invalidations at the same location can cause older scheduled executions to overwrite newer data.
🔎 Recommended fixes
Solution 1: Track load cycle with a unique identifier
Consider adding a unique load cycle identifier (e.g., incrementing counter or UUID) to
InnerLoadContextand capture it when scheduling:+ const thisLoadCycleId = inner.router.state.loadCycleId // or generate UUID const thisNavigationLocation = inner.location Promise.allSettled(asyncLoaderPromises).then(() => { - const latestLocation = inner.router.state.location - if (latestLocation === thisNavigationLocation) { + const isStillCurrent = inner.router.state.loadCycleId === thisLoadCycleId + if (isStillCurrent) { executeAllHeadFns(inner) } + }).catch((err) => { + inner.router.options.logger?.error('Error in scheduled head re-execution:', err) })Solution 2: Store and cancel stale executions
Store a reference to the scheduled promise in router state and cancel it when a new load starts:
+ // Cancel any pending head re-execution from previous load + inner.router.state.pendingHeadReExecution?.cancel() + const thisNavigationLocation = inner.location - Promise.allSettled(asyncLoaderPromises).then(() => { + const scheduledExecution = Promise.allSettled(asyncLoaderPromises).then(() => { const latestLocation = inner.router.state.location if (latestLocation === thisNavigationLocation) { executeAllHeadFns(inner) } + }).catch((err) => { + inner.router.options.logger?.error('Error in scheduled head re-execution:', err) }) + + inner.router.state.pendingHeadReExecution = { + promise: scheduledExecution, + cancel: () => { /* set flag to skip execution */ } + }At minimum, add the
.catch()handler to prevent unhandled rejections.
🧹 Nitpick comments (1)
packages/router-core/src/load-matches.ts (1)
586-605: Consider using router's logger instead of console.error.The error logging uses
console.error, but the router likely has a configured logger (e.g.,inner.router.options.logger) that should be preferred for consistency with the rest of the codebase.🔎 Proposed refactor
} catch (err) { - console.error(`Error executing head for route ${routeId}:`, err) + inner.router.options.logger?.error(`Error executing head for route ${routeId}:`, err) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/load-matches.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/load-matches.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/load-matches.ts
🧠 Learnings (2)
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/router-core/src/load-matches.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/router-core/src/load-matches.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (2)
packages/router-core/src/load-matches.ts (2)
846-855: Good defensive coding for navigation edge cases.The null check for
matchbefore resolving promises properly handles the scenario where navigation changes while an async loader is running, preventing crashes when matches become undefined.
862-873: LGTM: Null-safe access pattern.The null check for
matchbefore accessing its properties is consistent with the defensive pattern introduced in the finally block and appropriately guards against navigation changes.
The null check at line 862 was incorrect. That code runs synchronously (doesn't wait for async loader), so match exists there. Only the async callback's finally block needs null check for race conditions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/router-core/src/load-matches.ts (1)
963-970: Location comparison cannot detect same-route invalidation.As noted in previous review discussions, comparing
locationobjects is insufficient becauserouter.invalidate()on the same route triggers new loaders without changing the location. This allows stale scheduled head executions to proceed and potentially overwrite fresher data.Scenario: User navigates to
/article/123, async loaders start, thenrouter.invalidate()is called before loaders complete. New loaders start butthisNavigationLocation === latestLocationstill passes, allowing the old scheduled execution to run with stale data.Without a stored reference to the scheduled promise or a more robust navigation tracking mechanism (e.g., using a generation ID or load cycle counter), multiple invalidations on the same route can cause older executions to overwrite newer ones.
Consider one of these approaches:
- Introduce a per-load-cycle identifier (generation counter) and capture it alongside the location
- Store a reference to the scheduled promise and replace it if new loaders start
- Check if the captured
asyncLoaderPromisesare still the current ones before re-executing headsBased on learnings, the router may already track navigation state that could be leveraged here.
🧹 Nitpick comments (1)
packages/router-core/src/load-matches.ts (1)
948-971: Tests are needed to verify the fix.As discussed in the PR comments, tests are required for this change. The maintainer requested moving the e2e test from
e2e/solid-start/test-head-bugintoe2e/react-start/basicwith Playwright tests covering the reproduction scenario (navigate to authenticated article route, lose auth and reload, log in and redirect, use Back button to return to article).Do you need help writing the Playwright test for this scenario, or would you like me to open a separate issue to track the test addition?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/load-matches.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/load-matches.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/load-matches.ts
🧠 Learnings (2)
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/router-core/src/load-matches.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/router-core/src/load-matches.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (3)
packages/router-core/src/load-matches.ts (3)
586-605: Well-structured serial head execution with proper error isolation.The
executeAllHeadFnsfunction correctly executes head functions serially for all matches, with per-match try-catch blocks ensuring one failure doesn't block others. This approach prevents race conditions when updating match data.
846-855: Critical fix: Ensures loader promises always resolve.The finally block correctly handles the race condition where a match may become undefined during navigation while async loaders are running. Always resolving the promises ensures
Promise.allSettledworkflows can complete without hanging, which is essential for the scheduled head re-execution to work correctly.
948-949: LGTM: Initial head execution after loaders complete.Executing head functions after all loaders have settled (successfully or not) ensures the initial render has the most complete data available at that point.
| if (asyncLoaderPromises.length > 0) { | ||
| // Schedule re-execution after all async loaders complete (non-blocking) | ||
| // Use allSettled to handle both successful and failed loaders | ||
| const thisNavigationLocation = inner.location | ||
| Promise.allSettled(asyncLoaderPromises).then(() => { | ||
| // Only execute if this navigation is still current (not superseded by new navigation) | ||
| const latestLocation = inner.router.state.location | ||
| if (latestLocation === thisNavigationLocation) { | ||
| executeAllHeadFns(inner) | ||
| } | ||
| } catch (err) { | ||
| // Log error but continue executing other head functions | ||
| console.error(`Error executing head for route ${routeId}:`, err) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Missing error handler risks unhandled promise rejection.
The detached promise chain (lines 964-970) lacks a .catch() handler. If executeAllHeadFns throws or returns a rejected promise during the scheduled re-execution, this will produce an unhandled rejection. Past review comments indicated this was addressed in commit 90d4ba3, but the error handler is not present in the current code.
🔎 Add error handling to prevent unhandled rejections
const thisNavigationLocation = inner.location
Promise.allSettled(asyncLoaderPromises).then(() => {
// Only execute if this navigation is still current (not superseded by new navigation)
const latestLocation = inner.router.state.location
if (latestLocation === thisNavigationLocation) {
executeAllHeadFns(inner)
}
- })
+ }).catch((err) => {
+ console.error('Error in scheduled head re-execution:', err)
+ })
}📝 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.
| if (asyncLoaderPromises.length > 0) { | |
| // Schedule re-execution after all async loaders complete (non-blocking) | |
| // Use allSettled to handle both successful and failed loaders | |
| const thisNavigationLocation = inner.location | |
| Promise.allSettled(asyncLoaderPromises).then(() => { | |
| // Only execute if this navigation is still current (not superseded by new navigation) | |
| const latestLocation = inner.router.state.location | |
| if (latestLocation === thisNavigationLocation) { | |
| executeAllHeadFns(inner) | |
| } | |
| } catch (err) { | |
| // Log error but continue executing other head functions | |
| console.error(`Error executing head for route ${routeId}:`, err) | |
| } | |
| }) | |
| if (asyncLoaderPromises.length > 0) { | |
| // Schedule re-execution after all async loaders complete (non-blocking) | |
| // Use allSettled to handle both successful and failed loaders | |
| const thisNavigationLocation = inner.location | |
| Promise.allSettled(asyncLoaderPromises).then(() => { | |
| // Only execute if this navigation is still current (not superseded by new navigation) | |
| const latestLocation = inner.router.state.location | |
| if (latestLocation === thisNavigationLocation) { | |
| executeAllHeadFns(inner) | |
| } | |
| }).catch((err) => { | |
| console.error('Error in scheduled head re-execution:', err) | |
| }) | |
| } |
🤖 Prompt for AI Agents
In packages/router-core/src/load-matches.ts around lines 960 to 970, the
detached Promise.allSettled(...).then(...) chain can produce an unhandled
rejection if executeAllHeadFns throws or returns a rejected promise; add a
.catch handler to that promise chain to handle any errors from executeAllHeadFns
(or from the then callback) and prevent unhandled rejections — call the router's
error handler if available (e.g. inner.router.options?.onError or
inner.router.options?.onExecuteError) with the error, otherwise log to
console.error, and ensure the catch swallows or handles the error so the
detached promise is not left unhandled.
Root cause: When loader throws redirect, the catch block calls inner.router.navigate() which removes the old match from the router. Then finally block tries getMatch(matchId) but returns undefined, so promises never resolve, blocking Promise.allSettled. Solution: Capture match reference BEFORE entering try block, so we have a stable reference even if redirect removes it from router. Flow with redirect: 1. Get matchForCleanup (captures reference) 2. runLoader throws redirect 3. Catch: navigate() removes match from router 4. Finally: Use matchForCleanup (still valid) to resolve promises This allows Promise.allSettled to complete and navigation to proceed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Sure, but solid-start work as well, right? I don't know react, sorry. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/router-core/src/load-matches.ts (2)
959-970: Critical: Add error handler to prevent unhandled promise rejection.The detached promise chain lacks a
.catch()handler. IfexecuteAllHeadFnsthrows or returns a rejected promise during the scheduled re-execution, this will produce an unhandled rejection. Past review comments indicated this was addressed in commit 90d4ba3, but the error handler is still missing in the current code.🔎 Add error handling to prevent unhandled rejections
const thisNavigationLocation = inner.location Promise.allSettled(asyncLoaderPromises).then(() => { // Only execute if this navigation is still current (not superseded by new navigation) const latestLocation = inner.router.state.location if (latestLocation === thisNavigationLocation) { executeAllHeadFns(inner) } - }) + }).catch((err) => { + const logger = inner.router.options?.logger ?? console + logger.error('Error in scheduled head re-execution:', err) + }) }
962-969: Major: Location comparison insufficient to prevent stale head updates.When
router.invalidate()is called on the same route, it triggers new loaders while keeping the location unchanged. The scheduled head re-execution capturesthisNavigationLocationand later compares it against the current location. Since the location doesn't change during invalidation, this guard fails to detect that a new load cycle has begun, allowing stale scheduled executions to overwrite fresh data.Additionally, there's no stored reference to the scheduled promise, so if a new
loadMatchescycle starts before the previous promise completes, there's no way to cancel the stale execution.Consider one of these approaches:
- Store a reference to the scheduled execution promise and replace/cancel it if new loaders are detected
- Use a load-cycle ID or generation counter that increments on each
loadMatchescall (not just navigation)- Check if the captured loader promises are still the current ones before re-executing heads
Verify if the router has a generation ID or load-cycle tracking mechanism that could be used instead of location comparison:
#!/bin/bash # Search for generation/cycle/load tracking mechanisms in router state rg -n --type=ts -C3 'generation|loadId|cycleId|navigationId' packages/router-core/src/router.ts # Check if matches have version/generation tracking rg -n --type=ts -C3 'generation|version|loadId' packages/router-core/src/Matches.ts
🧹 Nitpick comments (1)
packages/router-core/src/load-matches.ts (1)
586-605: Consider using router's logger instead of console.error.The
executeAllHeadFnsfunction logs errors usingconsole.errordirectly. If the router has a configured logger (e.g.,inner.router.options?.logger), prefer using it for consistency with other error logging throughout the codebase.🔎 Proposed enhancement
} catch (err) { - console.error(`Error executing head for route ${routeId}:`, err) + const logger = inner.router.options?.logger ?? console + logger.error(`Error executing head for route ${routeId}:`, err) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/load-matches.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/load-matches.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/load-matches.ts
🧠 Learnings (3)
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/router-core/src/load-matches.ts
📚 Learning: 2025-12-21T12:52:35.231Z
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:35.231Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Applied to files:
packages/router-core/src/load-matches.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/router-core/src/load-matches.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (2)
packages/router-core/src/load-matches.ts (2)
839-854: Good defensive fix for redirect-induced match removal.Capturing the match reference before the try block ensures that the finally block can still resolve promises even if a redirect navigation removes the match from the router. This prevents
Promise.allSettledfrom hanging and is a solid improvement to the loader execution flow.
947-948: LGTM: Initial head execution correctly placed.Executing all head functions after loaders have completed ensures that the initial head data is available with loader results. This addresses the core issue where head() was executing before loaderData was ready.
|
Still getting PR test failures: Wondering if they were caused by introducing the re-run of head functions ( |
|
yes this is caused by rerunning head. needs to adapt the counts for the scenarios where a detached loader runs |
2 participants
Fixes #6221
The bug: When loaders run asynchronously (stale-while-revalidate), loadRouteMatch returns immediately while the loader runs in the background. Promise.allSettled(inner.matchPromises) waits for loadRouteMatch promises, not the actual loader completion, causing head() to execute with undefined loaderData.
The fix: After loadRouteMatch promises settle, explicitly wait for all loaderPromises to complete before executing head functions. This ensures loaderData is available when head() executes.
Reproduction scenario:
Fixes the issue identified in PR #6093 follow-up investigation.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.