feat(errortracking): add ignoredExceptionTypes config to skip RN-duplicate native crashes (closes #653)#666
Conversation
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
PostHogTests/PostHogErrorTrackingIgnoredTypesTest.swift:21-129
**Prefer parameterised tests**
The seven `@Test` functions that exercise `exceptionListMatchesIgnoredTypes` share an identical structure (properties, ignoredTypes, expected bool) and differ only in their input/output data. Swift Testing's `@Test(arguments:)` is made for exactly this pattern and is explicitly preferred by the team's review standards. `defaultIsEmpty` can stay as its own test since it covers a different subject.
### Issue 2 of 2
PostHog/ErrorTracking/PostHogErrorTrackingAutoCaptureIntegration.swift:187-198
**Verbose closure body can be simplified**
The `if-let / return true / return false` pattern is equivalent to a `guard` + direct return, which removes the superfluous parts per the team's simplicity rules and avoids shadowing the built-in `type(of:)` via the local `type` binding.
```suggestion
static func exceptionListMatchesIgnoredTypes(_ properties: [String: Any], ignoredTypes: [String]) -> Bool {
guard let exceptionList = properties["$exception_list"] as? [[String: Any]] else {
return false
}
let ignored = Set(ignoredTypes)
return exceptionList.contains { entry in
guard let exType = entry["type"] as? String else { return false }
return ignored.contains(exType)
}
}
```
Reviews (1): Last reviewed commit: "feat(errortracking): add ignoredExceptio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @Test("returns false when ignoredExceptionTypes is empty") | ||
| func emptyIgnoredListPassesAllExceptions() { | ||
| let properties: [String: Any] = [ | ||
| "$exception_list": [["type": "RCTFatalException", "value": "boom"]], | ||
| ] | ||
| #expect( | ||
| PostHogErrorTrackingAutoCaptureIntegration | ||
| .exceptionListMatchesIgnoredTypes(properties, ignoredTypes: []) == false | ||
| ) | ||
| } | ||
|
|
||
| @Test("matches when outer exception type is in ignored list") | ||
| func outerTypeMatchSuppresses() { | ||
| let properties: [String: Any] = [ | ||
| "$exception_list": [["type": "RCTFatalException", "value": "boom"]], | ||
| ] | ||
| #expect( | ||
| PostHogErrorTrackingAutoCaptureIntegration | ||
| .exceptionListMatchesIgnoredTypes( | ||
| properties, | ||
| ignoredTypes: ["RCTFatalException"] | ||
| ) == true | ||
| ) | ||
| } | ||
|
|
||
| @Test("matches when an underlying exception in the chain is in the ignored list") | ||
| func underlyingTypeMatchSuppresses() { | ||
| // PHPLCrashReportExceptionInfo doesn't currently expose | ||
| // `userInfo`, but for non-crash report paths the SDK builds a | ||
| // multi-entry `$exception_list` walking `NSUnderlyingErrorKey` | ||
| // (see `PostHogExceptionProcessor.buildExceptionList`). Make | ||
| // sure a wrapped RCTFatalException is still suppressed when | ||
| // it's not the outermost entry. | ||
| let properties: [String: Any] = [ | ||
| "$exception_list": [ | ||
| ["type": "NSException", "value": "wrapper"], | ||
| ["type": "RCTFatalException", "value": "boom"], | ||
| ], | ||
| ] | ||
| #expect( | ||
| PostHogErrorTrackingAutoCaptureIntegration | ||
| .exceptionListMatchesIgnoredTypes( | ||
| properties, | ||
| ignoredTypes: ["RCTFatalException"] | ||
| ) == true | ||
| ) | ||
| } | ||
|
|
||
| @Test("does not match exceptions whose type isn't in the ignored list") | ||
| func nonMatchPassesThrough() { | ||
| let properties: [String: Any] = [ | ||
| "$exception_list": [["type": "NSGenericException", "value": "boom"]], | ||
| ] | ||
| #expect( | ||
| PostHogErrorTrackingAutoCaptureIntegration | ||
| .exceptionListMatchesIgnoredTypes( | ||
| properties, | ||
| ignoredTypes: ["RCTFatalException"] | ||
| ) == false | ||
| ) | ||
| } | ||
|
|
||
| @Test("returns false when properties has no $exception_list key") | ||
| func missingExceptionListReturnsFalse() { | ||
| let properties: [String: Any] = [ | ||
| "$exception_level": "fatal", | ||
| ] | ||
| #expect( | ||
| PostHogErrorTrackingAutoCaptureIntegration | ||
| .exceptionListMatchesIgnoredTypes( | ||
| properties, | ||
| ignoredTypes: ["RCTFatalException"] | ||
| ) == false | ||
| ) | ||
| } | ||
|
|
||
| @Test("returns false when $exception_list is empty") | ||
| func emptyExceptionListReturnsFalse() { | ||
| let properties: [String: Any] = [ | ||
| "$exception_list": [[String: Any]](), | ||
| ] | ||
| #expect( | ||
| PostHogErrorTrackingAutoCaptureIntegration | ||
| .exceptionListMatchesIgnoredTypes( | ||
| properties, | ||
| ignoredTypes: ["RCTFatalException"] | ||
| ) == false | ||
| ) | ||
| } | ||
|
|
||
| @Test("match is exact and case-sensitive (NSException class names are stable identifiers)") | ||
| func caseSensitiveMatch() { | ||
| let properties: [String: Any] = [ | ||
| "$exception_list": [["type": "RCTFatalException", "value": "boom"]], | ||
| ] | ||
| #expect( | ||
| PostHogErrorTrackingAutoCaptureIntegration | ||
| .exceptionListMatchesIgnoredTypes( | ||
| properties, | ||
| ignoredTypes: ["rctfatalexception"] | ||
| ) == false | ||
| ) | ||
| } | ||
|
|
||
| @Test("config field default is empty so callers see no behavior change") | ||
| func defaultIsEmpty() { | ||
| let config = PostHogErrorTrackingConfig() | ||
| #expect(config.ignoredExceptionTypes.isEmpty) | ||
| } |
There was a problem hiding this comment.
The seven @Test functions that exercise exceptionListMatchesIgnoredTypes share an identical structure (properties, ignoredTypes, expected bool) and differ only in their input/output data. Swift Testing's @Test(arguments:) is made for exactly this pattern and is explicitly preferred by the team's review standards. defaultIsEmpty can stay as its own test since it covers a different subject.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: PostHogTests/PostHogErrorTrackingIgnoredTypesTest.swift
Line: 21-129
Comment:
**Prefer parameterised tests**
The seven `@Test` functions that exercise `exceptionListMatchesIgnoredTypes` share an identical structure (properties, ignoredTypes, expected bool) and differ only in their input/output data. Swift Testing's `@Test(arguments:)` is made for exactly this pattern and is explicitly preferred by the team's review standards. `defaultIsEmpty` can stay as its own test since it covers a different subject.
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Yes let's use parameterized tests where possible
|
Thank you @tsushanth, will have a look shortly. In the meantime can you please add changeset entry (see https://github.com/PostHog/posthog-ios/blob/92138fd8b48aa34a08ce54bdd366e4495d29d405/RELEASING.md) and run |
|
Thanks @ioannisj — added While here I also took greptile's second suggestion (9f961d5) — collapsed the Ready for another look whenever you have a moment. |
tsushanth
force-pushed
the
fix/error-tracking-ignored-exception-types
branch
from
9f961d5 to
11bfea9
Compare
|
Thanks for the patience @ioannisj. Rebased onto current |
|
@tsushanth taking a look now, apologies for the delay |
|
@tsushanth looks like it needs a fresh rebase since I can see reverting some changes on main (workflow, PR template etc) |
| /// See https://github.com/PostHog/posthog-ios/issues/653. | ||
| /// | ||
| /// Default: empty (no filtering). | ||
| @objc public var ignoredExceptionTypes: [String] = [] |
There was a problem hiding this comment.
Would it make sense providing a sensible default ["RCTFatalException"] here?
tsushanth
force-pushed
the
fix/error-tracking-ignored-exception-types
branch
from
11bfea9 to
d64a912
Compare
|
Thanks @ioannisj — pushed d64a9123c dropping the workaround commit that reverted the workflow / PR-template updates. The diff is now scoped to the five real files (errortracking config + integration, the new test, the changeset, and the |
Three asks from the 2026-06-18 review, all addressed: 1. Default `ignoredExceptionTypes` to `["RCTFatalException"]` so React Native apps get JS/native dedup out of the box. Native-only iOS apps never raise that type, so they're unaffected; the doc shows how to add more types or clear the default. (Mirrors what posthog-android's counterpart will default to.) 2. Honor `ignoredExceptionTypes` on the manual capture path too, not just the crash-report autocapture path. The filter now lives in the `captureExceptionEvent` funnel, which both `captureException(_:Error)` and `captureException(_:NSException)` flow through. 3. Parameterize the matcher tests via Swift Testing's `@Test(arguments:)` so adding a new shape is a single `MatchCase` row instead of another `@Test` block. Default-config check now asserts the new `["RCTFatalException"]` default. All 594 tests still pass (`make test`).
|
Thanks @ioannisj — sorry for the delay, addressed all three in 8b7d0a4: 1. Parameterized tests. Collapsed the seven hand-rolled cases into a single 2. Default to 3. Manual capture path. Threaded the same
|
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
@tsushanth look like builds are failing on non-ios platforms. Mind making sure the CI is green before approving? |
|
Fixed the watchOS/visionOS build failures: |
…icate native crashes (closes PostHog#653) React Native rethrows fatal JS errors via `RCTFatal(...)` as an uncaught `NSException` named `RCTFatalException`. When the `@posthog/react-native-plugin` enables native crash autocapture (`errorTracking.autocapture.nativeCrashes`), the iOS crash reporter captures that as a separate `$exception` event with a native stack trace — duplicating the event the JS layer has already captured with its own JS stack trace. The result is two `$exception` events for one logical error, with the duplicate carrying less useful context. sentry-react-native solves the equivalent on its iOS side with `addIgnoredExceptionForType(...)`; posthog-android added a parallel `errorTrackingConfig.ignoredExceptionTypes` filter (PostHog/posthog-android#569). This commit brings the same lever to posthog-ios so the JS-side event can be the single source of truth without forcing the React Native plugin to disable native crash autocapture entirely. Implementation: - PostHog/ErrorTracking/PostHogErrorTrackingConfig.swift: new `@objc public var ignoredExceptionTypes: [String] = []` documenting the RN use case and pointing at the parity rule in sentry-react-native + posthog-android. - PostHog/ErrorTracking/PostHogErrorTrackingAutoCaptureIntegration.swift: after `PostHogCrashReportProcessor.processReport(...)` runs and before `captureInternal("$exception", ...)`, walk the produced `$exception_list` for any `type` that matches an entry in `ignoredExceptionTypes` and short-circuit the capture if it does. Walks the full list (not just the outermost entry) so a wrapped RCTFatalException underneath an outer NSException wrapper is still suppressed. - PostHogTests/PostHogErrorTrackingIgnoredTypesTest.swift: new Swift Testing suite with 7 cases — empty list is a no-op, outer-type match suppresses, inner-chain match suppresses, mismatched types pass through, missing/empty `$exception_list` are safe, match is case-sensitive (NSException class names are stable identifiers), and the config field defaults to empty so the change is fully backwards-compatible. `swift build` clean. Could not run `swift test` to completion locally because the main branch's `PostHogSessionManagerTest` has pre-existing unrelated compile errors (`value of type 'PostHogSDK' has no member 'getSessionManager'`); CI should handle the test target correctly. Pairs with: PostHog/posthog-android#567 (the Android side of the same issue, fixed by PostHog/posthog-android#569).
…osure Drop the if-let/return true/return false pattern in favour of a guard + direct return, and rename the local binding from `type` to `exType` to avoid shadowing Swift's built-in `type(of:)`.
Three asks from the 2026-06-18 review, all addressed: 1. Default `ignoredExceptionTypes` to `["RCTFatalException"]` so React Native apps get JS/native dedup out of the box. Native-only iOS apps never raise that type, so they're unaffected; the doc shows how to add more types or clear the default. (Mirrors what posthog-android's counterpart will default to.) 2. Honor `ignoredExceptionTypes` on the manual capture path too, not just the crash-report autocapture path. The filter now lives in the `captureExceptionEvent` funnel, which both `captureException(_:Error)` and `captureException(_:NSException)` flow through. 3. Parameterize the matcher tests via Swift Testing's `@Test(arguments:)` so adding a new shape is a single `MatchCase` row instead of another `@Test` block. Default-config check now asserts the new `["RCTFatalException"]` default. All 594 tests still pass (`make test`).
The method was defined only inside the #if os(iOS) || os(macOS) || os(tvOS) block. The watchOS/visionOS stub class was missing it, causing a compile error when PostHogSDK.captureExceptionEvent calls it unconditionally. Added a no-op stub that returns false (crash reporting is unavailable on these platforms anyway).
ioannisj
force-pushed
the
fix/error-tracking-ignored-exception-types
branch
from
24fc9d9 to
29b0aee
Compare
2 participants
Why
Closes #653. Pairs with the Android side at PostHog/posthog-android#567 (fixed by PostHog/posthog-android#569).
React Native rethrows fatal JS errors via `RCTFatal(...)` as an uncaught `NSException` named `RCTFatalException`. When the `@posthog/react-native-plugin` enables native crash autocapture (`errorTracking.autocapture.nativeCrashes`), the iOS crash reporter captures that as a separate `$exception` event with a native stack trace — duplicating the event the JS layer already captured with its own (richer) JS stack trace. Result: two `$exception` events for one logical error, the duplicate carrying less useful context.
sentry-react-native solves the equivalent on its iOS side with `addIgnoredExceptionForType(...)`. posthog-android shipped a parallel `errorTrackingConfig.ignoredExceptionTypes` filter via PostHog/posthog-android#569. This PR brings the same lever to posthog-ios so callers can keep native crash autocapture on but dedup the RCTFatalException churn.
Fix
New config field
```swift
// PostHogErrorTrackingConfig.swift
@objc public var ignoredExceptionTypes: [String] = []
```
Caller-facing usage from the React Native plugin (or any consumer hitting this duplicate):
```swift
config.errorTrackingConfig.ignoredExceptionTypes = ["RCTFatalException"]
```
Filter site
In `PostHogErrorTrackingAutoCaptureIntegration.processPendingCrashReport()`, after `PostHogCrashReportProcessor.processReport(...)` produces the merged properties and before `captureInternal("$exception", ...)` runs:
```swift
let ignored = postHog.config.errorTrackingConfig.ignoredExceptionTypes
if !ignored.isEmpty, PostHogErrorTrackingAutoCaptureIntegration.exceptionListMatchesIgnoredTypes(finalProperties, ignoredTypes: ignored) {
hedgeLog("Crash report skipped: exception type is in errorTrackingConfig.ignoredExceptionTypes")
return
}
```
The helper walks the full `$exception_list` rather than only the outermost entry — a wrapped `RCTFatalException` underneath an outer `NSException` wrapper is still suppressed. Match is exact and case-sensitive (NSException class names are stable identifiers, not free text).
Tests
New `PostHogTests/PostHogErrorTrackingIgnoredTypesTest.swift` using the Swift Testing framework. Seven cases:
Plus a sanity check that the config field defaults to `[]`.
Backwards compatibility
Strict additive change:
Local verification