fix: wait for async loaders before executing head functions #6222
Conversation
📝 WalkthroughWalkthroughIntroduce a load-generation counter and guarded abort gate to detect stale non-preload loads; run all route head() functions serially after initial loaders, capture stable match references around async loaders, and schedule a guarded non-blocking re‑execution of head() after any in‑flight loaders settle. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Router
participant Loaders as "Loader(s)"
participant Head as "head() functions"
participant Scheduler as "Re-run Scheduler"
Client->>Router: navigate to /route
Router->>Loaders: invoke loaders (may run parallel/async)
note right of Loaders `#f0f7ff`: some loaders may remain pending
Loaders-->>Router: some loader results resolve/reject
Router->>Head: call executeAllHeadFns(inner) serially (uses current generation)
Head-->>Router: apply head data (per-call try/catch, update matches)
alt pending loaders detected
Router->>Scheduler: schedule non-blocking re-run (Promise.allSettled on pending)
Scheduler->>Loaders: await pending loaders settle
Loaders-->>Scheduler: pending settled
alt generation unchanged (shouldAbortLoad false)
Scheduler->>Head: re-invoke executeAllHeadFns(inner)
Head-->>Router: refreshed head data applied
else generation changed (stale)
Scheduler-->>Router: abort re-run
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, 1 inconclusive)
✅ Passed checks (3 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 | 13m 17s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 42s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-12-29 10:30:45 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?
![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 |
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 |
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.
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.
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? |
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.
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.
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/solid-start/basic/src/routes/test-head/article.$id.tsx (1)
5-13: Consider reducing the artificial delay for faster test execution.The 1000ms delay effectively demonstrates async behavior but could be reduced to 100-200ms for faster test runs without losing test fidelity.
🔎 Proposed optimization
const fetchArticle = async (id: string) => { - await new Promise((resolve) => setTimeout(resolve, 1000)) + await new Promise((resolve) => setTimeout(resolve, 100)) return isAuthed() ? { title: `Article Title for ${id}`, content: `Article content for ${id}\n`.repeat(10), } : null }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e/solid-start/basic/src/routeTree.gen.tse2e/solid-start/basic/src/routes/test-head/article.$id.tsxe2e/solid-start/basic/src/utils/fake-auth.tspackages/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:
e2e/solid-start/basic/src/utils/fake-auth.tse2e/solid-start/basic/src/routes/test-head/article.$id.tsxpackages/router-core/src/load-matches.tse2e/solid-start/basic/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/src/utils/fake-auth.tse2e/solid-start/basic/src/routes/test-head/article.$id.tsxpackages/router-core/src/load-matches.tse2e/solid-start/basic/src/routeTree.gen.ts
🧠 Learnings (7)
📚 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/src/utils/fake-auth.tse2e/solid-start/basic/src/routes/test-head/article.$id.tsx
📚 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/src/routes/test-head/article.$id.tsxpackages/router-core/src/load-matches.tse2e/solid-start/basic/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/src/routes/test-head/article.$id.tsxe2e/solid-start/basic/src/routeTree.gen.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
📚 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/src/routeTree.gen.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/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/src/routeTree.gen.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 (6)
e2e/solid-start/basic/src/utils/fake-auth.ts (1)
1-16: LGTM! Test utilities are appropriately simple.The client-only auth helpers are correctly implemented using
createClientOnlyFnto prevent SSR execution. The localStorage-based approach is suitable for e2e testing purposes.e2e/solid-start/basic/src/routes/test-head/article.$id.tsx (3)
15-25: LGTM! Route properly demonstrates the head/loader timing issue.The route configuration correctly:
- Disables SSR due to client-only auth checks
- Uses optional chaining in
head()to safely handle undefinedloaderData- Provides a fallback title that makes the bug observable in tests
27-48: LGTM! Component correctly implements the test scenario.The component properly uses Solid's reactive patterns and correctly triggers router invalidation after auth state changes, which is essential for testing the head re-execution behavior.
50-66: LGTM! NotAccessible component completes the test flow.The component correctly handles the unauthenticated state and triggers re-validation after login, allowing the test to verify head() execution with fresh loader data.
packages/router-core/src/load-matches.ts (2)
839-854: Good practice: capturing match reference before navigation.The changes correctly capture the match reference before the try block to ensure promise resolution even if navigation removes the match from the router state. This is essential for the
Promise.allSettledapproach used later inloadMatches.The finally block now uses the captured reference to reliably resolve promises, preventing hanging promises that could block the scheduled head re-execution.
947-972: This review misidentifies the commit's purpose.The commit a15a239 is titled "add Minimal e2e example to demo the head function's set title bug" and its message explicitly describes reproduction steps for the bug. The disabled Promise.allSettled code is intentional—the TODO comment states it is "temporarily disabled to make sure solid-start/basic example can reproduce the bug."
This commit adds a working reproduction example, not an attempted fix. The disabled code is necessary to keep the bug reproducible for demonstration and testing purposes. Uncommenting it would fix the bug but would defeat the purpose of having a reproduction example to track the issue.
If a separate PR is intended to fix this issue (rather than demonstrate it), that should be a different commit with a different purpose and messaging.
Likely an incorrect or invalid review comment.
| const executeAllHeadFns = async (inner: InnerLoadContext) => { | ||
| // Serially execute head functions for all matches | ||
| // Each execution is wrapped in try-catch to ensure all heads run even if one fails | ||
| for (const match of inner.matches) { | ||
| const { id: matchId, routeId } = match | ||
| const route = inner.router.looseRoutesById[routeId]! | ||
| try { | ||
| const headResult = executeHead(inner, matchId, route) | ||
| if (headResult) { | ||
| const head = await headResult | ||
| inner.updateMatch(matchId, (prev) => ({ | ||
| ...prev, | ||
| ...head, | ||
| })) | ||
| } | ||
| } catch (err) { | ||
| console.error(`Error executing head for route ${routeId}:`, err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l packages/router-core/src/load-matches.tsRepository: TanStack/router
Length of output: 104
🏁 Script executed:
# Get context around the executeAllHeadFns function and surrounding code
sed -n '580,620p' packages/router-core/src/load-matches.tsRepository: TanStack/router
Length of output: 1179
🏁 Script executed:
# Search for error handling patterns in the file
rg -A 3 -B 1 'onError|console\.(error|warn|log)|logger' packages/router-core/src/load-matches.ts | head -50Repository: TanStack/router
Length of output: 627
🏁 Script executed:
# Look for how head functions are executed elsewhere
rg -A 5 -B 2 'executeHead' packages/router-core/src/load-matches.tsRepository: TanStack/router
Length of output: 453
🏁 Script executed:
# Check the router options structure
rg -A 10 'options\.' packages/router-core/src/load-matches.ts | head -60Repository: TanStack/router
Length of output: 1929
🏁 Script executed:
# Look for navigation staleness checks or location comparisons
rg -B 3 -A 3 'location|stale|navigation.*id' packages/router-core/src/load-matches.ts | head -80Repository: TanStack/router
Length of output: 2600
🏁 Script executed:
# Check how inner.updateMatch is used and if there are guards
rg -B 2 -A 2 'updateMatch' packages/router-core/src/load-matches.ts | head -60Repository: TanStack/router
Length of output: 1697
🏁 Script executed:
# Look at the InnerLoadContext type to understand its structure
rg -B 5 -A 15 'type InnerLoadContext|interface InnerLoadContext' packages/router-core/src/load-matches.tsRepository: TanStack/router
Length of output: 788
🏁 Script executed:
# Check if there are any abortController or navigation context checks
rg -B 2 -A 2 'abortController|aborted' packages/router-core/src/load-matches.ts | head -40Repository: TanStack/router
Length of output: 930
🏁 Script executed:
# Search for where executeAllHeadFns is called to understand the context
rg -B 5 -A 5 'executeAllHeadFns' packages/router-core/src/load-matches.tsRepository: TanStack/router
Length of output: 1301
🏁 Script executed:
# Check how other head execution patterns handle staleness/cancellation
rg -B 10 -A 10 'head.*execute|executeHead.*await' packages/router-core/src/load-matches.ts | head -100Repository: TanStack/router
Length of output: 742
🏁 Script executed:
# Look for navigation/location change detection patterns
rg -B 5 -A 5 'location.*===|location.*===.*location|navigate.*change' packages/router-core/src/load-matches.ts | head -80Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check the route.options.onError pattern more thoroughly
rg -B 10 -A 10 'route\.options\.onError' packages/router-core/src/load-matches.tsRepository: TanStack/router
Length of output: 1263
Use route.options.onError for consistent error handling.
The executeAllHeadFns function uses console.error for error logging (line 604), while other error handlers in this file invoke route.options.onError?.(err) for consistency (see lines 738-741 and 825-826). Update the error handler to follow the same pattern:
try {
route.options.onError?.(err)
} catch (errorHandlerErr) {
console.error(`Error executing head for route ${routeId}:`, errorHandlerErr)
}The commented-out scheduled re-execution code (lines 963-971) already attempts to address staleness concerns with location comparison. If that code is re-enabled, ensure the staleness check using location reference comparison is implemented.
🤖 Prompt for AI Agents
In packages/router-core/src/load-matches.ts around lines 586 to 605, the catch
block currently uses console.error directly; replace it with calling
route.options.onError?.(err) inside a try/catch so custom route error handlers
are used consistently, and if the onError handler itself throws, catch that and
call console.error with a message like "Error executing head for route
{routeId}:" and the handler error; ensure you reference the route and routeId
variables already in scope and do not change surrounding control flow.
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]>
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]>
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]>
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]>
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]>
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]>
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]>
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]>
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]>
Open http://localhost:3000/test-head/article/123 1. Initially not logged in. You see page title "title n/a", content "Article Not Accessible." 2. Click "Log in" (see utils/fake-auth), which invalidates the route's loader data, trigger refetch; 1 second later, you should see the article content becoming available, But, the title stays n/a. 3. Now manually reload the page, confirm that both page title and article content are available. 4. Click "Log out", which again invalidates the loader data. 1 second later, you should see the article content becoming Unavailable, But, the title is still available! 5. Reload the page, confirm that both page title and article content are Unavailable.
yanghuidong
force-pushed
the
fix/head-premature-execution
branch
from
a15a239 to
b517fb8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/router-core/src/load-matches.ts (1)
601-603: Useroute.options.onErrorfor consistent error handling.The error handler here uses
console.errordirectly, while other error handlers in this file invokeroute.options.onError?.(err)(see lines 733-741). This inconsistency means custom error handlers configured on routes won't be called for head execution failures.🔎 Proposed fix for consistent error handling
} catch (err) { - console.error(`Error executing head for route ${routeId}:`, err) + try { + route.options.onError?.(err) + } catch (errorHandlerErr) { + console.error(`Error executing head for route ${routeId}:`, errorHandlerErr) + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
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.tse2e/solid-start/basic/src/routeTree.gen.tse2e/solid-start/basic/src/routes/test-head/article.$id.tsxe2e/solid-start/basic/src/utils/fake-auth.tspackages/router-core/src/load-matches.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- e2e/solid-start/basic-solid-query/tests/head.spec.ts
- e2e/solid-start/basic-solid-query/src/utils/fake-auth.ts
- e2e/solid-start/basic-solid-query/src/routes/test-head/article.$id.tsx
- e2e/solid-start/basic-solid-query/src/routes/fake-login.tsx
- e2e/solid-start/basic-solid-query/package.json
- e2e/solid-start/basic-solid-query/src/routeTree.gen.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:
e2e/solid-start/basic-solid-query/src/routes/test-head/dashboard.tsxe2e/solid-start/basic/src/routes/test-head/article.$id.tsxpackages/router-core/src/load-matches.tse2e/solid-start/basic/src/utils/fake-auth.tse2e/solid-start/basic/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/routes/test-head/dashboard.tsxe2e/solid-start/basic/src/routes/test-head/article.$id.tsxpackages/router-core/src/load-matches.tse2e/solid-start/basic/src/utils/fake-auth.tse2e/solid-start/basic/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/src/routes/test-head/article.$id.tsxpackages/router-core/src/load-matches.tse2e/solid-start/basic/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.tsx
📚 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-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.tse2e/solid-start/basic/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/src/utils/fake-auth.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/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/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/src/routeTree.gen.ts
🧬 Code graph analysis (2)
e2e/solid-start/basic-solid-query/src/routes/test-head/dashboard.tsx (2)
e2e/solid-start/basic-solid-query/src/routes/fake-login.tsx (1)
Route(5-11)e2e/solid-start/basic-solid-query/src/routes/test-head/article.$id.tsx (1)
Route(23-39)
e2e/solid-start/basic/src/routes/test-head/article.$id.tsx (1)
e2e/solid-start/basic/src/utils/fake-auth.ts (3)
isAuthed(3-3)fakeLogout(3-3)fakeLogin(3-3)
⏰ 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 (10)
e2e/solid-start/basic-solid-query/src/routes/test-head/dashboard.tsx (2)
1-8: LGTM! Clean route definition for e2e testing.The route structure is correct, and the static
head()function properly sets the document title for this test route.
10-45: LGTM! Well-structured test page with clear instructions.The component provides helpful context for testing the head() re-execution behavior with appropriate test identifiers.
e2e/solid-start/basic/src/utils/fake-auth.ts (2)
1-8: LGTM! Client-only auth utilities properly implemented.The
createClientOnlyFnwrapper ensures safe localStorage access, and the simple auth check is appropriate for e2e testing.
10-16: LGTM! Simple and effective fake auth implementation.The login/logout functions correctly manipulate localStorage for test scenarios, with proper client-only guards.
e2e/solid-start/basic/src/routes/test-head/article.$id.tsx (4)
1-13: LGTM! Appropriate test fixture with simulated async loading.The 1-second delay effectively simulates the async loader behavior that this PR aims to fix, and the conditional return based on auth state supports the test scenario.
15-25: LGTM! Route definition correctly tests the PR fix.The
head()function properly handles potentially undefinedloaderDatawith optional chaining, which is essential for testing the async loader timing fix. The fallback title ensures the test can verify when loaderData is and isn't available.
27-48: LGTM! Component correctly implements auth-gated content.The use of
router.invalidate()after logout properly triggers route re-validation, which is essential for testing the head() re-execution behavior.
50-66: LGTM! Unauthenticated fallback component works as expected.The login flow with
router.invalidate()mirrors the logout pattern and supports the test scenario for head() re-execution after authentication state changes.e2e/solid-start/basic/src/routeTree.gen.ts (1)
1-1001: Skipping review of autogenerated file.This file is automatically generated by TanStack Router and should not be manually modified or reviewed. The changes reflect the new test routes added in this PR.
Based on learnings, do not review
routeTree.gen.tsfiles.packages/router-core/src/load-matches.ts (1)
839-854: Good defensive pattern for async cleanup.Capturing the match reference before the try block ensures the finally cleanup can resolve promises even when redirect navigation removes the match from the router state. This prevents hanging promises and allows
Promise.allSettledto complete reliably.
… is gone, in `e2e/solid-start/basic` example
|
looking at the e2e test it might make sense to create a new separate e2e project for this specific topic |
Problem: When async loaders complete, we schedule a detached head() re-run to execute head functions with fresh loaderData. However, if a new navigation or invalidation starts before this re-run executes, it would update state with stale data. The previous location-based check only caught navigation to different locations, but missed same-location invalidations (e.g., manual reload, scheduled refetch). Solution: Implement a generation counter pattern to track load operations: 1. Add `router._loadGeneration` counter (excludes preloads) 2. Each non-preload `loadMatches()` call increments the counter 3. Store the generation in `inner.loadGeneration` when load starts 4. Before executing head re-runs or updates, check if generation matches This detects ALL cases where a load has been superseded: - Navigation to different location (new loadMatches call) - Invalidation on same location (new loadMatches call) - Any other scenario triggering a new load The generation counter is a standard pattern in reactive systems (React, RxJS) for detecting stale computations. Benefits: - No circular references (vs storing full context) - Minimal memory (4 bytes) - Simple numeric comparison - Clear semantics (higher = newer) 🤖 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)
612-641: Well-implemented abort guards with thorough staleness checks.The function correctly checks
shouldAbortLoadat critical points: before starting, before each match iteration, and after async operations complete.Consider using
route.options.onErrorfor consistency.The error handler uses
console.error(line 638), while other error handlers in this file invokeroute.options.onError?.(err)(see lines 229, 769). For consistency with the rest of the codebase, consider:🔎 Optional: Consistent error handling pattern
} catch (err) { - console.error(`Error executing head for route ${routeId}:`, err) + try { + route.options.onError?.(err) + } catch (errorHandlerErr) { + console.error(`Error executing head for route ${routeId}:`, errorHandlerErr) + } }
989-1013: Core fix addresses the PR objective correctly.The implementation properly:
- Executes head functions after initial loaders complete (line 990)
- Detects in-flight async loaders and schedules non-blocking re-execution (lines 1001-1012)
- Uses generation-based staleness check to prevent stale updates (line 1009)
This ensures
head()functions have access to freshloaderDataeven during stale-while-revalidate scenarios.Optional: Defensive
.catch()handler.While
executeAllHeadFnshas internal try-catch (mitigating unhandled rejection risk), a defensive.catch()provides an extra safety net:🔎 Optional defensive error handling
Promise.allSettled(asyncLoaderPromises).then(() => { if (!shouldAbortLoad(inner)) { executeAllHeadFns(inner) } - }) + }).catch((err) => { + // Defensive catch for any unexpected errors + if (process.env.NODE_ENV !== 'production') { + console.error('Error in scheduled head re-execution:', err) + } + })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/load-matches.tspackages/router-core/src/router.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/router.tspackages/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/router.tspackages/router-core/src/load-matches.ts
🧠 Learnings (4)
📚 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:
packages/router-core/src/router.tspackages/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/router.tspackages/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 (5)
packages/router-core/src/router.ts (1)
897-911: LGTM! Clean implementation of load-generation tracking.The generation counter with comprehensive JSDoc documentation clearly explains its purpose for detecting stale async operations. The underscore prefix correctly signals internal usage.
packages/router-core/src/load-matches.ts (4)
38-44: LGTM!The optional
loadGenerationfield with clear documentation explaining its relationship torouter._loadGenerationfor staleness detection.
46-64: LGTM! Generation-based staleness check correctly addresses invalidation edge cases.Unlike the previous location-based comparison (which couldn't detect same-location invalidations), this generation counter approach correctly detects when a load has been superseded by any new load operation—whether from navigation or invalidation.
874-890: Good defensive fix for async loader cleanup.Capturing the match reference before the try block ensures promise cleanup works correctly even when navigation removes the match during async operations. This prevents potential memory leaks and allows
Promise.allSettledto complete reliably.
940-944: LGTM!Correctly assigns generation numbers only for non-preload operations, allowing concurrent preloads while enabling staleness detection for navigations and invalidations.
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.