Skip to content
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

[react-router-6] Nested Routers produce wrong parametrized URL #5997

Open
3 tasks done
tirli opened this issue Oct 19, 2022 · 13 comments
Open
3 tasks done

[react-router-6] Nested Routers produce wrong parametrized URL #5997

tirli opened this issue Oct 19, 2022 · 13 comments

Comments

@tirli
Copy link

tirli commented Oct 19, 2022

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/react

SDK Version

7.16

Framework Version

React 18

Link to Sentry event

https://sentry.io/organizations/astraea/issues/3684294425/?project=5995515&query=is%3Aunresolved

Steps to Reproduce

I have quite a complex router so I recreated a small part of it here with repro steps in the readme: https://github.com/tirli/repro-sentry-react-router

I'm pretty sure several nested SentryRoutes are to blame here but I cannot rewrite my usage of the router :( Also, I've seen a lot of similar issues that are blaming the wrong order of sentry init and router usage so I checked this case and I feel like it's not the problem here

Expected Result

http://localhost:5173/monitor/projects/:projectId/:siteId in error details

Actual Result

http://localhost:5173/monitor/projects/someProjectId/someSiteId

@smeubank
Copy link
Member

thanks for raising this issue! The team will take a look at it and try to get back to you.

repro steps much appreciated 🙏

@onurtemizkan
Copy link
Collaborator

Thanks for reporting this @tirli.

I was able to debug the reproduction, and indeed the problem seems to be the use of Descendant Routes.

I have tried two scenarios to figure out how we can support this pattern.


1 - When you only use SentryRoutes on the root, and unwrapped Routes in descendants.

In this scenario, createRoutesFromChildren (Remix utility that Sentry uses) only returns top level routes (which in your case are monitor/* and *). The child element props (thus the descendant routes) are inaccessible, as they are not rendered at the time we can access them.

In this case, we also lose pageload/navigation transactions of child routes, which IMO makes this approach unworthy to follow unless we find a way to access props inside Route elements.


2 - When you use SentryRoutes for every Routes instance in the app. (Your Case)

In this case, createRoutesFromChildren is called for every SentryRoutes instance, and each have their own slice of route. So we fail to match the parameters, as we don't have the full route.

For example, in your project:

Root SentryRoute has: monitor/*
First child SentryRoute has: project/ and so on.

We don't have any info about the parent/child relationships of routes in the current structure, so it might not be easy to make multiple SentryRoutes instances work together and form the full path. We can try using useOutletContext, to define parent / child flags on the root SentryRoutes, but that will require an API change on our side.


We do not support most of the new features of React Router 6.4 yet (#5872). Maybe we can come up with a better or simpler solution for descendant routes, while working on that.

cc: @AbhiPrasad

@AbhiPrasad
Copy link
Member

Reading this through I think this is tricky to make work with our current API. Might be worthwhile to just try working on #5872, and then say that for nested routers to work you need to use the 6.4 solution. Thoughts?

@onurtemizkan
Copy link
Collaborator

I agree, this would end up quite hacky even if we change our API. Haven't tried, but 6.4 seems to make it fairly easier for us to instrument.

@github-actions
Copy link
Contributor

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@tirli
Copy link
Author

tirli commented Nov 29, 2022

@onurtemizkan, not sure what is the status of this. I see that all mentioned issues are resolved, and PRs are merged. Should it work now?

@onurtemizkan
Copy link
Collaborator

@tirli - As discussed, we attempted to support this pattern while working on React Router 6.4, but unfortunately it was not included with the initial implementation for the reasons discussed in #6172, specifically:

When I tried to replicate the issue combining createBrowserRouter, wildcard paths, and descendant routes defined in separate <Routes>, the matches object we're getting from the router is still path/*. It's not about our name parameterisation utility, as we are not getting info from the router. Besides that, I'm not sure if this pattern is recommended with the new API, but it still works in React Router 6.4.

I still think there should be a way to support this, but it needs more investigation.

Adding Backlog label so this doesn't get closed by the bot.

@jtguignard
Copy link

I have the same issue with only one SentryRoutes on the root and a lot of nested routes, willing to keep real ids. I ended up overwriting transaction name in beforeSendTransaction with window.location.pathname. It feels really hacky and not a good practice at all, but at least all the transactions have now a correct name in Sentry.

@arusahni
Copy link

arusahni commented Oct 3, 2023

Adding a +1 here. I'm encountering this with react-router-dom@6.16.0 and @sentry/react@7.73.0.

I have a router with a nested wildcard/splat subrouter:

<SentryRouter>
  <Route path="/objects/:objectSlug/*" element={<ObjectRoutes />} />
</SentryRouter>

and ObjectRoutes returns:

<SentryRoutes>
  <Route path="/items" element={<ObjectItems />}/>
  <!-- others snipped for brevity -->
</SentryRoutes>

All detected navigate transactions for the nested /items route use /objects/:objectSlug/* as the name. Ideally I'd have /objects/:objectSlug/items.

@lforst
Copy link
Member

lforst commented Oct 10, 2023

I feel like this is something we should support. Shouldn't be too hard to do 🤞

@jasikpark
Copy link

Just to provide a data-point: I'm using RRv6 w/

 const sentryCreateBrowserRouter = Sentry.wrapCreateBrowserRouter(createBrowserRouter);
  const router = sentryCreateBrowserRouter([
    {
      path: '/*',
      errorElement: <OopsPageError />,
      element: <App />,
    },
  ]);

And then <App /> contains a <Routes /> with all of the app <Route />s, with nested <Routes /> below those as well.

Is there anything I can do to improve parmetrization of my traces? Currently everything gets registered with a path of /* in Sentry

@Lms24
Copy link
Member

Lms24 commented Jan 29, 2024

Thanks for the update! We're aware that this still needs to be addressed from our end. We just currently have to prioritize other tasks. If anyone wants to help our with this in the meantime, PRs are always welcome :)

@jasikpark
Copy link

I wonder if remix-run/react-router#11113 will improve the ease of fixing this at some point...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Status: No status
Development

No branches or pull requests

10 participants