-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(react): Avoid multiple name updates on navigation spans #17438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
aea2217
to
57c8b22
Compare
size-limit report 📦
|
0e679dc
to
27c5da6
Compare
8da7c27
to
453f5c6
Compare
1d2839b
to
8507984
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the React Router v6/v7 compatibility utilities to prevent multiple navigation span name updates and fix incorrect transaction names for lazy routes.
- Refactors the
reactrouterv6-compat-utils
module into three separate files for better organization - Implements tracking to prevent duplicate navigation span name updates during navigation
- Fixes transaction naming issues that occurred when navigating between lazy routes
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
packages/react/test/reactrouterv6-compat-utils.test.tsx |
Removes entire test file as functionality moved to new modules |
packages/react/test/reactrouter-cross-usage.test.tsx |
Removes assertions expecting multiple updateName calls since they're now prevented |
packages/react/test/reactrouter-compat-utils/utils.test.ts |
Adds comprehensive tests for utility functions moved from old module |
packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts |
Adds tests for lazy route handling functionality |
packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx |
Adds tests for instrumentation-specific functionality |
packages/react/src/reactrouterv7.tsx |
Updates import path to new module structure |
packages/react/src/reactrouterv6.tsx |
Updates import path to new module structure |
packages/react/src/reactrouter-compat-utils/utils.ts |
New utility functions module with core router utilities |
packages/react/src/reactrouter-compat-utils/lazy-routes.tsx |
New module for lazy route handling functionality |
packages/react/src/reactrouter-compat-utils/instrumentation.tsx |
New main instrumentation module with navigation span tracking |
packages/react/src/reactrouter-compat-utils/index.ts |
New index file exporting all functionality from sub-modules |
dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts |
Adds tests for navigation between lazy routes |
dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/InnerLazyRoutes.tsx |
Adds navigation link to another lazy route |
dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/Index.tsx |
Adds navigation links to lazy routes |
dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/AnotherLazyRoutes.tsx |
Adds new lazy route component with navigation links |
dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/index.tsx |
Adds another lazy route configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/react/src/reactrouter-compat-utils/instrumentation.tsx
Outdated
Show resolved
Hide resolved
packages/react/src/reactrouter-compat-utils/instrumentation.tsx
Outdated
Show resolved
Hide resolved
1431408
to
60a9d7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew big change – Did not review all the tests in detail but looks good 🚀
packages/react/src/reactrouter-compat-utils/instrumentation.tsx
Outdated
Show resolved
Hide resolved
@onurtemizkan Added some minor things myself so I can release that tomorrow, thanks for the refactor! |
This patch prevents name updates on navigation spans from being applied more than once.
This was fine and allowed for cross-usage of different instrumentation ways (such as instrumenting
useRoutes
andcreateBrowserRouter
together), and didn't create any issues. But after adding support for lazy routes, it led to incorrect transaction names, especially when there's a navigation between two lazily-loaded routes.While on it, I also had to do some refactoring to keep the code readable and maintainable, so now
reactrouterv6-compat-utils
is separated into 3 source files (instrumentation, common utils, and lazy routes specific functionality).