refactor(router-core): flatten loadMatches #4961
Conversation
|
View your CI Pipeline Execution ↗ for commit 4e8d69e
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-plugin
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-plugin
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
WalkthroughRefactors router loading into a context-driven pipeline using InnerLoadContext, replaces the old loadMatches signature with a baseContext parameter, adds many private helpers for beforeLoad/loader/head/pending/redirect/notFound flows, and updates a Solid test to await router.latestLoadPromise instead of calling router.load(). Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant RouterCore
participant InnerLoadContext as Context
participant Matches
participant BeforeLoad
participant Loader
participant Head
participant ErrorHandling as Redirect/NotFound
App->>RouterCore: loadMatches(baseContext)
RouterCore->>RouterCore: create InnerLoadContext
RouterCore->>Context: resolvePreload / triggerOnReady
RouterCore->>RouterCore: handleRedirectAndNotFound (early checks)
RouterCore->>BeforeLoad: executeBeforeLoad (serial for matches)
alt redirect/notFound
BeforeLoad->>ErrorHandling: signal redirect/notFound
ErrorHandling-->>RouterCore: handled/converted
else proceed
RouterCore->>RouterCore: setupPendingTimeout / min-pending
RouterCore->>Matches: loadRouteMatch (parallel up to first bad)
Matches->>Loader: runLoader (loader + beforeLoad promises)
Loader-->>Matches: data / error
Matches->>Head: executeHead
Head-->>RouterCore: metadata
RouterCore->>Context: updateMatch per match
RouterCore-->>App: return resolved matches
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)packages/router-core/src/router.ts (6)
⏰ 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)
🔇 Additional comments (19)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (4)
packages/router-core/src/router.ts (4)
2098-2103: Consider extracting therenderedflag logic.The
triggerOnReadymethod has a side effect of settingrendered = true. Consider documenting why this flag is needed and its relationship to the loading lifecycle, as it's not immediately clear from the method name.private triggerOnReady = async (innerLoadContext: InnerLoadContext) => { + // Mark as rendered to prevent duplicate onReady calls during the same load cycle if (!innerLoadContext.rendered) { innerLoadContext.rendered = true await innerLoadContext.onReady?.() } }
2176-2225: Consider consistent error property assignment.In
handleSerialError, therouterCodeis assigned directly to the error object. Consider using a more type-safe approach or documenting this property addition.- err.routerCode = routerCode + // Attach router-specific error code for debugging + Object.defineProperty(err, 'routerCode', { + value: routerCode, + writable: false, + enumerable: true, + configurable: false + })
2276-2281: Consider extracting the makeMaybe helper to utils.The
makeMaybefunction at the bottom of the file seems like a general utility that could be moved to the utils module for better organization and potential reuse.Move the
makeMaybefunction to the utils module:// In utils.ts export function makeMaybe<T>(value: T, error: any): | { status: 'success'; value: T } | { status: 'error'; error: any } { if (error) { return { status: 'error', error } } return { status: 'success', value } }
2227-2296: SSR handling logic is well-structured but could benefit from documentation.The
isBeforeLoadSsrmethod handles complex SSR inheritance logic. Consider adding JSDoc comments to explain the inheritance rules, especially the parent override logic and the 'data-only' mode behavior.+/** + * Determines SSR mode for a route match, inheriting from parent matches + * and respecting configuration hierarchy. + * + * SSR modes: + * - true: Full SSR + * - 'data-only': SSR data but not components + * - false: No SSR + * + * Inheritance rules: + * - Shell mode: only SSR root route + * - Parent false: children inherit false + * - Parent 'data-only': children with true become 'data-only' + */ private isBeforeLoadSsr = async ( innerLoadContext: InnerLoadContext, matchId: string, index: number, route: AnyRoute, ) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/router-core/src/router.ts(6 hunks)packages/solid-router/tests/Transitioner.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router-core/src/router.ts (6)
packages/router-core/src/index.ts (10)
ParsedLocation(70-70)AnyRouteMatch(88-88)isRedirect(372-372)AnyRoute(161-161)rootRouteId(109-109)createControlledPromise(278-278)BeforeLoadContextOptions(179-179)LoaderFnContext(174-174)MakeRouteMatch(87-87)NotFoundError(377-377)packages/router-core/src/location.ts (1)
ParsedLocation(4-13)packages/router-core/src/Matches.ts (2)
AnyRouteMatch(239-239)MakeRouteMatch(217-237)packages/router-core/src/redirect.ts (1)
isRedirect(95-97)packages/router-core/src/root.ts (1)
rootRouteId(1-1)packages/router-core/src/utils.ts (1)
createControlledPromise(398-422)
🔇 Additional comments (6)
packages/solid-router/tests/Transitioner.test.tsx (2)
33-34: Test now aligns with new router loading pipeline.The switch from explicit
router.load()to usingrouter.latestLoadPromisecorrectly leverages the new context-driven loading flow coordinated by the RouterProvider. This change is consistent with the router-core refactoring that moved loading orchestration into a pipeline-based approach.
38-38: AllloadSpyexpectations correctly updated to a single call
Verified that in both the client and serverTransitioner.test.tsxfiles,loadSpyis now asserted withtoHaveBeenCalledTimes(1), and there are no remaining tests expecting two calls. This aligns with the refactored behavior where theRouterProviderhandles the initial load automatically on the client and does not trigger an extra load on the server. No further changes needed.packages/router-core/src/router.ts (4)
766-776: Well-structured context object for the loading pipeline.The
InnerLoadContexttype effectively encapsulates all the state needed for the refactored loading pipeline. The inclusion offirstBadMatchIndex,matchPromises, and other fields provides clean separation of concerns and makes the loading flow more maintainable.
2119-2159: Comprehensive error handling with proper cleanup.The
handleRedirectAndNotFoundmethod properly handles both redirect and not-found cases, ensuring promises are resolved and match states are updated correctly. The distinction between handling during preload vs regular load is well-considered.
2500-2542: Clean separation of concerns in beforeLoad handling.The
handleBeforeLoadmethod effectively orchestrates the SSR determination, skip checks, and execution flow. The chain of responsibility pattern withserverSsr→queueExecution→executeis well-structured.
2882-2940: Excellent refactoring of loadMatches with serial/parallel execution.The refactored
loadMatchesmethod clearly separates serial execution of beforeLoad handlers from parallel execution of loaders. The use ofinnerLoadContextto maintain state throughout the pipeline is clean and maintainable. The error handling for NotFound and Redirect exceptions is comprehensive.
After the "flattening" PR #4961, we now have a very clear *separate responsibility* the `Router` class is handling: loading matches. This PR proposes we split the code that handles "loading matches" into its own `load-matches.ts` file, with the entry points being - `loadMatches` - `loadRouteChunk` In future PRs, we might consider splitting other responsibilities out of the 3k LoC `router.ts` file --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This PR *should* be a no-op. Just restructuring the code to have all the functions used by `loadMatches` at the same level, instead of nested callbacks. In order to still have access to the data that was previously in scope because of nesting, we use a `innerLoadContext` object, which is basically just the arguments of `loadMatches`, and a few mutables. However, in order to do so, we have to rearrange promises in an area that is sensitive to any microtask order change, and we did change some of them. So this is not *really* a no-op, even though this does not attempt any optimization. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Smoother navigations with predictable pending states and minimum display durations. - More consistent head/meta updates after route loading. - Bug Fixes - More reliable redirects and 404 handling, including during preloading and SSR. - Reduced UI flicker by refining pending and preloading behavior. - Refactor - Reworked the routing load pipeline for improved performance, stability, and SSR behavior. - Streamlined before-load steps for clearer, more consistent execution. - Tests - Updated tests to reflect the new loading flow. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
After the "flattening" PR #4961, we now have a very clear *separate responsibility* the `Router` class is handling: loading matches. This PR proposes we split the code that handles "loading matches" into its own `load-matches.ts` file, with the entry points being - `loadMatches` - `loadRouteChunk` In future PRs, we might consider splitting other responsibilities out of the 3k LoC `router.ts` file --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This PR should be a no-op. Just restructuring the code to have all the functions used by
loadMatchesat the same level, instead of nested callbacks. In order to still have access to the data that was previously in scope because of nesting, we use ainnerLoadContextobject, which is basically just the arguments ofloadMatches, and a few mutables.However, in order to do so, we have to rearrange promises in an area that is sensitive to any microtask order change, and we did change some of them. So this is not really a no-op, even though this does not attempt any optimization.
Summary by CodeRabbit