ref(ui): decouple scraps/link from sentry specific code #102765
Conversation
so that we can call useLocation inside
…inkBehaviorContext set both the sentry app and tests set that, so this is a defensive fallback
this ensures links are rendered with RouterLinks
React Router falls back to rendering an 'a' tag anyway
github-actions
bot
added
the
Scope: Frontend
|
@JonasBa @evanpurkhiser I found two more places where we render a
sentry/static/app/bootstrap/processInitQueue.tsx Lines 43 to 47 in 781c011
sentry/static/app/views/integrationPipeline/pipelineView.tsx Lines 41 to 51 in 1471798 Since those two places use scraps components too, I think we should also wrap them in On the other hand, we should design scraps so that it “works” without a Provider around, it just isn’t augmented with sentry specific logic (like button tracking or the link transformation). I don’t know how relevant these things are in those two places, and I also wouldn’t know how to test them ... |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| behavior: ({to, ...props}: LinkProps) => { | ||
| const normalizedTo = locationDescriptorToTo(normalizeUrl(to, location)); | ||
|
|
||
| return { | ||
| to: normalizedTo, | ||
| ...props, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Bug: SentryLinkBehaviorProvider uses an external context, but useLinkBehavior() reads from a local context, bypassing custom link behavior.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The SentryLinkBehaviorProvider in static/app/scrapsProviders/link.tsx attempts to provide custom link behavior using an external LinkBehaviorContextProvider from @sentry/scraps. However, the useLinkBehavior() hook in static/app/components/core/link/link.tsx reads from a different, local LinkBehaviorContext. This mismatch prevents the custom behavior function, which includes URL normalization via normalizeUrl() and locationDescriptorToTo(), from ever being applied. Consequently, links will not have their URLs normalized as intended.
💡 Suggested Fix
Either modify useLinkBehavior() to read from the external @sentry/scraps context, or update SentryLinkBehaviorProvider to set values on the local context.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/scrapsProviders/link.tsx#L19-L26
Potential issue: The `SentryLinkBehaviorProvider` in
`static/app/scrapsProviders/link.tsx` attempts to provide custom link behavior using an
external `LinkBehaviorContextProvider` from `@sentry/scraps`. However, the
`useLinkBehavior()` hook in `static/app/components/core/link/link.tsx` reads from a
*different*, local `LinkBehaviorContext`. This mismatch prevents the custom `behavior`
function, which includes URL normalization via `normalizeUrl()` and
`locationDescriptorToTo()`, from ever being applied. Consequently, links will not have
their URLs normalized as intended.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
@sentry/scraps is an alias for static/app/components/core/
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| export function ScrapsTestingProviders({children}: {children: React.ReactNode}) { | ||
| return <SentryLinkBehaviorProvider>{children}</SentryLinkBehaviorProvider>; | ||
| } |
There was a problem hiding this comment.
Bug: Inconsistent Sentry Wrapping Between Test and Production
The test version of ScrapsTestingProviders is missing the SentryTrackingProvider wrapper, which is present in the production ScrapsProviders component. This inconsistency means tests won't include tracking context that production code has, potentially missing bugs related to tracking functionality. The production version wraps children with both SentryTrackingProvider and SentryLinkBehaviorProvider, but the test version only includes SentryLinkBehaviorProvider.
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues: |
This is a continuation of - #102644 which got reverted because it broke `_admin`. This version has the following changes: - We now add a `ScrapsProvider` around the root routes of `_admin`: 0f0615a - We now default to a `RouterLink` even if no Provider is found: 803054e - Falling back to an `a` tag is a bad idea because it will render `<a to=“...”>` which won’t work (this was the `_admin` issue). It turns out, this is actually how `disabled` is implemented for link, as will create links that can’t be clicked 🥲. I don’t think we need to decouple scraps from react-router right now; previously, we called `useLocation`, which would fail at runtime if there is no router provider, so we can’t use our `Link` component outside of a router context anyway. So now, we have a `ScrapsProvider` around all routes (I hope there aren’t any more places) and for cases where we have no Provider, we should still render a link from react-router.
This is a continuation of - #102644 which got reverted because it broke `_admin`. This version has the following changes: - We now add a `ScrapsProvider` around the root routes of `_admin`: 0f0615a - We now default to a `RouterLink` even if no Provider is found: 803054e - Falling back to an `a` tag is a bad idea because it will render `<a to=“...”>` which won’t work (this was the `_admin` issue). It turns out, this is actually how `disabled` is implemented for link, as will create links that can’t be clicked 🥲. I don’t think we need to decouple scraps from react-router right now; previously, we called `useLocation`, which would fail at runtime if there is no router provider, so we can’t use our `Link` component outside of a router context anyway. So now, we have a `ScrapsProvider` around all routes (I hope there aren’t any more places) and for cases where we have no Provider, we should still render a link from react-router.
This is a continuation of - #102644 which got reverted because it broke `_admin`. This version has the following changes: - We now add a `ScrapsProvider` around the root routes of `_admin`: 0f0615a - We now default to a `RouterLink` even if no Provider is found: 803054e - Falling back to an `a` tag is a bad idea because it will render `<a to=“...”>` which won’t work (this was the `_admin` issue). It turns out, this is actually how `disabled` is implemented for link, as will create links that can’t be clicked 🥲. I don’t think we need to decouple scraps from react-router right now; previously, we called `useLocation`, which would fail at runtime if there is no router provider, so we can’t use our `Link` component outside of a router context anyway. So now, we have a `ScrapsProvider` around all routes (I hope there aren’t any more places) and for cases where we have no Provider, we should still render a link from react-router.
This is a continuation of
which got reverted because it broke
_admin.This version has the following changes:
ScrapsProvideraround the root routes of_admin: 0f0615aRouterLinkeven if no Provider is found: 803054eatag is a bad idea because it will render<a to=“...”>which won’t work (this was the_adminissue). It turns out, this is actually howdisabledis implemented for link, as will create links that can’t be clicked 🥲. I don’t think we need to decouple scraps from react-router right now; previously, we calleduseLocation, which would fail at runtime if there is no router provider, so we can’t use ourLinkcomponent outside of a router context anyway.So now, we have a
ScrapsProvideraround all routes (I hope there aren’t any more places) and for cases where we have no Provider, we should still render a link from react-router.