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.
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: Session replay masking preview for SwiftUI #4737
base: main
Are you sure you want to change the base?
feat: Session replay masking preview for SwiftUI #4737
Changes from 4 commits
b893e95
ff2ad50
5555827
afcb89e
efdbf25
87b1b46
ef26620
b402fe6
6a9b2ac
88fce2d
fd559e9
f0a3ba7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m
: What's the use case for passing different redactOptions than the ones fromSentrySDK.options?.sessionReplay
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s up to the user to decide how they will use this. They might try different configurations directly from the preview without changing the project options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK that makes sense. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h
: We can move this to the main Sentry package as it has no dependencies on SwiftUI. @brustolin I think we could also use this for non SwiftUI apps. We could port this so people can use this with an option for any iOS app while debugging. With that we could also keep theSentryViewPhotographer
internal.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, not sure if we should. It think session replay should have its own lib.
Moving this to main just change what needs to be public, because this would need to be public in order for SentrySwiftUI to use it.
Lets go step-by-step.
After finishing this PR for SwiftUI we can debate debugging it for UIKit that has no preview.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Step-By-Step.
What stopped me from approving the PR is that we make the
SentryViewPhotographer
public so we can use it here. If we could already agree that it makes sense to have a publicSentryReplayMaskPreviewUIView
that users can also use for UIKit, we could align on the API ofSentryReplayMaskPreviewUIView
and keepSentryViewPhotographer
. With that approach we cover more use cases and avoid making things public that aren't meant to be public. We can also discuss this on a call if we go back and forth.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h
: Why limit this functionality only to previews? If somebody wants to use it when running the app on a simulator, let them. I quickly tried it, and it works. Or am I missing something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be said for so many things.
I choose to make it harder for the user to forget this on the app by mistake.
We can discuss this approach, but so far we had zero request for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about allowing it and logging a warning when we see it's used elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m
: I'm a bit confused about why we need a protocol for this and two separate implementations:SentryRedactDefaultOptions
andPreviewRedactOptions
. Can't we ditch the protocol and have one public final class for this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO protocol is the best approach, it gives a lot of flexibility and separation of concerns.
We can make SentryReplayOptions implement SentryRedactOptionsSentry without the need to have another level in the replay options.
And we can have a simpler object for screenshot redact, and then another one for preview redact, maybe with more options in there specific for preview.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@philipphofmann will probably not agree with changing this to public, we should at least somehow mark this class as "internal use only"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should stop using hacky solutions and add a lot of "code smells" into the code.
If the user try to use an undocumented class, that is not part of the main product, let them try.