Repository navigation
Name mobile tap spans only after labels from CONST.SENTRY_LABEL - #104054
Conversation
Sentry on iOS and Android named tap spans and breadcrumbs after accessibility labels, test IDs or the tapped text, which can contain user data. Read our sentryLabel prop instead and drop every label that is not in CONST.SENTRY_LABEL.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@abdulrahuman5196 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@MelvinBot review |
ReviewThe approach works, and I found no blocking issues. I traced it against the installed
All CI checks pass except the approval check. Non-blocking notes:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5dd07cc5a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| 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.
Each of the four new cases has a // Given and // Then but no literal // When, even though the testing rule incorporated by AGENTS.md (CLAUDE.md, Testing) requires all three sections for every test. Add a When comment at the actual touch(...) action in each case so future changes retain the scenario/action distinction.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
| extractTextFromChildren: false, | ||
| // Skip elements whose label is not in `CONST.SENTRY_LABEL` and keep looking at their parents. | ||
| // If no parent has a valid label, the tap is not reported. | ||
| ignoreNames: [UNKNOWN_LABEL], |
There was a problem hiding this comment.
On taps inside a component whose display name happens to equal a CONST.SENTRY_LABEL value, Sentry applies this same ignoreNames regex to the component name, so it retains the component even when no accepted sentryLabel was found. This already occurs for Socials (src/pages/signin/Socials.tsx:54): its actual prop at line 75 is dynamically suffixed and is rejected by UNKNOWN_LABEL, but the ancestor component name equals SENTRY_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.
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.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Answering to this
|
|
@abdulrahuman5196 this is ready for review, could you take a look please? |
|
@rlinoz could we please change reviewer? i would love to get that merged and be able to work on follow up PR already, and @abdulrahuman5196 doesn't seem to be active |
|
@arekm213 sorry for the delay. I was unavailable for last couple of days. But i will get it checked today. |
|
@arekm213 The approach looks good IMO, My only concern was the allowlist part, since it we have sentry data which is not present Const, then it would get dropped. But I saw your message in the issue here - #104022 (comment),
If we are aligned on this @rlinoz, I think we should be good. |
|
@arekm213 Kindly fix the merge conflicts. I could then check on platforms as well. |
|
@abdulrahuman5196 done! its ready for review again |
|
I think it is fine to drop the labels that are not in const, but I am worried about the ones we dynamically generate like |
|
@rlinoz Good call i added those, now label we build in code with a known prefix or suffix (search filters, the clear button on text inputs, social links, etc.) go through |
|
@MelvinBot review |
Review of the prefix/suffix changeThe new allowlist looks correct, and I found no blocking issues. All 3 prefixes and the Non-blocking notes:
What I checked
|
|
its ready for review, i would skip both non blocking notes from Melvin |
|
@abdulrahuman5196 can you please complete the checklist? |
|
checking now again |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiosHybrid-0-Screen_Recording_2026-11-02_at_8.31.18.PM.moviOS: mWeb SafariMacOS: Chrome / Safari |
abdulrahuman5196
left a comment
abdulrahuman5196
left a comment
There was a problem hiding this comment.
Changes looks good and works well. Reviewers checklist is also complete.
All yours. @rlinoz
🎀 👀 🎀
C+ Reviewed
|
Kindly re-run and watch out if the Reassure tests are passing, not sure why it is failing due to particular which could be unrelated. @rlinoz . I am unable to restart that test. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.5.28-4 🚀
|
|
No help site update needed. This PR only changes how Sentry names tap events on iOS and Android, with no change to the UI or user-facing behavior. |
|
Hi @arekm213. Step 2 can't be executed by QA team. 
|
|
I can do it internally |
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.5.30-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.5.30-0 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
On iOS and Android, Sentry names every tap span
<route>.<label>(opui.action.touch) and adds the same label totouchand rage tap breadcrumbs. The SDK takes the label from thesentry-labelprop, and when that is missing it falls back toaccessibilityLabel,aria-label,testIDand finally the text inside the tapped element. Our components set asentryLabelprop instead, which the SDK never reads on native, so tap spans end up named after whatever the user tapped: email addresses, full names, home addresses from the bank account form, report titles and amounts.This PR passes
touchEventBoundaryPropstoSentry.wrap:labelName: 'sentryLabel', so the SDK reads our existing labels (the same values web already sends asdata-sentry-label);extractTextFromChildren: false, so the text inside the tapped element is never read;ignoreNameswith a pattern that matches everything except known labels: the values inCONST.SENTRY_LABEL, and labels we build at runtime that start or end with a known prefix or suffix (Search-Filter-,Search-Advanced-Filter-,Search-FilterPopupApply-,Socials-,-ClearButton,-SkippedEmployees). The SDK skips every element whose component name or label matches it and keeps walking up the tree, so the accessibility label and test ID fallbacks can no longer name a tap.As a result, a tap is named after the nearest known label above the tapped element, for example
Report.LHN-OptionRow, or a label likeSearch-Filter-merchantfor a filter in the Spend filter bar. A tap with no such label creates no span and no breadcrumb. Because the label is chosen before the span starts, user data also stays out of everything built from the span name, such as the transaction name in trace headers and profiles.Trade-offs:
Search-Filter-reportField-project-code). An accessibility label or test ID that happens to have one of these formats would also get through.CONST.SENTRY_LABELare dropped. They can be moved intoCONST.SENTRY_LABELin a follow-up, which will also typesentryLabelso TypeScript checks these labels.Fixed Issues
$ #104022
PROPOSAL: N/A
Tests
npx jest tests/unit/touchEventBoundaryPropsTest.ts, and verify it passes.beforeBreadcrumb, verify everytouchbreadcrumb is labelled with a value fromCONST.SENTRY_LABEL(for exampleLHN-OptionRow,NavigationTabBar-Account,Search-TransactionListItem,Header-BackButton) or a label with a known prefix or suffix (for exampleSearch-Filter-merchantafter tapping a filter in the Spend filter bar), and that tapping an element without a label, such as "Schedule call" in a chat, logs no breadcrumb.Offline tests
Same as tests.
QA Steps
span.op:ui.action.touchand the build's release, and verify every name looks like<screen>.<label>, where the label is fromCONST.SENTRY_LABELor has a known prefix or suffix, with no names, emails, addresses or amounts.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
No visual changes.
Android: mWeb Chrome
No visual changes.
iOS: Native
No visual changes.
iOS: mWeb Safari
No visual changes.
MacOS: Chrome / Safari
No visual changes.