fix(react): improve handling of routes nested under path="/" #14821
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We noticed this in Sentry's `/issues/:groupId/` route, which uses SDK v8.43.0. The full route tree is complex, but the relevent parts are:
```js
<Route path="/" element={<div>root</div>}>
<Route path="/issues/:groupId/" element={<div>issues group</div>}>
<Route index element={<div>index</div>} />
</Route>
</Route>
```
If you navigate to e.g. `/issues/123`, the recorded transaction name is
`//issues/:groupId/` (notice the double slash). This is messy but doesn't have
too much of a consequence.
The worse issue is when you navigate to e.g. `/issues/123/` (note the trailing
slash), the transaction name is `/issues/123/` - it is not parameterized. This
breaks transaction grouping. On the `master` and `develop` branch of the SDK,
the transaction name is recorded as `/`. This causes the transactions to be
grouped incorrectly with the root, as well as other routes with this structure
(nested under a route with path `/`).
This commit fixes both of these issues and passes both the new tests (which reproduce the above issues) and all existing tests, but I still don't trust the change.
First, `newPath` now always has a leading slash, and I don't know how that
interacts with the other path concatenations inside `getNormalizedName`. It
feels like dealing with path concatenation should be handled in a more
systematic way.
Second, I revert a change from 3473447 where
```js
if (basename + branch.pathname === location.pathname) {
```
became
```
if (location.pathname.endsWith(basename + branch.pathname)) {
```
This is the change that difference in results between SDK versions. This will
always match when `basename` is empty, `branch.pathname` is `/`, and
`location.pathname` ends with `/` - in other words, if you have a parent route
with `path="/"`, every request ending in a slash will match it instead of the
more specific child route. This seems likely to be a wide regression in
transaction names. But, no tests failed from this change, which makes me
worried that there's some necessary behaviour that isn't covered.
mjq
commented
Dec 20, 2024
Comment on lines
429
to
+430
| const newPath = path[0] === '/' || pathBuilder[pathBuilder.length - 1] === '/' ? path : `/${path}`; | ||
| pathBuilder += newPath; | ||
| pathBuilder = trimSlash(pathBuilder) + prefixWithSlash(newPath); |
There was a problem hiding this comment.
Originally I thought to change these lines to
const newPath = prefixWithSlash(path);
pathBuilder = trimSlash(pathBuilder) + newPath;but newPath is used in a different path concatenation further down the method and, even though no tests fail, I didn't want collateral damage. (Tests probably should have failed though - I think the path concatenation in this method could be better handled, and is under-tested.)
There was a problem hiding this comment.
I think we can make a refactor/cleanup once we get the tests in a better state.
onurtemizkan
approved these changes
Dec 23, 2024
There was a problem hiding this comment.
Thanks for the fix @mjq! I think it's safe to merge, and I agree that we need to improve tests to cover as many route declaration patterns as possible.
|
|
||
| // If the path matches the current location, return the path | ||
| if (location.pathname.endsWith(basename + branch.pathname)) { | ||
| if (trimSlash(location.pathname) === trimSlash(basename + branch.pathname)) { |
There was a problem hiding this comment.
I'd expect this to break descendant routes. But in the final implementation, descendant routes don't end up here. So, I believe it's safe to revert.
Comment on lines
429
to
+430
| const newPath = path[0] === '/' || pathBuilder[pathBuilder.length - 1] === '/' ? path : `/${path}`; | ||
| pathBuilder += newPath; | ||
| pathBuilder = trimSlash(pathBuilder) + prefixWithSlash(newPath); |
There was a problem hiding this comment.
I think we can make a refactor/cleanup once we get the tests in a better state.
|
Thanks @onurtemizkan! Going to merge in that case 👍 |
onurtemizkan
pushed a commit
that referenced
this pull request
Jan 3, 2025
We noticed this in Sentry's `/issues/:groupId/` route, which uses SDK
v8.43.0. The full route tree is complex, but the relevant parts are:
```js
<Route path="/" element={<div>root</div>}>
<Route path="/issues/:groupId/" element={<div>issues group</div>}>
<Route index element={<div>index</div>} />
</Route>
</Route>
```
If you navigate to e.g. `/issues/123` (no trailing slash), the recorded
transaction name is `//issues/:groupId/` (notice the double slash). This
looks messy but doesn't have too much of a consequence.
The worse issue is when you navigate to e.g. `/issues/123/` (with
trailing slash), the transaction name is `/issues/123/` - it is not
parameterized. This breaks transaction grouping. On the `master` and
`develop` branch of the SDK, the transaction name is recorded as `/`.
This causes the transactions to be grouped incorrectly with the root, as
well as any other routes nested under a route with `path="/"`.
(Thanks @JoshFerge for noticing the bad data in Sentry! 🙌)
---
Note that this commit effectively reverts a change from #14304 where
```js
if (basename + branch.pathname === location.pathname) {
```
became
```js
if (location.pathname.endsWith(basename + branch.pathname)) {
```
This is the change that caused the difference in results between SDK
versions. This will always match when `basename` is empty,
`branch.pathname` is `/`, and `location.pathname` ends with `/` - in
other words, if you have a parent route with `path="/"`, every request
ending in a slash will match it instead of the more specific child
route. (Depending on how often this kind of `Route` nesting happens in
the wild, this could be a wide regression in transaction names.) But, no
tests failed from the removal of `endsWith`. @onurtemizkan, who wrote
the change in question:
> I'd expect this to break descendant routes. But in the final
> implementation, descendant routes don't end up here. So, I
> believe it's safe to revert.
onurtemizkan
pushed a commit
that referenced
this pull request
Jan 3, 2025
We noticed this in Sentry's `/issues/:groupId/` route, which uses SDK
v8.43.0. The full route tree is complex, but the relevant parts are:
```js
<Route path="/" element={<div>root</div>}>
<Route path="/issues/:groupId/" element={<div>issues group</div>}>
<Route index element={<div>index</div>} />
</Route>
</Route>
```
If you navigate to e.g. `/issues/123` (no trailing slash), the recorded
transaction name is `//issues/:groupId/` (notice the double slash). This
looks messy but doesn't have too much of a consequence.
The worse issue is when you navigate to e.g. `/issues/123/` (with
trailing slash), the transaction name is `/issues/123/` - it is not
parameterized. This breaks transaction grouping. On the `master` and
`develop` branch of the SDK, the transaction name is recorded as `/`.
This causes the transactions to be grouped incorrectly with the root, as
well as any other routes nested under a route with `path="/"`.
(Thanks @JoshFerge for noticing the bad data in Sentry! 🙌)
---
Note that this commit effectively reverts a change from #14304 where
```js
if (basename + branch.pathname === location.pathname) {
```
became
```js
if (location.pathname.endsWith(basename + branch.pathname)) {
```
This is the change that caused the difference in results between SDK
versions. This will always match when `basename` is empty,
`branch.pathname` is `/`, and `location.pathname` ends with `/` - in
other words, if you have a parent route with `path="/"`, every request
ending in a slash will match it instead of the more specific child
route. (Depending on how often this kind of `Route` nesting happens in
the wild, this could be a wide regression in transaction names.) But, no
tests failed from the removal of `endsWith`. @onurtemizkan, who wrote
the change in question:
> I'd expect this to break descendant routes. But in the final
> implementation, descendant routes don't end up here. So, I
> believe it's safe to revert.
JoshFerge
added a commit
to getsentry/sentry
that referenced
this pull request
Jan 9, 2025
upgrading so we can get the fix for our transaction names in getsentry/sentry-javascript#14821 in!
andrewshie-sentry
pushed a commit
to getsentry/sentry
that referenced
this pull request
Jan 22, 2025
upgrading so we can get the fix for our transaction names in getsentry/sentry-javascript#14821 in!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We noticed this in Sentry's
/issues/:groupId/route, which uses SDK v8.43.0. The full route tree is complex, but the relevant parts are:If you navigate to e.g.
/issues/123(no trailing slash), the recorded transaction name is//issues/:groupId/(notice the double slash). This looks messy but doesn't have too much of a consequence.The worse issue is when you navigate to e.g.
/issues/123/(with trailing slash), the transaction name is/issues/123/- it is not parameterized. This breaks transaction grouping. On themasteranddevelopbranch of the SDK, the transaction name is recorded as/. This causes the transactions to be grouped incorrectly with the root, as well as any other routes nested under a route withpath="/".(Thanks @JoshFerge for noticing the bad data in Sentry! 🙌)
This commit fixes both of these issues and passes both the new tests (which reproduce the above issues) and all existing tests, but I don't trust my changes.
I effectively revert a change from #14304 where
became
This is the change that caused the difference in results between SDK versions. This will always match when
basenameis empty,branch.pathnameis/, andlocation.pathnameends with/- in other words, if you have a parent route withpath="/", every request ending in a slash will match it instead of the more specific child route. (Depending on how often this kind ofRoutenesting happens in the wild, this could be a wide regression in transaction names.) But, no tests failed from the removal ofendsWith, which makes me worried that there's some necessary behaviour that isn't covered and would be broken by this fix. @onurtemizkan can you give some context into that change? Thanks!