sentry-react-native icon indicating copy to clipboard operation
sentry-react-native copied to clipboard

Attach screenshot

Open bruno-garcia opened this issue 3 years ago • 3 comments

Add option attachScreenshot that when opt in, attached a screenshot on error events.

bruno-garcia avatar Jul 09 '22 16:07 bruno-garcia

It's not just adding the attachScreenshot flag.

On iOS, native hard crashes would work, but the JS errors (even if it crashes the app) won't contain the screenshot, because we call captureEnvelope or storeEnvelope and those functions won't attach the screenshot before sending it over to Sentry.

On Android, We always store the envelope on the disk, and the file observer will pick it up and enrich the event with device context, that's already too late and the view might not be up to date anymore, leading to a false positive. There are more 2 issues. The first issue is that the currentActivity is null because the Android SDK is inited after the very first Activity is created. https://github.com/getsentry/sentry-java/blob/121d465b31546c7d98af7b4b260dc120db902f79/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java#L70 If you put the app in the background and restore to the foreground, it'd work because the currentActivity won't be null anymore. The second issue is the attachScreenshot flag, if you enable it via the manifest, it works, but if you use the manual init as the RN does, the event processor is created before the user flips the attach screenshot flag and its always false, and it does not install the activity observer, https://github.com/getsentry/sentry-java/blob/121d465b31546c7d98af7b4b260dc120db902f79/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java#L50-L64 I've raised an issue for that.

I feel that https://github.com/getsentry/team-mobile/issues/11 should be addressed first with this issue in mind. cc @bruno-garcia

marandaneto avatar Jul 25 '22 08:07 marandaneto

If we're triggering the screenshot from JS through the bridge it's possible the view will change, no? So possibly not exactly what was on the screen at that time? It's fine if that's the case, just good to understand and document it.

I feel that https://github.com/getsentry/team-mobile/issues/11 should be addressed first with this issue in mind.

Not sure this one affects things as much. We can take care of screenshots on each layer: In JS if there's an error, we get a screenshot through the bridge and pass it down to the native layers in the envelope. It doesn't need to live in the JS-land either, we could do all of that in the bridge if that makes things easier. For the native errors we just pass the flag down so they add screenshots when they capture errors on those layers.

Would that work?

So we need to figure out:

  • Might require: #1327
  • https://github.com/getsentry/sentry-java/issues/2185

bruno-garcia avatar Jul 26 '22 01:07 bruno-garcia

If we're triggering the screenshot from JS through the bridge it's possible the view will change, no? So possibly not exactly what was on the screen at that time? It's fine if that's the case, just good to understand and document it.

If we await the calls, technically the view is the same because we are in the calling thread.

The first issue is that the currentActivity is null because the Android SDK is inited after the very first Activity is created.

This is still an issue, we'd need to find a way to set the currentActivity via the RN SDK, maybe the RN plugin should implement its own logic for screenshots, and the EventProcessor just exposes the methods to not duplicate the same logic, similar to Slow/frozen frames.

marandaneto avatar Jul 26 '22 05:07 marandaneto