[NoQA] Tag injected third-party script errors in Sentry with third_party_code - #97223
[NoQA] Tag injected third-party script errors in Sentry with third_party_code#97223WojtekBoman wants to merge 13 commits into
third_party_code#97223Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
![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: ba2b5bae8d
ℹ️ 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".
WojtekBoman
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baff312401
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3bc8e5c0e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c984a0fdd6
ℹ️ 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".
…skewing injected-script diagnostics
…irdPartyErrorFilter
26780a3 to
e8b59f2
Compare
WojtekBoman
changed the title
third_party_code
|
@marufsharifi 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] |
bffd6e4 to
3fbc602
Compare
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
| const thirdPartyErrorFilterIntegration = isDevelopment() | ||
| ? undefined | ||
| : SentryReact.thirdPartyErrorFilterIntegration({ | ||
| filterKeys: [SENTRY_APPLICATION_KEY], | ||
| behaviour: 'apply-tag-if-exclusively-contains-third-party-frames', | ||
| }); |
There was a problem hiding this comment.
NAB:
The current classification has a few edge cases:
- Events without stack frames remain unclassified, so truly opaque rejections are still included in
third_party_code:false. - Errors thrown by third-party code through Sentry-wrapped browser APIs can contain an own-bundle
helpers.jsframe and be incorrectly treated as first-party. - If every frame lacks a filename or location, Sentry produces an empty frame-key list. Since
[].every()istrue, the event is incorrectly tagged as third-party.
The second case was reported in getsentry/sentry-javascript#20687 and fixed by getsentry/sentry-javascript#20802, released in JS SDK 10.54.0. We could upgrade to @sentry/react-native 8.14.2 / @sentry/core 10.57.0 and enable:
ignoreSentryInternalFrames: true,To catch them all, you could write a custom handler that would categorize them into three groups: stack_origin: first_party | third_party | unknown.
However, I’m not sure if this makes sense, so I’ll leave that up to you to decide. We could also add a comment noting that this solution has these edge cases.
There was a problem hiding this comment.
Good catches, all three are real. That said, I think the current change is sufficient for what we're
chasing here: all 313 APP-CN8 events have exactly two frames, both app:/// with line/col, coming
through auto.browser.global_handlers.onunhandledrejection, so there's no missing-frames case, no
sentryWrapped frame to mis-attribute, and nothing getting filtered out.
I agree we should enable ignoreSentryInternalFrames: true, I'll handle it as a followup as it requires upgrading @sentry It would increase the diff in this PR significantly. Thanks for the insightful review 🙇
| * plugin only runs for non-development builds, hence the guard: without a key in the bundle every frame | ||
| * looks foreign. | ||
| */ | ||
| const thirdPartyErrorFilterIntegration = isDevelopment() |
There was a problem hiding this comment.
The "stamp the key" and "enable the integration" guards are decoupled — worth tying them to one fact.
Whether the key is stamped and whether this integration is enabled are decided by two independent conditions:
- Enable (here, runtime):
isDevelopment()→Config.ENVIRONMENT === 'dev' - Stamp (build time):
config/rsbuild/rsbuild.common.ts:320→file !== '.env' && file !== '.env.development'
They stay consistent across normal builds (staging/prod/adhoc stamp and run non-dev; the dev server uses .env with ENVIRONMENT=dev), so there's no bug today. But the coupling is implicit.
The dangerous divergence is enabled + not stamped: with no stamped frame in the bundle, every app error looks exclusively third-party and gets mislabeled third_party_code: true — silently hiding real errors and defeating the feature.
const thirdPartyErrorFilterIntegration = isDevelopment()
? undefined
: SentryReact.thirdPartyErrorFilterIntegration({ ... });Could we derive both guards from the same "is this a stamped build" fact, or at least add a comment cross-referencing rsbuild.common.ts so a future change to one guard doesn't silently desync the other? Not blocking.
| filesToDeleteAfterUpload: './dist/**/*.map', | ||
| }, | ||
| // Stamps every chunk so the SDK can tell our frames from injected ones at runtime. | ||
| applicationKey: SENTRY_APPLICATION_KEY, |
There was a problem hiding this comment.
This is the build-time half of the guard pair (see my note on index.web.ts:34). The key is stamped whenever the plugin runs — gated on isDevelopment at line 320 — while the runtime integration is gated separately on isDevelopment().
They agree for all normal builds, but the two conditions are independent. A one-line comment here pointing at the runtime guard (and vice versa) would prevent a future edit to either from silently mislabeling every app error as third_party_code.
|
@MelvinBot Review |
ReviewThis looks solid and well-scoped. It swaps the earlier custom One dependency worth calling out — the correctness of this hinges on the two dev-gates never diverging:
The failure mode to avoid is: bundler doesn't stamp (dev) but runtime thinks it's non-dev → the integration sees every frame as foreign and mislabels all errors Other notes (non-blocking)
I didn't run the platform tests — the |
|
@marufsharifi I've already added a fix that should address your and Melvin comments :) |
5 participants
Explanation of Change
Errors thrown by injected code (consent tools, tag managers, browser extensions) were indistinguishable from our own in Sentry. This enables Sentry's
thirdPartyErrorFilterIntegrationon web:@sentry/webpack-pluginstamps every chunk with an application key, and events whose stack holds no stamped frame get taggedthird_party_code: true. The key is shared bysentryApplicationKey.tsbetween bundler config and app source. No-op in development and on native, where no key is stamped.Fixed Issues
$ #93837
PROPOSAL:
Tests
Test 1: third-party errors get tagged, own errors do not (non-development web build)
npm run build-staging(the integration is intentionally off undernpm run web, because@sentry/webpack-plugindoes not stamp the application key in development builds)npm run web:distand open the app in Chrome with DevTools openconst s = document.createElement('script'); s.src = 'data:text/javascript,setTimeout(() => { throw new Error("injected boom"); }, 0)'; document.body.appendChild(s);throw new Error('app boom')and press it)appproject and find both eventsthird_party_code: truethird_party_code: trueTest 2: development build and native are unaffected
npm run weband open the app in Chromethird_party_code: true, so nothing is mislabeled while no application key is stampedthird_party_codetag on native eventsOffline tests
N/A - no API calls, data fetching, or Onyx operations are added or changed. Sentry keeps its existing offline behavior (events are queued by the SDK and flushed when connectivity returns); this change only adds a tag to events that are already sent.
QA Steps
Same as tests, run against staging instead of a local build:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari