Repository navigation
Name mobile tap spans only after labels from CONST.SENTRY_LABEL #104054
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
39d330a
b5dd07c
ae4bce7
a99ede2
e8edc62
ceb18cc
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,53 @@ | ||
| import CONST from '@src/CONST'; | ||
|
|
||
| import type {TouchEventBoundary} from '@sentry/react-native'; | ||
| import type {ComponentProps} from 'react'; | ||
|
|
||
| import escapeRegExp from 'lodash/escapeRegExp'; | ||
|
|
||
| type TouchEventBoundaryProps = ComponentProps<typeof TouchEventBoundary>; | ||
|
|
||
| /** Returns all label strings from the nested `CONST.SENTRY_LABEL` object. */ | ||
| function collectSentryLabels(value: unknown): string[] { | ||
| if (typeof value === 'string') { | ||
| return [value]; | ||
| } | ||
| if (typeof value !== 'object' || value === null) { | ||
| return []; | ||
| } | ||
| return Object.values(value).flatMap(collectSentryLabels); | ||
| } | ||
|
|
||
| /** Builds a regex alternation that matches exactly one of the given strings. */ | ||
| function oneOf(values: string[]): string { | ||
| return `(?:${values.map(escapeRegExp).join('|')})`; | ||
| } | ||
|
|
||
| /** | ||
| * Prefixes and suffixes of labels we build at runtime. The rest of the label isn't checked, so only add one here when | ||
| * the rest can't carry user data. Accessibility labels and test IDs in these formats get through too. | ||
| */ | ||
| const DYNAMIC_LABEL_PREFIXES = ['Search-Filter-', 'Search-Advanced-Filter-', 'Search-FilterPopupApply-', `${CONST.SENTRY_LABEL.SOCIALS.LINK}-`]; | ||
| const DYNAMIC_LABEL_SUFFIXES = ['-ClearButton', '-SkippedEmployees']; | ||
|
|
||
| /** Matches any string that is not a label from `CONST.SENTRY_LABEL` or a label built in one of the formats above. */ | ||
| const UNKNOWN_LABEL = new RegExp(`^(?!(?:${oneOf(collectSentryLabels(CONST.SENTRY_LABEL))}|${oneOf(DYNAMIC_LABEL_PREFIXES)}.+|.+${oneOf(DYNAMIC_LABEL_SUFFIXES)})$)`); | ||
|
|
||
| /** | ||
| * Sentry on mobile names each tap after a label found on the tapped element or its parents. By default it uses | ||
| * accessibility labels, test IDs or text, which can contain user data like names, emails and amounts. | ||
| * This config makes it use only labels from `CONST.SENTRY_LABEL` (or built in one of the formats above), same as on web. | ||
| */ | ||
| const touchEventBoundaryProps: TouchEventBoundaryProps = { | ||
| // Use our `sentryLabel` prop as the label. | ||
| labelName: 'sentryLabel', | ||
| // Don't use text as the label. | ||
| extractTextFromChildren: false, | ||
| // Skip elements whose label is not a known label and keep looking at their parents. | ||
| // If no parent has a valid label, the tap is not reported. | ||
| // Sentry checks component names against this too. It works because the views that carry `sentryLabel` have no | ||
| // component name, so adding Sentry's component annotation Babel plugin would make every label get skipped. | ||
| ignoreNames: [UNKNOWN_LABEL], | ||
| }; | ||
|
|
||
| export default touchEventBoundaryProps; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| /* eslint-disable @typescript-eslint/naming-convention, no-underscore-dangle -- Sentry's touch boundary keeps its internals in underscore-prefixed fields */ | ||
| import touchEventBoundaryProps from '@libs/telemetry/touchEventBoundaryProps'; | ||
|
|
||
| import CONST from '@src/CONST'; | ||
|
|
||
| import {TouchEventBoundary} from '@sentry/react-native'; | ||
|
|
||
| /** The part of a React fiber that Sentry's touch boundary reads while walking up from the touched view. */ | ||
| type Fiber = { | ||
| memoizedProps: Record<string, unknown>; | ||
| elementType: string | {displayName: string}; | ||
| return?: Fiber; | ||
| }; | ||
|
|
||
| type TouchPathItem = {name?: string; label?: string}; | ||
|
|
||
| /** Sentry's private methods that pick the label for a touch and log it as a breadcrumb; the span is named after the same label. */ | ||
| type TouchEventBoundaryInternals = { | ||
| _onTouchStart: (event: {_targetInst: Fiber}) => void; | ||
| _logTouchEvent: (path: TouchPathItem[], label?: string) => void; | ||
| }; | ||
|
|
||
| /** Props of a view, plus the `displayName` of its component when it is a named component rather than a native view. */ | ||
| type View = Record<string, unknown> & {displayName?: string}; | ||
|
|
||
| /** Builds the fiber chain from the touched view up to the root, the way React Native hands it to the boundary. */ | ||
| function buildFiberChain(...viewsFromTouchedUp: View[]): Fiber { | ||
| let fiber: Fiber | undefined; | ||
| for (const {displayName, ...props} of [...viewsFromTouchedUp].reverse()) { | ||
| fiber = {memoizedProps: props, elementType: displayName ? {displayName} : 'RCTView', return: fiber}; | ||
| } | ||
| if (!fiber) { | ||
| throw new Error('A fiber chain needs at least one view'); | ||
| } | ||
| return fiber; | ||
| } | ||
|
|
||
| /** Touches the view at the start of the chain and returns the label Sentry would name the span after, and the breadcrumb path. */ | ||
| function touch(...viewsFromTouchedUp: View[]) { | ||
| const boundary = new TouchEventBoundary({...TouchEventBoundary.defaultProps, ...touchEventBoundaryProps}); | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- the label is picked in private methods, so the test reaches them directly | ||
| const internals = boundary as unknown as TouchEventBoundaryInternals; | ||
| const logTouchEvent = jest.spyOn(internals, '_logTouchEvent'); | ||
|
|
||
| internals._onTouchStart({_targetInst: buildFiberChain(...viewsFromTouchedUp)}); | ||
|
|
||
| return {wasLogged: logTouchEvent.mock.calls.length > 0, path: logTouchEvent.mock.calls.at(0)?.at(0), label: logTouchEvent.mock.calls.at(0)?.at(1)}; | ||
| } | ||
|
|
||
| describe('touchEventBoundaryProps', () => { | ||
| it('names a tap after the nearest sentryLabel, skipping the user data in the views inside it', () => { | ||
|
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.
Each of the four new cases has a AGENTS.md reference: AGENTS.md:L3-L3 Useful? React with 👍 / 👎.
Contributor
Author
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. Done |
||
| // Given a list row whose label sits above views that carry a name, an email and a report ID | ||
| // When the user taps the email inside it | ||
| const {label, path} = touch( | ||
| {children: 'john.smith@example.com'}, | ||
| {accessibilityLabel: 'Navigates to a chat with John Smith', testID: 'report-screen-123'}, | ||
| {sentryLabel: CONST.SENTRY_LABEL.LHN.OPTION_ROW, accessibilityLabel: 'John Smith, $879.17'}, | ||
| ); | ||
|
|
||
| // Then the tap is named after our label and none of the user data reaches the span or the breadcrumb | ||
| expect(label).toBe(CONST.SENTRY_LABEL.LHN.OPTION_ROW); | ||
| expect(path).toEqual([{label: CONST.SENTRY_LABEL.LHN.OPTION_ROW}]); | ||
| }); | ||
|
|
||
| it('skips named components between the tapped view and its label', () => { | ||
| // Given a named component with user data in its accessibility label, inside a labelled pressable | ||
| // When the user taps the avatar | ||
| const {label, path} = touch({displayName: 'Avatar', accessibilityLabel: 'John Smith'}, {displayName: 'Text'}, {sentryLabel: CONST.SENTRY_LABEL.LHN.OPTION_ROW}); | ||
|
|
||
| // Then neither the component names nor the accessibility label end up in the span or the breadcrumb | ||
| expect(label).toBe(CONST.SENTRY_LABEL.LHN.OPTION_ROW); | ||
| expect(path).toEqual([{label: CONST.SENTRY_LABEL.LHN.OPTION_ROW}]); | ||
| }); | ||
|
|
||
| it('does not report a tap on an element without a sentryLabel', () => { | ||
| // Given a view that only has an accessibility label, a test ID and text with user data | ||
| // When the user taps it | ||
| const {wasLogged, label} = touch({children: 'B. Smith, London (Licensing Trip), $879.17'}, {accessibilityLabel: 'B. Smith', testID: 'transaction-item-row'}); | ||
|
|
||
| // Then no span is named after it and no breadcrumb is logged | ||
| expect(label).toBeUndefined(); | ||
| expect(wasLogged).toBe(false); | ||
| }); | ||
|
|
||
| it('does not report a sentryLabel that is not a known label', () => { | ||
| // Given a sentryLabel that is neither in CONST.SENTRY_LABEL nor built with a known prefix or suffix | ||
| // When the user taps the element with that label | ||
| const {wasLogged} = touch({sentryLabel: 'john.smith@example.com'}); | ||
|
|
||
| // Then it is treated like a missing label, so a label can never carry data that wasn't written in the code | ||
| expect(wasLogged).toBe(false); | ||
| }); | ||
|
|
||
| it.each([ | ||
| `${CONST.SENTRY_LABEL.LHN.OPTION_ROW}-ClearButton`, | ||
| 'Search-Filter-merchant', | ||
| 'Search-Filter-statusNot', | ||
| 'Search-Filter-reportField-project-code', | ||
| 'Search-Advanced-Filter-category', | ||
| 'Search-FilterPopupApply-tag', | ||
| `${CONST.SENTRY_LABEL.SOCIALS.LINK}-Twitter`, | ||
| 'DynamicHRSyncResultsPage-SkippedEmployees', | ||
| ])('reports a sentryLabel built at runtime in a known format: %s', (sentryLabel) => { | ||
| // Given a sentryLabel built from values in the code | ||
| // When the user taps the element with that label | ||
| const {label} = touch({sentryLabel}); | ||
|
|
||
| // Then the tap is named after it | ||
| expect(label).toBe(sentryLabel); | ||
| }); | ||
|
|
||
| it.each(['Search-Filter', 'ClearButton', 'Report-Search-Filter-tag', 'LHN-OptionRow-ClearButton-Icon'])( | ||
| 'does not report a sentryLabel that only contains a known prefix or suffix: %s', | ||
| (sentryLabel) => { | ||
| // Given a sentryLabel that doesn't start with a known prefix or end with a known suffix | ||
| // When the user taps the element with that label | ||
| const {wasLogged} = touch({sentryLabel}); | ||
|
|
||
| // Then the tap is not reported | ||
| expect(wasLogged).toBe(false); | ||
| }, | ||
| ); | ||
| }); | ||
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.
On taps inside a component whose display name happens to equal a
CONST.SENTRY_LABELvalue, Sentry applies this sameignoreNamesregex to the component name, so it retains the component even when no acceptedsentryLabelwas found. This already occurs forSocials(src/pages/signin/Socials.tsx:54): its actual prop at line 75 is dynamically suffixed and is rejected byUNKNOWN_LABEL, but the ancestor component name equalsSENTRY_LABEL.SOCIALS.LINK("Socials"), causing those taps to be logged under a misleading generic label instead of being dropped. Use a filter that cannot admit component names (or explicitly ignore these collisions) to preserve the promised no-label/no-event behavior.Useful? React with 👍 / 👎.
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.
Socials doesn't set a displayName, and Sentry only reads displayName (not the function name), so it never shows up as a name here. I also checked every displayName in src against CONST.SENTRY_LABEL and none of them match. Even if one did, it would only add a breadcrumb with that component name, with no span and no user data.