feat(errortracking): ignoredExceptionTypes config to skip RN-duplicate JS errors#569
Open
tsushanth wants to merge 2 commits into
Open
feat(errortracking): ignoredExceptionTypes config to skip RN-duplicate JS errors#569tsushanth wants to merge 2 commits into
tsushanth wants to merge 2 commits into
Conversation
…e JS errors Closes PostHog#567. In React Native apps that enable native crash autocapture, a fatal JS error is captured twice: 1. The JS layer captures it via `uncaughtExceptions` autocapture (with the JS stack trace), and 2. React Native rethrows the same fatal JS error on the native side as `com.facebook.react.common.JavascriptException`. posthog-android's uncaught-exception handler then captures it as a second `$exception` event (with a native stack trace). `PostHogErrorTrackingConfig.ignoredExceptionTypes: MutableList<String>` suppresses this duplicate. Whenever the uncaught-exception handler fires, the throwable and every cause in its chain are checked against the list by class name — if any link matches, the SDK skips emitting `$exception` but still chains to the next handler (so the process termination / RN red-box / etc. behave the same way as before). The matching is purely class-name based; no `Class<*>` objects are loaded, so apps that don't have React Native (or any other optional dependency) on their classpath are not affected. Mirrors `sentry-android`'s `addIgnoredExceptionForType(...)`. Usage in an RN app: PostHogConfig(apiKey).apply { errorTrackingConfig.ignoredExceptionTypes.add( "com.facebook.react.common.JavascriptException", ) } Tests cover the three load-bearing cases: - direct match → skip capture, still chain - match anywhere in cause chain → skip capture, still chain - non-matching throwable with non-empty ignore list → still captures
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
posthog/src/main/java/com/posthog/errortracking/PostHogErrorTrackingAutoCaptureIntegration.kt:88-92
When the cause chain is what triggered the skip (not the direct throwable), this log reports the outer wrapper's class name — e.g. `java.lang.RuntimeException` — even though that class is not in `ignoredExceptionTypes`. A developer looking at these logs would wonder why `RuntimeException` was suppressed, since they only configured `com.facebook.react.common.JavascriptException`. Logging `throwable.javaClass.name` is only accurate when the outermost exception matched; the message should be phrased to cover the cause-chain case too.
```suggestion
} else {
config.logger.log(
"Skipping autocapture: ${throwable.javaClass.name} (or a cause in its chain) matches ignoredExceptionTypes",
)
}
```
### Issue 2 of 3
posthog/src/test/java/com/posthog/errortracking/PostHogErrorTrackingAutoCaptureIntegrationTest.kt:157-226
Prefer parameterised tests — the three new tests share identical setup (`remoteConfigHolder`, `isAutocaptureExceptionsEnabled`, `getDefaultUncaughtExceptionHandler`) and verify the same contract with different inputs. At minimum, the "skips capture" (direct match) and "still captures" (no match) cases are a natural `@ParameterizedTest` pair: the only axes that vary are the throwable type, the ignored-list contents, and whether `captureException` is expected to be called. Collapsing them reduces boilerplate and makes it immediately clear when a new edge case is added that it's intentionally distinct from existing ones. The cause-chain test is structurally different enough to stay standalone.
### Issue 3 of 3
posthog/src/test/java/com/posthog/errortracking/PostHogErrorTrackingAutoCaptureIntegrationTest.kt:228-234
The comment says "a local class named the same way", but the stub's actual JVM name is `com.posthog.errortracking.PostHogErrorTrackingAutoCaptureIntegrationTest$ReactNativeJavascriptExceptionStub` — nothing like `com.facebook.react.common.JavascriptException`. The tests work correctly because they pass `ReactNativeJavascriptExceptionStub::class.java.name` as the filter value, not the real RN FQCN. The comment should reflect the actual design: a stub with an arbitrary local name, registered under that same local name, so the name-based matching code path is exercised correctly without pulling in the RN dependency.
```suggestion
/**
* Local stand-in for `com.facebook.react.common.JavascriptException`. The real type
* lives in React Native, which isn't (and shouldn't be) a test dependency of the
* SDK. The ignored-exception filter is purely class-name based: tests register this
* stub's own JVM name as the ignored FQCN, which exercises the matching logic without
* requiring RN on the classpath. The stub name intentionally differs from the real RN
* class name; the code path is the same either way.
*/
private class ReactNativeJavascriptExceptionStub(message: String) : RuntimeException(message)
```
Reviews (1): Last reviewed commit: "feat(errortracking): ignoredExceptionTyp..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+88
to
+92
| } else { | ||
| config.logger.log( | ||
| "Skipping autocapture for ignored exception type: ${throwable.javaClass.name}", | ||
| ) | ||
| } |
There was a problem hiding this comment.
java.lang.RuntimeException — even though that class is not in ignoredExceptionTypes. A developer looking at these logs would wonder why RuntimeException was suppressed, since they only configured com.facebook.react.common.JavascriptException. Logging throwable.javaClass.name is only accurate when the outermost exception matched; the message should be phrased to cover the cause-chain case too.
Suggested change
| } else { | |
| config.logger.log( | |
| "Skipping autocapture for ignored exception type: ${throwable.javaClass.name}", | |
| ) | |
| } | |
| } else { | |
| config.logger.log( | |
| "Skipping autocapture: ${throwable.javaClass.name} (or a cause in its chain) matches ignoredExceptionTypes", | |
| ) | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/src/main/java/com/posthog/errortracking/PostHogErrorTrackingAutoCaptureIntegration.kt
Line: 88-92
Comment:
When the cause chain is what triggered the skip (not the direct throwable), this log reports the outer wrapper's class name — e.g. `java.lang.RuntimeException` — even though that class is not in `ignoredExceptionTypes`. A developer looking at these logs would wonder why `RuntimeException` was suppressed, since they only configured `com.facebook.react.common.JavascriptException`. Logging `throwable.javaClass.name` is only accurate when the outermost exception matched; the message should be phrased to cover the cause-chain case too.
```suggestion
} else {
config.logger.log(
"Skipping autocapture: ${throwable.javaClass.name} (or a cause in its chain) matches ignoredExceptionTypes",
)
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+157
to
+226
| fun `uncaughtException skips capture when throwable class is in ignoredExceptionTypes`() { | ||
| whenever(mockConfig.remoteConfigHolder).thenReturn(mockRemoteConfig) | ||
| whenever(mockRemoteConfig.isAutocaptureExceptionsEnabled()).thenReturn(true) | ||
| whenever(mockAdapter.getDefaultUncaughtExceptionHandler()).thenReturn(mockExceptionHandler) | ||
|
|
||
| val thread = Thread.currentThread() | ||
| // Simulates the React Native scenario: posthog-js has already captured the | ||
| // fatal JS error; React Native rethrows it natively as JavascriptException, | ||
| // and the SDK should not emit a duplicate $exception event. | ||
| val throwable = ReactNativeJavascriptExceptionStub("Unhandled JS Exception: ReferenceError") | ||
|
|
||
| val integration = | ||
| getSut(ignoredExceptionTypes = listOf(ReactNativeJavascriptExceptionStub::class.java.name)) | ||
| integration.install(mockPostHog) | ||
|
|
||
| integration.uncaughtException(thread, throwable) | ||
|
|
||
| verify(mockPostHog, never()).captureException(any<PostHogThrowable>(), anyOrNull()) | ||
| // The downstream handler still runs so the process terminates / RN's red-box | ||
| // appears as it would without PostHog installed. | ||
| verify(mockExceptionHandler).uncaughtException(thread, throwable) | ||
|
|
||
| integration.uninstall() | ||
| } | ||
|
|
||
| @Test | ||
| fun `uncaughtException skips capture when ignored class is anywhere in the cause chain`() { | ||
| whenever(mockConfig.remoteConfigHolder).thenReturn(mockRemoteConfig) | ||
| whenever(mockRemoteConfig.isAutocaptureExceptionsEnabled()).thenReturn(true) | ||
| whenever(mockAdapter.getDefaultUncaughtExceptionHandler()).thenReturn(mockExceptionHandler) | ||
|
|
||
| val thread = Thread.currentThread() | ||
| // The outermost type is RuntimeException, not the ignored type, but the | ||
| // cause chain contains the ignored type. Real RN apps wrap the JS exception | ||
| // inside platform-level wrappers, so walking the chain matters. | ||
| val inner = ReactNativeJavascriptExceptionStub("inner") | ||
| val outer = RuntimeException("outer", inner) | ||
|
|
||
| val integration = | ||
| getSut(ignoredExceptionTypes = listOf(ReactNativeJavascriptExceptionStub::class.java.name)) | ||
| integration.install(mockPostHog) | ||
|
|
||
| integration.uncaughtException(thread, outer) | ||
|
|
||
| verify(mockPostHog, never()).captureException(any<PostHogThrowable>(), anyOrNull()) | ||
| verify(mockExceptionHandler).uncaughtException(thread, outer) | ||
|
|
||
| integration.uninstall() | ||
| } | ||
|
|
||
| @Test | ||
| fun `uncaughtException still captures when throwable class is not in ignoredExceptionTypes`() { | ||
| whenever(mockConfig.remoteConfigHolder).thenReturn(mockRemoteConfig) | ||
| whenever(mockRemoteConfig.isAutocaptureExceptionsEnabled()).thenReturn(true) | ||
| whenever(mockAdapter.getDefaultUncaughtExceptionHandler()).thenReturn(mockExceptionHandler) | ||
|
|
||
| val thread = Thread.currentThread() | ||
| val throwable = RuntimeException("Genuine native crash") | ||
|
|
||
| val integration = | ||
| getSut(ignoredExceptionTypes = listOf("com.facebook.react.common.JavascriptException")) | ||
| integration.install(mockPostHog) | ||
|
|
||
| integration.uncaughtException(thread, throwable) | ||
|
|
||
| verify(mockPostHog).captureException(any<PostHogThrowable>(), anyOrNull()) | ||
| verify(mockExceptionHandler).uncaughtException(thread, throwable) | ||
|
|
||
| integration.uninstall() | ||
| } |
There was a problem hiding this comment.
remoteConfigHolder, isAutocaptureExceptionsEnabled, getDefaultUncaughtExceptionHandler) and verify the same contract with different inputs. At minimum, the "skips capture" (direct match) and "still captures" (no match) cases are a natural @ParameterizedTest pair: the only axes that vary are the throwable type, the ignored-list contents, and whether captureException is expected to be called. Collapsing them reduces boilerplate and makes it immediately clear when a new edge case is added that it's intentionally distinct from existing ones. The cause-chain test is structurally different enough to stay standalone.
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: posthog/src/test/java/com/posthog/errortracking/PostHogErrorTrackingAutoCaptureIntegrationTest.kt
Line: 157-226
Comment:
Prefer parameterised tests — the three new tests share identical setup (`remoteConfigHolder`, `isAutocaptureExceptionsEnabled`, `getDefaultUncaughtExceptionHandler`) and verify the same contract with different inputs. At minimum, the "skips capture" (direct match) and "still captures" (no match) cases are a natural `@ParameterizedTest` pair: the only axes that vary are the throwable type, the ignored-list contents, and whether `captureException` is expected to be called. Collapsing them reduces boilerplate and makes it immediately clear when a new edge case is added that it's intentionally distinct from existing ones. The cause-chain test is structurally different enough to stay standalone.
**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!
Comment on lines
+228
to
+234
| /** | ||
| * Local stand-in for `com.facebook.react.common.JavascriptException`. The real type | ||
| * lives in React Native, which isn't (and shouldn't be) a test dependency of the | ||
| * SDK. The ignored-exception filter is purely class-name based, so a local class | ||
| * named the same way exercises the same code path without dragging in RN. | ||
| */ | ||
| private class ReactNativeJavascriptExceptionStub(message: String) : RuntimeException(message) |
There was a problem hiding this comment.
com.posthog.errortracking.PostHogErrorTrackingAutoCaptureIntegrationTest$ReactNativeJavascriptExceptionStub — nothing like com.facebook.react.common.JavascriptException. The tests work correctly because they pass ReactNativeJavascriptExceptionStub::class.java.name as the filter value, not the real RN FQCN. The comment should reflect the actual design: a stub with an arbitrary local name, registered under that same local name, so the name-based matching code path is exercised correctly without pulling in the RN dependency.
Suggested change
| /** | |
| * Local stand-in for `com.facebook.react.common.JavascriptException`. The real type | |
| * lives in React Native, which isn't (and shouldn't be) a test dependency of the | |
| * SDK. The ignored-exception filter is purely class-name based, so a local class | |
| * named the same way exercises the same code path without dragging in RN. | |
| */ | |
| private class ReactNativeJavascriptExceptionStub(message: String) : RuntimeException(message) | |
| /** | |
| * Local stand-in for `com.facebook.react.common.JavascriptException`. The real type | |
| * lives in React Native, which isn't (and shouldn't be) a test dependency of the | |
| * SDK. The ignored-exception filter is purely class-name based: tests register this | |
| * stub's own JVM name as the ignored FQCN, which exercises the matching logic without | |
| * requiring RN on the classpath. The stub name intentionally differs from the real RN | |
| * class name; the code path is the same either way. | |
| */ | |
| private class ReactNativeJavascriptExceptionStub(message: String) : RuntimeException(message) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/src/test/java/com/posthog/errortracking/PostHogErrorTrackingAutoCaptureIntegrationTest.kt
Line: 228-234
Comment:
The comment says "a local class named the same way", but the stub's actual JVM name is `com.posthog.errortracking.PostHogErrorTrackingAutoCaptureIntegrationTest$ReactNativeJavascriptExceptionStub` — nothing like `com.facebook.react.common.JavascriptException`. The tests work correctly because they pass `ReactNativeJavascriptExceptionStub::class.java.name` as the filter value, not the real RN FQCN. The comment should reflect the actual design: a stub with an arbitrary local name, registered under that same local name, so the name-based matching code path is exercised correctly without pulling in the RN dependency.
```suggestion
/**
* Local stand-in for `com.facebook.react.common.JavascriptException`. The real type
* lives in React Native, which isn't (and shouldn't be) a test dependency of the
* SDK. The ignored-exception filter is purely class-name based: tests register this
* stub's own JVM name as the ignored FQCN, which exercises the matching logic without
* requiring RN on the classpath. The stub name intentionally differs from the real RN
* class name; the code path is the same either way.
*/
private class ReactNativeJavascriptExceptionStub(message: String) : RuntimeException(message)
```
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!
Address two of three Greptile review notes on PR PostHog#569: 1. The "skipping autocapture" log message previously read as if the throwable's outermost class was always the matched ignoredType. A cause-chain match would report e.g. `java.lang.RuntimeException` as suppressed even when only `JavascriptException` was in the ignore list. Reworded to surface that "the throwable or a cause in its chain" matched. 2. The `ReactNativeJavascriptExceptionStub` doc comment claimed it was "named the same way" as the real RN class — it isn't; the stub's JVM name lives under the test class. Restated the actual design: the stub's own FQCN is what tests register as the ignored value, exercising the same code path without needing RN on the classpath. Greptile's third note (parameterising the new tests) is intentionally deferred — the individual test names ("skips capture when…", "still captures when…", etc.) document distinct behaviours and read more clearly than a `@ParameterizedTest` table would. Happy to revisit if the maintainer prefers the table form.
Author
|
Thanks @greptile-apps — pushed
Issue 2 (parameterising the new tests): intentionally deferred. The individual test names — All 17 tests still pass after the changes. |
|
Both fixes look correct. Issue 1: The log now reads Issue 3: The stub doc comment correctly describes the design: tests register the stub's own JVM name as the ignored FQCN, exercising the name-based matching without pulling RN onto the classpath. Issue 2 (deferred): The rationale holds. These aren't symmetric variants — the cause-chain test threads LGTM on Tip: You can customize Greptile's behavior for this repo with |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #567.
In React Native apps that enable native crash autocapture, a fatal JS error is captured twice:
uncaughtExceptionsautocapture (with the JS stack trace), andcom.facebook.react.common.JavascriptException. posthog-android's uncaught-exception handler then captures it as a second$exceptionevent (with a native stack trace).The result is two
$exceptionevents for one logical error.API
New optional field on
PostHogErrorTrackingConfig:Setup in an RN app:
Whenever the uncaught-exception handler fires, the throwable and every cause in its chain are checked against the list by class name:
\$exception, but still chains to the next handler (so the process termination / RN red-box / etc. behave the same way as before).Mirrors
sentry-android'saddIgnoredExceptionForType(...).Implementation notes
Class<*>::isInstance. This is deliberate: matching by name means apps that don't have React Native (or any other optional dependency) on their classpath are not affected at startup or call time. The SDK never has to load or even know about RN's types — the consumer just passes the FQCN string.causeuntil either a match is found or a seen-set short-circuits a self-referential chain.defaultExceptionHandleris always invoked, ignored or not. The contract ofThread.UncaughtExceptionHandler(process termination, RN's red-box, etc.) shouldn't change based on whether we emit a\$exceptionevent.ignoredExceptionTypessee zero behavior difference.Tests
Three new tests in
PostHogErrorTrackingAutoCaptureIntegrationTest:uncaughtException skips capture when throwable class is in ignoredExceptionTypes— direct match.uncaughtException skips capture when ignored class is anywhere in the cause chain—RuntimeException("outer", cause = JsException). The outermost type is not in the list, but the chain match still suppresses.uncaughtException still captures when throwable class is not in ignoredExceptionTypes— guards against the strip widening to all throwables when the list is non-empty.The RN exception type is stubbed locally (
ReactNativeJavascriptExceptionStub) so the test doesn't drag React Native into the SDK's test classpath — the name-based check exercises the same code path either way.All 17 tests pass (14 existing + 3 new):
Test plan
./gradlew :posthog:test --tests "com.posthog.errortracking.*"— 17/17 pass./gradlew :posthog:compileDebugKotlin(via the test task) — cleanignoredExceptionTypes = [], all existing tests behave identically (no behavior change to the autocapture path).Sibling issue
A matching fix is needed on the iOS side per #653 (RN rethrows fatal JS as
RCTFatalException). Happy to ship that next as a separate PR if this shape is what you want — the iOS surface would bePostHogErrorTrackingConfig.ignoredExceptionNames: [String]with the same cause-chain walk.Notes for review
@marandaneto — you suggested
beforeSendas a "for now" workaround on the issue thread; this PR proposes the structural fix. Naming and scope are intentionally narrow: just the FQCN list, no Class-object overload, no auto-detection of RN. Happy to refactor if you'd rather keep the surface even smaller (e.g. hardcode the JavascriptException class internally and gate on an RN-detection bool) — let me know which shape you'd prefer.