Skip to content

Conversation

@imsherrill
Copy link

@imsherrill imsherrill commented Jan 21, 2026

Summary

When navigating with a string URL (e.g., navigate({ to: '/users/abc123' })), the match snapshot was incorrectly using params from the current location instead of extracting params from the destination URL.

This caused Route.useParams() to return undefined for dynamic params when the source route didn't have those params in its path.

The Bug

In buildLocation(), when dest.params is not provided:

  • nextParams was set to fromParams (params from current location)
  • The snapshot was built with these incorrect params
  • matchRoutesInternal() used the snapshot via fast-path
  • Matches had wrong params, causing useParams() to fail

Example scenario:

  • Navigate from /users/123/settings to /users/123/posts/abc
  • Using navigate({ to: '/users/123/posts/abc' }) (string URL)
  • Route.useParams() returned { userId: '123' } but postId was undefined
  • This happened because /settings route didn't have postId in its params

The Fix

Extract params from the final interpolated pathname using getMatchedRoutes() before building the snapshot:

// Before: used nextParams which could be wrong for string URLs
params: nextParams,

// After: extract params from the actual destination URL
const { routeParams: extractedParams } = this.getMatchedRoutes(nextPathname)
params: extractedParams,

Why This Only Affected Production

The snapshot fast-path (line 1290-1295 in matchRoutesInternal) is only used when the snapshot's session ID matches the current router session. In development with hot reloading, session IDs often don't match, causing the fallback path (which correctly extracts params from the URL) to be used instead.

Test Plan

  • Existing load.test.ts tests pass (26 tests)
  • Existing callbacks.test.ts tests pass (3 tests)
  • Existing match-by-path.test.ts tests pass (91 tests)
  • Existing path.test.ts tests pass (205 tests)

Considerations

A few things we thought about but believe are acceptable trade-offs:

  1. Extra getMatchedRoutes call: This adds one additional route matching operation per navigation. Route matching is a fast trie lookup, and this is called once per navigation (not in a hot loop), so the performance impact should be negligible. Happy to explore reusing the existing destMatches result if you'd prefer to avoid this.

  2. parsedParams typing: When using typed params with non-string values (e.g., params: { id: 123 }), the snapshot's parsedParams will now contain strings extracted from the URL rather than the original types. However, params are re-parsed later in the match creation flow using the route's parseParams function, so this shouldn't cause issues in practice.

  3. opts.leaveParams edge case: If leaveParams is true, the pathname retains $param placeholders and params won't be extracted. This is an existing edge case for template path preservation that this fix doesn't make worse.

Open to feedback on any of these points!

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Fixed an issue where route parameters were not correctly preserved during back/forward browser navigation when using string-based URLs.

✏️ Tip: You can customize this high-level summary in your review settings.

…ing navigation

When navigating with a string URL (e.g., `navigate({ to: '/items/abc123' })`),
the match snapshot was incorrectly using params from the current location
(`fromParams`) instead of extracting params from the destination URL.

This caused `Route.useParams()` to return undefined for dynamic params in
production when the source route didn't have those params in its path.

The fix extracts params from the final interpolated pathname using
`getMatchedRoutes()` before building the snapshot, ensuring params are
always correct regardless of whether explicit `params` were provided.

Fixes the case where:
- Navigating from `/reviews/123/overview` to `/reviews/123/proposer-budget/abc`
- Using `navigate({ to: '/reviews/123/proposer-budget/abc' })` (string URL)
- `Route.useParams()` in the destination returned `{ reviewId: '123' }` but
  `proposerBudgetId` was undefined because it wasn't in the source route's params
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 21, 2026

📝 Walkthrough

Walkthrough

In the buildLocation function within router-core's router.ts, the snapshot creation for back/forward navigation now derives route parameters directly from the interpolated pathname (extractedParams) instead of using the previously passed nextParams, ensuring correct parameter representation when dest.params is undefined.

Changes

Cohort / File(s) Summary
Param Derivation in buildLocation
packages/router-core/src/router.ts
Modified snapshot construction to use extractedParams (derived from nextPathname) instead of nextParams when building match snapshots, addressing cases where dest.params may be undefined during string URL navigation

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • schiller-manuel
  • nlynzaad
  • Sheraff

Poem

🐰 A hop, skip, and param extraction
Paths now yield their secrets true,
No more undefined params askew,
The interpolated way shines through,
Snapshots built with values new! 🌟

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main fix: extracting params from the URL for snapshot generation during string navigation, which is the core issue addressed in the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nlynzaad
Copy link
Contributor

nlynzaad commented Jan 24, 2026

@imsherrill thanks for this. I'm closing this as we reverted the PR that introduced the snapshots and will revisit this at a later date with more regression tests.

@nlynzaad nlynzaad closed this Jan 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants