Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Screenshots #2610

Merged
merged 30 commits into from
Dec 2, 2022
Merged

feat: Screenshots #2610

merged 30 commits into from
Dec 2, 2022

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Nov 9, 2022

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

The Screenshot feature required changes in sentry-java and sentry-cocoa. The sentry-react-native uses the native SDKs methods to capture screenshots. Those are copied over the bridge and the envelope bytes including the screenshots are assembled on the JS/RN side and then passed back to the native layers to be sent.

In case of native crashes, native SDKs capture screenshots themselves.

The screenshots are captured in the client and not in the integration due to Async/Await JS nature. In the case of multiple async integrations, the screen could be different if captured in integration.

💡 Motivation and Context

Closes: #2373
Closes: #2362

💚 How did you test it?

Using sample app.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

🔮 Next steps

  • Merge the new sentry-jave to main rebase this PR and if CI passes merge.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 69b7454

@marandaneto
Copy link
Contributor

Consider #2362 (comment)
Only this is not enough.

@krystofwoldrich
Copy link
Member Author

Consider #2362 (comment) Only this is not enough.

I know I've read it.

src/js/client.ts Outdated Show resolved Hide resolved
@krystofwoldrich krystofwoldrich marked this pull request as ready for review November 21, 2022 12:21
@marandaneto
Copy link
Contributor

@krystofwoldrich is this ready for a final review os is there something missing?
The PR description is not fully filled.

@krystofwoldrich
Copy link
Member Author

@marandaneto Ready for final review, it only waits for sentry-java to be released. I talked to @romtsn and he would like to finish something and then release it (likely this Wednesday).

@marandaneto
Copy link
Contributor

@krystofwoldrich Since you are blocked by the Java release, we can draft up the docs PR for RN, similar to https://docs.sentry.io/platforms/flutter/enriching-events/screenshots/

@marandaneto
Copy link
Contributor

@krystofwoldrich can we unblock this? it should not be a problem cutting a patch release on Android.

@krystofwoldrich
Copy link
Member Author

@marandaneto The release is out. So this is not blocked anymore.

@marandaneto
Copy link
Contributor

marandaneto commented Nov 29, 2022

@marandaneto The release is out. So this is not blocked anymore.

I'd way wait this getsentry/publish#1614
The release 6.9.0 isn't fully finished, not sure if it's reliable yet.

@marandaneto
Copy link
Contributor

@marandaneto The release is out. So this is not blocked anymore.

I'd way wait this getsentry/publish#1614 The release 6.9.0 isn't fully finished, not sure if it's reliable yet.

@krystofwoldrich you can unblock this -> #2653

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attach screenshot
4 participants