Commit 8d14c3f
committed
fix(react): improve handling of routes nested under path="/"
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.1 parent 65531f3 commit 8d14c3f
File tree
2 files changed
+81
-3
lines changed- packages/react
- src
- test
2 files changed
+81
-3
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
426 | 426 | | |
427 | 427 | | |
428 | 428 | | |
429 | | - | |
430 | | - | |
| 429 | + | |
| 430 | + | |
431 | 431 | | |
432 | 432 | | |
433 | | - | |
| 433 | + | |
434 | 434 | | |
435 | 435 | | |
436 | 436 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
594 | 594 | | |
595 | 595 | | |
596 | 596 | | |
| 597 | + | |
| 598 | + | |
| 599 | + | |
| 600 | + | |
| 601 | + | |
| 602 | + | |
| 603 | + | |
| 604 | + | |
| 605 | + | |
| 606 | + | |
| 607 | + | |
| 608 | + | |
| 609 | + | |
| 610 | + | |
| 611 | + | |
| 612 | + | |
| 613 | + | |
| 614 | + | |
| 615 | + | |
| 616 | + | |
| 617 | + | |
| 618 | + | |
| 619 | + | |
| 620 | + | |
| 621 | + | |
| 622 | + | |
| 623 | + | |
| 624 | + | |
| 625 | + | |
| 626 | + | |
| 627 | + | |
| 628 | + | |
| 629 | + | |
| 630 | + | |
| 631 | + | |
| 632 | + | |
| 633 | + | |
| 634 | + | |
| 635 | + | |
| 636 | + | |
| 637 | + | |
| 638 | + | |
| 639 | + | |
| 640 | + | |
| 641 | + | |
| 642 | + | |
| 643 | + | |
| 644 | + | |
| 645 | + | |
| 646 | + | |
| 647 | + | |
| 648 | + | |
| 649 | + | |
| 650 | + | |
| 651 | + | |
| 652 | + | |
| 653 | + | |
| 654 | + | |
| 655 | + | |
| 656 | + | |
| 657 | + | |
| 658 | + | |
| 659 | + | |
| 660 | + | |
| 661 | + | |
| 662 | + | |
| 663 | + | |
| 664 | + | |
| 665 | + | |
| 666 | + | |
| 667 | + | |
| 668 | + | |
| 669 | + | |
| 670 | + | |
| 671 | + | |
| 672 | + | |
| 673 | + | |
| 674 | + | |
597 | 675 | | |
598 | 676 | | |
599 | 677 | | |
| |||
0 commit comments