refactor: switch router stores to atom get/set API#7150
Merged
Conversation
Align the router store contract with createAtom so the React, Vue, and Solid adapters share one get/set shape without preserving the old state/setState compatibility layer.
github-actions
bot
added
package: react-router
package: router-core
package: router-plugin
package: solid-router
package: react-start
package: router-devtools-core
package: solid-start
package: start-server-core
package: start-client-core
package: router-ssr-query-core
package: vue-router
package: vue-start
labels
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR replaces direct store Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
View your CI Pipeline Execution ↗ for commit aed5dd4
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview12 package(s) bumped directly, 20 bumped as dependents. 🟩 Patch bumps
|
Contributor
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-plugin/src/core/route-hmr-statement.ts (1)
19-37:⚠️ Potential issue | 🟠 MajorType definitions still reference
setStatebut implementation usesset.The
AnyRouterWithPrivateMapstype declaressetStateon the match store maps (lines 23, 29, 35), but the implementation at line 125 callsstore.set(...). This mismatch will cause TypeScript errors. Update the type to usesetto match theRouterWritableStoreinterface frompackages/router-core/src/stores.ts.🔧 Proposed fix
type AnyRouterWithPrivateMaps = AnyRouter & { routesById: Record<string, AnyRoute> routesByPath: Record<string, AnyRoute> stores: AnyRouter['stores'] & { cachedMatchStoresById: Map< string, { - setState: (updater: (prev: AnyRouteMatch) => AnyRouteMatch) => void + set: (updater: (prev: AnyRouteMatch) => AnyRouteMatch) => void } > pendingMatchStoresById: Map< string, { - setState: (updater: (prev: AnyRouteMatch) => AnyRouteMatch) => void + set: (updater: (prev: AnyRouteMatch) => AnyRouteMatch) => void } > activeMatchStoresById: Map< string, { - setState: (updater: (prev: AnyRouteMatch) => AnyRouteMatch) => void + set: (updater: (prev: AnyRouteMatch) => AnyRouteMatch) => void } > } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-plugin/src/core/route-hmr-statement.ts` around lines 19 - 37, The type AnyRouterWithPrivateMaps declares match store maps with a setState method but the implementation calls store.set(...); update the three map value types (cachedMatchStoresById, pendingMatchStoresById, activeMatchStoresById) to use set instead of setState so they match the RouterWritableStore interface from packages/router-core/src/stores.ts; specifically replace the setState signature ((updater: (prev: AnyRouteMatch) => AnyRouteMatch) => void) with a set signature ((value: AnyRouteMatch | ((prev: AnyRouteMatch) => AnyRouteMatch)) => void) to align types with the actual store API used by the code.
🧹 Nitpick comments (2)
packages/react-router/src/Scripts.tsx (1)
61-64: Read active matches once in the SSR branch.Calling
.get()twice is unnecessary and can be avoided by reusing a single snapshot value.♻️ Proposed refactor
if (isServer ?? router.isServer) { - const assetScripts = getAssetScripts( - router.stores.activeMatchesSnapshot.get(), - ) - const scripts = getScripts(router.stores.activeMatchesSnapshot.get()) + const activeMatches = router.stores.activeMatchesSnapshot.get() + const assetScripts = getAssetScripts(activeMatches) + const scripts = getScripts(activeMatches) return renderScripts(router, scripts, assetScripts) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router/src/Scripts.tsx` around lines 61 - 64, The code calls router.stores.activeMatchesSnapshot.get() twice to compute assetScripts and scripts; capture the snapshot once into a local variable (e.g. const activeMatches = router.stores.activeMatchesSnapshot.get()) and pass that single value into getAssetScripts(...) and getScripts(...) to avoid redundant snapshot reads; change references in this block from direct .get() calls to the new activeMatches variable (affecting getAssetScripts and getScripts usages).packages/router-core/src/ssr/ssr-client.ts (1)
295-302: Redundant nestedbatch()call.The inner
router.batch()on line 296 is already inside the outerrouter.batch()from line 291. While this is not functionally incorrect (batch calls are typically idempotent), it adds unnecessary nesting.♻️ Suggested simplification
loadPromise.then(() => { router.batch(() => { // ensure router is not in status 'pending' anymore // this usually happens in Transitioner but if loading synchronously resolves, // Transitioner won't be rendered while loading so it cannot track the change from loading:true to loading:false if (router.stores.status.get() === 'pending') { - router.batch(() => { - router.stores.status.set(() => 'idle') - router.stores.resolvedLocation.set(() => - router.stores.location.get(), - ) - }) + router.stores.status.set(() => 'idle') + router.stores.resolvedLocation.set(() => + router.stores.location.get(), + ) } // hide the pending component once the load is finished router.updateMatch(match.id, (prev) => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/ssr/ssr-client.ts` around lines 295 - 302, The nested router.batch() call is redundant; remove the inner router.batch wrapper and directly call router.stores.status.set and router.stores.resolvedLocation.set inside the existing outer router.batch so you only have a single router.batch scope updating router.stores.status and router.stores.resolvedLocation (references: router.batch, router.stores.status.set, router.stores.resolvedLocation.set, router.stores.location.get).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/router-plugin/src/core/route-hmr-statement.ts`:
- Around line 19-37: The type AnyRouterWithPrivateMaps declares match store maps
with a setState method but the implementation calls store.set(...); update the
three map value types (cachedMatchStoresById, pendingMatchStoresById,
activeMatchStoresById) to use set instead of setState so they match the
RouterWritableStore interface from packages/router-core/src/stores.ts;
specifically replace the setState signature ((updater: (prev: AnyRouteMatch) =>
AnyRouteMatch) => void) with a set signature ((value: AnyRouteMatch | ((prev:
AnyRouteMatch) => AnyRouteMatch)) => void) to align types with the actual store
API used by the code.
---
Nitpick comments:
In `@packages/react-router/src/Scripts.tsx`:
- Around line 61-64: The code calls router.stores.activeMatchesSnapshot.get()
twice to compute assetScripts and scripts; capture the snapshot once into a
local variable (e.g. const activeMatches =
router.stores.activeMatchesSnapshot.get()) and pass that single value into
getAssetScripts(...) and getScripts(...) to avoid redundant snapshot reads;
change references in this block from direct .get() calls to the new
activeMatches variable (affecting getAssetScripts and getScripts usages).
In `@packages/router-core/src/ssr/ssr-client.ts`:
- Around line 295-302: The nested router.batch() call is redundant; remove the
inner router.batch wrapper and directly call router.stores.status.set and
router.stores.resolvedLocation.set inside the existing outer router.batch so you
only have a single router.batch scope updating router.stores.status and
router.stores.resolvedLocation (references: router.batch,
router.stores.status.set, router.stores.resolvedLocation.set,
router.stores.location.get).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c373cc76-99ae-4b9c-9bf7-b15b89f3028d
📒 Files selected for processing (67)
packages/react-router/src/Match.tsxpackages/react-router/src/Matches.tsxpackages/react-router/src/Scripts.tsxpackages/react-router/src/Transitioner.tsxpackages/react-router/src/headContentUtils.tsxpackages/react-router/src/link.tsxpackages/react-router/src/not-found.tsxpackages/react-router/src/routerStores.tspackages/react-router/src/ssr/RouterClient.tsxpackages/react-router/src/ssr/renderRouterToStream.tsxpackages/react-router/src/ssr/renderRouterToString.tsxpackages/react-router/src/useCanGoBack.tspackages/react-router/src/useLocation.tsxpackages/react-router/src/useMatch.tsxpackages/react-router/src/useRouterState.tsxpackages/react-start/src/useServerFn.tspackages/router-core/src/hash-scroll.tspackages/router-core/src/load-matches.tspackages/router-core/src/router.tspackages/router-core/src/scroll-restoration.tspackages/router-core/src/ssr/createRequestHandler.tspackages/router-core/src/ssr/ssr-client.tspackages/router-core/src/ssr/ssr-server.tspackages/router-core/src/stores.tspackages/router-core/tests/callbacks.test.tspackages/router-core/tests/granular-stores.test.tspackages/router-core/tests/load.test.tspackages/router-core/tests/routerTestUtils.tspackages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsxpackages/router-plugin/src/core/route-hmr-statement.tspackages/router-plugin/tests/add-hmr/snapshots/react/arrow-function@true.tsxpackages/router-plugin/tests/add-hmr/snapshots/react/arrow-function@webpack-hot.tsxpackages/router-plugin/tests/add-hmr/snapshots/react/createRootRoute-inline-component@true.tsxpackages/router-plugin/tests/add-hmr/snapshots/react/explicit-undefined-component@true.tsxpackages/router-plugin/tests/add-hmr/snapshots/react/function-declaration@true.tsxpackages/router-plugin/tests/add-hmr/snapshots/react/multi-component@true.tsxpackages/router-plugin/tests/add-hmr/snapshots/react/string-literal-keys@true.tsxpackages/router-plugin/tests/add-hmr/snapshots/solid/arrow-function@true.tsxpackages/router-ssr-query-core/src/index.tspackages/solid-router/src/Match.tsxpackages/solid-router/src/Matches.tsxpackages/solid-router/src/Scripts.tsxpackages/solid-router/src/Transitioner.tsxpackages/solid-router/src/headContentUtils.tsxpackages/solid-router/src/link.tsxpackages/solid-router/src/not-found.tsxpackages/solid-router/src/routerStores.tspackages/solid-router/src/ssr/RouterClient.tsxpackages/solid-router/src/ssr/renderRouterToStream.tsxpackages/solid-router/src/ssr/renderRouterToString.tsxpackages/solid-router/src/useCanGoBack.tspackages/solid-router/src/useLocation.tsxpackages/solid-router/src/useMatch.tsxpackages/solid-router/src/useRouterState.tsxpackages/solid-start/src/useServerFn.tspackages/start-client-core/src/client/hydrateStart.tspackages/start-server-core/src/createStartHandler.tspackages/vue-router/src/Match.tsxpackages/vue-router/src/Transitioner.tsxpackages/vue-router/src/link.tsxpackages/vue-router/src/routerStores.tspackages/vue-router/src/ssr/RouterClient.tsxpackages/vue-router/src/ssr/renderRouterToStream.tsxpackages/vue-router/src/ssr/renderRouterToString.tsxpackages/vue-router/src/useMatch.tsxpackages/vue-router/src/useRouterState.tsxpackages/vue-start/src/useServerFn.ts
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We update waitForRouteRemovalReload in the HMR e2e suite to read the active matches snapshot via .get() instead of .state, aligning the test with the atom get/set API introduced by this refactor. Without this change, .state resolves to undefined, the waitForFunction predicate never returns true, and both affected tests time out after 30 seconds.
Warning
❌ We could not verify this fix.
diff --git a/e2e/react-start/hmr/tests/app.spec.ts b/e2e/react-start/hmr/tests/app.spec.ts
index 8166f94610..f9e57e05c9 100644
--- a/e2e/react-start/hmr/tests/app.spec.ts
+++ b/e2e/react-start/hmr/tests/app.spec.ts
@@ -142,9 +142,9 @@ async function waitForRouteLoaderCrumb(
async function waitForRouteRemovalReload(page: Page) {
await page.waitForFunction(() => {
const router = (window as any).__TSR_ROUTER__
- const match = router?.stores?.activeMatchesSnapshot?.state?.find(
- (entry: any) => entry.routeId === '/child',
- )
+ const match = router?.stores?.activeMatchesSnapshot
+ ?.get()
+ ?.find((entry: any) => entry.routeId === '/child')
return !match?.invalid && match?.isFetching === false
})
Or Apply changes locally with:
npx nx-cloud apply-locally kzDs-0wvr
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
Match the atom setter contract by accepting direct values in RouterWritableStore and replace callback setters that do not depend on the previous value.
Update the HMR helper types and tests for the atom get/set API, reuse the SSR active match snapshot once, and remove the redundant nested batch during hydration cleanup.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-core/src/ssr/ssr-client.ts (1)
173-202:⚠️ Potential issue | 🟠 MajorPass
activeMatchesintocontextFnContext.matches.
activeMatchesis fetched from the live match stores afterrouter.options.hydrate?.(...), but Line 201 still passes the earliermatchesarray intoroute.options.context. SinceactiveMatchesSnapshot.get()rebuilds from the current store pool (packages/router-core/src/stores.ts:143-145, 292-300), those arrays can diverge if the hydrate hook updates match state. That leavescontext()reconstructing against stale matches while the rest of this block usesactiveMatches.Suggested fix
if (route.options.context) { const contextFnContext: RouteContextOptions<any, any, any, any, any> = { deps: match.loaderDeps, params: match.params, context: parentContext ?? {}, location, navigate: (opts: any) => router.navigate({ ...opts, _fromLocation: location, }), buildLocation: router.buildLocation, cause: match.cause, abortController: match.abortController, preload: false, - matches, + matches: activeMatches, routeId: route.id, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/ssr/ssr-client.ts` around lines 173 - 202, The context passed to route.options.context is using the stale matches array; update the construction of contextFnContext inside the loop (where matches is assigned to the matches field) to use activeMatches instead (i.e., set contextFnContext.matches = activeMatches) so context() runs against the current activeMatchesSnapshot.get() data after router.options.hydrate has run; adjust any references inside the async map for match so the matches property consistently points to activeMatches rather than the earlier matches variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/router-core/src/ssr/ssr-client.ts`:
- Around line 173-202: The context passed to route.options.context is using the
stale matches array; update the construction of contextFnContext inside the loop
(where matches is assigned to the matches field) to use activeMatches instead
(i.e., set contextFnContext.matches = activeMatches) so context() runs against
the current activeMatchesSnapshot.get() data after router.options.hydrate has
run; adjust any references inside the async map for match so the matches
property consistently points to activeMatches rather than the earlier matches
variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bedb064-3fc0-4df6-b0c4-b75b918bb950
📒 Files selected for processing (4)
e2e/react-start/hmr/tests/app.spec.tspackages/react-router/src/Scripts.tsxpackages/router-core/src/ssr/ssr-client.tspackages/router-plugin/src/core/route-hmr-statement.ts
✅ Files skipped from review due to trivial changes (1)
- packages/react-router/src/Scripts.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router-plugin/src/core/route-hmr-statement.ts
This was referenced Apr 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
state/setStatetoget/setand move the React and Vue adapters fromcreateStoretocreateAtomTesting
CI=1 NX_DAEMON=false pnpm nx affected --target=test:eslint --exclude=examples/**,e2e/** --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx affected --target=test:types --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx affected --target=test:unit --outputStyle=stream --skipRemoteCacheSummary by CodeRabbit