fix(router-core): use routeId for lifecycle hooks to fix onStay when loaderDeps change#6769
fix(router-core): use routeId for lifecycle hooks to fix onStay when loaderDeps change#6769sleitor wants to merge 3 commits intoTanStack:mainfrom
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces separate identity groups for caching and lifecycle hooks: cache identity (id + path + loaderDeps) remains, while lifecycle-hook identity uses Changes
Sequence Diagram(s)sequenceDiagram
participant Router
participant PrevMatches
participant NewMatches
participant LifecycleDispatcher
participant Cache
Router->>PrevMatches: load previous matches (with id, routeId)
Router->>NewMatches: compute new matches (with id, routeId)
Router->>Cache: compute cache identity matches (exiting/entering/staying by id)
Router->>LifecycleDispatcher: compute hook identity matches (exiting/entering/staying by routeId)
LifecycleDispatcher->>Router: dispatch onLeave/onEnter/onStay based on routeId groups
Cache->>Router: perform caching based on 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 c8be9de
☁️ 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
…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
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