Repository navigation
fix(react): Parameterize React Router transactions with encoded params - #25639
Conversation
`location.pathname` is percent-encoded, but React Router decodes the pathname before matching, so `RouteMatch.pathname` and `pathnameBase` are decoded. The instrumentation compared the two with plain string equality, so a route like `/clubs/:clubId` visited as `/clubs/%ED%95%9C%EA%B8%80` never matched and the transaction fell back to the raw URL with source `url`. Decode both sides, segment by segment like React Router's `decodePath`, before comparing in `getNormalizedName` and before stripping `pathnameBase` for descendant routes.
s1gr1d
left a comment
There was a problem hiding this comment.
Thanks, the looking into this and tests are great!
Instead of decoding, could we compare segment counts? Decoding never changes the number of / segments, so we don't need decodePathname. That also avoids two issues: /clubs/%2541 still doesn't match, because match.pathname gets decoded twice. And descendant routes now pass decoded paths back into matchRoutes, so /clubs/100%25 logs a "could not be decoded" warning.
Your tests already pass with the segment approach. Could you add those two URLs as cases too?
| * at all). Decoding both sides before comparing them keeps routes with an encoded parameter (non-ASCII, spaces, | ||
| * reserved characters) matchable regardless of which side is still encoded. | ||
| */ | ||
| export function decodePathname(pathname: string): string { |
There was a problem hiding this comment.
Heads-up: react-router 6.22.1+ already decodes match.pathname, so this decodes it a second time. A literal %41 in a param then doesn't match.
There was a problem hiding this comment.
Right, that double decode is gone with the segment-count approach. Resolved in the follow-up commit.
Drop decodePathname. getNormalizedName now checks that a branch spans as many segments as the location, and the descendant route lookups strip the parent pathnameBase from the raw pathname by segment count, so React Router decodes the remainder only once when matching it again. Prefix the basename without its trailing slash, since a basename such as '/' or '/app/' now reaches the route name return paths. Add /clubs/%2541 and /clubs/100%25 cases.
|
Thanks, that's much simpler. I removed
One side effect worth a look: a basename ending in |
|
Regarding the side-effect syou mentioned, they are fine: a basename with a trailing slash now gets parameterized like any other, and the case-insensitive match is the same as what React Router does. |
This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. See #25639 Co-authored-by: s1gr1d <32902192+s1gr1d@users.noreply.github.com>
Closes #25638
pnpm run lint) & (pnpm run test).location.pathnameis percent-encoded, but React Router decodes the pathname before matching, soRouteMatch.pathnameandpathnameBaseare decoded. The React Router instrumentation compared the two with plain string equality, so a route like/clubs/:clubIdvisited as/clubs/%ED%95%9C%EA%B8%80never matched and the transaction fell back to the raw URL with sourceurl. Every non-ASCII parameter value became its own transaction name. Descendant routes (<Routes>nested under a/*route) produced the raw URL with sourceroute.Decoding never changes the number of
/segments (an encoded slash stays%2F), so this compares segment counts instead of strings:getNormalizedNamechecks that the branch spans as many segments as the location, and the descendant-route lookups strip the parentpathnameBasefrom the rawlocation.pathnameby segment count, the wayuseRoutesdoes. The remainder stays encoded, so React Router decodes it exactly once when matching it again. This works whichever side is encoded (older 6.x routers matched the raw pathname or useddecodeURI), and values like%2541or100%25are not decoded twice. Since the check no longer relies on string equality, a basename ending in/('/','/app/') or differing in case from the URL now yields a parameterized name as well; the basename is prefixed without its trailing slash so the name has no//.Validation (in
packages/react):vitest runon the React Router suites (react-router,reactrouterv6,reactrouter-descendant-routes,reactrouter-cross-usage,reactrouter-compat-utils/*): 277 tests passed; the whole package: 400 passeddevelop; the%2541, trailing-slash basename and descendant100%25tests also fail on the previous (decoding) revision of this PRpnpm run lintandpnpm run lint:typesclean;oxfmt --checkpassed on the changed files