fix: commit route context #6053
Conversation
… faster than beforeLoad resolves
WalkthroughDeferred route-context merging: router-core no longer mutates the shared context during beforeLoad execution but records per-match beforeLoad context and commits a merged context after loaders complete. Tests for React and Solid validate context during immediate navigation; several store-update test expectations were relaxed/increased. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 562ad18
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/react-router/tests/store-updates-during-navigation.test.tsx (1)
139-139: Document rationale for higher update counts and consider relaxing overly brittle assertionsThe new expectations reflect extra store updates after the route-context commit changes, but the tests still read as "these numbers should not increase" without explaining why some increases are acceptable (e.g. now 12/10/19 updates). For future maintainers, it would help to briefly note that the additional updates are due to the new context commit path rather than accidental regressions, and, where behavior is known to vary (like the “nothing” case), prefer bounded ranges over exact counts to avoid unnecessary flakiness or churn when internal batching changes again.
Also applies to: 157-157, 173-173, 184-185, 226-226, 242-242, 258-258, 274-274, 292-292
packages/solid-router/tests/routeContext.test.tsx (1)
5-5: Solid immediate‑navigation context test looks good; tighten assertion and update stale noteThe new Solid test correctly exercises the “sleep in beforeLoad + immediate navigation” scenario and the
createEffect‑based tracking ofuseRouteContext()is a good fit. Two small follow‑ups:
- The note above the test now claims it passes only in solid-router but fails in react-router, which is no longer accurate given the new React test; consider updating or removing that comment to avoid confusion.
- To guard against a vacuous pass if
contextValueswere ever empty, add an assertion likeexpect(contextValues.length).toBeGreaterThan(0)before theevery(...)check.Also applies to: 2396-2452
packages/react-router/tests/routeContext.test.tsx (1)
12-12: React immediate‑navigation context test is solid; consider a non‑empty guard oncontextValuesThe new React test accurately mirrors the Solid scenario and validates that
useRouteContext()never exposes a pre‑beforeLoad context during immediate navigation; theuseEffect+navigatepattern is appropriate here, even underreactStrictModewhere the effect may fire twice.To make the assertion more robust, you might also assert that
contextValues.length > 0before calling.every(...), so the test can’t pass if, for some reason, the root component stopped rendering oruseRouteContext()stopped producing values.Also applies to: 2479-2532
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/react-router/tests/routeContext.test.tsx(2 hunks)packages/react-router/tests/store-updates-during-navigation.test.tsx(9 hunks)packages/router-core/src/load-matches.ts(7 hunks)packages/solid-router/tests/routeContext.test.tsx(2 hunks)
🧰 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/react-router/tests/routeContext.test.tsxpackages/react-router/tests/store-updates-during-navigation.test.tsxpackages/solid-router/tests/routeContext.test.tsxpackages/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/react-router/tests/routeContext.test.tsxpackages/react-router/tests/store-updates-during-navigation.test.tsxpackages/solid-router/tests/routeContext.test.tsxpackages/router-core/src/load-matches.ts
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/react-router/tests/routeContext.test.tsxpackages/react-router/tests/store-updates-during-navigation.test.tsxpackages/solid-router/tests/routeContext.test.tsxpackages/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)
381-387: Before‑load context refactor and loader context aggregation look consistent with the new semanticsThe changes in
executeBeforeLoadandgetLoaderContextare a clear improvement:
pending()andupdateContext()no longer mutatematch.contextmid‑beforeLoad; they only flip fetching flags and stash results into__beforeLoadContext, which avoids exposing partially‑merged context to components.- The beforeLoad
contextargument now merges router context, parentcontext, parent__beforeLoadContext, and the current route’s__routeContext, so child beforeLoads always see parent contributions even when the parent’s context hasn’t been committed yet.- Loader context is now derived by walking
inner.matches[0..index]and merging each match’s__routeContextand__beforeLoadContext, which produces a deterministic aggregate regardless of load ordering and matches how tests expect composed context to behave.This structure matches the intent of the new route‑context tests in both React and Solid and keeps the “source of truth” for composition in the non‑reactive fields rather than
match.contextitself.Also applies to: 398-400, 426-432, 459-465, 572-584
schiller-manuel
force-pushed
the
route-context-commit
branch
from
42d4a67 to
562ad18
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/solid-router/tests/store-updates-during-navigation.test.tsx (1)
125-298: Align flakiness comments and expectations, and consider ranges everywhereThe relaxed assertions using
toBeGreaterThanOrEqual/toBeLessThanOrEqualfor the async cases are a good way to handle small variability.Two minor points:
- The comments still say “sometimes (rarely) is 12” / “7”, but the upper bounds are now
<= 13/<= 8. It’d be clearer to either generalize the comment (“occasionally higher”) or update the numbers to match.- For the other tests that now use exact counts (7, 10, 19, 9, 7, 3), you might want to consider
<= Nstyle expectations as in the flaky ones, so the suite continues to guard against too many updates without overfitting to exact internals.packages/react-router/tests/store-updates-during-navigation.test.tsx (1)
125-294: Keep flaky‑test comments in sync with bounds and avoid over‑fitting exact countsThe move to
[11,13]and[8,9]ranges is a sensible way to account for minor variability in update counts.Two small follow‑ups:
- The comments still say “sometimes (rarely) is 12”, but
updatesis now allowed up to 13; it’d be good to either relax the wording or adjust the numbers so future readers aren’t surprised.- For the remaining tests with exact
toBe(...)expectations, you might consider<= Nor a tight range (as in the first and “nothing” tests), which better matches the suite’s intent (“doesn't update too many times”) and reduces churn when internal scheduling changes but behavior is still acceptable.packages/router-core/src/load-matches.ts (1)
367-373: BeforeLoad context may miss ancestorbeforeLoadvalues for the current navigation
beforeLoadFnContext.contextaggregates as:const parentMatchContext = parentMatch?.context ?? inner.router.options.context ?? undefined context: { ...parentMatchContext, ...parentMatch?.__beforeLoadContext, ...match.__routeContext, }However, the execution model runs all
beforeLoadhooks sequentially first (lines 921–927), then all loaders in parallel (lines 929–931). This means when a deep child'sbeforeLoadexecutes:
parentMatch.contextstill reflects the previous navigation (sincecommitContext()is only called after loaders complete, at line 840 or 882)- you only include the immediate parent's
__beforeLoadContext, not any grandparent or deeper ancestor values from the current navigationIn contrast, loaders (via
getLoaderContext, lines 562–606) and the finalmatch.context(viacommitContext, lines 749–764) both aggregate all ancestors'__routeContextand__beforeLoadContextby looping0..index, ensuring consistent context across a deep match hierarchy.If the design intent is for each
beforeLoadto see the same aggregated context available to loaders and components for the same ancestors, mirror the aggregation strategy ingetLoaderContext—walkinner.matches[0..index-1]and fold their__routeContextand__beforeLoadContextrather than relying on the potentially staleparentMatch.context.
🧹 Nitpick comments (2)
packages/vue-router/tests/store-updates-during-navigation.test.tsx (1)
126-305: Consider preferring upper bounds/ranges over exact update counts for long‑term stabilityThe updated expectations (33, 13, 31, 23, 47, 24, 22, 18, 7) look consistent with the new context/loader behavior, but asserting exact counts for most cases makes these tests quite brittle to internal refactors.
Since this suite is already framed as “doesn't update too many times” and you’re already using
>=/<=bounds for the “nothing” case, it may be more robust to standardize on “≤ N” (or a small range) instead of stricttoBe(N)here as well, so small internal re-orderings don’t force mechanical test updates while still catching real regressions.packages/router-core/src/load-matches.ts (1)
444-467: Nice fix on context commit ordering; consider centralizing aggregation helper and tightening base context handlingThe new strategy:
- storing only
__beforeLoadContextinexecuteBeforeLoad,- computing loader
contextingetLoaderContextby folding__routeContext+__beforeLoadContextfor matches up toindex, and- committing
match.contextincommitContext()after loaders complete (or are skipped),correctly removes the old dependence on parent commit timing and makes
contextdeterministic with respect to the match tree rather than loader ordering. This directly addresses the prior race onmatch.context.Two small follow‑ups you might consider:
DRY aggregation logic
getLoaderContextandcommitContextare nearly identical in how they aggregate:let context = inner.router.options.context ?? {} for (let i = 0; i <= index; i++) { const m = inner.router.getMatch(inner.matches[i]!.id) context = { ...context, ...(m.__routeContext ?? {}), ...(m.__beforeLoadContext ?? {}), } }versus:
const context = { ...inner.router.options.context } for (let i = 0; i <= index; i++) { const m = inner.router.getMatch(innerMatch.id) Object.assign(context, m.__routeContext, m.__beforeLoadContext) }Extracting a shared helper (e.g.
buildContextUntilIndex(inner, index)) would keep loader and committed contexts in lockstep and reduce the chance of subtle divergence later.Defensive base context initialization
getLoaderContextusesinner.router.options.context ?? {}, which safely handlesundefined/null.commitContextuses object spread oninner.router.options.context, which will throw if someone ever passesnull.Matching
getLoaderContext’s pattern here (e.g. start fromconst base = inner.router.options.context ?? {}; const context = { ...base };) would guard against that edge case and keep both call sites consistent.Also applies to: 562-584, 749-764, 879-883
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/react-router/tests/store-updates-during-navigation.test.tsx(9 hunks)packages/router-core/src/load-matches.ts(7 hunks)packages/solid-router/tests/store-updates-during-navigation.test.tsx(8 hunks)packages/vue-router/tests/store-updates-during-navigation.test.tsx(9 hunks)
🧰 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/solid-router/tests/store-updates-during-navigation.test.tsxpackages/react-router/tests/store-updates-during-navigation.test.tsxpackages/vue-router/tests/store-updates-during-navigation.test.tsxpackages/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/solid-router/tests/store-updates-during-navigation.test.tsxpackages/react-router/tests/store-updates-during-navigation.test.tsxpackages/vue-router/tests/store-updates-during-navigation.test.tsxpackages/router-core/src/load-matches.ts
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/solid-router/tests/store-updates-during-navigation.test.tsxpackages/react-router/tests/store-updates-during-navigation.test.tsxpackages/vue-router/tests/store-updates-during-navigation.test.tsx
📚 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: Add or update tests for any code changes
Applied to files:
packages/solid-router/tests/store-updates-during-navigation.test.tsx
⏰ 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
fixes #6040
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.