-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ref(ui): decouple scraps/link from sentry specific code #102765
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
Changes from all commits
e136ad0
40dd7ce
d036136
3c8612c
7c0cb6b
f1625c5
5678cea
8d3ff2a
0f0615a
803054e
804c515
281b404
9e28155
9181eb7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| import {createContext, useContext, type FunctionComponent} from 'react'; | ||
| import {Link as RouterLink} from 'react-router-dom'; | ||
| import * as Sentry from '@sentry/react'; | ||
|
|
||
| import type {LinkProps} from './link'; | ||
|
|
||
| type LinkBehavior = { | ||
| behavior: (props: LinkProps) => LinkProps; | ||
| component: FunctionComponent<LinkProps>; | ||
| }; | ||
|
|
||
| const LinkBehaviorContext = createContext<LinkBehavior | null>(null); | ||
|
|
||
| const defaultLinkBehavior = { | ||
| component: RouterLink, | ||
| behavior: props => props, | ||
| } satisfies LinkBehavior; | ||
|
|
||
| export const LinkBehaviorContextProvider = LinkBehaviorContext.Provider; | ||
|
|
||
| export const useLinkBehavior = (props: LinkProps) => { | ||
| const linkBehavior = useContext(LinkBehaviorContext); | ||
|
|
||
| if (process.env.NODE_ENV === 'production' && !linkBehavior) { | ||
| Sentry.logger.warn('LinkBehaviorContext not found'); | ||
| } | ||
| const {component, behavior} = linkBehavior ?? defaultLinkBehavior; | ||
|
|
||
| return {Component: component, behavior: () => behavior(props)}; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| import {SentryLinkBehaviorProvider} from './link'; | ||
| import {SentryTrackingProvider} from './tracking'; | ||
|
|
||
| export function ScrapsProviders({children}: {children: React.ReactNode}) { | ||
| return ( | ||
| <SentryTrackingProvider> | ||
| <SentryLinkBehaviorProvider>{children}</SentryLinkBehaviorProvider> | ||
| </SentryTrackingProvider> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import {useMemo} from 'react'; | ||
| import {Link as RouterLink} from 'react-router-dom'; | ||
|
|
||
| import type {LinkProps} from '@sentry/scraps/link'; | ||
| import {LinkBehaviorContextProvider} from '@sentry/scraps/link/linkBehaviorContext'; | ||
|
|
||
| import {locationDescriptorToTo} from 'sentry/utils/reactRouter6Compat/location'; | ||
| import normalizeUrl from 'sentry/utils/url/normalizeUrl'; | ||
| import {useLocation} from 'sentry/utils/useLocation'; | ||
|
|
||
| export function SentryLinkBehaviorProvider({children}: {children: React.ReactNode}) { | ||
| const location = useLocation(); | ||
|
|
||
| return ( | ||
| <LinkBehaviorContextProvider | ||
| value={useMemo( | ||
| () => ({ | ||
| component: RouterLink, | ||
| behavior: ({to, ...props}: LinkProps) => { | ||
| const normalizedTo = locationDescriptorToTo(normalizeUrl(to, location)); | ||
|
|
||
| return { | ||
| to: normalizedTo, | ||
| ...props, | ||
| }; | ||
| }, | ||
| }), | ||
| [location] | ||
| )} | ||
| > | ||
| {children} | ||
| </LinkBehaviorContextProvider> | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import {SentryLinkBehaviorProvider} from 'sentry/scrapsProviders/link'; | ||
|
|
||
| export function ScrapsTestingProviders({children}: {children: React.ReactNode}) { | ||
| return <SentryLinkBehaviorProvider>{children}</SentryLinkBehaviorProvider>; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Inconsistent Sentry Wrapping Between Test and ProductionThe 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. |
||
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.
Bug:
SentryLinkBehaviorProvideruses an external context, butuseLinkBehavior()reads from a local context, bypassing custom link behavior.Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The
SentryLinkBehaviorProviderinstatic/app/scrapsProviders/link.tsxattempts to provide custom link behavior using an externalLinkBehaviorContextProviderfrom@sentry/scraps. However, theuseLinkBehavior()hook instatic/app/components/core/link/link.tsxreads from a different, localLinkBehaviorContext. This mismatch prevents the custombehaviorfunction, which includes URL normalization vianormalizeUrl()andlocationDescriptorToTo(), 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/scrapscontext, or updateSentryLinkBehaviorProviderto set values on the local context.🤖 Prompt for AI Agent
Did we get this right? 👍 / 👎 to inform future reviews.
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.
@sentry/scrapsis an alias forstatic/app/components/core/