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

Add beforeScreenshot callback #3715

Merged
merged 7 commits into from
Apr 10, 2024

Conversation

doronpr
Copy link
Contributor

@doronpr doronpr commented Mar 26, 2024

to allow conditional screenshot attachment based on logic around event/hint

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

extend screenshot integration to allow conditional screenshot attachment based on logic around event/hint

💡 Motivation and Context

We at Wix, would like to attach screenshot in a more granular way.
This is critical in the context of PII (Personal Identifiable Information)

💚 How did you test it?

Locally, instead of sending attachScreenShot: true we simply extend the integration and add it.

📝 Checklist

  • I reviewed submitted code
  • [-] I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • All tests passing
  • No breaking changes

🔮 Next steps

github-tools-service and others added 2 commits March 26, 2024 11:13
to allow conditional screenshot attachment based on logic around event/hint
@krystofwoldrich krystofwoldrich changed the title extend screenshot integration to allow conditional screenshot attachment based on logic around event/hint Add beforeScreenshot callback Mar 26, 2024
@krystofwoldrich
Copy link
Member

Hi @doronpr,
thank you for the PR, this feature definitely makes sense.

We've already implemented it in some other SDKs, Dart and Android.

We also have an open issue in this repository

Could you update the PR to use beforeScreenshot option from the ReactNativeOptions?

Sentry.init({
  beforeScreenshot: (event, hint) => {
    return true;
  },
})

@doronpr
Copy link
Contributor Author

doronpr commented Mar 26, 2024

Hey @krystofwoldrich ok I can attempt to do that.
How would you recommend to get the options within the Integration?
I can use getCurrentHub().getClient() but I do see that it's deprecated.

@krystofwoldrich
Copy link
Member

Hey @krystofwoldrich ok I can attempt to do that.

Thank you.

I can use getCurrentHub().getClient() but I do see that it's deprecated.

Yes, it's deprecated, but you can use its replacement getClient from @sentry/core.

@doronpr
Copy link
Contributor Author

doronpr commented Mar 28, 2024

krystofwoldrich

It's ready for your review 😊

@doronpr
Copy link
Contributor Author

doronpr commented Apr 2, 2024

Hey @krystofwoldrich just making sure you didn't miss this and it's in your review queue?

@kahest
Copy link
Member

kahest commented Apr 2, 2024

Hey @doronpr thanks for the ping and your patience - the team was out on Friday and Monday due to public holidays, we'll take a look very soon!

@krystofwoldrich
Copy link
Member

Hi @doronpr, thank you, the code looks great.

Let's wait for CI to finish and then we can merge this.

@krystofwoldrich krystofwoldrich merged commit 5a22220 into getsentry:main Apr 10, 2024
78 of 80 checks passed
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.

4 participants