fix(router-core): use routeId for lifecycle hooks to fix onStay when loaderDeps change#6769
Conversation
…loaderDeps When loaderDeps change (e.g. from search param updates), the match.id changes because it includes the loaderDepsHash. This caused the lifecycle hooks (onLeave/onEnter/onStay) to compute exiting/entering/staying matches using match.id, which treated a loaderDeps change on the same route as 'leave + re-enter' instead of 'stay'. Fix: use match.routeId (consistent with loader's cause param logic) when computing exitingMatches, enteringMatches, and stayingMatches so that only an actual route change triggers onLeave/onEnter. Fixes TanStack#6765
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSeparate cache identity (match.id: route+path+loaderDeps) from lifecycle-hook identity (routeId only) so onEnter/onLeave/onStay are determined by route presence regardless of loaderDeps changes; tests added for loaderDeps/search-param scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Router
participant PrevMatches
participant NewMatches
participant Cache
participant LifecycleDispatcher
Router->>PrevMatches: load previous matches (ids, routeIds)
Router->>NewMatches: compute new matches (ids, routeIds)
Router->>Cache: compute id-based groups (exiting/entering/staying by id)
Router->>LifecycleDispatcher: compute routeId-based groups (hookExiting/hookEntering/hookStaying)
LifecycleDispatcher->>Router: dispatch onLeave/onEnter/onStay using routeId groups
Cache->>Router: run caching/GC using id-based groups
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/router-core/tests/callbacks.test.ts (1)
138-161: Consider assertingonStaypayload semantics, not only counts.Counts are good; adding
cause: 'stay'assertions would make this regression test harder to accidentally bypass.Suggested assertion hardening
await router.navigate({ to: '/foo', search: { page: '2' } }) expect(onEnter).toHaveBeenCalledTimes(1) // no new onEnter expect(onLeave).toHaveBeenCalledTimes(0) // no onLeave expect(onStay).toHaveBeenCalledTimes(1) // onStay fires + expect(onStay).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + cause: 'stay', + search: expect.objectContaining({ page: '2' }), + }), + ) // Update again — onStay fires again await router.navigate({ to: '/foo', search: { page: '3' } }) expect(onEnter).toHaveBeenCalledTimes(1) expect(onLeave).toHaveBeenCalledTimes(0) expect(onStay).toHaveBeenCalledTimes(2) + expect(onStay).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ + cause: 'stay', + search: expect.objectContaining({ page: '3' }), + }), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/tests/callbacks.test.ts` around lines 138 - 161, The test currently only checks invocation counts for onStay; strengthen it by asserting the payload semantics include cause: 'stay' when loaderDeps change: after calling setupWithLoaderDeps and performing router.navigate to the same path with updated search (the second and third navigate calls), inspect the arguments passed to the onStay mock (onStay.mock.calls) and add expectations that the event object passed to onStay contains a property cause with value 'stay' (and optionally that other expected fields like the new search are present) so the assertions validate both invocation and correct payload semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/router-core/tests/callbacks.test.ts`:
- Around line 138-161: The test currently only checks invocation counts for
onStay; strengthen it by asserting the payload semantics include cause: 'stay'
when loaderDeps change: after calling setupWithLoaderDeps and performing
router.navigate to the same path with updated search (the second and third
navigate calls), inspect the arguments passed to the onStay mock
(onStay.mock.calls) and add expectations that the event object passed to onStay
contains a property cause with value 'stay' (and optionally that other expected
fields like the new search are present) so the assertions validate both
invocation and correct payload semantics.
|
View your CI Pipeline Execution ↗ for commit e485707
☁️ Nx Cloud last updated this comment at |
|
@schiller-manuel do you agree that when params change (e.g. nav from To me this change makes sense, just asking for a second opinion. |
|
@sleitor don't worry about the "Bundle Size" job failing, this is unrelated to your PR, i'm working on it. |
|
Thanks for the heads up @Sheraff, appreciated! |
| exitingMatches = previousMatches.filter( | ||
| (match) => !newMatches.some((d) => d.id === match.id), | ||
| (match) => | ||
| !newMatches.some((d) => d.routeId === match.routeId), | ||
| ) | ||
| enteringMatches = newMatches.filter( | ||
| (match) => | ||
| !previousMatches.some((d) => d.id === match.id), | ||
| !previousMatches.some( | ||
| (d) => d.routeId === match.routeId, | ||
| ), | ||
| ) | ||
| stayingMatches = newMatches.filter((match) => | ||
| previousMatches.some((d) => d.id === match.id), | ||
| previousMatches.some( | ||
| (d) => d.routeId === match.routeId, | ||
| ), |
There was a problem hiding this comment.
This change might mess up the caching behavior: a cache entry should be route id + params + deps (what we have currently). But I may agree that as far as lifecycle events are concerned, it may not exactly correlate with the cache changes.
I had codex whip up a quick test showing the expected cache behavior #6772. This test works on main and should fail w/ your PR.
Now we need to decide the semantics of those lifecycle events. Are they tied to the cache entries' lifecycles? Or are they more tied to the route itself (i.e. routeId)?
There was a problem hiding this comment.
That's a great distinction to make. I think cache entries and lifecycle events serve different purposes and shouldn't necessarily share the same identity:
- Cache entries are about data identity —
route + params + depsmakes sense here, because different params or deps mean different data - Lifecycle hooks are about route presence —
onEnter/onStay/onLeavedescribe whether a route is appearing, persisting, or disappearing in the matched tree
From a user perspective, if I navigate from /posts/123 to /posts/456, I'd expect onStay for the /posts/$id route (the route is still there, just showing different data), not onLeave+onEnter. The cache correctly gets a new entry for 456, but the route itself didn't leave.
That said, I'm happy to defer to whatever semantics you and @schiller-manuel decide are correct here. If the conclusion is that lifecycle events should follow cache identity, I can revert and we can look for a different fix for #6765.
There was a problem hiding this comment.
the semantics as you described them are correct. so we need to fix the caching regression that @Sheraff pointed out and then we can merge this
There was a problem hiding this comment.
Good suggestion — done in the latest commit. Merged setupWithLoaderDeps and setupWithLoaderDepsForCaching into a single createFooRouter helper that accepts all options (onEnter/onLeave/onStay, loader, staleTime). All 9 tests still pass.
…ook identity (routeId) Cache entries use match.id (routeId + params + loaderDeps) so navigating between different params/deps correctly caches the previous match. Lifecycle hooks (onEnter/ onStay/onLeave) use routeId to track route *presence* in the matched tree — so navigating /posts/123 → /posts/456 fires onStay for /posts/$id, not onLeave+onEnter. Addresses caching regression spotted by @Sheraff in PR TanStack#6772 test.
|
Fixed in commit c8be9de — the caching regression is resolved by separating the two concerns:
Sheraff's test from #6772 should now pass with this change — the previous match gets cached and reused within staleTime as expected. |
SeanCassiere
left a comment
SeanCassiere
left a comment
There was a problem hiding this comment.
In terms of the events being fired, could we check to see whether there's testing in-place, to confirm that the router instance still emits the onBeforeNavigate and onResolved events on navigate?
In particular, to make sure we don't break Sentry's tracing integration - https://github.com/getsentry/sentry-javascript/blob/develop/packages/react/src/tanstackrouter.ts
- Add same-route match caching tests (from PR TanStack#6772 by Sheraff) to verify that switching lifecycle hooks to routeId does NOT break cache identity (which still correctly uses match.id = routeId + params + loaderDeps). Tests cover both loaderDeps variants and path param variants. - Add onBeforeNavigate subscription event tests to address SeanCassiere's request to verify router-level events still fire correctly after this change. These events are used by integrations like Sentry's TanStack Router instrumentation for page-transition tracking. All 1017 tests pass (29 test files, 0 type errors).
|
Addressed both remaining review requests in commit eb67444 — test coverage added to @Sheraff — caching regression tests (from #6772) Added the two tests from your test PR to verify that switching lifecycle hooks to
Both tests confirm that @SeanCassiere — Added three tests for the
Re: Full suite: 1017 tests pass, 0 type errors (29 test files). |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/router-core/tests/callbacks.test.ts (1)
43-73: Extract shared/foo+loaderDepsrouter setup to reduce duplication.
setupWithLoaderDepsandsetupWithLoaderDepsForCachingduplicate route construction andloaderDepslogic; consolidating lowers drift risk in future lifecycle/caching tests.♻️ Proposed refactor
+const pageLoaderDeps = ({ search }: { search: Record<string, unknown> }) => ({ + page: search['page'], +}) + +const createFooRouter = (opts: { + onEnter?: () => void + onLeave?: () => void + onStay?: () => void + loader?: () => unknown + staleTime?: number +} = {}) => { + const rootRoute = new BaseRootRoute({}) + const fooRoute = new BaseRoute({ + getParentRoute: () => rootRoute, + path: '/foo', + loaderDeps: pageLoaderDeps, + onEnter: opts.onEnter, + onLeave: opts.onLeave, + onStay: opts.onStay, + loader: opts.loader, + staleTime: opts.staleTime, + gcTime: opts.staleTime, + }) + return new RouterCore({ + routeTree: rootRoute.addChildren([fooRoute]), + history: createMemoryHistory(), + }) +}Also applies to: 167-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/tests/callbacks.test.ts` around lines 43 - 73, The test helper duplication: consolidate the duplicated `/foo` route + loaderDeps construction by extracting a single reusable function (e.g., setupWithLoaderDeps) that builds BaseRootRoute, BaseRoute (with getParentRoute, path '/foo', loaderDeps, onEnter/onLeave/onStay), produces routeTree and returns a RouterCore; then update the other helper (setupWithLoaderDepsForCaching) and any tests that used the old duplicate logic to call this single helper (optionally parameterize loaderDeps values or callbacks) so all tests reference the same construction (look for symbols setupWithLoaderDeps, setupWithLoaderDepsForCaching, BaseRootRoute, BaseRoute, loaderDeps, routeTree, RouterCore).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/router-core/tests/callbacks.test.ts`:
- Around line 43-73: The test helper duplication: consolidate the duplicated
`/foo` route + loaderDeps construction by extracting a single reusable function
(e.g., setupWithLoaderDeps) that builds BaseRootRoute, BaseRoute (with
getParentRoute, path '/foo', loaderDeps, onEnter/onLeave/onStay), produces
routeTree and returns a RouterCore; then update the other helper
(setupWithLoaderDepsForCaching) and any tests that used the old duplicate logic
to call this single helper (optionally parameterize loaderDeps values or
callbacks) so all tests reference the same construction (look for symbols
setupWithLoaderDeps, setupWithLoaderDepsForCaching, BaseRootRoute, BaseRoute,
loaderDeps, routeTree, RouterCore).
…teFooRouter helper
|
Addressed CodeRabbit's nitpick: consolidated |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
packages/router-core/src/router.ts
Outdated
| @@ -2426,6 +2436,23 @@ export class RouterCore< | |||
| previousMatches.some((d) => d.id === match.id), | |||
| ) | |||
There was a problem hiding this comment.
We don't use those anymore, do we?
There was a problem hiding this comment.
You're right — and are no longer used after the split. Only is still needed (for populating ). Removed both in the latest commit.
After splitting match identity into cache-level (match.id) and lifecycle-hook-level (match.routeId), enteringMatches and stayingMatches became unused. Only exitingMatches is still needed for cachedMatches. Addresses review comment from @Sheraff
4 participants
Summary
Fixes #6765
When
loaderDepschange (e.g. from search param updates), thematch.idchanges because it includes theloaderDepsHash. The lifecycle hooks (onLeave/onEnter/onStay) were computingexitingMatches,enteringMatches, andstayingMatchesusingmatch.id, which caused a loaderDeps change on the same route to be treated as "leave + re-enter" instead of "stay".Root Cause
match.idis composed asrouteId + interpolatedPath + loaderDepsHash. WhenloaderDepschange (e.g. due to a search param update), the hash changes andmatch.idchanges, making the old and new matches appear as different entries.The loader's
causeparameter already handles this correctly by usingrouteId:But the lifecycle hooks were using
match.id:Fix
Use
match.routeId(consistent with the loader'scauseparam logic) when computingexitingMatches,enteringMatches, andstayingMatches.Test
Added a test case in
callbacks.test.tsthat verifiesonStayfires (notonLeave/onEnter) when navigating within the same route and only loaderDeps-affecting search params change.Summary by CodeRabbit
Bug Fixes
Tests